fix(claude-code): restore hook enforcement after SessionStart#654
Open
pablontiv wants to merge 11 commits into
Open
fix(claude-code): restore hook enforcement after SessionStart#654pablontiv wants to merge 11 commits into
pablontiv wants to merge 11 commits into
Conversation
The Claude Code plugin reinforces its memory protocol with hooks after
SessionStart, but all of that reinforcement was a silent no-op — memory
capture worked only in short sessions where the initial SessionStart
injection sufficed. Four defects, verified against Claude Code 2.1.216:
1. user-prompt-submit.sh/.ps1 emitted {"systemMessage": ...}. On a
UserPromptSubmit hook only stdout/hookSpecificOutput.additionalContext
reaches the model; systemMessage renders to the terminal as
"UserPromptSubmit says: ..." (the root of Gentleman-Programming#145) and never reaches the
model. Switch both the first-message ToolSearch bootstrap and the
15-minute save nudge to additionalContext, matching session-start.sh.
2. The ToolSearch select: list used mcp__engram__* only. Plugin/marketplace
installs (the documented path) expose mcp__plugin_engram_engram__*, so
the bootstrap matched nothing. Gentleman-Programming#192 (commit 3b99f0a) narrowed the list to
the direct-MCP prefix, breaking the plugin path. List both prefixes;
ToolSearch select: loads whichever names exist and ignores the rest, so
one list serves both install modes. Resolves Gentleman-Programming#534.
3. subagent-stop.sh read .stdout, absent from the SubagentStop payload, so
OUTPUT was always empty and passive capture never fired. Read
last_assistant_message (parity with plugin/codex/scripts).
4. hooks.json SessionStart matcher was startup|clear, so resumed and forked
sessions received no context injection. Add resume and fork.
Tests: new plugin/claude_code_hook_enforcement_test.go covers defects 1/3/4;
setup_test.go's UsesCurrentMCPServerID is split into two mode-specific tests
that require both prefixes.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughClaude Code hooks now match additional session events, emit prompt content through ChangesClaude Code hook behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jul 22, 2026
There was a problem hiding this comment.
Pull request overview
This PR restores post-SessionStart memory-hook enforcement for the Claude Code plugin by fixing hook output delivery, tool discovery compatibility across install modes, SubagentStop payload parsing, and SessionStart matching for resumed/forked sessions. It strengthens the plugin’s reliability by ensuring the “memory protocol” reinforcement continues to reach the model (silently) across longer, resumed, and subagent-heavy sessions.
Changes:
- Switch
UserPromptSubmitoutputs fromsystemMessagetohookSpecificOutput.additionalContext(first-message ToolSearch bootstrap + 15-minute save nudge) to ensure the content reaches the model context and doesn’t render in the terminal UI. - Update ToolSearch
select:bootstrap to include bothmcp__plugin_engram_engram__*andmcp__engram__*prefixes so both plugin/marketplace and direct-MCP installs resolve tools. - Fix
SubagentStopcapture to read.last_assistant_message(with.stdoutfallback) and expandSessionStartmatcher to includeresumeandfork, with regression tests added.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/claude-code/scripts/user-prompt-submit.sh | Emits hookSpecificOutput.additionalContext (not systemMessage) and uses a dual-prefix ToolSearch select: list; updates the 15-minute nudge to the same output mechanism. |
| plugin/claude-code/scripts/user-prompt-submit.ps1 | Mirrors the shell hook behavior for Windows-native execution, emitting hookSpecificOutput.additionalContext and the same dual-prefix ToolSearch list. |
| plugin/claude-code/scripts/subagent-stop.sh | Reads subagent output from .last_assistant_message (with .stdout fallback) so passive capture works with Claude Code’s actual payload shape. |
| plugin/claude-code/hooks/hooks.json | Expands SessionStart matcher to cover resume and fork (while keeping compact handled by the separate compaction hook group). |
| plugin/claude_code_hook_enforcement_test.go | Adds deterministic regression tests to prevent silent reintroduction of the hook no-op defects (delivery mechanism, matcher coverage, and SubagentStop extraction). |
| internal/setup/setup_test.go | Splits ToolSearch name assertions into separate direct-MCP vs plugin-scoped prefix tests to lock in compatibility for both install modes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/setup/setup_test.go (1)
2355-2384: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLock in the full dual-prefix ToolSearch contract across both hook implementations.
The Bash tests cover only five tools, and the PowerShell test covers no selected tool names. A removed session/passive-capture tool can therefore silently regress in one or both install modes.
internal/setup/setup_test.go#L2355-L2384: derive or enumerate the complete agent tool set and assert every direct and plugin-scoped name in the Bash bootstrap.plugin/claude_code_hook_enforcement_test.go#L98-L113: assert the PowerShell bootstrap contains that same complete dual-prefix tool set.As per path instructions, tests must cover happy paths, error paths, and edge cases, and behavior changes without tests should be blocked.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/setup/setup_test.go` around lines 2355 - 2384, Expand TestClaudeCodeUserPromptHookCoversDirectMCPServerID and TestClaudeCodeUserPromptHookCoversPluginServerID in internal/setup/setup_test.go to derive or enumerate the complete agent tool set, including session and passive-capture tools, and assert every tool under both direct and plugin-scoped prefixes in the Bash bootstrap. Update the PowerShell bootstrap test in plugin/claude_code_hook_enforcement_test.go (lines 98-113) to assert the same complete dual-prefix tool set; preserve coverage for missing-tool/error cases and relevant empty or boundary inputs.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/claude_code_hook_enforcement_test.go`:
- Around line 106-112: Strengthen the PowerShell payload assertions in the
relevant test by requiring the emitted object wrapper, such as
hookSpecificOutput, and an exact UserPromptSubmit event assignment, rather than
merely searching for additionalContext and hookEventName anywhere in the script.
Preserve the existing systemMessage prohibition and ensure the assertions
validate the actual emitted payload structure.
- Around line 126-131: Update the assertions in the subagent-stop.sh validation
test to positively require an extraction expression containing both
last_assistant_message and .stdout as the fallback, while retaining the existing
rejection of stdout-only extraction. Ensure the test covers the non-Claude
harness fallback rather than merely checking that the former expression is
absent.
- Around line 64-68: Update the SessionStart matcher assertions in the relevant
test to split matcher on "|" and verify each expected value as an exact token,
preventing invalid supersets such as "resumed" from passing. Preserve coverage
for startup, resume, clear, and fork, and add cases covering valid matchers,
malformed or missing alternatives, and edge conditions so the behavior change is
fully tested.
---
Outside diff comments:
In `@internal/setup/setup_test.go`:
- Around line 2355-2384: Expand
TestClaudeCodeUserPromptHookCoversDirectMCPServerID and
TestClaudeCodeUserPromptHookCoversPluginServerID in internal/setup/setup_test.go
to derive or enumerate the complete agent tool set, including session and
passive-capture tools, and assert every tool under both direct and plugin-scoped
prefixes in the Bash bootstrap. Update the PowerShell bootstrap test in
plugin/claude_code_hook_enforcement_test.go (lines 98-113) to assert the same
complete dual-prefix tool set; preserve coverage for missing-tool/error cases
and relevant empty or boundary inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 21704eb8-82ec-4d8c-857a-08fe0d30e053
📒 Files selected for processing (6)
internal/setup/setup_test.goplugin/claude-code/hooks/hooks.jsonplugin/claude-code/scripts/subagent-stop.shplugin/claude-code/scripts/user-prompt-submit.ps1plugin/claude-code/scripts/user-prompt-submit.shplugin/claude_code_hook_enforcement_test.go
…hook message Addresses CodeRabbit review on the hook-enforcement PR: - SessionStart matcher test now splits on "|" and compares exact tokens, so an invalid superset like "resumed" no longer satisfies "resume". - PowerShell payload test asserts emit-only tokens (the hookSpecificOutput wrapper, the quoted UserPromptSubmit event value, and additionalContext = $message) instead of bare words that also appear in comments. - subagent-stop test positively requires the combined ".last_assistant_message // .stdout" extraction, so a last_assistant_message- only expression that would break non-Claude harnesses is rejected. Also replace the em dash in the Windows hook message (and its .sh counterpart) with ASCII "-": user-prompt-submit.ps1 is now pure ASCII, avoiding the PSUseBOMForUnicodeEncodedFile mis-decode on legacy Windows PowerShell where a non-BOM source is read using the ANSI codepage.
…re word
Copilot review: TestUserPromptSubmitShellUsesAdditionalContext asserted
strings.Contains(script, "additionalContext"), which the script's explanatory
comments also satisfy. Assert the emitted JSON key form ("additionalContext":),
which appears only in the printf payload — consistent with the hookEventName and
systemMessage assertions already in this test.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/claude_code_hook_enforcement_test.go`:
- Around line 86-90: Update the assertion in the hook output test to validate
the complete Claude Code payload structure, not just the standalone
"additionalContext" key. Require the emitted JSON to include hookSpecificOutput
with hookEventName set to "UserPromptSubmit" and the additionalContext field
nested within it, while preserving the existing failure behavior and covering
the malformed or missing-wrapper case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 29a1baa9-8db0-44e0-b26f-ed60a7631b29
📒 Files selected for processing (1)
plugin/claude_code_hook_enforcement_test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
plugin/claude_code_hook_enforcement_test.go:95
- TestUserPromptSubmitShellUsesAdditionalContext asserts the bootstrap emits an additionalContext JSON key and that no systemMessage JSON key exists, but it doesn’t assert that the 15-minute save nudge still emits the "MEMORY REMINDER" text via an additionalContext field. A regression could drop additionalContext from the nudge while leaving the bootstrap intact and still passing.
// Assert the emitted JSON key form ("additionalContext":), not the bare word:
// the explanatory comments also reference additionalContext, so a bare-word
// check would pass even if the emitted payload dropped the field.
if !strings.Contains(script, `"additionalContext":`) {
t.Error("user-prompt-submit.sh does not emit an additionalContext JSON field - hook output never reaches the model")
}
if !strings.Contains(script, `"hookEventName":"UserPromptSubmit"`) &&
!strings.Contains(script, `hookEventName: "UserPromptSubmit"`) {
t.Error("user-prompt-submit.sh additionalContext payload is missing hookEventName: UserPromptSubmit")
}
…put for both shell emit paths CodeRabbit: separate key checks passed even with additionalContext at the top level, where Claude Code ignores it. Assert the contiguous nested structure for the bootstrap printf and the jq-built nudge.
…h hook scripts CodeRabbit outside-diff finding: the mode-specific tests asserted only 5 of the 13 bootstrap tools and read only the bash hook, leaving the PowerShell select: list and 8 tools per prefix unasserted. Enumerate the complete bootstrap tool set once and assert every name under both prefixes in user-prompt-submit.sh and user-prompt-submit.ps1.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/claude_code_hook_enforcement_test.go (1)
139-150: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftExercise the fallback behavior, not only the source text.
This test does not cover payloads where
last_assistant_messageis present, only.stdoutis present, or both fields are missing/malformed. Add deterministic fixture cases asserting primary-field precedence, stdout fallback, and the expected empty/error behavior.As per path instructions, tests must cover happy paths, error paths, and edge cases, and behavior changes without tests should be blocked.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/claude_code_hook_enforcement_test.go` around lines 139 - 150, Expand TestSubagentStopReadsLastAssistantMessage beyond source-text checks by executing the subagent-stop extraction with deterministic payload fixtures. Assert last_assistant_message takes precedence when both fields exist, stdout is used when the primary field is absent, and missing or malformed fields produce the expected empty/error behavior; retain coverage for the existing script contract.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/claude_code_hook_enforcement_test.go`:
- Around line 92-94: Extend the nudge-path assertions in the relevant hook
enforcement test to reject jq’s unquoted systemMessage object-key form in
addition to the existing quoted-key check. Scope the assertion to the generated
script and ensure payloads containing either quoted or unquoted systemMessage
cannot pass, while preserving the current valid hookSpecificOutput structure
check.
---
Outside diff comments:
In `@plugin/claude_code_hook_enforcement_test.go`:
- Around line 139-150: Expand TestSubagentStopReadsLastAssistantMessage beyond
source-text checks by executing the subagent-stop extraction with deterministic
payload fixtures. Assert last_assistant_message takes precedence when both
fields exist, stdout is used when the primary field is absent, and missing or
malformed fields produce the expected empty/error behavior; retain coverage for
the existing script contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aec6edde-627a-47ea-a5e3-b97e2af7759f
📒 Files selected for processing (1)
plugin/claude_code_hook_enforcement_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/setup/setup_test.go`:
- Around line 2370-2375: The MCP ToolSearch assertions in
TestClaudeCodeUserPromptHookCoversDirectMCPServerID and its sibling test
currently use substring matching, allowing mem_save_prompt to satisfy mem_save.
Update both assertion sites in internal/setup/setup_test.go (2370-2375 and
2381-2385) to match exact comma/newline-delimited entries or parse the select
list, and add a regression case covering this overlap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cf9b0de6-f612-42d6-9c2d-678b3f819415
📒 Files selected for processing (1)
internal/setup/setup_test.go
pablontiv
force-pushed
the
fix/claude-code-hook-enforcement
branch
from
July 22, 2026 16:10
0f06914 to
0491b61
Compare
…spects its cooldown user-prompt-submit.sh wrote the last-nudge timestamp with `printf '%s'` (no trailing newline) and read it back with `read -r ... || VAR=""`. bash `read` returns non-zero at EOF without a delimiter, so the `||` cleared the value just read: the debounce state was always empty and the 15-minute save reminder fired on every message — the exact behavior the script header comment says the debounce prevents. Adds executed-behavior tests for the nudge path (fire, silent-when-recent, and debounce) that run the hook against fixture payloads with an httptest server injected via ENGRAM_PORT. TestNudgeIsDebouncedWithinCooldown is the regression proof: it fails against the pre-fix script.
Rewrote toolSearchSelectSet to scan every select: occurrence in script source and return the largest token set, eliminating accidental coupling to comment content. Added regression test proving a comment containing select:mcp__ before the real list does not fool the parser. Also documented the cooldown assumption in TestNudgeIsDebouncedWithinCooldown and cross-linked selectNames↔toolSearchSelectSet with sync guidance. Test: toolSearchSelectSet scans-largest for robustness; selectNames uses single anchor since it parses runtime output (no comments). Claude-Session: https://claude.ai/code/session_01DhfZeBjXBBFw8J6yM2wssg
Author
|
The review threads are addressed and the branch now carries a layered test strategy (details in the threads and the updated Test Plan). Writing the executed-hook nudge tests also surfaced a fifth, pre-existing defect — the save-nudge debounce never persisted, so the reminder fired on every message; it's fixed in the same PR ( |
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
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
Closes #653
🏷️ PR Type
type:bug— Bug fix📝 Summary
Restores the Claude Code plugin's post-
SessionStartmemory-hook enforcement, which was a silent no-op. Five defects — the first four verified against Claude Code2.1.216, the fifth found and covered by the new executed-hook tests:user-prompt-submit.sh/.ps1emittedsystemMessage, which renders to the terminal and never reaches the model (root of Bug: UserPromptSubmit hook output leaks into conversation UI on every session start #145) → switched tohookSpecificOutput.additionalContext, for both the first-message ToolSearch bootstrap and the 15-minute save nudge.select:list usedmcp__engram__*only, matching nothing on plugin/marketplace installs (which exposemcp__plugin_engram_engram__*). fix(setup): repair Claude MCP tool discovery #192 (3b99f0a) narrowed it to the direct-MCP prefix and broke the plugin path → now lists both prefixes;ToolSearch select:loads whichever names exist and ignores the rest. Resolves UserPromptSubmit hook injects wrong tool prefix when engram is installed as a plugin #534; supersedes fix(claude-plugin): use plugin-scoped MCP tool prefix in UserPromptSubmit hint #642.subagent-stop.shread.stdout(absent from theSubagentStoppayload) → readslast_assistant_message(parity withplugin/codex/scripts).hooks.jsonSessionStartmatcher wasstartup|clear→startup|resume|clear|fork.user-prompt-submit.shwrote the save-nudge's debounce timestamp withprintf '%s'(no trailing newline) and read it back withread -r … || VAR=""; bashreadreturns non-zero at EOF without a delimiter, so the||cleared the value and the debounce never persisted — the reminder fired on every message once a save was >15 min old, the exact behavior the script's header comment says the debounce prevents →printf '%s\n'.Scope is the Claude Code plugin only. Codex, pi (#546), and opencode are documented as out of scope in the issue.
📂 Changes
plugin/claude-code/scripts/user-prompt-submit.shsystemMessage→additionalContext(bootstrap + nudge); dual-prefixselect:list; nudge debounce timestampprintf '%s'→printf '%s\n'plugin/claude-code/scripts/user-prompt-submit.ps1plugin/claude-code/scripts/subagent-stop.sh.stdout→.last_assistant_message // .stdoutplugin/claude-code/hooks/hooks.jsonSessionStartmatcher addsresume,forkplugin/claude_code_hook_behavior_test.gohttptest) and asserts on parsed stdout / captured POST bodiesplugin/claude_code_hook_enforcement_test.gohooks.jsonmatcher and PowerShell fallback (interpreter-free)internal/setup/setup_test.goselect:names parsed and matched as exact comma-delimited tokens, both prefixes, both hooks🧪 Test Plan
Tests are layered so no contract depends on matching script source text:
Behavior (
plugin/claude_code_hook_behavior_test.go) — runs the hooks underbashwith fixture stdin and anhttptestserver injected throughENGRAM_PORT. Asserts parsed stdout for the bootstrap, second-message, nudge, no-nudge and debounce paths, and the captured POST body for SubagentStop extraction,.stdoutfallback, empty payload, and shell-metacharacter round-tripping. Skips ifbashorjqis absent.Static backstop (
plugin/claude_code_hook_enforcement_test.go) — runs with no interpreter. Covers thehooks.jsonSessionStartmatcher by exact token, and the PowerShell fallback, which has no dependable runner.ToolSearch names (
internal/setup/setup_test.go) — parses each hook'sselect:list and compares exact comma-delimited tokens, for both prefixes and both hook files.go test ./...go test -tags e2e ./internal/server/...go vet ./...Each test verified to fail when its contract is broken (payload un-nested,
.stdoutfallback dropped, extraction reverted, a tool name deleted fromselect:, debounce newline removed).Manually verified against Claude Code binary
2.1.216(unchanged from the original submission):additionalContextenters context silently whilesystemMessagerenders as"UserPromptSubmit says: ..."; theSubagentStoppayload provideslast_assistant_messageand nostdout; theSessionStartsource enum includesresume/fork; andToolSearch select:returns only the names that exist.💬 Notes for Reviewers
Defect 2 partially reverses #192 (
3b99f0a, "use current MCP tool names in hook"), which swapped theselect:list from the plugin-scoped prefix tomcp__engram__*. That fixed direct-MCP installs but broke the plugin/marketplace path — the two prefixes are each correct for a different install mode, so the fix lists both rather than picking one. The twointernal/setuptests added by #192 are updated accordingly (split into...CoversDirectMCPServerIDand...CoversPluginServerID, each requiring its prefix). Happy to adjust framing if you'd prefer the prefix change handled separately from the other three defects.Summary by CodeRabbit