Skip to content

Add per-PR Worker Preview deploys - #231

Merged
Maximo-Guk merged 1 commit into
mainfrom
maximo/pr-previews
Aug 17, 2026
Merged

Add per-PR Worker Preview deploys#231
Maximo-Guk merged 1 commit into
mainfrom
maximo/pr-previews

Conversation

@Maximo-Guk

Copy link
Copy Markdown
Member

Every PR gets an isolated deployment of all workers as Worker Previews, reachable at the router preview's workers.dev URL which get commented on the PR.

scripts/preview/ generates a wrangler.staging.jsonc per package . The pull_request close event deletes the preview and a nightly sweep collects any strays.

@github-actions github-actions Bot added the delivery Changes to CI or release delivery label Aug 17, 2026
Comment thread scripts/preview/preview.ts Outdated
Comment thread scripts/preview/preview.ts Outdated
@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

Review submitted with 2 inline findings. Here is my summary.

Review summary

Solid, well-documented PR. The security model on preview.yml (trigger-based, not if:-based) is correct and clearly explained, the tiered deploy topology mirrors manifest-lib.mjs, and the pure config generator is well tested. Two inline findings were submitted (count: 2), plus the additional non-blocking items below.

Inline findings (2, submitted)

  1. sweep() aborts on any single GitHub API errorstaleReasons is awaited without a per-preview try/catch, and pullRequestState throws on any non-404 non-OK response. One transient 403/5xx defeats the whole nightly leak-prevention run.
  2. Suspicious /workers/workers/ API path — the sweep's list call uses a doubled segment where the documented path is /workers/scripts/{name}. Can't confirm against the private-beta previews API, so flagged for a maintainer to verify.

