From 34e836ccb058506e749a757f504d3aa21213a531 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:27:32 +0800 Subject: [PATCH 1/2] feat(runtime,lint,spec): an action body's ctx.record writes are reported, not silently discarded (#4345) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ctx.record` in an L2 action body is a pre-fetched snapshot that nothing writes back, so every assignment to it was dropped — a correctly spelled, fully declared field exactly like an unknown one — while the action returned success. No diagnostic anywhere: the #4001 "silent no-op manufactures false completion" shape, one layer below the unknown-column drop #4271 covers. The snapshot stays read-only. An action's output is its return value and its write channel is `ctx.api`; making the snapshot writable would raise questions this bug does not answer (write back to what, under whose permissions, what `requiresRecord: false` means). What was wrong was the silence. Runtime: the sandbox installs a set/deleteProperty/defineProperty proxy over the snapshot, behind an accessor so a wholesale `ctx.record = {…}` cannot swap the recorder out, and reports the touched keys as `ScriptResult.droppedRecordWrites`. `actionBodyRunnerFactory` warns 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. As a run-time trap it sees the computed keys, `Object.assign` and aliases static analysis cannot, and it covers metadata authored through Studio or the API, which no lint inspects. Hooks carry no `record` and install no proxy. Lint: new advisory rule `action-body-record-write-discarded` (`validateActionRecordWrites`) in REFERENCE_INTEGRITY_RULES, so `os validate`, `os lint` and `os compile` all report it. It never consults declared fields and offers no did-you-mean — the field name is not the bug — and it dedupes defineStack's merged action copies by value, not identity, so one authored action reports once. Advisory because a body may use the snapshot as local scratch, which no analysis short of data-flow tells from an intended persist. Docs: `ScriptContext.record`, `ActionSchema.body`, `ScriptBodySchema`, `content/docs/ui/actions.mdx` and `content/docs/automation/hook-bodies.mdx` now state the read-only semantics and name the `ctx.input` analogy as the trap it is. Verified on the showcase app: zero findings before, exactly one after planting the issue's repro — naming both declared fields and proposing the very `ctx.api` call it replaced. Co-Authored-By: Claude Opus 5 --- .changeset/action-record-writes-discarded.md | 65 +++ content/docs/automation/hook-bodies.mdx | 16 +- content/docs/ui/actions.mdx | 26 ++ packages/lint/src/index.ts | 13 + packages/lint/src/lazy-deps.test.ts | 15 +- .../src/reference-integrity-suite.test.ts | 18 +- .../lint/src/reference-integrity-suite.ts | 25 ++ .../src/validate-action-record-writes.test.ts | 240 ++++++++++++ .../lint/src/validate-action-record-writes.ts | 370 ++++++++++++++++++ .../runtime/src/sandbox/body-runner.test.ts | 83 ++++ packages/runtime/src/sandbox/body-runner.ts | 46 +++ .../src/sandbox/quickjs-runner.test.ts | 139 +++++++ .../runtime/src/sandbox/quickjs-runner.ts | 94 ++++- packages/runtime/src/sandbox/script-runner.ts | 36 ++ packages/spec/src/data/hook-body.zod.ts | 8 + packages/spec/src/ui/action.zod.ts | 17 + 16 files changed, 1205 insertions(+), 6 deletions(-) create mode 100644 .changeset/action-record-writes-discarded.md create mode 100644 packages/lint/src/validate-action-record-writes.test.ts create mode 100644 packages/lint/src/validate-action-record-writes.ts diff --git a/.changeset/action-record-writes-discarded.md b/.changeset/action-record-writes-discarded.md new file mode 100644 index 0000000000..4dc08bd12f --- /dev/null +++ b/.changeset/action-record-writes-discarded.md @@ -0,0 +1,65 @@ +--- +"@objectstack/runtime": minor +"@objectstack/lint": minor +"@objectstack/spec": patch +--- + +feat(runtime,lint,spec): an action body's `ctx.record` writes are reported, not silently discarded (#4345) + +An L2 action body that assigns to `ctx.record` had every write dropped, with no +diagnostic anywhere: + +```ts +body: { language: 'js', source: "ctx.record.stage = 'won'; return { ok: true };" } +``` + +The action returned `{ ok: true }` and the record was unchanged. `ctx.record` is +a pre-fetched snapshot (`buildActionSandboxContext` hands the body a plain copy) +and `boundActionHandler` returns only the script's value — the hook path's +`applyMutationsToInput` write-back has no action counterpart. + +**`ctx.record` stays read-only.** An action's output is its return value and its +write channel is `ctx.api`; that model is coherent, and making the snapshot +writable would raise questions this bug does not answer (write back to what, +under whose permissions, and what `requiresRecord: false` means). What was wrong +was the SILENCE — and unlike #4271's unknown-column drop, this one swallowed +**correctly spelled, fully declared** fields too, so a rule that fired only on +unknown fields would have implied the declared ones persisted. + +Three layers now say so: + +- **Runtime (new).** The sandbox installs a `set`/`deleteProperty`/ + `defineProperty` proxy over the snapshot — behind an accessor, so a wholesale + `ctx.record = {…}` is caught too instead of swapping the recorder out — and + surfaces the keys a body touched as `ScriptResult.droppedRecordWrites`; + `actionBodyRunnerFactory` logs a warning naming the discarded fields and the + `ctx.api.object(...).update(...)` remedy. The write still works *inside* the + VM, so a body using the snapshot as scratch keeps its reads coherent — only + the silence is removed. Being a runtime trap rather than a static check, it + sees computed keys, `Object.assign` and aliases + (`const r = ctx.record; r.x = 1`), and it covers metadata authored through + Studio or the API, which no lint ever inspects. Hooks carry no `record`, so + they install no proxy and pay nothing. +- **Authoring (new rule, advisory).** `action-body-record-write-discarded` + (`validateActionRecordWrites`, in `REFERENCE_INTEGRITY_RULES`) warns on the + literal patterns in the exported `ACTION_RECORD_WRITE_PATTERNS` ledger, so + `os validate` / `os lint` / `os compile` all report it. It never consults the + object's declared fields and offers no did-you-mean: the field name is not the + bug. It stays advisory because a body may legitimately use the snapshot as + local scratch, which no analysis short of data-flow can tell from an intended + persist — gating would block correct builds. +- **Docs.** `ScriptContext.record`, `ActionSchema.body`, `ScriptBodySchema`, + `content/docs/ui/actions.mdx` and `content/docs/automation/hook-bodies.mdx` + now state the read-only semantics and the `ctx.api` remedy, and name the + `ctx.input` analogy as the trap it is. + +No behavioral change to any body that was already correct: verified against the +showcase app, which reports zero findings before and exactly one — naming both +declared fields and proposing the exact call it replaced — after planting the +issue's repro. + +New exports from `@objectstack/lint`: `validateActionRecordWrites`, +`extractActionRecordWrites`, `ACTION_RECORD_WRITE_PATTERNS`, +`ACTION_BODY_RECORD_WRITE_DISCARDED`, plus the `ActionRecordWriteFinding`, +`ActionRecordWriteSeverity`, `ActionRecordWritePattern` and +`ExtractedActionRecordWrite` types. diff --git a/content/docs/automation/hook-bodies.mdx b/content/docs/automation/hook-bodies.mdx index 3902b7dc20..c212b79424 100644 --- a/content/docs/automation/hook-bodies.mdx +++ b/content/docs/automation/hook-bodies.mdx @@ -94,8 +94,10 @@ The script sees only what the surrounding `ctx` object exposes: | Field | Description | Capability required | |---|---|---| -| `ctx.input` | Mutable record being inserted/updated/etc. | none | +| `ctx.input` | Hook: the mutable record being inserted/updated/etc. — assignments ARE written back to the engine. Action: the validated params bag. | none | | `ctx.previous` | Pre-update record (update events only). | none | +| `ctx.recordId` | Action only: the record id from the invocation URL. | none | +| `ctx.record` | Action only: the record the dispatcher pre-fetched. **Read-only** — a snapshot that is never written back, so assigning to it is discarded even for a declared field ([#4345](https://github.com/objectstack-ai/objectstack/issues/4345)). Persist via `ctx.api.object(...).update(...)`. | none | | `ctx.user` / `ctx.session` | Identity context. | none | | `ctx.api.object(name).find\|count\|aggregate` | Cross-object reads, scoped to current tenant. | `api.read` | | `ctx.api.object(name).insert\|update\|delete` | Cross-object writes. | `api.write` | @@ -141,6 +143,18 @@ Because the write set carries no static checking: Hooks mutate `ctx.input`/`ctx.result`; actions return their output value explicitly. +That asymmetry is load-bearing, and it is the one place authors reliably guess +wrong. An action has **no** mutable context surface: `ctx.record` is a read-only +pre-fetched snapshot, so `ctx.record.stage = 'won'` mutates a copy that dies +with the sandbox while the action reports success — and a correctly spelled, +fully declared field is dropped exactly like a misspelled one. An action +persists through `ctx.api.object(name).update({ id: ctx.recordId, … })` and +communicates through its return value. Both ends now say so rather than +swallowing the mistake: `os validate` / `os lint` / `os compile` warn on a +literal `ctx.record. = …`, and the sandbox logs the discarded fields at +invocation time, including the computed keys and aliased references a static +check cannot see ([#4345](https://github.com/objectstack-ai/objectstack/issues/4345)). + ### Engine The sandbox engine is **`quickjs-emscripten`** — pure-WASM, runs on every JS host. We considered `isolated-vm` but its native dependency disqualifies edge targets. The choice is hidden behind the `ScriptRunner` interface in `packages/runtime/src/sandbox/`, so a node-only deployment can swap in a faster engine later without touching call sites. diff --git a/content/docs/ui/actions.mdx b/content/docs/ui/actions.mdx index 66a88d1b58..4b0105186a 100644 --- a/content/docs/ui/actions.mdx +++ b/content/docs/ui/actions.mdx @@ -99,6 +99,32 @@ 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` warn on a literal `ctx.record. = …`, and the sandbox logs + the discarded fields at invocation time — including the computed keys and + aliased references static checks cannot see. + + ### 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/lint/src/index.ts b/packages/lint/src/index.ts index b90a09d129..06b73be457 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -271,6 +271,19 @@ export type { ExtractedHookBodyWrite, } from './validate-hook-body-writes.js'; +export { + validateActionRecordWrites, + extractActionRecordWrites, + ACTION_RECORD_WRITE_PATTERNS, + ACTION_BODY_RECORD_WRITE_DISCARDED, +} from './validate-action-record-writes.js'; +export type { + ActionRecordWriteFinding, + ActionRecordWriteSeverity, + ActionRecordWritePattern, + ExtractedActionRecordWrite, +} from './validate-action-record-writes.js'; + // One entry point for the reference-resolution rules above (#3583 §5 D5). // Adding a rule to `REFERENCE_INTEGRITY_RULES` runs it on `validate`, `lint` // and `compile` at once — the CLI call sites do not change. diff --git a/packages/lint/src/lazy-deps.test.ts b/packages/lint/src/lazy-deps.test.ts index ad817b1dcb..72f4c859c2 100644 --- a/packages/lint/src/lazy-deps.test.ts +++ b/packages/lint/src/lazy-deps.test.ts @@ -76,6 +76,9 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { const jsHook = (source, language) => ({ objects: [{ name: 'a', fields: { amount: {} } }], hooks: [{ name: 'h', object: 'a', events: ['beforeInsert'], body: { language: language ?? 'js', source } }] }); mod.validateHookBodyWrites(jsHook('input.x > 0', 'expression')); if (loaded('typescript')) fail('the hook-body write gate on an L1-only stack must not load typescript'); + const recordAction = (source) => ({ objects: [{ name: 'a', fields: { stage: {} } }], actions: [{ name: 'act', objectName: 'a', type: 'script', body: { language: 'js', source } }] }); + mod.validateActionRecordWrites(recordAction('return { ok: true };')); + if (loaded('typescript')) fail('the action-record write gate must not load typescript for a body that never mentions ctx.record'); const syntax = mod.validateReactPages(${reactStack('function Page(){ return
oops; }')}); if (!loaded('sucrase')) fail('sucrase was not loaded by a react-page syntax validation'); if (loaded('typescript')) fail('the syntax gate must not load typescript'); @@ -83,6 +86,8 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { const hookWrites = mod.validateHookBodyWrites(jsHook('ctx.input.amout = 1;')); if (!loaded('typescript')) fail('typescript was not loaded by an L2 hook-body write validation'); if (!hookWrites.some((f) => f.rule === 'hook-body-write-unknown-field')) fail('hook-body write gate produced no finding'); + const recordWrites = mod.validateActionRecordWrites(recordAction("ctx.record.stage = 'won';")); + if (!recordWrites.some((f) => f.rule === 'action-body-record-write-discarded')) fail('action-record write gate produced no finding'); const props = mod.validateReactPageProps(${reactStack('function Page(){ return ; }')}); if (!loaded('typescript')) fail('typescript was not loaded by a react-page props validation'); if (!props.some((f) => f.rule === 'react-prop-missing-required')) fail('props gate produced no finding'); @@ -117,7 +122,8 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { it('loads each dep lazily in-process and the gates still work', async () => { const req = createRequire(import.meta.url); - const { validateReactPages, validateReactPageProps, validateHookBodyWrites } = await import('./index.js'); + const { validateReactPages, validateReactPageProps, validateHookBodyWrites, validateActionRecordWrites } = + await import('./index.js'); // Stacks without a react-source page never touch either dep. expect(validateReactPages({ pages: [{ name: 'p', kind: 'object' }] })).toEqual([]); @@ -129,6 +135,13 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { expect(validateHookBodyWrites({ hooks: [hook(undefined)] })).toEqual([]); expect(validateHookBodyWrites({ hooks: [hook({ language: 'expression', source: 'input.x > 0' })] })).toEqual([]); expect(validateHookBodyWrites({ hooks: [hook({ language: 'js', source: 'return 1;' })] })).toEqual([]); + // Same contract one rule over: the action-record gate's prefilter needs + // BOTH `ctx` and `record` in the source, so a target-bound action, an L1 + // body, and a JS body that never touches the snapshot all stay parser-free. + const action = (body: unknown) => ({ name: 'a', objectName: 'o', type: 'script', body }); + expect(validateActionRecordWrites({ actions: [action(undefined)] })).toEqual([]); + expect(validateActionRecordWrites({ actions: [action({ language: 'expression', source: 'ctx.record.x' })] })).toEqual([]); + expect(validateActionRecordWrites({ actions: [action({ language: 'js', source: 'return ctx.recordId;' })] })).toEqual([]); for (const dep of LAZY_DEPS) { expect(depLoaded(req.cache, dep), `${dep} loaded before any react-source or L2-body validation`).toBe(false); } diff --git a/packages/lint/src/reference-integrity-suite.test.ts b/packages/lint/src/reference-integrity-suite.test.ts index e1f79868a1..c121780039 100644 --- a/packages/lint/src/reference-integrity-suite.test.ts +++ b/packages/lint/src/reference-integrity-suite.test.ts @@ -27,6 +27,7 @@ describe('reference-integrity suite — membership', () => { 'validateAiToolReferences', 'validateAiAgentAuthoring', 'validateHookBodyWrites', + 'validateActionRecordWrites', ]); }); @@ -59,7 +60,17 @@ describe('reference-integrity suite — every member actually runs', () => { ], actions: [ // validateObjectReferences: a param pointing at an object nothing declares. - { name: 'assign', label: 'Assign', params: [{ name: 'owner', reference: 'user' }] }, + // validateActionRecordWrites: the L2 body assigns to `ctx.record`, a + // read-only snapshot — discarded even though `name` IS declared on + // crm_lead (#4345). One fixture, two rules. + { + name: 'assign', + label: 'Assign', + objectName: 'crm_lead', + type: 'script', + params: [{ name: 'owner', reference: 'user' }], + body: { language: 'js', source: "ctx.record.name = 'assigned';" }, + }, ], views: [ { @@ -171,6 +182,7 @@ describe('reference-integrity suite — every member actually runs', () => { expect(rules).toContain('ai-skill-tool-unresolved'); expect(rules).toContain('agent-authoring-withdrawn'); expect(rules).toContain('hook-body-write-unknown-field'); + expect(rules).toContain('action-body-record-write-discarded'); }); it('carries a gating flow-template finding through the suite (#3810)', () => { @@ -192,9 +204,9 @@ describe('reference-integrity suite — every member actually runs', () => { expect(typeof f.message).toBe('string'); expect(typeof f.hint).toBe('string'); } - // Object references run first, hook-body writes last. + // Object references run first, action-record writes last. expect(findings[0].rule).toBe('object-reference-unknown'); - expect(findings[findings.length - 1].rule).toBe('hook-body-write-unknown-field'); + expect(findings[findings.length - 1].rule).toBe('action-body-record-write-discarded'); }); it('returns nothing for an empty stack', () => { diff --git a/packages/lint/src/reference-integrity-suite.ts b/packages/lint/src/reference-integrity-suite.ts index 6f7d3557f9..67d6f52033 100644 --- a/packages/lint/src/reference-integrity-suite.ts +++ b/packages/lint/src/reference-integrity-suite.ts @@ -45,6 +45,26 @@ * styles, seed replay safety, seed state machines, seed/security posture) stay * out — they answer a different question and have their own call sites. * + * ## One deliberate non-member: `validateActionRecordWrites` + * + * It resolves nothing against the stack. `ctx.record. = …` in an action + * body is discarded because the action path has no record write-back at all, so + * the finding does not depend on whether the field is declared — checking that + * would in fact be WRONG, since warning only on the unknown half would imply + * the declared half persists (#4345). By the membership test above it is a + * shape rule and belongs at its own call sites. + * + * It is carried here anyway, as a deliberate widening rather than an oversight, + * because "its own call sites" is not a neutral alternative: it means three + * hand-wired commands, and the drift that produces is exactly what this module + * exists to end — `validateReadonlyFlowWrites`, the closest sibling by subject + * (a write that silently does not land), still demonstrates it, wired into + * `validate` and `compile` but not `lint`. The suite's real invariant is that a + * stack gets the SAME answer from every command; membership by question-shape + * is how that was described, not what it is for. A future rule may take the + * same exemption — but it must say so here, in these terms, rather than quietly + * reading the membership test loosely. + * * ## Known remaining asymmetry * * `os doctor` runs only `validateWidgetBindings` and is NOT converted here: it @@ -66,6 +86,7 @@ import { validateAiSurfaceAffinity } from './validate-ai-surface-affinity.js'; import { validateAiToolReferences } from './validate-ai-tool-references.js'; import { validateAiAgentAuthoring } from './validate-ai-agent-authoring.js'; import { validateHookBodyWrites } from './validate-hook-body-writes.js'; +import { validateActionRecordWrites } from './validate-action-record-writes.js'; export type ReferenceIntegritySeverity = 'error' | 'warning'; @@ -118,6 +139,10 @@ export const REFERENCE_INTEGRITY_RULES: readonly ReferenceIntegrityRule[] = [ // read-side membership (#4271). Lazy: only a hook that actually carries a // `language:'js'` body loads the TypeScript parser. { name: 'validateHookBodyWrites', run: validateHookBodyWrites }, + // Writes an L2 ACTION body aims at `ctx.record` (#4345). Lazy on the same + // terms as the rule above. See "One deliberate non-member" in this module's + // header for why a rule that resolves no name is nevertheless carried here. + { name: 'validateActionRecordWrites', run: validateActionRecordWrites }, ]; /** diff --git a/packages/lint/src/validate-action-record-writes.test.ts b/packages/lint/src/validate-action-record-writes.test.ts new file mode 100644 index 0000000000..aaa3af2b16 --- /dev/null +++ b/packages/lint/src/validate-action-record-writes.test.ts @@ -0,0 +1,240 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { + ACTION_BODY_RECORD_WRITE_DISCARDED, + ACTION_RECORD_WRITE_PATTERNS, + extractActionRecordWrites, + validateActionRecordWrites, +} from './validate-action-record-writes.js'; + +/** A stack with one top-level script action bound to `crm_deal`. */ +const stackWith = (source: string, extra: Record = {}) => ({ + objects: [{ name: 'crm_deal', fields: [{ name: 'stage' }, { name: 'amount' }] }], + actions: [ + { + name: 'close_deal', + objectName: 'crm_deal', + type: 'script', + body: { language: 'js', source, capabilities: ['api.write'] }, + ...extra, + }, + ], +}); + +describe('ACTION_RECORD_WRITE_PATTERNS ledger reconciliation', () => { + // Each declared pattern is driven END-TO-END through the public validator — + // prefilter, action walk, value-dedupe and field collection all sit between + // the example and the finding, so any of them silently swallowing a pattern + // turns this red. A ledger entry cannot be declared-but-unverified (#3528). + it.each(ACTION_RECORD_WRITE_PATTERNS.map((p) => [p.id, p] as const))( + 'pattern %s extracts exactly its declared fields, end to end', + (_id, pattern) => { + const extracted = extractActionRecordWrites(pattern.example.source); + expect(extracted.map((w) => w.field)).toEqual([...pattern.example.fields]); + // Every extracted write is attributed to THIS entry — an example that + // happens to match a neighbouring pattern would not prove this one works. + expect([...new Set(extracted.map((w) => w.patternId))]).toEqual([pattern.id]); + + const findings = validateActionRecordWrites(stackWith(pattern.example.source)); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(ACTION_BODY_RECORD_WRITE_DISCARDED); + for (const field of pattern.example.fields) { + expect(findings[0].message).toContain(`'${field}'`); + } + }, + ); + + it('declares a unique, non-empty id per pattern', () => { + const ids = ACTION_RECORD_WRITE_PATTERNS.map((p) => p.id); + expect(ids.every((id) => id.length > 0)).toBe(true); + expect(new Set(ids).size).toBe(ids.length); + }); +}); + +describe('validateActionRecordWrites', () => { + it('flags a write to a DECLARED field — the defect #4345 is about', () => { + // `stage` is declared on crm_deal. It is dropped exactly like an unknown + // column would be, so silence here would imply it persisted. + const findings = validateActionRecordWrites(stackWith("ctx.record.stage = 'won'; return { ok: true };")); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].where).toBe('action "close_deal" › body'); + expect(findings[0].path).toBe('actions[0].body.source'); + expect(findings[0].message).toContain('READ-ONLY'); + expect(findings[0].message).toContain('whether or not the object declares'); + }); + + it('flags a declared and an undeclared field identically', () => { + const declared = validateActionRecordWrites(stackWith("ctx.record.stage = 'won';")); + const unknown = validateActionRecordWrites(stackWith("ctx.record.stgae = 'won';")); + expect(declared).toHaveLength(1); + expect(unknown).toHaveLength(1); + // No did-you-mean: the field name is not the bug, so suggesting a spelling + // fix would aim the author at the wrong one. + expect(unknown[0].hint).not.toContain('Did you mean'); + }); + + it('names the bound object and the ctx.api remedy in the hint', () => { + const [finding] = validateActionRecordWrites(stackWith("ctx.record.stage = 'won';")); + expect(finding.hint).toContain("ctx.api.object('crm_deal').update("); + expect(finding.hint).toContain('api.write'); + }); + + it('reports each field once, however often it is written', () => { + const findings = validateActionRecordWrites( + stackWith("ctx.record.stage = 'a'; ctx.record.stage = 'b'; ctx.record.amount = 1;"), + ); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain("'stage', 'amount'"); + }); + + it('walks object-embedded actions too', () => { + const findings = validateActionRecordWrites({ + objects: [ + { + name: 'crm_deal', + fields: [{ name: 'stage' }], + actions: [ + { + name: 'close_deal', + type: 'script', + body: { language: 'js', source: "ctx.record.stage = 'won';" }, + }, + ], + }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].path).toBe('objects[0].actions[0].body.source'); + expect(findings[0].hint).toContain("ctx.api.object('crm_deal')"); + }); + + it('reports a defineStack-merged action ONCE, at the position the author wrote it', () => { + // `mergeObjectActions` appends a top-level action carrying `objectName` + // into the object while keeping the top-level entry. After a Zod parse the + // two are structurally equal but NOT reference-identical, so the fixture + // uses independent clones — a shared reference would make identity dedupe + // look like it works and hide the double report. + const authored = () => ({ + name: 'close_deal', + objectName: 'crm_deal', + type: 'script', + body: { language: 'js', source: "ctx.record.stage = 'won';", capabilities: [] }, + }); + const findings = validateActionRecordWrites({ + objects: [{ name: 'crm_deal', fields: [{ name: 'stage' }], actions: [authored()] }], + actions: [authored()], + }); + expect(findings).toHaveLength(1); + expect(findings[0].path).toBe('actions[0].body.source'); + }); + + it('does not confuse two different actions that write the same field', () => { + const action = (name: string) => ({ + name, + objectName: 'crm_deal', + type: 'script', + body: { language: 'js', source: "ctx.record.stage = 'won';", capabilities: [] }, + }); + const findings = validateActionRecordWrites({ + objects: [{ name: 'crm_deal', fields: [{ name: 'stage' }] }], + actions: [action('close_deal'), action('reopen_deal')], + }); + expect(findings.map((f) => f.where)).toEqual([ + 'action "close_deal" › body', + 'action "reopen_deal" › body', + ]); + }); + + it('accepts the name-keyed map authoring shape', () => { + const findings = validateActionRecordWrites({ + objects: [{ name: 'crm_deal', fields: [{ name: 'stage' }] }], + actions: { + close_deal: { + objectName: 'crm_deal', + type: 'script', + body: { language: 'js', source: "ctx.record.stage = 'won';" }, + }, + }, + }); + expect(findings).toHaveLength(1); + expect(findings[0].where).toBe('action "close_deal" › body'); + }); + + describe('stays silent', () => { + const clean = (stack: Record) => expect(validateActionRecordWrites(stack)).toEqual([]); + + it('on a body that only READS the record', () => { + clean(stackWith('var id = ctx.recordId || (ctx.record && ctx.record.id); return { id: id };')); + }); + + it('on the documented remedy — persisting through ctx.api', () => { + clean( + stackWith( + "await ctx.api.object('crm_deal').update({ id: ctx.recordId, stage: 'won' }); return { ok: true };", + ), + ); + }); + + it('on an L1 expression body, which cannot assign at all', () => { + clean({ + objects: [{ name: 'crm_deal' }], + actions: [ + { + name: 'a', + objectName: 'crm_deal', + body: { language: 'expression', source: 'ctx.record.stage' }, + }, + ], + }); + }); + + it('on a target-bound action — a registered handler is not this sandbox seam', () => { + clean({ + objects: [{ name: 'crm_deal' }], + actions: [{ name: 'a', objectName: 'crm_deal', type: 'script', target: 'completeTask' }], + }); + }); + + it('on a hook body — a hook ctx carries no record', () => { + clean({ + objects: [{ name: 'crm_deal' }], + hooks: [{ name: 'h', object: 'crm_deal', body: { language: 'js', source: 'ctx.record.x = 1;' } }], + }); + }); + + it('on stacks with no actions at all', () => { + clean({}); + clean({ objects: [{ name: 'crm_deal' }] }); + }); + + it('on a body writing something that merely LOOKS like ctx.record', () => { + clean(stackWith("var rec = { record: {} }; rec.record.stage = 'won'; ctx.result.record = 1;")); + }); + + // Statically opaque — skipped deliberately, and covered at run time by the + // sandbox's write-recorder proxy instead of guessed at here. + it('on a computed key', () => { + clean(stackWith("var k = 'stage'; ctx.record[k] = 'won';")); + }); + + it('on an aliased reference (no data-flow analysis by design)', () => { + clean(stackWith("var r = ctx.record; r.stage = 'won';")); + }); + + it('on a spread-only Object.assign', () => { + clean(stackWith('Object.assign(ctx.record, patch);')); + }); + }); + + it('never throws on a syntactically broken body, and still reports what it recovered', () => { + // The TypeScript parser recovers rather than bailing, so a body the sandbox + // would refuse to compile still yields the writes it could see. Reporting + // them is right — the assignment is there — and the contract that matters + // is that a malformed body cannot take the whole validation run down. + const findings = validateActionRecordWrites(stackWith('ctx.record.stage = ;;; function(')); + expect(findings.map((f) => f.rule)).toEqual([ACTION_BODY_RECORD_WRITE_DISCARDED]); + expect(() => validateActionRecordWrites(stackWith('}{'))).not.toThrow(); + }); +}); diff --git a/packages/lint/src/validate-action-record-writes.ts b/packages/lint/src/validate-action-record-writes.ts new file mode 100644 index 0000000000..14f36a6cce --- /dev/null +++ b/packages/lint/src/validate-action-record-writes.ts @@ -0,0 +1,370 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Author-time check: an L2 (`language:'js'`) ACTION body that assigns to +// `ctx.record` (#4345). +// +// `ctx.record` is a read-only pre-fetched snapshot. `buildActionSandboxContext` +// hands the body a plain copy (`unwrapProxyToPlain`) and `boundActionHandler` +// returns only `result.value` — the hook path's `applyMutationsToInput` +// write-back has no action counterpart. So `ctx.record.stage = 'won'` mutates a +// copy that dies with the VM: the action returns success and the stored record +// is unchanged. +// +// ## Why this is a rule of its own, and not a case of #4271 +// +// The #4271 family (`hook-body-write-unknown-field`, and its action sibling) +// catches "an UNKNOWN column silently never lands, while declared fields land +// normally". This is the opposite shape: EVERY write is dropped, a correctly +// spelled and fully declared field included. Reporting only the unknown half +// would imply the declared half persists — manufacturing exactly the false +// completion this family exists to kill — which is why the unknown-field rule +// deliberately excludes `ctx.record` and points here instead. +// +// So this rule never consults the object's declared fields: field existence is +// irrelevant to whether the write lands, and a did-you-mean on the field name +// would aim the author at the wrong bug. +// +// ## Scope +// +// • ACTION bodies only. A hook's `ctx` carries no `record` at all, and a +// `target`-bound action (a registered bundle function, not a sandboxed +// body) gets the host's real context — neither goes through this seam. +// • The literal patterns in {@link ACTION_RECORD_WRITE_PATTERNS}, reconciled +// against the extractor by test, so a pattern cannot be declared-but- +// unverified (#3528's death). +// • Everything statically opaque — computed keys (`ctx.record[k] = …`), +// aliases (`const r = ctx.record; r.x = 1`), spreads — is skipped +// SILENTLY, the same asymmetry the sibling rules take: a false positive +// kills an advisory lint, a miss only leaves the gap open. Those cases are +// not lost, though: the runtime's write-recorder proxy (#4345, +// `quickjs-runner.ts`) catches every one of them at invocation time and +// logs the same remedy. This rule is the shift-LEFT of that signal, not its +// only copy. +// +// Severity: always `warning`. Unlike a `readonly` flow write — a certain no-op, +// which `validateReadonlyFlowWrites` gates on — an assignment to `ctx.record` +// may be a deliberate use of the snapshot as local scratch +// (`ctx.record.total = a + b; return { total: ctx.record.total }`), which is +// NOT a no-op and which no analysis short of data-flow can tell apart from an +// intended persist. Gating would block correct builds; the warning states what +// is true of both readings — the write does not leave the sandbox. +// +// Wired via REFERENCE_INTEGRITY_RULES so `os validate`, `os lint` and +// `os compile` all run it (see that module for the membership note). + +import { createRequire } from 'node:module'; +import type ts from 'typescript'; + +// The TypeScript compiler must NOT be imported at module top level — ~9 MB of +// CJS on the kernel boot path, while this gate only parses when an action +// actually carries an L2 JS body that mentions `ctx.record`. Same lazy-load +// contract as validate-hook-body-writes.ts / validate-react-page-props.ts +// (which see for the history); guarded by lazy-deps.test.ts. +let cachedTs: typeof ts | null = null; +function loadTypeScript(): typeof ts { + if (cachedTs) return cachedTs; + const anchor = + typeof import.meta !== 'undefined' && import.meta.url + ? import.meta.url + : typeof __filename !== 'undefined' + ? __filename + : process.cwd() + '/'; + try { + cachedTs = createRequire(anchor)('typescript') as typeof ts; + } catch (err) { + throw new Error( + `@objectstack/lint: checking an L2 (language:'js') action body requires the "typescript" package, which could ` + + `not be loaded (${err instanceof Error ? err.message : String(err)}). It is a declared dependency of ` + + `@objectstack/lint — if this deployment prunes packages, keep "typescript" in the image; it is only loaded ` + + `when an action with a JS body is validated.`, + ); + } + return cachedTs; +} + +export type ActionRecordWriteSeverity = 'warning'; + +export interface ActionRecordWriteFinding { + /** Advisory-only by contract — the type says so. */ + severity: ActionRecordWriteSeverity; + rule: string; + /** Human-readable location, e.g. `action "close_deal" › body`. */ + where: string; + /** Config path, e.g. `actions[0].body.source`. */ + path: string; + message: string; + hint: string; +} + +/** Rule id (registry entry). */ +export const ACTION_BODY_RECORD_WRITE_DISCARDED = 'action-body-record-write-discarded'; + +// ─── The write-pattern ledger ─────────────────────────────────────────────── +// +// Every syntactic shape the extractor recognizes, declared as data. The +// reconciliation test runs the extractor over each entry's example and asserts +// it yields EXACTLY the declared fields, tagged with this entry's id — so the +// answer to "which writes does this rule see?" is this list, not the +// extractor's code. + +/** One syntactic write shape the extractor recognizes. */ +export interface ActionRecordWritePattern { + /** Stable pattern id, carried on every extracted write. */ + readonly id: string; + /** Author-facing syntax summary (for docs/diagnostics, not matching). */ + readonly syntax: string; + /** Reconciliation fixture: extracting `source` must yield exactly `fields`. */ + readonly example: { + readonly source: string; + readonly fields: readonly string[]; + }; +} + +export const ACTION_RECORD_WRITE_PATTERNS: readonly ActionRecordWritePattern[] = [ + { + id: 'record-property-assign', + syntax: "ctx.record. = … | ctx.record[''] ⟨op⟩= …", + example: { + // Compound (`+=`) and logical (`??=`) assignment write their LHS exactly + // as `=` does — the example pins the whole operator range. + source: "ctx.record.stage = 'won'; ctx.record['owner'] ??= 'u1'; ctx.record.amount += 1;", + fields: ['stage', 'owner', 'amount'], + }, + }, + { + id: 'record-object-assign', + syntax: 'Object.assign(ctx.record, { : … })', + example: { + source: "Object.assign(ctx.record, { stage: 'won', 'amount': 1, owner });", + fields: ['stage', 'amount', 'owner'], + }, + }, + { + id: 'record-property-delete', + syntax: "delete ctx.record. | delete ctx.record['']", + example: { + source: "delete ctx.record.stage; delete ctx.record['owner'];", + fields: ['stage', 'owner'], + }, + }, +]; + +type AnyRec = Record; + +const isRec = (v: unknown): v is AnyRec => !!v && typeof v === 'object' && !Array.isArray(v); + +/** Coerce an array-or-name-keyed-map collection to an array (name injected). */ +function asArray(v: unknown): AnyRec[] { + if (Array.isArray(v)) return v.filter((x): x is AnyRec => isRec(x)); + if (isRec(v)) { + return Object.entries(v).map(([name, def]) => ({ + name, + ...(isRec(def) ? def : {}), + })); + } + return []; +} + +/** One statically-extracted `ctx.record` write found in an L2 action body. */ +export interface ExtractedActionRecordWrite { + /** Which {@link ACTION_RECORD_WRITE_PATTERNS} entry matched. */ + patternId: string; + field: string; +} + +/** + * Extract every literal `ctx.record` write an L2 action body performs. + * Parse-only (the source is never executed), error-tolerant (a body with syntax + * errors simply yields fewer matches), and lazy: the TypeScript compiler is not + * loaded when no pattern can possibly match. + */ +export function extractActionRecordWrites(source: string): ExtractedActionRecordWrite[] { + // Every recognizable pattern is rooted at `ctx.record`, so a body missing + // either identifier cannot match and must not pay the compiler load. + if (!/\bctx\b/.test(source) || !/\brecord\b/.test(source)) return []; + + const tsc = loadTypeScript(); + // The runtime wraps an action body as `new AsyncFunction('input', 'ctx', source)` + // — a FUNCTION BODY, not a module. Parse it in the same context so bare + // `return` / `await` mean what they mean at run time. + const sf = tsc.createSourceFile( + 'action-body.ts', + `async function __body(input, ctx) {\n${source}\n}`, + tsc.ScriptTarget.Latest, + /* setParentNodes */ false, + tsc.ScriptKind.TS, + ); + + const writes: ExtractedActionRecordWrite[] = []; + + /** `node` is exactly `ctx.record`. */ + const isCtxRecord = (node: ts.Node): boolean => + tsc.isPropertyAccessExpression(node) && + tsc.isIdentifier(node.expression) && + node.expression.text === 'ctx' && + node.name.text === 'record'; + + /** The literal field name of an expression rooted at `ctx.record`, if any. */ + const fieldOfRecordAccess = (expr: ts.Expression): string | undefined => { + if (tsc.isPropertyAccessExpression(expr) && tsc.isIdentifier(expr.name) && isCtxRecord(expr.expression)) { + return expr.name.text; + } + if (tsc.isElementAccessExpression(expr) && isCtxRecord(expr.expression)) { + const arg = expr.argumentExpression; + if (tsc.isStringLiteral(arg) || tsc.isNoSubstitutionTemplateLiteral(arg)) return arg.text; + } + return undefined; // computed key / nested path — statically opaque + }; + + /** Literal keys of an object-literal expression (spreads/computed skipped). */ + const literalObjectKeys = (node: ts.Expression): string[] => { + if (!tsc.isObjectLiteralExpression(node)) return []; + const keys: string[] = []; + for (const p of node.properties) { + if (tsc.isPropertyAssignment(p)) { + if (tsc.isIdentifier(p.name) || tsc.isStringLiteral(p.name)) keys.push(p.name.text); + } else if (tsc.isShorthandPropertyAssignment(p)) { + keys.push(p.name.text); + } + // spread / computed / method members are statically opaque — skipped + } + return keys; + }; + + const visit = (node: ts.Node): void => { + // Pattern: record-property-assign. FirstAssignment..LastAssignment spans + // `=` and every compound/logical assignment operator (`+=`, `??=`, …). + if ( + tsc.isBinaryExpression(node) && + node.operatorToken.kind >= tsc.SyntaxKind.FirstAssignment && + node.operatorToken.kind <= tsc.SyntaxKind.LastAssignment + ) { + const field = fieldOfRecordAccess(node.left); + if (field !== undefined) writes.push({ patternId: 'record-property-assign', field }); + } + + // Pattern: record-property-delete. + if (tsc.isDeleteExpression(node)) { + const field = fieldOfRecordAccess(node.expression); + if (field !== undefined) writes.push({ patternId: 'record-property-delete', field }); + } + + // Pattern: record-object-assign — Object.assign(ctx.record, {…}). + if (tsc.isCallExpression(node)) { + const callee = node.expression; + if ( + tsc.isPropertyAccessExpression(callee) && + tsc.isIdentifier(callee.expression) && + callee.expression.text === 'Object' && + callee.name.text === 'assign' && + node.arguments.length >= 2 && + isCtxRecord(node.arguments[0]) + ) { + // Later sources overwrite earlier ones but never remove a key, so every + // literal key is genuinely written regardless of the non-literal + // arguments around it. + for (const arg of node.arguments.slice(1)) { + for (const field of literalObjectKeys(arg)) { + writes.push({ patternId: 'record-object-assign', field }); + } + } + } + } + + tsc.forEachChild(node, visit); + }; + visit(sf); + return writes; +} + +/** An action reachable from the stack, with where it was found. */ +interface WalkedAction { + action: AnyRec; + /** The object the action binds to, when statically known. */ + object?: string; + path: string; +} + +/** + * Every action in the stack: top-level first, then object-embedded. + * + * Order matters. `defineStack`'s `mergeObjectActions` appends a top-level + * action carrying `objectName` into that object's `actions` while KEEPING the + * top-level entry, so one authored action is reachable twice. After a Zod parse + * the two copies are structurally equal but not reference-identical, which is + * why the caller dedupes by VALUE, not identity — and why walking top-level + * first makes the surviving report land where the author actually wrote it. + */ +function walkActions(stack: AnyRec): WalkedAction[] { + const out: WalkedAction[] = []; + asArray(stack.actions).forEach((action, i) => { + out.push({ + action, + object: typeof action.objectName === 'string' ? action.objectName : undefined, + path: `actions[${i}]`, + }); + }); + asArray(stack.objects).forEach((obj, oi) => { + const objectName = typeof obj.name === 'string' ? obj.name : undefined; + asArray(obj.actions).forEach((action, ai) => { + out.push({ + action, + object: typeof action.objectName === 'string' ? action.objectName : objectName, + path: `objects[${oi}].actions[${ai}]`, + }); + }); + }); + return out; +} + +/** + * Flag `ctx.record` writes in L2 action bodies. Pure `(stack) => Finding[]` + * (ADR-0019); safe on pre- or post-parse stacks. + */ +export function validateActionRecordWrites(stack: AnyRec): ActionRecordWriteFinding[] { + const findings: ActionRecordWriteFinding[] = []; + const seen = new Set(); + + for (const { action, object, path } of walkActions(stack)) { + const body = action.body; + if (!isRec(body) || body.language !== 'js') continue; + const source = body.source; + if (typeof source !== 'string' || source.trim() === '') continue; + + // Value-identity, not reference-identity: see walkActions. + const name = typeof action.name === 'string' && action.name ? action.name : undefined; + const dedupeKey = `${object ?? ''}${name ?? path}${source}`; + if (seen.has(dedupeKey)) continue; + seen.add(dedupeKey); + + const writes = extractActionRecordWrites(source); + if (writes.length === 0) continue; + + const fields: string[] = []; + for (const w of writes) if (!fields.includes(w.field)) fields.push(w.field); + + const quoted = fields.map((f) => `'${f}'`).join(', '); + const plural = fields.length > 1 ? 's' : ''; + findings.push({ + severity: 'warning', + rule: ACTION_BODY_RECORD_WRITE_DISCARDED, + where: `action "${name ?? path}" › body`, + path: `${path}.body.source`, + message: + `body writes ${fields.length} field${plural} to ctx.record (${quoted}), but ctx.record is a READ-ONLY ` + + `pre-fetched snapshot: the action path returns the script's value and never writes the record back, so ` + + `the write${plural} ${fields.length > 1 ? 'are' : 'is'} discarded — whether or not the object declares ` + + `${fields.length > 1 ? 'those fields' : 'that field'}. The action still reports success and the stored ` + + `record is unchanged (#4345).`, + hint: + `To persist, write through the repository: ` + + `await ctx.api.object('${object ?? ''}').update({ id: ctx.recordId, ${fields[0]}: … }) — and declare ` + + `the 'api.write' capability on the body. To compute a value for the RESPONSE instead, return it ` + + `(\`return { ${fields[0]}: … }\`) rather than assigning to ctx.record. Only the literal patterns in ` + + `ACTION_RECORD_WRITE_PATTERNS are checked — computed keys and aliased references are not, and the runtime ` + + `reports those at invocation time — and this warning never blocks a build.`, + }); + } + + return findings; +} 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..35bbc8f328 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.test.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.test.ts @@ -220,6 +220,145 @@ 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']); + }); + + 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..0ab4edf7bb 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,68 @@ 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 = []; + (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; }, + }); + }; + 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 +953,32 @@ 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.__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 3c79118b53..ed5c90161d 100644 --- a/packages/runtime/src/sandbox/script-runner.ts +++ b/packages/runtime/src/sandbox/script-runner.ts @@ -87,6 +87,20 @@ export interface ScriptContext { * Action only: the record loaded by the dispatcher before the action ran * (when the dispatcher pre-fetches it). May be undefined for actions * declared with `requiresRecord: false` or when no `recordId` was supplied. + * + * **READ-ONLY — it is a pre-fetched snapshot, not a write surface (#4345).** + * The action path hands the body a plain copy and returns only the script's + * value; there is no `ctx.record` write-back, so `ctx.record.stage = 'won'` + * mutates the copy inside the VM and is discarded when the VM is torn down — + * for a *declared* field exactly as much as for an unknown one. To persist, + * write through the repository: `ctx.api.object('').update({ id, … })`. + * + * Do not read this as `ctx.input`'s sibling: `ctx.input` IS written back on + * the hook path ({@link ScriptResult.mutatedInput}), and reasoning by analogy + * from that is what makes this trap cost a data loss rather than a typo. The + * asymmetry is now reported rather than assumed — writes are recorded and + * surfaced via {@link ScriptResult.droppedRecordWrites}, and caught earlier by + * `validateActionRecordWrites` in `@objectstack/lint`. */ record?: unknown; /** Engine-side `result` (only set for after* hooks). */ @@ -123,6 +137,28 @@ 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`, and aliased references + * (`const r = ctx.record; r.x = 1`). + * + * `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/data/hook-body.zod.ts b/packages/spec/src/data/hook-body.zod.ts index 133d5cff44..2968ec2ac5 100644 --- a/packages/spec/src/data/hook-body.zod.ts +++ b/packages/spec/src/data/hook-body.zod.ts @@ -145,6 +145,14 @@ export type ExpressionBody = z.infer; * still prefer a flow `update_record` node, whose structural `fields` config * is error-checked rather than advisory. * + * **On an ACTION body, `ctx.record` is not a write surface at all (#4345).** + * The hook path writes `ctx.input` back to the engine; the action path returns + * only the script's value, so a write to the pre-fetched `ctx.record` snapshot + * is discarded whether or not the field is declared — a different defect from + * the unknown-column one above, and the reason the write-set lint deliberately + * leaves `ctx.record` to `validateActionRecordWrites`. An action persists + * through `ctx.api.object(...).update({ id: ctx.recordId, … })`. + * * @example * ```json * { diff --git a/packages/spec/src/ui/action.zod.ts b/packages/spec/src/ui/action.zod.ts index 34d7fd2582..3a69860890 100644 --- a/packages/spec/src/ui/action.zod.ts +++ b/packages/spec/src/ui/action.zod.ts @@ -470,6 +470,23 @@ const actionObject = () => z.object({ * * Compiled-module bodies are not supported. Outbound IO (HTTP, etc.) goes * through Connector recipes (separate spec). + * + * **The body's write channel is `ctx.api` — `ctx.record` is READ-ONLY (#4345).** + * `ctx.record` is the record the dispatcher pre-fetched: a plain snapshot the + * runtime never writes back. `ctx.record.stage = 'won'` therefore mutates a + * copy that dies with the sandbox — for a DECLARED field exactly as much as + * for a misspelled one — while the action still returns success. To persist: + * + * ```js + * await ctx.api.object('crm_deal').update({ id: ctx.recordId, stage: 'won' }); + * ``` + * + * (with `capabilities: ['api.write']`). Do not reason by analogy from + * `ctx.input`, which IS written back on the hook path. Both ends now report + * the mistake instead of swallowing it: `validateActionRecordWrites` + * (`@objectstack/lint`, advisory) at authoring time for the literal patterns, + * and the sandbox itself at invocation time for every write, including the + * computed keys and aliases static analysis cannot see. */ body: HookBodySchema.optional().describe('Action body — expression (L1) or sandboxed JS (L2). Only used when type is `script`.'), From 4fe2fedc5c3826ef94cb7daa18c629569d180c5d Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:24:30 +0800 Subject: [PATCH 2/2] =?UTF-8?q?docs(actions):=20=E8=AF=B4=E6=98=8E?= =?UTF-8?q?=E4=B8=A4=E7=AB=AF=E9=83=BD=E5=8F=AA=E6=8A=A5=E5=88=B0=E4=B8=8D?= =?UTF-8?q?=E4=BA=86=E5=BA=93=E7=9A=84=E5=86=99;=E6=B4=BB=E5=86=99?= =?UTF-8?q?=E6=A8=A1=E5=BC=8F=E4=BF=9D=E6=8C=81=E5=AE=89=E9=9D=99=20(#4345?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- content/docs/ui/actions.mdx | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/content/docs/ui/actions.mdx b/content/docs/ui/actions.mdx index 4b0105186a..d81ca55ac8 100644 --- a/content/docs/ui/actions.mdx +++ b/content/docs/ui/actions.mdx @@ -119,10 +119,18 @@ Body-carrying actions are **registered automatically** at boot. 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` warn on a literal `ctx.record. = …`, and the sandbox logs - the discarded fields at invocation time — including the computed keys and - aliased references static checks cannot see. + 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)