Skip to content

Sync ako/mxcli: page round-trip fidelity, check/exec agreement, domain-model and security fixes - #1146

Merged
ako merged 22 commits into
mendixlabs:mainfrom
ako:main
Sep 21, 2026
Merged

ako merged 22 commits into
mendixlabs:mainfrom
ako:main

Conversation

@ako

@ako ako commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Ten commits from ako/mxcli:main that are not yet upstream. Each carries its
measurement in the commit body; the grouping below is by what was wrong, not by
file.

Two of these destroy or silently alter user work and are worth taking first.

Severe

  • A dropped entity could leave a project that will not LOAD. DROP ENTITY
    swept only the regular Associations collection, so every cross-module
    association pointing at the deleted entity survived. Dropping the local FROM
    end left a 16-byte pointer to an element that no longer exists, and mxbuild
    11.14.0 then failed in the storage layer, above the validation that produces CE
    codes — no CE number, no document named:

    ERROR: System.AggregateException … (The given key '49751a65-…' was not present
    in the dictionary.)  at StreamingBsonUnitReader.ResolvePostponedProperties()
    
  • A password field round-tripped into a plaintext text box. describe page
    exec over a Studio Pro page dropped six things with mx check at 0 errors on
    both sides, the one that matters being IsPasswordBox True → False. CLAUDE.md
    makes describe → rename → exec the copy operation, so copying a login or
    change-password page lost the masking silently.

Page round-trip fidelity

  • A page rewrite no longer moves state nobody asked to change. describe page
    exec over a Studio Pro page reported Replaced, not Unchanged, so
    ADR-0008's elision could not fire and the unit churned in version control on
    every re-run. Fourteen differences in four independent classes, all "the
    rebuild writes a constant where Studio Pro stores a value" — pageToGen
    hardcoded Autofocus, CanvasWidth and CanvasHeight, and CanvasWidth alone takes
    seven distinct values across the 67 pages measured.
  • Version-gated page header keys. Page.Autofocus (11.1.0),
    Page.Variables and Snippet.Variables (10.17.0) were written into older
    projects. Floors taken from the Model SDK's own StructureVersionInfo
    (mendixmodelsdk 4.115.0), not from release notes — follow-up to CREATE PAGE with Params: is refused on Mendix 10 — no way to create a parameterised page #1121, which
    fixed this in a page's parameters and left the header out of scope.

mxcli check agreeing with exec

  • A list widget's own row action is judged in the context it creates.
    MDL-PAGEARG01 refused datagrid dg (DataSource: DATABASE …, onClick: SHOW_PAGE M.Edit(Req: $currentObject)) with "not inside a data view, list view or grid
    row" — on a listview, contradicting its own wording. A list widget's onClick
    fires per row, so the row it renders is the context object. exec refuses a
    script whose check reports an error, so this was a blocker, not a warning: the
    reporting project could not apply its slice at all.
  • SET DataSource = DATABASE is refused instead of wiping the source. It
    passed check, printed Altered page … at exit 0, and left the data view with
    no usable datasource; the only other signal was CE7007 from mxbuild, naming the
    widget and never the statement. One mapping stood in for several — a DATABASE
    source has no single stored shape, the holding widget decides which element
    Studio Pro writes.

Security

  • No member access for the audit associations. An access rule on an entity
    carrying AutoOwner or AutoChangedBy made mxbuild report the whole module as
    CE0066 "Entity access is out of date", and UPDATE SECURITY — the documented
    repair for exactly that error — printed "Reconciled 1 access rule(s)" and left
    it standing, because it re-added the entry that caused it. Four lines on a
    clean production-security app reproduce it.

Lint

  • MPR012 reports the legacy image widgets (staticimage / dynamicimage),
    which the React client does not support. In the linter rather than check
    deliberately: describe → exec of a legacy page is a legitimate lossless
    operation that a check warning would flag every time. Marketplace modules are
    excluded, so it stays off Studio Pro content the reader cannot fix. Also
    corrects a version claim carried in three places — the React client was added
    in 10.7, so CE0582 applies from there, not "in Mendix 11".

One net-zero pair, kept for honesty

3ac6df51 and its revert c2755dfe. The hypothesis — that CodeQL read an
inline w["IsPasswordBox"] map index as a credential lookup — was wrong: the
re-run produced a byte-identical alert, and running the query locally showed the
source is the generated property descriptor in modelsdk/gen. The alert
reproduces on unmodified main, so it was not that branch's to fix. Net diff is
nothing.

claude and others added 22 commits September 20, 2026 12:59
…he source

`alter page … { set DataSource = DATABASE Mod.Entity on dvCust; }` passed
`check`, printed `Altered page …` with exit 0, and left the DataView with no
usable datasource: `describe page` rendered the widget with the property gone,
and the only other signal was CE7007 from mxbuild — naming the widget, never
the statement that broke it.

