From 705095b4c8d41ed22036f99cb75091d3549ebced Mon Sep 17 00:00:00 2001 From: David Abram Date: Thu, 1 Oct 2026 02:17:06 +0200 Subject: [PATCH 1/2] tests: Share mutation scope state conformance coverage The Codex, OpenCode, and Pi adapters now instantiate one common test-only suite for persistence and recovery bookkeeping, covering durable replacement, malformed and version validation, cleanup idempotence, recovery transitions, and concurrent flush claims. Remove duplicated adapter-specific coverage and update the recovery safety documentation to reference the shared contract. Co-authored-by: SCE --- .../hooks/codex_mutation_scope/state.rs | 465 +++++-------- cli/src/services/hooks/mod.rs | 2 + .../hooks/mutation_scope_state_conformance.rs | 641 ++++++++++++++++++ .../hooks/opencode_mutation_scope/state.rs | 377 ++++------ .../services/hooks/pi_mutation_scope/state.rs | 362 ++++------ .../cli/codex-mutation-scope-integration.md | 6 + context/cli/doctor-recovery-safety-model.md | 2 +- 7 files changed, 1076 insertions(+), 779 deletions(-) create mode 100644 cli/src/services/hooks/mutation_scope_state_conformance.rs diff --git a/cli/src/services/hooks/codex_mutation_scope/state.rs b/cli/src/services/hooks/codex_mutation_scope/state.rs index 8d81a8e0a..8887f1a55 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; 2] = [AttemptPhase::PendingStart, AttemptPhase::Active]; + + 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 = 3; + + fn state_path(git_dir: &Path) -> PathBuf { + state_path(git_dir) + } + + fn default_state() -> AdapterState { + AdapterState::default() + } + + 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..adcb24083 --- /dev/null +++ b/cli/src/services/hooks/mutation_scope_state_conformance.rs @@ -0,0 +1,641 @@ +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; +const PARALLEL_REMOVAL_COUNT: usize = 8; + +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: Clone + Debug + PartialEq; + + const ADAPTER: &'static str; + const SUPPORTED_VERSION: u32; + + fn state_path(git_dir: &Path) -> PathBuf; + + fn default_state() -> Self::State; + + 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 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::default_state()); + 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 = A::fixture_state( + RecoveryView::Pending(SEEDED_GENERATION), + SEEDED_NEXT_GENERATION, + 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::default_state()); +} + +pub(crate) fn state_round_trips_durably_through_the_canonical_path() { + let git_dir = ConformanceGitDir::create::("round-trip"); + let attempt_count = 3; + + for recovery in [ + RecoveryView::Flushing(SEEDED_GENERATION), + RecoveryView::Pending(SEEDED_GENERATION), + RecoveryView::Clear, + ] { + let state = A::fixture_state(recovery, SEEDED_NEXT_GENERATION, attempt_count); + 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(), + &A::fixture_state( + RecoveryView::Pending(SEEDED_GENERATION), + SEEDED_NEXT_GENERATION, + 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 = A::fixture_state( + RecoveryView::Pending(SEEDED_GENERATION), + SEEDED_NEXT_GENERATION, + 1, + ); + let replacement = A::fixture_state( + RecoveryView::Flushing(SEEDED_GENERATION + 1), + SEEDED_NEXT_GENERATION + 1, + 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::default_state()); + + 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 = A::fixture_state( + RecoveryView::Pending(SEEDED_GENERATION), + SEEDED_NEXT_GENERATION, + 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 = A::fixture_state( + RecoveryView::Flushing(SEEDED_GENERATION), + SEEDED_NEXT_GENERATION, + 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::Flushing(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 = A::fixture_state( + RecoveryView::Flushing(SEEDED_GENERATION), + SEEDED_NEXT_GENERATION, + 2, + ); + 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 + ); + assert_eq!(A::scope_ids(&normalized), A::scope_ids(&seeded)); +} + +pub(crate) fn complete_recovery_flush_clears_only_with_the_matching_generation< + A: StateConformance, +>() { + let git_dir = ConformanceGitDir::create::("complete-matching-generation"); + let seeded = A::fixture_state( + RecoveryView::Flushing(SEEDED_GENERATION), + SEEDED_NEXT_GENERATION, + 2, + ); + 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 + ); + assert_eq!(A::scope_ids(&completed), A::scope_ids(&seeded)); +} + +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 = A::fixture_state( + RecoveryView::Flushing(SEEDED_GENERATION), + SEEDED_NEXT_GENERATION, + 2, + ); + 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 + ); + assert_eq!(A::scope_ids(&relinquished), A::scope_ids(&seeded)); + 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(), + &A::fixture_state( + RecoveryView::Pending(SEEDED_GENERATION), + SEEDED_NEXT_GENERATION, + 0, + ), + ); + 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()); +} + +pub(crate) fn parallel_state_transactions_serialize_without_lost_updates() { + let git_dir = ConformanceGitDir::create::("parallel-transactions"); + let seeded = A::fixture_state( + RecoveryView::Flushing(SEEDED_GENERATION), + SEEDED_NEXT_GENERATION, + PARALLEL_REMOVAL_COUNT + 1, + ); + seed::(git_dir.path(), &seeded); + let mut scope_ids = A::scope_ids(&seeded); + let survivor = scope_ids.pop().expect("fixture should hold a survivor"); + let start = Barrier::new(PARALLEL_REMOVAL_COUNT + 1); + + thread::scope(|scope| { + let start = &start; + let git_dir = git_dir.path(); + for scope_id in &scope_ids { + scope.spawn(move || { + start.wait(); + A::remove_attempt(git_dir, scope_id).expect("concurrent removal should succeed"); + }); + } + scope.spawn(move || { + start.wait(); + A::normalize_recovery_after_boundary_lock_acquired(git_dir) + .expect("concurrent normalize should succeed"); + }); + }); + + let converged = reload::(git_dir.path()); + assert_eq!(A::scope_ids(&converged), vec![survivor]); + assert_eq!( + A::recovery_view(&converged), + RecoveryView::Pending(SEEDED_GENERATION) + ); + assert_eq!( + A::next_recovery_generation(&converged), + SEEDED_NEXT_GENERATION + ); +} + +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, + parallel_state_transactions_serialize_without_lost_updates, + } + }; + ($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..31db161ff 100644 --- a/cli/src/services/hooks/opencode_mutation_scope/state.rs +++ b/cli/src/services/hooks/opencode_mutation_scope/state.rs @@ -496,6 +496,142 @@ 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::PendingStart, + AttemptPhase::Active, + AttemptPhase::PendingAbandon, + ]; + + 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 = 1; + + fn state_path(git_dir: &Path) -> PathBuf { + state_path(git_dir) + } + + fn default_state() -> AdapterState { + AdapterState::default() + } + + 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 +676,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 +688,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 +903,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 +951,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 +973,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 +1021,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..bcf094683 100644 --- a/cli/src/services/hooks/pi_mutation_scope/state.rs +++ b/cli/src/services/hooks/pi_mutation_scope/state.rs @@ -450,6 +450,139 @@ 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::PendingStart, + AttemptPhase::Executed, + AttemptPhase::PendingAbandon, + ]; + + 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 = 2; + + fn state_path(git_dir: &Path) -> PathBuf { + state_path(git_dir) + } + + fn default_state() -> AdapterState { + AdapterState::default() + } + + 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 +615,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 +628,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 +752,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 +791,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 +816,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 +858,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`) | From 4adb0fec1f6a59beb8c59c24a727bddbc6a9df23 Mon Sep 17 00:00:00 2001 From: David Abram Date: Thu, 1 Oct 2026 02:40:44 +0200 Subject: [PATCH 2/2] tests: Align mutation-scope state conformance fixtures Use each adapter's declared state version and the shared default implementation in conformance tests. Separate quiescent and populated fixtures so recovery transitions are tested with the state shapes they actually require, while removing the outdated parallel transaction contract. Co-authored-by: SCE --- .../hooks/codex_mutation_scope/state.rs | 12 +- .../hooks/mutation_scope_state_conformance.rs | 155 +++++------------- .../hooks/opencode_mutation_scope/state.rs | 8 +- .../services/hooks/pi_mutation_scope/state.rs | 8 +- 4 files changed, 53 insertions(+), 130 deletions(-) diff --git a/cli/src/services/hooks/codex_mutation_scope/state.rs b/cli/src/services/hooks/codex_mutation_scope/state.rs index 8887f1a55..72016ef49 100644 --- a/cli/src/services/hooks/codex_mutation_scope/state.rs +++ b/cli/src/services/hooks/codex_mutation_scope/state.rs @@ -441,7 +441,11 @@ mod state_conformance { }; const FIXTURE_SESSION: &str = "session-conformance"; - const FIXTURE_PHASES: [AttemptPhase; 2] = [AttemptPhase::PendingStart, AttemptPhase::Active]; + const FIXTURE_PHASES: [AttemptPhase; 3] = [ + AttemptPhase::Active, + AttemptPhase::Active, + AttemptPhase::PendingStart, + ]; struct CodexStateConformance; @@ -457,16 +461,12 @@ mod state_conformance { type State = AdapterState; const ADAPTER: &'static str = "codex"; - const SUPPORTED_VERSION: u32 = 3; + const SUPPORTED_VERSION: u32 = ADAPTER_STATE_VERSION; fn state_path(git_dir: &Path) -> PathBuf { state_path(git_dir) } - fn default_state() -> AdapterState { - AdapterState::default() - } - fn fixture_state( recovery: RecoveryView, next_recovery_generation: u64, diff --git a/cli/src/services/hooks/mutation_scope_state_conformance.rs b/cli/src/services/hooks/mutation_scope_state_conformance.rs index adcb24083..22486a6c5 100644 --- a/cli/src/services/hooks/mutation_scope_state_conformance.rs +++ b/cli/src/services/hooks/mutation_scope_state_conformance.rs @@ -13,7 +13,6 @@ 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; -const PARALLEL_REMOVAL_COUNT: usize = 8; static NEXT_CONFORMANCE_GIT_DIR_ID: AtomicU64 = AtomicU64::new(0); @@ -38,15 +37,13 @@ pub(crate) enum FlushClaimView { } pub(crate) trait StateConformance { - type State: Clone + Debug + PartialEq; + type State: Debug + Default + PartialEq; const ADAPTER: &'static str; const SUPPORTED_VERSION: u32; fn state_path(git_dir: &Path) -> PathBuf; - fn default_state() -> Self::State; - fn fixture_state( recovery: RecoveryView, next_recovery_generation: u64, @@ -110,6 +107,18 @@ impl Drop for ConformanceGitDir { } } +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"); } @@ -171,7 +180,7 @@ pub(crate) fn missing_state_file_reads_as_default_without_fabricating_a_file< let state = A::read_state(git_dir.path()).expect("a missing state file must read as the default"); - assert_eq!(state, A::default_state()); + 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()); @@ -189,29 +198,34 @@ pub(crate) fn state_file_is_checkout_local_below_git_dir_sce(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::default_state()); + 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 attempt_count = 3; - - for recovery in [ - RecoveryView::Flushing(SEEDED_GENERATION), - RecoveryView::Pending(SEEDED_GENERATION), - RecoveryView::Clear, + 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 state = A::fixture_state(recovery, SEEDED_NEXT_GENERATION, attempt_count); let mut distinct_scope_ids = A::scope_ids(&state); distinct_scope_ids.sort_unstable(); distinct_scope_ids.dedup(); @@ -259,14 +273,7 @@ pub(crate) fn a_state_file_with_any_other_version_is_rejected("unsupported-version"); - seed::( - git_dir.path(), - &A::fixture_state( - RecoveryView::Pending(SEEDED_GENERATION), - SEEDED_NEXT_GENERATION, - 2, - ), - ); + 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"); @@ -292,16 +299,8 @@ pub(crate) fn interruption_before_rename_leaves_the_canonical_path_unaffected< >() { let git_dir = ConformanceGitDir::create::("interrupted-before-rename"); let canonical_path = A::state_path(git_dir.path()); - let established = A::fixture_state( - RecoveryView::Pending(SEEDED_GENERATION), - SEEDED_NEXT_GENERATION, - 1, - ); - let replacement = A::fixture_state( - RecoveryView::Flushing(SEEDED_GENERATION + 1), - SEEDED_NEXT_GENERATION + 1, - 3, - ); + let established = populated_pending_state::(1); + let replacement = populated_pending_state::(3); let error = A::persist_with_before_rename_hook( git_dir.path(), @@ -317,7 +316,7 @@ pub(crate) fn interruption_before_rename_leaves_the_canonical_path_unaffected< .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::default_state()); + assert_eq!(reload::(git_dir.path()), A::State::default()); seed::(git_dir.path(), &established); let established_bytes = canonical_bytes::(git_dir.path()); @@ -353,11 +352,7 @@ pub(crate) fn removing_an_unknown_attempt_is_a_safe_no_op() .expect("removing an unknown scope from absent state must succeed"); assert!(!A::state_path(git_dir.path()).exists()); - let seeded = A::fixture_state( - RecoveryView::Pending(SEEDED_GENERATION), - SEEDED_NEXT_GENERATION, - 2, - ); + let seeded = populated_pending_state::(2); seed::(git_dir.path(), &seeded); let seeded_bytes = canonical_bytes::(git_dir.path()); @@ -369,11 +364,7 @@ pub(crate) fn removing_an_unknown_attempt_is_a_safe_no_op() pub(crate) fn removing_an_already_removed_attempt_is_a_safe_no_op() { let git_dir = ConformanceGitDir::create::("remove-idempotent"); - let seeded = A::fixture_state( - RecoveryView::Flushing(SEEDED_GENERATION), - SEEDED_NEXT_GENERATION, - 3, - ); + let seeded = populated_pending_state::(3); seed::(git_dir.path(), &seeded); let scope_ids = A::scope_ids(&seeded); @@ -386,7 +377,7 @@ pub(crate) fn removing_an_already_removed_attempt_is_a_safe_no_op() { let git_dir = ConformanceGitDir::create::("normalize-orphaned-flushing"); - let seeded = A::fixture_state( - RecoveryView::Flushing(SEEDED_GENERATION), - SEEDED_NEXT_GENERATION, - 2, - ); + let seeded = quiescent_state::(RecoveryView::Flushing(SEEDED_GENERATION)); seed::(git_dir.path(), &seeded); A::normalize_recovery_after_boundary_lock_acquired(git_dir.path()) @@ -423,18 +410,13 @@ pub(crate) fn normalize_after_boundary_lock_reclaims_orphaned_flushing_to_pendin A::next_recovery_generation(&normalized), SEEDED_NEXT_GENERATION ); - assert_eq!(A::scope_ids(&normalized), A::scope_ids(&seeded)); } pub(crate) fn complete_recovery_flush_clears_only_with_the_matching_generation< A: StateConformance, >() { let git_dir = ConformanceGitDir::create::("complete-matching-generation"); - let seeded = A::fixture_state( - RecoveryView::Flushing(SEEDED_GENERATION), - SEEDED_NEXT_GENERATION, - 2, - ); + let seeded = quiescent_state::(RecoveryView::Flushing(SEEDED_GENERATION)); seed::(git_dir.path(), &seeded); let seeded_bytes = canonical_bytes::(git_dir.path()); @@ -464,18 +446,13 @@ pub(crate) fn complete_recovery_flush_clears_only_with_the_matching_generation< A::next_recovery_generation(&completed), SEEDED_NEXT_GENERATION ); - assert_eq!(A::scope_ids(&completed), A::scope_ids(&seeded)); } 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 = A::fixture_state( - RecoveryView::Flushing(SEEDED_GENERATION), - SEEDED_NEXT_GENERATION, - 2, - ); + let seeded = quiescent_state::(RecoveryView::Flushing(SEEDED_GENERATION)); seed::(git_dir.path(), &seeded); let seeded_bytes = canonical_bytes::(git_dir.path()); @@ -498,7 +475,6 @@ pub(crate) fn relinquish_recovery_flush_returns_only_the_claimed_generation_to_p A::next_recovery_generation(&relinquished), SEEDED_NEXT_GENERATION ); - assert_eq!(A::scope_ids(&relinquished), A::scope_ids(&seeded)); let relinquished_bytes = canonical_bytes::(git_dir.path()); A::relinquish_recovery_flush(git_dir.path(), SEEDED_GENERATION) @@ -510,11 +486,7 @@ pub(crate) fn only_one_concurrent_caller_claims_the_flush_for_a_generation("one-flush-owner"); seed::( git_dir.path(), - &A::fixture_state( - RecoveryView::Pending(SEEDED_GENERATION), - SEEDED_NEXT_GENERATION, - 0, - ), + &quiescent_state::(RecoveryView::Pending(SEEDED_GENERATION)), ); let start = Barrier::new(FLUSH_CLAIM_CONTENDER_COUNT); @@ -569,46 +541,6 @@ pub(crate) fn only_one_concurrent_caller_claims_the_flush_for_a_generation() { - let git_dir = ConformanceGitDir::create::("parallel-transactions"); - let seeded = A::fixture_state( - RecoveryView::Flushing(SEEDED_GENERATION), - SEEDED_NEXT_GENERATION, - PARALLEL_REMOVAL_COUNT + 1, - ); - seed::(git_dir.path(), &seeded); - let mut scope_ids = A::scope_ids(&seeded); - let survivor = scope_ids.pop().expect("fixture should hold a survivor"); - let start = Barrier::new(PARALLEL_REMOVAL_COUNT + 1); - - thread::scope(|scope| { - let start = &start; - let git_dir = git_dir.path(); - for scope_id in &scope_ids { - scope.spawn(move || { - start.wait(); - A::remove_attempt(git_dir, scope_id).expect("concurrent removal should succeed"); - }); - } - scope.spawn(move || { - start.wait(); - A::normalize_recovery_after_boundary_lock_acquired(git_dir) - .expect("concurrent normalize should succeed"); - }); - }); - - let converged = reload::(git_dir.path()); - assert_eq!(A::scope_ids(&converged), vec![survivor]); - assert_eq!( - A::recovery_view(&converged), - RecoveryView::Pending(SEEDED_GENERATION) - ); - assert_eq!( - A::next_recovery_generation(&converged), - SEEDED_NEXT_GENERATION - ); -} - macro_rules! mutation_scope_state_conformance_tests { ($adapter:ty) => { $crate::services::hooks::mutation_scope_state_conformance::mutation_scope_state_conformance_tests! { @@ -625,7 +557,6 @@ macro_rules! mutation_scope_state_conformance_tests { 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, - parallel_state_transactions_serialize_without_lost_updates, } }; ($adapter:ty => $($contract:ident),+ $(,)?) => { diff --git a/cli/src/services/hooks/opencode_mutation_scope/state.rs b/cli/src/services/hooks/opencode_mutation_scope/state.rs index 31db161ff..7fcc93dd1 100644 --- a/cli/src/services/hooks/opencode_mutation_scope/state.rs +++ b/cli/src/services/hooks/opencode_mutation_scope/state.rs @@ -506,9 +506,9 @@ mod state_conformance { const FIXTURE_SESSION: &str = "ses-conformance"; const FIXTURE_PHASES: [AttemptPhase; 3] = [ - AttemptPhase::PendingStart, AttemptPhase::Active, AttemptPhase::PendingAbandon, + AttemptPhase::PendingStart, ]; struct OpenCodeStateConformance; @@ -524,16 +524,12 @@ mod state_conformance { type State = AdapterState; const ADAPTER: &'static str = "opencode"; - const SUPPORTED_VERSION: u32 = 1; + const SUPPORTED_VERSION: u32 = ADAPTER_STATE_VERSION; fn state_path(git_dir: &Path) -> PathBuf { state_path(git_dir) } - fn default_state() -> AdapterState { - AdapterState::default() - } - fn fixture_state( recovery: RecoveryView, next_recovery_generation: u64, diff --git a/cli/src/services/hooks/pi_mutation_scope/state.rs b/cli/src/services/hooks/pi_mutation_scope/state.rs index bcf094683..923d0d891 100644 --- a/cli/src/services/hooks/pi_mutation_scope/state.rs +++ b/cli/src/services/hooks/pi_mutation_scope/state.rs @@ -460,9 +460,9 @@ mod state_conformance { const FIXTURE_SESSION: &str = "ses-conformance"; const FIXTURE_PHASES: [AttemptPhase; 3] = [ - AttemptPhase::PendingStart, AttemptPhase::Executed, AttemptPhase::PendingAbandon, + AttemptPhase::PendingStart, ]; struct PiStateConformance; @@ -478,16 +478,12 @@ mod state_conformance { type State = AdapterState; const ADAPTER: &'static str = "pi"; - const SUPPORTED_VERSION: u32 = 2; + const SUPPORTED_VERSION: u32 = ADAPTER_STATE_VERSION; fn state_path(git_dir: &Path) -> PathBuf { state_path(git_dir) } - fn default_state() -> AdapterState { - AdapterState::default() - } - fn fixture_state( recovery: RecoveryView, next_recovery_generation: u64,