feat: multi-CLI integration — Cursor, Copilot, Codex, OpenCode, Pi, Gemini + permissionMode tracking#185
Conversation
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
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>
…emini bypass block-sudo used a hardcoded toolNames list that omitted run_shell_command and sh. When the real Gemini CLI sends tool_name: run_shell_command the policy-registry filter excluded block-sudo entirely before the policy function ran. Fix: both block-sudo and block-read-outside-cwd now reference SHELL_TOOL_NAMES so all shell tool variants (including Gemini's run_shell_command) are covered. Add regression tests asserting the match list and policy behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…onfig.json Agents could use Read/ReadFile tools or shell commands to read .failproofai/policies-config.json, learning which security policies are active and potentially crafting commands to work around them. Fix: block-failproofai-commands now fires on all PreToolUse events (no toolNames filter). Checks file_path for Read/ReadFile and command string for Bash before the existing CLI/uninstall guards. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The header "Failproof AI Hook Policies (Claude Code)" was confusing when multiple CLIs are installed — it implied policies only applied to one CLI. The "Detected AI CLIs" section already shows per-CLI status at the bottom. Also change require-push-before-stop remote param default to empty string so the policy auto-detects the tracking remote instead of hardcoding origin. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sion support Populate permissionMode in activity entries for all CLIs (Codex reads approval_policy from session transcript; Cursor/Copilot/Gemini walk /proc ancestor tree to parse mode flags). Add resolve-permission-mode module with tests. Expand dashboard parsing: OpenCode sessions now load from opencode.db, Gemini/Pi native transcript discovery is broadened, and Gemini chat discovery targets real chat files while ignoring sidecar artifacts. Add custom-install and allow-patterns e2e test suites with integration adapter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…allback Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughThis PR adds multi‑integration support (Claude Code, Cursor, Gemini, Copilot, Codex, OpenCode, Pi): integration registry, payload normalization, integration-aware hook install/manager/handler/evaluator, activity-store migration/dedup, native transcript parsing and Copilot session support, extensive e2e/unit tests, docs, CLI enhancements (--cli, copilot-sync), loader/TS transpile changes, and removes several legacy per-CLI config files. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(240,240,255,0.5)
participant User
participant CLI as "bin/failproofai.mjs"
participant Handler as "src/hooks/handler"
participant Detector as "src/hooks/integrations"
participant Evaluator as "src/hooks/policy-evaluator"
participant Store as "src/hooks/hook-activity-store"
end
User->>CLI: invoke --hook + payload (optional --cli)
CLI->>Handler: handleHookEvent(eventType, cliOverride)
Handler->>Detector: detect integration & normalize payload
Detector-->>Handler: integrationId, canonicalEvent
Handler->>Handler: dedupe (fingerprint/locks)
alt duplicate
Handler-->>CLI: return {exitCode, hardStop}
else
Handler->>Evaluator: evaluate policies (normalized ctx)
Evaluator-->>Handler: decision, reason, hardStop
Handler->>Store: persist activity (integration + details)
Store-->>Handler: persisted
Handler-->>CLI: return {exitCode, hardStop}
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major 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 (3)
docs/getting-started.mdx (1)
162-168:⚠️ Potential issue | 🟡 MinorUninstalling section is stale for multi-CLI.
This PR adds hook installs for Cursor, Gemini, Copilot, Codex, OpenCode, and Pi, but the Uninstalling section still describes only
~/.claude/settings.json. Please update to reflect what--uninstallremoves across all integrations, and whether--cliis honored on uninstall.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started.mdx` around lines 162 - 168, Update the "Uninstalling" section for the command failproofai policies --uninstall to state that it removes hook entries from all supported CLIs (Claude plus Cursor, Gemini, Copilot, Codex, OpenCode, and Pi) rather than only referencing Claude; also clarify that config files in the user FailProofAI config directory are preserved and that the --cli flag can be used to limit uninstall to a specific CLI integration when provided.app/components/project-list.tsx (1)
287-292:⚠️ Potential issue | 🟡 Minor
colSpan={4}is off-by-one after adding the Integration column.The header now has five
<th>cells (Icon, Agent Root, Path, Integration, Last Modified), so the empty-state row should span 5. WithcolSpan={4}the placeholder cell will leave the rightmost column visually empty/misaligned when no folders match.🔧 Proposed fix
{paginatedFolders.length === 0 ? ( <tr> - <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"> No projects found matching the selected filter. </td> </tr>🤖 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 287 - 292, The empty-state row inside the ProjectList component (the conditional block checking paginatedFolders.length === 0) uses <td colSpan={4}> but the table header now has five <th> cells (Icon, Agent Root, Path, Integration, Last Modified); update the td's colSpan to 5 so the placeholder cell spans all columns and aligns with the header. Locate the conditional rendering with paginatedFolders.length === 0 and change colSpan from 4 to 5.docs/cli/remove-policies.mdx (1)
1-12:⚠️ Potential issue | 🟡 MinorPage header still says “Claude Code” only — out of sync with the rest of the doc.
The frontmatter description (line 3) and the intro line (line 10) both claim this command targets Claude Code's
settings.json, but the matrix and per-CLI sections below describe Cursor, Gemini, Copilot, Codex, OpenCode, and Pi as well. Update the description and intro so users browsing the docs index aren't misled.📝 Suggested copy
--- title: Uninstall policies -description: "Remove hook entries from Claude Code's settings" +description: "Remove FailproofAI hook entries from supported AI agent CLIs" --- ```bash failproofai policies --uninstall [policy-names...] [options]-Removes failproofai hook entries from Claude Code's
settings.json.
+Removes FailproofAI hook entries from the configured AI agent CLI(s). By default
+this targets Claude Code; use--clito scope removal to a specific integration
+(e.g.cursor,gemini,copilot,codex,opencode,pi).</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/cli/remove-policies.mdxaround lines 1 - 12, Update the frontmatter
description and the intro sentence in the "Uninstall policies" page so they no
longer mention only "Claude Code"; change the description and the first
paragraph to say the command removes FailproofAI hook entries from the
configured AI agent CLI(s) and that by default it targets Claude Code, with
guidance to use --cli to scope to a specific integration (examples: cursor,
gemini, copilot, codex, opencode, pi). Replace the current description string
and the intro line that references Claude Code's settings.json with the
suggested copy provided in the review (capitalized "FailproofAI", mention of
configured AI agent CLIs, default target, and the --cli usage and examples).</details> </blockquote></details> </blockquote></details>🟡 Minor comments (16)
src/codex/trace-parser.ts-31-37 (1)
31-37:⚠️ Potential issue | 🟡 MinorEdge case: single-character unquoted command is dropped.
The fallback
/\bcommand=([^\s].+)$/requires a non-whitespace char plus one-or-more additional chars, so a line ending incommand=q(or any 1-char unquoted command) won’t match and the line is silently skipped. Replacing[^\s].+with\S.*makes the second segment optional and also slightly more idiomatic.🩹 Proposed tweak
- line.match(/\bcommand=([^\s].+)$/); + line.match(/\bcommand=(\S.*)$/);Consider adding a unit test covering
command=ls(no quotes, short value) to lock this in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/codex/trace-parser.ts` around lines 31 - 37, The fallback regex in trace-parser.ts that builds quotedCommandMatch uses `\bcommand=([^\s].+)`, which fails for single-character unquoted values; update that alternative to use `\bcommand=(\S.*)$` (or `\bcommand=(\S.*)` if `$` not desired) so a single non-space char matches, and add a unit test exercising parse of lines like `... command=ls` and `... command=q` to lock the behavior; ensure this change is made where quotedCommandMatch is constructed and where its `[1]` group is read.components/integration-badge.tsx-28-28 (1)
28-28:⚠️ Potential issue | 🟡 Minor
text-[0.6rem]is below typical accessibility minimums.
0.6rem≈ 9.6px (assuming a 16px root). That's very small for an uppercase tracking-tight badge and likely fails practical readability for users who don't zoom. Tailwind'stext-xs(12px) is the conventional minimum for badges. Worth bumping unless you have explicit design constraints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/integration-badge.tsx` at line 28, The badge's font-size class `text-[0.6rem]` in the span (inside the IntegrationBadge component) is too small for accessibility; change it to a larger Tailwind size such as `text-xs` (or make it configurable via a prop) by updating the class string on the span element (`<span className={...}>`) to use `text-xs` instead of `text-[0.6rem]` and run a quick visual/accessibility check to ensure uppercase/tracking styling still reads well.src/hooks/hook-logger.ts-60-69 (1)
60-69:⚠️ Potential issue | 🟡 Minor
FAILPROOFAI_SUPPRESS_STDERR_FORsilently ignores most CLI IDs.The summary describes this env var as a "configurable comma-separated list... with wildcard
*and valuesopencode/pi", but the implementation hard-codes the only matchable values to literally"opencode"and"pi"(line 67). With this PR adding Cursor, Copilot, Codex, and Gemini integrations, a user settingFAILPROOFAI_SUPPRESS_STDERR_FOR=cursor,copilotwill get no suppression and no warning — a confusing footgun.Either:
- Drop the allowlist and accept any CLI id (recommended), so the env var is forward-compatible as new integrations land; or
- Validate input and log a warning for unknown values.
🔧 Proposed fix (option 1)
- // Suppress stderr for integrations that capture it (OpenCode, Pi, etc.) - // These integrations display stderr in the UI, so we only want the JSON response there. - // Detect by environment variables these integrations set. - const isOpenCode = !!process.env.OPENCODE_SESSION_ID || !!process.env.OPENCODE_WORKSPACE_ID; - const isPi = !!process.env.PI_SESSION_ID; - const suppressFor = (process.env.FAILPROOFAI_SUPPRESS_STDERR_FOR ?? "").toLowerCase().split(",").filter(Boolean); - - if (isOpenCode || isPi || suppressFor.includes("*") || suppressFor.some(s => s === "opencode" || s === "pi")) { - suppressStderr = true; - } + // Suppress stderr for integrations that capture it and surface it to the user. + // Auto-detect known captures via env vars; allow override via FAILPROOFAI_SUPPRESS_STDERR_FOR + // (comma-separated CLI ids, or '*' for all). + const isOpenCode = !!process.env.OPENCODE_SESSION_ID || !!process.env.OPENCODE_WORKSPACE_ID; + const isPi = !!process.env.PI_SESSION_ID; + const suppressFor = (process.env.FAILPROOFAI_SUPPRESS_STDERR_FOR ?? "") + .toLowerCase() + .split(",") + .map((s) => s.trim()) + .filter(Boolean); + const currentCli = (process.env.FAILPROOFAI_CLI ?? "").toLowerCase(); + + if ( + isOpenCode || + isPi || + suppressFor.includes("*") || + (currentCli && suppressFor.includes(currentCli)) + ) { + suppressStderr = true; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/hook-logger.ts` around lines 60 - 69, The current logic ignores most CLI IDs because it hard-codes allowed names; change it to accept any CLI id from FAILPROOFAI_SUPPRESS_STDERR_FOR so the setting is forward-compatible: compute a detectedIntegrationIds array (e.g., produce "opencode" when isOpenCode is true, "pi" when isPi is true, etc.), then set suppressStderr = true if suppressFor includes "*" or suppressFor.some(id => detectedIntegrationIds.includes(id)); keep the existing isOpenCode/isPi detection and the suppressFor parsing but remove the fixed string checks ("opencode" || "pi") so new integrations (cursor, copilot, etc.) specified in suppressFor will work without code changes.src/hooks/hooks-config.ts-11-14 (1)
11-14:⚠️ Potential issue | 🟡 Minor
getHomeDir()may diverge fromos.homedir()on Windows.Node's
os.homedir()uses libuv'sgetpwuid_ron POSIX andUSERPROFILEon Windows; it does not read theHOMEenv var directly. By preferringprocess.env.HOME, this helper:
- Lets E2E tests redirect to a tmp dir by setting
HOME(the likely intent).- On Windows, if a shell (Git Bash, MSYS) sets
HOMEto a path that differs fromUSERPROFILE, failproofai will now read/writepolicies-config.jsonat a different location than other native Windows tooling — a subtle source of "where did my config go?" reports.Consider scoping the override to test scenarios only, e.g. gate on an explicit
FAILPROOFAI_HOMEenv var, or document this Windows behavior change in the migration notes.♻️ Suggested alternative
function getHomeDir(): string { - const envHome = process.env.HOME; - return envHome && envHome.trim().length > 0 ? envHome : homedir(); + // FAILPROOFAI_HOME lets tests redirect config without affecting production HOME semantics. + const override = process.env.FAILPROOFAI_HOME; + if (override && override.trim().length > 0) return override; + return homedir(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/hooks-config.ts` around lines 11 - 14, The helper getHomeDir() currently prefers process.env.HOME over os.homedir(), which can diverge on Windows; change it so it defaults to homedir() and only uses an override env var (e.g. FAILPROOFAI_HOME) when explicitly set for tests or migration, i.e., update getHomeDir() to check process.env.FAILPROOFAI_HOME first, then fall back to homedir(), and remove implicit use of process.env.HOME so Windows behavior matches os.homedir().app/project/[name]/session/[sessionId]/page.tsx-21-28 (1)
21-28:⚠️ Potential issue | 🟡 MinorSource-detail truncation skips Windows-style paths.
After
replace(/\\/g, "/"), Windows absolute paths likeC:\Users\foo\...becomeC:/Users/foo/..., which does not satisfynormalized.startsWith("/"). Such paths are returned verbatim — no truncation, no leading-...indicator — so long Windows transcript paths render full-length in the UI. Consider also accepting paths matching/^[A-Za-z]:\//(or just always truncating when there are >4 segments after normalization), unless this code is intended for POSIX deployments only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/project/`[name]/session/[sessionId]/page.tsx around lines 21 - 28, The truncation logic in formatSourceDetailForDisplay currently only truncates POSIX absolute paths because it checks normalized.startsWith("/"); update it to also detect Windows absolute paths (e.g., /^[A-Za-z]:\// on the normalized value) or simply decide truncation based solely on parts.length after normalization; specifically, use the normalized variable to split into parts, and if parts.length > 4 return the shortened form (`.../${parts.slice(-4).join("/")}`) for both POSIX and Windows-drive paths (while still returning the original sourceDetail when it’s empty), ensuring you reference formatSourceDetailForDisplay, normalized, and parts when making the change.__tests__/e2e/hooks/policy-params.e2e.test.ts-336-339 (1)
336-339:⚠️ Potential issue | 🟡 MinorFragile assertion: bare
"42"substring may collide with unrelated stderr content.
result.stderrfor a denied PreToolUse can include durations, timestamps, paths, or numeric IDs that happen to contain42(e.g.42ms,1242, etc.), which would cause this test to falsely fail later. Consider asserting the absence of the hint-append shape rather than the bare number — for example,expect(reason).not.toMatch(/\.\s*42\b/)or check that stderr does not match the hint-append suffix used for valid hints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/hooks/policy-params.e2e.test.ts` around lines 336 - 339, The test currently uses a fragile substring check expect(reason).not.toContain("42") against result.stderr; replace it with a pattern that asserts the specific hint-append shape is absent (e.g., use result.stderr and expect(...).not.toMatch(/\.\s*42\b/) or an equivalent regex that matches a dot, optional whitespace, then 42 as a whole word) so unrelated occurrences of "42" (timestamps, durations) won't cause false failures; update the assertion referencing the variables reason or result.stderr in the test to use this regex-based negative match.src/hooks/loader-utils.ts-195-196 (1)
195-196:⚠️ Potential issue | 🟡 MinorTS temp filenames are not pid/UUID-disambiguated — concurrent transpiles race.
You hardened the shim path at line 85 with pid + UUID precisely to handle parallel hook subprocesses, but the temp source/output paths here use a fixed
sourcePath + ".__failproofai_ts_src__.ts"/".__failproofai_ts_out__.mjs"form. Two concurrent hook processes loading the same custom.tspolicy will collide: one writes/deletes while the other is still reading, producingENOENTfrom thereadFile(tmpOut)or stale content.♻️ Apply the same per-invocation suffix as the shim
- const tmpSrc = sourcePath + ".__failproofai_ts_src__.ts"; - const tmpOut = sourcePath + ".__failproofai_ts_out__.mjs"; + const id = `${process.pid}.${randomUUID()}`; + const tmpSrc = `${sourcePath}.__failproofai_ts_src__.${id}.ts`; + const tmpOut = `${sourcePath}.__failproofai_ts_out__.${id}.mjs`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/loader-utils.ts` around lines 195 - 196, The temp filenames tmpSrc and tmpOut are fixed and can collide across concurrent transpiles; change their construction to include a per-invocation disambiguator (e.g., process.pid + crypto.randomUUID() or Date.now()+Math.random()) like the shim path used elsewhere so each invocation writes/reads unique files; update all uses of tmpSrc and tmpOut (creation, write, transpile, read, and cleanup) to the new names and ensure cleanup still runs for the generated unique files.scripts/preuninstall.mjs-23-41 (1)
23-41:⚠️ Potential issue | 🟡 MinorRC cleanup regex misses CRLF endings and blank lines after the marker.
A few robustness gaps with the current pattern
/# failproofai copilot-sync\n[^\n]+\n?/g:
- CRLF line endings — on Windows shells (Git Bash, WSL with native RC, etc.) RC files may have
\r\n. The marker line will not be followed by\nalone, so the regex won't match and the bootstrap stays.- Blank line between marker and command —
[^\n]+requires ≥1 non-newline character on the next line. If install ever emits# failproofai copilot-sync\n\nsource …, only the marker stays as orphaned cruft.- No start-of-line anchor — extremely low risk, but anchoring with
^+mflag avoids matching the marker if it ever appears mid-line.♻️ Hardening
- const updated = content.replace(/# failproofai copilot-sync\n[^\n]+\n?/g, ""); + const updated = content.replace(/^# failproofai copilot-sync\r?\n(?:[^\r\n]*\r?\n)?/gm, "");The outer
try/catchalready keeps uninstall non-blocking, so this is a robustness improvement rather than a critical fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/preuninstall.mjs` around lines 23 - 41, The removeCopilotSyncFromRcFiles function’s replace regex should be hardened to match CRLF endings, allow optional blank lines between the marker and the command, and anchor the marker to start-of-line; update the replace that currently uses /# failproofai copilot-sync\n[^\n]+\n?/g to use a multiline, CRLF-aware pattern (for example use /^# failproofai copilot-sync\r?\n(?:\r?\n)*[^\r\n]+\r?\n?/gm) so the MARKER line and any blank lines plus the following command line are removed reliably across platforms.__tests__/hooks/policy-evaluator.test.ts-835-839 (1)
835-839:⚠️ Potential issue | 🟡 MinorClarify test intent:
piintentionally uses default style, not an unknown integration.The test correctly asserts
pi's intended formatting, but the description "unknown integration" and theas anycast obscure this intent. Line 35 of policy-evaluator.ts explicitly listspiamong integrations that intentionally use the default style. Either rename the test to clarify thatpideliberately uses the default (e.g., "uses default specialized style for Pi"), or split into two tests: one for an actual unknown integration (e.g.,"unknown-cli" as any) and a separate properly-typed test forpi.🤖 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 835 - 839, The test description is misleading: `pi` is intentionally listed to use the default style in policy-evaluator.ts, so either rename the test or split it into two; update the test that calls evaluatePolicies("PreToolUse", { tool_name: "Bash" }, { integration: "pi" }) to have a clear name like "uses default specialized style for Pi" and remove the `as any` cast (use the proper integration type), and optionally add a second test that passes a truly unknown integration (e.g., `"unknown-cli" as any`) asserting the same default-style behavior to cover the unknown-integration case; reference evaluatePolicies and the integration list in policy-evaluator.ts to ensure the intent matches the code.__tests__/e2e/hooks/codex-integration.e2e.test.ts-8-8 (1)
8-8:⚠️ Potential issue | 🟡 Minor
DEDUP_DIRis computed against the realhomedir()but tests overrideHOMEtoisoHome.
DEDUP_DIR = resolve(homedir(), ".failproofai", "cache", "dedup")resolves once at module load against the developer's real home directory. Yet every test overridesHOME: isoHome, so the failproofai binary will read/write its dedup cache underisoHome/.failproofai/cache/dedup— notDEDUP_DIR. This means:
- The
rmSync(DEDUP_DIR, ...)call on line 17 unnecessarily wipes the developer's real local dedup cache between every test run.- It is not actually clearing the cache the test process will use.
Consider deriving the dedup path from
isoHome(or simply skipping the rm sinceisoHomeis fresh per test):♻️ Proposed fix
-const DEDUP_DIR = resolve(homedir(), ".failproofai", "cache", "dedup"); @@ beforeEach(() => { projectDir = mkdtempSync(join(tmpdir(), "fp-e2e-codex-")); isoHome = mkdtempSync(join(tmpdir(), "fp-e2e-codex-home-")); - if (existsSync(DEDUP_DIR)) rmSync(DEDUP_DIR, { recursive: true, force: true }); mkdirSync(resolve(projectDir, ".codex"), { recursive: true });The same issue is repeated in
claude-code-integration.e2e.test.ts,gemini-integration.e2e.test.ts, andpi-integration.e2e.test.ts— please apply consistently.Also applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/hooks/codex-integration.e2e.test.ts` at line 8, DEDUP_DIR is computed once from homedir() at module load which points at the developer's real home, so tests that override HOME (isoHome) end up not clearing the actual test cache and instead wipe the developer's cache; fix by deriving the dedup path from the test-provided isoHome (or avoid rmSync entirely since isoHome is fresh) wherever DEDUP_DIR is used — update the module-level constant or replace usages in the e2e files (DEDUP_DIR, homedir(), rmSync calls) to compute resolve(isoHome, ".failproofai", "cache", "dedup") at runtime inside the test setup (or remove the rmSync if isoHome guarantees isolation) and apply the same change in claude-code-integration.e2e.test.ts, gemini-integration.e2e.test.ts, and pi-integration.e2e.test.ts.bin/failproofai.mjs-92-95 (1)
92-95:⚠️ Potential issue | 🟡 Minor
hardStopshould be used post-handler — currently ignored despite design intent.
handleHookEventreturns{ exitCode, hardStop }, butbin/failproofai.mjsdiscardshardStopwithvoid hardStop;. The policy-evaluator comment at line 100–102 states: "Logic moved to CLI wrapper to avoid piped hangs. evaluatePolicies now returns a hardStop flag instead of signaling directly." This is explicit evidence that the CLI wrapper is meant to handle the flag.
hardStopis computed for Cursor/Gemini integrations when session termination is needed (Cursor: on PostToolUse/Stop; Gemini: on Stop only), but the wrapper never acts on it. Either:
hardStopshould gate post-handler cleanup (e.g., forced terminal reset, suppressed logs, parent signal), in which case this is a missed wiring; orhardStopis informational and the caller doesn't need it — then remove from destructure to clarify intent.Confirm the intended post-handler behavior before proceeding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/failproofai.mjs` around lines 92 - 95, handleHookEvent returns { exitCode, hardStop } but the CLI currently discards hardStop using "void hardStop;", so either wire the flag into post-handler cleanup or remove it to clarify intent: if hardStop is meant to trigger a forced termination flow, replace the void with logic in the CLI wrapper (after the call to handleHookEvent) that performs the required hard-stop actions (e.g., forced terminal reset/cleanup, suppressing further logs, and sending the appropriate signal to parent/child processes) using the hardStop boolean; otherwise remove hardStop from the destructuring assignment and update handleHookEvent's contract/comments to reflect it is informational/unused. Ensure changes reference handleHookEvent, hardStop and exitCode and remove the no-op "void hardStop" line.src/hooks/policy-evaluator.ts-250-266 (1)
250-266:⚠️ Potential issue | 🟡 MinorStop + instruct branch ignores integration-specific stdout protocol.
For non-Claude integrations (cursor, gemini, copilot) on a
Stopevent withinstructdecisions, this branch emitsstdout = ""andexitCode = 2. Elsewhere in this file, deny handling (and even the "no policies" allow path) honors integration-specific JSON protocol viagetDenyStdout/getAllowStdout. The hard-stop is enforced via thehardStopflag, but the agent itself never receives the structured{ decision: "deny", continue: false, ... }(Gemini) or{ continue: false, permission: "deny", ... }(Cursor) JSON it normally expects on stop, so the user-facing message in those agents may degrade to a generic exit-2 failure rather than the intended instruct text.Consider routing the instruct message through the same
getDenyStdout(...)for these integrations onStop.🤖 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 250 - 266, The Stop branch currently returns stdout = "" and exitCode = 2 which skips integration-specific JSON output; change the Stop handling to produce the same structured stdout as other deny paths by calling getDenyStdout(...) (using session?.integration, policyNames[0], combined) instead of the empty string so non-Claude integrations (cursor, gemini, copilot) receive the expected JSON protocol while preserving exitCode = 2, policyName/decision/hardStop and other returned fields; update the Stop branch where eventType === "Stop" (and which currently calls getDenyMessage(...)) to use getDenyStdout for stdout and keep getDenyMessage for stderr if needed.__tests__/hooks/manager.test.ts-588-591 (1)
588-591:⚠️ Potential issue | 🟡 MinorLoose warning assertion —
msg.includes("Stop")will match unrelated text.Line 590 only checks that some warn message contains both
"Stop"and"require-commit-before-stop". Because"Stop"is a substring of unrelated banners (e.g.,"FailproofAI Security Stop","Stop hook installed"), and the policy name itself is likely echoed in routine install logs, this test can pass even if the unsupported‑stop warning is never emitted.The very next test (line 611) already uses the precise phrase
"does not support a Stop event"— match the same string here for symmetry.- const warnCalls = vi.mocked(console.warn).mock.calls.map((c) => String(c[0])); - expect(warnCalls.some((msg) => msg.includes("Stop") && msg.includes("require-commit-before-stop"))).toBe(true); + const warnCalls = vi.mocked(console.warn).mock.calls.map((c) => String(c[0])); + expect( + warnCalls.some( + (msg) => + msg.includes("does not support a Stop event") && + msg.includes("require-commit-before-stop"), + ), + ).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/manager.test.ts` around lines 588 - 591, The warning assertion is too loose: update the expectation that scans console.warn calls (the warnCalls derived from vi.mocked(console.warn).mock.calls) to look for the precise phrase "does not support a Stop event" instead of checking msg.includes("Stop"), and keep the existing check for "require-commit-before-stop" so the test asserts a message that contains both "does not support a Stop event" and "require-commit-before-stop".src/hooks/resolve-permission-mode.ts-87-93 (1)
87-93:⚠️ Potential issue | 🟡 Minor
parseCursorModereturns whatever follows--mode, including flags orundefined.Lines 90–91: when
--modeis present,argv[modeIdx + 1]is returned verbatim, without verifying that it is one of"plan" | "ask"(as the comment promises). If a user invokescursor --mode --some-other-flag(or--modeis the last token), this leaks"--some-other-flag"(orundefined) intopermissionMode, which is then persisted in activity entries and the dashboard Mode column.🛡️ Suggested fix
function parseCursorMode(argv: string[] | null): string | undefined { if (!argv) return undefined; if (argv.includes("--yolo") || argv.includes("--force")) return "yolo"; const modeIdx = argv.indexOf("--mode"); - if (modeIdx !== -1) return argv[modeIdx + 1]; // "plan" | "ask" + if (modeIdx !== -1) { + const val = argv[modeIdx + 1]; + if (val === "plan" || val === "ask") return val; + } return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/resolve-permission-mode.ts` around lines 87 - 93, parseCursorMode currently returns argv[modeIdx + 1] verbatim which can leak flags or undefined into permissionMode; change parseCursorMode to validate the token after "--mode": if it exists and equals "plan" or "ask" return that value, otherwise return undefined (treat missing token or a token that starts with "-" as invalid). Keep the existing short-circuit checks for "--yolo"/"--force" and the use of modeIdx/argv to locate the next token.__tests__/e2e/helpers/payloads.ts-118-127 (1)
118-127:⚠️ Potential issue | 🟡 Minor
CursorPayloads.preToolUse.writereturns anafterFileEditpayload — namespace/event mismatch.This factory lives under
preToolUsebut emitshook_event_name: "afterFileEdit", which is a post‑edit event. This contradicts the namespace semantics and thepreToolUse.bash()method in the same namespace (which correctly useshook_event_name: "preToolUse"). Either move the method to the existingpostToolUsenamespace or emit a pre-edit event name. The mismatch will cause incorrect test semantics when this helper is used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/helpers/payloads.ts` around lines 118 - 127, CursorPayloads.preToolUse.write is emitting a post-edit event (hook_event_name: "afterFileEdit") which conflicts with its namespace; either move the write factory into the postToolUse namespace (e.g., CursorPayloads.postToolUse.write) or change its hook_event_name to "preToolUse" so it matches the preToolUse namespace semantics and aligns with CursorPayloads.preToolUse.bash; update the hook_event_name and any tests that assume the event name accordingly to keep namespace/event consistent.CHANGELOG.md-6-30 (1)
6-30:⚠️ Potential issue | 🟡 MinorAppend
(#185)to each new Unreleased entry.The new bullets under
## Unreleaseddon't follow the requiredsingle line description ... (#PR)format.As per coding guidelines, "Every PR must include an update to
CHANGELOG.mdunder the## Unreleasedsection with appropriate subsections (Features, Fixes, Docs, Dependencies), formatted as a single line description followed by PR number."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 6 - 30, The Unreleased section entries under "## Unreleased" need to follow the single-line "description ... (`#PR`)" format and currently lack PR numbers; edit the bullets you added (the new Feature/Fixes/Docs lines) so each is a single-line entry ending with " (`#185`)", and ensure they remain under the correct subsections (Features, Fixes, Docs) and do not span multiple lines (merge wrapped lines into one sentence per bullet) so the changelog format is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
6fc53ad2-d93b-4038-9495-b6504fed181b📒 Files selected for processing (68)
.claude/settings.json.failproofai/policies-config.json.failproofai/policies/workflow-policies.mjs.gitignoreCHANGELOG.md__tests__/codex/trace-parser.test.ts__tests__/components/project-list.test.tsx__tests__/e2e/cli/custom-install-matrix.e2e.test.ts__tests__/e2e/cli/custom-install.e2e.test.ts__tests__/e2e/helpers/hook-runner.ts__tests__/e2e/helpers/integration-adapter.ts__tests__/e2e/helpers/payloads.ts__tests__/e2e/hooks/claude-code-integration.e2e.test.ts__tests__/e2e/hooks/codex-integration.e2e.test.ts__tests__/e2e/hooks/copilot-integration.e2e.test.ts__tests__/e2e/hooks/cursor-integration.e2e.test.ts__tests__/e2e/hooks/custom-hooks-allow-patterns.matrix.e2e.test.ts__tests__/e2e/hooks/custom-hooks.e2e.test.ts__tests__/e2e/hooks/gemini-integration.e2e.test.ts__tests__/e2e/hooks/opencode-integration.e2e.test.ts__tests__/e2e/hooks/pi-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/loader-utils.test.ts__tests__/hooks/manager.test.ts__tests__/hooks/policy-evaluator.test.ts__tests__/hooks/resolve-permission-mode.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.tsxdocs/built-in-policies.mdxdocs/cli/hook.mdxdocs/cli/install-policies.mdxdocs/cli/list-policies.mdxdocs/cli/remove-policies.mdxdocs/getting-started.mdxlib/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/hooks-config.tssrc/hooks/install-prompt.tssrc/hooks/integrations.tssrc/hooks/loader-utils.tssrc/hooks/manager.tssrc/hooks/policy-evaluator.tssrc/hooks/policy-registry.tssrc/hooks/resolve-permission-mode.tssrc/hooks/types.ts💤 Files with no reviewable changes (2)
- .claude/settings.json
- .failproofai/policies/workflow-policies.mjs
| // Secondary Detection: Payload-based (Stdin) | ||
| if (!integrationType) { | ||
| if (INTEGRATIONS.copilot.detect(parsed)) { | ||
| integrationType = "copilot"; | ||
| } else if (INTEGRATIONS.gemini.detect(parsed)) { | ||
| integrationType = "gemini"; | ||
| } else if (INTEGRATIONS.cursor.detect(parsed)) { | ||
| integrationType = "cursor"; | ||
| } else if (INTEGRATIONS.opencode.detect(parsed)) { | ||
| integrationType = "opencode"; | ||
| } else if (INTEGRATIONS.pi.detect(parsed)) { | ||
| integrationType = "pi"; | ||
| } | ||
|
|
||
| if (!integrationType) { | ||
| integrationType = "claude-code"; | ||
| } |
There was a problem hiding this comment.
Ambiguous no---cli payloads are biased to Copilot.
In the secondary-detection path you check INTEGRATIONS.copilot.detect(parsed) before INTEGRATIONS.cursor.detect(parsed), but both integrations accept shared camelCase events like preToolUse. That means a minimal Cursor payload can be mislabeled as Copilot, which then skews transcript lookup, permission-mode resolution, and dashboard attribution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/handler.ts` around lines 541 - 557, Secondary payload detection
biases Copilot because INTEGRATIONS.copilot.detect(parsed) is checked before
INTEGRATIONS.cursor.detect(parsed); update the detection logic in the handler
(the block that sets integrationType) to either (a) check
INTEGRATIONS.cursor.detect(parsed) before INTEGRATIONS.copilot.detect(parsed) or
(b) make Copilot detection stricter by requiring an additional Copilot-specific
field (e.g., vendor or product flag) so camelCase/shared events like preToolUse
don't misclassify Cursor. Modify the condition order or detection predicates for
INTEGRATIONS.copilot.detect and INTEGRATIONS.cursor.detect accordingly so
Cursor-specific payloads are matched first or Copilot requires the extra
discriminator.
| export function migrateIntegrationField(): void { | ||
| ensureDir(); | ||
| if (hasMigrationRun()) return; | ||
|
|
||
| try { | ||
| let migrated = 0; | ||
| const currentPath = join(storeDir, CURRENT_FILE); | ||
| const entries = readJsonlFile(currentPath); | ||
|
|
||
| if (entries.some((e) => !e.integration)) { | ||
| const updated = entries.map((e) => { | ||
| if (e.integration) return e; | ||
| // Infer from payload: check hook_event_name patterns for platform hints | ||
| let inferred = "claude-code"; // default fallback | ||
| if (e.hookEventName) { | ||
| const name = e.hookEventName; | ||
| if (name.startsWith("before") || name.startsWith("after")) { | ||
| if (!name.startsWith("Before") && !name.startsWith("After")) { | ||
| inferred = "cursor"; // camelCase before/after = Cursor | ||
| } | ||
| } | ||
| } | ||
| migrated++; | ||
| return { ...e, integration: inferred }; | ||
| }); | ||
| writeFileSync(currentPath, updated.map((e) => JSON.stringify(e)).join("\n") + "\n", "utf-8"); | ||
| } | ||
|
|
||
| // Migrate archived files | ||
| const archives = getArchiveFiles(); | ||
| for (const archiveName of archives) { | ||
| const archivePath = join(storeDir, archiveName); | ||
| const archivedEntries = readJsonlFile(archivePath); | ||
| if (archivedEntries.some((e) => !e.integration)) { | ||
| const updated = archivedEntries.map((e) => { | ||
| if (e.integration) return e; | ||
| let inferred = "claude-code"; | ||
| if (e.hookEventName) { | ||
| const name = e.hookEventName; | ||
| if (name.startsWith("before") || name.startsWith("after")) { | ||
| if (!name.startsWith("Before") && !name.startsWith("After")) { | ||
| inferred = "cursor"; | ||
| } | ||
| } | ||
| } | ||
| migrated++; | ||
| return { ...e, integration: inferred }; | ||
| }); | ||
| writeFileSync(archivePath, updated.map((e) => JSON.stringify(e)).join("\n") + "\n", "utf-8"); | ||
| } | ||
| } | ||
|
|
||
| markMigrationDone(); | ||
| } catch (e) { | ||
| // Non-fatal: migration failure doesn't block the system | ||
| } | ||
| } |
There was a problem hiding this comment.
Protect the migration with the same writer lock.
migrateIntegrationField() rewrites current.jsonl and archive pages without acquireLock(). If persistHookActivity() appends while the migration is rebuilding one of those files, the newer entry is lost on the final writeFileSync. This one-time migration needs the same lock, or an atomic temp-file/rename flow coordinated with writers.
🤖 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 - 559,
migrateIntegrationField() rewrites current.jsonl and archive pages without using
the writer lock, so acquire the same writer lock (call
acquireLock()/releaseLock() used by persistHookActivity()) at the start of
migrateIntegrationField() before reading/writing files and ensure release in a
finally block; while holding the lock perform the read/transform/write of
CURRENT_FILE and each archive file, then call markMigrationDone() and release
the lock. Alternatively, if you prefer lock-free safety, implement atomic writes
for CURRENT_FILE and each archive by writing to a temp file and fs.rename() into
place (or both: acquire lock and use temp+rename) to ensure no appends from
persistHookActivity() are lost.
| export function synchronizeCopilotProjectHooks(): void { | ||
| const globalSettingsPath = resolve(getCopilotHomeDir(), "config.json"); | ||
| if (!existsSync(globalSettingsPath)) return; | ||
|
|
||
| try { | ||
| // Search for project settings FIRST — if there's no project file to | ||
| // merge in, we must not touch existing user-scope hooks (wiping them | ||
| // would leave the user with no active hooks at all). | ||
| let currentDir = process.cwd(); | ||
| let projectSettingsPath: string | null = null; | ||
| const maxDepth = 10; | ||
| for (let i = 0; i < maxDepth; i++) { | ||
| const candidate = resolve(currentDir, ".github", "hooks", "failproofai.json"); | ||
| if (existsSync(candidate)) { | ||
| projectSettingsPath = candidate; | ||
| break; | ||
| } | ||
| const parent = resolve(currentDir, ".."); | ||
| if (parent === currentDir) break; | ||
| currentDir = parent; | ||
| } | ||
| if (!projectSettingsPath) return; // nothing to sync — leave user-scope hooks intact | ||
|
|
||
| const globalSettings = readJsonFile(globalSettingsPath); | ||
| if (!globalSettings.hooks) globalSettings.hooks = {}; | ||
| const gHooks = globalSettings.hooks as Record<string, any[]>; | ||
|
|
||
| // Only strip project-scope entries (npx invocations) so user-scope | ||
| // entries (local-binary invocations) survive re-syncs. | ||
| for (const event of Object.keys(gHooks)) { | ||
| gHooks[event] = gHooks[event].filter((h: any) => | ||
| !(typeof h.bash === "string" && h.bash.includes("npx -y failproofai") && h.bash.includes("--cli copilot")) | ||
| ); | ||
| } | ||
|
|
||
| { | ||
| const pSettings = readJsonFile(projectSettingsPath); | ||
| if (pSettings.hooks) { | ||
| const pHooks = pSettings.hooks as Record<string, any[]>; | ||
| for (const [event, entries] of Object.entries(pHooks)) { | ||
| if (!gHooks[event]) gHooks[event] = []; | ||
| for (const entry of entries) { | ||
| // Avoid duplicate commands | ||
| if (!gHooks[event].some((h: any) => h.bash === entry.bash)) { | ||
| gHooks[event].push(entry); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| writeJsonFile(globalSettingsPath, globalSettings); | ||
| } catch { | ||
| // Silenced — never break shell startup | ||
| } | ||
| } |
There was a problem hiding this comment.
Project-scope Copilot uninstall leaves the mirrored global hooks behind.
Project installs are copied into ~/.copilot/config.json by synchronizeCopilotProjectHooks(), but removeHooksFromFile() only edits the selected file. After uninstalling .github/hooks/failproofai.json, Copilot will still load the copied global hooks until another sync rewrites that config. Re-run the sync after project removals, or purge the mirrored --cli copilot entries from the global config here.
Also applies to: 973-989
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/integrations.ts` around lines 844 - 899,
synchronizeCopilotProjectHooks currently returns early when no project settings
exist, leaving previously mirrored project-scope hooks in the global config;
change it so that when projectSettingsPath is not found you still read
globalSettingsPath, iterate gHooks and purge any entries matching the
project-scope pattern (e.g., bash string contains "npx -y failproofai" and
"--cli copilot"), then writeJsonFile to persist the cleaned global config; apply
the same purge logic to removeHooksFromFile so uninstalling a project clears
mirrored --cli copilot entries from ~/.copilot/config.json as well.
| export async function maybeTranspileTypeScript(code: string, sourcePath: string): Promise<string> { | ||
| if (extname(sourcePath).toLowerCase() !== ".ts") return code; | ||
|
|
||
| // Write the TS source to a temp file so bun can read it | ||
| const tmpSrc = sourcePath + ".__failproofai_ts_src__.ts"; | ||
| const tmpOut = sourcePath + ".__failproofai_ts_out__.mjs"; | ||
| try { | ||
| await writeFile(tmpSrc, code, "utf-8"); | ||
| const result = spawnSync("bun", ["build", tmpSrc, "--target=node", "--format=esm", "--no-bundle", `--outfile=${tmpOut}`], { | ||
| encoding: "utf-8", | ||
| timeout: 15_000, | ||
| }); | ||
| if (result.status !== 0 || result.error) { | ||
| throw new Error(`TypeScript transpilation failed for ${sourcePath}: ${result.stderr || result.error?.message}`); | ||
| } | ||
| return await readFile(tmpOut, "utf-8"); | ||
| } finally { | ||
| try { await unlink(tmpSrc); } catch { /* ignore */ } | ||
| try { await unlink(tmpOut); } catch { /* ignore */ } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is bun guaranteed to be on PATH for end users who install failproofai via npm install -g failproofai (no separate bun install)?
💡 Result:
No, bun is not guaranteed to be on PATH for end users who install failproofai via npm install -g failproofai without a separate bun install.
Citations:
- 1: https://github.com/exospherehost/failproofai
- 2: https://registry.npmjs.org/graceful-fail
- 3: https://docs.befailproof.ai/testing
🏁 Script executed:
head -220 src/hooks/loader-utils.ts | tail -40Repository: exospherehost/failproofai
Length of output: 1536
🏁 Script executed:
sed -n '75,95p' src/hooks/loader-utils.tsRepository: exospherehost/failproofai
Length of output: 1030
🏁 Script executed:
sed -n '130,145p' src/hooks/loader-utils.tsRepository: exospherehost/failproofai
Length of output: 566
🏁 Script executed:
grep -n "maybeTranspileTypeScript" src/hooks/loader-utils.tsRepository: exospherehost/failproofai
Length of output: 236
🏁 Script executed:
rg "spawnSync.*bun" src/Repository: exospherehost/failproofai
Length of output: 223
🏁 Script executed:
rg "which.*bun|bun.*PATH|bun.*available" src/Repository: exospherehost/failproofai
Length of output: 160
Hard dependency on bun for TypeScript custom policies — no fallback, opaque error, and temp file races.
maybeTranspileTypeScript (line 127, called during custom hook loading) invokes spawnSync("bun", ...) unconditionally for any .ts source with no fallback mechanism.
Confirmed issues:
-
End users without bun will get opaque error: When bun is not on PATH,
spawnSyncfails with ENOENT. The error message becomes"TypeScript transpilation failed for <path>: undefined"becauseresult.stderris empty andresult.error?.messageis undefined in the spawn-failure path. The JSDoc claims "Throws if bun is not available" but the actual error message is not actionable. -
Not guaranteed for npm install users: Bun is not included as a dependency and is not on PATH for users who install via
npm install -g failproofaiwithout a separate bun installation. -
Temp file race on concurrent loads: Temp file paths (
sourcePath + ".__failproofai_ts_src__.ts"andsourcePath + ".__failproofai_ts_out__.mjs") use no pid/UUID suffix. When multiple hook subprocesses load the same.tscustom policy concurrently, they race on identical temp filenames. The ESM shim path (line 85) was hardened with${process.pid}.${randomUUID()}, but the TS temp paths were not. -
Blocking on every tool event:
spawnSyncblocks the hook subprocess. Each tool invocation (PreToolUse/PostToolUse) waits synchronously for the bun build to complete (15s timeout worst case), stalling tool calls for users with.tscustom policies.
Recommended fixes:
- Detect bun absence upfront and emit a clear, actionable error:
"TypeScript custom policies require bun on PATH; install from https://bun.sh". - Add pid/UUID suffix to TS temp files:
sourcePath + ".__failproofai_ts_src__." + process.pid + "." + randomUUID() + ".ts"(and similarly fortmpOut). - Consider async
spawnor caching transpiled output to avoid blocking every tool call. - Alternatively, provide a Node-native fallback (e.g., via a peer dependency on
typescript/esbuild/sucrase, or simple type stripping) so.tscustom policies remain loadable without bun.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/loader-utils.ts` around lines 191 - 211, maybeTranspileTypeScript
currently uses spawnSync("bun", ...) and fixed temp filenames (tmpSrc/tmpOut)
which causes opaque ENOENT errors when bun is missing, temp-file races, and
blocks the hook; update maybeTranspileTypeScript to (1) detect bun presence
upfront (e.g., try spawnSync("bun","--version") or fs.which equivalent) and
throw a clear, actionable error like "TypeScript custom policies require bun on
PATH; install from https://bun.sh" instead of the current vague message, (2)
make tmpSrc/tmpOut include process.pid and a random UUID (use process.pid +
randomUUID()) to avoid filename collisions, and (3) remove blocking behavior by
switching from spawnSync to an async spawn or implement caching of transpiled
output (or introduce a Node-native fallback using a bundler/transformer) so tool
hook invocations aren’t synchronously stalled by bun builds.
| // Clear policy config when removing from all scopes, or when no hooks remain in any scope | ||
| if (scope === "all") { | ||
| for (const s of integ.scopes) { | ||
| if (s === "repo") continue; | ||
| const existing = readScopedHooksConfig(s as HookScope, cwd); | ||
| if (existing.enabledPolicies.length > 0) { | ||
| writeScopedHooksConfig({ ...existing, enabledPolicies: [] }, s as HookScope, cwd); | ||
| } | ||
| } | ||
| } else if (!integ.scopes.some((s) => integ.hooksInstalledInSettings(s as any, cwd))) { | ||
| writeScopedHooksConfig({ ...configBeforeRemoval, enabledPolicies: [] }, configScope, cwd); | ||
| } |
There was a problem hiding this comment.
Don't clear the shared policy config until all integrations are gone.
This branch only checks whether the current integration still has hooks installed. In a multi-CLI setup, removing Claude hooks while Cursor/Copilot hooks remain will still zero enabledPolicies, and those remaining hooks will keep firing against an empty config. The cleanup needs to check hook presence across all installed integrations before clearing shared policy state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/manager.ts` around lines 433 - 444, The current removal logic
clears shared policy state when the current Integration (integ) has no hooks
installed, but it must first ensure no other integrations still have hooks;
change the check in the else branch to iterate over all Integration instances
and only clear enabledPolicies after confirming every integration's
hooksInstalledInSettings(...) returns false for the same scope(s). Specifically,
replace the integ-only check with a scan of the global/parent integrations
collection (e.g., integrations or installedIntegrations) calling
integ.hooksInstalledInSettings(s as any, cwd) for each integration and scope,
and only call writeScopedHooksConfig({ ...configBeforeRemoval, enabledPolicies:
[] }, configScope, cwd) when none of the integrations report hooks present; keep
the existing readScopedHooksConfig and writeScopedHooksConfig usage and the
scope === "all" branch unchanged.
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)
app/policies/hooks-client.tsx (1)
67-94:⚠️ Potential issue | 🔴 CriticalFix Windows path decoding in
decodeFolderName— encodeCwd and decodeFolderName are inconsistent.The SessionCell change to prefer
encodeCwd(cwd)exposes a critical bug: Windows paths don't round-trip correctly.
encodeCwd("C:/code/project")produces"C--code-project"✓- But
decodeFolderName("C--code-project")returns"C//code/project"✗ (should be"C:/code/project")This breaks the cwd comparison in
/project/[name]/page.tsx(line ~51:e.cwd === decodedName), causing virtual session matching to fail on Windows. UpdatedecodeFolderNameinlib/paths.tsto detect Windows drive letter patterns (e.g.,A-,Z-) and restore the colon:export function decodeFolderName(name: string): string { // Windows drive letter (e.g., "C--code-project" → "C:/code/project") if (/^[A-Za-z]-/.test(name)) { return name[0] + ":" + name.slice(2).replace(/-/g, "/"); } // Unix path (e.g., "-home-user-project" → "/home/user/project") if (name.startsWith("-")) { return "/" + name.slice(1).replace(/-/g, "/"); } return name.replace(/-/g, "/"); }🤖 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 67 - 94, SessionCell now uses encodeCwd(cwd) which encodes Windows drives like "C:/code" to "C--code-project", but decodeFolderName in lib/paths.ts doesn't reverse that correctly; update decodeFolderName to detect a Windows drive-letter pattern (e.g., /^[A-Za-z]-/) and restore the colon and slashes when decoding (so it returns "C:/..." for names starting with a drive-letter marker), while keeping existing logic for names that start with "-" (rooted Unix paths) and the default dash→slash replacement; ensure the function and encodeCwd remain consistent so comparisons in /project/[name]/page.ts (e.cwd === decodedName) work on Windows.
♻️ Duplicate comments (1)
app/policies/hooks-client.tsx (1)
265-265:⚠️ Potential issue | 🔴 Critical
DetailPanelcolSpanstill under-spans by one after Integration column.The header now renders 11
<th>elements (chevron, Decision, Event, Tool, Policy, Reason, Duration, Session, Mode, Integration, Time), butDetailPanelstill usescolSpan={10}. The expanded row will stop one column short, leaving the Time column visually unconstrained.🐛 Fix
return ( <tr> - <td colSpan={10} className="px-0 py-0"> + <td colSpan={11} className="px-0 py-0">Consider deriving
colSpanfrom a shared header-columns constant so future column additions don't silently drift again.🤖 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 265, DetailPanel's colSpan is hardcoded to 10 but the table header now renders 11 <th> (chevron, Decision, Event, Tool, Policy, Reason, Duration, Session, Mode, Integration, Time), causing the expanded row to under-span; change DetailPanel's colSpan to derive from the authoritative header definition (e.g., use headers.length or a shared HEADER_COLUMNS constant) instead of the magic number 10, and update the DetailPanel usage to colSpan={HEADER_COLUMNS.length} (or equivalent) so future header changes remain in sync.
🧹 Nitpick comments (2)
app/policies/hooks-client.tsx (2)
569-571: Avoidas any— typeitem.integrationagainst the badge's accepted union.
item.integrationis typed asstring | undefinedonHookActivityEntry, but it's being cast toanyto satisfyIntegrationBadge's prop type. This silently bypasses type checking — a stale/unknown integration value (e.g.,"claude_code"vs"claude-code") would compile but break rendering.Either widen
IntegrationBadge's prop to acceptstring | undefinedand handle unknowns inside, or narrowitem.integrationhere against the known set.♻️ Suggested narrowing
- <td className="px-3 py-2"> - <IntegrationBadge integration={item.integration as any} /> - </td> + <td className="px-3 py-2"> + <IntegrationBadge integration={item.integration} /> + </td>(Adjust
IntegrationBadge's prop type tostring | undefinedif needed so the cast can be removed cleanly.)🤖 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 569 - 571, item.integration is being cast to any to satisfy IntegrationBadge, which bypasses type checks; remove the cast and either (A) update IntegrationBadge's prop type to accept string | undefined and handle unknown values inside (e.g., show a fallback or sanitize known keys), or (B) perform a local type-narrowing before rendering (in the component that renders HookActivityEntry) by validating item.integration against the accepted union of integration keys and only pass a validated value (or undefined/fallback) to IntegrationBadge; reference IntegrationBadge and HookActivityEntry/item.integration when making the change.
428-441: Hardcoded integration list duplicates the source of truth.The
<option>list here,IntegrationBadge's accepted values, the server-side filter, and the install CLI list will all need to stay in sync. Consider exporting a singleINTEGRATIONSarray (id + display label) fromcomponents/integration-badge.tsx(or a sharedlib/integrations.ts) and mapping it here, so adding a new CLI doesn't silently miss this dropdown.🤖 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 428 - 441, The select's hardcoded <option> list (tied to filterIntegration and setFilterIntegration) duplicates the canonical integration definitions used by IntegrationBadge and server/CLI code; extract a single source of truth (e.g., export an INTEGRATIONS array of {id,label} from components/integration-badge.tsx or a new lib/integrations.ts) and replace the static options here by mapping INTEGRATIONS to <option value={id}>{label}</option>, ensuring IntegrationBadge, server filter, and CLI use the same exported INTEGRATIONS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@debug.mjs`:
- Around line 1-15: This file appears to be a local debug scratch and should be
removed from the PR (or moved to a git-ignored local scratch dir); if you must
keep it, replace the hardcoded absolute path in the payload by deriving
PROJECT_DIR via path.resolve(...) from the repo root, use the PROJECT_DIR
constant for both the exec cwd and the JSON payload's "cwd" (remove the unused
duplicate inline path), and ensure BINARY_PATH and env usage remain unchanged so
execSync still runs `bun ${BINARY_PATH}` with the same environment flags.
In `@debug2.mjs`:
- Around line 1-24: This file debug2.mjs is a local debug artifact with
hardcoded absolute paths (BINARY_PATH, PROJECT_DIR, FAILPROOFAI_DIST_PATH) and a
debug env flag (FAILPROOFAI_SKIP_KILL); delete it before merge or move it to a
non-committed location such as scripts/ or __tests__/manual/ and make it
portable by deriving paths from import.meta.url or process.cwd() (remove
user-specific hardcoded strings), stop setting FAILPROOFAI_SKIP_KILL, and ensure
env injection uses relative/derived paths so the script is runnable by any
contributor or CI.
---
Outside diff comments:
In `@app/policies/hooks-client.tsx`:
- Around line 67-94: SessionCell now uses encodeCwd(cwd) which encodes Windows
drives like "C:/code" to "C--code-project", but decodeFolderName in lib/paths.ts
doesn't reverse that correctly; update decodeFolderName to detect a Windows
drive-letter pattern (e.g., /^[A-Za-z]-/) and restore the colon and slashes when
decoding (so it returns "C:/..." for names starting with a drive-letter marker),
while keeping existing logic for names that start with "-" (rooted Unix paths)
and the default dash→slash replacement; ensure the function and encodeCwd remain
consistent so comparisons in /project/[name]/page.ts (e.cwd === decodedName)
work on Windows.
---
Duplicate comments:
In `@app/policies/hooks-client.tsx`:
- Line 265: DetailPanel's colSpan is hardcoded to 10 but the table header now
renders 11 <th> (chevron, Decision, Event, Tool, Policy, Reason, Duration,
Session, Mode, Integration, Time), causing the expanded row to under-span;
change DetailPanel's colSpan to derive from the authoritative header definition
(e.g., use headers.length or a shared HEADER_COLUMNS constant) instead of the
magic number 10, and update the DetailPanel usage to
colSpan={HEADER_COLUMNS.length} (or equivalent) so future header changes remain
in sync.
---
Nitpick comments:
In `@app/policies/hooks-client.tsx`:
- Around line 569-571: item.integration is being cast to any to satisfy
IntegrationBadge, which bypasses type checks; remove the cast and either (A)
update IntegrationBadge's prop type to accept string | undefined and handle
unknown values inside (e.g., show a fallback or sanitize known keys), or (B)
perform a local type-narrowing before rendering (in the component that renders
HookActivityEntry) by validating item.integration against the accepted union of
integration keys and only pass a validated value (or undefined/fallback) to
IntegrationBadge; reference IntegrationBadge and
HookActivityEntry/item.integration when making the change.
- Around line 428-441: The select's hardcoded <option> list (tied to
filterIntegration and setFilterIntegration) duplicates the canonical integration
definitions used by IntegrationBadge and server/CLI code; extract a single
source of truth (e.g., export an INTEGRATIONS array of {id,label} from
components/integration-badge.tsx or a new lib/integrations.ts) and replace the
static options here by mapping INTEGRATIONS to <option
value={id}>{label}</option>, ensuring IntegrationBadge, server filter, and CLI
use the same exported INTEGRATIONS.
🪄 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: bdec69f0-0754-4646-ae67-5a05fef8b7f0
📒 Files selected for processing (6)
__tests__/fixtures/gemini-project-debug/.failproofai/policies-config.json__tests__/fixtures/gemini-project-debug/.gemini/settings.jsonapp/policies/hooks-client.tsxdebug.mjsdebug2.mjssrc/hooks/resolve-permission-mode.ts
✅ Files skipped from review due to trivial changes (2)
- tests/fixtures/gemini-project-debug/.failproofai/policies-config.json
- tests/fixtures/gemini-project-debug/.gemini/settings.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/resolve-permission-mode.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
4-4:⚠️ Potential issue | 🟡 MinorStale
descriptionandkeywordsgiven multi-CLI scope.This PR adds first-class hook integration for Cursor, Copilot, Codex, OpenCode, Pi, and Gemini, but the package
descriptionstill markets only "Claude Code & the Agents SDK", andkeywordsmention onlyclaude/claude-code/claude-agents-sdk/anthropic. This hurts npm discoverability for users searching for the newly supported CLIs.📝 Suggested update
- "description": "The easiest way to manage policies that keep your AI agents reliable, on-task, and running autonomously — for Claude Code & the Agents SDK", + "description": "The easiest way to manage policies that keep your AI agents reliable, on-task, and running autonomously — for Claude Code, Cursor, Copilot, Codex, OpenCode, Pi, Gemini & the Agents SDK","keywords": [ "claude", "claude-code", "claude-agents-sdk", "anthropic", + "cursor", + "github-copilot", + "openai-codex", + "opencode", + "gemini-cli", "ai-agent",Also applies to: 42-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 4, Update the package.json metadata to reflect the new multi-CLI support: change the "description" string (currently mentioning only "Claude Code & the Agents SDK") to a broader description that lists or generically references the newly supported CLIs (Cursor, Copilot, Codex, OpenCode, Pi, Gemini) and update the "keywords" array to include relevant terms such as cursor, copilot, codex, opencode, pi, gemini, agents-sdk, and other applicable tags; locate and edit the "description" and "keywords" fields in package.json to ensure discoverability across the added CLIs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@package.json`:
- Line 4: Update the package.json metadata to reflect the new multi-CLI support:
change the "description" string (currently mentioning only "Claude Code & the
Agents SDK") to a broader description that lists or generically references the
newly supported CLIs (Cursor, Copilot, Codex, OpenCode, Pi, Gemini) and update
the "keywords" array to include relevant terms such as cursor, copilot, codex,
opencode, pi, gemini, agents-sdk, and other applicable tags; locate and edit the
"description" and "keywords" fields in package.json to ensure discoverability
across the added CLIs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35449e39-daea-44f4-bfb6-55423eae40b4
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
package.json
…board refinement - [CORE] Implement native Stop event mapping for OpenCode (session.idle) and Pi (stopReason). - [INSTALLER] Fix Stop support detection in manager.ts to correctly recognize session.idle and AfterAgent signals. - [DASHBOARD] Improve multi-CLI support in actions and UI, including per-CLI policy parameter overrides. - [CLEANUP] Remove debug-prompt diagnostic policy to ensure clean terminal output. - [TESTS] Update manager and e2e integration tests to reflect full Stop event support across all platforms. - [FIX] Resolve various dashboard state syncing issues with per-CLI policy toggles.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/hooks/builtin-policies.ts (2)
391-466:⚠️ Potential issue | 🟠 MajorStop-marker substring check creates a sanitization bypass.
Each PostToolUse sanitizer (
sanitizeJwt,sanitizeApiKeys,sanitizeConnectionStrings,sanitizePrivateKeyContent,sanitizeBearerTokens) early-returnsallow()whenever the tool output text contains the literal strings checked bylooksLikeFailproofStopMessage. A tool whose output contains an attacker- or LLM-controlled blob that includes either marker (MANDATORY ACTION REQUIRED from FailproofAI (policy:/[FailproofAI Security Stop] Policy:) will skip all secret redaction in the same payload, even when the rest of the output legitimately contains a JWT, API key, or PEM block.The strings are guessable and not session-bound, so they're trivially injectable. Consider one of:
- Anchor the check to the start of the output (or to a dedicated wrapper field) rather than
String#includes.- Mix in an unguessable per-process/session token into the marker before checking.
- Skip only the policy that produced the stop message (track which sanitizer is the source) rather than disabling all sanitizers.
Also applies to: 495-498
🤖 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 391 - 466, The current use of looksLikeFailproofStopMessage in sanitizeJwt, sanitizeApiKeys, sanitizeConnectionStrings, sanitizePrivateKeyContent, and sanitizeBearerTokens creates a global bypass because it returns allow() whenever the output contains the static marker; change this so a marker can't be trivially injected: either require the marker to be anchored at the start of the output (e.g., startsWith behavior) or validate it contains a per-session/per-process nonce before skipping sanitizers, or instead only skip the specific sanitizer that emitted the stop message (track the originating policy) rather than returning allow() for all sanitizers; update the checks in those functions and the looksLikeFailproofStopMessage implementation accordingly so secret-redaction runs unless the marker is proven authentic.
1456-1469:⚠️ Potential issue | 🟡 MinorUpdate the policy description to reflect its actual behavior.
The
requireCiGreenBeforeStoppolicy description (line 1809) reads "Require CI checks to pass on the current branch before Claude stops", but the implementation returnsallow()with warning messages for both failing and pending CI checks, rather than blocking. Update the description to "Warn if CI checks are failing or pending on the current branch" (or similar) so users understand that enabling this policy allows continuation with warnings, not enforcement.🤖 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 1456 - 1469, The policy description for requireCiGreenBeforeStop is misleading: update its human-readable description string used in the policy registration/definition (the text that currently reads "Require CI checks to pass on the current branch before Claude stops") to clearly state that it only warns and does not block, e.g. "Warn if CI checks are failing or pending on the current branch" so the behavior matches the implementation which returns allow() with warning messages for failing or pending checks.app/policies/hooks-client.tsx (1)
417-429:⚠️ Potential issue | 🟡 MinorAdd
Stopto the activity event filter.This dropdown still omits
Stop, so the new Stop events added in this PR can be recorded but not filtered from the dashboard.🤖 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 417 - 429, The activity event filter dropdown is missing the "Stop" option so Stop events can't be filtered; update the select tied to filterEventType (the element using value={filterEventType} and onChange={(e) => setFilterEventType(e.target.value)}) to include an additional option with value "Stop" (display text "Stop") alongside the other <option> entries so Stop events are selectable in the dashboard filter.
♻️ Duplicate comments (7)
src/hooks/handler.ts (3)
541-558:⚠️ Potential issue | 🟠 MajorSecondary detection still biases ambiguous payloads toward Copilot.
INTEGRATIONS.copilot.detect(parsed)is checked beforeINTEGRATIONS.cursor.detect(parsed). Both integrations accept shared camelCase events likepreToolUse, so a minimal Cursor payload that doesn't carry a Cursor-specific discriminator gets labeled as Copilot. That misattribution propagates through transcript lookup, permission-mode resolution, and dashboard attribution. Either reorder Cursor before Copilot, or tightencopilot.detectto require a Copilot-specific field (e.g., a vendor/product flag) before the heuristic claims the payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 541 - 558, The secondary payload-based integration detection biases ambiguous inputs toward Copilot because INTEGRATIONS.copilot.detect(parsed) runs before INTEGRATIONS.cursor.detect(parsed); update the logic in the integration detection block that sets integrationType (the if (!integrationType) branch) so that either INTEGRATIONS.cursor.detect(parsed) is checked before INTEGRATIONS.copilot.detect(parsed) or, better, tighten INTEGRATIONS.copilot.detect to require a Copilot-specific discriminator (e.g., vendor/product flag) on parsed before returning true; modify the copilot.detect implementation accordingly (or swap the detect call order) to prevent minimal Cursor payloads with shared camelCase events like preToolUse from being misattributed.
488-498:⚠️ Potential issue | 🟠 Major500ms stdin wall-clock fallback can still discard real hook payloads.
This resolves to
""on a wall-clock timer rather than on stream state. If a hook producer is slow to start writing (cold-start agents, slow IPC), the handler will parse an empty payload, lose session/tool metadata, and potentially miss policy decisions that should have fired. Drive resolution from'end'(and useprocess.stdin.isTTYto short-circuit the no-pipe case) instead of asetTimeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 488 - 498, The 500ms timeout can drop real stdin payloads; change the logic in the stdin-read block (the code that collects chunks and uses setTimeout resolving to "" and references chunks and rawEventType) to instead short-circuit immediately when process.stdin.isTTY is true, and otherwise resolve the promise only on the stream 'end' event (or when process.stdin.readableEnded is true), reject on 'error', and remove the setTimeout. Also ensure you attach an 'end' listener that concatenates the collected chunks and resolves with the full payload, and clean up listeners after resolve/reject to avoid leaks; keep the existing hookLogWarn(rawEventType) catch behavior.
350-356:⚠️ Potential issue | 🔴 CriticalFiring-lock still fails closed on every non-
EEXISTerror.If
mkdirSyncfailed earlier (no recovery — line 331 swallows it), or thededup/firing-locksdirectory is unwritable (perms /ENOSPC/ read-only volume),writeFileSyncwill throw a non-EEXISTerror and this branch returnsfalse. The handler at line 632 will then take the "instant-catch twin" path and silently skip policy evaluation for every event with a real session id — turning a deploy-time misconfiguration into a complete policy bypass.🛡️ Proposed fix to fail open on non-collision errors
try { writeFileSync(lockPath, String(process.pid), { flag: "wx" }); return true; } catch (e) { if ((e as any).code === "EEXIST") return false; - return false; + // FS error other than collision — fail open so we don't silently disable policies. + 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 350 - 356, The current writeFileSync catch always returns false on any error, causing the firing-lock to fail closed; change the catch in the locking code that uses writeFileSync(lockPath, ...) so that only EEXIST returns false (indicating the lock is held), while all other errors should be treated as non-collision (fail-open): log the error (including the error object) and return true so policy evaluation is not skipped. Update the catch to check (e as any).code === "EEXIST" → return false; otherwise processLogger.warn/error with the error and return true; also ensure any earlier mkdirSync errors are surfaced/logged similarly so failures to create the dedup/firing-locks directory don’t silently cause policy bypass.src/hooks/builtin-policies.ts (1)
1374-1404:⚠️ Potential issue | 🟠 Major
sessionHadToolUse()still only understands Claude transcripts.This helper scans for Claude-style
message.content[].type === "tool_use"blocks only. Cursor, Copilot, Codex, Gemini, Pi, and OpenCode transcripts use different shapes, sorequire-ci-green-before-stopwill incorrectly conclude "no tool use in this session, skipping CI check" for those integrations after real tool usage — defeating the policy on every non-Claude CLI. Either reuse the native transcript parsers added in this PR or scope this fast-path to Claude only and fail-open (return true) for other integrations.🤖 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 1374 - 1404, The sessionHadToolUse helper is Claude-specific and can mis-detect tool usage for other transcript formats; update sessionHadToolUse(ctx: PolicyContext) to either (A) call the repository's canonical transcript parsing utilities (use the shared/native transcript parsers added in this PR) to detect any tool-use blocks across supported providers instead of ad-hoc JSON parsing of ctx.session?.transcriptPath, or (B) if you don't want to depend on those parsers here, narrow the current fast-path to Claude only by detecting Claude transcripts (e.g., check an identifying field on each parsed entry such as a provider/source tag or a message.metadata/provider) and return true (fail-open) for any non-Claude transcript so requireCiGreenBeforeStop keeps enforcing CI for other integrations; make the change inside sessionHadToolUse and keep requireCiGreenBeforeStop calling it unchanged.app/policies/hooks-client.tsx (1)
267-267:⚠️ Potential issue | 🟡 MinorKeep
DetailPanelcolSpanin sync with the new Integration column.The table header now renders 11 columns, but the expanded row still spans 10. The detail panel will stop short of the final column and render misaligned.
Suggested fix
- <td colSpan={10} className="px-0 py-0"> + <td colSpan={11} className="px-0 py-0">Also applies to: 493-504
🤖 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 267, The DetailPanel expanded-row cell currently uses colSpan={10} but the table header now renders 11 columns; update the DetailPanel colSpan to match the new column count. Locate the expanded row <td colSpan={10}> instances in app/policies/hooks-client.tsx (the DetailPanel/expanded row render logic) and change the colSpan prop from 10 to 11 in both spots so the detail panel aligns with the new Integration column.src/hooks/integrations.ts (1)
844-895:⚠️ Potential issue | 🟠 MajorProject-scope Copilot uninstall still leaves mirrored global hooks behind.
synchronizeCopilotProjectHooks()returns early once.github/hooks/failproofai.jsonis gone, so previously mirrorednpx -y failproofai --cli copilotentries in~/.copilot/config.jsonare never purged.removeHooksFromFile()only edits the targeted file as well, so uninstalling the project hook can leave Copilot executing the stale global copy indefinitely.Also applies to: 973-989
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/integrations.ts` around lines 844 - 895, synchronizeCopilotProjectHooks currently returns early when projectSettingsPath is missing, leaving previously mirrored "npx -y failproofai --cli copilot" entries in the global config; change the function so that when no projectSettingsPath is found it still reads globalSettingsPath, applies the same filter that removes project-scope npx entries from gHooks (the same filter used later: check h.bash includes "npx -y failproofai" and "--cli copilot"), and writes the cleaned globalSettings back with writeJsonFile; keep existing behavior when projectSettingsPath exists (merge project hooks into gHooks) and reuse readJsonFile, gHooks, and writeJsonFile to avoid duplicating logic.src/hooks/manager.ts (1)
519-530:⚠️ Potential issue | 🟠 MajorStill clears shared
enabledPolicieswhile other integrations have hooks installed.The
else ifat line 528 only inspectsinteg(the integration currently being removed) —integ.scopes.some(s => integ.hooksInstalledInSettings(s, cwd)). In a multi-CLI setup, removing Claude hooks while Cursor/Copilot/Gemini/etc. hooks remain installed will still write{ enabledPolicies: [], cli: undefined }into the scoped config, after which the surviving hooks fire against an empty policy set (no policies will match). The check needs to walk all known integrations before zeroing the shared policy state.🛡️ Suggested fix — verify across all integrations before clearing
- } else if (!integ.scopes.some((s) => integ.hooksInstalledInSettings(s as any, cwd))) { - writeScopedHooksConfig({ ...configBeforeRemoval, enabledPolicies: [], cli: undefined }, configScope, cwd); - } + } else { + const { INTEGRATION_TYPES } = await import("./types"); + const anyHooksAnywhere = INTEGRATION_TYPES.some((id) => { + const otherInteg = getIntegration(id); + return otherInteg.scopes.some((s) => otherInteg.hooksInstalledInSettings(s as any, cwd)); + }); + if (!anyHooksAnywhere) { + writeScopedHooksConfig({ ...configBeforeRemoval, enabledPolicies: [], cli: undefined }, configScope, cwd); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/manager.ts` around lines 519 - 530, The else-if clears shared enabledPolicies by only checking the current integration (integ); change this to verify that no integration at all has hooks installed in the target scope before wiping policies. Replace the integ.scopes.some(...) check with a loop or any() over all integrations (e.g., integrations or allIntegrations) that calls eachIntegration.hooksInstalledInSettings(s as any, cwd) for the given scope(s), and only call writeScopedHooksConfig({ ...configBeforeRemoval, enabledPolicies: [], cli: undefined }, configScope, cwd) when none of the integrations report hooks installed for that scope; keep the rest of the block (configBeforeRemoval, configScope, cwd, writeScopedHooksConfig) unchanged.
🧹 Nitpick comments (10)
__tests__/e2e/cli/stop-event.e2e.test.ts (2)
1-58: File placement violates the e2e test location guideline.This file lives under
__tests__/e2e/cli/, but the project standard is__tests__/e2e/hooks/for hook-related e2e tests (other Stop/integration suites such as__tests__/e2e/hooks/opencode-integration.e2e.test.tsalready live there). Move the file to keep test discovery and ownership consistent.As per coding guidelines: "Always add unit tests in
__tests__/for new behaviour … e2e tests in__tests__/e2e/hooks/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/cli/stop-event.e2e.test.ts` around lines 1 - 58, The test file is placed under __tests__/e2e/cli/ but per project conventions all hook-related e2e tests must live under __tests__/e2e/hooks/; move this file into __tests__/e2e/hooks/ (preserving filename stop-event.e2e.test.ts), update any CI/test globs if needed, and ensure imports/relative paths still resolve (verify TMP_DIR, test descriptions "E2E: Stop Event Support (OpenCode & Pi)", and the two it() cases remain unchanged) so test discovery and ownership match other hook tests like opencode-integration.e2e.test.ts.
2-6: Unused imports and incomplete payload shape.
vi,writeFileSync,readFileSync,join, andhomedirare imported but never used.- The piped JSON omits
hook_event_name,transcript_path, andpermission_modethat the hook handler expects (seePayloads.stopin__tests__/e2e/helpers/payloads.ts). Even if the integration-specific mapping derives the event from--hook, missingtranscript_pathcan change which policies fire (e.g., transcript-reading policies skip on/dev/null), making behavior depend on the absence of fields rather than on the integration mapping under test.Prefer reusing
Payloads.stop(TMP_DIR)and extending it with the integration-specific keys (integration, integration-native session id), and drop the unused imports.Also applies to: 23-27, 44-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/cli/stop-event.e2e.test.ts` around lines 2 - 6, Remove the unused imports (vi, writeFileSync, readFileSync, join, homedir) from the test header and replace the handcrafted piped JSON with a payload built from Payloads.stop(TMP_DIR) extended with the integration-specific keys (set "integration" and the integration-native session id), ensuring you include hook_event_name, transcript_path, and permission_mode so the hook handler sees the full shape; update the test to reuse Payloads.stop(TMP_DIR) and merge in the integration mapping rather than omitting those fields.__tests__/hooks/hooks-config.test.ts (1)
78-85:mockFileshelper is duplicated across three describe blocks.The same 8-line helper appears at lines 78–85, 294–301, and 443–450. Hoist it to module scope once (it doesn't capture any block-local state) to keep these test groups in sync. Bonus: add one test for the project non-CLI vs global CLI
policyParamsprecedence to lock in the cross-scope behavior currently only implicitly defined.♻️ Proposed extraction
// `@vitest-environment` node import { describe, it, expect, vi, beforeEach } from "vitest"; import { readFileSync, writeFileSync, existsSync, mkdirSync } from "node:fs"; import { resolve } from "node:path"; import { homedir } from "node:os"; vi.mock("node:fs", () => ({ readFileSync: vi.fn(), writeFileSync: vi.fn(), existsSync: vi.fn(), mkdirSync: vi.fn(), })); const CONFIG_PATH = resolve(homedir(), ".failproofai", "policies-config.json"); +function mockFiles(files: Record<string, object>): void { + vi.mocked(existsSync).mockImplementation((p) => String(p) in files); + vi.mocked(readFileSync).mockImplementation((p) => { + const key = String(p); + if (key in files) return JSON.stringify(files[key]); + throw new Error("ENOENT"); + }); +}Then remove the three duplicate inner declarations.
Also applies to: 294-301, 443-450
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/hooks-config.test.ts` around lines 78 - 85, The helper function mockFiles is duplicated inside three describe blocks; move the function mockFiles(files: Record<string, object>): void to module scope (top of __tests__/hooks/hooks-config.test.ts) so all describes reuse it, then delete the three inner duplicate declarations; also add a small test that asserts project-level policyParams override global CLI policyParams (or vice-versa as the intended precedence) to lock the cross-scope behavior—use the same mockFiles helper and reference it inside that new test to verify precedence.src/hooks/handler.ts (2)
304-314: Replacerequire("node:fs")with the existing static import.
fsis already imported at line 29 (existsSync, readdirSync, mkdirSync, writeFileSync, readFileSync, appendFileSync). Mixing a dynamicrequire("node:fs")into an otherwise-ESM module is a code smell and breaks in pure-ESM bundles whererequireisn't defined.♻️ Proposed change
-import { existsSync, readdirSync, mkdirSync, writeFileSync, readFileSync, appendFileSync } from "fs"; +import { existsSync, readdirSync, mkdirSync, writeFileSync, readFileSync, appendFileSync, rmSync } from "fs"; @@ export function _resetDedupeCache(): void { try { const dedupDir = getDedupDir(); if (existsSync(dedupDir)) { - require("node:fs").rmSync(dedupDir, { recursive: true, force: true }); + rmSync(dedupDir, { 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 304 - 314, The _resetDedupeCache function uses require("node:fs").rmSync which mixes CommonJS require into an ESM module; update the static import list at the top to include rmSync (alongside existsSync, readdirSync, mkdirSync, writeFileSync, readFileSync, appendFileSync) and replace the require call inside _resetDedupeCache with a direct call to rmSync(dedupDir, { recursive: true, force: true }) so the function consistently uses the statically imported fs method.
519-558: Detection precedence chain is non-trivial — consider a small comment block summarizing it.The integration resolution now has four layers (CLI override → unique-event-name heuristics → payload detect → default), with subtle interaction with the silence guards above. Future maintainers will benefit from a one-line summary of the priority order at the top of section "2. Integration Detection & Normalization" (e.g., the bullet form already in the PR description). No change requested if you're confident the surrounding inline comments suffice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 519 - 558, Add a concise one-line summary comment above the "2. Integration Detection & Normalization" block that documents the precedence used to resolve integrationType (explicit CLI flag / normalizedCli or cliOverride → unique-event-name heuristics via isGeminiUniqueEvent/isCopilotUniqueEvent/CODEX_HOOK_EVENT_TYPES → payload-based detection via INTEGRATIONS.*.detect(parsed) → default "claude-code"), and mention that silence guards above can short-circuit this flow; reference the key symbols integrationType, normalizedCli, cliOverride, rawEventType, parsed, isGeminiUniqueEvent, isCopilotUniqueEvent, CODEX_HOOK_EVENT_TYPES, and INTEGRATIONS to make the priority clear for future maintainers.__tests__/actions/update-cli-policy.test.ts (1)
47-63: Add coverage forinheritcleanup edge cases and cross-CLI preservation.The single
inherittest only covers the path where the entry becomes fully empty after removal. Two cases worth pinning:
inheritwhenpolicyParamsfor the target policy still contains entries — thecli[cliName]entry should be retained (only the lists should be cleaned), otherwise per-CLI param overrides would be silently dropped.inherit/enable/disablefor one CLI when the config already has an unrelated CLI entry (e.g.,cursor) — that other entry must be preserved verbatim.Additionally, the
as anycast on line 55 papers over a real type that should be importable from@/src/hooks/policy-types(HooksConfig); typing the mock return value will catch shape regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/actions/update-cli-policy.test.ts` around lines 47 - 63, Add two new tests for toggleCliPolicyAction to cover inherit edge cases and preserve unrelated CLI entries: (1) test that calling toggleCliPolicyAction("claude-code","test-policy","inherit") retains the cli["claude-code"] object when cli["claude-code"].policyParams still has entries (only enabledPolicies/disabledPolicies are cleaned), and (2) test that toggling a policy for one CLI does not remove or alter an unrelated cli entry (e.g., "cursor") — assert writeHooksConfig receives the full config with the other CLI preserved. Update the mocked readHooksConfig return types to use the actual HooksConfig type from "@/src/hooks/policy-types" instead of using "as any" so test typings catch shape regressions; keep using readHooksConfig and writeHooksConfig mocks and reference toggleCliPolicyAction, cli[cliName], and policyParams in assertions.app/actions/get-hooks-config.ts (1)
106-131: LGTM.Mapping
INTEGRATIONSviadetectInstalled()and exposingcliOverrideskeyed by integration id with normalized empty defaults gives the dashboard a stable shape to render against. One small nit you may want to consider: the loop at lines 113–121 will pass through any string key the user has underconfig.cli, including typos (e.g."cursor-code"); if the dashboard UI iteratescliOverridesdirectly, those phantom keys will appear. Filtering to knownINTEGRATION_TYPESwould prevent that without breaking valid configs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/actions/get-hooks-config.ts` around lines 106 - 131, The loop that builds cliOverrides from rawCli currently copies any string key users place under config.cli (via rawCli and cliOverrides), which can introduce phantom entries; restrict this by only accepting keys that match your known integration list (e.g., INTEGRATION_TYPES or the ids returned from detectInstalledIntegrations()). Update the for (const [id, override] of Object.entries(rawCli)) loop to check that id is in the allowed integrations set before assigning to cliOverrides (use the integration id list from detectInstalledIntegrations() or INTEGRATION_TYPES), leaving the rest ignored so typos like "cursor-code" are not surfaced.src/hooks/hooks-config.ts (1)
27-90: Verify documented precedence vs. implementation forpolicyParamswhen CLI-level and non-CLI-level scopes collide.The doc at lines 34–37 states "first scope wins; CLI-level overrides global for the same key", but Step 2 (lines 62–78) makes any CLI-level scope win over any non-CLI scope. Concretely, if
project.policyParams["X"]exists andglobal_.cli[cliType].policyParams["X"]also exists, the global CLI value will win even though project non-CLI is a "higher" scope per the merge order.If that is intentional ("CLI override beats whatever non-CLI merge produced"), consider tightening the docstring to say so explicitly and adding a test case that pins this precedence so a future refactor doesn't silently invert it. The existing tests in
__tests__/hooks/hooks-config.test.tsonly cover (a) CLI vs non-CLI in the same scope and (b) project CLI vs global CLI — they don't cover the cross-scope CLI vs non-CLI collision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/hooks-config.ts` around lines 27 - 90, The implementation of policyParams in readMergedHooksConfig gives CLI-level policyParams precedence across scopes over non-CLI policyParams (mergedParams logic), which contradicts the docstring; either make behavior match the docstring (ensure non-CLI higher-scope policyParams like project.policyParams win over lower-scope CLI overrides) or update the docstring to state "CLI-level overrides global and lower scopes even over higher-scope non-CLI entries"; to fix: decide desired precedence, then (a) if changing code, adjust the mergedParams construction in readMergedHooksConfig to iterate scopes in true priority order (project/local/global) for CLI and non-CLI separately so per-scope non-CLI entries win before accepting CLI fallbacks, or (b) if keeping current behavior, update the function docstring lines describing policyParams to explicitly state that CLI-level entries win across scopes; finally add a unit test in __tests__/hooks/hooks-config.test.ts that asserts the chosen precedence for the collision case (e.g., project.policyParams["X"] vs global_.cli[cliType].policyParams["X"]) to lock in the behavior.__tests__/hooks/handler.test.ts (1)
137-149: Test title and assertion contradict each other.The title says "passes undefined cliType … when no integrationType arg given", but the assertion is
expect(calls[0][1]).toBeDefined()and the inline comment states the value falls back to"claude-code". Either the title is wrong (the handler does default to a defined value) or the assertion is wrong. Pick one; otherwise this test will mislead anyone debugging cliType propagation.♻️ Suggested rename + tighter assertion
- it("passes undefined cliType to readMergedHooksConfig when no integrationType arg given", async () => { + it("defaults cliType to 'claude-code' when no integrationType arg is given", async () => { mockStdin(JSON.stringify({})); const { readMergedHooksConfig } = await import("../../src/hooks/hooks-config"); await handleHookEvent("PreToolUse"); const calls = vi.mocked(readMergedHooksConfig).mock.calls; expect(calls.length).toBeGreaterThan(0); - // No explicit integrationArg → falls back to "claude-code" (the default in handler) - // The cliType is session.integration which is set from the arg or payload - // When neither arg nor payload has integration, defaults to "claude-code" - expect(calls[0][1]).toBeDefined(); + expect(calls[0][1]).toBe("claude-code"); });🤖 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 137 - 149, The test title contradicts the assertion; update the test for handleHookEvent to reflect that cliType is defined and defaults to "claude-code": rename the it(...) description to something like "passes default cliType 'claude-code' to readMergedHooksConfig when no integrationType arg given" and replace the loose expect(calls[0][1]).toBeDefined() with a precise assertion expect(calls[0][1]).toBe("claude-code"), referencing readMergedHooksConfig and handleHookEvent to make the intent explicit.src/hooks/manager.ts (1)
296-312: Inconsistent post-install messaging betweenclaude-codeandcopilotproject installs.Both
claude-codeandcopilotare flagged as "portable project installs" (line 303-304), but onlyclaude-codeprints aCommand: npx -y failproofailine (line 305-307). For Copilot the user sees"This file can be committed to git — no machine-specific paths."without ever being told what command was wired in. Either show aCommand:line for both portable installs (with the appropriate launcher) or drop it for both for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/manager.ts` around lines 296 - 312, The post-install messages special-case `claude-code` for printing the `Command: npx -y failproofai` line but treats `copilot` as portable without showing the command; update the conditional in src/hooks/manager.ts to use the `isPortableProjectInstall` flag instead of `integId === "claude-code"` and print a consistent `Command:` line for portable installs (using the same launcher or a small integId->launcher mapping if needed), then print the "This file can be committed..." message; otherwise keep printing `Binary: ${binaryPath}`. This affects the block referencing `isPortableProjectInstall`, `integId`, `scope`, `binaryPath`, `settingsPath`, and `integ.displayName`.
🤖 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/cli/stop-event.e2e.test.ts`:
- Around line 29-37: The test currently accepts any outcome by checking for
"ALLOW" in success and a lax regex in the catch; replace this with deterministic
assertions using the existing helpers: call Payloads.stop to build the payload,
run the hook runner, then use assertAllow from
__tests__/e2e/helpers/hook-runner.ts when you expect a 0 exit (ALLOW) or
assertStopInstruct when you expect a stop (non-zero exit with standardized
stderr prefix and exit code 2). Update both the try/catch block and the
alternate case (lines 50-56) to use these helpers so the test pins exit code,
empty stdout, and the required stderr prefix instead of matching a loose regex.
- Line 21: Replace the incorrect flag "--integration" with the correct renamed
flag "--cli" where the command string is constructed (the cmd variable building
`node ${binaryPath} ... --integration ... --stdin` and the second similar
construction later in the same test); update both occurrences so they read
`--cli opencode` (and `--cli` for any other hook entry), leaving binaryPath,
event types (session.idle and stop) and --stdin unchanged so flag parsing
succeeds.
In `@__tests__/hooks/handler.test.ts`:
- Around line 1353-1378: Update the two test titles so they match the asserted
behavior: change the it(...) descriptions for the "opencode" and "pi" cases to
state that permissionMode defaults to "default" (e.g., "opencode: permissionMode
defaults to 'default' (no mode concept)" and similarly for "pi"), keeping the
test body that calls handleHookEvent and expects persistHookActivity to be
called with permissionMode: "default"; references: the it blocks containing
"opencode" and "pi", handleHookEvent, and persistHookActivity.
- Around line 901-943: The tests are touching the real $HOME cache (dedupDir and
a test dir under os.homedir()), causing flakiness and pollution; change the
tests to create an isolated temp dir via fs.mkdtempSync(path.join(os.tmpdir(),
"failproofai-dedup-")) (same pattern as writeVirtualLogEntry), inject that
directory into the code under test (use the same env var or seam the dedup
implementation reads) before calling persistHookActivity/_resetForTest, snapshot
the locks directory contents before the test and assert only new lock files
created by this run (rather than global counts), and ensure you remove the temp
dir in afterEach/cleanup to avoid leaks; reference symbols: persistHookActivity,
_resetForTest, writeVirtualLogEntry, dedupDir.
In `@app/actions/update-cli-policy.ts`:
- Around line 12-21: The function toggleCliPolicyAction currently casts an
arbitrary integrationId to IntegrationType and writes it into config.cli without
validation, enabling forged integration keys; before mutating the config in
toggleCliPolicyAction (and the similar function around lines 62-71), validate
integrationId against the canonical list of supported integrations (the
IntegrationType enum/union or a central SUPPORTED_INTEGRATIONS array), reject or
throw an error for unknown values, and only then proceed with readHooksConfig(),
config.cli access, and updates; reference toggleCliPolicyAction,
readHooksConfig, config.cli and IntegrationType when adding the whitelist check
and returning/throwing for invalid ids.
In `@app/policies/hooks-client.tsx`:
- Around line 1318-1391: The CLI param UI is hidden for integrations when
cliMode === "inherit" even if the policy is globally enabled; update the
isForcedOn computation in hooks-client.tsx so that when selectedCliTab is truthy
it evaluates true if cliMode === "enable" OR (cliMode === "inherit" &&
policy.enabled), e.g. replace the current const isForcedOn = selectedCliTab ?
cliMode === "enable" : policy.enabled; with the new expression, and ensure any
downstream checks that show the params/settings button (the conditional
rendering that uses isForcedOn) keep using this updated variable so
inherited-but-enabled policies expose CLI param overrides.
In `@src/hooks/builtin-policies.ts`:
- Around line 60-135: getFilePath currently only checks top-level keys and
misses nested file path fields; update getFilePath to mirror getCommand by
parsing ctx.toolInput into record and, if top-level checks return empty, call
findNestedStringByKeys (or a new helper) with the file path keys
["file_path","filePath","path","relative_path","filename"] to search nested
payloads; ensure you still prefer direct top-level fields first
(record.file_path, record.filePath, etc.) and return the first non-empty trimmed
string found, keeping visits cap behavior consistent with
findNestedStringByKeys.
In `@src/hooks/handler.ts`:
- Around line 560-573: The current getInteg helper swallows any error from
getIntegration(type) and silently falls back to "claude-code"; change this so
configuration mistakes are not hidden by either logging and rethrowing or
refusing to process: update getInteg to call getIntegration(type) inside a
try/catch where the catch logs a clear warning (including the requested type and
the caught error) via the existing logger and then rethrows the error (so the
handler can refuse processing) instead of returning
getIntegration("claude-code"); adjust callers (the integ variable usage around
integ.normalizePayload(parsed) and integ.getCanonicalEventName(parsed,
rawEventType)) to rely on the non-fallback behavior.
In `@src/hooks/integrations.ts`:
- Around line 1358-1362: The session.idle handler currently emits both
"session.idle" and "SessionEnd", which incorrectly triggers SessionEnd policies;
update the handler in the session.idle function to only call
callcli("session.idle", { session_id: input.sessionID }) and remove the
callcli("SessionEnd", ...) invocation, keeping the existing try/catch behavior
and leaving other handlers unchanged.
- Around line 1511-1519: Remove the temporary session-source debug logging:
delete the block that builds debugInfo (ctx_sessionId, ctx_session_id,
pi_session_id, pi_sessionId, projectName) and the try/catch that calls (pi as
any).log(`[FailproofAI Debug] Session ID sources: ...`); this debug artifact
(referenced by debugInfo and pi.log) is noisy and leaks internal IDs on every
callcli() invocation and should not be shipped.
In `@src/hooks/manager.ts`:
- Around line 432-471: The global-removal branch mutates and writes the shared
scoped config inside the per-integration loop causing duplicate writes and
misleading telemetry; move the global-removal logic that operates on
policyNames/config (the block that computes removeSet, remaining,
filteredParams, builds updatedConfig and calls writeScopedHooksConfig, and the
telemetry call using getInstanceId/trackHookEvent) out of the for (const integId
of arr) loop so it runs once before iterating integrations, and leave the
per-integration loop to only handle integration-specific work (refer to symbols:
policyNames, config, writeScopedHooksConfig, filteredParams, updatedConfig,
getInstanceId, trackHookEvent, integ, arr, cliExplicit). If you prefer to keep
it inside the loop instead, assert or throw when arr.length > 1 and !cliExplicit
to force explicit CLI confirmation.
---
Outside diff comments:
In `@app/policies/hooks-client.tsx`:
- Around line 417-429: The activity event filter dropdown is missing the "Stop"
option so Stop events can't be filtered; update the select tied to
filterEventType (the element using value={filterEventType} and onChange={(e) =>
setFilterEventType(e.target.value)}) to include an additional option with value
"Stop" (display text "Stop") alongside the other <option> entries so Stop events
are selectable in the dashboard filter.
In `@src/hooks/builtin-policies.ts`:
- Around line 391-466: The current use of looksLikeFailproofStopMessage in
sanitizeJwt, sanitizeApiKeys, sanitizeConnectionStrings,
sanitizePrivateKeyContent, and sanitizeBearerTokens creates a global bypass
because it returns allow() whenever the output contains the static marker;
change this so a marker can't be trivially injected: either require the marker
to be anchored at the start of the output (e.g., startsWith behavior) or
validate it contains a per-session/per-process nonce before skipping sanitizers,
or instead only skip the specific sanitizer that emitted the stop message (track
the originating policy) rather than returning allow() for all sanitizers; update
the checks in those functions and the looksLikeFailproofStopMessage
implementation accordingly so secret-redaction runs unless the marker is proven
authentic.
- Around line 1456-1469: The policy description for requireCiGreenBeforeStop is
misleading: update its human-readable description string used in the policy
registration/definition (the text that currently reads "Require CI checks to
pass on the current branch before Claude stops") to clearly state that it only
warns and does not block, e.g. "Warn if CI checks are failing or pending on the
current branch" so the behavior matches the implementation which returns allow()
with warning messages for failing or pending checks.
---
Duplicate comments:
In `@app/policies/hooks-client.tsx`:
- Line 267: The DetailPanel expanded-row cell currently uses colSpan={10} but
the table header now renders 11 columns; update the DetailPanel colSpan to match
the new column count. Locate the expanded row <td colSpan={10}> instances in
app/policies/hooks-client.tsx (the DetailPanel/expanded row render logic) and
change the colSpan prop from 10 to 11 in both spots so the detail panel aligns
with the new Integration column.
In `@src/hooks/builtin-policies.ts`:
- Around line 1374-1404: The sessionHadToolUse helper is Claude-specific and can
mis-detect tool usage for other transcript formats; update
sessionHadToolUse(ctx: PolicyContext) to either (A) call the repository's
canonical transcript parsing utilities (use the shared/native transcript parsers
added in this PR) to detect any tool-use blocks across supported providers
instead of ad-hoc JSON parsing of ctx.session?.transcriptPath, or (B) if you
don't want to depend on those parsers here, narrow the current fast-path to
Claude only by detecting Claude transcripts (e.g., check an identifying field on
each parsed entry such as a provider/source tag or a message.metadata/provider)
and return true (fail-open) for any non-Claude transcript so
requireCiGreenBeforeStop keeps enforcing CI for other integrations; make the
change inside sessionHadToolUse and keep requireCiGreenBeforeStop calling it
unchanged.
In `@src/hooks/handler.ts`:
- Around line 541-558: The secondary payload-based integration detection biases
ambiguous inputs toward Copilot because INTEGRATIONS.copilot.detect(parsed) runs
before INTEGRATIONS.cursor.detect(parsed); update the logic in the integration
detection block that sets integrationType (the if (!integrationType) branch) so
that either INTEGRATIONS.cursor.detect(parsed) is checked before
INTEGRATIONS.copilot.detect(parsed) or, better, tighten
INTEGRATIONS.copilot.detect to require a Copilot-specific discriminator (e.g.,
vendor/product flag) on parsed before returning true; modify the copilot.detect
implementation accordingly (or swap the detect call order) to prevent minimal
Cursor payloads with shared camelCase events like preToolUse from being
misattributed.
- Around line 488-498: The 500ms timeout can drop real stdin payloads; change
the logic in the stdin-read block (the code that collects chunks and uses
setTimeout resolving to "" and references chunks and rawEventType) to instead
short-circuit immediately when process.stdin.isTTY is true, and otherwise
resolve the promise only on the stream 'end' event (or when
process.stdin.readableEnded is true), reject on 'error', and remove the
setTimeout. Also ensure you attach an 'end' listener that concatenates the
collected chunks and resolves with the full payload, and clean up listeners
after resolve/reject to avoid leaks; keep the existing hookLogWarn(rawEventType)
catch behavior.
- Around line 350-356: The current writeFileSync catch always returns false on
any error, causing the firing-lock to fail closed; change the catch in the
locking code that uses writeFileSync(lockPath, ...) so that only EEXIST returns
false (indicating the lock is held), while all other errors should be treated as
non-collision (fail-open): log the error (including the error object) and return
true so policy evaluation is not skipped. Update the catch to check (e as
any).code === "EEXIST" → return false; otherwise processLogger.warn/error with
the error and return true; also ensure any earlier mkdirSync errors are
surfaced/logged similarly so failures to create the dedup/firing-locks directory
don’t silently cause policy bypass.
In `@src/hooks/integrations.ts`:
- Around line 844-895: synchronizeCopilotProjectHooks currently returns early
when projectSettingsPath is missing, leaving previously mirrored "npx -y
failproofai --cli copilot" entries in the global config; change the function so
that when no projectSettingsPath is found it still reads globalSettingsPath,
applies the same filter that removes project-scope npx entries from gHooks (the
same filter used later: check h.bash includes "npx -y failproofai" and "--cli
copilot"), and writes the cleaned globalSettings back with writeJsonFile; keep
existing behavior when projectSettingsPath exists (merge project hooks into
gHooks) and reuse readJsonFile, gHooks, and writeJsonFile to avoid duplicating
logic.
In `@src/hooks/manager.ts`:
- Around line 519-530: The else-if clears shared enabledPolicies by only
checking the current integration (integ); change this to verify that no
integration at all has hooks installed in the target scope before wiping
policies. Replace the integ.scopes.some(...) check with a loop or any() over all
integrations (e.g., integrations or allIntegrations) that calls
eachIntegration.hooksInstalledInSettings(s as any, cwd) for the given scope(s),
and only call writeScopedHooksConfig({ ...configBeforeRemoval, enabledPolicies:
[], cli: undefined }, configScope, cwd) when none of the integrations report
hooks installed for that scope; keep the rest of the block (configBeforeRemoval,
configScope, cwd, writeScopedHooksConfig) unchanged.
---
Nitpick comments:
In `@__tests__/actions/update-cli-policy.test.ts`:
- Around line 47-63: Add two new tests for toggleCliPolicyAction to cover
inherit edge cases and preserve unrelated CLI entries: (1) test that calling
toggleCliPolicyAction("claude-code","test-policy","inherit") retains the
cli["claude-code"] object when cli["claude-code"].policyParams still has entries
(only enabledPolicies/disabledPolicies are cleaned), and (2) test that toggling
a policy for one CLI does not remove or alter an unrelated cli entry (e.g.,
"cursor") — assert writeHooksConfig receives the full config with the other CLI
preserved. Update the mocked readHooksConfig return types to use the actual
HooksConfig type from "@/src/hooks/policy-types" instead of using "as any" so
test typings catch shape regressions; keep using readHooksConfig and
writeHooksConfig mocks and reference toggleCliPolicyAction, cli[cliName], and
policyParams in assertions.
In `@__tests__/e2e/cli/stop-event.e2e.test.ts`:
- Around line 1-58: The test file is placed under __tests__/e2e/cli/ but per
project conventions all hook-related e2e tests must live under
__tests__/e2e/hooks/; move this file into __tests__/e2e/hooks/ (preserving
filename stop-event.e2e.test.ts), update any CI/test globs if needed, and ensure
imports/relative paths still resolve (verify TMP_DIR, test descriptions "E2E:
Stop Event Support (OpenCode & Pi)", and the two it() cases remain unchanged) so
test discovery and ownership match other hook tests like
opencode-integration.e2e.test.ts.
- Around line 2-6: Remove the unused imports (vi, writeFileSync, readFileSync,
join, homedir) from the test header and replace the handcrafted piped JSON with
a payload built from Payloads.stop(TMP_DIR) extended with the
integration-specific keys (set "integration" and the integration-native session
id), ensuring you include hook_event_name, transcript_path, and permission_mode
so the hook handler sees the full shape; update the test to reuse
Payloads.stop(TMP_DIR) and merge in the integration mapping rather than omitting
those fields.
In `@__tests__/hooks/handler.test.ts`:
- Around line 137-149: The test title contradicts the assertion; update the test
for handleHookEvent to reflect that cliType is defined and defaults to
"claude-code": rename the it(...) description to something like "passes default
cliType 'claude-code' to readMergedHooksConfig when no integrationType arg
given" and replace the loose expect(calls[0][1]).toBeDefined() with a precise
assertion expect(calls[0][1]).toBe("claude-code"), referencing
readMergedHooksConfig and handleHookEvent to make the intent explicit.
In `@__tests__/hooks/hooks-config.test.ts`:
- Around line 78-85: The helper function mockFiles is duplicated inside three
describe blocks; move the function mockFiles(files: Record<string, object>):
void to module scope (top of __tests__/hooks/hooks-config.test.ts) so all
describes reuse it, then delete the three inner duplicate declarations; also add
a small test that asserts project-level policyParams override global CLI
policyParams (or vice-versa as the intended precedence) to lock the cross-scope
behavior—use the same mockFiles helper and reference it inside that new test to
verify precedence.
In `@app/actions/get-hooks-config.ts`:
- Around line 106-131: The loop that builds cliOverrides from rawCli currently
copies any string key users place under config.cli (via rawCli and
cliOverrides), which can introduce phantom entries; restrict this by only
accepting keys that match your known integration list (e.g., INTEGRATION_TYPES
or the ids returned from detectInstalledIntegrations()). Update the for (const
[id, override] of Object.entries(rawCli)) loop to check that id is in the
allowed integrations set before assigning to cliOverrides (use the integration
id list from detectInstalledIntegrations() or INTEGRATION_TYPES), leaving the
rest ignored so typos like "cursor-code" are not surfaced.
In `@src/hooks/handler.ts`:
- Around line 304-314: The _resetDedupeCache function uses
require("node:fs").rmSync which mixes CommonJS require into an ESM module;
update the static import list at the top to include rmSync (alongside
existsSync, readdirSync, mkdirSync, writeFileSync, readFileSync, appendFileSync)
and replace the require call inside _resetDedupeCache with a direct call to
rmSync(dedupDir, { recursive: true, force: true }) so the function consistently
uses the statically imported fs method.
- Around line 519-558: Add a concise one-line summary comment above the "2.
Integration Detection & Normalization" block that documents the precedence used
to resolve integrationType (explicit CLI flag / normalizedCli or cliOverride →
unique-event-name heuristics via
isGeminiUniqueEvent/isCopilotUniqueEvent/CODEX_HOOK_EVENT_TYPES → payload-based
detection via INTEGRATIONS.*.detect(parsed) → default "claude-code"), and
mention that silence guards above can short-circuit this flow; reference the key
symbols integrationType, normalizedCli, cliOverride, rawEventType, parsed,
isGeminiUniqueEvent, isCopilotUniqueEvent, CODEX_HOOK_EVENT_TYPES, and
INTEGRATIONS to make the priority clear for future maintainers.
In `@src/hooks/hooks-config.ts`:
- Around line 27-90: The implementation of policyParams in readMergedHooksConfig
gives CLI-level policyParams precedence across scopes over non-CLI policyParams
(mergedParams logic), which contradicts the docstring; either make behavior
match the docstring (ensure non-CLI higher-scope policyParams like
project.policyParams win over lower-scope CLI overrides) or update the docstring
to state "CLI-level overrides global and lower scopes even over higher-scope
non-CLI entries"; to fix: decide desired precedence, then (a) if changing code,
adjust the mergedParams construction in readMergedHooksConfig to iterate scopes
in true priority order (project/local/global) for CLI and non-CLI separately so
per-scope non-CLI entries win before accepting CLI fallbacks, or (b) if keeping
current behavior, update the function docstring lines describing policyParams to
explicitly state that CLI-level entries win across scopes; finally add a unit
test in __tests__/hooks/hooks-config.test.ts that asserts the chosen precedence
for the collision case (e.g., project.policyParams["X"] vs
global_.cli[cliType].policyParams["X"]) to lock in the behavior.
In `@src/hooks/manager.ts`:
- Around line 296-312: The post-install messages special-case `claude-code` for
printing the `Command: npx -y failproofai` line but treats `copilot` as
portable without showing the command; update the conditional in
src/hooks/manager.ts to use the `isPortableProjectInstall` flag instead of
`integId === "claude-code"` and print a consistent `Command:` line for portable
installs (using the same launcher or a small integId->launcher mapping if
needed), then print the "This file can be committed..." message; otherwise keep
printing `Binary: ${binaryPath}`. This affects the block referencing
`isPortableProjectInstall`, `integId`, `scope`, `binaryPath`, `settingsPath`,
and `integ.displayName`.
🪄 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: ecb8a1a0-541b-4a35-b6cf-b3423fcace8e
📒 Files selected for processing (20)
CHANGELOG.md__tests__/actions/update-cli-policy.test.ts__tests__/e2e/cli/stop-event.e2e.test.ts__tests__/e2e/hooks/opencode-integration.e2e.test.ts__tests__/hooks/handler.test.ts__tests__/hooks/hooks-config.test.ts__tests__/hooks/integrations.test.ts__tests__/hooks/manager.test.tsapp/actions/get-hooks-config.tsapp/actions/install-hooks-web.tsapp/actions/update-cli-policy.tsapp/policies/hooks-client.tsxbin/failproofai.mjssrc/hooks/builtin-policies.tssrc/hooks/handler.tssrc/hooks/hooks-config.tssrc/hooks/integrations.tssrc/hooks/manager.tssrc/hooks/policy-types.tssrc/hooks/types.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/types.ts
- bin/failproofai.mjs
| try { | ||
| const output = execSync(cmd, { input: payload, encoding: "utf8" }); | ||
| expect(output).toContain("ALLOW"); | ||
| } catch (err: any) { | ||
| // If it's blocked, it means the hook fired and policies were evaluated! | ||
| // This is also a pass for 'Stop event support' | ||
| const combined = (err.stdout || "") + (err.stderr || "") + (err.message || ""); | ||
| expect(combined).toMatch(/ALLOW|Security Stop|require-commit-before-stop/); | ||
| } |
There was a problem hiding this comment.
Assertions are tautological — this test cannot fail meaningfully.
In both cases the try branch accepts any output containing the literal ALLOW, and the catch branch swallows every non-zero exit and only checks that the combined stdout/stderr/err.message matches /ALLOW|Security Stop|require-commit-before-stop/. Since err.message is "Command failed: …" containing the original command string, and no concrete behavior is pinned (allow vs stop, exit code, stderr prefix), regressions in the Stop-event mapping for OpenCode (session.idle) and Pi (stopReason) will not be caught.
Use the existing helpers in __tests__/e2e/helpers/hook-runner.ts (assertAllow / assertStopInstruct) and the Payloads.stop factory in __tests__/e2e/helpers/payloads.ts so the test pins exit code (0 vs 2), empty stdout, and the standardized stderr prefix ([FailproofAI / MANDATORY ACTION REQUIRED / ACTION BLOCKED BY FAILPROOFAI). Decide per case which outcome is expected with the policies enabled in this scratch dir, rather than accepting either.
Also applies to: 50-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/e2e/cli/stop-event.e2e.test.ts` around lines 29 - 37, The test
currently accepts any outcome by checking for "ALLOW" in success and a lax regex
in the catch; replace this with deterministic assertions using the existing
helpers: call Payloads.stop to build the payload, run the hook runner, then use
assertAllow from __tests__/e2e/helpers/hook-runner.ts when you expect a 0 exit
(ALLOW) or assertStopInstruct when you expect a stop (non-zero exit with
standardized stderr prefix and exit code 2). Update both the try/catch block and
the alternate case (lines 50-56) to use these helpers so the test pins exit
code, empty stdout, and the required stderr prefix instead of matching a loose
regex.
| 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.
Dedup tests write to and read from the real $HOME cache directory — flaky and pollution-prone.
Two concerns in these new dedup tests:
-
Lines 1116/1147/1169 —
dedupDir = path.join(os.homedir(), ".failproofai", "cache", "dedup", "firing-locks")is the production cache directory. On a developer machine or CI runner where failproofai has ever run, pre-existing lock files will:- silently satisfy
expect(lockFiles.length).toBeGreaterThanOrEqual(2)(line 1148) even when the integration-isolation logic regresses, - and break
expect(lockFiles).toHaveLength(0)(line 1170) due to unrelated state.
No cleanup runs after these tests, so they leak lock files into~/.failproofai.
- silently satisfy
-
Lines 905/941 — the storage-dedup test uses
path.join(os.homedir(), ".failproofai-test-dedup-storage")(still under$HOME) and reaches into_resetForTestviavi.importActualwhile the file globally mockshook-activity-store. The cleanup is good, but the test path should live underos.tmpdir()to avoid touching$HOMEat all and to play nicely with read-only/sandboxed CI homes.
Use fs.mkdtempSync(path.join(os.tmpdir(), "failproofai-dedup-")) (the same pattern already used in the writeVirtualLogEntry block at lines 970-973) and inject that directory via the same env-var/seam the dedup code reads, then rmSync in afterEach. For the integration-isolation test, also tighten the assertion to count only the locks created in this run (e.g., snapshot the directory before the call) so stale files can't mask regressions.
Also applies to: 1112-1172
🤖 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 901 - 943, The tests are
touching the real $HOME cache (dedupDir and a test dir under os.homedir()),
causing flakiness and pollution; change the tests to create an isolated temp dir
via fs.mkdtempSync(path.join(os.tmpdir(), "failproofai-dedup-")) (same pattern
as writeVirtualLogEntry), inject that directory into the code under test (use
the same env var or seam the dedup implementation reads) before calling
persistHookActivity/_resetForTest, snapshot the locks directory contents before
the test and assert only new lock files created by this run (rather than global
counts), and ensure you remove the temp dir in afterEach/cleanup to avoid leaks;
reference symbols: persistHookActivity, _resetForTest, writeVirtualLogEntry,
dedupDir.
| it("opencode: permissionMode is undefined (no mode concept)", async () => { | ||
| mockStdin(JSON.stringify({ | ||
| integration: "opencode", | ||
| session_id: "ses_abc123", | ||
| slug: "opencode", | ||
| cwd: "/project", | ||
| })); | ||
| const { persistHookActivity } = await import("../../src/hooks/hook-activity-store"); | ||
| await handleHookEvent("PreToolUse"); | ||
| expect(persistHookActivity).toHaveBeenCalledWith( | ||
| expect.objectContaining({ permissionMode: "default", integration: "opencode" }), | ||
| ); | ||
| }); | ||
|
|
||
| it("pi: permissionMode is undefined (no mode concept)", async () => { | ||
| mockStdin(JSON.stringify({ | ||
| integration: "pi", | ||
| session_id: "pi-sess-abc", | ||
| cwd: "/project", | ||
| })); | ||
| const { persistHookActivity } = await import("../../src/hooks/hook-activity-store"); | ||
| await handleHookEvent("PreToolUse"); | ||
| expect(persistHookActivity).toHaveBeenCalledWith( | ||
| expect.objectContaining({ permissionMode: "default", integration: "pi" }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Test titles say "permissionMode is undefined" but assertions expect "default".
Both the opencode and pi cases assert permissionMode: "default" while their it(...) titles claim it's undefined. This will mislead future readers about the actual contract of resolvePermissionMode for CLIs without a mode concept.
📝 Suggested rename
- it("opencode: permissionMode is undefined (no mode concept)", async () => {
+ it("opencode: permissionMode falls back to 'default' (no mode concept)", async () => {
...
- it("pi: permissionMode is undefined (no mode concept)", async () => {
+ it("pi: permissionMode falls back to 'default' (no mode concept)", async () => {📝 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.
| it("opencode: permissionMode is undefined (no mode concept)", async () => { | |
| mockStdin(JSON.stringify({ | |
| integration: "opencode", | |
| session_id: "ses_abc123", | |
| slug: "opencode", | |
| cwd: "/project", | |
| })); | |
| const { persistHookActivity } = await import("../../src/hooks/hook-activity-store"); | |
| await handleHookEvent("PreToolUse"); | |
| expect(persistHookActivity).toHaveBeenCalledWith( | |
| expect.objectContaining({ permissionMode: "default", integration: "opencode" }), | |
| ); | |
| }); | |
| it("pi: permissionMode is undefined (no mode concept)", async () => { | |
| mockStdin(JSON.stringify({ | |
| integration: "pi", | |
| session_id: "pi-sess-abc", | |
| cwd: "/project", | |
| })); | |
| const { persistHookActivity } = await import("../../src/hooks/hook-activity-store"); | |
| await handleHookEvent("PreToolUse"); | |
| expect(persistHookActivity).toHaveBeenCalledWith( | |
| expect.objectContaining({ permissionMode: "default", integration: "pi" }), | |
| ); | |
| }); | |
| it("opencode: permissionMode falls back to 'default' (no mode concept)", async () => { | |
| mockStdin(JSON.stringify({ | |
| integration: "opencode", | |
| session_id: "ses_abc123", | |
| slug: "opencode", | |
| cwd: "/project", | |
| })); | |
| const { persistHookActivity } = await import("../../src/hooks/hook-activity-store"); | |
| await handleHookEvent("PreToolUse"); | |
| expect(persistHookActivity).toHaveBeenCalledWith( | |
| expect.objectContaining({ permissionMode: "default", integration: "opencode" }), | |
| ); | |
| }); | |
| it("pi: permissionMode falls back to 'default' (no mode concept)", async () => { | |
| mockStdin(JSON.stringify({ | |
| integration: "pi", | |
| session_id: "pi-sess-abc", | |
| cwd: "/project", | |
| })); | |
| const { persistHookActivity } = await import("../../src/hooks/hook-activity-store"); | |
| await handleHookEvent("PreToolUse"); | |
| expect(persistHookActivity).toHaveBeenCalledWith( | |
| expect.objectContaining({ permissionMode: "default", integration: "pi" }), | |
| ); | |
| }); |
🤖 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 1353 - 1378, Update the two
test titles so they match the asserted behavior: change the it(...) descriptions
for the "opencode" and "pi" cases to state that permissionMode defaults to
"default" (e.g., "opencode: permissionMode defaults to 'default' (no mode
concept)" and similarly for "pi"), keeping the test body that calls
handleHookEvent and expects persistHookActivity to be called with
permissionMode: "default"; references: the it blocks containing "opencode" and
"pi", handleHookEvent, and persistHookActivity.
| export async function toggleCliPolicyAction( | ||
| integrationId: string, | ||
| policyName: string, | ||
| mode: "enable" | "disable" | "inherit", | ||
| ): Promise<void> { | ||
| const config = readHooksConfig(); | ||
| if (!config.cli) config.cli = {}; | ||
|
|
||
| const id = integrationId as IntegrationType; | ||
| if (!config.cli[id]) { |
There was a problem hiding this comment.
Reject unknown integrationIds at the server boundary.
Both actions cast an arbitrary client string to IntegrationType and write it into config.cli. A forged request can persist fake integration keys, which then pollute config normalization and UI state. Please validate integrationId against the supported integration list before mutating the config file.
Suggested fix
+import { listIntegrationIds } from "@/src/hooks/integrations";
import { readHooksConfig, writeHooksConfig } from "@/src/hooks/hooks-config";
import type { IntegrationType } from "@/src/hooks/types";
+
+function assertIntegrationId(integrationId: string): IntegrationType {
+ if (!listIntegrationIds().includes(integrationId as IntegrationType)) {
+ throw new Error(`Unsupported integration: ${integrationId}`);
+ }
+ return integrationId as IntegrationType;
+}
export async function toggleCliPolicyAction(
integrationId: string,
policyName: string,
mode: "enable" | "disable" | "inherit",
): Promise<void> {
const config = readHooksConfig();
if (!config.cli) config.cli = {};
-
- const id = integrationId as IntegrationType;
+
+ const id = assertIntegrationId(integrationId);
if (!config.cli[id]) {
config.cli[id] = {
enabledPolicies: [],
@@
export async function updateCliPolicyParamsAction(
integrationId: string,
policyName: string,
params: Record<string, unknown>,
): Promise<void> {
const config = readHooksConfig();
if (!config.cli) config.cli = {};
- const id = integrationId as IntegrationType;
+ const id = assertIntegrationId(integrationId);
if (!config.cli[id]) {
config.cli[id] = {
enabledPolicies: [],Also applies to: 62-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/actions/update-cli-policy.ts` around lines 12 - 21, The function
toggleCliPolicyAction currently casts an arbitrary integrationId to
IntegrationType and writes it into config.cli without validation, enabling
forged integration keys; before mutating the config in toggleCliPolicyAction
(and the similar function around lines 62-71), validate integrationId against
the canonical list of supported integrations (the IntegrationType enum/union or
a central SUPPORTED_INTEGRATIONS array), reject or throw an error for unknown
values, and only then proceed with readHooksConfig(), config.cli access, and
updates; reference toggleCliPolicyAction, readHooksConfig, config.cli and
IntegrationType when adding the whitelist check and returning/throwing for
invalid ids.
| // Global removal path (no --cli flag) | ||
| const removeSet = new Set(policyNames); | ||
| const remaining = config.enabledPolicies.filter((p) => !removeSet.has(p)); | ||
| const notEnabled = policyNames.filter((p) => !config.enabledPolicies.includes(p)); | ||
| if (notEnabled.length > 0) { | ||
| console.log(`Warning: policy(ies) not currently enabled: ${notEnabled.join(", ")}`); | ||
| } | ||
| const { policyParams: existingParams, ...baseConfig } = config; | ||
| const filteredParams = existingParams | ||
| ? Object.fromEntries(Object.entries(existingParams).filter(([k]) => !removeSet.has(k))) | ||
| : null; | ||
| const updatedConfig: HooksConfig = { | ||
| ...baseConfig, | ||
| enabledPolicies: remaining, | ||
| ...(filteredParams && Object.keys(filteredParams).length > 0 ? { policyParams: filteredParams } : {}), | ||
| }; | ||
| writeScopedHooksConfig(updatedConfig, configScope, cwd); | ||
|
|
||
| // Telemetry | ||
| try { | ||
| const distinctId = getInstanceId(); | ||
| const actuallyRemoved = policyNames.filter((p) => config.enabledPolicies.includes(p)); | ||
| await trackHookEvent(distinctId, "hooks_removed", { | ||
| scope, | ||
| integration: integ.id, | ||
| removal_mode: opts?.betaOnly ? "beta_policies" : "policies", | ||
| beta_only: opts?.betaOnly ?? false, | ||
| policies_removed: actuallyRemoved, | ||
| removed_count: actuallyRemoved.length, | ||
| ...(opts?.source ? { source: opts.source } : {}), | ||
| platform: platform(), | ||
| arch: arch(), | ||
| os_release: release(), | ||
| hostname_hash: hashToId(hostname()), | ||
| }); | ||
| } catch { /* best effort */ } | ||
|
|
||
| console.log(`Disabled ${policyNames.length - notEnabled.length} policy(ies) for ${integ.displayName}.`); | ||
| console.log(`Remaining: ${remaining.length > 0 ? remaining.join(", ") : "(none)"}`); | ||
| continue; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C5 'removeHooks\s*\(' --type=ts --type=js -g '!**/__tests__/**' -g '!**/dist/**'Repository: exospherehost/failproofai
Length of output: 1448
🏁 Script executed:
#!/bin/bash
# Get the removeHooks function implementation
sed -n '327,500p' src/hooks/manager.ts | head -200Repository: exospherehost/failproofai
Length of output: 7493
🏁 Script executed:
#!/bin/bash
# Find all calls to removeHooks to see how it's invoked
rg -nP 'removeHooks\s*\(' -A2 -B2 --type=ts --type=js -g '!**/__tests__/**' -g '!**/dist/**'Repository: exospherehost/failproofai
Length of output: 618
🏁 Script executed:
#!/bin/bash
# Find the CLI entry point and see how it handles removal commands
find . -name 'failproofai.mjs' -o -name 'failproofai.ts' | head -5Repository: exospherehost/failproofai
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Read the CLI entry point to see how removal is handled
cat -n bin/failproofai.mjs | head -300Repository: exospherehost/failproofai
Length of output: 14082
🏁 Script executed:
#!/bin/bash
# Continue reading to see uninstall handling
sed -n '300,450p' bin/failproofai.mjsRepository: exospherehost/failproofai
Length of output: 5760
🏁 Script executed:
#!/bin/bash
# More thorough search for removeHooks usage throughout the codebase
rg -r 'removeHooks' --type=ts --type=js -g '!**/dist/**' -g '!**/node_modules/**'Repository: exospherehost/failproofai
Length of output: 466
🏁 Script executed:
#!/bin/bash
# Search for all occurrences of removeHooks
rg 'removeHooks' --type=ts --type=js -g '!**/dist/**' -g '!**/node_modules/**'Repository: exospherehost/failproofai
Length of output: 6155
🏁 Script executed:
#!/bin/bash
# Look at removeHooks tests to understand how it's called
sed -n '1,50p' __tests__/hooks/manager.test.tsRepository: exospherehost/failproofai
Length of output: 1823
🏁 Script executed:
#!/bin/bash
# Search for test cases that call removeHooks with array integrations
rg -A5 'removeHooks.*integration.*\[' __tests__/hooks/manager.test.tsRepository: exospherehost/failproofai
Length of output: 452
🏁 Script executed:
#!/bin/bash
# Get more context around this test
rg -B10 -A15 'removeHooks.*integration.*\["claude-code"\]' __tests__/hooks/manager.test.tsRepository: exospherehost/failproofai
Length of output: 1554
🏁 Script executed:
#!/bin/bash
# Search for tests with multiple integrations in the array
rg -B5 -A10 'integration.*\[\s*["\x27][^"]+["\x27]\s*,\s*["\x27]' __tests__/hooks/manager.test.tsRepository: exospherehost/failproofai
Length of output: 778
🏁 Script executed:
#!/bin/bash
# Search for removeHooks calls with multiple integrations that don't set cliExplicit
rg -B3 -A3 'removeHooks.*integration.*\[.*,.*\]' __tests__/hooks/manager.test.ts | grep -v cliExplicitRepository: exospherehost/failproofai
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Look at all removeHooks calls in tests and check if any multi-integration calls omit cliExplicit
sed -n '1,1000p' __tests__/hooks/manager.test.ts | grep -A5 'removeHooks.*\['Repository: exospherehost/failproofai
Length of output: 619
🏁 Script executed:
#!/bin/bash
# Get comprehensive view of all removeHooks invocations in tests
rg 'await removeHooks' -B2 -A8 __tests__/hooks/manager.test.ts | head -200Repository: exospherehost/failproofai
Length of output: 6693
🏁 Script executed:
#!/bin/bash
# Search for removeHooks calls with array integration but without cliExplicit explicitly set
rg -B3 -A10 'removeHooks\(' __tests__/hooks/manager.test.ts | grep -B8 -A2 'integration.*\[.*,.*\]'Repository: exospherehost/failproofai
Length of output: 315
🏁 Script executed:
#!/bin/bash
# Check if there are any programmatic calls to removeHooks in non-test, non-test code
rg 'removeHooks\(' -B5 -A5 --type=ts --type=js -g '!__tests__/**' -g '!**/dist/**' | grep -v 'export async function'Repository: exospherehost/failproofai
Length of output: 1199
Global-removal path is wrapped in the per-integration loop — potential duplicate writes if multiple integrations are passed without cliExplicit.
Lines 432-471 modify the shared scoped config (config.enabledPolicies, policyParams) and emit telemetry inside for (const integId of arr). The code logic shows that when arr.length > 1 and cliExplicit is not set:
- 1st iteration: removes the named policies and writes the config
- 2nd+ iteration: re-reads the already-pruned config, computes
notEnabled = policyNames(all of them), logs misleading output withremoved_count: 0, and emits duplicate telemetry
While the CLI code currently prevents this scenario (it sets cliExplicit: true whenever multiple integrations are passed via --cli), the architectural issue remains in the function logic. Either:
- Hoist the global-removal branch out of the loop (run once before iterating), so it modifies shared state exactly once and the per-integ loop only handles per-integration concerns; or
- Assert that multi-integration removal requires
cliExplicit: true
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/manager.ts` around lines 432 - 471, The global-removal branch
mutates and writes the shared scoped config inside the per-integration loop
causing duplicate writes and misleading telemetry; move the global-removal logic
that operates on policyNames/config (the block that computes removeSet,
remaining, filteredParams, builds updatedConfig and calls
writeScopedHooksConfig, and the telemetry call using
getInstanceId/trackHookEvent) out of the for (const integId of arr) loop so it
runs once before iterating integrations, and leave the per-integration loop to
only handle integration-specific work (refer to symbols: policyNames, config,
writeScopedHooksConfig, filteredParams, updatedConfig, getInstanceId,
trackHookEvent, integ, arr, cliExplicit). If you prefer to keep it inside the
loop instead, assert or throw when arr.length > 1 and !cliExplicit to force
explicit CLI confirmation.
- Fix tool_input/toolInput shapes in Gemini and Copilot payloads (object, not raw string) - Isolate Cursor and Gemini e2e tests with per-test isoHome (mkdtempSync + HOME override) - Remove real-home DEDUP_DIR deletion from OpenCode e2e beforeEach (redundant with isoHome) - Restore process.env.HOME in log-entries test afterEach to prevent cross-test leakage - Fix DetailPanel colSpan 10 → 11 to span all activity table columns - Fix isForcedOn to show param-edit button when CLI is in inherit mode and global policy is enabled - Fix isOpencodeSessionMerged to compare encodedCwd against virtual folder names instead of session IDs - Add cwd field to ProjectFolder and pass it from readOpencodeDbEntries - Fix resolveAnyProjectPath to use existsSync instead of unreachable double-try pattern - Remove double-emit of SessionEnd on OpenCode session.idle events - Remove debug logging of Pi session ID sources from integrations.ts - Log warning in getInteg before falling back to claude-code for unknown integration types - Fix getFilePath to use findNestedStringByKeys for nested payload lookup - Fix scripts/codex-trace.mjs shebang from node to bun Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (3)
src/hooks/handler.ts (2)
350-356:⚠️ Potential issue | 🔴 CriticalOnly
EEXISTshould lose the firing lock.Right now any other
writeFileSync(..., { flag: "wx" })failure returnsfalse, so an unwritable cache dir orENOSPCmakes the handler silently treat the event as a duplicate and skip policy evaluation entirely. That is a policy bypass, not a harmless dedupe miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 350 - 356, The current try/catch around writeFileSync(lockPath, String(process.pid), { flag: "wx" }) treats every error as a duplicate and returns false; change it so only an EEXIST error returns false and all other errors are rethrown (or propagated) so the handler does not silently skip policy evaluation on ENOSPC, EACCES, etc.; update the catch in src/hooks/handler.ts to check (e as any).code === "EEXIST" => return false, otherwise rethrow the error (or throw a new Error with context) so failures to create the lock are not treated as dedupe.
488-491:⚠️ Potential issue | 🟠 MajorRemove the 500ms wall-clock fallback for stdin.
A slow but valid hook producer will resolve
""here beforeend, which means the handler falls back to{}and loses the real integration/session/tool payload for this invocation. Resolve on stream end, and only short-circuit immediately for TTY/no-pipe cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/handler.ts` around lines 488 - 491, Remove the 500ms setTimeout fallback in the stdin read block (the code that checks chunks and calls resolve("")) and instead resolve only when the stdin stream emits 'end'; keep an immediate short-circuit only for TTY/no-pipe cases (use process.stdin.isTTY or equivalent) so you don't prematurely resolve an empty string. Locate the stdin-reading logic around the existing setTimeout in src/hooks/handler.ts (the variables chunks and resolve) and replace the timeout-based fallback with an 'end' event handler that joins chunks and calls resolve(payload), ensuring you do not call resolve("") until you have confirmed stdin is a TTY/no-pipe.src/hooks/builtin-policies.ts (1)
1375-1397:⚠️ Potential issue | 🟠 MajorNon-Claude sessions will still look like “no tool use” here.
sessionHadToolUse()only recognizes Claude-stylemessage.content[].type === "tool_use"entries, butctx.session.transcriptPathstill points at the native Copilot/Cursor/Codex/Gemini/Pi/OpenCode transcript. The Claude-shaped mirror is written separately insrc/hooks/handler.tsLines 773-780, so Stop events for those integrations can returnfalsehere and skiprequire-ci-green-before-stop.🤖 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 1375 - 1397, sessionHadToolUse currently only detects Claude-style message.content[].type === "tool_use" and misses native transcripts; update sessionHadToolUse (and its use of ctx.session?.transcriptPath) to also detect non-Claude tool entries by checking parsed entry shapes used by the native mirrors: after JSON.parse(entry) inspect top-level fields such as entry.type === "tool", entry.event === "tool" (or "stop"/"tool" variants), presence of entry.tool or entry.tool_name or entry.tool_output, and any entry.message?.role === "tool" — return true when any of these are present; keep existing Claude detection (message.content array with block.type === "tool_use") and preserve the current fallback behavior on unreadable files/exceptions.
🧹 Nitpick comments (9)
__tests__/e2e/hooks/gemini-integration.e2e.test.ts (2)
84-90: Replace hand-rolled JSON string withJSON.stringify.The
toolArgsvalue is being constructed by manual string concatenation with backslash escaping. This is brittle and easy to break (e.g., ifPROJECT_DIRever contains a quote, brace, or newline). UseJSON.stringifyfor a correctly-escaped payload.♻️ Proposed fix
const payload = { session_id: "test-session-gemini-json-001", cwd: PROJECT_DIR, hook_event_name: "BeforeTool", toolName: "Shell", - toolArgs: "{\"command\":\"sudo apt-get update\",\"cwd\":\"" + PROJECT_DIR.replace(/\\/g, "\\\\") + "\"}", + toolArgs: JSON.stringify({ command: "sudo apt-get update", cwd: PROJECT_DIR }), };🤖 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 84 - 90, The payload construction uses a hand-rolled JSON string for toolArgs which is brittle; update the payload (the payload object created in the test) to set toolArgs via JSON.stringify({ command: "sudo apt-get update", cwd: PROJECT_DIR }) instead of manually concatenating and escaping PROJECT_DIR so the value is correctly escaped regardless of characters in PROJECT_DIR.
41-46: Consider using the sharedrunHookhelper.Other E2E suites in this PR use
runHook(event, payload, { homeDir, cli, cwd })from__tests__/e2e/helpers/hook-runner.ts, which centralizes env setup (includingFAILPROOFAI_TELEMETRY_DISABLED,FAILPROOFAI_DISABLE_INSTANT_CATCH) and JSON parsing. Switching this suite over reduces duplication and ensures consistent isolation flags across integrations. Optional, but recommended for long-term maintainability.🤖 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 41 - 46, Replace the manual spawnSync call that invokes BINARY_PATH (the block using spawnSync with "bun", BINARY_PATH, "--hook", "BeforeTool", "--cli", "gemini", input: JSON.stringify(payload), env: { ...baseEnv(), FAILPROOFAI_LOG_LEVEL: "info", FAILPROOFAI_SKIP_KILL: "true" }, cwd: PROJECT_DIR) with the shared runHook helper: call runHook("BeforeTool", payload, { homeDir: /* use same home used by other tests or baseEnv() equivalent */, cli: "gemini", cwd: PROJECT_DIR }) so the test inherits centralized env flags (FAILPROOFAI_TELEMETRY_DISABLED, FAILPROOFAI_DISABLE_INSTANT_CATCH, etc.) and JSON handling; preserve any explicit overrides like FAILPROOFAI_LOG_LEVEL or FAILPROOFAI_SKIP_KILL by passing them through the helper options or ensuring base test setup covers them.__tests__/e2e/helpers/payloads.ts (2)
154-173: Consider adding anoverridesparameter for parity withCopilotPayloads.
CopilotPayloadsaccepts anoverrides: Record<string, unknown>on most factories, which is useful for E2E tests that need to vary fields likesession_id,cwd, or addtranscript_path.GeminiPayloads.beforeTool.bashandafterAgentdon't, forcing callers (e.g.,withSessionIdinintegration-adapter.ts) to mutate via spread externally. Adding the sameoverridespattern keeps the helpers consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/helpers/payloads.ts` around lines 154 - 173, GeminiPayloads helpers lack an overrides parameter like CopilotPayloads, forcing external mutation; update GeminiPayloads.beforeTool.bash and GeminiPayloads.afterAgent to accept an optional overrides: Record<string, unknown> argument and merge it into the returned object (ensuring session_id and cwd defaults remain), e.g., add a third param or optional last param named overrides and spread it into the returned payload so callers (like withSessionId in integration-adapter.ts) can override fields without mutating the result.
118-127: Misleading namespace:afterFileEditplaced underpreToolUse.
CursorPayloads.preToolUse.writebuilds anafterFileEditpayload, which is a post-edit event. Grouping it underpreToolUseis confusing for callers (e.g.,integration-adapter.tswill pullpreToolUse.bashfor pre-tool denial assertions, butpreToolUse.writeactually returns a post-edit event). Consider moving it to apostToolUse(or dedicatedfileEdit) sub-namespace to match its semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/e2e/helpers/payloads.ts` around lines 118 - 127, The write payload in CursorPayloads is mis-grouped: CursorPayloads.preToolUse.write constructs an afterFileEdit (post-edit) event; move this factory out of the preToolUse namespace into a new postToolUse (or fileEdit) sub-namespace (e.g., CursorPayloads.postToolUse.write or CursorPayloads.fileEdit.write) and update all callers that reference CursorPayloads.preToolUse.write (such as integration-adapter.ts) to use the new namespace; ensure the returned object (session_id, workspace_roots, integration, hook_event_name: "afterFileEdit", tool_name: "edit_file", tool_input) remains unchanged.app/policies/hooks-client.tsx (1)
430-443: Source filter dropdown options from INTEGRATION_TYPES to prevent drift.The integration dropdown at lines 430–443 hard-codes the list
["claude-code", "cursor", "gemini", "copilot", "codex", "opencode", "pi"], which matchesINTEGRATION_TYPESinsrc/hooks/types.ts. When a new integration is added to the registry, this filter will silently become stale, preventing filtering by the new integration type. The installation modal already avoids this by dynamically consuming data fromgetIntegrationsStatusAction().Replace the hard-coded
<option>elements with a loop overINTEGRATION_TYPES(which is already imported successfully in other client code likelib/projects.ts):const { INTEGRATION_TYPES } = await import("@/src/hooks/types"); const integrationNames = { "claude-code": "Claude Code", "cursor": "Cursor", "gemini": "Gemini", "copilot": "Copilot", "codex": "Codex", "opencode": "OpenCode", "pi": "Pi Agent", } as const; INTEGRATION_TYPES.map((id) => ( <option key={id} value={id}> {integrationNames[id]} </option> ))Alternatively, derive display names dynamically from
getIntegration(id).displayNameto make the registry the single source of truth for both IDs and labels.🤖 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 430 - 443, Replace the hard-coded integration <option> elements in the select with a dynamic loop over INTEGRATION_TYPES: import or lazy-load INTEGRATION_TYPES from "@/src/hooks/types" and map each id to an <option key={id} value={id}> using either a local display-name map (e.g., mapping "claude-code" -> "Claude Code") or by calling getIntegration(id).displayName so labels come from the integration registry; update the select to use setFilterIntegration as before but produce options from INTEGRATION_TYPES instead of the fixed list.__tests__/hooks/manager.test.ts (2)
1239-1244: Loop body never asserts whenwriteCallsis empty.If a future change accidentally short-circuits the
scope: "all"wipe so thatwriteScopedHooksConfigis never invoked, this loop simply runs zero times and the test passes vacuously. Add an explicit length assertion to guard against silent regressions:♻️ Suggested guard
const writeCalls = vi.mocked(writeScopedHooksConfig).mock.calls; + expect(writeCalls.length).toBeGreaterThan(0); for (const [written] of writeCalls) { expect(written.enabledPolicies).toEqual([]); expect(written.cli).toBeUndefined(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/manager.test.ts` around lines 1239 - 1244, The loop over vi.mocked(writeScopedHooksConfig).mock.calls can be empty and let the test pass vacuously; add an explicit assertion on the number of calls (the writeCalls array) before iterating to ensure writeScopedHooksConfig was invoked (e.g., assert writeCalls.length is the expected count or > 0) so the test fails if the wipe short-circuits and no writes occur.
540-549: Cross-CLI substring contamination risk in--cli ${integ}matcher.
combinedContentBytes.toContain(\--cli ${integ}`)will match any prefix collision between integration IDs (e.g."copilot"would match"copilot-sync"or vice-versa if such an ID is ever added). This is fine for today'sINTEGRATION_TYPESbut is fragile. Consider asserting against a regex with a word/space boundary, e.g.new RegExp(`--cli${integ}(\s|"|$ )`)`, so future additions can't make this assertion silently false-positive. Same concern applies to Line 575.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/manager.test.ts` around lines 540 - 549, The test currently asserts substring presence using combinedContentBytes.toContain(`--cli ${integ}`) which can false-positive on prefix collisions; update the assertions that check combinedContentBytes (and the similar check around line 575) to use a regex that enforces a boundary after the integration id (e.g., new RegExp(`--cli ${integ}(\\s|"|$)`)) and use expect(...).toMatch(regex) so each INTEGRATION_TYPES entry is matched as a whole token; update references in the test to use combinedContentBytes, INTEGRATION_TYPES and the mocked writeCalls accordingly.lib/projects.ts (2)
17-17: Merge the mid-file import with the top-of-file import.
INTEGRATION_TYPESis imported from the same module already imported at Line 17 (IntegrationType). Inline imports work but break the convention used in the rest of the file and make the module's dependency surface harder to scan.🛠️ Suggested change
-import { IntegrationType } from "@/src/hooks/types"; +import { IntegrationType, INTEGRATION_TYPES } from "@/src/hooks/types"; @@ -import { INTEGRATION_TYPES } from "@/src/hooks/types"; const VIRTUAL_INTEGRATIONS = INTEGRATION_TYPES as unknown as string[];Also applies to: 125-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/projects.ts` at line 17, Merge the mid-file import of INTEGRATION_TYPES into the existing top import from "@/src/hooks/types" so both symbols are imported together (replace separate import statements for IntegrationType and INTEGRATION_TYPES with a single import). Update any import lines that reference IntegrationType and INTEGRATION_TYPES (and the duplicate mid-file import around lines referenced) to use one consolidated import declaration, preserving named imports and formatting consistent with the rest of the file.
96-123: Wrapdb.close()infinallyto avoid leaking the SQLite handle on query errors.
db.query(...).all()runs beforedb.close(). Ifquery/allthrows (corrupt DB, schema drift, etc.), the outercatchswallows the error and returns[], but the openDatabasehandle is never closed. Same pattern ingetOpencodeDbSessionsForCwdat Line 167–171. Minor in practice (the process will reclaim it), but atry { ... } finally { db.close() }keeps things tidy.🛠️ Suggested change
const db = new mod.Database(OPENCODE_DB_PATH, { readonly: true }); - const rows: Array<{ id: string; directory: string; time_created: number }> = db.query( - "SELECT id, directory, time_created FROM session ORDER BY time_created DESC" - ).all(); - db.close(); - return rows.map((row) => { + let rows: Array<{ id: string; directory: string; time_created: number }>; + try { + rows = db.query( + "SELECT id, directory, time_created FROM session ORDER BY time_created DESC" + ).all(); + } finally { + db.close(); + } + return rows.map((row) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/projects.ts` around lines 96 - 123, The SQLite Database handle opened in readOpencodeDbEntries (and similarly in getOpencodeDbSessionsForCwd) must be closed even if query/all throws: move the db.close() into a finally block (i.e., create the Database with const db = new mod.Database(...); then run the query inside try { ... } and ensure finally { db.close(); }) so the handle is always closed; update both functions (readOpencodeDbEntries and getOpencodeDbSessionsForCwd) to use this try/finally pattern around db.query(...).all() and remove db.close() from the normal flow.
🤖 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/gemini-integration.e2e.test.ts`:
- Around line 47-48: Remove the two debug console.log statements that print
Gemini output in __tests__/e2e/hooks/gemini-integration.e2e.test.ts by deleting
the lines that reference stdout and stderr (the console.log("Gemini STDOUT:",
stdout) and console.log("Gemini STDERR:", stderr) calls); if you need to keep
the data for debugging, replace with a non-verbose test-only logger guarded by a
debug flag, otherwise simply remove the statements so test output is not
polluted.
- Around line 115-121: Replace the manually constructed payload object with the
proper GeminiPayloads factory call so the test matches the BeforeTool schema:
use GeminiPayloads.beforeTool.bash("env", PROJECT_DIR) instead of the hardcoded
payload (the variable named payload) which currently sets tool_input as a
string; this ensures the payload produced by the test matches the object format
(tool_input: { command }) expected by getCommand and other tests.
In `@__tests__/hooks/manager.test.ts`:
- Around line 1019-1037: The test title says "does not write" but the assertion
expects a write; update the test to make intent consistent: either change the
assertion to expect writeScopedHooksConfig not to have been called (use
not.toHaveBeenCalled()) if removeHooks should skip writing when the policy list
isn't enabled, or if removeHooks intentionally writes a no-op config, rename the
test string to reflect that behavior; locate the test that calls removeHooks and
asserts on writeScopedHooksConfig and apply the chosen fix so the test name and
assertion align.
- Around line 1156-1175: The test title contradicts its inline comment and
assertion: update the test for removeHooks to accurately reflect current
behavior by renaming the it(...) description to something like "removes from CLI
enabledPolicies; global suppression requires a separate run" and add an
assertion that cli?.["gemini"]?.disabledPolicies does NOT contain "block-rm-rf"
(using the same writtenConfig from writeScopedHooksConfig call) so the test
verifies removal from enabledPolicies only; also update the inline comment to
match this expectation. Reference symbols: removeHooks, readScopedHooksConfig,
writeScopedHooksConfig, cli.gemini.enabledPolicies, cli.gemini.disabledPolicies.
In `@app/policies/hooks-client.tsx`:
- Around line 1105-1109: handleInstall currently calls
getIntegrationsStatusAction() and leaves integrationsList null on rejection, so
update the handleInstall function to attach a .catch to the promise returned by
getIntegrationsStatusAction(); in the .then branch keep
setIntegrationsList(results) as-is, and in the .catch call setActionError(err)
to surface the error, setShowIntegrationModal(false) to close the modal, and
setIntegrationsList([]) or other non-null fallback to avoid the perpetual
"Loading…" state; reference handleInstall, getIntegrationsStatusAction,
setIntegrationsList, setShowIntegrationModal, and setActionError when making the
changes.
- Around line 850-852: The modal's initial selected state is computed once from
integrations and stays empty when integrations are null; add a useEffect that
watches integrations (and modal open state if present) and when integrations
becomes non-null setSelected(new Set(integrations.filter(i => i.installed).map(i
=> i.id))) to sync selections; update the effect to run only when integrations
changes to avoid overwriting user interaction; refer to selected, setSelected,
integrations, handleInstall, and getIntegrationsStatusAction to locate the code.
- Around line 571-573: Remove the unnecessary type cast on the integration prop:
replace the casted prop so that IntegrationBadge receives item.integration
directly (no "as any"). Specifically, in the JSX where IntegrationBadge is
rendered, stop casting item.integration and pass it as-is
(HookActivityEntry.integration / item.integration -> IntegrationBadge
integration prop) so the TypeScript compiler can enforce correct types.
In `@lib/log-entries.ts`:
- Around line 188-217: The event classification using lowEvent.includes(...) in
the isUserEvent check misclassifies assistant/tool events containing substrings
like "message", "chat", or "input"; update the logic in the isUserEvent block to
use an explicit allow-list of normalized event names (e.g.,
["UserPromptSubmit","userPromptSubmitted","prompt","chat.userMessage"]) or
normalize hookEventName and compare exact values instead of substring matches,
keeping the same prompt extraction that uses e.toolInput and the prompt
variable; modify the lowEvent checks (the isUserEvent variable and related
condition) to use the new allow-list/normalized name comparison so
assistant/tool events (e.g., agentMessage, assistant.message, tool.message) no
longer route to the user branch.
- Around line 1724-1734: The mirroredPath using resolveProjectPath(sid)
incorrectly assumes the Claude mirrored JSONL lives under the session id; update
the logic in the block that sets mirroredPath / reads the file so that for
OpenCode sessions (ses_…) you derive the project folder from the entry's cwd
(e.g. use encodeCwd(matching[0].cwd!) instead of sid) before calling readFile,
then populate mirroredEntries/usedMirrored accordingly; alternatively, if you do
not intend to merge OpenCode mirrored files, remove this mirroredPath/readFile
try-catch block entirely so the activity-store fallback is the obvious path
(adjust references to mirroredEntries and usedMirrored where the merge branch is
checked).
- Around line 1753-1806: The filter is using VIRTUAL_INTEGRATIONS (cast from
INTEGRATION_TYPES) which currently includes "claude-code", causing Claude
sessions with activity entries to skip native JSONL parsing; update the matching
logic to exclude Claude by filtering out "claude-code" from VIRTUAL_INTEGRATIONS
(or adjust the predicate) so matchingEntries only includes true virtual
integrations (e.g., build VIRTUAL_INTEGRATIONS = (INTEGRATION_TYPES as
string[]).filter(i => i !== "claude-code") or add entry.integration !==
"claude-code" to the filter), keep the rest of the flow
(getAllHookActivityEntries, matchingEntries, mirrored merge) unchanged and
optionally rename VIRTUAL_INTEGRATIONS to clarify its scope.
In `@lib/projects.ts`:
- Around line 228-233: The insertion into projectMap is overwriting an existing
folder.sources array with a single-element array from folder.source; change the
logic in the block that calls projectMap.set (the code handling folder insertion
inside getVirtualProjectsFromActivityStore) so it preserves/merges the full
sources list: set sources to folder.sources if present, otherwise fall back to
folder.source wrapped as an array (i.e., use folder.sources ?? (folder.source ?
[folder.source] : [])), or merge existing map entry sources with folder.sources
when updating an existing entry so multi-source virtual folders are not reduced
to one source.
In `@src/hooks/builtin-policies.ts`:
- Around line 496-499: The helper looksLikeFailproofStopMessage currently uses
substring checks which allow bypasses; change it to require an exact trusted
marker match instead of includes — update the function
looksLikeFailproofStopMessage to compare the normalized input against exact
marker strings (or anchored regexes that match only the full trusted markers
such as "MANDATORY ACTION REQUIRED from FailproofAI (policy:" and "[FailproofAI
Security Stop] Policy:") rather than using text.includes, so only an exact
marker token (with any allowed canonical spacing/encoding) will return true.
- Around line 205-206: The current regexes ENV_FILE_PATH_RE and ENV_CMD_RE are
too broad because `env_\w*` matches arbitrary filenames like `env_config`;
update both patterns to only match common dotenv conventions (e.g. `.env`,
`.env.local`, `.env.production`, or `env.<suffix>`), for example replace
`env_\w*` with a more specific alternation such as
`(?:\.env(?:\.[A-Za-z0-9_-]+)?|env\.[A-Za-z0-9_-]+)` so ENV_FILE_PATH_RE remains
anchored to path boundaries and ENV_CMD_RE matches the command/file token form
without capturing unrelated names.
- Around line 631-634: The current guard in the REMOTE_SCRIPT_DOWNLOAD_RE branch
only detects short flags via /-[Oo]\b/, letting long-form flags like --output or
--output-document bypass the rule; update the conditional in the same block (the
if that references REMOTE_SCRIPT_DOWNLOAD_RE and calls deny(...)) to also detect
common long-form save flags (e.g., --output, --output-document, --remote-name)
by replacing the short-flag regex with a broader, case-insensitive pattern that
matches both short and long forms (or add an additional test for those long
flags) so downloads using --output/--output-document/etc. are correctly denied.
---
Duplicate comments:
In `@src/hooks/builtin-policies.ts`:
- Around line 1375-1397: sessionHadToolUse currently only detects Claude-style
message.content[].type === "tool_use" and misses native transcripts; update
sessionHadToolUse (and its use of ctx.session?.transcriptPath) to also detect
non-Claude tool entries by checking parsed entry shapes used by the native
mirrors: after JSON.parse(entry) inspect top-level fields such as entry.type ===
"tool", entry.event === "tool" (or "stop"/"tool" variants), presence of
entry.tool or entry.tool_name or entry.tool_output, and any entry.message?.role
=== "tool" — return true when any of these are present; keep existing Claude
detection (message.content array with block.type === "tool_use") and preserve
the current fallback behavior on unreadable files/exceptions.
In `@src/hooks/handler.ts`:
- Around line 350-356: The current try/catch around writeFileSync(lockPath,
String(process.pid), { flag: "wx" }) treats every error as a duplicate and
returns false; change it so only an EEXIST error returns false and all other
errors are rethrown (or propagated) so the handler does not silently skip policy
evaluation on ENOSPC, EACCES, etc.; update the catch in src/hooks/handler.ts to
check (e as any).code === "EEXIST" => return false, otherwise rethrow the error
(or throw a new Error with context) so failures to create the lock are not
treated as dedupe.
- Around line 488-491: Remove the 500ms setTimeout fallback in the stdin read
block (the code that checks chunks and calls resolve("")) and instead resolve
only when the stdin stream emits 'end'; keep an immediate short-circuit only for
TTY/no-pipe cases (use process.stdin.isTTY or equivalent) so you don't
prematurely resolve an empty string. Locate the stdin-reading logic around the
existing setTimeout in src/hooks/handler.ts (the variables chunks and resolve)
and replace the timeout-based fallback with an 'end' event handler that joins
chunks and calls resolve(payload), ensuring you do not call resolve("") until
you have confirmed stdin is a TTY/no-pipe.
---
Nitpick comments:
In `@__tests__/e2e/helpers/payloads.ts`:
- Around line 154-173: GeminiPayloads helpers lack an overrides parameter like
CopilotPayloads, forcing external mutation; update
GeminiPayloads.beforeTool.bash and GeminiPayloads.afterAgent to accept an
optional overrides: Record<string, unknown> argument and merge it into the
returned object (ensuring session_id and cwd defaults remain), e.g., add a third
param or optional last param named overrides and spread it into the returned
payload so callers (like withSessionId in integration-adapter.ts) can override
fields without mutating the result.
- Around line 118-127: The write payload in CursorPayloads is mis-grouped:
CursorPayloads.preToolUse.write constructs an afterFileEdit (post-edit) event;
move this factory out of the preToolUse namespace into a new postToolUse (or
fileEdit) sub-namespace (e.g., CursorPayloads.postToolUse.write or
CursorPayloads.fileEdit.write) and update all callers that reference
CursorPayloads.preToolUse.write (such as integration-adapter.ts) to use the new
namespace; ensure the returned object (session_id, workspace_roots, integration,
hook_event_name: "afterFileEdit", tool_name: "edit_file", tool_input) remains
unchanged.
In `@__tests__/e2e/hooks/gemini-integration.e2e.test.ts`:
- Around line 84-90: The payload construction uses a hand-rolled JSON string for
toolArgs which is brittle; update the payload (the payload object created in the
test) to set toolArgs via JSON.stringify({ command: "sudo apt-get update", cwd:
PROJECT_DIR }) instead of manually concatenating and escaping PROJECT_DIR so the
value is correctly escaped regardless of characters in PROJECT_DIR.
- Around line 41-46: Replace the manual spawnSync call that invokes BINARY_PATH
(the block using spawnSync with "bun", BINARY_PATH, "--hook", "BeforeTool",
"--cli", "gemini", input: JSON.stringify(payload), env: { ...baseEnv(),
FAILPROOFAI_LOG_LEVEL: "info", FAILPROOFAI_SKIP_KILL: "true" }, cwd:
PROJECT_DIR) with the shared runHook helper: call runHook("BeforeTool", payload,
{ homeDir: /* use same home used by other tests or baseEnv() equivalent */, cli:
"gemini", cwd: PROJECT_DIR }) so the test inherits centralized env flags
(FAILPROOFAI_TELEMETRY_DISABLED, FAILPROOFAI_DISABLE_INSTANT_CATCH, etc.) and
JSON handling; preserve any explicit overrides like FAILPROOFAI_LOG_LEVEL or
FAILPROOFAI_SKIP_KILL by passing them through the helper options or ensuring
base test setup covers them.
In `@__tests__/hooks/manager.test.ts`:
- Around line 1239-1244: The loop over
vi.mocked(writeScopedHooksConfig).mock.calls can be empty and let the test pass
vacuously; add an explicit assertion on the number of calls (the writeCalls
array) before iterating to ensure writeScopedHooksConfig was invoked (e.g.,
assert writeCalls.length is the expected count or > 0) so the test fails if the
wipe short-circuits and no writes occur.
- Around line 540-549: The test currently asserts substring presence using
combinedContentBytes.toContain(`--cli ${integ}`) which can false-positive on
prefix collisions; update the assertions that check combinedContentBytes (and
the similar check around line 575) to use a regex that enforces a boundary after
the integration id (e.g., new RegExp(`--cli ${integ}(\\s|"|$)`)) and use
expect(...).toMatch(regex) so each INTEGRATION_TYPES entry is matched as a whole
token; update references in the test to use combinedContentBytes,
INTEGRATION_TYPES and the mocked writeCalls accordingly.
In `@app/policies/hooks-client.tsx`:
- Around line 430-443: Replace the hard-coded integration <option> elements in
the select with a dynamic loop over INTEGRATION_TYPES: import or lazy-load
INTEGRATION_TYPES from "@/src/hooks/types" and map each id to an <option
key={id} value={id}> using either a local display-name map (e.g., mapping
"claude-code" -> "Claude Code") or by calling getIntegration(id).displayName so
labels come from the integration registry; update the select to use
setFilterIntegration as before but produce options from INTEGRATION_TYPES
instead of the fixed list.
In `@lib/projects.ts`:
- Line 17: Merge the mid-file import of INTEGRATION_TYPES into the existing top
import from "@/src/hooks/types" so both symbols are imported together (replace
separate import statements for IntegrationType and INTEGRATION_TYPES with a
single import). Update any import lines that reference IntegrationType and
INTEGRATION_TYPES (and the duplicate mid-file import around lines referenced) to
use one consolidated import declaration, preserving named imports and formatting
consistent with the rest of the file.
- Around line 96-123: The SQLite Database handle opened in readOpencodeDbEntries
(and similarly in getOpencodeDbSessionsForCwd) must be closed even if query/all
throws: move the db.close() into a finally block (i.e., create the Database with
const db = new mod.Database(...); then run the query inside try { ... } and
ensure finally { db.close(); }) so the handle is always closed; update both
functions (readOpencodeDbEntries and getOpencodeDbSessionsForCwd) to use this
try/finally pattern around db.query(...).all() and remove db.close() from the
normal flow.
🪄 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: 86b4a6b4-6cc2-4d90-b768-72cfc9ff047f
📒 Files selected for processing (16)
CHANGELOG.md__tests__/e2e/helpers/payloads.ts__tests__/e2e/hooks/cursor-integration.e2e.test.ts__tests__/e2e/hooks/gemini-integration.e2e.test.ts__tests__/e2e/hooks/opencode-integration.e2e.test.ts__tests__/hooks/builtin-policies.test.ts__tests__/hooks/manager.test.ts__tests__/lib/log-entries.test.ts__tests__/lib/projects.test.tsapp/policies/hooks-client.tsxlib/log-entries.tslib/projects.tsscripts/codex-trace.mjssrc/hooks/builtin-policies.tssrc/hooks/handler.tssrc/hooks/integrations.ts
✅ Files skipped from review due to trivial changes (2)
- src/hooks/integrations.ts
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/codex-trace.mjs
- tests/e2e/hooks/opencode-integration.e2e.test.ts
- tests/lib/projects.test.ts
- tests/e2e/hooks/cursor-integration.e2e.test.ts
| it("policy in both cli[X].enabledPolicies and global: removes from CLI and adds to disabledPolicies", async () => { | ||
| const { readScopedHooksConfig, writeScopedHooksConfig } = await import("../../src/hooks/hooks-config"); | ||
| vi.mocked(readScopedHooksConfig).mockReturnValue({ | ||
| enabledPolicies: ["block-sudo", "block-rm-rf"], | ||
| cli: { gemini: { enabledPolicies: ["block-rm-rf"] } }, | ||
| }); | ||
|
|
||
| const { removeHooks } = await import("../../src/hooks/manager"); | ||
| await removeHooks( | ||
| ["block-rm-rf"], | ||
| "user", | ||
| undefined, | ||
| { cliExplicit: true, integration: ["gemini"] }, | ||
| ); | ||
|
|
||
| const [writtenConfig] = vi.mocked(writeScopedHooksConfig).mock.calls[0]; | ||
| // Per the code: inCliEnabled is checked first, so it's removed from enabledPolicies | ||
| // The global still has it; a separate run would be needed to suppress that | ||
| expect(writtenConfig.cli?.["gemini"]?.enabledPolicies ?? []).not.toContain("block-rm-rf"); | ||
| }); |
There was a problem hiding this comment.
Test name and inline comment contradict each other.
The test title (Line 1156) claims behaviour "removes from CLI and adds to disabledPolicies", but the inline comment at Lines 1172–1173 explicitly states the policy is only removed from enabledPolicies and the global is not suppressed in this run. The assertion (Line 1174) only verifies the removal from cli.gemini.enabledPolicies. Either:
- the test should also assert the policy is not added to
cli.gemini.disabledPolicies(matching the documented behaviour), and the title should be reworded (e.g. "removes from CLI enabledPolicies first; global suppression requires a separate run"), or - if the intended behaviour is that both happen, add the missing assertion on
disabledPoliciesand fix the implementation accordingly.
As-is, a regression in either direction would not be caught.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/hooks/manager.test.ts` around lines 1156 - 1175, The test title
contradicts its inline comment and assertion: update the test for removeHooks to
accurately reflect current behavior by renaming the it(...) description to
something like "removes from CLI enabledPolicies; global suppression requires a
separate run" and add an assertion that cli?.["gemini"]?.disabledPolicies does
NOT contain "block-rm-rf" (using the same writtenConfig from
writeScopedHooksConfig call) so the test verifies removal from enabledPolicies
only; also update the inline comment to match this expectation. Reference
symbols: removeHooks, readScopedHooksConfig, writeScopedHooksConfig,
cli.gemini.enabledPolicies, cli.gemini.disabledPolicies.
| const ENV_FILE_PATH_RE = /(?:^|[\\/])(?:\.env(?!\w)|env_\w*)/i; | ||
| const ENV_CMD_RE = /(?:\.env(?!\w)|env_\w*)/i; |
There was a problem hiding this comment.
env_\w* is broad enough to block ordinary files.
These patterns now match names like src/env_utils.ts or cat env_config, so block-env-files will start denying unrelated project files instead of just dotenv-style secrets. Narrow this back to actual env-file conventions.
🤖 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 205 - 206, The current regexes
ENV_FILE_PATH_RE and ENV_CMD_RE are too broad because `env_\w*` matches
arbitrary filenames like `env_config`; update both patterns to only match common
dotenv conventions (e.g. `.env`, `.env.local`, `.env.production`, or
`env.<suffix>`), for example replace `env_\w*` with a more specific alternation
such as `(?:\.env(?:\.[A-Za-z0-9_-]+)?|env\.[A-Za-z0-9_-]+)` so ENV_FILE_PATH_RE
remains anchored to path boundaries and ENV_CMD_RE matches the command/file
token form without capturing unrelated names.
| function looksLikeFailproofStopMessage(text: string): boolean { | ||
| return text.includes("MANDATORY ACTION REQUIRED from FailproofAI (policy:") | ||
| || text.includes("[FailproofAI Security Stop] Policy:"); | ||
| } |
There was a problem hiding this comment.
This helper opens a sanitizer bypass.
All five PostToolUse sanitizers now return allow() whenever the output merely contains one of these marker substrings. A tool can include [FailproofAI Security Stop] Policy: alongside a JWT/API key/private key and skip redaction completely. This needs an exact trusted match, not a substring check.
🤖 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 496 - 499, The helper
looksLikeFailproofStopMessage currently uses substring checks which allow
bypasses; change it to require an exact trusted marker match instead of includes
— update the function looksLikeFailproofStopMessage to compare the normalized
input against exact marker strings (or anchored regexes that match only the full
trusted markers such as "MANDATORY ACTION REQUIRED from FailproofAI (policy:"
and "[FailproofAI Security Stop] Policy:") rather than using text.includes, so
only an exact marker token (with any allowed canonical spacing/encoding) will
return true.
| // Block explicit save-and-run: wget/curl with an explicit -o/-O output flag downloading a .sh URL. | ||
| // Shell redirections (curl ... > file.sh) are intentionally allowed since those are typically for review. | ||
| if (REMOTE_SCRIPT_DOWNLOAD_RE.test(cmd) && /-[Oo]\b/.test(cmd)) { | ||
| return deny("Downloading remote shell scripts directly is prohibited. Save the file and review it before execution."); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find src/hooks -name "builtin-policies.ts" | head -5Repository: exospherehost/failproofai
Length of output: 99
🏁 Script executed:
head -c 1000 src/hooks/builtin-policies.ts | head -20Repository: exospherehost/failproofai
Length of output: 969
🏁 Script executed:
sed -n '600,650p' src/hooks/builtin-policies.tsRepository: exospherehost/failproofai
Length of output: 2226
🏁 Script executed:
grep -n "REMOTE_SCRIPT_DOWNLOAD_RE" src/hooks/builtin-policies.ts | head -20Repository: exospherehost/failproofai
Length of output: 273
🏁 Script executed:
sed -n '1,200p' src/hooks/builtin-policies.ts | grep -n "const.*RE\|const.*PATTERN"Repository: exospherehost/failproofai
Length of output: 1373
Long-form output flags bypass the remote-script guard.
The check /-[Oo]\b/ matches only short forms (-o or -O), allowing curl --output install.sh https://.../install.sh and wget --output-document=file.sh https://.../install.sh to bypass this policy despite downloading shell scripts to disk. These long-form flags perform the same save-and-run operation as the short forms this guard attempts to block.
🤖 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 631 - 634, The current guard in
the REMOTE_SCRIPT_DOWNLOAD_RE branch only detects short flags via /-[Oo]\b/,
letting long-form flags like --output or --output-document bypass the rule;
update the conditional in the same block (the if that references
REMOTE_SCRIPT_DOWNLOAD_RE and calls deny(...)) to also detect common long-form
save flags (e.g., --output, --output-document, --remote-name) by replacing the
short-flag regex with a broader, case-insensitive pattern that matches both
short and long forms (or add an additional test for those long flags) so
downloads using --output/--output-document/etc. are correctly denied.
Pre-integrate upstream additions before merge to avoid conflict markers crashing the bun hook process during git merge/rebase: - Add requireNoConflictsBeforeStop function and BUILTIN_POLICIES entry - Update test counts (31→32 policies, 4→5 workflow policies) - Add params schema test for require-no-conflicts-before-stop - Add CHANGELOG entry and bump version to 0.0.6-beta.6 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
debug.mjs and debug2.mjs were throwaway scripts with hardcoded local paths used during Gemini integration development. Not suitable for a production repo. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…session fix - Canonicalize tool names across CLIs (WriteFile→Write, run_terminal_command→Bash, etc.) so policies work cross-agent without per-CLI string matching - Export isBashTool helper from public API - Add per-CLI policy configuration UI with 3-state overrides (Inherit/ON/OFF) and per-CLI params - Add per-CLI --uninstall scoping and policyParams/customPoliciesPath overrides - Populate permissionMode in activity entries for all CLIs (Codex, Cursor, Copilot, Gemini) - Fix Pi SessionStart session ID: defer until real UUID is available from tool_call, avoiding fallback lock-in - Fix various test isolation issues and e2e flakes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ion attribution, suppress browser-extension hydration warnings
- protect-env-vars: catch printf, compgen -v, declare -p/x, and ${!var} indirect expansion bypass patterns; extend boundary chars to include quotes/backticks so bash -c "env" is blocked
- block-read-outside-cwd: add extractRelativeTraversals() so ../.. style paths are resolved and checked against CWD, not just absolute paths
- Pi integration: reset currentSessionId/pendingSessionStart/pendingPromptSubmit at session_start to prevent stale ID carry-over across sessions; add 60s recency filter in getSessionIdFromFile(); remove stale PI_SESSION_ID env var from foundId chain; defer UserPromptSubmit until tool_call provides the real session ID
- UI: add suppressHydrationWarning to Button base component and ActivityTab filter elements to silence Dashlane/password-manager attribute injection errors
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 participants
Summary
resolvePermissionModeto populate the Mode column in the dashboard for all CLIs — readspermission_modefrom Claude Code stdin,approval_policyfrom Codex session transcripts, and CLI flags via/procancestor walk for Cursor/Copilot/Gemini--integrationflag to--cliwith multi-value install support and interactive multi-select promptblock-sudonow coversrun_shell_command/shtool names (Gemini bypass),block-failproofai-commandsblocks agents readingpolicies-config.jsonTest plan
bun run test:run— all unit tests passbun run test:e2e— all e2e tests passbun run build— Next.js + dist/index.js build succeedsdefault/bypassPermissions) and Codex (full-auto/default)block-sudoblockssudoon Gemini viarun_shell_commandandshtool namesfailproofai policies --install --cli <name>🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes / Chores