fix(objectql): gate by-id update/delete on record existence — 404 RECORD_NOT_FOUND instead of a 400 from further down the pipeline (#7867)

.changeset/action-body-write-not-found-gate.md

@@ -0,0 +1,93 @@
+---
+"@objectstack/objectql": patch
+"@objectstack/core": patch
+"@objectstack/metadata-protocol": patch
+"@objectstack/runtime": patch
+"@objectstack/plugin-audit": patch
+"@objectstack/plugin-auth": patch
+---
+
+fix(objectql): a by-id `update()`/`delete()` against a nonexistent record answers 404 `RECORD_NOT_FOUND` instead of a 400 from further down the pipeline (#7867)
+
+Nothing on the action-body write path ever asked whether the target row existed.
+`ctx.api.object(name).update({ id, … })` reached `ObjectQL.update()`'s by-id
+branch through `buildSandboxApi` → `ObjectRepository`, and that branch had **no
+existence gate at all**: `engine.update()` on a ghost id was a silent no-op that
+resolved `null`, so the write ran on into validation, the driver and the hook
+chain and died on whichever complained first.
+
+**Which one it died on varied with the object's declarations**, which is why the
+defect read as several unrelated bugs:
+
+- a **hooked** object → `400` `HookConditionError`, from an `afterUpdate`
+ condition reading `previous` on a row nobody read;
+- an **unhooked** object → `400` `VALIDATION_FAILED` "X is required", because
+ with no prior row a PATCH is validated as if it were a whole record.
+
+The 400 class varied; the missing 404 was the constant. Measured on one showcase
+stack, same id, same object, same second: `POST /actions/showcase_task/
+showcase_mark_done/<ghost>` answered 400 while `PATCH /data/showcase_task/
+<ghost>` answered 404. Both answer **404 `RECORD_NOT_FOUND`** now.
+
+`delete()` had the same shape and was the worse of the two: with no gate it
+reported success for a row that was never there, so a typo'd id, an
+already-deleted row and a real deletion were indistinguishable.
+
+**This is not a `previous`-binding bug.** `if (priorRecord) hookContext.previous
+= …` is correct and is untouched — ADR-0058 Addendum II / #4649 require that an
+absent row leave `previous` UNBOUND rather than fabricated. It was behaving
+correctly on a path that should never have been entered, so the fix removes the
+producer rather than specializing what it produced.
+
+**Where the gate went, and why there.** At the engine, in the by-id branches of
+`update()` and `delete()` — the one point all three action-body write faces
+funnel through (`ctx.api.object()`, its context-less repo-facade fallback, and
+`ctx.engine.update()`). A repository-level gate would have closed one of the
+three and made `ql.update(o, { id })` and `ctx.api.object(o).update({ id })`
+answer one ghost id two different ways. Two sibling paths already gated
+correctly — `protocol.updateData`/`deleteData` (#4435) and `callData`'s ObjectQL
+fallback (#5138) — and all three now throw the **same** `recordNotFoundError`,
+which moved to `@objectstack/core` so the engine can reach it without importing
+`@objectstack/metadata-protocol` (forbidden in the `/core` closure by ADR-0076
+D2's boundary ratchet). `@objectstack/metadata-protocol` re-exports it unchanged.
+
+Existence is asked with a pre-write read, never off the write's own result:
+`IDataDriver.update` declares no not-found signal, and the engine's post-write
+readback is `null` for a second reason (a write that moves the row out of the
+caller's row scope), so reading either would answer 404 to a write that landed.
+
+**Behaviour change worth knowing about — the by-id prior-row read is now
+unconditional.** #5284 (update) and #5929 (delete) had narrowed it to "does
+anything CONSUME the prior row?", skipping the read for objects with no hook, no
+prior-reading validation rule and no roll-up. Existence is a consumer that
+demand list never enumerated and the one consumer every by-id write has, and no
+cheaper question answers it — so the skip and the gate are mutually exclusive.
+The measured cost is small: #5929's own record enumerates the global hook
+registrants (plugin-sharing, service-storage, plugin-auth, plugin-audit), so on
+any kernel that loads them the demand was already true for every object and the
+narrowing skipped nothing. The read is genuinely new only for a bare
+`@objectstack/objectql/core` embedder — which is buying a 404 it did not have.
+
+Three read-count pins measured the old skip and now measure the read, each
+recording what changed and why at its own site: #5284's and #5929's in
+`packages/objectql`, and #5860's `sys_job_queue` case in `@objectstack/plugin-audit`.
+The DISPATCH half all three are actually about — the per-object `hasHooksFor`
+question, the `excludeObjects` subtraction, and the retired
+`sys_fetch_previous_*` builtins — is untouched and still pinned.
+
+One further case encoded the old silent no-op as correct: `@objectstack/plugin-auth`'s
+#5941 last-admin-guard test deleted a `sys_account` id that was never seeded and
+asserted it RESOLVED, to show the guard does not write-guard that object. It now
+deletes a REAL row — which states the same thing more strongly — and separately
+pins that a ghost id there is refused by the ENGINE rather than by the guard.
+
+**Scope.** By-id only. A `multi: true` predicate write matching zero rows still
+resolves "0 rows affected" — the same line both sibling paths draw.
+
+`@objectstack/runtime`: the sandbox error passthrough now also carries `status`
+alongside `code` and `fields`, so an error that names its own HTTP status keeps
+it across the QuickJS boundary. Without it the action surface answered the right
+diagnosis at the wrong status (`{ code: 'RECORD_NOT_FOUND', httpStatus: 400 }`);
+`domains/actions.ts` already honoured `.status` first — the number simply never
+arrived. A permission refusal thrown inside a body likewise keeps its 403 now
+instead of flattening to 400.

packages/core/src/index.ts

@@ -44,6 +44,12 @@ export * from './utils/migration-journal.js';
 // Export the runtime filter-placeholder resolver (framework#3582)
 export * from './utils/filter-tokens.js';
 
+// Export the shared single-record 404 (#4435/#5138, moved down here in #7867) —
+// the one `RECORD_NOT_FOUND` envelope `protocol.updateData`/`deleteData`,
+// `callData`'s ObjectQL fallback and the engine's own by-id write gate answer
+// with. `@objectstack/metadata-protocol` re-exports it from its original home.
+export * from './utils/record-not-found.js';
+
 // Export in-memory fallbacks for core-criticality services
 export * from './fallbacks/index.js';
 

packages/core/src/utils/record-not-found.ts

@@ -0,0 +1,64 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * [#4435] The 404 a single-record operation answers when the id names no row.
+ *
+ * Extracted so the READ and the two WRITE paths cannot disagree about it. They
+ * did: `getData` answered `404 RECORD_NOT_FOUND` while `updateData` returned
+ * `200 { record: null }` and `deleteData` returned `200 { success: true }` for
+ * any string in the path — so a typo'd id, an already-deleted row and a real
+ * deletion were indistinguishable, and a client PATCHing a concurrently deleted
+ * record was told its write had landed.
+ *
+ * That is the same silent-no-op shape the v17 train removed everywhere else
+ * this window (#4240/#4303/#4315 refuse missing fields, #4169 refuses unknown
+ * params, #4190 stopped dropping filters) — a write that touched zero rows
+ * reporting 200 is that shape one level up, on the verb where it costs the
+ * most.
+ *
+ * [#5138] EXPORTED, for the same "cannot disagree about it" reason one layer
+ * out. `@objectstack/runtime`'s `callData` is protocol-first with an ObjectQL
+ * FALLBACK, and the fallback had reinvented this fact three incompatible ways
+ * (`get` → `null`, `update` → a bare `Error` with no status ⇒ 500, `delete` →
+ * no check at all ⇒ `200 { deleted: true }` for a row that never existed). It
+ * now calls THIS function, so the two paths behind one `callData` answer a
+ * missing id identically — which is the only reason a caller may stop caring
+ * which of them served it. Re-spelling the envelope there would have been a
+ * second not-found envelope; `RECORD_NOT_FOUND` (#5088) is the one this repo
+ * has.
+ *
+ * ── [#7867] Why it lives in `@objectstack/core` and not where it was written ──
+ *
+ * Because the THIRD path that needed it could not reach the second one. An
+ * action body's `ctx.api.object(name).update({ id, … })` traverses neither
+ * `protocol.updateData` nor `callData`: it reaches `ObjectQL.update()`'s by-id
+ * branch directly, which had no existence gate at all, so a ghost id was a
+ * silent no-op that then died on whatever the pipeline complained about first
+ * (a `HookConditionError` 400 on a hooked object, a required-field
+ * `VALIDATION_FAILED` 400 on an unhooked one — the 400 class varied with the
+ * object's declarations; the missing 404 was the constant).
+ *
+ * The gate for that path belongs in the engine, and `packages/objectql` cannot
+ * import `@objectstack/metadata-protocol` where this function was written:
+ * ADR-0076 D2's boundary ratchet (`core-boundary.ratchet.test.ts`) forbids the
+ * whole `@objectstack/objectql/core` closure — `engine.ts` included — from
+ * pulling that package in. So the choice was a FOURTH spelling of the envelope
+ * or one home both layers already depend on. #5138's own sentence rules the
+ * first out, so this is the second: the factory moved down to the lowest
+ * package the three producers share, and `@objectstack/metadata-protocol`
+ * re-exports it unchanged for every existing importer.
+ *
+ * This is the same move `engineCanRollBack` made for the same reason — a fact
+ * two layers must agree on lives in the layer beneath both, not in a copy each.
+ */
+export function recordNotFoundError(object: string, id: string | number): Error {
+ const err = new Error(`Record ${id} not found in ${object}`) as Error & {
+ code?: string;
+ status?: number;
+ object?: string;
+ };
+ err.code = 'RECORD_NOT_FOUND';
+ err.status = 404;
+ err.object = object;
+ return err;
+}

packages/metadata-protocol/src/protocol.ts

@@ -3,7 +3,7 @@
 import type {
  DataProtocol, MetadataProtocol, PackageProtocol,
 } from '@objectstack/spec/api';
-import { IDataEngine, engineCanRollBack } from '@objectstack/core';
+import { IDataEngine, engineCanRollBack, recordNotFoundError } from '@objectstack/core';
 import { readEnvWithDeprecation, resolveTenancyPosture } from '@objectstack/types';
 // [#6285] ADR-0105 D1's authority on "does this deployment wall organizations?".
 // `resolveMultiOrgEnabled()` is DEMOTED and its own doc comment says answering
@@ -640,43 +640,22 @@ export function zodIssuesToMetadataIssues(issues: unknown): MetadataIssueEntry[]
 }
 
 /**
- * [#4435] The 404 a single-record operation answers when the id names no row.
- *
- * Extracted so the READ and the two WRITE paths cannot disagree about it. They
- * did: `getData` answered `404 RECORD_NOT_FOUND` while `updateData` returned
- * `200 { record: null }` and `deleteData` returned `200 { success: true }` for
- * any string in the path — so a typo'd id, an already-deleted row and a real
- * deletion were indistinguishable, and a client PATCHing a concurrently deleted
- * record was told its write had landed.
- *
- * That is the same silent-no-op shape the v17 train removed everywhere else
- * this window (#4240/#4303/#4315 refuse missing fields, #4169 refuses unknown
- * params, #4190 stopped dropping filters) — a write that touched zero rows
- * reporting 200 is that shape one level up, on the verb where it costs the
- * most.
- *
- * [#5138] EXPORTED, for the same "cannot disagree about it" reason one layer
- * out. `@objectstack/runtime`'s `callData` is protocol-first with an ObjectQL
- * FALLBACK, and the fallback had reinvented this fact three incompatible ways
- * (`get` → `null`, `update` → a bare `Error` with no status ⇒ 500, `delete` →
- * no check at all ⇒ `200 { deleted: true }` for a row that never existed). It
- * now calls THIS function, so the two paths behind one `callData` answer a
- * missing id identically — which is the only reason a caller may stop caring
- * which of them served it. Re-spelling the envelope there would have been a
- * second not-found envelope; `RECORD_NOT_FOUND` (#5088) is the one this repo
- * has.
+ * [#4435/#5138] The 404 a single-record operation answers when the id names no
+ * row — the repo's ONE `RECORD_NOT_FOUND` envelope.
+ *
+ * [#7867] The body moved to `@objectstack/core`
+ * (`utils/record-not-found.ts` — full provenance lives there); this is a
+ * re-export, so every existing importer of
+ * `@objectstack/metadata-protocol`'s `recordNotFoundError` is unchanged and
+ * the three producers still share one function object.
+ *
+ * ⛔ Do not re-declare it here. It moved because a THIRD producer needed it and
+ * could not reach this package: `ObjectQL.update()`/`delete()`'s by-id gate
+ * lives in `packages/objectql`, whose `/core` entry closure is forbidden by
+ * ADR-0076 D2's boundary ratchet from importing `@objectstack/metadata-protocol`
+ * at all. A local copy here would be the second spelling #5138 ruled out.
  */
-export function recordNotFoundError(object: string, id: string | number): Error {
- const err = new Error(`Record ${id} not found in ${object}`) as Error & {
- code?: string;
- status?: number;
- object?: string;
- };
- err.code = 'RECORD_NOT_FOUND';
- err.status = 404;
- err.object = object;
- return err;
-}
+export { recordNotFoundError };
 
 /**
  * A 400 for a `$filter` ARRAY that looks like a filter AST but is not one.

packages/objectql/src/engine-delete-dispatch.test.ts

@@ -35,7 +35,18 @@ function makeRecordingDriver() {
  supports: {},
  async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; },
  async find() { return []; },
- async findOne() { return null; },
+ // [#7867] Answers the row the by-id branch's not-found gate asks for.
+ // This used to be `return null`, which — now that a by-id update/delete
+ // refuses a ghost id with `RECORD_NOT_FOUND` — would make every by-id case
+ // in this file die at the gate and never reach the driver: a DOUBLE looser
+ // than the producer, hiding the very behaviour the file exists to observe
+ // (#4434/#4550's shape). It echoes back whatever id it was asked for, so it
+ // stays agnostic about the dispatch and can never make a `reject` case look
+ // like a `by-id` one.
+ async findOne(_o: string, ast: any) {
+ const id = ast?.where?.id;
+ return id === undefined || id === null ? null : { id, title: 'stored' };
+ },
  async create(_o: string, data: Record<string, unknown>) { return { id: 'r1', ...data }; },
  async update(_o: string, id: string, data: Record<string, unknown>) { return { id, ...data }; },
  async delete(_o: string, id: string) { calls.push({ fn: 'delete', arg: id }); return true; },