fix(metadata-protocol): insertManyData reports the dropped-field union at BATCH level instead of naming rows it cannot identify

.changeset/17290-insertmany-dropped-fields-name-no-row.md

@@ -0,0 +1,69 @@
+---
+'@objectstack/metadata-protocol': minor
+'@objectstack/objectql': patch
+'@objectstack/spec': patch
+---
+
+fix(metadata-protocol): `insertManyData` reports the dropped-field union at BATCH level instead of naming rows it cannot identify (#17290)
+
+<!-- adr-0087: not-required (no-migration-prescription) nothing authored or stored moves: no authorable key, no Zod schema and no stored `sys_metadata` shape changes — `packages/spec` declares no response schema for this face at all, so `objectstack migrate meta` has nothing to visit, `spec-changes.json` has nothing to project and the upgrade guide gains no row. What moves is one optional member on an inline TypeScript response type of a runtime protocol method, and the channel that reaches every affected consumer is the compiler at their own call site, which names the site more precisely than a ledger line could. The `packages/spec` file in this diff is a `.describe()` STRING — customer-facing prose that this change would otherwise leave false — not a schema, a key or an accept set; nothing it declares moves. -->
+
+**BREAKING** — `@objectstack/metadata-protocol`'s `insertManyData` no longer hangs
+`droppedFields` on each entry of `outcomes`; the response itself carries it, beside
+`outcomes`, exactly as `createManyData` already does. A TypeScript consumer that read
+the per-row member stops compiling, and the compiler names the site. The set reported
+is the same set — what is gone is a per-row attribution that could not be computed
+here and was wrong whenever it mattered. Nothing authored or stored changes shape.
+
+**What it got wrong.** Every create-side strip is the engine's, and its
+`onFieldsDropped` event is the UNION over the batch — the listener signature
+carries no row index. This seam reconstructed a row set from that union by
+asking which rows SUPPLIED each dropped name
+(`[...engineDropped].filter((f) => f in supplied)`), on the stated premise that
+"the strip only removes keys the ROW ITSELF supplied, so a dropped name belongs
+to exactly the rows whose supplied payload carried it". Maintainer ruling C
+falsifies the premise: the static-`readonly` strip runs INSIDE `engine.insert`,
+AFTER the `beforeInsert` hooks, and exempts keys a hook itself assigned —
+recorded per row (`hookWrittenKeys: rowHookWrittenKeys[i]`). So in a batch where
+a hook stamps a protected key on some rows and not others:
+
+- row A supplied `approval_status`, no hook write ⇒ stripped, enters the union;
+- row B supplied `approval_status`, its hook re-assigned it ⇒ **kept and
+  written**;
+- and row B's outcome carried `droppedFields: [{ fields: ['approval_status'] }]`
+  on a record that still held `approval_status`.
+
+A row the batch culled before the strip ran (a per-row validation failure) was
+named on the same test, having dropped nothing at all.
+
+⇒ A wrong attribution costs the reader a wrong investigation, and the import
+surface — which prefers this path over `createManyData` — is the consumer most
+likely to act on it while reconciling what landed.
+
+**Why not attribute per row instead.** The honest set is `{rows whose payload
+carried N}` minus `{rows whose beforeInsert hook assigned N}`, and the second
+half is computed per row upstream but does not cross this seam. The outcome's
+own `record` cannot stand in for it: a stripped `readonly` field is RE-DEFAULTED
+over exactly the keys the strip took, and a stripped `autonumber` is refilled by
+`applyAutonumbers` — so on both, the key is PRESENT on the row that really did
+drop it, and a post-hoc "is the key still there?" check would delete true
+attributions while leaving the hook-exempt false one standing. Comparing values
+fails on the very case `hookWrittenKeys` exists for: the hook assigning the
+value the caller also sent. Restoring row precision means giving the engine's
+drop report a per-row channel, not a reconstruction at the call site.
+
+**Prose corrected with it**, by CLAIM rather than by spelling — the docblock
+that authorised the inference is the thing that re-authorises the next author:
+`insertManyData`'s own docblock and `createManyData`'s parenthetical
+(`@objectstack/metadata-protocol`), `mergeDroppedFieldEvents`'s closing
+sentence, `engine.insertMany`'s docblock claim that "a caller holding the input
+rows can attribute each name back to the rows that carried it"
+(`@objectstack/objectql`, TSDoc emitted into its published `.d.ts`), and
+`CreateManyDataResponseSchema.droppedFields`'s `.describe()` parenthetical
+(`@objectstack/spec`, a string printed AT the customer).
+
+**Unchanged.** `updateManyData` and `batchData` keep per-row `droppedFields`,
+and they always could: each row is its own `engine.update` / `engine.insert`
+call, so that call's events are that row's — earned mechanically, not inferred.
+`createManyData`'s aggregated shape is untouched. No strip changes, no row
+changes, and the same field names are reported.

content/docs/references/api/protocol.mdx

@@ -570,7 +570,7 @@ A write-path strip event: caller-supplied fields legally dropped from the payloa
 | **object** | `string` | ✅ | Object name |
 | **records** | `Record<string, any>[]` | ✅ | Created records |
 | **count** | `number` | ✅ | Number of records created |
-| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when' \| 'primary_key'> }[]` | optional | Write-observability: caller-supplied `readonly` fields the in-engine create-side strip (`engine.insert`, `isSystem`-gated) removed before the rows were written. AGGREGATED across the batch (one event per object/reason with the UNION of dropped field names) rather than per-row, because this response is `{ object, records, count }` and has no per-row slot to hang a drop set on — a union is the only view it can represent. So read a name here as "at least one row dropped this field", NOT "every row dropped the same set": the strip runs INSIDE `engine.insert` after the `beforeInsert` hooks and exempts keys a hook itself wrote, tracked per row: rows where a hook stamped a protected key drop a different set from rows where it did not. Present ONLY when ≥1 field was dropped; the creates still succeeded without them (count/success unchanged). Optional — omit-when-empty keeps the shape backward-compatible. (The per-row `insertMany`/`batch` paths carry per-row `droppedFields` on each result instead — see BatchOperationResultSchema.) |
+| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when' \| 'primary_key'> }[]` | optional | Write-observability: caller-supplied `readonly` fields the in-engine create-side strip (`engine.insert`, `isSystem`-gated) removed before the rows were written. AGGREGATED across the batch (one event per object/reason with the UNION of dropped field names) rather than per-row, because this response is `{ object, records, count }` and has no per-row slot to hang a drop set on — a union is the only view it can represent. So read a name here as "at least one row dropped this field", NOT "every row dropped the same set": the strip runs INSIDE `engine.insert` after the `beforeInsert` hooks and exempts keys a hook itself wrote, tracked per row: rows where a hook stamped a protected key drop a different set from rows where it did not. Present ONLY when ≥1 field was dropped; the creates still succeeded without them (count/success unchanged). Optional — omit-when-empty keeps the shape backward-compatible. (The same reading applies to the partial-success bulk create, whose batch-level `droppedFields` names no row for the same reason. The paths that DO carry per-row `droppedFields` on each result are the bulk UPDATE and the mixed batch, where each row is its own engine call — see BatchOperationResultSchema.) |
 
 ### Nested Shape: `CreateManyDataResponse.droppedFields[number]`
 

packages/metadata-protocol/src/protocol.dropped-fields.bulk.test.ts

@@ -7,13 +7,16 @@
 // single-write now reports, and (b) thread the caller's execution `context` to
 // the engine so RLS/FLS/`readonlyWhen` run under the caller — a gap the
 // pre-#3455 `updateManyData`/`batchData` loops had. Channels:
-//   - updateManyData / batchData → per-row `droppedFields` on each result row;
-//   - insertManyData            → per-row `droppedFields` on each outcome;
-//   - createManyData            → aggregated top-level `droppedFields` (its
-//     response has no per-row slot, so a union is the only view it can
-//     represent; read a name there as "at least one row dropped this field",
-//     never "every row dropped the same set" — ruling C (#14147) exempts keys a
-//     `beforeInsert` hook assigned, recorded per row, so rows CAN differ).
+//   - updateManyData / batchData → per-row `droppedFields` on each result row,
+//     earned mechanically: one engine call per row, so that call's events are
+//     that row's;
+//   - createManyData / insertManyData → aggregated top-level `droppedFields`.
+//     `createManyData`'s response has no per-row slot, so a union is the only
+//     view it can represent. `insertManyData` HAS one (`outcomes[i]`) and still
+//     reports at the top level, because ruling C (#14147) exempts keys a
+//     `beforeInsert` hook assigned — recorded per row — so rows CAN differ and
+//     the union cannot be resolved back to rows. Read a name in either as "at
+//     least one row dropped this field", never "this row dropped it".
 
 import { describe, it, expect, vi } from 'vitest';
 import { assertEngineUpdateDispatch, assertEngineFindOnePredicate } from '@objectstack/metadata-core';
@@ -130,22 +133,48 @@ describe('createManyData — aggregated top-level droppedFields (#3455)', () =>
   });
 });
 
-describe('insertManyData — per-row droppedFields on outcomes (#3455)', () => {
-  it('attaches the create strip to the matching outcome row only', async () => {
-    // [#14147] The engine's listener carries no row index — it reports the
-    // batch UNION — so row precision here is recovered by asking which row
-    // SUPPLIED each dropped name. That recovery is what this case pins.
-    const insertMany = vi.fn(async (object: string, rows: any[], options?: any) => {
-      if (rows.some((r) => r && 'approval_status' in r)) {
+describe('insertManyData — BATCH-LEVEL droppedFields that names no row (#3455)', () => {
+  // This block used to pin the opposite: a per-row `droppedFields` on each
+  // outcome, reconstructed from the batch union by asking which row SUPPLIED
+  // each dropped name. Ruling C (#14147) falsifies that reconstruction — the
+  // strip runs after `beforeInsert` and exempts keys a hook assigned, per row —
+  // so the cases are REPLACED rather than amended. The engine double below
+  // models the exemption, which the old one had no concept of; that is why the
+  // old case stayed green through exactly the shape it was written for.
+
+  /**
+   * `hookStamps` names the rows whose `beforeInsert` hook re-assigns
+   * `approval_status`. Those rows KEEP it (ruling C); the others are stripped.
+   * Either way the engine reports ONE event, the union over the batch, with no
+   * row index — which is the real `engine.insert` contract this stands in for.
+   */
+  function makeInsertMany(hookStamps: ReadonlySet<number> = new Set(), dead: ReadonlySet<number> = new Set()) {
+    return vi.fn(async (object: string, rows: any[], options?: any) => {
+      const strippedAny = rows.some((r, i) => r && 'approval_status' in r && !hookStamps.has(i) && !dead.has(i));
+      if (strippedAny) {
         options?.onFieldsDropped?.({ object, fields: ['approval_status'], reason: 'readonly' });
       }
       return rows.map((r, i) => {
-        const { approval_status: _forged, ...kept } = r ?? {};
-        return { ok: true, record: { id: `rec-${i + 1}`, ...kept } };
+        if (dead.has(i)) return { ok: false, error: { code: 'VALIDATION_FAILED' } };
+        const { approval_status: forged, ...kept } = r ?? {};
+        // A stripped `readonly` field falls back to its `defaultValue` (#3043),
+        // so the key is present on the row that DID lose the caller's value —
+        // the reason a post-hoc check against `outcomes[i].record` cannot
+        // recover row precision either.
+        const value = hookStamps.has(i) ? forged : SCHEMA.fields.approval_status.defaultValue;
+        return { ok: true, record: { id: `rec-${i + 1}`, ...kept, approval_status: value } };
       });
     });
+  }
+
+  function makeProtocol(insertMany: ReturnType<typeof makeInsertMany>) {
     const engine = { registry: { getObject: () => SCHEMA }, insertMany };
-    const p = new ObjectStackProtocolImplementation(engine as any);
+    return new ObjectStackProtocolImplementation(engine as any);
+  }
+
+  it('surfaces the batch union on the response and hangs nothing on any outcome', async () => {
+    const insertMany = makeInsertMany();
+    const p = makeProtocol(insertMany);
 
     const res: any = await p.insertManyData({
       object: 'approval_case',
@@ -156,15 +185,71 @@ describe('insertManyData — per-row droppedFields on outcomes (#3455)', () => {
       context: { userId: 'u1' },
     });
 
-    expect(res.outcomes[0]).not.toHaveProperty('droppedFields');
-    expect(res.outcomes[1].droppedFields).toEqual([
+    expect(res.droppedFields).toEqual([
       { object: 'approval_case', fields: ['approval_status'], reason: 'readonly' },
     ]);
+    for (const o of res.outcomes) expect(o).not.toHaveProperty('droppedFields');
     // [#14147] The ingress hands the caller's row over WHOLE — judging it is
     // the engine's job now, and this assertion is what would catch a
     // reintroduced second strip at this seam.
     expect(insertMany.mock.calls[0][1][1]).toHaveProperty('approval_status');
   });
+
+  it('a hook exempts one row of the batch: the row that KEPT the value is not named', async () => {
+    // Row 0 and row 1 both forge `approval_status`. Row 1's `beforeInsert` hook
+    // re-assigns it, so ruling C keeps row 1's value and only row 0 is
+    // stripped. `f in supplied` — the reconstruction this response shape
+    // replaces — is true for BOTH, so it reported a dropped field on an outcome
+    // whose record carries the value that was written.
+    const p = makeProtocol(makeInsertMany(new Set([1])));
+
+    const res: any = await p.insertManyData({
+      object: 'approval_case',
+      records: [
+        { title: 'A', approval_status: 'approved' },
+        { title: 'B', approval_status: 'approved' },
+      ],
+      context: { userId: 'u1' },
+    });
+
+    expect(res.outcomes[1].record.approval_status, 'the hook wrote it — ruling C keeps it').toBe('approved');
+    expect(res.outcomes[1], 'a written value must never be reported as dropped').not.toHaveProperty('droppedFields');
+    expect(res.outcomes[0], 'and the stripped row is not named either — the set is batch-level')
+      .not.toHaveProperty('droppedFields');
+    expect(res.droppedFields).toEqual([
+      { object: 'approval_case', fields: ['approval_status'], reason: 'readonly' },
+    ]);
+  });
+
+  it('a row the batch culled supplied the name and is still not named', async () => {
+    // The engine's strip loop skips a row that already failed, so a dead row
+    // dropped nothing — but it supplied the key, which was enough for the
+    // reconstruction to name it on an outcome that carries no record at all.
+    const p = makeProtocol(makeInsertMany(new Set(), new Set([0])));
+
+    const res: any = await p.insertManyData({
+      object: 'approval_case',
+      records: [
+        { title: 'A', approval_status: 'approved' },
+        { title: 'B', approval_status: 'approved' },
+      ],
+    });
+
+    expect(res.outcomes[0].ok).toBe(false);
+    expect(res.outcomes[0]).not.toHaveProperty('droppedFields');
+    expect(res.droppedFields).toEqual([
+      { object: 'approval_case', fields: ['approval_status'], reason: 'readonly' },
+    ]);
+  });
+
+  it('nothing dropped ⇒ the key is absent, keeping the omit-when-empty shape', async () => {
+    const p = makeProtocol(makeInsertMany());
+    const res: any = await p.insertManyData({
+      object: 'approval_case',
+      records: [{ title: 'A' }],
+    });
+    expect(res).not.toHaveProperty('droppedFields');
+  });
 });
 
 describe('batchData — per-row droppedFields + context threading (#3455)', () => {

packages/metadata-protocol/src/protocol.readonly-insert.test.ts

@@ -216,27 +216,32 @@ describe('#14147 — the create ingress DELEGATES the readonly strip to engine.i
     ]);
   });
 
-  it('insertManyData forwards every row whole and keeps ROW precision from the union', async () => {
+  it('insertManyData forwards every row whole and reports the union at BATCH level', async () => {
     const { p, inserts } = makeProtocol();
     const res: any = await p.insertManyData({
       object: 'approval_case',
       records: [{ title: 'A', approval_status: 'approved' }, { title: 'B' }],
     });
     expect(inserts[0].data).toEqual([{ title: 'A', approval_status: 'approved' }, { title: 'B' }]);
-    // The engine's event is the batch UNION (its listener carries no row
-    // index); row precision is recovered by asking which row SUPPLIED the key.
-    expect(res.outcomes[0].droppedFields).toEqual([
+    // The engine's event is the batch UNION and its listener carries no row
+    // index. It used to be resolved back to rows by asking which row SUPPLIED
+    // each name; ruling C (#14147) exempts keys a `beforeInsert` hook assigned,
+    // per row, so that is a different set. The union is reported where it is
+    // true — on the response — and no outcome is named.
+    expect(res.droppedFields).toEqual([
       { object: 'approval_case', fields: ['approval_status'], reason: 'readonly' },
     ]);
-    expect(res.outcomes[1].droppedFields, 'row B supplied none of the dropped names').toBeUndefined();
+    for (const o of res.outcomes) {
+      expect(o, 'the union names no row').not.toHaveProperty('droppedFields');
+    }
   });
 });
 
 describe('#14147 — engine listener wiring (the firing control for every assertion above)', () => {
   // The faces enumerated here are the ones whose RESPONSE carries
   // `droppedFields`: `CreateDataResponse`, `CloneDataResponse` (since #15703),
-  // `CreateManyDataResponse`, and the per-row results of `insertManyData` /
-  // `batchData`. That is every create face; the case after this one pins the
+  // `CreateManyDataResponse`, `insertManyData`'s batch-level key, and the
+  // per-row results of `batchData`. That is every create face; the case after this one pins the
   // clone by name so the enumeration cannot silently lose the face that was
   // the exclusion until its contract gained the member.
   it('every create face whose response carries droppedFields passes an onFieldsDropped listener to the engine', async () => {