fix: preserve custom error handler continuations#366
Conversation
ako
left a comment
ako
left a comment
There was a problem hiding this comment.
PR #366 — fix: preserve custom error handler continuations
Reviewed against the CLAUDE.md checklist.
Blockers
1. statementVarRefs and statementsVarRefs duplicated across open PRs — compile error when merged
PR #337 adds statementVarRefs and statementsVarRefs to cmd_microflows_builder_graph.go. This PR adds the same two functions to cmd_microflows_builder_flows.go. Both files are in package executor, both PRs target main. Merging them in either order produces:
./cmd_microflows_builder_flows.go: statementVarRefs redeclared in this block
./cmd_microflows_builder_flows.go: statementsVarRefs redeclared in this block
The two implementations are almost identical but differ in two places:
- PR #337 is missing the
RestCallStmtbody field refs (s.Body.Template,s.Body.TemplateParams,s.Body.SourceVariable) that PR #366 adds — so the PR #366 version is the more complete one. - PR #337 has an
if s.Item != ""nil-guard in theAddToListStmtcase; PR #366 does not.
Fix options:
- Move the canonical (merged) implementation to a new shared file
cmd_microflows_builder_varutil.gobefore either PR merges, and remove the definitions from bothbuilder_graph.goandbuilder_flows.go. - Or rebase PR #366 on PR #337's branch so that
builder_graph.goalready has the functions andbuilder_flows.godoesn't need to redeclare them.
Also worth noting: PR #337 adds referencedVariableSet(stmts) map[string]bool and this PR adds statementsReferenceVar(stmts, varName) bool. Both wrap statementsVarRefs to answer "does this variable appear here?" with different return shapes. After both PRs land, the package will have two overlapping utilities. Consolidating to one is not strictly a blocker, but flagging it for cleanup.
Moderate Issues
2. Active-handler state split between fields and slice — invariant not documented
flowBuilder gains 7 new fields (emptyErrorHandlerFrom, errorHandlerTailFrom, errorHandlerSource, errorHandlerSkipVar, errorHandlerTailIsSource, errorHandlerReturnValue, pendingErrorHandlers). The design is: the active handler lives in the flat fields; completed handlers are queued in the slice via queueActivePendingErrorHandler(). activePendingErrorHandler() and setActivePendingErrorHandler() shuttle between these two representations.
This invariant is non-obvious. A future reader adding a new call site may write directly to a field instead of going through the accessor methods and silently break the queuing. A short comment on the struct fields stating the invariant would pay off.
3. findExistingRejoinMerge scans all objects
The function iterates the entire fb.objects slice looking for an ExclusiveMerge whose destination flow matches. For typical Studio Pro microflows this is fine in practice, but a note that this is an O(N) scan (and the N is bounded by normal microflow sizes) would make the intent clearer.
Minor Issues
4. AnchorLeft → AnchorTop change in newErrorHandlerFlow — rationale not documented
The change from AnchorLeft to AnchorTop as the destination connection index for error handler flows is presumably because error handlers enter the destination node from above rather than from the side. This is a visual Studio Pro layout change. A one-line comment explaining why would help future readers understand whether this is load-bearing or cosmetic.
5. hasReturnValue bool is redundant
fb.returnType is already stored, and fb.hasReturnValue is always assigned as returns != nil && returns.Type.Kind != ast.TypeVoid. Callers can compute this from fb.returnType without the extra field. Not a correctness issue, but it adds a third way to answer the same question alongside fb.returnValue != "".
Positive Notes
- The deduplication of the 10 copy-pasted error-handler blocks in
builder_actions.goandbuilder_calls.gointofinishCustomErrorHandleris a clear improvement. - 5 focused unit tests in
cmd_microflows_builder_terminal_test.gowith theflowPathExistsBFS helper cover the key routing scenarios well. - Bug-test file
349-custom-error-handler-routing.mdlhas thorough header documentation and correctly sets up the non-terminal handler scenario. - The
pendingErrorHandlerStatestruct cleanly separates the data from the builder fields, makingqueueActivePendingErrorHandlereasy to follow once the invariant is understood.
Summary: One blocker — the statementVarRefs/statementsVarRefs duplicate with PR #337 needs to be resolved before this can merge. The fix is a one-time extraction to a shared file. All other findings are moderate/minor documentation and style issues.
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor IssuesNone found. What Looks Good
RecommendationApprove the PR. It correctly fixes bug #349 with appropriate test coverage, follows project conventions, and introduces no regressions. The changes are focused, well-tested, and solve the specific problem described in the issue. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor IssuesNone found. What Looks Good
RecommendationApprove the PR. This is a well-scoped bug fix with adequate test coverage that resolves the reported issue without violating any project conventions or introducing risks. The changes follow existing patterns and maintain code quality standards. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
Symptom: the combined open-PR branch still produced an MPR check failure for a custom-error-handler path even though the described MDL round-tripped exactly. The handler rejoined before a DECLARE whose initial value referenced the failed action output, so Studio Pro reported the output variable as out of scope. Root cause: the combined branch was missing the already-submitted custom error handler fix that treats DECLARE initial values as output-variable references. That fix exists in PR mendixlabs#366 but had been lost during combined-branch conflict resolution. Fix: restore DECLARE initial-value reference tracking and the synthetic graph regression from the custom-error-handler PR. Tests: ran the targeted executor regression for output-dependent DECLARE routing.
Moves statementVarRefs / statementsVarRefs / referencedVariableSet / expressionStringVarRefs out of cmd_microflows_builder_graph.go into cmd_microflows_builder_varutil.go. Sibling builder passes (PR mendixlabs#366's pending error-handler queue, future planners) need the same walker without redeclaring it in their own file. Pure refactor — no behavior change. Also marks the variable-alias-collision proposal as Implemented now that the feature is shipping. Addresses ako's review on PR mendixlabs#337. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
hjotha
force-pushed
the
submit/custom-error-handler-routing
branch
from
d66c0ca to
3bf45d4
Compare
AI Code ReviewWhat Looks Good
RecommendationApprove the PR. The fix correctly resolves the reported issue with appropriate test coverage and maintains code quality standards. No changes are needed to the PR. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
Moves statementVarRefs / statementsVarRefs / referencedVariableSet / expressionStringVarRefs out of cmd_microflows_builder_graph.go into cmd_microflows_builder_varutil.go. Sibling builder passes (PR mendixlabs#366's pending error-handler queue, future planners) need the same walker without redeclaring it in their own file. Pure refactor — no behavior change. Also marks the variable-alias-collision proposal as Implemented now that the feature is shipping. Addresses ako's review on PR mendixlabs#337. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Review follow-up status on the current head:
Local validation after the update was green: make build
make test
make lint-go |
|
Review follow-up status:
Local validation after the update and latest conflict resolution: make build
make test |
PR #366 Review — fix: preserve custom error handler continuationsSubstantial and well-structured fix. The queued pending-state approach ( Moderate1. The function handles: Missing from the switch: Impact: if a THEN or ELSE branch returns and contains e.g. a 2.
Both functions should cover all 16 statement types. The fix is mechanical — add the missing cases; most can delegate to Minor
Changing the destination connection index for error handler flows is a BSON change that affects how Studio Pro renders the connection. The PR validation section only lists
Summary
|
hjotha
force-pushed
the
submit/custom-error-handler-routing
branch
from
8be98db to
fa45c7c
Compare
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor Issues
What Looks Good
RecommendationApprove. This is a well-focused bug fix that properly addresses the issue through the full pipeline with adequate test coverage. The changes are minimal, targeted, and maintain consistency with the existing codebase architecture. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
Symptom: all open PRs against main failed the shared build-and-test job in make test-integration, even when their local build/test/lint validation passed. The failures reproduced on origin/main, so they were baseline CI instability rather than PR-specific regressions. Root cause: TestWatcherDebounce could allow stale timer callbacks to send an extra message under slow scheduling, nanoflow integration fixtures used MDL syntax that no longer matched the grammar, and the doctype mx-check harness did not classify known page/nanoflow showcase consistency errors as expected limitations. Fix: guard watcher debounce callbacks with a generation counter, tighten the watcher burst test, update nanoflow fixtures to current MDL syntax, and extend the known consistency-error allowlist for showcase-only limitations. Tests: make build Tests: go test ./cmd/mxcli/tui -run TestWatcherDebounce -count=20 -v Tests: ./bin/mxcli check mdl-examples/doctype-tests/02b-nanoflow-examples.mdl Tests: go test -tags integration -count=1 -timeout 30m ./mdl/executor -run 'TestRoundtripNanoflow_(Loop|EnumParameter|Annotations)|TestMxCheck_DoctypeScripts/02b-nanoflow-examples.mdl|TestMxCheck_DoctypeScripts/03-page-examples.mdl' -v Tests: make test Tests: make lint-go Tests: make test-integration
Symptom: custom error-handler bodies that did not explicitly return were rebuilt as detached terminal paths, while empty custom handlers could either lose their error flow or rejoin into statements that read an output variable that is absent on the error path. Root cause: the microflow builder handled custom error handlers immediately at the source activity. It did not retain pending handler state until the next safe continuation was known, and a later handler could overwrite an earlier pending handler. Fix: queue pending custom handler state, route non-terminal handlers through the next safe continuation via a merge, preserve empty custom-handler flows, and terminate output-producing handlers before output-dependent continuation statements in void microflows. Tests: added synthetic builder regressions for non-terminal handler rejoin, consecutive pending handlers, empty output-handler termination, and empty no-output-handler rejoin. Ran make build, make lint-go, and make test.
Adds an MDL script under mdl-examples/bug-tests/ exercising a non-terminal custom error handler on a microflow call followed by a continuation call. After exec, `mx check` reports 0 errors, confirming the handler tail rejoins the continuation instead of becoming a detached terminal path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Symptom: a custom error handler for an output-producing action could rejoin before a later DECLARE whose initial value referenced that output variable. Studio Pro then rejected the roundtripped microflow because the output variable is not in scope on the error-handler path. Root cause: skip-variable routing collected references from many statement kinds, but omitted DECLARE initial values. Fix: include DECLARE initial values in statement reference analysis so handlers wait for, or terminate before, output-dependent declarations. Tests: add a graph-level regression that verifies the error-handler path cannot reach an output-dependent DECLARE, plus the existing custom-handler routing tests via make build and make test.
Symptom: merging the custom error-handler routing PR with the variable-alias PR would redeclare statementVarRefs and statementsVarRefs in package executor. Root cause: both PRs added local helper names for different pre-passes while sharing the same package namespace. Fix: rename the custom error-handler helper pair to handler-specific names and add review-requested comments documenting the pending-handler state invariant, the bounded rejoin scan, and the top-entry anchor used for error-handler flows. Tests: make build; focused error-handler executor tests; make test.
Symptom: a custom error handler inside a returning IF branch could be forced to terminate when the branch success tail declared values from the failed action output. The resulting MDL inserted a return into the handler, and the generated MPR no longer matched the original control flow. Root cause: branch merge detection only treated empty custom handlers as needing a merge, and output skip-var routing stopped at the first output-dependent statement in void microflows. Derived variables declared from the failed output and SHOW MESSAGE template arguments were also invisible to the skip-var scan. Fix: treat non-terminal custom handlers as branch merge inputs, route pending handlers from returning branches to that merge, carry skip-var state across DECLARE-derived variables, and include SHOW MESSAGE expressions in handler reference analysis. Tests: added a synthetic branch-level regression covering a custom handler that skips an output-derived success tail and rejoins at a shared error continuation. Ran make build, make test, and make lint-go.
Symptom: after rebasing the custom error-handler routing PR, empty custom handlers on Java and nanoflow calls were still handled by the old body-only path, so output-dependent tails had no safe error-handler termination. Root cause: the rebase kept legacy per-action handler code for those call actions instead of the shared finishCustomErrorHandler helper used by the rest of the PR. Fix: route both actions through finishCustomErrorHandler so empty handlers, output skip variables, and non-empty handlers use the same pending-handler state machine. Tests: make build; make test; make lint-go.
Symptom: the custom error-handler builder stored both returnType and a redundant hasReturnValue boolean, leaving two sources of truth for whether the microflow returns a value. Root cause: hasReturnValue was assigned from returnType during graph construction and copied into nested error-handler builders even though the same answer can be derived directly from returnType. Fix: remove the boolean field, add a small hasDeclaredReturnValue helper, and use it at the custom-handler routing decision point. Tests: ran make build, targeted executor custom-error-handler tests, make test, and make lint-go.
Symptom: describe emitted IF statements inside custom error-handler blocks without the matching END IF, so exec rejected valid roundtripped handlers. Root cause: error-handler traversal linearized every reachable activity until the first merge and formatted nested splits as plain statements, losing block structure. Fix: traverse error-handler branches structurally, stop at the handler rejoin merge, and emit IF/ELSE/END IF around nested exclusive splits. Tests: added a synthetic error-handler graph with a nested conditional and ran make test.
Symptom: describing a custom error handler could drop an empty else branch from an inner decision, and exec then wrote a decision with no valid false case for Studio Pro mx check. Root cause: custom handler traversal skipped false flows that rejoined at the handler-local merge, matching normal describe cleanup but losing required branch metadata inside the handler graph. Fix: emit the else marker whenever the false flow exists, while still avoiding traversal through an empty false branch. Tests: extend the structured custom-handler traversal regression to assert the empty else is preserved.
Symptom: MDL with an explicit empty else branch could execute into a custom error-handler split whose false path had no condition, causing mx check to report a missing or invalid split condition after roundtrip. Root cause: the visitor represented IF statements only by branch bodies. An explicit `else` with no statements was indistinguishable from an omitted else, so the builder omitted the false continuation that Studio Pro needs for that graph shape. Fix: add IfStmt.HasElse, set it when the source contains ELSE, and have the builder and MDL re-emitter preserve that explicit empty branch. Tests: added visitor coverage for parsing empty ELSE and executor coverage that the false path reaches the following return; ran make build && make test.
Symptom: custom error handlers containing `if X then return ... else end if` wrote a false branch rejoin flow without a case value. Studio Pro reported the outgoing sequence flow from the decision as missing its false condition. Root cause: addErrorHandlerFlow kept only the deferred tail node ID from nested merge-less splits. It discarded nextFlowCase and nextFlowAnchor before the pending error-handler rejoin was materialized. Fix: carry the deferred tail case and anchor through pending error-handler state, and apply them when the handler rejoins the normal continuation. Tests: added synthetic coverage for a custom error handler with an explicit empty else rejoining through CaseValue=false; ran make build && make test.
Symptom: custom error handlers could be routed as if their output-dependent continuation were safe when the continuation used less common action types. Root cause: the continuing-handler detector and skip-variable reference walker only covered a subset of microflow statements with ErrorHandling and input expressions. Fix: centralize statement ErrorHandling lookup, cover every current action statement with ErrorHandling, extend variable-reference detection for action arguments, REST auth, mapping, transform, download, and validation feedback inputs, and remove the vestigial errorY parameter from the skip helper. Tests: add synthetic unit coverage for the supported action statement matrix and skip-variable input detection.
hjotha
force-pushed
the
submit/custom-error-handler-routing
branch
from
fa45c7c to
1a1524b
Compare
AI Code ReviewReview SummaryThis PR fixes custom error handler continuation in microflows (issue #349) by queuing pending error handlers until the next safe continuation is known, instead of processing them immediately. The changes are focused and well-tested. Critical IssuesNone found. Moderate IssuesNone found. Minor Issues
What Looks Good
RecommendationApprove. The PR correctly addresses the bug with a well-designed solution, appropriate test coverage, and clean implementation. Please update the symptom table in Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
|
CI stabilization note: merged #412 into this branch so it uses the same integration-CI baseline as the rest of the active PR set. |
Symptom: CI failed in TestMxCheck_DoctypeScripts/empty_java_action_argument.mdl because a Java action declared as RETURNS Void was written as an entity return type named .void, and Studio Pro reported CE1613. Root cause: the generic data-type visitor treats bare qualified names as entity/enumeration references. Java action return types reused that generic path, so the explicit Void spelling became a qualified name instead of ast.TypeVoid. Fix: add a Java-action return-type wrapper that maps unqualified Void to ast.TypeVoid while leaving generic data-type parsing unchanged for parameters and attributes. Tests: added visitor coverage for explicit RETURNS Void; verified mxcli check for the doctype fixture and the targeted integration subtest that failed in GitHub Actions.
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor Issues
What Looks Good
RecommendationApprove the PR. The changes correctly fix the reported bug with appropriate test coverage and minimal scope creep. The nanoflow examples simplification, while slightly out of scope, doesn't undermine the fix's validity and appears to maintain equivalent test coverage in a more concise form. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
Review (updated): fix: preserve custom error handler continuationsThe previous blocker — Minor —
|
3 participants
Closes #349.
Part of #332.
Summary
Validation
make buildmake lint-gomake test