From 8c48fbe9808d2111e1f28d1e0b9aa9d061e10668 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Wed, 22 Jul 2026 13:13:43 +0530 Subject: [PATCH 1/2] feat(git-shim): path-scope enforcement for claims (QuorumGit adoption) Adopt QuorumGit's claim-scope classification for flare-git-shim, closing the top deferred guard gap: all existing branch-protection layers key off host cwd, not changed file paths, which is why opencode has twice edited the canonical checkout instead of its claim's worktree. - claims can now declare path-glob write scopes (`--scope`/MCP `scope` param on claim acquire); unscoped claims (the back-compat default) never deny another agent's unrelated work - flare-git-core::scope::classify_scopes classifies commit/push against live claim scopes: CLEAR/RELATED pass, OVERLAPPING (touches another live claim's declared scope) and OUT_OF_TREE (own claim held but committing from the canonical checkout) deny - flare-git-shim shells out to the new hidden `agentflare git scope-check` CLI command for commit/push (the shim itself has no DB access) and is deliberately fail-closed on any scope-resolution error, unlike this crate's usual fail-open default; existing bypass envs remain the escape hatch - claim acquire warns (non-blocking) when a new scope overlaps another live claim's declared scope -- v1 enforcement is at mutation time only Every denial is audited to ~/.agentflare/audit/git.jsonl, same sink as the rest of the shim's decisions. Not yet wired: the opencode tool.execute.before plugin (hook_redirect.rs) still only checks branch protection, not claim scopes -- left as a fast-follow since it's a softer, best-effort layer and the native shim/hook boundary is where real enforcement lives. Agentflare-Agent: claude-code_2-1-217_agent Agentflare-Branch: item-234-path-scope-claims --- Cargo.lock | 2 + crates/flare-git-core/src/lib.rs | 1 + crates/flare-git-core/src/scope.rs | 222 +++++++++++++++++++++++++++++ crates/flare-git-shim/Cargo.toml | 2 + crates/flare-git-shim/src/main.rs | 64 +++++++++ src/claims.rs | 122 +++++++++++++--- src/cli/claim.rs | 91 ++++++++---- src/cli/git.rs | 140 +++++++++++++++++- src/mcp_server/claim.rs | 18 ++- src/mcp_server/types.rs | 5 + 10 files changed, 617 insertions(+), 50 deletions(-) create mode 100644 crates/flare-git-core/src/scope.rs diff --git a/Cargo.lock b/Cargo.lock index 7d07dafd..98a37881 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1111,6 +1111,8 @@ version = "0.1.0" dependencies = [ "agentflare-shim", "flare-git-core", + "serde", + "serde_json", "tempfile", ] diff --git a/crates/flare-git-core/src/lib.rs b/crates/flare-git-core/src/lib.rs index 987184d5..d77f69ec 100644 --- a/crates/flare-git-core/src/lib.rs +++ b/crates/flare-git-core/src/lib.rs @@ -8,6 +8,7 @@ pub mod audit; pub mod branch; pub mod classify; pub mod provenance; +pub mod scope; pub mod shell; pub mod snapshot; pub mod worktree; diff --git a/crates/flare-git-core/src/scope.rs b/crates/flare-git-core/src/scope.rs new file mode 100644 index 00000000..ae2f7505 --- /dev/null +++ b/crates/flare-git-core/src/scope.rs @@ -0,0 +1,222 @@ +//! Path-scope classification for claim-aware git enforcement (QuorumGit +//! pattern adoption, item #234) — orthogonal to `classify`'s subcommand +//! policy. The call site (`flare-git-shim`, and eventually the opencode +//! `tool.execute.before` plugin) runs this alongside `classify()` for +//! `commit`/`push`, using live claim data `classify_pure` has no access to. +//! +//! Unscoped claims (no `scope` declared, or `["**"]`) never generate an +//! `Overlapping` verdict — only a claim with a genuinely narrower declared +//! scope is enforced against other agents. This keeps every claim made +//! before this feature shipped (and every claim that never bothers to +//! declare a scope) from silently blocking someone else's unrelated work in +//! the same repo, which is the normal case: many claims coexist per repo, +//! one per target. + +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum ScopeVerdict { + /// No changed paths to classify. + Clear, + /// Changed paths present but none fall inside any enforced scope. + Related, + /// A changed path falls inside another live claim's declared scope. + Overlapping { + owner: String, + target: String, + scope: String, + }, + /// The invoker holds a live claim but is committing/pushing from the + /// canonical checkout rather than that claim's own worktree. + OutOfTree { target: String }, +} + +/// One other agent's live claim, as relevant to scope classification. +#[derive(Debug, Clone)] +pub struct ClaimScope { + pub target: String, + pub owner: String, + /// Empty or `["**"]` — the back-compat default — is never enforced. + pub scopes: Vec, +} + +/// The portion of a glob before its first wildcard character. +#[must_use] +pub fn literal_prefix(glob: &str) -> &str { + glob.find(['*', '?', '[']).map_or(glob, |i| &glob[..i]) +} + +/// Conservative literal-prefix overlap test — QuorumGit's rule: compare +/// each glob's literal prefix against the other's, erring toward flagging +/// rather than missing a real overlap. +#[must_use] +pub fn globs_overlap(a: &str, b: &str) -> bool { + let (pa, pb) = (literal_prefix(a), literal_prefix(b)); + pa.starts_with(pb) || pb.starts_with(pa) +} + +/// `true` if `scopes` is the back-compat default (unscoped) and so must +/// never be used to deny another agent. +#[must_use] +pub fn scope_is_wildcard_or_empty(scopes: &[String]) -> bool { + scopes.is_empty() || scopes.iter().all(|s| s == "**") +} + +fn path_matches_scope(path: &str, scope: &str) -> bool { + scope == "**" || path.starts_with(literal_prefix(scope)) +} + +/// Classifies a set of changed paths against live claims held by other +/// agents, plus the invoker's own claim/worktree state. +/// +/// `own_target` is the invoker's own live claim in this repo, if any. +/// `in_own_worktree` is whether the invoker is currently inside a linked +/// worktree (irrelevant when `own_target` is `None`). `others` are OTHER +/// agents' live claims in this repo. +#[must_use] +pub fn classify_scopes( + changed_paths: &[String], + own_target: Option<&str>, + in_own_worktree: bool, + others: &[ClaimScope], +) -> ScopeVerdict { + if changed_paths.is_empty() { + return ScopeVerdict::Clear; + } + if let Some(target) = own_target + && !in_own_worktree + { + return ScopeVerdict::OutOfTree { + target: target.to_string(), + }; + } + for claim in others { + if scope_is_wildcard_or_empty(&claim.scopes) { + continue; + } + for path in changed_paths { + if let Some(scope) = claim.scopes.iter().find(|s| path_matches_scope(path, s)) { + return ScopeVerdict::Overlapping { + owner: claim.owner.clone(), + target: claim.target.clone(), + scope: scope.clone(), + }; + } + } + } + ScopeVerdict::Related +} + +#[cfg(test)] +mod tests { + use super::*; + + fn paths(v: &[&str]) -> Vec { + v.iter().map(|s| s.to_string()).collect() + } + + fn claim(target: &str, owner: &str, scopes: &[&str]) -> ClaimScope { + ClaimScope { + target: target.to_string(), + owner: owner.to_string(), + scopes: scopes.iter().map(|s| s.to_string()).collect(), + } + } + + #[test] + fn no_changed_paths_is_clear() { + assert_eq!(classify_scopes(&[], None, true, &[]), ScopeVerdict::Clear); + } + + #[test] + fn disjoint_scopes_never_interfere() { + let others = [claim("item#1", "a:1", &["crates/foo/"])]; + assert_eq!( + classify_scopes(&paths(&["crates/bar/src/lib.rs"]), None, true, &others), + ScopeVerdict::Related + ); + } + + #[test] + fn overlapping_scope_is_denied() { + let others = [claim("item#1", "a:1", &["crates/foo/"])]; + assert_eq!( + classify_scopes(&paths(&["crates/foo/src/lib.rs"]), None, true, &others), + ScopeVerdict::Overlapping { + owner: "a:1".to_string(), + target: "item#1".to_string(), + scope: "crates/foo/".to_string(), + } + ); + } + + #[test] + fn unscoped_other_claim_never_blocks() { + // The back-compat default (no scope declared) must not deny + // unrelated work just because *some* claim exists in the repo. + let others = [claim("item#1", "a:1", &[])]; + assert_eq!( + classify_scopes(&paths(&["crates/foo/src/lib.rs"]), None, true, &others), + ScopeVerdict::Related + ); + let wildcard = [claim("item#1", "a:1", &["**"])]; + assert_eq!( + classify_scopes(&paths(&["crates/foo/src/lib.rs"]), None, true, &wildcard), + ScopeVerdict::Related + ); + } + + #[test] + fn own_claim_in_canonical_checkout_is_out_of_tree() { + assert_eq!( + classify_scopes(&paths(&["src/main.rs"]), Some("item#2"), false, &[]), + ScopeVerdict::OutOfTree { + target: "item#2".to_string() + } + ); + } + + #[test] + fn own_claim_inside_its_worktree_passes() { + assert_eq!( + classify_scopes(&paths(&["src/main.rs"]), Some("item#2"), true, &[]), + ScopeVerdict::Related + ); + } + + #[test] + fn own_claim_with_no_changes_never_blocks() { + // e.g. `git commit --allow-empty` — nothing actually moved, so + // there is nothing to be "out of tree" about. + assert_eq!( + classify_scopes(&[], Some("item#2"), false, &[]), + ScopeVerdict::Clear + ); + } + + #[test] + fn out_of_tree_takes_priority_over_overlap_checks() { + let others = [claim("item#1", "a:1", &["src/"])]; + assert_eq!( + classify_scopes(&paths(&["src/main.rs"]), Some("item#2"), false, &others), + ScopeVerdict::OutOfTree { + target: "item#2".to_string() + } + ); + } + + #[test] + fn globs_overlap_is_symmetric_and_conservative() { + assert!(globs_overlap("crates/foo/", "crates/foo/src/")); + assert!(globs_overlap("crates/foo/src/", "crates/foo/")); + assert!(!globs_overlap("crates/foo/", "crates/bar/")); + assert!(globs_overlap("crates/foo/**", "crates/foo/src/lib.rs")); + } + + #[test] + fn literal_prefix_stops_at_first_wildcard() { + assert_eq!(literal_prefix("crates/foo/**"), "crates/foo/"); + assert_eq!(literal_prefix("crates/foo/"), "crates/foo/"); + assert_eq!(literal_prefix("**"), ""); + } +} diff --git a/crates/flare-git-shim/Cargo.toml b/crates/flare-git-shim/Cargo.toml index cf2c49d1..1311ca6d 100644 --- a/crates/flare-git-shim/Cargo.toml +++ b/crates/flare-git-shim/Cargo.toml @@ -14,6 +14,8 @@ path = "src/main.rs" [dependencies] agentflare-shim = { path = "../agentflare-shim" } flare-git-core = { path = "../flare-git-core" } +serde = { version = "1", features = ["derive"] } +serde_json = "1" [dev-dependencies] tempfile = "3" diff --git a/crates/flare-git-shim/src/main.rs b/crates/flare-git-shim/src/main.rs index 619060f9..84e37359 100644 --- a/crates/flare-git-shim/src/main.rs +++ b/crates/flare-git-shim/src/main.rs @@ -159,6 +159,54 @@ fn deny_canonical_detach_reason( )) } +#[derive(serde::Deserialize)] +struct ScopeCheckResult { + deny: bool, + reason: Option, +} + +/// Path-scope enforcement (item #234): shells out to `agentflare git +/// scope-check`, since this shim has no direct DB access to live claims. +/// Deliberately FAIL-CLOSED here, unlike the rest of this crate's fail-open +/// default -- any error resolving scope (binary missing, bad JSON, +/// non-zero exit) is treated as a deny. The existing bypass envs +/// (`AGENTFLARE_GIT_BYPASS` and friends, checked earlier in `main`) remain +/// the escape hatch for a broken/missing `agentflare` binary, same as any +/// other misclassification. +fn scope_check_deny_reason(subcommand: &str) -> Option { + let output = match std::process::Command::new("agentflare") + .args(["git", "scope-check", "--subcommand", subcommand]) + .output() + { + Ok(o) => o, + Err(e) => { + return Some(format!( + "scope-check could not run ('agentflare' on PATH?): {e}" + )); + } + }; + if !output.status.success() { + return Some(format!( + "scope-check exited non-zero: {}", + String::from_utf8_lossy(&output.stderr).trim() + )); + } + let stdout = String::from_utf8_lossy(&output.stdout); + let result: ScopeCheckResult = match serde_json::from_str(stdout.trim()) { + Ok(r) => r, + Err(e) => return Some(format!("scope-check returned unparseable output: {e}")), + }; + if result.deny { + Some( + result + .reason + .unwrap_or_else(|| "scope-check denied (no reason given)".to_string()), + ) + } else { + None + } +} + fn main() { let depth: u32 = env::var(RECURSION_ENV) .ok() @@ -275,6 +323,22 @@ fn main() { exit(1); } classify::Disposition::Passthrough | classify::Disposition::SilentExempt => { + if matches!(subcommand.as_str(), "commit" | "push") + && let Some(reason) = scope_check_deny_reason(&subcommand) + { + let scope_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, &scope_event); + } + eprintln!("agentflare git shim: denied — {reason}"); + exit(1); + } if snapshots_enabled() && classify::is_destructive(&subcommand, &rest) { let reason = format!("pre-{subcommand} snapshot ({})", rest.join(" ")); match snapshot::snapshot_before(&repo_root, &reason) { diff --git a/src/claims.rs b/src/claims.rs index 2ae0c8a7..3ad05d31 100644 --- a/src/claims.rs +++ b/src/claims.rs @@ -36,7 +36,21 @@ pub fn migrate(conn: &Connection) -> rusqlite::Result<()> { git_commit TEXT, PRIMARY KEY (repo, target) );", - ) + )?; + add_scope_column_if_missing(conn) +} + +/// Additive migration for installs that created `claims` before the `scope` +/// column existed — unlike `CREATE TABLE IF NOT EXISTS`, `ALTER TABLE ADD +/// COLUMN` isn't naturally idempotent, so this checks first. +fn add_scope_column_if_missing(conn: &Connection) -> rusqlite::Result<()> { + let has_scope: bool = conn + .prepare("SELECT 1 FROM pragma_table_info('claims') WHERE name = 'scope'")? + .exists([])?; + if !has_scope { + conn.execute("ALTER TABLE claims ADD COLUMN scope TEXT", [])?; + } + Ok(()) } #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] @@ -48,6 +62,10 @@ pub struct Claim { pub created_at: i64, pub heartbeat_at: i64, pub git_commit: Option, + /// Path globs this claim's owner declared write ownership over. Empty + /// (the back-compat default) means "no scope declared" — never used to + /// deny another agent (see `flare_git_core::scope`). + pub scope: Vec, /// Heartbeat older than the TTL — the claim is effectively available. pub stale: bool, } @@ -58,22 +76,28 @@ pub struct Claim { /// generic `ClaimLedger` knows about, so it can't be part of the atomic /// upsert itself; this is a deliberate two-step, not an oversight, and it's /// safe because the second statement only ever touches a row we just won. +// 8 positional args (one over clippy's default threshold) is still the +// clearest signature here -- every param is self-explanatory, and this +// ledger already has no builder/options-struct precedent elsewhere. +#[allow(clippy::too_many_arguments)] pub fn acquire( conn: &Connection, repo: &str, target: &str, owner: &str, git_commit: Option<&str>, + scope: Option<&[String]>, now: i64, ttl_secs: i64, ) -> rusqlite::Result { let outcome = LEDGER.acquire(conn, &[repo, target], owner, now, ttl_secs)?; if outcome == Acquire::Acquired { + let scope_json = scope.map(|s| serde_json::to_string(s).unwrap_or_default()); // Scoped to owner: if another owner steals the lease between LEDGER.acquire() // and this UPDATE, this must not overwrite their row's provenance with ours. conn.execute( - "UPDATE claims SET git_commit = ?3 WHERE repo = ?1 AND target = ?2 AND owner = ?4", - params![repo, target, git_commit, owner], + "UPDATE claims SET git_commit = ?3, scope = ?5 WHERE repo = ?1 AND target = ?2 AND owner = ?4", + params![repo, target, git_commit, owner, scope_json], )?; } Ok(outcome) @@ -119,7 +143,7 @@ pub fn list( ) -> rusqlite::Result> { let stale_before = now - ttl_secs; let mut stmt = conn.prepare( - "SELECT repo, target, owner, status, created_at, heartbeat_at, git_commit + "SELECT repo, target, owner, status, created_at, heartbeat_at, git_commit, scope FROM claims WHERE (?1 IS NULL OR repo = ?1) ORDER BY repo, target", @@ -127,6 +151,7 @@ pub fn list( let rows = stmt.query_map(params![repo], |r| { let heartbeat_at: i64 = r.get(5)?; let status: String = r.get(3)?; + let scope_json: Option = r.get(7)?; Ok(Claim { repo: r.get(0)?, target: r.get(1)?, @@ -136,6 +161,9 @@ pub fn list( created_at: r.get(4)?, heartbeat_at, git_commit: r.get(6)?, + scope: scope_json + .and_then(|s| serde_json::from_str(&s).ok()) + .unwrap_or_default(), }) })?; let all: Vec = rows.collect::>()?; @@ -148,6 +176,41 @@ pub fn list( }) } +/// Best-effort claim-time warning (never blocks -- v1 scope enforcement is +/// at mutation time, see `flare_git_core::scope`): does `scope` overlap a +/// DIFFERENT live claim's declared scope in the same repo? +pub fn scope_overlap_warning( + conn: &Connection, + repo: &str, + target: &str, + scope: &[String], + now: i64, + ttl_secs: i64, +) -> rusqlite::Result> { + if flare_git_core::scope::scope_is_wildcard_or_empty(scope) { + return Ok(None); + } + let others = list(conn, Some(repo), false, now, ttl_secs)?; + for other in others { + if other.target == target || flare_git_core::scope::scope_is_wildcard_or_empty(&other.scope) + { + continue; + } + if scope.iter().any(|a| { + other + .scope + .iter() + .any(|b| flare_git_core::scope::globs_overlap(a, b)) + }) { + return Ok(Some(format!( + "scope overlaps live claim '{}' (owner {}, scope {:?})", + other.target, other.owner, other.scope + ))); + } + } + Ok(None) +} + // --- identity / config resolution (impure; thin wrappers over env + git) --- /// `:` — same agent chain as handoff, plus an instance @@ -262,10 +325,10 @@ mod tests { fn acquire_free_target_then_held_by_other() { let c = mem(); assert_eq!( - acquire(&c, "o/r", "issue#1", "a:1", None, 1000, TTL).unwrap(), + acquire(&c, "o/r", "issue#1", "a:1", None, None, 1000, TTL).unwrap(), Acquire::Acquired ); - match acquire(&c, "o/r", "issue#1", "b:2", None, 1001, TTL).unwrap() { + match acquire(&c, "o/r", "issue#1", "b:2", None, None, 1001, TTL).unwrap() { Acquire::Held { owner, .. } => assert_eq!(owner, "a:1"), other => panic!("expected Held, got {other:?}"), } @@ -274,9 +337,9 @@ mod tests { #[test] fn reacquiring_own_live_claim_is_idempotent_and_refreshes_heartbeat() { let c = mem(); - acquire(&c, "o/r", "issue#1", "a:1", None, 1000, TTL).unwrap(); + acquire(&c, "o/r", "issue#1", "a:1", None, None, 1000, TTL).unwrap(); assert_eq!( - acquire(&c, "o/r", "issue#1", "a:1", None, 1500, TTL).unwrap(), + acquire(&c, "o/r", "issue#1", "a:1", None, None, 1500, TTL).unwrap(), Acquire::Acquired ); let hb: i64 = c @@ -288,15 +351,15 @@ mod tests { #[test] fn stale_claim_is_stealable_but_fresh_one_is_not() { let c = mem(); - acquire(&c, "o/r", "issue#1", "a:1", None, 1000, TTL).unwrap(); + acquire(&c, "o/r", "issue#1", "a:1", None, None, 1000, TTL).unwrap(); // Well within TTL — cannot steal. assert!(matches!( - acquire(&c, "o/r", "issue#1", "b:2", None, 1000 + 100, TTL).unwrap(), + acquire(&c, "o/r", "issue#1", "b:2", None, None, 1000 + 100, TTL).unwrap(), Acquire::Held { .. } )); // Past the TTL — steal succeeds and ownership transfers. assert_eq!( - acquire(&c, "o/r", "issue#1", "b:2", None, 1000 + TTL + 1, TTL).unwrap(), + acquire(&c, "o/r", "issue#1", "b:2", None, None, 1000 + TTL + 1, TTL).unwrap(), Acquire::Acquired ); let owner: String = c @@ -308,10 +371,10 @@ mod tests { #[test] fn done_target_is_reacquirable_by_anyone() { let c = mem(); - acquire(&c, "o/r", "issue#1", "a:1", None, 1000, TTL).unwrap(); + acquire(&c, "o/r", "issue#1", "a:1", None, None, 1000, TTL).unwrap(); assert!(done(&c, "o/r", "issue#1", "a:1", 1100).unwrap()); assert_eq!( - acquire(&c, "o/r", "issue#1", "b:2", None, 1200, TTL).unwrap(), + acquire(&c, "o/r", "issue#1", "b:2", None, None, 1200, TTL).unwrap(), Acquire::Acquired ); } @@ -319,7 +382,7 @@ mod tests { #[test] fn heartbeat_release_done_are_owner_scoped() { let c = mem(); - acquire(&c, "o/r", "issue#1", "a:1", None, 1000, TTL).unwrap(); + acquire(&c, "o/r", "issue#1", "a:1", None, None, 1000, TTL).unwrap(); assert!(!heartbeat(&c, "o/r", "issue#1", "b:2", 1100).unwrap()); assert!(!release(&c, "o/r", "issue#1", "b:2").unwrap()); assert!(!done(&c, "o/r", "issue#1", "b:2", 1100).unwrap()); @@ -330,8 +393,8 @@ mod tests { #[test] fn list_hides_stale_and_done_unless_requested() { let c = mem(); - acquire(&c, "o/r", "issue#1", "a:1", None, 1000, TTL).unwrap(); - acquire(&c, "o/r", "issue#2", "a:1", None, 1000, TTL).unwrap(); + acquire(&c, "o/r", "issue#1", "a:1", None, None, 1000, TTL).unwrap(); + acquire(&c, "o/r", "issue#2", "a:1", None, None, 1000, TTL).unwrap(); done(&c, "o/r", "issue#2", "a:1", 1000).unwrap(); // At now well past issue#1's TTL, it is stale. let now = 1000 + TTL + 5; @@ -345,14 +408,37 @@ mod tests { #[test] fn list_scopes_by_repo() { let c = mem(); - acquire(&c, "o/r1", "issue#1", "a:1", None, 1000, TTL).unwrap(); - acquire(&c, "o/r2", "issue#1", "a:1", None, 1000, TTL).unwrap(); + acquire(&c, "o/r1", "issue#1", "a:1", None, None, 1000, TTL).unwrap(); + acquire(&c, "o/r2", "issue#1", "a:1", None, None, 1000, TTL).unwrap(); let r1 = list(&c, Some("o/r1"), true, 1000, TTL).unwrap(); assert_eq!(r1.len(), 1); assert_eq!(r1[0].repo, "o/r1"); assert_eq!(list(&c, None, true, 1000, TTL).unwrap().len(), 2); } + #[test] + fn acquire_persists_and_overwrites_scope() { + let c = mem(); + let scope = vec!["crates/foo/".to_string()]; + acquire(&c, "o/r", "issue#1", "a:1", None, Some(&scope), 1000, TTL).unwrap(); + let claims = list(&c, Some("o/r"), true, 1000, TTL).unwrap(); + assert_eq!(claims[0].scope, scope); + + // Re-acquiring with no scope overwrites it back to unscoped, mirroring + // git_commit's always-overwrite behavior. + acquire(&c, "o/r", "issue#1", "a:1", None, None, 1000, TTL).unwrap(); + let claims = list(&c, Some("o/r"), true, 1000, TTL).unwrap(); + assert!(claims[0].scope.is_empty()); + } + + #[test] + fn list_defaults_missing_scope_to_empty() { + let c = mem(); + acquire(&c, "o/r", "issue#1", "a:1", None, None, 1000, TTL).unwrap(); + let claims = list(&c, Some("o/r"), true, 1000, TTL).unwrap(); + assert!(claims[0].scope.is_empty()); + } + #[test] fn normalize_repo_handles_https_ssh_alias_and_dotgit() { assert_eq!( diff --git a/src/cli/claim.rs b/src/cli/claim.rs index fbcc33df..3e2752a0 100644 --- a/src/cli/claim.rs +++ b/src/cli/claim.rs @@ -17,6 +17,11 @@ pub enum ClaimAction { /// Repo key (default: normalized origin remote, owner/name). #[arg(long)] repo: Option, + /// Path glob(s) this claim owns write scope over (repeatable), e.g. + /// --scope crates/foo/ --scope docs/foo/. Omit for the back-compat + /// default (unscoped -- never enforced against other agents). + #[arg(long)] + scope: Vec, }, /// Refresh the lease on a target you own. Heartbeat { @@ -64,36 +69,11 @@ impl ClaimArgs { let now = crate::claims::now(); match self.action { - ClaimAction::Acquire { target, repo } => { - // Only attach the current checkout's commit when the repo was - // auto-resolved from it; an explicit --repo may name a different - // repository, so HEAD here would be misleading provenance. - let commit = if repo.is_none() { git_commit() } else { None }; - let repo = require_repo(repo); - match crate::claims::acquire( - &conn, - &repo, - &target, - &owner, - commit.as_deref(), - now, - ttl, - ) { - Ok(crate::claims::Acquire::Acquired) => { - println!("claimed {repo} {target} (owner {owner})"); - } - Ok(crate::claims::Acquire::Held { - owner: holder, - age_secs, - }) => { - crate::ui::error(&format!( - "{repo} {target} already held by {holder} ({age_secs}s since heartbeat)" - )); - std::process::exit(1); - } - Err(e) => fail(e), - } - } + ClaimAction::Acquire { + target, + repo, + scope, + } => acquire_cmd(&conn, &owner, ttl, now, target, repo, scope), ClaimAction::Heartbeat { target, repo } => { let repo = require_repo(repo); report( @@ -155,6 +135,57 @@ impl ClaimArgs { } } +/// `ClaimAction::Acquire` handler, split out to keep `run`'s dispatch match +/// flat now that scope handling adds a warning check on top of the plain +/// acquire/held/error branches. +fn acquire_cmd( + conn: &rusqlite::Connection, + owner: &str, + ttl: i64, + now: i64, + target: String, + repo: Option, + scope: Vec, +) { + // Only attach the current checkout's commit when the repo was + // auto-resolved from it; an explicit --repo may name a different + // repository, so HEAD here would be misleading provenance. + let commit = if repo.is_none() { git_commit() } else { None }; + let repo = require_repo(repo); + let scope_arg = (!scope.is_empty()).then_some(scope.as_slice()); + match crate::claims::acquire( + conn, + &repo, + &target, + owner, + commit.as_deref(), + scope_arg, + now, + ttl, + ) { + Ok(crate::claims::Acquire::Acquired) => { + println!("claimed {repo} {target} (owner {owner})"); + if let Some(s) = scope_arg { + let warning = + crate::claims::scope_overlap_warning(conn, &repo, &target, s, now, ttl); + if let Ok(Some(warning)) = warning { + crate::ui::error(&format!("warning: {warning}")); + } + } + } + Ok(crate::claims::Acquire::Held { + owner: holder, + age_secs, + }) => { + crate::ui::error(&format!( + "{repo} {target} already held by {holder} ({age_secs}s since heartbeat)" + )); + std::process::exit(1); + } + Err(e) => fail(e), + } +} + /// A verb that returns "did it change my row" → owner-scoped success message. fn report(res: rusqlite::Result, verb: &str, repo: &str, target: &str, owner: &str) { match res { diff --git a/src/cli/git.rs b/src/cli/git.rs index 74ae7567..836b9095 100644 --- a/src/cli/git.rs +++ b/src/cli/git.rs @@ -13,7 +13,7 @@ use crate::paths::home; use clap::{Args, Subcommand}; -use flare_git_core::{audit, branch, provenance, snapshot}; +use flare_git_core::{audit, branch, classify, provenance, scope, shell, snapshot}; use std::fs; use std::io::Read as _; use std::path::{Path, PathBuf}; @@ -43,6 +43,10 @@ pub enum GitCommand { /// updates from stdin and appends them to the backstop audit log. #[command(hide = true)] RefTransactionLog, + /// (Internal, called by flare-git-shim.) Checks a commit/push against + /// live claim scopes -- see item #234. + #[command(hide = true)] + ScopeCheck(ScopeCheckArgs), } #[derive(Args)] @@ -68,6 +72,13 @@ pub struct TrailerInjectArgs { pub msg_file: PathBuf, } +#[derive(Args)] +pub struct ScopeCheckArgs { + /// The subcommand being checked -- "commit" or "push". + #[arg(long)] + pub subcommand: String, +} + #[derive(Args)] pub struct SnapshotArgs { #[command(subcommand)] @@ -144,6 +155,7 @@ pub fn run(args: GitArgs) { GitCommand::Snapshot(opts) => snapshot_cmd(opts), GitCommand::TrailerInject(opts) => trailer_inject(&opts.msg_file), GitCommand::RefTransactionLog => ref_transaction_log(), + GitCommand::ScopeCheck(opts) => scope_check(&opts.subcommand), } } @@ -467,3 +479,129 @@ fn ref_transaction_log() { let _ = audit::log_event(&path, &event); } } + +#[derive(serde::Serialize)] +struct ScopeCheckResult { + deny: bool, + reason: Option, +} + +fn scope_pass() -> ScopeCheckResult { + ScopeCheckResult { + deny: false, + reason: None, + } +} + +fn scope_deny(reason: String) -> ScopeCheckResult { + ScopeCheckResult { + deny: true, + reason: Some(reason), + } +} + +/// `agentflare git scope-check --subcommand commit|push` -- called by +/// flare-git-shim before letting a commit/push through, to enforce item +/// #234's claim path-scopes (data the shim itself has no DB access to). +/// Always prints one line of JSON to stdout and exits 0 -- denial lives IN +/// the JSON (`deny`/`reason`), not the exit code, so the shim can tell +/// "scope-check ran and said no" apart from "scope-check itself failed to +/// run at all" (the latter is the shim's fail-closed case, per this +/// feature's spec -- unlike this crate's usual fail-open default). +fn scope_check(subcommand: &str) { + let result = run_scope_check(subcommand); + let json = serde_json::to_string(&result).unwrap_or_else(|_| { + r#"{"deny":true,"reason":"internal error serializing scope-check result"}"#.to_string() + }); + println!("{json}"); +} + +fn run_scope_check(subcommand: &str) -> ScopeCheckResult { + // Scope enforcement only applies to agent-driven invocations, mirroring + // `flare-git-shim`'s existing canonical-detach guard -- interactive + // human use is never affected. + if !classify::agent_invocation_detected() { + return scope_pass(); + } + let cwd = std::env::current_dir().unwrap_or_default(); + let Some(repo_root) = branch::repo_toplevel(&cwd) else { + return scope_pass(); // not in a repo -- nothing to check + }; + let Some(repo) = crate::claims::resolve_repo(None) else { + return scope_pass(); // no resolvable repo key -> no claims possible + }; + let conn = match crate::db::open() { + Ok(c) => c, + Err(e) => return scope_deny(format!("cannot open claim ledger: {e}")), + }; + let now = crate::claims::now(); + let ttl = crate::claims::ttl_secs(); + let live = match crate::claims::list(&conn, Some(&repo), false, now, ttl) { + Ok(v) => v, + Err(e) => return scope_deny(format!("cannot query live claims: {e}")), + }; + if live.is_empty() { + return scope_pass(); + } + + let owner = crate::claims::owner_id(); + let agent = crate::claims::agent_of(&owner); + let own_target = live + .iter() + .find(|c| crate::claims::agent_of(&c.owner) == agent) + .map(|c| c.target.clone()); + let others: Vec = live + .iter() + .filter(|c| crate::claims::agent_of(&c.owner) != agent) + .map(|c| scope::ClaimScope { + target: c.target.clone(), + owner: c.owner.clone(), + scopes: c.scope.clone(), + }) + .collect(); + + let changed = changed_paths(&repo_root, subcommand); + let in_worktree = branch::is_linked_worktree(&repo_root); + match scope::classify_scopes(&changed, own_target.as_deref(), in_worktree, &others) { + scope::ScopeVerdict::Overlapping { + owner, + target, + scope, + } => scope_deny(format!( + "this touches path(s) inside claim '{target}' (owner {owner}, scope '{scope}') -- work inside that claim's own worktree, or coordinate with {owner}." + )), + scope::ScopeVerdict::OutOfTree { target } => scope_deny(format!( + "you hold claim '{target}' -- do this work in its isolated worktree, not the canonical checkout (see `git worktree add`)." + )), + scope::ScopeVerdict::Clear | scope::ScopeVerdict::Related => scope_pass(), + } +} + +/// Changed paths for the mutation about to happen -- staged paths for +/// `commit`, paths diffed against the default branch for `push`. A v1 +/// simplification for `push`: diffs current-vs-default rather than parsing +/// the actual push refspec across the CLI subprocess boundary. An +/// unreadable diff yields no changed paths (nothing to enforce), matching +/// this crate's fail-open default for diff resolution specifically -- only +/// scope RESOLUTION errors (ledger/DB) are fail-closed, per the spec. +fn changed_paths(repo_root: &Path, subcommand: &str) -> Vec { + let range_args: Vec = match subcommand { + "commit" => vec![ + "diff".to_string(), + "--cached".to_string(), + "--name-only".to_string(), + ], + "push" => { + let default_branch = branch::resolve_default_branch(repo_root); + let current = + branch::current_branch(repo_root).unwrap_or_else(|| default_branch.clone()); + let range = format!("{default_branch}...{current}"); + vec!["diff".to_string(), "--name-only".to_string(), range] + } + _ => return Vec::new(), + }; + let args: Vec<&str> = range_args.iter().map(String::as_str).collect(); + shell::run_in(repo_root, &args) + .map(|s| s.lines().map(String::from).collect()) + .unwrap_or_default() +} diff --git a/src/mcp_server/claim.rs b/src/mcp_server/claim.rs index c498c6d2..d61693ef 100644 --- a/src/mcp_server/claim.rs +++ b/src/mcp_server/claim.rs @@ -29,18 +29,34 @@ impl AgentflareMcp { } else { Self::git_provenance().and_then(|g| g.commit) }; + let scope_arg = (!req.scope.is_empty()).then_some(req.scope.as_slice()); let outcome = crate::claims::acquire( &conn, &repo, &target, &owner, commit.as_deref(), + scope_arg, crate::claims::now(), crate::claims::ttl_secs(), ) .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; Ok(match outcome { - crate::claims::Acquire::Acquired => serde_json::json!({ "status": "acquired", "repo": repo, "target": target, "owner": owner }), + crate::claims::Acquire::Acquired => { + let scope_warning = scope_arg.and_then(|s| { + crate::claims::scope_overlap_warning( + &conn, + &repo, + &target, + s, + crate::claims::now(), + crate::claims::ttl_secs(), + ) + .ok() + .flatten() + }); + serde_json::json!({ "status": "acquired", "repo": repo, "target": target, "owner": owner, "scope_warning": scope_warning }) + } crate::claims::Acquire::Held { owner: holder, age_secs } => serde_json::json!({ "status": "held", "repo": repo, "target": target, "owner": holder, "age_secs": age_secs }), }.to_string()) } diff --git a/src/mcp_server/types.rs b/src/mcp_server/types.rs index b3956edc..e5b4ef6d 100644 --- a/src/mcp_server/types.rs +++ b/src/mcp_server/types.rs @@ -106,6 +106,11 @@ pub(crate) struct ClaimRequest { #[schemars(description = "List across every repo in the ledger (default false) (list)")] #[serde(default)] pub(crate) all_repos: bool, + #[schemars( + description = "Path glob(s) this claim owns write scope over (acquire). Omit for the back-compat default (unscoped -- never enforced against other agents)." + )] + #[serde(default)] + pub(crate) scope: Vec, } #[derive(Debug, Deserialize, schemars::JsonSchema)] From 9b65cc17943d196325a0f857a709dad7caa50561 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Wed, 22 Jul 2026 14:05:39 +0530 Subject: [PATCH 2/2] fix(git-shim): close git commit -a scope-check bypass, warn on scope clear Two CodeRabbit findings on PR #303: - changed_paths() for "commit" only checked `git diff --cached`, so `git commit -a`/`--all` (which implicitly stages+commits tracked modifications without a prior `git add`) bypassed path-scope enforcement entirely. Union staged + working-tree diffs instead. - claims::acquire() always overwrites the scope column (matching git_commit's existing always-overwrite behavior), so re-acquiring an already-held claim without re-supplying --scope silently disabled enforcement with no signal anywhere. Added scope_clear_warning(), wired into both the CLI and MCP acquire handlers alongside the existing overlap warning. Agentflare-Agent: claude-code_2-1-217_agent Agentflare-Branch: item-234-path-scope-claims --- src/claims.rs | 74 +++++++++++++++++++++++++++++++- src/cli/claim.rs | 7 +++- src/cli/git.rs | 93 +++++++++++++++++++++++++++++++++++++---- src/mcp_server/claim.rs | 28 ++++++++----- 4 files changed, 180 insertions(+), 22 deletions(-) diff --git a/src/claims.rs b/src/claims.rs index 3ad05d31..42ab49aa 100644 --- a/src/claims.rs +++ b/src/claims.rs @@ -16,7 +16,7 @@ //! below for how it's threaded through instead. pub use db_kit::claim::Acquire; use db_kit::claim::ClaimLedger; -use rusqlite::{Connection, params}; +use rusqlite::{Connection, OptionalExtension, params}; /// Default lease: a claim whose owner hasn't heartbeat within this window is /// stealable, so a crashed/hung agent can't wedge a target forever. @@ -211,6 +211,41 @@ pub fn scope_overlap_warning( Ok(None) } +/// Best-effort claim-time warning: does re-acquiring with `new_scope` clear +/// an existing, previously-declared non-empty scope? `acquire()` always +/// overwrites the `scope` column (mirroring `git_commit`'s always-overwrite +/// behavior), so a caller re-acquiring an already-held claim without +/// re-supplying `--scope`/`scope` silently disables path-scope enforcement +/// for it -- read BEFORE calling `acquire()` so this reflects the row's +/// state prior to the overwrite. +pub fn scope_clear_warning( + conn: &Connection, + repo: &str, + target: &str, + new_scope: Option<&[String]>, +) -> rusqlite::Result> { + if new_scope.is_some_and(|s| !flare_git_core::scope::scope_is_wildcard_or_empty(s)) { + return Ok(None); // caller is declaring a real scope -- nothing being cleared + } + let existing: Option = conn + .query_row( + "SELECT scope FROM claims WHERE repo = ?1 AND target = ?2", + params![repo, target], + |r| r.get(0), + ) + .optional()? + .flatten(); + let existing_scope: Vec = existing + .and_then(|s| serde_json::from_str(&s).ok()) + .unwrap_or_default(); + if flare_git_core::scope::scope_is_wildcard_or_empty(&existing_scope) { + return Ok(None); + } + Ok(Some(format!( + "re-acquiring without --scope clears the existing scope {existing_scope:?} -- pass --scope again to keep enforcement active" + ))) +} + // --- identity / config resolution (impure; thin wrappers over env + git) --- /// `:` — same agent chain as handoff, plus an instance @@ -431,6 +466,43 @@ mod tests { assert!(claims[0].scope.is_empty()); } + #[test] + fn scope_clear_warning_fires_only_when_clearing_a_real_existing_scope() { + let c = mem(); + let scope = vec!["crates/foo/".to_string()]; + + // No existing claim yet -- nothing to clear. + assert!( + scope_clear_warning(&c, "o/r", "issue#1", None) + .unwrap() + .is_none() + ); + + acquire(&c, "o/r", "issue#1", "a:1", None, Some(&scope), 1000, TTL).unwrap(); + + // Re-declaring a real scope isn't a clear. + let other_scope = vec!["crates/bar/".to_string()]; + assert!( + scope_clear_warning(&c, "o/r", "issue#1", Some(&other_scope)) + .unwrap() + .is_none() + ); + + // Re-acquiring with no scope WOULD clear the existing one -- warn. + let warning = scope_clear_warning(&c, "o/r", "issue#1", None) + .unwrap() + .expect("clearing a declared scope must warn"); + assert!(warning.contains("crates/foo/"), "{warning}"); + + // Once cleared, re-checking with no scope is a no-op (nothing left to clear). + acquire(&c, "o/r", "issue#1", "a:1", None, None, 1000, TTL).unwrap(); + assert!( + scope_clear_warning(&c, "o/r", "issue#1", None) + .unwrap() + .is_none() + ); + } + #[test] fn list_defaults_missing_scope_to_empty() { let c = mem(); diff --git a/src/cli/claim.rs b/src/cli/claim.rs index 3e2752a0..58276bd7 100644 --- a/src/cli/claim.rs +++ b/src/cli/claim.rs @@ -153,6 +153,9 @@ fn acquire_cmd( let commit = if repo.is_none() { git_commit() } else { None }; let repo = require_repo(repo); let scope_arg = (!scope.is_empty()).then_some(scope.as_slice()); + let clear_warning = crate::claims::scope_clear_warning(conn, &repo, &target, scope_arg) + .ok() + .flatten(); match crate::claims::acquire( conn, &repo, @@ -165,7 +168,9 @@ fn acquire_cmd( ) { Ok(crate::claims::Acquire::Acquired) => { println!("claimed {repo} {target} (owner {owner})"); - if let Some(s) = scope_arg { + if let Some(warning) = clear_warning { + crate::ui::error(&format!("warning: {warning}")); + } else if let Some(s) = scope_arg { let warning = crate::claims::scope_overlap_warning(conn, &repo, &target, s, now, ttl); if let Ok(Some(warning)) = warning { diff --git a/src/cli/git.rs b/src/cli/git.rs index 836b9095..5f8fd559 100644 --- a/src/cli/git.rs +++ b/src/cli/git.rs @@ -578,19 +578,30 @@ fn run_scope_check(subcommand: &str) -> ScopeCheckResult { } /// Changed paths for the mutation about to happen -- staged paths for -/// `commit`, paths diffed against the default branch for `push`. A v1 -/// simplification for `push`: diffs current-vs-default rather than parsing -/// the actual push refspec across the CLI subprocess boundary. An -/// unreadable diff yields no changed paths (nothing to enforce), matching +/// `commit` (unioned with the working-tree diff, since `git commit -a`/ +/// `--all` implicitly stages+commits tracked modifications without a prior +/// `git add` -- checking `--cached` alone would let those paths bypass +/// scope enforcement entirely), paths diffed against the default branch for +/// `push`. A v1 simplification for `push`: diffs current-vs-default rather +/// than parsing the actual push refspec across the CLI subprocess boundary. +/// An unreadable diff yields no changed paths (nothing to enforce), matching /// this crate's fail-open default for diff resolution specifically -- only /// scope RESOLUTION errors (ledger/DB) are fail-closed, per the spec. fn changed_paths(repo_root: &Path, subcommand: &str) -> Vec { + if subcommand == "commit" { + let mut paths: Vec = shell::run_in(repo_root, &["diff", "--cached", "--name-only"]) + .map(|s| s.lines().map(String::from).collect()) + .unwrap_or_default(); + paths.extend( + shell::run_in(repo_root, &["diff", "--name-only"]) + .map(|s| s.lines().map(String::from).collect::>()) + .unwrap_or_default(), + ); + paths.sort(); + paths.dedup(); + return paths; + } let range_args: Vec = match subcommand { - "commit" => vec![ - "diff".to_string(), - "--cached".to_string(), - "--name-only".to_string(), - ], "push" => { let default_branch = branch::resolve_default_branch(repo_root); let current = @@ -605,3 +616,67 @@ fn changed_paths(repo_root: &Path, subcommand: &str) -> Vec { .map(|s| s.lines().map(String::from).collect()) .unwrap_or_default() } + +#[cfg(test)] +mod tests { + use super::*; + + fn run_git(dir: &Path, args: &[&str]) { + let out = std::process::Command::new("git") + .args(args) + .current_dir(dir) + .output() + .unwrap(); + assert!( + out.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&out.stderr) + ); + } + + fn init_repo() -> tempfile::TempDir { + let dir = tempfile::tempdir().unwrap(); + run_git(dir.path(), &["init", "-q", "-b", "master"]); + run_git(dir.path(), &["config", "user.email", "t@t"]); + run_git(dir.path(), &["config", "user.name", "t"]); + std::fs::write(dir.path().join("tracked.txt"), "v1\n").unwrap(); + run_git(dir.path(), &["add", "tracked.txt"]); + run_git(dir.path(), &["commit", "-q", "-m", "seed"]); + dir + } + + #[test] + fn changed_paths_for_commit_includes_unstaged_modifications_not_just_staged() { + // Regression for the CodeRabbit-flagged bypass on PR #303: `git + // commit -a`/`--all` implicitly stages+commits tracked + // modifications without a prior `git add`, so checking only + // `--cached` would miss them and let the change bypass scope + // enforcement entirely. + let repo = init_repo(); + std::fs::write(repo.path().join("tracked.txt"), "v2\n").unwrap(); + let paths = changed_paths(repo.path(), "commit"); + assert!( + paths.iter().any(|p| p == "tracked.txt"), + "unstaged modification must be included: {paths:?}" + ); + } + + #[test] + fn changed_paths_for_commit_dedupes_staged_and_unstaged() { + let repo = init_repo(); + std::fs::write(repo.path().join("new.txt"), "x\n").unwrap(); + run_git(repo.path(), &["add", "new.txt"]); + let paths = changed_paths(repo.path(), "commit"); + assert_eq!( + paths.iter().filter(|p| *p == "new.txt").count(), + 1, + "{paths:?}" + ); + } + + #[test] + fn changed_paths_for_commit_is_empty_when_clean() { + let repo = init_repo(); + assert!(changed_paths(repo.path(), "commit").is_empty()); + } +} diff --git a/src/mcp_server/claim.rs b/src/mcp_server/claim.rs index d61693ef..36deb070 100644 --- a/src/mcp_server/claim.rs +++ b/src/mcp_server/claim.rs @@ -30,6 +30,10 @@ impl AgentflareMcp { Self::git_provenance().and_then(|g| g.commit) }; let scope_arg = (!req.scope.is_empty()).then_some(req.scope.as_slice()); + let clear_warning = + crate::claims::scope_clear_warning(&conn, &repo, &target, scope_arg) + .ok() + .flatten(); let outcome = crate::claims::acquire( &conn, &repo, @@ -43,17 +47,19 @@ impl AgentflareMcp { .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; Ok(match outcome { crate::claims::Acquire::Acquired => { - let scope_warning = scope_arg.and_then(|s| { - crate::claims::scope_overlap_warning( - &conn, - &repo, - &target, - s, - crate::claims::now(), - crate::claims::ttl_secs(), - ) - .ok() - .flatten() + let scope_warning = clear_warning.or_else(|| { + scope_arg.and_then(|s| { + crate::claims::scope_overlap_warning( + &conn, + &repo, + &target, + s, + crate::claims::now(), + crate::claims::ttl_secs(), + ) + .ok() + .flatten() + }) }); serde_json::json!({ "status": "acquired", "repo": repo, "target": target, "owner": owner, "scope_warning": scope_warning }) }