Fix epilogue nits: stale refs, jq flag, spawn gate comment #92

Merged

forgejo_admin merged 1 commit from 91-epilogue-nits-stale-refs-jq-flag-spawn-gate into main

2026-03-14 19:20:32 +00:00

forgejo_admin commented

2026-03-14 18:40:21 +00:00

Contributor

Summary

Fixes 4 non-blocking nits from plan-pal-e-agency epilogue (nits #14-17), batched from QA reviews of PRs #87, #88, and #90.

Changes

commands/update-docs.md -- replaced stale sprint tool references (get_sprint_board, move_sprint_item) with board tool equivalents (list_board_items, update_board_item). Updated "Sprint name" gather context field and summary output to use "Board" terminology.
hooks/check-mcp-servers.sh -- added guard for scoped npm packages without @version suffix (e.g. @scope/pkg) where ${package%@*} produces an empty string. Falls back to the original package name.
hooks/check-agent-spawn.sh -- removed unnecessary -r flag from jq ... | length call (raw output has no effect on numeric output).
hooks/check-agent-spawn.sh -- updated header comment from "no issue, no agent" axiom to "capability-based agent spawn gate" to reflect the pass-through model introduced in PR #90.

Test Plan

bash -n syntax check passes on both modified shell scripts
Scoped package edge case verified: @scope/pkg -> @scope/pkg, @playwright/mcp@latest -> @playwright/mcp, some-package@1.0.0 -> some-package
Spawn gate functional test: valid dev prompt (with issue ref) passes, invalid prompt (no issue ref) is denied
Manual: verify hooks still work in a live session after merge

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #91
Plan: plan-pal-e-agency (traceability)

## Summary Fixes 4 non-blocking nits from plan-pal-e-agency epilogue (nits #14-17), batched from QA reviews of PRs #87, #88, and #90. ## Changes - **`commands/update-docs.md`** -- replaced stale sprint tool references (`get_sprint_board`, `move_sprint_item`) with board tool equivalents (`list_board_items`, `update_board_item`). Updated "Sprint name" gather context field and summary output to use "Board" terminology. - **`hooks/check-mcp-servers.sh`** -- added guard for scoped npm packages without `@version` suffix (e.g. `@scope/pkg`) where `${package%@*}` produces an empty string. Falls back to the original package name. - **`hooks/check-agent-spawn.sh`** -- removed unnecessary `-r` flag from `jq ... | length` call (raw output has no effect on numeric output). - **`hooks/check-agent-spawn.sh`** -- updated header comment from "no issue, no agent" axiom to "capability-based agent spawn gate" to reflect the pass-through model introduced in PR #90. ## Test Plan - [x] `bash -n` syntax check passes on both modified shell scripts - [x] Scoped package edge case verified: `@scope/pkg` -> `@scope/pkg`, `@playwright/mcp@latest` -> `@playwright/mcp`, `some-package@1.0.0` -> `some-package` - [x] Spawn gate functional test: valid dev prompt (with issue ref) passes, invalid prompt (no issue ref) is denied - [ ] Manual: verify hooks still work in a live session after merge ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #91 - Plan: `plan-pal-e-agency` (traceability)

forgejo_admin added 1 commit

2026-03-14 18:40:21 +00:00

Fix epilogue nits: stale sprint refs, jq flag, spawn gate comment 45e01f4171

- Replace stale sprint tool references with board tool equivalents
  in update-docs.md (MCP v0.3.0 migration)
- Handle scoped npm packages without @version suffix that produce
  empty fingerprint in check-mcp-servers.sh
- Remove unnecessary -r flag from jq length call in check-agent-spawn.sh
- Update header comment to reflect capability-based spawn gate model

Closes #91

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-14 18:41:05 +00:00

Author

Contributor

Self-Review: LGTM

Reviewed all 3 changed files (19 additions, 14 deletions) against the 4 acceptance criteria in issue #91.

Nit #14 -- update-docs.md: All 6 sprint references replaced with board equivalents. Stale remind-sprint-update.sh note removed (hook no longer exists). No sprint terminology remains.

Nit #15 -- check-mcp-servers.sh: Empty fingerprint guard added. Edge cases verified: @scope/pkg (no version) falls back to original, @playwright/mcp@latest strips correctly, some-package@1.0.0 strips correctly.

Nit #16 -- check-agent-spawn.sh: -r flag removed from jq ... | length. Correct -- -r only affects string output, not numeric.

Nit #17 -- check-agent-spawn.sh: Header comment updated to "capability-based agent spawn gate" with clarification about pass-through for types with no patterns.

No issues found. All changes are minimal and scoped to the nits described in the issue.

## Self-Review: LGTM Reviewed all 3 changed files (19 additions, 14 deletions) against the 4 acceptance criteria in issue #91. **Nit #14** -- `update-docs.md`: All 6 sprint references replaced with board equivalents. Stale `remind-sprint-update.sh` note removed (hook no longer exists). No sprint terminology remains. **Nit #15** -- `check-mcp-servers.sh`: Empty fingerprint guard added. Edge cases verified: `@scope/pkg` (no version) falls back to original, `@playwright/mcp@latest` strips correctly, `some-package@1.0.0` strips correctly. **Nit #16** -- `check-agent-spawn.sh`: `-r` flag removed from `jq ... | length`. Correct -- `-r` only affects string output, not numeric. **Nit #17** -- `check-agent-spawn.sh`: Header comment updated to "capability-based agent spawn gate" with clarification about pass-through for types with no patterns. No issues found. All changes are minimal and scoped to the nits described in the issue.

forgejo_admin commented

2026-03-14 18:43:14 +00:00

Author

Contributor

PR #92 Review

BLOCKERS

None.

NITS

settings.json still references remind-sprint-update.sh (line 206). The file itself still exists at hooks/remind-sprint-update.sh. This is outside the scope of this PR (the hook was already updated for board terminology in PR #83), but worth noting as a future cleanup -- the hook filename still says "sprint." Not blocking since the hook's internal text was already updated.
Test Plan checkbox -- the [ ] Manual: verify hooks still work in a live session after merge item is unchecked. Since these hooks are hardlinked and go live immediately on pull, post-merge manual verification is the right call. Just noting the test plan is honest about what hasn't been verified yet.

ANALYSIS

Nit #14 -- Stale sprint refs in update-docs.md: Correct. All 6 occurrences of sprint terminology replaced with board equivalents. The stale get_sprint_board and move_sprint_item MCP tool names are replaced with list_board_items and update_board_item, which match the actual MCP tools available in the pal-e-docs-mcp server. The removal of the remind-sprint-update.sh overlap note (the old blockquote warning about duplicate reminders) is appropriate -- it referenced behavior from the pre-board era. Grep confirms zero get_sprint_board or move_sprint_item references remain in the repo.

Nit #15 -- Scoped npm fingerprint edge case in check-mcp-servers.sh: Correct and necessary. For a scoped package without a version suffix (e.g., @scope/pkg), bash ${package%@*} performs shortest-suffix removal matching @*. Since the only @ is at position 0, the entire string is the shortest (and only) matching suffix, producing an empty string. The guard (if [[ -z "$stripped" ]]) correctly falls back to the original package name. The fix preserves existing behavior for versioned packages (@playwright/mcp@latest still strips to @playwright/mcp) and unscoped packages (some-package@1.0.0 still strips to some-package).

Nit #16 -- Unnecessary -r flag in check-agent-spawn.sh: Correct. jq -r (raw output) only affects string values by removing surrounding quotes. The | length filter always returns a bare integer regardless of -r. Removing the flag eliminates misleading intent without changing behavior.

Nit #17 -- Stale header comment in check-agent-spawn.sh: Correct. The old comment ("no issue, no agent" axiom) described the pre-PR#90 behavior where all agent types required pattern matching. After PR #90 introduced the pass-through model for types with empty required_patterns arrays (like Explore), the new comment ("capability-based agent spawn gate") and the expanded description accurately reflect current behavior. The schema at schemas/agent-spawn-requirements.json confirms Explore has "required_patterns": [].

SOP COMPLIANCE

Branch named after issue (91-epilogue-nits-stale-refs-jq-flag-spawn-gate references issue #91)
PR body follows template (Summary, Changes, Test Plan, Related -- all present)
Related references plan slug (plan-pal-e-agency)
Closes #91 present in PR body
No secrets committed (shell scripts and markdown only)
No unnecessary file changes (3 files, all directly scoped to the 4 nits)
Commit messages are descriptive (title clearly identifies all 4 fixes)

VERDICT: APPROVED

Tight, well-scoped batch of non-blocking nit fixes. All 4 changes are correct. The scoped npm edge case fix (nit #15) is the most substantive -- it addresses a real bug where @scope/pkg would produce an empty fingerprint and cause a false-positive "missing MCP server" warning. Since hooks are hardlinked and go live on pull, the low blast radius here (comment updates, a no-op jq flag removal, and a defensive guard) is appropriate.

## PR #92 Review ### BLOCKERS None. ### NITS 1. **`settings.json` still references `remind-sprint-update.sh`** (line 206). The file itself still exists at `hooks/remind-sprint-update.sh`. This is outside the scope of this PR (the hook was already updated for board terminology in PR #83), but worth noting as a future cleanup -- the hook filename still says "sprint." Not blocking since the hook's internal text was already updated. 2. **Test Plan checkbox** -- the `[ ] Manual: verify hooks still work in a live session after merge` item is unchecked. Since these hooks are hardlinked and go live immediately on pull, post-merge manual verification is the right call. Just noting the test plan is honest about what hasn't been verified yet. ### ANALYSIS **Nit #14 -- Stale sprint refs in `update-docs.md`**: Correct. All 6 occurrences of sprint terminology replaced with board equivalents. The stale `get_sprint_board` and `move_sprint_item` MCP tool names are replaced with `list_board_items` and `update_board_item`, which match the actual MCP tools available in the pal-e-docs-mcp server. The removal of the `remind-sprint-update.sh` overlap note (the old blockquote warning about duplicate reminders) is appropriate -- it referenced behavior from the pre-board era. Grep confirms zero `get_sprint_board` or `move_sprint_item` references remain in the repo. **Nit #15 -- Scoped npm fingerprint edge case in `check-mcp-servers.sh`**: Correct and necessary. For a scoped package without a version suffix (e.g., `@scope/pkg`), bash `${package%@*}` performs shortest-suffix removal matching `@*`. Since the only `@` is at position 0, the entire string is the shortest (and only) matching suffix, producing an empty string. The guard (`if [[ -z "$stripped" ]]`) correctly falls back to the original package name. The fix preserves existing behavior for versioned packages (`@playwright/mcp@latest` still strips to `@playwright/mcp`) and unscoped packages (`some-package@1.0.0` still strips to `some-package`). **Nit #16 -- Unnecessary `-r` flag in `check-agent-spawn.sh`**: Correct. `jq -r` (raw output) only affects string values by removing surrounding quotes. The `| length` filter always returns a bare integer regardless of `-r`. Removing the flag eliminates misleading intent without changing behavior. **Nit #17 -- Stale header comment in `check-agent-spawn.sh`**: Correct. The old comment ("no issue, no agent" axiom) described the pre-PR#90 behavior where all agent types required pattern matching. After PR #90 introduced the pass-through model for types with empty `required_patterns` arrays (like Explore), the new comment ("capability-based agent spawn gate") and the expanded description accurately reflect current behavior. The schema at `schemas/agent-spawn-requirements.json` confirms Explore has `"required_patterns": []`. ### SOP COMPLIANCE - [x] Branch named after issue (`91-epilogue-nits-stale-refs-jq-flag-spawn-gate` references issue #91) - [x] PR body follows template (Summary, Changes, Test Plan, Related -- all present) - [x] Related references plan slug (`plan-pal-e-agency`) - [x] `Closes #91` present in PR body - [x] No secrets committed (shell scripts and markdown only) - [x] No unnecessary file changes (3 files, all directly scoped to the 4 nits) - [x] Commit messages are descriptive (title clearly identifies all 4 fixes) ### VERDICT: APPROVED Tight, well-scoped batch of non-blocking nit fixes. All 4 changes are correct. The scoped npm edge case fix (nit #15) is the most substantive -- it addresses a real bug where `@scope/pkg` would produce an empty fingerprint and cause a false-positive "missing MCP server" warning. Since hooks are hardlinked and go live on pull, the low blast radius here (comment updates, a no-op jq flag removal, and a defensive guard) is appropriate.