refactor(test): apply cleanup review to the snapshot harness

Reuse: milestone hex encoding via Buffer.toString('hex'); the migrator now
imports the legacy Steps schema from snap-test.ts instead of redeclaring it.
Simplification: shared makeTodo/redirect handling and a verbatim-vpt set in
the migrator; dead NewStep fields dropped; shared find_beside_test_exe and
manifest_dir helpers in the harness; the always-true separator guard from
the upstream port removed.
Efficiency: redaction regexes compiled once per run (LazyLock), diagnostic
sort skipped when no blocks exist, fixture staging filters harness metadata
instead of copy-then-delete, per-step env only cloned when a step overrides
it, and the prompts milestone flag is cached at module load (per-keystroke
path).

Suite output is unchanged: all 8 trials pass against existing snapshots and
the migrator reproduces byte-identical fixtures.

Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT

Author: fengmk2

crates/vite_cli_snapshots/tests/cli_snapshots/flavor.rs

@@ -40,12 +40,32 @@ pub struct FlavorRuntime {
     pub path_env: OsString,
 }
 
+/// The harness crate's manifest dir. The runtime env var wins: cargo sets it
+/// for test processes, and nextest rewrites it when running a relocated
+/// archive (`--workspace-remap`), where the compile-time path is a
+/// build-machine path that no longer exists.
+pub fn manifest_dir() -> PathBuf {
+    std::env::var_os("CARGO_MANIFEST_DIR")
+        .map_or_else(|| PathBuf::from(env!("CARGO_MANIFEST_DIR")), PathBuf::from)
+}
+
 pub fn repo_root() -> PathBuf {
-    // Runtime env preferred over the compile-time path for relocated nextest
-    // archives (see main.rs).
-    let manifest_dir = std::env::var_os("CARGO_MANIFEST_DIR")
-        .map_or_else(|| PathBuf::from(env!("CARGO_MANIFEST_DIR")), PathBuf::from);
-    manifest_dir.parent().unwrap().parent().unwrap().to_path_buf()
+    manifest_dir().parent().unwrap().parent().unwrap().to_path_buf()
+}
+
+/// Searches for `name` next to the test executable (`target/<profile>/deps/`)
+/// and one directory up (`target/<profile>/`), where cargo puts bin targets
+/// and where nextest extracts archived binaries.
+fn find_beside_test_exe(name: &str) -> Result<Option<PathBuf>, String> {
+    let exe = std::env::current_exe().map_err(|e| format!("current_exe failed: {e}"))?;
+    let deps_dir = exe.parent().ok_or("test executable has no parent dir")?;
+    for dir in [deps_dir, deps_dir.parent().unwrap_or(deps_dir)] {
+        let candidate = dir.join(name);
+        if candidate.is_file() {
+            return Ok(Some(candidate));
+        }
+    }
+    Ok(None)
 }
 
 /// Locates the freshly built global `vp` binary next to this test executable
@@ -64,20 +84,12 @@ fn global_vp_path() -> Result<PathBuf, String> {
         }
         return Err(format!("VP_SNAP_GLOBAL_VP is set but {} does not exist", vp.display()));
     }
-    let exe = std::env::current_exe().map_err(|e| format!("current_exe failed: {e}"))?;
-    let deps_dir = exe.parent().ok_or("test executable has no parent dir")?;
     let name = format!("vp{}", std::env::consts::EXE_SUFFIX);
-    for dir in [deps_dir, deps_dir.parent().unwrap_or(deps_dir)] {
-        let candidate = dir.join(&name);
-        if candidate.is_file() {
-            return Ok(candidate);
-        }
-    }
-    Err(format!(
-        "global `vp` binary not found next to {}; run `just snapshot-test` \
-         (or `cargo build -p vite_global_cli`) first",
-        exe.display()
-    ))
+    find_beside_test_exe(&name)?.ok_or_else(|| {
+        "global `vp` binary not found next to the test executable; run \
+         `just snapshot-test` (or `cargo build -p vite_global_cli`) first"
+            .to_owned()
+    })
 }
 
 /// Locates the local JS CLI bin directory. `VP_SNAP_LOCAL_CLI_BIN_DIR`
