fix(metadata-protocol): an update by id answers the same refusal whether or not the id exists, as delete does

.changeset/harden-update-refusal-order.md

@@ -0,0 +1,15 @@
+---
+'@objectstack/metadata-protocol': patch
+---
+
+harden(data): an update by id answers the same refusal whether or not the id exists, as delete does
+
+Clause-②: no
+Metadata-protocol: none
+Skills-docs: none — no file under `skills/**` teaches what a by-id update answers for an id that names no record (a `git grep` of `skills/` for `RECORD_NOT_FOUND` names only the approvals record lock), so no skill line changes. The Update Record section of `content/docs/protocol/kernel/http-protocol.mdx` states the order.
+
+The by-id update doors now judge the update before they look the record up — the order the by-id delete door already has.
+
+- **What changes.** A caller the update is refused to — no edit permission on the object, or an external data source that refuses writes — now gets that refusal (`403 PERMISSION_DENIED`, `403 EXTERNAL_WRITE_FORBIDDEN`) for an id that names no record too, where it got `404 RECORD_NOT_FOUND`. This holds on `PATCH /api/v1/data/:object/:id`, on each row of `POST /api/v1/data/:object/updateMany` and of `POST /api/v1/data/:object/batch` with `operation: "update"`, and on every door that updates through them (the runtime dispatcher's `/data`, the `/mcp` HTTP tools, declarative update actions).
+- **What does not change.** A caller who may update the object still gets `404 RECORD_NOT_FOUND` for an id that names no record, and for a record its row-level access hides. Successful updates, the `405` for an operation the object does not expose, optimistic concurrency (`If-Match` / `expectedVersion`), upsert and system-context writes answer as before.
+- **Embedders calling the protocol directly.** `updateData`, `updateManyData` and `batchData`'s `update` operation no longer read the record before calling `engine.update`. They ask whether it exists only after the engine resolves a `null` readback, so an engine that resolves `null` for an id that names no record still gets `404 RECORD_NOT_FOUND`, and an update that moved its record out of the caller's row scope still answers `200` with `record: null`.

content/docs/protocol/kernel/http-protocol.mdx

@@ -713,6 +713,14 @@ field is left unchanged.
 > lacks edit permission on is *rejected* with `403 PermissionDeniedError`, not
 > stripped.
 
+**Refusals come before the record lookup:**
+The update is judged before the record is looked up, as a delete is. A caller who may
+not update the object (no edit permission, or an external data source that refuses
+writes) gets the same `403` whether or not `{record_id}` names a record, and so does
+each row of `updateMany` and of `batch` with `operation: "update"`. A caller who may
+update the object gets `404 RECORD_NOT_FOUND` for an id that names no record, and for
+a record its row-level access hides.
+
 ### Delete Record
 
 Delete a record by ID.

packages/metadata-protocol/src/protocol-batch-data.ts

@@ -311,15 +311,16 @@ export async function runBatchDataLoop(
                 }
                 case 'update': {
                     if (!record.id) throw rowRequiredIdError('update');
-                    // [#5088] Same existence gate as `updateMany` and the
-                    // single-record PATCH — a row naming no record must not
-                    // enter the write pipeline, where #4770's stored ⊕
-                    // payload merge has no stored side and #4775 blames the
-                    // hook for the resulting gap.
-                    await this.assertRecordExists(object, record.id);
                     // [#3455] Collect the engine's LEGAL write strips per row.
                     const dropped: DroppedFieldsEvent[] = [];
                     const updated = byIdUpdateRecord(await this.engine.update(object, record.data || {}, { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, ...ctxOpt } as any));
+                    // [#5088] Same existence answer as `updateMany` and the
+                    // single-record PATCH, and in the same place: AFTER the
+                    // engine has judged the write (its refusals, then its
+                    // not-found gate ahead of every hook), so a refused row
+                    // reads the same whether or not its id names a record —
+                    // see `assertRecordExists`.
+                    if (updated == null) await this.assertRecordExists(object, record.id);
                     omitInternalFieldsFromWriteResponse(batchSchema, updated); // [#7823]
                     results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) });
                     succeeded++;
