ci: add built-artifact release gate (#765, #749) - #766
Draft
tyler-reitz wants to merge 9 commits into
Draft
Conversation
Two dist-level regressions shipped as patch releases with nothing in the release flow comparing the built artifact: the 4.2.4 ObservableStatus type break (#749) and the 4.2.4/4.2.5 App Router / Vite crash caused by the CJS use-sync-external-store shim being inlined into the ESM output (#759, fixed in #760). In both cases the source was fine and only the emitted artifact regressed. Adds scripts/release-gate.mjs, run in CI as a new "Verify built artifact" job between build and publish. It verifies the packed reactfire.tgz that the publish job uploads verbatim, so it checks the exact bytes that ship, and it is dependency-free so the job needs no npm ci. Checks (#765 items 1, 2, 4, 5 plus #749): - no-cjs-in-esm: no CJS interop or dynamic require in the ESM entry - externals: externals stay external, nothing unexpected is inlined - exports-map: every path in exports/main/module/typings is in the tarball - types: emitted .d.ts match a checked-in accepted baseline - size: packed tarball and entry-point gzip within 10% of baseline Verified against the published tarballs: 4.2.5 fails no-cjs-in-esm and externals, 4.2.4 fails those plus types (surfacing the ObservableStatus union as a readable diff), and the current build passes. #749 asks for enforcement rather than a warning, but a textual diff cannot classify additive versus non-additive on its own. So the gate fails on any delta and the acknowledgement is `npm run gate:accept` plus a committed baseline, which puts the semver decision in front of a reviewer as a diff. Also repairs the size-limit config, which pointed at TSDX-era filenames (dist/reactfire.esm.js) that the vite build has not emitted for some time, so `npm run size` was silently checking nothing. Items 3 and 7 of #765 (entry-point load test, runtime smoke render) are not included; see release-gate/README.md.
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Follow-up to review feedback on the release gate. Three fixes, all cases
where the gate quietly under-delivered.
1. Add test/release-gate.test.mjs (21 tests, no emulators needed).
The gate had no tests, which is the exact failure mode it exists to prevent:
edit a marker regex, break detection, and every run still goes green. A
silently disarmed gate is worse than no gate because it manufactures
confidence. Fixtures are byte-faithful to what shipped, including the 4.2.5
require shim, and cover the checks end to end against synthetic packages.
Required a small refactor for testability: checks now take a report object
instead of mutating module-level arrays, helpers are exported, and main()
only runs when the script is invoked directly.
2. Fix collectImports missing every minified import form.
The patterns required whitespace after the keyword, so `import{a}from"react"`,
`export{a as b}from"react"` and `export*from"rxjs"` all returned nothing. It
worked only because rolldown happens to emit spaced import statements while
mangling identifiers, which is an incidental output detail rather than a
contract. That is the same class of build-output change that caused #759, so
the gate's own parser was vulnerable to what it exists to detect. Partial
failure was the dangerous case: full failure trips MUST_BE_EXTERNAL loudly,
but a partial format change silently weakens the unexpected-externals half.
3. Make listTypeFiles recursive.
tsconfig emits with rootDir ./src, so a subdirectory of src/ emits nested
declarations. The flat readdir meant dist/nextjs/*.d.ts would sit entirely
outside the #749 check the moment #739 lands src/nextjs.
Also adds an eslint override so plain .mjs is linted as Node with vitest
globals (typescript-eslint disables no-undef for .ts, but not .mjs), and
extends the lint and format globs to cover scripts/.
An independent review found the gate claimed broader coverage than it had. All five are fixed, and every fix is pinned by a test that fails when the fix is reverted (verified by mutating each one back). 1. exports-map never checked `main` or `typings`. It filtered to paths starting with ".", but `main` is "dist/index.umd.cjs" and `typings` is "dist/index.d.ts", neither of which has the prefix. So the two fields its own failure message named were the two it skipped. Probed directly: with both missing and no exports map, it reported zero failures. It passed until now only because exports['.'].require duplicates `main`; `typings` has no equivalent and was genuinely unchecked. The test that appeared to cover this deleted the one path that was covered, so it passed for the wrong reason. 2. The size tolerance was too loose to catch what it advertised. Measured from the published tarballs, the #759 shim inlining moved the ESM entry +3.1% gzip and the tarball +0.3%, both well inside the old ±10% band. The claim that a size guard "might well have caught the shim getting inlined" was wrong, and is corrected in the README: size is a coarse guard against gross packaging changes, not an inlining detector. Tolerance is now ±2%, above CI's version-stamp churn (+0.5% tarball) and below a #759-sized change. The check also had zero test coverage; deleting it outright left every test green. It is now covered, including at the #759 magnitude. 3. Added and removed .d.ts detection was unpinned. The logic was correct but nothing held it there. This is the case the recursion fix in the previous commit exists to feed: a new entry point emitting dist/nextjs/*.d.ts is exactly #739, and a disappearing declaration file is a hard breaking change. 4. Both bundle checks only ever opened the `module` entry. Nothing guarantees a single ESM chunk. A second entry point or rollup deciding to split would put code where an inlined require is invisible, and chunking is the kind of incidental build-output change this gate exists to be robust against. Both checks now walk every ESM file in the package, and expected externals are matched across their union so an import from a chunk still counts. 5. `react` was matched as an exact string, and --accept took a tarball. Switching to the automatic JSX runtime would import react/jsx-runtime and double false positive: unexpected external, plus "react was inlined" when it was not. Now matched as a pattern, like firebase already was. Separately, `--accept <tarball>` would have recorded a baseline from an arbitrary old build, blessing whatever regression it contained; it is now rejected, as are unknown flags.
The zizmor security scan reports 15 `unpinned-uses` findings against this file: actions referenced by floating tag (`actions/checkout@v4`) rather than pinned to a commit hash, which the blanket policy requires. A floating tag can be repointed at new code by whoever controls it, so an unpinned action is an unreviewed dependency with access to the job. Only 3 of the 15 are lines this branch added; the other 12 predate it. The scan runs only when a workflow file changes, and this is the first PR to touch .github/workflows/, so the condition was pre-existing and simply never surfaced before. Fixing only the 3 new ones would leave the check red, since it fails on any Medium/High finding. Each SHA is the current tip of that action's v4 tag, with the resolved release in a trailing comment so the version stays legible and Dependabot can still bump them. docs.yaml has the same two unpinned refs and is not touched here: the scan did not flag it, and this PR is already carrying a release gate. Worth a separate pass.
Clears the two remaining zizmor medium findings on test.yaml. excessive-permissions (6): the workflow declared no `permissions:` block, so every job ran with the default token scope. Added a top-level `permissions: contents: read`. No job writes to the repo through GITHUB_TOKEN; the publish job authenticates to npm with NODE_AUTH_TOKEN, so this does not touch the publish path's credentials. artipacked (4): actions/checkout persists the token into .git/config by default, leaving it readable by every later step in the job. Nothing here re-uses git credentials after checkout, so `persist-credentials: false` is safe. The publish job has no checkout step and is unchanged. Both classes predate this branch. Verified with zizmor 1.25.2 locally: test.yaml goes from 10 medium to 0. The 6 remaining cache-poisoning findings are pre-existing, are already suppressed by the CI config (the failing run exited 13/Medium, not 14/High), and fixing them would mean restructuring how the workflow caches around the publish path, which does not belong in this PR. docs.yaml has findings of its own and is untouched here; the scan only runs on changed workflow files. Worth a separate pass.
Contributor
Author
|
@jhuleatt this is ready for review. All checks green. Summary of what needs you, roughly in order of how much it matters: 1. One design decision, and it is the reason I have not merged this. #749 asks for the candidate's types to be diffed against the last published version on npm. This PR instead diffs against a baseline committed in the repo, updated deliberately via My lean is the npm-published version, but I did not want to change it unilaterally, because merging as-is auto-closes #749. If we go the npm route I would rather downgrade that to a plain reference and keep the issue open. Happy either way, I just want the call to be yours since it is your issue. 2. Two things needing admin, which I do not have.
3. Heads-up on something this PR uncovered, no action needed from you here. This is the first PR to touch 4. Also correcting myself. An earlier revision of this description claimed a working size guard would likely have caught the #759 shim inlining. That was wrong. Measured against the published tarballs, that regression moved the ESM entry only +3.1% gzip and the tarball +0.3%, so the ±10% band I originally used would have sailed straight past it. Tolerance is now ±2% and the description says plainly that Scope note: this implements #765 items 1, 2, 4, 5 plus #749. Items 3 and 7 (entry-point load test, runtime smoke render) are not included and are documented in Separately, #740 is still waiting on your semver call whenever you get a chance. |
#749 specifies packing the last published version and diffing against that. The gate diffed against a copy checked into `release-gate/baseline/` instead, which surfaces at PR time but can drift from npm. That drift is live rather than theoretical here: reactfire is published by hand, so a release can land on npm without the baseline being refreshed. The `types` and `size` checks now compare against `npm pack reactfire@latest`. If the registry is unreachable, or the package has never been published, both skip with a note rather than failing, so an outage cannot wedge a pull request that has nothing to do with the published surface. The bundle checks need no network and always run. Removing the baseline removes what `--accept` used to record, so the acknowledgment becomes a fingerprint: `release-gate/accepted.json` holds a digest of the type surface, the measured sizes, and the published version they were taken against. PR-time enforcement is preserved (a type change still fails until someone runs `npm run gate:accept` and commits the result, so the semver call is still made in review), but there is no longer a second copy of every `.d.ts` to maintain. The digest covers the exact surface accepted, so a later edit invalidates it rather than riding along, and it is scoped to one published version, so it expires on the next release instead of carrying forward silently. Also implements the half of #749 that was missing: an acknowledgment alone did not stop a type change shipping as a patch, which is exactly what 4.2.4 did. Once `package.json` moves off the published version the gate treats it as a release candidate and requires a minor or major bump for a changed type surface. During normal development the two versions match (the bump is its own commit, e.g. 7f93210 "4.2.6"), so the rule stays quiet and cannot false-positive on every types-touching PR. Classifying a diff as additive versus breaking still needs a real API differ and is deliberately left out. Tests go 38 to 56, covering the acknowledgment lifecycle (accepted, invalidated by a later edit, expired by a new release), the three version-rule cases, the no-published-release skip, the digest, and version parsing. Verified end to end against the published tarballs: 4.2.4 and 4.2.5 both fail, 4.2.6 passes.
The npm comparison had one fail-open path: any failure to download the published release, including a registry outage, skipped the `types` and `size` checks with a note and left the job green. A skip is indistinguishable from a pass in the check's status, so a blip would have produced a silently ungated release, which is the failure mode this gate exists to prevent. The two causes are now separated. "Never published" stays a skip, since at bootstrap there is genuinely nothing to compare against and it will not resolve by retrying. A failed fetch is retried three times with backoff and then reported as a failure, naming the underlying error and saying the job is re-runnable. Wedging a pull request for a few minutes is a better trade than a gate that has quietly stopped gating. The failure is reported once rather than once per affected check, so the registry error does not bury itself under two check names. `fetchPublished` now takes an injectable runner, so both classification and the retry behaviour are covered without touching the network: a 404 is not retried, a timeout is retried three times, and a retry that succeeds returns the package. Tests go 56 to 64.
CI stamps an experimental version into package.json before packing, so the gate sees `4.2.6-exp.<sha>` while 4.2.6 is published. Two comparisons assumed the candidate and the published release would be identical outside a real release, and both were wrong in CI. The version rule compared version strings. Since the stamped version is never equal to the published one, every pull-request build read as a release candidate, and since the numeric core matches, as a patch bump. The first pull request to legitimately change the type surface would have failed with a version error unrelated to the change. It compares the numeric core now, so a stamped build of the published version is not a release while a genuine bump, including its prereleases, still is. The size acknowledgment compared recorded byte counts exactly. Those numbers come from `gate:accept` run locally and are checked against a CI build, which the stamp alone makes differ (measured ~+0.5% on the tarball), so no acknowledgment would ever have applied in CI and an intended size change could not be landed at all. Recorded sizes are now matched within the existing tolerance, with the entry set still required to agree so an acknowledgment cannot silently cover a new entry point. Neither surfaced on this branch because the type surface is currently identical to 4.2.6, so the checks returned before reaching either comparison. Found by reading the CI log rather than from a failure. Tests go 64 to 69, covering the stamped-version case, a prerelease of a genuine bump in both directions, and acknowledgment matching at CI-sized drift, beyond tolerance, and across a changed entry set.
tyler-reitz
force-pushed
the
chore/built-artifact-gate
branch
from
July 28, 2026 19:51
6135f29 to
21492b4
Compare
tyler-reitz
marked this pull request as draft
July 28, 2026 20:46
Installs the packed tarball into a throwaway project with real react and firebase, then actually loads both entry points and checks that known exports are present. Runs against React 18 and 19, mirroring the type-check matrix, as the new "Verify package loads" job. `publish` now also gates on it. This catches the #759 class by observing the failure rather than by pattern-matching the artifact. Verified against the published tarballs: 4.2.4 import() throws the require-shim error, require() loads 4.2.5 import() throws the require-shim error, require() loads 4.2.6 both load Only the ESM entry breaks, so a check that loaded one entry point would have missed it. `exports-map` cannot see this at all, since every file it looks for is present in 4.2.5, the package just does not run. Deliberately a separate script and a separate CI job rather than a sixth check inside release-gate.mjs. It needs a real dependency tree, and keeping it out of the gate is what lets the gate stay dependency-free and unbreakable by a dependency change. A load that resolves but exports nothing is still broken, so the probes assert known exports from several submodules rather than only that the import settled. A probe that dies without printing is reported as a failure rather than read as a pass. 25 tests. The runner is injectable, so the orchestration is covered without installing anything. Mutation testing found a real gap: removing the CJS branch from collectFailures left every test green, because the suite only covered an ESM-only failure. Added the mirror cases. The remaining half of #765, item 7 (runtime smoke render), now has a narrower justification: it was argued for as the only check that observes a runtime failure, and this one does that for the #759 class. Its unique value is now limited to browser-only breakage. Noted in the README rather than acted on.
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
2 participants
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.
Summary
Adds a built-artifact release gate: a new Verify built artifact CI job between
buildandpublishthat checks the packedreactfire.tgzbefore it can be published.Two dist-level regressions have shipped as patch releases with nothing in the release flow comparing the built artifact:
ObservableStatus<T>from a flat interface into a discriminated union. Type-only, but it broke strict-TS consumers on a patch bump (Add a published .d.ts diff to the release process to catch breaking type changes #749).use-sync-external-store/shiminto the ESM output, producing a dynamicrequire()that threw in any browser bundle (App Router crash: reactfire 4.2.4/4.2.5 throw "dynamic usage of require is not supported" on the client #759, fixed by fix: externalize useSyncExternalStore shim to fix App Router crash #760).In both cases the source was fine and only the emitted artifact regressed.
The job verifies the same tarball the
publishjob uploads verbatim, so it checks the exact bytes that ship. It is dependency-free by design (node builtins plustar), so it needs nonpm ciand cannot itself be broken by a dependency change.Implements #765 items 1, 2, 4, 5 plus #749.
Decision needed before merge
#749 specifies packing the last published version and diffing against that. This PR instead diffs against a baseline committed in the repo (
release-gate/baseline/), updated deliberately vianpm run gate:accept.My lean is diffing against npm, since it cannot drift and is closer to the issue's intent. I have not changed it unilaterally because merging as-is auto-closes #749 — if we go the npm route, that should become a plain reference and the issue should stay open.
Checks
no-cjs-in-esmexternalsexports-mapexports/main/module/typingsmissing from the tarball.types.d.tsversus the accepted baseline, including added and removed declaration files.sizeOn
sizespecifically: it is a coarse guard against gross packaging changes, not an inlining detector, and should not be read as one. Measured from the published tarballs, the #759 inlining moved the ESM entry only +3.1% gzip (16815 to 17330 B) and the tarball +0.3%. The ±2% band is set above CI's version-stamp churn (+0.5% tarball, +0.2% entries) and below a #759-sized change.no-cjs-in-esmandexternalsare what actually catch inlining.Verification
Run against the published tarballs, not just the current build:
no-cjs-in-esmexternalstypes* 4.2.3 predates the shim import entirely, so that is expected rather than a false positive.
The 4.2.4
typesfailure renders theObservableStatuschange as a readable inline diff, which is the #749 case exactly.test/release-gate.test.mjs, 38 tests, no emulators needed (npm run test:gate). A gate that silently stops gating is worse than no gate, so the detection logic is pinned by tests rather than by having been verified by hand once. Fixtures are byte-faithful to the shapes that shipped, including the 4.2.5requireshim. Each check was mutation-tested (revert the logic, confirm a test fails), which caught one case where the size tolerance value was unpinned even though the check itself was covered.Workflow hardening (zizmor)
Two of the five commits are unrelated to the gate. This is the first PR to touch
.github/workflows/, and the zizmor scan only runs when a workflow file changes, so it fired here for the first time against a pre-existing, repo-wide condition:main'stest.yamlscores 34 findings (17 high, 8 medium). The check fails on any Medium or higher, so partial fixes would not clear it.unpinned-uses, the only mandatory check. Only 3 of the 15 were lines this branch added.permissions: contents: readandpersist-credentials: falseon the four checkouts. Clearedexcessive-permissionsandartipacked. No job writes to the repo viaGITHUB_TOKEN, and the publish job authenticates to npm withNODE_AUTH_TOKEN; that job has no checkout step and is otherwise unchanged.The 6 remaining
cache-poisoningfindings are pre-existing and already suppressed by the CI config. Fixing them would mean restructuring how the workflow caches around the publish path, which does not belong here.Notes for review
The obvious grep for #765 item 1 does not work, which is why the check looks the way it does. The issue suggests grepping the ESM dist for
require(. The shipped 4.2.5 bundle never writesrequire(: rolldown emittedtypeof require < "u"guards andrequire.apply(this, arguments), and minification renamed__commonJSto a single letter. A literal implementation of item 1 would have passed 4.2.5 clean. The gate matches the barerequireidentifier instead, across every ESM chunk rather than just the entry, since nothing guarantees the build stays single-chunk.On #749 wanting enforcement, not a warning. A textual diff cannot classify additive versus non-additive on its own. So the gate fails on any
.d.tsdelta, and the acknowledgement isnpm run gate:acceptplus a committed baseline, which puts the semver decision in front of a reviewer as a diff. Semantic classification via api-extractor would remove the judgement call but is a much larger project, deliberately not attempted here.Incidental repo changes. An eslint
**/*.mjsoverride, since typescript-eslint switchesno-undefoff for.tsand.tsxbut not for plain.mjs;scripts/added to thelintandformatglobs; a newtest:gatescript; and a repair to thesize-limitconfig, which pointed at TSDX-era filenames the vite build has not emitted for some time, sonpm run sizewas silently checking nothing.Follow-ups, not in this PR
reactandfirebaseinstalled, which would cost the gate its dependency-free property. Item 7 (runtime smoke render against Next App Router and Vite) is the strongest check and the only one that would have caught App Router crash: reactfire 4.2.4/4.2.5 throw "dynamic usage of require is not supported" on the client #759 by observing the crash rather than pattern-matching the artifact, but it is roughly a day and adds real CI minutes and flake surface. Neither is needed to close the two holes that actually shipped.no-cjs-in-esmhas nogate:acceptescape hatch, and matching the barerequireidentifier means it can also match the word inside a string literal or comment. Risk is latent rather than imminent:rxjsESM has zero occurrences, andrxfire's 8 are all in.cjs.jsfiles the ESM bundle never resolves. Fix is stripping strings and comments before matching.needs: [test, verify-package]edge. Making it required is about blocking merges.docs.yamlis still unhardened (2 medium, 2 high, plus 2 unpinned refs) and will surprise whoever edits it next.filesincludessrc, and npm does not exclude a nestednode_modules. A localnpm packwithsrc/nextjs/node_modulespresent produces a 186 MB tarball. Harmless in CI, but a live hazard for feat(nextjs): add firebase-cookie-middleware with security hardening #739: ifsrc/nextjslands as-is, everything under it publishes to npm. Thefilesallowlist probably wants narrowing as part of that packaging decision. The gate's local pack works around it by staging fromgit ls-filesplusdist/.Closes #749
Addresses #765 (items 1, 2, 4, 5)
🤖 Generated with Claude Code