ci: guard the last two generated bundles against drift - #3355
Conversation
`generate:manifests:check` runs inside `typecheck`, so stale generated output already fails a required check, but it only covered four of the six generators in `deno task generate`. prebundle-bridge.ts and prebundle-rsc-scripts.ts had no --check counterpart at all, and both write committed artifacts: src/studio/bridge/bridge-bundle.generated.ts src/server/services/rsc/endpoints/rsc-bundles.generated.ts Editing the Studio bridge or the RSC endpoint sources and forgetting to regenerate left those bundles stale with nothing failing. Both now take --check using the pattern from prebundle-hydration-runtime. The RSC bundle is formatted after it is written, so its check compares post-format on both sides rather than against the raw template. Verified by running each with --check on a clean tree (passes) and against a deliberately stale bundle (exits 1 with the regenerate instruction). Every generator in `generate` now has a check counterpart.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe prebundle generators now support ChangesGenerated artifact validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/prebundle-bridge.ts`:
- Around line 52-68: Add focused regression tests for the --check behavior in
scripts/build/prebundle-bridge.ts (lines 52-68), covering current and stale or
missing committed bundles; the implementation site requires no direct change.
Add corresponding success and failure tests in
scripts/build/prebundle-rsc-scripts.ts (lines 167-205), ensuring comparison uses
the formatted generated output; the implementation site requires no direct
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e914f3df-917c-4dc1-989b-61bb59765d81
📒 Files selected for processing (3)
deno.jsonscripts/build/prebundle-bridge.tsscripts/build/prebundle-rsc-scripts.ts
There was a problem hiding this comment.
Pull request overview
This PR extends Veryfront’s existing generated-artifact drift checks to cover the last two generators invoked by deno task generate, ensuring committed Studio bridge and RSC client bundles fail CI when stale.
Changes:
- Added
--checkmode toscripts/build/prebundle-bridge.tsto compare the committed bridge bundle against freshly generated output. - Added
--checkmode toscripts/build/prebundle-rsc-scripts.ts, formatting candidate output viadeno fmtbefore comparing to the committed file. - Wired both
--checkscripts intodeno task generate:manifests:checkso the requiredtypecheckgate catches drift.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scripts/build/prebundle-rsc-scripts.ts | Adds --check that formats generated output and compares it to the committed RSC bundles. |
| scripts/build/prebundle-bridge.ts | Adds --check that compares freshly bundled Studio bridge output to the committed generated file. |
| deno.json | Extends generate:manifests:check to include the two newly checkable generators. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review follow-ups on the generated-bundle guard. esbuild.stop() was called without await in prebundle-rsc-scripts, both in the new --check path (right before an early Deno.exit, where it matters most) and in the pre-existing write path one line below. Every other caller awaits it, so both now do. For tests, the realistic regression is not that the comparison is wrong, it is that a seventh generator gets added to `deno task generate` and its --check counterpart is forgotten. That is exactly how bridge and rsc drifted out of the guard in the first place. The new test expands both task chains and asserts they name the same scripts, that the check task passes --check to each, and that it checks nothing `generate` does not run. Verified by dropping the rsc check from the task: the test fails and names the offending script. Spawning the generators for real would cost an esbuild pass per case and prove little that `generate:manifests:check` does not already prove on every PR, so this checks the wiring instead. Wired into the CI lint job rather than only `test:scripts`, because no workflow invokes that task. (`test:scripts` also has a pre-existing failure in generate-api-reference, unrelated to this change.)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/generated-artifact-checks.test.ts`:
- Line 16: Update generated-artifact tests to import assertions from
`#veryfront/testing/assert.ts` and use describe() and it() from
`#veryfront/testing/bdd.ts` instead of `#std/assert` and Deno.test. Apply this
consistently throughout the test cases covered by the generated-artifact test
suite, preserving their existing assertions and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86f66004-43cb-4e20-b4e4-bc6f4065a898
📒 Files selected for processing (4)
.github/workflows/cicd.ymldeno.jsonscripts/build/generated-artifact-checks.test.tsscripts/build/prebundle-rsc-scripts.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/build/prebundle-rsc-scripts.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
scripts/build/prebundle-rsc-scripts.ts:220
- In the write path,
await esbuild.stop()won’t run ifwriteTextFile()or thedeno fmtcommand throws (for example due to filesystem permissions). Wrapping the write/format block intry/finallymakes cleanup deterministic and matches the pattern used elsewhere (e.g. build-npm-extension-packages.ts uses afinallyfor esbuild cleanup).
await Deno.writeTextFile(outputPath, output);
const fmtResult = await new Deno.Command("deno", {
args: ["fmt", outputPath],
stdout: "null",
stderr: "piped",
}).output();
if (!fmtResult.success) {
const err = new TextDecoder().decode(fmtResult.stderr).trim();
console.warn(`[prebundle-rsc-scripts] Warning: could not format output: ${err}`);
}
await esbuild.stop();
scripts/build/generated-artifact-checks.test.ts:45
scriptInvocations()stores one boolean per script path, butMap.set()overwrites prior entries. If a task ever invokes the same script twice (for example once with--checkand once without), this test could miss the missing-flag case depending on ordering. Consider aggregating so the stored value becomes false if any invocation of that script lacks--check.
for (const segment of command.split("&&")) {
const script = segment.match(/([\w./-]+\.ts)/)?.[1];
if (!script) continue;
found.set(script, segment.includes("--check"));
}
scripts/build/prebundle-rsc-scripts.ts:205
- In --check mode,
await esbuild.stop()runs only afterformatTypeScript(output)succeeds. Ifdeno fmtfails andformatTypeScriptthrows, the esbuild service is never stopped (and becauseDeno.exit(...)bypassesfinally, it is safest to stop explicitly before any early exit). Consider stopping esbuild in afinallyaround the formatting step so it runs even when formatting errors.
if (Deno.args.includes("--check")) {
const formatted = await formatTypeScript(output);
const committed = await Deno.readTextFile(outputPath).catch(() => null);
await esbuild.stop();
AGENTS.md requires describe()/it() from #veryfront/testing/bdd.ts and assertions from #veryfront/testing/assert.ts for test files. I had followed scripts/build/dnt-polyfill.test.ts instead, which uses #std/assert and Deno.test, but that is the older minority pattern. Both specifiers do resolve under scripts/test.deno.json, which maps #veryfront/ to ../src/, so there was no technical reason to deviate. Re-verified after the conversion: dropping the bridge check from the task still fails the test and names the offending script.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
scripts/build/prebundle-bridge.ts:68
prebundle-bridge.tsuses esbuild vianpm:esbuild, but the new--checkpath (now wired intogenerate:manifests:checkin CI) does not callesbuild.stop(). Other scripts that use esbuild consistently stop it (for examplescripts/build/prebundle-hydration-runtime.ts:127-129andscripts/build/build-npm-extension-packages.ts:499-530), which avoids leaving the esbuild child process alive and potentially keeping the Deno process open longer than intended.
Consider stopping esbuild in both the --check and write paths, and ensure it happens before any Deno.exit(1) on stale output.
// --check makes a stale committed bundle fail CI instead of relying on someone
// noticing it missing from a PR diff.
if (Deno.args.includes("--check")) {
const committed = await Deno.readTextFile(outputPath).catch(() => null);
if (committed !== output) {
console.error(
`[prebundle-bridge] ${outputPath} is stale.\n` +
` The committed bundle does not match src/studio/bridge/.\n` +
` Run \`deno task generate\` and commit the result.`,
);
Deno.exit(1);
}
console.log("[prebundle-bridge] Committed bundle is up to date");
} else {
await Deno.writeTextFile(outputPath, output);
console.log(`[prebundle-bridge] Written to ${outputPath} (${js.length} bytes)`);
}
scripts/build/prebundle-rsc-scripts.ts:205
- In the
--checkpath,formatTypeScript(output)runs beforeawait esbuild.stop(). Ifdeno fmtfails (invalid TS output, missing formatter, etc.), the thrown error skips the stop call and can leave the esbuild service running. Since this is an early-exit/CI path, it is worth ensuring cleanup happens even on formatting failures.
// --check makes a stale committed bundle fail CI instead of relying on someone
// noticing it missing from a PR diff.
if (Deno.args.includes("--check")) {
const formatted = await formatTypeScript(output);
const committed = await Deno.readTextFile(outputPath).catch(() => null);
await esbuild.stop();
if (committed !== formatted) {
console.error(
`[prebundle-rsc-scripts] ${outputPath} is stale.\n` +
` The committed bundle does not match src/rendering/rsc/.\n` +
` Run \`deno task generate\` and commit the result.`,
);
Deno.exit(1);
}
console.log("[prebundle-rsc-scripts] Committed bundle is up to date");
Deno.exit(0);
}
Why
generate:manifests:checkalready runs insidetypecheck, which is a required check, so stale generated output fails CI today. But it only covered four of the six generators indeno task generate.prebundle-bridge.tsandprebundle-rsc-scripts.tshad no--checkcounterpart at all, and both write committed artifacts:src/studio/bridge/bridge-bundle.generated.tssrc/server/services/rsc/endpoints/rsc-bundles.generated.tsEdit the Studio bridge or the RSC endpoint sources, forget
deno task generate, and those bundles go stale with nothing failing.What changed
Both scripts now take
--check, using the pattern already inprebundle-hydration-runtime.ts, and both are wired intogenerate:manifests:check.One wrinkle: the RSC bundle is formatted after it is written, so its check formats the candidate through
deno fmtand compares post-format on both sides. Comparing against the raw template would report stale on every run.Verification
prebundle-bridge --check, clean treeprebundle-rsc-scripts --check, clean treedeno task generate:manifests:checkdeno task typecheck(the gate that invokes it)deno task lint/fmt:checkParity check: every generator invoked by
deno task generatenow has a--checkcounterpart, with none missing.Note
scripts/build/prebundle-*.tssit outside the repo's fmt/lint scope by config, sodeno fmt/deno lintreport no target files for them. The repo-levellintandfmt:checktasks both pass.Summary by CodeRabbit