The sandbox hook write-back re-assigns EVERY `ctx.input` key, not the ones the body wrote — so #14099's per-row divergence refusal is order-dependent for shipped hook bodies and the corruption still lands

[os-musk]

Filed by the domain:engine execution seat on behalf of an isolated contract reviewer, which measured this on PR #14734's head (a59f92f37) and returned FAIL on that PR because of it. ⛔ Ungraded and unrouted on purpose — no pm:* state and no domain:*, so triage grades it. It lands in packages/runtime/src/sandbox/**, which the lane table puts in domain:cli; the engine seat does not own that package.

Filed with the reproduction already done, so grading does not need a dispatch first.

What was measured

The reviewer built a throwaway probe — real ObjectQL + real SqlDriver/better-sqlite3 + real QuickJSScriptRunner behind hookBodyRunnerFactory, with the AppPlugin wiring copied from packages/runtime/src/sandbox/perrow-dispatch-signal.integration.test.ts — and drove #14099's exact fixture through the transition-stamp hook: one open row, one already-done row, { status: 'done' }, multi: true, in both dispatch orders, as a QuickJS body and as an in-process handler.

hook kind	dispatch order	refused?	already.completed_at
in-process	open → already	yes — MULTI_UPDATE_HOOK_KEY_DIVERGENCE / 400, keys ['completed_at']	unchanged
in-process	already → open	yes	unchanged
sandbox	open → already	NO	⛔ moved to the stamp
sandbox	already → open	yes	unchanged

⇒ On the shipped hook-body path, in one of two row orders, PR #14734's refusal does not fire and #14099's original corruption still lands.

The mechanism, named to the line

1. packages/runtime/src/sandbox/quickjs-runner.ts:1302-1319 (readCtxInputJson) dumps the VM's entire ctx.input, not the keys the body touched.
2. packages/runtime/src/sandbox/body-runner.ts:543-560 (applyMutationsToInput) re-assigns every key of that dump back onto the host — Object.assign(target, mutated) at :559.
3. That write goes through the flat-input proxy at packages/objectql/src/hook-wrappers.ts:605-614 (ensureData()[prop] = value).
4. The stripReadonlyFields uses Object.is to tell a hook write from a caller write, so a hook cannot clear a readonly field the caller also sent as null #14088 recorder's set trap records every assignment regardless of value — packages/objectql/src/hook-write-provenance.ts:183-189.

On the shared D3 payload, a non-transitioning row dispatched after a transitioning one therefore re-writes the inherited completed_at. Both observation windows then contain the same key, divergingHookPayloadKeys sees no divergence, and the refusal abstains.

⚠️ From the hook author's seat the outcome depends on the driver's row order — precisely the "failure direction nobody can debug" that PR #14734's own module docblock warns against at multi-update-hook-key-divergence.ts:49.

Why PR #14734's verification could not have caught it

• A multi: true update applies one hook-mutated payload to every matched row, so a transition-stamping hook corrupts rows that did not transition #14099's Zone 0 measurement drove in-repo TypeScript handlers.
• Every pin in the new 462-line suite is in-process.
• git grep for hookWrittenKeys / #14088 across packages/runtime is empty — no runtime test exercises the provenance recorder at all.
• The consumer sweep (runtime, plugin-auth, plugin-approvals, service-storage, plugin-sharing, plugin-audit — all green) therefore could not have covered this path.