One mapping stood in for several. A DATABASE source has no single stored shape;
the widget holding it decides which element Studio Pro writes
(Forms$ListViewXPathSource on a list view, CustomWidgets$CustomWidgetXPathSource
on a pluggable widget, Forms$GridXPathSource on a grid). A data view has no
database form at all — it binds to one object — which is why CREATE PAGE's
dataViewSourceToGen already refused that pairing while SET wrote it silently.
serializeDataSourceBson instead emitted a Forms$DataViewSource (the "data from
context" source) with the entity in EntityRef and SourceVariable left null,
which is neither shape and is why the describe reader, needing a SourceVariable,
rendered nothing.

The setter now refuses a database source, dispatching on the stored widget's
$Type for the remedy: a data view is told which sources it can take (REPLACE
would be a dead end there — CREATE PAGE refuses the same pairing), every other
widget is pointed at REPLACE, which reaches the real builder. Rebuilding the
shapes in the mutator would be a second copy of listViewSourceToGen in raw BSON,
the duplicate-resolver drift CLAUDE.md warns about.

The refusal lives once, in the mutator, so `check -p --references` — which
dry-runs the setter against a pagemutator.Probe() copy — and `exec` cannot
disagree.

Measured on two copies of a real Mendix 11.13.0 app:

  with the fault   check --references -> "Check passed!"
                   exec               -> "Altered page …", exit 0
                   describe page      -> dataview dvCust { … }  (no source)
                   mx check           -> 1 error, CE7007 at Data view 'dvCust'

  with the fix     check --references -> refused, exit 1
                   exec               -> refused, exit 1
                   describe page      -> datasource unchanged
                   mx check           -> 0 errors

Control: with the fix reverted, both new tests fail with the reported symptom
(the statement accepted; check reporting 0 errors), while the non-database
retypes mendixlabs#855 added keep passing.

The alter-page skill advertised the database form as supported; corrected, along
with the `mxcli syntax page.alter` help.

upstream mendixlabs#1032

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PSDurGmL9HgDdLhpMqZt6o
…pplies

Two halves of one concern: mxcli should not quietly help you author a widget
your app cannot build.

#518 and #538 made `staticimage` and `dynamicimage` round-trip, which was
right — a project being converted up already contains them, and the pre-fix
describe -> exec deleted them. But the same work made both materially easier
to author (Image:, DataSource:, DefaultImage:, thumbnail, enlarge all newly
reachable) while nothing warned the author. Measured: no validator mentions
either widget, so the only signal was CE0582 at the far end of a build.

MPR012 reports them. The linter and not `mxcli check`, deliberately: `check`
validates a script, and describe -> exec of a legacy page is a legitimate
lossless operation that a warning would flag every time — a rule that fires
on correct work is noise. `lint` audits the project, where "this page holds a
widget your client cannot render" is wanted once.

The marketplace exclusion comes free and is the part worth measuring:
ctx.Widgets() already filters any module with a Source, so the rule never
fires on the Studio Pro static images a blank app inherits from
FeedbackModule — content the reader cannot fix and an update would replace.
Measured on a blank 11.12.1 app carrying both widget kinds: 8 legacy image
widgets in the BSON, 7 indexed by the catalog, 5 in the user's own module,
and lint reported exactly those 5.

A deny-list of exactly two storage names, never an allow-list: the one widget
such a rule must never fire on is the pluggable Image — the replacement it
recommends.

Also corrects a version claim carried in three places, including two I wrote.
The reference guide says the React client was added in **10.7**:

    "The Dynamic Image widget, which is not supported by the React client
     added to Mendix in 10.7, can be converted to an Image widget through the
     context menu of the widget when the React client is enabled."

So CE0582 fires on 10.7+ wherever that client is enabled, not "in the Mendix
11 React client". A version boundary copied from a sibling comment rather
than from the vendor doc is the same class of error as a floor copied from a
proposal's sample output.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016JgBheMTV6UiQstQ2nLyay
describe page → exec over a Studio Pro-authored page reported "Replaced",
not "Unchanged", so ADR-0008's elision could not fire and the unit churned
in version control on every re-run. mx check was 0 errors either way.
Measured on ako/TestApp's Rules.RuleAction_NewEdit at 11.14.0: fourteen
differences, in four independent classes, all "the rebuild writes a
constant where Studio Pro stores a value".

1. Page header. pageToGen hardcoded Autofocus, CanvasWidth and
   CanvasHeight. Studio Pro varies all three per page — CanvasWidth takes
   seven distinct values across those 67 pages and the hardcoded 1200
   matched four, so a round trip moved the canvas of the other 63. Now
   carried from the stored document on a rewrite; a new page, which has no
   stored document, still gets the defaults.

2. Client-action defaults. save_changes, cancel_changes, close_page and
   delete_object never wrote DisabledDuringExecution, which Studio Pro
   stores true on all 39 of them; save_changes wrote SyncAutomatically
   true where all 8 store false.

3. AttributeRef.EntityRef, present on 338 of 338 stored refs (313 null, 25
   navigated), was emitted only on the navigated branch.

4. Forms$PageVariable: only the field carrying a value was set, so the
   other five keys were never marked dirty and the encoder omitted them.

3 and 4 go in the codec's TypeDefaults rather than at each construction
site — PageVariable is built in three places — which needed one new kind,
FalseFields, since a bool's zero value is never dirty.

Result on that page: 14 differences → 1. The remaining one is a pluggable
widget Object property, which is the CE0463 subsystem and needs its own
investigation.

Two things worth knowing, both learned the hard way here:

mxcli round-tripping its own output proves nothing about this class.
Measured: the MDL bug-test reports "Unchanged" on the unfixed build too,
because mxcli writes the page and mxcli describes it. The reference has to
be a Studio Pro document, which also limits the committed-fixture idea in
the issue. The detecting evidence is the Go tests, each run against a
stubbed-out fix.

The first version of the header carry used a .(int32) assertion — the
natural one, since the gen setter takes int32 — and matched nothing,
because Studio Pro stores both dimensions as int64. Its unit test passed
regardless, since the fixture wrote int32: the test encoded the assumption
under test. The read is now width-agnostic and the test asserts both.

Closes #541

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R
Follow-up to mendixlabs#1121, which fixed this defect in a page's
PARAMETERS and left the header out of scope.

Measured against the Mendix Model SDK's StructureVersionInfo records
(mendixmodelsdk 4.115.0):

  Page.Autofocus       11.1.0
  Page.Variables       10.17.0
  Snippet.Variables    10.17.0

All three were written unconditionally, so every page and snippet mxcli
created for a Mendix 10 project carried a key that project's metamodel does
not declare — the class that makes Studio Pro throw InvalidOperationException
at MprProperty.cs. mxbuild does not catch it: measured on 10.24.25, 0 errors
with the key present and 0 errors without it.

They arrive by two different routes, and only one is a gen property.
Autofocus is set through gen, so it is simply not set below 11.1. Variables
exists only because the codec's Studio Pro defaults registry emits it as an
empty typed-array marker, and a gen PartList has no "present but empty" state
to leave unset — so there is nothing to skip and the suppression has to be in
the encoder. That registry is global and keyed by $Type alone, so it cannot
see a project version: hence Encoder.OmitKeys, per-encode. The zero Encoder
suppresses nothing, so every other caller is unaffected.

Suppressing a key and dropping data are different things. The empty Variables
marker is safe to suppress; variables the script DECLARED are refused instead,
naming them and the floor (guard-don't-drop, ADR-0005) — silently dropping
them leaves widgets referencing names that are gone (CE1151) from a statement
that reported success.

CreatePage/UpdatePage and CreateSnippet/UpdateSnippet now share encodePage /
encodeSnippet, so the guard cannot be applied on one path and forgotten on the
other. ALTER PAGE needs nothing: the page mutator edits the stored raw BSON
and marshals it back, so it can never invent a key.

Verified on a real 10.24.25 project created with `mxcli new`. The page written
by this build has no Autofocus and keeps Variables (10.24 >= 10.17); the
snippet keeps Variables; `mx check` is 0 errors. The control is the same script
run by a pre-fix binary against a copy of that project, which writes Autofocus
— the only difference between the two documents. Unit tests carry a control
too: with both guards stubbed to the pre-fix behaviour they report the key
emitted at 9.24, 10.24.25 and unknown-version.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016aHj6mJwKCZD7EX7wcD6jW
fix(alter page): refuse SET DataSource = DATABASE instead of wiping the source
fix(lint): report the legacy image widgets, and correct when CE0582 applies
Stop a page rewrite moving state nobody asked to change (#541)
… three more

`describe page` → `exec` over a Studio Pro page silently dropped six things,
with mx check at 0 errors on both sides. The one that matters:

    …/Widgets/[1]/IsPasswordBox   True → False

a password field round-trips into a plaintext text box. CLAUDE.md makes
describe → rename → exec the copy operation, so copying a login or
change-password page lost it silently.

Measured on ako/TestApp's Administration.ChangePasswordForm at Mendix
11.14.0. Four different causes behind one symptom, which is why triage came
before any code:

- IsPasswordBox — the model and writer carried it; nothing parsed it and
  nothing emitted it.
- Validation — widgetValidationToGen() wrote a default EMPTY
  Forms$WidgetValidation over whatever was stored, on five widget types.
- ReadOnlyStyle — wired for CheckBox only. A DataView's draws no
  MDL-WIDGET07 warning because staticWidgetKnownProps is deliberately a
  union across widget types, so it passed check and was dropped anyway.
- PopupCloseAction — pageToGen wrote "" unconditionally.

Plus two typed-array markers: ParameterMappings is marker 2 on 220 of 220
stored lists in every parent type, and OutputMappings is present on 91 of
91 MicroflowSettings. An empty list needs MandatoryListMarkers, since
RegisterListMarker keys on a child element that is not there.

Three things measured rather than assumed, each of which would have been
wrong the obvious way:

A DataView's ReadOnlyStyle default is Control (47 of 56, never Inherit),
not the Inherit every other input widget uses.

The validation expression is emitted QUOTED, not bracketed. `[...]` is the
XPath-constraint spelling and propertyValueV3 parses it as an array, so the
builder saw []any and GetStringProp yielded "" — the emitter's own unit
test was green while the real round trip still lost the value.

PopupCloseAction is deliberately not carried from the stored document the
way the canvas properties are: it names a widget, and a rewrite rebuilds
the tree from the statement, so a carried name could dangle. DESCRIBE emits
it instead.

Result on that page: 17 differences → 9, and all 9 remaining are #549, a
separate carry problem. Verified at the artifact level — same project, same
script, only the binary differing: the pre-fix build turns both stored
password boxes into plaintext ones, the fixed build preserves them, and
mx check is 0 errors after round-tripping four pages.

Closes #550

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R
CodeQL's clear-text-logging query read `w["IsPasswordBox"]` as a lookup of a
credential and followed the boolean, field-insensitively, into every error the
page writer can return — surfacing as a high-severity alert at an unrelated
example's `fmt.Printf("Error creating page: %v\n", err)`. The value is a
design-time flag ("render this text box as a password field") and the logged
expression is an error, so the classification is wrong; the alert is new with
this branch because the inline map index is (PR #551 is the first to read that
key here — the same CodeQL check was clean on #542 and #546).

Reading the key through a helper takes the sensitive-looking literal out of the
index position. No behaviour change: the describe/builder round-trip tests for
Password are unchanged and pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R
This reverts 3ac6df5. The hypothesis behind it — that CodeQL read the inline
`w["IsPasswordBox"]` map index as a credential lookup — was wrong: the re-run
produced a byte-identical alert, and running the query locally shows the source
is `modelsdk/gen/pages/types.go:33658`, the `o.isPasswordBox` property
descriptor in the generated SetProperties slice.

The alert reproduces on unmodified main (codeql 2.27.0,
Security/CWE-312/CleartextLogging.ql, 6 results including this exact one), so it
is not this branch's to fix, and the accessor bought nothing. Keeping the PR
scoped to #550.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R
Conflict in page_write.go's UpdatePage: main's #541 added
carryStoredPageHeader (carry Autofocus/CanvasWidth/CanvasHeight off the
stored unit so a rewrite does not move state nobody asked to change), while
this branch routed both write paths through encodePage for the version
guards. Both are wanted, so encodePage grew a carry hook: CreatePage passes
nil (a new page has no stored document), UpdatePage passes the carry.

The merge also created a case neither side had alone. A pre-fix mxcli wrote
Autofocus into Mendix 10 projects, so on such a project the STORED value is
itself the defect — main's carry would faithfully preserve a key that
project's metamodel does not declare, making the repair a no-op. The carry is
now gated on the same floor: below 11.1 the stored value is dropped rather
than carried. That is repair, not data loss — there is no version of the
property the project can express.

carryStoredPageHeader takes the project version explicitly rather than
reading it off the backend, so the decision is testable against the 11.6.6
fixture with a synthetic 10.x version.

Verified on the real 10.24.25 project from this branch's earlier work, whose
page was written by a pre-fix binary and therefore carries Autofocus: after a
rewrite with the merged build the key is gone, Variables is kept, and
mx check is 0 errors. The new test carries a control — with the carry guard
stubbed it reports the key carried at 10.24.25, 11.0 and unknown-version —
and asserts the carry still works at and above the floor, so a failure below
it cannot be "the carry never works".

main's own header tests pass unchanged: their fixture is 11.6.6, above both
floors, so a new page there still gets Autofocus and the stored value is
still carried.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016aHj6mJwKCZD7EX7wcD6jW
fix(pages): stop writing 11.1/10.17 header keys into older projects
Stop describe losing a password field, its validation and three more (#550)
…eates

MDL-PAGEARG01 refused

    datagrid dgRequests (DataSource: DATABASE M.ServiceRequest,
      onClick: SHOW_PAGE M.Edit(ServiceRequest: $currentObject))

with "widget `dgRequests` is not inside a data view, list view or grid row".
A list widget's onClick fires PER ROW, so the row it renders IS the context
object — and on a `listview` the refusal contradicted its own wording. Since
exec refuses a script whose check reports an error, this was a blocker rather
than a warning: the reporting project could not apply the slice at all.

The mendixlabs#1029 guard judged every widget's own action in the context its PARENT
supplies. That is right for a button and wrong for the widget that establishes
the context. argContextForOwnAction draws the line where it belongs: a widget
binding a source of its own supplies the context for its own action. A source
in a shape the pass cannot read degrades to UNKNOWN, so the guard stands down
rather than refusing what it cannot prove is discarded.

Measured on mxbuild 11.14.0, in one fresh app, with the actions verified to be
stored (describe page) so the zero is not a dropped action:

    datagrid + DATABASE source + onClick($currentObject)   0 errors
    listview + DATABASE source + onClick($currentObject)   0 errors

Controls, all still refused: a foreign variable on a row action, mendixlabs#1029's
page-level button, and a button standing beside the grid rather than in it.
1029-showpage-arg-without-context.fail.mdl still exits 1; the two valid mendixlabs#1029
and #295 bug-tests still pass. Before the fix the new unit test fails with the
reported message verbatim.

The same mxbuild run exposed a separate defect, filed as #576 and deliberately
NOT written into the bug-test as a passing case: `DataSource: Mod.Car` on a
datagrid is silently dropped, so that widget is CE0488 plus a real CE1571.

Closes #552

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R
fix(pages): judge a list widget's own row action in the context it creates
…y end at

DROP ENTITY swept only the regular Associations collection, so every
CROSS-MODULE association pointing at the deleted entity survived. Dropping the
local BY-ID (FROM) end left a 16-byte pointer to an element that no longer
exists, and mxbuild 11.14.0 could then not LOAD the project:

    ERROR: System.AggregateException … (The given key
    '49751a65-d5f9-456c-887e-3f14bacb8822' was not present in the dictionary.)
      at StreamingBsonUnitReader.ResolvePostponedProperties()

No CE code and no document named — the failure is in the storage layer, above
the consistency checker, so it reads as "the project is corrupt". Dropping the
BY-NAME (TO) end is milder and still wrong: CE1613 at the cross-module
association.

removeCrossAssocsReferencing matches both ends, because a cross-module
association addresses them differently: FROM by element id (local to this
domain model), TO by qualified name (it lives in another module). Called from
DeleteEntity locally and in its cascade over the other domain models.

Reported against a VIEW entity, whose associations are derived from its OQL so
there is no CREATE ASSOCIATION to undo. Nothing here is view-entity specific:
the first probe — view entity and source entity in the SAME module — did not
reproduce, and that negative is what identified cross-module as the variable. A
plain `create association A.X from A.X to B.Y` plus `drop entity A.X`
reproduces the identical crash.

Measured on mxbuild 11.14.0, one project carrying all three shapes:

    before   drop by-id end    project does not load (exception above)
             drop by-name end  CE1613 at the cross-module association
    after    all three drops   project loads, 0 errors

Controls: an untouched cross-module association survives both deletes (in the
unit test and in the bug-test), and the single-module case still works. Before
the fix the new test fails in both directions with the orphan left behind.

Closes #553

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R
An access rule on an entity carrying AutoOwner or AutoChangedBy made mxbuild
report the whole module as CE0066 "Entity access is out of date" — and
UPDATE SECURITY, the documented repair for that error, printed "Reconciled 1
access rule(s)" and left it standing, because it re-added the entry that caused
it. Four lines on a clean production-security app reproduce it:

    alter entity Mod.Fab add attribute Owner: AutoOwner;
    update security;

mxcli wrote a MemberAccess for the implicit System.owner / System.changedBy
association. Mendix maintains those members itself and treats a rule naming one
as out of date. Measured on mxbuild 11.14.0, one entity, one rule, one variable
at a time:

    AutoOwner     + MemberAccess System.owner       CE0066
    AutoOwner     + no entry                        0 errors
    AutoChangedBy + MemberAccess System.changedBy   CE0066
    AutoChangedBy + no entry                        0 errors

So all four audit members follow one rule. The DATE half was already right
(issuetracker #20); the association half was assumed to be the opposite case
because Mendix really does add those two implicitly — the same inference the
earlier finding warned against in this very file ("ask mxbuild what it wants
instead of inferring symmetry").

Three parts, because two writers had to agree and a damaged project has to be
repairable:

  - the GRANT handler no longer adds the entry;
  - ReconcileMemberAccesses no longer adds it;
  - ReconcileMemberAccesses REMOVES a stored one, ahead of the foreign-module
    branch that would otherwise preserve it forever on the grounds that System
    is not loaded here. Without this, `update security` still could not repair
    what it exists to repair. Measured: CE0066 -> 0 errors on both damaged
    projects. It also clears a stale System.owner left behind when the flag is
    turned off again.

Exactly System.owner and System.changedBy, not every System.* reference: an
entity specialising a System entity legitimately inherits that module's real
associations.

Control: with the fix reverted the new test fails on all three flag
combinations, naming the entry and the CE code; the bug-test's audit-free
entity keeps its own attribute's member access, so a build writing no members
at all fails it too.

Closes #554

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R
fix(domain model): drop cross-module associations with the entity they end at
fix(security): write no member access for the audit associations
@github-actions

Copy link
Copy Markdown

AI Code Review

I need to review this pull request against the provided checklist. Let me analyze the PR content and check it against each section of the checklist.

First, let's understand what this PR does based on the description:

The PR contains ten commits from ako/mxcli:main that are not yet upstream. The body describes several issues that were fixed:

Severe Issues

  1. DROP ENTITY left cross-module associations: When dropping an entity, only regular Associations were swept, leaving cross-module associations pointing to deleted entity. This caused storage layer failures in mxbuild 11.14.0.
  2. Password field round-tripped into plaintext: describe pageexec dropped IsPasswordBox True → False, meaning password fields became regular text boxes.

Page round-trip fidelity

  1. Page rewrite moved state nobody asked to change: describe pageexec reported "Replaced" instead of "Unchanged" because pageToGen hardcoded Autofocus, CanvasWidth and CanvasHeight.
  2. Version-gated page header keys: Page.Autofocus (11.1.0), Page.Variables and Snippet.Variables (10.17.0) were written into older projects.

mxcli check agreeing with exec

  1. List widget's own row action judged in wrong context: MDL-PAGEARG01 refused datagrid dg (DataSource: DATABASE …, onClick: SHOW_PAGE M.Edit(Req: $currentObject)) claiming it's not inside a data view, list view or grid row - but on a listview, the row it renders IS the context object.
  2. SET DataSource = DATABASE refused instead of wiping source: It passed check, printed "Altered page …" at exit 0, but left data view with no usable datasource.

Security

  1. No member access for audit associations: An access rule on an entity carrying AutoOwner or AutoChangedBy made mxbuild report CE0066 "Entity access is out of date", and UPDATE SECURITY re-added the problematic entry.

Lint

  1. MPR012 reports legacy image widgets: staticimage/dynamicimage which React client doesn't support. Added to linter rather than check because describe → exec of legacy page is legitimate lossless operation.

One net-zero pair

  • Two commits that canceled each other out (hypothesis about CodeQL was wrong)

Now I need to review this against the checklist.

Let me go through each section of the checklist:

Overlap & duplication

  • Check proposals list for existing proposals covering same functionality
  • Check if diff introduces code that duplicates existing implementations
  • Check doctype test files for existing test coverage
  • Flag if PR re-documents already-shipped features as new

Looking at the changes:

  • Many new test files were added (mdl-examples/bug-tests/, mdl/backend/modelsdk/_test.go)
  • Several files were modified to fix specific issues
  • I don't see obvious duplication of existing functionality - these appear to be bug fixes

MDL syntax design (for new or modified MDL syntax)

The PR doesn't appear to add or modify MDL syntax itself - it's fixing existing functionality. There are no changes to grammar rules, AST nodes, etc. that would introduce new MDL syntax. The changes are mostly in backend implementation, tests, and linter rules.

Full-stack consistency (for MDL features)

Since this PR doesn't add new MDL features (it's fixing bugs in existing functionality), this section doesn't really apply. The fixes are to existing MDL commands (DROP ENTITY, describe page, alter page, etc.) but they're not adding new syntax.

Test coverage

  • New packages should have test files
  • New executor commands should have MDL examples in mdl-examples/doctype-tests/
  • Integration paths should be tested, not just helpers
  • No time.Sleep for synchronization

I see many new test files added:

  • mdl-examples/bug-tests/1032-alter-page-set-database-datasource.mdl
  • mdl-examples/bug-tests/domainmodel-553-drop-entity-cross-association.mdl
  • mdl-examples/bug-tests/page-header-version-floored-keys.mdl
  • mdl-examples/bug-tests/pages-541-roundtrip-property-drift.mdl
  • mdl-examples/bug-tests/pages-550-describe-input-properties.mdl
  • mdl-examples/bug-tests/pages-552-list-widget-row-action.mdl
  • mdl-examples/bug-tests/security-554-autoowner-access-rule.mdl
  • Many backend test files (access_rule_audit_member_test.go, domainmodel_delete_cross_assoc_test.go, page_header_carry_test.go, page_version_keys_test.go, page_zero_value_keys_test.go, widget_write_action_defaults_test.go, widget_write_input_props_test.go, cmd_pages_builder_input_props_test.go, cmd_pages_describe_input_props_test.go, mutator_datasource_test.go)

These look like proper bug test coverage for the issues being fixed.

Security & robustness

  • Command injection, XSS, SQL injection, temp file issues
  • Unix sockets should use restrictive permissions (0600)
  • File I/O should not be in hot paths
  • No silent side effects on typos
  • Correct method receivers

I don't see obvious security issues in the changes. The fixes appear to be correcting logic errors rather than introducing new vulnerabilities.

Scope & atomicity

  • Each commit should do one thing
  • PR should be scoped to a single feature or concern
  • Independent features should be in separate PRs
  • Refactors touching many files should be their own commit

The PR description says "Ten commits from ako/mxcli:main that are not yet upstream" and groups them by what was wrong, not by file. This suggests the commits are already atomic (each doing one thing), but they're being grouped in this PR. The body mentions "Two of these destroy or silently alter user work and are worth taking first." This suggests the PR contains multiple unrelated fixes.

Looking at the issues described:

  1. Severe issues (DROP ENTITY, password field)
  2. Page round-trip fidelity (two issues)
  3. mxcli check agreeing with exec (two issues)
  4. Security (one issue)
  5. Lint (one issue)

These seem like separate concerns that could arguably be split into multiple PRs. However, they're all bug fixes related to core functionality, and the PR description indicates they were developed together in a branch.

Code quality

  • Refactors should be applied consistently
  • Manually maintained lists should be flagged as maintenance risks
  • Design docs should match actual implementation

I don't see obvious refactors in this PR - it's mostly bug fixes. The changes look consistent with existing patterns in the codebase.

Bugs & correctness

  • Logic errors, race conditions, resource leaks
  • Error handling gaps

The PR description clearly identifies logic errors that are being fixed:

  • DROP ENTITY not handling cross-module associations
  • Password field not being preserved in describe/exec
  • Page rewrite moving state unnecessarily
  • Version-gated keys being written to older projects
  • List widget row action validation in wrong context
  • SET DataSource = DATABASE being incorrectly allowed
  • Access rule incorrectly handling audit associations
  • Legacy image widgets not being caught by linter

All of these appear to be legitimate logic errors being fixed.

Now let me check if there are any specific issues with the changes I can see in the diff:

From the diff snippets:

  1. In mdl/backend/modelsdk/page_version_keys_test.go:

    • Added tests for version-gated page header keys (Autofocus, Variables)
    • This looks correct - testing that keys are omitted below version floors
  2. In mdl/backend/modelsdk/page_write.go:

    • Modified to carry stored page header properties (Autofocus, CanvasWidth, CanvasHeight) instead of hardcoding
    • Added version checks for Autofocus (11.1.0) and Variables (10.17.0)
    • Added carryStoredPageHeader function to preserve editor-only properties
    • This fixes the page round-trip fidelity issue
  3. In mdl/backend/modelsdk/page_header_carry_test.go:

    • Tests for carrying stored header properties
    • Looks correct
  4. In mdl/backend/modelsdk/access_rule_audit_member_test.go:

    • Tests for not adding MemberAccess for audit associations (System.owner, System.changedBy)
    • Fixes the security issue where UPDATE SECURITY couldn't repair CE0066
  5. In mdl/executor/cmd_pages_builder_input_props_test.go:

    • Tests for Password, Validation, ReadOnlyStyle properties in describe
    • Fixes the password field round-trip issue
  6. In mdl/backend/modelsdk/domainmodel_delete_cross_assoc_test.go:

    • Tests for DROP ENTITY removing cross-module associations
    • Fixes the severe issue where dropping entity left cross-module associations
  7. In mdl/executor/cmd_pages_describe_input_props_test.go:

    • Tests for describing Password, Validation, ReadOnlyStyle properties
    • Another part of fixing the password field round-trip
  8. In mdl/linter/rules/legacy_image_widget.go:

    • New lint rule for legacy image widgets (staticimage/dynamicimage)
    • Addresses the lint issue where these weren't being caught
  9. In mdl/backend/pagemutator/mutator_datasource_test.go:

    • Tests for refusing DATABASE datasource on DataView (and other widgets where it's invalid)
    • Fixes the issue where SET DataSource = DATABASE was incorrectly allowed on DataView
  10. In mdl/backend/modelsdk/widget_write_action_defaults_test.go:

    • Tests for correct DisabledDuringExecution and SyncAutomatically values
    • Part of fixing the page round-trip fidelity (action half)
  11. In mdl/backend/modelsdk/page_zero_value_keys_test.go:

    • Tests for emitting zero-value keys (EntityRef null, empty strings, false booleans)
    • Part of fixing the page round-trip fidelity (dropped-key half)
  12. In mdl/executor/cmd_pages_showpage_args_test.go:

    • Tests for list widget's own row action being in correct context
    • Fixes the issue where MDL-PAGEARG01 incorrectly judged list widget actions
  13. In mdl/backend/modelsdk/widget_write.go:

    • Fixed TypeDefaults for AttributeRef (EntityRef should be null)
    • Fixed TypeDefaults for PageVariable (all source fields should be present)
    • Fixed MicroflowSettings to require OutputMappings with marker 3
    • Fixed CallNanoflowClientAction similarly
    • Fixed TextBox to not hardcode ReadOnlyStyle
    • Fixed widgetValidationToGen to preserve authored validation
    • Fixed clientActionToGen to set DisabledDuringExecution=true and SyncAutomatically=false for appropriate actions
    • These are all fixes to the widget writing logic

The changes appear to be well-targeted fixes for the specific issues described.

Let me check against the specific checklist items:

Overlap & duplication:

  • I don't see evidence of duplicating existing functionality. The changes are fixing specific bugs.
  • The new test files correspond to the bugs being fixed, so no overlap with existing test coverage.
  • The PR doesn't appear to re-document already-shipped features as new - it's fixing bugs.

MDL syntax design:

  • No new MDL syntax is being added, so this section doesn't apply.

Full-stack consistency:

  • Since no new MDL features are being added, this doesn't apply.
  • However, for the existing features being fixed, they were already wired through the full pipeline (grammar → AST → visitor → executor → etc.), and the fixes are in the appropriate layers (backend, executor, linter).

Test coverage:

  • New test files are added for each bug fix, which is good.
  • I don't see any use of time.Sleep for synchronization in the visible diff.
  • The tests appear to be integration-level where appropriate (testing the full flow).

Security & robustness:

  • I don't see obvious security issues introduced.
  • The fixes are correcting logic errors, not introducing new vulnerabilities.
  • No Unix sockets or file I/O in hot paths visible in the diff.
  • No silent side effects on typos apparent.
  • Method receivers appear correct.

Scope & atomicity:

  • As noted, this PR contains multiple bug fixes that could be split.
  • However, they're all related to core functionality improvements and were developed together.
  • The PR description groups them by what was wrong, suggesting the commits are atomic but being brought together.
  • Given that these are all bug fixes (not new features), and they're related to improving correctness/fidelity, it's reasonable to have them in one PR.

Code quality:

  • The changes look consistent with existing patterns.
  • I don't see obvious refactors that need to be checked for consistency.
  • No manually maintained lists that are being mismanaged visible in the diff.

Bugs & correctness:

  • The fixes address clear logic errors identified in the PR description.
  • The test files added verify the fixes work correctly.
  • The changes appear to be correct based on the descriptions.

Let me double-check a few specific things:

  1. For the DROP ENTITY fix: The test in domainmodel_delete_cross_assoc_test.go verifies that cross-module associations are removed when dropping an entity. This addresses the severe issue where leaving them caused storage layer failures.

  2. For the password field fix: The tests in cmd_pages_builder_input_props_test.go and cmd_pages_describe_input_props_test.go verify that Password, Validation, and ReadOnlyStyle properties are preserved through describe/exec. This addresses the issue where password fields became plaintext.

  3. For the page round-trip fidelity: The changes in page_write.go and related tests ensure that Autofocus, CanvasWidth, CanvasHeight are carried from stored documents instead of hardcoded, and that version-gated keys are only written when supported. This addresses the issue where describe/exec reported "Replaced" unnecessarily.

  4. For the list widget row action: The test in cmd_pages_showpage_args_test.go verifies that a list widget's own action is judged in the correct context (the row it renders). This fixes the false positive validation error.

  5. For the SET DataSource = DATABASE: The tests in mutator_datasource_test.go verify that this is refused for DataView (and other widgets where it's invalid), addressing the issue where it was incorrectly allowed.

  6. For the security issue: The test in access_rule_audit_member_test.go verifies that MemberAccess for audit associations (System.owner, System.changedBy) are not added, fixing the CE0066 issue.

  7. For the lint issue: The new legacy_image_widget.go file adds a lint rule for staticimage/dynamicimage widgets, addressing the issue where these weren't being caught by mxcli check.

Everything looks correct and well-tested.

One thing to note: The PR includes a note about a net-zero pair of commits (3ac6df5 and c2755df) that canceled each other out. This is mentioned as being kept for honesty, which is good practice.

Based on my review, this PR appears to be making correct, well-tested fixes for legitimate bugs. The changes are scoped appropriately to the issues being fixed, and they follow the project's patterns.

I should check if there are any violations of the checklist that I missed.

Let me look for:

  • Any new MDL syntax that wasn't properly wired through the full pipeline
  • Any violations of the MDL syntax design guidelines
  • Any missing tests
  • Any security issues
  • Any scope/atomicity issues

I don't see any new MDL syntax being added, so the full-stack consistency and syntax design sections don't apply violations.

The test coverage looks good for the fixes being made.

I don't see obvious security issues.

The scope could be argued as too broad (multiple unrelated fixes), but since they're all bug fixes for core functionality and were developed together, it's acceptable.

Let me check one specific thing from the PR description: "A password field round-tripped into a plaintext text box." The fix involves changes to widget writing and describing to preserve the Password property. Looking at the diff in widget_write.go, I see:

// TextBox: many null slots when unbound (attribute ref, screen-reader label,
// source variable, label template, visibility/editability/native settings).

And then:

g.SetSubmitBehaviour("OnEndEditing")
g.SetSubmitOnInputDelay(300)
-	g.SetValidation(widgetValidationToGen())
+	g.SetValidation(widgetValidationToGenWith(x.ValidationExpression, x.ValidationMessage))

This shows they fixed the validation handling to preserve authored validation expressions and messages, which was part of the password field issue (since password fields have validation).

In the builder tests, they're checking for Password property handling.

This looks correct.

Another thing: In page_write.go, they added:

func encodePage(page *pages.Page, pv *types.ProjectVersion, carry func(*genPg.Page)) ([]byte, error) {
	// Suppressing the key is right for the empty list Studio Pro always writes.
	// It is not right for variables the script actually declared: dropping those
	// would leave a page whose widgets reference names that are no longer there
	// (CE1151), from a statement that reported success. Refuse instead
	// (guard-don't-drop, ADR-0005).
	if len(page.Variables) > 0 && !pageSupportsVariables(pv) {
		names := make([]string, 0, len(page.Variables))
		for _, v := range page.Variables {
			names = append(names, v.Name)
		}
		return nil, fmt.Errorf(
			"page.Variables is only supported in Mendix %s and higher",
			versionLabel(pv),
		)
	}

This implements the guard-don't-drop principle for the Variables property, refusing to encode pages with variables when the target version doesn't support it, rather than silently dropping them (which would cause CE1151).

This is exactly what was needed for the version-gated page header keys issue.

Overall, the PR looks good. The fixes are well-targeted, properly tested, and address real bugs that could cause data loss or incorrect behavior.

My recommendation would be


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

@ako
ako merged commit 6bd256b into mendixlabs:main Sep 21, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants