diff --git a/.changeset/action-record-writes-runtime-report.md b/.changeset/action-record-writes-runtime-report.md new file mode 100644 index 0000000000..8a94796d72 --- /dev/null +++ b/.changeset/action-record-writes-runtime-report.md @@ -0,0 +1,42 @@ +--- +"@objectstack/runtime": minor +--- + +feat(runtime): the sandbox reports an action body's discarded `ctx.record` writes at invocation time (#4345) + +#4362 closed the author-time half of #4345: `action-record-write-discarded` +warns when a body assigns to `ctx.record` and the snapshot is provably dead. +This is the run-time half, and it exists because a parse cannot reach three +things a running action can: + +- **computed keys and aliases** — `ctx.record[k] = v`, `const r = ctx.record; + r.x = 1`, which the lint deliberately skips rather than guess at; +- **a wholesale replacement** — `ctx.record = {…}`; +- **bodies no lint ever sees** — metadata authored through Studio or the API + never passes through `os validate` / `os lint` / `os compile`. + +The sandbox installs a `set`/`deleteProperty`/`defineProperty` proxy over the +snapshot, behind an accessor so a wholesale replacement cannot swap the recorder +out, and surfaces the touched keys as `ScriptResult.droppedRecordWrites`. +`actionBodyRunnerFactory` logs a warning naming the discarded fields and the +`ctx.api.object(...).update(...)` remedy. Writes still work *inside* the VM, so +a body using the snapshot as scratch keeps its reads coherent — only the silence +is removed. + +**Only dead writes are reported**, on the same reading #4362 uses: a snapshot +that leaves the body as a value may have carried the write with it, so + +```js +ctx.record.stage = 'won'; +await ctx.api.object('crm_deal').update(ctx.record); // lands — stays quiet +``` + +is not reported, while a plain property read does not rescue a write (the +`ctx.recordId || (ctx.record && ctx.record.id)` guard idiom real action bodies +are written with still reports). An `ownKeys` after a write marks the escape. +A wrong "discarded" asserts something false about the stored record, which is +worse than a miss. + +Hooks carry no `record`, so they install no proxy and pay nothing. `ctx.record` +remains read-only; whether the runtime should instead refuse or honour the write +is still open — reporting a discard prejudges neither answer. diff --git a/content/docs/ui/actions.mdx b/content/docs/ui/actions.mdx index 66a88d1b58..d81ca55ac8 100644 --- a/content/docs/ui/actions.mdx +++ b/content/docs/ui/actions.mdx @@ -99,6 +99,40 @@ export const MarkDoneAction = defineAction({ Body-carrying actions are **registered automatically** at boot. + + **`ctx.record` is read-only — persist through `ctx.api`.** + + `ctx.record` is the record the dispatcher pre-fetched before the action ran: a + snapshot the runtime never writes back. Assigning to it changes a copy that + dies with the sandbox, and the action still returns success: + + ```js + ctx.record.done = true; // ❌ discarded — even though `done` is a declared field + return { ok: true }; // the action reports success, the record is unchanged + + await ctx.api.object('showcase_task').update({ id: ctx.recordId, done: true }); // ✅ persists + ``` + + This holds for **declared** fields too — it is not a spelling problem, so no + did-you-mean will appear. Do not reason from `ctx.input`, which *is* written + back in a [hook body](/docs/automation/hook-bodies); an action's output is its + return value and its write channel is `ctx.api` (declare + `capabilities: ['api.write']`). + + You will be told rather than left guessing. `os validate` / `os lint` / + `os compile` raise `action-record-write-discarded`, and the sandbox logs the + discarded fields at invocation time — the latter also catching computed keys, + aliases and bodies authored in Studio, which no lint ever inspects. + + Both report only writes that reach **nothing**. Building a payload on the + snapshot and then persisting it is a normal, live pattern and stays quiet: + + ```js + ctx.record.done = true; + await ctx.api.object('showcase_task').update(ctx.record); // ✅ lands — not reported + ``` + + ### Path B: a registered handler (full TypeScript) For logic that belongs in real source files, point `target` at a handler name diff --git a/packages/runtime/src/sandbox/body-runner.test.ts b/packages/runtime/src/sandbox/body-runner.test.ts index 38b8b812f0..a163e5461f 100644 --- a/packages/runtime/src/sandbox/body-runner.test.ts +++ b/packages/runtime/src/sandbox/body-runner.test.ts @@ -201,4 +201,87 @@ describe('actionBodyRunnerFactory', () => { const out = await fn!({ params: {} }); expect(out).toEqual({ count: 3 }); }); + + // The hook path writes `ctx.input` back; the action path has no `ctx.record` + // counterpart, so a body that assigns to `ctx.record` gets a green action and + // an unchanged record. That stays true — an action's write channel is + // `ctx.api` — but it is no longer silent (#4345). + describe('discarded ctx.record writes are reported (#4345)', () => { + const warnsFor = async (source: string, record?: Record) => { + const warns: Array<{ msg: string; meta: any }> = []; + const factory = actionBodyRunnerFactory(runner, { + ql: {}, + appId: 'crm', + logger: { warn: (msg: string, meta: any) => warns.push({ msg, meta }) }, + }); + const fn = factory({ + name: 'close_deal', + object: 'crm_deal', + body: { language: 'js', source, capabilities: [] }, + }); + const value = await fn!({ record, recordId: record?.id }); + return { warns, value }; + }; + + it('warns naming the discarded fields and the ctx.api remedy', async () => { + const { warns, value } = await warnsFor("ctx.record.stage = 'won'; return { ok: true };", { + id: 'deal_1', + stage: 'negotiation', + }); + // The action still reports success — the warning is the only signal the + // author's intended write did not happen. + expect(value).toEqual({ ok: true }); + expect(warns).toHaveLength(1); + expect(warns[0].msg).toContain('read-only'); + expect(warns[0].msg).toContain("ctx.api.object('crm_deal').update("); + expect(warns[0].meta.fields).toEqual(['stage']); + expect(warns[0].meta.action).toBe('close_deal'); + }); + + it('warns for a DECLARED field exactly as for an unknown one — the whole point of #4345', async () => { + // The runner never consults the object's fields: `stage` (declared on the + // object in the issue's repro) and `stgae` (a typo) are dropped alike, so + // both must warn. A rule that fired only on the unknown one would imply + // the declared one landed. + const declared = await warnsFor("ctx.record.stage = 'won'; return null;", { id: 'd', stage: 'x' }); + const typo = await warnsFor("ctx.record.stgae = 'won'; return null;", { id: 'd', stage: 'x' }); + expect(declared.warns).toHaveLength(1); + expect(typo.warns).toHaveLength(1); + }); + + it('stays quiet when the body only reads the record', async () => { + const { warns, value } = await warnsFor('return { stage: ctx.record.stage };', { + id: 'deal_1', + stage: 'negotiation', + }); + expect(value).toEqual({ stage: 'negotiation' }); + expect(warns).toEqual([]); + }); + + it('stays quiet for an action with no pre-fetched record', async () => { + const { warns } = await warnsFor('return { ok: true };'); + expect(warns).toEqual([]); + }); + + it('stays quiet when the body persists correctly through ctx.api', async () => { + const updates: unknown[] = []; + const factory = actionBodyRunnerFactory(runner, { + ql: { object: () => ({ update: async (d: unknown) => { updates.push(d); return d; } }) }, + appId: 'crm', + logger: { warn: () => { throw new Error('the documented remedy must not warn'); } }, + }); + const fn = factory({ + name: 'close_deal', + object: 'crm_deal', + body: { + language: 'js', + source: + "await ctx.api.object('crm_deal').update({ id: ctx.recordId, stage: 'won' }); return { ok: true };", + capabilities: ['api.write'], + }, + }); + expect(await fn!({ record: { id: 'deal_1' }, recordId: 'deal_1' })).toEqual({ ok: true }); + expect(updates).toEqual([{ id: 'deal_1', stage: 'won' }]); + }); + }); }); diff --git a/packages/runtime/src/sandbox/body-runner.ts b/packages/runtime/src/sandbox/body-runner.ts index 2538b1f02a..d47d40ff90 100644 --- a/packages/runtime/src/sandbox/body-runner.ts +++ b/packages/runtime/src/sandbox/body-runner.ts @@ -30,6 +30,13 @@ * Writes go through `Object.assign`, which means the host engine's * flat-record Proxy (installed by `wrapDeclarativeHook`) sees them * via its set trap. + * + * The ACTION path deliberately has no step 4: its output is the script's + * return value, and its write channel is `ctx.api.object(...)`. In particular + * `ctx.record` is a read-only pre-fetched snapshot — writes to it are + * discarded whether or not the field is declared (#4345). That stays true; + * what changed is that the discard is now REPORTED + * ({@link warnDiscardedRecordWrites}) instead of silent. */ import type { Hook } from '@objectstack/spec/data'; @@ -131,6 +138,7 @@ export function actionBodyRunnerFactory( // configurable action default (5000ms) apply. timeoutMs: (body as any).timeoutMs ?? action.timeoutMs, }); + warnDiscardedRecordWrites(result, action.name, action.object, opts); return result.value; } catch (err: any) { opts.logger?.error?.('[BodyRunner] sandboxed action threw', err, { @@ -143,6 +151,41 @@ export function actionBodyRunnerFactory( }; } +/** + * Report `ctx.record` writes the action path discards (#4345). + * + * The hook path writes `ctx.input` back to the engine via + * {@link applyMutationsToInput}; the action path has no counterpart for + * `ctx.record`, by design — an action's output is its return value, and its + * write channel is `ctx.api`. What was wrong was not the design but the + * SILENCE: a body assigning to `ctx.record` got a successful action and an + * unchanged record, with no diagnostic anywhere, and the field being correctly + * declared changed nothing (the #4001 false-completion shape). + * + * Advisory, not fatal — the same reason the author-time lint warns rather than + * gates: a body may legitimately use the snapshot as local scratch + * (`ctx.record.total = a + b; return { total: ctx.record.total }`), which is + * indistinguishable here from an intended persist. The message states what is + * true of both — the writes did not leave the VM — and names the remedy. + * Ratcheting to a throw is a decision for field data, not a default. + */ +function warnDiscardedRecordWrites( + result: ScriptResult, + actionName: string, + object: string | undefined, + opts: FactoryOptions, +): void { + const fields = result.droppedRecordWrites; + if (!fields || fields.length === 0) return; + opts.logger?.warn?.( + `[BodyRunner] action '${actionName}' wrote ${fields.length} field(s) to ctx.record, which is a read-only ` + + `pre-fetched snapshot — the writes never left the sandbox and the stored record is unchanged. ` + + `To persist, call ctx.api.object('${object ?? ''}').update({ id: ctx.recordId, … }) ` + + `(needs the 'api.write' capability). See #4345.`, + { appId: opts.appId, action: actionName, object, fields }, + ); +} + function applyMutationsToInput(engineCtx: any, result: ScriptResult): void { const target = engineCtx?.input; if (!target || typeof target !== 'object') return; @@ -254,6 +297,9 @@ function buildActionSandboxContext(actionCtx: any, ql: any): ScriptContext { session: actionCtx?.session, object: typeof actionCtx?.object === 'string' ? actionCtx.object : undefined, recordId, + // A snapshot by construction, and read-only by contract (#4345): nothing + // downstream writes it back. `warnDiscardedRecordWrites` reports the writes + // a body makes to it rather than letting them vanish. record: unwrapProxyToPlain(actionCtx?.record), api: buildSandboxApi(actionCtx, ql, 'action body'), log: actionCtx?.logger, diff --git a/packages/runtime/src/sandbox/quickjs-runner.test.ts b/packages/runtime/src/sandbox/quickjs-runner.test.ts index 9e6f9516ea..d36799c2a3 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.test.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.test.ts @@ -220,6 +220,194 @@ describe('QuickJSScriptRunner — L2 action script', () => { }); }); +// `ctx.record` is a read-only pre-fetched snapshot: nothing writes it back, so +// every write to it is discarded — for a DECLARED field exactly as much as for +// an unknown one, which is what made #4345 a false-completion trap rather than +// a typo. The engine cannot make the write land (that is the action's `ctx.api` +// channel), but it can refuse to lose it in silence. +describe('QuickJSScriptRunner — ctx.record writes are recorded, not silently dropped (#4345)', () => { + const record = { id: 'deal_1', stage: 'negotiation', amount: 100 }; + + it('reports no dropped writes when the context carries no record', async () => { + const r = await runner.runScript( + { language: 'js', source: 'return { ok: true };', capabilities: [] }, + ctx(), + actionOpts, + ); + // Distinct from `[]`: no snapshot existed, so no recorder was installed. + expect(r.droppedRecordWrites).toBeUndefined(); + }); + + it('reports an empty write set when the record is only read', async () => { + const r = await runner.runScript( + { language: 'js', source: 'return { stage: ctx.record.stage };', capabilities: [] }, + ctx({ record: { ...record } }), + actionOpts, + ); + expect(r.value).toEqual({ stage: 'negotiation' }); + expect(r.droppedRecordWrites).toEqual([]); + }); + + it('records a plain property write — and the host snapshot is untouched', async () => { + const host = { ...record }; + const r = await runner.runScript( + { language: 'js', source: "ctx.record.stage = 'won'; return { ok: true };", capabilities: [] }, + ctx({ record: host }), + actionOpts, + ); + // The action still "succeeds" — that is the trap, and the report is the fix. + expect(r.value).toEqual({ ok: true }); + expect(r.droppedRecordWrites).toEqual(['stage']); + expect(host.stage).toBe('negotiation'); + }); + + it('forwards the write inside the VM so a body using the snapshot as scratch stays coherent', async () => { + const r = await runner.runScript( + { + language: 'js', + source: 'ctx.record.amount = ctx.record.amount * 2; return { amount: ctx.record.amount };', + capabilities: [], + }, + ctx({ record: { ...record } }), + actionOpts, + ); + expect(r.value).toEqual({ amount: 200 }); + expect(r.droppedRecordWrites).toEqual(['amount']); + }); + + it('sees what static analysis cannot: computed keys, Object.assign, aliases, delete', async () => { + const r = await runner.runScript( + { + language: 'js', + source: + "var k = 'st' + 'age'; ctx.record[k] = 'won';" + + "Object.assign(ctx.record, { amount: 1 });" + + 'var alias = ctx.record; alias.owner = "u1";' + + 'delete ctx.record.id;' + + 'return { ok: true };', + capabilities: [], + }, + ctx({ record: { ...record } }), + actionOpts, + ); + // First-write order, deduplicated. + expect(r.droppedRecordWrites).toEqual(['stage', 'amount', 'owner', 'id']); + }); + + it('catches a WHOLESALE replacement, and keeps recording afterwards', async () => { + // `ctx.record = {…}` would swap a bare proxy out and silence every later + // write — the one shape where the recorder could go quiet exactly when the + // author was most confident they had persisted. The accessor reports the + // replacement's own keys, which is what the author believed they wrote. + const r = await runner.runScript( + { + language: 'js', + source: "ctx.record = { stage: 'won', amount: 9 }; ctx.record.owner = 'u1'; return { ok: true };", + capabilities: [], + }, + ctx({ record: { ...record } }), + actionOpts, + ); + expect(r.value).toEqual({ ok: true }); + expect(r.droppedRecordWrites).toEqual(['stage', 'amount', 'owner']); + }); + + it('reports a non-object replacement without inventing field names', async () => { + const r = await runner.runScript( + { language: 'js', source: 'ctx.record = null; return null;', capabilities: [] }, + ctx({ record: { ...record } }), + actionOpts, + ); + expect(r.droppedRecordWrites).toEqual(['(whole record replaced)']); + }); + + it('keeps the snapshot readable through the accessor', async () => { + const r = await runner.runScript( + { + language: 'js', + source: 'return { keys: Object.keys(ctx.record).sort(), stage: ctx.record.stage };', + capabilities: [], + }, + ctx({ record: { ...record } }), + actionOpts, + ); + expect(r.value).toEqual({ keys: ['amount', 'id', 'stage'], stage: 'negotiation' }); + expect(r.droppedRecordWrites).toEqual([]); + }); + + it('reports each field once however often it is rewritten', async () => { + const r = await runner.runScript( + { + language: 'js', + source: "ctx.record.stage = 'a'; ctx.record.stage = 'b'; ctx.record.stage = 'c'; return null;", + capabilities: [], + }, + ctx({ record: { ...record } }), + actionOpts, + ); + expect(r.droppedRecordWrites).toEqual(['stage']); + }); + + // A write is only DEAD if the snapshot never leaves the body as a value. + // These four all LAND, so reporting them would be a false statement about the + // stored record — worse than saying nothing. + describe('a write whose snapshot escapes is live, and stays unreported', () => { + const api = { object: () => ({ update: async (d: unknown) => d }) }; + const live = (source: string) => + runner.runScript( + { language: 'js', source, capabilities: ['api.write'] }, + ctx({ record: { ...record }, api }), + actionOpts, + ); + + it('handed to ctx.api as the payload — the canonical live shape', async () => { + const r = await live("ctx.record.stage = 'won'; await ctx.api.object('d').update(ctx.record); return null;"); + expect(r.droppedRecordWrites).toEqual([]); + }); + + it('copied out of the body', async () => { + const r = await live("ctx.record.stage = 'won'; return { patch: Object.assign({}, ctx.record) };"); + expect(r.droppedRecordWrites).toEqual([]); + }); + + it('returned whole', async () => { + const r = await live("ctx.record.stage = 'won'; return ctx.record;"); + expect(r.droppedRecordWrites).toEqual([]); + }); + + it('serialised', async () => { + const r = await live("ctx.record.stage = 'won'; return { n: JSON.stringify(ctx.record).length };"); + expect(r.droppedRecordWrites).toEqual([]); + }); + + it('but a property READ does not rescue the write', async () => { + // Reading `ctx.record.id` consumes a field, not the object — the + // assignment still goes nowhere. This is the distinction that keeps the + // signal alive on real bodies: the showcase's own guard idiom below is + // exactly this shape. + const r = await live( + "var id = ctx.recordId || (ctx.record && ctx.record.id); ctx.record.stage = 'won'; return { id: id };", + ); + expect(r.droppedRecordWrites).toEqual(['stage']); + }); + + it('and enumerating BEFORE any write leaves the recorder armed', async () => { + const r = await live("var keys = Object.keys(ctx.record); ctx.record.stage = 'won'; return { n: keys.length };"); + expect(r.droppedRecordWrites).toEqual(['stage']); + }); + }); + + it('leaves the hook path alone — a hook ctx carries no record, so nothing is recorded', async () => { + const r = await runner.runScript( + { language: 'js', source: 'ctx.input.total = 1; return null;', capabilities: [] }, + ctx({ input: {} }), + hookOpts, + ); + expect(r.droppedRecordWrites).toBeUndefined(); + expect(r.mutatedInput).toEqual({ total: 1 }); + }); +}); + describe('QuickJSScriptRunner — async host APIs', () => { it('awaits Promise return values from host APIs (asyncified)', async () => { const api = { object: () => ({ count: async () => 7 }) }; diff --git a/packages/runtime/src/sandbox/quickjs-runner.ts b/packages/runtime/src/sandbox/quickjs-runner.ts index c3cde088b9..ae4d228d89 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.ts @@ -393,7 +393,11 @@ 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); - return { value, mutatedInput, durationMs: Date.now() - start }; + // …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 }; } const budget = budgetError(pumps); @@ -640,6 +644,84 @@ export class QuickJSScriptRunner implements ScriptRunner { throw new SandboxError(`failed to install ctx.api.transaction: ${formatErr(msg)}`); } sugar.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 + // write still WORKS inside the VM (the trap forwards it, so a body using the + // snapshot as scratch keeps its reads coherent); what changes is that it is + // no longer SILENT. Recording it here rather than diffing a post-run dump is + // what makes the signal exact: the trap fires for computed keys, + // `Object.assign`, and aliases (`const r = ctx.record; r.x = 1`) — the cases + // both a dump-diff and the author-time lint miss — and costs nothing on the + // hook path, which carries no `record` and so installs no proxy. + if (ctx.record === undefined) return; + const recordGuard = vm.evalCode( + `globalThis.__recordWrites = []; globalThis.__recordEscaped = false; + (function () { + var snapshot = __ctx.record; + if (!snapshot || typeof snapshot !== 'object') return; + var note = function (k) { + // Symbol keys are never record fields; forward them unrecorded. + if (typeof k === 'symbol') return; + if (globalThis.__recordWrites.indexOf(k) < 0) globalThis.__recordWrites.push(k); + }; + var wrap = function (target) { + return new Proxy(target, { + set: function (t, k, v) { note(k); t[k] = v; return true; }, + deleteProperty: function (t, k) { note(k); delete t[k]; return true; }, + defineProperty: function (t, k, d) { note(k); Object.defineProperty(t, k, d); return true; }, + // Escape detection. A write is only DEAD if the snapshot never + // leaves the body as a value — this is live, and reporting it + // would be a false statement, not just noise: + // ctx.record.stage = 'won'; + // await ctx.api.object('d').update(ctx.record); // it lands + // Consuming the object whole (marshalling it to a host call, + // JSON.stringify, spread, Object.keys, returning it) enumerates + // its keys; a plain property READ does not. So an ownKeys AFTER a + // write means the written value may have gone somewhere, and the + // recorder goes quiet. Same direction the author-time rule takes: + // treat ambiguity as live, because a wrong "discarded" is worse + // than a missed one. + ownKeys: function (t) { + if (globalThis.__recordWrites.length > 0) globalThis.__recordEscaped = true; + return Reflect.ownKeys(t); + }, + }); + }; + var current = wrap(snapshot); + // An accessor, not a plain assignment, so that replacing the snapshot + // WHOLESALE (\`ctx.record = { stage: 'won' }\`) is caught too. A bare + // proxy would be swapped out by that write and every later field write + // would go unrecorded — the one shape where the recorder could have + // gone quiet exactly when the author was most sure they had persisted. + Object.defineProperty(__ctx, 'record', { + configurable: true, + enumerable: true, + get: function () { return current; }, + set: function (v) { + if (v && typeof v === 'object') { + // Report the replacement's own keys: that is what the author + // believed they were writing. + Object.keys(v).forEach(note); + current = wrap(v); + } else { + note('(whole record replaced)'); + current = v; + } + }, + }); + })();`, + ); + if (recordGuard.error) { + const msg = vm.dump(recordGuard.error); + recordGuard.error.dispose(); + // Fatal, like the sugar above: the snippet interpolates no caller data, so + // a failure here means the VM is broken, not that some record shape defeated + // it. Falling back would silently restore the very blind spot this closes. + throw new SandboxError(`failed to install the ctx.record write recorder: ${formatErr(msg)}`); + } + recordGuard.value.dispose(); } } @@ -887,6 +969,34 @@ function readCtxInputJson(vm: QuickJSContext): Record | undefin } } +/** + * After the script has settled, dump the keys the write-recorder proxy saw on + * `ctx.record` (#4345). Those writes are discarded — the action path has no + * record write-back — so the host reports them rather than staying silent. + * + * Returns `undefined` if the read fails or the recorder was never installed; + * `[]` when the snapshot was present and untouched, which is the common case + * and the one that must stay quiet. + */ +function readRecordWritesJson(vm: QuickJSContext): string[] | undefined { + try { + const r = vm.evalCode( + `JSON.stringify(globalThis.__recordEscaped ? [] : (globalThis.__recordWrites || 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 fa59dd1e1f..112e8eab16 100644 --- a/packages/runtime/src/sandbox/script-runner.ts +++ b/packages/runtime/src/sandbox/script-runner.ts @@ -94,9 +94,14 @@ export interface ScriptContext { * no action-side counterpart. So `ctx.record.x = …` inside a body mutates a * copy that is then discarded, for DECLARED and undeclared fields alike; to * persist, write through `ctx.api.object(...)`. `@objectstack/lint` warns on - * a provably dead assignment (`action-record-write-discarded`, #4345); the - * open question of whether the runtime should instead refuse or honour the - * write is tracked there. + * a provably dead assignment (`action-record-write-discarded`, #4345). + * + * The runtime reports the same thing at INVOCATION time — see + * {@link ScriptResult.droppedRecordWrites}. That covers what a parse cannot: + * computed keys, aliases, and bodies authored through Studio or the API, + * which no lint ever inspects. It only reports; whether the runtime should + * instead refuse or honour the write remains open, and logging a discard + * prejudges neither answer. */ record?: unknown; /** Engine-side `result` (only set for after* hooks). */ @@ -133,6 +138,36 @@ export interface ScriptResult { * `undefined` if the dump failed or the script context did not expose `input`. */ mutatedInput?: Record; + /** + * Keys the script wrote on `ctx.record`, in first-write order (#4345). + * + * `ctx.record` is a read-only snapshot (see {@link ScriptContext.record}), so + * every one of these writes is DISCARDED. The engine records them rather than + * dropping them in silence: an author who believed the write persisted gets a + * host-side diagnostic naming the fields and the remedy, instead of a green + * action and an unchanged record — the #4001 "silent no-op manufactures false + * completion" shape. + * + * Recorded by a `set`/`deleteProperty`/`defineProperty` proxy installed over + * the snapshot inside the VM — behind an accessor, so a wholesale + * `ctx.record = {…}` is caught as well and does not swap the recorder out. + * Being a run-time trap rather than a parse, it sees what static analysis + * cannot: computed keys, `Object.assign`, aliased references + * (`const r = ctx.record; r.x = 1`), and bodies authored through Studio or + * the API, which no lint ever inspects. + * + * ONLY dead writes are reported. A snapshot that leaves the body as a value + * — `update(ctx.record)`, a spread, a return, a `JSON.stringify` — may well + * have carried the write somewhere, so the recorder goes quiet (an `ownKeys` + * after a write marks the escape). A plain property read does not rescue it. + * Same direction as the author-time rule: a wrong "discarded" asserts + * something false about the stored record, which is worse than a miss. + * + * `undefined` when the context carried no `record` (every hook, and actions + * with no pre-fetched record) or when the read-back failed; empty when a + * record was present and untouched. + */ + droppedRecordWrites?: string[]; } export interface ScriptRunOptions { diff --git a/packages/spec/src/ui/action.zod.ts b/packages/spec/src/ui/action.zod.ts index 5234f33d1b..9d67da3423 100644 --- a/packages/spec/src/ui/action.zod.ts +++ b/packages/spec/src/ui/action.zod.ts @@ -482,6 +482,10 @@ const actionObject = () => z.object({ * provably dead `ctx.record. = …` warns as discarded (#4345). * Advisory only, and blind to everything statically unknowable; see * `ScriptBodySchema` for the scope. + * + * The sandbox reports the discarded `ctx.record` writes at INVOCATION time as + * well, covering the computed keys, aliases and Studio/API-authored bodies a + * parse cannot reach. */ body: HookBodySchema.optional().describe('Action body — expression (L1) or sandboxed JS (L2). Only used when type is `script`.'),