diff --git a/packages/spec/scripts/lib/sharded-artifacts.ts b/packages/spec/scripts/lib/sharded-artifacts.ts index 2d213b1d53..6dffdf804e 100644 --- a/packages/spec/scripts/lib/sharded-artifacts.ts +++ b/packages/spec/scripts/lib/sharded-artifacts.ts @@ -352,6 +352,34 @@ function jsonTypeLabel(value: unknown): string { return `${/^[aeiou]/.test(type) ? 'an' : 'a'} ${type}`; } +/** + * The middle of the "this entry is not a string" message — field, entry index, + * what was found instead, and the offending value — or null when every entry in + * `list` is a string (#7076). + * + * It is the SKELETON only, deliberately: each reader prefixes its own source + * locator (a shard file in the working tree, a path at a revision) and appends + * its own remedy, because those two differ per site while the diagnosis does + * not. `aggregateCategoryShards` above spells the same middle inline — the fix + * that established this shape (#6751) landed before there was a second caller, + * and rewriting a landed message to route it through here would churn a pinned + * contract for no reader's benefit. The pin tests assert the shared substring on + * all three sites, so a divergence is caught rather than trusted. + */ +function nonStringEntryDetail( + field: string, + list: readonly unknown[], +): { index: number; detail: string } | null { + const index = list.findIndex((entry) => typeof entry !== 'string'); + if (index === -1) return null; + return { + index, + detail: + `${field}[${index}] is ${jsonTypeLabel(list[index])}, not a string (#5837): ` + + `${JSON.stringify(list[index])}`, + }; +} + /** * Aggregate a category-sharded directory into one sorted array, validating that * each shard answers only for its own category. @@ -429,6 +457,27 @@ export function aggregateApiSurfaceShards( if (!Array.isArray(shard.doc.exports)) { throw new Error(`${API_SURFACE_DIR_NAME}/${shard.name}.json has no "exports" array (#5837).`); } + // `ApiSurfaceShard.exports` is DECLARED `string[]`, and `readShards` gets + // there by a `JSON.parse` cast — so the declaration is a claim about the + // file, not a fact the type checker verified. `Array.isArray` above answers + // for the container and says nothing about the entries, exactly the gap + // #6751 closed one function up; this is the fourth sharded artifact, which + // that fix could not reach because it is not routed by `categoryOfDefKey`. + // Unchecked, a hand-edited non-string row travels into `surface` as a + // `string` and lands in the breadth diff in `build-api-surface.ts`: it is in + // the snapshot and not in the built surface, so it is counted as a REMOVED + // export and the run ends on "a REMOVED export … is a BREAKING change for + // third parties — bump @objectstack/spec to a new major (or restore it)". + // Red either way, but that sentence sends the author after an export that + // never existed instead of the shard row they broke (#7076). + const rows = shard.doc.exports as readonly unknown[]; + const bad = nonStringEntryDetail('exports', rows); + if (bad) { + throw new Error( + `${API_SURFACE_DIR_NAME}/${shard.name}.json ${bad.detail}. Every entry is an ` + + `" ()" export row — regenerate rather than reconcile by hand.`, + ); + } surface[shard.doc.entry] = shard.doc.exports; } return { surface, shards }; @@ -496,6 +545,15 @@ export function readShardedKeysAtRev( } const list = doc[field]; if (!Array.isArray(list)) return { error: `${file} at ${rev.slice(0, 12)} has no "${field}" array` }; + // The entry check the container check cannot do (#7076). Carried as + // `{ error }` rather than thrown because that is this reader's contract — + // `readSurfaceKeysAtRev` in `build-schemas.ts` prints the string under the + // gate's own name and exits, and a throw from here would escape that + // framing and print a baseline problem as a problem with the commit under + // test. See the same check below for the legacy layout, and the note there + // on why a bad entry costs more here than a bad container. + const bad = nonStringEntryDetail(field, list as readonly unknown[]); + if (bad) return { error: `${file} at ${rev.slice(0, 12)} ${bad.detail}` }; entries.push(...(list as string[])); } return { entries: entries.sort(), layout: 'sharded' }; @@ -520,5 +578,27 @@ export function readShardedKeysAtRev( } const list = doc[field]; if (!Array.isArray(list)) return { error: `${legacyName} at ${rev.slice(0, 12)} has no "${field}" array` }; + // Why a non-string entry is worth its own verdict here, when the container + // check already exists: a missing array stops the read, but a bad ENTRY used + // to be forwarded into the baseline SET, and the three gates that consume it + // fail three different ways, none of them naming this file (#7076): + // + // - the authorable-surface deletion gate maps every base entry through + // `entry.replace(RETIRED_MARK, '')` and dies on `replace is not a + // function` — the bare-JS-error shape #6751 removed one function up; + // - the json-schema.manifest removal check keeps the entry (it is in + // neither `generatedKeys` nor `RENAMED_DEFS`), reports it as a schema + // that left the published set, and demands a `RETIRED_DEFS_BY_MAJOR` + // registration for a def that never existed; + // - `compareAnchorKeys` reports it as a line the committed + // `authorable-surface.base.json` is missing, i.e. blames the anchor for + // not mirroring a baseline it mirrors correctly. + // + // All three are loud, so this is diagnostic quality and not a bypass — but a + // baseline read from an already-merged commit is exactly where "the artifact + // is corrupt" must be said by the reader, since no downstream gate can see + // which file the value came from. + const bad = nonStringEntryDetail(field, list as readonly unknown[]); + if (bad) return { error: `${legacyName} at ${rev.slice(0, 12)} ${bad.detail}` }; return { entries: [...(list as string[])].sort(), layout: 'legacy' }; } diff --git a/packages/spec/scripts/sharded-artifacts.test.ts b/packages/spec/scripts/sharded-artifacts.test.ts index b62f1db095..06ad9f5e8f 100644 --- a/packages/spec/scripts/sharded-artifacts.test.ts +++ b/packages/spec/scripts/sharded-artifacts.test.ts @@ -329,6 +329,38 @@ describe('sharded artifacts — the aggregate reads the whole directory (#5837)' expect(message).toContain('#5837'); }); + /** + * The fourth sharded artifact, held to the message #6751 established for the + * other three (#7076). `api-surface/` is not routed by `categoryOfDefKey`, so + * that fix could not reach it: its rows went straight from `Array.isArray` — + * a statement about the CONTAINER — into a `Record`. + * + * Deliberately adjacent to the three cases above rather than in a file of its + * own: what has to stay true is that these readers say the SAME thing, and the + * shared middle (`[] is , not a string (#5837): `) is + * only reviewable when the assertions sit next to each other. + */ + it('names the shard file, the entry index and the anchor for a non-string export row', () => { + const apiDir = path.join(dir, 'api'); + writeShards(apiDir, apiSurfaceShardTexts({ './data': ['Field (type)', 'Object (type)'] })); + rewriteShard(apiDir, 'data', (doc) => { + (doc.exports as unknown[])[1] = { name: 'Object' }; + }); + + const message = messageOf(() => aggregateApiSurfaceShards(apiDir)); + expect(message, 'names the shard file').toContain('api-surface/data.json'); + expect(message, 'names the entry, in the artifact’s own field').toContain('exports[1]'); + expect(message, 'carries the issue anchor').toContain('#5837'); + expect(message, 'says what was found instead').toContain('is an object, not a string'); + expect(message, 'quotes the offending value').toContain('{"name":"Object"}'); + // The reverse direction is NOT "threw a worse error before" — it is `messageOf`'s + // own `expect.fail`: unfixed, this reader RETURNS the corrupt row as a + // `string` and the diff in `build-api-surface.ts` reports it as a REMOVED + // export, demanding a major bump for an export that never existed. So the + // pin is that the reader rejects at all, and then that it rejects in the + // shared words. + }); + it('refuses a stray file in a generator-owned directory', () => { writeShards(dir, authorableSurfaceShardTexts(KEYS)); fs.writeFileSync(path.join(dir, 'notes.txt'), 'scratch\n'); @@ -428,6 +460,67 @@ describe('sharded artifacts — the historical baseline reader (#5837)', () => { expect(readShardedKeysAtRev(gitIn(root), rev, AUTHORABLE_SURFACE_DIR_NAME, 'keys')).toBeNull(); }); + /** + * The same defect class as the aggregate's fourth case (#6751), on the reader + * that cannot throw it (#7076). This one reports by `{ error }` — its callers + * in `build-schemas.ts` print that string under the gate's own name and exit — + * so the assertion is on the returned string, not on a thrown one, and the + * message skeleton has to be identical while the carrier is not. + * + * Why it matters more here than a bare container check: an unchecked entry was + * forwarded into the baseline SET, and the three gates that consume that set + * each fail somewhere else — `entry.replace is not a function` in the + * authorable-surface deletion check, a demand for a `RETIRED_DEFS_BY_MAJOR` + * registration in the manifest removal check, and "the anchor is not the + * baseline it claims to be" in `compareAnchorKeys`. None of the three can name + * the file the value came from; only this reader knows it. + */ + it('reports a non-string entry as an error naming the file, the entry and the anchor', () => { + const { root, rev } = repoWith((pkg) => { + const surfaceDir = path.join(pkg, AUTHORABLE_SURFACE_DIR_NAME); + writeShards(surfaceDir, authorableSurfaceShardTexts(KEYS)); + const file = path.join(surfaceDir, 'ui.json'); + const doc = JSON.parse(fs.readFileSync(file, 'utf-8')); + doc.keys[1] = 12345; + fs.writeFileSync(file, JSON.stringify(doc, null, 2) + '\n'); + }); + + const read = readShardedKeysAtRev(gitIn(root), rev, AUTHORABLE_SURFACE_DIR_NAME, 'keys'); + // Not `toThrow`, and not `entries` either: unfixed, this call SUCCEEDS and + // hands 12345 to the gates as a key. The pin is that it refuses instead. + expect(read, 'refuses rather than forwarding the bad entry').toHaveProperty('error'); + const message = (read as { error: string }).error; + expect(message, 'names the shard file').toContain(`${AUTHORABLE_SURFACE_DIR_NAME}/ui.json`); + expect(message, 'names the revision it read').toContain(rev.slice(0, 12)); + expect(message, 'names the entry, in the artifact’s own field').toContain('keys[1]'); + expect(message, 'carries the issue anchor').toContain('#5837'); + expect(message, 'says what was found instead').toContain('is a number, not a string'); + expect(message, 'quotes the offending value').toContain('12345'); + }); + + it('reports a non-string entry in the retired single-file layout the same way', () => { + // The legacy branch reads immutable history — a commit from before #5837 — + // so it is the one place where "regenerate the artifact" is not advice + // anyone can take. All the more reason for the message to name the file and + // the entry itself. + const { root, rev } = repoWith((pkg) => { + fs.writeFileSync( + path.join(pkg, 'authorable-surface.json'), + JSON.stringify({ description: 'legacy', keys: [...KEYS.slice(0, 2), null] }, null, 2) + '\n', + ); + }); + + const read = readShardedKeysAtRev(gitIn(root), rev, AUTHORABLE_SURFACE_DIR_NAME, 'keys'); + expect(read).toHaveProperty('error'); + const message = (read as { error: string }).error; + expect(message, 'names the retired single file').toContain('authorable-surface.json'); + expect(message, 'names the entry').toContain('keys[2]'); + expect(message, 'carries the issue anchor').toContain('#5837'); + // `typeof null === 'object'` is the trap `jsonTypeLabel` exists for, and it + // has to survive the trip through the `{ error }` carrier unchanged. + expect(message, 'distinguishes null from an object').toContain('is null, not a string'); + }); + it('reads the manifest layout by the same rule', () => { const defs = ['ai/Agent', 'ui/View']; const { root, rev } = repoWith((pkg) => {