fix: preserve rule-based decision subtype across microflow roundtrips

[hjotha]

Summary

A decision (ExclusiveSplit) whose condition was a rule call was being silently demoted to an expression split on round-trip. The BSON stores a Microflows$RuleSplitCondition (not ExpressionSplitCondition), and emitting the wrong subtype causes Studio Pro to raise CE0117 "Error(s) in expression".

Fix adds a RuleQualifiedName + ParameterName field to the split condition struct and wires the parser / writer to round-trip the subtype faithfully. The describer side of this fix is in PR #266 (stacked).

Part of umbrella #257.

Test plan

• Regression test exercises if Mod.Rule(param=$value) then ... round-trip; the resulting BSON contains Microflows$RuleSplitCondition, not the expression variant.
• go build ./... && go test ./... pass.

Stack

This PR is the second of a 3-PR stack. It depends on #263 (nested caption preservation) because both modify the same flow-builder structs. The describer-side counterpart is in #266, which depends on this PR.

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• Unused RuleID field: The RuleSplitCondition struct retains a RuleID field that is neither read nor written in the updated BSON logic (parser/writer now uses RuleQualifiedName under RuleCall). While harmless, it creates confusion about the struct's purpose. Consider removing it in a follow-up refactor.
• Incomplete backend method documentation: The new IsRule method in MicroflowBackend lacks detail about its performance characteristics (e.g., whether it caches results). This is minor since the implementation in MprBackend does not cache, but callers should be aware.

What Looks Good

• Correct bug fix: The core issue (rule-based splits mis-emitted as expression splits) is resolved by properly wiring RuleSplitCondition through the builder → SDK → BSON path.
• Robust test coverage: New tests validate:
  • Rule calls emit RuleSplitCondition with correct qualified name and parameter mappings
  • Non-rule calls fall back to ExpressionSplitCondition
  • Backend absence safely defaults to expression splits
  • Annotation fixes for nested IFs (preventing annotation bleed)
• Clean separation of concerns:
  • Backend abstraction maintained (IsRule added to interface, implemented in MPR/mock)
  • Executor logic stays BSON-free (delegates to ctx.Backend.IsRule)
  • SDK changes isolated to microflow types and BSON serialization
• Annotation fix: The nested IF annotation bug (where inner IF annotations overwrote outer split) is fixed by snappting/clearing pendingAnnotations before recursion – a well-justified improvement.
• Pattern consistency:
  • Uses standard Go idioms (pointer receivers for mutations, error handling)
  • Follows existing BSON serialization patterns (see writer_microflow.go)
  • Test names clearly express intent

Recommendation

Approve. The PR correctly fixes the reported bug with comprehensive tests, maintains architectural boundaries, and includes a valuable secondary fix for annotation handling. The minor issues (unused field, lightweight documentation) do not block merging and can be addressed in follow-up work. This PR is ready for integration.

---

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

[hjotha]

Re: the AI reviewer's minor note about the unused RuleID field — addressed in 1589b68. The field was only populated by the parser and never read anywhere downstream; the rule call is identified by RuleQualifiedName in the writer/describer path, so RuleID was pure dead weight. Removed from both the struct and the parser assignment.

The second note (performance/caching characteristics of IsRule) is a judgement call — IsRule does a linear scan over the microflow list in MprBackend, which matches other lookup methods in the same interface (GetMicroflowByQualifiedName, etc.). Adding caching just for rule-split validation would introduce inconsistency; if callers show up in hot paths later, the whole name-resolution layer can be memoised at once.

[github-actions]

AI Code Review

What Looks Good

• The PR correctly identifies and fixes the core issue: rule-based split conditions were being serialized as expression splits, causing Studio Pro validation errors (CE0117)
• Changes are well-scoped and focused on the specific problem without unnecessary modifications
• Comprehensive test coverage added including:
  • Unit tests for rule split condition handling (cmd_microflows_builder_rule_split_test.go)
  • Tests for annotation handling fixes (cmd_microflows_builder_annotations_test.go)
  • BSON serialization roundtrip tests (writer_rule_split_test.go)
• The fix properly wires through the full stack:
  • Backend interface (IsRule method)
  • Mock and MPR implementations
  • Executor logic for detecting rule calls and building proper split conditions
  • SDK changes to store qualified names instead of IDs in BSON
  • Updated parser/writer logic for the new structure
• The annotation handling improvements in cmd_microflows_builder_control.go fix a separate but related issue where nested IF statements would overwrite each other's captions
• Code follows existing patterns and conventions in the codebase
• No silent failures or error handling gaps apparent

Minor Issues

• In mdl/executor/cmd_microflows_builder.go, the tryBuildRuleSplitCondition function constructs ParameterName as call.Name + "." + name (e.g., "Module.IsEligible.Customer"). While this matches the test expectations, it would be good to verify this is indeed how Mendix stores parameter names in BSON for rule calls (though the test validates this approach).
• The PR combines two related but distinct fixes:
  1. Rule-based split condition preservation (main issue)
  2. Annotation handling for nested IF statements (prevents caption overwrite)
     While both are valuable and related to IF statement handling, ideally these might be split into separate PRs for clearer atomicity. However, since they touch the same code area and were developed together, this is acceptable.

