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

[tyler-reitz]

Summary

Rewritten after your review. The 737-line release-gate.mjs, its 666-line test file and the release-gate/README.md are all gone, replaced with three off-the-shelf tools and one small script.

Based on #767, so this PR shows only its own diff. GitHub will retarget it to main when that merges.

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

• 4.2.4 turned ObservableStatus<T> into a discriminated union, breaking 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 inlined the CJS use-sync-external-store/shim into the ESM output, so import('reactfire') threw (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).

The source was fine both times. Only the emitted artifact regressed.

What runs

Two jobs, both against the same reactfire.tgz the publish job uploads, so they check the exact bytes that ship.

Verify package

Tool	What it catches
publint --strict	manifest problems: missing exports conditions, bad paths, files referenced but not shipped
attw	type resolution across node10, node16 (CJS), node16 (ESM) and bundler
size-limit	entry-point gzip against a fixed budget

Verify package loads (React 18 and 19) installs the tarball into a throwaway project with real react and firebase, then imports both entry points.

Why one script survived

Neither publint nor attw catches #759. Their output is byte-identical across 4.2.3, 4.2.4, 4.2.5 and 4.2.6. Both analyse package structure, and #759 was a require inlined into the body of a structurally valid ESM file. Actually loading the package is the only thing that catches it:

	import('reactfire')	require('reactfire')
4.2.4	throws	loads
4.2.5	throws	loads
4.2.6	loads	loads

Only the ESM entry breaks, so a check that loaded a single entry point would have missed it.

scripts/entry-load.mjs is 220 lines, covered by 9 tests (down from 25).

Two live bugs this surfaced

1. No types condition on either exports branch, so CJS consumers on node16 resolution got no type declarations at all. Predates 4.2.4. Fixed here, and that fix is what takes publint from exit 1 to "All good!".
2. 16 extensionless relative imports in the emitted .d.ts (from './useObservable'), which node16 resolution rejects. Real bug, and the fix belongs in src, so it gets its own issue rather than riding along here. Suppressed via .attw.json so every other resolution check keeps enforcing. Deleting that one ignore rule is the acceptance test for the fix.

Separately, size-limit was already a devDependency and already pointed at TSDX-era filenames the vite build stopped emitting, so npm run size was silently checking nothing.

What this gives up

Worth stating plainly rather than leaving you to find it:

1. Dropping the bespoke no-cjs-in-esm and externals checks means App Router crash: reactfire 4.2.4/4.2.5 throw "dynamic usage of require is not supported" on the client #759's class is caught by observing the failure rather than pattern-matching for it. Sufficient for App Router crash: reactfire 4.2.4/4.2.5 throw "dynamic usage of require is not supported" on the client #759, which throws on a Node import. An inlining that breaks only in a browser bundler would slip past. That gap is Release process: add a built-artifact (dist/bundle) check to catch runtime regressions before publish #765 item 7, re-homed to the V5 demo app.
2. size-limit's absolute budget will not catch a regression inside the current 3.7 kB of headroom. The bespoke ±2% delta would have. For scale, App Router crash: reactfire 4.2.4/4.2.5 throw "dynamic usage of require is not supported" on the client #759 moved the bundle +3.1%.

Scope

Addresses #765 items 1 to 5. #765 stays open for item 7.

The .d.ts surface check for #749 is a separate follow-up using api-extractor: about 35 lines of config plus a generated report, no network, no comparison release to fetch.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[tyler-reitz]

@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.

[armando-navarro]

Thanks Tyler. Reviewing the PR as it stands now, with the npm comparison and the new load test in. I verified the reworked machinery end to end and it holds up. Approving. The asks below are non-blocking on today's state, though the first two deserve attention before or right after merge.

What I verified

