diff --git a/Cargo.lock b/Cargo.lock index de46ea75..0c48dcb4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1111,6 +1111,7 @@ version = "0.1.0" dependencies = [ "agent-detector", "agentflare-backend", + "agentflare-shim", "chrono", "dirs", "fs2", @@ -1127,6 +1128,7 @@ name = "flare-git-shim" version = "0.1.0" dependencies = [ "agentflare-shim", + "dirs", "flare-git-core", "serde", "serde_json", diff --git a/crates/agentflare-shim/src/lib.rs b/crates/agentflare-shim/src/lib.rs index 5eed1912..bf79718f 100644 --- a/crates/agentflare-shim/src/lib.rs +++ b/crates/agentflare-shim/src/lib.rs @@ -15,6 +15,35 @@ pub fn is_set(name: &str) -> bool { env::var_os(name).is_some_and(|v| !v.is_empty()) } +/// Project marker directory: its presence means agentflare actually tracks +/// this project. Shared by every shim binary that needs to scope its +/// behavior to agentflare-managed projects only. +pub const PROJECT_MARKER: &str = ".agentflare"; + +/// 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. Uses `paths_eq` rather than plain `==` +/// for the boundary check: a byte-equal comparison can miss the real match +/// when the ambient home dir and the walked-up ancestor differ only by case +/// or separator style (observed live: a real ambient `dirs::home_dir()` on a +/// Windows CI runner didn't byte-match the walk's own ancestor path, so the +/// boundary silently never triggered and the walk kept climbing). +#[must_use] +pub 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| paths_eq(h, d)) { + return false; + } + if d.join(PROJECT_MARKER).exists() { + return true; + } + dir = d.parent(); + } + false +} + /// Emits a trace line to stderr when `AGENTFLARE_SHIM_TRACE` is set. pub fn trace(msg: &str) { if is_set("AGENTFLARE_SHIM_TRACE") { @@ -141,4 +170,69 @@ mod tests { let b = Path::new("C:/Users/shiva/.agentflare/shims"); assert!(paths_eq(a, b), "/ vs \\ differences must match on Windows"); } + + #[test] + fn finds_marker_in_start_dir() { + let tmp = std::env::temp_dir().join(format!("agentflare-shim-test-{}", std::process::id())); + std::fs::create_dir_all(&tmp).unwrap(); + std::fs::write(tmp.join(PROJECT_MARKER), "").unwrap(); + assert!(in_scoped_project(&tmp, None)); + let _ = std::fs::remove_dir_all(&tmp); + } + + #[test] + fn finds_marker_in_an_ancestor_dir() { + let tmp = + std::env::temp_dir().join(format!("agentflare-shim-test-anc-{}", std::process::id())); + let sub = tmp.join("a").join("b"); + std::fs::create_dir_all(&sub).unwrap(); + std::fs::write(tmp.join(PROJECT_MARKER), "").unwrap(); + assert!(in_scoped_project(&sub, None)); + let _ = std::fs::remove_dir_all(&tmp); + } + + #[test] + fn stops_at_home_without_treating_agentflares_own_dir_as_a_project_marker() { + // ~/.agentflare is agentflare's own app-data dir, not a project + // marker -- walking past `home` (inclusive of home itself) must + // never false-positive on it. Regression for the bug the doc + // comment on `in_scoped_project` calls out. + let tmp = + std::env::temp_dir().join(format!("agentflare-shim-test-home-{}", std::process::id())); + let sub = tmp.join("sub"); + std::fs::create_dir_all(&sub).unwrap(); + std::fs::write(tmp.join(PROJECT_MARKER), "").unwrap(); + assert!(!in_scoped_project(&sub, Some(&tmp))); + let _ = std::fs::remove_dir_all(&tmp); + } + + #[cfg(any(windows, target_os = "macos"))] + #[test] + fn stops_at_home_even_when_it_only_case_matches_the_walked_ancestor() { + // Regression for the exact bug this fix closes: a real ambient home + // dir on a Windows CI runner didn't byte-match the walk's own + // ancestor path, so the `home` boundary never triggered. + let tmp = + std::env::temp_dir().join(format!("agentflare-shim-test-case-{}", std::process::id())); + let sub = tmp.join("sub"); + std::fs::create_dir_all(&sub).unwrap(); + std::fs::write(tmp.join(PROJECT_MARKER), "").unwrap(); + let uppercased_home = Path::new(&tmp.to_string_lossy().to_uppercase()).to_path_buf(); + assert!(!in_scoped_project(&sub, Some(&uppercased_home))); + let _ = std::fs::remove_dir_all(&tmp); + } + + #[test] + fn no_marker_anywhere_is_not_scoped() { + // Bound the walk-up with an explicit synthetic `home` one level + // above `tmp`, rather than `None` -- an unbounded walk from a real + // temp dir keeps climbing past this test's control (e.g. up into + // the real machine's actual `~/.agentflare`, giving a false pass/fail + // that has nothing to do with the logic under test). + let tmp = + std::env::temp_dir().join(format!("agentflare-shim-test-none-{}", std::process::id())); + std::fs::create_dir_all(&tmp).unwrap(); + assert!(!in_scoped_project(&tmp, tmp.parent())); + let _ = std::fs::remove_dir_all(&tmp); + } } diff --git a/crates/agentflare-shim/src/main.rs b/crates/agentflare-shim/src/main.rs index 92df7e07..a214d64e 100644 --- a/crates/agentflare-shim/src/main.rs +++ b/crates/agentflare-shim/src/main.rs @@ -30,7 +30,9 @@ 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}; +use agentflare_shim::{ + in_scoped_project, is_set, path_without_shim_dir, run_real, tool_name_from_exe, trace, +}; const KILL_SWITCHES: &[&str] = &["LEAN_CTX_DISABLED", "LEAN_CTX_NO_HOOK"]; @@ -43,30 +45,10 @@ const AGENT_ENV_VARS: &[&str] = &[ "CODEBUDDY", ]; -const PROJECT_MARKER: &str = ".agentflare"; - 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 -} - fn main() { let exe = match env::current_exe() { Ok(p) => p, @@ -110,57 +92,4 @@ fn main() { } } -#[cfg(test)] -mod tests { - use super::*; - use std::fs; - - #[test] - fn finds_marker_in_start_dir() { - let tmp = std::env::temp_dir().join(format!("agentflare-shim-test-{}", std::process::id())); - fs::create_dir_all(&tmp).unwrap(); - fs::write(tmp.join(PROJECT_MARKER), "").unwrap(); - assert!(in_scoped_project(&tmp, None)); - let _ = fs::remove_dir_all(&tmp); - } - - #[test] - fn finds_marker_in_an_ancestor_dir() { - let tmp = - std::env::temp_dir().join(format!("agentflare-shim-test-anc-{}", std::process::id())); - let sub = tmp.join("a").join("b"); - fs::create_dir_all(&sub).unwrap(); - fs::write(tmp.join(PROJECT_MARKER), "").unwrap(); - assert!(in_scoped_project(&sub, None)); - let _ = fs::remove_dir_all(&tmp); - } - - #[test] - fn stops_at_home_without_treating_agentflares_own_dir_as_a_project_marker() { - // ~/.agentflare is agentflare's own app-data dir, not a project - // marker -- walking past `home` (inclusive of home itself) must - // never false-positive on it. Regression for the bug the doc - // comment on `in_scoped_project` calls out. - let tmp = - std::env::temp_dir().join(format!("agentflare-shim-test-home-{}", std::process::id())); - let sub = tmp.join("sub"); - fs::create_dir_all(&sub).unwrap(); - fs::write(tmp.join(PROJECT_MARKER), "").unwrap(); - assert!(!in_scoped_project(&sub, Some(&tmp))); - let _ = fs::remove_dir_all(&tmp); - } - - #[test] - fn no_marker_anywhere_is_not_scoped() { - // Bound the walk-up with an explicit synthetic `home` one level - // above `tmp`, rather than `None` -- an unbounded walk from a real - // temp dir keeps climbing past this test's control (e.g. up into - // the real machine's actual `~/.agentflare`, giving a false pass/fail - // that has nothing to do with the logic under test). - let tmp = - std::env::temp_dir().join(format!("agentflare-shim-test-none-{}", std::process::id())); - fs::create_dir_all(&tmp).unwrap(); - assert!(!in_scoped_project(&tmp, tmp.parent())); - let _ = fs::remove_dir_all(&tmp); - } -} +// `in_scoped_project` and its tests moved to lib.rs (shared with flare-git-core). diff --git a/crates/flare-git-core/Cargo.toml b/crates/flare-git-core/Cargo.toml index fb6d4862..974298dc 100644 --- a/crates/flare-git-core/Cargo.toml +++ b/crates/flare-git-core/Cargo.toml @@ -13,6 +13,7 @@ serde_json = "1" chrono = "0.4" rusqlite = { version = "0.40", features = ["bundled"] } agentflare-backend = { package = "agentflare-backend", path = "../agentflare-backend" } +agentflare-shim = { path = "../agentflare-shim" } walkdir = "2" dirs = "6" agent-detector = "0.2.1" diff --git a/crates/flare-git-core/src/classify.rs b/crates/flare-git-core/src/classify.rs index 8a38d94d..d224d1af 100644 --- a/crates/flare-git-core/src/classify.rs +++ b/crates/flare-git-core/src/classify.rs @@ -9,11 +9,24 @@ //! git's full subcommand surface (submodule, bisect, notes, gc, lfs, ...). //! Only the specific, deliberately-chosen cases below (protected-branch //! 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. +//! mutating `worktree` subcommands) 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. +//! +//! `worktree`'s deny is further scoped: read-only subcommands (`list`, +//! `prune --dry-run`) always pass through regardless of tracking status -- +//! decided right here since it only needs `args`. Mutating `worktree` +//! subcommands still classify as `Deny` from this pure function, same as +//! every other deny case above -- but `classify()` (the I/O-resolving +//! wrapper) then downgrades ANY deny to `Passthrough` when the repo isn't +//! actually agentflare-tracked (`agentflare_shim::in_scoped_project`). +//! Every one of this policy's protections exists for agentflare's own +//! orchestration; none of that rationale holds in a project agentflare +//! doesn't track, and this shim is installed globally on PATH, so without +//! that gate it would police ordinary git use in every unrelated project on +//! the machine too. use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; @@ -221,14 +234,24 @@ pub fn classify_pure( // 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")); + 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))) { + 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(), } @@ -279,9 +302,20 @@ pub fn classify_pure( }, TrustRootTouch::Clean => 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(), - }, + "worktree" => { + let is_read_only = match args.first().map(String::as_str) { + Some("list") => true, + Some("prune") => args.iter().any(|a| a == "--dry-run"), + _ => false, + }; + if is_read_only { + Disposition::Passthrough + } else { + 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(), + } + } + } // 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. @@ -365,6 +399,22 @@ fn pushed_branch(repo_root: &Path, args: &[String]) -> Option { /// trust-root path, then delegates to `classify_pure`. #[must_use] pub fn classify(repo_root: &Path, subcommand: &str, args: &[String]) -> Event { + classify_with_home(repo_root, subcommand, args, dirs::home_dir().as_deref()) +} + +/// Same as `classify`, but with the `in_scoped_project` home boundary +/// injectable -- the real ambient home dir's exact path (short-name vs. +/// long-name forms, drive/case differences) isn't something a test should +/// depend on to stay deterministic across platforms/CI runners; a synthetic +/// `home` here mirrors how `agentflare_shim::in_scoped_project`'s own tests +/// already avoid that dependency. +#[must_use] +pub fn classify_with_home( + repo_root: &Path, + subcommand: &str, + args: &[String], + home: Option<&Path>, +) -> Event { let default_branch = resolve_default_branch(repo_root); // Resolve the actual pushed branch once, then derive both push facts from // it: whether it carries trust-root changes and whether it *is* the @@ -379,13 +429,26 @@ pub fn classify(repo_root: &Path, subcommand: &str, args: &[String]) -> Event { let targets_default_branch = pushed .as_deref() .is_some_and(|b| is_protected_branch(b, Some(&default_branch))); - let disposition = classify_pure( + let mut disposition = classify_pure( subcommand, args, &default_branch, &trust_root_touch, targets_default_branch, ); + // Every deny above (protected-branch checkout/switch/delete/rename, + // trust-root push, plumbing block, worktree) exists to protect agentflare's + // own orchestration in a project it actually tracks. None of that rationale + // holds in an untracked repo -- this shim is installed globally on PATH, so + // without this gate it would police ordinary git use in every unrelated + // project on the machine too, which is worse than the risk it's meant to + // prevent. `in_scoped_project` is agentflare-shim's own established + // project-detection walk-up (shared so this doesn't reinvent it). + if matches!(disposition, Disposition::Deny { .. }) + && !agentflare_shim::in_scoped_project(repo_root, home) + { + disposition = Disposition::Passthrough; + } Event { subcommand: subcommand.to_string(), args: args.to_vec(), @@ -515,6 +578,62 @@ mod tests { )); } + #[test] + fn worktree_remove_is_denied() { + assert!(matches!( + classify_pure( + "worktree", + &args(&["remove", "../x"]), + "master", + &TrustRootTouch::Clean, + false + ), + Disposition::Deny { .. } + )); + } + + #[test] + fn worktree_list_is_passthrough() { + assert_eq!( + classify_pure( + "worktree", + &args(&["list"]), + "master", + &TrustRootTouch::Clean, + false + ), + Disposition::Passthrough + ); + } + + #[test] + fn worktree_prune_dry_run_is_passthrough() { + assert_eq!( + classify_pure( + "worktree", + &args(&["prune", "--dry-run"]), + "master", + &TrustRootTouch::Clean, + false + ), + Disposition::Passthrough + ); + } + + #[test] + fn worktree_prune_without_dry_run_is_denied() { + assert!(matches!( + classify_pure( + "worktree", + &args(&["prune"]), + "master", + &TrustRootTouch::Clean, + false + ), + Disposition::Deny { .. } + )); + } + #[test] fn checkout_to_protected_branch_is_denied() { let d = classify_pure( @@ -838,7 +957,10 @@ mod tests { fn bare_push_on_default_branch_is_denied_end_to_end() { // The common case: `git push` while checked out on the default branch // resolves the current branch (master) and must be blocked, PR-only. + // Needs the `.agentflare` marker now that denies are gated to tracked + // repos -- this test is about the push-deny logic, not the gate. let repo = crate::shell::test_support::init_repo_with_branch("master"); + std::fs::create_dir_all(repo.path.join(".agentflare")).unwrap(); crate::shell::run_in(&repo.path, &["commit", "--allow-empty", "-m", "init"]).unwrap(); let event = classify(&repo.path, "push", &[]); assert!( @@ -848,6 +970,69 @@ mod tests { ); } + #[test] + fn worktree_add_is_denied_in_an_agentflare_tracked_repo() { + let repo = crate::shell::test_support::init_repo_with_branch("master"); + std::fs::create_dir_all(repo.path.join(".agentflare")).unwrap(); + std::fs::write(repo.path.join(".agentflare").join("project.json"), "{}").unwrap(); + let event = classify( + &repo.path, + "worktree", + &["add".to_string(), "../x".to_string()], + ); + assert!( + matches!(event.disposition, Disposition::Deny { .. }), + "{:?}", + event.disposition + ); + } + + #[test] + fn worktree_add_passes_through_in_an_untracked_repo() { + // No `.agentflare/project.json` -- this repo has nothing to do with + // agentflare's item-tracking system, so the orchestrator-managed + // rationale doesn't apply and ordinary worktree use must not be blocked. + // Bounds the walk-up with the repo's own parent as a synthetic home, + // same technique agentflare_shim::in_scoped_project's own tests use -- + // the real ambient home dir's exact path form isn't something a test + // should depend on to stay deterministic across platforms/CI runners. + let repo = crate::shell::test_support::init_repo_with_branch("master"); + let event = classify_with_home( + &repo.path, + "worktree", + &["add".to_string(), "../x".to_string()], + repo.path.parent(), + ); + assert_eq!(event.disposition, Disposition::Passthrough); + } + + #[test] + fn protected_branch_checkout_passes_through_in_an_untracked_repo() { + // The untracked-repo gate isn't worktree-specific: every deny this + // policy produces exists for agentflare's own orchestration, so none + // of it should apply outside a project agentflare actually tracks. + let repo = crate::shell::test_support::init_repo_with_branch("master"); + let event = classify_with_home( + &repo.path, + "checkout", + &["master".to_string()], + repo.path.parent(), + ); + assert_eq!(event.disposition, Disposition::Passthrough); + } + + #[test] + fn protected_branch_checkout_is_still_denied_in_a_tracked_repo() { + let repo = crate::shell::test_support::init_repo_with_branch("master"); + std::fs::create_dir_all(repo.path.join(".agentflare")).unwrap(); + let event = classify(&repo.path, "checkout", &["master".to_string()]); + assert!( + matches!(event.disposition, Disposition::Deny { .. }), + "{:?}", + event.disposition + ); + } + #[test] fn push_trust_root_deny_message_names_only_the_touched_path() { let touch = TrustRootTouch::Touched(vec!["Cargo.toml".to_string()]); diff --git a/crates/flare-git-shim/Cargo.toml b/crates/flare-git-shim/Cargo.toml index 1311ca6d..b88b5d1f 100644 --- a/crates/flare-git-shim/Cargo.toml +++ b/crates/flare-git-shim/Cargo.toml @@ -16,6 +16,7 @@ agentflare-shim = { path = "../agentflare-shim" } flare-git-core = { path = "../flare-git-core" } serde = { version = "1", features = ["derive"] } serde_json = "1" +dirs = "6" [dev-dependencies] tempfile = "3" diff --git a/crates/flare-git-shim/src/main.rs b/crates/flare-git-shim/src/main.rs index 84e37359..8eeb6718 100644 --- a/crates/flare-git-shim/src/main.rs +++ b/crates/flare-git-shim/src/main.rs @@ -133,9 +133,10 @@ fn snapshots_enabled() -> bool { /// 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 +/// via env markers, same as `agentflare-shim`'s own gate) AND an agentflare- +/// tracked project AND the canonical (non-worktree) checkout AND the command +/// would actually detach HEAD. Interactive human use, any use inside an +/// isolated worktree, and any project agentflare doesn't track, are all /// completely unaffected. fn deny_canonical_detach_reason( repo_root: &Path, @@ -148,6 +149,9 @@ fn deny_canonical_detach_reason( if !classify::agent_invocation_detected() { return None; } + if !agentflare_shim::in_scoped_project(repo_root, dirs::home_dir().as_deref()) { + return None; + } if branch::is_linked_worktree(repo_root) { return None; // agent worktrees are exactly where this is expected } diff --git a/crates/flare-git-shim/tests/shim_test.rs b/crates/flare-git-shim/tests/shim_test.rs index 8678c028..f17f2124 100644 --- a/crates/flare-git-shim/tests/shim_test.rs +++ b/crates/flare-git-shim/tests/shim_test.rs @@ -19,6 +19,9 @@ fn init_repo() -> tempfile::TempDir { 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(); + // All denials are now scoped to agentflare-tracked repos -- these tests + // are about the deny logic itself, so mark this fixture as tracked. + std::fs::create_dir_all(path.join(".agentflare")).unwrap(); dir } @@ -91,6 +94,34 @@ fn outside_a_git_repo_passes_through() { assert!(out.status.success(), "{out:?}"); } +#[test] +fn worktree_add_passes_through_in_a_git_repo_with_no_agentflare_tracking() { + // The real-world bug this closes: a standalone project with no + // `.agentflare/` marker at all got the same "orchestrator-managed by + // agentflare" deny as agentflare's own repo, through this exact binary. + 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(); + // Deliberately no .agentflare/ marker -- this is an untracked project. + + let home = tempfile::TempDir::new().unwrap(); + // Absolute, uniquely-named path (its own fresh TempDir, not created yet -- + // `git worktree add` creates it) rather than a relative "../wt", which + // risks colliding with leftover state from a previous run in the same + // parent directory. + let worktree_dir = tempfile::TempDir::new().unwrap(); + let worktree_path = worktree_dir.path().join("wt").to_string_lossy().to_string(); + let out = shim( + path, + home.path(), + &["worktree", "add", &worktree_path, "-b", "feature/x"], + ); + assert!(out.status.success(), "{out:?}"); +} + #[test] fn bypass_env_var_skips_classification_even_for_a_denied_command() { let repo = init_repo();