fix(automation,lint,spec): validate the expression slots a node's configSchema declares (#4027)

.changeset/flow-node-expression-ledger.md

@@ -0,0 +1,48 @@
+---
+'@objectstack/spec': minor
+'@objectstack/service-automation': minor
+'@objectstack/lint': minor
+---
+
+Validate the expression slots a flow node's `configSchema` declares (#4027).
+
+A node type's designer `configSchema` and the keys its validators traverse were
+two unreconciled lists. Both the engine's `registerFlow` pass and the author-time
+`objectstack validate` pass hardcoded `config.condition` / `edge.condition` and
+assumed every other node string was a `{var}` template — so a declared expression
+property outside that hardcoded set was validated by nobody.
+
+That is how #3528 shipped. `screen.fields[].visibleWhen` has been on the `screen`
+descriptor since #3304, typed `xExpression: 'expression'` (bare CEL) and offered
+to authors in Studio, but no validator traversed it. An app authored the
+predicate in the *other* dialect — `'{createOpportunity} == true'` — and it passed
+`tsc`, `objectstack validate` and registration in silence. Because `required` *is*
+enforced, a field the author had made conditional rendered unconditionally and
+blocked Submit on an input the user was never shown: the run paused forever and no
+resume was ever issued.
+
+Now:
+
+- **`FLOW_NODE_EXPRESSION_PATHS`** (`@objectstack/spec`) is the declared ledger of
+  expression-bearing node config paths, each recording the dialect it takes.
+- **Both validators read it.** A malformed `visibleWhen` is a located, quoted
+  error at `registerFlow` *and* at `objectstack validate` — `node 'screen_1'
+  (screen) screen field visibleWhen at config.fields[1].visibleWhen`.
+- **A reconciliation ratchet** derives the expression properties from the live
+  descriptors and fails CI in both directions: a new `xExpression` property with
+  no ledger entry, or a stale entry no descriptor declares. It walks every
+  registered builtin, not just `screen`.
+
+Dialects are recorded rather than assumed because there are three, and two of them
+disagree about braces: bare CEL (`{…}` is the #1491 brace-trap), single-brace
+`{var}` flow interpolation (`{…}` is correct), and the ADR-0032 §3 double-brace
+text template. Only bare-CEL slots are checked — `loop.collection` and
+`map.collection` are recorded as `flow-template` and deliberately left alone,
+since no validator implements their dialect and checking them under either of the
+other two would reject every currently-valid flow.
+
+`ActionDescriptor.configSchema`'s TSDoc no longer claims `registerFlow()`
+validates `config` against it. It never did: `FlowNodeSchema.config` is
+`z.record(z.unknown())`, so types, `required`, `enum` and unknown keys are still
+unenforced. The doc now states exactly what is checked and what is designer-facing
+only, so nothing relies on a guard that does not exist.

content/docs/references/automation/node-executor.mdx

@@ -70,7 +70,7 @@ Canonical cross-paradigm action/node descriptor (ADR-0018)
 | **icon** | `string` | optional | Icon id resolved by the designer |
 | **category** | `Enum<'logic' \| 'data' \| 'io' \| 'human' \| 'control' \| 'custom'>` | ✅ | Palette category |
 | **paradigms** | `Enum<'flow' \| 'approval'>[]` | ✅ | Authoring surfaces that may offer this action |
-| **configSchema** | `any` | optional | JSON Schema for the node config (drives form + parse validation) |
+| **configSchema** | `any` | optional | JSON Schema for the node config (drives the designer form; only declared expression slots are validated) |
 | **supportsPause** | `boolean` | ✅ | Supports async pause/resume |
 | **supportsCancellation** | `boolean` | ✅ | Supports cancellation |
 | **supportsRetry** | `boolean` | ✅ | Supports retry on failure |

packages/lint/src/validate-expressions.test.ts

@@ -521,4 +521,92 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => {
 
   // The ADR-0062 D7 `field.columnName`-on-external-objects lint was removed with
   // `field.columnName` itself (#2377): external column mapping is `external.columnMap`.
+
+  /**
+   * Descriptor-declared expression slots (#4027). Before this, the flow walk
+   * hardcoded `config.condition` and assumed every other node string was a
+   * template, so `screen.fields[].visibleWhen` — declared bare CEL since #3304 —
+   * was validated by nobody and #3528 shipped the wrong dialect through
+   * `objectstack validate` in silence.
+   */
+  describe('descriptor-declared expression slots (#4027)', () => {
+    const screenFlow = (fields: unknown[]) => ({
+      objects,
+      flows: [{
+        name: 'lead_conversion',
+        nodes: [
+          { id: 'start', type: 'start', config: { objectName: 'crm_lead' } },
+          { id: 'screen_1', type: 'screen', config: { fields } },
+        ],
+        edges: [],
+      }],
+    });
+
+    it('flags a `{var}` template dialect in a screen field visibleWhen', () => {
+      // The exact predicate HotCRM shipped (#3528).
+      const issues = validateStackExpressions(screenFlow([
+        { name: 'createOpportunity', type: 'boolean', required: true },
+        { name: 'opportunityName', type: 'text', required: true, visibleWhen: '{createOpportunity} == true' },
+      ]));
+      const found = issues.filter(i => i.where.includes('visibleWhen'));
+      expect(found).toHaveLength(1);
+      expect(found[0].severity).toBe('error');
+      expect(found[0].where).toContain("flow 'lead_conversion'");
+      expect(found[0].where).toContain("node 'screen_1'");
+      // Indexed into the repeater, so the author knows WHICH field.
+      expect(found[0].where).toContain('config.fields[1].visibleWhen');
+      expect(found[0].source).toBe('{createOpportunity} == true');
+    });
+
+    it('passes the corrected bare-CEL predicate', () => {
+      const issues = validateStackExpressions(screenFlow([
+        { name: 'createOpportunity', type: 'boolean', required: true, defaultValue: false },
+        { name: 'opportunityName', type: 'text', required: true, visibleWhen: 'createOpportunity == true' },
+      ]));
+      expect(issues.filter(i => i.where.includes('visibleWhen'))).toHaveLength(0);
+    });
+
+    it('does not report the screen field names as unknown object fields', () => {
+      // A screen's `visibleWhen` binds the screen's OWN collected values, not the
+      // trigger record's fields — so no schema hint is passed. Were one passed,
+      // `createOpportunity` would be flagged as an unknown `crm_lead` field and
+      // every correct predicate would carry a spurious finding.
+      const issues = validateStackExpressions(screenFlow([
+        { name: 'createOpportunity', type: 'boolean' },
+        { name: 'opportunityName', visibleWhen: 'createOpportunity == true' },
+      ]));
+      expect(issues).toHaveLength(0);
+    });
+
+    it('leaves a correct single-brace loop collection alone', () => {
+      // `loop.collection` is the single-brace `{var}` flow-interpolation dialect,
+      // where braces are CORRECT. It is recorded in the ledger as `flow-template`
+      // and deliberately not validated — checking it as a CEL predicate (or as an
+      // ADR-0032 §3 double-brace template) would fail every valid flow.
+      const issues = validateStackExpressions({
+        objects,
+        flows: [{
+          name: 'loop_flow',
+          nodes: [
+            { id: 'start', type: 'start', config: { objectName: 'crm_lead' } },
+            { id: 'loop_1', type: 'loop', config: { collection: '{tasks}', iteratorVariable: 'task' } },
+          ],
+          edges: [],
+        }],
+      });
+      expect(issues).toHaveLength(0);
+    });
+
+    it('tolerates a screen with no fields, a non-array fields, and no config', () => {
+      expect(validateStackExpressions(screenFlow([]))).toHaveLength(0);
+      expect(validateStackExpressions({
+        objects,
+        flows: [{
+          name: 'f',
+          nodes: [{ id: 's', type: 'screen', config: { fields: 'nope' } }, { id: 's2', type: 'screen' }],
+          edges: [],
+        }],
+      })).toHaveLength(0);
+    });
+  });
 });

