fix(hooks): deduplicate hook handler invocations when installed in mu… - #621
Conversation
📝 WalkthroughWalkthroughChangesHook invocation deduplication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HookHandler
participant DeduplicationCache
participant PolicyEvaluation
HookHandler->>DeduplicationCache: Check canonical invocation
DeduplicationCache-->>HookHandler: Return duplicate status and prior exit code
alt New invocation
HookHandler->>PolicyEvaluation: Evaluate policy
PolicyEvaluation-->>HookHandler: Return exit code
HookHandler->>DeduplicationCache: Record invocation and exit code
end
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
__tests__/hooks/dedup-invocation.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/hooks/dedup-invocation.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/hooks/handler.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
|
Your PR is awaiting review by a reviewer. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@__tests__/hooks/dedup-invocation.test.ts`:
- Around line 53-70: Add a unit-test assertion in the existing “returns
isDuplicate: false when tool_input or session differs” test using identical
tool_input but a different session_id, then verify isDuplicate is false through
isDuplicateInvocation after recording the original payload.
In `@src/hooks/dedup-invocation.ts`:
- Around line 9-13: Extend DedupRecord in src/hooks/dedup-invocation.ts (lines
9-13) with the response fields required to reproduce stdout, stderr, and
stop-event retry semantics; persist those fields when recording the completed
invocation (lines 48-53). In src/hooks/handler.ts (lines 225-229), emit the
cached response through the normal output path before returning, rather than
replaying only exitCode.
- Around line 21-24: Replace the raw toolInput portion of the key derived near
parsed.session_id, parsed.tool_name, and tool_input with a one-way digest of the
structured tuple (cli, eventType, session, tool, and tool input). Reuse the
identical derivation at the recording logic around lines 55-58 and persist only
the digest-based key around lines 82-83 in src/hooks/dedup-invocation.ts.
- Around line 29-40: Make invocation claiming atomic in
src/hooks/dedup-invocation.ts at lines 29-40 by adding a lock or pending record
state so only one process claims a previously unseen key; competing processes
must briefly wait for the completed record and reuse its result instead of
evaluating independently. Update recordInvocation at lines 63-83 to finalize the
pending claim without overwriting concurrent records or losing updates.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78c3a9fd-bc7b-4ec6-80ea-6a614a81dad0
📒 Files selected for processing (3)
__tests__/hooks/dedup-invocation.test.tssrc/hooks/dedup-invocation.tssrc/hooks/handler.ts
| it("returns isDuplicate: false when tool_input or session differs", async () => { | ||
| const { isDuplicateInvocation, recordInvocation } = await import("../../src/hooks/dedup-invocation"); | ||
| const payload1 = { | ||
| session_id: "s1", | ||
| tool_name: "Bash", | ||
| tool_input: { command: "ls" }, | ||
| }; | ||
| const payload2 = { | ||
| session_id: "s1", | ||
| tool_name: "Bash", | ||
| tool_input: { command: "pwd" }, | ||
| }; | ||
|
|
||
| recordInvocation("claude", "PreToolUse", payload1, 0); | ||
|
|
||
| const check = isDuplicateInvocation("claude", "PreToolUse", payload2); | ||
| expect(check.isDuplicate).toBe(false); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the session-ID key branch.
This case only changes tool_input; add a same-input/different-session_id assertion expecting isDuplicate: false.
As per coding guidelines, “Add corresponding unit tests for every new or changed behavior.”
🤖 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 `@__tests__/hooks/dedup-invocation.test.ts` around lines 53 - 70, Add a
unit-test assertion in the existing “returns isDuplicate: false when tool_input
or session differs” test using identical tool_input but a different session_id,
then verify isDuplicate is false through isDuplicateInvocation after recording
the original payload.
Source: Coding guidelines
| interface DedupRecord { | ||
| key: string; | ||
| timestamp: number; | ||
| exitCode: number; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Replay the full hook response, not only the exit code.
Normal handling writes result.stdout and result.stderr, but duplicates return only exitCode. A duplicate deny/instruct invocation can therefore receive a different CLI response. Persist and replay the required response payload alongside the result.
src/hooks/dedup-invocation.ts#L9-L13: include the response fields needed to reproduce the result.src/hooks/dedup-invocation.ts#L48-L53: record those fields with the completed invocation.src/hooks/handler.ts#L225-L229: emit the cached response before returning.
As per coding guidelines, “For stop events, preserve each CLI's retry semantics.”
📍 Affects 2 files
src/hooks/dedup-invocation.ts#L9-L13(this comment)src/hooks/dedup-invocation.ts#L48-L53src/hooks/handler.ts#L225-L229
🤖 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 `@src/hooks/dedup-invocation.ts` around lines 9 - 13, Extend DedupRecord in
src/hooks/dedup-invocation.ts (lines 9-13) with the response fields required to
reproduce stdout, stderr, and stop-event retry semantics; persist those fields
when recording the completed invocation (lines 48-53). In src/hooks/handler.ts
(lines 225-229), emit the cached response through the normal output path before
returning, rather than replaying only exitCode.
Source: Coding guidelines
| const session = (parsed.session_id as string) ?? ""; | ||
| const tool = (parsed.tool_name as string) ?? ""; | ||
| const toolInput = parsed.tool_input ? JSON.stringify(parsed.tool_input) : ""; | ||
| const key = `${cli}:${eventType}:${session}:${tool}:${toolInput}`; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not persist raw tool input in cache keys.
The JSON cache stores the full serialized tool_input as an object key. Tool inputs can contain commands, credentials, or file contents; stale values also remain until a later write triggers pruning. Derive the key from a one-way digest of a structured tuple instead.
src/hooks/dedup-invocation.ts#L21-L24: derive a non-reversible structured key.src/hooks/dedup-invocation.ts#L55-L58: use the identical derivation when recording.src/hooks/dedup-invocation.ts#L82-L83: persist only the digest-based key.
📍 Affects 1 file
src/hooks/dedup-invocation.ts#L21-L24(this comment)src/hooks/dedup-invocation.ts#L55-L58src/hooks/dedup-invocation.ts#L82-L83
🤖 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 `@src/hooks/dedup-invocation.ts` around lines 21 - 24, Replace the raw
toolInput portion of the key derived near parsed.session_id, parsed.tool_name,
and tool_input with a one-way digest of the structured tuple (cli, eventType,
session, tool, and tool input). Reuse the identical derivation at the recording
logic around lines 55-58 and persist only the digest-based key around lines
82-83 in src/hooks/dedup-invocation.ts.
| if (existsSync(DEDUP_CACHE_FILE)) { | ||
| try { | ||
| const raw = readFileSync(DEDUP_CACHE_FILE, "utf8"); | ||
| records = JSON.parse(raw) as Record<string, DedupRecord>; | ||
| } catch { | ||
| records = {}; | ||
| } | ||
| } | ||
|
|
||
| const existing = records[key]; | ||
| if (existing && now - existing.timestamp < DEDUP_WINDOW_MS) { | ||
| return { isDuplicate: true, exitCode: existing.exitCode }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make duplicate claiming atomic.
Concurrent hook processes can all read an empty cache before any reaches recordInvocation, so each evaluates policies and emits duplicate activity/telemetry—the exact case this PR targets. Use an atomic claim/lock with a pending state; later processes should wait briefly for the completed result rather than independently evaluating.
src/hooks/dedup-invocation.ts#L29-L40: atomically claim a previously unseen invocation.src/hooks/dedup-invocation.ts#L63-L83: finalize the claimed record without lost concurrent updates.
📍 Affects 1 file
src/hooks/dedup-invocation.ts#L29-L40(this comment)src/hooks/dedup-invocation.ts#L63-L83
🤖 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 `@src/hooks/dedup-invocation.ts` around lines 29 - 40, Make invocation claiming
atomic in src/hooks/dedup-invocation.ts at lines 29-40 by adding a lock or
pending record state so only one process claims a previously unseen key;
competing processes must briefly wait for the completed record and reuse its
result instead of evaluating independently. Update recordInvocation at lines
63-83 to finalize the pending claim without overwriting concurrent records or
losing updates.
Summary
Fixes #58 by introducing an invocation fingerprint deduplication cache (
src/hooks/dedup-invocation.ts) inhandleHookEvent, preventing duplicate policy execution, duplicate activity store logs, and duplicate telemetry when hooks are installed across multiple settings scopes (e.g. global~/.claude/settings.jsonand project.claude/settings.json).❌ Root Cause Analysis
When
failproofaiis installed in multiple scopes (e.g., both user scope~/.claude/settings.jsonand project scope.claude/settings.json):failproofai --hook PreToolUsetwice for the exact same event.handleHookEventran full policy evaluation, logged duplicate activity entries tohook-activity-store, and emitted duplicate telemetry for both processes.✅ Fix Details
Invocation Deduplication Cache (
src/hooks/dedup-invocation.ts):Created
isDuplicateInvocationandrecordInvocationusing a lightweight timestamped invocation fingerprint cache stored in~/.failproofai/cache/dedup-invocations.json.Fingerprint key:
${cli}:${eventType}:${sessionId}:${toolName}:${JSON.stringify(toolInput)}Hook Handler Integration (
src/hooks/handler.ts):handleHookEvent, computes invocation key after payload normalization.handleHookEventlogs a deduplication warning and immediately returns the previous exit code without duplicate policy re-evaluation, duplicate activity logging, or duplicate telemetry.recordInvocationupdates the cache.🧪 Unit Tests & Verification
Added unit test suite
__tests__/hooks/dedup-invocation.test.ts:returns isDuplicate: false on first invocationreturns isDuplicate: true when identical invocation occurs within deduplication windowreturns isDuplicate: false when tool_input or session differsTest Results:
vitest run __tests__/hooks/dedup-invocation.test.ts✓ 3 passed (3)Summary by CodeRabbit
New Features
Tests