fix(runtime): carry back only the keys a sandboxed hook body wrote - #14826
Conversation
`applyMutationsToInput` re-asserted every key of the post-run `ctx.input` dump onto the engine's flat-input Proxy, whose `set` trap the hook-write provenance recorder watches. A body that touched nothing therefore "wrote" every payload key, which made the per-row divergence refusal on a `multi: true` update blind in one of the two driver row orders. The QuickJS runner now arms a write recorder on `ctx.input` for hook bodies and reports the keys the body assigned, defined or deleted; the write-back re-asserts only those. The absence-from-dump deletion leg is unchanged, a write made through a value read from the input is still carried from the dump, and an unavailable recorder falls back to the full assign. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza
…ts the corruption Without it the ablated run stops at "no refusal was raised" and never reaches the row-state assertions, so the moved `completed_at` -- the defect itself -- is not reported by the pin that exists for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza
📓 Docs Drift CheckThis PR changes 1 package(s): 1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 24 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 5a18702f1b33556ace420e8d518df1971232fcda && git checkout 5a18702f1b33556ace420e8d518df1971232fcda
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f116b8f8d1b43ec2bfd64fbf0ebb0cf3c301b1c8 949a0a7f6be50916358740385927d88e28c66b06 && git checkout -B drift-repro f116b8f8d1b43ec2bfd64fbf0ebb0cf3c301b1c8 && git merge --no-ff 949a0a7f6be50916358740385927d88e28c66b06
node scripts/docs-audit/affected-docs.mjs --json f116b8f8d1b43ec2bfd64fbf0ebb0cf3c301b1c8
|
…ndbox-hook-write-back-key-set
|
PM seat — standing down on this PR's only red, because it is not this PR's. Recording the basis rather than asserting it. The failing check
Why it is not this PR'sVerified against the tree rather than taken from the report: Five files, all ⇒ Carded as #14832 ( Spending the one re-run allowed for a failure established as not-this-PR's. The hang is intermittent — the same child calibrated at 7 s minutes earlier in the same job — so a re-run is a real reading, not a wish. If it fails again it is still #14832's and this PR is not held for it. Merge follow-through — verified, and the ledger is untouchedThis PR was sent back one round because its branch predated #14785, which replaced the instrument it had measured itself with. That is resolved:
One declared narrowing, accepted: the whole-package vitest run (215 files / 3126 tests) is the pre-merge measurement, carried forward on three checkable facts — the population comes from the tool's own config; It was a narrowing rather than a wait because the shared verify lock was held continuously by another seat for 20+ minutes across two full queue budgets (still holding at 1225 s). Reported as a lock observation, ⛔ not acted on — correct; the lock is not a thing to route around. Flipping ready and arming. Generated by Claude Code |
Fixes #14758
The sandbox hook write-back now carries back the keys the body wrote, not every
key the post-run dump can see — so #14099's per-row divergence refusal is true on the
shipped hook-body path in either driver row order.
Session: https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza
The mechanism, and what moved
applyMutationsToInputended inObject.assign(target, mutatedInput), andmutatedInputis thereadCtxInputJsondump of the wholectx.input.targetis the engine's flat-input Proxy, so each of those assignments is a
setthe #14088provenance recorder records. A body that touched nothing therefore reported writing
every payload key.
Under D3 all matched rows share ONE payload, so the noise was order-dependent.
Measured here on this branch by reverting the fix in place (ablation below), with
#14099's own fixture driven through a real QuickJS body:
{status}{status,completed_at}{status,completed_at}{status,completed_at}In the second order the already-done row inherits
completed_atfrom thetransitioning row's write onto the shared payload, the blanket write-back re-asserts
it as that row's own write, the windows match, and the batch proceeds — landing
#14099's original corruption on a row that never transitioned.
The QuickJS runner now arms an input write-recorder on
ctx.inputfor hook bodies —the
readRecordWritesJson/__recordWritesrecorder shape from #4345 — and reportsthe keys the body assigned, defined or deleted; the write-back re-asserts only those.
What was deliberately NOT changed
body-runner.ts:555-558is untouched. It was neverexpressed by the merge: it reads deletion from the ENTRY snapshot as
absence-from-the-dump, before both merges, and the docblock at
:538-541statesthat ordering deliberately. A key the body never touched is present in the dump and
so is never deleted. Narrowing the carry-back to keys-the-body-assigned without the
deletion leg would have dropped a leg that works today — pinned by case 4 below.
inputthe branchesinvert: an empty key set is a trustworthy answer (armed, saw no write) and does
narrow; the untrustworthy answer is an ABSENT key set, and the host falls back to
today's full
Object.assignon it.packages/objectqlis untouched. The flat-input proxy and thestripReadonlyFieldsusesObject.isto tell a hook write from a caller write, so a hook cannot clear a readonly field the caller also sent as null #14088 recorderare correct as written; they were being fed noise. No objectql change is required —
see the objectql-sites section below.
One leg the recorder alone cannot cover, named rather than hidden
A body that writes THROUGH a value it read —
ctx.input.meta.x = 1— trips no trapon
ctx.inputitself, so the recorder cannot listmeta, and dropping it would beexactly the silent loss this card exists to end.
carriedInputKeysreads it from thedump the narrowest way available: an object-valued entry key whose dumped value no
longer matches the entry snapshot was written through, and is carried. Primitives
need no such leg — a primitive cannot be mutated in place, so every change to one is
an assignment the recorder already saw. That confinement is what keeps this from
re-widening into the value comparison #14099's ruling refused.
File face
packages/runtime/src/sandbox/only —domain:cli, one lane.packages/runtime/src/sandbox/quickjs-runner.tspackages/runtime/src/sandbox/body-runner.tspackages/runtime/src/sandbox/script-runner.tspackages/runtime/src/sandbox/hook-input-writeback-key-set.integration.test.ts.changeset/sandbox-hook-input-writeback-key-set.mdorigin/mainwas merged in ate149585290(a merge, not a rebase). That mergebrought no change to
packages/runtime/src/— onlypackage.json, the newtsconfig.test.jsonand the newtest-typecheck-debt.jsonfrom #14785. The file faceabove is unchanged by it.
Measurements
All numbers below are from the merged head
e149585290, which is this PR's head,except the one row explicitly marked pre-merge.
The pin — real
ObjectQL+ realSqlDriver(better-sqlite3) + realQuickJSScriptRunnerbehindhookBodyRunnerFactory, the probe's own shape. Fourcases; the dispatch order is asserted from inside the bodies rather than assumed:
MULTI_UPDATE_HOOK_KEY_DIVERGENCEwith status 400 and the diverging key list naming
completed_at, neither rowwritten;
matched row (the over-narrowing guard the grading comment made mandatory);
delete ctx.input.internal_note— still propagates.On the merged head: 4 passed.
Ablation (the fix reverted in place, restore proved by hash). The three edited
source files were checked out from
origin/main; the mutation was proved on disk bygit hash-objectmatching theorigin/mainblob and differing from theHEADblob,plus zero occurrences of
carriedInputKeysand__inputWrites. No build was involved— the pin imports these files from
srcby relative path. Result: 1 failed, 3 passedof 4, and the failure reports both halves of the finding:
Restored from
HEAD,git diff HEADempty and all three blobs hash-equal toHEAD,the same four cases read 4 passed.
Suites and gates:
pnpm --filter @objectstack/runtime exec vitest run src/sandbox/, merged headpnpm --filter @objectstack/runtime typecheck(now chainscheck:test-typecheck), merged headpnpm --filter @objectstack/runtime test(whole package) — pre-merge, carried forward under the declared narrowing belowpnpm --filter @objectstack/dogfood test(real shipped hook bodies)pnpm exec eslint . --no-inline-config(whole repo, not narrowed)scripts/pm/dispatch-gates.mjs --commands)pnpm check:dual-build-cjs-loads(after the full workspace build)pnpm check:nul-bytesEvery gate exit code was captured by redirecting to a file first and reading the
status before any pipe, and the verdicts quoted are the gates' own printed lines.
The one declared narrowing, stated so it can be checked rather than trusted. The
whole-package
vitest runwas measured before the merge, not after. Three factsclose the gap:
testscript is a barevitest run, whose inputs arepackages/runtime/vitest.config.tsandpackages/runtime/src/**;git diff --name-only ae0b1d702b HEADover each of those paths —src,test,the package vitest config and both root vitest configs — returns 0 files;
packages/runtimearepackage.json(thetypecheckscript, nottest),tsconfig.test.jsonandtest-typecheck-debt.json. Vitest reads none of them;check:test-typecheckreads all three, and it was run separately on the merged head, green.
So the source under test is byte-identical either side of the merge, and the parts
that actually exercise this change were re-run on the merged head anyway (the pin, and
the whole sandbox directory). This was a narrowing rather than a wait because the
shared verify lock was held continuously by another seat for over 20 minutes across
two full queue budgets.
The two NOT MEASURED families are structural, not red:
node scripts/check-test-completeness.mjs— the family list names it with noargument, and the gate itself prints that the local reading for it is NOT MEASURED
and is not a red;
node scripts/pm/check-half-states.mjs— a PM-board gate that needs an issuelisting this container cannot make; it prints the same NOT MEASURED refusal and is
unrelated to this diff.
The test-layer type check, on the instrument that is actually live
The new pin is a 283-line test file, and #14785 changed what reads it. On the
merged tree
packages/runtime'stypecheckis no longer a baretsc --noEmit: itchains
check:test-typecheck, which compiles the test layer under the newpackages/runtime/tsconfig.test.jsonagainst the EXACT, shrink-only, identity-pinnedpackages/runtime/test-typecheck-debt.json. An unledgered file with even one error isred, and this pin is unledgered.
pnpm --filter @objectstack/runtime typecheckon the merged head, exit 0:So the new pin carries zero type errors, and nothing was ledgered:
git diff origin/mainovertest-typecheck-debt.json,tsconfig.test.json,package.jsonandscripts/check-type-check-coverage.mjsis empty. Noany, no@ts-expect-error, nodisable comment and no weakened assertion was added to reach that.
A green here would say nothing if tsc never read the file, so that was measured rather
than assumed.
tsc --noEmit --listFiles -p tsconfig.test.jsonputs the new pin in theprogram (1 hit) along with all three edited source files (3 hits), 913
.tsfiles intotal; the raw run reports 191 diagnostics over 27 files — matching the ledger
exactly — and zero of them name the new pin.
An earlier revision of this PR body reported "206 diagnostics, exactly the recorded
206" against
TEST_DEBT['@objectstack/runtime']inscripts/check-type-check-coverage.mjs. That reading was correct on the pre-mergebase and is retired: #14785 graduated that entry out of the per-package ledger and
replaced it with the per-file one above. The paragraph is corrected rather than
deleted because the number moved (206 to 191) and the instrument changed, and a stale
figure in a PR body is what sends the next reader to the wrong gate.
objectql sites, named as the stop condition requires
None needed. The honest fix did not require an objectql change: the flat-input
proxy (
hook-wrappers.ts:582-660) and the recorder (hook-write-provenance.ts:163+)both behave correctly once the write-back stops asserting untouched keys. The only
objectql behaviour this changes is what those two now observe, which is the point.
For the record, the sites this fix reads against and leaves untouched: the
hook-wrappers.tssettrap that routes intodata, itsdeletePropertytrap at:645, and thehook-write-provenance.tssetanddeletePropertytraps.Card premises
The card's own #12277 premise is inaccurate as written, and the grading comment's
first reading of it was withdrawn; the withdrawal is what this PR implements —
deletion on the sandbox path works today through both traps, and the fix preserves
it. Everything else in the card re-measured true at
origin/main.Draft, not auto-merge: the PM seat lands this.
🤖 Generated with Claude Code
https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza