Split a fmt usage error from a fmt verdict across both workspace roots
improvise-b4ks. From the repo root `cargo fmt -p opfs-contract -- --check`
exits 1 with "package `opfs-contract` is not a member of the workspace" and
never runs --check, so a caller reading $? cannot tell "misformatted" from
"nothing was checked". Reproduced at 7cef9f8.
scripts/fmt-workspaces.sh discovers every [workspace] root and checks each
with `-p <member>` scoped by that root's own --manifest-path. Exit 2 is a
usage error or an unrunnable cargo; exit 1 is a formatting verdict.
Both obvious spellings were measured and both fail in the direction that
looks like success: `--all` crosses the boundary, because it formats local
path dependencies and harness/opfs-contract depends on improvise-io by path
(an injected fault in src/draw.rs was reported by the harness root); and the
bare --manifest-path form is blind to workspace members, because the repo
root is a package as well as a workspace (an injected fault in
crates/improvise-core exited 0).
scripts/fmt-workspaces-test.sh is the check_the_checker. Four rivals were
injected and each failed it. Its fixture had to be corrected twice -- a
missing path dependency and a virtual outer manifest -- and both vacuities
were found only by injecting the rival, never by reading.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -140,7 +140,10 @@ jobs:
|
|||||||
with:
|
with:
|
||||||
components: clippy, rustfmt
|
components: clippy, rustfmt
|
||||||
- uses: Swatinem/rust-cache@v2
|
- uses: Swatinem/rust-cache@v2
|
||||||
- run: cargo fmt --all -- --check
|
# NOT `cargo fmt --all`, which reaches only ONE of the two workspace
|
||||||
|
# roots. Exit 2 here is a usage error, never a verdict (improvise-b4ks).
|
||||||
|
- name: Every workspace root is formatted
|
||||||
|
run: ./scripts/fmt-workspaces.sh
|
||||||
- run: cargo clippy --workspace --all-targets -- -D warnings
|
- run: cargo clippy --workspace --all-targets -- -D warnings
|
||||||
- run: cargo test --workspace
|
- run: cargo test --workspace
|
||||||
# The comment cap and the test-evidence keys (improvise-9c8c); without
|
# The comment cap and the test-evidence keys (improvise-9c8c); without
|
||||||
@@ -275,8 +278,10 @@ jobs:
|
|||||||
# one guard whose absence is invisible until a tag is public and
|
# one guard whose absence is invisible until a tag is public and
|
||||||
# immutable. pr-reviews stands between a merge and an unread review, and
|
# immutable. pr-reviews stands between a merge and an unread review, and
|
||||||
# was blind to inline comments for as long as it existed (improvise-xgra).
|
# was blind to inline comments for as long as it existed (improvise-xgra).
|
||||||
|
# fmt-workspaces is here because its own failure mode is the bug it
|
||||||
|
# exists for: a usage error wearing a formatting failure's exit code.
|
||||||
- name: The remaining checkers refuse what they claim to
|
- name: The remaining checkers refuse what they claim to
|
||||||
run: ./scripts/checkers.sh pr-watch release-preflight pr-reviews
|
run: ./scripts/checkers.sh pr-watch release-preflight pr-reviews fmt-workspaces
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# Lints the one crate the `test` job cannot reach.
|
# Lints the one crate the `test` job cannot reach.
|
||||||
|
|||||||
@@ -338,3 +338,60 @@ allowlist is checked only in the sense that seven real suites pass through it;
|
|||||||
there is no case asserting a *particular* name is absent beyond `name`. A suite
|
there is no case asserting a *particular* name is absent beyond `name`. A suite
|
||||||
that grows a dependency on some other inherited variable will fail loudly on
|
that grows a dependency on some other inherited variable will fail loudly on
|
||||||
first run rather than be caught here. Stated rather than guarded.
|
first run rather than be caught here. Stated rather than guarded.
|
||||||
|
|
||||||
|
### The two-workspace fmt gate — `scripts/fmt-workspaces-test.sh`
|
||||||
|
|
||||||
|
The bug (improvise-b4ks) was a **gate decoupled from what it checks while
|
||||||
|
wearing the exit code of a real failure**. From the repo root,
|
||||||
|
`cargo fmt -p opfs-contract -- --check` exits **1** with `package
|
||||||
|
'opfs-contract' is not a member of the workspace` — `--check` never runs. A
|
||||||
|
caller reading `$?` cannot tell "this crate is misformatted" from "nothing was
|
||||||
|
checked", so the gate could neither report success when the crate was dirty nor
|
||||||
|
failure when it was clean. Reproduced at `7cef9f8`, exit 1, before any fix.
|
||||||
|
|
||||||
|
`scripts/fmt-workspaces.sh` replaces it: it discovers every `[workspace]` root
|
||||||
|
and checks each with `-p <member>` scoped by that root's own `--manifest-path`.
|
||||||
|
Exit **2** is a usage error or a cargo that could not run; **1** is a formatting
|
||||||
|
verdict; they are never the same number.
|
||||||
|
|
||||||
|
**Why `-p` and not the two obvious spellings** — both were measured, and both
|
||||||
|
fail in a direction that looks like success:
|
||||||
|
|
||||||
|
- `--manifest-path <root> --all` **crosses the workspace boundary**. `--all`
|
||||||
|
formats "all packages, and also their local path-based dependencies", and
|
||||||
|
`harness/opfs-contract` depends on `improvise-io` by path, so the inner root
|
||||||
|
reports the *outer* tree's files. Measured: an injected fault in `src/draw.rs`
|
||||||
|
was reported by the harness root's own invocation.
|
||||||
|
- `--manifest-path <root>` bare is **blind to the members**. The repo root is a
|
||||||
|
package (`improvise`) as well as a workspace, so the bare form checks only
|
||||||
|
that package. Measured: an injected fault in `crates/improvise-core/src/format.rs`
|
||||||
|
exited **0**.
|
||||||
|
|
||||||
|
**Four rivals were injected into the script and each failed the suite**:
|
||||||
|
`--all` for `-p` (the boundary leak, caught by counting — the outer file must be
|
||||||
|
named exactly once); the bare manifest-path (caught by the outer-fault case);
|
||||||
|
`return 2` collapsed to `return 1` on the usage arms (two cases); and "no
|
||||||
|
workspace root found" returning 0 instead of 2.
|
||||||
|
|
||||||
|
**The fixture had to be made faithful twice, and each vacuity was found only by
|
||||||
|
injecting the rival — not by reading.** The suite passed under a rival at both
|
||||||
|
stages, which is the only reason the gaps were visible:
|
||||||
|
|
||||||
|
- The first fixture's nested crate had **no path dependency** into the outer
|
||||||
|
tree, so `--all` had nothing to leak through and the boundary rival passed.
|
||||||
|
- The first fixture's outer root was a **virtual manifest** with no `[package]`,
|
||||||
|
where the bare form happens to reach every member, so that rival passed too.
|
||||||
|
|
||||||
|
Both are properties this repo really has, and a fixture that omits them tests a
|
||||||
|
repository that does not exist. That is the generalisable point: **a
|
||||||
|
check_the_checker's fixture must reproduce the structural features the hazard
|
||||||
|
depends on, and the only way to know it does is to inject the rival and watch
|
||||||
|
it fail.** A green suite over a simplified fixture is the same non-evidence as a
|
||||||
|
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.
|
||||||
|
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.
|
||||||
|
|||||||
Executable
+137
@@ -0,0 +1,137 @@
|
|||||||
|
#!/usr/bin/env bash
|
||||||
|
# check_the_checker for scripts/fmt-workspaces.sh. Every case MAKES the checked-
|
||||||
|
# for condition happen: a real misformatted file in a real nested workspace,
|
||||||
|
# which is what the documented `-p` spelling could never report (improvise-b4ks).
|
||||||
|
set -uo pipefail
|
||||||
|
|
||||||
|
here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
||||||
|
RUN="$here/fmt-workspaces.sh"
|
||||||
|
work="$(mktemp -d)"
|
||||||
|
trap 'rm -rf "$work"' EXIT
|
||||||
|
|
||||||
|
pass=0; fail=0
|
||||||
|
ok() { printf ' ok %s\n' "$1"; pass=$((pass+1)); }
|
||||||
|
bad() { printf ' FAIL %s\n %s\n' "$1" "$2"; fail=$((fail+1)); }
|
||||||
|
|
||||||
|
CLEAN='pub fn probe() -> u32 {
|
||||||
|
1
|
||||||
|
}
|
||||||
|
'
|
||||||
|
DIRTY='pub fn probe( ) ->u32{ 1 }
|
||||||
|
'
|
||||||
|
|
||||||
|
# Two workspace roots, exactly as this repo has: an outer one with a member
|
||||||
|
# crate, and `nested/` carrying its own [workspace] plus a PATH DEPENDENCY back
|
||||||
|
# into the outer tree -- which is what makes `--all` leak (harness/opfs-contract
|
||||||
|
# depends on improvise-io the same way, and `--all` formats path deps too).
|
||||||
|
scaffold() {
|
||||||
|
rm -rf "$work/repo"
|
||||||
|
mkdir -p "$work/repo/src" "$work/repo/a/src" "$work/repo/nested/src"
|
||||||
|
cat > "$work/repo/Cargo.toml" <<'EOF'
|
||||||
|
[workspace]
|
||||||
|
members = ["a"]
|
||||||
|
resolver = "2"
|
||||||
|
|
||||||
|
[package]
|
||||||
|
name = "root-pkg"
|
||||||
|
version = "0.1.0"
|
||||||
|
edition = "2021"
|
||||||
|
EOF
|
||||||
|
cat > "$work/repo/a/Cargo.toml" <<'EOF'
|
||||||
|
[package]
|
||||||
|
name = "outer-pkg"
|
||||||
|
version = "0.1.0"
|
||||||
|
edition = "2021"
|
||||||
|
EOF
|
||||||
|
cat > "$work/repo/nested/Cargo.toml" <<'EOF'
|
||||||
|
[workspace]
|
||||||
|
|
||||||
|
[package]
|
||||||
|
name = "inner-pkg"
|
||||||
|
version = "0.1.0"
|
||||||
|
edition = "2021"
|
||||||
|
|
||||||
|
[dependencies]
|
||||||
|
outer-pkg = { path = "../a" }
|
||||||
|
EOF
|
||||||
|
printf '%s' "$CLEAN" > "$work/repo/src/lib.rs"
|
||||||
|
printf '%s' "$CLEAN" > "$work/repo/a/src/lib.rs"
|
||||||
|
printf '%s' "$CLEAN" > "$work/repo/nested/src/lib.rs"
|
||||||
|
}
|
||||||
|
|
||||||
|
# $1 name, $2 expected exit, rest: args. Captures stdout+stderr into $OUT.
|
||||||
|
OUT=""
|
||||||
|
run_case() {
|
||||||
|
local name="$1" want="$2"; shift 2
|
||||||
|
OUT="$("$RUN" "$@" 2>&1)"
|
||||||
|
local got=$?
|
||||||
|
if [ "$got" != "$want" ]; then
|
||||||
|
bad "$name" "exit $got, wanted $want; output: ${OUT:0:300}"
|
||||||
|
return 1
|
||||||
|
fi
|
||||||
|
ok "$name"
|
||||||
|
return 0
|
||||||
|
}
|
||||||
|
|
||||||
|
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.
|
||||||
|
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
|
||||||
|
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 <members>`
|
||||||
|
# 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"
|
||||||
|
fi
|
||||||
|
fi
|
||||||
|
|
||||||
|
# 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.
|
||||||
|
scaffold
|
||||||
|
run_case "an unknown option is a usage error, not a verdict" 2 "$work/repo" --nope
|
||||||
|
run_case "a second repo-root is a usage error" 2 "$work/repo" "$work/repo"
|
||||||
|
run_case "a directory that does not exist is a usage error" 2 "$work/nowhere"
|
||||||
|
|
||||||
|
# "Nothing to check" must not read as "everything is clean": a find that matches
|
||||||
|
# nothing is how a moved directory turns this gate off silently.
|
||||||
|
mkdir -p "$work/empty"
|
||||||
|
run_case "a tree with no workspace root is a usage error, not a pass" 2 "$work/empty"
|
||||||
|
|
||||||
|
run_case "--help is a usage error, so a typo cannot pass" 2 "$work/repo" --help
|
||||||
|
|
||||||
|
# --fix must actually rewrite, or the gate has a repair path that does nothing.
|
||||||
|
scaffold
|
||||||
|
printf '%s' "$DIRTY" > "$work/repo/nested/src/lib.rs"
|
||||||
|
if run_case "--fix rewrites a nested file" 0 "$work/repo" --fix; then
|
||||||
|
printf '%s' "$CLEAN" > "$work/want.rs"
|
||||||
|
if cmp -s "$work/want.rs" "$work/repo/nested/src/lib.rs"; then
|
||||||
|
ok " and the file now matches rustfmt"
|
||||||
|
else
|
||||||
|
bad " and the file now matches rustfmt" "still: $(cat "$work/repo/nested/src/lib.rs")"
|
||||||
|
fi
|
||||||
|
fi
|
||||||
|
|
||||||
|
printf '\nfmt-workspaces-test: %d passed, %d failed\n' "$pass" "$fail"
|
||||||
|
[ "$fail" -eq 0 ]
|
||||||
Executable
+131
@@ -0,0 +1,131 @@
|
|||||||
|
#!/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
|
||||||
|
# spelling that is neither blind nor boundary-crossing (improvise-b4ks).
|
||||||
|
set -uo pipefail
|
||||||
|
|
||||||
|
usage() {
|
||||||
|
cat >&2 <<'EOF'
|
||||||
|
usage: fmt-workspaces.sh [--list] [--fix] [repo-root]
|
||||||
|
|
||||||
|
repo-root the tree to scan; defaults to the directory above this script
|
||||||
|
--list print each workspace root and its members, then exit
|
||||||
|
--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 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.
|
||||||
|
EOF
|
||||||
|
}
|
||||||
|
|
||||||
|
# `[workspace]` at the start of a line, in a Cargo.toml that is not vendored
|
||||||
|
# build output. `harness/opfs-contract` carries an empty one, so it is its own
|
||||||
|
# root and the outer `--all` never reaches it.
|
||||||
|
#
|
||||||
|
# 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"
|
||||||
|
( cd "$root" && find . -name Cargo.toml \
|
||||||
|
-not -path './target/*' -not -path '*/target/*' \
|
||||||
|
-not -path './.git/*' \
|
||||||
|
-not -path '*/node_modules/*' \
|
||||||
|
-not -path './.claude/*' ) \
|
||||||
|
| sort \
|
||||||
|
| while IFS= read -r rel; do
|
||||||
|
if grep -q '^\[workspace\]' "$root/${rel#./}"; then
|
||||||
|
printf '%s\n' "$root/${rel#./}"
|
||||||
|
fi
|
||||||
|
done
|
||||||
|
}
|
||||||
|
|
||||||
|
# The members of ONE workspace, named by cargo rather than parsed out of the
|
||||||
|
# manifest: a `members = [...]` list may carry globs and path dependencies.
|
||||||
|
members_of() {
|
||||||
|
local manifest="$1" meta
|
||||||
|
meta="$(cargo metadata --manifest-path "$manifest" --no-deps --format-version 1 2>/dev/null)" || return 1
|
||||||
|
printf '%s' "$meta" | python3 -c '
|
||||||
|
import json, sys
|
||||||
|
doc = json.load(sys.stdin)
|
||||||
|
names = [p["name"] for p in doc["packages"]]
|
||||||
|
if not names:
|
||||||
|
sys.exit(1)
|
||||||
|
print("\n".join(names))
|
||||||
|
' || return 1
|
||||||
|
}
|
||||||
|
|
||||||
|
main() {
|
||||||
|
local list=0 mode=--check root=""
|
||||||
|
while [ $# -gt 0 ]; do
|
||||||
|
case "$1" in
|
||||||
|
--list) list=1 ;;
|
||||||
|
--fix) mode=--fix ;;
|
||||||
|
-h|--help) usage; return 2 ;;
|
||||||
|
-*) printf 'fmt-workspaces: unknown option %s\n' "$1" >&2; usage; return 2 ;;
|
||||||
|
*)
|
||||||
|
if [ -n "$root" ]; then
|
||||||
|
printf 'fmt-workspaces: at most one repo-root, got %s and %s\n' "$root" "$1" >&2
|
||||||
|
return 2
|
||||||
|
fi
|
||||||
|
root="$1" ;;
|
||||||
|
esac
|
||||||
|
shift
|
||||||
|
done
|
||||||
|
|
||||||
|
if [ -z "$root" ]; then root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"; fi
|
||||||
|
if [ ! -d "$root" ]; then
|
||||||
|
printf 'fmt-workspaces: not a directory: %s\n' "$root" >&2
|
||||||
|
return 2
|
||||||
|
fi
|
||||||
|
command -v cargo >/dev/null 2>&1 || {
|
||||||
|
echo 'fmt-workspaces: cargo is not on PATH' >&2; return 2; }
|
||||||
|
# Without this, a missing rustfmt component exits 1 from `cargo fmt` and
|
||||||
|
# reads as "your code is misformatted" -- the very confusion this exists for.
|
||||||
|
cargo fmt --version >/dev/null 2>&1 || {
|
||||||
|
echo 'fmt-workspaces: cargo fmt is unavailable (rustfmt component missing?)' >&2
|
||||||
|
return 2; }
|
||||||
|
|
||||||
|
local -a roots=()
|
||||||
|
while IFS= read -r m; do [ -n "$m" ] && roots+=("$m"); done < <(workspace_roots "$root")
|
||||||
|
if [ ${#roots[@]} -eq 0 ]; then
|
||||||
|
printf 'fmt-workspaces: no [workspace] root under %s\n' "$root" >&2
|
||||||
|
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
|
||||||
|
return 2
|
||||||
|
fi
|
||||||
|
|
||||||
|
if [ "$list" = 1 ]; then
|
||||||
|
printf '%s: %s\n' "$manifest" "${pkgs[*]}"
|
||||||
|
continue
|
||||||
|
fi
|
||||||
|
|
||||||
|
# `-p` for every member, scoped by this root's own manifest. NOT `--all`,
|
||||||
|
# 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.
|
||||||
|
local -a flags=()
|
||||||
|
for member in "${pkgs[@]}"; do flags+=(-p "$member"); done
|
||||||
|
if [ "$mode" = --fix ]; then
|
||||||
|
cargo fmt --manifest-path "$manifest" "${flags[@]}" || rc=1
|
||||||
|
else
|
||||||
|
cargo fmt --manifest-path "$manifest" "${flags[@]}" -- --check || rc=1
|
||||||
|
fi
|
||||||
|
done
|
||||||
|
return $rc
|
||||||
|
}
|
||||||
|
|
||||||
|
main "$@"
|
||||||
Reference in New Issue
Block a user