fix(services,metadata): bind the row the ingress resolved, not the row the payload names

.changeset/ingress-resolved-id-wins-over-payload-id.md

@@ -0,0 +1,42 @@
+---
+'@objectstack/service-settings': patch
+'@objectstack/metadata': patch
+---
+
+Make the settings engine facade and the metadata database loader bind the row
+they resolved, not a row the payload names
+
+Two ingresses resolved an authoritative row id and then folded it into the
+write payload with the **losing** spread order — `{ id, ...data }` — so a
+caller-supplied `data.id` spread over the id the ingress had just resolved and
+silently retargeted the write:
+
+- `wrapEngineAsSettingsEngine`'s by-id `update` branch
+ (`@objectstack/service-settings`), whose id comes from the caller's
+ `where.id`.
+- `DatabaseLoader._update` (`@objectstack/metadata`), whose id arrives as a
+ separate parameter every caller resolves first (`existing.id`, from the read
+ immediately above).
+
+Both now spell it `{ ...data, id }` — the operation's id **after** the spread,
+so it wins. That is the convention the repo's other two ingresses already
+document: `rest-server.ts`'s batch update arm ("the operation's id AFTER the
+spread, so it wins") and `protocol.updateData`'s #6479 fix
+(`{ ...request.data, id: request.id }`).
+
+**No wrong write is known to have been reachable.** Both sites' current callers
+build fresh field literals and never put an `id` inside `data`, so this is
+hardening a fragile pattern rather than repairing a measured defect. What makes
+it worth the three characters is that neither site can be caught downstream:
+both pass **no `where`** to the engine, so the payload is the only id the engine
+ever sees, and the engine's conflicting-id refusal (`UPDATE_ID_MISMATCH`, 400)
+needs two disagreeing declarations before it can fire. The fold is the entire
+trust boundary at both sites, and it is one refactor — a caller handing back a
+row copy, and rows carry `id` — from the #6479 shape.
+
+Both are pinned with a payload whose `id` names a **different** row than the
+one the ingress resolved, asserting the resolved row is still the row bound. A
+pin exercising a payload without an `id` would have passed against both
+spellings. The doubles answer "which row does this bind?" with the producer's
+own `assertEngineUpdateDispatch`, so they cannot be kinder about it than a
+running server.

packages/metadata/src/loaders/database-loader-update-id-fold-wins.test.ts

