fix(compile): address fourth-round PR review on codemod refactor

Two findings + three suggestions from the latest Rust PR Reviewer pass:

1. Stage-3 typed-deserialize errors after codemods reused the same
   "Failed to parse YAML front matter" context as the Stage-1 raw
   parse, so a codemod that produced an invalid shape was
   indistinguishable from a user typo. Switch to with_context that
   surfaces the applied codemod ids when codemods ran:
   "Failed to parse YAML front matter after applying codemods (id1, id2);
    a codemod likely produced an invalid shape".

2. rename_key with ConflictPolicy::PreferNew returns Ok(true) when
   it drops a stale `old` key even though `new` is unchanged. That
   triggers a source-rewrite warning saying "we migrated" when
   really we "cleaned up a remnant". The behavior is correct (the
   mapping did mutate); the doc was misleading. Rewrote the
   rename_key doc comment to enumerate exactly when Ok(true) fires
   and call out the PreferNew/cleanup case.

3. Added `// TODO(codemods): remove when the first real codemod is
   registered.` markers next to every #[allow(dead_code)] /
   #[allow(unused_imports)] in src/compile/codemods/{mod,helpers}.rs
   so search-based cleanup is trivial when the registry stops being
   empty.

4. tests/codemod_tests.rs leaked temp dirs on assertion failure
   because `let _ = fs::remove_dir_all(&dir)` only ran on the happy
   path. Refactored to return tempfile::TempDir from the helpers
   and rely on RAII cleanup, matching the pattern already used by
   src/compile/codemod_integration_test.rs.

Skipped: the empty-mapping concern in reconstruct_source (a
mapping reduced to empty wouldn't deserialize as FrontMatter
anyway because of required name/description fields, so the
existing typed-deserialize error path catches it; #1 makes that
error informative).

All 1349 tests pass.

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

Author: jamesadevine

src/compile/codemods/helpers.rs

@@ -9,6 +9,7 @@ use serde_yaml::{Mapping, Value};
 
 /// Conflict policy used by [`rename_key`] when the destination key is
 /// already present.
