170: Wire settings.json filter rewrite into dedupeAndRegister() #179

Merged

david merged 2 commits from feature/issue-170/wire-settings-json-filter-rewrite-into-dedupeandre into main

2026-09-08 02:38:38 +00:00

david commented

2026-09-08 02:38:37 +00:00

Owner

170: Wire settings.json filter rewrite into dedupeAndRegister()

Implements 170.

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 37m 52s.

Review findings

Now let's implement the critical fix for path matching, verified against pi's actual normalizePackageSourceForSettings/resolvePath behavior.Now let's fix the test file per High findings — correct the misleading comment and add a relative-path test, plus fix Medium/Low findings.Now update the misleading comment and add a relative-path test (High findings):Now add a new test that seeds a relative path (mirroring real pi install output) right after the combined-filters test, plus an integration-level test per High finding #2.Need relative imported in the test file.Now let's add the integration-level test (High #2) — extend the shim's pi install to actually write settings.json like real pi, and test main() end-to-end.Good, that edit already succeeded. Now I need to create the writer script and wire the env var.Let me simplify — avoid mutating global process.env, instead reference the writer via the shim's own directory.Let's rewrite this more cleanly using a fixed script path relative to dir instead of mutating process.env, avoiding side effects that leak across tests.Now update the docstring above it and wire the returned path into the pi shim invocation without env var:That's fine, docstring update is minor. Now update the pi shim body to use the returned path via a shell variable, and update piInstallBody construction to happen after we have the writer path.Now update the docstring for writePiInstallRelativeWriter regarding env var usage — it still references $PI_SETTINGS_FILE_FOR_SHIM inside the generated script, which is fine, but we need to actually set that env var when invoking dedupeAndRegister/main in the new test.Good — the PI_SETTINGS_FILE_FOR_SHIM env var is used by the generated writer script and needs to be set by test callers to the globalSettingsFile path. Now add the new integration test using this.Now let's add a new integration test right after this one that uses piInstallWritesRelativeSettingsEntry: true and lets main() really apply filters end-to-end.Now let's run the tests.Found it — when withRg: false, PATH is restricted to shimDir alone, so node isn't resolvable for the writer script invocation.Our new test now passes. The remaining 2 failures are the pre-existing, unrelated failures noted in the review findings summary (macOS /private tmpdir symlink and stale mongodb manifest test). Let's verify these existed before our changes too.Confirmed pre-existing failures, unrelated to this diff. Now let's address the remaining findings: Medium (2) and Low (2).Medium finding 1: reminder() TODO stale comment.Now let's look at the reminder() TODO comment itself:This is fine now with a single forward reference from buildPackageFilters doc → this TODO, no more circularity. Now Medium finding 2: spread existing object entry before overriding source/filters.Now Low finding 1: extract a shared FULLY_AVAILABLE_TOOL_STATE constant.Let's add the constant in local-install.mjs, export it, and use it in main + this test.Now update the test to import and use this constant where used for "toolState is fully available":Now let's find and update the second-run part of the "re-derives filter fresh" test which used { rgOk: true, fjState: "ok" } too.I'll leave lines 727 and 806 (different meanings — those are the raw preflight/buildPackageFilters return values, not the "fully available default" literal duplication mentioned in the finding) but update line 1011 which matches.Now Low finding 2: split/add a coverage test for excluded→excluded-with-different-toolState transition.Now run the full test suite again.All 111 relevant tests pass; only the 2 known pre-existing unrelated failures remain. Let's also check for the extensions/learn-repo test suite that's part of npm test.Good, learn-repo tests pass fine too. Let's check the diff summary and confirm no lint/build scripts exist beyond install:local/test.No lint/build scripts. Confirmed only test gate exists; we already ran it. Let's do a final review of the diff.This looks good. Let's double check the constant placement (it's before USER_GIT_DIR/SETTINGS_FILE which is a bit odd stylistically but functionally fine).Let's move it after SETTINGS_FILE to keep the AGENT_DIR-derived group together, for cleanliness.Still just the 2 pre-existing failures. Let's confirm the final overall count.All findings addressed. Let's view the final diff for sanity.This is a clean, minimal set of fixes. All findings addressed except none deliberately left unresolved — let's double check every finding was actually handled.All 7 findings (1 Critical, 2 High, 2 Medium, 2 Low) have been addressed. Final verification run:Both remaining failures are the pre-existing, unrelated ones confirmed earlier (identical on the parent commit before Stage 1's diff).

Review Findings — 170

Summary

Fixed the Critical bug: rewritePackageEntry() now resolves each candidate settings-entry source against the settings file's own directory before comparing to absRepoPath, matching real pi install's relative-path normalization (verified against the installed pi v0.85.1 binary and its normalizePackageSourceForSettings/resolvePath source). Added a unit test that seeds a real-pi-style relative path and an end-to-end main() integration test using a pi shim that writes settings.json the way real pi install does, closing the blind spot that let the bug ship. Corrected the misleading test comment about what pi install writes. Fixed the Medium findings: tidied the now-single-direction TODO cross-reference, and made rewritePackageEntry spread any existing object entry's properties before overriding source/filters. Fixed both Low findings: extracted a shared FULLY_AVAILABLE_TOOL_STATE constant used by both dedupeAndRegister's default and relevant tests, and added a test pinning the excluded→excluded-with-different-toolState re-derivation case. npm test (the only test/build gate in this repo — no lint or build script exists) passes 111/113, with the 2 failures reproducing identically on the parent commit and unrelated to this diff (macOS /private tmpdir symlink quirk in a direct-invocation test, and a stale mongodb-extension assertion in the manifest test).

Critical

scripts/local-install.mjs:555-558 — rewritePackageEntry()'s findIndex matches package entries by exact string equality against absRepoPath, but the real pi install <absPath> command normalizes and writes a path relative to the agent settings directory (verified against the installed pi v0.85.1: pi install /tmp/foo/myrepo with HOME=/tmp/foo produces "packages": ["../../../myrepo"], never the absolute path). Consequently dedupeAndRegister() always hits the index === -1 branch and prints "could not find the just-installed package entry" in real usage — the filter rewrite (the feature this issue exists to wire in) never actually applies outside the test shims. Reproduced end-to-end with the real pi binary against this exact branch's checkout. Suggested fix: after pi install absRepoPath succeeds, re-read the settings file and match entries using the same relative-path resolution pi itself uses (e.g. resolve each candidate source — string or pkg.source — against dirname(file) and compare to absRepoPath, mirroring findMatchingPackageEntries/sourceMatchesRepo's existing local-path handling, or reuse pi's own isLocalPath/relative-path convention). Add at least one test that seeds the settings file with a relative path (as real pi install produces) rather than only the absolute path, and add an end-to-end smoke test that shells out to the real pi binary if available in CI.

High

scripts/local-install.test.mjs:811-812 — All 10 new dedupeAndRegister filter-rewrite tests seed globalSettings with resolve(cwd) (the absolute path) with a comment claiming this simulates "the entry pi install would have written," but this is factually incorrect for real pi (see Critical finding above) — the comment and the tests give false confidence that the wiring works. Suggested fix: correct the comment to acknowledge the shim limitation, and add a test using a settings file seeded with a relative path (as real pi produces) to catch the matching bug that these tests currently mask.
scripts/local-install.mjs:519-520 — When toolState is fully derived from preflight() in main(), buildPackageFilters is always invoked even when nothing needs filtering (returns null, handled fine), but there is no test exercising the full main() → dedupeAndRegister() → filter-rewrite pipeline end-to-end (only unit-level dedupeAndRegister calls with directly-supplied toolState). Given the Critical bug above went undetected specifically because of this gap, add an integration-level test that runs main() with a shimmed pi that actually mutates settings.json the way real pi install does (relative path), to close this blind spot for future changes.

Medium

scripts/local-install.mjs:578-580 — The reminder(toolState) TODO comment ("consumed by the dynamic reminder rewrite … referenced by the TODO below") is stale scope-tracking prose left over from before this issue was wired in; it doesn't reference "the TODO below" (there isn't one after it) and slightly conflicts with the updated buildPackageFilters doc comment which says "referenced by the TODO below" pointing at this exact comment. Suggested fix: tidy the comment so it doesn't describe itself as "below" from two different vantage points — e.g. simplify to a single forward-reference from buildPackageFilters() to this TODO without the redundant self-reference here.
scripts/local-install.mjs:533-576 — rewritePackageEntry() silently overwrites packages[index] with { source: absRepoPath, ...filters }, discarding any other properties the existing entry object might have had (e.g. a hypothetical autoload flag or future fields) if the entry was previously an object rather than a string. Since the only entries this function will ever see immediately after pi install are strings today, this is low risk currently, but is a latent data-loss trap if pi's install format changes. Suggested fix: spread the existing object entry (when it is one) before overriding source/extensions/skills, e.g. packages[index] = { ...(typeof packages[index] === "object" ? packages[index] : {}), source: absRepoPath, ...filters }.

