From 7cc52bdb1d1c5b6d12e4a774a71f834dbe9d2f2a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:51:59 -0700 Subject: [PATCH 1/3] fmt-workspaces-test: one fault case per fixture package, root included (improvise-90ea) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- scripts/fmt-workspaces-test.sh | 55 +++++++++++++++++----------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/scripts/fmt-workspaces-test.sh b/scripts/fmt-workspaces-test.sh index 7e2f292d..8ad50dd0 100755 --- a/scripts/fmt-workspaces-test.sh +++ b/scripts/fmt-workspaces-test.sh @@ -76,36 +76,37 @@ run_case() { scaffold run_case "both workspaces clean is a pass" 0 "$work/repo" -# The load-bearing case. `cargo fmt --all` at the outer root cannot reach this -# file, and `cargo fmt -p inner-pkg` from the outer root exits 1 saying `not a -# member of the workspace` -- the usage error this whole script exists to split -# from a real verdict. +# One fault case PER PACKAGE, enumerated from the fixture rather than +# hand-picked: three hand-picked shapes each missed a rival, and the fourth +# missed the root package itself (improvise-90ea). Reasoning: +# context/test-evidence/tooling.md. The enumeration is a find over +# `[package]` manifests, deliberately NOT the script's own `[workspace]` + +# `cargo metadata` discovery, which is the thing under test. scaffold -printf '%s' "$DIRTY" > "$work/repo/nested/src/lib.rs" -if run_case "a fault in the NESTED workspace is reported" 1 "$work/repo"; then - case "$OUT" in - *nested/src/lib.rs*) ok " and the nested file is named" ;; - *) bad " and the nested file is named" "output did not name it: ${OUT:0:300}" ;; - esac +pkg_manifests=() +while IFS= read -r m; do pkg_manifests+=("$m"); done < <( + cd "$work/repo" && grep -rl --include=Cargo.toml '^\[package\]' . | sort) +# An empty or shrunken enumeration would make the loop below vacuous. +if [ "${#pkg_manifests[@]}" = 3 ]; then + ok "the fixture enumerates all three packages (root, member, nested)" +else + bad "the fixture enumerates all three packages" "found: ${pkg_manifests[*]:-none}" fi - -scaffold -printf '%s' "$DIRTY" > "$work/repo/a/src/lib.rs" -if run_case "a fault in the OUTER workspace is reported" 1 "$work/repo"; then - case "$OUT" in - *a/src/lib.rs*) ok " and the outer file is named" ;; - *) bad " and the outer file is named" "output did not name it: ${OUT:0:300}" ;; - esac - # The rival: spelling the per-root check `--all` instead of `-p ` - # walks PAST the nested workspace boundary (measured), so the OUTER file is - # reported a second time under the inner root. Counting is what sees that. - n="$(printf '%s\n' "$OUT" | grep -c 'a/src/lib.rs')" - if [ "$n" = 1 ]; then - ok " and it is named ONCE -- no root reaches past its own boundary" - else - bad " and it is named ONCE" "named $n times; a root is walking past its boundary" +for m in "${pkg_manifests[@]}"; do + rel="$(dirname "${m#./}")/src/lib.rs"; rel="${rel#./}" + scaffold + printf '%s' "$DIRTY" > "$work/repo/$rel" + if run_case "a fault in $rel is reported" 1 "$work/repo"; then + # Counting, not matching: `--all` for `-p` walks PAST the nested boundary + # (measured) and names an outer file a second time under the inner root. + n="$(printf '%s\n' "$OUT" | grep -cF "$work/repo/$rel")" + if [ "$n" = 1 ]; then + ok " and $rel is named exactly once" + else + bad " and $rel is named exactly once" "named $n times; output: ${OUT:0:300}" + fi fi -fi +done # A usage error must NOT wear a formatting failure's exit code. This is the # whole ticket: `cargo fmt -p opfs-contract` from the repo root exits 1. From 808cbb7d92e9920ccba616612faed0e111596633 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:55:25 -0700 Subject: [PATCH 2/3] fmt-workspaces: refuse a package no workspace root reaches (improvise-90ea) The fixture loop covers every package the fixture has; this covers every package the tree has. The review's skip-the-root rival now fails the real repo's own gate (exit 2 naming Cargo.toml), not only the test suite. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/tooling.md | 63 ++++++++++++++++++++++++++++++++ scripts/fmt-workspaces-test.sh | 26 +++++++++++++ scripts/fmt-workspaces.sh | 59 +++++++++++++++++++++--------- 3 files changed, 131 insertions(+), 17 deletions(-) diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 5d2b0a1e..bda78aae 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -391,6 +391,8 @@ green suite over a vacuous assertion. **What is not covered.** The script asks `cargo metadata` for members, so a root whose metadata succeeds but whose member list is wrong is not detectable here. +*Narrowed by improvise-90ea, below:* a list that **omits** a package in the tree +is now refused; one that names a package it should not is still not detected. And `harness/opfs-contract`'s own `cargo fmt -p opfs-contract` step in the `harness-lint` job is left as it is: it resolves correctly because that step sets `working-directory`, and the ticket's own note confirms CI as written was @@ -438,3 +440,64 @@ same way: the suite passed *under a rival*. The generalisable rule is that a stub must fail in the **shape** of the real fault, not merely fail — a stub that breaks more than the real thing breaks satisfies the test through the wrong arm, and reads exactly like coverage. + +#### The root package, and why the fourth fix is structural — `scripts/fmt-workspaces-test.sh` + +A post-merge review of PR #159 found a **fourth** fixture vacuity +(improvise-90ea). The fixture's outer root is a package (`root-pkg`) as well as +a workspace, which is right, but no case ever dirtied **the root package's own +`src/lib.rs`**. Only the member `a/` and `nested/` were dirtied. So a script +that formats every member *except* the root package passed 17/17. In this repo +the root package is `improvise`, the whole TUI. Reproduced at `40b8913` with +the review's rival in `members_of` (drop the package whose manifest sits at +`workspace_root`): the suite passed 17/17 and `--list` left `improvise` out. + +Adding one more hand-picked case would have fixed the fourth and not the fifth. +Four vacuities in one checker all have the same cause: **the cases were chosen +by hand, one per hazard somebody had thought of.** So the fix is structural, +and it has two halves because it has to cover two sets of packages: + +- **Every package the fixture has.** The two hand-picked fault cases are + replaced by a loop over every `[package]` manifest in the fixture. Each one + gets its own misformatted `src/lib.rs`, must exit 1, and must be named + **exactly once**, so the `--all` boundary check now covers every package + rather than one. The enumeration is a `grep` for `[package]`, deliberately + *not* the script's `[workspace]` + `cargo metadata` discovery, since that + discovery is what is under test. A count assertion (three packages) stops + the loop going vacuous if the fixture loses a package. A package added to + the fixture gets its case automatically. +- **Every package the tree has.** The loop proves only that the script handles + the fixture's *shapes*. A real tree can hold a package no root lists (an + `exclude`d crate, say), and nothing in the fixture would say so. So the gate + itself now checks an invariant **before it formats anything**: every + `[package]` manifest the find reaches must be a member of some root, or it + exits **2** and names the package. That is exit 2 and not 1 because no + verdict was produced for that package, which is what exit 2 means here. + Paths are compared through `realpath`, because on macOS `mktemp` lives under + `/var` while cargo reports `/private/var`. + +**Rivals injected, and where each failed** (the suite went from 17 to 21 cases): + +- **The review's skip-the-root rival.** It now fails the fixture at `a fault in + src/lib.rs is reported` (exit 0). With the invariant in place it fails + **six** cases (exit 2, "no workspace root reaches"). It also fails the + **real repo** with no fixture involved: `fmt-workspaces.sh` exits 2 naming + `Cargo.toml`. That last one is the property worth having. The gate refuses + its own blindness in CI, not only in its test suite. +- **Disabling the invariant** (`if false`). It fails `a package no workspace + root reaches is exit 2, not a pass` with **exit 0**, even though that + package's `src/lib.rs` is misformatted. That exit 0 *is* the hazard: an + unreached package is silently clean. +- **The two earlier rivals, re-run against the loop.** `--all` for `-p` fails + both `a/src/lib.rs` and `src/lib.rs` "named exactly once". The second of + those was not predicted: the nested root's `--all` also reaches the root + package. The bare manifest-path form fails `a fault in a/src/lib.rs is + reported`. + +The real-repo control from the ticket still holds: a misformatted fn appended +to `src/ui/text.rs` is reported (`Diff in …/src/ui/text.rs:72`). + +**What is still not covered.** A root whose member list *includes* something it +should not is not detected, and a package in a directory the find excludes +(`target/`, `.claude/`, `node_modules/`) is outside the invariant by +construction. diff --git a/scripts/fmt-workspaces-test.sh b/scripts/fmt-workspaces-test.sh index 8ad50dd0..ce15d6ce 100755 --- a/scripts/fmt-workspaces-test.sh +++ b/scripts/fmt-workspaces-test.sh @@ -122,6 +122,32 @@ run_case "a tree with no workspace root is a usage error, not a pass" 2 "$work/e run_case "--help is a usage error, so a typo cannot pass" 2 "$work/repo" --help +# The loop above covers every package the FIXTURE has; this covers every +# package the TREE has. A package no root lists is checked by nothing, so the +# gate must refuse a verdict -- and name the package -- rather than pass. +scaffold +mkdir -p "$work/repo/orphan/src" +printf '[package]\nname = "orphan-pkg"\nversion = "0.1.0"\nedition = "2021"\n' \ + > "$work/repo/orphan/Cargo.toml" +printf '%s' "$DIRTY" > "$work/repo/orphan/src/lib.rs" +cat > "$work/repo/Cargo.toml" <<'EOF' +[workspace] +members = ["a"] +exclude = ["orphan"] +resolver = "2" + +[package] +name = "root-pkg" +version = "0.1.0" +edition = "2021" +EOF +if run_case "a package no workspace root reaches is exit 2, not a pass" 2 "$work/repo"; then + case "$OUT" in + *"no workspace root reaches"*orphan/Cargo.toml*) ok " and the unreached package is named" ;; + *) bad " and the unreached package is named" "output: ${OUT:0:300}" ;; + esac +fi + # The narrowed contract, pinned so it is not merely prose (ChatGPT Codex on # PR #159). rustfmt exits 1 for a source it cannot parse, exactly as for a # diff -- so exit 1 is "rustfmt did not certify this root clean", and what diff --git a/scripts/fmt-workspaces.sh b/scripts/fmt-workspaces.sh index abb6fec9..982d2211 100755 --- a/scripts/fmt-workspaces.sh +++ b/scripts/fmt-workspaces.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash # Format-checks every Cargo workspace root in the repo, from any directory. -# `cargo fmt --all` reaches only ONE of them. Reasoning: context/code/tooling.md -# section 4 -- including why `-p` scoped by `--manifest-path` is the only +# `cargo fmt --all` reaches only ONE of them. Reasoning: context/test-evidence/ +# tooling.md, "The two-workspace fmt gate" -- including why `-p` scoped by `--manifest-path` is the only # spelling that is neither blind nor boundary-crossing (improvise-b4ks). set -uo pipefail @@ -18,8 +18,8 @@ exit 1 rustfmt did not certify a root clean: a file differs from rustfmt's output, or rustfmt refused to parse one. Either way a FILE IS NAMED and the tree is what is wrong. exit 2 the gate did not run: usage error, no cargo, no rustfmt, cargo - metadata failed, or no [workspace] root found. NO verdict was - produced and NO file is named. + metadata failed, no [workspace] root found, or a [package] that no + root has as a member. NO verdict was produced and NO file is named. The split is "did the gate run", not "is every exit-1 cause a diff". That is narrower than it sounds and it is deliberate -- see the note on rc=1 below. @@ -38,8 +38,8 @@ EOF # The find is RELATIVE to the tree being scanned: an absolute `-path` filter # matches the ancestors too, and this repo is itself checked out under # `.claude/worktrees/` in every agent worktree, where that excluded everything. -workspace_roots() { - local root="$1" +manifests_with() { + local root="$1" section="$2" ( cd "$root" && find . -name Cargo.toml \ -not -path './target/*' -not -path '*/target/*' \ -not -path './.git/*' \ @@ -47,11 +47,12 @@ workspace_roots() { -not -path './.claude/*' ) \ | sort \ | while IFS= read -r rel; do - if grep -q '^\[workspace\]' "$root/${rel#./}"; then + if grep -q "^\\[$section\\]" "$root/${rel#./}"; then printf '%s\n' "$root/${rel#./}" fi done } +workspace_roots() { manifests_with "$1" workspace; } # The members of ONE workspace, named by cargo rather than parsed out of the # manifest: a `members = [...]` list may carry globs and path dependencies. @@ -61,10 +62,10 @@ members_of() { printf '%s' "$meta" | python3 -c ' import json, sys doc = json.load(sys.stdin) -names = [p["name"] for p in doc["packages"]] -if not names: +rows = [p["name"] + "\t" + p["manifest_path"] for p in doc["packages"]] +if not rows: sys.exit(1) -print("\n".join(names)) +print("\n".join(rows)) ' || return 1 } @@ -106,15 +107,39 @@ main() { return 2 fi - local rc=0 manifest member - for manifest in "${roots[@]}"; do - local -a pkgs=() - while IFS= read -r member; do [ -n "$member" ] && pkgs+=("$member"); done \ - < <(members_of "$manifest") - if [ ${#pkgs[@]} -eq 0 ]; then - printf 'fmt-workspaces: cargo metadata failed for %s\n' "$manifest" >&2 + # Pass 1: every root's members, before anything is formatted, so that a + # package NO root reaches is exit 2 rather than silently never checked. + local rc=0 manifest member i m + local -a members=() covered=() + for i in "${!roots[@]}"; do + local rows + rows="$(members_of "${roots[$i]}")" || rows="" + if [ -z "$rows" ]; then + printf 'fmt-workspaces: cargo metadata failed for %s\n' "${roots[$i]}" >&2 return 2 fi + members[$i]="$(printf '%s\n' "$rows" | cut -f1 | tr '\n' ' ')" + while IFS= read -r m; do covered+=("$m"); done < <(printf '%s\n' "$rows" | cut -f2) + done + local orphans + orphans="$(manifests_with "$root" package | python3 -c ' +import os, sys +covered = {os.path.realpath(p) for p in sys.argv[1:]} +for line in sys.stdin: + m = line.rstrip("\n") + if m and os.path.realpath(m) not in covered: + print(m) +' "${covered[@]}")" + if [ -n "$orphans" ]; then + printf 'fmt-workspaces: no workspace root reaches these packages, so nothing checks them:\n%s\n' \ + "$orphans" >&2 + return 2 + fi + + for i in "${!roots[@]}"; do + manifest="${roots[$i]}" + local -a pkgs=() + read -r -a pkgs <<< "${members[$i]}" if [ "$list" = 1 ]; then printf '%s: %s\n' "$manifest" "${pkgs[*]}" From 625c11319a93b01c7822300ca3e0c1fb714c06b2 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:09:46 -0700 Subject: [PATCH 3/3] fmt-workspaces: exit 2 means no verdict, not no output (improvise-ximf) The unreached-package arm exits 2 and names the package's Cargo.toml, so the usage text's "NO file is named" was false for it. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/tooling.md | 8 +++++--- scripts/fmt-workspaces.sh | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index bda78aae..c49691d2 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -417,9 +417,11 @@ getting wrong. The line the split actually needs is **"did the gate run"**: - exit 1 — rustfmt did not certify a root clean. A diff or a refusal to parse; either way **a file is named** and the tree is what is wrong. - exit 2 — the gate did not run. Usage, no cargo, no rustfmt, `cargo metadata` - failed, no `[workspace]` root. **No file is named.** Every such cause is - checked *above* the loop, which is what makes the narrowing true rather than - hopeful. + failed, no `[workspace]` root. **No source file is named.** Every such cause + is checked *above* the loop, which is what makes the narrowing true rather + than hopeful. (Since improvise-90ea, a package no root reaches is also exit 2, + and it names that package's `Cargo.toml`, which is the reason no verdict + exists. The line is "no verdict", not "no output"; improvise-ximf.) Both directions are now pinned, and two further rivals were injected: diff --git a/scripts/fmt-workspaces.sh b/scripts/fmt-workspaces.sh index 982d2211..98bc8370 100755 --- a/scripts/fmt-workspaces.sh +++ b/scripts/fmt-workspaces.sh @@ -19,7 +19,8 @@ exit 1 rustfmt did not certify a root clean: a file differs from rustfmt's and the tree is what is wrong. exit 2 the gate did not run: usage error, no cargo, no rustfmt, cargo metadata failed, no [workspace] root found, or a [package] that no - root has as a member. NO verdict was produced and NO file is named. + root has as a member. NO verdict was produced. No SOURCE file is + named; the unreached-package case names its Cargo.toml. The split is "did the gate run", not "is every exit-1 cause a diff". That is narrower than it sounds and it is deliberate -- see the note on rc=1 below.