From f090f934ad3caa53bea80e70a6f9d14beea988bd Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sat, 22 Aug 2026 18:34:23 +1000 Subject: [PATCH 01/16] test(build): type-check test/build as a 5th tsconfig project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test/build/ sat in no tsconfig: the root config excludes "test" wholesale and test/extension/tsconfig.unit.json includes only the protocol drift guard, so the whole directory was transpile-only under vitest. Every type-level assertion there was permanently vacuous — proof already on disk in the shape of a @ts-expect-error that could never fire. Add test/build/tsconfig.json (Node + ESM compiler view, no DOM lib) and append it to the compile chain. Clearing the four pre-existing errors it surfaces: - theme-palettes.test.ts had its @ts-expect-error above a Biome-wrapped multi-line import. TS7016 is reported at the module specifier, so the directive covered the wrong line and produced both an unused-directive error and the live error it meant to suppress. A namespace import keeps the specifier on the directive's line no matter how many bindings the file destructures. - notice-covers-bundled-deps.test.ts was missing the directive entirely on its untyped esbuild.config.mjs import, and Object.values() over the resulting any widens each entry to unknown — annotated at the boundary with the esbuild.BuildOptions[] shape the suite actually requires. Non-vacuity measured by deleting each new pin and watching the new project go red. Five suites in the directory still carry a file-level @ts-nocheck for their untyped .mjs import, which switches the whole file off; swapping those to the line-scoped directive costs 31 annotations and is tracked separately to keep this change to one purpose. --- package.json | 2 +- test/build/notice-covers-bundled-deps.test.ts | 7 +++- test/build/theme-palettes.test.ts | 13 ++++---- test/build/tsconfig.json | 33 +++++++++++++++++++ 4 files changed, 47 insertions(+), 8 deletions(-) create mode 100644 test/build/tsconfig.json diff --git a/package.json b/package.json index d46f6774..b9ac8b0f 100644 --- a/package.json +++ b/package.json @@ -305,7 +305,7 @@ ] }, "scripts": { - "compile": "tsc -p ./ && tsc -p ./test/extension/tsconfig.unit.json && tsc -p ./test/webview/tsconfig.json && tsc -p ./test/webview-browser/tsconfig.json", + "compile": "tsc -p ./ && tsc -p ./test/extension/tsconfig.unit.json && tsc -p ./test/webview/tsconfig.json && tsc -p ./test/webview-browser/tsconfig.json && tsc -p ./test/build/tsconfig.json", "compile:webview": "tsc -p ./src/webview", "build": "pnpm compile && pnpm compile:webview && node esbuild.config.mjs --production", "watch": "node esbuild.config.mjs --watch", diff --git a/test/build/notice-covers-bundled-deps.test.ts b/test/build/notice-covers-bundled-deps.test.ts index 7704e9ba..d83b2116 100644 --- a/test/build/notice-covers-bundled-deps.test.ts +++ b/test/build/notice-covers-bundled-deps.test.ts @@ -12,6 +12,8 @@ import { dirname, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import esbuild from "esbuild"; import { beforeAll, describe, expect, it } from "vitest"; +// @ts-expect-error — esbuild.config.mjs is plain JS with no .d.ts; the runtime +// shape (a factory returning esbuild BuildOptions) is exercised below. import { createBuildConfigs } from "../../esbuild.config.mjs"; const root = resolve(dirname(fileURLToPath(import.meta.url)), "../.."); @@ -60,7 +62,10 @@ describe("NOTICE covers every bundled third-party package", () => { // Every value is a config emitting into the packaged dist/ (host, webview, // test-harness). Union them all so a dependency bundled into ANY shipped // output must be attributed — not just host + webview. - const configs = Object.values(createBuildConfigs({ production: true })); + // Annotated because createBuildConfigs comes from the untyped .mjs above: + // Object.values() over an `any` widens each entry to `unknown`, and this is + // the shape the rest of the suite (and esbuild.build) actually requires. + const configs: esbuild.BuildOptions[] = Object.values(createBuildConfigs({ production: true })); const sets = await Promise.all(configs.map(shippedPackages)); bundled = [...new Set(sets.flatMap((s) => [...s]))].sort(); notice = readFileSync(resolve(root, "NOTICE"), "utf8"); diff --git a/test/build/theme-palettes.test.ts b/test/build/theme-palettes.test.ts index dc8cd2cc..3c9d2e0b 100644 --- a/test/build/theme-palettes.test.ts +++ b/test/build/theme-palettes.test.ts @@ -22,15 +22,16 @@ import { readFileSync } from "node:fs"; import { describe, expect, it } from "vitest"; +// Namespace import so the module specifier — where TS7016 is reported — stays on +// the same line as the directive. A named import wide enough to hold all four +// bindings wraps past Biome's line width, which pushes the specifier out from +// under the directive and leaves the suppression unused (and the error live). // @ts-expect-error — plain .mjs with no bundled types; vitest transpiles it. -import { - bodyThemeAttrs, - PALETTES, - THEME_KINDS, - themeVarsCss, -} from "../../scripts/preview/vscode-theme-palettes.mjs"; +import * as themePalettes from "../../scripts/preview/vscode-theme-palettes.mjs"; import { THEME_KINDS as WIRE_THEME_KINDS } from "../../src/shared/protocol"; +const { bodyThemeAttrs, PALETTES, THEME_KINDS, themeVarsCss } = themePalettes; + const stylesCss = readFileSync(new URL("../../src/webview/styles.css", import.meta.url), "utf8"); // Live rules only — see the comment-strip rationale at the selector assertions below. const liveCss = stylesCss.replace(/\/\*[\s\S]*?\*\//g, ""); diff --git a/test/build/tsconfig.json b/test/build/tsconfig.json new file mode 100644 index 00000000..9ed2c489 --- /dev/null +++ b/test/build/tsconfig.json @@ -0,0 +1,33 @@ +{ + // Type-check program for the build/tooling suite. vitest runs these files + // through esbuild transpile-only, so without this gate every type-level + // contract in test/build (the `@ts-expect-error` pins on the untyped .mjs + // imports, the workflow/SBOM parsing helpers) is erased and never enforced. + // + // These tests exercise the BUILD side of the repo — esbuild configs, the + // scripts/ tooling, the packaged workflow YAML — so the compiler view is + // Node + ESM, with no DOM lib and no vscode-webview types. + // + // The scripts/ and esbuild.config.mjs modules they import are plain .mjs with + // no bundled types. The shape to reach for is a line-scoped `@ts-expect-error` + // on the import alone — it self-verifies (goes red the day the module gains + // types) and leaves everything the test itself authors checked. Five suites + // (doc-sync, preview-server-theme, stale-todo-markers, todo-hygiene, + // verify-sbom-scope) still use a file-level `@ts-nocheck` instead, which + // switches the WHOLE file off — a type-level assertion in one of those is + // still vacuous. Swapping them over is its own TODO entry; do not read this + // config as proof that every file under it is checked. + // + // noEmit (inherited): this is a CI gate, not a build step — vitest still runs + // through its own transpiler. rootDir is the repo root so the cross-layer + // import from ../../src/shared resolves. + "extends": "../../tsconfig.base.json", + "compilerOptions": { + "module": "esnext", + "moduleResolution": "bundler", + "lib": ["es2022"], + "types": ["node"], + "rootDir": "../.." + }, + "include": ["**/*.ts", "../../src/shared/**/*.ts"] +} From c466d9dbd6717292af56f3e074e59e363298219f Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sat, 22 Aug 2026 22:33:38 +1000 Subject: [PATCH 02/16] docs(test): correct the comments this tsconfig change falsified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding test/build/ as a tsc project invalidated several nearby comments that asserted the opposite, and the review surfaced one annotation that reads as a checked pin but is not. - notice-covers-bundled-deps.test.ts: the `esbuild.BuildOptions[]` annotation looked like a compile-time pin on createBuildConfigs, but contextual typing feeds the target type back into Object.values's inference, so `any` satisfies it trivially. Spelled as an `as` cast instead, which is honest about being an unverified assertion. It is still load-bearing: without it the call is unknown[] (TS2345). - quoll-perf-flag.d.ts: "All four tsc programs" is now five. - publish-workflow-sbom-config.test.ts: the comment said its Record exhaustiveness check "never executes" because test/build/ is in no tsconfig. It executes now — this file carries no @ts-nocheck. The runtime assertion stays as a backstop, with the reason stated. - doc-sync / stale-todo-markers / todo-hygiene / preview-server-theme: all four justified their @ts-nocheck with "tsc does not include test/build/", which is no longer true of the directory. - tsconfig.json: replaced the hand-maintained five-filename list (also duplicated in CLAUDE.md) with a git grep the reader can run, and fixed the rootDir rationale — rootDir plays no part in module resolution, only emit layout and the TS6059 containment check. Measured: dropping the "Generate SBOM (SPDX)" entry yields TS2741 at the LOAD_BEARING literal; removing the cast yields TS2345. --- src/shared/quoll-perf-flag.d.ts | 2 +- test/build/doc-sync.test.ts | 5 ++++- test/build/notice-covers-bundled-deps.test.ts | 18 +++++++++++---- test/build/preview-server-theme.test.ts | 5 ++++- .../publish-workflow-sbom-config.test.ts | 15 +++++++------ test/build/stale-todo-markers.test.ts | 5 ++++- test/build/todo-hygiene.test.ts | 5 ++++- test/build/tsconfig.json | 22 ++++++++++++------- 8 files changed, 53 insertions(+), 24 deletions(-) diff --git a/src/shared/quoll-perf-flag.d.ts b/src/shared/quoll-perf-flag.d.ts index 67dac213..74096b63 100644 --- a/src/shared/quoll-perf-flag.d.ts +++ b/src/shared/quoll-perf-flag.d.ts @@ -13,6 +13,6 @@ // `const PERF_ENABLED = … ; if (PERF_ENABLED)` does NOT (esbuild keeps // `const o=!1; o&&fn()`). Do NOT reintroduce an intermediate const. // -// All four tsc programs include `src/shared/**`, so this one file is visible +// All five tsc programs include `src/shared/**`, so this one file is visible // everywhere with no tsconfig `include` edits. declare const QUOLL_PERF: boolean; diff --git a/test/build/doc-sync.test.ts b/test/build/doc-sync.test.ts index 6652020f..d42b8844 100644 --- a/test/build/doc-sync.test.ts +++ b/test/build/doc-sync.test.ts @@ -13,7 +13,10 @@ // separately by the local `pnpm check:doc-sync` run, not by this suite. // // @ts-nocheck — importing a plain .mjs with no bundled types; vitest runs -// this transpile-only and tsc does not include test/build/ in `pnpm compile`. +// this transpile-only. `test/build/tsconfig.json` DOES type-check this +// directory under `pnpm compile`, but this file opts out wholesale via the +// file-level directive, so nothing here is checked; swapping it for a +// line-scoped `@ts-expect-error` on the import alone is a tracked follow-up. import { describe, expect, it } from "vitest"; import { diff --git a/test/build/notice-covers-bundled-deps.test.ts b/test/build/notice-covers-bundled-deps.test.ts index d83b2116..30f8b6ef 100644 --- a/test/build/notice-covers-bundled-deps.test.ts +++ b/test/build/notice-covers-bundled-deps.test.ts @@ -62,10 +62,20 @@ describe("NOTICE covers every bundled third-party package", () => { // Every value is a config emitting into the packaged dist/ (host, webview, // test-harness). Union them all so a dependency bundled into ANY shipped // output must be attributed — not just host + webview. - // Annotated because createBuildConfigs comes from the untyped .mjs above: - // Object.values() over an `any` widens each entry to `unknown`, and this is - // the shape the rest of the suite (and esbuild.build) actually requires. - const configs: esbuild.BuildOptions[] = Object.values(createBuildConfigs({ production: true })); + // A CAST, deliberately — not a checked declaration. createBuildConfigs comes + // from the untyped .mjs above, so Object.values() over it widens each entry + // to `unknown`, which `.map(shippedPackages)` rejects; the cast is what + // narrows it to the shape esbuild.build actually requires. But it only + // ASSERTS that shape — nothing here verifies the factory really emits it. + // Written as an annotation it would READ like a compile-time pin while doing + // exactly this much: contextual typing feeds the annotation back into + // Object.values, and `any` satisfies whatever T it picks. Spelled as a + // cast so the file does not claim a check it is not performing. Giving + // esbuild.config.mjs a companion .d.mts (tracked in TODO.md) is what turns + // this back into a real pin. + const configs = Object.values( + createBuildConfigs({ production: true }) + ) as esbuild.BuildOptions[]; const sets = await Promise.all(configs.map(shippedPackages)); bundled = [...new Set(sets.flatMap((s) => [...s]))].sort(); notice = readFileSync(resolve(root, "NOTICE"), "utf8"); diff --git a/test/build/preview-server-theme.test.ts b/test/build/preview-server-theme.test.ts index 2d12b3ac..64563bfd 100644 --- a/test/build/preview-server-theme.test.ts +++ b/test/build/preview-server-theme.test.ts @@ -24,7 +24,10 @@ // /instance route only fills the template. No esbuild bundle is built on this path. // // @ts-nocheck — importing a plain .mjs with no bundled types; vitest runs this -// transpile-only and tsc does not include test/build/ in `pnpm compile`. +// transpile-only. `test/build/tsconfig.json` DOES type-check this directory +// under `pnpm compile`, but this file opts out wholesale via the file-level +// directive, so nothing here is checked; swapping it for a line-scoped +// `@ts-expect-error` on the import alone is a tracked follow-up. import type { Server } from "node:http"; import { afterEach, describe, expect, it } from "vitest"; diff --git a/test/build/publish-workflow-sbom-config.test.ts b/test/build/publish-workflow-sbom-config.test.ts index 26c12f27..0bcf2820 100644 --- a/test/build/publish-workflow-sbom-config.test.ts +++ b/test/build/publish-workflow-sbom-config.test.ts @@ -168,13 +168,14 @@ describe("CI rehearses the release SBOM sequence", () => { it.each(SHARED_STEPS)("keeps the load-bearing content of `%s`", (step) => { // Assert the needle EXISTS before using it. The `Record` type - // above looks like it already makes that impossible, but nothing enforces it - // here: `test/build/` is in none of the four tsconfigs `pnpm compile` runs - // (the trap CLAUDE.md documents for test/markdown and test/shared), so the - // exhaustiveness check never executes — and at runtime a missing entry makes - // this `expect(…).toMatch(undefined)`, which vitest PASSES silently. Without - // this line a forgotten entry ships with zero enforcement: verified by - // deleting one and watching the whole suite stay green. + // above IS enforced now — this file carries no `@ts-nocheck` and + // `test/build/tsconfig.json` type-checks the directory as `pnpm compile`'s + // 5th project, so a forgotten entry fails the build (measured: dropping the + // "Generate SBOM (SPDX)" entry yields TS2741 at the LOAD_BEARING literal). + // Kept as a runtime belt-and-braces check anyway, because the failure mode + // it covers is silent: if that type-level guarantee ever regresses, a + // missing entry degrades this to `expect(…).toMatch(undefined)`, which + // vitest PASSES. const needle = LOAD_BEARING[step]; expect(needle).toBeDefined(); expect(stepBlock(sbomJob, step, "ci.yml")).toMatch(needle); diff --git a/test/build/stale-todo-markers.test.ts b/test/build/stale-todo-markers.test.ts index 06e11457..f05f401e 100644 --- a/test/build/stale-todo-markers.test.ts +++ b/test/build/stale-todo-markers.test.ts @@ -17,7 +17,10 @@ // as test/build/todo-hygiene.test.ts. // // @ts-nocheck — importing a plain .mjs with no bundled types; vitest runs this -// transpile-only and tsc does not include test/build/ in `pnpm compile`. +// transpile-only. `test/build/tsconfig.json` DOES type-check this directory +// under `pnpm compile`, but this file opts out wholesale via the file-level +// directive, so nothing here is checked; swapping it for a line-scoped +// `@ts-expect-error` on the import alone is a tracked follow-up. import { describe, expect, it } from "vitest"; import { diff --git a/test/build/todo-hygiene.test.ts b/test/build/todo-hygiene.test.ts index 0f443ea6..18c12ed3 100644 --- a/test/build/todo-hygiene.test.ts +++ b/test/build/todo-hygiene.test.ts @@ -12,7 +12,10 @@ // `pnpm check:todo-hygiene` run, not by this suite. // // @ts-nocheck — importing a plain .mjs with no bundled types; vitest runs -// this transpile-only and tsc does not include test/build/ in `pnpm compile`. +// this transpile-only. `test/build/tsconfig.json` DOES type-check this +// directory under `pnpm compile`, but this file opts out wholesale via the +// file-level directive, so nothing here is checked; swapping it for a +// line-scoped `@ts-expect-error` on the import alone is a tracked follow-up. import { describe, expect, it } from "vitest"; import { diff --git a/test/build/tsconfig.json b/test/build/tsconfig.json index 9ed2c489..2d9b0821 100644 --- a/test/build/tsconfig.json +++ b/test/build/tsconfig.json @@ -11,16 +11,22 @@ // The scripts/ and esbuild.config.mjs modules they import are plain .mjs with // no bundled types. The shape to reach for is a line-scoped `@ts-expect-error` // on the import alone — it self-verifies (goes red the day the module gains - // types) and leaves everything the test itself authors checked. Five suites - // (doc-sync, preview-server-theme, stale-todo-markers, todo-hygiene, - // verify-sbom-scope) still use a file-level `@ts-nocheck` instead, which - // switches the WHOLE file off — a type-level assertion in one of those is - // still vacuous. Swapping them over is its own TODO entry; do not read this - // config as proof that every file under it is checked. + // types) and leaves everything the test itself authors checked. Several + // suites still use a file-level `@ts-nocheck` instead, which switches the + // WHOLE file off — a type-level assertion in one of those is still vacuous. + // (`git grep -l '@ts-nocheck' test/build` lists the current set; deliberately + // not enumerated here, because the same list is also written down in + // CLAUDE.md and a hand-copied roster rots the moment one file is swapped + // over. See TODO.md for the tracked follow-up to swap them one at a time.) + // Do not read this config as proof that every file under it is checked. // // noEmit (inherited): this is a CI gate, not a build step — vitest still runs - // through its own transpiler. rootDir is the repo root so the cross-layer - // import from ../../src/shared resolves. + // through its own transpiler. rootDir is pinned to the repo root explicitly + // (matching the sibling test tsconfigs) so the program's containment boundary + // is stated rather than inferred — the ../../src/shared files sit outside + // test/build/, and an inferred rootDir would silently widen with the include. + // It plays no part in resolving that cross-layer import: rootDir governs emit + // layout and the TS6059 containment check only, never module resolution. "extends": "../../tsconfig.base.json", "compilerOptions": { "module": "esnext", From 0e97dcadef6be5d4f44340875aa1a9b077a2cd85 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sat, 22 Aug 2026 22:44:38 +1000 Subject: [PATCH 03/16] docs(build): drop a magic tsc-program count and fix a sibling rootDir rationale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The perf-flag comment named a literal program count that this PR itself had to bump, and it was under-counting anyway (six programs include src/shared, not the five in the compile chain). State the property instead of the number. test/webview/tsconfig.json claimed rootDir is why its cross-layer imports resolve — the same false mechanism this PR corrected in the new config, which now sits next to it contradicting it. rootDir governs emit layout and the TS6059 containment check only. --- src/shared/quoll-perf-flag.d.ts | 4 ++-- test/webview/tsconfig.json | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/shared/quoll-perf-flag.d.ts b/src/shared/quoll-perf-flag.d.ts index 74096b63..093eaef1 100644 --- a/src/shared/quoll-perf-flag.d.ts +++ b/src/shared/quoll-perf-flag.d.ts @@ -13,6 +13,6 @@ // `const PERF_ENABLED = … ; if (PERF_ENABLED)` does NOT (esbuild keeps // `const o=!1; o&&fn()`). Do NOT reintroduce an intermediate const. // -// All five tsc programs include `src/shared/**`, so this one file is visible -// everywhere with no tsconfig `include` edits. +// Every tsc program in the repo includes `src/shared/**`, so this one file is +// visible everywhere with no tsconfig `include` edits. declare const QUOLL_PERF: boolean; diff --git a/test/webview/tsconfig.json b/test/webview/tsconfig.json index 788de19f..86c13675 100644 --- a/test/webview/tsconfig.json +++ b/test/webview/tsconfig.json @@ -14,7 +14,9 @@ // // noEmit (inherited): this is a CI gate, not a build step — vitest still // runs through its own transpiler. rootDir is the repo root so the - // cross-layer imports from ../../src/* resolve. + // cross-layer files under ../../src/* stay inside the program's containment + // boundary (TS6059) — it plays no part in resolving those imports, which is + // moduleResolution's job. "extends": "../../tsconfig.base.json", "compilerOptions": { "module": "esnext", From 6dbd87e20eda1afd9b96d0b4fc612c4c6943024d Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sat, 22 Aug 2026 23:49:09 +1000 Subject: [PATCH 04/16] docs(build): scope two claims this PR's own corrections over-broadened Cycle 2 of the review found that cycle 1's corrections had introduced two false statements of their own. The perf-flag comment had been broadened to "every tsc program in the repo includes src/shared/**". There are seven programs, not the five the compile script names: test/extension/tsconfig.json is the E2E emit config, rootDir ".", and pulls in no src/ file at all. Scope the claim to the programs that compile a QUOLL_PERF reference, name the exception, and point at `git ls-files '*tsconfig*.json'` as the way to check it -- reading the compile script misses the configs no script chains, which is how three separate passes over this PR reached the same wrong answer. test/build/tsconfig.json still justified omitting the @ts-nocheck roster by citing a duplicate list in CLAUDE.md that cycle 1 had itself deleted. Comment-only: there is no behaviour here to pin, which is why the tests that guard these facts land in a separate commit. --- src/shared/quoll-perf-flag.d.ts | 10 ++++++++-- test/build/tsconfig.json | 7 ++++--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/shared/quoll-perf-flag.d.ts b/src/shared/quoll-perf-flag.d.ts index 093eaef1..87f503f9 100644 --- a/src/shared/quoll-perf-flag.d.ts +++ b/src/shared/quoll-perf-flag.d.ts @@ -13,6 +13,12 @@ // `const PERF_ENABLED = … ; if (PERF_ENABLED)` does NOT (esbuild keeps // `const o=!1; o&&fn()`). Do NOT reintroduce an intermediate const. // -// Every tsc program in the repo includes `src/shared/**`, so this one file is -// visible everywhere with no tsconfig `include` edits. +// Every tsc program that compiles a `QUOLL_PERF` reference includes +// `src/shared/**`, so this one file reaches all of them with no tsconfig +// `include` edits. Not every program in the repo does: the E2E emit config +// (`test/extension/tsconfig.json`, `rootDir: "."`) pulls in no `src/` file at +// all — it deliberately mirrors the wire constants rather than importing them, +// and it references no QUOLL_PERF. Before widening this claim, enumerate with +// `git ls-files '*tsconfig*.json'`; reading the `compile` script instead misses +// the configs no script chains. declare const QUOLL_PERF: boolean; diff --git a/test/build/tsconfig.json b/test/build/tsconfig.json index 2d9b0821..46eab1b6 100644 --- a/test/build/tsconfig.json +++ b/test/build/tsconfig.json @@ -15,9 +15,10 @@ // suites still use a file-level `@ts-nocheck` instead, which switches the // WHOLE file off — a type-level assertion in one of those is still vacuous. // (`git grep -l '@ts-nocheck' test/build` lists the current set; deliberately - // not enumerated here, because the same list is also written down in - // CLAUDE.md and a hand-copied roster rots the moment one file is swapped - // over. See TODO.md for the tracked follow-up to swap them one at a time.) + // not enumerated here, because the tracked follow-up swaps them over one file + // at a time, so a hand-copied roster goes stale on the first swap. CLAUDE.md + // points at the same grep for that reason; the only place that does name the + // files is that TODO.md follow-up entry, where the roster IS the work item.) // Do not read this config as proof that every file under it is checked. // // noEmit (inherited): this is a CI gate, not a build step — vitest still runs From 52d657a5664e1fcada28588101f16e492137ded0 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sat, 22 Aug 2026 23:49:17 +1000 Subject: [PATCH 05/16] test(build): pin the compile chain and the E2E-program exception Every type-level guarantee in this repo holds only because `compile` runs tsc over the project that owns it. Drop a project from that chain -- a reformat, a bad merge, a "tidy the scripts" pass -- and build, test and CI all stay green while every assertion that project gated goes permanently vacuous. That silent revert is the failure mode test/build/tsconfig.json was added to close, and nothing pinned the wiring itself until now. Extracts the -p arguments rather than substring matching: `toContain("tsc -p ./")` is satisfied by any of the four nested project paths, so the root pin could never have gone red. Also pins the exception named by src/shared/quoll-perf-flag.d.ts -- the E2E program compiles no src/ file. Widening that include silently falsifies the comment; now it fails here instead. Verified red: compile chain: expected [ './', ...(3) ] to deeply equal [ './', ...(4) ] E2E exception: expected [ '../../src/shared/**/*.ts' ] to deeply equal [] --- test/build/compile-chain.test.ts | 72 ++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 test/build/compile-chain.test.ts diff --git a/test/build/compile-chain.test.ts b/test/build/compile-chain.test.ts new file mode 100644 index 00000000..45ec9274 --- /dev/null +++ b/test/build/compile-chain.test.ts @@ -0,0 +1,72 @@ +// Pins the tsc project chain behind `pnpm compile`. +// +// Every type-level guarantee in this repo holds only because `compile` runs +// tsc over the project that owns it — the `@ts-expect-error` pins in +// test/build, the branded-offset assertions in test/webview, the protocol +// drift guard in test/extension. Neither vitest (esbuild transpile-only) nor +// `pnpm build`'s bundling step type-checks anything on its own, so a project +// dropped from this chain by a reformat, a bad merge or a "tidy the scripts" +// pass leaves `pnpm build`, `pnpm test` and CI ALL GREEN while every assertion +// that project gated goes permanently vacuous. That silent revert is the exact +// failure mode test/build/tsconfig.json was added to close, and this test is +// its tripwire. Pinning all five projects rather than just test/build's is +// deliberate: the accident is not specific to this directory. +// +// The roster is written down, not derived from disk, because membership is a +// judgement call: `git ls-files '*tsconfig*.json'` lists EIGHT configs and +// three of them belong outside this chain on purpose — tsconfig.base.json is +// extends-only, src/webview/tsconfig.json runs as the separate +// `compile:webview` script, and test/extension/tsconfig.json is the E2E emit +// program driven by `pnpm test:e2e:run`. A derived list would quietly adopt +// whatever config lands next instead of forcing that call to be reviewed. +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it } from "vitest"; + +const pkg = JSON.parse( + readFileSync(fileURLToPath(new URL("../../package.json", import.meta.url)), "utf8") +); + +// Spelled exactly as the `-p` arguments appear in the script. Order matches the +// script for a simple exact compare; it is not load-bearing, so reorder both +// sides freely if the script's order ever changes for a real reason. +const CHAINED_PROJECTS = [ + "./", + "./test/extension/tsconfig.unit.json", + "./test/webview/tsconfig.json", + "./test/webview-browser/tsconfig.json", + "./test/build/tsconfig.json", +]; + +describe("pnpm compile project chain", () => { + it("type-checks exactly the five reviewed projects", () => { + const compileScript: string = pkg.scripts.compile; + // Extracting the `-p` arguments (rather than substring-matching each + // project) keeps the root `./` pin honest: a bare `toContain("tsc -p ./")` + // is satisfied by any of the four nested projects and would never go red. + const chained = [...compileScript.matchAll(/tsc -p (\S+)/g)].map((m) => m[1]); + expect(chained).toEqual(CHAINED_PROJECTS); + }); + + // The scoped claim in src/shared/quoll-perf-flag.d.ts names this program as + // THE exception: the E2E config compiles no src/ file, which is why "every + // program includes src/shared" was false. Three separate passes over this PR + // asserted the universal anyway, so pin the exception rather than trusting + // the next reader to re-derive it. Widening this include is allowed — but it + // falsifies that comment, and this is what makes that visible. + it("keeps the E2E program free of src/, as the perf-flag comment states", () => { + const e2e = JSON.parse( + readFileSync(fileURLToPath(new URL("../extension/tsconfig.json", import.meta.url)), "utf8") + ); + expect(e2e.include.filter((p: string) => p.includes("src/"))).toEqual([]); + // rootDir "." is what makes reaching into ../../src a TS6059 error rather + // than a silent widening, so it is part of the same guarantee. + expect(e2e.compilerOptions.rootDir).toBe("."); + }); + + it("runs that chain ahead of the bundle step", () => { + // `build` emitting dist/ without the gate would restore the same silent + // green: the bundle would ship from source no compiler ever looked at. + expect(pkg.scripts.build).toContain("pnpm compile"); + }); +}); From 00eed9eb6ed173dc973a704ce93b4e41ef42ded9 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sun, 23 Aug 2026 01:06:30 +1000 Subject: [PATCH 06/16] test(build): compare build steps, not substrings, for the compile gate The bundle-step assertion had the exact hole this file warns about for `tsc -p ./`: "pnpm compile" is a prefix of "pnpm compile:webview", which `build` also runs, so toContain() stayed green after the compile step was deleted outright. Split on && and compare whole steps, and pin the ordering against the esbuild step rather than implying it in the test name. Verified red by removing `pnpm compile &&` from `build`: AssertionError: expected [ 'pnpm compile:webview', ...(1) ] to include 'pnpm compile' --- test/build/compile-chain.test.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/build/compile-chain.test.ts b/test/build/compile-chain.test.ts index 45ec9274..ce4eb90c 100644 --- a/test/build/compile-chain.test.ts +++ b/test/build/compile-chain.test.ts @@ -67,6 +67,16 @@ describe("pnpm compile project chain", () => { it("runs that chain ahead of the bundle step", () => { // `build` emitting dist/ without the gate would restore the same silent // green: the bundle would ship from source no compiler ever looked at. - expect(pkg.scripts.build).toContain("pnpm compile"); + // + // Split into steps rather than substring-matching the script. "pnpm + // compile" is a PREFIX of "pnpm compile:webview", which `build` also runs, + // so toContain("pnpm compile") stays green after the compile step is + // deleted outright — the same trap this file calls out for `tsc -p ./` + // above, and it was live here until the cycle-2 simplify pass caught it. + const steps: string[] = pkg.scripts.build.split("&&").map((s: string) => s.trim()); + expect(steps).toContain("pnpm compile"); + expect(steps.indexOf("pnpm compile")).toBeLessThan( + steps.findIndex((s: string) => s.includes("esbuild.config.mjs")) + ); }); }); From 38f4a423768c5b546d6c62e155c0da000449e848 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sun, 23 Aug 2026 01:57:45 +1000 Subject: [PATCH 07/16] test(build): pin the whole compile script, both gates, and parse tsconfigs as JSONC Cycle 3 found the tripwire too loose to catch the failures it was written for. Extracting `-p` arguments never saw what sat between the steps, so three mutations left it green while the gate was dead: `;` instead of `&&` (in sh the exit status is the last command's, demoting the first four projects to non-gates), `|| true` on a step, and an `echo` in front of one. Pin the whole script by exact equality instead -- strictly stronger and simpler than the regex it replaces. The ordering test pinned `pnpm compile` but not `pnpm compile:webview`, which is the only tsc pass over src/webview and gates the same dist/webview/ output; moving it behind esbuild stayed green. Pin both gates, and guard the bundle step's index so a missing step fails closed. tsconfig reads now tolerate JSONC. Only whole-line `//` comments are dropped, so a `//` inside a string literal survives; test/extension/tsconfig.json is the only comment-free config of the eight, making a future doc comment likely -- and JSON.parse would have turned that into a SyntaxError, breaking `pnpm test` on a documentation edit. Also corrects two false claims in test/build/tsconfig.json: the roster is named in LEARNING.md as well as TODO.md, and the unanchored grep returns eight files, not five -- three are prose about the directive, including two suites that say they carry none. `git grep -l '^// @ts-nocheck' test/build` returns exactly the five that do. Verified red: dropping a project, `&&`->`;`, `|| true`, moving compile:webview behind the bundle step, and leaking src/ into the E2E include. Verified green: adding a `//` comment to that config. --- test/build/compile-chain.test.ts | 63 +++++++++++++++++++++++--------- test/build/tsconfig.json | 19 +++++++--- 2 files changed, 59 insertions(+), 23 deletions(-) diff --git a/test/build/compile-chain.test.ts b/test/build/compile-chain.test.ts index ce4eb90c..719d4642 100644 --- a/test/build/compile-chain.test.ts +++ b/test/build/compile-chain.test.ts @@ -38,14 +38,32 @@ const CHAINED_PROJECTS = [ "./test/build/tsconfig.json", ]; +// tsconfig files in this repo are JSONC. Only lines that are ENTIRELY a `//` +// comment are dropped: a strip-to-end-of-line would corrupt a `//` sitting +// inside a string literal (a URL, a protocol-relative path). Block comments and +// trailing commas are still unsupported and would throw — no config here uses +// them. Without this, adding a doc comment to a config read below turns a clean +// assertion failure into a SyntaxError, i.e. a doc edit breaking `pnpm test`. +const parseJsonc = (source: string) => + JSON.parse( + source + .split("\n") + .filter((line) => !line.trimStart().startsWith("//")) + .join("\n") + ); + describe("pnpm compile project chain", () => { it("type-checks exactly the five reviewed projects", () => { const compileScript: string = pkg.scripts.compile; - // Extracting the `-p` arguments (rather than substring-matching each - // project) keeps the root `./` pin honest: a bare `toContain("tsc -p ./")` - // is satisfied by any of the four nested projects and would never go red. - const chained = [...compileScript.matchAll(/tsc -p (\S+)/g)].map((m) => m[1]); - expect(chained).toEqual(CHAINED_PROJECTS); + // Pin the WHOLE script by exact equality rather than extracting its `-p` + // arguments. A partial extraction never sees what sits between and around + // the steps, so all of these stay green while the gate is dead: + // `tsc -p X ; tsc -p Y` (in sh the exit status is the LAST command's, which + // demotes the first four projects to non-gates), `tsc -p X || true`, and an + // `echo` in front of a step. Substring matching is weaker still — a bare + // `toContain("tsc -p ./")` is satisfied by any of the four nested projects, + // so the root pin would never go red. + expect(compileScript).toBe(CHAINED_PROJECTS.map((p) => `tsc -p ${p}`).join(" && ")); }); // The scoped claim in src/shared/quoll-perf-flag.d.ts names this program as @@ -55,7 +73,7 @@ describe("pnpm compile project chain", () => { // the next reader to re-derive it. Widening this include is allowed — but it // falsifies that comment, and this is what makes that visible. it("keeps the E2E program free of src/, as the perf-flag comment states", () => { - const e2e = JSON.parse( + const e2e = parseJsonc( readFileSync(fileURLToPath(new URL("../extension/tsconfig.json", import.meta.url)), "utf8") ); expect(e2e.include.filter((p: string) => p.includes("src/"))).toEqual([]); @@ -64,19 +82,30 @@ describe("pnpm compile project chain", () => { expect(e2e.compilerOptions.rootDir).toBe("."); }); - it("runs that chain ahead of the bundle step", () => { - // `build` emitting dist/ without the gate would restore the same silent + it("runs both type-check gates ahead of the bundle step", () => { + // `build` emitting dist/ without the gates would restore the same silent // green: the bundle would ship from source no compiler ever looked at. + // BOTH gates are pinned — `compile` covers the host and test programs, and + // `compile:webview` is the only tsc pass over src/webview, which lands in + // the same dist/webview/ output. Pinning `compile` alone left the webview + // half free to drift behind esbuild while this test stayed green. + // + // Split into steps rather than substring-matching the script: "pnpm + // compile" is a PREFIX of "pnpm compile:webview", so toContain("pnpm + // compile") stays green after the compile step is deleted outright — the + // same trap the `tsc -p ./` note above calls out, and it was live here + // until the cycle-2 simplify pass caught it. // - // Split into steps rather than substring-matching the script. "pnpm - // compile" is a PREFIX of "pnpm compile:webview", which `build` also runs, - // so toContain("pnpm compile") stays green after the compile step is - // deleted outright — the same trap this file calls out for `tsc -p ./` - // above, and it was live here until the cycle-2 simplify pass caught it. + // Unlike `compile`, `build` is deliberately NOT pinned by exact equality. + // Its roster is not the contract under test and the bundle step's flags are + // expected to move for ordinary reasons; only the relative order is + // load-bearing, so an exact pin here would be a false tripwire. const steps: string[] = pkg.scripts.build.split("&&").map((s: string) => s.trim()); - expect(steps).toContain("pnpm compile"); - expect(steps.indexOf("pnpm compile")).toBeLessThan( - steps.findIndex((s: string) => s.includes("esbuild.config.mjs")) - ); + const bundleStep = steps.findIndex((s: string) => s.includes("esbuild.config.mjs")); + expect(bundleStep).toBeGreaterThanOrEqual(0); + for (const gate of ["pnpm compile", "pnpm compile:webview"]) { + expect(steps).toContain(gate); + expect(steps.indexOf(gate)).toBeLessThan(bundleStep); + } }); }); diff --git a/test/build/tsconfig.json b/test/build/tsconfig.json index 46eab1b6..20a6bcd8 100644 --- a/test/build/tsconfig.json +++ b/test/build/tsconfig.json @@ -11,14 +11,21 @@ // The scripts/ and esbuild.config.mjs modules they import are plain .mjs with // no bundled types. The shape to reach for is a line-scoped `@ts-expect-error` // on the import alone — it self-verifies (goes red the day the module gains - // types) and leaves everything the test itself authors checked. Several + // types) and leaves everything the test itself authors checked. Five // suites still use a file-level `@ts-nocheck` instead, which switches the // WHOLE file off — a type-level assertion in one of those is still vacuous. - // (`git grep -l '@ts-nocheck' test/build` lists the current set; deliberately - // not enumerated here, because the tracked follow-up swaps them over one file - // at a time, so a hand-copied roster goes stale on the first swap. CLAUDE.md - // points at the same grep for that reason; the only place that does name the - // files is that TODO.md follow-up entry, where the roster IS the work item.) + // + // To list them, anchor the grep to the directive itself: + // `git grep -l '^// @ts-nocheck' test/build` returns exactly those five. The + // unanchored `git grep -l '@ts-nocheck' test/build` returns EIGHT and is + // misleading — three of those hits are prose ABOUT the directive rather than + // the directive: this file, plus theme-palettes.test.ts and + // publish-workflow-sbom-config.test.ts, which each note that they carry none. + // The roster is deliberately not enumerated here: the tracked follow-up swaps + // the files over one at a time, so a hand-copied list goes stale on the first + // swap. Two places do name them — the TODO.md follow-up entry, where the + // roster IS the work item, and the 2026-08-22 LEARNING.md entry — and keeping + // both current is part of that follow-up. // Do not read this config as proof that every file under it is checked. // // noEmit (inherited): this is a CI gate, not a build step — vitest still runs From bac0fea5023fb0360d014e619ffaf5f1e5a9f64a Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sun, 23 Aug 2026 02:07:17 +1000 Subject: [PATCH 08/16] test(build): name what test 1 now pins, and record why toContain is not redundant Cycle-3 simplify pass. Test 1's assertion was widened from a project roster to a whole-script pin, but its name still described only half of what fails it. The comment on `toContain` guards a live trap: a missing gate makes indexOf return -1, and -1 satisfies toBeLessThan, so the ordering assertion alone would pass silently. That line reads as redundant and is the one a future simplify pass would delete. Also reflows two comment blocks in test/build/tsconfig.json that the previous edit left broken mid-phrase. --- test/build/compile-chain.test.ts | 13 ++++++++----- test/build/tsconfig.json | 7 ++++--- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/test/build/compile-chain.test.ts b/test/build/compile-chain.test.ts index 719d4642..e2a2806f 100644 --- a/test/build/compile-chain.test.ts +++ b/test/build/compile-chain.test.ts @@ -53,7 +53,7 @@ const parseJsonc = (source: string) => ); describe("pnpm compile project chain", () => { - it("type-checks exactly the five reviewed projects", () => { + it("type-checks exactly the five reviewed projects, &&-chained with nothing else", () => { const compileScript: string = pkg.scripts.compile; // Pin the WHOLE script by exact equality rather than extracting its `-p` // arguments. A partial extraction never sees what sits between and around @@ -100,12 +100,15 @@ describe("pnpm compile project chain", () => { // Its roster is not the contract under test and the bundle step's flags are // expected to move for ordinary reasons; only the relative order is // load-bearing, so an exact pin here would be a false tripwire. - const steps: string[] = pkg.scripts.build.split("&&").map((s: string) => s.trim()); - const bundleStep = steps.findIndex((s: string) => s.includes("esbuild.config.mjs")); - expect(bundleStep).toBeGreaterThanOrEqual(0); + const buildScript: string = pkg.scripts.build; + const steps = buildScript.split("&&").map((s) => s.trim()); + const bundleStepIndex = steps.findIndex((s) => s.includes("esbuild.config.mjs")); + expect(bundleStepIndex).toBeGreaterThanOrEqual(0); for (const gate of ["pnpm compile", "pnpm compile:webview"]) { + // toContain is not redundant with the ordering check below: a missing + // gate makes indexOf return -1, which would pass `toBeLessThan` silently. expect(steps).toContain(gate); - expect(steps.indexOf(gate)).toBeLessThan(bundleStep); + expect(steps.indexOf(gate)).toBeLessThan(bundleStepIndex); } }); }); diff --git a/test/build/tsconfig.json b/test/build/tsconfig.json index 20a6bcd8..940db9f9 100644 --- a/test/build/tsconfig.json +++ b/test/build/tsconfig.json @@ -11,9 +11,9 @@ // The scripts/ and esbuild.config.mjs modules they import are plain .mjs with // no bundled types. The shape to reach for is a line-scoped `@ts-expect-error` // on the import alone — it self-verifies (goes red the day the module gains - // types) and leaves everything the test itself authors checked. Five - // suites still use a file-level `@ts-nocheck` instead, which switches the - // WHOLE file off — a type-level assertion in one of those is still vacuous. + // types) and leaves everything the test itself authors checked. Five suites + // still use a file-level `@ts-nocheck` instead, which switches the WHOLE file + // off — a type-level assertion in one of those is still vacuous. // // To list them, anchor the grep to the directive itself: // `git grep -l '^// @ts-nocheck' test/build` returns exactly those five. The @@ -26,6 +26,7 @@ // swap. Two places do name them — the TODO.md follow-up entry, where the // roster IS the work item, and the 2026-08-22 LEARNING.md entry — and keeping // both current is part of that follow-up. + // // Do not read this config as proof that every file under it is checked. // // noEmit (inherited): this is a CI gate, not a build step — vitest still runs From 557ff605418148c73519cd7869deb2757c8fa27d Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sun, 23 Aug 2026 02:24:20 +1000 Subject: [PATCH 09/16] test(build): close the last three gaps in the compile-chain tripwire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex and Fable were consulted independently on whether to fix these or stop; both returned FIX_NOW on all three, and both judged text-pinning the right approach rather than a treadmill -- only three scripts sit upstream of the guarantee, so pinning all three is a fixed point with no surface left to expand into. compile:webview's body was never pinned. `build` calling it means nothing if the script stops type-checking: "echo skip" or a trailing `|| true` left every other assertion green while the only tsc pass over src/webview disappeared. Pin the body exactly, as `compile` already is. The gate-order check split on `&&` and compared indices, which cannot see shell short-circuiting: `pnpm compile && pnpm compile:webview && false || node esbuild…` put both gates before the bundle segment while `||` ran the bundler on failure. Assert the required prefix instead -- it models the real property (the bundler is reachable only through two successful gates) and replaces the split/indexOf logic rather than adding to it. The E2E include filter matched "src/" as a substring, missing a bare-directory include spelled "../../src". Match `src` as a path segment. Verified by 12 mutations: all four previously-green holes now red, all seven earlier holes still red, and two negative controls still green -- an added bundler flag and a JSONC comment, confirming the pins did not widen into false tripwires. --- test/build/compile-chain.test.ts | 45 ++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/test/build/compile-chain.test.ts b/test/build/compile-chain.test.ts index e2a2806f..f1d0c454 100644 --- a/test/build/compile-chain.test.ts +++ b/test/build/compile-chain.test.ts @@ -76,7 +76,10 @@ describe("pnpm compile project chain", () => { const e2e = parseJsonc( readFileSync(fileURLToPath(new URL("../extension/tsconfig.json", import.meta.url)), "utf8") ); - expect(e2e.include.filter((p: string) => p.includes("src/"))).toEqual([]); + // Match `src` as a whole path SEGMENT: a bare directory include spelled + // "../../src" (no trailing slash) is valid tsconfig and pulls in the same + // files, but `includes("src/")` does not see it. + expect(e2e.include.filter((p: string) => /(^|\/)src(\/|$)/.test(p))).toEqual([]); // rootDir "." is what makes reaching into ../../src a TS6059 error rather // than a silent widening, so it is part of the same guarantee. expect(e2e.compilerOptions.rootDir).toBe("."); @@ -90,25 +93,29 @@ describe("pnpm compile project chain", () => { // the same dist/webview/ output. Pinning `compile` alone left the webview // half free to drift behind esbuild while this test stayed green. // - // Split into steps rather than substring-matching the script: "pnpm - // compile" is a PREFIX of "pnpm compile:webview", so toContain("pnpm - // compile") stays green after the compile step is deleted outright — the - // same trap the `tsc -p ./` note above calls out, and it was live here - // until the cycle-2 simplify pass caught it. + // Assert the required PREFIX rather than splitting into steps and checking + // order. Splitting on `&&` cannot see shell short-circuiting, so it accepts + // `pnpm compile && pnpm compile:webview && false || node esbuild…`: both + // gates precede the bundle-bearing segment, yet `||` runs the bundler when + // a gate fails. A prefix pin models what actually matters — the bundler is + // reachable only through two successful `&&` gates — and it replaces the + // split/indexOf logic rather than adding to it. // - // Unlike `compile`, `build` is deliberately NOT pinned by exact equality. - // Its roster is not the contract under test and the bundle step's flags are - // expected to move for ordinary reasons; only the relative order is - // load-bearing, so an exact pin here would be a false tripwire. + // `build` is still deliberately NOT pinned by exact equality: the bundle + // step's trailing flags are expected to move for ordinary reasons, and only + // the gating prefix is load-bearing. const buildScript: string = pkg.scripts.build; - const steps = buildScript.split("&&").map((s) => s.trim()); - const bundleStepIndex = steps.findIndex((s) => s.includes("esbuild.config.mjs")); - expect(bundleStepIndex).toBeGreaterThanOrEqual(0); - for (const gate of ["pnpm compile", "pnpm compile:webview"]) { - // toContain is not redundant with the ordering check below: a missing - // gate makes indexOf return -1, which would pass `toBeLessThan` silently. - expect(steps).toContain(gate); - expect(steps.indexOf(gate)).toBeLessThan(bundleStepIndex); - } + expect( + buildScript.startsWith("pnpm compile && pnpm compile:webview && node esbuild.config.mjs") + ).toBe(true); + }); + + it("keeps compile:webview a real tsc pass", () => { + // `build` calling `compile:webview` means nothing if that script stops + // type-checking: `"echo skip"` or a trailing `|| true` leaves every other + // assertion here green while the ONLY tsc pass over src/webview disappears + // — the same accident this file exists to catch, one level down. The body + // is pinned exactly, matching the treatment `compile` already gets. + expect(pkg.scripts["compile:webview"]).toBe("tsc -p ./src/webview"); }); }); From 95403f58cfd9bf7270777c763e831109c7229276 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sun, 23 Aug 2026 02:58:03 +1000 Subject: [PATCH 10/16] test(build): show the offending script text when the build-prefix pin fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `expect(script.startsWith(prefix)).toBe(true)` reports only "expected false to be true", which tells whoever trips this tripwire nothing. Compare the sliced prefix instead, so the failure prints the injected text -- under the short-circuit mutation the `&& false ||` is visible in the diff itself. Rename the test from "runs both type-check gates ahead of the bundle step" to "reaches the bundle step only through both type-check gates": the old name described the ordering check this commit's predecessor deleted, and ordering is the weaker property that let `&& false || node esbuild…` through in the first place. --- test/build/compile-chain.test.ts | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/test/build/compile-chain.test.ts b/test/build/compile-chain.test.ts index f1d0c454..2475676c 100644 --- a/test/build/compile-chain.test.ts +++ b/test/build/compile-chain.test.ts @@ -85,7 +85,7 @@ describe("pnpm compile project chain", () => { expect(e2e.compilerOptions.rootDir).toBe("."); }); - it("runs both type-check gates ahead of the bundle step", () => { + it("reaches the bundle step only through both type-check gates", () => { // `build` emitting dist/ without the gates would restore the same silent // green: the bundle would ship from source no compiler ever looked at. // BOTH gates are pinned — `compile` covers the host and test programs, and @@ -98,16 +98,18 @@ describe("pnpm compile project chain", () => { // `pnpm compile && pnpm compile:webview && false || node esbuild…`: both // gates precede the bundle-bearing segment, yet `||` runs the bundler when // a gate fails. A prefix pin models what actually matters — the bundler is - // reachable only through two successful `&&` gates — and it replaces the - // split/indexOf logic rather than adding to it. + // reachable only through two successful `&&` gates. // - // `build` is still deliberately NOT pinned by exact equality: the bundle - // step's trailing flags are expected to move for ordinary reasons, and only - // the gating prefix is load-bearing. + // The prefix stops at the bundler's name on purpose: `build` is still + // deliberately NOT pinned by exact equality, because the bundle step's + // trailing flags are expected to move for ordinary reasons and only the + // gating prefix is load-bearing. + const REQUIRED_BUILD_PREFIX = "pnpm compile && pnpm compile:webview && node esbuild.config.mjs"; const buildScript: string = pkg.scripts.build; - expect( - buildScript.startsWith("pnpm compile && pnpm compile:webview && node esbuild.config.mjs") - ).toBe(true); + // Compare a slice rather than asserting `startsWith(…)` is true: on failure + // this reports the actual leading text against the expected prefix, where + // the boolean form reports only `expected false to be true`. + expect(buildScript.slice(0, REQUIRED_BUILD_PREFIX.length)).toBe(REQUIRED_BUILD_PREFIX); }); it("keeps compile:webview a real tsc pass", () => { From 878d66ace0f6b60ab3bb1a7ab038047326334ee1 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sun, 23 Aug 2026 03:06:48 +1000 Subject: [PATCH 11/16] test(build): pin the build script exactly, closing the unconstrained tail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex and Fable independently found the prefix pin leaves the script's tail free: `… --production || node esbuild…` bundles ungated when a gate fails, and `… --production || true` makes `pnpm build` exit 0 on a failed type-check. The rename shipped in the previous commit therefore claimed more than the assertion delivered. Both proposed constraining the tail -- a grammar, or rejecting `[&|;\n]`. Taken instead: pin `build` exactly, as `compile` already is. Two reasons. The stated rationale for exempting it -- "the bundle step's trailing flags are expected to move for ordinary reasons" -- was never measured and is false: `build` has not changed since the initial commit. And a forbidden-character list is a denylist, while this file's entire history is denylists missing a case; that one would still admit `>`, `$(…)`, backticks and `#`. An exact match has no such gap. The cost is deliberate and now documented: changing `build` requires updating this string, the same contract `compile` carries. Verified by 13 mutations: both reported bypasses red, the three metacharacters a denylist would have missed (`>`, `$(…)`, `#`) red, all cycle-4 regressions still red, and a JSONC comment still green. --- test/build/compile-chain.test.ts | 37 ++++++++++++++++---------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/test/build/compile-chain.test.ts b/test/build/compile-chain.test.ts index 2475676c..8d442ec5 100644 --- a/test/build/compile-chain.test.ts +++ b/test/build/compile-chain.test.ts @@ -88,28 +88,27 @@ describe("pnpm compile project chain", () => { it("reaches the bundle step only through both type-check gates", () => { // `build` emitting dist/ without the gates would restore the same silent // green: the bundle would ship from source no compiler ever looked at. - // BOTH gates are pinned — `compile` covers the host and test programs, and + // BOTH gates matter — `compile` covers the host and test programs, and // `compile:webview` is the only tsc pass over src/webview, which lands in - // the same dist/webview/ output. Pinning `compile` alone left the webview - // half free to drift behind esbuild while this test stayed green. + // the same dist/webview/ output. // - // Assert the required PREFIX rather than splitting into steps and checking - // order. Splitting on `&&` cannot see shell short-circuiting, so it accepts - // `pnpm compile && pnpm compile:webview && false || node esbuild…`: both - // gates precede the bundle-bearing segment, yet `||` runs the bundler when - // a gate fails. A prefix pin models what actually matters — the bundler is - // reachable only through two successful `&&` gates. + // Pinned by exact equality, like `compile`, because every weaker shape + // tried here has been measurably bypassable. Splitting on `&&` and + // comparing indices cannot see short-circuiting, so it accepted + // `… && false || node esbuild…`. Pinning only the leading prefix left the + // tail free, so it accepted `… --production || node esbuild…` (bundles + // ungated when a gate fails) and `… --production || true` (exits 0 on a + // failed type-check). The obvious next patch — reject `[&|;\n]` in the + // tail — is a denylist, and this file's whole history is denylists missing + // a case: it would still admit `>`, `$(…)`, backticks and `#`. An exact + // match has no such gap. // - // The prefix stops at the bundler's name on purpose: `build` is still - // deliberately NOT pinned by exact equality, because the bundle step's - // trailing flags are expected to move for ordinary reasons and only the - // gating prefix is load-bearing. - const REQUIRED_BUILD_PREFIX = "pnpm compile && pnpm compile:webview && node esbuild.config.mjs"; - const buildScript: string = pkg.scripts.build; - // Compare a slice rather than asserting `startsWith(…)` is true: on failure - // this reports the actual leading text against the expected prefix, where - // the boolean form reports only `expected false to be true`. - expect(buildScript.slice(0, REQUIRED_BUILD_PREFIX.length)).toBe(REQUIRED_BUILD_PREFIX); + // The cost is deliberate: changing `build` now requires updating this + // string. That is the same contract `compile` already carries, and the + // measured price is nil — `build` has not changed since the initial commit. + expect(pkg.scripts.build).toBe( + "pnpm compile && pnpm compile:webview && node esbuild.config.mjs --production" + ); }); it("keeps compile:webview a real tsc pass", () => { From 99682b90f5916a26f264e21fa53818c858a6f18d Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sun, 23 Aug 2026 03:13:48 +1000 Subject: [PATCH 12/16] test(build): restore the vacuity episode the exact-pin rewrite compressed away Rewriting the gate test for exact equality shortened "pinning `compile` alone left the webview half free to drift behind esbuild while this test stayed green" to "BOTH gates matter", dropping the measured episode that explains why the second gate is pinned at all. Restored as a trailing clause, so the paragraph does not grow. Compressing a load-bearing fact into a shorter sentence is how three of this file's four vacuities were introduced; doing it while fixing the fourth was not the intended irony. --- test/build/compile-chain.test.ts | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/test/build/compile-chain.test.ts b/test/build/compile-chain.test.ts index 8d442ec5..39be4603 100644 --- a/test/build/compile-chain.test.ts +++ b/test/build/compile-chain.test.ts @@ -88,24 +88,25 @@ describe("pnpm compile project chain", () => { it("reaches the bundle step only through both type-check gates", () => { // `build` emitting dist/ without the gates would restore the same silent // green: the bundle would ship from source no compiler ever looked at. - // BOTH gates matter — `compile` covers the host and test programs, and - // `compile:webview` is the only tsc pass over src/webview, which lands in - // the same dist/webview/ output. + // BOTH gates are load-bearing — `compile` covers the host and test + // programs, and `compile:webview` is the only tsc pass over src/webview, + // which lands in the same dist/webview/ output; pinning `compile` alone + // once left that half free to drift while this test stayed green. // // Pinned by exact equality, like `compile`, because every weaker shape - // tried here has been measurably bypassable. Splitting on `&&` and - // comparing indices cannot see short-circuiting, so it accepted + // tried here turned out to be bypassable. Splitting on `&&` and comparing + // indices cannot see short-circuiting, so it accepted // `… && false || node esbuild…`. Pinning only the leading prefix left the // tail free, so it accepted `… --production || node esbuild…` (bundles // ungated when a gate fails) and `… --production || true` (exits 0 on a // failed type-check). The obvious next patch — reject `[&|;\n]` in the // tail — is a denylist, and this file's whole history is denylists missing - // a case: it would still admit `>`, `$(…)`, backticks and `#`. An exact - // match has no such gap. + // a case: it would still admit `>`, `$(…)`, backticks and `#`. Exact + // equality has no such gap. // // The cost is deliberate: changing `build` now requires updating this - // string. That is the same contract `compile` already carries, and the - // measured price is nil — `build` has not changed since the initial commit. + // string. `compile` already carries that same contract, and the measured + // price is nil — `build` has not changed since the initial commit. expect(pkg.scripts.build).toBe( "pnpm compile && pnpm compile:webview && node esbuild.config.mjs --production" ); From f24e7286cc22e17acf877b05886feaf07d3c0d18 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sun, 23 Aug 2026 03:23:03 +1000 Subject: [PATCH 13/16] test(build): correct the compile:webview claim and drop a single-use local "compile:webview is the only tsc pass over src/webview" is false: test/webview and test/webview-browser both include ../../src/webview/**/*.ts, so `compile` already compiles that source twice. What is unique is the ambient view -- compile:webview is the only pass using `types: ["vscode-webview"]` with no node, which is what catches webview code reaching for a Node global. The history claim was overstated for the same reason and is narrowed to the check that actually drifted. Also inlines `pkg.scripts.compile`, matching the other two script pins. --- test/build/compile-chain.test.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test/build/compile-chain.test.ts b/test/build/compile-chain.test.ts index 39be4603..28c120db 100644 --- a/test/build/compile-chain.test.ts +++ b/test/build/compile-chain.test.ts @@ -54,7 +54,6 @@ const parseJsonc = (source: string) => describe("pnpm compile project chain", () => { it("type-checks exactly the five reviewed projects, &&-chained with nothing else", () => { - const compileScript: string = pkg.scripts.compile; // Pin the WHOLE script by exact equality rather than extracting its `-p` // arguments. A partial extraction never sees what sits between and around // the steps, so all of these stay green while the gate is dead: @@ -63,7 +62,7 @@ describe("pnpm compile project chain", () => { // `echo` in front of a step. Substring matching is weaker still — a bare // `toContain("tsc -p ./")` is satisfied by any of the four nested projects, // so the root pin would never go red. - expect(compileScript).toBe(CHAINED_PROJECTS.map((p) => `tsc -p ${p}`).join(" && ")); + expect(pkg.scripts.compile).toBe(CHAINED_PROJECTS.map((p) => `tsc -p ${p}`).join(" && ")); }); // The scoped claim in src/shared/quoll-perf-flag.d.ts names this program as @@ -88,10 +87,14 @@ describe("pnpm compile project chain", () => { it("reaches the bundle step only through both type-check gates", () => { // `build` emitting dist/ without the gates would restore the same silent // green: the bundle would ship from source no compiler ever looked at. - // BOTH gates are load-bearing — `compile` covers the host and test - // programs, and `compile:webview` is the only tsc pass over src/webview, - // which lands in the same dist/webview/ output; pinning `compile` alone - // once left that half free to drift while this test stayed green. + // BOTH gates are load-bearing. `compile` covers the host and test + // programs — including two that DO compile src/webview (test/webview, + // test/webview-browser), so `compile:webview` is not the only tsc pass + // over that source. It is the only one that checks it under the ambient + // view it actually ships with (`types: ["vscode-webview"]`, no node), + // which is what catches webview code reaching for a Node global; the + // others widen or empty `types`. Pinning `compile` alone once left that + // check free to drift while this test stayed green. // // Pinned by exact equality, like `compile`, because every weaker shape // tried here turned out to be bypassable. Splitting on `&&` and comparing From f97448de7d053e417d994da479c49194039668f4 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sun, 23 Aug 2026 03:25:27 +1000 Subject: [PATCH 14/16] docs(build): record what the compile-chain tripwire does not cover The adversarial review measured the boundary rather than assuming it, so write it down where a reader will hit it: the chain is pinned, the contents of each link are not, the test's own execution path is not, and dist/ is still reachable outside the gate by design. A tripwire that is trusted for more than it checks is how the original bug survived -- test/build/ sat outside every tsconfig for months while its @ts-expect-error pins read as enforcement. The per-project content gap is filed as a follow-up; it needs a design, not another assertion. --- test/build/compile-chain.test.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/build/compile-chain.test.ts b/test/build/compile-chain.test.ts index 28c120db..cfbb398f 100644 --- a/test/build/compile-chain.test.ts +++ b/test/build/compile-chain.test.ts @@ -19,6 +19,17 @@ // `compile:webview` script, and test/extension/tsconfig.json is the E2E emit // program driven by `pnpm test:e2e:run`. A derived list would quietly adopt // whatever config lands next instead of forcing that call to be reviewed. +// +// What this does NOT cover — measured, so nobody assumes wider protection than +// exists. It pins the CHAIN, not the contents of each link: shrinking a +// project's `include`, setting `noCheck`, or flipping `strict` in +// tsconfig.base.json all leave these four assertions green (the E2E `include` +// check below is the one exception, and only for that config). Nor does it pin +// its own execution path — the vitest `include` and the CI wiring that runs +// `pnpm test` could stop invoking this file. And dist/ can still be produced +// outside the gate by `pnpm watch` or a direct esbuild call, which is by +// design; the release path is safe only because ci.yml and publish.yml both go +// through `pnpm build`, and that wiring is outside this file's guarantee. import { readFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; import { describe, expect, it } from "vitest"; From 2d825fc0da65e5b66106502d78652f7e7315aae5 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sun, 23 Aug 2026 03:37:21 +1000 Subject: [PATCH 15/16] test(build): delete three unmeasured mechanism claims from the comments Cycle 7 found the same claim corrected in one place and left standing 24 lines below ("the ONLY tsc pass over src/webview"), the correction ITSELF still overclaiming, and the new boundary note granting protection it does not have. The gate-2 rationale is now measured rather than reasoned: a `process.cwd()` probe in src/webview/shell.ts reddens compile:webview AND test/webview-browser but not test/webview, so the browser program already subsumes what compile:webview checks. The gate is pinned as defence in depth against an include-shrink nothing pins -- not because it uniquely catches Node globals, which was the cited mechanism and is false. The boundary note claimed the E2E include check was "the one exception" to shrink/noCheck/strict; it is not an exception to that list at all -- it fires when that config GAINS a src path. And CI runs `pnpm test:unit`, never the composite `pnpm test`. Every one of the five comment defects across this review has the same shape: an unmeasured explanation of WHY something matters, wrapped around an assertion that was correct. These edits remove mechanism claims rather than adding more. --- test/build/compile-chain.test.ts | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/test/build/compile-chain.test.ts b/test/build/compile-chain.test.ts index cfbb398f..9e205832 100644 --- a/test/build/compile-chain.test.ts +++ b/test/build/compile-chain.test.ts @@ -23,10 +23,11 @@ // What this does NOT cover — measured, so nobody assumes wider protection than // exists. It pins the CHAIN, not the contents of each link: shrinking a // project's `include`, setting `noCheck`, or flipping `strict` in -// tsconfig.base.json all leave these four assertions green (the E2E `include` -// check below is the one exception, and only for that config). Nor does it pin -// its own execution path — the vitest `include` and the CI wiring that runs -// `pnpm test` could stop invoking this file. And dist/ can still be produced +// tsconfig.base.json all leave these four assertions green — measured. The E2E +// `include` check below is not an exception to that list: it fires when that +// config GAINS a src path, not when any project's checking is weakened. Nor +// does this file pin its own execution path — the vitest `include` and the CI +// step that runs `pnpm test:unit` could stop invoking it. And dist/ can still be produced // outside the gate by `pnpm watch` or a direct esbuild call, which is by // design; the release path is safe only because ci.yml and publish.yml both go // through `pnpm build`, and that wiring is outside this file's guarantee. @@ -98,14 +99,14 @@ describe("pnpm compile project chain", () => { it("reaches the bundle step only through both type-check gates", () => { // `build` emitting dist/ without the gates would restore the same silent // green: the bundle would ship from source no compiler ever looked at. - // BOTH gates are load-bearing. `compile` covers the host and test - // programs — including two that DO compile src/webview (test/webview, - // test/webview-browser), so `compile:webview` is not the only tsc pass - // over that source. It is the only one that checks it under the ambient - // view it actually ships with (`types: ["vscode-webview"]`, no node), - // which is what catches webview code reaching for a Node global; the - // others widen or empty `types`. Pinning `compile` alone once left that - // check free to drift while this test stayed green. + // BOTH gates are pinned, though only `compile` is load-bearing TODAY: + // test/webview-browser already compiles src/webview with a superset + // include and an emptied `types`, so it currently subsumes what + // `compile:webview` checks (measured — a Node-global probe in + // src/webview/shell.ts reddens both). `compile:webview` is pinned as + // defence in depth: that subsumption rests on the browser project's + // `include`, which nothing here pins (see the boundary note at the top). + // Do not "simplify" this gate away on the strength of today's overlap. // // Pinned by exact equality, like `compile`, because every weaker shape // tried here turned out to be bypassable. Splitting on `&&` and comparing @@ -129,9 +130,10 @@ describe("pnpm compile project chain", () => { it("keeps compile:webview a real tsc pass", () => { // `build` calling `compile:webview` means nothing if that script stops // type-checking: `"echo skip"` or a trailing `|| true` leaves every other - // assertion here green while the ONLY tsc pass over src/webview disappears - // — the same accident this file exists to catch, one level down. The body - // is pinned exactly, matching the treatment `compile` already gets. + // assertion here green while a tsc pass over src/webview disappears — the + // same accident this file exists to catch, one level down. (It is not the + // only such pass today; see the gate comment above for why it is pinned + // anyway.) The body is pinned exactly, as `compile` already is. expect(pkg.scripts["compile:webview"]).toBe("tsc -p ./src/webview"); }); }); From b49c528bcea6e81af01d5f7b21e49411c9f49c40 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Sun, 23 Aug 2026 09:57:00 +1000 Subject: [PATCH 16/16] test(build): rewrap the boundary block and drop a duplicated word Cosmetic only. One comment line ran to 90 columns against the file's 81-column maximum, and "measured" appeared twice in the same paragraph -- the blanket lead-in and, three lines later, the scoped attribution naming which three claims were measured. Kept the scoped one. No assertion, test name or fact claim touched. --- test/build/compile-chain.test.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/test/build/compile-chain.test.ts b/test/build/compile-chain.test.ts index 9e205832..e0ea3dda 100644 --- a/test/build/compile-chain.test.ts +++ b/test/build/compile-chain.test.ts @@ -20,17 +20,18 @@ // program driven by `pnpm test:e2e:run`. A derived list would quietly adopt // whatever config lands next instead of forcing that call to be reviewed. // -// What this does NOT cover — measured, so nobody assumes wider protection than -// exists. It pins the CHAIN, not the contents of each link: shrinking a +// What this does NOT cover — written down so nobody assumes wider protection +// than exists. It pins the CHAIN, not the contents of each link: shrinking a // project's `include`, setting `noCheck`, or flipping `strict` in // tsconfig.base.json all leave these four assertions green — measured. The E2E // `include` check below is not an exception to that list: it fires when that // config GAINS a src path, not when any project's checking is weakened. Nor // does this file pin its own execution path — the vitest `include` and the CI -// step that runs `pnpm test:unit` could stop invoking it. And dist/ can still be produced -// outside the gate by `pnpm watch` or a direct esbuild call, which is by -// design; the release path is safe only because ci.yml and publish.yml both go -// through `pnpm build`, and that wiring is outside this file's guarantee. +// step that runs `pnpm test:unit` could stop invoking it. And dist/ can still +// be produced outside the gate by `pnpm watch` or a direct esbuild call, which +// is by design; the release path is safe only because ci.yml and publish.yml +// both go through `pnpm build`, and that wiring is outside this file's +// guarantee. import { readFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; import { describe, expect, it } from "vitest";