fix(build): ship gsd-read-injection-scanner hook to users#2407
Merged
fix(build): ship gsd-read-injection-scanner hook to users#2407
Conversation
The scanner was added in #2201 but never added to the HOOKS_TO_COPY allowlist in scripts/build-hooks.js, so it never landed in hooks/dist/. install.js reads from hooks/dist/, so every install on 1.37.0/1.37.1 emitted "Skipped read injection scanner hook — not found at target" and the read-time prompt-injection scanner was silently disabled. - Add gsd-read-injection-scanner.js to HOOKS_TO_COPY - Add it to EXPECTED_ALL_HOOKS regression test in install-hooks-copy Fixes #2406 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds the previously omitted Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/install-hooks-copy.test.cjs (1)
33-42: Extend uninstall-path test vectors for the new scanner hook as well.
EXPECTED_ALL_HOOKSnow includesgsd-read-injection-scanner.js, but uninstall-related vectors later in this file still omit it (expectedHookNames, mirroredisGsdHookCommand, andgsdCommands). Adding it there keeps lifecycle coverage symmetric.✅ Minimal test updates
const expectedHookNames = [ 'gsd-check-update', 'gsd-statusline', 'gsd-session-state', 'gsd-context-monitor', 'gsd-phase-boundary', 'gsd-prompt-guard', - 'gsd-read-guard', 'gsd-validate-commit', 'gsd-workflow-guard', + 'gsd-read-guard', 'gsd-read-injection-scanner', 'gsd-validate-commit', 'gsd-workflow-guard', ];const isGsdHookCommand = (cmd) => cmd && (cmd.includes('gsd-check-update') || cmd.includes('gsd-statusline') || cmd.includes('gsd-session-state') || cmd.includes('gsd-context-monitor') || cmd.includes('gsd-phase-boundary') || cmd.includes('gsd-prompt-guard') || - cmd.includes('gsd-read-guard') || cmd.includes('gsd-validate-commit') || + cmd.includes('gsd-read-guard') || cmd.includes('gsd-read-injection-scanner') || cmd.includes('gsd-validate-commit') || cmd.includes('gsd-workflow-guard'));const gsdCommands = [ 'node /path/gsd-check-update.js', 'node /path/gsd-statusline.js', 'bash /path/gsd-session-state.sh', 'node /path/gsd-context-monitor.js', 'bash /path/gsd-phase-boundary.sh', 'node /path/gsd-prompt-guard.js', 'node /path/gsd-read-guard.js', + 'node /path/gsd-read-injection-scanner.js', 'bash /path/gsd-validate-commit.sh', 'node /path/gsd-workflow-guard.js', ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/install-hooks-copy.test.cjs` around lines 33 - 42, The uninstall-path test vectors are missing the new scanner hook; update the uninstall-related arrays/sets so they match EXPECTED_ALL_HOOKS by including 'gsd-read-injection-scanner.js'. Specifically, add 'gsd-read-injection-scanner.js' to expectedHookNames, ensure isGsdHookCommand recognizes it, and include it in the gsdCommands list used by the uninstall-path tests so lifecycle coverage remains symmetric with EXPECTED_ALL_HOOKS.scripts/build-hooks.js (1)
17-30: Add an inventory drift guard around the manual hook allowlist.This fixes the current miss, but future omissions are still easy because
HOOKS_TO_COPYis hand-maintained. Add a check that allhooks/*.jsandhooks/*.share either explicitly allowlisted or explicitly excluded.♻️ Suggested hardening (example)
const HOOKS_TO_COPY = [ 'gsd-check-update-worker.js', 'gsd-check-update.js', 'gsd-context-monitor.js', 'gsd-prompt-guard.js', 'gsd-read-guard.js', 'gsd-read-injection-scanner.js', 'gsd-statusline.js', 'gsd-workflow-guard.js', // Community hooks (bash, opt-in via .planning/config.json hooks.community) 'gsd-session-state.sh', 'gsd-validate-commit.sh', 'gsd-phase-boundary.sh' ]; + +// Hooks intentionally present in hooks/ but not shipped. +const HOOKS_EXCLUDED_FROM_DIST = new Set([ + // e.g. 'experimental-hook.js' +]); + +function validateHookInventory() { + const discovered = fs.readdirSync(HOOKS_DIR, { withFileTypes: true }) + .filter((d) => d.isFile()) + .map((d) => d.name) + .filter((name) => name.endsWith('.js') || name.endsWith('.sh')); + + const unmanaged = discovered.filter( + (name) => !HOOKS_TO_COPY.includes(name) && !HOOKS_EXCLUDED_FROM_DIST.has(name) + ); + + if (unmanaged.length) { + throw new Error( + `Unmanaged hooks in hooks/: ${unmanaged.join(', ')}. ` + + `Add to HOOKS_TO_COPY or HOOKS_EXCLUDED_FROM_DIST.` + ); + } +} function build() { + validateHookInventory(); // Ensure dist directory exists if (!fs.existsSync(DIST_DIR)) { fs.mkdirSync(DIST_DIR, { recursive: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-hooks.js` around lines 17 - 30, HOOKS_TO_COPY is a hand-maintained allowlist that can drift from the actual hooks directory; add an inventory drift guard that enumerates files under hooks/*.js and hooks/*.sh, and ensures every discovered filename is either present in HOOKS_TO_COPY or present in a short explicit EXPLICIT_EXCLUDES array; implement this check right after the HOOKS_TO_COPY declaration (or at the start of the existing copy routine) and fail fast (throw or process.exit(1) with a clear message listing missing allowlist entries) so CI/builds break when new hook files are added but not reviewed/allowlisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/build-hooks.js`:
- Around line 17-30: HOOKS_TO_COPY is a hand-maintained allowlist that can drift
from the actual hooks directory; add an inventory drift guard that enumerates
files under hooks/*.js and hooks/*.sh, and ensures every discovered filename is
either present in HOOKS_TO_COPY or present in a short explicit EXPLICIT_EXCLUDES
array; implement this check right after the HOOKS_TO_COPY declaration (or at the
start of the existing copy routine) and fail fast (throw or process.exit(1) with
a clear message listing missing allowlist entries) so CI/builds break when new
hook files are added but not reviewed/allowlisted.
In `@tests/install-hooks-copy.test.cjs`:
- Around line 33-42: The uninstall-path test vectors are missing the new scanner
hook; update the uninstall-related arrays/sets so they match EXPECTED_ALL_HOOKS
by including 'gsd-read-injection-scanner.js'. Specifically, add
'gsd-read-injection-scanner.js' to expectedHookNames, ensure isGsdHookCommand
recognizes it, and include it in the gsdCommands list used by the uninstall-path
tests so lifecycle coverage remains symmetric with EXPECTED_ALL_HOOKS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 99c16994-595c-4fa3-9e76-880f538ae476
📒 Files selected for processing (3)
CHANGELOG.mdscripts/build-hooks.jstests/install-hooks-copy.test.cjs
…jection-scanner # Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked Issue
Fixes #2406
What was broken
The
gsd-read-injection-scannerPostToolUse hook (added in #2201) never landed in installed copies of GSD on 1.37.0 or 1.37.1. Every install printed:…and the read-time prompt-injection scanning feature was silently disabled for every user.
What this fix does
Adds
gsd-read-injection-scanner.jsto theHOOKS_TO_COPYallowlist inscripts/build-hooks.jsso the build step copies it intohooks/dist/.bin/install.jsreads fromhooks/dist/, so the file now lands at~/.claude/hooks/gsd-read-injection-scanner.json install, the skip warning goes away, and the PostToolUse Read matcher gets wired up as designed.Root cause
scripts/build-hooks.jsuses a hardcodedHOOKS_TO_COPYallowlist rather than enumeratinghooks/*.js/hooks/*.sh. #2328 added the scanner source underhooks/but didn't update the allowlist, and nothing in CI caught the gap — the npm tarball shipped the file athooks/gsd-read-injection-scanner.jsbut not athooks/dist/gsd-read-injection-scanner.js, which is whatinstall.js:5812reads.Confirmed against the published v1.37.1 tarball:
Testing
How I verified the fix
node scripts/build-hooks.js— output now includes✓ Copying gsd-read-injection-scanner.js...node --test tests/install-hooks-copy.test.cjs— 24/24 passing, including the regression test belowhooks/dist/and the copy-simulation test finds it at the install targetRegression test added?
gsd-read-injection-scanner.jstoEXPECTED_ALL_HOOKSintests/install-hooks-copy.test.cjs, so the existing "all expected hooks are copied from hooks/dist/ to target" test now fails fast if any future hook is added tohooks/but forgotten in the build allowlist.Platforms tested
Runtimes tested
Checklist
Fixes #2406confirmed-buglabel (pending maintainer triage)install-hooks-copysuite)Breaking changes
None.
Summary by CodeRabbit
Bug Fixes
Tests