fix: address merged microflow review follow-ups

[hjotha]

Closes #459.

Summary

• reject enum splits with more branches than the supported anchor table in both validation and builder paths
• add a doctype example that covers loop @caption metadata
• clean up the SOAP call web service grammar comment so it matches the qualified-name structured syntax

Validation

• make build
• ./bin/mxcli check mdl-examples/doctype-tests/02-microflow-examples.mdl
• go test ./mdl/executor -run 'EnumSplit|ValidateMicroflow.*EnumSplit' -count=1
• make test
• make lint-go

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• Grammar comment cleanup: The change in mdl/grammar/MDLParser.g4 updates a comment about SOAP call structured syntax. While this improves clarity, it's a documentation-only change and doesn't affect functionality. No action needed.
• Test organization: The new enum split tests are split between builder and validation files. This is acceptable but note that the validation test could share more setup with existing tests (though current implementation is clear).

What Looks Good

• Enum split branch limit: The PR correctly adds validation in both builder (addEnumSplit) and validation (validateStatement) paths to enforce the maximum branch limit (16), using the existing splitCaseOrderAnchors array. This prevents invalid microflows that would fail at runtime.
• Test coverage: New unit tests verify:
  • Builder rejects splits with >16 branches (returns empty ID + error)
  • Validation catches splits with >16 branches (returns appropriate error message)
  • Tests use helper functions (enumSplitWithBranchCount, enumSplitBranchCount) for clarity and reuse
• Doctest example: Added clear example 6.1b demonstrating @caption metadata on a loop in 02-microflow-examples.mdl, following existing example formatting conventions.
• Consistency:
  • Uses existing maxEnumSplitBranches derived from splitCaseOrderAnchors (no magic numbers)
  • Error message format matches existing validation errors ("enum split has X branches; at most Y branches are supported")
  • Follows established patterns in microflow executor code (error accumulation via fb.addError)

Recommendation

Approve the PR. Changes are minimal, focused, and address the stated issues:

1. Enum split branch limit now enforced in both validation and builder paths
2. Added doctype example for loop @caption metadata
3. Improved SOAP call grammar comment for clarity

All validation steps pass (make build, make test, make lint-go implied by PR description), and the changes maintain full-stack consistency for existing features (no new MDL syntax introduced). The PR is scoped to microflow improvements as indicated in the title.

---

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

[github-actions]

AI Code Review

Review Summary

This PR addresses microflow review follow-ups with three main changes:

1. Added a doctype example demonstrating loop @caption metadata
2. Implemented enum split branch limit validation (max 16 branches) in both builder and validation paths
3. Cleaned up SOAP call web service grammar comment to reflect qualified-name usage
4. Fixed anchor preservation for enum split first statements during describe→exec roundtrip

The changes are well-scoped, technically sound, and maintain full-stack consistency where applicable. No critical issues were found.

---

Critical Issues

• None found

Moderate Issues

• None found

Minor Issues

• Documentation placement: The new loop caption example (6.1b) is inserted between existing examples 6.1 and 6.2. While logically correct, consider adding a brief transition comment to improve readability in the doctest file.
• Test clarity: The enum split branch limit test uses maxEnumSplitBranches + 1 (17 branches) but doesn't explicitly reference the constant in the assertion message. The error message correctly shows "17 branches; at most 16", but the test could be more self-documenting by referencing the constant.
• Grammar comment: The SOAP call comment cleanup is helpful but slightly redundant since the rule already implies structured references. Minor wording tweak could improve clarity: "references as qualified names" → "references using qualified names".

What Looks Good

• Enum split validation:
  • Branch limit now properly enforced in both builder (addEnumSplit) and validator (validateStatement)
  • Correctly counts else body as an additional branch
  • Error message is concise and actionable
  • Comprehensive test coverage for both code paths
• Anchor preservation:
  • Fixes silent loss of @anchor(from:..., to:...) on first case statement
  • Maintains roundtrip integrity (describe → exec → describe)
  • Well-commented implementation explaining the flow anchoring logic
  • Dedicated test verifies anchor preservation
• Doctype example:
  • Practical demonstration of loop @caption metadata
  • Follows existing microflow example patterns
  • Valid MDL syntax that passes mxcli check
• Grammar cleanup:
  • Accurately reflects that SOAP calls use qualified names (not string literals) for structured references
  • Maintains awareness of STRING_LITERAL fallback for edge cases
• Code quality:
  • No duplication with existing implementations
  • Changes are focused and atomic per concern
  • Method receivers and error handling are correct
  • No hot-path file I/O or security issues

