diff --git a/crates/stella-cli/src/agent/diagnostics.rs b/crates/stella-cli/src/agent/diagnostics.rs index ba9e1c37..208f5eb2 100644 --- a/crates/stella-cli/src/agent/diagnostics.rs +++ b/crates/stella-cli/src/agent/diagnostics.rs @@ -48,3 +48,66 @@ impl DiagnosticRunner for GitDiagnosticRunner { run_command(cmd).await } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Real git, not a double. + /// + /// `stella-pipeline`'s `patch_body` strips everything above the first `@@` + /// and keeps git's binary sentence verbatim. Both rules are assertions + /// about what *this* argv prints, and every test of them over there runs + /// against a scripted string — so without this, the two could drift and + /// only a live run would notice. + #[tokio::test] + async fn an_untracked_text_file_yields_a_hunk_under_a_strippable_preamble() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("regex.txt"), "^\\d{4}-\\d{2}-\\d{2}$\n").unwrap(); + let runner = GitDiagnosticRunner::new(dir.path().to_path_buf()); + + let out = runner + .run_diagnostic(&DiagnosticInvocation::UntrackedPatch { + path: "regex.txt".to_string(), + }) + .await; + + // The preamble `patch_body` drops... + assert!(out.stdout_tail.contains("+++ b/regex.txt"), "{out:?}"); + // ...sits above the first hunk, so dropping it keeps the content. + let hunk = out.stdout_tail.find("\n@@ ").expect("a hunk header"); + let header = out.stdout_tail.find("+++ b/").expect("the preamble"); + assert!(header < hunk, "{out:?}"); + assert!( + out.stdout_tail[hunk..].contains("+^\\d{4}-\\d{2}-\\d{2}$"), + "the content survives below the hunk header: {out:?}" + ); + } + + /// The bytes of a database sidecar must never reach a prompt. Git decides + /// that for us — this pins that it still does, and in the wording + /// `patch_body` matches on. + #[tokio::test] + async fn an_untracked_binary_file_yields_gits_sentence_and_no_bytes() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("store.db"), [0u8, 159, 146, 150, 0, 1, 2]).unwrap(); + let runner = GitDiagnosticRunner::new(dir.path().to_path_buf()); + + let out = runner + .run_diagnostic(&DiagnosticInvocation::UntrackedPatch { + path: "store.db".to_string(), + }) + .await; + + let sentence = out + .stdout_tail + .lines() + .find(|l| l.starts_with("Binary files ") && l.ends_with(" differ")) + .unwrap_or_else(|| panic!("git did not report a binary file: {out:?}")); + assert!(sentence.contains("store.db"), "{sentence}"); + assert!( + !out.stdout_tail.contains("@@ "), + "a binary file must produce no hunk to render: {out:?}" + ); + } +} diff --git a/crates/stella-pipeline/src/pipeline/authored.rs b/crates/stella-pipeline/src/pipeline/authored.rs index 42a2cb33..ff84281f 100644 --- a/crates/stella-pipeline/src/pipeline/authored.rs +++ b/crates/stella-pipeline/src/pipeline/authored.rs @@ -37,17 +37,36 @@ const AUTHORED_SECTION_HEADER: &str = /// legal: every consumer downstream (`changed_paths`, `changed_lines`, /// `mutants_from_diff`) parses the joined string with one parser and cannot /// tell which half a hunk came from, which is the point. +/// `already_rendered` names the untracked paths the probe half now carries the +/// CONTENT of, not merely a marker for. Those chunks are dropped from the +/// authored half: both halves would otherwise render the same file, spending a +/// token budget twice to say one thing, and leaving a verifier to wonder +/// whether it is looking at one change or two. The probe's copy is the one +/// kept, by the same precedence this module already applies — on-disk state is +/// the stronger claim about what survived. +/// +/// This is the text-side analogue of the `max` (never a sum) that +/// `absorb_probe` applies to the two channels' line counts, and it exists for +/// the same reason: they are two views of one change, not two changes. pub(super) fn splice_authored( probe_text: String, authored: &crate::ports::AuthoredChange, + already_rendered: &[String], ) -> String { if authored.is_empty() { return probe_text; } + // Reuses the verifier-prompt chunk dropper rather than a second parser: + // one definition of "a chunk for this path" keeps the two callers from + // disagreeing about what a chunk boundary is. + let authored_text = crate::verify::strip_witness_hunks(&authored.text, already_rendered).diff; + if authored_text.trim().is_empty() { + return probe_text; + } if probe_text.trim().is_empty() { - return authored.text.clone(); + return authored_text; } - format!("{probe_text}\n{AUTHORED_SECTION_HEADER}\n{}", authored.text) + format!("{probe_text}\n{AUTHORED_SECTION_HEADER}\n{authored_text}") } #[cfg(test)] @@ -76,16 +95,51 @@ mod tests { fn an_empty_authored_change_leaves_the_probe_text_untouched() { let probe = "diff --git a/x b/x\n".to_string(); assert_eq!( - splice_authored(probe.clone(), &AuthoredChange::default()), + splice_authored(probe.clone(), &AuthoredChange::default(), &[]), probe ); } + /// The duplication guard: once the probe carries an untracked file's + /// content, the authored channel's copy of that same file is redundant and + /// must be dropped. Both halves rendering it would spend the diff budget + /// twice on one change and leave a verifier reading the same file twice. + #[test] + fn a_file_the_probe_already_rendered_is_not_repeated_by_the_authored_half() { + let probe = "+ untracked change: solution.py (+2 lines)\n\ + @@ -0,0 +1,2 @@\n+def ok(n):\n+ return n >= 2"; + let spliced = splice_authored( + probe.to_string(), + &authored_create(), + &["solution.py".to_string()], + ); + assert_eq!(spliced, probe, "the authored half added nothing: {spliced}"); + assert_eq!( + spliced.matches("def ok(n):").count(), + 1, + "the content appears exactly once: {spliced}" + ); + } + + /// …and the drop is per path. A file only the tools saw is still spliced, + /// which is the whole reason the authored channel exists. + #[test] + fn a_file_the_probe_did_not_render_is_still_spliced() { + let probe = "+ untracked change: other.py (+1 lines)\n@@ -0,0 +1 @@\n+x = 1"; + let spliced = splice_authored( + probe.to_string(), + &authored_create(), + &["other.py".to_string()], + ); + assert!(spliced.contains(AUTHORED_SECTION_HEADER), "{spliced}"); + assert!(spliced.contains("def ok(n):"), "{spliced}"); + } + /// The Terminal-Bench case: the probe could not look, so the authored diff is /// the entire answer and must not be buried under a joining header. #[test] fn a_blank_probe_yields_the_authored_diff_alone() { - let spliced = splice_authored(String::new(), &authored_create()); + let spliced = splice_authored(String::new(), &authored_create(), &[]); assert!(spliced.starts_with("--- /dev/null"), "{spliced}"); assert!(!spliced.contains(AUTHORED_SECTION_HEADER), "{spliced}"); } @@ -96,7 +150,7 @@ mod tests { #[test] fn both_channels_are_joined_with_the_tree_first() { let probe = "diff --git a/lib.rs b/lib.rs\n--- a/lib.rs\n+++ b/lib.rs\n@@ -1,1 +1,1 @@\n-old\n+new\n"; - let spliced = splice_authored(probe.to_string(), &authored_create()); + let spliced = splice_authored(probe.to_string(), &authored_create(), &[]); let tree = spliced.find("a/lib.rs").expect("tree half"); let header = spliced.find(AUTHORED_SECTION_HEADER).expect("the label"); let authored = spliced.find("b/solution.py").expect("authored half"); diff --git a/crates/stella-pipeline/src/pipeline/verify_probes.rs b/crates/stella-pipeline/src/pipeline/verify_probes.rs index 342e01ba..7d59b4b9 100644 --- a/crates/stella-pipeline/src/pipeline/verify_probes.rs +++ b/crates/stella-pipeline/src/pipeline/verify_probes.rs @@ -288,6 +288,7 @@ impl<'a> Pipeline<'a> { lines: 0, text: String::new(), available: false, + untracked_rendered: Vec::new(), }; }; let out = surface.diagnostics.run_diagnostic(diagnostic).await; @@ -318,10 +319,12 @@ impl<'a> Pipeline<'a> { DIFF_PROBE_FAILED.to_string() }, available: false, + untracked_rendered: Vec::new(), }; } let mut lines = count_diff_lines(&out.stdout_tail); let mut text = out.stdout_tail; + let mut untracked_rendered = Vec::new(); if matches!(diagnostic, DiagnosticInvocation::GitDiff) { let after = surface.repo_status.untracked_fingerprints().await; // Created (absent before) OR modified (fingerprint changed) this @@ -363,14 +366,20 @@ impl<'a> Pipeline<'a> { 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". + // change rather than the filename. 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". + // + // A hunk body is the file's content and makes the authored + // channel's copy redundant; git's binary sentence is not, so + // only the former claims the path. if !body.is_empty() { text.push('\n'); text.push_str(&body); + if body.starts_with("@@ ") { + untracked_rendered.push(path.to_string()); + } } } } @@ -378,6 +387,7 @@ impl<'a> Pipeline<'a> { lines, text, available: true, + untracked_rendered, } } @@ -442,7 +452,7 @@ impl<'a> Pipeline<'a> { // asserting that the tree was successfully re-read. state.diff_available = probe.available; state.diff_text = verification_honest_diff( - authored::splice_authored(probe.text, &authored), + authored::splice_authored(probe.text, &authored, &probe.untracked_rendered), state.signals.file_changes, ); } @@ -515,6 +525,22 @@ pub(super) struct DiffProbe { /// Whether the probe could read the working tree AT ALL. Never `false` /// merely because the diff came back empty. pub(super) available: bool, + /// Untracked paths whose CONTENT this probe's text already carries. + /// + /// The authored channel renders the same content for any file written + /// through the file tools, and the two halves are concatenated — so + /// without this, a tool-written untracked file reached the verifier + /// twice, spending a token budget twice to say one thing. Naming the + /// paths here lets [`super::authored::splice_authored`] drop the + /// redundant half, and keeps the choice of *which* half explicit rather + /// than inferred by re-parsing the text. + /// + /// The probe's copy is the one kept, for the reason `authored`'s module + /// docs already give: on-disk state is the stronger claim about what + /// survived the turn. A path whose body could not be rendered (binary, + /// failed probe) is deliberately absent, so the authored channel still + /// covers it. + pub(super) untracked_rendered: Vec, } /// How many `git diff --no-index --numstat` probes [`Pipeline::gather_diff`]