diff --git a/packages/spec/scripts/lib/sharded-artifacts.ts b/packages/spec/scripts/lib/sharded-artifacts.ts index 9f980415ba..2d213b1d53 100644 --- a/packages/spec/scripts/lib/sharded-artifacts.ts +++ b/packages/spec/scripts/lib/sharded-artifacts.ts @@ -340,6 +340,18 @@ function shardTextsByCategory( return out; } +/** + * The JSON type of a parsed value, article included, for a message that has to + * tell an author what they actually wrote. `typeof` alone answers `"object"` for + * both `null` and `[]` — the two hand-edit accidents most worth telling apart. + */ +function jsonTypeLabel(value: unknown): string { + if (value === null) return 'null'; + if (Array.isArray(value)) return 'an array'; + const type = typeof value; + return `${/^[aeiou]/.test(type) ? 'an' : 'a'} ${type}`; +} + /** * Aggregate a category-sharded directory into one sorted array, validating that * each shard answers only for its own category. @@ -370,7 +382,23 @@ export function aggregateCategoryShards( if (!Array.isArray(list)) { throw new Error(`${dirName}/${shard.name}.json has no "${field}" array (#5837).`); } - for (const raw of list as string[]) { + // `Array.isArray` says the field IS an array and nothing about what is in + // it, so this is where untyped JSON stops being untyped. A hand-edited + // non-string entry used to reach `categoryOfDefKey`, whose parameter is + // declared `string`, and die there on `key.indexOf is not a function` — + // right exit code, but all three call sites print `error.message` alone, so + // the author was told neither the shard file nor the entry (#6751). Every + // other defect class this reader rejects names both; so does this one now. + // The check belongs here rather than in `categoryOfDefKey`: the helper's + // contract already says `string`, and only its caller knows the file name. + for (const [index, raw] of (list as readonly unknown[]).entries()) { + if (typeof raw !== 'string') { + throw new Error( + `${dirName}/${shard.name}.json ${field}[${index}] is ${jsonTypeLabel(raw)}, not a string ` + + `(#5837): ${JSON.stringify(raw)}. Every entry is a "/[:]" key — ` + + `regenerate rather than reconcile by hand.`, + ); + } if (categoryOfDefKey(raw) !== shard.name) { throw new Error( `${dirName}/${shard.name}.json carries "${raw}", which belongs to category ` + diff --git a/packages/spec/scripts/sharded-artifacts.test.ts b/packages/spec/scripts/sharded-artifacts.test.ts index b922542efb..b62f1db095 100644 --- a/packages/spec/scripts/sharded-artifacts.test.ts +++ b/packages/spec/scripts/sharded-artifacts.test.ts @@ -87,6 +87,33 @@ function changed(before: Map, after: Map): strin return [...names].filter((n) => before.get(n) !== after.get(n)).sort(); } +/** + * The message of the error `run` throws, for the cases where the MESSAGE is the + * thing under test (#6751). `expect(...).toThrow()` cannot serve those: the + * reader threw before the fix too — with a bare `TypeError` — so a throw-only + * assertion is green on the defect it is supposed to pin. + */ +function messageOf(run: () => unknown): string { + try { + run(); + } catch (error) { + return error instanceof Error ? error.message : String(error); + } + return expect.fail('expected the shard reader to reject, but it returned a value'); +} + +/** Rewrite one shard's parsed document, in the canonical byte shape. */ +function rewriteShard( + root: string, + name: string, + mutate: (doc: Record) => void, +): void { + const file = path.join(root, `${name}.json`); + const doc = JSON.parse(fs.readFileSync(file, 'utf-8')); + mutate(doc); + fs.writeFileSync(file, JSON.stringify(doc, null, 2) + '\n'); +} + describe('sharded artifacts — locality: a change in one category moves one file (#5837)', () => { it('routes a key to the shard its category segment names, and only that one', () => { // The routing law itself. Everything below is a consequence of it, so it is @@ -229,6 +256,79 @@ describe('sharded artifacts — the aggregate reads the whole directory (#5837)' expect(() => aggregateCategoryShards(dir, 'keys')).toThrow(/declares category "data"/); }); + /** + * The fourth defect class of this reader (#6751). The other three name the + * shard file and carry an issue anchor; a non-string entry used to reach + * `categoryOfDefKey` — whose parameter is declared `string` — and die on + * `key.indexOf is not a function`. All three call sites in `build-schemas.ts` + * print `error.message` and nothing else, so that bare text WAS the whole + * diagnostic: no file among 14 shards, no entry, no anchor. + * + * Note what these cases may not do: assert only that it throws. The unfixed + * reader throws too, so `expect(...).toThrow()` — and even `toThrow(/./)` — + * stays green on exactly the defect. The message is the contract here, so the + * file, the entry and the anchor are each pinned by name. + */ + it('names the shard file, the entry index and the anchor when an entry is not a string', () => { + writeShards(dir, authorableSurfaceShardTexts(KEYS)); + rewriteShard(dir, 'ui', (doc) => { + (doc.keys as unknown[])[0] = 12345; + }); + + const message = messageOf(() => aggregateCategoryShards(dir, 'keys')); + expect(message, 'names the shard file').toContain('ui.json'); + expect(message, 'names the entry').toContain('keys[0]'); + 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'); + // The regression this exists for, stated as itself. + expect(message, 'never the bare JS error again').not.toContain('indexOf'); + }); + + it('distinguishes null and object entries, which `typeof` alone reports as one', () => { + // `typeof null === 'object'` is the trap the type label exists for: telling + // an author "object" when they wrote `null` sends them looking for a brace. + writeShards(dir, authorableSurfaceShardTexts(KEYS)); + rewriteShard(dir, 'ui', (doc) => { + (doc.keys as unknown[])[1] = null; + }); + expect(messageOf(() => aggregateCategoryShards(dir, 'keys'))).toContain( + 'keys[1] is null, not a string', + ); + + writeShards(dir, authorableSurfaceShardTexts(KEYS)); + rewriteShard(dir, 'ui', (doc) => { + (doc.keys as unknown[])[1] = { 'ui/View:name': true }; + }); + expect(messageOf(() => aggregateCategoryShards(dir, 'keys'))).toContain( + 'keys[1] is an object, not a string', + ); + }); + + it('speaks the same way for every sharded artifact, naming that artifact’s own field', () => { + // `categoryOfDefKey` is shared by all three category-sharded ratchets + // (authorable-surface/, json-schema.manifest/, authorable-defaults/), so the + // dumb message appeared under three different prefixes. The field name is + // part of the message for the same reason the file name is. + // + // The entry here is an ARRAY on purpose, and it is the worst of the class: + // `['ui/View'].indexOf('/')` is a perfectly valid Array.prototype call that + // answers -1, so this case never produced a TypeError at all — it fell + // through to `cannot shard "ui/View": … has no category segment`, naming a + // key that is not in the file and a cause that is not the defect. A wrong + // diagnosis outranks a bare one, which is why the type is checked before + // the routing rather than left to whatever `indexOf` happens to mean. + writeShards(dir, schemaManifestShardTexts(['ai/Agent', 'ui/View', 'ui/Dashboard'])); + rewriteShard(dir, 'ui', (doc) => { + (doc.schemas as unknown[])[1] = ['ui/View']; + }); + + const message = messageOf(() => aggregateCategoryShards(dir, 'schemas')); + expect(message).toContain('ui.json'); + expect(message).toContain('schemas[1] is an array, not a string'); + expect(message).toContain('#5837'); + }); + it('refuses a stray file in a generator-owned directory', () => { writeShards(dir, authorableSurfaceShardTexts(KEYS)); fs.writeFileSync(path.join(dir, 'notes.txt'), 'scratch\n');