diff --git a/cli/src/services/hooks/codex_mutation_scope/state.rs b/cli/src/services/hooks/codex_mutation_scope/state.rs index 8d81a8e0a..72016ef49 100644 --- a/cli/src/services/hooks/codex_mutation_scope/state.rs +++ b/cli/src/services/hooks/codex_mutation_scope/state.rs @@ -432,6 +432,146 @@ pub(crate) fn arm_mark_active_failure_for_tests() { fault::arm_mark_active_failure(); } +#[cfg(test)] +mod state_conformance { + use super::*; + use crate::services::hooks::mutation_scope_state_conformance::{ + mutation_scope_state_conformance_tests, CompletionView, FlushClaimView, RecoveryView, + StateConformance, + }; + + const FIXTURE_SESSION: &str = "session-conformance"; + const FIXTURE_PHASES: [AttemptPhase; 3] = [ + AttemptPhase::Active, + AttemptPhase::Active, + AttemptPhase::PendingStart, + ]; + + struct CodexStateConformance; + + fn fixture_key(index: usize) -> AttemptKey { + AttemptKey { + session_id: FIXTURE_SESSION.to_string(), + agent_id: (index % 2 == 1).then(|| format!("agent-{index}")), + tool_use_id: format!("exec-{index}"), + } + } + + impl StateConformance for CodexStateConformance { + type State = AdapterState; + + const ADAPTER: &'static str = "codex"; + const SUPPORTED_VERSION: u32 = ADAPTER_STATE_VERSION; + + fn state_path(git_dir: &Path) -> PathBuf { + state_path(git_dir) + } + + fn fixture_state( + recovery: RecoveryView, + next_recovery_generation: u64, + attempt_count: usize, + ) -> AdapterState { + let mut state = AdapterState { + next_recovery_generation, + recovery: match recovery { + RecoveryView::Clear => RecoveryState::Clear, + RecoveryView::Pending(generation) => RecoveryState::Pending { generation }, + RecoveryView::Flushing(generation) => RecoveryState::Flushing { generation }, + }, + ..AdapterState::default() + }; + for index in 0..attempt_count { + allocate_pending_start( + &mut state, + &fixture_key(index), + &format!("turn-{index}"), + "apply_patch", + ); + state.attempts[index].phase = FIXTURE_PHASES[index % FIXTURE_PHASES.len()]; + } + state + } + + fn read_state(git_dir: &Path) -> Result { + read_state(git_dir) + } + + fn persist(git_dir: &Path, state: &AdapterState) -> Result<()> { + write_state_durably(git_dir, state) + } + + fn persist_with_before_rename_hook( + git_dir: &Path, + state: &AdapterState, + before_rename: F, + ) -> Result<()> + where + F: FnOnce(&Path, &Path) -> Result<()>, + { + write_state_durably_inner(git_dir, state, before_rename) + } + + fn recovery_view(state: &AdapterState) -> RecoveryView { + match state.recovery { + RecoveryState::Clear => RecoveryView::Clear, + RecoveryState::Pending { generation } => RecoveryView::Pending(generation), + RecoveryState::Flushing { generation } => RecoveryView::Flushing(generation), + } + } + + fn next_recovery_generation(state: &AdapterState) -> u64 { + state.next_recovery_generation + } + + fn scope_ids(state: &AdapterState) -> Vec { + state + .attempts + .iter() + .map(|attempt| attempt.scope_id.clone()) + .collect() + } + + fn remove_attempt(git_dir: &Path, scope_id: &str) -> Result<()> { + remove_attempt(git_dir, scope_id) + } + + fn normalize_recovery_after_boundary_lock_acquired(git_dir: &Path) -> Result<()> { + normalize_recovery_after_boundary_lock_acquired(git_dir) + } + + fn complete_recovery_flush(git_dir: &Path, generation: u64) -> Result { + Ok(match complete_recovery_flush(git_dir, generation)? { + RecoveryFlushCompletion::Cleared => CompletionView::Cleared, + RecoveryFlushCompletion::Superseded => CompletionView::Superseded, + }) + } + + fn relinquish_recovery_flush(git_dir: &Path, generation: u64) -> Result<()> { + relinquish_recovery_flush(git_dir, generation) + } + + fn claim_flush_by_admitting_a_new_attempt( + git_dir: &Path, + contender: usize, + ) -> Result { + let decision = admit_tracked_attempt( + git_dir, + &fixture_key(contender), + &format!("turn-{contender}"), + "Bash", + )?; + Ok(match decision { + AdmitDecision::FlushClaimed { generation } => FlushClaimView::Claimed(generation), + AdmitDecision::RecoveryBlocked => FlushClaimView::Blocked, + other => FlushClaimView::Other(format!("{other:?}")), + }) + } + } + + mutation_scope_state_conformance_tests!(CodexStateConformance); +} + #[cfg(test)] mod tests { use std::sync::atomic::{AtomicU64, Ordering}; @@ -497,19 +637,6 @@ mod tests { } } - #[test] - fn read_state_returns_default_when_file_is_absent() { - let git_dir = unique_test_git_dir("read-default"); - - let state = read_state(&git_dir).expect("missing state file should read as default"); - assert_eq!(state, AdapterState::default()); - assert_eq!(state.version, 3); - assert!(state.recovery.is_clear()); - assert_eq!(state.next_recovery_generation, 1); - - remove_test_git_dir(&git_dir); - } - #[test] fn admit_allocates_sequential_attempt_seqs_across_distinct_keys() { let git_dir = unique_test_git_dir("sequential-allocation"); @@ -647,9 +774,11 @@ mod tests { let git_dir = unique_test_git_dir("admit-persists-pending-start"); std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - let AdmitDecision::Admitted(allocated) = - admit(&git_dir, &key("session-1", None, "exec-1"), "Bash") - else { + let AdmitDecision::Admitted(allocated) = admit( + &git_dir, + &key("session-7", Some("agent-2"), "exec-9"), + "apply_patch", + ) else { panic!("expected Admitted"); }; @@ -659,8 +788,13 @@ mod tests { 1, "I2: PendingStart must be durable before admit returns" ); - assert_eq!(state.attempts[0].scope_id, allocated.attempt.scope_id); + assert_eq!(state.attempts[0], allocated.attempt); assert_eq!(state.attempts[0].phase, AttemptPhase::PendingStart); + assert_eq!(state.attempts[0].session_id, "session-7"); + assert_eq!(state.attempts[0].turn_id, TEST_TURN); + assert_eq!(state.attempts[0].agent_id.as_deref(), Some("agent-2")); + assert_eq!(state.attempts[0].tool_use_id, "exec-9"); + assert_eq!(state.attempts[0].tool_name, "apply_patch"); remove_test_git_dir(&git_dir); } @@ -836,61 +970,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn only_one_concurrent_caller_claims_the_flush_for_a_generation() { - let git_dir = unique_test_git_dir("one-flush-owner"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - let generation = arm_recovery(&git_dir).expect("arming recovery should succeed"); - - let handles: Vec<_> = ["a", "b"] - .into_iter() - .map(|suffix| { - let git_dir = git_dir.clone(); - thread::spawn(move || { - admit_tracked_attempt( - &git_dir, - &key("session-1", None, &format!("exec-{suffix}")), - &format!("turn-{suffix}"), - "Bash", - ) - .expect("admit should not error") - }) - }) - .collect(); - - let mut decisions: Vec = handles - .into_iter() - .map(|handle| handle.join().expect("thread should not panic")) - .collect(); - decisions.sort_by_key(|decision| format!("{decision:?}")); - - let flush_claims = decisions - .iter() - .filter(|decision| matches!(decision, AdmitDecision::FlushClaimed { .. })) - .count(); - let blocked = decisions - .iter() - .filter(|decision| matches!(decision, AdmitDecision::RecoveryBlocked)) - .count(); - assert_eq!( - flush_claims, 1, - "I3: exactly one process may claim Flushing(g)" - ); - assert_eq!( - blocked, 1, - "the other concurrent caller must stay fail-closed" - ); - assert_eq!( - decisions.iter().find_map(|decision| match decision { - AdmitDecision::FlushClaimed { generation } => Some(*generation), - _ => None, - }), - Some(generation), - ); - - remove_test_git_dir(&git_dir); - } - #[test] fn arm_recovery_moves_clear_to_pending_with_a_fresh_generation() { let git_dir = unique_test_git_dir("arm-clear-to-pending"); @@ -943,35 +1022,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn complete_recovery_flush_clears_only_with_the_matching_generation() { - let git_dir = unique_test_git_dir("complete-matching-generation"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - arm_recovery(&git_dir).expect("arm"); - admit(&git_dir, &key("session-1", None, "exec-new"), "Bash"); - - assert_eq!( - complete_recovery_flush(&git_dir, 2).expect("wrong-generation completion"), - RecoveryFlushCompletion::Superseded, - ); - assert_eq!( - read_state(&git_dir).expect("state readable").recovery, - RecoveryState::Flushing { generation: 1 }, - ); - - assert_eq!( - complete_recovery_flush(&git_dir, 1).expect("matching completion"), - RecoveryFlushCompletion::Cleared, - ); - assert!(read_state(&git_dir) - .expect("state readable") - .recovery - .is_clear()); - - remove_test_git_dir(&git_dir); - } - #[test] fn complete_recovery_flush_is_a_no_op_when_a_newer_recovery_was_armed() { let git_dir = unique_test_git_dir("complete-superseded-by-newer"); @@ -1011,29 +1061,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn normalize_after_boundary_lock_reclaims_orphaned_flushing_to_pending_same_generation() { - let git_dir = unique_test_git_dir("normalize-orphaned-flushing"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - arm_recovery(&git_dir).expect("arm g1"); - admit(&git_dir, &key("session-1", None, "exec-new"), "Bash"); - assert_eq!( - read_state(&git_dir).expect("state readable").recovery, - RecoveryState::Flushing { generation: 1 }, - ); - let next_generation_before = read_state(&git_dir) - .expect("state readable") - .next_recovery_generation; - - normalize_recovery_after_boundary_lock_acquired(&git_dir) - .expect("normalize should succeed"); - - let state = read_state(&git_dir).expect("state readable"); - assert_eq!(state.recovery, RecoveryState::Pending { generation: 1 }); - assert_eq!(state.next_recovery_generation, next_generation_before); - } - #[test] fn normalize_after_boundary_lock_is_a_no_op_for_clear_or_pending() { let git_dir = unique_test_git_dir("normalize-noop"); @@ -1055,29 +1082,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn relinquish_recovery_flush_returns_a_claimed_generation_to_pending() { - let git_dir = unique_test_git_dir("relinquish-to-pending"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - arm_recovery(&git_dir).expect("arm"); - admit(&git_dir, &key("session-1", None, "exec-new"), "Bash"); - - relinquish_recovery_flush(&git_dir, 1).expect("relinquish should succeed"); - assert_eq!( - read_state(&git_dir).expect("state readable").recovery, - RecoveryState::Pending { generation: 1 }, - ); - - relinquish_recovery_flush(&git_dir, 1).expect("second relinquish is a no-op"); - assert_eq!( - read_state(&git_dir).expect("state readable").recovery, - RecoveryState::Pending { generation: 1 }, - ); - - remove_test_git_dir(&git_dir); - } - #[test] fn relinquish_recovery_flush_is_a_no_op_for_a_superseded_generation() { let git_dir = unique_test_git_dir("relinquish-superseded"); @@ -1096,26 +1100,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn recovery_state_survives_serialization_and_reload() { - let git_dir = unique_test_git_dir("recovery-round-trip"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - arm_recovery(&git_dir).expect("arm"); - assert_eq!( - read_state(&git_dir).expect("reload pending").recovery, - RecoveryState::Pending { generation: 1 }, - ); - - admit(&git_dir, &key("session-1", None, "exec-new"), "Bash"); - assert_eq!( - read_state(&git_dir).expect("reload flushing").recovery, - RecoveryState::Flushing { generation: 1 }, - ); - - remove_test_git_dir(&git_dir); - } - #[test] fn mark_active_transitions_phase_from_pending_start_to_active() { let git_dir = unique_test_git_dir("mark-active"); @@ -1151,130 +1135,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn removing_an_already_removed_attempt_is_a_safe_no_op() { - let git_dir = unique_test_git_dir("remove-idempotent"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - let attempt = admit_and_activate(&git_dir, &key("session-1", None, "exec-1"), "Bash"); - - remove_attempt(&git_dir, &attempt.scope_id).expect("first removal should succeed"); - remove_attempt(&git_dir, &attempt.scope_id) - .expect("duplicate terminal delivery after cleanup must be a safe no-op"); - - assert!(read_state(&git_dir) - .expect("state readable") - .attempts - .is_empty()); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn state_round_trips_durably_through_the_canonical_path() { - let git_dir = unique_test_git_dir("round-trip"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - let attempt = admit_and_activate( - &git_dir, - &key("session-7", Some("agent-2"), "exec-9"), - "apply_patch", - ); - arm_recovery(&git_dir).expect("arming recovery should succeed"); - - let reloaded = read_state(&git_dir).expect("state should reload"); - assert_eq!(reloaded.version, ADAPTER_STATE_VERSION); - assert_eq!(reloaded.next_attempt_seq, 2); - assert_eq!(reloaded.recovery, RecoveryState::Pending { generation: 1 }); - assert_eq!(reloaded.attempts.len(), 1); - assert_eq!(reloaded.attempts[0].phase, AttemptPhase::Active); - assert_eq!(reloaded.attempts[0].turn_id, "turn-1"); - assert_eq!(reloaded.attempts[0].agent_id.as_deref(), Some("agent-2")); - assert_eq!(reloaded.attempts[0].tool_name, "apply_patch"); - assert_eq!(reloaded.attempts[0].scope_id, attempt.scope_id); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn malformed_state_file_is_rejected_without_fabricating_bookkeeping() { - let git_dir = unique_test_git_dir("malformed-json"); - let dir = adapter_state_dir(&git_dir); - std::fs::create_dir_all(&dir).expect("state dir should be created"); - std::fs::write(state_path(&git_dir), b"not json") - .expect("malformed file should be written"); - - let error = read_state(&git_dir).expect_err("malformed state file must be rejected"); - assert!(error.to_string().contains("malformed")); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn an_unsupported_or_prior_version_state_file_is_rejected() { - for payload in [ - serde_json::json!({ - "version": 99, - "next_attempt_seq": 1, - "next_recovery_generation": 1, - "recovery": { "phase": "clear" }, - "attempts": [] - }), - serde_json::json!({ - "version": 1, - "next_attempt_seq": 1, - "recovery_pending": false, - "attempts": [] - }), - serde_json::json!({ - "version": 2, - "next_attempt_seq": 1, - "next_recovery_generation": 1, - "recovery": { "phase": "clear" }, - "attempts": [] - }), - ] { - let git_dir = unique_test_git_dir("unsupported-version"); - let dir = adapter_state_dir(&git_dir); - std::fs::create_dir_all(&dir).expect("state dir should be created"); - std::fs::write(state_path(&git_dir), payload.to_string()) - .expect("state file should be written"); - - let error = read_state(&git_dir).expect_err("unsupported version must be rejected"); - assert!( - error.to_string().contains("unsupported version"), - "payload {payload} produced {error}" - ); - - remove_test_git_dir(&git_dir); - } - } - - #[test] - fn interruption_before_rename_leaves_the_canonical_path_unaffected() { - let git_dir = unique_test_git_dir("interrupted-before-rename"); - let dir = adapter_state_dir(&git_dir); - std::fs::create_dir_all(&dir).expect("state dir should be created"); - - let state = AdapterState { - next_attempt_seq: 5, - ..AdapterState::default() - }; - let result = write_state_durably_inner(&git_dir, &state, |tmp_path, canonical_path| { - assert!(tmp_path.exists()); - assert!(!canonical_path.exists()); - Err(anyhow!("injected interruption before rename")) - }); - - assert!(result.is_err()); - assert!(!state_path(&git_dir).exists()); - assert_eq!( - read_state(&git_dir).expect("read should not error on an absent canonical file"), - AdapterState::default(), - ); - - remove_test_git_dir(&git_dir); - } - const PARALLEL_ADMISSION_COUNT: u64 = 6; #[test] @@ -1360,27 +1220,4 @@ mod tests { remove_test_git_dir(&git_dir); } - - #[test] - fn adapter_state_files_live_only_below_git_dir_sce() { - let git_dir = unique_test_git_dir("path-boundary"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - admit(&git_dir, &key("session-1", None, "exec-1"), "Bash"); - - let sce_dir = git_dir.join(SCE_STATE_DIR); - assert!(state_path(&git_dir).starts_with(&sce_dir)); - - let mut found_state_file = false; - for entry in std::fs::read_dir(&sce_dir).expect("sce dir should be readable") { - let entry = entry.expect("dir entry should be readable"); - assert!(entry.path().starts_with(&sce_dir)); - if entry.path() == state_path(&git_dir) { - found_state_file = true; - } - } - assert!(found_state_file); - - remove_test_git_dir(&git_dir); - } } diff --git a/cli/src/services/hooks/mod.rs b/cli/src/services/hooks/mod.rs index e8b00793f..7cc84229e 100644 --- a/cli/src/services/hooks/mod.rs +++ b/cli/src/services/hooks/mod.rs @@ -30,6 +30,8 @@ mod commit_hooks; mod conversation_trace; mod diff_trace; mod mutation_scope_lock; +#[cfg(test)] +mod mutation_scope_state_conformance; mod runtime; pub mod claude_bridge_session; diff --git a/cli/src/services/hooks/mutation_scope_state_conformance.rs b/cli/src/services/hooks/mutation_scope_state_conformance.rs new file mode 100644 index 000000000..22486a6c5 --- /dev/null +++ b/cli/src/services/hooks/mutation_scope_state_conformance.rs @@ -0,0 +1,572 @@ +use std::fmt::Debug; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::Barrier; +use std::thread; + +use anyhow::{anyhow, Result}; +use serde_json::{json, Value}; + +const SCE_STATE_DIR: &str = "sce"; +const SEEDED_GENERATION: u64 = 7; +const SEEDED_NEXT_GENERATION: u64 = SEEDED_GENERATION + 3; +const UNKNOWN_SCOPE_ID: &str = "state-conformance-unknown-scope"; +const INJECTED_INTERRUPTION: &str = "injected interruption before rename"; +const FLUSH_CLAIM_CONTENDER_COUNT: usize = 4; + +static NEXT_CONFORMANCE_GIT_DIR_ID: AtomicU64 = AtomicU64::new(0); + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum RecoveryView { + Clear, + Pending(u64), + Flushing(u64), +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum CompletionView { + Cleared, + Superseded, +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) enum FlushClaimView { + Claimed(u64), + Blocked, + Other(String), +} + +pub(crate) trait StateConformance { + type State: Debug + Default + PartialEq; + + const ADAPTER: &'static str; + const SUPPORTED_VERSION: u32; + + fn state_path(git_dir: &Path) -> PathBuf; + + fn fixture_state( + recovery: RecoveryView, + next_recovery_generation: u64, + attempt_count: usize, + ) -> Self::State; + + fn read_state(git_dir: &Path) -> Result; + + fn persist(git_dir: &Path, state: &Self::State) -> Result<()>; + + fn persist_with_before_rename_hook( + git_dir: &Path, + state: &Self::State, + before_rename: F, + ) -> Result<()> + where + F: FnOnce(&Path, &Path) -> Result<()>; + + fn recovery_view(state: &Self::State) -> RecoveryView; + + fn next_recovery_generation(state: &Self::State) -> u64; + + fn scope_ids(state: &Self::State) -> Vec; + + fn remove_attempt(git_dir: &Path, scope_id: &str) -> Result<()>; + + fn normalize_recovery_after_boundary_lock_acquired(git_dir: &Path) -> Result<()>; + + fn complete_recovery_flush(git_dir: &Path, generation: u64) -> Result; + + fn relinquish_recovery_flush(git_dir: &Path, generation: u64) -> Result<()>; + + fn claim_flush_by_admitting_a_new_attempt( + git_dir: &Path, + contender: usize, + ) -> Result; +} + +struct ConformanceGitDir(PathBuf); + +impl ConformanceGitDir { + fn create(contract: &str) -> Self { + let id = NEXT_CONFORMANCE_GIT_DIR_ID.fetch_add(1, Ordering::Relaxed); + let path = std::env::temp_dir().join(format!( + "sce-{}-mutation-scope-state-conformance-{contract}-{}-{id}", + A::ADAPTER, + std::process::id() + )); + std::fs::create_dir_all(&path).expect("git dir should be created"); + Self(path) + } + + fn path(&self) -> &Path { + &self.0 + } +} + +impl Drop for ConformanceGitDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +fn quiescent_state(recovery: RecoveryView) -> A::State { + A::fixture_state(recovery, SEEDED_NEXT_GENERATION, 0) +} + +fn populated_pending_state(attempt_count: usize) -> A::State { + A::fixture_state( + RecoveryView::Pending(SEEDED_GENERATION), + SEEDED_NEXT_GENERATION, + attempt_count, + ) +} + +fn seed(git_dir: &Path, state: &A::State) { + A::persist(git_dir, state).expect("seeding the canonical state should succeed"); +} + +fn reload(git_dir: &Path) -> A::State { + A::read_state(git_dir).expect("persisted state should be readable") +} + +fn canonical_bytes(git_dir: &Path) -> Vec { + std::fs::read(A::state_path(git_dir)).expect("canonical state file should be readable") +} + +fn overwrite_canonical_bytes(git_dir: &Path, bytes: &[u8]) { + let path = A::state_path(git_dir); + std::fs::create_dir_all(path.parent().expect("state path should have a parent")) + .expect("state dir should be created"); + std::fs::write(path, bytes).expect("canonical state file should be written"); +} + +fn assert_every_transaction_fails_closed(git_dir: &Path, rejected: &[u8]) { + assert!( + A::remove_attempt(git_dir, UNKNOWN_SCOPE_ID).is_err(), + "{}: remove_attempt must not proceed over a rejected state file", + A::ADAPTER + ); + assert!( + A::normalize_recovery_after_boundary_lock_acquired(git_dir).is_err(), + "{}: normalize must not proceed over a rejected state file", + A::ADAPTER + ); + assert!( + A::complete_recovery_flush(git_dir, SEEDED_GENERATION).is_err(), + "{}: complete_recovery_flush must not proceed over a rejected state file", + A::ADAPTER + ); + assert!( + A::relinquish_recovery_flush(git_dir, SEEDED_GENERATION).is_err(), + "{}: relinquish_recovery_flush must not proceed over a rejected state file", + A::ADAPTER + ); + assert!( + A::claim_flush_by_admitting_a_new_attempt(git_dir, 0).is_err(), + "{}: admission must not proceed over a rejected state file", + A::ADAPTER + ); + assert_eq!( + canonical_bytes::(git_dir), + rejected, + "{}: a rejected state file must never be replaced by fabricated bookkeeping", + A::ADAPTER + ); +} + +pub(crate) fn missing_state_file_reads_as_default_without_fabricating_a_file< + A: StateConformance, +>() { + let git_dir = ConformanceGitDir::create::("missing-state"); + + let state = + A::read_state(git_dir.path()).expect("a missing state file must read as the default"); + + assert_eq!(state, A::State::default()); + assert_eq!(A::recovery_view(&state), RecoveryView::Clear); + assert_eq!(A::next_recovery_generation(&state), 1); + assert!(A::scope_ids(&state).is_empty()); + assert!(!A::state_path(git_dir.path()).exists()); + assert!(!git_dir.path().join(SCE_STATE_DIR).exists()); +} + +pub(crate) fn state_file_is_checkout_local_below_git_dir_sce() { + let git_dir_a = ConformanceGitDir::create::("checkout-local-a"); + let git_dir_b = ConformanceGitDir::create::("checkout-local-b"); + let state_path_a = A::state_path(git_dir_a.path()); + let state_path_b = A::state_path(git_dir_b.path()); + + assert!(state_path_a.starts_with(git_dir_a.path().join(SCE_STATE_DIR))); + assert!(state_path_b.starts_with(git_dir_b.path().join(SCE_STATE_DIR))); + assert_eq!(state_path_a.file_name(), state_path_b.file_name()); + + let seeded = populated_pending_state::(2); + seed::(git_dir_a.path(), &seeded); + + assert!(state_path_a.is_file()); + assert_eq!(reload::(git_dir_a.path()), seeded); + assert!(!state_path_b.exists()); + assert_eq!(reload::(git_dir_b.path()), A::State::default()); +} + +pub(crate) fn state_round_trips_durably_through_the_canonical_path() { + let git_dir = ConformanceGitDir::create::("round-trip"); + let populated_attempt_count = 3; + let flushing = RecoveryView::Flushing(SEEDED_GENERATION); + let pending = RecoveryView::Pending(SEEDED_GENERATION); + + for (state, recovery, attempt_count) in [ + (quiescent_state::(flushing), flushing, 0), + ( + populated_pending_state::(populated_attempt_count), + pending, + populated_attempt_count, + ), + ( + quiescent_state::(RecoveryView::Clear), + RecoveryView::Clear, + 0, + ), + ] { + let mut distinct_scope_ids = A::scope_ids(&state); + distinct_scope_ids.sort_unstable(); + distinct_scope_ids.dedup(); + assert_eq!(distinct_scope_ids.len(), attempt_count); + + A::persist(git_dir.path(), &state).expect("durable write should succeed"); + + assert!(A::state_path(git_dir.path()).is_file()); + let on_disk: Value = serde_json::from_slice(&canonical_bytes::(git_dir.path())) + .expect("canonical state file should hold valid JSON"); + assert_eq!(on_disk["version"], json!(A::SUPPORTED_VERSION)); + + let reloaded = reload::(git_dir.path()); + assert_eq!(reloaded, state); + assert_eq!(A::recovery_view(&reloaded), recovery); + assert_eq!( + A::next_recovery_generation(&reloaded), + SEEDED_NEXT_GENERATION + ); + assert_eq!(A::scope_ids(&reloaded), A::scope_ids(&state)); + } +} + +pub(crate) fn malformed_state_file_is_rejected_without_fabricating_bookkeeping< + A: StateConformance, +>() { + for payload in ["not json", "", "{}", "{\"version\":"] { + let git_dir = ConformanceGitDir::create::("malformed-state"); + overwrite_canonical_bytes::(git_dir.path(), payload.as_bytes()); + + let error = A::read_state(git_dir.path()) + .expect_err("a malformed state file must be rejected, never read as default"); + assert!( + error.to_string().contains("malformed"), + "{}: payload {payload:?} produced {error}", + A::ADAPTER + ); + + assert_every_transaction_fails_closed::(git_dir.path(), payload.as_bytes()); + } +} + +pub(crate) fn a_state_file_with_any_other_version_is_rejected() { + let wrong_versions = (0..A::SUPPORTED_VERSION).chain([A::SUPPORTED_VERSION + 1, 99]); + + for version in wrong_versions { + let git_dir = ConformanceGitDir::create::("unsupported-version"); + seed::(git_dir.path(), &populated_pending_state::(2)); + + let mut payload: Value = serde_json::from_slice(&canonical_bytes::(git_dir.path())) + .expect("canonical state file should hold valid JSON"); + assert_eq!(payload["version"], json!(A::SUPPORTED_VERSION)); + payload["version"] = json!(version); + let rejected = serde_json::to_vec(&payload).expect("payload should serialize"); + overwrite_canonical_bytes::(git_dir.path(), &rejected); + + let error = A::read_state(git_dir.path()) + .expect_err("a state file with another version must be rejected, never migrated"); + assert!( + error.to_string().contains("unsupported version"), + "{}: version {version} produced {error}", + A::ADAPTER + ); + + assert_every_transaction_fails_closed::(git_dir.path(), &rejected); + } +} + +pub(crate) fn interruption_before_rename_leaves_the_canonical_path_unaffected< + A: StateConformance, +>() { + let git_dir = ConformanceGitDir::create::("interrupted-before-rename"); + let canonical_path = A::state_path(git_dir.path()); + let established = populated_pending_state::(1); + let replacement = populated_pending_state::(3); + + let error = A::persist_with_before_rename_hook( + git_dir.path(), + &established, + |tmp_path, hook_canonical_path| { + assert!(tmp_path.is_file()); + assert_ne!(tmp_path, hook_canonical_path); + assert_eq!(hook_canonical_path, canonical_path); + assert!(!hook_canonical_path.exists()); + Err(anyhow!(INJECTED_INTERRUPTION)) + }, + ) + .expect_err("an interrupted first write must fail"); + assert!(error.to_string().contains(INJECTED_INTERRUPTION)); + assert!(!canonical_path.exists()); + assert_eq!(reload::(git_dir.path()), A::State::default()); + + seed::(git_dir.path(), &established); + let established_bytes = canonical_bytes::(git_dir.path()); + + let error = A::persist_with_before_rename_hook( + git_dir.path(), + &replacement, + |tmp_path, hook_canonical_path| { + assert!(tmp_path.is_file()); + assert_ne!(tmp_path, hook_canonical_path); + assert_eq!( + std::fs::read(hook_canonical_path)?, + established_bytes, + "the canonical file must not be written before the rename" + ); + Err(anyhow!(INJECTED_INTERRUPTION)) + }, + ) + .expect_err("an interrupted replacement must fail"); + assert!(error.to_string().contains(INJECTED_INTERRUPTION)); + assert_eq!(canonical_bytes::(git_dir.path()), established_bytes); + assert_eq!(reload::(git_dir.path()), established); + + A::persist(git_dir.path(), &replacement) + .expect("a later durable write must succeed after an interrupted one"); + assert_eq!(reload::(git_dir.path()), replacement); +} + +pub(crate) fn removing_an_unknown_attempt_is_a_safe_no_op() { + let git_dir = ConformanceGitDir::create::("remove-unknown"); + + A::remove_attempt(git_dir.path(), UNKNOWN_SCOPE_ID) + .expect("removing an unknown scope from absent state must succeed"); + assert!(!A::state_path(git_dir.path()).exists()); + + let seeded = populated_pending_state::(2); + seed::(git_dir.path(), &seeded); + let seeded_bytes = canonical_bytes::(git_dir.path()); + + A::remove_attempt(git_dir.path(), UNKNOWN_SCOPE_ID) + .expect("removing an unknown scope from populated state must succeed"); + assert_eq!(canonical_bytes::(git_dir.path()), seeded_bytes); + assert_eq!(reload::(git_dir.path()), seeded); +} + +pub(crate) fn removing_an_already_removed_attempt_is_a_safe_no_op() { + let git_dir = ConformanceGitDir::create::("remove-idempotent"); + let seeded = populated_pending_state::(3); + seed::(git_dir.path(), &seeded); + let scope_ids = A::scope_ids(&seeded); + + A::remove_attempt(git_dir.path(), &scope_ids[1]).expect("first removal should succeed"); + + let after_removal = reload::(git_dir.path()); + assert_eq!( + A::scope_ids(&after_removal), + vec![scope_ids[0].clone(), scope_ids[2].clone()] + ); + assert_eq!( + A::recovery_view(&after_removal), + RecoveryView::Pending(SEEDED_GENERATION) + ); + assert_eq!( + A::next_recovery_generation(&after_removal), + SEEDED_NEXT_GENERATION + ); + let bytes_after_removal = canonical_bytes::(git_dir.path()); + + A::remove_attempt(git_dir.path(), &scope_ids[1]) + .expect("duplicate terminal delivery after cleanup must be a safe no-op"); + assert_eq!(canonical_bytes::(git_dir.path()), bytes_after_removal); + assert_eq!(reload::(git_dir.path()), after_removal); +} + +pub(crate) fn normalize_after_boundary_lock_reclaims_orphaned_flushing_to_pending_same_generation< + A: StateConformance, +>() { + let git_dir = ConformanceGitDir::create::("normalize-orphaned-flushing"); + let seeded = quiescent_state::(RecoveryView::Flushing(SEEDED_GENERATION)); + seed::(git_dir.path(), &seeded); + + A::normalize_recovery_after_boundary_lock_acquired(git_dir.path()) + .expect("normalize should succeed"); + + let normalized = reload::(git_dir.path()); + assert_eq!( + A::recovery_view(&normalized), + RecoveryView::Pending(SEEDED_GENERATION) + ); + assert_eq!( + A::next_recovery_generation(&normalized), + SEEDED_NEXT_GENERATION + ); +} + +pub(crate) fn complete_recovery_flush_clears_only_with_the_matching_generation< + A: StateConformance, +>() { + let git_dir = ConformanceGitDir::create::("complete-matching-generation"); + let seeded = quiescent_state::(RecoveryView::Flushing(SEEDED_GENERATION)); + seed::(git_dir.path(), &seeded); + let seeded_bytes = canonical_bytes::(git_dir.path()); + + for other_generation in [ + SEEDED_GENERATION - 1, + SEEDED_GENERATION + 1, + SEEDED_NEXT_GENERATION, + ] { + assert_eq!( + A::complete_recovery_flush(git_dir.path(), other_generation) + .expect("wrong-generation completion should return"), + CompletionView::Superseded, + ); + assert_eq!(canonical_bytes::(git_dir.path()), seeded_bytes); + } + assert_eq!(reload::(git_dir.path()), seeded); + + assert_eq!( + A::complete_recovery_flush(git_dir.path(), SEEDED_GENERATION) + .expect("matching completion should return"), + CompletionView::Cleared, + ); + + let completed = reload::(git_dir.path()); + assert_eq!(A::recovery_view(&completed), RecoveryView::Clear); + assert_eq!( + A::next_recovery_generation(&completed), + SEEDED_NEXT_GENERATION + ); +} + +pub(crate) fn relinquish_recovery_flush_returns_only_the_claimed_generation_to_pending< + A: StateConformance, +>() { + let git_dir = ConformanceGitDir::create::("relinquish-to-pending"); + let seeded = quiescent_state::(RecoveryView::Flushing(SEEDED_GENERATION)); + seed::(git_dir.path(), &seeded); + let seeded_bytes = canonical_bytes::(git_dir.path()); + + for other_generation in [SEEDED_GENERATION - 1, SEEDED_GENERATION + 1] { + A::relinquish_recovery_flush(git_dir.path(), other_generation) + .expect("wrong-generation relinquish should return"); + assert_eq!(canonical_bytes::(git_dir.path()), seeded_bytes); + } + assert_eq!(reload::(git_dir.path()), seeded); + + A::relinquish_recovery_flush(git_dir.path(), SEEDED_GENERATION) + .expect("relinquish should succeed"); + + let relinquished = reload::(git_dir.path()); + assert_eq!( + A::recovery_view(&relinquished), + RecoveryView::Pending(SEEDED_GENERATION) + ); + assert_eq!( + A::next_recovery_generation(&relinquished), + SEEDED_NEXT_GENERATION + ); + let relinquished_bytes = canonical_bytes::(git_dir.path()); + + A::relinquish_recovery_flush(git_dir.path(), SEEDED_GENERATION) + .expect("a second relinquish should return"); + assert_eq!(canonical_bytes::(git_dir.path()), relinquished_bytes); +} + +pub(crate) fn only_one_concurrent_caller_claims_the_flush_for_a_generation() { + let git_dir = ConformanceGitDir::create::("one-flush-owner"); + seed::( + git_dir.path(), + &quiescent_state::(RecoveryView::Pending(SEEDED_GENERATION)), + ); + let start = Barrier::new(FLUSH_CLAIM_CONTENDER_COUNT); + + let claims: Vec = thread::scope(|scope| { + let handles: Vec<_> = (0..FLUSH_CLAIM_CONTENDER_COUNT) + .map(|contender| { + let start = &start; + let git_dir = git_dir.path(); + scope.spawn(move || { + start.wait(); + A::claim_flush_by_admitting_a_new_attempt(git_dir, contender) + .expect("a contending admission should not error") + }) + }) + .collect(); + handles + .into_iter() + .map(|handle| handle.join().expect("contender should not panic")) + .collect() + }); + + let claimed = claims + .iter() + .filter(|claim| **claim == FlushClaimView::Claimed(SEEDED_GENERATION)) + .count(); + let blocked = claims + .iter() + .filter(|claim| **claim == FlushClaimView::Blocked) + .count(); + assert_eq!( + claimed, + 1, + "{}: exactly one caller may claim Flushing(g), got {claims:?}", + A::ADAPTER + ); + assert_eq!( + blocked, + FLUSH_CLAIM_CONTENDER_COUNT - 1, + "{}: every other concurrent caller must stay fail-closed, got {claims:?}", + A::ADAPTER + ); + + let claimed_state = reload::(git_dir.path()); + assert_eq!( + A::recovery_view(&claimed_state), + RecoveryView::Flushing(SEEDED_GENERATION) + ); + assert_eq!( + A::next_recovery_generation(&claimed_state), + SEEDED_NEXT_GENERATION + ); + assert!(A::scope_ids(&claimed_state).is_empty()); +} + +macro_rules! mutation_scope_state_conformance_tests { + ($adapter:ty) => { + $crate::services::hooks::mutation_scope_state_conformance::mutation_scope_state_conformance_tests! { + $adapter => + missing_state_file_reads_as_default_without_fabricating_a_file, + state_file_is_checkout_local_below_git_dir_sce, + state_round_trips_durably_through_the_canonical_path, + malformed_state_file_is_rejected_without_fabricating_bookkeeping, + a_state_file_with_any_other_version_is_rejected, + interruption_before_rename_leaves_the_canonical_path_unaffected, + removing_an_unknown_attempt_is_a_safe_no_op, + removing_an_already_removed_attempt_is_a_safe_no_op, + normalize_after_boundary_lock_reclaims_orphaned_flushing_to_pending_same_generation, + complete_recovery_flush_clears_only_with_the_matching_generation, + relinquish_recovery_flush_returns_only_the_claimed_generation_to_pending, + only_one_concurrent_caller_claims_the_flush_for_a_generation, + } + }; + ($adapter:ty => $($contract:ident),+ $(,)?) => { + $( + #[test] + fn $contract() { + $crate::services::hooks::mutation_scope_state_conformance::$contract::<$adapter>(); + } + )+ + }; +} + +pub(crate) use mutation_scope_state_conformance_tests; diff --git a/cli/src/services/hooks/opencode_mutation_scope/state.rs b/cli/src/services/hooks/opencode_mutation_scope/state.rs index fc9bbab6e..7fcc93dd1 100644 --- a/cli/src/services/hooks/opencode_mutation_scope/state.rs +++ b/cli/src/services/hooks/opencode_mutation_scope/state.rs @@ -496,6 +496,138 @@ pub(crate) fn set_attempt_owner_for_tests( updated } +#[cfg(test)] +mod state_conformance { + use super::*; + use crate::services::hooks::mutation_scope_state_conformance::{ + mutation_scope_state_conformance_tests, CompletionView, FlushClaimView, RecoveryView, + StateConformance, + }; + + const FIXTURE_SESSION: &str = "ses-conformance"; + const FIXTURE_PHASES: [AttemptPhase; 3] = [ + AttemptPhase::Active, + AttemptPhase::PendingAbandon, + AttemptPhase::PendingStart, + ]; + + struct OpenCodeStateConformance; + + fn fixture_key(index: usize) -> AttemptKey { + AttemptKey { + session_id: FIXTURE_SESSION.to_string(), + call_id: format!("call-{index}"), + } + } + + impl StateConformance for OpenCodeStateConformance { + type State = AdapterState; + + const ADAPTER: &'static str = "opencode"; + const SUPPORTED_VERSION: u32 = ADAPTER_STATE_VERSION; + + fn state_path(git_dir: &Path) -> PathBuf { + state_path(git_dir) + } + + fn fixture_state( + recovery: RecoveryView, + next_recovery_generation: u64, + attempt_count: usize, + ) -> AdapterState { + let mut state = AdapterState { + next_recovery_generation, + recovery: match recovery { + RecoveryView::Clear => RecoveryState::Clear, + RecoveryView::Pending(generation) => RecoveryState::Pending { generation }, + RecoveryView::Flushing(generation) => RecoveryState::Flushing { generation }, + }, + ..AdapterState::default() + }; + for index in 0..attempt_count { + allocate_pending_start(&mut state, &fixture_key(index), "apply_patch"); + state.attempts[index].phase = FIXTURE_PHASES[index % FIXTURE_PHASES.len()]; + if index % 2 == 1 { + state.attempts[index].owner = None; + } + } + state + } + + fn read_state(git_dir: &Path) -> Result { + read_state(git_dir) + } + + fn persist(git_dir: &Path, state: &AdapterState) -> Result<()> { + write_state_durably(git_dir, state) + } + + fn persist_with_before_rename_hook( + git_dir: &Path, + state: &AdapterState, + before_rename: F, + ) -> Result<()> + where + F: FnOnce(&Path, &Path) -> Result<()>, + { + write_state_durably_inner(git_dir, state, before_rename) + } + + fn recovery_view(state: &AdapterState) -> RecoveryView { + match state.recovery { + RecoveryState::Clear => RecoveryView::Clear, + RecoveryState::Pending { generation } => RecoveryView::Pending(generation), + RecoveryState::Flushing { generation } => RecoveryView::Flushing(generation), + } + } + + fn next_recovery_generation(state: &AdapterState) -> u64 { + state.next_recovery_generation + } + + fn scope_ids(state: &AdapterState) -> Vec { + state + .attempts + .iter() + .map(|attempt| attempt.scope_id.clone()) + .collect() + } + + fn remove_attempt(git_dir: &Path, scope_id: &str) -> Result<()> { + remove_attempt(git_dir, scope_id) + } + + fn normalize_recovery_after_boundary_lock_acquired(git_dir: &Path) -> Result<()> { + normalize_recovery_after_boundary_lock_acquired(git_dir) + } + + fn complete_recovery_flush(git_dir: &Path, generation: u64) -> Result { + Ok(match complete_recovery_flush(git_dir, generation)? { + RecoveryFlushCompletion::Cleared => CompletionView::Cleared, + RecoveryFlushCompletion::Superseded => CompletionView::Superseded, + }) + } + + fn relinquish_recovery_flush(git_dir: &Path, generation: u64) -> Result<()> { + relinquish_recovery_flush(git_dir, generation) + } + + fn claim_flush_by_admitting_a_new_attempt( + git_dir: &Path, + contender: usize, + ) -> Result { + let decision = admit_tracked_attempt(git_dir, &fixture_key(contender), "bash")?; + Ok(match decision { + AdmitDecision::FlushClaimed { generation } => FlushClaimView::Claimed(generation), + AdmitDecision::RecoveryBlocked => FlushClaimView::Blocked, + other => FlushClaimView::Other(format!("{other:?}")), + }) + } + } + + mutation_scope_state_conformance_tests!(OpenCodeStateConformance); +} + #[cfg(test)] mod tests { use std::sync::atomic::{AtomicU64, Ordering}; @@ -540,19 +672,6 @@ mod tests { } } - #[test] - fn read_state_returns_default_when_file_is_absent() { - let git_dir = unique_test_git_dir("read-default"); - - let state = read_state(&git_dir).expect("missing state file should read as default"); - assert_eq!(state, AdapterState::default()); - assert_eq!(state.version, 1); - assert!(state.recovery.is_clear()); - assert_eq!(state.next_recovery_generation, 1); - - remove_test_git_dir(&git_dir); - } - #[test] fn admit_persists_the_pending_start_attempt_before_returning() { let git_dir = unique_test_git_dir("admit-persists-pending-start"); @@ -565,9 +684,11 @@ mod tests { let state = read_state(&git_dir).expect("state should be readable"); assert_eq!(state.attempts.len(), 1); - assert_eq!(state.attempts[0].scope_id, allocated.attempt.scope_id); + assert_eq!(state.attempts[0], allocated.attempt); assert_eq!(state.attempts[0].phase, AttemptPhase::PendingStart); + assert_eq!(state.attempts[0].session_id, "ses-1"); assert_eq!(state.attempts[0].call_id, "call-1"); + assert_eq!(state.attempts[0].tool_name, "write"); remove_test_git_dir(&git_dir); } @@ -778,56 +899,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn only_one_concurrent_caller_claims_the_flush_for_a_generation() { - let git_dir = unique_test_git_dir("one-flush-owner"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - let generation = arm_recovery(&git_dir).expect("arming recovery should succeed"); - - let handles: Vec<_> = ["a", "b"] - .into_iter() - .map(|suffix| { - let git_dir = git_dir.clone(); - thread::spawn(move || { - admit_tracked_attempt( - &git_dir, - &key("ses-1", &format!("call-{suffix}")), - "bash", - ) - .expect("admit should not error") - }) - }) - .collect(); - - let decisions: Vec = handles - .into_iter() - .map(|handle| handle.join().expect("thread should not panic")) - .collect(); - - let flush_claims = decisions - .iter() - .filter(|decision| matches!(decision, AdmitDecision::FlushClaimed { .. })) - .count(); - let blocked = decisions - .iter() - .filter(|decision| matches!(decision, AdmitDecision::RecoveryBlocked)) - .count(); - assert_eq!(flush_claims, 1, "exactly one process may claim Flushing(g)"); - assert_eq!( - blocked, 1, - "the other concurrent caller must stay fail-closed" - ); - assert_eq!( - decisions.iter().find_map(|decision| match decision { - AdmitDecision::FlushClaimed { generation } => Some(*generation), - _ => None, - }), - Some(generation), - ); - - remove_test_git_dir(&git_dir); - } - #[test] fn arm_recovery_moves_clear_to_pending_with_a_fresh_generation() { let git_dir = unique_test_git_dir("arm-clear-to-pending"); @@ -876,70 +947,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn complete_recovery_flush_clears_only_with_the_matching_generation() { - let git_dir = unique_test_git_dir("complete-matching-generation"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - arm_recovery(&git_dir).expect("arm"); - admit(&git_dir, &key("ses-1", "call-new"), "bash"); - - assert_eq!( - complete_recovery_flush(&git_dir, 2).expect("wrong-generation completion"), - RecoveryFlushCompletion::Superseded, - ); - assert_eq!( - complete_recovery_flush(&git_dir, 1).expect("matching completion"), - RecoveryFlushCompletion::Cleared, - ); - assert!(read_state(&git_dir) - .expect("state readable") - .recovery - .is_clear()); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn normalize_after_boundary_lock_reclaims_orphaned_flushing_to_pending_same_generation() { - let git_dir = unique_test_git_dir("normalize-orphaned-flushing"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - arm_recovery(&git_dir).expect("arm g1"); - admit(&git_dir, &key("ses-1", "call-new"), "bash"); - assert_eq!( - read_state(&git_dir).expect("state readable").recovery, - RecoveryState::Flushing { generation: 1 }, - ); - - normalize_recovery_after_boundary_lock_acquired(&git_dir) - .expect("normalize should succeed"); - - assert_eq!( - read_state(&git_dir).expect("state readable").recovery, - RecoveryState::Pending { generation: 1 }, - ); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn relinquish_recovery_flush_returns_a_claimed_generation_to_pending() { - let git_dir = unique_test_git_dir("relinquish-to-pending"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - arm_recovery(&git_dir).expect("arm"); - admit(&git_dir, &key("ses-1", "call-new"), "bash"); - - relinquish_recovery_flush(&git_dir, 1).expect("relinquish should succeed"); - assert_eq!( - read_state(&git_dir).expect("state readable").recovery, - RecoveryState::Pending { generation: 1 }, - ); - - remove_test_git_dir(&git_dir); - } - #[test] fn mark_active_transitions_phase_and_rejects_unknown_scope_ids() { let git_dir = unique_test_git_dir("mark-active"); @@ -962,103 +969,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn removing_an_already_removed_attempt_is_a_safe_no_op() { - let git_dir = unique_test_git_dir("remove-idempotent"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - let attempt = admit_and_activate(&git_dir, &key("ses-1", "call-1"), "bash"); - - remove_attempt(&git_dir, &attempt.scope_id).expect("first removal should succeed"); - remove_attempt(&git_dir, &attempt.scope_id) - .expect("duplicate terminal delivery after cleanup must be a safe no-op"); - - assert!(read_state(&git_dir) - .expect("state readable") - .attempts - .is_empty()); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn state_round_trips_durably_through_the_canonical_path() { - let git_dir = unique_test_git_dir("round-trip"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - let attempt = admit_and_activate(&git_dir, &key("ses-7", "call-9"), "apply_patch"); - arm_recovery(&git_dir).expect("arming recovery should succeed"); - - let reloaded = read_state(&git_dir).expect("state should reload"); - assert_eq!(reloaded.version, ADAPTER_STATE_VERSION); - assert_eq!(reloaded.recovery, RecoveryState::Pending { generation: 1 }); - assert_eq!(reloaded.attempts.len(), 1); - assert_eq!(reloaded.attempts[0].phase, AttemptPhase::Active); - assert_eq!(reloaded.attempts[0].tool_name, "apply_patch"); - assert_eq!(reloaded.attempts[0].scope_id, attempt.scope_id); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn malformed_state_file_is_rejected_without_fabricating_bookkeeping() { - let git_dir = unique_test_git_dir("malformed-json"); - let dir = adapter_state_dir(&git_dir); - std::fs::create_dir_all(&dir).expect("state dir should be created"); - std::fs::write(state_path(&git_dir), b"not json") - .expect("malformed file should be written"); - - let error = read_state(&git_dir).expect_err("malformed state file must be rejected"); - assert!(error.to_string().contains("malformed")); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn an_unsupported_version_state_file_is_rejected() { - let git_dir = unique_test_git_dir("unsupported-version"); - let dir = adapter_state_dir(&git_dir); - std::fs::create_dir_all(&dir).expect("state dir should be created"); - std::fs::write( - state_path(&git_dir), - serde_json::json!({ - "version": 99, - "next_recovery_generation": 1, - "recovery": { "phase": "clear" }, - "attempts": [] - }) - .to_string(), - ) - .expect("state file should be written"); - - let error = read_state(&git_dir).expect_err("unsupported version must be rejected"); - assert!(error.to_string().contains("unsupported version")); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn interruption_before_rename_leaves_the_canonical_path_unaffected() { - let git_dir = unique_test_git_dir("interrupted-before-rename"); - let dir = adapter_state_dir(&git_dir); - std::fs::create_dir_all(&dir).expect("state dir should be created"); - - let state = AdapterState::default(); - let result = write_state_durably_inner(&git_dir, &state, |tmp_path, canonical_path| { - assert!(tmp_path.exists()); - assert!(!canonical_path.exists()); - Err(anyhow!("injected interruption before rename")) - }); - - assert!(result.is_err()); - assert!(!state_path(&git_dir).exists()); - assert_eq!( - read_state(&git_dir).expect("read should not error on an absent canonical file"), - AdapterState::default(), - ); - - remove_test_git_dir(&git_dir); - } - const PARALLEL_ADMISSION_COUNT: usize = 6; #[test] @@ -1107,17 +1017,4 @@ mod tests { remove_test_git_dir(&git_dir); } - - #[test] - fn adapter_state_files_live_only_below_git_dir_sce() { - let git_dir = unique_test_git_dir("path-boundary"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - admit(&git_dir, &key("ses-1", "call-1"), "bash"); - - let sce_dir = git_dir.join(SCE_STATE_DIR); - assert!(state_path(&git_dir).starts_with(&sce_dir)); - - remove_test_git_dir(&git_dir); - } } diff --git a/cli/src/services/hooks/pi_mutation_scope/state.rs b/cli/src/services/hooks/pi_mutation_scope/state.rs index a64c37aff..923d0d891 100644 --- a/cli/src/services/hooks/pi_mutation_scope/state.rs +++ b/cli/src/services/hooks/pi_mutation_scope/state.rs @@ -450,6 +450,135 @@ pub(crate) fn set_attempt_owner_for_tests( updated } +#[cfg(test)] +mod state_conformance { + use super::*; + use crate::services::hooks::mutation_scope_state_conformance::{ + mutation_scope_state_conformance_tests, CompletionView, FlushClaimView, RecoveryView, + StateConformance, + }; + + const FIXTURE_SESSION: &str = "ses-conformance"; + const FIXTURE_PHASES: [AttemptPhase; 3] = [ + AttemptPhase::Executed, + AttemptPhase::PendingAbandon, + AttemptPhase::PendingStart, + ]; + + struct PiStateConformance; + + fn fixture_key(index: usize) -> AttemptKey { + AttemptKey { + session_id: FIXTURE_SESSION.to_string(), + tool_call_id: format!("call-{index}"), + } + } + + impl StateConformance for PiStateConformance { + type State = AdapterState; + + const ADAPTER: &'static str = "pi"; + const SUPPORTED_VERSION: u32 = ADAPTER_STATE_VERSION; + + fn state_path(git_dir: &Path) -> PathBuf { + state_path(git_dir) + } + + fn fixture_state( + recovery: RecoveryView, + next_recovery_generation: u64, + attempt_count: usize, + ) -> AdapterState { + let mut state = AdapterState { + next_recovery_generation, + recovery: match recovery { + RecoveryView::Clear => RecoveryState::Clear, + RecoveryView::Pending(generation) => RecoveryState::Pending { generation }, + RecoveryView::Flushing(generation) => RecoveryState::Flushing { generation }, + }, + ..AdapterState::default() + }; + for index in 0..attempt_count { + allocate_pending_start(&mut state, &fixture_key(index), "write"); + state.attempts[index].phase = FIXTURE_PHASES[index % FIXTURE_PHASES.len()]; + } + state + } + + fn read_state(git_dir: &Path) -> Result { + read_state(git_dir) + } + + fn persist(git_dir: &Path, state: &AdapterState) -> Result<()> { + write_state_durably(git_dir, state) + } + + fn persist_with_before_rename_hook( + git_dir: &Path, + state: &AdapterState, + before_rename: F, + ) -> Result<()> + where + F: FnOnce(&Path, &Path) -> Result<()>, + { + write_state_durably_inner(git_dir, state, before_rename) + } + + fn recovery_view(state: &AdapterState) -> RecoveryView { + match state.recovery { + RecoveryState::Clear => RecoveryView::Clear, + RecoveryState::Pending { generation } => RecoveryView::Pending(generation), + RecoveryState::Flushing { generation } => RecoveryView::Flushing(generation), + } + } + + fn next_recovery_generation(state: &AdapterState) -> u64 { + state.next_recovery_generation + } + + fn scope_ids(state: &AdapterState) -> Vec { + state + .attempts + .iter() + .map(|attempt| attempt.scope_id.clone()) + .collect() + } + + fn remove_attempt(git_dir: &Path, scope_id: &str) -> Result<()> { + remove_attempt(git_dir, scope_id) + } + + fn normalize_recovery_after_boundary_lock_acquired(git_dir: &Path) -> Result<()> { + normalize_recovery_after_boundary_lock_acquired(git_dir) + } + + fn complete_recovery_flush(git_dir: &Path, generation: u64) -> Result { + Ok(match complete_recovery_flush(git_dir, generation)? { + RecoveryFlushCompletion::Cleared => CompletionView::Cleared, + RecoveryFlushCompletion::Superseded => CompletionView::Superseded, + }) + } + + fn relinquish_recovery_flush(git_dir: &Path, generation: u64) -> Result<()> { + relinquish_recovery_flush(git_dir, generation) + } + + fn claim_flush_by_admitting_a_new_attempt( + git_dir: &Path, + contender: usize, + ) -> Result { + let decision = admit_tracked_attempt(git_dir, &fixture_key(contender), "bash")?; + Ok(match decision { + AdmitDecision::FlushClaimed { generation } => FlushClaimView::Claimed(generation), + AdmitDecision::RecoveryBlocked => FlushClaimView::Blocked, + other => FlushClaimView::Other(format!("{other:?}")), + }) + } + } + + mutation_scope_state_conformance_tests!(PiStateConformance); +} + #[cfg(test)] mod tests { use std::sync::atomic::{AtomicU64, Ordering}; @@ -482,20 +611,6 @@ mod tests { admit_tracked_attempt(git_dir, key, tool_name).expect("admit should not error") } - #[test] - fn read_state_returns_default_when_file_is_absent() { - let git_dir = unique_test_git_dir("read-default"); - - let state = read_state(&git_dir).expect("missing state file should read as default"); - assert_eq!(state, AdapterState::default()); - assert_eq!(state.version, ADAPTER_STATE_VERSION); - assert!(state.recovery.is_clear()); - assert_eq!(state.next_recovery_generation, 1); - assert_eq!(state.next_attempt_seq, 1); - - remove_test_git_dir(&git_dir); - } - #[test] fn admit_persists_the_pending_start_attempt_with_attempt_seq_one() { let git_dir = unique_test_git_dir("admit-persists-pending-start"); @@ -509,9 +624,11 @@ mod tests { let state = read_state(&git_dir).expect("state should be readable"); assert_eq!(state.attempts.len(), 1); - assert_eq!(state.attempts[0].scope_id, allocated.attempt.scope_id); + assert_eq!(state.attempts[0], allocated.attempt); assert_eq!(state.attempts[0].phase, AttemptPhase::PendingStart); + assert_eq!(state.attempts[0].session_id, "ses-1"); assert_eq!(state.attempts[0].tool_call_id, "call-1"); + assert_eq!(state.attempts[0].tool_name, "write"); assert_eq!(state.next_attempt_seq, 2); remove_test_git_dir(&git_dir); @@ -631,101 +748,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn only_one_concurrent_caller_claims_the_flush_for_a_generation() { - let git_dir = unique_test_git_dir("one-flush-owner"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - seed_attempt_for_tests( - &git_dir, - &key("ses-1", "call-doomed"), - "bash", - AttemptPhase::PendingAbandon, - ); - let generation = begin_terminal_cleanup(&git_dir, &[]).expect("arm recovery"); - relinquish_recovery_flush(&git_dir, generation).expect("relinquish to Pending"); - - let handles: Vec<_> = ["a", "b"] - .into_iter() - .map(|suffix| { - let git_dir = git_dir.clone(); - thread::spawn(move || { - admit_tracked_attempt( - &git_dir, - &key("ses-1", &format!("call-{suffix}")), - "bash", - ) - .expect("admit should not error") - }) - }) - .collect(); - - let decisions: Vec = handles - .into_iter() - .map(|handle| handle.join().expect("thread should not panic")) - .collect(); - - let flush_claims = decisions - .iter() - .filter(|decision| matches!(decision, AdmitDecision::FlushClaimed { .. })) - .count(); - let blocked = decisions - .iter() - .filter(|decision| matches!(decision, AdmitDecision::RecoveryBlocked)) - .count(); - assert_eq!(flush_claims, 1, "exactly one process may claim Flushing(g)"); - assert_eq!( - blocked, 1, - "the other concurrent caller must stay fail-closed" - ); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn complete_recovery_flush_clears_only_with_the_matching_generation() { - let git_dir = unique_test_git_dir("complete-matching-generation"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - let generation = begin_terminal_cleanup(&git_dir, &[]).expect("arm"); - - assert_eq!( - complete_recovery_flush(&git_dir, generation + 1).expect("wrong-generation completion"), - RecoveryFlushCompletion::Superseded, - ); - assert_eq!( - complete_recovery_flush(&git_dir, generation).expect("matching completion"), - RecoveryFlushCompletion::Cleared, - ); - assert!(read_state(&git_dir) - .expect("state readable") - .recovery - .is_clear()); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn normalize_after_boundary_lock_reclaims_orphaned_flushing_to_pending_same_generation() { - let git_dir = unique_test_git_dir("normalize-orphaned-flushing"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - let generation = begin_terminal_cleanup(&git_dir, &[]).expect("arm g1"); - assert_eq!( - read_state(&git_dir).expect("state readable").recovery, - RecoveryState::Flushing { generation }, - ); - - normalize_recovery_after_boundary_lock_acquired(&git_dir) - .expect("normalize should succeed"); - - assert_eq!( - read_state(&git_dir).expect("state readable").recovery, - RecoveryState::Pending { generation }, - ); - - remove_test_git_dir(&git_dir); - } - #[test] fn mark_executed_transitions_pending_start_only() { let git_dir = unique_test_git_dir("mark-executed"); @@ -765,27 +787,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn removing_an_already_removed_attempt_is_a_safe_no_op() { - let git_dir = unique_test_git_dir("remove-idempotent"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - let AdmitDecision::Admitted(allocated) = admit(&git_dir, &key("ses-1", "call-1"), "bash") - else { - panic!("expected Admitted"); - }; - - remove_attempt(&git_dir, &allocated.attempt.scope_id).expect("first removal succeeds"); - remove_attempt(&git_dir, &allocated.attempt.scope_id) - .expect("duplicate terminal delivery after cleanup must be a safe no-op"); - - assert!(read_state(&git_dir) - .expect("state readable") - .attempts - .is_empty()); - - remove_test_git_dir(&git_dir); - } - #[test] fn a_new_attempt_after_terminal_cleanup_gets_a_fresh_attempt_seq_and_scope_id() { let git_dir = unique_test_git_dir("terminal-scope-id-non-reuse"); @@ -811,88 +812,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn state_round_trips_durably_through_the_canonical_path() { - let git_dir = unique_test_git_dir("round-trip"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - let AdmitDecision::Admitted(allocated) = admit(&git_dir, &key("ses-7", "call-9"), "write") - else { - panic!("expected Admitted"); - }; - mark_executed(&git_dir, &key("ses-7", "call-9")).expect("mark_executed succeeds"); - - let reloaded = read_state(&git_dir).expect("state should reload"); - assert_eq!(reloaded.version, ADAPTER_STATE_VERSION); - assert_eq!(reloaded.attempts.len(), 1); - assert_eq!(reloaded.attempts[0].phase, AttemptPhase::Executed); - assert_eq!(reloaded.attempts[0].tool_name, "write"); - assert_eq!(reloaded.attempts[0].scope_id, allocated.attempt.scope_id); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn malformed_state_file_is_rejected_without_fabricating_bookkeeping() { - let git_dir = unique_test_git_dir("malformed-json"); - let dir = adapter_state_dir(&git_dir); - std::fs::create_dir_all(&dir).expect("state dir should be created"); - std::fs::write(state_path(&git_dir), b"not json") - .expect("malformed file should be written"); - - let error = read_state(&git_dir).expect_err("malformed state file must be rejected"); - assert!(error.to_string().contains("malformed")); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn an_unsupported_version_state_file_is_rejected() { - let git_dir = unique_test_git_dir("unsupported-version"); - let dir = adapter_state_dir(&git_dir); - std::fs::create_dir_all(&dir).expect("state dir should be created"); - std::fs::write( - state_path(&git_dir), - serde_json::json!({ - "version": 99, - "next_attempt_seq": 1, - "next_recovery_generation": 1, - "recovery": { "phase": "clear" }, - "attempts": [] - }) - .to_string(), - ) - .expect("state file should be written"); - - let error = read_state(&git_dir).expect_err("unsupported version must be rejected"); - assert!(error.to_string().contains("unsupported version")); - - remove_test_git_dir(&git_dir); - } - - #[test] - fn interruption_before_rename_leaves_the_canonical_path_unaffected() { - let git_dir = unique_test_git_dir("interrupted-before-rename"); - let dir = adapter_state_dir(&git_dir); - std::fs::create_dir_all(&dir).expect("state dir should be created"); - - let state = AdapterState::default(); - let result = write_state_durably_inner(&git_dir, &state, |tmp_path, canonical_path| { - assert!(tmp_path.exists()); - assert!(!canonical_path.exists()); - Err(anyhow!("injected interruption before rename")) - }); - - assert!(result.is_err()); - assert!(!state_path(&git_dir).exists()); - assert_eq!( - read_state(&git_dir).expect("read should not error on an absent canonical file"), - AdapterState::default(), - ); - - remove_test_git_dir(&git_dir); - } - const PARALLEL_ADMISSION_COUNT: usize = 6; #[test] @@ -935,17 +854,4 @@ mod tests { remove_test_git_dir(&git_dir); } - - #[test] - fn adapter_state_files_live_only_below_git_dir_sce() { - let git_dir = unique_test_git_dir("path-boundary"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - admit(&git_dir, &key("ses-1", "call-1"), "bash"); - - let sce_dir = git_dir.join(SCE_STATE_DIR); - assert!(state_path(&git_dir).starts_with(&sce_dir)); - - remove_test_git_dir(&git_dir); - } } diff --git a/context/cli/codex-mutation-scope-integration.md b/context/cli/codex-mutation-scope-integration.md index 98753bf23..a72ebff88 100644 --- a/context/cli/codex-mutation-scope-integration.md +++ b/context/cli/codex-mutation-scope-integration.md @@ -146,6 +146,12 @@ file existence alone is never ownership. Both locks are `AdapterLockSpec` identities (`state::STATE_LOCK`, `state::BOUNDARY_LOCK`) over the single OS advisory-lock implementation in `cli/src/services/hooks/mutation_scope_lock.rs`, shared with the OpenCode and Pi adapters. +The persistence and recovery-bookkeeping contracts common to the Codex, OpenCode, +and Pi state files (missing/malformed/wrong-version reads, durable replace, +remove, normalize, complete, relinquish, single flush claimant) are asserted once +by the test-only suite in `cli/src/services/hooks/mutation_scope_state_conformance.rs`, +instantiated per adapter as `_mutation_scope::state::state_conformance`; +admission, lane, phase, and owner semantics stay in each adapter's own tests. Positive cleanup arms `recovery_pending`, abandons matching tracked scopes via generic `abandon`, and removes settled attempts. Known attempts keep tracked diff --git a/context/cli/doctor-recovery-safety-model.md b/context/cli/doctor-recovery-safety-model.md index 9185e9684..397726603 100644 --- a/context/cli/doctor-recovery-safety-model.md +++ b/context/cli/doctor-recovery-safety-model.md @@ -56,7 +56,7 @@ each adapter's real dispatch/recovery functions, not reading enum names. | `RecoveryNeverClearedWithUnresolvedAbandon` — doctor cannot clear recovery while unresolved lifecycle evidence exists | `claude_mutation_scope::health::tests::repair_blocked_removes_only_successfully_abandoned_attempts_and_keeps_recovery_pending_when_one_fails` | | `NoOrdinaryTransitionProducesInvalidHealth` — `Invalid` is reserved for structurally impossible states, never an ordinary transition's output | `claude_mutation_scope::health::tests::clear_recovery_with_pending_abandon_is_invalid`, `claude_mutation_scope::health::tests::invalid_takes_priority_over_blocked_in_mixed_impossible_state`, `opencode_mutation_scope::health::tests::clear_recovery_with_a_pending_abandon_attempt_is_a_structurally_impossible_state_classified_invalid` | | `DoctorRepairProducesOnlyOrdinaryLifecycleShapes` — a doctor repair cannot produce a state outside the adapter's own reachable transition set | `opencode_mutation_scope::health::tests::repair_blocked_clears_a_dead_owner_pending_start_end_to_end`; `claude_mutation_scope::health::tests::repair_blocked_removes_only_successfully_abandoned_attempts_and_keeps_recovery_pending_when_one_fails` | -| `RemovedAttemptsAreNeverResurrected` — a terminal/removed attempt is never resurrected | `claude_mutation_scope::state::tests::removing_an_already_removed_attempt_is_a_safe_no_op`, `opencode_mutation_scope::state::tests::removing_an_already_removed_attempt_is_a_safe_no_op`, `opencode_mutation_scope::tests::regression_f_start_replay_for_a_pending_abandon_identity_never_reactivates` | +| `RemovedAttemptsAreNeverResurrected` — a terminal/removed attempt is never resurrected | `claude_mutation_scope::state::tests::removing_an_already_removed_attempt_is_a_safe_no_op`, `opencode_mutation_scope::state::state_conformance::removing_an_already_removed_attempt_is_a_safe_no_op` (shared contract in `cli/src/services/hooks/mutation_scope_state_conformance.rs`, also instantiated for Codex and Pi), `opencode_mutation_scope::tests::regression_f_start_replay_for_a_pending_abandon_identity_never_reactivates` | | `InterruptedRecoveryStaysInOrdinaryRetryableState` — an interrupted repair remains fail-closed and retryable | `claude_mutation_scope::health::tests::repair_blocked_interrupted_by_a_failing_seam_leaves_state_a_later_repair_completes_without_duplication`, `opencode_mutation_scope::health::tests::repair_blocked_interrupted_before_the_seam_resolves_leaves_state_the_ordinary_recovery_path_completes_without_duplication` | | `ReportedFixedExcludesBlockedOrInvalid` — a reported `fixed` repair cannot coincide with a final `Blocked`/`Invalid` health | `doctor::inspect::tests::finalize_mutation_scope_repair_results_ignores_an_immediate_post_repair_read_that_the_final_report_contradicts`; `doctor::inspect::tests::full_report_multi_adapter_fix_mode_resolves_one_target_and_leaves_the_other_manual` (Claude's `Fixed` report is derived only from the final, freshly recomputed row, at the same moment the adjacent OpenCode row in that identical final report is still `Blocked`) |