Skip to content

Commit 582d3e5

Browse files
test(cli): os explain's key-retention sweep now judges the optional/required TABLE faces (#18538)
Fixes #17266 Clause-②: no ## What this is A **test-only pin**. There is no defect in the tree today — the card says so itself (⛔ *"Not a defect in what #16925 shipped — a declared gap in the guard it built"*) — and that was re-measured here rather than taken on trust: **every row on all nine bound entries names a real key.** So this lands green. The key-retention sweep in `packages/cli/test/commands.test.ts` reads the **EXAMPLE** face only, because `evaluate(key)` is the one face it reads. A catalog entry names keys on a **second** face — its `required` / `optional` tables — and no assertion in that file had ever read a row's `name` against a schema. (The #3244 pin above it reads one row's `type` STRING; it never asks whether the row's NAME is a key.) The trap is the same one 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 taker was the file itself.** The gap was declared in the test's own header, so that header is rewritten in the same diff — it no longer announces a gap that is closed, and it now points at the block that closes it. ## The row grammar — the actual design work 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 the pipe character, trim — and the row is judged **iff every part is a bare identifier**. That is deliberately wider than "one identifier", and the width is what earns its keep: it makes `view`'s required row (`list | form | listViews | formViews`, four slot names in the `name` position — the card's own blocking example) judgeable as **four keys** instead of waved through as prose. Measured: all four are real `ViewSchema` keys. A row that does not fit the grammar is ⛔ **not silently skipped** — it must be declared in `PROSE_ROWS`, and a declaration that stops being needed fails too, so a stale one cannot go on un-judging a row. `PROSE_ROWS` is **empty today**, measured: every row on all nine bound entries fits. ## Two techniques, because one entry does not read | technique | entries | reading | |:---|:---|:---| | `.shape` | 8 of 9 | the schema's own declared key set | | behavioural probe | `action` | plant the row's key on the entry's parsing example with a sentinel and read the verdict | `ActionSchema` resolves to a **pipe** (`lazySchema(() =` the arrow `actionObject().refine(...))`), and a pipe has no shape — the card predicted this and it holds. The probe never asks whether the VALUE is acceptable, only whether the schema KNOWS the name: an `unrecognized_keys` issue naming it is ABSENT (a strict object refused it), success with the key gone from the output is ABSENT (an open object stripped it silently), anything else is DECLARED. Measured across all nine entries: the two techniques **agree on every row the shape one can see**, and the probe returns ABSENT for 7 control names on all 9 entries — including `query`, whose open top level answers by DROPPING rather than refusing, which is the exact silent-strip mode this guard exists for. ⭐ Each entry's test carries an inline **control key** that must read ABSENT. That 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. ## Premises verified before writing any code 1. **"The table faces are unguarded today."** ✅ Located from its SYMBOL, never a line number. The only read of a table face in the whole file is `SCHEMAS.object.optional.find(...)` in the #3244 pin, which asserts the row's `type` string. Control words known present in the same file: `evaluate(` × 6, `.example` × 4. 2. **"`packages/cli/test/commands.test.ts` is the right and only place."** ✅ It is the **only** file in the repository that imports the explain catalog at all (repo-wide grep for `commands/explain`, excluding `node_modules` and `dist`). No sibling pin exists. 3. **"No table row is wrong today."** ✅ Confirmed by both techniques independently, on all 9 bound entries, every row, every alternation part. Zero disagreement, zero offenders. The two UNBOUND redirect entries carry zero rows. ## Ablation — a pin that fails on the wrong fix Two legs, both run from the **committed** state, both mutating the source the pin reads (`packages/cli/src/commands/explain.ts`), each with a trap restore on absolute paths. `HEAD` blob for that file: `09ab4c720ef0a52ffccbf9c19100a307d16568ef` **Leg 1 — `query`, the shape technique, silent-drop mode.** Table row `where` rewritten to `filters` (the historical defect, on the table face only — the example keeps `where`). - occurrence counts on disk: before `from=1 to=0`, after `from=0 to=1` - mutated blob `535ff0bf015b9a757d2a89dabb666cfc3fae7a2f` (not equal to HEAD blob) - vitest exit **1** — `os explain query — every QuerySchema table row names a key the schema declares` ✗, `Tests 1 failed | 49 passed (50)` - ⭐ **`os explain query — example survives QuerySchema with every declared key intact` stayed GREEN.** That is the card's claim made mechanical: the pre-existing retention assertion cannot see this face. - restored blob `09ab4c72...` equals the HEAD blob; `git diff HEAD` on the path EMPTY; `git status --porcelain` empty **Leg 2 — `action`, the probe technique, refuse-by-name mode.** Required row `target` rewritten to `flow` (the key the entry's own description says does not exist). - occurrence counts on disk: before `from=1 to=0`, after `from=0 to=1` - mutated blob `279748e6d171a66640fd020e37a24a8934e97ab7` (not equal to HEAD blob) - vitest exit **1** — `os explain action — every ActionSchema table row names a key the schema declares` ✗, `Tests 1 failed | 49 passed (50)`; both the parse and retention assertions on `action` stayed green - restored blob equals the HEAD blob; `git diff HEAD` on the path EMPTY; porcelain empty Final tree state after both legs: `git diff HEAD` exit **0** (clean). ⛔ No permanent ablation artefact is left behind. **No dist leg is owed here**: the mutated subject is imported by the test relatively (`from '../src/commands/explain'`), so it resolves from **source**, not through a package `exports` entry — there is no built artefact in the resolution path for it. ## Verification Run on `4fb225c6e` (the tip this PR opens with). **Gate derivation** — `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack`, derived from the actual changed path (1 path, `packages/cli/test/commands.test.ts`), re-derived after the final commit and **byte-identical**. Reconciled with `--ran` carrying exit codes: ``` Run reconciliation — 49 derived, 48 run, 1 NOT-MEASURED, 0 UNRUN. ``` - **47 green.** - ⚠️ `pnpm check:cross-package-test-inputs` exit **1** — PRE-EXISTING, ⛔ not this PR's. It fires wherever `packages/spec/dist` is built and names `packages/cli/test/init-created-files-summary.e2e.test.ts` (a file this diff does not touch), reporting that `packages/cli` descends `packages/spec/dist/` with no declared glob reaching inside it. Already filed as #18353 / #18348. ⛔ No new card. - ⚠️ `pnpm check:dual-build-cjs-loads` exit **3** — `PREREQUISITE NOT MET`, i.e. **NOT MEASURED**, ⛔ neither pass nor fail. It reads built output and 12 packages outside this diff's closure have no `dist/`; only a whole-repo `pnpm build` satisfies it, which CI does. This diff touches no `src/`, so it can move no `dist/`. **Beyond the derivation:** | check | result | |:---|:---| | `pnpm lint` (repo-wide, `eslint . --no-inline-config`) | exit **0** | | `pnpm --filter @objectstack/cli typecheck` | exit **0** (`tsc --noEmit` + `check:test-typecheck`) | | `pnpm --filter @objectstack/cli exec vitest run --project unit` (the tier this file runs in) | **211 files / 3013 tests passed** | | the edited file alone, verbose | **50 passed** (40 before, 10 new: 9 per-entry + 1 UNBOUND completeness) | ⭐ Typecheck coverage of the edited file asserted rather than assumed: `tsc -p tsconfig.test.json --listFiles` names `test/commands.test.ts` (the package's plain `tsconfig.json` does not, which is why the `typecheck` script chains `check:test-typecheck`). Raw `tsc -p tsconfig.test.json` reports 28 errors across 3 OTHER files, all ledgered and shrink-only; **zero** are in `test/commands.test.ts`. The `integration` tier is declared to CI: this diff touches no integration-layer file, no spawn entry point (`bin/`, `test/helpers/serve-process.ts`) and no driver/kernel boot path, and `commands.test.ts` lands in `unit` under `packages/cli/vitest-tiers.ts` (neither SPAWN nor KERNEL). ## Changeset — measured, not assumed `skip-changeset` is the correct disposition, and it was **measured** rather than reasoned from the path: - `@objectstack/cli`'s `files[]` is `["dist", "README.md", "CHANGELOG.md"]`; `npm pack --dry-run` ships `dist/`, `bin/`, `README.md`, `CHANGELOG.md`, `LICENSE`, `package.json` — no test source. - Symbol grep over the shipped paths, **with a positive control**: `SCHEMAS` (from `src/commands/explain.ts`) IS found in `dist/commands/explain.js`, so the grep can find a shipped symbol. Each of `PROSE_ROWS`, `os_explain_table_face_control_key`, `__os_explain_table_face_probe__`, `rowKeyNames` -> **0 shipped files**. - A diff that touches no `src/` cannot move `dist`. ⚠️ **`skip-changeset` is OWED on this PR and the author was fenced from writing labels by its dispatch.** `changeset-check` in `pr-automation.yml` has no path-based exemption — it requires either a changeset or that label — so this PR goes red until a seat applies it. Flagged here and in the report rather than applied. ## Acceptance notes Out of scope, noted and ⛔ not filed: - `os explain query`'s optional table lists 6 of `QuerySchema`'s 17 keys. The card declares missing rows out of scope by name and explains why (a **missing** row is an omission, not an error — nothing an author copies fails or is silently dropped). This PR's grammar judges rows that are PRESENT; it deliberately says nothing about absent ones. **Taker: none** — stated so the boundary stays legible. - `view`'s prose spells `filters` in a line comment and `filter` in the entry description. A code-comment inconsistency with no runtime or authoring reach; the card routes it to **#15171**, which owns that entry. ⛔ Not touched here (editing `view`'s entry is fenced). - ⛔ No `packages/spec` schema was relaxed, considered or touched. The new assertion's failure message says so in its own text: a wrong row is corrected on the ROW. ## The floor question, held This adds assertion coverage **inside a test file that already runs in CI** — a blind-spot repair of an EXISTING gate. ⛔ It adds no new required gate, hook or ratchet, no workflow step, no `check:*` script, no `package.json` script. The distinction ruling F draws (adding a door that must be passed vs. making an existing door see more) was never reached in the wrong direction; nothing in the delivery required it. --- _Generated by [Claude Code](https://claude.ai/code/session_01DvvamiacK328idtBYJBxV3)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2e0401a commit 582d3e5

