diff --git a/Cargo.lock b/Cargo.lock index 5c3918c7..df4f22f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1100,6 +1100,8 @@ dependencies = [ "serde", "serde_json", "tempfile", + "thiserror", + "toml", "walkdir", "which", ] diff --git a/crates/flare-git-core/Cargo.toml b/crates/flare-git-core/Cargo.toml index 3c628957..8756ef27 100644 --- a/crates/flare-git-core/Cargo.toml +++ b/crates/flare-git-core/Cargo.toml @@ -18,6 +18,8 @@ dirs = "6" agent-detector = "0.2.1" which = "6" fs2 = "0.4" +toml = "0.8" +thiserror = "2" [dev-dependencies] tempfile = "3" diff --git a/crates/flare-git-core/src/classify.rs b/crates/flare-git-core/src/classify.rs index d224d1af..7ea1c969 100644 --- a/crates/flare-git-core/src/classify.rs +++ b/crates/flare-git-core/src/classify.rs @@ -32,6 +32,7 @@ use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; use crate::branch::{current_branch, is_protected_branch, resolve_default_branch}; +use crate::policy_config::ResolvedGitShimPolicy; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum Disposition { @@ -51,7 +52,7 @@ pub struct Event { /// 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"]; +pub(crate) 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 -> @@ -156,7 +157,7 @@ const READ_ONLY_SUBCOMMANDS: &[&str] = &[ /// 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] = &[ +pub(crate) const ALLOWED_MUTATING_SUBCOMMANDS: &[&str] = &[ "add", "commit", "merge", @@ -173,7 +174,7 @@ const ALLOWED_MUTATING_SUBCOMMANDS: &[&str] = &[ /// 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] = &[ +pub(crate) const DENIED_PLUMBING_SUBCOMMANDS: &[&str] = &[ "read-tree", "update-index", "apply", @@ -214,13 +215,21 @@ pub fn classify_pure( default_branch: &str, trust_root_touch: &TrustRootTouch, push_targets_default_branch: bool, + policy: &ResolvedGitShimPolicy, ) -> Disposition { if READ_ONLY_SUBCOMMANDS.contains(&subcommand) - || ALLOWED_MUTATING_SUBCOMMANDS.contains(&subcommand) + || policy + .allowed_mutating_subcommands + .iter() + .any(|s| s.as_str() == subcommand) { return Disposition::Passthrough; } - if DENIED_PLUMBING_SUBCOMMANDS.contains(&subcommand) { + if policy + .denied_plumbing_subcommands + .iter() + .any(|s| s.as_str() == 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." @@ -344,17 +353,18 @@ pub enum TrustRootTouch { /// default to let through, but the caller shouldn't claim to know which /// path caused it. #[must_use] -pub fn resolve_trust_root_touch(repo_root: &Path, branch: &str, target: &str) -> TrustRootTouch { - let extra = extra_trust_root_paths_from_env(); +pub fn resolve_trust_root_touch( + repo_root: &Path, + branch: &str, + target: &str, + trust_root_paths: &[String], +) -> TrustRootTouch { let range = format!("{target}...{branch}"); match crate::shell::run_in(repo_root, &["diff", "--name-only", &range]) { Ok(names) => { let mut matched: Vec = names .lines() - .filter(|f| { - TRUST_ROOT_PATHS.iter().any(|p| f.starts_with(p)) - || extra.iter().any(|p| f.starts_with(p.as_str())) - }) + .filter(|f| trust_root_paths.iter().any(|p| f.starts_with(p.as_str()))) .map(str::to_string) .collect(); matched.sort(); @@ -415,6 +425,17 @@ pub fn classify_with_home( args: &[String], home: Option<&Path>, ) -> Event { + let policy = crate::policy_config::resolve(repo_root, home).unwrap_or_else(|e| { + eprintln!( + "WARNING: agentflare git-shim config at {} is invalid ({}) -- \ + using baseline policy only, no config-sourced additions applied. \ + Git operations are not blocked by this; fix the file to restore \ + your customizations.", + e.path.display(), + e.source + ); + ResolvedGitShimPolicy::baseline() + }); 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 @@ -424,7 +445,7 @@ pub fn classify_with_home( .flatten(); let trust_root_touch = pushed .as_deref() - .map(|b| resolve_trust_root_touch(repo_root, b, &default_branch)) + .map(|b| resolve_trust_root_touch(repo_root, b, &default_branch, &policy.trust_root_paths)) .unwrap_or(TrustRootTouch::Clean); let targets_default_branch = pushed .as_deref() @@ -435,6 +456,7 @@ pub fn classify_with_home( &default_branch, &trust_root_touch, targets_default_branch, + &policy, ); // Every deny above (protected-branch checkout/switch/delete/rename, // trust-root push, plumbing block, worktree) exists to protect agentflare's @@ -466,8 +488,16 @@ mod tests { #[test] fn read_only_subcommands_pass_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( - classify_pure("status", &[], "master", &TrustRootTouch::Clean, false), + classify_pure( + "status", + &[], + "master", + &TrustRootTouch::Clean, + false, + &policy + ), Disposition::Passthrough ); assert_eq!( @@ -476,7 +506,8 @@ mod tests { &args(&["-5"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -484,13 +515,15 @@ mod tests { #[test] fn ordinary_mutating_subcommands_pass_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "commit", &args(&["-m", "x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -500,7 +533,8 @@ mod tests { &args(&["HEAD~1"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -508,6 +542,7 @@ mod tests { #[test] fn unknown_subcommand_passes_through_by_default() { + let policy = ResolvedGitShimPolicy::baseline(); // Fail-open: this shim must never block a subcommand it hasn't // been explicitly taught to deny. assert_eq!( @@ -516,7 +551,8 @@ mod tests { &[], "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -526,7 +562,8 @@ mod tests { &args(&["update"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -536,7 +573,8 @@ mod tests { &args(&["start"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -546,7 +584,8 @@ mod tests { &args(&["pull"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -554,25 +593,42 @@ mod tests { #[test] fn plumbing_commands_are_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( - classify_pure("update-index", &[], "master", &TrustRootTouch::Clean, false), + classify_pure( + "update-index", + &[], + "master", + &TrustRootTouch::Clean, + false, + &policy + ), Disposition::Deny { .. } )); assert!(matches!( - classify_pure("apply", &[], "master", &TrustRootTouch::Clean, false), + classify_pure( + "apply", + &[], + "master", + &TrustRootTouch::Clean, + false, + &policy + ), Disposition::Deny { .. } )); } #[test] fn worktree_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( classify_pure( "worktree", &args(&["add", "../x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Deny { .. } )); @@ -580,13 +636,15 @@ mod tests { #[test] fn worktree_remove_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( classify_pure( "worktree", &args(&["remove", "../x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Deny { .. } )); @@ -594,13 +652,15 @@ mod tests { #[test] fn worktree_list_is_passthrough() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "worktree", &args(&["list"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -608,13 +668,15 @@ mod tests { #[test] fn worktree_prune_dry_run_is_passthrough() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "worktree", &args(&["prune", "--dry-run"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -622,13 +684,15 @@ mod tests { #[test] fn worktree_prune_without_dry_run_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( classify_pure( "worktree", &args(&["prune"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Deny { .. } )); @@ -636,25 +700,29 @@ mod tests { #[test] fn checkout_to_protected_branch_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); let d = classify_pure( "checkout", &args(&["master"]), "master", &TrustRootTouch::Clean, false, + &policy, ); assert!(matches!(d, Disposition::Deny { .. })); } #[test] fn switch_to_feature_branch_passes_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "switch", &args(&["feature/x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -662,6 +730,7 @@ mod tests { #[test] fn checkout_with_no_target_arg_passes_through() { + let policy = ResolvedGitShimPolicy::baseline(); // `git switch -` (previous branch) — nothing to protect against. assert_eq!( classify_pure( @@ -669,7 +738,8 @@ mod tests { &args(&["-"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -677,6 +747,7 @@ mod tests { #[test] fn push_touching_trust_root_on_feature_branch_passes_through() { + let policy = ResolvedGitShimPolicy::baseline(); // A PR-review gate still applies before this reaches the default // branch — same reasoning as any other feature-branch push. assert_eq!( @@ -685,7 +756,8 @@ mod tests { &args(&["origin", "feature/x"]), "master", &TrustRootTouch::Touched(vec!["Cargo.toml".to_string()]), - false + false, + &policy ), Disposition::Passthrough ); @@ -693,13 +765,15 @@ mod tests { #[test] fn push_touching_trust_root_on_default_branch_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( classify_pure( "push", &args(&["origin", "master"]), "master", &TrustRootTouch::Touched(vec!["Cargo.toml".to_string()]), - true + true, + &policy ), Disposition::Deny { .. } )); @@ -707,13 +781,15 @@ mod tests { #[test] fn push_not_touching_trust_root_passes_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "push", &args(&["origin", "feature/x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -721,6 +797,7 @@ mod tests { #[test] fn push_of_default_branch_is_denied_even_without_trust_root_changes() { + let policy = ResolvedGitShimPolicy::baseline(); // Enforce PR-only: pushing the default branch straight to a remote is // blocked regardless of what the diff touches. assert!(matches!( @@ -729,7 +806,8 @@ mod tests { &args(&["origin", "master"]), "master", &TrustRootTouch::Clean, - true + true, + &policy ), Disposition::Deny { .. } )); @@ -737,13 +815,15 @@ mod tests { #[test] fn push_of_feature_branch_is_not_a_default_branch_push() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "push", &args(&["origin", "feature/x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -751,13 +831,15 @@ mod tests { #[test] fn branch_delete_of_protected_branch_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( classify_pure( "branch", &args(&["-D", "master"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Deny { .. } )); @@ -767,7 +849,8 @@ mod tests { &args(&["--delete", "master"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Deny { .. } )); @@ -775,13 +858,15 @@ mod tests { #[test] fn branch_rename_of_protected_branch_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( classify_pure( "branch", &args(&["-M", "master", "renamed"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Deny { .. } )); @@ -789,13 +874,15 @@ mod tests { #[test] fn branch_delete_of_feature_branch_passes_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "branch", &args(&["-D", "feature/x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -803,8 +890,16 @@ mod tests { #[test] fn branch_listing_and_creation_pass_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( - classify_pure("branch", &[], "master", &TrustRootTouch::Clean, false), + classify_pure( + "branch", + &[], + "master", + &TrustRootTouch::Clean, + false, + &policy + ), Disposition::Passthrough ); assert_eq!( @@ -813,7 +908,8 @@ mod tests { &args(&["feature/new"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -1035,8 +1131,16 @@ mod tests { #[test] fn push_trust_root_deny_message_names_only_the_touched_path() { + let policy = ResolvedGitShimPolicy::baseline(); let touch = TrustRootTouch::Touched(vec!["Cargo.toml".to_string()]); - let d = classify_pure("push", &args(&["origin", "master"]), "master", &touch, true); + let d = classify_pure( + "push", + &args(&["origin", "master"]), + "master", + &touch, + true, + &policy, + ); let Disposition::Deny { reason } = d else { panic!("expected Deny, got {d:?}"); }; @@ -1053,12 +1157,14 @@ mod tests { #[test] fn push_with_unreadable_diff_on_default_branch_denies_with_unknown_message() { + let policy = ResolvedGitShimPolicy::baseline(); let d = classify_pure( "push", &args(&["origin", "master"]), "master", &TrustRootTouch::Unknown, true, + &policy, ); let Disposition::Deny { reason } = d else { panic!("expected Deny, got {d:?}"); @@ -1068,15 +1174,70 @@ mod tests { #[test] fn push_with_unreadable_diff_on_feature_branch_passes_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "push", &args(&["origin", "feature/x"]), "master", &TrustRootTouch::Unknown, - false + false, + &policy ), Disposition::Passthrough ); } + + #[test] + fn malformed_project_local_config_falls_back_to_baseline_without_blocking_git() { + 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(); + std::fs::write( + repo.path.join(".agentflare").join("config.toml"), + "this is not valid toml [[[", + ) + .unwrap(); + + // An ordinary read-only command must still pass through -- a broken + // config file must never block git operations. + let event = classify(&repo.path, "status", &[]); + assert_eq!( + event.disposition, + Disposition::Passthrough, + "{:?}", + event.disposition + ); + } + + #[test] + fn project_local_config_can_relax_a_denied_plumbing_subcommand() { + 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(); + + // Baseline: "apply" is in DENIED_PLUMBING_SUBCOMMANDS. + let before = classify(&repo.path, "apply", &["patch.diff".to_string()]); + assert!( + matches!(before.disposition, Disposition::Deny { .. }), + "{:?}", + before.disposition + ); + + // Project-local config explicitly allows it. ALLOWED_MUTATING is + // checked before DENIED_PLUMBING in classify_pure, so this relaxes it. + std::fs::write( + repo.path.join(".agentflare").join("config.toml"), + "[git_shim]\nextra_allowed_mutating_subcommands = [\"apply\"]\n", + ) + .unwrap(); + + let after = classify(&repo.path, "apply", &["patch.diff".to_string()]); + assert_eq!( + after.disposition, + Disposition::Passthrough, + "{:?}", + after.disposition + ); + } } diff --git a/crates/flare-git-core/src/config_loader.rs b/crates/flare-git-core/src/config_loader.rs new file mode 100644 index 00000000..52100a1f --- /dev/null +++ b/crates/flare-git-core/src/config_loader.rs @@ -0,0 +1,101 @@ +use std::path::{Path, PathBuf}; + +#[derive(Debug, Default)] +pub struct ConfigLayers { + pub project_local: Option<(PathBuf, toml::Value)>, + pub user_home: Option<(PathBuf, toml::Value)>, +} + +#[derive(Debug, thiserror::Error)] +#[error("{path}: {source}")] +pub struct LoaderError { + pub path: PathBuf, + #[source] + pub source: Box, +} + +fn parse_if_exists(path: &Path) -> Result, LoaderError> { + let Ok(contents) = std::fs::read_to_string(path) else { + return Ok(None); + }; + toml::from_str(&contents) + .map(|v| Some((path.to_path_buf(), v))) + .map_err(|source| LoaderError { + path: path.to_path_buf(), + source: Box::new(source), + }) +} + +pub fn locate_and_parse( + repo_root: &Path, + home: Option<&Path>, +) -> Result { + let project_local = parse_if_exists(&repo_root.join(".agentflare").join("config.toml"))?; + let user_home = match home { + Some(h) => parse_if_exists(&h.join(".agentflare").join("config.toml"))?, + None => None, + }; + Ok(ConfigLayers { + project_local, + user_home, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn missing_files_return_none_layers() { + let repo = tempfile::tempdir().unwrap(); + let home = tempfile::tempdir().unwrap(); + let layers = locate_and_parse(repo.path(), Some(home.path())).unwrap(); + assert!(layers.project_local.is_none()); + assert!(layers.user_home.is_none()); + } + + #[test] + fn parses_project_local_file() { + let repo = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(repo.path().join(".agentflare")).unwrap(); + std::fs::write( + repo.path().join(".agentflare").join("config.toml"), + "[git_shim]\nextra_trust_root_paths = [\"x\"]\n", + ) + .unwrap(); + let layers = locate_and_parse(repo.path(), None).unwrap(); + let (path, value) = layers.project_local.expect("expected project_local layer"); + assert_eq!(path, repo.path().join(".agentflare").join("config.toml")); + assert_eq!( + value + .get("git_shim") + .and_then(|g| g.get("extra_trust_root_paths")), + Some(&toml::Value::Array(vec![toml::Value::String("x".into())])) + ); + } + + #[test] + fn parses_user_home_file() { + let repo = tempfile::tempdir().unwrap(); + let home = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(home.path().join(".agentflare")).unwrap(); + std::fs::write( + home.path().join(".agentflare").join("config.toml"), + "[git_shim]\nextra_trust_root_paths = [\"y\"]\n", + ) + .unwrap(); + let layers = locate_and_parse(repo.path(), Some(home.path())).unwrap(); + assert!(layers.user_home.is_some()); + assert!(layers.project_local.is_none()); + } + + #[test] + fn malformed_toml_returns_error_naming_the_file() { + let repo = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(repo.path().join(".agentflare")).unwrap(); + let bad_path = repo.path().join(".agentflare").join("config.toml"); + std::fs::write(&bad_path, "this is not valid toml [[[").unwrap(); + let err = locate_and_parse(repo.path(), None).unwrap_err(); + assert_eq!(err.path, bad_path); + } +} diff --git a/crates/flare-git-core/src/lib.rs b/crates/flare-git-core/src/lib.rs index 13539537..d6ee8c16 100644 --- a/crates/flare-git-core/src/lib.rs +++ b/crates/flare-git-core/src/lib.rs @@ -7,7 +7,9 @@ pub mod audit; pub mod branch; pub mod classify; +pub mod config_loader; pub mod doctor; +pub mod policy_config; pub mod provenance; pub mod scope; pub mod shell; diff --git a/crates/flare-git-core/src/policy_config.rs b/crates/flare-git-core/src/policy_config.rs new file mode 100644 index 00000000..0b340d1e --- /dev/null +++ b/crates/flare-git-core/src/policy_config.rs @@ -0,0 +1,156 @@ +use std::path::{Path, PathBuf}; + +use serde::Deserialize; + +use crate::classify::{ + ALLOWED_MUTATING_SUBCOMMANDS, DENIED_PLUMBING_SUBCOMMANDS, TRUST_ROOT_PATHS, + extra_trust_root_paths_from_env, +}; +use crate::config_loader::{self, LoaderError}; + +#[derive(Debug, Default, Deserialize)] +struct ConfigFile { + #[serde(default)] + git_shim: GitShimConfig, +} + +#[derive(Debug, Default, Deserialize)] +struct GitShimConfig { + #[serde(default)] + extra_trust_root_paths: Vec, + #[serde(default)] + extra_allowed_mutating_subcommands: Vec, + #[serde(default)] + extra_denied_plumbing_subcommands: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ResolvedGitShimPolicy { + pub trust_root_paths: Vec, + pub allowed_mutating_subcommands: Vec, + pub denied_plumbing_subcommands: Vec, +} + +impl ResolvedGitShimPolicy { + #[must_use] + pub fn baseline() -> Self { + Self { + trust_root_paths: unioned(TRUST_ROOT_PATHS, [&extra_trust_root_paths_from_env()]), + allowed_mutating_subcommands: ALLOWED_MUTATING_SUBCOMMANDS + .iter() + .map(|s| (*s).to_string()) + .collect(), + denied_plumbing_subcommands: DENIED_PLUMBING_SUBCOMMANDS + .iter() + .map(|s| (*s).to_string()) + .collect(), + } + } +} + +fn unioned(baseline: &[&str], extra_layers: [&Vec; N]) -> Vec { + let mut out: Vec = baseline.iter().map(|s| (*s).to_string()).collect(); + for layer in extra_layers { + for item in layer { + if !out.contains(item) { + out.push(item.clone()); + } + } + } + out +} + +fn parse_git_shim(layer: Option<(PathBuf, toml::Value)>) -> Result { + let Some((path, value)) = layer else { + return Ok(GitShimConfig::default()); + }; + ConfigFile::deserialize(value) + .map(|f| f.git_shim) + .map_err(|source| LoaderError { + path, + source: Box::new(source), + }) +} + +pub fn resolve( + repo_root: &Path, + home: Option<&Path>, +) -> Result { + let layers = config_loader::locate_and_parse(repo_root, home)?; + let project_local = parse_git_shim(layers.project_local)?; + let user_home = parse_git_shim(layers.user_home)?; + + Ok(ResolvedGitShimPolicy { + trust_root_paths: unioned( + TRUST_ROOT_PATHS, + [ + &project_local.extra_trust_root_paths, + &user_home.extra_trust_root_paths, + &extra_trust_root_paths_from_env(), + ], + ), + allowed_mutating_subcommands: unioned( + ALLOWED_MUTATING_SUBCOMMANDS, + [ + &project_local.extra_allowed_mutating_subcommands, + &user_home.extra_allowed_mutating_subcommands, + ], + ), + denied_plumbing_subcommands: unioned( + DENIED_PLUMBING_SUBCOMMANDS, + [ + &project_local.extra_denied_plumbing_subcommands, + &user_home.extra_denied_plumbing_subcommands, + ], + ), + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn no_files_no_env_resolves_to_baseline() { + let repo = tempfile::tempdir().unwrap(); + let home = tempfile::tempdir().unwrap(); + let resolved = resolve(repo.path(), Some(home.path())).unwrap(); + assert_eq!(resolved, ResolvedGitShimPolicy::baseline()); + } + + #[test] + fn project_local_and_user_home_union_and_dedup() { + let repo = tempfile::tempdir().unwrap(); + let home = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(repo.path().join(".agentflare")).unwrap(); + std::fs::write( + repo.path().join(".agentflare").join("config.toml"), + "[git_shim]\nextra_trust_root_paths = [\"proj/\"]\n", + ) + .unwrap(); + std::fs::create_dir_all(home.path().join(".agentflare")).unwrap(); + std::fs::write( + home.path().join(".agentflare").join("config.toml"), + "[git_shim]\nextra_trust_root_paths = [\"proj/\", \"home/\"]\n", + ) + .unwrap(); + + let resolved = resolve(repo.path(), Some(home.path())).unwrap(); + let mut expected = ResolvedGitShimPolicy::baseline().trust_root_paths; + expected.push("proj/".to_string()); + expected.push("home/".to_string()); + assert_eq!(resolved.trust_root_paths, expected); + } + + #[test] + fn malformed_config_returns_error() { + let repo = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(repo.path().join(".agentflare")).unwrap(); + std::fs::write( + repo.path().join(".agentflare").join("config.toml"), + "not valid toml [[[", + ) + .unwrap(); + assert!(resolve(repo.path(), None).is_err()); + } +} diff --git a/src/paths.rs b/src/paths.rs index 069a6b7c..a024219c 100644 --- a/src/paths.rs +++ b/src/paths.rs @@ -105,34 +105,121 @@ pub(crate) mod test_support { // set_var("AGENTFLARE_HOME_OVERRIDE") race a set_var("PATH") on another // thread — exactly the UB set_var is unsafe for. use agent_registry::detect::PATH_LOCK as GLOBAL_STATE_LOCK; + use std::path::PathBuf; + + // Removes AGENTFLARE_HOME_OVERRIDE on drop -- including on unwind, so a + // panicking assertion inside `f()` can't leave the override set for + // whatever test runs next on another thread once GLOBAL_STATE_LOCK is + // released (poisoned-mutex recovery only protects the lock itself, not + // env state a previous holder forgot to restore). + struct ResetHomeOverrideOnDrop; + impl Drop for ResetHomeOverrideOnDrop { + fn drop(&mut self) { + unsafe { + // SAFETY: still under GLOBAL_STATE_LOCK for the duration of + // this guard's life. + std::env::remove_var("AGENTFLARE_HOME_OVERRIDE"); + } + } + } pub(crate) fn with_temp_home(f: impl FnOnce() -> T) -> T { let _guard = GLOBAL_STATE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - let dir = std::env::temp_dir().join("agentflare-test-home"); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).unwrap(); + // A fresh, uniquely-named directory per call -- not a fixed shared + // name -- so a previous call's leftover file handle (e.g. a SQLite + // -wal/-shm file Windows hasn't released yet) can never leak into + // the next call even if that previous directory hasn't finished + // being cleaned up. See git history for the shared-fixed-name bug + // this replaced (state.rs/vent::capture.rs tests intermittently + // observed each other's persisted state under parallel execution). + let dir = tempfile::tempdir().unwrap(); unsafe { // SAFETY: GLOBAL_STATE_LOCK mutex serializes all env mutations; // no other thread can read or write AGENTFLARE_HOME_OVERRIDE concurrently. - std::env::set_var("AGENTFLARE_HOME_OVERRIDE", &dir) - }; - let result = f(); - unsafe { - // SAFETY: GLOBAL_STATE_LOCK mutex serializes all env mutations. - std::env::remove_var("AGENTFLARE_HOME_OVERRIDE") + std::env::set_var("AGENTFLARE_HOME_OVERRIDE", dir.path()) }; - result + let _reset = ResetHomeOverrideOnDrop; + f() + } + + // Restores the original cwd on drop -- including on unwind, same + // reasoning as ResetHomeOverrideOnDrop above. + struct RestoreCwdOnDrop(PathBuf); + impl Drop for RestoreCwdOnDrop { + fn drop(&mut self) { + let _ = std::env::set_current_dir(&self.0); + } } pub(crate) fn with_temp_cwd(f: impl FnOnce() -> T) -> T { let _guard = GLOBAL_STATE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - let dir = std::env::temp_dir().join("agentflare-test-cwd"); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).unwrap(); + // Same reasoning as with_temp_home above: a unique dir per call + // instead of a fixed shared name. + let temp_dir = tempfile::tempdir().unwrap(); let original = std::env::current_dir().unwrap(); - std::env::set_current_dir(&dir).unwrap(); - let result = f(); - std::env::set_current_dir(&original).unwrap(); - result + std::env::set_current_dir(temp_dir.path()).unwrap(); + let _restore = RestoreCwdOnDrop(original); + f() + } +} + +#[cfg(test)] +mod tests { + use super::test_support::with_temp_home; + + // Regression test for a real Windows CI flake (state::tests::* and + // vent::capture::tests::* intermittently observed each other's + // persisted state under `cargo test --workspace`'s default parallel + // runner): with_temp_home used a single fixed directory name shared by + // every call, so a previous call's file (left behind if e.g. Windows + // hadn't yet released a SQLite -wal/-shm handle) could still be present + // when the next call's directory was supposed to be empty. + #[test] + fn with_temp_home_never_sees_a_previous_calls_leftover_file() { + for i in 0..20 { + with_temp_home(|| { + let marker = super::home().join("marker.txt"); + assert!( + !marker.exists(), + "iteration {i}: found a marker file left behind by a previous with_temp_home call at {}", + super::home().display() + ); + std::fs::write(&marker, "left behind on purpose").unwrap(); + }); + } + } + + // Same check under real thread contention -- GLOBAL_STATE_LOCK forces + // these to run one at a time, but back-to-back-under-contention is + // exactly the timing the original shared-fixed-directory bug needed to + // show up under Windows' delayed file-handle release. + #[test] + fn with_temp_home_isolates_calls_under_thread_contention() { + std::thread::scope(|scope| { + for _ in 0..8 { + scope.spawn(|| { + for _ in 0..20 { + with_temp_home(|| { + let marker = super::home().join("marker.txt"); + assert!( + !marker.exists(), + "found a marker file left behind by another with_temp_home call at {}", + super::home().display() + ); + std::fs::write(&marker, "left behind on purpose").unwrap(); + }); + } + }); + } + }); + } + + #[test] + fn with_temp_home_clears_the_override_env_var_after_returning() { + with_temp_home(|| {}); + assert!( + std::env::var("AGENTFLARE_HOME_OVERRIDE").is_err(), + "AGENTFLARE_HOME_OVERRIDE must not remain set once with_temp_home returns" + ); } }