fix: reject duplicate microflow output variables

[hjotha]

Summary

Rejects duplicate microflow output variables instead of silently generating aliases.

If an authored MDL script declares the same output variable twice in the same scope, validation now reports a duplicate-variable error. If describe encounters an existing MPR that already contains duplicate output variables, it preserves the original output variable names and emits a warning comment instead of transforming the model to generated names such as $Item_2.

Part of #332.

Changes

• Removes the silent variable-aliasing machinery for duplicate implicit outputs.
• Rejects duplicate output variables from create/retrieve/call/list/import/export/transform statements during validation.
• Emits -- WARNING: duplicate output variable ... model is invalid in describe output for invalid existing MPRs.
• Keeps shared variable-reference walker coverage for downstream builder passes.
• Removes aliasing proposal/docs/fixtures because the reviewed behavior is intentionally not supported.

Validation

• go test ./mdl/executor -run 'DuplicateOutput|StatementVarRefs' -count=1
• make build
• make test

[github-actions]

AI Code Review

What Looks Good

• The PR addresses a clear issue (duplicate implicit output variables breaking round-trip preservation) with a focused solution
• Changes are isolated to the microflow builder logic without affecting other areas
• Comprehensive test coverage includes both unit tests (cmd_microflows_builder_variable_alias_test.go) and doctype test (variable_alias_collision.test.mdl)
• Documentation updates (MDL_QUICK_REFERENCE.md and new proposal) clearly explain the behavior
• The aliasing logic correctly handles position-scoping, variable declaration checks, and reference resolution
• Follows the project's validation process (build, lint, test passes)

Recommendation

Approve the PR. The changes are correct, well-tested, and properly documented. No blocking issues found.

---

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

[github-actions]

AI Code Review

Critical Issues

• None found

Moderate Issues

• None found

Minor Issues

• In mdl/executor/cmd_microflows_builder_calls.go, the change to addCallMicroflowAction introduces a new variable useReturnVariable that duplicates logic from the outer scope. The variable is set based on fb.callOutputDeclarations[s] but then immediately rechecked in the condition if s.OutputVariable != "" && useReturnVariable. This could be simplified to avoid the extra variable, though it's not incorrect.

What Looks Good

• The fix correctly addresses duplicate implicit output variable collisions by assigning deterministic aliases (Foo_2, Foo_3) and rewriting downstream references.
• Comprehensive test coverage added: unit test (cmd_microflows_builder_variable_alias_test.go), bug test (mdl-examples/bug-tests/variable-alias-collision.mdl), and doctype test (mdl-examples/doctype-tests/variable_alias_collision.test.mdl).
• Documentation updated in MDL quick reference and a new proposal file explains the round-trip preservation rule.
• Changes are focused and scoped to the specific issue (duplicate implicit outputs at same position), following the atomicity principle.
• All resolver functions (resolveVariableName, resolveVariablePath, uniqueImplicitOutputVariable) are properly integrated throughout the microflow builder (variable declarations, change actions, create/retrieve actions, call microflows).
• The solution maintains backward compatibility and only affects round-trip scenarios (describe → exec → describe).

Recommendation
Approve the PR. The changes are correct, well-tested, and follow project conventions. The minor code duplication in call handling is trivial and does not affect correctness.

---

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

[hjotha]

Follow-up pushed in 5c3dbca.

This narrows duplicate-output aliasing to variables that are live on the continuation path. Terminal branch-local declarations no longer force _2 aliases outside that branch, while the existing same-position duplicate collision case remains covered.

Validation rerun on the branch:

• make build
• make test
• make lint-go
• go test ./mdl/executor -run 'TestBuildFlowGraph_(DuplicateImplicitOutputAtSamePositionGetsLocalAlias|TerminalBranchDuplicateOutputDoesNotForceAlias)' -count=1

[github-actions]

AI Code Review

Let me analyze this PR against the review checklist.

First, let's understand what this PR does:

• It's titled "fix: preserve duplicate implicit output aliases"
• The summary says it "Preserves duplicate implicit output variables by assigning deterministic local aliases such as Foo_2 and resolving downstream references to the active alias."
• It also "preserves duplicate call-microflow result names that are carried by the model but not actually declared or referenced, keeping UseReturnVariable=false instead of turning them into invalid duplicate declarations."
• Part of issue Microflow roundtrip stabilization for large-project audit findings #332

Now let me review against the checklist:

Overlap & duplication

• Check proposals list above for existing proposals covering the same functionality
  • There's a new proposal file: docs/11-proposals/PROPOSAL_microflow_variable_alias_collision.md
  • This is appropriate as it's documenting the fix
• Check if the diff introduces code that duplicates existing implementations (look for similar function names, command names, types)
  • The changes seem focused on flowBuilder in the microflow executor
  • I don't see obvious duplication of existing implementations
• Check doctype test files above for existing test coverage of the feature area
  • There are new test files:
    • mdl-examples/bug-tests/variable-alias-collision.mdl
    • mdl-examples/doctype-tests/variable_alias_collision.test.mdl
    • mdl/executor/cmd_microflows_builder_variable_alias_test.go (new test file)
  • These look appropriate for testing the new functionality
• Flag if the PR re-documents already-shipped features as new
  • This appears to be a fix for an existing issue, not documenting already-shipped features as new

MDL syntax design (for new or modified MDL syntax)

• The PR doesn't appear to add or modify MDL syntax itself (no changes to grammar files mentioned)
  • It's fixing how the builder handles duplicate implicit outputs
  • The changes are in the executor/builder code, not in the MDL language definition
  • There is an update to MDL_QUICK_REFERENCE.md to document the behavior
  • But this is documenting existing behavior, not adding new syntax
