Skip to content

Commit 7d0f911

Browse files
os-billclaude
andauthored
fix(spec,runtime): ActionEngineFacade.find takes the engine query envelope, not a bare filter (#19223)
Fixes #15124 Clause-②: yes `ctx.engine.find(object, query)` now takes the engine's query envelope — `EngineQueryOptions`, **by identity**, the same options bag `IDataEngine.find` and ObjectQL's own `engine.find` take. The bare-filter parameter shape is withdrawn. One platform, one query shape. Director seat ruling, decision batch #123 item 3, letter D (comment 5644710751), carrying the maintainer's 「同意」: > `ActionEngineFacade.find` takes the same query envelope as the engine's `find`; the bare-filter parameter shape is **withdrawn**. **BREAKING for action handlers**, landed under the launch-window convention: no deprecation window, migration in the changeset and registered as an ADR-0087 semantic entry (`action-engine-facade-find-query-envelope`). The changeset carries the arm the level axis needs — `Clause-②: yes (narrowing)` — and a `minor` bump on both published packages, per the no-major rule. ## Migration | You wrote | Write instead | | --- | --- | | `ctx.engine.find('task', { status: 'open' })` | `ctx.engine.find('task', { where: { status: 'open' } })` | | `ctx.engine.find('task', {})` | unchanged — an empty envelope is still the unfiltered read | Lossless and mechanical. `tsc --noEmit` over a consumer's handlers finds every unmigrated call, because a bare filter is now a compile error (below). ## What changed - **`packages/spec/src/ui/action-params.zod.ts`** — the declaration. `find(object, query: EngineQueryOptions)`, plus a member doc that states the envelope, the migration, the measured refusal and the `context` rule. - **`packages/runtime/src/action-execution.ts`** — the `find` arm passes the envelope through. The double-wrap is gone; `context` is still spread last, so the facade's own elevated `ExecutionContext` wins over a caller-supplied one. - **`packages/spec/src/ui/action-params.test.ts`** — #14175's MEASURED-GAP pin flipped into a refusal pin, plus positive controls for the envelope keys a handler can now reach. - **`packages/runtime/src/action-engine-facade-find-envelope.test.ts`** (new) — the runtime half: what argument the engine actually RECEIVED, not what rows came back. A rows-only pin is exactly what the original defect passed. - **`packages/spec/src/migrations/entries/semantic/18.action-engine-facade-find-query-envelope.ts`** (new) — the ADR-0087 D3 entry. Semantic rather than a D2 conversion because the rewrite lives in an authored TypeScript function body, which `migrate meta` cannot reach. - **`examples/app-todo/src/actions/task.handlers.ts`** — the one in-repo caller (see the probe below). - **`content/docs/ui/actions.mdx`** — the callout, inverted, with an upgrade note. - Generated: the migration registry, `api-surface-declarations/`, and the two skill reference indexes. ## Measurements the dispatch asked for **The blast-radius probe, re-run with controls.** `ActionEngineFacade` has **zero importers outside `packages/spec`** at `24d622b9` — the card's reading holds. Probe exit 0 / 40 hits, all prose or the unrelated runtime symbol `buildActionEngineFacade`; **firing control** `ActionHandlerContext` finds a real cross-package import (`examples/app-todo/.../task.handlers.ts`), **dark control** `ActionEngineFacadeZZZ` exits 1 / 0 hits. Exit codes captured before any pipe. ⚠️ **But importer count is the wrong denominator here, and the seat should read this.** The facade is reached through `ActionHandlerContext.engine`, so every handler annotated with the published context type is a typed caller without ever naming `ActionEngineFacade`. That is where the one real call site is. **Does anything in-repo call the facade with a bare filter?** Yes — one: `deleteCompletedTasks` in `examples/app-todo/src/actions/task.handlers.ts`, migrated here. Its sibling `exportTasksToCSV` passes `{}` and is unchanged. No other in-repo caller exists. **Is the envelope type importable without a cycle?** Yes, no type move needed. `packages/spec/src/data/data-engine.zod.ts` does not import from `ui/` (probe exit 1), and the import is `import type`, so it is erased entirely. **Loud refusal or silent acceptance?** Loud, on **both** paths — and the second one is the half I expected to be open: - an object literal (`{ status: 'completed' }`) fails the excess-property check; - a filter held in a `FilterCondition` **variable** fails **TS2559** — `EngineQueryOptions` is a weak type, every key optional, and a bag of field names has no property in common with it. `FilterCondition`'s string index signature does not rescue it. Both are pinned. Only a compile error is reachable; no runtime-only refusal is involved. **Does it widen?** Yes, and deliberately: `fields`, `orderBy`, `limit`, `offset`, `expand` and `search` are reachable from a handler for the first time — the old parameter had nowhere to carry them. The one key worth calling out is **`context`**: the envelope admits it because every engine option bag does, but this facade is trusted and context-less by design, so a caller-supplied `context` is **overridden, not honoured**. Documented on the member and pinned in the runtime test, because it is a security-shaped property of a spread ORDER. **The 8 regenerated declaration files, measured rather than waved at.** Only `ui.txt` carries semantics; the other seven carry declaration-emit ORDER churn (enum member order, `Exclude` key order), because adding one `import type` moved the d.ts chunking. Control: a **pristine worktree at the branch point** ran the same `build && gen:api-surface-declarations` and rewrote **nothing at all**. So every byte here is downstream of this diff, not pre-existing drift. ## `skills/**` readings Both changed files are **generator-owned** (`gen:skill-refs`), and `check-skills-token-ratchet` classifies them as "measured, not ratcheted" — no authored budget is spent (129392 / 145656, -16264, unchanged by this PR). - Changed files, whole-file: `skills/objectstack-data/references/_index.md` 67 → 70 (+3); `skills/objectstack-ui/references/_index.md` 57 → 60 (+3). Purely additive: the three new transitive spec modules the import pulls in. - Whole published package, sum of every `SKILL.md`: 6145 → 6145 (**0**). No `SKILL.md` is touched by this diff. ⚠️ **This diff therefore touches a Tier H governed surface** (`skills/**`). Landing waits for its tier's record; I have left it draft. ## Verification Repo-wide, not narrowed: **`eslint . --no-inline-config` over all 6911 files eslint's own config judges — 0 errors, 0 warnings.** No type-aware linting is configured, so nothing here can move an untouched file's verdict either way. | Run | Result | | --- | --- | | `dispatch-gates --commands` | **121 of 121 derived families green**, each exit code captured before any pipe | | spec tests (`action-params`, `migrations`, `data-engine`) | 3 files / 267 tests passed | | runtime tests (4 facade files, incl. the new pin) | 4 files / 27 tests passed | | `examples/app-todo` tests | 6 files / 233 tests passed | | `@objectstack/spec typecheck` | exit 0 (test-layer debt held, not grown) | | `@objectstack/runtime typecheck` | exit 0 (test-layer debt held, not grown) | | `@objectstack/example-todo typecheck` | exit 0, full dependency closure built | | `check:generated` | 16 of 16 up to date after regeneration | Everything ran against a real build — no `OS_SKIP_DTS`. All heavy runs went through `scripts/pm/os-verify-lock.sh`. ## Acceptance notes **To file (contract-violation class).** `scripts/check-spec-docblock-symbol-anchors.mjs` declares `CENSUS_RESIDUAL` **shrink-only** and its stale-row check prescribes deleting a row the day its citation is repaired — but two pinned counters make that deletion impossible. Measured on this exact repair: deleting the one repaired row reds the self-test on `liveTriage.pinned.length === CENSUS_17065.hardFindings` (a frozen, dated census), and then again on `SELF_TEST_BATTERY_FLOOR` 64 → 61, because the roster loop registers three cases per row. The census cannot simply be decremented either — its own arithmetic check binds `trackedTargetLineCitations + declinedCitations === commentProseLineCitations`. So the first legitimate repair has only dishonest exits: leave a stale row, or edit a dated record until it no longer reproduces at its own sha. Dedupe words: check-spec-docblock-symbol-anchors, CENSUS_RESIDUAL, hardFindings, SELF_TEST_BATTERY_FLOOR, shrink-only residual. **Carrier: #16960**, the repair card that gate's own header names, which hits this on its first repair. ⭐ **Which is why this PR deliberately KEEPS the `:1183` citation** in the member doc, as data about where the wrap used to live. My first rewrite dropped it, which silently repaired that residual row — a repair this lane was not dispatched to make and cannot complete honestly. **Noted, not filed.** A branch `claude/issue-19011-revert-declaration-text-snapshot` is in flight against the declaration-text snapshot family, which is the same artifact family as the eight files regenerated here. Carrier: whoever lands #19011 — a textual collision is likely, and the resolution is a regeneration, never a textual merge. **Deliberately NOT done.** #14175's changeset text is no longer a `.changeset/` file — it was consumed at release and now lives in `packages/spec/CHANGELOG.md`, which AGENTS.md forbids editing in a code PR (a factual error in a released entry is amended in a dedicated docs-only PR). The erratum this PR can deliver is its own changeset, which is the live channel to an upgrading consumer. Flagged for the seat rather than taken. ## 维护者速读(草稿) **改了什么。** 动作处理器里查数据的写法统一了。以前 `ctx.engine.find` 只收筛选条件本身,而平台其它地方的 `find` 都收完整查询信封,于是最自然的写法反而是错的 —— 多包一层 `where` 编译能过、运行不报错、永远返回空列表。现在这个参数就是引擎自己的查询类型:`find('task', { where: { status: 'open' } })`。 **为什么这样改。** 裁决选的是 D:与其为了拦住写错的人而把 `where` 变成全平台保留字(等于向每个客户的数据模型征用一个词),不如把参数形状本身收回来。代价对称了 —— 不保留任何词,并且顺带让处理器第一次能用 `fields` / `orderBy` / `limit` 分页和投影。 **风险与代价(含回滚)。** 这是破坏性变更:老写法从今天起编译不过。好消息是它**一定**编译不过 —— 对象字面量和变量两条路都实测会报错,所以升级者跑一次 `tsc` 就能拿到完整清单,不存在漏改后静默跑错的情况。仓内只有一个调用点,已改。真正要提醒升级者的是:之前写对了信封的人,他们的代码一直在静默返回空列表,所以不能只验证"还能跑",要验证"真的查出行来"。回滚 = revert 本 PR,无数据迁移、无存量元数据受影响。 **席位意见。** **你要做的。** 这个 diff 碰到了 `skills/**`(两个生成的引用索引,各 +3 行,不占技能包预算),属于 Tier H 治理面 —— 需要你点头才能落地,我已保持 draft。除此之外无需操作。 --- _Generated by [Claude Code](https://claude.ai/code/session_01JbZnqu8bt6YqfJsr9vaFb3)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c7448dc commit 7d0f911

21 files changed

Lines changed: 1420 additions & 652 deletions

File tree

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
---
2+
'@objectstack/spec': minor
3+
'@objectstack/runtime': minor
4+
---
5+
6+
**BREAKING for action handlers** — `ActionEngineFacade.find` takes the engine's query ENVELOPE; the bare-filter parameter shape is withdrawn (#15124)
7+
8+
Clause-②: yes (narrowing)
9+
10+
`ctx.engine.find(object, query)` now takes `EngineQueryOptions` — the same
11+
options bag `IDataEngine.find` and ObjectQL's own `engine.find` take, named by
12+
identity rather than restated. **One platform, one query shape.**
13+
14+
### Migration — FROM → TO
15+
16+
| You wrote | Write instead |
17+
| --- | --- |
18+
| `ctx.engine.find('task', { status: 'open' })` | `ctx.engine.find('task', { where: { status: 'open' } })` |
19+
| `ctx.engine.find('task', { amount: { $gt: 100 } })` | `ctx.engine.find('task', { where: { amount: { $gt: 100 } } })` |
20+
| `ctx.engine.find('task', {})` | unchanged — an empty envelope is still the unfiltered read |
21+
22+
The rewrite is lossless and mechanical: the filter moves under `where`, verbatim.
23+
`tsc --noEmit` over your handlers finds every unmigrated call — see below.
24+
25+
### Why the shape was withdrawn rather than the bar closed
26+
27+
Until now this parameter was the `where` HALF of a query while every other
28+
`find` on the platform took the whole envelope, and the runtime wrapped what it
29+
was given. That made the most natural spelling the wrong one, silently: an
30+
author who passed the engine's own envelope reached the engine as
31+
`{ where: { where: … } }` — a filter on a field named `where` — which matches no
32+
row and resolves to `[]` with **no error at all**. A handler that made the
33+
mistake ran to completion over zero rows for as long as it shipped, and its own
34+
hand-written test double, written to the same belief, passed every assertion.
35+
Because an empty `{}` skipped the wrap, one unfiltered read kept working under
36+
either belief, so a dead handler looked partially alive.
37+
38+
Refusing `where` at the top level instead — intersecting the old parameter with
39+
`{ where?: never }` — was rejected: it asserts a vocabulary fact the spec
40+
declares nowhere, reserving the field name `where` across every customer's data
41+
model to buy one parameter's compile-time check. Aligning the parameter removes
42+
the ambiguity at its root and reserves nothing.
43+
44+
### What the new declaration refuses, measured
45+
46+
If your handler is typed with the published `ActionHandlerContext`, a bare filter
47+
no longer type-checks on **either** path you can reach it by:
48+
49+
- an object literal (`{ status: 'completed' }`) fails the excess-property check —
50+
a field name is not an envelope key;
51+
- a filter held in a `FilterCondition` variable fails **TS2559** — every envelope
52+
key is optional, so a bag of field names has no property in common with it.
53+
54+
The envelope's own keys are typed too: `where: 'a = b'`, `fields: 'id,subject'`
55+
and `limit: '50'` are each refused.
56+
57+
**If your handler is NOT typed with it** — a handler in an `objectstack.config.js`
58+
/ `.mjs`, one annotated with your own copy of the context type, or a `(ctx: any)`
59+
handler — nothing above reaches you, so the facade refuses the withdrawn shape at
60+
**runtime** instead, before the engine, with the same prescription:
61+
62+
```
63+
find('task') was given a key 'status' the query envelope does not carry.
64+
ctx.engine.find(object, query) takes the engine QUERY ENVELOPE, not a bare
65+
filter — move the filter under `where`: find(object, { where: { … } }).
66+
Envelope keys: context, cursor, distinct, expand, fields, limit, offset,
67+
orderBy, search, searchFields, top, where.
68+
```
69+
70+
⚠️ **That refusal matters most for a filter whose value is `null`.** The engine's
71+
own unknown-option check exempts a `null` value, because on an option bag a
72+
`null` is a withdrawal. On a filter it is the "rows with no X" idiom, so
73+
`{ deleted_at: null }` would have been dropped unexecuted and the read would have
74+
widened to **every row** — including the ones you were excluding — with no error
75+
at all. It is refused instead.
76+
77+
### What this opens
78+
79+
`fields`, `orderBy`, `limit`, `offset` and `expand` are reachable from an action
80+
handler for the first time — under the old parameter there was nowhere to carry
81+
them. A caller-supplied `context` is **ignored**: this facade is trusted and
82+
context-less by design, and the runtime stamps its own elevated
83+
`ExecutionContext` last. Do not write one — it reads as authorization and is
84+
none.
85+
86+
### Checking a migrated handler
87+
88+
Do not settle for "it still resolves". A handler that had been passing the
89+
envelope was returning `[]` on **every** call, so a suite written against the
90+
mistake passes and the row count is the only witness. Re-run each migrated
91+
handler against seeded data and assert it returns the rows its filter selects.
92+
93+
<!-- adr-0087: registered action-engine-facade-find-query-envelope -->

‎content/docs/ui/actions.mdx‎

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -170,19 +170,36 @@ already `completed` — is not stamped, so nothing overwrites the handler's valu
170170
and nothing strips it either, and the action silently replaces the real
171171
completion timestamp with "now". That is why the snippet sends `status` alone.
172172

173-
<Callout type="warn">
174-
**`ctx.engine.find(object, filter)` takes a filter, not a query.** The second
175-
argument is the `where` half only — `{ status: 'completed' }`, operators
176-
(`{ amount: { $gt: 100 } }`), `$and` / `$or` / `$not` — and the runtime wraps
177-
it in `where` itself. Passing an ObjectQL envelope
178-
(`{ where: { status: 'completed' } }`) raises no error: it becomes
179-
`{ where: { where: … } }`, matches no row, and returns `[]`. An empty filter
180-
(`{}`) is passed through unwrapped, so the one unfiltered read works under
181-
either reading and a handler can look partially alive. The parameter is typed
182-
`FilterCondition` (`ActionEngineFacade` in `@objectstack/spec/ui`), which
183-
refuses a primitive or a mistyped `$and` / `$or` / `$not` but still admits
184-
`where` as a key — the sentence above is the contract, and a hand-written test
185-
double must honour it too.
173+
<Callout type="info">
174+
**`ctx.engine.find(object, query)` takes the engine's query envelope.** The
175+
second argument is the same options bag `engine.find` takes everywhere else —
176+
the filter goes under `where`, and `fields`, `orderBy`, `limit`, `offset` and
177+
`expand` mean what they mean on the engine. One platform, one query shape.
178+
179+
```typescript
180+
await ctx.engine.find('todo_task', { where: { status: 'completed' } });
181+
await ctx.engine.find('todo_task', { where: { amount: { $gt: 100 } }, fields: ['id', 'subject'], limit: 50 });
182+
await ctx.engine.find('todo_task', {}); // the unfiltered read
183+
```
184+
185+
The parameter is typed `EngineQueryOptions` (`ActionEngineFacade` in
186+
`@objectstack/spec/ui`), so if you annotate `ctx` with the published
187+
`ActionHandlerContext` a bare filter is a **compile error** at the call site —
188+
`{ status: 'completed' }` has nowhere to land, and neither does a filter held in
189+
a `FilterCondition` variable. A hand-written test double must honour the envelope
190+
too.
191+
192+
If you do **not** annotate it — a handler in an `objectstack.config.js` /
193+
`.mjs`, your own copy of the context type, or `(ctx: any)` — the facade refuses
194+
the bare filter at **runtime** instead, naming the stray key and prescribing the
195+
same fix. That runtime refusal is what stops `{ deleted_at: null }` from being
196+
dropped unexecuted and quietly widening the read to every row.
197+
198+
**Upgrading?** This parameter used to be the `where` half on its own, and the
199+
runtime wrapped it. `find(o, f)` becomes `find(o, { where: f })`; an unfiltered
200+
`find(o, {})` is unchanged. The old shape had the trap the other way round:
201+
writing the engine's own envelope produced `{ where: { where: … } }`, which
202+
matched no row and returned `[]` with no error at all.
186203
</Callout>
187204

188205
<Callout type="info">

‎examples/app-todo/src/actions/task.handlers.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,11 @@ import type { ActionHandlerContext } from '@objectstack/spec/ui';
3838
// written against the published type at all. The declaration now says
3939
// `string | string[]` (#15117), so the copy is gone and this example
4040
// type-checks against exactly the types a real app gets.
41+
//
42+
// And the copy's `query` bag turned out to be the shape the platform kept:
43+
// #15124 withdrew the bare-filter parameter and `ctx.engine.find` now takes the
44+
// ENGINE's query envelope — `find(object, { where: … })`, the same options bag
45+
// `engine.find` takes anywhere else. One platform, one query shape.
4146

4247
/**
4348
* Mark a single task as complete.
@@ -106,7 +111,9 @@ export async function massCompleteTasks(ctx: ActionHandlerContext): Promise<void
106111
/** Delete all completed tasks */
107112
export async function deleteCompletedTasks(ctx: ActionHandlerContext): Promise<void> {
108113
const { engine } = ctx;
109-
const completed = await engine.find('todo_task', { status: 'completed' });
114+
// [#15124] The filter goes under `where` — the second argument is the
115+
// engine's query envelope, not the `where` half on its own.
116+
const completed = await engine.find('todo_task', { where: { status: 'completed' } });
110117
const ids = completed.map((r) => r.id as string);
111118
if (ids.length > 0) {
112119
await engine.delete('todo_task', ids);
@@ -135,6 +142,7 @@ export async function setReminder(ctx: ActionHandlerContext): Promise<void> {
135142
/** Export tasks to CSV format */
136143
export async function exportTasksToCSV(ctx: ActionHandlerContext): Promise<string> {
137144
const { engine } = ctx;
145+
// An EMPTY envelope is still the unfiltered read — unchanged by #15124.
138146
const tasks = await engine.find('todo_task', {});
139147
const header = 'subject,status,priority,category,due_date';
140148
const rows = tasks.map((t) =>

‎packages/runtime/src/action-body-identity.test.ts‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,12 @@ import { QuickJSScriptRunner } from './sandbox/quickjs-runner.js';
3636
* `ctx.api` binding is exercised end-to-end.
3737
*/
3838
function makeSharingEngine(extra: Record<string, unknown> = {}) {
39-
const writes: Array<{ op: string; object: string; context: any }> = [];
40-
const gate = (op: string, object: string, context: any) => {
41-
writes.push({ op, object, context });
39+
// [#15124] `where` is recorded as well as `context`, so the "the caller's
40+
// predicate must survive" case below can assert the PREDICATE instead of
41+
// asserting that an entry exists.
42+
const writes: Array<{ op: string; object: string; context: any; where?: unknown }> = [];
43+
const gate = (op: string, object: string, context: any, where?: unknown) => {
44+
writes.push({ op, object, context, where });
4245
if (!context?.isSystem && !context?.userId) {
4346
throw new Error(`FORBIDDEN: insufficient privileges to ${op} ${object}`);
4447
}
@@ -58,7 +61,7 @@ function makeSharingEngine(extra: Record<string, unknown> = {}) {
5861
return { ok: true };
5962
},
6063
async find(object: string, options?: any) {
61-
gate('find', object, options?.context);
64+
gate('find', object, options?.context, options?.where);
6265
// [#16370] The by-id pre-load has to be ANSWERED: an action door now
6366
// refuses a row-scoped invocation whose caller-scope subject load did
6467
// not deliver the row, so a rig that answered every read with `[]`
@@ -121,7 +124,9 @@ describe('#3914 — ctx.engine (buildActionEngineFacade)', () => {
121124
await engine.insert('crm_case', { subject: 'x' });
122125
await engine.update('crm_case', 'case_1', { status: 'closed' });
123126
await engine.delete('crm_case', 'case_1');
124-
await engine.find('crm_case', { status: 'open' });
127+
// [#15124] The envelope, not a bare filter — the withdrawn shape is now
128+
// refused by the arm itself, so this call would throw if left as it was.
129+
await engine.find('crm_case', { where: { status: 'open' } });
125130

126131
expect(ql.writes.map((w: any) => w.op)).toEqual(['insert', 'update', 'delete', 'find']);
127132
for (const w of ql.writes) {
@@ -138,10 +143,14 @@ describe('#3914 — ctx.engine (buildActionEngineFacade)', () => {
138143
it('still passes the caller filter on find (context is additive, not a replacement)', async () => {
139144
const ql = makeSharingEngine();
140145
const engine = buildActionEngineFacade(deps, ql, { userId: 'u1' });
141-
await engine.find('crm_case', { status: 'open' });
146+
await engine.find('crm_case', { where: { status: 'open' } });
142147
expect(ql.writes[0].context).toMatchObject({ isSystem: true, userId: 'u1' });
143-
// the caller's predicate must survive alongside the injected context
144-
expect((ql.writes as any)[0]).toBeDefined();
148+
// [#15124] The caller's predicate must survive alongside the injected
149+
// context — asserted on the PREDICATE. This line used to read
150+
// `expect(ql.writes[0]).toBeDefined()`, which is true of any recorded
151+
// call whatever the arm did with the filter, so the one thing the case
152+
// is named for was the one thing it did not check.
153+
expect(ql.writes[0].where).toEqual({ status: 'open' });
145154
});
146155
});
147156

0 commit comments

Comments
 (0)