Skip to content

ci: gate speed — measured, plus the zero-tracking gate #271 was missing - #305

Merged
important-new merged 10 commits into
InspectorHub:mainfrom
important-new:ci/gate-speed
Aug 8, 2026
Merged

ci: gate speed — measured, plus the zero-tracking gate #271 was missing#305
important-new merged 10 commits into
InspectorHub:mainfrom
important-new:ci/gate-speed

Conversation

@important-new

Copy link
Copy Markdown
Contributor

Ten commits. Every change was measured before shipping, and two of them
contradicted the prediction that motivated them.

Closes the last open task on #271

#271 Task 5 — make the zero-client-tracking posture enforceable. The
report-view counter is defensible precisely because the browser reports nothing;
that posture is why the LIA holds, and it lived only in a document.

The plan's own pre-dispatch audit said the drafted rule set was miscalibrated,
and it was right: as written it flagged four legitimate things (a theme cookie, a
visibilitychange revalidate, an IntersectionObserver, an offscreen Image used
to decode a crop source). A gate that cries wolf on ordinary UI code gets
switched off, so the rules target only the mechanisms by which a browser SENDS:
navigator.sendBeacon, analytics globals, and a pixel whose interpolated src
nothing ever loads or decodes.

Expectation derived by running it rather than copying the audit's list — that
list had already drifted. 976 client files, zero findings. Proven red once per
rule before wiring in.

Speed, measured

before after
eslint cold 442s warm, 1 file 6s
test:unit shard 1/4 (422s whole) 148s
test:web shard 1/4 (159-181s whole) 55s
test:types typecheck 237.7s 6.9s
e2e 357s ~264s

Projected wall clock ~7.5min → ~5min, with two fewer runners.

test:types recompiled the entire server to check four files — its tsconfig
extends the api config AND re-lists server/**/*. That was an oversight from
the project-references work: the app config was converted to consume the emitted
.d.ts and this one was only stopped from fighting over the output directory.

type-check (api) was a second runner for the same answer. Once app and api
became project references, type-check:app IS tsc -b tsconfig.json, which
builds api and reports its errors. Verified before deleting: a type error planted
in server/api/admin.ts, which the app program does not import, is reported with
exit 2.

Costs, stated

  • Total runner time goes UP. Sharding takes npm ci from 10 runners to 16;
    splitting e2e adds a second browser install and a second worker build.
  • The e2e estimate was wrong the first time. The second Playwright config had
    been reusing the first's worker; standalone it must boot one, so 112s becomes
    192s and the split is −26%, not −33%.

