diff --git a/cli/src/services/mutation_trace/runtime/coordinator.rs b/cli/src/services/mutation_trace/runtime/coordinator.rs index a4d0bb89..3cb218ff 100644 --- a/cli/src/services/mutation_trace/runtime/coordinator.rs +++ b/cli/src/services/mutation_trace/runtime/coordinator.rs @@ -12,6 +12,7 @@ use crate::services::mutation_trace::types::{ self, ActorKind, AttemptId, Boundary, EventId, MutationEvent, ScopeId, TreeId, WorktreeId, }; +use super::external_taint::ExternalTaintMarker; use super::git_snapshot::GitSnapshotService; use super::worktree_lock::{acquire_inner, WorktreeLockError}; @@ -48,6 +49,18 @@ pub struct CoordinateOutcome { pub mutation_event: Option, } +/// Which pre-commit [`ExternalTaintMarker`] operation failed while coordinating a +/// boundary. Both happen **before** any protected work, so no +/// [`CoordinateOutcome`] exists yet. A marker-clear failure happens *after* a +/// durable commit and is reported through +/// [`CoordinateError::MarkerClearAfterCommit`] instead, which carries the +/// committed outcome. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ExternalTaintOperation { + Inspect, + Persist, +} + #[derive(Debug)] pub enum CoordinateError { SnapshotFailure { @@ -63,6 +76,30 @@ pub enum CoordinateError { revision: u64, }, LockAcquisition(anyhow::Error), + /// Inspecting or persisting the worktree-local external-taint marker failed. + /// Both operations run **before** any checkout-identity, DB, snapshot, or + /// protocol work, so no mutation boundary has committed and there is no + /// [`CoordinateOutcome`] to surface — the boundary is aborted fail-closed + /// with the fence left in whatever state it was in. + ExternalTaintMarker { + operation: ExternalTaintOperation, + source: anyhow::Error, + }, + /// The mutation boundary committed successfully to the Agent Trace DB and + /// produced a [`CoordinateOutcome`], but clearing the write-ahead + /// external-taint marker afterwards failed. The boundary did **not** fail: + /// `committed` carries the durable outcome (including any [`MutationEvent`]) + /// so the caller never loses it. The marker remains logically armed, so the + /// next invocation conservatively recovers. + MarkerClearAfterCommit { + source: anyhow::Error, + committed: Box, + }, + /// The caller-supplied Agent Trace DB provider returned `Err` after the + /// external-taint marker was already armed. The marker is intentionally + /// left in place so a later invocation treats the lost interval + /// conservatively. + AgentTraceDbUnavailable(anyhow::Error), Other(anyhow::Error), } @@ -87,6 +124,19 @@ impl std::fmt::Display for CoordinateError { "Worktree {worktree_id:?} requires recovery but its revision \ ({revision}) cannot be advanced" ), + CoordinateError::ExternalTaintMarker { operation, source } => write!( + f, + "External-taint marker {operation:?} operation failed before any \ + mutation boundary committed: {source}" + ), + CoordinateError::MarkerClearAfterCommit { source, .. } => write!( + f, + "Mutation boundary committed, but clearing the external-taint \ + marker failed: {source}" + ), + CoordinateError::AgentTraceDbUnavailable(source) => { + write!(f, "Repository Agent Trace DB is unavailable: {source}") + } CoordinateError::ScopeIdentityConflict(source) | CoordinateError::LockAcquisition(source) | CoordinateError::Other(source) => write!(f, "{source}"), @@ -111,46 +161,144 @@ impl SnapshotCapture for GitSnapshotService { } } -pub fn coordinate( +/// Coordinates one mutation-cursor runtime boundary end to end. +/// +/// The entrypoint owns the whole protected operation: it resolves `git_dir`, +/// acquires the [`WorktreeLock`](super::worktree_lock::WorktreeLock), arms the +/// worktree-local [`ExternalTaintMarker`] write-ahead — **before** acquiring the +/// Agent Trace DB — and only then invokes the caller-supplied `open_db` +/// provider, captures a snapshot, runs the snapshot / recovery / protocol / CAS +/// pipeline, and clears the marker on complete success. Any failure after the +/// marker is armed — including `open_db` returning `Err` — leaves the marker in +/// place for the next invocation. +pub fn coordinate