• The 94 tests pass (69 gate, 25 load), and a live gate run against npm 4.2.6 passes with 0.0% entry deltas. Since both sides are now measured on the same machine, the compression-environment noise a committed baseline would carry is designed away.
• The acknowledgment lifecycle behaves: an unacknowledged type drift fails, gate:accept lets exactly that surface through, and any later edit invalidates the digest. The suite also pins that a new npm release expires an old acknowledgment.
• With the registry unreachable, the gate fails loudly after three attempts instead of skipping. A skip would be indistinguishable from a pass, so this is the right call, and the failure text says so.
• The load test catches the real thing: against the published 4.2.5 tarball it fails with the exact rolldown require crash on the ESM entry, and the current build loads both entries cleanly on React 18.
• The version rule blocks a type change shipped as a patch (the 4.2.4 case), stays quiet for CI's -exp version rewrite today while the majors match, and still applies to prereleases of a genuine bump.
• Pipeline wiring: publish needs the test job plus both verify jobs on both publishing triggers, the new job reuses the same pinned action commits, needs no secrets, and runs on fork PRs.

The description no longer matches the PR

The body still describes the committed-baseline design. Five spots are now wrong:

• The "Decision needed before merge" section: the code has decided it.
• "Implements #765 items 1, 2, 4, 5": item 3 is in.
• The follow-ups bullet saying items 3 and 7 are not implemented.
• "38 tests": it is 69 plus 25 now.
• The new load-test job is not mentioned at all.

One more thought: since that decision had been explicitly flagged for Jeff, a short comment noting the switch and why would let his review start from the right frame rather than from the stale table.

On the footer specifically: your own description says that under the npm route "Closes #749" should become a plain reference with the issue left open, so that line wants the same update, with the final close-or-keep-open call staying with Jeff as you originally framed it. To be fair on the merits, the implementation does deliver #749's literal ask, so if Jeff prefers to let the merge close it, that is defensible too.

The latest comparison will block every v4 release once v5 ships

This one is invisible today and painful later, and since the v5 branch already exists I want it on the record now. I ran the version helpers against the future state where 5.0.0 holds npm latest:

• A v4 maintenance release 4.2.7 reads as a release candidate with isMinorOrMajorBump false, so the gate fails it as "a patch bump over 5.0.0".
• A v4 minor 4.3.0 is misjudged the same way, because a lower major can never register as a bump.
• Even a stamped canary build (4.2.6-exp.<sha>) reads as a release candidate against 5.0.0, and every v4 PR fails the types diff against the v5 surface before the version rule even runs. So the blast radius is the whole v4 line's CI, not just release cuts.
• Acknowledging clears the types diff but not the version rule, so a contributor who hits this cannot get green without a code change. The change itself is small (the comparand is one constant), which is exactly why it is worth planning rather than discovering.

