fix(compile): address PR review feedback on migration framework

Four findings from the Rust PR Reviewer bot:

1. check_pipeline's bail message had a redundant "error:" prefix that
   produced "Error: error: ..." once anyhow's top-level handler added
   its own "Error:" wrapper. Drop the prefix and reformat the hint
   onto an indented continuation line.

2. compile_pipeline_inner did a redundant serde_yaml::from_value
   round-trip just to satisfy ParsedSource.front_matter, even though
   perform_source_rewrite_if_needed never reads that field. Refactor
   the helper to take the four primitive fields it actually uses
   (mapping, body_raw, source_sha256, migrations) instead of the full
   struct. reconstruct_source likewise takes individual fields now.

3. Misleading comment in tests/migration_tests.rs claimed a
   "thread-local counter" was used; only a nanosecond timestamp was.
   Replaced rand_suffix() with timestamp + AtomicU64 monotonic seq so
   parallel tests scheduled in the same nanosecond always get
   distinct dirs.

4. Leading whitespace before the opening `---` was silently stripped
   on migration rewrite. parse_markdown_detailed already tolerates it
   on read; capture it as a new ParsedSource.leading_whitespace field
   and emit it from reconstruct_source so byte-faithful preservation
   extends to whitespace prefixes (BOM-stripped editor blank lines,
   etc.).

All 1369 tests still pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: jamesadevine

src/compile/common.rs

