feat(runtime): action body 被丢弃的 ctx.record 写在调用时可见 (#4345)

.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.

content/docs/ui/actions.mdx

@@ -99,6 +99,40 @@ export const MarkDoneAction = defineAction({
 
 Body-carrying actions are **registered automatically** at boot.
 
+<Callout type="warn">
+  **`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
+  ```
+</Callout>
+
 ### Path B: a registered handler (full TypeScript)
 
 For logic that belongs in real source files, point `target` at a handler name

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<string, unknown>) => {
+      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' }]);
+    });
+  });
 });

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 ?? '<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,