From 311635cea528e398fac54397e257bb693d6ccae2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 15:29:29 +0000 Subject: [PATCH 1/2] fix(approvals): the record-lock refusal names the record, not its primary key (#18153) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-row `RECORD_LOCKED` refusal is the one end-user-facing message of the four `lockedError` sites in this file — the console copies it into a toast verbatim — and it spelled the record `record '' of ''`, putting an opaque primary key and a machine identifier into user-facing prose on a path that reaches screenshots and support tickets. It now names the record the way its object declares it (ADR-0079 title pointer, object label), degrading to the label alone and then to "This record" — never back to the id. The id and the API name move to the console, where a support path still reads them. No read was added: the object label and title pointer come from the engine's in-memory registry, and the record is `ctx.previous`, the pre-image the engine has already read on every update shape measured. The three operator-facing refusals in the same file are unchanged and now pinned byte for byte; `RECORD_LOCKED` and its 409 are pinned in both directions. Claude-Session: https://claude.ai/code/session_01QGMBhvUoyD8t5zY8xHQhnP Co-authored-by: Claude --- .../18153-record-lock-message-user-facing.md | 43 +++ .../plugin-approvals/src/lifecycle-hooks.ts | 136 ++++++++- ...-lock-batch-row-status.integration.test.ts | 23 +- .../record-lock-user-facing-message.test.ts | 263 ++++++++++++++++++ 4 files changed, 462 insertions(+), 3 deletions(-) create mode 100644 .changeset/18153-record-lock-message-user-facing.md create mode 100644 packages/plugins/plugin-approvals/src/record-lock-user-facing-message.test.ts diff --git a/.changeset/18153-record-lock-message-user-facing.md b/.changeset/18153-record-lock-message-user-facing.md new file mode 100644 index 00000000000..d88ed0b84a6 --- /dev/null +++ b/.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 '' of '' 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. diff --git a/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts b/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts index 10159f22157..334a20ac809 100644 --- a/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts +++ b/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 '' of ''`, + * 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(object: string, args: any, opts?: any): Promise; + /** + * 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 | 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 | 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 }); diff --git a/packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts b/packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts index 792a0b43caa..b4d795dd5c0 100644 --- a/packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts +++ b/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 '' of ''`. 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); diff --git a/packages/plugins/plugin-approvals/src/record-lock-user-facing-message.test.ts b/packages/plugins/plugin-approvals/src/record-lock-user-facing-message.test.ts new file mode 100644 index 00000000000..4aa7062c87e --- /dev/null +++ b/packages/plugins/plugin-approvals/src/record-lock-user-facing-message.test.ts @@ -0,0 +1,263 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#18153] The record lock's per-row refusal is END-USER copy — pinned as such, + * and pinned APART from the three operator-facing refusals beside it. + * + * ## What the card was + * + * A record held by a live approval refused the write with + * `record '' of '' is locked while an approval is in progress`. + * The console toast copies that sentence verbatim, so an end user read an + * opaque primary key and a machine identifier, and a deny-path toast is exactly + * the string that reaches screenshots, screen recordings and support tickets. + * + * ## The three pins, and why each is here + * + * 1. **The user-facing half.** Driven end to end — real {@link ObjectQL}, real + * sqlite driver, the real `beforeUpdate` hook — because the defect was about + * the string a real refusal produces, not about a helper in isolation. The + * assertion is stated as an ABSENCE (no record id, no object API name in the + * body) as well as a presence, because the presence alone would stay green + * on a sentence that named the record correctly and then appended the id. + * + * 2. **The three operator refusals, unchanged.** The cap refusals and the + * unanswerable-intersection refusal are raised about a WRITE SHAPE, and + * naming the object's API name there is the useful thing to say. The card + * ruled them out of scope; this pins that, so a later "harmonise the lock's + * messages" sweep goes red instead of quietly sweeping them into the + * end-user shape. They are driven through a FAKE engine rather than the real + * rig on purpose: two of the three need more than {@link PENDING_LOCK_LIMIT} + * locked rows and the third needs the engine's own read to fail, and neither + * is a state a sqlite fixture should be bent into to assert a string. + * + * 3. **`RECORD_LOCKED` / 409, both directions.** The card moved the human + * sentence only; the wire vocabulary is contract (ADR-0112) and the 409 is + * pinned across the batch-row protocol surfaces. Both directions means: the + * code and status are asserted to BE those values, and the `CODE: message` + * envelope is asserted to still be the envelope — a rewording that reached + * either would be a widening, not a copy fix. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { bindApprovalLockHook, APPROVALS_HOOK_PACKAGE } from './lifecycle-hooks.js'; + +const OPPORTUNITY = { + name: 'opportunity', + label: 'Opportunity', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + amount: { name: 'amount', type: 'number' as const }, + approval_status: { name: 'approval_status', type: 'text' as const }, + }, +}; + +/** Same shape, no title-eligible field at all — the degradation leg. */ +const LEDGER_ENTRY = { + name: 'ledger_entry', + label: 'Ledger Entry', + nameField: 'id', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + amount: { name: 'amount', type: 'number' as const }, + approval_status: { name: 'approval_status', type: 'text' as const }, + }, +}; + +const APPROVAL_REQUEST = { + name: 'sys_approval_request', + label: 'Approval Request', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + object_name: { name: 'object_name', type: 'text' as const }, + record_id: { name: 'record_id', type: 'text' as const }, + status: { name: 'status', type: 'text' as const }, + flow_run_id: { name: 'flow_run_id', type: 'text' as const }, + node_config_json: { name: 'node_config_json', type: 'text' as const }, + }, +}; + +const NODE_CONFIG = JSON.stringify({ lockRecord: true, approvalStatusField: 'approval_status' }); + +describe('[#18153] the per-row refusal is user-facing copy', () => { + let engine: ObjectQL; + let opportunityId: string; + let ledgerId: string; + + afterEach(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + beforeEach(async () => { + engine = new ObjectQL(); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }), true); + await engine.init(); + // `packageId` is REQUIRED by `registerObject` — passed rather than elided so + // this file adds no raw `tsc` error to the package's TEST_DEBT ledger. + for (const o of [OPPORTUNITY, LEDGER_ENTRY, APPROVAL_REQUEST]) { + engine.registry.registerObject(o as any, 'com.objectstack.test.18153'); + } + await engine.syncSchemas(); + + opportunityId = String((await engine.insert('opportunity', { name: 'Acme renewal', amount: 100 })).id); + ledgerId = String((await engine.insert('ledger_entry', { amount: 100 })).id); + for (const [object, recordId] of [['opportunity', opportunityId], ['ledger_entry', ledgerId]] as const) { + await engine.insert('sys_approval_request', { + object_name: object, record_id: recordId, status: 'pending', + flow_run_id: 'run_1', node_config_json: NODE_CONFIG, + }, { context: { isSystem: true } } as any); + } + + bindApprovalLockHook(engine as any); + }); + + async function refusal(object: string, id: string): Promise { + let thrown: any = null; + try { + await engine.update(object, { amount: 999 }, { where: { id } } as any); + } catch (e) { thrown = e; } + expect(thrown).not.toBeNull(); + return thrown; + } + + it('names the record by its label and leaks neither the id nor the API name', async () => { + const thrown = await refusal('opportunity', opportunityId); + + expect(thrown.message).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 too: a sentence that named the record correctly and + // then appended the id would satisfy the equality above only by accident, + // and these two are what the card actually asked for. + expect(thrown.message).not.toContain(opportunityId); + expect(thrown.message).not.toContain(OPPORTUNITY.name); + }); + + it('degrades to the object label — ⛔ never back to the id — when no title resolves', async () => { + // `nameField: 'id'` is the pointer a title-less object ends up with, and it + // is exactly the shape that would re-introduce the defect if the resolver + // trusted the pointer. + const thrown = await refusal('ledger_entry', ledgerId); + + expect(thrown.message).toBe( + 'RECORD_LOCKED: This Ledger Entry is locked while an approval is in progress, ' + + 'and cannot be edited until that approval is complete', + ); + expect(thrown.message).not.toContain(ledgerId); + expect(thrown.message).not.toContain(LEDGER_ENTRY.name); + }); + + it('keeps the id and the API name reachable on the CONSOLE for support', async () => { + engine.unregisterHooksByPackage(APPROVALS_HOOK_PACKAGE); + const info: string[] = []; + bindApprovalLockHook(engine as any, { warn: () => {}, info: (m: any) => info.push(String(m)) }); + + await refusal('opportunity', opportunityId); + + // The card's own remedy: "moved to the console … not simply deleted if a + // support path still needs them". + expect(info.some((line) => line.includes(opportunityId) && line.includes('opportunity'))).toBe(true); + }); + + it('pins `RECORD_LOCKED` and 409 in both directions', async () => { + const thrown = await refusal('opportunity', opportunityId); + + expect(thrown.code).toBe('RECORD_LOCKED'); + expect(thrown.statusCode).toBe(409); + // The other direction: the wire vocabulary did not acquire a second + // spelling, and the `CODE: message` envelope is still the envelope. + expect(thrown.status).toBeUndefined(); + expect(String(thrown.message).startsWith('RECORD_LOCKED: ')).toBe(true); + }); +}); + +/** + * CONTROL — the three operator-facing refusals, byte for byte. + * + * A fake engine, because these three are reached by write SHAPES, not by record + * states: two need more locked rows than {@link PENDING_LOCK_LIMIT} and the + * third needs the intersection read to throw. Each refusal is driven through + * the REAL handler `bindApprovalLockHook` registers. + */ +describe('[#18153] CONTROL — the operator-facing refusals are untouched', () => { + const LIMIT = 1_000; + + /** Captures the handler the real binder registers, then drives it directly. */ + function bindAgainst(find: (object: string, args: any) => Promise) { + let handler!: (ctx: any) => Promise; + bindApprovalLockHook({ + registerHook: (_event: string, h: any) => { handler = h; }, + unregisterHooksByPackage: () => 0, + find: find as any, + } as any); + return handler; + } + + async function refusalFrom(handler: (ctx: any) => Promise, ctx: any): Promise { + let thrown: any = null; + try { await handler(ctx); } catch (e) { thrown = e; } + expect(thrown).not.toBeNull(); + return thrown; + } + + it('the named-id cap refusal still names the object', async () => { + const handler = bindAgainst(async () => []); + const ids = Array.from({ length: LIMIT + 1 }, (_v, i) => `r${i}`); + + const thrown = await refusalFrom(handler, { + object: 'leave_request', + input: { id: { $in: ids }, data: { amount: 1 } }, + }); + + expect(thrown.message).toBe( + `RECORD_LOCKED: refusing to authorize an update naming more than ${LIMIT} records of 'leave_request' — ` + + 'the approval lock cannot check them row by row; scope the write', + ); + expect(thrown.code).toBe('RECORD_LOCKED'); + expect(thrown.statusCode).toBe(409); + }); + + it('the predicate cap refusal still names the object', async () => { + const pending = Array.from({ length: LIMIT + 1 }, (_v, i) => ({ record_id: `r${i}`, status: 'pending' })); + const handler = bindAgainst(async () => pending); + + const thrown = await refusalFrom(handler, { + object: 'leave_request', + input: { data: { amount: 1 }, options: { where: { status: 'draft' } } }, + }); + + expect(thrown.message).toBe( + `RECORD_LOCKED: refusing a predicate update on 'leave_request': more than ${LIMIT} of its records carry a ` + + 'pending approval, so the lock cannot decide row by row; scope the write to the rows you mean', + ); + expect(thrown.code).toBe('RECORD_LOCKED'); + expect(thrown.statusCode).toBe(409); + }); + + it('the unanswerable-intersection refusal still names the object', async () => { + const handler = bindAgainst(async (object: string) => { + if (object === 'sys_approval_request') return [{ record_id: 'r1', status: 'pending' }]; + throw new Error('driver exploded'); + }); + + const thrown = await refusalFrom(handler, { + object: 'leave_request', + input: { data: { amount: 1 }, options: { where: { status: 'draft' } } }, + }); + + expect(thrown.message).toBe( + "RECORD_LOCKED: cannot determine which rows a predicate update on 'leave_request' would touch " + + '(driver exploded); 1 record(s) of it carry a pending approval, so the write is refused', + ); + expect(thrown.code).toBe('RECORD_LOCKED'); + expect(thrown.statusCode).toBe(409); + }); +}); From 68afd51bc4f8f81c158e0c32cf9502ae68b09eb0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 15:54:58 +0000 Subject: [PATCH 2/2] test(approvals): re-point the two message-shaped lock pins at the new sentence (#18153) Both asserted `record 'opp1' of 'opportunity'`. One WAS the defect's own assertion and now pins the label-named sentence plus the console demotion; the other used the id only as a discriminator between two competing approvals, so that discriminator moves to the console line, which still carries it. Claude-Session: https://claude.ai/code/session_01QGMBhvUoyD8t5zY8xHQhnP Co-authored-by: Claude --- .../src/approval-service.test.ts | 52 +++++++++++++++++-- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index d36db2fc81b..e65594ef6a1 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/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 () => {