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/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. 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`.'),