fix: preserve rule split describer output and free annotations

[hjotha]

Summary

Describer-side counterpart to the rule-split subtype fix, plus two related describer bugs that surfaced from the same end-to-end exercise:

• Rule split describer output — with the parser now preserving RuleSplitCondition, the describer also needs to emit the qualified rule call (Mod.Rule(param=$value)) in the if ... header rather than treating every split condition as an expression. Introduces formatSplitCondition to dispatch on the condition subtype.
• @caption for splits — ExclusiveSplit / InheritanceSplit carry a caption that the describer was dropping. Now emitted as @caption '...' before the if / inherit keyword.
• Free-floating annotations — Annotation objects with no AnnotationFlow (the "sticky note" style) were ignored by the describer. Now collected and emitted as @annotation '...' at the top of the microflow body.
• needMerge guard in flow builder — when both branches of an IF terminate in return, the builder must not emit a dangling SequenceFlow pointing at an empty DestinationPointer. Added guard rails in three emit sites.
• writer_core warning comment — added a tight warning on idToBsonBinary explaining that an empty id here is almost always a bug (produces a random UUID and surfaces as KeyNotFoundException in Studio Pro); the fix belongs in the caller.

Part of umbrella #257.

Test plan

• Regression tests cover rule-split describer output, split captions, free annotations, and needMerge guard.
• go build ./... && go test ./... pass.
• Studio Pro opens the round-tripped project without KeyNotFoundException or CE0117.

Stack

This PR is the top of a 3-PR stack. It depends on #265 (rule subtype preservation), which in turn depends on #263 (nested caption preservation). Once the two base PRs merge, this one can be rebased directly onto main.

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• The PR touches multiple related but distinct areas (rule splits, split captions, free annotations, needMerge guard, writer warning). While they're all part of the same end-to-end exercise, splitting these into separate PRs might improve reviewability. However, given their close interrelation in microflow describer/executor logic, keeping them together is acceptable.

What Looks Good

• Comprehensive test coverage: New test files cover rule split conditions, split captions/annotations, and BSON roundtrips for rule splits.
• Correct bug fixes:
  • Rule split describer now emits proper Mod.Rule(param=$value) instead of treating as expression (prevents CE0117)
  • Split captions are now properly emitted via @caption
  • Free-floating annotations are collected and emitted as @annotation '...'
  • needMerge guard prevents dangling SequenceFlow when both IF branches return
  • Helpful warning comment in writer_core about empty ID risks
• Proper state management: Fixed annotation handling in flow builders by snapshotting pendingAnnotations before recursion to prevent outer/inner statement annotation crosstalk.
• Clean implementation:
  • buildSplitCondition cleanly dispatches on condition subtype
  • Backend interface properly extended with IsRule method
  • Mock and MPR implementations follow established patterns
  • Describer improvements use existing escaping utilities

Recommendation

Approve. The PR correctly fixes describer/executor issues for microflow controls with appropriate test coverage and minimal risk. The changes are focused, follow project conventions, and improve roundtrip fidelity for MDL microflows. No blocking issues identified.

---

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

[github-actions]

AI Code Review

Review Summary

This PR fixes describer-side issues related to rule split output, split captions, free annotations, and flow builder guards. It's the top of a 3-PR stack that depends on prior parser fixes (#265 for rule subtype preservation, #263 for nested caption preservation).

What Looks Good

• Comprehensive bug fixes: Addresses all issues mentioned in the summary:
  • Rule split describer now properly emits qualified rule calls (Mod.Rule(param=$value)) instead of treating them as expressions
  • Split captions (@caption) are now correctly preserved and emitted
  • Free-floating annotations (sticky notes) are now collected and emitted at the top of microflow bodies
  • needMerge guards prevent dangling SequenceFlows when both IF branches terminate in return
  • Helpful warning comment added to idToBsonBinary about empty ID risks
• Thorough test coverage:
  • Added bug test MDL file (263-nested-caption-preservation.mdl)
  • Added unit tests for rule split handling (cmd_microflows_builder_rule_split_test.go)
  • Added unit tests for annotation handling (cmd_microflows_builder_annotations_test.go)
  • Added BSON roundtrip test for RuleSplitCondition (writer_rule_split_test.go)
