fix(metadata-protocol): insertManyData reports the dropped-field union at BATCH level instead of naming rows it cannot identify - #17688
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RuoNSXUbBoWHkNS4AknTrM
📓 Docs Drift CheckThis PR changes 3 package(s): 8 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 4 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 137 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin a5a07925fba72237d6ad15f3d842a05b971756fb && git checkout a5a07925fba72237d6ad15f3d842a05b971756fb
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 6465cc0a7c83ba60503bfd4ddd907d35220e4244 a38c0baab57a287c6e022acfff59d1147222fb75 && git checkout -B drift-repro 6465cc0a7c83ba60503bfd4ddd907d35220e4244 && git merge --no-ff a38c0baab57a287c6e022acfff59d1147222fb75
node scripts/docs-audit/affected-docs.mjs --json 6465cc0a7c83ba60503bfd4ddd907d35220e4244
|
…sertmany-dropped-field-row-attribution
Contract reviewHead reviewed: ① Derived judgments
② Semver grading
③ Boundary flagsNo ⭐ Triage's routing line was independently measured wrong by the round too — it named
Two
Independence pair
Verdict✅ PASSLanding pre-checks, all three by this seat: ① this comment · ② ⇒ Both carriers stripped in this same stroke.
Generated by Claude Code |
Fixes #17290
Clause-②: yesinsertManyDataattributed a batch-union dropped-field name to every row that supplied it. Maintainer ruling C's per-row hook exemption falsifies that inference, so the union is now reported where it is true — on the response — and no row is named.The defect, and why the inference cannot be rescued
Every create-side strip is the engine's, and its
onFieldsDroppedevent is the UNION over the batch; the listener signature carries no row index. This seam reconstructed a row set from that union:on the stated premise that "the strip only removes keys the ROW ITSELF supplied". Ruling C (#14147) put the static-
readonlystrip INSIDEengine.insert, AFTER thebeforeInserthooks, where it exempts keys a hook itself assigned — recorded per row (packages/objectql/src/engine.ts,hookWrittenKeys: rowHookWrittenKeys[i];packages/objectql/src/validation/rule-validator.ts:1268,if (hookWrittenKeys?.has(name)) continue;). "Supplied N" and "dropped N" are therefore different sets, in two directions:droppedFieldsnaming it — on an outcome whoserecordcarries the written value;ok: false) dropped nothing at all, and supplying the name was enough to have it named.The outcome's own
recordcannot repair it, which is why the card's suggested post-hoc check is not the cheaper route. A strippedreadonlyfield is RE-DEFAULTED over exactly the keys the strip took (engine.ts, #3043's contract), and a strippedautonumberis refilled byapplyAutonumbersafterwards — so the key is PRESENT on the row that really did drop it. Measured, inengine-autonumber-runtime-owned.test.tsagainst a real engine: both outcomes come back carryingaccount_number(['ACC-0001', 'ACC-0002']) when only the second row forged one. A "is the key still there?" check would delete TRUE attributions and leave the hook-exempt false one standing. Comparing values fails on the exact casehookWrittenKeyswas built for — the hook assigning the value the caller also sent.⇒ Per triage: attribute per row, or say plainly that the set is batch-level and name no rows — and ⛔ never keep row names under a caveat. The honest set (
{rows whose payload carried N}minus{rows whose beforeInsert hook assigned N}) is computed per row upstream and does not cross this seam, so the second exit is the correct one. Restoring row precision would mean giving the engine's drop report a per-row channel, not a reconstruction at the call site.The change
The events are now collected whole and merged the way
createManyDatamerges them, which also restores each event's ownreason(the old flatten-into-a-Set relabelled everything'readonly').updateManyDataandbatchDatakeep per-rowdroppedFieldsuntouched — they earn it mechanically, one engine call per row.The docblock dies with the code. Enumerated by CLAIM rather than by spelling — the census instruction #16933's dispatch introduced (ruling 5), which is what produced this card in the first place:
metadata-protocol/src/protocol.ts—insertManyDatadocblockmetadata-protocol/src/protocol.ts—createManyDatacommentinsertManyData"recovers row precision from the same union"metadata-protocol/src/protocol.ts—mergeDroppedFieldEventsdocblockinsertMany/batchpaths carry their own per-rowdroppedFields"metadata-protocol/src/protocol.dropped-fields.bulk.test.ts— headerinsertManyDatarowobjectql/src/engine.ts—insertManydocblockinsertManyDatadoes exactly that)"spec/src/api/protocol.zod.ts—CreateManyDataResponseSchema.droppedFields.describe()This agrees with #16933's landed prose correction (PR #17293, commit
439331895), which is the card triage said to read first: that PR kept the{ object, records, count }"no per-row slot" reason and corrected only the falsified one. Located by content —git log -S 'schema-uniform' -- packages/metadata-protocol/src/protocol.ts; the PM'sgit log --grep=16933zero was a dead instrument, since squash subjects need not carry the number.The pins are REPLACED, not amended
Three pin sites restated the same inference in their own comments and titles, and their engine doubles had no hook-write concept at all — looser than the producer, so each was green through exactly the shape it was written for. The double in
protocol.dropped-fields.bulk.test.tsnow models ruling C's exemption and the re-default, and the block gains the case the card asked for: a hook exempting one row of a batch.Verification
Head at measurement:
a38c0baab5.origin/mainmoved under this branch mid-run (49cd71548e→6465cc0a7c) and was merged in, never rebased; the new commit touchespackages/plugins/plugin-approvalsonly, so the serial file has no second edit on it. Build and test run throughscripts/pm/os-verify-lock.sh; every verdict below is quoted from the gate's own line, never from a bare$?after a pipe.pnpm --filter '@objectstack/metadata-protocol...' --filter '@objectstack/objectql...' --filter '@objectstack/spec...' build --concurrency=2—VERDICT command-exit 0... run typecheckon the three packages —VERDICT command-exit 0pnpm --filter @objectstack/metadata-protocol run test—Test Files 176 passed | 2 skipped (178)·Tests 2529 passed | 12 skipped (2541)pnpm --filter @objectstack/objectql run test—Test Files 296 passed (296)·Tests 4959 passed (4959)pnpm --filter @objectstack/spec run test—Test Files 473 passed (473)·Tests 13432 passed (13432)Tests 22 passed (22)+Tests 32 passed (32)pnpm exec eslint . --no-inline-config --format jsonata38c0baab5— 6633 files linted (population read from eslint's own config, count read from the JSON report), 0 errors, 0 warningsscripts/pm/dispatch-gates.mjs --commandsthen--ranwith an exit code recorded per family —112 derived famil(ies) accounted for — 111 run, 1 NOT-MEASUREDcheck:type-check-coverage—OK — 76/80 workspace packages type-checked;check:type-check-debt—OK — 5 ledger entr(ies) re-measured in 142.0s, 55 raw tsc error(s) total, none above its recorded numberNOT MEASURED:
pnpm check:dual-build-cjs-loads, reason: exit 3 PREREQUISITE NOT MET — it reads built output and 8 packages in this checkout have nodist/; clearing it needs the whole-tree build that CI'sBuild Corejob performs. ⛔ Recorded as not measured, never as a pass.Three gates needed work rather than just a run, and each is in the diff:
check:docswent red because a.describe()moved:content/docs/references/api/protocol.mdxis generated and is regenerated here (gen:schema+gen:docs, run on a committed non-merge state;authorable-surface.base.jsonis untouched, and the regeneration is a one-line change).check-adr-0087-registrationdemanded a disposition for the**BREAKING**token. The changeset carriesnot-required (no-migration-prescription)with its argument: no authorable key, no Zod schema and no stored shape moves —packages/specdeclares no response schema for this face at all — soobjectstack migrate metahas nothing to visit and the compiler at the consumer's own call site is the channel. The changeset therefore describes the break in prose rather than as a rewrite table, which is the shape this repo already uses for a compiler-carried break.check-engine-split-ratiorefused on a shallow clone (exit 2, "a ratio derived here would be real, plausible and WRONG"). Deepened per its own remedy (git fetch --shallow-since=2026-06-06) and re-run green.Reverse verification (one-off, restored)
With the repair committed, the pre-repair reconstruction was re-injected into
packages/metadata-protocol/src/protocol.tsbehind a unique marker and the pins re-run. The tests import./protocol.jsrelative to the same package, so vitest resolves the mutation fromsrcand no rebuild is involved.1; mutated bloba9a4d568…differs from theHEADblob7b43df47…Test Files 2 failed (2)·Tests 4 failed | 18 passed (22), including the defect itself —AssertionError: a written value must never be reported as dropped: expected { ok: true, …(2) } to not have property "droppedFields"/Received: [{ fields: ["approval_status"], object: "approval_case", reason: "readonly" }]EXIT INT TERMtrap viagit checkout HEAD -- PATH:git diff HEADempty, current blob7b43df47…equals theHEADblob, marker residue0Acceptance notes
packages/objectql/ the create path; the repair lands inpackages/metadata-protocol. Confirmed independently:insertManyDatais defined atpackages/metadata-protocol/src/protocol.ts, andpackages/objectql/src/engine.tscarried only a comment about it. The twopackages/objectqlfiles here are a falsified docblock and a pin that asserted the removed behaviour — consequences of the repair, not a widening of it.packages/rest/src/import-runner.ts, does not readdroppedFields: its owninsertManyDatainterface declaresoutcomes: Array<{ ok, record?, error? }>and the call site reads only.outcomes.Clause-②isyeson the mechanical floor anyway — a new key on a published payload.packages/specdeclares no Zod schema for theinsertManyDataresponse at all (onlyCreateManyDataResponseSchemaprose mentions it), so this surface's shape is carried by TypeScript alone. Successor: none identified — flagged for the seat rather than filed, since "should this face have a declared response schema" is a contract question and not a defect I measured.🤖 Generated with Claude Code
https://claude.ai/code/session_01RuoNSXUbBoWHkNS4AknTrM
Generated by Claude Code