From 4c64648615e56ca2d7d0c40ff3401c06211c72bd Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 14:58:41 +0800 Subject: [PATCH] =?UTF-8?q?feat(lint,spec):=20L2=20action=20body=20?= =?UTF-8?q?=E5=86=99=E4=B8=8D=E5=AD=98=E5=9C=A8=E5=AD=97=E6=AE=B5=E4=BB=8E?= =?UTF-8?q?=E7=9B=B2=E5=8C=BA=E5=8F=98=E4=B8=BA=E4=BD=9C=E8=80=85=E6=97=B6?= =?UTF-8?q?=20lint=20=E5=91=8A=E8=AD=A6=20(#4271)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The write-set lint #4305 gave L2 hook bodies now covers the other surface that carries one. An action body is the same artefact — same `HookBodySchema`, same `HookBodySchema.safeParse` in `actionBodyRunnerFactory`, same QuickJS sandbox — so it fails the same way: `ctx.api.object('crm_deal').update({ stag: 'won' })` succeeds, returns success to the caller, and the unknown column never lands. New rule `action-body-write-unknown-field`, advisory, wired into REFERENCE_INTEGRITY_RULES so `os validate` / `os lint` / `os compile` all report it. Both places the runtime reads actions from are walked (top-level `actions` and `objects[].actions`); a defineStack-merged action, which lives in both, is reported once at its authored path. Only the `ctx.api` write family carries over. An action's `ctx.input` is its PARAMS bag, not a record, so resolving those names against object fields would flag every correctly-named parameter. `ctx.record` is not a write surface either: the runner hands the body a plain snapshot and never writes it back, so `ctx.record.x = …` is discarded for declared and undeclared fields alike — a different defect, and flagging only its undeclared half would imply the declared half persists. So the rule ships a declared PARTITION of the shared HOOK_BODY_WRITE_PATTERNS — ACTION_BODY_WRITE_PATTERN_IDS plus ACTION_BODY_WRITE_EXCLUSIONS, each exclusion carrying its reason — tested to cover the shared ledger exactly, so a fourth pattern landing on the hook side fails this rule's test until someone classifies it. Every applicable pattern is proved end-to-end through the full validator; every exclusion is proved extractable-but-inapplicable. One extractor, one field index, one system-column set, shared rather than copied. The dedupe is by value (bound object + name + body source), not by object identity the way collectBundleActions can afford: the suite runs on the parsed stack and parsing rebuilds every node, so a merged action's two copies arrive distinct-but-equal. An identity check passed the unit fixture and reported the showcase app's one warning twice — caught by running `os validate` on it. Lands one notch tighter than the hook side on the boot path: the only applicable pattern is rooted at `ctx.api`, so an action body that never mentions it does not parse at all, let alone load the ~9 MB TypeScript compiler. Guarded by lazy-deps.test.ts. spec: ScriptBodySchema and ActionSchema.body point at the action-side rule and spell out that ctx.input (params) and ctx.record (a discarded snapshot) are not record-write surfaces. Doc comments only — all 8 generated artifacts verified unchanged. Co-Authored-By: Claude Opus 5 --- .changeset/action-body-write-set-lint.md | 63 ++++ packages/lint/src/index.ts | 17 + packages/lint/src/lazy-deps.test.ts | 29 +- .../src/reference-integrity-suite.test.ts | 18 +- .../lint/src/reference-integrity-suite.ts | 6 + .../src/validate-action-body-writes.test.ts | 324 ++++++++++++++++++ .../lint/src/validate-action-body-writes.ts | 286 ++++++++++++++++ .../lint/src/validate-hook-body-writes.ts | 28 +- packages/spec/src/data/hook-body.zod.ts | 7 + packages/spec/src/ui/action.zod.ts | 7 + 10 files changed, 776 insertions(+), 9 deletions(-) create mode 100644 .changeset/action-body-write-set-lint.md create mode 100644 packages/lint/src/validate-action-body-writes.test.ts create mode 100644 packages/lint/src/validate-action-body-writes.ts diff --git a/.changeset/action-body-write-set-lint.md b/.changeset/action-body-write-set-lint.md new file mode 100644 index 0000000000..dd7ec6928a --- /dev/null +++ b/.changeset/action-body-write-set-lint.md @@ -0,0 +1,63 @@ +--- +"@objectstack/lint": minor +"@objectstack/spec": patch +--- + +feat(lint): L2 action-body writes to undeclared fields warn at author time (#4271) + +The write-set lint that #4305 gave L2 hook bodies now covers the other surface +that carries one. An action body is the same artefact: the same +`HookBodySchema` union, parsed by the same `HookBodySchema.safeParse` in +`actionBodyRunnerFactory`, run in the same QuickJS sandbox. So it fails the +same way — `ctx.api.object('crm_deal').update({ stag: 'won' })` inside an +action succeeds, returns success to the caller, and the unknown column simply +never lands. Half the surface was still blind. + +**New rule — `action-body-write-unknown-field` (advisory).** Wired into +`REFERENCE_INTEGRITY_RULES`, so `os validate`, `os lint` and `os compile` all +report it; it never blocks a build. Both places the runtime reads actions from +are walked — top-level `actions` and `objects[].actions` — and a +`defineStack`-merged action, which lives in both, is reported once at its +authored path. That dedupe is by VALUE (bound object + name + body source), not +by object identity the way `collectBundleActions` can afford: the suite runs on +the schema-PARSED stack, and parsing rebuilds every node, so the two copies +arrive as distinct objects that are merely equal. An identity check passes a +shared-reference unit fixture and then reports the showcase app's one warning +twice — which is exactly what it did before the end-to-end run caught it. + +**Only the `ctx.api` write family carries over, and that is the point.** An +action's `ctx.input` is its PARAMS bag (`input: unwrapProxyToPlain(actionCtx +?.params)`), not a record, so resolving those names against object fields would +flag every correctly-named parameter — a pure false-positive machine, and a +false positive kills an advisory lint. `ctx.record` is not a write surface +either: the runner hands the body a plain snapshot and never writes it back, so +`ctx.record.x = …` is discarded for *declared* and undeclared fields alike — +a different defect from "the unknown column vanishes", and flagging only its +undeclared half would imply the declared half persists. + +So the rule ships a declared **partition** of the shared +`HOOK_BODY_WRITE_PATTERNS` rather than a second ledger: +`ACTION_BODY_WRITE_PATTERN_IDS` (today: `api-crud-literal`) and +`ACTION_BODY_WRITE_EXCLUSIONS` (`input-property-assign`, +`input-object-assign`), each exclusion carrying its reason. The two halves are +tested to cover the shared ledger exactly, so a fourth pattern landing on the +hook side fails this rule's test until someone classifies it — silence is not a +decision. Every applicable pattern is additionally proved end-to-end through +the full validator (prefilter, pattern filter and field check included), and +every exclusion is proved to be about applicability rather than an +unextractable shape: the shared extractor still sees it, and this rule still +reports nothing for it. + +One extractor, one field index, one system-column set, shared with the hook +rule rather than copied — two copies would drift into two different answers +about what a system column is. + +The lint stays off the kernel boot path, and lands one notch tighter than the +hook side: the only applicable pattern is rooted at `ctx.api`, so an action +body that never mentions it does not even parse, let alone load the ~9 MB +TypeScript compiler. Guarded by `lazy-deps.test.ts`. + +`@objectstack/spec`: `ScriptBodySchema` and `ActionSchema.body` now point at +the action-side rule and spell out that `ctx.input` (params) and `ctx.record` +(a discarded snapshot) are not record-write surfaces — doc comments only, no +schema or generated-artifact change. diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index b90a09d129..7f6be7d077 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -271,6 +271,23 @@ export type { ExtractedHookBodyWrite, } from './validate-hook-body-writes.js'; +// The same write-set check on action bodies — same schema, same sandbox, same +// silent no-op. Its ledger is a declared partition of HOOK_BODY_WRITE_PATTERNS +// (only the `ctx.api` family survives the context change), so the two rules +// share one extractor rather than growing two. +export { + validateActionBodyWrites, + ACTION_BODY_WRITE_PATTERNS, + ACTION_BODY_WRITE_PATTERN_IDS, + ACTION_BODY_WRITE_EXCLUSIONS, + ACTION_BODY_WRITE_UNKNOWN_FIELD, +} from './validate-action-body-writes.js'; +export type { + ActionBodyWriteFinding, + ActionBodyWriteSeverity, + ActionBodyWriteExclusion, +} from './validate-action-body-writes.js'; + // One entry point for the reference-resolution rules above (#3583 §5 D5). // Adding a rule to `REFERENCE_INTEGRITY_RULES` runs it on `validate`, `lint` // and `compile` at once — the CLI call sites do not change. diff --git a/packages/lint/src/lazy-deps.test.ts b/packages/lint/src/lazy-deps.test.ts index ad817b1dcb..004faf07a3 100644 --- a/packages/lint/src/lazy-deps.test.ts +++ b/packages/lint/src/lazy-deps.test.ts @@ -74,8 +74,12 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { if (loaded(dep)) fail(dep + ' was loaded eagerly, at import time'); } const jsHook = (source, language) => ({ objects: [{ name: 'a', fields: { amount: {} } }], hooks: [{ name: 'h', object: 'a', events: ['beforeInsert'], body: { language: language ?? 'js', source } }] }); + const jsAction = (source, language) => ({ objects: [{ name: 'a', fields: { amount: {} } }], actions: [{ name: 'act', label: 'Act', objectName: 'a', body: { language: language ?? 'js', source } }] }); mod.validateHookBodyWrites(jsHook('input.x > 0', 'expression')); if (loaded('typescript')) fail('the hook-body write gate on an L1-only stack must not load typescript'); + 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'); const syntax = mod.validateReactPages(${reactStack('function Page(){ return
oops; }')}); if (!loaded('sucrase')) fail('sucrase was not loaded by a react-page syntax validation'); if (loaded('typescript')) fail('the syntax gate must not load typescript'); @@ -83,6 +87,8 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { const hookWrites = mod.validateHookBodyWrites(jsHook('ctx.input.amout = 1;')); if (!loaded('typescript')) fail('typescript was not loaded by an L2 hook-body write validation'); if (!hookWrites.some((f) => f.rule === 'hook-body-write-unknown-field')) fail('hook-body write gate produced no finding'); + const 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 props = mod.validateReactPageProps(${reactStack('function Page(){ return ; }')}); if (!loaded('typescript')) fail('typescript was not loaded by a react-page props validation'); if (!props.some((f) => f.rule === 'react-prop-missing-required')) fail('props gate produced no finding'); @@ -117,7 +123,8 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { it('loads each dep lazily in-process and the gates still work', async () => { const req = createRequire(import.meta.url); - const { validateReactPages, validateReactPageProps, validateHookBodyWrites } = await import('./index.js'); + const { validateReactPages, validateReactPageProps, validateHookBodyWrites, validateActionBodyWrites } = + await import('./index.js'); // Stacks without a react-source page never touch either dep. expect(validateReactPages({ pages: [{ name: 'p', kind: 'object' }] })).toEqual([]); @@ -129,6 +136,17 @@ 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. + 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;' })], + }), + ).toEqual([]); for (const dep of LAZY_DEPS) { expect(depLoaded(req.cache, dep), `${dep} loaded before any react-source or L2-body validation`).toBe(false); } @@ -151,6 +169,15 @@ describe('lazy dependency loading (kernel boot-path contract)', () => { expect(depLoaded(req.cache, 'typescript')).toBe(true); expect(hookWrites.some((f) => f.rule === 'hook-body-write-unknown-field' && f.severity === 'warning')).toBe(true); + // …and the action gate works on the same terms (#4271 follow-up). + const actionWrites = validateActionBodyWrites({ + objects: [{ name: 'a', fields: { amount: {} } }], + actions: [action({ language: 'js', source: "await ctx.api.object('a').update({ amout: 1 });" })], + }); + expect(actionWrites.some((f) => f.rule === 'action-body-write-unknown-field' && 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 e1f79868a1..b2a643b7a8 100644 --- a/packages/lint/src/reference-integrity-suite.test.ts +++ b/packages/lint/src/reference-integrity-suite.test.ts @@ -27,6 +27,7 @@ describe('reference-integrity suite — membership', () => { 'validateAiToolReferences', 'validateAiAgentAuthoring', 'validateHookBodyWrites', + 'validateActionBodyWrites', ]); }); @@ -60,6 +61,18 @@ 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). + { + name: 'score_now', + label: 'Score Now', + objectName: 'crm_lead', + body: { + language: 'js', + source: "await ctx.api.object('crm_lead').update({ lead_score: 100 });", + }, + }, ], views: [ { @@ -171,6 +184,7 @@ describe('reference-integrity suite — every member actually runs', () => { expect(rules).toContain('ai-skill-tool-unresolved'); expect(rules).toContain('agent-authoring-withdrawn'); expect(rules).toContain('hook-body-write-unknown-field'); + expect(rules).toContain('action-body-write-unknown-field'); }); it('carries a gating flow-template finding through the suite (#3810)', () => { @@ -192,9 +206,9 @@ describe('reference-integrity suite — every member actually runs', () => { expect(typeof f.message).toBe('string'); expect(typeof f.hint).toBe('string'); } - // Object references run first, hook-body writes last. + // Object references run first, action-body writes last. expect(findings[0].rule).toBe('object-reference-unknown'); - expect(findings[findings.length - 1].rule).toBe('hook-body-write-unknown-field'); + expect(findings[findings.length - 1].rule).toBe('action-body-write-unknown-field'); }); it('returns nothing for an empty stack', () => { diff --git a/packages/lint/src/reference-integrity-suite.ts b/packages/lint/src/reference-integrity-suite.ts index 6f7d3557f9..6b0c77de4b 100644 --- a/packages/lint/src/reference-integrity-suite.ts +++ b/packages/lint/src/reference-integrity-suite.ts @@ -66,6 +66,7 @@ import { validateAiSurfaceAffinity } from './validate-ai-surface-affinity.js'; import { validateAiToolReferences } from './validate-ai-tool-references.js'; import { validateAiAgentAuthoring } from './validate-ai-agent-authoring.js'; import { validateHookBodyWrites } from './validate-hook-body-writes.js'; +import { validateActionBodyWrites } from './validate-action-body-writes.js'; export type ReferenceIntegritySeverity = 'error' | 'warning'; @@ -118,6 +119,11 @@ export const REFERENCE_INTEGRITY_RULES: readonly ReferenceIntegrityRule[] = [ // read-side membership (#4271). Lazy: only a hook that actually carries a // `language:'js'` body loads the TypeScript parser. { name: 'validateHookBodyWrites', run: validateHookBodyWrites }, + // The same check on the other surface that carries a `HookBodySchema` body: + // 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. + { 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 new file mode 100644 index 0000000000..d335ecd76c --- /dev/null +++ b/packages/lint/src/validate-action-body-writes.test.ts @@ -0,0 +1,324 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { + validateActionBodyWrites, + ACTION_BODY_WRITE_PATTERNS, + ACTION_BODY_WRITE_PATTERN_IDS, + ACTION_BODY_WRITE_EXCLUSIONS, + ACTION_BODY_WRITE_UNKNOWN_FIELD, +} from './validate-action-body-writes.js'; +import { + extractHookBodyWrites, + HOOK_BODY_WRITE_PATTERNS, + BODY_WRITE_SYSTEM_FIELDS, +} from './validate-hook-body-writes.js'; + +// Target objects: array-shaped and map-shaped `fields`, so both authoring +// shapes are resolved. +const dealObject = { + name: 'crm_deal', + fields: [ + { name: 'stage', type: 'text' }, + { name: 'amount', type: 'currency' }, + { name: 'discount_total', type: 'currency' }, + ], +}; +const contactObject = { + name: 'crm_contact', + fields: { + email: { type: 'text' }, + }, +}; + +/** A stack with a single top-level JS-body action (actions[0]) over `source`. */ +function stackWith(source: string, actionOverrides: Record = {}) { + return { + objects: [dealObject, contactObject], + actions: [ + { + name: 'close_deal', + label: 'Close Deal', + objectName: 'crm_deal', + body: { language: 'js', source }, + ...actionOverrides, + }, + ], + }; +} + +describe('ACTION_BODY_WRITE_PATTERNS — ledger partition and reconciliation', () => { + // The two halves are the declared answer to "which of the shared hook + // patterns mean the same thing in an action body?". Their union must BE the + // shared ledger: a fourth pattern landing on the hook side then fails here + // until someone classifies it, instead of being silently assumed either way. + it('partitions the shared hook ledger exactly — no phantom, no unclassified id', () => { + const shared = HOOK_BODY_WRITE_PATTERNS.map((p) => p.id).sort(); + const classified = [ + ...ACTION_BODY_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', () => { + expect(ACTION_BODY_WRITE_PATTERNS.map((p) => p.id)).toEqual([...ACTION_BODY_WRITE_PATTERN_IDS]); + }); + + it('gives every exclusion a non-empty reason', () => { + for (const exclusion of ACTION_BODY_WRITE_EXCLUSIONS) { + expect(exclusion.reason.length, `exclusion '${exclusion.id}' carries no reason`).toBeGreaterThan(0); + } + }); + + /** Objects for each distinct write target in an example, none declaring the written field. */ + const stackForExample = (pattern: (typeof HOOK_BODY_WRITE_PATTERNS)[number]) => ({ + objects: [ + ...new Set(pattern.example.writes.map((w) => w.object).filter((o): o is string => typeof o === 'string')), + ].map((name) => ({ name, fields: [{ name: 'placeholder_only', type: 'text' }] })), + actions: [{ name: 'run_it', label: 'Run', body: { language: 'js', source: pattern.example.source } }], + }); + + for (const pattern of ACTION_BODY_WRITE_PATTERNS) { + // End-to-end, not just extraction: the cheap `api` prefilter, the + // patternId filter and the field check all sit between the ledger example + // and a finding, and any of them could silently drop the pattern. + it(`reports every declared write of '${pattern.id}' through the full validator`, () => { + const expected = new Set( + pattern.example.writes + .filter((w) => w.object !== undefined && !BODY_WRITE_SYSTEM_FIELDS.has(w.field)) + .map((w) => `${w.object} ${w.field}`), + ); + expect(expected.size, `'${pattern.id}' declares no object-addressed write`).toBeGreaterThan(0); + + const findings = validateActionBodyWrites(stackForExample(pattern)); + expect(findings).toHaveLength(expected.size); + for (const finding of findings) { + expect(finding.severity).toBe('warning'); + expect(finding.rule).toBe(ACTION_BODY_WRITE_UNKNOWN_FIELD); + } + }); + } + + 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 + // reports nothing for it. + it(`excludes '${exclusion.id}': extractable by the shared extractor, silent here`, () => { + const pattern = HOOK_BODY_WRITE_PATTERNS.find((p) => p.id === exclusion.id); + expect(pattern, `exclusion '${exclusion.id}' names no shared pattern`).toBeDefined(); + expect(extractHookBodyWrites(pattern!.example.source).length).toBeGreaterThan(0); + // The example body PLUS a clean `ctx.api` call, so the cheap `api` + // prefilter passes and the source is genuinely parsed: what drops these + // writes is the pattern filter, not a short-circuit before it. None of + // the excluded examples' field names exists on crm_deal, so a filter that + // stopped working would warn here. + const findings = validateActionBodyWrites( + stackWith(`${pattern!.example.source} await ctx.api.object('crm_deal').update({ stage: 'won' });`), + ); + expect(findings).toEqual([]); + }); + } +}); + +describe('validateActionBodyWrites — ctx.api writes', () => { + it('warns on a field the named object never declares, with a did-you-mean', () => { + const findings = validateActionBodyWrites( + stackWith("await ctx.api.object('crm_deal').update({ discont_total: 0 });"), + ); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].rule).toBe(ACTION_BODY_WRITE_UNKNOWN_FIELD); + expect(findings[0].where).toBe('action "close_deal" › body'); + expect(findings[0].path).toBe('actions[0].body.source'); + expect(findings[0].message).toContain('discont_total'); + expect(findings[0].message).toContain('crm_deal'); + expect(findings[0].message).toContain('#4271'); + expect(findings[0].hint).toContain("'discount_total'"); + }); + + it('checks insert/create/update payloads (argument 0) and updateById at argument 1', () => { + const findings = validateActionBodyWrites( + stackWith( + "await ctx.api.object('crm_contact').insert({ emial: 'a' }); " + + "await ctx.api.object('crm_contact').create({ email: 'b' }); " + + "await ctx.api.object('crm_deal').updateById(ctx.recordId, { stag: 'won' });", + ), + ); + expect(findings.map((f) => f.message.match(/writing '(\w+)'/)?.[1])).toEqual(['emial', 'stag']); + expect(findings[0].message).toContain("ctx.api.object('crm_contact').insert"); + expect(findings[1].message).toContain('updateById'); + }); + + it('accepts declared fields and system columns', () => { + const findings = validateActionBodyWrites( + stackWith( + "await ctx.api.object('crm_deal').update({ stage: 'won', amount: 1, updated_by: ctx.user.id });", + ), + ); + expect(findings).toEqual([]); + }); + + it('stays silent on dynamic object names, unknown objects, and non-literal payloads', () => { + const findings = validateActionBodyWrites( + stackWith( + 'await ctx.api.object(input.target).update({ nope: 1 }); ' + // dynamic name + "await ctx.api.object('pkg_external').insert({ nope: 1 }); " + // other package + "await ctx.api.object('crm_deal').update(input.payload); " + // opaque payload + "await ctx.api.object('crm_deal').delete({ where: { id: ctx.recordId } });", // not a write-payload method + ), + ); + expect(findings).toEqual([]); + }); + + it('reports each unknown field once per action, even when written repeatedly', () => { + const findings = validateActionBodyWrites( + stackWith( + "await ctx.api.object('crm_deal').update({ totl: 1 }); " + + "await ctx.api.object('crm_deal').insert({ totl: 2 });", + ), + ); + expect(findings).toHaveLength(1); + }); +}); + +describe('validateActionBodyWrites — the params/record surfaces stay 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. + it('ignores ctx.input writes, however they are spelled', () => { + const findings = validateActionBodyWrites( + stackWith( + "ctx.input.not_a_field = 1; ctx.input['also_not'] = 2; ctx.input.count += 1; " + + 'Object.assign(ctx.input, { still_not: 3 });', + ), + ); + 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 touches params and record', () => { + const findings = validateActionBodyWrites( + stackWith( + "ctx.input.whatever = 1; ctx.record.whatever = 2; " + + "await ctx.api.object('crm_deal').update({ stag: 'won' });", + ), + ); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain("'stag'"); + }); +}); + +describe('validateActionBodyWrites — where action bodies live', () => { + it('checks object-embedded actions and reports them at their own path', () => { + const findings = validateActionBodyWrites({ + objects: [ + dealObject, + { + ...contactObject, + actions: [ + { + name: 'sync_contact', + label: 'Sync', + body: { language: 'js', source: "await ctx.api.object('crm_contact').update({ emial: 'x' });" }, + }, + ], + }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].where).toBe('action "sync_contact" › body'); + expect(findings[0].path).toBe('objects[1].actions[0].body.source'); + }); + + it('reports a defineStack-merged action once, at its authored path', () => { + // `mergeObjectActions` appends the action to its object's `actions` while + // preserving the top-level entry, so it is reachable twice. Two structurally + // equal but DISTINCT objects, because that is what the suite actually sees: + // it runs on the schema-parsed stack, and parsing rebuilds every node — an + // identity-based dedupe passes a shared-reference fixture and then reports + // the showcase app's one warning twice. + const action = () => ({ + name: 'close_deal', + label: 'Close Deal', + objectName: 'crm_deal', + body: { language: 'js', source: "await ctx.api.object('crm_deal').update({ stag: 'won' });" }, + }); + const findings = validateActionBodyWrites({ + objects: [{ ...dealObject, actions: [action()] }], + actions: [action()], + }); + expect(findings).toHaveLength(1); + expect(findings[0].path).toBe('actions[0].body.source'); + }); + + it('keeps same-named actions on different objects apart', () => { + // The bound object is part of the dedupe key, so collapsing a merged + // duplicate never swallows a genuinely separate action. + const body = { language: 'js', source: "await ctx.api.object('crm_deal').update({ stag: 'won' });" }; + const findings = validateActionBodyWrites({ + objects: [ + { ...dealObject, actions: [{ name: 'sync', label: 'Sync', body }] }, + { ...contactObject, actions: [{ name: 'sync', label: 'Sync', body }] }, + ], + }); + expect(findings.map((f) => f.path)).toEqual([ + 'objects[0].actions[0].body.source', + 'objects[1].actions[0].body.source', + ]); + }); + + it('accepts map-shaped actions collections (name injected)', () => { + const findings = validateActionBodyWrites({ + objects: [dealObject], + actions: { + close_deal: { + label: 'Close Deal', + body: { language: 'js', source: "await ctx.api.object('crm_deal').update({ stag: 'won' });" }, + }, + }, + }); + expect(findings).toHaveLength(1); + expect(findings[0].where).toBe('action "close_deal" › body'); + }); + + it('checks a body on a non-script action — the runtime binds one regardless of `type`', () => { + const findings = validateActionBodyWrites( + stackWith("await ctx.api.object('crm_deal').update({ stag: 'won' });", { type: 'url', target: '/x' }), + ); + expect(findings).toHaveLength(1); + }); +}); + +describe('validateActionBodyWrites — scope and shape tolerance', () => { + it('ignores actions with no body, L1 expression bodies, and empty sources', () => { + expect( + validateActionBodyWrites({ + objects: [dealObject], + actions: [ + { name: 'a', label: 'A', type: 'flow', target: 'some_flow' }, + { name: 'b', label: 'B', body: { language: 'expression', source: 'input.amount > 0' } }, + { name: 'c', label: 'C', body: { language: 'js', source: ' ' } }, + ], + }), + ).toEqual([]); + }); + + it('returns [] for stacks with no actions at all', () => { + expect(validateActionBodyWrites({})).toEqual([]); + expect(validateActionBodyWrites({ actions: [] })).toEqual([]); + expect(validateActionBodyWrites({ objects: [dealObject] })).toEqual([]); + }); + + it('does not throw on source with syntax errors — fewer matches, never a crash', () => { + expect(() => + validateActionBodyWrites(stackWith("await ctx.api.object('crm_deal').update({ stag: ; if (")), + ).not.toThrow(); + }); +}); diff --git a/packages/lint/src/validate-action-body-writes.ts b/packages/lint/src/validate-action-body-writes.ts new file mode 100644 index 0000000000..5e4621dd62 --- /dev/null +++ b/packages/lint/src/validate-action-body-writes.ts @@ -0,0 +1,286 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Author-time write-set check for L2 (`language:'js'`) ACTION bodies — the +// sibling of `validate-hook-body-writes.ts` (#4271 follow-up). +// +// An action body is the same artefact as a hook body: the same +// `HookBodySchema` union, parsed by the same `HookBodySchema.safeParse` in +// `actionBodyRunnerFactory` (packages/runtime/src/sandbox/body-runner.ts), run +// in the same QuickJS sandbox. So it fails the same way — an action that +// persists a field the target object never declares runs clean, returns +// success to the caller, and the unknown column simply never lands. Same +// silent no-op, same #4001 family; the hook rule alone left half the surface +// uncovered. +// +// ─── What does NOT carry over ─────────────────────────────────────────────── +// +// The context the body receives is NOT the hook context, so the hook ledger +// cannot be adopted wholesale. `buildActionSandboxContext` binds +// `input: unwrapProxyToPlain(actionCtx?.params ?? {})` — an action's +// `ctx.input` is its PARAMS BAG, validated upstream against the action's own +// `params` declaration (ADR-0104 D2), not a record. Resolving those names +// against object fields would flag every correctly-named parameter: a pure +// false-positive machine, and a false positive kills an advisory lint. Hence +// {@link ACTION_BODY_WRITE_EXCLUSIONS} — declared as data, with reasons, and +// 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. +// +// 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. +// +// 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. +// +// 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. + +import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared'; +import { + extractHookBodyWrites, + indexObjectFields, + BODY_WRITE_SYSTEM_FIELDS, + HOOK_BODY_WRITE_PATTERNS, + type HookBodyWritePattern, +} from './validate-hook-body-writes.js'; + +export type ActionBodyWriteSeverity = 'warning'; + +export interface ActionBodyWriteFinding { + /** Advisory-only by contract, exactly like the hook rule — the type says so. */ + severity: ActionBodyWriteSeverity; + rule: string; + /** Human-readable location, e.g. `action "close_deal" › body`. */ + where: string; + /** Config path, e.g. `actions[0].body.source`. */ + path: string; + message: string; + hint: string; +} + +// Rule id (registry entry). +export const ACTION_BODY_WRITE_UNKNOWN_FIELD = 'action-body-write-unknown-field'; + +// ─── 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 +// 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; +} + +/** + * Pattern ids carried over from {@link HOOK_BODY_WRITE_PATTERNS} verbatim. + * + * 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 + * never reasoned about (a false one) — the same asymmetry the extractor's + * silent bails follow. + */ +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[] = [ + { + id: 'input-property-assign', + reason: + "an action's ctx.input is its params bag (`input: unwrapProxyToPlain(actionCtx?.params)`), not a " + + 'record — `ctx.input.` writes a declared PARAMETER, which object fields cannot judge', + }, + { + id: 'input-object-assign', + reason: 'same surface as input-property-assign — Object.assign(ctx.input, …) targets the params bag', + }, +]; + +/** + * The subset of the shared ledger this rule actually sees — the published + * answer to "which writes does the action lint check?". + */ +export const ACTION_BODY_WRITE_PATTERNS: readonly HookBodyWritePattern[] = + HOOK_BODY_WRITE_PATTERNS.filter((p) => ACTION_BODY_WRITE_PATTERN_IDS.includes(p.id)); + +const APPLICABLE_IDS: ReadonlySet = new Set(ACTION_BODY_WRITE_PATTERN_IDS); + +type AnyRec = Record; + +const isRec = (v: unknown): v is AnyRec => !!v && typeof v === 'object' && !Array.isArray(v); + +/** Coerce an array-or-name-keyed-map collection to an array (name injected). */ +function asArray(v: unknown): AnyRec[] { + if (Array.isArray(v)) return v.filter((x): x is AnyRec => isRec(x)); + if (isRec(v)) { + return Object.entries(v).map(([name, def]) => ({ + name, + ...(isRec(def) ? def : {}), + })); + } + return []; +} + +/** One L2 action body found in the stack, with the location to report it at. */ +interface ActionBodySite { + name: string; + source: string; + path: string; +} + +/** The object an action binds to, by the same rule `collectBundleActions` uses. */ +function actionObjectBinding(action: AnyRec, parentObject?: string): string | undefined { + if (typeof action.object === 'string' && action.object) return action.object; + if (typeof action.objectName === 'string' && action.objectName) return action.objectName; + return parentObject; +} + +/** + * Every L2 action body in the stack, from both places the runtime reads them. + * + * `collectBundleActions` (packages/runtime/src/app-plugin.ts) registers + * `bundle.actions` AND `objects[].actions` — and `defineStack`'s + * `mergeObjectActions` appends an action carrying `objectName` to its object's + * array while PRESERVING the top-level entry, so a merged action is genuinely + * reachable twice. Walk both and collapse the duplicate, or every merged + * action's findings are reported twice. + * + * Deduplicated by VALUE — bound object, name and body source — not by object + * identity the way the runtime can afford to. The suite runs on the + * schema-PARSED stack (`validateReferenceIntegrity(result.data)` in `os + * validate` / `os compile`), and parsing rebuilds every node, so the two copies + * of a merged action arrive as distinct objects that are merely equal. An + * identity check silently degrades to no check at all there — which is how the + * showcase app reported its one action-body warning twice. + * + * Two same-named actions on DIFFERENT objects stay separate (the binding is in + * the key). Two on the SAME binding with byte-identical bodies collapse to one + * — they would emit the same sentence twice, so the second is noise. + * + * The top-level entry is walked first, so a merged action reports at + * `actions[i]` — the authored location, not the derived copy. + * + * `type` is deliberately not consulted: the runtime binds a handler from + * `action.body` alone (`actionBodyRunnerFactory` never reads `type`), so a body + * on a non-`script` action still runs and still fails silently. Checking what + * executes beats checking what the schema says should. + */ +function collectActionBodies(stack: AnyRec): ActionBodySite[] { + const sites: ActionBodySite[] = []; + const seen = new Set(); + + const collect = (actions: unknown, pathPrefix: string, parentObject?: string): void => { + asArray(actions).forEach((action, index) => { + const body = action.body; + if (!isRec(body) || body.language !== 'js') return; + const source = body.source; + if (typeof source !== 'string' || source.trim() === '') return; + const name = typeof action.name === 'string' && action.name ? action.name : `#${index}`; + const key = `${actionObjectBinding(action, parentObject) ?? ''}\u0000${name}\u0000${source}`; + if (seen.has(key)) return; + seen.add(key); + sites.push({ name, source, path: `${pathPrefix}[${index}].body.source` }); + }); + }; + + collect(stack.actions, 'actions'); + asArray(stack.objects).forEach((obj, objIndex) => { + const parentObject = typeof obj.name === 'string' && obj.name ? obj.name : undefined; + collect(obj.actions, `objects[${objIndex}].actions`, parentObject); + }); + + return sites; +} + +/** + * Validate L2 action-body writes against target-object field declarations. + * Pure `(stack) => Finding[]` (ADR-0019); safe on pre- or post-parse stacks. + */ +export function validateActionBodyWrites(stack: AnyRec): ActionBodyWriteFinding[] { + const findings: ActionBodyWriteFinding[] = []; + if (!isRec(stack)) return findings; + + const sites = collectActionBodies(stack); + if (sites.length === 0) return findings; + + // Built lazily: a stack whose action bodies never touch `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; + + objectFields ??= indexObjectFields(stack); + const where = `action "${site.name}" › body`; + const reported = new Set(); + + for (const w of writes) { + // Defensive: today every applicable pattern addresses its object + // explicitly. A future applicable pattern that does not (a `ctx.input`- + // shaped one) has no target to resolve against in an action, so it stays + // silent rather than being guessed at the action's `objectName`. + if (w.object === undefined) continue; + + const dedupeKey = `${w.object}\u0000${w.field}`; + if (reported.has(dedupeKey)) continue; + + const known = objectFields.get(w.object); + if (!known) continue; // object declared by another package — cannot judge + if (BODY_WRITE_SYSTEM_FIELDS.has(w.field) || known.has(w.field)) continue; + + reported.add(dedupeKey); + findings.push({ + severity: 'warning', + rule: ACTION_BODY_WRITE_UNKNOWN_FIELD, + where, + path: site.path, + message: + `body calls ctx.api.object('${w.object}').${w.method ?? 'update'}(…) writing '${w.field}', but ` + + `object '${w.object}' declares no such field. The action returns success while the unknown column ` + + `silently never lands (#4271).`, + hint: fixHint(w.field, [...known]), + }); + } + } + + return findings; +} + +/** Did-you-mean (declared + system columns as candidates) plus the fix. */ +function fixHint(field: string, declared: string[]): string { + const suggestion = formatSuggestion(findClosestMatches(field, [...declared, ...BODY_WRITE_SYSTEM_FIELDS])); + 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.` + ); +} diff --git a/packages/lint/src/validate-hook-body-writes.ts b/packages/lint/src/validate-hook-body-writes.ts index 8a8f571d68..c2289fb708 100644 --- a/packages/lint/src/validate-hook-body-writes.ts +++ b/packages/lint/src/validate-hook-body-writes.ts @@ -38,6 +38,13 @@ // test), so it runs on `os validate`, `os lint` and `os compile` at once. // Deliberately NOT in the `defineStack` runtime path: the TypeScript parser // has no place on kernel boot (see the lazy-load contract below). +// +// ACTION bodies run through the same `HookBodySchema` and the same sandbox, so +// they get the same treatment from a sibling rule — `validate-action-body- +// writes.ts`, which reuses this module's extractor, ledger and field index. +// It carries only the pattern subset that survives the context change (an +// action's `ctx.input` is its params bag, not a record); the reasoning is +// declared as data there, not restated here. import { createRequire } from 'node:module'; import type ts from 'typescript'; @@ -180,8 +187,12 @@ const INPUT_ENVELOPE_KEYS: ReadonlySet = new Set(['id', 'options', 'ast' * the sets the other field-resolving rules carry, because the cost asymmetry * is the same everywhere: over-inclusion is at worst a missed finding, * under-inclusion is a false one. + * + * Exported for `validate-action-body-writes.ts` only (not re-exported from the + * package barrel): the two body-write rules must agree on what a system column + * is, and two copies of this list would drift into two different answers. */ -const SYSTEM_FIELDS: ReadonlySet = new Set([ +export const BODY_WRITE_SYSTEM_FIELDS: ReadonlySet = new Set([ 'id', '_id', 'name', 'space', 'owner', 'owner_id', 'user_id', 'created_at', 'created_by', 'updated_at', 'updated_by', @@ -205,8 +216,13 @@ function asArray(v: unknown): AnyRec[] { return []; } -/** object name → its declared field names (both `fields` authoring shapes). */ -function indexObjectFields(stack: AnyRec): Map> { +/** + * object name → its declared field names (both `fields` authoring shapes). + * + * Exported for `validate-action-body-writes.ts` only (see + * {@link BODY_WRITE_SYSTEM_FIELDS} for why the two rules share rather than copy). + */ +export function indexObjectFields(stack: AnyRec): Map> { const out = new Map>(); for (const obj of asArray(stack.objects)) { const name = typeof obj.name === 'string' ? obj.name : undefined; @@ -414,7 +430,7 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] { // missing on EVERY named target (a multi-target body may branch per // object, so a partial miss is not statically wrong). if (!inputJudgeable) continue; - if (SYSTEM_FIELDS.has(w.field)) continue; + if (BODY_WRITE_SYSTEM_FIELDS.has(w.field)) continue; if (targetSets.some((s) => s!.has(w.field))) continue; reported.add(dedupeKey); @@ -438,7 +454,7 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] { // ctx.api write → the named object. const known = objectFields!.get(w.object); if (!known) continue; // object declared by another package — cannot judge - if (SYSTEM_FIELDS.has(w.field) || known.has(w.field)) continue; + if (BODY_WRITE_SYSTEM_FIELDS.has(w.field) || known.has(w.field)) continue; reported.add(dedupeKey); findings.push({ @@ -468,7 +484,7 @@ function unionCandidates(targetSets: ReadonlyArray | undefined>): st /** Did-you-mean (declared + system columns as candidates) plus the fix. */ function fixHint(field: string, declared: string[]): string { - const suggestion = formatSuggestion(findClosestMatches(field, [...declared, ...SYSTEM_FIELDS])); + const suggestion = formatSuggestion(findClosestMatches(field, [...declared, ...BODY_WRITE_SYSTEM_FIELDS])); return ( (suggestion ? `${suggestion} ` : '') + `Fix the field name, or declare '${field}' on the object. Only the literal write patterns in ` + diff --git a/packages/spec/src/data/hook-body.zod.ts b/packages/spec/src/data/hook-body.zod.ts index 133d5cff44..bd09bc798c 100644 --- a/packages/spec/src/data/hook-body.zod.ts +++ b/packages/spec/src/data/hook-body.zod.ts @@ -145,6 +145,13 @@ export type ExpressionBody = z.infer; * still prefer a flow `update_record` node, whose structural `fields` config * is error-checked rather than advisory. * + * 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. + * * @example * ```json * { diff --git a/packages/spec/src/ui/action.zod.ts b/packages/spec/src/ui/action.zod.ts index 34d7fd2582..a622618155 100644 --- a/packages/spec/src/ui/action.zod.ts +++ b/packages/spec/src/ui/action.zod.ts @@ -470,6 +470,13 @@ 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. */ body: HookBodySchema.optional().describe('Action body — expression (L1) or sandboxed JS (L2). Only used when type is `script`.'),