fix: verify-build diagnostic overstates the failure, and two robustness gaps in the same script #180

New Issue

clawbot · 2026-08-09T07:21:04+02:00

clawbot commented

2026-08-09 07:21:04 +02:00

Three non-blocking findings from the independent review of PR #178 (issue
#170), all in script/verify-build or its coupling to build.js. Grouped
because they are one small pass over one file.

Depends on #178 landing first.

1. The diagnostic claims a worse failure than actually occurs

script/verify-build:55-57 reports, when a bundle carries both markers:

> the build-time DEBUG value was never resolved and the debug branch is still
> live

The reviewer inspected the actual emitted output with the define removed:

var cB = typeof __BUILD_DEBUG__ < "u" ? __BUILD_DEBUG__ : !1, …

In extension context __BUILD_DEBUG__ is undeclared, so DEBUG evaluates to
false at runtime. The debug branch is not live and the artifact is not
drainable. The fallback goes the safe way.

The same overstatement appears in PR #178's commit body and originates in issue
#170's own text, which I wrote — I reasoned from the source and assumed the
fallback went the unsafe way, without checking the compiled output. That was my
error, not the implementer's; they inherited it.

What actually goes wrong is still worth failing the build over: DEBUG is no
longer resolved at build time. The release/debug distinction has silently
stopped being enforced, and which way the unresolved fallback happens to
evaluate is then an accident that a future refactor can flip. That is a broken
build contract. It is just not a live funds-loss bug, and the message should
not say it is.

Requirements

Reword the message at script/verify-build:55-57 to state the real failure:
the build-time DEBUG value was never resolved, so the release/debug
distinction is no longer enforced. Keep it a hard failure.
Do not overcorrect into implying it is harmless. The build must still fail.
Check the other seven failure messages in the script for the same kind of
overstatement while you are there, and say in the PR whether any needed
changing.

2. `grep` exit 2 is conflated with exit 1

script/verify-build:38 uses 2>/dev/null, which merges "grep could not read
the file" (exit 2) with "no match" (exit 1).

An unreadable bundle therefore fails safe — it is reported as carrying no
marker, which is a hard failure — but it is misdiagnosed, telling the
operator the emitted output changed shape when the real problem is a
permissions or I/O fault. The reviewer confirmed the fail-safe behaviour by
chmod 000 on a bundle.

Requirements

Distinguish grep exit 2 from exit 1 in the marker check and emit a distinct,
accurate message for the unreadable case.
Keep both paths hard failures. The point is diagnosis, not leniency.

3. `*.js` coupling between two files

build.js:35 and script/verify-build:76 both filter to *.js. A future
bundle emitted with a different extension would escape both the manifest
and the unlisted-bundle cross-check — silently, in both places at once, which
is exactly the correlated-blind-spot failure the two-source design exists to
avoid.

Requirements

Remove the duplicated assumption, or make the coupling explicit and
self-checking so the two cannot drift apart. Deriving the extension set from
one place is better than repeating the literal.
If the cleanest fix is genuinely to keep both filters, add a comment at each
site naming the other, and explain in the PR why a shared source was not
practical.

Explicitly not in scope

Recorded as considered and rejected, so they are not re-raised:

The shared unscoped _file variable at script/verify-build:48-52 and :77
— latent, safe today.
read -r not representing paths with leading or trailing whitespace.
AUTISTMASK_DEBUG=1 make build blessing a debug artifact as verified — this
is explicitly required by #170, and Docker CI does not inherit host env.

Definition of done

The both-markers diagnostic states the real failure and the build still
fails.
The other failure messages have been reviewed for overstatement.
An unreadable bundle produces a distinct, accurate message and still
fails.
The *.js assumption exists in one place, or the coupling is explicit
and documented at both sites.
All the failure modes demonstrated in PR #178 still trigger — re-run them
and say so in the PR.
make build, make build-debug, and make check all green.
TODO.md updated in the same commit.

