Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 41 additions & 12 deletions src/hook_redirect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
// redirect rule that needs IO (e.g. a backend DB lookup) can never wedge the
// host's tool call — it just falls through to allow instead.
use serde_json::{Value, json};
use std::path::Path;
use std::sync::mpsc;
use std::thread;
use std::time::Duration;
Expand Down Expand Up @@ -57,14 +58,26 @@ fn is_spec_like_path(path: &str) -> bool {
normalized.contains("/specs/") && normalized.ends_with(".md")
}

fn current_branch() -> Option<String> {
let cwd = std::env::current_dir().ok()?;
flare_git_core::branch::current_branch(&cwd)
/// Resolve the current branch of the repo containing `start_path`, or cwd if
/// `start_path` is None. `None` outside a git repo.
fn current_branch(start_path: Option<&Path>) -> Option<String> {
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()?)
}
}

fn default_branch() -> Option<String> {
let cwd = std::env::current_dir().ok()?;
Some(flare_git_core::branch::resolve_default_branch(&cwd))
/// 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> {
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()?,
))
}
}

/// Pure decision core for the branch guard — no git process spawned here, so
Expand Down Expand Up @@ -122,17 +135,33 @@ fn classify(
}

/// Build the PreToolUse deny decision for a classified redirect, or `None` to
/// let the call through unchanged.
/// let the call through unchanged. Resolves the target file's git repo for
/// branch guard checks (not host cwd), so editing a file outside any git repo
/// (e.g. ~/.claude/memory/) is never blocked, and editing a file in a
/// different repo than cwd checks that repo's branch, not cwd's.
pub fn redirect_decision(tool_name: &str, tool_input: Option<&Value>) -> Option<Value> {
let tool_name = tool_name.to_string();
let tool_input = tool_input.cloned();
decide_with_timeout(GATING_TIMEOUT, move || {
// Only mutating tools ever consult the branch guard (see
// `classify`) — resolving it unconditionally would spawn several
// git subprocesses on every single tool call (Read, Bash, Grep,
// ...), not just the handful that actually need it.
// Only mutating tools ever consult the branch guard — resolving it
// unconditionally would spawn several git subprocesses on every
// single tool call (Read, Bash, Grep, ...), not just the ones that
// need it. When we do check, resolve the target file's repo, not
// host cwd.
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),
}
Comment on lines 151 to +164

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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:

  1. Extraction failure bypass: Tools that don't use a top-level "file_path" or "path" (like MultiEdit or patch) will result in target_repo = None, skipping the guard entirely. You should fall back to the current directory if the path cannot be extracted.
  2. 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 with ENOENT, returning None and incorrectly bypassing the guard.
  3. 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.

Suggested change
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.

} else {
(None, None)
};
Expand Down
Loading