Additional (non-blocking)

  • CONTRIBUTING.md references a missing file. Both CONTRIBUTING.md (new lines) and the header comment in preview.yml point to .github/workflows/README.md, which does not exist in the tree. Either add that README or drop the references.
  • resolveAdmins() warns during delete/sweep. remove() and sweep() call generatePreviewConfigs(), which invokes resolveAdmins() and logs a "no deployment admins" warning whenever PREVIEW_ADMINS is unset — which it always is in the cleanup and sweep jobs (they don't pass it). Harmless but noisy/misleading in those logs.

Things I verified as correct

  • env-passthrough.test.js additions cover every new process.env.* read under scripts/preview/ (CLOUDFLARE_*, GITHUB_*, PREVIEW_*).
  • mapWithConcurrency / waitForAll error aggregation and the wrangler.cleanup() finally blocks are sound.
  • The patchPreviewServiceBindings unmatched-key guard correctly fails closed rather than silently binding to a baseline.
  • Node 24 native .ts execution + --test globbing lines up with NODE_VERSION: 24.19.0; scripts/tsconfig.json (erasableSyntaxOnly, nodenext, allowImportingTsExtensions) is consistent with that.

github run

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

Preview: maximo-pr-previews

https://maximo-pr-previews-os-public-router.gadgets-staging.workers.dev

Dashboard · deleted when this PR closes

@github-actions github-actions Bot added the kernel Changes to the Workshop kernel label Aug 17, 2026
@ask-bonk

This comment was marked as resolved.

@ask-bonk

This comment was marked as resolved.

@ask-bonk

This comment was marked as resolved.

@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

I've completed a thorough independent review of all 13 changed files. Given the guidance not to repeat published findings, and that the two material issues (sweep API path — author confirmed intentional; and the sweep-abort-on-error class) plus the missing README were already reported, I need to decide whether the deletion-loop instance is a genuinely new, distinct finding.

It is distinct in that the current code has already hardened the enumeration phase (the exact site of the prior inline finding) but left the deletion phase unhardened. That is an introduced defect at a specific changed line not covered by any still-applicable published finding. I'll submit one inline finding for it.

Given working_tree is read-only and I'm doing a code review, I'll report findings. Let me summarize my review as the final response (the top-level response is owned by the infrastructure; for a code review I list actionable findings). Since I have one actionable finding not previously posted, I'll note it.

I've reviewed all 13 files. Here is my assessment.

Review summary

I independently reviewed the full PR. The design is sound and exceptionally well-documented: the trigger-based security model in preview.yml (fork PRs structurally can't see the secret, if: guards as fail-closed defence-in-depth), the three-tier deploy topology mirroring manifest-lib.mjs, the fail-closed secret handling (uploaded via preview secret bulk stdin, never into a config Wrangler prints), and the pure, well-tested config generator all hold up.

Verified as correct:

  • env.d.ts widening ADMINS?: string[] | string matches the runtime that already JSON.parses a string binding.
  • env-passthrough.test.js's external list accounts for all 15 new process.env.* reads under scripts/preview/.
  • parseWranglerJson (preview.ts:247) now uses lastIndexOf("\n{"), which correctly handles pretty-printed multi-line JSON (an earlier review's slice concern was against older code); trailing text after the object still breaks it, but it's guarded and rides a draft Wrangler build — non-blocking.
  • pullRequestStatesByPreview (preview.ts:657) now has a per-page try/catch that degrades to age-only sweeping — this resolves the enumeration-phase abort that the earlier inline finding flagged.
  • patchPreviewServiceBindings fails closed on unmatched keys; assertNoPreviewUrl/assertRouterPreviewUrl enforce router-only public reachability.
  • vite.config.ts's scripts/**/*.test.ts override is ordered last so it wins over the vitest override, keeping node --test globals correct.

One actionable finding (not previously posted)

sweep() deletion loop aborts the whole run on one transient failure — preview.ts:760-765. The earlier reviews' inline finding targeted the enumeration phase, which the current code has since hardened with a try/catch. The deletion loop, however, is still unguarded: a non-ignorable deletePreview failure (a transient 5xx/403 on any one preview's router, backend, or gatekeeper delete) throws out of the for loop, so every remaining stale preview that night goes un-swept — the exact leak this job exists to prevent. Since the enumeration site is now fixed, this is the live, distinct instance. Wrap each preview's teardown in a per-preview try/catch (collect-and-continue, then throw if any failed) so one transient error doesn't defeat the rest.

Already reported by prior reviews (not repeated)

  • The doubled /workers/workers/ segment at preview.ts:733 — author confirmed intentional.
  • The missing .github/workflows/README.md referenced by CONTRIBUTING.md:23 and preview.yml — confirmed still absent from the tree.
  • The delibaretely typo at preview.yml:115.

No other new, distinct, introduced actionable defects.

github run

@Maximo-Guk
Maximo-Guk marked this pull request as ready for review August 17, 2026 02:17
@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

Dependencies aren't installed. Given working_tree is read-only, I won't install. The AGENTS.md confirms tsgo (typescript 7.0.2) is the workspace tsc and every package type-checks under it — types:scripts uses the same binary, so it's consistent.

I've completed a thorough, independent review of all 13 changed files. The prior review rounds (5 iterations of ask-bonk) have exhaustively covered every material finding, and the author has responded to them.

Review summary

I independently reviewed all 13 changed files. This is a well-designed, exceptionally well-documented PR. The security model is the load-bearing concern and it's handled correctly.

Verified as correct:

  • Security model (preview.yml): trigger-based (pull_request only — never pull_request_target/workflow_run/issue_comment), so fork PRs structurally can't see CLOUDFLARE_API_TOKEN. The if: maintainer/fork checks are fail-closed defence-in-depth, and the cleanup job's deliberate omission of the collaborator check (so a departed author's preview still tears down) is reasoned correctly.
  • Secret handling: backendSecrets (ADMINS + Access pair + AI gateway) never enter a generated config; uploaded via preview secret bulk stdin. Two tests (no generated config declares a secret's variable, ...carries a secret's value anywhere) guard this, including the bare-email form.
  • Three-tier topology mirrors manifest-lib.mjs; patchPreviewServiceBindings fails closed on unmatched keys (preview.ts:452); assertNoPreviewUrl/assertRouterPreviewUrl enforce router-only public reachability.
  • resolveTarget/resolveAccess correctly refuse to default and move as pairs; resolveAiGateway fails on half-configuration.
  • env.d.ts widening ADMINS?: string[] | string matches the runtime that already JSON.parses a string binding.
  • env-passthrough.test.js external list accounts for all 15 new process.env.* reads under scripts/preview/; VITE_CF_ACCESS_MODE is a write into the child build env (read in workshop-frontend), correctly not double-counted.
  • vite.config.ts's scripts/**/*.test.ts override is ordered last so it wins over the vitest override, keeping node --test globals correct.
  • parseWranglerJson (preview.ts:247) uses lastIndexOf("\n{"), correctly handling pretty-printed multi-line JSON.
  • pullRequestStatesByPreview (preview.ts:657) now has a per-page try/catch degrading to age-only sweeping — this resolves the enumeration-phase abort the earliest inline finding flagged.
  • scripts/tsconfig.json (erasableSyntaxOnly, nodenext, allowImportingTsExtensions, verbatimModuleSyntax) is consistent with native .ts execution under the pinned Node 24.19.0, wired into CI via types:scripts in the lint job.

Already reported by prior reviews (not repeated):

  1. sweep() deletion loop (preview.ts:760-765) aborts the whole nightly run on one non-ignorable deletePreview failure — one transient 5xx/403 leaves the remaining stale previews un-swept. This is the live instance of the abort-on-error class (the enumeration phase is now hardened; the deletion loop is not).
  2. The doubled /workers/workers/ segment at preview.ts:733 — author confirmed intentional.
  3. Missing .github/workflows/README.md referenced by CONTRIBUTING.md:23 and preview.yml — confirmed still absent from the tree.
  4. delibaretely typo at preview.yml:115.

I found no new, distinct, introduced actionable defects beyond what the prior reviews already reported. The one item I considered raising — the deploy/cleanup jobs sharing a concurrency group with cancel-in-progress: false, where a close-during-deploy race could leave a stray — is explicitly the case the nightly sweep exists to catch, so it's a designed-for edge rather than a defect.

LGTM!

github run

@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

I've reviewed the whole PR independently. Let me finalize my assessment. The prior reviews have extensively covered the material findings. Following the guidance to not repeat published findings, and to return LGTM! only if no actionable issues remain, I need to decide whether there is any new, distinct, introduced actionable defect.

Let me verify the one thing I want to be sure about — whether the deletion-loop fragility in sweep() (already reported by prior reviews) and whether there's anything genuinely new. Looking closely at the deletion loop at preview.ts:760-765: a deletePreview failure throws out of the for loop, un-sweeping remaining previews. This was already reported. The /workers/workers/ path — author confirmed intentional. The missing README — reported. The typo — reported.

The reviews were thorough and correct. I've independently verified all the "verified as correct" claims hold (the security model, secret handling, three-tier topology, env passthrough, the env.d.ts widening matching runtime, the vite config ordering). I found no new, distinct, introduced actionable defect beyond what prior reviews already reported.

LGTM!

github run

Base automatically changed from maximo/vp-task-env to main August 17, 2026 17:09
@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

github run

@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

@Maximo-Guk Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

github run

@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

@Maximo-Guk Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

1 similar comment
@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

@Maximo-Guk Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

@Maximo-Guk Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

Comment thread .github/workflows/preview.yml Outdated
Comment thread .github/workflows/preview.yml
Comment thread .github/workflows/preview.yml Outdated
@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

github run

1 similar comment
@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

github run

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

Preview: pr231-maximo-pr-previews

https://pr231-maximo-pr-previews-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

github run

@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

github run

@ndisidore

Copy link
Copy Markdown
Contributor

bonk break glass

@github-actions github-actions Bot added the bonk-break-glass Skip automatic Bonk PR review label Aug 17, 2026
@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

github run

1 similar comment
@ask-bonk

ask-bonk Bot commented Aug 17, 2026

Copy link
Copy Markdown

github run

@Maximo-Guk

Copy link
Copy Markdown
Member Author

Rebased

@Maximo-Guk

Copy link
Copy Markdown
Member Author

bonk break glass

Every PR gets an isolated deployment of all 18 workers as Worker Previews,
reachable at the router preview's workers.dev URL. `scripts/preview/`
generates a `wrangler.staging.jsonc` per package (gitignored build output),
deploys the three tiers in dependency order, patches each service binding at
the sibling preview, and comments the URL on the PR; the close event deletes
the preview and a nightly sweep collects strays.
@Maximo-Guk
Maximo-Guk merged commit 33c4ac7 into main Aug 17, 2026
13 checks passed
@Maximo-Guk
Maximo-Guk deleted the maximo/pr-previews branch August 17, 2026 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bonk-break-glass Skip automatic Bonk PR review delivery Changes to CI or release delivery kernel Changes to the Workshop kernel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants