issue-90: Implement pi registration with URL dedupe and reload reminder in scripts/local-install.mjs #98

Merged

david merged 3 commits from feature/issue-90/implement-pi-registration-with-url-dedupe-and-relo into main

2026-08-28 01:58:27 +00:00

david commented

2026-08-28 01:58:25 +00:00

Owner

90: Implement pi registration with URL dedupe and reload reminder in scripts/local-install.mjs

Implements 90.

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 33m 11s.

Review findings

The Stage 1 change is already committed (652e887); my remediation will land as working-tree changes. Let me read the test file.

Now I have the full picture. Let me check the project's gate commands before making changes.

Gate confirmed: npm test → node --test scripts/local-install.test.mjs. No build/lint scripts. Now I'll apply fixes in severity order — first the two Medium findings in scripts/local-install.mjs, then the Low docblock nit.

Medium 1 (BOM): Strip a leading UTF-8 BOM before JSON.parse so Node can read settings files that pi reads fine:

Now the test file — add the silent-failure shim variants and the new tests:

Now run the gates — npm test plus syntax checks on both modified files:

I double-escaped the backticks in the new RegExp lines — inside a template literal that terminates the string early. Let me inspect and fix:

Now re-run the gates:

All 43 tests pass (40 pre-existing + 3 new), syntax checks clean. Final verification of the working-tree diff:

All gates green. Remediation complete — here is the annotated findings document.

Review Findings — 90: Implement pi registration with URL dedupe and reload reminder in scripts/local-install.mjs

Summary

Fixed both medium robustness gaps and the low docblock nit: (1) dedupeAndRegister now strips a leading UTF-8 BOM before JSON.parse, so a BOM-prefixed settings file that pi can read is no longer silently skipped by dedupe (new test covers it); (2) both pi remove/pi install failure messages now use failureDetail(...) instead of combinedOutput(...), with new piRemoveSilentFail/piInstallSilentFail shim variants and two tests asserting the exit status 1 fallback; (3) the header docblock now notes the warn-only exceptions. Gates: test — npm test (node --test scripts/local-install.test.mjs) passed 43/43 (40 pre-existing + 3 new); build — no build script exists, node --check on both modified files passed; lint — none in repo. The two remaining low findings concern the already-committed commit message and branch name; remediating them requires ref mutations (git commit --amend, git branch -m) which this stage is prohibited from performing, so they are carried forward unresolved.

Critical

(empty)

High

(empty)

Medium

scripts/local-install.mjs:384 — The new warn-and-continue path can silently skip URL dedupe when Node's JSON.parse rejects a settings file that pi itself can still read (verified: a UTF-8 BOM prefix throws Unexpected token in Node's JSON.parse, while many tools strip it). In that case pi install <path> then succeeds and both the old URL entry and the new local-path entry stay registered — exactly the duplicate identity this feature exists to prevent. The previous behavior (abort with "Fix it and re-run") at least guaranteed dedupe was never silently skipped. Suggested fix: strip a leading BOM before parsing, e.g. JSON.parse(readFileSync(file, "utf-8").replace(/^\uFEFF/, "")), and/or after a successful pi install re-read the settings files and warn if an entry matching this repo's URL still remains.
scripts/local-install.mjs:414 — The pi remove/pi install failure messages embed combinedOutput(removed) / combinedOutput(installed) (line 425), which is empty when pi exits non-zero without writing anything, producing Aborted: \pi install …` failed:\n\nFix the pi error above…with no error "above".gitPull/npmInstallalready usefailureDetail(...)for exactly this reason and have a silent-failure test ("main reports the exit status when git pull fails without any output"), but the newpiRemoveFails/piInstallFailsshims (scripts/local-install.test.mjs:58-59) only model failing-with-stderr. Suggested fix: switch bothfail()calls tofailureDetail(removed)/failureDetail(installed), add silent variants (piRemoveSilentFail/piInstallSilentFail) to the shim, and a test asserting the exit status 1` fallback appears in the abort message.

Low

scripts/local-install.mjs:14 — The header docblock still states "Fail-fast: aborts with a non-zero exit and an actionable message on the first problem it hits", which this change (and the pre-existing FORGEJO_TOKEN warning) intentionally deviates from for non-fatal conditions. Suggested fix: reword to note the warn-and-continue exceptions, e.g. "except where a condition is warn-only (missing FORGEJO_TOKEN, unreadable pi settings file)".
commit 652e887 — Commit message 90: Implement pi registration… deviates from AGENTS.md's issue-driven format issue-<N>: <summary> (e.g. issue-42: add user auth). It does match the repo's existing bare-<N>: history, so this may be deliberate local practice — but as written it contradicts the documented standard. Suggested fix: amend to issue-90: implement pi registration with URL dedupe and reload reminder in scripts/local-install.mjs, or update AGENTS.md to codify the bare-number form.
branch feature/issue-90/implement-pi-registration-with-url-dedupe-and-relo — The kebab slug is truncated mid-word ("relo"); the full normalized title (…and-reload-reminder-in-scripts-local-install-mjs, ~106 chars) fits well within git's ref-length limit. Suggested fix: rename the branch to feature/issue-90/implement-pi-registration-with-url-dedupe-and-reload-reminder-in-scripts-local-install-mjs.

Documentation

Updated files:

docs/IMPLEMENTATION_PLAN.md