packages/lint/src/validate-expressions.ts

@@ -9,13 +9,16 @@
  * from `@objectstack/formula`, so the verdict matches `registerFlow` and the
  * agent `validate_expression` tool exactly.
  *
- * Scope (v1): flow predicates (start/decision `config.condition` + edge
- * `condition`), object validation-rule / formula predicates, and UI action
- * `visible` / `disabled` predicates. Each error is located (flow/object/action
- * + node/edge/field) with a corrective message.
+ * Scope: flow predicates (start/decision `config.condition` + edge `condition`),
+ * every **descriptor-declared** expression slot named by
+ * `FLOW_NODE_EXPRESSION_PATHS` (#4027 — e.g. a screen field's `visibleWhen`),
+ * object validation-rule / formula predicates, and UI action `visible` /
+ * `disabled` predicates. Each error is located (flow/object/action +
+ * node/edge/field) with a corrective message.
  */
 
 import { validateExpression } from '@objectstack/formula';
+import { resolveFlowNodeExpressions } from '@objectstack/spec/automation';
 
 export interface ExprIssue {
   where: string;
@@ -111,6 +114,18 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] {
     for (const w of res.warnings) issues.push({ where, message: w.message, source: w.source, severity: 'warning' });
   };
 
+  /**
+   * A declared bare-CEL slot (#4027). No object schema is passed: these slots
+   * bind the *screen's own* collected values, not the trigger record's fields, so
+   * a field-existence pass would report every field name as unknown.
+   */
+  const checkDeclaredPredicate = (where: string, raw: unknown): void => {
+    if (raw == null) return;
+    const res = validateExpression('predicate', raw as string | { dialect?: string; source?: string });
+    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 ──────────────────────────────────────────────────────────
   for (const flow of asArray(stack.flows)) {
     const flowName = typeof flow.name === 'string' ? flow.name : '(unnamed flow)';
@@ -124,6 +139,26 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] {
     for (const node of nodes) {
       const cfg = (node.config ?? {}) as AnyRec;
       check(`flow '${flowName}' · node '${node.id}' (${node.type}) condition`, cfg.condition, objectName);
+
+      // Descriptor-declared expression slots (#4027). Before this, the traversal
+      // hardcoded `condition` and assumed every other node string was a `{var}`
+      // template — so `screen.fields[].visibleWhen`, declared bare CEL since
+      // #3304, was validated by nobody and #3528 shipped a template-dialect
+      // predicate through compile, validate and run time in silence.
+      // Only `predicate` slots are checkable: `flow-template` slots take the
+      // single-brace `{var}` dialect `interpolate()` implements, which no
+      // validator covers (the `template` role enforces ADR-0032 §3's
+      // double-brace text template and would reject every correct
+      // `loop.collection`). The ledger records them regardless, so the
+      // reconciliation ratchet still sees the marker.
+      const nodeType = typeof node.type === 'string' ? node.type : '';
+      for (const found of resolveFlowNodeExpressions(nodeType, cfg)) {
+        if (found.entry.role !== 'predicate') continue;
+        checkDeclaredPredicate(
+          `flow '${flowName}' · node '${node.id}' (${nodeType}) ${found.entry.label} at config.${found.path}`,
+          found.value,
+        );
+      }
       // #1870 — a `script` node must declare a callable target (`actionType` or
       // `function`). A node with neither is a silent no-op that otherwise passes
       // build. (Function *existence* isn't checkable here — functions are code,