From 5f63448329d403d6075d68b6772ca6c470870b05 Mon Sep 17 00:00:00 2001 From: Mac Anderson Date: Thu, 6 Aug 2026 18:46:48 -0700 Subject: [PATCH 1/2] =?UTF-8?q?fix(stella-pipeline):=20unbreak=20main=20?= =?UTF-8?q?=E2=80=94=20clobbered=20flip-halt=20doubles=20and=20an=20uncove?= =?UTF-8?q?red=20ModelCallRole?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit main was red at 43402ae4: 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. --- .../src/management_prompt/tests.rs | 9 +- .../flip_halt_arming.rs | 144 +++++++++++++++++- 2 files changed, 143 insertions(+), 10 deletions(-) diff --git a/crates/stella-pipeline/src/management_prompt/tests.rs b/crates/stella-pipeline/src/management_prompt/tests.rs index 43c80ac28..cf99311a9 100644 --- a/crates/stella-pipeline/src/management_prompt/tests.rs +++ b/crates/stella-pipeline/src/management_prompt/tests.rs @@ -36,9 +36,10 @@ const SHARED_MANAGEMENT_PREAMBLE: &str = ""; /// family. Completeness is not compiler-checked here — that job belongs to /// the exhaustive match in [`management_system_block`], which forces a new /// variant to declare its prefix posture before this array matters. -const ALL_ROLES: [ModelCallRole; 14] = [ +const ALL_ROLES: [ModelCallRole; 15] = [ ModelCallRole::Unknown, ModelCallRole::Triage, + ModelCallRole::Research, ModelCallRole::Plan, ModelCallRole::PlanRepair, ModelCallRole::WitnessAuthor, @@ -82,8 +83,12 @@ fn management_system_block(role: ModelCallRole) -> Option { // adopt the split these arms move to `Some(...)` and the roles join // the parity witness automatically. ModelCallRole::Plan | ModelCallRole::PlanRepair => None, - // Never dispatched through the management chokepoint. + // Never dispatched through the management chokepoint. `Research` + // (#1778) runs as an engine sub-agent turn, so its system prompt + // rides its `SubAgentSpec` rather than this family — the same + // grouping, for the same reason, that `raw_usage.rs` gives it. ModelCallRole::Unknown + | ModelCallRole::Research | ModelCallRole::WitnessAuthor | ModelCallRole::WitnessRepair | ModelCallRole::AgentAuthor diff --git a/crates/stella-pipeline/src/pipeline/tests/verification_hardening/flip_halt_arming.rs b/crates/stella-pipeline/src/pipeline/tests/verification_hardening/flip_halt_arming.rs index d4f9895bc..d1ad31e50 100644 --- a/crates/stella-pipeline/src/pipeline/tests/verification_hardening/flip_halt_arming.rs +++ b/crates/stella-pipeline/src/pipeline/tests/verification_hardening/flip_halt_arming.rs @@ -1,16 +1,144 @@ -//! FlipHalt arming on the authored-witness path (#1793). +//! FlipHalt arming in revise turns, on both paths (#1793). //! //! The mid-turn early stop used to be armed only from a configured -//! `--test-command` baseline, so on the authored-witness path — the default -//! for every run without a configured command — a revision kept running to -//! its step and loop caps after the witness had already flipped. This pins -//! the repair: `witness_on_demand` arms the latch the moment the witness's -//! failing baseline is credited into the oracle, and the revision receives -//! it (unfired) through the same `run_engine_turn` seam the execute turn -//! uses. +//! `--test-command` baseline, and `revise_turn` passed no latch at all. So on +//! the authored-witness path — the default for every run without a configured +//! command — a revision kept running to its step and loop caps after the +//! witness had already flipped, and even a configured-command run's revisions +//! did the same. This pins both repairs: `witness_on_demand` arms the latch +//! the moment the witness's failing baseline is credited into the oracle, and +//! a revision receives it (unfired) through the same `run_engine_turn` seam +//! the execute turn uses. +//! +//! Both witnesses and the two doubles they need live here rather than in the +//! parent: `verification_hardening.rs` is close enough to the 1500-line +//! ceiling that it cannot hold them, and these are the only users. use super::*; +/// A shell double whose every command "passes": the output carries the +/// trailing exit-0 marker [`crate::flip_halt::exit_status`] parses. What +/// [`EmptyTools`] can never express — a worker *observing* the tracked test +/// succeed through a tool result. +struct PassingShell; +#[async_trait] +impl ToolExecutor for PassingShell { + fn schemas(&self) -> Vec { + vec![ToolSchema { + name: "bash".into(), + description: "run a shell command".into(), + input_schema: serde_json::json!({ "type": "object" }), + read_only: false, + speculation_safe: false, + }] + } + async fn execute(&self, _name: &str, _input: &Value) -> ToolOutput { + ToolOutput::Ok { + content: "1 passed\n[exit code: 0]".into(), + } + } +} + +/// A completion that runs `command` through the shell — the observation the +/// flip halt correlates by `call_id` and scores against the tracked test. +fn shell_call_result(command: &str) -> CompletionResult { + CompletionResult { + tool_calls: vec![ToolCall { + call_id: format!("call-shell-{command}"), + name: "bash".into(), + input: serde_json::json!({ "command": command }), + }], + ..text_result("") + } +} + +/// #1793 witness (configured-command side): a revision that observes the +/// tracked test go fail→pass halts at that step boundary instead of running +/// on. The provider is scripted with steps BEYOND the flip; consuming them +/// is exactly the waste `flip_halt` exists to stop, so the call count is the +/// assertion. +#[tokio::test] +async fn a_revision_halts_at_the_step_where_the_tracked_test_flips() { + let provider = ScriptedProvider::new(vec![ + text_result("single"), + // Execute turn: acts, but the suite still fails afterwards. + text_result("done"), + // Revision, step 1: re-run the tracked test — it now passes. + shell_call_result("cargo test -p x"), + // Steps the revision would burn WITHOUT the halt. They must never be + // consumed: the goal was met at the step above. + shell_call_result("cargo test -p x"), + text_result("revision done"), + ]); + let resolver = OneProvider(&provider); + // Baseline fails (arms the halt), post-execute fails (forces the + // revision), post-revise passes (the flip), confirmation passes (#859). + let runner = ScriptedRunner::scripted( + vec![ + TestScript::Fail, + TestScript::Fail, + TestScript::Pass, + TestScript::Pass, + ], + "@@ -1 +1 @@\n-old\n+new", + ); + let tools = PassingShell; + let recall = NoContextRecall; + let repo = NoRepoStructure; + let repo_status = NoRepoStatus; + let approvals = AutoApproveGate; + let sleeper = NoopSleeper; + let router = router(); + let (tx, _rx) = mpsc::unbounded_channel(); + + let config = PipelineConfig { + test_command: Some("cargo test -p x".into()), + diff_diagnostic: Some(DiagnosticInvocation::GitDiff), + ..PipelineConfig::default() + }; + let pipeline = Pipeline::new( + PipelinePorts { + router: &router, + providers: &resolver, + tools: &tools, + recall: &recall, + repo: &repo, + repo_status: &repo_status, + touches: &NoFileTouches, + diagnostics: &runner, + tests: &runner, + lint: None, + mutation: None, + coverage: None, + approvals: &approvals, + sleeper: &sleeper, + hooks: None, + candidate_workspaces: None, + mcp_prefetch: None, + steering: None, + }, + tx, + config, + ); + + let mut messages = vec![CompletionMessage::system("sys")]; + let mut budget = BudgetGuard::new(BudgetMode::Off, None, None); + let outcome = pipeline + .run("Fix the failing test", &mut messages, &mut budget) + .await + .expect("run succeeds"); + + let verdict = outcome.verdict.expect("a verdict was produced"); + assert!(verdict.passed, "the flip was confirmed: {verdict:?}"); + assert_eq!( + provider.prompts().len(), + 3, + "triage, execute, one revision step — the revision must halt at the \ + boundary where the tracked test flipped, not spend the scripted \ + steps beyond it" + ); +} + /// #1793 witness (authored side): after `witness_on_demand` seeds a failing /// witness, a revision that observes the witness command pass halts at that /// step boundary. As in the configured-command twin, the provider is From 8d33e25752e53519749a4989f769666f561acbcd Mon Sep 17 00:00:00 2001 From: Mac Anderson Date: Thu, 6 Aug 2026 18:52:02 -0700 Subject: [PATCH 2/2] fix(stella-pipeline): keep plan_stage inside the argument limit after the research stage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/stella-pipeline/src/pipeline.rs | 11 +++++------ crates/stella-pipeline/src/pipeline/scope_stage.rs | 8 ++++++-- .../src/pipeline/tests/management_accounting.rs | 6 ++++-- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/crates/stella-pipeline/src/pipeline.rs b/crates/stella-pipeline/src/pipeline.rs index e4b6c598d..70a6c4f0c 100644 --- a/crates/stella-pipeline/src/pipeline.rs +++ b/crates/stella-pipeline/src/pipeline.rs @@ -1462,8 +1462,7 @@ impl<'a> Pipeline<'a> { research: &[ResearchFinding], repo_structure: &str, revision: Option<&str>, - budget: &mut BudgetGuard, - total: &mut f64, + spend: &mut Spend<'_>, ) -> Result, PipelineBudgetAbort> { self.emit(AgentEvent::Stage { name: StageKind::Plan, @@ -1491,8 +1490,8 @@ impl<'a> Pipeline<'a> { overrides: &worker_overrides, timeout: self.config.engine.model_timeout, }, - budget, - total, + spend.budget, + spend.total, ) .await { @@ -1516,8 +1515,8 @@ impl<'a> Pipeline<'a> { overrides: &worker_overrides, timeout: self.config.engine.model_timeout, }, - budget, - total, + spend.budget, + spend.total, ) .await { diff --git a/crates/stella-pipeline/src/pipeline/scope_stage.rs b/crates/stella-pipeline/src/pipeline/scope_stage.rs index 77d4e273e..b7ff27e65 100644 --- a/crates/stella-pipeline/src/pipeline/scope_stage.rs +++ b/crates/stella-pipeline/src/pipeline/scope_stage.rs @@ -31,6 +31,11 @@ impl Pipeline<'_> { let repo_structure = self.repo.structure_summary().await; let mut revision: Option = None; let mut spent_revisions = 0usize; + // Built once and reborrowed per re-plan: the two halves travel + // together everywhere else in the crate, and bundling them is what + // keeps `plan_stage` structurally inside the argument limit rather + // than behind an `#[allow]`. + let mut spend = Spend { budget, total }; loop { let plan = match self @@ -40,8 +45,7 @@ impl Pipeline<'_> { research, &repo_structure, revision.as_deref(), - budget, - total, + &mut spend, ) .await { diff --git a/crates/stella-pipeline/src/pipeline/tests/management_accounting.rs b/crates/stella-pipeline/src/pipeline/tests/management_accounting.rs index 7e03e3fd8..629902302 100644 --- a/crates/stella-pipeline/src/pipeline/tests/management_accounting.rs +++ b/crates/stella-pipeline/src/pipeline/tests/management_accounting.rs @@ -453,8 +453,10 @@ async fn a_late_plan_is_abandoned_and_falls_back_to_the_single_step_plan() { &[], "", None, - &mut budget, - &mut total, + &mut Spend { + budget: &mut budget, + total: &mut total, + }, ) .await .expect("a wedged planner is never a run-ending failure");