ci: check the Worker configs against the code before deploying - #124
ci: check the Worker configs against the code before deploying#124nplusonedev wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
`wrangler.jsonc` names code that no TypeScript gate reads: the entrypoint, and every Durable Object class the entrypoint has to export. Rename a class without renaming its binding and `pnpm lint`, `pnpm typecheck` and `pnpm test` all stay green. Verified by renaming one `class_name`: typecheck reported 0 errors, the dry run exited 1 with "Your Worker depends on the following Durable Objects, which are not exported in your entrypoint file". Left alone, that first fails in the deploy jobs — after D1 migrations have been applied, and for the dispatcher after the substrate and canary have already shipped. `ci` gates every later job, so catching it here means nothing mutates. `--containers-rollout none` is load-bearing. A plain `--dry-run` still builds every container image (wrangler skips the push, not the build), which the deploy jobs then build again; with the flag each check is a ~2s bundle and the `ci` job needs no Docker. Scoped honestly: this does NOT catch a misspelled `cloudflare:*` import. `cloudflare:*` is an esbuild external, so any spelling bundles and exits 0. The vitest suite is what catches that, because its alias matches only the exact specifier.
8ba47ac to
9300096
Compare
|
Rebased onto current Re-validated the PR's claims on the rebased head:
Still valid, still worth merging as-is. |
There was a problem hiding this comment.
AI code review — 💬 Comment
Risk tier: full · 0 critical · 1 warnings · 0 suggestions
Reviewers: security
1. ⚠️ Warning — Dry-run duplicates worker bundling before deployment
📍 .github/workflows/deploy.yml:84-85
Both dry-run commands build each Worker bundle, and the subsequent deploy jobs will bundle them again. This adds a full duplicate build to every deployment CI run; consider validating the generated configuration or using a deploy flow that reuses the dry-run artifact if Wrangler supports it.
|
Validated the retriggered review's finding. Duplicate bundling — accurate observation, accepted as the cost of the gate. The dry runs do bundle both Workers, and the deploy jobs bundle them again. Measured on this branch: each dry run completes in a few seconds, unauthenticated, with no image builds ( |
The two dry runs cost seconds and pnpm test costs minutes, so ordered last a config that names missing code waited out the whole suite before saying so. After typecheck deliberately, so a real code error still fails with the typechecker's message rather than esbuild's.
wrangler.jsoncnames code that no TypeScript gate reads: the entrypoint, andevery Durable Object and Workflow class the entrypoint has to export. Rename a
class without renaming its binding and
pnpm lint,pnpm typecheckandpnpm testall stay green. The first failure is the deploy.The
cijob now bundles both Workers and stops there.What it catches, measured
Renamed one
class_nameinwrangler.jsoncand ran both gates:pnpm typecheckwrangler deploy --dry-runThe dry run's message:
Your Worker depends on the following Durable Objects, which are not exported in your entrypoint file: AgentBudgetTypo. Wrangler runsthe same validation for Workflow class exports, so the
runs-workflowbindingis covered too.
Left alone, that first fails in a job that has already mutated something. The
substratejob applies D1 migrations before it deploys, and thedeployjobapplies the dispatcher's migrations and runs only after the substrate and canary
have shipped.
cigates every later job, so catching it here means nothingmoved. Same argument the file already makes about job ordering: half a topology
is worse than none of it.
What it does not catch
Stated because the obvious guess is wrong. A misspelled
cloudflare:*import isnot caught. Changed the import to
cloudflare:workflowz: the dry run exited0 and the bad specifier appeared verbatim in the emitted bundle. Wrangler's
esbuild plugin externalizes on
filter: /^cloudflare:.*/, so any spelling afterthe colon passes.
The vitest suite is what catches that case, because an alias key matches the
exact specifier only and a typo falls through to Node resolution.
Also not caught, and this one is a deliberate trade: Dockerfile build failures.
See below.
Why
--containers-rollout noneWithout it a dry run still builds every container image. Wrangler passes
!dryRunas the push flag, not the build flag, so--dry-runskips only theupload. That meant the check rebuilt all 7 images on every push to main, images
the deploy jobs then build again on a runner with no layer cache.
With the flag: zero image builds, ~2s per check, and the
cijob needs noDocker.
The cost is honest. Wrangler skips the container build entirely when rollout is
none, so a broken Dockerfile, a bad base image, or aCOPYof a missing fileis not caught here and still surfaces in the deploy jobs. That is unchanged from
today —
mainhas no pre-deploy Dockerfile gate either — and paying 7 imagebuilds on every push to cover occasional
infra/**edits is the worse trade. Aconditional plain dry-run gated on
infra/**changes would close it later.Not a PR gate
deploy.ymltriggers on push tomainandworkflow_dispatchonly, so thecijob never runs on a pull request. This is a pre-deploy gate. PR-timecoverage would need its own
pull_requestworkflow, the waydocs.ymldoes it.Out of scope here.
Verification
Both commands run in a clean checkout against the pinned wrangler, no Cloudflare
credentials in the environment: exit 0, 2.1s and 1.6s, zero
Building imagelines,
Total Uploadstill reported.Boundary: "needs no Docker" was verified by observing that no build is invoked,
not by running on a host without Docker installed.
Split out of #123, where it was originally filed on a rationale that a negative
test disproved.