From 76379a94075507d3c4f74222f634a68c26bd938a Mon Sep 17 00:00:00 2001 From: vinodhalaharvi-claude Date: Mon, 25 May 2026 14:48:05 +0000 Subject: [PATCH] =?UTF-8?q?parse:=20fix=20bare-body=20parallel=20=E2=80=94?= =?UTF-8?q?=20block's=20own=20parens=20wrap=20the=20parallel?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #24, which shipped parallel <*> support but with a bug: it required a REDUNDANT inner paren — accepting only ( ( a <*> b ) ), not the bare ( a <*> b ) that the original grammar and the LLM actually use. The bare form failed live in Slack with: script.Parse: 1:40: unexpected token "<*>" (expected ")") Root cause: the block body was a *parsedPipeline (sequential-only), and <*> was a property of an inner group only. So the block's own ( ... ) could not itself be a parallel. The original internal/agentscript grammar lets the block's own parens wrap a parallel (Statement = ( "(" Parallel ")" | Command )); this now matches. Grammar (pkg/script/parse.go): expr = pipeline ( <*> pipeline )* // 1 ⇒ sequential, 2+ ⇒ parallel pipeline = stage ( >=> stage )* stage = call | "(" expr ")" // group = nesting/precedence block = backend mode "(" expr ")" // body IS an expr → may be parallel The block body is now a parsedExpr; a parsedGroup wraps a parsedExpr. toAST: exprToAST emits ast.Parallel for multi-branch exprs, wrapped in a one-stage Pipeline so Block.Body stays an ast.Pipeline (the invariant Lower + the memory bridge rely on); groupToAST unwraps a parallel stage inside a group. The always-Pipeline body contract is preserved. Verified end to end through the new pipeline: memory static ( ssl_check "google.com" <*> dns_lookup "github.com" ) → parse → resolve → RunMemory → BOTH branches execute. (Exact Slack shape.) Plus 3-way, nested-then-pipe, and ( search >=> analyze <*> search >=> analyze ) >=> merge >=> ask "..." Tests: parse_parallel_test.go covers bare-body parallel (→ Parallel stage), 3-way, nested, sequential-is-not-parallel, complex-resolves. Replaced obsolete reject-parallel assertion. All prior parse tests pass. CI: vet, gofmt, staticcheck, go test -race ./pkg/..., build pass. --- pkg/script/parse.go | 103 ++++++++++++++++++++---------- pkg/script/parse_parallel_test.go | 42 +++++------- pkg/script/parse_test.go | 10 +-- 3 files changed, 89 insertions(+), 66 deletions(-) diff --git a/pkg/script/parse.go b/pkg/script/parse.go index ec8485c..7181d45 100644 --- a/pkg/script/parse.go +++ b/pkg/script/parse.go @@ -84,13 +84,23 @@ type parsedScript struct { Blocks []*parsedBlock `@@+` } -// parsedBlock matches: ( ) +// parsedBlock matches: ( ) type parsedBlock struct { - Backend string `@("memory" | "temporal")` - Mode string `@("static" | "dynamic")` - OpenP struct{} `"("` - Body *parsedPipeline `@@` - CloseP struct{} `")"` + Backend string `@("memory" | "temporal")` + Mode string `@("static" | "dynamic")` + OpenP struct{} `"("` + Body *parsedExpr `@@` + CloseP struct{} `")"` +} + +// parsedExpr matches: ( "<*>" )* +// One pipeline ⇒ sequential; two or more ⇒ a parallel fan-out. Used by +// both the block body and any parenthesized group, so the block's OWN +// parens can wrap a parallel — memory static ( a <*> b ) — matching the +// original internal/agentscript grammar. +type parsedExpr struct { + First *parsedPipeline `@@` + Branches []*parsedPipeline `( "<*>" @@ )*` } // parsedPipeline matches: ( ">=>" )* @@ -100,19 +110,17 @@ type parsedPipeline struct { } // parsedStage is one element of a pipeline: either a call or a -// parenthesized group (which may be a parallel fan-out). +// parenthesized group (which is itself an expr, so it may be parallel). type parsedStage struct { Group *parsedGroup ` @@` Call *parsedCall `| @@` } -// parsedGroup matches: "(" ( "<*>" )* ")" -// One branch ⇒ grouping; two or more ⇒ parallel fan-out. +// parsedGroup matches: "(" ")" type parsedGroup struct { - Open struct{} `"("` - First *parsedPipeline `@@` - Branches []*parsedPipeline `( "<*>" @@ )*` - Close struct{} `")"` + Open struct{} `"("` + Expr *parsedExpr `@@` + Close struct{} `")"` } // parsedCall matches: * @@ -177,7 +185,7 @@ func blockToAST(pb *parsedBlock) (ast.Block, error) { if err != nil { return ast.Block{}, err } - body, err := pipelineToAST(pb.Body) + body, err := exprToAST(pb.Body) if err != nil { return ast.Block{}, err } @@ -210,9 +218,40 @@ func parseMode(s string) (ast.Mode, error) { // pipelineToAST always produces a Pipeline node, even for a single // call. That uniformity simplifies the resolver and lowering passes. +// exprToAST converts a parsedExpr (a "<*>"-separated list of pipelines). +// One pipeline ⇒ that pipeline; two or more ⇒ an ast.Parallel of the +// pipelines, wrapped in a one-stage Pipeline so a Block.Body is always an +// ast.Pipeline (the invariant Lower and the memory bridge rely on). This +// is what lets the block's own parens wrap a parallel: +// memory static ( a <*> b ). +func exprToAST(pe *parsedExpr) (ast.Node, error) { + if pe == nil || pe.First == nil { + return nil, fmt.Errorf("empty expression") + } + if len(pe.Branches) == 0 { + return pipelineToAST(pe.First) // sequential — already an ast.Pipeline + } + branches := make([]ast.Node, 0, 1+len(pe.Branches)) + first, err := pipelineToAST(pe.First) + if err != nil { + return nil, fmt.Errorf("branch 0: %w", err) + } + branches = append(branches, first) + for i, pp := range pe.Branches { + b, err := pipelineToAST(pp) + if err != nil { + return nil, fmt.Errorf("branch %d: %w", i+1, err) + } + branches = append(branches, b) + } + return ast.Pipeline{Stages: []ast.Node{ast.Parallel{Branches: branches}}}, nil +} + +// pipelineToAST converts a sequential pipeline of stages. Always returns +// an ast.Pipeline (even single-stage) to keep the body shape uniform. func pipelineToAST(pp *parsedPipeline) (ast.Node, error) { if pp == nil || pp.First == nil { - return nil, fmt.Errorf("empty block body") + return nil, fmt.Errorf("empty pipeline") } stages := make([]ast.Node, 0, 1+len(pp.Rest)) first, err := stageToAST(pp.First) @@ -230,8 +269,8 @@ func pipelineToAST(pp *parsedPipeline) (ast.Node, error) { return ast.Pipeline{Stages: stages}, nil } -// stageToAST converts one pipeline stage — either a call or a -// parenthesized group — into an AST node. +// stageToAST converts one pipeline stage — a call or a parenthesized +// group (which is itself an expr, possibly parallel). func stageToAST(ps *parsedStage) (ast.Node, error) { switch { case ps == nil: @@ -245,31 +284,25 @@ func stageToAST(ps *parsedStage) (ast.Node, error) { } } -// groupToAST converts a parenthesized group. One inner pipeline ⇒ just -// that pipeline (grouping/precedence). Two or more "<*>"-separated -// pipelines ⇒ a Parallel fan-out whose branches are those pipelines. +// groupToAST converts a parenthesized group "(" expr ")". Inside a group +// a Parallel is a legitimate stage, so a group wrapping a parallel expr +// yields the ast.Parallel directly (unwrapping the one-stage Pipeline +// that exprToAST adds for the body invariant). A sequential group yields +// its Pipeline node (grouping/precedence). func groupToAST(pg *parsedGroup) (ast.Node, error) { - if pg == nil || pg.First == nil { + if pg == nil || pg.Expr == nil { return nil, fmt.Errorf("empty group") } - first, err := pipelineToAST(pg.First) + node, err := exprToAST(pg.Expr) if err != nil { - return nil, fmt.Errorf("group branch 0: %w", err) - } - if len(pg.Branches) == 0 { - // Pure grouping — unwrap to the inner pipeline node. - return first, nil + return nil, err } - branches := make([]ast.Node, 0, 1+len(pg.Branches)) - branches = append(branches, first) - for i, pb := range pg.Branches { - b, err := pipelineToAST(pb) - if err != nil { - return nil, fmt.Errorf("group branch %d: %w", i+1, err) + if p, ok := node.(ast.Pipeline); ok && len(p.Stages) == 1 { + if _, isPar := p.Stages[0].(ast.Parallel); isPar { + return p.Stages[0], nil } - branches = append(branches, b) } - return ast.Parallel{Branches: branches}, nil + return node, nil } func callToAST(pc *parsedCall) (ast.Node, error) { diff --git a/pkg/script/parse_parallel_test.go b/pkg/script/parse_parallel_test.go index 7398570..a0bf696 100644 --- a/pkg/script/parse_parallel_test.go +++ b/pkg/script/parse_parallel_test.go @@ -8,16 +8,16 @@ import ( "github.com/vinodhalaharvi/agentscript/pkg/script/ast" ) -// The new parser MUST support parenthesized parallel <*>, matching the -// original internal/agentscript grammar. This is a hard regression guard: -// <*> worked in the original runtime and must work in pkg/script too. +// Parallel <*> must work, with the block's OWN parens wrapping the +// parallel (bare-body), matching the original internal/agentscript +// grammar. This is the exact shape the LLM emits. func TestParse_ParallelForms(t *testing.T) { cases := []string{ - `memory static ( ( a "x" <*> b "y" ) )`, - `memory static ( ( a "x" <*> b "y" <*> c "z" ) )`, + `memory static ( a "x" <*> b "y" )`, + `memory static ( a "x" <*> b "y" <*> c "z" )`, `memory static ( ( a "x" <*> b "y" ) >=> merge )`, `memory static ( ( a >=> b <*> c >=> d ) >=> merge >=> e "q" )`, - `temporal static ( ( echo "x" <*> echo "y" ) )`, + `temporal static ( echo "x" <*> echo "y" )`, } for _, src := range cases { if _, err := script.Parse(context.Background(), script.Source(src)); err != nil { @@ -26,18 +26,11 @@ func TestParse_ParallelForms(t *testing.T) { } } -// The parser must produce an ast.Parallel node for a multi-branch group. -// Block bodies are always wrapped in a Pipeline (the uniform invariant), -// so for `( ( a <*> b ) )` the body is a one-stage Pipeline whose stage -// is the Parallel. -func TestParse_ProducesParallelNode(t *testing.T) { - a, err := script.Parse(context.Background(), script.Source(`memory static ( ( a "x" <*> b "y" ) )`)) +func TestParse_BareBodyParallel(t *testing.T) { + a, err := script.Parse(context.Background(), script.Source(`memory static ( a "x" <*> b "y" )`)) if err != nil { t.Fatalf("parse: %v", err) } - if len(a.Blocks) != 1 { - t.Fatalf("blocks = %d", len(a.Blocks)) - } pipe, ok := a.Blocks[0].Body.(ast.Pipeline) if !ok { t.Fatalf("body should be ast.Pipeline, got %T", a.Blocks[0].Body) @@ -54,24 +47,19 @@ func TestParse_ProducesParallelNode(t *testing.T) { } } -// A single-branch group is just grouping — its body must not contain a -// Parallel node. -func TestParse_SingleGroupIsNotParallel(t *testing.T) { - a, err := script.Parse(context.Background(), script.Source(`memory static ( ( a "x" ) )`)) +func TestParse_SequentialIsNotParallel(t *testing.T) { + a, err := script.Parse(context.Background(), script.Source(`memory static ( a "x" >=> b "y" )`)) if err != nil { t.Fatalf("parse: %v", err) } - pipe, ok := a.Blocks[0].Body.(ast.Pipeline) - if !ok { - t.Fatalf("body should be ast.Pipeline, got %T", a.Blocks[0].Body) - } - if _, isPar := pipe.Stages[0].(ast.Parallel); isPar { - t.Error("single-branch group must not be a Parallel") + pipe := a.Blocks[0].Body.(ast.Pipeline) + for _, s := range pipe.Stages { + if _, isPar := s.(ast.Parallel); isPar { + t.Error("sequential pipeline must not contain a Parallel stage") + } } } -// End to end: a parallel program parses AND resolves against the full -// registry (the complex grammar the CLI examples use). func TestParse_ComplexGrammarResolves(t *testing.T) { src := `memory static ( ( search "x" >=> analyze "s" <*> search "y" >=> analyze "s" ) >=> merge >=> ask "who wins?" )` a, err := script.Parse(context.Background(), script.Source(src)) diff --git a/pkg/script/parse_test.go b/pkg/script/parse_test.go index 53781f2..dcb2bed 100644 --- a/pkg/script/parse_test.go +++ b/pkg/script/parse_test.go @@ -263,10 +263,12 @@ func TestParse_RejectsTrailingArrow(t *testing.T) { parseErr(t, `temporal static ( echo "a" >=> )`) } -func TestParse_RejectsParallelOperatorForNow(t *testing.T) { - // <*> is reserved syntax but not in the MVP grammar. Parsing it - // should fail until the grammar is extended. - parseErr(t, `temporal static ( echo "a" <*> echo "b" )`) +func TestParse_AcceptsParallelOperator(t *testing.T) { + // <*> parallel fan-out is now part of the grammar (parity with the + // original internal/agentscript grammar). It must parse. + if _, err := script.Parse(context.Background(), script.Source(`temporal static ( echo "a" <*> echo "b" )`)); err != nil { + t.Errorf("parallel <*> should parse now: %v", err) + } } // === Order: backend must precede mode ======================================