diff --git a/.github/scripts/auto-fix-steps.ts b/.github/scripts/auto-fix-steps.ts index e04a57c3..f08f6f97 100644 --- a/.github/scripts/auto-fix-steps.ts +++ b/.github/scripts/auto-fix-steps.ts @@ -22,7 +22,7 @@ import { dispatch } from "./lib/entry.js"; import { configureBotIdentity, leasePush } from "./lib/pr-branch.js"; import { capture, requireEnv, run, setOutput, status } from "./lib/workflow-step.js"; -/** In generated.ts table order, each generator after its sources: the descriptor-derived files and the gaps index are imported through src/, and the inputs table reads action.yml. */ +/** In generated.ts table order, each generator after its sources: the descriptor-derived files and the gaps index are imported through src/, and the inputs table reads INPUT_DECLS from src/flows/inputs.ts. */ export const GENERATORS = [ "build:openapi", "build:gaps-index", diff --git a/.github/scripts/package-smoke.ts b/.github/scripts/package-smoke.ts index 049b918c..0c52c45f 100644 --- a/.github/scripts/package-smoke.ts +++ b/.github/scripts/package-smoke.ts @@ -157,16 +157,28 @@ export function packedTarball(packJson: string, destination: string): string { return join(destination, filename); } +/** + * `npm pack` arguments under which stdout is the JSON alone. --ignore-scripts skips the package's `prepare` script + * from npm 11 on; npm 10 runs it regardless, in the foreground by default, so --foreground-scripts=false is what + * keeps the script's output (lefthook's sync line, in a checkout with the hooks installed) off stdout there. + */ +export function packArguments(destination: string): string[] { + return [ + "pack", + "--json", + "--ignore-scripts", + "--foreground-scripts=false", + "--pack-destination", + destination, + ]; +} + async function main(): Promise { run("bun", ["run", "build:schema"], REPO_ROOT); run("bun", ["run", "build:lib"], REPO_ROOT); run("bun", ["run", "lint:package"], REPO_ROOT); await withSmokeDirs("gsac-smoke-", ({ pack, consumer }) => { - // --ignore-scripts keeps the prepare hook's output out of the JSON stdout. - const tarball = packedTarball( - capture("npm", ["pack", "--json", "--ignore-scripts", "--pack-destination", pack], REPO_ROOT), - pack, - ); + const tarball = packedTarball(capture("npm", packArguments(pack), REPO_ROOT), pack); writeFileSync( join(consumer, "package.json"), `${JSON.stringify({ name: "smoke-consumer", private: true, type: "module" }, null, 2)}\n`, diff --git a/.github/workflows/auto-fix.yml b/.github/workflows/auto-fix.yml index 867a2d8a..7e6128f0 100644 --- a/.github/workflows/auto-fix.yml +++ b/.github/workflows/auto-fix.yml @@ -37,6 +37,8 @@ on: - "architecture.yml" - "docs/operate/check-mode.md" - "docs/start/getting-started.md" + - "docs/sections/*.docs.yml" + - "docs/schema.docs.yml" - ".github/scripts/gen-action-docs.ts" - ".github/scripts/gen-inputs-table.ts" - "docs/reference/coverage.md" @@ -55,6 +57,11 @@ on: - ".github/scripts/endpoint-docs.ts" - ".github/scripts/endpoint-docs.yml" - "test/e2e/openapi/validate.ts" + - "test/e2e/openapi/paths.ts" + # The formatter's config: every generator but the gaps index pipes its output through biome. Listed by hand: the + # trigger test (test/scripts/auto-fix-steps.test.ts) follows what a generator imports and loads, not what a subprocess opens. + - "biome.json" + - "biome.local.jsonc" permissions: contents: read diff --git a/architecture.yml b/architecture.yml index 7261d2da..f395456e 100644 --- a/architecture.yml +++ b/architecture.yml @@ -47,4 +47,3 @@ edges: report: [discovery, engine, github, io, private, problem, schema, sections, types] problem: [plain-data, schema, text] schema: [sections, types] - upstream-gaps: [types] diff --git a/docs/reference/architecture.md b/docs/reference/architecture.md index c9674895..c8bbb6c7 100644 --- a/docs/reference/architecture.md +++ b/docs/reference/architecture.md @@ -400,7 +400,6 @@ graph TD problem --> text schema --> sections schema --> types - upstream_gaps --> types ``` diff --git a/src/upstream-gaps/gap.ts b/src/upstream-gaps/gap.ts index 52edd696..0bcf2a5c 100644 --- a/src/upstream-gaps/gap.ts +++ b/src/upstream-gaps/gap.ts @@ -3,7 +3,7 @@ import type { Endpoints } from "@octokit/types"; /** - * Routes the pinned @octokit/types does not carry yet. The gap file carries a MustBeNever tripwire that + * Routes the pinned @octokit/types does not carry yet. The gap file carries an OctokitLacks tripwire that * fails typecheck when octokit ships a route; the graduate script then deletes the file * (documentedInSpec: true) or rewrites it spec-only (documentedInSpec: false). `documentedInSpec` is false * only for features GitHub's OpenAPI descriptor ALSO lags; those are excluded from the descriptor slice the @@ -48,6 +48,18 @@ export function defineGap>( return { ...gap, kind: "octokit" }; } +/** + * The tripwire a defineGap file declares over its own GAP: `type _X = OctokitLacks`. The constraint + * admits only routes the pinned @octokit/types lacks, so once octokit ships one, TS2344 fires inside that gap + * file: the one diagnostic the graduate script acts on. + * + * A gap file imports nothing but this file, so no layer edge in architecture.yml hangs on which gap files + * exist, and a graduation cannot leave one stale. + */ +export type OctokitLacks< + G extends { readonly routes: readonly Exclude[] }, +> = G; + /** * Routes are constrained to keyof Endpoints (octokit already ships them), so a typo or a not-yet-shipped * route fails here instead of as a missing SupplementalRoute downstream. diff --git a/src/upstream-gaps/lfs.ts b/src/upstream-gaps/lfs.ts index 024b4308..2d88004e 100644 --- a/src/upstream-gaps/lfs.ts +++ b/src/upstream-gaps/lfs.ts @@ -1,6 +1,4 @@ -import type { Endpoints } from "@octokit/types"; -import type { MustBeNever } from "../types.js"; -import { defineGap } from "./gap.js"; +import { defineGap, type OctokitLacks } from "./gap.js"; /** GitHub shipped the Git LFS repository toggle; @octokit/types does not carry these routes yet, nor does the published OpenAPI description. */ export const GAP = defineGap({ @@ -9,6 +7,4 @@ export const GAP = defineGap({ }); /** Fires when @octokit/types gains any of these routes: the graduate script retires this file per its documentedInSpec lifecycle. */ -type _GraduateThisFileOnceOctokitShipsIt = MustBeNever< - Extract<(typeof GAP.routes)[number], keyof Endpoints> ->; +type _GraduateThisFileOnceOctokitShipsIt = OctokitLacks; diff --git a/test/library/package-smoke.test.ts b/test/library/package-smoke.test.ts index d342de5e..01ee4a3c 100644 --- a/test/library/package-smoke.test.ts +++ b/test/library/package-smoke.test.ts @@ -1,7 +1,16 @@ import { describe, expect, test } from "bun:test"; -import { existsSync, writeFileSync } from "node:fs"; +import { execFileSync } from "node:child_process"; +import { existsSync, mkdirSync, writeFileSync } from "node:fs"; import { dirname, join } from "node:path"; -import { packedTarball, withSmokeDirs } from "../../.github/scripts/package-smoke.js"; +import { + packArguments, + packedTarball, + withSmokeDirs, +} from "../../.github/scripts/package-smoke.js"; +import { withTempDir } from "../temp-dir.js"; + +/** Room for the `npm pack` below: 1 to 3 s as a rule, and past bun's 5 s default on a slow runner. */ +const NPM_PACK_TIMEOUT = 30_000; describe("package smoke helpers", () => { test("withSmokeDirs hands out pack/ and consumer/ under one root and removes the root after the body", async () => { @@ -40,6 +49,36 @@ describe("package smoke helpers", () => { ).toBe("/dest/vivswan-github-settings-as-code-2.0.0.tgz"); }); + test( + "npm pack under the smoke's arguments prints the JSON alone for a package whose prepare script prints", + () => + withTempDir("gsac-pack-test-", (dir) => { + // Without --foreground-scripts=false an npm that runs `prepare` under --ignore-scripts (npm 10) puts what the + // script prints ahead of the JSON. CI packs with npm 11, which skips prepare, so its smoke job cannot see it. + const pkg = join(dir, "pkg"); + const destination = join(dir, "out"); + mkdirSync(pkg); + mkdirSync(destination); + writeFileSync( + join(pkg, "package.json"), + JSON.stringify({ + name: "prints-on-prepare", + version: "1.0.0", + scripts: { prepare: "echo sync hooks" }, + }), + ); + const stdout = execFileSync("npm", packArguments(destination), { + cwd: pkg, + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); + const tarball = packedTarball(stdout, destination); + expect(tarball).toBe(join(destination, "prints-on-prepare-1.0.0.tgz")); + expect(existsSync(tarball)).toBe(true); + }), + NPM_PACK_TIMEOUT, + ); + test("packedTarball rejects anything but exactly one tarball", () => { for (const output of [ "[]", diff --git a/test/scripts/auto-fix-steps.test.ts b/test/scripts/auto-fix-steps.test.ts index 9a270e9b..510527b0 100644 --- a/test/scripts/auto-fix-steps.test.ts +++ b/test/scripts/auto-fix-steps.test.ts @@ -8,7 +8,7 @@ import { describe, expect, setDefaultTimeout, test } from "bun:test"; import { execFileSync } from "node:child_process"; import { readFileSync } from "node:fs"; -import { join } from "node:path"; +import { join, relative, resolve } from "node:path"; import { parse } from "yaml"; import { FIX_OWNED_PATHS, @@ -21,6 +21,7 @@ import { generatorEntryPoint, generatorScripts, } from "../../.github/scripts/generated.js"; +import { SCHEMA_DESCRIPTIONS } from "../../src/sections/docs-registry.js"; import { ROOT } from "../root.js"; import { clone, @@ -62,18 +63,40 @@ describe("the fix-owned paths and generators track the generated-output table", expect(generatorScripts()).toEqual([...GENERATORS]); }); + const generatorFiles = generatorScripts().map((name) => + generatorEntryPoint(packageScripts[name] ?? ""), + ); + const triggersTheFix = (path: string): boolean => + workflow.on.pull_request.paths.some((pattern) => new Bun.Glob(pattern).match(path)); + test("a hand edit to a generated output or its generator triggers the fix", () => { - const files = generatorScripts().map((name) => generatorEntryPoint(packageScripts[name] ?? "")); - for (const file of files) { + for (const file of generatorFiles) { expect(file).not.toBeNull(); } - for (const path of [...generatedPaths(), ...files.map((file) => file ?? "")]) { - expect( - workflow.on.pull_request.paths.some((pattern) => new Bun.Glob(pattern).match(path)), - `on.paths: ${path}`, - ).toBe(true); + for (const path of [...generatedPaths(), ...generatorFiles.map((file) => file ?? "")]) { + expect(triggersTheFix(path), `on.paths: ${path}`).toBe(true); } }); + + test("an edit to a module a generator runs or to a docs file the registry loads triggers the fix", async () => { + // Without this, a generator input outside src/ changes with no fix run: a docs-only pull request leaves the + // generated page stale and fails on the drift test. The bundler names the modules whose code reaches a generator. + const bundle = await Bun.build({ + entrypoints: generatorFiles.map((file) => join(ROOT, file ?? "")), + root: ROOT, + target: "bun", + packages: "external", + metafile: true, + }); + const modules = Object.keys(bundle.metafile?.inputs ?? {}).map((path) => + relative(ROOT, resolve(path)), + ); + const docs = [...new Set(SCHEMA_DESCRIPTIONS.map((entry) => entry.source))]; + // Controls: a census that came back empty or partial would pass with nothing checked. + expect(modules).toContain("src/sections/labels/index.ts"); + expect(docs).toContain("docs/sections/labels.docs.yml"); + expect([...modules, ...docs].filter((path) => !triggersTheFix(path))).toEqual([]); + }); }); const GENERATOR_ENV: Record<(typeof GENERATORS)[number], string> = { diff --git a/test/scripts/graduate-upstream-gaps.test.ts b/test/scripts/graduate-upstream-gaps.test.ts index e3866aa3..f1ee560d 100644 --- a/test/scripts/graduate-upstream-gaps.test.ts +++ b/test/scripts/graduate-upstream-gaps.test.ts @@ -2,15 +2,18 @@ import { describe, expect, test } from "bun:test"; import { spawnSync } from "node:child_process"; import { copyFileSync, + cpSync, existsSync, mkdirSync, readdirSync, readFileSync, + rmSync, symlinkSync, writeFileSync, } from "node:fs"; import { join } from "node:path"; -import { gapFileBases, generateIndex } from "../../.github/scripts/gen-gaps-index.js"; +import { ARCHITECTURE_PATH, lintArchitecture } from "../../.github/scripts/arch-lint.js"; +import { gapFileBases, generateIndex, INDEX_PATH } from "../../.github/scripts/gen-gaps-index.js"; import { isGapFile, isSpecOnly, @@ -18,7 +21,7 @@ import { planGraduation, toSpecOnlyGapSource, } from "../../.github/scripts/graduate-upstream-gaps.js"; -import type { Diagnostic } from "../../.github/scripts/lib/tsc-diagnostics.js"; +import { type Diagnostic, parseDiagnostics } from "../../.github/scripts/lib/tsc-diagnostics.js"; import type { UnshippedGraphqlSdl } from "../../src/upstream-gaps/gap.js"; import { ROOT } from "../root.js"; import { withTempDir } from "../temp-dir.js"; @@ -26,6 +29,24 @@ import { withTempDir } from "../temp-dir.js"; const TRIPWIRE_MESSAGE = "Type '\"GET /repos/{owner}/{repo}/merge-queue\"' does not satisfy the constraint 'never'."; +/** Compiles the gaps index under `dir` and what it imports, with the repository's compiler options and the one line + * per diagnostic the graduate script reads. */ +function compileIndex(dir: string): { status: number | null; output: string } { + writeFileSync( + join(dir, "tsconfig.json"), + JSON.stringify({ extends: join(ROOT, "tsconfig.json"), include: [], files: [INDEX_PATH] }), + ); + const tsc = spawnSync( + join(ROOT, "node_modules", ".bin", "tsc"), + ["-p", dir, "--pretty", "false"], + { cwd: dir, encoding: "utf8" }, + ); + if (tsc.error) { + throw tsc.error; + } + return { status: tsc.status, output: tsc.stdout + tsc.stderr }; +} + describe("isGapFile", () => { // Only a .ts file directly under src/upstream-gaps/ is a gap. gap.ts and the generated index carry no tripwire; // a TS2344 in either means the machinery broke, and deleting it could never be the fix. Declaration and test @@ -125,10 +146,10 @@ describe("generateIndex", () => { "the index generated for %s type-checks beside its gap files, loads, and its SDL entries name them", (_label, bases) => withTempDir("gaps-index-", async (dir) => { - // A src/ mirror: every sibling of upstream-gaps/ and generated/ symlinked (a gap may import ../types.js), - // the gap files copied, the generated index written at its own path. The empty row matters on its own: the - // committed index compiles under the project typecheck only while a gap file exists, so a derivation written - // for a populated GAPS (say `(typeof GAPS)["lfs"]`) would first break the day the last gap graduates. + // A src/ mirror: every sibling of upstream-gaps/ and generated/ symlinked, the gap files copied, the generated + // index written at its own path. The empty row matters on its own: the committed index compiles under the + // project typecheck only while a gap file exists, so a derivation written for a populated GAPS (say + // `(typeof GAPS)["lfs"]`) would first break the day the last gap graduates. const gapsDir = join(dir, "src", "upstream-gaps"); const index = join(dir, "src", "generated", "upstream-gaps.ts"); mkdirSync(gapsDir, { recursive: true }); @@ -143,24 +164,7 @@ describe("generateIndex", () => { copyFileSync(join(GAPS_DIR, `${file}.ts`), join(gapsDir, `${file}.ts`)); } writeFileSync(index, generateIndex(bases)); - writeFileSync( - join(dir, "tsconfig.json"), - JSON.stringify({ - extends: join(ROOT, "tsconfig.json"), - include: [], - files: ["src/generated/upstream-gaps.ts"], - }), - ); - const tsc = spawnSync( - join(ROOT, "node_modules", ".bin", "tsc"), - ["-p", dir, "--pretty", "false"], - { cwd: dir, encoding: "utf8" }, - ); - if (tsc.error) { - throw tsc.error; - } - expect(tsc.stdout + tsc.stderr).toBe(""); - expect(tsc.status).toBe(0); + expect(compileIndex(dir)).toEqual({ status: 0, output: "" }); // Loading resolves every import. The GAPS key, not the import, is what names a gap file downstream: // unshippedGraphqlSdl() renders it as the file to retire, and a camelCase key would compile yet name a // file that does not exist. @@ -325,3 +329,85 @@ describe("spec-only sources never reach the deletion branch", () => { expect(isSpecOnly(source)).toBe(specOnly); }); }); + +describe("a graduation on a copy of src/", () => { + /** `body` over a scratch root holding what a graduation rewrites and what judges it: a copy of src/ and of + * architecture.yml, with node_modules linked for the compiler. */ + const withSrcCopy = (body: (dir: string) => T): Promise => + withTempDir("gaps-graduation-", (dir) => { + cpSync(join(ROOT, "src"), join(dir, "src"), { recursive: true }); + copyFileSync(join(ROOT, ARCHITECTURE_PATH), join(dir, ARCHITECTURE_PATH)); + symlinkSync(join(ROOT, "node_modules"), join(dir, "node_modules"), "dir"); + return body(dir); + }); + + const gapPath = (dir: string, base: string): string => + join(dir, "src", "upstream-gaps", `${base}.ts`); + const gapBases = (dir: string): string[] => + gapFileBases(readdirSync(join(dir, "src", "upstream-gaps"))); + const regenerateIndex = (dir: string): void => + writeFileSync(join(dir, INDEX_PATH), generateIndex(gapBases(dir))); + + // Without these rows a gap file that alone draws a declared edge passes every check until the day it graduates: + // the fix commit then leaves a stale allowance in architecture.yml, a file the bot may not write. + test.each<[state: string, graduate: (dir: string) => void]>([ + ["no gap file touched (control)", () => undefined], + [ + "every spec-pinned gap rewritten to spec-only", + (dir) => { + for (const base of gapBases(dir)) { + const source = readFileSync(gapPath(dir, base), "utf8"); + if (isSpecPinned(source)) { + writeFileSync( + gapPath(dir, base), + toSpecOnlyGapSource(source, `src/upstream-gaps/${base}.ts`), + ); + } + } + }, + ], + [ + "every gap file deleted", + (dir) => { + for (const base of gapBases(dir)) { + rmSync(gapPath(dir, base)); + } + }, + ], + ])("with %s, architecture.yml declares exactly the imports src/ draws", (_state, graduate) => + withSrcCopy((dir) => { + graduate(dir); + regenerateIndex(dir); + expect(lintArchitecture(dir)).toEqual([]); + }), + ); + + /** A spec-pinned gap over a route the pinned @octokit/types already carries: any gap file, the day octokit catches up. */ + const SHIPPED_GAP = [ + 'import { defineGap, type OctokitLacks } from "./gap.js";', + "", + "/** GitHub shipped the repository read; @octokit/types does not carry this route yet, nor does the published OpenAPI description. */", + "export const GAP = defineGap({", + ' routes: ["GET /repos/{owner}/{repo}"],', + " documentedInSpec: false,", + "});", + "", + "type _GraduateThisFileOnceOctokitShipsIt = OctokitLacks;", + "", + ].join("\n"); + + test("a gap whose route octokit ships fires TS2344 in its own file, plans as a graduation, and its rewrite compiles with lint:arch green", () => + withSrcCopy((dir) => { + // The control the tripwire's constraint needs: one that stopped firing would keep every gap forever, build green. + const gapFile = "src/upstream-gaps/shipped.ts"; + writeFileSync(join(dir, gapFile), SHIPPED_GAP); + regenerateIndex(dir); + const { diagnostics, unparsed } = parseDiagnostics(compileIndex(dir).output); + expect(unparsed).toEqual([]); + expect(planGraduation(diagnostics)).toEqual({ gapFiles: [gapFile], foreign: [] }); + writeFileSync(join(dir, gapFile), toSpecOnlyGapSource(SHIPPED_GAP, gapFile)); + regenerateIndex(dir); + expect(compileIndex(dir)).toEqual({ status: 0, output: "" }); + expect(lintArchitecture(dir)).toEqual([]); + })); +});