Repository navigation
Description
Split out of #8093 rather than fixed there, because the fix is a judgement call about a shared strip helper and #8093's scope fence is the report channel.
What #8093 fixed, and what it left
#8093 stopped droppedFields / onFieldsDropped naming the row's own primary key on a by-id update: the payload id that equals the bound row is the write's address, not payload, so it is not a drop. That closed the user-facing amber toast.
The log channel still says the untrue thing. stripReadonlyFields (packages/objectql/src/validation/rule-validator.ts) is unchanged by that PR — deliberately, since the strip itself must keep removing id from the SET clause — and it emits its readonlyStripWarning whenever it deletes a key.
Measured, after #8093's fix
A real ObjectQL + a real ObjectStackProtocolImplementation, updateData({ object: 'sys_user_preference', id: 'rec_1', data: { value: ['x'] } }) — a body with no id key:
droppedFields : null ← #8093's fix, correct
warn count : 1
WARN: Field 'id' on 'sys_user_preference' is read-only: the caller-supplied
value was DROPPED and the update is being COMMITTED WITHOUT IT — ...
Every one of those three claims is false for this write. The value was not caller-supplied (metadata-protocol's updateData folded the path id in, #6479); nothing the caller wanted was dropped; and the update was not "committed without" anything it asked for.
Why it is worth a card and not a shrug
- Volume. It fires on every single-record
PATCHof every object declaringidasreadonly: true— which is every platform object. The console's recents trace alone emits one per org switch. This is the log-side version of the exact "drown the real signal" cost sys_user_preference update reports droppedFields:[id] for a key the caller never sent — every org switch pops a user-facing warning toast #8093, rest: PATCH /data 对被静默剥离的写入字段无任何回传 — onFieldsDropped 通道未接线(follow-up #3407/#3413) #3431 and 审批场景下记录可写性的反馈全线失真:能改的显示「已锁定」,改不了的提示「更新成功」 #3794 were about;readonlyStripWarning's own doc argues it stays atwarnprecisely so real forgery attempts are visible, and this buries them. - The remedy prose is wrong here. The message tells the reader to pass
{ context: { isSystem: true } }. Following that advice for this write would exempt the caller from the static read-only strip entirely — a strictly worse posture, adopted to silence a line that should never have printed.
What the fix is NOT
⛔ Not "stop stripping id". The strip must stay: a same-value primary-key write is a harmless no-op on SQL but an outright rejection on stores with immutable primary keys, and #6435's block already ruled that widening the strip to the truthy-scalar data.id case "is a separate decision, not a rider here".
Options for whoever takes it
- A. Teach
stripReadonlyFieldswhich key is the write's address (an option alongsidepreserveAudit) and skip only the log line for it. Contained, but adds a parameter to a helper shared by the by-id branch, the multi branch and the insert-side sibling. - B. Have the by-id branch remove the addressing
idbefore the read-only pass, as addressing rather than as a read-only strip — no strip, no warn, no report, one concept in one place. Bigger blast radius: it changes what the driver receives for objects whoseidis not readonly, which isupdate的 **by-id** 路径同样把非标量data.id交给驱动写主键列(#6262 的孪生形状,where.id胜出时) #6435's explicitly separate decision. - C. Leave it and accept the log noise.
Recommendation from the #8093 lane: A, with B filed as the follow-up if the "addressing is not payload" idea is worth unifying across both channels.
Pointers
packages/objectql/src/validation/rule-validator.ts—stripReadonlyFields,readonlyStripWarningpackages/objectql/src/engine.ts—idAddressesThisRow, the sys_user_preference update reports droppedFields:[id] for a key the caller never sent — every org switch pops a user-facing warning toast #8093 predicate that already computes "thisidis the address"packages/spec/src/contracts/data-engine.ts—WriteObservabilityOptions, where sys_user_preference update reports droppedFields:[id] for a key the caller never sent — every org switch pops a user-facing warning toast #8093 wrote the invariant down
Filed unassigned by the #8093 dev lane (seat #6019, domain:engine-core).
Activity
hotlong commentedon Aug 12, 2026
Triage: lands in
packages/objectql(rule-validator.tsstripReadonlyFields/readonlyStripWarning) ⇒domain:engine-core, type Bug,pm:queue. The log line makes three measured-false claims on every single-record PATCH of every platform object, and its remedy prose steers callers towardisSystem: true— advice that would weaken posture to silence a line that should not print. That is a concrete defect against the warning's own documented purpose (keeping real forgery attempts visible), not a design question.Scope note for the dispatch: option A (teach the helper which key is the write's address, skip only the log line) is the queued scope, per the filing lane's own recommendation — the
idAddressesThisRowpredicate from #8093 already computes the needed fact. ⛔ Option B (remove the addressingidbefore the read-only pass) changes what drivers receive and is #6435's explicitly separate decision — file it as a follow-up if wanted, do not fold it in. The strip itself must keep removingid.Size/model suggestion: S–M / opus (shared-helper parameter design, three call sites).
Generated by Claude Code
os-zhuang commentedon Aug 12, 2026
Claim: PM loop,⚠️
domain:engine-coreseat (#6019)Session:
session_014C8pAprWdmtecFsEprZax4Branch:
claude/issue-8141-readonly-strip-address-warnWorktree:
objectstack-issue-8141Domain:
domain:engine-coreFile surface:
packages/objectql/src/validation/rule-validator.ts(stripReadonlyFields/readonlyStripWarning), the by-id call site inpackages/objectql/src/engine.tsthat must pass the address through, their tests, and a changeset.packages/spec/src/contracts/data-engine.tsonly if the #8093 invariant text needs extending — doc comment only, ⛔ no schema change (cross-seat, see below).Container & model: single container,
model: opus— matching triage's own S–M/opus read. The fable clause does not fire, and the reason is worth stating because the card sits next to a contract: Option A changes only which key the log line names. The strip keeps removingid, the driver receives exactly what it receives today, and no write that succeeds now begins to fail (or vice versa). No accept/reject behaviour moves, no public face widens.Serial constraints — derived from real diffs, not from symbol addresses
rule-validator.ts— free. Nothing in flight holds it. (A number field's declaredscaleis never enforced — values with more decimals are accepted and stored verbatim (min/max on the same field are enforced) #7501 will touchrecord-validator.ts, a different file, and it ispm:blockedon the Fable quota with no branch.)engine.ts— free in practice. finding: SqlDriver's #3821 recovery ladder widens an unresolvable projection to every field #7589's ruled landing (theknown.has(head)projection filter) is reserved on the seat post but ispm:blockedwith no branch and no PR, so there is no diff to collide with; it is also a different function from the by-id write path.function, un-erased (#8112) #8206 ([finding] Two objectql engine test fixtures buildaggregations: [{ func: … }]behind anas any— the declared key isfunction, so the aggregation they construct is silently dropped #8112), touches two test files and a ratchet baseline. Disjoint.Scope — ruled, not open
Triage queued option A: teach
stripReadonlyFieldswhich key is the write's address (an option alongsidepreserveAudit) and skip only the log line for it.⛔ Option B is explicitly out — removing the addressing
idbefore the read-only pass changes what the driver receives for objects whoseidis not readonly, and #6435 already ruled that a separate decision. ⛔ Do not fold it in because it looks cleaner from inside the code. If the "addressing is not payload" idea deserves unifying across both channels, that is a follow-up card, and filing one is welcome.⛔ "Stop stripping
id" is not on the table. A same-value primary-key write is a harmless no-op on SQL and an outright rejection on stores with immutable primary keys.What makes this fix correct rather than merely quiet
The predicate you need already exists:
idAddressesThisRowinengine.ts, written by #8093 (PR #8143, landeda0fdc564e) — the payloadidthat equals the bound row is the write's address, not payload. ⭐ Reuse that fact; do not re-derive a second, subtly different notion of "this id is the address." Two predicates that disagree in one edge case is a worse defect than the log line.warnfor exactly that reason. So the acceptance bar is a pair, both pinned:id(the card's measured repro,sys_user_preference/rec_1) ⇒ no WARN;idthat does not equal the bound row, and a caller-supplied value on some otherreadonlyfield ⇒ WARN still fires, unchanged in wording and level.⛔ A patch that makes (1) green without proving (2) is not acceptable — that is the difference between fixing a false claim and deleting a tripwire. Report the measurement for both, and reverse-verify: predict the direction before you run it.
Also check the third claim in the message while you are there. The card measures all three as false for this write ("caller-supplied", "DROPPED", "COMMITTED WITHOUT IT"), and the remedy prose steers the reader to
{ context: { isSystem: true } }— advice that would exempt the caller from the static strip entirely, a strictly worse posture adopted to silence a line that should never have printed. If the line still prints for genuine cases, its wording and its remedy should be true for those cases; say what you changed and why.Cross-seat
packages/spec/src/contracts/data-engine.tsisdomain:spec's surface (seat #6017). #8093 already extendedWriteObservabilityOptions' invariant text there with a declaration filed by this seat. If your change needs that text extended again, keep it to a doc comment, say so in the report, and I will file the declaration with a stated deadline — ⛔ do not change a schema or a type in that package on this card.Gates
Derive with
node scripts/pm/dispatch-gates.mjs <changed paths>— ⛔ never hand-enumerated.check:type-check-debt --re-measureneeds a built closure. ⛔ Baselines may only shrink;--loweris never run. Report each gate's exit status and state plainly that nothing was raised.Deliverable
Push early — even a WIP scaffold commit — then a draft PR whose first line is
Fixes #8141. ⛔ Do not flip to ready, ⛔ do not enable auto-merge; the PM does that after reading each job's ownconclusion.Generated by Claude Code
test(objectql): pin the address-key log exemption and the tripwire it…
os-zhuang commentedon Aug 12, 2026
{ "issue": 8141, "status": "done", "branch": "claude/issue-8141-readonly-strip-address-warn", "pr": "https://github.com/objectstack-ai/objectstack/pull/8216", "premise_still_valid": true, "summary": "Option A, exactly as ruled. `stripReadonlyFields` gains `options.addressKey`: the named key is still STRIPPED, it just no longer logs — placed after the `delete`, never as an early `continue`, so the driver's SET clause is byte-identical. The by-id update branch passes it from the SAME `idAddressesThisRow` predicate #8093 wired to `reportDroppedFields` (`addressKey: idAddressesThisRow ? 'id' : undefined`), so one predicate now feeds both channels and no second notion of `this id is the address` exists. Option B was not folded in and `id` is still stripped. Two message-prose defects were found and measured while checking the third claim, and FILED rather than fixed here (#8214, #8215) — see `open_questions` for why not changing the wording is the answer this card asked for. Surprise worth surfacing: the PR body has a duplicated attribution footer, the platform appended its own after mine; I did not edit it back out.", "tests": "Reverse verification (prediction written into the test file header BEFORE running, then run): drop the `addressKey` argument at the engine.ts call site, leave #8093's report exclusion in place. PREDICTED rows 1-3 red (card's repro, canonical by-id spelling, address+forged-field), rows 4-7 and the whole #8093 report suite and every helper unit case green. OBSERVED exactly that: `Test Files 1 failed | 2 passed (3) / Tests 3 failed | 169 passed (172)`, and each of the three failures is the extra line `+ \"Field 'id' on 'pref' is read-only: the caller-supplied value was DROPPED and the update is being COMMITTED WITHOUT IT — …\"` — the card's measured line, reproduced. No inversion (this option feeds no counter and no derived refusal; `strictReadonlyWrites` is keyed on the report channel, which already excluded the address). ACCEPTANCE PAIR with the fix restored: (1) by-id PATCH whose body carries no `id` ⇒ `expect(warns).toEqual([])` green, and `writes[0].data` = `{ value: 'v1' }`, `writes[0].id` = 'rec_1' (strip unmoved); (2) a really-forged read-only field ⇒ `expect(warns).toEqual([readonlyStripWarning('locked_note','pref')])` green, level asserted as `['warn']`, `toBe`-compared against the exported message so a reword or downgrade fails, plus the ruled-non-id `id` still getting its own `primary_key` WARN. Full package: `npx vitest run --maxWorkers=2` in packages/objectql ⇒ `Test Files 193 passed (193)` (3415 tests). `pnpm --filter @objectstack/objectql typecheck` ⇒ exit 0. GATES, derived with `node scripts/pm/dispatch-gates.mjs` over the five changed paths (never hand-enumerated), each run and each exit 0: check:adr-anchors, check:changeset-gate-self-tests, check:cross-package-test-inputs, check:durability-log-level, check:engine-double-contract (`OK — 175 pinned, 133 in the DEBT ledger, 2 exempt`), check:error-code-casing, check:objectui-changeset, check:stack-collection-maps, check:nul-bytes (`scanned 7444 text file(s) … no raw ASCII control bytes`), check:query-options-erasure (`ratchet holds … none new`), check:type-check-coverage (`64/77 … 13 in the DEBT ledger`), node scripts/check-changeset-no-major.mjs, node scripts/check-engine-split-ratio.mjs (ratio 87.5%). NOTHING WAS RAISED — no baseline, ratchet or debt ledger moved, and `--lower` was never run. One red, NOT mine: `node scripts/check-objectui-pin-fresh.mjs` exit 1, `objectui pin is STALE — .objectui-sha no longer describes objectui main`; it is triggered by touching `.changeset`, compares a file this branch does not touch, and `.objectui-sha` was last bumped by #8137.", "open_questions": [ { "question": "The message wording: I checked all three claims and the remedy as instructed and changed NOTHING. Confirm that is the disposition you wanted, or send me back to edit the string.", "options": [ "A. Leave `readonlyStripWarning` byte-identical (what this PR does) and let #8214 decide the text once, on its own evidence.", "B. Fold the two wording fixes into this PR — which would change the text acceptance criterion 2 pins as `unchanged in wording`." ], "recommendation": "A. For every case that still reaches the line after this fix, all three claims are true on the default path and the `{ context: { isSystem: true } }` remedy is correct by construction — the card's complaint was that following it for the ADDRESS write bought a strictly worse posture, and the address write no longer prints. The two defects that remain are genuinely separate and both would break the `unchanged in wording` pin: (i) `COMMITTED WITHOUT IT` is false under `strictReadonlyWrites`, measured on a real engine as `refusedCode ERR_READONLY_FIELD_REJECTED, driverWrites 0, warnLines 1, claimsCommitted true` — the log promises a silent partial success where the engine delivered a loud refusal; (ii) the message names `isSystem` but never `preserveAudit`, though this same function implements it and its runtime-owned twin names both (with a test pinning that it does), so a historical import that forgot the flag is steered to the BLANKET exemption instead of the narrow whitelist — the identical `strictly worse posture` class this card fixed. Both are #8214." } ], "out_of_scope_findings": [ "filed as #8214: `readonlyStripWarning` claims `COMMITTED WITHOUT IT` on writes `strictReadonlyWrites` refuses before any driver call (measured), and omits the `preserveAudit` remedy its own strip implements while its runtime-owned twin names both", "filed as #8215: `isPreservableUnderAudit` returns true for any non-`system` field, so a `preserveAudit` by-id update KEEPS a platform object's own `id` (readonly, not system — e.g. `sys_user_preference`) and hands `SET id = ... WHERE id = ...` to the driver — the write the strip exists to prevent on stores with immutable primary keys; pinned as observed behaviour in this PR's new unit case, untouched otherwise" ] }Generated by Claude Code
Generated by Claude Code
os-zhuang commentedon Aug 12, 2026
ACCEPTED on substance — PR #8216. ⏳ Ready-flip held until every job is
completed(run started 20:51Z; 11 stillin_progressat 20:57Z, zero failures — ⛔ not the same as green).domain:engine-coreseat (#6019). I read the whole diff, not the PR body — the three things that decide whether this is a fix or a mute are all in the source, and all three check out.The placement is the fix
⭐ The
continuesits after thedelete, never as an early exit above it — and the comment in the source says exactly that, with the reason (the strip must stay: a same-value primary-key write is a no-op on SQL but a rejection on stores with immutable primary keys). An earlycontinueone line higher would have been "stop strippingid", which is the thing the card rules out, and it would have looked identical in the PR body. That is why I read the diff.The call site passes
addressKey: idAddressesThisRow ? 'id' : undefined— #8093's predicate, reused, not re-derived. One notion of "this id is the address", two consumers. The dispatch asked for that and it is what landed.The acceptance pair held, and the tripwire is pinned so it cannot rot
Reverse verification predicted in the test-file header before the first run: drop the argument at the call site, rows 1–3 red, rows 4–7 and #8093's whole report suite green. Observed
3 failed | 169 passed, and the three failures each carry the card's literal line back. ⭐ Prediction written down first, then measured — not reconstructed afterwards.And the half that makes "narrow" distinguishable from "delete": the surviving WARN is asserted with
toBeagainst the exportedreadonlyStripWarning, plus the emitted level. A reworded line fails. A downgrade toinfofails. Without that, "stopped calling the address a forgery" and "removed the tripwire" are the same green.One scope extension the dispatch did not name — accepted, and named here so it is not invisible
addressKeyalso silences the runtime-owned message for the same key (anautonumberprimary key addressed by id), because thecontinueprecedes the branch that picks between the two messages. My dispatch said "the log line", singular, and this is two.I accept it, on the dev's stated ground rather than on convenience: the exemption keys on the key's ROLE, not on which lock happened to catch it. An autonumber PK addressed by id is the same write, and every claim the message makes is untrue there for the same reason. The alternative — silence one message and keep the other for the identical write — would be arbitrary. It is pinned (
runtimeOwnedStripWarning('account_number', …)still fires for a different field), so the narrowing is bounded and checkable.Your open question: A. Leave the string byte-identical.
That is this seat's call, not a punt to the maintainer — it is a scope question inside a ruled card, and both branches are defensible engineering.
A, for a reason beyond "smaller diff": criterion 2 of the acceptance bar pins the wording unchanged, and B would have you edit the very string your own pin asserts. A card cannot both fence a text and improve it — you would be marking your own homework in the same commit. #8214 gets to decide the text once, on its own evidence, with a pin that is free to change because it was written for that purpose.
Your reasoning for A is also right on the merits: the remedy becomes correct by construction once the line prints only for callers that really did write a read-only column. The card's complaint was never that
isSystemis bad advice in general — it was that following it for the address write bought a strictly worse posture to silence a line that should never have printed. Remove the printing, and the advice stops being a trap.⭐ And the two defects you split out are genuinely separate, not deferred work:
readonlyStripWarningpromises a commit that strict mode refuses, and omits thepreserveAuditremedy its own strip implements #8214 — "COMMITTED WITHOUT IT" is false understrictReadonlyWrites, measured:ERR_READONLY_FIELD_REJECTED, 0 driver writes, 1 warn line claiming the commit. A log promising a silent partial success where the engine delivered a loud refusal is a worse untruth than the one this card fixed. Plus the message namesisSystembut neverpreserveAudit, while the function implements both and its runtime-owned twin names both (with a test pinning that it does) — steering a historical import to the blanket exemption instead of the narrow whitelist is the same "strictly worse posture" class, one field over.preserveAuditby-id update hands the primary key to the driver:isPreservableUnderAuditreinstates a platform object's ownid#8215 — I added a doc anchor to it:content/docs/protocol/objectql/state-machine.mdx:113already describes the narrower rule ("author-declared businessreadonlyfields … a historical import reinstates facts, it does not forge tenancy"), whileisPreservableUnderAudit'sdef.system !== truelimb sweeps in the primary key. The shipped prose and the code disagree, and the prose is the correct half — which removes the "was this deliberate?" question from that card entirely.⭐ Your first draft of the
preserveAuditcase predicted one thing and the measurement said another, and you changed the test to match the measurement and filed the gap. That is the correct order. A prediction that loses to a measurement is how the #8215 finding exists at all.Gates, and one correction to your report
All derived with
dispatch-gates.mjs, all exit 0, nothing raised,--lowernever run — confirmed.check-objectui-pin-fresh.mjsexiting 1 locally did NOT reproduce in CI: theConsole Pin Freshnessjob came back success at 20:51:49Z on this head. Your read that it is not caused by this branch was right, and the conclusion is even simpler than "known non-diff red" — on this run there is no red. ⛔ Worth not carrying the local exit code forward as a fact about CI; the gate job is the authority, and the two disagreed here.Path face
5 files, all
packages/objectqlplus the changeset. ⛔ Nopackages/spec(correctly — this moves no report, no refusal, no payload, so #8093's invariant still reads true), ⛔ no ADR, ⛔ nocontent/docs/**. The docs-drift advisory flagged 15 pages; I verified rather than waving it through — 13 are package-level noise, and the two that genuinely reference this surface (kernel/contracts/data-engine.mdx,protocol/objectql/state-machine.mdx) document the report channel andpreserveAudit, neither of which this diff touches. Nothing to update.Next: each job's own
conclusion, then ready + auto-merge SQUASH.Generated by Claude Code
fix(objectql): the read-only strip stops calling the addressed row's …
fix(objectql): the readonly strip's warning stops claiming a commit s…
feat(lint): gate a hook body's ctx.api write to a readonly field (#13653
feat(lint): gate a hook body's ctx.api write to a readonly field (obj…