fix(rest): route batch creates through the create ingress so readonly is enforced (#3835)

.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.

.changeset/batch-create-readonly-ingress.md

@@ -0,0 +1,39 @@
+---
+"@objectstack/rest": minor
+---
+
+fix(rest): a batch create goes through the same create ingress as a single create (#3835)
+
+`readonly` meant two different things depending on which create endpoint you
+used. `POST /data/:object` runs the #3043 ingress strip, so a non-system caller
+cannot seed a read-only column — the field is dropped and reported. The
+cross-object transactional batch (`POST /batch`) called `ql.insert` directly and
+skipped that ingress entirely, and the engine's INSERT path is
+static-`readonly`-exempt **by design** (#3413, the strip lives one layer up), so
+nothing enforced it: the same forged `readonly` value that was rejected on one
+route was written through on the other.
+
+Measured on the showcase (`showcase_contact.lead_score`, `readonly: true`, same
+signed-in non-system user):
+
+| | before | after |
+|---|---|---|
+| `POST /data/showcase_contact` | `lead_score = null`, reported | unchanged |
+| `POST /batch` create | **`lead_score = 999` written, silent** | `null`, reported |
+
+The fix routes the batch's create ops through the protocol's `createData` rather
+than re-implementing the strip at the REST layer. That keeps **one** create
+ingress: a future change to its policy covers the batch for free, and the
+carve-outs already encoded there stay intact — notably the platform-object
+exemption (a `sys_`/`managedBy` object's own guard must *reject* a forged value
+with 403, not have it silently swallowed) and the `isSystem` exemption. The
+context passed through is the transaction context, so the insert still joins the
+batch transaction and rolls back with it, and `{ $ref: <opIndex> }` resolution is
+unaffected.
+
+`createData`'s `droppedFields` are folded into the batch response's per-op
+`droppedFields` list (#3794), so a batch create now reports its strips the same
+way an update does.
+
+Update ops are untouched: the engine enforces `readonly` and `readonlyWhen` on
+its own update path.

content/docs/references/api/batch.mdx

@@ -30,8 +30,8 @@ Industry alignment: Salesforce Bulk API, Microsoft Dynamics Bulk Operations
 ## TypeScript Usage
 
 ```typescript
-import { BatchConfig, BatchOperationResult, BatchOperationType, BatchOptions, BatchRecord, BatchUpdateRequest, BatchUpdateResponse, CrossObjectBatchOperation, CrossObjectBatchRequest, CrossObjectBatchResponse, DeleteManyRequest, UpdateManyRequest } from '@objectstack/spec/api';
-import type { BatchConfig, BatchOperationResult, BatchOperationType, BatchOptions, BatchRecord, BatchUpdateRequest, BatchUpdateResponse, CrossObjectBatchOperation, CrossObjectBatchRequest, CrossObjectBatchResponse, DeleteManyRequest, UpdateManyRequest } from '@objectstack/spec/api';
+import { BatchConfig, BatchOperationResult, BatchOperationType, BatchOptions, BatchRecord, BatchUpdateRequest, BatchUpdateResponse, CrossObjectBatchDroppedFields, CrossObjectBatchOperation, CrossObjectBatchRequest, CrossObjectBatchResponse, DeleteManyRequest, UpdateManyRequest } from '@objectstack/spec/api';
+import type { BatchConfig, BatchOperationResult, BatchOperationType, BatchOptions, BatchRecord, BatchUpdateRequest, BatchUpdateResponse, CrossObjectBatchDroppedFields, CrossObjectBatchOperation, CrossObjectBatchRequest, CrossObjectBatchResponse, DeleteManyRequest, UpdateManyRequest } from '@objectstack/spec/api';
 
 // Validate data
 const result = BatchConfig.parse(data);
@@ -136,6 +136,22 @@ const result = BatchConfig.parse(data);
 | **results** | `{ id?: string; success: boolean; errors?: { code: string; message: string; category?: string; details?: any; … }[]; data?: Record<string, any>; … }[]` | ✅ | Detailed results for each record |
 
 
+---
+
+## CrossObjectBatchDroppedFields
+
+A cross-object batch strip event: dropped fields plus the operation index
+
+### Properties
+
+| Property | Type | Required | Description |
+| :--- | :--- | :--- | :--- |
+| **object** | `string` | ✅ | Object the write targeted (resolved object name) |
+| **fields** | `string[]` | ✅ | Caller-supplied field names the engine removed from the write payload |
+| **reason** | `Enum<'readonly' \| 'readonly_when'>` | ✅ | Why the fields were dropped: static readonly (#2948) or a TRUE readonlyWhen predicate (#3042) |
+| **index** | `integer` | ✅ | Index of the operation in the request `operations` array |
+
+
 ---
 
 ## CrossObjectBatchOperation
@@ -171,6 +187,7 @@ const result = BatchConfig.parse(data);
 | Property | Type | Required | Description |
 | :--- | :--- | :--- | :--- |
 | **results** | `any[]` | ✅ | Per-operation result, index-aligned with the request operations |
+| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when'>; index: integer }[]` | optional | 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. |
 
 
 ---

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

@@ -42,13 +42,42 @@ function makeQl(overrides: Partial<Record<'insert' | 'update' | 'delete', any>>
   return ql;
 }
 
+/**
+ * A fake `createData` standing in for the protocol's create ingress (#3835):
+ * the batch's create ops go through it rather than calling `ql.insert`, so the
+ * #3043 static-`readonly` strip applies to a batch create exactly as it does to
+ * `POST /data/:object`. Delegates to the same `ql.insert` the other ops use, so
+ * assertions about what reached the engine still read off `ql.insert`.
+ */
+function makeCreateData(ql: any, opts: { readonlyFields?: string[] } = {}) {
+  const readonlyFields = opts.readonlyFields ?? [];
+  return vi.fn(async (request: any) => {
+    const supplied = request?.data ?? {};
+    const stripped: string[] = readonlyFields.filter((f) => f in supplied);
+    const data = { ...supplied };
+    for (const f of stripped) delete data[f];
+    const record = await ql.insert(request.object, data, { context: request.context });
+    return {
+      object: request.object,
+      id: record?.id,
+      record,
+      ...(stripped.length > 0
+        ? { droppedFields: [{ object: request.object, fields: stripped, reason: 'readonly' }] }
+        : {}),
+    };
+  });
+}
+
 /** Build a RestServer with an optional ObjectQL provider and object metadata. */
-function buildServer(opts: { ql?: any; objects?: any[] } = {}) {
+function buildServer(opts: { ql?: any; objects?: any[]; readonlyFields?: string[] } = {}) {
   const server = mockServer();
   const protocol: any = {
     getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', endpoints: {} }),
     getMetaTypes: vi.fn().mockResolvedValue([]),
     getMetaItems: vi.fn().mockResolvedValue(opts.objects ?? []),
+    createData: opts.ql
+      ? makeCreateData(opts.ql, { readonlyFields: opts.readonlyFields ?? [] })
+      : vi.fn(),
   };
   const objectQLProvider = opts.ql ? async () => opts.ql : undefined;
   const rest = new RestServer(
@@ -219,6 +248,103 @@ 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');
+  });
+
+  // ── create ingress parity (#3835) ─────────────────────────────────────────
+  //
+  // The engine's INSERT path is static-`readonly`-exempt by design (#3413), so
+  // the #3043 strip that stops a non-system caller from seeding a read-only
+  // column lives at the protocol's create ingress. This route used to call
+  // `ql.insert` directly and skip it, so `readonly` meant two different things
+  // depending on which create endpoint you used.
+
+  it('routes create ops through the protocol create ingress, not ql.insert', async () => {
+    const ql = makeQl();
+    const { route, protocol } = buildServer({ ql });
+    const res = await post(route, {
+      operations: [{ object: 'account', action: 'create', data: { name: 'Acme' } }],
+    });
+    expect(res.statusCode).toBe(200);
+    expect(protocol.createData).toHaveBeenCalledTimes(1);
+    const request = protocol.createData.mock.calls[0][0];
+    expect(request).toMatchObject({ object: 'account', data: { name: 'Acme' } });
+    // The ingress must run INSIDE this transaction — same context the other ops
+    // bind, or the create would commit independently of the rollback.
+    expect(request.context).toMatchObject({ __trx: true });
+  });
+
+  it('strips a caller-forged readonly column on a batch create and reports it', async () => {
+    const ql = makeQl();
+    const { route } = buildServer({ ql, readonlyFields: ['approval_status'] });
+    const res = await post(route, {
+      operations: [
+        { object: 'account', action: 'create', data: { name: 'Acme', approval_status: 'approved' } },
+      ],
+    });
+    expect(res.statusCode).toBe(200);
+    // Never reached the engine…
+    expect(ql.insert).toHaveBeenCalledWith('account', { name: 'Acme' }, expect.anything());
+    // …and the caller is told, tagged with the op that dropped it.
+    expect(res.body.droppedFields).toEqual([
+      { object: 'account', fields: ['approval_status'], reason: 'readonly', index: 0 },
+    ]);
+  });
+
+  it('keeps $ref resolution working through the ingress', async () => {
+    // `results` now holds the ingress's echoed record rather than the raw
+    // engine return — a later op must still resolve `{ $ref: 0 }` to its id.
+    const ql = makeQl();
+    const { route } = buildServer({ ql });
+    const res = await post(route, {
+      operations: [
+        { object: 'project', action: 'create', data: { name: 'Apollo' } },
+        { object: 'task', action: 'create', data: { title: 'Kickoff', project: { $ref: 0 } } },
+      ],
+    });
+    expect(res.statusCode).toBe(200);
+    expect(res.body.results[0].id).toBe('id_1');
+    expect(ql.insert).toHaveBeenLastCalledWith(
+      'task', { title: 'Kickoff', project: 'id_1' }, expect.anything(),
+    );
+  });
+
   // ── request validation ────────────────────────────────────────────────────
 
   it('rejects a non-atomic request (this endpoint is always atomic)', async () => {