Recommendation

Approve - The PR correctly fixes the reported issue with adequate test coverage, follows the project's architectural patterns, and maintains code quality standards. The minor concern about parameter name construction is validated by the included tests. The annotation fix, while slightly broadening the scope, addresses a related correctness issue in the same code area.

---

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

[github-actions]

AI Code Review

approve

Review Summary

This PR fixes a critical bug where rule-based IF conditions were being incorrectly serialized as ExpressionSplitCondition instead of RuleSplitCondition, causing Studio Pro validation errors (CE0117). The fix properly distinguishes between rule calls and expressions in IF conditions and ensures correct BSON serialization/deserialization.

Key Changes:

1. Backend Enhancement: Added IsRule() method to MicroflowBackend interface to determine if a qualified name refers to a rule
2. Flow Builder Logic: Implemented buildSplitCondition() and tryBuildRuleSplitCondition() to correctly choose between RuleSplitCondition and ExpressionSplitCondition
3. BSON Handling: Updated SDK microflow types and MPR parser/writer to properly handle RuleSplitCondition with RuleQualifiedName and ParameterMappings
4. Annotation Fixes: Resolved caption annotation leakage in nested control flow (related to PR fix: preserve decision/loop captions across nested control flow #263)
5. Comprehensive Testing: Added unit tests and bug test scripts covering:
   • Rule split condition emission
   • Expression split condition fallback
   • Backend-less fallback behavior
   • BSON serialization roundtrip
   • Nested caption preservation

Checklist Compliance:

• Overlap/Duplication: No issues found - PR addresses specific bug without duplicating existing functionality
• MDL Syntax: Not applicable (no new MDL syntax introduced)
• Full-stack Consistency: Properly wires through backend → executor → SDK layers for existing microflow IF statement handling
• Test Coverage: Excellent - includes unit tests and MDL bug test scripts verifying the fix
• Security/Robustness: No concerns - changes are focused on data handling without introducing vulnerabilities
• Scope/Atomicity: Appropriately scoped - focused on fixing rule-based decision subtype preservation with related annotation fixes
• Code Quality: Clean implementation following existing patterns with helpful comments

The PR correctly addresses the round-trip preservation issue for rule-based decisions and includes the necessary test coverage to prevent regression. The stacked approach (with PR #266 for describer side) is properly noted. Recommended for approval.

---

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

[ako]

PR #265 Review — fix: preserve decision/loop subtype across nested control flow

Overview

Full-stack fix for RuleSplitCondition preservation: new IsRule backend method → mock stub → MPR reader → builder dispatch → BSON writer. Also retroactively fills the test gaps flagged in the PR #263 review (loop/WHILE/InheritanceSplit caption tests + MDL bug script).

What's Good

• Clean backend abstraction compliance: IsRule is on the interface, mock has a Func-field stub, MPR implementation is in backend/mpr/, compile-time check via var _ backend.FullBackend = (*MprBackend)(nil) (which embeds MicroflowBackend) automatically covers IsRule
• tryBuildRuleSplitCondition / unwrapParenCall / extractNamedArg are clean, single-purpose helpers
• BSON Parameter field extracted as string (extractString) rather than binary ID — consistent with how RuleCall.Microflow is stored as a qualified name; the prior binary extraction was likely wrong
• Retroactive MDL bug script for fix: preserve decision/loop captions across nested control flow #263 and the loop/WHILE/InheritanceSplit caption tests fill the gaps flagged in that review

Notes / Non-blockers

IsRule performance (3 reads per call)
reader_documents.go:IsRule lists all units by type, builds a module map, then resolves the qualified name — 3 SQLite reads on every invocation. For a microflow with many IF branches calling rules this adds up. Consider caching the Microflows$Rule set on MprReader (lazily, invalidated on write), similar to how entity metadata is cached elsewhere.

Positional arguments to rule calls silently dropped
extractNamedArg only looks for NamedArgExpr nodes. If the caller passes positional args (myRule(arg1, arg2)), paramMappings stays empty and the rule call is emitted with no parameter mappings — no warning, no error. Consider logging a warning or returning an error when positional args are encountered:

if _, ok := arg.(*ast.NamedArgExpr); !ok {
    return nil, fmt.Errorf("rule call %q: positional arguments are not supported; use named arguments", qualifiedName)
}

Mock default for IsRuleFunc returns (false, nil)
This is a safe default (falls back to ExpressionSplitCondition) but differs from other stubs that return a "MockBackend.X not configured" error when the func is nil. Document the intentional fallback in a comment so future readers don't "fix" it to match the error pattern.

Verdict

LGTM — the correctness fix and test coverage are solid. The IsRule caching and positional-arg warning are worth addressing before the next performance-sensitive workload hits this path, but neither is a blocker for merging.