From 4ed95c4d9453bc5365892b772dc0f4a400e72ec0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 05:20:42 +0000 Subject: [PATCH 1/4] feat(objectql): refuse an afterFind that replaces find()'s array container `ObjectQL.find` declares `Promise` but ended its hook path with `return hookContext.result`, with nothing between the `afterFind` dispatch and that return re-checking the value. A handler assigning `ctx.result = { records: [ ... ] }` therefore made a `find()` declared to resolve to an array resolve to an envelope, silently. Ruled 2026-09-06 (direction 1): `find()` guarantees the array, and a hook that replaces the container is refused loudly. The check sits immediately after `triggerHooks('afterFind', ...)` and BEFORE `maskSecretFields` / `stripSearchCompanionFromRead`, both of which already assume the array. Shaping stays legal: mutating rows in place, dropping keys, filtering rows out and assigning a different ARRAY are all untouched. `Array.isArray` is the whole predicate. Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ Co-authored-by: Claude --- .../src/engine-find-hook-result-shape.test.ts | 249 ++++++++++++++++++ packages/objectql/src/engine.ts | 21 ++ .../objectql/src/find-hook-result-shape.ts | 179 +++++++++++++ packages/objectql/src/index.ts | 14 + .../spec/src/api/error-code-ledger.zod.ts | 17 ++ 5 files changed, 480 insertions(+) create mode 100644 packages/objectql/src/engine-find-hook-result-shape.test.ts create mode 100644 packages/objectql/src/find-hook-result-shape.ts diff --git a/packages/objectql/src/engine-find-hook-result-shape.test.ts b/packages/objectql/src/engine-find-hook-result-shape.test.ts new file mode 100644 index 0000000000..9e73808340 --- /dev/null +++ b/packages/objectql/src/engine-find-hook-result-shape.test.ts @@ -0,0 +1,249 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #15823 — `find()` guarantees the array, and an `afterFind` that REPLACES the +// container is refused loudly. +// +// ## The seam, and what used to happen +// +// `ObjectQL.find` declares `Promise`, and on the hook path it ended with +// `return hookContext.result` with nothing between the `afterFind` dispatch and +// that return asserting the value was still an array. An `afterFind` handler +// assigning `ctx.result = { records: [ … ] }` therefore made a `find()` declared +// to resolve to an array resolve to an envelope — no throw, no diagnostic, no +// log. Measured on a real `ObjectQL` over a real `SqlDriver` (#15823's own +// table: `ARRAY(len=1)` with no hooks, `OBJECT{records}` with the hook), and +// reproduced live before this suite was written by +// `plugin-auth/src/find-envelope-limb-removal.test.ts`, whose #15597 control +// drives fourteen real read call sites into a non-array through exactly this +// handler. +// +// Two readings of that fact pointed opposite ways — the engine guarantees the +// array, or the declaration is wrong and a hook may reshape a read — and the +// maintainer ruled the first (2026-09-06, director seat, #15823): the refusal +// goes in, and every array-or-envelope normalizer limb downstream of `find()` +// is dead BY TYPE. +// +// ## What this suite pins +// +// The ruling's three cases, verbatim, plus the one it left open: +// +// (a) an `afterFind` that assigns `ctx.result = { records: [...] }` +// ⇒ refused with `FIND_HOOK_RESULT_NOT_ARRAY`; +// (b) an `afterFind` that mutates rows in place, or reassigns a DIFFERENT +// ARRAY ⇒ still returns an array — SHAPING STAYS LEGAL, which is the +// half that keeps this from being a behaviour regression; +// (c) the no-hook path unchanged; +// (d) `undefined` / `null` — not named by the ruling, decided here and +// argued in `find-hook-result-shape.ts`: refused, same code. +// +// ⚠️ (b) is written against the container SHAPE, never identity: a handler that +// builds a brand-new array (`ctx.result = rows.map(…)`) is doing exactly what +// ADR-0077 line 71 means by "shape reads", and a check comparing against +// `opCtx.result` — or freezing, or cloning — would break it. `Array.isArray` is +// the whole predicate, and the reassign-a-different-array case below is what +// holds it to that. + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { + FIND_HOOK_RESULT_NOT_ARRAY_CODE, + FIND_HOOK_RESULT_NOT_ARRAY_STATUS, + FindHookResultNotArrayError, +} from './find-hook-result-shape.js'; + +function silentLogger() { + const logger: any = { + trace() {}, debug() {}, info() {}, warn() {}, error() {}, fatal() {}, + child() { return logger; }, + }; + return logger; +} + +/** Two stored rows, so "the array survived" is observable as a length. */ +const ROWS = [ + { id: 't1', name: 'first', done: false }, + { id: 't2', name: 'second', done: true }, +]; + +function makeDriver() { + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find() { return ROWS.map((r) => ({ ...r })); }, + async findOne() { return { ...ROWS[0] }; }, + async create(_o: string, data: any) { return data; }, + async update(_o: string, id: string, data: any) { return { id, ...data }; }, + async updateMany() { return 0; }, + async delete() { return true; }, + async deleteMany() { return 0; }, + async count() { return ROWS.length; }, + async bulkCreate(_o: string, rows: any[]) { return rows; }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + }; + return driver; +} + +async function makeEngine() { + const engine = new ObjectQL({ logger: silentLogger() }); + engine.registerDriver(makeDriver(), true); + await engine.init(); + engine.registry.registerObject({ + name: 'task', + fields: { + id: { name: 'id', type: 'text', primaryKey: true, readonly: true }, + name: { name: 'name', type: 'text' }, + done: { name: 'done', type: 'boolean' }, + }, + } as any, 'test'); + return engine; +} + +/** Run and return whatever came out — a value or the thrown error. */ +async function outcomeOf(run: () => Promise): Promise<{ value?: unknown; error?: any }> { + try { + return { value: await run() }; + } catch (error) { + return { error }; + } +} + +describe('#15823 (c) — the no-hook path is unchanged', () => { + it('find() with no hooks registered still answers the bare array', async () => { + const engine = await makeEngine(); + const out = await engine.find('task', {}); + expect(Array.isArray(out)).toBe(true); + expect(out).toHaveLength(2); + expect(out.map((r: any) => r.id)).toEqual(['t1', 't2']); + }); +}); + +describe('#15823 (a) — an afterFind that REPLACES the container is refused', () => { + it('assigning ctx.result = { records: [...] } is refused with FIND_HOOK_RESULT_NOT_ARRAY', async () => { + const engine = await makeEngine(); + engine.registerHook('afterFind', (ctx: any) => { + ctx.result = { records: [{ id: 'ENVELOPE' }] }; + }, { object: 'task' } as any); + + const { value, error } = await outcomeOf(() => engine.find('task', {})); + + // The defect shape: it must NOT come back as a value at all. + expect(value, 'find() answered instead of refusing').toBeUndefined(); + expect(error, 'find() did not refuse').toBeInstanceOf(FindHookResultNotArrayError); + // ADR-0112 envelope: the code AND the status, never a bare `toThrow()`. + expect(error.code).toBe(FIND_HOOK_RESULT_NOT_ARRAY_CODE); + expect(error.code).toBe('FIND_HOOK_RESULT_NOT_ARRAY'); + expect(error.status).toBe(FIND_HOOK_RESULT_NOT_ARRAY_STATUS); + // The ruling: "the message names the hook event and the object". + expect(error.message).toContain('afterFind'); + expect(error.message).toContain('task'); + // The observed shape is named, so the author is not left guessing which + // handler did it or what it produced. + expect(error.event).toBe('afterFind'); + expect(error.object).toBe('task'); + expect(error.observed).toBe('object'); + }); + + it('the refusal fires BEFORE maskSecretFields / stripSearchCompanionFromRead see it', async () => { + // Both consumers assume the array and run on `hookContext.result` between + // the dispatch and the return; the ruling puts the refusal ahead of them + // precisely because they already assume what it now enforces. Driven, not + // asserted about source: a handler that replaces the container with an + // object carrying a poisoned `length` would make an array-assuming consumer + // iterate garbage if either ran first. + const engine = await makeEngine(); + let poisonRead = 0; + engine.registerHook('afterFind', (ctx: any) => { + ctx.result = { + get length() { poisonRead += 1; return 3; }, + get 0() { poisonRead += 1; return { id: 'x' }; }, + }; + }, { object: 'task' } as any); + + const { error } = await outcomeOf(() => engine.find('task', {})); + expect(error?.code).toBe(FIND_HOOK_RESULT_NOT_ARRAY_CODE); + expect(poisonRead, 'a consumer walked the replaced container before the refusal').toBe(0); + }); +}); + +describe('#15823 (b) — SHAPING STAYS LEGAL', () => { + it('an afterFind that mutates rows IN PLACE still returns the array', async () => { + const engine = await makeEngine(); + engine.registerHook('afterFind', (ctx: any) => { + for (const row of ctx.result) delete row.name; + }, { object: 'task' } as any); + + const out: any = await engine.find('task', {}); + expect(Array.isArray(out)).toBe(true); + expect(out).toHaveLength(2); + expect('name' in out[0]).toBe(false); + expect(out[0].id).toBe('t1'); + }); + + it('an afterFind that reassigns a DIFFERENT array still returns the array', async () => { + // The identity half of the predicate, and the reason it is `Array.isArray` + // and nothing cleverer: this handler replaces the container object, and + // that is legal because the container is still an array. + const engine = await makeEngine(); + engine.registerHook('afterFind', (ctx: any) => { + ctx.result = ctx.result.map((r: any) => ({ id: r.id })); + }, { object: 'task' } as any); + + const out: any = await engine.find('task', {}); + expect(Array.isArray(out)).toBe(true); + expect(out).toEqual([{ id: 't1' }, { id: 't2' }]); + }); + + it('an afterFind that reassigns an EMPTY array still returns the array', async () => { + // Filtering rows down to none is shaping, not container replacement — and + // `[]` is exactly the value a lenient guard written as a truthiness check + // would have refused by accident. + const engine = await makeEngine(); + engine.registerHook('afterFind', (ctx: any) => { ctx.result = []; }, { object: 'task' } as any); + + const out: any = await engine.find('task', {}); + expect(Array.isArray(out)).toBe(true); + expect(out).toEqual([]); + }); +}); + +describe('#15823 (d) — the case the ruling did not name: undefined / null', () => { + // Decided here, argued in the module: a handler that assigns neither is not + // "replacing the container with an envelope", but it is equally not an array, + // so `find()`'s declared `Promise` is broken exactly as much. The + // supported way for an `afterFind` to refuse a read is to THROW from the + // handler; clearing the result is not a second spelling of that. + for (const [label, value] of [['undefined', undefined], ['null', null]] as const) { + it(`assigning ctx.result = ${label} is refused with the same code`, async () => { + const engine = await makeEngine(); + engine.registerHook('afterFind', (ctx: any) => { ctx.result = value; }, { object: 'task' } as any); + + const { value: answered, error } = await outcomeOf(() => engine.find('task', {})); + expect(answered, 'find() answered instead of refusing').toBeUndefined(); + expect(error?.code).toBe(FIND_HOOK_RESULT_NOT_ARRAY_CODE); + expect(error.observed).toBe(label); + expect(error.message).toContain('afterFind'); + expect(error.message).toContain('task'); + }); + } + + it('a string result is refused too — the predicate is Array.isArray, not typeof', async () => { + const engine = await makeEngine(); + engine.registerHook('afterFind', (ctx: any) => { ctx.result = 'rows'; }, { object: 'task' } as any); + + const { error } = await outcomeOf(() => engine.find('task', {})); + expect(error?.code).toBe(FIND_HOOK_RESULT_NOT_ARRAY_CODE); + expect(error.observed).toBe('string'); + }); +}); + +describe('#15823 — the refusal is registered ADR-0112 vocabulary', () => { + it('the code is a member of the generated ErrorCode union', async () => { + const { ErrorCode } = await import('@objectstack/spec/api'); + expect(ErrorCode.safeParse(FIND_HOOK_RESULT_NOT_ARRAY_CODE).success).toBe(true); + // Control: the union really does reject an unregistered spelling, so the + // assertion above is a reading rather than a schema that accepts anything. + expect(ErrorCode.safeParse('FIND_HOOK_RESULT_NOT_AN_ARRAY').success).toBe(false); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 333fce6857..630e3c67b6 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -92,6 +92,7 @@ import { } from './summary-aggregate.js'; import { ReadonlyFieldRejectedError } from './readonly-strict-errors.js'; import { HookTargetRebindError } from './hook-target-rebind-errors.js'; +import { FindHookResultNotArrayError } from './find-hook-result-shape.js'; import { DriverConnectError, DatasourceUnavailableError, @@ -9551,6 +9552,26 @@ export class ObjectQL implements IObjectQLEngine { hookContext.result = result; await this.triggerHooks('afterFind', hookContext); + // [#15823] `find()` GUARANTEES the array, so the one seam that can + // break the declaration is closed here. An `afterFind` handler may + // SHAPE a read — mutate rows, drop keys, filter rows out, assign a + // different ARRAY — but replacing the container is a hook-contract + // violation, refused loudly rather than returned as an envelope a + // caller's `Promise` says cannot occur (ruled 2026-09-06). + // + // ⛔ Placement is load-bearing and not the `return`: `maskSecretFields` + // and `stripSearchCompanionFromRead` below BOTH assume the array and + // both run on `hookContext.result`, so the check has to precede them — + // otherwise the first consumer to walk a replaced container is the + // one that reports the problem, from the wrong place. + if (!Array.isArray(hookContext.result)) { + throw new FindHookResultNotArrayError({ + object, + event: 'afterFind', + result: hookContext.result, + }); + } + // Never let secret-field plaintext (or its ref) leave through the // generic read path — mask after hooks run. Privileged consumers use // resolveSecret() against the stored ref instead. diff --git a/packages/objectql/src/find-hook-result-shape.ts b/packages/objectql/src/find-hook-result-shape.ts new file mode 100644 index 0000000000..30f7cd3ffc --- /dev/null +++ b/packages/objectql/src/find-hook-result-shape.ts @@ -0,0 +1,179 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#15823] The refusal that closes `find()`'s one unguarded seam: an + * `afterFind` handler that REPLACES the read's container instead of shaping + * what is inside it. + * + * ## What used to happen + * + * `ObjectQL.find` declares `Promise` and, on the hook path, ended with + * `return hookContext.result` — with nothing between the `afterFind` dispatch + * and that return re-checking the value against the declared array. A handler + * assigning `ctx.result = { records: [ … ] }` therefore made a `find()` declared + * to resolve to an array resolve to an envelope, silently: no throw, no + * diagnostic, no log. Measured on a real engine over a real SQL driver — + * `ARRAY(len=1)` with no hooks, `OBJECT{records}` with the handler. + * + * Of the four `return hookContext.result` sites in `engine.ts` this is the only + * one with a concrete declared shape to violate; `findOne`, `update` and + * `delete` all declare `Promise` and carry no enforceable declaration at + * all. That is a separate question about those declarations and is deliberately + * NOT answered here. + * + * ## Why a refusal rather than a wider declaration + * + * The fork was real and pointed both ways — either the engine guarantees the + * array, or `find()`'s declaration is wrong and the ~70 array-or-envelope + * normalizer limbs the #15094 census counted are load-bearing rather than dead. + * The maintainer ruled the first (2026-09-06): the protocol is the baseline and + * the declaration IS the contract, so the seam that can break it is closed + * rather than the contract widened. `packages/spec/src/data/hook.zod.ts` backs + * it — a read is ONE event regardless of shape — as does ADR-0077 line 71, whose + * "intercept or shape reads" means shaping rows INSIDE the array, not replacing + * the array. + * + * The consequence the ruling records: every array-or-envelope normalizer limb + * downstream of `find()` is now dead BY TYPE. A reviewer asked to delete one no + * longer has to establish reachability — "reachable in principle" is answered + * `no` by the engine. + * + * ## The predicate is `Array.isArray`, and nothing cleverer + * + * SHAPING STAYS LEGAL. A handler may mutate rows in place, drop keys, filter + * rows away, or build a brand-new array — `ctx.result = ctx.result.map(…)` is + * the ordinary spelling of "shape a read" and must keep working. So the check + * is on the container SHAPE and never on identity: comparing the value against + * the one the engine put there, freezing it, or cloning it would each refuse a + * legitimate reshaping. What is refused is exactly one thing — the container + * stops being an array. + * + * ## `undefined` / `null` are refused too — decided here, not by the ruling + * + * The ruling names the envelope case. A handler that assigns `undefined` (or + * `null`) is not "replacing the container with an envelope", but it is equally + * not an array, and it breaks `Promise` exactly as much: the caller gets + * a value its type says cannot occur, and the failure lands at a call site far + * from the handler. Admitting it would leave a second hole beside the one being + * closed, in the same slot, with no way to tell the two apart from the outside. + * A read that should answer nothing answers `[]`; a handler that wants to REFUSE + * a read throws from the handler, which is the supported spelling and already + * how every other hook guard says no. So one predicate covers every non-array, + * and {@link describeFindHookResult} is what makes the refusal say WHICH one it + * saw. + * + * ## Why `500`, and what that costs + * + * The request was well-formed and authorized; a hook this deployment installed + * broke the engine's declared contract. There is nothing the caller can change + * and nothing to retry — the definition of a 5xx, and the reason this is not + * `400` like its `MultiUpdateHookKeyDivergenceError` neighbour (whose remedy + * genuinely belongs to the caller) nor `403` like `HookUnscopedDataAccessError` + * (an authorization answer). + * + * ⚠️ Stated rather than discovered later: a DECLARED 5xx has its prose withheld + * at the HTTP doors (`declaresServerFault` in `packages/types/src/error-leak.ts`; + * `rest-server.ts` and `dispatcher-plugin.ts` both replace the message with + * `INTERNAL_ERROR_MESSAGE`), so an HTTP caller reads "Internal server error" + * plus this code. That is the right split rather than a loss: the code is the + * machine-readable half and crosses intact because it is REGISTERED, while the + * message below is addressed to the HOOK'S AUTHOR, who meets it in-process at + * the `find()` call and in the server log — not to the client. The alternative, + * declaring a 4xx to keep the prose on the wire, would tell the caller its + * request was at fault, which is false. + * + * ## Why a REGISTERED ADR-0112 code, not an `ERR_`-prefixed operational one + * + * Same call, and for the same reason, as `MULTI_UPDATE_HOOK_KEY_DIVERGENCE_CODE` + * one file over: the whole value of this refusal is that it is RECOGNISED. An + * unregistered spelling is demoted off `error.code` at every door + * (`resolveThrownHttpError`) and rides `declaredCode` instead, which is the + * wrong channel for the one code a host has to branch on to find its own + * misbehaving handler. + */ + +/** + * The wire code, registered in the spec's `ERROR_CODE_LEDGER` under + * `@objectstack/objectql`. ONE code, ONE wording — every non-array shape + * answers this, and which shape it was rides the message and + * {@link FindHookResultNotArrayError.observed}, which is where ADR-0112 D3/D4 + * put detail rather than growing the closed `code` vocabulary. + */ +export const FIND_HOOK_RESULT_NOT_ARRAY_CODE = 'FIND_HOOK_RESULT_NOT_ARRAY' as const; + +/** `500` — see the module note: a server-side handler broke a server-side contract. */ +export const FIND_HOOK_RESULT_NOT_ARRAY_STATUS = 500 as const; + +/** + * A one-word name for what the handler left in `ctx.result`, used in the + * message and exposed on the error. + * + * `'array'` is included so the function is total and the caller cannot hand a + * legal value to the error by mistake; the engine never constructs the error on + * that branch. + */ +export function describeFindHookResult(value: unknown): string { + if (Array.isArray(value)) return 'array'; + if (value === null) return 'null'; + if (value === undefined) return 'undefined'; + return typeof value; +} + +/** + * The ADR-0112 envelope `find()` raises when its `afterFind` dispatch returned + * with `hookContext.result` no longer an array. + * + * Thrown at the seam — immediately after `triggerHooks('afterFind', …)` and + * BEFORE `maskSecretFields` / `stripSearchCompanionFromRead`, both of which + * already assume the array — so the diagnosis names the handler that did it + * instead of surfacing as a `TypeError` at one of the ~140 `find()` call sites + * that trusted the declaration. + */ +export class FindHookResultNotArrayError extends Error { + override readonly name = 'FindHookResultNotArrayError'; + readonly code = FIND_HOOK_RESULT_NOT_ARRAY_CODE; + readonly status = FIND_HOOK_RESULT_NOT_ARRAY_STATUS; + /** The object being read. */ + readonly object: string; + /** The hook event whose dispatch the replacement was observed after. */ + readonly event: string; + /** What the handler left behind — {@link describeFindHookResult}. */ + readonly observed: string; + /** The remedy half, addressed to the hook's author rather than to a user. */ + readonly developerMessage: string; + + constructor(info: { object: string; event: string; result: unknown }) { + const observed = describeFindHookResult(info.result); + super(buildMessage(info.object, info.event, observed)); + this.object = info.object; + this.event = info.event; + this.observed = observed; + this.developerMessage = + `'find()' declares 'Promise' and roughly 140 call sites in this repository read it as ` + + `an array without checking. A '${info.event}' handler may SHAPE a read — mutate rows in place, ` + + `drop keys, filter rows out, or assign a different ARRAY built from them — but replacing the ` + + `container itself is refused, because the declaration is the contract (ADR-0077 line 71 means ` + + `shaping rows inside the array, and 'hook.zod.ts' makes a read one event regardless of shape). ` + + `To answer no rows, assign '[]'. To REFUSE the read, throw from the handler — that is the ` + + `supported way for a '${info.event}' guard to say no. To hand the caller a different ` + + `structure, do it in the caller, not in the hook. Branch on ` + + `\`code === '${FIND_HOOK_RESULT_NOT_ARRAY_CODE}'\` (ADR-0112) to detect this.`; + } +} + +/** + * The user-facing sentence. + * + * ⛔ It must not begin with a SQL verb — `@objectstack/rest`'s importer runs row + * errors through `sanitizeRowError`, whose SQL backstop replaces any message + * STARTING with `insert`/`update`/`delete` with generic text. The same + * constraint `DuplicateRecordError` and `MultiUpdateHookKeyDivergenceError` + * record, for the same reason. + */ +function buildMessage(object: string, event: string, observed: string): string { + return ( + `Refusing the read on '${object}': its '${event}' handler replaced 'ctx.result' with ` + + `${observed === 'undefined' || observed === 'null' ? observed : `a ${observed}`}, and 'find()' ` + + `guarantees an array. Shaping the rows is supported; replacing the container is not.` + ); +} diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index 8fc9bc1efc..2efb89b599 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -144,6 +144,20 @@ export { MULTI_UPDATE_HOOK_KEY_DIVERGENCE_STATUS, divergingHookPayloadKeys, } from './multi-update-hook-key-divergence.js'; +// [#15823] Thrown by `engine.find` when its `afterFind` dispatch returned with +// `ctx.result` no longer an array — the refusal that makes `find()`'s declared +// `Promise` enforceable at the one seam that could break it. Exported +// for the same reason as its neighbour above: the remedy belongs to the HOOK'S +// AUTHOR, who needs to NAME the condition, and `code === +// 'FIND_HOOK_RESULT_NOT_ARRAY'` is the boundary-crossing identity. +// `describeFindHookResult` rides along because it is the whole vocabulary of +// the `observed` field a consumer would otherwise re-derive. +export { + FindHookResultNotArrayError, + FIND_HOOK_RESULT_NOT_ARRAY_CODE, + FIND_HOOK_RESULT_NOT_ARRAY_STATUS, + describeFindHookResult, +} from './find-hook-result-shape.js'; // [#14010] `Hook.runAs` — the declared execution identity of a hook's `ctx.api` // data operations. The refusal a `runAs: 'user'` hook raises when its trigger // resolved no user (ADR-0112 code + status), the api that raises it, and the diff --git a/packages/spec/src/api/error-code-ledger.zod.ts b/packages/spec/src/api/error-code-ledger.zod.ts index c2cf7c7e27..ef4c66ae91 100644 --- a/packages/spec/src/api/error-code-ledger.zod.ts +++ b/packages/spec/src/api/error-code-ledger.zod.ts @@ -556,6 +556,23 @@ export const ERROR_CODE_LEDGER = { // been written (`TransactionUnsupportedError`, `transaction-errors.ts`; // ADR-0119 D1/D4 fail-closed posture). Same #8087-gate family. 'ERR_TRANSACTION_UNSUPPORTED', + // [#15823] an `afterFind` handler REPLACED `ctx.result` with something that + // is not an array, breaking the one `return hookContext.result` site in + // `engine.ts` that has a concrete declared shape to violate + // (`find(): Promise`; `findOne`/`update`/`delete` all declare + // `Promise`). Refused at the seam — immediately after the dispatch and + // ahead of `maskSecretFields` / `stripSearchCompanionFromRead`, which both + // already assume the array — rather than surfacing as a `TypeError` at one + // of ~140 call sites. SHAPING stays legal: mutating rows, dropping keys, + // filtering rows out and assigning a different ARRAY are all untouched; + // only the container is protected. Registered rather than left as an `ERR_` + // operational code for the same reason as `MULTI_UPDATE_HOOK_KEY_DIVERGENCE` + // below — a host has to RECOGNISE this to find its own misbehaving handler, + // and an unregistered spelling demotes off `error.code` at every door. Not + // a synonym of any standard member: the request is valid and authorized, + // and the fault is a server-side extension's, not the caller's. + // `FindHookResultNotArrayError`, `find-hook-result-shape.ts`. + 'FIND_HOOK_RESULT_NOT_ARRAY', // [#14010] a hook declared `runAs: 'user'` and its trigger resolved NO user // (an `isSystem` plugin/service write, a system-elevated flow node), so its // `ctx.api` data operation has no identity to scope to and is REFUSED From 6776c6822e6d6d9323277c09e5ac7c3e14b80a0a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 05:31:34 +0000 Subject: [PATCH 2/4] test(plugin-auth): the #15597 control now asserts the refusal, not the envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #15823 closes the seam this control drove: an `afterFind` assigning a non-array no longer produces an envelope at the fourteen real reads, it produces `FIND_HOOK_RESULT_NOT_ARRAY`. The control keeps both of its jobs — the mechanism is driven on every block and the refusal asserted, and `expectBareArray`'s discrimination is now checked directly, since no engine can hand it an envelope any more. #15597's own conclusion is untouched: the fourteen limbs were removed on the argument that they were right to remove even given an open seam. Also fixes the refusal message's article (`a object` -> `an object`) and pins it. Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ Co-authored-by: Claude --- .changeset/find-afterfind-array-guard.md | 22 +++++ content/docs/references/api/contract.mdx | 3 +- .../docs/references/api/error-code-ledger.mdx | 1 + .../src/engine-find-hook-result-shape.test.ts | 7 ++ .../objectql/src/find-hook-result-shape.ts | 11 ++- .../src/find-envelope-limb-removal.test.ts | 97 ++++++++++++++----- 6 files changed, 115 insertions(+), 26 deletions(-) create mode 100644 .changeset/find-afterfind-array-guard.md diff --git a/.changeset/find-afterfind-array-guard.md b/.changeset/find-afterfind-array-guard.md new file mode 100644 index 0000000000..b9b8ad18da --- /dev/null +++ b/.changeset/find-afterfind-array-guard.md @@ -0,0 +1,22 @@ +--- +"@objectstack/objectql": minor +"@objectstack/spec": minor +--- + +`ObjectQL.find()` now guarantees the array it declares: an `afterFind` hook that replaces the result container is refused with `FIND_HOOK_RESULT_NOT_ARRAY`. + +`find()` is declared `Promise`, but on the hook path it returned `hookContext.result` with nothing re-checking the value after the `afterFind` dispatch. A handler assigning `ctx.result = { records: [ … ] }` therefore made a read declared to resolve to an array resolve to an envelope instead — silently, with no throw, no diagnostic and no log, while roughly 140 call sites read the answer as an array on the strength of the declaration. + +The engine now refuses that, immediately after the `afterFind` dispatch and ahead of the two consumers that already assume the array (secret-field masking and the `__search` companion strip). The refusal is a named error, `FindHookResultNotArrayError`, carrying the registered ADR-0112 code `FIND_HOOK_RESULT_NOT_ARRAY` and HTTP `500`; its message names the hook event and the object, and `developerMessage` carries the remedy. + +**Shaping stays legal, and nothing about it changes.** A handler may still mutate rows in place, delete keys, filter rows out, or assign a *different array* built from them — `Array.isArray` is the whole predicate, deliberately, so that `ctx.result = ctx.result.map(…)` keeps working. Only the container is protected. + +What to do if this refusal fires: + +- to answer no rows, assign `[]`; +- to refuse the read, `throw` from the handler — the supported way for any hook guard to say no; +- to hand a caller a different structure, build it in the caller, not in the hook. + +`@objectstack/spec` widens by one member: `FIND_HOOK_RESULT_NOT_ARRAY` joins `ERROR_CODE_LEDGER` under `@objectstack/objectql`, so the generated `ErrorCode` union — and therefore `ApiErrorSchema.code` — accepts it. Additive: no existing code is removed or renamed. + +Scope: this closes the one `return hookContext.result` site in the engine with a concrete declared shape to violate. `findOne`, `update` and `delete` declare `Promise` and carry no enforceable declaration; that is a separate question about those declarations and is deliberately not answered here. diff --git a/content/docs/references/api/contract.mdx b/content/docs/references/api/contract.mdx index 5eb57e6560..bb91d3ec42 100644 --- a/content/docs/references/api/contract.mdx +++ b/content/docs/references/api/contract.mdx @@ -27,7 +27,7 @@ const result = ApiErrorSchema.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **code** | `Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| 'INVALID_FORMAT' \| 'VALUE_TOO_LONG' \| 'VALUE_TOO_SHORT' \| 'VALUE_OUT_OF_RANGE' \| … +295 more>` | ✅ | Error code (e.g. VALIDATION_ERROR; StandardErrorCode ∪ the ledger the serving side registers — ERROR_CODE_LEDGER for framework packages) | +| **code** | `Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| 'INVALID_FORMAT' \| 'VALUE_TOO_LONG' \| 'VALUE_TOO_SHORT' \| 'VALUE_OUT_OF_RANGE' \| … +296 more>` | ✅ | Error code (e.g. VALIDATION_ERROR; StandardErrorCode ∪ the ledger the serving side registers — ERROR_CODE_LEDGER for framework packages) | | **declaredCode** | `string` | optional | The producer-declared code, verbatim, when it is not a member of the closed `code` vocabulary — the open, author-authored channel (app-specific spellings; ADR-0112) | | **message** | `string` | ✅ | Readable error message | | **userMessage** | `string` | optional | Producer-marked user-facing refusal text, verbatim. Present exactly when the producer opted in at throw time; consumers render it to end users and keep their generic substitution for anything unmarked. Status-agnostic; never replaces `message`. | @@ -180,6 +180,7 @@ const result = ApiErrorSchema.parse(data); * `FILE_NOT_FOUND` * `FILTER_TOKEN_UNKNOWN` * `FILTER_TOKEN_UNRESOLVED` +* `FIND_HOOK_RESULT_NOT_ARRAY` * `FLOW_CONVERSION_CONFLICT` * `FLOW_DISABLED` * `FLOW_FAILED` diff --git a/content/docs/references/api/error-code-ledger.mdx b/content/docs/references/api/error-code-ledger.mdx index 6d85208aae..b7ba7f976d 100644 --- a/content/docs/references/api/error-code-ledger.mdx +++ b/content/docs/references/api/error-code-ledger.mdx @@ -296,6 +296,7 @@ const result = ErrorCode.parse(data); * `FILE_NOT_FOUND` * `FILTER_TOKEN_UNKNOWN` * `FILTER_TOKEN_UNRESOLVED` +* `FIND_HOOK_RESULT_NOT_ARRAY` * `FLOW_CONVERSION_CONFLICT` * `FLOW_DISABLED` * `FLOW_FAILED` diff --git a/packages/objectql/src/engine-find-hook-result-shape.test.ts b/packages/objectql/src/engine-find-hook-result-shape.test.ts index 9e73808340..e8f7251eb3 100644 --- a/packages/objectql/src/engine-find-hook-result-shape.test.ts +++ b/packages/objectql/src/engine-find-hook-result-shape.test.ts @@ -143,6 +143,13 @@ describe('#15823 (a) — an afterFind that REPLACES the container is refused', ( expect(error.event).toBe('afterFind'); expect(error.object).toBe('task'); expect(error.observed).toBe('object'); + // The article is computed, not hard-coded: `a object` shipped once in this + // module's first draft and was caught by driving it. + expect(error.message).toContain("replaced 'ctx.result' with an object"); + // The remedy half is addressed to the handler's author, and names both + // supported spellings — `[]` for no rows, a throw to refuse the read. + expect(error.developerMessage).toContain("assign '[]'"); + expect(error.developerMessage).toContain('throw from the handler'); }); it('the refusal fires BEFORE maskSecretFields / stripSearchCompanionFromRead see it', async () => { diff --git a/packages/objectql/src/find-hook-result-shape.ts b/packages/objectql/src/find-hook-result-shape.ts index 30f7cd3ffc..ede0117732 100644 --- a/packages/objectql/src/find-hook-result-shape.ts +++ b/packages/objectql/src/find-hook-result-shape.ts @@ -171,9 +171,14 @@ export class FindHookResultNotArrayError extends Error { * record, for the same reason. */ function buildMessage(object: string, event: string, observed: string): string { + // `undefined` / `null` name themselves; everything else takes an article, and + // `object` is the one that needs `an` — the shape this refusal exists for. + const what = + observed === 'undefined' || observed === 'null' + ? observed + : `${'aeiou'.includes(observed[0]) ? 'an' : 'a'} ${observed}`; return ( - `Refusing the read on '${object}': its '${event}' handler replaced 'ctx.result' with ` + - `${observed === 'undefined' || observed === 'null' ? observed : `a ${observed}`}, and 'find()' ` + - `guarantees an array. Shaping the rows is supported; replacing the container is not.` + `Refusing the read on '${object}': its '${event}' handler replaced 'ctx.result' with ${what}, ` + + `and 'find()' guarantees an array. Shaping the rows is supported; replacing the container is not.` ); } diff --git a/packages/plugins/plugin-auth/src/find-envelope-limb-removal.test.ts b/packages/plugins/plugin-auth/src/find-envelope-limb-removal.test.ts index e3b1bbc77f..4051f5fe33 100644 --- a/packages/plugins/plugin-auth/src/find-envelope-limb-removal.test.ts +++ b/packages/plugins/plugin-auth/src/find-envelope-limb-removal.test.ts @@ -29,10 +29,12 @@ * * `expect(Array.isArray(x)).toBe(true)` is the kind of assertion that can pass * because nothing could have made it fail. It could have here: `ObjectQL.find` - * returns `hookContext.result` on its hook path, so an `afterFind` handler CAN - * replace the result with an envelope — measured, not supposed (see the control - * case at the bottom, which drives exactly that and asserts every pin above it - * goes red). That is also the reason removal is right rather than merely safe: + * returned `hookContext.result` on its hook path, so an `afterFind` handler + * COULD replace the result with an envelope — measured, not supposed. Since + * #15823 the engine refuses that replacement (`FIND_HOOK_RESULT_NOT_ARRAY`), so + * the control case at the bottom now drives the same handler and asserts the + * REFUSAL, and checks `expectBareArray`'s discrimination directly. That is also + * the reason removal is right rather than merely safe: * a hook that corrupted `find()` into `{ records }` would be a contract * violation, and the limb did not repair it — it silently absorbed it at these * fourteen sites while this package's remaining `find()` call sites broke anyway @@ -47,7 +49,7 @@ */ import { describe, it, expect, afterEach } from 'vitest'; -import { ObjectQL } from '@objectstack/objectql'; +import { FIND_HOOK_RESULT_NOT_ARRAY_CODE, ObjectQL } from '@objectstack/objectql'; import { SqlDriver } from '@objectstack/driver-sql'; import { AuthManager } from './auth-manager.js'; import { authIdentityObjects } from './manifest.js'; @@ -289,14 +291,36 @@ describe('#15597 — the control: these pins CAN go red', () => { * The discrimination check for all fourteen cases above, and the reason the * removal argument is about hooks rather than about drivers. * - * `ObjectQL.find` ends with `return hookContext.result` on its hook path, so - * an `afterFind` handler that assigns a non-array makes `find()` resolve to - * one. Nothing in this tree does that — the only registered `afterFind` in - * the repo is plugin-audit's read recorder, which never touches `ctx.result` - * — but the mechanism EXISTS, which is what makes `expectBareArray` a real - * assertion instead of a tautology. + * ## What this control asserted before #15823, and why it changed + * + * `ObjectQL.find` used to end with `return hookContext.result` on its hook + * path with nothing re-checking the value, so an `afterFind` handler + * assigning a non-array made `find()` resolve to one — and this case drove + * exactly that, asserting every pin above it went red. That was a real + * measurement, and it is what made `expectBareArray` an assertion rather than + * a tautology. + * + * #15823 ruled that seam shut (maintainer, 2026-09-06, direction 1): + * `find()` GUARANTEES the array, and a handler that replaces the container is + * refused at the engine with `FIND_HOOK_RESULT_NOT_ARRAY`. So the mutation + * below no longer produces an envelope at these fourteen sites — it produces + * a refusal, which is a STRONGER statement of the same fact and is asserted + * as such here. + * + * ⛔ #15597's own conclusion is untouched by that. Its argument was that the + * fourteen limbs were right to remove *even given* an open seam — fourteen + * sites of false immunity being worse than one visible failure — and closing + * the seam only removes the "reachable in principle" caveat a reviewer used + * to have to pay. The limbs stay removed for the reason they were removed. + * + * ## The two jobs, kept + * + * (1) the mechanism is really gone — driven, on every one of the fourteen + * real reads, through their real production entry points; and (2) + * `expectBareArray` really discriminates — asserted directly, since no engine + * can hand it an envelope any more. */ - it('an afterFind hook that returns an envelope turns every shape pin red', async () => { + it('an afterFind hook that returns an envelope is now REFUSED at every one of the fourteen reads', async () => { const engine = await bootEngine(); await seedAll(engine); @@ -308,20 +332,49 @@ describe('#15597 — the control: these pins CAN go red', () => { { packageId: 'test.15597-control' }, ); - const survivors: string[] = []; + /** Blocks that answered a value instead of refusing — must be empty. */ + const answered: Array<{ id: string; value: unknown }> = []; + /** Blocks that refused with some OTHER error — must be empty. */ + const otherError: Array<{ id: string; code: unknown; message: string }> = []; + const refused: string[] = []; + for (const block of BLOCKS) { - const value = await block.populated(engine); - // Under the mutation the read really does answer an envelope… - expect(Array.isArray(value), `${block.id}: the hook did not take effect`).toBe(false); - // …and the pin's own assertion rejects it. try { - expectBareArray(value, block.id); - survivors.push(block.id); - } catch { - /* expected: the pin discriminates */ + answered.push({ id: block.id, value: await block.populated(engine) }); + } catch (error) { + const code = (error as { code?: unknown } | null)?.code; + if (code === FIND_HOOK_RESULT_NOT_ARRAY_CODE) { + refused.push(block.id); + // The refusal names the seam it is about — the ruling's own + // requirement, checked here on a real production read rather than + // only on the engine's own unit harness. + expect((error as Error).message, `${block.id}: message does not name the event`).toContain('afterFind'); + } else { + otherError.push({ id: block.id, code, message: String((error as Error)?.message) }); + } } } - expect(survivors, 'these pins passed on an envelope — they do not discriminate').toEqual([]); + + expect(answered, 'a read answered under the replaced container instead of refusing').toEqual([]); + expect(otherError, 'a read refused for some other reason — this control is measuring the wrong thing').toEqual([]); + expect(refused, 'every block must refuse').toEqual(BLOCKS.map((b) => b.id)); + }); + + it('expectBareArray still discriminates — it rejects the envelope the engine can no longer produce', () => { + // Job (2), now that no engine can supply the input: the helper the fourteen + // pins call is exercised against the exact shape the removed limbs claimed + // to defend against, plus the `{ data }` spelling one of them used. Without + // this the pins above would once again be assertions nothing was ever seen + // to fail. + for (const envelope of [{ records: [{ id: 'ENVELOPE' }] }, { data: [{ id: 'ENVELOPE' }] }, null, 'rows']) { + expect( + () => expectBareArray(envelope, 'control'), + `expectBareArray accepted ${JSON.stringify(envelope)} — it does not discriminate`, + ).toThrow(); + } + // …and passes the shape it is meant to pass, so the check above is not + // simply a helper that throws on everything. + expect(() => expectBareArray([{ id: 'r1' }], 'control')).not.toThrow(); }); }); From 7dd55b9f19f67869537eec0bef55bd8b1a6468fb Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 06:32:16 +0000 Subject: [PATCH 3/4] chore: re-anchor the system-context census and pay the spec load at module top Two gate repairs the guard's diff owes: - `check:system-context-census` reads DOC anchors keyed by LINE NUMBER, and the 21 lines the guard adds (plus the import) shifted fourteen of them in `content/docs/permissions/system-context.mdx`. Repaired with the script's own `--fix`; line numbers only, nothing semantic. Measured green at the merge base first, so the rot is this branch's. - `check:test-source-alias` refuses a first module load paid inside a clocked test body: the new pin's `await import('@objectstack/spec/api')` moves to a module-top import, so the transform is paid during collection. Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ Co-authored-by: Claude --- content/docs/permissions/system-context.mdx | 24 +++++++++---------- .../src/engine-find-hook-result-shape.test.ts | 8 +++++-- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 5ac5488fff..43dcbc40a4 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -109,17 +109,17 @@ that silently does not happen. | # | Behaviour when `isSystem` | Package | What you get / what you lose | Anchor | |:--|:---|:---|:---|:---| -| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:11733` | -| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11916` | -| 20 | **`readonly` strip bypassed — INSERT** | objectql | Same, on create — one gate over BOTH create-side passes since the 2026-09-03 ruling moved the static-`readonly` strip in beside the runtime-owned one and deleted the DataProtocol ingress copy. `isSystem` is the **only** exemption on this path: `preserveAudit` is deliberately not read on create, so a non-system historical import is still stripped | `objectql/src/engine.ts:10381` | -| 21 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:10514`, `readonly-strict-errors.ts:66` | -| 22 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:6240` | -| 23 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3920`, `:3930`, `:3957` | +| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:11754` | +| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11937` | +| 20 | **`readonly` strip bypassed — INSERT** | objectql | Same, on create — one gate over BOTH create-side passes since the 2026-09-03 ruling moved the static-`readonly` strip in beside the runtime-owned one and deleted the DataProtocol ingress copy. `isSystem` is the **only** exemption on this path: `preserveAudit` is deliberately not read on create, so a non-system historical import is still stripped | `objectql/src/engine.ts:10402` | +| 21 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:10535`, `readonly-strict-errors.ts:66` | +| 22 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:6241` | +| 23 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3921`, `:3931`, `:3958` | | 24 | Engine-owned / append-only write guard bypassed | plugin-security | Get: generic writes to `managedBy` engine-owned objects | `system-write-guard.ts:96`, `:120` | | 25 | Identity write guard bypassed (ADR-0092) | plugin-auth | Get: direct writes to identity tables through the generic data path | `identity-write-guard.ts:99` | -| 26 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6939` | -| 27 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:12535` | -| 28 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:12464` | +| 26 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6940` | +| 27 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:12556` | +| 28 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:12485` | | 29 | **Bulk data event `organizationId` OMITTED** — the batch is published "not asserted" | plugin-security | Get: nothing — the `data.records.*` event still publishes. Lose: the per-organization attribution: this exit is taken before the security middleware composes any tenant wall, so it records no Layer 0 verdict on the operation (`OperationContext.tenantLayer0Verdict`, #15813), and the engine's bulk producer — which reads that recorded verdict and nothing else — omits the key rather than filling it from the caller's `tenantId`; a tenant-scoped consumer then does not deliver the event inside an organization wall (#15225) | `security-plugin.ts:1686` | ### 3. Sharing (`plugin-sharing`) @@ -179,8 +179,8 @@ a reader tracing where elevation travels needs them. | # | Site | Package | What it does | |:--|:---|:---|:---| -| 62 | `objectql/src/engine.ts:3727` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | -| 63 | `objectql/src/engine.ts:14981` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | +| 62 | `objectql/src/engine.ts:3728` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | +| 63 | `objectql/src/engine.ts:15002` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | | 64 | `plugin-reports/src/report-service.ts:556` | plugin-reports | Threads the flag into the engine call that runs a report | | 65 | `body-runner.ts:279` | runtime | Rebuilds an `ExecutionContext` from a hook session, carrying the flag across | @@ -195,7 +195,7 @@ assuming `isSystem` covers it is a documented source of bugs. |:---|:---|:---| | "It suppresses triggers / record-change automation" | **No.** Only `skipTriggers` does. A bare `{ isSystem: true }` on a seed write re-fired automation on freshly seeded rows and wedged first boot | `metadata-protocol/src/seed-loader.ts:2032` (rationale at `:1942`–`1944`, #3760), `flow.zod.ts:743` | | "It skips the state machine" | **No.** That is `skipStateMachine`, carried by seed replay and by `treatAsHistorical` imports | `objectql/src/engine.ts` FSM gate; see [State Machine](/docs/protocol/objectql/state-machine) | -| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:10364`–`10381` | +| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:10385`–`10402` | | "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1590` (#3493 / #6640) | | "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280`–`281` | | "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:299` | diff --git a/packages/objectql/src/engine-find-hook-result-shape.test.ts b/packages/objectql/src/engine-find-hook-result-shape.test.ts index e8f7251eb3..13f6a4de4f 100644 --- a/packages/objectql/src/engine-find-hook-result-shape.test.ts +++ b/packages/objectql/src/engine-find-hook-result-shape.test.ts @@ -44,6 +44,11 @@ // holds it to that. import { describe, it, expect } from 'vitest'; +// [check:test-source-alias] Module-top, not `await import(...)` inside the case +// below: objectql resolves this specifier through `dist/`, so a first load paid +// inside a test body transforms that whole module graph while `testTimeout` is +// running. Collection is clocked against nothing; test bodies are. +import { ErrorCode } from '@objectstack/spec/api'; import { ObjectQL } from './engine.js'; import { FIND_HOOK_RESULT_NOT_ARRAY_CODE, @@ -246,8 +251,7 @@ describe('#15823 (d) — the case the ruling did not name: undefined / null', () }); describe('#15823 — the refusal is registered ADR-0112 vocabulary', () => { - it('the code is a member of the generated ErrorCode union', async () => { - const { ErrorCode } = await import('@objectstack/spec/api'); + it('the code is a member of the generated ErrorCode union', () => { expect(ErrorCode.safeParse(FIND_HOOK_RESULT_NOT_ARRAY_CODE).success).toBe(true); // Control: the union really does reject an unregistered spelling, so the // assertion above is a reading rather than a schema that accepts anything. From 5c8f04199e90162fe3a44539a259bf3e872dd881 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 06:56:20 +0000 Subject: [PATCH 4/4] Merge origin/main into claude/issue-15823-find-afterfind-array-guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `main` moved onto three of this branch's files (engine.ts, objectql/index.ts and the system-context census page). Landed through `scripts/pm/os-regen-merge.sh` so the os-regen-driven artifact is regenerated from the MERGED tree rather than text-merged: the driver merges those paths at exit 0 while silently keeping one side, and only a regeneration exposes it. The one artifact the pre-commit hook held the merge for — `content/docs/permissions/system-context.mdx` — is regenerated here with `pnpm gen:system-context-census` (15 anchors re-derived against the merged engine.ts; line numbers only). Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ Co-authored-by: Claude --- content/docs/permissions/system-context.mdx | 28 ++++++++++----------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 43dcbc40a4..d6dbd2a5f7 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -109,17 +109,17 @@ that silently does not happen. | # | Behaviour when `isSystem` | Package | What you get / what you lose | Anchor | |:--|:---|:---|:---|:---| -| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:11754` | -| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11937` | -| 20 | **`readonly` strip bypassed — INSERT** | objectql | Same, on create — one gate over BOTH create-side passes since the 2026-09-03 ruling moved the static-`readonly` strip in beside the runtime-owned one and deleted the DataProtocol ingress copy. `isSystem` is the **only** exemption on this path: `preserveAudit` is deliberately not read on create, so a non-system historical import is still stripped | `objectql/src/engine.ts:10402` | -| 21 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:10535`, `readonly-strict-errors.ts:66` | -| 22 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:6241` | -| 23 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3921`, `:3931`, `:3958` | +| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:11767` | +| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11950` | +| 20 | **`readonly` strip bypassed — INSERT** | objectql | Same, on create — one gate over BOTH create-side passes since the 2026-09-03 ruling moved the static-`readonly` strip in beside the runtime-owned one and deleted the DataProtocol ingress copy. `isSystem` is the **only** exemption on this path: `preserveAudit` is deliberately not read on create, so a non-system historical import is still stripped | `objectql/src/engine.ts:10415` | +| 21 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:10548`, `readonly-strict-errors.ts:66` | +| 22 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:6253` | +| 23 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3930`, `:3940`, `:3967` | | 24 | Engine-owned / append-only write guard bypassed | plugin-security | Get: generic writes to `managedBy` engine-owned objects | `system-write-guard.ts:96`, `:120` | | 25 | Identity write guard bypassed (ADR-0092) | plugin-auth | Get: direct writes to identity tables through the generic data path | `identity-write-guard.ts:99` | -| 26 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6940` | -| 27 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:12556` | -| 28 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:12485` | +| 26 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6952` | +| 27 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:12569` | +| 28 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:12498` | | 29 | **Bulk data event `organizationId` OMITTED** — the batch is published "not asserted" | plugin-security | Get: nothing — the `data.records.*` event still publishes. Lose: the per-organization attribution: this exit is taken before the security middleware composes any tenant wall, so it records no Layer 0 verdict on the operation (`OperationContext.tenantLayer0Verdict`, #15813), and the engine's bulk producer — which reads that recorded verdict and nothing else — omits the key rather than filling it from the caller's `tenantId`; a tenant-scoped consumer then does not deliver the event inside an organization wall (#15225) | `security-plugin.ts:1686` | ### 3. Sharing (`plugin-sharing`) @@ -160,10 +160,10 @@ The largest single consumer — **17 of the 105 sites**. | 49 | Action `requiredPermissions` bypassed | runtime | Get: engine self-invocation runs any action | `action-execution.ts:401` | | 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:5319`, `:6766`, `:7014`, `:7445`, `:7638` | | 51 | The shared metadata-write verdict itself returns `allowed` | metadata-core | Get: the one function all of row 50's doors consult answers yes before any capability is examined | `meta-write-capability.ts:134` | -| 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:421`, `domains/ai.ts:60`, `domains/automation.ts:989`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:543`, `external-datasource-routes.ts:302`, `package-routes.ts:97` | +| 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:421`, `domains/ai.ts:60`, `domains/automation.ts:989`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:552`, `external-datasource-routes.ts:302`, `package-routes.ts:97` | | 53 | MCP principal check satisfied | runtime | Get: MCP surface reachable with no user | `domains/mcp.ts:61` | | 54 | Package REST route capability gate bypassed | rest | Get: package read/write over REST without `manage_metadata` / `studio.access` / `setup.access` | `package-routes.ts:102` | -| 55 | Package domain capability gates bypassed | runtime | Get: package management and package-inventory reads without the capability | `domains/packages.ts:241`, `:274` | +| 55 | Package domain capability gates bypassed | runtime | Get: package management and package-inventory reads without the capability | `domains/packages.ts:250`, `:283` | | 56 | Activation write / authoring refusals do not fire | runtime | Get: activation artifacts writable and authorable without the activation-authoring capability | `activation-gate.ts:138`, `:189` | | 57 | Automation run-state read, flow-authoring write and unrelated-screen read all pass | runtime | Get: run state, flow writes and screen reads with no grant | `domains/automation.ts:254`, `:545`, `:635` | | 58 | Audience-binding suggestion recording skipped | plugin-security | Lose: install-time suggestions are not recorded for system callers | `suggested-audience-bindings.ts:703` | @@ -179,8 +179,8 @@ a reader tracing where elevation travels needs them. | # | Site | Package | What it does | |:--|:---|:---|:---| -| 62 | `objectql/src/engine.ts:3728` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | -| 63 | `objectql/src/engine.ts:15002` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | +| 62 | `objectql/src/engine.ts:3737` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | +| 63 | `objectql/src/engine.ts:15016` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | | 64 | `plugin-reports/src/report-service.ts:556` | plugin-reports | Threads the flag into the engine call that runs a report | | 65 | `body-runner.ts:279` | runtime | Rebuilds an `ExecutionContext` from a hook session, carrying the flag across | @@ -195,7 +195,7 @@ assuming `isSystem` covers it is a documented source of bugs. |:---|:---|:---| | "It suppresses triggers / record-change automation" | **No.** Only `skipTriggers` does. A bare `{ isSystem: true }` on a seed write re-fired automation on freshly seeded rows and wedged first boot | `metadata-protocol/src/seed-loader.ts:2032` (rationale at `:1942`–`1944`, #3760), `flow.zod.ts:743` | | "It skips the state machine" | **No.** That is `skipStateMachine`, carried by seed replay and by `treatAsHistorical` imports | `objectql/src/engine.ts` FSM gate; see [State Machine](/docs/protocol/objectql/state-machine) | -| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:10385`–`10402` | +| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:10398`–`10415` | | "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1590` (#3493 / #6640) | | "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280`–`281` | | "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:299` |