From fdec24921d0432817ac438f26ffd539c7e25208e Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:48:59 +0800 Subject: [PATCH 1/2] =?UTF-8?q?feat(lint):=20action=20body=20=E9=87=8C?= =?UTF-8?q?=E8=90=BD=E4=B8=8D=E4=BA=86=E5=9C=B0=E7=9A=84=20ctx.record=20?= =?UTF-8?q?=E5=86=99=E5=9C=A8=E4=BD=9C=E8=80=85=E6=97=B6=E5=91=8A=E8=AD=A6?= =?UTF-8?q?=20(#4345)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4344 deliberately left `ctx.record` alone and said why: an action's `ctx.record` is a plain snapshot (`unwrapProxyToPlain(actionCtx?.record)`) that `boundActionHandler` never writes back — the hook path's `applyMutationsToInput` has no action-side counterpart — so `ctx.record.x = …` is discarded for DECLARED and undeclared fields alike. Reporting that through the unknown-field rule would have been actively wrong: flagging only the undeclared half implies the declared half persists. It needed its own finding. New rule `action-record-write-discarded`, advisory. It is NOT "flag every ctx.record assignment" — mutating the snapshot to build a payload is legitimate: ctx.record.stage = 'won'; await ctx.api.object('crm_deal').update(ctx.record); // LIVE So the finding requires the write to be provably dead: `ctx.record` never escapes the body as a value. Property reads do not rescue a write; handing the object to anything — argument, assignment RHS, spread, return — does. Aliasing reads as an escape, the safe direction. Truthiness and type tests are NOT escapes, and that is what makes the rule fire on real code. Running it against the showcase surfaced it: `mark_done` opens with `ctx.recordId || (ctx.record && ctx.record.id)` — the idiom action bodies are actually written with — and counting that guard as an escape silenced the finding on the one body in the repo that had a record write. Only the LEFT operand of &&/||/?? is a test; `x || ctx.record` still escapes. One suite member, two rule ids: both findings fall out of one parse of one source on one surface. A second REFERENCE_INTEGRITY_RULES member would parse every action body twice to say two things about one walk, and hand-wiring it into the three CLI commands is the drift that suite exists to end — validateReadonlyFlowWrites is the standing proof, wired into validate and compile but never into lint. Trade-off written down at both ends. The ledger ratchet fired as designed. `record-property-assign` joins the shared HOOK_BODY_WRITE_PATTERNS — the extractor's shape inventory, not any one rule's — and both consumers had to classify it first. Not cosmetic on the hook side: a record write carries no `object`, and validateHookBodyWrites branched on exactly that to mean "a ctx.input write", so the new shape would have been reported as "the hook writes 'stage' to its input". The hook rule now declares its own consumed subset and its exclusion with a reason — a hook sandbox context has no ctx.record at all, so the expression throws at run time rather than silently no-op'ing, and a loud failure is not an advisory rule's business. extractHookBodyWriteSet is the new one-parse entry point (writes + ctxRecordEscapes); extractHookBodyWrites stays a thin projection. Boot path: the action prefilter widens from `api` to `api`-or-`record`, so a body reaching neither still never loads the ~9 MB TypeScript compiler. lazy-deps.test.ts pins it — and its header plus two case names, which still claimed every lazy dep waited on "a react page", now name the trigger each one pins (typescript has also been loaded by the hook-body gate since #4271). spec/runtime: ScriptBodySchema, ActionSchema.body and ScriptContext.record now state that ctx.api.object(...) is the only path that persists anything and that ctx.record is read-only in effect. Doc comments only — all 8 generated artifacts verified unchanged. Whether the runtime should instead refuse or honour a record write stays open on #4345. Co-Authored-By: Claude Opus 5 --- .../action-record-write-discarded-lint.md | 86 +++++++++ packages/lint/src/index.ts | 8 + packages/lint/src/lazy-deps.test.ts | 43 +++-- .../src/reference-integrity-suite.test.ts | 14 +- .../lint/src/reference-integrity-suite.ts | 11 ++ .../src/validate-action-body-writes.test.ts | 108 +++++++++-- .../lint/src/validate-action-body-writes.ts | 173 ++++++++++++------ .../src/validate-hook-body-writes.test.ts | 89 +++++++++ .../lint/src/validate-hook-body-writes.ts | 157 +++++++++++++++- packages/runtime/src/sandbox/script-runner.ts | 10 + packages/spec/src/data/hook-body.zod.ts | 12 +- packages/spec/src/ui/action.zod.ts | 17 +- 12 files changed, 627 insertions(+), 101 deletions(-) create mode 100644 .changeset/action-record-write-discarded-lint.md diff --git a/.changeset/action-record-write-discarded-lint.md b/.changeset/action-record-write-discarded-lint.md new file mode 100644 index 0000000000..9333ad8aa9 --- /dev/null +++ b/.changeset/action-record-write-discarded-lint.md @@ -0,0 +1,86 @@ +--- +"@objectstack/lint": minor +"@objectstack/spec": patch +"@objectstack/runtime": patch +--- + +feat(lint): an action body's discarded `ctx.record` write warns at author time (#4345) + +`#4344` deliberately left `ctx.record` alone, and said why: an action's +`ctx.record` is a plain snapshot (`unwrapProxyToPlain(actionCtx?.record)`) that +`boundActionHandler` never writes back — the hook path's +`applyMutationsToInput` has no action-side counterpart — so `ctx.record.x = …` +is discarded for **declared and undeclared fields alike**. Reporting that +through the unknown-field rule would have been actively wrong: flagging only +the undeclared half implies the declared half persists, which is the false +completion this rule family exists to stop manufacturing. It needed its own +finding, and now has one. + +**New rule — `action-record-write-discarded` (advisory).** + +**It is not "flag every `ctx.record.` assignment"** — that would be a +false-positive machine, because mutating the snapshot to build a payload is a +legitimate idiom: + +```js +ctx.record.stage = 'won'; +await ctx.api.object('crm_deal').update(ctx.record); // the write is LIVE +``` + +So the finding requires the write to be **provably dead**: reported only when +`ctx.record` never escapes the body as a value. Property reads +(`ctx.record.id`) do not rescue a write and do not suppress the finding; +handing the object to anything — an argument, an assignment RHS, a spread, a +return — does. Aliasing (`const r = ctx.record`) reads as an escape, which is +the safe direction: it costs a missed finding, never a false one. + +Truthiness and type tests are **not** escapes, and that distinction is what +makes the rule fire on real code rather than almost never. Running it against +the showcase app is what surfaced it: `mark_done` opens with +`ctx.recordId || (ctx.record && ctx.record.id)`, the defensive idiom action +bodies are actually written with, and counting that guard as an escape silenced +the finding on the one body in the repo that had a record write. A test reads +the reference and yields a boolean — or, for `&&`/`||`/`??`, yields the left +operand only when it is falsy, which is null or undefined and persists nothing. +Only the LEFT operand is a test: `x || ctx.record` really does evaluate to the +object, and still escapes. + +**One suite member, two rule ids.** Both findings fall out of one parse of one +source on one surface, so `validateActionBodyWrites` reports both rather than +`REFERENCE_INTEGRITY_RULES` growing a second member that would parse every +action body again to say two things about the same walk. The alternative — +hand-wiring it into the three CLI commands — is the drift that suite exists to +end, and `validateReadonlyFlowWrites` is the standing proof: wired into +`validate` and `compile`, never into `lint`. The trade-off is written down at +both ends rather than left to be rediscovered. + +**The ledger ratchet fired, as designed.** `record-property-assign` joins the +shared `HOOK_BODY_WRITE_PATTERNS` — the extractor's shape inventory, not any +one rule's — and both existing consumers had to classify it before it could +land. That was not cosmetic on the hook side: a `record-property-assign` write +carries no `object`, and `validateHookBodyWrites` branched on exactly that to +mean "a `ctx.input` write", so the new shape would have been reported as *"the +hook writes 'stage' to its input"*. The hook rule now declares its own +consumed subset (`HOOK_BODY_WRITE_PATTERN_IDS`) and its exclusion with a +reason — a hook sandbox context has no `ctx.record` at all +(`buildSandboxContext` never sets it), so the expression throws at run time +rather than silently no-op'ing, and a loud failure is not an advisory rule's +business. + +`extractHookBodyWriteSet` is the new one-parse entry point, returning the +writes plus the `ctxRecordEscapes` signal; `extractHookBodyWrites` stays as a +thin projection of it. + +**Boot path.** The action gate's prefilter widens from `api` to `api`-or- +`record`, so a body reaching neither still never loads the ~9 MB TypeScript +compiler. `lazy-deps.test.ts` pins it — and its header and two case names, +which still claimed every lazy dep waited on "a react page", now say which +trigger each one pins (typescript has also been loaded by the hook-body gate +since #4271). + +`@objectstack/spec` / `@objectstack/runtime`: `ScriptBodySchema`, +`ActionSchema.body` and `ScriptContext.record` now state that +`ctx.api.object(...)` is the only path that persists anything, and that +`ctx.record` is read-only in effect. Doc comments only — no schema or +generated-artifact change. Whether the runtime should instead refuse or honour +a record write stays open on #4345. diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 7f6be7d077..eb7f6b9f13 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -261,14 +261,19 @@ export type { export { validateHookBodyWrites, extractHookBodyWrites, + extractHookBodyWriteSet, HOOK_BODY_WRITE_PATTERNS, + HOOK_BODY_WRITE_PATTERN_IDS, + HOOK_BODY_WRITE_EXCLUSIONS, HOOK_BODY_WRITE_UNKNOWN_FIELD, } from './validate-hook-body-writes.js'; export type { HookBodyWriteFinding, HookBodyWriteSeverity, HookBodyWritePattern, + BodyWritePatternExclusion, ExtractedHookBodyWrite, + ExtractedHookBodyWriteSet, } from './validate-hook-body-writes.js'; // The same write-set check on action bodies — same schema, same sandbox, same @@ -279,8 +284,11 @@ export { validateActionBodyWrites, ACTION_BODY_WRITE_PATTERNS, ACTION_BODY_WRITE_PATTERN_IDS, + ACTION_RECORD_WRITE_PATTERNS, + ACTION_RECORD_WRITE_PATTERN_IDS, ACTION_BODY_WRITE_EXCLUSIONS, ACTION_BODY_WRITE_UNKNOWN_FIELD, + ACTION_RECORD_WRITE_DISCARDED, } from './validate-action-body-writes.js'; export type { ActionBodyWriteFinding, diff --git a/packages/lint/src/lazy-deps.test.ts b/packages/lint/src/lazy-deps.test.ts index 004faf07a3..b39d1997ae 100644 --- a/packages/lint/src/lazy-deps.test.ts +++ b/packages/lint/src/lazy-deps.test.ts @@ -1,21 +1,28 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. // // Boot-path contract: importing @objectstack/lint must NOT load its heavy, -// gate-only dependencies. The package sits on the kernel boot path, while -// each dep below serves a gate that only runs when a `kind:'react'` page is -// actually validated — so each must load lazily, on first use: +// gate-only dependencies. The package sits on the kernel boot path, while each +// dep below serves gates that only run when a particular, uncommon piece of +// metadata is actually validated — so each must load lazily, on first use: // - `typescript` (~9 MB, and has been pruned from production images before -// — see validate-react-page-props.ts), loaded by the react-props gate; +// — see validate-react-page-props.ts), loaded by the react-props gate AND +// by the L2 body write-set gates (validate-hook-body-writes.ts since +// #4271, validate-action-body-writes.ts since #4345); // - `sucrase` (~1.5 MB), loaded by the react syntax gate // (validate-react-pages.ts). // +// "A react page" was the whole story when this file was written; it is not any +// more, and the cases below say which trigger they are pinning. Keep them +// named that way — a test called "until a react page is validated" that also +// covers hook and action bodies is a test nobody can read against its subject. +// // Guarded at three levels because vitest inlines static imports through its // transform (they never hit the native require cache), so an in-worker // require.cache probe alone CANNOT catch a reintroduced eager import: // 1. structural — no src file may eagerly `import ... from` a lazy dep // (only `import type`, which erases at build); // 2. built dist — child `node` processes import both dist formats and prove -// each dep is absent until a react page is validated (skipped when dist +// each dep is absent until the metadata that needs it is validated (skipped when dist // has not been built); // 3. behavioral — each lazy path really loads its dep on demand and the // gate still produces findings. @@ -62,8 +69,8 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { }); // Reused by both dist probes: import the dist entry, prove no lazy dep came - // with it, then run each react-page gate and prove exactly its own dep loads - // — and that the gate still produces its finding. + // with it, then run each gate and prove exactly its own dep loads — and that + // the gate still produces its finding. const reactStack = (source: string) => `{ pages: [{ name: 'r', kind: 'react', source: ${JSON.stringify(source)} }] }`; const childBody = ` const probe = require('node:module').createRequire(process.cwd() + '/probe.js'); @@ -79,7 +86,7 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { if (loaded('typescript')) fail('the hook-body write gate on an L1-only stack must not load typescript'); mod.validateActionBodyWrites(jsAction('input.x > 0', 'expression')); mod.validateActionBodyWrites(jsAction('ctx.input.amout = 1;')); - if (loaded('typescript')) fail('the action-body write gate must not load typescript for a body that never touches ctx.api'); + if (loaded('typescript')) fail('the action-body write gate must not load typescript for a body that reaches neither ctx.api nor 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'); @@ -89,6 +96,8 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { if (!hookWrites.some((f) => f.rule === 'hook-body-write-unknown-field')) fail('hook-body write gate produced no finding'); const actionWrites = mod.validateActionBodyWrites(jsAction("await ctx.api.object('a').update({ amout: 1 });")); if (!actionWrites.some((f) => f.rule === 'action-body-write-unknown-field')) fail('action-body write gate produced no finding'); + const recordWrites = mod.validateActionBodyWrites(jsAction("ctx.record.amount = 1;")); + if (!recordWrites.some((f) => f.rule === 'action-record-write-discarded')) fail('action-body 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'); @@ -96,7 +105,7 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { }; `; - it.skipIf(!existsSync(join(distDir, 'index.cjs')))('built CJS dist does not load a lazy dep until a react page is validated', () => { + it.skipIf(!existsSync(join(distDir, 'index.cjs')))('built CJS dist loads no lazy dep at import time, and each only on its own trigger', () => { const out = execFileSync( process.execPath, ['-e', `${childBody}; check(require(${JSON.stringify(join(distDir, 'index.cjs'))}));`], @@ -105,7 +114,7 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { expect(out).toContain('OK'); }, COLD_LOAD_TIMEOUT_MS); - it.skipIf(!existsSync(join(distDir, 'index.js')))('built ESM dist does not load a lazy dep until a react page is validated', () => { + it.skipIf(!existsSync(join(distDir, 'index.js')))('built ESM dist loads no lazy dep at import time, and each only on its own trigger', () => { const out = execFileSync( process.execPath, [ @@ -136,15 +145,16 @@ 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([]); - // Action bodies narrow it further: the only pattern the action rule carries - // is rooted at `ctx.api`, so even an L2 body that writes params never parses. + // Action bodies narrow it further: both patterns the action rule carries + // are rooted at `ctx.api` or `ctx.record`, so an L2 body that only writes + // params never parses at all. const action = (body: unknown) => ({ name: 'act', label: 'Act', objectName: 'a', body }); expect(validateActionBodyWrites({ actions: [action(undefined)] })).toEqual([]); expect(validateActionBodyWrites({ actions: [action({ language: 'expression', source: 'input.x > 0' })] })).toEqual([]); expect( validateActionBodyWrites({ objects: [{ name: 'a', fields: { amount: {} } }], - actions: [action({ language: 'js', source: 'ctx.input.amout = 1; ctx.record.nope = 2;' })], + actions: [action({ language: 'js', source: 'ctx.input.amout = 1; return { ok: true };' })], }), ).toEqual([]); for (const dep of LAZY_DEPS) { @@ -177,6 +187,13 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { expect(actionWrites.some((f) => f.rule === 'action-body-write-unknown-field' && f.severity === 'warning')).toBe( true, ); + const recordWrites = validateActionBodyWrites({ + objects: [{ name: 'a', fields: { amount: {} } }], + actions: [action({ language: 'js', source: 'ctx.record.amount = 1;' })], + }); + expect(recordWrites.some((f) => f.rule === 'action-record-write-discarded' && f.severity === 'warning')).toBe( + true, + ); const props = validateReactPageProps({ pages: [{ name: 'r', kind: 'react', source: 'function Page(){ return ; }' }], diff --git a/packages/lint/src/reference-integrity-suite.test.ts b/packages/lint/src/reference-integrity-suite.test.ts index b2a643b7a8..258548f8b6 100644 --- a/packages/lint/src/reference-integrity-suite.test.ts +++ b/packages/lint/src/reference-integrity-suite.test.ts @@ -61,16 +61,19 @@ 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' }] }, - // validateActionBodyWrites: the L2 body persists a field crm_lead does - // not declare — the action returns success and the column never lands - // (#4271, the action half of the hook rule below). + // validateActionBodyWrites, both of its rule ids from one body: + // - the L2 body persists a field crm_lead does not declare — the action + // returns success and the column never lands (#4271); + // - and it assigns ctx.record, a snapshot the runtime never writes + // back, without ever passing it anywhere (#4345). { name: 'score_now', label: 'Score Now', objectName: 'crm_lead', body: { language: 'js', - source: "await ctx.api.object('crm_lead').update({ lead_score: 100 });", + source: + "ctx.record.name = 'scored'; await ctx.api.object('crm_lead').update({ lead_score: 100 });", }, }, ], @@ -185,6 +188,9 @@ describe('reference-integrity suite — every member actually runs', () => { expect(rules).toContain('agent-authoring-withdrawn'); expect(rules).toContain('hook-body-write-unknown-field'); expect(rules).toContain('action-body-write-unknown-field'); + // The one member that emits a second rule id — see the suite's comment on + // why it rides along instead of becoming its own entry. + expect(rules).toContain('action-record-write-discarded'); }); it('carries a gating flow-template finding through the suite (#3810)', () => { diff --git a/packages/lint/src/reference-integrity-suite.ts b/packages/lint/src/reference-integrity-suite.ts index 6b0c77de4b..952c2e152e 100644 --- a/packages/lint/src/reference-integrity-suite.ts +++ b/packages/lint/src/reference-integrity-suite.ts @@ -123,6 +123,17 @@ export const REFERENCE_INTEGRITY_RULES: readonly ReferenceIntegrityRule[] = [ // action bodies, run by the same sandbox. Only the `ctx.api` write family // carries over — an action's `ctx.input` is its params bag, not a record // (see that module's ledger). Lazy on the same terms. + // + // The ONE member here that emits two rule ids. Besides resolving `ctx.api` + // writes against declared fields (`action-body-write-unknown-field`), it + // reports a `ctx.record` write that can reach nothing + // (`action-record-write-discarded`, #4345) — not a resolution question, so + // by the charter above it does not belong in the suite. It rides along + // anyway because it falls out of the SAME parse of the SAME source: a + // separate member would parse every action body twice to say two things + // about one walk, and hand-wiring it into the CLI instead is exactly the + // drift this suite exists to end (`validateReadonlyFlowWrites` is the + // standing proof — wired into `validate` and `compile`, never into `lint`). { name: 'validateActionBodyWrites', run: validateActionBodyWrites }, ]; diff --git a/packages/lint/src/validate-action-body-writes.test.ts b/packages/lint/src/validate-action-body-writes.test.ts index 2e1498223e..15b624be81 100644 --- a/packages/lint/src/validate-action-body-writes.test.ts +++ b/packages/lint/src/validate-action-body-writes.test.ts @@ -5,8 +5,11 @@ import { validateActionBodyWrites, ACTION_BODY_WRITE_PATTERNS, ACTION_BODY_WRITE_PATTERN_IDS, + ACTION_RECORD_WRITE_PATTERNS, + ACTION_RECORD_WRITE_PATTERN_IDS, ACTION_BODY_WRITE_EXCLUSIONS, ACTION_BODY_WRITE_UNKNOWN_FIELD, + ACTION_RECORD_WRITE_DISCARDED, } from './validate-action-body-writes.js'; import { extractHookBodyWrites, @@ -56,13 +59,22 @@ describe('ACTION_BODY_WRITE_PATTERNS — ledger partition and reconciliation', ( const shared = HOOK_BODY_WRITE_PATTERNS.map((p) => p.id).sort(); const classified = [ ...ACTION_BODY_WRITE_PATTERN_IDS, + ...ACTION_RECORD_WRITE_PATTERN_IDS, ...ACTION_BODY_WRITE_EXCLUSIONS.map((e) => e.id), ].sort(); expect(classified).toEqual(shared); }); - it('derives the applicable subset from the shared ledger, in ledger order', () => { + it('assigns each ledger shape to at most one check', () => { + const overlap = ACTION_BODY_WRITE_PATTERN_IDS.filter((id) => + ACTION_RECORD_WRITE_PATTERN_IDS.includes(id), + ); + expect(overlap).toEqual([]); + }); + + it('derives each subset from the shared ledger, in ledger order', () => { expect(ACTION_BODY_WRITE_PATTERNS.map((p) => p.id)).toEqual([...ACTION_BODY_WRITE_PATTERN_IDS]); + expect(ACTION_RECORD_WRITE_PATTERNS.map((p) => p.id)).toEqual([...ACTION_RECORD_WRITE_PATTERN_IDS]); }); it('gives every exclusion a non-empty reason', () => { @@ -100,6 +112,22 @@ describe('ACTION_BODY_WRITE_PATTERNS — ledger partition and reconciliation', ( }); } + for (const pattern of ACTION_RECORD_WRITE_PATTERNS) { + // Same end-to-end demand on the other check: prefilter, pattern filter and + // the escape analysis all sit between the ledger example and a finding. + it(`reports every declared write of '${pattern.id}' as a discarded record write`, () => { + const fields = [...new Set(pattern.example.writes.map((w) => w.field))]; + expect(fields.length, `'${pattern.id}' declares no write`).toBeGreaterThan(0); + + const findings = validateActionBodyWrites(stackWith(pattern.example.source)); + expect(findings).toHaveLength(fields.length); + for (const finding of findings) { + expect(finding.severity).toBe('warning'); + expect(finding.rule).toBe(ACTION_RECORD_WRITE_DISCARDED); + } + }); + } + for (const exclusion of ACTION_BODY_WRITE_EXCLUSIONS) { // An exclusion must be about APPLICABILITY, not about a pattern nothing // can extract — so the shared extractor still sees it, and this rule still @@ -182,12 +210,10 @@ describe('validateActionBodyWrites — ctx.api writes', () => { }); }); -describe('validateActionBodyWrites — the params/record surfaces stay unchecked', () => { +describe('validateActionBodyWrites — the params surface stays unchecked', () => { // `ctx.input` is the action's PARAMS bag, not a record: flagging a declared // parameter name against object fields is the false positive that would kill - // the rule. `ctx.record` is a snapshot the runner never writes back, so every - // write to it is discarded — declared fields included — which is a different - // defect, not this one. + // the rule. it('ignores ctx.input writes, however they are spelled', () => { const findings = validateActionBodyWrites( stackWith( @@ -198,20 +224,78 @@ describe('validateActionBodyWrites — the params/record surfaces stay unchecked expect(findings).toEqual([]); }); - it('ignores ctx.record writes', () => { - const findings = validateActionBodyWrites(stackWith("ctx.record.stge = 'won'; ctx.record.nope = 1;")); - expect(findings).toEqual([]); + it('still checks the ctx.api writes of a body that also writes params', () => { + const findings = validateActionBodyWrites( + stackWith("ctx.input.whatever = 1; await ctx.api.object('crm_deal').update({ stag: 'won' });"), + ); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(ACTION_BODY_WRITE_UNKNOWN_FIELD); + }); +}); + +describe('validateActionBodyWrites — discarded ctx.record writes (#4345)', () => { + it('warns on a provably dead snapshot write, declared field or not', () => { + // `stage` IS declared on crm_deal — and that is the point: the runner hands + // the body a plain snapshot and never writes it back, so the assignment is + // discarded either way. A rule that only flagged the undeclared half would + // imply this one persists. + const findings = validateActionBodyWrites(stackWith("ctx.record.stage = 'won';")); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].rule).toBe(ACTION_RECORD_WRITE_DISCARDED); + expect(findings[0].where).toBe('action "close_deal" › body'); + expect(findings[0].path).toBe('actions[0].body.source'); + expect(findings[0].message).toContain('ctx.record.stage'); + expect(findings[0].message).toContain('#4345'); + expect(findings[0].hint).toContain('updateById'); + }); + + it('reads from ctx.record do not rescue the write', () => { + const findings = validateActionBodyWrites( + stackWith("var id = ctx.recordId || ctx.record.id; ctx.record.stage = 'won'; return { ok: true, id: id };"), + ); + expect(findings.map((f) => f.rule)).toEqual([ACTION_RECORD_WRITE_DISCARDED]); + }); + + it('stays silent when the snapshot is a payload under construction', () => { + // The legitimate idiom: mutate the snapshot, then persist it. The writes + // are live, and flagging them would be the false positive that kills an + // advisory rule. + expect( + validateActionBodyWrites( + stackWith("ctx.record.stage = 'won'; await ctx.api.object('crm_deal').update(ctx.record);"), + ), + ).toEqual([]); + // …and every other way the object can leave the body. + for (const escape of ['return ctx.record;', 'const r = ctx.record;', 'const c = { ...ctx.record };']) { + expect(validateActionBodyWrites(stackWith(`ctx.record.stage = 'won'; ${escape}`)), escape).toEqual([]); + } }); - it('still checks the ctx.api writes of a body that also touches params and record', () => { + it('reports each field once, and both rule ids from one body', () => { const findings = validateActionBodyWrites( stackWith( - "ctx.input.whatever = 1; ctx.record.whatever = 2; " + + "ctx.record.stage = 'won'; ctx.record.stage = 'lost'; ctx.record['amount'] += 1; " + "await ctx.api.object('crm_deal').update({ stag: 'won' });", ), ); - expect(findings).toHaveLength(1); - expect(findings[0].message).toContain("'stag'"); + expect(findings.map((f) => f.rule)).toEqual([ + ACTION_RECORD_WRITE_DISCARDED, + ACTION_RECORD_WRITE_DISCARDED, + ACTION_BODY_WRITE_UNKNOWN_FIELD, + ]); + expect(findings.slice(0, 2).map((f) => f.message.match(/ctx\.record\.(\w+)/)?.[1])).toEqual([ + 'stage', + 'amount', + ]); + }); + + it('stays silent on statically-opaque record writes', () => { + const findings = validateActionBodyWrites( + // computed key; nested sub-object write — neither is a literal field write + stackWith("const k = 'x'; ctx.record[k] = 1; ctx.record.address.city = 'SF';"), + ); + expect(findings).toEqual([]); }); }); diff --git a/packages/lint/src/validate-action-body-writes.ts b/packages/lint/src/validate-action-body-writes.ts index 86bb0ee0c4..e247954670 100644 --- a/packages/lint/src/validate-action-body-writes.ts +++ b/packages/lint/src/validate-action-body-writes.ts @@ -25,37 +25,61 @@ // partition-tested against the shared ledger so a pattern added to the hook // side later cannot be silently assumed to apply here. // -// `ctx.record` is likewise NOT a write surface and is deliberately unchecked. -// The runner hands the body a plain snapshot (`record: -// unwrapProxyToPlain(actionCtx?.record)`) and `boundActionHandler` returns -// `result.value` without writing anything back — no `applyMutationsToInput`, -// which is the hook path's alone. So `ctx.record.x = …` is discarded for -// DECLARED and undeclared fields alike. That is a different defect from "the -// unknown column silently vanishes", and flagging only its undeclared half -// would imply the declared half persists — precisely the false completion this -// rule family exists to stop manufacturing. Not this rule's business. +// So the unknown-field check keeps exactly one shape: `api-crud-literal` +// (`ctx.api.object('').insert|.create|.update|.updateById({…})`). That +// is the one path through which an action body actually persists anything, it +// addresses its target object explicitly (so the action's own `objectName` +// binding is irrelevant to the check), and the hook rule's extractor already +// recognizes it verbatim. // -// What survives is `api-crud-literal`: `ctx.api.object('') -// .insert|.create|.update|.updateById({…})`. That is the one path through -// which an action body actually persists anything, it addresses its target -// object explicitly (so the action's own `objectName` binding is irrelevant to -// the check), and the hook rule's extractor already recognizes it verbatim. +// ─── The second finding: a record write that reaches nothing (#4345) ──────── // -// Everything else keeps the hook rule's posture exactly: advisory `warning`, -// silent bail on anything statically unknowable (dynamic object names, -// non-literal payloads, cross-package targets), did-you-mean on a miss. +// `ctx.record` is not a write surface either, but it fails DIFFERENTLY, so it +// gets its own rule id rather than being folded in or dropped. The runner hands +// the body a plain snapshot (`record: unwrapProxyToPlain(actionCtx?.record)`) +// and `boundActionHandler` returns `result.value` without writing anything back +// — no `applyMutationsToInput`, which is the hook path's alone. So +// `ctx.record.x = …` is discarded for DECLARED and undeclared fields alike. +// Reporting that through the unknown-field rule would be actively wrong: +// flagging only the undeclared half implies the declared half persists — +// precisely the false completion this rule family exists to stop manufacturing. // -// Wired via REFERENCE_INTEGRITY_RULES, so `os validate`, `os lint` and -// `os compile` report it at once. Lazy like its sibling: an action body that -// never mentions `api` cannot match the one applicable pattern and never pays -// the TypeScript load. +// It is NOT enough to flag every `ctx.record. = …`, because mutating the +// snapshot to build a payload is a legitimate idiom: +// +// ctx.record.stage = 'won'; +// await ctx.api.object('crm_deal').update(ctx.record); // the write is LIVE +// +// So the finding requires the write to be PROVABLY dead: reported only when +// `ctx.record` never escapes as a value anywhere in the body (see +// `ctxRecordEscapes`). Property reads (`ctx.record.id`) do not rescue a write +// and do not suppress the finding; handing the object to anything — an +// argument, an assignment RHS, a spread, a return — does. Aliasing +// (`const r = ctx.record`) reads as an escape, which is the safe direction. +// +// ─── Shared posture ───────────────────────────────────────────────────────── +// +// Both findings keep the hook rule's posture: advisory `warning`, silent bail +// on anything statically unknowable (dynamic object names, non-literal +// payloads, cross-package targets), did-you-mean on a miss. +// +// ONE suite entry, TWO rule ids. Both findings fall out of one parse of one +// source on one surface, so splitting them into two `REFERENCE_INTEGRITY_RULES` +// members would parse every action body twice to say two things about the same +// walk; hand-wiring the second into the CLI instead is the drift that suite +// exists to end — `validateReadonlyFlowWrites` is the standing proof, wired +// into `validate` and `compile` but never into `lint`. +// +// Lazy like its sibling: a body mentioning neither `api` nor `record` cannot +// match either shape and never pays the TypeScript load. import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared'; import { - extractHookBodyWrites, + extractHookBodyWriteSet, indexObjectFields, IMPLICIT_FIELDS, HOOK_BODY_WRITE_PATTERNS, + type BodyWritePatternExclusion, type HookBodyWritePattern, } from './validate-hook-body-writes.js'; @@ -73,28 +97,26 @@ export interface ActionBodyWriteFinding { hint: string; } -// Rule id (registry entry). +// Rule ids (registry entries). Two, from one walk — see the header. export const ACTION_BODY_WRITE_UNKNOWN_FIELD = 'action-body-write-unknown-field'; +export const ACTION_RECORD_WRITE_DISCARDED = 'action-record-write-discarded'; // ─── The applicable-pattern ledger ────────────────────────────────────────── // // Not a second pattern list: a declared PARTITION of the shared -// `HOOK_BODY_WRITE_PATTERNS` into the shapes that mean the same thing in an -// action body and the shapes that do not. Both halves are data, and +// `HOOK_BODY_WRITE_PATTERNS` into the shapes each of this module's two checks +// consumes, plus the shapes neither does. Every part is data, and // validate-action-body-writes.test.ts asserts they cover the shared ledger -// exactly — so adding a fourth pattern on the hook side FAILS this rule's test -// until someone decides which half it belongs in. Silence is not a decision. - -/** A shared-ledger pattern that does NOT apply to action bodies, and why. */ -export interface ActionBodyWriteExclusion { - /** The {@link HOOK_BODY_WRITE_PATTERNS} entry id being excluded. */ - readonly id: string; - /** Why the shape does not mean the same thing in an action body. */ - readonly reason: string; -} +// exactly — so a pattern added on the hook side FAILS this rule's test until +// someone decides which part it belongs in. Silence is not a decision. +// (`record-property-assign` landing here is that ratchet's first live catch.) + +/** @deprecated alias — the exclusion shape is shared with the hook rule. */ +export type ActionBodyWriteExclusion = BodyWritePatternExclusion; /** - * Pattern ids carried over from {@link HOOK_BODY_WRITE_PATTERNS} verbatim. + * Ledger shapes the UNKNOWN-FIELD check consumes — resolved against the named + * object's declared fields. * * Include-list, not exclude-list, on purpose: an unclassified new pattern is * then inert here (a missed finding) rather than live against a context it was @@ -103,8 +125,14 @@ export interface ActionBodyWriteExclusion { */ export const ACTION_BODY_WRITE_PATTERN_IDS: readonly string[] = ['api-crud-literal']; -/** Shared-ledger patterns deliberately left out, each with its reason. */ -export const ACTION_BODY_WRITE_EXCLUSIONS: readonly ActionBodyWriteExclusion[] = [ +/** + * Ledger shapes the DISCARDED-RECORD-WRITE check consumes — never resolved + * against anything, because no field name can make a discarded write land. + */ +export const ACTION_RECORD_WRITE_PATTERN_IDS: readonly string[] = ['record-property-assign']; + +/** Shared-ledger patterns neither check consumes, each with its reason. */ +export const ACTION_BODY_WRITE_EXCLUSIONS: readonly BodyWritePatternExclusion[] = [ { id: 'input-property-assign', reason: @@ -118,13 +146,18 @@ export const ACTION_BODY_WRITE_EXCLUSIONS: readonly ActionBodyWriteExclusion[] = ]; /** - * The subset of the shared ledger this rule actually sees — the published - * answer to "which writes does the action lint check?". + * The subset of the shared ledger the unknown-field check sees — the published + * answer to "which writes does the action lint resolve against fields?". */ export const ACTION_BODY_WRITE_PATTERNS: readonly HookBodyWritePattern[] = HOOK_BODY_WRITE_PATTERNS.filter((p) => ACTION_BODY_WRITE_PATTERN_IDS.includes(p.id)); +/** The subset the discarded-record-write check sees. */ +export const ACTION_RECORD_WRITE_PATTERNS: readonly HookBodyWritePattern[] = + HOOK_BODY_WRITE_PATTERNS.filter((p) => ACTION_RECORD_WRITE_PATTERN_IDS.includes(p.id)); + const APPLICABLE_IDS: ReadonlySet = new Set(ACTION_BODY_WRITE_PATTERN_IDS); +const RECORD_WRITE_IDS: ReadonlySet = new Set(ACTION_RECORD_WRITE_PATTERN_IDS); type AnyRec = Record; @@ -224,22 +257,56 @@ export function validateActionBodyWrites(stack: AnyRec): ActionBodyWriteFinding[ const sites = collectActionBodies(stack); if (sites.length === 0) return findings; - // Built lazily: a stack whose action bodies never touch `ctx.api` never pays it. + // Built lazily: only the unknown-field check needs it, so a stack whose + // action bodies never reach `ctx.api` never pays it. let objectFields: Map> | null = null; for (const site of sites) { - // Cheap prefilter, narrower than the extractor's own: every applicable - // pattern is rooted at `ctx.api`, so a body without the `api` identifier - // cannot match and must not pay the ~9 MB TypeScript load. Pinned by the - // ledger test — an applicable pattern whose example fails this filter - // fails there, rather than going quietly unchecked here. - if (!/\bapi\b/.test(site.source)) continue; - - const writes = extractHookBodyWrites(site.source).filter((w) => APPLICABLE_IDS.has(w.patternId)); - if (writes.length === 0) continue; + // Cheap prefilter, narrower than the extractor's own: every consumed + // pattern is rooted at `ctx.api` or `ctx.record`, so a body carrying + // neither identifier cannot match and must not pay the ~9 MB TypeScript + // load. Pinned by the ledger test — a consumed pattern whose example fails + // this filter fails there, rather than going quietly unchecked here. + if (!/\bapi\b/.test(site.source) && !/\brecord\b/.test(site.source)) continue; + + // ONE parse per body, both checks read from it. + const { writes: allWrites, ctxRecordEscapes } = extractHookBodyWriteSet(site.source); + const writes = allWrites.filter((w) => APPLICABLE_IDS.has(w.patternId)); + const recordWrites = allWrites.filter((w) => RECORD_WRITE_IDS.has(w.patternId)); + if (writes.length === 0 && recordWrites.length === 0) continue; - objectFields ??= indexObjectFields(stack); const where = `action "${site.name}" › body`; + + // ── Discarded record writes (#4345) ────────────────────────────────── + // Reported only when the write is PROVABLY dead: `ctx.record` never + // leaves the body as a value, so nothing can persist the mutation. When + // it does escape, the snapshot may be a payload under construction, and + // every one of its writes is skipped — a missed finding, never a false one. + if (recordWrites.length > 0 && !ctxRecordEscapes) { + const reportedFields = new Set(); + for (const w of recordWrites) { + if (reportedFields.has(w.field)) continue; + reportedFields.add(w.field); + findings.push({ + severity: 'warning', + rule: ACTION_RECORD_WRITE_DISCARDED, + where, + path: site.path, + message: + `body assigns ctx.record.${w.field}, but an action's ctx.record is a plain snapshot the runtime ` + + `never writes back — the action returns success and the assignment is discarded, whether or not ` + + `'${w.field}' is a declared field (#4345).`, + hint: + `To persist it, write through the API: ctx.api.object('').updateById(ctx.recordId, ` + + `{ ${w.field}: … }). Reported only because ctx.record is never passed anywhere in this body — ` + + `mutating the snapshot and then handing it to an API write is a live payload and is not flagged. ` + + `This warning never blocks a build.`, + }); + } + } + + if (writes.length === 0) continue; + objectFields ??= indexObjectFields(stack); const reported = new Set(); for (const w of writes) { @@ -280,7 +347,7 @@ function fixHint(field: string, declared: string[]): string { return ( (suggestion ? `${suggestion} ` : '') + `Fix the field name, or declare '${field}' on the object. Only the literal write patterns in ` + - `ACTION_BODY_WRITE_PATTERNS are checked — an action's ctx.input is its params bag and ctx.record is a ` + - `discarded snapshot, so neither is a record-write surface — and this warning never blocks a build.` + `ACTION_BODY_WRITE_PATTERNS are checked — an action's ctx.input is its params bag, so it is not a ` + + `record-write surface and is never resolved against fields — and this warning never blocks a build.` ); } diff --git a/packages/lint/src/validate-hook-body-writes.test.ts b/packages/lint/src/validate-hook-body-writes.test.ts index 5c6f3b8164..02b9b93b38 100644 --- a/packages/lint/src/validate-hook-body-writes.test.ts +++ b/packages/lint/src/validate-hook-body-writes.test.ts @@ -4,7 +4,10 @@ import { describe, it, expect } from 'vitest'; import { validateHookBodyWrites, extractHookBodyWrites, + extractHookBodyWriteSet, HOOK_BODY_WRITE_PATTERNS, + HOOK_BODY_WRITE_PATTERN_IDS, + HOOK_BODY_WRITE_EXCLUSIONS, HOOK_BODY_WRITE_UNKNOWN_FIELD, } from './validate-hook-body-writes.js'; @@ -51,6 +54,35 @@ describe('HOOK_BODY_WRITE_PATTERNS — ledger ⇄ extractor reconciliation', () expect(new Set(ids).size).toBe(ids.length); }); + // The ledger is the extractor's shape inventory, not this rule's — it now + // carries a shape (`record-property-assign`) the hook surface does not have. + // So this rule declares which shapes it consumes, and the two halves must + // cover the ledger exactly: the next added shape fails here until someone + // decides whether a hook body can even express it. + it('partitions the ledger into what this rule consumes and what it leaves alone', () => { + const declared = HOOK_BODY_WRITE_PATTERNS.map((p) => p.id).sort(); + const classified = [ + ...HOOK_BODY_WRITE_PATTERN_IDS, + ...HOOK_BODY_WRITE_EXCLUSIONS.map((e) => e.id), + ].sort(); + expect(classified).toEqual(declared); + for (const exclusion of HOOK_BODY_WRITE_EXCLUSIONS) { + expect(exclusion.reason.length, `exclusion '${exclusion.id}' carries no reason`).toBeGreaterThan(0); + } + }); + + it('leaves an excluded shape extractable but unreported', () => { + // `ctx.record` does not exist in a hook sandbox context at all, so the + // expression throws at run time rather than silently no-op'ing — a loud + // failure this advisory rule deliberately stays out of. What must NOT + // happen is it landing in the ctx.input branch as "hook writes 'stage' to + // its input", which is what an unpartitioned rule would have reported. + expect(extractHookBodyWrites("ctx.record.stage = 'won';")).toEqual([ + { patternId: 'record-property-assign', field: 'stage' }, + ]); + expect(validateHookBodyWrites(stackWith("ctx.record.stage = 'won'; ctx.record.nope = 1;"))).toEqual([]); + }); + for (const pattern of HOOK_BODY_WRITE_PATTERNS) { it(`extracts exactly the declared writes for '${pattern.id}'`, () => { const extracted = extractHookBodyWrites(pattern.example.source); @@ -239,3 +271,60 @@ describe('validateHookBodyWrites — scope and shape tolerance', () => { expect(() => validateHookBodyWrites(stackWith('ctx.input.x = ; if ('))).not.toThrow(); }); }); + +describe('extractHookBodyWriteSet — the ctx.record liveness signal', () => { + // One parse yields both the writes and whether `ctx.record` ever leaves the + // body as a value. The action rule needs the second to tell a dead snapshot + // write from a payload under construction; nothing else may consume it + // without this distinction, so it is pinned here at the extractor. + const escapes = (source: string) => extractHookBodyWriteSet(source).ctxRecordEscapes; + + it('does not escape when ctx.record is only read from and written to', () => { + expect(escapes("var id = ctx.record.id; ctx.record.stage = 'won';")).toBe(false); + expect(escapes("ctx.record['amount'] += 1; if (ctx.record.done) { return null; }")).toBe(false); + expect(escapes('ctx.record.address.city = "SF";')).toBe(false); + }); + + it('does not escape through a truthiness or type test', () => { + // The defensive idiom real bodies open with — the showcase's own mark_done + // action is `ctx.recordId || (ctx.record && ctx.record.id)`. Reading a test + // as an escape would silence the rule on most bodies that have a record + // write at all. + expect(escapes('var id = ctx.recordId || (ctx.record && ctx.record.id);')).toBe(false); + expect(escapes('if (ctx.record) { ctx.record.stage = "won"; }')).toBe(false); + expect(escapes('if (!ctx.record) return null;')).toBe(false); + expect(escapes("if (typeof ctx.record === 'object') { ctx.record.x = 1; }")).toBe(false); + expect(escapes('var v = ctx.record ? 1 : 2;')).toBe(false); + expect(escapes('var v = ctx.record ?? {};')).toBe(false); + }); + + it('still escapes when a test position yields the object itself', () => { + // Only the LEFT operand of &&/||/?? is a pure test; the right one is the + // expression's value when the left is truthy/nullish. + expect(escapes('var r = fallback || ctx.record;')).toBe(true); + expect(escapes('var r = ctx.record ? ctx.record : {};')).toBe(true); + }); + + it('escapes when the object itself is handed to anything', () => { + expect(escapes("await ctx.api.object('crm_deal').update(ctx.record);")).toBe(true); // argument + expect(escapes('const r = ctx.record;')).toBe(true); // alias + expect(escapes('return ctx.record;')).toBe(true); // returned + expect(escapes('const copy = { ...ctx.record };')).toBe(true); // spread + expect(escapes('Object.assign(ctx.record, { a: 1 });')).toBe(true); // assign target + }); + + it('is false when the body never mentions ctx.record, including the no-parse fast path', () => { + expect(escapes("ctx.input.total = 1;")).toBe(false); + expect(escapes('return 1;')).toBe(false); // prefiltered out before the parse + }); + + it('reports one escape for the whole body, not per write', () => { + // A single escape anywhere disqualifies every record write in the body — + // the signal is body-scoped by design (no data-flow analysis). + const set = extractHookBodyWriteSet( + "ctx.record.stage = 'won'; ctx.record.amount = 1; await ctx.api.object('d').update(ctx.record);", + ); + expect(set.writes.filter((w) => w.patternId === 'record-property-assign')).toHaveLength(2); + expect(set.ctxRecordEscapes).toBe(true); + }); +}); diff --git a/packages/lint/src/validate-hook-body-writes.ts b/packages/lint/src/validate-hook-body-writes.ts index 10f2f44a42..783e3693a0 100644 --- a/packages/lint/src/validate-hook-body-writes.ts +++ b/packages/lint/src/validate-hook-body-writes.ts @@ -143,6 +143,17 @@ export const HOOK_BODY_WRITE_PATTERNS: readonly HookBodyWritePattern[] = [ writes: [{ field: 'total' }, { field: 'status' }, { field: 'discount' }], }, }, + { + // ACTION-only shape (the hook sandbox context has no `ctx.record` at all). + // Declared here because this ledger is the extractor's shape inventory, not + // any one rule's; every consumer declares which shapes it consumes. + id: 'record-property-assign', + syntax: "ctx.record. = … | ctx.record[''] ⟨op⟩= …", + example: { + source: "ctx.record.stage = 'won'; ctx.record['amount'] += 1;", + writes: [{ field: 'stage' }, { field: 'amount' }], + }, + }, { id: 'api-crud-literal', syntax: @@ -162,6 +173,44 @@ export const HOOK_BODY_WRITE_PATTERNS: readonly HookBodyWritePattern[] = [ }, ]; +/** A ledger pattern a given rule does NOT consume, and why. */ +export interface BodyWritePatternExclusion { + /** The {@link HOOK_BODY_WRITE_PATTERNS} entry id being excluded. */ + readonly id: string; + /** Why the shape does not mean the same thing on this rule's surface. */ + readonly reason: string; +} + +/** + * The ledger shapes THIS rule consumes. + * + * Declared rather than implied: before the ledger carried a shape the hook + * surface does not have, every write with no `object` was necessarily a + * `ctx.input` write, and the rule could branch on that alone. It no longer can + * — a `record-property-assign` write also carries no object, and would have + * been reported as "the hook writes 'stage' to its input", which is false. + * Each consumer declaring its own subset is what stops the next added shape + * from silently landing in a branch that was never written for it. + */ +export const HOOK_BODY_WRITE_PATTERN_IDS: readonly string[] = [ + 'input-property-assign', + 'input-object-assign', + 'api-crud-literal', +]; + +/** Ledger shapes this rule leaves alone, each with its reason. */ +export const HOOK_BODY_WRITE_EXCLUSIONS: readonly BodyWritePatternExclusion[] = [ + { + id: 'record-property-assign', + reason: + 'a hook sandbox context has no `ctx.record` at all — `buildSandboxContext` never sets it (a hook’s ' + + 'record IS `ctx.input`), so the expression throws at run time rather than silently no-op’ing. A loud ' + + 'failure the author sees on the first run is not this advisory rule’s business', + }, +]; + +const HOOK_APPLICABLE_IDS: ReadonlySet = new Set(HOOK_BODY_WRITE_PATTERN_IDS); + /** * `ctx.api.object(name)` write methods → index of the record-payload argument. * Mirrors `ObjectRepository` in packages/objectql (the surface hooks actually @@ -250,16 +299,45 @@ export interface ExtractedHookBodyWrite { field: string; } +/** Everything one parse of an L2 body yields. */ +export interface ExtractedHookBodyWriteSet { + /** Every literal write the {@link HOOK_BODY_WRITE_PATTERNS} ledger declares. */ + writes: ExtractedHookBodyWrite[]; + /** + * `ctx.record` is handed to something as a VALUE somewhere in the body — an + * argument, an assignment RHS, a spread, a return — rather than only having + * its properties read and written, or being truthiness/type tested. + * + * The action rule needs this to tell a dead snapshot write from a live one: + * `ctx.record.stage = 'won'; await ctx.api.object('d').update(ctx.record)` + * builds a payload and persists it, so the assignment is not a no-op. When + * this is true, no record write in the body can be judged, and none is + * reported. (One-level aliasing — `const r = ctx.record` — reads as an + * escape too, which is the safe direction: it suppresses findings.) + */ + ctxRecordEscapes: boolean; +} + /** * Extract every literal field write the pattern ledger declares from an L2 * body's source. 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. + * + * Thin projection of {@link extractHookBodyWriteSet} — use that one when the + * `ctx.record` liveness signal matters, so the body is parsed once, not twice. */ export function extractHookBodyWrites(source: string): ExtractedHookBodyWrite[] { + return extractHookBodyWriteSet(source).writes; +} + +/** {@link extractHookBodyWrites} plus the `ctx.record` liveness signal, one parse. */ +export function extractHookBodyWriteSet(source: string): ExtractedHookBodyWriteSet { // Every recognizable pattern begins at a `ctx` or `Object` identifier — a // body containing neither cannot match, and must not pay the compiler load. - if (!/\bctx\b/.test(source) && !/\bObject\b/.test(source)) return []; + if (!/\bctx\b/.test(source) && !/\bObject\b/.test(source)) { + return { writes: [], ctxRecordEscapes: false }; + } const tsc = loadTypeScript(); // The runtime wraps a hook body as `new AsyncFunction('ctx', source)` — a @@ -274,6 +352,9 @@ export function extractHookBodyWrites(source: string): ExtractedHookBodyWrite[] ); const writes: ExtractedHookBodyWrite[] = []; + /** Every `ctx.record` reference, and the subset that is only an access base. */ + const recordRefs: ts.Node[] = []; + const consumedRecordRefs = new Set(); /** `node` is exactly `ctx.`. */ const isCtxDot = (node: ts.Node, prop: string): boolean => @@ -282,12 +363,12 @@ export function extractHookBodyWrites(source: string): ExtractedHookBodyWrite[] node.expression.text === 'ctx' && node.name.text === prop; - /** The literal record-field name of an LHS rooted at `ctx.input`, if any. */ - const fieldOfInputLhs = (lhs: ts.Expression): string | undefined => { - if (tsc.isPropertyAccessExpression(lhs) && tsc.isIdentifier(lhs.name) && isCtxDot(lhs.expression, 'input')) { + /** The literal field name of an LHS rooted at `ctx.`, if any. */ + const fieldOfCtxLhs = (lhs: ts.Expression, prop: string): string | undefined => { + if (tsc.isPropertyAccessExpression(lhs) && tsc.isIdentifier(lhs.name) && isCtxDot(lhs.expression, prop)) { return lhs.name.text; } - if (tsc.isElementAccessExpression(lhs) && isCtxDot(lhs.expression, 'input')) { + if (tsc.isElementAccessExpression(lhs) && isCtxDot(lhs.expression, prop)) { const arg = lhs.argumentExpression; if (tsc.isStringLiteral(arg) || tsc.isNoSubstitutionTemplateLiteral(arg)) return arg.text; } @@ -318,11 +399,64 @@ export function extractHookBodyWrites(source: string): ExtractedHookBodyWrite[] node.operatorToken.kind >= tsc.SyntaxKind.FirstAssignment && node.operatorToken.kind <= tsc.SyntaxKind.LastAssignment ) { - const field = fieldOfInputLhs(node.left); - if (field !== undefined && !INPUT_ENVELOPE_KEYS.has(field)) { - writes.push({ patternId: 'input-property-assign', field }); + const inputField = fieldOfCtxLhs(node.left, 'input'); + if (inputField !== undefined && !INPUT_ENVELOPE_KEYS.has(inputField)) { + writes.push({ patternId: 'input-property-assign', field: inputField }); + } + // Pattern: record-property-assign. No envelope-key filter — `ctx.record` + // is a plain snapshot of the record, not the flat-input proxy, so it + // carries no operation envelope to exclude. + const recordField = fieldOfCtxLhs(node.left, 'record'); + if (recordField !== undefined) { + writes.push({ patternId: 'record-property-assign', field: recordField }); + } + } + + // `ctx.record` liveness, for the rule that judges whether a record write + // can possibly matter. A reference is CONSUMED when the position it sits in + // cannot hand the object to anything that might persist it; every other + // position — an argument, an assignment RHS, a spread, a return — can. + // + // 1. the base of a property/element access: `ctx.record.id`, + // `ctx.record.x = 1`, `ctx.record['k']`; + // 2. a truthiness or type test. `ctx.record && ctx.record.id` is the + // defensive idiom real action bodies are written with (the showcase's + // own `mark_done` opens with it), and reading a test as an escape + // would suppress the finding on most bodies that have one. A test + // reads the reference and yields a boolean — or, for `&&`/`||`/`??`, + // yields the LEFT operand only when it is falsy, which is null or + // undefined and persists nothing either way. Only the left operand is + // a test: `x || ctx.record` really does evaluate to the object. + if (tsc.isPropertyAccessExpression(node) || tsc.isElementAccessExpression(node)) { + if (isCtxDot(node.expression, 'record')) consumedRecordRefs.add(node.expression); + } + if (tsc.isBinaryExpression(node)) { + const op = node.operatorToken.kind; + if ( + (op === tsc.SyntaxKind.AmpersandAmpersandToken || + op === tsc.SyntaxKind.BarBarToken || + op === tsc.SyntaxKind.QuestionQuestionToken) && + isCtxDot(node.left, 'record') + ) { + consumedRecordRefs.add(node.left); } } + if (tsc.isPrefixUnaryExpression(node) && node.operator === tsc.SyntaxKind.ExclamationToken) { + if (isCtxDot(node.operand, 'record')) consumedRecordRefs.add(node.operand); + } + if (tsc.isTypeOfExpression(node) && isCtxDot(node.expression, 'record')) { + consumedRecordRefs.add(node.expression); + } + if ( + (tsc.isIfStatement(node) || tsc.isWhileStatement(node) || tsc.isDoStatement(node)) && + isCtxDot(node.expression, 'record') + ) { + consumedRecordRefs.add(node.expression); + } + if (tsc.isConditionalExpression(node) && isCtxDot(node.condition, 'record')) { + consumedRecordRefs.add(node.condition); + } + if (isCtxDot(node, 'record')) recordRefs.push(node); if (tsc.isCallExpression(node)) { const callee = node.expression; @@ -383,7 +517,10 @@ export function extractHookBodyWrites(source: string): ExtractedHookBodyWrite[] tsc.forEachChild(node, visit); }; visit(sf); - return writes; + return { + writes, + ctxRecordEscapes: recordRefs.some((ref) => !consumedRecordRefs.has(ref)), + }; } /** @@ -404,7 +541,7 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] { const source = body.source; if (typeof source !== 'string' || source.trim() === '') return; - const writes = extractHookBodyWrites(source); + const writes = extractHookBodyWrites(source).filter((w) => HOOK_APPLICABLE_IDS.has(w.patternId)); if (writes.length === 0) return; objectFields ??= indexObjectFields(stack); diff --git a/packages/runtime/src/sandbox/script-runner.ts b/packages/runtime/src/sandbox/script-runner.ts index 3c79118b53..fa59dd1e1f 100644 --- a/packages/runtime/src/sandbox/script-runner.ts +++ b/packages/runtime/src/sandbox/script-runner.ts @@ -87,6 +87,16 @@ 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 in effect. `buildActionSandboxContext` passes a plain snapshot + * (`unwrapProxyToPlain`), and `boundActionHandler` returns `result.value` + * without writing anything back — the hook path's `applyMutationsToInput` has + * 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. */ record?: unknown; /** Engine-side `result` (only set for after* hooks). */ diff --git a/packages/spec/src/data/hook-body.zod.ts b/packages/spec/src/data/hook-body.zod.ts index bd09bc798c..6b90741455 100644 --- a/packages/spec/src/data/hook-body.zod.ts +++ b/packages/spec/src/data/hook-body.zod.ts @@ -148,9 +148,15 @@ export type ExpressionBody = z.infer; * An **action** body carrying this same schema is checked by the sibling rule * `validateActionBodyWrites`, over the subset of that ledger which survives the * context change — `ctx.api.object('y').insert|create|update|updateById({ x })` - * and nothing else. An action's `ctx.input` is its PARAMS bag, not a record, - * and `ctx.record` is a snapshot the runner never writes back, so neither is a - * record-write surface and neither is resolved against object fields. + * and nothing else. An action's `ctx.input` is its PARAMS bag, not a record, so + * it is never resolved against object fields. + * + * `ctx.record` is not a write surface at all: the runner passes a snapshot and + * never writes it back, so an assignment to it is discarded whether or not the + * field is declared. That gets its own warning + * (`action-record-write-discarded`, #4345), reported only when `ctx.record` + * never leaves the body as a value — mutating the snapshot and then handing it + * to an API write is a payload under construction, and is not flagged. * * @example * ```json diff --git a/packages/spec/src/ui/action.zod.ts b/packages/spec/src/ui/action.zod.ts index a622618155..5234f33d1b 100644 --- a/packages/spec/src/ui/action.zod.ts +++ b/packages/spec/src/ui/action.zod.ts @@ -471,12 +471,17 @@ const actionObject = () => z.object({ * Compiled-module bodies are not supported. Outbound IO (HTTP, etc.) goes * through Connector recipes (separate spec). * - * The writes an L2 body persists are checked at author time by - * `validateActionBodyWrites` in `@objectstack/lint` — a literal - * `ctx.api.object('y').update({ x })` naming a field object `y` never - * declares warns with a did-you-mean, because the call succeeds while the - * unknown column silently never lands (#4271). Advisory only, and blind to - * everything statically unknowable; see `ScriptBodySchema` for the scope. + * `ctx.api.object(...)` is the ONLY way a body persists anything. `ctx.input` + * is the action's params bag, and `ctx.record` is a snapshot the runtime + * never writes back — assigning to it is discarded, declared field or not. + * + * Both are checked at author time by `validateActionBodyWrites` in + * `@objectstack/lint`: a literal `ctx.api.object('y').update({ x })` naming a + * field object `y` never declares warns with a did-you-mean, because the call + * succeeds while the unknown column silently never lands (#4271); and a + * provably dead `ctx.record. = …` warns as discarded (#4345). + * Advisory only, and blind to everything statically unknowable; see + * `ScriptBodySchema` for the scope. */ body: HookBodySchema.optional().describe('Action body — expression (L1) or sandboxed JS (L2). Only used when type is `script`.'), From 2258a2615f91172a22cb24d8e8d0e1a38fa2e9a5 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:53:54 +0800 Subject: [PATCH 2/2] Merge origin/main; document the ctx.record write in the section #4351 just rewrote #4351/#4355 rewrote content/docs/automation/hook-bodies.mdx to stop claiming the write side was unchecked. That section lands squarely on this branch's subject, so it is updated here rather than left false for a second time: the write-shape table gains the ctx.record row (and is no longer 'three shapes'), the silent-bail list gains the escape rule, and Signature conventions now says outright that an action's ctx.record is read-only in effect and ctx.api.object(...) is the one way a body persists anything. --- content/docs/automation/hook-bodies.mdx | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/content/docs/automation/hook-bodies.mdx b/content/docs/automation/hook-bodies.mdx index 00514d611d..56ba6d02d9 100644 --- a/content/docs/automation/hook-bodies.mdx +++ b/content/docs/automation/hook-bodies.mdx @@ -123,14 +123,16 @@ Static validation around a hook is asymmetric, and it is worth knowing exactly w - **Checked — read side.** `hook.condition` is validated at build time against the target object's fields by the expression validator (`@objectstack/lint`), including array-valued `hook.object` targets. A condition referencing a nonexistent field fails the lint. - **Checked — capability side.** `body.capabilities` gates which `ctx` APIs the body may call at all; the sandbox throws on an undeclared call. - **Checked — write side, advisory and literal-only.** Since [#4271](https://github.com/objectstack-ai/objectstack/issues/4271), `body.source` is **parsed** (never executed, never type-checked) and the field names it writes are resolved against the target object's declarations. An unknown field raises `hook-body-write-unknown-field` — a **warning** carrying a did-you-mean suggestion, which never blocks a build. Action bodies get the same check on their `ctx.api` writes (`action-body-write-unknown-field`). Both run under `os validate`, `os lint` and `os compile`. +- **Checked — writes that reach nothing at all.** Since [#4345](https://github.com/objectstack-ai/objectstack/issues/4345), an action body assigning to `ctx.record` raises `action-record-write-discarded`, also a warning. This one is **not** a field-resolution question: an action's `ctx.record` is a snapshot the runtime never writes back, so the assignment is discarded whether or not the field is declared — see [Signature conventions](#signature-conventions) below. -Three literal write shapes are recognized, and only these: +Four literal write shapes are recognized, and only these: | Write shape | Hook body | Action body | |---|---|---| | `ctx.input. = …` / `ctx.input[''] ⟨op⟩= …` (including `+=`, `??=`, …) | checked | not checked — an action's `ctx.input` is its **params bag**, not a record | | `Object.assign(ctx.input, { : … })` | checked | not checked — same surface | | `ctx.api.object('').insert\|create\|update({ : … })`, `.updateById(id, { : … })` | checked | checked | +| `ctx.record. = …` / `ctx.record[''] ⟨op⟩= …` | n/a — a hook context has no `ctx.record` (the expression throws) | checked: warns as **discarded**, declared field or not | **A missing warning is not a clean bill of health.** The rule bails *silently* on everything it cannot resolve statically, deliberately preferring a missed finding to a false one — a false positive kills an advisory lint, while a miss just leaves the gap open a little longer: @@ -139,7 +141,8 @@ Three literal write shapes are recognized, and only these: - `ctx.input` writes in a wildcard (`object: '*'`) hook — there is no single target to resolve against; - multi-target hooks where the field exists on *some* target: a body may legitimately branch per object, so only a field missing on **every** named target is flagged; - objects declared by another package; -- aliased input (`const doc = ctx.input; doc.x = 1`) — v1 does no data-flow analysis. +- aliased input (`const doc = ctx.input; doc.x = 1`) — v1 does no data-flow analysis; +- `ctx.record` writes in a body that hands `ctx.record` to anything — an argument, an assignment RHS, a spread, a return. Mutating the snapshot and then persisting it (`ctx.record.stage = 'won'; await ctx.api.object('crm_deal').update(ctx.record)`) is a **live** payload, so the whole body's record writes are skipped rather than guessed at. Truthiness and type guards (`ctx.record && ctx.record.id`, `if (!ctx.record) …`) are not escapes — they cannot persist anything. System/audit columns and the flat-input envelope keys (`id`, `options`, `ast`, `data`) are never flagged. @@ -170,6 +173,14 @@ Because the checking is advisory and literal-only: Hooks mutate `ctx.input`/`ctx.result`; actions return their output value explicitly. +An action's `ctx` is **not** a hook's. `ctx.input` is the action's **params bag** — validated against its declared `params`, not a record. `ctx.record` is the record the dispatcher pre-fetched, and it is **read-only in effect**: the sandbox receives a plain snapshot and the runtime never writes it back, so `ctx.record. = …` is discarded even for a perfectly valid field name. There is exactly one way an action body persists anything: + +```js +await ctx.api.object('crm_deal').updateById(ctx.recordId, { stage: 'won' }); +``` + +Mutating the snapshot *as a payload* and then handing it to such a call is fine — that write is live, and the lint leaves it alone. + ### 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.