fix: stop nested splits at parent merge joins

[hjotha]

Closes #455.

Summary

• resolves nested split joins against the active parent merge when the computed nested join is downstream
• prevents the empty-then swap from running against an over-wide nested join
• adds synthetic coverage for downstream parent-merge joins and independent local nested joins

Validation

• make build
• make test

[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

We are given a pull request that is focused on fixing an issue related to nested split joins and parent merge joins in microflows.
The PR title is: "fix: stop nested splits at parent merge joins"

The changes in the PR are:

1. In cmd/mxcli/tui/watcher.go:

   • Added import for "sync/atomic"
   • Added a debounceSeq (atomic.Uint64) to the Watcher struct and used it to avoid processing stale debounce timers.

2. In cmd/mxcli/tui/watcher_test.go:

   • Adjusted the test to remove a time.Sleep and added comments about keeping the burst tight.

3. In mdl-examples/doctype-tests/02b-nanoflow-examples.mdl:

   • This file was completely rewritten to be a much shorter example. It appears to be a cleanup of the nanoflow examples.

4. In mdl/executor/cmd_microflows_show_helpers.go:

   • Added a call to resolveNestedMergeID in traverseFlowUntilMerge to adjust the nestedMergeID.
   • Added two new functions: resolveNestedMergeID and canReachNode to handle the logic for resolving nested merge IDs.

5. In mdl/executor/cmd_microflows_traverse_test.go:

   • Added two new test cases for the new resolveNestedMergeID function.

6. In mdl/executor/roundtrip_doctype_test.go:

   • Updated the known CE errors for some test scripts to reflect changes in the expected output.

7. In mdl/executor/roundtrip_nanoflow_test.go:

   • Made two changes: one to remove a newline in a loop and another to adjust the return type of a nanoflow parameter (adding the word "Enum").

8. In mdl/visitor/visitor_javaaction.go:

   • Changed the way the return type is built for Java actions to use a new helper function buildJavaActionReturnType and added helper functions isVoidReturnType.

9. In mdl/visitor/visitor_javaaction_test.go:

   • Added a test for explicit void return type in Java actions.

Now, we must review this PR against the provided checklist.

Since the PR is about a bug fix (as indicated by the title and the summary) and not about adding new MDL syntax, we focus on the relevant parts of the checklist.

Let's break down the checklist items that are relevant:

Overlap & duplication

• We are not adding new MDL syntax, so we don't need to check for duplication of MDL features.
• However, we should check if the changes introduce code that duplicates existing implementations.

The changes are in:

• watcher.go: fixing a debounce issue in the TUI watcher (not related to core functionality, but a utility)
• watcher_test.go: adjusting a test
• 02b-nanoflow-examples.mdl: rewriting the nanoflow examples (this is a doctype test file, so it's for testing)
• cmd_microflows_show_helpers.go: the core fix for the microflow traversal logic
• cmd_microflows_traverse_test.go: tests for the new helper functions
• roundtrip_doctype_test.go: updating known CE errors (to avoid false positives in tests)
• roundtrip_nanoflow_test.go: minor adjustments to nanoflow tests
• visitor_javaaction.go and visitor_javaaction_test.go: changes to Java action return type handling (unrelated to the main fix?)

The main fix is in cmd_microflows_show_helpers.go and the tests in cmd_microflows_traverse_test.go.

We must check if there is any duplication of existing code. The new functions resolveNestedMergeID and canReachNode are added to handle a specific case in microflow traversal. We don't see an obvious duplication.

MDL syntax design

• The PR does not add or modify any MDL syntax (no changes to grammar, AST, visitor for new statements, etc.). Therefore, we skip this section.

Full-stack consistency

• Since no new MDL feature is added, we don't need to check the full pipeline.

Test coverage

• The PR includes:
  • New unit tests for the helper functions in cmd_microflows_traverse_test.go
  • Adjustments to existing tests (watcher_test, roundtrip tests) to reflect the changes.
• The doctype test file 02b-nanoflow-examples.mdl was rewritten, which might affect the test coverage for nanoflows. However, note that the PR also updated the roundtrip nanoflow test to adjust for the changes in the example.

We should check if the new tests are adequate and if the changes to the doctype test file are justified.

Security & robustness

• The changes do not appear to introduce any security issues (no command injection, XSS, etc.).
• The use of atomic.Uint64 in the watcher is for thread safety and is appropriate.

Scope & atomicity

• The PR is focused on fixing a specific issue: nested split joins against the active parent merge when the computed nested join is downstream, and preventing the empty-then swap from running against an over-wide nested join.
• The changes are related to microflow traversal and the TUI watcher debounce, and the nanoflow examples cleanup.
• However, note that the nanoflow examples cleanup (in 02b-nanoflow-examples.mdl) seems unrelated to the bug fix. It might be a separate cleanup that was included in the same PR.

The PR description says:

• resolves nested split joins against the active parent merge when the computed nested join is downstream
• prevents the empty-then swap from running against an over-wide nested join
• adds synthetic coverage for downstream parent-merge joins and independent local nested joins

The nanoflow examples cleanup does not seem to be related to the bug fix. This might be a violation of the scope and atomicity principle (each PR should do one thing).

However, note that the PR also updated the roundtrip nanoflow test to reflect the changes in the example. So the cleanup of the nanoflow examples might be necessary to make the tests pass? But the bug fix is in microflows, not nanoflows.

Let's look at the changes in the nanoflow examples: the original file was 86 lines and now it's 22 lines. It seems to be a cleanup to remove outdated or redundant examples. This is a refactoring of the test data.

Since the PR is titled as a fix and the nanoflow cleanup is not mentioned in the summary, it might be better to split it. However, the maintainers might have considered it as part of the same effort.

Given the instructions, we are to flag if the PR description needs "and" between unrelated items. The description has three items, all related to the microflow fix. The nanoflow cleanup is not mentioned.

Therefore, we should consider the nanoflow cleanup as an unrelated change and flag it.

But note: the PR also includes changes to the Java action visitor. This is also unrelated to the microflow fix.

Let's list the changes by topic:

A. TUI watcher debounce fix (watcher.go and watcher_test.go)
B. Microflow traversal fix (cmd_microflows_show_helpers.go, cmd_microflows_traverse_test.go)
C. Nanoflow examples cleanup (mdl-examples/doctype-tests/02b-nanoflow-examples.mdl)
D. Roundtrip test adjustments (roundtrip_doctype_test.go, roundtrip_nanoflow_test.go)
E. Java action return type fix (visitor_javaaction.go, visitor_javaaction_test.go)

The PR summary only mentions the microflow fix (items A, B, and D are related to the microflow fix? Actually, A is about the TUI watcher, which is unrelated to microflows).

The TUI watcher change is about fixing a debounce issue in the TUI (terminal UI) when watching files. This is unrelated to the microflow fix.

The Java action change is also unrelated.

Therefore, this PR contains multiple unrelated changes:

• TUI watcher debounce fix
• Microflow traversal fix
• Nanoflow examples cleanup
• Roundtrip test adjustments (which are necessary for the microflow fix and the nanoflow cleanup)
• Java action return type fix

This violates the scope and atomicity principle.

However, note that the PR is labeled as a fix and the title is about nested splits. The maintainers might have grouped multiple small fixes together.

We must decide: is it acceptable to have multiple unrelated fixes in one PR?

According to the checklist:

• Each PR is scoped to a single feature or concern — if the description needs "and" between unrelated items, split it

The description does not mention the TUI watcher, the nanoflow cleanup, or the Java action fix. It only mentions the microflow fix.

Therefore, we should request that the unrelated changes be split into separate PRs.

But note: the nanoflow examples cleanup and the roundtrip nanoflow test adjustment might be necessary because the microflow fix changed the behavior of the microflow executor, which in turn affected the nanoflow tests? Actually, the nanoflow examples are doctype tests, and the roundtrip nanoflow test is a separate test.

However, the changes to the nanoflow examples are a rewrite of the example file, which is not caused by the microflow fix.

Given the above, we have:

Critical Issues:
- The PR contains multiple unrelated changes (TUI watcher, microflow traversal, nanoflow examples, Java action) that are not mentioned in the description and should be split.

Moderate Issues:
- The nanoflow examples cleanup (02b-nanoflow-examples.mdl) is a significant rewrite that removes many examples. We should check if this is intentional and if it doesn't remove useful test cases. However, since it's a doctype test file, it's meant to showcase features. Removing examples might reduce the usefulness of the test file.

Minor Issues:
- The TUI watcher change: we added an atomic to avoid stale debounces. This is a good practice, but we should check if it's necessary. The test was adjusted to remove a time.Sleep and rely on the atomic instead.

What Looks Good:
- The microflow traversal fix includes new tests and seems to address the issue.
- The Java action fix for void return type is a good improvement and includes a test.

Recommendation:

We request changes to split the PR into multiple PRs, each addressing a single concern:

 1. TUI watcher debounce fix
 2. Microflow traversal fix (nested splits at parent merge joins)
 3. Nanoflow examples cleanup (if desired, as a separate refactoring)
 4. Java action return type fix (void return type)

However, note that the roundtrip test adjustments are tied to the changes in the examples and the microflow fix. We might need to adjust the plan.

Alternatively, if the maintainers consider these changes as part of a single effort (which they are not, according to the description), we might allow it but note that it's not ideal.

Given the strictness of the checklist, we should flag the scope issue.

Since the PR description only talks about the microflow fix, we consider the other changes as out of scope.

Therefore, we request changes to remove the unrelated changes and focus only on the microflow fix.

But note: the nanoflow examples cleanup and the Java action change are also fixes? They might be independent.

Let's check the nanoflow examples: the original file had a lot of commented out sections and example nanoflows that were not being used? The new version is much shorter and focuses on a minimal set. This might be intentional to reduce noise.

However, without more context, we cannot be sure.

Given the instructions, we are to review against the checklist. The checklist says:

    [ ] Each PR is scoped to a **single feature or concern** — if the description needs "and" between unrelated items, split it

The description does not mention the TUI watcher, the nanoflow examples, or the Java action. So we flag it.

We also note that the microflow fix is the main topic and the other changes are not justified in the description.

Therefore, we request that the author splits the PR.

However, if we are to review the PR as is, we must note the following:

 - The microflow traversal fix (the main fix) looks correct and has tests.
 - The TUI watcher fix is a good practice and the test is adjusted appropriately.
 - The nanoflow examples cleanup: we are removing a lot of examples. We should check if the examples that are removed are still covered elsewhere. If not, this might be a loss of test coverage. But note: the doctype tests are not the only tests; there are also unit tests and roundtrip tests.

 - The Java action fix: adding support for explicit Void return type is a good improvement and the test is present.

Since we are allowed to skip sections that are not applicable, and the main fix is in scope, we might still approve if we ignore the out-of-scope changes? But the checklist requires us to check scope.

We decide to flag the scope issue as a moderate issue and request changes to split the PR.

However, note that the PR might be small enough that the maintainers are okay with it. But the checklist is clear.

Let's look at the number of files changed: 9 files. This is a bit large for a single fix.

We will structure our review as:

 Critical Issues: None (no show-stoppers)
 Moderate Issues: Scope (multiple unrelated changes)
 Minor Issues: None
 What Looks Good: The microflow fix is well-tested and addresses the issue.

Recommendation: Request changes to split the PR into multiple PRs, each with a single concern.

But note: the author might have intended the nanoflow examples cleanup to be part of the microflow fix because the microflow fix changed the way nanoflows are processed? Actually, the microflow fix is in the microflow executor, and the nanoflow examples are for nanoflows. They are unrelated.

We stick to the scope issue.

However, note that the PR also includes changes to the roundtrip doctype test and roundtrip nanoflow test. These are necessary to update the expected output due to the changes in the examples and the microflow fix.

Therefore, if we split:

 PR1: TUI watcher debounce fix
    - watcher.go
    - watcher_test.go

 PR2: Microflow traversal fix
    - cmd_microflows_show_helpers.go
    - cmd_microflows_traverse_test.go
    - roundtrip_doctype_test.go (only the changes related to the microflow fix? but note that the roundtrip_doctype_test.go also had changes for the nanoflow examples cleanup? Actually, the changes in roundtrip_doctype_test.go are for the nanoflow examples and the microflow examples? We see:
         "03-page-examples.mdl": added CE0115, CE3637, CE5601
         "02b-nanoflow-examples.mdl": replaced the old CE0115 with CE0117, CE6035, and added CE0117, CE6035? Actually, it changed the known CE errors for 02b-nanoflow-examples.mdl.

    This file is updated because the nanoflow examples changed and the microflow examples might have changed? We don't see changes to 03-page-examples.mdl or 02-microflow-examples.mdl in this PR.

    Actually, the roundtrip_doctype_test.go changes are:
      - For "03-page-examples.mdl": added two new CE errors and kept one.
      - For "02b-nanoflow-examples.mdl": replaced the set of CE errors.

    This is due to the nanoflow examples cleanup and possibly the microflow fix? We don't know.

    Given the complexity, it might be safer to leave the roundtrip test adjustments in the same PR as the changes they are adjusting.

 PR3: Nanoflow examples cleanup
    - mdl-examples/doctype-tests/02b-nanoflow-examples.mdl
    - roundtrip_doctype_test.go (for the nanoflow part)
    - roundtrip_nanoflow_test.go (the two changes: removing a newline and adjusting the return type)

 PR4: Java action return type fix
    - visitor_javaaction.go
    - visitor_javaaction_test.go

This is a lot of splitting, but it follows the principle.

However, note that the microflow fix might have caused changes in the roundtrip tests for microflows? We don't see any changes to the roundtrip microflow test in this PR.

Given the time, we will flag the scope issue and request that the author splits the PR.

But note: the PR is from a contributor and we are to be helpful.

We will write:

 Moderate Issues:
    - Scope: The PR contains multiple unrelated changes (TUI watcher debounce fix, microflow traversal fix, nanoflow examples cleanup, Java action return type fix) that are not mentioned in the description. According to the checklist, each PR should be scoped to a single feature or concern. Please split this PR into separate PRs for each distinct change.

However, note that the nanoflow examples cleanup and the Java action fix might be considered as minor improvements that are acceptable to bundle? But the checklist says to split if the description needs "and" between unrelated items.

The description does not mention any of these, so we flag it.

We also note that the TUI watcher change is not related to the Mendix model manipulation at all, so it is definitely out of scope for a PR that is described as a fix for nested splits.

Therefore, we request changes.

If the author does not want to split, we might still accept if we consider that the TUI watcher fix and the Java action fix are trivial and the nanoflow examples cleanup is a refactoring that improves the test suite. But the checklist is strict.

We decide to follow the checklist strictly.

Final decision: Request changes to split the PR.

However, note that the PR

---

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