Checked rather than assumed

  • ESLint's default cache strategy is metadata and a fresh checkout renews every
    mtime — the widely reported "the cache does nothing on Actions". This repo
    already passes --cache-strategy content; that flag is load-bearing and the CI
    comment now says so.
  • A matrix job aggregates into ONE needs entry, so verify still catches a
    single red shard. verify is the only required check, so the renamed jobs do
    not strand branch protection.
  • ⚠️ upload-artifact@v4 FAILS on a duplicate artifact name, so the e2e report
    name is keyed on the matrix id.
  • Playwright re-runs a project's dependencies in EVERY shard
    ([BUG] Setup file runs for each shard on Github microsoft/playwright#21974) — that is why e2e is split, not sharded.
  • The seeded e2e config spreads ...base, so it can boot its own worker. Proven
    by running it standalone against a wiped .wrangler/state: 9 passed, exit 0.
  • A ref-deletion push no longer pays for a full build: the hook's fail-safe could
    not tell "this push deletes refs" from "I cannot tell what this push contains".

.tsbuildinfo deliberately NOT cached: tsc --build is documented to miss
breaking type changes from a dependency update when a build-info exists and to
report success with type errors present (microsoft/typescript-go#2666).

Full suite green locally: lint (28 gates), test:unit 723/5236, test:web 335/2232.

⚠️ None of the CI wiring is verified until this PR runs it. Every number above is
component-level and local.

🤖 Generated with Claude Code

important-new and others added 10 commits August 8, 2026 11:14
Deleting four merged branches ran four full worker builds and four network
audits, and timed out. The loop `continue`s past a deletion, which leaves
CHANGED empty, and the skip condition requires a NON-empty CHANGED — so the
fail-safe read "no evidence" and ran everything. **A deletion is positive
evidence that nothing ships, not an absence of evidence**, and that is the
distinction the condition was missing.

stdin is now slurped once at the top, because two steps need it and it can only
be read once. An empty stdin (someone running the hook by hand) still falls
through and runs the full check — that fail-safe is unchanged.

Verified all five inputs rather than the new one alone: deletion-only skips in
0s, two deletions skip, a mixed push runs the checks, an empty stdin runs
everything, and a genuinely bundle-affecting range still builds.

Also adds a .gitattributes this repo never had. A CRLF shebang makes the kernel
look for an interpreter named `sh\r`, so a hook committed from Windows dies with
"bad interpreter" on Linux while working here. The stored blobs are LF today
only because of how this machine happens to be configured; this states the
requirement instead of relying on it. CMS already did this; portal now does too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
…very run

Measured on this repo: a cold eslint is 442s, and a warm one with a single file
touched is 6s. The type-aware program is built lazily for the files actually
linted, so a cache hit skips that too — which refutes the guess that the program
build would dominate and make a cache pointless. CI's lint job is 5m39s.

The classic reason this does not work on GitHub Actions is that a fresh checkout
gives every file a new mtime, and ESLint's DEFAULT cache strategy is metadata, so
every file misses. `lint:eslint` already passes `--cache-strategy content`; that
flag is what makes persisting the cache worth anything, and this comment says so
next to the cache in case someone removes it later.

Keyed on the config and the lockfile, so a rule change or a plugin bump starts a
fresh cache rather than reusing verdicts computed under the old rules. The
restore-keys prefix CONTAINS that same hash, so a fallback can only come from a
run with identical rules — it over-invalidates, never under-invalidates, which
is the direction the two existing caches in this file already argue for.

Deliberately NOT extended to .tsbuildinfo / .types, despite that being the other
obvious cold cost. `tsc --build` is documented to miss breaking type changes from
a dependency update when a build-info already exists and to report success with
type errors present (microsoft/typescript-go#2666), and its up-to-date check is
mtime-based, which a fresh checkout defeats anyway (microsoft/TypeScript#59152).
"Reports success despite errors" is the exact failure this project keeps closing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
…retries it

The api suite is import-bound and passes only because every spec file gets its
own process; `--no-isolate` is 193.8s against 422.7s, with `import` down 88%.
The blocker is 1218 failures, dominated by state carried across file boundaries.

`clearMocks: true` was the obvious first step and was measured, not assumed:

  isolated suite            723 / 5236 green   ->  723 / 5236 green (unchanged)
  --no-isolate failures     1218               ->  1235
  this.client.prepare ...   20                 ->  213

It did not reduce the leakage and made one cluster ten times worse, so it is
reverted and the config now says why. Three working assumptions were disproved
alongside it: toRawD1 is a plain object literal and not mock-based, no spec
creates its database at module scope (0 top-level, 366 in beforeEach), and there
is no memoised drizzle instance in the production DB layer. Those rule places
out — the location of the sharing is still not established, which is why the
plan's first task is to find it rather than to fix anything.

Plan with acceptance criteria: docs/superpowers/plans/2026-08-08-test-isolation-debt.md
(superproject). 1235 is the meter; a change that does not move it down is not a
fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
`npm run test:unit:isolation-debt` runs the api suite without process isolation.
It is expected to FAIL — that is the point. The number of failures is the meter
for the work in docs/superpowers/plans/2026-08-08-test-isolation-debt.md, and a
change that does not move it down is not a fix.

Baseline today: 1235 failures, against a suite that is 723/5236 green when each
file gets its own process.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
This was the longest job in the workflow at 7m22s. The suite is import-bound —
723 spec files each rebuild the module graph in their own process, so `import`
dominates `tests`. Sharding buys the wall clock back with zero test changes and
no new failure mode: each shard is an ordinary, fully isolated run of a subset.

Measured locally: --shard=1/4 is 181 files / 1448 tests in 148s, against 422s for
the whole suite.

Three things checked rather than assumed before shipping this:
  - GitHub aggregates a matrix job into ONE entry in `needs`, so verify's
    contains(needs.*.result, 'failure') still catches a single red shard.
  - The matrix renames the check to "API unit tests (shard N/4)". Upstream's
    required status check is `verify` and nothing else, so branch protection is
    unaffected — had a per-job name been required, every PR would have blocked
    forever waiting for a check that no longer exists.
  - fail-fast: false, so one red shard does not hide the other three.

The other way to attack the same term — relaxing vitest's per-file process
isolation — was investigated and REJECTED rather than skipped. 348 specs mock
drizzle-orm/d1 at module scope and share one vi.fn() once the registry is shared,
their afterEach policies disagree (10 restore, 38 clear, 300 nothing), vitest
offers no per-file isolation override, and every single-lever probe redistributed
the failures instead of removing them. Full record and the external evidence:
docs/superpowers/plans/2026-08-08-test-isolation-debt.md in the superproject.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
Measured before shipping, like the api suite: `--shard=1/4` is 84 files / 586
tests in 55s against 159-181s for all 335. CI's web job was 4m7s.

The trade is stated in the workflow rather than buried: transform (40s) and setup
(26s) are per-shard fixed costs, so four shards spend roughly 40% MORE total CPU
to take ~65% off the wall clock. Right for a gate people wait on; wrong if these
minutes are ever billed by the second.

e2e is deliberately left alone. Its 5m57s breaks down as 72s of fixed cost
(npm ci 45s, browser install 18s) and two sequential Playwright invocations of
166s and 112s — and the 166s one includes `npm run build && wrangler dev` inside
the webServer command, a build the standalone build job spends 1m45s on. Sharding
multiplies all of that per runner. How much of the 166s is build versus tests is
the number that would decide it, and I do not have it, so no change was made.

⚠️ The cheaper shape is visible in that breakdown and is NOT sharding: the job
already runs two independent Playwright invocations back to back, so putting them
on two runners pays the fixed cost twice instead of four times — roughly 238s
against 357s. Recorded in the plan, not done here, because it too should be
measured first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
…ate work

Since the two programs became TypeScript project references, `type-check:app` IS
`tsc -b tsconfig.json`, which builds tsconfig.api.json first and reports its
errors. A separate job re-compiled the same project for the same answer, at
3m44s plus its own npm ci.

Verified rather than reasoned, because removing a gate deserves proof: a type
error planted in `server/api/admin.ts` — a file the app program does not import —
is reported by `type-check:app` as `server/api/admin.ts(53,7): error TS2322`,
exit 2. Everything tsconfig.api.json includes, including tests/e2e and
tests/helpers, is built by that reference.

`npm run type-check:api` still exists and is still the right local loop for
server-only work; pre-commit's api tier uses it. What was removed is the
duplicate CI runner, not the check.

The header comment above the section said the two were "independent tsc
programs" — true when it was written, false since the split, so it is corrected
here rather than left to mislead the next reader.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
`test:types` checks exactly four `*.spec-d.ts` assertion files, and took 4m36s in
CI (238s of typecheck locally) because its tsconfig `extends` tsconfig.api.json
AND re-lists `server/**/*` — recompiling the entire server program to reach four
files.

That was my own oversight when project references landed: the app config was
converted to consume the emitted `.d.ts`, and this config was only stopped from
fighting over the output directory. It now REFERENCES the api project like the
app one does. Typecheck time: 237.7s -> 6.9s.

⚠️ Two things that had to be right, and one I got wrong first:
  - `references` is NOT inherited through `extends`, and `tsc -p` does not build
    referenced projects — it expects the outputs and fails TS6305 without them.
    `test:types` therefore runs `tsc -b tsconfig.api.json` first.
  - That build must inherit the heap bump. My first version put it BEFORE
    cross-env, so it ran at the default ~4GB and died with the same OOM
    signature this whole line of work exists to fix. Both halves are wrapped now.

The job is merged into type-check (app) rather than kept separate, because that
job's `tsc -b` has already built the api project one line above — so the check
costs ~6s there instead of a fresh runner spending 4m36s rebuilding it. The
check still runs; only its runner is gone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
They were two steps on one runner: 72s of shared setup, then 166s and 112s back
to back = 357s. Measured breakdown of the first step: 38s build + boot, 21s
globalSetup seed, 104s actually running 205 tests. The second ran 9 tests in 10s
behind its own globalSetup.

Verified before splitting, because the second config had no server of its own to
lose: playwright.seeded.config.ts spreads `...base`, so it inherits the webServer
block and can boot one. Ran it standalone against a wiped .wrangler/state to
prove it does not depend on the first leg having set up the tenant — 9 passed,
exit 0.

⚠️ Corrected estimate. Sharing a runner, the second config REUSED the first's
worker; standalone it must build and boot one, and locally that takes it from
112s to 192s. So the second leg becomes the bottleneck: ~264s against 357s,
about 26% off — not the 33% a naive split of the two step times suggests. Total
runner time goes up by a second npm ci, a second browser install and a second
build. Right trade for a gate people wait on.

Two things checked rather than assumed:
  - upload-artifact@v4 FAILS on a duplicate artifact name rather than merging,
    so the report name is keyed on the matrix id. Without that, the second leg's
    failure upload would kill the job that was already failing and lose the
    report you actually wanted.
  - Upstream's only required status check is `verify`, so renaming the job to
    `e2e (…)` does not strand branch protection.

NOT sharded further. Playwright re-runs a project's dependencies in EVERY shard
(microsoft/playwright#21974), and playwright.config.ts records that all projects
share one wrangler-dev worker and one D1 seeded once — three separate causes had
to be fixed to reach 3 workers, the third found only because a run failed. More
shards would multiply setup that is already most of the second leg.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
…pectorHub#271 Task 5)

The report-view counter is defensible because the browser reports nothing — the
three numbers are recorded server-side. That posture is why the LIA holds, and
until now it lived only in docs/compliance/report-view-lia.md. A rule that lives
only in a document is a rule until someone does not read the document.

The plan's own pre-dispatch audit said the drafted rule set was miscalibrated,
and it was right: as written it flagged four legitimate things — a theme cookie,
a visibilitychange revalidate, an IntersectionObserver for infinite scroll, and
an offscreen Image used to decode a crop source. None of those report anything.
A gate that cries wolf on ordinary UI code gets switched off, so the rules here
target only the mechanisms by which a browser SENDS something:

  1. navigator.sendBeacon — one purpose, no innocent use.
  2. Analytics globals (gtag/dataLayer/_paq/posthog/mixpanel/amplitude/zaraz) —
     their presence in client code IS the vendor.
  3. A pixel: new Image() or <img> with an interpolated src that nothing ever
     loads or decodes. An image nobody looks at is a request, not a picture.

Cookies, IntersectionObserver, visibilitychange and same-origin fetch are NOT
flagged, deliberately.

Expectation derived by RUNNING it, as the audit demanded rather than by copying
its list — which had already drifted (it cites a `new Image()` in cropImage.ts
that greps to nothing today). Result: 976 client files, zero findings.

Proven to go red, once per rule, before being wired in: a planted sendBeacon, a
planted gtag call, and a planted interpolated-src pixel are each reported with
file, line, rule and reason; removing the probe returns exit 0.

⚠️ It fails when it cannot see. Scanning fewer than 50 files is a FAILURE rather
than a pass — "found nothing" and "looked at nothing" produce the same empty
result, and this repo has shipped that mistake before.

Registered in both `lint` and the hand-duplicated `lint:gates-full`, and in
run-gates' pre-commit set for the same reason the price gate is there: what it
catches is a capability ARRIVING, and "we ship no tracking" is far easier to
hold than "please remove the tracking you added". ~0.8s, the most expensive
entry in that set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
@important-new
important-new merged commit dbfa85f into InspectorHub:main Aug 8, 2026
19 checks passed
@important-new
important-new deleted the ci/gate-speed branch August 8, 2026 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant