diff --git a/.changeset/unknown-node-config-key-warning.md b/.changeset/unknown-node-config-key-warning.md new file mode 100644 index 0000000000..7266d091b3 --- /dev/null +++ b/.changeset/unknown-node-config-key-warning.md @@ -0,0 +1,47 @@ +--- +'@objectstack/service-automation': minor +'@objectstack/formula': minor +--- + +Warn on flow-node `config` keys the node type does not declare (#4045). + +`FlowNodeSchema.config` is `z.record(z.unknown())`, so a misspelled or invented +config key was accepted in total silence: `visibleIf` instead of `visibleWhen` +registered cleanly, was never read, and the only symptom was a feature that quietly +did not happen. That diagnostic vacuum is what made #3528 take three passes and two +wrong diagnoses to resolve. + +`registerFlow` now compares each node's `config` against its descriptor's +`configSchema` and warns on anything undeclared, located and with the declared set +listed: + +``` +[flow 'lead_conversion'] node 'screen_1' (screen): unknown config key `visibleIf` + at config.fields[0].visibleIf — It is not declared by this node type's + configSchema, so nothing reads it. Declared here: name, label, type, required, + visibleWhen. +``` + +The walk descends where the schema declares structure and **stops at free-form +keyValue maps**, whose keys are author data (`filter: { status: 'stale' }`). +Descending matters: the #3528 typo class lives *inside* the `screen` field +repeater, so a top-level-only comparison would miss the exact mistake this exists +to catch. + +**Warn, never reject.** An undeclared key is an author typo, a key the executor +genuinely reads that its hand-written `configSchema` never declared (`notify.source` +was exactly this), or dead config. Only 4 of the 13 schema-carrying builtins have +been audited for the second population, so hard-failing would gamble on the other +nine. Tightening to an error is a later, per-key decision once this warning has +measured the real distribution. Nothing about the published `configSchema` changes, +so no consumer sees a different shape. + +`@objectstack/formula` now exports `nearestName`, the edit-distance helper already +used for unknown-field and unknown-role suggestions, so "did you mean?" +diagnostics share one threshold. It is deliberately a bonus rather than the +mechanism — `visibleIf` → `visibleWhen` is distance 4 against a threshold of 3, so +the declared set is always listed instead of only as a fallback. + +Also fixes the first real finding from the new check: `showcase_inquiry_purge`'s +`get_record` node carried `mode: 'records'`, which no executor reads, with a comment +crediting it for behaviour that `limit > 1` actually produces. diff --git a/examples/app-showcase/src/automation/flows/index.ts b/examples/app-showcase/src/automation/flows/index.ts index 49176ba157..fd6b12e33d 100644 --- a/examples/app-showcase/src/automation/flows/index.ts +++ b/examples/app-showcase/src/automation/flows/index.ts @@ -1357,13 +1357,16 @@ export const InquiryPurgeFlow = defineFlow({ id: 'purge_check', type: 'get_record', label: 'Find closed inquiries', - // records mode + outputVariable: the result lands in the variable - // context, where the out-edge CEL below reads it (the engine routes on - // EDGE conditions — same contract as BudgetApprovalFlow's gate). + // `limit > 1` is what makes this a LIST read (`find`, not `findOne`) — the + // executor has no `mode` key, so a `mode: 'records'` used to sit here doing + // nothing while the comment credited it for the behaviour `limit` actually + // produced (found by the #4045 unknown-config-key check). `outputVariable` + // then lands the array in the variable context, where the out-edge CEL below + // reads it — the engine routes on EDGE conditions, same contract as + // BudgetApprovalFlow's gate. config: { objectName: 'showcase_inquiry', filter: { status: 'closed' }, - mode: 'records', limit: 200, outputVariable: 'closedInquiries', }, diff --git a/packages/formula/src/index.ts b/packages/formula/src/index.ts index babcdde9e3..08f5c3ecad 100644 --- a/packages/formula/src/index.ts +++ b/packages/formula/src/index.ts @@ -28,7 +28,7 @@ export type { CelFilterCompileResult, CelFilterCompileOptions, CelFilterFailReas export { matchesFilterCondition } from './matches-filter'; // ADR-0032 — shared validator + introspection (one validator for build, // registration, and the agent-callable validate_expression tool). -export { validateExpression, introspectScope, expectedDialect, inferExpressionType, CEL_STDLIB_FUNCTIONS } from './validate'; +export { validateExpression, introspectScope, expectedDialect, inferExpressionType, nearestName, CEL_STDLIB_FUNCTIONS } from './validate'; export type { FieldRole, ExprInput, ExprSchemaHint, ExprValidationError, ExprValidationResult, InferredValueType } from './validate'; export type { SeedValue, SeedPrimitive } from './seed-eval'; export type { DialectEngine, EvalContext, EvalResult, EvalError } from './types'; diff --git a/packages/formula/src/validate.ts b/packages/formula/src/validate.ts index 1fd6dd92a0..1b9999e97b 100644 --- a/packages/formula/src/validate.ts +++ b/packages/formula/src/validate.ts @@ -216,6 +216,20 @@ function checkFieldExistence(source: string, schema: ExprSchemaHint | undefined, } /** Cheap edit-distance suggestion for typo'd field names. */ +/** + * The closest candidate to `name`, or `undefined` when nothing is close enough + * to be worth suggesting. + * + * The public face of the same edit-distance heuristic this module already uses + * for unknown field refs and unknown roles, exported so other "did you mean?" + * diagnostics reuse one threshold instead of each inventing their own — an + * author (increasingly an agent) should not get a suggestion here and silence + * there for the same class of typo. + */ +export function nearestName(name: string, candidates: readonly string[]): string | undefined { + return nearest(name, candidates); +} + function nearest(name: string, candidates: readonly string[]): string | undefined { let best: string | undefined; let bestD = Infinity; diff --git a/packages/services/service-automation/src/builtin/config-unknown-keys.test.ts b/packages/services/service-automation/src/builtin/config-unknown-keys.test.ts new file mode 100644 index 0000000000..ca2893e728 --- /dev/null +++ b/packages/services/service-automation/src/builtin/config-unknown-keys.test.ts @@ -0,0 +1,159 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Unknown flow-node `config` keys are reported at registration (#4045). + * + * `FlowNodeSchema.config` is `z.record(z.unknown())`, so before this a misspelled + * or invented config key was accepted in **total silence**: `visibleIf` instead of + * `visibleWhen` registered cleanly, was never read, and the only symptom was a + * feature that quietly did not happen. That is the diagnostic vacuum that made + * #3528 take three passes and two wrong diagnoses. + * + * Warn, never reject — see `validateNodeConfigKeys` for why. These tests pin both + * halves of that: the warning fires with an actionable suggestion, AND + * registration still succeeds. + */ + +import { describe, it, expect } from 'vitest'; +import { AutomationEngine } from '../engine.js'; +import { installBuiltinNodes } from './index.js'; + +function recordingLogger() { + const warnings: string[] = []; + const logger: any = { + info() {}, error() {}, debug() {}, + warn(msg: string) { warnings.push(msg); }, + child() { return logger; }, + }; + return { logger, warnings }; +} + +function engineWith(logger: any) { + const engine = new AutomationEngine(logger); + installBuiltinNodes(engine, { logger, getService() { throw new Error('none'); } } as any); + return engine; +} + +/** A one-node flow carrying `config` verbatim on a node of `type`. */ +function flowWith(type: string, config: Record) { + return { + name: 'f', label: 'F', type: 'screen', status: 'active', version: 1, + nodes: [ + { id: 'start', type: 'start', label: 'S', config: {} }, + { id: 'n1', type, label: 'N', config }, + { id: 'end', type: 'end', label: 'E' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'n1', type: 'default' }, + { id: 'e2', source: 'n1', target: 'end', type: 'default' }, + ], + }; +} + +describe('unknown node config keys (#4045)', () => { + it('flags a misspelled key with a did-you-mean suggestion', () => { + const { logger, warnings } = recordingLogger(); + const engine = engineWith(logger); + + // The typo lives INSIDE the field repeater — where the real #3528 one did. + engine.registerFlow('f', flowWith('screen', { + fields: [{ name: 'opportunityName', required: true, visibleIf: 'createOpportunity == true' }], + })); + + const hit = warnings.find((w) => w.includes('visibleIf')); + expect(hit, 'the typo should be reported').toBeDefined(); + expect(hit).toContain("node 'n1'"); + expect(hit).toContain('(screen)'); + // Located to the exact element, so the author knows WHICH field. + expect(hit).toContain('config.fields[0].visibleIf'); + // The load-bearing half for an agent author: the correct key is named. Note + // it comes from the DECLARED SET, not the suggestion — `visibleIf` → + // `visibleWhen` is edit-distance 4 against `nearestName`'s threshold of 3, so + // this exact typo gets no did-you-mean. Printing the declared set is what + // makes the diagnostic actionable regardless. + expect(hit).toContain('Declared here: name, label, type, required, visibleWhen.'); + }); + + it('adds a did-you-mean when the typo IS within edit distance', () => { + const { logger, warnings } = recordingLogger(); + const engine = engineWith(logger); + + engine.registerFlow('f', flowWith('screen', { titl: 'Details' })); + + const hit = warnings.find((w) => w.includes('titl')); + expect(hit).toContain('did you mean `title`?'); + // …and still lists the declared set alongside it. + expect(hit).toContain('Declared here:'); + }); + + it('still registers the flow — warn, never reject', () => { + const { logger } = recordingLogger(); + const engine = engineWith(logger); + expect(() => engine.registerFlow('f', flowWith('screen', { visibleIf: 'a == true' }))).not.toThrow(); + // And it is really registered, not swallowed. + expect(engine.getFlow('f')).toBeDefined(); + }); + + it('lists the declared keys when nothing is close enough to suggest', () => { + const { logger, warnings } = recordingLogger(); + const engine = engineWith(logger); + + engine.registerFlow('f', flowWith('screen', { totallyMadeUp: 42 })); + + const hit = warnings.find((w) => w.includes('totallyMadeUp')); + expect(hit).toBeDefined(); + expect(hit).not.toContain('did you mean'); + // The declared set is always present, so there is always somewhere to go. + expect(hit).toContain('Declared here:'); + expect(hit).toContain('fields'); + }); + + it('reports every undeclared key, not just the first', () => { + const { logger, warnings } = recordingLogger(); + const engine = engineWith(logger); + + engine.registerFlow('f', flowWith('screen', { hideWhen: 'x', submitLabel: 'Go' })); + + expect(warnings.some((w) => w.includes('hideWhen'))).toBe(true); + expect(warnings.some((w) => w.includes('submitLabel'))).toBe(true); + }); + + it('stays quiet on a fully declared config', () => { + const { logger, warnings } = recordingLogger(); + const engine = engineWith(logger); + + engine.registerFlow('f', flowWith('screen', { + title: 'Details', + fields: [{ name: 'a', type: 'boolean', required: true, visibleWhen: 'b == true' }], + waitForInput: true, + defaults: { x: 1 }, + })); + + expect(warnings.filter((w) => w.includes('unknown config key'))).toEqual([]); + }); + + it('stays quiet for a node type that publishes no configSchema', () => { + // `decision` / `script` are deliberately schemaless (config-schemas.test.ts): + // nothing is declared, so nothing can be undeclared. + const { logger, warnings } = recordingLogger(); + const engine = engineWith(logger); + + engine.registerFlow('f', flowWith('decision', { condition: 'a == b', whateverElse: 1 })); + + expect(warnings.filter((w) => w.includes('unknown config key'))).toEqual([]); + }); + + it('does not flag the keys of a keyValue map — those are author data', () => { + // The walk descends where the schema declares structure and STOPS at a + // free-form map: `filter: { status: 'stale' }` keys are data, not config keys. + const { logger, warnings } = recordingLogger(); + const engine = engineWith(logger); + + engine.registerFlow('f', flowWith('get_record', { + objectName: 'crm_lead', + filter: { status: 'stale', anythingAtAll: true }, + })); + + expect(warnings.filter((w) => w.includes('unknown config key'))).toEqual([]); + }); +}); diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 2977ba9ab9..12cf222c49 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -17,7 +17,18 @@ import { ConnectorSchema } from '@objectstack/spec/integration'; // silently returned `false`, so EVERY start-node / edge condition (record-change // `previous.*`, `budget > 100000`, …) skipped its flow. A static import binds the // engine at module load in both ESM and CJS builds. -import { ExpressionEngine, validateExpression } from '@objectstack/formula'; +import { ExpressionEngine, validateExpression, nearestName } from '@objectstack/formula'; + +/** + * The slice of a descriptor's JSON-Schema `configSchema` that the undeclared-key + * walk reads (#4045). Structural only — no validation semantics. + */ +interface ConfigSchemaNode { + type?: string; + properties?: Record; + items?: ConfigSchemaNode; + additionalProperties?: unknown; +} import { runIsUnscopedUserMode, flowTouchesData } from './runtime-identity.js'; import { isGuardRefusal } from './guard-refusal.js'; @@ -1473,6 +1484,12 @@ export class AutomationEngine implements IAutomationService { // executeNode() already throws NO_EXECUTOR at run time for unknown types. this.validateNodeTypes(name, parsed); + // #4045 — warn on config keys the node's descriptor does not declare + // (a `visibleIf` typo is silently accepted today). Warn-only: see + // validateNodeConfigKeys for why hard-failing would gamble on the nine + // builtins whose read-vs-declared drift has not been audited. + this.validateNodeConfigKeys(name, parsed); + // ADR-0032 §Decision 1a — parse-validate every predicate at registration, // so a malformed condition (e.g. the #1491 `{record.x}` template-brace-in- // CEL mistake) is a LOUD registration error with the offending source, @@ -2717,6 +2734,107 @@ export class AutomationEngine implements IAutomationService { } } + /** + * Warn about node `config` keys the node type's descriptor does not declare + * (#4045 — the warn half of the unknown-key ladder). + * + * `FlowNodeSchema.config` is `z.record(z.unknown())`, so a misspelled or + * invented config key is accepted in total silence today: `visibleIf` instead + * of `visibleWhen` registers cleanly and then does nothing, which is exactly + * the failure shape that made #3528 take three passes to diagnose. The key is + * never read, so there is no runtime error to trace back — the only symptom + * is a feature that quietly does not happen. + * + * **Warn, never reject.** An undeclared key falls into three populations and + * this seam cannot yet tell them apart: an author typo (which we want to + * reject), a key the executor genuinely reads that its hand-written + * `configSchema` never declared (`notify.source` was exactly this until + * #4045 — rejecting those breaks working apps), and dead config nobody reads + * (harmless). Only 4 of the 13 schema-carrying builtins have been audited for + * the second population, so hard-failing here would gamble on the 9 that have + * not. The warning is what measures that distribution; tightening to an error + * is a later, per-key decision once the data exists, and belongs with a + * tombstone that carries the prescription (the `UNKNOWN_KEY_GUIDANCE` pattern + * in `object.zod.ts`). + * + * Soft-fail matches {@link validateNodeTypes} directly above — same function, + * same `logger.warn` channel, same reasoning about not breaking flows over a + * diagnostic. Nothing about the published `configSchema` changes, so no + * consumer (designer form generation included) sees a different shape. + */ + private validateNodeConfigKeys(flowName: string, flow: FlowParsed): void { + for (const node of flow.nodes) { + const schema = this.actionDescriptors.get(node.type)?.configSchema as ConfigSchemaNode | undefined; + // No descriptor, or a deliberately schemaless type (`decision`, + // `script`, `wait`, `subflow` publish none — see config-schemas.test) + // ⇒ nothing is declared, so nothing can be undeclared. + if (!schema) continue; + this.warnUndeclaredConfigKeys(flowName, node, schema, node.config, 'config'); + } + } + + /** + * Walk `value` against `schema` in lockstep, warning on keys the schema does + * not declare. + * + * Descends **only where the schema declares structure** — an object with fixed + * `properties`, or an array whose `items` do. It deliberately stops at a + * keyValue map (`additionalProperties: true` with no `properties`): those keys + * are author *data* (`filter: { status: 'stale' }`), not config keys, and + * flagging them would make the check useless noise. + * + * Descending matters rather than being thoroughness for its own sake: the + * #3528 typo class lives *inside* the `screen` field repeater, not at the top + * level. `visibleWhen` is a property of `fields[].items`, so a top-level-only + * comparison would miss `visibleIf` — the exact mistake this check exists to + * catch — while still reporting the rarer top-level ones. A test pins it. + */ + private warnUndeclaredConfigKeys( + flowName: string, + node: FlowNodeParsed, + schema: ConfigSchemaNode, + value: unknown, + path: string, + ): void { + if (schema.type === 'array' && schema.items) { + if (!Array.isArray(value)) return; + value.forEach((element, index) => { + this.warnUndeclaredConfigKeys(flowName, node, schema.items!, element, `${path}[${index}]`); + }); + return; + } + + // A free-form map declares no properties — its keys are author data. + if (!schema.properties || schema.additionalProperties === true) return; + if (value == null || typeof value !== 'object' || Array.isArray(value)) return; + + const declared = Object.keys(schema.properties); + for (const key of Object.keys(value as Record)) { + const child = schema.properties[key]; + if (child) { + this.warnUndeclaredConfigKeys(flowName, node, child, (value as Record)[key], `${path}.${key}`); + continue; + } + // The declared set is printed ALWAYS, not only as a fallback when the + // edit-distance heuristic misses. It misses more than you would + // expect: `visibleIf` → `visibleWhen` is distance 4 against a + // threshold of 3, so the very typo this check exists to catch gets no + // suggestion. Loosening `nearestName` is the wrong fix — it is shared + // with the unknown-field and unknown-role diagnostics, where a looser + // threshold means confidently wrong suggestions over hundreds of + // candidates. A config object declares at most a dozen keys, so simply + // listing them is both cheap and complete, and the suggestion becomes + // a bonus for the cases it does catch. + const suggestion = nearestName(key, declared); + this.logger.warn( + `[flow '${flowName}'] node '${node.id}' (${node.type}): unknown config key \`${key}\` at ${path}.${key}` + + (suggestion ? ` — did you mean \`${suggestion}\`?` : '') + + ` It is not declared by this node type's configSchema, so nothing reads it.` + + ` Declared here: ${declared.join(', ')}.`, + ); + } + } + /** * ADR-0032 §Decision 1a — parse-validate every predicate in the flow at * registration. Predicates are bare CEL; this catches the #1491 class