From a1b2c24b580e9a549446c10bd45f6d41a6e6a350 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Tue, 16 Jun 2026 10:24:59 +0800 Subject: [PATCH] feat(validate): did-you-mean warnings for field typos in flow conditions (#1928) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flow/automation conditions flatten the record's fields to top-level, so a bare `status` is correct — but a bare NON-field identifier is either a flow variable or a typo. When it's a near-miss of a known field, the build now emits a non-blocking `did you mean X?` warning instead of staying silent, closing the silent-skip gap for misspelled trigger-condition fields (#1877 family) without the false-positive risk of a hard gate (a real flow variable won't be edit-distance-close to a field name, so it stays quiet). - `ExprValidationResult` gains `warnings`; `firstUndeclaredReference(source, knownFields)` generalizes the bare-ref detector (record scope = no fields, flattened scope = trigger object's fields declared so only non-field refs surface). - `ExprIssue` gains `severity`; `objectstack compile` prints warnings and fails only on errors. Verified: a typo'd flow condition warns + still builds; all example apps build clean with zero warnings (flow variables like `expiring_deals` stay quiet). Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/flow-condition-typo-warnings.md | 17 ++++++ packages/cli/src/commands/compile.ts | 18 +++++-- .../src/utils/validate-expressions.test.ts | 41 ++++++++++++++ .../cli/src/utils/validate-expressions.ts | 15 ++++-- packages/formula/src/cel-engine.ts | 31 ++++++++--- packages/formula/src/validate.test.ts | 34 ++++++++++++ packages/formula/src/validate.ts | 54 ++++++++++++++----- 7 files changed, 183 insertions(+), 27 deletions(-) create mode 100644 .changeset/flow-condition-typo-warnings.md diff --git a/.changeset/flow-condition-typo-warnings.md b/.changeset/flow-condition-typo-warnings.md new file mode 100644 index 0000000000..7f0be11b09 --- /dev/null +++ b/.changeset/flow-condition-typo-warnings.md @@ -0,0 +1,17 @@ +--- +"@objectstack/formula": patch +"@objectstack/cli": patch +--- + +feat(validate): advisory did-you-mean warnings for likely field typos in flow conditions + +Adds a non-blocking warning channel to build-time expression validation (#1928 +tier 3). Flow / automation conditions flatten the record's fields to top-level, +so a bare `status` is correct — but a bare NON-field identifier is either a flow +variable or a typo. When it is a near-miss of a known field (edit distance), the +build now emits a `did you mean \`status\`?` warning instead of staying silent, +WITHOUT failing the build (a genuine flow variable won't be close to a field +name, so it stays quiet). `ExprValidationResult` gains a `warnings` array and +`ExprIssue` a `severity`; `objectstack compile` prints warnings and only fails on +errors. This closes the silent-skip gap for misspelled trigger-condition fields +(the #1877 family) without the false-positive risk of a hard gate. diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts index b33644ccb0..051f639391 100644 --- a/packages/cli/src/commands/compile.ts +++ b/packages/cli/src/commands/compile.ts @@ -135,19 +135,29 @@ export default class Compile extends Command { // located, corrective message instead of a silent runtime `false`. if (!flags.json) printStep('Validating expressions (ADR-0032)...'); const exprIssues = validateStackExpressions(result.data as Record); - if (exprIssues.length > 0) { + const exprErrors = exprIssues.filter((i) => i.severity !== 'warning'); + const exprWarnings = exprIssues.filter((i) => i.severity === 'warning'); + if (exprErrors.length > 0) { if (flags.json) { - console.log(JSON.stringify({ success: false, error: 'expression validation failed', issues: exprIssues })); + console.log(JSON.stringify({ success: false, error: 'expression validation failed', issues: exprErrors, warnings: exprWarnings })); this.exit(1); } console.log(''); - printError(`Expression validation failed (${exprIssues.length} issue${exprIssues.length > 1 ? 's' : ''})`); - for (const i of exprIssues.slice(0, 50)) { + printError(`Expression validation failed (${exprErrors.length} issue${exprErrors.length > 1 ? 's' : ''})`); + for (const i of exprErrors.slice(0, 50)) { console.log(` • ${i.where}: ${i.message}`); console.log(` source: \`${i.source}\``); } this.exit(1); } + // Advisory expression warnings (#1928 tier 3) — surfaced, never fatal. + if (exprWarnings.length > 0 && !flags.json) { + printWarning(`Expression warnings (${exprWarnings.length})`); + for (const i of exprWarnings.slice(0, 50)) { + console.log(` • ${i.where}: ${i.message}`); + console.log(` source: \`${i.source}\``); + } + } // 3c. Widget-binding diagnostics (issues #1719/#1721) — semantic checks // that need the widget's `dataset` reference resolved to its dataset diff --git a/packages/cli/src/utils/validate-expressions.test.ts b/packages/cli/src/utils/validate-expressions.test.ts index 66433bf242..6536487a6e 100644 --- a/packages/cli/src/utils/validate-expressions.test.ts +++ b/packages/cli/src/utils/validate-expressions.test.ts @@ -191,5 +191,46 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { }); expect(issues).toHaveLength(0); }); + + // #1928 tier 3 — a likely field typo in a flow condition is a non-blocking warning. + it('warns (severity=warning) on a likely field typo in a flow condition', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'crm_opportunity', fields: { stage: { type: 'select' }, amount: { type: 'currency' } } }], + flows: [{ + name: 'opp_won', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_opportunity', condition: 'stagee == "closed_won"' } }, + ], + edges: [], + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('warning'); + expect(issues[0].message).toMatch(/did you mean `stage`/); + }); + + it('does not warn when the bare ref is far from any field (likely a flow variable)', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'crm_opportunity', fields: { stage: { type: 'select' } } }], + flows: [{ + name: 'renewal', + nodes: [{ id: 'start', type: 'start', config: { objectName: 'crm_opportunity' } }], + edges: [{ id: 'e1', source: 'start', target: 'end', condition: 'expiring_deals.length > 0' }], + }], + }); + expect(issues).toHaveLength(0); + }); + + it('tags record-scoped bare-ref issues as errors', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'crm_lead', + fields: { lead_score: { type: 'number' } }, + validations: [{ name: 'r', expression: 'lead_score > 100' }], + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('error'); + }); }); }); diff --git a/packages/cli/src/utils/validate-expressions.ts b/packages/cli/src/utils/validate-expressions.ts index 2da5377015..63f0d271dd 100644 --- a/packages/cli/src/utils/validate-expressions.ts +++ b/packages/cli/src/utils/validate-expressions.ts @@ -20,6 +20,12 @@ export interface ExprIssue { where: string; message: string; source: string; + /** + * `error` fails the build (e.g. a bare ref in a record-scoped formula). `warning` + * is advisory and never fails it (e.g. a possible field typo in a flattened flow + * condition, which might be a flow variable). Absent ⇒ treat as `error`. + */ + severity?: 'error' | 'warning'; } type AnyRec = Record; @@ -67,7 +73,8 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { const fields = objectName ? fieldIndex.get(objectName) : undefined; const res = validateExpression('predicate', raw as string | { dialect?: string; source?: string }, objectName ? { objectName, fields, scope } : { scope }); - for (const e of res.errors) issues.push({ where, message: e.message, source: e.source }); + for (const e of res.errors) issues.push({ where, message: e.message, source: e.source, severity: 'error' }); + for (const w of res.warnings) issues.push({ where, message: w.message, source: w.source, severity: 'warning' }); }; // ── Flows ────────────────────────────────────────────────────────── @@ -146,9 +153,9 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // `record`-scoped — `record.`, never bare — so flag bare refs (#1928). const res = validateExpression('value', f.formula as string | { dialect?: string; source?: string }, objectName ? { objectName, fields: fieldIndex.get(objectName), scope: 'record' } : { scope: 'record' }); - for (const e of res.errors) { - issues.push({ where: `object '${objectName}' · field '${(f.name as string) ?? '?'}' formula`, message: e.message, source: e.source }); - } + const fieldWhere = `object '${objectName}' · field '${(f.name as string) ?? '?'}' formula`; + for (const e of res.errors) issues.push({ where: fieldWhere, message: e.message, source: e.source, severity: 'error' }); + for (const w of res.warnings) issues.push({ where: fieldWhere, message: w.message, source: w.source, severity: 'warning' }); } } } diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 88ac13f5e4..e2f2abd44a 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -60,9 +60,7 @@ const SCOPE_ROOTS = [ * bare field references. It reuses the real stdlib so function calls don't fault; * only undeclared *variables* do. Built once — `parse`/`check` do not mutate it. */ -let scopedEnv: Environment | undefined; -function getScopedEnv(): Environment { - if (scopedEnv) return scopedEnv; +function buildScopedEnv(knownFields: readonly string[]): Environment { const env = new Environment({ unlistedVariablesAreDyn: false, enableOptionalTypes: true, @@ -72,10 +70,20 @@ function getScopedEnv(): Environment { for (const root of SCOPE_ROOTS) { try { env.registerVariable(root, 'map'); } catch { /* duplicate — ignore */ } } - scopedEnv = env; + // `knownFields` are declared as `dyn` so they (and member/arith/compare on + // them) never fault — only a genuinely-undeclared top-level identifier does. + // Empty for a record-scope site (any bare field is a bug); the trigger + // object's fields for a flattened flow condition (only a NON-field bare ref — + // a typo or flow variable — is then interesting). + for (const field of knownFields) { + try { env.registerVariable(field, 'dyn'); } catch { /* duplicate / reserved — ignore */ } + } return env; } +// Roots-only env reused for the common record-scope check (no per-call rebuild). +let recordScopeEnv: Environment | undefined; + /** * In a `record`-scoped CEL site — a `Field.formula` or an object validation * predicate — the evaluation scope binds only the `record`/`previous`/… *namespaces* @@ -89,10 +97,16 @@ function getScopedEnv(): Environment { * automation conditions, where the record's fields ARE flattened to top-level * and bare references are correct. */ -export function detectBareReference(source: string): string | null { +export function firstUndeclaredReference( + source: string, + knownFields: readonly string[] = [], +): string | null { if (typeof source !== 'string' || !source.trim()) return null; try { - const result = getScopedEnv().parse(source).check?.() as + const env = knownFields.length === 0 + ? (recordScopeEnv ??= buildScopedEnv([])) + : buildScopedEnv(knownFields); + const result = env.parse(source).check?.() as | { valid: boolean; error?: { message?: string } } | undefined; if (result && result.valid === false) { @@ -106,6 +120,11 @@ export function detectBareReference(source: string): string | null { return null; } +/** @deprecated use {@link firstUndeclaredReference} with no fields. */ +export function detectBareReference(source: string): string | null { + return firstUndeclaredReference(source); +} + /** Coerce cel-js's BigInt-flavored return into spec-friendly JS values. */ function coerce(value: unknown): unknown { if (typeof value === 'bigint') { diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index fb478f4ecf..e09fb1c85e 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -119,6 +119,40 @@ describe('validateExpression (ADR-0032)', () => { }); }); + // #1928 tier 3 — flattened flow conditions reference fields bare, so a bare + // ref is not an error. A bare NON-field that is a near-miss of a known field + // is a likely typo → non-blocking warning (ok stays true). + describe('flow-condition typo warnings (#1928 tier 3)', () => { + const fields = ['stage', 'amount', 'status'] as const; + + it('warns (does not error) on a likely field typo in a flattened condition', () => { + const r = validateExpression('predicate', 'stagee == "closed_won"', { objectName: 'crm_opportunity', fields, scope: 'flattened' }); + expect(r.ok).toBe(true); + expect(r.errors).toHaveLength(0); + expect(r.warnings).toHaveLength(1); + expect(r.warnings[0].message).toMatch(/`stagee` is not a field/); + expect(r.warnings[0].message).toMatch(/did you mean `stage`/); + }); + + it('does not warn on a correct bare field reference', () => { + const r = validateExpression('predicate', 'stage == "closed_won" && previous.stage != "closed_won"', { objectName: 'crm_opportunity', fields, scope: 'flattened' }); + expect(r.ok).toBe(true); + expect(r.warnings).toHaveLength(0); + }); + + it('does not warn on a flow variable that is far from any field name', () => { + const r = validateExpression('predicate', 'expiring_deals.length > 0', { objectName: 'crm_opportunity', fields, scope: 'flattened' }); + expect(r.ok).toBe(true); + expect(r.warnings).toHaveLength(0); + }); + + it('emits no warnings without a field list (nothing to compare against)', () => { + const r = validateExpression('predicate', 'stagee == "x"', { scope: 'flattened' }); + expect(r.ok).toBe(true); + expect(r.warnings).toHaveLength(0); + }); + }); + describe('introspection', () => { it('reports the dialect + scope for a field role', () => { expect(expectedDialect('predicate')).toBe('cel'); diff --git a/packages/formula/src/validate.ts b/packages/formula/src/validate.ts index a274fc7cda..6ff8944f5a 100644 --- a/packages/formula/src/validate.ts +++ b/packages/formula/src/validate.ts @@ -17,7 +17,7 @@ * This validator detects that specific mistake and returns the exact fix. */ -import { celEngine, detectBareReference } from './cel-engine'; +import { celEngine, firstUndeclaredReference } from './cel-engine'; import { templateEngine } from './template-engine'; export type FieldRole = 'predicate' | 'value' | 'template'; @@ -46,9 +46,11 @@ export interface ExprSchemaHint { * it MUST be written `record.amount`. We flag bare refs. * - `'flattened'` → the record's own fields are spread to top-level alongside * flow variables (flow / automation conditions), so bare - * `status` is correct. We do NOT flag bare refs here — flow - * variables are not schema-knowable, so a bare identifier - * can't be soundly told apart from a typo. (Default.) + * `status` is correct and is NOT an error. Flow variables + * are not schema-knowable, so a non-field bare identifier + * can't be soundly told apart from a typo — but when one is + * a near-miss of a known field we emit a non-blocking + * did-you-mean *warning*. (Default.) */ scope?: 'record' | 'flattened'; } @@ -63,6 +65,13 @@ export interface ExprValidationError { export interface ExprValidationResult { ok: boolean; errors: ExprValidationError[]; + /** + * Non-blocking advisories (#1928 tier 3): a likely-typo'd field reference in a + * flattened flow condition. Never affects `ok` — callers surface these without + * failing the build, since a bare identifier there may legitimately be a flow + * variable. + */ + warnings: ExprValidationError[]; } /** A bare `{x}` that is NOT part of a `{{x}}` mustache hole. */ @@ -149,14 +158,15 @@ export function validateExpression( ): ExprValidationResult { const { dialect, source } = toSource(input); const errors: ExprValidationError[] = []; - if (!source.trim()) return { ok: true, errors }; + const warnings: ExprValidationError[] = []; + if (!source.trim()) return { ok: true, errors, warnings }; if (role === 'template') { // Templates must be the `template` dialect (or untyped string). Reject a // CEL envelope mistakenly placed in a text field. if (dialect && dialect !== 'template') { errors.push({ source, message: `expected a text template but got a \`${dialect}\` expression.` }); - return { ok: false, errors }; + return { ok: false, errors, warnings }; } const compiled = templateEngine.compile(source); if (!compiled.ok) { @@ -165,13 +175,13 @@ export function validateExpression( // A single `{x}` in a template is the legacy/deprecated form (ADR-0032 §3). const hint = SINGLE_BRACE_RE.test(source) ? bracesHintForTemplate(source) : null; if (hint) errors.push({ source, message: hint }); - return { ok: errors.length === 0, errors }; + return { ok: errors.length === 0, errors, warnings }; } // predicate | value → CEL if (dialect && dialect !== 'cel') { errors.push({ source, message: `expected a CEL expression but got a \`${dialect}\` dialect.` }); - return { ok: false, errors }; + return { ok: false, errors, warnings }; } const compiled = celEngine.compile(source); if (!compiled.ok) { @@ -184,11 +194,10 @@ export function validateExpression( }); } else { checkFieldExistence(source, schema, errors); - // In a `record`-scoped site a bare top-level identifier is a silent bug — - // it must be `record.` (#1928). Only flagged here; flow/automation - // conditions (`scope: 'flattened'`, the default) legitimately use bare refs. if (schema?.scope === 'record') { - const bare = detectBareReference(source); + // In a `record`-scoped site a bare top-level identifier is a silent bug — + // it must be `record.` (#1928). Hard error. + const bare = firstUndeclaredReference(source); if (bare) { errors.push({ source, @@ -198,9 +207,28 @@ export function validateExpression( `expression silently evaluates to null. Write \`record.${bare}\`.`, }); } + } else if (schema?.fields && schema.fields.length > 0) { + // Flattened flow/automation condition: the record's fields ARE bound at + // top-level, so a bare ref is normally correct. But a *non-field* bare + // identifier is either a flow variable or a typo. When it is a near-miss + // of a known field, warn (did-you-mean) WITHOUT failing the build — + // a genuine flow variable won't be edit-distance-close to a field. (#1928) + const unknown = firstUndeclaredReference(source, schema.fields); + if (unknown) { + const suggestion = nearest(unknown, schema.fields); + if (suggestion) { + warnings.push({ + source, + message: + `\`${unknown}\` is not a field of \`${schema.objectName ?? 'the trigger object'}\` — ` + + `did you mean \`${suggestion}\`? (flow conditions reference fields bare, e.g. \`${suggestion} == …\`). ` + + `If \`${unknown}\` is a flow variable this is safe to ignore.`, + }); + } + } } } - return { ok: errors.length === 0, errors }; + return { ok: errors.length === 0, errors, warnings }; } function bracesHintForTemplate(source: string): string {