diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2428f603..19349628 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -140,7 +140,10 @@ jobs: with: components: clippy, rustfmt - 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 test --workspace # 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 # 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). + # 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 - 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. diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 2356506e..c30c0fc3 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -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 that grows a dependency on some other inherited variable will fail loudly on 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 ` 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 --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 ` 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. diff --git a/scripts/fmt-workspaces-test.sh b/scripts/fmt-workspaces-test.sh new file mode 100755 index 00000000..a67fcb92 --- /dev/null +++ b/scripts/fmt-workspaces-test.sh @@ -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 ` + # 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 ] diff --git a/scripts/fmt-workspaces.sh b/scripts/fmt-workspaces.sh new file mode 100755 index 00000000..8fcbd7a6 --- /dev/null +++ b/scripts/fmt-workspaces.sh @@ -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 "$@"