From 9114893a97a956f369dfec0880a3f0639f3c7d8f Mon Sep 17 00:00:00 2001 From: "hetz.pty-rust" Date: Sun, 6 Sep 2026 11:19:36 +0200 Subject: [PATCH] fix(conformance): no hardcoded checkout path, and no silent substitute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is how docs/conformance.md went stale, so it is not tidying. Three sites defaulted to an absolute path on one developer's machine when PTY_NODE_CHECKOUT was unset. The path is a machine specific that CLAUDE.md forbids in a shipped artifact, but the silence is what did the damage. The test did not skip. It compared against whatever was at that path, which on one host is a checkout seven commits behind, and reported a confident failure about a completion entry that was not missing. CI never saw it, because the path does not exist on a runner, so there the filter returned None and the test skipped. It only misbehaved where that path happened to exist. The generator was worse. With no checkout it fell back to that path, and node_suites falls back again to the compiled-in SUITES list, so running it with the variable unset wrote a checked-in document describing a Node tree nobody named, looking exactly as authoritative as a correct one. That is how docs/conformance.md came to say 0.12.0+500eab2 while node-ref said 86dcc5e — and a stale map is what a stale comparison is built on. The two cases get different answers on purpose. A TEST skips: a missing reference means the comparison cannot be made, and that is not a failure. A GENERATOR errors: it writes a file that outlives the run and carries no memory of what it was made from. completions.rs was also the only test doing this. parity_shapes.rs and parity_fixtures.rs already consult node_checkout() and skip. This was a divergence from a local convention, not a missing one, so the fix copies them. Controlled in both directions, because a test that always skips is worse than one that compares against a stale checkout: at least the stale one fails sometimes. With the variable set the test RUNS and passes, 9 of 9. Unset, it skips and names the variable it needs. The generator refuses when unset, refuses with a clear reason when pointed somewhere without tests/, and gets past that check when pointed at the pinned checkout. No /home/ or /Users/ path remains anywhere outside target. --- .../src/bin/conformance_map.rs | 2 +- .../src/conformance_map_impl.rs | 31 ++++++++++++++++--- crates/pty-conformance/tests/completions.rs | 16 ++++++++-- 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/crates/pty-conformance/src/bin/conformance_map.rs b/crates/pty-conformance/src/bin/conformance_map.rs index 99372df..8883bc7 100644 --- a/crates/pty-conformance/src/bin/conformance_map.rs +++ b/crates/pty-conformance/src/bin/conformance_map.rs @@ -3,7 +3,7 @@ //! comments in `tests/*.rs`), or the reason it is not portable. //! //! Usage: `cargo run -p pty-conformance --bin conformance-map [--node ] [--out ]`. -//! Defaults: the checkout in `PTY_NODE_CHECKOUT` (or `/home/myobie/src/github.com/compoundingtech/pty`), +//! Defaults: the checkout named by `PTY_NODE_CHECKOUT` (required; there is no default), //! output to `docs/conformance.md` at the workspace root. fn main() { diff --git a/crates/pty-conformance/src/conformance_map_impl.rs b/crates/pty-conformance/src/conformance_map_impl.rs index 6c25beb..132892e 100644 --- a/crates/pty-conformance/src/conformance_map_impl.rs +++ b/crates/pty-conformance/src/conformance_map_impl.rs @@ -274,11 +274,32 @@ pub fn run(args: Vec) -> Result<(), String> { } i += 1; } - if checkout.is_none() { - let default = PathBuf::from("/home/myobie/src/github.com/compoundingtech/pty"); - if default.join("tests").is_dir() { - checkout = Some(default); - } + // No default, and no silent degrade. + // + // This used to fall back to an absolute path on one developer's machine, + // and `node_suites` falls back again to the compiled-in `SUITES` list when + // it has no checkout — so running this with the variable unset produced a + // checked-in document describing a Node tree nobody named, which looked + // exactly as authoritative as a correct one. That is how docs/conformance.md + // came to say `0.12.0+500eab2` while the pinned reference in `node-ref` was + // `86dcc5e`, and a stale map is what a stale comparison is built on. + // + // A test may skip when its reference is missing. A generator may not guess: + // it writes a file that outlives the run and carries no memory of what it + // was made from. + let Some(dir) = checkout.clone() else { + return Err("PTY_NODE_CHECKOUT is not set, and there is no default.\n\ + This writes docs/conformance.md from a Node checkout, and a map \ + built from an unnamed source is worse than no map.\n\ + Set PTY_NODE_CHECKOUT to the commit in crates/pty-conformance/node-ref, \ + or pass --checkout ." + .to_string()); + }; + if !dir.join("tests").is_dir() { + return Err(format!( + "PTY_NODE_CHECKOUT is {}, which has no tests/ directory.", + dir.display() + )); } let mapped = scan_tests(&manifest_dir().join("tests"))?; diff --git a/crates/pty-conformance/tests/completions.rs b/crates/pty-conformance/tests/completions.rs index bfd353f..9e7afa4 100644 --- a/crates/pty-conformance/tests/completions.rs +++ b/crates/pty-conformance/tests/completions.rs @@ -22,10 +22,20 @@ fn which(bin: &str) -> Option { if out.status.success() && !s.is_empty() { Some(PathBuf::from(s)) } else { None } } +/// The Node checkout to compare against, or `None`. +/// +/// **There is deliberately no fallback path.** This used to default to an +/// absolute path on one developer's machine, which meant that when +/// `PTY_NODE_CHECKOUT` was unset the test did not skip — it silently compared +/// against whatever happened to be at that path. On a machine where that +/// checkout was seven commits behind, it produced a confident failure about a +/// completion entry that was not missing at all. CI never saw it, because the +/// path does not exist on a runner, so there the filter returned `None` and the +/// test skipped. +/// +/// A test that cannot find its reference must say so. It must not guess. fn node_checkout_dir() -> Option { - node_checkout() - .or_else(|| Some(PathBuf::from("/home/myobie/src/github.com/compoundingtech/pty"))) - .filter(|p| p.join("completions").is_dir()) + node_checkout().filter(|p| p.join("completions").is_dir()) } /// node: tests/completions.test.ts:81