Three non-blocking findings from the independent review of PR #178 (issue #170), all in `script/verify-build` or its coupling to `build.js`. Grouped because they are one small pass over one file. Depends on #178 landing first. ## 1. The diagnostic claims a worse failure than actually occurs `script/verify-build:55-57` reports, when a bundle carries both markers: > the build-time DEBUG value was never resolved and the debug branch is still > live The reviewer inspected the actual emitted output with the define removed: ```js var cB = typeof __BUILD_DEBUG__ < "u" ? __BUILD_DEBUG__ : !1, … ``` In extension context `__BUILD_DEBUG__` is undeclared, so `DEBUG` evaluates to **false** at runtime. The debug branch is **not** live and the artifact is not drainable. The fallback goes the safe way. The same overstatement appears in PR #178's commit body and originates in issue #170's own text, which I wrote — I reasoned from the source and assumed the fallback went the unsafe way, without checking the compiled output. That was my error, not the implementer's; they inherited it. What actually goes wrong is still worth failing the build over: **`DEBUG` is no longer resolved at build time.** The release/debug distinction has silently stopped being enforced, and which way the unresolved fallback happens to evaluate is then an accident that a future refactor can flip. That is a broken build contract. It is just not a live funds-loss bug, and the message should not say it is. **Requirements** - Reword the message at `script/verify-build:55-57` to state the real failure: the build-time DEBUG value was never resolved, so the release/debug distinction is no longer enforced. Keep it a hard failure. - Do not overcorrect into implying it is harmless. The build must still fail. - Check the other seven failure messages in the script for the same kind of overstatement while you are there, and say in the PR whether any needed changing. ## 2. `grep` exit 2 is conflated with exit 1 `script/verify-build:38` uses `2>/dev/null`, which merges "grep could not read the file" (exit 2) with "no match" (exit 1). An unreadable bundle therefore fails **safe** — it is reported as carrying no marker, which is a hard failure — but it is **misdiagnosed**, telling the operator the emitted output changed shape when the real problem is a permissions or I/O fault. The reviewer confirmed the fail-safe behaviour by `chmod 000` on a bundle. **Requirements** - Distinguish grep exit 2 from exit 1 in the marker check and emit a distinct, accurate message for the unreadable case. - Keep both paths hard failures. The point is diagnosis, not leniency. ## 3. `*.js` coupling between two files `build.js:35` and `script/verify-build:76` both filter to `*.js`. A future bundle emitted with a different extension would escape **both** the manifest and the unlisted-bundle cross-check — silently, in both places at once, which is exactly the correlated-blind-spot failure the two-source design exists to avoid. **Requirements** - Remove the duplicated assumption, or make the coupling explicit and self-checking so the two cannot drift apart. Deriving the extension set from one place is better than repeating the literal. - If the cleanest fix is genuinely to keep both filters, add a comment at each site naming the other, and explain in the PR why a shared source was not practical. ## Explicitly not in scope Recorded as considered and rejected, so they are not re-raised: - The shared unscoped `_file` variable at `script/verify-build:48-52` and `:77` — latent, safe today. - `read -r` not representing paths with leading or trailing whitespace. - `AUTISTMASK_DEBUG=1 make build` blessing a debug artifact as verified — this is explicitly required by #170, and Docker CI does not inherit host env. ## Definition of done - [ ] The both-markers diagnostic states the real failure and the build still fails. - [ ] The other failure messages have been reviewed for overstatement. - [ ] An unreadable bundle produces a distinct, accurate message and still fails. - [ ] The `*.js` assumption exists in one place, or the coupling is explicit and documented at both sites. - [ ] All the failure modes demonstrated in PR #178 still trigger — re-run them and say so in the PR. - [ ] `make build`, `make build-debug`, and `make check` all green. - [ ] `TODO.md` updated in the same commit.

clawbot added this to the 1.0.0 milestone 2026-08-09 07:21:04 +02:00

clawbot referenced this issue

2026-08-10 16:13:32 +02:00

build: assert DEBUG is off in every emitted bundle as a post-build check (closes #170) #178

~~clawbot referenced this issue 2026-08-10 16:16:05 +02:00~~

milestone 1.0.0: approval-path security, build-integrity guard, e2e harness and filter coverage #190

clawbot referenced this issue from a commit

2026-08-11 14:23:15 +02:00

fix: correct verify-build diagnostics and close two robustness gaps (closes #180)

clawbot referenced a pull request that will close this issue

2026-08-11 14:23:43 +02:00

fix: correct verify-build diagnostics and close its vacuous passes (closes #180) #203