test(cli): os explain's key-retention sweep now judges the optional/required TABLE faces

packages/cli/test/commands.test.ts

@@ -197,7 +197,13 @@ describe('os explain — schema catalog accuracy', () => {
 // they get tests that ASSERT that reason. A guard reporting green over the
 // entries it never looked at is this card's own defect, one layer up.
 describe('os explain — every catalog entry swept against its spec schema (#14811)', () => {
-  type CatalogEntry = { name: string; example: string };
+  type CatalogRow = { name: string; type: string; description: string };
+  type CatalogEntry = {
+    name: string;
+    example: string;
+    required: CatalogRow[];
+    optional: CatalogRow[];
+  };
   const catalog = SCHEMAS as unknown as Record<string, CatalogEntry>;
 
   // The searched name surface, stated rather than assumed: absence below means
@@ -317,15 +323,14 @@ describe('os explain — every catalog entry swept against its spec schema (#148
   // is what notices if that reverses — with the entry named, instead of a green
   // sweep over a silently emptied example.
   //
-  // ⛔ It reads the EXAMPLE face only — the one face `evaluate` reads. The
-  // `optional` / `required` tables carry key names too, and no assertion in this
-  // file has ever looked at them; a row naming a key the schema does not have is
-  // the same defect on the other face (this card's own `filters` / `sort` lived
-  // on BOTH). Covering it is not a stricter version of this assertion but a
-  // different one: a table row is prose, not a key — `view`'s required row is
-  // spelled `list | form | listViews | formViews` — so it needs a way to tell a
-  // key name from a description before it can judge anything. Declared as a gap
-  // here rather than half-built.
+  // It reads the EXAMPLE face only — the one face `evaluate` reads. The
+  // `optional` / `required` tables carry key names too, and a row naming a key
+  // the schema does not have is the same defect on the other face (#16925's own
+  // `filters` / `sort` lived on BOTH). That face is covered by its own block
+  // below (#17266) rather than by a stricter version of this assertion, because
+  // it is a different question: a table row is prose until something decides it
+  // is a key name, so the other block has to carry that rule before it can judge
+  // any row at all.
   //
   // The walk is deliberately conservative — it reports a key present in the
   // INPUT and absent from the OUTPUT, and nothing else. Keys a schema ADDS
@@ -391,6 +396,202 @@ describe('os explain — every catalog entry swept against its spec schema (#148
     }
   }
 
+
+  // ── The OTHER face: the `required` / `optional` TABLE rows (#17266) ─────────
+  //
+  // Everything above judges the `example`. An entry has a second face that names
+  // keys — the two tables — and until this block landed no assertion in this
+  // file had ever read a row's `name` against a schema. (`SCHEMAS.object
+  // .optional.find(…)` in the #3244 pin above reads one row's `type` STRING; it
+  // never asks whether the row's NAME is a key.) The trap is the same one, on
+  // the other face and a step earlier: a reader copies a row, the schema
+  // silently strips the key, and the query runs unfiltered under an ordinary
+  // success. `os explain query` shipped `filters` / `sort` on BOTH faces; only
+  // the example half was guarded.
+  //
+  // ⭐ The design work is the ROW GRAMMAR, because a row's `name` is prose until
+  // something decides it is a key name. The declared rule: a row name is read as
+  // an ALTERNATION — split on `|`, trim — and the row is judged iff EVERY part
+  // is a bare identifier. That is deliberately wider than "one identifier": it
+  // is what makes `view`'s required row (`list | form | listViews | formViews`,
+  // four slot names in the `name` position) judgeable as four keys instead of
+  // waved through as prose. A row that does not fit the grammar is NOT silently
+  // skipped — it must be declared in `PROSE_ROWS` below, and a declaration that
+  // stops being needed fails too, so a stale one cannot go on un-judging a row.
+  //
+  // ⭐ Two techniques, because one entry does not read. `.shape` is the schema's
+  // own declared key set and is the primary reading; eight of the nine bound
+  // entries expose one. `ActionSchema` does not — it resolves to a pipe
+  // (`lazySchema(() => actionObject().refine(…))`), and a pipe has no shape — so
+  // its twelve rows are judged by ASKING the schema instead: plant the row's key
+  // on the entry's own parsing example with a sentinel value and read the
+  // verdict, never the value's validity. Measured across all nine entries on the
+  // day this landed, the two techniques agree on every row the shape one can
+  // see, and the probe returns ABSENT for control names on all nine — including
+  // `query`, whose open top level answers by DROPPING rather than refusing.
+  //
+  // ⛔ Each entry's test carries that control inline, and it is load-bearing
+  // rather than decorative: `action` has no second opinion, so a technique that
+  // answered "declared" to everything would report green over twelve rows and be
+  // indistinguishable from a real pass.
+  //
+  // ⛔ Like the sweep above, it does NOT fix what it turns red, and ⛔ it is
+  // never the schema that gives way: a row naming a key the schema does not have
+  // is corrected on the ROW (`packages/spec` is not this file's to relax).
+
+  // Row names that are prose rather than key names, each with the reason it
+  // cannot be judged, keyed `<entry>.<face>.<row name>`.
+  //
+  // ⭐ EMPTY today, and measured so: every row on all nine bound entries is a key
+  // name or an alternation of them. A new prose row turns the block below RED
+  // until it is declared here — ⛔ widening the grammar to make it green again
+  // re-opens exactly the face this block exists to close.
+  const PROSE_ROWS: Record<string, string> = {};
+
+  const KEY_NAME = /^[A-Za-z_$][A-Za-z0-9_$]*$/;
+
+  // `null` = prose: this row names no keys, so nothing below can judge it.
+  const rowKeyNames = (rowName: string): string[] | null => {
+    const parts = rowName.split('|').map((part) => part.trim());
+    return parts.every((part) => KEY_NAME.test(part)) ? parts : null;
+  };
+
+  // Technique 1 — the schema's own declared key set. `null` when the name does
+  // not resolve to something carrying a shape, which is the `action` case.
+  const declaredKeys = (schema: ZodLike): Set<string> | null => {
+    const shape = (schema as unknown as { shape?: unknown }).shape;
+    const resolved = typeof shape === 'function' ? (shape as () => unknown)() : shape;
+    if (typeof resolved !== 'object' || resolved === null || Array.isArray(resolved)) return null;
+    return new Set(Object.keys(resolved));
+  };
+
+  // Technique 2 — ask the schema. Three readings, none of them about whether the
+  // VALUE is acceptable:
+  //   • an `unrecognized_keys` issue naming the key -> ABSENT (a strict object
+  //     refused it by name);
+  //   • success with the key gone from the output -> ABSENT (an open object
+  //     stripped it silently — the failure mode this whole section exists for);
+  //   • anything else -> DECLARED. Either it survived, or the schema validated it
+  //     and rejected the sentinel — and only a key it knows can be validated. A
+  //     cross-field refinement cannot land here on an absent key: a strict object
+  //     reports the unknown key first, and an open one drops it before any
+  //     refinement sees it.
+  const PROBE_SENTINEL = '__os_explain_table_face_probe__';
+
+  const probeKey = (schema: ZodLike, example: unknown, key: string): boolean => {
+    const input = { ...(example as Record<string, unknown>), [key]: PROBE_SENTINEL };
+    const result = schema.safeParse(input);
+    if (result.success) {
+      return isWalkable(result.data) && Object.prototype.hasOwnProperty.call(result.data, key);
+    }
+    const issues = (result.error?.issues ?? []) as Array<{ code?: string; keys?: string[] }>;
+    return !issues.some(
+      (issue) => issue.code === 'unrecognized_keys' && issue.keys?.includes(key),
+    );
+  };
+
+  // A name no schema in this catalog declares, planted as the inline control.
+  const CONTROL_KEY = 'os_explain_table_face_control_key';
+
+  for (const [key, bound] of Object.entries(BOUND)) {
+    it(`os explain ${key} — every ${bound.schema} table row names a key the schema declares`, () => {
+      const schema = specSurface[bound.schema] as ZodLike;
+      const declared = declaredKeys(schema);
+      // The probe plants its key on the entry's own example, so it needs one
+      // that parses. Asserted rather than assumed: an entry that is BOTH
+      // known-broken and shape-less is judgeable by neither technique, and this
+      // says so instead of reporting green over rows nothing looked at.
+      if (declared === null) {
+        expect(
+          bound.card,
+          `os explain ${key}: ${bound.schema} has no readable shape, so its rows can only `
+            + 'be judged by planting keys on an example that parses — and this entry is '
+            + 'filed as known-broken, so its example does not. A third technique is owed '
+            + 'here; ⛔ do not let the rows go unjudged.',
+        ).toBeUndefined();
+      }
+      const example = declared === null ? evaluate(key) : undefined;
+      const technique = declared === null ? 'probe' : 'shape';
+      const isDeclaredKey = (name: string): boolean =>
+        declared === null ? probeKey(schema, example, name) : declared.has(name);
+
+      // Control: a name the schema cannot possibly declare must read ABSENT.
+      // Without it, a technique that answers "declared" to everything reports
+      // green over every row below — and for `action` there is no second
+      // opinion to catch that.
+      expect(
+        isDeclaredKey(CONTROL_KEY),
+        `os explain ${key}: the ${technique} technique called \`${CONTROL_KEY}\` a declared `
+          + `key of ${bound.schema}. It cannot tell a real key from an absent one, so every `
+          + 'row verdict in this test is meaningless.',
+      ).toBe(false);
+
+      const offenders: string[] = [];
+      const undeclaredProse: string[] = [];
+      const staleProse: string[] = [];
+
+      for (const face of ['required', 'optional'] as const) {
+        for (const row of catalog[key][face]) {
+          const id = `${key}.${face}.${row.name}`;
+          const names = rowKeyNames(row.name);
+          if (names === null) {
+            if (PROSE_ROWS[id] === undefined) undeclaredProse.push(id);
+            continue;
+          }
+          if (PROSE_ROWS[id] !== undefined) {
+            staleProse.push(id);
+            continue;
+          }
+          for (const name of names) {
+            if (!isDeclaredKey(name)) offenders.push(`${face} row \`${row.name}\` -> \`${name}\``);
+          }
+        }
+      }
+
+      expect(
+        undeclaredProse,
+        `os explain ${key}: table row(s) whose \`name\` is neither a key name nor an `
+          + 'alternation of them, so nothing here can judge them. Either spell the row as key '
+          + 'names, or declare it in `PROSE_ROWS` with the reason — ⛔ do not widen the '
+          + 'grammar, which un-judges every row it lets through.',
+      ).toEqual([]);
+
+      expect(
+        staleProse,
+        `os explain ${key}: \`PROSE_ROWS\` declares row(s) that DO parse as key names now. A `
+          + 'stale declaration goes on un-judging a row that could be judged — delete the '
+          + 'entry.',
+      ).toEqual([]);
+
+      expect(
+        offenders,
+        `os explain ${key}: table row(s) name key(s) ${bound.schema} does NOT declare `
+          + `(judged by the ${technique} technique). An author who copies such a row gets `
+          + 'one of two bad outcomes: a strict schema REFUSES the whole object by name, and '
+          + 'an OPEN one takes it, strips the key and reports success. The second is the trap '
+          + '`os explain query` shipped — a copied row, a silently stripped key, and a query '
+          + 'that ran unfiltered under an ordinary success. Correct the ROW to the schema\'s '
+          + 'own spelling (⛔ do not relax the schema to accept it).',
+      ).toEqual([]);
+    });
+  }
+
+  // The same completeness property the `classifies every entry` test asserts for
+  // the catalog, asserted for this face: the two UNBOUND entries have no schema
+  // to judge rows against, so the sweep above cannot reach them. Green over rows
+  // nothing looked at is the defect this whole section is about, so their
+  // emptiness is asserted rather than assumed.
+  it('the two UNBOUND entries carry no table rows — no key name on this face is unswept', () => {
+    for (const key of Object.keys(UNBOUND)) {
+      expect(
+        [...catalog[key].required, ...catalog[key].optional].map((row) => row.name),
+        `os explain ${key} is a redirect with no schema to judge rows against, so a table `
+          + 'row here names keys nothing can check. Bind the entry to a schema, or keep the '
+          + 'tables empty and put the guidance in the description.',
+      ).toEqual([]);
+    }
+  });
+
   it(`os explain workflow — ${UNBOUND.workflow}`, () => {
     expect('WorkflowSchema' in specSurface).toBe(false);
     expect(catalog.workflow.name).toContain('no standalone type');