fix: preserve duplicate implicit output aliases

[hjotha]

Summary

Preserves duplicate implicit output variables by assigning deterministic local aliases such as Foo_2 and resolving downstream references to the active alias.

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 #332.

Changes

• Tracks implicit output positions in the microflow builder.
• Assigns numeric aliases when a duplicate implicit output appears at the same canvas point.
• Resolves downstream variable references, attribute paths, change targets, and retrieve start variables through the active alias.
• Avoids declaring duplicate call-microflow result variables when a repeated result name is not referenced by the flow.
• Documents the aliasing behavior and adds a doctype fixture.

Validation

• make build
• make lint-go
• make test
• go test ./mdl/executor -run 'TestBuildFlowGraph_DuplicateUnusedCallOutputsDoNotDeclareReturnVariable|TestBuildFlowGraph_DuplicateImplicitOutputAtSamePositionGetsLocalAlias|TestBuildFlowGraph_PreservesDuplicateImplicitOutputAlias'
• ./bin/mxcli check mdl-examples/doctype-tests/variable_alias_collision.test.mdl

[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