Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions crates/stella-cli/src/agent/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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:?}"
);
}
}
64 changes: 59 additions & 5 deletions crates/stella-pipeline/src/pipeline/authored.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment thread
vercel[bot] marked this conversation as resolved.
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)]
Expand Down Expand Up @@ -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}");
}
Expand All @@ -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");
Expand Down
38 changes: 32 additions & 6 deletions crates/stella-pipeline/src/pipeline/verify_probes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -363,21 +366,28 @@ 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());
}
}
}
}
DiffProbe {
lines,
text,
available: true,
untracked_rendered,
}
}

Expand Down Expand Up @@ -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,
);
}
Expand Down Expand Up @@ -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<String>,
}

/// How many `git diff --no-index --numstat` probes [`Pipeline::gather_diff`]
Expand Down
Loading