fix: always emit merge for continuing inheritance split branches (CE0079)#475
fix: always emit merge for continuing inheritance split branches (CE0079)#475hjotha wants to merge 10 commits intomendixlabs:mainfrom
Conversation
hjotha
force-pushed
the
submit/inheritance-split-always-emit-merge
branch
2 times, most recently
from
9360f12 to
e5053c8
Compare
AI Code ReviewLet me analyze this PR against the review checklist. First, let me understand what this PR does based on the provided information: The PR is titled "fix: always emit merge for continuing inheritance split branches (CE0079)" From the body:
This PR is stacked on #365 which introduced Now, let me go through the review checklist: Overlap & duplication
MDL syntax design (for new or modified MDL syntax)The PR adds new MDL syntax:
Let me check against the guidelines:
Full-stack consistency (for MDL features)For new MDL features, they must be wired through the full pipeline:
Wait, I need to check if the visitor changes are missing. Let me look for visitor-related changes in the diff... Scanning through the diff, I don't see any changes to Actually, let me double-check. The PR is about fixing an existing implementation (it says it's patching a no-merge shortcut added in the same function as #365). So maybe the visitor was already added in #365? Looking at the PR description: "Depends on Stacked on #365 ( So #365 likely added the visitor and basic implementation, and this PR is fixing a bug in that implementation. That would explain why we don't see visitor changes here - they were in the prerequisite PR. Let me verify this by checking if the visitor changes would be in #365. Since we don't have access to #365, I'll have to assume based on the description that the visitor was added there. Test coverage
Security & robustness
Scope & atomicity
Code quality
Bugs & correctness
Let me now check specifically for the visitor concern. Since the PR says it depends on #365 which introduced Looking at the changes in func (fb *flowBuilder) addInheritanceSplit(s *ast.InheritanceSplitStmt) model.ID {
if len(s.Cases) == 0 && len(s.ElseBody) == 0 {
split := µflows.InheritanceSplit{
BaseMicroflowObject: microflows.BaseMicroflowObject{
BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())},
Position: model.Point{X: fb.posX, Y: fb.posY},
Size: model.Size{Width: ActivityWidth, Height: ActivityHeight},
},
ErrorHandlingType: microflows.ErrorHandlingTypeRollback,
VariableName: s.Variable,
}
fb.objects = append(fb.objects, split)
fb.posX += fb.spacing
return split.ID
}
return fb.addStructuredInheritanceSplit(s)
}And then there's the main implementation in For the visitor to work, we would need to see something in Let me check if there are any obvious missing pieces... Actually, looking at the executor changes, I see in case *ast.InheritanceSplitStmt:
return fb.addInheritanceSplit(s)This shows that the AST node Now let me check against the specific checklist items mentioned in the instructions: Overlap & duplication
MDL syntax design
Full-stack consistencyBased on the diff and the PR description depending on #365:
Test coverage
Security & robustness
Scope & atomicity
Code quality
Let me check one more thing - the checklist mentions "New backend methods on the interface" for backend abstraction compliance. Looking at the executor code, I see it's using Also checking for "No Let me also verify the fix makes sense by reading the explanation: "Previous code skipped the merge when exactly one non-split branch continued and wired the parent's next statement directly to that branch's tail — but re-describe then buried the parent continuation inside the case body, and Studio Pro rejected the terminating branches' outgoing flows with CE0079 ("condition value should be configured for an outgoing flow") because they had no merge to converge on." The fix: "Always emit an ExclusiveMerge when at least one branch continues." This makes sense - you need a merge point for the flow to continue properly after the split. Now, let me formulate my review. I notice that the PR modifies several files that are not directly related to the core fix but are part of implementing the full feature (since it's stacked on #365). However, since the PR description says it depends on #365 and is patching the no-merge shortcut, it's reasonable that it includes the full implementation if #365 was just introducing the function but not the full wiring. Actually, re-reading: "That PR introduces This suggests that #365 introduced the function Given that, and seeing that this PR does include grammar, AST, and executor changes, it's possible that #365 only introduced the function signature or a stub, and this PR is doing the full implementation plus the fix. But the PR says "Depends on Stacked on #365" and "Merge #365 first, then this branch merges cleanly on top." This is a bit confusing. Let me think about what makes most sense: Option 1: #365 added the full feature (grammar, AST, visitor, basic executor) including the problematic no-merge shortcut in Option 2: #365 introduced the Looking at the scope of changes in this PR, it includes:
This seems like too much for just a patch to a no-merge shortcut. It looks like this PR is actually implementing the full feature, with the fix being part of it. But the PR description says it's patching the no-merge shortcut added in the same function as #365. Let me look at what #365 might have been based on the name: And this PR is about inheritance split and cast statements. Given the evidence, I think what happened is:
However, the review instructions say to review this PR as submitted, so I should evaluate what's in this diff. Looking at the changes, this PR does appear to be implementing the full feature:
And within that implementation, it fixes the no-merge shortcut issue. Given that, let me evaluate it as a new feature implementation. For full-stack consistency:
The missing visitor changes are a potential issue. But since the PR depends on #365 which supposedly introduced Let me double-check if there are visitor changes I missed... After reviewing the diff again, I don't see any changes to However, given the PR description that it depends on #365 which introduced the function, I'll assume the visitor changes are in #365. For the purpose of this review, since I have to evaluate this PR as is, and the visitor changes aren't here, I should note this as a potential issue unless I can confirm they're in the dependency. But the instructions say: "Review against ALL of the following checklist items" and I need to structure my review. Let me see if I can find any visitor-related changes by searching for "visitor" in the diff... I don't see any. The directories changed are:
No Given that the PR says it depends on #365 which introduced For my review, I'll proceed under that assumption, but note that if reviewing in isolation, the visitor would be missing. Actually, let me think differently. The PR modifies case *ast.InheritanceSplitStmt:
return fb.addInheritanceSplit(s)This means the AST node Since the AST node is defined in this PR (in ast_microflow.go), and the executor handler is in this PR, the visitor must also be in this PR or in #365. Given the dependency on #365, it's in #365. Okay, with that assumption, let's finalize the review. What looks good:
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: type-based microflow decisions and cast actions could be read from MPRs but had no first-class MDL representation, so describe/exec round-trips could not preserve InheritanceSplit and CastAction graphs. Root cause: the microflow AST, grammar, visitor, builder, describer, validator, and MPR writer only modeled boolean exclusive splits and regular actions. InheritanceCase sequence flows and CastAction BSON were not emitted back to valid project data. Fix: add split type and cast statements, parse and build inheritance branches, describe existing InheritanceSplit graphs by resolving case entity names, serialize inheritance split/case/cast BSON, and recurse through type-split bodies in validation/reference/layout code. Tests: added parser, builder, describer, terminality, validation, and MPR writer regressions plus a doctype fixture checked with mxcli check. Also ran make build, make lint-go, and make test.
Symptom: an inheritance split branch containing a nested empty-then decision could lose the boolean case value on the continuation flow when the branch joined a shared split merge. Root cause: addStructuredInheritanceSplit consumed flowBuilder.nextConnectionPoint from the nested decision but did not carry flowBuilder.nextFlowCase through branch tail wiring. Fix: preserve the pending case value on inheritance branch tails and reapply it when connecting to the next statement, parent continuation, or shared merge. Tests: add a builder regression for nested empty-then inheritance branches that must keep CaseValue=true on continuation flows.
Symptom: type split cases with equivalent empty/fall-through bodies could be reordered after describe/exec/describe. Root cause: inheritance split case flows did not carry a stable ordering signal when multiple cases shared the same split-to-merge shape, so the describer could only rely on connection metadata that was identical across those branches. Fix: encode the parsed case order into valid split flow connection pairs and sort inheritance split flows by that encoded order during describe. Tests: added traversal coverage for shuffled stored inheritance case flows that must still describe in the original authoring order; existing inheritance split builder and cast coverage continues to pass.
Symptom: inheritance split roundtrips could lose branch flow anchors or collapse an explicit empty ELSE branch into an untyped flow. Root cause: inheritance branch building did not thread statement anchor metadata through split-to-body and body-to-merge flows, and empty ELSE tails used a plain sequence flow instead of an explicit inheritance case. Fix: propagate authored branch anchors across inheritance branch body flows and keep empty ELSE branches represented by an explicit empty InheritanceCase. Tests: added builder coverage for anchored inheritance branch bodies and tightened the existing case-flow assertion to select the intended typed branch.
Symptom: describing a microflow with an inheritance split inside an if could emit the parent continuation inside the matching split case. Re-executing that MDL made variables declared in the continuation branch-scoped, so Mendix mx check reported invalid or missing return/variable state. Root cause: nested inheritance split emission stopped branches only at the split's own merge. When the inheritance split had no merge because one branch returned and the other fell through to the parent if merge, branch traversal used an empty stop ID and consumed the parent tail. Fix: when emitting an inheritance split, prefer the split's own merge but fall back to the caller's stop ID. This keeps parent continuation statements outside the split cases while preserving standalone inheritance split behavior. Tests: added a synthetic nested if/split-type traversal regression that verifies the parent tail is emitted after both end split and end if.
addStructuredInheritanceSplit took a no-merge shortcut when exactly
one non-split branch continued and the rest terminated — it wired
the parent's next statement directly to that branch's tail.
Two failure modes followed:
1. Re-describe emitted the parent continuation inside the case
body (the post-split statement was visually buried inside the
first case).
2. Studio Pro raised CE0079 ("condition value should be configured
for an outgoing flow") on terminating branches because their
cases had no merge to converge on.
With this change any surviving branch causes a merge to be emitted,
so the parent always resumes after the split.
Test: new TestInheritanceSplitAlwaysEmitsMergeWhenBranchContinues
asserts that a one-case + terminating-else split still creates
the ExclusiveMerge.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
hjotha
force-pushed
the
submit/inheritance-split-always-emit-merge
branch
from
e5053c8 to
2fcaea0
Compare
|
CI stabilization note: merged #412 into this branch so it uses the same integration-CI baseline as the rest of the active PR set. |
AI Code ReviewCritical Issues
Moderate Issues
Minor Issues
What Looks Good
Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
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 correctly fixes the CE0079 issue by ensuring inheritance splits always emit a merge when branches continue, adds complete MDL support for inheritance split and cast statements through the full pipeline, and includes appropriate tests and documentation. The minor issues noted do not block approval. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
2 participants
Summary
addStructuredInheritanceSplitused to take a no-merge shortcut when exactly one non-split branch continued and the rest terminated — it wired the parent's next statement directly to that branch's tail. That caused re-describe to emit the parent continuation inside the case body, and Studio Pro raised CE0079 ("condition value should be configured for an outgoing flow") on terminating branches because their cases had no merge to converge on.ExclusiveMergeand wire surviving tails through it. Parent continuation then resumes after the merge as expected.Depends on
Stacked on #365 (
submit/microflow-inheritance-split-cast). That PR introducesaddStructuredInheritanceSplit; this PR patches the no-merge shortcut added in the same function. Merge #365 first, then this branch merges cleanly on top. The PR is opened againstmainbecause#365lives on a fork branch.Test plan
go build ./...andgo test ./mdl/executor/ -count=1passgo build -tags integration ./...builds cleanTestInheritanceSplitAlwaysEmitsMergeWhenBranchContinuesasserts the merge is created when one branch continues and the rest terminate🤖 Generated with Claude Code