diff --git a/.changeset/29026-flow-condition-null-guard.md b/.changeset/29026-flow-condition-null-guard.md new file mode 100644 index 00000000000..425110dc18e --- /dev/null +++ b/.changeset/29026-flow-condition-null-guard.md @@ -0,0 +1,17 @@ +--- +'@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 (narrowing) +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..b9e8de1e62c 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 ` +