Group gateway sessions by profile, surface convention policies, and close six silent-enforcement gaps - #622
Merged
Merged
Conversation
…m as a tree Hermes profiles are separate home directories with their own state.db and config.yaml, but both pillars hardcoded ~/.hermes — so non-default profiles were unaudited (absent from the dashboard) and unenforced (install wrote one config.yaml). Discovery now lives in lib/hermes-profiles.ts, shared by both pillars, and Integration grew an optional getSettingsPaths() so install covers every profile and reports installed only when all of them are hooked. OpenClaw now groups by agent as well as channel, fixing two agents on the same channel collapsing into one mixed row, and making the dashboard agree with the audit adapter. The projects panel renders user-scoped gateway CLIs as a collapsible tree (lib/project-tree.ts); cwd-based CLIs keep their flat rows. Legacy project links keep resolving. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…umn collision `.failproofai/policies/*policies.mjs` files are registered by the filesystem, never by policies-config.json, so the dashboard — which read only config — showed nothing while `failproofai policies` listed them with green ticks. Reported from a live gateway where four enforce-* policies were loading and enforcing but the configure view claimed none existed. get-hooks-config.ts now calls the same discoverPolicyFiles() the hook path uses, for project and user scope, grouped by declaring file. It parses rather than imports: manager.ts executes each file to list it, which would run arbitrary user code inside the long-lived dashboard server on every page load. Separately, nameColWidth is sized to the longest builtin name, and padEnd below the string length is a no-op — so a 42-char filename printed as `enforce-bengaluru-event-links-policies.mjs1 hook(s)` with no separator. Both covered by tests that fail without the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ailed Running `failproofai policies` from $HOME resolves both convention directories to the same path. The listing walked both, so every file printed twice — and the second pass showed "failed to load" for all of them, because the first pass had already imported the module and the ESM cache short-circuits customPolicies.add, so loadCustomHooks truthfully returned 0 hooks. Reported from a live install where four working policies all rendered with a red cross. The shared directory is now listed once and labelled "Project + User" so the collapse is visible rather than silent. The dashboard action already guarded this case; the CLI did not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dropping a file into .failproofai/policies/ wrote nothing anywhere, so an
operator with four working policies saw {"enabledPolicies": []} and concluded
discovery was broken. `failproofai policies` now mirrors what it lists into that
scope's config under a new conventionPolicies key, with the hook names the files
actually registered.
The record is descriptive, never authoritative — enforcement still discovers
from the filesystem and never reads the key. Pinned by a test asserting a policy
fires while the config lists a different, deleted file. An opt-in registry would
mean a freshly-copied policy doing nothing until some command refreshed it.
Written wholesale (deletions disappear), only on change, never when empty with
no existing config, and never from the hook path — a read-modify-write from
concurrent hook processes would corrupt the file governing enforcement.
Also fixes loading the same file twice in one process returning zero hooks: the
temp-file URL was deterministic so the second import hit the ESM module cache
and the body never re-ran. That was the real cause of the phantom
"failed to load". The specifier now carries a cache-busting query.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t config A project's .failproofai/policies-config.json is routinely committed — this repo tracks its own — so persisting the record there made a plain `failproofai policies` dirty the working tree and put a spurious diff in front of every contributor. A read command must not do that. User scope is where the record is wanted anyway; project discoveries are still listed, just not persisted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The icon renders in its own <td> and only the name cell carried the depth padding, so every child's folder icon sat at the same x as its parent's — a Hermes profile's channels read as a flat list with ragged text rather than a hierarchy. Both cells now take the same offset so icon and label move together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ruth Two defects found while auditing all 12 CLIs' hook contracts against upstream source. WorktreeCreate is not a permission gate on Claude — it is a worktree-PATH PROVIDER. Claude takes the stdout of the first hook that succeeds as the directory to create, and fails with "WorktreeCreate hook failed" when none supplies one. Our allow path writes nothing to stdout, correctly, by the contract every other event uses. So registering there broke `claude --worktree` and `/worktree` for every user, whatever any policy decided. Claude now installs from CLAUDE_INSTALL_EVENT_TYPES (canonical minus that event); writeHookEntries prunes our stale entry so reinstalling repairs an already-broken machine, while leaving anyone else's hooks on that event alone. No builtin matches the event, so nothing is lost. The integration suite decided PASS from our own hooks.log before checking whether the CLI ran the command anyway — so a silent allow (deny logged, command executed) scored PASS with the proof sitting unread on disk. That is the exact failure the suite exists to catch. Both probes now check ground truth first. Also records matchedPolicies on each activity row: policyName only names the policy that decided, so a plain allow wrote null and a row could not distinguish "your policy ran and allowed" from "nothing covers this event". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ctivity log A deny explains itself in the log; an observation-only event explained nothing — the row looked the same whether a policy fired and was ignored or no policy covered the event. That is what made a customer's Hermes guardrails look broken while they were running fine. Adds src/hooks/enforcement-capability.ts: per CLI, per canonical event, whether a deny is read at a call site that changes control flow. Established by tracing all 12 CLIs against upstream source, vendor contracts, or recorded live probes — 41 block cells, 64 observe, each carrying its evidence inline. The expanded activity row now names the policies that ran, states that the CLI discards verdicts for that event, and points at an event it does enforce on. An absent entry means unverified and renders nothing. A hedge is still a claim, and the previous home for this table was CLAUDE.md prose, which drifted into documenting Hermes subagent_stop as a working gate while upstream discarded its return value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each of these ran the policy, produced a verdict, recorded decision:"deny" and
counted it in telemetry — while the CLI submitted the prompt or ran the command
anyway, because the shape we emitted matched nothing it reads. All five CLIs can
block on these events; the gap was ours. That makes them worse than a missing
capability: the dashboard reported protection that did not exist.
copilot UserPromptSubmit exit 2 + stderr -> {decision:"block",reason} exit 0
copilot PermissionRequest nested decision -> flat {behavior,message}
cursor UserPromptSubmit {permission:...} -> {continue:false,user_message}
pi input {block:true} -> {action:"handled"}
pi user_bash {block:true} -> {result:BashResult}
Pi's shapes were verified against the installed package's own .d.ts:
InputEventResult and UserBashEventResult have no `block` field at all.
Cursor's tool events still use the flat permission shape, which they genuinely
read; a regression test pins that the new branch does not swallow them.
Capability matrix cells move from observe to block with the fix recorded inline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Hermes has no turn-end Stop event" stood in types.ts, policy-evaluator.ts and
CLAUDE.md, and a test asserted it. It was never checked against upstream and it
was wrong — so the 5 require-*-before-stop builtins were dead on Hermes.
pre_verify fires once per turn when the agent has edited code and is about to
finish (conversation_loop.py:6754). Upstream's parser accepts our Claude Stop
shape verbatim — {decision:"block",reason} means "block the stop", i.e. keep
going — and injects the reason as a synthetic user message before re-entering
the loop. The evaluator now emits the MANDATORY-ACTION wording there, since that
text is an instruction the model acts on, not a refusal notice.
No Python: pre_verify is in upstream's VALID_HOOKS and shell hooks register into
the same registry plugins use, so this is one more config.yaml entry.
Conditions documented rather than hidden: fires only on turns landing
write_file/patch, capped at 3 nudges per turn, and silently warn-and-skipped on
Hermes older than ~2026-06-30.
Also corrects two neighbouring claims in the same table: subagent_stop is NOT a
gate (parser is event-gated, call site discards the return — customer policies
there had zero enforcement), and Hermes DOES have an additional-context channel
(pre_llm_call, which we still do not install).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
readScopedHooksConfig fails soft, returning {enabledPolicies: []} on a syntax
error — right for the hook path, fatal for a writer. syncConventionPolicies
wrote that empty default straight back, so a hand-edited config with one stray
comma lost every enabled policy the moment someone ran `failproofai policies`, a
read-only command.
It now re-parses the file itself before writing and bails with a warning naming
the path, leaving the file byte-for-byte untouched.
Found by running the case, not by reading the code. Two tests pin it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Product decision: we do not install pre_verify. Back to five Hermes hooks, no canonical Stop event, and the 5 require-*-before-stop builtins inapplicable there — but now by choice rather than by a mistaken belief about the platform. Deliberately NOT reverted, because they are true regardless and were wrong before: subagent_stop is not a gate (upstream's parser is event-gated and the call site discards the return, so customer policies denying there had zero enforcement), and Hermes does have an additional-context channel (pre_llm_call). What pre_verify would buy, and the three upstream conditions it carries, are recorded in types.ts so the decision can be revisited without re-deriving it. The capability matrix leaves hermes Stop unkeyed — absent means the event never fires for us, so the UI stays silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects an adversarial review of this branch confirmed; two are
regressions the branch introduced.
Pi's tool gate was turned OFF. The commit correcting five inert deny shapes put
Pi's two returns in each other's handlers — both build the same
hook_event_name:"PreToolUse" payload, since user_bash maps there too, so one
search-and-replace hit the wrong one. tool_call (every tool the model calls)
returned {result:BashResult}, which ToolCallEventResult has no field for, so the
tool ran; user_bash kept {block:true}, which UserBashEventResult has no field
for. Strictly worse than the bug being fixed, and enforcement-capability.ts
asserted both were fine so no UI caveat covered it. A test now reads each
handler's source and fails if they swap again.
claude --worktree was still broken in this repo: WorktreeCreate was dropped from
CLAUDE_INSTALL_EVENT_TYPES but left in the committed .claude/settings.json, and
the dogfood tripwire asserted a bare count, which a swap leaves unchanged. It
now also asserts no registered event lies outside CLAUDE_INSTALL_EVENT_TYPES.
Project-scope policies were invisible in the shipped dashboard: the action
resolved from process.cwd(), but .next/standalone/server.js chdir()s into the
package on its first line, and it did not walk up to the .failproofai marker the
way enforcement does. The launcher forwards the pre-chdir cwd and both resolvers
go through findProjectConfigDir.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change updates hook enforcement, Hermes and OpenClaw project discovery, convention-policy synchronization, and gateway project presentation with expanded regression coverage. ChangesHook enforcement and verdict handling
Gateway projects and dashboard presentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/manager.ts (1)
265-290: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMulti-path install: a mid-loop failure leaves earlier profiles written but reports nothing about them.
With Hermes,
settingsPathshas one entry per profile. If path 2 throws (e.g.EACCESon one profile home), the already-written path 1 is inwrittenSettingsPathsbut the rethrow at Line 288 skips the reporting block at Line 334, so the user is told nothing about the partial install and cannot tell which profiles are now hooked. Including the failing path in the error/telemetry (and listing what was written) makes this recoverable.🛠️ Sketch
const settingsPaths = settingsPathsFor(integration, scope, cwd); - try { - for (const settingsPath of settingsPaths) { + for (const settingsPath of settingsPaths) { + try { const settings = integration.readSettings(settingsPath); integration.writeHookEntries(settings, binaryPath, scope); integration.writeSettings(settingsPath, settings); writtenSettingsPaths.push({ cli: cliId, path: settingsPath }); - } - } catch (err) { + } catch (err) { + if (writtenSettingsPaths.length > 0) { + console.error( + `Partially installed before failing at ${settingsPath}:\n` + + writtenSettingsPaths.map((w) => ` ${w.cli}: ${w.path}`).join("\n"), + ); + } + // ...existing errorType classification + telemetry, plus settings_path + throw err; + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/manager.ts` around lines 265 - 290, Update the multi-path write flow around settingsPathsFor and writtenSettingsPaths so mid-loop failures preserve and report partial progress: track the settingsPath currently being processed, include that failing path and the already-written paths in the error and hook_write_failed telemetry, and ensure the failure reporting is emitted before rethrowing. Keep successful multi-path installs unchanged.
🧹 Nitpick comments (7)
src/hooks/enforcement-capability.ts (1)
1-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated header prose. The JSDoc block (1-15) and the line-comment block (16-33) restate the same premise; consider keeping the JSDoc and reducing 16-33 to the legend plus probed versions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/enforcement-capability.ts` around lines 1 - 33, Remove the duplicated premise from the line-comment block in enforcement-capability.ts, preserving the JSDoc explanation above it. Retain only the block/observe/ABSENT legend and the probed-version information needed to document the table.__tests__/actions/get-hooks-config.test.ts (1)
194-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStub
USERPROFILEalongsideHOMEfor consistency. The sibling test stubs both;homedir()resolves fromUSERPROFILEon Windows, so this case wouldn't isolate there.🧪 Proposed tweak
vi.stubEnv("HOME", tmp); + vi.stubEnv("USERPROFILE", tmp);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/actions/get-hooks-config.test.ts` around lines 194 - 198, Update the “does not list the same file twice when cwd is the home directory” test to stub USERPROFILE to tmp alongside HOME before invoking the code under test, matching the sibling test’s environment setup and ensuring homedir() resolves to the temporary directory on Windows.app/policies/hooks-client.tsx (1)
1473-1473: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate total-policy arithmetic. The same
policies + custom + convention.reduce(...)expression now exists here and at Lines 1744-1747; a future counter change has to land in both. Extract acountPolicies(cfg: HooksConfigPayload)helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/policies/hooks-client.tsx` at line 1473, Extract the repeated total-policy calculation into a countPolicies helper accepting HooksConfigPayload, and replace the inline arithmetic at this location and the corresponding usage near the other occurrence with calls to that helper. Preserve the existing handling of optional customPolicies and conventionPolicies arrays.app/actions/get-hooks-config.ts (1)
80-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
conventionPoliciesis required on the payload but consumed optionally.app/policies/hooks-client.tsxreadsconfig.conventionPolicies?.…in three places. Either mark the field optional or drop the?.so the contract reads one way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/actions/get-hooks-config.ts` around lines 80 - 81, Align the conventionPolicies payload contract with its consumers: update the conventionPolicies field declaration to be optional if hooks-client.tsx intentionally supports its absence, or remove optional chaining from all three config.conventionPolicies accesses if the field must always be present. Keep the type and consumption behavior consistent.__tests__/hooks/list-convention-column.test.ts (1)
87-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe ANSI strip leaves the ESC bytes behind.
/\[[0-9;]*m/gdrops[32mbut not the preceding\x1B, soindexOfis offset by one byte per escape sequence. It balances out only because both rows currently carry the same number of sequences — a status marker that differs between rows (e.g.✗in red) would break the alignment assertion for the wrong reason.♻️ Proposed tweak
- return row.replace(/\[[0-9;]*m/g, "").indexOf("1 hook(s)"); + return row.replace(/\x1B\[[0-9;]*m/g, "").indexOf("1 hook(s)");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/hooks/list-convention-column.test.ts` around lines 87 - 90, Update the ANSI cleanup in the columnOf helper to remove each escape sequence together with its leading ESC byte, ensuring indexOf("1 hook(s)") is based on visible text regardless of differing status colors or markers.src/hooks/policy-evaluator.ts (1)
442-475: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDeny side is fixed for Copilot; the instruct side for the same two events still uses the Claude nested shape.
The comment establishes that Copilot normalizes
hookSpecificOutput-nested payloads to{}onPermissionRequest, and thatuserPromptSubmitonly reads{decision:"block"}. Aninstructverdict on those two events still falls through to the generichookSpecificOutput.additionalContextresponse (Line 999), so the instruction is likely dropped the same way the deny was — the same silent-non-enforcement class this PR closes. Worth confirming against the same bundle offsets you used for the deny shapes and, if it is inert, adding a Copilot instruct branch (e.g.{decision:"continue"}/additionalContextat the top level, whatever the normalizer accepts).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/policy-evaluator.ts` around lines 442 - 475, Add Copilot-specific instruct handling alongside the existing deny branches in the policy evaluator for UserPromptSubmit and PermissionRequest. Use the flat payload shapes accepted by Copilot’s normalizer, with the appropriate continue/instruction decision and top-level context, instead of allowing these events to fall through to the generic nested hookSpecificOutput response; preserve the existing deny behavior and metadata.app/components/project-list.tsx (1)
141-152: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueA folder row that also maps to a real project renders no link.
isFolderwins the branch, so if a project's synthetic path is a strict prefix of another's (e.g. a 2‑segmentopenclaw:<channel>alongsideopenclaw:<channel>:<x>),node.projectis set but the row only renders the toggle — that project becomes unreachable from the panel. Today's producers emit uniform 3‑segment paths, so this is latent; consider rendering the label as a link and keeping the chevron for that case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/project-list.tsx` around lines 141 - 152, Update the project-list row branching so a folder node with an assigned node.project renders the project label through the existing Link while retaining its folder toggle/chevron. Keep folder-only rows using the current non-link behavior, and preserve the existing projectDisplayName logic for filesystem projects and segment labels for gateway nodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/actions/get-hooks-config.test.ts`:
- Around line 138-142: Update the test suite setup for getHooksConfigAction to
clear FAILPROOFAI_LAUNCH_CWD in beforeEach using
vi.stubEnv("FAILPROOFAI_LAUNCH_CWD", ""), ensuring discoverConventionPolicies
uses the mocked process.cwd() and tests are isolated from ambient environment
values.
In `@app/actions/get-hooks-config.ts`:
- Around line 113-124: Update getHooksConfigAction and its policy discovery flow
to isolate filesystem failures per directory and file: make discoverPolicyFiles
failures produce no files for that directory, and make
parseCustomPoliciesFromFile failures produce an empty policy list for that file
while preserving its scope, basename, and path entry. Ensure
getHooksConfigAction still resolves with partial results and an empty policy
listing instead of rejecting when a directory or policy file is unreadable,
deleted, or not a regular file.
In `@src/hooks/custom-hooks-loader.ts`:
- Around line 30-37: Update loadAllCustomHooks to deduplicate policy files
before dynamic imports by normalizing explicit, project-convention, and
user-convention paths to canonical absolute paths and tracking already-loaded
paths for the process. Preserve discovery order and ensure each file is imported
and registered only once, including when projectRoot resolves to $HOME.
In `@src/hooks/enforcement-capability.ts`:
- Around line 43-46: Remove the developer-specific absolute path from the
comments near the hook runner offsets, retaining only the Claude artifact
version as the identifier; leave the byte-offset and event-agnostic behavior
descriptions unchanged.
---
Outside diff comments:
In `@src/hooks/manager.ts`:
- Around line 265-290: Update the multi-path write flow around settingsPathsFor
and writtenSettingsPaths so mid-loop failures preserve and report partial
progress: track the settingsPath currently being processed, include that failing
path and the already-written paths in the error and hook_write_failed telemetry,
and ensure the failure reporting is emitted before rethrowing. Keep successful
multi-path installs unchanged.
---
Nitpick comments:
In `@__tests__/actions/get-hooks-config.test.ts`:
- Around line 194-198: Update the “does not list the same file twice when cwd is
the home directory” test to stub USERPROFILE to tmp alongside HOME before
invoking the code under test, matching the sibling test’s environment setup and
ensuring homedir() resolves to the temporary directory on Windows.
In `@__tests__/hooks/list-convention-column.test.ts`:
- Around line 87-90: Update the ANSI cleanup in the columnOf helper to remove
each escape sequence together with its leading ESC byte, ensuring indexOf("1
hook(s)") is based on visible text regardless of differing status colors or
markers.
In `@app/actions/get-hooks-config.ts`:
- Around line 80-81: Align the conventionPolicies payload contract with its
consumers: update the conventionPolicies field declaration to be optional if
hooks-client.tsx intentionally supports its absence, or remove optional chaining
from all three config.conventionPolicies accesses if the field must always be
present. Keep the type and consumption behavior consistent.
In `@app/components/project-list.tsx`:
- Around line 141-152: Update the project-list row branching so a folder node
with an assigned node.project renders the project label through the existing
Link while retaining its folder toggle/chevron. Keep folder-only rows using the
current non-link behavior, and preserve the existing projectDisplayName logic
for filesystem projects and segment labels for gateway nodes.
In `@app/policies/hooks-client.tsx`:
- Line 1473: Extract the repeated total-policy calculation into a countPolicies
helper accepting HooksConfigPayload, and replace the inline arithmetic at this
location and the corresponding usage near the other occurrence with calls to
that helper. Preserve the existing handling of optional customPolicies and
conventionPolicies arrays.
In `@src/hooks/enforcement-capability.ts`:
- Around line 1-33: Remove the duplicated premise from the line-comment block in
enforcement-capability.ts, preserving the JSDoc explanation above it. Retain
only the block/observe/ABSENT legend and the probed-version information needed
to document the table.
In `@src/hooks/policy-evaluator.ts`:
- Around line 442-475: Add Copilot-specific instruct handling alongside the
existing deny branches in the policy evaluator for UserPromptSubmit and
PermissionRequest. Use the flat payload shapes accepted by Copilot’s normalizer,
with the appropriate continue/instruction decision and top-level context,
instead of allowing these events to fall through to the generic nested
hookSpecificOutput response; preserve the existing deny behavior and metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fc44c350-57a3-4206-b025-3e636d5e1de8
📒 Files selected for processing (52)
.claude/settings.jsonCHANGELOG.mdCLAUDE.md__tests__/actions/get-hooks-config.test.ts__tests__/components/project-list.test.tsx__tests__/hooks/convention-policy-config-sync.test.ts__tests__/hooks/dogfood-configs.test.ts__tests__/hooks/enforcement-capability.test.ts__tests__/hooks/handler.test.ts__tests__/hooks/inert-deny-shapes.test.ts__tests__/hooks/integrations.test.ts__tests__/hooks/list-convention-column.test.ts__tests__/hooks/manager.test.ts__tests__/hooks/new-telemetry.test.ts__tests__/hooks/pi-shim-shapes.test.ts__tests__/integration-suite/verdict-ordering.test.ts__tests__/lib/hermes-multi-profile.test.ts__tests__/lib/hermes-profiles.test.ts__tests__/lib/hermes-sqlite.test.ts__tests__/lib/openclaw-projects.test.ts__tests__/lib/paths.test.ts__tests__/lib/project-tree.test.ts__tests__/lib/projects.test.tsapp/actions/get-hooks-config.tsapp/components/project-list.tsxapp/policies/hooks-client.tsxapp/project/[name]/page.tsxdocs/dashboard.mdxintegration-suite/probe-cli.shlib/hermes-profiles.tslib/hermes-projects.tslib/hermes-sessions.tslib/openclaw-projects.tslib/openclaw-sessions.tslib/paths.tslib/project-tree.tslib/projects.tspi-extension/index.tsscripts/launch.tssrc/audit/cli-adapters/hermes.tssrc/audit/cli-adapters/openclaw.tssrc/hooks/configure-wizard.tssrc/hooks/custom-hooks-loader.tssrc/hooks/enforcement-capability.tssrc/hooks/handler.tssrc/hooks/hook-activity-store.tssrc/hooks/hooks-config.tssrc/hooks/integrations.tssrc/hooks/manager.tssrc/hooks/policy-evaluator.tssrc/hooks/policy-types.tssrc/hooks/types.ts
💤 Files with no reviewable changes (1)
- .claude/settings.json
…Rabbit #622) Convention discovery loaded the user directory even when it resolved to the same path as the project directory — which happens whenever the project root IS the home directory, the normal setup for a gateway. Every file loaded twice and customPolicies.add (an unconditional push) registered each hook twice, so every policy fired twice per event. A counting policy double-counts and trips its ceiling at half the real number. Masked by the runtime rather than prevented, and hidden from both sides: Bun caches dynamic imports by resolved path and ignores the ?v= cache-buster, so the second import was a no-op in the shipped binary; Node honours the query and would have double-registered, and the suite runs under Node but never exercised the overlapping-directory case. Paths are now deduped explicitly, with a test that fails without it. Also from the review: one unreadable policy file no longer blanks the whole Configure Policies tab (readFile throws on EACCES or a raced delete despite the existence check, and both callers swallow it, hanging the tab on "Loading…"); convention tests stub FAILPROOFAI_LAUNCH_CWD so an ambient value cannot bypass the cwd spy; developer home paths removed from shipped source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng install Two regressions this branch introduced, both found by running the case rather than reading it. A policy file reachable by two discovery routes ran twice per event. a06ccb9 deduplicated the project and user convention directories against each other, but not against an explicit `customPoliciesPath` — and pointing that at a file inside `.failproofai/policies/` whose name also matches the convention is an ordinary setup, exactly what `failproofai policies -i -c .failproofai/policies/my-policies.mjs` produces. `loadAllCustomHooks` imported it once as the explicit path and again as a convention file; `customPolicies.add` is an unconditional push, so every hook in it registered twice and fired twice per event, doubling side effects, cost, and any counter the policy keeps. Reproduced against the published `dist/cli.mjs`: module body evaluated twice, policy function ran twice, activity row listing `custom/<name>` and `.failproofai-project/<name>` side by side. Node-only, and so invisible from both directions — Bun ignores the `?v=` cache-buster, so the duplicate import is a silent no-op in a `bin/failproofai.mjs` run. All three routes now dedupe through one set of resolved paths. A hand-edited `settings.json` could abort the whole install. The pruning loop walks every key in `settings.hooks`, including keys failproofai has never written — a newer Claude event, another tool's entry, a typo — so their values are unvalidated input, and a non-array one threw (`{} is not iterable`). The throw escapes `installHooks` AFTER the selected policies are recorded as enabled, so the user is told they are covered while settings.json received no hook at all: silent non-enforcement, the failure this pruning exists to remove. Verified end to end — 0 hooks written, against 28 on the same file with the key well-formed. Non-array values are now skipped, and a matcher group is dropped only when we emptied it, so foreign and malformed entries are written back exactly as found instead of quietly deleted. Both tests verified to fail with their fix reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KA4bni27KVRwaperZKDwNB
NiveditJain
approved these changes
Jul 29, 2026
NiveditJain
left a comment
NiveditJain
left a comment
Member
There was a problem hiding this comment.
Approving after a deep functional review of this branch.
Vendor contracts verified against shipped code, not docs. All three changed deny shapes check out: Cursor's beforeSubmitPrompt gates on false === W.continue and prints user_message (permission is never read there, so the old shape really was inert); Copilot's permissionRequest reads a flat e.behavior (app.js @2606293, 1.0.75); Pi's emitInput returns early on action:"handled" and emitUserBash displays-and-returns without executing on {result}, with BashResult fields matching exactly. Hermes _parse_response is event-gated to pre_tool_call + pre_verify only, confirming both the subagent_stop correction and the pre_verify note, and the profile model matches hermes_cli/profiles.py including the parent.name == "profiles" climb.
Two regressions found and fixed in 9cba1ca — a policy file reachable by two discovery routes running twice per event, and a malformed key in settings.json aborting the install after policies were already marked enabled. Both reproduced against the published dist/cli.mjs and both tests verified to fail with their fix reverted.
Left for follow-up (none blocking): matchedPolicies names policies that never ran, because evaluatePolicies short-circuits on the first deny; EnforcementNote applies a deny-only table to allow/instruct rows; CACHE_SCHEMA_VERSION wasn't bumped although both projectName shapes changed, so cached audit results keep the old names for up to 7 days; and the OpenClaw adapter's switch to index lastInteractionAt over file mtime can make an incremental --since audit skip an appended transcript.
coderabbitai
Bot
added
bug
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)
src/hooks/custom-hooks-loader.ts (1)
107-120: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse a unique temporary entry path per Bun load.
clearCustomHooks()clears the global registry before eachloadAllCustomHooks({ sessionCwd })call, butentryTmpis deterministic and Bun ignores the?v=cache-buster. A second call with the same config/context can reuse the cached module after clearing the registry and return an empty hook list. Make the rewritten entry path unique per invocation and add a Bun regression that callsloadAllCustomHookstwice with real policy setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/custom-hooks-loader.ts` around lines 107 - 120, The rewritten entry path used by the custom-hook loader must be unique for every load invocation because Bun ignores the query-string cache buster. Update the entry-temp creation flow around entryTmp and loadSequence so repeated loadAllCustomHooks calls cannot reuse Bun’s cached module after clearCustomHooks; add a regression test that performs two loadAllCustomHooks calls with real policy setup and verifies hooks are returned both times.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/hooks/custom-hooks-loader.ts`:
- Around line 107-120: The rewritten entry path used by the custom-hook loader
must be unique for every load invocation because Bun ignores the query-string
cache buster. Update the entry-temp creation flow around entryTmp and
loadSequence so repeated loadAllCustomHooks calls cannot reuse Bun’s cached
module after clearCustomHooks; add a regression test that performs two
loadAllCustomHooks calls with real policy setup and verifies hooks are returned
both times.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c26d29bc-3a77-466b-b2e4-265b4c18464b
📒 Files selected for processing (9)
CHANGELOG.md__tests__/actions/get-hooks-config.test.ts__tests__/hooks/claude-prune-malformed-settings.test.ts__tests__/hooks/convention-dir-dedup.test.ts__tests__/hooks/loader-path-dedup.test.tsapp/actions/get-hooks-config.tssrc/hooks/custom-hooks-loader.tssrc/hooks/enforcement-capability.tssrc/hooks/integrations.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/hooks/enforcement-capability.ts
- app/actions/get-hooks-config.ts
- CHANGELOG.md
- src/hooks/integrations.ts
This was referenced Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Started as "why aren't my client's Hermes policies firing?" and turned into an audit of what our hooks actually enforce on each CLI. Thirteen commits; the through-line is silent non-enforcement — a policy runs, the verdict is recorded, telemetry counts it, and the agent does the thing anyway.
Profile grouping and the projects tree
Hermes profiles are separate home directories with their own
state.dbandconfig.yaml, but both pillars hardcoded~/.hermes— so non-default profiles were unaudited and unenforced. Discovery now lives inlib/hermes-profiles.ts, shared by both, andIntegrationgrew an optionalgetSettingsPaths()so install covers every profile. OpenClaw groups by agent as well as channel. User-scoped gateway CLIs render as a collapsible tree, with the folder icon indenting alongside its label.Convention policies were invisible
.failproofai/policies/*policies.mjsfiles are registered by the filesystem, never by config — so a dashboard reading only config showed nothing whilefailproofai policieslisted them with green ticks. Reported from a live gateway where four working policies looked absent.get-hooks-config.tsnow discovers them (parsed, never imported — the CLI executes policy files to list them, which would run arbitrary user code in the long-lived dashboard server)policies-config.jsonrecords them under a newconventionPolicieskey, descriptive only — enforcement still discovers from the filesystem and never reads it, pinned by a test where a policy fires while the config lists a different, deleted file"Policy ran, but this CLI cannot enforce here"
A deny explains itself in the log; an observation-only event explained nothing — the row looked identical whether a policy fired and was ignored or none covered the event.
src/hooks/enforcement-capability.tsrecords, per CLI per canonical event, whether a deny reaches a call site that changes control flow: all 12 CLIs traced to upstream source, vendor contracts or recorded live probes (41block, 64observe), each line carrying its evidence.An absent entry means unverified and renders nothing. A hedge in the UI is still a claim, and this table's previous home was CLAUDE.md prose — which drifted into documenting Hermes
subagent_stopas a working gate while upstream discards its return.Six enforcement gaps closed
claude --worktreebroken for every userWorktreeCreateis a path provider — Claude uses the first hook's stdout as the directory. Our correct silence made it abort. Dropped from what we install;writeHookEntriesprunes stale entries so reinstalling repairs an existing machineUserPromptSubmit/PermissionRequest, CursorUserPromptSubmit, Piinput/user_bash— all sent shapes the CLI does not parsesyncConventionPolicieswrote a fail-soft empty default over an unparseable config, destroying every enabled policy from a read-only commandchdirs into the package on startup; the action resolved fromprocess.cwd()and never walked up to the.failproofaimarkerThe five deny shapes were inert. Anyone who wrote a deny policy on those events saw it do nothing; it will now block. No API change and nothing errors, but prompts and commands that previously went through will be refused. Worth a release note.
Everything else is additive or strictly protective. Hermes is unchanged — still five hooks,
pre_verifydeliberately not installed (its contract is documented intypes.tsso the decision can be revisited).Verification
2468 unit + 313 e2e, lint 0 errors,
tscclean, build passes. Every fix has a test verified to fail without it. Two adversarial review passes over this branch each caught regressions it had introduced — the Pi handlers had their returns swapped, turning off a gate that worked; then the two in the section below — all fixed here.Now verified: the three changed deny shapes were checked against the vendors' shipped code, not just their docs.
UserPromptSubmit→{continue:false, user_message}1931.index.js:if (W && false === W.continue)→ aborts submission and printsuser_message.permissionis never read on this event, confirming the old shape was inert.PermissionRequest→{behavior:"deny", message}app.js@2606293 (1.0.75): `if (t==="permissionRequest") { let n = e.behavior; if (n==="allow"input→{action:"handled"},user_bash→{result: BashResult}runner.jsemitInputreturns early onaction==="handled"andagent-session.jsskips submission;emitUserBash→interactive-mode.jsif (eventResult?.result)displays and returns without executing.BashResultfields match exactly. The old{block:true}matched no branch in either — inert, as claimed.Also confirmed against upstream source: Hermes
_parse_responseis event-gated topre_tool_call+pre_verifyonly (so thesubagent_stopcorrection and thepre_verifynote are right), and the profile layout matcheshermes_cli/profiles.pyincluding theparent.name == "profiles"climb.Still not verified live: Copilot's
UserPromptSubmitconsumer moved into a Rustruntime.nodein 1.0.71+, so only the emitted shape could be checked, not the call site.Follow-up: two regressions this branch had introduced
Both found by running the case rather than reading it, and both fixed in
9cba1ca.A policy file reachable by two discovery routes ran twice per event.
a06ccb9deduped the project and user convention directories against each other, but not against an explicitcustomPoliciesPath— and pointing that at a file inside.failproofai/policies/whose name also matches the convention is an ordinary setup, exactly whatfailproofai policies -i -c .failproofai/policies/my-policies.mjsproduces. SincecustomPolicies.addis an unconditional push, every hook in the file registered twice and fired twice per event. Reproduced against the publisheddist/cli.mjs: module body evaluated twice, policy function ran twice, activity row listingcustom/<name>and.failproofai-project/<name>side by side. Node-only, so invisible from both directions — Bun ignores the?v=cache-buster, making the duplicate import a silent no-op there. All three routes now dedupe through one set of resolved paths.A hand-edited
settings.jsoncould abort the whole install. The pruning loop walks every key insettings.hooks, including keys failproofai has never written — a newer Claude event, another tool's entry, a typo — so their values are unvalidated input, and a non-array one threw ({} is not iterable). The throw escapesinstallHooksafter the selected policies are recorded as enabled, so the user is told they are covered while settings.json received no hook at all: silent non-enforcement, the failure this pruning exists to remove. Verified end to end — 0 hooks written, against 28 on the same file with the key well-formed. Non-array values are now skipped, and a matcher group is dropped only when we emptied it, so foreign and malformed entries are written back exactly as found instead of quietly deleted.Known and deliberately left for a follow-up:
matchedPolicieslists every policy that matched, butevaluatePoliciesshort-circuits on the first deny, so on a deny row it names policies that never executed.🤖 Generated with Claude Code
Summary by CodeRabbit