fix(metadata-protocol,spec): preserveAudit is UPDATE-only — narrow the contract and warn loudly on a non-system INSERT (#6640) - #6823
Merged
Conversation
…ly; regenerate field reference
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 112 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
github-actions
Bot
added
documentation
os-zhuang
marked this pull request as ready for review
August 8, 2026 23:39
This was referenced Aug 9, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6640
What was wrong
FieldSchema.readonly's.describe()promised the opt-in historical-import exemption (preserveAudit, #3493) on both write paths, andcontent/docs/protocol/objectql/security.mdx:264agreed. Only UPDATE ever implemented it:stripReadonlyFieldsin objectql's rule-validator consultsisPreservableUnderAudit(packages/objectql/src/validation/rule-validator.ts:1149). ReadspreserveAudit.stripReadonlyForInsertat the DataProtocol ingress (packages/metadata-protocol/src/protocol.ts) never hadpreserveAuditin its signature at all;context.isSystemwas its only exemption. Verified onorigin/main:git grep preserveAudit packages/metadata-protocol/src/protocol.tsreturned zero hits.REST import's
treatAsHistoricalputspreserveAudit: trueon the write context (packages/rest/src/import-runner.ts:407) and creates throughp.createData(:641/:762) — i.e. through exactly that ingress. So one historical import kept an author-declaredreadonlybusiness column (closed_at,resolved_by) on the rows it UPDATED and silently dropped it from the rows it CREATED.The trigger is not exotic. The audit family itself is
readonly: truein the registry'sAUDIT_FIELD_DEFS(packages/objectql/src/registry.ts:262), so an ordinary export → historical-import round-trip carries readonly columns on every row.All three halves of the issue's premise verified on
origin/mainbefore implementing; all three held.What this does — maintainer ruling of 2026-08-08 (option 2 + binding loudness rider)
1. Contract narrowed to the truth. The
.describe()text and the security doc now state the exemption as UPDATE-path only. The INSERT entry keeps honouringisSystemalone; replaying archival readonly facts on create requires the system context. HonouringpreserveAudithere instead would have handed a NON-system caller —treatAsHistoricalarrives on an ordinary REST import request — the approval/status columns #3043 exists to protect, in a single POST.2. The ignored request stops being silent. A non-system INSERT that requests
preserveAuditand actually loses fields now logs aWARNnaming the object, every stripped field, the UPDATE-only rule, and theisSystemremedy. It fires once per ingress call (the union across a batch, the same aggregationmergeDroppedFieldEventsalready applies), and only when something was really removed — an ordinary create that never asked for the exemption stays exactly as quiet as #3043 designed it. That specificity is the point: the drop itself already surfaces throughdroppedFields(#3431), but a caller who explicitly requested the exemption could not tell "your fields were stripped by the ordinary rule" from "the exemption you requested does not exist on this path". The warning says the second one, by name.Because the signal lives inside
stripReadonlyForInsert, all five ingress sites (createData,cloneData,batchDatacreate,createManyData,insertManyData) get it by construction rather than by five copies.No behaviour change to the strip itself, and no acceptance-surface change —
check:authorable-surfaceis byte-identical (working tree clean after the run). The spec diff is the describe string only, plus the whole-file regeneration ofcontent/docs/references/data/field.mdxthatcheck:docsdemands.Why a WARNING and not a throw — measured, not assumed
The ruling made loudness binding and left the shape to whichever can be both loud and non-breaking. A throw cannot be.
runImport's per-row writer collects a write error intotoFailedResult(rowNo, res.error)rather than aborting, so refusing at the ingress would not stop a historical import — it would convert every row it CREATES into a failed row while the rows it updates still succeed.Measured on this branch with a throwing variant of the same condition, driving the real
runImport→ real ingress with 2 new rows:{created: 2, errors: 0}→{created: 0, errors: 2}. Breaking the shippedtreatAsHistoricalflow for new rows is precisely the condition under which the ruling names the loud WARNING — strip still applied — as the containment-correct landing. So: loud warning, and this is the PR body saying so, as the ruling requires.Family precedent #5714/#5931 rejects outright, and the difference is measured rather than assumed: those two judge AUTHORING input, before anything runs. This one sits on a live write path, which is what moves it from throw to warn.
Tests — through the REAL entry, per the binding test note
The pre-existing
preserveAuditpins all driveengine.insertdirectly and therefore cannot see this ingress; that is how the gap survived. Every new pin goes through the shipped entry.packages/metadata-protocol/src/protocol.readonly-insert.test.ts(+6): non-system INSERT withpreserveAudit→ stripped AND one warning carrying the field names, the UPDATE-only rule and the remedy; system INSERT unchanged; ordinary non-preserveAuditcreate still silent;preserveAuditthat loses nothing still silent;createManyDatawarns once with the union across rows;batchDatacreate carries the same signal.packages/rest/src/import-runner-historical-readonly-insert.test.ts(new): the historical import end to end — realObjectStackProtocolImplementationover a mock engine, so the runner, the ingress and the signal are all shipped code and only storage is faked. Pins that the import still creates every row, that the strip is unchanged, that the signal fires, and that the system-context run replays the archival value.Reverse verification — direction predicted first
Predicted: removing the warn call (restoring the pre-fix silence) turns the loudness pins red and leaves every strip pin green, because this PR adds a signal and does not change the strip. Measured, exactly that:
Note the honest shape: the three "stays silent" / system-context pins stay green under the revert, and they should — they assert absence, which the pre-fix code also satisfies. They earn their place against the opposite regression (a warning that fires too widely), not against this one.
Gates run locally, foreground
File surface
Stayed inside the declared surface. Round-4 siblings on
protocol.ts— #6780 (:7829) and #6710 (:2426) — are untouched; this change sits at ~:1108.Generated by Claude Code