From ff41d70494ffe6b8231085a84fe3aaf9dab10419 Mon Sep 17 00:00:00 2001 From: sunny-wego Date: Tue, 22 Sep 2026 12:45:03 +0800 Subject: [PATCH 1/2] fix(typecheck): let the typecheck script generate the types it compares `src/api-types.d.ts` is generated from `contract/openapi.json` and is not committed. Checks A and C in `src/api-contract.ts` are compile-time comparisons against it, and when the file is absent they do not fail loudly - every published shape resolves to `never` and the compiler reports ~168 errors about code that is correct. Only `ci-cli` was protected, by a standalone step that ran `api-types:generate` ahead of the typecheck. The release lane typechecks with no such step, and a developer whose install skipped `postinstall` - a cache hit, `--ignore-scripts`, a `git clean -xfd` - gets the full wall of phantom errors and no hint that a generated file is missing. Move the generation into the `typecheck` script, so every caller inherits it: ci-cli, release-cli, `bun run check`, the husky pre-commit stanza and a plain terminal now compile against types regenerated from the vendored contract. The standalone ci-cli step is removed rather than kept - a second copy is a second thing to keep in step, and the lane that has one diverges from the lane that does not. Cost is 0.36s, on a check that was 0.32s warm and is now ~0.7s. `scripts/ci-contract-drift.test.ts` asserts the new shape: the script generates before it compiles, `&&` rather than `;`, and neither workflow carries a generate step of its own. Co-Authored-By: Claude Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MBuyVkpiDTnnPB7EbWw4ds --- .github/workflows/ci-cli.yml | 21 +++++----- .husky/pre-commit | 7 +++- package.json | 2 +- scripts/ci-contract-drift.test.ts | 66 ++++++++++++++++++++++++++----- 4 files changed, 75 insertions(+), 21 deletions(-) diff --git a/.github/workflows/ci-cli.yml b/.github/workflows/ci-cli.yml index c957bb4..03133f2 100644 --- a/.github/workflows/ci-cli.yml +++ b/.github/workflows/ci-cli.yml @@ -64,15 +64,18 @@ jobs: - name: Lint & format check (biome check) run: bun run lint - # Belt and braces over `postinstall`. `src/api-types.d.ts` is generated from - # `contract/openapi.json` and is not committed, so Checks A and C have - # nothing to compare against until it exists. `postinstall` already writes - # it during `bun install`; running it explicitly means a cache hit, a - # changed install flag or a future `--ignore-scripts` cannot quietly turn - # the two compile-time checks into a no-op. - - name: Generate API types from the vendored contract - run: bun run api-types:generate - + # No separate generate step. `bun run typecheck` regenerates + # `src/api-types.d.ts` itself, so this lane, the release lane and a + # developer's terminal all compile against the same freshly generated + # types - one source of truth, in package.json. + # + # This matters because the file is generated from `contract/openapi.json` + # and is not committed: without it, Checks A and C in `src/api-contract.ts` + # have nothing to compare against, and every comparison resolves to `never` + # rather than failing loudly. `postinstall` writes it during `bun install`, + # but a cache hit, a changed install flag or a future `--ignore-scripts` + # can skip that - which is a local-only mystery when the script does not + # own the generation. `scripts/ci-contract-drift.test.ts` asserts it does. - name: Typecheck run: bun run typecheck diff --git a/.husky/pre-commit b/.husky/pre-commit index 1911220..5a4699e 100644 --- a/.husky/pre-commit +++ b/.husky/pre-commit @@ -132,9 +132,14 @@ fi # --- the expensive one ----------------------------------------------------- -# ~0.32s warm, and the most common `ci-cli` failure after lint. tsc is whole-program, +# ~0.7s warm, and the most common `ci-cli` failure after lint. tsc is whole-program, # so there is no staged-only version of it: it runs when any TypeScript is staged. # +# `bun run typecheck` regenerates src/api-types.d.ts first (~0.36s of that 0.7s). +# That is deliberate and not worth trimming here: it is the single place the +# generation lives, shared with ci-cli and the release lane, and it is what makes +# the `contract` case below compile against the contract as just staged. +# # `contract` is in the condition for a case that stages no TypeScript at all. A # refresh commit is `contract/openapi.json` alone - CONTRIBUTING says to commit # that diff on its own, and the regenerated src/api-types.d.ts is gitignored, so diff --git a/package.json b/package.json index 580e970..c347cef 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "lint": "biome check", "lint:staged": "biome check --staged --no-errors-on-unmatched", "format": "biome check --write", - "typecheck": "bunx --package @typescript/native tsc --noEmit", + "typecheck": "bun run api-types:generate && bunx --package @typescript/native tsc --noEmit", "test": "bun --env-file=.env.local.example test ./src ./scripts", "check": "bun run lint && bun run typecheck && bun run test", "prepare": "husky" diff --git a/scripts/ci-contract-drift.test.ts b/scripts/ci-contract-drift.test.ts index 38babef..f061e6b 100644 --- a/scripts/ci-contract-drift.test.ts +++ b/scripts/ci-contract-drift.test.ts @@ -14,14 +14,20 @@ * carries a `servers` block naming its host, so a staging URL would rewrite * that line on every refresh and compare the committed contract against one * nothing ships against. - * - `src/api-types.d.ts` is generated, not committed. Checks A and C are - * compile-time comparisons against it; a committed copy is a second contract - * to keep in step, and a stale one silently checks the wrong shapes. + * - `src/api-types.d.ts` is generated, not committed, and the `typecheck` + * script is what generates it. Checks A and C are compile-time comparisons + * against it; a committed copy is a second contract to keep in step, and a + * stale one silently checks the wrong shapes. Move the generation back out + * into a workflow step and only the lane carrying that step is covered - + * the release lane and every developer's terminal compare against whatever + * `postinstall` last left behind, or against nothing at all. */ import { describe, expect, it } from "bun:test"; import { readFileSync } from "node:fs"; const CI = ".github/workflows/ci-cli.yml"; +/** The other lane that typechecks, and the reason generation belongs to the script. */ +const RELEASE = ".github/workflows/release-cli.yml"; const DRIFT_STEP = "Contract drift (warning only)"; @@ -130,14 +136,54 @@ describe("ci-cli: the contract drift step cannot veto a merge", () => { }); }); -describe("ci-cli: the generated types exist before anything compares them", () => { - it("generates them before the typecheck", () => { - const generate = steps.findIndex((step) => - step.run?.includes("api-types:generate"), +describe("the generated types exist before anything compares them", () => { + // The guarantee used to be a `ci-cli` step that ran `api-types:generate` + // ahead of the typecheck. That covered this one lane and nothing else: the + // release lane typechecks without it, and a developer whose install skipped + // `postinstall` got 168 errors about code that is fine, because Checks A and + // C resolve to `never` when the file is absent. The generation now lives in + // the `typecheck` script, so every caller inherits it. + it("are regenerated by the typecheck script itself", () => { + const typecheck = scripts.typecheck ?? ""; + expect(typecheck).toContain("bun run api-types:generate"); + // `&&`, not `;`: a failed generation must not be typechecked past. + expect(typecheck).not.toContain("; bunx"); + expect(typecheck.indexOf("api-types:generate")).toBeLessThan( + typecheck.indexOf("tsc"), ); - const typecheck = indexOfStep("Typecheck"); - expect(generate).toBeGreaterThan(-1); - expect(typecheck).toBeGreaterThan(generate); + }); + + it("are not regenerated by a second, drifting copy in ci-cli", () => { + // A standalone step here would be a second place to keep in step with + // package.json, and the lane that has one would silently diverge from the + // lane that does not. + const standalone = steps.filter( + (step) => + step.run?.includes("api-types:generate") && + !step.run?.includes("typecheck"), + ); + expect(standalone).toEqual([]); + expect(indexOfStep("Typecheck")).toBeGreaterThan(-1); + expect(stepNamed("Typecheck")?.run).toBe("bun run typecheck"); + }); + + it("cover the release lane too, which has no generate step of its own", () => { + const release = Bun.YAML.parse(readFileSync(RELEASE, "utf8")) as Workflow; + const releaseSteps = Object.values(release.jobs).flatMap( + (releaseJob) => releaseJob.steps ?? [], + ); + const typechecks = releaseSteps.filter((step) => + step.run?.includes("typecheck"), + ); + expect(typechecks.length).toBeGreaterThan(0); + for (const step of typechecks) expect(step.run).toBe("bun run typecheck"); + expect( + releaseSteps.filter( + (step) => + step.run?.includes("api-types:generate") && + !step.run?.includes("typecheck"), + ), + ).toEqual([]); }); it("runs the unit tests over ./src, where Check B lives", () => { From 98ffd625865bf67225f27f1e84bcaec204fa1635 Mon Sep 17 00:00:00 2001 From: sunny-wego Date: Tue, 22 Sep 2026 13:02:49 +0800 Subject: [PATCH 2/2] test(contract): assert the typecheck operator positively, not by blacklist The regression guard added alongside the shared typecheck script asserted `&&` by refusing the single string `; bunx`. A blacklist only refuses the separator its author thought of, so three worse spellings still passed - `;bunx` with no space, a bare `&` that backgrounds the generation and lets tsc race it, and a newline. So did `|| bunx`, which does not weaken the guarantee but inverts it: tsc would run only when generation had failed. Match the whole generate-to-bunx boundary with an anchored positive regex instead. A new separator now has to be written into the assertion to pass, rather than merely dodge it. Proved by discriminating input: each of the four bypasses is accepted by the old assertion and rejected by the new one, and flipping package.json to `||` turns this test red. Reported by CodeRabbit on #88. Co-Authored-By: Claude Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MBuyVkpiDTnnPB7EbWw4ds --- scripts/ci-contract-drift.test.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/scripts/ci-contract-drift.test.ts b/scripts/ci-contract-drift.test.ts index f061e6b..1f448a6 100644 --- a/scripts/ci-contract-drift.test.ts +++ b/scripts/ci-contract-drift.test.ts @@ -145,12 +145,17 @@ describe("the generated types exist before anything compares them", () => { // the `typecheck` script, so every caller inherits it. it("are regenerated by the typecheck script itself", () => { const typecheck = scripts.typecheck ?? ""; - expect(typecheck).toContain("bun run api-types:generate"); - // `&&`, not `;`: a failed generation must not be typechecked past. - expect(typecheck).not.toContain("; bunx"); - expect(typecheck.indexOf("api-types:generate")).toBeLessThan( - typecheck.indexOf("tsc"), + // Anchored and POSITIVE, matching the whole generate-to-bunx boundary, + // because a blacklist only refuses the separator someone already thought + // of. `; bunx` alone let three worse spellings through: `;bunx` (no + // space), a bare `&` (generation backgrounded, tsc racing it) and a + // newline - and `||`, which is the inversion of the guarantee, running + // tsc only when the generation FAILED. A new separator now has to be + // written into this line to pass, rather than merely dodge it. + expect(typecheck).toMatch( + /^\s*bun run api-types:generate\s+&&\s+bunx\b.*\btsc\b/, ); + expect(typecheck).toContain("--noEmit"); }); it("are not regenerated by a second, drifting copy in ci-cli", () => {