Narrow the fmt gate's exit contract to what it actually maintains
ChatGPT Codex on PR #159: both formatter invocations end `|| rc=1`, so any failure after the rustfmt probe was reported as "misformatted" although no verdict was produced. Correct, and the narrower form of the bug this gate exists for. Measured: an unparseable source exits 1, identical to a diff; an unknown rustfmt.toml option is only a warning (exit 0). Narrowed rather than re-classified, because telling a diff from a parse error means parsing rustfmt's prose, which is the check-adjacent-to-its-subject this repo keeps getting wrong. The line the split needs is "did the gate run": exit 1 names a file and blames the tree; exit 2 names none and blames the invocation, and every exit-2 cause is checked above the loop. Two more rivals injected: deleting the `cargo fmt --version` probe, and turning `|| rc=1` into `|| return 2`. The first exposed a vacuous test -- a cargo stub that failed for every argument also breaks `cargo metadata`, so the run exited 2 on the metadata arm and the probe could be deleted with the suite green. The stub now delegates every subcommand but `fmt`, which is the shape a missing rustfmt component actually has. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user