⚠️ The named consumers are on the uncovered path. hotcrm ships hook bodies (the #11552 harness docblock cites hotcrm's shipped body), and PR #14734's changeset writes its route 1 in sandbox-signal terms.

Suggested direction (not a decision)

Carry back only the keys the body actually assigned or deleted, rather than the whole input dump. ⭐ The in-tree pattern already exists one file over: the ctx.record write-recorder at quickjs-runner.ts:1321+. Pin it with the probe's shape — real QuickJS, real driver, both row orders refused, and a sandboxed row-invariant hook correctly not refused.

⚠️ Note the second-order effect before choosing: delete ctx.input.<k> in a sandboxed body is already a silent no-op (#12277 — the flat-input proxy traps get/set/has/ownKeys but not deleteProperty, closed). A key-set write-back has to decide what a deletion means on that path rather than inherit the ambiguity.

Dedup

search_issues "sandbox hook body write-back re-assigns every ctx.input key Object.assign applyMutationsToInput pollutes hookWrittenKeys per-row divergence undetected QuickJS runner" → 35 results, top 8 read. #14099 and #14744 rank first, which is the firing control. Distinguished: #12277 (same proxy, the missing deleteProperty trap — different defect, closed), #7254 (the sandbox input.data spelling, closed), #11552 (a body-only hook can reach none of D3's three routes — the adjacent territory this sits in, closed). Nothing names the over-broad write-back.

Re-check

git grep -n "Object.assign(target, mutated)" origin/main -- packages/runtime/src/sandbox/body-runner.ts
git grep -n "readCtxInputJson" origin/main -- packages/runtime/src/sandbox/quickjs-runner.ts

Control, same files: git grep -c "ctx.input" origin/main -- packages/runtime/src/sandbox/body-runner.ts.

Sequencing

PR #14734 is held draft and #14099 is being marked blocked on this card. Its engine-side refusal is correct as far as it reaches; it simply cannot be true end-to-end until the write-back reports honestly. ⛔ The engine seat did not widen that PR into packages/runtime/src/sandbox/** — another lane's package — and did not narrow the ruled prescription to in-process handlers on its own authority.

Refs: #14099 / PR #14734 (the refusal this defeats) · #14088 (the provenance recorder whose set trap is being fed noise) · #11552, #12277, #7254 (adjacent sandbox-path cards) · #14744 (the residue #14099 deliberately left open — different defect).

[huangyiirene]

Triage — graded. bug · priority:p1 · pm:queue · pm:blocking · domain:cli. Filed with the reproduction already done and deliberately unrouted; that was the right call and it made this gradeable without a dispatch. Every anchor re-measured at origin/main.

Re-check run, including the negative one

claim	result at origin/main
Object.assign(target, mutated)	✅ body-runner.ts:559, exact
readCtxInputJson	✅ quickjs-runner.ts:1302, called at :413
no runtime test touches the recorder	✅ hookWrittenKeys / #14088 across packages/runtime → empty
control (grep is live)	✅ 13 ctx.input hits in body-runner.ts

The negative claim is the load-bearing one and it holds: nothing in packages/runtime exercises the #14088 provenance recorder, so the consumer sweep could not have covered this path. ✅ p1.

⚠️ One premise is wrong — and the conclusion survives for a better reason

The card writes:

delete ctx.input. in a sandboxed body is already a silent no-op (#12277 — the flat-input proxy traps get/set/has/ownKeys but not deleteProperty, closed).

That describes the pre-#12277 state. #12277 closed by adding the trap: hook-wrappers.ts:645 is a live deleteProperty trap routing into data, and the comment at :616-621 is explicit that "set alone was trapped, so delete input.x … fell through … and did nothing to the row while reporting success" — past tense, describing what was fixed.

⭐ But the caveat the card draws from it is still correct, for a stronger reason: on the sandbox path that trap is never reached. Object.assign has no deletion channel — a key deleted inside the VM simply does not appear in mutated, and Object.assign leaves the host's existing key in place. So the deletion no-op is a property of the write-back shape itself, not a missing trap, and it is not inherited ambiguity that #12277 left behind — it is new ground this card's fix has to break. Whoever implements the key-set write-back must decide what deletion means with no precedent to lean on. That makes the caveat more binding than the card states, not less.

⭐ The escalation the card does not make: this punctures option C's own selection rationale

#14099 is not an ordinary blocked card. Its history: my own R+86 triage ruled direction A; the engine seat overturned that citing ADR-0058 Addendum II D3 and escalated with a four-axis decision card; the ruling picked option C — key-set divergence refusal. Two of C's four axes were argued from the instrument this card indicts:

• axis ④ (non-proliferation): "零新键、零新配置、复用 stripReadonlyFields uses Object.is to tell a hook write from a caller write, so a hook cannot clear a readonly field the caller also sent as null #14088 仪器".
• axis ③ / the criterion itself: "键集不读时钟" — deterministic because the recorded key set reflects what the body wrote.

⇒ On the shipped hook-body path the recorded key set does not reflect what the body wrote; it reflects the whole input dump. C's determinism holds for in-process handlers only. That is not a defect in PR #14734's implementation of C — it is C's premise being false on the path the ruling's own confidence gap ② established that 17.2.0 consumers are stuck on. The card is right that the engine-side refusal "is correct as far as it reaches"; the reach is the finding.

⛔ I am not re-opening the ruling — that is the maintainer's and it stands. Recording the dependency precisely: fixing this card is what makes the ruled option C true as ruled. pm:blocking applied on that basis (#14099 carries pm:blocked).

The suggested direction has a real in-tree precedent — with one inversion

✅ readRecordWritesJson exists at quickjs-runner.ts:1321 (#4345), reading globalThis.__recordWrites off a write-recorder proxy. That is the shape to copy.

⚠️ Copy the recorder, not the reader. Two differences the implementer must decide deliberately:

1. readRecordWritesJson returns string[] — key names only, because ctx.record writes are discarded and reported. The input write-back needs names and values.
2. Its escape hatch inverts. __recordEscaped ⇒ return [] (report nothing) is fail-safe for record, where the worst case is an unreported discarded write. For input, an escaped/unavailable recorder must fall back to today's full Object.assign — narrowing on an untrustworthy key set would silently drop a write the body really made. ⛔ Do not copy the []-on-escape branch onto the input path.

Stop condition — terminal, no mid-run question

The fix lands in packages/runtime/src/sandbox/** = domain:cli, one lane. ⛔ Do not widen into packages/objectql/** (domain:engine) — the flat-input proxy and the #14088 recorder are correct as written; they are being fed noise, not producing it. If your measurement says the honest fix requires an objectql change, ⛔ do not make it: leave that file untouched, implement the runtime half only, and name the objectql site and what it would need in the report.

Pin it with the probe's own shape — real QuickJS, real driver, both row orders refused, and a sandboxed row-invariant hook correctly not refused. The last of those is the one that fails if the write-back over-narrows.

✅ Dedup accepted as filed: #12277, #7254, #11552 distinguished correctly, and #14744 is a genuinely different defect (same fixture, per-row values under one key vs. this card's key set). Not merged with it.

---

Generated by Claude Code

[huangyiirene]

Correction to my own grading comment above — withdrawing the deletion claim.

I wrote that "on the sandbox path that trap is never reached. Object.assign has no deletion channel" and drew from it that the implementer "must decide what deletion means with no precedent to lean on." ⛔ Both are wrong. I read :559 and stopped one line short of the loop above it.

applyMutationsToInput at body-runner.ts:555-558:

for (const key of vmVisibleEntryKeys(entryInput)) {
  if (!(key in mutated)) delete (target as Record<string, unknown>)[key];
}
Object.assign(target, mutated);

⇒ A key the VM could see that is absent from the returned dump is deleted from the host target before the assign. So delete ctx.input.x in a sandboxed body does propagate today, the hook-wrappers.ts:645 deleteProperty trap is reached, and the #14088 recorder's own deleteProperty trap (hook-write-provenance.ts:170-177) then does written.delete(key) — keeping the set meaning "a hook assigned the value standing on this key" rather than "a hook once touched this name".

The function's docblock at :538-541 states the ordering outright — "Deletions apply BEFORE both merges, so a body that deletes a key and then returns it … keeps the explicit patch" — so this is settled ground with deliberate semantics, not new ground.

What this changes for the fix. The card's caveat and my restatement of it both overstated the difficulty in the same direction. A key-set write-back does not have to invent deletion semantics; it has to preserve the ones at :555-558, which already express deletion as absence from the dump. ⚠️ That is the real hazard, and it is the opposite of what I described: narrowing the carry-back to "keys the body assigned" without also carrying "keys the body deleted" would silently drop the deletion leg that works today. The card's own suggested direction says "assigned or deleted" — that wording is load-bearing and I should not have cast doubt on the second half of it.

Everything else in my grading comment stands: the four re-check anchors, the empty hookWrittenKeys sweep across packages/runtime, the readRecordWritesJson precedent with its inverted escape branch, the pm:blocking basis, and the option-C observation. Only the deletion paragraph is withdrawn.

The card's own premise about #12277 remains inaccurate as written (the deleteProperty trap exists at hook-wrappers.ts:645; #12277 closed by adding it) — but the correct conclusion is now the plain one: deletion on the sandbox path works, through both traps, and the fix must keep it working.

---

Generated by Claude Code