fix(spec): the two remaining shard readers name the file, entry and anchor for a non-string entry (#7076)

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 ` +
+          `"<Name> (<kind>)" 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' };
 }

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<string, string[]>`.
+   *
+   * 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 (`<field>[<i>] is <type>, not a string (#5837): <value>`) 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) => {