( repository_root: &Path, - db: &RepositoryAgentTraceDb, boundary: &RuntimeBoundary, -) -> Result { - coordinate_inner(repository_root, db, boundary, || {}) + open_db: P, +) -> Result +where + P: FnOnce() -> anyhow::Result, +{ + coordinate_inner(repository_root, boundary, open_db, || {}, |_attempt| Ok(())) } -fn coordinate_inner( +fn coordinate_inner( repository_root: &Path, - db: &RepositoryAgentTraceDb, boundary: &RuntimeBoundary, + open_db: P, on_lock_contention: F, + after_recovery: R, ) -> Result where + P: FnOnce() -> anyhow::Result, F: FnOnce(), + R: FnMut(u32) -> Result<()>, { let git_dir = resolve_git_dir(repository_root).map_err(CoordinateError::Other)?; let _lock = acquire_inner(&git_dir, WORKTREE_LOCK_TIMEOUT, on_lock_contention) .map_err(lock_acquisition)?; - let checkout_id = get_or_create_checkout_id(&git_dir).map_err(CoordinateError::Other)?; + let marker = ExternalTaintMarker::new(&git_dir); + let inherited_external_taint = + marker + .exists() + .map_err(|source| CoordinateError::ExternalTaintMarker { + operation: ExternalTaintOperation::Inspect, + source, + })?; + marker + .persist() + .map_err(|source| CoordinateError::ExternalTaintMarker { + operation: ExternalTaintOperation::Persist, + source, + })?; + + let outcome = coordinate_protected( + repository_root, + &git_dir, + boundary, + open_db, + inherited_external_taint, + after_recovery, + )?; + + match marker.clear() { + Ok(()) => Ok(outcome), + Err(source) => Err(CoordinateError::MarkerClearAfterCommit { + source, + committed: Box::new(outcome), + }), + } +} + +fn coordinate_protected( + repository_root: &Path, + git_dir: &Path, + boundary: &RuntimeBoundary, + open_db: P, + inherited_external_taint: bool, + after_recovery: R, +) -> Result +where + P: FnOnce() -> anyhow::Result, + R: FnMut(u32) -> Result<()>, +{ + let checkout_id = get_or_create_checkout_id(git_dir).map_err(CoordinateError::Other)?; let worktree_id = WorktreeId(checkout_id); + let db = open_db().map_err(CoordinateError::AgentTraceDbUnavailable)?; + let snapshot = GitSnapshotService::new(repository_root).map_err(CoordinateError::Other)?; - coordinate_boundary(db, &snapshot, &worktree_id, boundary) + coordinate_boundary_inner( + &db, + &snapshot, + &worktree_id, + boundary, + inherited_external_taint, + |_attempt| {}, + after_recovery, + ) } fn lock_acquisition(error: WorktreeLockError) -> CoordinateError { CoordinateError::LockAcquisition(anyhow::Error::new(error)) } +#[cfg(test)] fn coordinate_boundary( db: &RepositoryAgentTraceDb, capture: &C, worktree_id: &WorktreeId, boundary: &RuntimeBoundary, + inherited_external_taint: bool, ) -> Result { + coordinate_boundary_inner( + db, + capture, + worktree_id, + boundary, + inherited_external_taint, + |_attempt| {}, + |_attempt| Ok(()), + ) +} + +fn coordinate_boundary_inner( + db: &RepositoryAgentTraceDb, + capture: &C, + worktree_id: &WorktreeId, + boundary: &RuntimeBoundary, + inherited_external_taint: bool, + mut after_load: AfterLoad, + mut after_recovery: AfterRecovery, +) -> Result +where + C: SnapshotCapture, + AfterLoad: FnMut(u32), + AfterRecovery: FnMut(u32) -> Result<()>, +{ let store = MutationTraceStore::new(db); let observed_tree = match capture.capture().and_then(|tree| { @@ -175,7 +323,9 @@ fn coordinate_boundary( let scope_ref = types::boundary_scope(&type_boundary); let event_key_ref = types::boundary_event_key(&type_boundary); - for _ in 0..MAX_CAS_RETRY_ATTEMPTS { + let mut external_taint_pending = inherited_external_taint; + + for attempt_index in 0..MAX_CAS_RETRY_ATTEMPTS { let Some(projection) = store .load_worktree(worktree_id, scope_ref.as_ref(), event_key_ref.as_ref()) .map_err(CoordinateError::Other)? @@ -185,8 +335,14 @@ fn coordinate_boundary( ))); }; + after_load(attempt_index); + let mut state = projection.into_protocol_state(); + if external_taint_pending { + state = protocol::database_failure(&state, worktree_id); + } + if needs_recovery(&state, worktree_id) { let recovered = protocol::recover(&state, worktree_id, observed_tree.clone()); let Some(transition) = DurableTransition::between(&state, &recovered, worktree_id) @@ -204,7 +360,11 @@ fn coordinate_boundary( }; match store.commit(&transition).map_err(CoordinateError::Other)? { - CasResult::Applied => state = recovered, + CasResult::Applied => { + state = recovered; + external_taint_pending = false; + after_recovery(attempt_index).map_err(CoordinateError::Other)?; + } CasResult::Conflict => continue, } } @@ -363,7 +523,7 @@ mod tests { use super::*; use crate::services::mutation_trace::store::encode_revision; - use crate::services::mutation_trace::types::{Attribution, FailureKind, ScopeStatus}; + use crate::services::mutation_trace::types::{Attribution, EventKey, FailureKind, ScopeStatus}; static NEXT_TEST_DB_ID: AtomicU64 = AtomicU64::new(0); @@ -583,7 +743,7 @@ mod tests { let worktree = WorktreeId("wt-1".to_string()); let capture = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); - let outcome = coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush) + let outcome = coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush, false) .expect("first observation should succeed"); assert_eq!(outcome.observed_tree, TreeId("tree-a".to_string())); @@ -617,6 +777,7 @@ mod tests { event: EventId("evt-start".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("start should succeed"); @@ -630,6 +791,7 @@ mod tests { event: EventId("evt-advance".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("advance should succeed"); @@ -659,6 +821,7 @@ mod tests { event: EventId("evt-start".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("start should succeed"); @@ -669,12 +832,12 @@ mod tests { }; capture.push_success(TreeId("tree-b".to_string())); - let first = coordinate_boundary(&db, &capture, &worktree, &advance) + let first = coordinate_boundary(&db, &capture, &worktree, &advance, false) .expect("first advance should commit"); assert!(first.mutation_event.is_some()); capture.push_success(TreeId("tree-b".to_string())); - let replay = coordinate_boundary(&db, &capture, &worktree, &advance).expect( + let replay = coordinate_boundary(&db, &capture, &worktree, &advance, false).expect( "replaying the identical (scope, event) boundary must be a no-op, not an error", ); assert!( @@ -705,6 +868,7 @@ mod tests { event: EventId("evt-start".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("start should succeed"); @@ -718,6 +882,7 @@ mod tests { event: EventId("evt-close".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("close should succeed"); @@ -757,6 +922,7 @@ mod tests { event: EventId("evt-start-a".to_string()), actor_kind: actor_a, }, + false, ) .expect("starting scope a should succeed"); coordinate_boundary( @@ -768,6 +934,7 @@ mod tests { event: EventId("evt-start-b".to_string()), actor_kind: actor_b, }, + false, ) .expect("starting scope b should succeed"); @@ -781,6 +948,7 @@ mod tests { event: EventId("evt-advance-a".to_string()), actor_kind: actor_a, }, + false, ) .expect("advance should succeed"); @@ -816,8 +984,14 @@ mod tests { { let db = RepositoryAgentTraceDb::new_at(&db_path).expect("test db should open"); let bootstrap_capture = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); - coordinate_boundary(&db, &bootstrap_capture, &worktree, &RuntimeBoundary::Flush) - .expect("baseline flush should succeed"); + coordinate_boundary( + &db, + &bootstrap_capture, + &worktree, + &RuntimeBoundary::Flush, + false, + ) + .expect("baseline flush should succeed"); } let barrier = std::sync::Arc::new(std::sync::Barrier::new(WRITERS)); @@ -832,9 +1006,10 @@ mod tests { let capture = FakeSnapshotCapture::new(TreeId(format!("tree-writer-{i}"))); barrier.wait(); let outcome = - coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush).expect( - "each racing writer should eventually succeed after reload+recompute", - ); + coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush, false) + .expect( + "each racing writer should eventually succeed after reload+recompute", + ); ( outcome.revision, capture.capture_call_count(), @@ -885,6 +1060,7 @@ mod tests { event: EventId("evt-start-live".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("starting the live scope should succeed"); coordinate_boundary( @@ -896,6 +1072,7 @@ mod tests { event: EventId("evt-start-abandoned".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("starting the to-be-abandoned scope should succeed"); @@ -932,6 +1109,7 @@ mod tests { event: EventId("evt-advance-live".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("advance should trigger needs_rebaseline recovery first, then succeed"); assert!( @@ -977,6 +1155,7 @@ mod tests { event: EventId("evt-start".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("start should succeed"); @@ -1013,6 +1192,7 @@ mod tests { event: EventId("evt-advance".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("advance should trigger taint recovery first, then succeed"); @@ -1064,6 +1244,7 @@ mod tests { event: event.clone(), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect_err("recovery that cannot advance revision must reject the triggering boundary"); @@ -1125,13 +1306,13 @@ mod tests { let (db, db_path) = test_db("ac11-taints-existing"); let worktree = WorktreeId("wt-1".to_string()); let bootstrap = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); - coordinate_boundary(&db, &bootstrap, &worktree, &RuntimeBoundary::Flush) + coordinate_boundary(&db, &bootstrap, &worktree, &RuntimeBoundary::Flush, false) .expect("baseline flush should materialize the worktree"); let failing = FakeSnapshotCapture::new(TreeId("tree-b".to_string())); failing.push_failure("simulated git snapshot failure"); - let error = coordinate_boundary(&db, &failing, &worktree, &RuntimeBoundary::Flush) + let error = coordinate_boundary(&db, &failing, &worktree, &RuntimeBoundary::Flush, false) .expect_err("a capture failure against an existing worktree should be reported"); match error { CoordinateError::SnapshotFailure { @@ -1159,7 +1340,7 @@ mod tests { let (db, db_path) = test_db("ac11-taint-retry-succeeds"); let worktree = WorktreeId("wt-1".to_string()); let bootstrap = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); - coordinate_boundary(&db, &bootstrap, &worktree, &RuntimeBoundary::Flush) + coordinate_boundary(&db, &bootstrap, &worktree, &RuntimeBoundary::Flush, false) .expect("baseline flush should materialize the worktree"); let store = MutationTraceStore::new(&db); @@ -1202,7 +1383,7 @@ mod tests { let (db, db_path) = test_db("ac11-taint-exhaustion"); let worktree = WorktreeId("wt-1".to_string()); let bootstrap = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); - coordinate_boundary(&db, &bootstrap, &worktree, &RuntimeBoundary::Flush) + coordinate_boundary(&db, &bootstrap, &worktree, &RuntimeBoundary::Flush, false) .expect("baseline flush should materialize the worktree"); let store = MutationTraceStore::new(&db); @@ -1244,7 +1425,7 @@ mod tests { let failing = FakeSnapshotCapture::new(TreeId("unused".to_string())); failing.push_failure("simulated git snapshot failure before any baseline exists"); - let error = coordinate_boundary(&db, &failing, &worktree, &RuntimeBoundary::Flush) + let error = coordinate_boundary(&db, &failing, &worktree, &RuntimeBoundary::Flush, false) .expect_err("a capture failure with no prior worktree row should still be reported"); match error { CoordinateError::SnapshotFailure { @@ -1283,7 +1464,7 @@ mod tests { }, ); - let error = coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush) + let error = coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush, false) .expect_err( "a capture failure racing a concurrent materialization should still be reported", ); @@ -1320,13 +1501,17 @@ mod tests { let repo_root_clone = repo_root.clone(); let db_path_clone = db_path.clone(); let worker = thread::spawn(move || { - let db = RepositoryAgentTraceDb::new_at(&db_path_clone).expect("worker db should open"); - let outcome = - coordinate_inner(&repo_root_clone, &db, &RuntimeBoundary::Flush, move || { + let outcome = coordinate_inner( + &repo_root_clone, + &RuntimeBoundary::Flush, + || RepositoryAgentTraceDb::new_at(&db_path_clone), + move || { contention_tx .send(()) .expect("contention signal channel should still be open"); - }); + }, + |_attempt| Ok(()), + ); result_tx .send(()) .expect("result signal channel should still be open"); @@ -1362,4 +1547,633 @@ mod tests { remove_test_repo(&repo_root); } + + #[test] + fn public_coordinate_clears_marker_on_success() { + let repo_root = unique_test_repo("t02-success-clears-marker"); + init_repo(&repo_root); + let db_path = repo_root.join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("seed db should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + + let outcome = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("a first observation should succeed"); + assert_eq!( + outcome.revision, 0, + "a first-observation flush should not advance the revision" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "a successful coordinate() must clear the marker it armed" + ); + + remove_test_repo(&repo_root); + } + + #[test] + fn public_coordinate_leaves_marker_after_a_snapshot_failure() { + let repo_root = unique_test_repo("t02-snapshot-failure-marker"); + init_repo(&repo_root); + let db_path = repo_root.join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("seed db should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + + coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the baseline observation should succeed"); + assert!( + !marker.exists().expect("marker existence should resolve"), + "the successful baseline must have cleared its marker" + ); + + let tmp_index_dir = git_dir.join("sce").join("tmp"); + let _ = std::fs::remove_dir_all(&tmp_index_dir); + std::fs::write(&tmp_index_dir, b"not a directory").expect( + "planting a file where the snapshot service expects its temp-index directory should succeed", + ); + + let error = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect_err("a Git snapshot failure after marker arming should be reported"); + assert!( + matches!(error, CoordinateError::SnapshotFailure { .. }), + "expected SnapshotFailure, got {error:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "a snapshot failure after arming must leave the external-taint marker in place" + ); + + remove_test_repo(&repo_root); + } + + #[test] + fn public_coordinate_leaves_marker_after_a_non_snapshot_failure() { + let repo_root = unique_test_repo("t02-non-snapshot-failure-marker"); + init_repo(&repo_root); + let db_path = repo_root.join("agent-trace.db"); + let seed_db = RepositoryAgentTraceDb::new_at(&db_path).expect("seed db should open"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let checkout_id = get_or_create_checkout_id(&git_dir).expect("checkout id should resolve"); + insert_worktree_at_revision( + &seed_db, + &checkout_id, + "tree-a", + u64::MAX, + true, + "snapshot_failure", + false, + ); + drop(seed_db); + + let marker = ExternalTaintMarker::new(&git_dir); + let error = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect_err("mandatory recovery that cannot advance the revision must reject the boundary"); + assert!( + matches!(error, CoordinateError::RevisionExhausted { .. }), + "expected RevisionExhausted, got {error:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "a non-snapshot failure after arming must leave the external-taint marker in place" + ); + + remove_test_repo(&repo_root); + } + + #[test] + fn public_coordinate_fails_closed_when_the_marker_cannot_be_armed() { + let repo_root = unique_test_repo("t02-marker-arm-failure"); + init_repo(&repo_root); + let db_path = repo_root.join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("seed db should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + + // Plant a directory exactly where the marker file must be created, so + // `ExternalTaintMarker::persist` fails deterministically regardless of uid. + std::fs::create_dir_all(git_dir.join("sce").join("mutation-cursor-tainted")) + .expect("planting a directory at the marker path should succeed"); + + let provider_called = std::sync::atomic::AtomicBool::new(false); + let error = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + provider_called.store(true, Ordering::SeqCst); + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect_err("an unarmed marker must fail coordinate() closed"); + assert!( + matches!( + error, + CoordinateError::ExternalTaintMarker { + operation: ExternalTaintOperation::Persist, + .. + } + ), + "expected an ExternalTaintMarker persist failure, got {error:?}" + ); + assert!( + !provider_called.load(Ordering::SeqCst), + "a marker-arming failure must return before DB-provider, snapshot, or protocol work" + ); + + remove_test_repo(&repo_root); + } + + #[test] + fn public_coordinate_fails_closed_when_marker_inspection_fails() { + let repo_root = unique_test_repo("t02-marker-inspect-failure"); + init_repo(&repo_root); + let db_path = repo_root.join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("seed db should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let sce_dir = git_dir.join("sce"); + std::fs::create_dir_all(&sce_dir).expect("the sce directory should be creatable"); + + let held = acquire_inner(&git_dir, Duration::from_secs(5), || {}) + .expect("the test should hold the runtime lock before the worker runs"); + + let provider_called = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + let (contention_tx, contention_rx) = mpsc::channel(); + + let worker = { + let repo_root = repo_root.clone(); + let db_path = db_path.clone(); + let provider_called = std::sync::Arc::clone(&provider_called); + thread::spawn(move || { + coordinate_inner( + &repo_root, + &RuntimeBoundary::Flush, + || { + provider_called.store(true, Ordering::SeqCst); + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }, + move || { + contention_tx + .send(()) + .expect("contention signal channel should still be open"); + }, + |_attempt| Ok(()), + ) + }) + }; + + contention_rx.recv_timeout(Duration::from_secs(5)).expect( + "the worker's coordinate_inner should reach the WorktreeLock contention branch", + ); + + std::fs::remove_dir_all(&sce_dir).expect("the sce directory should be removable"); + std::fs::write(&sce_dir, b"not a directory") + .expect("planting a file where the sce directory was should succeed"); + + drop(held); + + let error = worker + .join() + .expect("worker thread should not panic") + .expect_err("marker inspection failure must fail coordinate() closed"); + assert!( + matches!( + error, + CoordinateError::ExternalTaintMarker { + operation: ExternalTaintOperation::Inspect, + .. + } + ), + "expected an ExternalTaintMarker inspect failure, got {error:?}" + ); + assert!( + !provider_called.load(Ordering::SeqCst), + "marker inspection failing closed must precede DB-provider, checkout, snapshot, and protocol work" + ); + + remove_test_repo(&repo_root); + } + + #[test] + fn public_coordinate_leaves_marker_when_the_db_provider_fails() { + let repo_root = unique_test_repo("t02-db-provider-failure-marker"); + init_repo(&repo_root); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + + let error = coordinate( + &repo_root, + &RuntimeBoundary::Flush, + || -> anyhow::Result { + Err(anyhow::anyhow!("simulated Agent Trace DB open failure")) + }, + ) + .expect_err("a DB provider that returns Err must fail coordinate()"); + assert!( + matches!(error, CoordinateError::AgentTraceDbUnavailable(_)), + "expected AgentTraceDbUnavailable, got {error:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "a DB-provider failure after arming must leave the external-taint marker present" + ); + + remove_test_repo(&repo_root); + } + + #[test] + fn inherited_external_taint_recovers_once_before_the_boundary() { + let (db, db_path) = test_db("t03-inherited-recovers-once"); + let worktree = WorktreeId("wt-1".to_string()); + let scope = ScopeId("scope-live".to_string()); + let capture = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); + + coordinate_boundary( + &db, + &capture, + &worktree, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + false, + ) + .expect("start should establish the baseline and the live scope"); + + let advance_capture = FakeSnapshotCapture::new(TreeId("tree-b".to_string())); + let outcome = coordinate_boundary( + &db, + &advance_capture, + &worktree, + &RuntimeBoundary::Advance { + scope: scope.clone(), + event: EventId("evt-advance".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + true, + ) + .expect("an inherited external-taint marker must recover, then process the boundary"); + + assert!( + outcome.mutation_event.is_none(), + "no mutation evidence may span the interval the inherited marker fenced off" + ); + assert_eq!( + outcome.revision, 3, + "one recovery transition (rev 1 -> 2), then the triggering boundary (rev 2 -> 3)" + ); + assert_eq!( + advance_capture.capture_call_count(), + 1, + "recovery and the triggering boundary must share the single already-captured snapshot" + ); + assert_eq!(advance_capture.pin_call_count(), 1); + + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree, Some(&scope), None) + .expect("load should succeed") + .expect("worktree should exist"); + assert!(!projection.worktree_state.tainted); + assert!(!projection.worktree_state.needs_rebaseline); + assert_eq!(projection.worktree_state.failure_kind, FailureKind::Healthy); + assert_eq!( + projection.worktree_state.cursor_tree, + TreeId("tree-b".to_string()), + "recovery rebaselines the cursor to this invocation's observed tree" + ); + assert_eq!( + projection.scopes.get(&scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "inherited-taint recovery abandons the live scopes the fenced interval made untrustworthy" + ); + + remove_test_db(&db_path); + } + + #[test] + fn inherited_external_taint_with_no_worktree_row_baselines_without_evidence() { + let (db, db_path) = test_db("t03-inherited-no-row"); + let worktree = WorktreeId("wt-1".to_string()); + let capture = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); + + let outcome = coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush, true) + .expect("a first-ever invocation carrying an inherited marker must still succeed"); + + assert!( + outcome.mutation_event.is_none(), + "a worktree with no prior durable row cannot produce evidence for the unknown interval" + ); + assert_eq!( + outcome.revision, 1, + "the freshly initialized worktree is baselined against the observed tree, then conservatively recovered once" + ); + + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree, None, None) + .expect("load should succeed") + .expect("the worktree row should now exist"); + assert!(!projection.worktree_state.tainted); + assert_eq!(projection.worktree_state.failure_kind, FailureKind::Healthy); + assert_eq!( + projection.worktree_state.cursor_tree, + TreeId("tree-a".to_string()) + ); + + remove_test_db(&db_path); + } + + #[test] + fn a_losing_recovery_cas_reinjects_external_taint_until_it_applies() { + let (db, db_path) = test_db("t03-recovery-cas-reinjection"); + let worktree = WorktreeId("wt-1".to_string()); + let scope = ScopeId("scope-live".to_string()); + let bootstrap = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); + + coordinate_boundary( + &db, + &bootstrap, + &worktree, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + false, + ) + .expect("bootstrap start should establish the baseline and live scope"); + + let store = MutationTraceStore::new(&db); + let competing_scope = ScopeId("competing-scope".to_string()); + let interfered = Cell::new(false); + let capture = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); + + let outcome = coordinate_boundary_inner( + &db, + &capture, + &worktree, + &RuntimeBoundary::Flush, + true, + |attempt| { + if attempt == 0 && !interfered.get() { + interfered.set(true); + commit_competing_advance( + &store, + &worktree, + &competing_scope, + "competing-event-1", + true, + ); + } + }, + |_attempt| Ok(()), + ) + .expect("recovery must recompute past the losing CAS and still succeed"); + + assert_eq!( + capture.capture_call_count(), + 1, + "a recovery CAS conflict must not trigger a second Git snapshot" + ); + assert_eq!( + outcome.revision, 3, + "bootstrap start (1) + competing advance (2) + exactly one landed recovery (3)" + ); + + let projection = store + .load_worktree(&worktree, Some(&scope), None) + .expect("load should succeed") + .expect("worktree should exist"); + assert!(!projection.worktree_state.tainted); + assert_eq!( + projection.scopes.get(&scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "the re-injected recovery still abandons the fenced-off live scope" + ); + + let competing_projection = store + .load_worktree(&worktree, Some(&competing_scope), None) + .expect("load should succeed") + .expect("worktree should exist"); + assert_eq!( + competing_projection + .scopes + .get(&competing_scope) + .map(|s| s.status), + Some(ScopeStatus::Abandoned) + ); + + remove_test_db(&db_path); + } + + #[test] + fn a_landed_recovery_clears_the_flag_so_a_boundary_cas_retry_does_not_re_recover() { + let (db, db_path) = test_db("t03-flag-clears-after-recovery"); + let worktree = WorktreeId("wt-1".to_string()); + let scope = ScopeId("scope-live".to_string()); + let bootstrap = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); + + coordinate_boundary( + &db, + &bootstrap, + &worktree, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + false, + ) + .expect("bootstrap start should establish the baseline and live scope"); + + let store = MutationTraceStore::new(&db); + let competing_scope = ScopeId("competing-scope".to_string()); + let interfered = Cell::new(false); + let capture = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); + capture.push_success(TreeId("tree-b".to_string())); + + let outcome = coordinate_boundary_inner( + &db, + &capture, + &worktree, + &RuntimeBoundary::Advance { + scope: scope.clone(), + event: EventId("evt-advance".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + true, + |_attempt| {}, + |attempt| { + if attempt == 0 && !interfered.get() { + interfered.set(true); + commit_competing_advance( + &store, + &worktree, + &competing_scope, + "competing-event-1", + true, + ); + } + Ok(()) + }, + ) + .expect("the boundary CAS retry after a landed recovery must still succeed"); + + assert_eq!( + capture.capture_call_count(), + 1, + "neither the recovery nor the boundary retry may take a second snapshot" + ); + assert_eq!( + outcome.revision, 4, + "start (1) + one landed recovery (2) + competing advance (3) + the retried boundary (4); a re-triggered recovery on the retry would land at 5" + ); + + remove_test_db(&db_path); + } + + #[test] + #[allow(clippy::too_many_lines)] + fn a_failure_after_recovery_before_boundary_commit_leaves_marker_and_forces_later_recovery() { + let repo_root = unique_test_repo("t-ac8-recovery-then-fail"); + init_repo(&repo_root); + let db_path = unique_test_db_path("t-ac8-recovery-then-fail"); + RepositoryAgentTraceDb::new_at(&db_path).expect("seed db should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + let ok_db = || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path); + + std::fs::write(repo_root.join("work.txt"), b"a").expect("the baseline edit should write"); + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the baseline observation should establish cursor A"); + let worktree_id = baseline.worktree_id.clone(); + let tree_a = baseline.observed_tree.clone(); + + let scope = ScopeId("scope-live".to_string()); + coordinate( + &repo_root, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect("starting the live scope should succeed"); + + marker.persist().expect( + "simulating a prior crashed invocation that armed but never cleared the marker", + ); + std::fs::write(repo_root.join("work.txt"), b"b").expect("the A -> B edit should write"); + + let advance_event = EventId("evt-advance".to_string()); + let event_key = EventKey { + scope_id: scope.clone(), + event_id: advance_event.clone(), + }; + + let error = coordinate_inner( + &repo_root, + &RuntimeBoundary::Advance { + scope: scope.clone(), + event: advance_event.clone(), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + || {}, + |_attempt| { + anyhow::bail!("injected failure after recovery, before the boundary commits") + }, + ) + .expect_err("the injected post-recovery failure must fail the invocation"); + assert!( + matches!(error, CoordinateError::Other(_)), + "expected CoordinateError::Other from the injected failure, got {error:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "a failure after recovery but before the boundary commits must leave the marker armed" + ); + + let store_db = ok_db().expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&store_db); + let after_fail = store + .load_worktree(&worktree_id, Some(&scope), Some(&event_key)) + .expect("loading the worktree row should succeed") + .expect("the worktree row should exist"); + assert!( + !after_fail.worktree_state.tainted, + "the recovery CAS committed durably before the injected failure" + ); + assert_eq!( + after_fail.worktree_state.failure_kind, + FailureKind::Healthy, + "recovery cleared the failure state before the injected failure" + ); + assert!( + after_fail.worktree_state.revision >= 2, + "the durable recovery advanced the revision past the start boundary" + ); + assert_ne!( + after_fail.worktree_state.cursor_tree, tree_a, + "recovery rebaselined the cursor away from A to the invocation's own observed tree" + ); + assert_eq!( + after_fail.scopes.get(&scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "the live scope was abandoned by the durable recovery" + ); + assert!( + !after_fail.processed_events.contains(&event_key), + "the triggering Advance must never have been processed" + ); + assert!( + store + .load_mutation_event(&worktree_id, after_fail.worktree_state.revision) + .expect("loading a mutation event should succeed") + .is_none(), + "no MutationEvent may be emitted for the boundary that never committed" + ); + drop(store_db); + + std::fs::write(repo_root.join("work.txt"), b"c").expect("the B -> C edit should write"); + let recovered = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the later invocation inherits the still-armed marker and recovers again"); + assert!( + recovered.mutation_event.is_none(), + "no evidence may cross the interval the still-armed marker fenced off" + ); + assert_ne!( + recovered.observed_tree, tree_a, + "the later recovery rebaselines to the newer tree C" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "the later successful recovery finally clears the marker" + ); + + let store_db = ok_db().expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&store_db); + let after_recover = store + .load_worktree(&worktree_id, Some(&scope), None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should exist"); + assert_eq!( + after_recover.worktree_state.cursor_tree, recovered.observed_tree, + "the later recovery rebaselines the cursor to its own observed tree" + ); + assert_eq!( + after_recover.scopes.get(&scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "the later invocation must not resurrect the abandoned scope" + ); + drop(store_db); + + remove_test_db(&db_path); + remove_test_repo(&repo_root); + } } diff --git a/cli/src/services/mutation_trace/runtime/external_taint.rs b/cli/src/services/mutation_trace/runtime/external_taint.rs new file mode 100644 index 00000000..38bdc7da --- /dev/null +++ b/cli/src/services/mutation_trace/runtime/external_taint.rs @@ -0,0 +1,257 @@ +//! External-taint marker primitive for the mutation-cursor runtime boundary. +//! +//! A worktree-local filesystem marker at `/sce/mutation-cursor-tainted`. +//! Its existence is its entire state: armed write-ahead at the start of the +//! protected runtime section, cleared only after a proven durable completion, and +//! read by a later invocation as the external signal that the previous +//! invocation never proved a trustworthy durable completion. +//! +//! Durability mirrors [`crate::services::checkout`]'s `persist_checkout_id_inner`: +//! [`ExternalTaintMarker::persist`] creates and `fsync`s the marker file, and +//! both `persist` and [`ExternalTaintMarker::clear`] do a best-effort +//! `#[cfg(unix)]` parent-directory `sync_all` whose error is not propagated. This +//! protects against process error, non-graceful process exit, `SIGKILL`, and +//! normal runtime restart — not host power loss or a filesystem-level crash. +//! +//! The filesystem marker is never authoritative for normal cursor state, and it +//! is never deleted via `Drop`: only an explicit `clear()` after a successful +//! `CoordinateOutcome` removes it. + +use std::fs::OpenOptions; +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; + +/// Subdirectory inside `/` where SCE runtime metadata lives. +const SCE_RUNTIME_DIR: &str = "sce"; + +/// File name for the external-taint marker inside `/sce/`. +const MARKER_FILE: &str = "mutation-cursor-tainted"; + +/// Worktree-local external-taint marker. +/// +/// Construct with [`ExternalTaintMarker::new`] from the worktree-specific Git +/// directory (as resolved by [`crate::services::checkout::resolve_git_dir`]). +/// Two linked worktrees resolve to two different Git directories and therefore +/// two independent markers. +#[derive(Debug, Clone)] +pub struct ExternalTaintMarker { + marker_dir: PathBuf, + marker_path: PathBuf, +} + +impl ExternalTaintMarker { + /// Builds the marker handle rooted at `/sce/mutation-cursor-tainted`. + #[must_use] + pub fn new(git_dir: &Path) -> Self { + let marker_dir = git_dir.join(SCE_RUNTIME_DIR); + let marker_path = marker_dir.join(MARKER_FILE); + Self { + marker_dir, + marker_path, + } + } + + /// Returns `true` when the marker file is present. + /// + /// # Errors + /// + /// Returns an error when the marker path cannot be inspected for a reason + /// other than absence. + pub fn exists(&self) -> Result { + match std::fs::symlink_metadata(&self.marker_path) { + Ok(_) => Ok(true), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(err) => Err(err).with_context(|| { + format!( + "Failed to inspect external-taint marker '{}'", + self.marker_path.display() + ) + }), + } + } + + /// Creates and `fsync`s the marker file. + /// + /// Idempotent: a marker that already exists is re-synced and left in place. + /// + /// # Errors + /// + /// Returns an error when the marker directory or file cannot be created or + /// synced. + pub fn persist(&self) -> Result<()> { + std::fs::create_dir_all(&self.marker_dir).with_context(|| { + format!( + "Failed to create external-taint marker directory '{}'", + self.marker_dir.display() + ) + })?; + + let file = OpenOptions::new() + .write(true) + .create(true) + .truncate(false) + .open(&self.marker_path) + .with_context(|| { + format!( + "Failed to create external-taint marker '{}'", + self.marker_path.display() + ) + })?; + file.sync_data().with_context(|| { + format!( + "Failed to sync external-taint marker '{}'", + self.marker_path.display() + ) + })?; + drop(file); + + self.best_effort_sync_marker_dir(); + + Ok(()) + } + + /// Removes the marker file. + /// + /// Idempotent: clearing an absent marker is success. + /// + /// # Errors + /// + /// Returns an error when the marker file exists but cannot be removed. + pub fn clear(&self) -> Result<()> { + match std::fs::remove_file(&self.marker_path) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(err) => { + return Err(err).with_context(|| { + format!( + "Failed to clear external-taint marker '{}'", + self.marker_path.display() + ) + }); + } + } + + self.best_effort_sync_marker_dir(); + + Ok(()) + } + + #[cfg(unix)] + fn best_effort_sync_marker_dir(&self) { + if let Ok(dir) = std::fs::File::open(&self.marker_dir) { + let _ = dir.sync_all(); + } + } + + #[cfg(not(unix))] + fn best_effort_sync_marker_dir(&self) {} +} + +#[cfg(test)] +mod tests { + use std::sync::atomic::{AtomicU64, Ordering}; + + use super::*; + + static NEXT_TEST_GIT_DIR_ID: AtomicU64 = AtomicU64::new(0); + + fn unique_test_git_dir(label: &str) -> PathBuf { + let id = NEXT_TEST_GIT_DIR_ID.fetch_add(1, Ordering::Relaxed); + std::env::temp_dir().join(format!( + "sce-external-taint-{label}-{}-{id}", + std::process::id() + )) + } + + fn remove_test_git_dir(git_dir: &Path) { + let _ = std::fs::remove_dir_all(git_dir); + } + + #[test] + fn marker_is_worktree_scoped() { + let git_dir_a = unique_test_git_dir("scoped-a"); + let git_dir_b = unique_test_git_dir("scoped-b"); + std::fs::create_dir_all(&git_dir_a).expect("git dir a should be created"); + std::fs::create_dir_all(&git_dir_b).expect("git dir b should be created"); + + let marker_a = ExternalTaintMarker::new(&git_dir_a); + let marker_b = ExternalTaintMarker::new(&git_dir_b); + + assert_ne!( + marker_a.marker_path, marker_b.marker_path, + "each git dir must derive an independent marker path" + ); + + marker_a.persist().expect("marker a should persist"); + + assert!( + marker_a + .exists() + .expect("marker a existence should resolve"), + "the armed marker must be visible in its own worktree" + ); + assert!( + !marker_b + .exists() + .expect("marker b existence should resolve"), + "arming worktree A must not arm worktree B" + ); + + remove_test_git_dir(&git_dir_a); + remove_test_git_dir(&git_dir_b); + } + + #[test] + fn marker_persists_until_explicitly_cleared() { + let git_dir = unique_test_git_dir("persists"); + std::fs::create_dir_all(&git_dir).expect("git dir should be created"); + + ExternalTaintMarker::new(&git_dir) + .persist() + .expect("marker should persist"); + + let reloaded = ExternalTaintMarker::new(&git_dir); + assert!( + reloaded.exists().expect("marker existence should resolve"), + "the marker must survive reconstruction of the handle" + ); + + reloaded.clear().expect("marker should clear"); + assert!( + !ExternalTaintMarker::new(&git_dir) + .exists() + .expect("marker existence should resolve"), + "an explicitly cleared marker must be gone" + ); + + remove_test_git_dir(&git_dir); + } + + #[test] + fn persist_and_clear_are_idempotent() { + let git_dir = unique_test_git_dir("idempotent"); + std::fs::create_dir_all(&git_dir).expect("git dir should be created"); + let marker = ExternalTaintMarker::new(&git_dir); + + marker + .clear() + .expect("clear on absent marker should succeed"); + + marker.persist().expect("first persist should succeed"); + marker.persist().expect("second persist should succeed"); + assert!( + marker.exists().expect("marker existence should resolve"), + "the marker must remain armed after repeated persist" + ); + + marker.clear().expect("first clear should succeed"); + marker.clear().expect("second clear should succeed"); + assert!( + !marker.exists().expect("marker existence should resolve"), + "the marker must remain cleared after repeated clear" + ); + + remove_test_git_dir(&git_dir); + } +} diff --git a/cli/src/services/mutation_trace/runtime/mod.rs b/cli/src/services/mutation_trace/runtime/mod.rs index 794a6e12..703058a6 100644 --- a/cli/src/services/mutation_trace/runtime/mod.rs +++ b/cli/src/services/mutation_trace/runtime/mod.rs @@ -1,4 +1,5 @@ mod coordinator; +mod external_taint; mod git_snapshot; mod worktree_lock; diff --git a/cli/src/services/mutation_trace/runtime/tests.rs b/cli/src/services/mutation_trace/runtime/tests.rs index aea7491d..a0a2f0d4 100644 --- a/cli/src/services/mutation_trace/runtime/tests.rs +++ b/cli/src/services/mutation_trace/runtime/tests.rs @@ -11,9 +11,12 @@ use crate::services::agent_trace_storage::{ }; use crate::services::checkout::{read_checkout_id, resolve_git_dir}; use crate::services::mutation_trace::store::MutationTraceStore; -use crate::services::mutation_trace::types::{ActorKind, EventId, ScopeId}; +use crate::services::mutation_trace::types::{ + ActorKind, EventId, FailureKind, ScopeId, ScopeStatus, +}; use super::coordinator::{coordinate, CoordinateError, RuntimeBoundary}; +use super::external_taint::ExternalTaintMarker; use super::git_snapshot::GitSnapshotService; use super::worktree_lock::WorktreeLock; @@ -81,16 +84,20 @@ fn linked_worktrees_have_independent_locks_and_worktree_ids() { "a linked worktree must resolve its own worktree-specific git dir, giving it a distinct lock and identity path" ); - let main_outcome = coordinate(&main_root, &db_main, &RuntimeBoundary::Flush) - .expect("first observation on the main worktree should succeed"); + let main_outcome = coordinate(&main_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("first observation on the main worktree should succeed"); let held = WorktreeLock::acquire(&main_git_dir, Duration::from_secs(5)) .expect("the main worktree's runtime lock should be acquirable"); - let db_linked = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path).expect( - "a second handle to the same caller-supplied repository-scoped DB path should open", - ); - let linked_outcome = coordinate(&linked_root, &db_linked, &RuntimeBoundary::Flush).expect( + let linked_outcome = coordinate(&linked_root, &RuntimeBoundary::Flush, || { + // A second handle to the same caller-supplied repository-scoped DB path, + // opened through the provider while the main worktree's lock is held. + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect( "coordinate() on the linked worktree must acquire its own distinct runtime lock while the main worktree's lock is still held", ); @@ -139,8 +146,8 @@ fn agent_trace_storage_and_coordinator_observe_the_same_checkout_id() { std::fs::create_dir_all(&state_root).expect("state root should be created"); let coordinator_db_path = repo_root.join("coordinator.db"); - let db = RepositoryAgentTraceDb::new_at(&coordinator_db_path) - .expect("the coordinator's repository DB should open"); + RepositoryAgentTraceDb::new_at(&coordinator_db_path) + .expect("the coordinator's repository DB should open with schema"); let barrier = Arc::new(Barrier::new(2)); @@ -162,8 +169,10 @@ fn agent_trace_storage_and_coordinator_observe_the_same_checkout_id() { }; barrier.wait(); - let outcome = coordinate(&repo_root, &db, &RuntimeBoundary::Flush) - .expect("the coordinator's first observation should succeed"); + let outcome = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&coordinator_db_path) + }) + .expect("the coordinator's first observation should succeed"); let storage_checkout_id = storage_thread .join() @@ -193,8 +202,10 @@ fn a_snapshot_failure_then_recovery_cycle_runs_through_the_public_api() { let db_path = repo_root.join("agent-trace.db"); let db = RepositoryAgentTraceDb::new_at(&db_path).expect("repository DB should open"); - let baseline = coordinate(&repo_root, &db, &RuntimeBoundary::Flush) - .expect("the baseline observation should materialize the worktree"); + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the baseline observation should materialize the worktree"); let worktree_id = baseline.worktree_id.clone(); let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); @@ -207,12 +218,12 @@ fn a_snapshot_failure_then_recovery_cycle_runs_through_the_public_api() { let scope = ScopeId("scope-recovery".to_string()); let failure = coordinate( &repo_root, - &db, &RuntimeBoundary::Start { scope: scope.clone(), event: EventId("evt-during-failure".to_string()), actor_kind: ActorKind::ClaudeCode, }, + || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path), ) .expect_err("a Git snapshot failure against a materialized worktree should be reported"); match failure { @@ -240,8 +251,10 @@ fn a_snapshot_failure_then_recovery_cycle_runs_through_the_public_api() { std::fs::remove_file(&tmp_index_dir) .expect("removing the planted file should let the snapshot service recreate its temp dir"); - let recovered = coordinate(&repo_root, &db, &RuntimeBoundary::Flush) - .expect("the coordinator should recover from the taint and process the boundary"); + let recovered = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the coordinator should recover from the taint and process the boundary"); assert_eq!( recovered.worktree_id, worktree_id, "recovery must operate on the same worktree identity" @@ -259,3 +272,696 @@ fn a_snapshot_failure_then_recovery_cycle_runs_through_the_public_api() { cleanup(&repo_root); } + +#[test] +fn a_successful_coordinate_through_the_public_api_leaves_no_external_taint_marker() { + let repo_root = unique_path("public-success-no-marker"); + init_repo(&repo_root); + // The DB lives outside the worktree so it never perturbs the observed tree. + let db_path = unique_path("public-success-no-marker-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the repository DB should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + + let outcome = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("a first observation through the public entrypoint should succeed"); + assert_eq!( + outcome.revision, 0, + "a first-observation flush should not advance the revision" + ); + assert!( + !marker + .exists() + .expect("marker existence should resolve after a successful coordinate()"), + "a successful coordinate() must clear the external-taint marker it armed" + ); + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&repo_root); +} + +#[test] +#[allow(clippy::too_many_lines)] +fn a_db_open_failure_after_arming_leaves_the_marker_and_the_next_invocation_rebaselines_without_evidence( +) { + let repo_root = unique_path("public-db-open-failure-gap"); + init_repo(&repo_root); + // The DB lives outside the worktree so it never perturbs the observed tree. + let db_path = unique_path("public-db-open-failure-gap-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the repository DB should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + let ok_db = || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path); + + // A trusted baseline at cursor A, then one exclusive AI edit A -> B. + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the baseline observation should materialize the worktree"); + let worktree_id = baseline.worktree_id.clone(); + let scope = ScopeId("scope-across-the-gap".to_string()); + coordinate( + &repo_root, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect("starting the scope should succeed"); + std::fs::write(repo_root.join("work.txt"), b"v1").expect("the A -> B edit should write"); + let advanced = coordinate( + &repo_root, + &RuntimeBoundary::Advance { + scope: scope.clone(), + event: EventId("evt-advance".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect("the advance should commit exactly one event"); + let tree_b = advanced.observed_tree.clone(); + assert!( + advanced.mutation_event.is_some(), + "the exclusive A -> B edit must land one trustworthy event before the gap" + ); + + // A boundary invocation whose DB provider fails after the marker is armed, + // then the working tree keeps moving during the lost interval: B -> C. + let db_failure = coordinate( + &repo_root, + &RuntimeBoundary::Flush, + || -> anyhow::Result { + Err(anyhow::anyhow!("simulated Agent Trace DB open failure")) + }, + ) + .expect_err("a failing DB provider must fail coordinate()"); + assert!( + matches!(db_failure, CoordinateError::AgentTraceDbUnavailable(_)), + "expected AgentTraceDbUnavailable, got {db_failure:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "the armed marker must survive an invocation whose DB provider returned Err" + ); + std::fs::write(repo_root.join("work.txt"), b"v2-during-the-gap").expect("the B -> C edit"); + + // The next successful invocation rebaselines to C with no evidence for the gap. + let recovered = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the follow-up invocation with a working provider should recover, then process its boundary"); + let tree_c = recovered.observed_tree.clone(); + assert_ne!( + tree_c, tree_b, + "the gap edit must have moved the observed tree" + ); + assert!( + recovered.mutation_event.is_none(), + "no mutation evidence may span the interval the DB-open failure fenced off" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "the recovering invocation must clear the marker on success" + ); + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree_id, Some(&scope), None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should exist"); + assert_eq!( + projection.worktree_state.cursor_tree, tree_c, + "recovery must rebaseline the cursor to the follow-up invocation's own observed tree" + ); + assert!(!projection.worktree_state.tainted); + assert_eq!( + projection.scopes.get(&scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "a scope live across the fenced interval must be abandoned afterward" + ); + for revision in 1..=recovered.revision { + if let Some(event) = store + .load_mutation_event(&worktree_id, revision) + .expect("loading a mutation event should succeed") + { + assert_ne!( + event.after_tree, tree_c, + "no MutationEvent may treat an interval ending at the post-gap tree as one trustworthy AI-attributable interval" + ); + } + } + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&repo_root); +} + +#[test] +fn a_stale_marker_rebaselines_to_the_current_tree_abandons_scopes_then_processes_the_boundary() { + let repo_root = unique_path("public-stale-marker-rebaseline"); + init_repo(&repo_root); + // The DB lives outside the worktree so it never perturbs the observed tree. + let db_path = unique_path("public-stale-marker-rebaseline-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the repository DB should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + let ok_db = || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path); + + // Trusted cursor A plus an active scope S. + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the baseline observation should materialize the worktree"); + let worktree_id = baseline.worktree_id.clone(); + let tree_a = baseline.observed_tree.clone(); + let scope = ScopeId("scope-stranded".to_string()); + coordinate( + &repo_root, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect("starting scope S should succeed"); + + // A crashed invocation: the marker was armed and never cleared. + marker + .persist() + .expect("simulating a crashed invocation that armed but never cleared the marker"); + + // The working tree moves on to C while the process was gone. + std::fs::write( + repo_root.join("stranded.txt"), + b"edited-while-the-process-was-gone", + ) + .expect("the A -> C edit should write"); + + // The next invocation inherits the stale marker. + let recovered = coordinate( + &repo_root, + &RuntimeBoundary::Advance { + scope: scope.clone(), + event: EventId("evt-advance".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect("an inherited marker must recover, then process the triggering boundary"); + let tree_c = recovered.observed_tree.clone(); + assert_ne!( + tree_c, tree_a, + "the working tree must have moved during the gap" + ); + assert!( + recovered.mutation_event.is_none(), + "no A -> C evidence may be emitted across the fenced interval" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "a successful recovery must clear the inherited marker" + ); + + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree_id, Some(&scope), None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should exist"); + assert_eq!( + projection.worktree_state.cursor_tree, tree_c, + "recovery must rebaseline the cursor to the current tree C" + ); + assert!(!projection.worktree_state.tainted); + assert_eq!( + projection.scopes.get(&scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "the scope that was live across the gap must be abandoned during recovery" + ); + + // The triggering boundary was processed inside the inheriting invocation: + // a plain follow-up flush finds nothing left to recover or advance. + let stable = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("a plain flush after recovery should succeed"); + assert_eq!( + stable.revision, recovered.revision, + "recovery and the triggering boundary already completed in the inheriting invocation" + ); + assert!(stable.mutation_event.is_none()); + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&repo_root); +} + +#[test] +fn a_first_ever_failed_invocation_that_never_materialized_a_worktree_row_creates_no_evidence() { + let repo_root = unique_path("public-first-ever-failure"); + init_repo(&repo_root); + // The DB lives outside the worktree so it never perturbs the observed tree. + let db_path = unique_path("public-first-ever-failure-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the repository DB should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + + std::fs::write( + repo_root.join("pre-existing.txt"), + b"content before any observation", + ) + .expect("a pre-existing edit should write"); + + // The very first invocation fails opening the DB, after arming the marker. + let first = coordinate( + &repo_root, + &RuntimeBoundary::Flush, + || -> anyhow::Result { + Err(anyhow::anyhow!( + "simulated Agent Trace DB open failure on the first-ever invocation" + )) + }, + ) + .expect_err("the first-ever invocation's DB provider fails"); + assert!( + matches!(first, CoordinateError::AgentTraceDbUnavailable(_)), + "expected AgentTraceDbUnavailable, got {first:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "the first-ever failed invocation still leaves an armed marker" + ); + + // More edits during the still-unobserved interval. + std::fs::write( + repo_root.join("during-the-gap.txt"), + b"more unknown-interval content", + ) + .expect("another unobserved edit should write"); + + // The first *successful* invocation establishes a baseline with no evidence + // for the unknown interval. + let established = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the first successful invocation establishes the baseline"); + assert!( + established.mutation_event.is_none(), + "a worktree with no prior durable row cannot produce evidence for the unknown interval" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "the successful baseline must clear the inherited marker" + ); + + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&established.worktree_id, None, None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should now exist"); + assert_eq!( + projection.worktree_state.cursor_tree, established.observed_tree, + "the baseline is established against the first observed tree" + ); + assert!(!projection.worktree_state.tainted); + assert_eq!(projection.worktree_state.failure_kind, FailureKind::Healthy); + for revision in 0..=established.revision { + assert!( + store + .load_mutation_event(&established.worktree_id, revision) + .expect("loading a mutation event should succeed") + .is_none(), + "no MutationEvent may exist for a worktree whose history began with an unobserved interval" + ); + } + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&repo_root); +} + +#[test] +#[allow(clippy::too_many_lines)] +fn linked_worktrees_keep_independent_external_taint_markers_over_a_shared_db() { + let main_root = unique_path("public-taint-linked-main"); + init_repo(&main_root); + let linked_root = unique_path("public-taint-linked-secondary"); + run_git( + &main_root, + &[ + "worktree", + "add", + "--quiet", + linked_root.to_str().expect("worktree path should be UTF-8"), + ], + ); + + // One shared repository DB, outside either worktree so it never perturbs a + // captured tree. + let db_path = unique_path("public-taint-linked-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the shared repository DB should open"); + + let main_git_dir = resolve_git_dir(&main_root).expect("main git dir should resolve"); + let linked_git_dir = resolve_git_dir(&linked_root).expect("linked git dir should resolve"); + let main_marker = ExternalTaintMarker::new(&main_git_dir); + let linked_marker = ExternalTaintMarker::new(&linked_git_dir); + + // Baseline both worktrees against the one shared DB. + let main_baseline = coordinate(&main_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the main worktree baseline should succeed"); + let linked_baseline = coordinate(&linked_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the linked worktree baseline should succeed"); + assert_ne!(main_baseline.worktree_id, linked_baseline.worktree_id); + + // Strand a live scope in the linked worktree behind an armed marker. + let linked_scope = ScopeId("scope-linked".to_string()); + coordinate( + &linked_root, + &RuntimeBoundary::Start { + scope: linked_scope.clone(), + event: EventId("evt-linked-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path), + ) + .expect("starting the linked worktree's scope should succeed"); + linked_marker + .persist() + .expect("arming the linked worktree's marker should succeed"); + + // A boundary in the MAIN worktree must not observe the linked worktree's marker. + std::fs::write(main_root.join("main-work.txt"), b"v1") + .expect("a main-worktree edit should write"); + coordinate(&main_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the main worktree flush must succeed without inheriting the linked worktree's marker"); + assert!( + !main_marker + .exists() + .expect("marker existence should resolve"), + "the main worktree clears its own marker on success" + ); + assert!( + linked_marker + .exists() + .expect("marker existence should resolve"), + "the main worktree's invocation must not touch the linked worktree's independent marker" + ); + + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the shared DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let linked_mid = store + .load_worktree(&linked_baseline.worktree_id, Some(&linked_scope), None) + .expect("loading the linked worktree row should succeed") + .expect("the linked worktree row should exist"); + assert_eq!( + linked_mid.scopes.get(&linked_scope).map(|s| s.status), + Some(ScopeStatus::Active), + "a marker in the linked worktree must not trigger recovery of the linked worktree from the main worktree's invocation" + ); + + // The linked worktree's own next invocation inherits and recovers. + std::fs::write(linked_root.join("linked-work.txt"), b"v1") + .expect("a linked-worktree edit should write"); + let linked_recovered = coordinate(&linked_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the linked worktree's own invocation recovers from its inherited marker"); + assert!( + linked_recovered.mutation_event.is_none(), + "the linked worktree's conservative recovery emits no evidence for its fenced interval" + ); + assert!( + !linked_marker + .exists() + .expect("marker existence should resolve"), + "the linked worktree clears its marker after its own successful recovery" + ); + + let linked_after = store + .load_worktree(&linked_baseline.worktree_id, Some(&linked_scope), None) + .expect("loading the linked worktree row should succeed") + .expect("the linked worktree row should exist"); + assert_eq!( + linked_after.scopes.get(&linked_scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "the linked worktree's inherited-taint recovery abandons its own live scope" + ); + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&linked_root); + cleanup(&main_root); +} + +#[test] +fn a_snapshot_failure_arms_the_marker_and_the_next_invocation_recovers_once() { + let repo_root = unique_path("public-snapshot-failure-marker-recovery"); + init_repo(&repo_root); + // The DB lives outside the worktree so it never perturbs the observed tree. + let db_path = unique_path("public-snapshot-failure-marker-recovery-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the repository DB should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + let ok_db = || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path); + + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the baseline observation should materialize the worktree"); + let worktree_id = baseline.worktree_id.clone(); + assert!(!marker.exists().expect("marker existence should resolve")); + + // Plant a file where the snapshot service needs its temp-index directory. + let tmp_index_dir = git_dir.join("sce").join("tmp"); + let _ = std::fs::remove_dir_all(&tmp_index_dir); + std::fs::write(&tmp_index_dir, b"not a directory") + .expect("planting a file where the snapshot service expects its temp-index directory"); + + let scope = ScopeId("scope-during-the-failure".to_string()); + let failure = coordinate( + &repo_root, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-during-failure".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect_err("a Git snapshot failure after marker arming should be reported"); + assert!( + matches!( + failure, + CoordinateError::SnapshotFailure { + persisted_taint: true, + .. + } + ), + "expected a SnapshotFailure that durably tainted the worktree, got {failure:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "a snapshot failure after arming must leave BOTH a durable taint and the external-taint marker" + ); + + { + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree_id, None, None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should still exist"); + assert!(projection.worktree_state.tainted); + assert_eq!( + projection.worktree_state.failure_kind, + FailureKind::SnapshotFailure + ); + } + + std::fs::remove_file(&tmp_index_dir) + .expect("removing the planted file should let the snapshot service recreate its temp dir"); + std::fs::write(repo_root.join("work.txt"), b"v1") + .expect("an edit before recovery should write"); + + let recovered = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the coordinator should recover from the taint and process the boundary"); + assert!( + recovered.mutation_event.is_none(), + "the conservative recovery emits no evidence for the fenced interval" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "a successful recovery clears the marker" + ); + + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree_id, None, None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should still exist"); + assert!( + !projection.worktree_state.tainted, + "recovery must clear the durable taint" + ); + assert_eq!(projection.worktree_state.failure_kind, FailureKind::Healthy); + assert_eq!( + projection.worktree_state.cursor_tree, recovered.observed_tree, + "recovery rebaselines the cursor to the recovering invocation's own observed tree" + ); + + // A plain follow-up flush proves the recovery already fully completed once: + // there is nothing left to recover. + let stable = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("a plain flush after recovery should succeed"); + assert_eq!( + stable.revision, recovered.revision, + "the single conservative recovery already completed; a follow-up flush is a no-op" + ); + assert!(stable.mutation_event.is_none()); + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&repo_root); +} + +#[test] +#[allow(clippy::too_many_lines)] +fn a_marker_clear_failure_after_a_durable_boundary_keeps_the_marker_for_a_later_recovery() { + let repo_root = unique_path("public-marker-clear-failure"); + init_repo(&repo_root); + // The DB lives outside the worktree so it never perturbs the observed tree. + let db_path = unique_path("public-marker-clear-failure-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the repository DB should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + let marker_path = git_dir.join("sce").join("mutation-cursor-tainted"); + let ok_db = || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path); + + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the baseline observation should materialize the worktree"); + let worktree_id = baseline.worktree_id.clone(); + + let scope = ScopeId("scope-attributable".to_string()); + coordinate( + &repo_root, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect("starting the scope should succeed"); + std::fs::write(repo_root.join("work.txt"), b"v1") + .expect("an exclusive edit before the boundary should write"); + + let clear_marker_path = marker_path.clone(); + let clear_db_path = db_path.clone(); + let error = coordinate( + &repo_root, + &RuntimeBoundary::Advance { + scope: scope.clone(), + event: EventId("evt-advance".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + move || { + std::fs::remove_file(&clear_marker_path) + .expect("the armed marker file should be present mid-invocation"); + std::fs::create_dir_all(clear_marker_path.join("nested")) + .expect("planting a non-empty directory at the marker path should succeed"); + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&clear_db_path) + }, + ) + .expect_err("clearing a marker that is now a non-empty directory must fail"); + + let committed = match error { + CoordinateError::MarkerClearAfterCommit { committed, .. } => committed, + other => panic!("expected MarkerClearAfterCommit, got {other:?}"), + }; + assert!( + marker.exists().expect("marker existence should resolve"), + "the marker stays logically armed after a post-commit clear failure" + ); + + let (durable_revision, durable_cursor) = { + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree_id, Some(&scope), None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should exist"); + let event = store + .load_mutation_event(&worktree_id, projection.worktree_state.revision) + .expect("loading the committed mutation event should succeed") + .expect("the attributable Advance must have committed one durable event"); + assert_eq!(event.after_tree, projection.worktree_state.cursor_tree); + ( + projection.worktree_state.revision, + projection.worktree_state.cursor_tree.clone(), + ) + }; + + assert_eq!(committed.worktree_id, worktree_id); + assert_eq!(committed.revision, durable_revision); + assert_eq!(committed.observed_tree, durable_cursor); + assert!( + committed.mutation_event.is_some(), + "the committed outcome carried by the error must still expose the MutationEvent" + ); + + std::fs::remove_dir_all(&marker_path).expect("removing the planted directory should succeed"); + marker + .persist() + .expect("re-arming a plain marker file should succeed"); + std::fs::write(repo_root.join("work.txt"), b"v2").expect("a later edit should write"); + + let recovered = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the later invocation recovers from the still-armed marker"); + assert!( + recovered.mutation_event.is_none(), + "the deferred conservative recovery emits no evidence for the interval it could not prove" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "the successful later recovery finally clears the marker" + ); + assert_ne!( + recovered.observed_tree, durable_cursor, + "the later recovery rebaselines to the newer tree" + ); + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&repo_root); +} diff --git a/cli/src/services/sync/sync.rs b/cli/src/services/sync/sync.rs index 32f1b35d..fadc6ee2 100644 --- a/cli/src/services/sync/sync.rs +++ b/cli/src/services/sync/sync.rs @@ -1139,13 +1139,11 @@ mod tests { seed_one_row_per_stream(&db); let server = TestHttpServer::start(); - server.queue_response(CannedResponse::json(200, &state_response(0, 0, 0, 0))); - for _ in 0..4 { - server.queue_response(CannedResponse::json( - 404, - &json!({"message": "unknown ingestion route"}), - )); - } + server.queue_response(CannedResponse::json(200, &state_response(0, 1, 1, 1))); + server.queue_response(CannedResponse::json( + 404, + &json!({"message": "unknown ingestion route"}), + )); let client = test_client(&server); let error = run_sync_against( @@ -1179,9 +1177,9 @@ mod tests { .iter() .filter(|request| request.path == "/agent-trace/ingestion/batch") .count(); - assert!( - (1..=4).contains(&batch_count), - "terminal /batch statuses must not resend batches; observed {batch_count}" + assert_eq!( + batch_count, 1, + "only `messages` should send a /batch request, and a terminal status must not resend it; observed {batch_count}" ); remove_test_db(&db_path); @@ -1240,18 +1238,15 @@ mod tests { let metadata = db .verify_or_initialize_repository_metadata("repo-malformed-batch") .expect("metadata should initialize"); - seed_one_row_per_stream(&db); + seed_messages(&db, 1); let server = TestHttpServer::start(); server.queue_response(CannedResponse::json(200, &state_response(0, 0, 0, 0))); // Syntactically successful but undecodable as `AgentTraceIngestionBatchResponse`. server.queue_response(CannedResponse::json(200, &json!({"unexpected": "shape"}))); - // The four initial stream requests overlap. Messages receives the - // malformed response, while the other streams receive their normal - // responses. Reconciliation then refetches state and resends messages. - server.queue_response(CannedResponse::json(200, &batch_response(1))); - server.queue_response(CannedResponse::json(200, &batch_response(1))); - server.queue_response(CannedResponse::json(200, &batch_response(1))); + // Only messages has pending data in this test. The malformed successful + // batch response must cause an explicit state reconciliation before the + // same batch is retried. server.queue_response(CannedResponse::json(200, &state_response(0, 0, 0, 0))); server.queue_response(CannedResponse::json(200, &batch_response(1))); let client = test_client(&server); @@ -1264,20 +1259,26 @@ mod tests { ) .expect("an undecodable 2xx /batch body should still reconcile via /state and succeed"); + assert_eq!(server.call_count(), 4); + let requests = server.captured_requests(); assert_eq!( - server.call_count(), - 7, - "an undecodable 2xx /batch body must reconcile via /state before resending, not fail immediately" + requests + .iter() + .map(|request| (request.method.as_str(), request.path.as_str())) + .collect::>(), + vec![ + ("POST", "/agent-trace/ingestion/state"), + ("POST", "/agent-trace/ingestion/batch"), + ("POST", "/agent-trace/ingestion/state"), + ("POST", "/agent-trace/ingestion/batch"), + ], + "an undecodable 2xx /batch body must reconcile via /state before retrying the same batch" ); - for stream in [ - report.streams.messages, - report.streams.parts, - report.streams.diff_traces, - report.streams.agent_traces, - ] { - assert_eq!(stream.uploaded, 1); - assert_eq!(stream.final_cursor, 1); - } + assert_eq!(report.streams.messages.uploaded, 1); + assert_eq!(report.streams.messages.final_cursor, 1); + assert_eq!(report.streams.parts.uploaded, 0); + assert_eq!(report.streams.diff_traces.uploaded, 0); + assert_eq!(report.streams.agent_traces.uploaded, 0); remove_test_db(&db_path); } diff --git a/context/cli/mutation-trace-external-taint.md b/context/cli/mutation-trace-external-taint.md new file mode 100644 index 00000000..caef5b1e --- /dev/null +++ b/context/cli/mutation-trace-external-taint.md @@ -0,0 +1,185 @@ +# Mutation-cursor external-taint boundary (`runtime::external_taint`) + +The worktree-local durability boundary for the mutation cursor: a filesystem +marker that a later hook invocation reads as the external signal that the +previous invocation never proved a trustworthy durable completion. This is the +concrete runtime refinement of the abstract `ProtocolState.external_taint` / +`databaseFailure` / `recover` semantics in +[`mutation-trace-protocol.md`](mutation-trace-protocol.md) — it changes no +protocol semantics, adds no database state, and needs no migration. + +Built by the `mutation-cursor-external-taint` plan +(`context/plans/mutation-cursor-external-taint.md`), which extends the +[`mutation-trace-runtime-coordinator.md`](mutation-trace-runtime-coordinator.md) +work. + +## `ExternalTaintMarker` primitive + +`cli/src/services/mutation_trace/runtime/external_taint.rs` — +`ExternalTaintMarker::new(git_dir: &Path)` builds a handle rooted at +`/sce/mutation-cursor-tainted`, the worktree-specific Git directory as +resolved by `checkout::resolve_git_dir`. Two linked worktrees resolve to two +different Git directories and therefore two independent markers. + +The marker file is empty; **its existence is its entire state**: + +- `exists() -> Result` — `symlink_metadata`; a `NotFound` is `Ok(false)`, + any other inspection failure is `Err`. +- `persist() -> Result<()>` — `create_dir_all` the `sce/` directory, + `create`-open the marker without truncation (`write(true).create(true).truncate(false)`, + since the contents carry no meaning), `sync_data()` it, then a best-effort + `#[cfg(unix)]` parent-directory `sync_all` whose error is not propagated. + Idempotent: an existing marker is re-opened and re-synced, never rewritten. +- `clear() -> Result<()>` — `remove_file`, treating `NotFound` as success, then + the same best-effort parent-directory sync. Idempotent. + +Durability mirrors `checkout::persist_checkout_id_inner`: it protects against +process error, non-graceful process exit, `SIGKILL`, and normal runtime +restart — not host power loss or a filesystem-level crash (the parent-directory +sync is best-effort). The marker is never removed via `Drop`; only an explicit +`clear()` removes it. It is never authoritative for normal cursor state. + +Every method — `new`/`exists`/`persist`/`clear` — is now reached by the +`coordinate()` fence (below), so the module carries no `allow(dead_code)`. + +Inline `#[cfg(test)] mod tests` follows the unique-`std::env::temp_dir()`-path +precedent (see [`../patterns.md`](../patterns.md)): marker path is worktree +scoped, the marker survives reconstruction of the handle until an explicit +`clear()`, and `persist`/`clear` are idempotent. + +## The write-ahead fence around `coordinate()` + +The public entrypoint in +[`mutation-trace-runtime-coordinator.md`](mutation-trace-runtime-coordinator.md) +owns the whole protected operation and no longer receives an already-open DB +handle: + +```text +coordinate(repository_root, boundary, open_db) + open_db: impl FnOnce() -> anyhow::Result +``` + +Order inside the held `WorktreeLock`: + +```text +resolve git_dir → acquire WorktreeLock + → ExternalTaintMarker::new(git_dir) + → inherited_external_taint = marker.exists()? + → marker.persist()? ← fence armed here, write-ahead + → get_or_create_checkout_id → WorktreeId + → open_db() ← DB acquired INSIDE the fence + → GitSnapshotService::new + → coordinate_boundary(&db, .., inherited_external_taint) + → marker.clear()? ← only on a successful outcome +``` + +**Safety invariant:** no failure after the marker is armed — including a +failure to open the Agent Trace DB, or to resolve checkout identity — can +disappear without leaving the worktree-local signal for the next invocation. +Arming *before* `open_db()` is the whole point: if the DB open fails, the marker +is already on disk, so a later invocation that opens the DB successfully still +sees the inherited signal instead of trusting a lost interval. + +The marker is cleared only by `coordinate()`'s success path (`marker.clear()` +after an `Ok` outcome). Every error path — snapshot failure, DB provider `Err`, +checkout-identity failure, DB read/write failure, CAS exhaustion, scope-identity +conflict, unexpected error — returns with the marker present. No `Drop` clears +it. + +### `CoordinateError` variants + +- `ExternalTaintMarker { operation: ExternalTaintOperation, source }` where + `ExternalTaintOperation` is `Inspect | Persist`. Both operations run + **before** any checkout-identity, DB, snapshot, or protocol work, so **no + mutation boundary has committed** and there is no `CoordinateOutcome` to + surface. The boundary is aborted fail closed — an unarmed fence must not let + the boundary proceed. +- `MarkerClearAfterCommit { source, committed: Box }` — the + mutation boundary **committed successfully** to the Agent Trace DB (the + `CoordinateOutcome`, including any `MutationEvent`, is durable), but the + trailing `marker.clear()` failed. This is deliberately **not** the same shape + as an `Inspect`/`Persist` failure: the committed outcome is carried in + `committed` so the caller never loses access to it (a future harness must read + it out of the error and still route the `MutationEvent` onward), the `Display` + text says the boundary committed and only cleanup failed, and the marker stays + logically armed so the next invocation conservatively recovers. +- `AgentTraceDbUnavailable(source)` — the caller-supplied `open_db` provider + returned `Err` after the marker was armed. The marker is intentionally left in + place; the lower-level `coordinate_boundary` pipeline is never entered. + +Summary of the pre-commit vs. post-commit distinction: + +```text +Inspect / Persist failure → no mutation boundary committed; + no CoordinateOutcome exists; + boundary aborted fail closed. + +Clear failure → mutation boundary already committed; +(MarkerClearAfterCommit) CoordinateOutcome (with any MutationEvent) + remains available in `committed`; + marker stays armed; + next invocation conservatively recovers. +``` + +### Inherited taint recovery + +`coordinate()` reads `inherited_external_taint` from `marker.exists()` before +arming the marker and threads it into `coordinate_boundary`, which seeds an +invocation-local `external_taint_pending` flag. While that flag is set, each +freshly loaded projection is overlaid with `protocol::database_failure` before +the recovery check, so `protocol::recover` performs exactly one conservative +recovery against the single already-captured snapshot — cursor rebaselined to +the observed tree, revision advanced once, live scopes abandoned, no +`MutationEvent` for the fenced interval — and the triggering boundary is then +processed against the recovered state. A worktree with no durable row yet is +baselined against the observed tree first, then recovered the same way. + +The overlay is never persisted: `DurableTransition` ignores `external_taint` and +`WorktreeProjection::into_protocol_state()` always returns it empty, so passing +the overlaid state as the CAS baseline is safe. A losing recovery CAS keeps +`external_taint_pending` set, so the next reload re-injects the overlay and +recomputes until recovery lands or the retry budget is spent; once it lands the +flag clears, so a later boundary-CAS retry in the same invocation does not +re-trigger recovery. The filesystem marker is never touched here — +`coordinate()`'s success path still owns clearing it. + +A private `coordinate_inner(.., after_recovery)` / `coordinate_boundary_inner(.., +after_load, after_recovery)` seam lets tests inject a failure at the exact +transition *after* the recovery CAS returns `Applied` and *before* the triggering +boundary is prepared (production passes `|_| Ok(())`). It is never exposed +publicly. + +Inline `coordinator.rs` tests drive the public `coordinate()` against real +repositories: a successful call clears the marker; a snapshot failure, a +non-snapshot failure (revision-exhausted recovery), and a DB provider returning +`Err` each leave the marker present; and both marker-I/O failure operations — +`Inspect` (a deterministic `ENOTDIR`, no permission changes) and `Persist` — +fail the call closed before the DB provider is ever invoked. Pipeline tests +cover the inherited-taint overlay directly: one conservative recovery sharing +the single snapshot before the boundary, a first-ever inherited marker with no +worktree row baselining without evidence, a losing recovery CAS re-injecting the +overlay until it applies, and the flag clearing so a post-recovery boundary-CAS +retry does not recover again. +`a_failure_after_recovery_before_boundary_commit_leaves_marker_and_forces_later_recovery` +uses the `after_recovery` seam to prove the recovery-committed / boundary-failed +window: the recovery is durable (cursor rebaselined, scope abandoned, revision +advanced), the triggering boundary is unprocessed with no `MutationEvent`, the +on-disk marker survives, and a later `coordinate()` inherits it and recovers +conservatively again without resurrecting the abandoned scope. +`runtime/tests.rs`'s +`a_marker_clear_failure_after_a_durable_boundary_keeps_the_marker_for_a_later_recovery` +proves an attributable `Advance` commits durably, the returned +`MarkerClearAfterCommit` carries the matching `committed` outcome +(`worktree_id` / `revision` / `observed_tree`, and `mutation_event.is_some()`), +and a later invocation still recovers off the still-armed marker. + +## On-disk layout addition + +```text +/sce/ +└── mutation-cursor-tainted (runtime::external_taint, empty; existence = armed) +``` + +See also: [`mutation-trace-runtime-coordinator.md`](mutation-trace-runtime-coordinator.md), +[`mutation-trace-protocol.md`](mutation-trace-protocol.md), +[`checkout-identity.md`](checkout-identity.md). diff --git a/context/cli/mutation-trace-protocol.md b/context/cli/mutation-trace-protocol.md index 14459949..8bd83068 100644 --- a/context/cli/mutation-trace-protocol.md +++ b/context/cli/mutation-trace-protocol.md @@ -203,7 +203,18 @@ guard — `abandon` resolves it through the referenced scope's own materialized `worktree_id` rather than taking a `WorktreeId` directly — which keeps `external_taint ⊆ ProtocolState.worktrees` an invariant of every state this module can produce, since `database_failure` is the sole path that inserts -into `external_taint`. +into `external_taint`. The concrete runtime refinement of `external_taint` is +the worktree-local `/sce/mutation-cursor-tainted` marker (see +[`mutation-trace-external-taint.md`](mutation-trace-external-taint.md)), armed +write-ahead before Agent Trace DB acquisition and overlaid onto +`database_failure` recovery only when a later invocation inherits it; +`WorktreeProjection::into_protocol_state()` itself always returns an empty +`external_taint`. A pre-protected marker inspect/persist failure means no +mutation boundary committed; a marker-*clear* failure means the boundary already +committed durably — the coordinator surfaces that as +`CoordinateError::MarkerClearAfterCommit`, carrying the committed +`CoordinateOutcome` so no evidence is lost, and leaves the marker armed so the +next invocation still promotes it to protocol `external_taint`. Future responsibility split (mirrors "Runtime scope materialization" above): the coordinator/store layer resolves/materializes worktree identity/state and @@ -213,14 +224,12 @@ loads a `ProtocolState`; `protocol.rs` only transitions already-known ones. The plan's file split anticipated three seams beyond `protocol.rs`. `store.rs`, `runtime/git_snapshot.rs`, and `coordinator.rs` (with its public `coordinate()` -entrypoint) now all exist as real call sites (see -[`mutation-trace-runtime-coordinator.md`](mutation-trace-runtime-coordinator.md)), -with the runtime layer covered by cross-module integration tests; only -harness/command wiring remains: +entrypoint) now all exist as real call sites, covered by cross-module +integration tests; only harness/command wiring remains: ```mermaid flowchart LR - coordinator["coordinator.rs (implemented)\n(imperative shell:\nlock, DB load, Git snapshot,\nCAS/retry, persist)"] + coordinator["coordinator.rs (implemented)\n(imperative shell: lock,\nexternal-taint fence, DB provider,\nGit snapshot, CAS/retry, persist)"] protocol["protocol.rs\n(pure transitions —\nprepare/commit/attribution/\ntaint/abandon/recover\nall implemented)"] git_snapshot["runtime/git_snapshot.rs (implemented)\n(isolated Git snapshot,\ntemporary index, tree capture/diff,\nSCE-owned ref pinning)"] store["store.rs\n(cursor/revision, scopes,\nprocessed events, mutation\nevidence, CAS transaction)"] @@ -230,18 +239,13 @@ flowchart LR coordinator --> store ``` -- **`coordinator.rs`** (implemented) — its public `coordinate()` acquires the - per-worktree lock, derives `WorktreeId` from checkout identity, drives one - Git snapshot, and calls the pure protocol under a bounded CAS-retry loop. -- **`runtime/git_snapshot.rs`** (implemented) — captures an isolated worktree - snapshot and pins it durably; called by `coordinator.rs`. -- **`store.rs`** (implemented) — loads and persists worktree/scope/event state - via a CAS-guarded commit; never remaps an existing `ScopeId`'s - `actor_kind`/`worktree_id` (see "Runtime scope materialization" above). -- **`protocol.rs`** — assumes referenced scopes already exist in - `ProtocolState.scopes`; validates and transitions lifecycle state only. - -`protocol.rs` stays free of any Git object, DB row, or CAS transaction concept. +`coordinator.rs` (see +[`mutation-trace-runtime-coordinator.md`](mutation-trace-runtime-coordinator.md)) +owns the lock, the external-taint fence, the caller-supplied DB provider, one +Git snapshot, and a bounded CAS-retry loop; `store.rs` never remaps an existing +`ScopeId`'s `actor_kind`/`worktree_id`; `protocol.rs` assumes referenced scopes +already exist and stays free of any Git object, DB row, or CAS transaction +concept. ## Authoritative source diff --git a/context/cli/mutation-trace-runtime-coordinator.md b/context/cli/mutation-trace-runtime-coordinator.md index 5228e996..082ed7c7 100644 --- a/context/cli/mutation-trace-runtime-coordinator.md +++ b/context/cli/mutation-trace-runtime-coordinator.md @@ -22,11 +22,10 @@ documented convention. ## Current code surface The per-worktree runtime lock, the isolated Git snapshot service, the -coordinator's internal protocol-integration pipeline, and the public, -lock-wrapped `coordinate()` entrypoint that drives the lock and checkout -identity around that pipeline all exist, with cross-module integration tests -in `runtime/tests.rs` exercising the public API end to end. Only -harness/command wiring remains. +coordinator's protocol-integration pipeline, and the public `coordinate()` +entrypoint (lock, external-taint fence, checkout identity, and DB provider +around that pipeline) all exist, with `runtime/tests.rs` exercising the public +API end to end. Only harness/command wiring remains. - `cli/src/services/mutation_trace/runtime/worktree_lock.rs` — `WorktreeLock::acquire(git_dir: &Path, timeout: Duration) -> @@ -88,34 +87,44 @@ harness/command wiring remains. `Flush` carries nothing — its worktree is always the invocation's own already-resolved one, never caller-supplied) and documents the `(ScopeId, EventId)` replay-identity contract a future harness adapter must - uphold. The public `coordinate(repository_root, db, boundary) -> - Result` entrypoint takes an - already-resolved `RepositoryAgentTraceDb` from its caller — it never - resolves or opens the repository-scoped Agent Trace DB itself — and owns - the critical section: resolve `git_dir` via `checkout::resolve_git_dir`, - acquire the `WorktreeLock` (bounded 10s, held for the whole call), resolve + uphold. The public `coordinate(repository_root, boundary, open_db) -> + Result` entrypoint owns the whole + protected operation. It does **not** receive an already-open DB handle: + `open_db: impl FnOnce() -> anyhow::Result` is a + caller-supplied provider it invokes itself, so DB acquisition falls inside + the external-taint fence. The critical section: resolve `git_dir` via + `checkout::resolve_git_dir`, acquire the `WorktreeLock` (bounded 10s, held + for the whole call), arm the `ExternalTaintMarker` write-ahead, resolve checkout identity via `checkout::get_or_create_checkout_id` and wrap it as `WorktreeId` — no caller-supplied `WorktreeId` or `Boundary` is ever - accepted — then construct `GitSnapshotService` and delegate to the internal, - generic-over-`SnapshotCapture` pipeline. Identity flows - `repository_root → git_dir → WorktreeLock → checkout ID → WorktreeId`; - the `RepositoryAgentTraceDb` is not on that chain. (`coordinate()` is a one-line - delegation to a private `coordinate_inner(.., on_lock_contention: - impl FnOnce())` test seam; production passes a no-op closure.) A - `WorktreeLock` acquisition failure (timeout or I/O) surfaces as - `CoordinateError::LockAcquisition`. The - pipeline does, per invocation: capture and pin + accepted — invoke `open_db()`, construct `GitSnapshotService`, delegate to + the internal generic-over-`SnapshotCapture` pipeline, and clear the marker + only on a successful outcome. Identity flows + `repository_root → git_dir → WorktreeLock → checkout ID → WorktreeId`; the + DB is not on that chain. (`coordinate()` is a one-line delegation to a + private `coordinate_inner(.., open_db, on_lock_contention: impl FnOnce(), + after_recovery: impl FnMut(u32) -> Result<()>)` test seam; production passes a + no-op contention closure and `|_| Ok(())`.) A `WorktreeLock` + acquisition failure surfaces as `CoordinateError::LockAcquisition`; pre-commit + marker-I/O and DB-provider failures have their own fail-closed variants, and a + post-commit `marker.clear()` failure surfaces as + `CoordinateError::MarkerClearAfterCommit { source, committed }` — the boundary + did commit, so the durable `CoordinateOutcome` (with any `MutationEvent`) rides + along in `committed` rather than being lost, and the marker stays armed. See + [`mutation-trace-external-taint.md`](mutation-trace-external-taint.md) for the + fence ordering, the safety invariant, and the `CoordinateError` variants it + adds. The pipeline does, per invocation: capture and pin exactly one Git snapshot; on failure, run a bounded taint-retry loop instead (below) and return without touching the rest of the pipeline; on success, idempotently materialize the worktree row and, for hook boundaries, the scope row; then loop (bounded, `MAX_CAS_RETRY_ATTEMPTS = 5`, no backoff): - load durable state fresh, recover first if the worktree is tainted or needs - rebaseline (its own CAS commit, reusing the one captured tree as the - rebaseline target), then `prepare`/`commit` the triggering boundary against - that state (a second CAS commit) — reloading and recomputing from scratch - on `Conflict`, without ever re-capturing or re-pinning. A settled no-op - result (a stale, rejected, or replayed attempt) is a successful return, not - an error. + load durable state fresh, recover first if the worktree is tainted, needs + rebaseline, or inherited an external-taint marker (overlaid as + `database_failure`; its CAS commit reuses the one captured tree), then + `prepare`/`commit` the triggering boundary against that state (a second CAS + commit) — reloading and recomputing from scratch on `Conflict`, without ever + re-capturing or re-pinning. A settled no-op result (a stale, rejected, or + replayed attempt) is a successful return, not an error. A capture or pin failure is handled by its own bounded taint-retry loop: a fresh `load_worktree` on every iteration, always evaluated after the @@ -128,11 +137,12 @@ harness/command wiring remains. `Conflict`, reporting `persisted_taint: false` only once every bounded attempt has been exhausted. -The runtime lock guards the coordinator's own critical section (snapshot -capture, worktree/scope materialization, recovery, and the CAS retry loop): -`coordinate()` acquires it before resolving checkout identity and holds it -until the call returns. It is held on every `coordinate()` call, unlike the -checkout-identity-creation lock. +The runtime lock guards the coordinator's own critical section (external-taint +marker arming/clearing, snapshot capture, worktree/scope materialization, +recovery, and the CAS retry loop): `coordinate()` acquires it before arming the +marker and resolving checkout identity, and holds it until the call returns. It +is held on every `coordinate()` call, unlike the checkout-identity-creation +lock. ## Two distinct locks, two distinct invariants @@ -153,6 +163,7 @@ On-disk layout so far: ├── checkout-id (services::checkout) ├── checkout-id.lock (services::checkout) ├── mutation-cursor.lock (runtime::worktree_lock) +├── mutation-cursor-tainted (runtime::external_taint, empty; existence = fence armed) └── tmp/ └── index- (runtime::git_snapshot, ephemeral per capture) @@ -199,16 +210,22 @@ taint-retry loop taints an existing worktree, survives a losing CAS before committing on retry, reports `persisted_taint: false` once exhausted, makes no write when no worktree row exists yet, and still finds and taints a worktree another caller materializes concurrently during this invocation's -own failing capture. One further test proves the critical-section -serialization with a real happens-before ordering: `coordinate()` delegates -to a private `coordinate_inner(.., on_lock_contention: impl FnOnce())` that -takes the lock via `worktree_lock::acquire_inner` (T02's seam, `pub(super)`), -and with a first `WorktreeLock` held, a worker's `coordinate_inner` call -observes the real `TryLockError::WouldBlock` branch (signalling a channel -from `on_lock_contention`) while that first guard is still alive, then — once -the guard is dropped — the same invocation acquires the lock and returns -`Ok`. Production `coordinate()` passes a no-op closure, so its code path, -signature, and lock semantics are unchanged. +own failing capture. Further tests drive the public `coordinate()` against +real repositories: the critical-section serialization (a worker's +`coordinate_inner(.., open_db, on_lock_contention)` observes the real +`TryLockError::WouldBlock` branch while a first `WorktreeLock` is held, then +acquires and returns `Ok` once it drops); and the external-taint fence — a +successful call clears the marker, while a snapshot failure, a non-snapshot +failure, a DB-provider `Err`, and an un-armable marker each leave it present +(the last failing closed before the DB provider runs). A further test drives the +private `after_recovery` seam to inject a failure at the exact +recovery-committed / boundary-not-yet-prepared transition and proves the +recovery is durable, the boundary unprocessed with no `MutationEvent`, the +on-disk marker still present, and a later `coordinate()` re-recovering +conservatively off it; `runtime/tests.rs` separately proves an attributable +`Advance` that commits durably then fails its trailing `marker.clear()` surfaces +`MarkerClearAfterCommit` carrying the matching committed outcome (including its +`MutationEvent`). `runtime/tests.rs` is `runtime`'s own `#[cfg(test)] mod tests`, holding cross-module integration tests that drive only the public `coordinate()` API @@ -218,30 +235,29 @@ precedent: two linked worktrees of one repository (different `git_dir` → different lock paths → different `WorktreeId`s) are proven independently locked by holding one worktree's `WorktreeLock` across a synchronous `coordinate()` call for the other and observing that call return `Ok` before -the held guard is dropped — a shared lock could not be acquired while the -guard is alive, and no wall-clock timing is used. The test opens one -repository-scoped DB path itself and hands a separate handle to each -`coordinate()` call (`coordinate()` does not resolve the DB), then asserts -both distinct worktree rows coexist in that one supplied DB, and that a tree -pinned by one worktree's coordinator resolves through the other's `GIT_DIR`. -A first-ever `agent_trace_storage` resolution and a `coordinate()` call on -the same checkout converge on one checkout identity (matching the on-disk -`checkout-id` file); and a full failure/recovery cycle — baseline call, a -snapshot-failing call that durably taints the worktree, then a recovery call -that clears the taint before processing its boundary — runs entirely through -the public entrypoint. +the held guard is dropped. Each call is handed a provider closure that opens +the one shared repository-scoped DB path (`coordinate()` never resolves the +DB), and both distinct worktree rows then coexist in it. A first-ever +`agent_trace_storage` resolution and a `coordinate()` call on the same +checkout converge on one checkout identity; and a full failure/recovery +cycle — baseline call, a snapshot-failing call that durably taints the +worktree, then a recovery call that clears the taint before processing its +boundary — runs entirely through the public entrypoint. ## Status -The per-worktree runtime lock, the isolated Git snapshot service, the -coordinator's internal protocol-integration pipeline (above), and the public, -lock-wrapped `coordinate()` entrypoint (resolving `git_dir`, acquiring -`WorktreeLock`, resolving checkout identity, deriving `WorktreeId`, delegating -to the pipeline) are all implemented, and `runtime/tests.rs` covers the -public `coordinate()` API end to end (above). A `pub(crate)` re-export of -`coordinate()` beyond `runtime` and any harness/command wiring remain future -work tracked by the `mutation-cursor-runtime-coordinator` plan's follow-ups. +The lock, snapshot service, protocol-integration pipeline, and the public +`coordinate()` entrypoint (resolve `git_dir` → `WorktreeLock` → arm the +external-taint marker → checkout identity → caller-supplied DB provider → +pipeline → clear the marker on success) are all implemented, with +`runtime/tests.rs` covering the public API end to end; an inherited external-taint +marker is now overlaid onto `database_failure` recovery on the next invocation. A +`pub(crate)` re-export of `coordinate()` beyond `runtime` and harness/command +wiring remain future work tracked by the `mutation-cursor-external-taint` and +`mutation-cursor-runtime-coordinator` plans. See also: [`mutation-trace-protocol.md`](mutation-trace-protocol.md), [`mutation-trace-store.md`](mutation-trace-store.md), -[`checkout-identity.md`](checkout-identity.md). +[`mutation-trace-external-taint.md`](mutation-trace-external-taint.md) +(the `/sce/mutation-cursor-tainted` write-ahead fence armed by +`coordinate()`), [`checkout-identity.md`](checkout-identity.md). diff --git a/context/context-map.md b/context/context-map.md index 71858fd0..f00bb36d 100644 --- a/context/context-map.md +++ b/context/context-map.md @@ -27,7 +27,8 @@ Feature/domain context: - `context/cli/mutation-trace-revision-refinement.md` (the Quint `revision: int` → Rust `WorktreeState::revision: u64` bounded-integer refinement: the private `next_revision` checked-arithmetic helper `commit`/`taint`/`abandon`/`recover` all route through instead of a raw `+ 1`, so a worktree at `revision: u64::MAX` is a guarded no-op/rejection rather than a silent wrap to `0`) - `context/cli/mutation-trace-quint-connect.md` (`#[cfg(test)]`-only Quint Connect model-based-testing harness in `cli/src/services/mutation_trace/mbt/` continuously checking `protocol.rs` against `spec/mutation_cursor.qnt`: the verification-only `mbtAction`/`MbtAction` record-payload transport excluded from comparison, the operation-identity-vs-`MbtStutter` distinction on guarded/no-op branches with its two deterministic regressions, finite ID mapping, the AC5 comparable-state field list, `randomPrepare` staying a single `step` branch, deterministic/generated (500×30, seed-reproducible) test coverage, and the two Nix checks — generic `checks.cli-tests` and dedicated `checks.mutation-trace-quint-connect` — that both require the pinned Quint binary plus the top-level `spec/` directory in `workspaceSrc`'s Nix fileset) - `context/cli/mutation-trace-store.md` (durable persistence for the mutation-cursor protocol in `cli/src/services/mutation_trace/store.rs`, built by the `mutation-cursor-store-persistence` plan: the one-directional `protocol.rs` -> `DurableTransition::between` (pure structural diff) -> `store.rs` (SQL translation) -> `RepositoryAgentTraceDb` boundary; migration `003_mutation_trace_protocol.sql`'s five tables; `AttemptState`/`external_taint` non-persistence; the 8-byte big-endian `BLOB` revision encoding plus explicit non-`Debug` enum codecs; the bounded hot-path `load_worktree` read vs. the cold-path `load_mutation_event`; `commit`'s single-`BEGIN IMMEDIATE` CAS batch via `TursoDb::execute_transactional_cas_batch`, distinguishing `Conflict`/retryable-transient/deterministic-`Err` outcomes; and the store's non-goals — no Git/filesystem I/O, no attribution/boundary-kind decisions, no retry-after-`Conflict` loop, no terminal-scope garbage collection) -- `context/cli/mutation-trace-runtime-coordinator.md` (imperative-shell runtime layer in `cli/src/services/mutation_trace/runtime/`, built by the `mutation-cursor-runtime-coordinator` plan: the per-worktree OS advisory lock, `runtime::worktree_lock::WorktreeLock::acquire(git_dir, timeout)` at `/sce/mutation-cursor.lock`, bounded `try_lock()` polling with a distinct matchable timeout error and RAII release, and its distinction from the separate checkout-identity-creation lock; and the isolated Git snapshot service, `runtime::git_snapshot::GitSnapshotService` (`capture_tree`/`pin_tree`/`diff_trees`), writing tree/blob objects into the repository's normal object database and pinning durable ones via a create-only, idempotent `refs/sce/mutation-cursor//` ref rather than a private object store; and `runtime::coordinator`'s protocol-integration pipeline (`RuntimeBoundary`/`CoordinateOutcome`/`CoordinateError`, the load → recover-if-needed → prepare/commit CAS-retry loop, and its own bounded snapshot-failure taint-retry loop) plus the public `coordinate(repository_root, db, boundary)` entrypoint that acquires the `WorktreeLock`, derives `WorktreeId` from `checkout::get_or_create_checkout_id`, and drives that pipeline under one held lock; `runtime/tests.rs` covers the public `coordinate()` API end to end against real linked worktrees and a real Agent Trace DB; a `pub(crate)` re-export of `coordinate()` beyond `runtime` and harness/command wiring remain future work) +- `context/cli/mutation-trace-runtime-coordinator.md` (imperative-shell runtime layer in `cli/src/services/mutation_trace/runtime/`, built by the `mutation-cursor-runtime-coordinator` plan: the per-worktree OS advisory lock, `runtime::worktree_lock::WorktreeLock::acquire(git_dir, timeout)` at `/sce/mutation-cursor.lock`, bounded `try_lock()` polling with a distinct matchable timeout error and RAII release, and its distinction from the separate checkout-identity-creation lock; and the isolated Git snapshot service, `runtime::git_snapshot::GitSnapshotService` (`capture_tree`/`pin_tree`/`diff_trees`), writing tree/blob objects into the repository's normal object database and pinning durable ones via a create-only, idempotent `refs/sce/mutation-cursor//` ref rather than a private object store; and `runtime::coordinator`'s protocol-integration pipeline (`RuntimeBoundary`/`CoordinateOutcome`/`CoordinateError`, the load → recover-if-needed → prepare/commit CAS-retry loop, and its own bounded snapshot-failure taint-retry loop) plus the public `coordinate(repository_root, boundary, open_db)` entrypoint that acquires the `WorktreeLock`, arms the external-taint write-ahead fence, derives `WorktreeId` from `checkout::get_or_create_checkout_id`, invokes the caller-supplied `open_db` provider so DB acquisition falls inside that fence, drives the pipeline under one held lock, and clears the marker only on success; `runtime/tests.rs` covers the public `coordinate()` API end to end against real linked worktrees and a real Agent Trace DB; an inherited external-taint marker is overlaid onto `protocol::database_failure` recovery on the next invocation, against the single captured snapshot and re-injected across a losing recovery CAS; a `pub(crate)` re-export of `coordinate()` beyond `runtime` and harness/command wiring remain future work) +- `context/cli/mutation-trace-external-taint.md` (the worktree-local mutation-cursor durability boundary in `cli/src/services/mutation_trace/runtime/external_taint.rs`, built by the `mutation-cursor-external-taint` plan: the `ExternalTaintMarker` primitive — `new(git_dir)`/`exists()`/`persist()`/`clear()` over an empty file at `/sce/mutation-cursor-tainted` whose existence is its entire state, `checkout::persist_checkout_id_inner`-style durability (`sync_data` plus best-effort `#[cfg(unix)]` parent-dir `sync_all`), idempotent persist/clear, `NotFound`-on-clear as success, no `Drop` deletion — as the concrete runtime refinement of the abstract `ProtocolState.external_taint`; armed by the reshaped `coordinate()` entrypoint write-ahead after the `WorktreeLock` and before Agent Trace DB acquisition (a caller-supplied DB provider closure), cleared only on a successful `CoordinateOutcome`, with dedicated fail-closed pre-commit `CoordinateError::ExternalTaintMarker` (`Inspect`/`Persist` only)/`AgentTraceDbUnavailable` variants plus a post-commit `MarkerClearAfterCommit { source, committed }` that carries the durable outcome so a failed trailing clear never hides a committed `MutationEvent`; an inherited marker seeds an invocation-local `external_taint_pending` flag that overlays `protocol::database_failure` onto each freshly loaded projection so `recover` runs once against the captured snapshot, held across a losing recovery CAS and cleared once it lands) - `context/sce/cli-exit-code-contract.md` (stable class-based `sce` process exit-code contract in `cli/src/app.rs`, so automation can branch on failure category without parsing error text) - `context/sce/cli-error-code-taxonomy.md` (stable user-facing `SCE-ERR-*` diagnostic code classes rendered by `cli/src/app.rs`, complementing the numeric exit-code classes) - `context/sce/cli-stdout-stderr-contract.md` (implemented stream contract in `cli/src/app.rs`: command payloads on stdout only, redacted diagnostics on stderr) diff --git a/context/plans/mutation-cursor-external-taint.md b/context/plans/mutation-cursor-external-taint.md new file mode 100644 index 00000000..9345a142 --- /dev/null +++ b/context/plans/mutation-cursor-external-taint.md @@ -0,0 +1,707 @@ +# Plan: mutation-cursor-external-taint + +## Change summary + +Add the external durability boundary for the mutation cursor: a worktree-local +filesystem marker at `/sce/mutation-cursor-tainted`, armed write-ahead +at the start of mutation-cursor boundary processing, that a later invocation +reads as the external signal that the previous invocation never proved a +trustworthy durable completion. This is the concrete runtime refinement of the +already-verified abstract `ProtocolState.external_taint` / `databaseFailure` / +`recover` semantics — it changes no protocol semantics, adds no database state, +and requires no migration. + +The fence must begin at the mutation-cursor runtime boundary, **before** +repository Agent Trace DB acquisition — not after an already-open +`RepositoryAgentTraceDb` has been handed in. The current +`coordinate(repository_root, &db, boundary)` signature arms the fence too late: +if the hook runtime's DB open fails, `coordinate()` is never entered, no marker +is armed, and a later invocation that opens the DB successfully sees no +inherited marker and can treat a lost `A → C` interval as trustworthy evidence. +This plan reshapes the outer `coordinate()` entrypoint to own the whole +protected operation: it resolves `git_dir`, acquires the `WorktreeLock`, arms +the marker, and only then acquires the DB (through a caller-supplied provider), +runs the snapshot / recovery / protocol / CAS pipeline, and clears the marker +only on complete success. The lower-level `coordinate_boundary` / +`coordinate_with_db` pipeline (the #244 snapshot/protocol/store logic) still +receives an already-open `&RepositoryAgentTraceDb` internally; +`MutationTraceStore` and `protocol.rs` never open or resolve a DB. + +Target ordering the plan establishes: + +```text +RuntimeBoundary + → resolve git_dir + → acquire WorktreeLock + → inspect inherited marker + → persist marker + → get/create checkout ID (WorktreeId) + → open RepositoryAgentTraceDb (caller-supplied provider) + → capture + pin one snapshot + → if inherited taint: overlay database_failure, recover against that snapshot + → prepare / commit the triggering boundary + → DB CAS + → complete success + → clear marker +``` + +Safety invariant: **no failure after a mutation-cursor boundary enters its +protected runtime section — including failure to open the Agent Trace DB +itself, or to resolve checkout identity — can disappear without leaving a +worktree-local external-taint signal for the next invocation.** + +All work is confined to `cli/src/services/mutation_trace/runtime/` (a new +`external_taint.rs` primitive, entrypoint reshaping plus arming and +inherited-taint recovery wiring in `coordinator.rs`, and updated integration +tests in `runtime/tests.rs`) and the durable context/spec docs that describe +the coordinator. It extends the existing `mutation-cursor-runtime-coordinator` +work and preserves that coordinator's current lock, snapshot, DB-backed +`SnapshotFailure`, and CAS-retry behavior. No harness or command wiring is part +of this plan, and `coordinate()` stays reachable only from within `runtime`. + +## Acceptance criteria + +How this plan is proven complete. Each criterion is observable and names the +check that proves it. `/validate` runs these checks; no task in the stack +performs final validation. + +- [x] AC1: A marker is stored beneath the caller-supplied worktree-specific Git + directory (`/sce/mutation-cursor-tainted`), survives reconstruction + of the marker handle, and two distinct `git_dir` inputs derive independent + marker paths and marker state. (Actual linked-worktree independence over a + real `git worktree add` pair is AC12.) + - Validate: `runtime::external_taint::tests::marker_is_worktree_scoped` and + `runtime::external_taint::tests::marker_persists_until_explicitly_cleared` +- [x] AC2: `persist` then `persist` then `clear` then `clear` all succeed; + `clear` on an absent marker is success. + - Validate: `runtime::external_taint::tests::persist_and_clear_are_idempotent` +- [x] AC3: A public `coordinate()` call arms its marker internally and clears it + only after a successful `CoordinateOutcome`. + - Validate: `runtime::coordinator::tests::public_coordinate_clears_marker_on_success` + and `runtime::tests::a_successful_coordinate_through_the_public_api_leaves_no_external_taint_marker` +- [x] AC4: After marker arming, any coordinator error leaves the marker present — + proven for the snapshot-failure path and deterministic non-snapshot failure + paths. + - Validate: `runtime::coordinator::tests::public_coordinate_leaves_marker_after_a_snapshot_failure`, + `runtime::coordinator::tests::public_coordinate_leaves_marker_after_a_non_snapshot_failure`, + and `runtime::coordinator::tests::public_coordinate_leaves_marker_when_the_db_provider_fails` +- [x] AC5: Given durable cursor A, an active scope S, an inherited marker, and a + current worktree C, the next invocation rebaselines to C, emits no A→C + mutation evidence, abandons S, then processes its triggering boundary. + - Validate: `runtime::tests::a_stale_marker_rebaselines_to_the_current_tree_abandons_scopes_then_processes_the_boundary` + and `runtime::coordinator::tests::inherited_external_taint_recovers_once_before_the_boundary` +- [x] AC6: External-taint recovery and triggering-boundary processing use the + same `observed_tree`; no second Git snapshot occurs. + - Validate: `runtime::coordinator::tests::inherited_external_taint_recovers_once_before_the_boundary` + (call-counting `FakeSnapshotCapture` asserting exactly one `capture` and one `pin`) +- [x] AC7: A losing recovery CAS re-injects external taint on reload and + recomputes recovery until `Applied` or retry exhaustion; the marker remains + present throughout the retry loop. + - Validate: `runtime::coordinator::tests::a_losing_recovery_cas_reinjects_external_taint_until_it_applies` + and `runtime::coordinator::tests::a_landed_recovery_clears_the_flag_so_a_boundary_cas_retry_does_not_re_recover`; + on-disk-marker survival across the retry loop is + `runtime::tests::a_snapshot_failure_arms_the_marker_and_the_next_invocation_recovers_once` +- [x] AC8: A failure injected after recovery has committed but before the + triggering boundary completes leaves recovery durable, the boundary + incomplete, and the marker still present; a later invocation recovers + conservatively again. A post-commit `marker.clear()` failure likewise keeps + the marker armed and returns the committed `CoordinateOutcome` inside + `CoordinateError::MarkerClearAfterCommit` rather than a boundary failure. + - Validate: `runtime::coordinator::tests::a_failure_after_recovery_before_boundary_commit_leaves_marker_and_forces_later_recovery` + and `runtime::tests::a_marker_clear_failure_after_a_durable_boundary_keeps_the_marker_for_a_later_recovery` +- [x] AC9: A marker that survives an invocation which never materialized a + worktree row causes the next successful invocation to establish a baseline + with no evidence for the unknown interval. + - Validate: `runtime::coordinator::tests::inherited_external_taint_with_no_worktree_row_baselines_without_evidence` + and `runtime::tests::a_first_ever_failed_invocation_that_never_materialized_a_worktree_row_creates_no_evidence` +- [x] AC10: A worktree with live scopes and inherited external taint persists + those scopes as `Abandoned` during recovery and never treats them as eligible + exclusive attribution after the unknown interval. + - Validate: `runtime::coordinator::tests::inherited_external_taint_recovers_once_before_the_boundary` + (asserts `ScopeStatus::Abandoned` after recovery) and + `runtime::coordinator::tests::recovers_from_snapshot_failure_taint_abandoning_live_scopes` +- [x] AC11: End to end, no `MutationEvent` treats an interval spanning an + incomplete/failed invocation (baseline A, scope start, edit→B, failed + invocation, edit→C, next successful invocation) as one trustworthy + AI-attributable A/B→C interval. + - Validate: `runtime::tests::a_db_open_failure_after_arming_leaves_the_marker_and_the_next_invocation_rebaselines_without_evidence` + (per-revision `load_mutation_event` sweep asserting no event ends at the post-gap tree) +- [x] AC12: An external-taint marker in linked worktree A does not trigger + recovery in linked worktree B, even with a shared repository Agent Trace DB. + - Validate: `runtime::tests::linked_worktrees_keep_independent_external_taint_markers_over_a_shared_db` +- [x] AC13: When marker inspection or marker persistence cannot be established, + `coordinate()` returns a distinct external-taint marker error before any + checkout-identity, DB, snapshot, or protocol processing. + - Validate: `runtime::coordinator::tests::public_coordinate_fails_closed_when_the_marker_cannot_be_armed` + and `runtime::coordinator::tests::public_coordinate_fails_closed_when_marker_inspection_fails` +- [x] AC14: DB acquisition is inside the external-taint fence. Once + mutation-cursor boundary processing has acquired the `WorktreeLock` and armed + the marker, a failure to resolve or open the repository Agent Trace DB (the + caller-supplied provider returns `Err`) makes `coordinate()` return an error + with the marker still present, even though no `RepositoryAgentTraceDb` was + ever available and the lower-level coordinator pipeline was never entered. The + follow-up invocation, given a working DB provider, snapshots the current tree, + runs external-taint recovery, and produces no evidence across the lost + interval. + - Validate: `runtime::tests::a_db_open_failure_after_arming_leaves_the_marker_and_the_next_invocation_rebaselines_without_evidence` + (marker armed → provider `Err` → error returned → marker still present → follow-up recovery) + and `runtime::coordinator::tests::public_coordinate_leaves_marker_when_the_db_provider_fails` + +### Full validation + +Repository-wide checks `/validate` runs after the last task, regardless of +which criterion they map to. + +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::` +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` +- `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings` +- `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check` +- `nix flake check` +- `nix run .#pkl-check-generated` +- Confirm the existing Quint Connect model-based-testing harness + (`checks.cli-tests` / `checks.mutation-trace-quint-connect`) stays green; + no `spec/mutation_cursor.qnt` behavior change is expected. + +### Context sync + +- `context/cli/mutation-trace-runtime-coordinator.md` — document the + `ExternalTaintMarker` primitive and its worktree-scoped path; the reshaped + `coordinate()` entrypoint that owns `WorktreeLock`, marker arming, the + caller-supplied DB provider, the snapshot/pipeline, and marker clear; the + arm-before-DB-acquisition ordering and the safety invariant; the + inherited-vs-armed distinction and the invocation-local `external_taint_pending` + overlay onto `database_failure`; the new pre-commit marker-I/O and + DB-provider-failure `CoordinateError` variants and their fail-closed + semantics, plus the post-commit `MarkerClearAfterCommit` variant that carries + the committed `CoordinateOutcome`; and the added `/sce/` + on-disk-layout entry. +- `context/cli/mutation-trace-protocol.md` — note that the concrete runtime + refinement of `ProtocolState.external_taint` is the stale worktree-local + marker, armed write-ahead before DB acquisition and promoted to protocol + external taint only when inherited by a later invocation. +- `context/context-map.md` — refresh the coordinator domain-file annotation. +- `spec/mutation_cursor.md` — record that the abstract `externalTaint` marker's + concrete refinement is `/sce/mutation-cursor-tainted`, armed + write-ahead at the start of the protected runtime section (before Agent Trace + DB acquisition) and becoming protocol external taint only when inherited. +- Verify-only pass over `context/overview.md`, `context/architecture.md`, + `context/glossary.md`, `context/patterns.md`. + +### Deviations from stated scope + +- **Unrelated sync-test stabilization (test-only).** During full-suite + validation, two pre-existing sync tests were found to depend on concurrent + stream request ordering: + + - `services::sync::sync::tests::terminal_batch_status_fails_without_state_reconciliation` + previously seeded all four streams while asserting that `messages` must be + the stream whose terminal error is observed first. The fixture now advances + the other three cursors (`state_response(0, 1, 1, 1)`), so only `messages` + sends one `/batch` request (`assert_eq!(batch_count, 1)`). + + - `services::sync::sync::tests::malformed_2xx_batch_response_still_reconciles_via_state` + previously seeded all four streams and depended on concurrent requests + consuming canned responses in a particular order. It now seeds only one + `messages` row and deterministically asserts the intended request sequence: + `/state → /batch → /state → /batch`. + + No sync production behavior changed. These are test-only determinism fixes + discovered during PR validation and are not part of the external-taint + architecture. Recorded here rather than silently violating the + "mutation-cursor runtime + docs only" scope statement. +- **PR-review follow-up (T05, post-stack).** After the T01–T04 stack, PR #245 + review added: a distinct `CoordinateError::MarkerClearAfterCommit { source, + committed: Box }` so a post-commit `marker.clear()` failure + no longer collapses into the same shape as a pre-commit inspect/persist + failure and can no longer hide a committed `MutationEvent` (`ExternalTaintOperation` + loses its `Clear` arm); a private `coordinate_inner(.., after_recovery)` / + `coordinate_boundary_inner(.., after_recovery: FnMut(u32) -> Result<()>)` seam + and the AC8 regression + `a_failure_after_recovery_before_boundary_commit_leaves_marker_and_forces_later_recovery`; + removal of the now-obsolete module-wide `#![allow(dead_code)]` in + `external_taint.rs`; and the acceptance-criteria / `Validate:` bookkeeping fix + above. + +## Task context synchronization lifecycle + +Persist this field in every plan; this is durable plan state, not chat state: + +- **Task context synchronization:** every task carries `pending | synced | blocked`. + A completed task must be `synced` before another task can start or the plan can + finish. +- For `blocked`, record **Blocker**, **Required action**, and **Retry condition** + beside the status. Never infer `synced` from conversation history; write every + lifecycle transition to the plan file. + +## Constraints and non-goals + +- **In scope:** `cli/src/services/mutation_trace/runtime/external_taint.rs` + (new), `cli/src/services/mutation_trace/runtime/mod.rs`, + `cli/src/services/mutation_trace/runtime/coordinator.rs` (including reshaping + the public `coordinate()` signature and updating its existing call sites in + `runtime/tests.rs`), `cli/src/services/mutation_trace/runtime/tests.rs`; + docs-only edits to `context/cli/mutation-trace-runtime-coordinator.md`, + `context/cli/mutation-trace-protocol.md`, `context/context-map.md`, and + `spec/mutation_cursor.md`. +- **Out of scope:** ref reconciliation; harness adapters and any + hook/command/`diff_traces` wiring; a `pub(crate)` re-export of `coordinate()`; + redesigning Agent Trace storage resolution beyond, at most, a narrow split + that lets the marker be armed before the existing DB-open primitive runs; + changes to `protocol.rs`, `spec/mutation_cursor.qnt`, or the Quint refinement + matrix; any `store.rs` / SQL / migration change; a daemon; cross-machine + locking; snapshot-ref reclamation; new Agent Trace evidence formats. +- **Constraints:** + - No new Cargo dependencies unless an unavoidable platform issue is discovered. + - The DB CAS stays the protocol linearization point. + - The outer `coordinate()` entrypoint owns `WorktreeLock`, + `ExternalTaintMarker`, the DB-provider call, the snapshot service, the + coordinator pipeline, and the marker clear. DB acquisition is supplied to + `coordinate()` as a caller-provided `FnOnce` provider (or equivalent seam), + not resolved by `coordinate()` itself — repository identity and remote come + from config the coordinator does not read. + - `MutationTraceStore` and `protocol.rs` never open or resolve a DB; the + lower-level `coordinate_boundary` / `coordinate_with_db` pipeline still + receives an already-open `&RepositoryAgentTraceDb`. + - The marker is never armed before the `WorktreeLock` is held, and + same-worktree marker state is inspected/persisted/cleared only while that + lock is held. + - The filesystem marker is never authoritative for normal cursor state. + - No RAII/`Drop`-based marker deletion; only a successful `CoordinateOutcome` + clears the marker. + - `WorktreeProjection::into_protocol_state()` still returns an empty + `external_taint`; the filesystem overlay is applied by runtime code only. + - Marker durability follows the existing + `checkout::persist_checkout_id_inner` style (`fsync` the marker file, + best-effort `#[cfg(unix)]` parent-directory `sync_all`). +- **Non-goal:** introducing a new protocol `FailureKind`; making + `WorktreeProjection::into_protocol_state()` read filesystem state; opening the + DB before arming the marker (that would leave DB-open failure uncovered, which + is the specific failure this plan exists to close); making the protocol or + store layer responsible for opening or resolving the DB; broadening the + durability claim to host power loss / filesystem crash. + +## Assumptions + +The user's change request states "exact names may change", "the exact type is +flexible", and "this exact API is NOT mandatory"; the following are recorded +local choices, not new requirements. + +- `coordinate()` is reshaped to + `coordinate(repository_root: &Path, boundary: &RuntimeBoundary, open_db: impl FnOnce() -> anyhow::Result) -> Result`. + The private `coordinate_inner(.., on_lock_contention, open_db)` test seam is + kept. The lower-level pipeline function is renamed/kept as + `coordinate_boundary` (or `coordinate_with_db`) taking `&RepositoryAgentTraceDb` + plus `inherited_external_taint`. A future harness adapter builds the provider + closure around `agent_trace_storage::resolve_agent_trace_storage_for_hook_runtime` + and hands `coordinate()` the resulting `.db`. +- Pre-commit marker I/O failures surface as a dedicated `CoordinateError` + variant, e.g. + `ExternalTaintMarker { operation: ExternalTaintOperation, source: anyhow::Error }` + with `ExternalTaintOperation { Inspect, Persist }`. A post-commit + `marker.clear()` failure surfaces as a *distinct* variant that carries the + already-committed outcome, e.g. + `MarkerClearAfterCommit { source: anyhow::Error, committed: Box }` + (see the PR #245 review deviation below) — it is never collapsed into the + inspect/persist shape, because those happen before any protected work and a + clear failure happens after a durable commit. A DB-provider failure after the + marker is armed surfaces as a separate variant, e.g. + `CoordinateError::AgentTraceDbUnavailable(anyhow::Error)`, and intentionally + leaves the marker in place. +- The new primitive lives at `runtime/external_taint.rs` as + `ExternalTaintMarker`, backed by an empty file at + `/sce/mutation-cursor-tainted`; its existence is the entire state. +- Marker durability protects against process error, non-graceful process exit, + `SIGKILL`, and normal runtime restart: `persist()` creates and `fsync`s the + marker file, `clear()` removes it, and both do a best-effort `#[cfg(unix)]` + parent-directory `sync_all` whose error is not propagated (mirroring + `checkout::persist_checkout_id_inner`). The plan does not claim durability + across host power loss or a filesystem-level crash, because that + parent-directory sync is best-effort. +- Test module and function names follow the paths named in the acceptance + criteria (`runtime::external_taint::tests::*`, `runtime::tests::*`). + +## Task stack + +- [x] T01: `Add the external-taint marker primitive` (status:done) + - Task ID: T01 + - Completed: 2026-08-30 + - Files changed: + - `cli/src/services/mutation_trace/runtime/external_taint.rs` (new) — + `ExternalTaintMarker` with `new`/`exists`/`persist`/`clear`, local + `SCE_RUNTIME_DIR` / `MARKER_FILE` consts, checkout-identity-style + durability, cfg-gated best-effort parent-dir sync, inline `#[cfg(test)] + mod tests`. + - `cli/src/services/mutation_trace/runtime/mod.rs` — `mod external_taint;`. + - Result: New primitive backed by an empty file at + `/sce/mutation-cursor-tainted`; existence is the entire state. + `persist()` does `create_dir_all` + non-truncating `create` open + (`write(true).create(true).truncate(false)` — marker contents carry no + meaning) + `sync_data()` + best-effort `#[cfg(unix)]` parent-dir `sync_all` + (error swallowed); `clear()` does `remove_file` (`NotFound` → `Ok`) + the + same best-effort dir sync; `exists()` via `symlink_metadata`. No `Drop` + deletion. Module carries `#![allow(dead_code)]` (per `services/capabilities.rs` + precedent) since nothing wires it in until T02. No `coordinator.rs`, + protocol, store, or error-type change. + - Verify (re-run after the PR #245 review fixes — `truncate` removal, AC1 wording): + - `test ...runtime::external_taint` — PASS (3 passed: `marker_is_worktree_scoped`, + `marker_persists_until_explicitly_cleared`, `persist_and_clear_are_idempotent`). + - `test ...runtime::` — PASS (38 passed, 0 failed). + - `clippy --all-targets -- -D warnings` — PASS (clean). + - `fmt -- --check` — PASS (clean). + - Context impact: docs-update-needed. Introduces a new runtime module + (`ExternalTaintMarker`) and a new durable on-disk artifact + (`/sce/mutation-cursor-tainted`). Affected durable context: + `context/cli/mutation-trace-runtime-coordinator.md` (primitive + on-disk + layout entry), `spec/mutation_cursor.md` (concrete refinement of the + abstract `externalTaint` marker). Matches the plan's Context sync section; + no behavior reaches the coordinator or protocol yet. + - Scope: In — new `cli/src/services/mutation_trace/runtime/external_taint.rs` + defining `ExternalTaintMarker` with `new(git_dir)`, `exists()`, `persist()`, + `clear()`; empty marker file at `/sce/mutation-cursor-tainted`; + checkout-identity-style durability (`fsync` the file on create, best-effort + `#[cfg(unix)]` parent-dir `sync_all` on create and remove); `NotFound` on + clear treated as success; no `Drop` deletion; registration in + `runtime/mod.rs`. Out — any `coordinator.rs` change, any protocol/store/ + error-type change, harness wiring. + - Dependencies: none + - Done when: the type compiles and is registered under the module's existing + `#[allow(dead_code)]` precedent; `persist`/`clear` are idempotent; the path + is derived from the caller-supplied worktree `git_dir`; inline + `#[cfg(test)] mod tests` (unique `std::env::temp_dir()` paths, per + `context/patterns.md`) covers persistence across marker-value reconstruction, + idempotent persist, idempotent clear, and two `git_dir`s resolving to + independent marker paths. + - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::external_taint`; + `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings`; + `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check`. + - Context synchronization: synced + +- [x] T02: `Arm the write-ahead fence around the full runtime boundary, DB acquisition included` (status:done) + - Task ID: T02 + - Completed: 2026-08-30 + - Files changed: + - `cli/src/services/mutation_trace/runtime/coordinator.rs` — reshaped the + public `coordinate()` to take a caller-supplied + `open_db: impl FnOnce() -> anyhow::Result` provider + instead of `&RepositoryAgentTraceDb`; `coordinate_inner` now resolves + `git_dir` → acquires `WorktreeLock` → constructs `ExternalTaintMarker` → + reads `inherited_external_taint = marker.exists()?` → `marker.persist()?` → + runs the new `coordinate_protected` (checkout identity → `open_db()` → + `GitSnapshotService` → `coordinate_boundary`) → `marker.clear()?` only on + `Ok`; added `ExternalTaintOperation { Inspect, Persist, Clear }` and the + `CoordinateError::ExternalTaintMarker { operation, source }` and + `CoordinateError::AgentTraceDbUnavailable(_)` variants plus their `Display` + arms; `coordinate_boundary` gained an unused `_inherited_external_taint: + bool` param (consumed in T03); updated the `two_threads_…_serialize` + `coordinate_inner` call site and added six inline tests + (`public_coordinate_clears_marker_on_success`, + `public_coordinate_leaves_marker_after_a_snapshot_failure`, + `public_coordinate_leaves_marker_after_a_non_snapshot_failure`, + `public_coordinate_fails_closed_when_the_marker_cannot_be_armed` + (`ExternalTaintOperation::Persist` path), + `public_coordinate_fails_closed_when_marker_inspection_fails` + (`ExternalTaintOperation::Inspect` path — holds the runtime lock, swaps + `/sce` from a directory to a regular file at the worker's + lock-contention point so `marker.exists()` hits a deterministic `ENOTDIR` + with no permission changes, asserts the DB provider is never called), + `public_coordinate_leaves_marker_when_the_db_provider_fails`). + - `cli/src/services/mutation_trace/runtime/tests.rs` — updated all five + `coordinate()` call sites to pass a provider closure + (`|| RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&path)`). + - Result: The external-taint fence now spans the whole protected runtime + section. `coordinate()` arms the worktree-local marker after `WorktreeLock` + acquisition and before checkout-identity, DB-provider, snapshot, and + protocol work; a successful `CoordinateOutcome` clears it; every failure + after arming (snapshot failure, DB provider `Err`, revision exhaustion, CAS + exhaustion, scope conflict, DB read/write, unexpected) returns with the + marker present; both the `ExternalTaintOperation::Inspect` and + `ExternalTaintOperation::Persist` marker-I/O failure paths fail closed with + the `CoordinateError::ExternalTaintMarker` error before any checkout, + DB-provider, snapshot, or protocol work — each proven by a dedicated + deterministic regression test (AC13). `MutationTraceStore` and + `protocol.rs` are untouched; `coordinate_boundary` still receives an + already-open `&RepositoryAgentTraceDb`. Inherited-taint recovery mapping is + still T03 (`_inherited_external_taint` is threaded but unused). + - Verify: + - `test ...services::mutation_trace::runtime::coordinator` — PASS (21 passed, + 0 failed; `public_coordinate_fails_closed_when_marker_inspection_fails` + also re-run 5× for determinism). + - `test ...services::mutation_trace::runtime::` — PASS (44 passed, 0 failed). + - `clippy --all-targets -- -D warnings` — PASS (clean). + - `fmt -- --check` — PASS (clean). + - Context impact: docs-update-needed. Reshapes the public `coordinate()` + entrypoint (caller-supplied DB provider, `WorktreeLock`/marker ownership, + arm-before-DB-acquisition ordering, marker clear on success only) and adds + two `CoordinateError` variants with fail-closed semantics. Affected durable + context: `context/cli/mutation-trace-runtime-coordinator.md` (reshaped + entrypoint, marker arming, DB provider, new error variants, safety + invariant), `context/cli/mutation-trace-protocol.md` (write-ahead marker as + concrete refinement armed before DB acquisition), `context/context-map.md` + (coordinator annotation refresh), `spec/mutation_cursor.md` (write-ahead + timing of the concrete `externalTaint` refinement). Matches the plan's + Context sync section. No protocol semantics, DB state, or migration changed. + - Scope: In — `runtime/coordinator.rs`: reshape the public `coordinate()` so + it no longer receives an already-open `&RepositoryAgentTraceDb` but a + caller-supplied DB provider (`impl FnOnce() -> anyhow::Result` + or equivalent seam); reorder the outer path to resolve `git_dir` → acquire + `WorktreeLock` → construct `ExternalTaintMarker` → read + `inherited_external_taint = marker.exists()?` → `marker.persist()?` → + `get_or_create_checkout_id` (`WorktreeId`) → invoke the DB provider → + construct `GitSnapshotService` → run the lower-level pipeline + (`coordinate_boundary` / `coordinate_with_db`, taking `&RepositoryAgentTraceDb` + plus `inherited_external_taint`, unused until T03) → `marker.clear()?` only + on `Ok`, leaving the marker on every `Err`; add a distinct marker-I/O + `CoordinateError` variant (inspect/persist failure returns before checkout, + DB, snapshot, or protocol work; clear failure after a successful boundary + returns `Err` with the marker left in place) and a distinct + DB-provider-failure variant that also leaves the marker; update the existing + `coordinate()` call sites in `runtime/tests.rs` to pass a provider closure. + Out — mapping inherited taint into recovery (T03); new integration scenarios + (T04). + - Dependencies: T01 + - Done when: every `coordinate()` call arms the marker after `WorktreeLock` + acquisition and before checkout-identity, DB-provider, snapshot, and + protocol work; a successful `CoordinateOutcome` clears it; every path after + arming leaves it present — snapshot failure, DB provider returning `Err`, + checkout-identity failure, DB read/write failure, CAS exhaustion, + scope-identity conflict, unexpected error; marker inspect/persist failure + returns the new marker error before any of that work; the private + `coordinate_inner` test seam and `on_lock_contention` closure are preserved; + `coordinator.rs` inline tests prove success→cleared, the snapshot-failure + path and one deterministic non-snapshot failure path both retaining the + marker, the fail-closed inspect/persist error path, and a fake DB provider + returning `Err` after arming leaving the marker present. + - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::coordinator`; + `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::`; + `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings`; + `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check`. + - Context synchronization: synced + +- [x] T03: `Map inherited marker into protocol recovery` (status:done) + - Task ID: T03 + - Completed: 2026-08-30 + - Files changed: + - `cli/src/services/mutation_trace/runtime/coordinator.rs` — renamed + `coordinate_boundary`'s `_inherited_external_taint` param to + `inherited_external_taint` and split the body into a 5-arg + `coordinate_boundary` wrapper plus a private + `coordinate_boundary_inner(.., after_load: FnMut(u32), after_recovery: FnMut(u32))` + test seam (mirroring the existing `coordinate_inner` / + `run_taint_retry_loop_inner` seam precedent; production passes no-ops). + Seeded `let mut external_taint_pending = inherited_external_taint;` before + the CAS loop; after each `load_worktree` the loop overlays + `state = protocol::database_failure(&state, worktree_id)` while + `external_taint_pending`, so the existing `needs_recovery` / + `protocol::recover` path runs against the one already-captured + `observed_tree`. On recovery-CAS `Applied` the flag is cleared (and + `after_recovery` fires); on `Conflict` the existing `continue` keeps it set + so the next reload re-injects the overlay. Added four inline tests + (`inherited_external_taint_recovers_once_before_the_boundary`, + `inherited_external_taint_with_no_worktree_row_baselines_without_evidence`, + `a_losing_recovery_cas_reinjects_external_taint_until_it_applies`, + `a_landed_recovery_clears_the_flag_so_a_boundary_cas_retry_does_not_re_recover`). + - Result: An inherited external-taint marker (T02's `inherited_external_taint`) + now drives protocol recovery. `external_taint_pending` is invocation-local + and seeded from the inherited flag; while set, `protocol::database_failure` + is overlaid onto every freshly loaded projection before the recovery check, + so `protocol::recover` performs exactly one conservative recovery (cursor := + observed tree, revision += 1, external taint cleared, live scopes → + `Abandoned`, no `MutationEvent` for the fenced interval) against the single + captured snapshot, then the triggering boundary is processed against the + recovered state. The overlay is never persisted (`DurableTransition` ignores + `external_taint`; `into_protocol_state()` always returns it empty). A losing + recovery CAS re-injects the overlay on the next reload and recomputes until + `Applied` or retry exhaustion; once recovery lands, the flag is clear so a + later boundary-CAS retry in the same invocation does not re-trigger recovery. + A first-ever inherited marker with no durable worktree row is baselined + against the observed tree by the existing `initialize_worktree`, then + conservatively recovered once. The filesystem marker is never touched here — + `coordinate_inner`'s success path still owns clearing it. No `protocol.rs`, + `store.rs`, SQL, or migration change. + - Verify: + - `test ...services::mutation_trace::runtime::coordinator` — PASS (25 passed, + 0 failed). + - `test ...services::mutation_trace::runtime::` — PASS (48 passed, 0 failed). + - `clippy --all-targets -- -D warnings` — PASS (clean). + - `fmt -- --check` — PASS (clean, after `cargo fmt`). + - Context impact: docs-update-needed. Adds the invocation-local + `external_taint_pending` overlay onto `database_failure` and its + inherited-vs-armed recovery semantics to the coordinator pipeline; no + protocol semantics, DB state, error variants, or public signatures changed + beyond T02's already-recorded reshape. Affected durable context: + `context/cli/mutation-trace-runtime-coordinator.md` (inherited-taint overlay + onto `database_failure`, one-recovery-per-inherited-marker, flag lifecycle + across recovery-CAS conflict/apply), `context/cli/mutation-trace-protocol.md` + (the stale marker becomes protocol external taint only when inherited by a + later invocation), `spec/mutation_cursor.md` (same inherited-only promotion). + Matches the plan's Context sync section. + - Deviations: added the private `coordinate_boundary_inner` `after_load` / + `after_recovery` test seams — consistent with the existing + `coordinate_inner(on_lock_contention)` and + `run_taint_retry_loop_inner(after_load)` precedent — because deterministically + forcing a recovery-CAS conflict, and a boundary-CAS conflict after a landed + recovery, is otherwise only reachable through non-deterministic thread races. + - Scope: In — `runtime/coordinator.rs`: invocation-local + `external_taint_pending`, seeded from `inherited_external_taint` and threaded + into the lower-level pipeline; when set, overlay `protocol::database_failure` + onto each freshly loaded projection before the existing `needs_recovery` + check, so `protocol::recover` runs against the single already-captured + `observed_tree`; keep `external_taint_pending` set across a recovery-CAS + `Conflict` and clear the in-memory flag only on recovery-CAS `Applied`; + never touch the filesystem marker here (T02's success path owns clearing); + first-ever inherited marker with no durable worktree row initializes the + worktree against the observed tree, then recovers. Out — filesystem marker + writes; new integration scenarios (T04). + - Dependencies: T02 + - Done when: an inherited marker forces exactly one recovery transition + (cursor := observed tree, revision += 1, `tainted`/external taint cleared, + active scopes → `Abandoned`, no `MutationEvent` for the skipped interval) + before the triggering boundary is processed against the recovered state; + recovery and boundary share one snapshot (call-counting `SnapshotCapture` + proves a single `capture`); a recovery-CAS conflict re-injects + `database_failure` on reload and recomputes; after recovery-CAS `Applied` + the flag is clear so a later boundary-CAS retry in the same invocation does + not re-trigger recovery; a first-ever inherited marker with no worktree row + produces no evidence for the unknown interval; `coordinator.rs` inline tests + cover each case. + - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::coordinator`; + `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings`; + `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check`. + - Context synchronization: synced + +- [x] T04: `Add restart/failure integration tests through coordinate()` (status:done) + - Task ID: T04 + - Completed: 2026-08-30 + - Files changed: + - `cli/src/services/mutation_trace/runtime/tests.rs` — three added imports + (`FailureKind`, `ScopeStatus`; `ExternalTaintOperation`; + `super::external_taint::ExternalTaintMarker`) and seven new cross-module + `#[test]`s driving only the public `coordinate()` API against real + `git init` / `git worktree add` repos and real temp-file + `RepositoryAgentTraceDb`s (DB placed in a sibling temp dir, outside the + worktree, so it never perturbs the captured tree): + `a_successful_coordinate_through_the_public_api_leaves_no_external_taint_marker`, + `a_db_open_failure_after_arming_leaves_the_marker_and_the_next_invocation_rebaselines_without_evidence` + (AC14 + the end-to-end A→B→gap→C no-trusted-evidence story, with a + `load_mutation_event` revision sweep asserting no event ends at the + post-gap tree), + `a_stale_marker_rebaselines_to_the_current_tree_abandons_scopes_then_processes_the_boundary` + (AC5), `a_first_ever_failed_invocation_that_never_materialized_a_worktree_row_creates_no_evidence` + (AC9), `linked_worktrees_keep_independent_external_taint_markers_over_a_shared_db` + (AC12), `a_snapshot_failure_arms_the_marker_and_the_next_invocation_recovers_once`, + `a_marker_clear_failure_after_a_durable_boundary_keeps_the_marker_for_a_later_recovery`. + Two of the seven carry `#[allow(clippy::too_many_lines)]` (end-to-end + multi-step scenarios; crate has precedent for the allow). + - Result: The external-taint fence is now proven end to end through the public + `coordinate()` entrypoint only, no production-code change. Success clears the + marker; a DB-provider `Err` after arming (AC14) and a Git snapshot failure + both leave the marker present (the snapshot path also leaves a durable + `SnapshotFailure` taint), and the next working invocation performs a single + conservative recovery, rebaselines the cursor to the current tree, abandons + scopes that were live across the fenced interval, emits no `MutationEvent` + across the gap, then processes its triggering boundary. A first-ever failed + invocation that never materialized a worktree row cannot create evidence for + the unknown interval. A stale marker in linked worktree A does not recover + linked worktree B over one shared DB; each worktree clears only its own + marker. A marker-clear failure after a durable boundary (injected by swapping + the marker file for a directory inside the caller-supplied `open_db` closure) + returns `CoordinateError::ExternalTaintMarker { operation: Clear }` with the + boundary already durable and the marker left for a later conservative + re-recovery. Tests use a sibling-dir DB so consecutive no-edit flushes are + genuinely stable. + - Verify: + - `test ...services::mutation_trace::runtime::tests` — PASS (10 passed, 0 failed). + - `test ...services::mutation_trace::runtime::` — PASS (55 passed, 0 failed). + - `test ...` (full CLI suite) — PASS (844 passed, 0 failed). + - `clippy --all-targets -- -D warnings` — PASS (clean). + - `fmt -- --check` — PASS (clean). + - Context impact: none. Test-only change; no production code, public + interface, protocol semantics, DB state, on-disk layout, or documented + behavior changed. The added tests only exercise the `coordinate()` / + `ExternalTaintMarker` behavior already documented by T01–T03's context + synchronization. The mandatory five-root-file verification pass still applies. + - Deviations: the DB is placed in a sibling temp directory rather than inside + the repo worktree (as some pre-existing tests in this file do) so the SQLite + file never appears in a captured tree and revision-stability / no-evidence + assertions hold; two long end-to-end tests carry + `#[allow(clippy::too_many_lines)]`. + - Scope: In — `runtime/tests.rs` cross-module tests driving only the public + `coordinate()` API against real `git init` / `git worktree add` repositories + and real temp-file `RepositoryAgentTraceDb`s (unique-temp-path precedent), + with the DB passed through the provider closure: successful invocation + leaves no marker; a failed invocation leaves the marker; a **DB provider + returning `Err` after the marker is armed** leaves the marker present and + the follow-up invocation (working provider) rebaselines to the current tree + with no evidence across the lost interval; a stale marker on the next + invocation rebaselines to the current tree, emits no evidence across the + gap, abandons prior live scopes, then processes its boundary; a first-ever + failed invocation that never materialized a worktree row cannot create + evidence; two linked worktrees keep independent markers over one shared DB + (a marker in A does not recover B); snapshot-failure interaction leaves both + a durable `SnapshotFailure` and the marker, and the next invocation performs + a single conservative recovery; a marker-clear failure after a durable + boundary keeps the marker for a later conservative re-recovery. Out — any + production-code change. + - Dependencies: T03 + - Done when: the listed scenarios pass through the public entrypoint only, + including the end-to-end "trusted A → failed/incomplete invocation (DB-open + failure included) → filesystem changes → next successful invocation + rebaselines to C with no A/B→C evidence" story; `runtime::tests` and the + full CLI test suite pass. + - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::tests`; + `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml`. + - Context synchronization: synced + +## Open questions + +None. The entry-contract question raised in the prior draft is resolved by this +plan: PR #245 establishes an outer mutation-cursor runtime boundary that arms +external taint before Agent Trace DB acquisition, and future harness adapters +must call that protected boundary (`harness adapter → coordinate() → marker → +DB → snapshot / protocol / CAS`) rather than opening the DB themselves and +calling a lower-level coordinator. Whether a later change also splits +`agent_trace_storage` resolution into "resolve identity/path" and "open DB" +halves is left to implementation — the plan only requires the narrowest seam +that puts DB acquisition inside the fence, and the caller-supplied provider +closure achieves that without touching `agent_trace_storage` at all. + +## Validation Report + +**Status:** validated +**Date:** 2026-08-30 (revalidated after the AC8 `processed_events` assertion tightening) + +### Commands run + +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::` -> exit 0 (56 passed, 0 failed; every AC-mapped `external_taint`, `coordinator`, and cross-module `runtime::tests` case passed) +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` -> exit 0 (845 passed, 0 failed; full CLI suite, including `mbt` Quint-refinement tests and the stabilized `sync` batch tests) +- `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings` -> exit 0 (clean, no warnings) +- `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check` -> exit 0 (clean) +- `nix run .#pkl-check-generated` -> exit 0 (ephemeral Pkl generation passed: 141 files, inventory sha256 bf5db9c962cc9ce2776b4fc218dcbd8787fa7567744a5e2faff1fc9f9212a003) +- `nix flake check` -> exit 0 (all 4 flake checks passed, including `checks.cli-tests` and `checks.mutation-trace-quint-connect`) + +### Success-criteria verification + +- [x] AC1: Marker is stored under the caller-supplied worktree git-dir, survives handle reconstruction, and two `git_dir` inputs derive independent paths/state -> `external_taint::tests::marker_is_worktree_scoped` + `marker_persists_until_explicitly_cleared` passed +- [x] AC2: `persist`/`persist`/`clear`/`clear` all succeed; `clear` on an absent marker is success -> `external_taint::tests::persist_and_clear_are_idempotent` passed +- [x] AC3: Public `coordinate()` arms its marker internally and clears it only after a successful `CoordinateOutcome` -> `coordinator::tests::public_coordinate_clears_marker_on_success` + `runtime::tests::a_successful_coordinate_through_the_public_api_leaves_no_external_taint_marker` passed +- [x] AC4: After arming, any coordinator error leaves the marker present (snapshot-failure and deterministic non-snapshot paths) -> `coordinator::tests::public_coordinate_leaves_marker_after_a_snapshot_failure`, `..._after_a_non_snapshot_failure`, `..._when_the_db_provider_fails` passed +- [x] AC5: Inherited marker rebaselines to current tree, emits no A→C evidence, abandons scopes, then processes the boundary -> `runtime::tests::a_stale_marker_rebaselines_to_the_current_tree_abandons_scopes_then_processes_the_boundary` + `coordinator::tests::inherited_external_taint_recovers_once_before_the_boundary` passed +- [x] AC6: Recovery and triggering-boundary processing share one `observed_tree`; no second Git snapshot -> `coordinator::tests::inherited_external_taint_recovers_once_before_the_boundary` (call-counting capture asserts exactly one `capture`/`pin`) passed +- [x] AC7: Losing recovery CAS re-injects external taint on reload and recomputes until `Applied`/exhaustion; marker persists through the loop -> `coordinator::tests::a_losing_recovery_cas_reinjects_external_taint_until_it_applies`, `a_landed_recovery_clears_the_flag_so_a_boundary_cas_retry_does_not_re_recover`, `runtime::tests::a_snapshot_failure_arms_the_marker_and_the_next_invocation_recovers_once` passed +- [x] AC8: Failure after recovery commit but before boundary completion leaves recovery durable, boundary incomplete, marker present; post-commit `marker.clear()` failure returns `MarkerClearAfterCommit` carrying the committed outcome -> `coordinator::tests::a_failure_after_recovery_before_boundary_commit_leaves_marker_and_forces_later_recovery` + `runtime::tests::a_marker_clear_failure_after_a_durable_boundary_keeps_the_marker_for_a_later_recovery` passed +- [x] AC9: Marker surviving an invocation that never materialized a worktree row → next successful invocation baselines with no evidence for the unknown interval -> `coordinator::tests::inherited_external_taint_with_no_worktree_row_baselines_without_evidence` + `runtime::tests::a_first_ever_failed_invocation_that_never_materialized_a_worktree_row_creates_no_evidence` passed +- [x] AC10: Worktree with live scopes and inherited taint persists them as `Abandoned` during recovery, never eligible for exclusive attribution afterward -> `coordinator::tests::inherited_external_taint_recovers_once_before_the_boundary` (asserts `ScopeStatus::Abandoned`) + `recovers_from_snapshot_failure_taint_abandoning_live_scopes` passed +- [x] AC11: End to end, no `MutationEvent` treats an interval spanning a failed invocation as one trustworthy AI-attributable interval -> `runtime::tests::a_db_open_failure_after_arming_leaves_the_marker_and_the_next_invocation_rebaselines_without_evidence` (per-revision `load_mutation_event` sweep, no event ends at the post-gap tree) passed +- [x] AC12: A marker in linked worktree A does not trigger recovery in linked worktree B over a shared repository Agent Trace DB -> `runtime::tests::linked_worktrees_keep_independent_external_taint_markers_over_a_shared_db` passed +- [x] AC13: When marker inspection or persistence cannot be established, `coordinate()` returns a distinct external-taint marker error before any checkout-identity, DB, snapshot, or protocol processing -> `coordinator::tests::public_coordinate_fails_closed_when_the_marker_cannot_be_armed` + `..._when_marker_inspection_fails` passed +- [x] AC14: DB acquisition is inside the fence — a DB-provider `Err` after lock+arming makes `coordinate()` return an error with the marker present though no `RepositoryAgentTraceDb` existed; the follow-up invocation with a working provider snapshots, recovers, and produces no evidence across the lost interval -> `runtime::tests::a_db_open_failure_after_arming_leaves_the_marker_and_the_next_invocation_rebaselines_without_evidence` + `coordinator::tests::public_coordinate_leaves_marker_when_the_db_provider_fails` passed + +### Failed checks and follow-ups + +- None. + +### Residual risks + +- Marker durability is best-effort at the parent-directory `sync_all` level (`#[cfg(unix)]`, error swallowed); the plan explicitly does not claim durability across host power loss or filesystem-level crash. +- The `spec/mutation_cursor.qnt` model and Quint refinement matrix are unchanged by this plan; the abstract↔concrete correspondence for the write-ahead marker is asserted by the Rust integration tests and prose refinement notes, not by a machine-checked refinement. +- The recorded sync-test stabilization (`cli/src/services/sync/sync.rs`, test-only) touched `terminal_batch_status_fails_without_state_reconciliation` and `malformed_2xx_batch_response_still_reconciles_via_state`; no sync production behavior changed and the full suite is green. diff --git a/spec/mutation_cursor.md b/spec/mutation_cursor.md index 7bb5e0d7..67a1e6c4 100644 --- a/spec/mutation_cursor.md +++ b/spec/mutation_cursor.md @@ -45,7 +45,7 @@ Database unavailability is different. `databaseFailure(worktree)` changes only: externalTaint: Set[WorktreeId] ``` -`externalTaint` is the abstract external durability boundary: conceptually, the filesystem `TAINTED` marker that can survive an unavailable database. It is not a database row and does not model marker paths or filesystem syscalls. +`externalTaint` is the abstract external durability boundary: conceptually, the filesystem `TAINTED` marker that can survive an unavailable database. It is not a database row and does not model marker paths or filesystem syscalls. Its concrete refinement in the CLI is the worktree-local marker file `/sce/mutation-cursor-tainted`, armed write-ahead at the start of the protected runtime section — before Agent Trace DB acquisition — and cleared only after a proven durable completion; it becomes protocol `externalTaint` (overlaid onto `databaseFailure` recovery) only when a later invocation inherits it. If that trailing clear fails *after* the boundary has committed durably, the CLI keeps the marker armed and returns the committed outcome inside a distinct error rather than reporting a boundary failure, so the next invocation still recovers conservatively. See `context/cli/mutation-trace-external-taint.md`. Thus the model does **not** perform this contradictory transition: