Skip to content

fix: address merged microflow review followups#408

Open
hjotha wants to merge 2 commits intomendixlabs:mainfrom
hjotha:submit/merged-pr-review-followups
Open

fix: address merged microflow review followups#408
hjotha wants to merge 2 commits intomendixlabs:mainfrom
hjotha:submit/merged-pr-review-followups

Conversation

@hjotha
Copy link
Copy Markdown
Contributor

@hjotha hjotha commented Apr 30, 2026

Summary

This PR collects follow-ups from reviews on already-merged microflow roundtrip PRs so the actionable feedback does not get lost.

Fixes:

  • Stop manual while true detection from treating continue inside a nested loop as a continue for the outer loop.
  • Mark add $Item to $List and remove $Item from $List target lists as list consumers so owner-both reverse retrieves keep the correct source form.
  • Set nanoflow return-value render context on the direct describe nanoflow path so value-returning nanoflows do not emit bare return; for empty EndEvents.

Review follow-up coverage:

  • Add writer coverage that commit actions always serialize default ErrorHandlingType.
  • Fix the EndEvent empty-return writer test to exercise the actual empty string case.
  • Add a negative regression assertion for non-empty change-object refresh inference.
  • Add download-file formatter coverage for the no-browser flag case.
  • Tighten reverse-association compact XPath name validation and make the owner-both condition explicit.
  • Document MXCLI_EXEC_TIMEOUT in the quick reference.

Closes #404.
Closes #405.
Closes #406.

Tests

  • make build
  • make test
  • make lint-go

