From 58d89cdd6c90bd35e30d75d70643de8b9410bb78 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 10:40:22 +0000 Subject: [PATCH] fix(spec): check:dual-source-exports / check:exported-any / check:skill-examples refuse a stale dist (#7181) All three read the built `dist/*.d.ts` and documented that as a precondition without enforcing it, so each could report a verdict about a build nobody made. They now adopt #7122's `inspectDistFreshness` before the first declaration is read. The primitive's refusal named `check:api-surface` by hand, which would have made three gates prescribe a fourth gate's command -- following it re-runs something that was never refused, so the refusal reads as cleared. The caller now passes its own re-run command; a third `mode` value was measured and rejected, because `mode` selects the damage sentence (writing a wrong baseline vs agreeing with one) and both existing values are semantically right for these callers. Two findings that change the card's framing, both pinned by tests: - `check-dual-source-exports.ts --update` REWRITES a tracked baseline, so the "all three are check-only, none can launder a bad baseline" premise does not hold for it. That path gets `mode: 'generate'`. - the anti-vacuity floors these gates carry are self-test and missing-dist floors. None of them fires on a present-but-stale dist, which is the state being refused. Co-Authored-By: Claude Opus 5 --- packages/spec/scripts/build-api-surface.ts | 6 +- .../spec/scripts/check-dual-source-exports.ts | 29 ++ packages/spec/scripts/check-exported-any.ts | 28 ++ packages/spec/scripts/check-skill-examples.ts | 36 +++ .../scripts/dist-freshness-adoption.test.ts | 292 ++++++++++++++++++ packages/spec/scripts/dist-freshness.test.ts | 42 ++- packages/spec/scripts/lib/dist-freshness.ts | 46 ++- 7 files changed, 462 insertions(+), 17 deletions(-) create mode 100644 packages/spec/scripts/dist-freshness-adoption.test.ts diff --git a/packages/spec/scripts/build-api-surface.ts b/packages/spec/scripts/build-api-surface.ts index b62cd20e46..668eac0dea 100644 --- a/packages/spec/scripts/build-api-surface.ts +++ b/packages/spec/scripts/build-api-surface.ts @@ -73,7 +73,11 @@ const CHECK = process.argv.includes('--check'); // `ts.createProgram` has run over a stale dist, every answer below it is // confidently wrong, and both writing it and checking against it are worse than // stopping here. -const freshness = inspectDistFreshness(PKG_DIR, CHECK ? 'check' : 'generate'); +const freshness = inspectDistFreshness( + PKG_DIR, + CHECK ? 'check' : 'generate', + `pnpm --filter @objectstack/spec ${CHECK ? 'check' : 'gen'}:api-surface`, +); if (!freshness.fresh) { console.error(freshness.message); process.exit(1); diff --git a/packages/spec/scripts/check-dual-source-exports.ts b/packages/spec/scripts/check-dual-source-exports.ts index 2da48a5a63..5e3c0f0405 100644 --- a/packages/spec/scripts/check-dual-source-exports.ts +++ b/packages/spec/scripts/check-dual-source-exports.ts @@ -48,12 +48,24 @@ * chunk, so distinct dist declarations imply distinct source declarations; the * self-test pins the detector itself, and the count assertions keep a silent * resolution failure from reading as "clean". + * + * That precondition is enforced since #7181, and `--update` is why it matters + * here more than in this file's two sibling gates. #7181 was filed on the reading + * that all three "are check-only — none writes a tracked artifact, so none can + * launder a wrong baseline into a commit". This one does: `--update` REWRITES + * `dual-source-exports.baseline.json`, and on a stale dist it writes a partition + * computed from declarations that predate the edit. The ratchet then makes that + * self-consistent in both directions — a name that only became dual-source after + * the last build is written out as clean, and the plain run compares the same + * baseline against the same stale dist and agrees. That is #7122's laundering + * shape exactly, one artifact over. See lib/dist-freshness.ts. */ import ts from 'typescript'; import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { join, resolve } from 'node:path'; import { tmpdir } from 'node:os'; import { fileURLToPath } from 'node:url'; +import { inspectDistFreshness } from './lib/dist-freshness'; const PKG_DIR = resolve(fileURLToPath(new URL('.', import.meta.url)), '..'); const BASELINE_PATH = resolve(PKG_DIR, 'dual-source-exports.baseline.json'); @@ -218,6 +230,23 @@ if (SELF_TEST) selfTest(); // ── Audit ──────────────────────────────────────────────────────────────────── +// BEFORE a single `.d.ts` is read (#7181, adopting #7122's primitive). `--update` +// is `generate`-shaped — it writes a tracked baseline — so it gets the writing +// damage, and the plain run gets the false-green one. Placed after `--self-test` +// on purpose: that path builds its own fixture in a temp dir and never reads +// `dist/`, so refusing it on a stale dist would refuse a run that is unaffected. +const freshness = inspectDistFreshness( + PKG_DIR, + UPDATE ? 'generate' : 'check', + UPDATE + ? 'pnpm --filter @objectstack/spec exec tsx scripts/check-dual-source-exports.ts --update' + : 'pnpm --filter @objectstack/spec check:dual-source-exports', +); +if (!freshness.fresh) { + console.error(freshness.message); + process.exit(1); +} + const entries = collectEntries(); const { findings, names, reExports } = scan(makeProgram(Object.values(entries)), entries); diff --git a/packages/spec/scripts/check-exported-any.ts b/packages/spec/scripts/check-exported-any.ts index b5a9c75f76..7150b80a00 100644 --- a/packages/spec/scripts/check-exported-any.ts +++ b/packages/spec/scripts/check-exported-any.ts @@ -70,6 +70,13 @@ * whose green result is "nothing found". * * Reads the built dist — run after `pnpm --filter @objectstack/spec build`. + * + * That last sentence is a PRECONDITION, and since #7181 it is enforced rather + * than merely documented: the audit refuses a dist that is missing or older than + * `src/`. The self-test above is the anti-vacuity floor for a broken DETECTOR; it + * says nothing about the vintage of the declarations the audit then reads, and on + * a stale dist this gate reports "no exported type resolves to `any`" without + * having read the export the developer just added. See lib/dist-freshness.ts. */ import ts from 'typescript'; import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; @@ -77,6 +84,7 @@ import { createRequire } from 'node:module'; import { dirname, join, resolve } from 'node:path'; import { tmpdir } from 'node:os'; import { fileURLToPath } from 'node:url'; +import { inspectDistFreshness } from './lib/dist-freshness'; const PKG_DIR = resolve(fileURLToPath(new URL('.', import.meta.url)), '..'); const SELF_TEST = process.argv.includes('--self-test'); @@ -266,6 +274,26 @@ if (SELF_TEST) selfTest(); // ── Audit ──────────────────────────────────────────────────────────────────── +// BEFORE a single `.d.ts` is read (#7181, adopting #7122's primitive). The +// existing floors — the self-test's count assertions, and the "Could not resolve +// module symbol … Is the package built?" throw in `scan` — cover a MISSING dist +// and a broken detector. Neither can see the case this refuses: a dist that is +// present and resolves fine but predates the edit under test. There the audit +// runs to completion and prints `✅ no exported type resolves to \`any\`` about a +// build nobody made, which is a false green on exactly the export the developer +// just wrote. It sits after `--self-test` deliberately: that path compiles a temp +// fixture against the real zod and never touches `dist/`, so refusing it on a +// stale dist would be over-reach. +const freshness = inspectDistFreshness( + PKG_DIR, + 'check', + 'pnpm --filter @objectstack/spec check:exported-any', +); +if (!freshness.fresh) { + console.error(freshness.message); + process.exit(1); +} + const entries = collectEntries(); const { violations, declared, types, schemas } = scan(makeProgram(Object.values(entries)), entries, KNOWN_ANY); diff --git a/packages/spec/scripts/check-skill-examples.ts b/packages/spec/scripts/check-skill-examples.ts index 8af7423643..33d80723cd 100644 --- a/packages/spec/scripts/check-skill-examples.ts +++ b/packages/spec/scripts/check-skill-examples.ts @@ -52,6 +52,13 @@ * step in CI — alongside `check:api-surface` / the example-app typecheck, its * fellow "real consumer" gates — not before it like `check:skill-refs`. * + * Since #7181 that ordering is enforced rather than assumed: the type-check half + * refuses a dist that is missing or older than `src/`. The existing "is the spec + * built" guard below only answers ABSENCE; a present-but-stale dist type-checks + * every example against the previous build and prints `✅ N prose examples + * type-check against @objectstack/spec` — a green about a rename the developer + * has already made and this run never saw. See lib/dist-freshness.ts. + * * ── The third anti-idle assertion: no bare `any` in a marked block (#5943) ─── * A marker is the author's claim "this block compiles", and the two guards above * (orphan marker, zero blocks) exist because a gate that checks nothing must not @@ -101,6 +108,8 @@ import os from 'os'; import path from 'path'; import ts from 'typescript'; +import { inspectDistFreshness } from './lib/dist-freshness'; + // ── Paths ──────────────────────────────────────────────────────────────────── const REPO_ROOT = path.resolve(__dirname, '../../..'); @@ -676,6 +685,33 @@ function main() { ); } + // BEFORE any declaration is resolved (#7181, adopting #7122's primitive). + // + // Placement differs from the two sibling gates on purpose, because the ROUTE to + // the dist differs: those resolve entry points in-process with + // `ts.createProgram`, so their first `.d.ts` read is their first statement of + // work. Here the declarations are reached indirectly — `specPaths()` turns the + // exports map into a tsconfig `paths` table and a spawned `tsc` follows it — and + // everything above this line (extraction, the orphan-marker guard, the zero-block + // guard, the bare-`any` guard) is dist-independent and worth reporting even when + // the build is stale. So the guard sits at the boundary rather than at the top: + // no verdict below it is computed, and no honest finding above it is suppressed. + // + // The `missing` check immediately following is NOT redundant. It answers "was + // the package built at all", which this also covers via `state: 'missing'`; but + // it stays because it is the one that survives a PARTIAL dist — a subpath whose + // `.d.ts` was never emitted while the newest declaration on disk is still newer + // than `src/`, which the mtime rule reads as fresh. + const freshness = inspectDistFreshness( + SPEC_DIR, + 'check', + 'pnpm --filter @objectstack/spec check:skill-examples', + ); + if (!freshness.fresh) { + console.error(freshness.message); + process.exit(1); + } + const { paths, missing } = specPaths(); if (missing.some((m) => m.endsWith('index.d.ts')) && !fs.existsSync(paths['@objectstack/spec']?.[0] ?? '')) { fail( diff --git a/packages/spec/scripts/dist-freshness-adoption.test.ts b/packages/spec/scripts/dist-freshness-adoption.test.ts new file mode 100644 index 0000000000..c51e2cf0e2 --- /dev/null +++ b/packages/spec/scripts/dist-freshness-adoption.test.ts @@ -0,0 +1,292 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// The three gates that adopted #7122's dist precondition (#7181), pinned end to +// end: `check:dual-source-exports`, `check:exported-any`, `check:skill-examples`. +// +// ## Why these cases run the real scripts instead of the primitive +// +// `dist-freshness.test.ts` already pins the RULE and its wording in both +// directions. What #7181 is about is not the rule — it is the WIRING, and the +// wiring has exactly two ways to be wrong that a unit test cannot see: +// +// 1. the guard is absent, or sits BELOW the first `.d.ts` read, so the analysis +// has already consumed the stale declarations by the time it speaks; +// 2. the guard is present but the gate refuses in a state CI is legitimately +// in, which is how a guard gets deleted rather than obeyed. +// +// So each gate gets both directions against its own real entry point: a stale +// dist must produce the refusal INSTEAD of a verdict, and a fresh one must +// produce the ordinary verdict with no refusal in sight. The positive control is +// what makes the negative one mean anything — without it, every "refuses" case +// below would also pass if the script simply could not run in this sandbox. +// +// ## The identity assertion, and why it is `not.toContain('api-surface')` +// +// Before #7181 the refusal named `check:api-surface` by hand for every caller. +// A developer told to re-run a gate they never ran runs something that was never +// refused, sees it pass, and reads the refusal as cleared. Each case therefore +// asserts the message names ITSELF and does not name the gate whose wording this +// primitive was written for. +// +// ## Why a sandbox rather than the real package +// +// Same reason `dist-freshness.test.ts` gives: these scripts resolve everything +// from their own `__dirname`, and making the real `packages/spec/dist` stale to +// drive a refusal would corrupt whatever else is running in the container. Each +// run happens in a repo-shaped temp tree that copies `scripts/`, symlinks the +// read-only inputs, and seeds a `dist/` whose mtimes this test controls. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { spawnSync, type SpawnSyncReturns } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const HERE = path.dirname(fileURLToPath(import.meta.url)); +const PKG = path.resolve(HERE, '..'); +const REPO_ROOT = path.resolve(PKG, '../..'); +const TSX = path.join(PKG, 'node_modules', '.bin', 'tsx'); + +const OLD = Math.floor(Date.now() / 1000) - 3600; +const NEW = Math.floor(Date.now() / 1000) - 60; + +/** A repo-shaped temp tree whose `packages/spec` runs the real gates. */ +function sandboxSpec(): { root: string; spec: string } { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'os-dist-adoption-')); + const spec = path.join(root, 'packages', 'spec'); + fs.mkdirSync(spec, { recursive: true }); + // The root `scripts/` the shared freshness rule lives in, at its real relative + // depth — `lib/dist-freshness.ts` imports it as `../../../../scripts/…`. + fs.symlinkSync(path.join(REPO_ROOT, 'scripts'), path.join(root, 'scripts'), 'dir'); + fs.cpSync(path.join(PKG, 'scripts'), path.join(spec, 'scripts'), { recursive: true }); + fs.symlinkSync(path.join(PKG, 'node_modules'), path.join(spec, 'node_modules'), 'dir'); + // The real exports map: every gate here derives the `.d.ts` it reads from it. + fs.copyFileSync(path.join(PKG, 'package.json'), path.join(spec, 'package.json')); + return { root, spec }; +} + +/** Every `dist/**.d.ts` the exports map points the gates at. */ +function declaredDts(spec: string): string[] { + const pkg = JSON.parse(fs.readFileSync(path.join(spec, 'package.json'), 'utf8')) as { + exports?: Record; + }; + const out: string[] = []; + for (const [sub, val] of Object.entries(pkg.exports ?? {})) { + if (!sub.startsWith('.')) continue; + const dts = val?.require?.types ?? val?.import?.types; + if (typeof dts === 'string' && dts.endsWith('.d.ts')) out.push(path.resolve(spec, dts)); + } + return out; +} + +/** + * Seed `src/` and the declared `dist/*.d.ts` at controlled mtimes. + * + * The declarations are trivial but REAL — they parse, and every entry point + * resolves — which is what makes the fresh-dist controls non-vacuous: each gate + * runs its whole analysis over them and reaches its ordinary "nothing found" + * verdict. The same tree, aged, is then the only variable in the refusal cases. + */ +function seed(spec: string, { distMtime, srcMtime }: { distMtime: number; srcMtime: number }): void { + fs.mkdirSync(path.join(spec, 'src'), { recursive: true }); + fs.writeFileSync(path.join(spec, 'src/index.ts'), 'export const live = 1;\n'); + fs.utimesSync(path.join(spec, 'src/index.ts'), srcMtime, srcMtime); + + for (const dts of declaredDts(spec)) { + fs.mkdirSync(path.dirname(dts), { recursive: true }); + fs.writeFileSync(dts, 'export {};\n'); + fs.utimesSync(dts, distMtime, distMtime); + } +} + +function runGate(spec: string, script: string, args: string[] = []): SpawnSyncReturns { + return spawnSync(TSX, [`scripts/${script}`, ...args], { + cwd: spec, + encoding: 'utf8', + env: { ...process.env, OS_SKIP_DTS: '' }, + }); +} + +/** Both halves of the refusal: it fired, and it named the right gate. */ +function expectRefusal(run: SpawnSyncReturns, rerun: string): void { + expect(run.status).toBe(1); + expect(run.stderr).toContain('OLDER than packages/spec/src'); + expect(run.stderr).toContain('pnpm --filter @objectstack/spec build'); + expect(run.stderr).toContain(rerun); + // The #7181 defect itself: every one of these used to print a fourth gate's name. + expect(run.stderr).not.toContain('api-surface'); +} + +let tree: { root: string; spec: string }; + +beforeEach(() => { + tree = sandboxSpec(); +}); + +afterEach(() => { + fs.rmSync(tree.root, { recursive: true, force: true }); +}); + +// ── check:dual-source-exports ──────────────────────────────────────────────── +// +// The one of the three that can WRITE. #7181 was filed on the reading that all +// three are check-only; `--update` rewrites `dual-source-exports.baseline.json`, +// so on a stale dist this gate can launder a wrong ratchet into a commit exactly +// the way `gen:api-surface` did (#7122). + +const DUAL = 'check-dual-source-exports.ts'; +const DUAL_BASELINE = 'dual-source-exports.baseline.json'; +const SENTINEL = '{ "_comment": "sentinel", "entries": [] }\n'; + +function seedDualBaseline(spec: string): void { + fs.writeFileSync(path.join(spec, DUAL_BASELINE), SENTINEL); +} + +describe('check:dual-source-exports refuses a stale dist (#7181)', () => { + it('POSITIVE CONTROL: a fresh dist runs the whole audit and reaches its verdict', () => { + seed(tree.spec, { distMtime: NEW, srcMtime: OLD }); + seedDualBaseline(tree.spec); + + const run = runGate(tree.spec, DUAL); + expect(run.status).toBe(0); + expect(run.stdout).toContain('no new dual-source exports'); + expect(run.stderr).not.toContain('OLDER than'); + }); + + it('refuses the audit on a stale dist instead of reporting "no new dual-source exports"', () => { + seed(tree.spec, { distMtime: OLD, srcMtime: NEW }); + seedDualBaseline(tree.spec); + + const run = runGate(tree.spec, DUAL); + expectRefusal(run, 'pnpm --filter @objectstack/spec check:dual-source-exports'); + expect(run.stdout).not.toContain('no new dual-source exports'); + }); + + it('refuses --update and writes NOTHING — the laundering path #7181 assumed absent', () => { + // A ratchet regenerated from declarations that predate the edit is wrong in + // both directions at once: a name that only became dual-source after the last + // build is written out as clean, and the plain run then compares that baseline + // against the same stale dist and agrees with it. + seed(tree.spec, { distMtime: OLD, srcMtime: NEW }); + seedDualBaseline(tree.spec); + + const run = runGate(tree.spec, DUAL, ['--update']); + expect(run.status).toBe(1); + expect(run.stderr).toContain('WRITE a baseline'); + expect(fs.readFileSync(path.join(tree.spec, DUAL_BASELINE), 'utf8')).toBe(SENTINEL); + }); + + it('still runs --self-test on a stale dist — that path never reads dist/', () => { + // Guard placement, asserted from the other side. The self-test compiles its + // own fixture in a temp dir; refusing it would refuse a run the stale dist + // cannot affect, and a gate that cannot be exercised without a build is a + // gate people stop exercising. + seed(tree.spec, { distMtime: OLD, srcMtime: NEW }); + seedDualBaseline(tree.spec); + + const run = runGate(tree.spec, DUAL, ['--self-test']); + expect(run.status).toBe(0); + expect(run.stdout).toContain('self-test'); + expect(run.stderr).not.toContain('OLDER than'); + }); +}); + +// ── check:exported-any ─────────────────────────────────────────────────────── + +const ANY = 'check-exported-any.ts'; + +describe('check:exported-any refuses a stale dist (#7181)', () => { + it('POSITIVE CONTROL: a fresh dist runs the whole audit and reaches its verdict', () => { + seed(tree.spec, { distMtime: NEW, srcMtime: OLD }); + + const run = runGate(tree.spec, ANY); + expect(run.status).toBe(0); + expect(run.stdout).toContain('no exported type resolves to'); + expect(run.stderr).not.toContain('OLDER than'); + }); + + it('refuses the audit on a stale dist instead of reporting "no exported type resolves to any"', () => { + // The existing floors cannot see this state: the self-test's count assertions + // pin the DETECTOR against a temp fixture, and `scan`'s "Is the package built?" + // throw fires only when a module symbol will not resolve at all. A dist that + // resolves fine and merely predates the edit passes both and prints a green. + seed(tree.spec, { distMtime: OLD, srcMtime: NEW }); + + const run = runGate(tree.spec, ANY); + expectRefusal(run, 'pnpm --filter @objectstack/spec check:exported-any'); + expect(run.stdout).not.toContain('no exported type resolves to'); + }); + + it('still runs --self-test on a stale dist — that path never reads dist/', () => { + seed(tree.spec, { distMtime: OLD, srcMtime: NEW }); + + const run = runGate(tree.spec, ANY, ['--self-test']); + expect(run.status).toBe(0); + expect(run.stdout).toContain('self-test'); + expect(run.stderr).not.toContain('OLDER than'); + }); +}); + +// ── check:skill-examples ───────────────────────────────────────────────────── +// +// The one whose route to the dist is NOT `collectEntries` + `ts.createProgram`: +// it turns the exports map into a tsconfig `paths` table and a spawned `tsc` +// follows it. So the guard sits at that boundary rather than at the top of the +// script, and the two cases below pin both consequences — no verdict below it is +// computed, and the dist-independent guards above it still speak. + +const SKILL = 'check-skill-examples.ts'; + +/** One marked, self-contained example under the sandbox's `skills/`. */ +function seedSkill(root: string, body: string[]): void { + const dir = path.join(root, 'skills'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, 'fixture.md'), + ['# Fixture skill', '', '', '```ts', ...body, '```', ''].join('\n'), + 'utf8', + ); +} + +describe('check:skill-examples refuses a stale dist (#7181)', () => { + it('POSITIVE CONTROL: a fresh dist type-checks the marked example and reports it', () => { + seed(tree.spec, { distMtime: NEW, srcMtime: OLD }); + seedSkill(tree.root, ['export const greeting: string = "ok";']); + + const run = runGate(tree.spec, SKILL); + expect(run.status).toBe(0); + expect(run.stdout).toContain('type-check against @objectstack/spec'); + expect(run.stderr).not.toContain('OLDER than'); + }, 60_000); + + it('refuses before tsc on a stale dist instead of reporting the examples type-check', () => { + seed(tree.spec, { distMtime: OLD, srcMtime: NEW }); + seedSkill(tree.root, ['export const greeting: string = "ok";']); + + const run = runGate(tree.spec, SKILL); + expectRefusal(run, 'pnpm --filter @objectstack/spec check:skill-examples'); + expect(run.stdout).not.toContain('type-check against @objectstack/spec'); + }, 60_000); + + it('lets the dist-independent guards above it speak first, even on a stale dist', () => { + // Deliberate ordering, and the reason this gate's guard is not at the top of + // `main()` like its siblings': an orphan marker checks nothing whatever the + // build state is, so reporting the freshness refusal in its place would trade + // a true finding for a build instruction. The freshness guard covers only what + // depends on the dist. + seed(tree.spec, { distMtime: OLD, srcMtime: NEW }); + const dir = path.join(tree.root, 'skills'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, 'orphan.md'), + ['# Fixture skill', '', '', '', '```ts', 'export const a = 1;', '```', ''].join('\n'), + 'utf8', + ); + + const run = runGate(tree.spec, SKILL); + expect(run.status).toBe(1); + expect(run.stderr).toContain('os:check marker not directly above'); + expect(run.stderr).not.toContain('OLDER than packages/spec/src'); + }, 60_000); +}); diff --git a/packages/spec/scripts/dist-freshness.test.ts b/packages/spec/scripts/dist-freshness.test.ts index 39a3b0cf3e..fef07de9f9 100644 --- a/packages/spec/scripts/dist-freshness.test.ts +++ b/packages/spec/scripts/dist-freshness.test.ts @@ -56,6 +56,13 @@ function write(rel: string, content: string, mtimeEpochSeconds: number): string const OLD = Math.floor(Date.now() / 1000) - 3600; const NEW = Math.floor(Date.now() / 1000) - 60; +// The caller's own re-run command. Passed rather than assumed since #7181: the +// wording used to name `api-surface` by hand, so the three gates that adopted +// this next each printed a fourth gate's name. `dist-freshness-adoption.test.ts` +// pins that each of them now prints its own. +const GEN_RERUN = 'pnpm --filter @objectstack/spec gen:api-surface'; +const CHECK_RERUN = 'pnpm --filter @objectstack/spec check:api-surface'; + beforeEach(() => { sandbox = fs.mkdtempSync(path.join(os.tmpdir(), 'os-dist-freshness-')); }); @@ -73,7 +80,7 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e write('dist/contracts/index.d.ts', 'export {};', OLD); write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW); - const verdict = inspectDistFreshness(sandbox, 'generate'); + const verdict = inspectDistFreshness(sandbox, 'generate', GEN_RERUN); expect(verdict.fresh).toBe(false); if (verdict.fresh) return; expect(verdict.state).toBe('stale'); @@ -88,7 +95,7 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e write('dist/contracts/index.d.ts', 'export {};', OLD); write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW); - const verdict = inspectDistFreshness(sandbox, 'check'); + const verdict = inspectDistFreshness(sandbox, 'check', CHECK_RERUN); expect(verdict.fresh).toBe(false); if (verdict.fresh) return; // The two modes must not print the same damage: one WRITES a wrong baseline, @@ -103,7 +110,7 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e write('dist/contracts/index.d.ts', 'export {};', OLD); write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW); - const verdict = inspectDistFreshness(sandbox, 'generate'); + const verdict = inspectDistFreshness(sandbox, 'generate', GEN_RERUN); if (verdict.fresh) throw new Error('expected a refusal'); expect(verdict.message).toContain('WRITE a baseline'); expect(verdict.message).toContain('BREAKING'); @@ -119,7 +126,7 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e // regress into something quieter. write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW); - const verdict = inspectDistFreshness(sandbox, 'generate'); + const verdict = inspectDistFreshness(sandbox, 'generate', GEN_RERUN); expect(verdict.fresh).toBe(false); if (verdict.fresh) return; expect(verdict.state).toBe('missing'); @@ -134,7 +141,7 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', OLD); write('dist/contracts/index.js', 'export {};', NEW); - const verdict = inspectDistFreshness(sandbox, 'generate'); + const verdict = inspectDistFreshness(sandbox, 'generate', GEN_RERUN); expect(verdict.fresh).toBe(false); if (verdict.fresh) return; expect(verdict.state).toBe('missing'); @@ -155,7 +162,7 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e write('dist/contracts/index.js', 'export {};', NEW); write('dist/.build-input-hash', `${'a'.repeat(64)}\n`, NEW); - const verdict = inspectDistFreshness(sandbox, 'generate'); + const verdict = inspectDistFreshness(sandbox, 'generate', GEN_RERUN); expect(verdict.fresh).toBe(false); if (verdict.fresh) return; expect(verdict.state).toBe('stale'); @@ -168,7 +175,24 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e write('dist/index.d.ts', 'export {};', OLD + 60); write('src/contracts/nested/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW); - expect(inspectDistFreshness(sandbox, 'generate').fresh).toBe(false); + expect(inspectDistFreshness(sandbox, 'generate', GEN_RERUN).fresh).toBe(false); + }); + + it('prescribes the CALLER of the moment, not a gate name baked into the wording (#7181)', () => { + // The defect #7181 was filed on. `mode` used to select `${gen|check}:api-surface` + // by hand, so every gate that adopted this primitive told its user to re-run + // a gate they had not run — and following that instruction re-runs something + // that was never refused, which reads as "the refusal cleared itself". + // + // Written with a name no gate in this repo has, so it cannot pass by + // coincidence with any real caller's string. + write('dist/contracts/index.d.ts', 'export {};', OLD); + write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW); + + const verdict = inspectDistFreshness(sandbox, 'check', 'pnpm run check:not-a-real-gate'); + if (verdict.fresh) throw new Error('expected a refusal'); + expect(verdict.message).toContain('pnpm run check:not-a-real-gate'); + expect(verdict.message).not.toContain('api-surface'); }); it('lets a dist NEWER than src through, in both modes', () => { @@ -179,8 +203,8 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', OLD); write('dist/contracts/index.d.ts', 'export {};', NEW); - expect(inspectDistFreshness(sandbox, 'generate')).toEqual({ fresh: true }); - expect(inspectDistFreshness(sandbox, 'check')).toEqual({ fresh: true }); + expect(inspectDistFreshness(sandbox, 'generate', GEN_RERUN)).toEqual({ fresh: true }); + expect(inspectDistFreshness(sandbox, 'check', CHECK_RERUN)).toEqual({ fresh: true }); }); }); diff --git a/packages/spec/scripts/lib/dist-freshness.ts b/packages/spec/scripts/lib/dist-freshness.ts index 5d90c9703a..00f4d5cd0d 100644 --- a/packages/spec/scripts/lib/dist-freshness.ts +++ b/packages/spec/scripts/lib/dist-freshness.ts @@ -71,6 +71,28 @@ * discarded: it still guards `pnpm dev`. It is simply blind to the half of the * dist that this generator is made of. Closing the `OS_SKIP_DTS` hole in the * stamp itself is a separate change to a separate script. + * + * ## Why the caller names ITSELF, and why that is not a third `mode` (#7181) + * + * #7181 adopted this in three more dist-reading gates and asked whether `mode` + * wants a third value. Measured against the code, it does not: `mode` is read in + * exactly two places, and only one of them is about semantics. + * + * - the DAMAGE sentence — "writing a wrong baseline" vs "agreeing with one". + * Those are the only two things a dist reader does, and all four call sites + * land in one of them (`check:dual-source-exports --update` regenerates a + * tracked baseline and is `generate`-shaped; the rest are `check`-shaped). + * - the RE-RUN command, which used to be spelled `${gen|check}:api-surface` by + * hand. That is not a semantic difference at all — it is the caller's own + * name, and hardcoding it made three adopted gates print a fourth gate's. + * + * A third value would therefore have to mean "check-shaped, but print a different + * command", i.e. text wearing a semantic label — and it would still be wrong for + * the fifth caller. So the name is a REQUIRED argument instead: the compiler makes + * every new caller state how to re-run itself, and there is no default to inherit + * the wrong gate's identity from. `--update` above is the reason it is a full + * command string rather than an npm script name — that path is not reachable + * through one. */ import { existsSync, readdirSync } from 'node:fs'; import { join } from 'node:path'; @@ -112,8 +134,18 @@ function hasDeclarations(dir: string, depth = 0): boolean { * Returns the verdict rather than exiting so the rule and its wording can be * driven from a test in both directions — a guard only ever observed green is * indistinguishable from one that matches nothing (#4690). + * + * @param mode what this run would do with the dist — see `DistReadMode`. + * @param rerun the exact command that re-runs THIS caller after the build, e.g. + * `pnpm --filter @objectstack/spec check:exported-any`. Required on purpose: + * a default would hand a new caller the previous gate's identity, which is the + * defect #7181 was filed for (three gates printing `check:api-surface`). */ -export function inspectDistFreshness(pkgDir: string, mode: DistReadMode): DistFreshness { +export function inspectDistFreshness( + pkgDir: string, + mode: DistReadMode, + rerun: string, +): DistFreshness { if (!distIsStale(pkgDir)) return { fresh: true }; const state: 'missing' | 'stale' = hasDeclarations(join(pkgDir, 'dist')) ? 'stale' : 'missing'; @@ -123,10 +155,10 @@ export function inspectDistFreshness(pkgDir: string, mode: DistReadMode): DistFr ? `Regenerating now would WRITE a baseline describing a build that no longer matches src:\n` + ` every export added since that build reads as a REMOVAL, and this generator's own rule\n` + ` calls a removed export a BREAKING change. The wrong baseline is then self-consistent —\n` + - ` check:api-surface compares it against the same stale dist and passes (#7122, #4687).` + ` the checking half compares it against the same stale dist and passes (#7122, #4687).` : `A verdict now would be computed against a build that no longer matches src, so this\n` + - ` check would report the public API "unchanged" without ever reading the exports under\n` + - ` test — a FALSE GREEN on exactly the change it exists to catch (#7122).`; + ` check would reach its conclusion without ever reading the declarations under test —\n` + + ` a FALSE GREEN on exactly the change it exists to catch (#7122).`; const cause = state === 'missing' @@ -143,8 +175,8 @@ export function inspectDistFreshness(pkgDir: string, mode: DistReadMode): DistFr ` ${damage}\n\n` + ` Build first, then re-run:\n\n` + ` pnpm --filter @objectstack/spec build\n` + - ` pnpm --filter @objectstack/spec ${mode === 'generate' ? 'gen' : 'check'}:api-surface\n\n` + - ` (Do NOT use OS_SKIP_DTS=1 for this one — AGENTS.md §9 names it as the flag that cannot\n` + - ` serve gen:api-surface.)`, + ` ${rerun}\n\n` + + ` (Do NOT use OS_SKIP_DTS=1 for this one — AGENTS.md §9 names it as the flag that emits JS\n` + + ` and skips exactly the declarations this reads.)`, }; }