Skip to content

Commit 57618e8

Browse files
feat(check): surface specific violations with unified diff output (#278)
* feat(check): surface specific violations with unified diff output Replace the opaque 'does not match' error with a unified diff showing exactly which lines differ between the on-disk pipeline and the freshly compiled output. This makes it immediately clear what changed and why recompilation is needed. Changes: - Add the 'similar' crate for line-level diffing - Replace normalize_whitespace comparison with clean_generated_yaml canonicalization on both sides (preserves indentation and internal spaces, unlike the old approach which stripped all whitespace) - Show a unified diff with 3 lines of context on check failure - Truncate diff output after 80 changed lines for large pipelines - Warn when the pipeline was compiled by a different compiler version - Add tests for format_diff and clean_generated_yaml Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(check): correct truncation counter and total change count in diff output - Move changed_lines_shown increment after the truncation check so the count matches what is actually printed (was off-by-one: 81 vs 80). - Compute total_added/total_removed in a first pass over all changes so the truncation message reports the real total, not just lines seen before the break. - Use total counts in the Summary line so it stays accurate even when the diff is truncated. - Add test_format_diff_truncates_at_limit covering the truncation path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(check): buffer hunk lines to prevent orphaned headers on truncation - Buffer each hunk's rendered lines before emitting the @@ header, so mid-hunk truncation doesn't leave a malformed hunk with mismatched line counts - Consolidate two iter_all_changes() passes into a single fold for total_added/total_removed counting Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent be3b4c5 commit 57618e8

3 files changed

Lines changed: 216 additions & 34 deletions

File tree

Cargo.lock

Lines changed: 20 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ subtle = "2.6.1"
3333
rand = "0.10.1"
3434
base64 = "0.22.1"
3535
glob-match = "0.2.1"
36+
similar = "3.1.0"
3637

3738
[dev-dependencies]
3839
reqwest = { version = "0.12", features = ["blocking"] }

src/compile/mod.rs

Lines changed: 195 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -202,8 +202,9 @@ pub async fn compile_all_pipelines(skip_integrity: bool, debug_pipeline: bool) -
202202
/// Check that a compiled pipeline YAML matches its source markdown.
203203
///
204204
/// Reads the `@ado-aw` header from `pipeline_path` to discover the source
205-
/// markdown file, compiles it fresh, and compares (whitespace-normalized)
206-
/// against the existing pipeline file. Returns an error if they differ.
205+
/// markdown file, compiles it fresh, and compares the canonicalized output
206+
/// against the existing pipeline file. When they differ, a unified diff is
207+
/// printed to stderr showing exactly which lines changed.
207208
pub async fn check_pipeline(pipeline_path: &str) -> Result<()> {
208209
let pipeline_path = Path::new(pipeline_path);
209210

@@ -228,6 +229,16 @@ pub async fn check_pipeline(pipeline_path: &str) -> Result<()> {
228229
)
229230
})?;
230231

232+
// Warn if the pipeline was generated by a different compiler version
233+
let current_version = env!("CARGO_PKG_VERSION");
234+
if !header_meta.version.is_empty() && header_meta.version != current_version {
235+
eprintln!(
236+
"Warning: pipeline was generated by ado-aw v{}, current version is v{}. \
237+
Version differences may cause expected changes in the output.",
238+
header_meta.version, current_version
239+
);
240+
}
241+
231242
// The header stores the source path relative to the repository root.
232243
// Walk up from the pipeline file to find the .git directory, then resolve
233244
// the source path relative to that root.
@@ -286,13 +297,19 @@ pub async fn check_pipeline(pipeline_path: &str) -> Result<()> {
286297
false,
287298
)
288299
.await?;
289-
let pipeline_yaml = clean_generated_yaml(&pipeline_yaml);
290300

