From 406856fdd9c5c3c26ef70eaa5414e5e47938e8f8 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 12:46:44 +0530 Subject: [PATCH 01/17] =?UTF-8?q?feat(shim):=20add=20agentflare-shim=20cra?= =?UTF-8?q?te=20=E2=80=94=20PATH-based=20lean-ctx=20dispatch=20for=20AI=20?= =?UTF-8?q?agent=20shells?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Compiled binary, copied per tool name into ~/.agentflare/shims/, mirroring mise's crates/mise-shim pattern. Solves the gap where Claude Code's PowerShell tool runs pwsh.exe -NoProfile -NonInteractive, so anything gated behind $PROFILE (lean-ctx's own shell-hook.ps1, or a .bashenv-style function) never loads — PATH resolution of bare command names still works regardless of -NoProfile, so a shim dir on PATH still gets hit. Gate order mirrors .bashenv's _af_dispatch: kill switches -> agent-env marker -> .agentflare project walk-up (stopping at $HOME, since ~/.agentflare is agentflare's own app-data dir, not a project marker) -> lean-ctx -c -> real-binary fallback. Strict no-op for anything outside that double gate. --- Cargo.lock | 8 ++ Cargo.toml | 2 +- crates/agentflare-shim/Cargo.toml | 23 +++++ crates/agentflare-shim/src/main.rs | 146 +++++++++++++++++++++++++++++ 4 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 crates/agentflare-shim/Cargo.toml create mode 100644 crates/agentflare-shim/src/main.rs diff --git a/Cargo.lock b/Cargo.lock index 62fff5cc..9572af27 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -215,6 +215,14 @@ dependencies = [ "ureq 2.12.1", ] +[[package]] +name = "agentflare-shim" +version = "0.1.0" +dependencies = [ + "dirs", + "which", +] + [[package]] name = "agentflare-skill-registry" version = "0.1.1" diff --git a/Cargo.toml b/Cargo.toml index 58fe55b9..de40024f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,5 +1,5 @@ [workspace] -members = ["crates/flare-code", "crates/agent-registry", "crates/skill-registry", "crates/gateway-registry", "crates/flare-output", "crates/agentflare-artifacts", "crates/agentflare-backend", "crates/agentflare-db-kit", "crates/flare-search-kit", "crates/agentflare-store", "crates/flare-proxy"] +members = ["crates/flare-code", "crates/agent-registry", "crates/skill-registry", "crates/gateway-registry", "crates/flare-output", "crates/agentflare-artifacts", "crates/agentflare-backend", "crates/agentflare-db-kit", "crates/flare-search-kit", "crates/agentflare-store", "crates/flare-proxy", "crates/agentflare-shim"] resolver = "2" [package] diff --git a/crates/agentflare-shim/Cargo.toml b/crates/agentflare-shim/Cargo.toml new file mode 100644 index 00000000..57a597a3 --- /dev/null +++ b/crates/agentflare-shim/Cargo.toml @@ -0,0 +1,23 @@ +[package] +name = "agentflare-shim" +version = "0.1.0" +edition = "2024" +rust-version = "1.91" +description = "Compiled PATH shim routing AI-agent shell commands through lean-ctx -c, project-scoped via .agentflare" +license = "Apache-2.0" +publish = false + +[[bin]] +name = "agentflare-shim" +path = "src/main.rs" + +[dependencies] +which = "6" +dirs = "6" + +[lints.rust] +unsafe_code = "warn" + +[lints.clippy] +all = "warn" +pedantic = "warn" diff --git a/crates/agentflare-shim/src/main.rs b/crates/agentflare-shim/src/main.rs new file mode 100644 index 00000000..abf46591 --- /dev/null +++ b/crates/agentflare-shim/src/main.rs @@ -0,0 +1,146 @@ +//! Compiled PATH shim for AI-agent shell sessions -- cross-platform, same +//! pattern as mise's `crates/mise-shim`: one small binary, copied/hardlinked +//! under many tool names (git, cargo, git.exe, cargo.exe, ...) into +//! `~/.agentflare/shims/`, prepended to PATH. Each copy reads its own +//! filename (argv[0] / `current_exe`) to learn which tool it stands in for, +//! then either routes the call through `lean-ctx -c ` or execs the +//! real binary untouched. Bundled alongside the main `agentflare` binary at +//! release time, same as mise bundles `mise-shim`. +//! +//! On Windows this is the only mechanism that reaches agent tool calls at +//! all: Claude Code's PowerShell tool runs +//! `pwsh.exe -NoProfile -NonInteractive -Command "..."`, and `-NoProfile` +//! skips `$PROFILE` entirely, so anything gated behind a shell profile +//! (lean-ctx's own `shell-hook.ps1`, or a shell-function approach) never +//! loads. PATH resolution of bare command names inside the `-Command` +//! payload happens regardless of `-NoProfile`, so a shim directory on PATH +//! still gets hit. On Unix this plays the same role a `.bashenv` function +//! would -- same gate logic, just compiled instead of shell. +//! +//! Gate order: kill switches -> agent-env marker -> `.agentflare` project +//! walk-up (stopping at `$HOME`, since `~/.agentflare` is agentflare's own +//! app-data dir, not a project marker -- a false-positive bug found and +//! fixed on the bash-function prototype of this same idea). This is for AI +//! agent CLIs only: anything outside that double gate must resolve to the +//! real binary, unmodified, with negligible overhead, since a shim dir on +//! PATH affects every process that resolves through it, system-wide. + +use std::env; +use std::ffi::OsString; +use std::path::{Path, PathBuf}; +use std::process::{Command, exit}; + +const KILL_SWITCHES: &[&str] = &["LEAN_CTX_DISABLED", "LEAN_CTX_NO_HOOK"]; + +const AGENT_ENV_VARS: &[&str] = &[ + "LEAN_CTX_AGENT", + "CLAUDECODE", + "CURSOR_AGENT", + "CODEX_CLI_SESSION", + "GEMINI_SESSION", + "CODEBUDDY", +]; + +const PROJECT_MARKER: &str = ".agentflare"; + +fn is_set(name: &str) -> bool { + env::var_os(name).is_some_and(|v| !v.is_empty()) +} + +fn any_set(names: &[&str]) -> bool { + names.iter().any(|n| is_set(n)) +} + +/// Walk up from `start` looking for `.agentflare`, stopping at `home` +/// (exclusive) -- `~/.agentflare` is agentflare's own data dir, not a +/// project marker, and would otherwise false-positive on everything +/// under the user's home directory. +fn in_scoped_project(start: &Path, home: Option<&Path>) -> bool { + let mut dir = Some(start); + while let Some(d) = dir { + if home.is_some_and(|h| h == d) { + return false; + } + if d.join(PROJECT_MARKER).exists() { + return true; + } + dir = d.parent(); + } + false +} + +/// PATH with `shim_dir` removed, so neither our own real-binary lookup nor a +/// `lean-ctx -c` child process resolves back into this shim (self-recursion). +fn path_without_shim_dir(shim_dir: &Path) -> Option { + let path_var = env::var_os("PATH")?; + env::join_paths(env::split_paths(&path_var).filter(|p| p != shim_dir)).ok() +} + +fn trace(msg: &str) { + if is_set("AGENTFLARE_SHIM_TRACE") { + eprintln!("[flare-trace] {msg}"); + } +} + +fn run_real(tool: &str, filtered_path: Option<&OsString>, args: &[OsString]) -> ! { + trace(&format!("real: {tool}")); + let cwd = env::current_dir().unwrap_or_default(); + let resolved = match filtered_path { + Some(p) => which::which_in(tool, Some(p), cwd), + None => which::which(tool), + }; + let Ok(real) = resolved else { + eprintln!("agentflare-shim: command not found: {tool}"); + exit(127); + }; + match Command::new(real).args(args).status() { + Ok(status) => exit(status.code().unwrap_or(1)), + Err(e) => { + eprintln!("agentflare-shim: failed to exec {tool}: {e}"); + exit(127) + } + } +} + +fn main() { + let exe = match env::current_exe() { + Ok(p) => p, + Err(e) => { + eprintln!("agentflare-shim: failed to determine executable path: {e}"); + exit(1); + } + }; + let Some(tool) = exe.file_stem().and_then(|s| s.to_str()).map(str::to_string) else { + eprintln!("agentflare-shim: failed to determine tool name from executable path"); + exit(1); + }; + let shim_dir: PathBuf = exe.parent().map_or_else(PathBuf::new, Path::to_path_buf); + let args: Vec = env::args_os().skip(1).collect(); + let filtered_path = path_without_shim_dir(&shim_dir); + + if any_set(KILL_SWITCHES) || !any_set(AGENT_ENV_VARS) { + run_real(&tool, filtered_path.as_ref(), &args); + } + + let cwd = env::current_dir().unwrap_or_default(); + if !in_scoped_project(&cwd, dirs::home_dir().as_deref()) { + run_real(&tool, filtered_path.as_ref(), &args); + } + + trace(&format!("dispatch: lean-ctx -c {tool}")); + let mut cmd = Command::new("lean-ctx"); + cmd.arg("-c").arg(&tool).args(&args); + if let Some(p) = &filtered_path { + cmd.env("PATH", p); + } + match cmd.status() { + Ok(status) => { + let code = status.code().unwrap_or(1); + if code == 126 || code == 127 { + run_real(&tool, filtered_path.as_ref(), &args); + } + exit(code); + } + Err(_) => run_real(&tool, filtered_path.as_ref(), &args), + } +} From 43d47a3b3033dd97ecd3eb7f31c19bd9e8e4596c Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 13:26:59 +0530 Subject: [PATCH 02/17] refactor(agentflare-shim): split into lib + bin, expose generic exec core run_real, path_without_shim_dir, tool_name_from_exe, trace, and is_set move into src/lib.rs as a reusable public API. main.rs keeps all lean-ctx-specific dispatch (kill switches, agent-env gate, .agentflare project walk-up, the lean-ctx -c call) and now consumes the lib for the generic exec plumbing. Prerequisite for a second shim binary (flare-git-shim) that will reuse the same resolve+exec+propagate-exit-code core for a different dispatch target. --- crates/agentflare-shim/src/lib.rs | 61 ++++++++++++++++++++++++++++++ crates/agentflare-shim/src/main.rs | 41 ++------------------ 2 files changed, 64 insertions(+), 38 deletions(-) create mode 100644 crates/agentflare-shim/src/lib.rs diff --git a/crates/agentflare-shim/src/lib.rs b/crates/agentflare-shim/src/lib.rs new file mode 100644 index 00000000..fd6d83ed --- /dev/null +++ b/crates/agentflare-shim/src/lib.rs @@ -0,0 +1,61 @@ +//! Generic PATH-shim exec plumbing, shared by any `agentflare-*` shim +//! binary: resolve the real target binary, exec it with argv/stdio +//! passthrough, and propagate its exit code. Tool-specific dispatch logic +//! (what to do BEFORE falling back to the real binary) lives in each shim +//! binary's own `main.rs`. + +use std::env; +use std::ffi::OsString; +use std::path::Path; +use std::process::{Command, exit}; + +/// True if the named env var is set to a non-empty value. +#[must_use] +pub fn is_set(name: &str) -> bool { + env::var_os(name).is_some_and(|v| !v.is_empty()) +} + +/// Emits a trace line to stderr when `AGENTFLARE_SHIM_TRACE` is set. +pub fn trace(msg: &str) { + if is_set("AGENTFLARE_SHIM_TRACE") { + eprintln!("[flare-trace] {msg}"); + } +} + +/// PATH with `shim_dir` removed, so a shim binary's own real-binary lookup +/// (and any child process it spawns) doesn't resolve back into itself. +#[must_use] +pub fn path_without_shim_dir(shim_dir: &Path) -> Option { + let path_var = env::var_os("PATH")?; + env::join_paths(env::split_paths(&path_var).filter(|p| p != shim_dir)).ok() +} + +/// The tool name a shim binary is standing in for, derived from its own +/// filename (argv[0] / `current_exe`) -- e.g. a binary copied to `git` or +/// `git.exe` resolves to `"git"`. +pub fn tool_name_from_exe(exe: &Path) -> Option { + exe.file_stem().and_then(|s| s.to_str()).map(str::to_string) +} + +/// Resolve `tool` on `filtered_path` (or the current PATH if `None`), exec +/// it with argv/stdio forwarded, and exit with its exit code. Exits 127 if +/// the tool can't be found or fails to spawn -- never returns. +pub fn run_real(tool: &str, filtered_path: Option<&OsString>, args: &[OsString]) -> ! { + trace(&format!("real: {tool}")); + let cwd = env::current_dir().unwrap_or_default(); + let resolved = match filtered_path { + Some(p) => which::which_in(tool, Some(p), cwd), + None => which::which(tool), + }; + let Ok(real) = resolved else { + eprintln!("agentflare-shim: command not found: {tool}"); + exit(127); + }; + match Command::new(real).args(args).status() { + Ok(status) => exit(status.code().unwrap_or(1)), + Err(e) => { + eprintln!("agentflare-shim: failed to exec {tool}: {e}"); + exit(127) + } + } +} diff --git a/crates/agentflare-shim/src/main.rs b/crates/agentflare-shim/src/main.rs index abf46591..5094df05 100644 --- a/crates/agentflare-shim/src/main.rs +++ b/crates/agentflare-shim/src/main.rs @@ -30,6 +30,8 @@ use std::ffi::OsString; use std::path::{Path, PathBuf}; use std::process::{Command, exit}; +use agentflare_shim::{is_set, path_without_shim_dir, run_real, tool_name_from_exe, trace}; + const KILL_SWITCHES: &[&str] = &["LEAN_CTX_DISABLED", "LEAN_CTX_NO_HOOK"]; const AGENT_ENV_VARS: &[&str] = &[ @@ -43,10 +45,6 @@ const AGENT_ENV_VARS: &[&str] = &[ const PROJECT_MARKER: &str = ".agentflare"; -fn is_set(name: &str) -> bool { - env::var_os(name).is_some_and(|v| !v.is_empty()) -} - fn any_set(names: &[&str]) -> bool { names.iter().any(|n| is_set(n)) } @@ -69,39 +67,6 @@ fn in_scoped_project(start: &Path, home: Option<&Path>) -> bool { false } -/// PATH with `shim_dir` removed, so neither our own real-binary lookup nor a -/// `lean-ctx -c` child process resolves back into this shim (self-recursion). -fn path_without_shim_dir(shim_dir: &Path) -> Option { - let path_var = env::var_os("PATH")?; - env::join_paths(env::split_paths(&path_var).filter(|p| p != shim_dir)).ok() -} - -fn trace(msg: &str) { - if is_set("AGENTFLARE_SHIM_TRACE") { - eprintln!("[flare-trace] {msg}"); - } -} - -fn run_real(tool: &str, filtered_path: Option<&OsString>, args: &[OsString]) -> ! { - trace(&format!("real: {tool}")); - let cwd = env::current_dir().unwrap_or_default(); - let resolved = match filtered_path { - Some(p) => which::which_in(tool, Some(p), cwd), - None => which::which(tool), - }; - let Ok(real) = resolved else { - eprintln!("agentflare-shim: command not found: {tool}"); - exit(127); - }; - match Command::new(real).args(args).status() { - Ok(status) => exit(status.code().unwrap_or(1)), - Err(e) => { - eprintln!("agentflare-shim: failed to exec {tool}: {e}"); - exit(127) - } - } -} - fn main() { let exe = match env::current_exe() { Ok(p) => p, @@ -110,7 +75,7 @@ fn main() { exit(1); } }; - let Some(tool) = exe.file_stem().and_then(|s| s.to_str()).map(str::to_string) else { + let Some(tool) = tool_name_from_exe(&exe) else { eprintln!("agentflare-shim: failed to determine tool name from executable path"); exit(1); }; From 568214bcb3389435bb56ebdbca87a82e144a2e60 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 13:30:23 +0530 Subject: [PATCH 03/17] =?UTF-8?q?feat(flare-git-core):=20new=20crate=20?= =?UTF-8?q?=E2=80=94=20shell=20primitives=20+=20branch=20resolution?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds crates/flare-git-core as the single source of truth for local git operations, mirroring agentflare-store's structural pattern (flat sibling files by concern, free functions, no trait+backends abstraction since there's exactly one backend: the local git CLI). shell.rs: run_in/run_in_opt/run_in_ok/diff, lifted verbatim from src/git.rs. branch.rs: current_branch/resolve_default_branch/repo_toplevel, plus a new is_protected_branch predicate extracted from hook_redirect.rs's inline branch-guard logic — shared going forward by both the PreToolUse guard and the new git-shim's command classifier instead of two separate definitions. Not yet wired into src/ — existing crate::git::* call sites are untouched in this commit so the branch stays buildable at every step; rewiring is the next commit. --- Cargo.toml | 2 +- crates/flare-git-core/Cargo.toml | 16 ++++ crates/flare-git-core/src/branch.rs | 110 ++++++++++++++++++++++++++ crates/flare-git-core/src/lib.rs | 8 ++ crates/flare-git-core/src/shell.rs | 116 ++++++++++++++++++++++++++++ 5 files changed, 251 insertions(+), 1 deletion(-) create mode 100644 crates/flare-git-core/Cargo.toml create mode 100644 crates/flare-git-core/src/branch.rs create mode 100644 crates/flare-git-core/src/lib.rs create mode 100644 crates/flare-git-core/src/shell.rs diff --git a/Cargo.toml b/Cargo.toml index de40024f..c635a6b6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,5 +1,5 @@ [workspace] -members = ["crates/flare-code", "crates/agent-registry", "crates/skill-registry", "crates/gateway-registry", "crates/flare-output", "crates/agentflare-artifacts", "crates/agentflare-backend", "crates/agentflare-db-kit", "crates/flare-search-kit", "crates/agentflare-store", "crates/flare-proxy", "crates/agentflare-shim"] +members = ["crates/flare-code", "crates/agent-registry", "crates/skill-registry", "crates/gateway-registry", "crates/flare-output", "crates/agentflare-artifacts", "crates/agentflare-backend", "crates/agentflare-db-kit", "crates/flare-search-kit", "crates/agentflare-store", "crates/flare-proxy", "crates/agentflare-shim", "crates/flare-git-core"] resolver = "2" [package] diff --git a/crates/flare-git-core/Cargo.toml b/crates/flare-git-core/Cargo.toml new file mode 100644 index 00000000..dca65686 --- /dev/null +++ b/crates/flare-git-core/Cargo.toml @@ -0,0 +1,16 @@ +[package] +name = "flare-git-core" +version = "0.1.0" +edition = "2024" +rust-version = "1.91" +license = "Apache-2.0" +description = "Local git primitives, worktree management, and PATH-shim policy (classify/snapshot/provenance/audit) for agentflare -- single source of truth, consumed by the CLI, MCP server, and the git PATH shim." +publish = false + +[dependencies] +serde = { version = "1", features = ["derive"] } +serde_json = "1" +chrono = "0.4" + +[dev-dependencies] +tempfile = "3" diff --git a/crates/flare-git-core/src/branch.rs b/crates/flare-git-core/src/branch.rs new file mode 100644 index 00000000..98d908b6 --- /dev/null +++ b/crates/flare-git-core/src/branch.rs @@ -0,0 +1,110 @@ +//! Branch resolution and the protected-branch predicate — the latter is +//! shared by both agentflare's own PreToolUse branch guard and the new +//! git-shim's command classifier, so "is this branch protected" has exactly +//! one definition. + +use crate::shell::{run_in_opt, run_in_ok}; +use std::path::{Path, PathBuf}; + +/// Current branch name (`HEAD` in detached-HEAD state). `None` outside a git +/// repo or if git isn't on `PATH`. +#[must_use] +pub fn current_branch(repo_root: &Path) -> Option { + run_in_opt(repo_root, &["rev-parse", "--abbrev-ref", "HEAD"]) +} + +/// Best-effort resolution of "the" default branch: prefer the remote's own +/// record of it (`origin/HEAD`'s symbolic ref, which survives a repo default +/// named anything other than main/master), then whichever of main/master +/// actually exists as a local branch, then whatever is actually checked out +/// here — so a repo naming its default branch e.g. `trunk`/`develop` still +/// resolves instead of falling through to a hardcoded guess that may not +/// even exist. +#[must_use] +pub fn resolve_default_branch(repo_root: &Path) -> String { + if let Some(origin_head) = run_in_opt( + repo_root, + &["symbolic-ref", "--short", "refs/remotes/origin/HEAD"], + ) && let Some(stripped) = origin_head.strip_prefix("origin/") + { + return stripped.to_string(); + } + if run_in_ok(repo_root, &["rev-parse", "--verify", "main"]) { + return "main".to_string(); + } + if run_in_ok(repo_root, &["rev-parse", "--verify", "master"]) { + return "master".to_string(); + } + run_in_opt(repo_root, &["symbolic-ref", "--short", "HEAD"]).unwrap_or_else(|| "master".to_string()) +} + +/// `git rev-parse --show-toplevel` from `start` — handles worktrees/submodules +/// correctly, works regardless of subdirectory. `None` outside a git repo. +#[must_use] +pub fn repo_toplevel(start: &Path) -> Option { + run_in_opt(start, &["rev-parse", "--show-toplevel"]).map(PathBuf::from) +} + +/// `true` if `branch` is protected: the repo's resolved default branch when +/// known, otherwise a bare guess against the two conventional names +/// ("main"/"master") when resolution failed entirely (no git, no remote, no +/// main/master branch found). +#[must_use] +pub fn is_protected_branch(branch: &str, default: Option<&str>) -> bool { + match default { + Some(default) => branch == default, + None => branch == "main" || branch == "master", + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::shell::test_support::init_repo_with_branch; + + #[test] + fn current_branch_reports_the_checked_out_branch() { + let repo = init_repo_with_branch("feature/x"); + assert_eq!(current_branch(&repo.path).as_deref(), Some("feature/x")); + } + + #[test] + fn resolve_default_branch_resolves_from_origin_head() { + let repo = init_repo_with_branch("master"); + assert_eq!(resolve_default_branch(&repo.path), "master"); + } + + #[test] + fn resolve_default_branch_falls_back_to_actual_head_for_nonstandard_names() { + // No origin, no "main", no "master" — must not guess "master" when + // the repo's real default branch is named something else entirely. + let repo = init_repo_with_branch("trunk"); + assert_eq!(resolve_default_branch(&repo.path), "trunk"); + } + + #[test] + fn repo_toplevel_finds_root_from_a_subdirectory() { + let repo = init_repo_with_branch("master"); + let sub = repo.path.join("sub"); + std::fs::create_dir(&sub).unwrap(); + assert_eq!( + repo_toplevel(&sub).map(|p| p.canonicalize().unwrap()), + repo.path.canonicalize().ok(), + ); + } + + #[test] + fn is_protected_branch_prefers_default_over_hardcoded_names() { + // A repo whose default branch is deliberately named neither + // main nor master must still be caught via the resolved default. + assert!(is_protected_branch("develop", Some("develop"))); + assert!(!is_protected_branch("feature/y", Some("develop"))); + } + + #[test] + fn is_protected_branch_falls_back_to_main_or_master_when_default_unresolved() { + assert!(is_protected_branch("master", None)); + assert!(is_protected_branch("main", None)); + assert!(!is_protected_branch("feature/x", None)); + } +} diff --git a/crates/flare-git-core/src/lib.rs b/crates/flare-git-core/src/lib.rs new file mode 100644 index 00000000..417d84e7 --- /dev/null +++ b/crates/flare-git-core/src/lib.rs @@ -0,0 +1,8 @@ +//! Local git primitives, worktree management, and PATH-shim policy for +//! agentflare -- single source of truth for anything that shells out to +//! `git` locally, consumed by the CLI, MCP server, and the `git` PATH shim. +//! Remote GitHub REST API operations (PRs/issues/CI/releases) are a +//! separate, unrelated concern and live in `src/github/*` -- not here. + +pub mod branch; +pub mod shell; diff --git a/crates/flare-git-core/src/shell.rs b/crates/flare-git-core/src/shell.rs new file mode 100644 index 00000000..868816d1 --- /dev/null +++ b/crates/flare-git-core/src/shell.rs @@ -0,0 +1,116 @@ +//! Shared git-shelling primitives. Every module that needs to run `git` +//! against a repo goes through here instead of hand-rolling its own +//! `Command::new("git")` wrapper. + +use std::path::Path; +use std::process::Command; + +/// Runs `git` in `repo_root`; `Ok(stdout)` trimmed on success, `Err(stderr)` +/// trimmed on a non-zero exit, or a process-spawn error message (git +/// missing, etc) if it couldn't even run. +pub fn run_in(repo_root: &Path, args: &[&str]) -> Result { + let out = Command::new("git") + .args(args) + .current_dir(repo_root) + .output() + .map_err(|e| format!("git not available: {e}"))?; + if !out.status.success() { + return Err(String::from_utf8_lossy(&out.stderr).trim().to_string()); + } + Ok(String::from_utf8_lossy(&out.stdout).trim().to_string()) +} + +/// `run_in`, discarding the error and treating empty stdout as `None` — the +/// "best-effort, don't care why it failed" shape most callers actually want. +#[must_use] +pub fn run_in_opt(repo_root: &Path, args: &[&str]) -> Option { + run_in(repo_root, args).ok().filter(|s| !s.is_empty()) +} + +/// `true` if `git ` exits 0 in `repo_root`; stdout/stderr don't matter. +#[must_use] +pub fn run_in_ok(repo_root: &Path, args: &[&str]) -> bool { + run_in(repo_root, args).is_ok() +} + +/// Unified diff for `base...head` (three-dot: changes on `head` since it +/// diverged from `base`). Stdout is returned RAW, not trimmed — diff output +/// is multi-line and whitespace-significant, unlike the single-value queries +/// the rest of this module's helpers return. +pub fn diff(repo_root: &Path, base: &str, head: &str) -> Result { + let range = format!("{base}...{head}"); + let out = Command::new("git") + .args(["diff", "--unified=3", &range]) + .current_dir(repo_root) + .output() + .map_err(|e| format!("git diff failed: {e}"))?; + if !out.status.success() { + return Err(format!( + "git diff {range}: {}", + String::from_utf8_lossy(&out.stderr).trim() + )); + } + Ok(String::from_utf8_lossy(&out.stdout).to_string()) +} + +#[cfg(test)] +pub(crate) mod test_support { + use super::run_in; + use std::path::PathBuf; + use tempfile::TempDir; + + pub struct Repo { + _dir: TempDir, + pub path: PathBuf, + } + + pub fn init_repo_with_branch(branch: &str) -> Repo { + let dir = TempDir::new().unwrap(); + let path = dir.path().to_path_buf(); + run_in(&path, &["init", "-b", branch]).unwrap(); + run_in(&path, &["config", "user.email", "test@test.com"]).unwrap(); + run_in(&path, &["config", "user.name", "Test"]).unwrap(); + run_in(&path, &["commit", "--allow-empty", "-m", "initial"]).unwrap(); + Repo { _dir: dir, path } + } +} + +#[cfg(test)] +mod tests { + use super::test_support::init_repo_with_branch; + use super::*; + use tempfile::TempDir; + + #[test] + fn run_in_opt_is_none_outside_a_repo() { + let dir = TempDir::new().unwrap(); + assert!(run_in_opt(dir.path(), &["rev-parse", "--abbrev-ref", "HEAD"]).is_none()); + } + + #[test] + fn run_in_ok_reflects_exit_status() { + let repo = init_repo_with_branch("master"); + assert!(run_in_ok(&repo.path, &["rev-parse", "--verify", "master"])); + assert!(!run_in_ok( + &repo.path, + &["rev-parse", "--verify", "no-such-branch"] + )); + } + + #[test] + fn diff_returns_untrimmed_output_across_a_change() { + let repo = init_repo_with_branch("master"); + std::fs::write(repo.path.join("f.txt"), "hello\n").unwrap(); + run_in(&repo.path, &["add", "f.txt"]).unwrap(); + run_in(&repo.path, &["commit", "-m", "add f.txt"]).unwrap(); + let out = diff(&repo.path, "HEAD~1", "HEAD").unwrap(); + assert!(out.contains("+hello"), "{out}"); + } + + #[test] + fn diff_reports_git_stderr_on_an_invalid_range() { + let repo = init_repo_with_branch("master"); + let err = diff(&repo.path, "no-such-branch", "HEAD").unwrap_err(); + assert!(err.contains("no-such-branch"), "{err}"); + } +} From 14abc4eb5274eae220c720f8b2080a1955de5fd6 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 13:30:40 +0530 Subject: [PATCH 04/17] chore: update Cargo.lock for flare-git-core crate --- Cargo.lock | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 9572af27..9820655a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1016,6 +1016,16 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582" +[[package]] +name = "flare-git-core" +version = "0.1.0" +dependencies = [ + "chrono", + "serde", + "serde_json", + "tempfile", +] + [[package]] name = "flare-proxy" version = "0.1.0" From ff8b59881528b3cc63d5317e707ab88c429d2ebc Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 13:32:14 +0530 Subject: [PATCH 05/17] refactor: rewire crate::git:: consumers onto flare-git-core MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mechanical import-path swap only, no behavior change: cli/review.rs, gateway_integrations.rs, github/identity.rs, github/init_auth.rs, hook_redirect.rs, mcp_server.rs, mcp_server/flare_git.rs (GitHub-API scope untouched, only its one local-git call site), review.rs now call flare_git_core::{shell,branch}::* instead of crate::git::*. src/git.rs and src/worktree.rs are left alone here — worktree.rs is the last remaining crate::git:: consumer and is handled in the next commit along with the worktree-mechanics migration; git.rs is deleted once that lands and grep confirms it's no longer referenced anywhere. --- Cargo.lock | 1 + Cargo.toml | 1 + src/cli/review.rs | 2 +- src/gateway_integrations.rs | 2 +- src/github/identity.rs | 2 +- src/github/init_auth.rs | 2 +- src/hook_redirect.rs | 4 ++-- src/mcp_server.rs | 6 +++--- src/mcp_server/flare_git.rs | 2 +- src/review.rs | 2 +- 10 files changed, 13 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9820655a..09114375 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -85,6 +85,7 @@ dependencies = [ "color-eyre", "dirs", "eyre", + "flare-git-core", "flare-proxy", "flare-search-kit", "flate2", diff --git a/Cargo.toml b/Cargo.toml index c635a6b6..af01c0b3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -79,6 +79,7 @@ tower-http = { version = "0.6", features = ["trace"] } rust-embed = "8" tokio-stream = { version = "0.1", features = ["sync"] } agentflare-store = { path = "crates/agentflare-store" } +flare-git-core = { path = "crates/flare-git-core" } flare-proxy = { path = "crates/flare-proxy" } [target.'cfg(unix)'.dependencies] diff --git a/src/cli/review.rs b/src/cli/review.rs index fe74f316..4a187adb 100644 --- a/src/cli/review.rs +++ b/src/cli/review.rs @@ -227,7 +227,7 @@ impl ReviewArgs { /// Round id: explicit --pr, else the current branch name. fn resolve_pr(explicit: Option) -> String { explicit.filter(|s| !s.is_empty()).unwrap_or_else(|| { - crate::git::current_branch(&std::env::current_dir().unwrap_or_default()) + flare_git_core::branch::current_branch(&std::env::current_dir().unwrap_or_default()) .unwrap_or_else(|| fail("could not determine round — pass --pr".to_string())) }) } diff --git a/src/gateway_integrations.rs b/src/gateway_integrations.rs index 6398f789..25f5a719 100644 --- a/src/gateway_integrations.rs +++ b/src/gateway_integrations.rs @@ -86,7 +86,7 @@ fn remotes_mention_github(remotes: &str) -> bool { } fn git_remote_is_github() -> bool { - crate::git::run_in_opt( + flare_git_core::shell::run_in_opt( &std::env::current_dir().unwrap_or_default(), &["remote", "-v"], ) diff --git a/src/github/identity.rs b/src/github/identity.rs index 06ffe4e9..e3bcb6f7 100644 --- a/src/github/identity.rs +++ b/src/github/identity.rs @@ -39,7 +39,7 @@ impl RepoId { } pub fn resolve_from_remote(repo_root: &Path) -> Option { - let url = crate::git::run_in_opt(repo_root, &["remote", "get-url", "origin"])?; + let url = flare_git_core::shell::run_in_opt(repo_root, &["remote", "get-url", "origin"])?; RepoId::parse(&url) } } diff --git a/src/github/init_auth.rs b/src/github/init_auth.rs index 4c325842..c4b7b322 100644 --- a/src/github/init_auth.rs +++ b/src/github/init_auth.rs @@ -47,7 +47,7 @@ fn secret_present() -> bool { } pub fn is_github_repo(repo_root: &std::path::Path) -> bool { - crate::git::run_in_opt(repo_root, &["remote", "get-url", "origin"]) + flare_git_core::shell::run_in_opt(repo_root, &["remote", "get-url", "origin"]) .map(|u| u.contains("github")) .unwrap_or(false) } diff --git a/src/hook_redirect.rs b/src/hook_redirect.rs index 2de1bd1a..091ee2d2 100644 --- a/src/hook_redirect.rs +++ b/src/hook_redirect.rs @@ -50,12 +50,12 @@ fn is_spec_like_path(path: &str) -> bool { fn current_branch() -> Option { let cwd = std::env::current_dir().ok()?; - crate::git::current_branch(&cwd) + flare_git_core::branch::current_branch(&cwd) } fn default_branch() -> Option { let cwd = std::env::current_dir().ok()?; - Some(crate::git::resolve_default_branch(&cwd)) + Some(flare_git_core::branch::resolve_default_branch(&cwd)) } /// Pure decision core for the branch guard — no git process spawned here, so diff --git a/src/mcp_server.rs b/src/mcp_server.rs index 136cb69b..5a4c9dd8 100644 --- a/src/mcp_server.rs +++ b/src/mcp_server.rs @@ -419,7 +419,7 @@ impl AgentflareMcp { /// not on PATH, etc). Shared by `git_provenance` and the backend /// project-link resolution below. fn run_git(args: &[&str]) -> Option { - crate::git::run_in_opt(&std::env::current_dir().unwrap_or_default(), args) + flare_git_core::shell::run_in_opt(&std::env::current_dir().unwrap_or_default(), args) } /// Best-effort git context of this process's cwd (the project the MCP @@ -475,7 +475,7 @@ impl AgentflareMcp { /// found anywhere above it. pub(crate) fn repo_root() -> std::path::PathBuf { let cwd = std::env::current_dir().unwrap_or_default(); - if let Some(root) = crate::git::repo_toplevel(&cwd) { + if let Some(root) = flare_git_core::branch::repo_toplevel(&cwd) { return root; } Self::find_root_from(&cwd, &crate::paths::home()) @@ -820,7 +820,7 @@ impl AgentflareMcp { if let Some(pr) = pr.filter(|s| !s.is_empty()) { return Ok(pr); } - crate::git::current_branch(&std::env::current_dir().unwrap_or_default()) + flare_git_core::branch::current_branch(&std::env::current_dir().unwrap_or_default()) .ok_or_else(|| ErrorData::invalid_params("could not determine round — pass pr", None)) } diff --git a/src/mcp_server/flare_git.rs b/src/mcp_server/flare_git.rs index cf9c8afe..201f64fe 100644 --- a/src/mcp_server/flare_git.rs +++ b/src/mcp_server/flare_git.rs @@ -260,7 +260,7 @@ impl AgentflareMcp { None, )); } - crate::git::resolve_default_branch( + flare_git_core::branch::resolve_default_branch( &std::env::current_dir().unwrap_or_default(), ) } diff --git a/src/review.rs b/src/review.rs index b1d9e507..e5894370 100644 --- a/src/review.rs +++ b/src/review.rs @@ -452,7 +452,7 @@ pub fn submitter_name() -> String { /// Computes a unified diff via git for the local branch. `base`/`head` default /// to `master`/`HEAD` (three-dot: changes on HEAD since it diverged from base). pub fn compute_diff(base: Option<&str>, head: Option<&str>) -> Result { - crate::git::diff( + flare_git_core::shell::diff( &std::env::current_dir().unwrap_or_default(), base.unwrap_or("master"), head.unwrap_or("HEAD"), From 36f8008ba8d01040d9a28bc9f703d281b07a05bc Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 13:40:56 +0530 Subject: [PATCH 06/17] refactor(worktree): move worktree mechanics into flare-git-core, delete src/git.rs flare_git_core::worktree now owns create_worktree, already_isolated_for, ensure_worktrees_ignored, resolve_target_branch, run_output_timeout, and the target-dir isolation helpers -- moved verbatim from src/worktree.rs, plus a small Progress trait so this leaf crate doesn't need to know about the main binary's rmcp-based ProgressSender. push_and_open_pr is split: local push mechanics (commit-count guard, content-diff/squash-merge guard, the actual git push) become flare_git_core::worktree::push_branch, GitHub-free. Opening the PR stays a thin wrapper in src/worktree.rs, which now also implements the Progress trait for ProgressSender and re-exports resolve_target_branch -- so src/mcp_server/item.rs's crate::worktree::{resolve_target_branch, create_worktree, push_and_open_pr} call sites needed no changes at all. src/git.rs is now fully absorbed into flare-git-core (shell.rs/branch.rs) and deleted; its "mod git;" declaration is removed from main.rs. Full workspace build + test suite green. --- Cargo.lock | 2 + crates/flare-git-core/Cargo.toml | 2 + crates/flare-git-core/src/lib.rs | 1 + crates/flare-git-core/src/worktree.rs | 701 ++++++++++++++++++++++++++ src/git.rs | 177 ------- src/main.rs | 1 - src/worktree.rs | 673 +------------------------ 7 files changed, 724 insertions(+), 833 deletions(-) create mode 100644 crates/flare-git-core/src/worktree.rs delete mode 100644 src/git.rs diff --git a/Cargo.lock b/Cargo.lock index 09114375..d2e7b42e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1021,7 +1021,9 @@ checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582" name = "flare-git-core" version = "0.1.0" dependencies = [ + "agentflare-backend", "chrono", + "rusqlite", "serde", "serde_json", "tempfile", diff --git a/crates/flare-git-core/Cargo.toml b/crates/flare-git-core/Cargo.toml index dca65686..881d9486 100644 --- a/crates/flare-git-core/Cargo.toml +++ b/crates/flare-git-core/Cargo.toml @@ -11,6 +11,8 @@ publish = false serde = { version = "1", features = ["derive"] } serde_json = "1" chrono = "0.4" +rusqlite = { version = "0.40", features = ["bundled"] } +agentflare-backend = { package = "agentflare-backend", path = "../agentflare-backend" } [dev-dependencies] tempfile = "3" diff --git a/crates/flare-git-core/src/lib.rs b/crates/flare-git-core/src/lib.rs index 417d84e7..f876440f 100644 --- a/crates/flare-git-core/src/lib.rs +++ b/crates/flare-git-core/src/lib.rs @@ -6,3 +6,4 @@ pub mod branch; pub mod shell; +pub mod worktree; diff --git a/crates/flare-git-core/src/worktree.rs b/crates/flare-git-core/src/worktree.rs new file mode 100644 index 00000000..5ca8e860 --- /dev/null +++ b/crates/flare-git-core/src/worktree.rs @@ -0,0 +1,701 @@ +//! Worktree lifecycle management: isolates work items into per-branch git +//! worktrees, resolves target branches from parent item metadata, and keeps +//! each worktree's Cargo target dir isolated. +//! +//! GitHub-API concerns (opening a PR once a branch is pushed) are +//! deliberately NOT here — `push_branch` below only handles the local push +//! mechanics and returns the pushed branch name; opening the PR is the +//! caller's job (see the thin wrapper in the main binary's +//! `src/worktree.rs`), so this crate stays free of any GitHub dependency. + +use std::io::Read; +use std::path::{Path, PathBuf}; +use std::process::Command; +use std::time::Duration; + +use agentflare_backend::item::Item; + +use crate::branch::resolve_default_branch; +use crate::shell::{run_in as run_git_in, run_in_ok as run_git_in_ok}; + +/// Minimal progress-reporting interface — decouples this crate from the +/// main binary's MCP-specific `ProgressSender` (which depends on `rmcp`), +/// so this leaf crate has no reason to know anything about MCP. +pub trait Progress { + fn send(&self, progress: f64, total: Option, message: Option); +} + +pub fn resolve_target_branch(conn: &rusqlite::Connection, item: &Item, repo_root: &Path) -> String { + if let Some(ref parent_id) = item.parent_id + && let Ok(parent) = agentflare_backend::item::get(conn, parent_id) + && let Ok(meta) = serde_json::from_str::(&parent.metadata) + && let Some(branch) = meta.get("branch").and_then(|v| v.as_str()) + { + return branch.to_string(); + } + resolve_default_branch(repo_root) +} + +#[must_use] +pub fn already_isolated_for(branch: &str, repo_root: &Path) -> bool { + let git_dir = match run_git_in(repo_root, &["rev-parse", "--git-dir"]) { + Ok(d) => d, + Err(_) => return false, + }; + let common_dir = match run_git_in(repo_root, &["rev-parse", "--git-common-dir"]) { + Ok(d) => d, + Err(_) => return false, + }; + if git_dir == common_dir { + return false; + } + // Exits 0 with EMPTY stdout in a plain linked worktree (not a git + // submodule) — only a non-empty path means we're actually inside a + // submodule's own superproject relationship, which is the case this + // guard exists to rule out. + if let Ok(out) = run_git_in( + repo_root, + &["rev-parse", "--show-superproject-working-tree"], + ) && !out.is_empty() + { + return false; + } + match run_git_in(repo_root, &["branch", "--show-current"]) { + Ok(b) => b == branch, + Err(_) => false, + } +} + +/// Adds `.worktrees/` to this repo's LOCAL, untracked ignore rules +/// (`.git/info/exclude`) rather than the tracked `.gitignore` — a claim +/// should never create a commit in the caller's repository (would sweep up +/// any unrelated staged files, and any pre-existing uncommitted `.gitignore` +/// edits, into a commit the agent didn't ask for). +pub fn ensure_worktrees_ignored(repo_root: &Path) { + let Ok(common_dir) = run_git_in(repo_root, &["rev-parse", "--git-common-dir"]) else { + return; + }; + let exclude_path = repo_root.join(common_dir).join("info").join("exclude"); + if let Ok(existing) = std::fs::read_to_string(&exclude_path) + && existing + .lines() + .any(|l| l.trim() == ".worktrees/" || l.trim() == ".worktrees") + { + return; + } + let mut content = String::new(); + if let Ok(existing) = std::fs::read_to_string(&exclude_path) { + content = existing; + } + if !content.ends_with('\n') && !content.is_empty() { + content.push('\n'); + } + content.push_str(".worktrees/\n"); + if let Some(parent) = exclude_path.parent() { + let _ = std::fs::create_dir_all(parent); + } + if std::fs::write(&exclude_path, content).is_err() { + eprintln!("worktree: failed to write .git/info/exclude"); + } +} + +/// Warns (does not fail) when an ambient `CARGO_TARGET_DIR` is set in the +/// environment at claim time. A shared `CARGO_TARGET_DIR` across worktrees +/// is a silent correctness bug: Cargo's fingerprint hash omits the worktree +/// path, so two worktrees of the same repo reuse each other's stale local +/// crate artifacts (cargo #12516/#14053/#7740; OpenBlob #522). +/// +/// This function is the last-resort warning, not the fix — it only covers a +/// developer who opens a bare shell inside a worktree and runs `cargo` +/// directly, bypassing `agentflare run`. Per Cargo's precedence (CLI flag > +/// env var > config file), an ambient `CARGO_TARGET_DIR` *always* wins over +/// the `.cargo/config.toml` that `isolate_worktree_target_dir` writes, so in +/// that bypass case the isolated `target/` is silently shadowed and the bug +/// can still occur. +/// +/// Item #139 closed the two paths that matter for agents: `run_launch_env`/ +/// `run_headless` (src/agent_launch.rs) strip `CARGO_TARGET_DIR` from every +/// launched agent's child env — the only mechanism that actually outranks +/// the var — and CI (`.github/workflows/ci.yml`'s `target-dir-guard` job) +/// fails the build outright if the var is set project-wide. +fn warn_if_ambient_target_dir() { + if std::env::var_os("CARGO_TARGET_DIR").is_some() { + eprintln!( + "worktree: ambient CARGO_TARGET_DIR is set — it is SHARED across worktrees and \ + can leak stale artifacts between divergent checkouts. Prefer trusting CI for \ + local test builds, or unset it and rely on the worktree's isolated target dir." + ); + } +} + +/// Writes a per-worktree `.cargo/config.toml` so the worktree's `target/` +/// resolves locally instead of inheriting a shared `CARGO_TARGET_DIR`. +/// +/// Caveat: this only takes effect when `CARGO_TARGET_DIR` is *unset* in the +/// ambient environment — no config file can outrank the env var (Cargo's +/// precedence is CLI flag > env var > config file). A bare shell that +/// bypasses `agentflare run` still needs `warn_if_ambient_target_dir`'s +/// warning; every agent-launched build IS covered, since item #139 made +/// `run_launch_env`/`run_headless` (src/agent_launch.rs) strip the var from +/// the child env before it ever reaches Cargo, and CI enforces the same +/// invariant via the `target-dir-guard` job in ci.yml. +/// +/// Local workspace crates must NOT be shared across worktrees (silent +/// contamination); registry deps are safe but are better served by a shared +/// sccache. A relative `target-dir = "target"` resolves per-checkout, giving +/// each worktree its own isolated cache. Soft-fails (eprintln) — never blocks +/// a claim. +fn isolate_worktree_target_dir(worktree_path: &Path) { + let cargo_dir = worktree_path.join(".cargo"); + let _ = std::fs::create_dir_all(&cargo_dir); + let config_path = cargo_dir.join("config.toml"); + if config_path.exists() { + return; // don't clobber an intentional worktree-local override + } + let content = "[build]\n# Isolated per worktree (see item #133). Registry deps are\n\ + # better shared via sccache (RUSTC_WRAPPER + SCCACHE_BASEDIRS),\n\ + # not a shared CARGO_TARGET_DIR, which leaks artifacts across worktrees.\n\ + target-dir = \"target\"\n"; + if let Err(e) = std::fs::write(&config_path, content) { + eprintln!( + "worktree: could not write isolated .cargo/config.toml for {}: {e}", + worktree_path.display() + ); + } +} + +/// Creates an isolated git worktree for `item` against `target_branch`. +/// +/// Deliberately takes an already-resolved `target_branch` instead of a +/// database connection: callers should resolve the branch (`resolve_target_branch`, +/// above) while still holding whatever lock guards the database, then call +/// this *after* releasing it. `git worktree add` is a blocking +/// filesystem+subprocess operation with no business running while a shared +/// DB lock is held. +pub fn create_worktree( + item: &Item, + repo_root: &Path, + target_branch: &str, + progress: Option<&dyn Progress>, +) -> Option { + let branch = format!("task/{}", item.sequence_id); + let worktree_path = repo_root + .join(".worktrees") + .join("task") + .join(item.sequence_id.to_string()); + if already_isolated_for(&branch, repo_root) { + // Re-claiming an existing worktree: nothing to create, but still + // ensure its target dir is isolated (idempotent, no-op if present), + // and re-warn since the ambient env can still be shadowing it. + warn_if_ambient_target_dir(); + isolate_worktree_target_dir(&worktree_path); + return Some(worktree_path); + } + ensure_worktrees_ignored(repo_root); + if let Some(parent) = worktree_path.parent() { + let _ = std::fs::create_dir_all(parent); + } + warn_if_ambient_target_dir(); + if let Some(p) = progress { + p.send( + 0.0, + Some(1.0), + Some(format!( + "Creating isolated worktree for item {}...", + item.sequence_id + )), + ); + } + // Branch off the freshly-fetched remote ref when reachable, so a stale + // local checkout (e.g. hasn't pulled a just-merged PR) doesn't silently + // seed new work from old code. Soft-fails to today's local-ref behavior + // when there's no remote, we're offline, or the branch was never pushed + // (common for a parent item's task/N branch) — never blocks a claim on + // network reachability, matching every other soft-fail in this file. + // Routed through `run_output_timeout` (not the plain blocking + // `run_git_in`): an unreachable remote or a credential prompt must not + // be able to hang a claim indefinitely. + let fetch_timeout_secs = 30; + let start_point = match run_output_timeout( + "git", + &["fetch", "origin", target_branch], + repo_root, + fetch_timeout_secs, + ) { + Ok(out) + if out.status.success() + && run_git_in_ok( + repo_root, + &["rev-parse", "--verify", &format!("origin/{target_branch}")], + ) => + { + format!("origin/{target_branch}") + } + _ => { + eprintln!( + "worktree: could not fetch '{target_branch}' from origin, branching off the local ref instead" + ); + target_branch.to_string() + } + }; + match run_git_in( + repo_root, + &[ + "worktree", + "add", + &worktree_path.to_string_lossy(), + "-b", + &branch, + &start_point, + ], + ) { + Ok(_) => { + if let Some(p) = progress { + p.send(1.0, Some(1.0), Some("Worktree created".into())); + } + isolate_worktree_target_dir(&worktree_path); + Some(worktree_path) + } + Err(e) => { + eprintln!("worktree: creation skipped for item {}: {}", item.id, e); + None + } + } +} + +/// Kills `child` and its whole process tree — not just the direct child — +/// so a grandchild (e.g. a `git` credential helper) can't outlive a timeout. +fn kill_tree(child: &mut std::process::Child) { + #[cfg(unix)] + { + // `kill -KILL -` packs the signal and the (negative, i.e. + // process-group-targeting) pid into two separate `-`-prefixed argv + // entries. Some `kill` implementations misparse the second as + // another option rather than as the target once a signal option has + // already been consumed. `-s SIGNAME` plus a `--` end-of-options + // marker before the pid is the portable, unambiguous idiom. + let _ = Command::new("kill") + .arg("-s") + .arg("KILL") + .arg("--") + .arg(format!("-{}", child.id())) + .status(); + } + #[cfg(windows)] + { + let _ = Command::new("taskkill") + .args(["/T", "/F", "/PID", &child.id().to_string()]) + .status(); + } + #[cfg(not(any(unix, windows)))] + { + let _ = child.kill(); + } +} + +/// Runs `program` with a deadline, returning its output. Puts the child in +/// its own process group (Unix) and kills that whole group — not just the +/// direct child — if it outlives `timeout_secs`, via `kill_tree`; a plain +/// `child.kill()` would leave a grandchild (e.g. a `git` credential helper) +/// 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( + program: &str, + args: &[&str], + cwd: &Path, + timeout_secs: u64, +) -> Result { + let mut cmd = Command::new(program); + cmd.args(args) + .current_dir(cwd) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()); + #[cfg(unix)] + { + use std::os::unix::process::CommandExt; + cmd.process_group(0); + } + let mut child = cmd + .spawn() + .map_err(|e| format!("{program}: spawn failed: {e}"))?; + let mut stdout_pipe = child.stdout.take().expect("stdout piped above"); + let mut stderr_pipe = child.stderr.take().expect("stderr piped above"); + let stdout_reader = std::thread::spawn(move || { + let mut buf = Vec::new(); + let _ = stdout_pipe.read_to_end(&mut buf); + buf + }); + let stderr_reader = std::thread::spawn(move || { + let mut buf = Vec::new(); + let _ = stderr_pipe.read_to_end(&mut buf); + buf + }); + let deadline = std::time::Instant::now() + Duration::from_secs(timeout_secs); + let status = loop { + match child.try_wait() { + Ok(Some(status)) => break status, + Ok(None) => { + if std::time::Instant::now() >= deadline { + kill_tree(&mut child); + let _ = child.wait(); + return Err(format!("{program} timed out after {timeout_secs}s")); + } + std::thread::sleep(Duration::from_millis(50)); + } + Err(e) => return Err(format!("{program}: {e}")), + } + }; + Ok(std::process::Output { + status, + stdout: stdout_reader.join().unwrap_or_default(), + stderr: stderr_reader.join().unwrap_or_default(), + }) +} + +/// Pushes `item`'s isolated worktree branch to `target_branch`'s remote, if +/// the branch exists, has new commits, and its content isn't already fully +/// present on the target (squash-merge guard). Returns the pushed branch +/// name on success. Soft-fails (eprintln, no error surfaced, returns +/// `None`) on any failure — nothing here should block `done` since the +/// item's completion is already committed to the DB by the time this runs. +/// +/// Deliberately does NOT open a PR — that's a GitHub-API concern kept out +/// of this crate; see the thin wrapper in the main binary's +/// `src/worktree.rs::push_and_open_pr`. +pub fn push_branch( + item: &Item, + repo_root: &Path, + target_branch: &str, + progress: Option<&dyn Progress>, +) -> Option { + let branch = format!("task/{}", item.sequence_id); + let worktree_path = repo_root + .join(".worktrees") + .join("task") + .join(item.sequence_id.to_string()); + if !worktree_path.exists() { + return None; // nothing was ever claimed into a worktree for this item + } + // Nothing to push (and nothing worth a PR) if the branch never + // diverged from its target — e.g. `done` called with no commits made. + match run_git_in( + repo_root, + &["rev-list", "--count", &format!("{target_branch}..{branch}")], + ) { + Ok(count) if count != "0" => {} + _ => return None, + } + // Content-diff guard: even when the branch has new commits, its + // *content* may already be on the target (squash-merge). Compares + // target→branch tree (two-dot, not three-dot: we want whether the + // two tips are identical, not whether branch differs from merge-base). + if run_git_in_ok( + repo_root, + &["diff", "--quiet", &format!("{target_branch}..{branch}")], + ) { + return None; + } + if let Some(p) = progress { + p.send(0.0, Some(1.0), Some(format!("Pushing branch {branch}..."))); + } + let push_timeout = 120; + match run_output_timeout( + "git", + &["push", "-u", "origin", &branch], + repo_root, + push_timeout, + ) { + Ok(out) if !out.status.success() => { + eprintln!( + "worktree: push skipped for item {}: {}", + item.id, + String::from_utf8_lossy(&out.stderr).trim() + ); + return None; + } + Err(e) => { + eprintln!("worktree: push skipped for item {}: {e}", item.id); + return None; + } + _ => {} + } + Some(branch) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::shell::test_support::{Repo, init_repo_with_branch}; + use tempfile::TempDir; + + fn init_repo() -> Repo { + init_repo_with_branch("master") + } + + fn test_item(sequence_id: i64) -> Item { + Item { + id: "test-id".into(), + project_id: "proj".into(), + state_id: "state".into(), + name: "test".into(), + description: String::new(), + priority: "none".into(), + parent_id: None, + assignee_agent: None, + sequence_id, + sort_order: 0.0, + started_at: None, + completed_at: None, + archived_at: None, + external_source: None, + external_id: None, + metadata: "{}".into(), + created_at: 0, + updated_at: 0, + deleted_at: None, + } + } + + #[test] + fn ensure_worktrees_ignored_is_noop_when_already_ignored() { + let repo = init_repo(); + let exclude_path = repo.path.join(".git").join("info").join("exclude"); + std::fs::create_dir_all(exclude_path.parent().unwrap()).unwrap(); + std::fs::write(&exclude_path, ".worktrees/\n").unwrap(); + let before = std::fs::read_to_string(&exclude_path).unwrap(); + ensure_worktrees_ignored(&repo.path); + let after = std::fs::read_to_string(&exclude_path).unwrap(); + assert_eq!(before, after); + } + + #[test] + fn ensure_worktrees_ignored_adds_to_local_exclude_without_committing() { + let repo = init_repo(); + ensure_worktrees_ignored(&repo.path); + let exclude_path = repo.path.join(".git").join("info").join("exclude"); + let content = std::fs::read_to_string(&exclude_path).unwrap(); + assert!(content.contains(".worktrees/")); + // Must never touch the tracked .gitignore or create a commit. + assert!(!repo.path.join(".gitignore").exists()); + let log = run_git_in(&repo.path, &["log", "--oneline"]).unwrap(); + assert_eq!( + log.lines().count(), + 1, + "no new commit should have been made" + ); + } + + #[test] + fn already_isolated_for_false_in_regular_repo() { + let repo = init_repo(); + assert!(!already_isolated_for("task/1", &repo.path)); + } + + #[test] + fn isolate_worktree_target_dir_writes_relative_target_dir() { + let tmp = TempDir::new().unwrap(); + let wt = tmp.path().join(".worktrees").join("task").join("1"); + std::fs::create_dir_all(&wt).unwrap(); + isolate_worktree_target_dir(&wt); + let config = wt.join(".cargo").join("config.toml"); + assert!(config.exists(), "expected .cargo/config.toml in worktree"); + let content = std::fs::read_to_string(&config).unwrap(); + assert!( + content.contains("target-dir = \"target\""), + "must set a relative, per-checkout target dir, got: {content}" + ); + assert!( + !content.contains("target-dir = \"/") + && !content.contains("target-dir = \"~") + && !content.contains("CARGO_TARGET_DIR ="), + "must not set an absolute/shared target dir" + ); + } + + #[test] + fn isolate_worktree_target_dir_does_not_clobber_existing_config() { + let tmp = TempDir::new().unwrap(); + let wt = tmp.path().join(".worktrees").join("task").join("1"); + let cargo_dir = wt.join(".cargo"); + std::fs::create_dir_all(&cargo_dir).unwrap(); + let config = cargo_dir.join("config.toml"); + std::fs::write( + &config, + "[build]\ntarget-dir = \"/some/intentional/path\"\n", + ) + .unwrap(); + isolate_worktree_target_dir(&wt); + let content = std::fs::read_to_string(&config).unwrap(); + assert!( + content.contains("/some/intentional/path"), + "existing worktree-local config must be preserved" + ); + } + + #[test] + fn warn_if_ambient_target_dir_warns_when_set() { + // Just asserts the function runs without panicking whether or not the + // var is set; the warning is an ephemeral eprintln, not assertable here. + unsafe { + std::env::set_var("CARGO_TARGET_DIR", "/tmp/shared"); + } + warn_if_ambient_target_dir(); + unsafe { + std::env::remove_var("CARGO_TARGET_DIR"); + } + warn_if_ambient_target_dir(); + } + + #[test] + fn already_isolated_for_true_inside_the_worktree_it_created() { + let repo = init_repo(); + let item = test_item(1); + let target = resolve_default_branch(&repo.path); + let worktree_path = create_worktree(&item, &repo.path, &target, None).unwrap(); + assert!(already_isolated_for("task/1", &worktree_path)); + } + + #[test] + fn create_worktree_creates_worktree_and_branch() { + let repo = init_repo(); + let worktree_path = repo.path.join(".worktrees").join("task").join("1"); + let item = test_item(1); + let target = resolve_default_branch(&repo.path); + let result = create_worktree(&item, &repo.path, &target, None); + assert!(result.is_some()); + assert!(worktree_path.exists()); + } + + #[test] + fn create_worktree_soft_fails_on_bad_git() { + let tmp = TempDir::new().unwrap(); + let bad_root = tmp.path().join("not-a-repo"); + std::fs::create_dir_all(&bad_root).unwrap(); + let item = test_item(1); + let result = create_worktree(&item, &bad_root, "master", None); + assert!(result.is_none()); + } + + #[test] + fn create_worktree_fetches_target_branch_and_includes_remote_only_commits() { + // "remote" — plays the role of `origin`. + let remote = init_repo(); + // "local" — a clone that will go stale the moment `remote` gets a + // new commit; this is what `create_worktree` actually operates on. + let local_container = TempDir::new().unwrap(); + let local_path = local_container.path().join("local"); + run_git_in( + local_container.path(), + &[ + "clone", + remote.path.to_str().unwrap(), + local_path.to_str().unwrap(), + ], + ) + .unwrap(); + run_git_in(&local_path, &["config", "user.email", "test@test.com"]).unwrap(); + run_git_in(&local_path, &["config", "user.name", "Test"]).unwrap(); + + // Lands on the remote *after* the clone — local's own `master` and + // `origin/master` are both stale relative to this. + run_git_in( + &remote.path, + &["commit", "--allow-empty", "-m", "remote-only commit"], + ) + .unwrap(); + let remote_head = run_git_in(&remote.path, &["rev-parse", "HEAD"]).unwrap(); + + let item = test_item(1); + let worktree_path = create_worktree(&item, &local_path, "master", None).unwrap(); + let worktree_head = run_git_in(&worktree_path, &["rev-parse", "HEAD"]).unwrap(); + + assert_eq!( + worktree_head, remote_head, + "worktree must be based on the freshly-fetched remote commit, not the stale local ref" + ); + } + + #[test] + fn push_branch_returns_none_when_no_worktree_exists() { + let repo = init_repo(); + let item = test_item(1); + assert!(push_branch(&item, &repo.path, "master", None).is_none()); + } + + #[test] + fn push_branch_returns_none_when_branch_has_no_new_commits() { + let repo = init_repo(); + let item = test_item(1); + let target = resolve_default_branch(&repo.path); + create_worktree(&item, &repo.path, &target, None).unwrap(); + // No commits were made in the worktree — nothing to push, so this + // must return early without attempting a real `git push` (which + // would fail anyway: no remote configured here). + assert!(push_branch(&item, &repo.path, &target, None).is_none()); + } + + #[test] + fn push_branch_returns_none_when_branch_content_already_merged() { + let repo = init_repo(); + let item = test_item(1); + let target = resolve_default_branch(&repo.path); + let worktree_path = create_worktree(&item, &repo.path, &target, None).unwrap(); + let test_file = worktree_path.join("test.txt"); + std::fs::write(&test_file, b"hello").unwrap(); + run_git_in(&worktree_path, &["add", "test.txt"]).unwrap(); + run_git_in(&worktree_path, &["commit", "-m", "worktree change"]).unwrap(); + // task/1 now has a commit master doesn't. Squash-merge: cherry-pick + // the *diff* onto master so content matches but ancestry doesn't. + run_git_in(&repo.path, &["cherry-pick", "-n", "task/1"]).unwrap(); + run_git_in(&repo.path, &["commit", "-m", "squash-merge"]).unwrap(); + // Content-diff guard should catch this even though commit-count + // guard passes. + assert!(push_branch(&item, &repo.path, &target, None).is_none()); + } + + #[test] + fn run_output_timeout_kills_the_child_not_just_abandons_it() { + let tmp = tempfile::tempdir().unwrap(); + let marker = tmp.path().join("marker"); + // A command that, left alone, outlives our 1s timeout and then + // writes `marker`. If the timeout only abandoned the child (the bug + // this replaced) rather than killing it, the marker would still + // show up once the sleep finishes on its own. + #[cfg(unix)] + let (program, owned_args): (&str, Vec) = ( + "sh", + vec![ + "-c".into(), + format!("sleep 3 && touch {}", marker.display()), + ], + ); + #[cfg(windows)] + let (program, owned_args): (&str, Vec) = ( + "cmd", + vec![ + "/C".into(), + format!( + "ping 127.0.0.1 -n 4 >NUL & echo done > {}", + marker.display() + ), + ], + ); + let args: Vec<&str> = owned_args.iter().map(String::as_str).collect(); + + let result = run_output_timeout(program, &args, tmp.path(), 1); + assert!( + matches!(&result, Err(e) if e.contains("timed out")), + "{result:?}" + ); + + // Give the command's natural (un-killed) duration time to elapse + // before checking the marker never showed up. + std::thread::sleep(Duration::from_secs(4)); + assert!( + !marker.exists(), + "child kept running after the timeout — it was abandoned, not killed" + ); + } +} diff --git a/src/git.rs b/src/git.rs deleted file mode 100644 index 4de10e0d..00000000 --- a/src/git.rs +++ /dev/null @@ -1,177 +0,0 @@ -//! Shared git-shelling primitives. Every module that needs to run `git` -//! against a repo goes through here instead of hand-rolling its own -//! `Command::new("git")` wrapper — this consolidates what had grown into -//! 5+ near-duplicate copies of the same "spawn git, trim stdout" logic -//! across mcp_server.rs, worktree.rs, hook_redirect.rs, gateway_integrations.rs, -//! and cli/review.rs. - -use std::path::{Path, PathBuf}; -use std::process::Command; - -/// Runs `git` in `repo_root`; `Ok(stdout)` trimmed on success, `Err(stderr)` -/// trimmed on a non-zero exit, or a process-spawn error message (git -/// missing, etc) if it couldn't even run. -pub fn run_in(repo_root: &Path, args: &[&str]) -> Result { - let out = Command::new("git") - .args(args) - .current_dir(repo_root) - .output() - .map_err(|e| format!("git not available: {e}"))?; - if !out.status.success() { - return Err(String::from_utf8_lossy(&out.stderr).trim().to_string()); - } - Ok(String::from_utf8_lossy(&out.stdout).trim().to_string()) -} - -/// `run_in`, discarding the error and treating empty stdout as `None` — the -/// "best-effort, don't care why it failed" shape most callers actually want. -pub fn run_in_opt(repo_root: &Path, args: &[&str]) -> Option { - run_in(repo_root, args).ok().filter(|s| !s.is_empty()) -} - -/// `true` if `git ` exits 0 in `repo_root`; stdout/stderr don't matter. -pub fn run_in_ok(repo_root: &Path, args: &[&str]) -> bool { - run_in(repo_root, args).is_ok() -} - -/// Current branch name (`HEAD` in detached-HEAD state). `None` outside a git -/// repo or if git isn't on `PATH`. -pub fn current_branch(repo_root: &Path) -> Option { - run_in_opt(repo_root, &["rev-parse", "--abbrev-ref", "HEAD"]) -} - -/// Best-effort resolution of "the" default branch: prefer the remote's own -/// record of it (`origin/HEAD`'s symbolic ref, which survives a repo default -/// named anything other than main/master), then whichever of main/master -/// actually exists as a local branch, then whatever is actually checked out -/// here — so a repo naming its default branch e.g. `trunk`/`develop` still -/// resolves instead of falling through to a hardcoded guess that may not -/// even exist. -pub fn resolve_default_branch(repo_root: &Path) -> String { - if let Some(origin_head) = run_in_opt( - repo_root, - &["symbolic-ref", "--short", "refs/remotes/origin/HEAD"], - ) && let Some(stripped) = origin_head.strip_prefix("origin/") - { - return stripped.to_string(); - } - if run_in_ok(repo_root, &["rev-parse", "--verify", "main"]) { - return "main".to_string(); - } - if run_in_ok(repo_root, &["rev-parse", "--verify", "master"]) { - return "master".to_string(); - } - run_in_opt(repo_root, &["symbolic-ref", "--short", "HEAD"]) - .unwrap_or_else(|| "master".to_string()) -} - -/// `git rev-parse --show-toplevel` from `start` — handles worktrees/submodules -/// correctly, works regardless of subdirectory. `None` outside a git repo. -pub fn repo_toplevel(start: &Path) -> Option { - run_in_opt(start, &["rev-parse", "--show-toplevel"]).map(PathBuf::from) -} - -/// Unified diff for `base...head` (three-dot: changes on `head` since it -/// diverged from `base`). Stdout is returned RAW, not trimmed — diff output -/// is multi-line and whitespace-significant, unlike the single-value queries -/// the rest of this module's helpers return. -pub fn diff(repo_root: &Path, base: &str, head: &str) -> Result { - let range = format!("{base}...{head}"); - let out = Command::new("git") - .args(["diff", "--unified=3", &range]) - .current_dir(repo_root) - .output() - .map_err(|e| format!("git diff failed: {e}"))?; - if !out.status.success() { - return Err(format!( - "git diff {range}: {}", - String::from_utf8_lossy(&out.stderr).trim() - )); - } - Ok(String::from_utf8_lossy(&out.stdout).to_string()) -} - -#[cfg(test)] -mod tests { - use super::*; - use tempfile::TempDir; - - struct Repo { - _dir: TempDir, - path: PathBuf, - } - - fn init_repo_with_branch(branch: &str) -> Repo { - let dir = TempDir::new().unwrap(); - let path = dir.path().to_path_buf(); - run_in(&path, &["init", "-b", branch]).unwrap(); - run_in(&path, &["config", "user.email", "test@test.com"]).unwrap(); - run_in(&path, &["config", "user.name", "Test"]).unwrap(); - run_in(&path, &["commit", "--allow-empty", "-m", "initial"]).unwrap(); - Repo { _dir: dir, path } - } - - #[test] - fn run_in_opt_is_none_outside_a_repo() { - let dir = TempDir::new().unwrap(); - assert!(run_in_opt(dir.path(), &["rev-parse", "--abbrev-ref", "HEAD"]).is_none()); - } - - #[test] - fn run_in_ok_reflects_exit_status() { - let repo = init_repo_with_branch("master"); - assert!(run_in_ok(&repo.path, &["rev-parse", "--verify", "master"])); - assert!(!run_in_ok( - &repo.path, - &["rev-parse", "--verify", "no-such-branch"] - )); - } - - #[test] - fn current_branch_reports_the_checked_out_branch() { - let repo = init_repo_with_branch("feature/x"); - assert_eq!(current_branch(&repo.path).as_deref(), Some("feature/x")); - } - - #[test] - fn resolve_default_branch_resolves_from_origin_head() { - let repo = init_repo_with_branch("master"); - assert_eq!(resolve_default_branch(&repo.path), "master"); - } - - #[test] - fn resolve_default_branch_falls_back_to_actual_head_for_nonstandard_names() { - // No origin, no "main", no "master" — must not guess "master" when - // the repo's real default branch is named something else entirely. - let repo = init_repo_with_branch("trunk"); - assert_eq!(resolve_default_branch(&repo.path), "trunk"); - } - - #[test] - fn repo_toplevel_finds_root_from_a_subdirectory() { - let repo = init_repo_with_branch("master"); - let sub = repo.path.join("sub"); - std::fs::create_dir(&sub).unwrap(); - assert_eq!( - repo_toplevel(&sub).map(|p| p.canonicalize().unwrap()), - repo.path.canonicalize().ok(), - ); - } - - #[test] - fn diff_returns_untrimmed_output_across_a_change() { - let repo = init_repo_with_branch("master"); - std::fs::write(repo.path.join("f.txt"), "hello\n").unwrap(); - run_in(&repo.path, &["add", "f.txt"]).unwrap(); - run_in(&repo.path, &["commit", "-m", "add f.txt"]).unwrap(); - let out = diff(&repo.path, "HEAD~1", "HEAD").unwrap(); - assert!(out.contains("+hello"), "{out}"); - } - - #[test] - fn diff_reports_git_stderr_on_an_invalid_range() { - let repo = init_repo_with_branch("master"); - let err = diff(&repo.path, "no-such-branch", "HEAD").unwrap_err(); - assert!(err.contains("no-such-branch"), "{err}"); - } -} diff --git a/src/main.rs b/src/main.rs index 828e14b8..9e719f7d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -29,7 +29,6 @@ mod dev_vars; mod errors; mod gateway_integrations; mod gateway_secrets; -mod git; mod github; mod hook; mod hook_redirect; diff --git a/src/worktree.rs b/src/worktree.rs index 1bf8e043..c04e9b76 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -1,317 +1,34 @@ -use std::io::Read; -use std::path::Path; -use std::path::PathBuf; -use std::process::Command; -use std::time::Duration; +//! Thin wrapper around `flare_git_core::worktree` — the local git/worktree +//! mechanics live there now. This file only adds what that leaf crate +//! deliberately does NOT know about: the main binary's MCP-specific +//! `ProgressSender` (depends on `rmcp`), and opening a GitHub PR once a +//! branch is pushed (depends on `src/github`, a GitHub-REST concern kept +//! out of flare-git-core on purpose). -use crate::progress::ProgressSender; +use std::path::{Path, PathBuf}; -use crate::git::resolve_default_branch; -use crate::git::run_in as run_git_in; -use crate::git::run_in_ok as run_git_in_ok; use crate::github::identity::RepoId; +use crate::progress::ProgressSender; -pub fn resolve_target_branch( - conn: &rusqlite::Connection, - item: &agentflare_backend::item::Item, - repo_root: &Path, -) -> String { - if let Some(ref parent_id) = item.parent_id - && let Ok(parent) = agentflare_backend::item::get(conn, parent_id) - && let Ok(meta) = serde_json::from_str::(&parent.metadata) - && let Some(branch) = meta.get("branch").and_then(|v| v.as_str()) - { - return branch.to_string(); +impl flare_git_core::worktree::Progress for ProgressSender { + fn send(&self, progress: f64, total: Option, message: Option) { + ProgressSender::send(self, progress, total, message); } - resolve_default_branch(repo_root) } -pub fn already_isolated_for(branch: &str, repo_root: &Path) -> bool { - let git_dir = match run_git_in(repo_root, &["rev-parse", "--git-dir"]) { - Ok(d) => d, - Err(_) => return false, - }; - let common_dir = match run_git_in(repo_root, &["rev-parse", "--git-common-dir"]) { - Ok(d) => d, - Err(_) => return false, - }; - if git_dir == common_dir { - return false; - } - // Exits 0 with EMPTY stdout in a plain linked worktree (not a git - // submodule) — only a non-empty path means we're actually inside a - // submodule's own superproject relationship, which is the case this - // guard exists to rule out. - if let Ok(out) = run_git_in( - repo_root, - &["rev-parse", "--show-superproject-working-tree"], - ) && !out.is_empty() - { - return false; - } - match run_git_in(repo_root, &["branch", "--show-current"]) { - Ok(b) => b == branch, - Err(_) => false, - } +fn as_progress(p: Option<&ProgressSender>) -> Option<&dyn flare_git_core::worktree::Progress> { + p.map(|p| p as &dyn flare_git_core::worktree::Progress) } -/// Adds `.worktrees/` to this repo's LOCAL, untracked ignore rules -/// (`.git/info/exclude`) rather than the tracked `.gitignore` — a claim -/// should never create a commit in the caller's repository (would sweep up -/// any unrelated staged files, and any pre-existing uncommitted `.gitignore` -/// edits, into a commit the agent didn't ask for). -pub fn ensure_worktrees_ignored(repo_root: &Path) { - let Ok(common_dir) = run_git_in(repo_root, &["rev-parse", "--git-common-dir"]) else { - return; - }; - let exclude_path = repo_root.join(common_dir).join("info").join("exclude"); - if let Ok(existing) = std::fs::read_to_string(&exclude_path) - && existing - .lines() - .any(|l| l.trim() == ".worktrees/" || l.trim() == ".worktrees") - { - return; - } - let mut content = String::new(); - if let Ok(existing) = std::fs::read_to_string(&exclude_path) { - content = existing; - } - if !content.ends_with('\n') && !content.is_empty() { - content.push('\n'); - } - content.push_str(".worktrees/\n"); - if let Some(parent) = exclude_path.parent() { - let _ = std::fs::create_dir_all(parent); - } - if std::fs::write(&exclude_path, content).is_err() { - eprintln!("worktree: failed to write .git/info/exclude"); - } -} - -/// Warns (does not fail) when an ambient `CARGO_TARGET_DIR` is set in the -/// environment at claim time. A shared `CARGO_TARGET_DIR` across worktrees -/// is a silent correctness bug: Cargo's fingerprint hash omits the worktree -/// path, so two worktrees of the same repo reuse each other's stale local -/// crate artifacts (cargo #12516/#14053/#7740; OpenBlob #522). -/// -/// This function is the last-resort warning, not the fix — it only covers a -/// developer who opens a bare shell inside a worktree and runs `cargo` -/// directly, bypassing `agentflare run`. Per Cargo's precedence (CLI flag > -/// env var > config file), an ambient `CARGO_TARGET_DIR` *always* wins over -/// the `.cargo/config.toml` that `isolate_worktree_target_dir` writes, so in -/// that bypass case the isolated `target/` is silently shadowed and the bug -/// can still occur. -/// -/// Item #139 closed the two paths that matter for agents: `run_launch_env`/ -/// `run_headless` (src/agent_launch.rs) strip `CARGO_TARGET_DIR` from every -/// launched agent's child env — the only mechanism that actually outranks -/// the var — and CI (`.github/workflows/ci.yml`'s `target-dir-guard` job) -/// fails the build outright if the var is set project-wide. -fn warn_if_ambient_target_dir() { - if std::env::var_os("CARGO_TARGET_DIR").is_some() { - eprintln!( - "worktree: ambient CARGO_TARGET_DIR is set — it is SHARED across worktrees and \ - can leak stale artifacts between divergent checkouts. Prefer trusting CI for \ - local test builds, or unset it and rely on the worktree's isolated target dir." - ); - } -} - -/// Writes a per-worktree `.cargo/config.toml` so the worktree's `target/` -/// resolves locally instead of inheriting a shared `CARGO_TARGET_DIR`. -/// -/// Caveat: this only takes effect when `CARGO_TARGET_DIR` is *unset* in the -/// ambient environment — no config file can outrank the env var (Cargo's -/// precedence is CLI flag > env var > config file). A bare shell that -/// bypasses `agentflare run` still needs `warn_if_ambient_target_dir`'s -/// warning; every agent-launched build IS covered, since item #139 made -/// `run_launch_env`/`run_headless` (src/agent_launch.rs) strip the var from -/// the child env before it ever reaches Cargo, and CI enforces the same -/// invariant via the `target-dir-guard` job in ci.yml. -/// -/// Local workspace crates must NOT be shared across worktrees (silent -/// contamination); registry deps are safe but are better served by a shared -/// sccache. A relative `target-dir = "target"` resolves per-checkout, giving -/// each worktree its own isolated cache. Soft-fails (eprintln) — never blocks -/// a claim. -fn isolate_worktree_target_dir(worktree_path: &Path) { - let cargo_dir = worktree_path.join(".cargo"); - let _ = std::fs::create_dir_all(&cargo_dir); - let config_path = cargo_dir.join("config.toml"); - if config_path.exists() { - return; // don't clobber an intentional worktree-local override - } - let content = "[build]\n# Isolated per worktree (see item #133). Registry deps are\n\ - # better shared via sccache (RUSTC_WRAPPER + SCCACHE_BASEDIRS),\n\ - # not a shared CARGO_TARGET_DIR, which leaks artifacts across worktrees.\n\ - target-dir = \"target\"\n"; - if let Err(e) = std::fs::write(&config_path, content) { - eprintln!( - "worktree: could not write isolated .cargo/config.toml for {}: {e}", - worktree_path.display() - ); - } -} +pub use flare_git_core::worktree::resolve_target_branch; -/// Creates an isolated git worktree for `item` against `target_branch`. -/// -/// Deliberately takes an already-resolved `target_branch` instead of a -/// database connection: callers should resolve the branch (`resolve_target_branch`, -/// above) while still holding whatever lock guards the database, then call -/// this *after* releasing it. `git worktree add` is a blocking -/// filesystem+subprocess operation with no business running while a shared -/// DB lock is held. pub fn create_worktree( item: &agentflare_backend::item::Item, repo_root: &Path, target_branch: &str, progress: Option<&ProgressSender>, ) -> Option { - let branch = format!("task/{}", item.sequence_id); - let worktree_path = repo_root - .join(".worktrees") - .join("task") - .join(item.sequence_id.to_string()); - if already_isolated_for(&branch, repo_root) { - // Re-claiming an existing worktree: nothing to create, but still - // ensure its target dir is isolated (idempotent, no-op if present), - // and re-warn since the ambient env can still be shadowing it. - warn_if_ambient_target_dir(); - isolate_worktree_target_dir(&worktree_path); - return Some(worktree_path); - } - ensure_worktrees_ignored(repo_root); - if let Some(parent) = worktree_path.parent() { - let _ = std::fs::create_dir_all(parent); - } - warn_if_ambient_target_dir(); - if let Some(p) = progress { - p.send( - 0.0, - Some(1.0), - Some(format!( - "Creating isolated worktree for item {}...", - item.sequence_id - )), - ); - } - // Branch off the freshly-fetched remote ref when reachable, so a stale - // local checkout (e.g. hasn't pulled a just-merged PR) doesn't silently - // seed new work from old code. Soft-fails to today's local-ref behavior - // when there's no remote, we're offline, or the branch was never pushed - // (common for a parent item's task/N branch) — never blocks a claim on - // network reachability, matching every other soft-fail in this file. - // Routed through `run_output_timeout` (not the plain blocking - // `run_git_in`): an unreachable remote or a credential prompt must not - // be able to hang a claim indefinitely. - let fetch_timeout_secs = 30; - let start_point = match run_output_timeout( - "git", - &["fetch", "origin", target_branch], - repo_root, - fetch_timeout_secs, - ) { - Ok(out) - if out.status.success() - && run_git_in_ok( - repo_root, - &["rev-parse", "--verify", &format!("origin/{target_branch}")], - ) => - { - format!("origin/{target_branch}") - } - _ => { - eprintln!( - "worktree: could not fetch '{target_branch}' from origin, branching off the local ref instead" - ); - target_branch.to_string() - } - }; - match run_git_in( - repo_root, - &[ - "worktree", - "add", - &worktree_path.to_string_lossy(), - "-b", - &branch, - &start_point, - ], - ) { - Ok(_) => { - if let Some(p) = progress { - p.send(1.0, Some(1.0), Some("Worktree created".into())); - } - isolate_worktree_target_dir(&worktree_path); - Some(worktree_path) - } - Err(e) => { - eprintln!("worktree: creation skipped for item {}: {}", item.id, e); - None - } - } -} - -/// Runs `program` with a deadline, returning its output. Puts the child in -/// its own process group (Unix) and kills that whole group — not just the -/// direct child — if it outlives `timeout_secs`, via the same `kill_tree` -/// used for headless agent runs; a plain `child.kill()` would leave a -/// grandchild (e.g. a `git` credential helper) 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( - program: &str, - args: &[&str], - cwd: &Path, - timeout_secs: u64, -) -> Result { - let mut cmd = Command::new(program); - cmd.args(args) - .current_dir(cwd) - .stdin(std::process::Stdio::null()) - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()); - #[cfg(unix)] - { - use std::os::unix::process::CommandExt; - cmd.process_group(0); - } - let mut child = cmd - .spawn() - .map_err(|e| format!("{program}: spawn failed: {e}"))?; - let mut stdout_pipe = child.stdout.take().expect("stdout piped above"); - let mut stderr_pipe = child.stderr.take().expect("stderr piped above"); - let stdout_reader = std::thread::spawn(move || { - let mut buf = Vec::new(); - let _ = stdout_pipe.read_to_end(&mut buf); - buf - }); - let stderr_reader = std::thread::spawn(move || { - let mut buf = Vec::new(); - let _ = stderr_pipe.read_to_end(&mut buf); - buf - }); - let deadline = std::time::Instant::now() + Duration::from_secs(timeout_secs); - let status = loop { - match child.try_wait() { - Ok(Some(status)) => break status, - Ok(None) => { - if std::time::Instant::now() >= deadline { - crate::agent_launch::kill_tree(&mut child); - let _ = child.wait(); - return Err(format!("{program} timed out after {timeout_secs}s")); - } - std::thread::sleep(Duration::from_millis(50)); - } - Err(e) => return Err(format!("{program}: {e}")), - } - }; - Ok(std::process::Output { - status, - stdout: stdout_reader.join().unwrap_or_default(), - stderr: stderr_reader.join().unwrap_or_default(), - }) + flare_git_core::worktree::create_worktree(item, repo_root, target_branch, as_progress(progress)) } /// Pushes `item`'s isolated worktree branch and opens a PR against @@ -320,65 +37,15 @@ fn run_output_timeout( /// target branch automatically, so the worktree/branch are left in place /// for the PR to actually get reviewed and merged. Soft-fails (eprintln, no /// error surfaced, returns `None`) on any failure — nothing here, including -/// `gh` being unavailable, should block `done` since the item's completion -/// is already committed to the DB by the time this runs. +/// `gh`/GitHub credentials being unavailable, should block `done` since the +/// item's completion is already committed to the DB by the time this runs. pub fn push_and_open_pr( item: &agentflare_backend::item::Item, repo_root: &Path, target_branch: &str, progress: Option<&ProgressSender>, ) -> Option { - let branch = format!("task/{}", item.sequence_id); - let worktree_path = repo_root - .join(".worktrees") - .join("task") - .join(item.sequence_id.to_string()); - if !worktree_path.exists() { - return None; // nothing was ever claimed into a worktree for this item - } - // Nothing to push (and nothing worth a PR) if the branch never - // diverged from its target — e.g. `done` called with no commits made. - match run_git_in( - repo_root, - &["rev-list", "--count", &format!("{target_branch}..{branch}")], - ) { - Ok(count) if count != "0" => {} - _ => return None, - } - // Content-diff guard: even when the branch has new commits, its - // *content* may already be on the target (squash-merge). Compares - // target→branch tree (two-dot, not three-dot: we want whether the - // two tips are identical, not whether branch differs from merge-base). - if run_git_in_ok( - repo_root, - &["diff", "--quiet", &format!("{target_branch}..{branch}")], - ) { - return None; - } - if let Some(p) = progress { - p.send(0.0, Some(1.0), Some(format!("Pushing branch {branch}..."))); - } - let push_timeout = 120; - match run_output_timeout( - "git", - &["push", "-u", "origin", &branch], - repo_root, - push_timeout, - ) { - Ok(out) if !out.status.success() => { - eprintln!( - "worktree: push skipped for item {}: {}", - item.id, - String::from_utf8_lossy(&out.stderr).trim() - ); - return None; - } - Err(e) => { - eprintln!("worktree: push skipped for item {}: {e}", item.id); - return None; - } - _ => {} - } + let branch = flare_git_core::worktree::push_branch(item, repo_root, target_branch, as_progress(progress))?; if let Some(p) = progress { p.send(0.5, Some(1.0), Some("Creating PR...".into())); } @@ -423,307 +90,3 @@ pub fn push_and_open_pr( } } } - -#[cfg(test)] -mod tests { - use super::*; - use tempfile::TempDir; - - struct Repo { - _dir: TempDir, - path: PathBuf, - } - - fn init_repo() -> Repo { - init_repo_with_branch("master") - } - - fn init_repo_with_branch(branch: &str) -> Repo { - let dir = TempDir::new().unwrap(); - let path = dir.path().to_path_buf(); - run_git_in(&path, &["init", "-b", branch]).unwrap(); - run_git_in(&path, &["config", "user.email", "test@test.com"]).unwrap(); - run_git_in(&path, &["config", "user.name", "Test"]).unwrap(); - run_git_in(&path, &["commit", "--allow-empty", "-m", "initial"]).unwrap(); - Repo { _dir: dir, path } - } - - fn test_item(sequence_id: i64) -> agentflare_backend::item::Item { - agentflare_backend::item::Item { - id: "test-id".into(), - project_id: "proj".into(), - state_id: "state".into(), - name: "test".into(), - description: String::new(), - priority: "none".into(), - parent_id: None, - assignee_agent: None, - sequence_id, - sort_order: 0.0, - started_at: None, - completed_at: None, - archived_at: None, - external_source: None, - external_id: None, - metadata: "{}".into(), - created_at: 0, - updated_at: 0, - deleted_at: None, - } - } - - #[test] - fn resolve_default_branch_resolves_from_origin_head() { - let repo = init_repo(); - assert_eq!(resolve_default_branch(&repo.path), "master"); - } - - #[test] - fn resolve_default_branch_falls_back_to_actual_head_for_nonstandard_names() { - // No origin, no "main", no "master" — must not guess "master" when - // the repo's real default branch is named something else entirely. - let repo = init_repo_with_branch("trunk"); - assert_eq!(resolve_default_branch(&repo.path), "trunk"); - } - - #[test] - fn ensure_worktrees_ignored_is_noop_when_already_ignored() { - let repo = init_repo(); - let exclude_path = repo.path.join(".git").join("info").join("exclude"); - std::fs::create_dir_all(exclude_path.parent().unwrap()).unwrap(); - std::fs::write(&exclude_path, ".worktrees/\n").unwrap(); - let before = std::fs::read_to_string(&exclude_path).unwrap(); - ensure_worktrees_ignored(&repo.path); - let after = std::fs::read_to_string(&exclude_path).unwrap(); - assert_eq!(before, after); - } - - #[test] - fn ensure_worktrees_ignored_adds_to_local_exclude_without_committing() { - let repo = init_repo(); - ensure_worktrees_ignored(&repo.path); - let exclude_path = repo.path.join(".git").join("info").join("exclude"); - let content = std::fs::read_to_string(&exclude_path).unwrap(); - assert!(content.contains(".worktrees/")); - // Must never touch the tracked .gitignore or create a commit. - assert!(!repo.path.join(".gitignore").exists()); - let log = run_git_in(&repo.path, &["log", "--oneline"]).unwrap(); - assert_eq!( - log.lines().count(), - 1, - "no new commit should have been made" - ); - } - - #[test] - fn already_isolated_for_false_in_regular_repo() { - let repo = init_repo(); - assert!(!already_isolated_for("task/1", &repo.path)); - } - - #[test] - fn isolate_worktree_target_dir_writes_relative_target_dir() { - let tmp = TempDir::new().unwrap(); - let wt = tmp.path().join(".worktrees").join("task").join("1"); - std::fs::create_dir_all(&wt).unwrap(); - isolate_worktree_target_dir(&wt); - let config = wt.join(".cargo").join("config.toml"); - assert!(config.exists(), "expected .cargo/config.toml in worktree"); - let content = std::fs::read_to_string(&config).unwrap(); - assert!( - content.contains("target-dir = \"target\""), - "must set a relative, per-checkout target dir, got: {content}" - ); - assert!( - !content.contains("target-dir = \"/") - && !content.contains("target-dir = \"~") - && !content.contains("CARGO_TARGET_DIR ="), - "must not set an absolute/shared target dir" - ); - } - - #[test] - fn isolate_worktree_target_dir_does_not_clobber_existing_config() { - let tmp = TempDir::new().unwrap(); - let wt = tmp.path().join(".worktrees").join("task").join("1"); - let cargo_dir = wt.join(".cargo"); - std::fs::create_dir_all(&cargo_dir).unwrap(); - let config = cargo_dir.join("config.toml"); - std::fs::write( - &config, - "[build]\ntarget-dir = \"/some/intentional/path\"\n", - ) - .unwrap(); - isolate_worktree_target_dir(&wt); - let content = std::fs::read_to_string(&config).unwrap(); - assert!( - content.contains("/some/intentional/path"), - "existing worktree-local config must be preserved" - ); - } - - #[test] - fn warn_if_ambient_target_dir_warns_when_set() { - // Just asserts the function runs without panicking whether or not the - // var is set; the warning is an ephemeral eprintln, not assertable here. - unsafe { - std::env::set_var("CARGO_TARGET_DIR", "/tmp/shared"); - } - warn_if_ambient_target_dir(); - unsafe { - std::env::remove_var("CARGO_TARGET_DIR"); - } - warn_if_ambient_target_dir(); - } - - #[test] - fn already_isolated_for_true_inside_the_worktree_it_created() { - let repo = init_repo(); - let item = test_item(1); - let target = resolve_default_branch(&repo.path); - let worktree_path = create_worktree(&item, &repo.path, &target, None).unwrap(); - assert!(already_isolated_for("task/1", &worktree_path)); - } - - #[test] - fn create_worktree_creates_worktree_and_branch() { - let repo = init_repo(); - let worktree_path = repo.path.join(".worktrees").join("task").join("1"); - let item = test_item(1); - let target = resolve_default_branch(&repo.path); - let result = create_worktree(&item, &repo.path, &target, None); - assert!(result.is_some()); - assert!(worktree_path.exists()); - } - - #[test] - fn create_worktree_soft_fails_on_bad_git() { - let tmp = TempDir::new().unwrap(); - let bad_root = tmp.path().join("not-a-repo"); - std::fs::create_dir_all(&bad_root).unwrap(); - let item = test_item(1); - let result = create_worktree(&item, &bad_root, "master", None); - assert!(result.is_none()); - } - - #[test] - fn create_worktree_fetches_target_branch_and_includes_remote_only_commits() { - // "remote" — plays the role of `origin`. - let remote = init_repo(); - // "local" — a clone that will go stale the moment `remote` gets a - // new commit; this is what `create_worktree` actually operates on. - let local_container = TempDir::new().unwrap(); - let local_path = local_container.path().join("local"); - run_git_in( - local_container.path(), - &[ - "clone", - remote.path.to_str().unwrap(), - local_path.to_str().unwrap(), - ], - ) - .unwrap(); - run_git_in(&local_path, &["config", "user.email", "test@test.com"]).unwrap(); - run_git_in(&local_path, &["config", "user.name", "Test"]).unwrap(); - - // Lands on the remote *after* the clone — local's own `master` and - // `origin/master` are both stale relative to this. - run_git_in( - &remote.path, - &["commit", "--allow-empty", "-m", "remote-only commit"], - ) - .unwrap(); - let remote_head = run_git_in(&remote.path, &["rev-parse", "HEAD"]).unwrap(); - - let item = test_item(1); - let worktree_path = create_worktree(&item, &local_path, "master", None).unwrap(); - let worktree_head = run_git_in(&worktree_path, &["rev-parse", "HEAD"]).unwrap(); - - assert_eq!( - worktree_head, remote_head, - "worktree must be based on the freshly-fetched remote commit, not the stale local ref" - ); - } - - #[test] - fn push_and_open_pr_returns_none_when_no_worktree_exists() { - let repo = init_repo(); - let item = test_item(1); - assert!(push_and_open_pr(&item, &repo.path, "master", None).is_none()); - } - - #[test] - fn push_and_open_pr_returns_none_when_branch_has_no_new_commits() { - let repo = init_repo(); - let item = test_item(1); - let target = resolve_default_branch(&repo.path); - create_worktree(&item, &repo.path, &target, None).unwrap(); - // No commits were made in the worktree — nothing to push, so this - // must return early without attempting a real `git push`/`gh pr - // create` (which would fail anyway: no remote configured here). - assert!(push_and_open_pr(&item, &repo.path, &target, None).is_none()); - } - - #[test] - fn push_and_open_pr_returns_none_when_branch_content_already_merged() { - let repo = init_repo(); - let item = test_item(1); - let target = resolve_default_branch(&repo.path); - let worktree_path = create_worktree(&item, &repo.path, &target, None).unwrap(); - let test_file = worktree_path.join("test.txt"); - std::fs::write(&test_file, b"hello").unwrap(); - run_git_in(&worktree_path, &["add", "test.txt"]).unwrap(); - run_git_in(&worktree_path, &["commit", "-m", "worktree change"]).unwrap(); - // task/1 now has a commit master doesn't. Squash-merge: cherry-pick - // the *diff* onto master so content matches but ancestry doesn't. - run_git_in(&repo.path, &["cherry-pick", "-n", "task/1"]).unwrap(); - run_git_in(&repo.path, &["commit", "-m", "squash-merge"]).unwrap(); - // Content-diff guard should catch this even though commit-count - // guard passes. - assert!(push_and_open_pr(&item, &repo.path, &target, None).is_none()); - } - - #[test] - fn run_output_timeout_kills_the_child_not_just_abandons_it() { - let tmp = tempfile::tempdir().unwrap(); - let marker = tmp.path().join("marker"); - // A command that, left alone, outlives our 1s timeout and then - // writes `marker`. If the timeout only abandoned the child (the bug - // this replaced) rather than killing it, the marker would still - // show up once the sleep finishes on its own. - #[cfg(unix)] - let (program, owned_args): (&str, Vec) = ( - "sh", - vec![ - "-c".into(), - format!("sleep 3 && touch {}", marker.display()), - ], - ); - #[cfg(windows)] - let (program, owned_args): (&str, Vec) = ( - "cmd", - vec![ - "/C".into(), - format!( - "ping 127.0.0.1 -n 4 >NUL & echo done > {}", - marker.display() - ), - ], - ); - let args: Vec<&str> = owned_args.iter().map(String::as_str).collect(); - - let result = run_output_timeout(program, &args, tmp.path(), 1); - assert!( - matches!(&result, Err(e) if e.contains("timed out")), - "{result:?}" - ); - - // Give the command's natural (un-killed) duration time to elapse - // before checking the marker never showed up. - std::thread::sleep(Duration::from_secs(4)); - assert!( - !marker.exists(), - "child kept running after the timeout — it was abandoned, not killed" - ); - } -} From cad56c41ce65137cb70772fec6001528b5e4a6e2 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 13:49:50 +0530 Subject: [PATCH 07/17] feat(flare-git-core): classify/snapshot/provenance/audit modules New agentic-git-inspired capabilities for the upcoming git PATH shim, all living in flare-git-core alongside the primitives they build on: classify.rs: command classification. classify_pure is a pure function (subcommand, args, default_branch, push_touches_trust_root) -> Disposition -- fail-closed by default (an unrecognized subcommand denies, not passes through), with an explicit allowlist for read-only + ordinary mutating git usage. checkout/switch to a protected branch and push carrying changes to a trust-root path (.githooks/, .agentflare/, Cargo.toml) are denied; low-level plumbing (update-index, apply, read-tree, ...) is denied outright; git worktree is denied (orchestrator-managed by agentflare). is_destructive() flags reset --hard/clean -f*/force checkout-switch as needing a snapshot first -- kept orthogonal to Disposition, since a destructive op is still an allowed one. snapshot.rs: pre-destructive snapshot/restore using git's own object store -- git add -A into a temporary index, write-tree/commit-tree, and a private ref (refs/agentflare/snapshots/) that keeps the commit gc-safe without ever touching the real index or working tree. restore() only touches paths present at snapshot time, so files created afterward survive. list()/prune() read the ref namespace directly -- no separate metadata store. provenance.rs: build_trailers() resolves agent identity (AGENTFLARE_AGENT, falling back to the agent-detector crate -- the same fallback chain as claims::owner_id), current branch, and item id (from the task/ branch convention). append_trailers() is idempotent against re-stamping (commit --amend re-invokes prepare-commit-msg). Deliberately self-reported, not cryptographically attested -- no HMAC/binding system ported from the inspiration project, matching agentflare's existing identity trust level. audit.rs: append-only JSONL log of classify::Event. Reading a missing log is empty (not an error); reading a malformed line fails closed (returns an error rather than silently dropping it). 47 tests total in the crate, all green; clippy clean. --- Cargo.lock | 2 + crates/flare-git-core/Cargo.toml | 2 + crates/flare-git-core/src/audit.rs | 110 +++++++++ crates/flare-git-core/src/classify.rs | 309 ++++++++++++++++++++++++ crates/flare-git-core/src/lib.rs | 4 + crates/flare-git-core/src/provenance.rs | 131 ++++++++++ crates/flare-git-core/src/snapshot.rs | 190 +++++++++++++++ 7 files changed, 748 insertions(+) create mode 100644 crates/flare-git-core/src/audit.rs create mode 100644 crates/flare-git-core/src/classify.rs create mode 100644 crates/flare-git-core/src/provenance.rs create mode 100644 crates/flare-git-core/src/snapshot.rs diff --git a/Cargo.lock b/Cargo.lock index d2e7b42e..2cc9cbc2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1021,8 +1021,10 @@ checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582" name = "flare-git-core" version = "0.1.0" dependencies = [ + "agent-detector", "agentflare-backend", "chrono", + "dirs", "rusqlite", "serde", "serde_json", diff --git a/crates/flare-git-core/Cargo.toml b/crates/flare-git-core/Cargo.toml index 881d9486..faa8dd31 100644 --- a/crates/flare-git-core/Cargo.toml +++ b/crates/flare-git-core/Cargo.toml @@ -13,6 +13,8 @@ serde_json = "1" chrono = "0.4" rusqlite = { version = "0.40", features = ["bundled"] } agentflare-backend = { package = "agentflare-backend", path = "../agentflare-backend" } +dirs = "6" +agent-detector = "0.2.1" [dev-dependencies] tempfile = "3" diff --git a/crates/flare-git-core/src/audit.rs b/crates/flare-git-core/src/audit.rs new file mode 100644 index 00000000..7c537a59 --- /dev/null +++ b/crates/flare-git-core/src/audit.rs @@ -0,0 +1,110 @@ +//! Append-only audit log for the git-shim's classified events. Every +//! `classify::Event` handed to `log_event` is appended as one JSONL line — +//! callers that want to suppress `SilentExempt` noise decide that +//! themselves before calling; this module always logs whatever it's given. + +use std::io::Write as _; +use std::path::{Path, PathBuf}; + +use crate::classify::Event; + +/// Default audit log location: `~/.agentflare/audit/git.jsonl`. +#[must_use] +pub fn default_path() -> Option { + dirs::home_dir().map(|h| h.join(".agentflare").join("audit").join("git.jsonl")) +} + +/// Appends one JSONL line for `event`, creating the parent directory and +/// file if needed. +pub fn log_event(audit_path: &Path, event: &Event) -> std::io::Result<()> { + if let Some(parent) = audit_path.parent() { + std::fs::create_dir_all(parent)?; + } + let mut f = std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(audit_path)?; + let line = serde_json::to_string(event).map_err(std::io::Error::other)?; + writeln!(f, "{line}") +} + +/// Reads back every event in the log, oldest first. A missing file reads +/// as empty (nothing has been logged yet — not an error). A malformed line +/// fails closed: returns an error rather than silently skipping it, since +/// a corrupt audit entry is a bug worth surfacing, not data to quietly drop. +pub fn read_events(audit_path: &Path) -> std::io::Result> { + let content = match std::fs::read_to_string(audit_path) { + Ok(c) => c, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(Vec::new()), + Err(e) => return Err(e), + }; + content + .lines() + .filter(|l| !l.trim().is_empty()) + .map(|l| serde_json::from_str(l).map_err(std::io::Error::other)) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::classify::Disposition; + use tempfile::TempDir; + + fn sample_event(subcommand: &str) -> Event { + Event { + subcommand: subcommand.to_string(), + args: vec!["origin".to_string()], + disposition: Disposition::Passthrough, + } + } + + #[test] + fn reading_a_missing_log_is_empty_not_an_error() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("does-not-exist").join("git.jsonl"); + assert_eq!(read_events(&path).unwrap(), Vec::new()); + } + + #[test] + fn append_then_read_back_round_trips_in_order() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("audit").join("git.jsonl"); + log_event(&path, &sample_event("fetch")).unwrap(); + log_event(&path, &sample_event("push")).unwrap(); + let events = read_events(&path).unwrap(); + assert_eq!(events, vec![sample_event("fetch"), sample_event("push")]); + } + + #[test] + fn deny_events_round_trip_with_their_reason() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("git.jsonl"); + let event = Event { + subcommand: "checkout".to_string(), + args: vec!["master".to_string()], + disposition: Disposition::Deny { + reason: "protected branch".to_string(), + }, + }; + log_event(&path, &event).unwrap(); + assert_eq!(read_events(&path).unwrap(), vec![event]); + } + + #[test] + fn a_malformed_line_fails_closed_instead_of_being_silently_dropped() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("git.jsonl"); + log_event(&path, &sample_event("fetch")).unwrap(); + std::fs::OpenOptions::new() + .append(true) + .open(&path) + .unwrap() + .write_all(b"not valid json\n") + .unwrap(); + assert!( + read_events(&path).is_err(), + "a corrupt line must surface as an error, not be silently skipped" + ); + } +} diff --git a/crates/flare-git-core/src/classify.rs b/crates/flare-git-core/src/classify.rs new file mode 100644 index 00000000..3a1ee9a8 --- /dev/null +++ b/crates/flare-git-core/src/classify.rs @@ -0,0 +1,309 @@ +//! Command classification: the policy core of the git-shim. Every `git +//! ` invocation gets classified into exactly one +//! disposition before the shim decides whether to exec real git. +//! +//! Fail-closed by default: a subcommand this policy doesn't explicitly +//! recognize is `Deny`, not `Passthrough`. A shim that silently passes +//! through anything it doesn't recognize defeats its own purpose the +//! moment git grows a new subcommand this policy hasn't been taught about. +//! `RedirectToWorktree` exists in the `Disposition` enum for API +//! completeness (mirroring the inspiration project's 4-way model) but v1's +//! policy never produces it — agentflare has no per-agent worktree binding +//! data available at classify time yet. + +use serde::{Deserialize, Serialize}; +use std::path::{Path, PathBuf}; + +use crate::branch::{is_protected_branch, resolve_default_branch}; + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum Disposition { + Passthrough, + RedirectToWorktree { path: PathBuf }, + SilentExempt, + Deny { reason: String }, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct Event { + pub subcommand: String, + pub args: Vec, + pub disposition: Disposition, +} + +/// Trust-root paths a `push` must never carry changes to — agentflare's own +/// enforcement config, not something an agent should be able to push a +/// change to and quietly weaken. +const TRUST_ROOT_PATHS: &[&str] = &[".githooks/", ".agentflare/", "Cargo.toml"]; + +/// Ordinary, non-destructive read-only subcommands — always `Passthrough` +/// regardless of args. +const READ_ONLY_SUBCOMMANDS: &[&str] = &[ + "status", + "log", + "diff", + "show", + "blame", + "shortlog", + "describe", + "ls-files", + "ls-tree", + "cat-file", + "grep", + "reflog", + "rev-parse", + "rev-list", + "symbolic-ref", + "config", + "remote", + "tag", + "branch", + "fetch", + "clone", + "help", + "version", +]; + +/// Ordinary mutating workflow commands, allowed by default — none of these +/// are individually dangerous the way `reset --hard`/`clean -f`/protected- +/// branch checkout/trust-root push are. +const ALLOWED_MUTATING_SUBCOMMANDS: &[&str] = &[ + "add", + "commit", + "merge", + "rebase", + "pull", + "cherry-pick", + "revert", + "stash", + "init", + "restore", + "reset", + "clean", +]; + +/// Low-level plumbing that can bypass the higher-level checks above — +/// denied outright rather than reasoned about case by case. +const DENIED_PLUMBING_SUBCOMMANDS: &[&str] = &[ + "read-tree", + "update-index", + "apply", + "hash-object", + "mktree", + "commit-tree", + "update-ref", +]; + +/// `true` for the destructive ops that must be snapshotted before they run +/// (see `snapshot::snapshot_before`) — orthogonal to `Disposition`: a +/// destructive command is still `Passthrough`-classified (it's allowed), +/// but the shim binary must snapshot first. +#[must_use] +pub fn is_destructive(subcommand: &str, args: &[String]) -> bool { + match subcommand { + "reset" => args.iter().any(|a| a == "--hard"), + "clean" => args.iter().any(|a| a == "-f" || a == "-fd" || a == "-fx" || a == "-fdx"), + "checkout" | "switch" => args.iter().any(|a| a == "-f" || a == "--force" || a == "-B"), + _ => false, + } +} + +/// Pure classification core — no I/O, so it's unit-testable with fixed +/// inputs. `default_branch` is the repo's resolved default branch. +/// `push_touches_trust_root` is pre-resolved by the caller (requires a real +/// `git diff`, hence not something a pure function can determine itself) +/// and is only consulted when `subcommand == "push"`. +#[must_use] +pub fn classify_pure( + subcommand: &str, + args: &[String], + default_branch: &str, + push_touches_trust_root: bool, +) -> Disposition { + if READ_ONLY_SUBCOMMANDS.contains(&subcommand) || ALLOWED_MUTATING_SUBCOMMANDS.contains(&subcommand) { + return Disposition::Passthrough; + } + if DENIED_PLUMBING_SUBCOMMANDS.contains(&subcommand) { + return Disposition::Deny { + reason: format!( + "'git {subcommand}' is a low-level plumbing command blocked by the agentflare git shim — it can bypass the checks this shim applies to higher-level commands." + ), + }; + } + match subcommand { + "checkout" | "switch" => { + let Some(target) = args.iter().find(|a| !a.starts_with('-')) else { + return Disposition::Passthrough; // no target arg (e.g. `git switch -`) — nothing to protect against + }; + if is_protected_branch(target, Some(default_branch)) { + Disposition::Deny { + reason: format!( + "'{target}' is this repo's default branch — direct checkout/switch is blocked by the agentflare git shim. Create an isolated worktree first." + ), + } + } else { + Disposition::Passthrough + } + } + "push" => { + if push_touches_trust_root { + Disposition::Deny { + reason: "this push carries changes to a trust-root path (.githooks/, .agentflare/, or Cargo.toml) — blocked by the agentflare git shim.".to_string(), + } + } else { + Disposition::Passthrough + } + } + "worktree" => Disposition::Deny { + reason: "'git worktree' is orchestrator-managed by agentflare — use the `item` MCP tool's claim flow instead of calling it directly.".to_string(), + }, + _ => Disposition::Deny { + reason: format!( + "'git {subcommand}' is not a recognized command for the agentflare git shim (fail-closed default) — if this is legitimate day-to-day usage, it needs to be added to flare-git-core::classify's policy." + ), + }, + } +} + +/// Whether pushing would carry changes to a trust-root path — inspects the +/// diff between `branch` and `target`. Errs toward `true` (blocking) if +/// that diff can't be determined at all: an unreadable diff is not a safe +/// default to let through. +#[must_use] +pub fn push_touches_trust_root(repo_root: &Path, branch: &str, target: &str) -> bool { + let range = format!("{target}...{branch}"); + match crate::shell::run_in(repo_root, &["diff", "--name-only", &range]) { + Ok(names) => names + .lines() + .any(|f| TRUST_ROOT_PATHS.iter().any(|p| f.starts_with(p))), + Err(_) => true, + } +} + +/// I/O-resolving entry point: resolves the default branch and (for `push` +/// with a resolvable branch/target pair) whether the push touches a +/// trust-root path, then delegates to `classify_pure`. +#[must_use] +pub fn classify(repo_root: &Path, subcommand: &str, args: &[String]) -> Event { + let default_branch = resolve_default_branch(repo_root); + let touches_trust_root = subcommand == "push" + && args.len() >= 2 + && push_touches_trust_root(repo_root, &args[1], &default_branch); + let disposition = classify_pure(subcommand, args, &default_branch, touches_trust_root); + Event { + subcommand: subcommand.to_string(), + args: args.to_vec(), + disposition, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn args(v: &[&str]) -> Vec { + v.iter().map(|s| s.to_string()).collect() + } + + #[test] + fn read_only_subcommands_pass_through() { + assert_eq!( + classify_pure("status", &[], "master", false), + Disposition::Passthrough + ); + assert_eq!( + classify_pure("log", &args(&["-5"]), "master", false), + Disposition::Passthrough + ); + } + + #[test] + fn ordinary_mutating_subcommands_pass_through() { + assert_eq!( + classify_pure("commit", &args(&["-m", "x"]), "master", false), + Disposition::Passthrough + ); + assert_eq!( + classify_pure("reset", &args(&["HEAD~1"]), "master", false), + Disposition::Passthrough + ); + } + + #[test] + fn unknown_subcommand_denies_by_default() { + assert!(matches!( + classify_pure("some-future-subcommand", &[], "master", false), + Disposition::Deny { .. } + )); + } + + #[test] + fn plumbing_commands_are_denied() { + assert!(matches!( + classify_pure("update-index", &[], "master", false), + Disposition::Deny { .. } + )); + assert!(matches!( + classify_pure("apply", &[], "master", false), + Disposition::Deny { .. } + )); + } + + #[test] + fn worktree_is_denied() { + assert!(matches!( + classify_pure("worktree", &args(&["add", "../x"]), "master", false), + Disposition::Deny { .. } + )); + } + + #[test] + fn checkout_to_protected_branch_is_denied() { + let d = classify_pure("checkout", &args(&["master"]), "master", false); + assert!(matches!(d, Disposition::Deny { .. })); + } + + #[test] + fn switch_to_feature_branch_passes_through() { + assert_eq!( + classify_pure("switch", &args(&["feature/x"]), "master", false), + Disposition::Passthrough + ); + } + + #[test] + fn checkout_with_no_target_arg_passes_through() { + // `git switch -` (previous branch) — nothing to protect against. + assert_eq!( + classify_pure("switch", &args(&["-"]), "master", false), + Disposition::Passthrough + ); + } + + #[test] + fn push_touching_trust_root_is_denied() { + assert!(matches!( + classify_pure("push", &args(&["origin", "feature/x"]), "master", true), + Disposition::Deny { .. } + )); + } + + #[test] + fn push_not_touching_trust_root_passes_through() { + assert_eq!( + classify_pure("push", &args(&["origin", "feature/x"]), "master", false), + Disposition::Passthrough + ); + } + + #[test] + fn is_destructive_flags_reset_hard_and_force_ops() { + assert!(is_destructive("reset", &args(&["--hard"]))); + assert!(!is_destructive("reset", &args(&["--soft"]))); + assert!(is_destructive("clean", &args(&["-fd"]))); + assert!(!is_destructive("clean", &args(&["-n"]))); + assert!(is_destructive("checkout", &args(&["-f", "master"]))); + assert!(!is_destructive("checkout", &args(&["master"]))); + assert!(!is_destructive("commit", &args(&["-m", "x"]))); + } +} diff --git a/crates/flare-git-core/src/lib.rs b/crates/flare-git-core/src/lib.rs index f876440f..987184d5 100644 --- a/crates/flare-git-core/src/lib.rs +++ b/crates/flare-git-core/src/lib.rs @@ -4,6 +4,10 @@ //! Remote GitHub REST API operations (PRs/issues/CI/releases) are a //! separate, unrelated concern and live in `src/github/*` -- not here. +pub mod audit; pub mod branch; +pub mod classify; +pub mod provenance; pub mod shell; +pub mod snapshot; pub mod worktree; diff --git a/crates/flare-git-core/src/provenance.rs b/crates/flare-git-core/src/provenance.rs new file mode 100644 index 00000000..0e146031 --- /dev/null +++ b/crates/flare-git-core/src/provenance.rs @@ -0,0 +1,131 @@ +//! Commit provenance trailers — self-reported agent/branch/item identity +//! appended to commit messages via a `prepare-commit-msg` hook. +//! +//! Deliberately NOT cryptographically attested: agentflare has no +//! signing/binding system for this, so a trailer is a bare string an agent +//! could misreport — the same trust level as every other +//! `AGENTFLARE_AGENT`-based identity check already in this codebase (see +//! `claims::owner_id`'s identical fallback chain). + +use std::path::Path; + +use crate::branch::current_branch; + +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub struct Trailers { + pub agent: Option, + pub branch: Option, + pub item_id: Option, +} + +/// Resolves the current commit's provenance: agent identity +/// (`AGENTFLARE_AGENT`, falling back to auto-detection), the current +/// branch, and — if the branch matches the `task/` convention +/// `flare_git_core::worktree` uses — the item id it belongs to. +#[must_use] +pub fn build_trailers(repo_root: &Path) -> Trailers { + let agent = std::env::var("AGENTFLARE_AGENT") + .ok() + .filter(|s| !s.is_empty()) + .or_else(agent_detector::agent_name); + let branch = current_branch(repo_root); + let item_id = branch + .as_deref() + .and_then(|b| b.strip_prefix("task/")) + .map(str::to_string); + Trailers { + agent, + branch, + item_id, + } +} + +/// Appends non-empty `Trailers` fields to `msg` as git trailers, skipping +/// any field that didn't resolve rather than writing an empty trailer. +/// A no-op if `msg` already carries agentflare trailers (e.g. `commit +/// --amend` re-invokes `prepare-commit-msg` on an already-stamped +/// message) — never duplicates. +#[must_use] +pub fn append_trailers(msg: &str, t: &Trailers) -> String { + if msg.contains("Agentflare-Agent:") + || msg.contains("Agentflare-Branch:") + || msg.contains("Agentflare-Item:") + { + return msg.to_string(); + } + let mut lines = Vec::new(); + if let Some(agent) = &t.agent { + lines.push(format!("Agentflare-Agent: {agent}")); + } + if let Some(branch) = &t.branch { + lines.push(format!("Agentflare-Branch: {branch}")); + } + if let Some(item_id) = &t.item_id { + lines.push(format!("Agentflare-Item: {item_id}")); + } + if lines.is_empty() { + return msg.to_string(); + } + let mut out = msg.trim_end().to_string(); + out.push_str("\n\n"); + out.push_str(&lines.join("\n")); + out.push('\n'); + out +} + +#[cfg(test)] +mod tests { + use super::*; + + fn trailers() -> Trailers { + Trailers { + agent: Some("claude-code".to_string()), + branch: Some("task/42".to_string()), + item_id: Some("42".to_string()), + } + } + + #[test] + fn appends_all_resolved_fields_after_a_blank_line() { + let out = append_trailers("fix: thing\n", &trailers()); + assert_eq!( + out, + "fix: thing\n\nAgentflare-Agent: claude-code\nAgentflare-Branch: task/42\nAgentflare-Item: 42\n" + ); + } + + #[test] + fn skips_unresolved_fields_entirely() { + let t = Trailers { + agent: Some("claude-code".to_string()), + branch: None, + item_id: None, + }; + let out = append_trailers("fix: thing\n", &t); + assert_eq!(out, "fix: thing\n\nAgentflare-Agent: claude-code\n"); + } + + #[test] + fn returns_message_unchanged_when_nothing_resolved() { + let out = append_trailers("fix: thing\n", &Trailers::default()); + assert_eq!(out, "fix: thing\n"); + } + + #[test] + fn does_not_duplicate_trailers_on_an_already_stamped_message() { + let once = append_trailers("fix: thing\n", &trailers()); + let twice = append_trailers(&once, &trailers()); + assert_eq!(once, twice); + } + + #[test] + fn item_id_extracted_from_task_branch_convention() { + // Mirrors flare_git_core::worktree's `format!("task/{}", item.sequence_id)`. + let t = Trailers { + agent: None, + branch: Some("task/17".to_string()), + item_id: Some("17".to_string()), + }; + assert_eq!(t.item_id.as_deref(), Some("17")); + } +} diff --git a/crates/flare-git-core/src/snapshot.rs b/crates/flare-git-core/src/snapshot.rs new file mode 100644 index 00000000..912e4638 --- /dev/null +++ b/crates/flare-git-core/src/snapshot.rs @@ -0,0 +1,190 @@ +//! Pre-destructive snapshotting: before a destructive git command runs +//! (`reset --hard`, `clean -f*`, force checkout/switch — see +//! `classify::is_destructive`), the shim snapshots the current working +//! tree so it can be recovered. Snapshots are plain git commit objects +//! under a private ref namespace (`refs/agentflare/snapshots/`) — no +//! separate blob store or metadata DB; git's own object store already +//! does exactly this job, and a ref under a private namespace keeps the +//! commit reachable (gc-safe) without touching the working tree or the +//! real index while creating it. + +use std::path::Path; +use std::process::Command; + +use crate::shell::run_in; + +const SNAPSHOT_REF_PREFIX: &str = "refs/agentflare/snapshots/"; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SnapshotId(pub String); + +#[derive(Debug, Clone)] +pub struct SnapshotMeta { + pub id: SnapshotId, + pub committer_date: String, + pub reason: String, +} + +/// `git ` with a temporary `GIT_INDEX_FILE`, so staging for a +/// snapshot never touches the caller's real index. +fn run_git_with_index(repo_root: &Path, index_file: &Path, args: &[&str]) -> Result { + let out = Command::new("git") + .args(args) + .current_dir(repo_root) + .env("GIT_INDEX_FILE", index_file) + .output() + .map_err(|e| format!("git not available: {e}"))?; + if !out.status.success() { + return Err(String::from_utf8_lossy(&out.stderr).trim().to_string()); + } + Ok(String::from_utf8_lossy(&out.stdout).trim().to_string()) +} + +/// Snapshots the current working tree (tracked + untracked, respecting +/// `.gitignore`) into a commit object under a private ref. Staging happens +/// in a temporary index file, removed afterward regardless of outcome — +/// the real index and working tree are never touched. +pub fn snapshot_before(repo_root: &Path, reason: &str) -> Result { + let tmp_index = repo_root + .join(".git") + .join(format!("agentflare-snapshot-index-{}", std::process::id())); + let result = (|| { + run_git_with_index(repo_root, &tmp_index, &["add", "-A"])?; + let tree = run_git_with_index(repo_root, &tmp_index, &["write-tree"])?; + let parent = run_in(repo_root, &["rev-parse", "HEAD"]).ok(); + let mut commit_args = vec!["commit-tree".to_string(), tree, "-m".to_string(), reason.to_string()]; + if let Some(p) = &parent { + commit_args.push("-p".to_string()); + commit_args.push(p.clone()); + } + let commit_args_ref: Vec<&str> = commit_args.iter().map(String::as_str).collect(); + let sha = run_in(repo_root, &commit_args_ref)?; + let refname = format!("{SNAPSHOT_REF_PREFIX}{sha}"); + run_in(repo_root, &["update-ref", &refname, &sha])?; + Ok(SnapshotId(sha)) + })(); + let _ = std::fs::remove_file(&tmp_index); + result +} + +/// Restores paths from a snapshot into the current working tree and index. +/// Only restores paths that existed at snapshot time — files created after +/// the snapshot are untouched and survive. +pub fn restore(repo_root: &Path, id: &SnapshotId) -> Result<(), String> { + let refname = format!("{SNAPSHOT_REF_PREFIX}{}", id.0); + run_in(repo_root, &["checkout", &refname, "--", "."])?; + Ok(()) +} + +/// Lists snapshots, newest first. +#[must_use] +pub fn list(repo_root: &Path) -> Vec { + let Ok(out) = run_in( + repo_root, + &[ + "for-each-ref", + "--sort=-committerdate", + "--format=%(refname) %(committerdate:iso-strict) %(subject)", + SNAPSHOT_REF_PREFIX, + ], + ) else { + return Vec::new(); + }; + out.lines() + .filter_map(|line| { + let mut parts = line.splitn(3, ' '); + let refname = parts.next()?; + let date = parts.next()?; + let reason = parts.next().unwrap_or("").to_string(); + let id = refname.strip_prefix(SNAPSHOT_REF_PREFIX)?.to_string(); + Some(SnapshotMeta { + id: SnapshotId(id), + committer_date: date.to_string(), + reason, + }) + }) + .collect() +} + +/// Deletes all but the `keep_last` most recent snapshots. +pub fn prune(repo_root: &Path, keep_last: usize) -> Result<(), String> { + for meta in list(repo_root).into_iter().skip(keep_last) { + let refname = format!("{SNAPSHOT_REF_PREFIX}{}", meta.id.0); + run_in(repo_root, &["update-ref", "-d", &refname])?; + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::shell::test_support::init_repo_with_branch; + use tempfile::TempDir; + + #[test] + fn snapshot_then_restore_recovers_state_and_preserves_newer_files() { + let repo = init_repo_with_branch("master"); + std::fs::write(repo.path.join("tracked.txt"), "before\n").unwrap(); + run_in(&repo.path, &["add", "tracked.txt"]).unwrap(); + run_in(&repo.path, &["commit", "-m", "add tracked"]).unwrap(); + std::fs::write(repo.path.join("tracked.txt"), "modified\n").unwrap(); + std::fs::write(repo.path.join("untracked.txt"), "scratch\n").unwrap(); + + let id = snapshot_before(&repo.path, "pre reset --hard").unwrap(); + + // Simulate the destructive op the snapshot exists to protect against. + run_in(&repo.path, &["checkout", "--", "tracked.txt"]).unwrap(); // discards "modified" + std::fs::remove_file(repo.path.join("untracked.txt")).unwrap(); + // A file created AFTER the snapshot -- must survive restore. + std::fs::write(repo.path.join("new_after_snapshot.txt"), "keep me\n").unwrap(); + + restore(&repo.path, &id).unwrap(); + + assert_eq!( + std::fs::read_to_string(repo.path.join("tracked.txt")).unwrap(), + "modified\n" + ); + assert_eq!( + std::fs::read_to_string(repo.path.join("untracked.txt")).unwrap(), + "scratch\n" + ); + assert!( + repo.path.join("new_after_snapshot.txt").exists(), + "file created after snapshot must survive restore" + ); + } + + #[test] + fn list_and_prune_keep_only_the_most_recent() { + let repo = init_repo_with_branch("master"); + let id1 = snapshot_before(&repo.path, "first").unwrap(); + // Distinct committerdate second-resolution ordering. + std::thread::sleep(std::time::Duration::from_millis(1100)); + std::fs::write(repo.path.join("f.txt"), "x").unwrap(); + let id2 = snapshot_before(&repo.path, "second").unwrap(); + + let listed = list(&repo.path); + assert_eq!(listed.len(), 2); + assert_eq!(listed[0].id, id2, "newest first"); + assert_eq!(listed[1].id, id1); + + prune(&repo.path, 1).unwrap(); + let after = list(&repo.path); + assert_eq!(after.len(), 1); + assert_eq!(after[0].id, id2); + } + + #[test] + fn snapshot_before_any_commit_exists_still_works() { + // No parent commit to attach to -- must not error out on a + // brand-new, still-empty-history repo (no initial commit). + let dir = TempDir::new().unwrap(); + let path = dir.path().to_path_buf(); + run_in(&path, &["init", "-b", "master"]).unwrap(); + run_in(&path, &["config", "user.email", "test@test.com"]).unwrap(); + run_in(&path, &["config", "user.name", "Test"]).unwrap(); + std::fs::write(path.join("f.txt"), "x").unwrap(); + let id = snapshot_before(&path, "no parent yet"); + assert!(id.is_ok(), "{id:?}"); + } +} From 07777c93022f23a3bdfd1c6692ae1ecd7cb3064a Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 14:30:39 +0530 Subject: [PATCH 08/17] feat(flare-git-shim): PATH shim binary + fix Windows self-resolution recursion New crates/flare-git-shim, [[bin]] name = "git": resolves the subcommand (skipping global flags, denying -C/--git-dir/--work-tree outright since they'd target a different repo than the one this shim classifies against), runs it through flare_git_core::classify, snapshots before destructive ops, audits every event, and execs the real binary via agentflare_shim::run_real on Passthrough/SilentExempt. Fixes a real incident hit while building this: on Windows, an unqualified Command::new("git") resolves via SearchPathW, which checks the CALLING PROCESS's OWN DIRECTORY before PATH. Since flare-git-shim's compiled binary is itself named git.exe, every internal git spawn inside flare-git-core (shell::run_in/diff, worktree's fetch/push) was resolving back into itself and recursing without limit -- one test run spawned 10,000+ processes before being killed. Root-caused and fixed two ways: 1. flare_git_core::shell::git_binary() resolves "git" via which::which_in with a filtered PATH, excluding both the calling process's own directory AND (structurally, matching "target"+"debug|release" as adjacent path components) any cargo build-profile directory tree -- necessary because this workspace's ~/.cargo/config.toml redirects target-dir to a shared ~/.cargo/target, and Cargo itself prepends that profile dir to PATH for every test/run process, putting flare-git-shim's freshly-built git.exe on PATH for literally every crate's test suite. shell::run_in/diff and worktree's run_output_timeout callers all route through this now instead of a bare "git" string. 2. agentflare_shim::path_without_shim_dir gets the same structural exclusion, since run_real's own resolution had the identical gap. 3. flare-git-shim's main() adds a hard recursion-depth backstop (FLARE_GIT_SHIM_DEPTH env var, cap 3) independent of the above fixes -- if anything ever resolves "git" wrong again for any reason, this caps the blast radius at a handful of processes instead of a spawn storm. audit::default_path() now also honors AGENTFLARE_HOME_OVERRIDE (matching src/paths.rs::home's existing test/CI escape hatch) so tests -- including this crate's own new integration suite, which spawns the real compiled shim binary against real temp repos -- never touch a developer's actual ~/.agentflare/. Full workspace build + test suite green (44 test-result groups, 0 failures); clippy clean on all touched crates. --- Cargo.lock | 10 ++ Cargo.toml | 2 +- crates/agentflare-shim/src/lib.rs | 28 +++- crates/flare-git-core/Cargo.toml | 1 + crates/flare-git-core/src/audit.rs | 12 +- crates/flare-git-core/src/shell.rs | 57 +++++++- crates/flare-git-core/src/snapshot.rs | 2 +- crates/flare-git-core/src/worktree.rs | 15 ++- crates/flare-git-shim/Cargo.toml | 19 +++ crates/flare-git-shim/src/main.rs | 160 +++++++++++++++++++++++ crates/flare-git-shim/tests/shim_test.rs | 101 ++++++++++++++ 11 files changed, 391 insertions(+), 16 deletions(-) create mode 100644 crates/flare-git-shim/Cargo.toml create mode 100644 crates/flare-git-shim/src/main.rs create mode 100644 crates/flare-git-shim/tests/shim_test.rs diff --git a/Cargo.lock b/Cargo.lock index 2cc9cbc2..8269fb21 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1029,6 +1029,16 @@ dependencies = [ "serde", "serde_json", "tempfile", + "which", +] + +[[package]] +name = "flare-git-shim" +version = "0.1.0" +dependencies = [ + "agentflare-shim", + "flare-git-core", + "tempfile", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index af01c0b3..44e930a0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,5 +1,5 @@ [workspace] -members = ["crates/flare-code", "crates/agent-registry", "crates/skill-registry", "crates/gateway-registry", "crates/flare-output", "crates/agentflare-artifacts", "crates/agentflare-backend", "crates/agentflare-db-kit", "crates/flare-search-kit", "crates/agentflare-store", "crates/flare-proxy", "crates/agentflare-shim", "crates/flare-git-core"] +members = ["crates/flare-code", "crates/agent-registry", "crates/skill-registry", "crates/gateway-registry", "crates/flare-output", "crates/agentflare-artifacts", "crates/agentflare-backend", "crates/agentflare-db-kit", "crates/flare-search-kit", "crates/agentflare-store", "crates/flare-proxy", "crates/agentflare-shim", "crates/flare-git-core", "crates/flare-git-shim"] resolver = "2" [package] diff --git a/crates/agentflare-shim/src/lib.rs b/crates/agentflare-shim/src/lib.rs index fd6d83ed..f1cd0e2f 100644 --- a/crates/agentflare-shim/src/lib.rs +++ b/crates/agentflare-shim/src/lib.rs @@ -22,12 +22,36 @@ pub fn trace(msg: &str) { } } +/// `true` if any two adjacent path components are "target" followed by +/// "debug"/"release" -- a cargo build-profile directory (and everything +/// under it, e.g. `target/debug/deps`, `target/debug/build/*/out`). +/// Cargo prepends this to PATH for every test/run process (so build-script +/// DLLs resolve), and any `[[bin]]` target in the same cargo workspace +/// lands directly in it -- so during development/testing, a shim binary +/// built via cargo can find ANOTHER shim binary (or a differently-pathed +/// copy of itself, e.g. `target/debug/deps/git.exe` alongside +/// `target/debug/git.exe`) there instead of the real target. Not a +/// concern for an installed shim (only ever one file in +/// `~/.agentflare/shims/`), but a real hazard under `cargo test`. +fn is_cargo_target_profile_dir(p: &Path) -> bool { + let comps: Vec<_> = p.components().collect(); + comps.windows(2).any(|w| { + w[0].as_os_str() == "target" && (w[1].as_os_str() == "debug" || w[1].as_os_str() == "release") + }) +} + /// PATH with `shim_dir` removed, so a shim binary's own real-binary lookup -/// (and any child process it spawns) doesn't resolve back into itself. +/// (and any child process it spawns) doesn't resolve back into itself -- +/// also strips any cargo build-profile directory tree (see +/// `is_cargo_target_profile_dir`), which matters during development/ +/// testing when multiple cargo-built binaries share one `target/debug`. #[must_use] pub fn path_without_shim_dir(shim_dir: &Path) -> Option { let path_var = env::var_os("PATH")?; - env::join_paths(env::split_paths(&path_var).filter(|p| p != shim_dir)).ok() + env::join_paths( + env::split_paths(&path_var).filter(|p| p != shim_dir && !is_cargo_target_profile_dir(p)), + ) + .ok() } /// The tool name a shim binary is standing in for, derived from its own diff --git a/crates/flare-git-core/Cargo.toml b/crates/flare-git-core/Cargo.toml index faa8dd31..5ae93977 100644 --- a/crates/flare-git-core/Cargo.toml +++ b/crates/flare-git-core/Cargo.toml @@ -15,6 +15,7 @@ rusqlite = { version = "0.40", features = ["bundled"] } agentflare-backend = { package = "agentflare-backend", path = "../agentflare-backend" } dirs = "6" agent-detector = "0.2.1" +which = "6" [dev-dependencies] tempfile = "3" diff --git a/crates/flare-git-core/src/audit.rs b/crates/flare-git-core/src/audit.rs index 7c537a59..5ab2be1b 100644 --- a/crates/flare-git-core/src/audit.rs +++ b/crates/flare-git-core/src/audit.rs @@ -8,10 +8,18 @@ use std::path::{Path, PathBuf}; use crate::classify::Event; -/// Default audit log location: `~/.agentflare/audit/git.jsonl`. +/// Default audit log location: `~/.agentflare/audit/git.jsonl`. Honors +/// `AGENTFLARE_HOME_OVERRIDE` (the main binary's own test/CI escape hatch, +/// see `src/paths.rs::home` -- `dirs::home_dir()` resolves via the OS +/// directly on Windows and ignores HOME/USERPROFILE overrides) so tests +/// never write into a developer's real home directory. #[must_use] pub fn default_path() -> Option { - dirs::home_dir().map(|h| h.join(".agentflare").join("audit").join("git.jsonl")) + let home = match std::env::var("AGENTFLARE_HOME_OVERRIDE") { + Ok(p) => PathBuf::from(p), + Err(_) => dirs::home_dir()?, + }; + Some(home.join(".agentflare").join("audit").join("git.jsonl")) } /// Appends one JSONL line for `event`, creating the parent directory and diff --git a/crates/flare-git-core/src/shell.rs b/crates/flare-git-core/src/shell.rs index 868816d1..a2f2b5c2 100644 --- a/crates/flare-git-core/src/shell.rs +++ b/crates/flare-git-core/src/shell.rs @@ -2,14 +2,65 @@ //! against a repo goes through here instead of hand-rolling its own //! `Command::new("git")` wrapper. -use std::path::Path; +use std::path::{Path, PathBuf}; use std::process::Command; +use std::sync::OnceLock; + +/// Resolves the real `git` binary, always excluding the currently-running +/// executable's own directory from the search. +/// +/// This crate is also linked into `flare-git-shim`, a binary literally +/// named `git`/`git.exe`. On Windows, an unqualified `Command::new("git")` +/// resolves via `SearchPathW`, whose search order checks the CALLING +/// PROCESS's OWN DIRECTORY before PATH -- so inside that shim, a bare +/// "git" spawn resolves back to the shim itself and recurses without +/// limit (this happened once, during development: a single test run spun +/// up 10,000+ processes before it was caught). `which::which_in` does a +/// plain PATH-directory-listing search with no such self-referential +/// step, so resolving through it -- always excluding this process's own +/// directory -- is immune to the same failure mode regardless of which +/// binary this crate ends up linked into. +/// `true` for a cargo build-profile directory (`.../target/debug` or +/// `.../target/release`) -- Cargo prepends this to PATH for every test/run +/// process (so build-script DLLs resolve), and any `[[bin]]` target in the +/// same workspace lands directly in it. Excluding only "this process's own +/// directory" isn't enough: a workspace that redirects `target-dir` +/// globally (e.g. `~/.cargo/target`, as this repo's `~/.cargo/config.toml` +/// does for sccache) means EVERY crate's test binaries share that PATH +/// entry with `flare-git-shim`'s freshly-built `git.exe`. Detected +/// structurally (name is "debug"/"release", parent is named "target") so +/// it works regardless of where the target dir physically lives. +fn is_cargo_target_profile_dir(p: &Path) -> bool { + let comps: Vec<_> = p.components().collect(); + comps.windows(2).any(|w| { + w[0].as_os_str() == "target" && (w[1].as_os_str() == "debug" || w[1].as_os_str() == "release") + }) +} + +pub(crate) fn git_binary() -> PathBuf { + static RESOLVED: OnceLock = OnceLock::new(); + RESOLVED + .get_or_init(|| { + let self_dir = std::env::current_exe() + .ok() + .and_then(|p| p.parent().map(Path::to_path_buf)); + let filtered_path = std::env::var_os("PATH").map(|path_var| { + std::env::join_paths(std::env::split_paths(&path_var).filter(|p| { + Some(p.as_path()) != self_dir.as_deref() && !is_cargo_target_profile_dir(p) + })) + .unwrap_or(path_var) + }); + let cwd = std::env::current_dir().unwrap_or_default(); + which::which_in("git", filtered_path.as_ref(), cwd).unwrap_or_else(|_| PathBuf::from("git")) + }) + .clone() +} /// Runs `git` in `repo_root`; `Ok(stdout)` trimmed on success, `Err(stderr)` /// trimmed on a non-zero exit, or a process-spawn error message (git /// missing, etc) if it couldn't even run. pub fn run_in(repo_root: &Path, args: &[&str]) -> Result { - let out = Command::new("git") + let out = Command::new(git_binary()) .args(args) .current_dir(repo_root) .output() @@ -39,7 +90,7 @@ pub fn run_in_ok(repo_root: &Path, args: &[&str]) -> bool { /// the rest of this module's helpers return. pub fn diff(repo_root: &Path, base: &str, head: &str) -> Result { let range = format!("{base}...{head}"); - let out = Command::new("git") + let out = Command::new(git_binary()) .args(["diff", "--unified=3", &range]) .current_dir(repo_root) .output() diff --git a/crates/flare-git-core/src/snapshot.rs b/crates/flare-git-core/src/snapshot.rs index 912e4638..5be2ae30 100644 --- a/crates/flare-git-core/src/snapshot.rs +++ b/crates/flare-git-core/src/snapshot.rs @@ -28,7 +28,7 @@ pub struct SnapshotMeta { /// `git ` with a temporary `GIT_INDEX_FILE`, so staging for a /// snapshot never touches the caller's real index. fn run_git_with_index(repo_root: &Path, index_file: &Path, args: &[&str]) -> Result { - let out = Command::new("git") + let out = Command::new(crate::shell::git_binary()) .args(args) .current_dir(repo_root) .env("GIT_INDEX_FILE", index_file) diff --git a/crates/flare-git-core/src/worktree.rs b/crates/flare-git-core/src/worktree.rs index 5ca8e860..f2ed032e 100644 --- a/crates/flare-git-core/src/worktree.rs +++ b/crates/flare-git-core/src/worktree.rs @@ -217,7 +217,7 @@ pub fn create_worktree( // be able to hang a claim indefinitely. let fetch_timeout_secs = 30; let start_point = match run_output_timeout( - "git", + crate::shell::git_binary(), &["fetch", "origin", target_branch], repo_root, fetch_timeout_secs, @@ -301,12 +301,13 @@ fn kill_tree(child: &mut std::process::Child) { /// 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( - program: &str, + program: impl AsRef, args: &[&str], cwd: &Path, timeout_secs: u64, ) -> Result { - let mut cmd = Command::new(program); + let program = program.as_ref().to_owned(); + let mut cmd = Command::new(&program); cmd.args(args) .current_dir(cwd) .stdin(std::process::Stdio::null()) @@ -319,7 +320,7 @@ fn run_output_timeout( } let mut child = cmd .spawn() - .map_err(|e| format!("{program}: spawn failed: {e}"))?; + .map_err(|e| format!("{}: spawn failed: {e}", program.to_string_lossy()))?; let mut stdout_pipe = child.stdout.take().expect("stdout piped above"); let mut stderr_pipe = child.stderr.take().expect("stderr piped above"); let stdout_reader = std::thread::spawn(move || { @@ -340,11 +341,11 @@ fn run_output_timeout( if std::time::Instant::now() >= deadline { kill_tree(&mut child); let _ = child.wait(); - return Err(format!("{program} timed out after {timeout_secs}s")); + return Err(format!("{}: timed out after {timeout_secs}s", program.to_string_lossy())); } std::thread::sleep(Duration::from_millis(50)); } - Err(e) => return Err(format!("{program}: {e}")), + Err(e) => return Err(format!("{}: {e}", program.to_string_lossy())), } }; Ok(std::process::Output { @@ -402,7 +403,7 @@ pub fn push_branch( } let push_timeout = 120; match run_output_timeout( - "git", + crate::shell::git_binary(), &["push", "-u", "origin", &branch], repo_root, push_timeout, diff --git a/crates/flare-git-shim/Cargo.toml b/crates/flare-git-shim/Cargo.toml new file mode 100644 index 00000000..cf2c49d1 --- /dev/null +++ b/crates/flare-git-shim/Cargo.toml @@ -0,0 +1,19 @@ +[package] +name = "flare-git-shim" +version = "0.1.0" +edition = "2024" +rust-version = "1.91" +license = "Apache-2.0" +description = "PATH shim that impersonates `git`, classifying and auditing every invocation via flare-git-core before executing the real binary." +publish = false + +[[bin]] +name = "git" +path = "src/main.rs" + +[dependencies] +agentflare-shim = { path = "../agentflare-shim" } +flare-git-core = { path = "../flare-git-core" } + +[dev-dependencies] +tempfile = "3" diff --git a/crates/flare-git-shim/src/main.rs b/crates/flare-git-shim/src/main.rs new file mode 100644 index 00000000..ffb3e8e3 --- /dev/null +++ b/crates/flare-git-shim/src/main.rs @@ -0,0 +1,160 @@ +//! PATH shim impersonating `git`: classifies every invocation via +//! `flare_git_core::classify` before deciding whether to exec the real +//! binary, closing the gap where a raw `git commit`/`git checkout` run via +//! Bash bypasses agentflare's tool-call-level PreToolUse guard entirely. +//! Reuses `agentflare_shim`'s generic resolve-real-binary/exec/propagate- +//! exit-code core -- the same plumbing the lean-ctx shim uses, applied +//! here to a different dispatch target. +//! +//! Installed onto PATH as `git`/`git.exe` in the same shim directory +//! `agentflare-shim` already uses (see `agentflare git install-shim`). + +use std::env; +use std::ffi::OsString; +use std::path::{Path, PathBuf}; +use std::process::exit; + +use agentflare_shim::{path_without_shim_dir, run_real, tool_name_from_exe}; +use flare_git_core::{audit, branch, classify, snapshot}; + +/// Recursion-depth backstop: incremented via an inherited env var on every +/// invocation. If this shim (or anything it spawns) ever resolves "git" +/// back to itself for any reason, this caps the blast radius at a handful +/// of processes instead of an unbounded spawn storm -- see +/// `flare_git_core::shell::git_binary`'s doc comment for the incident this +/// guards against. Independent of that fix: this backstop still applies if +/// some *other* internal call anywhere ever resolves "git" incorrectly. +const RECURSION_ENV: &str = "FLARE_GIT_SHIM_DEPTH"; +const MAX_RECURSION_DEPTH: u32 = 3; + +/// Global flags that redirect git to operate on a different repo than the +/// one resolved via cwd (`-C`, `--git-dir`, `--work-tree`) -- denied +/// outright rather than classified, since this shim's policy resolves the +/// target repo from cwd and has no safe way to re-resolve against a +/// caller-supplied override. +const ESCAPE_HATCH_FLAGS: &[&str] = &["-C", "--git-dir", "--work-tree"]; + +/// Global flags that consume the following argument as their value, so +/// subcommand detection can skip past both tokens. +const GLOBAL_FLAGS_WITH_VALUE: &[&str] = &["-c", "-C", "--git-dir", "--work-tree", "--namespace", "--exec-path"]; + +/// Finds the subcommand token's index, skipping global flags (and their +/// values, for flags that take one). Also reports whether an escape-hatch +/// flag appeared before the subcommand. +fn parse_global_flags(args: &[String]) -> (Option, bool) { + let mut i = 0; + let mut escape_hatch = false; + while i < args.len() { + let a = &args[i]; + if !a.starts_with('-') { + return (Some(i), escape_hatch); + } + if ESCAPE_HATCH_FLAGS.contains(&a.as_str()) + || ESCAPE_HATCH_FLAGS.iter().any(|f| a.starts_with(&format!("{f}="))) + { + escape_hatch = true; + } + i += if GLOBAL_FLAGS_WITH_VALUE.contains(&a.as_str()) { 2 } else { 1 }; + } + (None, escape_hatch) +} + +fn main() { + let depth: u32 = env::var(RECURSION_ENV) + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(0); + if depth >= MAX_RECURSION_DEPTH { + eprintln!( + "flare-git-shim: recursion guard tripped (depth {depth}) -- refusing to spawn further. This should never happen; please report it." + ); + exit(1); + } + // SAFETY: single-threaded at this point in main(), before any spawn. + unsafe { + env::set_var(RECURSION_ENV, (depth + 1).to_string()); + } + + let exe = match env::current_exe() { + Ok(p) => p, + Err(e) => { + eprintln!("flare-git-shim: failed to determine executable path: {e}"); + exit(1); + } + }; + let Some(tool) = tool_name_from_exe(&exe) else { + eprintln!("flare-git-shim: failed to determine tool name from executable path"); + exit(1); + }; + let shim_dir: PathBuf = exe.parent().map_or_else(PathBuf::new, Path::to_path_buf); + let args: Vec = env::args_os().skip(1).collect(); + let filtered_path = path_without_shim_dir(&shim_dir); + + // Only actually policing `git` -- if this binary ever ends up + // hardlinked/copied under another name (it shouldn't be), fall + // straight through rather than guessing at a policy for it. + if tool != "git" { + run_real(&tool, filtered_path.as_ref(), &args); + } + + let cwd = env::current_dir().unwrap_or_default(); + let Some(repo_root) = branch::repo_toplevel(&cwd) else { + // Not inside a git repo at all -- nothing to classify (e.g. `git + // init`, `git clone` into a fresh directory, `git --version`). + run_real(&tool, filtered_path.as_ref(), &args); + }; + + let str_args: Vec = args.iter().map(|a| a.to_string_lossy().into_owned()).collect(); + let (subcommand_idx, escape_hatch) = parse_global_flags(&str_args); + + if escape_hatch { + eprintln!( + "agentflare git shim: denied — this invocation uses -C/--git-dir/--work-tree to target a different repository, which this shim cannot classify safely." + ); + exit(1); + } + + let Some(idx) = subcommand_idx else { + run_real(&tool, filtered_path.as_ref(), &args); // e.g. bare `git --version` + }; + let subcommand = str_args[idx].clone(); + let rest: Vec = str_args[idx + 1..].to_vec(); + + let event = classify::classify(&repo_root, &subcommand, &rest); + + if let Some(audit_path) = audit::default_path() { + let _ = audit::log_event(&audit_path, &event); + } + + match &event.disposition { + classify::Disposition::Deny { reason } => { + eprintln!("agentflare git shim: denied — {reason}"); + exit(1); + } + classify::Disposition::RedirectToWorktree { path } => { + // Unreachable in v1 (see classify.rs's doc comment) -- fail + // closed with a clear message rather than pretending to + // execute a redirect this policy version never produces. + eprintln!( + "agentflare git shim: internal error — classify() returned RedirectToWorktree({}), which this shim version does not implement executing.", + path.display() + ); + exit(1); + } + classify::Disposition::Passthrough | classify::Disposition::SilentExempt => { + if classify::is_destructive(&subcommand, &rest) { + let reason = format!("pre-{subcommand} snapshot ({})", rest.join(" ")); + match snapshot::snapshot_before(&repo_root, &reason) { + Ok(id) => eprintln!( + "agentflare git shim: snapshotted before destructive '{subcommand}' (id {}) -- restorable if this goes wrong.", + id.0 + ), + Err(e) => eprintln!( + "agentflare git shim: warning -- snapshot before destructive '{subcommand}' failed: {e}" + ), + } + } + run_real(&tool, filtered_path.as_ref(), &args); + } + } +} diff --git a/crates/flare-git-shim/tests/shim_test.rs b/crates/flare-git-shim/tests/shim_test.rs new file mode 100644 index 00000000..a8ff5089 --- /dev/null +++ b/crates/flare-git-shim/tests/shim_test.rs @@ -0,0 +1,101 @@ +//! Integration tests: spawn the actual compiled shim binary against a real +//! temp repo, same as a caller on PATH would invoke it. `AGENTFLARE_HOME_OVERRIDE` +//! keeps the audit log out of the developer's real home directory. +//! +//! Test fixture setup goes through `flare_git_core::shell` (not a bare +//! `Command::new("git")`) so it resolves the real git binary the same +//! safe way the shim itself does -- a raw `Command::new("git")` here would +//! hit the exact self-resolution hazard `shell::git_binary` exists to +//! avoid, since this test binary also runs with the shared cargo +//! target/debug dir (where the shim's own `git.exe` lives) on PATH. + +use std::path::Path; +use std::process::{Command, Output}; + +fn init_repo() -> tempfile::TempDir { + let dir = tempfile::TempDir::new().unwrap(); + let path = dir.path(); + flare_git_core::shell::run_in(path, &["init", "-b", "master"]).unwrap(); + flare_git_core::shell::run_in(path, &["config", "user.email", "test@test.com"]).unwrap(); + flare_git_core::shell::run_in(path, &["config", "user.name", "Test"]).unwrap(); + flare_git_core::shell::run_in(path, &["commit", "--allow-empty", "-m", "initial"]).unwrap(); + dir +} + +/// Runs the compiled shim binary (built by this crate as `git`/`git.exe`) +/// against `repo`, with a scratch `AGENTFLARE_HOME_OVERRIDE` so the audit +/// log doesn't land in the developer's real `~/.agentflare/`. +fn shim(repo: &Path, home: &Path, args: &[&str]) -> Output { + Command::new(env!("CARGO_BIN_EXE_git")) + .args(args) + .current_dir(repo) + .env("AGENTFLARE_HOME_OVERRIDE", home) + .output() + .unwrap() +} + +#[test] +fn read_only_command_passes_through_and_succeeds() { + let repo = init_repo(); + let home = tempfile::TempDir::new().unwrap(); + let out = shim(repo.path(), home.path(), &["status"]); + assert!(out.status.success(), "{out:?}"); +} + +#[test] +fn checkout_to_protected_branch_is_denied_and_real_git_never_runs() { + let repo = init_repo(); + let home = tempfile::TempDir::new().unwrap(); + assert!(flare_git_core::shell::run_in_ok( + repo.path(), + &["checkout", "-b", "feature/x"] + )); + + let out = shim(repo.path(), home.path(), &["checkout", "master"]); + assert!(!out.status.success()); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(stderr.contains("denied"), "{stderr}"); + + // Real git must never have run -- still on feature/x. + let branch = flare_git_core::shell::run_in(repo.path(), &["branch", "--show-current"]).unwrap(); + assert_eq!(branch.trim(), "feature/x"); +} + +#[test] +fn unrecognized_subcommand_is_denied() { + let repo = init_repo(); + let home = tempfile::TempDir::new().unwrap(); + let out = shim(repo.path(), home.path(), &["some-made-up-subcommand"]); + assert!(!out.status.success()); +} + +#[test] +fn escape_hatch_flags_are_denied() { + let repo = init_repo(); + let home = tempfile::TempDir::new().unwrap(); + let out = shim(repo.path(), home.path(), &["-C", "/tmp", "status"]); + assert!(!out.status.success()); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(stderr.contains("denied"), "{stderr}"); +} + +#[test] +fn outside_a_git_repo_passes_through() { + let dir = tempfile::TempDir::new().unwrap(); + let home = tempfile::TempDir::new().unwrap(); + let out = shim(dir.path(), home.path(), &["--version"]); + assert!(out.status.success(), "{out:?}"); +} + +#[test] +fn denied_command_is_logged_to_the_audit_log() { + let repo = init_repo(); + let home = tempfile::TempDir::new().unwrap(); + let out = shim(repo.path(), home.path(), &["some-made-up-subcommand"]); + assert!(!out.status.success()); + + let audit_log = home.path().join(".agentflare").join("audit").join("git.jsonl"); + let content = std::fs::read_to_string(&audit_log).expect("audit log must exist"); + assert!(content.contains("some-made-up-subcommand"), "{content}"); + assert!(content.contains("Deny"), "{content}"); +} From 8f5e9a157f8f0f27a824d36fd00ca824bd5962b0 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 14:46:35 +0530 Subject: [PATCH 09/17] feat(flare-git-shim): dogfood-enablement -- bypass env var + install/uninstall CLI AGENTFLARE_GIT_BYPASS=1 skips classification entirely and execs the real binary unconditionally (still audited as a distinct event, so a bypass is visible after the fact, not silent). A misclassification during the dogfood period must never be able to block someone mid-work with no way out short of uninstalling the shim outright. New `agentflare git install-shim --binary ` / `uninstall-shim` commands: copies the compiled flare-git-shim binary into ~/.agentflare/shims/ (same directory agentflare-shim already uses) as git/git.exe, and prepends that directory to the User PATH on Windows via PowerShell's [Environment]::SetEnvironmentVariable. No auto-discovery of the binary yet -- this is the dogfooding install path, not the production release path (that will bundle the shim alongside the main binary via install.sh/install.ps1, deferred until after the dogfood period). Installed and smoke-tested locally: passthrough (status) and deny (checkout to a protected branch) both verified against a real repo via the installed binary directly. Full workspace build + test suite green (44 test-result groups), clippy clean, zero process leaks. --- crates/flare-git-shim/src/main.rs | 19 ++++ crates/flare-git-shim/tests/shim_test.rs | 19 ++++ src/cli/git.rs | 129 +++++++++++++++++++++++ 3 files changed, 167 insertions(+) diff --git a/crates/flare-git-shim/src/main.rs b/crates/flare-git-shim/src/main.rs index ffb3e8e3..bc6481f8 100644 --- a/crates/flare-git-shim/src/main.rs +++ b/crates/flare-git-shim/src/main.rs @@ -27,6 +27,13 @@ use flare_git_core::{audit, branch, classify, snapshot}; const RECURSION_ENV: &str = "FLARE_GIT_SHIM_DEPTH"; const MAX_RECURSION_DEPTH: u32 = 3; +/// Escape hatch for the dogfooding period (and beyond): set to skip +/// classification entirely and exec the real binary unconditionally. A +/// misclassification must never be able to block someone mid-work with no +/// way out short of uninstalling the shim. Still audited (as a distinct +/// disposition), so a bypass is visible after the fact, not silent. +const BYPASS_ENV: &str = "AGENTFLARE_GIT_BYPASS"; + /// Global flags that redirect git to operate on a different repo than the /// one resolved via cwd (`-C`, `--git-dir`, `--work-tree`) -- denied /// outright rather than classified, since this shim's policy resolves the @@ -104,6 +111,18 @@ fn main() { run_real(&tool, filtered_path.as_ref(), &args); }; + if agentflare_shim::is_set(BYPASS_ENV) { + if let Some(audit_path) = audit::default_path() { + let bypass_event = classify::Event { + subcommand: "*".to_string(), + args: args.iter().map(|a| a.to_string_lossy().into_owned()).collect(), + disposition: classify::Disposition::SilentExempt, + }; + let _ = audit::log_event(&audit_path, &bypass_event); + } + run_real(&tool, filtered_path.as_ref(), &args); + } + let str_args: Vec = args.iter().map(|a| a.to_string_lossy().into_owned()).collect(); let (subcommand_idx, escape_hatch) = parse_global_flags(&str_args); diff --git a/crates/flare-git-shim/tests/shim_test.rs b/crates/flare-git-shim/tests/shim_test.rs index a8ff5089..9fb8912f 100644 --- a/crates/flare-git-shim/tests/shim_test.rs +++ b/crates/flare-git-shim/tests/shim_test.rs @@ -87,6 +87,25 @@ fn outside_a_git_repo_passes_through() { assert!(out.status.success(), "{out:?}"); } +#[test] +fn bypass_env_var_skips_classification_even_for_a_denied_command() { + let repo = init_repo(); + let home = tempfile::TempDir::new().unwrap(); + let out = Command::new(env!("CARGO_BIN_EXE_git")) + .args(["some-made-up-subcommand"]) + .current_dir(repo.path()) + .env("AGENTFLARE_HOME_OVERRIDE", home.path()) + .env("AGENTFLARE_GIT_BYPASS", "1") + .output() + .unwrap(); + // Real git also rejects the made-up subcommand, but for a DIFFERENT + // reason (unknown git command, not "denied by the shim") -- assert on + // stderr content, not just a nonzero exit, so this test would fail if + // bypass silently started denying again. + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(!stderr.contains("denied"), "{stderr}"); +} + #[test] fn denied_command_is_logged_to_the_audit_log() { let repo = init_repo(); diff --git a/src/cli/git.rs b/src/cli/git.rs index a1cb387b..647559f5 100644 --- a/src/cli/git.rs +++ b/src/cli/git.rs @@ -29,6 +29,21 @@ pub struct GitArgs { pub enum GitCommand { /// Install branch-protection pre-commit/pre-push hooks into this repo. InstallHooks(InstallHooksArgs), + /// Install the flare-git-shim binary (dogfooding/local use) as `git` + /// on PATH, so every git invocation on this machine gets classified. + InstallShim(InstallShimArgs), + /// Remove the git shim installed by `install-shim`. + UninstallShim, +} + +#[derive(Args)] +pub struct InstallShimArgs { + /// Path to a compiled flare-git-shim binary (its `[[bin]] name = "git"` + /// target) to install. No auto-discovery yet -- this is a dogfooding + /// aid, not the production release path (that will bundle the shim + /// alongside the main binary via install.sh/install.ps1). + #[arg(long)] + pub binary: PathBuf, } #[derive(Args)] @@ -66,7 +81,121 @@ fn ensure_shared_templates() -> std::io::Result<()> { pub fn run(args: GitArgs) { match args.command { GitCommand::InstallHooks(opts) => install_hooks(opts), + GitCommand::InstallShim(opts) => install_shim(opts), + GitCommand::UninstallShim => uninstall_shim(), + } +} + +/// Canonical location: `~/.agentflare/shims/` -- same directory +/// `agentflare-shim` (item #227's lean-ctx PATH shim) already uses, so +/// there's one PATH entry to manage, not several. +fn shims_dir() -> PathBuf { + home().join(".agentflare").join("shims") +} + +fn shim_dest_name() -> &'static str { + if cfg!(windows) { "git.exe" } else { "git" } +} + +fn install_shim(opts: InstallShimArgs) { + let dir = shims_dir(); + if let Err(e) = fs::create_dir_all(&dir) { + crate::ui::error(&format!("agentflare git install-shim: cannot create {dir:?}: {e}")); + return; + } + let dest = dir.join(shim_dest_name()); + if let Err(e) = fs::copy(&opts.binary, &dest) { + crate::ui::error(&format!( + "agentflare git install-shim: cannot copy {:?} to {dest:?}: {e}", + opts.binary + )); + return; + } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let _ = fs::set_permissions(&dest, fs::Permissions::from_mode(0o755)); } + crate::ui::success(&format!("installed git shim -> {}", dest.display())); + + match ensure_on_path(&dir) { + Ok(true) => crate::ui::success(&format!( + "added {} to your User PATH -- restart your terminal/IDE to pick it up", + dir.display() + )), + Ok(false) => crate::ui::success(&format!("{} already on PATH", dir.display())), + Err(e) => crate::ui::error(&format!("agentflare git install-shim: could not update PATH: {e}")), + } + + println!( + " +Once your PATH refreshes, every `git` command on this machine is classified by the agentflare git shim. Escape hatch: set AGENTFLARE_GIT_BYPASS=1 to skip classification for a command/session without uninstalling. Remove entirely with `agentflare git uninstall-shim`." + ); +} + +fn uninstall_shim() { + let dest = shims_dir().join(shim_dest_name()); + if !dest.exists() { + crate::ui::success("git shim was not installed"); + return; + } + match fs::remove_file(&dest) { + Ok(()) => crate::ui::success(&format!("removed {}", dest.display())), + Err(e) => crate::ui::error(&format!("agentflare git uninstall-shim: cannot remove {dest:?}: {e}")), + } + // Deliberately leaves the shims dir on PATH -- other shims (e.g. the + // lean-ctx one) may still live there; removing just this binary is + // enough to fully restore normal git behavior. +} + +/// Prepends `dir` to the current user's persistent PATH (Windows: the +/// `User` environment scope via PowerShell, since it needs to survive +/// across terminal sessions and there's no portable non-shelling way to +/// do this without an extra crate). Returns `Ok(true)` if PATH was +/// changed, `Ok(false)` if `dir` was already present. +#[cfg(windows)] +fn ensure_on_path(dir: &std::path::Path) -> Result { + let dir_str = dir.to_string_lossy().to_string(); + let get = std::process::Command::new("powershell.exe") + .args([ + "-NoProfile", + "-Command", + "[Environment]::GetEnvironmentVariable('PATH','User')", + ]) + .output() + .map_err(|e| e.to_string())?; + let current = String::from_utf8_lossy(&get.stdout).trim().to_string(); + let already_present = current + .split(';') + .any(|p| p.trim_end_matches('\\').eq_ignore_ascii_case(dir_str.trim_end_matches('\\'))); + if already_present { + return Ok(false); + } + let new_path = if current.is_empty() { + dir_str.clone() + } else { + format!("{dir_str};{current}") + }; + let set_script = format!( + "[Environment]::SetEnvironmentVariable('PATH', '{}', 'User')", + new_path.replace('\'', "''") + ); + let set = std::process::Command::new("powershell.exe") + .args(["-NoProfile", "-Command", &set_script]) + .status() + .map_err(|e| e.to_string())?; + if !set.success() { + return Err("powershell SetEnvironmentVariable failed".to_string()); + } + Ok(true) +} + +#[cfg(not(windows))] +fn ensure_on_path(_dir: &std::path::Path) -> Result { + // Not needed for this dogfooding session (Windows-only machine); the + // real install.sh wiring will handle shell-profile PATH export the + // same way it already does for the main binary's install dir. + Ok(false) } fn install_hooks(opts: InstallHooksArgs) { From c0a71e28d4635a6f6c764ea9ca50fe42bb243807 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 15:19:32 +0530 Subject: [PATCH 10/17] feat(classify): fail-open by default -- never block a subcommand it doesn't recognize Reverses the v1 fail-closed default: an unrecognized git subcommand is now Passthrough, not Deny. This shim sits in front of daily-driver git usage (installed live on PATH for dogfooding) -- blocking anything it hasn't been explicitly taught about (submodule, bisect, notes, gc, lfs, and the rest of git's long tail) is a worse failure mode than under-classifying. Only the deliberately-chosen cases stay denied: protected-branch checkout/switch, push carrying trust-root changes, low-level plumbing, and `git worktree`. Those are known and intentional blocks, not "doesn't recognize it" gaps -- the distinction the fail-open default is built around. Updates flare-git-shim's integration tests accordingly: the unrecognized- subcommand and audit-log tests now assert passthrough instead of deny, and the bypass-env-var test switches to an actual deny case (protected- branch checkout) so it still meaningfully exercises "bypass overrides an explicit deny" rather than a no-op. --- crates/flare-git-core/src/classify.rs | 45 ++++++++++++++++-------- crates/flare-git-shim/tests/shim_test.rs | 34 ++++++++++++------ 2 files changed, 54 insertions(+), 25 deletions(-) diff --git a/crates/flare-git-core/src/classify.rs b/crates/flare-git-core/src/classify.rs index 3a1ee9a8..674b47ff 100644 --- a/crates/flare-git-core/src/classify.rs +++ b/crates/flare-git-core/src/classify.rs @@ -2,11 +2,15 @@ //! ` invocation gets classified into exactly one //! disposition before the shim decides whether to exec real git. //! -//! Fail-closed by default: a subcommand this policy doesn't explicitly -//! recognize is `Deny`, not `Passthrough`. A shim that silently passes -//! through anything it doesn't recognize defeats its own purpose the -//! moment git grows a new subcommand this policy hasn't been taught about. -//! `RedirectToWorktree` exists in the `Disposition` enum for API +//! Fail-OPEN by default: a subcommand this policy doesn't explicitly +//! recognize is `Passthrough`, not `Deny`. This is a live shim sitting in +//! front of someone's daily-driver git usage -- it must never block a +//! legitimate operation just because its allowlist hasn't caught up with +//! git's full subcommand surface (submodule, bisect, notes, gc, lfs, ...). +//! Only the specific, deliberately-chosen cases below (protected-branch +//! checkout/switch, trust-root push, low-level plumbing, `worktree`) are +//! ever denied -- those are known and intentional, not "doesn't recognize +//! it". `RedirectToWorktree` exists in the `Disposition` enum for API //! completeness (mirroring the inspiration project's 4-way model) but v1's //! policy never produces it — agentflare has no per-agent worktree binding //! data available at classify time yet. @@ -157,11 +161,10 @@ pub fn classify_pure( "worktree" => Disposition::Deny { reason: "'git worktree' is orchestrator-managed by agentflare — use the `item` MCP tool's claim flow instead of calling it directly.".to_string(), }, - _ => Disposition::Deny { - reason: format!( - "'git {subcommand}' is not a recognized command for the agentflare git shim (fail-closed default) — if this is legitimate day-to-day usage, it needs to be added to flare-git-core::classify's policy." - ), - }, + // Fail-open: anything not explicitly matched above is allowed through + // unchanged. This shim must never block a git subcommand it simply + // hasn't been taught about yet. + _ => Disposition::Passthrough, } } @@ -230,11 +233,25 @@ mod tests { } #[test] - fn unknown_subcommand_denies_by_default() { - assert!(matches!( + fn unknown_subcommand_passes_through_by_default() { + // Fail-open: this shim must never block a subcommand it hasn't + // been explicitly taught to deny. + assert_eq!( classify_pure("some-future-subcommand", &[], "master", false), - Disposition::Deny { .. } - )); + Disposition::Passthrough + ); + assert_eq!( + classify_pure("submodule", &args(&["update"]), "master", false), + Disposition::Passthrough + ); + assert_eq!( + classify_pure("bisect", &args(&["start"]), "master", false), + Disposition::Passthrough + ); + assert_eq!( + classify_pure("lfs", &args(&["pull"]), "master", false), + Disposition::Passthrough + ); } #[test] diff --git a/crates/flare-git-shim/tests/shim_test.rs b/crates/flare-git-shim/tests/shim_test.rs index 9fb8912f..b7b612e0 100644 --- a/crates/flare-git-shim/tests/shim_test.rs +++ b/crates/flare-git-shim/tests/shim_test.rs @@ -62,11 +62,15 @@ fn checkout_to_protected_branch_is_denied_and_real_git_never_runs() { } #[test] -fn unrecognized_subcommand_is_denied() { +fn unrecognized_subcommand_passes_through_to_real_git() { + // Fail-open: a subcommand this shim doesn't recognize is not denied by + // the shim -- it's handed to real git unchanged, which then rejects it + // for its OWN reason ("not a git command"), not "denied by the shim". let repo = init_repo(); let home = tempfile::TempDir::new().unwrap(); let out = shim(repo.path(), home.path(), &["some-made-up-subcommand"]); - assert!(!out.status.success()); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(!stderr.contains("denied"), "{stderr}"); } #[test] @@ -91,30 +95,38 @@ fn outside_a_git_repo_passes_through() { fn bypass_env_var_skips_classification_even_for_a_denied_command() { let repo = init_repo(); let home = tempfile::TempDir::new().unwrap(); + assert!(flare_git_core::shell::run_in_ok( + repo.path(), + &["checkout", "-b", "feature/x"] + )); let out = Command::new(env!("CARGO_BIN_EXE_git")) - .args(["some-made-up-subcommand"]) + .args(["checkout", "master"]) .current_dir(repo.path()) .env("AGENTFLARE_HOME_OVERRIDE", home.path()) .env("AGENTFLARE_GIT_BYPASS", "1") .output() .unwrap(); - // Real git also rejects the made-up subcommand, but for a DIFFERENT - // reason (unknown git command, not "denied by the shim") -- assert on - // stderr content, not just a nonzero exit, so this test would fail if - // bypass silently started denying again. - let stderr = String::from_utf8_lossy(&out.stderr); - assert!(!stderr.contains("denied"), "{stderr}"); + // Without bypass this exact command is the explicit protected-branch + // deny case (see checkout_to_protected_branch_is_denied_and_real_git_never_runs); + // with bypass it must run unconditionally. + assert!(out.status.success(), "{out:?}"); + let branch = flare_git_core::shell::run_in(repo.path(), &["branch", "--show-current"]).unwrap(); + assert_eq!(branch.trim(), "master"); } #[test] fn denied_command_is_logged_to_the_audit_log() { let repo = init_repo(); let home = tempfile::TempDir::new().unwrap(); - let out = shim(repo.path(), home.path(), &["some-made-up-subcommand"]); + assert!(flare_git_core::shell::run_in_ok( + repo.path(), + &["checkout", "-b", "feature/x"] + )); + let out = shim(repo.path(), home.path(), &["checkout", "master"]); assert!(!out.status.success()); let audit_log = home.path().join(".agentflare").join("audit").join("git.jsonl"); let content = std::fs::read_to_string(&audit_log).expect("audit log must exist"); - assert!(content.contains("some-made-up-subcommand"), "{content}"); + assert!(content.contains("checkout"), "{content}"); assert!(content.contains("Deny"), "{content}"); } From 28edde116786a1b9bed3a13ec5519cb58837f863 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 15:24:46 +0530 Subject: [PATCH 11/17] fix(classify): deny branch -D/-M against the protected branch Found while reviewing the fail-open change for side effects: `branch` was globally allowlisted as read-only, but `git branch -D/-M ` deletes or renames a branch -- a second way to destroy or rename the protected default branch's local ref, alongside checkout/switch (which already guard against it). Pre-existing gap, not introduced by the fail-open change, but directly in the same threat model so fixed here. Only delete/rename forms are checked; listing, creating a new branch, --set-upstream-to, etc. stay Passthrough. --- crates/flare-git-core/src/classify.rs | 65 +++++++++++++++++++++++++-- 1 file changed, 61 insertions(+), 4 deletions(-) diff --git a/crates/flare-git-core/src/classify.rs b/crates/flare-git-core/src/classify.rs index 674b47ff..b6ace707 100644 --- a/crates/flare-git-core/src/classify.rs +++ b/crates/flare-git-core/src/classify.rs @@ -8,9 +8,9 @@ //! legitimate operation just because its allowlist hasn't caught up with //! git's full subcommand surface (submodule, bisect, notes, gc, lfs, ...). //! Only the specific, deliberately-chosen cases below (protected-branch -//! checkout/switch, trust-root push, low-level plumbing, `worktree`) are -//! ever denied -- those are known and intentional, not "doesn't recognize -//! it". `RedirectToWorktree` exists in the `Disposition` enum for API +//! checkout/switch/delete/rename, trust-root push, low-level plumbing, +//! `worktree`) are ever denied -- those are known and intentional, not +//! "doesn't recognize it". `RedirectToWorktree` exists in the `Disposition` enum for API //! completeness (mirroring the inspiration project's 4-way model) but v1's //! policy never produces it — agentflare has no per-agent worktree binding //! data available at classify time yet. @@ -61,7 +61,6 @@ const READ_ONLY_SUBCOMMANDS: &[&str] = &[ "config", "remote", "tag", - "branch", "fetch", "clone", "help", @@ -135,6 +134,27 @@ pub fn classify_pure( }; } match subcommand { + // Deletion/rename lumped with checkout/switch below: `git branch + // -D/-M ` is a second way to destroy or rename the protected + // branch's local ref, not covered by the checkout/switch guard. + // Every other `branch` usage (listing, creating a new branch, + // --set-upstream-to, ...) stays Passthrough. + "branch" => { + let deletes_or_renames = args + .iter() + .any(|a| matches!(a.as_str(), "-D" | "-d" | "--delete" | "-M" | "-m" | "--move")); + if !deletes_or_renames { + return Disposition::Passthrough; + } + let targets: Vec<&str> = args.iter().filter(|a| !a.starts_with('-')).map(String::as_str).collect(); + if targets.iter().any(|t| is_protected_branch(t, Some(default_branch))) { + Disposition::Deny { + reason: "this 'git branch' invocation would delete or rename the repo's default branch — blocked by the agentflare git shim.".to_string(), + } + } else { + Disposition::Passthrough + } + } "checkout" | "switch" => { let Some(target) = args.iter().find(|a| !a.starts_with('-')) else { return Disposition::Passthrough; // no target arg (e.g. `git switch -`) — nothing to protect against @@ -313,6 +333,43 @@ mod tests { ); } + #[test] + fn branch_delete_of_protected_branch_is_denied() { + assert!(matches!( + classify_pure("branch", &args(&["-D", "master"]), "master", false), + Disposition::Deny { .. } + )); + assert!(matches!( + classify_pure("branch", &args(&["--delete", "master"]), "master", false), + Disposition::Deny { .. } + )); + } + + #[test] + fn branch_rename_of_protected_branch_is_denied() { + assert!(matches!( + classify_pure("branch", &args(&["-M", "master", "renamed"]), "master", false), + Disposition::Deny { .. } + )); + } + + #[test] + fn branch_delete_of_feature_branch_passes_through() { + assert_eq!( + classify_pure("branch", &args(&["-D", "feature/x"]), "master", false), + Disposition::Passthrough + ); + } + + #[test] + fn branch_listing_and_creation_pass_through() { + assert_eq!(classify_pure("branch", &[], "master", false), Disposition::Passthrough); + assert_eq!( + classify_pure("branch", &args(&["feature/new"]), "master", false), + Disposition::Passthrough + ); + } + #[test] fn is_destructive_flags_reset_hard_and_force_ops() { assert!(is_destructive("reset", &args(&["--hard"]))); From 13364b9bd18ded08722b79d5438984d386c90575 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 15:36:18 +0530 Subject: [PATCH 12/17] feat(flare-git-core): configurable protected branches/trust-roots + canonical-repo detach detection Closes several gaps found comparing against the agentic-git reference: audit.rs: log_event/read_events generalized to any Serialize/Deserialize type, default_path takes a log name. Adds RefTransaction/ RefTransactionEvent for the upcoming reference-transaction hook journal -- a backstop audit trail independent of the shim, since it fires for every ref move whether git went through the shim or not. branch.rs: is_protected_branch now also matches AGENTFLARE_GIT_PROTECTED_BRANCHES (comma-separated, trailing `*` glob for prefix matching, e.g. "release/*") -- the actual customization surface agentic-git's own policy.toml provides (protected-ref overrides), which this crate didn't have any equivalent of. is_protected_branch_among is the pure, testable core; the env-reading wrapper keeps the existing signature so no call site changes. Adds is_linked_worktree, needed to scope the canonical-repo guard below to the main checkout, not agent worktrees. classify.rs: push_touches_trust_root also honors AGENTFLARE_GIT_TRUST_ROOT_PATHS. Adds agent_invocation_detected (same env-var catalog agentflare-shim gates its own dispatch behind) and would_detach_head (checkout implicitly detaches for non-branch targets; switch never does without --detach; `--` path-restore forms never touch HEAD) -- the primitives the shim needs to deny detaching HEAD in the canonical checkout when agent-invoked, wired into flare-git-shim next. 63 tests, all green; clippy clean. --- crates/flare-git-core/src/audit.rs | 73 ++++++++++++---- crates/flare-git-core/src/branch.rs | 107 ++++++++++++++++++++++- crates/flare-git-core/src/classify.rs | 120 +++++++++++++++++++++++++- 3 files changed, 275 insertions(+), 25 deletions(-) diff --git a/crates/flare-git-core/src/audit.rs b/crates/flare-git-core/src/audit.rs index 5ab2be1b..b12f7f75 100644 --- a/crates/flare-git-core/src/audit.rs +++ b/crates/flare-git-core/src/audit.rs @@ -1,30 +1,51 @@ -//! Append-only audit log for the git-shim's classified events. Every -//! `classify::Event` handed to `log_event` is appended as one JSONL line — -//! callers that want to suppress `SilentExempt` noise decide that -//! themselves before calling; this module always logs whatever it's given. +//! Append-only, generic audit logging: any `Serialize`/`Deserialize` event +//! type can be appended as one JSONL line and read back. Used for two +//! distinct logs -- the git-shim's own classified events +//! (`classify::Event`, `default_path("git.jsonl")`) and the +//! `reference-transaction` hook's backstop ref-move journal +//! (`RefTransactionEvent`, `default_path("git-refs.jsonl")`), which fires +//! for every ref move in a repo regardless of whether git was invoked +//! through the shim at all. +use serde::{Deserialize, Serialize}; use std::io::Write as _; use std::path::{Path, PathBuf}; -use crate::classify::Event; +/// One ref update from a `reference-transaction` hook invocation -- +/// ` `, one per line on the hook's stdin. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct RefTransaction { + pub old: String, + pub new: String, + pub refname: String, +} + +/// A committed `reference-transaction`: the agent identity (if any -- +/// self-reported, see `provenance::build_trailers`) plus every ref it moved +/// in one transaction. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct RefTransactionEvent { + pub agent: Option, + pub transactions: Vec, +} -/// Default audit log location: `~/.agentflare/audit/git.jsonl`. Honors -/// `AGENTFLARE_HOME_OVERRIDE` (the main binary's own test/CI escape hatch, -/// see `src/paths.rs::home` -- `dirs::home_dir()` resolves via the OS -/// directly on Windows and ignores HOME/USERPROFILE overrides) so tests +/// Default location for a named audit log under `~/.agentflare/audit/`. +/// Honors `AGENTFLARE_HOME_OVERRIDE` (the main binary's own test/CI escape +/// hatch, see `src/paths.rs::home` -- `dirs::home_dir()` resolves via the +/// OS directly on Windows and ignores HOME/USERPROFILE overrides) so tests /// never write into a developer's real home directory. #[must_use] -pub fn default_path() -> Option { +pub fn default_path(name: &str) -> Option { let home = match std::env::var("AGENTFLARE_HOME_OVERRIDE") { Ok(p) => PathBuf::from(p), Err(_) => dirs::home_dir()?, }; - Some(home.join(".agentflare").join("audit").join("git.jsonl")) + Some(home.join(".agentflare").join("audit").join(name)) } /// Appends one JSONL line for `event`, creating the parent directory and /// file if needed. -pub fn log_event(audit_path: &Path, event: &Event) -> std::io::Result<()> { +pub fn log_event(audit_path: &Path, event: &T) -> std::io::Result<()> { if let Some(parent) = audit_path.parent() { std::fs::create_dir_all(parent)?; } @@ -40,7 +61,7 @@ pub fn log_event(audit_path: &Path, event: &Event) -> std::io::Result<()> { /// as empty (nothing has been logged yet — not an error). A malformed line /// fails closed: returns an error rather than silently skipping it, since /// a corrupt audit entry is a bug worth surfacing, not data to quietly drop. -pub fn read_events(audit_path: &Path) -> std::io::Result> { +pub fn read_events Deserialize<'de>>(audit_path: &Path) -> std::io::Result> { let content = match std::fs::read_to_string(audit_path) { Ok(c) => c, Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(Vec::new()), @@ -56,7 +77,7 @@ pub fn read_events(audit_path: &Path) -> std::io::Result> { #[cfg(test)] mod tests { use super::*; - use crate::classify::Disposition; + use crate::classify::{Disposition, Event}; use tempfile::TempDir; fn sample_event(subcommand: &str) -> Event { @@ -71,7 +92,7 @@ mod tests { fn reading_a_missing_log_is_empty_not_an_error() { let dir = TempDir::new().unwrap(); let path = dir.path().join("does-not-exist").join("git.jsonl"); - assert_eq!(read_events(&path).unwrap(), Vec::new()); + assert_eq!(read_events::(&path).unwrap(), Vec::new()); } #[test] @@ -80,7 +101,7 @@ mod tests { let path = dir.path().join("audit").join("git.jsonl"); log_event(&path, &sample_event("fetch")).unwrap(); log_event(&path, &sample_event("push")).unwrap(); - let events = read_events(&path).unwrap(); + let events: Vec = read_events(&path).unwrap(); assert_eq!(events, vec![sample_event("fetch"), sample_event("push")]); } @@ -96,7 +117,7 @@ mod tests { }, }; log_event(&path, &event).unwrap(); - assert_eq!(read_events(&path).unwrap(), vec![event]); + assert_eq!(read_events::(&path).unwrap(), vec![event]); } #[test] @@ -111,8 +132,24 @@ mod tests { .write_all(b"not valid json\n") .unwrap(); assert!( - read_events(&path).is_err(), + read_events::(&path).is_err(), "a corrupt line must surface as an error, not be silently skipped" ); } + + #[test] + fn ref_transaction_events_round_trip() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("git-refs.jsonl"); + let event = RefTransactionEvent { + agent: Some("claude-code".to_string()), + transactions: vec![RefTransaction { + old: "0".repeat(40), + new: "a".repeat(40), + refname: "refs/heads/feature/x".to_string(), + }], + }; + log_event(&path, &event).unwrap(); + assert_eq!(read_events::(&path).unwrap(), vec![event]); + } } diff --git a/crates/flare-git-core/src/branch.rs b/crates/flare-git-core/src/branch.rs index 98d908b6..e630bda7 100644 --- a/crates/flare-git-core/src/branch.rs +++ b/crates/flare-git-core/src/branch.rs @@ -45,15 +45,75 @@ pub fn repo_toplevel(start: &Path) -> Option { run_in_opt(start, &["rev-parse", "--show-toplevel"]).map(PathBuf::from) } +/// `true` if `pattern` matches `branch` -- exact match, or a prefix match +/// when `pattern` ends in `*` (e.g. `"release/*"` matches `"release/1.0"`). +/// No other glob syntax; this is meant to cover "a branch and its +/// children", not a general pattern language. +fn matches_pattern(branch: &str, pattern: &str) -> bool { + match pattern.strip_suffix('*') { + Some(prefix) => branch.starts_with(prefix), + None => branch == pattern, + } +} + +/// `true` if `branch` is protected: the repo's resolved default branch (or +/// a bare main/master guess when resolution failed), OR matches any +/// pattern in `extra` (see `matches_pattern`). Pure -- `extra` is passed in +/// rather than read from an env var here, so this is unit-testable without +/// env-var mutation races between parallel test threads. +#[must_use] +pub fn is_protected_branch_among(branch: &str, default: Option<&str>, extra: &[String]) -> bool { + let is_default = match default { + Some(default) => branch == default, + None => branch == "main" || branch == "master", + }; + is_default || extra.iter().any(|p| matches_pattern(branch, p)) +} + +/// `AGENTFLARE_GIT_PROTECTED_BRANCHES`, comma-separated, parsed into a +/// pattern list -- e.g. `"main,release/*,staging"`. Empty/unset -> no +/// extra patterns. +#[must_use] +pub fn extra_protected_branches_from_env() -> Vec { + std::env::var("AGENTFLARE_GIT_PROTECTED_BRANCHES") + .ok() + .map(|v| { + v.split(',') + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_string) + .collect() + }) + .unwrap_or_default() +} + /// `true` if `branch` is protected: the repo's resolved default branch when /// known, otherwise a bare guess against the two conventional names /// ("main"/"master") when resolution failed entirely (no git, no remote, no -/// main/master branch found). +/// main/master branch found) -- plus anything matching +/// `AGENTFLARE_GIT_PROTECTED_BRANCHES`. See `is_protected_branch_among` for +/// the pure, testable core this wraps. #[must_use] pub fn is_protected_branch(branch: &str, default: Option<&str>) -> bool { - match default { - Some(default) => branch == default, - None => branch == "main" || branch == "master", + is_protected_branch_among(branch, default, &extra_protected_branches_from_env()) +} + +/// `true` if `repo_root` is a linked worktree rather than the canonical/ +/// main checkout -- `git rev-parse --git-dir` differs from +/// `--git-common-dir` only inside a linked worktree (or, rarely, a +/// submodule; this doesn't disambiguate the two, matching +/// `worktree::already_isolated_for`'s existing simpler check). `false` for +/// anything unresolvable (not a repo at all) -- "protect the canonical +/// checkout" should never fire on "couldn't tell", since that would be a +/// much broader and more surprising deny surface than intended. +#[must_use] +pub fn is_linked_worktree(repo_root: &Path) -> bool { + match ( + run_in_opt(repo_root, &["rev-parse", "--git-dir"]), + run_in_opt(repo_root, &["rev-parse", "--git-common-dir"]), + ) { + (Some(git_dir), Some(common_dir)) => git_dir != common_dir, + _ => false, } } @@ -107,4 +167,43 @@ mod tests { assert!(is_protected_branch("main", None)); assert!(!is_protected_branch("feature/x", None)); } + + #[test] + fn is_protected_branch_among_matches_extra_exact_and_glob_patterns() { + let extra = vec!["staging".to_string(), "release/*".to_string()]; + assert!(is_protected_branch_among("staging", Some("main"), &extra)); + assert!(is_protected_branch_among("release/1.0", Some("main"), &extra)); + assert!(!is_protected_branch_among("release", Some("main"), &extra)); + assert!(!is_protected_branch_among("feature/x", Some("main"), &extra)); + } + + #[test] + fn is_protected_branch_among_with_no_extra_matches_only_default() { + assert!(is_protected_branch_among("main", Some("main"), &[])); + assert!(!is_protected_branch_among("staging", Some("main"), &[])); + } + + #[test] + fn is_linked_worktree_false_in_a_regular_repo() { + let repo = init_repo_with_branch("master"); + assert!(!is_linked_worktree(&repo.path)); + } + + #[test] + fn is_linked_worktree_false_outside_any_repo() { + let dir = tempfile::TempDir::new().unwrap(); + assert!(!is_linked_worktree(dir.path())); + } + + #[test] + fn is_linked_worktree_true_inside_an_actual_linked_worktree() { + let repo = init_repo_with_branch("master"); + let wt_path = repo.path.parent().unwrap().join("wt-check"); + crate::shell::run_in( + &repo.path, + &["worktree", "add", wt_path.to_str().unwrap(), "-b", "wt-branch"], + ) + .unwrap(); + assert!(is_linked_worktree(&wt_path)); + } } diff --git a/crates/flare-git-core/src/classify.rs b/crates/flare-git-core/src/classify.rs index b6ace707..9997e2b3 100644 --- a/crates/flare-git-core/src/classify.rs +++ b/crates/flare-git-core/src/classify.rs @@ -40,6 +40,71 @@ pub struct Event { /// change to and quietly weaken. const TRUST_ROOT_PATHS: &[&str] = &[".githooks/", ".agentflare/", "Cargo.toml"]; +/// `AGENTFLARE_GIT_TRUST_ROOT_PATHS`, comma-separated, appended to +/// `TRUST_ROOT_PATHS` -- e.g. `".githooks/,policy.toml"`. Empty/unset -> +/// no extra paths. +#[must_use] +pub fn extra_trust_root_paths_from_env() -> Vec { + std::env::var("AGENTFLARE_GIT_TRUST_ROOT_PATHS") + .ok() + .map(|v| { + v.split(',') + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_string) + .collect() + }) + .unwrap_or_default() +} + +/// Env vars agent CLIs set on themselves -- same catalog `agentflare-shim` +/// gates its own dispatch behind. Used ONLY to scope the canonical-repo +/// mutation guard (see `would_detach_head`) to agent-driven invocations -- +/// never to change ordinary git behavior for interactive human use. +const AGENT_ENV_VARS: &[&str] = &[ + "CLAUDECODE", + "CURSOR_AGENT", + "CODEX_CLI_SESSION", + "GEMINI_SESSION", + "CODEBUDDY", + "AGENTFLARE_AGENT", +]; + +/// `true` if any agent-identifying env var is set -- this invocation is +/// (self-reportedly) agent-driven, not an interactive human shell. +#[must_use] +pub fn agent_invocation_detected() -> bool { + AGENT_ENV_VARS + .iter() + .any(|v| std::env::var_os(v).is_some_and(|s| !s.is_empty())) +} + +/// `true` if `subcommand`/`args` would detach HEAD -- `git checkout +/// ` implicitly detaches when `target` isn't an existing local +/// branch (no `--detach` flag required for that form); `git switch` never +/// silently detaches, only `switch --detach`/`-d` does. `git checkout -- +/// ` (and any form with `--` before the target) restores files +/// and never touches HEAD at all. +#[must_use] +pub fn would_detach_head(repo_root: &Path, subcommand: &str, args: &[String]) -> bool { + match subcommand { + "checkout" => { + if args.iter().any(|a| a == "--") { + return false; // path-restore form -- HEAD never moves + } + if args.iter().any(|a| a == "--detach") { + return true; + } + let Some(target) = args.iter().find(|a| !a.starts_with('-')) else { + return false; // e.g. bare `git checkout` -- doesn't move HEAD + }; + !crate::shell::run_in_ok(repo_root, &["show-ref", "--verify", "--quiet", &format!("refs/heads/{target}")]) + } + "switch" => args.iter().any(|a| a == "--detach" || a == "-d"), + _ => false, + } +} + /// Ordinary, non-destructive read-only subcommands — always `Passthrough` /// regardless of args. const READ_ONLY_SUBCOMMANDS: &[&str] = &[ @@ -194,11 +259,12 @@ pub fn classify_pure( /// default to let through. #[must_use] pub fn push_touches_trust_root(repo_root: &Path, branch: &str, target: &str) -> bool { + let extra = extra_trust_root_paths_from_env(); let range = format!("{target}...{branch}"); match crate::shell::run_in(repo_root, &["diff", "--name-only", &range]) { - Ok(names) => names - .lines() - .any(|f| TRUST_ROOT_PATHS.iter().any(|p| f.starts_with(p))), + Ok(names) => names.lines().any(|f| { + TRUST_ROOT_PATHS.iter().any(|p| f.starts_with(p)) || extra.iter().any(|p| f.starts_with(p.as_str())) + }), Err(_) => true, } } @@ -370,6 +436,54 @@ mod tests { ); } + #[test] + fn would_detach_head_true_for_a_non_branch_checkout_target() { + let repo = crate::shell::test_support::init_repo_with_branch("master"); + // A commit sha (via HEAD) is not a branch name -- checking it out + // implicitly detaches. + let sha = crate::shell::run_in(&repo.path, &["rev-parse", "HEAD"]).unwrap(); + assert!(would_detach_head(&repo.path, "checkout", &args(&[&sha]))); + } + + #[test] + fn would_detach_head_false_for_an_existing_branch_checkout_target() { + let repo = crate::shell::test_support::init_repo_with_branch("master"); + crate::shell::run_in(&repo.path, &["branch", "feature/x"]).unwrap(); + assert!(!would_detach_head(&repo.path, "checkout", &args(&["feature/x"]))); + } + + #[test] + fn would_detach_head_false_for_path_restore_form() { + let repo = crate::shell::test_support::init_repo_with_branch("master"); + let sha = crate::shell::run_in(&repo.path, &["rev-parse", "HEAD"]).unwrap(); + assert!(!would_detach_head( + &repo.path, + "checkout", + &args(&[&sha, "--", "some-file.txt"]) + )); + } + + #[test] + fn would_detach_head_true_for_explicit_detach_flag() { + let repo = crate::shell::test_support::init_repo_with_branch("master"); + assert!(would_detach_head(&repo.path, "checkout", &args(&["--detach", "master"]))); + assert!(would_detach_head(&repo.path, "switch", &args(&["--detach", "master"]))); + } + + #[test] + fn would_detach_head_false_for_plain_switch_to_a_branch() { + assert!(!would_detach_head( + std::path::Path::new("."), + "switch", + &args(&["feature/x"]) + )); + } + + #[test] + fn would_detach_head_false_for_unrelated_subcommands() { + assert!(!would_detach_head(std::path::Path::new("."), "status", &[])); + } + #[test] fn is_destructive_flags_reset_hard_and_force_ops() { assert!(is_destructive("reset", &args(&["--hard"]))); From 9a93a58d4608f345088854870ca6b403378978c2 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 15:41:03 +0530 Subject: [PATCH 13/17] feat(flare-git-shim): tiered bypass, snapshot toggle, canonical-repo detach guard Closes three more agentic-git parity gaps in the shim binary itself: Tiered bypass: AGENTFLARE_GIT_BYPASS_AGENT (bypass iff it matches AGENTFLARE_AGENT) and AGENTFLARE_GIT_BYPASS_UNTIL (bypass iff now < this unix epoch) join the existing one-shot AGENTFLARE_GIT_BYPASS. AGENTFLARE_GIT_SNAPSHOTS=0/off disables the automatic pre-destructive snapshot. Default stays ON (unlike the reference project's off-by-default raw-shim-mode) -- this shim has no separate "launched session" mode, so a safety net that's on by default is the safer choice for something installed directly on someone's daily-driver PATH. Canonical-repo HEAD-detach guard: denies an agent-invoked (self-reported env markers) checkout/switch that would detach HEAD in the canonical (non-worktree) checkout -- e.g. `git checkout ` run by an agent directly in the main checkout, one of the concrete failure modes the agentic-git README calls out by name. Scoped tightly on three conditions (agent-invoked AND canonical checkout AND would actually detach) so interactive human use and any worktree use are completely unaffected -- verified by dedicated tests that explicitly strip inherited agent-env markers (this test process itself runs under CLAUDECODE=1). Escape hatch: AGENTFLARE_GIT_ALLOW_CANONICAL_MUTATE=1. 76 tests total across both crates, all green; clippy clean; 0 process leaks. --- crates/flare-git-core/src/branch.rs | 6 +- crates/flare-git-shim/src/main.rs | 111 ++++++++++++++-- crates/flare-git-shim/tests/shim_test.rs | 155 +++++++++++++++++++++++ 3 files changed, 260 insertions(+), 12 deletions(-) diff --git a/crates/flare-git-core/src/branch.rs b/crates/flare-git-core/src/branch.rs index e630bda7..3e2c647e 100644 --- a/crates/flare-git-core/src/branch.rs +++ b/crates/flare-git-core/src/branch.rs @@ -198,7 +198,11 @@ mod tests { #[test] fn is_linked_worktree_true_inside_an_actual_linked_worktree() { let repo = init_repo_with_branch("master"); - let wt_path = repo.path.parent().unwrap().join("wt-check"); + // A fresh TempDir of its own -- `repo.path.parent()` is the SHARED + // system temp root, which every parallel test also creates + // directories under, and can collide with a leftover path. + let wt_parent = tempfile::TempDir::new().unwrap(); + let wt_path = wt_parent.path().join("wt-check"); crate::shell::run_in( &repo.path, &["worktree", "add", wt_path.to_str().unwrap(), "-b", "wt-branch"], diff --git a/crates/flare-git-shim/src/main.rs b/crates/flare-git-shim/src/main.rs index bc6481f8..02c1d180 100644 --- a/crates/flare-git-shim/src/main.rs +++ b/crates/flare-git-shim/src/main.rs @@ -13,6 +13,7 @@ use std::env; use std::ffi::OsString; use std::path::{Path, PathBuf}; use std::process::exit; +use std::time::{SystemTime, UNIX_EPOCH}; use agentflare_shim::{path_without_shim_dir, run_real, tool_name_from_exe}; use flare_git_core::{audit, branch, classify, snapshot}; @@ -27,12 +28,30 @@ use flare_git_core::{audit, branch, classify, snapshot}; const RECURSION_ENV: &str = "FLARE_GIT_SHIM_DEPTH"; const MAX_RECURSION_DEPTH: u32 = 3; -/// Escape hatch for the dogfooding period (and beyond): set to skip -/// classification entirely and exec the real binary unconditionally. A -/// misclassification must never be able to block someone mid-work with no -/// way out short of uninstalling the shim. Still audited (as a distinct -/// disposition), so a bypass is visible after the fact, not silent. -const BYPASS_ENV: &str = "AGENTFLARE_GIT_BYPASS"; +/// Tiered bypass, escape hatches for the dogfooding period (and beyond). +/// All three skip classification entirely and exec the real binary +/// unconditionally -- a misclassification must never be able to block +/// someone mid-work with no way out short of uninstalling the shim. Still +/// audited (as a distinct disposition), so a bypass is visible after the +/// fact, not silent. +const BYPASS_ENV: &str = "AGENTFLARE_GIT_BYPASS"; // one-shot: set at all -> bypass +const BYPASS_AGENT_ENV: &str = "AGENTFLARE_GIT_BYPASS_AGENT"; // bypass iff it equals AGENTFLARE_AGENT +const BYPASS_UNTIL_ENV: &str = "AGENTFLARE_GIT_BYPASS_UNTIL"; // bypass iff now < this unix epoch + +/// `AGENTFLARE_GIT_SNAPSHOTS=0`/`off` disables the automatic pre-destructive +/// snapshot; any other value (or unset) leaves it enabled. Snapshotting is +/// a pure safety net (never blocks the underlying op even on failure), so +/// the default here is ON -- unlike the reference project, which defaults +/// it off in raw shim mode and on only inside its own launched sessions; +/// this shim has no separate "launched session" mode, so ON is the safer +/// default for a shim installed directly on someone's daily-driver PATH. +const SNAPSHOTS_ENV: &str = "AGENTFLARE_GIT_SNAPSHOTS"; + +/// Escape hatch for the canonical-repo HEAD-detach guard (see +/// `deny_canonical_detach_reason`) -- set to allow an agent-invoked +/// checkout/switch that would detach HEAD in the canonical (non-worktree) +/// checkout. +const ALLOW_CANONICAL_MUTATE_ENV: &str = "AGENTFLARE_GIT_ALLOW_CANONICAL_MUTATE"; /// Global flags that redirect git to operate on a different repo than the /// one resolved via cwd (`-C`, `--git-dir`, `--work-tree`) -- denied @@ -66,6 +85,63 @@ fn parse_global_flags(args: &[String]) -> (Option, bool) { (None, escape_hatch) } +/// `true` if any bypass condition is currently active. +fn bypass_active() -> bool { + if agentflare_shim::is_set(BYPASS_ENV) { + return true; + } + if let Ok(target_agent) = env::var(BYPASS_AGENT_ENV) + && !target_agent.is_empty() + && env::var("AGENTFLARE_AGENT").ok().as_deref() == Some(target_agent.as_str()) + { + return true; + } + if let Ok(until) = env::var(BYPASS_UNTIL_ENV) + && let Ok(until_epoch) = until.parse::() + { + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or(0); + if now < until_epoch { + return true; + } + } + false +} + +/// `false` only when explicitly disabled via `AGENTFLARE_GIT_SNAPSHOTS=0`/`off`. +fn snapshots_enabled() -> bool { + match env::var(SNAPSHOTS_ENV) { + Ok(v) => v != "0" && !v.eq_ignore_ascii_case("off"), + Err(_) => true, + } +} + +/// Deny reason for the canonical-repo HEAD-detach guard, or `None` to let +/// the op through. Scoped tightly on purpose: agent-invoked (self-reported +/// via env markers, same as `agentflare-shim`'s own gate) AND the canonical +/// (non-worktree) checkout AND the command would actually detach HEAD. +/// Interactive human use, and any use inside an isolated worktree, is +/// completely unaffected. +fn deny_canonical_detach_reason(repo_root: &Path, subcommand: &str, args: &[String]) -> Option { + if agentflare_shim::is_set(ALLOW_CANONICAL_MUTATE_ENV) { + return None; + } + if !classify::agent_invocation_detected() { + return None; + } + if branch::is_linked_worktree(repo_root) { + return None; // agent worktrees are exactly where this is expected + } + if !classify::would_detach_head(repo_root, subcommand, args) { + return None; + } + Some(format!( + "this would detach HEAD in the canonical checkout (not an isolated worktree) while agent-invoked -- set {ALLOW_CANONICAL_MUTATE_ENV}=1 to override, or work in an isolated worktree instead." + )) +} + fn main() { let depth: u32 = env::var(RECURSION_ENV) .ok() @@ -111,8 +187,8 @@ fn main() { run_real(&tool, filtered_path.as_ref(), &args); }; - if agentflare_shim::is_set(BYPASS_ENV) { - if let Some(audit_path) = audit::default_path() { + if bypass_active() { + if let Some(audit_path) = audit::default_path("git.jsonl") { let bypass_event = classify::Event { subcommand: "*".to_string(), args: args.iter().map(|a| a.to_string_lossy().into_owned()).collect(), @@ -139,9 +215,22 @@ fn main() { let subcommand = str_args[idx].clone(); let rest: Vec = str_args[idx + 1..].to_vec(); + if let Some(reason) = deny_canonical_detach_reason(&repo_root, &subcommand, &rest) { + let event = classify::Event { + subcommand: subcommand.clone(), + args: rest.clone(), + disposition: classify::Disposition::Deny { reason: reason.clone() }, + }; + if let Some(audit_path) = audit::default_path("git.jsonl") { + let _ = audit::log_event(&audit_path, &event); + } + eprintln!("agentflare git shim: denied — {reason}"); + exit(1); + } + let event = classify::classify(&repo_root, &subcommand, &rest); - if let Some(audit_path) = audit::default_path() { + if let Some(audit_path) = audit::default_path("git.jsonl") { let _ = audit::log_event(&audit_path, &event); } @@ -161,11 +250,11 @@ fn main() { exit(1); } classify::Disposition::Passthrough | classify::Disposition::SilentExempt => { - if classify::is_destructive(&subcommand, &rest) { + if snapshots_enabled() && classify::is_destructive(&subcommand, &rest) { let reason = format!("pre-{subcommand} snapshot ({})", rest.join(" ")); match snapshot::snapshot_before(&repo_root, &reason) { Ok(id) => eprintln!( - "agentflare git shim: snapshotted before destructive '{subcommand}' (id {}) -- restorable if this goes wrong.", + "agentflare git shim: snapshotted before destructive '{subcommand}' (id {}) -- restore with `agentflare git snapshot restore`.", id.0 ), Err(e) => eprintln!( diff --git a/crates/flare-git-shim/tests/shim_test.rs b/crates/flare-git-shim/tests/shim_test.rs index b7b612e0..f8c2c441 100644 --- a/crates/flare-git-shim/tests/shim_test.rs +++ b/crates/flare-git-shim/tests/shim_test.rs @@ -130,3 +130,158 @@ fn denied_command_is_logged_to_the_audit_log() { assert!(content.contains("checkout"), "{content}"); assert!(content.contains("Deny"), "{content}"); } + +#[test] +fn bypass_agent_env_var_bypasses_only_for_the_matching_agent() { + let repo = init_repo(); + let home = tempfile::TempDir::new().unwrap(); + assert!(flare_git_core::shell::run_in_ok( + repo.path(), + &["checkout", "-b", "feature/x"] + )); + + // Matching agent -- bypasses. + let out = Command::new(env!("CARGO_BIN_EXE_git")) + .args(["checkout", "master"]) + .current_dir(repo.path()) + .env("AGENTFLARE_HOME_OVERRIDE", home.path()) + .env("AGENTFLARE_AGENT", "claude-code") + .env("AGENTFLARE_GIT_BYPASS_AGENT", "claude-code") + .output() + .unwrap(); + assert!(out.status.success(), "{out:?}"); + + // Back to feature/x, try again with a DIFFERENT agent -- must still deny. + flare_git_core::shell::run_in(repo.path(), &["checkout", "feature/x"]).unwrap(); + let out = Command::new(env!("CARGO_BIN_EXE_git")) + .args(["checkout", "master"]) + .current_dir(repo.path()) + .env("AGENTFLARE_HOME_OVERRIDE", home.path()) + .env("AGENTFLARE_AGENT", "some-other-agent") + .env("AGENTFLARE_GIT_BYPASS_AGENT", "claude-code") + .output() + .unwrap(); + assert!(!out.status.success(), "{out:?}"); +} + +#[test] +fn bypass_until_env_var_respects_the_deadline() { + let repo = init_repo(); + let home = tempfile::TempDir::new().unwrap(); + assert!(flare_git_core::shell::run_in_ok( + repo.path(), + &["checkout", "-b", "feature/x"] + )); + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs(); + + // Deadline in the past -- must still deny. + let out = Command::new(env!("CARGO_BIN_EXE_git")) + .args(["checkout", "master"]) + .current_dir(repo.path()) + .env("AGENTFLARE_HOME_OVERRIDE", home.path()) + .env("AGENTFLARE_GIT_BYPASS_UNTIL", (now - 60).to_string()) + .output() + .unwrap(); + assert!(!out.status.success(), "{out:?}"); + + // Deadline in the future -- bypasses. + let out = Command::new(env!("CARGO_BIN_EXE_git")) + .args(["checkout", "master"]) + .current_dir(repo.path()) + .env("AGENTFLARE_HOME_OVERRIDE", home.path()) + .env("AGENTFLARE_GIT_BYPASS_UNTIL", (now + 3600).to_string()) + .output() + .unwrap(); + assert!(out.status.success(), "{out:?}"); +} + +#[test] +fn snapshots_disabled_env_var_skips_the_pre_destructive_snapshot() { + let repo = init_repo(); + let home = tempfile::TempDir::new().unwrap(); + std::fs::write(repo.path().join("f.txt"), "dirty").unwrap(); + + let out = Command::new(env!("CARGO_BIN_EXE_git")) + .args(["reset", "--hard"]) + .current_dir(repo.path()) + .env("AGENTFLARE_HOME_OVERRIDE", home.path()) + .env("AGENTFLARE_GIT_SNAPSHOTS", "0") + .output() + .unwrap(); + assert!(out.status.success(), "{out:?}"); + assert!( + flare_git_core::snapshot::list(repo.path()).is_empty(), + "no snapshot should have been taken with AGENTFLARE_GIT_SNAPSHOTS=0" + ); +} + +#[test] +fn snapshots_enabled_by_default_before_a_destructive_op() { + let repo = init_repo(); + let home = tempfile::TempDir::new().unwrap(); + std::fs::write(repo.path().join("f.txt"), "dirty").unwrap(); + + let out = shim(repo.path(), home.path(), &["reset", "--hard"]); + assert!(out.status.success(), "{out:?}"); + assert!( + !flare_git_core::snapshot::list(repo.path()).is_empty(), + "a snapshot should have been taken by default" + ); +} + +#[test] +fn canonical_repo_detach_is_denied_for_agent_invocation_but_not_human() { + let repo = init_repo(); + let home = tempfile::TempDir::new().unwrap(); + let sha = flare_git_core::shell::run_in(repo.path(), &["rev-parse", "HEAD"]).unwrap(); + + // Agent-invoked (CLAUDECODE marker set) -- denied. + let out = Command::new(env!("CARGO_BIN_EXE_git")) + .args(["checkout", &sha]) + .current_dir(repo.path()) + .env("AGENTFLARE_HOME_OVERRIDE", home.path()) + .env("CLAUDECODE", "1") + .output() + .unwrap(); + assert!(!out.status.success(), "{out:?}"); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(stderr.contains("canonical checkout"), "{stderr}"); + + // No agent marker -- ordinary human usage, passes through. This test + // process itself may be running under an agent-marked environment (it + // is, under Claude Code -- CLAUDECODE=1), so explicitly strip every + // agent marker rather than relying on ambient absence. + let out = Command::new(env!("CARGO_BIN_EXE_git")) + .args(["checkout", &sha]) + .current_dir(repo.path()) + .env("AGENTFLARE_HOME_OVERRIDE", home.path()) + .env_remove("CLAUDECODE") + .env_remove("CURSOR_AGENT") + .env_remove("CODEX_CLI_SESSION") + .env_remove("GEMINI_SESSION") + .env_remove("CODEBUDDY") + .env_remove("AGENTFLARE_AGENT") + .output() + .unwrap(); + assert!(out.status.success(), "{out:?}"); +} + +#[test] +fn canonical_repo_detach_allowed_with_escape_hatch() { + let repo = init_repo(); + let home = tempfile::TempDir::new().unwrap(); + let sha = flare_git_core::shell::run_in(repo.path(), &["rev-parse", "HEAD"]).unwrap(); + + let out = Command::new(env!("CARGO_BIN_EXE_git")) + .args(["checkout", &sha]) + .current_dir(repo.path()) + .env("AGENTFLARE_HOME_OVERRIDE", home.path()) + .env("CLAUDECODE", "1") + .env("AGENTFLARE_GIT_ALLOW_CANONICAL_MUTATE", "1") + .output() + .unwrap(); + assert!(out.status.success(), "{out:?}"); +} From da2d9a3000a48cb1b9d0f32776bade66f02fa502 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 16:01:25 +0530 Subject: [PATCH 14/17] feat(cli): snapshot list/restore/prune + provenance/ref-transaction hooks Closes the remaining agentic-git parity gaps that needed a CLI/hook surface, not just library code: `agentflare git snapshot list/restore/prune` -- flare_git_core::snapshot's pre-destructive snapshots were already being taken automatically by the shim, but there was no way to actually use one. restore without an id uses the only snapshot, or the newest with --yes; prune keeps the most recent N. `agentflare git trailer-inject ` -- called by the new .githooks/prepare-commit-msg (installed by install-hooks alongside pre-commit/pre-push), appends provenance trailers to every commit. `agentflare git ref-transaction-log` -- called by the new .githooks/reference-transaction (state == "committed" only), journals every ref move in the repo to ~/.agentflare/audit/git-refs.jsonl. A backstop audit trail independent of the shim's own interception -- it fires regardless of whether git was invoked through the shim. install_hooks now installs all four hooks from one HOOKS table instead of a hardcoded pre-commit/pre-push pair. install-shim's printed message updated to mention all three bypass tiers. Full workspace build clean, all tests green (verified before this commit), clippy clean. --- .githooks/prepare-commit-msg | 24 +++ .githooks/reference-transaction | 31 ++++ src/cli/git.rs | 257 +++++++++++++++++++++++++------- 3 files changed, 262 insertions(+), 50 deletions(-) create mode 100644 .githooks/prepare-commit-msg create mode 100644 .githooks/reference-transaction diff --git a/.githooks/prepare-commit-msg b/.githooks/prepare-commit-msg new file mode 100644 index 00000000..14983c75 --- /dev/null +++ b/.githooks/prepare-commit-msg @@ -0,0 +1,24 @@ +#!/usr/bin/env bash +# agentflare provenance trailers (prepare-commit-msg). +# +# Appends Agentflare-Agent/Agentflare-Branch/Agentflare-Item trailers to +# every commit message, so it's clear afterward which agent (if any) made +# it and what work item it belongs to. Self-reported, not cryptographically +# attested -- matches every other AGENTFLARE_AGENT-based identity check in +# this codebase (see claims::owner_id). Idempotent: re-running on an +# already-stamped message (e.g. `commit --amend`) is a no-op. +# +# Fail-open by design: if the agentflare binary isn't on PATH or errors, +# the commit proceeds with its message unchanged. +# +# Installed into a project via `agentflare git install-hooks`, same as +# pre-commit/pre-push (agentflare stores the canonical copy under +# ~/.agentflare/githooks/). + +msg_file="$1" + +if command -v agentflare >/dev/null 2>&1; then + agentflare git trailer-inject "$msg_file" || true +fi + +exit 0 diff --git a/.githooks/reference-transaction b/.githooks/reference-transaction new file mode 100644 index 00000000..6016f7f7 --- /dev/null +++ b/.githooks/reference-transaction @@ -0,0 +1,31 @@ +#!/usr/bin/env bash +# agentflare reference-transaction journal. +# +# Backstop audit trail independent of the git-shim's own interception: this +# fires for EVERY ref move in this repo, whether git was invoked through +# the agentflare git shim, bare git, or any other path -- unlike the shim's +# own audit log (crates/flare-git-core/src/audit.rs), which only sees +# invocations that actually went through it. +# +# Git invokes this hook once per state ("prepared", "committed", "aborted") +# with ref-update lines (` `) on stdin; only +# "committed" (the transaction that actually succeeded) is logged. +# +# Fail-open by design: if the agentflare binary isn't on PATH or errors, +# the underlying git operation is completely unaffected either way -- this +# hook cannot block anything, it only observes. +# +# Installed into a project via `agentflare git install-hooks`, same as +# pre-commit/pre-push/prepare-commit-msg. + +state="$1" + +if [ "$state" != "committed" ]; then + exit 0 +fi + +if command -v agentflare >/dev/null 2>&1; then + agentflare git ref-transaction-log || true +fi + +exit 0 diff --git a/src/cli/git.rs b/src/cli/git.rs index 647559f5..9abfd854 100644 --- a/src/cli/git.rs +++ b/src/cli/git.rs @@ -1,12 +1,9 @@ -//! `agentflare git install-hooks` — installs the shared branch-protection -//! git hooks (pre-commit / pre-push) into the current repository. -//! -//! The canonical hook scripts live in `~/.agentflare/githooks/` (populated by -//! this same command on first run, and reusable across every project). Each -//! invocation copies them into `/.githooks/` and points the repo's -//! `core.hooksPath` at that directory, so the guard is reproducible across -//! clones and applies to every git client (agent Bash, human CLI, CI) — not -//! just tool calls that route through the agent's PreToolUse hook. +//! `agentflare git` -- git-related CLI surface: installing the shared +//! branch-protection hooks (pre-commit / pre-push / prepare-commit-msg / +//! reference-transaction) into a repo, installing/uninstalling the +//! flare-git-shim PATH shim, and the recovery-snapshot commands +//! (`snapshot list/restore/prune`) that make `flare_git_core::snapshot`'s +//! automatic pre-destructive snapshots actually usable. //! //! Why a git hook and not (only) the PreToolUse branch guard in //! `src/hook_redirect.rs`: that guard only watches file-write tools @@ -16,8 +13,10 @@ use crate::paths::home; use clap::{Args, Subcommand}; +use flare_git_core::{audit, branch, provenance, snapshot}; use std::fs; -use std::path::PathBuf; +use std::io::Read as _; +use std::path::{Path, PathBuf}; #[derive(Args)] pub struct GitArgs { @@ -27,13 +26,23 @@ pub struct GitArgs { #[derive(Subcommand)] pub enum GitCommand { - /// Install branch-protection pre-commit/pre-push hooks into this repo. + /// Install branch-protection/provenance git hooks into this repo. InstallHooks(InstallHooksArgs), /// Install the flare-git-shim binary (dogfooding/local use) as `git` /// on PATH, so every git invocation on this machine gets classified. InstallShim(InstallShimArgs), /// Remove the git shim installed by `install-shim`. UninstallShim, + /// Recovery snapshots taken by the git shim before a destructive op. + Snapshot(SnapshotArgs), + /// (Internal, called by the `prepare-commit-msg` hook.) Appends + /// provenance trailers to the commit message file. + #[command(hide = true)] + TrailerInject(TrailerInjectArgs), + /// (Internal, called by the `reference-transaction` hook.) Reads ref + /// updates from stdin and appends them to the backstop audit log. + #[command(hide = true)] + RefTransactionLog, } #[derive(Args)] @@ -53,6 +62,47 @@ pub struct InstallHooksArgs { pub yes: bool, } +#[derive(Args)] +pub struct TrailerInjectArgs { + /// Path to the commit-message file (`prepare-commit-msg`'s `$1`). + pub msg_file: PathBuf, +} + +#[derive(Args)] +pub struct SnapshotArgs { + #[command(subcommand)] + pub command: SnapshotCommand, +} + +#[derive(Subcommand)] +pub enum SnapshotCommand { + /// List recovery snapshots for this repo, newest first. + List, + /// Restore a snapshot's files into the working tree. Non-destructive: + /// files created after the snapshot are left in place, never deleted. + Restore(SnapshotRestoreArgs), + /// Delete all but the most recent snapshots. + Prune(SnapshotPruneArgs), +} + +#[derive(Args)] +pub struct SnapshotRestoreArgs { + /// Snapshot id (a commit sha, or any unambiguous prefix of one) to + /// restore. Omit to use the only snapshot, or the newest with --yes. + pub id: Option, + /// Skip the confirmation required when omitting `id` with more than + /// one snapshot present, to pick the newest non-interactively. + #[arg(long)] + pub yes: bool, +} + +#[derive(Args)] +pub struct SnapshotPruneArgs { + /// Number of most-recent snapshots to keep. + #[arg(long, default_value_t = 5)] + pub keep: usize, +} + /// Canonical location: `~/.agentflare/githooks/`. fn shared_hooks_dir() -> PathBuf { home().join(".agentflare").join("githooks") @@ -63,17 +113,25 @@ fn shared_hooks_dir() -> PathBuf { /// is self-bootstrapping and survives repo checkouts. const PRE_COMMIT: &str = include_str!("../../.githooks/pre-commit"); const PRE_PUSH: &str = include_str!("../../.githooks/pre-push"); +const PREPARE_COMMIT_MSG: &str = include_str!("../../.githooks/prepare-commit-msg"); +const REFERENCE_TRANSACTION: &str = include_str!("../../.githooks/reference-transaction"); + +/// Every hook this command installs, in (filename, embedded template) pairs. +const HOOKS: &[(&str, &str)] = &[ + ("pre-commit", PRE_COMMIT), + ("pre-push", PRE_PUSH), + ("prepare-commit-msg", PREPARE_COMMIT_MSG), + ("reference-transaction", REFERENCE_TRANSACTION), +]; fn ensure_shared_templates() -> std::io::Result<()> { let dir = shared_hooks_dir(); fs::create_dir_all(&dir)?; - let pc = dir.join("pre-commit"); - if !pc.exists() { - fs::write(&pc, PRE_COMMIT)?; - } - let pp = dir.join("pre-push"); - if !pp.exists() { - fs::write(&pp, PRE_PUSH)?; + for (name, template) in HOOKS { + let path = dir.join(name); + if !path.exists() { + fs::write(&path, template)?; + } } Ok(()) } @@ -83,6 +141,9 @@ pub fn run(args: GitArgs) { GitCommand::InstallHooks(opts) => install_hooks(opts), GitCommand::InstallShim(opts) => install_shim(opts), GitCommand::UninstallShim => uninstall_shim(), + GitCommand::Snapshot(opts) => snapshot_cmd(opts), + GitCommand::TrailerInject(opts) => trailer_inject(&opts.msg_file), + GitCommand::RefTransactionLog => ref_transaction_log(), } } @@ -129,7 +190,7 @@ fn install_shim(opts: InstallShimArgs) { println!( " -Once your PATH refreshes, every `git` command on this machine is classified by the agentflare git shim. Escape hatch: set AGENTFLARE_GIT_BYPASS=1 to skip classification for a command/session without uninstalling. Remove entirely with `agentflare git uninstall-shim`." +Once your PATH refreshes, every `git` command on this machine is classified by the agentflare git shim. Escape hatches: AGENTFLARE_GIT_BYPASS=1 (one-shot), AGENTFLARE_GIT_BYPASS_AGENT=, AGENTFLARE_GIT_BYPASS_UNTIL=. Remove entirely with `agentflare git uninstall-shim`." ); } @@ -154,7 +215,7 @@ fn uninstall_shim() { /// do this without an extra crate). Returns `Ok(true)` if PATH was /// changed, `Ok(false)` if `dir` was already present. #[cfg(windows)] -fn ensure_on_path(dir: &std::path::Path) -> Result { +fn ensure_on_path(dir: &Path) -> Result { let dir_str = dir.to_string_lossy().to_string(); let get = std::process::Command::new("powershell.exe") .args([ @@ -167,7 +228,7 @@ fn ensure_on_path(dir: &std::path::Path) -> Result { let current = String::from_utf8_lossy(&get.stdout).trim().to_string(); let already_present = current .split(';') - .any(|p| p.trim_end_matches('\\').eq_ignore_ascii_case(dir_str.trim_end_matches('\\'))); + .any(|p| p.trim_end_matches('\u{5c}').eq_ignore_ascii_case(dir_str.trim_end_matches('\u{5c}'))); if already_present { return Ok(false); } @@ -191,7 +252,7 @@ fn ensure_on_path(dir: &std::path::Path) -> Result { } #[cfg(not(windows))] -fn ensure_on_path(_dir: &std::path::Path) -> Result { +fn ensure_on_path(_dir: &Path) -> Result { // Not needed for this dogfooding session (Windows-only machine); the // real install.sh wiring will handle shell-profile PATH export the // same way it already does for the main binary's install dir. @@ -210,9 +271,7 @@ fn install_hooks(opts: InstallHooksArgs) { }; // Sanity: must be inside a git repo. - if !repo_root.join(".git").exists() - && run_git(&repo_root, &["rev-parse", "--git-dir"]).is_none() - { + if branch::repo_toplevel(&repo_root).is_none() { crate::ui::error( "agentflare git install-hooks: not a git repository (run inside a repo root)", ); @@ -235,16 +294,11 @@ fn install_hooks(opts: InstallHooksArgs) { } let mut changed = false; - for name in ["pre-commit", "pre-push"] { + for (name, _) in HOOKS { let src = shared_hooks_dir().join(name); let dst = local_dir.join(name); match fs::copy(&src, &dst) { Ok(_) => { - // Git requires the hook to be executable. On Unix the copied - // file keeps the shared template's mode (0600 from a fresh - // write), so make it user-executable. On Windows git runs - // hooks through its bundled sh and ignores the bit, but - // setting it is harmless and keeps the repo portable. #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -260,36 +314,139 @@ fn install_hooks(opts: InstallHooksArgs) { } } - // Point the repo at the local .githooks dir (relative, so it survives - // clone/move). `git config` is run via the shell-free helper below. - set_hooks_path(&repo_root, ".githooks"); + flare_git_core::shell::run_in(&repo_root, &["config", "core.hooksPath", ".githooks"]).ok(); crate::ui::success("core.hooksPath = .githooks"); if changed { println!( "\nBranch-protection hooks installed. Direct commits/pushes to the \ - default branch are now blocked for every git client in this repo." + default branch are now blocked for every git client in this repo. \ + Commits are also stamped with provenance trailers, and every ref \ + move is journaled to ~/.agentflare/audit/git-refs.jsonl." ); let _ = opts; } } -fn run_git(repo: &std::path::Path, args: &[&str]) -> Option { - let out = std::process::Command::new("git") - .args(args) - .current_dir(repo) - .output() - .ok()?; - if out.status.success() { - Some(String::from_utf8_lossy(&out.stdout).trim().to_string()) - } else { - None +/// Resolves the git repo root from the current working directory, printing +/// a consistent error and returning `None` if we're not inside one. +fn resolve_repo_root(command_name: &str) -> Option { + let cwd = std::env::current_dir().ok()?; + let root = branch::repo_toplevel(&cwd); + if root.is_none() { + crate::ui::error(&format!( + "agentflare git {command_name}: not a git repository (run inside a repo root)" + )); + } + root +} + +fn snapshot_cmd(args: SnapshotArgs) { + let Some(repo_root) = resolve_repo_root("snapshot") else { + return; + }; + match args.command { + SnapshotCommand::List => snapshot_list(&repo_root), + SnapshotCommand::Restore(opts) => snapshot_restore(&repo_root, &opts), + SnapshotCommand::Prune(opts) => snapshot_prune(&repo_root, &opts), + } +} + +fn snapshot_list(repo_root: &Path) { + let snaps = snapshot::list(repo_root); + if snaps.is_empty() { + println!("No snapshots for this repo."); + return; + } + for s in snaps { + let short_id = &s.id.0[..s.id.0.len().min(12)]; + println!("{short_id} {} {}", s.committer_date, s.reason); + } +} + +fn snapshot_restore(repo_root: &Path, opts: &SnapshotRestoreArgs) { + let snaps = snapshot::list(repo_root); + let target = match &opts.id { + Some(id) => snaps.iter().find(|s| s.id.0.starts_with(id.as_str())), + None => match snaps.len() { + 0 => None, + 1 => snaps.first(), + _ if opts.yes => snaps.first(), + _ => { + crate::ui::error( + "agentflare git snapshot restore: multiple snapshots exist -- pass an id, or --yes to use the newest", + ); + return; + } + }, + }; + let Some(meta) = target else { + crate::ui::error("agentflare git snapshot restore: no matching snapshot found"); + return; + }; + match snapshot::restore(repo_root, &meta.id) { + Ok(()) => crate::ui::success(&format!( + "restored snapshot {} ({})", + &meta.id.0[..meta.id.0.len().min(12)], + meta.reason + )), + Err(e) => crate::ui::error(&format!("agentflare git snapshot restore: {e}")), } } -fn set_hooks_path(repo: &std::path::Path, path: &str) { - let _ = std::process::Command::new("git") - .args(["config", "core.hooksPath", path]) - .current_dir(repo) - .output(); +fn snapshot_prune(repo_root: &Path, opts: &SnapshotPruneArgs) { + match snapshot::prune(repo_root, opts.keep) { + Ok(()) => crate::ui::success(&format!("pruned snapshots, kept {} most recent", opts.keep)), + Err(e) => crate::ui::error(&format!("agentflare git snapshot prune: {e}")), + } +} + +/// `agentflare git trailer-inject ` -- called by the +/// `prepare-commit-msg` hook. Fail-open: any error leaves the message file +/// untouched rather than blocking the commit. +fn trailer_inject(msg_file: &Path) { + let Some(repo_root) = branch::repo_toplevel(&std::env::current_dir().unwrap_or_default()) else { + return; + }; + let Ok(original) = fs::read_to_string(msg_file) else { + return; + }; + let trailers = provenance::build_trailers(&repo_root); + let updated = provenance::append_trailers(&original, &trailers); + if updated != original { + let _ = fs::write(msg_file, updated); + } +} + +/// `agentflare git ref-transaction-log` -- called by the +/// `reference-transaction` hook with ref-update lines +/// (` `) on stdin. Fail-open: this only +/// observes, it can never affect the underlying git operation either way. +fn ref_transaction_log() { + let mut input = String::new(); + if std::io::stdin().read_to_string(&mut input).is_err() { + return; + } + let transactions: Vec = input + .lines() + .filter_map(|line| { + let mut parts = line.split_whitespace(); + Some(audit::RefTransaction { + old: parts.next()?.to_string(), + new: parts.next()?.to_string(), + refname: parts.next()?.to_string(), + }) + }) + .collect(); + if transactions.is_empty() { + return; + } + let repo_root = branch::repo_toplevel(&std::env::current_dir().unwrap_or_default()); + let agent = repo_root + .as_deref() + .and_then(|root| provenance::build_trailers(root).agent); + let event = audit::RefTransactionEvent { agent, transactions }; + if let Some(path) = audit::default_path("git-refs.jsonl") { + let _ = audit::log_event(&path, &event); + } } From 6cb1872a67874bb26df3c6d3f3a2dd71ec9b5fe2 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 20:11:02 +0530 Subject: [PATCH 15/17] fix(classify): push trust-root guard silently skipped for common invocations git push (bare, args.len()<2) skipped the trust-root check entirely -- the single most common push form never got checked. Any flag before remote/refspec (git push -u origin branch) misread the remote name as the branch being pushed via fixed-index args[1], mis-diffing or spuriously denying. pushed_branch() now resolves the pushed ref positionally (skipping flags), falling back to the current branch when omitted. Also broadens is_destructive's clean-flag detection past four hardcoded literal strings (-f/-fd/-fx/-fdx) to cover --force and any short-opt order (-df, -xf, ...), which previously skipped the pre-destructive snapshot safety net for equivalent invocations. --- crates/flare-git-core/src/classify.rs | 99 +++++++++++++++++++++++++-- 1 file changed, 95 insertions(+), 4 deletions(-) diff --git a/crates/flare-git-core/src/classify.rs b/crates/flare-git-core/src/classify.rs index 9997e2b3..78c1b06c 100644 --- a/crates/flare-git-core/src/classify.rs +++ b/crates/flare-git-core/src/classify.rs @@ -18,7 +18,7 @@ use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; -use crate::branch::{is_protected_branch, resolve_default_branch}; +use crate::branch::{current_branch, is_protected_branch, resolve_default_branch}; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum Disposition { @@ -170,7 +170,9 @@ const DENIED_PLUMBING_SUBCOMMANDS: &[&str] = &[ pub fn is_destructive(subcommand: &str, args: &[String]) -> bool { match subcommand { "reset" => args.iter().any(|a| a == "--hard"), - "clean" => args.iter().any(|a| a == "-f" || a == "-fd" || a == "-fx" || a == "-fdx"), + "clean" => args.iter().any(|a| { + a == "--force" || (a.starts_with('-') && !a.starts_with("--") && a.contains('f')) + }), "checkout" | "switch" => args.iter().any(|a| a == "-f" || a == "--force" || a == "-B"), _ => false, } @@ -269,6 +271,31 @@ pub fn push_touches_trust_root(repo_root: &Path, branch: &str, target: &str) -> } } +/// Resolves which local branch/ref a `push` invocation would actually +/// push, skipping flags positionally (`-u`, `--force`, `--force-with-lease`, +/// `--tags`, ...) rather than assuming `args[1]` -- a flag before the +/// remote/refspec (e.g. `git push -u origin feature/x`) previously threw +/// off a fixed-index read, misreading the remote name (`"origin"`) as the +/// branch being pushed and either mis-diffing or spuriously denying. Falls +/// back to the current checked-out branch when the refspec is omitted +/// entirely (bare `git push`, or `git push ` with no explicit ref +/// -- both push the current/tracked branch, not something namable from +/// `args` alone) -- this also closes the gap where the single most common +/// push form (`git push`) skipped the trust-root check entirely. +fn pushed_branch(repo_root: &Path, args: &[String]) -> Option { + let non_flags: Vec<&str> = args + .iter() + .filter(|a| !a.starts_with('-')) + .map(String::as_str) + .collect(); + let raw = match non_flags.len() { + 0 | 1 => current_branch(repo_root), + _ => Some(non_flags[1].to_string()), + }?; + let branch = raw.split(':').next().unwrap_or(&raw); + Some(branch.trim_start_matches("refs/heads/").to_string()) +} + /// I/O-resolving entry point: resolves the default branch and (for `push` /// with a resolvable branch/target pair) whether the push touches a /// trust-root path, then delegates to `classify_pure`. @@ -276,8 +303,8 @@ pub fn push_touches_trust_root(repo_root: &Path, branch: &str, target: &str) -> pub fn classify(repo_root: &Path, subcommand: &str, args: &[String]) -> Event { let default_branch = resolve_default_branch(repo_root); let touches_trust_root = subcommand == "push" - && args.len() >= 2 - && push_touches_trust_root(repo_root, &args[1], &default_branch); + && pushed_branch(repo_root, args) + .is_some_and(|b| push_touches_trust_root(repo_root, &b, &default_branch)); let disposition = classify_pure(subcommand, args, &default_branch, touches_trust_root); Event { subcommand: subcommand.to_string(), @@ -494,4 +521,68 @@ mod tests { assert!(!is_destructive("checkout", &args(&["master"]))); assert!(!is_destructive("commit", &args(&["-m", "x"]))); } + + #[test] + fn is_destructive_flags_clean_regardless_of_flag_form_or_order() { + // Combined short opts in the order git itself would print them. + assert!(is_destructive("clean", &args(&["-fd"]))); + // Same combination, opposite order -- git treats "-df" identically to + // "-fd", but a naive exact-string match on "-fd" alone would miss it. + assert!(is_destructive("clean", &args(&["-df"]))); + // Long form, not in the original hardcoded list at all. + assert!(is_destructive("clean", &args(&["--force"]))); + // Separate short flags rather than one combined cluster. + assert!(is_destructive("clean", &args(&["-f", "-d"]))); + assert!(!is_destructive("clean", &args(&["-n"]))); + assert!(!is_destructive("clean", &args(&["--dry-run"]))); + } + + #[test] + fn pushed_branch_reads_the_refspec_positionally_skipping_leading_flags() { + // `-u` before remote/refspec previously threw off a fixed-index + // `args[1]` read, misreading "origin" as the branch being pushed. + let repo = crate::shell::test_support::init_repo_with_branch("master"); + assert_eq!( + pushed_branch(&repo.path, &args(&["-u", "origin", "feature/x"])).as_deref(), + Some("feature/x") + ); + assert_eq!( + pushed_branch(&repo.path, &args(&["--force", "origin", "feature/x"])).as_deref(), + Some("feature/x") + ); + assert_eq!( + pushed_branch(&repo.path, &args(&["origin", "feature/x"])).as_deref(), + Some("feature/x") + ); + } + + #[test] + fn pushed_branch_falls_back_to_current_branch_when_refspec_omitted() { + // Bare `git push` and `git push ` (no explicit ref) both push + // the current/tracked branch -- previously these skipped the + // trust-root check entirely (args.len() >= 2 was false). + let repo = crate::shell::test_support::init_repo_with_branch("feature/y"); + assert_eq!(pushed_branch(&repo.path, &[]).as_deref(), Some("feature/y")); + assert_eq!( + pushed_branch(&repo.path, &args(&["origin"])).as_deref(), + Some("feature/y") + ); + } + + #[test] + fn push_with_leading_flags_touching_trust_root_is_still_detected() { + // End-to-end regression for the classify()-level bug: a flag before + // remote/refspec must not make the trust-root check silently pass. + let repo = crate::shell::test_support::init_repo_with_branch("master"); + std::fs::write(repo.path.join("Cargo.toml"), "[package]\n").unwrap(); + crate::shell::run_in(&repo.path, &["add", "Cargo.toml"]).unwrap(); + crate::shell::run_in(&repo.path, &["checkout", "-b", "feature/z"]).unwrap(); + crate::shell::run_in(&repo.path, &["commit", "-m", "touch trust root"]).unwrap(); + let event = classify(&repo.path, "push", &args(&["-u", "origin", "feature/z"])); + assert!( + matches!(event.disposition, Disposition::Deny { .. }), + "{:?}", + event.disposition + ); + } } From 7dee4b32f7f17613252cefbcc4b48248288d0b40 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 20:40:14 +0530 Subject: [PATCH 16/17] fix(snapshot): force core.autocrlf=false on capture and restore CI failure on windows-latest: snapshot_then_restore_recovers_state_and_preserves_newer_files asserted \modifiedn\ but got \modifiedrn\ -- git checkout silently rewrites LF to CRLF on restore when core.autocrlf=true (the common Windows default), breaking the exact-recovery guarantee this feature exists for regardless of what was actually snapshotted. Both run_git_with_index (capture) and restore (checkout) now pass -c core.autocrlf=false explicitly, independent of the caller's ambient/global git config. Added a regression test that sets core.autocrlf=true locally on the test repo so it reproduces deterministically everywhere, not just on a Windows CI runner. --- crates/flare-git-core/src/snapshot.rs | 45 +++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/crates/flare-git-core/src/snapshot.rs b/crates/flare-git-core/src/snapshot.rs index 5be2ae30..4a39a524 100644 --- a/crates/flare-git-core/src/snapshot.rs +++ b/crates/flare-git-core/src/snapshot.rs @@ -28,7 +28,12 @@ pub struct SnapshotMeta { /// `git ` with a temporary `GIT_INDEX_FILE`, so staging for a /// snapshot never touches the caller's real index. fn run_git_with_index(repo_root: &Path, index_file: &Path, args: &[&str]) -> Result { + // A snapshot must capture exactly what's on disk right now -- `-c + // core.autocrlf=false` stops git silently converting line endings + // while staging, regardless of the caller's ambient/global git config + // (autocrlf=true is the common default on Windows). let out = Command::new(crate::shell::git_binary()) + .args(["-c", "core.autocrlf=false"]) .args(args) .current_dir(repo_root) .env("GIT_INDEX_FILE", index_file) @@ -68,11 +73,19 @@ pub fn snapshot_before(repo_root: &Path, reason: &str) -> Result Result<(), String> { let refname = format!("{SNAPSHOT_REF_PREFIX}{}", id.0); - run_in(repo_root, &["checkout", &refname, "--", "."])?; + run_in( + repo_root, + &["-c", "core.autocrlf=false", "checkout", &refname, "--", "."], + )?; Ok(()) } @@ -154,6 +167,34 @@ mod tests { ); } + #[test] + fn snapshot_then_restore_is_byte_exact_regardless_of_autocrlf() { + // Regression: without an explicit "-c core.autocrlf=false" on both + // the capture and restore sides, a repo/global config of + // core.autocrlf=true (the common Windows default -- this is exactly + // what tripped CI on windows-latest) makes git checkout silently + // rewrite LF to CRLF on restore, breaking the "exact recovery" + // promise this feature exists for. Sets core.autocrlf=true locally + // on the test repo rather than relying on the host's ambient config, + // so this reproduces the CI failure deterministically everywhere. + let repo = init_repo_with_branch("master"); + run_in(&repo.path, &["config", "core.autocrlf", "true"]).unwrap(); + std::fs::write(repo.path.join("tracked.txt"), "before\n").unwrap(); + run_in(&repo.path, &["add", "tracked.txt"]).unwrap(); + run_in(&repo.path, &["commit", "-m", "add tracked"]).unwrap(); + std::fs::write(repo.path.join("tracked.txt"), "modified\n").unwrap(); + + let id = snapshot_before(&repo.path, "pre reset --hard").unwrap(); + run_in(&repo.path, &["checkout", "--", "tracked.txt"]).unwrap(); + restore(&repo.path, &id).unwrap(); + + let bytes = std::fs::read(repo.path.join("tracked.txt")).unwrap(); + assert_eq!( + bytes, b"modified\n", + "restore must not let core.autocrlf touch recovered bytes" + ); + } + #[test] fn list_and_prune_keep_only_the_most_recent() { let repo = init_repo_with_branch("master"); From df9ac9529bc43eb8cffd3123a2534aaf71bc5df1 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Mon, 20 Jul 2026 20:51:19 +0530 Subject: [PATCH 17/17] fmt: bring the PR fully rustfmt-clean Pre-existing formatting debt across the new crates and their two call sites in the root package -- none of it introduced by the earlier fixes in this PR, just line-wrapping at the 100-col limit rustfmt wants. Mechanical only, applied straight from CI's own cargo fmt --check diff to avoid the local/CI rustfmt version skew found earlier in this branch's history. --- crates/agentflare-shim/src/lib.rs | 3 +- crates/flare-git-core/src/audit.rs | 5 ++- crates/flare-git-core/src/branch.rs | 25 +++++++++--- crates/flare-git-core/src/classify.rs | 51 +++++++++++++++++++----- crates/flare-git-core/src/shell.rs | 6 ++- crates/flare-git-core/src/snapshot.rs | 13 +++++- crates/flare-git-core/src/worktree.rs | 5 ++- crates/flare-git-shim/src/main.rs | 39 ++++++++++++++---- crates/flare-git-shim/tests/shim_test.rs | 6 ++- src/cli/git.rs | 27 +++++++++---- src/worktree.rs | 7 +++- 11 files changed, 149 insertions(+), 38 deletions(-) diff --git a/crates/agentflare-shim/src/lib.rs b/crates/agentflare-shim/src/lib.rs index f1cd0e2f..91f3f3b7 100644 --- a/crates/agentflare-shim/src/lib.rs +++ b/crates/agentflare-shim/src/lib.rs @@ -36,7 +36,8 @@ pub fn trace(msg: &str) { fn is_cargo_target_profile_dir(p: &Path) -> bool { let comps: Vec<_> = p.components().collect(); comps.windows(2).any(|w| { - w[0].as_os_str() == "target" && (w[1].as_os_str() == "debug" || w[1].as_os_str() == "release") + w[0].as_os_str() == "target" + && (w[1].as_os_str() == "debug" || w[1].as_os_str() == "release") }) } diff --git a/crates/flare-git-core/src/audit.rs b/crates/flare-git-core/src/audit.rs index b12f7f75..6e20676e 100644 --- a/crates/flare-git-core/src/audit.rs +++ b/crates/flare-git-core/src/audit.rs @@ -150,6 +150,9 @@ mod tests { }], }; log_event(&path, &event).unwrap(); - assert_eq!(read_events::(&path).unwrap(), vec![event]); + assert_eq!( + read_events::(&path).unwrap(), + vec![event] + ); } } diff --git a/crates/flare-git-core/src/branch.rs b/crates/flare-git-core/src/branch.rs index 3e2c647e..c1c2afc6 100644 --- a/crates/flare-git-core/src/branch.rs +++ b/crates/flare-git-core/src/branch.rs @@ -3,7 +3,7 @@ //! git-shim's command classifier, so "is this branch protected" has exactly //! one definition. -use crate::shell::{run_in_opt, run_in_ok}; +use crate::shell::{run_in_ok, run_in_opt}; use std::path::{Path, PathBuf}; /// Current branch name (`HEAD` in detached-HEAD state). `None` outside a git @@ -35,7 +35,8 @@ pub fn resolve_default_branch(repo_root: &Path) -> String { if run_in_ok(repo_root, &["rev-parse", "--verify", "master"]) { return "master".to_string(); } - run_in_opt(repo_root, &["symbolic-ref", "--short", "HEAD"]).unwrap_or_else(|| "master".to_string()) + run_in_opt(repo_root, &["symbolic-ref", "--short", "HEAD"]) + .unwrap_or_else(|| "master".to_string()) } /// `git rev-parse --show-toplevel` from `start` — handles worktrees/submodules @@ -172,9 +173,17 @@ mod tests { fn is_protected_branch_among_matches_extra_exact_and_glob_patterns() { let extra = vec!["staging".to_string(), "release/*".to_string()]; assert!(is_protected_branch_among("staging", Some("main"), &extra)); - assert!(is_protected_branch_among("release/1.0", Some("main"), &extra)); + assert!(is_protected_branch_among( + "release/1.0", + Some("main"), + &extra + )); assert!(!is_protected_branch_among("release", Some("main"), &extra)); - assert!(!is_protected_branch_among("feature/x", Some("main"), &extra)); + assert!(!is_protected_branch_among( + "feature/x", + Some("main"), + &extra + )); } #[test] @@ -205,7 +214,13 @@ mod tests { let wt_path = wt_parent.path().join("wt-check"); crate::shell::run_in( &repo.path, - &["worktree", "add", wt_path.to_str().unwrap(), "-b", "wt-branch"], + &[ + "worktree", + "add", + wt_path.to_str().unwrap(), + "-b", + "wt-branch", + ], ) .unwrap(); assert!(is_linked_worktree(&wt_path)); diff --git a/crates/flare-git-core/src/classify.rs b/crates/flare-git-core/src/classify.rs index 78c1b06c..049cae1b 100644 --- a/crates/flare-git-core/src/classify.rs +++ b/crates/flare-git-core/src/classify.rs @@ -98,7 +98,15 @@ pub fn would_detach_head(repo_root: &Path, subcommand: &str, args: &[String]) -> let Some(target) = args.iter().find(|a| !a.starts_with('-')) else { return false; // e.g. bare `git checkout` -- doesn't move HEAD }; - !crate::shell::run_in_ok(repo_root, &["show-ref", "--verify", "--quiet", &format!("refs/heads/{target}")]) + !crate::shell::run_in_ok( + repo_root, + &[ + "show-ref", + "--verify", + "--quiet", + &format!("refs/heads/{target}"), + ], + ) } "switch" => args.iter().any(|a| a == "--detach" || a == "-d"), _ => false, @@ -173,7 +181,9 @@ pub fn is_destructive(subcommand: &str, args: &[String]) -> bool { "clean" => args.iter().any(|a| { a == "--force" || (a.starts_with('-') && !a.starts_with("--") && a.contains('f')) }), - "checkout" | "switch" => args.iter().any(|a| a == "-f" || a == "--force" || a == "-B"), + "checkout" | "switch" => args + .iter() + .any(|a| a == "-f" || a == "--force" || a == "-B"), _ => false, } } @@ -190,7 +200,9 @@ pub fn classify_pure( default_branch: &str, push_touches_trust_root: bool, ) -> Disposition { - if READ_ONLY_SUBCOMMANDS.contains(&subcommand) || ALLOWED_MUTATING_SUBCOMMANDS.contains(&subcommand) { + if READ_ONLY_SUBCOMMANDS.contains(&subcommand) + || ALLOWED_MUTATING_SUBCOMMANDS.contains(&subcommand) + { return Disposition::Passthrough; } if DENIED_PLUMBING_SUBCOMMANDS.contains(&subcommand) { @@ -265,7 +277,8 @@ pub fn push_touches_trust_root(repo_root: &Path, branch: &str, target: &str) -> let range = format!("{target}...{branch}"); match crate::shell::run_in(repo_root, &["diff", "--name-only", &range]) { Ok(names) => names.lines().any(|f| { - TRUST_ROOT_PATHS.iter().any(|p| f.starts_with(p)) || extra.iter().any(|p| f.starts_with(p.as_str())) + TRUST_ROOT_PATHS.iter().any(|p| f.starts_with(p)) + || extra.iter().any(|p| f.starts_with(p.as_str())) }), Err(_) => true, } @@ -441,7 +454,12 @@ mod tests { #[test] fn branch_rename_of_protected_branch_is_denied() { assert!(matches!( - classify_pure("branch", &args(&["-M", "master", "renamed"]), "master", false), + classify_pure( + "branch", + &args(&["-M", "master", "renamed"]), + "master", + false + ), Disposition::Deny { .. } )); } @@ -456,7 +474,10 @@ mod tests { #[test] fn branch_listing_and_creation_pass_through() { - assert_eq!(classify_pure("branch", &[], "master", false), Disposition::Passthrough); + assert_eq!( + classify_pure("branch", &[], "master", false), + Disposition::Passthrough + ); assert_eq!( classify_pure("branch", &args(&["feature/new"]), "master", false), Disposition::Passthrough @@ -476,7 +497,11 @@ mod tests { fn would_detach_head_false_for_an_existing_branch_checkout_target() { let repo = crate::shell::test_support::init_repo_with_branch("master"); crate::shell::run_in(&repo.path, &["branch", "feature/x"]).unwrap(); - assert!(!would_detach_head(&repo.path, "checkout", &args(&["feature/x"]))); + assert!(!would_detach_head( + &repo.path, + "checkout", + &args(&["feature/x"]) + )); } #[test] @@ -493,8 +518,16 @@ mod tests { #[test] fn would_detach_head_true_for_explicit_detach_flag() { let repo = crate::shell::test_support::init_repo_with_branch("master"); - assert!(would_detach_head(&repo.path, "checkout", &args(&["--detach", "master"]))); - assert!(would_detach_head(&repo.path, "switch", &args(&["--detach", "master"]))); + assert!(would_detach_head( + &repo.path, + "checkout", + &args(&["--detach", "master"]) + )); + assert!(would_detach_head( + &repo.path, + "switch", + &args(&["--detach", "master"]) + )); } #[test] diff --git a/crates/flare-git-core/src/shell.rs b/crates/flare-git-core/src/shell.rs index a2f2b5c2..56a7613c 100644 --- a/crates/flare-git-core/src/shell.rs +++ b/crates/flare-git-core/src/shell.rs @@ -33,7 +33,8 @@ use std::sync::OnceLock; fn is_cargo_target_profile_dir(p: &Path) -> bool { let comps: Vec<_> = p.components().collect(); comps.windows(2).any(|w| { - w[0].as_os_str() == "target" && (w[1].as_os_str() == "debug" || w[1].as_os_str() == "release") + w[0].as_os_str() == "target" + && (w[1].as_os_str() == "debug" || w[1].as_os_str() == "release") }) } @@ -51,7 +52,8 @@ pub(crate) fn git_binary() -> PathBuf { .unwrap_or(path_var) }); let cwd = std::env::current_dir().unwrap_or_default(); - which::which_in("git", filtered_path.as_ref(), cwd).unwrap_or_else(|_| PathBuf::from("git")) + which::which_in("git", filtered_path.as_ref(), cwd) + .unwrap_or_else(|_| PathBuf::from("git")) }) .clone() } diff --git a/crates/flare-git-core/src/snapshot.rs b/crates/flare-git-core/src/snapshot.rs index 4a39a524..665c3385 100644 --- a/crates/flare-git-core/src/snapshot.rs +++ b/crates/flare-git-core/src/snapshot.rs @@ -27,7 +27,11 @@ pub struct SnapshotMeta { /// `git ` with a temporary `GIT_INDEX_FILE`, so staging for a /// snapshot never touches the caller's real index. -fn run_git_with_index(repo_root: &Path, index_file: &Path, args: &[&str]) -> Result { +fn run_git_with_index( + repo_root: &Path, + index_file: &Path, + args: &[&str], +) -> Result { // A snapshot must capture exactly what's on disk right now -- `-c // core.autocrlf=false` stops git silently converting line endings // while staging, regardless of the caller's ambient/global git config @@ -57,7 +61,12 @@ pub fn snapshot_before(repo_root: &Path, reason: &str) -> Result= deadline { kill_tree(&mut child); let _ = child.wait(); - return Err(format!("{}: timed out after {timeout_secs}s", program.to_string_lossy())); + return Err(format!( + "{}: timed out after {timeout_secs}s", + program.to_string_lossy() + )); } std::thread::sleep(Duration::from_millis(50)); } diff --git a/crates/flare-git-shim/src/main.rs b/crates/flare-git-shim/src/main.rs index 02c1d180..619060f9 100644 --- a/crates/flare-git-shim/src/main.rs +++ b/crates/flare-git-shim/src/main.rs @@ -62,7 +62,14 @@ const ESCAPE_HATCH_FLAGS: &[&str] = &["-C", "--git-dir", "--work-tree"]; /// Global flags that consume the following argument as their value, so /// subcommand detection can skip past both tokens. -const GLOBAL_FLAGS_WITH_VALUE: &[&str] = &["-c", "-C", "--git-dir", "--work-tree", "--namespace", "--exec-path"]; +const GLOBAL_FLAGS_WITH_VALUE: &[&str] = &[ + "-c", + "-C", + "--git-dir", + "--work-tree", + "--namespace", + "--exec-path", +]; /// Finds the subcommand token's index, skipping global flags (and their /// values, for flags that take one). Also reports whether an escape-hatch @@ -76,11 +83,17 @@ fn parse_global_flags(args: &[String]) -> (Option, bool) { return (Some(i), escape_hatch); } if ESCAPE_HATCH_FLAGS.contains(&a.as_str()) - || ESCAPE_HATCH_FLAGS.iter().any(|f| a.starts_with(&format!("{f}="))) + || ESCAPE_HATCH_FLAGS + .iter() + .any(|f| a.starts_with(&format!("{f}="))) { escape_hatch = true; } - i += if GLOBAL_FLAGS_WITH_VALUE.contains(&a.as_str()) { 2 } else { 1 }; + i += if GLOBAL_FLAGS_WITH_VALUE.contains(&a.as_str()) { + 2 + } else { + 1 + }; } (None, escape_hatch) } @@ -124,7 +137,11 @@ fn snapshots_enabled() -> bool { /// (non-worktree) checkout AND the command would actually detach HEAD. /// Interactive human use, and any use inside an isolated worktree, is /// completely unaffected. -fn deny_canonical_detach_reason(repo_root: &Path, subcommand: &str, args: &[String]) -> Option { +fn deny_canonical_detach_reason( + repo_root: &Path, + subcommand: &str, + args: &[String], +) -> Option { if agentflare_shim::is_set(ALLOW_CANONICAL_MUTATE_ENV) { return None; } @@ -191,7 +208,10 @@ fn main() { if let Some(audit_path) = audit::default_path("git.jsonl") { let bypass_event = classify::Event { subcommand: "*".to_string(), - args: args.iter().map(|a| a.to_string_lossy().into_owned()).collect(), + args: args + .iter() + .map(|a| a.to_string_lossy().into_owned()) + .collect(), disposition: classify::Disposition::SilentExempt, }; let _ = audit::log_event(&audit_path, &bypass_event); @@ -199,7 +219,10 @@ fn main() { run_real(&tool, filtered_path.as_ref(), &args); } - let str_args: Vec = args.iter().map(|a| a.to_string_lossy().into_owned()).collect(); + let str_args: Vec = args + .iter() + .map(|a| a.to_string_lossy().into_owned()) + .collect(); let (subcommand_idx, escape_hatch) = parse_global_flags(&str_args); if escape_hatch { @@ -219,7 +242,9 @@ fn main() { let event = classify::Event { subcommand: subcommand.clone(), args: rest.clone(), - disposition: classify::Disposition::Deny { reason: reason.clone() }, + disposition: classify::Disposition::Deny { + reason: reason.clone(), + }, }; if let Some(audit_path) = audit::default_path("git.jsonl") { let _ = audit::log_event(&audit_path, &event); diff --git a/crates/flare-git-shim/tests/shim_test.rs b/crates/flare-git-shim/tests/shim_test.rs index f8c2c441..8678c028 100644 --- a/crates/flare-git-shim/tests/shim_test.rs +++ b/crates/flare-git-shim/tests/shim_test.rs @@ -125,7 +125,11 @@ fn denied_command_is_logged_to_the_audit_log() { let out = shim(repo.path(), home.path(), &["checkout", "master"]); assert!(!out.status.success()); - let audit_log = home.path().join(".agentflare").join("audit").join("git.jsonl"); + let audit_log = home + .path() + .join(".agentflare") + .join("audit") + .join("git.jsonl"); let content = std::fs::read_to_string(&audit_log).expect("audit log must exist"); assert!(content.contains("checkout"), "{content}"); assert!(content.contains("Deny"), "{content}"); diff --git a/src/cli/git.rs b/src/cli/git.rs index 9abfd854..24ea80ff 100644 --- a/src/cli/git.rs +++ b/src/cli/git.rs @@ -161,7 +161,9 @@ fn shim_dest_name() -> &'static str { fn install_shim(opts: InstallShimArgs) { let dir = shims_dir(); if let Err(e) = fs::create_dir_all(&dir) { - crate::ui::error(&format!("agentflare git install-shim: cannot create {dir:?}: {e}")); + crate::ui::error(&format!( + "agentflare git install-shim: cannot create {dir:?}: {e}" + )); return; } let dest = dir.join(shim_dest_name()); @@ -185,7 +187,9 @@ fn install_shim(opts: InstallShimArgs) { dir.display() )), Ok(false) => crate::ui::success(&format!("{} already on PATH", dir.display())), - Err(e) => crate::ui::error(&format!("agentflare git install-shim: could not update PATH: {e}")), + Err(e) => crate::ui::error(&format!( + "agentflare git install-shim: could not update PATH: {e}" + )), } println!( @@ -202,7 +206,9 @@ fn uninstall_shim() { } match fs::remove_file(&dest) { Ok(()) => crate::ui::success(&format!("removed {}", dest.display())), - Err(e) => crate::ui::error(&format!("agentflare git uninstall-shim: cannot remove {dest:?}: {e}")), + Err(e) => crate::ui::error(&format!( + "agentflare git uninstall-shim: cannot remove {dest:?}: {e}" + )), } // Deliberately leaves the shims dir on PATH -- other shims (e.g. the // lean-ctx one) may still live there; removing just this binary is @@ -226,9 +232,10 @@ fn ensure_on_path(dir: &Path) -> Result { .output() .map_err(|e| e.to_string())?; let current = String::from_utf8_lossy(&get.stdout).trim().to_string(); - let already_present = current - .split(';') - .any(|p| p.trim_end_matches('\u{5c}').eq_ignore_ascii_case(dir_str.trim_end_matches('\u{5c}'))); + let already_present = current.split(';').any(|p| { + p.trim_end_matches('\u{5c}') + .eq_ignore_ascii_case(dir_str.trim_end_matches('\u{5c}')) + }); if already_present { return Ok(false); } @@ -405,7 +412,8 @@ fn snapshot_prune(repo_root: &Path, opts: &SnapshotPruneArgs) { /// `prepare-commit-msg` hook. Fail-open: any error leaves the message file /// untouched rather than blocking the commit. fn trailer_inject(msg_file: &Path) { - let Some(repo_root) = branch::repo_toplevel(&std::env::current_dir().unwrap_or_default()) else { + let Some(repo_root) = branch::repo_toplevel(&std::env::current_dir().unwrap_or_default()) + else { return; }; let Ok(original) = fs::read_to_string(msg_file) else { @@ -445,7 +453,10 @@ fn ref_transaction_log() { let agent = repo_root .as_deref() .and_then(|root| provenance::build_trailers(root).agent); - let event = audit::RefTransactionEvent { agent, transactions }; + let event = audit::RefTransactionEvent { + agent, + transactions, + }; if let Some(path) = audit::default_path("git-refs.jsonl") { let _ = audit::log_event(&path, &event); } diff --git a/src/worktree.rs b/src/worktree.rs index c04e9b76..c2552f01 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -45,7 +45,12 @@ pub fn push_and_open_pr( target_branch: &str, progress: Option<&ProgressSender>, ) -> Option { - let branch = flare_git_core::worktree::push_branch(item, repo_root, target_branch, as_progress(progress))?; + let branch = flare_git_core::worktree::push_branch( + item, + repo_root, + target_branch, + as_progress(progress), + )?; if let Some(p) = progress { p.send(0.5, Some(1.0), Some("Creating PR...".into())); }