fix: preserve annotations attached inside loop bodies

[hjotha]

Part of #332.

Summary

• copy loop/while body annotation flows into the parent microflow graph
• collect annotation captions recursively from nested loop object collections
• merge loop-local annotation flows into loop body traversal
• add regressions for annotated nested decisions and annotation flows stored inside loop object collections

Closes #330

Validation

• go test ./mdl/executor -run 'TestLoopBodyIfAnnotationPromotedToParentFlows|TestTraverseFlow_LoopBodyUsesNestedAnnotationFlows'
• make build
• make lint-go
• make test

[github-actions]

AI Code Review

What Looks Good

• The PR correctly identifies and fixes the issue where annotations inside loop bodies weren't being preserved in the parent microflow graph
• The test TestLoopBodyIfAnnotationPromotedToParentFlows properly validates the fix for an annotated nested decision inside a loop body
• The changes are minimal and focused: adding two lines to propagate annotationFlows in loop/while statement handling, and refactoring annotation collection to be recursive for nested loops
• The refactoring in cmd_microflows_show_helpers.go cleanly separates concerns with collectAnnotationCaptions and mergeAnnotationsByTarget functions
• No new MDL syntax is introduced, so syntax design checks don't apply
• The fix maintains backend abstraction compliance by working entirely within the executor layer
• Test coverage is added for the specific bug scenario
• The changes are scoped to a single concern (preserving loop body annotations)

Recommendation

Approve. The PR correctly fixes the annotation preservation issue in loop bodies with minimal, well-targeted changes and appropriate test coverage. All relevant checklist items are satisfied for this bug fix.

---

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

[hjotha]

Updated the branch to also cover annotation flows stored directly inside loop object collections. Validation after the update:

• go test ./mdl/executor -run 'TestLoopBodyIfAnnotationPromotedToParentFlows|TestTraverseFlow_LoopBodyUsesNestedAnnotationFlows'
• make build
• make lint-go
• make test

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

None found.

What Looks Good

• The PR adds a comprehensive bug test in mdl-examples/bug-tests/330-preserve-loop-body-annotations.mdl that reproduces the issue and validates the fix.
• The unit tests (TestLoopBodyIfAnnotationPromotedToParentFlows and TestTraverseFlow_LoopBodyUsesNestedAnnotationFlows) properly cover the AST→BSON build path and the describer side.
• The fix is minimal and focused:
  • In the builder (cmd_microflows_builder_control.go), annotation flows from loop bodies are now propagated to the parent graph for both LoopStmt and WhileStmt.
  • In the show helpers (cmd_microflows_show_helpers.go), annotation caption collection is made recursive to handle nested loop object collections, and loop-local annotations are merged into the body traversal.
• The changes follow the existing patterns in the codebase (e.g., similar propagation of flows, use of helper functions for annotation handling).
• No MDL syntax is modified, so full-stack consistency checks for new syntax are not applicable.
• The PR is scoped to a single bug fix (annotations in loop bodies) with no unrelated changes.
• Test coverage includes both unit tests and an MDL bug test, avoiding time.Sleep for synchronization.

Recommendation

approve - The PR correctly addresses the issue with minimal, well-tested changes that follow existing patterns. No blocking concerns identified.

---

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

[ako]

Review: fix: preserve annotations attached inside loop bodies

Summary: Clean two-part fix. The builder side is a symmetric two-liner matching the existing sequence-flow promotion pattern; the describer side correctly handles the split between annotation objects (stored in loop ObjectCollections) and annotation flows (now promoted to the parent level).

Builder fix (two lines)

fb.annotationFlows = append(fb.annotationFlows, loopBuilder.annotationFlows...)

Applied symmetrically in both addLoopStatement and addWhileStatement, matching the existing fb.flows = append(fb.flows, loopBuilder.flows...) promotion. The final MicroflowObjectCollection already packages fb.annotationFlows into AnnotationFlows, so no further wiring is needed. ✓

Describer fix

The object-vs-flow split is key: annotation Objects live inside the loop's ObjectCollection; annotation Flows are now promoted to the parent level by the builder. The describer fix handles both sides:

