From 8e1f03a4a0a701f5c547609849efbacb579d5497 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 13 Aug 2026 19:33:51 +0200 Subject: [PATCH] fix(gate): review round 4 --- four discriminators that constrained nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every item here is the same failure in a different place: a property fixed or claimed, and a suite that would stay green without it. **CANONICAL ANCESTRY WAS UNWITNESSED.** Round 3 fixed word-splitting and lexical traversal and verified both BY HAND, which is exactly the evidence that does not survive the next edit --- reverting to the `for _anc in $(... dirname ...)` loop left all 26 rows green. There is now a row driving a **space-containing root** (an unquoted expansion tears it into fragments and the real ancestor is never tested, so the guard passes on the path it must reject) and a **symlinked root** (a marker visible only after `pwd -P`, which is what `detect_project` sees). `M-G-8` restores the old loop and fails it. The traversal is encoded in framing §2a rather than left as an implementation detail. **THE SOCKET GUARD MISSED ITS OWN BOUNDARY.** The rows generated ~51- and ~71-byte paths against a 55-byte cutoff, so they constrained the guard's EXISTENCE and not its VALUE: raising the budget from 103 to 118 would have kept both green. They now hit **exactly 55 accepted and 56 rejected**, assert the measured byte lengths, and check that the refusal reports precisely one byte over. **REJECTION-CLEANUP CHECKED ONE AREA OF TWO.** Only `/tmp` was inspected, so leaking AMBIENT alone would have passed --- and AMBIENT is created before the guard can measure anything, which is the whole reason the trap moved earlier. Both areas are inspected now, the ambient one under the derived per-worktree target whose hashed name the test does not compute. **THE MULTIBYTE ROW DEPENDED ON THE INHERITED LOCALE.** Under `LC_ALL=C`, `${#var}` already counts bytes, so the character-counting mutant passed and the row's verdict was a property of the environment rather than of the code. It sets `LC_ALL=C.UTF-8` explicitly; `M-G-6` now fails even when the harness itself runs under `LC_ALL=C`. Stale test prose corrected, including one claim that was the very inference the ancestor check exists to refute: **placement under a managed root does not make a path marker-free**. Also, the tmp parent is SHARED between worktrees and untouched by `--prune` (unlike the ambient root, which is per-worktree), and the module header named framing revision 4. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/gate-script-framing.md | 11 ++ tests/gate_script_acceptance.rs | 271 ++++++++++++++++++++++++-------- 2 files changed, 218 insertions(+), 64 deletions(-) diff --git a/docs/gate-script-framing.md b/docs/gate-script-framing.md index b8839e7..659a1d9 100644 --- a/docs/gate-script-framing.md +++ b/docs/gate-script-framing.md @@ -477,6 +477,17 @@ Four decisions inside that, each of which had a cheaper wrong answer: the gate owns is *necessary, not sufficient*, so the precondition is verified rather than assumed. + **THE TRAVERSAL IS CANONICAL AND MUST NOT WORD-SPLIT**, and both + halves are contract rather than style. An unquoted `$(...)` + expansion splits on `IFS`, so a gate root containing a **space** + is torn into fragments and its real ancestor is never tested — + the guard then passes on exactly the path it exists to reject. + And `dirname` walks **lexical** ancestry while `detect_project` + canonicalizes, so a **symlinked** root hides a marker the editor + plainly sees; the gate and the editor must not disagree about the + same tree. The walk resolves with `pwd -P` first and iterates a + quoted loop, and both shapes are witnessed. + **MIRRORING THE NAMES IS NOT ENOUGH — the TYPES are part of the contract.** `match_marker` (`src/project.rs`) requires `.git` to be a **directory** and the seven language markers to be **files**, so diff --git a/tests/gate_script_acceptance.rs b/tests/gate_script_acceptance.rs index 453afc9..59de460 100644 --- a/tests/gate_script_acceptance.rs +++ b/tests/gate_script_acceptance.rs @@ -1,6 +1,7 @@ //! `scripts/gate` — the behaviour a shell script can be held to. //! -//! Framing: `docs/gate-script-framing.md` §4 (revision 4, approved). +//! Framing: `docs/gate-script-framing.md` §4, and §2a for the +//! `TMPDIR` isolation these rows cover (revision 6). //! //! # Why these tests exist, and why they are shaped like this //! @@ -513,9 +514,16 @@ fn the_isolated_tmpdir_reaches_a_spawned_child_under_the_managed_root() { }) .unwrap_or_else(|| panic!("the gate must announce its TMPDIR; stdout:\n{out}")); - // The contract is **under the managed target root**, which is what - // makes it both marker-free and disk-backed in production: the - // target root sits beside the build artifacts, not in `/tmp`. + // The contract is **under the managed gate root**, which is what + // makes it disk-backed in production: the gate root sits beside the + // build artifacts, not in `/tmp`. + // + // **Placement does NOT make it marker-free** — that is the false + // inference the ancestor check exists to correct, and an earlier + // version of this comment made it. A `.git` in `$HOME` or above + // `$HOME/build` re-roots fixtures just as `/tmp/.git` did. + // Marker-freeness is a separate, verified precondition; see + // `the_ancestor_check_honours_marker_types`. // // Deliberately NOT asserting `!starts_with("/tmp/")` here. This // test's own root is a `tempfile::tempdir()`, so on a normal @@ -587,7 +595,10 @@ fn the_isolated_tmpdir_is_reaped_when_the_run_ends() { "the exit trap must remove the TMPDIR even when a gate failed; \ {announced} survived" ); - // The parent stays: it is the per-worktree home the next run uses. + // The parent stays. It is SHARED between worktrees — it hangs off + // the gate root, not the derived per-worktree target, for the + // socket budget — and `--prune` never touches it, because prune + // only considers directories carrying an ownership marker. assert!( Path::new(&announced) .parent() @@ -722,93 +733,140 @@ fn the_ancestor_check_honours_marker_types() { ); } -/// The socket-path guard: rejection, acceptance, and cleanup on -/// rejection. +/// The socket-path guard, **at the boundary**: 55 bytes accepted, 56 +/// rejected, with the measured lengths asserted. /// -/// **The ordinary gate only ever exercises the passing side.** Every -/// other row here runs with a short root, so the guard is silent and a -/// broken guard would look identical. These construct the boundary -/// deliberately. +/// **Straddling the cutoff is not testing it.** An earlier version of +/// this row generated roughly 51- and 71-byte paths against a 55-byte +/// cutoff; raising `SUN_LEN_BUDGET` from 103 to 118 would have left +/// both green, so the row constrained the guard's existence and not its +/// value. These aim at 55 and 56 exactly and assert the byte lengths +/// they achieved, so a drifting budget fails here rather than in a +/// socket bind. /// /// The budget is **103 usable bytes** — Darwin's `sun_path[104]` minus -/// its terminating NUL, the supported-platform floor — less a 48-byte -/// fixture reserve, so a root whose derived TMPDIR exceeds 55 bytes is -/// refused. +/// its terminating NUL, the supported-platform floor — less the 48-byte +/// fixture reserve. #[test] -fn the_socket_path_guard_rejects_accepts_and_cleans_up() { - // The gate appends `/tmp/XXXXXX` (11 bytes) to the root, so a root - // of N bytes yields a TMPDIR of N + 11. - let over_root = long_root(60); +fn the_socket_path_guard_holds_at_its_exact_boundary() { + // The gate derives `/tmp/XXXXXX`, i.e. root + 11 bytes. + const DERIVED: usize = 11; + const CUTOFF: usize = 103 - 48; + + let ok_root = root_of_len(CUTOFF - DERIVED); + let out = run_guarded(ok_root.path()); + let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); + let announced = stdout + .lines() + .find_map(|l| { + l.split_once("gate: tmpdir") + .map(|(_, p)| p.trim().to_owned()) + }) + .unwrap_or_else(|| { + panic!( + "a root at the cutoff must be ACCEPTED and announce its \ + TMPDIR; stdout:\n{stdout}stderr:\n{}", + String::from_utf8_lossy(&out.stderr) + ) + }); + assert_eq!( + announced.len(), + CUTOFF, + "the accepted fixture must sit exactly ON the cutoff, not below \ + it — otherwise a widened budget still passes. Path: {announced}" + ); + + let over_root = root_of_len(CUTOFF - DERIVED + 1); let out = run_guarded(over_root.path()); let err = String::from_utf8_lossy(&out.stderr).into_owned(); assert!( - !out.status.success(), - "a root past the budget must be refused; stderr:\n{err}" + !out.status.success() && err.contains("too long for a unix socket path"), + "one byte past the cutoff must be REJECTED; stderr:\n{err}" ); - assert!( - err.contains("too long for a unix socket path"), - "and must say why; stderr:\n{err}" + // Keyed on the line that reports the measurement, not on the first + // `gate: ` line — that one is the path. + let measured = err + .lines() + .find(|l| l.contains("bytes, but a fixture needs")) + .and_then(|l| l.split_whitespace().nth(1)) + .and_then(|s| s.parse::().ok()); + assert_eq!( + measured, + Some(CUTOFF + 1), + "and the refusal must report exactly one byte over; stderr:\n{err}" ); - // REJECTION MUST NOT LEAK. The guard creates both temporary areas - // before it can measure anything, so a rejection that exits before - // the trap is armed leaves them behind — on every rejection, which - // is worse than the failure it prevents. - let leaked: Vec<_> = walkdir_shallow(over_root.path()); + // REJECTION MUST NOT LEAK, and **both** areas are checked. The + // guard creates the ambient root and the TMPDIR before it can + // measure anything, so a rejection that exits before the trap is + // armed leaves both behind. Checking only `/tmp` would pass + // while `gate-ambient` leaked. + let leaked = leaked_temp_areas(over_root.path()); assert!( leaked.is_empty(), - "a rejected run must reap what it created; found {leaked:?}" - ); - - // Just inside the budget: accepted. - let ok_root = long_root(40); - let out = run_guarded(ok_root.path()); - let err = String::from_utf8_lossy(&out.stderr).into_owned(); - assert!( - !err.contains("too long for a unix socket path"), - "a root inside the budget must not be refused; stderr:\n{err}" + "a rejected run must reap BOTH created areas; found {leaked:?}" ); } -/// The guard counts BYTES, not characters. +/// The guard counts BYTES, not characters — **under a UTF-8 locale**. /// -/// `${#var}` counts characters under a UTF-8 locale while `sun_path` is +/// `${#var}` counts characters in a UTF-8 locale while `sun_path` is /// byte-limited, so a multibyte path measures short and passes a check -/// it should fail. Each `é` here is one character and **two bytes**. +/// it should fail. **The locale is set explicitly**: under an inherited +/// `LC_ALL=C`, `${#var}` already counts bytes and the character-counting +/// mutant would pass, making this row's verdict depend on the +/// environment rather than on the code. #[test] fn the_socket_path_guard_counts_bytes_not_characters() { - // Character-length ~34 but byte-length ~68: rejected only if the - // guard measures bytes. + // Each `é` is one character and two bytes, so this root is under + // the cutoff by character count and over it by byte count — the + // only shape that separates the two implementations. let root = tempfile::Builder::new() - .prefix(&"é".repeat(24)) + .prefix(&"é".repeat(22)) .tempdir_in(short_root_base()) .expect("multibyte root"); - let chars = root.path().to_string_lossy().chars().count(); - let bytes = root.path().to_string_lossy().len(); + let path = root.path().to_string_lossy().into_owned(); + let chars = path.chars().count(); + let bytes = path.len(); assert!( - bytes > chars, - "fixture must actually be multibyte: {chars} chars, {bytes} bytes" + chars + 11 <= 55 && bytes + 11 > 55, + "fixture must straddle: {chars} chars (must pass) vs {bytes} \ + bytes (must fail), path {path}" ); - let out = run_guarded(root.path()); + let out = std::process::Command::new(gate()) + .arg("--self-test") + .current_dir(repo_root()) + .env("PMACS_GATE_TARGET_ROOT", root.path()) + .env("PMACS_GATE_ALLOW_ANCESTOR_MARKER", "1") + .env("LC_ALL", "C.UTF-8") + .env("LANG", "C.UTF-8") + .env_remove("TMPDIR") + .output() + .expect("run gate"); let err = String::from_utf8_lossy(&out.stderr).into_owned(); assert!( !out.status.success() && err.contains("too long for a unix socket path"), - "a multibyte root over the BYTE budget must be refused — it is \ - {chars} characters but {bytes} bytes; stderr:\n{err}" + "a root over the BYTE budget must be refused even though it is \ + under the CHARACTER budget ({chars} chars, {bytes} bytes); \ + stderr:\n{err}" ); } -/// A root of exactly `total` bytes, so the boundary can be aimed at. -fn long_root(total: usize) -> tempfile::TempDir { +/// A root whose full path is exactly `total` bytes. +fn root_of_len(total: usize) -> tempfile::TempDir { let base = short_root_base(); - // `/XXXXXX` - let fixed = base.display().to_string().len() + 1 + 6; - let pad = total.saturating_sub(fixed); - tempfile::Builder::new() - .prefix(&"a".repeat(pad)) + let fixed = base.display().to_string().len() + 1 + 6; // `/` + XXXXXX + let dir = tempfile::Builder::new() + .prefix(&"a".repeat(total.saturating_sub(fixed))) .tempdir_in(&base) - .expect("sized root") + .expect("sized root"); + assert_eq!( + dir.path().to_string_lossy().len(), + total, + "fixture must be exactly {total} bytes" + ); + dir } /// Run the gate with the ancestor escape set (so only the LENGTH guard @@ -824,17 +882,102 @@ fn run_guarded(root: &Path) -> std::process::Output { .expect("run gate") } -/// Entries left under `root/tmp` and any `gate-ambient` leaf. -fn walkdir_shallow(root: &Path) -> Vec { +/// Leftovers in **both** areas a run creates: `/tmp/*` and the +/// derived target's `gate-ambient/*`. +fn leaked_temp_areas(root: &Path) -> Vec { let mut out = Vec::new(); - for sub in ["tmp"] { - if let Ok(rd) = std::fs::read_dir(root.join(sub)) { - out.extend(rd.filter_map(Result::ok).map(|e| e.path())); + if let Ok(rd) = std::fs::read_dir(root.join("tmp")) { + out.extend(rd.filter_map(Result::ok).map(|e| e.path())); + } + // The ambient root DOES live under the derived per-worktree target + // (unlike the tmp parent, which is shared), and its name is a hash + // this test does not compute — so every `gate-ambient` beneath the + // root is inspected. + if let Ok(rd) = std::fs::read_dir(root) { + for entry in rd.filter_map(Result::ok) { + if let Ok(inner) = std::fs::read_dir(entry.path().join("gate-ambient")) { + out.extend(inner.filter_map(Result::ok).map(|e| e.path())); + } } } out } +/// The ancestor walk is **canonical** and **does not word-split**. +/// +/// Both properties were fixed without a witness, and reverting to the +/// obvious `for _anc in $(... dirname ...)` loop left every other row +/// green — so the suite constrained the check's existence and neither +/// of its two hard-won properties. +/// +/// * **A space in the root** is torn into fragments by an unquoted +/// `$(...)` expansion, and the real ancestor is then never tested — +/// the guard passes on exactly the path it must reject. +/// * **A symlinked root** hides a marker under lexical `dirname` that +/// `detect_project` sees after canonicalization, so the gate and the +/// editor would disagree about the same tree. +#[test] +fn the_ancestor_walk_is_canonical_and_does_not_word_split() { + // A space in the path, with a marker above it. + let spaced = tempfile::Builder::new() + .prefix("has space ") + .tempdir_in(short_root_base()) + .expect("spaced base"); + assert!( + spaced.path().to_string_lossy().contains(' '), + "fixture must actually contain a space" + ); + let marker = spaced.path().join(".git"); + std::fs::create_dir(&marker).expect("marker"); + let root = spaced.path().join("inner"); + std::fs::create_dir_all(&root).expect("root"); + + let out = run_unescaped(&root); + let err = String::from_utf8_lossy(&out.stderr).into_owned(); + assert!( + err.contains(&format!("{}", marker.display())), + "a marker above a root containing a SPACE must be found and \ + named; stderr:\n{err}" + ); + + // A symlinked root whose marker is only visible after resolving. + let base = tempfile::Builder::new() + .prefix("sym-") + .tempdir_in(short_root_base()) + .expect("sym base"); + let real = base.path().join("real"); + std::fs::create_dir_all(real.join("inner")).expect("real tree"); + let hidden = real.join(".git"); + std::fs::create_dir(&hidden).expect("hidden marker"); + let link = base.path().join("link"); + std::os::unix::fs::symlink(&real, &link).expect("symlink"); + + let out = run_unescaped(&link.join("inner")); + let err = String::from_utf8_lossy(&out.stderr).into_owned(); + assert!( + !out.status.success(), + "a marker reachable only after canonicalization must still \ + refuse; stderr:\n{err}" + ); + assert!( + err.contains(&format!("{}", hidden.display())), + "and must name it at its RESOLVED path, which is what \ + `detect_project` would see; stderr:\n{err}" + ); +} + +/// Run the gate with the ancestor check ACTIVE (no escape). +fn run_unescaped(root: &Path) -> std::process::Output { + std::process::Command::new(gate()) + .arg("--self-test") + .current_dir(repo_root()) + .env("PMACS_GATE_TARGET_ROOT", root) + .env_remove("PMACS_GATE_ALLOW_ANCESTOR_MARKER") + .env_remove("TMPDIR") + .output() + .expect("run gate") +} + /// The seam handoff §3 keeps authority over: a script cannot infer /// which acceptance suites a change touched, so it runs what it is /// handed — each one, in order.