feat: integrate Cursor, Copilot, OpenCode, Gemini, and Pi IDE support with comprehensive tests#131
Closed
yashupadhyayy1 wants to merge 34 commits into
Closed
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a multi‑integration hook subsystem: integration registry (Claude/Cursor/Gemini/Copilot/Codex/opencode/pi), payload normalization, integration-aware install/list/remove, process+storage dedupe, virtual JSONL logging, integration-specific policy outputs/hard-stop behavior, Copilot sync utilities, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (bin/failproofai.mjs)
participant Handler as Handler (src/hooks/handler.ts)
participant Integration as Integration Registry
participant Evaluator as Policy Evaluator
participant Store as Hook Activity Store
participant Dedup as Dedup Cache
CLI->>Handler: handleHookEvent(eventType, cliOverride?)
Handler->>Integration: detect/normalize integration & canonicalize event
Handler->>Dedup: tryAcquireFiringLock / process-wide dedupe
alt already active
Dedup-->>Handler: exit 0
else
Handler->>Evaluator: evaluatePolicies(ctx)
Evaluator-->>Handler: {decision, hardStop, stdout, stderr}
Handler->>Store: persistHookActivity(entry)
Store-->>Handler: written or skipped (duplicate)
Handler->>Handler: writeVirtualLogEntry (non-Claude)
Handler-->>CLI: return {exitCode, hardStop}
end
sequenceDiagram
participant User as User CLI
participant Manager as Hook Manager
participant Integration as Copilot Integration
participant Settings as .copilot/config.json
participant RC as Shell RC
User->>Manager: installHooks(..., integration='copilot')
Manager->>Integration: getSettingsPath(scope)
Integration->>Settings: read, merge, dedupe hooks
Integration->>Settings: write updated settings
Integration->>RC: append copilot-sync marker to rc
Manager-->>User: installation complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
app/components/project-list.tsx (1)
278-289:⚠️ Potential issue | 🟡 MinorUpdate the empty-state colspan for the new Integration column.
The table now renders 5 columns, but the empty row still spans 4, so the empty-state cell no longer covers the full table width.
Proposed fix
- <td colSpan={4} className="px-4 py-8 text-center text-muted-foreground"> + <td colSpan={5} className="px-4 py-8 text-center text-muted-foreground">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/project-list.tsx` around lines 278 - 289, The empty-state row in project-list.tsx still uses colSpan={4} but the table now has 5 columns (an Integration column was added), so update the empty-state cell (the td rendered when paginatedFolders.length === 0) to use colSpan={5} so the message spans the full table width.__tests__/e2e/hooks/custom-hooks.e2e.test.ts (1)
120-135:⚠️ Potential issue | 🟡 MinorPreserve the short-circuit assertion by checking the custom reason is absent.
This test is meant to prove the custom hook never runs after builtin deny.
stderrmatching/sudo/iverifies the builtin reason, but not the absence of the custom deny output.Proposed fix
// Builtin deny — stderr should mention block-sudo, not custom assertPreToolUseDeny(result); expect(result.stderr).toMatch(/sudo/i); + expect(result.stderr).not.toContain("custom blocked");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/hooks/custom-hooks.e2e.test.ts` around lines 120 - 135, The test "builtin fires before custom: builtin deny short-circuits, custom never runs" currently only asserts the builtin deny via assertPreToolUseDeny and a sudo stderr match; add an assertion that the custom deny message is absent to preserve the short-circuit guarantee. After runHook(...) and assertPreToolUseDeny(result), check that result.stderr does not contain the custom deny text (e.g. "custom blocked") or the custom policy name added via customPolicies.add so the test fails if the custom hook ran.__tests__/hooks/builtin-policies.test.ts (1)
1979-2025:⚠️ Potential issue | 🟡 MinorTest description/assertion mismatch across workflow policies.
Many
require-commit-before-stop,require-push-before-stop, andrequire-pr-before-stopcases were renamed to"warns (non-blocking)"but still assertdecision === "deny". For example at lines 1979–2024 ("warns (non-blocking) …" →decisionis"deny"), and similarly at 2132–2157, 2366–2405, and 2465–2485. Meanwhilerequire-ci-green-before-stopwas correctly flipped to"allow".Readers will be misled about the actual contract under test. Either:
- revert these titles to
"denies"/"blocks"where the behavior is still a hard stop, or- if these policies are intended to become warn-style too, update the code and assertions accordingly (do not only update titles).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/builtin-policies.test.ts` around lines 1979 - 2025, The test titles for the policies require-commit-before-stop, require-push-before-stop, and require-pr-before-stop were changed to "warns (non-blocking)" but the assertions still expect a hard block (result.decision === "deny"); update either the test titles or the assertions to match the intended behavior: if these policies remain blocking, revert the test descriptions back to "denies"/"blocks" where you see the vi.mocked(execSync) cases calling policy.fn and asserting expect(result.decision).toBe("deny") (these tests reference policy.fn and makeCtx); otherwise, change the policy implementation to return a non-blocking decision (e.g., "allow" with a warning in result.reason) and update assertions to expect "allow" and check for result.reason containing "uncommitted changes". Ensure consistency between the test title, the assertion on result.decision, and the behavior of the policy.fn being invoked.src/hooks/policy-evaluator.ts (1)
186-258: 🛠️ Refactor suggestion | 🟠 MajorDuplicated hardStop/exitCode block — extract a helper.
The exact same block (lines 188–204) is repeated at lines 249–258 for the Stop-event instruct path. Any future terminal-event change (e.g., adding a new integration or renaming an event) will need to be made in both places — easy to miss.
♻️ Proposed extraction
+function resolveExitAndHardStop( + integration: string | undefined, + eventType: string, +): { exitCode: number; hardStop: boolean } { + if (integration !== "gemini" && integration !== "cursor" && integration !== "copilot") { + return { exitCode: 2, hardStop: false }; + } + const terminalEvents = ["SessionStart", "Stop", "AfterAgent", "SessionEnd"]; + if (integration === "cursor") terminalEvents.push("PostToolUse"); + return { exitCode: 0, hardStop: terminalEvents.includes(eventType) }; +}Then replace both sites with:
- let exitCode = 2; - let hardStop = false; - if (session?.integration === "gemini" || session?.integration === "cursor" || session?.integration === "copilot") { - exitCode = 0; - const terminalEvents = ["SessionStart", "Stop", "AfterAgent", "SessionEnd"]; - if (session?.integration === "cursor") { - terminalEvents.push("PostToolUse"); - } - hardStop = terminalEvents.includes(eventType); - } + const { exitCode, hardStop } = resolveExitAndHardStop(session?.integration, eventType);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/policy-evaluator.ts` around lines 186 - 258, Duplicate logic computing exitCode and hardStop (based on session.integration and eventType) appears in the deny and Stop->instruct branches; extract it into a single helper like computeExitCodeAndHardStop(session, eventType) that returns { exitCode, hardStop }, implement the integration-specific rules (gemini/cursor/copilot => exitCode=0 and cursor adds "PostToolUse" to terminalEvents) inside that function, and replace both inline blocks that set exitCode/hardStop with calls to this helper (use the returned values where exitCode and hardStop are expected).app/policies/hooks-client.tsx (1)
254-254:⚠️ Potential issue | 🟡 MinorUpdate the expanded row span for the new Integration column.
Adding the Integration column brings the table to 11 columns, but
DetailPanelstill spans 10, leaving the expanded panel misaligned.Proposed fix
- <td colSpan={10} className="px-0 py-0"> + <td colSpan={11} className="px-0 py-0">Also applies to: 490-490
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/policies/hooks-client.tsx` at line 254, The DetailPanel's expanded-row cell is still using colSpan={10} and needs to be updated to match the new 11-column table: change the two occurrences of <td colSpan={10}> in app/policies/hooks-client.tsx (the DetailPanel/expanded row JSX) to <td colSpan={11}> so the expanded panel aligns with the new Integration column.src/hooks/builtin-policies.ts (1)
1309-1323:⚠️ Potential issue | 🟠 MajorKeep
require-ci-green-before-stopenforcing CI status.The policy is still described as requiring CI to pass, but failing and pending checks now return
allow, so Stop is no longer blocked when CI is red or still running.🛡️ Proposed fix
if (failing.length > 0) { const names = failing.map((r) => `"${r.name}"`).join(", "); - return allow( - `⚠ Warning: CI checks are failing on branch "${branch}": ${names}. Consider fixing these before merging.`, + return deny( + `CI checks are failing on branch "${branch}": ${names}. Fix these before stopping.`, ); } @@ if (pending.length > 0) { const names = pending.map((r) => `"${r.name}"`).join(", "); - return allow( - `⚠ Warning: CI checks are still running on branch "${branch}": ${names}. You can continue, but check will be needed before merging.`, + return deny( + `CI checks are still running on branch "${branch}": ${names}. Wait for them to finish before stopping.`, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/builtin-policies.ts` around lines 1309 - 1323, The policy currently returns allow(...) when checks are failing (failing) or still running (pending), which defeats the require-ci-green-before-stop intent; change those returns to call stop(...) (or the policy's blocking result function) instead of allow, keeping the same message and using the existing variables (failing, pending, branch, names) so the policy blocks stops while CI is red or in progress.
🟠 Major comments (20)
lib/copilot-sessions.ts-42-46 (1)
42-46:⚠️ Potential issue | 🟠 MajorAvoid reading every Copilot event log into memory on the list path.
countLines()loads eachevents.jsonlfully, andgetCopilotSessions()runs this across all sessions. Large Copilot logs can make the dashboard slow or memory-heavy just to render a session list. Stream the file or defereventCountto the detail view.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/copilot-sessions.ts` around lines 42 - 46, The current countLines function reads the whole events.jsonl into memory and getCopilotSessions calls it for every session, causing high memory use; change countLines to stream the file (use fs.createReadStream + readline.createInterface) and count lines incrementally, or better yet remove the call from getCopilotSessions and defer computing eventCount until the session detail view (return null/undefined or omit eventCount in getCopilotSessions), ensuring references to countLines, getCopilotSessions, events.jsonl and eventCount are updated accordingly so list rendering no longer reads full files into memory.lib/projects.ts-155-161 (1)
155-161:⚠️ Potential issue | 🟠 MajorDon’t drop all standalone OpenCode sessions when any virtual OpenCode project exists.
isOpencodeSessionMerged()returns true for everyses_*file as soon as there is any virtual folder withopencodeinsources, because virtual folder names are encoded CWDs and will almost always differ from the session ID. This hides unrelated OpenCode sessions from the project list. Use an exact session/workspace mapping, or keep standaloneses_*entries until that mapping exists.Safer interim fix
- if (folder.source === "opencode" && folder.name.startsWith("ses_") && isOpencodeSessionMerged(folder.name, virtualFolders)) { - continue; - }Also applies to: 181-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/projects.ts` around lines 155 - 161, isOpencodeSessionMerged currently treats any virtual folder with "opencode" as merging all ses_* sessions because it only checks f.name !== sessionId; update the logic in isOpencodeSessionMerged(sessionId: string, virtualFolders: ProjectFolder[]) to require an exact mapping between the sessionId and the virtual folder (e.g., compare decoded CWD or an explicit session-to-workspace key) before returning true so unrelated ses_* entries are not hidden; also make the same change to the similar block that the reviewer noted (the other function/condition around lines 181-185) so standalone ses_* entries are preserved until a concrete mapping exists.lib/copilot-sessions.ts-13-14 (1)
13-14:⚠️ Potential issue | 🟠 MajorUse the centralized Copilot path helper here too.
getCopilotSessionStateDir()ignoresCOPILOT_SESSION_STATE_PATH, while the rest of the new path layer supports that override. This makes Copilot session discovery read the real home directory in tests or redirected deployments.Proposed fix
-import { homedir } from "os"; import { runtimeCache } from "./runtime-cache"; import { logWarn, logError } from "./logger"; import { formatDate } from "./utils"; +import { getCopilotSessionStatePath } from "./paths"; @@ export function getCopilotSessionStateDir(): string { - return join(homedir(), ".copilot", "session-state"); + return getCopilotSessionStatePath(); }Also applies to: 27-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/copilot-sessions.ts` around lines 13 - 14, getCopilotSessionStateDir currently reads the real home directory (via homedir()) and ignores COPILOT_SESSION_STATE_PATH; replace that logic with the centralized Copilot path helper so the override is respected. Update getCopilotSessionStateDir (and the other occurrences around the same area) to call the existing centralized Copilot path helper function that returns the session state dir (the helper that already honors COPILOT_SESSION_STATE_PATH) instead of using homedir(), ensuring discovery honors the override.lib/paths.ts-57-70 (1)
57-70:⚠️ Potential issue | 🟠 MajorDon’t rely on
encodeCwd()being reversible for hyphenated paths.The new helper is documented as the inverse of
decodeFolderName(), but/repo/my-projectencodes to-repo-my-projectand decodes as/repo/my/project. Any virtual session lookup that decodes the folder name back to a CWD will miss projects with hyphens. Prefer comparing encoded names directly, e.g.encodeCwd(entry.cwd) === basename(projectPath), instead of decoding to recover the original path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/paths.ts` around lines 57 - 70, encodeCwd() is not reversible for paths containing hyphens (e.g., "/repo/my-project" → "-repo-my-project" decodes incorrectly), so update call sites that attempt to recover original CWD via decodeFolderName() to instead compare encoded forms directly: use encodeCwd(entry.cwd) === <encoded project folder> (or compare to path basename after encoding) rather than decoding folder names; search for usages of decodeFolderName and replace those comparisons with an equality check against encodeCwd(...) or precomputed encoded folder names to avoid missing hyphenated projects.lib/projects.ts-313-317 (1)
313-317:⚠️ Potential issue | 🟠 MajorActually skip activity entries without a CWD.
The comment says empty CWD entries are skipped, but
if (entry.cwd && entry.cwd !== cwd) continuelets missing-CWD entries through for every project. Those sessions will appear globally.Proposed fix
- // Match by cwd — empty cwd entries are skipped - if (entry.cwd && entry.cwd !== cwd) continue; + // Match by cwd — empty cwd entries are skipped + if (!entry.cwd || entry.cwd !== cwd) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/projects.ts` around lines 313 - 317, The loop over allActivity in lib/projects.ts incorrectly allows entries with missing/empty cwd to pass through; update the guard that currently reads "if (entry.cwd && entry.cwd !== cwd) continue" to instead skip entries without a cwd or with a different cwd (i.e., require a non-empty cwd equal to the local cwd), keeping the existing checks for entry.sessionId and VIRTUAL_INTEGRATIONS unchanged so that entries with no cwd are not treated as global; look for the loop using the variable entry and the cwd/VIRTUAL_INTEGRATIONS checks to modify..failproofai/policies-config.json-2-2 (1)
2-2:⚠️ Potential issue | 🟠 MajorAvoid committing a no-op policy config at the project root.
Line 2 disables all configured policies. If the CLI loads this checked-in config outside tests, the project’s security/safety checks silently stop running. Please keep no-op policy config in test fixtures/env setup, or restore the intended default policy set here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.failproofai/policies-config.json at line 2, The checked-in config currently sets the "enabledPolicies" key to an empty array which disables all policies; update the .failproofai/policies-config.json so that "enabledPolicies" either lists the intended default policy IDs (restore the project's default policy set) or remove the key entirely so the CLI falls back to defaults, and ensure any no-op/empty configs are only used in test fixtures or env-specific overrides rather than committed at the repo root.lib/projects.ts-188-203 (1)
188-203:⚠️ Potential issue | 🟠 MajorPreserve all merged
sources, not just the primary source.Virtual folders can already carry multiple integrations in
folder.sources, but the insert/merge path collapses them to[folder.source]. Multi-integration badges will show only the first integration.Proposed fix
const existing = projectMap.get(folder.name); + const folderSources = folder.sources?.length + ? folder.sources + : folder.source + ? [folder.source] + : []; if (existing) { - // Merge sources - if (folder.source && !existing.sources.includes(folder.source)) { - existing.sources.push(folder.source); + for (const source of folderSources) { + if (!existing.sources.includes(source)) { + existing.sources.push(source); + } } @@ projectMap.set(folder.name, { ...folder, - sources: folder.source ? [folder.source] : [], + sources: [...folderSources], }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/projects.ts` around lines 188 - 203, The merge path currently collapses folder.sources to just folder.source when inserting or merging (in the projectMap handling around variables projectMap, existing, folder and existing.sources), so multi-integration entries are lost; change both the insert and merge logic to incorporate folder.sources (if present) rather than only folder.source, merging arrays into existing.sources and deduplicating values (e.g., use a Set or filter) and when creating a new map entry set sources to folder.sources || (folder.source ? [folder.source] : []) so all provided integrations are preserved.lib/projects.ts-270-272 (1)
270-272:⚠️ Potential issue | 🟠 MajorValidate
ses_names before joining OpenCode paths.The OpenCode branch accepts any string starting with
ses_, including names with path separators. This bypasses the root-escape checks used for Claude and Copilot. Add a strict session-name regex and resolve-prefix check before returning the path.Proposed fix
+const OPENCODE_SESSION_RE = /^ses_[A-Za-z0-9_-]+$/; + @@ if (name.startsWith("ses_")) { - return { path: join(getOpencodeStoragePath(), "session_diff", `${name}.json`), source: "opencode" }; + if (!OPENCODE_SESSION_RE.test(name)) throw new RangeError("Invalid OpenCode session ID"); + const sessionDiffRoot = resolve(join(getOpencodeStoragePath(), "session_diff")); + const candidate = resolve(join(sessionDiffRoot, `${name}.json`)); + if (!candidate.startsWith(sessionDiffRoot + sep)) throw new RangeError("OpenCode path escapes root"); + return { path: candidate, source: "opencode" }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/projects.ts` around lines 270 - 272, The ses_ branch currently accepts any string starting with "ses_" and directly joins it into the OpenCode path; tighten this by first validating the session name with a strict regex (e.g. require whole name to match something like ^ses_[A-Za-z0-9_-]+$) and only proceed if it passes, then build an absolute path using path.resolve(getOpencodeStoragePath(), "session_diff", `${name}.json`) and perform a resolve-prefix check to ensure the resolved path starts with path.resolve(getOpencodeStoragePath()) before returning it; update the if (name.startsWith("ses_")) block in lib/projects.ts to enforce these checks (use the same symbols name, getOpencodeStoragePath, join/resolve and the session_diff folder) and return an error or null if validation/resolution fails.__tests__/e2e/hooks/gemini-integration.e2e.test.ts-14-23 (1)
14-23:⚠️ Potential issue | 🟠 MajorDon't
rm -rfthe developer's real~/.failproofai/cache/dedup.
DEDUP_DIRis resolved fromhomedir()and unconditionally removed inbeforeEach. On a developer or CI machine where other tests/sessions reuse~/.failproofai, this wipes live state and creates cross-test interference (the comment even acknowledges Cursor/Copilot E2Es share the same bucket).Isolate HOME for the subprocess instead (the hook-runner helper already supports
opts.homeDir), e.g.:- // Cursor and Copilot e2e tests share the same SESSION_ID + sudo fingerprint as - // Gemini, so their firing-lock file (5s bucket) can still be on disk when - // Gemini runs and block this test with "instant-catch twin". Clear it. - const DEDUP_DIR = resolve(homedir(), ".failproofai", "cache", "dedup"); + // Give each test run a dedicated HOME so dedup state is isolated. + const FAKE_HOME = resolve(__dirname, "../../fixtures/gemini-home");…and pass
HOME: FAKE_HOMEin the spawned env (alongsidemkdirSync(FAKE_HOME, { recursive: true })/ cleanup inafterEach). That removes both the blast-radius concern and the inter-test coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/hooks/gemini-integration.e2e.test.ts` around lines 14 - 23, The test currently computes DEDUP_DIR from homedir() and unconditionally deletes it in beforeEach; change the setup to create and use an isolated fake HOME (e.g., const FAKE_HOME = resolve(tmpdir(), "failproofai-test-<unique>")), ensure beforeEach mkdirSync(FAKE_HOME, { recursive: true }) and compute DEDUP_DIR from FAKE_HOME instead of homedir(), and when spawning the hook-runner use its opts.homeDir or pass HOME: FAKE_HOME in the spawned env; finally, remove the fake HOME in afterEach instead of touching the real ~/.failproofai to avoid blasting developer state (refer to DEDUP_DIR, beforeEach, afterEach, and the hook-runner opts.homeDir/HOME env).scripts/codex-trace.mjs-4-4 (1)
4-4:⚠️ Potential issue | 🟠 Major
.mjsimporting a.tssource file will fail under plainnode.
codex:traceis wired asnode scripts/codex-trace.mjs, which imports from../src/codex/trace-parser(a.tsfile). Node cannot resolve.tsextensions without a loader (e.g.,tsx,ts-node) or--experimental-strip-types(Node ≥ 22.6). The minimum Node version inpackage.jsonis 20.9.0, which does not support type-stripping. Nodist/codex/trace-parser.jsbuild artifact exists.Point the import at a built artifact (e.g.,
../dist/codex/trace-parser.js) with a prior build step, invoke viabunortsxinpackage.json, or upgrade the minimum Node version and use--experimental-strip-typesexplicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/codex-trace.mjs` at line 4, scripts/codex-trace.mjs currently imports the TypeScript source ../src/codex/trace-parser which fails under plain Node; update the invocation to use a built JS artifact or run with a TS loader: either change the import in scripts/codex-trace.mjs to point to ../dist/codex/trace-parser.js (ensuring a prior build step that outputs that file) or update the package.json codex:trace script to run the file with a TS-aware runner (e.g., "tsx scripts/codex-trace.mjs" or "node --experimental-strip-types" on Node ≥22.6) while keeping the call to writeCodexTraceFile unchanged; pick one approach and implement the corresponding build/preflight step so Node can resolve the module at runtime.__tests__/hooks/policy-evaluator.test.ts-753-756 (1)
753-756:⚠️ Potential issue | 🟠 MajorDo not use
pias the unknown integration sentinel.Pi is a supported integration in this PR, so this test codifies it as the default fallback path and can hide a missing Pi blocking protocol. Use a truly unknown key here and add a separate Pi-specific expectation.
Proposed adjustment
it("uses default specialized style for unknown integration", async () => { - const result = await evaluatePolicies("PreToolUse", { tool_name: "Bash" }, { integration: "pi" as any }); + const result = await evaluatePolicies("PreToolUse", { tool_name: "Bash" }, { integration: "unknown-integration" as any }); expect(result.exitCode).toBe(2); expect(result.stderr).toBe("[FailproofAI Security Stop] Policy: blocker - Forbidden"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/policy-evaluator.test.ts` around lines 753 - 756, The test uses "pi" as the unknown integration sentinel but Pi is supported in this PR; update the test that calls evaluatePolicies("PreToolUse", { tool_name: "Bash" }, { integration: "pi" as any }) to use a truly unknown key (e.g., "unknown_integration") so the default-specialized-style path is exercised, and add a separate assertion/test case that explicitly checks the Pi-specific behavior by calling evaluatePolicies with integration: "pi" and asserting the expected Pi outcome; update the test name or add a new it(...) block accordingly to keep behaviors distinct.__tests__/e2e/hooks/cursor-integration.e2e.test.ts-12-18 (1)
12-18:⚠️ Potential issue | 🟠 MajorIsolate HOME before clearing the dedup cache.
This deletes the real user’s
~/.failproofai/cache/dedupduring tests. Use a per-test HOME/USERPROFILE under the fixture and pass it to everyexecSync/spawnSynccall.Proposed direction
-// Firing-lock files can persist across test cases. Clear them. -const DEDUP_DIR = resolve(require("node:os").homedir(), ".failproofai", "cache", "dedup"); +const TEST_HOME = resolve(PROJECT_DIR, ".home"); +const DEDUP_DIR = resolve(TEST_HOME, ".failproofai", "cache", "dedup"); +const testEnv = (extra: NodeJS.ProcessEnv = {}) => ({ + ...process.env, + HOME: TEST_HOME, + USERPROFILE: TEST_HOME, + FAILPROOFAI_DIST_PATH: process.cwd(), + ...extra, +}); describe("E2E: Cursor Integration", () => { beforeEach(() => { if (existsSync(PROJECT_DIR)) rmSync(PROJECT_DIR, { recursive: true, force: true }); if (existsSync(DEDUP_DIR)) rmSync(DEDUP_DIR, { recursive: true, force: true }); mkdirSync(PROJECT_DIR, { recursive: true }); + mkdirSync(TEST_HOME, { recursive: true });Then replace command envs with
env: testEnv()orenv: testEnv({ FAILPROOFAI_SKIP_KILL: "true" }).Also applies to: 31-34, 44-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/hooks/cursor-integration.e2e.test.ts` around lines 12 - 18, The tests clear the global dedup cache by using DEDUP_DIR in the beforeEach block (and similar blocks at the other occurrences), which can delete the real user's ~/.failproofai; modify the setup to isolate HOME by setting a per-test temporary HOME/USERPROFILE (use the existing PROJECT_DIR fixture or a temp dir) and ensure all shell invocations use a controlled environment by replacing direct execSync/spawnSync env usage with env: testEnv() or env: testEnv({ FAILPROOFAI_SKIP_KILL: "true" }); update the beforeEach that computes DEDUP_DIR and any other places (lines referenced around DEDUP_DIR, beforeEach, and test harness calls) to compute the dedup path under that per-test HOME and pass testEnv() into every execSync/spawnSync call so tests only touch the isolated cache.lib/log-entries.ts-820-829 (1)
820-829:⚠️ Potential issue | 🟠 MajorExclude
claude-codefrom the virtual-integration fallback.
INTEGRATION_TYPESincludesclaude-code, so a normal Claude session with a matching activity-store row can return the synthesized activity entries before reading the real.claude/projects/<session>.jsonltranscript. That can hide the actual conversation/tool-result pairing from the dashboard.🐛 Proposed fix
- const VIRTUAL_INTEGRATIONS = INTEGRATION_TYPES as unknown as string[]; + const VIRTUAL_INTEGRATIONS = (INTEGRATION_TYPES as readonly string[]) + .filter((integration) => integration !== "claude-code");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/log-entries.ts` around lines 820 - 829, INTEGRATION_TYPES currently contains "claude-code" which should be excluded from the virtual-integration fallback; update the logic around VIRTUAL_INTEGRATIONS/getAllHookActivityEntries so that the list used for matching does not include "claude-code" (e.g., build VIRTUAL_INTEGRATIONS = (INTEGRATION_TYPES as unknown as string[]).filter(i => i !== "claude-code") or add an extra predicate to the matchingEntries filter to skip entries where entry.integration === "claude-code"); ensure you update references to VIRTUAL_INTEGRATIONS and the matchingEntries filter so sessions for Claude read the real .claude transcript instead of synthesized hook activity entries.src/hooks/hook-activity-store.ts-393-446 (1)
393-446:⚠️ Potential issue | 🟠 MajorRequire a real
sessionIdbefore write-time deduplication.Line 416 treats
undefined === undefinedas a match, so two unrelated events withoutsessionIdbut with the same event type/decision/policy inside 5 seconds can be silently dropped. This also conflicts withcollapseDuplicates, which intentionally refuses to collapse entries without a session id.🛡️ Proposed fix
function isDuplicateInLog(entry: HookActivityEntry, filePath: string): boolean { + if (!entry.sessionId) return false; + try { const stats = statSync(filePath); if (stats.size === 0) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/hook-activity-store.ts` around lines 393 - 446, The dedup logic in isDuplicateInLog currently treats undefined sessionId values as equal; require a real sessionId before deduplicating by adding a truthiness check so both prev.sessionId and entry.sessionId must be non-empty (e.g., prev.sessionId && entry.sessionId && prev.sessionId === entry.sessionId) before proceeding to compare eventType/hookEventName/decision/policyName/toolName and the 5s time window; this keeps behavior consistent with collapseDuplicates which refuses to collapse entries without a session id and prevents unrelated events with missing sessionId from being dropped.src/hooks/builtin-policies.ts-1680-1690 (1)
1680-1690:⚠️ Potential issue | 🟠 MajorDon’t register the diagnostic policy unconditionally.
debug-promptis always active even when it is not enabled by configuration. That can add diagnostic policy results to normalUserPromptSubmithandling and activity attribution in production. Gate it behind an explicit enabled name or debug env flag.🧹 Proposed fix
- // Diagnostic policy to verify prompt capture for multi-agent support - registerPolicy( - "debug-prompt", - "Diagnostic policy to verify prompt capture", - async (ctx) => ({ - decision: "allow", - reason: `Prompt captured from ${ctx.session?.integration ?? "unknown"}`, - }), - { events: ["UserPromptSubmit"] }, - 100, - ); + if (enabledSet.has("debug-prompt") || process.env.FAILPROOFAI_DEBUG_PROMPT === "1") { + registerPolicy( + "debug-prompt", + "Diagnostic policy to verify prompt capture", + async (ctx) => ({ + decision: "allow", + reason: `Prompt captured from ${ctx.session?.integration ?? "unknown"}`, + }), + { events: ["UserPromptSubmit"] }, + 100, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/builtin-policies.ts` around lines 1680 - 1690, The diagnostic policy "debug-prompt" is being registered unconditionally via registerPolicy("debug-prompt", ...) which makes it active in production; guard the registration with an explicit config/flag check (e.g., consult a config value like enableDebugPolicies or an env var such as process.env.DEBUG_PROMPT or process.env.DEBUG_POLICIES) and only call registerPolicy for "debug-prompt" when that flag is truthy; update the code around the registerPolicy invocation so the policy registration is skipped unless the configured debug flag is enabled.src/hooks/hook-activity-store.ts-503-555 (1)
503-555:⚠️ Potential issue | 🟠 MajorTake the activity-store lock during migration rewrites.
migrateIntegrationField()rewritescurrent.jsonland archived pages while hook processes can append underpersistHookActivity()’s lock. Without taking the same lock here, a dashboard-triggered migration can overwrite a concurrent append and lose activity rows.🔒 Proposed fix
export function migrateIntegrationField(): void { ensureDir(); - if (hasMigrationRun()) return; + acquireLock(); try { + if (hasMigrationRun()) return; let migrated = 0; const currentPath = join(storeDir, CURRENT_FILE); const entries = readJsonlFile(currentPath); @@ markMigrationDone(); } catch (e) { // Non-fatal: migration failure doesn't block the system + } finally { + releaseLock(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/hook-activity-store.ts` around lines 503 - 555, migrateIntegrationField() currently rewrites current.jsonl and archive pages without holding the activity-store lock used by persistHookActivity(), risking lost appends — wrap the entire migration (the logic reading/writing CURRENT_FILE and each archive from getArchiveFiles() using readJsonlFile/writeFileSync) in the same activity-store lock used by persistHookActivity(); acquire the lock before reading/updating files and release it after markMigrationDone(), ensuring the lock API and error handling match persistHookActivity() so concurrent persistHookActivity() calls are blocked during the migration.src/hooks/handler.ts-30-281 (1)
30-281:⚠️ Potential issue | 🟠 MajorDedup lock files in
~/.failproofai/cache/dedup/are never garbage-collected.Both
tryRecordEvent(Line 219) andtryAcquireFiringLock(Line 275) write.lockfiles withflag: "wx"and never delete them. On an active developer machine with frequent hook firings (PreToolUse/PostToolUse per tool call), this directory will grow without bound — one file per event per 2s bucket. Over weeks/months this becomes hundreds of thousands of inodes, andexistsSynclookups stay cheap but directory listings (e.g.rm -rf) get painful, and on constrained systems the user can hit filesystem inode limits.Please add periodic pruning (e.g. a best-effort sweep removing any
.lockfile older than ~60s on each invocation, or on handler start) so the cache stays bounded. Something like:function pruneStaleLocks(now: number, ttlMs = 60_000): void { try { for (const name of readdirSync(DEDUP_DIR)) { const p = join(DEDUP_DIR, name); try { if (now - statSync(p).mtimeMs > ttlMs) rmSync(p, { recursive: true, force: true }); } catch { /* ignore */ } } } catch { /* ignore */ } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 30 - 281, Add a best-effort pruning routine (e.g., pruneStaleLocks(now, ttlMs = 60_000)) that scans DEDUP_DIR (and the "firing-locks" subdir), statSync each entry and rmSync any file/dir whose mtimeMs is older than ttlMs, swallowing errors; call this prune from ensureDedupDir() after creating the directory and at the start of tryRecordEvent() and tryAcquireFiringLock() (before existsSync checks) so stale .lock files are removed on each invocation and the dedup cache stays bounded.src/hooks/handler.ts-444-475 (1)
444-475:⚠️ Potential issue | 🟠 MajorUnique-event-name fallback misclassifies Cursor events as Copilot.
COPILOT_HOOK_EVENT_TYPES(preToolUse,postToolUse,sessionStart,sessionEnd,subagentStop,agentStop,userPromptSubmitted,errorOccurred) overlaps almost entirely withCURSOR_HOOK_EVENT_TYPES. For any invocation that arrives withoutintegrationOverrideorparsed.integrationand whose event name is, say,preToolUse, this branch setsintegrationType = "copilot"before the payload-shape detection on Lines 459–470 ever runs — so Cursor payloads get labeled Copilot and routed through Copilot-specific transcript paths/env lookups.The install flow always emits
--integration, so real installs aren't affected. But the "unique signature" comment isn't accurate and the fallback order should prefer payload-shape detection (which does differentiate cursor from copilot viaworkspace_roots/ camelCase + sessionId etc.). At minimum, only treat event names that are inCOPILOT_HOOK_EVENT_TYPESand not inCURSOR_HOOK_EVENT_TYPESas a unique Copilot signal — otherwise fall through to payload detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 444 - 475, The current name-based fallback in the event handler (the block that checks GEMINI_UNIQUE, COPILOT_HOOK_EVENT_TYPES, CODEX_HOOK_EVENT_TYPES and sets integrationType) misclassifies Cursor events as "copilot"; change the logic so either (a) you check payload-based detection (INTEGRATIONS.copilot.detect / .gemini.detect / .cursor.detect / etc.) before any COPILOT_HOOK_EVENT_TYPES check, or (b) tighten the Copilot-only branch to require eventType is in COPILOT_HOOK_EVENT_TYPES AND not in CURSOR_HOOK_EVENT_TYPES before setting integrationType = "copilot"; update the code around GEMINI_UNIQUE, COPILOT_HOOK_EVENT_TYPES, CURSOR_HOOK_EVENT_TYPES and the secondary detection using INTEGRATIONS.*.detect to ensure cursor payloads reach payload-shape detection first.__tests__/hooks/handler.test.ts-826-868 (1)
826-868:⚠️ Potential issue | 🟠 MajorMove test directory to
os.tmpdir()and wrap cleanup infinally.The test uses
os.homedir()to create a test directory (Line 830), which violates the established pattern in this repository (see__tests__/hooks/hook-activity-store.test.ts:37). Additionally, cleanup is not wrapped in afinallyblock, so if any assertion fails before reaching Line 866, the directory leaks on contributors' machines.Use
fs.mkdtempSync(path.join(os.tmpdir(), ...))and ensure cleanup always runs:Suggested fix
- const testDir = path.join(os.homedir(), ".failproofai-test-dedup-storage"); - _resetForTest(testDir); - - // Simulation: First record call + const testDir = fs.mkdtempSync(path.join(os.tmpdir(), "failproofai-dedup-")); + _resetForTest(testDir); + + try { + // Simulation: First record call ... - expect(lines.length).toBe(1); // ONLY ONE LINE recorded despite two persist calls - - // Cleanup - fs.rmSync(testDir, { recursive: true, force: true }); + expect(lines.length).toBe(1); + } finally { + fs.rmSync(testDir, { recursive: true, force: true }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/handler.test.ts` around lines 826 - 868, Replace usage of os.homedir() with a temp dir created via fs.mkdtempSync(path.join(os.tmpdir(), "failproofai-test-")) and ensure cleanup always runs by wrapping the test body in a try/finally: call _resetForTest(tempDir) then run the two persistHookActivity(...) calls and assertions inside try, and in finally remove the tempDir with fs.rmSync(tempDir, { recursive: true, force: true }); reference the test helpers persistHookActivity and _resetForTest and the testDir variable (rename to tempDir) so the test uses os.tmpdir() and never leaks the directory on failures.src/hooks/types.ts-155-167 (1)
155-167:⚠️ Potential issue | 🟠 MajorCorrect Gemini
BeforeModelandAfterModelcanonical event name mappings.Per Gemini CLI documentation,
BeforeModelfires before sending ANY request to the LLM (not just initial user submission), andAfterModelfires after receiving the LLM response (not after tool execution). The current mappings—BeforeModel: "UserPromptSubmit"andAfterModel: "PostToolUse"—misrepresent these semantics and will cause policies to execute at unintended lifecycle points.Policies gated on
"UserPromptSubmit"will run on every model round-trip rather than just initial prompts, and policies gated on"PostToolUse"will run after model generation rather than after tool execution. This can silently change behavior of prompt-sanitization, tool-result filtering, and other policies.Either add a comment explaining why this mapping is intentional, or update to mappings that accurately reflect Gemini's hook semantics (e.g., use
Notification/no-op, or new canonical names that distinguish model output from tool execution).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/types.ts` around lines 155 - 167, The GEMINI_EVENT_MAP currently maps BeforeModel and AfterModel incorrectly (BeforeModel -> "UserPromptSubmit" and AfterModel -> "PostToolUse"); either replace those mappings so they don't trigger user-prompt/tool-post policies (e.g., set BeforeModel: "Notification" and AfterModel: "Notification" to act as no-ops), or if the current behavior is intentional add a clear comment above GEMINI_EVENT_MAP explaining why BeforeModel and AfterModel are mapped to "UserPromptSubmit" and "PostToolUse" despite Gemini semantics; update the map entries for the symbols GEMINI_EVENT_MAP, BeforeModel, and AfterModel accordingly.
🧹 Nitpick comments (11)
__tests__/lib/projects.test.ts (1)
243-274: Add coverage for the new OpenCode resolver and virtual-session CWD filter.The invalid-name test only exercises the Claude path branch. Please add cases for
ses_names containing separators/traversal and for activity entries withoutcwdnot appearing ingetSessionFiles(). As per coding guidelines, “Always add unit tests for new behaviour. Place tests in tests/.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/projects.test.ts` around lines 243 - 274, Add unit tests covering the OpenCode resolver and virtual-session CWD filter: extend __tests__/lib/projects.test.ts to include (1) invalid ses_-prefixed names (e.g., containing path separators or traversal like "ses_../etc") passed to resolveAnyProjectPath and assert they throw RangeError, and (2) a test for getSessionFiles that feeds activity entries missing the cwd field and asserts those entries are excluded from the returned session file list; reference resolveAnyProjectPath and getSessionFiles to locate where to add these cases.__tests__/e2e/helpers/hook-runner.ts (1)
89-115: LGTM on the contract shift.The new stderr-prefix checks cover the three specialized branding strings produced by
getDenyMessageinsrc/hooks/policy-evaluator.ts, which matches the evaluator's new output shape. Consider extracting the prefix list to a shared constant so the evaluator and these assertions cannot drift:♻️ Optional dedup
+const DENY_PREFIXES = [ + "[FailproofAI", + "MANDATORY ACTION REQUIRED", + "ACTION BLOCKED BY FAILPROOFAI", +]; + export function assertPreToolUseDeny(result: HookRunResult): void { expect(result.exitCode).toBe(2); expect(result.stdout).toBe(""); - const hasPrefix = - result.stderr.includes("[FailproofAI") || - result.stderr.includes("MANDATORY ACTION REQUIRED") || - result.stderr.includes("ACTION BLOCKED BY FAILPROOFAI"); - expect(hasPrefix).toBe(true); + expect(DENY_PREFIXES.some((p) => result.stderr.includes(p))).toBe(true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/helpers/hook-runner.ts` around lines 89 - 115, Extract the deny-message prefixes used in assertPreToolUseDeny and assertPostToolUseDeny into a shared constant that can be imported from the same module that defines getDenyMessage (policy-evaluator.ts), then replace the duplicated inline checks in both functions with a membership test against that constant; ensure the constant name is descriptive (e.g., DENY_MESSAGE_PREFIXES) and exported so both the evaluator (getDenyMessage) and the test helpers (assertPreToolUseDeny/assertPostToolUseDeny) reference the same source of truth to prevent drift.__tests__/hooks/hook-activity-store.test.ts (1)
182-182: Prefer ESMimportoverrequirein a TS/ESM test file.Mixing
require("node:fs")inside a module that already uses top-levelimportis inconsistent and may break if the test config ever moves to strict ESM. MovewriteFileSyncinto the top-level import.♻️ Proposed change
-import { mkdtempSync, rmSync } from "node:fs"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; ... - const { writeFileSync } = require("node:fs"); // Manually write old-format entries without integration field🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/hook-activity-store.test.ts` at line 182, The test currently uses a runtime require for writeFileSync (const { writeFileSync } = require("node:fs")), which mixes CommonJS into an ESM/TS module; replace that require with a top-level ESM import (import { writeFileSync } from "node:fs") and remove the require line so all imports are consistent with the module system and TypeScript ESM expectations.__tests__/e2e/hooks/gemini-integration.e2e.test.ts (3)
33-69:FAILPROOFAI_DIST_PATH: process.cwd()is fragile.Setting dist path to the test runner's cwd assumes the suite is invoked from the repo root. If someone runs
vitestfrom a subdirectory (e.g.,cd __tests__ && vitest), the spawned binary won't finddist/index.js. Resolve it deterministically from__dirnamelikeBINARY_PATH:+const DIST_PATH = resolve(__dirname, "../../../dist"); ... -env: { ...process.env, FAILPROOFAI_DIST_PATH: process.cwd() } +env: { ...process.env, FAILPROOFAI_DIST_PATH: DIST_PATH }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/hooks/gemini-integration.e2e.test.ts` around lines 33 - 69, The tests set FAILPROOFAI_DIST_PATH using process.cwd(), which is brittle; change the env usage to resolve the dist path deterministically the same way BINARY_PATH is derived (use __dirname-based resolution) and replace occurrences of FAILPROOFAI_DIST_PATH: process.cwd() in the execSync/spawnSync env objects (the two places around the Gemini integration tests) so the spawned binary always points to the correct dist/index.js regardless of where vitest is invoked from.
46-47: Drop debugconsole.logs.These will clutter E2E output on every run. If they're useful for local debugging, gate them behind
process.env.DEBUG_E2Eor remove.- console.log("Gemini STDOUT:", stdout); - console.log("Gemini STDERR:", stderr);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/hooks/gemini-integration.e2e.test.ts` around lines 46 - 47, Remove the unconditional debug logging in the E2E test by either deleting the two console.log calls or gating them behind a DEBUG_E2E environment flag; locate the two console.log("Gemini STDOUT:", stdout) and console.log("Gemini STDERR:", stderr) lines in the gemini-integration.e2e.test and wrap them with an if (process.env.DEBUG_E2E) { ... } check (or remove entirely) so normal CI runs remain clean while local debugging is still possible.
66-72: UsespawnSyncfor the allow case too.
execSyncthrows on any non-zero exit, which for this test is fine but loses the stderr/stdout separation and silently aborts instead of yielding an asserting failure with diagnostics. Mirror the deny case'sspawnSyncpattern so both branches report consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/hooks/gemini-integration.e2e.test.ts` around lines 66 - 72, Replace the execSync call in the allow branch with the same spawnSync pattern used in the deny case: call spawnSync(BINARY_PATH, ["--hook", "BeforeTool", "--integration", "gemini"], { input: JSON.stringify(payload), cwd: PROJECT_DIR, env: { ...process.env, FAILPROOFAI_DIST_PATH: process.cwd(), FAILPROOFAI_SKIP_KILL: "true" }, encoding: "utf-8" }), assert spawnResult.status === 0 (or use expect to fail with diagnostics) and read spawnResult.stdout, then JSON.parse(spawnResult.stdout.trim()) and expect it toEqual({ decision: "allow" }); ensure you import/require spawnSync if not already and preserve the same env/cwd keys and input handling as the deny-case implementation.__tests__/hooks/integrations.test.ts (1)
557-605: Exercise the production Pi session-id extraction path.These tests duplicate the extraction logic locally, so they can pass even if
pi.writeHookEntries()generates a brokengetSessionIdFromFile. Prefer asserting the generated extension content or exporting/testing the actual helper. As per coding guidelines, “Always add unit tests for new behaviour. Place tests in__tests__/.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/integrations.test.ts` around lines 557 - 605, The tests are re-implementing the session-ID extraction logic locally instead of exercising the production code; replace the local extractUuid/regex checks with calls to the real implementation (import or require getSessionIdFromFile from the module that pi.writeHookEntries generates or invoke pi.writeHookEntries() to obtain the generated extension and use its exported getSessionIdFromFile) and assert its behavior against the same filename examples (valid and invalid), ensuring you remove the duplicated extraction logic and validate the actual getSessionIdFromFile used in production.src/hooks/manager.ts (1)
263-270: Two ways to passintegrationintoremoveHooks— pick one.The signature accepts
integrationas a positional arg andopts?.integration, withoptswinning (Line 270). That's confusing public API: callers have to know which one is load-bearing, and typos in the unused slot fail silently. Recommend droppingopts.integration(since a dedicated positional now exists) or consolidating to a single options object. Same applies to the callers inbin/failproofai.mjsif they currently pass both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/manager.ts` around lines 263 - 270, The removeHooks function currently accepts integration both as a positional parameter (integration) and via opts.integration (used by getIntegration), which creates an ambiguous API; pick one source and remove the other. Update removeHooks to use only the positional integration parameter (remove checks/read of opts.integration) or conversely remove the positional integration parameter and rely solely on opts.integration, then update the call site(s) (e.g., callers from bin/failproofai.mjs) to pass integration in the chosen place; ensure getIntegration is called with the single canonical value (e.g., getIntegration(integration)) and remove any references to opts.integration to avoid silent failures.src/hooks/handler.ts (2)
491-513:process.env.COPILOT_SESSION_ID/COPILOT_CMD_IDlisted twice in the same||chain.Lines 507 and 512–513 check the same env vars. The second occurrence is dead. Not a bug, just confusing when reading:
(integrationType === "copilot" ? process.env.COPILOT_SESSION_ID || process.env.COPILOT_CMD_ID : undefined) || process.env.CURSOR_SESSION_ID || process.env.CLAUDE_SESSION_ID || process.env.GEMINI_SESSION_ID || - process.env.PI_SESSION_ID || - process.env.COPILOT_SESSION_ID || - process.env.COPILOT_CMD_ID; + process.env.PI_SESSION_ID;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 491 - 513, The session extraction chain in the const extractedSessionId (refer to the extractedSessionId variable and its || chain and the integrationType ternary for "copilot") contains duplicated environment checks for process.env.COPILOT_SESSION_ID and process.env.COPILOT_CMD_ID; remove the redundant second occurrence at the end of the || chain so each env var is only checked once, keeping the prioritized copilot ternary and the other session env vars (CURSOR_SESSION_ID, CLAUDE_SESSION_ID, GEMINI_SESSION_ID, PI_SESSION_ID) intact.
647-669:...Object.values(COPILOT_EVENT_MAP)duplicates entries already inALL_CANONICAL_EVENTS.Every value in
COPILOT_EVENT_MAP(SessionStart,SessionEnd,UserPromptSubmit,PreToolUse,PostToolUse,Stop,SubagentStop) is already listed explicitly above. The spread is a no-op on aSetbut muddies intent. Drop it, or replace the explicit list with a union built from all*_EVENT_MAPrecords so the set stays in sync as integrations are added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 647 - 669, ALL_CANONICAL_EVENTS currently contains explicit event names and also spreads ...Object.values(COPILOT_EVENT_MAP), which duplicates entries and is confusing; update the ALL_CANONICAL_EVENTS definition to either remove the spread and keep only the explicit list, or (preferred) construct the set by merging all event map values (e.g., from COPILOT_EVENT_MAP and other *_EVENT_MAPs) so it stays in sync with integrations—make the change where ALL_CANONICAL_EVENTS is declared and ensure downstream logic that uses canonicalEventName, isLifecycle, and eventFingerprint remains unchanged.src/hooks/integrations.ts (1)
1205-1240:(this as any)._lastPathcouplesgetSettingsPathandwriteHookEntriesthrough shared mutable state.Both
opencode(Line 1219) andpi(Line 1393) integrations stash the last-resolved path onthissowriteHookEntriescan read it back. This is a shared singleton (INTEGRATIONS.opencode/INTEGRATIONS.pi), so any interleaved call sequence — e.g.getSettingsPath("user")followed bygetSettingsPath("project")beforewriteHookEntriesruns — will silently write to the wrong file. The currentinstallHookspath is sequential so it works today, but this is a footgun for future callers (bulk installs, tests, server-side use).Prefer a local-variable flow: have
writeHookEntriesaccept asettingsPath(orscope + cwd) so the resolution is explicit and idempotent. Same applies topi.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/integrations.ts` around lines 1205 - 1240, getSettingsPath and writeHookEntries currently communicate via shared mutable state (this._lastPath), which can cause race/ordering bugs; change writeHookEntries in the opencode and pi integration objects to accept an explicit settingsPath (or scope and cwd) instead of reading (this as any)._lastPath, remove/stop setting _lastPath inside getSettingsPath and stop relying on it in writeHookEntries, and update all callers (e.g., installHooks) to resolve the path via getSettingsPath(...) and pass that path into writeHookEntries so the path resolution is local, explicit, and idempotent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8fe2e4f0-e2ce-48a3-b900-f9a76b9b0fcd
📒 Files selected for processing (48)
.claude/settings.json.failproofai/policies-config.json.failproofai/policies/workflow-policies.mjsCHANGELOG.md__tests__/codex/trace-parser.test.ts__tests__/components/project-list.test.tsx__tests__/e2e/helpers/hook-runner.ts__tests__/e2e/helpers/payloads.ts__tests__/e2e/hooks/copilot-integration.e2e.test.ts__tests__/e2e/hooks/cursor-integration.e2e.test.ts__tests__/e2e/hooks/custom-hooks.e2e.test.ts__tests__/e2e/hooks/gemini-integration.e2e.test.ts__tests__/e2e/hooks/policy-params.e2e.test.ts__tests__/hooks/block-read-outside-cwd.test.ts__tests__/hooks/builtin-policies.test.ts__tests__/hooks/handler.test.ts__tests__/hooks/hook-activity-store.test.ts__tests__/hooks/integration-deduplication.test.ts__tests__/hooks/integrations.test.ts__tests__/hooks/manager.test.ts__tests__/hooks/policy-evaluator.test.ts__tests__/lib/log-entries.test.ts__tests__/lib/projects.test.tsapp/actions/get-hook-activity.tsapp/components/project-list.tsxapp/components/refresh-button.tsxapp/policies/hooks-client.tsxapp/project/[name]/page.tsxapp/project/[name]/session/[sessionId]/page.tsxbin/failproofai.mjscomponents/integration-badge.tsxlib/copilot-sessions.tslib/log-entries.tslib/paths.tslib/projects.tspackage.jsonscripts/codex-trace.mjsscripts/preuninstall.mjssrc/codex/trace-parser.tssrc/hooks/builtin-policies.tssrc/hooks/handler.tssrc/hooks/hook-activity-store.tssrc/hooks/hook-logger.tssrc/hooks/integrations.tssrc/hooks/manager.tssrc/hooks/policy-evaluator.tssrc/hooks/policy-registry.tssrc/hooks/types.ts
💤 Files with no reviewable changes (2)
- .claude/settings.json
- .failproofai/policies/workflow-policies.mjs
Comment on lines
+227
to
+236
| // For testing: reset deduplication state | ||
| export function _resetDedupeCache(): void { | ||
| try { | ||
| if (existsSync(DEDUP_DIR)) { | ||
| require("node:fs").rmSync(DEDUP_DIR, { recursive: true, force: true }); | ||
| } | ||
| } catch { | ||
| // Ignore cleanup errors in tests | ||
| } | ||
| } |
There was a problem hiding this comment.
require("node:fs") inside an ESM module — and rmSync is already imported.
This file uses top-level import syntax throughout, so it's ESM. require is not defined in ESM and will throw ReferenceError: require is not defined at runtime (Vitest/esbuild may paper over it in tests, but the shipped bundle won't). Also, rmSync is already imported on Line 17, so the require indirection isn't needed at all.
🛠️ Proposed fix
export function _resetDedupeCache(): void {
try {
if (existsSync(DEDUP_DIR)) {
- require("node:fs").rmSync(DEDUP_DIR, { recursive: true, force: true });
+ rmSync(DEDUP_DIR, { recursive: true, force: true });
}
} catch {
// Ignore cleanup errors in tests
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // For testing: reset deduplication state | |
| export function _resetDedupeCache(): void { | |
| try { | |
| if (existsSync(DEDUP_DIR)) { | |
| require("node:fs").rmSync(DEDUP_DIR, { recursive: true, force: true }); | |
| } | |
| } catch { | |
| // Ignore cleanup errors in tests | |
| } | |
| } | |
| // For testing: reset deduplication state | |
| export function _resetDedupeCache(): void { | |
| try { | |
| if (existsSync(DEDUP_DIR)) { | |
| rmSync(DEDUP_DIR, { recursive: true, force: true }); | |
| } | |
| } catch { | |
| // Ignore cleanup errors in tests | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/handler.ts` around lines 227 - 236, The _resetDedupeCache function
uses require("node:fs") which is invalid in an ESM module and redundant because
rmSync is already imported; replace the require call by directly calling the
imported rmSync (i.e., use rmSync(DEDUP_DIR, { recursive: true, force: true }))
and keep the existsSync/try-catch logic as-is, verifying that rmSync is imported
at the top of the file.
yashupadhyayy1
force-pushed
the
feat/cursor-integration
branch
from
64358b0 to
4b08415
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
app/policies/hooks-client.tsx (1)
490-560:⚠️ Potential issue | 🟡 MinorUpdate the expanded-row colspan after adding the Integration column.
The table now has 11 columns, but
DetailPanelstill renderscolSpan={10}at Line 254, so expanded details will be one column short.🎨 Proposed fix
- <td colSpan={10} className="px-0 py-0"> + <td colSpan={11} className="px-0 py-0">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/policies/hooks-client.tsx` around lines 490 - 560, The DetailPanel's expanded-row colSpan is still set to 10 but the table now has 11 columns (an Integration column was added); update the DetailPanel component (the JSX element rendering colSpan={10}) to use colSpan={11} (or compute cols dynamically) so the expanded details span the full width; look for the DetailPanel / <td colSpan={...}> usage in app/policies/hooks-client.tsx and change the numeric value or replace with a dynamic count that includes the new Integration column.src/hooks/builtin-policies.ts (2)
1672-1690:⚠️ Potential issue | 🟠 MajorDo not register the diagnostic policy unconditionally.
debug-promptbypassesenabledNamesand will run for everyUserPromptSubmit, adding noisy allow reasons even when users did not enable it. Gate it behind an explicit policy flag or remove it from production registration.🐛 Proposed fix
- // Diagnostic policy to verify prompt capture for multi-agent support - registerPolicy( - "debug-prompt", - "Diagnostic policy to verify prompt capture", - async (ctx) => ({ - decision: "allow", - reason: `Prompt captured from ${ctx.session?.integration ?? "unknown"}`, - }), - { events: ["UserPromptSubmit"] }, - 100, - ); + if (enabledSet.has("debug-prompt")) { + registerPolicy( + "debug-prompt", + "Diagnostic policy to verify prompt capture", + async (ctx) => ({ + decision: "allow", + reason: `Prompt captured from ${ctx.session?.integration ?? "unknown"}`, + }), + { events: ["UserPromptSubmit"] }, + 100, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/builtin-policies.ts` around lines 1672 - 1690, The diagnostic "debug-prompt" policy is being registered unconditionally in registerBuiltinPolicies, bypassing enabledNames and causing noisy allow reasons; change registerBuiltinPolicies to only register the "debug-prompt" policy when enabledNames contains "debug-prompt" (or remove it from production), i.e., wrap the registerPolicy call for "debug-prompt" in a conditional check against enabledSet (or move it into BUILTIN_POLICIES) so it respects the same enablement mechanism used for BUILTIN_POLICIES and registerPolicy.
1309-1323:⚠️ Potential issue | 🟠 MajorKeep
require-ci-green-before-stopenforcing failures.This policy is named/described as requiring CI to pass, but failing and pending checks now return
allow(), so Stop is no longer blocked when CI is red or still running. Usedeny()/instruct()here, or rename/reclassify the policy as warning-only.🐛 Proposed fix
if (failing.length > 0) { const names = failing.map((r) => `"${r.name}"`).join(", "); - return allow( - `⚠ Warning: CI checks are failing on branch "${branch}": ${names}. Consider fixing these before merging.`, + return deny( + `CI checks are failing on branch "${branch}": ${names}. Fix them before stopping.`, ); } @@ if (pending.length > 0) { const names = pending.map((r) => `"${r.name}"`).join(", "); - return allow( - `⚠ Warning: CI checks are still running on branch "${branch}": ${names}. You can continue, but check will be needed before merging.`, + return deny( + `CI checks are still running on branch "${branch}": ${names}. Wait for them to finish before stopping.`, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/builtin-policies.ts` around lines 1309 - 1323, The policy currently named/expected to enforce CI green uses allow(...) for failing and pending checks; update the branch-check logic in the require-ci-green-before-stop policy (the block that builds names from failing and pending and currently calls allow) to call deny(...) (or instruct(...)) instead so Stop is blocked when checks are failing or still running; keep the existing messages or adjust them to reflect a blocking denial and ensure the same failing/pending arrays and message strings are reused when replacing allow with deny/instruct.src/hooks/manager.ts (1)
210-251:⚠️ Potential issue | 🔴 CriticalRemove the stale Claude-only hook writer.
After delegating to
integ.writeHookEntries()/integ.writeSettings()at line 207, lines 210–251 contain dead code that references unimported symbols (HOOK_EVENT_TYPES,ClaudeHookEntry,ClaudeHookMatcher,FAILPROOFAI_HOOK_MARKER,isFailproofaiHook,writeSettings), preventing compilation and introducing duplication of hook configuration across integration-specific formats.🐛 Proposed fix
- if (!settings.hooks) { - settings.hooks = {}; - } - - for (const eventType of HOOK_EVENT_TYPES) { - const command = scope === "project" - ? `npx -y failproofai --hook ${eventType}` - : `"${binaryPath}" --hook ${eventType}`; - const hookEntry: ClaudeHookEntry = { - type: "command", - command, - timeout: 60_000, - [FAILPROOFAI_HOOK_MARKER]: true, - }; - - if (!settings.hooks[eventType]) { - settings.hooks[eventType] = []; - } - - const matchers: ClaudeHookMatcher[] = settings.hooks[eventType]; - - // Find existing failproofai matcher - let found = false; - for (const matcher of matchers) { - if (!matcher.hooks) continue; - const failproofaiIdx = matcher.hooks.findIndex((h: ClaudeHookEntry | Record<string, unknown>) => - isFailproofaiHook(h as Record<string, unknown>) - ); - if (failproofaiIdx >= 0) { - matcher.hooks[failproofaiIdx] = hookEntry; - found = true; - break; - } - } - - if (!found) { - // Append a new matcher with the failproofai hook - matchers.push({ hooks: [hookEntry] }); - } - } - - writeSettings(settingsPath, settings); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/manager.ts` around lines 210 - 251, Remove the stale Claude-specific hook-writing block (the loop that references HOOK_EVENT_TYPES, ClaudeHookEntry, ClaudeHookMatcher, FAILPROOFAI_HOOK_MARKER, isFailproofaiHook and the subsequent writeSettings call) and instead rely solely on integ.writeHookEntries()/integ.writeSettings() that were added at/around line 207; delete the duplicated logic and any now-unused imports/definitions for those Claude-only symbols and ensure there is only a single call to write the settings via integ.writeSettings().src/hooks/policy-evaluator.ts (1)
187-344:⚠️ Potential issue | 🔴 CriticalDeclare missing variables before use in deny/instruct protocol branches.
TypeScript compilation fails with 9 errors in the deny and instruct return blocks:
displayToolis undefined at lines 192, 229, 255exitCode,stdout,stderr,hardStopare undefined at lines 299–305 (fallback return for other event types)exitCode,stdout,stderr,hardStopare undefined at lines 337–344 (instruct+Stop block)The variables
stdoutandstderrare declared at the start of the deny block (lines 185–186) but are not in scope for the fallback return statement at line 299. Similarly,exitCodeandhardStopare never declared in either the deny or instruct branches.Declare
displayToolusingtoolNamefrom the function scope, and declareexitCodeandhardStopbased on the session integration type before the deny block, or restructure the returns to avoid referencing undefined variables.Compilation errors
src/hooks/policy-evaluator.ts(192,50): error TS2304: Cannot find name 'displayTool'. src/hooks/policy-evaluator.ts(229,44): error TS2304: Cannot find name 'displayTool'. src/hooks/policy-evaluator.ts(255,32): error TS2304: Cannot find name 'displayTool'. src/hooks/policy-evaluator.ts(299,9): error TS18004: No value exists in scope for the shorthand property 'exitCode'. src/hooks/policy-evaluator.ts(305,9): error TS18004: No value exists in scope for the shorthand property 'hardStop'. src/hooks/policy-evaluator.ts(337,9): error TS18004: No value exists in scope for the shorthand property 'exitCode'. src/hooks/policy-evaluator.ts(338,9): error TS18004: No value exists in scope for the shorthand property 'stdout'. src/hooks/policy-evaluator.ts(339,9): error TS18004: No value exists in scope for the shorthand property 'stderr'. src/hooks/policy-evaluator.ts(344,9): error TS18004: No value exists in scope for the shorthand property 'hardStop'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/policy-evaluator.ts` around lines 187 - 344, The deny/instruct branches reference variables that are not in scope—declare displayTool and the common return vars before those branches: set const displayTool = toolName (used by deny returns) and initialize exitCode, stdout, stderr, hardStop (e.g., let exitCode = 0; let stdout = ""; let stderr = ""; let hardStop = false) based on session?.integration where needed (match existing per-integration semantics used in the PreToolUse/PostToolUse/Stop blocks) so all return objects (the fallback deny return and the instruct+Stop return) reference defined values; update uses in the deny block (references in the PreToolUse/PostToolUse/Stop branches) and the bottom instruct block to use these declared variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/hooks/policy-evaluator.test.ts`:
- Around line 753-756: The test currently uses "pi" which is a real integration;
update the test case for evaluatePolicies("PreToolUse", { tool_name: "Bash" }, {
integration: "pi" as any }) to use a truly unknown integration string (e.g.,
"unknown_integration") so it exercises the default specialized style, and add a
new separate assertion/test that calls evaluatePolicies with integration: "pi"
and asserts the Pi-specific blocking response (the expected Pi synchronous
blocking message/exitCode). Ensure you modify the existing spec that references
evaluatePolicies to use the unknown integration and add a new it(...) for the
Pi-specific expected behavior.
In `@lib/copilot-sessions.ts`:
- Around line 66-77: The session `lastModified` is using the session directory
mtime (dirStat) which doesn't update when appending to events.jsonl; change the
logic in the block that currently awaits Promise.all([safeStat(sessionPath),
countLines(eventsPath)]) to also stat the events file and use its mtime if
present: call safeStat(eventsPath) (or include it in the Promise.all), compute
mtime = (eventsStat?.mtime ?? dirStat?.mtime ?? new Date(0)), and set
lastModified/lastModifiedFormatted from that mtime so recent sessions are
ordered by the events.jsonl modification time.
- Around line 27-29: Replace the hardcoded "~/.copilot/session-state" in
getCopilotSessionStateDir with the configured path lookup that honors the
COPILOT_SESSION_STATE_PATH override: call the existing path helper that reads
COPILOT_SESSION_STATE_PATH (or, if that helper is not accessible here, check
process.env.COPILOT_SESSION_STATE_PATH first) and return its value; fall back to
the current join(homedir(), ".copilot", "session-state") only when no override
is present so session discovery respects configured/test storage.
In `@lib/projects.ts`:
- Around line 315-317: The loop currently uses "if (entry.cwd && entry.cwd !==
cwd) continue;" which allows entries with missing/empty entry.cwd to be included
and leak sessions; change the guard to require an exact match so entries without
a cwd are skipped — replace that line with a check like "if (entry.cwd !== cwd)
continue;" (or "if (!entry.cwd || entry.cwd !== cwd) continue;") so only entries
whose entry.cwd equals cwd are processed; keep the rest of the loop (e.g., the
date = new Date(entry.timestamp) logic) unchanged.
- Around line 270-272: Validate and sanitize project names that start with
"ses_" before building the path: in the branch where you check
name.startsWith("ses_") (using the local variable name and
getOpencodeStoragePath()/session_diff), first ensure the suffix matches a strict
session-id pattern (e.g. /^[A-Za-z0-9_-]{1,64}$/) and reject otherwise, then
construct the path using path.resolve(path.join(getOpencodeStoragePath(),
"session_diff", `${name}.json`)) and verify the resolved path begins with the
resolved session_diff directory (resolve
getOpencodeStoragePath()+"/session_diff"); if the check fails return an
error/null (or throw) instead of returning the unsafe path so the existing
return { path: ..., source: "opencode" } is only used for validated names.
In `@scripts/preuninstall.mjs`:
- Around line 23-35: The removeCopilotSyncFromRcFiles function currently only
strips a standalone marker via the regex in updated; update that logic to also
handle inline markers (marker appended at end of a command) by using a more
robust pattern: match either a whole line that equals the MARKER or any
occurrence of the MARKER preceded by optional whitespace and an optional
delimiter (space, semicolon, or comment delimiter) and remove just the marker
and any trailing whitespace/newline, and if the marker is the only thing on the
line remove the entire line; adjust the replacement applied to the updated
variable (and keep using MARKER, rcFiles and writeFileSync) so both standalone
and inline marker usages are cleaned up.
In `@src/hooks/integrations.ts`:
- Around line 1463-1471: Remove the generated Pi debug logging that builds and
logs debugInfo (ctx_sessionId, ctx_session_id, pi_session_id, pi_sessionId,
projectName) and the surrounding try/catch; specifically delete the debugInfo
object and the try { (pi as any).log?... } catch {} call in
src/hooks/integrations.ts (the code referencing pi, debugInfo and .log). If you
need to keep this for troubleshooting instead of deleting, guard the entire
block behind an explicit, opt-in debug flag (e.g., FAILPROOF_DEBUG or a runtime
config) so it only runs when that flag is true.
- Around line 1324-1329: The code currently recurses-delete the entire plugins
directory by calling rmSync(dir, { recursive: true, force: true }) when
dir.includes(".opencode/plugins"); instead, remove only the generated settings
file and avoid deleting the plugins root. Replace the rmSync(dir, { recursive:
true, force: true }) call with logic that unlinks/removes settingsPath (e.g.,
unlinkSync or rmSync(settingsPath, { force: true })), then attempt to remove dir
only if it is empty and its basename is not "plugins" (or the path does not
strictly equal the ".opencode/plugins" directory); keep references to
settingsPath, dir, and rmSync so you modify the block around those identifiers.
- Around line 209-215: The computed local variable command inside
buildHookEntry(string, eventType, scope?) is never used — the returned object
always embeds the machine-specific Node executable and binary path; update the
return value in buildHookEntry to use the previously computed command (and
append the --stdin flag when building the hook entry) instead of the hardcoded
`"${process.execPath}" "${binaryPath}" ...` string so that project-scoped hooks
use the scoped Claude command (the command variable) and global installs keep
the full execPath/binary form; adjust quoting as needed to preserve spaces and
existing flags.
---
Outside diff comments:
In `@app/policies/hooks-client.tsx`:
- Around line 490-560: The DetailPanel's expanded-row colSpan is still set to 10
but the table now has 11 columns (an Integration column was added); update the
DetailPanel component (the JSX element rendering colSpan={10}) to use
colSpan={11} (or compute cols dynamically) so the expanded details span the full
width; look for the DetailPanel / <td colSpan={...}> usage in
app/policies/hooks-client.tsx and change the numeric value or replace with a
dynamic count that includes the new Integration column.
In `@src/hooks/builtin-policies.ts`:
- Around line 1672-1690: The diagnostic "debug-prompt" policy is being
registered unconditionally in registerBuiltinPolicies, bypassing enabledNames
and causing noisy allow reasons; change registerBuiltinPolicies to only register
the "debug-prompt" policy when enabledNames contains "debug-prompt" (or remove
it from production), i.e., wrap the registerPolicy call for "debug-prompt" in a
conditional check against enabledSet (or move it into BUILTIN_POLICIES) so it
respects the same enablement mechanism used for BUILTIN_POLICIES and
registerPolicy.
- Around line 1309-1323: The policy currently named/expected to enforce CI green
uses allow(...) for failing and pending checks; update the branch-check logic in
the require-ci-green-before-stop policy (the block that builds names from
failing and pending and currently calls allow) to call deny(...) (or
instruct(...)) instead so Stop is blocked when checks are failing or still
running; keep the existing messages or adjust them to reflect a blocking denial
and ensure the same failing/pending arrays and message strings are reused when
replacing allow with deny/instruct.
In `@src/hooks/manager.ts`:
- Around line 210-251: Remove the stale Claude-specific hook-writing block (the
loop that references HOOK_EVENT_TYPES, ClaudeHookEntry, ClaudeHookMatcher,
FAILPROOFAI_HOOK_MARKER, isFailproofaiHook and the subsequent writeSettings
call) and instead rely solely on integ.writeHookEntries()/integ.writeSettings()
that were added at/around line 207; delete the duplicated logic and any
now-unused imports/definitions for those Claude-only symbols and ensure there is
only a single call to write the settings via integ.writeSettings().
In `@src/hooks/policy-evaluator.ts`:
- Around line 187-344: The deny/instruct branches reference variables that are
not in scope—declare displayTool and the common return vars before those
branches: set const displayTool = toolName (used by deny returns) and initialize
exitCode, stdout, stderr, hardStop (e.g., let exitCode = 0; let stdout = ""; let
stderr = ""; let hardStop = false) based on session?.integration where needed
(match existing per-integration semantics used in the
PreToolUse/PostToolUse/Stop blocks) so all return objects (the fallback deny
return and the instruct+Stop return) reference defined values; update uses in
the deny block (references in the PreToolUse/PostToolUse/Stop branches) and the
bottom instruct block to use these declared variables.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7f6aa35-98b6-415c-b6ae-3665d7e5ecac
📒 Files selected for processing (47)
.claude/settings.json.failproofai/policies-config.json.failproofai/policies/workflow-policies.mjsCHANGELOG.md__tests__/codex/trace-parser.test.ts__tests__/components/project-list.test.tsx__tests__/e2e/helpers/hook-runner.ts__tests__/e2e/helpers/payloads.ts__tests__/e2e/hooks/copilot-integration.e2e.test.ts__tests__/e2e/hooks/cursor-integration.e2e.test.ts__tests__/e2e/hooks/custom-hooks.e2e.test.ts__tests__/e2e/hooks/gemini-integration.e2e.test.ts__tests__/e2e/hooks/policy-params.e2e.test.ts__tests__/hooks/block-read-outside-cwd.test.ts__tests__/hooks/builtin-policies.test.ts__tests__/hooks/handler.test.ts__tests__/hooks/hook-activity-store.test.ts__tests__/hooks/integration-deduplication.test.ts__tests__/hooks/integrations.test.ts__tests__/hooks/manager.test.ts__tests__/hooks/policy-evaluator.test.ts__tests__/lib/log-entries.test.ts__tests__/lib/projects.test.tsapp/actions/get-hook-activity.tsapp/components/project-list.tsxapp/components/refresh-button.tsxapp/policies/hooks-client.tsxapp/project/[name]/page.tsxapp/project/[name]/session/[sessionId]/page.tsxbin/failproofai.mjscomponents/integration-badge.tsxlib/copilot-sessions.tslib/log-entries.tslib/paths.tslib/projects.tsscripts/codex-trace.mjsscripts/preuninstall.mjssrc/codex/trace-parser.tssrc/hooks/builtin-policies.tssrc/hooks/handler.tssrc/hooks/hook-activity-store.tssrc/hooks/hook-logger.tssrc/hooks/integrations.tssrc/hooks/manager.tssrc/hooks/policy-evaluator.tssrc/hooks/policy-registry.tssrc/hooks/types.ts
💤 Files with no reviewable changes (2)
- .claude/settings.json
- .failproofai/policies/workflow-policies.mjs
✅ Files skipped from review due to trivial changes (10)
- .failproofai/policies-config.json
- src/hooks/policy-registry.ts
- tests/components/project-list.test.tsx
- app/components/refresh-button.tsx
- scripts/codex-trace.mjs
- tests/e2e/hooks/copilot-integration.e2e.test.ts
- bin/failproofai.mjs
- CHANGELOG.md
- src/hooks/hook-activity-store.ts
- src/hooks/handler.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- tests/hooks/block-read-outside-cwd.test.ts
- app/actions/get-hook-activity.ts
- tests/codex/trace-parser.test.ts
- tests/e2e/hooks/policy-params.e2e.test.ts
- src/hooks/hook-logger.ts
- app/components/project-list.tsx
- tests/e2e/helpers/hook-runner.ts
- tests/hooks/manager.test.ts
- components/integration-badge.tsx
- app/project/[name]/page.tsx
- tests/lib/log-entries.test.ts
- app/project/[name]/session/[sessionId]/page.tsx
- tests/e2e/hooks/cursor-integration.e2e.test.ts
- tests/hooks/integration-deduplication.test.ts
- tests/e2e/hooks/gemini-integration.e2e.test.ts
- lib/paths.ts
- tests/hooks/hook-activity-store.test.ts
- src/codex/trace-parser.ts
- tests/hooks/integrations.test.ts
- tests/e2e/hooks/custom-hooks.e2e.test.ts
Comment on lines
+27
to
+29
| export function getCopilotSessionStateDir(): string { | ||
| return join(homedir(), ".copilot", "session-state"); | ||
| } |
There was a problem hiding this comment.
Honor the configured Copilot session-state path.
This hardcodes ~/.copilot/session-state, but the path helper supports COPILOT_SESSION_STATE_PATH. Session discovery will ignore configured/test storage and show no Copilot sessions when that override is used.
🔧 Proposed fix
-import { join } from "path";
-import { homedir } from "os";
+import { join } from "path";
import { runtimeCache } from "./runtime-cache";
import { logWarn, logError } from "./logger";
import { formatDate } from "./utils";
+import { getCopilotSessionStatePath } from "./paths";
@@
export function getCopilotSessionStateDir(): string {
- return join(homedir(), ".copilot", "session-state");
+ return getCopilotSessionStatePath();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function getCopilotSessionStateDir(): string { | |
| return join(homedir(), ".copilot", "session-state"); | |
| } | |
| import { join } from "path"; | |
| import { COPILOT_SESSION_STATE_PATH } from "./paths"; | |
| import { runtimeCache } from "./runtime-cache"; | |
| import { logWarn, logError } from "./logger"; | |
| import { formatDate } from "./utils"; | |
| export function getCopilotSessionStateDir(): string { | |
| return COPILOT_SESSION_STATE_PATH; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/copilot-sessions.ts` around lines 27 - 29, Replace the hardcoded
"~/.copilot/session-state" in getCopilotSessionStateDir with the configured path
lookup that honors the COPILOT_SESSION_STATE_PATH override: call the existing
path helper that reads COPILOT_SESSION_STATE_PATH (or, if that helper is not
accessible here, check process.env.COPILOT_SESSION_STATE_PATH first) and return
its value; fall back to the current join(homedir(), ".copilot", "session-state")
only when no override is present so session discovery respects configured/test
storage.
Comment on lines
+66
to
+77
| const [dirStat, eventCount] = await Promise.all([ | ||
| safeStat(sessionPath), | ||
| countLines(eventsPath), | ||
| ]); | ||
|
|
||
| const mtime = dirStat?.mtime ?? new Date(0); | ||
| return { | ||
| sessionId: entry.name, | ||
| path: sessionPath, | ||
| eventsPath, | ||
| lastModified: mtime, | ||
| lastModifiedFormatted: formatDate(mtime), |
There was a problem hiding this comment.
Use events.jsonl mtime for recent-session ordering.
Appending to events.jsonl does not reliably update the session directory mtime, so active Copilot sessions can sort as stale and show an old lastModified.
🕒 Proposed fix
- const [dirStat, eventCount] = await Promise.all([
+ const [dirStat, eventsStat, eventCount] = await Promise.all([
safeStat(sessionPath),
+ safeStat(eventsPath),
countLines(eventsPath),
]);
- const mtime = dirStat?.mtime ?? new Date(0);
+ const mtime = eventsStat?.mtime ?? dirStat?.mtime ?? new Date(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [dirStat, eventCount] = await Promise.all([ | |
| safeStat(sessionPath), | |
| countLines(eventsPath), | |
| ]); | |
| const mtime = dirStat?.mtime ?? new Date(0); | |
| return { | |
| sessionId: entry.name, | |
| path: sessionPath, | |
| eventsPath, | |
| lastModified: mtime, | |
| lastModifiedFormatted: formatDate(mtime), | |
| const [dirStat, eventsStat, eventCount] = await Promise.all([ | |
| safeStat(sessionPath), | |
| safeStat(eventsPath), | |
| countLines(eventsPath), | |
| ]); | |
| const mtime = eventsStat?.mtime ?? dirStat?.mtime ?? new Date(0); | |
| return { | |
| sessionId: entry.name, | |
| path: sessionPath, | |
| eventsPath, | |
| lastModified: mtime, | |
| lastModifiedFormatted: formatDate(mtime), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/copilot-sessions.ts` around lines 66 - 77, The session `lastModified` is
using the session directory mtime (dirStat) which doesn't update when appending
to events.jsonl; change the logic in the block that currently awaits
Promise.all([safeStat(sessionPath), countLines(eventsPath)]) to also stat the
events file and use its mtime if present: call safeStat(eventsPath) (or include
it in the Promise.all), compute mtime = (eventsStat?.mtime ?? dirStat?.mtime ??
new Date(0)), and set lastModified/lastModifiedFormatted from that mtime so
recent sessions are ordered by the events.jsonl modification time.
Comment on lines
+270
to
+272
| if (name.startsWith("ses_")) { | ||
| return { path: join(getOpencodeStoragePath(), "session_diff", `${name}.json`), source: "opencode" }; | ||
| } |
There was a problem hiding this comment.
Validate ses_ project names before constructing the path.
A route parameter like ses_/../../... still passes startsWith("ses_") and is joined into the opencode storage path. Keep this branch constrained to a safe session-id shape and verify the resolved path stays under session_diff.
🛡️ Proposed fix
if (name.startsWith("ses_")) {
- return { path: join(getOpencodeStoragePath(), "session_diff", `${name}.json`), source: "opencode" };
+ if (!/^ses_[A-Za-z0-9_-]+$/.test(name)) {
+ throw new RangeError("Invalid opencode session ID");
+ }
+ const opencodeRoot = resolve(getOpencodeStoragePath(), "session_diff");
+ const candidate = resolve(join(opencodeRoot, `${name}.json`));
+ if (!candidate.startsWith(opencodeRoot + sep)) {
+ throw new RangeError("Opencode path escapes root");
+ }
+ return { path: candidate, source: "opencode" };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (name.startsWith("ses_")) { | |
| return { path: join(getOpencodeStoragePath(), "session_diff", `${name}.json`), source: "opencode" }; | |
| } | |
| if (name.startsWith("ses_")) { | |
| if (!/^ses_[A-Za-z0-9_-]+$/.test(name)) { | |
| throw new RangeError("Invalid opencode session ID"); | |
| } | |
| const opencodeRoot = resolve(getOpencodeStoragePath(), "session_diff"); | |
| const candidate = resolve(join(opencodeRoot, `${name}.json`)); | |
| if (!candidate.startsWith(opencodeRoot + sep)) { | |
| throw new RangeError("Opencode path escapes root"); | |
| } | |
| return { path: candidate, source: "opencode" }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/projects.ts` around lines 270 - 272, Validate and sanitize project names
that start with "ses_" before building the path: in the branch where you check
name.startsWith("ses_") (using the local variable name and
getOpencodeStoragePath()/session_diff), first ensure the suffix matches a strict
session-id pattern (e.g. /^[A-Za-z0-9_-]{1,64}$/) and reject otherwise, then
construct the path using path.resolve(path.join(getOpencodeStoragePath(),
"session_diff", `${name}.json`)) and verify the resolved path begins with the
resolved session_diff directory (resolve
getOpencodeStoragePath()+"/session_diff"); if the check fails return an
error/null (or throw) instead of returning the unsafe path so the existing
return { path: ..., source: "opencode" } is only used for validated names.
Comment on lines
+315
to
+317
| // Match by cwd — empty cwd entries are skipped | ||
| if (entry.cwd && entry.cwd !== cwd) continue; | ||
| const date = new Date(entry.timestamp); |
There was a problem hiding this comment.
Skip activity entries that do not belong to this cwd.
The comment says empty cwd entries are skipped, but if (entry.cwd && entry.cwd !== cwd) continue; includes entries with missing cwd in every project’s session list. That can leak unrelated virtual sessions across project pages.
🐛 Proposed fix
- // Match by cwd — empty cwd entries are skipped
- if (entry.cwd && entry.cwd !== cwd) continue;
+ // Match by cwd — empty cwd entries are skipped
+ if (!entry.cwd || entry.cwd !== cwd) continue;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Match by cwd — empty cwd entries are skipped | |
| if (entry.cwd && entry.cwd !== cwd) continue; | |
| const date = new Date(entry.timestamp); | |
| // Match by cwd — empty cwd entries are skipped | |
| if (!entry.cwd || entry.cwd !== cwd) continue; | |
| const date = new Date(entry.timestamp); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/projects.ts` around lines 315 - 317, The loop currently uses "if
(entry.cwd && entry.cwd !== cwd) continue;" which allows entries with
missing/empty entry.cwd to be included and leak sessions; change the guard to
require an exact match so entries without a cwd are skipped — replace that line
with a check like "if (entry.cwd !== cwd) continue;" (or "if (!entry.cwd ||
entry.cwd !== cwd) continue;") so only entries whose entry.cwd equals cwd are
processed; keep the rest of the loop (e.g., the date = new Date(entry.timestamp)
logic) unchanged.
Comment on lines
+23
to
+35
| function removeCopilotSyncFromRcFiles() { | ||
| const MARKER = "# failproofai copilot-sync"; | ||
| const rcFiles = [ | ||
| resolve(homedir(), ".bashrc"), | ||
| resolve(homedir(), ".zshrc"), | ||
| ]; | ||
| for (const rc of rcFiles) { | ||
| if (!existsSync(rc)) continue; | ||
| try { | ||
| const content = readFileSync(rc, "utf8"); | ||
| if (!content.includes(MARKER)) continue; | ||
| const updated = content.replace(/# failproofai copilot-sync\n[^\n]+\n?/g, ""); | ||
| writeFileSync(rc, updated, "utf8"); |
There was a problem hiding this comment.
Handle inline Copilot sync markers during uninstall.
Line 34 only removes # failproofai copilot-sync when it is on its own line followed by another line. If the rc entry is written as a command with the marker at the end of the same line, uninstall will leave the command behind.
🧹 Proposed robust cleanup
function removeCopilotSyncFromRcFiles() {
const MARKER = "# failproofai copilot-sync";
@@
const content = readFileSync(rc, "utf8");
if (!content.includes(MARKER)) continue;
- const updated = content.replace(/# failproofai copilot-sync\n[^\n]+\n?/g, "");
+ const lines = content.split(/\r?\n/);
+ const updatedLines = [];
+ for (let i = 0; i < lines.length; i++) {
+ const line = lines[i];
+ if (line.includes(MARKER)) {
+ if (line.trim() === MARKER && /failproofai.*copilot-sync/.test(lines[i + 1] ?? "")) {
+ i++;
+ }
+ continue;
+ }
+ updatedLines.push(line);
+ }
+ const updated = updatedLines.join("\n");
writeFileSync(rc, updated, "utf8");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/preuninstall.mjs` around lines 23 - 35, The
removeCopilotSyncFromRcFiles function currently only strips a standalone marker
via the regex in updated; update that logic to also handle inline markers
(marker appended at end of a command) by using a more robust pattern: match
either a whole line that equals the MARKER or any occurrence of the MARKER
preceded by optional whitespace and an optional delimiter (space, semicolon, or
comment delimiter) and remove just the marker and any trailing
whitespace/newline, and if the marker is the only thing on the line remove the
entire line; adjust the replacement applied to the updated variable (and keep
using MARKER, rcFiles and writeFileSync) so both standalone and inline marker
usages are cleaned up.
Comment on lines
+1463
to
+1471
| // DEBUG: Log available session values | ||
| const debugInfo = { | ||
| ctx_sessionId: ctx?.sessionId, | ||
| ctx_session_id: ctx?.session?.id, | ||
| pi_session_id: pi.session?.id, | ||
| pi_sessionId: pi.sessionId, | ||
| projectName, | ||
| }; | ||
| try { (pi as any).log?.(\`[FailproofAI Debug] Session ID sources: \${JSON.stringify(debugInfo)}\`); } catch {} |
There was a problem hiding this comment.
Remove generated Pi debug logging.
The generated extension logs session IDs and project names on every CLI call. That is noisy and exposes session/workspace metadata in user logs; keep this behind an explicit debug flag or drop it.
🧹 Proposed fix
- // DEBUG: Log available session values
- const debugInfo = {
- ctx_sessionId: ctx?.sessionId,
- ctx_session_id: ctx?.session?.id,
- pi_session_id: pi.session?.id,
- pi_sessionId: pi.sessionId,
- projectName,
- };
- try { (pi as any).log?.(\`[FailproofAI Debug] Session ID sources: \${JSON.stringify(debugInfo)}\`); } catch {}
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // DEBUG: Log available session values | |
| const debugInfo = { | |
| ctx_sessionId: ctx?.sessionId, | |
| ctx_session_id: ctx?.session?.id, | |
| pi_session_id: pi.session?.id, | |
| pi_sessionId: pi.sessionId, | |
| projectName, | |
| }; | |
| try { (pi as any).log?.(\`[FailproofAI Debug] Session ID sources: \${JSON.stringify(debugInfo)}\`); } catch {} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/integrations.ts` around lines 1463 - 1471, Remove the generated Pi
debug logging that builds and logs debugInfo (ctx_sessionId, ctx_session_id,
pi_session_id, pi_sessionId, projectName) and the surrounding try/catch;
specifically delete the debugInfo object and the try { (pi as any).log?... }
catch {} call in src/hooks/integrations.ts (the code referencing pi, debugInfo
and .log). If you need to keep this for troubleshooting instead of deleting,
guard the entire block behind an explicit, opt-in debug flag (e.g.,
FAILPROOF_DEBUG or a runtime config) so it only runs when that flag is true.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/policy-evaluator.ts (1)
220-240:⚠️ Potential issue | 🟡 MinorStop+instruct stderr attributes combined reasons to only the first policy.
When multiple policies produce instruct results on
Stop,getDenyMessageis called withpolicyNames[0]and the newline-joinedcombinedreason from all policies. The rendered message becomes[FailproofAI] <firstPolicy>: <reason1>\n<reason2>\n..., which misleadingly attributes every instruction to the first policy and — for gemini/cursor/copilot — injects later reasons inside a branded template tied to the first policy name.Consider formatting each instruct entry individually (as the allow path already does at line 257) and then joining, e.g.:
- const stderr = getDenyMessage(session?.integration, policyNames[0], combined); + const stderr = instructEntries + .map((e) => getDenyMessage(session?.integration, e.policyName, e.reason)) + .join("\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/policy-evaluator.ts` around lines 220 - 240, The Stop path uses getDenyMessage(session?.integration, policyNames[0], combined) which attributes all combined reasons to the first policy; instead, build stderr by mapping over instructEntries and calling getDenyMessage(session?.integration, entry.policyName, entry.reason) for each entry, then join those per-policy messages with newlines (similar to the allow path at line 257), and return that joined string as stderr while leaving the rest of the returned object (exitCode, stdout, policyName(s), reason, decision, hardStop) unchanged.
♻️ Duplicate comments (1)
src/hooks/handler.ts (1)
228-236:⚠️ Potential issue | 🔴 Critical
require("node:fs")in ESM — andrmSyncis not actually imported.This module uses top-level
importsorequirewill throwReferenceError: require is not definedat runtime outside of Vitest/esbuild. The earlier suggestion to "just usermSync" is correct in spirit, but note thatrmSyncis not currently in thefsimport list at line 28 — you need to add it.🛠️ Proposed fix
-import { existsSync, readdirSync, mkdirSync, writeFileSync, readFileSync, appendFileSync } from "fs"; +import { existsSync, readdirSync, mkdirSync, writeFileSync, readFileSync, appendFileSync, rmSync } from "fs"; @@ export function _resetDedupeCache(): void { try { if (existsSync(DEDUP_DIR)) { - require("node:fs").rmSync(DEDUP_DIR, { recursive: true, force: true }); + rmSync(DEDUP_DIR, { recursive: true, force: true }); } } catch { // Ignore cleanup errors in tests } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 228 - 236, The _resetDedupeCache function uses require("node:fs").rmSync which fails in ESM; replace the dynamic require with a direct call to rmSync and add rmSync to the existing top-level fs imports (so update the import list at the top of the file to include rmSync). Keep the existing existsSync check and try/catch, and call rmSync(DEDUP_DIR, { recursive: true, force: true }) inside that block to perform the cleanup.
🧹 Nitpick comments (2)
src/hooks/policy-evaluator.ts (1)
75-78: Remove deadtriggerHardStopstub.
triggerHardStopis never referenced (the new contract is to returnhardStoponEvaluationResultand let the CLI wrapper inbin/failproofai.mjshandle termination). Keeping a no-op function with an unused parameter is misleading — a future maintainer may try to wire it up.♻️ Proposed removal
-function triggerHardStop(integration: string | undefined): void { - // Logic moved to CLI wrapper to avoid piped hangs. - // evaluatePolicies now returns a hardStop flag instead of signaling directly. -} -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/policy-evaluator.ts` around lines 75 - 78, Remove the dead no-op function triggerHardStop from the file and any related export, since evaluatePolicies now returns a hardStop flag on EvaluationResult and the CLI wrapper (bin/failproofai.mjs) handles termination; ensure there are no remaining references to triggerHardStop (search for the symbol) and that evaluatePolicies and EvaluationResult remain the single source of truth for hard-stop behavior.src/hooks/handler.ts (1)
491-513: DuplicateCOPILOT_SESSION_ID/ minor dedup in the session-id fallback chain.
process.env.COPILOT_SESSION_IDis consulted twice — once guarded byintegrationType === "copilot"at line 507, and again unguarded at line 512. The second lookup also runsCOPILOT_CMD_IDfor any integration, which is probably not intended. Suggest collapsing to a single ordered chain where copilot-specific env vars are only consulted for copilot, and generic vars afterward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 491 - 513, The session-id fallback chain in the extractedSessionId expression duplicates process.env.COPILOT_SESSION_ID and consults COPILOT_CMD_ID unconditionally; update the logic so copilot-specific env vars (process.env.COPILOT_SESSION_ID and process.env.COPILOT_CMD_ID) are only checked when integrationType === "copilot" (inside that ternary), and remove the duplicate/unconditional references at the end of the chain so the remaining fallbacks are the generic env vars (process.env.CURSOR_SESSION_ID, process.env.CLAUDE_SESSION_ID, process.env.GEMINI_SESSION_ID, process.env.PI_SESSION_ID) after the copilot-only block; modify the extractedSessionId assignment accordingly to preserve existing checks (parsed.session_id, parsed.sessionId, parsed.sessionID, parsed.conversation_id, parsed.conversationID, parsed.chat_id, parsed.chatId, parsed.tab_id, parsed.tabId, nested parsed.data variants) but with a single, properly-scoped copilot env fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/hooks/handler.test.ts`:
- Around line 826-868: The test is polluting the user's home dir, lacks
guaranteed cleanup, and shadows the top-level fs import; change the test to
create an isolated temp dir using os.tmpdir() + fs.mkdtempSync (replace testDir
assignment and call _resetForTest with that temp dir), remove the dynamic import
"const fs = await import('node:fs')" and use the existing fs binding, and wrap
the assertion and reads in a try/finally so fs.rmSync(testDir, { recursive:
true, force: true }) always runs (references: persistHookActivity,
_resetForTest, testDir, and the expect(lines.length).toBe(1) assertion).
In `@src/hooks/handler.ts`:
- Around line 413-421: The 500ms setTimeout in the stdin-read Promise (the timer
created near the comment handling chunks and process.stdin) is never cleared,
keeping the event loop alive and risking a late resolve; capture the timeout
handle when calling setTimeout, then call .unref() on it (or
clearTimeout(handle)) when the Promise resolves or rejects from the
'end'/'error' handlers (and also clear/unref it in the early-return branch that
checks process.stdin.readableEnded) to prevent the timer from keeping the
process alive and to avoid potential double-resolve races.
---
Outside diff comments:
In `@src/hooks/policy-evaluator.ts`:
- Around line 220-240: The Stop path uses getDenyMessage(session?.integration,
policyNames[0], combined) which attributes all combined reasons to the first
policy; instead, build stderr by mapping over instructEntries and calling
getDenyMessage(session?.integration, entry.policyName, entry.reason) for each
entry, then join those per-policy messages with newlines (similar to the allow
path at line 257), and return that joined string as stderr while leaving the
rest of the returned object (exitCode, stdout, policyName(s), reason, decision,
hardStop) unchanged.
---
Duplicate comments:
In `@src/hooks/handler.ts`:
- Around line 228-236: The _resetDedupeCache function uses
require("node:fs").rmSync which fails in ESM; replace the dynamic require with a
direct call to rmSync and add rmSync to the existing top-level fs imports (so
update the import list at the top of the file to include rmSync). Keep the
existing existsSync check and try/catch, and call rmSync(DEDUP_DIR, { recursive:
true, force: true }) inside that block to perform the cleanup.
---
Nitpick comments:
In `@src/hooks/handler.ts`:
- Around line 491-513: The session-id fallback chain in the extractedSessionId
expression duplicates process.env.COPILOT_SESSION_ID and consults COPILOT_CMD_ID
unconditionally; update the logic so copilot-specific env vars
(process.env.COPILOT_SESSION_ID and process.env.COPILOT_CMD_ID) are only checked
when integrationType === "copilot" (inside that ternary), and remove the
duplicate/unconditional references at the end of the chain so the remaining
fallbacks are the generic env vars (process.env.CURSOR_SESSION_ID,
process.env.CLAUDE_SESSION_ID, process.env.GEMINI_SESSION_ID,
process.env.PI_SESSION_ID) after the copilot-only block; modify the
extractedSessionId assignment accordingly to preserve existing checks
(parsed.session_id, parsed.sessionId, parsed.sessionID, parsed.conversation_id,
parsed.conversationID, parsed.chat_id, parsed.chatId, parsed.tab_id,
parsed.tabId, nested parsed.data variants) but with a single, properly-scoped
copilot env fallback.
In `@src/hooks/policy-evaluator.ts`:
- Around line 75-78: Remove the dead no-op function triggerHardStop from the
file and any related export, since evaluatePolicies now returns a hardStop flag
on EvaluationResult and the CLI wrapper (bin/failproofai.mjs) handles
termination; ensure there are no remaining references to triggerHardStop (search
for the symbol) and that evaluatePolicies and EvaluationResult remain the single
source of truth for hard-stop behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05da611a-d358-42b9-b0f9-45e046facb7e
📒 Files selected for processing (5)
__tests__/hooks/handler.test.tssrc/hooks/handler.tssrc/hooks/integrations.tssrc/hooks/manager.tssrc/hooks/policy-evaluator.ts
✅ Files skipped from review due to trivial changes (1)
- src/hooks/manager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/integrations.ts
Comment on lines
+826
to
+868
| describe("Mechanism-Level Deduplication", () => { | ||
| it("prevents duplicate log entries at the STORAGE level (Choke Point)", async () => { | ||
| const { persistHookActivity, _resetForTest } = await vi.importActual("../../src/hooks/hook-activity-store") as any; | ||
|
|
||
| const testDir = path.join(os.homedir(), ".failproofai-test-dedup-storage"); | ||
| _resetForTest(testDir); | ||
|
|
||
| // Simulation: First record call | ||
| const entry1 = { | ||
| timestamp: Date.now(), | ||
| eventType: "Stop", | ||
| sessionId: "sess-dedup", | ||
| decision: "allow", | ||
| policyName: "test-policy", | ||
| durationMs: 100 | ||
| } as any; | ||
| persistHookActivity(entry1); | ||
|
|
||
| // Simulation: Second record call with slightly different duration/timestamp | ||
| // but same sessionId and eventType. | ||
| // Window is > 50ms to hit "Twin" detection. | ||
| const entry2 = { | ||
| timestamp: Date.now() + 200, | ||
| eventType: "Stop", | ||
| sessionId: "sess-dedup", | ||
| decision: "allow", | ||
| policyName: "test-policy", | ||
| durationMs: 95 | ||
| } as any; | ||
| persistHookActivity(entry2); | ||
|
|
||
| // Verify that after the second call, the store logic should have dropped it | ||
| const fs = await import("node:fs"); | ||
| const logPath = path.join(testDir, "current.jsonl"); | ||
| const content = fs.readFileSync(logPath, "utf-8").trim(); | ||
| const lines = content.split("\n").filter(l => l.trim().length > 0); | ||
|
|
||
| expect(lines.length).toBe(1); // ONLY ONE LINE recorded despite two persist calls | ||
|
|
||
| // Cleanup | ||
| fs.rmSync(testDir, { recursive: true, force: true }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'hook-activity-store.ts' --exec rg -n -C2 '_resetForTest'Repository: exospherehost/failproofai
Length of output: 227
Fix test isolation and resource cleanup in dedup storage test.
Three issues need attention:
testDir = path.join(os.homedir(), ...)pollutes the real user/CI home directory. Useos.tmpdir()withfs.mkdtempSync()for isolation (matches thewriteVirtualLogEntrysuite below).- No
try/finallyaround the assertions — ifexpect(lines.length).toBe(1)fails, the temp dir is never cleaned up. const fs = await import("node:fs")is unnecessary sincefsis already imported at the top level; using the dynamic import shadows the existing binding.
♻️ Sketch of fix
- const testDir = path.join(os.homedir(), ".failproofai-test-dedup-storage");
+ const testDir = fs.mkdtempSync(path.join(os.tmpdir(), "failproofai-dedup-"));
_resetForTest(testDir);
- ...
- const fs = await import("node:fs");
- const logPath = path.join(testDir, "current.jsonl");
- ...
- fs.rmSync(testDir, { recursive: true, force: true });
+ try {
+ ...
+ const logPath = path.join(testDir, "current.jsonl");
+ const content = fs.readFileSync(logPath, "utf-8").trim();
+ const lines = content.split("\n").filter((l) => l.trim().length > 0);
+ expect(lines.length).toBe(1);
+ } finally {
+ fs.rmSync(testDir, { recursive: true, force: true });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe("Mechanism-Level Deduplication", () => { | |
| it("prevents duplicate log entries at the STORAGE level (Choke Point)", async () => { | |
| const { persistHookActivity, _resetForTest } = await vi.importActual("../../src/hooks/hook-activity-store") as any; | |
| const testDir = path.join(os.homedir(), ".failproofai-test-dedup-storage"); | |
| _resetForTest(testDir); | |
| // Simulation: First record call | |
| const entry1 = { | |
| timestamp: Date.now(), | |
| eventType: "Stop", | |
| sessionId: "sess-dedup", | |
| decision: "allow", | |
| policyName: "test-policy", | |
| durationMs: 100 | |
| } as any; | |
| persistHookActivity(entry1); | |
| // Simulation: Second record call with slightly different duration/timestamp | |
| // but same sessionId and eventType. | |
| // Window is > 50ms to hit "Twin" detection. | |
| const entry2 = { | |
| timestamp: Date.now() + 200, | |
| eventType: "Stop", | |
| sessionId: "sess-dedup", | |
| decision: "allow", | |
| policyName: "test-policy", | |
| durationMs: 95 | |
| } as any; | |
| persistHookActivity(entry2); | |
| // Verify that after the second call, the store logic should have dropped it | |
| const fs = await import("node:fs"); | |
| const logPath = path.join(testDir, "current.jsonl"); | |
| const content = fs.readFileSync(logPath, "utf-8").trim(); | |
| const lines = content.split("\n").filter(l => l.trim().length > 0); | |
| expect(lines.length).toBe(1); // ONLY ONE LINE recorded despite two persist calls | |
| // Cleanup | |
| fs.rmSync(testDir, { recursive: true, force: true }); | |
| }); | |
| }); | |
| describe("Mechanism-Level Deduplication", () => { | |
| it("prevents duplicate log entries at the STORAGE level (Choke Point)", async () => { | |
| const { persistHookActivity, _resetForTest } = await vi.importActual("../../src/hooks/hook-activity-store") as any; | |
| const testDir = fs.mkdtempSync(path.join(os.tmpdir(), "failproofai-dedup-")); | |
| _resetForTest(testDir); | |
| try { | |
| // Simulation: First record call | |
| const entry1 = { | |
| timestamp: Date.now(), | |
| eventType: "Stop", | |
| sessionId: "sess-dedup", | |
| decision: "allow", | |
| policyName: "test-policy", | |
| durationMs: 100 | |
| } as any; | |
| persistHookActivity(entry1); | |
| // Simulation: Second record call with slightly different duration/timestamp | |
| // but same sessionId and eventType. | |
| // Window is > 50ms to hit "Twin" detection. | |
| const entry2 = { | |
| timestamp: Date.now() + 200, | |
| eventType: "Stop", | |
| sessionId: "sess-dedup", | |
| decision: "allow", | |
| policyName: "test-policy", | |
| durationMs: 95 | |
| } as any; | |
| persistHookActivity(entry2); | |
| // Verify that after the second call, the store logic should have dropped it | |
| const logPath = path.join(testDir, "current.jsonl"); | |
| const content = fs.readFileSync(logPath, "utf-8").trim(); | |
| const lines = content.split("\n").filter((l) => l.trim().length > 0); | |
| expect(lines.length).toBe(1); // ONLY ONE LINE recorded despite two persist calls | |
| } finally { | |
| fs.rmSync(testDir, { recursive: true, force: true }); | |
| } | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/hooks/handler.test.ts` around lines 826 - 868, The test is
polluting the user's home dir, lacks guaranteed cleanup, and shadows the
top-level fs import; change the test to create an isolated temp dir using
os.tmpdir() + fs.mkdtempSync (replace testDir assignment and call _resetForTest
with that temp dir), remove the dynamic import "const fs = await
import('node:fs')" and use the existing fs binding, and wrap the assertion and
reads in a try/finally so fs.rmSync(testDir, { recursive: true, force: true })
always runs (references: persistHookActivity, _resetForTest, testDir, and the
expect(lines.length).toBe(1) assertion).
Comment on lines
+413
to
421
|
|
||
| // Handle the case where stdin is not a pipe or is empty | ||
| setTimeout(() => { | ||
| if (chunks.length === 0) resolve(""); | ||
| }, 500); // 500ms timeout for slow pipes | ||
|
|
||
| process.stdin.on("error", reject); | ||
| // If stdin is already closed or not piped, resolve immediately | ||
| if (process.stdin.readableEnded) resolve(""); | ||
| }); |
There was a problem hiding this comment.
Unrefed 500 ms timeout keeps the event loop alive and may double-resolve.
The setTimeout at line 415 is never cleared when 'end' or 'error' resolves first, so:
- On a fast path with piped data, the handler still pins the event loop for up to 500 ms after work completes (noticeable in CLI exit latency and flaky tests).
- The Promise will ignore the late
resolve(""), but the timer keeps the process alive regardless.
Capture the handle and either .unref() it or clearTimeout on the end/error branches.
🛠️ Proposed fix
- setTimeout(() => {
- if (chunks.length === 0) resolve("");
- }, 500); // 500ms timeout for slow pipes
+ const t = setTimeout(() => {
+ if (chunks.length === 0) resolve("");
+ }, 500);
+ t.unref?.();
+ process.stdin.on("end", () => clearTimeout(t));
+ process.stdin.on("error", () => clearTimeout(t));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/handler.ts` around lines 413 - 421, The 500ms setTimeout in the
stdin-read Promise (the timer created near the comment handling chunks and
process.stdin) is never cleared, keeping the event loop alive and risking a late
resolve; capture the timeout handle when calling setTimeout, then call .unref()
on it (or clearTimeout(handle)) when the Promise resolves or rejects from the
'end'/'error' handlers (and also clear/unref it in the early-return branch that
checks process.stdin.readableEnded) to prevent the timer from keeping the
process alive and to avoid potential double-resolve races.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/hooks/manager.test.ts`:
- Line 67: The tests currently set process.env.FAILPROOFAI_DIST_PATH globally
which prevents exercising the project-scope "npx -y failproofai" branch; in the
affected test cases in __tests__/hooks/manager.test.ts (the spots around line 67
and ranges 284-296, 328-356, 378-388) remove or unset that env var before
invoking the code that should run the npx flow—specifically, ensure you call
delete process.env.FAILPROOFAI_DIST_PATH (or set it to undefined) in the
individual test(s) that assert the npx invocation so the code path that checks
for the absence of FAILPROOFAI_DIST_PATH is taken; keep other tests that rely on
the env var unchanged.
- Around line 545-569: Add unit tests exercising promptIntegrationSelection:
import promptIntegrationSelection and simulate interactive stdin by mocking
process.stdin.isTTY, setRawMode, resume, and emitting key events to cover arrow
up/down navigation, space toggle, Ctrl+A (select all), Enter to confirm, and
verify non-TTY fallback path returns the default selection; also assert the
empty-selection guard (prevent accepting no selection), and ensure cleanup runs
by checking setRawMode(false) is called, listeners are removed, and cursor
visibility is restored. Use vi.mocked to stub any dependencies (e.g., available
integration list), emit 'data' events on a mocked stdin stream to simulate keys,
and assert returned selections and side-effects (listener removal and raw mode
reset) for each scenario.
In `@src/hooks/install-prompt.ts`:
- Around line 432-436: Replace the local CommonJS require call that shadows the
top-level readline import: remove the line using require("node:readline") and
instead call the already-imported readline.emitKeypressEvents(process.stdin) so
the existing Key type and emitKeypressEvents function are used by
keypressHandler; ensure process.stdin is passed to emitKeypressEvents and that
the keypressHandler signature continues to reference readline.Key.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e57aaf5-47b6-44af-ba0a-420f9edbf64c
📒 Files selected for processing (4)
__tests__/hooks/manager.test.tsbin/failproofai.mjssrc/hooks/install-prompt.tssrc/hooks/manager.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- bin/failproofai.mjs
- src/hooks/manager.ts
| describe("hooks/manager", () => { | ||
| beforeEach(() => { | ||
| vi.resetAllMocks(); | ||
| process.env.FAILPROOFAI_DIST_PATH = MOCK_DIST_PATH; |
There was a problem hiding this comment.
Unset FAILPROOFAI_DIST_PATH in project-scope npx tests.
Line 67 sets FAILPROOFAI_DIST_PATH for every test, but the project-scope integration path only emits npx -y failproofai when that env var is absent. These assertions currently target the env-unset branch while running with the env var set.
Proposed fix
it("project scope uses portable npx -y failproofai command for all event types", async () => {
+ delete process.env.FAILPROOFAI_DIST_PATH;
vi.mocked(existsSync).mockReturnValue(true);
vi.mocked(readFileSync).mockReturnValue("{}");
@@
it("re-install on project scope migrates absolute-path hooks to npx format", async () => {
+ delete process.env.FAILPROOFAI_DIST_PATH;
vi.mocked(existsSync).mockReturnValue(true);
const existingSettings = {
@@
it("project scope console output shows portable command info", async () => {
+ delete process.env.FAILPROOFAI_DIST_PATH;
vi.mocked(existsSync).mockReturnValue(true);
vi.mocked(readFileSync).mockReturnValue("{}");Also applies to: 284-296, 328-356, 378-388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/hooks/manager.test.ts` at line 67, The tests currently set
process.env.FAILPROOFAI_DIST_PATH globally which prevents exercising the
project-scope "npx -y failproofai" branch; in the affected test cases in
__tests__/hooks/manager.test.ts (the spots around line 67 and ranges 284-296,
328-356, 378-388) remove or unset that env var before invoking the code that
should run the npx flow—specifically, ensure you call delete
process.env.FAILPROOFAI_DIST_PATH (or set it to undefined) in the individual
test(s) that assert the npx invocation so the code path that checks for the
absence of FAILPROOFAI_DIST_PATH is taken; keep other tests that rely on the env
var unchanged.
Comment on lines
+432
to
+436
| // Use a single data→keypress pipeline | ||
| const readline = require("node:readline"); | ||
| readline.emitKeypressEvents(process.stdin); | ||
|
|
||
| function keypressHandler(_str: string | undefined, key: readline.Key): void { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the readline import/require usage and package module settings.
rg -n -C3 'import \* as readline|const\s+readline\s*=\s*require\("node:readline"\)|key:\s*readline\.Key' src/hooks/install-prompt.ts
fd '^(package|tsconfig.*)\.json$' . -x sh -c '
echo "--- $1"
rg -n "\"type\"|\"module\"" "$1" || true
' sh {}Repository: exospherehost/failproofai
Length of output: 1099
Use the existing readline import instead of the local require.
Line 433 shadows the imported readline namespace with a local require, breaking the readline.Key type reference on line 436 in ESM environments. Since tsconfig.json outputs "module": "esnext", the require() call will not be available at runtime. The top-level import already provides emitKeypressEvents and the Key type.
Proposed fix
process.stdin.setRawMode(true);
process.stdin.resume();
// Use a single data→keypress pipeline
- const readline = require("node:readline");
readline.emitKeypressEvents(process.stdin);
function keypressHandler(_str: string | undefined, key: readline.Key): void {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Use a single data→keypress pipeline | |
| const readline = require("node:readline"); | |
| readline.emitKeypressEvents(process.stdin); | |
| function keypressHandler(_str: string | undefined, key: readline.Key): void { | |
| process.stdin.setRawMode(true); | |
| process.stdin.resume(); | |
| // Use a single data→keypress pipeline | |
| readline.emitKeypressEvents(process.stdin); | |
| function keypressHandler(_str: string | undefined, key: readline.Key): void { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/install-prompt.ts` around lines 432 - 436, Replace the local
CommonJS require call that shadows the top-level readline import: remove the
line using require("node:readline") and instead call the already-imported
readline.emitKeypressEvents(process.stdin) so the existing Key type and
emitKeypressEvents function are used by keypressHandler; ensure process.stdin is
passed to emitKeypressEvents and that the keypressHandler signature continues to
reference readline.Key.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
src/hooks/handler.ts (2)
227-236:⚠️ Potential issue | 🔴 CriticalReplace ESM-invalid
require()with an importedrmSync.This module uses ESM imports, so
require("node:fs")can throw at runtime. ImportrmSyncwith the other fs helpers and call it directly.🛠️ Proposed fix
-import { existsSync, readdirSync, mkdirSync, writeFileSync, readFileSync, appendFileSync } from "fs"; +import { existsSync, readdirSync, mkdirSync, writeFileSync, readFileSync, appendFileSync, rmSync } from "fs"; @@ export function _resetDedupeCache(): void { try { if (existsSync(DEDUP_DIR)) { - require("node:fs").rmSync(DEDUP_DIR, { recursive: true, force: true }); + rmSync(DEDUP_DIR, { recursive: true, force: true }); } } catch {#!/bin/bash # Verify CommonJS require usage remains in the ESM-style handler module. rg -n 'require\("node:fs"\)|import .* from "fs"' src/hooks/handler.tsAlso applies to: 28-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 227 - 236, The _resetDedupeCache function currently uses require("node:fs") which is invalid in this ESM module; replace that runtime require by importing rmSync from 'fs' alongside the other fs helpers and call rmSync(DEDUP_DIR, { recursive: true, force: true }) directly inside the try block, keeping the existsSync check and the existing catch behavior; update the import list at the top of the file to include rmSync so the function no longer uses require().
398-420:⚠️ Potential issue | 🟡 MinorClear or unref the stdin fallback timer.
The 500ms timer is never cleared when
endorerrorfires first, so fast hook invocations can still keep the process alive until the timer expires.🛠️ Proposed fix
payload = await new Promise<string>((resolve, reject) => { const chunks: string[] = []; let totalBytes = 0; + let settled = false; + const finish = (value: string) => { + if (settled) return; + settled = true; + clearTimeout(timeout); + resolve(value); + }; process.stdin.setEncoding("utf8"); @@ - resolve(""); + finish(""); return; } chunks.push(chunk); }); - process.stdin.on("end", () => resolve(chunks.join(""))); + process.stdin.on("end", () => finish(chunks.join(""))); // Handle the case where stdin is not a pipe or is empty - setTimeout(() => { - if (chunks.length === 0) resolve(""); + const timeout = setTimeout(() => { + if (chunks.length === 0) finish(""); }, 500); // 500ms timeout for slow pipes + timeout.unref?.(); - process.stdin.on("error", reject); - if (process.stdin.readableEnded) resolve(""); + process.stdin.on("error", (err) => { + clearTimeout(timeout); + reject(err); + }); + if (process.stdin.readableEnded) finish(""); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 398 - 420, The 500ms fallback timer started inside the payload Promise can keep the process alive because it isn't cleared when stdin 'end', 'error', early resolve (e.g., when MAX_STDIN_BYTES exceeded) or when process.stdin.readableEnded happens; update the Promise in the payload reading logic to store the timer id, clear that timer (clearTimeout) in the 'data' branch that resolves early (MAX_STDIN_BYTES), in the 'end' and 'error' handlers, and in the readableEnded check, and call timer.unref() after creating it so the timer won't keep the event loop alive if it remains; ensure you reference the same timer variable in the Promise surrounding code that reads stdin to avoid leaks.src/hooks/integrations.ts (2)
1463-1471:⚠️ Potential issue | 🟠 MajorRemove generated Pi debug logging.
The generated extension logs session IDs and project names on every CLI call. That exposes session/workspace metadata in user logs; remove it or guard it behind an explicit debug flag.
🧹 Proposed fix
- // DEBUG: Log available session values - const debugInfo = { - ctx_sessionId: ctx?.sessionId, - ctx_session_id: ctx?.session?.id, - pi_session_id: pi.session?.id, - pi_sessionId: pi.sessionId, - projectName, - }; - try { (pi as any).log?.(\`[FailproofAI Debug] Session ID sources: \${JSON.stringify(debugInfo)}\`); } catch {} -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/integrations.ts` around lines 1463 - 1471, Remove the generated Pi debug logging that prints session IDs and project names: delete the debugInfo object and the try { (pi as any).log?... } catch {} block (or replace it with a conditional that only runs when an explicit debug flag is set). Specifically update the code around the debugInfo variable and the (pi as any).log call so session/workspace metadata is no longer logged unless a secure, explicit debug flag (e.g., a new isDebugEnabled or process.env.FAILPROOF_DEBUG) is checked before constructing/logging debugInfo.
1318-1330:⚠️ Potential issue | 🔴 CriticalDo not recursively delete the OpenCode plugins directory.
Uninstalling
failproofai.tscurrently removes the whole.opencode/pluginsdirectory, which can delete unrelated user/project plugins. Only remove the generated file.🛡️ Proposed fix
if (content.includes("failproofai")) { unlinkSync(settingsPath); - // If the directory is empty after removal, clean it up too - const dir = dirname(settingsPath); - try { - if (dir.includes(".opencode/plugins")) { - rmSync(dir, { recursive: true, force: true }); - } - } catch {} return OPENCODE_HOOK_EVENT_TYPES.length; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/integrations.ts` around lines 1318 - 1330, The current removeHooksFromFile method unlinks the settingsPath file but then forcefully rmSyncs the entire .opencode/plugins directory; change it to only delete the file and avoid recursive deletion of plugins: in removeHooksFromFile, keep unlinkSync(settingsPath) when content.includes("failproofai"), remove the unconditional rmSync(dir, { recursive: true, force: true }) call, and instead, if you must remove the parent directory, only attempt a non-recursive removal when the parent dir is empty (e.g., check fs.readdirSync(dirname(settingsPath)).length === 0) and that dirname(settingsPath) is the direct plugin folder (still contains ".opencode/plugins"), using rmdirSync or rmSync without recursive to fail safely if not empty; reference removeHooksFromFile, settingsPath, and the ".opencode/plugins" check when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/e2e/hooks/cursor-integration.e2e.test.ts`:
- Line 13: Replace the require call with the already-imported homedir function:
update the DEDUP_DIR initialization (symbol DEDUP_DIR) to call homedir() instead
of require("node:os").homedir(), ensuring the file uses the imported homedir
from the top of the test file (the existing homedir import) so the ESM runtime
does not rely on require.
In `@__tests__/hooks/integrations.test.ts`:
- Around line 557-589: The test reimplements extraction with a local extractUuid
and asserts that a .txt filename yields a UUID, but the real implementation
getSessionIdFromFile only accepts .jsonl; replace the local extractUuid usage
with a call to the actual getSessionIdFromFile function in the test and remove
or change the wrong-extension assertion (the .txt case) to expect undefined so
the test aligns with the production behavior of getSessionIdFromFile.
In `@bin/failproofai.mjs`:
- Line 156: The help text for the --cli option is missing two supported values;
update the places that render the CLI choices (where INTEGRATION_TYPES is
referenced and the strings that list available CLIs) to include "opencode" and
"pi" so the top-level and policies help output match the actual supported
values; find the help generation for "--cli" and the policy help strings in
bin/failproofai.mjs (the lines that currently show
"claude-code|cursor|gemini|copilot|codex") and add "opencode|pi" to those option
listings.
- Around line 401-418: The list subcommand currently treats "--scope" as a known
flag (knownListFlags) but never consumes its value, causing the value to appear
in positionalArgs; to fix, treat "--scope" like "--cli" by finding its index
(e.g., const scopeIdx = subArgs.indexOf("--scope")) and if scopeIdx >= 0 add
scopeIdx and scopeIdx+1 to listConsumedIdxs (or at minimum add the value index)
so the scope value is excluded from positionalArgs; ensure you also handle the
case where "--scope" is the last arg and throw a CliError for a missing value.
- Around line 352-363: The current validation only checks the first integration
via getIntegration(integrationArg[0]), so add validation for every selected
integration: iterate over integrationArg, call getIntegration(name) for each
(same function), build each validScopes as [...integ.scopes, "all"], and verify
the parsed scope (the value derived from subArgs/--scope) is present for each
integ; if any integration lacks the scope, throw a CliError naming the offending
integration (use integ.displayName or the integration name) and listing its
valid values; keep the existing missing-value check for --scope and apply the
per-integration validation after determining scope.
In `@src/hooks/handler.ts`:
- Around line 243-280: The dedupe lock failure currently causes
tryAcquireFiringLock to return false for any write error, skipping policy
evaluation; update tryAcquireFiringLock so that only EEXIST causes a false
(meaning a twin ran), and any other error lets execution proceed (return true)
after optionally logging the write failure; modify the catch in the
writeFileSync block to check (e as any).code === "EEXIST" -> return false,
otherwise log a warning (including lockPath and error) and return true so
policies still run; keep references to lockDir, lockPath, signature, identity,
createHash and the 5-second bucket logic intact.
In `@src/hooks/integrations.ts`:
- Around line 359-363: The buildHookEntry implementation writes machine-specific
paths (process.execPath and binaryPath) into .cursor/hooks.json and also drops
scope; change buildHookEntry to accept and pass through the scope argument and
when scope === "project" emit the same portable project-style command used by
the other integrations (instead of embedding process.execPath/binaryPath), while
for non-project scopes keep the existing executable-based command; update
references to CURSOR_EVENT_MAP and the returned object so the --scope flag is
preserved and the command string is portable for project-scoped hooks.
---
Duplicate comments:
In `@src/hooks/handler.ts`:
- Around line 227-236: The _resetDedupeCache function currently uses
require("node:fs") which is invalid in this ESM module; replace that runtime
require by importing rmSync from 'fs' alongside the other fs helpers and call
rmSync(DEDUP_DIR, { recursive: true, force: true }) directly inside the try
block, keeping the existsSync check and the existing catch behavior; update the
import list at the top of the file to include rmSync so the function no longer
uses require().
- Around line 398-420: The 500ms fallback timer started inside the payload
Promise can keep the process alive because it isn't cleared when stdin 'end',
'error', early resolve (e.g., when MAX_STDIN_BYTES exceeded) or when
process.stdin.readableEnded happens; update the Promise in the payload reading
logic to store the timer id, clear that timer (clearTimeout) in the 'data'
branch that resolves early (MAX_STDIN_BYTES), in the 'end' and 'error' handlers,
and in the readableEnded check, and call timer.unref() after creating it so the
timer won't keep the event loop alive if it remains; ensure you reference the
same timer variable in the Promise surrounding code that reads stdin to avoid
leaks.
In `@src/hooks/integrations.ts`:
- Around line 1463-1471: Remove the generated Pi debug logging that prints
session IDs and project names: delete the debugInfo object and the try { (pi as
any).log?... } catch {} block (or replace it with a conditional that only runs
when an explicit debug flag is set). Specifically update the code around the
debugInfo variable and the (pi as any).log call so session/workspace metadata is
no longer logged unless a secure, explicit debug flag (e.g., a new
isDebugEnabled or process.env.FAILPROOF_DEBUG) is checked before
constructing/logging debugInfo.
- Around line 1318-1330: The current removeHooksFromFile method unlinks the
settingsPath file but then forcefully rmSyncs the entire .opencode/plugins
directory; change it to only delete the file and avoid recursive deletion of
plugins: in removeHooksFromFile, keep unlinkSync(settingsPath) when
content.includes("failproofai"), remove the unconditional rmSync(dir, {
recursive: true, force: true }) call, and instead, if you must remove the parent
directory, only attempt a non-recursive removal when the parent dir is empty
(e.g., check fs.readdirSync(dirname(settingsPath)).length === 0) and that
dirname(settingsPath) is the direct plugin folder (still contains
".opencode/plugins"), using rmdirSync or rmSync without recursive to fail safely
if not empty; reference removeHooksFromFile, settingsPath, and the
".opencode/plugins" check when making this change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f6a0de7-05a5-4e51-adea-59c0e9d53632
📒 Files selected for processing (14)
__tests__/e2e/hooks/copilot-integration.e2e.test.ts__tests__/e2e/hooks/cursor-integration.e2e.test.ts__tests__/e2e/hooks/gemini-integration.e2e.test.ts__tests__/hooks/integrations.test.ts__tests__/hooks/manager.test.tsbin/failproofai.mjsdocs/cli/hook.mdxdocs/cli/install-policies.mdxdocs/cli/list-policies.mdxdocs/cli/remove-policies.mdxdocs/getting-started.mdxsrc/hooks/handler.tssrc/hooks/integrations.tssrc/hooks/manager.ts
✅ Files skipped from review due to trivial changes (6)
- docs/getting-started.mdx
- docs/cli/install-policies.mdx
- docs/cli/hook.mdx
- docs/cli/remove-policies.mdx
- docs/cli/list-policies.mdx
- tests/e2e/hooks/copilot-integration.e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/hooks/gemini-integration.e2e.test.ts
- src/hooks/manager.ts
| const CURSOR_HOOKS_PATH = resolve(PROJECT_DIR, ".cursor", "hooks.json"); | ||
| const CONFIG_PATH = resolve(PROJECT_DIR, ".failproofai", "policies-config.json"); | ||
| // Firing-lock files can persist across test cases. Clear them. | ||
| const DEDUP_DIR = resolve(require("node:os").homedir(), ".failproofai", "cache", "dedup"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this file mixes ESM imports with CommonJS require.
rg -n 'import .* from "node:os"|require\("node:os"\)' __tests__/e2e/hooks/cursor-integration.e2e.test.tsRepository: exospherehost/failproofai
Length of output: 199
🏁 Script executed:
cat -n __tests__/e2e/hooks/cursor-integration.e2e.test.ts | head -30Repository: exospherehost/failproofai
Length of output: 1787
Use the imported homedir() instead of require() on line 13.
This file uses ESM imports throughout, but line 13 calls require("node:os").homedir(). In an ESM runtime, require is undefined unless shimmed. The imported homedir function from line 5 is already available and should be used directly.
🛠️ Proposed fix
-const DEDUP_DIR = resolve(require("node:os").homedir(), ".failproofai", "cache", "dedup");
+const DEDUP_DIR = resolve(homedir(), ".failproofai", "cache", "dedup");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const DEDUP_DIR = resolve(require("node:os").homedir(), ".failproofai", "cache", "dedup"); | |
| const DEDUP_DIR = resolve(homedir(), ".failproofai", "cache", "dedup"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/e2e/hooks/cursor-integration.e2e.test.ts` at line 13, Replace the
require call with the already-imported homedir function: update the DEDUP_DIR
initialization (symbol DEDUP_DIR) to call homedir() instead of
require("node:os").homedir(), ensuring the file uses the imported homedir from
the top of the test file (the existing homedir import) so the ESM runtime does
not rely on require.
Comment on lines
+557
to
+589
| it("getSessionIdFromFile correctly extracts UUID from Pi session filenames", () => { | ||
| // Test the extraction logic using string methods (not regex) | ||
| // Format: TIMESTAMP_UUID.jsonl | ||
| const extractUuid = (filename: string): string | undefined => { | ||
| const underscore = filename.lastIndexOf('_'); | ||
| const dot = filename.lastIndexOf('.'); | ||
| if (underscore > 0 && dot > underscore) { | ||
| return filename.slice(underscore + 1, dot); | ||
| } | ||
| return undefined; | ||
| }; | ||
|
|
||
| // Valid Pi session filename with timestamp and UUID | ||
| const validFilename = "2026-04-21T12-35-19-493Z_019db009-b545-7792-bad7-6ea5116cd432.jsonl"; | ||
| expect(extractUuid(validFilename)).toBe("019db009-b545-7792-bad7-6ea5116cd432"); | ||
|
|
||
| // Another valid format with different timestamp | ||
| const valid2 = "2026-01-01T00-00-00-000Z_aaaabbbb-cccc-dddd-eeee-ffff00001111.jsonl"; | ||
| expect(extractUuid(valid2)).toBe("aaaabbbb-cccc-dddd-eeee-ffff00001111"); | ||
|
|
||
| // Valid: UUID can contain dashes and hex chars | ||
| const valid3 = "2026-01-01T00-00-00-000Z_f0f0f0f0-a1a1-b2b2-c3c3-d4d4d4d4d4d4.jsonl"; | ||
| expect(extractUuid(valid3)).toBe("f0f0f0f0-a1a1-b2b2-c3c3-d4d4d4d4d4d4"); | ||
|
|
||
| // Invalid: no underscore | ||
| expect(extractUuid("2026-04-21T12-35-19-493Z.jsonl")).toBeUndefined(); | ||
|
|
||
| // Invalid: no dot | ||
| expect(extractUuid("2026-04-21T12-35-19-493Z_019db009-b545-7792-bad7-6ea5116cd432")).toBeUndefined(); | ||
|
|
||
| // Edge case: wrong extension - still extracts UUID (finds last dot) | ||
| expect(extractUuid("2026-04-21T12-35-19-493Z_019db009-b545-7792-bad7-6ea5116cd432.txt")).toBe("019db009-b545-7792-bad7-6ea5116cd432"); | ||
| }); |
There was a problem hiding this comment.
Align the Pi filename test with the generated implementation.
This test reimplements extraction locally and expects .txt to work, but the generated Pi extension only considers files ending in .jsonl. Exercise the generated output or remove the wrong-extension expectation so the test does not encode behavior production never uses.
As per coding guidelines, “Never edit a test to make it pass. Fix the code. Tests may only be changed when the test itself is wrong or when the feature under test was intentionally changed.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/hooks/integrations.test.ts` around lines 557 - 589, The test
reimplements extraction with a local extractUuid and asserts that a .txt
filename yields a UUID, but the real implementation getSessionIdFromFile only
accepts .jsonl; replace the local extractUuid usage with a call to the actual
getSessionIdFromFile function in the test and remove or change the
wrong-extension assertion (the .txt case) to expect undefined so the test aligns
with the production behavior of getSessionIdFromFile.
| --beta Remove only beta policies | ||
| --custom, -c Clear the customPoliciesPath from config | ||
|
|
||
| --cli claude-code|cursor|gemini|copilot|codex Target platform (default: claude-code) |
There was a problem hiding this comment.
Include opencode and pi in the --cli help text.
INTEGRATION_TYPES supports seven CLIs, but the top-level and policies help omit opencode and pi, so users cannot discover supported values from --help.
📝 Proposed fix
- --cli claude-code|cursor|gemini|copilot|codex Target platform (default: claude-code)
+ --cli claude-code|cursor|gemini|copilot|codex|opencode|pi Target platform (default: claude-code)- --cli claude-code|cursor|gemini|copilot|codex Target platform (default: claude-code)
+ --cli claude-code|cursor|gemini|copilot|codex|opencode|pi Target platform (default: claude-code)Also applies to: 239-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/failproofai.mjs` at line 156, The help text for the --cli option is
missing two supported values; update the places that render the CLI choices
(where INTEGRATION_TYPES is referenced and the strings that list available CLIs)
to include "opencode" and "pi" so the top-level and policies help output match
the actual supported values; find the help generation for "--cli" and the policy
help strings in bin/failproofai.mjs (the lines that currently show
"claude-code|cursor|gemini|copilot|codex") and add "opencode|pi" to those option
listings.
Comment on lines
+352
to
+363
| const { getIntegration } = await import("../src/hooks/integrations"); | ||
| // Use the first integration for scope validation | ||
| const integ = getIntegration(integrationArg[0]); | ||
| const validScopes = [...integ.scopes, "all"]; | ||
|
|
||
| const scopeIdx = subArgs.indexOf("--scope"); | ||
| const scope = scopeIdx >= 0 ? subArgs[scopeIdx + 1] : "user"; | ||
| if (scopeIdx >= 0 && (!scope || scope.startsWith("-"))) { | ||
| throw new CliError("Missing value for --scope. Valid values: user, project, local, all"); | ||
| throw new CliError(`Missing value for --scope. Valid values: ${validScopes.join(", ")}`); | ||
| } | ||
| if (scopeIdx >= 0 && !["user", "project", "local", "all"].includes(scope)) { | ||
| throw new CliError(`Invalid scope: ${scope}. Valid values: user, project, local, all`); | ||
| if (scopeIdx >= 0 && !validScopes.includes(scope)) { | ||
| throw new CliError(`Invalid scope: ${scope}. Valid values for ${integ.displayName}: ${validScopes.join(", ")}`); |
There was a problem hiding this comment.
Validate uninstall scopes against every selected CLI.
Using only integrationArg[0] means failproofai policies --uninstall --cli claude-code cursor --scope local passes CLI validation even though Cursor has no local scope. Downstream code can then resolve an unintended default path for unsupported integrations.
🛠️ Proposed fix
- const { getIntegration } = await import("../src/hooks/integrations");
- // Use the first integration for scope validation
- const integ = getIntegration(integrationArg[0]);
- const validScopes = [...integ.scopes, "all"];
+ const { getIntegration } = await import("../src/hooks/integrations");
+ const integrations = integrationArg.map((id) => getIntegration(id));
+ const validScopes = [
+ ...integrations
+ .map((integ) => new Set(integ.scopes))
+ .reduce((acc, scopes) => new Set([...acc].filter((scope) => scopes.has(scope)))),
+ "all",
+ ];
@@
- throw new CliError(`Invalid scope: ${scope}. Valid values for ${integ.displayName}: ${validScopes.join(", ")}`);
+ throw new CliError(`Invalid scope: ${scope}. Valid values for selected CLIs: ${validScopes.join(", ")}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/failproofai.mjs` around lines 352 - 363, The current validation only
checks the first integration via getIntegration(integrationArg[0]), so add
validation for every selected integration: iterate over integrationArg, call
getIntegration(name) for each (same function), build each validScopes as
[...integ.scopes, "all"], and verify the parsed scope (the value derived from
subArgs/--scope) is present for each integ; if any integration lacks the scope,
throw a CliError naming the offending integration (use integ.displayName or the
integration name) and listing its valid values; keep the existing missing-value
check for --scope and apply the per-integration validation after determining
scope.
Comment on lines
+243
to
+280
| function tryAcquireFiringLock(eventType: string, sessionId: string | undefined, fingerprint?: string): boolean { | ||
| if (!sessionId) return true; // Can't safely deduplicate without session | ||
|
|
||
| const lockDir = join(DEDUP_DIR, "firing-locks"); | ||
| if (!existsSync(lockDir)) { | ||
| try { mkdirSync(lockDir, { recursive: true }); } catch { /* ignore */ } | ||
| } | ||
|
|
||
| // Identity is based on event type + session ID. | ||
| // For tool events, we also include the fingerprint of the command/input. | ||
| const isTool = eventType?.toLowerCase().includes("tool"); | ||
| const signatureParts = [eventType, sessionId]; | ||
| if (isTool && fingerprint) { | ||
| signatureParts.push(fingerprint); | ||
| } | ||
|
|
||
| // Read stdin payload (Claude passes JSON) | ||
| // Use a 5-second bucket to ensure the lock only applies to near-simultaneous firings (twins). | ||
| // This prevents stale locks from previous runs/tests from blocking fresh operations. | ||
| const bucket = Math.floor(Date.now() / 5000); | ||
| const signature = signatureParts.join("|") + `|${bucket}`; | ||
| const identity = createHash("sha256").update(signature).digest("hex"); | ||
| const lockPath = join(lockDir, `${identity}.lock`); | ||
|
|
||
| // Check if lock exists (bucket ensured it's recent) | ||
| if (existsSync(lockPath)) { | ||
| return false; // Twin already ran in this 5s window | ||
| } | ||
|
|
||
| // Acquire lock: atomic exclusive-create. `wx` fails with EEXIST if a twin | ||
| // process got here first — that's how we make this a real lock instead of a | ||
| // racy overwrite. | ||
| try { | ||
| writeFileSync(lockPath, String(process.pid), { flag: "wx" }); | ||
| return true; | ||
| } catch (e) { | ||
| if ((e as any).code === "EEXIST") return false; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Do not skip policy evaluation when the dedupe lock cannot be written.
If ~/.failproofai/cache/dedup is unwritable or lock creation fails for any reason other than EEXIST, tryAcquireFiringLock() returns false, and the hook exits 0 before evaluating policies. Dedup failures should proceed without dedup rather than bypass enforcement.
🛡️ Proposed fix
const lockDir = join(DEDUP_DIR, "firing-locks");
if (!existsSync(lockDir)) {
- try { mkdirSync(lockDir, { recursive: true }); } catch { /* ignore */ }
+ try {
+ mkdirSync(lockDir, { recursive: true });
+ } catch {
+ return true;
+ }
}
@@
try {
writeFileSync(lockPath, String(process.pid), { flag: "wx" });
return true;
} catch (e) {
if ((e as any).code === "EEXIST") return false;
- return false;
+ return true;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function tryAcquireFiringLock(eventType: string, sessionId: string | undefined, fingerprint?: string): boolean { | |
| if (!sessionId) return true; // Can't safely deduplicate without session | |
| const lockDir = join(DEDUP_DIR, "firing-locks"); | |
| if (!existsSync(lockDir)) { | |
| try { mkdirSync(lockDir, { recursive: true }); } catch { /* ignore */ } | |
| } | |
| // Identity is based on event type + session ID. | |
| // For tool events, we also include the fingerprint of the command/input. | |
| const isTool = eventType?.toLowerCase().includes("tool"); | |
| const signatureParts = [eventType, sessionId]; | |
| if (isTool && fingerprint) { | |
| signatureParts.push(fingerprint); | |
| } | |
| // Read stdin payload (Claude passes JSON) | |
| // Use a 5-second bucket to ensure the lock only applies to near-simultaneous firings (twins). | |
| // This prevents stale locks from previous runs/tests from blocking fresh operations. | |
| const bucket = Math.floor(Date.now() / 5000); | |
| const signature = signatureParts.join("|") + `|${bucket}`; | |
| const identity = createHash("sha256").update(signature).digest("hex"); | |
| const lockPath = join(lockDir, `${identity}.lock`); | |
| // Check if lock exists (bucket ensured it's recent) | |
| if (existsSync(lockPath)) { | |
| return false; // Twin already ran in this 5s window | |
| } | |
| // Acquire lock: atomic exclusive-create. `wx` fails with EEXIST if a twin | |
| // process got here first — that's how we make this a real lock instead of a | |
| // racy overwrite. | |
| try { | |
| writeFileSync(lockPath, String(process.pid), { flag: "wx" }); | |
| return true; | |
| } catch (e) { | |
| if ((e as any).code === "EEXIST") return false; | |
| return false; | |
| } | |
| function tryAcquireFiringLock(eventType: string, sessionId: string | undefined, fingerprint?: string): boolean { | |
| if (!sessionId) return true; // Can't safely deduplicate without session | |
| const lockDir = join(DEDUP_DIR, "firing-locks"); | |
| if (!existsSync(lockDir)) { | |
| try { | |
| mkdirSync(lockDir, { recursive: true }); | |
| } catch { | |
| return true; | |
| } | |
| } | |
| // Identity is based on event type + session ID. | |
| // For tool events, we also include the fingerprint of the command/input. | |
| const isTool = eventType?.toLowerCase().includes("tool"); | |
| const signatureParts = [eventType, sessionId]; | |
| if (isTool && fingerprint) { | |
| signatureParts.push(fingerprint); | |
| } | |
| // Use a 5-second bucket to ensure the lock only applies to near-simultaneous firings (twins). | |
| // This prevents stale locks from previous runs/tests from blocking fresh operations. | |
| const bucket = Math.floor(Date.now() / 5000); | |
| const signature = signatureParts.join("|") + `|${bucket}`; | |
| const identity = createHash("sha256").update(signature).digest("hex"); | |
| const lockPath = join(lockDir, `${identity}.lock`); | |
| // Check if lock exists (bucket ensured it's recent) | |
| if (existsSync(lockPath)) { | |
| return false; // Twin already ran in this 5s window | |
| } | |
| // Acquire lock: atomic exclusive-create. `wx` fails with EEXIST if a twin | |
| // process got here first — that's how we make this a real lock instead of a | |
| // racy overwrite. | |
| try { | |
| writeFileSync(lockPath, String(process.pid), { flag: "wx" }); | |
| return true; | |
| } catch (e) { | |
| if ((e as any).code === "EEXIST") return false; | |
| return true; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/handler.ts` around lines 243 - 280, The dedupe lock failure
currently causes tryAcquireFiringLock to return false for any write error,
skipping policy evaluation; update tryAcquireFiringLock so that only EEXIST
causes a false (meaning a twin ran), and any other error lets execution proceed
(return true) after optionally logging the write failure; modify the catch in
the writeFileSync block to check (e as any).code === "EEXIST" -> return false,
otherwise log a warning (including lockPath and error) and return true so
policies still run; keep references to lockDir, lockPath, signature, identity,
createHash and the 5-second bucket logic intact.
Point Copilot user-scope hook installs to COPILOT_HOME/.copilot config.json so user-level hooks are actually loaded, and add Codex log trace parsing utilities plus tests for extracting tool-call records. Made-with: Cursor
- buildHookEntry/writeHookEntries accept optional scope param - project scope uses npx -y failproofai; user/local use binary path - resolveFailproofaiBinary falls back to which/where failproofai - test: add execSync mock and fix binary path expectations Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ity entries - Add migrateIntegrationField() to backfill missing integration field on old entries - Migration runs once on first dashboard load (marked with migration marker) - Detects integration type from hook_event_name patterns for pre-feature entries - Falls back to "claude-code" for entries without detection signals - Add defensive fallback in IntegrationBadge for any remaining edge cases - Includes comprehensive tests for migration logic This solves the issue where old activity entries (created before multi-platform support) were showing as "—" because they lacked the integration field. After migration, all entries have accurate integration data. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Update hook handler to support cursor IDE integration - Enhance hook-activity-store with integration tracking - Add corresponding unit and e2e tests for integration flows Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
OpenCode and Pi display stderr in their UI. The debug-prompt policy was outputting diagnostic messages like '[failproofai] debug-prompt: Prompt captured from opencode' to stderr, creating unwanted clutter in the user interface. Solution: Suppress stderr output for allow entries (informational messages) only for OpenCode and Pi, while keeping it for other integrations (Claude, Cursor, Gemini, Copilot, Codex) where stderr is used for logging but not displayed in UI. All important policy information still reaches these integrations via stdout (hookSpecificOutput.additionalContext). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…lication Split policy evaluation logic into distinct phases (match, evaluate, enforce) for better separation of concerns and maintainability. Remove redundant registry lookups and streamline the hook runner pipeline. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…calls for non-Claude agents - Fix normalizePayload for Cursor: map beforeShellExecution/afterShellExecution to run_terminal_command, file events to edit_file, MCP events to mcp_tool — prevents policies like block-sudo and block-rm-rf from being bypassed when tool_name was incorrectly set to the hook event name - Fix beforeReadFile events: wrap top-level file_path into tool_input so blockEnvFiles and other file-path policies can inspect the path - Fix getCanonicalEventName for Cursor to also consult CURSOR_EVENT_MAP on the payload's hook_event_name as a safety net - Fix warn-repeated-tool-calls: derive per-session tracker path from session_id when transcriptPath is absent (Cursor, Copilot, Codex etc.), storing counters in ~/.failproofai/cache/tool-calls/<session_id>.json - Add E2E regression tests for beforeShellExecution sudo block and beforeReadFile env-file block Co-Authored-By: Yash Upadhyay <yashupadhyayy1@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Install Copilot hook commands with native camelCase event names (sessionStart, preToolUse, etc.) instead of pre-mapping to PascalCase. This restores the unique on-the-wire signal that distinguishes Copilot from Claude even when the --integration flag is not honored (e.g. older published handlers). Also harden synchronizeCopilotProjectHooks: early-return when no project hooks file exists so user-scope entries survive, and only strip project-scope (npx) entries during merge rather than wiping all failproofai hooks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…otocol
FIXES REGRESSION:
- Duplicate hooks causing prompts to fail when sent to Copilot
- Duplicate dashboard entries (Claude + Copilot for same event)
- Multiple hook processes firing in parallel with conflicting exit codes
CHANGES:
1. Hook Deduplication (integrations.ts):
- Modified Copilot writeHookEntries to remove ALL failproofai hooks
before writing new ones (not just replace first match)
- Prevents accumulation of duplicate hooks in config files
- Ensures exactly ONE failproofai hook per event type
2. Copilot Blocking Protocol (policy-evaluator.ts):
- Restored exitCode: 0 for Copilot policy denials
- Added event-specific JSON payloads for Copilot:
* PreToolUse/Selection: permissionDecision: "deny"
* PostToolUse: additionalContext with reason
* Stop/Agent events: action: "cancel"
- Ensures Copilot CLI correctly parses security blocks
3. Copilot Event Attribution (handler.ts):
- Fixed integration detection for camelCase events
- Added HOOK_EVENT_TYPES import for PascalCase distinction
- Silence guard now prevents legacy Claude-labeled Copilot events
- Prioritized COPILOT_SESSION_ID for empty payloads
4. Integration Detection (integrations.ts):
- Enhanced Copilot detect() with env var checks
- Fallback for headless/empty-payload scenarios
5. Test Coverage:
- Added 6 comprehensive deduplication tests
- All 1012 tests passing (1006 existing + 6 new)
REGRESSION PREVENTION:
- Tests prevent future duplicate hook accumulation
- Handler validates camelCase/PascalCase event signatures
- Deduplication works across multiple hook installs
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gration - Refine blocking policy logic and test assertions in builtin-policies - Expand integration test suite with additional scenarios - Improve hook handler initialization for better integration support - Optimize integrations module for cursor/copilot protocol handling Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…ng synthetic ID Pi stores sessions in ~/.pi/agent/sessions/<workspace>/ with filenames like: YYYY-MM-DDTHH-MM-SS-sssZ_<SESSION-UUID>.jsonl The session ID is the UUID in the filename, which is stable across events. The old fallback was generating a synthetic 'pi-ProjectName-timestamp' ID on every event, causing session fragmentation. Added getSessionIdFromFile() helper to the generated Pi extension that: 1. Computes the workspace folder name from cwd 2. Reads the session files from the workspace directory 3. Extracts the UUID from the most recently modified .jsonl filename 4. Falls back to synthetic ID only if no real session file exists Uses string methods instead of regex for compatibility with Pi's TypeScript runtime. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…entation - Remove integration documentation files (execution guide, plan, test cases) - Clean up disabled/old bin files - Expand log-entries and projects lib functionality - Update handler, policies, and integration test coverage - Add session ID navigation to project page Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…lve test failures Session log dashboard now displays user input and LLM output for all integrations: - Add toolInput/toolOutput to activityEntry for proper dashboard rendering - Wire writeVirtualLogEntry for non-Claude integrations with UUID threading - Fix tool_output field fallback in writeVirtualLogEntry PostToolUse - Add unit tests verifying toolInput/toolOutput handling Earlier fixes restored from session commits: - integrations.ts: use computed scope-aware command in buildHookEntry - manager.ts: remove duplicate hook-writing loop - policy-evaluator.ts: unify deny path and fix hardStop logic Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Renamed '--integration' flag to '--cli' throughout the codebase. - Enabled '--cli' to accept multiple values (e.g., '--cli cursor gemini'). - Updated 'failproofai policies' to show a summary of all available CLIs and their hook status. - Refreshed documentation in 'getting-started.mdx' and CLI reference files. - Updated all E2E and unit tests to reflect the new flag name. - Removed the '--integration' legacy alias fully from source and rebuilt artifacts. Co-authored-by: Gemini CLI <gemini-cli@google.com>
Document scope availability per integration in both install and uninstall CLI references so users can quickly choose valid --scope values. Made-with: Cursor
Expand install/uninstall scope matrices with CLI-specific official config layers and explicit unsupported layers so users can quickly see what failproofai manages versus external system settings. Made-with: Cursor
yashupadhyayy1
force-pushed
the
feat/cursor-integration
branch
from
beca282 to
b56cc79
Compare
Remove continue: false from tool-level deny responses so the agent turn continues and Gemini can explain the block to the user (matching Claude's behavior). Only AfterAgent/Stop retains continue: false per spec. Fall back to exit code 2 for BeforeToolSelection since the spec explicitly disallows decision/continue/systemMessage on that event. Separate reason (concise, agent-facing) from systemMessage (verbose, terminal display). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ell matching - Gemini deny responses now use explicit agent-facing reason: 'FailproofAI is blocking this action...' - For Gemini pre-execution events, omit systemMessage and retry suffix from reason - Keep systemMessage for post-execution events where supported - Preserve BeforeToolSelection fallback behavior and protocol expectations - Broaden shell tool-name coverage for PreToolUse builtins via shared SHELL_TOOL_NAMES (includes Bash, Shell, run_shell_command, run_terminal_command, bash_login_shell, sh) - Prevent Gemini shell alias bypasses so active policies block consistently pre-exec Tests and docs: - Update policy-evaluator tests for new Gemini reason and systemMessage expectations - Update Gemini integration e2e expectations for BeforeTool deny payload - Add Gemini e2e regression for Shell tool-name env blocking - Include manager/docs wording refinements provided in this branch: * 'hooks not active' copy * 'Detected AI CLIs' section and status legend * clarified activation hint Co-authored-by: Codex (GPT-5)
…e, and coverage Source fixes (9 bugs, 1 silent failure): - B1: OpenCode removeHooksFromFile no longer deletes parent .opencode/plugins/ dir - B2: tryAcquireFiringLock now includes integration in hash (cross-CLI dedup collision) - B3: getDedupeKey always prepends integration as primary component - B4: Session ID env-var fallbacks scoped to matching integration only - B5: Silence Guard fires after payload parse, not only when --cli is present - B6: Cursor normalizePayload maps edit_file→Write, read_file→Read for policy matching - B7: .ts custom hook files now transpiled via bun build before ESM rewriting - B8: Gemini normalizePayload extracts session_id from payload fields + env var - B9: Custom policy policyParams no longer silently dropped (no schema = pass raw params) - S1: installHooks warns when Stop-event policies are installed for OpenCode/Pi - Fix block-curl-pipe-sh false positive: curl/wget redirect (>) is allowed; only explicit -o/-O output flags trigger the block Test additions (1105 unit + 242 E2E, all green): - handler.test.ts: cross-CLI dedup isolation + session ID env-var scoping - integrations.test.ts: Cursor tool name canonicalization + Gemini session_id extraction + OpenCode removeHooksFromFile dir-safety (rmSync never called) - loader-utils.test.ts: createEsmShim exports, rewriteFileTree import rewriting, circular import termination, maybeTranspileTypeScript bun invocation - manager.test.ts: Stop-event compatibility warning for OpenCode/Pi - E2E: claude-code-integration, codex-integration, opencode-integration, pi-integration (4 new files covering all previously untested CLIs) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add .codex to .gitignore (mirrors .cursor entry) - Align gemini-integration.e2e.test.ts assertions with current getGeminiProhibitedMessage format (systemMessage/reason parity) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
|
Superseded by #185 which includes all the same work plus additional fixes and permissionMode tracking. |
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
2 participants
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.
Summary
Integrate Cursor, Copilot, OpenCode, Gemini, and Pi IDE support into failproofai with comprehensive test coverage and infrastructure improvements.
Features
Improvements
Files Changed
Test Results
Cleanup
Co-Authored-By: Yash Upadhyay yashupadhyayy1@gmail.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores