From 66af6bba1ef4062a1384b107def2425a0bd1e1f0 Mon Sep 17 00:00:00 2001 From: Sasha Varlamov Date: Sun, 2 Aug 2026 23:10:26 +0000 Subject: [PATCH 1/4] perf(daemon): ignore temporary repos without remotes --- src/commands/git_ai_handlers.rs | 30 ++++-- src/daemon.rs | 158 ++++++++++++++++++++++++++++++++ src/git/repository.rs | 30 ++++++ tests/daemon_mode.rs | 79 ++++++++++++++++ 4 files changed, 288 insertions(+), 9 deletions(-) diff --git a/src/commands/git_ai_handlers.rs b/src/commands/git_ai_handlers.rs index a8afe91e9f..a8417273f3 100644 --- a/src/commands/git_ai_handlers.rs +++ b/src/commands/git_ai_handlers.rs @@ -498,13 +498,14 @@ fn handle_checkpoint(args: &[String]) { } } - // Check repository allowlist before sending to daemon. - // Skip entirely when no allow/exclude filters are configured (common case) - // to avoid spawning a `git remote -v` subprocess. + // Skip disposable temporary repositories and enforce the repository allowlist before sending + // anything to the daemon. Repository discovery and config reads stay in the checkpoint process, + // outside the trace2 ingestion path. let t_allowlist = std::time::Instant::now(); { let config = config::Config::get(); - if config.has_repository_filters() { + let filter_temporary_repos = crate::git::repository::temporary_repo_filter_enabled(); + if filter_temporary_repos || config.has_repository_filters() { let mut checked_repos = std::collections::HashSet::new(); for request in &requests { for file in &request.files { @@ -513,12 +514,23 @@ fn handle_checkpoint(args: &[String]) { crate::git::repository::discover_repository_in_path_no_git_exec( &file.repo_work_dir, ) - && !config.is_allowed_repository(&Some(repo)) { - eprintln!( - "Skipping checkpoint because repository is excluded or not in allow_repositories list" - ); - std::process::exit(0); + if filter_temporary_repos + && repo.is_temporary_without_remote_url().unwrap_or(false) + { + eprintln!( + "Skipping checkpoint for temporary repository without a remote URL" + ); + std::process::exit(0); + } + if config.has_repository_filters() + && !config.is_allowed_repository(&Some(repo)) + { + eprintln!( + "Skipping checkpoint because repository is excluded or not in allow_repositories list" + ); + std::process::exit(0); + } } } } diff --git a/src/daemon.rs b/src/daemon.rs index 774d4162e4..e3cee6a36a 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -106,6 +106,7 @@ const CHECKPOINT_FAMILY_DRAIN_CONCURRENCY: usize = 2; #[cfg(not(windows))] const TRACE_SOCKET_RECV_BUFFER_BYTES: usize = 512 * 1024; const TRACE_INGEST_QUEUE_CAPACITY: usize = 16_384; +const TEMP_REPO_FILTER_CACHE_CAPACITY: usize = 1_024; #[cfg(not(windows))] const TRACE_CONNECTION_BOOTSTRAP_READ_TIMEOUT: Duration = Duration::from_millis(100); #[cfg(windows)] @@ -478,6 +479,25 @@ fn is_terminal_root_trace_event(event: &str, sid: &str, root: &str) -> bool { sid == root && event == "atexit" } +fn trace_payload_explicit_worktree(payload: &Value) -> Option { + payload + .get(TRACE_ROOT_WORKTREE_FIELD) + .or_else(|| payload.get("worktree")) + .or_else(|| payload.get("repo_working_dir")) + .and_then(Value::as_str) + .map(PathBuf::from) +} + +fn trace_payload_may_change_remote_config(payload: &Value) -> bool { + let argv = trace_payload_effective_argv(payload); + matches!( + trace_payload_primary_command(payload) + .or_else(|| trace_argv_primary_command(&argv)) + .as_deref(), + Some("config" | "remote") + ) +} + fn daemon_worktree_from_repo_path(repo_path: &Path) -> Option { if repo_path.file_name().and_then(|name| name.to_str()) == Some(".git") { return repo_path.parent().map(PathBuf::from); @@ -2720,6 +2740,13 @@ struct TraceIngressState { root_close_markers_enqueued: HashSet, } +#[derive(Debug, Clone)] +struct TraceRepoFilterRoot { + repo_path: PathBuf, + ignored: bool, + invalidates_cache: bool, +} + #[doc(hidden)] pub struct ActorDaemonCoordinator { backend: Arc, @@ -2770,6 +2797,8 @@ pub struct ActorDaemonCoordinator { processed_trace_ingest_seq: AtomicUsize, trace_ingest_progress_notify: Notify, trace_ingress_state: Mutex, + temp_repo_filter_cache: Mutex>, + temp_repo_filter_roots: Mutex>, shutting_down: AtomicBool, shutdown_action: AtomicU8, shutdown_notify: Notify, @@ -2872,6 +2901,8 @@ impl ActorDaemonCoordinator { processed_trace_ingest_seq: AtomicUsize::new(0), trace_ingest_progress_notify: Notify::new(), trace_ingress_state: Mutex::new(TraceIngressState::default()), + temp_repo_filter_cache: Mutex::new(HashMap::new()), + temp_repo_filter_roots: Mutex::new(HashMap::new()), shutting_down: AtomicBool::new(false), shutdown_action: AtomicU8::new(DaemonExitAction::Stop.as_u8()), shutdown_notify: Notify::new(), @@ -6473,10 +6504,137 @@ impl ActorDaemonCoordinator { Ok(outcome) } + async fn should_ignore_trace_payload(&self, payload: &Value) -> Result { + if !crate::git::repository::temporary_repo_filter_enabled() { + return Ok(false); + } + #[cfg(feature = "test-support")] + if trace_payload_effective_argv(payload) + .iter() + .any(|arg| arg == "git-ai.test-readiness-probe") + { + return Ok(false); + } + + let Some(root_sid) = Self::trace_payload_root_sid(payload) else { + return Ok(false); + }; + let event = payload + .get("event") + .and_then(Value::as_str) + .unwrap_or_default(); + let terminal = event == TRACE_CONNECTION_CLOSED_EVENT + || is_terminal_root_trace_event( + event, + payload + .get("sid") + .and_then(Value::as_str) + .unwrap_or_default(), + &root_sid, + ); + + let tracked_root = { + let mut roots = self.temp_repo_filter_roots.lock().map_err(|_| { + GitAiError::Generic("temporary repository root cache lock poisoned".to_string()) + })?; + if terminal { + roots.remove(&root_sid) + } else { + roots.get(&root_sid).cloned() + } + }; + if let Some(tracked_root) = tracked_root { + if terminal && tracked_root.invalidates_cache { + self.temp_repo_filter_cache + .lock() + .map_err(|_| { + GitAiError::Generic( + "temporary repository decision cache lock poisoned".to_string(), + ) + })? + .remove(&tracked_root.repo_path); + } + return Ok(tracked_root.ignored); + } + + if event != "def_repo" || crate::daemon::trace_normalizer::def_repo_is_secondary(payload) { + return Ok(false); + } + let Some(repo_path) = trace_payload_explicit_worktree(payload) else { + return Ok(false); + }; + + let cached = self + .temp_repo_filter_cache + .lock() + .map_err(|_| { + GitAiError::Generic("temporary repository decision cache lock poisoned".to_string()) + })? + .get(&repo_path) + .copied(); + let ignored = if let Some(ignored) = cached { + ignored + } else { + let repo_path_for_check = repo_path.clone(); + let ignored = crate::tokio_runtime::spawn_blocking_result(move || { + let repo = discover_repository_in_path_no_git_exec(&repo_path_for_check)?; + repo.is_temporary_without_remote_url() + }) + .await + .unwrap_or(false); + let mut cache = self.temp_repo_filter_cache.lock().map_err(|_| { + GitAiError::Generic("temporary repository decision cache lock poisoned".to_string()) + })?; + if cache.len() >= TEMP_REPO_FILTER_CACHE_CAPACITY + && let Some(evicted) = cache.keys().next().cloned() + { + cache.remove(&evicted); + } + cache.insert(repo_path.clone(), ignored); + ignored + }; + + let invalidates_cache = trace_payload_may_change_remote_config(payload); + if ignored || invalidates_cache { + self.temp_repo_filter_roots + .lock() + .map_err(|_| { + GitAiError::Generic("temporary repository root cache lock poisoned".to_string()) + })? + .insert( + root_sid.clone(), + TraceRepoFilterRoot { + repo_path, + ignored, + invalidates_cache, + }, + ); + } + + if ignored { + { + let mut normalizer = self.normalizer.lock().await; + let _ = normalizer.sweep_orphans_for_roots(std::slice::from_ref(&root_sid)); + } + if let Some(family) = self + .replace_pending_root_entry(&root_sid, FamilySequencerEntry::Canceled) + .await? + { + self.drain_ready_family_sequencers_after_root_cleared(Some(family)) + .await?; + } + } + + Ok(ignored) + } + async fn ingest_trace_payload_fast(self: Arc, payload: Value) -> Result<(), GitAiError> { if !is_trace_payload(&payload) { return Ok(()); } + if self.should_ignore_trace_payload(&payload).await? { + return Ok(()); + } match self.apply_trace_payload_to_state(payload).await? { TracePayloadApplyOutcome::None | TracePayloadApplyOutcome::QueuedFamily => {} TracePayloadApplyOutcome::Applied(applied) => { diff --git a/src/git/repository.rs b/src/git/repository.rs index 4ca4ec4a3b..33ac86dd10 100644 --- a/src/git/repository.rs +++ b/src/git/repository.rs @@ -17,6 +17,7 @@ use std::collections::{HashMap, HashSet}; use std::ffi::OsStr; use std::path::{Path, PathBuf}; use std::process::{Child, Command, Output}; +use std::sync::OnceLock; use std::sync::atomic::{AtomicUsize, Ordering}; #[cfg(windows)] @@ -31,6 +32,18 @@ thread_local! { } static INTERNAL_GIT_HOOKS_DISABLED_DEPTH_GLOBAL: AtomicUsize = AtomicUsize::new(0); +pub(crate) fn temporary_repo_filter_enabled() -> bool { + #[cfg(any(test, feature = "test-support"))] + { + std::env::var("GIT_AI_TEST_ENABLE_TEMP_REPO_FILTER").as_deref() == Ok("1") + } + + #[cfg(not(any(test, feature = "test-support")))] + { + true + } +} + pub struct InternalGitHooksGuard; impl Drop for InternalGitHooksGuard { @@ -1297,6 +1310,23 @@ impl Repository { Ok(remotes) } + /// Returns whether this repository lives under the system temporary directory and has no + /// configured remote URL. Reading remote configuration is deferred until after the cheap path + /// check so normal repositories do not incur any additional config I/O. + pub fn is_temporary_without_remote_url(&self) -> Result { + static TEMP_DIR: OnceLock = OnceLock::new(); + + let temp_dir = TEMP_DIR.get_or_init(|| { + let path = std::env::temp_dir(); + path.canonicalize().unwrap_or(path) + }); + if !self.canonical_workdir.starts_with(temp_dir) { + return Ok(false); + } + + Ok(self.remotes_with_urls()?.is_empty()) + } + fn load_optional_config_file( path: &Path, source: gix_config::Source, diff --git a/tests/daemon_mode.rs b/tests/daemon_mode.rs index 98cc816836..0c2736d2a0 100644 --- a/tests/daemon_mode.rs +++ b/tests/daemon_mode.rs @@ -826,6 +826,85 @@ fn claude_fixture_path() -> PathBuf { .join("example-claude-code.jsonl") } +#[test] +fn daemon_ignores_temporary_repositories_until_they_have_a_remote_url() { + const ENABLE_TEMP_REPO_FILTER: (&str, &str) = ("GIT_AI_TEST_ENABLE_TEMP_REPO_FILTER", "1"); + + let repo = TestRepo::new_with_daemon_env(&[ENABLE_TEMP_REPO_FILTER]); + let file_path = repo.path().join("temporary-repo.txt"); + fs::write(&file_path, "ignored AI line\n").expect("failed to write ignored AI edit"); + + let checkpoint_output = repo + .git_ai_with_env( + &[ + "checkpoint", + "mock_ai", + file_path.to_str().expect("test path should be UTF-8"), + ], + &[ENABLE_TEMP_REPO_FILTER], + ) + .expect("ignored checkpoint should exit successfully"); + assert!( + checkpoint_output + .contains("Skipping checkpoint for temporary repository without a remote URL"), + "checkpoint should explain why it was skipped: {checkpoint_output}" + ); + + repo.git_without_test_sync_for_test(&["add", "temporary-repo.txt"], &[]) + .expect("staging ignored edit should succeed"); + repo.git_without_test_sync_for_test(&["commit", "-m", "Ignored temporary commit"], &[]) + .expect("committing ignored edit should succeed"); + repo.sync_daemon_force(); + + let mut file = repo.filename("temporary-repo.txt"); + file.assert_committed_lines(lines!["ignored AI line".unattributed_human()]); + assert!( + repo.daemon_completion_entries().iter().all(|entry| { + entry.primary_command.as_deref() != Some("commit") + && entry.primary_command.as_deref() != Some("checkpoint") + }), + "the daemon should not process the temporary local-only repository" + ); + + repo.git_without_test_sync_for_test( + &[ + "remote", + "add", + "origin", + "https://github.com/acme/temporary-repo.git", + ], + &[], + ) + .expect("adding a remote should succeed"); + repo.sync_daemon_force(); + + fs::write(&file_path, "ignored AI line\ntracked AI line\n") + .expect("failed to write tracked AI edit"); + let checkpoint_output = repo + .git_ai_with_env( + &[ + "checkpoint", + "mock_ai", + file_path.to_str().expect("test path should be UTF-8"), + ], + &[ENABLE_TEMP_REPO_FILTER], + ) + .expect("checkpoint should run after adding a remote"); + assert!( + checkpoint_output.contains("checkpoint_requests=1"), + "checkpoint should be sent after adding a remote: {checkpoint_output}" + ); + + repo.git(&["add", "temporary-repo.txt"]) + .expect("staging tracked edit should succeed"); + repo.commit("Tracked remote-backed commit") + .expect("committing tracked edit should succeed"); + file.assert_committed_lines(lines![ + "ignored AI line".unattributed_human(), + "tracked AI line".ai(), + ]); +} + fn assert_post_commit_uploads_prompt_cas() { let mock_api = MockApiServer::start(); let _api_base_url = ScopedEnvVar::set("GIT_AI_API_BASE_URL", mock_api.base_url()); From 5a594b6311a97c42bc336e65dd0345527bb0e95e Mon Sep 17 00:00:00 2001 From: Sasha Varlamov Date: Sun, 2 Aug 2026 23:18:21 +0000 Subject: [PATCH 2/4] test(e2e): configure remote for temp repositories --- .github/workflows/install-scripts-local.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/install-scripts-local.yml b/.github/workflows/install-scripts-local.yml index 3d0905946a..7118b4ecf1 100644 --- a/.github/workflows/install-scripts-local.yml +++ b/.github/workflows/install-scripts-local.yml @@ -127,6 +127,7 @@ jobs: git init git config user.email "e2e-test@example.com" git config user.name "E2E Test" + git remote add origin https://github.com/git-ai-project/git-ai.git echo "# E2E Test Repo" > README.md git add README.md # The proxy symlink doesn't exist yet, so 'git' here is still the real system git @@ -295,6 +296,7 @@ jobs: git init git config user.email "e2e-test@example.com" git config user.name "E2E Test" + git remote add origin https://github.com/git-ai-project/git-ai.git Set-Content -Path "README.md" -Value "# E2E Test Repo" git add README.md git commit -m "Initial commit" From acdb28605974b07e0f00c75bb77b8230cccaeddc Mon Sep 17 00:00:00 2001 From: Sasha Varlamov Date: Mon, 3 Aug 2026 02:25:32 +0000 Subject: [PATCH 3/4] fix(daemon): refresh ignored temporary repositories --- src/daemon.rs | 168 +++++++++++++++++++++++++++---------------- tests/daemon_mode.rs | 21 +++--- 2 files changed, 115 insertions(+), 74 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index e3cee6a36a..7646080ab4 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -106,7 +106,7 @@ const CHECKPOINT_FAMILY_DRAIN_CONCURRENCY: usize = 2; #[cfg(not(windows))] const TRACE_SOCKET_RECV_BUFFER_BYTES: usize = 512 * 1024; const TRACE_INGEST_QUEUE_CAPACITY: usize = 16_384; -const TEMP_REPO_FILTER_CACHE_CAPACITY: usize = 1_024; +const TEMP_REPO_FILTER_ALLOW_CACHE_CAPACITY: usize = 1_024; #[cfg(not(windows))] const TRACE_CONNECTION_BOOTSTRAP_READ_TIMEOUT: Duration = Duration::from_millis(100); #[cfg(windows)] @@ -488,16 +488,6 @@ fn trace_payload_explicit_worktree(payload: &Value) -> Option { .map(PathBuf::from) } -fn trace_payload_may_change_remote_config(payload: &Value) -> bool { - let argv = trace_payload_effective_argv(payload); - matches!( - trace_payload_primary_command(payload) - .or_else(|| trace_argv_primary_command(&argv)) - .as_deref(), - Some("config" | "remote") - ) -} - fn daemon_worktree_from_repo_path(repo_path: &Path) -> Option { if repo_path.file_name().and_then(|name| name.to_str()) == Some(".git") { return repo_path.parent().map(PathBuf::from); @@ -2740,13 +2730,6 @@ struct TraceIngressState { root_close_markers_enqueued: HashSet, } -#[derive(Debug, Clone)] -struct TraceRepoFilterRoot { - repo_path: PathBuf, - ignored: bool, - invalidates_cache: bool, -} - #[doc(hidden)] pub struct ActorDaemonCoordinator { backend: Arc, @@ -2797,8 +2780,8 @@ pub struct ActorDaemonCoordinator { processed_trace_ingest_seq: AtomicUsize, trace_ingest_progress_notify: Notify, trace_ingress_state: Mutex, - temp_repo_filter_cache: Mutex>, - temp_repo_filter_roots: Mutex>, + temp_repo_filter_allow_cache: Mutex>, + temp_repo_filter_roots: Mutex>, shutting_down: AtomicBool, shutdown_action: AtomicU8, shutdown_notify: Notify, @@ -2901,8 +2884,8 @@ impl ActorDaemonCoordinator { processed_trace_ingest_seq: AtomicUsize::new(0), trace_ingest_progress_notify: Notify::new(), trace_ingress_state: Mutex::new(TraceIngressState::default()), - temp_repo_filter_cache: Mutex::new(HashMap::new()), - temp_repo_filter_roots: Mutex::new(HashMap::new()), + temp_repo_filter_allow_cache: Mutex::new(HashSet::new()), + temp_repo_filter_roots: Mutex::new(HashSet::new()), shutting_down: AtomicBool::new(false), shutdown_action: AtomicU8::new(DaemonExitAction::Stop.as_u8()), shutdown_notify: Notify::new(), @@ -6533,28 +6516,21 @@ impl ActorDaemonCoordinator { &root_sid, ); - let tracked_root = { + let ignored_root = { let mut roots = self.temp_repo_filter_roots.lock().map_err(|_| { GitAiError::Generic("temporary repository root cache lock poisoned".to_string()) })?; if terminal { roots.remove(&root_sid) } else { - roots.get(&root_sid).cloned() + roots.contains(&root_sid) } }; - if let Some(tracked_root) = tracked_root { - if terminal && tracked_root.invalidates_cache { - self.temp_repo_filter_cache - .lock() - .map_err(|_| { - GitAiError::Generic( - "temporary repository decision cache lock poisoned".to_string(), - ) - })? - .remove(&tracked_root.repo_path); + if ignored_root { + if terminal { + self.clear_trace_root_tracking(&root_sid)?; } - return Ok(tracked_root.ignored); + return Ok(true); } if event != "def_repo" || crate::daemon::trace_normalizer::def_repo_is_secondary(payload) { @@ -6564,16 +6540,15 @@ impl ActorDaemonCoordinator { return Ok(false); }; - let cached = self - .temp_repo_filter_cache + let cached_allow = self + .temp_repo_filter_allow_cache .lock() .map_err(|_| { - GitAiError::Generic("temporary repository decision cache lock poisoned".to_string()) + GitAiError::Generic("temporary repository allow cache lock poisoned".to_string()) })? - .get(&repo_path) - .copied(); - let ignored = if let Some(ignored) = cached { - ignored + .contains(&repo_path); + let ignored = if cached_allow { + false } else { let repo_path_for_check = repo_path.clone(); let ignored = crate::tokio_runtime::spawn_blocking_result(move || { @@ -6582,36 +6557,29 @@ impl ActorDaemonCoordinator { }) .await .unwrap_or(false); - let mut cache = self.temp_repo_filter_cache.lock().map_err(|_| { - GitAiError::Generic("temporary repository decision cache lock poisoned".to_string()) - })?; - if cache.len() >= TEMP_REPO_FILTER_CACHE_CAPACITY - && let Some(evicted) = cache.keys().next().cloned() - { - cache.remove(&evicted); + if !ignored { + let mut cache = self.temp_repo_filter_allow_cache.lock().map_err(|_| { + GitAiError::Generic( + "temporary repository allow cache lock poisoned".to_string(), + ) + })?; + if cache.len() >= TEMP_REPO_FILTER_ALLOW_CACHE_CAPACITY + && let Some(evicted) = cache.iter().next().cloned() + { + cache.remove(&evicted); + } + cache.insert(repo_path.clone()); } - cache.insert(repo_path.clone(), ignored); ignored }; - let invalidates_cache = trace_payload_may_change_remote_config(payload); - if ignored || invalidates_cache { + if ignored { self.temp_repo_filter_roots .lock() .map_err(|_| { GitAiError::Generic("temporary repository root cache lock poisoned".to_string()) })? - .insert( - root_sid.clone(), - TraceRepoFilterRoot { - repo_path, - ignored, - invalidates_cache, - }, - ); - } - - if ignored { + .insert(root_sid.clone()); { let mut normalizer = self.normalizer.lock().await; let _ = normalizer.sweep_orphans_for_roots(std::slice::from_ref(&root_sid)); @@ -6623,6 +6591,8 @@ impl ActorDaemonCoordinator { self.drain_ready_family_sequencers_after_root_cleared(Some(family)) .await?; } + // Keep ingress connection state until atexit or the synthesized connection-close + // marker arrives. The terminal payload then clears all per-root bookkeeping above. } Ok(ignored) @@ -10118,6 +10088,78 @@ mod tests { coord.request_shutdown(); } + #[serial] + #[tokio::test] + async fn ignored_temp_repo_terminal_payload_clears_trace_tracking() { + let _filter = EnvVarGuard::set("GIT_AI_TEST_ENABLE_TEMP_REPO_FILTER", "1"); + let coord = ActorDaemonCoordinator::new(); + let temp = tempfile::tempdir().unwrap(); + let worktree = temp.path().join("repo"); + let init = std::process::Command::new("git") + .arg("-C") + .arg(temp.path()) + .args(["init", "repo"]) + .output() + .expect("git init should run"); + assert!( + init.status.success(), + "git init failed: {}", + String::from_utf8_lossy(&init.stderr) + ); + + let sid = "20260411T120000.000000-Psid-ignored-temp"; + coord.trace_root_connection_opened(sid).unwrap(); + let mut start = serde_json::json!({ + "event": "start", + "sid": sid, + "argv": ["git", "commit", "-m", "ignored commit"], + "worktree": worktree, + "time_ns": 1u64, + }); + assert!(coord.prepare_trace_payload_for_ingest(&mut start)); + coord.record_trace_payload_enqueued_root(Some(sid)).unwrap(); + + let def_repo = serde_json::json!({ + "event": "def_repo", + "sid": sid, + "repo": 1, + "worktree": worktree, + "time_ns": 2u64, + }); + assert!(coord.should_ignore_trace_payload(&def_repo).await.unwrap()); + + let close_marker_roots = coord + .record_trace_connection_close(&[sid.to_string()]) + .unwrap(); + assert_eq!(close_marker_roots, [sid.to_string()]); + coord.record_trace_payload_enqueued_root(Some(sid)).unwrap(); + let close_marker = serde_json::json!({ + "event": TRACE_CONNECTION_CLOSED_EVENT, + "sid": sid, + "time_ns": 3u64, + }); + assert!( + coord + .should_ignore_trace_payload(&close_marker) + .await + .unwrap() + ); + + let ingress = coord.trace_ingress_state.lock().unwrap(); + assert!(!ingress.root_argv.contains_key(sid)); + assert!(!ingress.root_open_connections.contains_key(sid)); + assert!(!ingress.root_close_markers_enqueued.contains(sid)); + drop(ingress); + assert!( + !coord + .queued_trace_payloads_by_root + .lock() + .unwrap() + .contains_key(sid) + ); + assert!(!coord.temp_repo_filter_roots.lock().unwrap().contains(sid)); + } + #[tokio::test] async fn readonly_trace_connection_close_without_atexit_clears_tracking() { let coord = ActorDaemonCoordinator::new(); diff --git a/tests/daemon_mode.rs b/tests/daemon_mode.rs index 0c2736d2a0..b23d0c1ca9 100644 --- a/tests/daemon_mode.rs +++ b/tests/daemon_mode.rs @@ -827,7 +827,7 @@ fn claude_fixture_path() -> PathBuf { } #[test] -fn daemon_ignores_temporary_repositories_until_they_have_a_remote_url() { +fn daemon_ignores_temporary_repositories_until_remote_config_changes_out_of_band() { const ENABLE_TEMP_REPO_FILTER: (&str, &str) = ("GIT_AI_TEST_ENABLE_TEMP_REPO_FILTER", "1"); let repo = TestRepo::new_with_daemon_env(&[ENABLE_TEMP_REPO_FILTER]); @@ -866,17 +866,16 @@ fn daemon_ignores_temporary_repositories_until_they_have_a_remote_url() { "the daemon should not process the temporary local-only repository" ); - repo.git_without_test_sync_for_test( - &[ - "remote", - "add", - "origin", - "https://github.com/acme/temporary-repo.git", - ], - &[], + let mut git_config = fs::OpenOptions::new() + .append(true) + .open(repo.path().join(".git/config")) + .expect("repository config should exist"); + writeln!( + git_config, + "\n[remote \"origin\"]\n\turl = https://github.com/acme/temporary-repo.git" ) - .expect("adding a remote should succeed"); - repo.sync_daemon_force(); + .expect("remote config should be written directly"); + git_config.flush().expect("remote config should be flushed"); fs::write(&file_path, "ignored AI line\ntracked AI line\n") .expect("failed to write tracked AI edit"); From fc637724106cf1133e9231471de261577a76a998 Mon Sep 17 00:00:00 2001 From: Sasha Varlamov Date: Mon, 3 Aug 2026 02:35:51 +0000 Subject: [PATCH 4/4] fix(daemon): close temp repo filter gaps --- .../workflows/nightly-agent-integration.yml | 2 + src/daemon.rs | 80 ++++++++++++------- 2 files changed, 55 insertions(+), 27 deletions(-) diff --git a/.github/workflows/nightly-agent-integration.yml b/.github/workflows/nightly-agent-integration.yml index 3894f1657d..d464e8e604 100644 --- a/.github/workflows/nightly-agent-integration.yml +++ b/.github/workflows/nightly-agent-integration.yml @@ -200,6 +200,7 @@ jobs: git init git config user.email "ci@git-ai.test" git config user.name "CI Test" + git remote add origin https://github.com/git-ai-project/git-ai.git echo "# Integration Test Repo" > README.md git add README.md git commit -m "Initial commit" @@ -298,6 +299,7 @@ jobs: git init git config user.email "ci@git-ai.test" git config user.name "CI Test" + git remote add origin https://github.com/git-ai-project/git-ai.git echo "# Integration Test Repo" > README.md git add README.md git commit -m "Initial commit" diff --git a/src/daemon.rs b/src/daemon.rs index 7646080ab4..e70c8bc064 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -2725,6 +2725,9 @@ struct TraceIngressState { /// Roots whose start event was identified as definitely read-only. All /// subsequent events for these roots (including exit) take the fast path. root_definitely_read_only: HashSet, + /// Roots suppressed by the temporary-repository filter. Keeping this with the other ingress + /// state lets connection-close handling synthesize a terminal marker before clearing it. + root_ignored_temp_repositories: HashSet, root_open_connections: HashMap, unidentified_open_connections: usize, root_close_markers_enqueued: HashSet, @@ -2781,7 +2784,6 @@ pub struct ActorDaemonCoordinator { trace_ingest_progress_notify: Notify, trace_ingress_state: Mutex, temp_repo_filter_allow_cache: Mutex>, - temp_repo_filter_roots: Mutex>, shutting_down: AtomicBool, shutdown_action: AtomicU8, shutdown_notify: Notify, @@ -2885,7 +2887,6 @@ impl ActorDaemonCoordinator { trace_ingest_progress_notify: Notify::new(), trace_ingress_state: Mutex::new(TraceIngressState::default()), temp_repo_filter_allow_cache: Mutex::new(HashSet::new()), - temp_repo_filter_roots: Mutex::new(HashSet::new()), shutting_down: AtomicBool::new(false), shutdown_action: AtomicU8::new(DaemonExitAction::Stop.as_u8()), shutdown_notify: Notify::new(), @@ -3656,6 +3657,9 @@ impl ActorDaemonCoordinator { } fn trace_root_needs_close_marker(ingress: &TraceIngressState, root_sid: &str) -> bool { + if ingress.root_ignored_temp_repositories.contains(root_sid) { + return true; + } if ingress.root_definitely_read_only.contains(root_sid) { return false; } @@ -3677,6 +3681,7 @@ impl ActorDaemonCoordinator { ingress.root_target_repo_only.remove(root_sid); ingress.root_last_activity_ns.remove(root_sid); ingress.root_definitely_read_only.remove(root_sid); + ingress.root_ignored_temp_repositories.remove(root_sid); ingress.root_open_connections.remove(root_sid); ingress.root_close_markers_enqueued.remove(root_sid); } @@ -6517,14 +6522,10 @@ impl ActorDaemonCoordinator { ); let ignored_root = { - let mut roots = self.temp_repo_filter_roots.lock().map_err(|_| { - GitAiError::Generic("temporary repository root cache lock poisoned".to_string()) + let ingress = self.trace_ingress_state.lock().map_err(|_| { + GitAiError::Generic("trace ingress state lock poisoned".to_string()) })?; - if terminal { - roots.remove(&root_sid) - } else { - roots.contains(&root_sid) - } + ingress.root_ignored_temp_repositories.contains(&root_sid) }; if ignored_root { if terminal { @@ -6551,13 +6552,13 @@ impl ActorDaemonCoordinator { false } else { let repo_path_for_check = repo_path.clone(); - let ignored = crate::tokio_runtime::spawn_blocking_result(move || { + let checked = crate::tokio_runtime::spawn_blocking_result(move || { let repo = discover_repository_in_path_no_git_exec(&repo_path_for_check)?; repo.is_temporary_without_remote_url() }) - .await - .unwrap_or(false); - if !ignored { + .await; + let ignored = checked.as_ref().copied().unwrap_or(false); + if matches!(checked, Ok(false)) { let mut cache = self.temp_repo_filter_allow_cache.lock().map_err(|_| { GitAiError::Generic( "temporary repository allow cache lock poisoned".to_string(), @@ -6574,11 +6575,10 @@ impl ActorDaemonCoordinator { }; if ignored { - self.temp_repo_filter_roots + self.trace_ingress_state .lock() - .map_err(|_| { - GitAiError::Generic("temporary repository root cache lock poisoned".to_string()) - })? + .map_err(|_| GitAiError::Generic("trace ingress state lock poisoned".to_string()))? + .root_ignored_temp_repositories .insert(root_sid.clone()); { let mut normalizer = self.normalizer.lock().await; @@ -10090,7 +10090,7 @@ mod tests { #[serial] #[tokio::test] - async fn ignored_temp_repo_terminal_payload_clears_trace_tracking() { + async fn ignored_temp_repo_connection_close_clears_trace_tracking() { let _filter = EnvVarGuard::set("GIT_AI_TEST_ENABLE_TEMP_REPO_FILTER", "1"); let coord = ActorDaemonCoordinator::new(); let temp = tempfile::tempdir().unwrap(); @@ -10109,14 +10109,14 @@ mod tests { let sid = "20260411T120000.000000-Psid-ignored-temp"; coord.trace_root_connection_opened(sid).unwrap(); - let mut start = serde_json::json!({ - "event": "start", - "sid": sid, - "argv": ["git", "commit", "-m", "ignored commit"], - "worktree": worktree, - "time_ns": 1u64, - }); - assert!(coord.prepare_trace_payload_for_ingest(&mut start)); + { + let mut ingress = coord.trace_ingress_state.lock().unwrap(); + ingress.root_argv.insert( + sid.to_string(), + vec!["git".to_string(), "unknown".to_string()], + ); + ingress.root_mutating.insert(sid.to_string(), false); + } coord.record_trace_payload_enqueued_root(Some(sid)).unwrap(); let def_repo = serde_json::json!({ @@ -10149,6 +10149,7 @@ mod tests { assert!(!ingress.root_argv.contains_key(sid)); assert!(!ingress.root_open_connections.contains_key(sid)); assert!(!ingress.root_close_markers_enqueued.contains(sid)); + assert!(!ingress.root_ignored_temp_repositories.contains(sid)); drop(ingress); assert!( !coord @@ -10157,7 +10158,32 @@ mod tests { .unwrap() .contains_key(sid) ); - assert!(!coord.temp_repo_filter_roots.lock().unwrap().contains(sid)); + } + + #[serial] + #[tokio::test] + async fn temporary_repo_inspection_error_is_not_cached_as_allowed() { + let _filter = EnvVarGuard::set("GIT_AI_TEST_ENABLE_TEMP_REPO_FILTER", "1"); + let coord = ActorDaemonCoordinator::new(); + let temp = tempfile::tempdir().unwrap(); + let missing_worktree = temp.path().join("missing-repo"); + let payload = serde_json::json!({ + "event": "def_repo", + "sid": "20260411T120000.000000-Psid-missing-temp", + "repo": 1, + "worktree": missing_worktree, + "time_ns": 1u64, + }); + + assert!(!coord.should_ignore_trace_payload(&payload).await.unwrap()); + assert!( + !coord + .temp_repo_filter_allow_cache + .lock() + .unwrap() + .contains(&missing_worktree), + "a failed inspection must fail open for this payload without caching the decision" + ); } #[tokio::test]