diff --git a/.changeset/9220-readme-exports-list-precondition.md b/.changeset/9220-readme-exports-list-precondition.md new file mode 100644 index 0000000000..fb692bd183 --- /dev/null +++ b/.changeset/9220-readme-exports-list-precondition.md @@ -0,0 +1,12 @@ +--- +--- + +`scripts/check-readme-exports.mjs --list` no longer crashes on an unbuilt tree +(objectui#9220). Every `documentedTypes` row now comes out of one factory with +one key set, so the row formatter's field access is safe for any verdict and the +verdict check is back to governing presentation rather than safety; and `--list` +answers an unmet precondition the way `check-doc-snippet-types.mjs` already +does — it prints every row and the census it could derive, then +`PRECONDITION NOT MET (exit 2)` naming the unbuilt packages and a scoped build +command. Exit 2 is dedicated: exit 1 still means a verdict was read and a README +is wrong. Tooling only; no package is released by this change. diff --git a/content/docs/guide/ci-cd-pipeline.md b/content/docs/guide/ci-cd-pipeline.md index 026e367835..ba7db7d759 100644 --- a/content/docs/guide/ci-cd-pipeline.md +++ b/content/docs/guide/ci-cd-pipeline.md @@ -1574,6 +1574,15 @@ reject while `BaseSchema` carries an index signature and its Zod mirror is `.pas exports the name. Run it locally with `pnpm check:readme-exports` after a build, or `node scripts/check-readme-exports.mjs --list` to see every self-import it judged. +`--list` is the diagnostic you reach for in the state where the gate just failed, so it answers in +that state rather than dying in it (objectui#9220): on a tree where a tracked package is unbuilt it +prints every row and the census it *could* derive, then `PRECONDITION NOT MET (exit 2)` naming the +unbuilt packages and a build command scoped to them. The separate exit code is the point — exit 1 +means "a verdict was read and a README is wrong", exit 2 means "nothing above is a verdict". Before +that card the same state was an uncaught `TypeError` in the row formatter, which printed no census, +no row past the first unjudgeable declaration, and left exit 1, indistinguishable from the +fabricated-name failure the gate exists to report. + ## Docs Route Eager Closure (`docs-route-eager-closure.yml`) **Triggers:** Push and PR to `main`/`develop`, merge-queue builds, plus manual dispatch — with **no diff --git a/scripts/__tests__/check-readme-exports.test.ts b/scripts/__tests__/check-readme-exports.test.ts index e64cd41ba0..33b99bdf92 100644 --- a/scripts/__tests__/check-readme-exports.test.ts +++ b/scripts/__tests__/check-readme-exports.test.ts @@ -39,6 +39,7 @@ import { describe, expect, it } from 'vitest'; import { REQUIRED_CONTEXTS } from '../dependabot-merge-gate.mjs'; import { CODE_LANGS, + EXIT_CODES, FLOORS, MIN_PARTIAL_REASON, PARTIAL_EXCERPTS, @@ -50,6 +51,7 @@ import { findPartialMarkers, packageDirOf, parseReadmeOverrides, + renderList, scan, summarise, typeEntryOf, @@ -1251,6 +1253,156 @@ describe('the --readme override, which is what keeps the self-test off the worki }); }); +/** + * `--list` on an UNBUILT tree (objectui#9220). + * + * ## Why this block exists and what the ablation leg is + * + * `--list` is this gate's own documented diagnostic and the only state a + * developer reaches for it in is the state where the gate just failed. It used + * to CRASH there: the row formatter read `t.fabricated.length` behind a guard + * that whitelisted two literal verdicts, `unjudgeable-type` was not one of + * them, and the run died on the first declaration it could not judge — + * `TypeError: Cannot read properties of undefined (reading 'length')`, with no + * census, no row past that one, and exit 1, the same code the gate uses to + * report a genuinely fabricated name. + * + * ⭐ THE ABLATION LEG, and the reason this block asserts a SHAPE and not only + * the absence of a throw: back `documentedTypeRow` out of the + * `unjudgeable-type` push in `check-readme-exports.mjs` — restore the bare + * `documentedTypes.push({ ...site, verdict: 'unjudgeable-type' })` — and + * `renderList` throws again on the fixture below. Both the "prints every row" + * case and the identical-key-set case go red; the exit-code cases go red too, + * because nothing returns at all. Measured, not predicted, on this branch — + * the numbers are in the pull request. + * + * The whitelist is NOT the repair, and this block is written so that re-adding + * one would not satisfy it: a third verdict string would make TODAY's row safe + * and leave the construct — a field-access guard enumerated by verdict — intact + * for the next verdict anyone adds. What is asserted below is that EVERY row + * carries the same key set, which is a fact about fields rather than about the + * membership of a list. + */ +describe('`--list` REFUSES on an unbuilt tree instead of dying in it (objectui#9220)', () => { + const UNBUILT_README = 'packages/unbuilt/README.md'; + const root = fixtureTree({ + ...PIN_FIXTURE, + 'packages/pin/README.md': '# @fix/pin\n', + // Declares a type entry, and that entry is not on disk. This is the card's + // `mv packages/plugin-kanban/dist /tmp/parked` as a fixture: the one state + // the gate's own failure text sends a developer to inspect. + 'packages/unbuilt/package.json': manifest('@fix/unbuilt', './dist/index.d.ts'), + [`packages/unbuilt/README.md`]: '# @fix/unbuilt\n', + }); + + /** + * The unbuilt package's README is walked FIRST on purpose. The defect was not + * only "it throws" — it was "no row past the first bad one", so a listing that + * hit the unjudgeable declaration last would have looked almost healthy. + */ + const run = () => + scan(root, { + readmes: [UNBUILT_README, PIN_README], + packageDirs: ['packages/unbuilt', ...PIN_PACKAGES], + readmeOverrides: { + [UNBUILT_README]: writeReadme(root, '# @fix/unbuilt\n\n```ts\ninterface Parked {\n id: string;\n}\n```\n'), + [PIN_README]: writeReadme(root, '# @fix/pin\n\n```ts\ninterface Widget {\n id: string;\n label?: string;\n hidden?: boolean;\n}\n```\n'), + }, + floors: {}, + }); + + it('CONTROL: the fixture really is in the state under test', () => { + // Without this leg every assertion below could be green because the walk + // found nothing — the failure mode this gate itself exists to catch. + const result = run(); + expect(result.census.packagesUnbuilt).toBe(1); + expect(result.census.typesUnjudgeable).toBe(1); + expect(result.documentedTypes.map((t) => t.verdict)).toEqual(['unjudgeable-type', 'matches']); + }); + + it('does NOT throw, and prints every row — including the ones AFTER the unjudgeable one', () => { + const rendered = renderList(run()); + const rows = rendered.rows.filter((r) => r !== ''); + expect(rows.some((r) => r.startsWith('unjudgeable-type') && r.includes('interface Parked'))).toBe(true); + expect(rows.some((r) => r.startsWith('matches') && r.includes('interface Widget'))).toBe(true); + // The census is the other half of what the crash destroyed. + expect(rendered.rows.at(-1)).toContain('documented type(s)'); + }); + + it('leaves through a DEDICATED exit code, which is NOT the fabricated-name code', () => { + expect(renderList(run()).exitCode).toBe(EXIT_CODES.couldNotRun); + expect(EXIT_CODES.couldNotRun).not.toBe(EXIT_CODES.readmesFailed); + expect(EXIT_CODES.couldNotRun).not.toBe(EXIT_CODES.verified); + }); + + it('names the precondition, the unbuilt package, and a build command scoped to it', () => { + const notices = renderList(run()).notices.join('\n'); + expect(notices).toContain(`PRECONDITION NOT MET (exit ${EXIT_CODES.couldNotRun})`); + expect(notices).toContain('@fix/unbuilt'); + expect(notices).toContain('--filter @fix/unbuilt'); + // Scoped: the package that IS built must not be in the build command. + expect(notices).not.toContain('--filter @fix/pin'); + }); + + it('CONTROL, known direction: with nothing unbuilt it exits 0 and issues no notice', () => { + // The same renderer over the same fixture minus the unbuilt package. If this + // leg ever goes green-by-accident alongside the ones above, the refusal is + // firing unconditionally and `--list` has stopped being usable at all. + const result = scan(root, { + readmes: [PIN_README], + packageDirs: PIN_PACKAGES, + readmeOverrides: { + [PIN_README]: writeReadme(root, '# @fix/pin\n\n```ts\ninterface Widget {\n id: string;\n label?: string;\n hidden?: boolean;\n}\n```\n'), + }, + floors: {}, + }); + expect(result.census.packagesUnbuilt).toBe(0); + const rendered = renderList(result); + expect(rendered.exitCode).toBe(EXIT_CODES.verified); + expect(rendered.notices).toEqual([]); + }); + + it('gives EVERY row the same key set, whatever its verdict — the field guard is not a verdict list', () => { + // ⭐ The assertion the whitelist repair cannot satisfy. `unjudgeable-type`, + // `local-declaration`, `not-a-property-type` and a COMPARED row all come out + // of one factory, so a verdict added tomorrow is safe to format without + // anyone remembering to touch `renderList`. + const result = scan(root, { + readmes: [UNBUILT_README, PIN_README], + packageDirs: ['packages/unbuilt', ...PIN_PACKAGES], + readmeOverrides: { + [UNBUILT_README]: writeReadme(root, '# @fix/unbuilt\n\n```ts\ninterface Parked {\n id: string;\n}\n```\n'), + [PIN_README]: writeReadme( + root, + '# @fix/pin\n\n```ts\ninterface Widget {\n id: string;\n label?: string;\n hidden?: boolean;\n}\n' + + 'interface Local {\n only: string;\n}\ntype Mode = \'a\' | \'b\';\n```\n', + ), + }, + floors: {}, + }); + const verdicts = result.documentedTypes.map((t) => t.verdict); + expect(verdicts).toContain('unjudgeable-type'); + expect(verdicts).toContain('local-declaration'); + expect(verdicts).toContain('matches'); + const keySets = result.documentedTypes.map((t) => Object.keys(t).sort().join(',')); + expect(new Set(keySets).size).toBe(1); + for (const row of result.documentedTypes) { + expect(Array.isArray(row.fabricated)).toBe(true); + expect(Array.isArray(row.omitted)).toBe(true); + } + }); + + it('prints the census detail ONLY where a comparison happened — safety and presentation are separate', () => { + const rows = renderList(run()).rows; + const unjudgeable = rows.find((r) => r.startsWith('unjudgeable-type')); + const compared = rows.find((r) => r.startsWith('matches')); + // Not "0 key(s) vs own 0 of 0", which would state a comparison that never + // ran — the same defect one level up from the crash. + expect(unjudgeable).not.toContain('key(s)'); + expect(compared).toContain('doc 3 key(s) + 0 method(s) vs own 3 of 3'); + }); +}); + describe('wiring — the gate is reachable and every pull-request shape starts it', () => { const workflowDir = path.join(repoRoot, '.github/workflows'); const workflowFiles = fs.readdirSync(workflowDir).filter((f) => f.endsWith('.yml')); diff --git a/scripts/check-readme-exports.mjs b/scripts/check-readme-exports.mjs index add472da51..41e13c5ceb 100644 --- a/scripts/check-readme-exports.mjs +++ b/scripts/check-readme-exports.mjs @@ -6,11 +6,17 @@ * name that package really exports. * * Run: node scripts/check-readme-exports.mjs (also `pnpm check:readme-exports`) - * node scripts/check-readme-exports.mjs --list # every self-binding judged + * node scripts/check-readme-exports.mjs --list # every self-binding judged (exit 2 if unbuilt) * node scripts/check-readme-exports.mjs --json * node scripts/check-readme-exports.mjs --readme packages/plugin-gantt/README.md=/tmp/x.md * Exit: 0 = every self-import names a real export, 1 = a fabricated name, a * wrong-path name, a package that cannot be judged, or a collapsed scan. + * `--list` adds 2 = PRECONDITION NOT MET: a tracked package is unbuilt, so + * the rows it printed are what this run could SEE and not a verdict. 1 and + * 2 are separate because a caller that reads only the status otherwise + * cannot tell "the gate caught a fabricated name" from "the gate could not + * look" -- and before objectui#9220 that second state was not even an exit + * code, it was an uncaught TypeError in the formatter, which also left 1. * * ## The defect (objectui#5043, the root cause of the #5010-#5016 family) * @@ -831,6 +837,86 @@ function readJson(path) { } } +/** + * The gate's exit codes, named so that callers and tests can talk about them. + * `couldNotRun` is the convention this repository already declared, in + * `check-doc-snippet-types.mjs` and `check-skill-examples.mjs`, and it is here + * for the reason it is there: a crash and "the gate read a verdict" both leave + * through a non-zero exit, so the exit code stops discriminating exactly where + * a caller needs it to (objectui#9220). + * + * ⛔ `readmesFailed` is NOT re-derived by anything; `main()` spells `1` out + * literally and this member does not reach it. Read it as documentation of + * that number, never as the thing that produces it -- AGENTS.md #9. What IS + * mechanical is the one claim that matters here: `couldNotRun` must differ from + * it, which `check-readme-exports.test.ts` asserts. + */ +export const EXIT_CODES = Object.freeze({ + /** Every judged binding and declaration held, and the population is real. */ + verified: 0, + /** The gate RAN. A README or the ledger is at fault -- a verdict was read. */ + readmesFailed: 1, + /** The gate COULD NOT RUN. Nothing it printed is a verdict about a README. */ + couldNotRun: 2, +}); + +/** + * ONE row shape for `documentedTypes`, whatever the verdict (objectui#9220). + * + * Four call sites push into that array and three of them used to push a bare + * `{ ...site, verdict }`, so whether a consumer could read `row.fabricated` + * depended on which branch produced the row. `--list` guarded that read with a + * whitelist of two literal verdicts -- and a whitelist enumerated by verdict + * cannot express a fact about FIELDS: the third bare shape, `unjudgeable-type`, + * was not in it and the formatter died reading `undefined.length` on precisely + * the unbuilt tree the gate's own failure text sends a developer to inspect. + * + * So the repair is not another string in that whitelist. Every row carries + * every field, and the verdict goes back to governing PRESENTATION -- whether + * this census detail is worth printing -- instead of SAFETY. `compared` is what + * the presentation side reads, and it is the honest question: a row that + * compared nothing has zeroes and empty arrays, and printing "0 of 0" for it + * would state a comparison that never happened. + * + * @typedef {{ file: string, line: number, package: string | null, typeName: string, kind: string }} DocumentedTypeSite + * @typedef {DocumentedTypeSite & { + * verdict: string, + * compared: boolean, + * documented: number, + * documentedMethods: number, + * otherMembers: number, + * shippedOwn: number, + * shippedAll: number, + * fabricated: string[], + * omitted: string[], + * excerpt: string | null, + * }} DocumentedTypeRow + * + * @param {DocumentedTypeSite} site The README site: file, line, package, typeName, kind. + * @param {string} verdict + * @param {Partial} [detail] The measured half, at the ONE call site that compares. + * @returns {DocumentedTypeRow} + */ +function documentedTypeRow(site, verdict, detail = undefined) { + return { + ...site, + verdict, + // `compared: false` and the zeroes below are not placeholders for a + // measurement that is coming -- they are the measurement. Nothing was + // compared, so nothing was documented against, fabricated or omitted. + compared: detail !== undefined, + documented: 0, + documentedMethods: 0, + otherMembers: 0, + shippedOwn: 0, + shippedAll: 0, + fabricated: [], + omitted: [], + excerpt: null, + ...detail, + }; +} + /** * The one scan. `main()`, `--list`, `--json` and the test suite all go through * here, so the tests exercise the real code path rather than an imitation. @@ -1036,7 +1122,7 @@ export function scan( // only fails where the missing surface would have changed a verdict, // which is precisely "this block declares a type". counters.typesUnjudgeable++; - documentedTypes.push({ ...site, verdict: 'unjudgeable-type' }); + documentedTypes.push(documentedTypeRow(site, 'unjudgeable-type')); findings.push({ ...site, verdict: 'unjudgeable-type', reason: record.state, declaredEntry: record.declaredEntry }); // AND the shrink-only rule is suspended for whatever declared this // declaration an excerpt. An entry or a marker here suppressed nothing @@ -1070,12 +1156,12 @@ export function scan( // a failure. (Whether a name owned by ANOTHER package should be // judged here is measured in the header and deliberately not done.) counters.typesLocal++; - documentedTypes.push({ ...site, verdict: 'local-declaration' }); + documentedTypes.push(documentedTypeRow(site, 'local-declaration')); continue; } if (!hit.shape) { counters.typesNotAShape++; - documentedTypes.push({ ...site, verdict: 'not-a-property-type' }); + documentedTypes.push(documentedTypeRow(site, 'not-a-property-type')); continue; } counters.typesResolved++; @@ -1116,18 +1202,22 @@ export function scan( usedExcerpts.add(ledgerKey); } } - documentedTypes.push({ - ...site, - verdict: fabricated.length > 0 ? 'fabricated-key' : omitted.length === 0 ? 'matches' : excerpt === null ? 'stale-omission' : `partial-${excerpt.source}`, - documented: declared.keys.length, - documentedMethods: declared.methods.length, - otherMembers: declared.other, - shippedOwn: hit.shape.own.size, - shippedAll: hit.shape.all.size, - fabricated, - omitted, - excerpt: excerpt === null ? null : excerpt.source, - }); + documentedTypes.push( + documentedTypeRow( + site, + fabricated.length > 0 ? 'fabricated-key' : omitted.length === 0 ? 'matches' : excerpt === null ? 'stale-omission' : `partial-${excerpt.source}`, + { + documented: declared.keys.length, + documentedMethods: declared.methods.length, + otherMembers: declared.other, + shippedOwn: hit.shape.own.size, + shippedAll: hit.shape.all.size, + fabricated, + omitted, + excerpt: excerpt === null ? null : excerpt.source, + }, + ), + ); } for (const binding of findImportBindings(block.body, { jsx: block.jsx })) { @@ -1261,6 +1351,91 @@ export function scan( return { census, packages: [...packages.values()], orphans, bindings, documentedTypes, findings, vacuous }; } +/** + * `--list`, as data: the rows it prints, the notices it prints to stderr, and + * the code it leaves through. Split out of the entry point so the pin tests can + * drive it over a FIXTURE tree -- the entry point can only ever scan + * `repoRoot()`, and a build state is not something a test may arrange there. + * + * ## The two jobs the old formatter had jammed into one ternary (objectui#9220) + * + * SAFETY -- may these fields be read at all -- is now settled for every row by + * `documentedTypeRow`, upstream of here and independent of the verdict. + * PRESENTATION -- is this census detail worth printing -- is what is left, and + * it reads `compared`, which is the fact it is actually asking about. + * + * ## Why a listing can refuse + * + * `--list` is this gate's own documented diagnostic, and the state a developer + * reaches for it in is the state where the gate just failed. On an unbuilt tree + * it used to die on the first declaration it could not judge: no rows past that + * one, no census, and nothing naming the precondition -- while its exit code, + * 1, was the same code the gate uses to report a genuinely fabricated name. + * + * So it prints everything it has AND THEN refuses, the way + * `check-doc-snippet-types.mjs` already does for the same state: the rows are + * what this run could see, the notice says they are not a verdict, and + * `EXIT_CODES.couldNotRun` carries that difference to a caller that only reads + * the status. + * + * @param {ReturnType} result + * @returns {{ rows: string[], notices: string[], exitCode: number }} + */ +export function renderList(result) { + const rows = []; + for (const b of result.bindings) { + if (b.verdict === 'not-self') continue; + const mark = b.verdict.padEnd(17); + rows.push(`${mark} ${b.file}:${b.line} ${b.exportName ?? `(${b.kind})`} <- ${b.specifier}`); + } + for (const t of result.documentedTypes) { + const mark = t.verdict.padEnd(17); + const detail = t.compared + ? ` doc ${t.documented} key(s) + ${t.documentedMethods} method(s) vs own ${t.shippedOwn} of ${t.shippedAll}` + + (t.fabricated.length > 0 ? ` fabricated: ${t.fabricated.join(', ')}` : '') + + (t.omitted.length > 0 ? ` omitted: ${t.omitted.join(', ')}` : '') + : ''; + rows.push(`${mark} ${t.file}:${t.line} ${t.kind} ${t.typeName}${detail}`); + } + rows.push(''); + rows.push(summarise(result)); + + // Every unbuilt package, not only the ones carrying a README. An unbuilt + // package contributes no names to `nameOwners`, so a README import of a name + // that package really does export is judged `fabricated` instead of + // `wrong-path` -- a package with no README of its own still moves verdicts in + // other packages' rows. `no-type-entry` is deliberately NOT here: no build + // fixes it, and it is a verdict about the manifest rather than a precondition. + const unbuilt = result.packages.filter((p) => p.state === 'unbuilt'); + if (unbuilt.length === 0) return { rows, notices: [], exitCode: EXIT_CODES.verified }; + + const named = unbuilt.map((p) => p.name ?? p.dir); + // A scoped filter list stops being a scoping when it names the whole + // population -- at that point it is `pnpm build` spelled out at forty times + // the length, and a developer who has to edit the line before running it has + // been handed a worse command, not a more precise one. + const buildCommand = + unbuilt.length === result.packages.length + ? 'pnpm build' + : `pnpm exec turbo run build ${named.map((n) => `--filter ${n}`).join(' ')} --concurrency=2`; + + const notices = [ + `\nPRECONDITION NOT MET (exit ${EXIT_CODES.couldNotRun}) — the rows above are NOT a verdict about any README.`, + `${unbuilt.length} of ${result.packages.length} tracked package(s) declare a type entry that is not on disk, so ` + + 'their export surface was never read. Every declaration owned by one of them is listed above as ' + + '`unjudgeable-type` with no census detail, and an import of a name one of them really does export reads ' + + 'above as `fabricated` rather than `wrong-path`.', + `This is "I could not run", NOT "I ran and found a fabricated name" (exit ${EXIT_CODES.readmesFailed}). ` + + 'Build what the gate needs, then re-run:\n\n' + + ` ${buildCommand}\n` + + ' node scripts/check-readme-exports.mjs --list\n', + 'The package(s) that are not built:', + ...unbuilt.map((p) => ` ${(p.name ?? p.dir).padEnd(38)} ${p.dir} declares \`${p.declaredEntry}\``), + '', + ]; + return { rows, notices, exitCode: EXIT_CODES.couldNotRun }; +} + function repoRoot() { return resolve(dirname(fileURLToPath(import.meta.url)), '..'); } @@ -1467,22 +1642,10 @@ if (isEntrypoint(import.meta.url)) { ); } else if (process.argv.includes('--list')) { const result = scan(repoRoot(), { readmeOverrides: overrides }); - for (const b of result.bindings) { - if (b.verdict === 'not-self') continue; - const mark = b.verdict.padEnd(17); - console.log(`${mark} ${b.file}:${b.line} ${b.exportName ?? `(${b.kind})`} <- ${b.specifier}`); - } - for (const t of result.documentedTypes) { - const mark = t.verdict.padEnd(17); - const detail = - t.verdict === 'local-declaration' || t.verdict === 'not-a-property-type' - ? '' - : ` doc ${t.documented} key(s) + ${t.documentedMethods} method(s) vs own ${t.shippedOwn} of ${t.shippedAll}` + - (t.fabricated.length > 0 ? ` fabricated: ${t.fabricated.join(', ')}` : '') + - (t.omitted.length > 0 ? ` omitted: ${t.omitted.join(', ')}` : ''); - console.log(`${mark} ${t.file}:${t.line} ${t.kind} ${t.typeName}${detail}`); - } - console.log(`\n${summarise(result)}`); + const { rows, notices, exitCode } = renderList(result); + for (const row of rows) console.log(row); + for (const notice of notices) console.error(notice); + process.exit(exitCode); } else { main(overrides); }