From 0d0e4ca82aa6e687e1bcfc2d0169b28f3193dd2f Mon Sep 17 00:00:00 2001 From: Mac Anderson Date: Thu, 6 Aug 2026 21:16:38 -0700 Subject: [PATCH 1/3] feat(stella-pipeline): let the verifier read an untracked file's content, not just its name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gather_diff` described every untracked file the turn created or modified with one marker line — the path and an added-line count — because `git diff` cannot see an unstaged file and the numstat probe was the only thing asked about it. For a task whose entire deliverable IS an untracked file, that made the verifier a reviewer of filenames: it could confirm something had been written and nothing about what. A real run graded such a change PASS and said so in as many words, that "the unseen content cannot itself justify a FAIL". Adds `DiagnosticInvocation::UntrackedPatch` beside the existing numstat — the same `git diff --no-index` probe minus `--numstat` — and appends its hunks under the marker they belong to. Two deliberate restrictions: - The patch's `diff --git`/`---`/`+++` preamble is stripped. `warrant::changed_paths` reads `+++ ` lines as the set of paths a change touched, and an untracked path already reaches it through the marker. Letting both channels name the file would move untracked-only changes off the `paths.is_empty()` branch that keeps them `Required`, quietly relaxing when a witness is owed. This change makes the verifier see more; it must not make the warrant ask for less. - Binary content stays git's `Binary files ... differ` sentence. That is the useful evidence, and it is what keeps a database sidecar's bytes out of a prompt. The verifier's system prompt asserted this content was unrenderable, which this makes false, so it moves in the same commit. --- crates/stella-cli/src/agent/tools.rs | 8 +++ crates/stella-pipeline/src/pipeline/tests.rs | 41 +++++++++++ .../src/pipeline/verify_probes.rs | 69 ++++++++++++++++++- crates/stella-pipeline/src/ports.rs | 16 +++++ crates/stella-pipeline/src/verify.rs | 6 +- crates/stella-serve/src/remote.rs | 4 ++ 6 files changed, 140 insertions(+), 4 deletions(-) diff --git a/crates/stella-cli/src/agent/tools.rs b/crates/stella-cli/src/agent/tools.rs index dcb555040..5274d0373 100644 --- a/crates/stella-cli/src/agent/tools.rs +++ b/crates/stella-cli/src/agent/tools.rs @@ -673,6 +673,14 @@ impl DiagnosticRunner for GitDiagnosticRunner { DiagnosticInvocation::UntrackedNumstat { path } => { cmd.args(["diff", "--no-index", "--numstat", "--", "/dev/null", path]); } + DiagnosticInvocation::UntrackedPatch { path } => { + // Same probe as the numstat above, minus `--numstat`, so the + // two answers about one untracked file can never disagree + // about which file they read. `--no-color` because a + // configured `color.ui = always` would otherwise paint SGR + // escapes into a prompt. + cmd.args(["diff", "--no-index", "--no-color", "--", "/dev/null", path]); + } } cmd.current_dir(&self.root).env("PWD", &self.root); for var in stella_tools::exec::GIT_REPO_ENV_VARS { diff --git a/crates/stella-pipeline/src/pipeline/tests.rs b/crates/stella-pipeline/src/pipeline/tests.rs index efd627d8e..f4747877b 100644 --- a/crates/stella-pipeline/src/pipeline/tests.rs +++ b/crates/stella-pipeline/src/pipeline/tests.rs @@ -328,6 +328,10 @@ pub(super) struct ScriptedRunner { pub(super) diff: String, /// Untracked files this workspace reports, as `(path, added_lines)`. untracked: Vec<(String, u32)>, + /// Content the `UntrackedPatch` probe serves, as `(path, file_body)`. A + /// path absent here answers with an empty probe, which is the shape a + /// host with no patch channel produces. + untracked_content: Vec<(String, String)>, /// What a failing run prints. Configurable so a test can plant a /// distinctive token and assert on where it does — and does not — travel. failure_tail: String, @@ -365,6 +369,7 @@ impl ScriptedRunner { test_runs: std::sync::atomic::AtomicU32::new(0), diff: diff.to_string(), untracked: Vec::new(), + untracked_content: Vec::new(), failure_tail: "test failed".to_string(), diff_exit_code: 0, diff_stderr: String::new(), @@ -401,6 +406,17 @@ impl ScriptedRunner { .collect(); self } + /// Script what the `UntrackedPatch` probe reads back for these paths, as + /// `(path, file_body)`. The runner wraps each body in the same header + /// preamble real `git diff --no-index` emits, so a test exercises the + /// stripping too rather than a pre-cleaned string. + pub(super) fn with_untracked_content(mut self, content: Vec<(&str, &str)>) -> Self { + self.untracked_content = content + .into_iter() + .map(|(p, body)| (p.to_string(), body.to_string())) + .collect(); + self + } /// #1539: script the availability probe — only these programs report /// usable. An empty vec models a workspace with no toolchain at all. pub(super) fn with_available_runners(mut self, programs: Vec<&str>) -> Self { @@ -411,6 +427,31 @@ impl ScriptedRunner { #[async_trait] impl DiagnosticRunner for ScriptedRunner { async fn run_diagnostic(&self, invocation: &DiagnosticInvocation) -> CmdOutcome { + if let DiagnosticInvocation::UntrackedPatch { path } = invocation { + // Real `git diff --no-index -- /dev/null ` output: the + // header preamble the pipeline must strip, then the hunk. + let patch = self + .untracked_content + .iter() + .find(|(candidate, _)| candidate == path) + .map(|(p, body)| { + let lines = body.lines().count(); + let added: String = + body.lines().map(|l| format!("+{l}\n")).collect::(); + format!( + "diff --git a/dev/null b/{p}\nnew file mode 100644\n\ + index 0000000..1111111\n--- /dev/null\n+++ b/{p}\n\ + @@ -0,0 +1,{lines} @@\n{added}" + ) + }) + .unwrap_or_default(); + return CmdOutcome { + exit_code: if patch.is_empty() { 0 } else { 1 }, + stdout_tail: patch, + stderr_tail: String::new(), + kind: CmdKind::Completed, + }; + } if let DiagnosticInvocation::UntrackedNumstat { path } = invocation { let numstat = self .untracked diff --git a/crates/stella-pipeline/src/pipeline/verify_probes.rs b/crates/stella-pipeline/src/pipeline/verify_probes.rs index fefe21479..45b2c5c31 100644 --- a/crates/stella-pipeline/src/pipeline/verify_probes.rs +++ b/crates/stella-pipeline/src/pipeline/verify_probes.rs @@ -238,6 +238,33 @@ impl<'a> Pipeline<'a> { .unwrap_or(1) } + /// The reviewable body of one untracked file's patch — its hunks, with + /// git's file headers removed. + /// + /// The headers are dropped rather than passed through because + /// [`crate::witness::warrant::changed_paths`] reads `+++ `/`--- ` lines to + /// decide which paths a change touched, and an untracked path already + /// reaches it through the marker line this body sits under. Letting both + /// channels name the file would move untracked-only changes off the + /// `paths.is_empty()` branch that keeps them + /// [`WitnessWarrant::Required`](crate::witness::warrant::WitnessWarrant), + /// turning "the verifier can now read the file" into a silent relaxation + /// of when a witness is owed. One change, one effect. + /// + /// A binary file keeps git's `Binary files … differ` sentence instead of + /// its bytes: that sentence is the useful evidence (there is nothing here + /// a reviewer can read), and it is also what stops a database sidecar + /// from pouring escape bytes into a prompt. + async fn untracked_patch_body(&self, surface: CandidateSurface<'_>, path: &str) -> String { + let out = surface + .diagnostics + .run_diagnostic(&DiagnosticInvocation::UntrackedPatch { + path: path.to_string(), + }) + .await; + patch_body(&out.stdout_tail) + } + /// Run the diff command and return `(changed_line_count, raw_diff)`. /// /// `git diff` cannot see untracked files, so a turn whose entire change is @@ -316,14 +343,18 @@ impl<'a> Pipeline<'a> { // the tail would let a large untracked change slip under a budget // it should have tripped. `buffered` preserves input order, so // the appended evidence stays deterministic. - let counted: Vec<(&str, u32)> = + let counted: Vec<(&str, u32, String)> = futures_util::stream::iter(fresh.into_iter().map(|path| async move { - (path, self.untracked_added_lines(surface, path).await) + let (added, body) = futures_util::join!( + self.untracked_added_lines(surface, path), + self.untracked_patch_body(surface, path), + ); + (path, added, body) })) .buffered(UNTRACKED_NUMSTAT_CONCURRENCY) .collect() .await; - for (path, added) in counted { + for (path, added, body) in counted { lines += added; // The shared builder, so `witness::warrant` can parse these // paths back out and hold them to the same path rules as @@ -331,6 +362,16 @@ impl<'a> Pipeline<'a> { // blinded the warrant to every untracked file. text.push('\n'); text.push_str(&crate::witness::warrant::untracked_change_line(path, added)); + // …and the content underneath it, so a verifier grades the + // change rather than the filename (#2027). The marker alone + // is a line count: a run whose entire deliverable was one + // untracked file was graded PASS by a verifier that said so + // in as many words — "the unseen content cannot itself + // justify a FAIL". + if !body.is_empty() { + text.push('\n'); + text.push_str(&body); + } } } DiffProbe { @@ -481,3 +522,25 @@ pub(super) struct DiffProbe { /// costs roughly one round-trip instead of N, low enough that a turn which /// creates hundreds cannot fork an unbounded burst of git processes. const UNTRACKED_NUMSTAT_CONCURRENCY: usize = 16; + +/// Reduce one `git diff --no-index -- /dev/null ` patch to the part a +/// reviewer can read: the hunks, or git's binary sentence. +/// +/// Everything above the first `@@` is git's file-header preamble +/// (`diff --git`, `new file mode`, `index`, `--- `, `+++ `), which +/// [`Pipeline::untracked_patch_body`] explains must not survive into the +/// diff text. An output with neither a hunk nor a binary sentence — an empty +/// probe, a failed one — contributes nothing rather than a confusing +/// fragment; the marker line above it still states the file changed. +fn patch_body(raw: &str) -> String { + if let Some(start) = raw.find("\n@@ ") { + return raw[start + 1..].trim_end().to_string(); + } + if raw.starts_with("@@ ") { + return raw.trim_end().to_string(); + } + raw.lines() + .find(|line| line.starts_with("Binary files ") && line.ends_with(" differ")) + .unwrap_or_default() + .to_string() +} diff --git a/crates/stella-pipeline/src/ports.rs b/crates/stella-pipeline/src/ports.rs index cf395055d..f4390e073 100644 --- a/crates/stella-pipeline/src/ports.rs +++ b/crates/stella-pipeline/src/ports.rs @@ -555,6 +555,22 @@ impl CmdOutcome { pub enum DiagnosticInvocation { GitDiff, UntrackedNumstat { path: String }, + /// The full patch for one untracked file — its *content*, not just its + /// shape. + /// + /// [`Self::UntrackedNumstat`] answers "how many lines", which is all the + /// diff-size budget and the zero-diff guard ever needed. A verifier needs + /// the other half: for a task whose entire deliverable IS an untracked + /// file, a marker naming the path and a line count is a review of a + /// filename. Graded that way, a verdict can only restate that something + /// was written — which is what one did, in as many words, before this + /// variant existed ("the unseen regex content cannot itself justify a + /// FAIL"). + /// + /// Git renders binary content as `Binary files ... differ`, so the bytes + /// of a database sidecar or a compiled artifact can never reach a prompt + /// through here. + UntrackedPatch { path: String }, } #[async_trait] diff --git a/crates/stella-pipeline/src/verify.rs b/crates/stella-pipeline/src/verify.rs index 4970395fe..3d4ba49c8 100644 --- a/crates/stella-pipeline/src/verify.rs +++ b/crates/stella-pipeline/src/verify.rs @@ -1191,7 +1191,11 @@ static VERIFIER_INSTRUCTIONS: LazyLock = LazyLock::new(|| { about the change's intent, and nothing else.\n\n\ Inside the diff, a line beginning with `{UNTRACKED_CHANGE_PREFIX}` is likewise a \ note from the pipeline, not a source line: it names a file the turn created or \ - modified outside version control's view, whose content no probe could render.\n\n\ + modified outside version control's view. The hunks below such a note are that \ + file's content, and are the change itself — review them as you would any other \ + file's. A note carrying `Binary files ... differ` instead, or standing alone, is \ + a file whose content could not be rendered; that is a channel saying nothing, \ + never evidence the file is empty or wrong.\n\n\ {DIFF_STAT_LINE_NOTE}" ) }); diff --git a/crates/stella-serve/src/remote.rs b/crates/stella-serve/src/remote.rs index b80e2fa6d..900d58b5a 100644 --- a/crates/stella-serve/src/remote.rs +++ b/crates/stella-serve/src/remote.rs @@ -875,6 +875,10 @@ impl DiagnosticRunner for RemoteVerificationRunner { "invocation": "untracked_numstat", "path": path, }), + DiagnosticInvocation::UntrackedPatch { path } => serde_json::json!({ + "invocation": "untracked_patch", + "path": path, + }), }; dispatch_verification_call( &self.frames, From b676dcf69339ff15aaa8298ab960662651873def Mon Sep 17 00:00:00 2001 From: Mac Anderson Date: Thu, 6 Aug 2026 21:24:15 -0700 Subject: [PATCH 2/3] test(stella-pipeline): witness that the verifier reads untracked content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fails on the previous behaviour with the whole defect visible in the assertion output — the verifier's diff section is one line, `+ untracked change: regex.txt (+1 lines)`, and nothing else. Asserts on the verifier's own prompt rather than the probe's return value: the latter would pass while the text was dropped anywhere between the probe and the model. The double lives in verification_honesty.rs rather than extending ScriptedRunner in pipeline/tests.rs, which is a god file sitting exactly at its 2537-line ceiling. ScriptedRunner's fallback arm already answers UntrackedPatch with empty stdout, so no existing scenario changes shape. --- crates/stella-pipeline/src/pipeline/tests.rs | 41 ------ .../pipeline/tests/verification_honesty.rs | 117 ++++++++++++++++++ .../src/pipeline/verify_probes.rs | 63 ++++++++++ crates/stella-pipeline/src/ports.rs | 8 +- 4 files changed, 186 insertions(+), 43 deletions(-) diff --git a/crates/stella-pipeline/src/pipeline/tests.rs b/crates/stella-pipeline/src/pipeline/tests.rs index f4747877b..efd627d8e 100644 --- a/crates/stella-pipeline/src/pipeline/tests.rs +++ b/crates/stella-pipeline/src/pipeline/tests.rs @@ -328,10 +328,6 @@ pub(super) struct ScriptedRunner { pub(super) diff: String, /// Untracked files this workspace reports, as `(path, added_lines)`. untracked: Vec<(String, u32)>, - /// Content the `UntrackedPatch` probe serves, as `(path, file_body)`. A - /// path absent here answers with an empty probe, which is the shape a - /// host with no patch channel produces. - untracked_content: Vec<(String, String)>, /// What a failing run prints. Configurable so a test can plant a /// distinctive token and assert on where it does — and does not — travel. failure_tail: String, @@ -369,7 +365,6 @@ impl ScriptedRunner { test_runs: std::sync::atomic::AtomicU32::new(0), diff: diff.to_string(), untracked: Vec::new(), - untracked_content: Vec::new(), failure_tail: "test failed".to_string(), diff_exit_code: 0, diff_stderr: String::new(), @@ -406,17 +401,6 @@ impl ScriptedRunner { .collect(); self } - /// Script what the `UntrackedPatch` probe reads back for these paths, as - /// `(path, file_body)`. The runner wraps each body in the same header - /// preamble real `git diff --no-index` emits, so a test exercises the - /// stripping too rather than a pre-cleaned string. - pub(super) fn with_untracked_content(mut self, content: Vec<(&str, &str)>) -> Self { - self.untracked_content = content - .into_iter() - .map(|(p, body)| (p.to_string(), body.to_string())) - .collect(); - self - } /// #1539: script the availability probe — only these programs report /// usable. An empty vec models a workspace with no toolchain at all. pub(super) fn with_available_runners(mut self, programs: Vec<&str>) -> Self { @@ -427,31 +411,6 @@ impl ScriptedRunner { #[async_trait] impl DiagnosticRunner for ScriptedRunner { async fn run_diagnostic(&self, invocation: &DiagnosticInvocation) -> CmdOutcome { - if let DiagnosticInvocation::UntrackedPatch { path } = invocation { - // Real `git diff --no-index -- /dev/null ` output: the - // header preamble the pipeline must strip, then the hunk. - let patch = self - .untracked_content - .iter() - .find(|(candidate, _)| candidate == path) - .map(|(p, body)| { - let lines = body.lines().count(); - let added: String = - body.lines().map(|l| format!("+{l}\n")).collect::(); - format!( - "diff --git a/dev/null b/{p}\nnew file mode 100644\n\ - index 0000000..1111111\n--- /dev/null\n+++ b/{p}\n\ - @@ -0,0 +1,{lines} @@\n{added}" - ) - }) - .unwrap_or_default(); - return CmdOutcome { - exit_code: if patch.is_empty() { 0 } else { 1 }, - stdout_tail: patch, - stderr_tail: String::new(), - kind: CmdKind::Completed, - }; - } if let DiagnosticInvocation::UntrackedNumstat { path } = invocation { let numstat = self .untracked diff --git a/crates/stella-pipeline/src/pipeline/tests/verification_honesty.rs b/crates/stella-pipeline/src/pipeline/tests/verification_honesty.rs index fa6294566..513d9187c 100644 --- a/crates/stella-pipeline/src/pipeline/tests/verification_honesty.rs +++ b/crates/stella-pipeline/src/pipeline/tests/verification_honesty.rs @@ -40,3 +40,120 @@ fn a_real_diff_passes_through_unchanged() { assert_eq!(verification_honest_diff(diff.clone(), 0), diff); assert_eq!(verification_honest_diff(diff.clone(), 5), diff); } + +use super::*; + +/// The regex the fixture task writes. Distinctive so an assertion that finds +/// it in a prompt cannot be finding something else. +const REGEX: &str = r"^\d{4}-\d{2}-\d{2}$"; + +/// A diagnostics double for the untracked-only shape: `git diff` sees nothing +/// (the file was never staged), the numstat probe counts one added line, and +/// the patch probe serves the file under the header preamble real git emits. +struct PatchRunner; + +#[async_trait] +impl DiagnosticRunner for PatchRunner { + async fn run_diagnostic(&self, invocation: &DiagnosticInvocation) -> CmdOutcome { + let stdout = match invocation { + DiagnosticInvocation::GitDiff => String::new(), + DiagnosticInvocation::UntrackedNumstat { .. } => "1\t0\tregex.txt".to_string(), + DiagnosticInvocation::UntrackedPatch { .. } => format!( + "diff --git a/dev/null b/regex.txt\nnew file mode 100644\n\ + index 0000000..1111111\n--- /dev/null\n+++ b/regex.txt\n\ + @@ -0,0 +1 @@\n+{REGEX}\n" + ), + }; + CmdOutcome { + exit_code: 0, + stdout_tail: stdout, + stderr_tail: String::new(), + kind: CmdKind::Completed, + } + } +} + +/// The witness: a task whose entire deliverable is an untracked file must +/// reach the verifier as its CONTENT, not as its name. +/// +/// Before the `UntrackedPatch` probe, `gather_diff` described such a file with +/// one marker line — the path and an added-line count — because `git diff` +/// cannot see an unstaged file and nothing else was asked about it. A real run +/// of exactly this shape returned PASS reasoning that "the unseen content +/// cannot itself justify a FAIL": the model was grading a filename. +/// +/// Asserting on the verifier's own prompt is the point. An assertion on the +/// probe's return value would still pass if the text were dropped anywhere +/// between the probe and the model. +#[tokio::test] +async fn the_verifier_reads_an_untracked_files_content_not_just_its_name() { + let provider = ScriptedProvider::new(vec![ + text_result("CLASS: single\nWITNESS: no\nVERIFIER: yes"), + text_result("Wrote the regex to regex.txt."), + text_result("PASS the regex handles the stated cases"), + ]); + let resolver = OneProvider(&provider); + let runner = PatchRunner; + let tests = ScriptedRunner::new(vec![], ""); + let tools = EmptyTools; + let recall = NoContextRecall; + let repo = NoRepoStructure; + // Empty before the turn, carrying the new file after it — the fingerprint + // delta that makes `gather_diff` bill the file to this turn. + let repo_status = SeqRepoStatus::new(vec![vec![], vec![("regex.txt", "sha256:a")]]); + let approvals = AutoApproveGate; + let sleeper = NoopSleeper; + let router = router(); + let (tx, _rx) = mpsc::unbounded_channel(); + + let pipeline = Pipeline::new( + PipelinePorts { + router: &router, + providers: &resolver, + tools: &tools, + recall: &recall, + repo: &repo, + repo_status: &repo_status, + touches: &NoFileTouches, + diagnostics: &runner, + tests: &tests, + lint: None, + mutation: None, + coverage: None, + approvals: &approvals, + sleeper: &sleeper, + hooks: None, + candidate_workspaces: None, + mcp_prefetch: None, + steering: None, + }, + tx, + PipelineConfig { + test_command: None, + diff_diagnostic: Some(DiagnosticInvocation::GitDiff), + witness_writer: false, + ..PipelineConfig::default() + }, + ); + + let mut messages = vec![CompletionMessage::system("sys")]; + let mut budget = BudgetGuard::new(BudgetMode::Off, None, None); + pipeline + .run("Write a regex to regex.txt", &mut messages, &mut budget) + .await + .expect("run completes"); + + let verifier_prompt = provider + .prompts() + .into_iter() + .find(|p| p.contains("independent code reviewer")) + .expect("the verifier was asked"); + assert!( + verifier_prompt.contains(REGEX), + "the verifier graded the change without ever seeing it: {verifier_prompt}" + ); + assert!( + verifier_prompt.contains("untracked change: regex.txt"), + "the marker still names the file the content belongs to: {verifier_prompt}" + ); +} diff --git a/crates/stella-pipeline/src/pipeline/verify_probes.rs b/crates/stella-pipeline/src/pipeline/verify_probes.rs index 45b2c5c31..342e01ba3 100644 --- a/crates/stella-pipeline/src/pipeline/verify_probes.rs +++ b/crates/stella-pipeline/src/pipeline/verify_probes.rs @@ -544,3 +544,66 @@ fn patch_body(raw: &str) -> String { .unwrap_or_default() .to_string() } + +#[cfg(test)] +mod patch_body_tests { + use super::patch_body; + + /// Exactly what `git diff --no-index -- /dev/null ` prints for a + /// newly created text file. + const CREATED: &str = "diff --git a/dev/null b/regex.txt\n\ + new file mode 100644\n\ + index 0000000..9f2c1d3\n\ + --- /dev/null\n\ + +++ b/regex.txt\n\ + @@ -0,0 +1,2 @@\n\ + +^\\d{4}-\\d{2}-\\d{2}$\n\ + +second line\n"; + + #[test] + fn the_content_survives() { + let body = patch_body(CREATED); + assert!(body.starts_with("@@ -0,0 +1,2 @@"), "{body}"); + assert!(body.contains("+^\\d{4}-\\d{2}-\\d{2}$"), "{body}"); + assert!(body.contains("+second line"), "{body}"); + } + + /// The restriction that keeps this change from also relaxing the warrant: + /// no `+++ `/`--- ` line may survive, because + /// [`crate::witness::warrant::changed_paths`] reads those as the set of + /// paths a change touched, and the marker line already names this one. + #[test] + fn no_header_survives_for_changed_paths_to_read() { + let body = patch_body(CREATED); + assert!( + crate::witness::warrant::changed_paths(&body).is_empty(), + "the body named a path to the warrant: {body}" + ); + for line in body.lines() { + assert!(!line.starts_with("+++ "), "{line}"); + assert!(!line.starts_with("--- "), "{line}"); + assert!(!line.starts_with("diff --git"), "{line}"); + } + } + + /// A database sidecar is the case that must never spill bytes into a + /// prompt — git says so itself, and that sentence is the evidence. + #[test] + fn binary_content_is_reported_but_never_rendered() { + let raw = "diff --git a/dev/null b/.stella/private/store.db\n\ + Binary files /dev/null and b/.stella/private/store.db differ\n"; + assert_eq!( + patch_body(raw), + "Binary files /dev/null and b/.stella/private/store.db differ" + ); + } + + /// A probe that failed, or a host with no patch channel, contributes + /// nothing rather than a fragment. The marker line above it still states + /// the file changed. + #[test] + fn an_unreadable_probe_contributes_nothing() { + assert_eq!(patch_body(""), ""); + assert_eq!(patch_body("fatal: not a git repository\n"), ""); + } +} diff --git a/crates/stella-pipeline/src/ports.rs b/crates/stella-pipeline/src/ports.rs index f4390e073..2878c2e76 100644 --- a/crates/stella-pipeline/src/ports.rs +++ b/crates/stella-pipeline/src/ports.rs @@ -554,7 +554,9 @@ impl CmdOutcome { #[derive(Debug, Clone, PartialEq, Eq)] pub enum DiagnosticInvocation { GitDiff, - UntrackedNumstat { path: String }, + UntrackedNumstat { + path: String, + }, /// The full patch for one untracked file — its *content*, not just its /// shape. /// @@ -570,7 +572,9 @@ pub enum DiagnosticInvocation { /// Git renders binary content as `Binary files ... differ`, so the bytes /// of a database sidecar or a compiled artifact can never reach a prompt /// through here. - UntrackedPatch { path: String }, + UntrackedPatch { + path: String, + }, } #[async_trait] From 656821c0834c2da25004ea142f00c426e73788b8 Mon Sep 17 00:00:00 2001 From: Mac Anderson Date: Thu, 6 Aug 2026 21:30:23 -0700 Subject: [PATCH 3/3] refactor(stella-cli): extract the Git diagnostic argv mapping into its own module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `UntrackedPatch` arm pushed `agent/tools.rs` to 1504 lines, over the 1500-line limit, and that file is not grandfathered — so the arm had to land somewhere else rather than the ceiling move. `impl DiagnosticRunner for GitDiagnosticRunner` is the natural seam: it is the one place a `DiagnosticInvocation` becomes a process, and it needs nothing from `tools` but the runner's root, its baseline commit, and the two spawn helpers. Moving it takes tools.rs to 1470. `agent.rs` grows by the one line that declares the module — the irreducible case the baseline documents an escape hatch for, so the +1 lands as a visible baseline diff. Regenerating also ratchets command_deck.rs (-55) and bus.rs (-235) down to what main already earned. --- crates/stella-cli/src/agent.rs | 1 + crates/stella-cli/src/agent/diagnostics.rs | 50 ++++++++++++++++++++++ crates/stella-cli/src/agent/tools.rs | 40 ++--------------- scripts/file-size-baseline.txt | 6 +-- 4 files changed, 57 insertions(+), 40 deletions(-) create mode 100644 crates/stella-cli/src/agent/diagnostics.rs diff --git a/crates/stella-cli/src/agent.rs b/crates/stella-cli/src/agent.rs index 35043850a..49ea12263 100644 --- a/crates/stella-cli/src/agent.rs +++ b/crates/stella-cli/src/agent.rs @@ -47,6 +47,7 @@ use crate::{OutputFormat, config::Config, resume_frame}; use stella_context::EpisodeOutcome; mod coverage; +mod diagnostics; mod engine; mod goal; mod graph; diff --git a/crates/stella-cli/src/agent/diagnostics.rs b/crates/stella-cli/src/agent/diagnostics.rs new file mode 100644 index 000000000..ba9e1c373 --- /dev/null +++ b/crates/stella-cli/src/agent/diagnostics.rs @@ -0,0 +1,50 @@ +//! The closed Git diagnostic vocabulary, as argv. +//! +//! Split out of [`super::tools`], which sits against the file-size limit, and +//! coherent on its own terms: this is the one place a +//! [`DiagnosticInvocation`] becomes a process. The runner's state and +//! construction stay beside the rest of the workspace ports in `tools`; only +//! the mapping lives here. +//! +//! Every variant maps to fixed argv. Paths are literal arguments and no shell +//! is involved, which is the property that lets the pipeline hand this +//! model-supplied path without sanitizing it. + +use async_trait::async_trait; +use stella_pipeline::{CmdOutcome, DiagnosticInvocation, DiagnosticRunner}; + +use super::tools::{GitDiagnosticRunner, run_command, scrub_model_subprocess}; + +#[async_trait] +impl DiagnosticRunner for GitDiagnosticRunner { + async fn run_diagnostic(&self, invocation: &DiagnosticInvocation) -> CmdOutcome { + let mut cmd = tokio::process::Command::new("git"); + scrub_model_subprocess(&mut cmd); + match invocation { + DiagnosticInvocation::GitDiff => match self.baseline_commit() { + Some(baseline) => { + cmd.args(["diff", baseline]); + } + None => { + cmd.args(["diff"]); + } + }, + DiagnosticInvocation::UntrackedNumstat { path } => { + cmd.args(["diff", "--no-index", "--numstat", "--", "/dev/null", path]); + } + DiagnosticInvocation::UntrackedPatch { path } => { + // The same probe as the numstat above, minus `--numstat`, so + // the two answers about one untracked file can never disagree + // about which file they read. `--no-color` because a + // configured `color.ui = always` would otherwise paint SGR + // escapes into a verifier's prompt. + cmd.args(["diff", "--no-index", "--no-color", "--", "/dev/null", path]); + } + } + cmd.current_dir(&self.root).env("PWD", &self.root); + for var in stella_tools::exec::GIT_REPO_ENV_VARS { + cmd.env_remove(var); + } + run_command(cmd).await + } +} diff --git a/crates/stella-cli/src/agent/tools.rs b/crates/stella-cli/src/agent/tools.rs index 5274d0373..e1a35c3e9 100644 --- a/crates/stella-cli/src/agent/tools.rs +++ b/crates/stella-cli/src/agent/tools.rs @@ -15,7 +15,7 @@ use stella_pipeline::{ /// Apply the cross-crate policy shared by every model/repository-controlled /// subprocess. Kept as a named seam so the CLI's pipeline-only spawns have a /// direct regression test rather than relying only on stella-tools tests. -fn scrub_model_subprocess(command: &mut tokio::process::Command) { +pub(super) fn scrub_model_subprocess(command: &mut tokio::process::Command) { stella_tools::subprocess_env::scrub_sensitive_env(command); } @@ -596,7 +596,7 @@ impl GitDiagnosticRunner { Self { root, baseline } } - fn baseline_commit(&self) -> Option<&str> { + pub(super) fn baseline_commit(&self) -> Option<&str> { self.baseline.as_deref() } } @@ -656,41 +656,7 @@ fn test_process(invocation: &TestInvocation, root: &std::path::Path) -> tokio::p cmd } -#[async_trait::async_trait] -impl DiagnosticRunner for GitDiagnosticRunner { - async fn run_diagnostic(&self, invocation: &DiagnosticInvocation) -> CmdOutcome { - let mut cmd = tokio::process::Command::new("git"); - scrub_model_subprocess(&mut cmd); - match invocation { - DiagnosticInvocation::GitDiff => match self.baseline_commit() { - Some(baseline) => { - cmd.args(["diff", baseline]); - } - None => { - cmd.args(["diff"]); - } - }, - DiagnosticInvocation::UntrackedNumstat { path } => { - cmd.args(["diff", "--no-index", "--numstat", "--", "/dev/null", path]); - } - DiagnosticInvocation::UntrackedPatch { path } => { - // Same probe as the numstat above, minus `--numstat`, so the - // two answers about one untracked file can never disagree - // about which file they read. `--no-color` because a - // configured `color.ui = always` would otherwise paint SGR - // escapes into a prompt. - cmd.args(["diff", "--no-index", "--no-color", "--", "/dev/null", path]); - } - } - cmd.current_dir(&self.root).env("PWD", &self.root); - for var in stella_tools::exec::GIT_REPO_ENV_VARS { - cmd.env_remove(var); - } - run_command(cmd).await - } -} - -async fn run_command(mut cmd: tokio::process::Command) -> CmdOutcome { +pub(super) async fn run_command(mut cmd: tokio::process::Command) -> CmdOutcome { cmd.stdout(std::process::Stdio::piped()); cmd.stderr(std::process::Stdio::piped()); // Cancellation drops this future without unwinding into the timeout arm diff --git a/scripts/file-size-baseline.txt b/scripts/file-size-baseline.txt index 1196660a6..ddaa7c7b2 100644 --- a/scripts/file-size-baseline.txt +++ b/scripts/file-size-baseline.txt @@ -15,11 +15,11 @@ 1911 bench/terminal_bench_analysis/tb21_evidence_contract.py 4334 bench/terminal_bench_analysis/tests/test_tb21_analysis.py 1659 bench/terminal_bench_analysis/tests/test_tb21_evidence_contract.py -2269 crates/stella-cli/src/agent.rs +2270 crates/stella-cli/src/agent.rs 1752 crates/stella-cli/src/agent/tests.rs -4621 crates/stella-cli/src/command_deck.rs +4566 crates/stella-cli/src/command_deck.rs 1507 crates/stella-cli/src/fleet_cmd.rs -2126 crates/stella-core/src/bus.rs +1891 crates/stella-core/src/bus.rs 2572 crates/stella-core/src/driver.rs 3681 crates/stella-core/src/driver/tests.rs 1781 crates/stella-model/src/anthropic/tests.rs