From 8b42fe3bc8b834d4e5654fa795fa2895ebe08c25 Mon Sep 17 00:00:00 2001 From: Allen Li Date: Thu, 15 Oct 2026 11:40:51 -0700 Subject: [PATCH 1/4] fix(compiler): emit grouping parentheses from operator precedence Compiled expressions are Python source, but the compiler recovered grouping by looking for parenthesized_expression nodes around each operand in the CST. That missed ternary operands and the targets of member access, subscript and call, and it cannot work for ASTs built without a CST. Add parentheses purely from the shape of the tree, using Python's precedence table: ternary < or < and < not < comparison < additive < multiplicative < unary sign < primary. - Binary operators are left-associative, so an equal-precedence right operand is wrapped (1 - (2 - 3)). - Python chains comparisons, so a comparison that is an operand of a comparison is always wrapped on either side. - Ternaries are right-associative: only the alternative may be an unparenthesized ternary; any other ternary operand is wrapped. - not wraps operands that bind more loosely than not; unary +/- wrap anything below unary. - Targets of member access, subscript and call must be primary. The comparison path also copied the source whitespace around the operator by slicing CST text. Nothing depends on it, so comparisons are now emitted as `left op right`. Redundant source parentheses are no longer echoed, e.g. `(1 + 2)` now compiles to `1 + 2`. Supersedes the CST-based checks from #95; follows up #94. --- .../src/expressions/compile-expression.ts | 135 ++++++++----- packages/compiler/test/expressions.test.ts | 189 +++++++++++++----- .../compiler/test/operator-grouping.test.ts | 109 ++++++++++ 3 files changed, 326 insertions(+), 107 deletions(-) create mode 100644 packages/compiler/test/operator-grouping.test.ts diff --git a/packages/compiler/src/expressions/compile-expression.ts b/packages/compiler/src/expressions/compile-expression.ts index 50674314..afed86ab 100644 --- a/packages/compiler/src/expressions/compile-expression.ts +++ b/packages/compiler/src/expressions/compile-expression.ts @@ -7,6 +7,7 @@ import { type Expression, + type BinaryOperator, type TemplatePart, MemberExpression, AtIdentifier, @@ -31,7 +32,7 @@ import { decomposeAtMemberExpression, escapeStringValue, } from '@agentscript/language'; -import type { Range, SyntaxNode } from '@agentscript/types'; +import type { Range } from '@agentscript/types'; import type { CompilerContext } from '../compiler-context.js'; import { extractNumberValue } from '../ast-helpers.js'; @@ -297,7 +298,7 @@ function compileMemberExpression( } default: { - const obj = compileExprNode(expr.object, ctx, opts); + const obj = compileAccessTarget(expr.object, ctx, opts); return `${obj}.${property}`; } } @@ -308,7 +309,7 @@ function compileMemberExpression( if (expr.property === 'length') { return `len(${obj})`; } - return `${obj}.${expr.property}`; + return `${compileAccessTarget(expr.object, ctx, opts)}.${expr.property}`; } function compileAtIdentifier(expr: AtIdentifier, ctx: CompilerContext): string { @@ -437,7 +438,7 @@ function compileSubscriptExpression( return `system.uploaded_files[${compileIndex()}]`; } - const obj = compileExprNode(expr.object, ctx, opts); + const obj = compileAccessTarget(expr.object, ctx, opts); return `${obj}[${compileIndex()}]`; } @@ -458,33 +459,62 @@ function compileSlice( return `${start}:${stop}`; } -function getParenthesizedCst(expr: Expression): SyntaxNode | undefined { - let node = expr.__cst?.node; - let parenthesized: SyntaxNode | undefined; - // The dialect unwraps grouping and expression/atom nodes from the AST. - // Retain the outermost grouping node for comparison source-text boundaries. - while (node) { - if (node.type === 'parenthesized_expression') parenthesized = node; - const parent = node.parent; - if ( - parent?.type !== 'parenthesized_expression' && - parent?.type !== 'expression' && - parent?.type !== 'atom' - ) { - break; - } - node = parent; +/** + * Operator binding strength, following Python's precedence because compiled + * expressions are evaluated as Python. Higher binds tighter; everything that + * is not an operator expression (literals, references, calls, subscripts, + * member accesses) is primary. + */ +const PRECEDENCE = { + ternary: 1, + or: 2, + and: 3, + not: 4, + comparison: 5, + additive: 6, + multiplicative: 7, + unary: 8, + primary: 9, +} as const; + +function binaryPrecedence(operator: BinaryOperator): number { + switch (operator) { + case 'or': + return PRECEDENCE.or; + case 'and': + return PRECEDENCE.and; + case '+': + case '-': + return PRECEDENCE.additive; + case '*': + case '/': + return PRECEDENCE.multiplicative; } - return parenthesized; } +function precedenceOf(expr: Expression): number { + if (expr instanceof TernaryExpression) return PRECEDENCE.ternary; + if (expr instanceof ComparisonExpression) return PRECEDENCE.comparison; + if (expr instanceof UnaryExpression) { + return expr.operator === 'not' ? PRECEDENCE.not : PRECEDENCE.unary; + } + if (expr instanceof BinaryExpression) return binaryPrecedence(expr.operator); + return PRECEDENCE.primary; +} + +/** + * Compile an operand, adding parentheses when it binds more loosely than + * `minPrecedence`. Grouping comes from the shape of the tree, not from the + * source text, so the output is correct for hand-built ASTs too. + */ function compileOperand( expr: Expression, + minPrecedence: number, ctx: CompilerContext, opts: CompileExpressionOptions ): string { const compiled = compileExprNode(expr, ctx, opts); - return getParenthesizedCst(expr) ? `(${compiled})` : compiled; + return precedenceOf(expr) < minPrecedence ? `(${compiled})` : compiled; } function compileBinaryExpression( @@ -492,8 +522,10 @@ function compileBinaryExpression( ctx: CompilerContext, opts: CompileExpressionOptions ): string { - const left = compileOperand(expr.left, ctx, opts); - const right = compileOperand(expr.right, ctx, opts); + // Left-associative: an equal-precedence right operand needs parentheses. + const precedence = binaryPrecedence(expr.operator); + const left = compileOperand(expr.left, precedence, ctx, opts); + const right = compileOperand(expr.right, precedence + 1, ctx, opts); return `${left} ${expr.operator} ${right}`; } @@ -502,10 +534,10 @@ function compileUnaryExpression( ctx: CompilerContext, opts: CompileExpressionOptions ): string { - const operand = compileOperand(expr.operand, ctx, opts); if (expr.operator === 'not') { - return `not ${operand}`; + return `not ${compileOperand(expr.operand, PRECEDENCE.not, ctx, opts)}`; } + const operand = compileOperand(expr.operand, PRECEDENCE.unary, ctx, opts); return `${expr.operator}${operand}`; } @@ -514,28 +546,11 @@ function compileComparisonExpression( ctx: CompilerContext, opts: CompileExpressionOptions ): string { - const left = compileOperand(expr.left, ctx, opts); - const right = compileOperand(expr.right, ctx, opts); - - // Preserve original whitespace around the operator from the source text - const cstText = expr.__cst?.node?.text; - if (cstText) { - const leftCst = (getParenthesizedCst(expr.left) ?? expr.left.__cst?.node) - ?.text; - const rightCst = (getParenthesizedCst(expr.right) ?? expr.right.__cst?.node) - ?.text; - if (leftCst && rightCst) { - // Extract the operator with surrounding whitespace from the CST - const leftStart = cstText.indexOf(leftCst); - const leftEnd = leftStart + leftCst.length; - const rightStart = cstText.lastIndexOf(rightCst); - if (leftStart >= 0 && rightStart > leftEnd) { - const operatorWithSpace = cstText.slice(leftEnd, rightStart); - return `${left}${operatorWithSpace}${right}`; - } - } - } - + // Python chains comparisons (`a < b < c` is `a < b and b < c`), so a + // comparison operand of a comparison is always parenthesized. + const minPrecedence = PRECEDENCE.comparison + 1; + const left = compileOperand(expr.left, minPrecedence, ctx, opts); + const right = compileOperand(expr.right, minPrecedence, ctx, opts); return `${left} ${expr.operator} ${right}`; } @@ -544,18 +559,34 @@ function compileTernaryExpression( ctx: CompilerContext, opts: CompileExpressionOptions ): string { - const consequence = compileExprNode(expr.consequence, ctx, opts); - const condition = compileExprNode(expr.condition, ctx, opts); - const alternative = compileExprNode(expr.alternative, ctx, opts); + // Right-associative: only the alternative may be an unparenthesized ternary. + const operandMin = PRECEDENCE.ternary + 1; + const consequence = compileOperand(expr.consequence, operandMin, ctx, opts); + const condition = compileOperand(expr.condition, operandMin, ctx, opts); + const alternative = compileOperand( + expr.alternative, + PRECEDENCE.ternary, + ctx, + opts + ); return `${consequence} if ${condition} else ${alternative}`; } +/** The target of `.x`, `[i]` or `(...)` must be primary. */ +function compileAccessTarget( + expr: Expression, + ctx: CompilerContext, + opts: CompileExpressionOptions +): string { + return compileOperand(expr, PRECEDENCE.primary, ctx, opts); +} + function compileCallExpression( expr: CallExpression, ctx: CompilerContext, opts: CompileExpressionOptions ): string { - const func = compileExprNode(expr.func, ctx, opts); + const func = compileAccessTarget(expr.func, ctx, opts); const args = expr.args .map((a: Expression) => compileExprNode(a, ctx, opts)) .join(', '); diff --git a/packages/compiler/test/expressions.test.ts b/packages/compiler/test/expressions.test.ts index 19ac8213..ba7ce716 100644 --- a/packages/compiler/test/expressions.test.ts +++ b/packages/compiler/test/expressions.test.ts @@ -20,6 +20,7 @@ import { AtIdentifier, BinaryExpression, UnaryExpression, + TernaryExpression, ComparisonExpression, SubscriptExpression, SliceExpression, @@ -370,8 +371,23 @@ describe('compileExpression', () => { }); }); - describe('binary expressions', () => { + describe('operator grouping', () => { + function compileSource(source: string): string { + for (const name of ['a', 'b', 'c']) ctx.mutableVariableNames.add(name); + const { rootNode } = parse(`value: ${source}\n`); + const mapping = + rootNode.namedChildren.find(node => node.type === 'mapping') ?? + rootNode; + const result = new Dialect().parse(mapping, { value: ExpressionValue }); + + expect(result.diagnostics).toEqual([]); + const compiled = compileExpression(result.value.value, ctx); + expect(ctx.diagnostics).toEqual([]); + return compiled; + } + it.each([ + // Binary operands keep the grouping they need. ['(1 + 2) * (3 + 4)', '(1 + 2) * (3 + 4)'], ['(1 + 2) * 3', '(1 + 2) * 3'], ['1 * (2 + 3)', '1 * (2 + 3)'], @@ -380,18 +396,126 @@ describe('compileExpression', () => { '(True or False) and (False or False)', '(True or False) and (False or False)', ], - ])('preserves operand grouping in %s', (source, expected) => { - const { rootNode } = parse(`value: ${source}\n`); - const mapping = - rootNode.namedChildren.find(node => node.type === 'mapping') ?? - rootNode; - const result = new Dialect().parse(mapping, { value: ExpressionValue }); + // Redundant parentheses are dropped. + ['(1 + 2)', '1 + 2'], + ['((1 + 2)) * 3', '(1 + 2) * 3'], + ['(1 * 2) + 3', '1 * 2 + 3'], + ['(1 * 2 + 3)', '1 * 2 + 3'], + ['(True and False) or True', 'True and False or True'], + // Associativity. + ['1 - (2 - 3)', '1 - (2 - 3)'], + ['(1 - 2) - 3', '1 - 2 - 3'], + ['1 - 2 - 3', '1 - 2 - 3'], + ['1 / (2 * 3)', '1 / (2 * 3)'], + ['1 - (2 + 3)', '1 - (2 + 3)'], + // Unary operators. + ['not (True and False)', 'not (True and False)'], + [ + '@variables.a and not (@variables.b or @variables.c)', + 'state.a and not (state.b or state.c)', + ], + ['-(1 + 2)', '-(1 + 2)'], + ['-(1 + 2) * 3', '-(1 + 2) * 3'], + ['not (@variables.a == 1)', 'not state.a == 1'], + ['not (not True)', 'not not True'], + ['-(-1)', '--1'], + ['not (1 if True else 2)', 'not (1 if True else 2)'], + ['-(1 if True else 2)', '-(1 if True else 2)'], + ['not True', 'not True'], + ['-1', '-1'], + // Comparisons chain in Python, so nested comparisons stay grouped. + ['(True or False) == False', '(True or False) == False'], + ['True == (False or True)', 'True == (False or True)'], + ['(1 + 2) < (3 + 4)', '1 + 2 < 3 + 4'], + ['(@variables.a) == (@variables.a)', 'state.a == state.a'], + ['@variables.a == @variables.a', 'state.a == state.a'], + ['((1)) == ((2))', '1 == 2'], + ['(1 == 2) == False', '(1 == 2) == False'], + ['1 == (2 == 3)', '1 == (2 == 3)'], + ['(1 < 2) < 3', '(1 < 2) < 3'], + ['(1 + 2) == (3)', '1 + 2 == 3'], + ['(1 if True else 2) == 1', '(1 if True else 2) == 1'], + ['1 == 1', '1 == 1'], + ['1==1', '1 == 1'], + // Ternaries. + [ + '(1 if True else 2) if False else 3', + '(1 if True else 2) if False else 3', + ], + ['1 if (2 if True else 3) else 4', '1 if (2 if True else 3) else 4'], + [ + '1 if True else (2 if False else 3)', + '1 if True else 2 if False else 3', + ], + ['1 if (True or False) else 2', '1 if True or False else 2'], + ['(1 + 2) if True else (3 + 4)', '1 + 2 if True else 3 + 4'], + ['1 if True else 2 if False else 3', '1 if True else 2 if False else 3'], + ['1 + (2 if True else 3)', '1 + (2 if True else 3)'], + // Access targets. + ['(1 + 2)[0]', '(1 + 2)[0]'], + ['(@variables.a + @variables.b)[0]', '(state.a + state.b)[0]'], + ['(@variables.a or @variables.b)[0]', '(state.a or state.b)[0]'], + ['(@variables.a or @variables.b).x', '(state.a or state.b).x'], + ['(a + b)()', '(a + b)()'], + ['(1 if True else 2).x', '(1 if True else 2).x'], + ['(@variables.a).x', 'state.a.x'], + ['@variables.a[0]', 'state.a[0]'], + ['(@variables.a + @variables.b).length', 'len(state.a + state.b)'], + ['f((1 + 2) * 3)', 'f((1 + 2) * 3)'], + ])('compiles %s as %s', (source, expected) => { + expect(compileSource(source)).toBe(expected); + }); - expect(result.diagnostics).toEqual([]); - expect(result.value.value).toBeInstanceOf(BinaryExpression); - expect(compileExpression(result.value.value, ctx)).toBe(expected); + it.each([ + [ + 'a nested binary on the left', + new BinaryExpression( + new BinaryExpression(new NumberLiteral(1), '+', new NumberLiteral(2)), + '*', + new NumberLiteral(3) + ), + '(1 + 2) * 3', + ], + [ + 'a nested binary on the right of a left-associative operator', + new BinaryExpression( + new NumberLiteral(1), + '-', + new BinaryExpression(new NumberLiteral(2), '-', new NumberLiteral(3)) + ), + '1 - (2 - 3)', + ], + [ + 'a ternary used as a condition', + new TernaryExpression( + new NumberLiteral(1), + new TernaryExpression( + new NumberLiteral(2), + new BooleanLiteral(true), + new NumberLiteral(3) + ), + new NumberLiteral(4) + ), + '1 if (2 if True else 3) else 4', + ], + [ + 'not applied to or', + new UnaryExpression( + 'not', + new BinaryExpression( + new BooleanLiteral(true), + 'or', + new BooleanLiteral(false) + ) + ), + 'not (True or False)', + ], + ])('groups %s without any CST', (_name, expr, expected) => { + expect(compileExpression(expr, ctx)).toBe(expected); }); + }); + describe('binary expressions', () => { it('should compile binary addition', () => { const expr = new BinaryExpression( new NumberLiteral(1), @@ -412,30 +536,6 @@ describe('compileExpression', () => { }); describe('unary expressions', () => { - it.each([ - ['not (True and False)', 'not (True and False)'], - [ - '@variables.a and not (@variables.b or @variables.c)', - 'state.a and not (state.b or state.c)', - ], - ['-(1 + 2)', '-(1 + 2)'], - ['not True', 'not True'], - ['-1', '-1'], - ])('preserves operand grouping in %s', (source, expected) => { - ctx.mutableVariableNames.add('a'); - ctx.mutableVariableNames.add('b'); - ctx.mutableVariableNames.add('c'); - const { rootNode } = parse(`value: ${source}\n`); - const mapping = - rootNode.namedChildren.find(node => node.type === 'mapping') ?? - rootNode; - const result = new Dialect().parse(mapping, { value: ExpressionValue }); - - expect(result.diagnostics).toEqual([]); - expect(compileExpression(result.value.value, ctx)).toBe(expected); - expect(ctx.diagnostics).toEqual([]); - }); - it('should compile not operator', () => { const expr = new UnaryExpression('not', new BooleanLiteral(true)); expect(compileExpression(expr, ctx)).toBe('not True'); @@ -448,27 +548,6 @@ describe('compileExpression', () => { }); describe('comparison expressions', () => { - it.each([ - ['(True or False) == False', '(True or False) == False'], - ['True == (False or True)', 'True == (False or True)'], - ['(1 + 2) < (3 + 4)', '(1 + 2) < (3 + 4)'], - ['((1 + 2)) == ((3 + 4))', '(1 + 2) == (3 + 4)'], - ['(1 + 2)==(1 + 2)', '(1 + 2)==(1 + 2)'], - ['1 == 1', '1 == 1'], - ['1==1', '1==1'], - ])('preserves operand grouping and spacing in %s', (source, expected) => { - const { rootNode } = parse(`value: ${source}\n`); - const mapping = - rootNode.namedChildren.find(node => node.type === 'mapping') ?? - rootNode; - const result = new Dialect().parse(mapping, { value: ExpressionValue }); - - expect(result.diagnostics).toEqual([]); - expect(result.value.value).toBeInstanceOf(ComparisonExpression); - expect(compileExpression(result.value.value, ctx)).toBe(expected); - expect(ctx.diagnostics).toEqual([]); - }); - it('should compile == comparison', () => { ctx.mutableVariableNames.add('x'); const expr = new ComparisonExpression( diff --git a/packages/compiler/test/operator-grouping.test.ts b/packages/compiler/test/operator-grouping.test.ts new file mode 100644 index 00000000..c2cef198 --- /dev/null +++ b/packages/compiler/test/operator-grouping.test.ts @@ -0,0 +1,109 @@ +/* + * Copyright (c) 2026, Salesforce, Inc. + * All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * For full license text, see the LICENSE file in the repo root or https://www.apache.org/licenses/LICENSE-2.0 + */ + +/** + * End-to-end checks that operator grouping survives the full compile: + * parse -> compile -> inspect the emitted expression strings. + */ +import { describe, it, expect } from 'vitest'; +import { compile } from '../src/compile.js'; +import { parseSource } from './test-utils.js'; +import { STATE_UPDATE_ACTION } from '../src/constants.js'; + +const HEADER = ` +config: + agent_name: "TestBot" + agent_type: "AgentforceServiceAgent" + default_agent_user: "test@example.com" + +variables: + a: mutable boolean = False + b: mutable boolean = False + c: mutable boolean = False + total: mutable number = 0 +`; + +function compileOutput(body: string) { + const { output, diagnostics } = compile(parseSource(HEADER + body)); + expect(diagnostics.filter(d => d.severity === 1)).toEqual([]); + return output; +} + +describe('operator grouping end to end', () => { + it('keeps grouping on arithmetic assigned in a set statement', () => { + const output = compileOutput(` +start_agent main: + description: "Test" + reasoning: + instructions: -> + set @variables.total = (1 + 2) * (3 + 4) + | Do something +`); + const node = output.agent_version.nodes.find( + n => n.developer_name === 'main' + )!; + const updates = (node.before_reasoning_iteration ?? []).flatMap( + a => + (a as { state_updates?: Record[] }).state_updates ?? [] + ); + expect(updates).toContainEqual({ total: '(1 + 2) * (3 + 4)' }); + }); + + it('keeps grouping under not in an if condition', () => { + const output = compileOutput(` +start_agent main: + description: "Test" + reasoning: + instructions: -> + if not (@variables.a and @variables.b): + set @variables.c = True + | Do something +`); + const node = output.agent_version.nodes.find( + n => n.developer_name === 'main' + )!; + const updates = (node.before_reasoning_iteration ?? []).flatMap( + a => + (a as { state_updates?: Record[] }).state_updates ?? [] + ); + expect( + updates.some(u => Object.values(u).includes('not (state.a and state.b)')) + ).toBe(true); + }); + + it('keeps grouping in an available when clause', () => { + const output = compileOutput(` +start_agent main: + description: "Test" + reasoning: + instructions: -> + | Test + actions: + go: @utils.transition to @topic.destination + available when (@variables.a or @variables.b) and @variables.c + +topic destination: + description: "destination" +`); + const node = output.agent_version.nodes.find( + n => n.developer_name === 'main' + )!; + const tool = node.tools.find(t => t.target === STATE_UPDATE_ACTION)!; + expect(tool.enabled).toBe('(state.a or state.b) and state.c'); + }); + + it('keeps grouping inside a template interpolation', () => { + const output = compileOutput(` +start_agent main: + description: "Test" + reasoning: + instructions: -> + | Total {!(@variables.total + 1) * 2} +`); + expect(JSON.stringify(output)).toContain('{{(state.total + 1) * 2}}'); + }); +}); From 0498226aaad0ff7232b16fb40d2a2b60c8138824 Mon Sep 17 00:00:00 2001 From: Allen Li Date: Thu, 15 Oct 2026 12:21:55 -0700 Subject: [PATCH 2/4] fix(compiler): wrap number literals before member access and nested signs A number literal before `.` lexes as a float (`1.real`), so it is now parenthesized. Negative literals built in code print with a leading `-` and now rank as unary, and a signed operand of unary +/- is always wrapped so `-(-1)` no longer prints as `--1`. Regenerate the two golden fixtures whose compiled conditions change: one drops parentheses made redundant by and-over-or precedence, the other drops doubled spaces before `is not`. Reword a compile-collect comment that said the expression emitter never adds parentheses. --- .../src/expressions/compile-expression.ts | 15 +++++++++++---- packages/compiler/src/nodes/compile-collect.ts | 4 ++-- packages/compiler/test/expressions.test.ts | 18 +++++++++++++++++- .../connection_with_unified_email.camel.json | 6 +++--- .../connection_with_unified_email.snake.json | 6 +++--- .../post_action_conditionals.camel.json | 2 +- .../post_action_conditionals.snake.json | 2 +- 7 files changed, 38 insertions(+), 15 deletions(-) diff --git a/packages/compiler/src/expressions/compile-expression.ts b/packages/compiler/src/expressions/compile-expression.ts index afed86ab..ee47aac1 100644 --- a/packages/compiler/src/expressions/compile-expression.ts +++ b/packages/compiler/src/expressions/compile-expression.ts @@ -304,12 +304,16 @@ function compileMemberExpression( } } - const obj = compileExprNode(expr.object, ctx, opts); // Convert .length to len() function call for runtime compatibility if (expr.property === 'length') { - return `len(${obj})`; + return `len(${compileExprNode(expr.object, ctx, opts)})`; } - return `${compileAccessTarget(expr.object, ctx, opts)}.${expr.property}`; + // A number before `.` would lex as a float literal (`1.real`), so wrap it. + const obj = + expr.object instanceof NumberLiteral + ? `(${compileExprNode(expr.object, ctx, opts)})` + : compileAccessTarget(expr.object, ctx, opts); + return `${obj}.${expr.property}`; } function compileAtIdentifier(expr: AtIdentifier, ctx: CompilerContext): string { @@ -499,6 +503,8 @@ function precedenceOf(expr: Expression): number { return expr.operator === 'not' ? PRECEDENCE.not : PRECEDENCE.unary; } if (expr instanceof BinaryExpression) return binaryPrecedence(expr.operator); + // A negative literal (only constructible in code) prints with a leading `-`. + if (expr instanceof NumberLiteral && expr.value < 0) return PRECEDENCE.unary; return PRECEDENCE.primary; } @@ -537,7 +543,8 @@ function compileUnaryExpression( if (expr.operator === 'not') { return `not ${compileOperand(expr.operand, PRECEDENCE.not, ctx, opts)}`; } - const operand = compileOperand(expr.operand, PRECEDENCE.unary, ctx, opts); + // Wrap a nested sign too, so `-(-1)` never prints as `--1`. + const operand = compileOperand(expr.operand, PRECEDENCE.primary, ctx, opts); return `${expr.operator}${operand}`; } diff --git a/packages/compiler/src/nodes/compile-collect.ts b/packages/compiler/src/nodes/compile-collect.ts index bea16736..1c696375 100644 --- a/packages/compiler/src/nodes/compile-collect.ts +++ b/packages/compiler/src/nodes/compile-collect.ts @@ -260,8 +260,8 @@ export function buildPriorGuardByTarget( /** * Right-associate a list of operands under a binary operator, parenthesizing * the tail so that 3+ operand expressions read `A OP (B OP (C OP D))`. The - * runtime evaluator requires explicit parens for 3+ operand expressions (the - * compiler's expression emitter never auto-parenthesizes), and this matches the + * runtime evaluator requires explicit parens for 3+ operand expressions (these + * strings are built here, not by the expression emitter), and this matches the * only existing 3-operand `enabled` condition in the golden corpus * (`pronto_customer_support`: `A and (B and C)`). For 1 operand the operand is * returned as-is; for 2 it is `A OP B` (no parens needed). diff --git a/packages/compiler/test/expressions.test.ts b/packages/compiler/test/expressions.test.ts index ba7ce716..9ef9156a 100644 --- a/packages/compiler/test/expressions.test.ts +++ b/packages/compiler/test/expressions.test.ts @@ -418,7 +418,9 @@ describe('compileExpression', () => { ['-(1 + 2) * 3', '-(1 + 2) * 3'], ['not (@variables.a == 1)', 'not state.a == 1'], ['not (not True)', 'not not True'], - ['-(-1)', '--1'], + ['-(-1)', '-(-1)'], + ['-(-@variables.a)', '-(-state.a)'], + ['(-@variables.a).x', '(-state.a).x'], ['not (1 if True else 2)', 'not (1 if True else 2)'], ['-(1 if True else 2)', '-(1 if True else 2)'], ['not True', 'not True'], @@ -437,6 +439,9 @@ describe('compileExpression', () => { ['(1 if True else 2) == 1', '(1 if True else 2) == 1'], ['1 == 1', '1 == 1'], ['1==1', '1 == 1'], + ['@variables.a is not None', 'state.a is not None'], + // An unparenthesized chain parses left-nested, so it is not chained. + ['3 > 2 > 1', '(3 > 2) > 1'], // Ternaries. [ '(1 if True else 2) if False else 3', @@ -459,6 +464,7 @@ describe('compileExpression', () => { ['(a + b)()', '(a + b)()'], ['(1 if True else 2).x', '(1 if True else 2).x'], ['(@variables.a).x', 'state.a.x'], + ['(1).real', '(1).real'], ['@variables.a[0]', 'state.a[0]'], ['(@variables.a + @variables.b).length', 'len(state.a + state.b)'], ['f((1 + 2) * 3)', 'f((1 + 2) * 3)'], @@ -510,6 +516,16 @@ describe('compileExpression', () => { ), 'not (True or False)', ], + [ + 'a negative literal as a member target', + new MemberExpression(new NumberLiteral(-1), 'real'), + '(-1).real', + ], + [ + 'a negative literal under unary minus', + new UnaryExpression('-', new NumberLiteral(-1)), + '-(-1)', + ], ])('groups %s without any CST', (_name, expr, expected) => { expect(compileExpression(expr, ctx)).toBe(expected); }); diff --git a/packages/compiler/test/fixtures/expected/connection_with_unified_email.camel.json b/packages/compiler/test/fixtures/expected/connection_with_unified_email.camel.json index 85d3569b..c71dd2d8 100644 --- a/packages/compiler/test/fixtures/expected/connection_with_unified_email.camel.json +++ b/packages/compiler/test/fixtures/expected/connection_with_unified_email.camel.json @@ -1255,7 +1255,7 @@ "enabled": "True", "stateUpdates": [ { - "AgentScriptInternal_condition_1": "state.QualificationEnabled == True and state.HasSalesInterest is not None or (state.QualificationEnabled == False and state.MeetingSchedulingEnabled == True and state.WantsMeeting is not None) or (state.QualificationEnabled == False and state.MeetingSchedulingEnabled == False)" + "AgentScriptInternal_condition_1": "state.QualificationEnabled == True and state.HasSalesInterest is not None or state.QualificationEnabled == False and state.MeetingSchedulingEnabled == True and state.WantsMeeting is not None or state.QualificationEnabled == False and state.MeetingSchedulingEnabled == False" } ] }, @@ -2656,7 +2656,7 @@ "enabled": "True", "stateUpdates": [ { - "AgentScriptInternal_condition_1": "state.ProspectId is not None and state.RecordCreationIntent == \"qualify\" and (state.VerificationEnabled == True or (state.VerificationEnabled == False and state.IsNewRecord != \"false\"))" + "AgentScriptInternal_condition_1": "state.ProspectId is not None and state.RecordCreationIntent == \"qualify\" and (state.VerificationEnabled == True or state.VerificationEnabled == False and state.IsNewRecord != \"false\")" } ] }, @@ -2814,7 +2814,7 @@ "enabled": "state.AgentScriptInternal_next_topic==\"__EMPTY__\"", "stateUpdates": [ { - "AgentScriptInternal_condition_1": "state.ProspectId is not None and state.RecordCreationIntent == \"qualify\" and (state.VerificationEnabled == True or (state.VerificationEnabled == False and state.IsNewRecord != \"false\"))" + "AgentScriptInternal_condition_1": "state.ProspectId is not None and state.RecordCreationIntent == \"qualify\" and (state.VerificationEnabled == True or state.VerificationEnabled == False and state.IsNewRecord != \"false\")" } ] }, diff --git a/packages/compiler/test/fixtures/expected/connection_with_unified_email.snake.json b/packages/compiler/test/fixtures/expected/connection_with_unified_email.snake.json index 7078edfd..8ceff801 100644 --- a/packages/compiler/test/fixtures/expected/connection_with_unified_email.snake.json +++ b/packages/compiler/test/fixtures/expected/connection_with_unified_email.snake.json @@ -1255,7 +1255,7 @@ "enabled": "True", "state_updates": [ { - "AgentScriptInternal_condition_1": "state.QualificationEnabled == True and state.HasSalesInterest is not None or (state.QualificationEnabled == False and state.MeetingSchedulingEnabled == True and state.WantsMeeting is not None) or (state.QualificationEnabled == False and state.MeetingSchedulingEnabled == False)" + "AgentScriptInternal_condition_1": "state.QualificationEnabled == True and state.HasSalesInterest is not None or state.QualificationEnabled == False and state.MeetingSchedulingEnabled == True and state.WantsMeeting is not None or state.QualificationEnabled == False and state.MeetingSchedulingEnabled == False" } ] }, @@ -2656,7 +2656,7 @@ "enabled": "True", "state_updates": [ { - "AgentScriptInternal_condition_1": "state.ProspectId is not None and state.RecordCreationIntent == \"qualify\" and (state.VerificationEnabled == True or (state.VerificationEnabled == False and state.IsNewRecord != \"false\"))" + "AgentScriptInternal_condition_1": "state.ProspectId is not None and state.RecordCreationIntent == \"qualify\" and (state.VerificationEnabled == True or state.VerificationEnabled == False and state.IsNewRecord != \"false\")" } ] }, @@ -2814,7 +2814,7 @@ "enabled": "state.AgentScriptInternal_next_topic==\"__EMPTY__\"", "state_updates": [ { - "AgentScriptInternal_condition_1": "state.ProspectId is not None and state.RecordCreationIntent == \"qualify\" and (state.VerificationEnabled == True or (state.VerificationEnabled == False and state.IsNewRecord != \"false\"))" + "AgentScriptInternal_condition_1": "state.ProspectId is not None and state.RecordCreationIntent == \"qualify\" and (state.VerificationEnabled == True or state.VerificationEnabled == False and state.IsNewRecord != \"false\")" } ] }, diff --git a/packages/compiler/test/fixtures/expected/post_action_conditionals.camel.json b/packages/compiler/test/fixtures/expected/post_action_conditionals.camel.json index 79179ed9..937dcd9a 100644 --- a/packages/compiler/test/fixtures/expected/post_action_conditionals.camel.json +++ b/packages/compiler/test/fixtures/expected/post_action_conditionals.camel.json @@ -123,7 +123,7 @@ } ], "name": "retrieve_order_details", - "enabled": "state.email is not None and state.order_number is not None" + "enabled": "state.email is not None and state.order_number is not None" }, { "type": "action", diff --git a/packages/compiler/test/fixtures/expected/post_action_conditionals.snake.json b/packages/compiler/test/fixtures/expected/post_action_conditionals.snake.json index 95a4f3a3..635397d3 100644 --- a/packages/compiler/test/fixtures/expected/post_action_conditionals.snake.json +++ b/packages/compiler/test/fixtures/expected/post_action_conditionals.snake.json @@ -123,7 +123,7 @@ } ], "name": "retrieve_order_details", - "enabled": "state.email is not None and state.order_number is not None" + "enabled": "state.email is not None and state.order_number is not None" }, { "type": "action", From aa5ee1a77e238940e69b0edf813e155a8998173d Mon Sep 17 00:00:00 2001 From: Allen Li Date: Mon, 26 Oct 2026 15:14:18 -0400 Subject: [PATCH 3/4] fix(compiler): keep unparenthesized comparison chains chained The AST nests a source chain like `0 < a < 10` the same way as an explicit `(0 < a) < 10`, so always parenthesizing a comparison operand turned Python chains into nested comparisons and changed their meaning. Use the CST to tell them apart: an operand that is part of an unparenthesized chain in the source is emitted as a chain, and any other comparison operand of a comparison (explicit grouping, or a tree built in code) is still parenthesized. Adds source-to-Python regression tests for a numeric range guard and a string chain. --- .../src/expressions/compile-expression.ts | 49 +++++++++-- packages/compiler/test/expressions.test.ts | 29 ++++++- .../compiler/test/operator-grouping.test.ts | 86 ++++++++++++++++++- 3 files changed, 153 insertions(+), 11 deletions(-) diff --git a/packages/compiler/src/expressions/compile-expression.ts b/packages/compiler/src/expressions/compile-expression.ts index ee47aac1..5a609cea 100644 --- a/packages/compiler/src/expressions/compile-expression.ts +++ b/packages/compiler/src/expressions/compile-expression.ts @@ -32,7 +32,7 @@ import { decomposeAtMemberExpression, escapeStringValue, } from '@agentscript/language'; -import type { Range } from '@agentscript/types'; +import type { Range, SyntaxNode } from '@agentscript/types'; import type { CompilerContext } from '../compiler-context.js'; import { extractNumberValue } from '../ast-helpers.js'; @@ -553,12 +553,47 @@ function compileComparisonExpression( ctx: CompilerContext, opts: CompileExpressionOptions ): string { - // Python chains comparisons (`a < b < c` is `a < b and b < c`), so a - // comparison operand of a comparison is always parenthesized. - const minPrecedence = PRECEDENCE.comparison + 1; - const left = compileOperand(expr.left, minPrecedence, ctx, opts); - const right = compileOperand(expr.right, minPrecedence, ctx, opts); - return `${left} ${expr.operator} ${right}`; + // Python chains comparisons (`a < b < c` is `a < b and b < c`). A chain + // written in the source stays a chain; any other comparison operand of a + // comparison, such as `(a < b) < c` or one built in code, is parenthesized. + const compileSide = (side: Expression) => + isSourceChainLink(side, expr) + ? compileExprNode(side, ctx, opts) + : compileOperand(side, PRECEDENCE.comparison + 1, ctx, opts); + return `${compileSide(expr.left)} ${expr.operator} ${compileSide(expr.right)}`; +} + +/** + * Whether `operand` is a link of a comparison chain written without + * parentheses, like `a < b` in `a < b < c`. The AST nests a chain the same + * way as `(a < b) < c`, so only the source CST can tell them apart. + */ +function isSourceChainLink( + operand: Expression, + comparison: ComparisonExpression +): boolean { + const outer = comparison.__cst?.node; + let node = operand.__cst?.node; + if (!(operand instanceof ComparisonExpression) || !outer || !node) { + return false; + } + // Climb past `expression` wrappers. A `parenthesized_expression` stops the + // climb, so an explicitly grouped operand never matches. + do { + node = node.parent ?? undefined; + } while (node?.type === 'expression'); + return node !== undefined && isSameNode(node, outer); +} + +/** Compare by position, since a parser backend may wrap a node more than once. */ +function isSameNode(a: SyntaxNode, b: SyntaxNode): boolean { + return ( + a.type === b.type && + a.startRow === b.startRow && + a.startCol === b.startCol && + a.endRow === b.endRow && + a.endCol === b.endCol + ); } function compileTernaryExpression( diff --git a/packages/compiler/test/expressions.test.ts b/packages/compiler/test/expressions.test.ts index 9ef9156a..6a348df3 100644 --- a/packages/compiler/test/expressions.test.ts +++ b/packages/compiler/test/expressions.test.ts @@ -425,7 +425,7 @@ describe('compileExpression', () => { ['-(1 if True else 2)', '-(1 if True else 2)'], ['not True', 'not True'], ['-1', '-1'], - // Comparisons chain in Python, so nested comparisons stay grouped. + // Comparisons chain in Python, so grouped comparisons stay grouped. ['(True or False) == False', '(True or False) == False'], ['True == (False or True)', 'True == (False or True)'], ['(1 + 2) < (3 + 4)', '1 + 2 < 3 + 4'], @@ -440,8 +440,18 @@ describe('compileExpression', () => { ['1 == 1', '1 == 1'], ['1==1', '1 == 1'], ['@variables.a is not None', 'state.a is not None'], - // An unparenthesized chain parses left-nested, so it is not chained. - ['3 > 2 > 1', '(3 > 2) > 1'], + // An unparenthesized chain stays a Python chain. + ['3 > 2 > 1', '3 > 2 > 1'], + ['0 < @variables.a < 10', '0 < state.a < 10'], + ['"a" < "b" < "c"', '"a" < "b" < "c"'], + ['1 < 2 <= 3 == 3', '1 < 2 <= 3 == 3'], + ['0 < @variables.a + 1 < 10', '0 < state.a + 1 < 10'], + ['(0 < @variables.a < 10)', '0 < state.a < 10'], + ['not 0 < @variables.a < 10', 'not 0 < state.a < 10'], + ['((1 < 2) < 3) < 4', '((1 < 2) < 3) < 4'], + ['(1 < 2 < 3) < 4', '(1 < 2 < 3) < 4'], + ['(1 < 2) < 3 < 4', '(1 < 2) < 3 < 4'], + ['1 < (2 < 3 < 4)', '1 < (2 < 3 < 4)'], // Ternaries. [ '(1 if True else 2) if False else 3', @@ -516,6 +526,19 @@ describe('compileExpression', () => { ), 'not (True or False)', ], + [ + 'a comparison on the left of a comparison', + new ComparisonExpression( + new ComparisonExpression( + new NumberLiteral(3), + '>', + new NumberLiteral(2) + ), + '>', + new NumberLiteral(1) + ), + '(3 > 2) > 1', + ], [ 'a negative literal as a member target', new MemberExpression(new NumberLiteral(-1), 'real'), diff --git a/packages/compiler/test/operator-grouping.test.ts b/packages/compiler/test/operator-grouping.test.ts index c2cef198..58be8873 100644 --- a/packages/compiler/test/operator-grouping.test.ts +++ b/packages/compiler/test/operator-grouping.test.ts @@ -9,10 +9,14 @@ * End-to-end checks that operator grouping survives the full compile: * parse -> compile -> inspect the emitted expression strings. */ +import { execFileSync, spawnSync } from 'node:child_process'; import { describe, it, expect } from 'vitest'; import { compile } from '../src/compile.js'; import { parseSource } from './test-utils.js'; -import { STATE_UPDATE_ACTION } from '../src/constants.js'; +import { + RUNTIME_CONDITION_VARIABLE, + STATE_UPDATE_ACTION, +} from '../src/constants.js'; const HEADER = ` config: @@ -25,6 +29,7 @@ variables: b: mutable boolean = False c: mutable boolean = False total: mutable number = 0 + amount: mutable number = 0 `; function compileOutput(body: string) { @@ -107,3 +112,82 @@ start_agent main: expect(JSON.stringify(output)).toContain('{{(state.total + 1) * 2}}'); }); }); + +describe('comparison chains end to end', () => { + /** Compile `if :` and return the emitted condition expression. */ + function compileCondition(condition: string): string { + const output = compileOutput(` +start_agent main: + description: "Test" + reasoning: + instructions: -> + if ${condition}: + set @variables.c = True + | Test +`); + const node = output.agent_version.nodes.find( + n => n.developer_name === 'main' + )!; + const updates = (node.before_reasoning_iteration ?? []).flatMap( + a => + (a as { state_updates?: Record[] }).state_updates ?? [] + ); + const slot = `${RUNTIME_CONDITION_VARIABLE}_1`; + const update = updates.find(u => slot in u); + expect(update).toBeDefined(); + return update![slot]; + } + + /** Evaluate a compiled expression with Python, the language it runs as. */ + function evaluate(expression: string, state: Record) { + const script = [ + 'import json, sys, types', + 'state = types.SimpleNamespace(**json.loads(sys.argv[2]))', + 'print(json.dumps(eval(sys.argv[1], {"state": state})))', + ].join('\n'); + const stdout = execFileSync( + 'python3', + ['-c', script, expression, JSON.stringify(state)], + { encoding: 'utf8' } + ); + return JSON.parse(stdout) as unknown; + } + + const hasPython = spawnSync('python3', ['--version']).status === 0; + + it('keeps a numeric range guard chained', () => { + expect(compileCondition('0 < @variables.amount < 10')).toBe( + '0 < state.amount < 10' + ); + }); + + it('keeps a string chain chained', () => { + expect(compileCondition('"a" < "b" < "c"')).toBe('"a" < "b" < "c"'); + }); + + it('keeps an explicitly grouped comparison grouped', () => { + expect(compileCondition('(3 > 2) > 1')).toBe('(3 > 2) > 1'); + }); + + describe.skipIf(!hasPython)('evaluated as Python', () => { + it.each([ + [100, false], + [-5, false], + [0, false], + [10, false], + [5, true], + ])('a range guard with amount %s is %s', (amount, expected) => { + const condition = compileCondition('0 < @variables.amount < 10'); + expect(evaluate(condition, { amount })).toBe(expected); + }); + + it('a string chain is True', () => { + expect(evaluate(compileCondition('"a" < "b" < "c"'), {})).toBe(true); + }); + + it('a grouped comparison compares the inner result', () => { + expect(evaluate(compileCondition('3 > 2 > 1'), {})).toBe(true); + expect(evaluate(compileCondition('(3 > 2) > 1'), {})).toBe(false); + }); + }); +}); From cecb4c93bbbaf21a0d90123c19fdebb84e7d3d5c Mon Sep 17 00:00:00 2001 From: Allen Li Date: Mon, 26 Oct 2026 15:25:07 -0400 Subject: [PATCH 4/4] docs(compiler): clarify why comparison chain links compare CST nodes by position --- packages/compiler/src/expressions/compile-expression.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/compiler/src/expressions/compile-expression.ts b/packages/compiler/src/expressions/compile-expression.ts index 5a609cea..a2f9008a 100644 --- a/packages/compiler/src/expressions/compile-expression.ts +++ b/packages/compiler/src/expressions/compile-expression.ts @@ -585,7 +585,7 @@ function isSourceChainLink( return node !== undefined && isSameNode(node, outer); } -/** Compare by position, since a parser backend may wrap a node more than once. */ +/** Compare by position; `SyntaxNode` does not promise stable object identity. */ function isSameNode(a: SyntaxNode, b: SyntaxNode): boolean { return ( a.type === b.type &&