Skip to content
Merged
Show file tree
Hide file tree
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
122 changes: 122 additions & 0 deletions crates/flare-git-core/src/branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,67 @@ pub fn resolve_default_branch(repo_root: &Path) -> String {
.unwrap_or_else(|| "master".to_string())
}

/// Bounded-fetch freshness check: how far HEAD has fallen behind a
/// freshly-fetched `origin/<default_branch>` -- the no-worktree gap item #7
/// closes (the worktree/claim path already gets this for free from
/// `create_worktree`'s own fetch-before-branch behavior). `None` on ANY
/// failure -- offline, no remote, fetch timeout, detached HEAD, or already
/// on the default branch (that case is the protected-branch guard's job,
/// not this one) -- this is advisory, so a network hiccup must never be
/// mistaken for staleness. `Some((default_branch, commits_behind))` on
/// success; `commits_behind` is how many commits are reachable from
/// `origin/<default_branch>` but not from HEAD's merge-base with it (0 =
/// fully caught up).
#[must_use]
pub fn commits_behind_origin_default(
repo_root: &Path,
fetch_timeout_secs: u64,
) -> Option<(String, u64)> {
let default_branch = resolve_default_branch(repo_root);
let current = current_branch(repo_root)?;
if current == default_branch {
return None;
}
let fetched = crate::worktree::run_output_timeout(
crate::shell::git_binary(),
&["fetch", "origin", &default_branch],
repo_root,
fetch_timeout_secs,
)
.map(|out| out.status.success())
.unwrap_or(false);
if !fetched {
return None;
}
let remote_ref = format!("origin/{default_branch}");
if !run_in_ok(repo_root, &["rev-parse", "--verify", &remote_ref]) {
return None;
}
let merge_base = run_in_opt(repo_root, &["merge-base", "HEAD", &remote_ref])?;
let commits_behind = run_in_opt(
repo_root,
&[
"rev-list",
"--count",
&format!("{merge_base}..{remote_ref}"),
],
)?
.parse()
.ok()?;
Some((default_branch, commits_behind))
}

/// Advisory message for [`commits_behind_origin_default`]'s result -- `None`
/// when fully caught up (0 commits behind).
#[must_use]
pub fn staleness_reason(default_branch: &str, commits_behind: u64) -> Option<String> {
(commits_behind > 0).then(|| {
format!(
"this branch is {commits_behind} commit(s) behind origin/{default_branch} — consider merging/rebasing before continuing, so edits build on the latest code"
)
})
}

/// `git rev-parse --show-toplevel` from `start` — handles worktrees/submodules
/// correctly, works regardless of subdirectory. `None` outside a git repo.
#[must_use]
Expand Down Expand Up @@ -225,4 +286,65 @@ mod tests {
.unwrap();
assert!(is_linked_worktree(&wt_path));
}

fn init_remote_and_stale_local_clone()
-> (crate::shell::test_support::Repo, PathBuf, tempfile::TempDir) {
let remote = init_repo_with_branch("master");
let local_container = tempfile::TempDir::new().unwrap();
let local_path = local_container.path().join("local");
crate::shell::run_in(
local_container.path(),
&[
"clone",
remote.path.to_str().unwrap(),
local_path.to_str().unwrap(),
],
)
.unwrap();
crate::shell::run_in(&local_path, &["config", "user.email", "t@t.com"]).unwrap();
crate::shell::run_in(&local_path, &["config", "user.name", "t"]).unwrap();
crate::shell::run_in(&local_path, &["checkout", "-b", "feature/x"]).unwrap();
(remote, local_path, local_container)
}

#[test]
fn commits_behind_origin_default_reports_missed_remote_commits() {
let (remote, local_path, _container) = init_remote_and_stale_local_clone();
crate::shell::run_in(&remote.path, &["commit", "--allow-empty", "-m", "new work"]).unwrap();

let (default_branch, behind) = commits_behind_origin_default(&local_path, 10).unwrap();
assert_eq!(default_branch, "master");
assert_eq!(behind, 1);
}