Fine to land as-is today (latest is 4.2.6, so nothing fires). My ask is just to file it with a clear "before 5.0.0 takes latest" milestone, or fix it here if you prefer. Two shapes that seem workable: make the compare target branch-aware (a per-branch dist-tag, or derive it from the candidate's own major), or make the version rule major-aware. Relatedly, once the gate lands on v5, every PR there will diff against the v4 surface and demand acknowledgment churn against a baseline that means nothing for that branch, so the same fix helps both.

One gate:accept covers types and size together

accepted.json records the type digest and the size snapshot in one step, and the size check honors whatever the snapshot says:

• I built the ordinary case: a candidate with one benign added declaration file and a tarball 40% over published, acknowledged the normal way, passes with zero failures. The +40% shows up only as a log note.
• The committed file records the size as a raw byte count with no delta or percentage, so nothing in the acknowledgment diff itself flags that a size regression rode along with the type change.
• Bounded, to be fair: the primary inlining detectors do not consult the acknowledgment at all, so this only softens the size backstop.

Some cheap improvements:

• Have gate:accept print the size deltas it is about to bless, so whoever runs it sees what they are signing before committing (recording a readable delta in the file would go one better and put it in the PR diff too).
• Have it run the three absolute checks first and refuse to record on failure, which also keeps a broken build from becoming anyone's blessed snapshot.
• A related docs nit: the README's accepting-a-change section skips the build-first step its running-it section includes, so on a fresh clone the accept command errors until you build.

Two carried notes on the checker itself

• The checks never look at package.json's shape, only at whether referenced paths exist. A tarball with the require export condition removed, typings deleted, and peerDependencies deleted still passes everything (I re-ran this against the current head). An in-gate assertion on the extracted tarball's critical fields, excluding version since CI rewrites it, would close it.
• Nothing covers the wiring from a non-empty failures list to a non-zero exit. It works today (I checked all three exit codes), but a refactor could leave the gate exiting 0 forever with every test green. One child-process test against a crafted bad tarball would pin it.

Small notes

• The load job has no retry or outage handling the way the gate's fetch does, and neither new job caches npm downloads, so every PR pays two uncached installs per React version. Fine to leave, worth knowing.
• The load fixture pins firebase to ^11.10.0, which matches the repo's own devDependency today. When the repo moves to firebase 12, the fixture will quietly keep testing against 11 unless the constant moves with it. A one-line comment tying the two together would prevent the drift.
• type-check is not in publish's needs list (pre-existing, not this PR): a type error in source would not block a canary.

If any of this reads wrong, say so and I will dig back in. The version-helper runs and the coupling fixture are one-liners to re-run.

[tyler-reitz]

Thanks Armando, this was a genuinely useful review. Two of your asks are fixed and pushed (head is now b50ede7, CI green). One is dissolved rather than fixed, because the check it applied to has left this PR. The rest are open and I've said where each one is going.

The latest comparison: fixed here rather than filed

You were right that this is invisible today and painful later, and right that it is worth planning rather than discovering. I took your first suggested shape, deriving the target from the candidate's own major, so a 4.x build now compares against reactfire@^4.

I went with derivation over a per-branch dist-tag for the same reason we dropped the committed baseline: a v4 tag has to be maintained correctly on every publish, and since Jeff publishes by hand on a broken CI/CD path, it would drift. Deriving needs no upkeep.

When the candidate's major has nothing published (the v5 line before 5.0.0 ships), it falls back to the dist-tag and says so in the log. npm reports that case as ETARGET, which is distinct from the E404 for a package that does not exist at all, so the two get different handling: no package stays a skip, no matching version falls back.

Verified on a runner rather than just locally. From the CI log on the current head:

release gate: reactfire@4.2.6-exp.9c0a62c (reactfire.tgz)
comparing against reactfire@4.2.6 (npm ^4)

That is CI's stamped version deriving ^4 correctly, which was the case I most wanted to see, since it is the one your analysis showed would take out every v4 pull request rather than just release cuts.

One related change worth flagging: fetchPublished now throws when a caller does not say what it is comparing, rather than defaulting to latest. Defaulting would reinstate exactly this bug, silently, in whichever call site forgot to pass the version. That turned out to matter within the hour, which I will come back to below.

One gate:accept covering types and size: the types half has left this PR

Your ordinary case is a real hole, and I did not patch it. The types check moved out of this PR entirely, into the follow-up that carries a semantic API differ, so gate:accept here now covers size alone and there is nothing for a size regression to ride along on.

The reason for the move: the acknowledgment existed to make a human decide "additive or breaking". That decision is now made mechanically by a differ that asks tsc whether the new surface is assignable to the old one, so the ritual was buying a signature rather than a judgement, while reddening CI over a JSDoc edit and expiring every open PR's acknowledgment on each release.

Splitting it that way also means this PR now covers exactly #765, the runtime half, and the follow-up covers exactly #749, the type half. That is why the Closes #749 footer is going away, which lands in roughly the same place you did, just for a different reason.

This does mean part of what you verified no longer applies, specifically the acknowledgment lifecycle: the type digest, its invalidation on a later edit, and its expiry on a new release. I would rather tell you than have you find it in the diff. The size half of that lifecycle is unchanged and your verification of it still holds. If you would rather re-look before this merges, say so and I will hold it.

Your other two suggestions on gate:accept still stand against the size-only version, and I have not done them: printing the deltas it is about to bless, and running the absolute checks first so a broken build cannot become a blessed snapshot. Both are worth doing. The README nit is fixed; the accepting section now includes the build step.

A near miss worth recording

Removing the types check almost reintroduced the fail-open you verified earlier. checkSize was staying quiet on a failed registry fetch and relying on checkTypes having already failed the run for the same reason. Delete checkTypes and a registry failure becomes a silent skip across the whole gate.

Rather than move the coupling to the other check, reportUnavailable now dedupes on the report object, so both comparison checks report unconditionally and the first one wins. No check depends on another having run. Mutation-tested specifically, since it is invisible unless npm is unreachable.

The related catch: the differ was calling fetchPublished with no candidate version, and because that now throws rather than defaulting, it failed loudly on rebase instead of quietly comparing v4 builds against the v5 surface. That is the second time on this branch that a deliberate loud failure has paid for itself.

Still open

• The description. Yours is the second correct read of it and it is now further out of date than when you wrote that, since the check count changed again. Rewriting before I mark this ready.
• package.json shape assertion. Agreed this is a real gap, and your repro is convincing. Going into the follow-up rather than growing this PR a third time.
• Non-empty failures to non-zero exit. Same, and I want the child-process test you describe rather than a unit test, for the reason you give.
• Load job retry and npm caching, the firebase pin comment, type-check not in publish's needs. Noted, none done. The firebase pin one I would like to fix cheaply since the drift is silent.

Current state: 83 tests on this branch across the gate and load suites, no emulators needed, and the full workflow green on b50ede7. Still a draft until the description is rewritten.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[tyler-reitz]

@armando-navarro flagging you directly, since my reply above asks you something and I did not mention you in it.

The part that needs you rather than just informing you: this is now marked ready and shows as approved off your 08:48 review, but that review predates the two commits that changed the PR's scope. The types check and its acknowledgment lifecycle, which you verified in detail, have moved out of this PR into the follow-up that classifies type changes mechanically. The size half of that lifecycle is unchanged and your verification of it still holds.

So: happy to hold the merge until you have re-looked. No rush at all, this can wait until you are back.

Fixed from your review in the meantime: the latest comparand (now derives the target from the candidate's own major, verified on a runner) and the shared gate:accept. Your remaining findings are all recorded in the follow-ups section of the description.

[jhuleatt]

I appreciate the work, but need some convincing that this is worth the complexity it brings. release-gate.mjs alone is 700 lines, and I'm not confident I could debug it if it breaks down the line.

[jhuleatt]

Thanks for making the Zizmor changes, but please pull them out into their own PR and also change the docs workflow. That will keep this PR more focused

[tyler-reitz]

Done, split out as #767, with docs.yaml included. test.yaml goes from 8 medium to 0, and docs.yaml clears entirely. 32 lines, no functional change to any job.

Worth flagging one thing: I applied the hardening to main directly rather than cherry-picking the two commits from here, because they were authored on top of the gate and carry the verify-package job with them. So this PR will still show the workflow changes until #767 lands and I rebase.

These commits will come out of this PR as part of the rework.

[jhuleatt]

Some of these tests don't seem worth the extra lines of code. Could you please trim the test files down to core functionality?

[tyler-reitz]

Done. 25 tests down to 9, against a 220-line script.

One thing worth flagging: while trimming, I broke the require() half of the check on purpose to see if any test would notice. None did. Everything stayed green while the check was only really testing import(). The old 25 missed it too, so the trim exposed it rather than caused it. The 9th test is the one that catches it.

[jhuleatt]

3 should be covered by the test suite, correct?

[tyler-reitz]

No. Check 3 there is exports-map (#765 item 4), and the test suite never loads the packed tarball, so nothing in it would notice a path in exports missing from the shipped files.

publint --strict covers it now, along with manifest problems we weren't checking at all. It caught a live one: neither exports branch had a types condition, so CJS consumers on node16 got no declarations. Fixed in this PR.

[tyler-reitz]

Reworked this in place rather than opening a new PR, so your comments stay attached. It's a different PR now: release-gate.mjs (737 lines), its tests (666) and the README are gone, replaced by publint, attw, size-limit and one 220-line load script. 7 files, +689/-10.

Description is rewritten and all five of your comments have replies. Three are on files that no longer exist, so they'll show as outdated.

Based on #767 so the diff here is only this PR's own work. It'll retarget to main when that lands.

Dismissing for re-review: this approval predates a full rewrite of the PR. It was submitted against 47e3f96, which is no longer in this branch. release-gate.mjs, its tests and the README have been removed and replaced with publint, attw, size-limit and one load script. Already re-requested, no action needed until you are back.