fix(stella-cli): unbreak main — unresolved SkipReason doc link in daemon::boot - #1970
Conversation
…not resolve it daemon::boot is a private module, so an intra-doc link to its pub(super) SkipReason resolves to nothing and rustdoc -D warnings rejects it. The link landed in #1939 and was masked until now: cargo doc bails on the first crate that fails, so stella-store's private-link error (fixed in #1965) hid this one entirely. Verified: RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps exits 0.
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 guide (collapsed on small PRs)Reviewer's GuideFixes a broken rustdoc intra-doc link in a private module by replacing an unresolved link to SkipReason::NoResumePoint with plain prose, allowing workspace docs to build cleanly under -D warnings. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Red for a reason that is not this diff — blocked on #1971. This branch is
None of those are reachable from a The one step this PR is about, Sequence: land #1971, then update this branch so its job re-runs against a compiling |
…alibration, and a revision halted on an already-green command (#1787, #1595, #1793) (#1964) > **Correction to my comment above:** the tracking issue for `main`'s broken test build is **#1972**, not #1970 — I cited the number before the issue was filed. Nothing else in that comment changes. ## What & why Three defects picked off the backlog for Terminal-Bench impact, one per metric. **#1793 landed on `main` independently while this was open (#1945), so this PR now defers to that implementation** — see §2. What remains is described below. ### #1787 — solve rate: a verifier that thinks before it answers parsed as no answer `parse_verifier_response` consulted only the first non-empty line. A reply opening with `Here is my assessment:` carried no verdict token there and returned `None` — and `None` is the signal for `heuristic_fallback`, which passes only on an observed flip or green touched tests and **otherwise FAILS**. So this did not degrade one verdict; it converted *every* verdict from a preambling model into a heuristic that mostly refuses. Reasoning models preamble by construction, which makes this a whole model family the verifier could not grade with. Two positions are now authoritative, tried in order: the reply's **first** non-empty line, then its **last**. Deliberately two, and not a scan of the body — intermediate prose is where a verifier *discusses* failing tests, and reading it would reintroduce the misread the token set was narrowed to prevent. The head still wins outright, so a reply that leads with its verdict parses exactly as before whatever its closing line says. This is the narrow half of the issue's item 1. The forced structured-output path needs a provider-parity posture per invariant 8 and is deliberately **not** here; #1787 stays open for it and for item 3. ### #1595 — cost: the default `stella run` estimated worst Five of `stella-cli`'s seven assembly sites seeded a `CalibrationMap` and handed it to the engine. The two that did not were `run_pipeline_one_shot` — which *is* `stella run` — and fleet workers, because `Pipeline` had no way to accept one. The cost is larger than a lost seed: with no map at all the correction is **inert for the whole run**, so the drift those turns measured was never applied to them either. `Pipeline` gains `with_calibration`, borrowed rather than owned because `CalibrationMap` is deliberately not `Clone` (an owned field would also cost `PipelineConfig` its `Clone`, which many call sites rely on). It attaches to **every** engine the pipeline builds through one `attach` helper — because seeding N-1 of N engines is exactly the shape of the bug being fixed, one level down. That turned out to be load-bearing within this PR: the review bot caught that the **research stage** (#1778, landed on `main` since I wrote `attach`) built its engine outside it. Fixed, and it also fixes a pre-existing bug unrelated to calibration — **a paused run's research sub-agents did not park**, because that engine never received the turn gate either. ### #1793 — a revision halted on an already-green command `main` fixed #1793 first (#1945), arriving at the same two changes this branch had. Main's implementation is taken wholesale and the duplicate dropped. One refinement survives, as `flip_halt::for_revision`, composed **on top of** `FlipHalt::unfired` rather than replacing it. `unfired` refuses a latch that already fired — the halt the *execute* turn stopped on. It cannot cover the other way a revision opens on an already-green command, because that halt has never fired: `witness_on_demand` arms a **fresh** `FlipHalt` after execution, and by then the witness it names has usually already flipped — the ordinary case, since a witness is written to pass once the work is done. A verifier can still reject such a candidate (a lint regression, a refuted verdict), and the revision then inherits an unfired halt on a command that is green before it starts, ending at the first step boundary where the model re-runs that test — before addressing anything the verifier objected to. So the oracle decides: only `FlipState::Failing` means the flip is still *ahead* of the turn. Same rule `run_candidate` already applies when it refuses to arm on a green configured baseline. Closes #1595 Refs #1787 Refs #1793 Refs #1972 ## The witness - [x] This PR includes a witness test (fails on `main`, passes here) | Witness | Covers | |---|---| | `a_verifier_that_preambles_is_read_at_its_conclusion` | a preamble before PASS/FAIL is no longer an unparseable reply | | `intermediate_prose_never_decides_a_verdict` | the widening's safety property: body prose never decides, and a reply stating no verdict is still `None` | | `the_pipeline_path_sizes_its_budget_with_the_callers_calibration` | a lent map reaches the engines | | `a_pipeline_lent_no_calibration_reports_the_identity_factor` | an unlent one invents nothing | | `a_revision_on_a_green_or_flaky_command_is_never_halted` | the `FlipState::Failing` guard — the half `unfired` cannot cover | | `a_revision_inherits_the_halt_while_the_tracked_command_is_still_red` | and it still arms when the flip is genuinely ahead | The two #1787 witnesses were checked the artisanal way: `git stash push -- crates/stella-pipeline/src/verify.rs` (source only, tests left in place) → both fail; restored → both pass. The calibration witnesses read the factor off each **engine** step's `StepManifest`, filtered to `call_seq == 0`. The pipeline's management roles ride the same step at 1, 2, … and carry the identity factor whatever the engines were lent — a raw management call has no transcript to compact — so counting them would make the test assert something untrue of the seam. ## The gate - [x] `cargo fmt --check` - [x] `cargo clippy --workspace --all-targets -- -D warnings` - [x] `cargo test --workspace` - [x] Docs updated where behavior changed (`pipeline/attachments.rs` module doc; `for_revision`'s rule stated where the semantics live; `stella-parity`'s `calibration.drift` row rewritten) - [x] `Closes #N` appears both here and as a commit trailer ## Nothing left behind - [x] Filed: #1948, #1949, #1950, #1972 - **#1972** — `main` cannot compile `stella-pipeline`'s test suite, independent of this branch. Fixed here because this PR could not be verified on it; filed separately so unbreaking `main` is tracked either way. Details in the comment above. - **#1948** — `Verdict::reasoning` is bounded twice in *different units* (chars at construction, bytes when forwarded); a multi-byte reply gets cut to a third of the budget with two truncation markers. - **#1949** — the arming half of #1793 has no end-to-end witness beyond `main`'s own; `turn.halt_on_goal_met` stays `ShippedUnwitnessed`. - **#1950** — nothing proves `run_pipeline_one_shot` and the fleet worker do the *lending*. Delete either `.with_calibration(..)` and the workspace suite stays green — which is how the original gap survived. `calibration.drift` stays `ShippedUnwitnessed` with its `missing` field rewritten to say so. ## Ground-rule check - [x] No I/O added to `stella-core`; no new dependencies - [x] No new outbound network calls - [x] No new cross-boundary types ## Anything reviewers should know? **Three commits here are not mine to claim as feature work** — they unbreak the base. `main` at `43402ae` fails `cargo test -p stella-pipeline --no-run` with three errors (#1945's test fixtures were never committed; #1953's new `ModelCallRole` doesn't satisfy #1941's deliberately-exhaustive match), and `cargo doc -D warnings` fails on a private intra-doc link in `daemon/boot.rs` that is byte-identical on `main`. Reconstructing #1945's fixtures is the part worth a reviewer's eye: the semantics are pinned by its witness passing *for the stated reason* (the `command` key `command_of` reads; the `[exit code: 0]` marker `exit_status` parses), but if that author intended something different, this is where to say so. **The file-size baseline moves, and mostly down.** `agent.rs` +3 and `fleet_cmd.rs` +3 are the call-site wiring — the seed and the lend, which need the store, the config and the pipeline all in scope. `pipeline/tests.rs` +1 is a `mod` declaration. Against those, **`pipeline.rs` ratchets down 11 lines**: `with_turn_gate` moved into the new `pipeline/attachments.rs` beside `with_calibration`, and the repeated gate-attachment blocks collapsed into `attach`. **Not measured on the bench.** These are picked for Terminal-Bench impact and argued from the code and the issues, not from a before/after run — no provider credentials were available in the environment this was written in, so no benchmark run was performed. The cost and wall-clock claims are mechanism-level; #1770/#1289 are where a real measurement would land. --------- Co-authored-by: Claude <noreply@anthropic.com>
…ead's tasks rows (#1708) (#1967) ## What A sub-session worker's closeout wrote **its own** private task board into the shared `tasks` table under the **lead's** `session_id` (`crates/stella-cli/src/subsession.rs`, `run_worker`). At the table's `UNIQUE(session_id, task_id)` key that write was pure corruption, twice over: 1. **Ordinal collision** — a worker board numbers from "1" in its own namespace, so its task "1" upserted over the lead's unrelated task "1". 2. **`/clear` seal bypass (#1692)** — the seal that quarantines pre-clear workers lives on the driver (`SubSessions::seal_task_board` → `session_clear::settle_worker_task`); this write ran on the worker's own thread where no seal can reach, so a pre-clear worker repopulated the mirror the user destroyed. ## The decision (of the three shapes in #1708) **Drop the write** — the shape #1708's investigation identified as the only one that fixes both defects alone: - The write's stated purpose ("so `tasks` queries see sub-agent boards too") is unachievable at this key — neither reader (`Store::list_session_tasks`, the Observatory's sessions query) can distinguish a worker row from a lead row. - The delegation outcome the session cares about is the **lead's** board row, which the driver already mirrors at both of its own write sites (the lead's turn end in `command_deck.rs`, worker settlement in `session_clear::settle_worker_task`). - Namespacing fixes only defect 1; routing through the driver fixes only defect 2. Dropping fixes both, with no migration and no cross-thread plumbing — and it makes the `/clear` seal airtight rather than advisory, because the driver becomes the session mirror's *sole* writer. ## The witness seam (the part #1708's investigation could not find) The worker's store closeout moves to a new sibling module, `crates/stella-cli/src/subsession/closeout.rs` — required anyway, since `subsession.rs` sits 24 lines under the file-size gate's 1500-line ceiling and could not absorb tests. `close_worker_execution` is the exact production path `run_worker` calls, and its signature takes **no session id**, so the closeout structurally cannot address session-keyed rows. Two witness tests drive that seam against a real in-memory store: - `a_workers_private_board_never_lands_in_the_sessions_tasks_rows` — the lead's mirror survives a worker closeout byte-identical, the table row **count** is pinned too (so a NULL-session append can't hide), and the execution row still closes (so "fixed by deleting the closeout" can't pass). - `a_pre_clear_workers_closeout_cannot_repopulate_a_cleared_mirror` — after `clear_session_tasks` (the persisted half of `/clear`), a worker closeout leaves the mirror exactly as empty as the user made it. **Fail→pass flip, demonstrated:** the seam is new, so `git stash` alone can't show the flip. Instead the pre-#1708 board mirror was temporarily grafted back into `close_worker_execution` (same write, keyed to the lead's session exactly as `run_worker` was) — both witnesses **fail** against the graft with the exact corruption #1708 describes ("the lead's mirror survives a worker closeout untouched" assertion trips), and **pass** with it removed. The graft was reverted before shipping. ## Verification Measured on this branch **with current `main` merged in** (head `73f99926`): - `cargo test -p stella-cli` — **1456 pass**, 0 fail, including both new witnesses - `cargo clippy -p stella-cli --all-targets -- -D warnings` — clean - `cargo fmt --check` — clean - `make guards-fast` (all 25 guards) — clean; `file-size` reports **"none grew"** against main's regenerated baseline ## CI is red for reasons outside this diff `fmt + clippy + test` fails on **five breaks inherited from `main`**, none in a file this PR touches (the diff is 3 files, all `stella-cli`): | Break | Crate | Already covered by | |---|---|---| | `unresolved link to SkipReason::NoResumePoint` (`daemon/boot.rs:59`, added by `a2806246` / #1939) | stella-cli | **#1970** | | `too many arguments (8/7)` (clippy) | stella-pipeline | **#1971** | | `PassingShell` not found | stella-pipeline | **#1971** | | `shell_call_result` not found | stella-pipeline | **#1971** | | non-exhaustive match: `ModelCallRole::Research` not covered | stella-pipeline | **#1971** | The last one is the root cause of three of these: `main` added a `Research` variant to `stella_protocol::ModelCallRole` without updating downstream exhaustive matches. This branch point has no `Research` variant at all, which is why the break is provably inherited rather than caused. No competing unbreak is included here deliberately — duplicate PRs racing the same seam is how `main` gets re-broken. This PR should go green once #1970 and #1971 land; it is already merged up to current `main` and conflict-free. No migration: the corrupted rows self-heal — the driver's next lead-board mirror upserts the colliding ordinals back to the lead's state, and `/clear` deletes the rest; stray rows beyond the lead's ordinal range in existing databases are dead rows in a table whose only production readers are per-session queries the driver now exclusively feeds. Exemplar for the shape: the pipeline's witness stage gives authored witnesses exactly this lifetime — scaffolding for one run, discarded with it — which is the model applied to a worker's private board here. ## Filed, not fixed - **#1968** — this PR deliberately closes the door on persisting sub-agent boards. If they are wanted, it takes a `lane` column + migration, routing through the driver, *and* a lane-aware reader, all three together; the issue scopes each and says why any one alone reintroduces a defect. - **#1969** — existing databases already hold corrupt rows from this bug, and they are **indistinguishable by construction** (that is #1708's whole complaint), so no migration can repair them. They render in the Observatory's Sessions tab. Needs a maintainer's call between accept / caveat / document. Refs #1692, #1631. Closes #1708
…eir fixes (#2015) ## The problem `main` at 6c34553 fails **five** independent gates. Four fixes already exist across #2000, #2003 and #2005 — but **two of those PRs are red on exactly the gate the other one repairs**, so none of them can merge: - **#2003** regenerates `scripts/file-size-baseline.txt` and touches nothing else → fails `wire-schema`, because main's `docs/wire/` is stale against its own types. - **#2005** regenerates `docs/wire/` and the park consumers → fails `file-size`, because the baseline skew is #2003's fix. That is a deadlock, and it is why main has stayed red while three unbreak PRs sat open. This branch carries both halves plus #2000's repairs so the set can land in one merge. ## The five gates | Gate | Break | Fix from | |---|---|---| | `lint` | a dead `spend` local | #2000 | | `doc-warnings` | `[`CompactionRewrite`]` resolves only via the crate-level re-export | #2000 | | `format-check` | missing trailing newline in `event/tests.rs` | #2005 | | `file-size` | baseline skew (`driver.rs` +1, `pipeline/tests.rs` +1) | #2003 | | `wire-schema` | `docs/wire/` stale against #1994's `TurnParked`/`TurnWoken` | #2005 | The rustdoc one is worth a note: **layered masking**, the shape #1965 records. Rustdoc stops at the first crate that fails to document, so #1970 had to repair `stella-cli` before `stella-protocol` underneath it became visible at all. Anyone fixing one layer and re-running would reasonably have believed they were done. ## Authorship The substantive hunks are **from #2000, #2003 and #2005** — collected here, not re-derived, so their authors keep the credit. Close those three as superseded if this lands, or close this one if they can be sequenced another way; the point is that they cannot each be green independently. I also opened #2010 for the rustdoc break before finding #2000 already covered it, and closed it as a duplicate. ## Verification Run against this exact tree, each gate with the command the Makefile uses: | Check | Result | |---|---| | `cargo clippy --workspace --all-targets -- -D warnings` | clean | | `RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps` | clean | | `cargo fmt --all -- --check` | clean | | `scripts/check-file-size.sh` | OK — none grew | | `make wire-schema` | OK — `docs/wire/` matches the types | | `cargo test -p stella-protocol -p stella-pipeline -p stella-tui -p stella-core` | pass | Each was also confirmed **failing** on `origin/main` beforehand, so this is a demonstrated repair rather than an assumed one. One caveat, stated rather than buried: `stella-tui`'s `run_deck_paints_folds_resizes_and_restores_under_a_real_pty` failed once in the batch run and passed in isolation (20s, against an 81s timeout) — a real-PTY timing flake under concurrent build load, not a regression. The baseline was regenerated with `make file-size-update`, never hand-edited — a hand-merged baseline is what produced the current skew. ## Note for reviewers No witness test: every hunk restores an existing gate to green rather than changing behavior. The reproduction table above is the evidence, and each gate flips from fail to pass across this diff. ## Summary by Sourcery Unstick main by combining previously separate fixes so all gates pass together, including lint, doc warnings, formatting, file-size checks, and wire-schema consistency. Bug Fixes: - Repair lint break by removing the unused `Spend` local from the pipeline scope stage. - Fix rustdoc warnings by correcting the `CompactionRewrite` intra-crate link to the crate-level re-export and mirroring it in generated wire docs. - Restore format-check to green by adding the missing trailing newline in `event/tests.rs`. - Update `scripts/file-size-baseline.txt` to reflect current binary sizes so file-size checks match the regenerated code and tests. - Bring `docs/wire` schemas back in sync with protocol types and serveframe definitions, including the `CompactionRewrite` documentation changes. - Ensure observatory transcripts correctly capture and render `turn_parked` and `turn_woken` events so journal gaps and wake reasons are visible. - Adjust the management prompt tests to match the current set of model roles and avoid stale expectations. - Update the fleet dashboard UI to properly represent parked turns as a distinct state instead of misclassifying them as blocked. Enhancements: - Extend the observatory database query and journal rendering to include parked and woken turn events, with operator-facing descriptions and timing details. - Add UI support in the fleet dashboard for displaying parked turns and holding their state across park/wake so operators can distinguish deliberate waits from stalls. - Clarify the flip-halt arming test module layout and move doubles into the child module to protect against silent deletion on parent rewrites. Tests: - Tidy verification hardening tests around flip-halt arming by delegating doubles into the child module and simplifying the parent’s documentation. - Align management prompt tests with the current role handling to keep the test suite reflecting real behavior. - Confirm wire-format tests and tag tables remain unchanged while restoring formatting and wire-schema consistency. Chores: - Regenerate wire schema artifacts and file-size baselines using the project’s existing tooling so all gates share a consistent view of the repository state.
…es left behind (#2014) ## What & why `main` is red at `6c345532` on **three separate gates** — `cargo fmt --check`, `cargo clippy -D warnings`, and the file-size ratchet — plus workspace rustdoc. Every open PR inherits all of it. The cause is not one bad change. Four sessions fixed the *same* red base concurrently (#1964, #1970, #1971, and an earlier push to #1964's branch). The merge that closed #1964 resolved every overlap by **keeping both sides**, which is the dangerous resolution here: it produces code that still compiles, so nothing conflicted and nobody had to look at it, and the damage only shows up under `-D warnings`. ### clippy (`-D warnings`) — four merge artefacts | Site | Lint | |---|---| | `management_prompt/tests.rs` | `ModelCallRole::Research` appears **twice** in one `\|` chain → `unreachable_patterns` | | `pipeline/scope_stage.rs` | a hoisted `let mut spend` **and** a per-iteration inline `Spend` → `unused_variables` + `unused_mut` | | `tests/verification_hardening.rs` | `SHELL_TOOL`, `shell_call_result`, `PassingShell` duplicated into the `flip_halt_arming` child → three `dead_code` | In each case the duplicate is deleted and the *used* copy kept. For `scope_stage` that is the inline `Spend`, because the loop replans after a rejected scope card and a moved bundle could not be handed to the next attempt — #1971's comment beside it already says so. ### fmt `crates/stella-protocol/src/event/tests.rs` is missing the trailing newline `rustfmt` wants after `mod tag_table;`. Unrelated to the merges and failing on its own. ### rustdoc `StepUsage` links a bare `` [`CompactionRewrite`] ``, re-exported at the crate root but never in `event`'s scope — the next line already spells the field `crate::CompactionRewrite`, so the link now matches. This one was **invisible** until the `stella-cli` link above it was fixed: `cargo doc` stops at the first failing crate, so a broken link one dependency layer down masks every link beneath it. Third occurrence of that pattern here. ### file-size ratchet `driver.rs` and `pipeline/tests.rs` each sit one line over a stale ceiling. Regenerated with `make file-size-update` rather than hand-edited — which is why the diff mostly **tightens**: `pipeline.rs` drops 3451 → 3181 and `bus.rs` 2126 → 1891. Both were already true and neither was recorded. ## The witness No witness test: this is a build/lint/format repair with no behaviour change. The gate *is* the witness, and each failure was reproduced locally before and after. ## The gate Run on this tree, not inferred: - [x] `make guards-fast` — all 25 guards plus `cargo fmt --check` - [x] `cargo clippy --workspace --all-targets -- -D warnings` - [x] `RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps` (24 crates) - [x] `cargo test --workspace` - [x] `./scripts/check-file-size.sh` and `check-god-files` ## Nothing left behind Already filed and linked, not duplicated: - **#1986** — `ci.yml` does not run on a push to `main`, which is *why* all eight of these landed unnoticed. This PR is the fourth cleanup in a row caused by that gap; it is the fix worth prioritising. - **#1972** — the original red-main report this chain started from. - **#1645** — red PRs keep landing (`enforce_admins` off). - **#1977** — `ALL_ROLES` is a hand-maintained array that can silently under-test the role family; it is exactly what let the `Research` arm drift in the first place. Refs #1986 Refs #1972 Refs #1977 ## Summary by Sourcery Repair main by resolving merge artefacts and bringing formatting, linting, documentation, and file-size checks back to green. Bug Fixes: - Remove duplicated management prompt role arm to fix unreachable pattern lint. - Drop unused scope-stage budget variable to clear unused variable lints. - Delete duplicate shell tooling fakes from verification hardening tests to remove dead code lints. - Correct rustdoc link for compaction rewrite events so documentation builds cleanly. - Add missing trailing newline in event tests module to satisfy rustfmt. Enhancements: - Clarify documentation around daemon boot resume-point handling and location of shell doubles for flip-halt tests. Build: - Regenerate file-size baseline to reflect current driver and pipeline module sizes, re-aligning with the file-size ratchet checks. Tests: - Tighten test layout by consolidating flip-halt shell doubles into a single module referenced by the arming tests. Chores: - Minor comment and whitespace cleanups across daemon boot, pipeline scope stage, and event tests.
What & why
main's workspace doc gate is red:crates/stella-cli/src/daemon/boot.rs's module doc intra-doc-linksSkipReason::NoResumePoint, butdaemon::bootis a private module andSkipReasonispub(super), so rustdoc resolves nothing andrustdoc::broken_intra_doc_linksfails under-D warnings.The link landed in #1939 and was invisible until now, which is the part worth recording:
cargo docbails on the first crate that fails, sostella-store's private-link error (fixed in #1965) masked this one entirely. Fixing one rustdoc break surfaces the next one down — expect to iterate, not to assume the first fix was the last.The fix is the same shape as #1965's: name the item in prose rather than link something rustdoc cannot see, with a parenthetical saying why.
The witness
RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-depsexits 101 onmainand 0 with this change, verified by exit code rather than by reading output.The gate
cargo doc --workspace --no-depsunder-D warnings(exit 0, the whole workspace — re-run after the fix to confirm nothing further was masked)cargo fmt --checkRefs #1939, #1965.
Summary by Sourcery
Documentation: