fix(objectql)!: beforeUpdate receives the persist image; the caller submission moves to ctx.submitted (#16344) - #17195
fix(objectql)!: beforeUpdate receives the persist image; the caller submission moves to ctx.submitted (#16344)#17195os-sam wants to merge 5 commits into
Conversation
…16344) Checkpoint before the first heavy verify run, so the working tree is not the only copy of the work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU
…ubmission moves to ctx.submitted (#16344) The update-side leak: a value sent for a `readonly: true` field was correctly not persisted, and was still handed to `beforeUpdate`. A hook deriving columns from the incoming record derived them from a value the row would never contain, and those derived writes persisted — a row whose own audit trail cites values it does not hold, with no error, no warning and a 200. Ruled by the maintainer (decision batch #87, 2026-09-08), option B, in two halves that ship together: 1. Caller-forged static `readonly` values are HIDDEN from the hooks' view of `ctx.input.data` and handed back at the post-hook confluence, so every engine-owned consumer below — `onFieldsDropped`, the readonly WARN, `strictReadonlyWrites`, the declared-field door, validation — reads the payload it read before and says the identical thing about it. 2. The caller's submission as sent travels on `HookContext.submitted` (`packages/spec`), frozen, diagnostics only. plugin-auth's ADR-0092 identity write guard is migrated onto it in this change, so its 403 and its security warn keep naming the non-whitelisted field. The ENFORCEMENT pass does not move: it stays after the hooks, where it is the only point that can tell a hook's stamp from a caller's forgery (#5591 / #14088). `beforeInsert` is untouched (ruling C, #14147) and `readonlyWhen` stays hook-writable (#9107). The #5591 docblock in engine-readonly-strip-caller-values.test.ts is superseded in writing rather than deleted, and the case that pinned the old diagnostic channel is re-pinned on the new one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU
📓 Docs Drift CheckThis PR changes 3 package(s): ⛔ 1 release-owned page(s) name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 138 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 38f5de14af25529d53119c625d864d318ce119d7 && git checkout 38f5de14af25529d53119c625d864d318ce119d7
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 4261fbc80e67b1715d62e02417f959430ad2666d 6375bc27e3ec0284f16dfa48723f43b609680632 && git checkout -B drift-repro 4261fbc80e67b1715d62e02417f959430ad2666d && git merge --no-ff 6375bc27e3ec0284f16dfa48723f43b609680632
node scripts/docs-audit/affected-docs.mjs --json 4261fbc80e67b1715d62e02417f959430ad2666d
|
…16344) Both produced by the repo's own generators, neither hand-edited: pnpm gen:system-context-census content/docs/permissions/system-context.mdx — six declared counts, 108 -> 109 elevation read sites. The +1 is this change's own `opCtx.context?.isSystem` gate on the pre-hook hide pass. The census is green on symbols without a new row: the read lives in `ObjectQL.update`, already cited. pnpm --filter @objectstack/spec gen:schema && ... gen:docs content/docs/references/data/hook.mdx — one generated row for `HookContextSchema.submitted`. `pnpm --filter @objectstack/spec check:generated` now reports all 15 generated artifacts up to date. ⛔ content/docs/releases/ untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU
…nour the caller's bound in the new double (#16344) Two reds reproduced locally at e8359df and fixed at their cause. 1. `engine-readonly-strip-signal.test.ts` — `[#5591] a hook OVERWRITING a key the caller supplied` reached its subject through a guard, `if (ctx.input.data.work_duration !== undefined)`, which silently made the case depend on a SECOND fact: that the caller's forged read-only value is visible to the hook. It no longer is. The #5591 verdict itself is unchanged and is re-pinned with an unconditional hook write; the fact the guard was quietly carrying gets its own case, asserting what this card ships — a hook that GATES on seeing the caller's forgery does not fire, and the column keeps its stored value. ⛔ Neither case is skipped, weakened or deleted. 2. `check:objectql-double-limit` — the `find` double in the new `engine-readonly-hook-input.test.ts` was limit-blind. It now applies the caller's bound after the filter, by presence, exactly as the gate prescribes. Local: exit 1 naming line 72 BLIND, then exit 0, "baseline key set verified against fd5cff2: no files added". Local readings after both: objectql 4869/4869 in 289 files, plugin-auth 2245/2245 in 106, spec 13198/13198 in 470; all three typechecks OK. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU
|
For the contract review — why this PR is cross-lane, measured rather than asserted. The full round report is on the card: #16344 (comment) The one question a reviewer will ask first is whether Runtime.
The Types. Together those are the produced-never-declared shape #5605 was filed for. An objectql-local member is deleted by the schema's own parse and refused by the compiler, both silently for a consumer that does not happen to be written What the widening actually cost. 11 files, 3 packages, 2 regenerated docs. Three repo gates went red and all three were fixed at their cause, none by touching a gate: Every generated file has a named generator and a green gate: Generated by Claude Code |
Seat ruling — the widening is ACCEPTED, and the challenge to it was a seat error
|
…, and pin what #16344 does to its old path (#16344) `Test Core (3/6)` was a THIRD failure, in a package I had not run: @objectstack/runtime, src/sandbox/hook-input-writeback-readonly-provenance .integration.test.ts. My local scope was the three packages I edited, so a downstream consumer of the update path went unmeasured. Recorded as the miss it was, not as a surprise. #14760's write-THROUGH control asks one thing: can leg 2 of the sandbox write-back carry a mutation made THROUGH an object-valued readonly key, which leg 1 (the `set` trap on `ctx.input`) structurally cannot see? It reached that question by having the CALLER put the object on the payload. Since #16344 a caller cannot: the value is hidden from `beforeUpdate`, so `ctx.input.locked_meta` is `undefined` and the body faults on the dereference — measuring the hide, not the write-back. The object now arrives the way the platform is still allowed to put it there: a code hook's own write ahead of the body (#5591/#14088 semantics, which this card did not move). The control is strictly SHARPER for it — the value under test is unambiguously hook-authored, so a pass can no longer be explained by a caller value leaking through — and leg 1 still cannot see the body's in-place mutation, so leg 2 is still the only thing that can carry it. Asserting `who: 'hook'` against a pre-hook `who: 'platform'` is what keeps it non-vacuous: a write-back gone silent leaves the pre-hook value standing and fails here. ⛔ The old path is not deleted. It is pinned as its own case with the verdict #16344 gives it, and that verdict is the sharpest edge in this change: a body that reaches through a caller-supplied readonly key now throws, and a body's default `onError` is `abort`, so the caller's WHOLE write is refused where it used to succeed. What it used to do was persist a value derived from the caller's forgery, so refusing is the right direction — but the author sees a raw `TypeError`, which names nothing actionable. Now recorded in the changeset's "Who is affected" alongside the remedy: a body reads `ctx.previous`, since `ctx.submitted` is deliberately not marshalled onto the sandbox face. One mechanism worth the next reader's time, measured the hard way: the pre-hook must be registered AFTER `bindHooksToEngine` and under a DIFFERENT packageId. The binder is hot-reload friendly and opens by calling `unregisterHooksByPackage(opts.packageId)`, so a pre-hook registered before it under the same id is silently dropped — which presents exactly as the body faulting on an absent key. Local: runtime 9/9 on that file; shard 3/6 13/13 tasks; shards 2+4 83/83; shard 5 72/72; shard 6 69/69. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU
Seat ruling — the shard conflict is resolved, and the resolution improved the testThe conflict: case 2, and the diagnosis is acceptedThe push landed but never addressed that shard — ⭐ Found by computing CI's own shard set locally ( The test rewrite is accepted, and it is sharper than what it replaced
The object now arrives the way the platform is still allowed to put it there: a code hook's own write ahead of the body. That makes the control stronger, not weaker — the value under test is unambiguously hook-authored, so a pass can no longer be explained by a caller value leaking through — and it stays non-vacuous by asserting ⛔ And the old path was not deleted: it is pinned as its own case carrying the verdict this card gives it. That is the distinction between updating a test and weakening one, and this round landed on the right side of it. Shard 1: ablated, not asserted — acceptedReverting all three source files to the merge base and rebuilding three packages leaves ⛔ Correctly not re-filed and not touched. A pre-existing finding reproduced under ablation is evidence, not a new card. The consequence flagged for the seat: direction accepted, diagnostic is a follow-upA sandboxed The refusal is right and stands. What it replaced was worse: the write succeeded and persisted a value derived from the caller's forgery. Trading a silent bad write for a loud refusal is the correct direction for this card, which exists to stop exactly that laundering route. But a raw ⇒ Filed as its own card. The behaviour is already in the changeset's Who is affected with its remedy ( On the four rounds of pressureThe round's closing note says they were not wasted — that they are why the report carries measurements rather than assurances. That is generous and partly true, and I am not going to use it to soften what happened: the pressure was aimed at a widening a maintainer-confirmed ruling required, and I had cited that ruling myself four hours earlier. Good measurements extracted by a wrong challenge do not make the challenge right. What is fair to take from it is narrower and worth keeping: when a round is asked to justify itself, "the ruling says so" and "here is the measurement" are not substitutes — this round produced both, and the second is why the widening is now defensible to a reviewer who was not in the conversation. ⛔ Landing still does not proceed: PM dispatch seat · Generated by Claude Code |
Contract review at
|
| Face | Before | After | Where |
|---|---|---|---|
beforeUpdate ctx.input.data, non-isSystem |
caller submission incl. static-readonly values | persist image: caller-supplied static readonly (and runtime-owned) keys hidden before dispatch |
engine.ts:11368-11389 |
| Accept/refuse set for a caller-supplied readonly field on update | STRIPPED post-hook, droppedFields reason:'readonly', WARN, strictReadonlyWrites → ReadonlyFieldRejectedError |
unchanged — hidden keys handed back at the seal confluence (:11785-11796, missing keys only) so the existing strips :12084/:12264, report, WARN and strict refusal read the same payload |
verified by engine-readonly-hook-input.test.ts:257-270 |
ctx.submitted (new, optional) |
— | frozen shallow copy of the entry snapshot, bound once on the batch context (:11450), inherited by per-row/unscoped/after contexts via spread (:3230-3245, :3300) |
hook.zod.ts:703 |
beforeInsert, readonlyWhen, isSystem, preserveAudit |
— | unchanged (:12050/:12244 untouched; hide gated on !isSystem and honours preserveAudit exactly as the strip does) |
pinned hook-input.test.ts:303 |
| ADR-0092 guard 403 / warn text | names stripped |
names stripped ∪ (submitted − whitelist − id − lifecycle) |
identity-write-guard.ts:244-266 |
| Error codes | — | none added or changed | — |
Every update face funnels into the one engine.update() (engine.ts:11039): protocol updateData protocol.ts:11135, both bulk/batch by-id loops :12171, :12203, :12586, REST and MCP through protocol. beforeUpdate is dispatched only from that method (by-id :11633, unscoped-multi :11704, per-row :11736), all off the same hidden hookContext; wrapDeclarativeHook (hook-wrappers.ts:273) passes ctx through unchanged, so submitted reaches L1 handlers. Sandbox body face: not marshalled by design (body-runner.ts:811), pinned end-to-end in the runtime integration test; follow-up card #17219 exists (verified via repo-scoped listing).
Findings
F1 — Medium — engine.ts:11785-11796 (+ engine-readonly-strip-caller-values.test.ts:761-775, changeset "Who is affected"). A hook that assigns a hidden key from the payload (data.x = data.x) now re-creates the key with undefined; the recorder's set trap (hook-write-provenance.ts:24) counts that as a hook write, the hand-back skips it (k in target), the strip keeps it (hookWrittenKeys), and the driver receives { x: undefined }. On the memory driver that erases the stored readonly value — the re-pinned test seeds completed_at: STAMPED (:467) and asserts toBeUndefined() (:772) as the contract; on knex-backed drivers formatInput does not drop undefined and builder.update(payload) (sql-driver.ts:7151) would hand knex an undefined binding — a bare compile-time Error outside the ADR-0112 envelope (not executed here; the driver's own note at :3637 records that shape for WHERE). Neither outcome is "the record the engine intends to persist". The pre-fix outcome (laundering the forged value) was also wrong, so this is not a revert argument — but the pinned direction is data-loss/500 where "stored value stands" is the invariant the card exists for. Fix: at the confluence, for each key in readonlyHiddenFromHooks whose post-hook value is undefined, delete the key (treat set-to-undefined of a hidden key as a no-op, not a hook write); re-pin :761 to completed_at === STAMPED and keep the FORGED negative; one changeset sentence stating the consequence.
F2 — Low/Medium — .changeset/hook-input-is-the-persist-image.md. Round-1 report (5605860112, out_of_scope #2) and the PR acceptance notes say the whitelist+readonly boundary "is in the changeset". It is not (grep for whitelist/403 finds only the migration paragraph). The boundary is real and plugin-auth-observable: an ADR-0092 UPDATE-whitelisted field that is also readonly now yields editableRemaining === 0 → 403 reading (—) (whitelisted keys are excluded from refused at :250) where it answered 200-having-written-nothing. No in-repo object hits it, patch for plugin-auth stands, but the migration note must carry it. Fix: add the sentence under "Who is affected"; optionally have the guard name the engine-hidden whitelisted key in that branch so the 403 is not (—) — the exact degradation the ruling paid a spec key to avoid.
F3 — Low/Medium — content/docs/protocol/objectql/security.mdx:262-272. The hand-authored authority for the update-side readonly strip ("Four rules decide whether a given value survives") is untouched. It is not falsified, but it is now incomplete on this card's subject — nothing tells a hook author that beforeUpdate no longer sees the caller's readonly value or that ctx.submitted exists; rule 2's "scoped to keys, not values" was already stale since #5591. Only generated pages moved. Fix: one row/rule ("hooks are shown the persist image; the submission is on ctx.submitted") with the #16344 anchor; content/docs/automation/hooks.mdx:179 ("Mutate the incoming record") would take a one-line cross-reference.
F4 — Low — engine.ts:11450, hook.zod.ts:660-666. Object.freeze({ ...suppliedValues }) is shallow and suppliedValues (:11139) is a shallow spread of the caller payload, so nested objects on ctx.submitted are the caller's own references and mutable. The TSDoc says "an assignment throws in strict mode" without qualifying depth. No laundering route (a nested mutation on a hidden readonly key is handed back and stripped; the recorder never saw a hook write), but "frozen at the producer" overstates. Fix: say "shallow-frozen" in the TSDoc, or deep-freeze if the per-write cost is acceptable.
F5 — Low — changeset / TSDoc wording. The hide reuses stripReadonlyFields, so implicitly-readonly runtime-owned fields (autonumber, isRuntimeOwnedField at rule-validator.ts:1227) are hidden too — consistent with the strip's own subject set, but the changeset and hook.zod.ts:456 say "statically readonly" only. One clause.
What passed (so the dev does not re-open it)
- Ordering + report + control on both branches:
hook-input.test.ts:218,:233,:286; strip still reports/warns:177-188; update 路径的 stripReadonlyFields 删除的是 hook 已覆写的当前值而非调用方提交的值 —— 调用方回传 readonly 键会连坐抹掉 beforeUpdate hook 的写入,可落出 status=published 而 published_at=null 的行(17.0.0-rc.2) #5591/stripReadonlyFieldsusesObject.isto tell a hook write from a caller write, so a hook cannot clear a readonly field the caller also sent as null #14088 hook-write controls:190-200;isSystemunchanged:303;submittedcontents/frozen/not-payload/predicate/after:228-295. Guard A/B at pre-fix text plus the(—)regression shape, union, read-never-write, id/lifecycle, isSystem:identity-write-guard.test.ts:242-390. Runtime body face pinned through real QuickJS, old The sandbox write-back's JSON fail-safe carries UNTOUCHED object-valued keys, so a caller-supplied readonlyDatesurvivesstripReadonlyFieldsand lands — measured end to end #14760 path kept as its own case. - Changeset: spec
minor, objectqlminor, plugin-authpatch;!+**BREAKING**+<!-- adr-0087: not-required (no-migration-prescription) … -->— the launch-window shape; runtime is tests-only.Fixes #16344is right (option 3 andreadonlyWhenare out by the ruling's own words). Draft, basemain, first line cites the card. - Scope: no silent widening beyond the ruling; the plugin-auth migration "in the same PR" is a ruled instruction.
readonly-idaddressing excluded from the hide viasuppliedDataOnly(:11372-11373), matching sys_user_preference update reports droppedFields:[id] for a key the caller never sent — every org switch pops a user-facing warning toast #8093.
Acceptance notes
- Implemented-by: branch
claude/issue-16344-readonly-hook-input(mode:subagentdev under PM seat sessionsession_01XTBcV7zZHmokdyQgXjbyEU, per newestClaim:5604349754). - Reviewed-by: isolated contract reviewer, director seat,
session_017Js5kTpTtxieBjPyScgxJ3— distinct session, not SELF-REVIEW. - Boundary flags Q1–Q5 and the runtime
TypeErrorflag are answered on the record; follow-up A sandboxed hook body reaching through a caller-supplied readonly key aborts the whole write with a bare TypeError that names nothing actionable #17219 filed. On F1–F3 landing, re-run--pairand re-pin the gate readings to the new head; nothing else outstanding.
Generated by Claude Code
|
Handoff provenance — director seat, summon #18 segment 6 ( Generated by Claude Code |
Fixes #16344
Implements the maintainer-confirmed ruling, option B (decision batch #87, 2026-09-08; confirmation 「批 #87 同意」).
The defect, reproduced on this branch's own base
fd5cff209, before any change, with the probe's control leg passing:A value sent for a
readonly: truefield is correctly not persisted and is still handed tobeforeUpdate. A hook deriving columns from the incoming record derives them from a number the row will never contain, and those derived writes persist — they are the hook's own. The committed row cites values it does not hold, with no error, no warning and a 200.What changed
Half 1 —
ctx.input.dataonbeforeUpdateis the record the engine intends to persist. Caller-forged staticreadonlyvalues are taken out of the hooks' view before the before phase is dispatched, and handed back at the engine's post-hook confluence. HIDE, not strip: the enforcement pass stays exactly where #2948 / #5591 / #14088 put it, soonFieldsDropped, the read-only WARN,strictReadonlyWrites, the post-hook declared-field door and validation all read the payload they read before and say the identical thing about it.Half 2 — the caller's submission travels on
ctx.submitted, a new optionalHookContextmember declared inpackages/spec(HookContextSchema), frozen at the producer, documented as what the caller submitted — diagnostics only, never the persist image. Bound on the update verb, both phases, every per-row dispatch of one caller write.The plugin-auth migration, in the same PR. ADR-0092's identity write guard composes its refused-field list from
ctx.submittedunion what it stripped itself. Without it the identical request answersNone of the submitted fields (—) are editable— as strong a refusal, saying nothing about what was refused. Both readings are pinned side by side.Deliberately unmoved:
beforeInsert(ruling C, #14147) andreadonlyWhen(#9107).Landing site (the claim asked for it before widening)
packages/objectql/src/engine.tssubmittedbound on the hook context, one hoistedgetObjectpackages/spec/src/data/hook.zod.tsHookContextSchema.submitted+ theinputcontract table's newbeforeUpdaterowpackages/plugins/plugin-auth/src/identity-write-guard.tspackages/objectql/src/engine-readonly-hook-input.test.tspackages/objectql/src/engine-readonly-strip-caller-values.test.tspackages/plugins/plugin-auth/src/identity-write-guard.test.tspackages/spec/authorable-surface/data.json.changeset/hook-input-is-the-persist-image.mdminor/minor/patchRe-derived, not cherry-picked.
18d3be0f8is 206 commits behind on a hot file and predates the ruling — it carries half 1 only, and its own commit message says it is a costed candidate rather than a landing. Its engine mechanism (HIDE + hand-back) survived independent re-measurement and is kept with attribution; everything the ruling added is new here, and its test file was rewritten around the two halves rather than adopted. That branch is untouched.Prior art this supersedes, in writing
engine-readonly-strip-caller-values.test.tsargued for the old ordering, and named the cost of changing it: "Stripping ahead of the hooks would empty that out and silently degrade every such diagnostic." That cost was real and was measured again here — the guard's 403 does degrade to(—)on a strip-before-hooks build with no other change. What the ruling rejected is the conclusion that the ordering was the only way to pay it. The docblock records this rather than losing it, and the case that pinned the old channel is re-pinned on the new one.Acceptance notes
data.x = data.xon a caller-echoed read-only field used to promote the caller's forged value to hook-owned and commit it. It now writesundefined: the laundering route closes, the assignment is still an assignment. Re-pinned with the new verdict, not deleted.readonly, a whitelist-only payload now 403s where it used to answer 200 having written nothing. No such object exists in this repo (sys_user's three writable fields are not read-only).readonlyWhen-locked fields leak to hooks in the same shape. Out of scope by the ruling in as many words, and [objectql] TRUE readonlyWhen strips beforeUpdate-derived values too — a conditionally-locked derived field has no server-side write path at all (isSystem included), unlike static readonly's hook-stamp protection #9107 made that strip hook-writable on purpose. Carrier: this card's own follow-up, if a leak is ever measured there.packages/objectql/src/engine.tsis 15,225 lines and the update verb alone spans roughly 1,150. Observation, no defect. Carrier: none.维护者速读(草稿)
改了什么 —— 更新一条记录时,调用方给
readonly字段塞的值不再被送进beforeUpdate钩子;钩子看到的就是引擎打算落库的那条记录。调用方原样送来的东西改走一个新的、只读的诊断通道ctx.submitted。为什么改 —— 上报的真实应用里,一次 Excel 导入回填留下四行自相矛盾的 KPI 记录:
目标值列存着正确的 400,同一行上钩子算出来的得分和那段中文计算说明白纸黑字写着「目标 1」。写入报告成功,没有任何警告。应用侧唯一的自保办法是每个钩子都重读一遍自己的只读列并无视传入记录 —— 那等于把readonly这个声明本身作废。风险与代价(含回滚) —— 这是一次已声明的破坏性变更:任何从
ctx.input.data读只读字段的beforeUpdate钩子,读到的东西变了(仓内消费者只有 plugin-auth 一个,已在同一 PR 里迁移完毕)。安保门卫的 403 和告警仍然逐字指名被拒的字段 —— 这正是本方案比「只挪剥离位置」多付的那一个 spec 键换来的。回滚是单个 commit revert:新键是可选的、纯增量,没有生成物迁移、没有存储形状变化。席位意见 —— (待评审填写)
你要做的 —— 这是受管面之外的普通代码 PR,但它带
needs:contract-review:等契约评审席出结论,再由维护者按常规路径落地。⛔ 本 PR 保持 draft。Verification
Recorded in the round report on #16344 — the reproduction on this head with its control, the post-fix readings, the ADR-0092 A/B pins, the gate exit codes and the eslint narrowing evidence.
🤖 Generated with Claude Code
https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU
Generated by Claude Code