From 65f85cba0e4c1961ac4be3b5d8a858dba7f5d17f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 22:12:15 +0000 Subject: [PATCH] fix(spec): --check must not write json-schema.manifest.json (#4711) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The manifest ratchet in scripts/build-schemas.ts had no CHECK discriminator. `check:authorable-surface` (build-schemas.ts --check) — one of the eight generated-artifact gates `check:generated` runs — recomputed the emitted schema set and, on any addition or renamed-away def, rewrote the tracked json-schema.manifest.json in place and exited 0. Two defects, one missing `if`: 1. A check edited the working tree. Whatever the file held locally was overwritten by a command whose entire job is to look, which is how a `git stash pop` / worktree / merge-conflict operation fails for a reason nobody traces back to a gate. It is the #4675 merge-driver trap from the other side too: run any check mid-merge and a manifest computed from a half-merged tree lands on disk. 2. The additions branch could never go red in CI. Seven of the eight artifacts mean "stale => fail, run the generator"; this one meant "stale => I'll write it for you", inside the same `check:generated` summary. The ratchet is now isomorphic to the authorable-surface ratchet immediately below it: in --check it prints the unrecorded keys plus the `gen:schema` remedy and exits 1; outside --check it writes exactly as before. The `missing` branch (a published schema disappeared) is untouched — it already exited 1. New runtime e2e tests (scripts/build-schemas-check-mode.test.ts) pin the exit code AND the file bytes for all three branches, plus a negative control so "always red in check mode" cannot pass. They spawn the real script in a temp sandbox that copies scripts/ and symlinks src/, node_modules and package.json, so no test-only seam is added to the gate and a concurrent `gen:schema` from a turbo build cannot race the repo's tracked manifest. Runtime rather than compile-time because this package type-checks neither scripts/ nor *.test.ts (#4642). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0176qgxgCXTJCUv4YFLtusP9 --- .changeset/check-mode-manifest-no-write.md | 35 +++ .../scripts/build-schemas-check-mode.test.ts | 210 ++++++++++++++++++ packages/spec/scripts/build-schemas.ts | 38 +++- 3 files changed, 279 insertions(+), 4 deletions(-) create mode 100644 .changeset/check-mode-manifest-no-write.md create mode 100644 packages/spec/scripts/build-schemas-check-mode.test.ts diff --git a/.changeset/check-mode-manifest-no-write.md b/.changeset/check-mode-manifest-no-write.md new file mode 100644 index 0000000000..234be48add --- /dev/null +++ b/.changeset/check-mode-manifest-no-write.md @@ -0,0 +1,35 @@ +--- +'@objectstack/spec': patch +--- + +fix(spec): `build-schemas.ts --check` no longer writes `json-schema.manifest.json` (#4711) + +The manifest ratchet had no `CHECK` discriminator. `check:authorable-surface` +(`build-schemas.ts --check`) — one of the eight generated-artifact gates +`check:generated` runs — recomputed the emitted schema set and, on any addition +or renamed-away key, **rewrote the tracked `json-schema.manifest.json` in place +and exited 0**. Two defects, one missing `if`: + +1. **A check edited the working tree.** Whatever the file held locally was + overwritten by a command whose entire job is to look, which is how a + `git stash pop` / worktree / merge-conflict operation fails for a reason + nobody traces back to a gate. It is also the #4675 merge-driver trap from the + other side: run any check mid-merge and a manifest computed from a + half-merged tree gets committed to disk — "a plausible generated file is an + invisible error". +2. **The additions branch could never go red in CI.** Seven of the eight + artifacts mean "stale ⇒ fail, run the generator"; this one meant "stale ⇒ + I'll write it for you", inside the same `check:generated` summary. + +The ratchet is now isomorphic to the authorable-surface ratchet immediately +below it: in `--check` it prints the unrecorded keys and the `gen:schema` +remedy, then exits 1; outside `--check` it writes exactly as before. The +`missing` branch (a published schema disappeared) is untouched — it already +exited 1. + +**Behavioural change for contributors:** adding a schema export without running +`pnpm --filter @objectstack/spec gen:schema` now fails `check:authorable-surface` +/ `check:generated` instead of being silently repaired. `check:generated --fix` +(and `check:docs`, which runs `gen:schema` first) regenerate it as before, so no +CI job changes shape — a clean checkout with a current manifest stays green. +No published API, schema or authorable key changes. diff --git a/packages/spec/scripts/build-schemas-check-mode.test.ts b/packages/spec/scripts/build-schemas-check-mode.test.ts new file mode 100644 index 0000000000..3748da0c39 --- /dev/null +++ b/packages/spec/scripts/build-schemas-check-mode.test.ts @@ -0,0 +1,210 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Pins that `build-schemas.ts --check` — the script behind +// `check:authorable-surface`, one of the eight generated-artifact gates +// `check:generated` runs — reports and NEVER writes (#4711). +// +// The defect these tests exist for: the manifest ratchet had no `CHECK` +// discriminator at all. `--check` recomputed the emitted schema set and, on any +// addition, rewrote the tracked `json-schema.manifest.json` in place and exited +// 0. Two things follow, and both were observed: +// +// 1. A "check" edited the working tree. The developer's own manifest content +// was overwritten by a command whose entire job is to look — which is how +// `git stash pop` / worktree / merge-conflict work fails for a reason +// nobody traces back to a gate. +// 2. The additions branch could never go red in CI. Seven of the eight +// generated artifacts mean "stale ⇒ fail, run the generator"; this one +// meant "stale ⇒ I'll write it for you", inside the same `check:generated` +// summary. A gate that repairs what it is meant to detect reports success +// forever. +// +// So the assertions here are deliberately about the SIDE EFFECT and the EXIT +// CODE, not about the diff arithmetic (which was always correct): every check +// case compares the manifest bytes before and after the run. +// +// ── Why a sandbox rather than the real package ──────────────────────────── +// The script resolves every path from its own `__dirname`, so running it in +// place would mutate the repo's tracked `json-schema.manifest.json` — and under +// `turbo run test` a `pnpm --filter @objectstack/spec build` (whose first step +// is `gen:schema`) can be writing that very file concurrently, which would make +// these tests both destructive and flaky. Instead each run happens in a temp +// tree that COPIES `scripts/` (so `__dirname` lands there) and symlinks the +// read-only inputs — `src/`, `node_modules/`, `package.json`. That keeps the +// production code path byte-for-byte: no test-only seam is added to the gate, +// because a seam is itself a place where the gate can differ from what CI runs. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { spawnSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { RENAMED_DEFS } from './lib/renamed-defs'; + +const HERE = path.dirname(fileURLToPath(import.meta.url)); +const PKG = path.resolve(HERE, '..'); +const TSX = path.join(PKG, 'node_modules', '.bin', 'tsx'); +const REAL_MANIFEST = path.join(PKG, 'json-schema.manifest.json'); + +/** + * Every run loads the entire spec surface and emits ~1700 JSON Schemas (~7s + * alone, more under turbo's parallel test load). A timeout here should mean + * "the script hung", not "the runner was busy" — cf. the same note in + * check-react-blocks-declaration-parity.test.ts. + */ +const SPAWN_TIMEOUT_MS = 180_000; + +/** A schema key the committed manifest carries; dropping it fakes "one addition pending". */ +const KNOWN_KEY = 'ui/View'; +/** A key no build can emit — the `missing` (disappearance) ratchet's input. */ +const PHANTOM_KEY = 'ui/ZzzNeverEmittedByAnyBuild'; + +let sandbox: string; +let script: string; +let manifestPath: string; +let pristine: string; + +beforeAll(() => { + pristine = fs.readFileSync(REAL_MANIFEST, 'utf8'); + sandbox = fs.mkdtempSync(path.join(os.tmpdir(), 'build-schemas-check-')); + fs.cpSync(path.join(PKG, 'scripts'), path.join(sandbox, 'scripts'), { recursive: true }); + for (const entry of ['src', 'node_modules', 'package.json']) { + fs.symlinkSync(path.join(PKG, entry), path.join(sandbox, entry)); + } + // The authorable-surface ratchet runs after the manifest one; give it the + // committed snapshot so a check that gets that far judges the same contract. + fs.copyFileSync( + path.join(PKG, 'authorable-surface.json'), + path.join(sandbox, 'authorable-surface.json'), + ); + script = path.join(sandbox, 'scripts', 'build-schemas.ts'); + manifestPath = path.join(sandbox, 'json-schema.manifest.json'); +}); + +afterAll(() => { + if (sandbox) fs.rmSync(sandbox, { recursive: true, force: true }); +}); + +function run(args: string[] = []): { status: number; output: string } { + const r = spawnSync(TSX, [script, ...args], { + cwd: sandbox, + encoding: 'utf8', + timeout: SPAWN_TIMEOUT_MS, + stdio: ['ignore', 'pipe', 'pipe'], + }); + return { status: r.status ?? -1, output: `${r.stdout ?? ''}${r.stderr ?? ''}` }; +} + +/** Seed the sandbox manifest from the committed one; returns the exact bytes written. */ +function seedManifest(mutate: (schemas: string[]) => string[]): string { + const doc = JSON.parse(pristine) as { description?: string; schemas: string[] }; + doc.schemas = mutate(doc.schemas); + const text = JSON.stringify(doc, null, 2) + '\n'; + fs.writeFileSync(manifestPath, text); + return text; +} + +const readManifest = () => fs.readFileSync(manifestPath, 'utf8'); + +describe('build-schemas.ts --check — a check reports, it does not write (#4711)', () => { + it( + 'fails on a manifest behind on additions, and leaves the file byte-identical', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + expect(JSON.parse(pristine).schemas).toContain(KNOWN_KEY); + const stale = seedManifest((s) => s.filter((k) => k !== KNOWN_KEY)); + + const { status, output } = run(['--check']); + + // The exit code is half the fix: before #4711 this branch exited 0. + expect(status).toBe(1); + expect(output).toMatch(/json-schema\.manifest\.json is out of date \(1 schema\(s\) not recorded\)/); + expect(output).toContain(`+ json-schema/${KNOWN_KEY}.json`); + // The remedy must be the generator, exactly as the other seven artifacts say. + expect(output).toMatch(/gen:schema/); + // …and the file is the other half: not rewritten, not touched. + expect(readManifest()).toBe(stale); + expect(output).not.toContain('📒'); + }, + ); + + it( + 'fails on a manifest still listing a def RENAMED_DEFS moved away, without writing it', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + // The other half of the same condition, and the half that has no other + // reporter: a renamed-away source key is deliberately NOT "missing" (the + // disappearance ratchet excludes it, since the def is published under the + // new name), so before #4711 the only thing that ever noticed it was the + // silent rewrite. #4684 / #4703 both depend on that key actually leaving + // the manifest. + const [renamedSource] = Object.keys(RENAMED_DEFS); + // Loud on purpose: an empty table makes this branch dead code, which is a + // decision (delete the branch, or the test) — not something to skip past. + expect(renamedSource, 'RENAMED_DEFS is empty — this test exercises nothing').toBeTruthy(); + const withStaleRename = seedManifest((s) => [...s, renamedSource].sort()); + + const { status, output } = run(['--check']); + + expect(status).toBe(1); + expect(output).toMatch( + /json-schema\.manifest\.json is out of date .*1 renamed-away key\(s\) still listed/, + ); + expect(output).toContain(`- json-schema/${renamedSource}.json (renamed away)`); + expect(readManifest()).toBe(withStaleRename); + }, + ); + + it( + 'keeps the disappearance ratchet intact: a schema in the manifest that no build emits still exits 1', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + const withPhantom = seedManifest((s) => [...s, PHANTOM_KEY].sort()); + + const { status, output } = run(['--check']); + + expect(status).toBe(1); + expect(output).toMatch(/1 previously published schema\(s\) disappeared from this build/); + expect(output).toContain(`- json-schema/${PHANTOM_KEY}.json`); + expect(readManifest()).toBe(withPhantom); + }, + ); + + it( + 'still writes the manifest outside --check, so gen:schema keeps recording additions', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + const stale = seedManifest((s) => s.filter((k) => k !== KNOWN_KEY)); + + const { status, output } = run([]); + + expect(status).toBe(0); + expect(output).toContain('📒 json-schema.manifest.json updated (+1 schema(s))'); + expect(readManifest()).not.toBe(stale); + expect(JSON.parse(readManifest()).schemas).toContain(KNOWN_KEY); + }, + ); + + it( + 'is silent about the manifest when it is up to date — the new failure is staleness, not --check itself', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + // Negative control. Without it, "always exit 1 in check mode" would pass + // every assertion above while breaking the gate for everyone. + // NOTE: this asserts status 0, so it also re-proves that the COMMITTED + // manifest and authorable-surface snapshots are current — the same thing + // `check:authorable-surface` asserts in CI. If it fails here, run + // `pnpm --filter @objectstack/spec gen:schema` and commit the result. + const current = seedManifest((s) => s); + + const { status, output } = run(['--check']); + + expect(output).not.toMatch(/json-schema\.manifest\.json is out of date/); + expect(output).not.toContain('📒'); + expect(readManifest()).toBe(current); + expect(status).toBe(0); + }, + ); +}); diff --git a/packages/spec/scripts/build-schemas.ts b/packages/spec/scripts/build-schemas.ts index 9c1d4dd494..889512a15c 100644 --- a/packages/spec/scripts/build-schemas.ts +++ b/packages/spec/scripts/build-schemas.ts @@ -41,9 +41,12 @@ 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 committed authorable-surface snapshot without rewriting -// it, so CI fails on an uncommitted ADDITION too (the write and check paths share -// the same code — same discipline as build-docs.ts). +// `--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). const CHECK = process.argv.includes('--check'); 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}`; @@ -342,7 +345,34 @@ const added = [...generatedKeys].filter((key) => !(manifest?.schemas ?? []).incl // existed. Without this the stale key would sit in the manifest forever, kept // alive only by its RENAMED_DEFS entry. const renamedAway = (manifest?.schemas ?? []).filter((key) => key in RENAMED_DEFS); -if (!manifest || added.length > 0 || renamedAway.length > 0) { +const manifestChanged = !manifest || added.length > 0 || renamedAway.length > 0; +if (manifestChanged && CHECK) { + // Removals already exited above; reaching here in check mode means the manifest + // is behind on ADDITIONS (or still lists a def that RENAMED_DEFS moved away). + // Report it — never write. `--check` is what `check:authorable-surface` (and so + // `check:generated`) runs, and a check that edits a tracked file is wrong twice + // over: it makes `git stash` / `git worktree` / merge-conflict work fail for + // reasons nobody traces back to a gate, and it makes this branch the one + // generated artifact of eight that can never go red in CI — "stale ⇒ rewrite it + // for you" instead of "stale ⇒ run the generator" (#4711). Same split as the + // authorable-surface ratchet below. + console.error( + manifest + ? `\n❌ json-schema.manifest.json is out of date (${added.length} schema(s) not recorded` + + `${renamedAway.length > 0 ? `, ${renamedAway.length} renamed-away key(s) still listed` : ''}).` + : `\n❌ json-schema.manifest.json is missing (${generatedKeys.size} schema(s) unrecorded).`, + ); + for (const key of added.slice(0, 20)) console.error(` + json-schema/${key}.json`); + if (added.length > 20) console.error(` … and ${added.length - 20} more`); + for (const key of renamedAway) console.error(` - json-schema/${key}.json (renamed away)`); + console.error( + `\n Run \`pnpm --filter @objectstack/spec gen:schema\` and commit the result. A schema\n` + + ` absent from the manifest is one this ratchet can never report as disappeared later,\n` + + ` because it was never in the baseline (#2978).`, + ); + process.exit(1); +} +if (manifestChanged && !CHECK) { const updated: SchemaManifest = { description: 'Ratchet manifest of every JSON Schema emitted by scripts/build-schemas.ts. ' +