Recommendation

Approve. The PR successfully addresses the microflow review follow-ups with technically sound changes that improve validation, preserve metadata roundtrips, and enhance documentation. All changes align with project conventions and pass the required validation steps. Minor documentation improvements are suggested but not blocking.

---

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

[hjotha]

CI stabilization note: merged #412 into this branch so it uses the same integration-CI baseline as the rest of the active PR set.

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• The nanoflow example rewrite in 02b-nanoflow-examples.mdl removes substantial historical examples. While the new examples are clearer, verify that removed examples (like security grants, access checks, and lifecycle operations) are still covered elsewhere in the test suite. The PR adds equivalent new examples but maintains a different structure.
• The SOAP call grammar comment change in MDLParser.g4 is purely documentation; ensure it accurately reflects the implementation (no functional change).

What Looks Good

• Enum split branch limit validation is now centralized and consistent between builder and validator paths, properly counting else branches.
• Anchor preservation fix for enum split first statements ensures @anchor metadata round-trips correctly through describe/exec cycles.
• TUI watcher debouncing now uses atomic sequencing to prevent race conditions on rapid file changes.
• Doctest examples updated to demonstrate loop @caption metadata and modernized nanoflow examples improve readability.
• Test coverage added for enum split branch limits and anchor preservation in both unit and integration tests.

Recommendation

Approve. Changes are focused, well-tested, and address microflow review follow-ups without violating MDL syntax principles or architectural boundaries. Minor documentation improvements in examples are acceptable.

---

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

[github-actions]

AI Code Review

We are given a PR that has several changes. We need to review it against the checklist.

First, let's note the PR's title and body:

Title: "fix: address merged microflow review follow-ups"

Body:

• Closes Handle merged microflow review follow-ups #459.
• Summary:
  • reject enum splits with more branches than the supported anchor table in both validation and builder paths
  • add a doctype example that covers loop @caption metadata
  • clean up the SOAP call web service grammar comment so it matches the qualified-name structured syntax

Validation steps:

• make build
• ./bin/mxcli check mdl-examples/doctype-tests/02-microflow-examples.mdl
• go test ./mdl/executor -run 'EnumSplit|ValidateMicroflow.*EnumSplit' -count=1
• make test
• make lint-go

Now, we must review against the checklist.

We'll break down the changes by file:

1. cmd/mxcli/tui/watcher.go and cmd/mxcli/tui/watcher_test.go: Changes to the watcher for debouncing. This is not related to MDL syntax or the main features mentioned (enum splits, loop caption, SOAP comment). It's an internal TUI improvement.

2. mdl-examples/doctype-tests/02-microflow-examples.mdl: Added an example for a loop with a custom caption (using @caption). This is a new example for an existing feature (loop with caption). It does not change the syntax but adds an example.

3. mdl-examples/doctype-tests/02b-nanoflow-examples.mdl: This file was completely rewritten. It appears to be a refactoring of the nanoflow examples to be more concise and to remove some outdated content (like the helper microflow and page). It also updates the examples to use a more modern style (e.g., using nf_ prefix, removing some roles, etc.). However, note that the PR description does not mention this change. We must check if this is intentional and if it breaks anything.

4. mdl/executor/cmd_microflows_builder_actions.go: Changes to the enum split handling in the microflow builder. It adds a check for the number of branches (cases + else body) against a maximum (which is set to the length of splitCaseOrderAnchors). It also refactors the flow building to better handle anchors (for the first statement in a case) and to avoid dropping anchor information.

5. mdl/executor/cmd_microflows_builder_enum_split_test.go: Added tests for the enum split builder to reject too many branches and to preserve the anchor on the first statement in a case.

6. mdl/executor/cmd_microflows_builder_validate.go: Added a validation check for enum splits to reject too many branches (same as in the builder).

7. mdl/executor/roundtrip_doctype_test.go: Updated the known CE errors for some doctype files (added some and removed one for nanoflow examples).

8. mdl/executor/roundtrip_nanoflow_test.go: Changed a test to use Enum keyword in the nanoflow parameter (from $Color: testModule.NfColor to $Color: Enum testModule.NfColor). This is a syntax change? Actually, it's making the test more explicit by adding the Enum keyword. But note: the PR description does not mention this.

9. mdl/executor/validate_microflow_enum_split_test.go: Added a test for the validation of enum splits rejecting too many branches.

10. mdl/grammar/MDLParser.g4: Changed a comment in the SOAP call web service grammar rule to clarify that it uses qualified names (and STRING_LITERAL as fallback).

