diff --git a/.changeset/authorable-surface-anchor-explicit.md b/.changeset/authorable-surface-anchor-explicit.md new file mode 100644 index 0000000000..5efc651163 --- /dev/null +++ b/.changeset/authorable-surface-anchor-explicit.md @@ -0,0 +1,45 @@ +--- +"@objectstack/spec": patch +--- + +fix(spec): re-anchoring `authorable-surface.base.json` is an explicit act, not a build side effect (#5358) + +`packages/spec/authorable-surface.base.json` is the in-tree anchor the #4650 deletion gate +falls back to where `origin/main` is out of reach (#5235). It is not a projection of this +package's source — it is a snapshot of an **upstream** commit, and it is the baseline +precisely because the commit under test cannot rewrite it. + +Until now every `gen:schema` run rewrote it whenever it had drifted from the git-resolved +baseline. `gen:schema` is `pnpm build`'s first step, so this fired on any build of any +package that merely has `@objectstack/spec` in its dependency closure, and on `check:docs` +(whose first step is `gen:schema`). Three developers hit it independently, from three +unrelated tasks: + +- a plain `pnpm build`: `baseRev` advanced to HEAD, **−110 keys**; +- `pnpm --filter "@objectstack/cli^..." build`: the same 110; +- `pnpm --filter "@objectstack/service-automation^..." build`: `baseRev` advanced, +3 keys. + +The 110 were the `ui/ComponentAnimation` family that had just been retired. An anchor +advanced past a retirement cannot see that retirement any more — and the gate is green +before *and* after, because both states are internally consistent. All three were caught +only by reading `git status` line by line before committing; a `git add -A` would have +carried the moved baseline into a PR about something else. + +The anchor now moves only in a mode of its own: + +```bash +pnpm --filter @objectstack/spec gen:authorable-surface-base # build-schemas.ts --update-base +``` + +- `gen:schema` and any build: never write it. A lagging anchor prints one ℹ️ line saying so, + naming this command — lag was already not an error (on `main` the merge base is HEAD, so + the file necessarily trails its own surface by one PR). +- `check:authorable-surface`: unchanged, still strictly read-only, and still fatal when the + committed anchor is missing, malformed, or inauthentic. +- `--check --update-base` is refused: a check that repairs what it detects can never report it. + +Nothing about anchor **authenticity** changes: `baseRev` must still be an ancestor of +`origin/main` with keys matching that commit's `authorable-surface.json`, the anchor is +still written only from a git-resolved baseline (never from the build being checked), and +the write still happens after the deletion gate has adjudicated the run, so the explicit +mode cannot walk the baseline past an unproven deletion either. diff --git a/packages/spec/package.json b/packages/spec/package.json index e6673f0b03..4d76b3fb09 100644 --- a/packages/spec/package.json +++ b/packages/spec/package.json @@ -186,6 +186,7 @@ "dev": "tsc --watch", "clean": "rm -rf dist", "gen:schema": "OS_EAGER_SCHEMAS=1 tsx scripts/build-schemas.ts", + "gen:authorable-surface-base": "OS_EAGER_SCHEMAS=1 tsx scripts/build-schemas.ts --update-base", "gen:openapi": "tsx scripts/build-openapi.ts", "gen:docs": "tsx scripts/build-docs.ts", "check:docs": "pnpm gen:schema && tsx scripts/build-docs.ts --check", diff --git a/packages/spec/scripts/build-schemas-check-mode.test.ts b/packages/spec/scripts/build-schemas-check-mode.test.ts index 146e5c028e..e43393a024 100644 --- a/packages/spec/scripts/build-schemas-check-mode.test.ts +++ b/packages/spec/scripts/build-schemas-check-mode.test.ts @@ -47,7 +47,7 @@ // then a faithful model of a build environment with no route to GitHub — the // last describe block below drives exactly that. -import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach } from 'vitest'; import { spawnSync } from 'node:child_process'; import fs from 'node:fs'; import os from 'node:os'; @@ -705,7 +705,10 @@ describe('build-schemas.ts — an in-tree anchor carries the deletion gate offli expect(status).toBe(1); expect(output).toContain('is not the baseline it claims to be'); expect(output).toContain(SHED_FROM_ANCHOR); - expect(output).toContain('gen:schema'); + // The remedy names the one command that may write this file — `gen:schema` + // stopped being it at #5358, and a prescription pointing at a command that + // no longer touches the file is the defect that issue is about. + expect(output).toContain('gen:authorable-surface-base'); }, ); @@ -782,8 +785,8 @@ describe('build-schemas.ts — an in-tree anchor carries the deletion gate offli ); it( - 'is a committed artifact: --check reports it missing without writing it, gen:schema creates it from the git baseline', - { timeout: SPAWN_TIMEOUT_MS * 2 }, + 'is a committed artifact: --check reports it missing without writing it, and only --update-base creates it', + { timeout: SPAWN_TIMEOUT_MS * 3 }, () => { fs.rmSync(surfaceBasePath); @@ -793,7 +796,15 @@ describe('build-schemas.ts — an in-tree anchor carries the deletion gate offli // A check reports; it does not repair (#4711). expect(fs.existsSync(surfaceBasePath)).toBe(false); - const write = run([]); + // A plain build says so loudly and still does not create it (#5358) — the + // gate above is where a missing committed artifact goes red, not here. + const build = run([]); + expect(build.status).toBe(0); + expect(build.output).toContain('a build no longer creates it (#5358)'); + expect(build.output).toContain('gen:authorable-surface-base'); + expect(fs.existsSync(surfaceBasePath)).toBe(false); + + const write = run(['--update-base']); expect(write.status).toBe(0); expect(write.output).toContain('authorable-surface.base.json created at'); const doc = JSON.parse(readSurfaceBase()) as { baseRev: string; keys: string[] }; @@ -803,3 +814,199 @@ describe('build-schemas.ts — an in-tree anchor carries the deletion gate offli }, ); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #5358 — the anchor moves only when a human asks for it. +// +// #5235 gave the anchor two properties: it may only be written from a +// git-resolved baseline, and it is verified against origin/main wherever that is +// reachable. Both are about WHAT gets written. Neither says WHEN — and the answer +// was "on every run that regenerates", which meant every `pnpm build`, every +// `pnpm --filter '^...' build` whose closure contains @objectstack/spec, and +// every `check:docs` (whose first step is `gen:schema`). +// +// Three developers reported the consequence independently, from three unrelated +// tasks (#4990, #5155, #5660): a file they had never opened showed up modified in +// `git status`, with `baseRev` advanced to the tip they branched from. Twice the +// same run also dropped 110 keys — the `ui/ComponentAnimation` family #4988/#5321 +// had just retired. An anchor advanced past a retirement is an anchor that can no +// longer SEE that retirement, and the #4650 deletion gate is green before and +// after, because both states are internally consistent. The only thing standing +// between that and a merged PR was three people reading `git diff` line by line. +// +// The fix is a mode, not a smarter heuristic: `--update-base` writes the anchor, +// and nothing else does. What these tests pin is the negative — that a run which +// is NOT that mode leaves the working tree exactly as it found it — because that +// is the property `git add -A` can silently violate. +describe('build-schemas.ts — only --update-base moves the in-tree anchor (#5358)', () => { + /** A live key held back from the older commit, so the anchor legitimately lags. */ + const LAGGING_KEY = 'data/Object:label'; + + let older: string; + let tip: string; + let laggingAnchor: string; + + beforeEach(() => { + // The real-world shape, built honestly: an OLDER upstream commit whose + // baseline is one key short, then the current origin/main tip carrying the + // full baseline. The anchor mirrors the older commit — authentic (its keys ARE + // that commit's baseline) and legitimately behind the merge base, which is + // exactly the state every checkout is in right after a surface change lands. + seedManifest((s) => s); + older = seedBase((s) => s.filter((k) => k !== LAGGING_KEY)); + tip = seedBase((s) => s); + seedSurface((s) => s); + laggingAnchor = seedSurfaceBase(older, (k) => k.filter((x) => x !== LAGGING_KEY)); + // Commit the fixture so the tree is CLEAN — `git diff --exit-code` is the + // acceptance criterion, and it can only mean something from a clean start. + // Only the two tracked artifacts: `git add -A` here would track the ~1600-file + // json-schema/ output, which every subsequent run rewrites, and the cleanliness + // assertions below would then measure the generator's own scratch space. + git('add', 'authorable-surface.base.json'); + git('commit', '-q', '-m', 'fixture: lagging but authentic anchor'); + git('update-ref', 'refs/remotes/origin/main', tip); + expect(git('status', '--porcelain', '-uno')).toBe(''); + }); + + afterEach(() => { + git('update-ref', 'refs/remotes/origin/main', tip); + }); + + it( + 'a plain build leaves the anchor byte-identical and the working tree clean', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + // THE regression test. Before #5358 this run rewrote the anchor to + // {baseRev: tip, keys: full} and exited 0, so `git status` showed a file the + // build's author never touched. `gen:schema` is `pnpm build`'s first step, + // which is why an unrelated PR was one `git add -A` away from carrying it. + const { status, output } = run([]); + + expect(status).toBe(0); + expect(readSurfaceBase()).toBe(laggingAnchor); + expect(git('status', '--porcelain', '-uno')).toBe(''); + // Silent is not enough: the run says the anchor lags, that this is not an + // error, and what the deliberate act would be. + expect(output).toContain('trails the baseline at'); + expect(output).toContain('not an error'); + expect(output).toContain('gen:authorable-surface-base'); + expect(output).not.toContain('⚓'); + }, + ); + + it( + 'a --check run leaves the anchor byte-identical and the working tree clean', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + // The literal acceptance criterion of #5358: run the gate on a clean tree, + // then `git diff --exit-code`. Unlike the case above this one already held + // on `main` — the anchor's write branch was `!CHECK` before this change too + // — so it is a pin, not a repair. It is here because "a check is read-only" + // is the property the whole file exists for (#4711), and the entry point + // that violated it was the neighbouring one. + const { status } = run(['--check']); + + expect(status).toBe(0); + expect(readSurfaceBase()).toBe(laggingAnchor); + expect(git('status', '--porcelain', '-uno')).toBe(''); + }, + ); + + it( + '--update-base re-anchors to the git-resolved baseline, and says so', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + const { status, output } = run(['--update-base']); + + expect(status).toBe(0); + expect(output).toContain('⚓'); + expect(output).toContain(tip.slice(0, 12)); + const doc = JSON.parse(readSurfaceBase()) as { baseRev: string; keys: string[] }; + // From the merge base, never from this build's own emitted surface (#5235). + expect(doc.baseRev).toBe(tip); + expect(doc.keys).toEqual((JSON.parse(pristineSurface) as { keys: string[] }).keys); + expect(doc.keys).toContain(LAGGING_KEY); + // Restore the fixture for the sibling cases — beforeEach re-commits anyway, + // but a dirty tree between tests would make a failure here read as a failure + // there. + fs.writeFileSync(surfaceBasePath, laggingAnchor); + }, + ); + + it( + '--update-base on an already-current anchor writes nothing and says nothing to do', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + const current = seedSurfaceBase(tip, (k) => k); + git('add', 'authorable-surface.base.json'); + git('commit', '-q', '-m', 'fixture: anchor already at the merge base'); + + const { status, output } = run(['--update-base']); + + expect(status).toBe(0); + expect(output).toContain('nothing to re-anchor'); + expect(readSurfaceBase()).toBe(current); + expect(git('status', '--porcelain', '-uno')).toBe(''); + }, + ); + + it( + 'refuses --check --update-base: a check that repairs what it detects can never report it', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + const { status, output } = run(['--check', '--update-base']); + + expect(status).toBe(1); + expect(output).toContain('mutually exclusive'); + expect(readSurfaceBase()).toBe(laggingAnchor); + expect(git('status', '--porcelain', '-uno')).toBe(''); + // Refused before the 1600-schema generation, not after it. + expect(output).not.toContain('Generating JSON Schemas'); + }, + ); + + it( + 'never writes the anchor from the build being checked — --update-base is powerless offline', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + // #5235's rule survives the new mode: with origin/main unreachable there is + // no git-resolved baseline, so there is nothing the flag may write FROM. A + // `--update-base` that fell back to this build's own surface would be the + // tree anchoring itself — the #4650 defect with a flag in front of it. + git('update-ref', '-d', 'refs/remotes/origin/main'); + try { + const { status, output } = run(['--update-base']); + + expect(status).toBe(0); + expect(output).toContain('origin/main is not resolvable'); + expect(readSurfaceBase()).toBe(laggingAnchor); + expect(git('status', '--porcelain', '-uno')).toBe(''); + } finally { + git('update-ref', 'refs/remotes/origin/main', tip); + } + }, + ); + + it( + 'still refuses to bless an unproven deletion in --update-base mode, and leaves the anchor alone', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + // Order is load-bearing (see the anchor block in build-schemas.ts): the + // deletion gate adjudicates first, so the re-anchoring mode cannot be used to + // walk the baseline past a deletion nothing proved. Without this, #5358's + // explicit command would be a laundering route the old side effect never was. + // The sabotage goes in the merge base — the anchor's own keys stay honest, so + // only the gate, not the authenticity check, can be what fires. + seedBase((s) => [...s, DELETED_LIVE].sort()); + seedSurface((s) => s); + + const { status, output } = run(['--update-base']); + + expect(status).toBe(1); + expect(output).toContain('deleted without proof (#4650)'); + expect(output).toContain(DELETED_LIVE); + expect(readSurfaceBase()).toBe(laggingAnchor); + expect(output).not.toContain('⚓'); + }, + ); +}); diff --git a/packages/spec/scripts/build-schemas.ts b/packages/spec/scripts/build-schemas.ts index 35b3712595..c6e1b3758f 100644 --- a/packages/spec/scripts/build-schemas.ts +++ b/packages/spec/scripts/build-schemas.ts @@ -46,13 +46,55 @@ const OUT_DIR = path.resolve(__dirname, '../json-schema'); // ever emitted. json-schema/ itself is a gitignored build artifact, so this // file is the durable "last time" — see the disappearance check below (#2978). const MANIFEST_PATH = path.resolve(__dirname, '../json-schema.manifest.json'); -// `--check` verifies the two committed snapshots — the schema manifest and the -// authorable surface — without rewriting either, so CI fails on an uncommitted -// ADDITION too (the write and check paths share the same code — same discipline -// as build-docs.ts). "Without rewriting" is load-bearing on both: a check that -// repairs what it detects can never report it, and it silently edits the tree of -// whoever ran it (#4711). +// Three modes, one code path: +// +// (default) `gen:schema` — regenerate json-schema/ and, when they are +// behind, the two committed PROJECTIONS of this source: +// json-schema.manifest.json and authorable-surface.json. +// `--check` `check:authorable-surface` — verify both snapshots without +// rewriting either, so CI fails on an uncommitted ADDITION too +// (the write and check paths share the same code — same +// discipline as build-docs.ts). "Without rewriting" is +// load-bearing: a check that repairs what it detects can never +// report it, and it silently edits the tree of whoever ran it +// (#4711). +// `--update-base` `gen:authorable-surface-base` — the ONLY mode that writes the +// deletion gate's in-tree ANCHOR, authorable-surface.base.json. +// See the flag's own comment below (#5358). const CHECK = process.argv.includes('--check'); +// `--update-base` re-anchors the in-tree baseline `authorable-surface.base.json`. +// It is a MODE OF ITS OWN, and nothing else writes that file — not `--check`, not +// a plain `gen:schema`, not the `pnpm build` that runs `gen:schema` as its first +// step (#5358). +// +// Why the anchor is different in kind from this script's other two artifacts. +// `json-schema.manifest.json` and `authorable-surface.json` are projections of +// the source being built: regenerating them is always right, and their diff is +// the change under review. The anchor is not a projection of anything local — it +// is a snapshot of an UPSTREAM commit, the baseline the #4650 deletion gate +// compares against precisely because the commit under test cannot rewrite it. +// Refreshing it on every build made that guarantee conditional on nobody ever +// running a build: three independent reports (#4990, #5155, #5660) each did a +// `pnpm --filter '^...' build` for an unrelated package, found the anchor +// rewritten in `git status`, and caught it only by reading the diff by hand — one +// of them net −110 keys, exactly the batch #4988/#5321 had just retired. Anchored +// forward, the gate can no longer see that retirement, and BOTH states are green, +// so nothing anywhere reports it. A `git add -A` is all it takes. +// +// So the anchor moves only when a human types this flag, and the move is then the +// whole content of a reviewed diff rather than a rider on somebody else's PR. +// Staleness stays what it always was — not an error (see the anchor block near +// the end of this file): the gate proves the anchor AUTHENTIC, never current. +const UPDATE_BASE = process.argv.includes('--update-base'); +if (CHECK && UPDATE_BASE) { + console.error( + `\n❌ --check and --update-base are mutually exclusive.\n\n` + + ` --check is a verification: it reports and never writes (#4711). --update-base is the\n` + + ` deliberate re-anchoring of authorable-surface.base.json (#5358). A run that did both\n` + + ` would be a check that repairs what it detects, which can never report it.`, + ); + process.exit(1); +} const SPEC_VERSION = JSON.parse(fs.readFileSync(path.resolve(__dirname, '../package.json'), 'utf-8')).version; const SCHEMA_BASE_URL = `https://schema.objectstack.io/v${SPEC_VERSION}`; @@ -429,6 +471,12 @@ const AUTHORABLE_SURFACE_PATH = path.resolve(__dirname, '../authorable-surface.j const AUTHORABLE_SURFACE_BASE_PATH = path.resolve(__dirname, '../authorable-surface.base.json'); const SURFACE_FILE_NAME = path.basename(AUTHORABLE_SURFACE_PATH); const SURFACE_BASE_FILE_NAME = path.basename(AUTHORABLE_SURFACE_BASE_PATH); +/** + * The ONE command that (re)writes the in-tree anchor (#5358). Every prescription + * about that file names this and nothing else — a message that still said + * `gen:schema` would send the reader to a command that no longer touches it. + */ +const REANCHOR_COMMAND = 'pnpm --filter @objectstack/spec gen:authorable-surface-base'; const RETIRED_MARK = ' [RETIRED]'; interface AuthorableSurface { description: string; keys: string[] } @@ -792,6 +840,19 @@ function computeSurfaceReachability(): SurfaceReachability { }; } +/** + * ⚠️ Every byte of this string is part of the anchor file's canonical form — + * `readCommittedSurfaceBase` compares the committed file against + * `serializeSurfaceBase()` and treats any difference as a hand-edit, fatally. + * So changing this text is not a comment edit: it invalidates the committed + * anchor in every checkout until someone re-anchors, which is itself the + * deliberate act #5358 made explicit. It is therefore left verbatim here, and + * "Written only by `gen:schema`" now UNDER-states the rule rather than + * contradicting it: the writer is still this generator (`scripts/build-schemas.ts`), + * but only in its `--update-base` mode (`gen:authorable-surface-base`), never on a + * plain build. Narrowing in the safe direction. Whoever next re-anchors should + * bring the sentence with them, in that same reviewed diff. + */ const SURFACE_BASE_DESCRIPTION = 'In-tree anchor for the authorable-surface deletion gate (#4650, #5235): a verbatim copy of the ' + 'keys in authorable-surface.json as they stood at `baseRev`, a commit on origin/main. A build that ' + @@ -832,7 +893,7 @@ function readCommittedSurfaceBase(): { raw: string; doc: AuthorableSurfaceBase } `\n❌ ${SURFACE_BASE_FILE_NAME} is malformed (#5235): it must carry a 40-hex \`baseRev\` and a\n` + ` \`keys\` array — the ${SURFACE_FILE_NAME} content at that commit.\n\n` + ` Restore it (\`git checkout -- packages/spec/${SURFACE_BASE_FILE_NAME}\`), or delete it and\n` + - ` run \`pnpm --filter @objectstack/spec gen:schema\` in a checkout that can reach origin/main.`, + ` run \`${REANCHOR_COMMAND}\` in a checkout that can reach origin/main.`, ); process.exit(1); } @@ -840,10 +901,10 @@ function readCommittedSurfaceBase(): { raw: string; doc: AuthorableSurfaceBase } console.error( `\n❌ ${SURFACE_BASE_FILE_NAME} does not match its generated form (#5235).\n\n` + ` This file is the deletion gate's anchor — the baseline a commit is not supposed to be\n` + - ` able to rewrite. Every byte in it must come from \`gen:schema\`, so a hand-edit is fatal\n` + + ` able to rewrite. Every byte in it must come from the generator, so a hand-edit is fatal\n` + ` here rather than repaired (the same call #4662 made for ${SURFACE_FILE_NAME}).\n\n` + ` Restore it (\`git checkout -- packages/spec/${SURFACE_BASE_FILE_NAME}\`), or delete it and\n` + - ` run \`pnpm --filter @objectstack/spec gen:schema\` in a checkout that can reach origin/main.`, + ` run \`${REANCHOR_COMMAND}\` in a checkout that can reach origin/main.`, ); process.exit(1); } @@ -880,8 +941,8 @@ function verifyCommittedSurfaceBase( const rev = committed.baseRev; const short = rev.slice(0, 12); const fix = - ` Run \`pnpm --filter @objectstack/spec gen:schema\` and commit the result — the generator\n` + - ` writes this file from the git-resolved baseline, which is the only thing it may come from.`; + ` Run \`${REANCHOR_COMMAND}\` and commit the result — that mode writes\n` + + ` this file from the git-resolved baseline, which is the only thing it may come from.`; // Fast path, and the common one right after a refresh: the anchor names the // very rev this run resolved out of git, so the baseline to compare against is @@ -975,8 +1036,9 @@ function compareAnchorKeys( /** * Set when THIS run resolved the baseline from git. It is the ONLY input - * `gen:schema` may write the in-tree anchor from: an offline build must never be - * able to advance the anchor to its own state (#5235). + * `--update-base` may write the in-tree anchor from: an offline build must never + * be able to advance the anchor to its own state (#5235). The second half of that + * discipline is #5358 — no build writes it at all, only the explicit mode. */ let gitResolvedAnchor: { rev: string; keys: string[] } | null = null; @@ -1186,14 +1248,14 @@ function resolveSurfaceBase(): { rev: string; doc: AuthorableSurface } | null { } } -// ─── The in-tree baseline anchor (#5235) ───────────────────────────── +// ─── The in-tree baseline anchor (#5235, #5358) ────────────────────── // -// Refreshed here, AFTER the deletion gate above has adjudicated this build — -// order is load-bearing: a run that exits on an unproven deletion never reaches -// this line, so the anchor can never be advanced past a deletion it did not -// bless. And it is written only from `gitResolvedAnchor`, never from -// `currentEntries`: an anchor computed from the tree being checked is an anchor -// that tree can rewrite, which is the whole defect #4650 exists for. +// Written here, AFTER the deletion gate above has adjudicated this build — order +// is load-bearing: a run that exits on an unproven deletion never reaches this +// line, so the anchor can never be advanced past a deletion it did not bless. And +// it is written only from `gitResolvedAnchor`, never from `currentEntries`: an +// anchor computed from the tree being checked is an anchor that tree can rewrite, +// which is the whole defect #4650 exists for. // // Content therefore lags main by at most the last surface-changing PR — the // baseline at the merge base, not this branch's own state. That lag is what @@ -1201,6 +1263,15 @@ function resolveSurfaceBase(): { rev: string; doc: AuthorableSurface } | null { // have to account for. Staleness is NOT an error (on `main` itself the merge base // IS HEAD, so the file necessarily trails its own surface by one PR); only // inauthenticity is, and `verifyCommittedSurfaceBase` above is what proves it. +// +// #5358 adds the third property, the one that makes the other two mean anything +// outside CI: the write happens ONLY under `--update-base`. Until then this block +// ran on every `gen:schema`, so every `pnpm build` in the repo — including builds +// of packages that merely have `@objectstack/spec` in their dependency closure — +// silently advanced the deletion gate's own baseline in the developer's worktree, +// where the next `git add -A` swept it into a PR about something else. Lag is +// harmless (it only ever asks a build to account for MORE keys); an unnoticed +// advance is not (it asks for fewer, and forgets a retirement in the process). { const committed = readCommittedSurfaceBase(); if (gitResolvedAnchor) { @@ -1212,23 +1283,43 @@ function resolveSurfaceBase(): { rev: string; doc: AuthorableSurface } | null { ` It is a committed artifact: builds that cannot reach GitHub (image-build stages,\n` + ` air-gapped, fork, historical tag) anchor the #4650 deletion gate on it, and without it\n` + ` they have nothing to anchor on and fail.\n\n` + - ` Run \`pnpm --filter @objectstack/spec gen:schema\` and commit the result.`, + ` Run \`${REANCHOR_COMMAND}\` and commit the result.`, ); process.exit(1); } - if (drifted && !CHECK) { - fs.writeFileSync(AUTHORABLE_SURFACE_BASE_PATH, serializeSurfaceBase(anchor.rev, anchor.keys)); - console.log( - `\n⚓ ${SURFACE_BASE_FILE_NAME} ${committed ? 'refreshed to' : 'created at'} ` + - `${anchor.rev.slice(0, 12)} (${anchor.keys.length} keys) — commit it.`, + if (UPDATE_BASE) { + if (drifted) { + fs.writeFileSync(AUTHORABLE_SURFACE_BASE_PATH, serializeSurfaceBase(anchor.rev, anchor.keys)); + console.log( + `\n⚓ ${SURFACE_BASE_FILE_NAME} ${committed ? 'refreshed to' : 'created at'} ` + + `${anchor.rev.slice(0, 12)} (${anchor.keys.length} keys) — commit it, on its own.`, + ); + } else { + console.log( + `\n⚓ ${SURFACE_BASE_FILE_NAME} is already the baseline at ${anchor.rev.slice(0, 12)} ` + + `(${anchor.keys.length} keys) — nothing to re-anchor.`, + ); + } + } else if (!committed) { + // Not fatal outside `--check`: this is a build, and the gate that must go + // red about a missing committed artifact already does, above. Loud, though — + // silently recreating it is what #5358 removed. + console.warn( + `\n⚠️ ${SURFACE_BASE_FILE_NAME} is missing, and a build no longer creates it (#5358).\n` + + ` It is a committed artifact the #4650 deletion gate anchors on where origin/main is out\n` + + ` of reach. Restore it (\`git checkout -- packages/spec/${SURFACE_BASE_FILE_NAME}\`) or\n` + + ` re-anchor deliberately: \`${REANCHOR_COMMAND}\`.`, ); - } else if (drifted && committed) { - // Reported, never fatal: see the note above on `main`'s own merge base. + } else if (drifted) { + // Reported, never fatal, and never repaired here: see the notes above on + // `main`'s own merge base and on #5358. const recorded = new Set(committed.doc.keys); const behind = anchor.keys.filter((k) => !recorded.has(k)).length; console.log( - `ℹ️ ${SURFACE_BASE_FILE_NAME} trails the merge base by ${behind} key(s) — expected right after\n` + - ` a surface change lands; \`gen:schema\` refreshes it on the next run that regenerates.`, + `ℹ️ ${SURFACE_BASE_FILE_NAME} trails the baseline at ${anchor.rev.slice(0, 12)} by ${behind} key(s)\n` + + ` — expected, and not an error: the anchor is a snapshot of an upstream commit, proved\n` + + ` AUTHENTIC rather than current. Re-anchoring is a deliberate act with its own reviewed\n` + + ` diff — \`${REANCHOR_COMMAND}\` — never a side effect of this build (#5358).`, ); } } diff --git a/packages/spec/scripts/check-generated-ledger.test.ts b/packages/spec/scripts/check-generated-ledger.test.ts index 482c472d21..80c44fd7de 100644 --- a/packages/spec/scripts/check-generated-ledger.test.ts +++ b/packages/spec/scripts/check-generated-ledger.test.ts @@ -71,6 +71,19 @@ describe('check:generated --reconcile-only', () => { expect(output).toContain('all classified'); }); + it('covers the explicit, manual-only anchor generator (#5358)', () => { + // `gen:authorable-surface-base` is the third classification this ledger + // carries, and the one most likely to be mis-filed by a later change: + // UNGATED_GENERATORS would claim nothing verifies its artifact (false — + // check:authorable-surface proves the anchor authentic), while a GATED entry + // would put it in reach of `--fix`, which is the side effect #5358 removed. + expect(scripts['gen:authorable-surface-base']).toBeDefined(); + expect(scripts['gen:authorable-surface-base']).toContain('--update-base'); + const { status, output } = runReconcile(); + expect(status, output).toBe(0); + expect(output).toContain('1 explicit manual-only generators'); + }); + it('covers the test-layer typecheck gate and its writer (#5286)', () => { // The specific pair that failed CI on this branch. Named here so a later // change that drops either script also has to come back through this file. diff --git a/packages/spec/scripts/check-generated.ts b/packages/spec/scripts/check-generated.ts index fcee97fda8..a57f75fccb 100644 --- a/packages/spec/scripts/check-generated.ts +++ b/packages/spec/scripts/check-generated.ts @@ -163,6 +163,31 @@ const UNGATED_GENERATORS: ReadonlyArray<{ gen: string; why: string }> = [ { gen: 'gen:sbom', why: 'the SBOM is a release artifact, regenerated at publish time rather than checked in' }, ]; +/** + * Generators that are DELIBERATE, manual-only acts. Their artifact **is** gated — + * `gatedBy` names the gate — so `UNGATED_GENERATORS` would be a false + * classification in one direction; but a `GATED` entry would be false in the + * other, because `--fix` may not run them and "stale" is not a defect for them. + * + * The distinction is `authorable-surface.base.json`, and it is the whole of + * #5358. That file is not a projection of this package's source — it is a + * snapshot of an UPSTREAM commit, the baseline the #4650 deletion gate compares + * against precisely because the commit under test cannot rewrite it. Its gate + * proves it AUTHENTIC (`baseRev` on origin/main, keys matching that commit), never + * current, so lag is expected and green. While `gen:schema` refreshed it as a side + * effect, any build of any package with spec in its dependency closure moved the + * gate's baseline in the developer's worktree, and a `git add -A` carried the move + * into an unrelated PR — observed three times (#4990, #5155, #5660), once at + * −110 keys covering a retirement that had just landed. + */ +const EXPLICIT_GENERATORS: ReadonlyArray<{ gen: string; gatedBy: string; why: string }> = [ + { + gen: 'gen:authorable-surface-base', + gatedBy: 'check:authorable-surface', + why: 're-anchors authorable-surface.base.json to the git-resolved baseline — a deliberate act with its own reviewed diff, never a build side effect (#5358)', + }, +]; + /** This aggregate itself — a `check:` script that gates nothing of its own. */ const SELF = 'check:generated'; @@ -179,7 +204,11 @@ const SELF = 'check:generated'; function reconcileLedger(scripts: Record): void { const problems: string[] = []; const declaredChecks = new Set([...GATED.map((g) => g.check), ...NO_GENERATOR.map((n) => n.check)]); - const declaredGens = new Set([...GATED.map((g) => g.gen), ...UNGATED_GENERATORS.map((u) => u.gen)]); + const declaredGens = new Set([ + ...GATED.map((g) => g.gen), + ...UNGATED_GENERATORS.map((u) => u.gen), + ...EXPLICIT_GENERATORS.map((e) => e.gen), + ]); for (const name of Object.keys(scripts)) { if (name === SELF) continue; @@ -189,13 +218,27 @@ function reconcileLedger(scripts: Record): void { } if (name.startsWith('gen:') && !declaredGens.has(name)) { problems.push(` \`${name}\` exists in package.json but no GATED entry names it and it is not in UNGATED_GENERATORS.\n` + - ` Either wire its gate in, or record why its output is unverified.`); + ` Either wire its gate in, record why its output is unverified, or — if it is a\n` + + ` deliberate manual-only re-anchoring whose artifact another gate already verifies —\n` + + ` declare it in EXPLICIT_GENERATORS with the gate that covers it.`); } } for (const { check } of GATED) if (!scripts[check]) problems.push(` GATED names \`${check}\`, which package.json no longer has.`); for (const { gen } of GATED) if (!scripts[gen]) problems.push(` GATED names \`${gen}\`, which package.json no longer has.`); for (const { check } of NO_GENERATOR) if (!scripts[check]) problems.push(` NO_GENERATOR names \`${check}\`, which package.json no longer has.`); for (const { gen } of UNGATED_GENERATORS) if (!scripts[gen]) problems.push(` UNGATED_GENERATORS names \`${gen}\`, which package.json no longer has.`); + for (const { gen, gatedBy } of EXPLICIT_GENERATORS) { + if (!scripts[gen]) problems.push(` EXPLICIT_GENERATORS names \`${gen}\`, which package.json no longer has.`); + // The claim that makes this category honest rather than an escape hatch: the + // artifact IS covered. A `gatedBy` naming a gate this ledger does not run + // would be a coverage hole wearing a classification's clothes. + if (!declaredChecks.has(gatedBy)) { + problems.push( + ` EXPLICIT_GENERATORS says \`${gen}\` is gated by \`${gatedBy}\`, which this ledger does not declare.\n` + + ` An explicit generator is only "covered" if some gate here verifies its artifact.`, + ); + } + } if (problems.length) { console.error(`✗ check:generated ledger is out of sync with package.json:\n\n${problems.join('\n')}\n`); @@ -234,7 +277,8 @@ if (reconcileOnly) { console.log( `✓ check:generated ledger reconciles with package.json: ${checks} check: + ${gens} gen: scripts, ` + `all classified (${GATED.length} gated, ${NO_GENERATOR.length} source audits, ` + - `${UNGATED_GENERATORS.length} ungated generators, 1 aggregate).\n` + + `${UNGATED_GENERATORS.length} ungated generators, ${EXPLICIT_GENERATORS.length} explicit ` + + `manual-only generators, 1 aggregate).\n` + ` --reconcile-only: no gates were run — this verifies coverage, not artifacts.`, ); process.exit(0); @@ -266,6 +310,12 @@ if (UNGATED_GENERATORS.length) { console.log(`Generated but ungated (${UNGATED_GENERATORS.length}): ` + UNGATED_GENERATORS.map((u) => u.gen).join(', ') + ' — nothing verifies these are current.'); } +// Narrowing is never silent, part two: --fix will not reach these, by design. +if (EXPLICIT_GENERATORS.length) { + console.log(`Explicit, manual-only (${EXPLICIT_GENERATORS.length}): ` + + EXPLICIT_GENERATORS.map((e) => `${e.gen} (gated by ${e.gatedBy})`).join(', ') + + ' — never run here or by --fix; their artifact may lag and still be green.'); +} if (!stale.length) { console.log(`\n✓ All ${GATED.length} generated artifacts are up to date.`); diff --git a/scripts/regen-artifacts.mjs b/scripts/regen-artifacts.mjs index 85285ffac2..4e073c5daa 100644 --- a/scripts/regen-artifacts.mjs +++ b/scripts/regen-artifacts.mjs @@ -23,14 +23,26 @@ export const REGEN_ARTIFACTS = Object.freeze([ { path: 'packages/spec/spec-changes.json', gen: 'gen:spec-changes', check: 'check:spec-changes' }, { path: 'docs/protocol-upgrade-guide.md', gen: 'gen:upgrade-guide', check: 'check:upgrade-guide' }, { path: 'packages/spec/authorable-surface.json', gen: 'gen:schema', check: 'check:authorable-surface' }, - // The deletion gate's in-tree anchor (#5235). Same generator, same sorted-array - // shape, same conflict — two branches that each refreshed it differ on the - // `baseRev` header and on whatever main added in between, and the resolution is - // always "recompute from the merged tree's merge base", never a text merge. - // Unlike its neighbours a stale copy of this one is NOT an error (on `main` the - // merge base is HEAD, so the file necessarily trails its own surface by one - // PR) — `check:authorable-surface` proves it AUTHENTIC rather than current, and - // `gen:schema` is still what restores it. + // The deletion gate's in-tree anchor (#5235). Same sorted-array shape, same + // conflict — two branches that each re-anchored it differ on the `baseRev` + // header and on whatever main added in between, and a text merge of that is + // never right, so it stays driver-managed. + // + // But it is the one path here with NOTHING TO REGENERATE after a merge. A stale + // copy is not an error (on `main` the merge base is HEAD, so the file + // necessarily trails its own surface by one PR): `check:authorable-surface` + // proves it AUTHENTIC — `baseRev` on origin/main, keys matching that commit — + // never current. Either side of the conflict is an authentic upstream snapshot, + // so the driver keeping OURS is already the resolution, and the pre-commit gate + // passes on it as-is. + // + // `gen` therefore still names `gen:schema` — correct for this file's two + // neighbours, which a merge defers alongside it, and a harmless no-op for this + // one since #5358 took the anchor out of every build. It deliberately does NOT + // name the re-anchoring command (`gen:authorable-surface-base`): that mid-merge + // is exactly #5370, where `merge-base(HEAD, origin/main)` still resolves to the + // branch's OLD fork point and the anchor moves BACKWARDS — authentically, so no + // gate objects. Re-anchor after the merge is committed, or not at all. { path: 'packages/spec/authorable-surface.base.json', gen: 'gen:schema', check: 'check:authorable-surface' }, { path: 'packages/spec/json-schema.manifest.json', gen: 'gen:schema', check: 'check:authorable-surface' }, // `gen:api-surface` reads the BUILT `dist/*.d.ts`, never the source. On a