@@ -335,8 +336,8 @@ export async function runBatchDataLoop(
                         // duplicate-keyed — an authorization/update
                         // scenario reported as a key collision — or, on a
                         // store without a unique id constraint, wrote a
-                        // second row. Same probe as the update branch above
-                        // and the single-record path (#4620): it asks the
+                        // second row. The same `probeRecord` the update branch
+                        // above asks after its write (#4620): it asks the
                         // database a fact, and whether the caller may WRITE
                         // the row it proves stays #1994's decision inside
                         // `engine.update`.
@@ -739,17 +740,19 @@ export async function runUpdateManyLoop(
             // handling — at best a 404 with `undefined` interpolated into
             // the message, at worst a where-clause with no id at all.
             if (!record.id) throw rowRequiredIdError('update');
-            // [#5088] Third gap, the same shape: no existence gate. A row
-            // naming no record went straight into `engine.update`, so the
-            // hook pipeline ran over a payload-only record and the row came
-            // back `INTERNAL_ERROR` from #4775's condition abort — blaming
-            // the hook for a caller's stale id. Probe first, per row, so
-            // this face answers what the single-record PATCH answers.
-            await this.assertRecordExists(object, record.id);
             const dropped: DroppedFieldsEvent[] = [];
             const opts: any = { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); } };
             if (context !== undefined) opts.context = context;
             const updated = byIdUpdateRecord(await this.engine.update(object, record.data || {}, opts));
+            // [#5088] Third gap, the same shape: no existence answer. A row
+            // naming no record came back `INTERNAL_ERROR` from #4775's
+            // condition abort — blaming the hook for a caller's stale id. It
+            // answers what the single-record PATCH answers now, asked where
+            // that door asks it: AFTER the engine has judged the write (its
+            // refusals, then its not-found gate ahead of every hook), so a
+            // refused row reads the same whether or not its id names a
+            // record — see `assertRecordExists`.
+            if (updated == null) await this.assertRecordExists(object, record.id);
             omitInternalFieldsFromWriteResponse(updateManySchema, updated); // [#7823]
             results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) });
             succeeded++;

packages/metadata-protocol/src/protocol.batch-row-conformance.test.ts

@@ -28,6 +28,7 @@ import { describe, it, expect, vi } from 'vitest';
 import { BatchOperationResultSchema, BatchUpdateResponseSchema } from '@objectstack/spec/api';
 import { ObjectStackProtocolImplementation } from './protocol.js';
 import { assertEngineUpdateDispatch, assertEngineDeleteDispatch, assertEngineFindOnePredicate } from '@objectstack/metadata-core';
+import { recordNotFoundError } from '@objectstack/core';
 
 const SCHEMA = { name: 'invoice', fields: { title: { name: 'title', type: 'text' } } };
 
@@ -56,11 +57,13 @@ function makeStoreEngine() {
             rows.set(rec.id, rec);
             return rec;
         }),
-        update: vi.fn(async (_object: string, data: any, options?: any) => {
+        update: vi.fn(async (object: string, data: any, options?: any) => {
             assertEngineUpdateDispatch(data, options);
             const id = options?.where?.id;
             const current = rows.get(id);
-            if (!current) throw new Error(`no such record: ${id}`);
+            // ObjectQL's by-id not-found gate: the engine refuses a row that is
+            // not there itself, after the write is judged and before any hook.
+            if (!current) throw recordNotFoundError(object, id);
             if (data?.title === POISON) throw new Error('update exploded');
             const next = { ...current, ...data };
             rows.set(id, next);
@@ -156,10 +159,10 @@ describe('batchData rows conform to BatchOperationResultSchema (#4793)', () => {
         expectConformantResponse(res, 2);
         expect(res.results[0].data).toBeUndefined();
         // [#5088] Was an unclassified engine throw (`no such record`, rendered
-        // INTERNAL_ERROR) because the row reached `engine.update` at all. The
-        // update branch now runs the same existence probe as the single-record
-        // PATCH, so a missing id is refused BEFORE the write with the catalogued
-        // 404 — see `protocol.bulk-record-not-found.test.ts`.
+        // INTERNAL_ERROR). The engine now refuses a missing id itself with the
+        // catalogued 404 — after it has judged the write, which is why the
+        // update branch asks nothing about existence ahead of it — see
+        // `protocol.bulk-record-not-found.test.ts`.
         expect(res.results[1].errors[0]).toMatchObject({ code: 'RECORD_NOT_FOUND', httpStatus: 404 });
         expect(res.results[1].errors[0].message).toMatch(/not found/);
     });