-
Notifications
You must be signed in to change notification settings - Fork 31
fix(hooks): deduplicate hook handler invocations when installed in mu… #621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| // @vitest-environment node | ||
| import { describe, it, expect, beforeEach, vi } from "vitest"; | ||
|
|
||
| vi.mock("node:fs", async () => { | ||
| const actual = await vi.importActual<typeof import("node:fs")>("node:fs"); | ||
| let mockStore: Record<string, string> = {}; | ||
| return { | ||
| ...actual, | ||
| existsSync: vi.fn((path: string) => path in mockStore), | ||
| readFileSync: vi.fn((path: string) => mockStore[path] ?? "{}"), | ||
| writeFileSync: vi.fn((path: string, content: string) => { | ||
| mockStore[path] = content; | ||
| }), | ||
| mkdirSync: vi.fn(), | ||
| __resetMockStore: () => { | ||
| mockStore = {}; | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| describe("hooks/dedup-invocation", () => { | ||
| beforeEach(async () => { | ||
| vi.resetAllMocks(); | ||
| const fs = await import("node:fs"); | ||
| (fs as unknown as { __resetMockStore: () => void }).__resetMockStore(); | ||
| }); | ||
|
|
||
| it("returns isDuplicate: false on first invocation", async () => { | ||
| const { isDuplicateInvocation } = await import("../../src/hooks/dedup-invocation"); | ||
| const result = isDuplicateInvocation("claude", "PreToolUse", { | ||
| session_id: "s1", | ||
| tool_name: "Bash", | ||
| tool_input: { command: "ls" }, | ||
| }); | ||
| expect(result.isDuplicate).toBe(false); | ||
| }); | ||
|
|
||
| it("returns isDuplicate: true when identical invocation occurs within deduplication window", async () => { | ||
| const { isDuplicateInvocation, recordInvocation } = await import("../../src/hooks/dedup-invocation"); | ||
| const payload = { | ||
| session_id: "s1", | ||
| tool_name: "Bash", | ||
| tool_input: { command: "ls" }, | ||
| }; | ||
|
|
||
| recordInvocation("claude", "PreToolUse", payload, 0); | ||
|
|
||
| const check = isDuplicateInvocation("claude", "PreToolUse", payload); | ||
| expect(check.isDuplicate).toBe(true); | ||
| expect(check.exitCode).toBe(0); | ||
| }); | ||
|
|
||
| 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); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| import { readFileSync, writeFileSync, existsSync, mkdirSync } from "node:fs"; | ||
| import { join } from "node:path"; | ||
| import { homedir } from "node:os"; | ||
|
|
||
| const DEDUP_CACHE_DIR = join(homedir(), ".failproofai", "cache"); | ||
| const DEDUP_CACHE_FILE = join(DEDUP_CACHE_DIR, "dedup-invocations.json"); | ||
| const DEDUP_WINDOW_MS = 2000; | ||
|
|
||
| interface DedupRecord { | ||
| key: string; | ||
| timestamp: number; | ||
| exitCode: number; | ||
| } | ||
|
Comment on lines
+9
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Replay the full hook response, not only the exit code. Normal handling writes
As per coding guidelines, “For stop events, preserve each CLI's retry semantics.” 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| export function isDuplicateInvocation( | ||
| cli: string, | ||
| eventType: string, | ||
| parsed: Record<string, unknown>, | ||
| ): { isDuplicate: boolean; exitCode: number } { | ||
| try { | ||
| 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}`; | ||
|
Comment on lines
+21
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Do not persist raw tool input in cache keys. The JSON cache stores the full serialized
📍 Affects 1 file
🤖 Prompt for AI Agents |
||
|
|
||
| const now = Date.now(); | ||
| let records: Record<string, DedupRecord> = {}; | ||
|
|
||
| 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 }; | ||
|
Comment on lines
+29
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Make duplicate claiming atomic. Concurrent hook processes can all read an empty cache before any reaches
📍 Affects 1 file
🤖 Prompt for AI Agents |
||
| } | ||
| } catch { | ||
| // Fail-open: if dedup check fails, treat as not duplicate | ||
| } | ||
| return { isDuplicate: false, exitCode: 0 }; | ||
| } | ||
|
|
||
| export function recordInvocation( | ||
| cli: string, | ||
| eventType: string, | ||
| parsed: Record<string, unknown>, | ||
| exitCode: number, | ||
| ): void { | ||
| try { | ||
| 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}`; | ||
|
|
||
| const now = Date.now(); | ||
| let records: Record<string, DedupRecord> = {}; | ||
|
|
||
| mkdirSync(DEDUP_CACHE_DIR, { recursive: true }); | ||
|
|
||
| if (existsSync(DEDUP_CACHE_FILE)) { | ||
| try { | ||
| const raw = readFileSync(DEDUP_CACHE_FILE, "utf8"); | ||
| records = JSON.parse(raw) as Record<string, DedupRecord>; | ||
| } catch { | ||
| records = {}; | ||
| } | ||
| } | ||
|
|
||
| // Clean up stale records older than 10 seconds to keep cache small | ||
| const cleaned: Record<string, DedupRecord> = {}; | ||
| for (const [k, v] of Object.entries(records)) { | ||
| if (now - v.timestamp < 10_000) { | ||
| cleaned[k] = v; | ||
| } | ||
| } | ||
|
|
||
| cleaned[key] = { key, timestamp: now, exitCode }; | ||
| writeFileSync(DEDUP_CACHE_FILE, JSON.stringify(cleaned), "utf8"); | ||
| } catch { | ||
| // Non-blocking write | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the session-ID key branch.
This case only changes
tool_input; add a same-input/different-session_idassertion expectingisDuplicate: false.As per coding guidelines, “Add corresponding unit tests for every new or changed behavior.”
🤖 Prompt for AI Agents
Source: Coding guidelines