169: Implement buildPackageFilters() with exhaustive unit tests #178

Merged

david merged 2 commits from feature/issue-169/implement-buildpackagefilters-with-exhaustive-unit into main

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

david commented

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

Owner

169: Implement buildPackageFilters() with exhaustive unit tests

Implements 169.

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 21m 27s.

Review findings

Now let's rename the test titles to align with the plan doc wording (Low finding) and add a note/link to follow-up (Medium finding). Let's check surrounding context for the "before" test naming and preflight() if-block style.Now let's add a doc note referencing the follow-up step (Medium finding), consistent with the plan doc style used for the existing TODO comments.Now run the tests.Confirmed same pre-existing 2 failures, unrelated to this change. Let's verify buildPackageFilters tests pass specifically.All 6 pass with names now matching the plan doc exactly. Let's view the final diff.Both findings addressed. Left the Medium "not wired in yet" architectural point as documentation-only (not fixing the actual wiring since that's explicitly out of scope for this issue per the plan's phased design) — I added a doc note pointing to the follow-up rather than doing the wiring itself, so I'll mark it resolved since the concern (reviewer confusion) is addressed via documentation, which was the suggested fix.# Review Findings — 169

Summary

Applied both open findings. Medium: added a JSDoc note on buildPackageFilters() explicitly stating it is not yet wired into dedupeAndRegister()/reminder() and pointing to the dependent "settings.json filter rewrite"/"dynamic reminder rewrite" steps (matching the suggested fix — documenting the phased-plan context so reviewers don't mistake this for the full feature). Low (test naming): renamed all 6 buildPackageFilters test titles to match the wording in docs/INSTALL_SCRIPT_TOOL_DETECTION_TEST_PLAN.md verbatim, improving traceability. Left the Low stylistic finding about terse one-line if guards unresolved — that terse style is already used throughout the surrounding file (e.g. detectFjState, parseGitRemote, isThisRepo), so "fixing" it would mean deviating from established local convention rather than following it; not worth the churn for a purely cosmetic, non-blocking note. Re-ran npm test (the only gate defined in package.json): 99/101 pass, same 2 pre-existing failures as before my changes (macOS /tmp vs /private/tmp symlink mismatch, and a stale mongodb-extension manifest expectation) — both reproduced identically before this remediation, confirming no regressions introduced.

Critical

(none)

High

(none)

Medium

scripts/local-install.mjs:284 — buildPackageFilters() is exported but not yet called from dedupeAndRegister()/reminder(), so the new filtering behavior has no production effect until the follow-up step lands. This is consistent with the phased plan doc, but leaves issue 169 as pure dead code from an end-user perspective for however long the next step is delayed. Suggested fix: none required now, but track/link the follow-up issue (settings.json rewrite) explicitly in the PR description so reviewers don't mistake this for the full feature.

Low

scripts/local-install.test.mjs:764 — Test names deviate slightly from the wording in docs/INSTALL_SCRIPT_TOOL_DETECTION_TEST_PLAN.md (e.g. "rg is missing" vs. planned "rg is absent"; last test says "excludes pr-comments before rg" rather than the plan's "excludes all three when both fj and rg are absent"). Behaviorally equivalent and still exhaustive, but future readers cross-referencing the plan doc may be confused by the naming mismatch. Suggested fix: align test titles with the plan doc's wording (or update the plan doc) for easier traceability.
scripts/local-install.mjs:286-290 — The two if conditions for extensions (fjState check, then rgOk check) and the single if for skills are terse one-liners without blank-line separation from the return guard, slightly diverging from the more explicit multi-line if blocks used elsewhere in the file (e.g. preflight()). Suggested fix: purely stylistic; no action needed unless the team wants uniform brace style across the file.

## 169: Implement buildPackageFilters() with exhaustive unit tests Implements [169](https://git.excelera.net/david/pi-extensions-and-skills/issues/169). > 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 21m 27s. ## Review findings Now let's rename the test titles to align with the plan doc wording (Low finding) and add a note/link to follow-up (Medium finding). Let's check surrounding context for the "before" test naming and preflight() if-block style.Now let's add a doc note referencing the follow-up step (Medium finding), consistent with the plan doc style used for the existing TODO comments.Now run the tests.Confirmed same pre-existing 2 failures, unrelated to this change. Let's verify buildPackageFilters tests pass specifically.All 6 pass with names now matching the plan doc exactly. Let's view the final diff.Both findings addressed. Left the Medium "not wired in yet" architectural point as documentation-only (not fixing the actual wiring since that's explicitly out of scope for this issue per the plan's phased design) — I added a doc note pointing to the follow-up rather than doing the wiring itself, so I'll mark it resolved since the concern (reviewer confusion) is addressed via documentation, which was the suggested fix.# Review Findings — 169 ## Summary Applied both open findings. Medium: added a JSDoc note on `buildPackageFilters()` explicitly stating it is not yet wired into `dedupeAndRegister()`/`reminder()` and pointing to the dependent "settings.json filter rewrite"/"dynamic reminder rewrite" steps (matching the suggested fix — documenting the phased-plan context so reviewers don't mistake this for the full feature). Low (test naming): renamed all 6 `buildPackageFilters` test titles to match the wording in `docs/INSTALL_SCRIPT_TOOL_DETECTION_TEST_PLAN.md` verbatim, improving traceability. Left the Low stylistic finding about terse one-line `if` guards unresolved — that terse style is already used throughout the surrounding file (e.g. `detectFjState`, `parseGitRemote`, `isThisRepo`), so "fixing" it would mean deviating from established local convention rather than following it; not worth the churn for a purely cosmetic, non-blocking note. Re-ran `npm test` (the only gate defined in `package.json`): 99/101 pass, same 2 pre-existing failures as before my changes (macOS `/tmp` vs `/private/tmp` symlink mismatch, and a stale mongodb-extension manifest expectation) — both reproduced identically before this remediation, confirming no regressions introduced. ## Critical (none) ## High (none) ## Medium - [x] scripts/local-install.mjs:284 — `buildPackageFilters()` is exported but not yet called from `dedupeAndRegister()`/`reminder()`, so the new filtering behavior has no production effect until the follow-up step lands. This is consistent with the phased plan doc, but leaves issue 169 as pure dead code from an end-user perspective for however long the next step is delayed. Suggested fix: none required now, but track/link the follow-up issue (settings.json rewrite) explicitly in the PR description so reviewers don't mistake this for the full feature. ## Low - [x] scripts/local-install.test.mjs:764 — Test names deviate slightly from the wording in `docs/INSTALL_SCRIPT_TOOL_DETECTION_TEST_PLAN.md` (e.g. "rg is missing" vs. planned "rg is absent"; last test says "excludes pr-comments before rg" rather than the plan's "excludes all three when both fj and rg are absent"). Behaviorally equivalent and still exhaustive, but future readers cross-referencing the plan doc may be confused by the naming mismatch. Suggested fix: align test titles with the plan doc's wording (or update the plan doc) for easier traceability. - [ ] scripts/local-install.mjs:286-290 — The two `if` conditions for `extensions` (fjState check, then rgOk check) and the single `if` for `skills` are terse one-liners without blank-line separation from the return guard, slightly diverging from the more explicit multi-line `if` blocks used elsewhere in the file (e.g. `preflight()`). Suggested fix: purely stylistic; no action needed unless the team wants uniform brace style across the file.

david added 2 commits

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