## 90: Implement pi registration with URL dedupe and reload reminder in scripts/local-install.mjs Implements [90](https://git.excelera.net/david/pi-extensions-and-skills/issues/90). > 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 33m 11s. ## Review findings The Stage 1 change is already committed (652e887); my remediation will land as working-tree changes. Let me read the test file. Now I have the full picture. Let me check the project's gate commands before making changes. Gate confirmed: `npm test` → `node --test scripts/local-install.test.mjs`. No build/lint scripts. Now I'll apply fixes in severity order — first the two Medium findings in `scripts/local-install.mjs`, then the Low docblock nit. **Medium 1 (BOM):** Strip a leading UTF-8 BOM before `JSON.parse` so Node can read settings files that pi reads fine: Now the test file — add the silent-failure shim variants and the new tests: Now run the gates — `npm test` plus syntax checks on both modified files: I double-escaped the backticks in the new RegExp lines — inside a template literal that terminates the string early. Let me inspect and fix: Now re-run the gates: All 43 tests pass (40 pre-existing + 3 new), syntax checks clean. Final verification of the working-tree diff: All gates green. Remediation complete — here is the annotated findings document. # Review Findings — 90: Implement pi registration with URL dedupe and reload reminder in scripts/local-install.mjs ## Summary Fixed both medium robustness gaps and the low docblock nit: (1) `dedupeAndRegister` now strips a leading UTF-8 BOM before `JSON.parse`, so a BOM-prefixed settings file that pi can read is no longer silently skipped by dedupe (new test covers it); (2) both `pi remove`/`pi install` failure messages now use `failureDetail(...)` instead of `combinedOutput(...)`, with new `piRemoveSilentFail`/`piInstallSilentFail` shim variants and two tests asserting the `exit status 1` fallback; (3) the header docblock now notes the warn-only exceptions. Gates: **test** — `npm test` (`node --test scripts/local-install.test.mjs`) passed **43/43** (40 pre-existing + 3 new); **build** — no build script exists, `node --check` on both modified files passed; **lint** — none in repo. The two remaining low findings concern the already-committed commit message and branch name; remediating them requires ref mutations (`git commit --amend`, `git branch -m`) which this stage is prohibited from performing, so they are carried forward unresolved. ## Critical (empty) ## High (empty) ## Medium - [x] scripts/local-install.mjs:384 — The new warn-and-continue path can silently skip URL dedupe when Node's `JSON.parse` rejects a settings file that pi itself can still read (verified: a UTF-8 BOM prefix throws `Unexpected token` in Node's `JSON.parse`, while many tools strip it). In that case `pi install <path>` then succeeds and both the old URL entry and the new local-path entry stay registered — exactly the duplicate identity this feature exists to prevent. The previous behavior (abort with "Fix it and re-run") at least guaranteed dedupe was never silently skipped. Suggested fix: strip a leading BOM before parsing, e.g. `JSON.parse(readFileSync(file, "utf-8").replace(/^\uFEFF/, ""))`, and/or after a successful `pi install` re-read the settings files and warn if an entry matching this repo's URL still remains. - [x] scripts/local-install.mjs:414 — The `pi remove`/`pi install` failure messages embed `combinedOutput(removed)` / `combinedOutput(installed)` (line 425), which is empty when pi exits non-zero without writing anything, producing `Aborted: \`pi install …\` failed:\n\nFix the pi error above…` with no error "above". `gitPull`/`npmInstall` already use `failureDetail(...)` for exactly this reason and have a silent-failure test ("main reports the exit status when git pull fails without any output"), but the new `piRemoveFails`/`piInstallFails` shims (scripts/local-install.test.mjs:58-59) only model failing-with-stderr. Suggested fix: switch both `fail()` calls to `failureDetail(removed)` / `failureDetail(installed)`, add silent variants (`piRemoveSilentFail`/`piInstallSilentFail`) to the shim, and a test asserting the `exit status 1` fallback appears in the abort message. ## Low - [x] scripts/local-install.mjs:14 — The header docblock still states "Fail-fast: aborts with a non-zero exit and an actionable message on the first problem it hits", which this change (and the pre-existing FORGEJO_TOKEN warning) intentionally deviates from for non-fatal conditions. Suggested fix: reword to note the warn-and-continue exceptions, e.g. "except where a condition is warn-only (missing FORGEJO_TOKEN, unreadable pi settings file)". - [ ] commit 652e887 — Commit message `90: Implement pi registration…` deviates from AGENTS.md's issue-driven format `issue-<N>: <summary>` (e.g. `issue-42: add user auth`). It does match the repo's existing bare-`<N>:` history, so this may be deliberate local practice — but as written it contradicts the documented standard. Suggested fix: amend to `issue-90: implement pi registration with URL dedupe and reload reminder in scripts/local-install.mjs`, or update AGENTS.md to codify the bare-number form. - [ ] branch feature/issue-90/implement-pi-registration-with-url-dedupe-and-relo — The kebab slug is truncated mid-word ("relo"); the full normalized title (`…and-reload-reminder-in-scripts-local-install-mjs`, ~106 chars) fits well within git's ref-length limit. Suggested fix: rename the branch to `feature/issue-90/implement-pi-registration-with-url-dedupe-and-reload-reminder-in-scripts-local-install-mjs`. ## Documentation Updated files: - docs/IMPLEMENTATION_PLAN.md

david added 3 commits

2026-08-28 01:58:26 +00:00