11. mdl/visitor/visitor_javaaction.go and mdl/visitor/visitor_javaaction_test.go: Changes to handle void return type in Java actions. This adds a helper function to check if the return type is void (by checking if the qualified name is an empty module and the name is "void", case-insensitive). Also added a test for explicit void return type.

Now, let's go through the checklist:

Overlap & duplication

• The PR does not seem to introduce duplicate functionality. The changes are about enum splits, loop caption example, and SOAP comment cleanup. The nanoflow examples rewrite might be overlapping with existing examples, but it's a refactoring of the same file. We should check if the nanoflow examples file was already covering the same functionality. Since it's a rewrite, it might be removing some examples and adding others. However, the PR does not mention this change, so we must be cautious.

  Looking at the diff of 02b-nanoflow-examples.mdl: it removed a lot of content (helper microflow, helper page, several nanoflow examples) and replaced them with a minimal set. This might be because the previous examples were outdated or incorrect. However, without an issue or discussion, we cannot be sure. But note: the PR description does not mention this file, so it might be out of scope. We should flag if it's not related to the issue.

  However, the PR says it closes Handle merged microflow review follow-ups #459. We don't have the issue, but we can assume that the changes in the PR are intended to fix that issue. The issue might be about enum splits and loop caption. The nanoflow examples rewrite might be unrelated.

  Since the PR description does not mention the nanoflow examples, and we are reviewing for the PR, we should note that this change is not explained. It might be a mistake or it might be part of the issue. We'll have to see if it breaks anything.

  But note: the validation step includes running ./bin/mxcli check mdl-examples/doctype-tests/02-microflow-examples.mdl and the enum split tests, but not the nanoflow examples. So the nanoflow examples change might not be tested by the provided validation steps.

  However, the PR also runs make test and make lint-go, so hopefully the nanoflow examples are tested in the test suite.

  Given that the nanoflow examples file is a doctype test, it should be valid. We'll assume the rewrite is intentional and correct.

MDL syntax design (for new or modified MDL syntax)

The PR does not add new MDL syntax. It:

• Adds an example of existing syntax (loop with @caption).
• Changes a comment in the grammar (not syntax).
• Changes the validation and builder for enum splits (but this is not changing the syntax, just adding a limit and fixing anchor handling).
• Changes the nanoflow examples file (which uses existing syntax, but note: the rewrite changes the examples to use a different style, but the syntax is the same).
• Changes the Java action visitor to handle void return type (which is about interpreting existing syntax, not changing it).

Therefore, there is no new or modified MDL syntax in this PR. So we can skip this section.

Full-stack consistency (for MDL features)

Since there is no new MDL syntax, we don't need to check the full pipeline. However, note that the enum split validation and builder changes are part of an existing feature (enum split). We are not adding a new feature, but we are improving an existing one. The changes are in the executor (builder and validator) and the tests. We do not need to touch the grammar, AST, visitor, LSP, etc. because we are not changing the syntax.

But note: the PR does change the visitor for Java actions (to handle void return type). This is not a syntax change, but an improvement in handling an existing syntax (the RETURNS Void in a Java action). However, the Java action syntax is already supported. The change is in the visitor to correctly interpret the void return type.

We should check if this change requires any of the full-stack steps? Since it's not a syntax change, no. The grammar, AST, etc. are not touched.

However, note that the Java action visitor change is in mdl/visitor/visitor_javaaction.go. This is part of the visitor that builds the AST. We are not changing the AST structure, but we are changing how we build the AST for a Java action statement (specifically the return type). This is acceptable because we are not changing the AST node types, just the way we populate an existing field.

But we must check if the AST node for CreateJavaActionStmt already had a ReturnType field? Yes, it does. We are just changing how we set it.

Therefore, no full-stack consistency issues.

Test coverage

• The PR adds tests for the enum split builder and validation (in cmd_microflows_builder_enum_split_test.go and validate_microflow_enum_split_test.go).
• It also adds a test for the Java action void return type (in visitor_javaaction_test.go).
• The doctype examples are updated (02-microflow-examples.mdl gets a new example for loop caption, and 02b-nanoflow-examples.mdl is rewritten).
• The validation steps include running the enum split tests and the microflow examples check.

We should note that the nanoflow examples rewrite might have removed some test coverage? But the rewrite is intended to be equivalent. We'll trust that the author tested it.

Security & robustness

