fix(gate): decide guard verdicts before printing so a closed pipe cannot forge a failure (#1815) - #1844
fix(gate): decide guard verdicts before printing so a closed pipe cannot forge a failure (#1815)#1844macanderson wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Reviewer's GuideRefactors several gate guard shell scripts to decide and buffer verdict output before emitting it, hardening them against SIGPIPE-induced false failures when their output is piped, while adding a dedicated guard SIGPIPE test and related Makefile target. Sequence diagram for buffered guard verdict emission with SIGPIPE handlingsequenceDiagram
actor ExternalReader
participant GuardScript
participant ReportBuffer as report
ExternalReader->>GuardScript: invoke check-*.sh | head -1
Note over GuardScript: Guard runs checks, does not print yet
GuardScript->>ReportBuffer: note("FAIL ...")
GuardScript->>ReportBuffer: note("more detail ...")
GuardScript->>ReportBuffer: note("" )
alt failures_found
GuardScript->>GuardScript: status=1 or fail=1
else no_failures
GuardScript->>GuardScript: status=0 or fail=0
end
Note over ExternalReader: May exit early, closing pipe
ExternalReader--xGuardScript: [stdout pipe closed]
GuardScript->>GuardScript: emit()
GuardScript->>GuardScript: trap '' PIPE
GuardScript->>ExternalReader: printf '%s' "$report" >&2 || true
alt no_failures
GuardScript->>GuardScript: trap '' PIPE
GuardScript->>ExternalReader: echo "... OK ..." || true
GuardScript-->>ExternalReader: exit 0
else failures_found
GuardScript-->>ExternalReader: exit 1
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…not forge a failure Piping a guard's output is the normal way to read it — `| tail`, `| rg FAIL`, `| head` — and check-god-files.sh reported a FALSE failure naming a different crate on each such run. Two mechanisms, both SIGPIPE: * Its crate-membership test was `printf ... | grep -qx`. `grep -q` exits the moment it matches, printf then dies writing into the closed pipe, and `set -o pipefail` reports the pipeline as failed — so a crate that IS present intermittently read as absent, and the guard blamed whichever crate the race landed on. Replaced with the pure-shell case membership check check-command-docs.sh already uses. * Guards printed their report incrementally while still deciding it. When stdout (or stderr under `2>&1`) is a pipe whose reader has exited, the next write dies of SIGPIPE — 141 outright, or EPIPE plus `set -e` = exit 1 where SIGPIPE is inherited ignored — and whatever partial state the scan had reached becomes the verdict. Every swept guard now decides its verdict fully before printing: failure lines are buffered into a variable and emitted in one final write, and that write runs under `trap '' PIPE` with its failure discarded, so an early-exiting reader can change neither the verdict nor the exit code. check-file-size.sh and check-left-behind.sh already buffered their reports and needed only the hardened final write. Swept: check-god-files, check-invariants, check-left-behind, check-file-size, check-command-docs, check-gate-parity, check-role-names, check-cargo-install-pins, check-repro-wiring. Output text is byte-identical in every case. check-role-names' subshell marker file is gone: the `sed | while` loop is now redirected from process substitution, so `fail=1` and the buffered report survive. Witness: scripts/test-guard-sigpipe.sh (make guard-sigpipe-test) pipes each guard into `head -1` and `true` and asserts exit 0. On the old scripts all nine `| true` cases die with exit 141 (18 of 27 cases pass); on the new ones all 27 pass. Closes #1815
…el step additions Two gate steps landed on main in parallel — module-reachability (#1833) and self-driving-test in GATE_STEPS (#1821) — and each PR bumped the spelled-out count from twenty-three to twenty-four, so their merge left both AGENTS.md and CONTRIBUTING.md claiming twenty-four steps while GATE_STEPS holds twenty-five. check-gate-parity.sh catches exactly this, which is why docs-guards went red on every PR based on current main. The step lists themselves already name every step; only the two counts were stale. Same fix as the parallel unbreak PRs #1845/#1863 — the edits are identical, so whichever lands first the others still merge clean. Refs #1815
0927d12 to
a326831
Compare
|
Rebased on Notes:
|
What & why
scripts/check-god-files.shreported a false failure naming a different crate on every run whenever its output was read through a pipe (| tail,| rg FAIL,| head) — the routine way a gate is triaged. Two SIGPIPE mechanisms, both fixed here:has_god_files()wasprintf '%s\n' "$crates_with_god_files" | grep -qx "$1".grep -qexits the moment it matches,printfthen dies writing into the closed pipe (line 88's "write error: Broken pipe"), andset -o pipefailreports the pipeline as failed — so a crate that IS present intermittently read as absent, and whichever crate the race landed on got blamed. Replaced with the pure-shellcasemembership check thatscripts/check-command-docs.shalready uses for exactly this reason (its comment documents the same race).set -e= exit 1 where SIGPIPE is inherited ignored — and whatever partial state the scan had reached becomes the verdict.The structural fix is the one the issue names: decide the verdict fully before printing anything. Failure lines are buffered into a variable and emitted in one final write, and that write runs under
trap '' PIPEwith its failure discarded — so an early-exiting reader can change neither the verdict nor the exit code.scripts/check-file-size.shis the exemplar shape being copied.Closes #1815
The sweep — every
scripts/check-*.sh, and what each heldRestructured (incremental report loops under
set -e):check-god-files.sh— both mechanisms above; the actual bug.check-invariants.sh— FAIL echoes inside the duplicate-scan and citation loops.check-command-docs.sh— FAIL echoes inside all four check loops (it already had the safecontains(); only the reporting was interleaved).check-gate-parity.sh—noteemitted inside the per-document/per-step loops.check-role-names.sh—noteinside producer loops; also itssed | whilesubshell + marker-file workaround is now a< <(…)redirect (matching producer 5 in the same file), sofail=1and the buffered report survive without the temp file.check-cargo-install-pins.sh— echo inside the token loop.check-repro-wiring.sh—noteinterleaved through every wiring check.Already verdict-before-print; only the final OK write needed hardening (it died 141 under
| true):check-file-size.sh(the exemplar — its verdict was always safe)check-left-behind.shInspected and left alone — verdict decided before printing, no loop-shaped reports:
check-action-pins.sh,check-brand-case.sh,check-design-refs.sh,check-empty-diff.sh,check-license-allowlist-parity.sh,check-no-scratch.sh,check-no-secrets.sh,check-stat-portability.sh, andcheck-wire-schema.sh(needs a compiled workspace; this PR's verification was shell-only). Their final OK line still carries the 141-under-| trueexposure — filed as #1838 rather than fixed untested here.Output text is byte-identical in every modified script — same lines, same streams, same order; only when they are written changed.
The witness
main, passes here)scripts/test-guard-sigpipe.sh(make guard-sigpipe-test, wired likefile-size-test; hermetic, not part ofgate): pipes each swept guard intohead -1and intotrue(the deterministic harshest reader — it closes the pipe before the guard writes anything) and asserts the guard's ownPIPESTATUS[0]is 0.Verified in both directions on this tree:
HEADbefore committing):passed 18, failed 9— every one of the nine| truecases dies, e.g.FAIL check-god-files.sh | true — rc=141 … line 207: printf: write error: Broken pipe, suite exit 1.passed 27, failed 0, suite exit 0../scripts/check-god-files.sh | head -1 ; echo "rc=${PIPESTATUS[0]}"reproducedrc=1blamingstella-model(and, on other runs, the OK line with rc=0 — the race) before the fix; after it, 17 consecutive piped runs print the OK line with rc=0.The gate
check-file-size.sh)check-god-files.shagainst a synthetic broken fixture repo emits the same FAIL report text and exits 1shellcheckclean on all nine modified guards plus the new test scriptmake format-check/clippy/test untouched by it; CI runs them)Closes #1815appears both above and as a commit trailerNothing left behind
| true); handoff includes the epilogue shape to copy and the witness harness to extend.Ground-rule check
stella-core; no new depsAnything reviewers should know?
trap '' PIPEis set only immediately before emission, never at script start, so child pipelines during the scan keep normal SIGPIPE semantics.| head -1god-files case runs ten times because the membership race was intermittent; the deterministic old-code witness is the| truerow for every guard.check-role-names.shloses itsmktempmarker file — that was a workaround for the subshell the pipe created, and the redirect removes the subshell itself.Summary by Sourcery
Guard against false failures in gate shell checks caused by SIGPIPE and pipefail when their output is piped, and add a regression test to ensure guards remain green when their stdout is closed early.
Bug Fixes:
Enhancements:
Tests: