From d7ced14a782bbd40280ed2c155ac1cb9407304b4 Mon Sep 17 00:00:00 2001 From: Val Alexander <68980965+BunsDev@users.noreply.github.com> Date: Tue, 13 Oct 2026 08:04:34 -0500 Subject: [PATCH] fix(goals): guard continuation against replacement and pause races Follow-up to fixes #175. Coven-Harness: Codex / GPT-6 --- src-rust/crates/core/Cargo.toml | 1 + src-rust/crates/core/src/goal.rs | 192 ++++++++- src-rust/crates/query/src/goal_loop.rs | 536 +++++++++++++++++++++---- 3 files changed, 634 insertions(+), 95 deletions(-) diff --git a/src-rust/crates/core/Cargo.toml b/src-rust/crates/core/Cargo.toml index 1df2b147..34d49951 100644 --- a/src-rust/crates/core/Cargo.toml +++ b/src-rust/crates/core/Cargo.toml @@ -105,3 +105,4 @@ cpal = { version = "0.15", optional = true } [dev-dependencies] tempfile = { workspace = true } +rusqlite = { workspace = true, features = ["trace"] } diff --git a/src-rust/crates/core/src/goal.rs b/src-rust/crates/core/src/goal.rs index 289fab63..daec82ef 100644 --- a/src-rust/crates/core/src/goal.rs +++ b/src-rust/crates/core/src/goal.rs @@ -680,26 +680,48 @@ impl GoalStore { /// Pause an active goal. Repeating a pause for a paused goal is idempotent. pub fn pause_active_goal(&self, session_id: &str) -> Result<(), GoalError> { - let updated = self - .conn + self.pause_active_goal_with_outcome(session_id).map(|_| ()) + } + + /// Pause an active goal and report whether this call changed its status. + /// + /// A `false` outcome means the goal was already paused. Other terminal + /// states retain the existing `NotActive` error behavior. + pub fn pause_active_goal_with_outcome(&self, session_id: &str) -> Result { + let transaction = self.immediate_transaction()?; + let goal = Self::transaction_goal(&transaction, session_id)?.ok_or_else(|| { + GoalError::NotFound { + session_id: session_id.to_string(), + } + })?; + if goal.status == GoalStatus::Paused { + return Ok(false); + } + if goal.status != GoalStatus::Active { + return Err(GoalError::NotActive { + session_id: session_id.to_string(), + }); + } + let updated = transaction .execute( "UPDATE goals SET status = 'paused', updated_at_ms = ?1 - WHERE session_id = ?2 AND status = 'active'", - rusqlite::params![Self::sqlite_i64(Self::now_ms(), "timestamp")?, session_id], + WHERE session_id = ?2 AND id = ?3 AND status = 'active'", + rusqlite::params![ + Self::sqlite_i64(Self::now_ms(), "timestamp")?, + session_id, + goal.id, + ], ) .map_err(|err| GoalError::Db(err.to_string()))?; - if updated > 0 { - return Ok(()); - } - match self.try_get_goal(session_id)? { - Some(goal) if goal.status == GoalStatus::Paused => Ok(()), - Some(_) => Err(GoalError::NotActive { - session_id: session_id.to_string(), - }), - None => Err(GoalError::NotFound { + if updated == 0 { + return Err(GoalError::NotActive { session_id: session_id.to_string(), - }), + }); } + transaction + .commit() + .map_err(|err| GoalError::Db(err.to_string()))?; + Ok(true) } /// Pause the goal only if `expected_goal_id` still owns the session row. @@ -708,6 +730,19 @@ impl GoalStore { session_id: &str, expected_goal_id: &str, ) -> Result<(), GoalError> { + self.pause_active_goal_for_goal_with_outcome(session_id, expected_goal_id) + .map(|_| ()) + } + + /// Pause the expected active goal and report whether this call changed it. + /// + /// A `false` outcome means the expected goal was already paused. A + /// replacement remains an error and is never modified. + pub fn pause_active_goal_for_goal_with_outcome( + &self, + session_id: &str, + expected_goal_id: &str, + ) -> Result { let transaction = self.immediate_transaction()?; let goal = Self::transaction_goal(&transaction, session_id)?.ok_or_else(|| { GoalError::NotFound { @@ -722,7 +757,7 @@ impl GoalStore { }); } if goal.status == GoalStatus::Paused { - return Ok(()); + return Ok(false); } if goal.status != GoalStatus::Active { return Err(GoalError::NotActive { @@ -748,7 +783,7 @@ impl GoalStore { transaction .commit() .map_err(|err| GoalError::Db(err.to_string()))?; - Ok(()) + Ok(true) } /// Resume a paused goal if it has not reached the automatic turn cap. @@ -1358,6 +1393,102 @@ mod tests { )); } + #[test] + fn strict_pause_reports_whether_it_transitioned_the_active_goal() { + let store = open_tmp(); + store.set_goal("session", "work", None).unwrap(); + + assert!(store.pause_active_goal_with_outcome("session").unwrap()); + assert!(!store.pause_active_goal_with_outcome("session").unwrap()); + + store.set_goal("complete", "finished", None).unwrap(); + store.complete_active_goal("complete").unwrap(); + assert!(matches!( + store.pause_active_goal_with_outcome("complete"), + Err(GoalError::NotActive { session_id }) if session_id == "complete" + )); + } + + #[test] + fn strict_pause_keeps_paused_classification_when_resume_races() { + #[derive(Clone, Copy, PartialEq, Eq)] + enum RacePhase { + Waiting, + PauseUpdateCompleted, + PausedRowRead, + ResumeAttempted, + ResumeCompleted, + } + + static RACE_STATE: OnceLock<(Mutex, Condvar)> = OnceLock::new(); + + fn race_state() -> &'static (Mutex, Condvar) { + RACE_STATE.get_or_init(|| (Mutex::new(RacePhase::Waiting), Condvar::new())) + } + + fn pause_race_profile(sql: &str, _: Duration) { + let (lock, condvar) = race_state(); + let mut phase = lock.lock().unwrap(); + if sql.starts_with("UPDATE goals SET status = 'paused'") { + *phase = RacePhase::PauseUpdateCompleted; + condvar.notify_all(); + while *phase != RacePhase::ResumeCompleted { + phase = condvar.wait(phase).unwrap(); + } + } else if sql.contains("FROM goals WHERE session_id") && *phase == RacePhase::Waiting { + *phase = RacePhase::PausedRowRead; + condvar.notify_all(); + while *phase != RacePhase::ResumeAttempted { + phase = condvar.wait(phase).unwrap(); + } + } + } + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("goals.sqlite"); + let mut first = GoalStore::open(&path).unwrap(); + first.set_goal("session", "work", None).unwrap(); + first.pause_active_goal("session").unwrap(); + let second = GoalStore::open(&path).unwrap(); + let (lock, _) = race_state(); + *lock.lock().unwrap() = RacePhase::Waiting; + first.conn.profile(Some(pause_race_profile)); + + let resumer = thread::spawn(move || { + let (lock, condvar) = race_state(); + let mut phase = lock.lock().unwrap(); + while *phase == RacePhase::Waiting { + phase = condvar.wait(phase).unwrap(); + } + match *phase { + RacePhase::PauseUpdateCompleted => { + drop(phase); + second.resume_paused_goal("session").unwrap(); + let mut phase = lock.lock().unwrap(); + *phase = RacePhase::ResumeCompleted; + condvar.notify_all(); + } + RacePhase::PausedRowRead => { + *phase = RacePhase::ResumeAttempted; + condvar.notify_all(); + drop(phase); + second.resume_paused_goal("session").unwrap(); + } + _ => unreachable!("unexpected pause race phase"), + } + }); + + let outcome = first.pause_active_goal("session"); + first.conn.profile(None); + resumer.join().unwrap(); + + assert_eq!(outcome, Ok(())); + assert_eq!( + first.try_get_goal("session").unwrap().unwrap().status, + GoalStatus::Active + ); + } + #[test] fn guarded_transitions_reject_missing_and_non_active_goals() { let store = open_tmp(); @@ -1406,6 +1537,35 @@ mod tests { assert_eq!(current.turns_used, 0); } + #[test] + fn expected_goal_strict_pause_reports_outcome_and_preserves_replacement() { + let store = open_tmp(); + let original = store.set_goal("session", "original", None).unwrap(); + + assert!(store + .pause_active_goal_for_goal_with_outcome("session", &original.id) + .unwrap()); + assert!(!store + .pause_active_goal_for_goal_with_outcome("session", &original.id) + .unwrap()); + + let replacement = store.set_goal("session", "replacement", None).unwrap(); + let error = store + .pause_active_goal_for_goal_with_outcome("session", &original.id) + .unwrap_err(); + assert!(matches!( + error, + GoalError::Replaced { + expected_goal_id, + actual_goal_id, + .. + } if expected_goal_id == original.id && actual_goal_id == replacement.id + )); + let current = store.try_get_goal("session").unwrap().unwrap(); + assert_eq!(current.id, replacement.id); + assert_eq!(current.status, GoalStatus::Active); + } + #[test] fn expected_goal_budget_limit_rejects_replacement_without_mutating_it() { let store = open_tmp(); diff --git a/src-rust/crates/query/src/goal_loop.rs b/src-rust/crates/query/src/goal_loop.rs index 27b9c6ac..1f234d16 100644 --- a/src-rust/crates/query/src/goal_loop.rs +++ b/src-rust/crates/query/src/goal_loop.rs @@ -16,6 +16,19 @@ use claurst_core::{ goal_continuation_message, Goal, GoalError, GoalStatus, GoalStore, MAX_GOAL_TURNS, }; +#[cfg(test)] +use std::cell::RefCell; + +#[cfg(test)] +thread_local! { + static BEFORE_COMPLETED_TURN_RECORD: RefCell>> = + const { RefCell::new(None) }; + static BEFORE_AUTOMATIC_TERMINAL_TRANSITION: RefCell>> = + const { RefCell::new(None) }; + static BEFORE_BUDGET_LIMIT_TRANSITION: RefCell>> = + const { RefCell::new(None) }; +} + /// Result returned to the caller after a completed query loop turn. #[derive(Debug)] pub enum GoalContinuation { @@ -110,16 +123,35 @@ fn stop_for_terminal_status(goal: &Goal) -> Option { Some(GoalContinuation::Stop { reason }) } -fn reload_after_racing_transition( +fn current_goal( store: &GoalStore, session_id: &str, - transition_error: GoalError, -) -> Result { + expected_goal_id: Option<&str>, +) -> Result { let goal = store .try_get_goal(session_id)? .ok_or_else(|| GoalError::NotFound { session_id: session_id.to_string(), })?; + if let Some(expected_goal_id) = expected_goal_id { + if goal.id != expected_goal_id { + return Err(GoalError::Replaced { + session_id: session_id.to_string(), + expected_goal_id: expected_goal_id.to_string(), + actual_goal_id: goal.id, + }); + } + } + Ok(goal) +} + +fn reload_after_racing_transition( + store: &GoalStore, + session_id: &str, + expected_goal_id: Option<&str>, + transition_error: GoalError, +) -> Result { + let goal = current_goal(store, session_id, expected_goal_id)?; stop_for_terminal_status(&goal).ok_or(transition_error) } @@ -129,13 +161,14 @@ fn check_and_continue_goal_in_store( total_tokens_used: u64, turn_elapsed_secs: u64, ) -> Result { - match store.record_completed_turn(session_id, total_tokens_used, turn_elapsed_secs) { - Ok(()) => check_goal_guards(store, session_id, None), - Err(error @ GoalError::NotActive { .. }) => { - reload_after_racing_transition(store, session_id, error) - } - Err(error) => Err(error), - } + let expected_goal_id = current_goal(store, session_id, None)?.id; + check_and_continue_goal_after_turn( + store, + session_id, + Some(&expected_goal_id), + total_tokens_used, + turn_elapsed_secs, + ) } fn check_and_continue_goal_in_store_for_goal( @@ -145,15 +178,47 @@ fn check_and_continue_goal_in_store_for_goal( total_tokens_used: u64, turn_elapsed_secs: u64, ) -> Result { - match store.record_completed_turn_for_goal( + check_and_continue_goal_after_turn( + store, session_id, - expected_goal_id, + Some(expected_goal_id), total_tokens_used, turn_elapsed_secs, - ) { - Ok(()) => check_goal_guards(store, session_id, Some(expected_goal_id)), + ) +} + +fn check_and_continue_goal_after_turn( + store: &GoalStore, + session_id: &str, + expected_goal_id: Option<&str>, + total_tokens_used: u64, + turn_elapsed_secs: u64, +) -> Result { + let goal = current_goal(store, session_id, expected_goal_id)?; + if let Some(decision) = stop_for_terminal_status(&goal) { + return Ok(decision); + } + + #[cfg(test)] + BEFORE_COMPLETED_TURN_RECORD.with(|hook| { + if let Some(hook) = hook.borrow_mut().take() { + hook(); + } + }); + + let record = match expected_goal_id { + Some(expected_goal_id) => store.record_completed_turn_for_goal( + session_id, + expected_goal_id, + total_tokens_used, + turn_elapsed_secs, + ), + None => store.record_completed_turn(session_id, total_tokens_used, turn_elapsed_secs), + }; + match record { + Ok(()) => check_goal_guards(store, session_id, expected_goal_id), Err(error @ GoalError::NotActive { .. }) => { - reload_after_racing_transition(store, session_id, error) + reload_after_racing_transition(store, session_id, expected_goal_id, error) } Err(error) => Err(error), } @@ -164,39 +229,46 @@ fn check_goal_guards( session_id: &str, expected_goal_id: Option<&str>, ) -> Result { - let goal = store - .try_get_goal(session_id)? - .ok_or_else(|| GoalError::NotFound { - session_id: session_id.to_string(), - })?; - if let Some(expected_goal_id) = expected_goal_id { - if goal.id != expected_goal_id { - return Err(GoalError::Replaced { - session_id: session_id.to_string(), - expected_goal_id: expected_goal_id.to_string(), - actual_goal_id: goal.id, - }); - } - } + let goal = current_goal(store, session_id, expected_goal_id)?; if let Some(decision) = stop_for_terminal_status(&goal) { return Ok(decision); } if goal.turns_used >= MAX_GOAL_TURNS { + #[cfg(test)] + BEFORE_AUTOMATIC_TERMINAL_TRANSITION.with(|hook| { + if let Some(hook) = hook.borrow_mut().take() { + hook(); + } + }); + let transition = match expected_goal_id { Some(expected_goal_id) => { - store.pause_active_goal_for_goal(session_id, expected_goal_id) + store.pause_active_goal_for_goal_with_outcome(session_id, expected_goal_id) } - None => store.pause_active_goal(session_id), + None => store.pause_active_goal_with_outcome(session_id), }; - if let Err(error) = transition { - return match error { - error @ GoalError::NotActive { .. } => { - reload_after_racing_transition(store, session_id, error) - } - other => Err(other), - }; + match transition { + Ok(true) => {} + Ok(false) => { + return reload_after_racing_transition( + store, + session_id, + expected_goal_id, + GoalError::NotActive { + session_id: session_id.to_string(), + }, + ); + } + Err(error) => { + return match error { + error @ GoalError::NotActive { .. } => { + reload_after_racing_transition(store, session_id, expected_goal_id, error) + } + other => Err(other), + }; + } } return Ok(GoalContinuation::Stop { reason: StopReason::RunawayGuard { @@ -206,6 +278,13 @@ fn check_goal_guards( } if goal.is_over_budget(goal.tokens_used) { + #[cfg(test)] + BEFORE_BUDGET_LIMIT_TRANSITION.with(|hook| { + if let Some(hook) = hook.borrow_mut().take() { + hook(); + } + }); + let transition = match expected_goal_id { Some(expected_goal_id) => { store.budget_limit_active_goal_for_goal(session_id, expected_goal_id) @@ -215,7 +294,7 @@ fn check_goal_guards( if let Err(error) = transition { return match error { error @ GoalError::NotActive { .. } => { - reload_after_racing_transition(store, session_id, error) + reload_after_racing_transition(store, session_id, expected_goal_id, error) } other => Err(other), }; @@ -267,15 +346,128 @@ pub fn mark_goal_complete(session_id: &str) -> Result<(), String> { #[cfg(test)] mod tests { - use std::path::PathBuf; + use std::{path::PathBuf, sync::Mutex}; use super::*; use claurst_core::{GoalError, GoalStore}; + static DEFAULT_GOAL_STORE_ENV_LOCK: Mutex<()> = Mutex::new(()); + fn goal_path(dir: &tempfile::TempDir) -> PathBuf { dir.path().join("goals.sqlite") } + fn assert_terminal_stop(decision: GoalContinuation, status: GoalStatus) { + match (decision, status) { + ( + GoalContinuation::Stop { + reason: StopReason::GoalComplete, + }, + GoalStatus::Complete, + ) + | ( + GoalContinuation::Stop { + reason: StopReason::Paused, + }, + GoalStatus::Paused, + ) + | ( + GoalContinuation::Stop { + reason: StopReason::BudgetLimited, + }, + GoalStatus::BudgetLimited, + ) => {} + (decision, status) => { + panic!("expected terminal stop for {status:?}, got {decision:?}"); + } + } + } + + fn assert_progress_unchanged(store: &GoalStore, session_id: &str) { + let goal = store.try_get_goal(session_id).unwrap().unwrap(); + assert_eq!(goal.tokens_used, 42); + assert_eq!(goal.time_used_secs, 5); + assert_eq!(goal.turns_used, 1); + } + + fn set_terminal_goal(store: &GoalStore, session_id: &str, status: GoalStatus) -> String { + let goal = store.set_goal(session_id, "finish", None).unwrap(); + store.record_completed_turn(session_id, 42, 5).unwrap(); + store.set_status(session_id, status).unwrap(); + goal.id + } + + fn setup_runaway_goal(store: &GoalStore, session_id: &str) -> String { + let goal = store.set_goal(session_id, "finish", None).unwrap(); + for turn in 1..MAX_GOAL_TURNS { + store + .record_completed_turn(session_id, u64::from(turn), 1) + .unwrap(); + } + goal.id + } + + fn pause_before_automatic_terminal_transition(path: &Path, session_id: &str) { + let path = path.to_path_buf(); + let session_id = session_id.to_string(); + BEFORE_AUTOMATIC_TERMINAL_TRANSITION.with(|hook| { + *hook.borrow_mut() = Some(Box::new(move || { + GoalStore::open(&path) + .unwrap() + .pause_active_goal(&session_id) + .unwrap(); + })); + }); + } + + fn replace_before_automatic_terminal_transition(path: &Path, session_id: &str) { + let path = path.to_path_buf(); + let session_id = session_id.to_string(); + BEFORE_AUTOMATIC_TERMINAL_TRANSITION.with(|hook| { + *hook.borrow_mut() = Some(Box::new(move || { + GoalStore::open(&path) + .unwrap() + .set_goal(&session_id, "replacement objective", None) + .unwrap(); + })); + }); + } + + fn replace_before_completed_turn_record(path: &Path, session_id: &str) { + let path = path.to_path_buf(); + let session_id = session_id.to_string(); + BEFORE_COMPLETED_TURN_RECORD.with(|hook| { + *hook.borrow_mut() = Some(Box::new(move || { + GoalStore::open(&path) + .unwrap() + .set_goal(&session_id, "replacement objective", None) + .unwrap(); + })); + }); + } + + fn replace_before_budget_limit_transition(path: &Path, session_id: &str) { + let path = path.to_path_buf(); + let session_id = session_id.to_string(); + BEFORE_BUDGET_LIMIT_TRANSITION.with(|hook| { + *hook.borrow_mut() = Some(Box::new(move || { + GoalStore::open(&path) + .unwrap() + .set_goal(&session_id, "replacement objective", None) + .unwrap(); + })); + }); + } + + fn assert_replacement_is_active(store: &GoalStore, session_id: &str) { + let replacement = store.try_get_goal(session_id).unwrap().unwrap(); + assert_eq!(replacement.objective, "replacement objective"); + assert_eq!(replacement.status, GoalStatus::Active); + assert_eq!(replacement.tokens_used, 0); + assert_eq!(replacement.time_used_secs, 0); + assert_eq!(replacement.turns_used, 0); + } + #[test] fn explicit_path_records_progress_before_budget_stop() { let dir = tempfile::tempdir().unwrap(); @@ -325,6 +517,186 @@ mod tests { )); } + #[test] + fn default_wrapper_reports_concurrent_pause_at_runaway_boundary() { + let _env_lock = DEFAULT_GOAL_STORE_ENV_LOCK.lock().unwrap(); + let dir = tempfile::tempdir().unwrap(); + let previous_home = std::env::var_os("COVEN_CODE_HOME"); + std::env::set_var("COVEN_CODE_HOME", dir.path()); + + let path = GoalStore::default_path(); + let store = GoalStore::open(&path).unwrap(); + setup_runaway_goal(&store, "session"); + pause_before_automatic_terminal_transition(&path, "session"); + + let decision = check_and_continue_goal("session", 999, 1); + assert_terminal_stop(decision, GoalStatus::Paused); + + match previous_home { + Some(home) => std::env::set_var("COVEN_CODE_HOME", home), + None => std::env::remove_var("COVEN_CODE_HOME"), + } + } + + #[test] + fn explicit_path_wrapper_reports_concurrent_pause_at_runaway_boundary() { + let dir = tempfile::tempdir().unwrap(); + let path = goal_path(&dir); + let store = GoalStore::open(&path).unwrap(); + setup_runaway_goal(&store, "session"); + pause_before_automatic_terminal_transition(&path, "session"); + + let decision = check_and_continue_goal_at_path(&path, "session", 999, 1).unwrap(); + assert_terminal_stop(decision, GoalStatus::Paused); + } + + #[test] + fn expected_id_wrapper_reports_concurrent_pause_at_runaway_boundary() { + let dir = tempfile::tempdir().unwrap(); + let path = goal_path(&dir); + let store = GoalStore::open(&path).unwrap(); + let goal_id = setup_runaway_goal(&store, "session"); + pause_before_automatic_terminal_transition(&path, "session"); + + let decision = + check_and_continue_goal_at_path_for_goal(&path, "session", &goal_id, 999, 1).unwrap(); + assert_terminal_stop(decision, GoalStatus::Paused); + } + + #[test] + fn default_wrapper_does_not_pause_a_replacement_at_runaway_boundary() { + let _env_lock = DEFAULT_GOAL_STORE_ENV_LOCK.lock().unwrap(); + let dir = tempfile::tempdir().unwrap(); + let previous_home = std::env::var_os("COVEN_CODE_HOME"); + std::env::set_var("COVEN_CODE_HOME", dir.path()); + + let path = GoalStore::default_path(); + let store = GoalStore::open(&path).unwrap(); + setup_runaway_goal(&store, "session"); + replace_before_automatic_terminal_transition(&path, "session"); + + let decision = check_and_continue_goal("session", 999, 1); + + match previous_home { + Some(home) => std::env::set_var("COVEN_CODE_HOME", home), + None => std::env::remove_var("COVEN_CODE_HOME"), + } + + assert!(matches!( + decision, + GoalContinuation::Stop { + reason: StopReason::Error(message), + } if message.contains("replaced") + )); + assert_replacement_is_active(&GoalStore::open(&path).unwrap(), "session"); + } + + #[test] + fn explicit_path_wrapper_does_not_pause_a_replacement_at_runaway_boundary() { + let dir = tempfile::tempdir().unwrap(); + let path = goal_path(&dir); + let store = GoalStore::open(&path).unwrap(); + setup_runaway_goal(&store, "session"); + replace_before_automatic_terminal_transition(&path, "session"); + + let error = check_and_continue_goal_at_path(&path, "session", 999, 1).unwrap_err(); + + assert!(matches!(error, GoalError::Replaced { .. })); + assert_replacement_is_active(&GoalStore::open(&path).unwrap(), "session"); + } + + #[test] + fn default_wrapper_does_not_record_progress_on_a_replacement() { + let _env_lock = DEFAULT_GOAL_STORE_ENV_LOCK.lock().unwrap(); + let dir = tempfile::tempdir().unwrap(); + let previous_home = std::env::var_os("COVEN_CODE_HOME"); + std::env::set_var("COVEN_CODE_HOME", dir.path()); + + let path = GoalStore::default_path(); + let store = GoalStore::open(&path).unwrap(); + store + .set_goal("session", "original objective", None) + .unwrap(); + replace_before_completed_turn_record(&path, "session"); + + let decision = check_and_continue_goal("session", 123, 7); + + match previous_home { + Some(home) => std::env::set_var("COVEN_CODE_HOME", home), + None => std::env::remove_var("COVEN_CODE_HOME"), + } + + assert!(matches!( + decision, + GoalContinuation::Stop { + reason: StopReason::Error(message), + } if message.contains("replaced") + )); + assert_replacement_is_active(&GoalStore::open(&path).unwrap(), "session"); + } + + #[test] + fn explicit_path_wrapper_does_not_record_progress_on_a_replacement() { + let dir = tempfile::tempdir().unwrap(); + let path = goal_path(&dir); + let store = GoalStore::open(&path).unwrap(); + store + .set_goal("session", "original objective", None) + .unwrap(); + replace_before_completed_turn_record(&path, "session"); + + let error = check_and_continue_goal_at_path(&path, "session", 123, 7).unwrap_err(); + + assert!(matches!(error, GoalError::Replaced { .. })); + assert_replacement_is_active(&GoalStore::open(&path).unwrap(), "session"); + } + + #[test] + fn default_wrapper_does_not_budget_limit_a_replacement() { + let _env_lock = DEFAULT_GOAL_STORE_ENV_LOCK.lock().unwrap(); + let dir = tempfile::tempdir().unwrap(); + let previous_home = std::env::var_os("COVEN_CODE_HOME"); + std::env::set_var("COVEN_CODE_HOME", dir.path()); + + let path = GoalStore::default_path(); + let store = GoalStore::open(&path).unwrap(); + store + .set_goal("session", "original objective", Some(100)) + .unwrap(); + replace_before_budget_limit_transition(&path, "session"); + + let decision = check_and_continue_goal("session", 100, 7); + + match previous_home { + Some(home) => std::env::set_var("COVEN_CODE_HOME", home), + None => std::env::remove_var("COVEN_CODE_HOME"), + } + + assert!(matches!( + decision, + GoalContinuation::Stop { + reason: StopReason::Error(message), + } if message.contains("replaced") + )); + assert_replacement_is_active(&GoalStore::open(&path).unwrap(), "session"); + } + + #[test] + fn explicit_path_wrapper_does_not_budget_limit_a_replacement() { + let dir = tempfile::tempdir().unwrap(); + let path = goal_path(&dir); + let store = GoalStore::open(&path).unwrap(); + store + .set_goal("session", "original objective", Some(100)) + .unwrap(); + replace_before_budget_limit_transition(&path, "session"); + + let error = check_and_continue_goal_at_path(&path, "session", 100, 7).unwrap_err(); + + assert!(matches!(error, GoalError::Replaced { .. })); + assert_replacement_is_active(&GoalStore::open(&path).unwrap(), "session"); + } + #[test] fn explicit_path_reports_missing_goal() { let dir = tempfile::tempdir().unwrap(); @@ -338,48 +710,54 @@ mod tests { let path = goal_path(&dir); let store = GoalStore::open(&path).unwrap(); - for (session_id, status, expected_reason) in [ - ("complete", GoalStatus::Complete, StopReason::GoalComplete), - ("paused", GoalStatus::Paused, StopReason::Paused), - ( - "budget-limited", - GoalStatus::BudgetLimited, - StopReason::BudgetLimited, - ), + for (session_id, status) in [ + ("complete", GoalStatus::Complete), + ("paused", GoalStatus::Paused), + ("budget-limited", GoalStatus::BudgetLimited), ] { - store.set_goal(session_id, "finish", None).unwrap(); - match status { - GoalStatus::Complete => store.complete_active_goal(session_id).unwrap(), - GoalStatus::Paused => store.pause_active_goal(session_id).unwrap(), - GoalStatus::BudgetLimited => store.budget_limit_active_goal(session_id).unwrap(), - GoalStatus::Active => unreachable!("only terminal states are under test"), - } + let goal_id = set_terminal_goal(&store, session_id, status.clone()); let decision = check_and_continue_goal_at_path(&path, session_id, 999, 8).unwrap(); - assert!(matches!( - (decision, expected_reason), - ( - GoalContinuation::Stop { - reason: StopReason::GoalComplete - }, - StopReason::GoalComplete - ) | ( - GoalContinuation::Stop { - reason: StopReason::Paused - }, - StopReason::Paused - ) | ( - GoalContinuation::Stop { - reason: StopReason::BudgetLimited - }, - StopReason::BudgetLimited - ) - )); - - let goal = store.try_get_goal(session_id).unwrap().unwrap(); - assert_eq!(goal.tokens_used, 0); - assert_eq!(goal.time_used_secs, 0); - assert_eq!(goal.turns_used, 0); + assert_terminal_stop(decision, status.clone()); + assert_progress_unchanged(&store, session_id); + + let decision = + check_and_continue_goal_at_path_for_goal(&path, session_id, &goal_id, 999, 8) + .unwrap(); + assert_terminal_stop(decision, status); + assert_progress_unchanged(&store, session_id); + } + } + + #[test] + fn default_wrappers_terminal_goals_stop_without_recording_progress() { + let _env_lock = DEFAULT_GOAL_STORE_ENV_LOCK.lock().unwrap(); + let dir = tempfile::tempdir().unwrap(); + let previous_home = std::env::var_os("COVEN_CODE_HOME"); + std::env::set_var("COVEN_CODE_HOME", dir.path()); + + let path = GoalStore::default_path(); + let store = GoalStore::open(&path).unwrap(); + + for (session_id, status) in [ + ("complete", GoalStatus::Complete), + ("paused", GoalStatus::Paused), + ("budget-limited", GoalStatus::BudgetLimited), + ] { + let goal_id = set_terminal_goal(&store, session_id, status.clone()); + + let decision = check_and_continue_goal(session_id, 999, 8); + assert_terminal_stop(decision, status.clone()); + assert_progress_unchanged(&store, session_id); + + let decision = check_and_continue_goal_for_goal(session_id, &goal_id, 999, 8); + assert_terminal_stop(decision, status); + assert_progress_unchanged(&store, session_id); + } + + match previous_home { + Some(home) => std::env::set_var("COVEN_CODE_HOME", home), + None => std::env::remove_var("COVEN_CODE_HOME"), } }