diff --git a/package.json b/package.json index 26eecf6f6..8aed08f85 100644 --- a/package.json +++ b/package.json @@ -92,6 +92,7 @@ "dead-source-files:check": "node --experimental-strip-types scripts/check-dead-source-files.ts", "dead-exports:check": "node --experimental-strip-types scripts/check-dead-exports.ts", "publishable-deps:check": "node --experimental-strip-types scripts/check-publishable-deps.ts", + "focus-manifest-fields:check": "node --experimental-strip-types scripts/check-focus-manifest-fields.ts", "release-commit-parsing:check": "tsx scripts/check-release-commit-parsing.ts", "releasable-commit-types:check": "tsx scripts/check-releasable-commit-types.ts", "checkers-wired:check": "node --experimental-strip-types scripts/check-checkers-wired.ts", @@ -147,7 +148,7 @@ "test:smoke:browser:install": "playwright install chromium", "test:smoke:browser": "node --experimental-strip-types scripts/smoke-ui-browser.ts", "pretest:ci": "npm run check-node-version", - "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run turbo-inputs:check && npm run workspace-dep-ranges:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run dead-exports:check && npm run publishable-deps:check && npm run fixture-clock-races:check && npm run typecheck-coverage:check && npm run checkers-wired:check && npm run release-commit-parsing:check && npm run releasable-commit-types:check && npm run regate-sort-key:check && npm run maintainer-associations:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", + "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run turbo-inputs:check && npm run workspace-dep-ranges:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run dead-exports:check && npm run publishable-deps:check && npm run fixture-clock-races:check && npm run typecheck-coverage:check && npm run checkers-wired:check && npm run focus-manifest-fields:check && npm run release-commit-parsing:check && npm run releasable-commit-types:check && npm run regate-sort-key:check && npm run maintainer-associations:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", "test:release": "npm run test:ci && npm run changelog:check", "test:release:mcp": "npm run test:ci", "test:watch": "vitest", diff --git a/scripts/check-focus-manifest-fields.ts b/scripts/check-focus-manifest-fields.ts new file mode 100644 index 000000000..e08df750b --- /dev/null +++ b/scripts/check-focus-manifest-fields.ts @@ -0,0 +1,109 @@ +#!/usr/bin/env node +// `FOCUS_MANIFEST_TOP_LEVEL_FIELDS` must equal the keys `parseFocusManifest` actually reads (#9979, item 4 +// of #9860). +// +// The list's own comment says "Every top-level key `parseFocusManifest` below actually reads." That is a +// statement about code twenty lines further down, and nothing enforced it -- the same shape as the +// `turbo.json` inputs comment #9860 calls out as the tell: a snapshot written as though it were a guarantee. +// +// DRIFT IS SILENT IN BOTH DIRECTIONS, and asymmetrically bad, because this list is the single source of +// truth for unknown-field detection (#9065): +// +// • DECLARED BUT NEVER READ -- the parser accepts the field, warns about nothing, and ignores it. An +// operator puts it in `.loopover.yml`, sees no error, and the setting does nothing at all. This is the +// worse direction: it looks configured. +// • READ BUT NOT DECLARED -- the parser consumes the field while simultaneously warning "unknown top-level +// field" about it, and config-lint's `recognizedFieldsFor` leaves it out of the recognized count. A +// working field reported as a mistake. +// +// COMPUTED, NOT RESTATED. There is deliberately no second list here. The declared names are read out of the +// array literal and the used names out of the function body, so this file has nothing to keep in sync and +// cannot itself go stale -- the bar #9853 set and the one #9860 asks for. +// +// A REGEX OVER SOURCE, not the TypeScript AST, and that is a real tradeoff worth stating. The access pattern +// it depends on is uniform and mechanical (`record.` inside one function), and {@link parserReadsAre} +// FAILS LOUDLY if the shape it relies on ever disappears rather than quietly matching nothing -- which is the +// failure mode that would turn this into another guard that reads as coverage while checking nothing. +import { readFileSync } from "node:fs"; +import { pathToFileURL } from "node:url"; + +export const FOCUS_MANIFEST_SOURCE = "packages/loopover-engine/src/focus-manifest.ts"; + +const DECLARATION = /export const FOCUS_MANIFEST_TOP_LEVEL_FIELDS = \[(.*?)\] as const;/s; +const PARSER_START = "export function parseFocusManifest("; + +/** PURE. The names in the exported array literal, in source order. Throws when the declaration is missing -- + * a renamed or deleted constant must break this checker, not silently empty it. */ +export function declaredFields(source: string): string[] { + const match = DECLARATION.exec(source); + if (match?.[1] === undefined) { + throw new Error(`could not find the FOCUS_MANIFEST_TOP_LEVEL_FIELDS declaration in ${FOCUS_MANIFEST_SOURCE}`); + } + return [...match[1].matchAll(/"([^"]+)"/g)].map((entry) => entry[1]!); +} + +/** PURE. The `record.` / `record[""]` names read inside `parseFocusManifest`'s body. + * + * The body is delimited by the next top-level `export ` after the signature: this file declares many parsers + * and reading past the boundary would pull in another function's field names. Throws when either the + * signature or any read is missing, for the reason in the header -- a checker that matches nothing must not + * report success. */ +export function parserReadsAre(source: string): string[] { + const start = source.indexOf(PARSER_START); + if (start === -1) throw new Error(`could not find ${PARSER_START} in ${FOCUS_MANIFEST_SOURCE}`); + const end = source.indexOf("\nexport ", start + PARSER_START.length); + const body = end === -1 ? source.slice(start) : source.slice(start, end); + const names = new Set(); + for (const match of body.matchAll(/\brecord\.([A-Za-z_$][\w$]*)/g)) names.add(match[1]!); + for (const match of body.matchAll(/\brecord\["([^"]+)"\]/g)) names.add(match[1]!); + if (names.size === 0) { + throw new Error("found no `record.` reads in parseFocusManifest -- the access pattern this checker relies on has changed"); + } + return [...names].sort(); +} + +export type FieldDrift = { declaredNotRead: string[]; readNotDeclared: string[] }; + +/** PURE. The two-way comparison. */ +export function findFieldDrift(declared: readonly string[], read: readonly string[]): FieldDrift { + const readSet = new Set(read); + const declaredSet = new Set(declared); + return { + declaredNotRead: declared.filter((field) => !readSet.has(field)), + readNotDeclared: read.filter((field) => !declaredSet.has(field)), + }; +} + +/** PURE. The failure text, naming each field and what its specific breakage looks like to an operator. */ +export function describeFieldDrift(drift: FieldDrift): string { + const lines = ["", `FOCUS_MANIFEST_TOP_LEVEL_FIELDS no longer matches what parseFocusManifest reads (${FOCUS_MANIFEST_SOURCE}):`, ""]; + if (drift.declaredNotRead.length > 0) { + lines.push(" DECLARED but never read -- the parser accepts these and silently ignores them, so an operator"); + lines.push(" who sets one gets no warning and no effect:"); + for (const field of drift.declaredNotRead) lines.push(` • ${field}`); + lines.push("", " Either read the field in parseFocusManifest, or drop it from the list (and consider"); + lines.push(" RETIRED_FOCUS_MANIFEST_FIELDS if operators may already have it in a manifest).", ""); + } + if (drift.readNotDeclared.length > 0) { + lines.push(" READ but not declared -- the parser consumes these while warning \"unknown top-level field\""); + lines.push(" about them, and config-lint omits them from the recognized count:"); + for (const field of drift.readNotDeclared) lines.push(` • ${field}`); + lines.push("", " Add each to FOCUS_MANIFEST_TOP_LEVEL_FIELDS.", ""); + } + return lines.join("\n"); +} + +/* v8 ignore start -- the self-execution guard; every pure branch above is driven directly in tests. */ +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + const source = readFileSync(FOCUS_MANIFEST_SOURCE, "utf8"); + const declared = declaredFields(source); + const read = parserReadsAre(source); + const drift = findFieldDrift(declared, read); + if (drift.declaredNotRead.length === 0 && drift.readNotDeclared.length === 0) { + process.stdout.write(`focus-manifest-fields: ${declared.length} top-level field(s) declared and read; no drift.\n`); + process.exit(0); + } + process.stderr.write(describeFieldDrift(drift)); + process.exit(1); +} +/* v8 ignore stop */ diff --git a/test/unit/check-focus-manifest-fields.test.ts b/test/unit/check-focus-manifest-fields.test.ts new file mode 100644 index 000000000..103f3313a --- /dev/null +++ b/test/unit/check-focus-manifest-fields.test.ts @@ -0,0 +1,128 @@ +import { readFileSync } from "node:fs"; + +import { describe, expect, it } from "vitest"; + +import { + declaredFields, + describeFieldDrift, + findFieldDrift, + FOCUS_MANIFEST_SOURCE, + parserReadsAre, +} from "../../scripts/check-focus-manifest-fields"; + +// #9979 (item 4 of #9860): the guard for a comment that asserts a fact about the code below it. +// +// `FOCUS_MANIFEST_TOP_LEVEL_FIELDS`'s header says "Every top-level key parseFocusManifest below actually +// reads", and nothing enforced it. The list is the single source of truth for unknown-field detection, so +// drift is silent in both directions and worse in the declared-but-unread one: the parser accepts a field, +// warns about nothing, and ignores it, which looks to an operator exactly like a setting that works. + +const SOURCE = readFileSync(FOCUS_MANIFEST_SOURCE, "utf8"); + +/** A miniature file with the same two shapes the checker reads, so the parsing can be driven over inputs the + * real file will never contain -- including the broken ones. */ +function fixture(input: { declared: readonly string[]; read: readonly string[] }): string { + return [ + "// preamble", + `export const FOCUS_MANIFEST_TOP_LEVEL_FIELDS = [\n${input.declared.map((field) => ` "${field}",`).join("\n")}\n] as const;`, + "", + "export function parseFocusManifest(raw: unknown): FocusManifest {", + " const record = raw as Record;", + ...input.read.map((field) => ` const ${field} = record.${field};`), + " return manifest;", + "}", + "", + "export function parseSomethingElse(raw: unknown) {", + " const record = raw as Record;", + " return record.notATopLevelField;", + "}", + ].join("\n"); +} + +describe("declaredFields", () => { + it("reads the names out of the array literal in source order", () => { + expect(declaredFields(fixture({ declared: ["source", "gate"], read: [] }))).toEqual(["source", "gate"]); + }); + + it("THROWS when the declaration is gone, rather than reporting an empty list", () => { + // A renamed or deleted constant must break this checker loudly. Returning [] would make the comparison + // trivially pass and turn the guard into decoration. + expect(() => declaredFields("export const SOMETHING_ELSE = [] as const;")).toThrow(/could not find the FOCUS_MANIFEST_TOP_LEVEL_FIELDS declaration/); + }); +}); + +describe("parserReadsAre", () => { + it("collects record. reads from parseFocusManifest", () => { + expect(parserReadsAre(fixture({ declared: [], read: ["gate", "settings"] }))).toEqual(["gate", "settings"]); + }); + + it("REGRESSION: stops at the next top-level export, so a sibling parser's fields do not leak in", () => { + // focus-manifest.ts declares many parsers that all destructure a local named `record`. Reading past the + // function boundary would pull `notATopLevelField` in and report it as undeclared drift forever. + expect(parserReadsAre(fixture({ declared: [], read: ["gate"] }))).not.toContain("notATopLevelField"); + }); + + it("also handles bracket access", () => { + const source = fixture({ declared: [], read: ["gate"] }).replace("record.gate", 'record["publicStats"]'); + expect(parserReadsAre(source)).toEqual(["publicStats"]); + }); + + it("THROWS when the signature is missing", () => { + expect(() => parserReadsAre("export const X = 1;")).toThrow(/could not find export function parseFocusManifest/); + }); + + it("THROWS when the access pattern yields nothing, rather than passing vacuously", () => { + // The most dangerous silent failure: someone refactors to destructuring (`const { gate } = record`) and a + // regex-based checker matches zero fields. Zero matches must be an error, never "no drift". + const refactored = ["export function parseFocusManifest(raw: unknown) {", " const { gate } = raw as Record;", " return gate;", "}"].join("\n"); + expect(() => parserReadsAre(refactored)).toThrow(/access pattern this checker relies on has changed/); + }); +}); + +describe("findFieldDrift", () => { + it("reports nothing when the two agree", () => { + expect(findFieldDrift(["a", "b"], ["a", "b"])).toEqual({ declaredNotRead: [], readNotDeclared: [] }); + }); + + it("catches a field declared but never read -- the direction that looks configured and does nothing", () => { + expect(findFieldDrift(["a", "ghost"], ["a"]).declaredNotRead).toEqual(["ghost"]); + }); + + it("catches a field read but never declared -- a working field warned about as unknown", () => { + expect(findFieldDrift(["a"], ["a", "orphan"]).readNotDeclared).toEqual(["orphan"]); + }); + + it("is order-insensitive, since the list is source-ordered and the reads are sorted", () => { + expect(findFieldDrift(["b", "a"], ["a", "b"])).toEqual({ declaredNotRead: [], readNotDeclared: [] }); + }); +}); + +describe("describeFieldDrift", () => { + it("names each field and what its breakage looks like to an operator", () => { + const message = describeFieldDrift(findFieldDrift(["a", "ghost"], ["a", "orphan"])); + expect(message).toContain("ghost"); + expect(message).toContain("orphan"); + expect(message).toContain("silently ignores them"); + expect(message).toContain("Add each to FOCUS_MANIFEST_TOP_LEVEL_FIELDS"); + }); +}); + +describe("the real focus-manifest.ts", () => { + it("INVARIANT: every declared top-level field is read by parseFocusManifest, and vice versa", () => { + // The whole point, asserted against the actual file rather than a fixture. + expect(findFieldDrift(declaredFields(SOURCE), parserReadsAre(SOURCE))).toEqual({ declaredNotRead: [], readNotDeclared: [] }); + }); + + it("reads a non-trivial number of fields, so the invariant above cannot pass on an empty set", () => { + // Guards the guard: if both extractors somehow returned [], the assertion above would pass. They cannot + // (both throw on no match), and this pins the expectation anyway. + expect(declaredFields(SOURCE).length).toBeGreaterThan(20); + expect(parserReadsAre(SOURCE).length).toBeGreaterThan(20); + }); + + it("includes the fields the parser's own structure guarantees", () => { + // A spot-check with real names, so a wholesale extraction failure that still returned *something* is + // caught rather than shrugged at. + expect(declaredFields(SOURCE)).toEqual(expect.arrayContaining(["source", "gate", "settings", "review", "publicStats"])); + }); +});