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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
Edward Langley
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
+42
-17
@@ -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[*]}"
|
||||
|
||||
Reference in New Issue
Block a user