@github-actions
Copy link
Copy Markdown

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

  • In mdl/executor/cmd_microflows_builder_control.go, the comment for containsContinueForCurrentLoop says it "mirrors containsBreakForCurrentLoop" but the implementation differs slightly (it returns early for LoopStmt/WhileStmt cases while the break version doesn't). This is actually correct behavior since continues in nested loops should not propagate outward, but the comment could be clearer about why the implementations differ.

What Looks Good

Recommendation

Approve. The PR correctly implements focused bug fixes with appropriate test coverage and documentation updates. The changes are minimal, targeted, and maintain code quality standards. Minor comment clarification suggested but not blocking.


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

@hjotha hjotha force-pushed the submit/merged-pr-review-followups branch from 7a08d1b to 3e8d0a6 Compare April 30, 2026 07:42
@github-actions
Copy link
Copy Markdown

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

None found.

What Looks Good

Recommendation

Approve. This PR consists of well-targeted bug fixes and test improvements for previously merged microflow/nanoflow functionality. Each change addresses a specific reported issue with appropriate test coverage, and the code quality is maintained throughout. As these are follow-ups to already-merged work and don't introduce new MDL syntax, they appropriately bypass the full-stack consistency requirements for new features while still maintaining the project's standards for bug fixes and test coverage.


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

@ako ako added this to the v0.8.0 milestone Apr 30, 2026
@hjotha hjotha force-pushed the submit/merged-pr-review-followups branch 2 times, most recently from 23019b5 to 22b7936 Compare May 2, 2026 10:04
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 2, 2026

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

  • In mdl/executor/roundtrip_nanoflow_test.go, the parameter type notation was changed from ($Color: + testModule + .NfColor) to ($Color: Enum + testModule + .NfColor). Parameters in MDL don't use the : Enum syntax - this should likely be just ($Color: + testModule + .NfColor) or if it's specifically testing enum parameters, it should follow the correct MDL syntax for enum parameters (which would still not include the word "Enum" in the type annotation).

  • The doctype test file mdl-examples/doctype-tests/02b-nanoflow-examples.mdl was significantly simplified, removing many examples that demonstrated important nanoflow features. While streamlining tests can be good, we should ensure essential nanoflow features are still covered. The current version covers basics but misses some intermediate patterns.

What Looks Good

  • The watcher debounce fix using sync/atomic properly addresses potential race conditions in rapid file change scenarios.
  • The manual while true detection fix correctly handles nested loops by distinguishing continues in inner vs outer loops.
  • The list consumer marking for add/remove statements properly tracks target lists as inputs, fixing the owner-both reverse retrieve issue.
  • The nanoflow describe context fix ensures return-value nanoflows don't emit bare return; statements.
  • Test coverage is improved with new test files for the specific bugs fixed (Manual while-true detection over-triggers when continue lives only inside nested loop body #404, Reverse retrieve incorrectly classified when output feeds add/remove-from-list statements #405) and enhanced existing tests.
  • Documentation for MXCLI_EXEC_TIMEOUT was added to the quick reference as requested.
  • The commit action test verifies the default ErrorHandlingType is properly serialized.
  • The download-file test covers the no-browser flag case.
  • XPath validation was tightened to reject more invalid patterns.
  • All changes are focused, well-tested, and address specific issues identified in previous review cycles.

Recommendation

Approve with minor noted issues (the parameter type syntax in the nanoflow test should be corrected). The PR successfully addresses the review follow-ups from merged microflow roundtrip PRs and improves test coverage and robustness.


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

@hjotha
Copy link
Copy Markdown
Contributor Author

hjotha commented May 2, 2026

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
Copy link
Copy Markdown

github-actions Bot commented May 2, 2026

AI Code Review

Review Summary

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

None found.

What Looks Good

Recommendation

Approve - The PR consists of well-targeted bug fixes with appropriate test coverage and documentation updates. All changes are minor improvements that address specific issues without introducing new functionality or breaking existing patterns. The PR successfully resolves the stated follow-up items from merged microflow PRs.


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

@github-actions github-actions Bot mentioned this pull request May 3, 2026
1. Manual while-true detection over-triggered when the only continue lived
   inside a nested loop. The continue scan crossed loop boundaries while the
   break scan did not, so the outer while could be rebuilt as a manual
   back-edge. Stop the continue scan at nested loops and add a regression test.

2. Owner-both reverse retrieves feeding add/remove-to-list were still treated
   as object-only consumers. The list pre-scan ignored AddToListStmt and
   RemoveFromListStmt target lists, so AssociationRetrieveSource could be
   suppressed. Track those list consumers and add coverage for the helper.

3. Direct nanoflow describe did not set the return-value render context used
   by EndEvent formatting. Thread the wrapped nanoflow return type through the
   formatter so value-returning nanoflows do not emit bare return; for empty
   EndEvents.

Also folds in low-risk review followups: commit-action writer coverage,
change-object refresh negative coverage, download-file formatter coverage,
reverse-retrieve name validation tightening, and documentation for
MXCLI_EXEC_TIMEOUT.

Tests: make build
Tests: make test
Tests: make lint-go

Closes mendixlabs#404.
Closes mendixlabs#405.
Closes mendixlabs#406.
@hjotha hjotha force-pushed the submit/merged-pr-review-followups branch from 7d13996 to 5d88b0a Compare May 4, 2026 06:14
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

None found.

What Looks Good

The PR successfully addresses multiple follow-up issues from previous microflow roundtrip work:

  1. Fixes manual while true detection (issue Manual while-true detection over-triggers when continue lives only inside nested loop body #404):

    • Correctly modified containsContinueStmt to containsContinueForCurrentLoop to prevent nested loop continues from affecting outer loop detection
    • Added comprehensive test TestBuildFlowGraph_ManualWhileTrueIgnoresNestedLoopContinue that verifies the fix works as intended
    • The fix aligns with how containsBreakForCurrentLoop handles nested loops
  2. Marks add/remove from list targets as list consumers (issue Reverse retrieve incorrectly classified when output feeds add/remove-from-list statements #405):

    • Enhanced collectListInputVariables to properly track AddToListStmt and RemoveFromListStmt target lists
    • Added test TestCollectListInputVariables_AddRemoveFromList confirming both statement types mark their target lists as inputs
    • This ensures owner-both reverse retrieves maintain correct source form
  3. Sets nanoflow return-value context for describe path (issue describeNanoflow loses void-microflow EndEvent fix #406):

    • Modified describeNanoflow to set DescribingMicroflowHasReturnValue based on wrapper microflow's return type
    • Added test TestDescribeNanoflow_ReturningFlowSkipsEmptyEndEvent verifying value-returning nanoflows don't emit bare return;
    • Properly restores previous context value using defer
  4. Additional valuable improvements:

    • Commit action serialization test ensures default ErrorHandlingType is always written
    • Enhanced EndEvent test naming and coverage
    • Negative regression test for non-empty change-object refresh inference
    • Download-file formatter test for no-browser flag case
    • Tightened reverse-association XPath validation with explicit owner-both condition
    • Documentation update for MXCLI_EXEC_TIMEOUT environment variable

All fixes include appropriate test coverage, follow existing code patterns, and maintain backward compatibility. The changes are well-scoped to the microflow/nanoflow execution logic without introducing unrelated modifications.

Recommendation

Approve - The PR addresses legitimate follow-up issues from previous work with appropriate fixes, tests, and documentation. All changes are correct, well-tested, and maintain code quality standards. No blocking issues identified.


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

formatAction emitted `$Var = call microflow/nanoflow/java/javascript/external`
whenever the action's ResultVariableName/OutputVariableName was
non-empty, ignoring UseReturnVariable. Studio Pro stores the
variable name as a UI fallback even when UseReturnVariable=false
(the action discards its return), so re-exec of described MDL
introduced phantom output variable declarations.

Symptom: microflows with two call actions pointing at the same
sub-microflow/Java action, both with UseReturnVariable=false but
both carrying the same ResultVariableName, round-tripped as two
declarations of the same output variable, triggering CE0111
"Duplicate variable name" in Studio Pro.

Fix: emit the `$Var = ` prefix only when UseReturnVariable is true.
Applies to MicroflowCallAction, NanoflowCallAction, JavaActionCallAction,
JavaScriptActionCallAction, and CallExternalAction.

Existing tests that relied on the old behavior now set UseReturnVariable
explicitly — they were exercising the wrong BSON shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@hjotha hjotha force-pushed the submit/merged-pr-review-followups branch from 5d88b0a to 1568e64 Compare May 4, 2026 06:24
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

AI Code Review

Critical Issues

None

Moderate Issues

None

Minor Issues

None

What Looks Good

The PR successfully addresses multiple follow-up issues from previously merged microflow roundtrip PRs with appropriate fixes and test coverage:

  1. Issue Manual while-true detection over-triggers when continue lives only inside nested loop body #404 - Fixed manual while true detection to ignore continue statements inside nested loops by:

    • Correcting containsContinueStmt to containsContinueForCurrentLoop in isManualWhileTrueCandidate
    • Adding comprehensive test TestBuildFlowGraph_ManualWhileTrueIgnoresNestedLoopContinue verifying outer while is built as LoopedActivity not ExclusiveMerge
    • The fix prevents incorrect infinite loop generation in BSON
  2. Issue Reverse retrieve incorrectly classified when output feeds add/remove-from-list statements #405 - Marked add to list/remove from list target lists as list consumers:

    • Enhanced collectListInputVariables to handle AddToListStmt/RemoveFromListStmt
    • Fixed owner-both condition in expandReverseReference logic
    • Added test TestCollectListInputVariables_AddRemoveFromList verifying list variables are tracked as inputs
    • Resolves AssociationRetrieveSource suppression issue for Owner=Both reverses
  3. Issue describeNanoflow loses void-microflow EndEvent fix #406 - Set nanoflow return-value render context:

    • Modified describeNanoflow to set DescribingMicroflowHasReturnValue via wrapper microflow
    • Updated formatActivity EndEvent handling to skip bare return; when describing value-returning flows
    • Added test TestDescribeNanoflow_ReturningFlowSkipsEmptyEndEvent validating proper nanoflow description
    • Fixes empty return; emission for value-returning nanoflows with empty EndEvents

Additional improvements:

  • Added test coverage for CommitAction default ErrorHandlingType serialization
  • Fixed EndEvent empty-return test to exercise actual empty string case
  • Added negative regression test for non-empty change-object refresh inference
  • Enhanced download-file formatter coverage for no-browser flag case
  • Tightened reverse-association XPath validation with explicit owner-both condition
  • Documented MXCLI_EXEC_TIMEOUT in quick reference

All changes are accompanied by appropriate unit tests that verify the fixes without relying on time.Sleep for synchronization. The modifications are focused, well-scoped, and maintain consistency with existing code patterns.

Recommendation

Approve the PR. It addresses all stated issues with correct fixes and sufficient test coverage, maintaining the project's quality standards. No blocking concerns identified.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants