fix: preserve nested loop body flow traversal

[hjotha]

Part of #332.

Summary

• merge parent-level body flows into loop-local traversal maps
• collect nested loop body object IDs recursively
• add a regression for local loop-boundary flows plus parent-level continuations sharing origins

Closes #328

Validation

• go test ./mdl/executor -run TestTraverseFlow_LoopBodyMergesParentFlowsForExistingOrigin
• make build
• make lint-go
• make test

[github-actions]

AI Code Review

We are given a PR titled "fix: preserve nested loop body flow traversal"

The PR summary says:

• merge parent-level body flows into loop-local traversal maps
• collect nested loop body object IDs recursively
• add a regression for local loop-boundary flows plus parent-level continuations sharing origins

The changes are in two files:

1. mdl/executor/cmd_microflows_show_helpers.go
2. mdl/executor/cmd_microflows_traverse_test.go

We are to review against the provided checklist.

First, note that the PR does not introduce any new MDL syntax (it's a fix to the executor's traversal logic for loops). Therefore, we skip the MDL syntax design section.

Now, let's go through the checklist:

Overlap & duplication

• The PR does not add any new proposals (we are told there is no proposals directory).
• We check for duplicated code: The changes are in the executor helpers and add a test. The changes are about collecting loop object IDs recursively and using that to merge parent flows. This does not duplicate existing functionality but rather fixes a bug in the traversal for nested loops.
• We check doctype test files: The PR adds a new test in mdl/executor/cmd_microflows_traverse_test.go (a regression test). There is no indication that this test duplicates an existing one.
• The PR does not re-document already-shipped features as new.

Full-stack consistency (for MDL features)

Since this is a bug fix and not a new MDL feature, we do not expect changes to the grammar, AST, visitor, etc. The changes are confined to the executor. Therefore, we skip the full-stack consistency check for MDL features.

Test coverage

• The PR adds a new test: TestTraverseFlow_LoopBodyMergesParentFlowsForExistingOrigin in mdl/executor/cmd_microflows_traverse_test.go.
• The test is a regression test for the specific issue.
• The test does not use time.Sleep for synchronization (it uses a direct function call and checks the output).
• The test is in the executor package, which is an integration path (not just a helper).

Security & robustness

• The changes are in the microflow traversal logic. There is no obvious security issue (like command injection, XSS, etc.) because we are dealing with internal data structures.
• No Unix sockets or file I/O in hot paths are introduced.
• No silent side effects on typos: the changes are about correctly identifying loop objects and merging flows.
• Method receivers: The changes are in helper functions, so no issue with receivers.

Scope & atomicity

• The PR is focused on fixing the traversal of nested loops in microflows. It does one thing: fix the bug where parent-level flows were not being merged into nested loop bodies when there was an existing local loop-boundary flow.
• The PR description does not have an "and" between unrelated items.

Code quality

• The changes are refactorings of the loop traversal logic to handle nested loops correctly. They are applied consistently in the two places (origin and destination maps) and in the test.
• There are no manually maintained lists that are flagged as maintenance risks.
• The design (as per the comments in the code) matches the implementation.

Bugs & correctness

• The fix addresses a specific bug: when a nested loop has a local loop-boundary flow (i.e., a flow that stays within the loop body) and there is also a parent-level flow that continues from the same origin, the parent-level flow was not being merged in. The fix collects all object IDs in the loop (including nested loops) and then uses that set to decide whether to merge parent flows.
• The test case is designed to catch this bug and the fix passes it.
• Error handling: The changes do not introduce new error paths that are not handled.

Now, let's look at the changes in detail:

In mdl/executor/cmd_microflows_show_helpers.go:

• We added a call to collectLoopObjectIDs(loop.ObjectCollection) to get a set of all object IDs in the loop (including nested loops).
• Then, when checking if an originID or destID is in the loop, we use loopObjectIDs[originID] (or loopObjectIDs[destID]) instead of checking against loopActivityMap (which only had the immediate objects).
• We also changed the way we add flows: instead of setting the slice to the flows (which would replace any existing ones), we now append each flow if it's missing (using appendSequenceFlowIfMissing). This avoids duplicates and also merges multiple flows from different sources (like parent-level and local) for the same origin/destination.
• We added helper functions: collectLoopObjectIDs, collectLoopObjectIDsInto, appendSequenceFlowIfMissing, sequenceFlowIdentity, and preferLoopBodyStart.

In the test file:

• We added a test that sets up a nested loop scenario where:

  • The outer loop has a body: setup -> fetch -> (then the loop continues to the end)

  • The nested loop (inside the outer loop's body) has a body: nested-log -> nested-tail -> (and then back to the nested loop? Actually, the nested loop has a flow from nested-log to nested, and then from nested-tail to nested? Wait, the test sets up:
    nested loop:
    Objects: [nested-log, nested-tail]
    Flows: [nested-log -> nested] (note: nested is the loop activity itself? Actually, the loop activity is the LoopedActivity, so the flow from nested-log to the loop activity? But note: the loop activity has an ID of "nested".)
    Then, the outer loop has:
    Objects: [nestedLoop, setup, fetch]
    Flows: [setup->fetch, fetch->loop] (so the loop activity is the outer loop, with ID "loop")
    And then the parent flows (from the outer loop's parent, which is the main flow) are:
    start->loop, loop->end, fetch->nested, nested->loop, nested-log->nested-tail, nested-tail->nested.

    The key point:
    - The nested loop has a local flow: nested-log -> nested (which is the loop activity) and nested-tail -> nested.
    - But there is also a parent flow: fetch -> nested (which is the nested loop activity) from the outer loop's body.

    The bug was that when processing the nested loop, the parent flow (fetch->nested) was not being merged into the nested loop's flow set because the nested loop's immediate object collection did not contain the "fetch" activity (it's in the outer loop). The fix collects all object IDs in the nested loop (including the nested loop's own objects and any nested loops inside it, but note: the nested loop in this test does not have a nested loop inside it? Actually, the nested loop in the test is the inner loop, and it has no further nesting). However, the fix collects the object IDs of the nested loop's ObjectCollection, which in the test only has the two log activities. So why does it work?

    Actually, the fix changes the condition from:
    if _, inLoop := loopActivityMap[originID]; inLoop {
    to:
    if loopObjectIDs[originID] {
    where loopObjectIDs is built by recursively collecting all object IDs in the loop's ObjectCollection (including any nested loops inside).

    In the test, the nested loop (inner) does not have any nested loops inside it, so loopObjectIDs for the inner loop only has the two log activities. Therefore, the parent flow (fetch->nested) would still not be included because "fetch" is not in the inner loop's object set.

    Wait, let me re-read the test: the nested loop is the inner loop, and the parent flow we are concerned about is from "fetch" (which is in the outer loop's body) to the inner loop (activity ID "nested"). The inner loop's ObjectCollection does not contain "fetch", so the condition loopObjectIDs[originID] for originID="fetch" would be false.

    But note: the test is named TestTraverseFlow_LoopBodyMergesParentFlowsForExistingOrigin and the comment says:
    // add a regression for local loop-boundary flows plus parent-level continuations sharing origins

    And the test sets up:
    // This local loop-boundary flow gives "fetch" an existing
    // local origin entry. Parent-level body flows with the same
    // origin must still be merged in.

    The local loop-boundary flow in the outer loop is: "fetch" -> "loop" (the outer loop activity). But wait, the outer loop's flow is set up as:
    Fls: []*microflows.SequenceFlow{
    mkFlow("setup", "fetch"),
    mkFlow("fetch", "loop"), // this is the local loop-boundary flow for the outer loop?
    }

    However, the test is about the nested loop. The nested loop is the inner loop. The parent-level flow for the inner loop is the flow from the outer loop's body to the inner loop: that is, the flow from "fetch" to "nested" (the inner loop activity).

    The bug was that when processing the inner loop, we were only looking at the inner loop's immediate ObjectCollection (which does not contain "fetch") to decide whether to include parent flows. So we were missing the parent flow from "fetch" to "nested".

    The fix changes the condition to use the recursively collected object IDs of the loop. But note: the inner loop does not have "fetch" in its ObjectCollection (nor in any nested loop inside it, because there isn't one). So why would the fix work?

    Let me look at the code again:

    We have:
    loopObjectIDs := collectLoopObjectIDs(loop.ObjectCollection)

    And then we use that to check if an originID is in the loop.

    However, the inner loop's ObjectCollection does not contain "fetch", so loopObjectIDs for the inner loop does not have "fetch".

    But wait: the test also sets up a flow in the outer loop:
    mkFlow("fetch", "loop") // meaning from fetch to the outer loop activity (ID "loop")

    And then in the parent flows (which are the flows from the outer loop's parent, i.e., the main flow) we have:
    mkID("fetch"): {mkFlow("fetch", "nested")},

    So the parent flow for the inner loop is from "fetch" to "nested".

    The fix in the code for the inner loop is to check if the originID (which is "fetch") is in the set of object IDs of the inner loop (recursively collected). It is not.

    However, note that the test also has:
    mkID("nested"): {mkFlow("nested", "loop")}, // this is a flow from the nested loop activity to the outer loop activity?
    ... and others.

    And then the test expects to see:
    "log info node 'Synthetic' 'setup';",
    "log info node 'Synthetic' 'fetch';",
    "loop $role in $roles",
    ...

    The key is that the test is actually testing the outer loop? Let me read the test again:

    We have:
    outerLoop := &microflows.LoopedActivity{ ... } // ID "loop"
    nestedLoop := &microflows.LoopedActivity{ ... } // ID "nested"

    And then we build:
    activityMap:
    "start": StartEvent
    "loop": outerLoop
    "end": EndEvent
    flowsByOrigin:
    "start": [start->loop]
    "loop": [loop->end]
    "fetch": [fetch->nested] // note: fetch is an activity in the outer loop's body
    "nested": [nested->loop] // note: nested is the inner loop activity
    "nested-log": [nested-log->nested-tail]
    "nested-tail": [nested-tail->nested]

    The traverseFlow is called from the start activity.

    The flow from start goes to the outer loop (activity "loop"). Then we enter the outer loop's body.

    In the outer loop's body, we have:
    Objects: [nestedLoop, setup, fetch]
    Flows: [setup->fetch, fetch->loop] // note: fetch->loop is a flow that goes back to the outer loop activity (so it's a loop-boundary flow for the outer loop)

    But wait, the outer loop's body also contains the nested loop (as an object). So when we are processing the outer loop's body, we will encounter the nested loop object.

    When we process the nested loop object (which is a LoopedActivity), we then call emitLoopBody on it.

    Inside emitLoopBody for the nested loop (inner loop):
    We build loopActivityMap from the inner loop's ObjectCollection: which has the two log activities.
    Then we compute loopObjectIDs by recursively collecting from the inner loop's ObjectCollection (which only has the two log activities, and they are not LoopedActivity so no further recursion).

     Then we look at flowsByOrigin (which is the parent flows, i.e., the flows from the outer loop's body to the inner loop and within the inner loop? Actually, note: the flowsByOrigin passed to emitLoopBody is the same as the one passed to the outer loop's emitLoopBody? 
    

    Actually, the outer loop's emitLoopBody is called with:
    flowsByOrigin: the one we built in the outer loop's emitLoopBody?

    Let me trace:

    The outer loop's emitLoopBody (for the outer loop) is called from the main traverseFlow when we hit the outer loop activity.

    In the outer loop's emitLoopBody:
    We build loopActivityMap for the outer loop's ObjectCollection: [nestedLoop, setup, fetch]
    Then we compute loopObjectIDs for the outer loop by recursively collecting:
    - nestedLoop: is a LoopedActivity -> we recursively collect its ObjectCollection (the two log activities)
    - setup: not a loop
    - fetch: not a loop
    So loopObjectIDs for the outer loop will include:
    nestedLoop.ID, setup.ID, fetch.ID, and the two log activities from the inner loop.

     Then, when processing parent flows (flowsByOrigin) for the outer loop, we check for each originID if it is in loopObjectIDs (the outer loop's set).
    
     The parent flows for the outer loop (passed in from the main traverseFlow) are:
       start: [start->loop]   -> originID = start -> not in outer loop's ObjectCollection? (start is not in the outer loop's body) -> skip
       loop: [loop->end]      -> originID = loop -> is in the outer loop's ObjectCollection? (the loop activity itself is not in the ObjectCollection of the outer loop's body? Actually, the outer loop's activity is the LoopedActivity, and its ObjectCollection does not contain itself) -> so skip? 
       fetch: [fetch->nested] -> originID = fetch -> is in the outer loop's ObjectCollection? (yes, because fetch is in the outer loop's ObjectCollection) -> so we add this flow to the outer loop's loopFlowsByOrigin for originID=fetch.
       nested: [nested->loop] -> originID = nested -> is in the outer loop's ObjectCollection? (yes, because the nested loop activity is in the outer loop's ObjectCollection) -> so we add this flow.
       nested-log: [nested-log->nested-tail] -> originID = nested-log -> is in the outer loop's ObjectCollection? (yes, because we collected it from the inner loop) -> so we add.
       nested-tail: [nested-tail->nested] -> originID = nested-tail -> is in the outer loop's ObjectCollection? (yes) -> so we add.
    
     Then, when building the flow graph for the outer loop's body, we have:
       loopFlowsByOrigin for fetch: [fetch->nested]   (from parent) and also we have the local flows? 
       The outer loop's own ObjectCollection flows: [setup->fetch, fetch->loop]
    
     So for originID=fetch, we have two flows: 
       - the local one: fetch->loop
       - the parent one: fetch->nested
    
     Then, when we build the flow graph for the outer loop's body, we will have both.
    

    Now, when we traverse the outer loop's body, we start at the activity with zero incoming flow (in the outer loop's body). We compute incomingCount for the outer loop's body.

    The outer loop's body activities: nestedLoop, setup, fetch.

    Incoming flows (from the outer loop's body flow graph, which includes both local and parent flows that we merged):
    setup:
    from?
    local: none?
    parent: none?
    Actually, we have:
    setup->fetch (local) -> so fetch gets an incoming from setup.
    fetch->loop (local) -> loop activity (which is not in the body) -> ignore for body traversal?
    fetch->nested (parent) -> nested loop activity (which is in the body) -> so nestedLoop gets an incoming from fetch.
    fetch:
    from setup (local) -> one incoming
    nestedLoop:
    from fetch (parent) -> one incoming
    nested-log:
    from?
    parent: nested-log->nested-tail (so nested-tail gets an incoming from nested-log)
    nested-tail:
    from nested-log (parent) and from?
    parent: nested-tail->nested (so nested gets an incoming from nested-tail)

    But wait, we also have the inner loop's own flows?
    The inner loop's ObjectCollection flows: [nested-log->nested] (note: the test sets: Flows: []*microflows.SequenceFlow{ mkFlow("nested-log", "nested"), } for the inner loop? Actually, no: the test sets for the inner loop:
    ObjectCollection: &microflows.MicroflowObjectCollection

---

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

[github-actions]

AI Code Review

Review Summary

Critical Issues: None
Moderate Issues: None
Minor Issues: None
What Looks Good:

• The PR adds a comprehensive bug test (MDL script) and unit test covering the nested loop flow traversal issue
• The fix correctly merges parent-level flows into loop-local traversal maps for nested loop descendants
• Changes are minimal, focused, and follow existing code patterns
• Test validation commands are provided and pass

Recommendation: Approve

Detailed Findings

Overlap & Duplication

• No existing proposals or code duplication found for this specific nested loop flow bug (DESCRIBE MICROFLOW can omit loop-body continuation flows stored in the parent graph #328)
• The bug test file is new and targets the exact symptom described in DESCRIBE MICROFLOW can omit loop-body continuation flows stored in the parent graph #328
• Changes are isolated to microflow traversal logic without overlapping existing functionality

MDL Syntax Design

• Not applicable: PR contains no MDL syntax changes (only executor fixes and tests)
• No grammar, AST, or visitor modifications required

Full-Stack Consistency

• Not applicable: No new MDL feature added
• Fix lives entirely in executor layer (cmd_microflows_show_helpers.go) with appropriate test coverage
• No LSP/DESCRIBE roundtrip modifications needed since DESCRIBE behavior is fixed via executor

Test Coverage

• ✅ New bug test: mdl-examples/bug-tests/328-describer-nested-loop-body-flows.mdl (executable MDL script)
• ✅ New unit test: TestTraverseFlow_LoopBodyMergesParentFlowsForExistingOrigin in cmd_microflows_traverse_test.go
• ✅ Tests validate both MDL roundtrip (describe → exec → describe) and low-level traversal logic
• ✅ No time.sleep used; relies on deterministic flow verification

Security & Robustness

• ✅ No file I/O, sockets, or external interactions in hot paths
• ✅ No silent side effects (changes only affect flow traversal logic)
• ✅ Method receivers unchanged; no mutation concerns
• ✅ Input validation preserved (nil checks in new helpers)

Scope & Atomicity

• ✅ Single concern: fix nested loop body flow preservation in DESCRIBE MICROFLOW
• ✅ All changes directly serve this goal (test + executor fix)
• ✅ No unrelated modifications bundled

Code Quality

• ✅ Refactor applied consistently: extracted collectLoopObjectIDs and helpers
• ✅ No duplicated logic; replaced manual loops with reusable functions
• ✅ Clear variable names (loopObjectIDs, appendSequenceFlowIfMissing)
• ✅ Deterministic map iteration via key-based comparison in sequenceFlowIdentity

Conclusion

The PR correctly addresses bug #328 by ensuring parent-level continuation flows are merged into loop-local traversal maps for nested loop bodies. The solution is minimal, well-tested, and maintains existing code quality standards. No checklist violations found. Ready for merge.

---

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