Repository navigation
fix(build): fail the build when an @include marker is missing - #304
Merged
Merged
Conversation
netravnen
force-pushed
the
fix/build-include-marker-guard
branch
from
August 19, 2026 22:27
7c003e9 to
0e486f4
Compare
netravnen
force-pushed
the
fix/build-include-marker-guard
branch
from
August 19, 2026 22:28
0e486f4 to
aac7d02
Compare
netravnen
force-pushed
the
fix/build-include-marker-guard
branch
from
August 19, 2026 22:29
aac7d02 to
6055efd
Compare
netravnen
force-pushed
the
fix/build-include-marker-guard
branch
from
August 19, 2026 22:31
6055efd to
b1698a6
Compare
netravnen
force-pushed
the
fix/build-include-marker-guard
branch
from
August 19, 2026 23:05
b1698a6 to
0d7de55
Compare
netravnen
force-pushed
the
fix/build-include-marker-guard
branch
from
August 19, 2026 23:06
0d7de55 to
533e506
Compare
netravnen
force-pushed
the
fix/build-include-marker-guard
branch
from
August 19, 2026 23:07
533e506 to
d55e86f
Compare
netravnen
force-pushed
the
fix/build-include-marker-guard
branch
from
August 19, 2026 23:09
d55e86f to
f344c0d
Compare
INCLUDE_RE.sub never asserted that it matched anything, so a .src.js that lost its `/* @include admincom-common.js */` marker built clean. The generated file passed node --check -- the syntax is valid, the references are not -- the script reported "up to date", and the userscript threw ReferenceError in the browser on first use, because dbg, fetchWithRetry and the rest of the shared fragment were silently never inlined. Merging to master is the deploy for this repo, so that reaches every admin before anyone notices. Every script here depends on the shared fragment, so zero markers means the marker was lost, not that a script genuinely needs no lib. The guard says so and names where to record the exception if one ever does. Changes: - render() uses INCLUDE_RE.subn and raises ValueError on zero substitutions, naming the source file and what would have gone wrong. The error propagates out of main() before anything is written, so a failed build leaves no partially generated file behind. - New tests/build-include-marker.test.js covers the failure case and, just as importantly, the success case actually inlining the lib -- a guard test that only checks the error path cannot tell a working build from a broken one. - One case asserts the two build guards stay distinguishable: both run over the same sources, and a missing marker must not be diagnosed as a version problem. Security: - N/A -- build-time only, no product-facing behavior change. Testing: - node --test: 544 tests, 543 pass, 1 skipped (live tests are opt-in). - Fixtures and the spawn harness come from tests/helpers/build-script- runner.js, shared with the @Version parity tests. Its defaults produce a well-formed single-script repo, so a fixture built to exercise one guard does not trip the other. - Guard reverted to the old sub() call and confirmed red. - build_userscripts.py --check still reports all three up to date, so the guard does not fire on the real sources. Backwards Compatibility: - A source with a marker builds byte-identically; all three generated files are unchanged. Only the previously silent zero-marker case changes, from success to a named failure. Assisted-by: Claude:claude-opus-5
netravnen
force-pushed
the
fix/build-include-marker-guard
branch
from
August 20, 2026 09:25
f344c0d to
f0ba5b1
Compare
netravnen
marked this pull request as ready for review
August 20, 2026 09:27
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
INCLUDE_RE.sub never asserted that it matched anything, so a .src.js that
lost its
/* @include admincom-common.js */marker built clean. Thegenerated file passed node --check -- the syntax is valid, the references
are not -- the script reported "up to date", and the userscript threw
ReferenceError in the browser on first use, because dbg, fetchWithRetry
and the rest of the shared fragment were silently never inlined. Merging
to master is the deploy for this repo, so that reaches every admin before
anyone notices.
Every script here depends on the shared fragment, so zero markers means
the marker was lost, not that a script genuinely needs no lib. The guard
says so and names where to record the exception if one ever does.
Changes:
substitutions, naming the source file and what would have gone wrong.
The error propagates out of main() before anything is written, so a
failed build leaves no partially generated file behind.
just as importantly, the success case actually inlining the lib -- a
guard test that only checks the error path cannot tell a working build
from a broken one.
over the same sources, and a missing marker must not be diagnosed as a
version problem.
Security:
Testing:
runner.js, shared with the @Version parity tests. Its defaults produce
a well-formed single-script repo, so a fixture built to exercise one
guard does not trip the other.
guard does not fire on the real sources.
Backwards Compatibility:
files are unchanged. Only the previously silent zero-marker case
changes, from success to a named failure.
Assisted-by: Claude:claude-opus-5
Stack created with GitHub Stacks CLI • Give Feedback 💬