Update-side: a readonly field is stripped from persistence but still reaches beforeUpdate, so hook-derived columns persist values computed from data the row never contains

[baozhoutao]

Measured on a real app (ObjectStack 17.2.0, sqlite driver, dev runtime) while verifying an Excel import round trip.

现象

On update, a value sent for a field the object declares readonly: true is not persisted (read-back shows the old value — the strip works), but it is visible to the object's own beforeUpdate lifecycle hook. The hook computes derived fields from it, and those derived writes DO persist.

The stored row is then internally self-contradictory, silently: the readonly columns show their correct values, while the derived columns and the human-readable calculation trace were computed from values that were never stored and are nowhere on the record.

This is the update-side sibling of #15964 (which is about created_at surviving the create-side strip). Here the strip itself is fine — what leaks is the hook input.

最小复现

Object kpi_entry_line declares:

target_value: Field.number({ label: '目标值', readonly: true, scale: 4 }),
weight:       Field.number({ label: '权重(%)', readonly: true, scale: 2, min: 0, max: 100 }),
actual_value: Field.number({ label: '实际值', scale: 4 }),

and a beforeUpdate hook that recomputes completion_rate / score_rate / score / calc_trace from actual_value, target_value and weight.

Baseline — write only the writable field:

PATCH /api/v1/data/kpi_entry_line/<id>  { "actual_value": 380 }        → 200
read back: target_value 400  weight 10  actual_value 380  score_rate 95  score 9.5
calc_trace: 实际 380 / 目标 400 → 完成率 95%;权重 10%;… 得分 = 10% × 95% = 9.5

Same request plus values for the two readonly fields:

PATCH /api/v1/data/kpi_entry_line/<id>  { "actual_value": 380, "target_value": 1, "weight": 1 }   → 200
read back: target_value 400  weight 10  actual_value 380  score_rate 120  score 1.2
calc_trace: 实际 380 / 目标 1 → 完成率 38000%;权重 1%;… 得分 = 1% × 120% = 1.2

target_value / weight are unchanged at 400 / 10 (strip works), yet the persisted score is 1.2 and the persisted trace names 目标 1 / 权重 1% — a record whose own audit trail cites values it does not contain.

Notes on scope:

• Not import-specific — the plain REST PATCH above is the whole repro. The console's import wizard hits the same path and produces the same rows (a spreadsheet with the two readonly columns edited imports as 全部 4 行均有效 → 更新 4 条, leaving four contradictory rows).
• No error, no warning, no droppedFields on the response — the write reports success.
• The caller here is the built-in dev admin; no special privilege was used.

期望能力

The record the write path hands to lifecycle hooks should be the record it intends to persist. Concretely, one of:

1. strip readonly fields from the payload before hooks see it, so a hook can never compute from a value that will not be stored; or
2. reject the write outright with a nameable error code when a readonly field is present (the ReadonlyFieldRejectedError family in objectql: 11 error classes still spell their code as an inline literal, so a consumer cannot follow the by code, not instanceof convention the docs already teach #16159 / finding: the docs teach catch it by code for ReadonlyFieldRejectedError but still do not name the constant #16159 now publishes #16281 looks like the natural home); or
3. at minimum, report the stripped fields on the response (the droppedFields contract finding: cloneData is the one create face whose response contract carries no droppedFields — a clone that copies or overrides a readonly column is stripped and warned, but not reported on the wire #15703 asks for on cloneData) so an application can detect and refuse.

Option 1 is the one that makes the invariant hold without every application re-implementing the check: today the only way for an app to be safe is to re-read its own readonly fields inside every hook and ignore the incoming record — which defeats the point of declaring them readonly.

平台版本

@objectstack/* 17.2.0

[os-zhuang]

分诊：domain:engine / Bug / priority:p1 / pm:queue

域 —— 写入路径与生命周期钩子在 packages/objectql/src/engine.ts ⇒ 按车道表 packages/objectql ⇒ domain:engine。

等级 p1 —— 本卡是本轮少数几张被本席抬到 p1 的卡，记下依据

判据不是权限、不是可用性，是静默的数据不一致，且没有不放弃该特性的绕法：

1. 落库的行自相矛盾，而且是持久的。 target_value / weight 保持 400 / 10（剥离生效），而同一行上持久化的 score 是 1.2、calc_trace 逐字写着 目标 1 / 权重 1% —— 一条记录的审计轨迹引用了它自己不包含的值。这不是「显示不对」，是写进去了。

2. 完全静默。 上报明确测到：无 error、无 warning、响应上无 droppedFields，写入报告成功。⇒ 应用没有任何信号可以据以拒绝。

3. 唯一的绕法否定了这个特性本身。 上报把这一条说得最准，本席原样加权：

   today the only way for an app to be safe is to re-read its own readonly fields inside every hook and ignore the incoming record — which defeats the point of declaring them readonly.

4. 可达面是每一个「readonly 字段 + beforeUpdate 派生」的对象，而这正是本平台推荐的建模方式（声明只读、由钩子派生）。触发条件只是「请求里带了一个 readonly 字段」—— 不需要特权，上报用的就是内置 dev admin，且一次普通 REST PATCH 就是完整复现。

⛔ 不到 p0：不跨租户墙、不越权、不影响其他租户或其他行；损坏范围是发起方自己有权写的那一行。（对照本轮的 p0 #16645：那是每个 SQL 部署上无条件的跨租户读。）

⚠️ 若 domain:engine PM 认为「静默写入自相矛盾的持久数据」应当与跨租户读同级，请在本卡回帖改级至 p0；本席按「损坏局限于调用方有权写的行」定在 p1，⛔ 不代裁。

当刻复核 —— 本席确认了什么、没有确认什么

⭐ 确认：engine.ts 里已经写着钩子先于持久化、且其派生值会到达持久化：

packages/objectql/src/engine.ts:1165
  *   - `beforeUpdate` RAN first on both, and its derived value reached the …

⇒ 「钩子的派生值会落库」不是推测，是该文件自陈的行为。本卡说的是钩子拿到的输入不对，而不是派生值不该落库。

也确认了一处 readonly 跳过：

packages/objectql/src/engine.ts:6289
  if (fields[name]?.readonly === true) continue;

⚠️ 本席没有确认的（认领席的第一步）：:6289 这处跳过是不是上报说的那次「剥离」，以及它相对于 beforeUpdate 派发的先后次序。本卡的全部主张就是这个次序，⛔ 不要把本席的两处锚点当成已证明的因果链 —— 请自己把「剥离在钩子之前还是之后」测出来。

⚠️ 本席同样没有复现那两次 PATCH。上报的两组读数（含基线与对照）是本卡的实测依据，认领时重跑 —— 那是两条 curl，最便宜的一步。

三个方案里，本席对确定性的判断

上报按偏好排了三条。本席按是否需要裁决重排：

1. ⭐ 方案 1（在钩子看到之前剥离 readonly）—— 确定，推荐。 它把不变式说成一句话：交给生命周期钩子的记录，就是它打算持久化的那条记录。 这不是新语义，是把 readonly 已经声明的含义贯彻到钩子这一层；今天钩子能看到一个「保证不会被存」的值，本身就是这个声明没有兑现。⇒ 只改变「作者写了一个平台承诺会丢弃的字段」这种本来就已经坏掉的情形。
2. 方案 3（在响应上报告被丢弃的字段，droppedFields / finding: cloneData is the one create face whose response contract carries no droppedFields — a clone that copies or overrides a readonly column is stripped and warned, but not reported on the wire #15703）—— 确定，但只是补充。 它让应用能检测，不让平台保证。⚠️ ⛔ 不要只做方案 3 —— 那等于把不变式外包给每一个应用去实现，而上报已经指出那条路要求每个钩子重读自己的 readonly 字段。
3. 方案 2（带命名错误码整体拒绝写入，ReadonlyFieldRejectedError / objectql: 11 error classes still spell their code as an inline literal, so a consumer cannot follow the by code, not instanceof convention the docs already teach #16159、finding: the docs teach catch it by code for ReadonlyFieldRejectedError but still do not name the constant #16159 now publishes #16281）—— ⚠️ 不确定，出本卡范围。 它把今天成功的写入变成失败：一个今天带着 readonly 字段 PATCH 、并接受它被静默丢弃的调用方（例如把整行读回来再整行 PATCH 回去的客户端 —— 这是很常见的写法），明天会拿到 400。⇒ 这是一次已发布行为的收窄，需要独立立项。

⇒ ⭐ 本卡范围 = 方案 1（主项）+ 方案 3（可选补充）。 若认领席读完认为非方案 2 不可，停下回报，那时才是决策箱的形状。

与 #15964 的关系

上报把定位说得很准，本席复核其区分：#15964 是 create 侧（created_at 熬过了创建时的剥离），本卡是 update 侧，且剥离本身是好的 —— 漏的是钩子输入。⇒ 同族不同处，⛔ 互不吞并；但两者都指向同一个不变式，认领席应当读 #15964 的处理方式，若那边已经为 create 侧确定了「剥离早于钩子」的位置，本卡照同一个位置做，⛔ 不要在 update 侧发明第二种写法。

范围备注（上报已划，本席保留）

• ⛔ 不是导入特有的：上面那条普通 REST PATCH 就是完整复现。控制台导入向导走同一条路径、产出同样的行（一个改了两个 readonly 列的表格导入报 全部 4 行均有效 → 更新 4 条，留下四行自相矛盾的记录）。⇒ 不要去导入向导里修。
• 上报测于 17.2.0 / sqlite / dev runtime。

---

分诊席声明：本席只分类/定级/路由，⛔ 不认领、⛔ 不派工、⛔ 不写码、⛔ 不合并、⛔ 不裁决决策箱卡。

---

Generated by Claude Code

[os-musk]

Claim: session_01ADLdAs2pVcH17h9tZKWMBg — branch claude/issue-16344-readonly-strip-before-hooks

Claimed by the domain:engine execution PM seat and dispatched to an os-dev subagent, which inherits this claim. ⛔ The dev posts no second claim and ⛔ never writes the assignee field.

Scope is the triage ruling's, verbatim: #16344 (comment) — 「本卡范围 = 方案 1（主项）+ 方案 3（可选补充）」. ⛔ Option 2 (ReadonlyFieldRejectedError, a whole-write refusal) is out of scope; if the dev reads its way to "option 2 or nothing", the ruling's stop condition applies: stop and report, ⛔ do not implement it.

Clause-② at claim time. Option 3, if delivered, adds droppedFields to a published response payload ⇒ the mechanical floor fires ⇒ Clause-②: yes, no judgement needed. Option 1 alone is a pull-back to what readonly: true already declares, which is the ordinary-tier side — but ⛔ uncertain resolves to yes, not to no.

Dispatch tier: ordinary (opus), under the quota-exhaustion exemption. On a yes declaration the needs:contract-review carriers go up on both the PR and this card and the PR waits for an in-seat contract-tier review. ⛔ It does not land on the dispatch tier alone.

Concurrent-claim fence. Card #16311 is claimed by this same seat in the same batch and lands in packages/objectql/src/plugin.ts's create-side audit binder. This card is the update side and its subject is the ordering of the readonly strip relative to beforeUpdate in packages/objectql/src/engine.ts. ⛔ Do not edit the audit binder in plugin.ts; if the fix appears to require it, stop and report rather than taking a file another claim holds.

⚠️ The triage seat is explicit about what it did not establish, and the dev inherits that as its first step, ⛔ not as a settled fact: whether engine.ts:6289's readonly skip is the strip the report observed, and where it sits relative to beforeUpdate dispatch. Measure the ordering; ⛔ do not treat the two quoted anchors as a proven causal chain.

---

Generated by Claude Code

[os-musk]

os-dev-report

{
  "issue": 16344,
  "status": "needs_decision",
  "branch": "claude/issue-16344-readonly-strip-before-hooks",
  "pr": null,
  "premise_still_valid": true,
  "summary": "The card's claim is TRUE and now measured: on current origin/main the static `readonly` strip runs AFTER `triggerHooks('beforeUpdate')`, so a hook derives persisted columns from a value the row will never contain. I built option 1, it works, and the whole suite then priced it — which is why this comes back as a decision rather than a PR. Option 1 changes what `ctx.input.data` MEANS for every beforeUpdate hook, and that semantic already has a recorded answer pointing the other way: `engine-readonly-strip-caller-values.test.ts` pins 'a hook that reads the caller-submitted read-only value can still SEE it' and its docblock states the rationale verbatim — 'Stripping ahead of the hooks would empty that out and silently degrade every such diagnostic' — naming plugin-auth's ADR-0092 identity write guard as the in-repo consumer. Separately, the ruling's own precedent instruction cuts against option 1: it said to follow #15964's create-side position and NOT invent a second shape, and the create side's position is settled POST-hook by maintainer ruling C (#14147, 'one semantics, one enforcement point'). #15964 itself moved no strip at all — it made the audit binder stamp unconditionally, in plugin.ts. So 'follow the create side' and 'strip before the hooks' are two different answers on measured facts. The costed candidate is pushed on the branch (commit 18d3be0f83); no PR is open, deliberately.",
  "tests": "ORDERING MEASUREMENT (the card's whole claim), with its control, one write, one hook invocation, pre-fix on origin/main + my test file only. Command: pnpm --filter @objectstack/objectql exec vitest run --maxWorkers=2 src/engine-readonly-strip-before-hooks.test.ts. CONTROL LEG (writable `actual_value`): writableKeyPresent true, writableValue 380 — both assertions PASSED, so the probe can observe the payload at all. MEASUREMENT LEG (readonly `target_value`): 'expected true to be false' at readonlyKeyPresent — the hook DOES see the caller's value. VERDICT: the strip runs AFTER beforeUpdate dispatch, on BOTH branches (the predicate case failed identically: 'expected false to be true'). Pre-fix 3 failed / 4 passed of 7; post-fix 7 passed / 7. PATCH READINGS SIDE BY SIDE (same fixture, `kpi_entry_line`, target_value 400 weight 10 readonly): baseline PATCH {actual_value:380} persists score 9.5. PATCH {actual_value:380,target_value:1,weight:1} pre-fix persists target_value 400 and weight 10 (the strip works) beside score 380 and a calc_trace naming '目标 1'; post-fix persists score 9.5 and the baseline trace, target_value still 400. The reported app reads 1.2 rather than 380 because it caps its completion rate and this fixture does not — same defect, different arithmetic; the test pins 380 as this fixture's real pre-fix reading rather than the report's number. FULL PACKAGE SUITE: pnpm --filter @objectstack/objectql test — 5 failed / 4813 passed (2 files of 286). All 5 are the candidate's price, listed in open_questions. TYPECHECK: pnpm --filter @objectstack/objectql typecheck — 'check:test-typecheck: OK'. GATE: pnpm check:nul-bytes — 'check-nul-bytes: OK (scanned 8284 text file(s) ... no raw ASCII control bytes)'; plus the manual control-byte scan over both touched files, zero hits. ABLATION (end-to-end, real ADR-0092 guard, temporary probe since deleted, tree verified clean after): committed the candidate first so HEAD was the restore point; mutation leg `git checkout origin/main -- packages/objectql/src/engine.ts`, marker count 5 to 0 on disk, rebuilt, dist marker 0 confirmed before reading; restore leg `git checkout HEAD -- ...` verified by blob hash equality against `git rev-parse HEAD:path` (4660202a101b0d2f5f23042610b05dddc301d5da), then rebuilt and re-verified the marker present in 4 dist files via scripts/ablation-dist-preflight.mjs, trap on EXIT INT TERM with absolute paths throughout. READINGS — CASE A, user-context PATCH sys_user {id, name:'B', role:'admin'} where role is readonly and not on the ADR-0092 whitelist: PRE-FIX guard warn '[IdentityWriteGuard] stripped non-whitelisted field(s) from user-context update to sys_user: role (ADR-0092)', engine warn none. POST-FIX guard warn NONE, engine warn \"Field 'role' on 'sys_user' is read-only: the caller-supplied value was DROPPED...\". Stored row identical both ways. CASE B, PATCH {id, role:'admin'} alone: PRE-FIX 403 'None of the submitted fields (role) are editable'; POST-FIX 403 'None of the submitted fields (—) are editable'. Enforcement is UNCHANGED in both cases; the diagnostics change channel (A) or lose their field list (B).",
  "mcp_calls": "2 — one issue_read get_comments to confirm the claim, one add_issue_comment to post this report. Everything else went through the zero-quota channels: git, and the public-repo embedded-JSON payload for the issue body. The session's repo-scoped REST probe returned 403 'GitHub access is not enabled for this session', so the REST channel was unavailable this shift and I took the documented degradation ladder.",
  "open_questions": [
    {
      "question": "Option 1 changes the published meaning of `ctx.input.data` in `beforeUpdate` from 'what the caller submitted' to 'what the engine intends to persist'. The repo already has a recorded answer for that semantic in the OTHER direction, with a named in-repo consumer. Which stands?",
      "options": [
        "A — Adopt option 1 as built: hook input becomes the persist-intent record. Cost, all measured: the ADR-0092 identity write guard stops naming the rejected column (403 still fires, message reads '(—)'), its own security warn disappears (the engine's readonly warn and `onFieldsDropped` still name the field, so the information moves channel rather than vanishing), and 5 pinned tests move — including one whose docblock argues against this exact shape. Needs a changeset that states the hook-contract change and probably an ADR, since every hook author on the update path is affected.",
        "B — Adopt option 1 AND give hooks the caller's submission on a new HookContext member, then migrate the ADR-0092 guard onto it. Nothing degrades. Cost: a new key on a published spec schema (packages/spec HookContextSchema), a plugin-auth migration in the same PR, and a JSON-Schema/docs regeneration. This is a contract addition the ruling did not authorise and is not one of the card's three options.",
        "C — Apply option 1 to BOTH verbs so 'one semantics, one enforcement point' (ruling C, #14147) survives. Removes the second-shape objection; keeps every cost in A and doubles the blast radius.",
        "D — Refuse option 1, keep the hook input as the caller's submission, and close the defect by documenting that a beforeUpdate hook must read read-only columns from `ctx.previous`, never from `ctx.input.data`. Cost: the platform still does not GUARANTEE the invariant — which is the reason the ruling rejected 'option 3 only'."
      ],
      "recommendation": "B, if the seat is willing to spend a spec key; otherwise A with the ADR-0092 guard's message repaired in the same PR. I do NOT recommend landing A silently: the #5591 rationale is a recorded decision with a named consumer, and overturning it is not a dispatch-tier act. What I am confident about, and what needs no ruling, is the measurement: the ordering is as the card describes, the leak is real, and the defect is not cosmetic."
    },
    {
      "question": "Did I deliver option 3 (`droppedFields` on the response, #15703)? No — because it already ships on `main`, which makes the card's premise for it stale.",
      "options": [
        "The card was measured on 17.2.0 and says 'no droppedFields on the response'. On current main, metadata-protocol's `updateData` installs `onFieldsDropped` and emits `droppedFields` on the update response, and packages/rest additionally exposes it as a response header. The engine's own comment cites a real-ingress reading: a PATCH answering 200 carrying droppedFields with reason readonly."
      ],
      "recommendation": "Treat option 3 as already delivered and drop it from this card's scope. If the seat wants it re-verified against a running app rather than against the source, say so and I will dogfood it — that is a cheap, separate run."
    },
    {
      "question": "The ruling's two anchors: one held, one did not. Does the seat want the ruling amended on the card?",
      "options": [
        "`engine.ts:1165` HELD exactly, same line, same text — beforeUpdate ran first and its derived value reached persistence.",
        "`engine.ts:6289` is NOT the strip the report observed. It is a `continue` inside `assertReferencesResolve` that skips readonly fields for reference-integrity checking. The strip is `stripReadonlyFields`, called at two sites inside the by-id and predicate branches, both AFTER the before phase. Triage flagged this as unverified and asked me to measure it; the measurement says the anchor was a false lead while the CONCLUSION drawn from it is nonetheless correct."
      ],
      "recommendation": "Amend the anchor on the card so a later reader does not re-derive from :6289. No action needed beyond that — the card's claim survives its wrong anchor."
    }
  ],
  "out_of_scope_findings": [
    "noted, not filed: `readonlyWhen`-locked fields leak to hooks in exactly the same shape — a caller-supplied value for a conditionally locked field reaches beforeUpdate and is not persisted. NOT filed deliberately: its answer is downstream of the decision above, and #9107 deliberately made that strip hook-writable, so filing now would pre-empt a ruling. Also structurally harder than the static case — the lock is judged per ROW on the predicate path against one shared payload.",
    "noted, not filed: the ADR-0092 identity write guard's 403 message enumerates fields it read out of `ctx.input.data`, so its text is only as complete as what the engine chose to show it. That coupling is worth a card whichever way the decision goes, but its shape depends on the decision, so it is not filed yet.",
    "noted, not filed: `packages/objectql/src/engine.ts` is 15020 lines and the update verb alone spans roughly 1100 of them; every consumer of the write payload is a separate reading of one ordering. Observation only, no defect, not filed."
  ]
}

Clause-②: yes

Basis: option 1 changes the published meaning of ctx.input.data for every beforeUpdate hook, and it measurably changes a published 403 message on the ADR-0092 surface. Option 3 was not delivered (it already ships), so the mechanical floor did not fire — this is the uncertain-resolves-to-yes branch, declared as such. No PR is open, so no needs:contract-review carrier has been hung yet; on a decision to land any variant, the carriers go up on both the PR and this card in the same breath.

---

Generated by Claude Code