ci: run the coverage gate in a parallel job instead of serially after validate - #2160
Conversation
`build` ran `npm run validate` and `npm run coverage` back to back, and on a recent green run those two steps were 84% of a 17m9s wall clock (7m41 + 6m50). They are serial for no reason: `coverage` consumes nothing `validate` produces. Every client's `test:coverage` is self-sufficient — web and cli build the test servers themselves (cli its own bin too), tui and launcher run from source — and the only consumers of `clients/*/build` are the smokes, which stay in `build` alongside the `validate` that produces them. So split `coverage` into a sibling job. Both publish jobs become `needs: [build, coverage]`: that is the safety property, not a side effect — left as `needs: build` a release could publish with the coverage gate red. Deliberately NOT backgrounding the two inside one job: that puts two vitest fleets on one 4-core runner, which already times tests out at the 5s default. Wall clock ~17m -> ~10m, for one extra runner and a duplicated ~24s install. Splitting `coverage` per client is the next lever and stays out of scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
Pull request overview
Splits the coverage gate from build to reduce CI wall time while preserving release gating.
Changes:
- Runs
validateandcoveragein parallel jobs. - Requires both jobs before npm or container publishing.
- Updates CI documentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.github/workflows/main.yml |
Adds the parallel coverage job and publish dependencies. |
AGENTS.md |
Documents the revised CI structure. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
@copilot review round 1 — no comments generated, so nothing to address. Noting for the record what the review did not need to catch, since the risk here is behavioral rather than in the diff text:
|
…tale The old name — "Run install, format, lint, build, and test on every push" — enumerated steps rather than naming the workflow, and was wrong in two directions: it never mentioned the coverage gate, and "on every push" reads past the two release-gated publish jobs. `CI` renders as `CI / build` and `CI / coverage` in the checks list, where the job name already carries the detail. Display-only: nothing references the name (no badge, no `workflow_run`, no sibling workflow), and branch protection keys off JOB names, which are unchanged. Also: README described the publish jobs as `needs: build`, which the previous commit falsified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
Verified on a real pushRun 33033968658 — this is the check the issue asked for before trusting the publish gating.
Identical start timestamps, so the two genuinely ran in parallel rather than queueing — and Wall clock 17m 9s → 9m 18s, a 46% cut — better than the ~10m the issue estimated, because Second commit: workflow renameRenamed the workflow Safe because nothing consumes it — no status badge, no
Also fixed a README line that my first commit falsified: it still described the publish jobs as @copilot re-requested for the rename commit, which wasn't in the first review. |
|
Rename commit's run (33034414631) is green too, and reproduces the split rather than just passing:
Same start second again, both ✅, publish jobs correctly skipped. Two independent runs now show the same shape, so the ~9-10m wall clock is the steady state rather than one lucky scheduling. Copilot has reviewed twice (before and after the rename) with no comments either time. Ready for a maintainer look. |
Closes #2159
What
buildrannpm run validateandnpm run coverageas two sequential steps in one job. On a recent green run that was 17m 9s wall clock and those two steps were 84% of it (7m41 + 6m50). This splitscoverageinto its own job so the two run in parallel.Why it's safe
coverageconsumes nothingvalidateproduces — each client'stest:coverageis self-sufficient:test:coveragevalidate's build?test-servers:build && vitest run --project=unit --project=integration --coveragetest-servers:build && npm run build && vitest run --coveragevitest run --coveragevitest run --coverageThe only consumers of
clients/*/buildare the smokes, which stay inbuildalongside thevalidatethat produces them.Re-confirmed the two test files the issue flagged, and neither reads compiled output:
clients/launcher/__tests__/parse-launcher-argv.test.ts—"/path/to/launcher/build/index.js"is an argv string fixture.clients/tui/__tests__/tsupConfig.test.ts— readspackage.jsonfiles andnode_modulesmanifests only.A repo-wide grep for tests referencing
clients/*/buildordistturns up only those two plusclients/cli/__tests__/e2e.test.ts, which resolves../build/index.js— and cli's owntest:coveragebuilds it first, so it is covered either way.Publish gating
Both publish jobs move from
needs: buildtoneeds: [build, coverage]. That is the whole safety property of the change, not a side effect: left as-is, a release could publish while the coverage gate is red.Not done, deliberately
Backgrounding the two commands inside one job would skip the second install, but it puts two vitest fleets on one 4-core runner — a configuration already known to time tests out at the 5s default. Separate jobs get separate runners and separate CPU.
Splitting
coverageper client (web is the bulk of the 6m50) is the next lever and stays out of scope, as the existing workflow comment says.Expected result
build≈ 24s install + 7m41 validate + ~2m gates/Playwright/smokes/Storybook ≈ ~10m 15scoverage≈ 24s install + 6m50 ≈ ~7m 15s, in parallel~17m → ~10m. Cost: one extra runner and a duplicated ~24s install.
Docs
Updated the
validatestep's comment (it explained why coverage was a separate step) and the two AGENTS.md passages describing the CI step order.npm run ci— the local pre-push gate — is unchanged and stays sequential; the invariant that passing it locally means CI's gates pass still holds, it is just no longer the same order.Verification
The diff touches only
.github/workflows/main.ymlandAGENTS.md— neither is an input to any format/lint/typecheck/test gate, sonpm run cihas nothing to say about it. The real verification is this PR's own CI run: it exercises the new two-job layout directly, which is what the issue asks for ("verify on a real push that the jobs are genuinely independent"). I'll confirm both jobs go green before merge.🤖 Generated with Claude Code
https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh