From be843d8cb5821ea4c07025aa5de4f6f3c56573d9 Mon Sep 17 00:00:00 2001 From: Eric J Date: Wed, 29 Jul 2026 23:36:26 -0700 Subject: [PATCH] ci(build): run pnpm vp build - verify never did (#61) `verify` is check + type-check + tests; none of them build. So a change that passed all three and broke the production build reached `main` unnoticed until somebody deployed - possibly days later, and landing on whoever was deploying rather than whoever broke it. A separate job rather than a phase inside `verify`: `deploy` already runs `pnpm build` straight after `pnpm run verify`, so folding it in would build twice on every deploy and slow the local gate people run by hand. Separate also means it runs in parallel with the tests. **The default shallow checkout is correct, measured rather than assumed.** #61 was filed believing the build stamp needed deeper history and wanted `fetch-depth` chosen carefully. It does not: `scripts/buildStamp.ts` runs exactly `rev-parse HEAD`, `rev-parse --short HEAD` and `status --porcelain`, none of which read history. "The build stamp reads git history" was imprecise - it reads git *state*. Recorded in the job, because the next person will assume the same thing. Noted in the job and not fixed: on a `pull_request` event checkout lands on the merge commit, so this job's stamp is a synthetic SHA existing nowhere in the repo. Harmless while CI never ships its artifact - `deploy:app` builds locally and uploads that. Zero-warning enforcement is deliberately NOT added. `vp build` has no `--fail-on-warn` (vp 0.2.6), grepping build output is the exact fragility that already produced a false negative here once, and an `onLog` hook that throws would hard-fail every local build the first time a dependency emits one benign warning. Warnings stay visible in the log and unenforced; the reasoning is in the job so it is not re-litigated from scratch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01F3a2HLJ4beKARi7SPmaM54 --- .github/workflows/verify.yml | 60 ++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index 363d0de1..35b944fc 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -87,3 +87,63 @@ jobs: - run: pnpm install --frozen-lockfile - run: pnpm run verify + + # `verify` is check + type-check + tests. None of them build, so a change that + # passes all three and breaks the production build reached `main` unnoticed + # until somebody deployed - possibly days later, and landing on whoever was + # deploying rather than whoever broke it (issue #61). This job closes that. + # + # Deliberately a SEPARATE job rather than an extra phase inside `verify`: + # `deploy` already runs `pnpm build` immediately after `pnpm run verify`, so + # folding the build into `verify` would build twice on every deploy and slow + # the local gate people actually run by hand. Separate also means it runs in + # parallel with the test job instead of after it. + build: + runs-on: ubuntu-latest + # The build is ~1s of Rolldown after ~30s of setup. 15 minutes is a hang + # detector, matching the job above. + timeout-minutes: 15 + steps: + # Default `fetch-depth: 1` is correct here, and that was MEASURED rather + # than assumed - #61 was filed believing the build needed deeper history. + # `scripts/buildStamp.ts` runs exactly three git commands: + # `rev-parse HEAD`, `rev-parse --short HEAD`, and `status --porcelain`. + # None of them read history, so a shallow checkout is enough and the + # earlier "the build stamp reads git history" was imprecise - it reads git + # *state*. + # + # Worth knowing rather than fixing: on a `pull_request` event checkout + # lands on the merge commit, so the stamp this job produces is a synthetic + # SHA that exists nowhere in the repo. Harmless, because CI never deploys + # its artifact - `deploy:app` builds locally and uploads that. If a CI + # build ever becomes the thing that ships, this needs revisiting. + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - uses: pnpm/action-setup@008330803749db0355799c700092d9a85fd074e9 # v6.0.9 + + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version-file: .node-version + cache: pnpm + + - run: pnpm install --frozen-lockfile + + # This catches a build that FAILS. It does not enforce "zero warnings", + # which CLAUDE.md leans on ("anything that does appear is new and worth + # reading"). Both ways of enforcing that were considered and rejected: + # + # - Grepping the build output for /warn/i. `vp build` has no + # `--fail-on-warn` (checked against vp 0.2.6 `--help`), and this repo + # has already been burned once by grepping build output - a version + # grep returned zero because the minifier had rewritten the string. + # A false pass here is worse than no check. + # - A `build.rollupOptions.onLog` hook that throws on `warn`. Robust, but + # it would hard-fail every local build too the moment a dependency + # emits one benign warning, and dependency-sourced warnings are exactly + # what this repo has seen (zlib-asm needed two suppressions before #46 + # removed it). The value is in reading a new warning, not in blocking on + # it. + # + # So warnings stay visible in this job's log and unenforced. If that ever + # needs to change, the `onLog` route is the sturdy one - not a grep. + - run: pnpm vp build