Skip to content

Commit 38182ff

Browse files
os-zhuangclaude
andauthored
feat(lint): flow-node-write-unknown-field covers create_record too (#4271) (#4374)
#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 `{<node>.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: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent a4e2684 commit 38182ff

3 files changed

Lines changed: 209 additions & 32 deletions

File tree

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
---
2+
"@objectstack/lint": patch
3+
---
4+
5+
feat(lint): `flow-node-write-unknown-field` covers `create_record` too (#4271)
6+
7+
#4369 shipped the flow write-set gate on `update_record` alone and parked
8+
`create_record` in `FLOW_WRITE_NODE_TYPES_DEFERRED` with its reason — a gating
9+
rule earning its severity one measured surface at a time, recorded as data
10+
rather than left as silence. This measures the other half and moves it across.
11+
12+
**The INSERT path fails the same way, one notch harder.** Same literal
13+
`config.fields` map, same `objectName` binding, same journey to the driver — the
14+
engine hands an undeclared key to `driver.create` verbatim, alongside the audit
15+
stamps. On SQLite/knex it becomes `table deal has no column named stagee` and
16+
the statement is rejected whole, so the correctly named fields in the same
17+
payload never land either. The extra harm is what does *not* exist afterwards:
18+
the row is never created, so every later node reading `{<node>.id}` from that
19+
node's `outputVariable` is working from a record that was never written. An
20+
`update_record` failure at least leaves the record intact.
21+
22+
So the message now names that consequence on `create_record` and only there —
23+
"…and the record is never created at all" — instead of one sentence blurred to
24+
fit both.
25+
26+
Nothing else moves: same rule id, same `error` severity, the same silent bails
27+
(templated `objectName`, non-literal `fields`, cross-package objects, objects
28+
declaring no fields, dotted keys), and `runAs` is still not consulted. Each skip
29+
is now pinned on the create surface as well as the update one, so the two node
30+
types cannot drift into different behaviour.
31+
32+
**`FLOW_WRITE_NODE_TYPES_DEFERRED` is now empty and deliberately kept.** The
33+
partition test derives the full `fields`-write-map set behaviourally from the
34+
spec's executor-written config schemas, so a node type that grows one later
35+
belongs to neither list and fails that test until someone classifies it.
36+
Deleting the empty array would turn that forced decision back into a default.
37+
38+
Two non-members are now excluded on the shape of their failure rather than by
39+
omission, both stated in the module header and one pinned by a test:
40+
`get_record.fields` is a projection (`z.array(z.string())`) — a READ, where an
41+
unknown entry narrows the selection instead of breaking the statement — and
42+
`screen.defaults` is forwarded into the `ScreenSpec` the client renders, so an
43+
unknown key is a prefill the renderer ignores. That inert "skips it and renders
44+
the rest" case is exactly what this rule's `error` severity is defined against.
45+
46+
Verified against the repo's own apps: app-crm, app-todo and app-showcase all
47+
still validate clean with `create_record` covered — including crm's
48+
convert-lead flow, which creates an account and an opportunity before updating
49+
the lead.

packages/lint/src/validate-flow-node-writes.test.ts

Lines changed: 118 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,13 +97,36 @@ describe('FLOW_WRITE_NODE_TYPES — the covered-node ledger', () => {
9797
expect(classified).toEqual(withWriteMap);
9898
});
9999

100+
// Every write-map node type is covered as of #4371, so the deferred list is
101+
// empty and the two tests below are vacuous TODAY. Both are kept live because
102+
// they are what makes a FUTURE deferral honest: the partition test above
103+
// forces a new node type into one list or the other, and these decide what
104+
// the uncovered list is allowed to mean.
105+
it('every covered type actually reports — the ledger describes behaviour, not intent', () => {
106+
for (const type of FLOW_WRITE_NODE_TYPES) {
107+
const findings = validateFlowNodeWrites({
108+
objects: [dealObject],
109+
flows: [
110+
{
111+
name: 'f',
112+
nodes: [{ id: 'n', type, config: { objectName: 'deal', fields: { stagee: 'won' } } }],
113+
},
114+
],
115+
});
116+
expect(findings.map((f) => f.rule), `covered type '${type}' reports nothing`).toEqual([
117+
FLOW_NODE_WRITE_UNKNOWN_FIELD,
118+
]);
119+
expect(findings[0].severity).toBe('error');
120+
}
121+
});
122+
100123
it('gives every deferral a non-empty reason', () => {
101124
for (const deferral of FLOW_WRITE_NODE_TYPES_DEFERRED) {
102125
expect(deferral.reason.length, `deferral '${deferral.type}' carries no reason`).toBeGreaterThan(0);
103126
}
104127
});
105128

106-
it('leaves every deferred type unchecked — the ledger describes behaviour, not intent', () => {
129+
it('leaves every deferred type unchecked', () => {
107130
for (const deferral of FLOW_WRITE_NODE_TYPES_DEFERRED) {
108131
const findings = validateFlowNodeWrites({
109132
objects: [dealObject],
@@ -117,6 +140,26 @@ describe('FLOW_WRITE_NODE_TYPES — the covered-node ledger', () => {
117140
expect(findings, `deferred type '${deferral.type}' is being checked`).toEqual([]);
118141
}
119142
});
143+
144+
// A node type NOT in the ledger at all must stay silent — the guard that the
145+
// covered set is an allowlist rather than "anything with a fields key".
146+
it('ignores a fields-bearing node type outside the ledger', () => {
147+
const findings = validateFlowNodeWrites({
148+
objects: [dealObject],
149+
flows: [
150+
{
151+
name: 'f',
152+
nodes: [
153+
// `screen` carries `defaults`/`fields`, but an object-form screen
154+
// forwards them to the client renderer — an unknown key there is an
155+
// ignored prefill, not a write that reaches storage.
156+
{ id: 'n', type: 'screen', config: { objectName: 'deal', fields: { stagee: 'won' } } },
157+
],
158+
},
159+
],
160+
});
161+
expect(findings).toEqual([]);
162+
});
120163
});
121164

122165
describe('validateFlowNodeWrites', () => {
@@ -268,6 +311,80 @@ describe('validateFlowNodeWrites', () => {
268311
);
269312
});
270313

314+
// ── create_record — the same map on the INSERT surface (#4371) ───────
315+
it('errors when a create_record node writes a field the object never declares', () => {
316+
const flow = {
317+
name: 'seed_deal',
318+
nodes: [
319+
{ id: 'start', type: 'start', config: {} },
320+
{
321+
id: 'seed',
322+
type: 'create_record',
323+
label: 'Seed deal',
324+
config: { objectName: 'deal', fields: { name: 'ACME', stagee: 'won' }, outputVariable: 'created' },
325+
},
326+
],
327+
};
328+
const findings = validateFlowNodeWrites({ objects: [dealObject], flows: [flow] });
329+
expect(findings).toHaveLength(1);
330+
expect(findings[0].severity).toBe('error');
331+
expect(findings[0].path).toBe('flows[0].nodes[1].config.fields.stagee');
332+
expect(findings[0].where).toBe('flow "seed_deal" › node "Seed deal"');
333+
// The INSERT consequence is strictly worse than the UPDATE one and the
334+
// message says so: the row never exists, so `{created.id}` downstream is
335+
// reading from a record that was never written.
336+
expect(findings[0].message).toContain('the record is never created at all');
337+
});
338+
339+
it('names only the UPDATE consequence for an update_record node', () => {
340+
const findings = validateFlowNodeWrites({
341+
objects: [dealObject],
342+
flows: [flowWith({ stagee: 'won' })],
343+
});
344+
expect(findings[0].message).not.toContain('never created at all');
345+
});
346+
347+
it('takes every skip on create_record too', () => {
348+
const createFlow = (config: Record<string, unknown>) => ({
349+
name: 'f',
350+
nodes: [{ id: 'c', type: 'create_record', config }],
351+
});
352+
// templated object, non-literal fields, unknown object, fieldless object
353+
expect(
354+
validateFlowNodeWrites({
355+
objects: [dealObject],
356+
flows: [createFlow({ objectName: '{target}', fields: { stagee: 1 } })],
357+
}),
358+
).toEqual([]);
359+
expect(
360+
validateFlowNodeWrites({ objects: [dealObject], flows: [createFlow({ objectName: 'deal', fields: '{all}' })] }),
361+
).toEqual([]);
362+
expect(
363+
validateFlowNodeWrites({ objects: [], flows: [createFlow({ objectName: 'deal', fields: { stagee: 1 } })] }),
364+
).toEqual([]);
365+
expect(
366+
validateFlowNodeWrites({
367+
objects: [{ name: 'deal', external: true }],
368+
flows: [createFlow({ objectName: 'deal', fields: { stagee: 1 } })],
369+
}),
370+
).toEqual([]);
371+
});
372+
373+
it('does NOT flag a readonly field on create_record — INSERT is engine-exempt from that strip', () => {
374+
// The readonly rule skips create_record entirely (a create may legitimately
375+
// seed readonly columns). This rule asks a different question, so a DECLARED
376+
// readonly field is clean here for its own reason: it resolves to a column.
377+
const withReadonly = {
378+
name: 'deal',
379+
fields: { stage: { type: 'text' }, approval_status: { type: 'text', readonly: true } },
380+
};
381+
const findings = validateFlowNodeWrites({
382+
objects: [withReadonly],
383+
flows: [{ name: 'f', nodes: [{ id: 'c', type: 'create_record', config: { objectName: 'deal', fields: { approval_status: 'approved' } } }] }],
384+
});
385+
expect(findings).toEqual([]);
386+
});
387+
271388
// ── the family boundary ──────────────────────────────────────────────
272389
it('does not duplicate the readonly rule: a declared readonly field is that rule’s business, not this one', () => {
273390
const withReadonly = {

packages/lint/src/validate-flow-node-writes.ts

Lines changed: 42 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
22
//
3-
// Author-time write-set check for a flow `update_record` node's `fields` — the
3+
// Author-time write-set check for a flow CRUD node's `fields` write map — the
44
// THIRD surface in the family #4271 opened, and the one the docs spent the
55
// longest recommending as the safe alternative to the other two.
66
//
@@ -29,35 +29,47 @@
2929
// advisory. Nothing between the node and storage removes the key: the flow
3030
// executor calls the data engine directly (bypassing the metadata-protocol
3131
// ingress, which strips `readonly` — not unknown — keys anyway), the engine's
32-
// UPDATE path strips only readonly/readonlyWhen, and the SQL driver's
32+
// write paths strip only readonly/readonlyWhen, and the SQL driver's
3333
// `formatInput` / `applyWriteColumnMap` pass an unrecognized key straight
34-
// through (`m[k] ?? k`). Both halves were measured, not inferred:
34+
// through (`m[k] ?? k`). Every branch below was measured, not inferred:
3535
//
36-
// • Through the engine, an undeclared key reaches `driver.update` verbatim,
37-
// alongside the audit stamps.
38-
// • On SQLite/knex it then becomes `update "deal" set "name" = 'n2',
36+
// • Through the engine, an undeclared key reaches `driver.update` /
37+
// `driver.create` verbatim, alongside the audit stamps.
38+
// • On SQLite/knex an UPDATE becomes `update "deal" set "name" = 'n2',
3939
// "stagee" = 'won' … → no such column: stagee`. The statement is rejected
4040
// WHOLE: `name` — spelled correctly, in the same payload — does not land
4141
// either, and the step fails with a driver error naming a column, far from
4242
// the authoring mistake.
43+
// • An INSERT fails the same way (`table deal has no column named stagee`),
44+
// and one notch harder: the row is never created at all, so every later
45+
// node that expected `{<node>.id}` is working from a record that does not
46+
// exist.
4347
// • On a schemaless datasource (memory, MongoDB) nothing rejects it, so the
4448
// stray key is persisted into a column the object never declares — where no
4549
// schema-driven read surface will return it.
4650
//
47-
// Neither outcome is "the rest still works". That is the same call
51+
// No outcome is "the rest still works". That is the same call
4852
// `validate-searchable-fields` makes for a stale entry and
4953
// `validate-flow-template-paths` makes for a filter-position token: gate when
5054
// the miss breaks or corrupts the operation, advise when it merely narrows the
5155
// output. Every skip below exists so that gate only ever fires on a certainty.
5256
//
5357
// ─── Scope ──────────────────────────────────────────────────────────────────
5458
//
55-
// {@link FLOW_WRITE_NODE_TYPES} — today `update_record` alone — with the
56-
// deliberate non-member (`create_record`) declared as data in
57-
// {@link FLOW_WRITE_NODE_TYPES_DEFERRED} rather than left as silence, and the
58-
// two halves partition-tested against the CRUD node types that carry a `fields`
59-
// write map. A node type that grows one later fails that test until someone
60-
// classifies it.
59+
// {@link FLOW_WRITE_NODE_TYPES} — every CRUD node type that carries a `fields`
60+
// WRITE map: `update_record` (#4369) and `create_record` (#4371). The deferred
61+
// half, {@link FLOW_WRITE_NODE_TYPES_DEFERRED}, is now empty, and the partition
62+
// test still derives the full set behaviourally from the spec's
63+
// executor-written config schemas — so a node type that grows a write map later
64+
// lands on neither side and fails that test until someone classifies it.
65+
//
66+
// `get_record.fields` is NOT a member and never will be: it is a projection
67+
// (`z.array(z.string())`), a READ, and an unknown entry there narrows the
68+
// selection rather than breaking the statement. `screen.defaults` is not one
69+
// either — an object-form screen forwards it into the `ScreenSpec` the client
70+
// renders, so an unknown key is a form prefill the renderer ignores: inert, the
71+
// "skips it and renders the rest" case this rule's severity is defined against.
72+
// Both are excluded on the shape of their failure, not by omission.
6173
//
6274
// `runAs` is deliberately NOT consulted, unlike its readonly sibling. A
6375
// `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';
101113
// a write map later cannot land on the uncovered side by nobody noticing.
102114

103115
/** Flow node types whose `config.fields` keys this rule resolves. */
104-
export const FLOW_WRITE_NODE_TYPES: readonly string[] = ['update_record'];
116+
export const FLOW_WRITE_NODE_TYPES: readonly string[] = ['update_record', 'create_record'];
105117

106118
/** A `fields`-bearing node type this rule does NOT cover yet, and why. */
107119
export interface FlowWriteNodeDeferral {
@@ -112,24 +124,21 @@ export interface FlowWriteNodeDeferral {
112124
}
113125

114126
/**
115-
* `fields`-bearing CRUD node types deliberately left out of v1.
127+
* `fields`-bearing CRUD node types deliberately left uncovered.
128+
*
129+
* **Empty, and that is the point.** #4369 shipped `update_record` alone and
130+
* parked `create_record` here with its reason — a gating rule earning its
131+
* severity one measured surface at a time — rather than leaving the other half
132+
* as silence. #4371 measured the INSERT path (`table deal has no column named
133+
* stagee`, and the row never created at all), found it strictly worse than the
134+
* UPDATE one, and moved it across.
116135
*
117-
* `create_record` fails identically — same literal map, same `objectName`
118-
* binding, same driver fate — and covering it is one entry in
119-
* {@link FLOW_WRITE_NODE_TYPES} plus its fixtures. It is out of scope here only
120-
* because the reported gap (#4001 family, the `update_record` surface the docs
121-
* recommended) is the UPDATE one, and a gating rule earns its severity one
122-
* measured surface at a time. Recorded rather than omitted so the remaining
123-
* half is a decision on the record, not a discovery someone makes twice.
136+
* The slot stays because the partition test derives the full `fields`-write-map
137+
* set from the spec's own config schemas: a node type that grows one later
138+
* belongs to neither list and fails that test until someone puts it in one.
139+
* Deleting this array would turn that forced decision back into a default.
124140
*/
125-
export const FLOW_WRITE_NODE_TYPES_DEFERRED: readonly FlowWriteNodeDeferral[] = [
126-
{
127-
type: 'create_record',
128-
reason:
129-
"same literal `config.fields` map and same `objectName` binding as update_record, so the check carries " +
130-
'over verbatim; deferred only to land the gating severity on one surface first',
131-
},
132-
];
141+
export const FLOW_WRITE_NODE_TYPES_DEFERRED: readonly FlowWriteNodeDeferral[] = [];
133142

134143
type AnyRec = Record<string, unknown>;
135144

@@ -231,7 +240,9 @@ export function validateFlowNodeWrites(stack: AnyRec): FlowNodeWriteFinding[] {
231240
`${node.type} writes '${fieldName}', but object '${objectName}' declares no such field. Nothing ` +
232241
`between the node and storage removes the key: on a SQL datasource the driver rejects the whole ` +
233242
`statement ('no such column'), so the correctly named fields in this same payload never land ` +
234-
`either; on a schemaless one the stray key is persisted into a column no read surface returns.`,
243+
`either${
244+
node.type === 'create_record' ? ' and the record is never created at all' : ''
245+
}; on a schemaless one the stray key is persisted into a column no read surface returns.`,
235246
hint: fixHint(fieldName, [...known]),
236247
});
237248
}

0 commit comments

Comments
 (0)