@@ -0,0 +1,149 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * [#11231] `DatabaseLoader._update` folds its authoritative `id` PARAMETER
+ * into the write payload — and that id outranks a payload naming another row.
+ *
+ * ## The shape
+ *
+ * `_update(table, id, data)` takes the row address as a separate parameter —
+ * every caller resolves it first (`existing.id`, from the `baseFilter(type,
+ * name)` read just above) and then hands it here. The method folds it into the
+ * payload and passes **no `where`** to the engine, so the payload is the only
+ * id the engine sees; the engine's conflicting-id refusal
+ * (`UPDATE_ID_MISMATCH`, 400 — #11142/#11230) needs two disagreeing
+ * declarations and therefore cannot cover this site. The fold IS the trust
+ * boundary.
+ *
+ * Spelled `{ id, ...data }` the fold LOSES: a `data.id` spreads over the
+ * resolved id and the write silently retargets to a row the loader never read
+ * and never version-checked. Spelled `{ ...data, id }` — the convention
+ * documented at `rest-server.ts`'s batch arm ("the operation's id AFTER the
+ * spread, so it wins") and at `protocol.updateData` (#6479) — the parameter
+ * wins, which is what a separate id parameter MEANS.
+ *
+ * ## Why this drives `_update` directly
+ *
+ * Today's in-repo callers (`save`'s update arm, `registerRollback`) build a
+ * fresh field literal — `{ metadata, version, checksum, updated_at, state }` —
+ * so none of them can currently produce the conflict, and no public path can
+ * reach it. That is exactly what makes this a fragile-pattern pin rather than
+ * a repro: the guarded fact is the METHOD's contract, "the id parameter names
+ * the row", which a future caller passing a row copy (rows carry `id`) would
+ * otherwise break silently. Pinning it through a caller that cannot express
+ * the conflict would pin nothing.
+ *
+ * ## Why every case carries a CONFLICT
+ *
+ * A payload with no `id` produces the same write under BOTH spellings — such a
+ * case passes against the defect and measures nothing. Each case below hands
+ * `_update` a payload whose `id` names a DIFFERENT row than the id parameter.
+ *
+ * The bound row is not re-derived here: the double's `update` asks
+ * `assertEngineUpdateDispatch`, the predicate `ObjectQL.update` itself
+ * dispatches on, so it cannot be kinder or stricter than a running server
+ * (#4550/#5480, the contract `check:engine-double-contract` keeps).
+ */
+
+import { describe, expect, it } from 'vitest';
+import { assertEngineUpdateDispatch } from '@objectstack/metadata-core';
+import type { IDataEngine } from '@objectstack/spec/contracts';
+import { DatabaseLoader } from './database-loader.js';
+
+/** The row the loader resolved and passed as the `id` parameter. */
+const RESOLVED = 'meta_resolved';
+/** The row a payload `id` claims instead. Never the row that should be written. */
+const CLAIMED = 'meta_claimed';
+
+interface SeenUpdate {
+ objectName: string;
+ data: Record<string, unknown>;
+ dispatch: ReturnType<typeof assertEngineUpdateDispatch>;
+}
+
+function makeRecordingEngine(seen: SeenUpdate[]): IDataEngine {
+ return {
+ async update(
+ objectName: string,
+ data: Record<string, unknown>,
+ options?: Record<string, unknown>,
+ ) {
+ const dispatch = assertEngineUpdateDispatch(data, options);
+ seen.push({ objectName, data, dispatch });
+ return { ...data };
+ },
+ } as unknown as IDataEngine;
+}
+
+/** Reach the private fold the way the loader's own callers do. */
+function updateVia(
+ loader: DatabaseLoader,
+ table: string,
+ id: string,
+ data: Record<string, unknown>,
+): Promise<Record<string, unknown>> {
+ return (
+ loader as unknown as {
+ _update(t: string, i: string, d: Record<string, unknown>): Promise<Record<string, unknown>>;
+ }
+ )._update(table, id, data);
+}
+
+describe('[#11231] DatabaseLoader._update — the id parameter outranks a payload id', () => {
+ it('binds the row named by the id parameter, not the row the payload claims', async () => {
+ const seen: SeenUpdate[] = [];
+ const loader = new DatabaseLoader({ engine: makeRecordingEngine(seen) });
+
+ await updateVia(loader, 'sys_metadata', RESOLVED, {
+ id: CLAIMED,
+ version: 7,
+ state: 'active',
+ });
+
+ expect(seen).toHaveLength(1);
+ const [call] = seen;
+
+ // The load-bearing assertion: which row the engine BINDS. On the losing
+ // spread order this reads `CLAIMED`.
+ expect(call.dispatch).toEqual({ kind: 'by-id', id: RESOLVED });
+ expect(call.data.id).toBe(RESOLVED);
+ expect(call.data.id).not.toBe(CLAIMED);
+ });
+
+ it('keeps the payload’s other fields while overriding only its id', async () => {
+ const seen: SeenUpdate[] = [];
+ const loader = new DatabaseLoader({ engine: makeRecordingEngine(seen) });
+
+ await updateVia(loader, 'sys_metadata', RESOLVED, {
+ id: CLAIMED,
+ metadata: '{"a":1}',
+ version: 7,
+ checksum: 'abc',
+ state: 'active',
+ });
+
+ expect(seen[0].objectName).toBe('sys_metadata');
+ expect(seen[0].data).toEqual({
+ id: RESOLVED,
+ metadata: '{"a":1}',
+ version: 7,
+ checksum: 'abc',
+ state: 'active',
+ });
+ });
+
+ it('writes the resolved row for the history table too', async () => {
+ const seen: SeenUpdate[] = [];
+ const loader = new DatabaseLoader({ engine: makeRecordingEngine(seen) });
+
+ // The fold is per-call, not per-table: the same method serves every table
+ // the loader writes, so the guarantee cannot be table-specific.
+ await updateVia(loader, 'sys_metadata_history', RESOLVED, {
+ id: CLAIMED,
+ event_seq: 3,
+ });
+
+ expect(seen[0].objectName).toBe('sys_metadata_history');
+ expect(seen[0].dispatch).toEqual({ kind: 'by-id', id: RESOLVED });
+ });
+});

packages/metadata/src/loaders/database-loader.ts

@@ -273,7 +273,14 @@ export class DatabaseLoader implements MetadataLoader {
 
  private async _update(table: string, id: string, data: Record<string, unknown>): Promise<Record<string, unknown>> {
  if (this.engine) {
- return this.engine.update(table, { id, ...data });
+ // [#11231] The resolved id AFTER the spread, so it WINS — the same
+ // convention as `rest-server.ts`'s batch arm and `protocol.updateData`
+ // (#6479). No `where` is passed, so the payload is the only id the
+ // engine sees and its conflicting-id refusal (`UPDATE_ID_MISMATCH`) has
+ // nothing to compare against: a `data.id` spread over the id parameter
+ // would retarget the write to a row no caller resolved. The separate
+ // `id` parameter is the row address — do not let a payload outrank it.
+ return this.engine.update(table, { ...data, id });
  }
  return this.driver!.update(table, id, data);
  }

packages/services/service-settings/src/settings-engine-id-fold-wins.test.ts

@@ -0,0 +1,157 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * [#11231] The settings engine facade folds the id it RESOLVED into the write
+ * payload — and that id outranks a payload naming a different row.
+ *
+ * ## The shape
+ *
+ * `wrapEngineAsSettingsEngine`'s `update` is an ingress: it reads the row
+ * address out of the caller's `where.id`, then hands ObjectQL a payload with
+ * that id folded in. On that branch it passes **no `where`** to the engine, so
+ * the payload is the ONLY id the engine ever sees — which is also why the
+ * engine's conflicting-id refusal (`UPDATE_ID_MISMATCH`, 400 — #11142/#11230)
+ * cannot cover this site: a refusal needs two declarations to disagree, and
+ * the fold leaves exactly one. The fold IS the trust boundary here.
+ *
+ * Spelled `{ id, ...data }` the fold LOSES — a caller-supplied `data.id`
+ * spreads over the resolved id and silently retargets the write to a row the
+ * ingress never resolved, never authorised and never read. Spelled
+ * `{ ...data, id }` — the convention the repo's other two ingresses already
+ * document (`rest-server.ts`'s batch arm, "the operation's id AFTER the
+ * spread, so it wins", and `protocol.updateData`'s #6479 fix) — the resolved
+ * id wins.
+ *
+ * ## Why every case below carries a CONFLICT
+ *
+ * A payload with no `id` in it produces the same write under BOTH spellings,
+ * so a case shaped that way passes against the defect and measures nothing.
+ * Each case here hands the facade a payload whose `id` names a DIFFERENT row
+ * than `where.id` and asserts the engine still binds the resolved one. That is
+ * the ingress-level fact a future refactor would break — a caller handing back
+ * a row copy (rows carry `id`) is the whole population this guards.
+ *
+ * ## The bound row is not re-derived here
+ *
+ * The double's `update` asks `assertEngineUpdateDispatch` — the predicate
+ * `ObjectQL.update` itself dispatches on — which row a call binds, so it can
+ * be neither kinder nor stricter than a running server about which id wins
+ * (#4550/#5480, and the contract `check:engine-double-contract` keeps).
+ */
+
+import { describe, expect, it } from 'vitest';
+import { assertEngineUpdateDispatch } from '@objectstack/objectql';
+import type { IDataEngine } from '@objectstack/spec/contracts';
+import { wrapEngineAsSettingsEngine } from './settings-service-plugin.js';
+
+/** The row the ingress resolved from `where.id` — the only legitimate target. */
+const RESOLVED = 'sys_setting_resolved';
+/** The row a payload `id` claims instead. Never the row that should be written. */
+const CLAIMED = 'sys_setting_claimed';
+
+interface SeenUpdate {
+ objectName: string;
+ data: Record<string, unknown>;
+ options: Record<string, unknown> | undefined;
+ /** Which row the REAL engine would bind for this call. */
+ dispatch: ReturnType<typeof assertEngineUpdateDispatch>;
+}
+
+/**
+ * An engine double that records the call and answers "which row?" with the
+ * producer's verdict rather than a hand-written re-reading of the ladder.
+ */
+function makeRecordingEngine(seen: SeenUpdate[]): IDataEngine {
+ return {
+ async update(
+ objectName: string,
+ data: Record<string, unknown>,
+ options?: Record<string, unknown>,
+ ) {
+ const dispatch = assertEngineUpdateDispatch(data, options);
+ seen.push({ objectName, data, options, dispatch });
+ return { ...data };
+ },
+ } as unknown as IDataEngine;
+}
+
+describe('[#11231] wrapEngineAsSettingsEngine — the resolved id outranks a payload id', () => {
+ it('binds the row `where.id` resolved, not the row the payload claims', async () => {
+ const seen: SeenUpdate[] = [];
+ const wrapped = wrapEngineAsSettingsEngine(makeRecordingEngine(seen));
+
+ await wrapped.update('sys_setting', {
+ where: { id: RESOLVED },
+ data: { id: CLAIMED, value: 'rotated' },
+ });
+
+ expect(seen).toHaveLength(1);
+ const [call] = seen;
+
+ // The load-bearing assertion: the row the engine BINDS. On the losing
+ // spread order this reads `CLAIMED`.
+ expect(call.dispatch).toEqual({ kind: 'by-id', id: RESOLVED });
+
+ // …and the payload the facade actually handed over carries the resolved
+ // id, so nothing downstream of the engine can re-derive the claimed one.
+ expect(call.data.id).toBe(RESOLVED);
+ expect(call.data.id).not.toBe(CLAIMED);
+ });
+
+ it('keeps the payload’s other fields while overriding only its id', async () => {
+ const seen: SeenUpdate[] = [];
+ const wrapped = wrapEngineAsSettingsEngine(makeRecordingEngine(seen));
+
+ await wrapped.update('sys_setting', {
+ where: { id: RESOLVED },
+ data: { id: CLAIMED, value: 'rotated', updated_by: 'admin' },
+ });
+
+ // The fix overrides the id and nothing else — a fold that dropped caller
+ // fields would be a different defect wearing this one's fix.
+ expect(seen[0].data).toEqual({
+ id: RESOLVED,
+ value: 'rotated',
+ updated_by: 'admin',
+ });
+ });
+
+ it('forwards context and bypassTenantAudit alongside the winning id (#8030)', async () => {
+ const seen: SeenUpdate[] = [];
+ const wrapped = wrapEngineAsSettingsEngine(makeRecordingEngine(seen));
+
+ await wrapped.update('sys_setting', {
+ where: { id: RESOLVED },
+ data: { id: CLAIMED, value_enc: 'new-handle' },
+ bypassTenantAudit: true,
+ context: { isSystem: true },
+ });
+
+ expect(seen[0].dispatch).toEqual({ kind: 'by-id', id: RESOLVED });
+ // The by-id branch still forwards both driver options — the #8030 fix is
+ // on the same three lines this card edits, so it is pinned beside it.
+ expect(seen[0].options).toEqual({
+ bypassTenantAudit: true,
+ context: { isSystem: true },
+ });
+ });
+
+ it('leaves the multi branch addressing by `where`, with no id folded in', async () => {
+ const seen: SeenUpdate[] = [];
+ const wrapped = wrapEngineAsSettingsEngine(makeRecordingEngine(seen));
+
+ // The settings row write takes this branch in practice: its `where` is the
+ // composite (namespace, key, scope, user_id) and carries no id at all.
+ await wrapped.update('sys_setting', {
+ where: { namespace: 'mail', key: 'smtp_host', scope: 'global' },
+ data: { value: 'smtp.example.com' },
+ });
+
+ expect(seen[0].dispatch).toEqual({ kind: 'multi' });
+ expect(seen[0].data).not.toHaveProperty('id');
+ expect(seen[0].options).toMatchObject({
+ where: { namespace: 'mail', key: 'smtp_host', scope: 'global' },
+ multi: true,
+ });
+ });
+});

packages/services/service-settings/src/settings-service-plugin.ts

@@ -466,7 +466,14 @@ export function wrapEngineAsSettingsEngine(engine: IDataEngine): SettingsEngine
  };
  const id = (where as any)?.id;
  if (id !== undefined && id !== null) {
- return eng.update(objectName, { id, ...data }, driverOpts);
+ // [#11231] The operation's id AFTER the spread, so it WINS — the same
+ // convention as `rest-server.ts`'s batch arm and `protocol.updateData`
+ // (#6479). This branch passes no `where` to the engine, so the payload
+ // is the only id the engine sees and its conflicting-id refusal
+ // (`UPDATE_ID_MISMATCH`) has nothing to compare against: a
+ // caller-supplied `data.id` spread over the resolved id would retarget
+ // the write silently. Do not flip this back to `{ id, ...data }`.
+ return eng.update(objectName, { ...data, id }, driverOpts);
  }
  return eng.update(objectName, data, {
  where,