From afacc31b4632fcb7c44934c480829ae5eefe140a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 14:34:30 +0000 Subject: [PATCH 1/4] feat(lint): the null-guard gate judges a record-triggered flow's start, edge and decision conditions A record-triggered flow's record and previous are total over the trigger object's declared fields, so an ordering or arithmetic comparison on a nullable field faults on the first empty value and the run fails there. The gate now covers the start condition, every edge condition and every decision branch expression of such a flow, judges bare flattened field names as record. when the flow's bound set is provable and no variable or node id holds the name, and presumes what the start condition proves at every condition downstream. A count or sum roll-up summary is always valued (zero children is zero) and no longer counts as nullable. Claude-Session: https://claude.ai/code/session_01JQwxbZfKtKX5FguJoubPfK Co-authored-by: Claude --- .changeset/29026-flow-condition-null-guard.md | 15 + ...idate-expressions.flow-null-guards.test.ts | 203 ++++++++++++++ .../lint/src/validate-expressions.test.ts | 74 +++-- packages/lint/src/validate-expressions.ts | 126 ++++++--- .../lint/src/validate-null-guards.test.ts | 69 +++++ packages/lint/src/validate-null-guards.ts | 264 ++++++++++++++---- 6 files changed, 630 insertions(+), 121 deletions(-) create mode 100644 .changeset/29026-flow-condition-null-guard.md create mode 100644 packages/lint/src/validate-expressions.flow-null-guards.test.ts diff --git a/.changeset/29026-flow-condition-null-guard.md b/.changeset/29026-flow-condition-null-guard.md new file mode 100644 index 00000000000..1a71cd8d7b3 --- /dev/null +++ b/.changeset/29026-flow-condition-null-guard.md @@ -0,0 +1,15 @@ +--- +'@objectstack/lint': minor +--- + +feat(lint): the publish-time null-guard gate judges a record-triggered flow's start, edge and decision-branch conditions + +Clause-②: no +Metadata-protocol: narrowed + +`os build`, `os validate`, `os lint` and the runtime publish gate now refuse a record-triggered flow (start `triggerType: 'record-…'`) whose start condition, edge `condition` or `decision` branch `expression` applies an ordering (`< <= > >=`) or arithmetic (`+ - * / %`) operator to a nullable declared field of the trigger object with no `!= null` / `== null` / `!isBlank()` test in front of it. The run's `record` and `previous` carry every declared field, nulls included, so such a comparison faults with `no such overload` on the first record where the field is empty, and the run fails there (the write that fired it still lands). + +- **Who moves.** A flow condition such as `budget > 200000` on an optional `budget`. Fix: `budget != null && budget > 200000`, or put the `!= null` test in the flow's start condition, which gates every run, so every edge and branch after it may compare freely. `has(record.budget)` is not a guard: the key is always present. +- **Bare names count.** The run spreads the record's fields to top level, so a bare `budget` is read as `record.budget`, unless the flow declares a variable or a node id of that name, or has a node that can bind names nobody declared (a `script`, `connector_action`, `wait`, `subflow` or `map`); those bare names are not judged. At the runtime publish gate bare names are not judged. +- **Not judged.** Flows with no record trigger (an `api` flow's request body, an action's or `map` item's record are not made total), a `screen` field's `visibleWhen`, and `config.condition` on any node but `start`. +- **Fewer findings elsewhere.** A `count` or `sum` roll-up summary is now always valued for this gate (zero children is zero), on validation rules and hook conditions too; `min` / `max` / `avg` roll-ups stay nullable. diff --git a/packages/lint/src/validate-expressions.flow-null-guards.test.ts b/packages/lint/src/validate-expressions.flow-null-guards.test.ts new file mode 100644 index 00000000000..b8f41a9e182 --- /dev/null +++ b/packages/lint/src/validate-expressions.flow-null-guards.test.ts @@ -0,0 +1,203 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #29026 — the null-guard gate on a record-triggered flow's start, edge and +// `decision` branch conditions. `validate-null-guards.test.ts` pins the +// decision procedure; this file pins the wiring: which flows, which slots, +// which bare names, and what the start condition proves downstream. The +// ledger row in `validate-null-guards.ts` carries the runtime measurements. + +import { describe, it, expect } from 'vitest'; + +import { validateStackExpressions } from './validate-expressions.js'; + +// `budget` and `owner` are optional, as on `showcase_project`; `name` is +// required and `spent` has a default, so neither may ever be flagged. +const project = { + name: 'showcase_project', + fields: { + name: { type: 'text', required: true }, + owner: { type: 'text' }, + health: { type: 'select', options: [{ value: 'green', default: true }, { value: 'red' }] }, + budget: { type: 'currency' }, + spent: { type: 'currency', defaultValue: 0 }, + }, +}; + +type Rec = Record; + +const start = (config: Rec = {}): Rec => ({ + id: 'start', + type: 'start', + config: { objectName: 'showcase_project', triggerType: 'record-after-update', ...config }, +}); + +const flow = (overrides: Rec): Rec => ({ + name: 'escalate', + label: 'Escalate', + type: 'autolaunched', + nodes: [start(), { id: 'triage', type: 'decision' }, { id: 'alert', type: 'end' }, { id: 'calm', type: 'end' }], + edges: [ + { id: 'e1', source: 'start', target: 'triage' }, + { id: 'e2', source: 'triage', target: 'alert', condition: 'budget > 200000' }, + { id: 'e3', source: 'triage', target: 'calm', isDefault: true }, + ], + ...overrides, +}); + +/** The null-guard findings alone — other passes judge other things about these flows. */ +const nullGuardIssues = (flows: Rec[]) => + validateStackExpressions({ objects: [project], flows }).filter((i) => i.message.includes('declares as nullable')); + +describe('flow conditions under the null-guard gate (#29026)', () => { + it('rejects the bare unguarded comparison that failed the showcase escalation run', () => { + const issues = nullGuardIssues([flow({})]); + expect(issues).toHaveLength(1); + const [issue] = issues; + expect(issue!.severity).toBe('error'); + expect(issue!.where).toContain("edge 'e2'"); + expect(issue!.path).toBe('flows[0].edges[1].condition'); + expect(issue!.message).toContain('`budget`'); + expect(issue!.message).toContain('the flow run fails at this condition'); + expect(issue!.message).toContain("Guard it with '!= null'"); + }); + + it('rejects the `record.` spelling too, and accepts either guarded spelling', () => { + const edges = (condition: string) => [ + { id: 'e1', source: 'start', target: 'triage' }, + { id: 'e2', source: 'triage', target: 'alert', condition }, + { id: 'e3', source: 'triage', target: 'calm', isDefault: true }, + ]; + expect(nullGuardIssues([flow({ edges: edges('record.budget > 200000') })])).toHaveLength(1); + expect(nullGuardIssues([flow({ edges: edges('record.budget != null && record.budget > 200000') })])).toEqual([]); + expect(nullGuardIssues([flow({ edges: edges('budget != null && budget > 200000') })])).toEqual([]); + // `has()` is true on the materialized null, so it is no guard here either. + const hasOnly = nullGuardIssues([flow({ edges: edges('has(record.budget) && record.budget > 200000') })]); + expect(hasOnly).toHaveLength(1); + expect(hasOnly[0]!.message).toContain('`has(record.budget)` does not guard it'); + }); + + it('judges the start condition, and presumes what it proves at every edge downstream', () => { + const unguardedStart = nullGuardIssues([flow({ + nodes: [start({ condition: 'health == "red" && budget > 100000' }), { id: 'triage', type: 'decision' }, { id: 'alert', type: 'end' }, { id: 'calm', type: 'end' }], + edges: [{ id: 'e1', source: 'start', target: 'triage' }], + })]); + expect(unguardedStart).toHaveLength(1); + expect(unguardedStart[0]!.where).toContain("node 'start' (start) condition"); + expect(unguardedStart[0]!.path).toBe('flows[0].nodes[0].config.condition'); + + // The start condition gates every run, so its guard holds at e2. + expect(nullGuardIssues([flow({ + nodes: [start({ condition: 'budget != null && budget > 100000' }), { id: 'triage', type: 'decision' }, { id: 'alert', type: 'end' }, { id: 'calm', type: 'end' }], + })])).toEqual([]); + }); + + it('judges a `decision` branch expression', () => { + const issues = nullGuardIssues([flow({ + nodes: [ + start(), + { id: 'triage', type: 'decision', config: { conditions: [{ label: 'critical', expression: 'record.budget > 200000' }, { label: 'normal', expression: 'true' }] } }, + { id: 'alert', type: 'end' }, + { id: 'calm', type: 'end' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'triage' }], + })]); + expect(issues).toHaveLength(1); + expect(issues[0]!.where).toContain('conditions[0].expression'); + }); + + it('judges an edge inside a container region', () => { + const issues = nullGuardIssues([flow({ + nodes: [ + start(), + { + id: 'fan', + type: 'parallel', + config: { + branches: [{ + name: 'Owner', + nodes: [{ id: 'gate', type: 'decision' }, { id: 'ping', type: 'end' }], + edges: [{ id: 'b1', source: 'gate', target: 'ping', condition: 'record.budget * 2 > 10' }], + }], + }, + }, + ], + edges: [{ id: 'e1', source: 'start', target: 'fan' }], + })]); + expect(issues).toHaveLength(1); + expect(issues[0]!.where).toContain("edge 'b1'"); + }); + + it('leaves a flow with no record trigger alone: nothing makes its record total', () => { + const noTrigger = flow({ nodes: [{ id: 'start', type: 'start', config: { objectName: 'showcase_project' } }, { id: 'triage', type: 'decision' }, { id: 'alert', type: 'end' }, { id: 'calm', type: 'end' }] }); + expect(nullGuardIssues([noTrigger])).toEqual([]); + const api = flow({ type: 'api', nodes: [{ id: 'start', type: 'start', config: { objectName: 'showcase_project', triggerType: 'api' } }, { id: 'triage', type: 'decision' }, { id: 'alert', type: 'end' }, { id: 'calm', type: 'end' }] }); + expect(nullGuardIssues([api])).toEqual([]); + }); + + it('leaves a bare name a declared variable holds alone — the variable wins — and still judges `record.`', () => { + const variables = [{ name: 'budget', type: 'number', defaultValue: 5 }]; + expect(nullGuardIssues([flow({ variables })])).toEqual([]); + const issues = nullGuardIssues([flow({ + variables, + edges: [ + { id: 'e1', source: 'start', target: 'triage' }, + { id: 'e2', source: 'triage', target: 'alert', condition: 'record.budget > 200000' }, + { id: 'e3', source: 'triage', target: 'calm', isDefault: true }, + ], + })]); + expect(issues).toHaveLength(1); + }); + + it('leaves bare names alone on a flow whose nodes can bind names nobody declared', () => { + const withScript = (condition: string) => flow({ + nodes: [start(), { id: 'calc', type: 'script', config: { function: 'calc' } }, { id: 'triage', type: 'decision' }, { id: 'alert', type: 'end' }, { id: 'calm', type: 'end' }], + edges: [ + { id: 'e0', source: 'start', target: 'calc' }, + { id: 'e1', source: 'calc', target: 'triage' }, + { id: 'e2', source: 'triage', target: 'alert', condition }, + { id: 'e3', source: 'triage', target: 'calm', isDefault: true }, + ], + }); + expect(nullGuardIssues([withScript('budget > 200000')])).toEqual([]); + expect(nullGuardIssues([withScript('record.budget > 200000')])).toHaveLength(1); + }); + + it('does not judge an inert `config.condition` on a non-start node', () => { + expect(nullGuardIssues([flow({ + nodes: [start(), { id: 'triage', type: 'decision', config: { condition: 'record.budget > 1' } }, { id: 'alert', type: 'end' }, { id: 'calm', type: 'end' }], + edges: [{ id: 'e1', source: 'start', target: 'triage' }], + })])).toEqual([]); + }); + + it('treats a `count` / `sum` roll-up as always valued, and a `max` one as nullable', () => { + // Zero children is zero for `count` / `sum` (objectql's + // `summaryEmptySetValue`); `min` / `max` / `avg` over no rows is `null`. + const report = { + name: 'expense_report', + fields: { + status: { type: 'select', options: [{ value: 'draft', default: true }, { value: 'submitted' }] }, + total_amount: { type: 'summary', summaryOperations: { object: 'expense_line', field: 'amount', function: 'sum' } }, + line_count: { type: 'summary', summaryOperations: { object: 'expense_line', function: 'count' } }, + largest_line: { type: 'summary', summaryOperations: { object: 'expense_line', field: 'amount', function: 'max' } }, + }, + }; + const quorum = (condition: string): Rec => ({ + name: 'quorum', + label: 'Quorum', + type: 'autolaunched', + nodes: [{ id: 'start', type: 'start', config: { objectName: 'expense_report', triggerType: 'record-after-update', condition } }, { id: 'end', type: 'end' }], + edges: [{ id: 'e1', source: 'start', target: 'end' }], + }); + const judge = (condition: string) => + validateStackExpressions({ objects: [report], flows: [quorum(condition)] }) + .filter((i) => i.message.includes('declares as nullable')); + expect(judge('status == "submitted" && total_amount >= 5000 && line_count > 1')).toEqual([]); + expect(judge('largest_line > 1000').map((i) => i.message)).toEqual([expect.stringContaining('`largest_line`')]); + }); + + it('never flags a non-nullable field', () => { + expect(nullGuardIssues([flow({ + edges: [{ id: 'e1', source: 'start', target: 'triage' }, { id: 'e2', source: 'triage', target: 'alert', condition: 'spent > 5 && size(name) > 1' }], + })])).toEqual([]); + }); +}); diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index 4deb11c026f..2372f244283 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -37,8 +37,11 @@ import type { ExprIssue } from './validate-expressions.js'; import { validateSemanticRoles, SEMANTIC_ROLE_FIELD_UNPROVISIONED } from './validate-semantic-roles.js'; describe('validateStackExpressions (ADR-0032 build-time)', () => { + // [#29026] `rating` is required: a record-triggered flow comparing an + // optional field with no `!= null` in front is refused by the null-guard + // gate, and this suite's flows compare `rating` to pin other verdicts. const objects = [ - { name: 'crm_lead', fields: { rating: { type: 'number' }, status: { type: 'text' } } }, + { name: 'crm_lead', fields: { rating: { type: 'number', required: true }, status: { type: 'text' } } }, ]; it('passes a clean stack', () => { @@ -3207,28 +3210,30 @@ describe('null-guard gate (#4763)', () => { ).toHaveLength(0); }); - // #4811 re-measured the reason this one is excluded. It is NOT the - // flattened scope (this gate never resolves a bare identifier — only - // `record.`/`previous.`, and a record-triggered run binds both - // roots — #22677). It is that `record-change-trigger.ts` seeds the flow's - // record as `{ ...(inputData ?? {}), ...after }` (spelled `inputDoc` until - // #5671 dropped that alias read) with no `materializeDeclaredFields`, - // so a declared column the write never mentioned is an ABSENT key — and on - // an absent key the `!= null` this gate prescribes faults exactly like the - // comparison it was meant to guard. - it('leaves flow conditions alone (trigger record is not total over declared fields)', () => { + // RE-JUDGED IN PLACE (#29026). This pinned `toHaveLength(0)` while the + // record-change trigger seeded the flow's record sparse; since #4953 it + // materializes every declared field, so a record-triggered flow's edge is + // covered and `validate-expressions.flow-null-guards.test.ts` pins the + // wiring. What stays out is pinned here: a non-start node's + // `config.condition` (inert — nothing evaluates it), and a flow with no + // record trigger, whose record nothing makes total. + it('judges a record-triggered flow edge, and leaves an inert node condition and an untriggered flow alone', () => { + const flow = (triggerType?: string) => ({ + name: 'escalate', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'showcase_project', ...(triggerType ? { triggerType } : {}) } }, + { id: 'd', type: 'decision', config: { condition: 'record.budget > 100000' } }, + ], + edges: [{ id: 'e1', source: 'd', target: 'end', condition: 'record.spent > record.budget' }], + }); + const triggered = validateStackExpressions({ objects: [project], flows: [flow('record-after-update')] }); + expect(triggered.map((i) => i.where)).toEqual([expect.stringContaining("edge 'e1'")]); + expect(triggered[0]!.message).toContain('`record.budget`'); + // (With no trigger the run binds no `record` at all, which the flow CEL + // root judge reports; that is its verdict, not this gate's.) expect( - validateStackExpressions({ - objects: [project], - flows: [{ - name: 'escalate', - nodes: [ - { id: 'start', type: 'start', config: { objectName: 'showcase_project', triggerType: 'record-after-update' } }, - { id: 'd', type: 'decision', config: { condition: 'record.budget > 100000' } }, - ], - edges: [{ id: 'e1', source: 'd', target: 'end', condition: 'record.spent > record.budget' }], - }], - }), + validateStackExpressions({ objects: [project], flows: [flow()] }) + .filter((i) => i.message.includes('declares as nullable')), ).toHaveLength(0); }); @@ -3435,10 +3440,20 @@ const READ_SURFACES: Array<{ receiver: string; expected: string[]; declaredBy: s { receiver: 'def', // `referenceTo` is absent — likewise. - expected: ['defaultValue', 'options', 'reference', 'required', 'type'], + // [#29026] `summaryOperations` joined: a `count` / `sum` roll-up is + // always valued for the null-guard gate, and its function is read below. + expected: ['defaultValue', 'options', 'reference', 'required', 'summaryOperations', 'type'], declaredBy: 'FieldSchema', keys: () => Object.keys(FieldSchema.shape), }, + { + // [#29026] The roll-up's aggregate function — the null-guard gate's + // nullability judgment reads it and nothing else here. + receiver: 'summaryOperations', + expected: ['function'], + declaredBy: 'FieldSchema.summaryOperations', + keys: () => shapeKeysOf(FieldSchema.shape.summaryOperations), + }, { // [#24798] One `bulkActionDefs[]` entry: its `confirmText` template slot, // and its `name` for the location (its params are read by the spec's @@ -3712,6 +3727,11 @@ describe('validateStackExpressions — reads only keys the spec declares (meta-t // node walk's own sites. Its keys are `Map.prototype.set` / `get`, never // metadata keys; the config it describes is read off the tabled `cfg`. 'nodeSlotKeys', + // [#29026] The flow null-guard wiring: the flow CEL root judge's own + // result (`provable`), the flow's declared-name set and the gate's + // bare-name set. Their keys are that module's result key and Set + // methods, never metadata keys. + 'rootScope', 'declaredVariables', 'nullGuardBare', ]); expect(receivers.filter((r) => !tabled.has(r) && !PLUMBING.has(r))).toEqual([]); }); @@ -4618,8 +4638,10 @@ describe('assignment value envelope — located findings (#15137)', () => { * controls below are those two shapes staying green. */ describe('structural condition shape (#15662)', () => { + // [#29026] `rating` is required so the record-triggered null-guard gate + // stays out of the shape verdicts this suite pins (its controls compare it). const objects = [ - { name: 'crm_lead', fields: { rating: { type: 'number' }, status: { type: 'text' } } }, + { name: 'crm_lead', fields: { rating: { type: 'number', required: true }, status: { type: 'text' } } }, ]; const flowWith = (opts: { startCondition?: unknown; decisionCondition?: unknown; edgeCondition?: unknown }) => ({ @@ -4785,8 +4807,10 @@ describe('structural condition shape (#15662)', () => { * one shared refusal to prevent. */ describe('blank structural condition (#17495)', () => { + // [#29026] `rating` is required so the record-triggered null-guard gate + // stays out of the shape verdicts this suite pins (its controls compare it). const objects = [ - { name: 'crm_lead', fields: { rating: { type: 'number' }, status: { type: 'text' } } }, + { name: 'crm_lead', fields: { rating: { type: 'number', required: true }, status: { type: 'text' } } }, ]; const flowWith = (opts: { startCondition?: unknown; decisionCondition?: unknown; edgeCondition?: unknown }) => ({ diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index d802beb5bee..f17346aa123 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -109,6 +109,7 @@ import { structuralConditionRefusal, closedScopeTemplateRefusal, templateSlotSource, + resolveFlowTriggerKind, } from '@objectstack/spec/automation'; // [#24798] A bulk action's `confirmText` is a `template` slot over the closed // scope the spec declares for it — judged below by the same template judge. @@ -150,8 +151,8 @@ import { } from './flow-cel-root-scope.js'; import type { FlowCelEntrance } from './flow-cel-root-scope.js'; import { injectedColumnDefsFor, injectedColumnsFor, unprovisionedInjectedColumnsFor } from './system-fields.js'; -import { findUnguardedNullableOperands, nullGuardMessage } from './validate-null-guards.js'; -import type { NullGuardOutcome } from './validate-null-guards.js'; +import { findUnguardedNullableOperands, nullGuardMessage, provenNonNullKeys } from './validate-null-guards.js'; +import type { NullGuardOptions, NullGuardOutcome } from './validate-null-guards.js'; import { recordsOf } from './object-graph.js'; export interface ExprIssue { where: string; @@ -455,8 +456,9 @@ function fieldEntries(obj: AnyRec): Array<[string, AnyRec]> { * Deliberately conservative — this feeds a **build-breaking** verdict, so every * uncertainty resolves to "not nullable" (no finding). A field is treated as * always-valued when it is `required`, carries a `defaultValue`, declares a - * default option (`options: [{ …, default: true }]` — the select idiom), or is - * an autonumber the platform populates. + * default option (`options: [{ …, default: true }]` — the select idiom), is + * an autonumber the platform populates, or is a `count` / `sum` roll-up + * summary ({@link isAlwaysValuedSummary}). * * The select-idiom branch is grounded, not assumed (#7246). It was the only * consumer of `SelectOption.default` anywhere in the repo while the insert path @@ -474,6 +476,7 @@ function isNullableField(def: AnyRec): boolean { if (def.required === true) return false; if (def.defaultValue !== undefined && def.defaultValue !== null) return false; if (def.type === 'autonumber') return false; + if (isAlwaysValuedSummary(def)) return false; const options = def.options; if (Array.isArray(options) && options.some((o) => !!o && typeof o === 'object' && (o as AnyRec).default === true)) { return false; @@ -493,6 +496,31 @@ function isNullableField(def: AnyRec): boolean { * needs the registry's column definitions, not this module's guesses. Leaving * them out costs coverage, never a false error. */ +/** + * [#29026] A `count` / `sum` roll-up summary is always valued: zero children is + * zero. `@objectstack/objectql`'s `summaryEmptySetValue` is the authority — the + * engine seeds a new parent row with it, falls back to it when an aggregate + * over no rows answers `null`, and backfills a stored `NULL` with it — and this + * package may not import a runtime, so the two functions are named here. A + * `min` / `max` / `avg` over no rows IS `null`, and stays nullable. Measured on + * a record-triggered run: `showcase_expense_report.total_amount` (a `sum`) + * reads `0` on a report with no lines + * (`examples/app-showcase/test/notify-recipient-optional-fields.test.ts`). + */ +function isAlwaysValuedSummary(def: AnyRec): boolean { + if (def.type !== 'summary' || !def.summaryOperations || typeof def.summaryOperations !== 'object') return false; + const summaryOperations = def.summaryOperations as AnyRec; + return summaryOperations.function === 'count' || summaryOperations.function === 'sum'; +} + +/** + * [#29026] The names a flow run binds OVER the flattened record fields, so a + * bare read of one is never the field: `record` is set before the flattening + * (`seedRunVariables`), `previous` after it, and `vars` / `current_user` win in + * `celScope`. Subtracted from the null-guard gate's bare-name set. + */ +const FLOW_ENGINE_BOUND_NAMES: ReadonlySet = new Set(['record', 'previous', 'vars', 'current_user']); + function buildNullableFieldIndex(objects: AnyRec[]): Map> { const idx = new Map>(); for (const obj of objects) { @@ -2273,16 +2301,16 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression * CEL evaluates over a record made **total** for every declared field * (`materializeDeclaredFields`, #1871/#4649) — today: object validation rules * (`rule-validator.ts`, fail-closed since #4761), lifecycle hook `condition`s, - * and field `requiredWhen` (#4811). + * field `requiredWhen` (#4811), and a record-triggered flow's start, edge and + * `decision` branch conditions (#29026, outcome `run-fails`). * * Totality is the criterion that can DISQUALIFY a surface: on a total binding * `has()` is uniformly true and `!= null` is the fix, while on a SPARSE one * `has()` is a genuine guard and `!= null` faults with `No such key` — so * pointing this gate at a sparse-bound surface would reject correct metadata * and prescribe a fix that breaks it. It is not by itself a licence to cover: - * since #6454 field `readonlyWhen` is total and still excluded, by clause 3 of - * the #4953 ruling (the gate widens only once BOTH server-side seams are - * total, and the flow trigger-record half is not wired yet). + * since #6454 field `readonlyWhen` is total and still excluded — its widening + * is a card of its own that nobody has taken (clause 3 of the #4953 ruling). * `validate-null-guards.ts` carries the measured evidence table and the * per-surface ledger (action predicates, flow conditions, field * `readonlyWhen`, sharing rules and `Field.formula` are each excluded there @@ -2298,13 +2326,15 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression // explicitly rather than inferred, because the two possibilities are // opposite failures and the message has to name the right one (#4811). outcome: NullGuardOutcome = 'fail-closed', + // [#29026] A flow's flattened bare names and its start condition's proofs. + scope: Pick = {}, ): void => { if (!objectName) return; const nullableFields = nullableIndex.get(objectName); if (!nullableFields || nullableFields.size === 0) return; const source = celSourceOf(raw); if (!source) return; - for (const finding of findUnguardedNullableOperands(source, { nullableFields })) { + for (const finding of findUnguardedNullableOperands(source, { nullableFields, ...scope })) { issues.push({ where, message: nullGuardMessage(subject, objectName, finding, outcome), @@ -2640,6 +2670,44 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression } }; + /** + * [#29026] The null-guard gate on this flow's start, edge and `decision` + * branch conditions — on a RECORD-TRIGGERED flow only, the one entrance + * that makes the run's `record` / `previous` total + * (`record-change-trigger.ts` `buildContext`); `validate-null-guards.ts`'s + * ledger, flow row, carries the measurements behind each choice here. + * + * - Bare names: the run flattens the record's fields, nulls included, to + * top level wherever no variable holds the name, so a bare `budget` IS + * `record.budget` — judged only while the flow's bound set is provable + * (`rootScope`: no node can bind a name the reader cannot see) and the + * name is not a declared variable or node id, which win. The names the + * engine binds over the flattening (`previous`, and `vars` / + * `current_user` in `celScope`) are never the field. + * - The start condition gates every run before any node, so what it + * proves non-null is presumed at every edge and branch downstream. + * - Outcome `run-fails`: the condition throws, the run fails, and the + * trigger isolates the write that fired it. + */ + const nullGuardObject = resolveFlowTriggerKind(flow) === 'record_change' ? objectName : undefined; + const nullGuardBare = new Set(); + if (nullGuardObject && rootScope.provable) { + for (const name of nullableIndex.get(nullGuardObject) ?? []) { + if (!declaredVariables.has(name) && !FLOW_ENGINE_BOUND_NAMES.has(name)) nullGuardBare.add(name); + } + } + const startConditionSource = nullGuardObject ? celSourceOf(startCfg.condition) : undefined; + const nullGuardPresumed = startConditionSource + ? provenNonNullKeys(startConditionSource, { bareFields: nullGuardBare }) + : new Set(); + const checkFlowNullGuards = (where: string, raw: unknown, afterStart: boolean): void => { + if (!nullGuardObject) return; + checkNullGuards(where, where, raw, nullGuardObject, 'run-fails', { + bareFields: nullGuardBare, + ...(afterStart ? { presumed: nullGuardPresumed } : {}), + }); + }; + /** * [#14089] The one bare-identifier case a flattened flow scope may not stay * silent about: a name bound as BOTH a declared flow variable and a field on @@ -2758,6 +2826,10 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression check(nodeCondWhere, cfg.condition, objectName); warnShadowedFieldReads(nodeCondWhere, cfg.condition); refuseUnboundRoots(nodeCondWhere, cfg.condition, from, 'predicate'); + // [#29026] The start node's is the trigger gate; on any other node + // the key is inert (`flow-inert-node-condition` says so) and judging + // it would only add a second finding about a predicate nothing runs. + if (node.type === 'start' && graph.path.length === 0) checkFlowNullGuards(nodeCondWhere, cfg.condition, false); } // Descriptor-declared expression slots (#4027). Before this, the traversal @@ -2855,6 +2927,11 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression warnShadowedFieldReads(slotWhere, found.value); // [#22565] The same scope again, judged for roots the run cannot bind. refuseUnboundRoots(slotWhere, found.value, slotFrom, 'predicate'); + // [#29026] A `decision` branch is evaluated by the engine's + // `evaluateCondition`, like an edge. The other `predicate` slots + // (a screen field's `visibleWhen`) fault into another outcome, which + // nobody has measured, so they are not judged here. + if (nodeType === 'decision') checkFlowNullGuards(slotWhere, found.value, true); } // [#19939] The `{…}` template dialect is retired from the value slots // (the C half of #11182 ruling D, which this replaced: until then a @@ -2981,6 +3058,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression check(edgeCondWhere, edge.condition, objectName); warnShadowedFieldReads(edgeCondWhere, edge.condition); refuseUnboundRoots(edgeCondWhere, edge.condition, from, 'predicate'); + checkFlowNullGuards(edgeCondWhere, edge.condition, true); } // [#26858] A top-level edge's findings also carry the slot's key path, // indexed over the list the author wrote (the coerced one drops `null` @@ -2993,29 +3071,6 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression for (let k = first; k < issues.length; k++) issues[k] = { ...issues[k]!, path }; } } - // No `checkNullGuards` on node/edge conditions — and NOT for the reason - // #4811 first recorded (#4811 re-measured it). The stated blocker was the - // flattened scope: a bare identifier like `budget > 100000` might be a flow - // variable rather than a field. That is true of a bare-identifier checker, - // but this gate never resolves a bare identifier — it matches only - // `record.` / `previous.`, and the engine binds both roots - // unconditionally, so it is immune to that ambiguity. - // - // The real blocker is totality. `record-change-trigger.ts` seeds the flow's - // record as `{ ...(inputData ?? {}), ...after }` (spelled `inputDoc` until - // #5671 dropped that alias read) with no `materializeDeclaredFields`, - // so a declared column the write never mentioned is an ABSENT key, not a - // null one — and there `record.x != null` faults (`No such key`) exactly - // like the comparison it was meant to guard. The gate's prescription is - // unsound on this surface until the trigger's record is made total the way - // #4649 made the validation-rule and hook bindings total. - // - // (For the record, the flattened-scope ambiguity is separately real and - // would need its own criterion: flow inputs shadow record fields — - // `if (!variables.has(k))` in `AutomationEngine.execute` — and a node's - // `outputVariable` can overwrite either, so a sound bare-identifier pass - // must subtract flow inputs, every `outputVariable`, screen-collected - // variable names and node ids.) } } @@ -3219,9 +3274,10 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression // formulas are `value` role (any return type), still CEL. They are // `record`-scoped — `record.`, never bare — so flag bare refs (#1928). // - // No `checkNullGuards` here, and unlike the action / flow surfaces this - // is NOT a totality verdict (#4811). A formula is `value`-role and - // natively nullable, and `guard ? value : null` is the blessed shape + // No `checkNullGuards` here, and unlike the action surface (and a flow + // with no record trigger) this is NOT a totality verdict (#4811). A + // formula is `value`-role and natively nullable, and + // `guard ? value : null` is the blessed shape // (#3306 rewrites it through `dyn(...)`). Whether unguarded arithmetic // such as `record.budget - record.spent` should be *forbidden* here is a // question about what authors are allowed to write — a product decision diff --git a/packages/lint/src/validate-null-guards.test.ts b/packages/lint/src/validate-null-guards.test.ts index a575108b620..8cc77e73bb2 100644 --- a/packages/lint/src/validate-null-guards.test.ts +++ b/packages/lint/src/validate-null-guards.test.ts @@ -12,6 +12,7 @@ import { findUnguardedNullableOperands, nullGuardMessage, NULL_GUARD_HINT, + provenNonNullKeys, } from './validate-null-guards.js'; const nullableFields = new Set(['start_date', 'end_date', 'budget', 'spent', 'score']); @@ -208,6 +209,66 @@ describe('findUnguardedNullableOperands — one answer to "what parses" (#4812)' }); }); +describe("findUnguardedNullableOperands — a flow's flattened bare names (#29026)", () => { + // A record-triggered run spreads the record's fields to top level, nulls + // included, so a bare `budget` IS `record.budget`. The caller names which + // bare names are flattened fields; nothing else changes. + const bareFields = new Set(['budget', 'score']); + const findBare = (source: string, presumed?: ReadonlySet) => + findUnguardedNullableOperands(source, { nullableFields, bareFields, ...(presumed ? { presumed } : {}) }); + + it('rejects the unguarded bare comparison that failed the showcase run', () => { + const findings = findBare('budget > 200000'); + expect(findings).toHaveLength(1); + expect(findings[0]).toMatchObject({ operand: 'budget', field: 'budget', operator: '>' }); + }); + + it('accepts a bare `!= null` guard, and a guard on either spelling guards both', () => { + expect(findBare('budget != null && budget > 200000')).toEqual([]); + expect(findBare('record.budget != null && budget > 200000')).toEqual([]); + expect(findBare('budget != null && record.budget > 200000')).toEqual([]); + expect(findBare('budget == null || budget <= 200000')).toEqual([]); + }); + + it('still says `has()` does not guard a bare read', () => { + const [finding] = findBare('has(record.budget) && budget > 1'); + expect(finding).toMatchObject({ operand: 'budget', hasOnlyGuard: true }); + expect(nullGuardMessage('edge', 'showcase_project', finding!, 'run-fails')).toContain( + '`has(record.budget)` does not guard it', + ); + }); + + it('leaves a bare name the caller did not name alone (a variable, or not a field)', () => { + // `spent` is nullable but not in `bareFields`: a declared variable of the + // flow holds that name, and the variable wins over the flattened field. + expect(findBare('spent > 1')).toEqual([]); + // Without `bareFields` the gate is exactly what it was: no bare name judged. + expect(find('budget > 200000')).toEqual([]); + }); + + it('does not read a comprehension or `cel.bind` variable as the field it shadows', () => { + expect(findBare('[1, 2].exists(budget, budget > 1)')).toEqual([]); + expect(findBare('cel.bind(score, 3, score > 1)')).toEqual([]); + // …but the macro's receiver, and the bind's initialiser, are outside the binding. + expect(findBare('[budget].exists(x, x > 1) && budget > 1').map((f) => f.operand)).toEqual(['budget']); + expect(findBare('cel.bind(x, score + 1, x > 1)').map((f) => f.operand)).toEqual(['score']); + }); + + it('presumes what a gating predicate proved (`presumed`)', () => { + const proved = provenNonNullKeys('budget != null && score != null', { bareFields }); + expect([...proved].sort()).toEqual(['record.budget', 'record.score']); + expect(findBare('budget > 500000', proved)).toEqual([]); + expect(findBare('record.score <= 3', proved)).toEqual([]); + // Proved by only one arm of an `||` is not proved. + const half = provenNonNullKeys('budget != null || score != null', { bareFields }); + expect(findBare('budget > 500000', half).map((f) => f.operand)).toEqual(['budget']); + }); + + it('proves nothing from a source that does not parse', () => { + expect(provenNonNullKeys('budget != null &&', { bareFields }).size).toBe(0); + }); +}); + describe('nullGuardMessage', () => { it('names the rule, the operand and the `!= null` fix', () => { const [finding] = find('has(record.end_date) && record.end_date < 5'); @@ -220,6 +281,14 @@ describe('nullGuardMessage', () => { expect(msg).toContain(NULL_GUARD_HINT); }); + it("names a flow condition's real outcome: the run fails and the write lands (#29026)", () => { + const [finding] = find('record.budget > 5'); + const msg = nullGuardMessage("flow 'f' · edge 'e2' condition", 'showcase_project', finding!, 'run-fails'); + expect(msg).toContain('the flow run fails at this condition'); + expect(msg).not.toContain('the write is rejected'); + expect(msg).toContain(NULL_GUARD_HINT); + }); + it('closes with the runtime rejection wording verbatim (one voice, two gates)', () => { // Lifted from `unevaluableRuleError` in // packages/objectql/src/validation/rule-validator.ts — if that text moves, diff --git a/packages/lint/src/validate-null-guards.ts b/packages/lint/src/validate-null-guards.ts index 64638b82d14..9230e69e7fd 100644 --- a/packages/lint/src/validate-null-guards.ts +++ b/packages/lint/src/validate-null-guards.ts @@ -90,13 +90,15 @@ * | field `requiredWhen` | TOTAL | same `merged` in `evaluateValidationRules` — fail-OPEN, so an unguarded predicate enforces NOTHING in silence | covered (#4811) | * | field `readonlyWhen` | TOTAL | `rule-validator.ts` `readonlyWhenBindings` materialises BOTH roots (#4953 clause 1, landed in #6454) | excluded — NOT on totality; see below | * | action `visible` / `disabled` | sparse | evaluated client-side; no materialization exists in `objectui` | excluded (decided — #4953 clause 2) | - * | flow / edge `condition` | TOTAL | `record-change-trigger.ts` `buildContext` layers `previous` under the payload/after-row then runs BOTH `record` and `previous` through a structural-mirror `materializeDeclaredFields` (#4953 clause 1's other half, services lane) | excluded — NOT on totality; see below | + * | flow start / edge / `decision` condition — record-triggered flow | TOTAL | `record-change-trigger.ts` `buildContext` layers `previous` under the payload/after-row then runs BOTH `record` and `previous` through a structural-mirror `materializeDeclaredFields` (#4953 clause 1's other half, services lane); `seedRunVariables` flattens that same total record to bare names | covered (#29026), outcome `run-fails`; see below | + * | the same slots, any other flow | sparse or unknown | the inbound hook hands the request body, an action an empty record carrying at most `id`, a `map` item its own keys; nothing materializes them | excluded | * | sharing-rule `condition` | n/a | compiled to a SQL filter; `NULL > x` is three-valued, never faults | excluded | * | field `expression` (`Field.formula`) | n/a | product judgement, not a wiring gap — see below | excluded | * - * The four exclusions that are *not* self-evident, spelled out because a - * surface excluded without a reason is indistinguishable from one nobody - * looked at — the failure mode this whole family of issues is about: + * The three exclusions that are *not* self-evident, and the one covered row + * whose wiring is not either, spelled out because a surface excluded without + * a reason is indistinguishable from one nobody looked at — the failure mode + * this whole family of issues is about: * * - **Field `readonlyWhen` — the row where `binding` and `verdict` came * apart.** Its binding is no longer sparse. Since #6454 (the engine-core @@ -109,13 +111,14 @@ * describes code that no longer exists. What keeps the row excluded is * clause 3 of the same ruling: the gate widens once BOTH server-side seams * are total. The other one — flow trigger-record seeding, services lane — - * is ALSO wired now (#4953 clause 1's other half; see the `flow / edge - * condition` row below), so both server-side seams the ruling named are - * total as of that landing. The actual gate widening clause 3 promises is - * tracked separately (#4811) rather than folded into either seam's own - * PR — this row (and the one below) stay excluded here on purpose, so the - * `binding` column keeps meaning what #4811 needs it to mean: a fact about - * the surface, not a verdict this module renders on itself. + * is ALSO wired now (#4953 clause 1's other half; see the flow row below), + * so both server-side seams the ruling named are total as of that landing. + * The gate widening clause 3 promises was tracked separately (#4811, + * closed) rather than folded into either seam's own PR; the flow row has + * since been widened on its own card (#29026), and this row has not — it + * stays excluded until a card widens it, so the `binding` column keeps + * meaning a fact about the surface, not a verdict this module renders on + * itself. * * Two facts to carry into that widening; neither is bookkeeping: * @@ -213,13 +216,12 @@ * correction was a separate card on purpose: it changes what authors are * TOLD TO WRITE, which is not a lint change, and it does not disturb this * row's verdict (still excluded, still permanently). - * - **Flow / edge `condition` — the row where `binding` and `verdict` came - * apart, same shape as `readonlyWhen` above.** #4811 originally excluded + * - **Flow start / edge / `decision` condition — covered since #29026, on a + * record-triggered flow only.** #4811 originally excluded * these for flattened-scope ambiguity ("a bare identifier may be a flow - * variable"). That reason does not actually apply to this module — - * {@link findUnguardedNullableOperands} only ever resolves `record.` / - * `previous.` and never a bare identifier, and the engine binds - * `record` / `previous` unconditionally. The real blocker was totality: + * variable"). That reason never applied to the `record.` / + * `previous.` half — the engine binds both roots whenever the run was + * handed a record. The real blocker was totality: * the trigger used to seed the record as `{...(inputData ?? {}), ...after}` * — spelled `inputDoc` here until #5671 dropped that alias read — so a * declared column the write never mentioned was an ABSENT key, and the @@ -239,12 +241,44 @@ * `{...(inputData ?? {}), ...after}` seed) describes code that no longer * exists. * - * What keeps the row excluded is the SAME clause-3 reason the - * `readonlyWhen` row states: the gate's actual widening is tracked - * separately (#4811), not folded into this seam's own PR. - * (The flattened-scope ambiguity is real for a *bare-identifier* checker — - * flow inputs shadow record fields, and a node's `outputVariable` can - * overwrite either — but that is a different, unbuilt pass.) + * What the wiring (`validate-expressions.ts`, the flow walk) decides, and + * why each part is the way it is: + * + * 1. **Which flows.** Trigger kind `record_change` (the spec's + * `resolveFlowTriggerKind`) on a declared `objectName` — the one + * entrance that materializes. Every other entrance hands a record + * nothing makes total (an `api` flow's request body, a `type: 'flow'` + * action's empty record, a `map` item), so on a flow with no record + * trigger the inversion in the table above is live and the gate stays + * off. A record-triggered flow ALSO launched from such an entrance is + * still judged: on its trigger runs `has()` alone is the trap, and the + * guard safe on both bindings, `has(record.x) && record.x != null`, + * passes this gate. + * 2. **Bare names.** The motivating fault was bare (`budget > 200000`): + * `seedRunVariables` spreads the record's fields to top level, nulls + * included, wherever no variable holds the name. So a bare name is + * judged as `record.` when the flow's bound set is provable + * (`flowCelRootScope`) and the name is not one of its declared + * variables or node ids (`collectFlowVariableNames`) — those win, and + * a flow whose nodes can bind names nobody declared leaves its bare + * names unjudged. Guarding either spelling guards both. + * 3. **The start condition gates every run.** `execute()` evaluates it + * before any node, whatever the entrance, so what it proves non-null + * is presumed at every edge and `decision` branch of the flow + * ({@link provenNonNullKeys}); an author does not re-guard downstream. + * 4. **Outcome `run-fails`, not `fail-closed`.** `evaluateCondition` + * throws on the fault (ADR-0032 §1c) and the record-change trigger + * isolates the write, so the run fails and the write lands. + * + * Not reached: `previous` is bound to `null` on a create leg + * (`seedRunVariables`), so on a `record-after-write` flow + * `previous.x != null` itself faults there; the full guard is + * `previous != null && previous.x != null`. The gate still reports the + * unguarded `previous.x` (it faults on the update leg's null), and + * `previous != null` alone is not accepted as guarding `previous.x`. + * Other declared predicate slots (a `screen` field's `visibleWhen`) are + * not judged: their fault does not take this outcome, and nobody has + * measured what it does take. * - **Field `expression`** (the slot `Field.formula({ expression: … })` * writes; spelled `formula` here until #5026 renamed the read to the key * `FieldSchema` actually declares). Excluded by product judgement, not by @@ -281,8 +315,33 @@ export interface NullGuardOptions { nullableFields: ReadonlySet; /** Roots bound to the object's record shape. Defaults to `record`/`previous`. */ roots?: readonly string[]; + /** + * Names that, read BARE, are the bound record's field of the same name — a + * flow's flattened scope, where `seedRunVariables` spreads the trigger + * record's fields to top level wherever no variable already holds the name. + * The caller decides the set (the flow's declared variables and node ids + * subtracted, since those win); empty by default, so every other surface + * keeps judging `record.` / `previous.` alone. A bare `budget` and + * `record.budget` read the same value there, so a guard on either spelling + * guards both. A name a comprehension macro or `cel.bind` binds is that + * binding, never the field, inside the part it is bound in. + */ + bareFields?: ReadonlySet; + /** + * Guard keys already proven non-null before this predicate runs — the + * flow's start condition, which gates every run ({@link provenNonNullKeys}). + */ + presumed?: ReadonlySet; +} + +/** How operands resolve: the record roots, and the bare names the scope flattens. */ +interface Resolver { + readonly roots: readonly string[]; + readonly bare: ReadonlySet; } +const NO_BARE: ReadonlySet = new Set(); + export interface NullGuardFinding { /** The operand as written, e.g. `record.end_date`. */ operand: string; @@ -309,13 +368,22 @@ function isNullLiteral(node: unknown): boolean { } /** - * `record.` (or `previous.`) → the field name; anything else — - * a bare id, a nested traversal `record.account.region`, an index — → null. + * `record.` (or `previous.`) → the field name; a bare name the + * resolver flattens → that name; anything else — a nested traversal + * `record.account.region`, an index, any other bare id — → null. * Deliberately single-segment: a nested path is a lookup traversal whose * nullability this object's field list cannot decide. + * + * `key` is what a guard is matched on: a bare flattened `budget` IS + * `record.budget`, so both spellings share the key `record.budget`. */ -function fieldOf(node: unknown, roots: readonly string[]): { operand: string; field: string } | null { +function fieldOf(node: unknown, r: Resolver): { operand: string; field: string; key: string } | null { if (!isNode(node)) return null; + if (node.op === 'id') { + const name = (node as AnyNode).args; + if (typeof name !== 'string' || !r.bare.has(name)) return null; + return { operand: name, field: name, key: `record.${name}` }; + } if (node.op !== '.' && node.op !== '.?') return null; const args = node.args as unknown; if (!Array.isArray(args) || args.length < 2) return null; @@ -323,8 +391,43 @@ function fieldOf(node: unknown, roots: readonly string[]): { operand: string; fi if (typeof seg !== 'string') return null; if (!isNode(recv) || recv.op !== 'id') return null; const root = (recv as AnyNode).args; - if (typeof root !== 'string' || !roots.includes(root)) return null; - return { operand: `${root}.${seg}`, field: seg }; + if (typeof root !== 'string' || !r.roots.includes(root)) return null; + const operand = `${root}.${seg}`; + return { operand, field: seg, key: operand }; +} + +/** cel-js's comprehension macros: the receiver calls that bind their first argument in the rest. */ +const COMPREHENSION_MACROS: ReadonlySet = new Set(['all', 'exists', 'exists_one', 'map', 'filter']); + +/** + * A comprehension macro (`list.exists(x, …)`) or `cel.bind(x, init, …)`: the + * name it binds, the parts evaluated OUTSIDE the binding, and the parts the + * name is bound in. `undefined` for any other node. + */ +function bindingScope(node: AnyNode): { name: string; outside: unknown[]; inside: unknown[] } | undefined { + if (node.op !== 'rcall' || !Array.isArray(node.args)) return undefined; + const [name, receiver, list] = node.args as [unknown, unknown, unknown]; + if (typeof name !== 'string' || !Array.isArray(list)) return undefined; + const first = list[0]; + const bound = isNode(first) && first.op === 'id' && typeof (first as AnyNode).args === 'string' + ? ((first as AnyNode).args as string) + : undefined; + if (!bound) return undefined; + if (COMPREHENSION_MACROS.has(name) && list.length >= 2) { + return { name: bound, outside: [receiver], inside: list.slice(1) }; + } + if (name === 'bind' && isNode(receiver) && receiver.op === 'id' && (receiver as AnyNode).args === 'cel' && list.length === 3) { + return { name: bound, outside: [list[1]], inside: [list[2]] }; + } + return undefined; +} + +/** The resolver inside a binding of `name`: that name is the binding, not the record. */ +function shadow(r: Resolver, name: string): Resolver { + if (!r.bare.has(name) && !r.roots.includes(name)) return r; + const bare = new Set(r.bare); + bare.delete(name); + return { roots: r.roots.filter((root) => root !== name), bare }; } /** Children of a node, whatever its arg shape. */ @@ -371,18 +474,18 @@ function intersect(a: ReadonlySet, b: ReadonlySet): Set * `isBlank` being *true* says nothing about non-nullness (it is true FOR null); * it appears in {@link falseGuards} instead. */ -function truthGuards(node: unknown, roots: readonly string[]): Set { +function truthGuards(node: unknown, roots: Resolver): Set { if (!isNode(node)) return new Set(); switch (node.op) { case '!=': { const [l, r] = (node.args as [unknown, unknown]) ?? []; if (isNullLiteral(r)) { const f = fieldOf(l, roots); - return f ? new Set([f.operand]) : new Set(); + return f ? new Set([f.key]) : new Set(); } if (isNullLiteral(l)) { const f = fieldOf(r, roots); - return f ? new Set([f.operand]) : new Set(); + return f ? new Set([f.key]) : new Set(); } return new Set(); } @@ -407,18 +510,18 @@ function truthGuards(node: unknown, roots: readonly string[]): Set { } /** Operands proven non-null **when `node` evaluates false** (the `!`/`||` side). */ -function falseGuards(node: unknown, roots: readonly string[]): Set { +function falseGuards(node: unknown, roots: Resolver): Set { if (!isNode(node)) return new Set(); switch (node.op) { case '==': { const [l, r] = (node.args as [unknown, unknown]) ?? []; if (isNullLiteral(r)) { const f = fieldOf(l, roots); - return f ? new Set([f.operand]) : new Set(); + return f ? new Set([f.key]) : new Set(); } if (isNullLiteral(l)) { const f = fieldOf(r, roots); - return f ? new Set([f.operand]) : new Set(); + return f ? new Set([f.key]) : new Set(); } return new Set(); } @@ -438,25 +541,42 @@ function falseGuards(node: unknown, roots: readonly string[]): Set { if (callName(node) !== 'isBlank') return new Set(); const [only] = callArgs(node); const f = fieldOf(only, roots); - return f ? new Set([f.operand]) : new Set(); + return f ? new Set([f.key]) : new Set(); } default: return new Set(); } } -/** Collect every `has(record.)` operand appearing anywhere in the tree. */ -function collectHasOperands(node: unknown, roots: readonly string[], out: Set): void { +/** Collect the guard key of every `has(record.)` appearing anywhere in the tree. */ +function collectHasOperands(node: unknown, roots: Resolver, out: Set): void { if (!isNode(node)) return; if (callName(node) === 'has') { for (const a of callArgs(node)) { const f = fieldOf(a, roots); - if (f) out.add(f.operand); + if (f) out.add(f.key); } } for (const child of childNodes(node)) collectHasOperands(child, roots, out); } +/** + * The guard keys `source` proves non-null whenever it evaluates TRUE — for a + * predicate that gates everything after it, such as a flow's start condition + * (the engine evaluates it before any node, on every run, so a run that + * reaches an edge is a run on which it held). Empty for a source that does not + * parse. Feed the result to {@link NullGuardOptions.presumed}. + */ +export function provenNonNullKeys( + source: string, + opts: Pick, +): Set { + if (typeof source !== 'string' || !source.trim()) return new Set(); + const ast = parseCelToAst(source); + if (!ast) return new Set(); + return truthGuards(ast, { roots: opts.roots ?? DEFAULT_RECORD_ROOTS, bare: opts.bareFields ?? NO_BARE }); +} + /** * Find every ordering/arithmetic operand that resolves to a nullable declared * field and is not dominated by a real null guard. Returns `[]` for anything @@ -487,22 +607,22 @@ export function findUnguardedNullableOperands( ): NullGuardFinding[] { if (typeof source !== 'string' || !source.trim()) return []; if (opts.nullableFields.size === 0) return []; - const roots = opts.roots ?? DEFAULT_RECORD_ROOTS; + const top: Resolver = { roots: opts.roots ?? DEFAULT_RECORD_ROOTS, bare: opts.bareFields ?? NO_BARE }; const ast = parseCelToAst(source); if (!ast) return []; const hasOperands = new Set(); - collectHasOperands(ast, roots, hasOperands); + collectHasOperands(ast, top, hasOperands); const findings: NullGuardFinding[] = []; const seen = new Set(); - const report = (operandNode: unknown, operator: string, guards: ReadonlySet): void => { + const report = (operandNode: unknown, operator: string, guards: ReadonlySet, roots: Resolver): void => { const f = fieldOf(operandNode, roots); if (!f) return; if (!opts.nullableFields.has(f.field)) return; - if (guards.has(f.operand)) return; + if (guards.has(f.key)) return; // NUL separates the composite key's two halves (it can appear in neither a // field path nor an operator). Written as the `\u0000` ESCAPE, never as a raw // byte: a raw NUL makes grep/ripgrep treat the whole file as binary and @@ -518,56 +638,65 @@ export function findUnguardedNullableOperands( operand: f.operand, field: f.field, operator, - hasOnlyGuard: hasOperands.has(f.operand), + hasOnlyGuard: hasOperands.has(f.key), }); }; - const visit = (node: unknown, guards: ReadonlySet): void => { + const visit = (node: unknown, guards: ReadonlySet, roots: Resolver): void => { if (!isNode(node)) return; const op = node.op as string; if (op === '&&') { const [l, r] = node.args as [unknown, unknown]; - visit(l, guards); + visit(l, guards, roots); // Left-to-right: only the LEFT conjunct's proofs reach the right one. - visit(r, union(guards, truthGuards(l, roots))); + visit(r, union(guards, truthGuards(l, roots)), roots); return; } if (op === '||') { const [l, r] = node.args as [unknown, unknown]; - visit(l, guards); + visit(l, guards, roots); // Reaching the right arm means the left one was FALSE. - visit(r, union(guards, falseGuards(l, roots))); + visit(r, union(guards, falseGuards(l, roots)), roots); return; } if (op === '?:') { const [c, t, f] = node.args as [unknown, unknown, unknown]; - visit(c, guards); - visit(t, union(guards, truthGuards(c, roots))); - visit(f, union(guards, falseGuards(c, roots))); + visit(c, guards, roots); + visit(t, union(guards, truthGuards(c, roots)), roots); + visit(f, union(guards, falseGuards(c, roots)), roots); return; } if (op === 'call' && callName(node) === 'has') { // `has(x)` itself never faults — and it never counts as a guard either. return; } + const binding = bindingScope(node); + if (binding) { + // The bound name is the macro's element / `cel.bind`'s value inside the + // part it is bound in — never the record's field of the same name. + for (const part of binding.outside) visit(part, guards, roots); + const inner = shadow(roots, binding.name); + for (const part of binding.inside) visit(part, guards, inner); + return; + } if (FAULTING_BINARY_OPS.has(op)) { const [l, r] = node.args as [unknown, unknown]; - report(l, op, guards); - report(r, op, guards); - visit(l, guards); - visit(r, guards); + report(l, op, guards, roots); + report(r, op, guards, roots); + visit(l, guards, roots); + visit(r, guards, roots); return; } if (op === '-_') { - report(node.args, '-', guards); - visit(node.args, guards); + report(node.args, '-', guards, roots); + visit(node.args, guards, roots); return; } - for (const child of childNodes(node)) visit(child, guards); + for (const child of childNodes(node)) visit(child, guards, roots); }; - visit(ast, new Set()); + visit(ast, new Set(opts.presumed ?? []), top); return findings; } @@ -590,9 +719,20 @@ export type NullGuardOutcome = * its clause below still describes the `requiredWhen` runtime it was written * for — re-word it before a surface that genuinely skips adopts it. */ - | 'fail-open'; + | 'fail-open' + /** + * A record-triggered flow's start, edge or `decision` branch condition: + * `evaluateCondition` throws on the fault (ADR-0032 §1c, never a quiet + * `false`), so the RUN fails there, while the record-change trigger + * isolates the write that fired it — the write lands, the flow's work does + * not happen. + */ + | 'run-fails'; const OUTCOME_CLAUSE: Record = { + 'run-fails': + 'so the flow run fails at this condition — the write that fired it still lands, and the work ' + + 'the flow exists to do is not done', 'fail-closed': 'so the rule enforces nothing and the write is rejected fail-closed (a predicate that cannot ' + 'evaluate refuses the write rather than being skipped)', @@ -620,8 +760,10 @@ export function nullGuardMessage( outcome: NullGuardOutcome = 'fail-closed', ): string { const owner = objectName ? `'${objectName}'` : 'this object'; + // A bare operand is a flattened field; its `has()` was spelled on the record. + const hasOperand = finding.operand.includes('.') ? finding.operand : `record.${finding.operand}`; const hasNote = finding.hasOnlyGuard - ? ` \`has(${finding.operand})\` does not guard it.` + ? ` \`has(${hasOperand})\` does not guard it.` : ''; return ( `${subject} applies \`${finding.operator}\` to \`${finding.operand}\`, which ${owner} declares ` + From c81920589f66f83aae8d64661bfbad9443079ff8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 14:35:17 +0000 Subject: [PATCH 2/4] fix(lint): one dash in the flow outcome clause Claude-Session: https://claude.ai/code/session_01JQwxbZfKtKX5FguJoubPfK Co-authored-by: Claude --- packages/lint/src/validate-null-guards.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/lint/src/validate-null-guards.ts b/packages/lint/src/validate-null-guards.ts index 9230e69e7fd..b9e8de1e62c 100644 --- a/packages/lint/src/validate-null-guards.ts +++ b/packages/lint/src/validate-null-guards.ts @@ -731,7 +731,7 @@ export type NullGuardOutcome = const OUTCOME_CLAUSE: Record = { 'run-fails': - 'so the flow run fails at this condition — the write that fired it still lands, and the work ' + + 'so the flow run fails at this condition: the write that fired it still lands, and the work ' + 'the flow exists to do is not done', 'fail-closed': 'so the rule enforces nothing and the write is rejected fail-closed (a predicate that cannot ' + From cc7fa9bbcf25f0a673d84b74cfcb0c4d9cae6675 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 15:15:12 +0000 Subject: [PATCH 3/4] chore(changeset): answer the ADR-0087 disposition for #29026 Claude-Session: https://claude.ai/code/session_01JQwxbZfKtKX5FguJoubPfK Co-authored-by: Claude --- .changeset/29026-flow-condition-null-guard.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.changeset/29026-flow-condition-null-guard.md b/.changeset/29026-flow-condition-null-guard.md index 1a71cd8d7b3..88b9bfe6b91 100644 --- a/.changeset/29026-flow-condition-null-guard.md +++ b/.changeset/29026-flow-condition-null-guard.md @@ -7,6 +7,8 @@ feat(lint): the publish-time null-guard gate judges a record-triggered flow's st Clause-②: no Metadata-protocol: narrowed + + `os build`, `os validate`, `os lint` and the runtime publish gate now refuse a record-triggered flow (start `triggerType: 'record-…'`) whose start condition, edge `condition` or `decision` branch `expression` applies an ordering (`< <= > >=`) or arithmetic (`+ - * / %`) operator to a nullable declared field of the trigger object with no `!= null` / `== null` / `!isBlank()` test in front of it. The run's `record` and `previous` carry every declared field, nulls included, so such a comparison faults with `no such overload` on the first record where the field is empty, and the run fails there (the write that fired it still lands). - **Who moves.** A flow condition such as `budget > 200000` on an optional `budget`. Fix: `budget != null && budget > 200000`, or put the `!= null` test in the flow's start condition, which gates every run, so every edge and branch after it may compare freely. `has(record.budget)` is not a guard: the key is always present. From c7417c1fc82d413709beb46dda9d1a175e650fc4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 16:08:08 +0000 Subject: [PATCH 4/4] chore(changeset): declare the narrowing arm beside Metadata-protocol: narrowed Claude-Session: https://claude.ai/code/session_01JQwxbZfKtKX5FguJoubPfK Co-authored-by: Claude --- .changeset/29026-flow-condition-null-guard.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/29026-flow-condition-null-guard.md b/.changeset/29026-flow-condition-null-guard.md index 88b9bfe6b91..425110dc18e 100644 --- a/.changeset/29026-flow-condition-null-guard.md +++ b/.changeset/29026-flow-condition-null-guard.md @@ -4,7 +4,7 @@ feat(lint): the publish-time null-guard gate judges a record-triggered flow's start, edge and decision-branch conditions -Clause-②: no +Clause-②: no (narrowing) Metadata-protocol: narrowed