From 22c37718af234ee33df30a86c5ab9c03e2917980 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:36:25 +0800 Subject: [PATCH] feat(lint): flow-node-write-unknown-field covers create_record too (#4271) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4369 shipped the flow write-set gate on `update_record` alone and parked `create_record` in FLOW_WRITE_NODE_TYPES_DEFERRED with its reason — a gating rule earning its severity one measured surface at a time, recorded as data rather than left as silence. This measures the other half and moves it across. The INSERT path fails the same way, one notch harder. Same literal `config.fields` map, same `objectName` binding, same journey to the driver — the engine hands an undeclared key to `driver.create` verbatim, alongside the audit stamps. On SQLite/knex it becomes `table deal has no column named stagee` and the statement is rejected whole, so the correctly named fields in the same payload never land either. The extra harm is what does NOT exist afterwards: the row is never created, so every later node reading `{.id}` from that node's `outputVariable` is working from a record that was never written. An `update_record` failure at least leaves the record intact. The message names that consequence on `create_record` and only there, instead of one sentence blurred to fit both. Nothing else moves: same rule id, same `error` severity, the same silent bails, and `runAs` still not consulted. Each skip is pinned on the create surface as well as the update one, so the two node types cannot drift apart. FLOW_WRITE_NODE_TYPES_DEFERRED is now empty and deliberately kept: the partition test derives the full `fields`-write-map set behaviourally from the spec's executor-written config schemas, so a node type that grows one later belongs to neither list and fails that test until someone classifies it. Deleting the empty array would turn that forced decision back into a default. Two non-members are now excluded on the shape of their failure rather than by omission — `get_record.fields` is a projection (a READ, where an unknown entry narrows the selection instead of breaking the statement) and `screen.defaults` is forwarded into the ScreenSpec the client renders, so an unknown key is a prefill the renderer ignores. That inert case is what this rule's severity is defined against; a test pins that an out-of-ledger node type stays silent. Verified against the repo's own apps: app-crm, app-todo and app-showcase all still validate clean with create_record covered — including crm's convert-lead flow, which creates an account and an opportunity before updating the lead. Co-Authored-By: Claude Opus 5 --- .changeset/flow-create-record-write-lint.md | 49 ++++++++ .../src/validate-flow-node-writes.test.ts | 119 +++++++++++++++++- .../lint/src/validate-flow-node-writes.ts | 73 ++++++----- 3 files changed, 209 insertions(+), 32 deletions(-) create mode 100644 .changeset/flow-create-record-write-lint.md diff --git a/.changeset/flow-create-record-write-lint.md b/.changeset/flow-create-record-write-lint.md new file mode 100644 index 0000000000..ddec38c2ed --- /dev/null +++ b/.changeset/flow-create-record-write-lint.md @@ -0,0 +1,49 @@ +--- +"@objectstack/lint": patch +--- + +feat(lint): `flow-node-write-unknown-field` covers `create_record` too (#4271) + +#4369 shipped the flow write-set gate on `update_record` alone and parked +`create_record` in `FLOW_WRITE_NODE_TYPES_DEFERRED` with its reason — a gating +rule earning its severity one measured surface at a time, recorded as data +rather than left as silence. This measures the other half and moves it across. + +**The INSERT path fails the same way, one notch harder.** Same literal +`config.fields` map, same `objectName` binding, same journey to the driver — the +engine hands an undeclared key to `driver.create` verbatim, alongside the audit +stamps. On SQLite/knex it becomes `table deal has no column named stagee` and +the statement is rejected whole, so the correctly named fields in the same +payload never land either. The extra harm is what does *not* exist afterwards: +the row is never created, so every later node reading `{.id}` from that +node's `outputVariable` is working from a record that was never written. An +`update_record` failure at least leaves the record intact. + +So the message now names that consequence on `create_record` and only there — +"…and the record is never created at all" — instead of one sentence blurred to +fit both. + +Nothing else moves: same rule id, same `error` severity, the same silent bails +(templated `objectName`, non-literal `fields`, cross-package objects, objects +declaring no fields, dotted keys), and `runAs` is still not consulted. Each skip +is now pinned on the create surface as well as the update one, so the two node +types cannot drift into different behaviour. + +**`FLOW_WRITE_NODE_TYPES_DEFERRED` is now empty and deliberately kept.** The +partition test derives the full `fields`-write-map set behaviourally from the +spec's executor-written config schemas, so a node type that grows one later +belongs to neither list and fails that test until someone classifies it. +Deleting the empty array would turn that forced decision back into a default. + +Two non-members are now excluded on the shape of their failure rather than by +omission, both stated in the module header and one pinned by a test: +`get_record.fields` is a projection (`z.array(z.string())`) — a READ, where an +unknown entry narrows the selection instead of breaking the statement — and +`screen.defaults` is forwarded into the `ScreenSpec` the client renders, so an +unknown key is a prefill the renderer ignores. That inert "skips it and renders +the rest" case is exactly what this rule's `error` severity is defined against. + +Verified against the repo's own apps: app-crm, app-todo and app-showcase all +still validate clean with `create_record` covered — including crm's +convert-lead flow, which creates an account and an opportunity before updating +the lead. diff --git a/packages/lint/src/validate-flow-node-writes.test.ts b/packages/lint/src/validate-flow-node-writes.test.ts index 3db6b6db65..d6d89ea4dd 100644 --- a/packages/lint/src/validate-flow-node-writes.test.ts +++ b/packages/lint/src/validate-flow-node-writes.test.ts @@ -97,13 +97,36 @@ describe('FLOW_WRITE_NODE_TYPES — the covered-node ledger', () => { expect(classified).toEqual(withWriteMap); }); + // Every write-map node type is covered as of #4371, so the deferred list is + // empty and the two tests below are vacuous TODAY. Both are kept live because + // they are what makes a FUTURE deferral honest: the partition test above + // forces a new node type into one list or the other, and these decide what + // the uncovered list is allowed to mean. + it('every covered type actually reports — the ledger describes behaviour, not intent', () => { + for (const type of FLOW_WRITE_NODE_TYPES) { + const findings = validateFlowNodeWrites({ + objects: [dealObject], + flows: [ + { + name: 'f', + nodes: [{ id: 'n', type, config: { objectName: 'deal', fields: { stagee: 'won' } } }], + }, + ], + }); + expect(findings.map((f) => f.rule), `covered type '${type}' reports nothing`).toEqual([ + FLOW_NODE_WRITE_UNKNOWN_FIELD, + ]); + expect(findings[0].severity).toBe('error'); + } + }); + 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', () => { + it('leaves every deferred type unchecked', () => { for (const deferral of FLOW_WRITE_NODE_TYPES_DEFERRED) { const findings = validateFlowNodeWrites({ objects: [dealObject], @@ -117,6 +140,26 @@ describe('FLOW_WRITE_NODE_TYPES — the covered-node ledger', () => { expect(findings, `deferred type '${deferral.type}' is being checked`).toEqual([]); } }); + + // A node type NOT in the ledger at all must stay silent — the guard that the + // covered set is an allowlist rather than "anything with a fields key". + it('ignores a fields-bearing node type outside the ledger', () => { + const findings = validateFlowNodeWrites({ + objects: [dealObject], + flows: [ + { + name: 'f', + nodes: [ + // `screen` carries `defaults`/`fields`, but an object-form screen + // forwards them to the client renderer — an unknown key there is an + // ignored prefill, not a write that reaches storage. + { id: 'n', type: 'screen', config: { objectName: 'deal', fields: { stagee: 'won' } } }, + ], + }, + ], + }); + expect(findings).toEqual([]); + }); }); describe('validateFlowNodeWrites', () => { @@ -268,6 +311,80 @@ describe('validateFlowNodeWrites', () => { ); }); + // ── create_record — the same map on the INSERT surface (#4371) ─────── + it('errors when a create_record node writes a field the object never declares', () => { + const flow = { + name: 'seed_deal', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { + id: 'seed', + type: 'create_record', + label: 'Seed deal', + config: { objectName: 'deal', fields: { name: 'ACME', stagee: 'won' }, outputVariable: 'created' }, + }, + ], + }; + const findings = validateFlowNodeWrites({ objects: [dealObject], flows: [flow] }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + expect(findings[0].path).toBe('flows[0].nodes[1].config.fields.stagee'); + expect(findings[0].where).toBe('flow "seed_deal" › node "Seed deal"'); + // The INSERT consequence is strictly worse than the UPDATE one and the + // message says so: the row never exists, so `{created.id}` downstream is + // reading from a record that was never written. + expect(findings[0].message).toContain('the record is never created at all'); + }); + + it('names only the UPDATE consequence for an update_record node', () => { + const findings = validateFlowNodeWrites({ + objects: [dealObject], + flows: [flowWith({ stagee: 'won' })], + }); + expect(findings[0].message).not.toContain('never created at all'); + }); + + it('takes every skip on create_record too', () => { + const createFlow = (config: Record) => ({ + name: 'f', + nodes: [{ id: 'c', type: 'create_record', config }], + }); + // templated object, non-literal fields, unknown object, fieldless object + expect( + validateFlowNodeWrites({ + objects: [dealObject], + flows: [createFlow({ objectName: '{target}', fields: { stagee: 1 } })], + }), + ).toEqual([]); + expect( + validateFlowNodeWrites({ objects: [dealObject], flows: [createFlow({ objectName: 'deal', fields: '{all}' })] }), + ).toEqual([]); + expect( + validateFlowNodeWrites({ objects: [], flows: [createFlow({ objectName: 'deal', fields: { stagee: 1 } })] }), + ).toEqual([]); + expect( + validateFlowNodeWrites({ + objects: [{ name: 'deal', external: true }], + flows: [createFlow({ objectName: 'deal', fields: { stagee: 1 } })], + }), + ).toEqual([]); + }); + + it('does NOT flag a readonly field on create_record — INSERT is engine-exempt from that strip', () => { + // The readonly rule skips create_record entirely (a create may legitimately + // seed readonly columns). This rule asks a different question, so a DECLARED + // readonly field is clean here for its own reason: it resolves to a column. + const withReadonly = { + name: 'deal', + fields: { stage: { type: 'text' }, approval_status: { type: 'text', readonly: true } }, + }; + const findings = validateFlowNodeWrites({ + objects: [withReadonly], + flows: [{ name: 'f', nodes: [{ id: 'c', type: 'create_record', config: { objectName: 'deal', fields: { approval_status: 'approved' } } }] }], + }); + expect(findings).toEqual([]); + }); + // ── the family boundary ────────────────────────────────────────────── it('does not duplicate the readonly rule: a declared readonly field is that rule’s business, not this one', () => { const withReadonly = { diff --git a/packages/lint/src/validate-flow-node-writes.ts b/packages/lint/src/validate-flow-node-writes.ts index 1f2cc3c8ae..1e634bd113 100644 --- a/packages/lint/src/validate-flow-node-writes.ts +++ b/packages/lint/src/validate-flow-node-writes.ts @@ -1,6 +1,6 @@ // 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 +// Author-time write-set check for a flow CRUD node's `fields` write map — 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. // @@ -29,22 +29,26 @@ // 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 +// write paths strip 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 (`m[k] ?? k`). Every branch below was 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', +// • Through the engine, an undeclared key reaches `driver.update` / +// `driver.create` verbatim, alongside the audit stamps. +// • On SQLite/knex an UPDATE 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. +// • An INSERT fails the same way (`table deal has no column named stagee`), +// and one notch harder: the row is never created at all, so every later +// node that expected `{.id}` is working from a record that does not +// exist. // • 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 +// No 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 @@ -52,12 +56,20 @@ // // ─── 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. +// {@link FLOW_WRITE_NODE_TYPES} — every CRUD node type that carries a `fields` +// WRITE map: `update_record` (#4369) and `create_record` (#4371). The deferred +// half, {@link FLOW_WRITE_NODE_TYPES_DEFERRED}, is now empty, and the partition +// test still derives the full set behaviourally from the spec's +// executor-written config schemas — so a node type that grows a write map later +// lands on neither side and fails that test until someone classifies it. +// +// `get_record.fields` is NOT a member and never will be: it is a projection +// (`z.array(z.string())`), a READ, and an unknown entry there narrows the +// selection rather than breaking the statement. `screen.defaults` is not one +// either — an object-form screen forwards it into the `ScreenSpec` the client +// renders, so an unknown key is a form prefill the renderer ignores: inert, the +// "skips it and renders the rest" case this rule's severity is defined against. +// Both are excluded on the shape of their failure, not by omission. // // `runAs` is deliberately NOT consulted, unlike its readonly sibling. A // `runAs:'system'` flow is elevated past the readonly strip, which is why that @@ -101,7 +113,7 @@ export const FLOW_NODE_WRITE_UNKNOWN_FIELD = 'flow-node-write-unknown-field'; // 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']; +export const FLOW_WRITE_NODE_TYPES: readonly string[] = ['update_record', 'create_record']; /** A `fields`-bearing node type this rule does NOT cover yet, and why. */ export interface FlowWriteNodeDeferral { @@ -112,24 +124,21 @@ export interface FlowWriteNodeDeferral { } /** - * `fields`-bearing CRUD node types deliberately left out of v1. + * `fields`-bearing CRUD node types deliberately left uncovered. + * + * **Empty, and that is the point.** #4369 shipped `update_record` alone and + * parked `create_record` here with its reason — a gating rule earning its + * severity one measured surface at a time — rather than leaving the other half + * as silence. #4371 measured the INSERT path (`table deal has no column named + * stagee`, and the row never created at all), found it strictly worse than the + * UPDATE one, and moved it across. * - * `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. + * The slot stays because the partition test derives the full `fields`-write-map + * set from the spec's own config schemas: a node type that grows one later + * belongs to neither list and fails that test until someone puts it in one. + * Deleting this array would turn that forced decision back into a default. */ -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', - }, -]; +export const FLOW_WRITE_NODE_TYPES_DEFERRED: readonly FlowWriteNodeDeferral[] = []; type AnyRec = Record; @@ -231,7 +240,9 @@ export function validateFlowNodeWrites(stack: AnyRec): FlowNodeWriteFinding[] { `${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.`, + `either${ + node.type === 'create_record' ? ' and the record is never created at all' : '' + }; on a schemaless one the stray key is persisted into a column no read surface returns.`, hint: fixHint(fieldName, [...known]), }); }