From fadfac14ad8e7bbcfaa0c511d20bb49db35142c2 Mon Sep 17 00:00:00 2001 From: Stella Test Date: Thu, 6 Aug 2026 04:00:43 -0700 Subject: [PATCH] fix(stella-cli): record a deliberate stop distinctly on the unsupervised writers too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #1828 (#1653) taught the supervised registry writers to record SessionStatus::Stopped for a policy stop, but SessionPresence::finish and its callers still collapsed the outcome to a bool on the way in — so an UNSUPERVISED headless run (a pipe, CI, --foreground) that ended itself by policy still aged into the SESSIONS overlay as Error, indistinguishable from a crash, with no later supervised write to correct it. Widen SessionPresence::finish (and one_shot_notification's wording) past the bool: the caller now hands it the terminal SessionStatus, projected by the one existing decider (daemon::outcome_status) via a fifth projection in agent/outcome.rs, pipeline_session_status, so every writer reads a deliberate stop (Stopped), an interrupt (Cancelled), and a crash (Error) the same way. The goal loop still answers with a String that cannot carry the abort kind, and the deck's session_exit reads the same stringly run_lead_turn — both audited and filed as #1862. Witness: outcome::tests::an_unsupervised_deliberate_stop_projects_stopped_not_error (structural — the projection did not exist and the widened finish call sites do not compile on the old signature, mirroring the #1653 witness). Closes #1826 Refs #1653 Refs #1862 --- crates/stella-cli/src/agent.rs | 12 ++-- crates/stella-cli/src/agent/goal.rs | 18 ++++- crates/stella-cli/src/agent/outcome.rs | 96 +++++++++++++++++++++++-- crates/stella-cli/src/agent/presence.rs | 47 ++++++++---- crates/stella-cli/src/daemon.rs | 13 ++-- 5 files changed, 152 insertions(+), 34 deletions(-) diff --git a/crates/stella-cli/src/agent.rs b/crates/stella-cli/src/agent.rs index a507c72a9..28fba98fe 100644 --- a/crates/stella-cli/src/agent.rs +++ b/crates/stella-cli/src/agent.rs @@ -65,8 +65,8 @@ pub(crate) use graph::spawn_session_graph; #[cfg(test)] use graph::{GraphSummary, format_graph_stats, index_workspace_graph_blocking}; use outcome::{ - pipeline_episode_outcome, pipeline_failure_reason, pipeline_status_label, - pipeline_status_result, + pipeline_episode_outcome, pipeline_failure_reason, pipeline_session_status, + pipeline_status_label, pipeline_status_result, }; pub(crate) use outcome::{pipeline_execution_closeout, settled_cost_since}; use output::*; @@ -605,10 +605,10 @@ async fn run_pipeline_one_shot( // always lands a notification; a successful one only when it ran long // enough that the user has plausibly looked away. `Enter` on the // notification (or the SESSIONS overlay) replays the journal. - let run_ok = matches!(&result, Ok(o) if matches!(o.status, PipelineStatus::Completed)); + let session_status = pipeline_session_status(&result); let run_secs = turn_start.elapsed().as_secs(); - let notify = presence.one_shot_notification(run_ok, run_secs, prompt); - presence.finish(run_ok, notify); + let notify = presence.one_shot_notification(session_status, run_secs, prompt); + presence.finish(session_status, notify); match &result { Ok(outcome) => { @@ -1122,7 +1122,7 @@ pub async fn run_interactive(cfg: &Config, budget_limit: Option) -> Result< if let Some(set) = &mcp { set.close_all().await; } - presence.finish(true, None); + presence.finish(stella_store::SessionStatus::Complete, None); println!("\n {}", "Goodbye! ✦".magenta()); Ok(()) } diff --git a/crates/stella-cli/src/agent/goal.rs b/crates/stella-cli/src/agent/goal.rs index 1061f574a..ba9ff5974 100644 --- a/crates/stella-cli/src/agent/goal.rs +++ b/crates/stella-cli/src/agent/goal.rs @@ -182,8 +182,11 @@ pub(crate) async fn run_raw_one_shot( // `SessionPresence::one_shot_notification`, shared with the pipeline path). let run_secs = u64::try_from(crate::memory::unix_now_secs().saturating_sub(started_unix)).unwrap_or(0); - let notify = presence.one_shot_notification(outcome.is_ok(), run_secs, prompt); - presence.finish(outcome.is_ok(), notify); + // The failure itself, not a bool: an unsupervised deliberate stop must + // reach the registry as `Stopped`, never age into it as a crash (#1826). + let status = crate::daemon::outcome_status(outcome.as_ref().map(|_| ())); + let notify = presence.one_shot_notification(status, run_secs, prompt); + presence.finish(status, notify); outcome } @@ -372,8 +375,17 @@ pub async fn run_goal_cmd( } else { 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())); presence.finish( - outcome.is_ok(), + crate::daemon::outcome_status(terminal.as_ref().map(|_| ())), Some((notify, crate::command_deck::prompt_line(goal, 160))), ); outcome diff --git a/crates/stella-cli/src/agent/outcome.rs b/crates/stella-cli/src/agent/outcome.rs index 65d8b6610..8b5af2550 100644 --- a/crates/stella-cli/src/agent/outcome.rs +++ b/crates/stella-cli/src/agent/outcome.rs @@ -1,9 +1,9 @@ //! One reading of a finished pipeline run, shared by every surface. //! -//! A [`PipelineStatus`] has to be projected four different ways — a store -//! label, a JSON `reason`, an episodic-memory outcome, and the process exit -//! `Result` — and each surface (one-shot, deck, fleet, arena) needs the same -//! projections. Keeping them here, as total `match`es over the enum, is what +//! A [`PipelineStatus`] has to be projected five different ways — a store +//! label, a JSON `reason`, an episodic-memory outcome, the process exit +//! `Result`, and the terminal SESSIONS-registry status — and each surface +//! (one-shot, deck, fleet, arena) needs the same projections. Keeping them here, as total `match`es over the enum, is what //! stops `stella run --output-format json` and the audit row from disagreeing //! about whether the same run passed. //! @@ -79,6 +79,27 @@ pub(super) fn pipeline_status_result(status: &PipelineStatus) -> Result<(), CliF } } +/// The terminal SESSIONS-registry status a finished pipeline run records — +/// the fifth projection, feeding `SessionPresence::finish` on the +/// unsupervised paths (`stella run` in a pipe, CI, `--foreground`) that +/// `crate::daemon::record_outcome_if_supervised` never reaches (#1826). +/// +/// Routed through [`crate::daemon::outcome_status`] rather than re-matched +/// here, so every registry writer — supervised or not — reads a deliberate +/// stop (`Stopped`), a user interrupt (`Cancelled`), and a crash (`Error`) +/// off the same decider (#1653). +pub(super) fn pipeline_session_status( + result: &Result, +) -> stella_store::SessionStatus { + let terminal = match result { + Ok(outcome) => pipeline_status_result(&outcome.status), + // A hard pipeline error carries no abort kind — a genuine failure, + // the same reading the process exit gives it. + Err(error) => Err(CliFailure::error(error.to_string())), + }; + crate::daemon::outcome_status(terminal.as_ref().map(|_| ())) +} + /// The process-boundary answer a finished engine turn owes `main`. /// /// The one projection from [`TurnOutcome`] to an exit code, so every surface @@ -128,6 +149,73 @@ mod tests { assert_eq!(settled_cost_since(1.25, 1.25), 0.0); } + /// A pipeline run terminal enough to project, with every field the + /// projection ignores held constant. + fn pipeline_outcome(status: PipelineStatus) -> PipelineOutcome { + PipelineOutcome { + status, + task_class: stella_pipeline::TaskClass::SingleTask, + final_text: String::new(), + total_cost_usd: 0.0, + verdict: None, + score: None, + revisions: 0, + candidates_run: 1, + } + } + + /// #1826 witness — the presence half of #1653: the projection the + /// unsupervised registry writers feed `SessionPresence::finish` + /// distinguishes a deliberate stop from a crash. Before, `finish(ok: + /// bool, …)` collapsed both to [`stella_store::SessionStatus::Error`] — + /// this projection did not exist, and the widened `finish` call sites do + /// not compile against the old signature. + /// + /// Like `daemon::tests::a_deliberate_stop_records_a_status_distinct_from_a_crash`, + /// this relies on no test in this binary ever calling + /// `signals::note_interrupt`, so `interrupted_exit_code()` is reliably + /// `None` here. + #[test] + fn an_unsupervised_deliberate_stop_projects_stopped_not_error() { + let stopped = Ok(pipeline_outcome(PipelineStatus::Aborted { + reason: "stuck-loop detected (persisted after a steering warning)".into(), + kind: AbortKind::DeliberateStop, + })); + let crashed = Ok(pipeline_outcome(PipelineStatus::Aborted { + reason: "the model call would not commit after retries".into(), + kind: AbortKind::Failure, + })); + + assert_eq!( + pipeline_session_status(&stopped), + stella_store::SessionStatus::Stopped, + "a run that ended itself by policy must not read as a crash" + ); + assert_eq!( + pipeline_session_status(&crashed), + stella_store::SessionStatus::Error + ); + } + + /// The arms the widening must not disturb: a completed run stays + /// `Complete`, and a hard pipeline error — which carries no abort kind — + /// stays `Error`. + #[test] + fn the_remaining_terminal_arms_project_unchanged() { + assert_eq!( + pipeline_session_status(&Ok(pipeline_outcome(PipelineStatus::Completed))), + stella_store::SessionStatus::Complete + ); + let hard = Err(PipelineRunError { + cause: PipelineError::ScopeReviewRequiredHeadless, + total_cost_usd: 0.0, + }); + assert_eq!( + pipeline_session_status(&hard), + 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/agent/presence.rs b/crates/stella-cli/src/agent/presence.rs index 4d08c4a5f..c533ec067 100644 --- a/crates/stella-cli/src/agent/presence.rs +++ b/crates/stella-cli/src/agent/presence.rs @@ -81,20 +81,30 @@ impl SessionPresence { /// The headless one-shot → `/inbox` notification decision, shared by the /// pipeline and raw (`--no-pipeline`) paths so the two cannot drift: a - /// failed run always lands a notification; a successful one only when it - /// ran long enough (60s) that the user has plausibly looked away. + /// run that did not complete always lands a notification, worded by how + /// it actually ended — a policy stop is deliberate, and "FAILED" for it + /// was the same dishonesty the registry status carried (#1826); a + /// successful one notifies only when it ran long enough (60s) that the + /// user has plausibly looked away. pub(crate) fn one_shot_notification( &self, - run_ok: bool, + status: stella_store::SessionStatus, run_secs: u64, prompt: &str, ) -> Option<(String, String)> { - let title = if !run_ok { - format!("{}: run FAILED", self.name()) - } else if run_secs >= 60 { - format!("{}: run finished ({run_secs}s)", self.name()) - } else { - return None; + let title = match status { + stella_store::SessionStatus::Complete if run_secs >= 60 => { + format!("{}: run finished ({run_secs}s)", self.name()) + } + stella_store::SessionStatus::Complete => return None, + stella_store::SessionStatus::Stopped => { + format!("{}: run stopped by policy", self.name()) + } + stella_store::SessionStatus::Cancelled => { + format!("{}: run cancelled", self.name()) + } + // Anything else a terminal write carries is a genuine failure. + _ => format!("{}: run FAILED", self.name()), }; Some((title, crate::command_deck::prompt_line(prompt, 160))) } @@ -122,12 +132,19 @@ impl SessionPresence { /// notification linked to this session — the headless → `/inbox` flow: /// a finished `stella run` surfaces in every deck's inbox, and `Enter` /// replays it. - pub(crate) fn finish(&mut self, ok: bool, notify: Option<(String, String)>) { - self.record.status = if ok { - stella_store::SessionStatus::Complete - } else { - stella_store::SessionStatus::Error - }; + /// + /// `status` is the caller's own terminal answer projected by + /// [`crate::daemon::outcome_status`] (or `super::outcome`'s pipeline + /// projection) — never a bool. The bool collapsed a deliberate stop into + /// `Error`, and on an unsupervised headless run no later supervised write + /// corrected it, so the SESSIONS overlay painted a policy stop as a crash + /// (#1826, the presence half of #1653). + pub(crate) fn finish( + &mut self, + status: stella_store::SessionStatus, + notify: Option<(String, String)>, + ) { + self.record.status = status; let _ = self.registry.upsert(&self.record); // The headless counterpart of the deck's exit compaction. A one-shot // run writes fewer objects than a long deck session, but it is also the diff --git a/crates/stella-cli/src/daemon.rs b/crates/stella-cli/src/daemon.rs index d131056c3..301dad126 100644 --- a/crates/stella-cli/src/daemon.rs +++ b/crates/stella-cli/src/daemon.rs @@ -989,12 +989,13 @@ const LOCK_FD_SCAN_LIMIT: i32 = 64; /// be indistinguishable from one that died the moment its window closed. /// /// A no-op in an unsupervised process, and harmless where a surface already -/// wrote its own answer: `SessionPresence::finish` runs first on the two paths -/// that have one, and this write lands after it (agreeing on every outcome -/// except a deliberate stop, where this one knows better — the presence sees -/// only a bool, #1653). This is what covers the paths that do not — -/// `stella fleet`, and any future long-running verb that is handed to the -/// supervisor before it grows a session presence. +/// wrote its own answer: `SessionPresence::finish` runs first on the paths +/// that have one, and this write lands after it — both now project the +/// terminal answer through [`outcome_status`] (#1653, #1826), so the two +/// writes agree on every outcome, deliberate stops included. This is what +/// covers the paths that do not — `stella fleet`, and any future +/// long-running verb that is handed to the supervisor before it grows a +/// session presence. pub(crate) fn record_outcome_if_supervised(outcome: Result<(), &crate::failure::CliFailure>) { let Some(id) = supervised_id() else { return;