fix(stella-cli): make the resume frame impossible to forget (#1672) - #1930
Conversation
…ot guard
`main` fails the gate's `doc-warnings` step:
error: could not document `stella-cli`
--> crates/stella-cli/src/daemon/boot.rs:130
|
| /// ([`supervised::APPROVAL_REQUEST`]).
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no item named `supervised` in scope
|
= note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`
`boot.rs` imports `stella_store::{SessionRecord, SessionRegistry, SessionStatus}`
but not the `supervised` module, so the shortened path does not resolve. Fully
qualified, it does.
This is mine, from PR #1920 (#1698). I ran `cargo test`, clippy, `fmt --check`
and four guard scripts on that branch and reported them — but not
`RUSTDOCFLAGS="-D warnings" cargo doc`, which is a separate gate step, and the
one this needed.
It reached `main` because the required checks never ran: every `ci.yml` run
since 19:03 is still `queued` (#1899), so the branch merged on stale/absent
contexts. That is #1645's failure mode arriving by a different road — not a red
check that was overridden, but no check at all.
Verified: `RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps` exits 0.
`cargo test -p stella-cli --bin stella boot` — 24 passed.
The pipeline frame that lets a resume say what it is not restoring was declared at exactly one of four `Pipeline::new` call sites. The Command Deck, the goal loop and fleet workers declared nothing, so a checkpoint any of them left read as a plain engine turn and any resume from it degraded in silence — the exact failure #1615 closed for `stella run` alone. ## A mechanism, not a convention The obvious fix is one `resume_frame::declare` line beside each construction. That is what I wrote first, and the file-size gate rejected it: two of the four sites are in grandfathered god files closed to growth, and +1 line each put `command_deck.rs` and `fleet_cmd.rs` over their ceilings. The issue anticipated this and suggested routing through a helper. That turns out to be the better design regardless: `resume_frame::pipeline` declares and constructs in one call, so a surface **cannot come to hold a pipeline whose checkpoints do not say what they are**. Remembering is no longer part of the contract. One call replaces one call, so all three god files land exactly at their ceilings with no baseline bump: command_deck.rs 4621 / 4621 fleet_cmd.rs 1504 / 1504 agent.rs 2266 / 2266 `agent.rs` needed one line back to get there; extending its existing `use crate::{…}` group rather than adding an import line is where it came from. Callers still chain what is genuinely theirs — the deck and fleet workers add `with_turn_gate`, because the gate is per-surface while the frame is not. ## Witness `every_pipeline_construction_declares_its_resume_frame` walks `stella-cli/src` and fails on any `Pipeline::new(` outside the one helper, naming file and line. Verified to catch the regression it exists for by deleting the goal loop's call: these sites call `Pipeline::new` directly and so declare no resume frame … ["…/crates/stella-cli/src/agent/goal.rs:775"] It greps the source rather than driving a turn, deliberately, and the issue asks for exactly this: what went wrong was **wiring**, not logic. Every unit test here already passed while three surfaces never called the function. A behavioural test would need one scripted run per surface and would still only cover the surfaces somebody remembered to write a test for — the same gap one level up. The repo uses source-grep guards for this shape already (the `stella fullauto` wrapper guards from #1619). The guard skips its own file: the needle appears in its string literal, so it would otherwise report itself forever. Found by running it. ## One rustdoc repair Dropping the now-unused `Pipeline` imports orphaned an intra-doc link in `goal.rs` that resolved through one of them; it is fully qualified now. Caught by `RUSTDOCFLAGS="-D warnings" cargo doc`, which is a gate step — and which is also how I found that `main` was already red for an unrelated broken link of mine (PR #1929, which this branch is stacked on). `cargo test -p stella-cli` — 1446 passed, 0 failed. Clippy `-D warnings`, `fmt --check`, workspace rustdoc `-D warnings`, `check-file-size`, `check-god-files` and `check-left-behind` all clean. Closes #1672 Refs #1615, #1671
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Reviewer's GuideCentralizes pipeline construction through a new resume_frame::pipeline helper that always declares the resume frame before building a Pipeline, updates all existing call sites to use it (while preserving per-surface turn-gate wiring), and adds a source-grep test to enforce that no other direct Pipeline::new usages exist, plus a minor rustdoc link fix. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Retargeted to This branch therefore still carries two commits: The second is byte-identical to #1927's fix. If #1927 merges first, rebasing this branch drops it automatically (git recognises the identical patch); if this merges first, #1927 becomes the no-op. Either order is safe — I have kept it rather than dropping it now, because removing it would leave this branch red on the Note that the #1672 change itself also repairs a doc link — dropping the now-unused
|
|
Merging this does double duty — it is probably the one to take first.
This branch already carries the one-line fix (
Either unblocks all seven of my open PRs at once. The duplicated one-liner resolves itself — whichever lands second, git recognises the identical patch on rebase, and I have kept it here deliberately rather than dropping it, so this branch is not itself red on a break it did not cause. For what it is worth the break is mine, from #1920, which merged during the window when Verified locally on this branch: |
1 participant
fix(stella-cli): make the resume frame impossible to forget
The pipeline frame that lets a resume say what it is not restoring was declared
at exactly one of four
Pipeline::newcall sites. The Command Deck, the goalloop and fleet workers declared nothing, so a checkpoint any of them left read
as a plain engine turn and any resume from it degraded in silence — the exact
failure #1615 closed for
stella runalone.A mechanism, not a convention
The obvious fix is one
resume_frame::declareline beside each construction.That is what I wrote first, and the file-size gate rejected it: two of the four
sites are in grandfathered god files closed to growth, and +1 line each put
command_deck.rsandfleet_cmd.rsover their ceilings.The issue anticipated this and suggested routing through a helper. That turns
out to be the better design regardless:
resume_frame::pipelinedeclares andconstructs in one call, so a surface cannot come to hold a pipeline whose
checkpoints do not say what they are. Remembering is no longer part of the
contract.
One call replaces one call, so all three god files land exactly at their
ceilings with no baseline bump:
agent.rsneeded one line back to get there; extending its existinguse crate::{…}group rather than adding an import line is where it came from.Callers still chain what is genuinely theirs — the deck and fleet workers add
with_turn_gate, because the gate is per-surface while the frame is not.Witness
every_pipeline_construction_declares_its_resume_framewalksstella-cli/srcand fails on any
Pipeline::new(outside the one helper, naming file and line.Verified to catch the regression it exists for by deleting the goal loop's
call:
It greps the source rather than driving a turn, deliberately, and the issue
asks for exactly this: what went wrong was wiring, not logic. Every unit
test here already passed while three surfaces never called the function. A
behavioural test would need one scripted run per surface and would still only
cover the surfaces somebody remembered to write a test for — the same gap one
level up. The repo uses source-grep guards for this shape already (the
stella fullautowrapper guards from #1619).The guard skips its own file: the needle appears in its string literal, so it
would otherwise report itself forever. Found by running it.
One rustdoc repair
Dropping the now-unused
Pipelineimports orphaned an intra-doc link ingoal.rsthat resolved through one of them; it is fully qualified now. Caughtby
RUSTDOCFLAGS="-D warnings" cargo doc, which is a gate step — and which isalso how I found that
mainwas already red for an unrelated broken link ofmine (PR #1929, which this branch is stacked on).
cargo test -p stella-cli— 1446 passed, 0 failed. Clippy-D warnings,fmt --check, workspace rustdoc-D warnings,check-file-size,check-god-filesandcheck-left-behindall clean.Closes #1672
Refs #1615, #1671
Summary by Sourcery
Ensure all stella-cli pipelines declare their resume frame via a single construction helper to prevent silent degradation when resuming from checkpoints.
Bug Fixes:
Enhancements:
resume_frame::pipelinehelper that couples resume frame declaration with pipeline construction so surfaces cannot forget to declare the frame.Tests:
Pipeline::newcall goes through the resume-frame-aware helper and fails if any direct constructions remain.