291-
// Compare ignoring whitespace differences
292-
if normalize_whitespace(&pipeline_yaml) != normalize_whitespace(&existing) {
301+
// Canonicalize both sides: trim trailing whitespace, collapse blank lines,
302+
// but preserve indentation and internal spaces for meaningful comparison.
303+
let expected = clean_generated_yaml(&pipeline_yaml);
304+
let existing = clean_generated_yaml(&existing);
305+
306+
if expected != existing {
307+
let diff_output = format_diff(&existing, &expected, pipeline_path);
308+
eprintln!("{}", diff_output);
309+
293310
anyhow::bail!(
294311
"Integrity check failed: generated pipeline for '{}' does not match {}. \
295-
Re-run compilation to update the pipeline file.",
312+
Re-run `ado-aw compile` to update the pipeline file.",
296313
front_matter.name,
297314
pipeline_path.display()
298315
);
@@ -302,6 +319,96 @@ pub async fn check_pipeline(pipeline_path: &str) -> Result<()> {
302319
Ok(())
303320
}
304321

322+
/// Maximum number of changed lines to display in the diff output.
323+
/// Keeps terminal output manageable for large pipeline files.
324+
const MAX_DIFF_CHANGED_LINES: usize = 80;
325+
326+
/// Format a unified-style diff between the existing and expected pipeline content.
327+
///
328+
/// Shows changed lines with surrounding context and a summary of the total
329+
/// number of changes. Output is truncated after [`MAX_DIFF_CHANGED_LINES`]
330+
/// changed lines to avoid overwhelming the terminal.
331+
fn format_diff(existing: &str, expected: &str, pipeline_path: &Path) -> String {
332+
use similar::{ChangeTag, TextDiff};
333+
334+
let diff = TextDiff::from_lines(existing, expected);
335+
let mut output = String::new();
336+
337+
output.push_str(&format!(
338+
"\n--- {} (on disk)\n+++ {} (expected from source)\n",
339+
pipeline_path.display(),
340+
pipeline_path.display(),
341+
));
342+
343+
// First pass: count total changes across the full diff.
344+
let (total_added, total_removed) = diff.iter_all_changes().fold((0usize, 0usize), |(a, r), c| {
345+
match c.tag() {
346+
ChangeTag::Insert => (a + 1, r),
347+
ChangeTag::Delete => (a, r + 1),
348+
ChangeTag::Equal => (a, r),
349+
}
350+
});
351+
352+
let mut changed_lines_shown = 0usize;
353+
let mut truncated = false;
354+
355+
for hunk in diff.unified_diff().context_radius(3).iter_hunks() {
356+
if truncated {
357+
break;
358+
}
359+
360+
// Buffer hunk lines so we only emit the header if we have content to show.
361+
// This avoids orphaned hunk headers when truncation fires mid-hunk.
362+
let mut hunk_buf = String::new();
363+
364+
for change in hunk.iter_changes() {
365+
let tag = change.tag();
366+
let line = change.value();
367+
368+
if tag != ChangeTag::Equal {
369+
if changed_lines_shown >= MAX_DIFF_CHANGED_LINES {
370+
truncated = true;
371+
break;
372+
}
373+
changed_lines_shown += 1;
374+
}
375+
376+
let prefix = match tag {
377+
ChangeTag::Delete => "-",
378+
ChangeTag::Insert => "+",
379+
ChangeTag::Equal => " ",
380+
};
381+
// Lines from TextDiff include trailing newlines; write directly.
382+
if line.ends_with('\n') {
383+
hunk_buf.push_str(&format!("{}{}", prefix, line));
384+
} else {
385+
hunk_buf.push_str(&format!("{}{}\n", prefix, line));
386+
}
387+
}
388+
389+
if !hunk_buf.is_empty() {
390+
output.push_str(&format!("{}\n", hunk.header()));
391+
output.push_str(&hunk_buf);
392+
}
393+
}
394+
395+
if truncated {
396+
output.push_str(&format!(
397+
"\n... diff truncated after {} changed lines (showing {} of {} total changes)\n",
398+
MAX_DIFF_CHANGED_LINES,
399+
changed_lines_shown,
400+
total_added + total_removed,
401+
));
402+
}
403+
404+
output.push_str(&format!(
405+
"\nSummary: {} line(s) added, {} line(s) removed\n",
406+
total_added, total_removed
407+
));
408+
409+
output
410+
}
411+
305412
/// Walk up from `start` to find the nearest directory containing `.git`.
306413
fn find_repo_root(start: &Path) -> Option<PathBuf> {
307414
let mut current = start.to_path_buf();
@@ -315,14 +422,6 @@ fn find_repo_root(start: &Path) -> Option<PathBuf> {
315422
}
316423
}
317424

318-
/// Normalize a string by removing all whitespace characters.
319-
///
320-
/// Used for integrity checks so that formatting-only differences
321-
/// (trailing spaces, blank lines, indentation changes) are ignored.
322-
fn normalize_whitespace(s: &str) -> String {
323-
s.chars().filter(|c| !c.is_whitespace()).collect()
324-
}
325-
326425
/// Clean up spacing artifacts in generated YAML.
327426
///
328427
/// After template placeholder replacement, empty placeholders leave behind
@@ -483,37 +582,99 @@ Body
483582
}
484583

485584
#[test]
486-
fn test_normalize_whitespace_strips_all_whitespace() {
487-
assert_eq!(normalize_whitespace("a b c"), "abc");
488-
assert_eq!(normalize_whitespace("a\n b\n c\n"), "abc");
489-
assert_eq!(normalize_whitespace(" hello world "), "helloworld");
585+
fn test_clean_generated_yaml_strips_trailing_whitespace() {
586+
let a = clean_generated_yaml("key: value\nother: data\n");
587+
let b = clean_generated_yaml("key: value \nother: data \n");
588+
assert_eq!(a, b);
589+
}
590+
591+
#[test]
592+
fn test_clean_generated_yaml_collapses_blank_lines() {
593+
// Consecutive blank lines collapse to a single blank line
594+
let a = clean_generated_yaml("key: value\n\nother: data\n");
595+
let b = clean_generated_yaml("key: value\n\n\nother: data\n\n");
596+
assert_eq!(a, b);
597+
}
598+
599+
#[test]
600+
fn test_clean_generated_yaml_preserves_indentation() {
601+
let input = "steps:\n - bash: echo hello\n displayName: greet\n";
602+
let cleaned = clean_generated_yaml(input);
603+
assert!(cleaned.contains(" - bash: echo hello"));
604+
assert!(cleaned.contains(" displayName: greet"));
490605
}
491606

492607
#[test]
493-
fn test_normalize_whitespace_identical_content_matches() {
494-
let a = "key: value\nother: data\n";
495-
let b = "key: value\nother: data\n";
496-
assert_eq!(normalize_whitespace(a), normalize_whitespace(b));
608+
fn test_clean_generated_yaml_preserves_internal_spaces() {
609+
let input = "script: echo a b c\n";
610+
let cleaned = clean_generated_yaml(input);
611+
assert!(cleaned.contains("echo a b c"));
497612
}
498613

499614
#[test]
500-
fn test_normalize_whitespace_ignores_trailing_spaces() {
501-
let a = "key: value\nother: data\n";
502-
let b = "key: value \nother: data \n";
503-
assert_eq!(normalize_whitespace(a), normalize_whitespace(b));
615+
fn test_format_diff_shows_added_lines() {
616+
let existing = "line1\nline2\n";
617+
let expected = "line1\nline2\nline3\n";
618+
let diff = format_diff(existing, expected, Path::new("test.yml"));
619+
assert!(diff.contains("+line3"));
620+
assert!(diff.contains("1 line(s) added"));
504621
}
505622

506623
#[test]
507-
fn test_normalize_whitespace_ignores_blank_lines() {
508-
let a = "key: value\nother: data\n";
509-
let b = "key: value\n\n\nother: data\n\n";
510-
assert_eq!(normalize_whitespace(a), normalize_whitespace(b));
624+
fn test_format_diff_shows_removed_lines() {
625+
let existing = "line1\nline2\nline3\n";
626+
let expected = "line1\nline3\n";
627+
let diff = format_diff(existing, expected, Path::new("test.yml"));
628+
assert!(diff.contains("-line2"));
629+
assert!(diff.contains("1 line(s) removed"));
511630
}
512631

513632
#[test]
514-
fn test_normalize_whitespace_detects_content_difference() {
515-
let a = "key: value1\n";
516-
let b = "key: value2\n";
517-
assert_ne!(normalize_whitespace(a), normalize_whitespace(b));
633+
fn test_format_diff_shows_changed_lines() {
634+
let existing = "key: old_value\nother: data\n";
635+
let expected = "key: new_value\nother: data\n";
636+
let diff = format_diff(existing, expected, Path::new("test.yml"));
637+
assert!(diff.contains("-key: old_value"));
638+
assert!(diff.contains("+key: new_value"));
639+
assert!(diff.contains("1 line(s) added, 1 line(s) removed"));
640+
}
641+
642+
#[test]
643+
fn test_format_diff_identical_produces_no_hunks() {
644+
let content = "line1\nline2\n";
645+
let diff = format_diff(content, content, Path::new("test.yml"));
646+
assert!(diff.contains("0 line(s) added, 0 line(s) removed"));
647+
assert!(!diff.contains("@@"));
648+
}
649+
650+
#[test]
651+
fn test_format_diff_includes_file_path() {
652+
let diff = format_diff("a\n", "b\n", Path::new("my-pipeline.yml"));
653+
assert!(diff.contains("my-pipeline.yml (on disk)"));
654+
assert!(diff.contains("my-pipeline.yml (expected from source)"));
655+
}
656+
657+
#[test]
658+
fn test_format_diff_truncates_at_limit() {
659+
// 100 unique old lines replaced by 100 unique new lines = 200 total changes.
660+
let existing: String = (0..100).map(|i| format!("old{}\n", i)).collect();
661+
let expected: String = (0..100).map(|i| format!("new{}\n", i)).collect();
662+
let diff = format_diff(&existing, &expected, Path::new("test.yml"));
663+
assert!(
664+
diff.contains("... diff truncated"),
665+
"diff should be truncated for >80 changed lines"
666+
);
667+
// Exactly 80 changed lines shown, 200 total changes (100 removed + 100 added).
668+
assert!(
669+
diff.contains("showing 80 of 200 total changes"),
670+
"truncation message should report 80 shown of 200 total, got:\n{}",
671+
diff
672+
);
673+
// Summary should reflect ALL changes, not just the shown ones.
674+
assert!(
675+
diff.contains("100 line(s) added, 100 line(s) removed"),
676+
"summary should report full totals, got:\n{}",
677+
diff
678+
);
518679
}
519680
}

0 commit comments

Comments
 (0)