fix(formula): refuse a non-string expression source through errors[] instead of a raw TypeError

.changeset/expression-source-non-string-refused.md

@@ -0,0 +1,17 @@
+---
+"@objectstack/formula": minor
+---
+
+`validateExpression` now refuses a non-string expression `source` through `errors[]`, instead of throwing a raw `TypeError` that wiped out the caller's located reporting.
+
+`validateExpression(role, input)` accepts `string | { dialect?, source? }`, and read the envelope's `source` unguarded — `if (!source.trim())`. `ExprInput` declares `source?: string`, but every production call site casts, because the value comes out of **metadata**, where a declaration is a claim about stored data and not a guarantee about it. An envelope whose `source` was present and not a string therefore threw `TypeError: source.trim is not a function` out of a validator whose own docblock promises it never throws.
+
+**The defect was not "it throws" — it was that it threw the wrong kind and bypassed a whole located-reporting contract.** `AutomationEngine.validateFlowExpressions` collects located findings and throws one assembled error naming the flow, the node, the slot and the source (ADR-0032 §1d); `@objectstack/lint`'s stack walk attributes every finding to the hook, sharing rule, action or field it came from. An exception raised *inside* the shared validator skipped both, so the author was handed an internal message naming none of them. Measured before the fix, on a stack whose `hooks[].condition` was `{ source: { nested: 1 } }`: the whole `objectstack validate` run died on `source.trim is not a function`. After: one located `error` reading ``hook 'gate_hook' (lead) condition``.
+
+The guard sits at `toSource`, the entry `validateExpression` and `inferExpressionType` share — **once**, not in each caller's own `try`/`catch`, which is the tolerant-consumer shape Prime Directive #12 forbids. `validateExpression` returns `ok: false` with one `ExprValidationError` naming what was found and both authorable forms; `inferExpressionType` answers `'unknown'`, its existing "cannot prove a type".
+
+**No exported symbol or signature moves** — measured by diffing the built `dist/index.d.ts` before and after: 39 exported declarations on both sides, and `validateExpression`'s declaration byte-identical. What changes is behaviour at a published entry, which is why this is `minor` rather than `patch`: an input that previously produced **no verdict at all** now produces a rejection.
+
+**What does not change.** Absent, `null`, empty and whitespace-only sources still read as "not authored" (`ok: true`), an `{ ast }` envelope carrying no `source` is still admitted (its admission is `ExpressionSchema`'s rule, not this entry's), and a malformed *string* still gets its own diagnostic — the brace trap, the dialect mismatch, the unknown function — never the shape refusal. No input that previously returned `ok: true` now returns `ok: false`, and none that returned `ok: false` now returns `ok: true`.
+
+A caller that relied on catching the `TypeError` would need to read `result.ok` instead. None does: all nine production call sites (`@objectstack/lint` ×4, its docs gate ×2, `@objectstack/service-automation` ×3) read `.errors`/`.warnings` directly, and the one call site inside a `try` (`@objectstack/mcp`'s `validate_expression` tool) has a handler-level catch that degrades to an error result and declares its `expression` parameter `z.string()`.

packages/formula/src/validate-nonstring-source.test.ts

@@ -0,0 +1,159 @@
+/**
+ * #15663 — an expression envelope whose `source` is NOT a string is refused
+ * through `errors[]`, not by throwing a raw `TypeError` out of the validator.
+ *
+ * ## The acceptance test is not "it no longer throws"
+ *
+ * `validateExpression`'s own docblock promises it never throws, and every
+ * consumer is built on that promise: `AutomationEngine.validateFlowExpressions`
+ * collects LOCATED findings and throws one assembled error naming the flow, the
+ * node, the slot and the source (ADR-0032 §1d), and `@objectstack/lint`'s stack
+ * walk attributes each finding to the hook / sharing rule / action it came from.
+ * A `TypeError` escaping from inside the validator bypassed all of it — the
+ * author got `source.trim is not a function` and no location at all.
+ *
+ * So the pin is TWO-sided: the refusal arrives on the `errors[]` channel here,
+ * and the caller's location survives to the author (pinned next door, in
+ * `@objectstack/lint`'s `validate-expressions-nonstring-source.test.ts`).
+ *
+ * ## Why the entry, once
+ *
+ * `validateExpression` is the shared parse every predicate/value slot in the
+ * platform goes through, and the value arrives from METADATA — `ExprInput`
+ * declares `source?: string`, but every production call site casts, because a
+ * declaration is a claim about stored data and not a guarantee about it. The
+ * guard therefore belongs at the entry both public functions share, never in
+ * each caller's own try/catch (Prime Directive #12's tolerant-consumer shape).
+ */
+import { describe, it, expect } from 'vitest';
+
+import { validateExpression, inferExpressionType } from './validate';
+
+/** The refusal sentence, spelled once. Not exported from the package — the
+ *  published surface does not move for this fix; the text travels `errors[]`. */
+const REFUSAL = 'an expression envelope carries its expression as a string `source`';
+
+describe('#15663 — a non-string envelope `source`', () => {
+  describe('is refused through `errors[]`, for every role', () => {
+    const CASES: Array<[label: string, source: unknown, found: string]> = [
+      ['an object', { nested: 1 }, 'found an object'],
+      ['a nested object (the card\'s own value)', { nested: 1 }, 'found an object'],
+      ['a number', 1, 'found a number'],
+      ['an array', ['a'], 'found an array'],
+      ['a boolean', true, 'found a boolean'],
+    ];
+
+    for (const role of ['predicate', 'value', 'template'] as const) {
+      for (const [label, source, found] of CASES) {
+        it(`${role}: refuses ${label} without throwing`, () => {
+          const r = validateExpression(role, { source } as never);
+          expect(r.ok).toBe(false);
+          expect(r.errors).toHaveLength(1);
+          expect(r.errors[0].message).toContain(REFUSAL);
+          expect(r.errors[0].message).toContain(found);
+          // The defect's own signature must be gone from what the author reads.
+          expect(r.errors[0].message).not.toContain('is not a function');
+        });
+      }
+    }
+
+    it('does not throw — the property the whole located-reporting contract rests on', () => {
+      expect(() => validateExpression('predicate', { source: { nested: 1 } } as never)).not.toThrow();
+      expect(() => validateExpression('value', { dialect: 'cel', source: ['a'] } as never)).not.toThrow();
+      expect(() => validateExpression('template', { source: true } as never)).not.toThrow();
+    });
+
+    it('refuses regardless of the declared dialect — the shape is judged first', () => {
+      for (const dialect of ['cel', 'template', 'cron', undefined]) {
+        const r = validateExpression('predicate', { dialect, source: 1 } as never);
+        expect(r.ok).toBe(false);
+        expect(r.errors[0].message).toContain(REFUSAL);
+      }
+    });
+  });
+
+  describe('the message is self-correcting — it names both authorable forms', () => {
+    it('a CEL role is shown bare CEL and a `cel` envelope', () => {
+      const m = validateExpression('predicate', { source: 1 } as never).errors[0].message;
+      expect(m).toContain('record.rating >= 4');
+      expect(m).toContain("dialect: 'cel'");
+    });
+
+    it('the `template` role is shown a TEMPLATE, not a CEL predicate', () => {
+      const m = validateExpression('template', { source: 1 } as never).errors[0].message;
+      expect(m).toContain('{{ record.name }}');
+      expect(m).toContain("dialect: 'template'");
+      // Prescribing bare CEL to a text template is advice that cannot succeed.
+      expect(m).not.toContain('record.rating >= 4');
+    });
+
+    it('attributes to the empty string — the value that WOULD be the location is the value being refused', () => {
+      // `source` is declared `string` on `ExprValidationError` and every caller
+      // renders it; echoing the offending non-string would put an object into it.
+      expect(validateExpression('predicate', { source: { nested: 1 } } as never).errors[0].source).toBe('');
+    });
+  });
+
+  describe('`inferExpressionType` — the SECOND consumer of the same entry', () => {
+    it('answers `unknown` instead of throwing', () => {
+      expect(() => inferExpressionType({ source: 1 } as never)).not.toThrow();
+      expect(inferExpressionType({ source: 1 } as never)).toBe('unknown');
+      expect(inferExpressionType({ dialect: 'cel', source: ['a'] } as never)).toBe('unknown');
+    });
+
+    it('CONTROL — a real numeric expression still infers `number`', () => {
+      expect(inferExpressionType({ dialect: 'cel', source: '1 + 1' })).toBe('number');
+      expect(inferExpressionType('1 + 1')).toBe('number');
+    });
+  });
+
+  /**
+   * CONTROLS. The reject set and the accept set of everything that already
+   * RETURNED are unchanged by this fix — only the population that used to
+   * produce no verdict at all (a crash) moved, and it moved into the reject set.
+   * These are the shapes that must keep their existing verdict exactly.
+   */
+  describe('CONTROLS — what must NOT change', () => {
+    it('bare text still validates', () => {
+      expect(validateExpression('predicate', 'record.rating >= 4').ok).toBe(true);
+      expect(validateExpression('value', 'record.amount / 100').ok).toBe(true);
+      expect(validateExpression('template', 'Hi {{ record.name }}').ok).toBe(true);
+    });
+
+    it('a well-formed envelope still validates', () => {
+      expect(validateExpression('predicate', { dialect: 'cel', source: '1 == 1' }).ok).toBe(true);
+      expect(validateExpression('predicate', { source: '1 == 1' }).ok).toBe(true);
+    });
+
+    it('"not authored" still reads as `ok: true` — absent, null, empty, whitespace', () => {
+      expect(validateExpression('predicate', null).ok).toBe(true);
+      expect(validateExpression('predicate', undefined).ok).toBe(true);
+      expect(validateExpression('predicate', '').ok).toBe(true);
+      expect(validateExpression('predicate', '   ').ok).toBe(true);
+      expect(validateExpression('predicate', {}).ok).toBe(true);
+      expect(validateExpression('predicate', { dialect: 'cel' }).ok).toBe(true);
+      expect(validateExpression('predicate', { source: undefined }).ok).toBe(true);
+      // A NULL `source` is "not authored" too, and stays so: only a PRESENT
+      // non-string is the fourth population this card refuses.
+      expect(validateExpression('predicate', { source: null } as never).ok).toBe(true);
+    });
+
+    it('an `{ ast }` envelope carrying no `source` is still admitted', () => {
+      // Its admission is `ExpressionSchema`'s rule, not this entry's; the guard
+      // must not start refusing it as "a non-string source".
+      expect(validateExpression('predicate', { ast: { kind: 'whatever' } } as never).ok).toBe(true);
+    });
+
+    it('a malformed STRING still gets its own diagnostic, not the shape refusal', () => {
+      const brace = validateExpression('predicate', '{record.rating} >= 4');
+      expect(brace.ok).toBe(false);
+      expect(brace.errors[0].message).not.toContain(REFUSAL);
+      expect(brace.errors[0].message).toContain('template brace');
+      expect(brace.errors[0].source).toBe('{record.rating} >= 4');
+
+      const dialect = validateExpression('template', { dialect: 'cel', source: 'record.x' });
+      expect(dialect.ok).toBe(false);
+      expect(dialect.errors[0].message).not.toContain(REFUSAL);
+    });
+  });
+});

packages/formula/src/validate.ts

@@ -197,12 +197,59 @@ export function expectedDialect(role: FieldRole): 'cel' | 'template' {
   return role === 'template' ? 'template' : 'cel';
 }
 
-function toSource(input: ExprInput): { dialect?: string; source: string } {
+/** `an array` / `an object` / `a number` — never called with a string or nullish. */
+function describeNonStringSource(value: unknown): string {
+  if (Array.isArray(value)) return 'an array';
+  if (typeof value === 'object') return 'an object';
+  return `a ${typeof value}`;
+}
+
+/**
+ * Normalize the three authorable spellings — bare text, an envelope, absent —
+ * into the dialect and the source text the checks below read.
+ *
+ * `nonStringSource` is the fourth population, which is not authorable at all: an
+ * envelope whose `source` is present but is NOT a string. `ExprInput` declares
+ * `source?: string`, so this cannot arrive through a typed call — but every
+ * production call site casts, because the value comes out of METADATA, where the
+ * declaration is a claim and not a guarantee. Read unguarded it reached
+ * `source.trim()` and threw a bare `TypeError: source.trim is not a function`
+ * out of a validator whose whole contract is that it never throws: callers that
+ * exist to collect LOCATED findings (`AutomationEngine.validateFlowExpressions`,
+ * `@objectstack/lint`'s stack walk) were bypassed entirely, so `registerFlow`
+ * died on an internal message naming neither the flow nor the node, and
+ * `objectstack validate` died naming neither the hook nor the sharing rule.
+ *
+ * The refusal is reported HERE, once, at the entry every predicate/value slot in
+ * the platform shares — never by each caller wrapping the call in a try/catch,
+ * which is the tolerant-consumer shape Prime Directive #12 forbids.
+ *
+ * ⚠️ Only a PRESENT non-string qualifies. `null` / `undefined` / a missing
+ * `source` still normalize to `''` and still read as "not authored" (`ok: true`),
+ * unchanged — including the `{ ast }` envelope, which carries no `source` at all
+ * and whose admission is `ExpressionSchema`'s rule, not this function's.
+ */
+function toSource(input: ExprInput): { dialect?: string; source: string; nonStringSource?: string } {
   if (input == null) return { source: '' };
   if (typeof input === 'string') return { source: input };
-  return { dialect: input.dialect, source: input.source ?? '' };
+  const raw = (input as { source?: unknown }).source;
+  if (raw == null) return { dialect: input.dialect, source: '' };
+  if (typeof raw !== 'string') {
+    return { dialect: input.dialect, source: '', nonStringSource: describeNonStringSource(raw) };
+  }
+  return { dialect: input.dialect, source: raw };
 }
 
+/**
+ * The refusal text for a non-string envelope `source`, shared by the message
+ * every role composes. Deliberately NOT exported: the published surface of
+ * `@objectstack/formula` does not move for this fix — the refusal travels the
+ * `errors[]` channel that already exists, so no consumer needs a new symbol to
+ * read it.
+ */
+const NON_STRING_SOURCE_REFUSAL =
+  'an expression envelope carries its expression as a string `source`';
+
 function bracesHint(source: string): string | null {
   const m = SINGLE_BRACE_RE.exec(source);
   if (!m) return null;
@@ -549,15 +596,34 @@ function checkRoleCatalog(
  * Validate one expression for a given field role. Never throws — returns a
  * structured result. Call sites decide whether to throw (build/registration)
  * or report (agent tool).
+ *
+ * "Never throws" is the contract, and it now holds for the one input that used
+ * to break it: an envelope whose `source` is not a string is refused through
+ * `errors[]` like any other malformed expression, so the caller's located
+ * reporting survives to the author. See {@link toSource}.
  */
 export function validateExpression(
   role: FieldRole,
   input: ExprInput,
   schema?: ExprSchemaHint,
 ): ExprValidationResult {
-  const { dialect, source } = toSource(input);
+  const { dialect, source, nonStringSource } = toSource(input);
   const errors: ExprValidationError[] = [];
   const warnings: ExprValidationError[] = [];
+  if (nonStringSource !== undefined) {
+    // Attributed to the empty string, deliberately: the value that would be the
+    // location IS the value being refused, so echoing it would put a non-string
+    // into a `source: string` slot every caller renders.
+    errors.push({
+      source: '',
+      message:
+        `invalid ${role} envelope: ${NON_STRING_SOURCE_REFUSAL} — found ${nonStringSource}. ` +
+        `Write the expression as bare text (e.g. ${role === 'template' ? '`Hi {{ record.name }}`' : '`record.rating >= 4`'}), ` +
+        `or as an envelope whose \`source\` is that text ` +
+        `(e.g. \`{ dialect: '${expectedDialect(role)}', source: '…' }\`).`,
+    });
+    return { ok: false, errors, warnings };
+  }
   if (!source.trim()) return { ok: true, errors, warnings };
 
   if (role === 'template') {
@@ -735,7 +801,12 @@ function celTypeToValueType(celType: string | null): InferredValueType {
  * construction — see {@link inferCelType}.
  */
 export function inferExpressionType(input: ExprInput, schema?: ExprSchemaHint): InferredValueType {
-  const { source } = toSource(input);
+  const { source, nonStringSource } = toSource(input);
+  // The second consumer of the shared entry, and it crashed identically. There
+  // is no `errors[]` here to route a refusal through, and this function is
+  // conservative by construction: a source it cannot read is a type it cannot
+  // prove, which is exactly what `'unknown'` already means.
+  if (nonStringSource !== undefined) return 'unknown';
   if (!source.trim()) return 'unknown';
   return celTypeToValueType(inferCelType(source, schema?.fields));
 }