fix(gates): catch lint and type errors before push, not in CI - #1620
Conversation
Two open PRs burned full CI cycles this week on defects a single local command would have caught: #1606 on a react-hooks/set-state-in-effect lint error, #1618 on a TS2339 for `mode.devOnly` (a union member that lacks the property, where app-modes.ts already exports the correct `"devOnly" in mode` guard). Neither lint nor typecheck was in the pre-push path. Typecheck could not simply be added, because it was already unusable (outstanding-issues #210). tsconfig.json's `include` carries `.next/types/**/*.ts` and `.next/dev/types/**/*.ts` — gitignored build artifacts — so deleting a page leaves the stale generated validator importing a removed module. Reproduced rather than inferred: a planted `.next/dev/types/validator.ts` referencing a removed mockup page yields `error TS2307: Cannot find module .../mockups/deleted-mockup-route/page.js`, base config exit 2, source-only config exit 0. Full source typecheck is clean (71s cold, 8.8s warm). Red locally and green in CI is how the gate got abandoned, which is how the real type error then reached CI. - tsconfig.typecheck.json + `typecheck:source`: identical compiler options, minus the `.next` globs, with a separate tsbuildinfo so the two incremental caches cannot invalidate each other. Route-signature validation is not lost; `next build` still covers it in CI. - guard-push.mjs gains a fourth guard running eslint over the pushed files and this typecheck. Verified to reproduce both defects above with CI-identical messages. Scoped to the lint roots and to pushes that touch TS, skips loudly when node_modules is absent rather than pushing people to GUARD_PUSH_DISABLE=1, and overridable with SKIP_STATIC_GUARD=1. Also corrects a doc claim that made #1580 surprising: "mockups are exempt" was being read as blanket. Mockups are exempt from the wiring and reachability gates and nothing else — they are still typechecked, and their client chunks still count toward check:bundle-budget, which totals every built chunk rather than the initial production bundle. That the budget's scope contradicts ledger #13's "not an initial production bundle" position is a real unmade decision, now recorded as #237 rather than papered over. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T97Kqdj9Xh1Cubv5ms3KVy
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:5 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
Comment |
#1606#1606 is closed, but it carried the one fix nothing else in the queue provides: MobileResultFilterControl's native <select> paints a harsh system-blue highlight on phones, and #1615 keeps that native select (its change is the iOS 16px anti-zoom rule). So the fix does not survive #1615 landing. Records it as #238 with the two defects the redo must not repeat: the unresolved keyboard trap on disabled options, and the set-state-in-effect lint error that PR #1620's new pre-push guard would now catch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T97Kqdj9Xh1Cubv5ms3KVy
…ote-container browser gate drift
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…umber # Conflicts: # docs/outstanding-issues.md # docs/scripts-index.md
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Treat typecheck:source:internal as a shared read-only coordinator lease with a distinct per-worktree buildinfo file, drop the pinned in-repo cache path, and harden staticGuard: acquire a short exclusive lease (fail-open when busy), use a private eslint cache, escalate lint on eslint policy changes, fail closed when the push tip is not HEAD, cover eslint-rules, and add Vitest coverage. Align hook/docs wording with the fourth guard and point CLAUDE.md at #252. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Keep run-heavy routing (lint:changed:internal / typecheck:source:internal) from the parallel babysit tip, and retain shared source-typecheck lease, distinct per-worktree buildinfo, private eslint cache, eslint policy escalation, HEAD≠tip fail-closed, Vitest coverage, and docs alignment. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Pre-push invokes run-heavy via plain node, so the npm_execpath spawn path was skipped and the fallback dropped effectiveForwarded — undoing the per-worktree buildinfo injection. Also warn when staticGuard passes on a dirty working tree. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Babysit update (head after Bugbot)Addressed Devin review threads (all three resolved) and a Bugbot finding on this PR:
Ledger rows for this PR’s open follow-ups are Local proof: |
Keep docs:check-inventory green after adding the pre-push eslint wrapper script to package.json. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
- Treat "Database focused-test capacity is full" as coordinator busy so shared typecheck slot exhaustion fails open instead of faking a type error. - Skip source typecheck when every changed .ts path is excluded by tsconfig.typecheck.json (edge functions, archive, scratch, worktrees). - Restore check-github-shell-access.mjs (and its Role notes) in the scripts index.
Prefer exit 75 + DATABASE_HEAVY_RUN_ADMISSION_BUSY over prose matching so tsc/eslint output that quotes busy strings cannot false-pass the static guard.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…ache Addresses follow-up Devin on PR #1620: - Reorder staticGuard so tip-vs-HEAD fails closed only when lint/typecheck will actually read the working tree; ignore tag refs in the tip check. - Pin a distinct tsBuildInfoFile on tsconfig.typecheck.json so direct tsc does not collide with the base config cache (run-heavy still overrides).
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Addresses Devin on PR #1620 — a prior eslint failure must still block the push if the follow-up source typecheck cannot get a coordinator slot.
Summary
react-hooks/set-state-in-effectlint error, Phone Choose mode sheet: review + two large YES mockups #1618 onTS2339formode.devOnly(a union member that lacks the property —src/lib/app-modes.ts:400already exports the correct"devOnly" in modeguard). Neither lint nor typecheck was in the pre-push path.guard-push.mjsgains a fourth guard covering both.#210). Typecheck could not simply be added, because it was already unusable.tsconfig.json'sincludecarries.next/types/**/*.tsand.next/dev/types/**/*.ts— gitignored build artifacts — so deleting a page leaves the stale generated validator importing a removed module. Newtsconfig.typecheck.json+npm run typecheck:sourceanswer "is the source sound?" deterministically.check:bundle-budget.#237. The budget totals every built chunk, which contradicts ledger#013's "not an initial production bundle" position. Recorded rather than papered over; no metric was changed here.Root cause was reproduced, not inferred. A planted
.next/dev/types/validator.tsreferencing a removed mockup page yieldserror TS2307: Cannot find module '.../mockups/deleted-mockup-route/page.js'— base config exit 2, source-only config exit 0. Red locally and green in CI (fresh checkout, no.next) is how the gate got abandoned, which is how a real type error then reached CI.Verified against real build artifacts, not just the synthetic fixture: after
npm run build,.next/types/validator.tsexists and is absent from the source config's file list. The only.nextfile that still enters isroutes.d.ts, vianext-env.d.ts; it contains zero imports (a pure string-literal route union), so it cannot produce the missing-module error. Source-only typecheck also passes with no.nextpresent at all, so a fresh checkout is unaffected.The new guard is scoped to the lint roots and to pushes touching TypeScript, skips loudly when
node_modulesis absent rather than pushing people toGUARD_PUSH_DISABLE=1(which would lose the format and drift guards too), and is overridable withSKIP_STATIC_GUARD=1. Route-signature validation is not lost —next buildstill covers it in CI.Verification
npm run verify:pr-local— could not complete, and not because of this diff: it fails closed at itscheck:installed-lock-parityprecondition withplaywright: installed 1.62.0 does not match locked 1.62.1, a pre-existing container drift. This diff changes no dependency. The gates it selects for this scope were run directly instead:npm run lint— exit 0npm run typecheck— exit 0 (base config, as CI runs it)npm run test— 489 files passed, 5097 passed / 4 skippednpm run build— compiled successfully;Client bundle secret surface check passed.npm run check:bundle-budget—1406.9 KiB gzip; baseline 1406.4 KiB; within tolerancenpx prettier --check .—All matched files use Prettier code style!(repo-wide, not per-file)npm run check:outstanding-issues—235 rows (104 open, 131 archived), unique ids, next-id=238npm run docs:check-scripts—428 npm-run reference(s) resolve;docs:check-inventorycurrent afterdocs:updatenode scripts/guard-push.mjs --self-testpasses;tests/guard-push.test.ts15 passed; the guard was driven directly against reconstructions of both defects and blocked each with CI-identical messages (TS2339 ... Property 'devOnly' does not exist,error Calling setState synchronously within an effect).npm run verify:ui— not run: no UI, routing, styling, or browser behaviour changed.npm run verify:release— not run: no release or handoff confidence claimed.Risk and rollout
node_modulesis absent, andSKIP_STATIC_GUARD=1overrides it. The residual gap is deliberate: it reads the working tree rather than the pushed blobs (unlike the format guard, which materialises the commit because formatting drift between tree and commit is silent and common). A lint or type error present in the commit but not the tree implies editing after committing, which the guard surfaces with a dirty-tree note. Reproducing anode_modules-linked worktree fortsccosts more than that gap.tsconfig.typecheck.jsonandtypecheck:sourceare new and unreferenced by CI, and no existing gate's behaviour changed.Notes
#210is closed with its residual stated rather than silently: plainnpm run typecheckstill reads.nextand so still goes red against a stale build. That is correct behaviour for a stale artifact, and CI's Build job verifies the generated types for real — clear.nextif you want the full check locally.#237is recorded but deliberately not decided here. The budget'stotalGzipBytescomes frommeasureChunkPaths(walkJsFiles(CHUNKS_DIR))— every built chunk — while the manifest-scopedinitialDashboardChunksset is used only for the fixture-payload assertion. Choosing between "exclude mockup chunks so the number means production weight" and "keep counting them as a deliberate hygiene ceiling" changes what the gate is for, which is a call for the repo owner. Measured context: main sits at 1406.4 KiB against a 10% tolerance, and 59 of ~105 app routes are mockups.No
Clinical Governance Preflightsection: this diff touches build tooling and documentation only — no ingestion, answer generation, search/ranking, source rendering, document access, privacy, or production env. NoRAG impact:line for the same reason — no file under any RAG-ranking surface is touched.Generated by Claude Code