Conversation
📝 WalkthroughWalkthroughAdds support for multiple explicit custom policy files through repeated CLI flags and ordered ChangesCustom policy management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as policies CLI
participant Installer as installHooks
participant Config as hooks config
participant HookEvent as handleHookEvent
participant Loader as loadAllCustomHooks
participant Dashboard as Policies dashboard
CLI->>Installer: collect repeated --custom paths
Installer->>Config: store ordered customPoliciesPaths
HookEvent->>Config: read paths and disabled IDs
HookEvent->>Loader: load and identify policy hooks
Loader-->>HookEvent: return eligible hooks
Dashboard->>Config: persist toggle state
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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: 5
🧹 Nitpick comments (2)
app/actions/get-hooks-config.ts (1)
200-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate custom-policy-paths fallback logic across two files. Both sites independently compute
config.customPoliciesPaths ?? (config.customPoliciesPath ? [config.customPoliciesPath] : [])to normalize legacy/plural custom policy paths from aHooksConfig.
app/actions/get-hooks-config.ts#L200-L214: extract this fallback into a shared exported helper (e.g.resolveCustomPoliciesPaths(config)insrc/hooks/hooks-config.ts) and call it here.src/hooks/manager.ts#L700-L721: use the same shared helper instead of re-derivingexplicitPathsinline.🤖 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 200 - 214, The custom-policy path normalization is duplicated across the hooks configuration loader and manager. Add and export a shared resolveCustomPoliciesPaths helper in src/hooks/hooks-config.ts, then update app/actions/get-hooks-config.ts lines 200-214 and src/hooks/manager.ts lines 700-721 to use it instead of their inline fallback or explicitPaths derivation.__tests__/e2e/hooks/custom-hooks.e2e.test.ts (1)
19-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the first policy observable.
The first file only returns
allow()with no visible effect. A regression that loads onlysecondPathstill passes, so this does not verify that both files—or their order—are honored. Make the second policy depend on an observable effect from the first, or add assertions that distinguish both registrations.🤖 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__/e2e/hooks/custom-hooks.e2e.test.ts` around lines 19 - 32, The test “loads multiple explicit custom policy files” does not verify that firstPath is loaded or ordered before secondPath because the first policy’s allow result is unobservable. Update the fixture policies and assertions so the second policy’s behavior depends on an observable effect from the first, or otherwise assert distinct evidence that both registrations execute in the expected order.
🤖 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 `@CHANGELOG.md`:
- Line 6: Update the single-line feature entry in CHANGELOG.md to append the
actual pull request number in parentheses, preserving the existing description
and single-line format.
In `@docs/custom-policies.mdx`:
- Around line 75-80: Update the custom-policy documentation examples and
accompanying text to use plural terminology for multiple explicit entries:
change “the custom policies path” to “custom policies paths” and “file” to
“files” around the --custom uninstall and explicit --custom descriptions, while
preserving the existing command behavior and ordering explanation.
In `@src/hooks/custom-hooks-loader.ts`:
- Around line 233-242: Normalize every custom policy path with resolve before
checking loadedPaths or comparing against convention paths, including paths
already marked absolute. Update the path construction in the custom policy
loading loop, then use that normalized value consistently for existsSync,
deduplication, and loadSingleFile.
- Around line 231-242: Fix the per-event loading flow around loadSingleFile so
Bun reliably republishes custom hooks after policy-registry is cleared, without
relying on the ?v= cache-busting query or loadSequence; alternatively, remove
the fresh-loading guarantee and update the documented contract. Apply the loader
change at src/hooks/custom-hooks-loader.ts:231-242 and align the corresponding
statement at docs/custom-policies.mdx:82 with the implemented behavior.
In `@src/hooks/manager.ts`:
- Around line 215-260: Update the custom policy path condition in the
surrounding configuration-update flow so an empty customPoliciesPath array is
treated as absent rather than entering the write and validation branch. Preserve
processing for non-empty arrays and string values, while avoiding deletion of
the legacy field or writing an empty customPoliciesPaths value when no paths are
supplied.
---
Nitpick comments:
In `@__tests__/e2e/hooks/custom-hooks.e2e.test.ts`:
- Around line 19-32: The test “loads multiple explicit custom policy files” does
not verify that firstPath is loaded or ordered before secondPath because the
first policy’s allow result is unobservable. Update the fixture policies and
assertions so the second policy’s behavior depends on an observable effect from
the first, or otherwise assert distinct evidence that both registrations execute
in the expected order.
In `@app/actions/get-hooks-config.ts`:
- Around line 200-214: The custom-policy path normalization is duplicated across
the hooks configuration loader and manager. Add and export a shared
resolveCustomPoliciesPaths helper in src/hooks/hooks-config.ts, then update
app/actions/get-hooks-config.ts lines 200-214 and src/hooks/manager.ts lines
700-721 to use it instead of their inline fallback or explicitPaths derivation.
🪄 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: 79975120-a4ec-49f8-ba89-b4d12aa657a9
📒 Files selected for processing (13)
CHANGELOG.md__tests__/e2e/hooks/custom-hooks.e2e.test.ts__tests__/hooks/hooks-config.test.ts__tests__/hooks/manager.test.tsapp/actions/get-hooks-config.tsapp/policies/hooks-client.tsxbin/failproofai.mjsdocs/custom-policies.mdxsrc/hooks/custom-hooks-loader.tssrc/hooks/handler.tssrc/hooks/hooks-config.tssrc/hooks/manager.tssrc/hooks/policy-types.ts
Installing with --custom replaced the configured set, so `-c a` then `-c b` left only `b` — and nothing was printed to say `a` had stopped applying. The policies just went inert. Repeated flags in ONE command worked; across commands the earlier ones vanished, which is how a user hit it within minutes. Now additive, mirroring enabledPolicies directly above it in the same function: union with what is configured, deduped, order preserved. `replace` (passed by the configure wizard) still makes the given set authoritative, exactly as it does for enabled policies — so the wizard can still remove, and `policies --uninstall -c` still clears everything. Carried-over paths are filtered by existence first: a file deleted after it was configured must not make every future install exit(1) on the strict validation below. Only paths added by THIS invocation are validated; earlier ones were validated when they were added. Four tests, verified failing without the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
__tests__/e2e/hooks/custom-hooks.e2e.test.ts (1)
19-80: 🩺 Stability & Availability | 🔵 TrivialComplete required clean-container and full-CI validation.
The PR summary says the Docker validation stalled and repository-wide failures remain. Re-run the required clean-container E2E validation and ensure all required CI jobs pass before pushing. As per coding guidelines, “After every push, wait for all CI jobs to finish and fix any failure” and “Validate every non-trivial change end-to-end in a clean Docker container that mimics a real installation.”
Also applies to: 461-498
🤖 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__/e2e/hooks/custom-hooks.e2e.test.ts` around lines 19 - 80, Complete the required clean-container end-to-end validation for the custom policy scenarios covered by the tests around runHook, then rerun the full CI suite and resolve every repository-wide failure before pushing. After each push, wait for all CI jobs to finish and verify they pass.Source: Coding guidelines
🤖 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 `@app/actions/get-hooks-config.ts`:
- Line 180: Align dashboard policy state with runtime scope merging: in
app/actions/get-hooks-config.ts:180, derive enabled/disabled status from the
merged configuration used by src/hooks/handler.ts rather than a single raw
config. In app/actions/update-hooks-config.ts:32-42, add scope-aware mutation
and provenance handling so enabling a policy removes the effective disable
across the relevant scope(s), not merely from the selected config copy.
- Around line 213-219: Update the per-path processing in the
parsedCustomPolicies Promise.all within PoliciesTab configuration loading so
errors from parseCustomPoliciesFromFile are caught for each explicit policy path
and that path returns an empty policy list. Preserve successful parsing, policy
ID generation, and disabled-state handling, allowing other files to load and
customPolicies to be flattened normally.
---
Nitpick comments:
In `@__tests__/e2e/hooks/custom-hooks.e2e.test.ts`:
- Around line 19-80: Complete the required clean-container end-to-end validation
for the custom policy scenarios covered by the tests around runHook, then rerun
the full CI suite and resolve every repository-wide failure before pushing.
After each push, wait for all CI jobs to finish and verify they pass.
🪄 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: 2682531b-442f-480c-a792-011ca6ba0a0a
📒 Files selected for processing (14)
CHANGELOG.md__tests__/actions/get-hooks-config.test.ts__tests__/actions/update-hooks-config.test.ts__tests__/e2e/hooks/custom-hooks.e2e.test.ts__tests__/hooks/hooks-config.test.tsapp/actions/get-hooks-config.tsapp/actions/update-hooks-config.tsapp/policies/hooks-client.tsxdocs/configuration.mdxdocs/custom-policies.mdxsrc/hooks/custom-hooks-loader.tssrc/hooks/handler.tssrc/hooks/hooks-config.tssrc/hooks/policy-types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGELOG.md
- src/hooks/hooks-config.ts
- src/hooks/policy-types.ts
- docs/custom-policies.mdx
nk-ag
previously approved these changes
Jul 29, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/actions/get-hooks-config.ts`:
- Around line 213-228: Deduplicate resolved custom policy paths before parsing
in the configuration flow around configuredCustomPolicyPaths and
resolvedCustomPaths. Use a Set-based approach that canonicalizes equivalent
paths and preserves first-seen order, so each file is parsed once while existing
policy ID and disabled-state handling remains unchanged. Add a regression test
covering equivalent entries such as rules.mjs and ./rules.mjs.
🪄 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: 10a86ec5-7c47-4d10-ab45-e89dd01b72df
📒 Files selected for processing (11)
CHANGELOG.md__tests__/actions/get-hooks-config.test.ts__tests__/actions/update-hooks-config.test.ts__tests__/e2e/hooks/custom-hooks.e2e.test.ts__tests__/hooks/list-convention-column.test.ts__tests__/hooks/manager.test.tsapp/actions/get-hooks-config.tsapp/actions/update-hooks-config.tsapp/policies/hooks-client.tsxsrc/hooks/hooks-config.tssrc/hooks/manager.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/actions/update-hooks-config.test.ts
- app/actions/update-hooks-config.ts
- tests/e2e/hooks/custom-hooks.e2e.test.ts
- tests/hooks/manager.test.ts
- app/policies/hooks-client.tsx
- src/hooks/manager.ts
nk-ag
approved these changes
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
customPoliciesPathsconfiguration while retaining legacycustomPoliciesPathsupport--customflags and load, list, remove, and display all configured filesToggle edge-case coverage
Validation
Summary by CodeRabbit
--customsupport for multiple explicit custom policy files, with correct install/uninstall and load order.disabledCustomPolicies.customPoliciesPaths(array) with legacycustomPoliciesPathretained.Latest CLI status fix
failproofai policiesnow renders dashboard-disabled explicit policies asOFFwithout a green checkmarkOFFwhen all policies are disabled andMIXEDwhen enabled and disabled siblings coexist; disabled names are labeled(OFF)