fix(vmtest-harness): narrow the swallowed-die exemption and wire the host-only gates into CI - #5017
Open
mac-duetto wants to merge 2 commits into
Open
fix(vmtest-harness): narrow the swallowed-die exemption and wire the host-only gates into CI#5017mac-duetto wants to merge 2 commits into
mac-duetto wants to merge 2 commits into
Conversation
…host-only gates into CI Two verified defects in the harness, plus the CI wiring that would have caught both. Neither bug was reachable by review alone; both were latent because the harness's four test scripts and its one mechanical invariant ran nowhere — `grep -rn "vmtest" .github/` returned zero. #5014 — `local x=$(f)` is not exempt `check-no-swallowed-die.sh:248` exempted both `x=$(f)` and `local x=$(f)` from its argument-position finding. The bare form is correct: it propagates the child status to `set -e`. The `local` form is not, and the driver already says so at `vmtest:28-29` — `local` is itself a command, so its own status (always 0) is the one the shell sees and the child's is lost. Verified on the documented target, /bin/bash 3.2.57: `x=$(f)` yields rc 60, `local x=$(f)` yields rc 0 and continues past the fatal. That matters more than "the run continues past a die". The side channel records the classification but does not restore control flow, and `on_exit` (`vmtest:659`) exits with `$rc`, not `VMTEST_EXIT` — so a failed install run exits 0 with a MEASURE line contradicting its own exit status. `vmtest:649-657` records that a runtime exit-code override was tried, reverted, and declined *because this static guard covers it*. The guard is a load-bearing precondition of the exit-code contract, not a lint. Deletes `(local[ \t]+)?` from the regex and corrects the comment, which had claimed both forms propagate. Currently latent: every real call site already follows declare-first/assign-second, so the narrowed guard stays green on the clean tree with zero call sites to repair and zero new false positives. The selftest gains a `local`-form case (10 -> 11), because the guard's green was not evidence of coverage without one. Confirmed load-bearing: restoring the old regex makes that case pass the mutation, exit 0. #5015 — the tool-name invariant was red on main `lib/vm.sh:3` declares that file the only one in the harness that may contain the virtualisation tool's name, so a second backend can be swapped in (DOC-1 §12.2). The check listed two files: `lib/verify.sh:1442` named the tool inside a prose comment, 1,429 lines after that file's own warning at :13-15 that the grep "matches COMMENTS as readily as code". A regression, not an accepted exception — `MANIFEST.md:1913-1919` records the check green at `f181a44e`. Reworded to name the tool by description. CI wiring New `.github/workflows/vmtest-harness.yml`, one lightweight job modelled on `line-cap.yml`, path-filtered to `vmtest-harness/**` and the workflow file. It runs the guard selftest, the guard, `subshell-classification.sh`, `test-preflight-single-run.sh`, and the invariant grep as its own failing step. Selftest before gate, matching line-cap.yml's ordering. Every script is host-only: no VM is created and no network is touched. `subshell-classification.sh` sources the driver through `--source-only`; `test-preflight-single-run.sh` puts a stub for the virtualisation CLI on PATH and redirects the registry and $HOME into a temporary root. Runs on macos-14, not ubuntu-latest. The scripts pin /bin/bash 3.2.57 and `test-preflight-single-run.sh` reads BSD `ps` and renames argv[0] with `exec -a`; a Linux runner would exercise them under bash 5 with GNU `ps` and could report green for semantics the real target fails. The invariant step compares against the exact expected path, so an empty result — a vacuous scan — fails too. Closes #5014 Closes #5015 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two adversarially-verified defects in
vmtest-harness/and wires the harness's existing host-only test scripts into CI. Closes #5014 and #5015 together because the wiring is what makes the second one load-bearing — both bugs were latent for the same reason:grep -rn "vmtest" .github/returned zero.#5014 (MEDIUM) —
local x=$(f)was exempted, and it genuinely swallows the child statustests/check-no-swallowed-die.sh:248exempted bothx=$(f)andlocal x=$(f). The bare form is correct — it propagates the child status toset -e. Thelocalform is not, and the driver already says so atvmtest:28-29:localis itself a command, so its own status (always 0) wins and the child's is lost.This is not a lint.
vmtest:649-657records that a runtime exit-code override was tried, reverted, and deliberately declined because this static guard covers it. With the hole open, a failed install run exits 0 while its MEASURE line reports the real failure —on_exit(vmtest:659) exits with$rc, notVMTEST_EXIT.Changes: delete
(local[ \t]+)?from the regex; correct the comment at :243-245, which had claimed both forms propagate; add alocal-form case to the selftest.#5015 (LOW) — the tool-name invariant was red on main
lib/vm.sh:3declares that file the only one in the harness that may contain the virtualisation tool's name (DOC-1 §12.2, so a second backend can be swapped in).lib/verify.sh:1442named it in a prose comment — 1,429 lines after that same file's warning at :13-15 that the grep "matches COMMENTS as readily as code". Reworded to name the tool by description.CI wiring
New
.github/workflows/vmtest-harness.yml— one lightweight job modelled byte-for-byte online-cap.yml(this repo's convention is one gate per file, not a shared lint workflow;generation-artifact-lint.yml:19-21andagent-assets.yml:13-15state why). Path-filtered tovmtest-harness/**plus the workflow file itself.Steps, selftest-before-gate per
line-cap.yml's ordering: guard selftest → guard →subshell-classification.sh→test-preflight-single-run.sh→ the invariant grep as its own failing step.Two choices worth reviewing:
runs-on: macos-14, notubuntu-latest. The scripts pin/bin/bash3.2.57 (vmtest:17) andtest-preflight-single-run.shreads BSDpsand renames argv[0] withexec -a. A Linux runner would exercise them under bash 5 with GNUpsand could report green for semantics the real target fails — the exact false negative that lets a swallowed-status construct through. Scripts are invoked as/bin/bash <script>rather than the repo-standardbash scripts/…so the interpreter is pinned regardless of what a future runner image puts on PATH, and one step records the version so the claim is auditable in the log.paths:filter, which ci: docs-only PRs run the full Rust build (Clippy/Format/MSRV/Test) — add path filtering #4468 normally forbids. That rule binds gates in branch protection, where a path-skipped required check never reports and leaves the PR pending. This gate is not a required context and the harness is self-contained, so the hazard does not apply. Justified inline in the workflow.All four scripts are host-only — no VM is created, no network is touched.
subshell-classification.shsources the driver through its--source-onlyhook;test-preflight-single-run.shputs a stub for the virtualisation CLI on PATH and redirects the run registry and$HOMEinto a temporary root.Test ladder — rung 2 (test-only stabilization / test harness)
No Rust changes at all, so the ladder's
-p <crate>gate has no crate to name; the harness's own scripts are the equivalent proof, and they are exactly what this PR wires into CI. Rung 1 (comments) also applies to theverify.shreword. Claiming 2 rather than 1 because the guard's scanning behaviour genuinely changed, so a comment-only gate would not have been enough.Commands run on
/bin/bash3.2.57 (arm64-apple-darwin25) — the documented target:The narrowed guard stays green on the clean tree — zero call sites to repair, zero new false positives, exactly as #5014 measured. Every real site already follows declare-first/assign-second.
Both new gates confirmed load-bearing (a gate that cannot fail proves nothing):
local _z=$(tsv_scope_packages)intoscenarios/install-local.sh→OK — no classifying function sits in a status-discarding context, EXIT=0. The pre-fix guard passes the mutation; the new selftest case is what closes that.verify.shreword → the invariant step exits 1 and lists bothlib/verify.shandlib/vm.sh.Repo gates that run regardless of path filter were also run locally and are green:
check_generation_artifacts.sh,check_line_cap.sh,check_doc_numbers.sh.Changelog
No fragment — exempt.
scripts/check_changelog_fragment.shscopes evidence to crates whosecrates/<crate>/src/**changed;vmtest-harness/**and.github/workflows/**are outsidecrates/entirely, so no crate requires evidence and the gate passes green. Substantively the same answer: this is a test-harness fix plus CI wiring with no user-visible change to any published crate.Out of scope
Deliberately not done, per the brief: no widening of the invariant grep to
*.rs(no Rust in the harness yet), no bash→Rust port work, no refactor of the guard beyond the one-token regex change.🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools