sneak/AutistMask
1
0
Fork 0
Code Issues 42 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

test: verify-build's failure modes are only ever proven by hand — make the battery a committed target #227

New Issue
Closed
opened 2026-08-11 15:08:53 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-11 15:08:53 +02:00
Collaborator
Copy Link

script/verify-build is the build-integrity guard, and nothing in make check tests it. Every one of its ~17 failure modes is re-proven by hand, once per change, by whoever happens to be touching it.

That has already cost real defects. Three separate reviews of this script each found a fresh vacuous pass — the grep exit-2 conflation, the discarded find status, and the line-delimited walk — and each was caught by a human-equivalent constructing a tree by hand, not by a test. The most recent fix (#223) also turned up that NUL delimiting alone was insufficient, which only surfaced because someone built the case and ran it.

Two workers have independently raised this. A working battery already exists in throwaway form at /srv/work/issue-223-battery/battery.sh; it just is not committed and does not run.

The obstacle, which is the reason this needs thought rather than just doing it

The Dockerfile declares no USER, so CI runs as root, and under root chmod 000 does not stop find. A permission-based case would be a guaranteed false green in CI while passing locally — worse than no test. That has to be solved, not worked around:

  • give the CI container a non-root USER, or
  • have the harness skip permission cases when id -u is 0 and say so loudly in its output, so a green run never silently means "the permission cases did not run".

The second is cheaper; the first is honest. Either is acceptable if the "did not run" case is impossible to mistake for a pass.

Implementation requirements

  • Add script/test-verify-build, invoked from script/check so make check covers it.
  • Build the fixture tree in a temp dir, never in dist/ — the current hand battery mutates the real build output.
  • Assert exit status AND the message for each case; a guard that fails for the wrong reason is a defect this harness should catch.
  • Cover every case the hand battery covers: trailing space, embedded newline, dist/ as a symlink, unwalkable subtree, dangling and directory-pointing symlinks, symlink to a listed bundle under an unlisted path, missing/empty/unreadable manifest, missing/empty/unreadable listed bundle, unlisted extension, no marker, both markers, wrong marker, plus the untouched-dist/ control.
  • POSIX sh, matching the house style of the other script/ entrypoints.

Definition of done

  • make check fails if any verify-build failure mode stops failing.
  • The root/permission problem is resolved such that a green run cannot mean "those cases were skipped" without saying so unmistakably.
  • The harness builds nothing in dist/ and leaves no residue.
  • Deliberately breaking one guard in verify-build makes the harness fail — demonstrated, with captured output.
  • TODO.md updated in the same commit.
  • make check passes.
`script/verify-build` is the build-integrity guard, and nothing in `make check` tests it. Every one of its ~17 failure modes is re-proven by hand, once per change, by whoever happens to be touching it. That has already cost real defects. Three separate reviews of this script each found a fresh vacuous pass — the `grep` exit-2 conflation, the discarded `find` status, and the line-delimited walk — and each was caught by a human-equivalent constructing a tree by hand, not by a test. The most recent fix (https://git.eeqj.de/sneak/AutistMask/issues/223) also turned up that NUL delimiting alone was insufficient, which only surfaced because someone built the case and ran it. Two workers have independently raised this. A working battery already exists in throwaway form at `/srv/work/issue-223-battery/battery.sh`; it just is not committed and does not run. ## The obstacle, which is the reason this needs thought rather than just doing it The Dockerfile declares no `USER`, so CI runs as root, and under root `chmod 000` does not stop `find`. A permission-based case would be a **guaranteed false green** in CI while passing locally — worse than no test. That has to be solved, not worked around: - give the CI container a non-root `USER`, or - have the harness skip permission cases when `id -u` is 0 and say so loudly in its output, so a green run never silently means "the permission cases did not run". The second is cheaper; the first is honest. Either is acceptable if the "did not run" case is impossible to mistake for a pass. ## Implementation requirements - Add `script/test-verify-build`, invoked from `script/check` so `make check` covers it. - Build the fixture tree in a temp dir, never in `dist/` — the current hand battery mutates the real build output. - Assert exit status AND the message for each case; a guard that fails for the wrong reason is a defect this harness should catch. - Cover every case the hand battery covers: trailing space, embedded newline, `dist/` as a symlink, unwalkable subtree, dangling and directory-pointing symlinks, symlink to a listed bundle under an unlisted path, missing/empty/unreadable manifest, missing/empty/unreadable listed bundle, unlisted extension, no marker, both markers, wrong marker, plus the untouched-`dist/` control. - POSIX `sh`, matching the house style of the other `script/` entrypoints. ## Definition of done - [ ] `make check` fails if any `verify-build` failure mode stops failing. - [ ] The root/permission problem is resolved such that a green run cannot mean "those cases were skipped" without saying so unmistakably. - [ ] The harness builds nothing in `dist/` and leaves no residue. - [ ] Deliberately breaking one guard in `verify-build` makes the harness fail — demonstrated, with captured output. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.
clawbot added this to the 1.0.0 milestone 2026-08-11 15:08:53 +02:00
clawbot commented 2026-08-12 10:19:52 +02:00
Author
Collaborator
Copy Link

Plan, mainly to record the root/permission resolution before writing it.

Fixture: script/test-verify-build builds a synthetic tree in mktemp -d containing script/verify-build as a symlink to the real script plus a fake dist/. verify-build derives its ROOT from dirname $0/.., so it operates on the fixture and never touches the repo's dist/. The symlink (rather than a copy) means the harness exercises the live script, which is what makes a deliberate break in it fail the harness. TMPDIR is pointed inside the work dir so verify-build's own mktemp leaves nothing behind either.

Root/permission: neither of the two options as stated, but a strict superset of the second, because a case that is permanently skipped in CI gates nothing — and CI is the only place this runs unattended. The harness picks a runner for the three permission-dependent cases and then proves the runner before trusting it, with two probes: a mode-644 file must be readable through it (else the runner is broken) and a mode-000 file must NOT be (else permissions are ineffective). Non-root: the runner is "run directly" and both probes pass trivially. Root: it tries setpriv --reuid=65534 then runuser -u nobody, both present in the pinned node:22-slim CI image, so the cases really run in CI. Only if no candidate passes both probes are those cases skipped, and then the run prints a banner naming each skipped case and a final line that says SKIPPED rather than passed. A false green is impossible by construction: either the mechanism is demonstrated to work on a mode-000 file in this very run, or the output says the cases did not run.

Not adding a non-root USER to the Dockerfile: RUN script/bootstrap and COPY . . leave /app and node_modules root-owned, and RUN make build writes dist/, so a non-root USER needs a chown -R /app layer that duplicates the whole node_modules tree in the image. That is a CI-container change with a real size cost, outside this issue.

Cases: exactly the battery's list, each asserting exit status AND a message substring, so a guard failing for the wrong reason is a harness failure. Wired into script/check after script/test. One TODO.md bullet in the same commit. Will report the deliberate-break output and the added make check time on the PR.

Plan, mainly to record the root/permission resolution before writing it. Fixture: `script/test-verify-build` builds a synthetic tree in `mktemp -d` containing `script/verify-build` as a **symlink to the real script** plus a fake `dist/`. `verify-build` derives its `ROOT` from `dirname $0/..`, so it operates on the fixture and never touches the repo's `dist/`. The symlink (rather than a copy) means the harness exercises the live script, which is what makes a deliberate break in it fail the harness. `TMPDIR` is pointed inside the work dir so `verify-build`'s own `mktemp` leaves nothing behind either. Root/permission: neither of the two options as stated, but a strict superset of the second, because a case that is permanently skipped in CI gates nothing — and CI is the only place this runs unattended. The harness picks a runner for the three permission-dependent cases and then **proves the runner before trusting it**, with two probes: a mode-644 file must be readable through it (else the runner is broken) and a mode-000 file must NOT be (else permissions are ineffective). Non-root: the runner is "run directly" and both probes pass trivially. Root: it tries `setpriv --reuid=65534` then `runuser -u nobody`, both present in the pinned `node:22-slim` CI image, so the cases really run in CI. Only if no candidate passes both probes are those cases skipped, and then the run prints a banner naming each skipped case and a final line that says SKIPPED rather than passed. A false green is impossible by construction: either the mechanism is demonstrated to work on a mode-000 file in this very run, or the output says the cases did not run. Not adding a non-root `USER` to the Dockerfile: `RUN script/bootstrap` and `COPY . .` leave `/app` and `node_modules` root-owned, and `RUN make build` writes `dist/`, so a non-root `USER` needs a `chown -R /app` layer that duplicates the whole `node_modules` tree in the image. That is a CI-container change with a real size cost, outside this issue. Cases: exactly the battery's list, each asserting exit status AND a message substring, so a guard failing for the wrong reason is a harness failure. Wired into `script/check` after `script/test`. One `TODO.md` bullet in the same commit. Will report the deliberate-break output and the added `make check` time on the PR.
clawbot commented 2026-08-12 10:28:01 +02:00
Author
Collaborator
Copy Link

Built as planned, no deviations: #249, which carries the full record including the deliberate-break output.

Verified: make check green on the branch (18 battery cases, 0.7s of a 15.2s check); script/cibuild exit 0 with RUN make check executed rather than CACHED, and its log showing the permission cases enabled via setpriv and all 18 passing as uid 0 in the container; the same image with setpriv and runuser moved aside produces the skip banner and a final line reading SKIPPED AND NOT PROVEN; breaking has_marker's exit-2 branch fails 3 cases and exits 1, one of them caught only by the message assertion because that break keeps the exit status at 1 while blaming the bundle for a permissions fault; no dist/ created and nothing left in /tmp.

Built as planned, no deviations: [#249](https://git.eeqj.de/sneak/AutistMask/pulls/249), which carries the full record including the deliberate-break output. Verified: `make check` green on the branch (18 battery cases, 0.7s of a 15.2s check); `script/cibuild` exit 0 with `RUN make check` executed rather than `CACHED`, and its log showing the permission cases enabled via `setpriv` and all 18 passing as uid 0 in the container; the same image with `setpriv` and `runuser` moved aside produces the skip banner and a final line reading `SKIPPED AND NOT PROVEN`; breaking `has_marker`'s exit-2 branch fails 3 cases and exits 1, one of them caught only by the message assertion because that break keeps the exit status at 1 while blaming the bundle for a permissions fault; no `dist/` created and nothing left in `/tmp`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#227
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?