feat: add WHAT/FIX/REF remediation format to all denial messages#44
feat: add WHAT/FIX/REF remediation format to all denial messages#44
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughStandardizes hook denial/error messages into a WHAT/FIX/REF remediation format across hooks and updates repository metadata: punctuation and test totals in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
| Filename | Overview |
|---|---|
| hooks/src/gitcheck.rs | All 8 FR-GS denial messages and 3 worktree-enforcement messages reformatted to WHAT/FIX/REF; no logic changes, tests only assert on GitResult::Block(_) variant not message content |
| hooks/src/sandbox.rs | System-path denials (FR-PS-1) and FR-WE-1/WE-2 fallback reformatted to WHAT/FIX/REF; exempt REDIRECT config-path message correctly left unchanged; WORKTREE_MISSING prefix preserved |
| hooks/src/lib.rs | worktree_missing_msg updated to WHAT/FIX/REF format while preserving the WORKTREE_MISSING:{repo} sentinel prefix that integration tests and lazy-worktree logic depend on |
| CLAUDE.md | Test count synced to 246 with detailed per-category breakdown; rusqlite added to dependency description |
| .coderabbit.yaml | New CodeRabbit reviewer config encoding the same invariants as the greptile.json custom rules |
| greptile.json | New Greptile reviewer config with six custom rules; correctly self-excluded from review via ignorePatterns |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Claude Code tool call] --> B{permissions hook}
B --> C{File write?}
B --> D{Bash command?}
C --> E[sandbox::check_path_with_context]
E --> F{System path?}
F -->|yes| G["Deny — WHAT/FIX/REF\nREF: architecture.md#forbidden-dependencies"]
F -->|no| H{Worktree active?}
H -->|config persist path| I["Deny — REDIRECT: config path must persist\n(exempt from WHAT/FIX/REF)"]
H -->|main checkout| J["Deny — WHAT/FIX/REF\nREF: architecture.md#key-invariants"]
H -->|no worktree| K["Deny — WORKTREE_MISSING:repo + WHAT/FIX/REF"]
D --> L[gitcheck::check_git_safety]
L -->|FR-GS-1..8 matched| M["Block — WHAT/FIX/REF\nREF: CLAUDE.md#…"]
L -->|ok| N[gitcheck::check_worktree_enforcement]
N -->|main checkout targeted| O["Deny — WHAT/FIX/REF\nREF: architecture.md#key-invariants"]
N -->|ok| P[Allow]
Reviews (7): Last reviewed commit: "fix(sandbox): migrate FR-PS-1 system pat..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (2)
hooks/src/sandbox.rs (1)
133-136: Inconsistency: Persistent config redirect message still uses "REDIRECT:" prefix.Lines 133-136 retain the old
REDIRECT:format while Lines 170-174 were updated toWHAT:/FIX:/REF:. If the PR intends to standardize on WHAT/FIX/REF for all denial messages, this block should also be updated.If the difference is intentional (REDIRECT for config paths vs WHAT for main checkout writes), the test at Line 849 correctly expects "REDIRECT" for config paths — but then Lines 170-174 shouldn't have been reached for config paths. Verify the test expectations align with the intended behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/sandbox.rs` around lines 133 - 136, The denial message in the PathDecision::Deny branch that returns the persistent config redirect currently uses the old "REDIRECT:" prefix while other denial messages were standardized to the "WHAT:/FIX:/REF:" format; update that return string (the PathDecision::Deny that composes the message with repo_prefix and after_id) to use the standardized "WHAT:/FIX:/REF:" fields (providing WHAT, FIX, and REF text consistent with other denials), and then update any tests that assert the old "REDIRECT" prefix (or, if the "REDIRECT" behavior is intentional, ensure the other standardized messages are adjusted instead so tests and behavior remain consistent).hooks/src/gitcheck.rs (1)
74-78: Consider: Message format inconsistency withpermissions.rs.This file now uses
WHAT:/FIX:/REF:format for denials, but per relevant snippets,permissions.rsstill emitsBLOCKED:prefixed messages (e.g., Lines 43-50, 203-211, 222-226). If agents parse denial output by prefix to categorize errors or trigger remediation, this inconsistency could cause issues.This may be intentional scope for this PR, but worth confirming whether
permissions.rsshould be updated in a follow-up to ensure consistent agent-parseable output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/gitcheck.rs` around lines 74 - 78, check_git_safety currently emits denial messages using the WHAT:/FIX:/REF: format while permissions.rs still emits BLOCKED:-prefixed messages, causing inconsistent, parse-unstable outputs; pick one canonical denial format and make the other match it (either update permissions.rs emitters that produce "BLOCKED:" to instead emit the WHAT:/FIX:/REF: structure, or change check_git_safety to emit "BLOCKED:"), updating all relevant message constructors/return paths in permissions.rs and the check_git_safety function so agents can consistently parse prefixes (search for the "BLOCKED:" literal and the check_git_safety function to locate and change the emitters).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/src/gitcheck.rs`:
- Around line 84-88: The denial message passed to GitResult::Block uses a
backslash-continued multi-line string that fails rustfmt; replace that with a
rustfmt-friendly form (for example a raw string literal r#"..."#, or use concat!
to join single-line literals) for the GitResult::Block call so the WHAT/FIX/REF
message is formatted as one clean string literal (preserve the exact text:
"WHAT: Force push without --force-with-lease. FIX: Use `git push
--force-with-lease origin <branch>` instead. REF:
CLAUDE.md#supply-chain-policy").
In `@hooks/src/sandbox.rs`:
- Around line 170-174: The failure is caused by changing the redirect message
format in PathDecision::Deny so it no longer contains "REDIRECT", breaking the
test_worktree_config_paths_redirected assertion; fix by either restoring the
"REDIRECT:" token in the deny message (update the PathDecision::Deny(...) format
string to include "REDIRECT: ..." or prepend "REDIRECT" alongside the new
"WHAT/FIX/REF" content) or by updating the test_worktree_config_paths_redirected
assertion to look for the new marker(s) (e.g., check for "WHAT" or the combined
"REDIRECT" + new format) so both the message in sandbox.rs and the test remain
consistent.
- Around line 166-174: The formatting of the multi-line format! call that builds
the deny message (around the wt_path and PathDecision::Deny usage in sandbox.rs)
is off; run rustfmt (cargo fmt) to reformat the file, or adjust the string
literal to a single properly formatted expression so that cargo fmt passes
(ensure the format! invocation wrapping the message and {wt_path} is formatted
according to rustfmt).
- Around line 197-201: The string literal passed to PathDecision::Deny in
sandbox.rs is not formatted to Rustfmt's rules; run `cargo fmt` to auto-fix or
replace the escaped multi-line literal with a properly formatted/raw string so
rustfmt can wrap it (e.g., use a raw string r#"..."# or concatenate short string
literals) and keep the return as PathDecision::Deny(... .into()) so the symbol
PathDecision::Deny and its message formatting are preserved.
---
Nitpick comments:
In `@hooks/src/gitcheck.rs`:
- Around line 74-78: check_git_safety currently emits denial messages using the
WHAT:/FIX:/REF: format while permissions.rs still emits BLOCKED:-prefixed
messages, causing inconsistent, parse-unstable outputs; pick one canonical
denial format and make the other match it (either update permissions.rs emitters
that produce "BLOCKED:" to instead emit the WHAT:/FIX:/REF: structure, or change
check_git_safety to emit "BLOCKED:"), updating all relevant message
constructors/return paths in permissions.rs and the check_git_safety function so
agents can consistently parse prefixes (search for the "BLOCKED:" literal and
the check_git_safety function to locate and change the emitters).
In `@hooks/src/sandbox.rs`:
- Around line 133-136: The denial message in the PathDecision::Deny branch that
returns the persistent config redirect currently uses the old "REDIRECT:" prefix
while other denial messages were standardized to the "WHAT:/FIX:/REF:" format;
update that return string (the PathDecision::Deny that composes the message with
repo_prefix and after_id) to use the standardized "WHAT:/FIX:/REF:" fields
(providing WHAT, FIX, and REF text consistent with other denials), and then
update any tests that assert the old "REDIRECT" prefix (or, if the "REDIRECT"
behavior is intentional, ensure the other standardized messages are adjusted
instead so tests and behavior remain consistent).
🪄 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
Run ID: 1299f601-104f-4166-96b7-44443b779260
📒 Files selected for processing (4)
CLAUDE.mdhooks/src/gitcheck.rshooks/src/lib.rshooks/src/sandbox.rs
Update test count to 231 (178 unit + 4 doc + 17 integration + 10 proptest + 22 memory). Mention rusqlite as memory crate dependency. Fix trailing period on H-4 purity bullet.
…ective 22) Restructure all hook denial messages to follow the harness engineering WHAT/FIX/REF format so the agent can self-repair without human intervention: - lib.rs: WORKTREE_MISSING now includes WHAT/FIX/REF - sandbox.rs: REDIRECT and fallback BLOCKED messages use WHAT/FIX/REF - gitcheck.rs: all 8 git safety blocks + worktree enforcement use WHAT/FIX/REF with pointers to CLAUDE.md and docs/architecture.md Reference: harness.md Pillar 2c — custom linters with remediation messages.
Greptile correctly identified the count (231) didn't match the actual cargo test output (246). Missing: claude_md (5), memory integration (4), separate proptest binary (10). Correct: 188 hooks unit + 5 claude_md + 17 integration + 10 proptest + 22 memory unit + 4 memory integration = 246.
frits-v
force-pushed
the
feat/remediation-messages
branch
2 times, most recently
from
4e09c2a to
5549c67
Compare
frits-v
force-pushed
the
feat/remediation-messages
branch
from
5549c67 to
90a258d
Compare
1 participant
Summary
lib.rs: WORKTREE_MISSING includes WHAT/FIX/REFsandbox.rs: REDIRECT and fallback BLOCKED messages use WHAT/FIX/REFgitcheck.rs: all 8 git safety blocks + worktree enforcement use WHAT/FIX/REFrusqliteas memory crate dependencyReference:
.agents/research/harness.mdPillar 2c — custom linters with remediation messages.Test plan
cargo test— all 246 tests passcargo clippy— no warningscargo fmt --check— clean🤖 Generated with Claude Code