fix(objectql): a preserveAudit by-id update no longer hands the primary key to the driver

.changeset/preserveaudit-primary-key-never-preservable.md

@@ -0,0 +1,5 @@
+---
+'@objectstack/objectql': patch
+---
+
+`preserveAudit` no longer reinstates a record's own primary key (#8215). `isPreservableUnderAudit` now excludes `id` before consulting the whitelist, so a historical-import (`preserveAudit`) by-id update stops handing `SET id = 'rec_1' WHERE id = 'rec_1'` to the driver — a write that is a no-op on SQL but an outright rejection on stores with immutable primary keys. The REST ingress folds the path id into every update body, so bulk historical imports hit this without ever sending an `id` themselves. The flag keeps doing its actual job: the audit/timestamp family and author-declared business `readonly` fields (`closed_at`, `resolved_by`, …) are still preserved. On the insert side the shared predicate now also strips a caller-seeded `autonumber` primary key under `preserveAudit`; business `autonumber` identifiers (`account_number`, …) remain preservable.

content/docs/protocol/objectql/state-machine.mdx

@@ -110,7 +110,7 @@ transitions: {
 - Only a rule with `severity: 'error'` (the default) blocks the write; `warning`/`info` are logged.
 - **Seed writes are exempt** (#3433). Curated seed data — package bootstrap fixtures, marketplace templates, per-org replay, all loaded by `SeedLoaderService` — is a snapshot of established facts, not a record walking its lifecycle, so it bypasses the `state_machine` rule entirely: a seed may be born mid-lifecycle (a `completed` project, a `closed_won` opportunity) and neither `initialStates` (insert) nor `transitions` (update) is enforced. Every *other* validation still runs, so a seed must still satisfy field shape, `format`, `script`, and the rest. `os lint` warns when a seeded value is not a state the machine declares, so a typo is still caught before boot.
 - **A "historical" data import is exempt too** (#3479). Migrating established facts — a batch of already-`closed` tickets, `closed_won` deals — is the same "snapshot, not a lifecycle event" situation. Set `treatAsHistorical: true` on the import request (default **off**) and the runner puts `skipStateMachine` on the write context, so `initialStates` doesn't reject those mid-lifecycle rows. A normal import leaves it off and still walks the FSM — the strict behavior is the default, so the exemption is always an explicit opt-in.
-- **`treatAsHistorical` also preserves the original audit timeline** (#3493) — **on the rows an import UPDATES** (#6640). Skipping the FSM is only half of migrating established facts; the other half is keeping *when* they happened and *who* did them. Under the same flag the write context also carries `preserveAudit`, which (1) makes `updated_at` / `updated_by` **client-preferred** — a supplied historical last-modified survives instead of being stamped with the import instant — and (2) admits a **whitelist** through the static-`readonly` write strip: the audit/timestamp family plus author-declared business `readonly` fields (`closed_at`, `resolved_by`, …). Platform-managed `system` columns outside that family (`organization_id` and other tenancy/generated columns) stay stripped — a historical import reinstates facts, it does not forge tenancy. Like the FSM exemption this is opt-in: a normal write still auto-stamps `updated_at`/`updated_by` and strips `readonly` exactly as before, and permissions / RLS / field-level security are unchanged.
+- **`treatAsHistorical` also preserves the original audit timeline** (#3493) — **on the rows an import UPDATES** (#6640). Skipping the FSM is only half of migrating established facts; the other half is keeping *when* they happened and *who* did them. Under the same flag the write context also carries `preserveAudit`, which (1) makes `updated_at` / `updated_by` **client-preferred** — a supplied historical last-modified survives instead of being stamped with the import instant — and (2) admits a **whitelist** through the static-`readonly` write strip: the audit/timestamp family plus author-declared business `readonly` fields (`closed_at`, `resolved_by`, …) — but never the record's own primary key (`id`), which is the address of the write rather than a fact being restored (#8215). Platform-managed `system` columns outside that family (`organization_id` and other tenancy/generated columns) stay stripped — a historical import reinstates facts, it does not forge tenancy. Like the FSM exemption this is opt-in: a normal write still auto-stamps `updated_at`/`updated_by` and strips `readonly` exactly as before, and permissions / RLS / field-level security are unchanged.
 - **…but a historical `upsert` still drops those columns from the rows it CREATES** (#6640). `preserveAudit` is an **UPDATE-path exemption and nothing else reads it**, because the two write paths run two different strips: UPDATE is stripped inside the engine (`stripReadonlyFields`), which consults `preserveAudit`; CREATE is stripped earlier, at the DataProtocol ingress (`stripReadonlyForInsert`, #3043) that every REST-import create travels, and that one's only exemption is `context.isSystem`. So a single `treatAsHistorical` upsert keeps `closed_at` on the rows it **matches** and strips it — together with a supplied `created_at` / `updated_at`, which the injected audit columns also declare `readonly` — from the rows it **inserts**. The asymmetry is deliberate, not an oversight: `treatAsHistorical` arrives on an ordinary (non-system) import request, so honouring it on create would let any caller seed the approval/status columns that create-side strip exists to protect. The ignored request is at least no longer silent — the server logs a `WARN` naming the object, the stripped fields and this UPDATE-only rule — but the strip still applies. **To replay archival read-only facts on the rows an import creates, write from a system context** (`isSystem`). Full rule and rationale: [Security & Access Control](/docs/protocol/objectql/security).
 - **Undoing a historical import is symmetric** (#3549 / #3556). The import undo (`POST /api/v1/data/import/jobs/:jobId/undo`) logically rolls back a finished job — deleting the rows it created and restoring the captured pre-import snapshot on the rows it updated. That restore write now carries `preserveAudit` too, but **only** when the job was flagged `treatAsHistorical`, so the snapshotted `updated_at` / `updated_by` and business `readonly` fields (`closed_at`, …) are reinstated verbatim instead of being re-stamped to the undo instant. The undo is unaffected by the create-side carve-out above: it only ever *deletes* the rows the import created and *updates* the rows it touched, so every write it makes is on the path where the exemption is real. Without it the undo would silently overwrite the very timeline the historical import preserved; a normal (non-historical) import's undo keeps the default stamp/strip.
 

packages/objectql/src/validation/rule-validator.test.ts

@@ -799,6 +799,51 @@ describe('stripReadonlyFields — preserveAudit whitelist (#3493)', () => {
  expect(out).toEqual({ closed_at: '2021-03-01T00:00:00Z' });
  });
 
+ it('NEVER keeps the primary key — a readonly `id` is stripped even under preserveAudit (#8215)', () => {
+ // Every platform object declares its `id` `readonly: true` and nothing
+ // flags it `system` (`sys_user_preference`: `Field.text({ label:
+ // 'Preference ID', required: true, readonly: true })`), so the pre-#8215
+ // second limb read it as a business field like `closed_at` — and a
+ // historical import's by-id update handed `SET id = 'rec_1' WHERE id =
+ // 'rec_1'` to the driver: a no-op on SQL, an outright rejection on stores
+ // with immutable primary keys. The REST ingress folds the path id into
+ // every update body (#6479), so the importer never even typed the key it
+ // was being handed back. The address of a write is not a fact being
+ // restored; the whitelist stops at it.
+ const supplied = { id: 'rec_1', closed_at: '2021-03-01T00:00:00Z' };
+ const out = stripReadonlyFields(
+ { fields: { ...historicalFields.fields, id: { type: 'text', readonly: true } } },
+ { ...supplied },
+ supplied,
+ undefined,
+ { preserveAudit: true },
+ );
+ expect(out).toEqual({ closed_at: '2021-03-01T00:00:00Z' });
+ });
+
+ it('…while the flag keeps doing its actual job on the same payload — timeline + business fields survive (#8215)', () => {
+ // The other direction, pinned so the narrowing cannot creep: #3493 scoped
+ // `preserveAudit` to "reinstate the original timeline", and everything in
+ // that scope still rides through beside a stripped `id`.
+ const supplied = {
+ id: 'rec_1',
+ created_at: '2020-01-01T00:00:00Z',
+ created_by: 'u_creator',
+ updated_at: '2021-03-01T00:00:00Z',
+ updated_by: 'u_old',
+ closed_at: '2021-03-01T00:00:00Z',
+ };
+ const out = stripReadonlyFields(
+ { fields: { ...historicalFields.fields, id: { type: 'text', readonly: true } } },
+ { ...supplied },
+ supplied,
+ undefined,
+ { preserveAudit: true },
+ );
+ const { id: _address, ...reinstated } = supplied;
+ expect(out).toEqual(reinstated);
+ });
+
  it('STILL strips a non-audit system column (organization_id) under preserveAudit — no tenancy backdoor', () => {
  const supplied = { organization_id: 'org_forged', closed_at: '2021-03-01T00:00:00Z' };
  const out = stripReadonlyFields(
@@ -965,8 +1010,13 @@ describe('stripReadonlyFields — addressKey silences the LOG, never the strip (
  const supplied = { id: 'rec_1', value: 'v1' };
  const { out, warns, levels } = stripWithWarns(addressedFields, { ...supplied }, supplied);
  expect(out).toEqual({ value: 'v1' });
- expect(warns).toEqual([readonlyStripWarning('id', 'pref', { preserveAuditApplies: true })]);
+ expect(warns).toEqual([readonlyStripWarning('id', 'pref')]);
  expect(levels).toEqual(['warn']);
+ // [#8215] The line no longer offers `{ context: { preserveAudit: true } }`
+ // for the primary key — the whitelist stopped covering it, so the flag
+ // would not keep `id`, and offering it would be exactly the false-remedy
+ // shape #8214 removed. Pinned literally, not via the composer.
+ expect(warns[0]).not.toContain('preserveAudit');
  });
 
  it('a DIFFERENT read-only field in the same payload still WARNs, unchanged in wording and level', () => {
@@ -1003,30 +1053,38 @@ describe('stripReadonlyFields — addressKey silences the LOG, never the strip (
  expect(warns).toEqual([readonlyStripWarning('locked_note', 'pref', { preserveAuditApplies: true })]);
  });
 
- it('composes with preserveAudit rather than overriding it', () => {
+ it('composes with preserveAudit — and the primary key is stripped even under the flag (#8215)', () => {
  const supplied = { id: 'rec_1', created_at: '2020-01-01T00:00:00Z', organization_id: 'org_forged' };
  const { out, warns } = stripWithWarns(
  { name: 'pref', fields: { ...addressedFields.fields, ...historicalFields.fields } },
  { ...supplied },
  supplied,
  { preserveAudit: true, addressKey: 'id' },
  );
- // Measured, and NOT what the first draft of this case predicted: under a
- // historical import the address never reaches the address rule at all. A
- // `readonly` field that is not `system` is an author-declared business
- // field to {@link isPreservableUnderAudit}, so `preserveAudit` KEEPS `id`
- // one line earlier — which is pre-#8141 behaviour, unchanged here, and
- // #6435's separate decision if anyone wants it different. `created_at` is
- // reinstated by the same whitelist; `organization_id` is a non-audit system
- // column, so it is still stripped and still loud.
- expect(out).toEqual({ id: 'rec_1', created_at: '2020-01-01T00:00:00Z' });
- // [#8214] NO `preserveAuditApplies` here, and that is the load-bearing
- // half: `organization_id` is `system` and outside the audit family, so
- // `preserveAudit` does NOT rescue it — this very call had the flag ON and
- // stripped it anyway. A blanket "use preserveAudit" sentence would be a
- // lie exactly here, which is why the remedy is derived per field.
- expect(warns).toEqual([readonlyStripWarning('organization_id', 'pref')]);
+ // [#8215] Until this card, `preserveAudit` KEPT `id` one line before the
+ // address rule was consulted — a `readonly` field with no `system: true`
+ // read as an author-declared business field to `isPreservableUnderAudit`,
+ // and the driver received `SET id = 'rec_1' WHERE id = 'rec_1'` (a no-op
+ // on SQL, an outright rejection on stores with immutable primary keys —
+ // #6435 / #8141). The primary key is the ADDRESS of the write, not a fact
+ // a historical import is restoring, so the whitelist no longer covers it:
+ // `id` is stripped even under the flag, exactly as it is without it.
+ // `created_at` is still reinstated by the whitelist; `organization_id` is
+ // a non-audit system column, still stripped and still loud.
+ expect(out).toEqual({ created_at: '2020-01-01T00:00:00Z' });
+ // …and #8141 composes: the address stays out of the LOG as well as the
+ // payload — the one surviving line names the tenancy forgery alone.
+ // Load-bearing facts pinned as literals rather than through the composer,
+ // so a rewording of both sides cannot keep them green.
+ expect(warns).toHaveLength(1);
+ expect(warns[0]).toContain("Field 'organization_id'");
+ expect(warns[0]).not.toContain("Field 'id'");
+ // [#8214] The remedy stays derived per field: this very call had
+ // `preserveAudit` ON and stripped `organization_id` anyway, so the line
+ // must not offer the flag.
  expect(warns[0]).not.toContain('preserveAudit');
+ // The wording contract itself is still the composer's single source.
+ expect(warns).toEqual([readonlyStripWarning('organization_id', 'pref')]);
  });
 
  it('silences the RUNTIME-OWNED message for the same key, on the same ground', () => {
@@ -1208,6 +1266,27 @@ describe('stripRuntimeOwnedFields — the INSERT-side strip (#5503)', () => {
  );
  expect(out).toEqual({ account_number: 'LEGACY-7' });
  });
+
+ it('but NEVER the primary key — an `autonumber` id seeded under preserveAudit is stripped (#8215)', () => {
+ // The predicate is shared with the update-side strip, and the exclusion
+ // keys on the key's ROLE, not on which lock caught it: `id` is the
+ // record's address on every physical table (the driver provisions it as
+ // the primary key), so it is not one of the "legacy record numbers"
+ // #5503's whitelist exists to reinstate — that case is the business
+ // identifier (`account_number`), pinned KEPT one case up.
+ const numberedId = {
+ name: 'an_account',
+ fields: {
+ id: { type: 'autonumber' },
+ account_number: { type: 'autonumber', autonumberFormat: 'ACC-{0000}' },
+ },
+ };
+ const supplied = { id: 'LEGACY-ID-7', account_number: 'LEGACY-7' };
+ const out = stripRuntimeOwnedFields(
+ numberedId, { ...supplied }, supplied, undefined, { preserveAudit: true },
+ );
+ expect(out).toEqual({ account_number: 'LEGACY-7' });
+ });
 });
 
 // #6339 — the insert-side twin of #5591, and wrong for the identical reason:

packages/objectql/src/validation/rule-validator.ts

@@ -191,6 +191,14 @@
 import { ExpressionEngine, collectCelRootIdentifiers } from '@objectstack/formula';
 import type { Expression } from '@objectstack/spec';
 import { AUDIT_PROVENANCE_FIELDS, RUNTIME_OWNED_FIELD_TYPES } from '@objectstack/spec/data';
+// [#8215] The canonical spelling of the primary-key column — the sanctioned use
+// of this registry ("what is the canonical spelling of the column that plays
+// role X"). The role is structural, not per-object: the driver provisions `id`
+// as the primary key on every physical table (`resolveInjectedSystemColumns`
+// reports it unconditionally, even under `systemFields: false`), and the
+// engine's whole by-id addressing reads that key (`idAddressesThisRow`,
+// `addressKey: 'id'`).
+import { SystemFieldName } from '@objectstack/spec/system';
 import Ajv, { type ValidateFunction } from 'ajv';
 // #5029 — `format` is NOT built into ajv 8; it ships in this separate package.
 // See the `const ajv` note below for why the runtime registers it.
@@ -1400,8 +1408,43 @@ const AUDIT_TIMELINE_FIELDS: ReadonlySet<string> = new Set(AUDIT_PROVENANCE_FIEL
  * (`organization_id` / tenancy, generated columns): a historical import may
  * reinstate established facts, but must not forge tenancy or system-generated
  * values.
+ *
+ * ### [#8215] …and NEVER the row's own primary key
+ *
+ * The second limb used to read every platform object's `id` as an
+ * author-declared business field, because platform objects declare it
+ * `readonly: true` and nothing flags it `system` (`sys_user_preference`:
+ * `Field.text({ label: 'Preference ID', required: true, readonly: true })`).
+ * So on a `preserveAudit` by-id update the strip KEPT `id` and the driver
+ * received `SET id = 'rec_1' WHERE id = 'rec_1'` — from exactly the caller
+ * least able to diagnose it: a bulk historical import whose payload never
+ * named an `id` at all (the REST ingress folds the path id into every update
+ * body, #6479).
+ *
+ * The primary key fails both of the whitelist's own tests:
+ * - it is the ADDRESS of the write, not a fact being restored — #3493 scoped
+ * this flag to "reinstate the original timeline", and a row's address is
+ * not part of its timeline;
+ * - the strip protects it for a STORE-PORTABILITY reason, not an authorship
+ * one — a same-value primary-key write is a no-op on SQL but an outright
+ * rejection on stores with immutable primary keys (#6435 / #8141), so
+ * "the author marked it readonly, the importer may reinstate it" never
+ * described this column.
+ *
+ * Keyed on {@link SystemFieldName.ID}, the platform-wide primary-key role:
+ * the driver provisions `id` on every physical table, unconditionally
+ * (`resolveInjectedSystemColumns` reports it even under `systemFields:
+ * false`), so the name IS the role — there is no per-field `primaryKey`
+ * marker in the spec for a def-based test to read. Consulted before the
+ * audit-family limb to state priority, not to change it (`id` is not in the
+ * family). This also governs {@link stripRuntimeOwnedFields}: an `autonumber`
+ * primary key seeded under a `preserveAudit` insert is now stripped like any
+ * other caller-supplied record number — #5503's "reinstate legacy record
+ * numbers" case is the business identifier (`account_number`), which stays
+ * preservable; the record's address never was one.
  */
 function isPreservableUnderAudit(name: string, def: ConditionalFieldDef): boolean {
+ if (name === SystemFieldName.ID) return false;
  if (AUDIT_TIMELINE_FIELDS.has(name)) return true;
  return def.system !== true;
 }