diff --git a/.changeset/expression-source-non-string-refused.md b/.changeset/expression-source-non-string-refused.md new file mode 100644 index 0000000000..083d5c2b03 --- /dev/null +++ b/.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()`. diff --git a/packages/formula/src/validate-nonstring-source.test.ts b/packages/formula/src/validate-nonstring-source.test.ts new file mode 100644 index 0000000000..8ed6a26be1 --- /dev/null +++ b/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); + }); + }); +}); diff --git a/packages/formula/src/validate.ts b/packages/formula/src/validate.ts index e2b884a843..fb85ec3b5c 100644 --- a/packages/formula/src/validate.ts +++ b/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)); } diff --git a/packages/lint/src/validate-expressions-nonstring-source.test.ts b/packages/lint/src/validate-expressions-nonstring-source.test.ts new file mode 100644 index 0000000000..92d2cf706e --- /dev/null +++ b/packages/lint/src/validate-expressions-nonstring-source.test.ts @@ -0,0 +1,132 @@ +/** + * #15663, downstream half — the caller's LOCATION survives to the author. + * + * The entry-side pin lives in `@objectstack/formula` + * (`validate-nonstring-source.test.ts`) and says the refusal arrives on the + * `errors[]` channel. That alone is not the acceptance test. What made this a + * p2 rather than a cosmetic crash is that a raw `TypeError` escaping from + * inside `validateExpression` bypassed the whole located-reporting contract: + * this walk exists to attribute every finding to the hook / sharing rule / + * action / field it came from, and an exception thrown out of the shared + * validator took the entire run down instead, naming none of them. + * + * ## The population pinned here is the one the adjacent cards did NOT close + * + * #15572 refuses a non-string in the LEDGER-DECLARED predicate slots and #15662 + * refuses one in the STRUCTURAL condition slots (`config.condition`, + * `edge.condition`), both before `validateExpression` is reached — so on those + * slots the crash was already unreachable when this card was picked up + * (measured, not assumed: the card's own driven `registerFlow` repro no longer + * reproduces). Every OTHER slot this walk visits still arrives at the shared + * entry with whatever the metadata holds, and had no guard at all. Hooks and + * sharing rules are two of them, and they are what these pins drive. + */ +import { describe, it, expect } from 'vitest'; + +import { validateStackExpressions } from './validate-expressions'; + +const REFUSAL = 'an expression envelope carries its expression as a string `source`'; + +/** The card's own value, plus the rest of the non-string population. */ +const NON_STRING_SOURCES: Array<[label: string, value: unknown]> = [ + ['a nested object', { source: { nested: 1 } }], + ['a number', { source: 1 }], + ['an array', { dialect: 'cel', source: ['a'] }], + ['a boolean', { source: true }], +]; + +describe('#15663 — a non-string envelope `source` in a walked slot', () => { + describe("a lifecycle hook's `condition`", () => { + for (const [label, value] of NON_STRING_SOURCES) { + it(`reports ${label} as a located finding instead of crashing the run`, () => { + const issues = validateStackExpressions({ + hooks: [{ name: 'gate_hook', object: 'lead', condition: value }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain(REFUSAL); + expect(issues[0].severity).toBe('error'); + // The whole point: the author is told WHERE. + expect(issues[0].where).toBe("hook 'gate_hook' (lead) condition"); + expect(issues[0].message).not.toContain('is not a function'); + }); + } + + it('does not throw — a TypeError here took the whole walk down', () => { + expect(() => + validateStackExpressions({ + hooks: [{ name: 'gate_hook', object: 'lead', condition: { source: { nested: 1 } } }], + }), + ).not.toThrow(); + }); + }); + + describe("a sharing rule's `condition`", () => { + it('reports a located finding naming the rule and its object', () => { + const issues = validateStackExpressions({ + sharingRules: [{ name: 'wide_open', object: 'lead', condition: { source: 1 } }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain(REFUSAL); + expect(issues[0].where).toBe("sharingRule 'wide_open' (lead) condition"); + }); + }); + + describe("an action's `visible` predicate", () => { + it('reports a located finding naming the object and the action', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'lead', + actions: [{ name: 'promote', visible: { source: ['a'] } }], + }], + }); + expect(issues.length).toBeGreaterThanOrEqual(1); + const refusals = issues.filter((i) => i.message.includes(REFUSAL)); + expect(refusals).toHaveLength(1); + expect(refusals[0].where).toContain("action 'promote' visible"); + }); + }); + + describe('the finding does not corrupt the report — `source` stays a string', () => { + it('every issue carries a string `source`, so a renderer cannot be handed an object', () => { + const issues = validateStackExpressions({ + hooks: [{ name: 'gate_hook', object: 'lead', condition: { source: { nested: 1 } } }], + }); + for (const i of issues) expect(typeof i.source).toBe('string'); + }); + }); + + describe('CONTROLS — the walk must be unchanged for everything that already returned', () => { + it('a valid predicate on the same slot still reports nothing', () => { + expect( + validateStackExpressions({ + hooks: [{ name: 'gate_hook', object: 'lead', condition: 'record.rating >= 4' }], + }), + ).toHaveLength(0); + }); + + it('a well-formed ENVELOPE on the same slot still reports nothing', () => { + expect( + validateStackExpressions({ + hooks: [{ name: 'gate_hook', object: 'lead', condition: { dialect: 'cel', source: '1 == 1' } }], + }), + ).toHaveLength(0); + }); + + it('an absent / empty condition still reports nothing', () => { + expect(validateStackExpressions({ hooks: [{ name: 'h', object: 'lead' }] })).toHaveLength(0); + expect( + validateStackExpressions({ hooks: [{ name: 'h', object: 'lead', condition: ' ' }] }), + ).toHaveLength(0); + }); + + it('a malformed STRING still gets its OWN diagnostic — the brace trap, not the shape refusal', () => { + const issues = validateStackExpressions({ + hooks: [{ name: 'gate_hook', object: 'lead', condition: '{record.rating} >= 4' }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].message).not.toContain(REFUSAL); + expect(issues[0].message).toContain('template brace'); + expect(issues[0].source).toBe('{record.rating} >= 4'); + }); + }); +});