+// TODO(codemods): remove when the first real codemod is registered.
 #[allow(dead_code)]
 #[derive(Debug, Clone, Copy)]
 pub enum ConflictPolicy {
@@ -21,6 +22,7 @@ pub enum ConflictPolicy {
 }
 
 /// Remove and return the value at `key`, or `None` if absent.
+// TODO(codemods): remove when the first real codemod is registered.
 #[allow(dead_code)]
 pub fn take_key(m: &mut Mapping, key: &str) -> Option<Value> {
     m.remove(Value::String(key.to_string()))
@@ -30,6 +32,7 @@ pub fn take_key(m: &mut Mapping, key: &str) -> Option<Value> {
 ///
 /// Prefer this over `Mapping::insert` in migrations: silent overwrite is
 /// almost always a bug when transforming user data.
+// TODO(codemods): remove when the first real codemod is registered.
 #[allow(dead_code)]
 pub fn insert_no_overwrite(
     m: &mut Mapping,
@@ -48,13 +51,24 @@ pub fn insert_no_overwrite(
 
 /// Rename `old` → `new` according to `policy`.
 ///
-/// Returns `Ok(true)` when the rename happened (regardless of policy
-/// branch), `Ok(false)` when `old` was absent (no-op).
+/// Returns `Ok(true)` when the mapping was mutated in any way:
+///
+/// - the `old` key was moved to `new` (the typical rename), OR
+/// - both keys were present with [`ConflictPolicy::PreferOld`] and
+///   `new` was overwritten, OR
+/// - both keys were present with [`ConflictPolicy::PreferNew`] and
+///   the stale `old` key was dropped (note: the `new` value did not
+///   change, but the mapping still mutated — codemod authors using
+///   `PreferNew` should be aware that `Ok(true)` here means
+///   "cleaned up a remnant," not "migrated semantic content").
+///
+/// Returns `Ok(false)` when `old` was absent (no-op).
 ///
 /// The mapping is left **unchanged** on the error path. Callers can
 /// rely on this invariant when chaining migrations: a failed rename
 /// won't leave the mapping in a half-mutated state for the next call
 /// to inspect.
+// TODO(codemods): remove when the first real codemod is registered.
 #[allow(dead_code)]
 pub fn rename_key(
     m: &mut Mapping,

src/compile/codemods/mod.rs

@@ -40,6 +40,7 @@ mod helpers;
 // and uses one of these helpers, both the re-export attribute and
 // the per-item `dead_code` allows on `helpers.rs` should be
 // removed.
+// TODO(codemods): remove when the first real codemod is registered.
 #[allow(unused_imports)]
 pub use helpers::{insert_no_overwrite, rename_key, take_key, ConflictPolicy};
 
@@ -73,6 +74,7 @@ pub struct Codemod {
     pub summary: &'static str,
     /// Compiler version that introduced this codemod (e.g. "0.27.0").
     /// Provenance only; not consumed by the runner.
+    // TODO(codemods): remove when the first real codemod is registered.
     #[allow(dead_code)]
     pub introduced_in: &'static str,
     /// The transformation. Returns `Ok(true)` when the codemod
@@ -107,6 +109,7 @@ pub struct AppliedCodemod {
 
 impl CodemodReport {
     /// An empty report (no codemod fired).
+    // TODO(codemods): remove when the first real codemod is registered.
     #[allow(dead_code)]
     pub fn empty() -> Self {
         Self {
@@ -120,6 +123,7 @@ impl CodemodReport {
     }
 
     /// IDs of codemods that ran, in order. Helpful for tests.
+    // TODO(codemods): remove when the first real codemod is registered.
     #[allow(dead_code)]
     pub fn applied_ids(&self) -> Vec<&'static str> {
         self.applied.iter().map(|a| a.id).collect()
@@ -130,6 +134,7 @@ impl CodemodReport {
 ///
 /// Equivalent to [`apply_codemods_with`] called with the global
 /// [`CODEMODS`] registry.
+// TODO(codemods): remove when the first real codemod is registered.
 #[allow(dead_code)]
 pub fn apply_codemods(fm: &mut Mapping) -> Result<CodemodReport> {
     apply_codemods_with(fm, CODEMODS)

src/compile/common.rs

@@ -182,10 +182,23 @@ pub(crate) fn parse_markdown_detailed_with_registry(
 
     // Stage 3: deserialize the (possibly modified) mapping into the
     // typed FrontMatter. Errors here mean either the user wrote an
-    // unsupported shape or a codemod produced invalid output.
-    let front_matter: FrontMatter =
-        serde_yaml::from_value(serde_yaml::Value::Mapping(mapping.clone()))
-            .context("Failed to parse YAML front matter")?;
+    // unsupported shape or a codemod produced invalid output. The
+    // error context differs by case so the user can tell which.
+    let front_matter: FrontMatter = serde_yaml::from_value(
+        serde_yaml::Value::Mapping(mapping.clone()),
+    )
+    .with_context(|| {
+        if report.changed() {
+            let ids = report.applied_ids().join(", ");
+            format!(
+                "Failed to parse YAML front matter after applying codemods ({}); \
+                 a codemod likely produced an invalid shape",
+                ids
+            )
+        } else {
+            "Failed to parse YAML front matter".to_string()
+        }
+    })?;
 
     Ok(ParsedSource {
         front_matter,

tests/codemod_tests.rs

@@ -17,69 +17,51 @@
 //! - The full `compile` -> `check` round-trip succeeds.
 
 use std::fs;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::process::Command;
-
-/// Set up a unique temp directory for each test run.
-fn fresh_temp_dir(label: &str) -> PathBuf {
-    let dir = std::env::temp_dir().join(format!(
-        "ado-aw-codemod-tests-{}-{}-{}",
-        label,
-        std::process::id(),
-        rand_suffix(),
-    ));
-    fs::create_dir_all(&dir).expect("create temp dir");
-    dir
+use tempfile::TempDir;
+
+/// Set up a unique temp directory for each test run. Returned as a
+/// `TempDir` so RAII cleans the directory up even if a test panics.
+fn fresh_temp_dir() -> TempDir {
+    tempfile::Builder::new()
+        .prefix("ado-aw-codemod-tests-")
+        .tempdir()
+        .expect("create temp dir")
 }
 
 /// Same as [`fresh_temp_dir`] but also creates an empty `.git/`
 /// directory at the root so `ado-aw check` (which walks up to the
 /// repo root) can resolve a source path from the compiled lock
 /// file's `@ado-aw` header.
-fn fresh_git_temp_dir(label: &str) -> PathBuf {
-    let dir = fresh_temp_dir(label);
-    fs::create_dir(dir.join(".git")).expect("create .git dir");
+fn fresh_git_temp_dir() -> TempDir {
+    let dir = fresh_temp_dir();
+    fs::create_dir(dir.path().join(".git")).expect("create .git dir");
     dir
 }
 
-/// 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);
-    let seq = SEQ.fetch_add(1, Ordering::Relaxed);
-    format!("{:x}-{:x}", nanos, seq)
-}
-
 fn ado_aw_binary() -> PathBuf {
     PathBuf::from(env!("CARGO_BIN_EXE_ado-aw"))
 }
 
 /// Run `ado-aw compile <source>`, returning the captured output.
-fn run_compile(source: &PathBuf) -> std::process::Output {
+fn run_compile(source: &Path) -> std::process::Output {
     Command::new(ado_aw_binary())
         .args(["compile", source.to_str().unwrap()])
         .output()
         .expect("Failed to run ado-aw compile")
 }
 
 /// Run `ado-aw check <pipeline>`, returning the captured output.
-fn run_check(pipeline: &PathBuf) -> std::process::Output {
+fn run_check(pipeline: &Path) -> std::process::Output {
     Command::new(ado_aw_binary())
         .args(["check", pipeline.to_str().unwrap()])
         .output()
         .expect("Failed to run ado-aw check")
 }
 
 /// Write a source file to `dir/agent.md` and return its path.
-fn write_source(dir: &PathBuf, content: &str) -> PathBuf {
+fn write_source(dir: &Path, content: &str) -> PathBuf {
     let path = dir.join("agent.md");
     fs::write(&path, content).expect("write source");
     path
@@ -89,9 +71,9 @@ fn write_source(dir: &PathBuf, content: &str) -> PathBuf {
 
 #[test]
 fn compile_succeeds_on_current_source() {
-    let dir = fresh_temp_dir("current-source");
+    let dir = fresh_temp_dir();
     let original = "---\nname: smoketest\ndescription: smoketest description\n---\n## Body\n\nHello.\n";
-    let source = write_source(&dir, original);
+    let source = write_source(dir.path(), original);
 
     let output = run_compile(&source);
 
@@ -125,15 +107,13 @@ fn compile_succeeds_on_current_source() {
         "no codemod warning expected, got stderr: {}",
         stderr
     );
-
-    let _ = fs::remove_dir_all(&dir);
 }
 
 #[test]
 fn compile_then_check_round_trip_passes() {
-    let dir = fresh_git_temp_dir("round-trip");
+    let dir = fresh_git_temp_dir();
     let source = write_source(
-        &dir,
+        dir.path(),
         "---\nname: round-trip-agent\ndescription: round-trip\n---\n## Body\n",
     );
 
@@ -154,16 +134,14 @@ fn compile_then_check_round_trip_passes() {
         String::from_utf8_lossy(&check_output.stdout),
         String::from_utf8_lossy(&check_output.stderr)
     );
-
-    let _ = fs::remove_dir_all(&dir);
 }
 
 // ─── Non-mapping front matter ──────────────────────────────────────────────
 
 #[test]
 fn compile_rejects_non_mapping_top_level_yaml() {
-    let dir = fresh_temp_dir("non-mapping");
-    let source = write_source(&dir, "---\n- a\n- b\n---\nbody\n");
+    let dir = fresh_temp_dir();
+    let source = write_source(dir.path(), "---\n- a\n- b\n---\nbody\n");
 
     let output = run_compile(&source);
 
@@ -177,6 +155,4 @@ fn compile_rejects_non_mapping_top_level_yaml() {
         "stderr should report non-mapping error, got: {}",
         stderr
     );
-
-    let _ = fs::remove_dir_all(&dir);
 }