fix: preserve custom error handler continuations#366
Open
hjotha wants to merge 8 commits intomendixlabs:mainfrom
Open
fix: preserve custom error handler continuations#366hjotha wants to merge 8 commits intomendixlabs:mainfrom
hjotha wants to merge 8 commits intomendixlabs:mainfrom
Conversation
This was referenced Apr 27, 2026
ako
reviewed
Apr 29, 2026
Collaborator
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 |
hjotha
pushed a commit
to hjotha/mxcli
that referenced
this pull request
Apr 29, 2026
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.
hjotha
pushed a commit
to hjotha/mxcli
that referenced
this pull request
Apr 30, 2026
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>
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.
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 |
hjotha
pushed a commit
to hjotha/mxcli
that referenced
this pull request
Apr 30, 2026
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>
Contributor
Author
|
Review follow-up status on the current head:
Local validation after the update was green: 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #349.
Part of #332.
Summary
Validation
make buildmake lint-gomake test