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