diff --git a/.changeset/quiet-pugs-tickle.md b/.changeset/quiet-pugs-tickle.md new file mode 100644 index 0000000000..c523bf123b --- /dev/null +++ b/.changeset/quiet-pugs-tickle.md @@ -0,0 +1,24 @@ +--- +'@objectstack/cli': patch +--- + +`os explain query` now teaches the two keys `QuerySchema` actually declares. + +The entry's example and its two optional-table rows named `filters` and `sort`. +Neither is a key of `BaseQuerySchema`, which is a plain `z.object` — so both +were dropped silently: an author who copied the example got a query that parsed +clean and ran with no filter and no ordering, with nothing in the output saying +so. + +Both faces now read the schema's own spellings: + +- `where` — one condition **tree**, not a `Filter[]`. A field-keyed entry is a + condition on that field (a bare value is implicit equality, an object is a map + of `$` operators), and `$and` / `$or` / `$not` combine conditions. +- `orderBy` — sort nodes, each `{ field, order }`. The direction key is spelled + `order`; `direction` is rejected by name. + +No schema changed, and no accept set moved: the correction is to the catalog +entry only. The `os explain` catalog sweep also gains a key-retention assertion +— an example must parse **and** come back with every key it declares — so the +next entry whose schema strips a key is named instead of passing. diff --git a/packages/cli/src/commands/explain.ts b/packages/cli/src/commands/explain.ts index dde84bcebf..09ab4c720e 100644 --- a/packages/cli/src/commands/explain.ts +++ b/packages/cli/src/commands/explain.ts @@ -238,6 +238,12 @@ export const SCHEMAS: Record = { docsPath: 'ui/app', }, + // `BaseQuerySchema` (`spec/src/data/query.zod.ts`) is a plain `z.object`, so an + // unknown top-level key here is DROPPED rather than refused — the one open top + // level among this catalog's bound entries (closing it is #4001's to schedule). + // The two keys are therefore spelled exactly as the schema declares them: + // `where` (not `filters`) and `orderBy` (not `sort`). Spelt wrong, a copied + // example parses clean and returns unfiltered, unordered rows with no signal. query: { name: 'Query', description: 'Declarative data retrieval definition used for fetching and filtering records from objects.', @@ -246,16 +252,20 @@ export const SCHEMAS: Record = { ], optional: [ { name: 'fields', type: 'string[]', description: 'Fields to select' }, - { name: 'filters', type: 'Filter[]', description: 'Where conditions' }, - { name: 'sort', type: 'SortConfig[]', description: 'Order by configuration' }, + { name: 'where', type: 'FilterCondition', description: 'The condition TREE the query filters by — one object, never a `Filter[]`. A field-keyed entry is a condition on that field (a bare value is implicit equality, an object is a map of `$` operators such as `$in` / `$gte` / `$contains`), and `$and` / `$or` / `$not` combine conditions.' }, + { name: 'orderBy', type: 'SortNode[]', description: 'Sort nodes, each `{ field, order }` with `order` one of `asc` / `desc`. The direction key is spelled `order` — `direction` is rejected by name, because when it was merely dropped the sort fell back to `asc` and, with `limit`, returned a different set of rows under an ordinary success.' }, { name: 'limit', type: 'number', description: 'Maximum records to return' }, { name: 'offset', type: 'number', description: 'Pagination offset' }, ], example: `{ object: 'project_task', fields: ['title', 'status', 'assigned_to'], - filters: [{ field: 'status', operator: 'eq', value: 'open' }], - sort: [{ field: 'created_at', order: 'desc' }], + // \`where\` is ONE condition tree, not a \`Filter[]\`: key it by field — a bare value + // is implicit equality, an object is a map of \`$\` operators — and combine with + // \`$and\` / \`$or\` / \`$not\`. + where: { status: 'open', priority: { $in: ['high', 'urgent'] } }, + // A sort node spells its direction \`order\`, never \`direction\`. + orderBy: [{ field: 'created_at', order: 'desc' }], limit: 50, }`, related: ['object', 'field', 'view'], diff --git a/packages/cli/test/commands.test.ts b/packages/cli/test/commands.test.ts index 57af6b8bfd..d793d40105 100644 --- a/packages/cli/test/commands.test.ts +++ b/packages/cli/test/commands.test.ts @@ -210,7 +210,7 @@ describe('os explain — every catalog entry swept against its spec schema (#148 ...specAutomation, }; - type ParseResult = { success: boolean; error?: { issues: unknown[] } }; + type ParseResult = { success: boolean; data?: unknown; error?: { issues: unknown[] } }; type ZodLike = { safeParse: (value: unknown) => ParseResult }; // The catalog stores examples as authored source, so evaluate the literal — @@ -295,6 +295,102 @@ describe('os explain — every catalog entry swept against its spec schema (#148 } } + // ── Key RETENTION: an example must SURVIVE its parse, not merely pass it ──── + // + // The sweep above asserts `safeParse(...).success === true` — and that stays + // green over a key the schema SILENTLY STRIPS, because a plain `z.object` + // drops what it does not declare and still reports success. The `query` entry + // shipped teaching `filters` and `sort`, neither of them a `QuerySchema` key, + // and every run of the sweep above was green on it (#16925). "Parses" is + // therefore not the property worth asserting on its own; "parses AND comes + // back whole" is, and only the second one can see this failure mode. + // + // ⭐ This is a RATCHET, not a patch over a large hole. All nine bound entries + // were read back to spec on the day it landed: eight refuse an unknown key + // outright — seven `strictObject`, plus `object`, whose docblock states the + // "No silent strip (ADR-0032 / #1535)" contract explicitly — and `query` alone + // had an open top level, deliberately so and already owned (`query.zod.ts`: + // "Deliberately NOT taken here: `BaseQuerySchema`'s own top level stays + // non-strict. That is #4001's to schedule."). So it is green across the whole + // catalog the day it lands. What it defends is the day a bound entry resolves + // to an open top level again: the direction of travel is *closing*, and this + // 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. + // + // 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 + // (defaults) are not drift; a value a schema TRANSFORMS to a non-object is not + // a dropped key, so the walk stops rather than guessing. Arrays are matched + // positionally, which is what every schema in this catalog does today. + const isWalkable = (value: unknown): value is Record => + typeof value === 'object' && value !== null && !(value instanceof Date); + + const droppedKeys = ( + input: unknown, + output: unknown, + path: string[] = [], + out: string[] = [], + ): string[] => { + if (Array.isArray(input)) { + if (!Array.isArray(output)) return out; + input.forEach((item, i) => droppedKeys(item, output[i], [...path, String(i)], out)); + return out; + } + if (!isWalkable(input) || Array.isArray(output) || !isWalkable(output)) return out; + for (const key of Object.keys(input)) { + if (!Object.prototype.hasOwnProperty.call(output, key)) out.push([...path, key].join('.')); + else droppedKeys(input[key], output[key], [...path, key], out); + } + return out; + }; + + for (const [key, bound] of Object.entries(BOUND)) { + if (bound.card === undefined) { + it(`os explain ${key} — example survives ${bound.schema} with every declared key intact`, () => { + const schema = specSurface[bound.schema] as ZodLike; + const example = evaluate(key); + const result = schema.safeParse(example); + // Retention is only a question about a parse that succeeded — stated, + // so a failure here reads as "the parse broke" and not as a drop. + expect( + result.success, + `os explain ${key}: its example must parse before retention can be judged`, + ).toBe(true); + expect( + droppedKeys(example, result.data), + `os explain ${key}: ${bound.schema} SILENTLY DROPPED key(s) its example declares. ` + + 'The example teaches keys the schema does not have, so an author who copies it ' + + 'gets a parse that succeeds and a value with those keys gone — no error, no ' + + 'warning. Correct the example to the schema\'s own spellings (⛔ do not relax ' + + 'the schema to accept them). The `parses` assertion above cannot see this: a ' + + 'non-strict object reports success and strips.', + ).toEqual([]); + }); + } else { + // Asserted, never skipped: an entry whose example does not parse cannot be + // judged for retention, and that reason is a property of this file rather + // than an omission the reader has to notice. + it( + `os explain ${key} — retention NOT judged: its example does not parse yet ` + + `(known-broken, filed as #${bound.card})`, + () => { + const schema = specSurface[bound.schema] as ZodLike; + expect(schema.safeParse(evaluate(key)).success).toBe(false); + }, + ); + } + } + it(`os explain workflow — ${UNBOUND.workflow}`, () => { expect('WorkflowSchema' in specSurface).toBe(false); expect(catalog.workflow.name).toContain('no standalone type');