chore: rough edges in script/discard-dist-on-failure — status swallowed when stderr is closed, silent SIGINT, and a censored-word failure wipes a good release build #342

New Issue

clawbot · 2026-08-23T15:35:59+02:00

clawbot commented

2026-08-23 15:35:59 +02:00

Non-blocking findings from the review of #341 (#341 (comment)), filed rather than held against that PR. Each was measured, not inferred. None is reachable on the normal make build path.

Exit status is swallowed when stderr is closed. script/discard-dist-on-failure:52-58 — with stderr closed, the final echo ... >&2 fails, set -eu aborts before exit "$_status", and the wrapper returns 2 instead of the wrapped step's status. Measured: discard-dist-on-failure sh -c 'exit 6' 2>&- returns 2. This contradicts the script's documented contract that it returns the step's own status.
SIGINT leaves a partial dist/ with no message. On a group SIGINT mid-make build, the wrapper dies from the signal, discard_dist never runs, and dist/ is left partially written and unmentioned. The mktemp'd receipt trap still fires correctly. Arguably right — an interrupt is not a build failure — but it is currently undocumented, so decide and then say so in one of the two places.
A censored-word failure destroys an otherwise-valid release artifact. script/check-censored --require-dist runs inside the wrapper, so a censorship-check failure wipes a dist/ that built and verified cleanly. Consistent with the "every step of build:" rule the PR implemented, but it is a non-safety failure destroying a good artifact — unlike the debug-bundle case the wipe exists for. Worth an explicit decision either way.
README.md:165 — the bullet ending ...is reported as loudly as one that was has no terminal period, unlike every sibling bullet. Prettier does not catch it.

Definition of done

(1) fixed: the wrapped step's exit status survives a failed diagnostic write.
(2) and (3) each decided and implemented, with the resulting behaviour stated where the script describes itself.
(4) fixed.
make check green.

Non-blocking findings from the review of https://git.eeqj.de/sneak/AutistMask/pulls/341 (https://git.eeqj.de/sneak/AutistMask/pulls/341#issuecomment-69032), filed rather than held against that PR. Each was measured, not inferred. None is reachable on the normal `make build` path. 1. **Exit status is swallowed when stderr is closed.** `script/discard-dist-on-failure:52-58` — with stderr closed, the final `echo ... >&2` fails, `set -eu` aborts before `exit "$_status"`, and the wrapper returns 2 instead of the wrapped step's status. Measured: `discard-dist-on-failure sh -c 'exit 6' 2>&-` returns 2. This contradicts the script's documented contract that it returns the step's own status. 2. **SIGINT leaves a partial `dist/` with no message.** On a group SIGINT mid-`make build`, the wrapper dies from the signal, `discard_dist` never runs, and `dist/` is left partially written and unmentioned. The mktemp'd receipt trap still fires correctly. Arguably right — an interrupt is not a build failure — but it is currently undocumented, so decide and then say so in one of the two places. 3. **A censored-word failure destroys an otherwise-valid release artifact.** `script/check-censored --require-dist` runs inside the wrapper, so a censorship-check failure wipes a `dist/` that built and verified cleanly. Consistent with the "every step of `build:`" rule the PR implemented, but it is a non-safety failure destroying a good artifact — unlike the debug-bundle case the wipe exists for. Worth an explicit decision either way. 4. `README.md:165` — the bullet ending `...is reported as loudly as one that was` has no terminal period, unlike every sibling bullet. Prettier does not catch it. ## Definition of done - [ ] (1) fixed: the wrapped step's exit status survives a failed diagnostic write. - [ ] (2) and (3) each decided and implemented, with the resulting behaviour stated where the script describes itself. - [ ] (4) fixed. - [ ] `make check` green.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#342