From 6e5526923d272359808100f36ff050675f892484 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 15:33:02 +0000 Subject: [PATCH 1/2] feat(spec,metadata-protocol)!: reject a sort node spelling its direction `direction` (#4721) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SortNodeSchema` was a plain `z.object`, so zod's `.strip` default applied: SortNodeSchema.parse({ field: 'updated_at', direction: 'desc' }) → { field: 'updated_at', order: 'asc' } The foreign key was discarded, `order` fell back to `asc`, and the sort ran the OPPOSITE way under an ordinary success — with `limit`, a different set of rows, with no signal anywhere in the response. `direction` is not a typo: it is `IReportService.orderBy`'s live vocabulary, which plugin-auth's objectql adapter already translates by hand. A translation known to be necessary and enforced nowhere is the ADR-0049 shape. Both doors onto that shape are closed here, in one change: - `SortNodeSchema` (spec/src/data/query.zod.ts) → `strictObject` with `aliases: { direction: 'order' }`, so the rejection carries the translation. Edit distance can never bridge `direction` → `order`, so a bare "unrecognized key" would leave the caller where the silent strip did. - `normalizeSortNodes` (metadata-protocol/src/protocol.ts) — the ingress every external `orderBy` funnels through — refuses `{ field, direction }` with 400 INVALID_SORT naming `order` and quoting the corrected node. Closing only the schema would repeat the #1535/#4522 door asymmetry: `SortNodeSchema.parse` is reachable by three paths the REST normalizer never sees, and the normalizer runs ahead of any QueryAST parse. Deliberately NOT in scope: `QuerySchema`'s top level stays non-strict (`QuerySchema.safeParse({object:'sales', nonsenseKey:1}).success === true`) — tracked in the #4001 campaign map for its own batch. The `{field: direction}` map form is untouched: there `direction` is an ordinary column name, and refusing it would be the mirror-image bug. Strictness ledger: `query.zod.ts` keeps its `open` class for the four dialect sites; `SortNodeSchema` is carved out as authorable (4 strip of 5), which also resolves the recorded classification conflict — the FILE was the wrong unit. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ehu85kbvMcrNTUJjwxvLJ9 --- .changeset/sort-node-direction-rejected.md | 65 +++++++++++ .../2026-07-unknown-key-strictness-ledger.md | 55 +++++---- .../src/protocol.orderby-vocabulary.test.ts | 110 +++++++++++++++++- packages/metadata-protocol/src/protocol.ts | 61 ++++++++++ .../src/query-expression-conformance.test.ts | 36 ++++++ packages/spec/src/data/query.test.ts | 80 +++++++++++++ packages/spec/src/data/query.zod.ts | 59 ++++++++-- 7 files changed, 437 insertions(+), 29 deletions(-) create mode 100644 .changeset/sort-node-direction-rejected.md diff --git a/.changeset/sort-node-direction-rejected.md b/.changeset/sort-node-direction-rejected.md new file mode 100644 index 0000000000..6b478cdc35 --- /dev/null +++ b/.changeset/sort-node-direction-rejected.md @@ -0,0 +1,65 @@ +--- +"@objectstack/spec": major +"@objectstack/metadata-protocol": major +--- + +feat(spec,metadata-protocol)!: a sort node spelling its direction `direction` is a 400, not a silently reversed page (#4721) + +**FROM → TO:** `orderBy: [{ field: 'updated_at', direction: 'desc' }]` → +`orderBy: [{ field: 'updated_at', order: 'desc' }]`. One word. If you are on the +`{field, direction}` shape because you moved code over from +`IReportService.orderBy`, that contract is unchanged — it is `orderBy` on the +QueryAST / `EngineQueryOptions` axis that has always been `{field, order}`. + +## What was wrong + +`SortNodeSchema` was a plain `z.object`, so zod's default `.strip` applied. +Measured on `main` before this change: + +``` +SortNodeSchema.parse({ field: 'updated_at', direction: 'desc' }) + → { field: 'updated_at', order: 'asc' } +``` + +`direction` was discarded and `order` fell back to its `asc` default. The sort +therefore ran in the **opposite** direction and the request succeeded. Paired +with `limit` — which is how a caller asks for "the latest N" — that is not a +reordered page but a **different set of rows**, returned under an ordinary 200 +with nothing in the response to distinguish it from the answer that was asked +for. + +`direction` is not a typo. It is the live vocabulary of a neighbouring contract, +`IReportService.orderBy` (`@objectstack/spec/contracts`), and +`plugin-auth/objectql-adapter.ts` already translates between the two by hand — a +translation known to be necessary and enforced nowhere, which is the ADR-0049 +shape. + +## What changed + +Both doors onto that shape, in one change: + +1. **`SortNodeSchema`** (`spec/src/data/query.zod.ts`) is now `strictObject` + with `aliases: { direction: 'order' }`. An unknown key is rejected, and + `direction` specifically gets the translation in the error message — edit + distance can never bridge `direction` → `order`, so a bare "unrecognized key" + would leave the caller exactly where the silent strip did. +2. **`normalizeSortNodes`** (`metadata-protocol/src/protocol.ts`), the ingress + every REST/RPC `orderBy` funnels through, refuses `{ field, direction }` with + `400 INVALID_SORT` naming `order` and quoting the corrected node. Closing only + the schema would repeat the door asymmetry of #1535/#4522: `SortNodeSchema` is + reachable by three paths the REST normalizer never sees. + +| `orderBy` you send | Before | After | +|:--|:--|:--| +| `[{ field: 'x', order: 'desc' }]` | descending | unchanged — descending | +| `[{ field: 'x', direction: 'desc' }]` | **200, ascending** | `400 INVALID_SORT`, message names `order` | +| `[{ field: 'x', order: 'desc', direction: 'asc' }]` | 200, descending | `400 INVALID_SORT` | +| `'-x'` / `['-x']` / `{ x: 'desc' }` | descending | unchanged | +| `{ direction: 'desc' }` (the `{field: direction}` map) | sorts by column `direction` | unchanged — a column may legitimately be called `direction` | + +Scope is deliberately narrow: **`QuerySchema`'s top level is untouched** and +still accepts undeclared keys (`QuerySchema.safeParse({ object: 'sales', +nonsenseKey: 1 }).success === true`). That is tracked in the #4001 campaign map +for its own batch, not smuggled in here. + +Related: #4674, #4720, #4363, #4371, #4001, ADR-0049. diff --git a/docs/audits/2026-07-unknown-key-strictness-ledger.md b/docs/audits/2026-07-unknown-key-strictness-ledger.md index 0151f81526..84fd6e0f98 100644 --- a/docs/audits/2026-07-unknown-key-strictness-ledger.md +++ b/docs/audits/2026-07-unknown-key-strictness-ledger.md @@ -510,7 +510,8 @@ not verdicts). | `external-lookup.zod.ts` | 12 | mixed (p) | authored config + wire results | | `seed-loader.zod.ts` | 12 | mixed (p) | seed file shapes are authored; loader state is runtime | | `field.zod.ts` | 11 | authorable | partially strict | -| `filter.zod.ts` / `query.zod.ts` | 11+5 | open | query dialect — user data flows through; validated semantically elsewhere. `query.zod.ts` dropped one site in #4196: `FieldNodeSchema`'s nested-select object form was declared-but-inert and narrowed to `z.string()`, so the union's second member is gone. Four more left in #4286 with the `joins`/`windowFunctions` removals: `JoinNodeBaseSchema`, `WindowFunctionNodeSchema`, and `WindowSpecSchema`'s two blocks (outer + `frame`) were deleted with their clusters. Class unchanged | +| `filter.zod.ts` | 11 | open | query dialect — user data flows through the predicate values; validated semantically elsewhere | +| `query.zod.ts` | 5 | open, **except `SortNodeSchema` → authorable** | Blanket `open` was the imprecise verdict here, not the strictness. Four sites are the dialect proper (`BaseQuerySchema`, `AggregationNodeSchema`, `FullTextSearchSchema`, `GroupByNodeSchema`'s object arm) and keep the class. `SortNodeSchema` is not dialect: a closed two-key tuple `{field, order}` with **no user-data face at all** — so #4721 carved it out and it is **strict as of #4721** (`strictObject` + `aliases: { direction: 'order' }`). What that bought, measured on `main` first: `SortNodeSchema.parse({field, direction:'desc'})` → `{field, order:'asc'}` — the sort ran the OTHER WAY, and with `limit` that is a different set of rows under an ordinary 200. Per the 11:41Z ruling on #4721 this is a NEW door, not the completion of #4371's: that check is a hand-written top-level allowlist in `objectql/src/engine.ts` (`rejectUnknownEngineOptions`) that never recurses into `orderBy[]`, and `QuerySchema` itself is **not** strict (probe: `QuerySchema.safeParse({object:'sales', nonsenseKey:1}).success === true`) — top-level strictness is #4001's, tracked separately. Site history: one site dropped in #4196 (`FieldNodeSchema`'s nested-select object form narrowed to `z.string()`); four more in #4286 with the `joins`/`windowFunctions` removals (`JoinNodeBaseSchema`, `WindowFunctionNodeSchema`, `WindowSpecSchema`'s outer + `frame`) | | `driver-nosql.zod.ts` / `driver.zod.ts` / `driver-sql.zod.ts` | 10+9+2 | wire | driver capability contracts | | `datasource.zod.ts` | 6 | authorable | **strict as of #4001 data step** — all 6: `DatasourceSchema` (+ `pool` / `ssl`), `ExternalDatasourceSettingsSchema` (+ `validation`), `DriverDefinitionSchema`. **#4583 B/C dropped two more sites**: the `healthCheck` and `retryPolicy` blocks are gone — nothing scheduled a probe and nothing retried, so their strictness was validating a shape no code consumed. `config` stays `z.record` **at this level** by construction (per-driver shapes), but is no longer unchecked: **#4410** made `DatasourceSchema`'s refinement parse it against the contract for the declared driver (`driver/config-registry.zod.ts`), so the openness here is a shape this level cannot express rather than the absence of one. This row used to add "the driver's own `configSchema` validates them", which was false until #4410 landed the parse site it names. #4410 extended the same parse to each `readReplicas` entry; **#4468 retired that key** — no driver ever opened a replica connection and no query path splits reads from writes, so the entries were being checked against a contract nothing would apply. Strictness makes a dropped key loud; it cannot make a slot live, and a *precisely validated* dead slot is the more convincing lie | **#4583 dropped the ninth site**: `DatasourceCapabilities` is gone — eleven flags no code read, on a block whose strictness was the clearest case of this row's own closing sentence. `readOnly` in particular was *precisely validated* and completely inert, and had been relocated twice (#4410, #4465) toward somewhere it might be enforced; the shipped CRM example called a datasource a read replica on the strength of it while writes went through. Class unchanged | `driver/memory.zod.ts` / `driver/mongo.zod.ts` / `driver/postgres.zod.ts` | 6+1+1 | authorable | The per-driver shapes for the `config` slot — what an author actually writes under `datasource.config` (`host`, `port`, `filename`). **Undeclared here until the coverage walk went recursive** (see below): a subdirectory was invisible to the gate, so these sites sat outside the map while the map reported full coverage. **Strict as of #4410**, which is also what unblocked them: this row previously read "strictness here would enforce nothing" because nothing parsed `datasource.config` against these schemas and both `*DriverSpec.configSchema` literals were `{}`. Now `DatasourceSchema` parses `config` against them, and the same schemas project onto `configSchema` and onto the Studio connection form. (#4410 also ran the parse over each `readReplicas` entry; #4468 retired that key outright — see the row above.) `postgres.zod.ts` drops a site: its `ssl` was a `boolean | {ca, cert, key, …}` union, and the object arm is gone — certificates now live in the datasource-level `ssl` block (declared, strict, and until #4410 read by nobody), leaving `config.ssl` as the on/off shorthand. That narrowing is forced by the same projection: the Studio form renders anything that is not boolean/enum/number as a TEXT INPUT, so a union here would have produced a wizard whose every `ssl` value the new gate rejects. `memory.zod.ts` keeps 6 but loses two KEYS — `indexes` / `maxRecordsPerObject`, which `InMemoryDriverConfig` has no field for, removed under ADR-0049 rather than blessed by the new gate | @@ -654,7 +655,7 @@ is complete and so nobody re-triages them from scratch next batch. `options`, which is `open`. Of those 123, `app.zod.ts`'s single site is held pending the finding-16 `.extend()` check rather than counted as ready. -#### `data/` — 121 strip of 162 +#### `data/` — 120 strip of 162 | File | Strip | Sites | Class | Batch | |---|---|---|---|---| @@ -668,14 +669,14 @@ the finding-16 `.extend()` check rather than counted as ready. | `analytics.zod.ts` | 8 | 8 | mixed (p) | `Metric` / `Dimension` / `Cube` / `AnalyticsQuery` — cube definitions are authored; needs a per-schema read | | `document.zod.ts` | 8 | 8 | wire (p) | `DocumentTemplate` / `ESignatureConfig` read authorable on their face — the `(p)` is unresolved, verify before scheduling either way | | `driver/memory.zod.ts` | 5 | 6 | authorable | The persistence-adapter union under `datasource.config`; `datasource.config` HAS been parsed against these since #4410, so strictness here now binds | -| `query.zod.ts` | 5 | 5 | open | ⚠️ **classification conflict — see #4721.** The row calls the query dialect `open`; #4721 asks for `SortNodeSchema.strict()`. Both cannot be right. Resolve the class before writing code | +| `query.zod.ts` | 4 | 5 | open | ~~⚠️ classification conflict — see #4721~~ **RESOLVED (11:41Z ruling, closed by #4721).** The conflict was real and the answer was that per-FILE classification was the imprecise instrument: `SortNodeSchema` was carved out as `authorable` and closed (`strictObject` + `aliases: { direction: 'order' }`), the other 4 sites keep `open`. Those 4 are the dialect proper — `BaseQuerySchema`, `AggregationNodeSchema`, `FullTextSearchSchema`, `GroupByNodeSchema`'s object arm — and `BaseQuerySchema`'s own top-level strictness is #4001's to schedule, deliberately **not** taken by #4721 | | `external-catalog.zod.ts` | 4 | 4 | wire (p) | **out of scope** | | `hook.zod.ts` | 4 | 6 | wire | **out of scope** — `HookContextSchema` + `.session`/`.provenance`/`.user` are the runtime shape handed to a handler; verified in the data step | | `field.zod.ts` | 3 | 11 | authorable | `LocationCoordinates` / `CurrencyValue` / `Address` — field VALUE shapes, not field config; check whether they are record data (→ open) before closing | | `driver-sql.zod.ts` | 2 | 2 | wire | **out of scope** | | `field-value.zod.ts` | 1 | 2 | mixed (p) | `LocationValueSchema` — record data, very likely **open**; its sibling `FileValueSchema` is already `z.looseObject` | -**Authorable strip in `data/`: ~22 firm** (`object` 14 + `driver/memory` 5 + `field` 3), **plus ~33 needing a per-schema verdict** (`external-lookup` 12, `seed-loader` 12, `analytics` 8, `field-value` 1). 66 are wire/open and out of the ruling's forced scope. +**Authorable strip in `data/`: ~22 firm** (`object` 14 + `driver/memory` 5 + `field` 3), **plus ~33 needing a per-schema verdict** (`external-lookup` 12, `seed-loader` 12, `analytics` 8, `field-value` 1). 65 are wire/open and out of the ruling's forced scope — 66 until #4721 closed `query.zod.ts`'s `SortNodeSchema`, which is the one row in this directory where the per-schema read moved a site OUT of `open` rather than confirming it. #### `security/` — 13 strip of 20 @@ -773,25 +774,39 @@ is the confirmation the campaign's own progress log was missing. estimated — read it, do not re-derive it, and do not plan off `strictObject(` occurrence counts (finding 19 explains what that undercounts). - Two things in that map need a decision before any code is written, and both - are classification questions rather than implementation ones: - - - **`data/query.zod.ts` is classed `open`, and #4721 asks for - `SortNodeSchema.strict()`.** Both cannot be right. Measured, so the decision - is made against facts rather than recollection: `SortNodeSchema.parse({ - field, direction: 'desc' })` returns `{ field, order: 'asc' }` — the wrong - rows, with no signal — and #4721's premise that the top level already - rejects unknown option keys is true but of a **different mechanism**: - #4371's check is a hand-written allowlist in `objectql/src/engine.ts` - (`rejectUnknownEngineOptions`) that iterates `Object.entries(bag)` at the - top level only. It is a bespoke guard at one door, which is this campaign's - finding 17 exactly, so "same invariant, one level down" is not available as - a justification — closing the sort node is a *new* door, not the completion - of an existing one. + Two things in that map needed a decision before any code was written, and + both were classification questions rather than implementation ones. The first + is now settled: + + - ~~**`data/query.zod.ts` is classed `open`, and #4721 asks for + `SortNodeSchema.strict()`.**~~ **SETTLED (2026-08-03 11:41Z ruling; closed + by #4721.)** Measured, so the decision was made against facts rather than + recollection: `SortNodeSchema.parse({ field, direction: 'desc' })` returns + `{ field, order: 'asc' }` — the wrong rows, with no signal — while #4721's + premise that the top level already rejects unknown option keys turned out + to be about a **different mechanism** and, on re-measurement, not even true + of the schema: #4371's check is a hand-written allowlist in + `objectql/src/engine.ts` (`rejectUnknownEngineOptions`) that iterates + `Object.entries(bag)` at the top level only, and `QuerySchema` itself is not + strict (`QuerySchema.safeParse({object:'sales', nonsenseKey:1}).success === + true`). That is finding 17 exactly — a bespoke guard at one door — so "same + invariant, one level down" was **not** available as a justification, and the + ruling does not use it: closing the sort node is a **new** door. + + **The answer was that the FILE was the wrong unit.** `open` was awarded to + the query dialect because user data flows through predicate values; + `SortNodeSchema` is a closed two-key tuple with no user-data face, so it was + re-classed `authorable` and closed while the other four sites kept `open`. + The generalisable part: when a blanket per-file class collides with a + per-schema finding, **suspect the blanket first** — this ledger classifies + sites, and a file is only a convenient bag of them. Both doors were closed + in the same change (`SortNodeSchema` + `normalizeSortNodes` in + `metadata-protocol`), per finding 6's asymmetry. + - **`ui/app.zod.ts`'s `BaseNavItemSchema`** is the base that the strict discriminated-union members `.extend()`. Finding 16 is the warning: closing a base closes every extension of it, including any that is deliberately a - wire shape. + wire shape. **Still open.** Done in the registered-types batch: `strictObject` (`shared/strict-object.ts`) replaced the four-part wiring recipe, and `seed` + `doc` became the first two diff --git a/packages/metadata-protocol/src/protocol.orderby-vocabulary.test.ts b/packages/metadata-protocol/src/protocol.orderby-vocabulary.test.ts index c89d7ea2ee..668ddd30dd 100644 --- a/packages/metadata-protocol/src/protocol.orderby-vocabulary.test.ts +++ b/packages/metadata-protocol/src/protocol.orderby-vocabulary.test.ts @@ -20,8 +20,22 @@ // Nothing caught this because both sites erased their types (`} as any)` and // `const opts: any`), the protocol's `INVALID_SORT` normalizer does not run on // calls the protocol makes to `this.engine.find` directly, and that normalizer -// rejects bad VALUES rather than unknown KEYS — the schema is not `.strict()`, -// so `direction` was dropped rather than flagged. +// rejected bad VALUES rather than unknown KEYS. +// +// ── #4721: the strip-era half of that last clause is RETIRED ───────────────── +// +// The sentence above used to end "— the schema is not `.strict()`, so +// `direction` was dropped rather than flagged", and that was a pin on the SILENT +// behaviour: a record of the hole, not a defence of it. #4720 (the two internal +// sites above) fixed the callers `tsc` covers; #4721 closes the hole itself, on +// both doors at once, so the pin is replaced by the rejection tests at the +// bottom of this file. Measured on `main` before the change: +// +// SortNodeSchema.parse({ field: 'updated_at', direction: 'desc' }) +// → { field: 'updated_at', order: 'asc' } +// +// EXTERNAL callers (REST / RPC `orderBy`) were never covered by #4720 at all — +// they have no `tsc` — which is the gap #4721 exists for. import { describe, it, expect, vi } from 'vitest'; import { ObjectStackProtocolImplementation } from './protocol.js'; @@ -130,3 +144,95 @@ describe('searchAll sorts newest-first (#4674)', () => { expect(sort[0]).not.toHaveProperty('direction'); }); }); + +/** + * #4721 — the same vocabulary mistake made by an EXTERNAL caller. + * + * #4720 restored the types on the two internal sites above, which is the right + * cure for callers `tsc` can see. It does nothing for the other row of the + * table: a REST/RPC client putting `direction` in `orderBy` has no type checker, + * and got back an ordinary 200 over rows sorted the OTHER WAY — with `limit`, + * a different set of rows entirely, with no signal in the response. + * + * The rejection lives in `normalizeSortNodes`, which every ingress funnels + * through. Its schema-side twin is `SortNodeSchema`'s + * `aliases: { direction: 'order' }` (`spec/src/data/query.zod.ts`, pinned in + * `spec/src/data/query.test.ts`) — both closed in one change on purpose: + * a guard on one door only is the asymmetry #1535 shipped and #4522 came back + * for, and `SortNodeSchema.parse` is reachable by three paths the REST + * normalizer never sees. + */ +describe('an external `orderBy` spelling the direction `direction` is refused (#4721)', () => { + const CONTACT_ROWS = [ + { id: 'r1', name: 'A', updated_at: '2024-01-01T00:00:00.000Z' }, + { id: 'r2', name: 'B', updated_at: '2024-02-01T00:00:00.000Z' }, + ]; + const OBJECT = { + name: 'contact', + fields: { + name: { name: 'name', type: 'text' }, + updated_at: { name: 'updated_at', type: 'datetime' }, + // A real column literally called `direction`, so the map-form case + // below is not hypothetical. + direction: { name: 'direction', type: 'text' }, + }, + }; + + function makeProtocol() { + const find = makeFind({ contact: CONTACT_ROWS }); + const engine = { registry: { getObject: (n: string) => (n === 'contact' ? OBJECT : undefined) }, find }; + return { p: new ObjectStackProtocolImplementation(engine as any), find }; + } + + it.each([ + ['array element', { orderBy: [{ field: 'updated_at', direction: 'desc' }] }], + ['unwrapped single node', { orderBy: { field: 'updated_at', direction: 'desc' } }], + ['OData spelling', { $orderby: [{ field: 'updated_at', direction: 'desc' }] }], + ])('is a 400 INVALID_SORT, not an ascending 200 — %s', async (_label, query) => { + const { p, find } = makeProtocol(); + await expect(p.findData({ object: 'contact', query })).rejects.toMatchObject({ + status: 400, + code: 'INVALID_SORT', + }); + // The point of refusing at the ingress: the engine is never reached, so + // there is no "successful" wrong-direction page to mistake for an answer. + expect(find).not.toHaveBeenCalled(); + }); + + it('the rejection names `order` — a refusal without the translation is no better', async () => { + const { p } = makeProtocol(); + // `direction` is not a typo of `order`; edit distance can never bridge + // them, so a generic "unrecognized key" leaves the caller exactly where + // the silent strip did. + await expect(p.findData({ + object: 'contact', query: { orderBy: [{ field: 'updated_at', direction: 'desc' }] }, + })).rejects.toThrow(/`\{ field: 'updated_at', order: 'desc' \}`/); + }); + + it('rejects it even when `order` is also present', async () => { + const { p } = makeProtocol(); + await expect(p.findData({ + object: 'contact', + query: { orderBy: [{ field: 'updated_at', order: 'desc', direction: 'asc' }] }, + })).rejects.toMatchObject({ code: 'INVALID_SORT' }); + }); + + it('leaves a COLUMN named `direction` alone in the map form', async () => { + // `{ direction: 'desc' }` is the `{field: direction}` map — "sort by the + // `direction` column" — not a sort node with the wrong key. Refusing it + // would be the mirror-image bug: a legitimate query rejected because a + // column shares a name with a foreign vocabulary. + const { p, find } = makeProtocol(); + await p.findData({ object: 'contact', query: { orderBy: { direction: 'desc' } } }); + expect(optionsFrom(find).orderBy).toEqual([{ field: 'direction', order: 'desc' }]); + }); + + it('control — the canonical spelling still reaches the engine and sorts', async () => { + const { p, find } = makeProtocol(); + const r: any = await p.findData({ + object: 'contact', query: { orderBy: [{ field: 'updated_at', order: 'desc' }], limit: 1 }, + }); + expect(optionsFrom(find).orderBy).toEqual([{ field: 'updated_at', order: 'desc' }]); + expect(r.records.map((x: any) => x.id)).toEqual(['r2']); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index c7c9a1609a..1068ed3d1d 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1082,6 +1082,48 @@ function invalidSortError( return err; } +/** + * [#4721] A sort node that spells its direction `direction` instead of `order`. + * + * This is the one unknown key on this axis that has a KNOWN right answer, so it + * gets a rejection that carries the translation rather than a generic refusal. + * `direction` is not a typo — it is the live vocabulary of a neighbouring + * contract (`IReportService.orderBy`, `spec/src/contracts/report-service.ts`), + * which `plugin-auth/objectql-adapter.ts` already translates to `order` by hand. + * A necessary translation nothing enforced is exactly ADR-0049's shape. + * + * Measured on `main` before this rejection existed, on the schema side of the + * same door: + * + * ``` + * SortNodeSchema.parse({ field: 'updated_at', direction: 'desc' }) + * → { field: 'updated_at', order: 'asc' } + * ``` + * + * So the failure was not "unsorted" — it was sorted the OTHER WAY, and with a + * `limit` that means a different set of rows came back under a 200. Worse than + * the family `invalidSortError` was built for (#3948, #4226): those return the + * right rows in an arbitrary order, this one returns the wrong rows. + * + * `INVALID_SORT` rather than a new code: one condition — "this sort was not + * applied as written" — keeps one wire code however the caller reached it. + */ +function invalidSortDirectionKeyError(param: string, field: string): Error { + return invalidSortError( + param, + `spells the sort direction for '${field}' as \`direction\`, which is not a key on this ` + + 'axis — the QueryAST sort node is `{ field, order }`', + { + hint: + ` Write \`{ field: '${field}', order: 'desc' }\`. \`direction\` is` + + " `IReportService.orderBy`'s vocabulary, a genuinely different contract; on this" + + ' axis it was silently dropped and `order` fell back to `asc`, so a descending' + + ' request came back ascending — and with `limit`, a different set of rows.', + extra: { field, key: 'direction' }, + }, + ); +} + /** * [#4254] An aggregation-axis value (`groupBy` / `aggregations`) whose SHAPE * the spec's `QueryAST` cannot read — a non-array, an entry that names no @@ -1140,6 +1182,13 @@ function invalidQueryError( * route and the runtime dispatcher a single answer instead of four. Anything * that still cannot be read as a sort is a 400 rather than a silent no-op: per * #3948, an unapplied sort must not look like an applied one. + * + * [#4721] One key gets named treatment on top of that: a node written + * `{ field, direction }` is refused with {@link invalidSortDirectionKeyError} + * rather than read as "sort by `field`, direction unspecified". It is the wire + * half of a door whose schema half is `SortNodeSchema`'s + * `aliases: { direction: 'order' }` — both closed in one change, because + * guarding one door only is the asymmetry #1535 shipped and #4522 came back for. */ function normalizeSortNodes(value: unknown, param: string): Array<{ field: string, order: 'asc' | 'desc' }> { const direction = (raw: unknown, subject: string): 'asc' | 'desc' => { @@ -1165,6 +1214,18 @@ function normalizeSortNodes(value: unknown, param: string): Array<{ field: strin if (el && typeof el === 'object' && !Array.isArray(el)) { const node = el as { field?: unknown, order?: unknown }; if (typeof node.field === 'string' && node.field.trim()) { + // [#4721] `{ field, direction }` — the wire half of the door + // `SortNodeSchema`'s `aliases: { direction: 'order' }` closes on + // the schema side. Checked HERE and not left to the schema + // because an external caller's `orderBy` never reaches it: this + // normalizer runs at the ingress, ahead of any QueryAST parse. + // + // Rejecting only on a well-formed sort NODE is deliberate. In + // the sibling `{field: direction}` map form a key named + // `direction` is an ordinary column name ("sort by the + // `direction` column"), and that form does not reach this + // branch — it has no string `field`. + if ('direction' in node) throw invalidSortDirectionKeyError(param, node.field.trim()); return { field: node.field.trim(), order: direction(node.order, `'${node.field}'`) }; } } diff --git a/packages/objectql/src/query-expression-conformance.test.ts b/packages/objectql/src/query-expression-conformance.test.ts index 7c79840c89..1487537776 100644 --- a/packages/objectql/src/query-expression-conformance.test.ts +++ b/packages/objectql/src/query-expression-conformance.test.ts @@ -318,6 +318,42 @@ describe('#4226 — sort / select / expand on the list path (real ObjectQL engin .rejects.toMatchObject({ status: 400, code: 'INVALID_SORT' }); }); + /** + * [#4721] The one unknown KEY on this axis, against the real engine. + * + * Every rejection above is about a bad VALUE — a field that does not exist, + * a direction that is not asc/desc. `{ field, direction }` is well-formed on + * both counts and was therefore accepted, with the foreign key stripped and + * `order` left on its `asc` default: a DESCENDING request answered + * ascending, 200, no signal. Paired with `top` that is not a reordered page + * but a different set of rows — the "latest N" footgun below, reached + * through a spelling rather than a typo. + * + * `direction` is `IReportService.orderBy`'s live vocabulary, which + * `plugin-auth/objectql-adapter.ts` already translates by hand; the schema + * half of this door (`SortNodeSchema`'s `aliases: { direction: 'order' }`) + * landed in the same change. + */ + it('sorting with `direction` instead of `order` is a 400, not a silently ASCENDING page', async () => { + // The control first: this is what the caller meant, and it works. + expect(titles(await protocol.findData({ + object: 'showcase_task', query: { orderBy: [{ field: 'title', order: 'desc' }], top: 2 }, + }))).toEqual(['E', 'D']); + + // Same request, foreign spelling. Pre-#4721 this resolved to ['A','B'] — + // the opposite end of the table, under an ordinary success. + await expect(protocol.findData({ + object: 'showcase_task', query: { orderBy: [{ field: 'title', direction: 'desc' }], top: 2 }, + })).rejects.toMatchObject({ status: 400, code: 'INVALID_SORT', field: 'title' }); + + // And the rejection hands over the translation — `direction` → `order` + // is not reachable by edit distance, so a bare refusal would leave the + // caller exactly where the silent strip did. + await expect(protocol.findData({ + object: 'showcase_task', query: { orderBy: [{ field: 'title', direction: 'desc' }] }, + })).rejects.toThrow(/order: 'desc'/); + }); + it('an unapplied sort can no longer hide behind `top` — the "latest N" footgun', async () => { // This pairing is the whole reason the sort axis matters. Pre-fix it // answered 200 with an arbitrary 2 of 5 rows and no way to tell. diff --git a/packages/spec/src/data/query.test.ts b/packages/spec/src/data/query.test.ts index ab25681884..537691093b 100644 --- a/packages/spec/src/data/query.test.ts +++ b/packages/spec/src/data/query.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from 'vitest'; import { QuerySchema, FieldNodeSchema, + SortNodeSchema, AggregationFunction, type QueryAST, } from './query.zod'; @@ -115,6 +116,85 @@ describe('FieldNode — the nested-select object form is REMOVED (#4196)', () => }); }); +/** + * #4721 — the sort node is closed against unknown keys, and `direction` gets a + * named translation on the way out. + * + * The number that made this worth a wire-breaking change is not "a key was + * dropped" but which way the rows then came back. Measured on `main` before the + * change: + * + * ``` + * SortNodeSchema.parse({ field: 'updated_at', direction: 'desc' }) + * → { field: 'updated_at', order: 'asc' } + * ``` + * + * Descending in, ascending out, success reported. So these tests pin the + * REJECTION and the PRESCRIPTION, not merely "unknown keys are unrecognized": + * a strict schema whose error says `Unrecognized key(s): direction` and nothing + * else leaves the caller exactly where the silent strip did — they still do not + * know that the word they want is `order`. Edit distance cannot get there + * (`direction` → `order` is not a typo), which is why the alias table is the + * load-bearing half. + */ +describe('SortNode — closed, and `direction` carries its translation (#4721)', () => { + it('accepts the canonical two-key node, defaulting `order` to asc', () => { + expect(SortNodeSchema.parse({ field: 'updated_at', order: 'desc' })) + .toEqual({ field: 'updated_at', order: 'desc' }); + expect(SortNodeSchema.parse({ field: 'updated_at' })) + .toEqual({ field: 'updated_at', order: 'asc' }); + }); + + it('REJECTS `direction` instead of silently sorting the other way', () => { + const r = SortNodeSchema.safeParse({ field: 'updated_at', direction: 'desc' }); + expect(r.success, 'this parsed to `order: asc` before #4721').toBe(false); + expect(r.error!.issues[0].message).toContain('`direction` → `order`'); + }); + + it('names the surface and echoes the offending key', () => { + const message = SortNodeSchema.safeParse({ field: 'x', direction: 'desc' }) + .error!.issues[0].message; + expect(message).toContain('this sort node'); + expect(message).toContain('`direction`'); + }); + + it('rejects `direction` even when `order` is also present', () => { + // Both keys written means the caller is guessing. Half-honouring the guess + // is how the two vocabularies stay alive; refusing it is how one wins. + expect(SortNodeSchema.safeParse({ field: 'x', order: 'desc', direction: 'asc' }).success) + .toBe(false); + }); + + it('closes the same door through `QueryAST.orderBy`, which is where it is written', () => { + const r = QuerySchema.safeParse({ + object: 'sales', + orderBy: [{ field: 'updated_at', direction: 'desc' }], + limit: 20, + }); + expect(r.success).toBe(false); + expect(JSON.stringify(r.error!.issues)).toContain('`direction` → `order`'); + + // The control: the canonical spelling still parses through the same path. + expect(QuerySchema.parse({ + object: 'sales', orderBy: [{ field: 'updated_at', order: 'desc' }], limit: 20, + })).toMatchObject({ orderBy: [{ field: 'updated_at', order: 'desc' }] }); + }); + + it('suggests a declared key for an ordinary typo, and never a tombstone', () => { + const message = SortNodeSchema.safeParse({ field: 'x', ordr: 'desc' }) + .error!.issues[0].message; + expect(message).toContain('`ordr` → `order`'); + }); + + it('leaves the REST of the query dialect open — the carve-out is this schema only', () => { + // `query.zod.ts` stays classed `open` in the strictness ledger; only + // `SortNodeSchema` was re-classed authorable. Pinned so a later blanket + // `.strict()` on the file is a deliberate decision with its own ruling + // rather than a side effect of this change (#4001 tracks the top level). + expect(QuerySchema.safeParse({ object: 'sales', nonsenseKey: 1 }).success).toBe(true); + }); +}); + describe('QuerySchema - Aggregations', () => { // ============================================================================ // Basic Aggregation Tests diff --git a/packages/spec/src/data/query.zod.ts b/packages/spec/src/data/query.zod.ts index de5dfd1fe4..67d24ea81f 100644 --- a/packages/spec/src/data/query.zod.ts +++ b/packages/spec/src/data/query.zod.ts @@ -3,16 +3,61 @@ import { z } from 'zod'; import { FilterConditionSchema } from './filter.zod'; +import { lazySchema } from '../shared/lazy-schema'; +import { retiredKey } from '../shared/retired-key'; +import { strictObject } from '../shared/strict-object'; + /** * Sort Node - * Represents "Order By". + * Represents "Order By" — one `{ field, order }` pair. + * + * **Closed against unknown keys (#4721, #4001).** This is the one site in + * `query.zod.ts` carved out of the file's blanket `open` classification, and the + * carve-out is what the shape earns: the rest of the file is the query DIALECT, + * where user data flows through predicate values, while a sort node is a closed + * two-key tuple with no user-data face at all. Classing the whole file was the + * imprecise thing, not closing this schema. + * + * What it stops, measured on `main` before the change: + * + * ``` + * SortNodeSchema.parse({ field: 'updated_at', direction: 'desc' }) + * → { field: 'updated_at', order: 'asc' } + * ``` + * + * `direction` was stripped, `order` fell back to its `asc` default, and the sort + * ran in the OPPOSITE direction under an ordinary success. Paired with `limit` — + * which is how a caller asks for "the latest N" — that is not a reordered page + * but a DIFFERENT SET OF ROWS, with no signal anywhere in the response. + * + * `direction` gets a named alias rather than a distance-based suggestion because + * it is not a typo: it is `IReportService.orderBy`'s live vocabulary + * (`contracts/report-service.ts`), a genuinely different contract that + * `plugin-auth/objectql-adapter.ts` already translates by hand. Edit distance + * can never reach a different WORD for the same intent — the `visibleWhen → + * visible` class (see `shared/strict-object.ts`) — so only a hand-written entry + * puts the prescription in the author's hands. + * + * The wire-facing half of the same door is `normalizeSortNodes` + * (`metadata-protocol/src/protocol.ts`), which rejects `direction` by name with + * `400 INVALID_SORT` before a request ever reaches this schema. Both were closed + * in one change deliberately: closing only the schema is the door asymmetry + * #1535 shipped and #4522 had to come back for. */ -import { lazySchema } from '../shared/lazy-schema'; -import { retiredKey } from '../shared/retired-key'; -export const SortNodeSchema = lazySchema(() => z.object({ - field: z.string(), - order: z.enum(['asc', 'desc']).default('asc') -})); +export const SortNodeSchema = lazySchema(() => strictObject( + { + surface: 'this sort node', + history: + 'Until #4721 an unknown key here was dropped silently and `order` fell back to its `asc` ' + + 'default, so a descending request came back ascending — and with `limit`, a different ' + + 'set of rows under an ordinary 200.', + aliases: { direction: 'order' }, + }, + { + field: z.string(), + order: z.enum(['asc', 'desc']).default('asc'), + }, +)); /** * Aggregation Function Enum From ca677768d9ffe24794037ab1783dd9c60e4ef2a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 16:03:42 +0000 Subject: [PATCH 2/2] docs(spec): keep SortNodeSchema's page description short; rationale moves to line comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `build-docs.ts` takes the FIRST `/** */` block in a `.zod.ts` file as the reference page's description, and joins every line of it with a blank line. The long #4721 rationale therefore rendered as a 56-line wall at the top of `content/docs/references/data/query.mdx`, where a customer reads what a sort node IS — not why one schema in the file is strict. The prose is unchanged, it is just `//` instead of `/** */` so the generator cannot pick it up, with a note at the top saying why it must stay that way. Regenerated: query.mdx (two lines), and the two skill reference indexes, which grew transitive entries because query.zod.ts now imports shared/strict-object. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ehu85kbvMcrNTUJjwxvLJ9 --- content/docs/references/data/query.mdx | 4 +- packages/spec/src/data/query.zod.ts | 74 ++++++++++--------- skills/objectstack-api/references/_index.md | 4 + skills/objectstack-query/references/_index.md | 8 ++ 4 files changed, 55 insertions(+), 35 deletions(-) diff --git a/content/docs/references/data/query.mdx b/content/docs/references/data/query.mdx index 358fd85e33..320ee03145 100644 --- a/content/docs/references/data/query.mdx +++ b/content/docs/references/data/query.mdx @@ -7,7 +7,9 @@ description: Query protocol schemas Sort Node -Represents "Order By". +Represents "Order By" — one `\{ field, order \}` pair. Unknown keys are + +REJECTED (#4721); spell the direction `order`, never `direction`. **Source:** `packages/spec/src/data/query.zod.ts` diff --git a/packages/spec/src/data/query.zod.ts b/packages/spec/src/data/query.zod.ts index 67d24ea81f..6f7e9d4b5f 100644 --- a/packages/spec/src/data/query.zod.ts +++ b/packages/spec/src/data/query.zod.ts @@ -9,41 +9,47 @@ import { strictObject } from '../shared/strict-object'; /** * Sort Node - * Represents "Order By" — one `{ field, order }` pair. - * - * **Closed against unknown keys (#4721, #4001).** This is the one site in - * `query.zod.ts` carved out of the file's blanket `open` classification, and the - * carve-out is what the shape earns: the rest of the file is the query DIALECT, - * where user data flows through predicate values, while a sort node is a closed - * two-key tuple with no user-data face at all. Classing the whole file was the - * imprecise thing, not closing this schema. - * - * What it stops, measured on `main` before the change: - * - * ``` - * SortNodeSchema.parse({ field: 'updated_at', direction: 'desc' }) - * → { field: 'updated_at', order: 'asc' } - * ``` - * - * `direction` was stripped, `order` fell back to its `asc` default, and the sort - * ran in the OPPOSITE direction under an ordinary success. Paired with `limit` — - * which is how a caller asks for "the latest N" — that is not a reordered page - * but a DIFFERENT SET OF ROWS, with no signal anywhere in the response. - * - * `direction` gets a named alias rather than a distance-based suggestion because - * it is not a typo: it is `IReportService.orderBy`'s live vocabulary - * (`contracts/report-service.ts`), a genuinely different contract that - * `plugin-auth/objectql-adapter.ts` already translates by hand. Edit distance - * can never reach a different WORD for the same intent — the `visibleWhen → - * visible` class (see `shared/strict-object.ts`) — so only a hand-written entry - * puts the prescription in the author's hands. - * - * The wire-facing half of the same door is `normalizeSortNodes` - * (`metadata-protocol/src/protocol.ts`), which rejects `direction` by name with - * `400 INVALID_SORT` before a request ever reaches this schema. Both were closed - * in one change deliberately: closing only the schema is the door asymmetry - * #1535 shipped and #4522 had to come back for. + * Represents "Order By" — one `{ field, order }` pair. Unknown keys are + * REJECTED (#4721); spell the direction `order`, never `direction`. */ +// ⚠️ Keep the block above short: `build-docs.ts` takes the FIRST JSDoc block in +// the file as this page's description, so the rationale below is line comments. +// +// ─── Why this one schema is strict while the rest of the file is not (#4721) ── +// +// `query.zod.ts` is classed `open` in the #4001 strictness ledger, and +// `SortNodeSchema` is carved out of that blanket. The carve-out is what the +// shape earns: the rest of the file is the query DIALECT, where user data flows +// through predicate values, while a sort node is a closed two-key tuple with no +// user-data face at all. Classing by FILE was the imprecise instrument here, not +// closing this schema. +// +// What the closure stops, measured on `main` before the change: +// +// SortNodeSchema.parse({ field: 'updated_at', direction: 'desc' }) +// → { field: 'updated_at', order: 'asc' } +// +// `direction` was stripped, `order` fell back to its `asc` default, and the sort +// ran in the OPPOSITE direction under an ordinary success. Paired with `limit` — +// which is how a caller asks for "the latest N" — that is not a reordered page +// but a DIFFERENT SET OF ROWS, with no signal anywhere in the response. +// +// `direction` gets a named alias rather than a distance-based suggestion because +// it is not a typo: it is `IReportService.orderBy`'s live vocabulary +// (`contracts/report-service.ts`), a genuinely different contract that +// `plugin-auth/objectql-adapter.ts` already translates by hand. Edit distance +// can never reach a different WORD for the same intent — the `visibleWhen → +// visible` class (see `shared/strict-object.ts`) — so only a hand-written entry +// puts the prescription in the author's hands. +// +// The wire-facing half of the same door is `normalizeSortNodes` +// (`metadata-protocol/src/protocol.ts`), which rejects `direction` by name with +// `400 INVALID_SORT` before a request ever reaches this schema. Both were closed +// in one change deliberately: closing only the schema is the door asymmetry +// #1535 shipped and #4522 had to come back for. +// +// Deliberately NOT taken here: `BaseQuerySchema`'s own top level stays +// non-strict. That is #4001's to schedule. export const SortNodeSchema = lazySchema(() => strictObject( { surface: 'this sort node', diff --git a/skills/objectstack-api/references/_index.md b/skills/objectstack-api/references/_index.md index 1ff4bbefca..446d3444f8 100644 --- a/skills/objectstack-api/references/_index.md +++ b/skills/objectstack-api/references/_index.md @@ -24,12 +24,16 @@ from `node_modules` — there is no local copy in the skill bundle. - `node_modules/@objectstack/spec/src/api/error-code-ledger.zod.ts` — Error-Code Ledger (ADR-0112 D3). - `node_modules/@objectstack/spec/src/api/realtime-shared.zod.ts` — Realtime Shared Protocol - `node_modules/@objectstack/spec/src/data/data-engine.zod.ts` — Data Engine Protocol +- `node_modules/@objectstack/spec/src/data/field.zod.ts` — Field Type Enum - `node_modules/@objectstack/spec/src/data/filter.zod.ts` — Unified Query DSL Specification - `node_modules/@objectstack/spec/src/data/query.zod.ts` — Sort Node - `node_modules/@objectstack/spec/src/kernel/execution-context.zod.ts` — Execution Context Schema +- `node_modules/@objectstack/spec/src/kernel/metadata-protection.zod.ts` — Metadata Protection Model — Phase 1 (ADR-0010) - `node_modules/@objectstack/spec/src/security/explain.zod.ts` — [ADR-0090 D6] Access-explanation contract — `explain(principal, object, +- `node_modules/@objectstack/spec/src/shared/expression.zod.ts` — Expression Protocol - `node_modules/@objectstack/spec/src/shared/http.zod.ts` — Shared HTTP Schemas - `node_modules/@objectstack/spec/src/shared/identifiers.zod.ts` — System Identifier Schema +- `node_modules/@objectstack/spec/src/shared/suggestions.zod.ts` — "Did you mean?" Suggestion Utilities ## How to read these diff --git a/skills/objectstack-query/references/_index.md b/skills/objectstack-query/references/_index.md index 0418e32b8d..8586f0b0ae 100644 --- a/skills/objectstack-query/references/_index.md +++ b/skills/objectstack-query/references/_index.md @@ -13,6 +13,14 @@ from `node_modules` — there is no local copy in the skill bundle. - `node_modules/@objectstack/spec/src/data/filter.zod.ts` — Unified Query DSL Specification - `node_modules/@objectstack/spec/src/data/query.zod.ts` — Sort Node +## Transitive dependencies + +- `node_modules/@objectstack/spec/src/data/field.zod.ts` — Field Type Enum +- `node_modules/@objectstack/spec/src/kernel/metadata-protection.zod.ts` — Metadata Protection Model — Phase 1 (ADR-0010) +- `node_modules/@objectstack/spec/src/shared/expression.zod.ts` — Expression Protocol +- `node_modules/@objectstack/spec/src/shared/identifiers.zod.ts` — System Identifier Schema +- `node_modules/@objectstack/spec/src/shared/suggestions.zod.ts` — "Did you mean?" Suggestion Utilities + ## How to read these 1. The schemas are runtime Zod definitions. Use `Read` on the absolute