@@ -102,6 +102,10 @@ pub struct ParsedSource {
     /// The migrated front-matter mapping, with `schema-version`
     /// bumped. Used to reconstruct the source for rewrite.
     pub front_matter_mapping: serde_yaml::Mapping,
+    /// Whitespace bytes that appeared before the opening `---` fence,
+    /// preserved verbatim so that source rewrite is byte-faithful.
+    /// Empty in the typical case where the file starts with `---`.
+    pub leading_whitespace: String,
     /// The body region exactly as it appeared after the closing `---`,
     /// byte-for-byte (no trim). Includes any leading newline.
     pub body_raw: String,
@@ -138,8 +142,12 @@ pub(crate) fn parse_markdown_detailed_with_registry(
 
     // Allow leading whitespace before the opening fence (preserves
     // historical leniency). We compute a byte offset into `content` so
-    // that `body_raw` extraction is purely byte-faithful.
+    // that `body_raw` extraction is purely byte-faithful, and we keep
+    // the whitespace prefix around so that source rewrites preserve
+    // anything the user (or their editor) put before the opening
+    // fence.
     let leading_ws = content.bytes().take_while(|b| b.is_ascii_whitespace()).count();
+    let leading_whitespace = content[..leading_ws].to_string();
     let after_lead = &content[leading_ws..];
     if !after_lead.starts_with("---") {
         anyhow::bail!("Markdown file must start with YAML front matter (---)");
@@ -184,23 +192,37 @@ pub(crate) fn parse_markdown_detailed_with_registry(
         markdown_body,
         migrations: report,
         front_matter_mapping: mapping,
+        leading_whitespace,
         body_raw,
         source_sha256,
     })
 }
 
-/// Reconstruct full source content from a migrated [`ParsedSource`].
+/// Reconstruct full source content from migration outputs.
+///
+/// Takes the individual fragments rather than the full
+/// [`ParsedSource`] so callers that have already destructured the
+/// parse don't have to round-trip a fresh `front_matter` through
+/// serde just to satisfy the typed field.
 ///
 /// Output shape:
+/// - `leading_whitespace` (typically empty)
 /// - `---\n`
 /// - the migrated YAML mapping (`serde_yaml::to_string` always ends
 ///   with `\n`)
 /// - `---`
 /// - the original body region byte-for-byte (`body_raw`)
-pub fn reconstruct_source(parsed: &ParsedSource) -> Result<String> {
-    let yaml_serialized = serde_yaml::to_string(&parsed.front_matter_mapping)
+pub fn reconstruct_source(
+    leading_whitespace: &str,
+    front_matter_mapping: &serde_yaml::Mapping,
+    body_raw: &str,
+) -> Result<String> {
+    let yaml_serialized = serde_yaml::to_string(front_matter_mapping)
         .context("Failed to serialize migrated front matter")?;
-    Ok(format!("---\n{}---{}", yaml_serialized, parsed.body_raw))
+    Ok(format!(
+        "{}---\n{}---{}",
+        leading_whitespace, yaml_serialized, body_raw
+    ))
 }
 
 fn yaml_value_kind(v: &serde_yaml::Value) -> &'static str {
@@ -2539,13 +2561,22 @@ mod tests {
 
     // ─── parse_markdown_detailed ──────────────────────────────────────────────
 
+    fn reconstruct(parsed: &ParsedSource) -> String {
+        reconstruct_source(
+            &parsed.leading_whitespace,
+            &parsed.front_matter_mapping,
+            &parsed.body_raw,
+        )
+        .unwrap()
+    }
+
     #[test]
     fn parse_markdown_detailed_preserves_body_byte_for_byte() {
         // Case 1: trailing newline.
         let original = "---\nname: x\ndescription: y\n---\nbody line\n";
         let parsed = parse_markdown_detailed(original).unwrap();
         assert_eq!(parsed.body_raw, "\nbody line\n");
-        let reconstructed = reconstruct_source(&parsed).unwrap();
+        let reconstructed = reconstruct(&parsed);
         // No migrations ran, so the YAML round-trip is the only
         // structural change. The body region is byte-faithful.
         assert!(reconstructed.ends_with("---\nbody line\n"));
@@ -2554,14 +2585,14 @@ mod tests {
         let original = "---\nname: x\ndescription: y\n---\nbody";
         let parsed = parse_markdown_detailed(original).unwrap();
         assert_eq!(parsed.body_raw, "\nbody");
-        let reconstructed = reconstruct_source(&parsed).unwrap();
+        let reconstructed = reconstruct(&parsed);
         assert!(reconstructed.ends_with("---\nbody"));
 
         // Case 3: empty body, but trailing newline.
         let original = "---\nname: x\ndescription: y\n---\n";
         let parsed = parse_markdown_detailed(original).unwrap();
         assert_eq!(parsed.body_raw, "\n");
-        let reconstructed = reconstruct_source(&parsed).unwrap();
+        let reconstructed = reconstruct(&parsed);
         assert!(reconstructed.ends_with("---\n"));
 
         // Case 4: blank lines between closing fence and content are
@@ -2580,13 +2611,30 @@ mod tests {
         let original = "---\nname: x\ndescription: y\n---\n## body\n";
         let parsed = parse_markdown_detailed(original).unwrap();
         assert!(!parsed.migrations.changed());
-        let reconstructed = reconstruct_source(&parsed).unwrap();
+        let reconstructed = reconstruct(&parsed);
         // Find the closing fence in both and compare the suffix.
         let orig_suffix = &original[original.find("\n---\n").unwrap()..];
         let recon_suffix = &reconstructed[reconstructed.find("\n---\n").unwrap()..];
         assert_eq!(orig_suffix, recon_suffix, "body region must be byte-identical");
     }
 
+    #[test]
+    fn parse_markdown_detailed_preserves_leading_whitespace() {
+        // Leading blank lines / spaces (e.g. from editor BOM-strippers
+        // adding a blank line) must round-trip through reconstruction
+        // so migration rewrites don't silently normalize them away.
+        let original = "\n  \n---\nname: x\ndescription: y\n---\nbody\n";
+        let parsed = parse_markdown_detailed(original).unwrap();
+        assert_eq!(parsed.leading_whitespace, "\n  \n");
+        let reconstructed = reconstruct(&parsed);
+        assert!(
+            reconstructed.starts_with("\n  \n---\n"),
+            "expected leading whitespace preserved, got: {:?}",
+            &reconstructed[..20.min(reconstructed.len())]
+        );
+        assert!(reconstructed.ends_with("---\nbody\n"));
+    }
+
     #[test]
     fn parse_markdown_detailed_allows_leading_whitespace() {
         let original = "\n  \n---\nname: x\ndescription: y\n---\nbody\n";

src/compile/migration_integration_test.rs

@@ -194,8 +194,16 @@ async fn perform_source_rewrite_lost_update_guard() {
     std::fs::write(&source_path, "different bytes\n").unwrap();
 
     // Rewrite must refuse.
-    let result =
-        perform_source_rewrite_if_needed(&source_path, &original, &parsed).await;
+    let result = perform_source_rewrite_if_needed(
+        &source_path,
+        &original,
+        &parsed.leading_whitespace,
+        &parsed.front_matter_mapping,
+        &parsed.body_raw,
+        &parsed.source_sha256,
+        &parsed.migrations,
+    )
+    .await;
     let err = result.expect_err("expected lost-update guard to fire");
     assert!(
         format!("{:#}", err).contains("changed during compilation"),

src/compile/mod.rs

@@ -127,10 +127,11 @@ async fn compile_pipeline_inner(
 
     let parsed = common::parse_markdown_detailed_with_registry(&content, registry)?;
     let mut front_matter = parsed.front_matter;
-    let markdown_body = parsed.markdown_body.clone();
-    let migrations = parsed.migrations.clone();
-    let front_matter_mapping = parsed.front_matter_mapping.clone();
-    let body_raw = parsed.body_raw.clone();
+    let markdown_body = parsed.markdown_body;
+    let migrations = parsed.migrations;
+    let front_matter_mapping = parsed.front_matter_mapping;
+    let leading_whitespace = parsed.leading_whitespace;
+    let body_raw = parsed.body_raw;
     let source_sha256 = parsed.source_sha256;
 
     // Sanitize all front matter text fields before any further processing.
@@ -205,21 +206,16 @@ async fn compile_pipeline_inner(
     // we abort before the lock file ever gets touched.
     let mut migrated = false;
     if migrations.changed() {
-        // Rebuild the parsed handle (the typed front_matter has been
-        // sanitized and consumed; rewrite needs the unmodified mapping).
-        let parsed_for_rewrite = common::ParsedSource {
-            front_matter: serde_yaml::from_value(serde_yaml::Value::Mapping(
-                front_matter_mapping.clone(),
-            ))
-            .context("Failed to round-trip migrated front matter for rewrite")?,
-            markdown_body: markdown_body.clone(),
-            migrations: migrations.clone(),
-            front_matter_mapping: front_matter_mapping.clone(),
-            body_raw: body_raw.clone(),
-            source_sha256,
-        };
-        migrated = perform_source_rewrite_if_needed(input_path, &content, &parsed_for_rewrite)
-            .await?;
+        migrated = perform_source_rewrite_if_needed(
+            input_path,
+            &content,
+            &leading_whitespace,
+            &front_matter_mapping,
+            &body_raw,
+            &source_sha256,
+            &migrations,
+        )
+        .await?;
     }
 
     // Write output via atomic_write so a crash mid-write cannot leave a
@@ -263,9 +259,14 @@ async fn compile_pipeline_inner(
 async fn perform_source_rewrite_if_needed(
     input_path: &Path,
     original_content: &str,
-    parsed: &common::ParsedSource,
+    leading_whitespace: &str,
+    front_matter_mapping: &serde_yaml::Mapping,
+    body_raw: &str,
+    source_sha256: &[u8; 32],
+    migrations: &migrations::MigrationReport,
 ) -> Result<bool> {
-    let new_content = common::reconstruct_source(parsed)?;
+    let new_content =
+        common::reconstruct_source(leading_whitespace, front_matter_mapping, body_raw)?;
     if new_content == original_content {
         // Migrations ran but their net effect is a no-op on disk —
         // skip the rewrite to avoid gratuitous diffs.
@@ -285,7 +286,7 @@ async fn perform_source_rewrite_if_needed(
     let mut hasher = sha2::Sha256::new();
     hasher.update(&current_bytes);
     let current_hash: [u8; 32] = hasher.finalize().into();
-    if current_hash != parsed.source_sha256 {
+    if &current_hash != source_sha256 {
         anyhow::bail!(
             "source file {} changed during compilation; refusing to migrate. Re-run `ado-aw compile`.",
             input_path.display()
@@ -304,10 +305,10 @@ async fn perform_source_rewrite_if_needed(
     eprintln!(
         "warning: migrated front matter in {}: schema-version {} -> {}",
         input_path.display(),
-        parsed.migrations.from_version,
-        parsed.migrations.to_version
+        migrations.from_version,
+        migrations.to_version
     );
-    for applied in &parsed.migrations.applied {
+    for applied in &migrations.applied {
         eprintln!("  - {}: {}", applied.id, applied.summary);
     }
     eprintln!(
@@ -519,7 +520,7 @@ pub async fn check_pipeline(pipeline_path: &str) -> Result<()> {
     // we want to fail loudly so `ado-aw compile` is run.
     if parsed.migrations.changed() {
         anyhow::bail!(
-            "error: {} is at schema-version {}; this compiler requires schema-version {}.\n\
+            "{} is at schema-version {}; this compiler requires schema-version {}.\n  \
              hint: run `ado-aw compile {}` to migrate the source in place.",
             source_path.display(),
             parsed.migrations.from_version,

tests/migration_tests.rs

@@ -27,8 +27,6 @@ fn fresh_temp_dir(label: &str) -> PathBuf {
         "ado-aw-migration-tests-{}-{}-{}",
         label,
         std::process::id(),
-        // Add a thread-local counter to avoid intra-process collisions
-        // when multiple tests run in parallel.
         rand_suffix(),
     ));
     fs::create_dir_all(&dir).expect("create temp dir");
@@ -45,14 +43,20 @@ fn fresh_git_temp_dir(label: &str) -> PathBuf {
     dir
 }
 
-/// Lightweight randomness for test temp dir uniqueness — no crate dep.
+/// Suffix that's unique within the process for the lifetime of a
+/// single test binary run. Uses a wall-clock nanosecond timestamp
+/// combined with a monotonic atomic counter so two parallel tests
+/// scheduled in the same nanosecond still get distinct directories.
 fn rand_suffix() -> String {
+    use std::sync::atomic::{AtomicU64, Ordering};
     use std::time::{SystemTime, UNIX_EPOCH};
+    static SEQ: AtomicU64 = AtomicU64::new(0);
     let nanos = SystemTime::now()
         .duration_since(UNIX_EPOCH)
         .map(|d| d.as_nanos())
         .unwrap_or(0);
-    format!("{:x}", nanos)
+    let seq = SEQ.fetch_add(1, Ordering::Relaxed);
+    format!("{:x}-{:x}", nanos, seq)
 }
 
 fn ado_aw_binary() -> PathBuf {