ci: add built-artifact release gate (#765, #749) by tyler-reitz · Pull Request #766 · FirebaseExtended/reactfire

tyler-reitz · 2026-07-27T21:33:51Z

Summary

Adds a built-artifact release gate: a new Verify built artifact CI job between build and publish that checks the packed reactfire.tgz before it can be published.

Two dist-level regressions have shipped as patch releases with nothing in the release flow comparing the built artifact:

4.2.4 changed 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).
4.2.4 / 4.2.5 bundled the CJS use-sync-external-store/shim into the ESM output, producing a dynamic require() 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 publish job uploads verbatim, so it checks the exact bytes that ship. It is dependency-free by design (node builtins plus tar), so it needs no npm ci and 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 via npm run gate:accept.

	committed baseline (this PR)	diff against npm
Surfaces the diff	at PR time, as a reviewable change	at release time
Network needed	no	yes
Can drift	yes, if a release goes out without updating it	no
Matches what #749 asks for	not literally	yes

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

Check	What it catches
`no-cjs-in-esm`	A CJS module inlined into any ESM chunk, i.e. the #759 crash.
`externals`	Externals getting inlined (duplicate React instance, the shim regression) or new dependencies leaking out as runtime imports.
`exports-map`	Any path in `exports` / `main` / `module` / `typings` missing from the tarball.
`types`	Any change to the emitted `.d.ts` versus the accepted baseline, including added and removed declaration files.
`size`	Packed tarball and entry-point gzip moving more than ±2%.

On size specifically: 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-esm and externals are what actually catch inlining.

Verification

Run against the published tarballs, not just the current build:

	4.2.3	4.2.4	4.2.5	current 4.2.6
`no-cjs-in-esm`	pass	fail	fail	pass
`externals`	fail*	fail	fail	pass
`types`	n/a	fail	pass	pass

* 4.2.3 predates the shim import entirely, so that is expected rather than a false positive.

The 4.2.4 types failure renders the ObservableStatus change 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.5 require shim. 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's test.yaml scores 34 findings (17 high, 8 medium). The check fails on any Medium or higher, so partial fixes would not clear it.

All 15 action references pinned to commit SHAs, with the resolved release in a trailing comment so versions stay legible and Dependabot can still bump them. Cleared unpinned-uses, the only mandatory check. Only 3 of the 15 were lines this branch added.
Top-level permissions: contents: read and persist-credentials: false on the four checkouts. Cleared excessive-permissions and artipacked. No job writes to the repo via GITHUB_TOKEN, and the publish job authenticates to npm with NODE_AUTH_TOKEN; that job has no checkout step and is otherwise unchanged.

The 6 remaining cache-poisoning findings 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 writes require(: rolldown emitted typeof require < "u" guards and require.apply(this, arguments), and minification renamed __commonJS to a single letter. A literal implementation of item 1 would have passed 4.2.5 clean. The gate matches the bare require identifier 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.ts 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. 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 **/*.mjs override, since typescript-eslint switches no-undef off for .ts and .tsx but not for plain .mjs; scripts/ added to the lint and format globs; a new test:gate script; and a repair to the size-limit config, which pointed at TSDX-era filenames the vite build has not emitted for some time, so npm run size was silently checking nothing.

Follow-ups, not in this PR

Release process: add a built-artifact (dist/bundle) check to catch runtime regressions before publish #765 items 3 and 7 are not implemented, so Release process: add a built-artifact (dist/bundle) check to catch runtime regressions before publish #765 stays open. Item 3 (both entry points load) needs a fixture with react and firebase installed, 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-esm has no gate:accept escape hatch, and matching the bare require identifier means it can also match the word inside a string literal or comment. Risk is latent rather than imminent: rxjs ESM has zero occurrences, and rxfire's 8 are all in .cjs.js files the ESM bundle never resolves. Fix is stripping strings and comments before matching.
Making this a required status check needs admin. Worth being precise: the gate already blocks publishing via the needs: [test, verify-package] edge. Making it required is about blocking merges.
docs.yaml is still unhardened (2 medium, 2 high, plus 2 unpinned refs) and will surprise whoever edits it next.
files includes src, and npm does not exclude a nested node_modules. A local npm pack with src/nextjs/node_modules present produces a 186 MB tarball. Harmless in CI, but a live hazard for feat(nextjs): add firebase-cookie-middleware with security hardening #739: if src/nextjs lands as-is, everything under it publishes to npm. The files allowlist probably wants narrowing as part of that packaging decision. The gate's local pack works around it by staging from git ls-files plus dist/.

Closes #749
Addresses #765 (items 1, 2, 4, 5)

🤖 Generated with Claude Code

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.

gemini-code-assist · 2026-07-27T21:34:18Z

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.

tyler-reitz · 2026-07-28T00:55:34Z

@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 npm run gate:accept. Committed baseline surfaces the diff at PR time, needs no network, and cannot be skipped; but it can drift from npm if a release ever goes out without updating it. Diffing against npm cannot drift, and is closer to what the issue actually asks for.

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.

Make Verify built artifact a required status check on main. Worth being precise: the gate already blocks publishing through the needs: [test, verify-package] edge, so that path is covered today. Making it required is about blocking merges.
Protect the v5 branch. main is protected, v5 is not, and v5 now holds real work (aec5b83 forward-integration, 0b70a12 for fix: surface observable errors via status instead of re-throwing #735). An accidental force-push or branch deletion would lose it. Ask is to mirror main's protection onto v5, with one carve-out: Publish (NPM) must not be a required check there, because forward-integration heads are simultaneously main's tip and inherit main's own failing publish run. That is why chore: forward-integrate main into v5 #758 showed as UNSTABLE while still being mergeable.

3. Heads-up on something this PR uncovered, no action needed from you here.

This is the first PR to touch .github/workflows/, and the zizmor scan only runs when a workflow file changes, so it fired for the first time against a pre-existing, repo-wide condition. main's test.yaml scores 34 findings (17 high, 8 medium). Two commits here clear it for that file. docs.yaml is still unhardened and will do the same thing to whoever edits it next. Worth its own pass at some point.

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 size is a coarse packaging guard, not an inlining detector. Flagging it because it was the stated rationale for repairing the dead size-limit config.

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 release-gate/README.md, so #765 stays open. Item 7 is roughly a day on its own and adds real CI minutes and flake surface.

Separately, #740 is still waiting on your semver call whenever you get a chance.

tyler-reitz requested review from armando-navarro and jhuleatt July 27, 2026 21:33

tyler-reitz assigned jhuleatt Jul 27, 2026

tyler-reitz added 4 commits July 27, 2026 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ci: add built-artifact release gate (#765, #749) - #766

ci: add built-artifact release gate (#765, #749)#766
tyler-reitz wants to merge 5 commits into
mainfrom
chore/built-artifact-gate

tyler-reitz commented Jul 27, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot commented Jul 27, 2026

Uh oh!

tyler-reitz commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

tyler-reitz commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Decision needed before merge

Checks

Verification

Workflow hardening (zizmor)

Notes for review

Follow-ups, not in this PR

Uh oh!

gemini-code-assist Bot commented Jul 27, 2026

Uh oh!

tyler-reitz commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tyler-reitz commented Jul 27, 2026 •

edited

Loading