fix(stella-pipeline): unbreak main — three parallel-merge collisions in one round - #1975
Closed
macanderson wants to merge 2 commits into
Closed
fix(stella-pipeline): unbreak main — three parallel-merge collisions in one round#1975macanderson wants to merge 2 commits into
macanderson wants to merge 2 commits into
Conversation
…an uncovered ModelCallRole main was red at 43402ae: stella-pipeline's lib test build failed with three compile errors, so every PR against it inherits a red gate. Two independent parallel-merge collisions, neither visible to the CI of the PR that caused it: 1. #1951 rewrote tests/verification_hardening.rs from a pre-#1945 base, deleting the PassingShell double, shell_call_result, and the configured-command witness, while leaving the 'mod flip_halt_arming;' #1945 had added — so the child module referenced two symbols that no longer existed. Restored into flip_halt_arming.rs itself rather than the parent: verification_hardening.rs is 1434 lines against a 1500 ceiling and cannot hold them, and the child is their only user. Both #1793 witnesses now sit in one module. 2. #1778 added ModelCallRole::Research; management_prompt/tests.rs holds a deliberately exhaustive match over the enum, which #1778's own CI never compiled against. Research runs as an engine sub-agent turn, so its system prompt rides its SubAgentSpec and it joins the never-dispatched-through-the-chokepoint arm — the same grouping, with the same reasoning, that raw_usage.rs already gives it. Added to ALL_ROLES too, since that array is 'every role the crate can dispatch'. No behavior change: test-only code plus one test-only match arm.
… the research stage Third break on main from the same round of parallel merges: #1778 added 'research: &[ResearchFinding]' to plan_stage, taking it to 8 arguments against clippy's limit of 7. The gate runs clippy at -D warnings, so stella-pipeline could not pass it. Fixed structurally, not with an #[allow]: budget and total are exactly the pair the crate's own Spend envelope bundles (stage_budget.rs), and they travel together everywhere else — verifier() already took this shape in #1951. plan_stage takes Spend, plan_with_review builds one before its re-plan loop and reborrows it per iteration, and neither uses the two for anything else. 8 arguments become 7 and the meaning is unchanged. No behavior change.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
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
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
Reviewer's GuideRepairs three merge-induced breakages in stella-pipeline: restores flip-halt test doubles and a configured-command witness into the flip_halt_arming module, updates management_prompt tests to account for the new Research ModelCallRole, and refactors plan_stage’s budget/total parameters into a Spend struct to satisfy Clippy’s argument limit while preserving behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was referenced Aug 7, 2026
Owner
Author
|
Follow-ups from the "Nothing left behind" section, now filed as handoffs:
|
This was referenced Aug 7, 2026
macanderson
added a commit
that referenced
this pull request
Aug 7, 2026
…ngress (#1787) (#1982) > **Stacked on #1975.** Based on `unbreak-main-pipeline` because `main` does not > currently compile `stella-pipeline`'s test target; the diff below is the one > commit on top. GitHub retargets this to `main` when #1975 merges. ## What & why The last unbounded ingress into the verdict prompt, and the item #1787 folds in at the end of its body: > Also worth folding in: the trusted evidence summary has no length bound > (`oracle_trace` grows per observation; the diff has a token budget, the > trusted zone does not) — `pipeline/evidence.rs` since the extraction. `verifier_evidence_summary` renders `oracle_trace` in full. That trace gains an observation per verification round, and the repair gate (#1479) keeps granting rounds for as long as a measured budget affords them — so the one channel that grows without limit was also the one channel with no ceiling. Every other input to that prompt is bounded: the diff has a token budget, recall frames have `bound_recalled_frames`, and `Verdict::reasoning` got its cap in #1932. Bounded to the newest 24 observations, with the drop **stated in-band**: ``` oracle_trace=[…76 earlier observation(s) omitted → candidate:pass → candidate:fail → …] ``` Three choices worth naming, because each has a wrong-looking alternative: - **Newest kept, oldest dropped.** The recent runs are what the verdict weighs; a trace clipped from the front would hand the verifier a history that stops before the evidence. - **Stated, not silent.** A trace that silently began mid-run reads as the whole run — the verifier would draw conclusions about a first observation that was not the first. - **Clipped where the value is constructed**, not at a downstream consumer. That is the same "structural, not by convention" rule #1932 applied to `reasoning`, and it is why the stored `LadderSnapshot` and `verdict_provenance` are deliberately untouched: the bound is on the *prompt ingress*, not on the record. 24 is sized far above a normal run (a baseline plus a handful of rounds), so the bound only ever bites a pathological loop. It is a named constant next to its rationale rather than a literal. ## The witness - [x] This PR includes a witness test `a_pathological_oracle_trace_is_clipped_with_the_drop_stated` — a 100-observation trace renders exactly 24 entries behind the `…76 earlier observation(s) omitted →` marker, and still ends on the newest observation. `an_ordinary_oracle_trace_renders_unchanged` is the other half, and the one that matters for regression: a 5-observation trace is asserted **byte-identical to `render_oracle_trace`**, main's own unbounded function, which is still present and still used for provenance. So every prompt the bound does not bite is unchanged to the byte — which also means no verdict-reuse digest (#1431) moves for an ordinary run. Honest note on "fails on main": these pin a bound that does not exist on `main`, so the failure there is that `bounded_oracle_trace` is not defined — the same shape as #1932's witnesses for the `reasoning` cap, and the shape any "add a missing ceiling" change has. The behavioural claim is carried by the second test, which compares against main's function directly rather than against a copied expectation. ## The gate - `cargo test -p stella-pipeline` — 585 + 5 + 2 + 5 + 6 + 4 = **607 passed, 0 failed** - `cargo clippy -p stella-pipeline --all-targets` — clean - `cargo fmt --check -p stella-pipeline` — clean - `scripts/check-file-size.sh` — OK, none grew (`evidence.rs` was extracted from `pipeline.rs` precisely so this kind of channel can be added without touching a god file, and that still holds) Full workspace left to CI. ## Nothing left behind `Refs #1787`, deliberately not `Closes` — this is the folded-in bound only. Item 1 (a provider-parity-aware structured verdict output path, invariant 8) remains open and is being approached from a different angle in #1964; item 2 shipped as #1932 and item 3 as #1951. Refs #1787 ## Summary by Sourcery Bound the oracle trace rendered in the verifier prompt to a fixed number of recent observations and document the truncation in-band while preserving full traces in stored provenance. Enhancements: - Introduce a bounded oracle trace renderer for the verifier prompt, limiting the trusted-zone trace to the newest 24 observations and prefixing output with an omission marker when older entries are dropped. Tests: - Add tests ensuring pathological long oracle traces are clipped with an explicit omission notice and that ordinary short traces remain byte-identical to the existing unbounded rendering.
macanderson
added a commit
that referenced
this pull request
Aug 7, 2026
…ngress (#1787) (#2002) ## Why this PR exists **#1787's fix is not in `main`.** PR #1982 carried it, but its base was the topic branch `unbreak-main-pipeline`, whose own PR (#1975) was **closed, not merged**. #1982 then merged into that dead branch, so the oracle-trace bound landed nowhere `main` can see, and nothing is carrying that branch forward. It also merged in a **broken** state. While the base was being reconciled with `main`, git's auto-merge of the two independently-written unbreaks concatenated both sides, leaving: - `struct PassingShell` and `fn shell_call_result` **defined twice** - `async fn a_revision_halts_at_the_step_where_the_tracked_test_flips` defined twice - a duplicate `ModelCallRole::Research` match arm (unreachable pattern) None of that compiles. `unbreak-main-pipeline` currently holds it; `main` is unaffected. This PR is the clean landing: **`main` plus `evidence.rs`, and nothing else.** ## What it does (#1787) Bounds the oracle trace at the verifier-prompt ingress. The trace grows once per verification round and the repair gate can keep granting rounds while a measured budget affords them — so unlike the diff, which rides under a token budget, this channel had **no ceiling at all**. - `MAX_ORACLE_TRACE_OBSERVATIONS = 24` — sized far above a normal run (baseline plus a handful of rounds) so the bound only bites a pathological loop. - `bounded_oracle_trace` keeps the **newest** observations and states the drop **in-band** (`…N earlier observation(s) omitted → …`), so the verifier reads "earlier observations exist" rather than a trace that silently starts mid-run. - The **stored snapshot keeps the full trace**; only the prompt ingress is clipped — the structural-bound rule from #1932. ## Witnesses - `a_pathological_oracle_trace_is_clipped_with_the_drop_stated` — a 100-observation trace renders clipped to the newest 24 with the omission counted in-band. - `an_ordinary_oracle_trace_renders_unchanged` — the bound does not touch a normal run, so this cannot ship as "always clip". Observations alternate pass/fail in the fixture so a clipped render is distinguishable from a repeated one. ## Verification - `cargo test -p stella-pipeline` — **585 pass**, 0 fail, including both witnesses above - `cargo fmt --check -p stella-pipeline` — clean - Diff vs `main` is exactly one file: `crates/stella-pipeline/src/pipeline/evidence.rs` (+74/−2) ## CI is red on `main`'s breaks, not this diff This branch is merged up to current `main`. Every failing step fails in a file this PR does not touch, and each already has a dedicated unbreak in flight: | Failing step | Where | Covered by | |---|---|---| | `check-file-size` | `scripts/file-size-baseline.txt` (parallel-merge skew) | **#2003**, **#2008** | | `cargo fmt --check` | not this crate's file | **#2005** | | clippy: unused `spend` / unused `mut` | `pipeline/scope_stage.rs:34` — a dead local `#1985` left behind | **#2000** | | rustdoc: unresolved `CompactionRewrite` | `stella-protocol` | **#2010** | The clippy one is worth naming precisely, since it is `stella-pipeline`: `main`'s `scope_stage.rs` binds `let mut spend = Spend { budget, total };` and then never uses it — the loop constructs a fresh `Spend` inline per iteration. `spend` occurs exactly once in the file. That is `main`'s dead local, untouched by this PR. No competing unbreak is included here on purpose — six are already open against `main`, and duplicating one is how `main` gets re-broken. ## Note on the dead branch `unbreak-main-pipeline` still holds the duplicate-definition breakage and the only copy of #1982's merge. It is not reachable from `main` and its PR is closed, so nothing needs to be reverted — but it should not be revived without first taking `main`'s copies of `flip_halt_arming.rs`, `management_prompt/tests.rs` and `scope_stage.rs`, which is what this PR does. Filed as #2001. Closes #1787
9 tasks
macanderson
added a commit
that referenced
this pull request
Aug 7, 2026
…#1976) (#2036) ## What & why Three times a PR has landed on `main` that silently deleted code another PR added to the same file hours earlier, and CI could not see it. The most recent (fixed in #1975): #1951 rewrote `crates/stella-pipeline/src/pipeline/tests/verification_hardening.rs` from a pre-#1945 base, deleting the `PassingShell` double, `shell_call_result`, and the witness `a_revision_halts_at_the_step_where_the_tracked_test_flips` that #1945 had added hours earlier. It was caught **only by luck**: #1945 had also added a `mod flip_halt_arming;` line that survived the rewrite and referenced two of the deleted symbols, so `main` went red. Had #1945 added only the test and its doubles — no new module — the deletion would have compiled clean and silently removed a witness from the tree. *A witness that no longer exists cannot fail*, so nothing downstream would ever have reported it. Same shape on record for #1860 reverting #1836's forwarding in four crates. Both PRs are green against the `main` they branched from, and the merge is textually clean: git has no conflict to report, because one side simply does not contain the other's lines. ### The design decision that matters `scripts/check-deleted-tests.sh` compares the **base branch tip** against the **merge result** — never the PR's branch point. That pair is the whole guard. Comparing against the branch point would miss exactly this defect: a test added to `main` *after* the PR branched is absent from the branch point too, so its disappearance would look like nothing at all. Comparing main's tip against the merged tree asks the question that matters — "did everything main had survive this merge?" — and is quiet on a merely stale branch, because git merges main's own additions in unless the PR's side actively removed them. On a `pull_request` event the checkout is `refs/pull/N/merge`, so `HEAD^1` *is* the base branch tip. The guard needs no PR metadata, and because it compares two **trees** rather than two histories, `fetch-depth: 2` is sufficient — no full-depth clone. ### It asks for an acknowledgement, it does not forbid deletion A removed test is not automatically wrong — renames, folding into a table-driven case, and deliberate removal with the feature covered are all ordinary. So a removal fails **only while unnamed**: writing the test's name in the PR description (or a commit message) passes it. That mechanism is deliberately weak. The goal is not to adjudicate whether a deletion was correct — a script cannot — but to convert an invisible deletion into a sentence a reviewer reads. ### Deliberately NOT a `GATE_STEPS` entry It is the one question here about *two* trees, and a local `make gate` has no second tree to compare, so there is nothing for it to do there. This also means the five-edit gate-parity dance does not apply; `check-gate-parity` still reports 25 steps, unchanged. Closes #1976 ## The witness - [x] This PR includes a witness test (fails on `main`, passes here), **or** - [ ] No witness needed The witness is reproduced from **real history**, not a synthetic fixture — `eddf9700` is #1945 (added the witness), `2a142b26` is #1951 (deleted it): ``` $ PR_BODY="" ./scripts/check-deleted-tests.sh eddf970 2a142b2 check-deleted-tests: FAILED These tests exist in eddf970 but not in the merged tree, and nothing in the PR description or the branch's commit messages names them: a_revision_halts_at_the_step_where_the_tracked_test_flips ``` It names exactly the test that was really lost. The other three required behaviours, all verified: ``` $ PR_BODY="Folded a_revision_halts_at_the_step_where_the_tracked_test_flips into …" \ ./scripts/check-deleted-tests.sh eddf970 2a142b2 check-deleted-tests: OK — 1 removed test(s), each named in the PR description or a commit. $ ./scripts/check-deleted-tests.sh 1feb029 eddf970 # a range that only ADDS tests check-deleted-tests: OK — 6790 test(s) in 1feb029, none lost by the merge. $ ./scripts/check-deleted-tests.sh origin/main HEAD # this very branch check-deleted-tests: OK — 6865 test(s) in origin/main, none lost by the merge. ``` A genuine rename is reported too (the old name is gone), which is intended — naming it in the PR is the whole cost. ## The gate - [x] `shellcheck` — clean - [x] `make guards-fast` — all green, including `check-gate-parity` (25 steps, unchanged) and `check-action-pins` - [x] Docs updated: AGENTS.md § witness tests gains the paragraph, and the CI description now lists the guard among what `ci.yml` adds beyond the gate - [x] `Closes #1976` appears both here and as a commit trailer No Rust changed, so fmt/clippy/test are untouched by this PR. ## Nothing left behind - [x] Filed: see below Two limitations are **measured and documented in the script header** rather than left implicit: 1. **The key is the bare test name, unqualified by file or module.** That makes a test *moved* between modules silently fine (the common legitimate case), at the cost that a duplicated name masks a deletion. Measured: 51 of 6867 distinct test names (0.74%) are duplicated today, nearly all the per-adapter provider suites where one contract is asserted against each vendor under one name. The house style of long sentence-shaped test names is what makes the unqualified key work — it is not a general assumption. 2. **`#[test]` inside a multi-line string fixture is counted.** Two lines in the tree (`witness/density.rs`, `candidate_ws/witness_tools.rs` — code that analyses test code) put `#[test]` at the start of a continuation line inside a string literal. It is symmetric noise, so a difference detector cancels it; it could only false-positive if such a fixture were edited, which lands in the acknowledge path by design. ## Ground-rule check - [x] No I/O added to `stella-core`; no new deps - [x] No new outbound network calls ## Anything reviewers should know? `fetch-depth: 2` on the `check` job's checkout is the one change that touches every CI run. It is one extra commit, not a full clone — deliberately the smallest thing that makes the merge commit's first parent readable. The guard runs on `pull_request` only: on a squash-merged push to `main` there is no merge commit to inspect, and the report would arrive too late to act on anyway. The script self-skips on a non-merge `HEAD` with no explicit base, so running it by hand needs a base ref: `./scripts/check-deleted-tests.sh origin/main`. ## Summary by Sourcery Add a CI guard that detects tests removed by a PR’s merge unless their deletion is explicitly acknowledged, and document this behaviour in agent guidance. CI: - Update the main CI workflow to fetch two commits for pull_request checkouts and run a deleted-test guard that compares the base branch tip to the merge result, passing only when removed tests are named in the PR body. Documentation: - Extend AGENTS.md to describe the deleted-test guard, its scope, and its requirement to mention removed tests in PR descriptions. Chores: - Add the scripts/check-deleted-tests.sh utility to scan test trees across two revisions and enforce acknowledgement of removed tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
mainis red at 43402ae:cargo test -p stella-pipelinefails to compilewith three errors, and
cargo clippy -p stella-pipeline --all-targetsfails afourth. Until this lands every PR against
maininherits a red gate, so thisis deliberately narrow — no behavior change, nothing but the repairs.
Three independent parallel-merge collisions from the same round of merges.
None was visible to the CI of the PR that caused it, because each PR was green
against the
mainit branched from.1. Clobbered flip-halt test doubles (#1945 vs #1951)
#1945 added
mod flip_halt_arming;topipeline/tests/verification_hardening.rstogether with the two doublesthe child module uses —
PassingShellandshell_call_result— plus theconfigured-command witness
a_revision_halts_at_the_step_where_the_tracked_test_flips.#1951 then rewrote that file from a pre-#1945 base. It deleted all three, and
left the
mod flip_halt_arming;line standing — so the child module referencedtwo symbols that no longer existed:
Restored into
flip_halt_arming.rsitself rather than back into theparent, for two reasons:
verification_hardening.rsis 1434 lines against the1500 ceiling and cannot hold them, and the child module is their only user.
Both #1793 witnesses — the configured-command side and the authored-witness
side — now sit in one module, which is where a reader looking for "how is the
flip halt witnessed" would expect them.
2.
ModelCallRole::Researchnot covered (#1778)#1778 added the
Researchvariant.management_prompt/tests.rsholds adeliberately exhaustive
matchoverModelCallRole— its doc says a newrole "must declare its prefix posture here, not escape the family witness by
omission" — which #1778's CI never compiled against.
Researchruns as an engine sub-agent turn (research_stage.rsbuilds aSubAgentSpeccarryingRESEARCH_SYSTEM_PROMPT), never through the managementchokepoint — so it joins the never-dispatched arm returning
None. That is thesame grouping, with the same stated reasoning, that
raw_usage.rsalreadygives it. Added to
ALL_ROLESas well, since that array is documented as"every role the crate can dispatch".
3.
plan_stageover the argument limit (#1778)#1778 also added
research: &[ResearchFinding]toplan_stage, taking it to 8arguments against clippy's limit of 7. The gate runs clippy at
-D warnings.Fixed structurally rather than with an
#[allow]:budgetandtotalareexactly the pair the crate's own
Spendenvelope bundles(
pipeline/stage_budget.rs), they travel together everywhere else, andverifier()already took this shape in #1951.plan_stagenow takesSpend;plan_with_review— which uses the two for nothing but that call — builds onebefore its re-plan loop and reborrows it per iteration. 8 arguments become 7
and the meaning is unchanged.
The witness
Deliberately none, and this is the case the template's "explain how you
verified instead" is for: a repair to a broken build is witnessed by the
build. The evidence is that
cargo test -p stella-pipelinedoes not compile onmainand compiles here, which no test inside that crate could express — atest that cannot be built cannot fail informatively.
The two witnesses #1951 deleted are restored verbatim and do run again, so
#1793's coverage returns with this PR rather than being re-asserted by it.
The gate
Run on this branch, from the worktree:
cargo test -p stella-pipeline— 583 + 5 + 2 + 5 + 6 + 4 = 605 passed, 0failed (on
main: does not compile)cargo clippy -p stella-pipeline --all-targets— clean (onmain: 1 warning= 1 error under the gate's
-D warnings)cargo fmt --check -p stella-pipeline— cleancargo check --all-targetsonstella-protocol,stella-core,stella-tui,stella-serve,stella-cli— clean, confirmingstella-pipelinewas theonly crate the new enum variant broke
scripts/check-file-size.sh— OK, none grewscripts/check-god-files.sh— OKscripts/check-left-behind.sh— OK, 0 grandfatheredThe full workspace suite is left to CI.
Nothing left behind
Two follow-ups worth tracking, neither fixable inside an unbreak PR:
rewrites a test file from a stale base silently drops whatever landed
underneath it, and the only thing that caught it here was a
moddeclaration surviving. I will file an issue for it rather than leave the
observation in this description.
ALL_ROLESinmanagement_prompt/tests.rsis a hand-maintained array whoseown doc admits completeness "is not compiler-checked here". It stayed
correct this time only because the neighbouring
matchis exhaustive.Worth a
strum-style derive or a const assertion; also going in the issue.Summary by Sourcery
Repair stella-pipeline so it builds and passes clippy again by resolving recent merge collisions in tests, enum handling, and pipeline budgeting signatures.
Bug Fixes:
Tests: