41: Add npm run local:install script to update and install all skills and extensions #44

Merged

david merged 3 commits from feature/issue-41/add-npm-run-local-install-script-to-update-and-ins into main

2026-08-18 13:17:59 +00:00

david commented

2026-08-18 13:17:58 +00:00

Owner

41: Add npm run local:install script to update and install all skills and extensions

Implements 41.

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 20m 20s.

Review findings

Node v22, no root lint/build configs, node_modules present. Now let me apply the fixes to scripts/local-install.mjs:Let me verify the edits landed correctly:Script changes are in. Now update package.json (test wiring) and the README (project-scope dedupe wording):Now let me rewrite the test file with coverage for the new behavior — orchestration tests (preflight failures, dedupeAndRegister, main) plus the new pure-helper cases:Now let me run the gates:All 30 tests pass. Let me check the full run for warnings and verify no stray output issues:Let me run the full gate set (npm test via the new script, plus a dry-run install to confirm deps resolve) and check the final diff:All gates green. One small polish — findMatchingPackageEntries's doc comment still says "global settings" but it now serves both scopes:Let me do a final review of the complete diff to confirm coherence:All fixes are applied, gates are green, and the working tree contains only the four expected files (no .pi-loop changes). Here is the annotated findings document.

Review Findings — 41

Summary

Remediated in a single pass. Fixed the destructive-dedupe edge case (substring fast-path replaced with an end-anchored boundary regex), the two parseGitRemote gaps (scp-like with port, scheme-less slash form), the .env inline-comment parsing, the Unix-only commandAvailable (added where on Windows — deliberately not the suggested --version probe, see Friction), the missing test script wiring, and extended dedupe to the project-scope settings file (pi remove -l). Added the previously-missing orchestration coverage: 13 new tests (30 total) exercise preflight failure ordering/messages, dedupeAndRegister against a temp settings.json with pi/git/fj/rg/npm shims on PATH, and the main happy path. Gates: node --check both files PASS, node --test scripts/local-install.test.mjs PASS (30/30), npm test PASS (30/30), npm install --dry-run PASS. The one unresolved finding is the commit-message amendment (c723e5e), which is not actionable in this stage because the pipeline forbids commit/history operations; it must be amended by a human before merge.

Critical

High

