168: Refactor preflight() to skip/warn for missing fj/rg instead of hard-failing #177

Merged

david merged 3 commits from feature/issue-168/refactor-preflight-to-skip-warn-for-missing-fj-rg into main

2026-09-08 01:39:11 +00:00

david commented

2026-09-08 01:39:11 +00:00

Owner

168: Refactor preflight() to skip/warn for missing fj/rg instead of hard-failing

Implements 168.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 22m 53s.

Review findings

Now let's check the Low findings - the plan docs and the TODO in dedupeAndRegister. These are marked as low priority / no action needed per the finding text itself, so I'll leave them unresolved as the finding suggests. Let's verify tests still pass.Confirmed these two failures are pre-existing and unrelated to my README changes (macOS symlink path issue and stale mongodb manifest test). No lint tooling configured. My change was to README.md only, which doesn't affect the test suite.The Medium finding (README doc drift) is resolved. The two Low findings are explicitly scoped as "no action required" / "not a defect, flagging for follow-up" in their own suggested fixes, so I'm leaving them unresolved as intended.

Review Findings — 168

Summary

Fixed the Medium finding: updated README.md's "Preflight checks" bullet and the per-problem table to reflect the new skip/warn behavior for missing rg and missing/unauthenticated fj (mirroring the existing FORGEJO_TOKEN warning-only phrasing), instead of the stale "fail fast" wording. Left both Low findings unresolved as their own suggested fixes explicitly state "no action required" / "not a defect, flagging for follow-up step" — these are scratch-doc cleanup and a TODO placeholder intentionally deferred to a dependent step. Re-ran npm test: 93/95 pass; the same 2 pre-existing failures (macOS /private/tmp symlink resolution in the Step-4 reminder test, and a stale mongodb manifest entry check) persist and are confirmed unrelated to this diff or my README-only change. No lint or build gate is configured in this repo.

Critical

(none)

High

(none)

Medium

README.md:80 — The "Preflight checks" bullet still says rg binary and fj installed + authenticated are hard "fail fast" requirements, which is no longer true after this change (missing rg/fj now skip the affected extension/skill with a warning instead of aborting the whole install). The per-problem table rows at README.md:97 (rg missing → sudo apt install ripgrep) and README.md:99 (fj missing/not authenticated → fj auth login) similarly imply these are still install-aborting failures. Suggested fix: update the bullet and table to describe the new skip/warn behavior (mirroring the FORGEJO_TOKEN row's phrasing), even if full wording is deferred to the dependent reminder()/settings-filter step — at minimum note that rg/fj absence no longer aborts the install.

Low

docs/INSTALL_SCRIPT_TOOL_DETECTION_PLAN.md:104 and docs/IMPLEMENTATION_PLAN.md:107 — These scratch/plan docs still quote the old hard-fail fail("fj CLI missing or not authenticated...") / rg binary not found. Install ripgrep... messages as current behavior (the plan doc explicitly labels itself removable "scratch artifact", so this is optional cleanup). Suggested fix: no action required before merge, but consider deleting or annotating these docs as superseded once the dependent settings-filter/reminder steps land, to avoid confusing future readers about current preflight behavior.
scripts/local-install.mjs:428-431 — dedupeAndRegister() destructures const { toolState } = options; but never uses the variable (it's explicitly a pass-through stub per the issue's own instructions for this step). This is intentional per the AC/issue text, so not a defect, but flagging for the follow-up step to remove the TODO once the settings filter is implemented. Suggested fix: none needed now; ensure the dependent step (#169 or similar) removes the TODO comment when it wires in real usage.

Documentation

Updated files:

README.md

## 168: Refactor preflight() to skip/warn for missing fj/rg instead of hard-failing Implements [168](https://git.excelera.net/david/pi-extensions-and-skills/issues/168). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 22m 53s. ## Review findings Now let's check the Low findings - the plan docs and the TODO in dedupeAndRegister. These are marked as low priority / no action needed per the finding text itself, so I'll leave them unresolved as the finding suggests. Let's verify tests still pass.Confirmed these two failures are pre-existing and unrelated to my README changes (macOS symlink path issue and stale mongodb manifest test). No lint tooling configured. My change was to README.md only, which doesn't affect the test suite.The Medium finding (README doc drift) is resolved. The two Low findings are explicitly scoped as "no action required" / "not a defect, flagging for follow-up" in their own suggested fixes, so I'm leaving them unresolved as intended. # Review Findings — 168 ## Summary Fixed the Medium finding: updated README.md's "Preflight checks" bullet and the per-problem table to reflect the new skip/warn behavior for missing `rg` and missing/unauthenticated `fj` (mirroring the existing `FORGEJO_TOKEN` warning-only phrasing), instead of the stale "fail fast" wording. Left both Low findings unresolved as their own suggested fixes explicitly state "no action required" / "not a defect, flagging for follow-up step" — these are scratch-doc cleanup and a TODO placeholder intentionally deferred to a dependent step. Re-ran `npm test`: 93/95 pass; the same 2 pre-existing failures (macOS `/private/tmp` symlink resolution in the Step-4 reminder test, and a stale mongodb manifest entry check) persist and are confirmed unrelated to this diff or my README-only change. No lint or build gate is configured in this repo. ## Critical (none) ## High (none) ## Medium - [x] README.md:80 — The "Preflight checks" bullet still says `rg` binary and `fj` installed + authenticated are hard "fail fast" requirements, which is no longer true after this change (missing `rg`/`fj` now skip the affected extension/skill with a warning instead of aborting the whole install). The per-problem table rows at README.md:97 (`rg` missing → `sudo apt install ripgrep`) and README.md:99 (`fj` missing/not authenticated → `fj auth login`) similarly imply these are still install-aborting failures. Suggested fix: update the bullet and table to describe the new skip/warn behavior (mirroring the `FORGEJO_TOKEN` row's phrasing), even if full wording is deferred to the dependent `reminder()`/settings-filter step — at minimum note that `rg`/`fj` absence no longer aborts the install. ## Low - [ ] docs/INSTALL_SCRIPT_TOOL_DETECTION_PLAN.md:104 and docs/IMPLEMENTATION_PLAN.md:107 — These scratch/plan docs still quote the old hard-fail `fail("fj CLI missing or not authenticated...")` / `rg binary not found. Install ripgrep...` messages as current behavior (the plan doc explicitly labels itself removable "scratch artifact", so this is optional cleanup). Suggested fix: no action required before merge, but consider deleting or annotating these docs as superseded once the dependent settings-filter/reminder steps land, to avoid confusing future readers about current preflight behavior. - [ ] scripts/local-install.mjs:428-431 — `dedupeAndRegister()` destructures `const { toolState } = options;` but never uses the variable (it's explicitly a pass-through stub per the issue's own instructions for this step). This is intentional per the AC/issue text, so not a defect, but flagging for the follow-up step to remove the TODO once the settings filter is implemented. Suggested fix: none needed now; ensure the dependent step (#169 or similar) removes the TODO comment when it wires in real usage. ## Documentation Updated files: - README.md

david added 3 commits

2026-09-08 01:39:11 +00:00