fix(hooks): deduplicate multi-scope invocations#534
Conversation
📝 WalkthroughWalkthroughHook invocation handling now deduplicates stable Claude hook payloads across processes. One process evaluates and publishes the response, while duplicates replay it without repeating policy evaluation or activity persistence. ChangesHook Invocation Deduplication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Claude
participant handleHookEvent
participant claimHookInvocation
participant DeduplicationDirectory
Claude->>handleHookEvent: send hook payload
handleHookEvent->>claimHookInvocation: claim event and identifiers
claimHookInvocation->>DeduplicationDirectory: acquire lock or read response
DeduplicationDirectory-->>claimHookInvocation: owner or duplicate response
claimHookInvocation-->>handleHookEvent: return claim
handleHookEvent-->>Claude: replay response or evaluate normally
Possibly related PRs
Suggested labels: 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
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![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
🧹 Nitpick comments (1)
__tests__/hooks/hook-invocation-dedup.test.ts (1)
68-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the timeout-exhaustion fallback and TTL-based stale-entry cleanup.
The suite covers owner/duplicate election, isolation, and explicit-release takeover, but not the case where a claimed lock never gets released or completed (simulating a hard crash) and a waiting process exhausts
WAIT_TIMEOUT_MSbefore falling back toindependent, nor theremoveOldEntries/staleness-TTL cleanup path. These are both explicitly called out fallback behaviors in the PR description. Consider adding a test that pre-creates a.lockfile (without a matching result) and assertsclaimHookInvocationeventually resolves toindependent, ideally using fake timers to avoid a real multi-second wait.Use async timer APIs (advanceTimersByTimeAsync) instead of synchronous variants to prevent promise/timer deadlocks
Based on path instructions, "Every new or changed behavior must have a corresponding unit test under
__tests__/."🤖 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/hook-invocation-dedup.test.ts` around lines 68 - 79, Add tests under __tests__ covering claimHookInvocation’s hard-crash fallback: pre-create a lock without a result, use fake timers with advanceTimersByTimeAsync, and assert the claim resolves as independent after WAIT_TIMEOUT_MS. Also cover removeOldEntries and TTL-based stale-entry cleanup, ensuring stale lock entries are removed and ownership can proceed.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 `@__tests__/hooks/handler.test.ts`:
- Around line 1258-1278: Add a unit test alongside the duplicate-path test for
the owner branch of handleHookEvent: mock claimHookInvocation with role "owner"
and vi.fn() complete/release callbacks, mock evaluatePolicies to return a known
{exitCode, stdout, stderr} result, then assert complete receives that exact
result and release is called after handling, including when evaluation completes
through the handler’s finally cleanup path.
In `@src/hooks/hook-invocation-dedup.ts`:
- Around line 15-16: Update WAIT_TIMEOUT_MS and the claimHookInvocation flow so
waiting covers the owner’s worst-case runtime when multiple custom hooks are
evaluated sequentially, rather than expiring at 12 seconds. Derive the timeout
from the existing per-hook custom-hook budget and evaluation limits, or add
owner heartbeat/touch support that extends active waits; preserve deduplication
until the owner finishes.
- Around line 113-123: Update the complete callback’s result publication flow to
handle an existing resultPath before or when calling rename, such as removing
the destination first or retrying with a direct write after rename fails.
Preserve the best-effort cleanup of lockPath and ensure completed is set only
after the response is successfully published.
---
Nitpick comments:
In `@__tests__/hooks/hook-invocation-dedup.test.ts`:
- Around line 68-79: Add tests under __tests__ covering claimHookInvocation’s
hard-crash fallback: pre-create a lock without a result, use fake timers with
advanceTimersByTimeAsync, and assert the claim resolves as independent after
WAIT_TIMEOUT_MS. Also cover removeOldEntries and TTL-based stale-entry cleanup,
ensuring stale lock entries are removed and ownership can proceed.
🪄 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
Run ID: 226ae02f-ac0e-47ee-9c50-fe100d510d8d
📒 Files selected for processing (4)
__tests__/hooks/handler.test.ts__tests__/hooks/hook-invocation-dedup.test.tssrc/hooks/handler.tssrc/hooks/hook-invocation-dedup.ts
| it("replays a duplicate decision without evaluating or persisting again", async () => { | ||
| const { claimHookInvocation } = await import("../../src/hooks/hook-invocation-dedup"); | ||
| vi.mocked(claimHookInvocation).mockResolvedValueOnce({ | ||
| role: "duplicate", | ||
| response: { exitCode: 2, stdout: "", stderr: "blocked once" }, | ||
| }); | ||
| mockStdin(JSON.stringify({ | ||
| session_id: "session-1", | ||
| tool_use_id: "tool-1", | ||
| tool_name: "Bash", | ||
| })); | ||
| const { evaluatePolicies } = await import("../../src/hooks/policy-evaluator"); | ||
| const { persistHookActivity } = await import("../../src/hooks/hook-activity-store"); | ||
|
|
||
| const exitCode = await handleHookEvent("PreToolUse"); | ||
|
|
||
| expect(exitCode).toBe(2); | ||
| expect(stderrSpy).toHaveBeenCalledWith("blocked once"); | ||
| expect(evaluatePolicies).not.toHaveBeenCalled(); | ||
| expect(persistHookActivity).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Good duplicate-path coverage, but the owner-side complete()/release() wiring is untested here.
This test verifies the duplicate branch thoroughly, but no test overrides the mock to {role: "owner", complete: vi.fn(), release: vi.fn()} to assert handleHookEvent calls complete({exitCode, stdout, stderr}) with evaluatePolicies's actual result and calls release() in finally. That's the other, arguably more important, half of this handler's new control flow (owner publish + cleanup lifecycle).
Based on path instructions, "Every new or changed behavior must have a corresponding unit test under __tests__/."
🤖 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/handler.test.ts` around lines 1258 - 1278, Add a unit test
alongside the duplicate-path test for the owner branch of handleHookEvent: mock
claimHookInvocation with role "owner" and vi.fn() complete/release callbacks,
mock evaluatePolicies to return a known {exitCode, stdout, stderr} result, then
assert complete receives that exact result and release is called after handling,
including when evaluation completes through the handler’s finally cleanup path.
Source: Path instructions
| const RESULT_TTL_MS = 5 * 60_000; | ||
| const WAIT_TIMEOUT_MS = 12_000; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep outline src/hooks/hook-invocation-dedup.ts --view expandedRepository: FailproofAI/failproofai
Length of output: 909
🏁 Script executed:
sed -n '1,220p' src/hooks/hook-invocation-dedup.ts && printf '\n---\n' && sed -n '1,240p' src/hooks/policy-evaluator.tsRepository: FailproofAI/failproofai
Length of output: 15640
🏁 Script executed:
rg -n "claimHookInvocation|WAIT_TIMEOUT_MS|timeout|10s|10000|custom hook|evaluatePolicies" src/hooks src | sed -n '1,220p'Repository: FailproofAI/failproofai
Length of output: 14633
🏁 Script executed:
sed -n '180,340p' src/hooks/handler.tsRepository: FailproofAI/failproofai
Length of output: 7332
WAIT_TIMEOUT_MS should track the owner’s real worst-case runtime. claimHookInvocation() gives up after 12s, but the owner path can run longer than that once multiple custom hooks are loaded: each hook gets a 10s Promise.race budget, and evaluatePolicies() runs them sequentially. Duplicate processes can therefore abandon dedup, return role: "independent", and re-run policy evaluation/telemetry while the owner is still working.
Derive the wait from the custom-hook budget, or add a heartbeat/touch mechanism so waiters can keep waiting while the owner is active.
🤖 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/hook-invocation-dedup.ts` around lines 15 - 16, Update
WAIT_TIMEOUT_MS and the claimHookInvocation flow so waiting covers the owner’s
worst-case runtime when multiple custom hooks are evaluated sequentially, rather
than expiring at 12 seconds. Derive the timeout from the existing per-hook
custom-hook budget and evaluation limits, or add owner heartbeat/touch support
that extends active waits; preserve deduplication until the owner finishes.
| complete: async (response) => { | ||
| try { | ||
| const tmpPath = `${resultPath}.${process.pid}.tmp`; | ||
| await writeFile(tmpPath, JSON.stringify(response), "utf8"); | ||
| await rename(tmpPath, resultPath); | ||
| completed = true; | ||
| } catch { | ||
| // Publishing is best-effort; the owner must still return its policy result. | ||
| } finally { | ||
| try { await unlink(lockPath); } catch { /* best-effort */ } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant ranges.
ast-grep outline src/hooks/hook-invocation-dedup.ts --view expanded || true
wc -l src/hooks/hook-invocation-dedup.ts
sed -n '1,220p' src/hooks/hook-invocation-dedup.ts
# Find cleanup / expiry logic for resultPath and lockPath.
rg -n "resultPath|lockPath|removeOldEntries|rename\\(|unlink\\(" src/hooks -SRepository: FailproofAI/failproofai
Length of output: 7890
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, pathlib, json
path = pathlib.Path('src/hooks/hook-invocation-dedup.ts')
print(path.exists())
PYRepository: FailproofAI/failproofai
Length of output: 166
🌐 Web query:
Node.js fs.rename overwrite semantics Windows destination exists docs
💡 Result:
In Node.js, the behavior of fs.rename and fs.renameSync when the destination path (newPath) already exists is platform-dependent [1]. On POSIX-compliant systems (such as Linux and macOS), fs.rename typically overwrites the existing file at the destination [2][1]. On Windows, however, the behavior differs significantly. The operation is not guaranteed to overwrite an existing destination file [1]. Attempting to rename a file to a destination that already exists on Windows often results in an error (commonly EPERM or EEXIST) [1][3]. This is due to underlying Windows file system behaviors and restrictions—such as file locking—that prevent the seamless replacement of an existing file via a simple rename operation [3]. Because of these cross-platform inconsistencies, it is considered a best practice to check if the destination file exists before performing a rename operation if you require portable behavior [1]. You can verify the existence of the destination file using fs.access or fs.stat and then use fs.unlink to remove it if necessary before calling fs.rename [1]. Top results: [1][3]
Citations:
- 1: https://codingeasypeasy.com/blog/how-to-rename-a-file-with-fsrename-in-nodejs-a-comprehensive-guide/
- 2: https://stackoverflow.com/questions/21219018/node-js-does-fs-rename-overwrite-file-if-already-exists
- 3: EPERM when renaming files on Windows nodejs/node#29481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const fs = require('fs/promises');
const path = require('path');
const os = require('os');
(async () => {
const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'rename-probe-'));
const src = path.join(dir, 'src.txt');
const dst = path.join(dir, 'dst.txt');
await fs.writeFile(src, 'src');
await fs.writeFile(dst, 'dst');
try {
await fs.rename(src, dst);
const data = await fs.readFile(dst, 'utf8');
console.log(JSON.stringify({rename: 'ok', dst: data}));
} catch (e) {
console.log(JSON.stringify({rename: 'error', code: e.code, message: e.message}));
} finally {
await fs.rm(dir, { recursive: true, force: true });
}
})();
JSRepository: FailproofAI/failproofai
Length of output: 189
Handle existing resultPath before rename() on Windows.
rename(tmpPath, resultPath) can fail when the destination already exists, and the best-effort catch leaves the response unpublished. That makes duplicates wait out the full timeout and fall back to independent evaluation; unlink the destination first or retry with a direct write on rename() failure.
🤖 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/hook-invocation-dedup.ts` around lines 113 - 123, Update the
complete callback’s result publication flow to handle an existing resultPath
before or when calling rename, such as removing the destination first or
retrying with a direct write after rename fails. Preserve the best-effort
cleanup of lockPath and ensure completed is set only after the response is
successfully published.
|
Automated code review started - full review. Results will be posted here. |
|
⏳ Phase 0: Reading existing review threads complete. Found 3 unresolved review threads from coderabbitai[bot] on:
|
|
✅ Build & test complete. All checks green:
Now preparing the full detailed review... |
| } | ||
|
|
||
| invocationClaim = await claimHookInvocation(eventType, cli, parsed); | ||
| if (invocationClaim.role === "duplicate") { |
There was a problem hiding this comment.
🔴 Critical — Duplicate replay skips per-CLI output shaping
When the duplicate path returns, it calls process.stdout.write and process.stderr.write directly on the raw stored strings (lines 204-205). However, the normal evaluation path's output is shaped per-CLI by evaluatePolicies (which returns CLI-specific JSON shapes for deny/instruct decisions — cursor, pi, hermes, goose, etc.).
The replayed response is the RAW exitCode/stdout/stderr triple that the owner stored, which is correct for Claude (exit 2 + stderr), but if the handler ever adds post-evaluation output reshaping outside of evaluatePolicies, this path would miss it.
Current status: Safe today because all output shaping happens inside evaluatePolicies and the owner stores the already-shaped result. But this is a fragile coupling — the two output paths (owner vs duplicate) have diverged in responsibility.
Fix: Consider routing the replayed response through a common output function or at least document that all output shaping must be complete before invocationClaim.complete() is called in the owner path.
| import type { IntegrationType } from "./types"; | ||
|
|
||
| const RESULT_TTL_MS = 5 * 60_000; | ||
| const WAIT_TIMEOUT_MS = 12_000; |
There was a problem hiding this comment.
Warning: WAIT_TIMEOUT_MS (12s) may be shorter than owner worst-case runtime.
Duplicates poll for 12s before falling through to independent evaluation. But the owner evaluates custom hooks sequentially, each with a 10s Promise.race budget (handler.ts line 270). With 2+ custom hooks, owner runtime exceeds 12s and duplicates abandon dedup, producing duplicate telemetry and activity entries.
Fix options:
(1) Derive WAIT_TIMEOUT_MS from custom hook count x 10s + buffer.
(2) Owner heartbeat: periodically touch the lock file mtime so waiters can extend their deadline.
(3) Cap WAIT_TIMEOUT_MS at max possible hook budget, e.g. max hooks x 10s + 5s.
| try { | ||
| const tmpPath = `${resultPath}.${process.pid}.tmp`; | ||
| await writeFile(tmpPath, JSON.stringify(response), "utf8"); | ||
| await rename(tmpPath, resultPath); |
There was a problem hiding this comment.
Minor: rename() may fail on Windows when resultPath already exists.
On line 117, rename(tmpPath, resultPath) can fail with EPERM/EEXIST on Windows if a resultPath already exists (e.g. from a previous owner that crashed after writing but before unlinking). The catch block swallows the error silently and leaves the response unpublished. This makes duplicates wait the full WAIT_TIMEOUT_MS and fall back to independent evaluation.
The race is narrow: the only scenario is a crashed owner that left a stale resultPath AND a new owner that claims the same invocation key before RESULT_TTL_MS expires (5 min). Still worth hardening.
Fix: Before rename(), try unlink(resultPath) to remove any stale destination, or catch rename errors and retry with a direct writeFile(resultPath, ...). On non-Windows platforms, rename already atomically replaces the destination.
| expect(stderrSpy).toHaveBeenCalledWith("blocked once"); | ||
| expect(evaluatePolicies).not.toHaveBeenCalled(); | ||
| expect(persistHookActivity).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Suggestion: Add test for owner claim path in handleHookEvent.
The duplicate path is well-tested here (replay without evaluatePolicies or persistHookActivity), but there is no test that asserts the owner path: that when claimHookInvocation returns { role: "owner", complete, release }, the handler calls complete() with the evaluation result and release() in finally.
This is important because the owner publishes the result for duplicates to consume — if the wiring breaks, duplicates never get a response and fall back to independent evaluation silently.
Test outline: mock claimHookInvocation to return role=owner with vi.fn() callbacks, mock evaluatePolicies to return a known result, then assert complete was called with { exitCode, stdout, stderr } and release was called after handling.
Automated Code ReviewExecutive SummaryThis PR adds cross-process deduplication for Claude hook invocations when FailproofAI is installed in multiple scopes. It uses exclusive file locks to elect a single "owner" process for policy evaluation; duplicate processes wait briefly and replay the owner's response without re-evaluating policies or persisting duplicate activity/telemetry. The implementation is solid — clean separation of concerns, comprehensive tests (91 new lines of tests covering 5 scenarios), and careful fail-open design. All 2069 unit tests and 307 e2e tests pass. Four findings identified: one architectural concern about divergent output paths, one timing issue with WAIT_TIMEOUT_MS vs custom hook runtime, a Windows portability edge case, and a missing test for the owner claim wiring. Change Architecturegraph TD
A[handleHookEvent in handler.ts] --> B[claimHookInvocation in hook-invocation-dedup.ts]
B -->|"role: owner"| C[Owner evaluates policies + publishes result]
B -->|"role: duplicate"| D[Duplicate replays stored response]
B -->|"role: independent"| E[Non-Claude / legacy: normal evaluation]
C --> F[complete: writeFile + rename atomically]
C --> G[release: unlink lock on exit/error]
D --> H[Skip evaluatePolicies, persistHookActivity, trackHookEvent]
style B fill:#90EE90
style C fill:#87CEEB
style D fill:#87CEEB
style E fill:#FFD700
Legend: Green = New module | Blue = New code paths | Yellow = Existing path (pass-through) Breaking ChangesNo breaking changes detected. The new dedup module is only active for Claude payloads with both session_id and tool_use_id. Other integrations (Codex, Cursor, Hermes, Goose, etc.) receive Issues Found
Logical / Bug AnalysisThread safety of lock acquisition (lines 102-133):
Fail-open design (lines 141-144):
Result TTL and cleanup (lines 44-59):
Polling loop (lines 135-140):
Handler integration (handler.ts lines 202-208 and 324-330):
invocationKey uniqueness (lines 32-42):
Evidence — Build & Test ResultsLint: 0 errors (5 pre-existing warnings in unrelated files — TypeScript: pass Unit Tests: E2E Tests: Production Build: pass (Next.js build completed, standalone mode: 1519 files / 33.42 MB) Issue LinkageThis PR addresses #58 — deduplication of multi-scope hook invocations for Claude Code. The implementation fully matches the issue description. Human Review FeedbackOne previous automated review from coderabbitai[bot] identified 3 findings. My review confirms all 3 remain valid and unaddressed in this PR version:
No human reviewers have commented on this PR yet. Suggestions
VerdictVERDICT: APPROVED Automated code review. 2026-07-16 12:57 UTC |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. All tests pass (2069 unit + 307 e2e). 4 findings posted as inline comments — none blocking. Well-designed cross-process dedup with correct fail-open semantics.
|
Tip For best results, initiate chat on the files or code changes.
The timeout alignment, Windows-safe result publishing, and owner-path handler test remain worthwhile follow-ups; the output-path concern is lower risk but worth keeping in mind if response formatting moves outside policy evaluation. |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
|
Your PR is awaiting review by a moderator. Till then you can join the Discord for conversation: https://discord.gg/qaFM2uYFb |
|
Your PR is awaiting review by a moderator. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
|
Your PR is awaiting review by a reviewer. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
|
Really solid — lock/replay/fail-open is the right shape and the tests are more thorough than most of what comes through here. One thing bit me when I poked at it: I planted a stale lock (owner killed after claiming but before it publishes) and the sibling process sat there the full 12s before failing open. Worth dropping that ceiling a long way, or writing the pid into the lock so a dead owner can be spotted rather than waited out. Bigger question though — the key is session + tool_use_id but not the binary. Today both scopes evaluate and any deny wins; with this, whoever grabs the lock decides. If user scope is on |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Code Review: #534 — fix(hooks): deduplicate multi-scope invocations
Verdict: APPROVED
Summary
This PR adds cross-process deduplication for Claude hook invocations when installed in multiple config scopes (user/project/local). The solution uses filesystem-based exclusive locking to elect one "owner" process; duplicate processes replay its response without re-evaluating policies, persisting activity, or firing telemetry.
What Changed
src/hooks/hook-invocation-dedup.ts(+149 new) — Core dedup module: file-lock owner election, atomic result publishing, polling__tests__/hooks/hook-invocation-dedup.test.ts(+91 new) — 4 tests: owner election, isolation, takeover, non-Claude passthroughsrc/hooks/handler.ts(+18) — 3 integration points: early-return for duplicates, complete() after evaluation, release() in finally__tests__/hooks/handler.test.ts(+26) — 1 integration test verifying duplicates skip evaluation/persistence
Correctness
- SHA-256 hashing of
cli\|eventType\|sessionId\|toolUseId— unique per invocation open(lockPath, "wx")provides atomic exclusive file creation- Atomic result publishing via writeFile(tmp) then rename(tmp, resultPath)
- Early return before config loading, policy evaluation, activity persistence, telemetry
- Duplicate replay preserves exact exit code, stdout, stderr — enforcement not weakened
- release() in finally handles owner-exits-before-publishing takeover
- 5-min TTL cleanup prevents unbounded disk growth
- 12s timeout with 10ms poll interval — generous owner window, bounded duplicate wait
Security
- No secrets, tokens, or credentials
- Cache in
~/.failproofai/cache/hook-invocations/— same directory family as existing config - Hash inputs are deterministic identifiers — no user-controlled paths or injection
- Fail-open: any filesystem error returns
{ role: "independent" }— never blocks enforcement
Code Quality
- Clean separation: dedup module is self-contained, handler integration is 3 touch points
- Discriminated union type makes owner/duplicate/independent control flow type-safe
- Every I/O operation wrapped in try-catch with best-effort semantics
- Dedup limited to
cli === "claude"with stable session_id + tool_use_id — other integrations pass through as independent - Test-only escape hatch via
setHookInvocationDedupDirForTests()
Testing
- 4 new unit tests + 1 handler integration test — all passing
- Default mock of claimHookInvocation returns { role: "independent" } — 58 existing handler tests unaffected
- Full suite: 122 files, 2069 tests pass
- All CI checks green (build, quality, docs, test matrix, e2e, security scans)
Minor Observations (non-blocking)
- Polling loop uses 10ms interval; fs.watch could deliver lower latency (negligible in practice)
- removeOldEntries reads all entries each invocation; fire-and-forget prevents blocking
- setHookInvocationDedupDirForTests is production-exported; naming convention is clear
Looks Good
- Fail-open is the right safety property for an optimization
- Discriminated union makes three-way control flow explicit
- Handler integration is surgically minimal
- Tests cover full state machine: owner wins -> publishes -> duplicate replays -> third sees cached
- No regression risk
Reviewed by Hermes Agent
3 participants
Problem
Claude Code runs every matching user, project, and local hook registration. When Failproof AI is installed in more than one scope, each process receives the same hook payload and independently evaluates policies, writes activity, and sends telemetry. This creates duplicate dashboard rows and inflated statistics, and repeats potentially expensive custom policy evaluation.
Solution
Add a runtime cross-process deduplication guard for Claude hook payloads that have stable
session_idandtool_use_idvalues.Deduplication is intentionally limited to Claude payloads with stable invocation identifiers. Other integrations and identifier-less lifecycle events retain their existing behavior, avoiding accidental suppression of legitimate repeated events.
User impact
A tool call protected by hooks in multiple Claude scopes now produces one policy evaluation and one dashboard/statistics entry. Every duplicate process receives the same decision, including deny exit codes and response payloads, so deduplication does not weaken enforcement.
Tests
CONTRIBUTING.md gates
bun run lint— pass (0 errors; 5 pre-existing warnings in unrelated files)bunx tsc --noEmit— passbun run test:run— pass, 122 files / 2069 testsbun run build— pass, production Next.js build completedFixes #58.
Prepared with AI assistance and manually reviewed and validated before submission.
Summary by CodeRabbit