• collectAnnotationCaptions: Recursively descends into LoopedActivity.ObjectCollection to collect caption IDs from nested annotations. The continue after the Annotation case is redundant (a Go object can only be one type) but harmless.
• emitLoopBody: Merges parent annotationsByTarget with the loop-local annotation map. This handles the case where annotation flows are stored in the loop's own ObjectCollection (older MPR files that predate the builder fix).

Together these cover both the new MPR layout (flows promoted, captions found recursively) and existing MPR files (flows in loop ObjectCollection, found by the loop-local buildAnnotationsByTarget). ✓

mergeAnnotationsByTarget — returns overlay by reference when base is empty

if len(base) == 0 {
    return overlay
}

This returns the same map as overlay, not a copy. In emitLoopBody the overlay is a freshly built map from buildAnnotationsByTarget, so mutation of the result wouldn't affect anything else. Safe for the current caller. Worth a comment if this function gets additional callers in future.

Test coverage

TestLoopBodyIfAnnotationPromotedToParentFlows: Verifies the builder path — an annotation on a nested IF inside a LOOP ends up in the parent object collection's annotation map. Correctly traces through the promoted flow + recursive caption collection. ✓

TestTraverseFlow_LoopBodyUsesNestedAnnotationFlows: Pre-builds annotationsByTarget from the loop's ObjectCollection and passes it directly to traverseFlow, verifying that @annotation appears in the rendered output. This exercises annotation rendering but not the emitLoopBody merge path itself (the merge logic is tested indirectly by the MDL bug-test script rather than a Go unit test).

A Go-level test that constructs a full LoopedActivity with nested annotations and exercises emitLoopBody end-to-end would close this gap, but the existing coverage is sufficient for the described bug.

Bug test script

Documents the symptom (annotations on IF inside LOOP disappear after roundtrip) and the dependency on PR #327 for full fixpoint roundtrip. The scope note is helpful context. ✓

Verdict

Approve. The fix is correct, minimal, and symmetric between the builder and describer sides.

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

None found.

What Looks Good

• Bug test coverage: The PR adds a dedicated MDL bug test script (mdl-examples/bug-tests/330-preserve-loop-body-annotations.mdl) that reproduces the issue and validates the fix via describe/exec roundtrip, satisfying the checklist requirement for bug fixes to include MDL test scripts in bug-tests/.
• Unit test coverage: Two new Go tests are added:
  • TestLoopBodyIfAnnotationPromotedToParentFlows verifies the builder correctly promotes nested annotation flows from loop bodies to the parent graph.
  • TestTraverseFlow_LoopBodyUsesNestedAnnotationFlows ensures the describer handles annotation flows stored inside loop object collections during traversal.
• Focused changes: The fix is surgical and addresses the root cause precisely:
  • Builder: Adds fb.annotationFlows = append(fb.annotationFlows, loopBuilder.annotationFlows...) in both addLoopStatement and addWhileStatement to copy annotation flows from loop bodies to parent graph.
  • Describer: Implements recursive annotation caption collection via collectAnnotationCaptions() that walks into nested loop object collections, and uses mergeAnnotationsByTarget() to merge loop-local annotations into per-body traversal.
• No syntax changes: Since this is a behavioral fix (not new MDL syntax), the PR correctly avoids modifying grammar/AST/LSP, focusing solely on executor/describer logic.
• Deterministic helper functions: New helpers collectAnnotationCaptions and mergeAnnotationsByTarget are pure, side-effect free, and handle nil safely.
• Validation: The PR runs the specified regression tests (TestLoopBodyIfAnnotationPromotedToParentFlows|TestTraverseFlow_LoopBodyUsesNestedAnnotationFlows), make build, make lint-go, and make test per the validation section.
• Atomic scope: All changes relate exclusively to preserving annotations in loop bodies (no unrelated modifications).

Recommendation

Approve. The PR fully addresses bug #330 with appropriate test coverage, follows the project's architecture (backend abstraction compliance is maintained as changes are confined to executor), and meets all checklist criteria for bug fixes. No changes are needed.

---

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

[hjotha]

Review follow-up status on the current head:

• Documented the mergeAnnotationsByTarget aliasing contract for the empty-base fast path.
• Kept the existing builder/describer coverage and bug-test fixture unchanged because the review found the behavior correct.

Local validation after the update was green:

make build
make test
make lint-go