fix(hook): resolve PreToolUse branch guard against target file's repo, not host cwd - #283
Conversation
…, not host cwd Agentflare-Agent: 1 Agentflare-Branch: fix/pre-tool-use-path-resolve
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBranch lookup in ChangesTarget repository branch guard
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/hook_redirect.rs (1)
63-77: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid unnecessary
PathBufallocations.Both
current_branchanddefault_branchunconditionally allocate a newPathBufvia.to_path_buf()whenstart_pathis provided, just to borrow it on the next line. You can avoid this allocation by matching directly.♻️ Proposed refactor
fn current_branch(start_path: Option<&Path>) -> Option<String> { - let path = start_path - .map(|p| p.to_path_buf()) - .or_else(|| std::env::current_dir().ok())?; - flare_git_core::branch::current_branch(&path) + if let Some(p) = start_path { + flare_git_core::branch::current_branch(p) + } else { + flare_git_core::branch::current_branch(&std::env::current_dir().ok()?) + } } /// Resolve the default branch of the repo containing `start_path`, or cwd if /// `start_path` is None. fn default_branch(start_path: Option<&Path>) -> Option<String> { - let path = start_path - .map(|p| p.to_path_buf()) - .or_else(|| std::env::current_dir().ok())?; - Some(flare_git_core::branch::resolve_default_branch(&path)) + if let Some(p) = start_path { + Some(flare_git_core::branch::resolve_default_branch(p)) + } else { + Some(flare_git_core::branch::resolve_default_branch(&std::env::current_dir().ok()?)) + } }🤖 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/hook_redirect.rs` around lines 63 - 77, Remove the unnecessary PathBuf allocation in both current_branch and default_branch by matching on start_path directly and borrowing the provided Path, while using the current working directory only when start_path is None. Preserve the existing Option handling and branch-resolution behavior.
🤖 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 `@src/hook_redirect.rs`:
- Around line 147-163: Update the target repository resolution in the
MUTATING_TOOLS branch around target_repo so failed path extraction falls back to
the current directory, empty parent paths are treated as ".", and non-existent
parent directories walk upward through ancestors until the first existing
directory before calling repo_toplevel. Preserve the no-guard behavior only when
no repository is found after this fallback resolution.
---
Nitpick comments:
In `@src/hook_redirect.rs`:
- Around line 63-77: Remove the unnecessary PathBuf allocation in both
current_branch and default_branch by matching on start_path directly and
borrowing the provided Path, while using the current working directory only when
start_path is None. Preserve the existing Option handling and branch-resolution
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 17ac9e92-0c9b-48df-98b3-5189798a4290
📒 Files selected for processing (1)
src/hook_redirect.rs
| let (current, default) = if MUTATING_TOOLS.contains(&tool_name.as_str()) { | ||
| (current_branch(), default_branch()) | ||
| let target_repo = tool_input.as_ref().and_then(|ti| { | ||
| ti.get("file_path") | ||
| .or_else(|| ti.get("path")) | ||
| .and_then(Value::as_str) | ||
| .map(Path::new) | ||
| .and_then(|p| p.parent()) | ||
| .and_then(flare_git_core::branch::repo_toplevel) | ||
| }); | ||
| match target_repo { | ||
| Some(repo) => ( | ||
| current_branch(Some(&repo)), | ||
| default_branch(Some(&repo)), | ||
| ), | ||
| // Target path not in any git repo → no branch guard. | ||
| None => (None, None), | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Fix branch guard bypasses for missing parameters, new directories, and local files.
The current implementation has three edge cases that will silently bypass the branch guard:
- Extraction failure bypass: Tools that don't use a top-level
"file_path"or"path"(likeMultiEditorpatch) will result intarget_repo = None, skipping the guard entirely. You should fall back to the current directory if the path cannot be extracted. - Empty parent paths: When editing a file in the current directory (e.g.,
"file.txt"),p.parent()returns an empty path (""). Using this as a working directory for the git subprocess fails withENOENT, returningNoneand incorrectly bypassing the guard. - Non-existent directories: If writing to a new file in a subdirectory that doesn't exist yet (e.g.,
"new_dir/file.txt"), the parent directory won't exist. The git subprocess will fail, again bypassing the guard.
To fix all three, separate the path extraction, walk up the ancestors to the first existing directory (mapping "" to "."), and properly fall back to the current directory when extraction fails.
🐛 Proposed fix
- let (current, default) = if MUTATING_TOOLS.contains(&tool_name.as_str()) {
- let target_repo = tool_input.as_ref().and_then(|ti| {
- ti.get("file_path")
- .or_else(|| ti.get("path"))
- .and_then(Value::as_str)
- .map(Path::new)
- .and_then(|p| p.parent())
- .and_then(flare_git_core::branch::repo_toplevel)
- });
- match target_repo {
- Some(repo) => (
- current_branch(Some(&repo)),
- default_branch(Some(&repo)),
- ),
- // Target path not in any git repo → no branch guard.
- None => (None, None),
- }
+ let (current, default) = if MUTATING_TOOLS.contains(&tool_name.as_str()) {
+ let target_path = tool_input.as_ref().and_then(|ti| {
+ ti.get("file_path")
+ .or_else(|| ti.get("path"))
+ .and_then(Value::as_str)
+ .map(Path::new)
+ });
+ let target_repo = target_path.and_then(|p| {
+ for ancestor in p.ancestors().skip(1) {
+ let check = if ancestor == Path::new("") { Path::new(".") } else { ancestor };
+ if check.exists() {
+ return flare_git_core::branch::repo_toplevel(check);
+ }
+ }
+ None
+ });
+
+ match (target_path, target_repo) {
+ // Path was extracted but isn't in any git repo → no branch guard.
+ (Some(_), None) => (None, None),
+ // Path couldn't be extracted, or repo was found → use target_repo or fall back to cwd.
+ (_, repo) => (
+ current_branch(repo.as_deref()),
+ default_branch(repo.as_deref()),
+ ),
+ }📝 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.
| let (current, default) = if MUTATING_TOOLS.contains(&tool_name.as_str()) { | |
| (current_branch(), default_branch()) | |
| let target_repo = tool_input.as_ref().and_then(|ti| { | |
| ti.get("file_path") | |
| .or_else(|| ti.get("path")) | |
| .and_then(Value::as_str) | |
| .map(Path::new) | |
| .and_then(|p| p.parent()) | |
| .and_then(flare_git_core::branch::repo_toplevel) | |
| }); | |
| match target_repo { | |
| Some(repo) => ( | |
| current_branch(Some(&repo)), | |
| default_branch(Some(&repo)), | |
| ), | |
| // Target path not in any git repo → no branch guard. | |
| None => (None, None), | |
| } | |
| let (current, default) = if MUTATING_TOOLS.contains(&tool_name.as_str()) { | |
| let target_path = tool_input.as_ref().and_then(|ti| { | |
| ti.get("file_path") | |
| .or_else(|| ti.get("path")) | |
| .and_then(Value::as_str) | |
| .map(Path::new) | |
| }); | |
| let target_repo = target_path.and_then(|p| { | |
| for ancestor in p.ancestors().skip(1) { | |
| let check = if ancestor == Path::new("") { Path::new(".") } else { ancestor }; | |
| if check.exists() { | |
| return flare_git_core::branch::repo_toplevel(check); | |
| } | |
| } | |
| None | |
| }); | |
| match (target_path, target_repo) { | |
| // Path was extracted but isn't in any git repo → no branch guard. | |
| (Some(_), None) => (None, None), | |
| // Path couldn't be extracted, or repo was found → use target_repo or fall back to cwd. | |
| (_, repo) => ( | |
| current_branch(repo.as_deref()), | |
| default_branch(repo.as_deref()), | |
| ), | |
| } |
🤖 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/hook_redirect.rs` around lines 147 - 163, Update the target repository
resolution in the MUTATING_TOOLS branch around target_repo so failed path
extraction falls back to the current directory, empty parent paths are treated
as ".", and non-existent parent directories walk upward through ancestors until
the first existing directory before calling repo_toplevel. Preserve the no-guard
behavior only when no repository is found after this fallback resolution.
…ocation Agentflare-Agent: 1 Agentflare-Branch: fix/pre-tool-use-path-resolve
Agentflare-Agent: 1 Agentflare-Branch: fix/pre-tool-use-path-resolve
…missing path fields (#291) * fix(hook): close branch-guard bypasses for bare filenames, new dirs, and missing path fields CodeRabbit flagged this as critical on PR #283 but it was squash-merged without a fix. target_repo resolution called repo_toplevel() directly on p.parent(), which fails (and silently skips the branch guard) for: - a bare filename (parent is \, ENOENT) - a new file under a not-yet-created directory (parent doesn't exist) - a MUTATING_TOOLS call with no top-level file_path/path (e.g. MultiEdit) Now walks ancestors to the first directory that actually exists on disk before resolving its repo toplevel, and falls back to cwd when the path can't be extracted at all -- matching the original bug's intended fix. Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: fix/hook-redirect-branch-guard-bypass * fix(hook): stop mutating process cwd in tests; call repo_toplevel once Two issues found reviewing this PR's own CI run: - The ancestor-walk tests used std::env::set_current_dir, which is global process state -- it collided with an unrelated parallel test (hook::tests::session_start_message_shows_pending_items_from_backend_db, which resolves its project from cwd) and broke CI's ubuntu build. Rewritten to use absolute paths and cwd-independent ground-truth comparisons instead, so no test here touches the real cwd. - CodeRabbit nitpick on the ancestor walk: repo_toplevel() (a git subprocess) was being re-invoked for every existing ancestor instead of stopping at the first one, since git rev-parse --show-toplevel already walks upward internally. Now finds the first existing ancestor and calls repo_toplevel exactly once. Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: fix/hook-redirect-branch-guard-bypass
1 participant
Bug:
current_branch()/default_branch()in the PreToolUse redirect classifier usedstd::env::current_dir()to resolve which repo to check. Two failure modes:file_pathis outside any git repo (e.g.~/.claude/memory/) → write falsely denied with "'master' is this repo's default branch"file_pathis inside a repo on master → write allowed when it shouldn't beFix: extract
file_path/pathfrom tool input, resolve its gittoplevelviarepo_toplevel(), and check that repo's branch. If the target path is not in any git repo, skip the branch guard entirely.Also fixes the opencode stray-diff incident class (item #234's path-not-cwd principle, same root cause).
Summary by CodeRabbit