fix(approvals): the record-lock refusal names the record, not its primary key (#18153)

.changeset/18153-record-lock-message-user-facing.md

@@ -0,0 +1,43 @@
+---
+'@objectstack/plugin-approvals': patch
+---
+
+fix(approvals): the record-lock refusal names the record, not its primary key (#18153)
+
+Clause-②: no
+
+A record held by a live approval refused the write with
+`record '<id>' of '<apiName>' is locked while an approval is in progress`. The
+console copies that sentence into a toast verbatim, so an end user read an
+opaque primary key and a machine identifier — neither of which tells them an
+approval has the record — and a deny-path toast is exactly the string that ends
+up in screenshots, screen recordings and support tickets.
+
+It now reads `Opportunity 'Acme renewal' is locked while an approval is in
+progress, and cannot be edited until that approval is complete`, degrading to
+`This Opportunity is locked …` when the object declares no resolvable title and
+to `This record is locked …` when the registry is unreachable — ⛔ never back to
+the id. The record id and the object's API name are not deleted: they move to
+the CONSOLE (`logger.info`, alongside the pending request's id), which is where
+a support path reads them and where a screen recording does not.
+
+**No read was added.** Both halves were already in hand at the refusal: the
+object's `label` and its ADR-0079 title pointer come from the engine's in-memory
+registry (`getSchema`), and the record itself is `ctx.previous`, the pre-image
+the engine has already read — measured on all four update shapes (by-id,
+`updateManyData`, predicate `multi`, unscoped `multi`), every one of which
+dispatches the hook per row with `previous` bound. Deliberately NOT used: a
+system-context read of the record on the deny path (it would title a row the
+caller may not be allowed to READ — the very state this lock exists to gate) and
+the `payload_json` snapshot (served redacted per reader).
+
+**Nothing else moved.** `RECORD_LOCKED` and its `409` are unchanged and pinned
+in both directions, the `CODE: message` envelope is unchanged, and the three
+OPERATOR-facing refusals in the same file — the two `PENDING_LOCK_LIMIT` cap
+messages and the unanswerable-intersection message — still name the object's API
+name, which is the useful thing to say to whoever has to rescope that write.
+They are pinned byte for byte so a later "harmonise the lock's messages" sweep
+cannot fold them into the end-user shape.
+
+A client asserting on the old sentence's text will need updating; a client
+branching on `error.code` or the 409 needs no change.

packages/plugins/plugin-approvals/src/approval-service.test.ts

@@ -2336,9 +2336,42 @@ describe('record-lock hook — predicate (multi) updates (#4778)', () => {
     await expect(predicateUpdate(undefined, { amount: 999 })).rejects.toThrow(/RECORD_LOCKED/);
   });
 
-  it('names the locked record and its object in the refusal', async () => {
-    await expect(predicateUpdate({ stage: 'new' }, { amount: 999 }))
-      .rejects.toThrow(/record 'opp1' of 'opportunity' is locked/);
+  /**
+   * [#18153] This used to assert `record 'opp1' of 'opportunity' is locked` —
+   * the defect itself: the sentence the console copies into a toast carried the
+   * internal record id and the object's API name. It now names the record the
+   * way its object declares it, and the two identifiers move to the console.
+   *
+   * The whole input to that sentence is the pair set up here: ONE registry read
+   * (`getSchema`, in-memory) and the pre-image the real engine already binds on
+   * `ctx.previous`. No read was added on the deny path.
+   */
+  it('names the locked record by its LABEL and demotes the id to the console (#18153)', async () => {
+    (engine as any).getSchema = (object: string) => (object === 'opportunity'
+      ? {
+          name: 'opportunity', label: 'Opportunity', nameField: 'name',
+          fields: { id: { type: 'text' }, name: { type: 'text' } },
+        }
+      : undefined);
+    unbindAllHooks(engine as any);
+    const info: string[] = [];
+    bindApprovalLockHook(engine as any, { warn: () => {}, info: (m: any) => info.push(String(m)) });
+    const previous = { id: 'opp1', name: 'Acme renewal' };
+
+    let body = '';
+    try {
+      await predicateUpdate({ stage: 'new' }, { amount: 999 }, { previous });
+    } catch (e: any) { body = String(e?.message); }
+
+    expect(body).toBe(
+      "RECORD_LOCKED: Opportunity 'Acme renewal' is locked while an approval is in progress, " +
+      'and cannot be edited until that approval is complete',
+    );
+    // Stated as an absence as well, because that is what the card asked for.
+    expect(body).not.toContain('opp1');
+    expect(body).not.toContain('opportunity');
+    // Not deleted — MOVED. A support path still reads both off the console.
+    expect(info.some(l => l.includes('opp1') && l.includes('opportunity'))).toBe(true);
   });
 
   // ── and it must not over-block: a lock is a PER-ROW verdict ────────
@@ -2462,9 +2495,20 @@ describe('record-lock hook — predicate (multi) updates (#4778)', () => {
       flow_run_id: 'run_2',
       node_config_json: JSON.stringify({ lockRecord: false }),
     });
+    unbindAllHooks(engine as any);
+    const info: string[] = [];
+    bindApprovalLockHook(engine as any, { warn: () => {}, info: (m: any) => info.push(String(m)) });
+
     await expect(predicateUpdate({ id: { $in: ['opp2'] } }, { amount: 999 })).resolves.toBeUndefined();
     await expect(predicateUpdate({ id: { $in: ['opp1', 'opp2'] } }, { amount: 999 }))
-      .rejects.toThrow(/record 'opp1'/);
+      .rejects.toThrow(/RECORD_LOCKED/);
+    // [#18153] WHICH of the two requests refused is no longer decidable from the
+    // user-facing sentence — the id left it on purpose — so the discriminator
+    // moves to the console line the refusal writes. Without it this test would
+    // pass on a refusal raised by `opp2`'s opted-OUT request, which is the exact
+    // confusion it exists to rule out.
+    expect(info.filter(l => l.includes("record 'opp1'"))).toHaveLength(1);
+    expect(info.some(l => l.includes("record 'opp2'"))).toBe(false);
   });
 
   it('ignores a request that is no longer pending', async () => {

packages/plugins/plugin-approvals/src/lifecycle-hooks.ts

@@ -68,10 +68,54 @@
  * by-id path: extending a guard to more rows must move the *allow* rules with
  * the *deny* rules, or fail-open merely becomes false-positive.
  *
+ * ## The per-record refusal is END-USER copy — the other three are not (#18153)
+ *
+ * Four sites raise {@link lockedError}, and they answer two different readers.
+ * Three of them are OPERATOR boundary messages — over the
+ * {@link PENDING_LOCK_LIMIT} cap on a named-id write, over it on a predicate
+ * write, and an unanswerable intersection query — raised about a WRITE SHAPE,
+ * not about a record. Naming the object's API name there is the useful thing to
+ * say, because the reader is whoever has to rescope that write, and their
+ * wording is deliberately left alone.
+ *
+ * The fourth — the per-row verdict at the foot of {@link bindApprovalLockHook}
+ * — is the one an end user reads in a toast when a drag or an inline edit is
+ * refused. It used to spell the record as `record '<id>' of '<apiName>'`,
+ * putting an opaque primary key and a machine identifier into user-facing
+ * prose. That is the defect `@objectstack/objectql`'s `resolveRecordTitle`
+ * exists to stop ("no id fallback", its header), and it is also a DISCLOSURE
+ * surface: a deny-path toast is exactly the string that ends up in screenshots,
+ * screen recordings and support tickets. So the sentence now names the record
+ * the way the object declares it — {@link recordLockRefusal} — and the id plus
+ * the API name are demoted to the CONSOLE, where a support path can still read
+ * them.
+ *
+ * What that costs is ZERO extra reads, which is worth stating because the
+ * opposite was assumed. Both halves are already in hand at the refusal:
+ *
+ *   - the object's `label` and its ADR-0079 title pointer come from
+ *     `engine.getSchema(object)` — an in-memory registry read, no I/O;
+ *   - the record itself is `ctx.previous`, the pre-image the engine has
+ *     ALREADY read. Measured against the real engine + a real sqlite driver on
+ *     all four update shapes (by-id, `updateManyData`, predicate `multi`, and
+ *     unscoped `multi`): every one dispatches this hook per row with `previous`
+ *     bound and `input.id` set, so the title is free on each.
+ *
+ * Two things it deliberately does NOT do. It does not read the record itself
+ * on the deny path: a title fetched as SYSTEM would be a title for a row the
+ * caller may not be allowed to READ, and this hook exists precisely to gate
+ * rows in that state (the #4630 rule above). And it does not mine
+ * `payload_json`, whose whole discipline is that it is served REDACTED per
+ * reader — a field lifted out of it into an error message would route around
+ * that. When no title is in hand the sentence degrades to the object's label,
+ * and when that is missing too, to "This record" — ⛔ never back to the id.
+ *
  * Registered under `packageId: 'plugin-approvals:lock'` so it can be cleanly
  * unbound on plugin stop.
  */
 
+import { resolveDisplayField } from '@objectstack/spec/data';
+
 export const APPROVALS_HOOK_PACKAGE = 'plugin-approvals:lock';
 
 interface MinimalEngine {
@@ -83,6 +127,15 @@ interface MinimalEngine {
   }): void;
   unregisterHooksByPackage(packageId: string): number;
   find<T = any>(object: string, args: any, opts?: any): Promise<T[]>;
+  /**
+   * The registry read that lets the refusal name a record the way its object
+   * declares it (`label`, ADR-0079 `nameField`). REQUIRED on `IObjectQLEngine`
+   * — the slot's actual occupant — and optional HERE for the same reason every
+   * other member of this interface is structural: the hook is bound against
+   * fakes and foreign engines in tests, and a missing registry must degrade the
+   * WORDING, never the lock.
+   */
+  getSchema?(objectName: string): any | undefined;
 }
 
 /**
@@ -148,6 +201,65 @@ function lockedError(message: string): never {
   throw err;
 }
 
+/**
+ * The record's human title, or `undefined` — ⛔ NEVER its id (#18153).
+ *
+ * `resolveDisplayField` is ADR-0079's single arbiter of "which field is the
+ * title" (`nameField`, then the deprecated `displayNameField` alias, then a
+ * deterministic derivation), asked here rather than re-derived so this sentence
+ * and the approvals inbox's `record_title` cannot name different fields for one
+ * object.
+ *
+ * Two guards sit on top of it, and both exist because this message is the one
+ * the id must not reach:
+ *
+ *   - the derivation tier "first title-eligible field by declaration order"
+ *     will happily land on a `text` primary key, so a title pointer spelled
+ *     `id` is refused outright — the same `declared !== 'id'` line
+ *     `ApprovalService.resolveDisplayField` already draws;
+ *   - a title whose VALUE is the record id is refused too, so "no id in the
+ *     toast" holds structurally rather than by trusting the pointer.
+ *
+ * An empty or whitespace-only title is absence, not a title.
+ */
+function recordTitleOf(
+  schema: unknown,
+  record: Record<string, unknown> | null | undefined,
+  recordId: string,
+): string | undefined {
+  if (!schema || !record || typeof record !== 'object') return undefined;
+  const field = resolveDisplayField(schema as any);
+  if (!field || field === 'id' || field === '_id') return undefined;
+  const raw = record[field];
+  if (raw === null || raw === undefined || typeof raw === 'object') return undefined;
+  const title = String(raw).trim();
+  if (!title) return undefined;
+  if (recordId && title === recordId) return undefined;
+  return title;
+}
+
+/**
+ * The END-USER sentence for a record held by a live approval (#18153).
+ *
+ * Three degradations, in the order the material runs out, and none of them
+ * reaches for the id or the object's API name:
+ *
+ *   - title + label → `Opportunity 'Acme renewal' is locked …`
+ *   - label only    → `This Opportunity is locked …`
+ *   - neither       → `This record is locked …`
+ *
+ * The lock's own clause is kept verbatim from the message this replaced ("is
+ * locked while an approval is in progress"), so a reader who has seen the old
+ * text recognises the new one; what is added is the one thing the old sentence
+ * never said — what has to happen before the record can be edited again.
+ */
+function recordLockRefusal(objectLabel: string | undefined, recordTitle: string | undefined): string {
+  const subject = recordTitle
+    ? (objectLabel ? `${objectLabel} '${recordTitle}'` : `'${recordTitle}'`)
+    : (objectLabel ? `This ${objectLabel}` : 'This record');
+  return `${subject} is locked while an approval is in progress, and cannot be edited until that approval is complete`;
+}
+
 /**
  * The record ids a write names outright — a scalar id or an `{ $in: [...] }` —
  * or `null` when the ids cannot be read off it (any other predicate, or an
@@ -389,9 +501,29 @@ export function bindApprovalLockHook(engine: MinimalEngine, logger?: MinimalLogg
       const mirror = config?.approvalStatusField;
       if (typeof mirror === 'string' && mirror && changedFields.every((f) => f === mirror)) continue;
 
-      lockedError(
-        `record '${String(pending?.record_id ?? '')}' of '${object}' is locked while an approval is in progress`,
+      // ── The one END-USER sentence of the four (#18153) ─────────────
+      // See the module docstring. The id and the API name are not deleted —
+      // they move to the console, which is where a support path reads them
+      // and where a screen recording does not.
+      const recordId = String(pending?.record_id ?? '');
+      let schema: unknown;
+      try { schema = engine.getSchema?.(object); } catch { /* registry unavailable — label degrades */ }
+      const objectLabel = typeof (schema as any)?.label === 'string' && (schema as any).label
+        ? String((schema as any).label)
+        : undefined;
+      // `previous` is the pre-image the engine already read for THIS row; it is
+      // used only when it really is the row this pending request names, so a
+      // dispatch that ever carried a different row cannot title the wrong
+      // record. No fallback read — see the module docstring.
+      const previous = ctx?.previous as Record<string, unknown> | undefined;
+      const previousId = previous ? String(previous.id ?? previous._id ?? '') : '';
+      const record = previousId && previousId === recordId ? previous : undefined;
+
+      logger?.info?.(
+        `[approvals] update refused RECORD_LOCKED: record '${recordId}' of '${object}' ` +
+        `is held by pending approval request '${String(pending?.id ?? '')}'`,
       );
+      lockedError(recordLockRefusal(objectLabel, recordTitleOf(schema, record, recordId)));
     }
   }, { packageId: APPROVALS_HOOK_PACKAGE, priority: 50 });
 

packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts

@@ -19,6 +19,14 @@
  * single-spelling defect that made `/api/v1/data` answer 500 to this very
  * refusal until #7525.
  *
+ * ⚠️ The `message` in that block is the text as MEASURED IN #8502 and is left
+ * verbatim as the record of what was observed then. #18153 reworded exactly
+ * that sentence — it was the one user-facing refusal of the four, and it put
+ * the internal record id and the object's API name into a toast — so the
+ * assertion below carries the CURRENT text and no longer matches the quote.
+ * What this file pins is unchanged: which `code`, which `httpStatus`, and that
+ * both survive the batch-row lowering.
+ *
  * ## Why the pin lives HERE
  *
  * `metadata-protocol` cannot import this plugin, and its own pins therefore
@@ -121,9 +129,22 @@ describe('[#8570] a locked record\'s batch row carries the 409 the hook declared
     expect(res.results[0].success).toBe(false);
     expect(res.results[0].errors[0]).toEqual({
       code: 'RECORD_LOCKED',
-      message: `RECORD_LOCKED: record '${lockedId}' of 'opportunity' is locked while an approval is in progress`,
+      // [#18153] The sentence names the record the way the object declares it
+      // ('Deal' is this row's `name`, and `nameField` resolves to it) instead
+      // of `record '<id>' of '<apiName>'`. The `code`/`httpStatus` pair either
+      // side of it is what this file exists for and is unmoved.
+      message: "RECORD_LOCKED: Opportunity 'Deal' is locked while an approval is in progress, and cannot be edited until that approval is complete",
       httpStatus: 409,
     });
+    // The same row, read as the CARD reads it: no internal identifier in the
+    // user-visible body (#18153). The object is named by its LABEL
+    // ('Opportunity'), which is why the API-name assertion is case-sensitive on
+    // purpose — the two differ by exactly that, and the label is the half a
+    // user can act on.
+    const body: string = res.results[0].errors[0].message;
+    expect(body).not.toContain(lockedId);
+    expect(body).not.toContain(opportunity.name);
+    expect(body).toContain(opportunity.label);
 
     // The refusal was a refusal: nothing reached the store.
     expect((await engine.findOne('opportunity', { where: { id: lockedId } }))?.amount).toBe(100);