Skip to content

Commit 4e6ca32

Browse files
fix(spec): the shard reader names the file, entry and anchor for a non-string entry (#6751) (#7075)
`aggregateCategoryShards` cast each shard's array to `string[]` and handed the entries straight to `categoryOfDefKey`, whose parameter is declared `string`. A hand-edited non-string entry therefore died inside the helper on a bare `key.indexOf is not a function`. All three call sites in `build-schemas.ts` print `error.message` and nothing else, so that text was the whole diagnostic: no shard file among 14, no entry, no issue anchor — while the reader's other three defect classes all name the file and carry one. The check lands at the reader, not in `categoryOfDefKey`: the helper's contract already says `string`, and only its caller knows the file name. The `as string[]` cast is replaced by a real `typeof` check that narrows, so the entry type is now verified where untyped JSON enters rather than asserted. Gate behaviour is unchanged — same exit 1, same verdicts. Claude-Session: https://claude.ai/code/session_01AZgRyPVwi1jLb1mNNuUQ9o Co-authored-by: Claude <noreply@anthropic.com>
1 parent b88f5e8 commit 4e6ca32

2 files changed

Lines changed: 129 additions & 1 deletion

File tree

packages/spec/scripts/lib/sharded-artifacts.ts

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,18 @@ function shardTextsByCategory(
340340
return out;
341341
}
342342

343+
/**
344+
* The JSON type of a parsed value, article included, for a message that has to
345+
* tell an author what they actually wrote. `typeof` alone answers `"object"` for
346+
* both `null` and `[]` — the two hand-edit accidents most worth telling apart.
347+
*/
348+
function jsonTypeLabel(value: unknown): string {
349+
if (value === null) return 'null';
350+
if (Array.isArray(value)) return 'an array';
351+
const type = typeof value;
352+
return `${/^[aeiou]/.test(type) ? 'an' : 'a'} ${type}`;
353+
}
354+
343355
/**
344356
* Aggregate a category-sharded directory into one sorted array, validating that
345357
* each shard answers only for its own category.
@@ -370,7 +382,23 @@ export function aggregateCategoryShards(
370382
if (!Array.isArray(list)) {
371383
throw new Error(`${dirName}/${shard.name}.json has no "${field}" array (#5837).`);
372384
}
373-
for (const raw of list as string[]) {
385+
// `Array.isArray` says the field IS an array and nothing about what is in
386+
// it, so this is where untyped JSON stops being untyped. A hand-edited
387+
// non-string entry used to reach `categoryOfDefKey`, whose parameter is
388+
// declared `string`, and die there on `key.indexOf is not a function` —
389+
// right exit code, but all three call sites print `error.message` alone, so
390+
// the author was told neither the shard file nor the entry (#6751). Every
391+
// other defect class this reader rejects names both; so does this one now.
392+
// The check belongs here rather than in `categoryOfDefKey`: the helper's
393+
// contract already says `string`, and only its caller knows the file name.
394+
for (const [index, raw] of (list as readonly unknown[]).entries()) {
395+
if (typeof raw !== 'string') {
396+
throw new Error(
397+
`${dirName}/${shard.name}.json ${field}[${index}] is ${jsonTypeLabel(raw)}, not a string ` +
398+
`(#5837): ${JSON.stringify(raw)}. Every entry is a "<category>/<Def>[:<prop>]" key — ` +
399+
`regenerate rather than reconcile by hand.`,
400+
);
401+
}
374402
if (categoryOfDefKey(raw) !== shard.name) {
375403
throw new Error(
376404
`${dirName}/${shard.name}.json carries "${raw}", which belongs to category ` +

packages/spec/scripts/sharded-artifacts.test.ts

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,33 @@ function changed(before: Map<string, string>, after: Map<string, string>): strin
8787
return [...names].filter((n) => before.get(n) !== after.get(n)).sort();
8888
}
8989

90+
/**
91+
* The message of the error `run` throws, for the cases where the MESSAGE is the
92+
* thing under test (#6751). `expect(...).toThrow()` cannot serve those: the
93+
* reader threw before the fix too — with a bare `TypeError` — so a throw-only
94+
* assertion is green on the defect it is supposed to pin.
95+
*/
96+
function messageOf(run: () => unknown): string {
97+
try {
98+
run();
99+
} catch (error) {
100+
return error instanceof Error ? error.message : String(error);
101+
}
102+
return expect.fail('expected the shard reader to reject, but it returned a value');
103+
}
104+
105+
/** Rewrite one shard's parsed document, in the canonical byte shape. */
106+
function rewriteShard(
107+
root: string,
108+
name: string,
109+
mutate: (doc: Record<string, unknown>) => void,
110+
): void {
111+
const file = path.join(root, `${name}.json`);
112+
const doc = JSON.parse(fs.readFileSync(file, 'utf-8'));
113+
mutate(doc);
114+
fs.writeFileSync(file, JSON.stringify(doc, null, 2) + '\n');
115+
}
116+
90117
describe('sharded artifacts — locality: a change in one category moves one file (#5837)', () => {
91118
it('routes a key to the shard its category segment names, and only that one', () => {
92119
// 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)'
229256
expect(() => aggregateCategoryShards(dir, 'keys')).toThrow(/declares category "data"/);
230257
});
231258

259+
/**
260+
* The fourth defect class of this reader (#6751). The other three name the
261+
* shard file and carry an issue anchor; a non-string entry used to reach
262+
* `categoryOfDefKey` — whose parameter is declared `string` — and die on
263+
* `key.indexOf is not a function`. All three call sites in `build-schemas.ts`
264+
* print `error.message` and nothing else, so that bare text WAS the whole
265+
* diagnostic: no file among 14 shards, no entry, no anchor.
266+
*
267+
* Note what these cases may not do: assert only that it throws. The unfixed
268+
* reader throws too, so `expect(...).toThrow()` — and even `toThrow(/./)` —
269+
* stays green on exactly the defect. The message is the contract here, so the
270+
* file, the entry and the anchor are each pinned by name.
271+
*/
272+
it('names the shard file, the entry index and the anchor when an entry is not a string', () => {
273+
writeShards(dir, authorableSurfaceShardTexts(KEYS));
274+
rewriteShard(dir, 'ui', (doc) => {
275+
(doc.keys as unknown[])[0] = 12345;
276+
});
277+
278+
const message = messageOf(() => aggregateCategoryShards(dir, 'keys'));
279+
expect(message, 'names the shard file').toContain('ui.json');
280+
expect(message, 'names the entry').toContain('keys[0]');
281+
expect(message, 'carries the issue anchor').toContain('#5837');
282+
expect(message, 'says what was found instead').toContain('is a number, not a string');
283+
expect(message, 'quotes the offending value').toContain('12345');
284+
// The regression this exists for, stated as itself.
285+
expect(message, 'never the bare JS error again').not.toContain('indexOf');
286+
});
287+
288+
it('distinguishes null and object entries, which `typeof` alone reports as one', () => {
289+
// `typeof null === 'object'` is the trap the type label exists for: telling
290+
// an author "object" when they wrote `null` sends them looking for a brace.
291+
writeShards(dir, authorableSurfaceShardTexts(KEYS));
292+
rewriteShard(dir, 'ui', (doc) => {
293+
(doc.keys as unknown[])[1] = null;
294+
});
295+
expect(messageOf(() => aggregateCategoryShards(dir, 'keys'))).toContain(
296+
'keys[1] is null, not a string',
297+
);
298+
299+
writeShards(dir, authorableSurfaceShardTexts(KEYS));
300+
rewriteShard(dir, 'ui', (doc) => {
301+
(doc.keys as unknown[])[1] = { 'ui/View:name': true };
302+
});
303+
expect(messageOf(() => aggregateCategoryShards(dir, 'keys'))).toContain(
304+
'keys[1] is an object, not a string',
305+
);
306+
});
307+
308+
it('speaks the same way for every sharded artifact, naming that artifact’s own field', () => {
309+
// `categoryOfDefKey` is shared by all three category-sharded ratchets
310+
// (authorable-surface/, json-schema.manifest/, authorable-defaults/), so the
311+
// dumb message appeared under three different prefixes. The field name is
312+
// part of the message for the same reason the file name is.
313+
//
314+
// The entry here is an ARRAY on purpose, and it is the worst of the class:
315+
// `['ui/View'].indexOf('/')` is a perfectly valid Array.prototype call that
316+
// answers -1, so this case never produced a TypeError at all — it fell
317+
// through to `cannot shard "ui/View": … has no category segment`, naming a
318+
// key that is not in the file and a cause that is not the defect. A wrong
319+
// diagnosis outranks a bare one, which is why the type is checked before
320+
// the routing rather than left to whatever `indexOf` happens to mean.
321+
writeShards(dir, schemaManifestShardTexts(['ai/Agent', 'ui/View', 'ui/Dashboard']));
322+
rewriteShard(dir, 'ui', (doc) => {
323+
(doc.schemas as unknown[])[1] = ['ui/View'];
324+
});
325+
326+
const message = messageOf(() => aggregateCategoryShards(dir, 'schemas'));
327+
expect(message).toContain('ui.json');
328+
expect(message).toContain('schemas[1] is an array, not a string');
329+
expect(message).toContain('#5837');
330+
});
331+
232332
it('refuses a stray file in a generator-owned directory', () => {
233333
writeShards(dir, authorableSurfaceShardTexts(KEYS));
234334
fs.writeFileSync(path.join(dir, 'notes.txt'), 'scratch\n');

0 commit comments

Comments
 (0)