fix(approvals,rest): surface the node's lock policy and the batch's dropped fields (#3794)

An approval flow reported record writability wrong in both directions: what the
user could change said "locked", and what they couldn't said "updated
successfully". Both halves were missing signal, not wrong behaviour — the server
did the right thing and told nobody.

`rowFromRequest` now emits `locks_record`, read from the same `node_config_json`
snapshot the `beforeUpdate` lock hook reads, with the same default-true. A client
could previously only see "a request is pending" and had to guess whether that
locked the record; the Console guessed "locked" every time, which hides the whole
point of a `lockRecord: false` node.

`POST /batch` (cross-object transactional batch) never wired `onFieldsDropped`,
so the one write path the Console's master-detail record form takes was also the
one path with no write-observability — a `readonlyWhen` strip there was
completely silent. It now collects per-op events and returns them as a top-level
`droppedFields` list tagged with each operation's index; omitted when empty.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: baozhoutao

.changeset/approval-lock-and-batch-write-observability.md

@@ -0,0 +1,40 @@
+---
+"@objectstack/plugin-approvals": minor
+"@objectstack/spec": minor
+"@objectstack/rest": minor
+---
+
+fix(approvals,rest): tell the truth about what a write did — `locks_record` on the request, `droppedFields` on the cross-object batch (#3794)
+
+Two halves of the same complaint: in an approval flow the platform reported
+record writability wrong in *both* directions — what you could change said
+"locked", and what you couldn't said "updated successfully".
+
+**`locks_record` on the approval request.** The lock hook reads one thing to
+decide whether a pending approval blocks writes: the node config snapshot's
+`lockRecord` (`=== false` ⇒ the update goes through). Nothing exposed that, so a
+client had only "a pending request exists" and had to guess — and the Console
+guessed "locked", every time. A `lockRecord: false` node exists precisely so the
+approver can amend the record while deciding on it; painting "Locked for
+approval" over that hides the whole feature, and approvers never try. Request
+rows (`getRequest` / `listRequests`, and therefore `GET /api/v1/approvals/requests`)
+now carry `locks_record`, read from the same snapshot the hook reads, with the
+same default-true. Absent on rows from an older server ⇒ assume locked.
+
+**`droppedFields` on `POST /batch`.** The engine strips writes to `readonly`
+(#2948) and `readonlyWhen`-locked (#3042) fields and completes the write without
+them. Every write path already reported which fields it dropped (#3431/#3455) —
+except the cross-object transactional batch, which never wired
+`onFieldsDropped` at all. That path is the Console record form's save for a
+master-detail record, so it is exactly where a *user* edits a `readonlyWhen`
+field: they changed it, the form said "updated successfully", the value never
+moved, and nothing anywhere said so. The response now carries a top-level
+`droppedFields` list, each event tagged with the `index` of the operation that
+produced it (`results` entries are bare record echoes with no envelope to hang a
+per-row list on). Omitted entirely when nothing was dropped, so the shape stays
+backward-compatible; the batch still commits either way — a strip is legal
+semantics, not an error.
+
+The Console half of both fixes ships in objectui: the detail band now
+distinguishes "in approval (editable)" from "locked for approval", and the
+write-warning toast fires on batch saves.

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

@@ -183,6 +183,38 @@ describe('ApprovalService (node era)', () => {
     expect(JSON.parse(raw.node_config_json)).toMatchObject({ behavior: 'first_response', lockRecord: true });
   });
 
+  // #3794 — the row tells a client whether THIS node locks the record, so
+  // "pending" and "locked" stop being the same signal. Read from the same
+  // snapshot key the `beforeUpdate` lock hook reads, with the same default.
+  it('rows surface locks_record from the node config (default true)', async () => {
+    const req = await svc.openNodeRequest(openInput(['u9']), CTX);
+    expect(req.locks_record).toBe(true);
+    const [listed] = await svc.listRequests({ status: 'pending' }, SYS);
+    expect(listed.locks_record).toBe(true);
+  });
+
+  it('rows surface locks_record=false for a lockRecord:false node', async () => {
+    const req = await svc.openNodeRequest(
+      openInput(['u9'], {}, { lockRecord: false }),
+      CTX,
+    );
+    expect(req.locks_record).toBe(false);
+    const [listed] = await svc.listRequests({ status: 'pending' }, SYS);
+    expect(listed.locks_record).toBe(false);
+    const fetched = await svc.getRequest(req.id, SYS);
+    expect(fetched?.locks_record).toBe(false);
+  });
+
+  it('locks_record defaults to true when the node config omits lockRecord', async () => {
+    // A node that says nothing about locking locks — matching the hook, which
+    // only returns early on an explicit `lockRecord === false`.
+    const req = await svc.openNodeRequest(
+      openInput(['u9'], {}, { lockRecord: undefined }),
+      CTX,
+    );
+    expect(req.locks_record).toBe(true);
+  });
+
   it('openNodeRequest: deduplicates a pending request per (object, record)', async () => {
     await svc.openNodeRequest(openInput(['u9']), CTX);
     await expect(svc.openNodeRequest(openInput(['u9'], { runId: 'run_2' }), CTX))

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

@@ -280,6 +280,11 @@ function rowFromRequest(row: any): ApprovalRequestRow {
     payload: parseJson(row.payload_json, undefined),
     flow_run_id: row.flow_run_id ?? undefined,
     flow_node_id: row.flow_node_id ?? undefined,
+    // #3794: the node's record-lock policy, read from the SAME config snapshot
+    // the `beforeUpdate` lock hook reads (`lockRecord === false` ⇒ allow), so a
+    // client can tell "pending" from "locked" instead of assuming every pending
+    // request locks. Default-true mirrors the hook's default.
+    locks_record: cfg?.lockRecord !== false,
     completed_at: row.completed_at ?? undefined,
     created_at: row.created_at ?? undefined,
     updated_at: row.updated_at ?? undefined,

packages/rest/src/rest-batch-endpoint.test.ts

@@ -219,6 +219,45 @@ describe('POST {basePath}/batch — cross-object transactional batch', () => {
     expect(ql.transaction).not.toHaveBeenCalled();
   });
 
+  // ── write observability (#3794) ───────────────────────────────────────────
+  //
+  // The console's record form saves a master-detail record through THIS route,
+  // so a `readonlyWhen`-locked field edited in that form was stripped by the
+  // engine and the response said nothing — the user saw "updated successfully"
+  // over a value that never changed. Every other write path already reports its
+  // strips (#3431/#3455); this one now does too.
+
+  it('surfaces engine-stripped fields as droppedFields tagged with the op index', async () => {
+    const ql = makeQl({
+      update: vi.fn(async (_object: string, data: any, opts: any) => {
+        opts?.onFieldsDropped?.({ object: 'invoice', fields: ['tax_rate'], reason: 'readonly_when' });
+        const { tax_rate: _stripped, ...kept } = data;
+        return kept;
+      }),
+    });
+    const { route } = buildServer({ ql });
+    const res = await post(route, {
+      operations: [
+        { object: 'account', action: 'create', data: { name: 'Acme' } },
+        { object: 'invoice', action: 'update', id: 'inv_1', data: { status: 'paid', tax_rate: 9 } },
+      ],
+    });
+    expect(res.statusCode).toBe(200);
+    // The batch still committed — a strip is legal semantics, not an error.
+    expect(res.body.results).toHaveLength(2);
+    expect(res.body.droppedFields).toEqual([
+      { object: 'invoice', fields: ['tax_rate'], reason: 'readonly_when', index: 1 },
+    ]);
+  });
+
+  it('omits droppedFields entirely when nothing was stripped', async () => {
+    const ql = makeQl();
+    const { route } = buildServer({ ql });
+    const res = await post(route, { operations: [{ object: 'account', data: { name: 'Acme' } }] });
+    expect(res.statusCode).toBe(200);
+    expect(res.body).not.toHaveProperty('droppedFields');
+  });
+
   // ── request validation ────────────────────────────────────────────────────
 
   it('rejects a non-atomic request (this endpoint is always atomic)', async () => {

packages/rest/src/rest-server.ts

@@ -6594,23 +6594,35 @@ export class RestServer {
                         return result;
                     };
 
+                    // [#3794] Write-observability on THIS surface too. The engine
+                    // strips `readonly` / `readonlyWhen` writes silently, and every
+                    // other write path already reports what it dropped (#3431/#3455)
+                    // — but this one did not, and it is precisely the path the
+                    // console's record form takes for a master-detail save. Result:
+                    // a user edited a `readonlyWhen`-locked field, got "updated
+                    // successfully", and the value never changed with nothing said
+                    // (#3794 problem 2). Each event is tagged with its operation
+                    // index, since `results` entries are bare record echoes with no
+                    // envelope to hang a per-row list on.
+                    const dropped: Array<DroppedFieldsEvent & { index: number }> = [];
                     const results = await ql.transaction(async (trxCtx: any) => {
                         const out: any[] = [];
-                        for (const op of ops) {
+                        for (const [index, op] of ops.entries()) {
                             const data = resolveRefs(op.data, out);
+                            const onFieldsDropped = (e: DroppedFieldsEvent) => { dropped.push({ ...e, index }); };
                             if (op.action === 'create') {
-                                out.push(await ql.insert(op.object, data, { context: trxCtx }));
+                                out.push(await ql.insert(op.object, data, { context: trxCtx, onFieldsDropped }));
                             } else if (op.action === 'update') {
                                 const id = op.id ?? data?.id;
-                                out.push(await ql.update(op.object, { ...data, id }, { context: trxCtx }));
+                                out.push(await ql.update(op.object, { ...data, id }, { context: trxCtx, onFieldsDropped }));
                             } else { // 'delete'
                                 out.push(await ql.delete(op.object, { where: { id: op.id }, context: trxCtx }));
                             }
                         }
                         return out;
                     }, context);
 
-                    res.json({ results });
+                    res.json({ results, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) });
                 } catch (error: any) {
                     // Log only genuine server faults; client 4xx (validation,
                     // unresolved ref, atomic rollback of a bad op) are expected.

packages/spec/json-schema.manifest.json

@@ -152,6 +152,7 @@
     "api/CreateRequest",
     "api/CreateViewRequest",
     "api/CreateViewResponse",
+    "api/CrossObjectBatchDroppedFields",
     "api/CrossObjectBatchOperation",
     "api/CrossObjectBatchRequest",
     "api/CrossObjectBatchResponse",

packages/spec/src/api/batch.zod.ts

@@ -298,13 +298,37 @@ export const CrossObjectBatchRequestSchema = lazySchema(() => z.object({
 
 export type CrossObjectBatchRequest = z.input<typeof CrossObjectBatchRequestSchema>;
 
+/**
+ * One strip event on a cross-object batch, tagged with the operation it
+ * belongs to (#3794). The per-object bulk paths hang `droppedFields` on their
+ * per-row result object; this batch's `results` entries are the raw record
+ * echoes (`z.unknown()`), with no envelope to hang anything on — so the events
+ * ride a parallel top-level list and carry `index` to point back at the
+ * operation that produced them.
+ */
+export const CrossObjectBatchDroppedFieldsSchema = lazySchema(() => DroppedFieldsEventSchema.extend({
+  index: z.number().int().min(0).describe('Index of the operation in the request `operations` array'),
+}).describe('A cross-object batch strip event: dropped fields plus the operation index'));
+
+export type CrossObjectBatchDroppedFields = z.infer<typeof CrossObjectBatchDroppedFieldsSchema>;
+
 /**
  * Response for the cross-object transactional batch — one result per operation,
  * index-aligned with the request `operations` (create/update echo the record,
  * delete echoes the driver's delete result).
  */
 export const CrossObjectBatchResponseSchema = lazySchema(() => z.object({
   results: z.array(z.unknown()).describe('Per-operation result, index-aligned with the request operations'),
+  droppedFields: z.array(CrossObjectBatchDroppedFieldsSchema).optional().describe(
+    'Write-observability (#3407/#3431/#3455/#3794): caller-supplied fields the engine LEGALLY ' +
+    'stripped from an operation before it was written — static `readonly` (#2948) or a TRUE ' +
+    '`readonlyWhen` predicate (#3042). This endpoint is the console record form\'s save path ' +
+    '(master-detail writes parent + children in one transaction), so without it the ONE surface ' +
+    'where a user edits a `readonlyWhen` field reported plain success while the value never ' +
+    'landed. Each event carries the `index` of its operation. Present ONLY when ≥1 field was ' +
+    'dropped; the batch still committed without them (results/success semantics unchanged). ' +
+    'Optional — omit-when-empty keeps the shape backward-compatible.'
+  ),
 }));
 
 export type CrossObjectBatchResponse = z.infer<typeof CrossObjectBatchResponseSchema>;

packages/spec/src/contracts/approval-service.ts

@@ -70,6 +70,27 @@ export interface ApprovalRequestRow {
     type?: 'text' | 'user' | 'department' | 'position' | 'team';
     multiple?: boolean;
   }>;
+  /**
+   * Whether THIS request's node locks the target record for writes (#3794).
+   * Mirrors the one policy the `beforeUpdate` lock hook actually enforces:
+   * the node config snapshot's `lockRecord`, defaulting to `true` when the
+   * node says nothing (`lockRecord === false` ⇒ the hook returns early and
+   * the record stays editable while the request is pending).
+   *
+   * Surfaced because "a pending request exists" and "the record is locked"
+   * are NOT the same statement, and a client that conflates them tells the
+   * user the opposite of the truth in one direction or the other: a console
+   * that renders "Locked for approval" on every pending request hides a
+   * `lockRecord: false` node's whole point (the approver is meant to edit
+   * while deciding), and one that renders nothing lets a user fill a form
+   * that the hook will reject with `RECORD_LOCKED`. Read it instead of
+   * re-deriving from the record's `approval_status` mirror — that field says
+   * "in approval", never "locked", and some flows never materialize it.
+   *
+   * Optional only for version skew (a row from an older server has no
+   * opinion); the service always emits it. Absent ⇒ assume locked.
+   */
+  locks_record?: boolean;
   completed_at?: string;
   created_at?: string;
   updated_at?: string;