Skip to content

Commit 65f85cb

Browse files
committed
fix(spec): --check must not write json-schema.manifest.json (#4711)
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176qgxgCXTJCUv4YFLtusP9
1 parent 2823d82 commit 65f85cb

3 files changed

Lines changed: 279 additions & 4 deletions

File tree

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
---
2+
'@objectstack/spec': patch
3+
---
4+
5+
fix(spec): `build-schemas.ts --check` no longer writes `json-schema.manifest.json` (#4711)
6+
7+
The manifest ratchet had no `CHECK` discriminator. `check:authorable-surface`
8+
(`build-schemas.ts --check`) — one of the eight generated-artifact gates
9+
`check:generated` runs — recomputed the emitted schema set and, on any addition
10+
or renamed-away key, **rewrote the tracked `json-schema.manifest.json` in place
11+
and exited 0**. Two defects, one missing `if`:
12+
13+
1. **A check edited the working tree.** Whatever the file held locally was
14+
overwritten by a command whose entire job is to look, which is how a
15+
`git stash pop` / worktree / merge-conflict operation fails for a reason
16+
nobody traces back to a gate. It is also the #4675 merge-driver trap from the
17+
other side: run any check mid-merge and a manifest computed from a
18+
half-merged tree gets committed to disk — "a plausible generated file is an
19+
invisible error".
20+
2. **The additions branch could never go red in CI.** Seven of the eight
21+
artifacts mean "stale ⇒ fail, run the generator"; this one meant "stale ⇒
22+
I'll write it for you", inside the same `check:generated` summary.
23+
24+
The ratchet is now isomorphic to the authorable-surface ratchet immediately
25+
below it: in `--check` it prints the unrecorded keys and the `gen:schema`
26+
remedy, then exits 1; outside `--check` it writes exactly as before. The
27+
`missing` branch (a published schema disappeared) is untouched — it already
28+
exited 1.
29+
30+
**Behavioural change for contributors:** adding a schema export without running
31+
`pnpm --filter @objectstack/spec gen:schema` now fails `check:authorable-surface`
32+
/ `check:generated` instead of being silently repaired. `check:generated --fix`
33+
(and `check:docs`, which runs `gen:schema` first) regenerate it as before, so no
34+
CI job changes shape — a clean checkout with a current manifest stays green.
35+
No published API, schema or authorable key changes.
Lines changed: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,210 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// Pins that `build-schemas.ts --check` — the script behind
4+
// `check:authorable-surface`, one of the eight generated-artifact gates
5+
// `check:generated` runs — reports and NEVER writes (#4711).
6+
//
7+
// The defect these tests exist for: the manifest ratchet had no `CHECK`
8+
// discriminator at all. `--check` recomputed the emitted schema set and, on any
9+
// addition, rewrote the tracked `json-schema.manifest.json` in place and exited
10+
// 0. Two things follow, and both were observed:
11+
//
12+
// 1. A "check" edited the working tree. The developer's own manifest content
13+
// was overwritten by a command whose entire job is to look — which is how
14+
// `git stash pop` / worktree / merge-conflict work fails for a reason
15+
// nobody traces back to a gate.
16+
// 2. The additions branch could never go red in CI. Seven of the eight
17+
// generated artifacts mean "stale ⇒ fail, run the generator"; this one
18+
// meant "stale ⇒ I'll write it for you", inside the same `check:generated`
19+
// summary. A gate that repairs what it is meant to detect reports success
20+
// forever.
21+
//
22+
// So the assertions here are deliberately about the SIDE EFFECT and the EXIT
23+
// CODE, not about the diff arithmetic (which was always correct): every check
24+
// case compares the manifest bytes before and after the run.
25+
//
26+
// ── Why a sandbox rather than the real package ────────────────────────────
27+
// The script resolves every path from its own `__dirname`, so running it in
28+
// place would mutate the repo's tracked `json-schema.manifest.json` — and under
29+
// `turbo run test` a `pnpm --filter @objectstack/spec build` (whose first step
30+
// is `gen:schema`) can be writing that very file concurrently, which would make
31+
// these tests both destructive and flaky. Instead each run happens in a temp
32+
// tree that COPIES `scripts/` (so `__dirname` lands there) and symlinks the
33+
// read-only inputs — `src/`, `node_modules/`, `package.json`. That keeps the
34+
// production code path byte-for-byte: no test-only seam is added to the gate,
35+
// because a seam is itself a place where the gate can differ from what CI runs.
36+
37+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
38+
import { spawnSync } from 'node:child_process';
39+
import fs from 'node:fs';
40+
import os from 'node:os';
41+
import path from 'node:path';
42+
import { fileURLToPath } from 'node:url';
43+
44+
import { RENAMED_DEFS } from './lib/renamed-defs';
45+
46+
const HERE = path.dirname(fileURLToPath(import.meta.url));
47+
const PKG = path.resolve(HERE, '..');
48+
const TSX = path.join(PKG, 'node_modules', '.bin', 'tsx');
49+
const REAL_MANIFEST = path.join(PKG, 'json-schema.manifest.json');
50+
51+
/**
52+
* Every run loads the entire spec surface and emits ~1700 JSON Schemas (~7s
53+
* alone, more under turbo's parallel test load). A timeout here should mean
54+
* "the script hung", not "the runner was busy" — cf. the same note in
55+
* check-react-blocks-declaration-parity.test.ts.
56+
*/
57+
const SPAWN_TIMEOUT_MS = 180_000;
58+
59+
/** A schema key the committed manifest carries; dropping it fakes "one addition pending". */
60+
const KNOWN_KEY = 'ui/View';
61+
/** A key no build can emit — the `missing` (disappearance) ratchet's input. */
62+
const PHANTOM_KEY = 'ui/ZzzNeverEmittedByAnyBuild';
63+
64+
let sandbox: string;
65+
let script: string;
66+
let manifestPath: string;
67+
let pristine: string;
68+
69+
beforeAll(() => {
70+
pristine = fs.readFileSync(REAL_MANIFEST, 'utf8');
71+
sandbox = fs.mkdtempSync(path.join(os.tmpdir(), 'build-schemas-check-'));
72+
fs.cpSync(path.join(PKG, 'scripts'), path.join(sandbox, 'scripts'), { recursive: true });
73+
for (const entry of ['src', 'node_modules', 'package.json']) {
74+
fs.symlinkSync(path.join(PKG, entry), path.join(sandbox, entry));
75+
}
76+
// The authorable-surface ratchet runs after the manifest one; give it the
77+
// committed snapshot so a check that gets that far judges the same contract.
78+
fs.copyFileSync(
79+
path.join(PKG, 'authorable-surface.json'),
80+
path.join(sandbox, 'authorable-surface.json'),
81+
);
82+
script = path.join(sandbox, 'scripts', 'build-schemas.ts');
83+
manifestPath = path.join(sandbox, 'json-schema.manifest.json');
84+
});
85+
86+
afterAll(() => {
87+
if (sandbox) fs.rmSync(sandbox, { recursive: true, force: true });
88+
});
89+
90+
function run(args: string[] = []): { status: number; output: string } {
91+
const r = spawnSync(TSX, [script, ...args], {
92+
cwd: sandbox,
93+
encoding: 'utf8',
94+
timeout: SPAWN_TIMEOUT_MS,
95+
stdio: ['ignore', 'pipe', 'pipe'],
96+
});
97+
return { status: r.status ?? -1, output: `${r.stdout ?? ''}${r.stderr ?? ''}` };
98+
}
99+
100+
/** Seed the sandbox manifest from the committed one; returns the exact bytes written. */
101+
function seedManifest(mutate: (schemas: string[]) => string[]): string {
102+
const doc = JSON.parse(pristine) as { description?: string; schemas: string[] };
103+
doc.schemas = mutate(doc.schemas);
104+
const text = JSON.stringify(doc, null, 2) + '\n';
105+
fs.writeFileSync(manifestPath, text);
106+
return text;
107+
}
108+
109+
const readManifest = () => fs.readFileSync(manifestPath, 'utf8');
110+
111+
describe('build-schemas.ts --check — a check reports, it does not write (#4711)', () => {
112+
it(
113+
'fails on a manifest behind on additions, and leaves the file byte-identical',
114+
{ timeout: SPAWN_TIMEOUT_MS },
115+
() => {
116+
expect(JSON.parse(pristine).schemas).toContain(KNOWN_KEY);
117+
const stale = seedManifest((s) => s.filter((k) => k !== KNOWN_KEY));
118+
119+
const { status, output } = run(['--check']);
120+
121+
// The exit code is half the fix: before #4711 this branch exited 0.
122+
expect(status).toBe(1);
123+
expect(output).toMatch(/json-schema\.manifest\.json is out of date \(1 schema\(s\) not recorded\)/);
124+
expect(output).toContain(`+ json-schema/${KNOWN_KEY}.json`);
125+
// The remedy must be the generator, exactly as the other seven artifacts say.
126+
expect(output).toMatch(/gen:schema/);
127+
// …and the file is the other half: not rewritten, not touched.
128+
expect(readManifest()).toBe(stale);
129+
expect(output).not.toContain('📒');
130+
},
131+
);
132+
133+
it(
134+
'fails on a manifest still listing a def RENAMED_DEFS moved away, without writing it',
135+
{ timeout: SPAWN_TIMEOUT_MS },
136+
() => {
137+
// The other half of the same condition, and the half that has no other
138+
// reporter: a renamed-away source key is deliberately NOT "missing" (the
139+
// disappearance ratchet excludes it, since the def is published under the
140+
// new name), so before #4711 the only thing that ever noticed it was the
141+
// silent rewrite. #4684 / #4703 both depend on that key actually leaving
142+
// the manifest.
143+
const [renamedSource] = Object.keys(RENAMED_DEFS);
144+
// Loud on purpose: an empty table makes this branch dead code, which is a
145+
// decision (delete the branch, or the test) — not something to skip past.
146+
expect(renamedSource, 'RENAMED_DEFS is empty — this test exercises nothing').toBeTruthy();
147+
const withStaleRename = seedManifest((s) => [...s, renamedSource].sort());
148+
149+
const { status, output } = run(['--check']);
150+
151+
expect(status).toBe(1);
152+
expect(output).toMatch(
153+
/json-schema\.manifest\.json is out of date .*1 renamed-away key\(s\) still listed/,
154+
);
155+
expect(output).toContain(`- json-schema/${renamedSource}.json (renamed away)`);
156+
expect(readManifest()).toBe(withStaleRename);
157+
},
158+
);
159+
160+
it(
161+
'keeps the disappearance ratchet intact: a schema in the manifest that no build emits still exits 1',
162+
{ timeout: SPAWN_TIMEOUT_MS },
163+
() => {
164+
const withPhantom = seedManifest((s) => [...s, PHANTOM_KEY].sort());
165+
166+
const { status, output } = run(['--check']);
167+
168+
expect(status).toBe(1);
169+
expect(output).toMatch(/1 previously published schema\(s\) disappeared from this build/);
170+
expect(output).toContain(`- json-schema/${PHANTOM_KEY}.json`);
171+
expect(readManifest()).toBe(withPhantom);
172+
},
173+
);
174+
175+
it(
176+
'still writes the manifest outside --check, so gen:schema keeps recording additions',
177+
{ timeout: SPAWN_TIMEOUT_MS },
178+
() => {
179+
const stale = seedManifest((s) => s.filter((k) => k !== KNOWN_KEY));
180+
181+
const { status, output } = run([]);
182+
183+
expect(status).toBe(0);
184+
expect(output).toContain('📒 json-schema.manifest.json updated (+1 schema(s))');
185+
expect(readManifest()).not.toBe(stale);
186+
expect(JSON.parse(readManifest()).schemas).toContain(KNOWN_KEY);
187+
},
188+
);
189+
190+
it(
191+
'is silent about the manifest when it is up to date — the new failure is staleness, not --check itself',
192+
{ timeout: SPAWN_TIMEOUT_MS },
193+
() => {
194+
// Negative control. Without it, "always exit 1 in check mode" would pass
195+
// every assertion above while breaking the gate for everyone.
196+
// NOTE: this asserts status 0, so it also re-proves that the COMMITTED
197+
// manifest and authorable-surface snapshots are current — the same thing
198+
// `check:authorable-surface` asserts in CI. If it fails here, run
199+
// `pnpm --filter @objectstack/spec gen:schema` and commit the result.
200+
const current = seedManifest((s) => s);
201+
202+
const { status, output } = run(['--check']);
203+
204+
expect(output).not.toMatch(/json-schema\.manifest\.json is out of date/);
205+
expect(output).not.toContain('📒');
206+
expect(readManifest()).toBe(current);
207+
expect(status).toBe(0);
208+
},
209+
);
210+
});

packages/spec/scripts/build-schemas.ts

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,12 @@ const OUT_DIR = path.resolve(__dirname, '../json-schema');
4141
// ever emitted. json-schema/ itself is a gitignored build artifact, so this
4242
// file is the durable "last time" — see the disappearance check below (#2978).
4343
const MANIFEST_PATH = path.resolve(__dirname, '../json-schema.manifest.json');
44-
// `--check` verifies the committed authorable-surface snapshot without rewriting
45-
// it, so CI fails on an uncommitted ADDITION too (the write and check paths share
46-
// the same code — same discipline as build-docs.ts).
44+
// `--check` verifies the two committed snapshots — the schema manifest and the
45+
// authorable surface — without rewriting either, so CI fails on an uncommitted
46+
// ADDITION too (the write and check paths share the same code — same discipline
47+
// as build-docs.ts). "Without rewriting" is load-bearing on both: a check that
48+
// repairs what it detects can never report it, and it silently edits the tree of
49+
// whoever ran it (#4711).
4750
const CHECK = process.argv.includes('--check');
4851
const SPEC_VERSION = JSON.parse(fs.readFileSync(path.resolve(__dirname, '../package.json'), 'utf-8')).version;
4952
const SCHEMA_BASE_URL = `https://schema.objectstack.io/v${SPEC_VERSION}`;
@@ -342,7 +345,34 @@ const added = [...generatedKeys].filter((key) => !(manifest?.schemas ?? []).incl
342345
// existed. Without this the stale key would sit in the manifest forever, kept
343346
// alive only by its RENAMED_DEFS entry.
344347
const renamedAway = (manifest?.schemas ?? []).filter((key) => key in RENAMED_DEFS);
345-
if (!manifest || added.length > 0 || renamedAway.length > 0) {
348+
const manifestChanged = !manifest || added.length > 0 || renamedAway.length > 0;
349+
if (manifestChanged && CHECK) {
350+
// Removals already exited above; reaching here in check mode means the manifest
351+
// is behind on ADDITIONS (or still lists a def that RENAMED_DEFS moved away).
352+
// Report it — never write. `--check` is what `check:authorable-surface` (and so
353+
// `check:generated`) runs, and a check that edits a tracked file is wrong twice
354+
// over: it makes `git stash` / `git worktree` / merge-conflict work fail for
355+
// reasons nobody traces back to a gate, and it makes this branch the one
356+
// generated artifact of eight that can never go red in CI — "stale ⇒ rewrite it
357+
// for you" instead of "stale ⇒ run the generator" (#4711). Same split as the
358+
// authorable-surface ratchet below.
359+
console.error(
360+
manifest
361+
? `\n❌ json-schema.manifest.json is out of date (${added.length} schema(s) not recorded` +
362+
`${renamedAway.length > 0 ? `, ${renamedAway.length} renamed-away key(s) still listed` : ''}).`
363+
: `\n❌ json-schema.manifest.json is missing (${generatedKeys.size} schema(s) unrecorded).`,
364+
);
365+
for (const key of added.slice(0, 20)) console.error(` + json-schema/${key}.json`);
366+
if (added.length > 20) console.error(` … and ${added.length - 20} more`);
367+
for (const key of renamedAway) console.error(` - json-schema/${key}.json (renamed away)`);
368+
console.error(
369+
`\n Run \`pnpm --filter @objectstack/spec gen:schema\` and commit the result. A schema\n` +
370+
` absent from the manifest is one this ratchet can never report as disappeared later,\n` +
371+
` because it was never in the baseline (#2978).`,
372+
);
373+
process.exit(1);
374+
}
375+
if (manifestChanged && !CHECK) {
346376
const updated: SchemaManifest = {
347377
description:
348378
'Ratchet manifest of every JSON Schema emitted by scripts/build-schemas.ts. ' +

0 commit comments

Comments
 (0)