171: Make reminder() report dynamic install/skip counts; remove dead fj/rg fail() code #180

Merged

david merged 2 commits from feature/issue-171/make-reminder-report-dynamic-install-skip-counts-r into main

2026-09-08 03:05:36 +00:00

david commented

2026-09-08 03:05:36 +00:00

Owner

171: Make reminder() report dynamic install/skip counts; remove dead fj/rg fail() code

Implements 171.

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 26m 55s.

Review findings

Now let's add the defensive test for a partial/malformed toolState.Now let's add the PR description note finding (Medium) — since there's no PR yet in this stage, I'll note it as addressed via a code comment/commit note isn't quite right. Let's check what's feasible — maybe add a comment in the code referencing this verification, since PR doesn't exist yet in this stage.Both pre-existing failures confirmed, unrelated to my change (matches the review's own finding). New reminder tests pass (118 vs previous 117 pass count, +1 new test added, all green except the 2 pre-existing).

Now let's add a comment addressing the Medium finding about documenting the verification result in code, since PR doesn't exist yet at this stage.Both Low findings are resolved. The Medium finding is a PR-description/process recommendation that can't be addressed at this stage (no PR exists yet — that's created by a downstream commit/PR skill), so it remains unresolved but I'll note it for the pipeline.Confirmed: 118 passing (up from 117, +1 new test), same 2 pre-existing unrelated failures. Lint gate N/A (no lint script/config). No build step required for this plain Node script project.

Review Findings — 171

Summary

Fixed both Low findings: reminder() now destructures toolState with per-field defaults (rgOk = true, fjState = "ok") so a partial object like {} falls back to the fully-available behaviour instead of silently misreading missing fields as "rg missing"; added a corresponding unit test (reminder falls back to full counts when given a partial toolState missing both fields) asserting reminder({}) behaves like the full-count default. Re-ran npm test: 118/120 pass (one more passing than before, from the new test), with the same 2 pre-existing failures reproduced identically on main (macOS /private tmpdir-symlink path mismatch, and a stale mongodb-manifest assertion) — both unrelated to this change and out of scope. No lint script/config exists in the repo (lint gate N/A); no build step applies (plain ESM Node scripts, node --check passes on both changed files). The Medium finding (PR description should explicitly note the fj/rg dead-code verification was a no-op) is left unresolved — this remediation stage doesn't open the PR itself, so there is no PR description to amend yet; that note should be added by whichever downstream step creates the PR.

Critical

High

Medium