• Proper pipeline wiring: Correctly addresses the describer/output side of the pipeline for these fixes, building on the parser fixes from dependent PRs
• Clean implementation:
  • Uses snapshot/restore pattern for pendingAnnotations to prevent overwriting in nested statements
  • Properly handles all split types (ExclusiveSplit, InheritanceSplit) and loop types (LoopStmt, WhileStmt)
  • Follows existing code patterns and conventions
  • Appropriate error handling in backend methods

Minor Issues

• The PR touches many files but is justified as related bug fixes surfaced from the same end-to-end exercise. While it addresses multiple issues, they are all closely connected to describer output fixes.
• No significant code quality or maintainability concerns observed.

Recommendation

Approve - The PR correctly fixes the reported issues with comprehensive test coverage, proper pipeline wiring, and clean implementation that follows project conventions. The scope is appropriate for the interconnected nature of the bugs being fixed.

---

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

[ako]

PR #266 Review — fix: preserve rule split describer output and free annotations

Overview

Describer-side counterpart to PRs #263 and #265. Adds formatSplitCondition to dispatch rule vs expression split conditions, emits @caption for ExclusiveSplit/InheritanceSplit, collects and emits free-floating annotations, and guards three SequenceFlow emit sites against dangling pointers when both branches of an IF terminate.

What's Good

• formatSplitCondition is clean: the LastIndex strip for parameter names is correct, the "true" fallback is safe and produces valid MDL
• needMerge guard (!thenReturns && needMerge) at all three emit sites is correct — without it an empty mergeID produces a KeyNotFoundException in Studio Pro
• collectFreeAnnotations correctly identifies unreferenced Annotation objects and the @annotation prefix approach is simple and doesn't affect activity ordering
• idToBsonBinary warning comment is well-placed and actionable

Issues

Loop caption describer gap (non-blocker, but inconsistent with builder)
applyAnnotations in this PR gains a *microflows.LoopedActivity case so the builder can set loop captions. However emitObjectAnnotations in cmd_microflows_show_helpers.go only handles ActionActivity, ExclusiveSplit, and InheritanceSplit — it does not emit @caption for LoopedActivity. The new tests (TestLoopCaptionPreserved, TestWhileLoopCaptionPreserved) verify the builder side but not the describe → exec roundtrip. A caption applied via @caption 'foo' on a LOOP will be silently dropped on the next DESCRIBE.

Suggested addition to emitObjectAnnotations:

if loop, ok := obj.(*microflows.LoopedActivity); ok && loop.Caption != "" {
    escapedCaption := strings.ReplaceAll(loop.Caption, "'", "''")
    *lines = append(*lines, indentStr+fmt.Sprintf("@caption '%s'", escapedCaption))
}

Duplicated free-annotation prefix block
The 5-line free annotation prefix block appears verbatim in both describeMicroflow and renderMicroflowMDL in cmd_microflows_show.go. These two callers share identical logic — worth extracting into a one-liner helper:

func prependFreeAnnotations(lines []string, oc *microflows.MicroflowObjectCollection) []string {
    for _, text := range collectFreeAnnotations(oc) {
        lines = append([]string{fmt.Sprintf("@annotation '%s'", strings.ReplaceAll(text, "'", "''"))}, lines...)
    }
    return lines
}

(Or just keep it but it's the kind of duplication that drifts.)

Minor Notes

• Free annotations are always prepended at the start of the body regardless of canvas position. Since annotations have no execution semantics this is acceptable, but describe output will reorder them relative to their spatial neighbours on re-exec.
• ParameterName / ParameterID dual extraction for old BSON docs (binary UUID vs string) remains a known pre-existing limitation — not a regression from this PR.

Verdict

LGTM pending the loop caption describer gap. The needMerge guard and formatSplitCondition are the correctness-critical pieces and both look right. The loop caption issue is a consistency gap (builder and describer disagree on what @caption on a LOOP means) but doesn't break existing functionality.