diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index c30c0fc3..5d2b0a1e 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -395,3 +395,46 @@ 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 never the broken caller. + +#### The exit contract, narrowed after review — `scripts/fmt-workspaces-test.sh` + +ChatGPT Codex on PR #159 found that the contract above promised more than the +script maintained: both formatter invocations end `|| rc=1`, so **any** failure +after the `cargo fmt --version` probe was reported as "misformatted" even +though no verdict had been produced. The finding was correct and was measured +rather than assumed: + +- an **unparseable source** (`pub fn a( {{{`) exits **1** — identical to a diff; +- an **unknown `rustfmt.toml` option** is only a warning, and exits **0**. + +Of the two remedies offered, the contract was **narrowed** rather than the +failure re-classified, because parsing rustfmt's prose to tell a diff from a +parse error is precisely the check-adjacent-to-its-subject this repo keeps +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. + +Both directions are now pinned, and two further rivals were injected: + +- deleting the `cargo fmt --version` probe; +- turning `|| rc=1` into `|| return 2`, which collapses a verdict into the + usage code — the mirror of the original bug, and caught by three cases. + +**The first of those exposed a vacuous test, again only by injection.** The +original stub was a `cargo` that failed for every argument — but such a cargo +also breaks `cargo metadata`, so the run exited 2 on the *metadata* arm and the +probe could be deleted with the suite still green. A missing rustfmt component +is `cargo metadata` **succeeding** while only `cargo fmt` fails, so the stub now +delegates every other subcommand to the real cargo and fails `fmt` alone. Both +stubs are kept, because they exercise different arms. + +That is the third fixture correction on this one checker, all three found the +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. diff --git a/scripts/fmt-workspaces-test.sh b/scripts/fmt-workspaces-test.sh index a67fcb92..7e2f292d 100755 --- a/scripts/fmt-workspaces-test.sh +++ b/scripts/fmt-workspaces-test.sh @@ -121,6 +121,59 @@ 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 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 +# earns exit 2 is only that the gate never ran. A file must still be NAMED. +scaffold +printf 'pub fn a( {{{\n' > "$work/repo/nested/src/lib.rs" +if run_case "a source rustfmt cannot parse is a verdict, not a usage error" 1 "$work/repo"; then + case "$OUT" in + *nested/src/lib.rs*) ok " and the unparseable file is still named" ;; + *) bad " and the unparseable file is still named" "output: ${OUT:0:300}" ;; + esac +fi + +# The other half of the same contract: a cause that produces NO verdict and +# names NO file must be 2. A formatter stub that refuses to run is the honest +# injection -- `cargo` resolves, so this is not the `command -v` arm, and it +# reaches the `cargo fmt --version` probe the way a missing rustfmt component +# would. Note the stub dir cannot simply REPLACE PATH: the shebang resolves +# `bash` through it, so an empty PATH exits 127 before the script starts. +scaffold +mkdir -p "$work/stub" +printf '#!/usr/bin/env bash\nexit 1\n' > "$work/stub/cargo" +chmod +x "$work/stub/cargo" +PATH="$work/stub:$PATH" "$RUN" "$work/repo" >/dev/null 2>&1 +rc=$? +if [ "$rc" = 2 ]; then + ok "a cargo that cannot run at all is exit 2, not a verdict" +else + bad "a cargo that cannot run at all is exit 2" "exit $rc" +fi + +# That case alone is NOT enough, and injecting a rival proved it: a wholly +# broken cargo also breaks `cargo metadata`, so it exits 2 on the metadata arm +# and the rustfmt probe could be deleted with the suite still green. A MISSING +# RUSTFMT COMPONENT is the case the probe exists for -- metadata succeeds and +# only `cargo fmt` fails -- so the stub has to delegate everything else. +real_cargo="$(command -v cargo)" +scaffold +mkdir -p "$work/stub2" +{ + printf '#!/usr/bin/env bash\n' + printf 'if [ "${1:-}" = fmt ]; then echo "error: no such command: \`fmt\`" >&2; exit 101; fi\n' + printf 'exec %s "$@"\n' "$real_cargo" +} > "$work/stub2/cargo" +chmod +x "$work/stub2/cargo" +PATH="$work/stub2:$PATH" "$RUN" "$work/repo" >/dev/null 2>&1 +rc=$? +if [ "$rc" = 2 ]; then + ok "a missing rustfmt component is exit 2, not \"your code is misformatted\"" +else + bad "a missing rustfmt component is exit 2" "exit $rc -- the probe is gone or unreached" +fi + # --fix must actually rewrite, or the gate has a repair path that does nothing. scaffold printf '%s' "$DIRTY" > "$work/repo/nested/src/lib.rs" diff --git a/scripts/fmt-workspaces.sh b/scripts/fmt-workspaces.sh index 8fcbd7a6..abb6fec9 100755 --- a/scripts/fmt-workspaces.sh +++ b/scripts/fmt-workspaces.sh @@ -14,12 +14,20 @@ usage: fmt-workspaces.sh [--list] [--fix] [repo-root] --fix rewrite the offending files instead of reporting them exit 0 every workspace root is formatted -exit 1 a file is misformatted; the files are named on stdout -exit 2 usage error, or cargo could not be run -- NOT a formatting verdict +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. + +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. Exit 2 exists because `cargo fmt -p opfs-contract -- --check` from the repo -root exits 1 with `not a member of the workspace`, which is indistinguishable -from a real failure to a caller reading $?. Never collapse 2 into 1. +root exits 1 with `not a member of the workspace`, having checked nothing and +named nothing, which a caller reading $? cannot tell from a real failure. +Never collapse 2 into 1. EOF } @@ -117,6 +125,12 @@ main() { # which walks PAST a nested workspace boundary and reports the outer tree's # files under the inner root's name; and not the bare manifest-path form, # which silently checks only the root package and misses every member. + # `|| rc=1` deliberately does NOT try to separate a diff from a source + # rustfmt could not parse: both exit 1 and both NAME A FILE, so both are + # facts about the tree. Measured: an unclosed delimiter exits 1, and an + # unknown rustfmt.toml option is only a warning (exit 0). What exit 2 is + # reserved for is "the gate did not run", and every such cause is checked + # ABOVE this loop. Reasoning: context/test-evidence/tooling.md. local -a flags=() for member in "${pkgs[@]}"; do flags+=(-p "$member"); done if [ "$mode" = --fix ]; then