feat(delivery): record WHICH files a phase touched, not just how many
`capture_phase_diff_at` parsed `git diff --stat` down to three integers and
threw the filenames away. Nothing downstream could name a single file a coding
phase changed: the World can draw a coding station but nothing underneath it,
and an operator reading a mission sees "11 files" with no way to learn which.
A second `--name-status` call now records the paths into the code_diff metadata
and into `names.txt` beside `diffstat.txt`, so raw evidence survives
independently of the JSONB.
Three ways this could have been wrong, each guarded:
- Different revision or excludes from the `--stat` call would make
`files_changed` and the path list describe different diffs, with no way to
tell which lied. A source-walk test pins both to the same `base_sha` and
the same `excludes`.
- Running after `git reset --quiet` would drop newly CREATED files, since
`--intent-to-add` is what makes them visible to diff at all — and the stat
would still count them, so the list would look merely incomplete rather
than wrong. A test asserts the ordering.
- A rename is `R100\told\tnew` — three fields. Taking field two records where
the file USED to be, naming a path nobody can open, and the bug is
invisible in any repo where nothing was renamed. `changed_paths` is now
shared with auto_merge (which had the same parse) and takes the NEW path,
with tests for renames and copies.
The list is capped at 500 paths with `files_truncated` beside it: a cap that
silently clips is worse than no cap, because "touched 12 files" and "touched at
least 500" would look identical.
Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
f37c6b92d8
commit
cb8184e784
@@ -73,6 +73,10 @@ pub(crate) const EXCLUDED_PATHS: &[&str] = &[
|
||||
/// generated or vendored got committed), and the head of it is what an
|
||||
/// operator needs to see to work out what happened.
|
||||
const MAX_PATCH_BYTES: usize = 4 * 1024 * 1024;
|
||||
/// Cap on the recorded path list. A cap that silently truncates is worse than
|
||||
/// no cap, so the metadata carries `files_truncated` beside it — a reader must
|
||||
/// be able to tell "touched 12 files" from "touched at least 500".
|
||||
const MAX_CAPTURED_PATHS: usize = 500;
|
||||
|
||||
/// Who delivery commits as.
|
||||
///
|
||||
@@ -121,6 +125,13 @@ pub struct Capture {
|
||||
/// callers deciding anything on the strength of "no changes" must not.
|
||||
pub diff_error: Option<String>,
|
||||
pub truncated: bool,
|
||||
/// The paths this phase touched, with their `--name-status` letter. The
|
||||
/// diffstat gives counts only; this is what lets anything downstream say
|
||||
/// WHICH files changed.
|
||||
pub files: Vec<(char, String)>,
|
||||
/// The path list hit `MAX_CAPTURED_PATHS`. Recorded so a reader can tell a
|
||||
/// complete list from a clipped one.
|
||||
pub files_truncated: bool,
|
||||
pub patch_path: PathBuf,
|
||||
/// Set once the work has been committed to a mission branch.
|
||||
pub committed: Option<Commit>,
|
||||
@@ -274,11 +285,38 @@ pub async fn capture_phase_diff_at(
|
||||
}
|
||||
};
|
||||
|
||||
// The paths themselves, not just the counts.
|
||||
//
|
||||
// The diffstat gives three integers and throws the filenames away, so
|
||||
// nothing downstream could say WHICH files a phase touched — the World
|
||||
// could draw a "coding" station but nothing under it. Same `base_sha` and
|
||||
// the same excludes as the `--stat` call above: if the two disagreed,
|
||||
// `files_changed` and this list would contradict each other and nobody
|
||||
// could tell which one lied.
|
||||
//
|
||||
// Must run BEFORE the reset below — `--intent-to-add` is what makes newly
|
||||
// created files visible to diff at all.
|
||||
let mut name_args = vec!["diff", base_sha.as_str(), "--name-status", "--"];
|
||||
name_args.extend(excludes.iter().map(String::as_str));
|
||||
let name_status = match git(&repo, &name_args).await {
|
||||
Ok(s) => s,
|
||||
Err(e) => {
|
||||
note_diff_failure("name-status", e);
|
||||
String::new()
|
||||
}
|
||||
};
|
||||
|
||||
// Put the index back. `--intent-to-add` is a mutation of the agent's
|
||||
// workspace, and capture must not change what a later commit would see.
|
||||
let _ = git(&repo, &["reset", "--quiet"]).await;
|
||||
|
||||
let (files_changed, insertions, deletions) = parse_diffstat(&diffstat);
|
||||
// Shared with auto_merge so the two cannot disagree about what a
|
||||
// `--name-status` line means (renames are three fields; the NEW path is the
|
||||
// one that changed).
|
||||
let all_paths = crate::auto_merge::changed_paths(&name_status);
|
||||
let files_truncated = all_paths.len() > MAX_CAPTURED_PATHS;
|
||||
let files: Vec<(char, String)> = all_paths.into_iter().take(MAX_CAPTURED_PATHS).collect();
|
||||
let empty = patch.trim().is_empty();
|
||||
let truncated = patch.len() > MAX_PATCH_BYTES;
|
||||
let stored = if truncated {
|
||||
@@ -297,6 +335,9 @@ pub async fn capture_phase_diff_at(
|
||||
let patch_path = dir.join("diff.patch");
|
||||
std::fs::write(&patch_path, &stored)
|
||||
.map_err(|e| format!("write {}: {e}", patch_path.display()))?;
|
||||
// Raw evidence on disk, independent of the JSONB. When the metadata and
|
||||
// the picture disagree, this is the tiebreaker.
|
||||
let _ = std::fs::write(dir.join("names.txt"), &name_status);
|
||||
std::fs::write(dir.join("diffstat.txt"), &diffstat)
|
||||
.map_err(|e| format!("write diffstat: {e}"))?;
|
||||
|
||||
@@ -457,6 +498,13 @@ pub async fn capture_phase_diff_at(
|
||||
// did nothing" must check this first.
|
||||
"diff_error": diff_error,
|
||||
"truncated": truncated,
|
||||
// WHICH files, not just how many. Same base_sha and the same excludes
|
||||
// as `files_changed`, so the two describe the same diff.
|
||||
"files": files
|
||||
.iter()
|
||||
.map(|(st, path)| serde_json::json!({ "status": st.to_string(), "path": path }))
|
||||
.collect::<Vec<_>>(),
|
||||
"files_truncated": files_truncated,
|
||||
"excluded_paths": EXCLUDED_PATHS,
|
||||
});
|
||||
std::fs::write(
|
||||
@@ -508,6 +556,8 @@ pub async fn capture_phase_diff_at(
|
||||
empty,
|
||||
diff_error,
|
||||
truncated,
|
||||
files,
|
||||
files_truncated,
|
||||
patch_path,
|
||||
}))
|
||||
}
|
||||
@@ -1147,6 +1197,51 @@ fn untrusted_empty_reason(empty: bool, diff_error: Option<&str>) -> Option<Strin
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod changed_path_capture_tests {
|
||||
/// The path list and `files_changed` must describe the SAME diff.
|
||||
///
|
||||
/// They come from two separate git invocations — `--stat` and
|
||||
/// `--name-status`. If those are ever given different revisions or
|
||||
/// different exclude pathspecs, the count and the list disagree and there
|
||||
/// is no way to tell which is right: both look like plausible output.
|
||||
#[test]
|
||||
fn both_diff_calls_use_the_same_revision_and_excludes() {
|
||||
let src = include_str!("mission_delivery.rs");
|
||||
let body = src
|
||||
.split("let mut stat_args")
|
||||
.nth(1)
|
||||
.and_then(|s| s.split("let (files_changed").next())
|
||||
.expect("the capture block");
|
||||
assert!(
|
||||
body.contains("let mut name_args = vec![\"diff\", base_sha.as_str(), \"--name-status\", \"--\"]"),
|
||||
"the name-status call must use the same base_sha as --stat"
|
||||
);
|
||||
assert!(
|
||||
body.contains("name_args.extend(excludes.iter().map(String::as_str))"),
|
||||
"and the same excludes, or files_changed and the path list describe \
|
||||
different diffs"
|
||||
);
|
||||
}
|
||||
|
||||
/// `--name-status` must run before the index is put back, or newly created
|
||||
/// files — which `--intent-to-add` is what makes visible — vanish from the
|
||||
/// list while still being counted by the stat.
|
||||
#[test]
|
||||
fn paths_are_read_before_the_index_reset() {
|
||||
let src = include_str!("mission_delivery.rs");
|
||||
let name_at = src.find("--name-status").expect("name-status call");
|
||||
let reset_at = src
|
||||
.find("git(&repo, &[\"reset\", \"--quiet\"])")
|
||||
.expect("index reset");
|
||||
assert!(
|
||||
name_at < reset_at,
|
||||
"the path list must be captured while --intent-to-add is still in \
|
||||
effect, or created files are invisible to it"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
Reference in New Issue
Block a user