huskies: merge 1188 story Merge tool results return a summary, not the full gate log (35KB per call)

2026-07-17 11:58:37 +00:00
parent ea8e6edc94
commit dac0278218
8 changed files with 545 additions and 14 deletions
@@ -3,3 +3,145 @@
 //! Currently, the bulk of the merge I/O is handled by `crate::agents::merge`
 //! and `crate::io::story_metadata`. This file is the designated home for any
 //! future I/O helpers that are extracted from merge-related MCP handlers.
+
+use std::path::{Path, PathBuf};
+
+/// Directory (relative to the project root) where full untruncated merge
+/// reports are written, one file per merge attempt.
+const MERGE_REPORTS_DIR: &str = "merge_reports";
+
+/// Maximum number of report files retained per story; older files beyond
+/// this count are pruned after each write.
+const MAX_REPORTS_PER_STORY: usize = 5;
+
+/// Write the full untruncated merge output to
+/// `.huskies/merge_reports/{story_id}-{unix_timestamp}.log` and prune older
+/// reports for the same story beyond [`MAX_REPORTS_PER_STORY`].
+///
+/// Returns the path to the written file (relative to `project_root`) for
+/// inclusion in tool responses, or `None` if the write failed — a report is
+/// a convenience artifact, so a filesystem hiccup here must not fail the
+/// merge itself.
+pub fn write_merge_report(project_root: &Path, story_id: &str, output: &str) -> Option<String> {
+    let dir = project_root.join(".huskies").join(MERGE_REPORTS_DIR);
+    std::fs::create_dir_all(&dir).ok()?;
+
+    let timestamp = std::time::SystemTime::now()
+        .duration_since(std::time::UNIX_EPOCH)
+        .unwrap_or_default()
+        .as_secs();
+    let file_name = format!("{story_id}-{timestamp}.log");
+    std::fs::write(dir.join(&file_name), output).ok()?;
+
+    prune_old_reports(&dir, story_id);
+
+    Some(format!(".huskies/{MERGE_REPORTS_DIR}/{file_name}"))
+}
+
+/// Delete the oldest report files for `story_id` in `dir` beyond
+/// [`MAX_REPORTS_PER_STORY`]. Filenames embed a unix timestamp, so lexical
+/// sort order matches chronological order.
+fn prune_old_reports(dir: &Path, story_id: &str) {
+    let prefix = format!("{story_id}-");
+    let Ok(read_dir) = std::fs::read_dir(dir) else {
+        return;
+    };
+    let mut paths: Vec<PathBuf> = read_dir
+        .flatten()
+        .filter(|entry| {
+            entry
+                .file_name()
+                .to_str()
+                .is_some_and(|n| n.starts_with(&prefix) && n.ends_with(".log"))
+        })
+        .map(|entry| entry.path())
+        .collect();
+    paths.sort();
+
+    if paths.len() > MAX_REPORTS_PER_STORY {
+        for old in &paths[..paths.len() - MAX_REPORTS_PER_STORY] {
+            let _ = std::fs::remove_file(old);
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn write_merge_report_creates_file_with_full_output() {
+        let tmp = tempfile::tempdir().unwrap();
+        let output = "full untruncated gate output\nline 2\n";
+        let path = write_merge_report(tmp.path(), "42_story_foo", output)
+            .expect("write_merge_report should succeed");
+        assert!(path.starts_with(".huskies/merge_reports/42_story_foo-"));
+        assert!(path.ends_with(".log"));
+        let full_path = tmp.path().join(&path);
+        assert_eq!(std::fs::read_to_string(full_path).unwrap(), output);
+    }
+
+    #[test]
+    fn write_merge_report_prunes_beyond_max_per_story() {
+        let tmp = tempfile::tempdir().unwrap();
+        let dir = tmp.path().join(".huskies").join(MERGE_REPORTS_DIR);
+        std::fs::create_dir_all(&dir).unwrap();
+
+        // Seed MAX_REPORTS_PER_STORY + 2 files with distinct, ordered names
+        // (avoids relying on real-clock timestamps to establish ordering).
+        for i in 0..(MAX_REPORTS_PER_STORY + 2) {
+            std::fs::write(
+                dir.join(format!("77_story-{i:010}.log")),
+                format!("report {i}"),
+            )
+            .unwrap();
+        }
+
+        prune_old_reports(&dir, "77_story");
+
+        let remaining: Vec<String> = std::fs::read_dir(&dir)
+            .unwrap()
+            .flatten()
+            .map(|e| e.file_name().to_string_lossy().to_string())
+            .collect();
+        assert_eq!(remaining.len(), MAX_REPORTS_PER_STORY);
+        // The two oldest (lowest-numbered) files must be gone.
+        assert!(!remaining.contains(&"77_story-0000000000.log".to_string()));
+        assert!(!remaining.contains(&"77_story-0000000001.log".to_string()));
+        // The newest must survive.
+        assert!(remaining.contains(&format!("77_story-{:010}.log", MAX_REPORTS_PER_STORY + 1)));
+    }
+
+    #[test]
+    fn write_merge_report_prune_only_affects_matching_story() {
+        let tmp = tempfile::tempdir().unwrap();
+        let dir = tmp.path().join(".huskies").join(MERGE_REPORTS_DIR);
+        std::fs::create_dir_all(&dir).unwrap();
+        std::fs::write(dir.join("99_other-0000000000.log"), "keep me").unwrap();
+
+        for i in 0..(MAX_REPORTS_PER_STORY + 2) {
+            std::fs::write(
+                dir.join(format!("77_story-{i:010}.log")),
+                format!("report {i}"),
+            )
+            .unwrap();
+        }
+
+        prune_old_reports(&dir, "77_story");
+
+        assert!(
+            dir.join("99_other-0000000000.log").exists(),
+            "pruning one story's reports must not touch another story's files"
+        );
+    }
+
+    #[test]
+    fn write_merge_report_returns_none_when_project_root_is_unwritable_file() {
+        // Passing a path that is a regular file (not a directory) makes
+        // create_dir_all fail, exercising the graceful-`None` path.
+        let tmp = tempfile::tempdir().unwrap();
+        let not_a_dir = tmp.path().join("not_a_dir");
+        std::fs::write(&not_a_dir, "i am a file").unwrap();
+        assert!(write_merge_report(&not_a_dir, "1_story", "output").is_none());
+    }
+}
@@ -10,9 +10,13 @@
 pub mod io;
 /// Pure merge-status message formatting.
 pub mod status;
+/// Pure condensed gate/merge-failure summaries.
+pub mod summary;

 #[allow(unused_imports)]
 pub use status::format_merge_status_message;
+#[allow(unused_imports)]
+pub use summary::{summarize_merge_failure_kind, summarize_merge_result};

 // ── Error type ────────────────────────────────────────────────────────────────

@@ -57,6 +57,7 @@ mod tests {
            result,
            worktree_cleaned_up: false,
            story_archived: false,
+            report_path: None,
        }
    }

@@ -0,0 +1,239 @@
+//! Condensed summaries of gate/merge failure output for MCP tool responses
+//! and chat notifications.
+//!
+//! Full gate/test output can run into hundreds of KB; these pure functions
+//! reduce it to a bounded, human-scannable digest: the failing gate's name,
+//! parsed failing test names (when the output looks like a `cargo test`
+//! run), and the last [`SUMMARY_TAIL_LINES`] lines of raw output. No I/O.
+
+use crate::agents::gates::GateFailureKind;
+use crate::agents::merge::MergeResult;
+use crate::pipeline_state::MergeFailureKind;
+use crate::service::notifications::format::truncate_gate_output;
+
+/// Number of trailing output lines retained in a gate-failure summary.
+///
+/// Kept a few lines under `notifications::format::MERGE_FAILURE_TAIL_LINES`
+/// (30): chat notifications re-apply that cap on top of this summary's
+/// `Failing gate:`/`Failing tests:`/`--- output tail ---` header lines, and
+/// the total must stay at or under 30 lines so that second pass is a no-op
+/// instead of truncating the header off the front of an already-shaped
+/// summary.
+pub const SUMMARY_TAIL_LINES: usize = 25;
+
+/// Parse `cargo test` output for the list of failing test names.
+///
+/// libtest prints one or more `failures:` sections listing the fully
+/// qualified name of each failing test on its own indented line, terminated
+/// by a blank line or the `test result:` summary line. Returns names in
+/// first-seen order with duplicates removed (a name can repeat across the
+/// `failures:` section and the final summary re-listing).
+pub fn extract_failing_test_names(output: &str) -> Vec<String> {
+    let mut names = Vec::new();
+    let mut in_failures = false;
+    for line in output.lines() {
+        let trimmed = line.trim();
+        if trimmed == "failures:" {
+            in_failures = true;
+            continue;
+        }
+        if in_failures {
+            if trimmed.is_empty() || trimmed.starts_with("test result:") {
+                in_failures = false;
+                continue;
+            }
+            if !names.iter().any(|n: &String| n == trimmed) {
+                names.push(trimmed.to_string());
+            }
+        }
+    }
+    names
+}
+
+/// Human-readable label for the gate/step that produced a [`MergeResult`].
+fn failing_gate_label(result: &MergeResult) -> &'static str {
+    match result {
+        MergeResult::Success { .. } => "none",
+        MergeResult::Conflict { .. } => "git merge",
+        MergeResult::GateFailure { failure_kind, .. } => match failure_kind {
+            Some(GateFailureKind::Fmt) => "fmt",
+            Some(GateFailureKind::Lint) => "clippy",
+            Some(GateFailureKind::Test) => "tests",
+            Some(GateFailureKind::SourceMapCheck) => "source-map-check",
+            Some(GateFailureKind::ContentConflict) => "git merge",
+            Some(GateFailureKind::Build) => "build",
+            Some(GateFailureKind::Other) | None => "unknown",
+        },
+        MergeResult::NoCommits { .. } => "no commits",
+        MergeResult::Other { .. } => "unknown",
+    }
+}
+
+/// Human-readable label for the gate/step that produced a [`MergeFailureKind`].
+fn failing_gate_label_for_kind(kind: &MergeFailureKind) -> &'static str {
+    match kind {
+        MergeFailureKind::ConflictDetected(_) => "git merge",
+        MergeFailureKind::GatesFailed(_) => "quality gates",
+        MergeFailureKind::EmptyDiff => "empty diff",
+        MergeFailureKind::NoCommits => "no commits",
+        MergeFailureKind::Other(_) => "unknown",
+    }
+}
+
+/// Build a condensed summary: a `Failing gate:` line, an optional
+/// `Failing tests:` line (when any are parseable), and the last
+/// [`SUMMARY_TAIL_LINES`] lines of `output`.
+fn summarize(label: &str, output: &str) -> String {
+    let mut summary = format!("Failing gate: {label}\n");
+    let test_names = extract_failing_test_names(output);
+    if !test_names.is_empty() {
+        summary.push_str(&format!("Failing tests: {}\n", test_names.join(", ")));
+    }
+    summary.push_str("--- output tail ---\n");
+    summary.push_str(&truncate_gate_output(output, SUMMARY_TAIL_LINES));
+    summary
+}
+
+/// Summarize a completed [`MergeResult`] for MCP tool responses.
+///
+/// Returns an empty string for [`MergeResult::Success`] — successful merges
+/// carry no gate-failure summary.
+pub fn summarize_merge_result(result: &MergeResult) -> String {
+    if matches!(result, MergeResult::Success { .. }) {
+        return String::new();
+    }
+    summarize(failing_gate_label(result), result.output())
+}
+
+/// Summarize a [`MergeFailureKind`] for chat notifications, so the same
+/// bounded shaping (label + parsed test names + output tail) reaches the
+/// room instead of the raw gate/conflict output that `display_reason`
+/// embeds verbatim.
+pub fn summarize_merge_failure_kind(kind: &MergeFailureKind) -> String {
+    summarize(failing_gate_label_for_kind(kind), &kind.to_gate_output())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::agents::gates::GateFailureKind;
+
+    // ── extract_failing_test_names ────────────────────────────────────────────
+
+    #[test]
+    fn extract_failing_test_names_single_failures_section() {
+        let output = "running 3 tests\ntest foo::bar ... FAILED\ntest foo::baz ... ok\n\n\
+             failures:\n    foo::bar\n\ntest result: FAILED. 1 passed; 1 failed; 0 ignored\n";
+        assert_eq!(extract_failing_test_names(output), vec!["foo::bar"]);
+    }
+
+    #[test]
+    fn extract_failing_test_names_dedupes_across_sections() {
+        // cargo test prints the failing list once mid-output and again in the
+        // final "failures:" summary block; both must collapse to one entry.
+        let output = "failures:\n    foo::bar\n\nfailures:\n    foo::bar\n\ntest result: FAILED.\n";
+        assert_eq!(extract_failing_test_names(output), vec!["foo::bar"]);
+    }
+
+    #[test]
+    fn extract_failing_test_names_multiple_names() {
+        let output = "failures:\n    a::one\n    b::two\n\ntest result: FAILED.\n";
+        assert_eq!(extract_failing_test_names(output), vec!["a::one", "b::two"]);
+    }
+
+    #[test]
+    fn extract_failing_test_names_empty_when_no_failures_section() {
+        let output = "test result: ok. 3 passed; 0 failed\n";
+        assert!(extract_failing_test_names(output).is_empty());
+    }
+
+    #[test]
+    fn extract_failing_test_names_empty_on_build_error_output() {
+        let output = "error[E0063]: missing field `plan` in initializer of `Stage`\n";
+        assert!(extract_failing_test_names(output).is_empty());
+    }
+
+    // ── summarize_merge_result ─────────────────────────────────────────────────
+
+    #[test]
+    fn summarize_merge_result_success_is_empty() {
+        let result = MergeResult::Success {
+            conflicts_resolved: false,
+            conflict_details: None,
+            gate_output: "all good".to_string(),
+        };
+        assert_eq!(summarize_merge_result(&result), "");
+    }
+
+    #[test]
+    fn summarize_merge_result_build_error_names_the_gate() {
+        let result = MergeResult::GateFailure {
+            output: "error[E0063]: missing field `plan`".to_string(),
+            failure_kind: Some(GateFailureKind::Build),
+        };
+        let summary = summarize_merge_result(&result);
+        assert!(summary.starts_with("Failing gate: build\n"));
+        assert!(summary.contains("error[E0063]"));
+        assert!(!summary.contains("Failing tests:"));
+    }
+
+    #[test]
+    fn summarize_merge_result_test_failure_includes_names() {
+        let output = "running 1 test\ntest foo::bar ... FAILED\n\n\
+             failures:\n    foo::bar\n\ntest result: FAILED. 0 passed; 1 failed\n";
+        let result = MergeResult::GateFailure {
+            output: output.to_string(),
+            failure_kind: Some(GateFailureKind::Test),
+        };
+        let summary = summarize_merge_result(&result);
+        assert!(summary.starts_with("Failing gate: tests\n"));
+        assert!(summary.contains("Failing tests: foo::bar\n"));
+    }
+
+    #[test]
+    fn summarize_merge_result_conflict_names_git_merge() {
+        let result = MergeResult::Conflict {
+            details: Some("CONFLICT in src/lib.rs".to_string()),
+            output: "merge failed".to_string(),
+        };
+        let summary = summarize_merge_result(&result);
+        assert!(summary.starts_with("Failing gate: git merge\n"));
+        assert!(summary.contains("merge failed"));
+    }
+
+    #[test]
+    fn summarize_merge_result_truncates_long_output() {
+        let lines: Vec<String> = (1..=100).map(|i| format!("line{i}")).collect();
+        let output = lines.join("\n");
+        let result = MergeResult::GateFailure {
+            output,
+            failure_kind: Some(GateFailureKind::Test),
+        };
+        let summary = summarize_merge_result(&result);
+        assert!(summary.contains("output truncated"));
+        assert!(summary.contains("line100"));
+        assert!(
+            !summary.contains("line1\n"),
+            "line1 should have been dropped by the tail"
+        );
+    }
+
+    // ── summarize_merge_failure_kind ───────────────────────────────────────────
+
+    #[test]
+    fn summarize_merge_failure_kind_gates_failed() {
+        let kind = MergeFailureKind::GatesFailed("test result: FAILED. 1 failed".to_string());
+        let summary = summarize_merge_failure_kind(&kind);
+        assert!(summary.starts_with("Failing gate: quality gates\n"));
+        assert!(summary.contains("test result: FAILED"));
+    }
+
+    #[test]
+    fn summarize_merge_failure_kind_does_not_include_raw_prefix_sentence() {
+        // display_reason() would prefix "Quality gates failed: "; the summary
+        // must not duplicate that human-sentence framing.
+        let kind = MergeFailureKind::GatesFailed("boom".to_string());
+        let summary = summarize_merge_failure_kind(&kind);
+        assert!(!summary.contains("Quality gates failed:"));
+    }
+}