ci: add built-artifact release gate (#765) - #766
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.
|
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.
|
@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.
6135f29 to
21492b4
Compare
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.
armando-navarro
left a comment
armando-navarro
left a comment
There was a problem hiding this comment.
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:acceptlets 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
requirecrash 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
-expversion rewrite today while the majors match, and still applies to prereleases of a genuine bump. - Pipeline wiring:
publishneeds 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.7reads as a release candidate withisMinorOrMajorBumpfalse, so the gate fails it as "a patch bump over 5.0.0". - A v4 minor
4.3.0is 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 against5.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:acceptprint 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 therequireexport condition removed,typingsdeleted, andpeerDependenciesdeleted still passes everything (I re-ran this against the current head). An in-gate assertion on the extracted tarball's critical fields, excludingversionsince 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-checkis not inpublish'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.
The gate downloaded `reactfire@latest` to compare against. That breaks the entire v4 line the moment 5.0.0 takes that dist-tag: every v4 candidate reads as a release candidate that is not a bump, because a lower major can never register as one. A 4.2.7 maintenance release, a 4.3.0 minor, and even the stamped canary builds CI packs would all fail against a type surface from a different major line, so the blast radius is every v4 pull request rather than just release cuts. Derive the target from the candidate instead: `reactfire@^4` for a 4.x build. Not a per-branch dist-tag, which would have to be maintained correctly on every publish, and reactfire is published by hand, so it would drift. Same reasoning that ruled out a committed baseline. When the candidate's major has nothing published (the v5 line before 5.0.0 ships), fall back to the dist-tag and say so in the log. npm reports that as ETARGET, which is distinct from the E404 it returns 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. `fetchPublished` now throws rather than defaulting when the caller does not say what it is comparing. Defaulting to `latest` would reinstate this exact bug, silently, in whichever caller forgot to pass the version. Reported by Armando in review.
Splits this pull request along the seam between the two issues it was serving. What remains is #765: does the built artifact still work. The type surface, which is #749, moves to the follow-up that also carries the API compatibility check, so it lands together with the thing that classifies it rather than ahead of it. The motivation is that the gate's `types` check answered "did anything change" and then asked a human to decide whether the change was breaking, via `gate:accept` and a committed digest. That decision is now made mechanically by the differ. Landing the acknowledgment here and deleting it days later would put code through `main` twice, and would discard review effort already spent on it. Removed: `checkTypes`, `typesDigest`, `diffLines`, `listTypeFiles`, `isReleaseCandidate`, `isMinorOrMajorBump`, the `types` field in `accepted.json`, and their tests. The acknowledgment survives for `size`, which has no automatic classifier: there is no principled way to decide that a bundle growing 4% was intended. One thing worth calling out, because a refactor could have reintroduced it silently. `checkSize` stayed quiet on a failed registry fetch and relied on `checkTypes` having already failed the run for the same reason. With `checkTypes` gone, that deferral would have turned a registry failure into a silent skip, which is exactly the fail-open this gate was fixed for earlier. `checkSize` now reports it itself, and a test pins it.
|
Thanks Armando, this was a genuinely useful review. Two of your asks are fixed and pushed (head is now The
|
tyler-reitz
changed the title
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
@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 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 |
…fies it Brings the `types` check into this pull request rather than the gate's, so the diff and its interpretation arrive together. FirebaseExtended#766 now covers FirebaseExtended#765, the runtime half; this covers FirebaseExtended#749, the type half. The check reports what moved and does not fail on it, because a type change is not a regression: adding a hook legitimately changes the surface. Whether the change is *breaking* is `Check API compatibility`'s answer. What still fails here is the bump: once package.json's numeric version moves off the published one, a changed type surface cannot ship as a patch. That is 4.2.4 stated precisely, and it is the half the differ cannot give, since adding an optional property to an interface is invisible to assignability but is still a minor. No acknowledgment. The `gate:accept` ritual existed to make a human decide additive against breaking; with that decided mechanically it bought a signature rather than a judgement, at the cost of reddening CI over a JSDoc edit and expiring every open pull request's acknowledgment on each release. It survives for `size`, which has no classifier. Two things worth flagging for review: `reportUnavailable` now reports a shared cause once, tracked on the report object. Both comparison checks fail for the same reason when the registry is unreachable, and one error under two check names buries it. Deliberately not done by having `size` defer to `types`: that coupling is invisible, and it already bit once, where removing `types` turned a registry failure into a silent skip because `size` had been staying quiet on its behalf. `check.mjs` now extracts the candidate before fetching, so it can derive the comparison target from the candidate's own major. It was calling `fetchPublished` with no version, which the gate now refuses rather than silently defaulting to the `latest` dist-tag. Without this the differ would compare every v4 build against the v5 surface once 5.0.0 ships, which is the defect the gate fixed.
jhuleatt
left a comment
jhuleatt
left a comment
There was a problem hiding this comment.
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.
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| This covers the **runtime** half of the release checks: does the built artifact | ||
| still work. The **type** half, diffing the emitted `.d.ts` against the published | ||
| release and applying semver to it, is | ||
| [#749](https://github.com/FirebaseExtended/reactfire/issues/749) and is handled | ||
| separately. It needs `typescript`, and keeping it out of this file is what lets | ||
| the gate stay dependency-free. |
There was a problem hiding this comment.
typescript is now covered by the normal test suite, correct?
| }); | ||
| }); | ||
|
|
||
| describe('parseProbeOutput', () => { |
There was a problem hiding this comment.
Some of these tests don't seem worth the extra lines of code. Could you please trim the test files down to core functionality?
There was a problem hiding this comment.
Was this file meant to be included? It reads more like an agent conversation summary
| * Checks: | ||
| * 1. no CJS interop / dynamic `require` in the ESM entry (issue #765, item 1) | ||
| * 2. externals stay external, nothing unexpected is inlined (issue #765, item 2) | ||
| * 3. every path in `exports`/`main`/`module`/`typings` exists (issue #765, item 4) |
There was a problem hiding this comment.
3 should be covered by the test suite, correct?
3 participants
Summary
Adds two CI jobs between
buildandpublishthat check 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.
Both jobs verify the same tarball the
publishjob uploads verbatim, so they check the exact bytes that ship. The gate is dependency-free by design (node builtins plustar), so it needs nonpm ciand cannot itself be broken by a dependency change.Addresses #765 items 1, 2, 3, 4 and 5. Item 6, the published
.d.tsdiff, is #749 and lands in a follow-up; see the scope note below.Scope: this is the runtime half only
An earlier revision of this PR also diffed the emitted
.d.tsand carriedCloses #749. That has moved out, so this PR now covers exactly one issue.The reason is that a textual
.d.tsdiff can tell you something changed but not whether the change is breaking, so it had to fail on any delta and ask a human to classify it vianpm run gate:acceptand a committed fingerprint. A follow-up implements that classification mechanically, by askingtscwhether the new type surface is assignable to the old one. The diff and the thing that interprets it belong in the same PR, so both are in the follow-up rather than split across two.Concretely:
gate:accepthere covers size only. That also closes a hole Armando found, where one acknowledgment blessed types and size together, so a size regression could ride along on a type change and surface only as a log note.Checks
no-cjs-in-esmexternalsexports-mapexports/main/module/typingsmissing from the tarball.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.Which release it compares against
sizecompares against a release downloaded withnpm pack, not against a copy committed to the repo. An earlier revision used a committed baseline and flagged the choice as needing a decision before merge. It is decided: a checked-in baseline goes stale the moment a release is published without refreshing it, and reactfire is published by hand, so that path is live rather than theoretical.The release it picks is the newest one sharing the candidate's major (
reactfire@^4for a 4.x build), not thelatestdist-tag.That detail matters more than it looks, and Armando caught it in review. Using
latestbreaks the entire v4 line the moment 5.0.0 takes that tag: a 4.2.7 compared against 5.0.0 is a different version but not an increase, because a lower major can never register as a bump, so maintenance releases, minors, and even the stamped canary builds CI produces would all be measured against an artifact from a different major line. That is every v4 pull request, not just release cuts.Deriving the target from the candidate rather than reading a per-branch dist-tag is deliberate: a
v4tag would have to be maintained correctly on every publish, which is the same drift argument that ruled out the committed baseline.When the candidate's major has nothing published yet (the
v5line before 5.0.0 ships), it falls back to the dist-tag and says so. npm reports that case asETARGET, distinct from theE404for a package that does not exist at all, so the two get different handling: no package stays a skip, no matching version falls back.A failed fetch fails the job after three attempts, rather than skipping. A skip is indistinguishable from a pass in a check's status, so a registry blip would otherwise produce a silently ungated release.
Entry-point load test (#765 item 3)
scripts/entry-load.mjsand the Verify package loads job install the packed tarball into a throwaway project with realreactandfirebase, then load both entry points and assert that known exports are present. 13s per leg.It is a separate script and job on purpose. It needs a real dependency tree, and keeping it out of
release-gate.mjsis what lets the gate stay dependency-free.It catches the #759 class by observing the failure rather than by pattern-matching the artifact. Verified against the published tarballs:
import('reactfire')require('reactfire')requireshim errorrequireshim errorNote that only the ESM entry breaks, so a check that loaded one entry point would have missed it, and
exports-mapcannot see it at all: every file it looks for is present in 4.2.5, the package simply does not run.This is worth flagging because it changes the case for item 7. Item 7's justification was being the only check that catches a runtime regression by observing it. That is no longer true, so its remaining unique value is narrower: failures that appear only in a browser or bundler context and not on a Node import.
Verification
Run against the published tarballs, not just the current build:
no-cjs-in-esmexternals* 4.2.3 predates the shim import entirely, so that is expected rather than a false positive.
test/release-gate.test.mjsandtest/entry-load.test.mjs, 83 tests, no emulators needed (npm run test:gate,npm run test:loads). 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). That caught a case where the size tolerance value was unpinned even though the check itself was covered, and a case where deleting the CJS branch of the load test left all its tests green.
Three bugs in this PR were found by reading output rather than by tests, all of them invisible while the type surface matched the published release:
4.2.6-exp.<sha>) read as a release bump, which would have failed the first PR to legitimately change types.Workflow hardening (zizmor)
Two of the eleven 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. The check fails on any Medium or higher, so partial fixes would not clear it.unpinned-uses, the only mandatory check.permissions: contents: readandpersist-credentials: falseon the 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.Now at 0 medium. The 7 remaining
cache-poisoningfindings are pre-existing in kind 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.fetchPublishedthrows rather than defaulting when a caller does not say which version it is comparing. Defaulting tolatestwould silently reinstate the v4-against-v5 bug in whichever call site forgot to pass it. This is not hypothetical: it caught exactly that omission in the follow-up branch.Incidental repo changes. An eslint
**/*.mjsoverride, since typescript-eslint switchesno-undefoff for.tsand.tsxbut not for plain.mjs;scripts/added to thelintandformatglobs; newtest:gate,test:loads,gateandloadsscripts; 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
.d.tsdiff and its semver classification (Add a published .d.ts diff to the release process to catch breaking type changes #749, Release process: add a built-artifact (dist/bundle) check to catch runtime regressions before publish #765 item 6) land in a follow-up, along with the API compatibility job.package.json's shape, only that referenced paths exist. Armando showed that a tarball with therequireexport condition removed,typingsdeleted, andpeerDependenciesdeleted still passes everything. Worth an in-gate assertion on the extracted manifest.gate:acceptcould print the size deltas it is about to bless, and refuse to record when the absolute checks fail, so a broken build cannot become a blessed snapshot.no-cjs-in-esmhas no escape hatch, and matching the barerequireidentifier means it can also match the word inside a string literal or comment. Latent rather than imminent:rxjsESM has zero occurrences. Fix is stripping strings and comments before matching.firebaseto^11.10.0, matching the repo's devDependency today. When the repo moves to firebase 12 the fixture will quietly keep testing against 11 unless the constant moves with it.needs: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. The gate's local pack works around it by staging fromgit ls-filesplusdist/.Addresses #765 (items 1 to 5)
Refs #749
🤖 Generated with Claude Code