• The changes in the watcher use sync/atomic for a debounce sequence. This is safe.
• The enum split changes add error checking for too many branches, which is robust.
• The Java action change adds a helper function to check for void return type, which is safe.

No obvious security issues.

Scope & atomicity

The PR has multiple changes:

• Watcher debounce improvement (unrelated to the issue?)
• Loop caption example (related to the issue? The issue might be about enum splits and loop caption)
• SOAP comment cleanup (unrelated to the issue? but mentioned in the PR)
• Enum split validation and builder improvements (core of the issue)
• Nanoflow examples rewrite (unexplained)
• Java action void return type handling (unexplained)

The PR description says it addresses "merged microflow review follow-ups". This suggests that the changes are follow-ups from a previous microflow-related PR. The enum split changes are clearly microflow-related. The loop caption example is also microflow-related (since loops are in microflows and nanoflows). The SOAP comment cleanup might be unrelated. The nanoflow examples rewrite and Java action change are also unexplained.

However, note that the PR closes #459. Without seeing the issue, we cannot be sure. But the validation steps include testing the enum split and the microflow examples, so the core of the PR is about microflows.

We might consider splitting the PR into:

• One for the enum split and loop caption (which are microflow related and explained in the summary)
• One for the watcher debounce (TUI improvement)
• One for the SOAP comment cleanup (documentation)
• One for the nanoflow examples rewrite (if intentional)
• One for the Java action void return type (if needed)

But since the PR is small and the changes are localized, and the validation passes, we might accept it as a single PR if the unexplained changes are actually part of the issue or are minor and safe.

However, the nanoflow examples rewrite is a significant change to a doctype test file. We should check if it is necessary and correct.

Let's look at the nanoflow examples rewrite: it removed the helper microflow and helper page, and several nanoflow examples. It replaced them with a minimal set. This might be because the previous examples were causing errors or were outdated. Since the PR does not mention it, we must be cautious.

But note: the validation step includes make test, which likely runs the doctype tests. So if the rewrite broke something, the tests would fail. Since the author ran make test and it passed, we can assume the rewrite is correct.

Similarly, the Java action change: the author added a test and it passed.

Therefore, we can consider the PR as a whole.

Code quality

The changes look reasonable. The enum split refactoring is to avoid duplicating code (by introducing enumSplitBranchCount and maxEnumSplitBranches). The watcher change uses atomic for a sequence number to avoid race conditions in the debouncer.

Bugs & correctness

The enum split changes fix two issues:

1. Rejecting enum splits with too many branches (in both validation and builder).
2. Preserving the anchor on the first statement in a case (so that round-trip works).

The Java action change fixes the handling of void return type.

The loop caption example adds a missing example.

The SOAP comment cleanup improves the comment.

The nanoflow examples rewrite might fix outdated examples.

The watcher change fixes a potential race condition in the debouncer.

Now, let's structure the review:

Critical Issues: None found.
Moderate Issues: None found.
Minor Issues:

• The PR includes changes that are not mentioned in the description (nanoflow examples rewrite, Java action void return type, watcher debounce). While they might be correct, it would be better to have them in separate PRs or at least explain them in the description.
• However, the PR description says it addresses "merged microflow review follow-ups", and the nanoflow examples and Java action are not microflow-specific. The watcher is TUI. The SOAP comment is documentation.

But note: the nanoflow examples are in the doctype tests for nanoflows, and the Java action is related to microflows (since Java actions can be used in microflows). So they are somewhat related.

Given that the validation passes and the changes are safe, we can accept them.

What Looks Good:

• The enum split changes are well-tested and address the issue of too many branches and anchor preservation.
• The loop caption example is a good addition to the doctype tests.
• The SOAP comment cleanup is helpful for future developers.
• The watcher debounce improvement is a good robustness fix.

Recommendation: Approve the PR, but note that the unexplained changes (nanoflow examples rewrite, Java action void return type, watcher debounce) should be explained in the description or split into separate PRs in the future. However, for this PR, since they are small and the validation passes, we can approve.

But wait: the PR description does not mention the nanoflow examples rewrite at all. We must check if this change is intentional and if it is part of the issue. Since we don't have the issue, we rely on the author's testing.

Given that the author ran make test and it passed, we assume it's correct.

Let's check the nanoflow examples rewrite: it removed the User and Admin module roles and replaced them with no roles? Actually, the rewrite does not create any roles. Then it grants execute on the nanoflows to the User role. But the User role is not defined! This will cause an error when the script is run.

Look at the rewritten 02b-nanoflow-examples.mdl:

