Skip to content

fix(stella-cli): record a deliberate stop distinctly on the unsupervised writers too (#1826) - #1878

Merged
macanderson merged 4 commits into
mainfrom
fix/1826-unsupervised-stop-status
Aug 6, 2026
Merged

fix(stella-cli): record a deliberate stop distinctly on the unsupervised writers too (#1826)#1878
macanderson merged 4 commits into
mainfrom
fix/1826-unsupervised-stop-status

Conversation

@macanderson

@macanderson macanderson commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Problem

PR #1828 (#1653) taught the supervised registry writers (daemon::outcome_status and its callers) to record SessionStatus::Stopped for a policy stop — but SessionPresence::finish(ok: bool, …) and its three call sites still collapsed the outcome to a bool on the way in. On a supervised run this was masked, because record_outcome_if_supervised writes after finish and corrects it. On an unsupervised headless run (a pipe, CI, --foreground) there is no later write: a deliberate stop (AbortKind::DeliberateStop — stuck-loop escalation, step cap, enforced budget, ended scope review; exit 3) aged into the SESSIONS overlay as Error, indistinguishable from a crash.

Approach

Mirrors #1828's shape: widen the writer past the bool, and keep one decider.

  • crates/stella-cli/src/agent/outcome.rspipeline_session_status, the fifth projection the module doc now names: Result<PipelineOutcome, PipelineRunError> → terminal SessionStatus, routed through crate::daemon::outcome_status rather than re-matched, so every registry writer — supervised or not — reads a deliberate stop (Stopped), a user interrupt (Cancelled), and a crash (Error) off the same function.
  • crates/stella-cli/src/agent/presence.rsSessionPresence::finish now takes the projected SessionStatus; one_shot_notification words the inbox entry by how the run actually ended (a policy stop notifies "run stopped by policy", an interrupt "run cancelled" — "FAILED" for a deliberate stop was the same dishonesty the status carried; the issue delegated this wording decision).
  • Call sites: agent.rs (pipeline one-shot — projects the PipelineStatus it holds; the interactive exit passes Complete explicitly), agent/goal.rs raw one-shot (feeds its Result<(), CliFailure> straight to outcome_status, the same idiom as agent/resume.rs).
  • agent/goal.rs goal loop: audited — its Result<(), String> has no room for the abort kind (the run_resume answers with a String, so a resumed turn's deliberate stop cannot exit 3 #1637 collapse one level deeper), so a policy-stopped goal round still records Error; it now at least projects through outcome_status so an interrupt records Cancelled. Filed as The goal loop and the deck's lead turn answer with a String, so their terminal writers cannot record a policy stop #1862, cited at the call site.
  • Deck session_exit (command_deck.rs:2123): audited, unchanged — run_lead_turn returns the same stringly Result<(), String>, so the deck cannot see a deliberate stop today; covered by The goal loop and the deck's lead turn answer with a String, so their terminal writers cannot record a policy stop #1862 (and command_deck.rs is a god file closed to growth).
  • daemon.rs: the record_outcome_if_supervised doc said the presence "sees only a bool" — now stale, updated (stale comments are bugs).

God files: agent.rs sits at its exact 2269-line ceiling and this diff keeps it at exactly 2269 (all edits line-neutral); new logic lands in agent/outcome.rs and agent/presence.rs, the siblings the issue names.

Witness

outcome::tests::an_unsupervised_deliberate_stop_projects_stopped_not_error — a deliberate stop projects Stopped, a crash Error (plus the_remaining_terminal_arms_project_unchanged pinning Complete and the hard-error arm). The witness is structural, mirroring #1828's own (daemon::tests::a_deliberate_stop_records_a_status_distinct_from_a_crash, whose doc says "on the old signature this test does not even compile"): on main, pipeline_session_status does not exist and the widened finish call sites do not compile, so the fail-on-old is a compile failure rather than an assertion failure. On the old code the collapse is visible in source: finish wrote if ok { Complete } else { Error } — a deliberate stop could not reach any other status.

Verification

  • cargo test -p stella-cli --bin stella -j 21429 passed, 0 failed (includes the new witnesses, daemon::tests, and the goal tests).
  • cargo clippy -p stella-cli -j 2 --all-targets -- -D warnings — clean (with main's pre-existing stella-pipeline lint patched locally, see below).
  • cargo fmt -p stella-cli -- --check — clean; check-file-sizeagent.rs unchanged at its exact ceiling.

Main-side gate note: main was red on three pre-existing gate steps when this branch was cut (clippy clone_on_copy in stella-pipeline — which masked a second field_reassign_with_default error behind it — file-size ceilings for event.rs/deck_render.rs, and gate-parity). All three were fixed on main by #1894 (my parallel unbreak #1873 was superseded and is closed with zero diff). This branch has since had main merged in and its CI runs against the green tip; it contains none of those failures and none of their fixes.

Closes #1826
Refs #1653
Refs #1862

Summary by Sourcery

Ensure unsupervised runs record distinct session statuses for deliberate policy stops, user interrupts, and crashes, aligning all registry writers on a single outcome projection.

Bug Fixes:

  • Fix unsupervised headless runs incorrectly recording deliberate policy stops as generic errors in the SESSIONS registry.

Enhancements:

  • Add a shared projection from pipeline outcomes to terminal session status so supervised and unsupervised writers use the same outcome classification.
  • Improve headless inbox notification wording to reflect whether a run completed, was stopped by policy, cancelled, or genuinely failed.

Tests:

  • Add tests confirming the new pipeline-to-session status projection distinguishes deliberate stops from crashes and preserves existing terminal states.

…sed writers too

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

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@vercel

vercel Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
stella-cli-docs Ignored Ignored Preview Aug 6, 2026 6:35pm

@sourcery-ai

sourcery-ai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Unifies how terminal session status is derived and recorded for both supervised and unsupervised runs so deliberate policy stops are distinguished from crashes, by widening the SessionPresence finish API to take a SessionStatus, adding a shared pipeline→SessionStatus projection, updating call sites and notifications, and adding tests to lock in the behavior.

Sequence diagram for unified terminal session status derivation in unsupervised runs

sequenceDiagram
    participant Agent as run_pipeline_one_shot
    participant Outcome as pipeline_session_status
    participant Daemon as daemon::outcome_status
    participant Presence as SessionPresence
    participant Registry as registry.upsert

    Agent->>Outcome: pipeline_session_status(&result)
    Outcome->>Daemon: outcome_status(terminal.as_ref().map(|_| ()))
    Daemon-->>Outcome: SessionStatus
    Outcome-->>Agent: SessionStatus

    Agent->>Presence: one_shot_notification(status, run_secs, prompt)
    Presence-->>Agent: Option<(String, String)>

    Agent->>Presence: finish(status, notify)
    Presence->>Registry: upsert(&self.record)

    alt AbortKind::DeliberateStop
        Note over Daemon,Presence: outcome_status → SessionStatus::Stopped
    else AbortKind::Failure
        Note over Daemon,Presence: outcome_status → SessionStatus::Error
    end
Loading

File-Level Changes

Change Details Files
Introduce a shared projection from pipeline results to terminal SessionStatus used by unsupervised writers.
  • Document PipelineStatus as now being projected a fifth way into a terminal SESSIONS-registry status.
  • Add pipeline_session_status that converts Result<PipelineOutcome, PipelineRunError> into SessionStatus via crate::daemon::outcome_status, treating hard pipeline errors as genuine failures.
  • Add tests ensuring deliberate stops project to Stopped, crashes to Error, completed runs to Complete, and hard errors remain Error.
crates/stella-cli/src/agent/outcome.rs
Widen SessionPresence APIs from bools to SessionStatus and make notifications reflect the actual terminal status.
  • Change one_shot_notification to accept SessionStatus instead of bool and to word titles as 'stopped by policy', 'run cancelled', 'run finished', or 'run FAILED' based on status and run duration.
  • Change finish to accept and persist a SessionStatus directly, removing the internal bool→status collapse and documenting that status must come from outcome_status or the pipeline projection.
crates/stella-cli/src/agent/presence.rs
Update agent call sites to compute and pass SessionStatus instead of bools for one-shot and goal flows.
  • In run_pipeline_one_shot, derive session_status via pipeline_session_status and feed it to one_shot_notification and finish instead of using a run_ok bool.
  • In run_raw_one_shot, derive SessionStatus via crate::daemon::outcome_status(outcome.as_ref().map(
_
Align supervised outcome recording docs with the new unified outcome_status-based projection.
  • Update record_outcome_if_supervised documentation to state that both supervised and presence writes project through outcome_status and now agree on all outcomes, including deliberate stops.
crates/stella-cli/src/daemon.rs

Assessment against linked issues

Issue Objective Addressed Explanation
#1826 Widen SessionPresence::finish beyond a bool so that callers pass a proper SessionStatus derived from the terminal pipeline/CLI outcome (including AbortKind::DeliberateStop mapping to SessionStatus::Stopped), and update one_shot_notification to key off this status while preserving its basic notification semantics.
#1826 Ensure unsupervised/headless runs (pipeline and raw --no-pipeline) record a deliberate stop as SessionStatus::Stopped rather than Error in the SESSIONS registry by routing their outcomes through a shared projection (e.g., via daemon::outcome_status), so supervised and unsupervised writers agree.
#1826 Audit the deck’s session_exit writer for the same collapse of deliberate stops to a bool, and update it to record SessionStatus::Stopped if/when a deliberate-stop outcome becomes observable there; add a witness test that drives an unsupervised headless deliberate stop and asserts the stored status is Stopped (not Error).

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@macanderson
macanderson merged commit 7c558f1 into main Aug 6, 2026
4 checks passed
@macanderson
macanderson deleted the fix/1826-unsupervised-stop-status branch August 6, 2026 19:05
macanderson added a commit that referenced this pull request Aug 6, 2026
… turn to the terminal writers (#1893)

## What & why

The goal loop (`run_goal_cmd` / `run_goal_turn` /
`run_goal_pipeline_turn`) and the deck's lead turn (`run_lead_turn` /
`run_lead_pipeline_turn`) answered with `Result<(), String>`, which has
no room for the abort's typed `AbortKind` — so on their paths a
deliberate stop (stuck-loop escalation, step cap, enforced budget) was
indistinguishable from a crash by the time the terminal
SESSIONS-registry status was written. A policy-stopped goal run recorded
`SessionStatus::Error`, never `Stopped`, and exited `1` instead of `3`.

This chases #1637's shape one level deeper, exactly along the seam #1862
specs:

- **`stella-core`**: `GoalOutcome::Unmet` now carries `kind:
Option<AbortKind>` — the typed kind of the working turn's abort, `None`
for the backstops that are not turn aborts (round cap, unreachable
verifier). `GoalOutcome` has no consumer outside `stella-cli`.
- **`stella-cli` goal loop**: the three drivers answer with
`crate::failure::CliFailure`. The folds that stringified
`PipelineStatus::Aborted` / `GoalOutcome::Unmet` are now shared
projections in `agent/outcome.rs` (`goal_round_break`,
`goal_unmet_failure` — siblings of `pipeline_status_result` /
`turn_outcome_result`, same messages as before), and `run_goal_cmd`'s
terminal `presence.finish` projects the *real* failure through
`daemon::outcome_status` instead of a reconstructed `CliFailure::error`.
The inbox notification for a deliberate stop now says "stopped by
policy" rather than "FAILED".
- **`stella-cli` deck**: `run_lead_turn` / `run_lead_pipeline_turn`
answer with `CliFailure` through the existing `turn_outcome_result` /
`pipeline_status_result` projections, and the `session_exit` write reads
`daemon::outcome_status` — one decider for every terminal writer
(#1653/#1826/#1862). `command_deck.rs` shrank by 7 lines; `agent.rs`
stayed at its exact ceiling (one line-neutral visibility edit:
`pub(crate) mod outcome;`).

Design exemplar: the same total-`match` projection module pattern
`agent/outcome.rs` already established (and `std`'s "constructors on the
error type" shape for `CliFailure::from_abort`) — no new patterns
invented.

Closes #1862

Refs #1826, #1653, #1637

## The witness

- [x] This PR includes a witness test (fails on `main`, passes here)

Same family and justification as #1826's
`an_unsupervised_deliberate_stop_projects_stopped_not_error`:

-
`agent::outcome::tests::a_policy_stopped_goal_round_projects_stopped_not_error`
— the fold an aborted working round takes to the terminal registry write
keeps `AbortKind::DeliberateStop`, and `outcome_status` projects
`Stopped`; the `Failure` kind still projects `Error`. Fails on the old
code the way #1826's witness does: the projection did not exist, and the
old fold stringified the status so the terminal write could only
reconstruct `CliFailure::error` → `Error`.
-
`agent::outcome::tests::a_policy_stopped_raw_goal_loop_projects_stopped_not_error`
— the raw (`--no-pipeline`) half; the kind-less backstops (round cap)
stay `Error`.
- `stella-core`:
`goal::tests::session_budget_caps_total_spend_across_rounds` now asserts
the enforced-budget stop reaches `Unmet` as
`Some(AbortKind::DeliberateStop)`, and
`aborted_working_turn_ends_the_goal_loop` asserts a provider failure
reaches it as `Some(AbortKind::Failure)` — neither pattern compiles
against the old kind-less enum.

## The gate

- [x] `cargo fmt --check` (touched crates)
- [x] clippy `-D warnings` — `stella-cli` + `stella-core` clean.
**Pre-existing break, not this PR's**:
`stella-pipeline/src/pipeline/witness_stage.rs:55` fails `clone_on_copy`
on the base branch and on `main`; open unbreak PRs #1873 / #1859 own it.
I verified my crates lint clean with that one line patched locally
(patch not included — a peer PR owns the fix).
- [x] `cargo test -p stella-core` goal suite (18/18) and `cargo test -p
stella-cli --bin stella` (1431/1431) — scoped per the 16GB-machine
constraint; CI runs the full workspace.
- [x] `RUSTDOCFLAGS="-D warnings" cargo doc -p stella-core -p stella-cli
--no-deps` clean
- [x] Docs: doc comments updated where the folds moved; no flags changed
- [x] CLA signed
- [x] `Closes #1862` above and as a commit trailer

`scripts/check-file-size.sh` flags `stella-protocol/src/event.rs` (+2)
and `stella-tui/src/deck_render.rs` (+3): both overages exist verbatim
on the base branch and are named by unbreak PR #1873 — untouched here.

## Nothing left behind

- **Base branch**: PR #1878 (`fix/1826-unsupervised-stop-status`) is not
merged yet, and this change builds directly on its `finish(status, …)`
projections — so this PR targets that branch as base. **Retarget to
`main` after #1878 merges.**
- Behavioral note, deliberate: a deliberately stopped goal run now exits
`3` (was `1`), consistent with the exit-code taxonomy in `failure.rs`
and with what #1637 already did for the resume driver;
`bench/harbor_adapter` reads exactly this code. Also the deck's
soft-stop (`SOFT_STOP_REASON`) session exit now records through
`outcome_status` like every other writer.
- The pre-existing `main` breaks encountered during verification
(pipeline clippy, two file-size overages) are already tracked by open
PRs #1873 / #1859 — nothing new to file.

## Summary by Sourcery

Preserve typed abort information through goal and lead execution paths
so terminal session status and exit codes distinguish deliberate policy
stops from crashes.

New Features:
- Propagate AbortKind via GoalOutcome::Unmet and new CLI projections so
policy-stopped goal runs now surface as "Stopped" with dedicated
messaging instead of generic failures.

Bug Fixes:
- Ensure goal runs and lead turns that stop due to policy or enforced
budgets are recorded as SessionStatus::Stopped and exit with the correct
non-error code instead of being treated as crashes.
- Fix deck soft-stop handling so the session exit status and user-facing
error events are derived from the shared outcome projection rather than
raw strings.

Enhancements:
- Standardize CLI failure handling for goal and lead flows by returning
CliFailure instead of String and reusing shared outcome projection
helpers.
- Expose agent outcome helpers for reuse across goal and deck paths,
consolidating terminal status decisions into a single
outcome_status-based mechanism.
- Extend goal-related tests in stella-core and stella-cli to assert that
enforced budgets and provider failures carry the correct AbortKind
through to session status projection.

Tests:
- Add witness tests under agent::outcome validating that policy-stopped
goal rounds and raw goal loops project SessionStatus::Stopped instead of
Error.
- Update stella-core goal tests to assert that budget caps and provider
failures are reflected as GoalOutcome::Unmet with the appropriate
AbortKind values.

---------

Co-authored-by: Stella Test <test@stella.local>
macanderson added a commit that referenced this pull request Aug 7, 2026
Resolves the conflicts in favour of the implementations that already
shipped on main where the two sides fixed the same issue independently:

- #1653 (deliberate stop vs crash): main's SessionStatus::Stopped design
  and Result-shaped outcome_status/record_outcome_if_supervised (#1828,
  #1878) over this branch's Cancelled-collapsing Option shape.
- #1690 (pid addressing): main's resolve() with the shared only() helper
  and its error wording (#1723).
- #1616 drain residue: main's arm_panic_drain + pump_thread self-join
  guard; this branch's unwired drain_shared duplicate is dropped.
- fleet claims parse test: main moved it to stella-cli/src/tests.rs;
  the copy this branch kept in fleet_claims.rs would have duplicated it.
macanderson added a commit that referenced this pull request Aug 7, 2026
… a recorded-error crash at boot (#1696)

Main independently landed the rest of this batch while it sat: the
deliberate-stop status (#1653 via SessionStatus::Stopped, #1828/#1878),
pid addressing (#1690 via #1723), the parked-run boot skip (#1698 via
#1920), and the #1616 drain/panic-hook machinery. Those files are taken
from main wholesale; the duplicated pid test, the stale-signature
outcome_status test, and the unwired drain_shared duplicate go with
them.

What remains is #1696, rebuilt on main's design: boot::decide lets
`Error` fall through to the resume-point check, because since #1653 a
policy stop records `Stopped` and `Error` means only 'it fell over'.
Safe twice over — a deliberate stop retracts its checkpoint on every
terminal path, so a pre-#1653 row is filtered by NoResumePoint anyway,
and the boot attempt ledger still bounds an Error that resumes into
another Error.

Closes #1696
macanderson added a commit that referenced this pull request Aug 7, 2026
#1939)

## What this delivers now

**#1696 only.** While this batch sat, main independently landed the rest
of it:

- #1653 (deliberate stop vs crash) — via `SessionStatus::Stopped`
(#1828, #1878), a richer design than this branch's `Cancelled` collapse
- #1690 (pid addressing) — via #1723, with the shared `only()` resolver
- #1698 (parked run skipped by the boot sweep) — via #1920
- the #1616 panic-drain machinery — via `arm_panic_drain` + the
`pump_thread` self-join guard; this branch's unwired `drain_shared`
duplicate is dropped

The merge takes main's shipped implementations for all of those, and
rebuilds the one unshipped fix on top of them.

## The remaining fix (#1696)

`boot::decide` used to skip **every** terminal status, `Error` included
— a compromise forced by #1653's ambiguity, at the honest cost of
stranding a crash that lived just long enough to record `Error`. With
#1653 landed, a policy stop records `Stopped`, so `Error` means only "it
fell over", and this lets it fall through to the resume-point check.

Safe twice over, both checked rather than assumed:
- a deliberate stop retracts its checkpoint on every terminal path, so a
pre-#1653 row that stored `Error` for a policy stop is filtered by
`NoResumePoint` anyway;
- the boot attempt ledger still retires an `Error` that resumes into
another `Error` after `MAX_BOOT_ATTEMPTS`.

## Witness

`a_crash_that_recorded_itself_is_continued_but_a_policy_stop_is_not`
fails on main (where `decide` skips every terminal status) and passes
here. The exhaustive property
`nothing_is_ever_continued_without_a_resume_point` is widened to pin the
new rule: only live statuses and `Error` are ever continued.

Verified locally: `cargo test -p stella-cli` daemon suite (75 passed),
`cargo clippy -p stella-cli --all-targets -- -D warnings` clean.

Closes #1696
Refs #1653, #1690, #1698 — shipped on main by #1878, #1723, #1920
respectively.

---------

Co-authored-by: Stella Test <test@stella.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SessionPresence::finish and the deck's exit writer still collapse a deliberate stop to a bool

1 participant