scripts/local-install.mjs:608 — The commit message says "remove dead fj/rg fail() code," but the diff contains no code removal for that (the cleanup was already done in an earlier commit, #168/7490894). This is fine functionally, but the PR description should explicitly state "cleanup pass verified no dead code remained; nothing to remove" per the issue's own instruction ("call out explicitly in the PR description if anything was found and removed"). Suggested fix: when opening the PR, add a line noting the grep-verification result (no fail() calls reference rg/fj) so reviewers don't have to re-derive that the cleanup half of the issue was a verification-only no-op.

Low

scripts/local-install.mjs:608 — reminder(toolState = FULLY_AVAILABLE_TOOL_STATE) silently treats a partial object like {} as "rg missing, fj ok" (since toolState.rgOk is undefined → falsy) rather than falling back to the full default. This isn't currently reachable from any call site (only main() calls it, always with a real { rgOk, fjState } from preflight()), so it's a latent footgun rather than an active bug. Suggested fix: either destructure with per-field defaults (const { rgOk = true, fjState = "ok" } = toolState;) or add a short comment noting the default only covers the fully-omitted-argument case, not partial objects.
scripts/local-install.test.mjs:864 — The new reminder tests are thorough for the documented scenarios but don't cover a partial/malformed toolState (e.g. {} or { rgOk: true } missing fjState), which would help guard against the Low finding above regressing silently. Suggested fix: add one test asserting reminder({}) behaviour explicitly (even if only to document current behaviour), or add a defensive default per the fix above and adjust accordingly.

## 171: Make reminder() report dynamic install/skip counts; remove dead fj/rg fail() code Implements [171](https://git.excelera.net/david/pi-extensions-and-skills/issues/171). > 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 26m 55s. ## Review findings Now let's add the defensive test for a partial/malformed `toolState`.Now let's add the PR description note finding (Medium) — since there's no PR yet in this stage, I'll note it as addressed via a code comment/commit note isn't quite right. Let's check what's feasible — maybe add a comment in the code referencing this verification, since PR doesn't exist yet in this stage.Both pre-existing failures confirmed, unrelated to my change (matches the review's own finding). New reminder tests pass (118 vs previous 117 pass count, +1 new test added, all green except the 2 pre-existing). Now let's add a comment addressing the Medium finding about documenting the verification result in code, since PR doesn't exist yet at this stage.Both Low findings are resolved. The Medium finding is a PR-description/process recommendation that can't be addressed at this stage (no PR exists yet — that's created by a downstream commit/PR skill), so it remains unresolved but I'll note it for the pipeline.Confirmed: 118 passing (up from 117, +1 new test), same 2 pre-existing unrelated failures. Lint gate N/A (no lint script/config). No build step required for this plain Node script project. # Review Findings — 171 ## Summary Fixed both Low findings: `reminder()` now destructures `toolState` with per-field defaults (`rgOk = true`, `fjState = "ok"`) so a partial object like `{}` falls back to the fully-available behaviour instead of silently misreading missing fields as "rg missing"; added a corresponding unit test (`reminder falls back to full counts when given a partial toolState missing both fields`) asserting `reminder({})` behaves like the full-count default. Re-ran `npm test`: 118/120 pass (one more passing than before, from the new test), with the same 2 pre-existing failures reproduced identically on `main` (macOS `/private` tmpdir-symlink path mismatch, and a stale mongodb-manifest assertion) — both unrelated to this change and out of scope. No lint script/config exists in the repo (lint gate N/A); no build step applies (plain ESM Node scripts, `node --check` passes on both changed files). The Medium finding (PR description should explicitly note the fj/rg dead-code verification was a no-op) is left unresolved — this remediation stage doesn't open the PR itself, so there is no PR description to amend yet; that note should be added by whichever downstream step creates the PR. ## Critical ## High ## Medium - [ ] scripts/local-install.mjs:608 — The commit message says "remove dead fj/rg fail() code," but the diff contains no code removal for that (the cleanup was already done in an earlier commit, #168/7490894). This is fine functionally, but the PR description should explicitly state "cleanup pass verified no dead code remained; nothing to remove" per the issue's own instruction ("call out explicitly in the PR description if anything was found and removed"). Suggested fix: when opening the PR, add a line noting the grep-verification result (no `fail()` calls reference rg/fj) so reviewers don't have to re-derive that the cleanup half of the issue was a verification-only no-op. ## Low - [x] scripts/local-install.mjs:608 — `reminder(toolState = FULLY_AVAILABLE_TOOL_STATE)` silently treats a partial object like `{}` as "rg missing, fj ok" (since `toolState.rgOk` is `undefined` → falsy) rather than falling back to the full default. This isn't currently reachable from any call site (only `main()` calls it, always with a real `{ rgOk, fjState }` from `preflight()`), so it's a latent footgun rather than an active bug. Suggested fix: either destructure with per-field defaults (`const { rgOk = true, fjState = "ok" } = toolState;`) or add a short comment noting the default only covers the fully-omitted-argument case, not partial objects. - [x] scripts/local-install.test.mjs:864 — The new `reminder` tests are thorough for the documented scenarios but don't cover a partial/malformed `toolState` (e.g. `{}` or `{ rgOk: true }` missing `fjState`), which would help guard against the Low finding above regressing silently. Suggested fix: add one test asserting `reminder({})` behaviour explicitly (even if only to document current behaviour), or add a defensive default per the fix above and adjust accordingly.

david added 2 commits

2026-09-08 03:05:36 +00:00