diff --git a/crates/stella-cli/src/agent.rs b/crates/stella-cli/src/agent.rs index 28fba98fe..a7778ea3a 100644 --- a/crates/stella-cli/src/agent.rs +++ b/crates/stella-cli/src/agent.rs @@ -50,7 +50,7 @@ mod coverage; mod engine; mod goal; mod graph; -mod outcome; +pub(crate) mod outcome; mod output; mod persistence; mod presence; diff --git a/crates/stella-cli/src/agent/goal.rs b/crates/stella-cli/src/agent/goal.rs index ba9ff5974..ec8b66a01 100644 --- a/crates/stella-cli/src/agent/goal.rs +++ b/crates/stella-cli/src/agent/goal.rs @@ -208,7 +208,7 @@ pub async fn run_goal_cmd( goal: &str, budget_limit: Option, use_pipeline: bool, -) -> Result<(), String> { +) -> Result<(), crate::failure::CliFailure> { crate::enterprise_telemetry::authorize_execution_surface( crate::enterprise_telemetry::ExecutionSurface::Goal, )?; @@ -370,22 +370,19 @@ pub async fn run_goal_cmd( // Goal runs are long by construction — always land the inbox // notification (Enter on it replays this session's journal). let goal_secs = crate::memory::unix_now_secs().saturating_sub(started_unix); - let notify = if outcome.is_ok() { - format!("{}: goal met ({goal_secs}s)", presence.name()) - } else { - format!("{}: goal run FAILED", presence.name()) + let notify = match &outcome { + Ok(()) => format!("{}: goal met ({goal_secs}s)", presence.name()), + Err(failure) if failure.is_deliberate_stop() => { + format!("{}: goal run stopped by policy", presence.name()) + } + Err(_) => format!("{}: goal run FAILED", presence.name()), }; - // The goal loop still answers with a `String`, which has no room for the - // abort's typed kind (#1637's collapse one level deeper — #1862), so a - // policy-stopped goal round can only record `Error` here. Projected - // through `outcome_status` all the same, so a user interrupt records - // `Cancelled` rather than aging into a crash (#1826). - let terminal = outcome - .as_ref() - .map(|_| ()) - .map_err(|e| crate::failure::CliFailure::error(e.clone())); + // The typed abort survives to this terminal write (#1862): the same + // decider as every other registry writer projects a deliberate stop as + // `Stopped`, a user interrupt as `Cancelled`, and a crash as `Error` + // (#1653, #1826). presence.finish( - crate::daemon::outcome_status(terminal.as_ref().map(|_| ())), + crate::daemon::outcome_status(outcome.as_ref().map(|_| ())), Some((notify, crate::command_deck::prompt_line(goal, 160))), ); outcome @@ -425,7 +422,7 @@ pub(crate) async fn run_goal_turn( // before the turn runs — reflection stores the self-review 1:1 with an // execution, and an unstamped round files an id-less row. session_memory: Option<&mut crate::memory::SessionMemory>, -) -> Result<(), String> { +) -> Result<(), crate::failure::CliFailure> { let turn_start = Instant::now(); let execution = begin_execution(store, "goal", goal, cfg, session); if let (Some((_, id)), Some(m)) = (&execution, session_memory) { @@ -545,13 +542,16 @@ pub(crate) async fn run_goal_turn( rounds, reason, cost_usd, + kind, } => { tui::cost_summary( cost_usd, &format!("{}/{}", cfg.provider.id, cfg.model_id), turn_start.elapsed(), ); - Err(format!("goal not met after {rounds} round(s): {reason}")) + // The typed kind survives the loop (#1862): a working turn's + // deliberate stop exits `3` and records `Stopped`. + Err(outcome::goal_unmet_failure(rounds, &reason, kind)) } } } @@ -588,7 +588,7 @@ async fn run_goal_pipeline_turn( // Same contract as `run_goal_turn`: stamp the execution id into the // caller's memory before the turn runs, so reflection can name its row. session_memory: Option<&mut crate::memory::SessionMemory>, -) -> Result<(), String> { +) -> Result<(), crate::failure::CliFailure> { let turn_start = Instant::now(); let execution = begin_execution(store, "goal", goal, cfg, session); // Rebound mutable and NOT consumed by the id stamp, so the same memory @@ -717,7 +717,7 @@ async fn run_goal_pipeline_turn( .with_calibration(calibration); let mut total_cost_usd = 0.0f64; - let mut result: Option> = None; + let mut result: Option> = None; let mut goal_met = false; for round in 1..=goal_config.max_rounds { @@ -773,25 +773,17 @@ async fn run_goal_pipeline_turn( match pipeline.run(&round_goal, messages, budget).await { Ok(outcome) => { total_cost_usd += outcome.total_cost_usd; - match outcome.status { - PipelineStatus::Completed => {} - PipelineStatus::VerificationFailed { verdict } => { - result = Some(Err(format!( - "goal not met: verification failed: {}", - verdict.summary - ))); - break; - } - PipelineStatus::Aborted { reason, .. } => { - result = Some(Err(format!( - "goal not met: working round aborted: {reason}" - ))); - break; - } + // The fold keeps the abort's typed kind (#1862), so the + // terminal registry write can tell a policy stop from a + // crash. A completed round is no break at all — the loop + // goes on to its verifier assessment below. + if let Some(failure) = outcome::goal_round_break(&outcome.status) { + result = Some(Err(failure)); + break; } } Err(e) => { - result = Some(Err(e.to_string())); + result = Some(Err(crate::failure::CliFailure::error(e.to_string()))); break; } } @@ -807,7 +799,9 @@ async fn run_goal_pipeline_turn( { Ok(pair) => pair, Err(reason) => { - result = Some(Err(format!("goal not met: verifier unavailable: {reason}"))); + result = Some(Err(crate::failure::CliFailure::error(format!( + "goal not met: verifier unavailable: {reason}" + )))); break; } }; @@ -853,10 +847,10 @@ async fn run_goal_pipeline_turn( &format!("{}/{}", cfg.provider.id, cfg.model_id), turn_start.elapsed(), ); - Err(format!( + Err(crate::failure::CliFailure::error(format!( "goal not met after {} round(s): round cap reached without a passing verdict", goal_config.max_rounds - )) + ))) } } }; diff --git a/crates/stella-cli/src/agent/outcome.rs b/crates/stella-cli/src/agent/outcome.rs index 8b5af2550..a756fc254 100644 --- a/crates/stella-cli/src/agent/outcome.rs +++ b/crates/stella-cli/src/agent/outcome.rs @@ -66,7 +66,7 @@ pub(super) fn pipeline_episode_outcome(status: &PipelineStatus) -> EpisodeOutcom } } -pub(super) fn pipeline_status_result(status: &PipelineStatus) -> Result<(), CliFailure> { +pub(crate) fn pipeline_status_result(status: &PipelineStatus) -> Result<(), CliFailure> { match status { PipelineStatus::Completed => Ok(()), PipelineStatus::VerificationFailed { verdict } => Err(CliFailure::error(format!( @@ -114,7 +114,7 @@ pub(super) fn pipeline_session_status( /// could not tell a resumed policy stop from a resumed crash (#1637). /// /// [`AbortKind`]: stella_core::AbortKind -pub(super) fn turn_outcome_result(outcome: &TurnOutcome) -> Result<(), CliFailure> { +pub(crate) fn turn_outcome_result(outcome: &TurnOutcome) -> Result<(), CliFailure> { match outcome { TurnOutcome::Completed { .. } => Ok(()), TurnOutcome::Aborted { reason, kind, .. } => { @@ -123,6 +123,45 @@ pub(super) fn turn_outcome_result(outcome: &TurnOutcome) -> Result<(), CliFailur } } +/// The terminal break a goal round's pipeline status folds to — `None` keeps +/// the loop running to its verifier assessment. The goal-loop analogue of +/// [`pipeline_status_result`], with the loop's own message prefixes: the fold +/// used to stringify the status into `Err(String)`, which dropped the abort's +/// typed [`AbortKind`] right there, so the goal loop's terminal registry +/// write could only record a policy-stopped round as an error (#1862). +/// +/// [`AbortKind`]: stella_core::AbortKind +pub(crate) fn goal_round_break(status: &PipelineStatus) -> Option { + match status { + PipelineStatus::Completed => None, + PipelineStatus::VerificationFailed { verdict } => Some(CliFailure::error(format!( + "goal not met: verification failed: {}", + verdict.summary + ))), + PipelineStatus::Aborted { reason, kind } => Some(CliFailure::from_abort( + format!("goal not met: working round aborted: {reason}"), + *kind, + )), + } +} + +/// The failure an unmet goal loop reports at the process boundary — the raw +/// (`--no-pipeline`) goal loop's half of #1862. The abort's typed kind +/// survives when a working turn aborted; the backstops that are not turn +/// aborts (round cap, unreachable verifier) carry no kind and stay plain +/// failures, exactly as the untyped chain always read them. +pub(crate) fn goal_unmet_failure( + rounds: usize, + reason: &str, + kind: Option, +) -> CliFailure { + let message = format!("goal not met after {rounds} round(s): {reason}"); + match kind { + Some(kind) => CliFailure::from_abort(message, kind), + None => CliFailure::error(message), + } +} + #[cfg(test)] mod tests { use super::*; @@ -216,6 +255,67 @@ mod tests { ); } + /// #1862 witness, the goal loop's half of #1826's: the fold an aborted + /// working round takes on its way to the terminal registry write keeps + /// the abort's typed kind, so `run_goal_cmd`'s `presence.finish` + /// projects a policy-stopped goal round as `Stopped`. Before, the fold + /// stringified the status into `Err(String)` and the terminal write + /// could only reconstruct `CliFailure::error` — every stopped goal run + /// aged into the registry as a crash. + /// + /// Like `an_unsupervised_deliberate_stop_projects_stopped_not_error`, + /// this relies on no test in this binary ever calling + /// `signals::note_interrupt`, so `interrupted_exit_code()` is reliably + /// `None` here. + #[test] + fn a_policy_stopped_goal_round_projects_stopped_not_error() { + let stopped = goal_round_break(&PipelineStatus::Aborted { + reason: "stuck-loop detected (persisted after a steering warning)".into(), + kind: AbortKind::DeliberateStop, + }) + .expect("an aborted round ends the goal loop"); + let crashed = goal_round_break(&PipelineStatus::Aborted { + reason: "the model call would not commit after retries".into(), + kind: AbortKind::Failure, + }) + .expect("an aborted round ends the goal loop"); + + assert_eq!( + crate::daemon::outcome_status(Err(&stopped)), + stella_store::SessionStatus::Stopped, + "a goal round that ended itself by policy must not read as a crash" + ); + assert_eq!( + crate::daemon::outcome_status(Err(&crashed)), + stella_store::SessionStatus::Error + ); + // A completed round is not a break at all — the loop goes on to its + // verifier assessment. + assert!(goal_round_break(&PipelineStatus::Completed).is_none()); + } + + /// The raw (`--no-pipeline`) goal loop's half of the same witness: an + /// `Unmet` outcome whose working turn deliberately stopped projects + /// `Stopped`, while the kind-less backstops keep reading as failures. + #[test] + fn a_policy_stopped_raw_goal_loop_projects_stopped_not_error() { + let stopped = goal_unmet_failure( + 2, + "working turn aborted: session budget limit reached", + Some(AbortKind::DeliberateStop), + ); + let capped = goal_unmet_failure(8, "round cap (8) reached without a passing verdict", None); + + assert_eq!( + crate::daemon::outcome_status(Err(&stopped)), + stella_store::SessionStatus::Stopped + ); + assert_eq!( + crate::daemon::outcome_status(Err(&capped)), + stella_store::SessionStatus::Error + ); + } + #[test] fn a_deliberately_stopped_turn_carries_its_own_exit_code() { let failure = turn_outcome_result(&TurnOutcome::Aborted { diff --git a/crates/stella-cli/src/command_deck.rs b/crates/stella-cli/src/command_deck.rs index 3ab7a34aa..f2f64add3 100644 --- a/crates/stella-cli/src/command_deck.rs +++ b/crates/stella-cli/src/command_deck.rs @@ -225,7 +225,7 @@ fn debug_log_path() -> Option { /// How one dispatched turn ended, as seen by the driver loop. enum TurnEnd { /// The turn future resolved (completed or aborted-with-reason). - Finished(Result<(), String>), + Finished(Result<(), crate::failure::CliFailure>), /// The user stopped it mid-flight; the future was dropped. `hold` is the /// double-Esc variant: the interrupted prompt goes back to the FRONT of /// the backlog and dispatch parks until the user's next submission @@ -1593,7 +1593,7 @@ pub async fn run_deck_session( // The lead lane's pause seam — `p` on the lead row (#1219). let lead_pause = lead_control::LeadPause::new(); let end = { - // Both arms return `Result<(), String>`, so one pinned future + // Both arms return `Result<(), CliFailure>`, so one pinned future // drives either path through the same select loop. let turn = async { if pipeline_on { @@ -2053,7 +2053,7 @@ pub async fn run_deck_session( match end { TurnEnd::Finished(outcome) => { if let Err(reason) = &outcome { - if reason == stella_core::SOFT_STOP_REASON { + if reason.message() == stella_core::SOFT_STOP_REASON { // A user choice, not a failure: no Error row — the // work is kept and the next prompt continues from it. let _ = in_tx.send(Inbound::Event { @@ -2069,7 +2069,7 @@ pub async fn run_deck_session( let _ = in_tx.send(Inbound::Event { agent: LEAD.to_string(), event: AgentEvent::Error { - message: reason.clone(), + message: reason.to_string(), retryable: false, }, }); @@ -2122,11 +2122,9 @@ pub async fn run_deck_session( // ones someone comparing turns wants to see — so this is not // conditioned on the outcome. cfg.durability.mark_turn_end(); - session_exit = if outcome.is_err() { - stella_store::SessionStatus::Error - } else { - stella_store::SessionStatus::Complete - }; + // One decider for every terminal writer (#1653/#1826/#1862): + // a lead turn that ended in a deliberate stop exits `Stopped`. + session_exit = crate::daemon::outcome_status(outcome.as_ref().map(|_| ())); session_record.status = stella_store::SessionStatus::NeedsInput; let _ = session_registry.upsert(&session_record); let turn_secs = crate::memory::unix_now_secs().saturating_sub(started_unix); @@ -4230,7 +4228,7 @@ async fn run_lead_turn( // Phase 2 (#713): this turn's `ContextRecall`, carried from the caller // because recall runs before this channel exists. recall_event: Option, -) -> Result<(), String> { +) -> Result<(), crate::failure::CliFailure> { budget.begin_turn(); let (tx, rx) = mpsc::unbounded_channel::(); @@ -4350,10 +4348,9 @@ async fn run_lead_turn( } } - match outcome { - TurnOutcome::Completed { .. } => Ok(()), - TurnOutcome::Aborted { reason, .. } => Err(reason), - } + // The abort's typed kind rides through (#1862): the session-exit writer + // reads it off the same projection as every other terminal writer. + agent::outcome::turn_outcome_result(&outcome) } /// One staged-pipeline turn for the lead agent (`/pipeline` ON): the deck @@ -4398,7 +4395,7 @@ async fn run_lead_pipeline_turn( steering: &Arc, pause: &lead_control::LeadPause, mcp: Option>, -) -> Result<(), String> { +) -> Result<(), crate::failure::CliFailure> { budget.begin_turn(); let (tx, rx) = mpsc::unbounded_channel::(); @@ -4557,14 +4554,10 @@ async fn run_lead_pipeline_turn( } match result { - Ok(outcome) => match outcome.status { - PipelineStatus::Completed => Ok(()), - PipelineStatus::VerificationFailed { verdict } => { - Err(format!("verification failed: {}", verdict.summary)) - } - PipelineStatus::Aborted { reason, .. } => Err(reason), - }, - Err(e) => Err(e.to_string()), + // The shared projection keeps the abort's typed kind (#1862) and the + // exact messages the string arms carried before. + Ok(outcome) => agent::outcome::pipeline_status_result(&outcome.status), + Err(e) => Err(crate::failure::CliFailure::error(e.to_string())), } } diff --git a/crates/stella-cli/src/command_deck/authoring.rs b/crates/stella-cli/src/command_deck/authoring.rs index 1ae905d7f..256bf13ed 100644 --- a/crates/stella-cli/src/command_deck/authoring.rs +++ b/crates/stella-cli/src/command_deck/authoring.rs @@ -81,7 +81,7 @@ pub(super) fn forward_reflection_events( pub(super) async fn record_and_reflect_turn( memory: &mut Option, prompt: &str, - outcome: &Result<(), String>, + outcome: &Result<(), crate::failure::CliFailure>, registry: &ToolRegistry, files_before: usize, started_unix: i64, diff --git a/crates/stella-core/src/goal.rs b/crates/stella-core/src/goal.rs index dcde43084..4cdfbcb60 100644 --- a/crates/stella-core/src/goal.rs +++ b/crates/stella-core/src/goal.rs @@ -45,6 +45,7 @@ use tokio::sync::mpsc::UnboundedSender; use crate::budget::BudgetGuard; use crate::driver::{Engine, TurnOutcome}; +use crate::step::AbortKind; use crate::subagent::{SubAgentHost, SubAgentOutcome, SubAgentSpec, truncate_chars}; /// Tuning for [`Engine::run_goal`]. `Default` is sized for interactive @@ -89,6 +90,13 @@ pub enum GoalOutcome { rounds: usize, reason: String, cost_usd: f64, + /// The typed kind of the abort that ended the loop, carried from + /// [`TurnOutcome::Aborted`] when a working turn aborted — the bit a + /// terminal status writer needs to record a deliberate stop + /// distinctly from a crash (#1862). `None` for the backstops that + /// are not turn aborts (round cap, unreachable verifier), which + /// read as plain failures. + kind: Option, }, } @@ -201,11 +209,12 @@ impl Engine<'_> { self.with_turn_instance(self.config.turn_instance.saturating_add(round_offset)); match round_engine.run_turn(messages, budget, events).await { TurnOutcome::Completed { .. } => {} - TurnOutcome::Aborted { reason, .. } => { + TurnOutcome::Aborted { reason, kind, .. } => { return GoalOutcome::Unmet { rounds: round, reason: format!("working turn aborted: {reason}"), cost_usd: budget.session_spent_usd() - starting_cost_usd, + kind: Some(kind), }; } } @@ -227,6 +236,7 @@ impl Engine<'_> { rounds: round, reason: format!("verifier unavailable: {reason}"), cost_usd: budget.session_spent_usd() - starting_cost_usd, + kind: None, }; } }; @@ -262,6 +272,7 @@ impl Engine<'_> { goal_config.max_rounds ), cost_usd: budget.session_spent_usd() - starting_cost_usd, + kind: None, } } @@ -815,12 +826,16 @@ mod tests { rounds, reason, cost_usd, + kind, } => { // Stopped well before the round cap, on the budget backstop… assert!( rounds < config.max_rounds, "the loop ran to the round cap ({rounds}) instead of the budget" ); + // …whose typed kind survives the loop (#1862): an enforced + // budget is the engine choosing to stop, not falling over. + assert_eq!(kind, Some(AbortKind::DeliberateStop)); assert!( reason.contains("budget"), "the abort reason should cite the budget: {reason}" @@ -1047,8 +1062,10 @@ mod tests { .await; match outcome { - GoalOutcome::Unmet { reason, .. } => { + GoalOutcome::Unmet { reason, kind, .. } => { assert!(reason.contains("working turn aborted"), "{reason}"); + // A provider failure keeps its typed kind too (#1862). + assert_eq!(kind, Some(AbortKind::Failure)); // The verifier was never consulted about an aborted turn. assert_eq!(verifier.calls.load(Ordering::SeqCst), 0); }