create module NanoflowExamples;
// ... (no roles created)
...
grant execute on nanoflow NanoflowExamples.NF_ValidateProduct to NanoflowExamples.User;
...

The role NanoflowExamples.User is used in the grant but never created. This is an error.

In the original file, there were:

create module role NanoflowExamples.User;
create module role NanoflowExamples.Admin;

But in the rewrite, these are removed.

This is a bug. The PR should not have removed the role definitions.

Therefore, we have found a bug in the nanoflow examples rewrite.

We must request changes to fix this.

Additionally, the Java action change: we should check if it is necessary and correct. But it seems safe.

But the nanoflow examples rewrite is broken because of the missing role.

Let's fix the nanoflow examples by adding back the role definitions? Or remove the grants that use the undefined roles.

Looking at the rewrite, it only has grants for:

• NF_ValidateProduct to User
• NF_SaveProduct to User
• NF_FormatPrice to User

And then it revokes NF_ValidateProduct from User.

So we need the User role to exist.

We have two options:

1. Add back the role definitions for User and Admin (even if we don't use Admin in the grants, it was in the original and we removed it, but the rewrite only uses User).
2. Change the grants to not use a role (but that doesn't make sense) or use a different role.

Since the original had two roles and the rewrite only uses one, we can just add the User role.

But note: the rewrite also removed the Admin role. We don't see any grant to Admin in the rewrite, so we don't need to add it back? However, the original had a grant to Admin for NF_FormatPrice? Let's check:

Original:

grant execute on nanoflow NanoflowExamples.N008_FormatPrice to NanoflowExamples.User, NanoflowExamples.Admin;

In the rewrite, we have:

grant execute on nanoflow NanoflowExamples.NF_FormatPrice to NanoflowExamples.User;

So we removed the Admin role from the grant. Therefore, we don't need the Admin role.

But we do need the User role.

So we should add:

create module role NanoflowExamples.User;

We can add it after the module creation.

Alternatively, we can remove the grants that use the undefined role and use no role? But that would change the meaning.

Given that the example is about security, it's better to keep the role.

Therefore, we must fix the nanoflow examples file by adding the missing role.

Let's also note that the rewrite removed the helper microflow and helper page. This might be intentional to simplify the example, but we must ensure that the examples that remain do not depend on them.

Looking at the remaining nanoflows:

• NF_Empty: no dependencies.
• NF_ValidateProduct: only uses the Product entity (which is defined).
• NF_SaveProduct: calls NF_ValidateProduct (so it depends on that nanoflow).
• NF_FormatPrice: uses $Amount and $Currency and returns a string.

None of them depend on the helper microflow or page. So it's safe to remove them.

But the role is missing.

Therefore, we must request a change to add the missing role in the nanoflow examples

---

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

[ako]

Review: fix: address merged microflow review follow-ups

Summary

Three targeted follow-ups from earlier reviews: (1) the enum split branch-count guard now includes the else body, not just named cases; (2) @anchor on the first statement inside a case branch is now forwarded to the split→firstActivity flow instead of silently dropped; (3) maxEnumSplitBranches named var removes the magic-number inline comparison. Good regression-test coverage for all three. Grammar comment cleanup is an improvement.

No blockers.

---

Minor

1. Hardcoded 17/16 in both test error-string assertions

TestEnumSplitBuilderRejectsMoreThanSupportedBranches and TestValidateMicroflowBody_EnumSplitRejectsMoreThanSupportedBranches both check for the literal string "enum split has 17 branches; at most 16 branches are supported". If splitCaseOrderAnchors grows, both tests fail with a confusing mismatch instead of surfacing the real breakage. Should use fmt.Sprintf:

expected := fmt.Sprintf("enum split has %d branches; at most %d branches are supported",
    maxEnumSplitBranches+1, maxEnumSplitBranches)
if !strings.Contains(errors, expected) { ... }

2. Unrelated changes bundled (recurring — #366, #370, #474, #448, #450, #452, #460)

Watcher debounce, Java action RETURNS Void, nanoflow example cleanup, and nanoflow roundtrip test changes are all independent of the enum split anchor work. CLAUDE.md: "Each PR is scoped to a single feature or concern."

---

Positives

• Else body correctly contributes to the branch count — the previous guard counted only len(s.Cases) and could have let an ELSE branch push the anchor index past the table end.
• TestEnumSplitBuilderPreservesFirstStatementAnchor is a well-structured direct test that pins the specific failure mode (dropped DestinationConnectionIndex).
• maxEnumSplitBranches as a named var eliminates the "magic 16" from two different places.