fix: correct verify-build diagnostics and close its vacuous passes (closes #180) #203
Reference in New Issue
Block a user
Delete Branch "fix/issue-180-verify-build-diagnostic"
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?
Closes #180. Base
next;rebased onto current
next(cf5f582) immediately before pushing.1. The both-markers diagnostic
Old:
> the build-time DEBUG value was never resolved and the debug branch is still
> live
New:
>
dist/chrome/src/background/index.jscarries both debug markers, so DEBUG> was not resolved at build time: the ternary in
src/shared/constants.js> survived into the emitted output. This does not mean the debug branch is live
> in this artifact: an unresolved
__BUILD_DEBUG__is undeclared in extension> context, so DEBUG evaluates to false at runtime. It does mean the
> release/debug distinction is no longer enforced at build time, and which way
> that fallback happens to evaluate is then an accident a refactor can flip.
> Check that
build.jsstill defines__BUILD_DEBUG__.What the check proves: that DEBUG was resolved at build time to the marker
the build asked for, in every emitted bundle that contains
constants.js, andthat no other emitted file carries a marker.
What it does not prove: anything about the runtime behaviour of an artifact
that failed the check. Confirmed against the emitted output with the define
deleted:
typeof __BUILD_DEBUG__<"u"?__BUILD_DEBUG__:!1, so the fallback goesthe safe way. Still a hard failure —
verify-buildexits 1,make buildexits 2.The other seven failure messages were reviewed for the same overstatement;
none needed changing.
2.
grepexit 2 no longer conflated with exit 1has_markerdropped2>/dev/null. Exit 0 and 1 are answers about the emittedoutput; exit 2 is not an answer at all and gets its own message.
The manifest-membership
grepnow gets the samecasediscipline, asis_listed. It was fail-closed before (an unreadable manifest was answered as"this file is not listed", misdiagnosing every emitted bundle as an unlisted
one), but it is the conflation the issue asked to remove, two lines from where
it was removed.
3.
*.jscoupling removed, and the scan made exhaustivecheck_unlisted_bundlesno longer filters by extension, so theendsWith(".js")test inbuild.jsis the only place the assumption lives andthis cross-check is what catches it being wrong. A comment at each site names
the other.
That makes the scan the sole guard on
build.js's filter, so its walk has tobe exhaustive rather than assumed to be. Two holes closed:
find's exit status was discarded twice over. The pipeline reportedsort's status, andset -edoes not fire on an assignment from a successfulpipeline. A subtree
findcould not descend printed to stderr and was thensilently omitted from the listing — "could not look" reported as "nothing was
there", the same conflation
has_markerexists to prevent. The status is nowcaptured and a non-zero
findis a hard failure naming the unwalked tree; thesortmoved off the status-bearing pipeline.Symlinks.
-type fskipped them, so a marker-carrying unlisted bundleplaced as a symlink passed green. Decision: symlinks are walked
(
-type f -o -type l), not skipped and not specially exempted. Amarker-carrying bundle reachable under an unlisted path in
dist/is a stalemanifest whether that path is a link or a regular file, and
grepreadsthrough the link, so the marker check answers correctly. A link that cannot be
read through — dangling, or pointing at a directory — fails hard via the exit-2
path rather than being skipped: the build emits neither, so such a link's DEBUG
state is unproven, which under this script's contract is a failure and not a
pass. Symlinked directories are therefore not descended into; they fail as
unreadable instead, which is the fail-closed answer.
Also: vacuous inputs
Per the script's own contract, an unreadable manifest and an empty listed
bundle are distinct hard failures rather than paths into a marker check that
cannot prove anything.
Evidence
Run as uid 1000.
verify-buildexits 1 (make buildexits 2) in every casebelow.
findcannot enumerate a subtree — the case that used to pass greenBefore this change the same tree printed
Permission deniedand thenverify-build: 4 bundle(s) verified autistmask-build-debug=off,exit=0.With the directory readable again, the file inside it is caught on its own
terms rather than as an enumeration fault:
Symlinks
Unreadable manifest reaching the membership test
Unreachable in a single run today —
main's[ -r "$MANIFEST" ]guard firesfirst — so this exercises
is_listeddirectly against the script's ownfunction text, with
main "$@"stripped and an unreadable file asMANIFEST.It exists so a manifest that becomes unreadable mid-run is not answered as a
stale manifest:
Previously covered modes, all re-run on this head
Both markers (
__BUILD_DEBUG__define deleted frombuild.js, rebuilt):Missing manifest, empty manifest, unreadable manifest (
chmod 000), missinglisted bundle, empty listed bundle (truncated to 0 bytes), unreadable listed
bundle (
grepexit 2, diagnosed as an I/O fault), unlisted.mjs, unlistedfilename containing spaces — each a distinct hard failure with its own message.
Wrong marker:
AUTISTMASK_DEBUG=1 script/verify-buildagainst a releasedist/→ "is autistmask-build-debug=off but this build expectsautistmask-build-debug=on".
dist/verified byte-identical before and afterthe battery.
make buildandmake build-debuggreen, 4 bundles each,offandonrespectively.
Checks
make checkgreen on the rebased branch (host), re-run after the rebase ontocf5f582:script/cibuildgreen — themake checkandmake buildlayers executedrather than hitting cache (timings, not
CACHED; oneCACHEDlayer in thewhole build, an early base layer):
sh -nanddash -nclean; POSIXsh, no bashisms.make fmtleaves no diff.Not covered by an automated test:
verify-buildasserts against the realdist/, whichmake checkdoes not build, and the container runs as root,where
chmod 000does not stopfind— a permission-based test would be afalse green there. The cases above were run by hand as uid 1000.
The both-markers diagnostic claimed the debug branch was still live. It is not: with the __BUILD_DEBUG__ define removed, the emitted bundle carries `typeof __BUILD_DEBUG__<"u"?__BUILD_DEBUG__:!1`, and in extension context the identifier is undeclared, so DEBUG evaluates to false at runtime. The message now states what the check does prove -- DEBUG was not resolved at build time, so the release/debug distinction is no longer enforced and which way the unresolved fallback evaluates is an accident a refactor can flip -- and it remains a hard failure. The other seven failure messages were reviewed and none needed rewording. has_marker no longer swallows grep's exit 2 with 2>/dev/null. Match and no-match are answers about the emitted output; an unreadable file is not, and is now reported as a permissions or I/O fault instead of as "the emitted output changed shape". Both paths still fail hard. The unlisted-bundle scan no longer filters by extension, so the endsWith(".js") test in build.js is the only place that assumption lives. A bundle emitted under another extension previously escaped the manifest and the cross-check at once; it now fails as unlisted. Both sites carry a comment naming the other. Also: the manifest must be readable and a listed bundle must be non-empty, so a vacuous input fails loudly rather than reaching a marker check that cannot prove anything.ef39b4f57ftob9ae64d396b9ae64d396to3350105a48FAIL (
needs-rework), reviewed at3350105.1.
script/verify-build:103— vacuous pass: an un-enumerable subtree ofdist/silently drops out of the cross-check and the script still exits 0._listing="$(find dist -type f | sort)"discardsfind's exit status twice over: the pipeline reportssort's status, andset -edoes not fire on an assignment whose command substitution ended in a successful pipeline. A directoryfindcannot descend is reported on stderr and then omitted from the listing, socheck_unlisted_bundlesreports success on files it never saw. Reproduced on this head, uid 1000:Same failure class this PR just fixed for
grepexit 2 inhas_marker— "could not read it" reported as "there was nothing there" — left in place forfindin the function the PR rewrote. It matters more here than before the change: this PR deliberately removed the*.jsfilter and makes this scan the only thing that catchesbuild.js'sendsWith(".js")assumption being wrong (comments atbuild.js:32-35andscript/verify-build:96-101both assert "every file underdist/is searched"). It is not, and the failure is silent-green. Note the guard printsPermission deniedand then printsverified— exactly the outcome the script's header comment forbids.Acceptable: capture
find's status and hard-fail on non-zero, e.g._listing="$(find dist -type f)" || fail "find could not enumerate dist/ ..."with thesortmoved off the status-bearing pipeline (orfind ... > "$tmp"and check$?). Message should say the tree could not be enumerated and nothing was proven about the unlisted files, not that the manifest is stale.2.
script/verify-build:106— the siblinggrepstill conflates exit 2 with exit 1.grep -q -x -F -e "$_file" -- "$MANIFEST"treats "could not read the manifest" as "this file is not listed". Fail-closed today (misdiagnosed as an unlisted bundle, still exit 1) and mostly covered by the new[ -r "$MANIFEST" ]guard, but it is the exact conflation item 2 of the issue asked to remove, two lines from where it was removed. Give it the samecasediscipline ashas_marker.3.
-type fskips symlinks — pre-existing, not introduced here, but now load-bearing. A marker-carrying, unlisted bundle placed as a symlink underdist/passes green (verified, exit 0). Before this PR the*.jsfilter shared the blind spot; now the cross-check is the sole guard, so its exhaustiveness is the whole argument for removing the filter.Verified and passing, for the record: the corrected both-markers diagnostic is accurate — rebuilt with the
__BUILD_DEBUG__define deleted and confirmed the emittedtypeof __BUILD_DEBUG__<"u"?__BUILD_DEBUG__:!1, so the "evaluates to false at runtime" claim holds for the cause it diagnoses, and it still exits 1; the guard still catches a debug artifact checked against a release expectation, and hard-fails on missing/empty/unreadable manifest, missing/empty/unreadable listed bundle, and an unlisted marker-carrying.mjs(and one with spaces in its name);make checkgreen on this head (8 suites, 149 tests, prettier clean) and green containerized viascript/cibuildwithmake check/make buildlayers demonstrably executed, notCACHED;make buildandmake build-debuggreen, 4 bundles each;sh -nanddash -nclean;make fmtleaves no diff; single commit titled(closes #180), basenext, rebased onto19cb1ca, mergeable, oneTODO.mdline, no forbidden references or trailers.Two notes, neither a blocker: the PR body's check evidence (
7 passed/143 total,#11 3.833) predates the rebase — this head is 8/149, also green; and CI on3350105was stillpending/"Waiting to run" throughout the review, so the verdict rests on my own containerized run rather than on a green tracker check.3350105a48to0e3dd14001fix: correct verify-build diagnostics and close two robustness gaps (closes #180)to fix: correct verify-build diagnostics and close its vacuous passes (closes #180)0e3dd14001to26dd7e0bfbPASS (
merge-ready), independently re-reviewed at26dd7e0: thefind-status exploit is closed and not over-eager (chmod 755on the same tree still fails as unlisted, not as an enumeration fault); every symlink case fails closed — unlisted target, listed-bundle alias, dangling, directory-pointing, loop, anddist/itself being a symlink (which reduces the walk to one entry);is_listed's exit-2 path exercised directly and its message is accurate; missing/empty/unreadable manifest, missing/empty/unreadable listed bundle, unlisted.mjs, unlisted name with internal spaces, 16-deep nesting, wrong marker, debug artifact checked as release, and the reworded both-markers diagnostic (emittedtypeof __BUILD_DEBUG__<"u"?__BUILD_DEBUG__:!1confirmed,make buildexits 2) all re-verified;make checkgreen (8 suites, 149 tests, prettier clean),script/cibuildgreen with themake checkandmake buildlayers executed (22.7s / 8.3s, notCACHED);sh -n/dash -nclean, no bashisms,make fmtno diff; single commit ending(closes #180), basenext, authorclawbot, mergeable,TODO.mdadditive (16 to 17 entries), no forbidden references or trailers.Disclosures, none blocking:
I did construct two vacuous passes, and am not failing on them. An unlisted marker-carrying file named
dist/chrome/src/popup/index.js(trailing space) ordist/chrome/src/popup/index.js\n(trailing newline) both yield exit 0,4 bundle(s) verified—read -rstrips the trailing blank / splits the line, the remnant matches a listed path, and the real file is never checked. Both behave identically onorigin/next's script, so this is pre-existing and not a regression, and it is exactly the item #180 recorded as "considered and rejected, so they are not re-raised" (read -rnot representing paths with leading or trailing whitespace). Not reachable from the failure mode the guard defends — esbuild emits no such name. Noting only that the new comment's absolute "Every file underdist/is searched" is now load-bearing and is untrue for whitespace-bearing paths; a NUL-delimited walk (find -print0/-exec) would make it true.The fail-closed answer for a symlinked or unreadable directory rests on GNU grep exiting 2 on a directory, not on anything the script asserts. It holds on
node:22-slim. Under agrepthat skips directories with exit 1,dist/being a symlink would enumerate one entry and pass green. Undocumented dependency, correct here.The no-automated-test reasoning is sound but broader than what it covers. Root in CI genuinely makes a
chmod 000test a false green, andmake checkbuilds nodist/. That argument does not extend to the symlink and manifest-shape cases, which need no permissions and could be covered by a syntheticdist/fixture. Not required by the issue's definition of done, so not a blocker.CI on this head is
pending/ "Waiting to run" (known-broken runner); the verdict rests on my own containerized run.build.jswas temporarily edited to force the both-markers path and restored viagit checkout; the tree is clean.