Skip to content

Commit 69eda19

Browse files
committed
fix(cli): correct os explain query to where / orderBy, and assert key retention
The `query` catalog entry taught `filters` and `sort`. `BaseQuerySchema` is a plain `z.object`, so both were dropped silently: the example parsed clean and came back with no filter and no ordering, and the #14811 sweep was green on it because `safeParse(...).success` stays true through a silent strip. Both faces corrected to the schema's own spellings — `where` (one condition tree, not a `Filter[]`) and `orderBy` (sort nodes whose direction key is `order`, never `direction`). The sweep grows a key-retention assertion: an example must parse AND survive the parse with every declared key intact. Green across all nine bound entries today — eight refuse unknown keys outright and `query` was the only open top level — so it lands as a ratchet against a bound entry resolving to an open top level again, not as a patch over a large hole. Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 Co-authored-by: Claude <noreply@anthropic.com>
1 parent ae19f5e commit 69eda19

2 files changed

Lines changed: 111 additions & 5 deletions

File tree

packages/cli/src/commands/explain.ts

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,12 @@ export const SCHEMAS: Record<string, SchemaInfo> = {
238238
docsPath: 'ui/app',
239239
},
240240

241+
// `BaseQuerySchema` (`spec/src/data/query.zod.ts`) is a plain `z.object`, so an
242+
// unknown top-level key here is DROPPED rather than refused — the one open top
243+
// level among this catalog's bound entries (closing it is #4001's to schedule).
244+
// The two keys are therefore spelled exactly as the schema declares them:
245+
// `where` (not `filters`) and `orderBy` (not `sort`). Spelt wrong, a copied
246+
// example parses clean and returns unfiltered, unordered rows with no signal.
241247
query: {
242248
name: 'Query',
243249
description: 'Declarative data retrieval definition used for fetching and filtering records from objects.',
@@ -246,16 +252,20 @@ export const SCHEMAS: Record<string, SchemaInfo> = {
246252
],
247253
optional: [
248254
{ name: 'fields', type: 'string[]', description: 'Fields to select' },
249-
{ name: 'filters', type: 'Filter[]', description: 'Where conditions' },
250-
{ name: 'sort', type: 'SortConfig[]', description: 'Order by configuration' },
255+
{ 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.' },
256+
{ 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.' },
251257
{ name: 'limit', type: 'number', description: 'Maximum records to return' },
252258
{ name: 'offset', type: 'number', description: 'Pagination offset' },
253259
],
254260
example: `{
255261
object: 'project_task',
256262
fields: ['title', 'status', 'assigned_to'],
257-
filters: [{ field: 'status', operator: 'eq', value: 'open' }],
258-
sort: [{ field: 'created_at', order: 'desc' }],
263+
// \`where\` is ONE condition tree, not a \`Filter[]\`: key it by field — a bare value
264+
// is implicit equality, an object is a map of \`$\` operators — and combine with
265+
// \`$and\` / \`$or\` / \`$not\`.
266+
where: { status: 'open', priority: { $in: ['high', 'urgent'] } },
267+
// A sort node spells its direction \`order\`, never \`direction\`.
268+
orderBy: [{ field: 'created_at', order: 'desc' }],
259269
limit: 50,
260270
}`,
261271
related: ['object', 'field', 'view'],

packages/cli/test/commands.test.ts

Lines changed: 97 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ describe('os explain — every catalog entry swept against its spec schema (#148
210210
...specAutomation,
211211
};
212212

213-
type ParseResult = { success: boolean; error?: { issues: unknown[] } };
213+
type ParseResult = { success: boolean; data?: unknown; error?: { issues: unknown[] } };
214214
type ZodLike = { safeParse: (value: unknown) => ParseResult };
215215

216216
// 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
295295
}
296296
}
297297

298+
// ── Key RETENTION: an example must SURVIVE its parse, not merely pass it ────
299+
//
300+
// The sweep above asserts `safeParse(...).success === true` — and that stays
301+
// green over a key the schema SILENTLY STRIPS, because a plain `z.object`
302+
// drops what it does not declare and still reports success. The `query` entry
303+
// shipped teaching `filters` and `sort`, neither of them a `QuerySchema` key,
304+
// and every run of the sweep above was green on it (#16925). "Parses" is
305+
// therefore not the property worth asserting on its own; "parses AND comes
306+
// back whole" is, and only the second one can see this failure mode.
307+
//
308+
// ⭐ This is a RATCHET, not a patch over a large hole. All nine bound entries
309+
// were read back to spec on the day it landed: eight refuse an unknown key
310+
// outright — seven `strictObject`, plus `object`, whose docblock states the
311+
// "No silent strip (ADR-0032 / #1535)" contract explicitly — and `query` alone
312+
// had an open top level, deliberately so and already owned (`query.zod.ts`:
313+
// "Deliberately NOT taken here: `BaseQuerySchema`'s own top level stays
314+
// non-strict. That is #4001's to schedule."). So it is green across the whole
315+
// catalog the day it lands. What it defends is the day a bound entry resolves
316+
// to an open top level again: the direction of travel is *closing*, and this
317+
// is what notices if that reverses — with the entry named, instead of a green
318+
// sweep over a silently emptied example.
319+
//
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.
329+
//
330+
// The walk is deliberately conservative — it reports a key present in the
331+
// INPUT and absent from the OUTPUT, and nothing else. Keys a schema ADDS
332+
// (defaults) are not drift; a value a schema TRANSFORMS to a non-object is not
333+
// a dropped key, so the walk stops rather than guessing. Arrays are matched
334+
// positionally, which is what every schema in this catalog does today.
335+
const isWalkable = (value: unknown): value is Record<string, unknown> =>
336+
typeof value === 'object' && value !== null && !(value instanceof Date);
337+
338+
const droppedKeys = (
339+
input: unknown,
340+
output: unknown,
341+
path: string[] = [],
342+
out: string[] = [],
343+
): string[] => {
344+
if (Array.isArray(input)) {
345+
if (!Array.isArray(output)) return out;
346+
input.forEach((item, i) => droppedKeys(item, output[i], [...path, String(i)], out));
347+
return out;
348+
}
349+
if (!isWalkable(input) || Array.isArray(output) || !isWalkable(output)) return out;
350+
for (const key of Object.keys(input)) {
351+
if (!Object.prototype.hasOwnProperty.call(output, key)) out.push([...path, key].join('.'));
352+
else droppedKeys(input[key], output[key], [...path, key], out);
353+
}
354+
return out;
355+
};
356+
357+
for (const [key, bound] of Object.entries(BOUND)) {
358+
if (bound.card === undefined) {
359+
it(`os explain ${key} — example survives ${bound.schema} with every declared key intact`, () => {
360+
const schema = specSurface[bound.schema] as ZodLike;
361+
const example = evaluate(key);
362+
const result = schema.safeParse(example);
363+
// Retention is only a question about a parse that succeeded — stated,
364+
// so a failure here reads as "the parse broke" and not as a drop.
365+
expect(
366+
result.success,
367+
`os explain ${key}: its example must parse before retention can be judged`,
368+
).toBe(true);
369+
expect(
370+
droppedKeys(example, result.data),
371+
`os explain ${key}: ${bound.schema} SILENTLY DROPPED key(s) its example declares. `
372+
+ 'The example teaches keys the schema does not have, so an author who copies it '
373+
+ 'gets a parse that succeeds and a value with those keys gone — no error, no '
374+
+ 'warning. Correct the example to the schema\'s own spellings (⛔ do not relax '
375+
+ 'the schema to accept them). The `parses` assertion above cannot see this: a '
376+
+ 'non-strict object reports success and strips.',
377+
).toEqual([]);
378+
});
379+
} else {
380+
// Asserted, never skipped: an entry whose example does not parse cannot be
381+
// judged for retention, and that reason is a property of this file rather
382+
// than an omission the reader has to notice.
383+
it(
384+
`os explain ${key} — retention NOT judged: its example does not parse yet `
385+
+ `(known-broken, filed as #${bound.card})`,
386+
() => {
387+
const schema = specSurface[bound.schema] as ZodLike;
388+
expect(schema.safeParse(evaluate(key)).success).toBe(false);
389+
},
390+
);
391+
}
392+
}
393+
298394
it(`os explain workflow — ${UNBOUND.workflow}`, () => {
299395
expect('WorkflowSchema' in specSurface).toBe(false);
300396
expect(catalog.workflow.name).toContain('no standalone type');

0 commit comments

Comments
 (0)