scripts/local-install.mjs:361 — The core behavior of the feature (preflight ordering and fail-fast messages, dedupeAndRegister reading a settings file and calling pi remove/pi install, main orchestration) has zero test coverage. local-install.test.mjs only tests the pure helpers; the riskiest part of the script (it mutates global ~/.pi/agent/settings.json, runs git pull, and npm install) is untested. Suggested fix: add tests that create a temp settings.json plus stub pi/git/fj shims on PATH (or injectable commands) and assert (a) dedupeAndRegister removes URL entries and registers the local path exactly once, and (b) preflight aborts with the expected message/order for each failure (dirty tree, non-main, missing FORGEJO_TOKEN, missing rg).
scripts/local-install.mjs:101 — source.includes(REPO_IDENTITY) is a substring match, so sourceMatchesRepo("https://git.excelera.net/david/pi-extensions-and-skills-extra") and …-backup both return true (verified). Because dedupeAndRegister then runs pi remove <source> on every match, an unrelated package whose URL/path merely contains this repo's identity as a prefix would be removed from the user's pi settings. Suggested fix: drop the substring fast-path and decide only from isThisRepo(parseGitRemote(source)), or constrain the fast-path with a boundary regex (identity followed by end-of-string / .git / @ref / #ref only).
scripts/local-install.mjs:57-77 — parseGitRemote mis-handles two valid git URL forms, so a real URL entry can be missed by the dedupe (leaving duplicate skills/extensions after registration): scp-like with a port (git@host:2222:owner/repo parses as owner: "2222:david") and the scheme-less slash form (git@git.excelera.net/david/pi-extensions-and-skills returns null, verified). Suggested fix: handle host:port in the scp-like branch and add the git@host/owner/repo form (or document the accepted forms and assert them in tests).

Low

AGENTS.md:19-22 — Commit c723e5e uses 41: Add npm run local:install … but AGENTS.md specifies issue-<N>: <summary> (repo history is mixed, but this change should follow the documented format). Suggested fix: amend the commit message to issue-41: Add npm run local:install script to update and install all skills and extensions.
scripts/local-install.mjs:117-135 — parseEnvFile keeps inline comments in values (DATABASE_URL=postgres://x # dev → "postgres://x # dev"), so preflight passes and the postgres extension fails later with a confusing error. Suggested fix: strip an unquoted trailing #… comment when parsing, and add a test for it.
scripts/local-install.mjs:187-192 — commandAvailable shells out to sh -c, which is Unix-only; pi itself is cross-platform (Windows users install via pi.cmd), so the pi/rg/fj preflight checks would fail there. Suggested fix: use spawnSync(cmd, ["--version"]) directly (no shell) or otherwise abstract the PATH check.
package.json:22-24 — The new test file has no wiring: there is no test script, so node --test scripts/local-install.test.mjs is not part of any automated gate. Suggested fix: add "test": "node --test scripts/local-install.test.mjs" to scripts.
scripts/local-install.mjs:315-321 — Dedupe reads only global ~/.pi/agent/settings.json; if the repo was ever installed project-locally (pi install -l → <project>/.pi/settings.json), that stale URL entry is not removed, contradicting the README claim "any existing settings entry that references this repo's URL … is removed". Suggested fix: also scan the project-scope settings file (or soften the README wording to "global settings").

Documentation

Updated files:

README.md
docs/IMPLEMENTATION_PLAN.md

## 41: Add npm run local:install script to update and install all skills and extensions Implements [41](https://git.excelera.net/david/pi-extensions-and-skills/issues/41). > 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 20m 20s. ## Review findings Node v22, no root lint/build configs, `node_modules` present. Now let me apply the fixes to `scripts/local-install.mjs`:Let me verify the edits landed correctly:Script changes are in. Now update `package.json` (test wiring) and the README (project-scope dedupe wording):Now let me rewrite the test file with coverage for the new behavior — orchestration tests (preflight failures, `dedupeAndRegister`, `main`) plus the new pure-helper cases:Now let me run the gates:All 30 tests pass. Let me check the full run for warnings and verify no stray output issues:Let me run the full gate set (`npm test` via the new script, plus a dry-run install to confirm deps resolve) and check the final diff:All gates green. One small polish — `findMatchingPackageEntries`'s doc comment still says "global settings" but it now serves both scopes:Let me do a final review of the complete diff to confirm coherence:All fixes are applied, gates are green, and the working tree contains only the four expected files (no `.pi-loop` changes). Here is the annotated findings document. # Review Findings — 41 ## Summary Remediated in a single pass. Fixed the destructive-dedupe edge case (substring fast-path replaced with an end-anchored boundary regex), the two `parseGitRemote` gaps (scp-like with port, scheme-less slash form), the `.env` inline-comment parsing, the Unix-only `commandAvailable` (added `where` on Windows — deliberately not the suggested `--version` probe, see Friction), the missing `test` script wiring, and extended dedupe to the project-scope settings file (`pi remove -l`). Added the previously-missing orchestration coverage: 13 new tests (30 total) exercise `preflight` failure ordering/messages, `dedupeAndRegister` against a temp `settings.json` with `pi`/`git`/`fj`/`rg`/`npm` shims on PATH, and the `main` happy path. Gates: `node --check` both files **PASS**, `node --test scripts/local-install.test.mjs` **PASS (30/30)**, `npm test` **PASS (30/30)**, `npm install --dry-run` **PASS**. The one unresolved finding is the commit-message amendment (`c723e5e`), which is not actionable in this stage because the pipeline forbids commit/history operations; it must be amended by a human before merge. ## Critical ## High - [x] scripts/local-install.mjs:361 — The core behavior of the feature (preflight ordering and fail-fast messages, `dedupeAndRegister` reading a settings file and calling `pi remove`/`pi install`, `main` orchestration) has **zero test coverage**. `local-install.test.mjs` only tests the pure helpers; the riskiest part of the script (it mutates global `~/.pi/agent/settings.json`, runs `git pull`, and `npm install`) is untested. Suggested fix: add tests that create a temp `settings.json` plus stub `pi`/`git`/`fj` shims on PATH (or injectable commands) and assert (a) `dedupeAndRegister` removes URL entries and registers the local path exactly once, and (b) `preflight` aborts with the expected message/order for each failure (dirty tree, non-main, missing `FORGEJO_TOKEN`, missing `rg`). - [x] scripts/local-install.mjs:101 — `source.includes(REPO_IDENTITY)` is a substring match, so `sourceMatchesRepo("https://git.excelera.net/david/pi-extensions-and-skills-extra")` and `…-backup` both return `true` (verified). Because `dedupeAndRegister` then runs `pi remove <source>` on every match, an unrelated package whose URL/path merely contains this repo's identity as a prefix would be **removed from the user's pi settings**. Suggested fix: drop the substring fast-path and decide only from `isThisRepo(parseGitRemote(source))`, or constrain the fast-path with a boundary regex (identity followed by end-of-string / `.git` / `@ref` / `#ref` only). - [x] scripts/local-install.mjs:57-77 — `parseGitRemote` mis-handles two valid git URL forms, so a real URL entry can be missed by the dedupe (leaving duplicate skills/extensions after registration): scp-like with a port (`git@host:2222:owner/repo` parses as `owner: "2222:david"`) and the scheme-less slash form (`git@git.excelera.net/david/pi-extensions-and-skills` returns `null`, verified). Suggested fix: handle `host:port` in the scp-like branch and add the `git@host/owner/repo` form (or document the accepted forms and assert them in tests). ## Low - [ ] AGENTS.md:19-22 — Commit `c723e5e` uses `41: Add npm run local:install …` but AGENTS.md specifies `issue-<N>: <summary>` (repo history is mixed, but this change should follow the documented format). Suggested fix: amend the commit message to `issue-41: Add npm run local:install script to update and install all skills and extensions`. - [x] scripts/local-install.mjs:117-135 — `parseEnvFile` keeps inline comments in values (`DATABASE_URL=postgres://x # dev` → `"postgres://x # dev"`), so preflight passes and the postgres extension fails later with a confusing error. Suggested fix: strip an unquoted trailing ` #…` comment when parsing, and add a test for it. - [x] scripts/local-install.mjs:187-192 — `commandAvailable` shells out to `sh -c`, which is Unix-only; pi itself is cross-platform (Windows users install via `pi.cmd`), so the `pi`/`rg`/`fj` preflight checks would fail there. Suggested fix: use `spawnSync(cmd, ["--version"])` directly (no shell) or otherwise abstract the PATH check. - [x] package.json:22-24 — The new test file has no wiring: there is no `test` script, so `node --test scripts/local-install.test.mjs` is not part of any automated gate. Suggested fix: add `"test": "node --test scripts/local-install.test.mjs"` to `scripts`. - [x] scripts/local-install.mjs:315-321 — Dedupe reads only global `~/.pi/agent/settings.json`; if the repo was ever installed project-locally (`pi install -l` → `<project>/.pi/settings.json`), that stale URL entry is not removed, contradicting the README claim "any existing settings entry that references this repo's URL … is removed". Suggested fix: also scan the project-scope settings file (or soften the README wording to "global settings"). ## Documentation Updated files: - README.md - docs/IMPLEMENTATION_PLAN.md

david added 3 commits

2026-08-18 13:17:58 +00:00