From dc28101bfef4f87c597c658b6203cd8ab20a1a7b Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:13:14 +0800 Subject: [PATCH] feat(lint): a flow update_record node writing an undeclared field gates the build (#4271) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The write-set family #4305 (hooks) and #4344 (actions) opened had a third surface, and it was the one the docs had spent the longest recommending as the safe alternative to the other two. A flow `update_record` node whose `config.fields` names a field the target object never declares was caught by NOTHING: `validate-readonly-flow-writes.ts` walks that exact map and explicitly stepped over the unknown key (`if (!meta) continue; // a form/field-layout lint concern` — a referral to a rule that does not check writes), and `validate-flow-template-paths.ts` checks the `{record.}` READ tokens interpolated into node config, never the write-side key. New rule `flow-node-write-unknown-field`, wired into REFERENCE_INTEGRITY_RULES so `os validate`, `os lint` and `os compile` report it at once — one more path than the hand-wired readonly rule next door reaches. It GATES where its two siblings advise. The hook and action rules are advisory because they PARSE JavaScript: the finding is only as good as the extractor. Nothing here is parsed — `config.fields` is a literal map next to a literal `objectName`, the same certainty `flow-update-readonly-field` already gates on one config key over. A rule that errors on a write the engine strips while only warning on a write that names no column at all would be incoherent in the same map. And the runtime consequence is not the benign "consumer skips the unknown name and renders the rest" that keeps page-field-unknown / form-field-unknown advisory. Both halves were measured: through the engine an undeclared key reaches `driver.update` verbatim, and on SQLite/knex it becomes `no such column: stagee` — the statement is rejected WHOLE, so the correctly named fields in the same payload never land either. On a schemaless datasource the stray key is persisted into a column no schema-driven read returns. One field index and one implicit-field set across all three surfaces: `indexObjectFields` and `IMPLICIT_FIELDS` are imported from the hook rule rather than copied. Every skip is silent and exists so the gate only fires on a certainty — templated `objectName`, non-literal `fields`, cross-package objects, objects declaring no fields at all (external / introspected schemas), dotted keys. `runAs` is deliberately not consulted, unlike the readonly rule: an elevated identity bypasses the readonly strip, but no run identity conjures a column. Scope is declared as data: FLOW_WRITE_NODE_TYPES (today `update_record`) and FLOW_WRITE_NODE_TYPES_DEFERRED (`create_record`, with its reason) are partition-tested against the CRUD node types that carry a `fields` write map, derived behaviourally from the spec's executor-written config schemas. Docs: automation/hook-bodies.mdx and automation/hooks.mdx carried the pre-#4271 framing (write side "not checked", an "accepted gap with no planned closure") long after both body rules landed. Both now state the real asymmetry, with an explicit callout that a clean run is not proof. Co-Authored-By: Claude Opus 5 --- .changeset/flow-node-write-set-lint.md | 85 ++++++ content/docs/automation/hook-bodies.mdx | 2 +- content/docs/automation/hooks.mdx | 7 +- packages/lint/src/index.ts | 17 ++ .../src/reference-integrity-suite.test.ts | 15 +- .../lint/src/reference-integrity-suite.ts | 8 + .../src/validate-flow-node-writes.test.ts | 283 ++++++++++++++++++ .../lint/src/validate-flow-node-writes.ts | 253 ++++++++++++++++ .../src/validate-readonly-flow-writes.test.ts | 5 +- .../lint/src/validate-readonly-flow-writes.ts | 7 +- packages/spec/src/data/hook-body.zod.ts | 5 +- 11 files changed, 677 insertions(+), 10 deletions(-) create mode 100644 .changeset/flow-node-write-set-lint.md create mode 100644 packages/lint/src/validate-flow-node-writes.test.ts create mode 100644 packages/lint/src/validate-flow-node-writes.ts diff --git a/.changeset/flow-node-write-set-lint.md b/.changeset/flow-node-write-set-lint.md new file mode 100644 index 0000000000..37343501b3 --- /dev/null +++ b/.changeset/flow-node-write-set-lint.md @@ -0,0 +1,85 @@ +--- +"@objectstack/lint": minor +"@objectstack/spec": patch +--- + +feat(lint): a flow `update_record` node writing an undeclared field gates the build (#4271) + +The write-set family #4305 (hooks) and #4344 (actions) opened had a third +surface, and it was the one the docs had spent the longest recommending as the +safe alternative to the other two. A flow `update_record` node whose +`config.fields` names a field the target object never declares was caught by +**nothing**: `validate-readonly-flow-writes.ts` walks that exact map and +explicitly stepped over the unknown key (`if (!meta) continue; // a +form/field-layout lint concern` — a referral to a rule that does not check +writes), and `validate-flow-template-paths.ts` checks the `{record.}` +READ tokens interpolated into node config, never the write-side key. So the +surface `hook-bodies.mdx` pointed authors at — "prefer a flow `update_record` +node, whose structural `fields` config is checked" — was the least checked of +the three. + +**New rule — `flow-node-write-unknown-field`, and it is an `error`.** Wired into +`REFERENCE_INTEGRITY_RULES`, so `os validate`, `os lint` and `os compile` report +it at once (one more place than the hand-wired readonly rule next door reaches). + +**Why it gates where its two siblings advise.** The hook and action rules are +advisory because they PARSE JavaScript: the finding is only as good as the +extractor, and a false positive kills an advisory lint. Nothing here is parsed — +`config.fields` is a literal map next to a literal `objectName`, the same +certainty `flow-update-readonly-field` already gates on one config key over. A +rule that errors on a write the engine *strips* while only warning on a write +that names no column at all would be incoherent in the same `fields` map. + +And the runtime consequence is not the benign "consumer skips the unknown name +and renders the rest" that keeps `page-field-unknown` / `form-field-unknown` +advisory. Both halves were measured, not inferred: + +- Through the engine, an undeclared key reaches `driver.update` verbatim — the + flow executor calls the data engine directly, the UPDATE path strips only + readonly/readonlyWhen, and the SQL driver's `formatInput` / + `applyWriteColumnMap` pass an unrecognized key straight through (`m[k] ?? k`). +- On SQLite/knex it becomes `update "deal" set "name" = 'n2', "stagee" = 'won' … + → no such column: stagee`. The statement is rejected **whole**: `name` — + spelled correctly, in the same payload — does not land either, and the step + fails with a driver error naming a column, far from the authoring mistake. +- On a schemaless datasource nothing rejects it, so the stray key is persisted + into a column the object never declares, where no schema-driven read returns + it. + +That is the call `validate-searchable-fields` makes for a stale entry and +`validate-flow-template-paths` makes for a filter-position token: gate when the +miss breaks or corrupts the operation, advise when it merely narrows the output. + +**One field index and one implicit-field set across all three surfaces.** +`indexObjectFields` and `IMPLICIT_FIELDS` are imported from the hook rule rather +than copied, so the three rules cannot drift on what is writable without being +authored — the shape #4330 collapsed one package over. + +Every skip exists so the gate only ever fires on a certainty, and each is +silent: a templated `objectName`, a non-literal `fields` map, an object this +stack does not define, an object that declares no fields at all (external / +datasource-introspected schemas, the same skip `validate-searchable-fields` +takes), and dotted keys (a nested-path write, not a top-level column). `runAs` +is deliberately NOT consulted, unlike the readonly rule that skips +`runAs:'system'` — an elevated identity bypasses the readonly strip, but no run +identity conjures a column. + +**Scope is declared as data, not left as silence.** `FLOW_WRITE_NODE_TYPES` +(today `update_record`) and `FLOW_WRITE_NODE_TYPES_DEFERRED` (`create_record`, +with its reason) are partition-tested against the CRUD node types that carry a +`fields` write map — derived behaviourally from the spec's executor-written +config schemas, not restated — so a node type that grows one later fails that +test until someone classifies it. + +`@objectstack/spec`: `ScriptBodySchema`'s "prefer a flow `update_record` node, +whose structural `fields` config is error-checked" note now names the rule that +makes it true. Doc comment only — no schema or generated-artifact change. + +Docs: #4355 had just rewritten `automation/hook-bodies.mdx` to record this gap +honestly — "**Prefer a flow `update_record` node when the write set is fixed — +but not for *this* check** … writing a field the object never declares is +currently reported by nothing at all. On that one axis an L2 body is now the +better-checked surface." That bullet, and the matching note in +`automation/hooks.mdx`, are the two sentences this change makes false. Both now +say the axis has flipped back — and why the flow side lands a level *stronger* +than the body side rather than merely level with it. diff --git a/content/docs/automation/hook-bodies.mdx b/content/docs/automation/hook-bodies.mdx index 56ba6d02d9..9a6ba155ae 100644 --- a/content/docs/automation/hook-bodies.mdx +++ b/content/docs/automation/hook-bodies.mdx @@ -161,7 +161,7 @@ Because the checking is advisory and literal-only: - **Treat `hook-body-write-unknown-field` as a build failure by convention.** It does not gate, but the rule is tuned for near-zero false positives — in practice a warning is a real typo. - **Check by hand what the parser cannot see.** Computed keys, spreads, aliased input and dynamic object names are invisible to the rule; for an array or `"*"` hook, every field must exist on every target. -- **Prefer a flow `update_record` node when the write set is fixed — but not for *this* check.** A flow node's writes are structured config: they diff field-by-field, render in the Console designer, and a write to a `readonly:true` field is a **gating error** (`flow-update-readonly-field`) that hooks have no counterpart for. What an `update_record` node does *not* get today is a field-existence check — writing a field the object never declares is currently reported by nothing at all. On that one axis an L2 body is now the better-checked surface. +- **Prefer a flow `update_record` node when the write set is fixed — and for *this* check most of all.** A flow node's writes are structured config: they diff field-by-field, render in the Console designer, and a write to a `readonly:true` field is a **gating error** (`flow-update-readonly-field`) that hooks have no counterpart for. Since [#4271](https://github.com/objectstack-ai/objectstack/issues/4271) the field-existence check gates there too — `flow-node-write-unknown-field` is an **error**, not the advisory warning a body gets, because a node's `fields` is a literal map next to a literal `objectName`: there is no parser in between that could have mis-extracted it, so a finding is a certainty rather than a best effort. - **Exercise the hook against a real object before shipping** — on SQL drivers the mistake surfaces on the first write; schemaless drivers won't tell you. ### Signature conventions diff --git a/content/docs/automation/hooks.mdx b/content/docs/automation/hooks.mdx index 8446f53c85..c3e4de45c1 100644 --- a/content/docs/automation/hooks.mdx +++ b/content/docs/automation/hooks.mdx @@ -39,9 +39,10 @@ Two structural reasons to prefer the flow when either could work: hook body's write set is checked too, but only for the literal patterns a parser can recognise and only as an advisory warning (see [Hook & Action Bodies](/docs/automation/hook-bodies#write-set-checking)). - Note the one axis where this reverses: writing a field the target object never - declares warns in a hook body, and is reported nowhere in an `update_record` - node. + Field existence is checked on both surfaces, at different strengths: a hook + body gets a warning, an `update_record` node a **gating error** + (`flow-node-write-unknown-field`) — its `fields` is a literal map next to a + literal `objectName`, so nothing could have mis-read it. - **A flow reviews as data.** The node graph diffs field-by-field and renders in the Console designer with per-node run history; a hook body reviews as code only. diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index eb7f6b9f13..53ecf28b31 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -296,6 +296,23 @@ export type { ActionBodyWriteExclusion, } from './validate-action-body-writes.js'; +// The same write-set question on the third surface — a flow `update_record` +// node's structural `config.fields`. Shares the hook rule's field index and +// implicit-field set so all three agree on what is writable without being +// authored; gates (`error`) rather than advising, because a literal key against +// a literal object name is a certainty the parsed-JS rules cannot claim. +export { + validateFlowNodeWrites, + FLOW_NODE_WRITE_UNKNOWN_FIELD, + FLOW_WRITE_NODE_TYPES, + FLOW_WRITE_NODE_TYPES_DEFERRED, +} from './validate-flow-node-writes.js'; +export type { + FlowNodeWriteFinding, + FlowNodeWriteSeverity, + FlowWriteNodeDeferral, +} from './validate-flow-node-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/reference-integrity-suite.test.ts b/packages/lint/src/reference-integrity-suite.test.ts index 258548f8b6..eb579f6cfc 100644 --- a/packages/lint/src/reference-integrity-suite.test.ts +++ b/packages/lint/src/reference-integrity-suite.test.ts @@ -28,6 +28,7 @@ describe('reference-integrity suite — membership', () => { 'validateAiAgentAuthoring', 'validateHookBodyWrites', 'validateActionBodyWrites', + 'validateFlowNodeWrites', ]); }); @@ -166,6 +167,15 @@ describe('reference-integrity suite — every member actually runs', () => { type: 'get_record', config: { objectName: 'crm_lead', filter: { name: '{record.budget}' } }, }, + // validateFlowNodeWrites: the node's structural write map names a + // field crm_lead does not declare — the third surface in the #4271 + // family, and the only one whose finding is a certainty rather than + // an extraction (so it gates). + { + id: 'stamp', + type: 'update_record', + config: { objectName: 'crm_lead', fields: { lead_score: 100 } }, + }, ], }, ], @@ -191,6 +201,7 @@ describe('reference-integrity suite — every member actually runs', () => { // 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'); + expect(rules).toContain('flow-node-write-unknown-field'); }); it('carries a gating flow-template finding through the suite (#3810)', () => { @@ -212,9 +223,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, action-body writes last. + // Object references run first, flow-node writes last. expect(findings[0].rule).toBe('object-reference-unknown'); - expect(findings[findings.length - 1].rule).toBe('action-body-write-unknown-field'); + expect(findings[findings.length - 1].rule).toBe('flow-node-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 952c2e152e..7c56200342 100644 --- a/packages/lint/src/reference-integrity-suite.ts +++ b/packages/lint/src/reference-integrity-suite.ts @@ -67,6 +67,7 @@ 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'; +import { validateFlowNodeWrites } from './validate-flow-node-writes.js'; export type ReferenceIntegritySeverity = 'error' | 'warning'; @@ -135,6 +136,13 @@ export const REFERENCE_INTEGRITY_RULES: readonly ReferenceIntegrityRule[] = [ // drift this suite exists to end (`validateReadonlyFlowWrites` is the // standing proof — wired into `validate` and `compile`, never into `lint`). { name: 'validateActionBodyWrites', run: validateActionBodyWrites }, + // The third surface that writes a record field set: a flow `update_record` + // node's `config.fields`. Same question as the two rules above, but the map + // is structural metadata rather than parsed JS, so a finding is a certainty + // and gates (`error`) — see that module for why, and why the docs' long- + // standing "prefer a flow node, it's checked" advice was the least true of + // the three until it landed. + { name: 'validateFlowNodeWrites', run: validateFlowNodeWrites }, ]; /** diff --git a/packages/lint/src/validate-flow-node-writes.test.ts b/packages/lint/src/validate-flow-node-writes.test.ts new file mode 100644 index 0000000000..3db6b6db65 --- /dev/null +++ b/packages/lint/src/validate-flow-node-writes.test.ts @@ -0,0 +1,283 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { + CreateRecordConfigSchema, + DeleteRecordConfigSchema, + GetRecordConfigSchema, + UpdateRecordConfigSchema, +} from '@objectstack/spec/automation'; + +import { + validateFlowNodeWrites, + FLOW_NODE_WRITE_UNKNOWN_FIELD, + FLOW_WRITE_NODE_TYPES, + FLOW_WRITE_NODE_TYPES_DEFERRED, +} from './validate-flow-node-writes.js'; +import { IMPLICIT_FIELDS } from './validate-hook-body-writes.js'; + +// Target objects — map-shaped and array-shaped `fields`, so both authoring +// shapes are resolved by the shared index. +const dealObject = { + name: 'deal', + label: 'Deal', + fields: { + stage: { type: 'text' }, + amount: { type: 'currency' }, + notes: { type: 'text' }, + }, +}; + +const leadObject = { + name: 'crm_lead', + fields: [ + { name: 'company', type: 'text' }, + { name: 'converted_account', type: 'lookup' }, + ], +}; + +/** A flow with one `update_record` node at nodes[1]. */ +function flowWith( + fields: unknown, + nodeConfigOverrides: Record = {}, + nodeOverrides: Record = {}, +) { + return { + name: 'close_deal', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { + id: 'mark', + type: 'update_record', + label: 'Mark won', + config: { objectName: 'deal', filter: { id: '{recordId}' }, fields, ...nodeConfigOverrides }, + ...nodeOverrides, + }, + ], + edges: [], + }; +} + +describe('FLOW_WRITE_NODE_TYPES — the covered-node ledger', () => { + // The ledger's two halves are the published answer to "which flow node types + // have their write map checked?". Their union must BE the set of CRUD node + // types that carry one — derived from the spec's executor-written config + // schemas, not restated here, so a node type that grows a `fields` write map + // later fails this test until someone classifies it. + const CRUD_CONFIG_SCHEMAS = { + get_record: GetRecordConfigSchema, + create_record: CreateRecordConfigSchema, + update_record: UpdateRecordConfigSchema, + delete_record: DeleteRecordConfigSchema, + } as const; + + /** + * True when the node type's config declares `fields` as a WRITE MAP. + * Behavioural, not structural: a `{ some_key: value }` probe survives a + * `z.record()` and is refused by `get_record`'s `z.array(z.string())` + * projection or stripped by `delete_record`, which declares no `fields`. + */ + const carriesWriteMap = (schema: { safeParse: (v: unknown) => { success: boolean; data?: unknown } }): boolean => { + const parsed = schema.safeParse({ objectName: 'probe', fields: { probe_key: 1 } }); + if (!parsed.success) return false; + const fields = (parsed.data as Record | undefined)?.fields; + return !!fields && typeof fields === 'object' && !Array.isArray(fields); + }; + + it('partitions the fields-bearing CRUD node types exactly — no phantom, no unclassified type', () => { + const withWriteMap = Object.entries(CRUD_CONFIG_SCHEMAS) + .filter(([, schema]) => carriesWriteMap(schema)) + .map(([type]) => type) + .sort(); + const classified = [ + ...FLOW_WRITE_NODE_TYPES, + ...FLOW_WRITE_NODE_TYPES_DEFERRED.map((d) => d.type), + ].sort(); + expect(classified).toEqual(withWriteMap); + }); + + it('gives every deferral a non-empty reason', () => { + for (const deferral of FLOW_WRITE_NODE_TYPES_DEFERRED) { + expect(deferral.reason.length, `deferral '${deferral.type}' carries no reason`).toBeGreaterThan(0); + } + }); + + it('leaves every deferred type unchecked — the ledger describes behaviour, not intent', () => { + for (const deferral of FLOW_WRITE_NODE_TYPES_DEFERRED) { + const findings = validateFlowNodeWrites({ + objects: [dealObject], + flows: [ + { + name: 'f', + nodes: [{ id: 'n', type: deferral.type, config: { objectName: 'deal', fields: { stagee: 'won' } } }], + }, + ], + }); + expect(findings, `deferred type '${deferral.type}' is being checked`).toEqual([]); + } + }); +}); + +describe('validateFlowNodeWrites', () => { + // ── the gap this rule closes ───────────────────────────────────────── + it('errors when an update_record node writes a field the object never declares', () => { + const findings = validateFlowNodeWrites({ + objects: [dealObject], + flows: [flowWith({ stagee: 'won' })], + }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + expect(findings[0].rule).toBe(FLOW_NODE_WRITE_UNKNOWN_FIELD); + expect(findings[0].path).toBe('flows[0].nodes[1].config.fields.stagee'); + expect(findings[0].where).toBe('flow "close_deal" › node "Mark won"'); + expect(findings[0].message).toContain('stagee'); + expect(findings[0].message).toContain("object 'deal'"); + // Did-you-mean off the object's declared fields. + expect(findings[0].hint).toMatch(/Did you mean (one of: )?'stage'/); + }); + + it('flags every unknown key in one node, and only those', () => { + const findings = validateFlowNodeWrites({ + objects: [dealObject], + flows: [flowWith({ stage: 'won', stagee: 'won', amountt: 1, notes: 'ok' })], + }); + expect(findings.map((f) => f.path)).toEqual([ + 'flows[0].nodes[1].config.fields.stagee', + 'flows[0].nodes[1].config.fields.amountt', + ]); + }); + + it('resolves array-shaped object.fields', () => { + const findings = validateFlowNodeWrites({ + objects: [leadObject], + flows: [flowWith({ comapny: 'ACME' }, { objectName: 'crm_lead' })], + }); + expect(findings).toHaveLength(1); + expect(findings[0].hint).toMatch(/Did you mean (one of: )?'company'/); + }); + + it('resolves the target object via the `object` alias (pre-conversion source)', () => { + const findings = validateFlowNodeWrites({ + objects: [dealObject], + flows: [flowWith({ stagee: 'won' }, { objectName: undefined, object: 'deal' })], + }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_NODE_WRITE_UNKNOWN_FIELD); + }); + + it('checks a runAs:system flow too — no run identity conjures a column', () => { + const flow = { ...flowWith({ stagee: 'won' }), runAs: 'system' }; + const findings = validateFlowNodeWrites({ objects: [dealObject], flows: [flow] }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + }); + + it('checks flows given as a name-keyed map, not only an array', () => { + const findings = validateFlowNodeWrites({ + objects: [dealObject], + flows: { + close_deal: { + nodes: [{ id: 'mark', type: 'update_record', config: { objectName: 'deal', fields: { stagee: 'x' } } }], + }, + }, + }); + expect(findings).toHaveLength(1); + expect(findings[0].where).toBe('flow "close_deal" › node "mark"'); + }); + + // ── clean: writes that resolve ─────────────────────────────────────── + it('does NOT flag declared fields', () => { + const findings = validateFlowNodeWrites({ + objects: [dealObject], + flows: [flowWith({ stage: 'won', amount: 100, notes: 'x' })], + }); + expect(findings).toEqual([]); + }); + + it('does NOT flag implicitly-writable system columns', () => { + // The same set the hook and action rules exempt — the three surfaces agree + // on what is writable without being authored in `fields`. + const fields = Object.fromEntries([...IMPLICIT_FIELDS].map((f) => [f, 'x'])); + const findings = validateFlowNodeWrites({ objects: [dealObject], flows: [flowWith(fields)] }); + expect(findings).toEqual([]); + }); + + // ── silent bails: nothing statically knowable ──────────────────────── + it('skips a templated objectName (target resolved at run time)', () => { + const findings = validateFlowNodeWrites({ + objects: [dealObject], + flows: [flowWith({ stagee: 'won' }, { objectName: '{targetObject}' })], + }); + expect(findings).toEqual([]); + }); + + it('skips a non-literal fields map', () => { + expect(validateFlowNodeWrites({ objects: [dealObject], flows: [flowWith('{allFields}')] })).toEqual([]); + expect(validateFlowNodeWrites({ objects: [dealObject], flows: [flowWith([{ stagee: 1 }])] })).toEqual([]); + expect(validateFlowNodeWrites({ objects: [dealObject], flows: [flowWith(undefined)] })).toEqual([]); + expect(validateFlowNodeWrites({ objects: [dealObject], flows: [flowWith({})] })).toEqual([]); + }); + + it('skips an object this stack does not define (another package)', () => { + const findings = validateFlowNodeWrites({ objects: [], flows: [flowWith({ stagee: 'won' })] }); + expect(findings).toEqual([]); + }); + + it('skips an object that declares no fields (external / introspected schema)', () => { + const findings = validateFlowNodeWrites({ + objects: [{ name: 'deal', label: 'Deal', external: true }], + flows: [flowWith({ stagee: 'won' })], + }); + expect(findings).toEqual([]); + }); + + it('skips a dotted key (a nested-path write, not a top-level column)', () => { + const findings = validateFlowNodeWrites({ + objects: [dealObject], + flows: [flowWith({ 'address.city': 'Beijing' })], + }); + expect(findings).toEqual([]); + }); + + it('skips a node with no config at all', () => { + const flow = { name: 'f', nodes: [{ id: 'n', type: 'update_record' }] }; + expect(validateFlowNodeWrites({ objects: [dealObject], flows: [flow] })).toEqual([]); + }); + + // ── shape robustness ───────────────────────────────────────────────── + it('returns [] for a stack with no flows', () => { + expect(validateFlowNodeWrites({ objects: [dealObject] })).toEqual([]); + expect(validateFlowNodeWrites({})).toEqual([]); + }); + + it('falls back to node id, then index, for the location label', () => { + const byId = { + name: 'f', + nodes: [{ id: 'my_node', type: 'update_record', config: { objectName: 'deal', fields: { stagee: 'x' } } }], + }; + expect(validateFlowNodeWrites({ objects: [dealObject], flows: [byId] })[0].where).toBe( + 'flow "f" › node "my_node"', + ); + + const byIndex = { + nodes: [{ type: 'update_record', config: { objectName: 'deal', fields: { stagee: 'x' } } }], + }; + expect(validateFlowNodeWrites({ objects: [dealObject], flows: [byIndex] })[0].where).toBe( + 'flow "#0" › node "#0"', + ); + }); + + // ── the family boundary ────────────────────────────────────────────── + it('does not duplicate the readonly rule: a declared readonly field is that rule’s business, not this one', () => { + const withReadonly = { + name: 'deal', + fields: { stage: { type: 'text' }, approval_status: { type: 'text', readonly: true } }, + }; + const findings = validateFlowNodeWrites({ + objects: [withReadonly], + flows: [flowWith({ approval_status: 'approved' })], + }); + expect(findings).toEqual([]); + }); +}); diff --git a/packages/lint/src/validate-flow-node-writes.ts b/packages/lint/src/validate-flow-node-writes.ts new file mode 100644 index 0000000000..1f2cc3c8ae --- /dev/null +++ b/packages/lint/src/validate-flow-node-writes.ts @@ -0,0 +1,253 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Author-time write-set check for a flow `update_record` node's `fields` — the +// THIRD surface in the family #4271 opened, and the one the docs spent the +// longest recommending as the safe alternative to the other two. +// +// A flow node that writes a field the target object never declares +// (`config.fields.stagee` against an object whose column is `stage`) was caught +// by nothing. `validate-readonly-flow-writes.ts` walks this exact map and +// explicitly stepped over the unknown key ("a form/field-layout lint concern, +// not this rule's" — a referral to a rule that does not check writes); +// `validate-flow-template-paths.ts` checks the `{record.}` READ tokens +// interpolated into node config, never the write-side key. So the surface the +// hook-body docs pointed authors at — "prefer a flow update_record node, whose +// structural `fields` config is checked" — was the least checked of the three. +// +// ─── Why this one GATES where its two siblings advise ─────────────────────── +// +// `hook-body-write-unknown-field` (#4305) and `action-body-write-unknown-field` +// (#4344) are `warning` because they PARSE JavaScript: the finding is only as +// good as the extractor, and a false positive kills an advisory lint. Nothing +// here is parsed. `config.fields` is a literal, structural map next to a +// literal `objectName` — when this rule speaks, the key provably resolves to no +// column, at the same certainty `flow-update-readonly-field` already gates on +// one config key over. +// +// And the runtime consequence is not the benign "consumer skips the unknown +// name and does the rest" that keeps `page-field-unknown` / `form-field-unknown` +// advisory. Nothing between the node and storage removes the key: the flow +// executor calls the data engine directly (bypassing the metadata-protocol +// ingress, which strips `readonly` — not unknown — keys anyway), the engine's +// UPDATE path strips only readonly/readonlyWhen, and the SQL driver's +// `formatInput` / `applyWriteColumnMap` pass an unrecognized key straight +// through (`m[k] ?? k`). Both halves were measured, not inferred: +// +// • Through the engine, an undeclared key reaches `driver.update` verbatim, +// alongside the audit stamps. +// • On SQLite/knex it then becomes `update "deal" set "name" = 'n2', +// "stagee" = 'won' … → no such column: stagee`. The statement is rejected +// WHOLE: `name` — spelled correctly, in the same payload — does not land +// either, and the step fails with a driver error naming a column, far from +// the authoring mistake. +// • On a schemaless datasource (memory, MongoDB) nothing rejects it, so the +// stray key is persisted into a column the object never declares — where no +// schema-driven read surface will return it. +// +// Neither outcome is "the rest still works". That is the same call +// `validate-searchable-fields` makes for a stale entry and +// `validate-flow-template-paths` makes for a filter-position token: gate when +// the miss breaks or corrupts the operation, advise when it merely narrows the +// output. Every skip below exists so that gate only ever fires on a certainty. +// +// ─── Scope ────────────────────────────────────────────────────────────────── +// +// {@link FLOW_WRITE_NODE_TYPES} — today `update_record` alone — with the +// deliberate non-member (`create_record`) declared as data in +// {@link FLOW_WRITE_NODE_TYPES_DEFERRED} rather than left as silence, and the +// two halves partition-tested against the CRUD node types that carry a `fields` +// write map. A node type that grows one later fails that test until someone +// classifies it. +// +// `runAs` is deliberately NOT consulted, unlike its readonly sibling. A +// `runAs:'system'` flow is elevated past the readonly strip, which is why that +// rule skips it — but no run identity conjures a column, so an unknown field is +// unknown at every privilege level. +// +// Wired via REFERENCE_INTEGRITY_RULES (it resolves a field NAME written in +// metadata against what the stack declares — the suite's exact membership +// test), so `os validate`, `os lint` and `os compile` report it at once. The +// readonly rule next door is still hand-wired into two of those three; this one +// does not repeat that. + +import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared'; + +import { indexObjectFields, IMPLICIT_FIELDS } from './validate-hook-body-writes.js'; + +export type FlowNodeWriteSeverity = 'error'; + +export interface FlowNodeWriteFinding { + /** Always `error` — a literal key against a literal object is a certainty (see module note). */ + severity: FlowNodeWriteSeverity; + rule: string; + /** Human-readable location, e.g. `flow "close_deal" › node "Mark won"`. */ + where: string; + /** Config path, e.g. `flows[0].nodes[3].config.fields.stagee`. */ + path: string; + message: string; + hint: string; +} + +// Rule id (registry entry). +export const FLOW_NODE_WRITE_UNKNOWN_FIELD = 'flow-node-write-unknown-field'; + +// ─── The covered-node ledger ──────────────────────────────────────────────── +// +// Which flow node types have their `config.fields` write map resolved against +// the target object, declared as data — and, next to it, which `fields`-bearing +// node type deliberately does not yet, with its reason. Both halves are +// partition-tested against the CRUD schemas in +// `@objectstack/spec/automation/builtin-node-config`, so a node type that grows +// a write map later cannot land on the uncovered side by nobody noticing. + +/** Flow node types whose `config.fields` keys this rule resolves. */ +export const FLOW_WRITE_NODE_TYPES: readonly string[] = ['update_record']; + +/** A `fields`-bearing node type this rule does NOT cover yet, and why. */ +export interface FlowWriteNodeDeferral { + /** The `FlowNode.type` left uncovered. */ + readonly type: string; + /** Why it is not covered, in terms a reviewer can act on. */ + readonly reason: string; +} + +/** + * `fields`-bearing CRUD node types deliberately left out of v1. + * + * `create_record` fails identically — same literal map, same `objectName` + * binding, same driver fate — and covering it is one entry in + * {@link FLOW_WRITE_NODE_TYPES} plus its fixtures. It is out of scope here only + * because the reported gap (#4001 family, the `update_record` surface the docs + * recommended) is the UPDATE one, and a gating rule earns its severity one + * measured surface at a time. Recorded rather than omitted so the remaining + * half is a decision on the record, not a discovery someone makes twice. + */ +export const FLOW_WRITE_NODE_TYPES_DEFERRED: readonly FlowWriteNodeDeferral[] = [ + { + type: 'create_record', + reason: + "same literal `config.fields` map and same `objectName` binding as update_record, so the check carries " + + 'over verbatim; deferred only to land the gating severity on one surface first', + }, +]; + +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 []; +} + +/** + * The target object of a CRUD node, when statically knowable. Reads the + * canonical `objectName` and its historical `object` alias — a pre-parse source + * may still carry the alias until the 'flow-node-crud-object-alias' conversion + * (#3796) canonicalizes it at load. A templated value (contains `{`) is + * resolved from flow variables at run time, so it is skipped rather than + * guessed. Same read as `validate-readonly-flow-writes.ts`, which walks the + * same nodes for the other question. + */ +function readLiteralObjectName(config: AnyRec): string | undefined { + const raw = config.objectName ?? config.object; + if (typeof raw !== 'string' || raw.includes('{')) return undefined; + return raw || undefined; +} + +const COVERED_TYPES: ReadonlySet = new Set(FLOW_WRITE_NODE_TYPES); + +/** + * Validate flow write-node `fields` keys against the target object's declared + * fields. Pure `(stack) => Finding[]` (ADR-0019); safe on pre- or post-parse + * stacks. + */ +export function validateFlowNodeWrites(stack: AnyRec): FlowNodeWriteFinding[] { + const findings: FlowNodeWriteFinding[] = []; + if (!isRec(stack)) return findings; + + const flows = asArray(stack.flows); + if (flows.length === 0) return findings; + + // Built lazily: a stack whose flows carry no write node never pays it. + let objectFields: Map> | null = null; + + flows.forEach((flow, flowIndex) => { + const flowName = typeof flow.name === 'string' && flow.name ? flow.name : `#${flowIndex}`; + const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; + + nodes.forEach((node, nodeIndex) => { + if (!isRec(node)) return; + if (typeof node.type !== 'string' || !COVERED_TYPES.has(node.type)) return; + + const config = isRec(node.config) ? node.config : undefined; + if (!config) return; + + // A non-literal write map (templated string, spread result, array) is not + // statically knowable — skip rather than guess. + const fields = config.fields; + if (!isRec(fields)) return; + const written = Object.keys(fields); + if (written.length === 0) return; + + const objectName = readLiteralObjectName(config); + if (!objectName) return; // templated / dynamic object — resolved at run time + + objectFields ??= indexObjectFields(stack); + const known = objectFields.get(objectName); + if (!known) return; // object declared by another package — cannot judge its fields + // An object that declares NO fields is an external / datasource-introspected + // schema whose columns are resolved at runtime (the same skip + // validate-searchable-fields takes). Every write to it would otherwise be + // flagged, and this rule gates. + if (known.size === 0) return; + + const nodeName = + typeof node.label === 'string' && node.label + ? node.label + : typeof node.id === 'string' && node.id + ? node.id + : `#${nodeIndex}`; + + for (const fieldName of written) { + if (known.has(fieldName) || IMPLICIT_FIELDS.has(fieldName)) continue; + // A dotted key addresses a nested path, not a top-level column — the + // document drivers forward it verbatim. Not statically a missing field. + if (fieldName.includes('.')) continue; + + findings.push({ + severity: 'error', + rule: FLOW_NODE_WRITE_UNKNOWN_FIELD, + where: `flow "${flowName}" › node "${nodeName}"`, + path: `flows[${flowIndex}].nodes[${nodeIndex}].config.fields.${fieldName}`, + message: + `${node.type} writes '${fieldName}', but object '${objectName}' declares no such field. Nothing ` + + `between the node and storage removes the key: on a SQL datasource the driver rejects the whole ` + + `statement ('no such column'), so the correctly named fields in this same payload never land ` + + `either; on a schemaless one the stray key is persisted into a column no read surface returns.`, + hint: fixHint(fieldName, [...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, ...IMPLICIT_FIELDS])); + return ( + (suggestion ? `${suggestion} ` : '') + + `Fix the field name, or declare '${field}' on the object. This gates the build rather than warning: ` + + `the key is literal and so is the object, so unlike the hook/action body rules there is nothing here ` + + `that could have been mis-extracted.` + ); +} diff --git a/packages/lint/src/validate-readonly-flow-writes.test.ts b/packages/lint/src/validate-readonly-flow-writes.test.ts index 6b3debeb24..5b87218538 100644 --- a/packages/lint/src/validate-readonly-flow-writes.test.ts +++ b/packages/lint/src/validate-readonly-flow-writes.test.ts @@ -215,7 +215,10 @@ describe('validateReadonlyFlowWrites', () => { expect(findings).toEqual([]); }); - it('does NOT flag an unknown field (not this rule’s concern)', () => { + // Unknown fields belong to `flow-node-write-unknown-field` + // (validate-flow-node-writes.ts). Pinned here so the two rules cannot start + // double-reporting the same key. + it('does NOT flag an unknown field (the flow-node write rule owns that)', () => { const findings = validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [flowWith({ nonexistent_field: 'x' }, { runAs: 'user' })], diff --git a/packages/lint/src/validate-readonly-flow-writes.ts b/packages/lint/src/validate-readonly-flow-writes.ts index 24052ddb31..d36d6b1447 100644 --- a/packages/lint/src/validate-readonly-flow-writes.ts +++ b/packages/lint/src/validate-readonly-flow-writes.ts @@ -159,7 +159,12 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind for (const fieldName of Object.keys(fields as AnyRec)) { const meta = fieldMap.get(fieldName); - if (!meta) continue; // unknown field — a form/field-layout lint concern, not this rule's + // Unknown field — `validate-flow-node-writes.ts` owns that question + // (`flow-node-write-unknown-field`, also gating). This rule is about a + // field the object DOES declare and the engine then strips; a name that + // resolves to no column is a different failure with a different fix, so + // the two never double-report the same key. + if (!meta) continue; if (meta.readonly) { findings.push({ diff --git a/packages/spec/src/data/hook-body.zod.ts b/packages/spec/src/data/hook-body.zod.ts index 6b90741455..15918e96d8 100644 --- a/packages/spec/src/data/hook-body.zod.ts +++ b/packages/spec/src/data/hook-body.zod.ts @@ -142,8 +142,9 @@ export type ExpressionBody = z.infer; * computed keys, spreads, aliased `ctx.input`, dynamic object names, * wildcard-target hooks — remain opaque and are skipped silently, so the * warning's absence is not proof of correctness. When the write set is fixed, - * still prefer a flow `update_record` node, whose structural `fields` config - * is error-checked rather than advisory. + * still prefer a flow `update_record` node: `validateFlowNodeWrites` resolves + * its `config.fields` keys with no parser in between, so that surface gates + * (`flow-node-write-unknown-field`, `error`) where this one advises. * * An **action** body carrying this same schema is checked by the sibling rule * `validateActionBodyWrites`, over the subset of that ledger which survives the