• Since there's no new MDL syntax being added, this section doesn't really apply
  • However, the documentation update in MDL_QUICK_REFERENCE.md should be checked
  • The update adds a line about duplicate implicit output: "| Duplicate implicit output | $Var, $Var_2, $Var_3 | Describe may alias same-position duplicate outputs for round-trip preservation |"
  • This follows the existing pattern in the reference document
  • It's descriptive and reads as English
  • It uses qualified names conceptually (though it's showing examples with simple names)
  • It's consistent with the format

Full-stack consistency (for MDL features)

• Since this is a fix to existing functionality rather than a new MDL feature, the full-stack consistency requirements don't strictly apply
  • However, let's check if the changes are properly layered:
  • Changes are in:
    • mdl/executor/cmd_microflows_builder.go (core logic)
    • mdl/executor/cmd_microflows_builder_actions.go (applying the logic to various actions)
    • mdl/executor/cmd_microflows_builder_calls.go (handling call microflow outputs)
    • mdl/executor/cmd_microflows_builder_control.go (handling control flow)
    • mdl/executor/cmd_microflows_builder_graph.go (planning logic)
    • Plus test files and documentation
  • This follows the pattern of changes being in the executor layer where the flowBuilder lives
  • No changes to grammar, AST, visitor, or backend - which makes sense since this is about how the builder processes existing AST
  • The fix is in the right place: the flowBuilder that constructs the microflow graph from AST

Test coverage

• New packages should have test files
  • No new packages, but there is a new test file: cmd_microflows_builder_variable_alias_test.go
• New executor commands should have MDL examples in mdl-examples/doctype-tests/
  • There's a new doctype test: variable_alias_collision.test.mdl
• Integration paths should be tested, not just helpers
  • There are bug test files and doctype test files
  • The PR mentions validation including running specific tests
• 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 builder fix
• 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 fix seems to be about correctly handling variable references, not creating resources
• Correct method receivers (pointer vs value) for mutations
  • The changes look to be using pointer receivers appropriately (fb *flowBuilder)

Scope & atomicity

• Each commit should do one thing — a feature, a bugfix, or a refactor, not a mix
  • The PR description indicates it's focused on preserving duplicate implicit output aliases
  • It also handles duplicate call-microflow result names
  • These are related aspects of the same issue (variable name collisions in microflows)
  • Seems appropriately scoped
• PR should be scoped to a single feature or concern — flag if description needs "and" between unrelated items
  • The description mentions two related things: implicit output aliases and call-microflow result names
  • Both are about handling duplicate variable names in microflows
  • Seems like a single concern
• Independent features should be in separate PRs
  • Doesn't appear to have unrelated features
• Refactors touching many files should be their own commit
  • The PR shows multiple files changed, but they're all related to the flowBuilder logic
  • Seems like a cohesive change

Code quality

• Refactors should be applied consistently (look for old patterns still present)
  • The changes introduce new patterns (variable aliasing) but seem consistent within themselves
• Manually maintained lists (keyword lists, type mappings) should be flagged as maintenance risks
  • I don't see any such lists being modified
• Design docs should match the actual implementation
  • The proposal document and reference document updates seem to match the implementation

Bugs & correctness

• Logic errors, race conditions, resource leaks
  • Let me think through the logic:
    • For duplicate implicit outputs at same position: assigns numeric aliases (_2, _3, etc.) and rewrites references
    • For duplicate call-microflow outputs that aren't referenced: keeps UseReturnVariable=false instead of declaring duplicate variables
  • The implementation looks correct:
    • uniqueImplicitOutputVariable() checks if we've seen this variable at this position before
    • If so and it's already declared, it assigns an alias
    • Various methods (resolveVariableName, resolveVariablePath) use the aliases when resolving references
    • For call microflows, planMicroflowCallOutputDeclarations() determines when to declare return variables
• Error handling gaps
  • I don't see obvious error handling gaps in the changes

Let me check a few specific implementation details:

1. In cmd_microflows_builder.go:

   • Added fields: callOutputDeclarations, variableAliases, outputVarPositions
   • Added snapshot/restore functionality for variable state (important for control flow)
   • Added helper methods: resolveVariableName, resolveVariablePath, uniqueImplicitOutputVariable
   • uniqueImplicitOutputVariable() seems correct: checks if we've seen this var at current position before and if it's already declared

2. In cmd_microflows_builder_actions.go:

   • Changed addCreateObjectAction to use uniqueImplicitOutputVariable for the output variable
   • Changed addChangeObjectAction to use resolveVariableName
   • Changed addRetrieveAction to use uniqueImplicitOutputVariable for output and resolveVariableName for start variable
   • These changes look correct - they're applying the aliasing/resolution where needed

3. In cmd_microflows_builder_calls.go:

   • Added logic to determine useReturnVariable based on whether the output is referenced
   • This prevents declaring duplicate variables for unused call outputs
   • The logic looks correct: useReturnVariable = s.OutputVariable != "" && fb.callOutputDeclarations[s]
   • And then only register the result type if useReturnVariable is true

4. In cmd_microflows_builder_control.go:

   • Added snapshot/restore logic for if statements to handle variable scoping correctly across branches
   • This is important because variable aliases should not leak between branches inappropriately
   • The implementation looks reasonable

5. In cmd_microflows_builder_graph.go:

   • Added planning logic for call microflow outputs (planMicroflowCallOutputDeclarations)
   • Added helper functions to count outputs, track referenced variables, etc.
   • This logic determines when to actually declare a call microflow's return variable

The test file cmd_microflows_builder_variable_alias_test.go looks good:

• Tests duplicate implicit output at same position gets local alias
• Tests that terminal branch duplicate output doesn't force alias (important edge case)
• Tests unused call outputs don't declare return variables

The bug test and doctest files look appropriate for testing the end-to-end behavior.

One thing I notice: the PR updates MDL_QUICK_REFERENCE.md to document the behavior. This is good for users to understand what DESCRIBE might produce.

Let me check if there are any issues with the implementation:

In uniqueImplicitOutputVariable():

if previous, ok := fb.outputVarPositions[varName]; ok &&
	previous.X == position.X && previous.Y == position.Y &&
	fb.isVariableDeclared(varName) {

The condition checks fb.isVariableDeclared(varName). But when we're processing the second occurrence, the first occurrence has already been processed and should have declared the variable. So this should be true for the second and subsequent occurrences.

However, there's a potential issue: what if the first occurrence hasn't been processed yet? But since we're processing statements sequentially, when we see the second occurrence, the first should have already been processed.

Actually, looking more carefully: the method is called when processing a CreateObjectStmt or RetrieveStmt. For the first occurrence, fb.outputVarPositions[varName] won't exist yet, so it will skip the aliasing logic and just store the position. For the second occurrence at the same position, it will find the previous position, see that it matches, and check if the variable is declared (which it should be from the first occurrence).

This seems correct.

Another thing: in the alias generation loop:

for i := 2; ; i++ {
	candidate := fmt.Sprintf("%s_%d", varName, i)
	if !fb.isVariableDeclared(candidate) {
		fb.variableAliases[varName] = candidate
		return candidate
	}
}

This will keep incrementing until it finds an undeclared name. This should work, but theoretically could loop forever if all possible aliases are taken. In practice, with reasonable variable names, this shouldn't be an issue.

The implementation in cmd_microflows_builder_calls.go for call microflow outputs:

useReturnVariable := s.OutputVariable != ""
if s.OutputVariable != "" && fb.callOutputDeclarations != nil {
	useReturnVariable = fb.callOutputDeclarations[s]
}

action := &microflows.MicroflowCallAction{
	// ...
	UseReturnVariable:  useReturnVariable,
}

// ...

if s.OutputVariable != "" && useReturnVariable {
	fb.registerResultVariableType(s.OutputVariable, fb.lookupMicroflowReturnType(mfQN))
}

This looks correct. It determines whether to use the return variable based on planning, and only registers the result type if we're actually going to use the return variable.

The variable state snapshot/restore in control flow looks important for correctness. Without this, variable aliases could leak between branches inappropriately.

Let me check if there are any missing pieces:

• Are there other places where variable names are used that need to be updated to use the resolved names?
  • I see changes in:
    • addCreateObjectAction
    • addChangeObjectAction
    • addRetrieveAction
    • addCallMicroflowAction (indirectly through the useReturnVariable logic)
    • In cmd_microflows_builder.go: resolveAssociationPaths (for VariableExpr and AttributePathExpr)
    • In cmd_microflows_builder_control.go: the if statement handling uses snapshot/restore

I think that covers the main places where variable names are used in microflow activities.

One potential issue: in cmd_microflows_builder.go, the resolveAssociationPaths function handles VariableExpr and AttributePathExpr, but what about other expression types that might contain variable references?

Looking at the switch statement in resolveAssociationPaths:

switch e := expr.(type) {
case *ast.VariableExpr:
	return &ast.VariableExpr{Name: fb.resolveVariableName(e.Name)}
case *ast.AttributePathExpr:
	// ... uses fb.resolveVariableName(e.Variable)
// ... other cases

It doesn't handle other expression types that might contain variables, like:

• BinaryExpr (could have variables on left/right)
• UnaryExpr
• CallExpr (could have variables as arguments)
• etc.

This could be a problem! If there are variable references in expressions that aren't handled by resolveAssociationPaths, they won't get the aliasing applied.

Let me check if there are other places where expressions are processed:

In cmd_microflows_builder_actions.go:

• addChangeVariableAction uses fb.exprToString(s.Value) - this converts the whole expression to a string, so if the expression contains variable names that need aliasing, they

---

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

[ako]

PR #337 — fix: preserve duplicate implicit output aliases

Overview: Two independent fixes for alias collisions in the microflow builder:

1. Implicit output collisions (create/retrieve producing the same $Var at the same canvas position): assigns Foo_2, Foo_3 suffixes via uniqueImplicitOutputVariable().
2. Duplicate call-microflow result names (same output variable across two branches, not referenced): sets UseReturnVariable=false via pre-pass planMicroflowCallOutputDeclarations().

The variable-state snapshot/restore in addIfStatement is the right approach for scoping aliases to branches. Tests are solid. Good to see a bug-test file alongside the doctype fixture.

---

Moderate

statementVarRefs has incomplete AST coverage — AggregateListStmt, ListOperationStmt, SendRestRequestStmt, CallExternalActionStmt, ExportToMappingStmt, and CreateListStmt are not handled. referencedVars is used to decide whether a duplicated call-microflow output is safe to suppress (UseReturnVariable=false). A variable referenced only via one of these missing types will be mis-classified as unreferenced, causing UseReturnVariable=false when the downstream code still reads the variable.

Concrete failing pattern:

$Items = call Module.GetItems();   -- branch A
$Items = call Module.GetItems();   -- branch B, same name
$Count = COUNT($Items);            -- AggregateListStmt — not seen by statementVarRefs

Both call outputs get UseReturnVariable=false → $Items never declared → COUNT($Items) references undefined variable.

Fix: add the missing cases to statementVarRefs in cmd_microflows_builder_graph.go:

case *ast.AggregateListStmt:
    refs = append(refs, s.InputVariable)
case *ast.ListOperationStmt:
    refs = append(refs, s.InputVariable, s.SecondVariable)
case *ast.CallExternalActionStmt:
    for _, arg := range s.Arguments {
        refs = append(refs, exprVarRefs(arg.Value)...)
    }
case *ast.CreateListStmt:
    // no refs consumed (output-only)
case *ast.SendRestRequestStmt:
    refs = append(refs, exprVarRefs(s.Body)...)

---

Minor

doctype-tests/variable_alias_collision.test.mdl missing prerequisites — the file creates SampleAliases.ACT_DuplicateOutputPosition with return type SampleAliases.Item but never creates the module or entity. Running this against a project without that setup will fail. Compare 02-microflow-examples.mdl which opens with create module MfTest; and entity definitions. Add:

create module SampleAliases;
create entity SampleAliases.Item ( Name : string(100) );
/

before the microflow block (matching the bug-test file which already has this).

---

countMicroflowCallOutputs + inner walk in planMicroflowCallOutputDeclarations duplicate ~40 lines of identical switch boilerplate. Each time a new statement type gains an ErrorHandling field or output variable, it must be added in two places. A shared walkStatements(body []ast.MicroflowStatement, fn func(ast.MicroflowStatement)) helper extracting the recursion would halve the maintenance surface. Not a blocker for this PR, but worth a follow-up.

---

Proposal status is still "Draft" — PROPOSAL_microflow_variable_alias_collision.md line 3. Once this PR merges, it should read Status: Implemented.

---

MDL_QUICK_REFERENCE.md description is slightly misleading — "Describe may alias same-position duplicate outputs for round-trip preservation." The alias is assigned by the builder, which runs on both exec and describe paths. Suggested wording: "Builder assigns numeric aliases when duplicate implicit outputs collide at the same canvas position".

---

Recurring Findings table update

Suggest adding row 11:

| 11 | AST walker switch-case duplicated across multiple functions — new statement types must be added in every copy | Code quality | Extract a shared walkStatements(body, fn) helper; pass a per-function closure for the per-node logic |

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• In mdl/executor/cmd_microflows_builder_control.go, the variable postIfVarStateSet is initialized but then reassigned in multiple places. While functionally correct, this could be slightly clearer if declared without initialization and set only when needed.
• The comment in mdl/executor/cmd_microflows_builder_variable_alias_test.go for TestStatementVarRefsIncludesNonCallConsumers could be more concise, but this is very minor.
• The proposal documentation mentions "round-trip-only aliasing rule" but the implementation affects both describe and exec paths - this is actually correct behavior but could be clarified in the documentation.

What Looks Good

1. Comprehensive Fix: The solution properly handles duplicate implicit outputs at the same position by assigning deterministic aliases and rewriting all downstream references (variable paths, attribute paths, change targets, retrieve start variables).
2. Call Flow Handling: Correctly preserves duplicate call-microflow result names that aren't referenced by avoiding invalid duplicate declarations.
3. Test Coverage: Excellent test coverage including:
   • Unit tests for the aliasing logic
   • Bug test demonstrating the issue and fix
   • Doctype test for round-trip validation
   • Extended existing terminal tests
4. Documentation:
   • Added proposal documenting the behavior
   • Updated MDL quick reference with explanation
   • Clear usage instructions in test files
5. Code Quality:
   • Proper variable state snapshotting/restoring for control flow
   • Consistent application of alias resolution across all relevant builder methods
   • Clean separation of concerns (tracking positions, resolving names, handling call outputs)
   • No disruption to existing functionality (verified by terminal test showing non-terminal positions don't force aliasing)

Recommendation

Approve the pull request. The implementation correctly addresses the duplicate implicit output variable collision issue with a deterministic, round-trip-preserving solution. The changes are minimal, focused, and thoroughly tested with appropriate documentation. The solution maintains backward compatibility while fixing the described round-trip failure mode.

---

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

[github-actions]

AI Code Review

What Looks Good

• Focused fix: The PR addresses a specific issue (variable alias collision in microflows) with a targeted solution that preserves model validity during round-trip operations.
• Comprehensive testing: Includes unit tests covering duplicate implicit outputs, terminal branch behavior, and unused call microflow outputs, plus doctype and bug test fixtures.
• Documentation: Updates MDL_QUICK_REFERENCE.md, adds a detailed proposal file, and includes clear test fixtures that explain the problem and solution.
• Proper scoping: Changes are confined to the microflow builder where variable resolution occurs, with appropriate handling of branching logic (if/else) via state snapshots.
• Call microflow handling: Correctly avoids declaring duplicate return variables for unused outputs, preventing invalid BSON generation.
• Consistent implementation: Variable aliasing logic is applied uniformly across create/retrieve/change operations and downstream references (variables, attribute paths, etc.).

Recommendation

Approve - The PR is well-scoped, thoroughly tested, properly documented, and implements a sound solution for preserving duplicate implicit output aliases during microflow round-trip operations. It maintains backward compatibility while fixing the described issue without introducing new syntax or breaking existing functionality.

---

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

[hjotha]

Review follow-up status on the current head:

• Extracted the variable-reference walker to cmd_microflows_builder_varutil.go.
• Added coverage for the missing statement kinds noted in review, including aggregate/list operations, REST/request mapping cases, external actions, export mapping, and list add/remove consumers.
• Updated the doctype fixture with its own synthetic module/entity prerequisites.
• Marked the proposal as implemented and updated the quick-reference wording to describe the builder behavior.

Local validation after the update was green:

make build
make test
make lint-go

[hjotha]

Review follow-up status:

• Extracted the variable-reference walker to cmd_microflows_builder_varutil.go so this PR and fix: preserve custom error handler continuations #366 share one canonical implementation instead of defining duplicate package-level helpers.
• Added the missing AST coverage for aggregate/list/external action/REST/export statements.
• Added the missing doctype prerequisites and marked the variable alias proposal as implemented.
• Updated the quick-reference wording to describe the builder behavior directly.

Local validation after the update:

make build
make test

[ako]

Design concern: silent aliasing treats an invalid model as valid

The premise of this PR is that legacy projects can contain duplicate output variable names that "Studio Pro can keep in the model." But Studio Pro does not allow two activities to share the same output variable name — it flags this as a model error. So a project exhibiting this pattern is already invalid, and the correct response is to surface that, not to paper over it with silent renaming.

Two specific problems:

1. DESCRIBE of an invalid model should say so, not emit $Item_2

If describe encounters two activities at the same canvas position with the same output name, the project is broken. Emitting $Item_2 in the describe output produces MDL that, when re-executed, creates a different model than the original (one with a genuinely distinct $Item_2 variable). The roundtrip is not a fixpoint — it silently transforms the model.

The right output is a comment flagging the problem:

-- WARNING: duplicate output variable $Item at position (100, 200) — model is invalid; open in Studio Pro to fix
$Item = create SampleAliases.Item (Name = 'second');

2. Authored MDL with duplicate output names should be rejected, not accepted

The example script:

$Item = create SampleAliases.Item (Name = 'first');
$Item = create SampleAliases.Item (Name = 'second');

This is invalid MDL and should produce a validation error:

error: variable '$Item' is already declared in this scope

Silently accepting it and renaming the second to $Item_2 means the user gets a model with a variable they never named, with no indication that anything was changed.

Suggested direction

• In the validator (validate_microflow.go): reject duplicate implicit output names with a clear error.
• In describe (cmd_microflows_show_helpers.go): if duplicate output names are detected while traversing an existing MPR, emit a -- WARNING comment rather than inventing aliases.

The aliasing machinery (uniqueImplicitOutputVariable, variableAliases, outputVarPositions) added here should be removed — it is handling a case that should never exist in a valid project.

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• In cmd_microflows_builder_actions.go, the comment added for RefreshInClient logic is helpful but slightly verbose for inline documentation. Consider moving detailed explanations to external documentation if this pattern appears elsewhere.

What Looks Good

• Targeted fix: Changes are precisely scoped to handle duplicate implicit outputs and unused call microflow outputs without affecting other functionality
• Comprehensive testing: Includes unit tests for various scenarios (same-position duplicates, branch handling, Java actions, SourceExpr handling) and integration tests via doctest/bugtest
• Correct aliasing logic: Properly scopes aliases to canvas positions, cleans up aliases when position changes, and avoids conflicts with explicitly declared variables
• Downstream reference resolution: Correctly rewrites variable references, attribute paths, change targets, and retrieve start variables through active aliases
• Call output handling: Smartly avoids declaring return variables for duplicated call outputs that aren't referenced, preventing invalid BSON generation
• State management: Properly saves/restores variable state in control flow constructs (if/else) to maintain alias correctness across branches
• Documentation: Adds clear explanation to MDL quick reference and implements a detailed proposal document
• Round-trip preservation: Validated through describe→exec→describe testing as mentioned in the PR
• Method receivers: All builder methods correctly use *flowBuilder receivers
• No syntax changes: Since this fixes behavior for existing MDL syntax rather than adding new syntax, no grammar/AST/visitor changes were needed (appropriate)

Recommendation

Approve. The PR correctly implements deterministic aliasing for duplicate implicit outputs while preserving round-trip validity. Changes are focused, well-tested, and maintain backward compatibility

---

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

[hjotha]

Review follow-up status:

• Reworked the branch to address the design concern: duplicate output variables are treated as invalid instead of silently aliased.
• Removed variableAliases, outputVarPositions, uniqueImplicitOutputVariable, aliasing docs, and aliasing fixtures.
• Added validation coverage for duplicate authored outputs and describe coverage that emits a warning for invalid existing MPRs without inventing $Item_2 names.
• Kept the shared variable-reference walker coverage that downstream builder passes use.

Validation on the updated branch:

go test ./mdl/executor -run 'DuplicateOutput|StatementVarRefs' -count=1
make build
make test

[github-actions]

AI Code Review

Critical Issues

• None found

Moderate Issues

• None found

Minor Issues

• The resolver functions resolveVariableName and resolveVariablePath in cmd_microflows_builder.go are currently identity functions (just return input). While this is acceptable as a placeholder for future aliasing logic, the actual aliasing implementation isn't visible in this diff. However, the PR body states aliasing is implemented, and tests cover the behavior.
• In cmd_microflows_builder_actions.go, the line removing OutputVariable: s.OutputVariable from MicroflowCallAction is correct (as this action uses ResultVariableName instead), but it would be safer to add a comment explaining why this removal is necessary to prevent confusion for future reviewers.
• The variable state snapshotting/restoring logic in cmd_microflows_builder_control.go (for if-statements) appears correct but could benefit from additional comments explaining the branch state merging strategy.

What Looks Good

• The changes are tightly scoped to solving duplicate implicit output variables and duplicate call-microflow result names without modifying MDL syntax.
• New utility file cmd_microflows_builder_varutil.go properly isolates variable reference tracking logic.
• Validation logic is centralized via validateOutputVariable, reducing duplication and ensuring consistent error messages across statement types.
• Doctype test fixture added (variable_alias_collision.test.mdl) and specific tests run validate the fix.
• DESCRIBE output now includes warnings for duplicate output variables (via duplicateOutputVariableWarnings), improving roundtrip visibility.

---

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

[hjotha]

Follow-up observation from the local roundtrip audit: the -- WARNING: duplicate output variable comment currently produced by the describer correctly flags 2 microflows in a real project whose flow graph has this shape:

enum split
├── case A → if pre-check → [true] → shared ExclusiveMerge ─┐
├── case B → if pre-check → [true] → shared ExclusiveMerge ─┤
│                                                           ↓
│                                              shared IF + action + return
├── case (empty) → log error → return
└── case other → log error → return

Two cases (A, B) fan in to a common ExclusiveMerge and the subtree beyond the merge does the actual work (a call java action → create object → return). The BSON stores a single copy of each shared action; mx check accepts this as a valid shape. On describe the walker enters the shared subtree once per case and emits $UUID = call java action … and $NewCompanyActivity = create … twice. Re-exec then produces a truly duplicated BSON that Studio Pro rejects with CE0111.

MDL does not have a native way to represent "shared subtree reachable from multiple cases" (no goto, no labeled block). This PR's current behavior — emit the warning, let the user fix it in Studio Pro — is still the right default. Just leaving a marker that the warning is not a describer bug in those cases; it's signalling an MDL representability gap.

No action requested on this PR.

[github-actions]

AI Code Review

Critical Issues

• None found.

Moderate Issues

• None found.

Minor Issues

• Unrelated changes in PR body: The PR includes two modifications not mentioned in the description:
  1. Atomic debounce improvement in cmd/mxcli/tui/watcher.go/watcher_test.go (robustness enhancement for file watching).
  2. Nanoflow test formatting fix in `mdl/executor/roundtrip_nano

---

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

Let me review this pull request according to the checklist provided.

Summary of the PR

The PR is titled "fix: reject duplicate microflow output variables" and its body states:

Rejects duplicate microflow output variables instead of silently generating aliases.

If an authored MDL script declares the same output variable twice in the same scope, validation now reports a duplicate-variable error. If describe encounters an existing MPR that already contains duplicate output variables, it preserves the original output variable names and emits a warning comment instead of transforming the model to generated names such as $Item_2.

The changes include:

• Removes the silent variable-aliasing machinery for duplicate implicit outputs
• Rejects duplicate output variables from create/retrieve/call/list/import/export/transform statements during validation
• Emits -- WARNING: duplicate output variable ... model is invalid in describe output for invalid existing MPRs
• Keeps shared variable-reference walker coverage for downstream builder passes
• Removes aliasing proposal/docs/fixtures because the reviewed behavior is intentionally not supported

Let me now review this against the checklist:

Overlap & duplication

First, I need to check if there are existing proposals covering the same functionality. The PR mentions "Part of #332" but there's no proposals directory listed in the context, so I'll assume there are no conflicting proposals.

Looking at the diff, I see changes to:

1. cmd/mxcli/tui/watcher.go and watcher_test.go (unrelated to the main feature - seems to be about debouncing in the TUI)
2. mdl-examples/doctype-tests/02b-nanoflow-examples.mdl (significant simplification of nanoflow examples)
3. Several files in mdl/executor/ related to microflow building and validation
4. New test file: mdl/executor/cmd_microflows_duplicate_output_test.go
5. Changes to mdl/executor/cmd_microflows_show.go
6. Changes to roundtrip_doctype_test.go and roundtrip_nanoflow_test.go
7. Changes to mdl/visitor/visitor_javaaction.go and visitor_javaaction_test.go

The core changes are in the microflow executor, specifically around validating duplicate output variables.

Let me check if this duplicates existing functionality:

• The PR removes "silent variable-aliasing machinery for duplicate implicit outputs"
• It adds validation to reject duplicate output variables
• It changes the describe output to warn instead of transforming

Looking at the doctype test files listed in the context, I don't see any that specifically test duplicate output variable handling, so this appears to be new functionality rather than duplicating existing tests.

The nanoflow examples file (02b-nanoflow-examples.mdl) was significantly simplified, removing many examples. This seems to be cleanup unrelated to the main feature, but it's part of this PR.

MDL syntax design

The PR doesn't appear to add or modify any MDL syntax (grammar rules, keywords, statement types). It's changing how existing syntax is validated and handled.

Looking at the changes:

• No new grammar rules
• No new tokens
• No new statement types
• Just changes to validation and output formatting

So the MDL syntax design checklist doesn't really apply here since no MDL syntax is being added or modified.

Full-stack consistency

Since this isn't adding new MDL syntax (just changing validation/error handling of existing syntax), the full-stack consistency requirements don't apply in the same way.

However, let's check if the changes are properly layered:

• Validation is happening in the executor (mdl/executor/cmd_microflows_builder_validate.go)
• The describe output changes are in mdl/executor/cmd_microflows_show.go
• There's a new test file for the validation logic

This seems properly layered - validation in the builder/validator, output formatting in the show/describe functionality.

Test coverage

Let's check the test coverage:

1. New test file: mdl/executor/cmd_microflows_duplicate_output_test.go - this tests:

   • ValidateMicroflowBodyRejectsDuplicateImplicitOutputs
   • ValidateMicroflowBodyRejectsDuplicateCallOutputs
   • FormatMicroflowActivitiesWarnsAboutDuplicateModelOutputs

2. Changes to existing tests:

   • cmd/mxcli/tui/watcher_test.go (minor test adjustment)
   • roundtrip_doctype_test.go (updated known CE errors)
   • roundtrip_nanoflow_test.go (minor syntax fix)
   • mdl/visitor/visitor_javaaction_test.go (added test for explicit void return type)

The new test file specifically tests the duplicate output variable validation, which is good. It covers:

• Create + Retrieve duplicate
• Call microflow + Call Java action duplicate
• Describe output warning for existing models with duplicates

The changes to roundtrip_doctype_test.go show they're aware that some existing tests might now produce CE errors due to this change, and they've updated the known errors list accordingly.

Security & robustness

Looking at the changes:

• No obvious security issues (command injection, XSS, etc.)
• The changes are about validation and error reporting
• No file I/O in hot paths mentioned
• No Unix sockets involved
• The changes add proper error checking rather than removing it

The validation now properly errors on duplicate variables instead of silently aliasing them, which is more robust.

Scope & atomicity

The PR description says it's about "rejecting duplicate microflow output variables instead of silently generating aliases."

Looking at the changes:

• Core logic: validation changes in cmd_microflows_builder_validate.go
• Describe output: cmd_microflows_show.go
• Tests: new test file + updates to existing test files
• Unrelated: TUI watcher debouncing improvements
• Unrelated: Nanoflow examples simplification
• Unrelated: Java action visitor changes (void return type handling)

The TUI watcher changes, nanoflow examples simplification, and Java action visitor changes seem unrelated to the main feature. This violates the "each PR should be scoped to a single feature or concern" principle.

However, let me check if these might be related:

• The TUI watcher changes: improving debouncing - could be considered infrastructure improvement
• Nanoflow examples simplification: removing outdated examples - could be considered cleanup
• Java action visitor changes: fixing return type handling for Void - could be considered bug fix

But they're bundled with the main feature change, which makes the PR less focused.

Code quality

Let's look for any issues:

1. In the TUI watcher changes:

   • Added "sync/atomic" import
   • Used atomic.Uint64 for debounce sequence
   • This looks correct for preventing race conditions in the debounce logic

2. In the microflow validation:

   • Extracted duplicate validation into validateOutputVariable helper function
   • Applied this helper to all statement types that have output variables
   • This is good code reuse and consistency

3. In the describe output:

   • Added duplicateOutputVariableWarnings function
   • Added actionOutputVariableName helper to extract variable names from different action types
   • This looks well-structured

4. The Java action changes:

   • Added buildJavaActionReturnType and isVoidReturnType functions
   • This appears to be fixing proper handling of Void return types
   • Added test for this functionality

The code quality of the main feature changes looks good.

Bugs & correctness

Let's think about correctness:

The main change is replacing silent aliasing with explicit validation errors for duplicate output variables.

From the PR description:

If an authored MDL script declares the same output variable twice in the same scope, validation now reports a duplicate-variable error. If describe encounters an existing MPR that already contains duplicate output variables, it preserves the original output variable names and emits a warning comment instead of transforming the model to generated names such as $Item_2.

This seems correct:

• For new scripts: validation error prevents creation of invalid models
• For existing invalid models: warn but don't try to "fix" by aliasing (which could be wrong)

The implementation:

• In validation: checks if variable is already declared, adds error if so
• In describe: walks through all actions, counts output variables, warns for duplicates

This looks correct.

Now let me check if there are any issues:

1. The TUI watcher changes:

   • Uses atomic.Uint64 for sequence tracking
   • Compares current sequence with captured sequence in debounce function
   • This is a standard way to cancel stale debounce timers
   • Looks correct

2. Nanoflow examples simplification:

   • Removed many examples, keeping only a minimal set
   • Changed formatting to use more consistent style
   • Updated grants and commands to match simplified examples
   • This seems like cleanup, but it's a significant change to example files

3. Java action visitor changes:

   • Changed ReturnType assignment to use buildJavaActionReturnType
   • Added isVoidReturnType helper
   • Added test for explicit Void return type
   • This looks like it was fixing a bug where Void wasn't handled properly

Let me double-check if the Java action changes are actually related to the main feature. The main feature is about duplicate output variables in microflows/nanoflows. The Java action changes are about return type handling. These seem unrelated.

Similarly, the TUI watcher changes are unrelated to MDL microflow validation.

The nanoflow examples simplification is also not directly related to the duplicate output variable validation.

This suggests the PR contains multiple unrelated changes, which violates the scoping principle.

However, let me check if there's any connection I'm missing:

• Could the nanoflow examples have been demonstrating duplicate output variables? Looking at the before/after, the simplified examples don't show any duplicate variable declarations.
• The Java action changes don't seem related to output variables.
• The TUI watcher is purely UI infrastructure.

It appears these are unrelated changes bundled together.

Recommendation

The core functionality (rejecting duplicate microflow output variables) is well-implemented and tested. However, the PR contains several unrelated changes:

1. TUI watcher debouncing improvements (infrastructure)
2. Nanoflow examples simplification (documentation cleanup)
3. Java action visitor changes for Void return type handling (bug fix)
4. The main feature: duplicate output variable validation

According to the checklist:

[ ] Each PR is scoped to a single feature or concern — if the description needs "and" between unrelated items, split it

The PR description only mentions the duplicate output variable functionality, but includes other unrelated changes.

I would recommend:

1. Approving the core functionality (duplicate output variable validation)
2. Requesting that the unrelated changes be split into separate PRs

Specifically:

• The TUI watcher changes should be in a separate infrastructure PR
• The nanoflow examples simplification should be in a separate documentation PR
• The Java action visitor changes should be in a separate bug fix PR
• Only the duplicate output variable validation changes should be in this PR

Let me verify what exactly constitutes the core feature changes:

From the diff, the core feature appears to be in:

• mdl/executor/cmd_microflows_builder_validate.go (validation logic)
• mdl/executor/cmd_microflows_show.go (describe output warnings)
• mdl/executor/cmd_microflows_duplicate_output_test.go (new tests)
• Updates to roundtrip_doctype_test.go and roundtrip_nanoflow_test.go (to account for new validation)

The other changes seem unrelated.

Actually, looking more carefully at the roundtrip test updates:

• roundtrip_doctype_test.go: added CE0115, CE5601 to 03-page-examples.mdl and CE0115, CE0117, CE6035 to 02b-nanoflow-examples.mdl
• roundtrip_nanoflow_test.go: fixed loop syntax and enum parameter syntax

These updates to the roundtrip tests seem to be addressing issues that were uncovered by the nanoflow examples simplification or are unrelated to the main feature.

Given that the nanoflow examples file was significantly changed, it's likely that the roundtrip test updates are related to those changes, not the duplicate output variable validation.

Let me re-examine what exactly is needed for the duplicate output variable feature:

1. Validation changes in cmd_microflows_builder_validate.go - definitely core
2. Describe output changes in cmd_microflows_show.go - definitely core
3. New test file cmd_microflows_duplicate_output_test.go - definitely core
4. Updates to roundtrip tests to account for new validation behavior - arguably core
5. Everything else - not core

The roundtrip test updates are likely needed because the validation changes would cause previously passing tests to now fail with validation errors, so they need to be updated to reflect the new expected behavior (either fixing the test scripts or updating the known errors list).

Looking at the roundtrip_doctype_test.go changes:

• Added CE0115, CE5601 to 03-page-examples.mdl
• Added CE0115, CE0117, CE6035 to 02b-nanoflow-examples.mdl

And in roundtrip_nanoflow_test.go:

• Fixed loop syntax: changed loop $Item in $Items begin to proper format
• Fixed enum parameter: added Enum keyword

These seem to be fixing issues in the test scripts themselves, not just updating known errors. This suggests the test scripts had bugs that were uncovered when running the tests, possibly unrelated to the main feature.

Actually, looking at the nanoflow examples file that was simplified (02b-nanoflow-examples.mdl), it's likely that the roundtrip_nanoflow_test.go was using examples from that file, and when the examples were simplified, the test scripts needed to be updated.

This reinforces that the nanoflow examples simplification and related test updates are unrelated to the main feature.

Let me try to isolate just the core feature changes:

Core feature (duplicate output variable validation):

• mdl/executor/cmd_microflows_builder_validate.go: validation logic
• mdl/executor/cmd_microflows_show.go: describe output warnings
• mdl/executor/cmd_microflows_duplicate_output_test.go: new tests
• mdl/executor/roundtrip_doctype_test.go: updates to known CE errors (to account for validation now catching duplicates)
• mdl/executor/roundtrip_nanoflow_test.go: ? (need to check if related)

Looking at roundtrip_nanoflow_test.go changes:

• Fixed loop syntax in TestRoundtripNanoflow_Loop
• Fixed enum parameter syntax in TestRoundtripNanoflow_EnumParameter

These appear to be fixing bugs in the test scripts themselves, likely unrelated to the duplicate output variable feature.

So the core feature changes are:

• mdl/executor/cmd_microflows_builder_validate.go
• mdl/executor/cmd_microflows_show.go
• mdl/executor/cmd_microflows_duplicate_output_test.go
• mdl/executor/roundtrip_doctype_test.go (the known errors updates)

Everything else appears to be unrelated changes bundled into this PR.

Given the checklist item about scope and atomicity:

[ ] Each PR is scoped to a single feature or concern — if the description needs "and" between unrelated items, split it

And the fact that the PR description only mentions the duplicate output variable functionality, I should request that the unrelated changes be split out.

However, as a reviewer, I need to make a call on whether to approve or request changes.

The core functionality looks correct and well-tested. The unrelated changes, while not ideal to bundle, don't appear to be harmful (they seem to be improvements or fixes).

But following the contribution guidelines strictly, I should request changes to split the unrelated work into separate PRs.

Let me check if there's any guidance in CLAUDE.md about bundled changes:

From the "Scope & atomicity" section:

[ ] Each PR is scoped to a single feature or concern — if the description needs "and" between unrelated items, split it
[ ] Independent features (e.g., a new command, a formatter, UX improvements) go in separate PRs even if developed together
[ ] Refactors that touch many files (e.g., renaming a helper across executors) are their own commit, not bundled with feature work

This clearly states that unrelated items should be split into separate PRs.

The PR contains:

• Main feature: duplicate output variable validation
• Unrelated: TUI watcher debouncing
• Unrelated: Nanoflow examples simplification
• Unrelated: Java action visitor changes (Void return type)
• Possibly unrelated: some test updates

I should request changes to split these into separate PRs.

However, let me consider if any of these could be argued as related:

1. TUI watcher debouncing: Purely UI infrastructure, unrelated to MDL validation
2. Nanoflow examples simplification: Documentation cleanup, not related to validation logic
3. Java action visitor changes: Fixing return type handling, not related to output variable validation
4. Test updates: Some are likely related to the main feature (roundtrip_doctype_test.go known errors updates), others appear to be fixing test script bugs

Given the strict interpretation of the guidelines, I should request changes.

But let me think about the practical impact:

• The core feature is correctly implemented
• The unrelated changes seem to be improvements
• The PR is still reviewable

However, as a code reviewer following the project's contribution guidelines, I need to enforce the scoping principle.

Therefore, I will request changes to split the unrelated work into separate PRs.

Actually, let me double-check the nanoflow examples simplification. Is it possible that this was done because the examples were demonstrating bad patterns that the new validation would catch?

Looking at the before/after of 02b-nanoflow-examples.mdl:

Before: Had examples like N002_ValidateProduct, N003_CountProducts, etc.
After: Minimal set like NF_Empty, NF_ValidateProduct, NF_SaveProduct, NF_FormatPrice

The removed examples included things like:

• N002_ValidateProduct (validates a Product)
• N003_CountProducts (counts products in a list)
• N004_BuildProduct (creates and returns a Product)
• N005_ShowStatusMessage (shows message based on status)
• N006_SaveProduct (validates and saves via server microflow)
• N007_OpenProductDetail (opens page with parameter)
• N008_FormatPrice (formats price as currency)

---

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