feat: allow expressions in add-to-list statements

[hjotha]

Summary

Allows microflow add-to-list statements to preserve expression-valued add items.

Changes

• Extends the grammar from add $Item to $List to add expression to $List.
• Stores the parsed expression on AddToListStmt while preserving the legacy simple-variable Item field.
• Writes the expression value into ChangeListAction.Value during rebuild.
• Adds proposal docs, a doctype fixture, quick-reference entry, and skill guidance for the new syntax.

Validation

• go test ./mdl/visitor -run TestAddToList
• go test ./mdl/executor -run TestAddToListBuilder
• ./bin/mxcli check mdl-examples/doctype-tests/add_expression_to_list.test.mdl
• make build
• make lint-go
• make test

Closes #361
Part of #352
Part of #332

[ako]

Review — feat: allow expressions in add-to-list statements

Overview

Clean feature extension — grammar change is minimal (ADD expression TO VARIABLE), the visitor dual-sets Item and Value for backward compat, and the builder fallback chain is correct. The proposal and skill docs are good.

---

Moderate: validate_microflow.go and builder_graph.go only track s.Item, not s.Value

Two places consume AddToListStmt and look only at s.Item:

validate_microflow.go:425:

case *ast.AddToListStmt:
    refs = append(refs, s.Item, s.List)

cmd_microflows_builder_graph.go:312:

case *ast.AddToListStmt:
    if s.Item != "" {
        inputs[s.Item] = true
    }

For add head($SourceItems) to $Items, s.Item = "" and s.Value = CallExpr{...}. Neither place sees $SourceItems as a referenced variable — the validator will silently skip it (missing "variable not declared" check) and the flow graph won't include it in the activity's input set (affecting layout/ordering analysis).

Fix: walk the expression when s.Value != nil:

case *ast.AddToListStmt:
    if s.Value != nil {
        refs = append(refs, exprVarRefs(s.Value)...)
    } else {
        refs = append(refs, s.Item)
    }
    refs = append(refs, s.List)

Same pattern in builder_graph.go:

case *ast.AddToListStmt:
    if s.Value != nil {
        walkExpr(s.Value)
    } else if s.Item != "" {
        inputs[s.Item] = true
    }

---

Moderate: .test.mdl suffix — CI won't run the doctype example

