From d82f7e46263f75b4bc5185edaf69eaaa4d44f621 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 02:46:06 +0000 Subject: [PATCH 1/2] fix(runtime): carry back only the keys a sandboxed hook body wrote `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 Claude-Session: https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza --- .../sandbox-hook-input-writeback-key-set.md | 50 ++++ packages/runtime/src/sandbox/body-runner.ts | 135 ++++++++- ...nput-writeback-key-set.integration.test.ts | 279 ++++++++++++++++++ .../runtime/src/sandbox/quickjs-runner.ts | 154 +++++++++- packages/runtime/src/sandbox/script-runner.ts | 28 ++ 5 files changed, 644 insertions(+), 2 deletions(-) create mode 100644 .changeset/sandbox-hook-input-writeback-key-set.md create mode 100644 packages/runtime/src/sandbox/hook-input-writeback-key-set.integration.test.ts diff --git a/.changeset/sandbox-hook-input-writeback-key-set.md b/.changeset/sandbox-hook-input-writeback-key-set.md new file mode 100644 index 0000000000..b3690a0161 --- /dev/null +++ b/.changeset/sandbox-hook-input-writeback-key-set.md @@ -0,0 +1,50 @@ +--- +"@objectstack/runtime": patch +--- + +fix(runtime): the sandbox hook write-back carries the keys the body wrote, not every key it could see + +A sandboxed `before*` hook body's mutations were written back onto the engine's +payload with `Object.assign(target, mutatedInput)`, where `mutatedInput` is the +whole post-run `ctx.input` — every key the body could see, touched or not. That +target is the engine's flat-input Proxy, and every assignment through it is +recorded by the hook-write provenance recorder. So the write-back was reporting +that a body which touched nothing had written every payload key. + +The per-row divergence refusal that ends "one hook-mutated payload applied to +every matched row" on a `multi: true` update reads exactly that recording: per +row, the key set the hook chain assigned, refusing the batch when two rows +disagree. All rows share ONE payload, so the noise was order-dependent — with a +transition stamp bound to `beforeUpdate`, one open row and one already-completed +row: + +- already-done row dispatched first: the windows differ, the batch is refused; +- open row dispatched first: the already-done row inherits `completed_at` from + the transitioning 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 refusal + abstains, moving a `completed_at` on a record that never transitioned. + +The refusal was therefore true for in-process handlers and, in one of two driver +row orders, silently untrue for shipped hook bodies. The QuickJS runner now arms +a write recorder on `ctx.input` for hook bodies — the same recorder shape +`ctx.record` has used since the discarded-record-write report — and the +write-back re-asserts only the keys the body assigned, defined or deleted. + +Nothing else about the channel moves: + +- deletion still propagates, unchanged. It was never expressed by the merge: + the write-back reads it from the entry snapshot as absence-from-the-dump, + before both merges, and a key the body never touched is present in the dump + and so is never deleted. +- a write made THROUGH a value read from the input (`ctx.input.meta.x = 1`) + trips no trap on `ctx.input` itself, so it is carried from the dump instead: + an object-valued entry key whose dumped value no longer matches the entry + snapshot was written through. Primitives need no such leg — a primitive + cannot be mutated in place. +- when the recorder cannot speak — an older runner, a read that failed, a body + that replaced `ctx.input` with a non-object — the write-back falls back to + the full assign it did before. Narrowing on a key set that is not trustworthy + would silently drop a write the body really made. An empty key set is a + different answer from an absent one and does narrow. + +Hook bodies only; the action path has no input write-back to inform. diff --git a/packages/runtime/src/sandbox/body-runner.ts b/packages/runtime/src/sandbox/body-runner.ts index dc0edd2508..f9df19c874 100644 --- a/packages/runtime/src/sandbox/body-runner.ts +++ b/packages/runtime/src/sandbox/body-runner.ts @@ -504,6 +504,86 @@ function vmVisibleEntryKeys(entryInput: unknown): string[] { return out; } +/** + * [#14758] Which keys of the exit dump the write-back should re-assert, or + * `undefined` to assert all of them (the pre-#14758 behaviour). + * + * Two sources, unioned, and neither is sufficient alone: + * + * 1. `writtenKeys` — what the VM's `ctx.input` recorder saw the body assign, + * define or delete. Exact for everything that goes through a trap, which + * includes the cases a dump diff cannot see: a computed key, an + * `Object.assign(ctx.input, …)`, an alias, and — the one #14088 exists for + * — an IDEMPOTENT write, a body assigning the value already standing on the + * key. Keys it names that the dump does not carry are dropped: a key + * assigned and then deleted, or assigned `undefined`, is absent from the + * dump and belongs to the deletion leg, not to this merge. + * 2. Object-valued entry keys whose dumped value no longer matches the entry + * snapshot. A body that writes THROUGH a value it read (`ctx.input.meta.x = + * 1`) never trips a trap on `ctx.input`, so (1) cannot list it and dropping + * it would be exactly the silent loss this card exists to end. The + * comparison is confined to keys whose ENTRY value is an object because a + * primitive cannot be mutated in place — every change to one is an + * assignment (1) already saw — and confining it there is what keeps this + * leg from re-widening into the value diff #14099's ruling refused. + * + * `undefined` (no narrowing) whenever the evidence is not there: no recorder, + * or no usable entry snapshot to read leg 2 from. + */ +function carriedInputKeys( + mutated: Record, + writtenKeys: readonly string[] | undefined, + entryInput: unknown, +): string[] | undefined { + if (!writtenKeys) return undefined; + if (!entryInput || typeof entryInput !== 'object' || Array.isArray(entryInput)) return undefined; + const entry = entryInput as Record; + const carried = new Set(); + for (const key of writtenKeys) { + if (Object.prototype.hasOwnProperty.call(mutated, key)) carried.add(key); + } + for (const [key, before] of Object.entries(entry)) { + if (carried.has(key)) continue; + if (!before || typeof before !== 'object') continue; + if (!Object.prototype.hasOwnProperty.call(mutated, key)) continue; + if (!sameJsonValue(before, mutated[key])) carried.add(key); + } + return [...carried]; +} + +/** + * Structural equality under JSON semantics, used only to answer "did the body + * write through this object-valued key?". + * + * Key ORDER is deliberately not significant: a JSON round-trip through the VM + * preserves insertion order for string keys but reorders integer-like ones, and + * a reorder is not a write. Every failure direction is the safe one — anything + * this cannot prove equal is reported as changed and therefore CARRIED, which + * is the pre-#14758 behaviour for that key. That covers the host values JSON + * cannot represent (a `Date` arrives back as a string and compares unequal, so + * it is carried exactly as it was before this card). + * + * Terminates on a cyclic `a`: `b` is always JSON-parsed and therefore finite, + * so the walk is bounded by `b`'s depth. + */ +function sameJsonValue(a: unknown, b: unknown): boolean { + if (a === b) return true; + if (a === null || b === null) return false; + if (typeof a !== 'object' || typeof b !== 'object') return false; + if (Array.isArray(a) !== Array.isArray(b)) return false; + if (Array.isArray(a)) { + const other = b as unknown[]; + return a.length === other.length && a.every((v, i) => sameJsonValue(v, other[i])); + } + const left = a as Record; + const right = b as Record; + const leftKeys = Object.keys(left); + if (leftKeys.length !== Object.keys(right).length) return false; + return leftKeys.every( + (k) => Object.prototype.hasOwnProperty.call(right, k) && sameJsonValue(left[k], right[k]), + ); +} + /** * Write one settled body's mutations back onto the host `ctx.input`. * @@ -539,6 +619,51 @@ function vmVisibleEntryKeys(entryInput: unknown): string[] { * Deletions apply BEFORE both merges, so a body that deletes a key and then * returns it (`delete ctx.input.x; return { x: 1 };`) keeps the explicit patch * — the return value is the later, more deliberate statement of the two. + * + * ## [#14758] Why the merge is a KEY SET and no longer the whole dump + * + * `mutatedInput` is the whole post-run `ctx.input`, so `Object.assign(target, + * mutated)` re-asserted every key a body could see, touched or not. `target` is + * the engine's flat-input Proxy, whose `set` trap the #14088 hook-write + * provenance recorder watches — so the write-back was TELLING the engine that a + * body which touched nothing had written every payload key. + * + * #14099's per-row divergence refusal reads exactly that recording: per row, + * the key set the hook chain assigned, refusing a `multi: true` batch when two + * rows disagree. Under D3 all rows share ONE payload, so the noise was + * order-dependent — measured on `origin/main` with #14099's own fixture driven + * through a QuickJS body, one open row and one already-done row: + * + * ``` + * already -> open row1 {status} row2 {status,completed_at} refused + * open -> already row1 {status,completed_at} row2 {status,completed_at} NOT refused + * ``` + * + * In the second order the already-done row inherits `completed_at` from the + * transitioning row's write onto the shared payload, the blanket write-back + * re-asserts it as that row's own write, the windows match — and #14099's + * corruption lands on a row that never transitioned. So the carry-back is now + * the body's own key set ({@link ScriptResult.mutatedInputKeys}). + * + * Three properties of the narrowing, each load-bearing: + * + * - **Deletion is untouched.** It was never expressed by the merge: the loop + * above reads it from the ENTRY snapshot as absence-from-dump, and a key the + * body never touched is present in the dump and so is never deleted. A + * narrowing that had also filtered the deletion leg would have dropped a leg + * that works. + * - **A key set that cannot speak is not narrowed on.** `mutatedInputKeys` + * `undefined` — a runner with no recorder, a read that failed, a body that + * replaced `ctx.input` with a non-object — falls back to the full + * `Object.assign` verbatim. `[]` is a real answer and does narrow. + * - **Writes made THROUGH a value are still carried.** `ctx.input.meta.x = 1` + * mutates an object the body reached from `ctx.input`; no trap on `ctx.input` + * itself ever fires, so the recorder cannot list `meta`. The dump is the only + * witness for those, and {@link carriedInputKeys} reads it 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 saw. */ function applyMutationsToInput( engineCtx: any, @@ -556,7 +681,15 @@ function applyMutationsToInput( for (const key of vmVisibleEntryKeys(entryInput)) { if (!(key in mutated)) delete (target as Record)[key]; } - Object.assign(target, mutated); + const carried = carriedInputKeys(mutated, result.mutatedInputKeys, entryInput); + if (carried === undefined) { + // The recorder could not speak — pre-#14758 behaviour, verbatim. + Object.assign(target, mutated); + } else { + for (const key of carried) { + (target as Record)[key] = mutated[key]; + } + } } if ( result.value && diff --git a/packages/runtime/src/sandbox/hook-input-writeback-key-set.integration.test.ts b/packages/runtime/src/sandbox/hook-input-writeback-key-set.integration.test.ts new file mode 100644 index 0000000000..c2dab3379d --- /dev/null +++ b/packages/runtime/src/sandbox/hook-input-writeback-key-set.integration.test.ts @@ -0,0 +1,279 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#14758] The sandbox write-back carries back the keys the BODY wrote — so + * #14099's per-row divergence refusal is true for shipped hook bodies too, in + * either row order. + * + * ## What was measured broken + * + * #14099's refusal reads the #14088 provenance recorder: per row, the set of + * payload keys the hook chain ASSIGNED. On the in-process face that set is + * exact. On the sandbox face it was the whole input dump, because + * `applyMutationsToInput` ended in `Object.assign(target, mutatedInput)` and + * `readCtxInputJson` dumps every key of `ctx.input`, touched or not. Every + * assignment goes through the flat-input proxy, whose `set` trap the recorder + * watches — so a body that touched nothing still "wrote" every key. + * + * On D3's shared batch payload that is order-dependent, and it defeats the + * refusal in exactly one of the two orders. #14099's own fixture, driven + * through a QuickJS body (measured on `origin/main` before this fix): + * + * | dispatch order | row 1 window | row 2 window | refused? | + * |------------------|-----------------------|-----------------------|----------| + * | open → already | {status,completed_at} | {status,completed_at} | **NO** | + * | already → open | {status} | {status,completed_at} | yes | + * + * In the first order the already-done row inherits `completed_at` from the + * transitioning row's write onto THE shared payload, the blanket write-back + * re-assigns it, both windows match, and the batch proceeds — landing #14099's + * original corruption: a `completed_at` that moves on a row that never + * transitioned. + * + * ## Why this harness + * + * The defect lives in the composition of three real components no unit mock + * exercises: QuickJS marshalling, objectql's flat-input proxy, and the #14088 + * recorder's `set` trap. So this drives REAL `ObjectQL` + REAL `SqlDriver` + * (better-sqlite3) + REAL `QuickJSScriptRunner` behind `hookBodyRunnerFactory` + * — the wiring `AppPlugin` performs — exactly as the reviewer's probe did. + * + * ## The four cases, and why the last one is mandatory + * + * 1. sandbox, open → already — refused (the order that used to land the + * corruption); + * 2. sandbox, already → open — refused (the order that always was); + * 3. sandbox, row-INVARIANT body — NOT refused, and its write LANDS on every + * matched row. This is the over-narrowing guard: a write-back that carried + * back too little would make an honest batch refuse, or drop the write; + * 4. sandbox deletion — `delete ctx.input.x` still propagates + * (`body-runner.ts`'s absence-from-dump leg, #12277), because a key-set + * write-back that carried only ASSIGNMENTS would silently drop it. + * + * The dispatch order is asserted from inside the bodies rather than assumed, + * so a driver that stopped returning matched rows in insertion order fails + * loudly here instead of quietly turning case 1 into a second copy of case 2. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { ObjectQL, bindHooksToEngine } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { hookBodyRunnerFactory } from './body-runner.js'; +import { QuickJSScriptRunner } from './quickjs-runner.js'; +import { + captureExpectedReadRefusals, + type ExpectedReadRefusalCapture, +} from '../expected-read-refusal-noise.js'; + +const TASK = { + name: 'duly_task', + fields: { + title: { type: 'text' }, + bucket: { type: 'text' }, + status: { type: 'text' }, + completed_at: { type: 'text' }, + touched_by: { type: 'text' }, + internal_note: { type: 'text' }, + }, +}; + +const STAMP = '2026-09-03T09:00:00.000Z'; +const EARLIER = '2026-01-01T00:00:00.000Z'; +const ABSENT_TENANCY_TABLE = 'sys_organization'; + +/** + * #14099's fixture as a shipped hook BODY: stamp `completed_at` on the + * transition into `done`, and only on the transition. Reports the row it saw + * so the test can assert the order it actually drove. + */ +const TRANSITION_STAMP_SOURCE = ` + ctx.log.info('row', { title: ctx.previous.title, index: ctx.dispatch ? ctx.dispatch.index : null }); + if (ctx.input.status === 'done' && ctx.previous.status !== 'done') { + ctx.input.completed_at = '${STAMP}'; + } +`; + +/** Row-INVARIANT: the same key on every row, whatever the row looks like. */ +const ROW_INVARIANT_SOURCE = ` + ctx.log.info('row', { title: ctx.previous.title, index: ctx.dispatch ? ctx.dispatch.index : null }); + ctx.input.touched_by = 'hook'; +`; + +/** Row-invariant DELETION — the leg `body-runner.ts:555-558` already carries. */ +const ROW_INVARIANT_DELETE_SOURCE = ` + ctx.log.info('row', { title: ctx.previous.title, index: ctx.dispatch ? ctx.dispatch.index : null }); + delete ctx.input.internal_note; +`; + +type Boot = { + engine: ObjectQL; + seen: any[]; + dir: string; + noise: ExpectedReadRefusalCapture; +}; + +describe('#14758 — the sandbox write-back carries the keys the body wrote', () => { + let booted: Boot | null = null; + + afterEach(async () => { + try { await booted?.engine.destroy(); } catch { /* noop */ } + if (booted?.dir) rmSync(booted.dir, { recursive: true, force: true }); + booted = null; + }); + + async function boot(source: string): Promise { + const dir = mkdtempSync(join(tmpdir(), 'os-14758-')); + const driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: join(dir, 'data.sqlite') }, + useNullAsDefault: true, + }); + const noise = captureExpectedReadRefusals([ABSENT_TENANCY_TABLE]); + noise.captureDriver(driver); + await driver.initObjects([TASK]); + const engine = new ObjectQL(); + noise.captureEngine(engine); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(TASK as any, 'duly'); + + const seen: any[] = []; + const logger = { + debug: () => {}, + info: (_m: string, meta?: any) => { seen.push(meta); }, + warn: () => {}, + error: () => {}, + }; + engine.setDefaultBodyRunner( + hookBodyRunnerFactory(new QuickJSScriptRunner(), { ql: engine, appId: 'duly', logger }), + ); + bindHooksToEngine(engine, [{ + name: 'duly_task_stamp', + object: 'duly_task', + events: ['beforeUpdate'], + body: { language: 'js', source, capabilities: ['log'] }, + } as any], { packageId: 'duly' }); + + booted = { engine, seen, dir, noise }; + return booted; + } + + /** Seeds #14099's two rows in the requested dispatch order. */ + async function seed(engine: ObjectQL, order: 'open-first' | 'already-first') { + const open = { + title: 'open', bucket: 'b1', status: 'open', + completed_at: null, internal_note: 'n1', + }; + const already = { + title: 'already', bucket: 'b1', status: 'done', + completed_at: EARLIER, internal_note: 'n2', + }; + const first = order === 'open-first' ? open : already; + const second = order === 'open-first' ? already : open; + await engine.insert('duly_task', first as any); + await engine.insert('duly_task', second as any); + } + + const byTitle = async (engine: ObjectQL, title: string) => + ((await engine.find('duly_task', { where: { title } })) as any[])[0]; + + it.each([ + ['open-first', ['open', 'already']], + ['already-first', ['already', 'open']], + ] as const)( + 'the transition stamp is refused in BOTH row orders (%s) and nothing is written', + async (order, expectedOrder) => { + const { engine, seen, noise } = await boot(TRANSITION_STAMP_SOURCE); + await seed(engine, order); + seen.splice(0); + + let err: any; + try { + await engine.update( + 'duly_task', + { status: 'done' }, + { multi: true, where: { bucket: 'b1' } } as any, + ); + } catch (e) { err = e; } + + // The order this case actually drove — asserted, never assumed. + expect(seen.map((o) => o.title)).toEqual(expectedOrder); + expect(seen.map((o) => o.index)).toEqual([0, 1]); + + // ADR-0112 envelope: code AND status, plus the key that diverged. + expect(err).toBeDefined(); + expect(err?.code).toBe('MULTI_UPDATE_HOOK_KEY_DIVERGENCE'); + expect(err?.status).toBe(400); + expect(err?.keys).toEqual(['completed_at']); + + // #14099's corruption: the already-done row's stamp must not move — and + // the refusal lands BEFORE any write, so neither row changed at all. + const already = await byTitle(engine, 'already'); + const open = await byTitle(engine, 'open'); + expect(already.completed_at).toBe(EARLIER); + expect(already.status).toBe('done'); + expect(open.status).toBe('open'); + expect(open.completed_at ?? null).toBeNull(); + + expect(noise.silentChannels()).toEqual([]); + }, + 60000, + ); + + it('a row-INVARIANT sandboxed body is NOT refused, and its write lands on every row', async () => { + const { engine, seen, noise } = await boot(ROW_INVARIANT_SOURCE); + await seed(engine, 'open-first'); + seen.splice(0); + + await engine.update( + 'duly_task', + { status: 'done' }, + { multi: true, where: { bucket: 'b1' } } as any, + ); + + expect(seen.map((o) => o.title)).toEqual(['open', 'already']); + + const already = await byTitle(engine, 'already'); + const open = await byTitle(engine, 'open'); + // The body's write reached the SET clause for both matched rows — the + // over-narrowing guard. + expect(open.touched_by).toBe('hook'); + expect(already.touched_by).toBe('hook'); + // …and the caller's own payload key still landed. + expect(open.status).toBe('done'); + expect(already.status).toBe('done'); + // The already-done row's stamp is untouched: nothing carried it back. + expect(already.completed_at).toBe(EARLIER); + expect(open.completed_at ?? null).toBeNull(); + + expect(noise.silentChannels()).toEqual([]); + }, 60000); + + it('a row-invariant sandboxed DELETE still propagates (the absence-from-dump leg)', async () => { + const { engine, seen, noise } = await boot(ROW_INVARIANT_DELETE_SOURCE); + await seed(engine, 'open-first'); + seen.splice(0); + + await engine.update( + 'duly_task', + { status: 'done', internal_note: 'CALLER-SENT' }, + { multi: true, where: { bucket: 'b1' } } as any, + ); + + expect(seen.map((o) => o.title)).toEqual(['open', 'already']); + + const already = await byTitle(engine, 'already'); + const open = await byTitle(engine, 'open'); + // The caller sent `internal_note`; every row's body deleted it from the + // shared payload, so the stored rows keep their own originals. + expect(open.internal_note).toBe('n1'); + expect(already.internal_note).toBe('n2'); + expect(open.status).toBe('done'); + expect(already.status).toBe('done'); + + expect(noise.silentChannels()).toEqual([]); + }, 60000); +}); diff --git a/packages/runtime/src/sandbox/quickjs-runner.ts b/packages/runtime/src/sandbox/quickjs-runner.ts index 6756fb725e..68662eca4c 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.ts @@ -411,11 +411,22 @@ export class QuickJSScriptRunner implements ScriptRunner { const value = resStr === 'null' ? undefined : safeJsonParse(resStr); // Capture mutated ctx.input so the host can write through. const mutatedInput = readCtxInputJson(vm); + // …and, on the hook path, WHICH of those keys the body actually + // wrote (#14758), so the write-back carries the body's own key set + // rather than re-asserting the whole dump onto the engine's payload. + const mutatedInputKeys = + args.origin.kind === 'hook' ? readInputWritesJson(vm) : undefined; // …and the ctx.record writes the host will NOT write through, so it // can say so instead of dropping them silently (#4345). const droppedRecordWrites = args.ctx.record !== undefined ? readRecordWritesJson(vm) : undefined; - return { value, mutatedInput, droppedRecordWrites, durationMs: Date.now() - start }; + return { + value, + mutatedInput, + mutatedInputKeys, + droppedRecordWrites, + durationMs: Date.now() - start, + }; } const budget = budgetError(pumps); @@ -859,6 +870,115 @@ export class QuickJSScriptRunner implements ScriptRunner { } sugar.value.dispose(); + // [#14758] The hook path's INPUT write-recorder — the instrument that lets + // `applyMutationsToInput` carry back the keys the body wrote instead of + // every key it could see. + // + // ## What it repairs + // + // The write-back's only witness used to be `readCtxInputJson`'s dump, which + // is the WHOLE `ctx.input`, touched or not. Every key of that dump was then + // re-assigned onto the engine's flat-input proxy, and every one of those + // assignments is a `set` the #14088 provenance recorder + // (`objectql/src/hook-write-provenance.ts`) records. So on the shipped + // hook-body path a body that touched NOTHING still "wrote" every payload + // key — and #14099's per-row divergence refusal, whose whole criterion is + // that recorded key set, could not see a divergence that was there. On D3's + // one shared batch payload the outcome depended on the driver's row order: + // the row that transitions writes `completed_at` onto the payload, the next + // row inherits it, the blanket write-back re-asserts it, both observation + // windows match, and the batch proceeds. + // + // ## Shape copied from the `ctx.record` recorder below, with one inversion + // + // Same reason it is a recorder and not a post-run dump diff: the traps fire + // for computed keys, `Object.assign(ctx.input, …)`, and aliases + // (`const i = ctx.input; i.x = 1`), and an idempotent write + // (`ctx.input.status = 'done'` on a row already `done`) is a write — which + // is exactly the #14088 distinction a value diff cannot make. + // + // ⛔ The ESCAPE branch is inverted on purpose. `__recordEscaped ⇒ []` is + // fail-safe for `record`, where the worst case is an unreported discarded + // write. Here `[]` is a TRUSTWORTHY reading — "the recorder was armed and + // saw no write" — and narrowing to nothing is the correct answer for it. + // The untrustworthy reading is the ABSENT one (`__inputWrites` null or + // unreadable), and the host answers that by falling back to today's full + // `Object.assign`: narrowing on a key set that cannot speak would silently + // drop a write the body really made. + // + // Installed AFTER the `ctx.dispatch` / `ctx.input.options` graft above, so + // the graft's own `defineProperty` is not recorded as a body write. Hooks + // only: the action path has no `applyMutationsToInput` counterpart + // (`ScriptContext.record`), so there is nothing for a recorder to inform. + // + // The snippet interpolates no caller data (values crossed via + // `setObjectJson`), so a failure here means the VM is broken — fatal, like + // the graft and the tx sugar. + if (origin.kind === 'hook' && ctx.input !== undefined) { + const inputGuard = vm.evalCode( + `globalThis.__inputWrites = []; + (function () { + var snapshot = __ctx.input; + if (!snapshot || typeof snapshot !== 'object') { + // Nothing to record through; the host falls back to its dump. + globalThis.__inputWrites = null; + return; + } + var note = function (k) { + if (globalThis.__inputWrites === null) return; + // Symbol keys are never payload fields; forward them unrecorded. + if (typeof k === 'symbol') return; + if (globalThis.__inputWrites.indexOf(k) < 0) globalThis.__inputWrites.push(k); + }; + // Every trap forwards through Reflect and records only what the + // forward REPORTED, so a refused write (the frozen, non-writable + // 'options' the graft defines) records nothing and still behaves + // exactly as it did untrapped. + var wrap = function (target) { + return new Proxy(target, { + set: function (t, k, v) { var ok = Reflect.set(t, k, v); if (ok) note(k); return ok; }, + defineProperty: function (t, k, d) { + var ok = Reflect.defineProperty(t, k, d); if (ok) note(k); return ok; + }, + deleteProperty: function (t, k) { + var ok = Reflect.deleteProperty(t, k); if (ok) note(k); return ok; + }, + }); + }; + var current = wrap(snapshot); + // An accessor, not a plain assignment, for the same reason the + // record recorder uses one: replacing the input WHOLESALE + // ('ctx.input = { status: "done" }') would otherwise swap the proxy + // out and leave every later write unrecorded. The replacement's own + // keys ARE the write the author is making, so they are noted; keys + // the replacement drops are carried by the host's + // absence-from-dump deletion leg, unchanged. + Object.defineProperty(__ctx, 'input', { + configurable: true, + enumerable: true, + get: function () { return current; }, + set: function (v) { + if (v && typeof v === 'object') { + Object.keys(v).forEach(note); + current = wrap(v); + } else { + // Not an object: there is no key set to speak of and the dump + // will not be one either. Say so rather than guess. + globalThis.__inputWrites = null; + current = v; + } + }, + }); + })();`, + ); + if (inputGuard.error) { + const msg = vm.dump(inputGuard.error); + inputGuard.error.dispose(); + throw new SandboxError(`failed to install the ctx.input write recorder: ${formatErr(msg)}`); + } + inputGuard.value.dispose(); + } + // `ctx.record` is a READ-ONLY snapshot: the action path returns the script's // value and never writes the record back, so `ctx.record.x = …` is discarded // — for a declared field exactly as much as for an unknown one (#4345). The @@ -1346,6 +1466,38 @@ function readRecordWritesJson(vm: QuickJSContext): string[] | undefined { } } +/** + * [#14758] After the script has settled, dump the keys the write-recorder proxy + * saw on `ctx.input` — the keys the BODY assigned, defined or deleted, as + * opposed to every key `readCtxInputJson` can see. + * + * The escape branch is the INVERSE of {@link readRecordWritesJson}'s, and that + * is the whole point of having two readers: + * + * - `[]` is a real answer here — "the recorder was armed and the body wrote + * nothing" — and the host narrows the carry-back to nothing on it; + * - `undefined` means the recorder cannot speak (never installed, replaced + * with a non-object, or unreadable), and the host falls back to the full + * `Object.assign` it did before this card. Narrowing on a key set that is + * not trustworthy would silently drop a write the body really made. + */ +function readInputWritesJson(vm: QuickJSContext): string[] | undefined { + try { + const r = vm.evalCode(`JSON.stringify(globalThis.__inputWrites || null)`); + if (r.error) { + r.error.dispose(); + return undefined; + } + const s = vm.dump(r.value); + r.value.dispose(); + if (typeof s !== 'string' || s === 'null') return undefined; + const parsed = safeJsonParse(s); + return Array.isArray(parsed) ? parsed.filter((k): k is string => typeof k === 'string') : undefined; + } catch { + return undefined; + } +} + function safeJsonParse(s: string | undefined): unknown { if (s === undefined || s === '') return undefined; try { diff --git a/packages/runtime/src/sandbox/script-runner.ts b/packages/runtime/src/sandbox/script-runner.ts index e0fc0086b5..c6ac5922ce 100644 --- a/packages/runtime/src/sandbox/script-runner.ts +++ b/packages/runtime/src/sandbox/script-runner.ts @@ -417,6 +417,34 @@ export interface ScriptResult { * `undefined` if the dump failed or the script context did not expose `input`. */ mutatedInput?: Record; + /** + * [#14758] Hook path only: the keys of {@link mutatedInput} the BODY actually + * assigned, defined or deleted — as opposed to every key the dump can see. + * + * `mutatedInput` alone cannot answer that question: it is the whole + * post-run `ctx.input`, so a body that touched nothing produces a dump + * identical to one that rewrote every field. The host write-back re-asserts + * whatever it is given onto the engine's flat-input proxy, and every one of + * those assignments is recorded by the #14088 hook-write provenance + * recorder — which is what #14099's per-row divergence refusal reads. Handing + * it the whole dump made that refusal blind on the shipped hook-body path, + * and blind in a way that depended on the driver's row order. + * + * ⚠️ `[]` and `undefined` are DIFFERENT answers, and a consumer that fuses + * them reintroduces the defect from the other side: + * + * - `[]` — the recorder was armed and the body wrote nothing. Carry back + * nothing. + * - `undefined` — this runner cannot say (no recorder installed, the runner + * predates this field, the read failed). Carry back the whole dump, which + * is the pre-#14758 behaviour: narrowing on a key set that cannot speak + * would silently drop a write the body really made. + * + * Keys reachable only THROUGH a value on `ctx.input` — `ctx.input.meta.x = 1` + * — never touch a trap and so are not listed here; `applyMutationsToInput` + * covers them from the dump instead (see its docblock). + */ + mutatedInputKeys?: string[]; /** * Keys the script wrote on `ctx.record`, in first-write order (#4345). * From ae0b1d702bafa9116910782235a59cce43be01ac Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 03:09:17 +0000 Subject: [PATCH 2/2] test(runtime): soft-assert the refusal envelope so the pin also reports 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 Claude-Session: https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza --- .../hook-input-writeback-key-set.integration.test.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/runtime/src/sandbox/hook-input-writeback-key-set.integration.test.ts b/packages/runtime/src/sandbox/hook-input-writeback-key-set.integration.test.ts index c2dab3379d..fc8bbdea5c 100644 --- a/packages/runtime/src/sandbox/hook-input-writeback-key-set.integration.test.ts +++ b/packages/runtime/src/sandbox/hook-input-writeback-key-set.integration.test.ts @@ -204,10 +204,14 @@ describe('#14758 — the sandbox write-back carries the keys the body wrote', () expect(seen.map((o) => o.index)).toEqual([0, 1]); // ADR-0112 envelope: code AND status, plus the key that diverged. - expect(err).toBeDefined(); - expect(err?.code).toBe('MULTI_UPDATE_HOOK_KEY_DIVERGENCE'); - expect(err?.status).toBe(400); - expect(err?.keys).toEqual(['completed_at']); + // `expect.soft` so a run where the refusal DOES NOT fire still reaches + // the row-state assertions below and reports the corruption that lands, + // instead of stopping at the missing envelope. That is the whole finding + // of this card, and a pin that hides half of it teaches half of it. + expect.soft(err).toBeDefined(); + expect.soft(err?.code).toBe('MULTI_UPDATE_HOOK_KEY_DIVERGENCE'); + expect.soft(err?.status).toBe(400); + expect.soft(err?.keys).toEqual(['completed_at']); // #14099's corruption: the already-done row's stamp must not move — and // the refusal lands BEFORE any write, so neither row changed at all.