From 87d2a43a6905c1c40a38d079256e3de17a75faeb Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 20:55:43 +0000 Subject: [PATCH 1/2] test(cli): the os explain key-retention sweep now judges the TABLE faces too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep read the `example` face only — the one face `evaluate` reads. The `required` / `optional` tables name keys as well, and no assertion in this file had ever read a row's `name` against a schema, so the half of #16925's defect that lived in the table rows was unguarded: a reader copies a row, the schema silently strips the key, and the query runs unfiltered under an ordinary success. The design work is the row grammar. A row name is read as an ALTERNATION — split on `|`, trim — and judged iff every part is a bare identifier, which makes `view`'s four-slot required row judgeable as four keys instead of waved through as prose. A row that does not fit must be declared in `PROSE_ROWS` (empty today), and a stale declaration fails too. Two techniques, because one entry does not read: `.shape` for the eight bound entries that expose one, and a behavioural probe for `ActionSchema`, which resolves to a pipe. Each entry's test carries an inline control key that must read ABSENT, since `action` has no second opinion. Every row on all nine bound entries names a real key today, so this lands green. Claude-Session: https://claude.ai/code/session_01DvvamiacK328idtBYJBxV3 Co-authored-by: Claude --- packages/cli/test/commands.test.ts | 219 +++++++++++++++++++++++++++-- 1 file changed, 209 insertions(+), 10 deletions(-) diff --git a/packages/cli/test/commands.test.ts b/packages/cli/test/commands.test.ts index d793d401057..46afc940adf 100644 --- a/packages/cli/test/commands.test.ts +++ b/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; // 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,200 @@ 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 `..`. + // + // ⭐ 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 = {}; + + 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 | 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), [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 a ` + + 'parse that succeeds and a value with that key gone — no error, no warning, and ' + + 'a query that runs unfiltered. 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'); From 4fb225c6efe3af9b0b72ea58d1db22dbb16ee787 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 20:57:31 +0000 Subject: [PATCH 2/2] test(cli): state both failure modes in the table-row message, not only the open one A strict schema refuses the row's key by name; an open one takes the object, strips the key and reports success. The message named only the second, so the verdict read wrong on the eight strict entries. Claude-Session: https://claude.ai/code/session_01DvvamiacK328idtBYJBxV3 Co-authored-by: Claude --- packages/cli/test/commands.test.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/cli/test/commands.test.ts b/packages/cli/test/commands.test.ts index 46afc940adf..905fc332e87 100644 --- a/packages/cli/test/commands.test.ts +++ b/packages/cli/test/commands.test.ts @@ -566,10 +566,12 @@ describe('os explain — every catalog entry swept against its spec schema (#148 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 a ` - + 'parse that succeeds and a value with that key gone — no error, no warning, and ' - + 'a query that runs unfiltered. Correct the ROW to the schema\'s own spelling ' - + '(⛔ do not relax the schema to accept it).', + + `(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([]); }); }