diff --git a/crates/flare-git-core/src/branch.rs b/crates/flare-git-core/src/branch.rs index c1c2afc6..148993f8 100644 --- a/crates/flare-git-core/src/branch.rs +++ b/crates/flare-git-core/src/branch.rs @@ -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/` -- 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/` 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 { + (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] @@ -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}"); + } } diff --git a/crates/flare-git-core/src/worktree.rs b/crates/flare-git-core/src/worktree.rs index b886c186..7719ec6d 100644 --- a/crates/flare-git-core/src/worktree.rs +++ b/crates/flare-git-core/src/worktree.rs @@ -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, args: &[&str], cwd: &Path, diff --git a/src/hook.rs b/src/hook.rs index 9acabd36..8f45b104 100644 --- a/src/hook.rs +++ b/src/hook.rs @@ -288,6 +288,30 @@ pub fn pre_tool_use(_agent: &str) { let mut nudges: Vec = 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/ -- 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); + } + if let Some(nudge) = crate::optimize::batching_nudge(&record.recent_tool_calls, &parsed.tool_name) { diff --git a/src/hook_redirect.rs b/src/hook_redirect.rs index 57eb652a..9783a043 100644 --- a/src/hook_redirect.rs +++ b/src/hook_redirect.rs @@ -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; @@ -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", @@ -134,6 +134,66 @@ fn default_branch(start_path: Option<&Path>) -> Option { } } +/// 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 @@ -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) diff --git a/src/optimize/runtime.rs b/src/optimize/runtime.rs index 46237194..e59f2d6f 100644 --- a/src/optimize/runtime.rs +++ b/src/optimize/runtime.rs @@ -7,6 +7,11 @@ use std::path::PathBuf; pub struct RuntimeState { #[serde(default)] pub sessions: HashMap, + /// 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, } #[derive(Serialize, Deserialize, Default, Clone)] @@ -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;