@@ -113,18 +125,12 @@ fn vpt_path() -> Result<PathBuf, String> {
     if compile_time.is_file() {
         return Ok(compile_time);
     }
-    let exe = std::env::current_exe().map_err(|e| format!("current_exe failed: {e}"))?;
     let name = format!("vpt{}", std::env::consts::EXE_SUFFIX);
-    let deps_dir = exe.parent().ok_or("test executable has no parent dir")?;
-    for dir in [deps_dir, deps_dir.parent().unwrap_or(deps_dir)] {
-        let candidate = dir.join(&name);
-        if candidate.is_file() {
-            return Ok(candidate);
-        }
-    }
-    Err("`vpt` binary not found (checked CARGO_BIN_EXE_vpt, the compile-time \
+    find_beside_test_exe(&name)?.ok_or_else(|| {
+        "`vpt` binary not found (checked CARGO_BIN_EXE_vpt, the compile-time \
          path, and next to the test executable)"
-        .to_owned())
+            .to_owned()
+    })
 }
 
 /// Directory holding an already-provisioned managed JS runtime that each

crates/vite_cli_snapshots/tests/cli_snapshots/main.rs

@@ -600,11 +600,12 @@ fn run_case(
     let case_root = tmpdir.join(format!("{fixture_name}_case_{case_index}_{}", flavor.as_str()));
     let stage = case_root.join("workspace");
     std::fs::create_dir_all(&stage).unwrap();
-    CopyOptions::new().copy_tree(fixture_path, &stage).unwrap();
     // The case definition and recorded snapshots are harness metadata, not
-    // part of the workspace under test.
-    let _ = std::fs::remove_dir_all(stage.join("snapshots"));
-    let _ = std::fs::remove_file(stage.join("snapshots.toml"));
+    // part of the workspace under test, so they are never copied in.
+    CopyOptions::new()
+        .filter(|path, _| Ok(path != Path::new("snapshots") && path != Path::new("snapshots.toml")))
+        .copy_tree(fixture_path, &stage)
+        .unwrap();
 
     let case_home = CaseHome::provision(&case_root, case.seed_runtime);
 
@@ -643,10 +644,18 @@ fn run_case(
         assert!(!argv.is_empty(), "step argv must not be empty");
         let program = runtime.resolve_program(&argv[0])?;
 
-        let mut step_env = case_env.clone();
-        for (k, v) in step.envs() {
-            step_env.insert(k.clone(), v.into());
-        }
+        // Most steps add no env of their own; borrow the case env then.
+        let step_env_override;
+        let step_env: &BTreeMap<String, OsString> = if step.envs().is_empty() {
+            &case_env
+        } else {
+            let mut env = case_env.clone();
+            for (k, v) in step.envs() {
+                env.insert(k.clone(), v.into());
+            }
+            step_env_override = env;
+            &step_env_override
+        };
         let step_cwd = stage.join(step.cwd().unwrap_or(case.cwd.as_str()));
         let timeout = step.timeout();
 
@@ -656,7 +665,7 @@ fn run_case(
                 cmd.arg(arg);
             }
             cmd.env_clear();
-            for (k, v) in &step_env {
+            for (k, v) in step_env {
                 cmd.env(k, v);
             }
             cmd.cwd(&step_cwd);
@@ -745,7 +754,7 @@ fn run_case(
                 step.interactions().is_empty(),
                 "interactions require a PTY; remove `tty = false` or the interactions"
             );
-            let (state, raw) = run_step_piped(&program, &argv[1..], &step_env, &step_cwd, timeout);
+            let (state, raw) = run_step_piped(&program, &argv[1..], step_env, &step_cwd, timeout);
             let mut block = String::new();
             push_fenced_block(&mut block, &raw);
             (state, block)
@@ -813,12 +822,7 @@ fn main() {
     let tmp_dir = tempfile::tempdir().unwrap();
     let tmp_dir_path: Arc<Path> = Arc::from(tmp_dir.path().canonicalize().unwrap());
 
-    // Prefer the runtime env var: cargo sets it for test processes, and
-    // nextest rewrites it when running a relocated archive
-    // (--workspace-remap), where the compile-time path no longer exists.
-    let manifest_dir = std::env::var_os("CARGO_MANIFEST_DIR")
-        .map_or_else(|| PathBuf::from(env!("CARGO_MANIFEST_DIR")), PathBuf::from);
-    let fixtures_dir = manifest_dir.join("tests/cli_snapshots/fixtures");
+    let fixtures_dir = flavor::manifest_dir().join("tests/cli_snapshots/fixtures");
 
     let mut fixture_paths = std::fs::read_dir(&fixtures_dir)
         .unwrap_or_else(|e| panic!("failed to read {}: {e}", fixtures_dir.display()))

crates/vite_cli_snapshots/tests/cli_snapshots/redact.rs

@@ -5,7 +5,28 @@
 //! stdout/stderr interleaving, so every rule here should correspond to a real
 //! source of nondeterminism (paths, durations, versions, machine parallelism).
 
-use std::borrow::Cow;
+use std::{borrow::Cow, sync::LazyLock};
+
+// Compiled once per run: redaction runs on every snapshotted step, and regex
+// compilation dominates matching cost at that frequency.
+static UUID_RE: LazyLock<regex::Regex> = LazyLock::new(|| {
+    regex::Regex::new(r"[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}").unwrap()
+});
+static DURATION_RE: LazyLock<regex::Regex> =
+    LazyLock::new(|| regex::Regex::new(r"\b\d+(\.\d+)?(ns|µs|ms|s)\b").unwrap());
+static VERSION_RE: LazyLock<regex::Regex> = LazyLock::new(|| {
+    regex::Regex::new(r"\bv?\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?\b").unwrap()
+});
+static THREAD_RE: LazyLock<regex::Regex> =
+    LazyLock::new(|| regex::Regex::new(r"\d+ threads").unwrap());
+static NODE_WARNING_RE: LazyLock<regex::Regex> =
+    LazyLock::new(|| regex::Regex::new(r"(?m)^\(node:\d+\) ExperimentalWarning:.*\n?").unwrap());
+static NODE_TRACE_WARNING_RE: LazyLock<regex::Regex> = LazyLock::new(|| {
+    regex::Regex::new(
+        r"(?m)^\(Use `node --trace-warnings \.\.\.` to show where the warning was created\)\n?",
+    )
+    .unwrap()
+});
 
 #[expect(
     clippy::disallowed_types,
@@ -72,33 +93,21 @@ pub fn redact_output(mut output: String, paths: &[(&str, &'static str)]) -> Stri
     redact_string(&mut output, &borrowed);
 
     // Redact UUIDs to "<uuid>"
-    let uuid_regex =
-        regex::Regex::new(r"[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}").unwrap();
-    output = uuid_regex.replace_all(&output, "<uuid>").into_owned();
+    output = UUID_RE.replace_all(&output, "<uuid>").into_owned();
 
     // Redact durations like "0ns", "123ms" or "1.23s" to "<duration>".
     // Runs before version redaction so "1.23s" never half-matches as a version.
-    let duration_regex = regex::Regex::new(r"\b\d+(\.\d+)?(ns|µs|ms|s)\b").unwrap();
-    output = duration_regex.replace_all(&output, "<duration>").into_owned();
+    output = DURATION_RE.replace_all(&output, "<duration>").into_owned();
 
     // Redact semver-shaped versions (bundled tool versions, Node versions).
-    let version_regex =
-        regex::Regex::new(r"\bv?\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?\b").unwrap();
-    output = version_regex.replace_all(&output, "<version>").into_owned();
+    output = VERSION_RE.replace_all(&output, "<version>").into_owned();
 
     // Redact thread counts like "16 threads" to "<n> threads"
-    let thread_regex = regex::Regex::new(r"\d+ threads").unwrap();
-    output = thread_regex.replace_all(&output, "<n> threads").into_owned();
+    output = THREAD_RE.replace_all(&output, "<n> threads").into_owned();
 
     // Remove Node.js experimental warnings (e.g., Type Stripping warnings)
-    let node_warning_regex =
-        regex::Regex::new(r"(?m)^\(node:\d+\) ExperimentalWarning:.*\n?").unwrap();
-    output = node_warning_regex.replace_all(&output, "").into_owned();
-    let node_trace_warning_regex = regex::Regex::new(
-        r"(?m)^\(Use `node --trace-warnings \.\.\.` to show where the warning was created\)\n?",
-    )
-    .unwrap();
-    output = node_trace_warning_regex.replace_all(&output, "").into_owned();
+    output = NODE_WARNING_RE.replace_all(&output, "").into_owned();
+    output = NODE_TRACE_WARNING_RE.replace_all(&output, "").into_owned();
 
     // Remove ^C echo that Unix terminal drivers emit when ETX (0x03) is written
     // to the PTY. Windows ConPTY does not echo it.
@@ -111,8 +120,11 @@ pub fn redact_output(mut output: String, paths: &[(&str, &'static str)]) -> Stri
 
     // Sort consecutive diagnostic blocks to handle non-deterministic tool output
     // (e.g., oxlint reports warnings in arbitrary order due to multi-threading).
-    // Each block starts with "  ! " and ends at the next empty line.
-    output = sort_diagnostic_blocks(&output);
+    // Each block starts with "  ! " and ends at the next empty line. Most
+    // screens have none, so skip the split/rejoin allocation entirely then.
+    if output.contains("  ! ") {
+        output = sort_diagnostic_blocks(&output);
+    }
 
     output
 }
@@ -148,12 +160,12 @@ fn sort_diagnostic_blocks(output: &str) -> String {
 
             blocks.sort();
 
-            for (j, block) in blocks.iter().enumerate() {
+            // Restore an empty-line separator after every block (`i` never
+            // exceeds parts.len(), so the upstream guard here was always
+            // true; keep the behavior, drop the misleading condition).
+            for block in &blocks {
                 result.extend_from_slice(block);
-                // Restore empty line separators (between blocks + trailing)
-                if j < blocks.len() - 1 || i <= parts.len() {
-                    result.push("");
-                }
+                result.push("");
             }
         } else {
             result.push(parts[i]);

packages/prompts/src/__tests__/milestone.spec.ts

@@ -1,9 +1,19 @@
-import { afterEach, describe, expect, it } from 'vitest';
-
-import { milestone, promptMilestone } from '../milestone.js';
+import { afterEach, describe, expect, it, vi } from 'vitest';
 
 const originalEnv = process.env.VP_EMIT_MILESTONES;
 
+// The enabled flag is cached at module load (the harness sets the env before
+// spawning the CLI), so each test imports a fresh module copy under its env.
+async function loadMilestone(value: string | undefined) {
+  vi.resetModules();
+  if (value === undefined) {
+    delete process.env.VP_EMIT_MILESTONES;
+  } else {
+    process.env.VP_EMIT_MILESTONES = value;
+  }
+  return import('../milestone.js');
+}
+
 afterEach(() => {
   if (originalEnv === undefined) {
     delete process.env.VP_EMIT_MILESTONES;
@@ -13,22 +23,22 @@ afterEach(() => {
 });
 
 describe('milestone', () => {
-  it('emits nothing unless VP_EMIT_MILESTONES=1', () => {
-    delete process.env.VP_EMIT_MILESTONES;
-    expect(milestone('vp')).toBe('');
-    process.env.VP_EMIT_MILESTONES = '0';
-    expect(milestone('vp')).toBe('');
+  it('emits nothing unless VP_EMIT_MILESTONES=1', async () => {
+    const unset = await loadMilestone(undefined);
+    expect(unset.milestone('vp')).toBe('');
+    const disabled = await loadMilestone('0');
+    expect(disabled.milestone('vp')).toBe('');
   });
 
-  it('encodes the name as a hex OSC 8 hyperlink with a zero-width anchor', () => {
-    process.env.VP_EMIT_MILESTONES = '1';
+  it('encodes the name as a hex OSC 8 hyperlink with a zero-width anchor', async () => {
+    const { milestone } = await loadMilestone('1');
     // "vp" is 0x76 0x70; the sequence must match vite-task's
     // pty_terminal_test_client protocol byte for byte.
     expect(milestone('vp')).toBe('\x1b]8;;https://milestone.invalid/7670\x1b\\​\x1b]8;;\x1b\\');
   });
 
-  it('formats prompt milestones as <kind>:<id>:<state>', () => {
-    process.env.VP_EMIT_MILESTONES = '1';
+  it('formats prompt milestones as <kind>:<id>:<state>', async () => {
+    const { milestone, promptMilestone } = await loadMilestone('1');
     expect(promptMilestone('select', 'template', '1')).toBe(milestone('select:template:1'));
     expect(promptMilestone('confirm', undefined, 'yes')).toBe(milestone('confirm:confirm:yes'));
   });

packages/prompts/src/milestone.ts

@@ -16,8 +16,13 @@ const OSC8_OPEN = '\x1b]8;;';
 const ST = '\x1b\\';
 const ZERO_WIDTH_ANCHOR = '​';
 
+// Cached at module load: the harness sets the env before spawning the CLI,
+// and milestone() runs on every prompt render (every keystroke), so the
+// disabled path must stay a single branch.
+const MILESTONES_ENABLED = process.env.VP_EMIT_MILESTONES === '1';
+
 export function milestonesEnabled(): boolean {
-  return process.env.VP_EMIT_MILESTONES === '1';
+  return MILESTONES_ENABLED;
 }
 
 /**
@@ -26,13 +31,10 @@ export function milestonesEnabled(): boolean {
  * the marker arrives in the output stream together with the render it marks.
  */
 export function milestone(name: string): string {
-  if (!milestonesEnabled()) {
+  if (!MILESTONES_ENABLED) {
     return '';
   }
-  let hex = '';
-  for (const byte of Buffer.from(name, 'utf8')) {
-    hex += byte.toString(16).padStart(2, '0');
-  }
+  const hex = Buffer.from(name, 'utf8').toString('hex');
   return `${OSC8_OPEN}${MILESTONE_URI_PREFIX}${hex}${ST}${ZERO_WIDTH_ANCHOR}${OSC8_OPEN}${ST}`;
 }