mdl-examples/doctype-tests/add_expression_to_list.test.mdl is skipped by TestMxCheck_DoctypeScripts (same issue flagged in PRs #334 and #336). This is now the third PR in this batch with this suffix. Rename to .mdl.

---

Minor: Item field leaves a maintenance trap in the AST

AddToListStmt.Item is now a derived shadow of Value — set only when Value is a VariableExpr, used only as a fallback when Value is nil. Any code path that constructs AddToListStmt manually with only Item set (e.g., tests, codegen) will silently get the old behavior without setting Value, making the dual-state confusing. The AST comment says "kept for simple $Var compatibility" — consider whether the field is still needed at all, or whether the builder fallback "$" + s.Item can be dropped in favour of always requiring Value.

---

Minor: TestAddToListKeepsSimpleVariableCompatibility doesn't assert Value

The test checks addStmt.Item == "Order" but not that addStmt.Value is a *ast.VariableExpr{Name: "Order"}. The dual-state behaviour of the visitor is only half-tested.

---

Positive

• Grammar change is backward-compatible: bare $Item is still a valid expression, so existing scripts parse unchanged.
• Builder fallback chain (exprToString(Value) → "$" + Item) is safe and correctly prioritises the expression.
• Visitor correctly identifies VariableExpr to populate Item for the legacy path.

[github-actions]

AI Code Review

Let me review this pull request step by step against the checklist.

Summary of Changes

This PR adds support for expression-valued add-to-list statements in MDL (Mendix Definition Language), allowing syntax like add head($SourceList) to $TargetList; in addition to the existing add $Item to $List;.

The changes include:

1. Updating the grammar in MDLParser.g4 to allow expression instead of just VARIABLE in add-to-list statements
2. Modifying the AddToListStmt AST node to store both the legacy Item field and a new Value expression field
3. Updating the executor to use the expression value when available, falling back to the legacy item variable
4. Updating validation to track variables used in expressions
5. Adding documentation, tests, and a doctype fixture
6. Updating the quick reference guide

Review Against Checklist

Overlap & duplication

• Check proposals list above for existing proposals covering the same functionality
• Check if the diff introduces code that duplicates existing implementations (look for similar function names, command names, types)
• Check doctype test files above for existing test coverage of the feature area
• Flag if the PR re-documents already-shipped features as new

Looking at the proposals list, I see this PR implements the feature described in the new proposal PROPOSAL_microflow_add_expression_to_list.md. There's no indication of duplication with existing functionality.

The doctype test files listed don't show any existing test for this specific feature, so the new test file add_expression_to_list.mdl is appropriate.

This doesn't appear to be re-documenting already-shipped features as new.

MDL syntax design (for new or modified MDL syntax)

• Follows standard CREATE/ALTER/DROP/SHOW/DESCRIBE pattern — no custom verbs for CRUD operations
• Uses Module.Element qualified names everywhere — no bare names, no implicit module context
• Property lists use ( Key: value, ... ) format with colon separators, one per line
• Colon for property definitions (Key: value), AS for name-to-name mappings ('oldName' AS 'newName') — do not flag AS in mapping/renaming contexts as wrong
• Keywords are full English words — no abbreviations, no symbols for domain operations
• Statement reads as English — a business analyst can understand the intent
• No keyword overloading — each keyword has one consistent meaning
• Diff-friendly — adding one property produces a one-line diff
• Consistent with existing MDL patterns (check docs/01-project/MDL_QUICK_REFERENCE.md)

The PR modifies the add statement syntax from add $Item to $List to add expression to $List.

Let's check against the guidelines:

• It still uses the standard CRUD verb "add" (though "add" isn't strictly CRUD, it's an established MDL keyword for list operations)
• It uses qualified names in the example (head($SourceItems))
• The syntax reads as English: "add expression to list"
• No keyword overloading - "add" still means adding to a list
• The change is diff-friendly (modified one grammar rule)
• Consistent with existing patterns - the quick reference was updated to show both forms

The syntax design skill consulted check: The PR body doesn't explicitly mention consulting .claude/skills/design-mdl-syntax.md, but the changes seem to follow the guidelines.

Full-stack consistency (for MDL features)

New MDL commands or language features must be wired through the full pipeline. If the diff adds any MDL feature, check:

• Grammar rule added to MDLParser.g4 (and MDLLexer.g4 if new tokens)
• AST node type added in mdl/ast/
• Visitor handler in mdl/visitor/
• Executor handler in mdl/executor/
• LSP wiring (cmd/mxcli/lsp.go) if the feature adds formatting, diagnostics, or navigation
• DESCRIBE roundtrip — if the feature creates artifacts, DESCRIBE should output re-executable MDL
• VS Code extension (vscode-mdl/package.json) updated if new LSP capabilities added

Checking the changes:

1. Grammar: ✅ Modified MDLParser.g4 (line 1948) to change addToListStatement from ADD VARIABLE TO VARIABLE to ADD expression TO VARIABLE
2. AST: ✅ Modified ast_microflow.go to add Value Expression field to AddToListStmt while keeping the legacy Item field
3. Visitor: ❌ I don't see explicit changes to the visitor in the diff, but the PR description says "Stores the parsed expression on AddToListStmt". Let me check if this is handled implicitly by the ANTLR visitor generation or if it's missing.
   • Looking at the diff, I see changes to validate_microflow.go which suggests the visitor is correctly setting the Value field
   • There's a test TestCollectObjectInputVariablesSeesAddExpressionValue that checks if variables from expressions are collected
   • This suggests the visitor is working correctly, but I don't see explicit visitor changes in the diff
4. Executor: ✅ Modified cmd_microflows_builder_actions.go to use the expression value
5. LSP: ❌ No changes to cmd/mxcli/lsp.go visible in the diff
6. DESCRIBE roundtrip: The PR adds a doctype test, and the validation test suggests it's working
7. VS Code extension: ❌ No changes to vscode-mdl/package.json visible

Wait, let me double-check the visitor. The PR description says: "Stores the parsed expression on AddToListStmt while preserving the legacy simple-variable Item field."

Looking at the changes:

• In validate_microflow_add_to_list_test.go, there's a test that builds MDL and checks references
• In validate_microflow.go, the referencedVars function was updated to handle the Value field
• In cmd_microflows_builder_graph.go, the collectObjectInputVariables function was updated

This suggests the visitor is correctly setting the Value field, but I don't see explicit visitor changes in the diff. Since ANTLR-generated visitors might not need manual changes if the grammar change is compatible, this might be OK.

Let me check if there are any visitor changes I missed... Scanning the diff again, I don't see explicit changes to mdl/visitor/. This could be a concern if the visitor needs to be updated to handle the new grammar structure.

Actually, looking more carefully at the grammar change:
Before: addToListStatement : ADD VARIABLE TO VARIABLE
After: addToListStatement : ADD expression TO VARIABLE

This is a significant change - it's replacing a specific token (VARIABLE) with a general rule (expression). This would definitely require visitor changes to handle the new structure.

Let me look for visitor changes again... I don't see any in the diff. This is likely an issue.

Test coverage

• New packages should have test files
• New executor commands should have MDL examples in mdl-examples/doctype-tests/
• Integration paths should be tested, not just helpers
• No time.Sleep for synchronization — use channels or polling with timeout

The PR includes:

• New doctype test: mdl-examples/doctype-tests/add_expression_to_list.mdl ✅
• New executor tests: cmd_microflows_builder_add_to_list_test.go ✅
• New validation test: validate_microflow_add_to_list_test.go ✅
• The PR mentions running go test ./mdl/visitor -run TestAddToList and go test ./mdl/executor -run TestAddToListBuilder ✅

Security & robustness

• Command injection, XSS, SQL injection, temp file issues
• Unix sockets should use restrictive permissions (0600)
• File I/O should not be in hot paths (event loops, per-keystroke handlers)
• No silent side effects on typos (e.g., auto-creating resources on misspelled names)
• Correct method receivers (pointer vs value) for mutations

I don't see obvious security issues in the changes. The expression handling goes through existing expression validation pathways.

Scope & atomicity

• Each commit should do one thing — a feature, a bugfix, or a refactor, not a mix
• PR should be scoped to a single feature or concern — flag if description needs "and" between unrelated items
• Independent features should be in separate PRs
• Refactors touching many files should be their own commit

This PR is scoped to a single feature: allowing expressions in add-to-list statements. All changes relate to this feature.

Code quality

• Refactors should be applied consistently (look for old patterns still present)
• Manually maintained lists (keyword lists, type mappings) should be flagged as maintenance risks
• Design docs should match the actual implementation

The changes look consistent. The legacy Item field is preserved for backward compatibility.

Bugs & correctness

• Logic errors, race conditions, resource leaks
• Error handling gaps

I don't see obvious logic errors. The fallback logic in the executor looks correct:

value := fb.exprToString(s.Value)
if value == "" && s.Item != "" {
    value = "$" + s.Item
}

Critical Issues Found

1. Missing visitor updates: The grammar change from ADD VARIABLE TO VARIABLE to ADD expression TO VARIABLE is substantial and would require updates to the ANTLR visitor to properly parse the expression and set the Value field in AddToListStmt. I don't see explicit visitor changes in the diff.

   Without proper visitor updates:

   • The Value field would not be populated correctly
   • Expression parsing might fail or produce incorrect AST
   • The validation and executor tests might be testing incorrect behavior

2. Missing LSP and VS Code extension updates: For a language feature change, we should update the LSP implementation and VS Code extension if it affects syntax highlighting, parsing, or other language features.

Moderate Issues Found

1. Documentation of design skill consultation: The PR doesn't explicitly mention consulting .claude/skills/design-mdl-syntax.md as required by the checklist.

Minor Issues Found

None specifically called out.

What Looks Good

1. Comprehensive testing: The PR includes unit tests for the builder, validation, and a doctype test.
2. Backward compatibility: The legacy Item field is preserved with a fallback mechanism.
3. Proper variable tracking: Validation and input variable collection were updated to handle expressions.
4. Clear documentation: The quick reference, skills documentation, and proposal were updated.
5. Clean implementation: The fallback logic in the executor is clear and correct.

Recommendation

I would request changes due to the critical issue of missing visitor updates. The grammar change requires corresponding changes in the visitor to properly parse expressions and populate the AST.

Before approval, I need to see:

1. Visitor updates to handle the new grammar structure for add-to-list statements
2. Confirmation that the LSP and VS Code extension considerations were evaluated (even if no changes were needed)

Once visitor changes are added, this PR looks strong and ready to merge.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[github-actions]

AI Code Review

What Looks Good

The PR comprehensively implements the requested feature to allow expressions in add-to-list statements while maintaining backward compatibility. All necessary pipeline components are updated:

• Grammar modified to accept expression instead of just VARIABLE
• AST node extended with Value Expression field while preserving legacy Item field
• Executor properly handles both expression and variable forms
• Validation updated to track variables in expressions
• Comprehensive test coverage added (unit tests, doctest)
• Documentation updated across skills, quick reference, proposal, and README
• Implementation follows existing MDL patterns and design guidelines

Recommendation

Approve the PR. The implementation is complete, well-tested, and properly documented. No LSP updates are needed as this extends an existing statement rather than adding new language capabilities requiring LSP changes. The feature maintains backward compatibility and follows the project's architectural principles.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[hjotha]

Review follow-up status:

The current branch already includes the requested AddToListStmt.Value handling:

• cmd_microflows_builder_graph.go walks s.Value with exprVarRefs(...) when present.
• validate_microflow.go also walks s.Value when present before falling back to the legacy s.Item field.

I revalidated the focused executor coverage:

go test ./mdl/executor -run 'AddToList|ExpressionToList|StatementVarRefs' -count=1

[github-actions]

AI Code Review

Review Summary

This PR implements support for expressions in add-to-list statements (e.g., add head($SourceList) to $TargetList), addressing issue #361. The changes are thorough and follow the project's architectural patterns.

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• The LSP wiring and VS Code extension updates aren't visible in the diff, but since this modifies an existing statement type rather than adding a new one, and the syntax is analogous to the existing form, LSP changes may not be required. The existing infrastructure likely handles this extension correctly.

What Looks Good

1. Full-stack implementation: Grammar → AST → Visitor → Executor → Backend abstraction all properly updated
2. Backward compatibility: Legacy Item field preserved alongside new Value field
3. Comprehensive testing: Unit tests, validation tests, and doctest fixture all present
4. Documentation: Proposal, quick reference, and skills documentation updated
5. MDL syntax compliance:
   • Uses standard CRUD-like verb (add)
   • Reads as English for citizen developers
   • Maintains qualified name usage where applicable
   • Properly distinguishes colon (property definitions) from AS (name mapping)
   • No keyword overloading or symbolic syntax
6. Robust implementation:
   • Expression value properly converted to string for BSON storage
   • Fallback to legacy variable when expression empty
   • Validation correctly tracks expression variables
   • Follows existing patterns in the codebase

Recommendation

Approve the PR. The implementation is complete, well-tested, and adheres to all architectural and design guidelines. The feature satisfies the requirement to preserve Studio Pro's expression-valued list actions while maintaining full backward compatibility. No blocking issues were identified.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[github-actions]

AI Code Review

Review Summary

Critical Issues

• None found

Moderate Issues

• Visitor implementation visibility: While validation tests confirm the visitor works (`go test ./

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[hjotha]

CI stabilization note: merged #412 into this branch so it uses the same integration-CI baseline as the rest of the active PR set.

[github-actions]

AI Code Review

What Looks Good

• The implementation correctly preserves backward compatibility by keeping the legacy Item field and using it as fallback when no expression is present
• Expression variables are properly collected for validation and data flow analysis in collectObjectInputVariables and referencedVars
• Comprehensive test coverage including:
  • Unit tests for builder behavior (cmd_microflows_builder_add_to_list_test.go)
  • Validation tests ensuring expression variables are tracked (validate_microflow_add_to_list_test.go)
  • Doctest demonstrating the new syntax (mdl-examples/doctype-tests/add_expression_to_list.test.mdl)
• Documentation updates are thorough:
  • Quick reference updated (MDL_QUICK_REFERENCE.md)
  • Proposal document added (PROPOSAL_microflow_add_expression_to_list.md)
  • Skill guidance updated (write-microflows.md)
  • README updated to reference the new proposal
• The nanoflow examples cleanup improves example quality by removing outdated content
• The watcher debounce improvement enhances robustness (watcher.go and watcher_test.go)

Moderate Issues

• LSP wiring not explicitly verified: While the feature likely doesn't require new LSP capabilities (as it extends existing expression parsing), it would be good to confirm expression highlighting/completion works correctly in the VS Code extension for the new syntax.
• DESCRIBE roundtrip not explicitly demonstrated: The doctest exists, but we don't see explicit verification that mxcli describe produces valid MDL for the new syntax that can be round-tripped.

Recommendation

Approve the PR. The changes are well-implemented, follow the project's patterns, have adequate test coverage, and address the stated need. The implementation correctly wires through the full pipeline (grammar → AST → visitor → executor) and maintains backward compatibility. The minor issues don't block approval, and the benefits outweigh the concerns. The PR is scoped to a single feature (expressions in add-to-list statements) with beneficial example cleanup as a related improvement.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[github-actions]

AI Code Review

Let me review this pull request against the checklist provided in CLAUDE.md.

The PR is titled "feat: allow expressions in add-to-list statements" and aims to allow microflow add-to-list statements to preserve expression-valued add items.

First, I'll check the key areas of the checklist:

Overlap & duplication

• The PR adds a new feature for add-to-list statements to accept expressions
• Looking at the doctype test files, I don't see an existing test for this specific functionality
• The PR adds a new doctype test: mdl-examples/doctype-tests/add_expression_to_list.mdl
• This doesn't appear to duplicate existing functionality

MDL syntax design

The PR modifies the MDL syntax for add-to-list statements:

• Changes from add $Item to $List to add expression to $List
• This follows the standard pattern for MDL statements
• Uses qualified names (though in this case it's variables, not module elements)
• Property lists aren't really applicable here (it's a simple statement)
• Uses English: "add expression to $list" reads as English
• No keyword overloading - "add" still means add to list
• The diff shows the quick reference updated appropriately

Let me check the specific syntax design guidelines from CLAUDE.md:

• Design skill consulted — read .claude/skills/design-mdl-syntax.md before designing syntax
  • The PR doesn't explicitly mention this, but the syntax seems to follow existing patterns
• Follows standard patterns — uses create/alter/drop/show/describe, not custom verbs
  • This is modifying an existing statement, not adding a new verb
• Reads as English — a business analyst understands the statement on first reading
  • "add expression to $list" does read as English
• Qualified names — uses Module.Element everywhere, no implicit module context
  • This is about variables in microflows, not module elements, so qualified names aren't applicable here
• Property format — uses ( key: value, ... ) with colon separators, one per line
  • Not applicable to this statement type
• LLM-friendly — one example is sufficient for an LLM to generate correct variants
  • The PR adds examples in the skill file and quick reference
• Diff-friendly — adding one property is a one-line diff
  • The change modifies the grammar rule and AST structure
• Consistent with existing MDL patterns (check docs/01-project/MDL_QUICK_REFERENCE.md)
  • The quick reference is updated to show the new syntax

Full-stack consistency

Let me check if the feature is wired through the full pipeline:

• Grammar — rule added to MDLParser.g4 (and MDLLexer.g4 if new tokens)
  • The diff shows MDLParser.g4 changed from ADD VARIABLE TO VARIABLE to ADD expression TO VARIABLE
• Parser regenerated — make grammar run, generated files committed
  • The diff shows changes to generated parser files (though the instructions say to note but skip these)
• AST — node type added in mdl/ast/
  • Actually, the AST node was modified, not added: AddToListStmt now has Value Expression field in addition to Item string
• Visitor — ANTLR listener bridges parse tree to AST in mdl/visitor/
  • The diff doesn't show visitor changes, but this would be needed. Let me check if it's implied...
  • Actually, looking at the changes, I don't see explicit visitor changes. The AST was modified to store the expression, so the visitor would need to populate this field. This might be missing.
• Executor — thin handler in mdl/executor/ dispatches to ctx.Backend.*; no BSON in the handler
  • The diff shows changes to cmd_microflows_builder_actions.go which handles the ADD TO LIST action
• Backend method — data access or mutation wired through mdl/backend/ interface and implemented in mdl/backend/mpr/
  • For microflow actions, this would be in the microflow builder, which seems to be handled in the executor changes
• LSP — if the feature adds formatting, diagnostics, or navigation targets, wire it into cmd/mxcli/lsp.go and register the capability
  • Not mentioned in the PR, and this seems like a microflow statement change that wouldn't need LSP changes
• DESCRIBE roundtrip — if the feature creates artifacts, describe should output re-executable MDL
  • The PR mentions this in the summary: "Writes the expression value into ChangeListAction.Value during rebuild"
  • There's a doctype test that should verify this roundtrip
• VS Code extension — if new LSP capabilities are added, update vscode-mdl/package.json
  • Not applicable here

Test coverage

• New packages should have test files
  • Not applicable (no new packages)
• New executor commands should have MDL examples in mdl-examples/doctype-tests/
  • The PR adds mdl-examples/doctype-tests/add_expression_to_list.mdl
• Integration paths should be tested, not just helpers
  • The PR includes:
    • go test ./mdl/visitor -run TestAddToList
    • go test ./mdl/executor -run TestAddToListBuilder
    • ./bin/mxcli check mdl-examples/doctype-tests/add_expression_to_list.test.mdl
• No time.Sleep for synchronization — use channels or polling with timeout
  • I don't see any time.Sleep in the changes

Security & robustness

• Command injection, XSS, SQL injection, temp file issues
  • Not applicable to this microflow statement change
• Unix sockets should use restrictive permissions (0600)
  • Not applicable
• File I/O should not be in hot paths (event loops, per-keystroke handlers)
  • Not applicable
• No silent side effects on typos (e.g., auto-creating resources on misspelled names)
  • The changes look safe
• Correct method receivers (pointer vs value) for mutations
  • Hard to verify from the diff, but nothing obviously wrong

Scope & atomicity

• Each commit should do one thing — a feature, a bugfix, or a refactor, not a mix
  • The PR appears to be focused on one feature: allowing expressions in add-to-list statements
• PR should be scoped to a single feature or concern — flag if description needs "and" between unrelated items
  • The description is focused on one feature
• Independent features should be in separate PRs
  • Only one feature here
• Refactors touching many files should be their own commit
  • Not really a refactor

Code quality

• Refactors should be applied consistently (look for old patterns still present)
  • Not a refactor
• Manually maintained lists (keyword lists, type mappings) should be flagged as maintenance risks
  • Not applicable
• Design docs should match the actual implementation
  • The PR updates the skill file and quick reference

Bugs & correctness

Let me look for potential issues:

1. In the AST modification:

   // AddToListStmt represents: ADD expr TO $List
   type AddToListStmt struct {
   -	Item        string               // Item variable to add
   +	Item        string               // Item variable to add, kept for simple $Var compatibility
   +	Value       Expression           // Item expression to add
   	List        string               // Target list variable
   	Annotations *ActivityAnnotations // Optional @position, @caption, @color, @annotation
   }

   This looks correct - keeping Item for compatibility and adding Value for expressions.

2. In the executor:

   // addAddToListAction creates an ADD TO list statement.
   func (fb *flowBuilder) addAddToListAction(s *ast.AddToListStmt) model.ID {
   +	value := fb.exprToString(s.Value)
   +	if value == "" && s.Item != "" {
   +		value = "$" + s.Item
   +	}
   	action := &microflows.ChangeListAction{
    	BaseElement:    model.BaseElement{ID: model.ID(types.GenerateID())},
    	Type:           microflows.ChangeListTypeAdd,
    	ChangeVariable: s.List,
   -		Value:          "$" + s.Item,
   +		Value:          value,
   	}
   
   	activity := &microflows.ActionActivity{

   This logic looks correct - use the expression if available, fall back to the simple variable form.

3. In the validation:

   func referencedVars(stmt ast.MicroflowStatement) []string {
   case *ast.AddToListStmt:
   -	refs = append(refs, s.Item, s.List)
   +	if s.Value != nil {
   +		refs = append(refs, exprVarRefs(s.Value)...)
   +	} else {
   +		refs = append(refs, s.Item)
   +	}
   +	refs = append(refs, s.List)

   This correctly handles variable references in expressions.

4. The exprVarRefs function was updated to handle additional expression types:

   func exprVarRefs(expr ast.Expression) []string {
   case *ast.ParenExpr:
    	refs = append(refs, exprVarRefs(e.Inner)...)
   +	case *ast.IfThenElseExpr:
   +		refs = append(refs, exprVarRefs(e.Condition)...)
   +		refs = append(refs, exprVarRefs(e.ThenExpr)...)
   +		refs = append(refs, exprVarRefs(e.ElseExpr)...)
   +	case *ast.SourceExpr:
   +		refs = append(refs, exprVarRefs(e.Expression)...)

   This looks correct for handling variable references in complex expressions.

Potential issues I notice:

1. I don't see explicit changes to the visitor code that would populate the new Value field in AddToListStmt. The AST was modified to store the expression, but without visitor changes, the Value field would never be populated. Let me double-check the diff...

Looking at the diff again, I don't see any changes to mdl/visitor/. This is a potential issue - the grammar was changed to parse expression instead of VARIABLE, but if the visitor isn't updated to store that expression in the AST node, then the Value field will always be nil.

However, looking more carefully at the grammar change:

addToListStatement
-    : ADD VARIABLE TO VARIABLE
+    : ADD expression TO VARIABLE

And in the AST:

type AddToListStmt struct {
	Item        string               // Item variable to add, kept for simple $Var compatibility
	Value       Expression           // Item expression to add
	List        string               // Target list variable
	Annotations *ActivityAnnotations // Optional @position, @caption, @color, @annotation
}

For the visitor to work correctly, it would need to:

1. Parse the expression and store it in Value
2. For backward compatibility, if the expression is a simple variable, also store it in Item

Since I don't see visitor changes in the diff, this is likely missing. But let me check if the visitor changes might be in a part of the diff that wasn't shown (the note said the diff was truncated).

Actually, looking at the validation changes, they reference exprVarRefs(s.Value), which suggests that Value is being populated somewhere. And the test in cmd_microflows_builder_add_to_list_test.go shows:

fb.addAddToListAction(&ast.AddToListStmt{
	Value: &ast.AttributePathExpr{
		Variable: "Order",
		Path:     []string{"Number"},
	},
	List: "Numbers",
})

This suggests that in tests, they're directly setting the Value field, which implies the visitor must be populating it correctly in real usage.

Let me think about how the visitor would work:

• When it sees ADD expression TO VARIABLE, it would:
  1. Parse the expression and store it in Value
  2. Check if the expression is a simple variable (like $Item), and if so, also store that variable name in Item for backward compatibility

This logic would need to be in the visitor. Since I don't see visitor changes in the diff, but the tests and validation logic depend on Value being populated, I suspect the visitor changes might be in a part of the diff that wasn't shown in the truncated view, or perhaps they were made in a way that didn't show up in the diff I'm looking at.

Actually, looking at the files changed in the diff:

• .claude/skills/mendix/write-microflows.md
• docs/01-project/MDL_QUICK_REFERENCE.md
• docs/11-proposals/PROPOSAL_microflow_add_expression_to_list.md
• docs/11-proposals/README.md
• mdl-examples/doctype-tests/add_expression_to_list.mdl
• mdl/ast/ast_microflow.go
• mdl/executor/cmd_microflows_builder_actions.go
• mdl/executor/cmd_microflows_builder_add_to_list_test.go
• mdl/executor/cmd_microflows_builder_graph.go
• mdl/executor/validate_microflow.go
• mdl/executor/validate_microflow_add_to_list_test.go
• mdl/grammar/MDLParser.g4
• mdl/grammar/parser/MDLParser.interp
• mdl/grammar/parser/MDLParser.g4 (wait, this is listed twice?)

I don't see any visitor files in the list of changes. This is concerning.

However, let me check if the visitor changes might be implicit. Sometimes when you modify the AST, the visitor changes are straightforward and might not have been considered worth showing in the diff summary, but they should still be there.

Actually, looking at the PR description: "Stores the parsed expression on AddToListStmt while preserving the legacy simple-variable Item field." This implies the visitor was modified to do this storage.

Given that:

1. The tests show the Value field being used
2. The validation logic checks Value
3. The executor uses Value
4. The PR description states the expression is stored

I think it's reasonable to assume the visitor was modified appropriately, even if those changes aren't visible in the diff excerpt we're looking at (possibly because the diff was truncated or the visitor changes were in a section not shown).

Let me check one more thing - in the microflow builder test, they're directly constructing an AddToListStmt with a Value field, which suggests that in normal operation, the visitor would be setting this field.

Actually, wait - I just realized something. The visitor changes might not be in the diff because they're in a file that wasn't included in the truncated diff. The note said: "NOTE: The diff was truncated to 80k characters. Total size: 627786 bytes." So it's very possible the visitor changes are in the part that wasn't shown.

Given the evidence:

• The PR description says it stores the parsed expression
• The tests and validation logic depend on Value being set
• The executor uses Value with a fallback to Item

I think it's safe to assume the visitor was modified correctly.

Let me check for any other potential issues:

In the quick reference update:

| Add to list | `add expression to $list;` | Also accepts existing `add $item to $list;` form |

This looks good - it shows both forms.

In the skill file update:

## List Operations

```mdl
-- Existing variable form
add $Item to $Items;

-- Expression-valued add, useful when round-tripping Studio Pro list-add values
add head($SourceItems) to $Items;

Use expression-valued add only when the expression returns an object compatible with the target list element type.

This also looks good - it explains the new syntax and when to use it.

The proposal document looks comprehensive.

Now, let me check if there are any violations of the checklist:

## Overlap & duplication
- No existing proposal for this exact feature (I checked the proposals list in the context)
- No duplication of existing implementations
- Added new doctype test, doesn't re-document existing features

## MDL syntax design
- Follows standard patterns (modifying existing statement, not adding new verb)
- Reads as English
- For microflow variables, qualified names concept doesn't apply directly, but it's using standard variable syntax
- Not applicable for property format
- LLM-friendly with examples
- Diff-friendly (the changes are localized)
- Consistent with existing MDL patterns (quick reference updated)

## Full-stack consistency
- Grammar: changed in MDLParser.g4
- Parser regenerated: shown in diff (generated files)
- AST: modified in ast_microflow.go
- Visitor: assumed to be modified (not in visible diff but implied by other changes)
- Executor: modified in cmd_microflows_builder_actions.go
- Backend: for microflows, this is handled in the executor
- LSP: not needed for this change
- DESCRIBE roundtrip: tested via the doctype test
- VS Code extension: not needed

## Test coverage
- Added doctype test: mdl-examples/doctype-tests/add_expression_to_list.mdl
- Added unit tests for executor and validation
- Integration paths tested (the mxcli check command)

## Security & robustness
- No obvious security issues
- No file I/O in hot paths
- No silent side effects on typos (the validation should catch undefined variables in expressions)

## Scope & atomicity
- Focused on single feature
- No unrelated items in description

## Code quality
-

---
*Automated review via OpenRouter (Nemotron Super 120B) — [workflow source](https://github.com/mendixlabs/mxcli/blob/main/.github/workflows/ai-review.yml)*