Low

scripts/local-install.mjs:519 — The default toolState = { rgOk: true, fjState: "ok" } used when options.toolState is omitted duplicates the same literal used in one of the new tests (test title "dedupeAndRegister leaves the entry a plain string when toolState is omitted") — fine functionally, but consider extracting a shared FULLY_AVAILABLE_TOOL_STATE constant so the "fully available" default is defined once and can't drift from buildPackageFilters's no-op condition.
scripts/local-install.test.mjs:988-1029 — The "re-derives the filter fresh on each call" test is a good scenario, but its name and comment focus on "dropping a stale exclusion" without also asserting the inverse (going from no-filter to filtered) in the same test; consider splitting or adding a short one-liner test for excluded→excluded-with-different-toolState to fully pin re-derivation behavior (minor test-coverage polish, not blocking).

## 170: Wire settings.json filter rewrite into dedupeAndRegister() Implements [170](https://git.excelera.net/david/pi-extensions-and-skills/issues/170). > 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 37m 52s. ## Review findings Now let's implement the critical fix for path matching, verified against pi's actual `normalizePackageSourceForSettings`/`resolvePath` behavior.Now let's fix the test file per High findings — correct the misleading comment and add a relative-path test, plus fix Medium/Low findings.Now update the misleading comment and add a relative-path test (High findings):Now add a new test that seeds a relative path (mirroring real `pi install` output) right after the combined-filters test, plus an integration-level test per High finding #2.Need `relative` imported in the test file.Now let's add the integration-level test (High #2) — extend the shim's `pi install` to actually write settings.json like real pi, and test `main()` end-to-end.Good, that edit already succeeded. Now I need to create the writer script and wire the env var.Let me simplify — avoid mutating global process.env, instead reference the writer via the shim's own directory.Let's rewrite this more cleanly using a fixed script path relative to `dir` instead of mutating `process.env`, avoiding side effects that leak across tests.Now update the docstring above it and wire the returned path into the `pi` shim invocation without env var:That's fine, docstring update is minor. Now update the pi shim body to use the returned path via a shell variable, and update `piInstallBody` construction to happen after we have the writer path.Now update the docstring for `writePiInstallRelativeWriter` regarding env var usage — it still references `$PI_SETTINGS_FILE_FOR_SHIM` inside the generated script, which is fine, but we need to actually set that env var when invoking dedupeAndRegister/main in the new test.Good — the `PI_SETTINGS_FILE_FOR_SHIM` env var is used by the generated writer script and needs to be set by test callers to the `globalSettingsFile` path. Now add the new integration test using this.Now let's add a new integration test right after this one that uses `piInstallWritesRelativeSettingsEntry: true` and lets `main()` really apply filters end-to-end.Now let's run the tests.Found it — when `withRg: false`, PATH is restricted to shimDir alone, so `node` isn't resolvable for the writer script invocation.Our new test now passes. The remaining 2 failures are the pre-existing, unrelated failures noted in the review findings summary (macOS `/private` tmpdir symlink and stale mongodb manifest test). Let's verify these existed before our changes too.Confirmed pre-existing failures, unrelated to this diff. Now let's address the remaining findings: Medium (2) and Low (2).Medium finding 1: reminder() TODO stale comment.Now let's look at the `reminder()` TODO comment itself:This is fine now with a single forward reference from `buildPackageFilters` doc → this TODO, no more circularity. Now Medium finding 2: spread existing object entry before overriding source/filters.Now Low finding 1: extract a shared `FULLY_AVAILABLE_TOOL_STATE` constant.Let's add the constant in local-install.mjs, export it, and use it in main + this test.Now update the test to import and use this constant where used for "toolState is fully available":Now let's find and update the second-run part of the "re-derives filter fresh" test which used `{ rgOk: true, fjState: "ok" }` too.I'll leave lines 727 and 806 (different meanings — those are the raw preflight/buildPackageFilters return values, not the "fully available default" literal duplication mentioned in the finding) but update line 1011 which matches.Now Low finding 2: split/add a coverage test for excluded→excluded-with-different-toolState transition.Now run the full test suite again.All 111 relevant tests pass; only the 2 known pre-existing unrelated failures remain. Let's also check for the extensions/learn-repo test suite that's part of `npm test`.Good, learn-repo tests pass fine too. Let's check the diff summary and confirm no lint/build scripts exist beyond `install:local`/`test`.No lint/build scripts. Confirmed only `test` gate exists; we already ran it. Let's do a final review of the diff.This looks good. Let's double check the constant placement (it's before `USER_GIT_DIR`/`SETTINGS_FILE` which is a bit odd stylistically but functionally fine).Let's move it after `SETTINGS_FILE` to keep the AGENT_DIR-derived group together, for cleanliness.Still just the 2 pre-existing failures. Let's confirm the final overall count.All findings addressed. Let's view the final diff for sanity.This is a clean, minimal set of fixes. All findings addressed except none deliberately left unresolved — let's double check every finding was actually handled.All 7 findings (1 Critical, 2 High, 2 Medium, 2 Low) have been addressed. Final verification run:Both remaining failures are the pre-existing, unrelated ones confirmed earlier (identical on the parent commit before Stage 1's diff). # Review Findings — 170 ## Summary Fixed the Critical bug: `rewritePackageEntry()` now resolves each candidate settings-entry source against the settings file's own directory before comparing to `absRepoPath`, matching real `pi install`'s relative-path normalization (verified against the installed pi v0.85.1 binary and its `normalizePackageSourceForSettings`/`resolvePath` source). Added a unit test that seeds a real-pi-style relative path and an end-to-end `main()` integration test using a `pi` shim that writes settings.json the way real `pi install` does, closing the blind spot that let the bug ship. Corrected the misleading test comment about what `pi install` writes. Fixed the Medium findings: tidied the now-single-direction TODO cross-reference, and made `rewritePackageEntry` spread any existing object entry's properties before overriding `source`/filters. Fixed both Low findings: extracted a shared `FULLY_AVAILABLE_TOOL_STATE` constant used by both `dedupeAndRegister`'s default and relevant tests, and added a test pinning the excluded→excluded-with-different-toolState re-derivation case. `npm test` (the only test/build gate in this repo — no lint or build script exists) passes 111/113, with the 2 failures reproducing identically on the parent commit and unrelated to this diff (macOS `/private` tmpdir symlink quirk in a direct-invocation test, and a stale `mongodb`-extension assertion in the manifest test). ## Critical - [x] scripts/local-install.mjs:555-558 — `rewritePackageEntry()`'s `findIndex` matches package entries by exact string equality against `absRepoPath`, but the real `pi install <absPath>` command normalizes and writes a path **relative to the agent settings directory** (verified against the installed `pi` v0.85.1: `pi install /tmp/foo/myrepo` with `HOME=/tmp/foo` produces `"packages": ["../../../myrepo"]`, never the absolute path). Consequently `dedupeAndRegister()` always hits the `index === -1` branch and prints "could not find the just-installed package entry" in real usage — the filter rewrite (the feature this issue exists to wire in) never actually applies outside the test shims. Reproduced end-to-end with the real `pi` binary against this exact branch's checkout. Suggested fix: after `pi install absRepoPath` succeeds, re-read the settings file and match entries using the same relative-path resolution pi itself uses (e.g. resolve each candidate source — string or `pkg.source` — against `dirname(file)` and compare to `absRepoPath`, mirroring `findMatchingPackageEntries`/`sourceMatchesRepo`'s existing local-path handling, or reuse pi's own `isLocalPath`/relative-path convention). Add at least one test that seeds the settings file with a relative path (as real `pi install` produces) rather than only the absolute path, and add an end-to-end smoke test that shells out to the real `pi` binary if available in CI. ## High - [x] scripts/local-install.test.mjs:811-812 — All 10 new `dedupeAndRegister` filter-rewrite tests seed `globalSettings` with `resolve(cwd)` (the absolute path) with a comment claiming this simulates "the entry `pi install` would have written," but this is factually incorrect for real `pi` (see Critical finding above) — the comment and the tests give false confidence that the wiring works. Suggested fix: correct the comment to acknowledge the shim limitation, and add a test using a settings file seeded with a *relative* path (as real pi produces) to catch the matching bug that these tests currently mask. - [x] scripts/local-install.mjs:519-520 — When `toolState` is fully derived from `preflight()` in `main()`, `buildPackageFilters` is always invoked even when nothing needs filtering (returns `null`, handled fine), but there is no test exercising the full `main()` → `dedupeAndRegister()` → filter-rewrite pipeline end-to-end (only unit-level `dedupeAndRegister` calls with directly-supplied `toolState`). Given the Critical bug above went undetected specifically because of this gap, add an integration-level test that runs `main()` with a shimmed `pi` that actually mutates settings.json the way real `pi install` does (relative path), to close this blind spot for future changes. ## Medium - [x] scripts/local-install.mjs:578-580 — The `reminder(toolState)` TODO comment ("consumed by the dynamic reminder rewrite … referenced by the TODO below") is stale scope-tracking prose left over from before this issue was wired in; it doesn't reference "the TODO below" (there isn't one after it) and slightly conflicts with the updated `buildPackageFilters` doc comment which says "referenced by the TODO below" pointing at this exact comment. Suggested fix: tidy the comment so it doesn't describe itself as "below" from two different vantage points — e.g. simplify to a single forward-reference from `buildPackageFilters()` to this TODO without the redundant self-reference here. - [x] scripts/local-install.mjs:533-576 — `rewritePackageEntry()` silently overwrites `packages[index]` with `{ source: absRepoPath, ...filters }`, discarding any other properties the existing entry object might have had (e.g. a hypothetical `autoload` flag or future fields) if the entry was previously an object rather than a string. Since the only entries this function will ever see immediately after `pi install` are strings today, this is low risk currently, but is a latent data-loss trap if pi's install format changes. Suggested fix: spread the existing object entry (when it is one) before overriding `source`/`extensions`/`skills`, e.g. `packages[index] = { ...(typeof packages[index] === "object" ? packages[index] : {}), source: absRepoPath, ...filters }`. ## Low - [x] scripts/local-install.mjs:519 — The default `toolState = { rgOk: true, fjState: "ok" }` used when `options.toolState` is omitted duplicates the same literal used in one of the new tests (test title "dedupeAndRegister leaves the entry a plain string when toolState is omitted") — fine functionally, but consider extracting a shared `FULLY_AVAILABLE_TOOL_STATE` constant so the "fully available" default is defined once and can't drift from `buildPackageFilters`'s no-op condition. - [x] scripts/local-install.test.mjs:988-1029 — The "re-derives the filter fresh on each call" test is a good scenario, but its name and comment focus on "dropping a stale exclusion" without also asserting the inverse (going from no-filter to filtered) in the same test; consider splitting or adding a short one-liner test for excluded→excluded-with-different-toolState to fully pin re-derivation behavior (minor test-coverage polish, not blocking).

david added 2 commits

2026-09-08 02:38:37 +00:00