1 file changed

Lines changed: 211 additions & 10 deletions

File tree

‎packages/cli/test/commands.test.ts‎

Lines changed: 211 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,13 @@ describe('os explain — schema catalog accuracy', () => {
197197
// they get tests that ASSERT that reason. A guard reporting green over the
198198
// entries it never looked at is this card's own defect, one layer up.
199199
describe('os explain — every catalog entry swept against its spec schema (#14811)', () => {
200-
type CatalogEntry = { name: string; example: string };
200+
type CatalogRow = { name: string; type: string; description: string };
201+
type CatalogEntry = {
202+
name: string;
203+
example: string;
204+
required: CatalogRow[];
205+
optional: CatalogRow[];
206+
};
201207
const catalog = SCHEMAS as unknown as Record<string, CatalogEntry>;
202208

203209
// 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
317323
// is what notices if that reverses — with the entry named, instead of a green
318324
// sweep over a silently emptied example.
319325
//
320-
// ⛔ It reads the EXAMPLE face only — the one face `evaluate` reads. The
321-
// `optional` / `required` tables carry key names too, and no assertion in this
322-
// file has ever looked at them; a row naming a key the schema does not have is
323-
// the same defect on the other face (this card's own `filters` / `sort` lived
324-
// on BOTH). Covering it is not a stricter version of this assertion but a
325-
// different one: a table row is prose, not a key — `view`'s required row is
326-
// spelled `list | form | listViews | formViews` — so it needs a way to tell a
327-
// key name from a description before it can judge anything. Declared as a gap
328-
// here rather than half-built.
326+
// It reads the EXAMPLE face only — the one face `evaluate` reads. The
327+
// `optional` / `required` tables carry key names too, and a row naming a key
328+
// the schema does not have is the same defect on the other face (#16925's own
329+
// `filters` / `sort` lived on BOTH). That face is covered by its own block
330+
// below (#17266) rather than by a stricter version of this assertion, because
331+
// it is a different question: a table row is prose until something decides it
332+
// is a key name, so the other block has to carry that rule before it can judge
333+
// any row at all.
329334
//
330335
// The walk is deliberately conservative — it reports a key present in the
331336
// 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
391396
}
392397
}
393398

399+
400+
// ── The OTHER face: the `required` / `optional` TABLE rows (#17266) ─────────
401+
//
402+
// Everything above judges the `example`. An entry has a second face that names
403+
// keys — the two tables — and until this block landed no assertion in this
404+
// file had ever read a row's `name` against a schema. (`SCHEMAS.object
405+
// .optional.find(…)` in the #3244 pin above reads one row's `type` STRING; it
406+
// never asks whether the row's NAME is a key.) The trap is the same one, on
407+
// the other face and a step earlier: a reader copies a row, the schema
408+
// silently strips the key, and the query runs unfiltered under an ordinary
409+
// success. `os explain query` shipped `filters` / `sort` on BOTH faces; only
410+
// the example half was guarded.
411+
//
412+
// ⭐ The design work is the ROW GRAMMAR, because a row's `name` is prose until
413+
// something decides it is a key name. The declared rule: a row name is read as
414+
// an ALTERNATION — split on `|`, trim — and the row is judged iff EVERY part
415+
// is a bare identifier. That is deliberately wider than "one identifier": it
416+
// is what makes `view`'s required row (`list | form | listViews | formViews`,
417+
// four slot names in the `name` position) judgeable as four keys instead of
418+
// waved through as prose. A row that does not fit the grammar is NOT silently
419+
// skipped — it must be declared in `PROSE_ROWS` below, and a declaration that
420+
// stops being needed fails too, so a stale one cannot go on un-judging a row.
421+
//
422+
// ⭐ Two techniques, because one entry does not read. `.shape` is the schema's
423+
// own declared key set and is the primary reading; eight of the nine bound
424+
// entries expose one. `ActionSchema` does not — it resolves to a pipe
425+
// (`lazySchema(() => actionObject().refine(…))`), and a pipe has no shape — so
426+
// its twelve rows are judged by ASKING the schema instead: plant the row's key
427+
// on the entry's own parsing example with a sentinel value and read the
428+
// verdict, never the value's validity. Measured across all nine entries on the
429+
// day this landed, the two techniques agree on every row the shape one can
430+
// see, and the probe returns ABSENT for control names on all nine — including
431+
// `query`, whose open top level answers by DROPPING rather than refusing.
432+
//
433+
// ⛔ Each entry's test carries that control inline, and it is load-bearing
434+
// rather than decorative: `action` has no second opinion, so a technique that
435+
// answered "declared" to everything would report green over twelve rows and be
436+
// indistinguishable from a real pass.
437+
//
438+
// ⛔ Like the sweep above, it does NOT fix what it turns red, and ⛔ it is
439+
// never the schema that gives way: a row naming a key the schema does not have
440+
// is corrected on the ROW (`packages/spec` is not this file's to relax).
441+
442+
// Row names that are prose rather than key names, each with the reason it
443+
// cannot be judged, keyed `<entry>.<face>.<row name>`.
444+
//
445+
// ⭐ EMPTY today, and measured so: every row on all nine bound entries is a key
446+
// name or an alternation of them. A new prose row turns the block below RED
447+
// until it is declared here — ⛔ widening the grammar to make it green again
448+
// re-opens exactly the face this block exists to close.
449+
const PROSE_ROWS: Record<string, string> = {};
450+
451+
const KEY_NAME = /^[A-Za-z_$][A-Za-z0-9_$]*$/;
452+
453+
// `null` = prose: this row names no keys, so nothing below can judge it.
454+
const rowKeyNames = (rowName: string): string[] | null => {
455+
const parts = rowName.split('|').map((part) => part.trim());
456+
return parts.every((part) => KEY_NAME.test(part)) ? parts : null;
457+
};
458+
459+
// Technique 1 — the schema's own declared key set. `null` when the name does
460+
// not resolve to something carrying a shape, which is the `action` case.
461+
const declaredKeys = (schema: ZodLike): Set<string> | null => {
462+
const shape = (schema as unknown as { shape?: unknown }).shape;
463+
const resolved = typeof shape === 'function' ? (shape as () => unknown)() : shape;
464+
if (typeof resolved !== 'object' || resolved === null || Array.isArray(resolved)) return null;
465+
return new Set(Object.keys(resolved));
466+
};
467+
468+
// Technique 2 — ask the schema. Three readings, none of them about whether the
469+
// VALUE is acceptable:
470+
// • an `unrecognized_keys` issue naming the key -> ABSENT (a strict object
471+
// refused it by name);
472+
// • success with the key gone from the output -> ABSENT (an open object
473+
// stripped it silently — the failure mode this whole section exists for);
474+
// • anything else -> DECLARED. Either it survived, or the schema validated it
475+
// and rejected the sentinel — and only a key it knows can be validated. A
476+
// cross-field refinement cannot land here on an absent key: a strict object
477+
// reports the unknown key first, and an open one drops it before any
478+
// refinement sees it.
479+
const PROBE_SENTINEL = '__os_explain_table_face_probe__';
480+
481+
const probeKey = (schema: ZodLike, example: unknown, key: string): boolean => {
482+
const input = { ...(example as Record<string, unknown>), [key]: PROBE_SENTINEL };
483+
const result = schema.safeParse(input);
484+
if (result.success) {
485+
return isWalkable(result.data) && Object.prototype.hasOwnProperty.call(result.data, key);
486+
}
487+
const issues = (result.error?.issues ?? []) as Array<{ code?: string; keys?: string[] }>;
488+
return !issues.some(
489+
(issue) => issue.code === 'unrecognized_keys' && issue.keys?.includes(key),
490+
);
491+
};
492+
493+
// A name no schema in this catalog declares, planted as the inline control.
494+
const CONTROL_KEY = 'os_explain_table_face_control_key';
495+
496+
for (const [key, bound] of Object.entries(BOUND)) {
497+
it(`os explain ${key} — every ${bound.schema} table row names a key the schema declares`, () => {
498+
const schema = specSurface[bound.schema] as ZodLike;
499+
const declared = declaredKeys(schema);
500+
// The probe plants its key on the entry's own example, so it needs one
501+
// that parses. Asserted rather than assumed: an entry that is BOTH
502+
// known-broken and shape-less is judgeable by neither technique, and this
503+
// says so instead of reporting green over rows nothing looked at.
504+
if (declared === null) {
505+
expect(
506+
bound.card,
507+
`os explain ${key}: ${bound.schema} has no readable shape, so its rows can only `
508+
+ 'be judged by planting keys on an example that parses — and this entry is '
509+
+ 'filed as known-broken, so its example does not. A third technique is owed '
510+
+ 'here; ⛔ do not let the rows go unjudged.',
511+
).toBeUndefined();
512+
}
513+
const example = declared === null ? evaluate(key) : undefined;
514+
const technique = declared === null ? 'probe' : 'shape';
515+
const isDeclaredKey = (name: string): boolean =>
516+
declared === null ? probeKey(schema, example, name) : declared.has(name);
517+
518+
// Control: a name the schema cannot possibly declare must read ABSENT.
519+
// Without it, a technique that answers "declared" to everything reports
520+
// green over every row below — and for `action` there is no second
521+
// opinion to catch that.
522+
expect(
523+
isDeclaredKey(CONTROL_KEY),
524+
`os explain ${key}: the ${technique} technique called \`${CONTROL_KEY}\` a declared `
525+
+ `key of ${bound.schema}. It cannot tell a real key from an absent one, so every `
526+
+ 'row verdict in this test is meaningless.',
527+
).toBe(false);
528+
529+
const offenders: string[] = [];
530+
const undeclaredProse: string[] = [];
531+
const staleProse: string[] = [];
532+
533+
for (const face of ['required', 'optional'] as const) {
534+
for (const row of catalog[key][face]) {
535+
const id = `${key}.${face}.${row.name}`;
536+
const names = rowKeyNames(row.name);
537+
if (names === null) {
538+
if (PROSE_ROWS[id] === undefined) undeclaredProse.push(id);
539+
continue;
540+
}
541+
if (PROSE_ROWS[id] !== undefined) {
542+
staleProse.push(id);
543+
continue;
544+
}
545+
for (const name of names) {
546+
if (!isDeclaredKey(name)) offenders.push(`${face} row \`${row.name}\` -> \`${name}\``);
547+
}
548+
}
549+
}
550+
551+
expect(
552+
undeclaredProse,
553+
`os explain ${key}: table row(s) whose \`name\` is neither a key name nor an `
554+
+ 'alternation of them, so nothing here can judge them. Either spell the row as key '
555+
+ 'names, or declare it in `PROSE_ROWS` with the reason — ⛔ do not widen the '
556+
+ 'grammar, which un-judges every row it lets through.',
557+
).toEqual([]);
558+
559+
expect(
560+
staleProse,
561+
`os explain ${key}: \`PROSE_ROWS\` declares row(s) that DO parse as key names now. A `
562+
+ 'stale declaration goes on un-judging a row that could be judged — delete the '
563+
+ 'entry.',
564+
).toEqual([]);
565+
566+
expect(
567+
offenders,
568+
`os explain ${key}: table row(s) name key(s) ${bound.schema} does NOT declare `
569+
+ `(judged by the ${technique} technique). An author who copies such a row gets `
570+
+ 'one of two bad outcomes: a strict schema REFUSES the whole object by name, and '
571+
+ 'an OPEN one takes it, strips the key and reports success. The second is the trap '
572+
+ '`os explain query` shipped — a copied row, a silently stripped key, and a query '
573+
+ 'that ran unfiltered under an ordinary success. Correct the ROW to the schema\'s '
574+
+ 'own spelling (⛔ do not relax the schema to accept it).',
575+
).toEqual([]);
576+
});
577+
}
578+
579+
// The same completeness property the `classifies every entry` test asserts for
580+
// the catalog, asserted for this face: the two UNBOUND entries have no schema
581+
// to judge rows against, so the sweep above cannot reach them. Green over rows
582+
// nothing looked at is the defect this whole section is about, so their
583+
// emptiness is asserted rather than assumed.
584+
it('the two UNBOUND entries carry no table rows — no key name on this face is unswept', () => {
585+
for (const key of Object.keys(UNBOUND)) {
586+
expect(
587+
[...catalog[key].required, ...catalog[key].optional].map((row) => row.name),
588+
`os explain ${key} is a redirect with no schema to judge rows against, so a table `
589+
+ 'row here names keys nothing can check. Bind the entry to a schema, or keep the '
590+
+ 'tables empty and put the guidance in the description.',
591+
).toEqual([]);
592+
}
593+
});
594+
394595
it(`os explain workflow — ${UNBOUND.workflow}`, () => {
395596
expect('WorkflowSchema' in specSurface).toBe(false);
396597
expect(catalog.workflow.name).toContain('no standalone type');

0 commit comments

Comments
 (0)