#[test]
fn commits_behind_origin_default_zero_when_fully_caught_up() {
let (_remote, local_path, _container) = init_remote_and_stale_local_clone();
let (_, behind) = commits_behind_origin_default(&local_path, 10).unwrap();
assert_eq!(behind, 0);
}

#[test]
fn commits_behind_origin_default_none_on_the_default_branch_itself() {
let repo = init_repo_with_branch("master");
assert!(commits_behind_origin_default(&repo.path, 10).is_none());
}

#[test]
fn commits_behind_origin_default_none_without_a_remote() {
let repo = init_repo_with_branch("feature/x");
assert!(commits_behind_origin_default(&repo.path, 1).is_none());
}

#[test]
fn staleness_reason_silent_when_caught_up() {
assert!(staleness_reason("master", 0).is_none());
}

#[test]
fn staleness_reason_names_branch_and_commit_count() {
let reason = staleness_reason("master", 3).unwrap();
assert!(reason.contains("3 commit"), "{reason}");
assert!(reason.contains("origin/master"), "{reason}");
}
}
2 changes: 1 addition & 1 deletion crates/flare-git-core/src/worktree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ fn kill_tree(child: &mut std::process::Child) {
/// running and the process genuinely un-reaped, not just "late". Stdout/
/// stderr are drained on separate threads so a child that fills an OS pipe
/// buffer can't deadlock the wait loop.
fn run_output_timeout(
pub(crate) fn run_output_timeout(
program: impl AsRef<std::ffi::OsStr>,
args: &[&str],
cwd: &Path,
Expand Down
24 changes: 24 additions & 0 deletions src/hook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,30 @@ pub fn pre_tool_use(_agent: &str) {

let mut nudges: Vec<String> = vec![];

// Freshness guard (item #7): on a session's first mutating-tool call,
// warn if the target repo's branch has fallen behind a freshly-fetched
// origin/<default> -- the no-worktree gap `create_worktree`'s own
// fetch-before-branch behavior doesn't cover (that only runs at claim
// time, for the worktree path). `insert` returning `true` doubles as
// the "haven't checked this session yet" gate, so this only ever fires
// once per session -- a bounded `git fetch` on every edit would burn
// into PreToolUse's 5s hook budget for no added benefit. A short 1s
// fetch timeout leaves headroom in that budget; soft-fails (no nudge)
// rather than risk overrunning it.
if crate::hook_redirect::MUTATING_TOOLS.contains(&parsed.tool_name.as_str())
&& runtime
.staleness_checked_sessions
.insert(parsed.session_id.clone())
&& let crate::hook_redirect::TargetRepo::Found(repo) =
crate::hook_redirect::resolve_mutating_target_repo(parsed.tool_input.as_ref())
&& let Some((default_branch, commits_behind)) =
flare_git_core::branch::commits_behind_origin_default(&repo, 1)
&& let Some(reason) =
flare_git_core::branch::staleness_reason(&default_branch, commits_behind)
{
nudges.push(reason);
}
Comment on lines +301 to +313

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 | 🟠 Major | ⚡ Quick win

Per-session gate can be consumed by a non-repo target, permanently skipping the freshness check for the rest of the session.

runtime.staleness_checked_sessions.insert(...) fires as soon as the tool is mutating — before resolve_mutating_target_repo is even evaluated. If the session's first mutating call targets a path outside any git repo (TargetRepo::Outside), the one-shot gate is consumed with no staleness check ever having run, and it will never run again for that session even if a later mutating call targets a genuinely stale repo.

Reorder so the repo must resolve to Found before the session's single check is spent:

🐛 Proposed fix
     if crate::hook_redirect::MUTATING_TOOLS.contains(&parsed.tool_name.as_str())
-        && runtime
-            .staleness_checked_sessions
-            .insert(parsed.session_id.clone())
-        && let crate::hook_redirect::TargetRepo::Found(repo) =
+        && let crate::hook_redirect::TargetRepo::Found(repo) =
             crate::hook_redirect::resolve_mutating_target_repo(parsed.tool_input.as_ref())
+        && runtime
+            .staleness_checked_sessions
+            .insert(parsed.session_id.clone())
         && let Some((default_branch, commits_behind)) =
             flare_git_core::branch::commits_behind_origin_default(&repo, 1)
📝 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
if crate::hook_redirect::MUTATING_TOOLS.contains(&parsed.tool_name.as_str())
&& runtime
.staleness_checked_sessions
.insert(parsed.session_id.clone())
&& let crate::hook_redirect::TargetRepo::Found(repo) =
crate::hook_redirect::resolve_mutating_target_repo(parsed.tool_input.as_ref())
&& let Some((default_branch, commits_behind)) =
flare_git_core::branch::commits_behind_origin_default(&repo, 1)
&& let Some(reason) =
flare_git_core::branch::staleness_reason(&default_branch, commits_behind)
{
nudges.push(reason);
}
if crate::hook_redirect::MUTATING_TOOLS.contains(&parsed.tool_name.as_str())
&& let crate::hook_redirect::TargetRepo::Found(repo) =
crate::hook_redirect::resolve_mutating_target_repo(parsed.tool_input.as_ref())
&& runtime
.staleness_checked_sessions
.insert(parsed.session_id.clone())
&& let Some((default_branch, commits_behind)) =
flare_git_core::branch::commits_behind_origin_default(&repo, 1)
&& let Some(reason) =
flare_git_core::branch::staleness_reason(&default_branch, commits_behind)
{
nudges.push(reason);
}
🤖 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.rs` around lines 301 - 313, Reorder the conditions in the
staleness-check block so resolve_mutating_target_repo returns TargetRepo::Found
before inserting the session ID into runtime.staleness_checked_sessions. Ensure
non-repo targets do not consume the per-session gate, while preserving the
existing single-check behavior for resolved repositories.


if let Some(nudge) =
crate::optimize::batching_nudge(&record.recent_tool_calls, &parsed.tool_name)
{
Expand Down
116 changes: 69 additions & 47 deletions src/hook_redirect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +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::path::{Path, PathBuf};
use std::sync::mpsc;
use std::thread;
use std::time::Duration;
Expand All @@ -23,7 +23,7 @@ const GATING_TIMEOUT: Duration = Duration::from_millis(2000);
/// `multiedit` are opencode-specific) — the opencode branch-guard plugin
/// (`~/.config/opencode/plugin/branch-guard.js`) calls this same classifier
/// via `agentflare hook pre-tool-use` instead of duplicating branch logic.
const MUTATING_TOOLS: &[&str] = &[
pub(crate) const MUTATING_TOOLS: &[&str] = &[
"Write",
"write",
"Edit",
Expand Down Expand Up @@ -134,6 +134,66 @@ fn default_branch(start_path: Option<&Path>) -> Option<String> {
}
}

/// Where a mutating tool's edit will actually land, for any guard that
/// needs "target isn't in a repo at all" to differ from "tool gave no path,
/// fall back to cwd" -- conflating the two is exactly the AGENTFLARE-6 bug
/// (a file's own repo/branch silently swapped for the host cwd's).
pub(crate) enum TargetRepo {
/// Repo resolved -- from the tool's target path, or cwd when the tool
/// gave no path at all (e.g. MultiEdit).
Found(PathBuf),
/// Tool gave an explicit path that isn't inside ANY git repo -- callers
/// must not fall back to cwd's repo/branch instead.
Outside,
}

/// Resolves the repo a mutating tool's edit targets. Walks up from the
/// target path to the first ancestor that actually exists on disk before
/// asking git for its toplevel -- a bare filename's parent is "" (no such
/// dir) and a new file's parent may not exist yet, either of which would
/// otherwise make the git subprocess fail and silently skip whichever guard
/// calls this. `git rev-parse --show-toplevel` already walks up from its
/// start dir looking for `.git`, so only the FIRST existing ancestor needs
/// to actually be handed to it.
pub(crate) fn resolve_mutating_target_repo(tool_input: Option<&Value>) -> TargetRepo {
let target_path = tool_input.and_then(|ti| {
// opencode's native tools send camelCase `filePath`.
ti.get("file_path")
.or_else(|| ti.get("path"))
.or_else(|| ti.get("filePath"))
.and_then(Value::as_str)
.map(Path::new)
});
let Some(p) = target_path else {
let Ok(cwd) = std::env::current_dir() else {
return TargetRepo::Outside;
};
return match flare_git_core::branch::repo_toplevel(&cwd) {
Some(repo) => TargetRepo::Found(repo),
None => TargetRepo::Outside,
};
};
let Some(first_existing) = p.ancestors().skip(1).find(|ancestor| {
let check = if *ancestor == Path::new("") {
Path::new(".")
} else {
*ancestor
};
check.exists()
}) else {
return TargetRepo::Outside;
};
let check = if first_existing == Path::new("") {
Path::new(".")
} else {
first_existing
};
match flare_git_core::branch::repo_toplevel(check) {
Some(repo) => TargetRepo::Found(repo),
None => TargetRepo::Outside,
}
}

/// Pure decision core for the branch guard — no git process spawned here, so
/// it's unit-testable with fake branch names regardless of which branch this
/// actual repo happens to be on when `cargo test` runs (same reason
Expand Down Expand Up @@ -221,51 +281,13 @@ pub fn redirect_decision(tool_name: &str, tool_input: Option<&Value>) -> Option<
// 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()) {
let target_path = tool_input.as_ref().and_then(|ti| {
// opencode's native tools send camelCase `filePath`; without
// it here the target repo resolves to None and the branch
// guard silently allows the edit.
ti.get("file_path")
.or_else(|| ti.get("path"))
.or_else(|| ti.get("filePath"))
.and_then(Value::as_str)
.map(Path::new)
});
// Walk up from the target to the first ancestor that actually
// exists on disk before asking git for its toplevel -- a bare
// filename's parent is "" (no such dir) and a new file's parent
// may not exist yet, either of which would otherwise make the
// git subprocess fail and silently skip the guard.
// `git rev-parse --show-toplevel` already walks up from its
// start dir looking for `.git`, so only the FIRST existing
// ancestor needs to actually be handed to it -- every higher
// ancestor is already covered by that walk, and re-spawning git
// per ancestor just burns time against GATING_TIMEOUT.
let target_repo = target_path.and_then(|p| {
let first_existing = p.ancestors().skip(1).find(|ancestor| {
let check = if *ancestor == Path::new("") {
Path::new(".")
} else {
*ancestor
};
check.exists()
})?;
let check = if first_existing == Path::new("") {
Path::new(".")
} else {
first_existing
};
flare_git_core::branch::repo_toplevel(check)
});
match (target_path, target_repo) {
// Path was extracted but isn't in any git repo -- no guard.
(Some(_), None) => (None, None),
// Path couldn't be extracted (tool has no file_path/path,
// e.g. MultiEdit) -- fall back to cwd; repo found -- use it.
(_, repo) => (
current_branch(repo.as_deref()),
default_branch(repo.as_deref()),
),
match resolve_mutating_target_repo(tool_input.as_ref()) {
// Target isn't in any git repo (or has no path at all and
// cwd isn't one either) -- no guard.
TargetRepo::Outside => (None, None),
TargetRepo::Found(repo) => {
(current_branch(Some(&repo)), default_branch(Some(&repo)))
}
}
} else {
(None, None)
Expand Down
9 changes: 9 additions & 0 deletions src/optimize/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ use std::path::PathBuf;
pub struct RuntimeState {
#[serde(default)]
pub sessions: HashMap<String, SessionRecord>,
/// Session ids the freshness guard (item #7) has already run for --
/// the guard does a bounded `git fetch`, so it must only run once per
/// session (on the first mutating-tool call), not on every edit.
#[serde(default)]
pub staleness_checked_sessions: std::collections::HashSet<String>,
}

#[derive(Serialize, Deserialize, Default, Clone)]
Expand Down Expand Up @@ -48,6 +53,10 @@ pub fn prune_stale_sessions(state: &mut RuntimeState, now: u64) {
state
.sessions
.retain(|_, record| now.saturating_sub(record.start_ts) < STALE_SESSION_SECS);
let live_sessions = &state.sessions;
state
.staleness_checked_sessions
.retain(|id| live_sessions.contains_key(id));
}

pub const SESSION_HYGIENE_TURN_THRESHOLD: u32 = 80;
Expand Down
Loading