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