Investigation · CI & test strategy
Are the tests worth it, and where should each gate live?
You sent a voice memo asking six things about our test/CI/deploy machinery. This is the investigation you asked for — your questions as sections, answered with measured data where it exists, marked as opinion where it doesn't. Every number is tagged: 🟢 measured (run ID / API), 🟡 estimate (source named), 🔴 opinion.
"I'm just going to ramble. I hope you transcribe this, but I'm just curious — how valuable are these tests, because this has been a constant problem this week where tests are blocking and merges are blocking developer progress. We want high throughput, smooth systems… design, plan, implement, open PR, hopefully PR reviewed and merged as fast as possible — and a huge bottleneck has been the PR reviewer, but that's a pretty necessary system. Then the other is making sure we're not merging any issues. So we do need tests, but the question becomes where do we do all this? … Maybe merge to main but deploy in batches — every hour or two, and those batches are where the tests run… and we just always work on preview. … how do we reduce this gap? … the GitHub reviewer — do we need multi-turn or is one turn enough? … we have over 30,000 tests, so I don't want to create so many that it balloons and bloats our quality and friction. Should we give expiration dates to certain tests… remove tests once they expire… slim down tests, or test per impact? So if we change file B, only test file B and maybe adjacent siblings. Analyze the current infrastructure and propose a preview or artifact… use pretty HTML so I can follow along."
TL;DR — the six answers in one screen
| Your question | Short answer | Confidence |
|---|---|---|
| 1. Are the tests worth it? | Yes — but not equally. Round-1 review + the affected-subset suite earn their keep; the full suite on every event does not. | 🟢/🔴 |
| 2. Where should each gate live? | Already fixed tonight: cheap affected tests gate PR + main; the full suite is nightly + hub-escalation. Batching is a downgrade from what we now have. | 🟢 |
| 3. Is the reviewer earning its cost? | Round 1 yes (67% of PRs change after it). Round 2+ is a coin flip — make it opt-in. | 🟡 |
| 4. Test expiration / lifecycle | Don't expire by date. Expire by coverage + failure signal. Concrete mechanism below. | 🔴 |
| 5. Test-per-impact (file B → test B + siblings) | ~60% shipped tonight (the affected-graph model). The remaining 40% is sibling-adjacency + the deletion-safety story. | 🟢 |
| 6. A pretty HTML doc to follow along | You're reading it. | 🟢 |
Methodology & data sources
Every number below traces to one of: the GitHub billing API
(real spend export), the Actions runs API (per-job billed
minutes, ceil to the 1-min floor GitHub bills at), or a replay
(real merged trees re-run through the new selection logic). The measured
dataset — 145 push-run timings Jul 22–23, step-level before/afters, the
30-push replay split, per-actor claude.yml attribution, and the
review-family run counts — was gathered during the 2026-07-23 cost sprint.
🟢 measured = a run ID or the billing API backs it · 🟡 estimate = derived, source named · 🔴 opinion = a recommendation, not a measurement.
1 · Are the tests worth it?
The honest answer is that "the tests" is the wrong unit — the question is which tests, run how often. We have 🟡 ~30k tests (your number; the suite runs 12-way sharded). The cost was never the tests existing — it was running all of them on every merge.
🟢 Before tonight, the full 12-shard suite ran on
every push:main — 46 full runs on Jul 23, 51 on Jul 22
— at 57–59 billed job-min each (runs API, median 59, IDs
30052721572…30060253715). That's ~2,700 job-min/day on the test
lane alone, the single biggest line item behind July running 172% of the
$200 budget.
🔴 So: tests are worth it as a merge gate (they stop real regressions — see §3, where the bot + suite caught a prod-freezing deploy bug tonight). They are not worth running exhaustively on every event. The fix isn't fewer tests — it's running the relevant ones per event and the full set on a cadence. Which is exactly what shipped tonight.
2 · Where should each gate live? (and the batched-deploy question)
🟢 As of tonight the gates are placed like this:
| Event | What runs | Why |
|---|---|---|
| PR push | Affected-subset tests (capped) + review round 1 | Fast feedback; catches the author's own diff |
| Merge to main | Affected-subset again (its own diff) → deploy gate | Every merge tested exactly for what it changed |
| Hub-sized merge (>200 files) | Escalates to the full 12-shard suite | Wide blast radius still gets full coverage before deploy |
| Nightly | Full 12-shard suite | Cross-file backstop for anything the affected graph misses |
🟢 Result, measured on the first real runs: push:main billed cost 59 → 10 job-min (−83%), merge→prod promote 4.8 → 3.7 min, and the reviewer stays where it's cheap (the branch).
On "deploy in batches (hourly/2h/6h)"
🔴 I'd argue against it, and here's the trade with tonight's numbers as the baseline. Batching means: merge freely, hold deploys, run heavy tests at the batch boundary, ship on a timer. The case for it is that you run the expensive suite fewer times. But we just achieved that without batching — the affected-subset model already cut the per-merge cost 83%, and the full suite already only runs nightly + on hub-merges. Batching would buy little additional cost saving.
🟢 What batching would cost: latency. Tonight merge→prod is 3.7 min continuous. An hourly batch reintroduces up to 60 min of deploy staleness by construction; a 6h batch, up to 6h. It also re-creates the "which of the N merged PRs broke the batch?" untangling problem that the merge queue had (that pain is exactly why ADR-0107 retired the queue).
On "always work on preview / preview mirrors raw data with PII protection"
🔴 We're close but not there. Previews are opt-in
today (a [preview] commit builds one). "Always on preview with a
real-time data mirror" is a bigger build — it needs the anonymized-subset
pipeline (ADR-0097 already rewrites PII in-flight into a fake
+1206/@example.test convention for stage). Wiring
that into a per-branch live preview is a genuine project, not a config flip.
Worth scoping separately if you want it; flagging honestly that it's not
a tonight-sized change.
3 · Is the GitHub reviewer earning its cost? (multi-turn vs one turn)
🟡 Proxy method, stated as such: "a commit landed after review round N" can include unrelated pushes, so these are upper bounds on review-driven change. Sample: last 57 merged PRs with bot reviews, ex-Dependabot.
| Signal | Value | Read |
|---|---|---|
| PRs that changed after review round 1 | 67% | Round 1 clearly earns its keep |
| PRs that go multi-round (2+) | 44% | Nearly half |
| …of those, any code change after round 2 | 48% | A coin flip |
| Median review rounds / max | 1 / 7 | One PR ate seven rounds |
| Cost per extra round | ~5.6 min | Wall-clock, blocks merge |
🟢 Round 1 is not the thing to cut — tonight alone the bot caught a deploy-ordering race that would have frozen every prod promote, plus real bugs in the cost-sprint scripts. That's the reviewer paying for itself in a single night.
review check auto-passes when the delta
is small and touches only files round 1 already saw; anything bigger (new
files, >N lines, workflow/security paths) still demands full re-review, and
anyone can force one with @claude review. Trade: we accept small
unreviewed deltas on already-reviewed files to save ~6–12 min/multi-round PR.
This one needs your sign-off — it's a quality tradeoff, not a pure win.
To make this 🟢 instead of 🟡: trace whether post-round-2 commits actually touch the lines round 2 flagged. That's scriptable and it's the next cut.
4 · Test expiration & lifecycle
🔴 Don't expire tests by date — a calendar-old test can be the one guarding your highest-traffic path. Expire by signal. Concrete mechanism proposal:
- Tag, don't delete. Add a machine-readable pragma to a test's header — the reason it exists (regression for bug #N, drift guard, characterization). A test with no stated reason is the candidate, not a test that's simply old.
- Prune on two signals, both required: (a) zero unique coverage — every line it touches is covered by another test — and (b) it has never failed (never caught a real regression) across a long window. Either alone is not enough.
- Deletion-safety story is non-negotiable. "Slim the suite" without one is exactly how the #2923 / #2874 class of regressions came back (drift guards deleted, then the drift landed unnoticed). Any pruning PR must show the coverage-delta of what it removes.
🔴 Honest caution: pruning is a real project with a real downside, and the savings are modest (tests are cheap to hold; they're expensive to run exhaustively — which §2 already fixed). I'd rank this below shipping the round-2-opt-in and the sibling-adjacency work.
5 · Test-per-impact (change file B → test B + siblings)
🟢 This is ~60% shipped as of tonight. The
affected-graph model (vitest --changed) now gates both
the PR lane and the main lane — a change selects only the test files whose
import graph touches the diff. That's your "test file B" already live.
🔴 The open 40%:
- Sibling-adjacency selection — the "and maybe adjacent siblings" half. The import graph catches direct dependents; it doesn't catch a sibling that shares behavior without importing. Proposal: extend selection to the directory-sibling set for changed files, gated behind the same cap so it can't balloon.
- The escalation backstop stays. Whatever the selector misses, the nightly full suite + the hub-merge escalation catch — that's the safety net that makes aggressive selection safe. Don't remove it.
What already shipped tonight (so you can see the baseline)
| PR | Change | Measured effect |
|---|---|---|
| #4444 | Affected-subset on push:main + hub escalation | push cost 59→10 job-min (−83%) |
| #4450 | tsc→tsgo type-check | ~240s → 23s (−83%) |
| #4451 | Native-arm worker build (killed QEMU) | 18 min → 3.0 min build (−83%) |
| #4454 | Stale-run cancellation on all 16 PR workflows + drift guard | — |
🟢 End-to-end PR-open → live in prod, measured on a controlled canary through the full gate: 43 min median → 8.2 min (−81%). The final cost verdict is one complete billed day under the new model (Jul 24) against the $200/mo budget — pulled from the billing API when the day closes.
Codex pressure-test — my take and Codex's, side by side
Per Gera's ask, the whole design above was fed to OpenAI Codex (independent model, full context, explicitly told not to hand-wave agreement). Verbatim verdict: "The cost posture is sensible, but the system is not production-grade as written." Below is where we agree, where I concede, and where I push back — nothing softened.
| Topic | My design | Codex's take | Resolution |
|---|---|---|---|
| Promotion evidence | Each push tests its own event.before..sha range; tip-of-main guard prevents out-of-order promotes. |
🔴 Sharpest catch: the tip guard proves ordering, not completeness. If push A's run goes red, push B's green run (which tested only A..B) still promotes a deploy containing A's untested regression. Wants a per-artifact deployment cursor: test last-deployed-SHA..candidate, hold promotion if anything in that cumulative range lacks evidence. |
Concede — real gap, adopt. This is the strongest finding in the review. Promote should verify every push:main run since the last deployed SHA is green (or re-test the cumulative range), per deployable. |
| Hub escalation | >200 affected files → full 12-shard suite before deploy. | Threshold is a capacity control, not a risk model — a 5-line auth/config change is riskier than 500 generated files. Wants risk-class rules (lockfile, workflow YAML, config, env contracts, shared test setup → always full) + full-suite debt that persists to later candidates until a full run passes. | Concede both. Risk-class escalation rules are a small ci.yml diff; debt propagation folds into the deployment-cursor work above. |
| 24h nightly backstop | Accepted risk window; nightly red pages a human. | Not sane alone — but notes the ~12 hub-escalation full suites/day shrink the effective gap to ~2h on average. Wants: freshness rule (no green full suite in N hours of active deploying → schedule one), post-deploy smoke per artifact, promotion freeze on red, and push-workflow-absence alerting (the exact dark-deploy failure we already lived through tonight). | Adopt smoke + freeze + absence-alert; freshness rule over a fixed midday run. Reject universal auto-rollback — Codex itself flags worker/Lambda side effects + Temporal replay compatibility make blind rollback worse; freeze-then-human is right for those. |
| Auto-merge | Armed at PR-open for all PRs; PAT so merges fire push workflows (#4474). | Arm at ready, not open (green ≠ author intent). PAT = personnel blast radius → use a scoped GitHub App token. Re-recommends merge queue (merge_group) so evidence covers the actual merge candidate. Wants CODEOWNERS human review on security/CI/billing/migration surfaces. |
Concede arm-at-ready + App token + CODEOWNERS-on-sensitive-paths (all cheap). Push back on the merge queue: we measured it for a week — 66 trains for 63 PRs, it never batched, median time-to-merge 20+ min (ADR-0107). Codex is arguing from first principles without that history; the cumulative-cursor fix addresses the same correctness concern without resurrecting the queue. |
| Reviewer round-2 opt-in | Round 1 mandatory; round 2+ opt-in (48% of round-2s show no follow-up commit). | 🔴 False economy: 48% is not low, and "any later commit" is a weak value metric. Wants delta-scoped incremental round 2 (auto-skip only formatting/docs/generated), full re-review forced on new files / deps / CI / auth / migrations, and explicitly: don't classify safety by line count — small post-review fixes are where subtle mistakes live. | Concede. Tonight is its own receipt: the bot's round-2 on #4444 caught two real bugs in my round-1 fix, one of which would have frozen every prod deploy. Withdraw the blanket opt-in; propose delta-scoped incremental re-review instead. |
| Test pruning | No date expiry; prune by coverage-overlap + failure-history signal. | Coverage+history is only candidate generation — coverage proves execution, not equivalent assertions. Concrete pipeline: classify by criticality (incident-derived tests tagged), candidate-local mutation testing (retained tests must kill the same mutants), prefer consolidation over deletion, deletion PR carries the evidence, revert-to-restore. Explicit non-reasons: "never failed", "old", "slow", "flaky". | Adopt wholesale — it's my §4 direction made concrete, and candidate-local mutation testing is the missing safety mechanism. Codex agrees a tombstone database would be over-engineered at our size. |
| Sibling-adjacency (memo Q5) | Extend selection to directory siblings, capped. | Weak heuristic — runs irrelevant tests while still missing remote semantic consumers (Temporal contracts, SQS payloads, generated schemas). Only adopt if shadow data proves recall improves. | Concede — demote. Replace with selector-correctness telemetry first (shadow-run unselected tests on a sample, track false-negative recall); only ship adjacency if the data says it earns its cost. |
Gaps Codex named that the design missed entirely
- 🔴 Affected-graph blind spots as a class: dynamic imports, Next.js root layouts/middleware, config/env/lockfile changes, fixtures/snapshots/prompts opened by pathname, generated clients, deletions/renames, and cross-artifact semantic edges (Temporal signals, SQS payloads, webhook contracts) that no import graph sees. Fix: explicit broad-impact rules + never silently cap the affected set (cap-crossing must escalate, not truncate).
- Flaky-test policy: record first-attempt failures even when retry passes; quarantine lane with owner + issue + expiry; never quarantine critical smoke/contract tests indefinitely.
- Shard integrity: assert the 12-shard manifest is complete + disjoint — a test silently dropped from discovery is invisible today.
- Immutable artifact promotion: deploy the exact tested build digest; don't rebuild post-test and call it equivalent.
- Cache trust boundaries: untrusted PR branches must not write caches trusted main builds restore; include selector version + shard manifest in cache keys.
- LLM-reviewer as single point of failure: audited break-glass path with human approval; treat repo prompt-injection as a real threat (the bot holds no secrets or mutation authority — true today, keep it pinned).
The production-ready sequence (revised for the review)
- Now (correctness, small diffs): deployment cursor + full-suite debt in
vercel-promote/deploy gates · risk-class escalation rules (lockfile/workflow/config → full) · push-workflow-absence alert · arm-at-ready · App token replaces the PAT. - Next (safety net): per-artifact post-deploy smoke + promotion freeze · full-suite freshness rule · shard-integrity assertion · flake quarantine lane.
- Then (efficiency, data-gated): selector-correctness telemetry → sibling-adjacency only if recall data supports it · consolidation-based test pruning with candidate-local mutation testing · delta-scoped incremental round-2 review (replaces the opt-in proposal).
The three decisions this leaves you
- Batched deploys? My read: no, as a cost lever (§2). Yes, only if you want a human release gate — decide that in the deploy-model doc.
- Round-2+ reviews: the blanket opt-in proposal is withdrawn after the Codex review (and after round-2 caught two real bugs in my own #4444 fix tonight). The replacement on the table: delta-scoped incremental re-review — auto-skip only formatting/docs/generated, full re-review forced on new files / deps / CI / auth / migrations.
- Sibling-adjacency + test pruning? Both buildable, both ranked below the two above because tonight's work already captured most of the cost. Say the word and I'll scope them as their own PRs.