Sync ako/mxcli: check-time fixes for #1100–#1104, and describe → exec fidelity - #1116
Merged
Merged
Conversation
A test block is a microflow body — that is what the runner turns it into —
and `mxcli check` and the LSP were handing the file to the top-level
grammar instead. DECLARE is not a top-level statement, so the parser
resynced; RETRIEVE is a non-reserved keyword, so it was swallowed as an
identifier; and the leftover `FROM …` started an OQL query, whose follow
set is {GROUP_BY, SELECT, HAVING}. The reported error therefore told the
author their RETRIEVE needed a SELECT — on a statement
`mxcli syntax microflow.retrieve` prints as its own example
(mendixlabs#1103).
Not a corner case: the VS Code extension binds MDL to `.mdl`, which
`.test.mdl` matches, so every test file open in the editor was a wall of
squiggles. 9 of this repository's 10 test files reported errors this way,
one of them 392; all 10 now report 0.
testrunner.CheckSource renders each block as the microflow it becomes,
padded so the body keeps its SOURCE line numbers — wrapper fragments go
on the lines the doc comment and the '/' separator occupied, so a
diagnostic's line:col is the author's with no mapping table to drift.
Everything downstream then applies unchanged: an uncompilable body, an
unusable @expect or @verify (MDL-TEST01), and the semantic rules.
`make check-mdl` sweeps test files too, which is what keeps this true;
`.fail.test.mdl` names one whose annotations are deliberately unusable,
and the two existing fixtures of that kind are renamed to match.
Control: stubbing IsTestFile back to false reproduces the reporter's
message verbatim, `line 6:76 mismatched input 'LIMIT' expecting
{GROUP_BY, SELECT, HAVING}`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017s476QkXr9CFMvKspVzcvu
`retrieve $x from Mod.E … limit 1` is compiled to Mendix's "First object" range, so $x is an OBJECT and not a one-element list. That is deliberate and documented — but nothing between the author and mxbuild said so: `check --references` passed, and DESCRIBE re-emits `limit 1`, so an object retrieve and a list retrieve are identical MDL text. The first sign was CE0097 "The selected 'x' variable must be of type List" at the far end of a build, and inside a .test.mdl file not even that — the injected test simply failed to build (mendixlabs#1103). MDL-RETRIEVE01 tracks the variables a limit-1-no-offset retrieve binds, in statement order so a rebinding clears them, and flags a later list use — list operation, aggregate, loop, ADD/REMOVE. Keyed on exactly the condition the writer uses, because a check that disagrees with the writer it describes is worse than no check. The confusion is structural rather than careless: the same word means the opposite on `import from mapping`, where `first` binds the object and `limit 1` a one-element list. The message therefore names both working spellings instead of only refusing. Behaviour is unchanged; only the silence is fixed. Measured on the whole mdl-examples corpus (558 scripts, `make check-mdl`): no new failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017s476QkXr9CFMvKspVzcvu
Two halves of mendixlabs#1104, both about a failed `mxcli test` run telling the reader nothing and then poisoning every later run. MxBuild puts the same text in Message for every failing build, so "build failed: The project cannot be deployed, because it contains errors." cannot distinguish "your test does not compile" from "an unrelated document is broken". The parsed problems were in hand and discarded one line before they were needed: --attach and every --watch rebuild built their error with fmt.Errorf, while the attribution that turns a build error into the failing test's row was wired only into the --local boot. Both now return a *docker.BuildFailedError and go through one shared resultsForBuildFailure, so an error in a generated test microflow becomes that test's ERROR row and one in the project names its document. The hint no longer repeats the errors the error already renders, and names a leftover generated flow with the DROP that removes it. Cleanup now removes every generated MxTest.Test_* the project holds rather than the current suite's. The names are positional — Test_test_1, _2, … from the test's index in its file — and every test file reuses them, so a run with fewer tests than the last one left the surplus behind; under --attach the MxTest module always pre-exists (the dev loop installed it), so the whole-module drop never fired. One leftover that does not build then failed every subsequent run of every test file. Keying on the suite was wrong the other way too: it issued DROP for flows a part-way injection never created, and those failures made cleanup announce "the project has been left modified" for a project it had just cleaned. What cleanup genuinely cannot remove is now named, with its DROP. Measured end to end: with a planted bad MxTest.Test_test_2, a known-good one-test suite failed and left it in place before, and after the fix run 1 fails and cleans while run 2 passes. Reverting buildFailure() to fmt.Errorf reproduces the reported sentence verbatim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017s476QkXr9CFMvKspVzcvu
CLAUDE.md gains the `limit 1` idiom, because it is a cardinality change with identical source text on both sides and LLM-generated MDL walks into it; and the test-loop bullet now carries the three things a failed run must do. MDL_QUICK_REFERENCE and `mxcli syntax microflow.retrieve` spell out which LIMIT forms bind an object and which a list, and name the opposite meaning the same word has on `import from mapping` — the contradiction is what makes the mistake reasonable, so saying it is worth more than the rule alone. The test-microflows skill gains "check a test file before you run it", which is now possible and much faster than a run. Three findings records: two in cmd/mxcli (the format a tool owns being handed to another tool's parser; deriving what to clean up from what exists rather than from what you meant to create) and one in mdl/executor (a clause that changes a variable's cardinality needs a check keyed on exactly the writer's condition). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017s476QkXr9CFMvKspVzcvu
163 files, 41,674 lines. This finishes docs/plans/2026-09-14-retire-legacy-engine.md. Zero importers was not the same as safe to rm -rf. Two live dependencies survived, and neither is visible to a check written against the parent package's import path. sdk/mpr/version had six importers, two of them shipping code (cmd/mxcli/docker/build.go and patch.go) — a subpackage is a different import path. And cmd/mxcli/docker/update_widgets_test.go read sdk/mpr/testdata/v1-project by filesystem path, an os.DirFS string rather than an import. So: before deleting a package, search for three things — its own import path, its subpackages' paths, and its directory as a literal string (testdata, go:embed, scripts). The last two are invisible to any importer census. The six importers went to mdl/types, not to modelsdk/mpr/version. sdk/mpr/version.ProjectVersion is `type ProjectVersion = types.ProjectVersion`, an alias, so types.ProjectVersion is the same type — while modelsdk/mpr/version declares a duplicate struct that would have been a different one. Read the declaration, not the name. The v1 fixture moved to modelsdk/mpr/testdata/. Everything was repointed and proven green with the package still present, which separates "the repoint was wrong" from "the deletion was wrong". Two measurements worth keeping. The shipped binary is identical in size before and after, so the linker had already dropped the package: this removes source weight, not runtime behaviour. And sdk/widgets fell to zero importers as a side effect but is deliberately kept — modelsdk/widgets/dirty_template_test.go reads sdk/widgets/templates/mendix-11.6 by path. Same trap, caught by grepping the directory name rather than the import. The import guard added in the previous slice is removed: with the package gone, an import is a compile error, strictly stronger than a test asserting it. Gates: build, vet (incl. -tags integration), go test ./..., check-mdl (560), check-findings, and the full repo-wide integration suite: exit 0, no failures. docker check and check --post-migration are byte-identical to the pre-deletion binary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018hifgRSawfaRWXS44YKtSJ
…tent A column has two Widgets-typed slots — `content` and `filter` — and the write path fills both. The reader took the FIRST widgets-typed property it met instead of keying on the resolved property key, and the writer emits column properties alphabetically, so `content` won and the filter was dropped. `describe → exec`, which is how a page is copied or edited, then deleted the filter, with mxcli check, exec and mx check clean at both ends. A filter-only column round-tripped by accident: with `content` empty the filter landed in the content list and was re-emitted in the column body, where the builder routes it back to the filter slot by widget type. That is why the common shape looked correct and only the combination broke. Route on the property key instead, keeping the first-wins heuristic only for a document whose keys do not resolve, and emit both lists in the column body. Measured on 11.6.6: the round trip now reports `Unchanged page` — the description reproduces the stored document — where it reported `Replaced page` and lost the filter before. Fixes #489. Reported upstream as mendixlabs#1111, where the describe gap read as a missing DataGrid2 capability; the capability is there — the same column renders checkbox cells and a working filter in a browser on 11.6.6. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G5JMoL3NegeVcn5SwSCqFK
Deleting sdk/mpr left the always-in-context routing document pointing at files that are gone. CLAUDE.md is where a session looks first, so a dead path there does not just go stale — it sends the next reader somewhere that cannot be opened, and every one of these was a routing instruction rather than prose. Eight sites. The architecture tree listed sdk/mpr's reader/writer/parser/utils and no modelsdk at all, which had it describing the deleted engine as the format layer and omitting the real one. The BSON storage-name procedure said to verify against sdk/mpr/parser_microflow.go; that now points at modelsdk/codec and modelsdk/gen. The write-choke-point list named both engines' files — there is one engine, so the list is now one entry and says so. The test-first checklist sent a parser test to sdk/mpr/ and a backend mutation test to mdl/backend/mpr/, neither of which exists. Useful Files offered sdk/mpr/parser.go and writer_widgets.go. Two checklist items were rules about a package that no longer exists, so they are restated as rules about the engine that does: "no sdk/mpr write imports in the executor" becomes "no engine internals in the executor" (modelsdk/mpr, modelsdk/codec, modelsdk/gen), with the note that a missing backend method gets implemented rather than bypassed — which is what the last five slices kept running into. The shared-types rule described sdk/mpr re-exporting aliases; it now states the rule directly and cites modelsdk/mpr/version.ProjectVersion as the cautionary case, since that one duplicates types.ProjectVersion instead of aliasing it and the two print under the same name. Every path the file now names was checked to exist. Docs under docs/ are deliberately untouched: they describe what the code used to do, and ADRs are immutable by convention. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018hifgRSawfaRWXS44YKtSJ
`ReadOnlyStyle:` parsed, passed check, and reached a writer that hardcoded "Inherit". DESCRIBE reads and emits the property, so describe → exec on a Studio Pro-authored page silently downgraded Control to Inherit. Not cosmetic: the value decides how a READ-ONLY check box renders — Text gives the words "Yes"/"No", Control gives the disabled checkbox glyph. In a DataGrid2 cell that is the whole of "show a Boolean as a checkbox", and it was unreachable from MDL. pages.CheckBox carries the value, the builder canonicalises it (Inherit / Control / Text, case-insensitive in, Mendix casing out) and refuses an unknown member — mxbuild tolerates one and Studio Pro then cannot open the project — and the codec writes Inherit when unset, so a script that never mentions it produces the document it always did. Verified on 11.6.6: the page mxcli writes from `ReadOnlyStyle: Control` is semantically identical to the hand-patched document that was checked in a browser (re-authoring it reports `Unchanged page`), renders 9 disabled checkbox cells, and the column's drop-down filter still cuts 9 rows to 3. `mx check` 0 errors; 562 MDL example scripts unchanged. Scope is the check box, matching what DESCRIBE reads back today. The other input widgets still write a hardcoded Inherit and are not read back either; the retired sdk/mpr serializer (no live callers) is left alone. Fixes #490. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G5JMoL3NegeVcn5SwSCqFK
Two claims in the syntax topic and the quick reference were wrong, and both pushed users away from a combination that works: - "A Boolean column takes no filter at all." The drop-down filter's own widget XML declares its attribute types as Enum AND Boolean, and a Boolean column filters Yes/No — measured in a browser on 11.6.6. - Neither said a column can hold a custom-content widget and a filter at the same time. `content` and `filter` are separate Widgets-typed slots with no dependency on `showContentAs`, so a read-only checkbox cell and a drop-down filter live in the same braces. Also documents `ReadOnlyStyle:` where it matters — it is what makes a read-only check box render as the glyph rather than the text Yes/No. The first claim is the likely origin of mendixlabs#1111, which asked Mendix for a new Boolean column type to do something mxcli could already do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G5JMoL3NegeVcn5SwSCqFK
`$n = COUNT(FILTER($reqs, $currentObject/Status = Mod.E.Approved))` passed `mxcli check`, execed with "Created microflow", wrote zero list properties, and then failed the build with CE0012 "The 'List' property is required." (mendixlabs#1101). MDL's expression grammar makes list operations look composable. Mendix's model is not: each is a separate ACTIVITY whose list is stored as a VARIABLE reference, with no slot for a nested computation. buildSetStatementNode took every list operand through extractVariableName, which reads *ast.VariableExpr and *ast.IdentifierExpr and returns "" for anything else with no default branch — so the inner call was discarded, list and predicate together, and the activity written with an empty AggregateVariableName. `describe` read the result back as `$n = count($)`. Measured on mxbuild 11.6.6, one microflow per project: count(filter(...)) CE0012 head/tail/filter(...) CE0096, the list-operation flavour of the same sum(filter(...), 1) CE0012 + CE0117 union($l, filter(...)) the SECOND operand lost the same way sort(filter(...), Name) mxbuild ABORTS with InvalidOperationException — the sort attribute resolves against the absent list's entity, so the document cannot be loaded at all. No error code, no line. count('nonsense') CE0012 — nesting is not required to lose the list The conversion now records what it could not reduce to a variable (ast.UnresolvedOperand) and MDL-LISTOP02 refuses it at check time, naming the operand and printing the two-statement rewrite. Keying on the conversion's RESULT rather than on the argument's node type is what makes it uniform: buildSetAggregate resolves an attribute path that extractVariableName cannot, so a node-type predicate would differ per arm and drift apart again — which that function's own comment already warns about. The switch moved into buildListOrAggregateStatement so all fourteen arms share one tail. Refusing rather than materialising an implicit variable covers every spelling from one rule, including the literal operand that no materialising would fix, and needs no project, so plain `mxcli check` reports it. Verified: - the printed remedy, pasted verbatim: check clean, exec clean, mxbuild 0 errors - the reporter's workaround and the nested form in ONE project: exactly 1 error - 584 corpus scripts: 1 flagged, and it is the new bug test (positive control) - guard stubbed: all 9 new tests fail, the accept-control stays green Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KTdvGwVewkdgNQGDCxpQPZ
`describe enumeration System.WorkflowActivityType` answered "enumeration not found" while `describe entity System.WorkflowActivityRecord` printed `ActivityType: Enumeration(System.WorkflowActivityType)` in the same session, so a System enum's values could only be guessed at until the build rejected a guess with CE1613 (mendixlabs#1102). `modelsdk/meta.SystemEnumerations` — all 15 enumerations with their values — had been in the tree since mendixlabs#889 with zero non-test consumers. The System module is not stored in the .mpr (the string `WorkflowActivityType` occurs in 0 of 370 mprcontents units), so its entities, associations and Java actions each have a Build* helper appended to a listing; the enumeration half had the data and no wiring. Adding `BuildSystemEnumerations` and appending it in `ListEnumerations` fixes four reported symptoms at once, since the catalog builder reads through the same backend: - `describe enumeration System.X` reports the values - `show enumerations` lists them (15 rows) - `check --references` no longer rejects an attribute typed against one — a false positive that blocked valid scripts, the mendixlabs#1071 direction of this gap - `CATALOG.attributes ⋈ CATALOG.enumerations` resolves 19 of 19 on the expr-checker fixture, where it resolved 0 Making them visible also made them addressable by the write paths, and the System module has no stored unit for a write to live in. `CREATE ENUMERATION System.BrandNewThing` already reported success and wrote a unit whose ContainerID was the synthetic module id 00000000-…-0001, present in no Unit row — an orphan with a dangling parent, measured at 369 → 370 units. So every enumeration write naming System is now refused before the backend is touched: create, alter, drop, move (both ends), rename, and `DROP MODULE System`, whose cascade would otherwise warn once per synthesized document. DESCRIBE emits `--` comment lines for these rather than a `create or modify` statement the guard would reject, as DESCRIBE BUILDING BLOCK does for the other read-only doctype. The system-module skill's enumeration section had drifted into wrong casing (`created`, `end`, `single`, `microflow`, `error`, `user`, `external`) and was missing three enumerations including WorkflowActivityState — so a developer copying `created` out of it hit the very CE1613 the section existed to prevent. Regenerated from the table and pinned by a test. The write-workflows skill documented the bug as a permanent limitation with a workaround; updated. Tests: the resolvability guard the entity half has had all along (TestModelerSystemEntities_HaveResolvableGeneralizations) now has its enumeration sibling. Each half of the fix was reverted to confirm its tests fail with the reported symptom, and the doc guard was checked against the original drift. Controls cover stored enumerations still listing, resolving by ID, user enumeration writes still working, and a user enumeration still describing as re-executable MDL. Not fixed, filed separately: `search` still does not match these (its index is built from value captions, and the meta table carries names only — defaulting a caption to the value name would be inventing text indistinguishable from a developer's own), and enum-split CASE values are validated against the enumeration for no module, user or System. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHM1in4g85vBtHnbjSejSd
Delete sdk/mpr — the legacy engine is gone
…s#1100) `$out = $out + $r/Status` inside `LOOP $r IN $reqs` — an Enumeration concatenated into a String — passed `check --references`, was written by `exec`, and failed the build with CE0117 at the Change variable activity, while the same mistake on a parameter was refused as E004. The report reads as "the checker is skipped inside a LOOP body". It is not, and the control says so: `'status=' + $T/Status` on a *parameter*, written one line inside the loop, was refused before this change. The body was always walked; the variable scope had two holes, and both had to close before the reported script reported anything. 1. `buildVarEntityScope` recorded CREATE, database RETRIEVE and parameters but never `LoopStmt.LoopVariable`, so `$r/Status` resolved to no attribute and inferred KindUnknown — which every rule tolerates by design. 2. `CheckAdapter` never set `Context.Scope` at all, so a `DECLARE $out String` was Unknown too. E004 needs BOTH operands typed, so closing (1) alone still reported nothing. That is also why `$out = $out + $Req/Status` was silent with no loop in sight; the report's case A hides it by putting a literal on the left. The list sources a loop can iterate are typed with it: a database retrieve, an association retrieve (far end resolved through the association index, which is why parameters are seeded before the body walk), a CREATE LIST, and the list operations that carry their input's element type through. Two other block-scoped positions the report asked about are covered: an ON ERROR handler body, which was not walked at all, and a FIND/FILTER predicate, where `$currentObject` is bound to the element type of the list under test. A bare attribute name in a predicate still resolves to nothing — binding bare names would change what a bare identifier means everywhere in an expression. The DataTypeKind and ON ERROR tables now live once in adapters with mdl/executor delegating: `validate_member_refs.go` already typed loop variables from the list and the expression checker's walk did not, which is the drift these two copies invite. Measured: exec-then-type-check over 591 mdl-examples scripts gives 11 violations before and 11 after, same rules on the same lines. The sweep earned its keep — a first cut reported Mendix's STRING `find($Hay, $Needle)` as a non-Boolean predicate, because the visitor still builds it as a list operation and the flow builder disambiguates it later (ledger #63); a predicate is now checked only when the input list's element entity is known, applying the same disambiguation. Controls: each of the four scope sources reverted in turn fails a distinct test with the reported symptom (empty violations). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012af91Z2nhirBJyY9MLRJqp
Refuse a list operation nested inside another (MDL-LISTOP02)
fix(exprcheck): type LOOP variables and declared variables (mendixlabs#1100)
Keep a DataGrid2 column's filter on describe, and write a check box's ReadOnlyStyle
fix(enumerations): expose the System module's enumerations, read-only
AI Code ReviewReview SummaryThis PR removes the legacy
Critical IssuesNone found. Moderate IssuesNone found. Minor Issues
What Looks Good
RecommendationApprove. The PR successfully removes deprecated legacy Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
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
2 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.
Ten commits since the last sync, all bug fixes found by working reported issues against real projects and mxbuild. Every fix was measured end to end —
mxcli check→exec→mx check— with a control that proves the test detects the defect.mxcli checkcatches things that previously only mxbuild didEach of these had the same signature:
checkclean,execreporting success, and the failure only at build time — or later.MDL-LISTOP02,#1101).$n = COUNT(FILTER($reqs, …))dropped the inner call entirely — list and predicate — and wrote an activity with an empty List, failing the build with CE0012 / CE0096. Measured across every spelling:head/tail/filter/sort, both operands ofunion, and a bare string literal.sort(filter(…), Attr)was worse than the report — mxbuild aborts with anInvalidOperationExceptionrather than reporting an error, because the sort attribute resolves against the now-absent list's entity.LIMIT 1retrieve then used as a list (MDL-RETRIEVE01,#1103).limit 1compiles to Mendix's "First object" range, so the variable is an OBJECT.DESCRIBEre-emitslimit 1either way, so an object retrieve and a list retrieve are identical MDL text — CE0097 at the far end of a build was the first sign.#1100).$out = $out + $r/StatusinsideLOOP $r IN $reqs— an Enumeration concatenated into a String — passedcheck --referencesand failed with CE0117, while the same mistake on a parameter was already refused..test.mdlfile parses as the microflow bodies it is (#1103). The file was handed to the top-level grammar, soRETRIEVEwas swallowed as an identifier and the leftoverFROM …started an OQL query — the reader was told their retrieve needed aSELECT. Nine of this repo's ten test files reported errors that way, one of them 392.make check-mdlnow sweeps them.describe → execfidelityThis is how a page is copied or edited, so a property lost on read is a property deleted from the model, with everything clean at both ends.
contentwon and the filter was dropped.ReadOnlyStyleis written instead of a hardcoded constant. Not cosmetic: the value decides whether a read-only check box renders as the words "Yes"/"No" or as the disabled checkbox glyph.Read coverage
#1102).describe enumeration System.WorkflowActivityTypeanswered "enumeration not found" whiledescribe entityprinted that very type in the same session, so a System enum's values could only be guessed at until the build rejected a guess.Test runner
mxcli testrun says what mxbuild rejected, and leaves nothing behind (#1104). MxBuild puts identical text inMessagefor every failing build, so the error could not distinguish "your test does not compile" from "an unrelated document is broken" — the real errors were in hand and discarded. Cleanup now removes every generatedMxTest.Test_*: the names are positional and reused per file, so keying cleanup on the suite left flows behind whenever a later run had fewer tests than an earlier one, and one leftover that does not build fails every later run of every test file.Docs
Findings recorded for the above, plus corrected DataGrid2 column-filter guidance and CLAUDE.md pointed at the engine that exists.
Note on two commits.
Delete sdk/mpr — the legacy engine is goneandPoint CLAUDE.md at the engine that existsare already upstream under different SHAs (applied via#1114), sogit cherrymarks them as equivalent. They appear in the range but contribute no new content