From d63c323e8fea464e6b966e6b7f5335bb82dac3a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 18:40:08 +0000 Subject: [PATCH 1/7] fix(microflow): reject unknown annotations, and scope MPR008 per canvas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the four problems in upstream #884. ## An unknown annotation was silently dropped The grammar accepts any @name and extractMicroflowAnnotations' switch had no default arm, so an unrecognised annotation parsed and did nothing. The reporter found this as "@size(600, 300) parses without error but produces no effect". The sharper case is a TYPO of an annotation that does work: @postion(999, 111) -- check passed; the position was silently discarded On a workflow whose entire point is scripted canvas layout, that throws away the thing being authored and reports success. Both forms are now MDL059, an error rather than a warning, and enforced by exec as well as check. Annotations are read generically rather than through a type switch: fifteen statement types carry them, and a switch that skips the sixteenth reintroduces the silent drop one level up. TestEveryActivityAnnotationsFieldIsNamedAnnotations parses the AST package's own source for every *ActivityAnnotations field and asserts the accessor reaches it, and TestKnownAnnotationsMatchTheVisitor parses the visitor's case labels and compares both directions, so the restated list cannot drift from what the visitor implements. Checked against the real corpus for false positives: 405 MDL blocks across .claude/skills/mendix and docs-site/src all still pass. ## MPR008 compared nodes on different canvases The rule recursed into a LoopedActivity's ObjectCollection and appended the children into the same flat list as the microflow's own canvas, then compared every pair. A LoopedActivity's children are positioned RELATIVE to the container — verified in the stored BSON, an outer activity at 200;230 beside a loop child at 141;130 — so it reported overlaps that cannot happen on screen. A false positive is worse than silence for a rule whose job is to be trusted about positions. overlapPlanes now returns one plane per canvas and the pair loop iterates within each. The container itself stays on its parent's plane; only its children move. Extracting it also gives the rule its first real coverage: its test file carried a note that the logic "cannot be unit-tested without building a mock mpr.Reader", which is why this had none. Verified both directions against a real project with a pre-fix binary: the cross-canvas pair is reported by the old binary and not the new one, while a genuine same-canvas pair at (300,200)/(310,210) is still reported by both. The reporter's own quoted pair does not reproduce — (141,130) vs (134,230) is dy=100 against activityBoxHeight=60, so it never fired; the mechanism is real but that evidence was not, and a constructed pair was needed. Refs upstream #884. The remaining two problems (container Size from child positions, and bezier control vectors surviving a rewrite) are untouched. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013uQvFDd5R4eNqqita59jM8 --- .claude/skills/fix-issue.md | 2 + .../884-annotations-and-lint-planes.mdl | 80 +++++++++ mdl/ast/annotations.go | 36 ++++ mdl/ast/annotations_test.go | 111 ++++++++++++ mdl/ast/ast_microflow.go | 11 ++ mdl/executor/validate.go | 3 + mdl/executor/validate_annotations_test.go | 71 ++++++++ mdl/executor/validate_microflow.go | 41 +++++ .../rules/mpr008_overlapping_activities.go | 167 ++++++++++-------- .../mpr008_overlapping_activities_test.go | 69 ++++++++ mdl/visitor/visitor_microflow_statements.go | 7 + 11 files changed, 528 insertions(+), 70 deletions(-) create mode 100644 mdl-examples/bug-tests/884-annotations-and-lint-planes.mdl create mode 100644 mdl/ast/annotations.go create mode 100644 mdl/ast/annotations_test.go create mode 100644 mdl/executor/validate_annotations_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index bcc4bc2e8..6250cc8aa 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -507,3 +507,5 @@ extracting `OffsetExpression`/`LimitExpression`. | An import activity's Range — Studio Pro's **All / First / Custom** — is absent from `DESCRIBE MICROFLOW`, and a `limit`/`offset` set in Studio Pro does not survive an mxcli round trip | Worse than "undescribed". `Microflows$ImportMappingCall.Range` is polymorphic — `ConstantRange{SingleObject}` (All/First) or `CustomRange{LimitExpression, OffsetExpression}` (Custom) — and mxcli wrote only the first and read only `SingleObject`. So **Custom was unrepresentable**, a bounded import became unbounded on any rewrite, and all three settings described identically, so describe→edit→exec silently changed the activity | `mdl/grammar/domains/MDLMicroflow.g4` (`importMappingRange`) + `MDLLexer.g4` (`FIRST`) + `MDLSettings.g4` (keyword list), `mdl/visitor/visitor_import_export_mapping.go`, `mdl/ast/ast_microflow.go`, `sdk/microflows/microflows_actions.go` (`RangeSingleObject`, `RangeSingleObjectOf`), `mdl/executor/cmd_microflows_builder_calls.go` (`addImportFromMappingAction`), `mdl/executor/cmd_microflows_format_action.go` (`formatImportMappingRange`), `mdl/backend/modelsdk/microflow_write.go` (`importMappingRangeToGen`) + `microflow_read_actions.go`, `sdk/mpr/writer_microflow_actions.go` (`importMappingRange`) + `parser_microflow_actions.go` | **The Range and the result variable's CARDINALITY are separate axes** — the first fix folded them and every `all` wrote a `ListType` against an object-rooted mapping, which mxbuild rejects with **CE0243** ("the mapping used to return 'List of X' but now returns 'X'"). Mendix's own `FeedbackModule.SUB_Feedback_PostToAppInsights` settles it: `ConstantRange{SingleObject:false}` against an **ObjectType** variable. The stored `VariableType` is the authority on cardinality; the range is its own flag (hence `RangeSingleObject *bool`, nil = "not authored, fall back"). **Count the write sites before declaring a serialization fixed** — the `ImportMappingCall` is built at THREE places (`importXmlActionToGen`, the REST `ResultHandlingMapping` case, and the legacy writer), and patching two let a limit reach the model while a `ConstantRange` was still written; extract one helper. **DESCRIBE must emit one of the three, never nothing** — silence re-enters the builder's inference, and an object-rooted mapping set to All (Studio Pro's default) comes back as First. **`first` is not `limit 1`**: one binds an OBJECT, the other a one-element LIST, so they cannot share syntax. **A platform rule you cannot author a positive case for is documentation, not validation** — Mendix rejects `offset` on a non-list mapping with **CE6100** while accepting `limit`, and mxcli cannot currently author a list-rooted import mapping (array-root mappings emit CE5015), so the constraint is documented rather than guessed at in a checker. Tests `mdl/executor/cmd_microflows_import_range_test.go`, `mdl/backend/modelsdk/microflow_import_range_test.go`, `sdk/mpr/parser_microflow_import_range_test.go`, example `mdl-examples/bug-tests/881-import-mapping-range.mdl`. upstream #881 | | `CREATE IMPORT MAPPING` is reported to "not preserve JSON member names": `total` becomes `Total`, `camelCase` becomes `CamelCase`, `__Value` becomes `__ValueItem`. The import then fails at runtime with `key not found: Path(QName(None,),None,)` at `MappingCache.storeValueMappingElement` | The capitalisation is **Mendix's own** and not the defect. A `JsonStructures$JsonElement` stores TWO names — `Path` (the raw JSON key, `(Object)\|uuid`, which the RUNTIME resolves by) and `ExposedName` (derived by capitalising the initial, and suffixing `Item` on an array's item object, which Studio Pro DISPLAYS). The real defect: mxcli's DESCRIBE emits `ExposedName` while its builder resolved only raw keys, so mxcli's own output did not round-trip — re-executing a DESCRIBE FABRICATED a path (`(Object)\|Total`; for arrays the `\|(Object)` item marker vanished entirely, leaving `(Object)\|EntityInstances\|__ValueItem`) and zeroed MaxOccurs. Export mappings had the identical bug. Separately, mxcli wrote `IsDefaultType` on every `ValueMappingElement` (a property only the OBJECT element types own) and cloned the JSON structure's `OriginalValue` — the SAMPLE value from the snippet — onto every mapping element, where Studio Pro leaves it empty | `mdl/executor/cmd_import_mappings.go` (`jsonSchemaIndex`, `newJSONSchemaIndex`, `resolve`, `memberNames`; `buildImportMappingElementModel` now returns an error), `mdl/executor/cmd_export_mappings.go` (same), `mdl/backend/modelsdk/mapping_write.go` + `sdk/mpr/writer_import_mapping.go` + `sdk/mpr/writer_export_mapping.go` (drop `IsDefaultType` from the value writers); `cmd_import_mappings.go` again for the `OriginalValue` clone | **A blank Mendix app is a free Studio-Pro-authored fixture** — `FeedbackModule.IMM_PostResponse` + `JSON_AppInsightsResponse` settle "what does Studio Pro actually write?" with no Studio Pro and no marketplace download: they store `ExposedName "Uuid"` against `Path "(Object)|uuid"`. Author the SAME mapping over the SAME structure in the SAME module with mxcli and diff the two units — the only variable left is which tool wrote it. **Run the reporter's repro as written before believing the diagnosis**: theirs produced paths byte-identical to the structure's, so their MDL was not the failing input; the corruption needed a DESCRIBE in the middle. **DESCRIBE emitting a different name than the parser accepts is a round-trip bug even when both are "correct"** — the fix is to accept BOTH spellings, not to change what DESCRIBE prints, because the exposed name is the one Studio Pro shows. **Never fabricate a path for an unresolved member**: the invented path passed `mxcli check` and surfaced only in mxbuild (CE5015) or at runtime, so the tool that wrote it reported success — refuse, and list the spellings that would have worked. **`generated/metamodel` decides property ownership**: `isDefaultType` is declared on `Import/ExportObjectMappingElement` and on neither `ValueMappingElement`, and per CLAUDE.md's overlay rule an extra property is the shape mxbuild tolerates and Studio Pro will not open — so a green build proves nothing here. **Count the samples before calling a difference a defect**: two Studio-Pro-authored mappings in a blank app (~15 value elements) all write `OriginalValue: ""` while their structures carry 17 non-empty samples, which is what makes "the sample belongs to the structure" a measurement rather than an opinion. **Running the reporter's repro is how you find out it is not a reproduction**: theirs PASSED at runtime on 11.6.6 and 11.13.0, so the runtime error they see needs something their standalone file does not carry — say so instead of claiming the fix. And **control the harness before believing its failure**: an early runtime run failed until the control (Studio Pro's own mapping, same harness) failed identically, which located the fault in the test's missing module-role grants, not in the mapping. Tests `mdl/executor/cmd_mappings_member_resolution_test.go`, `mdl/backend/modelsdk/mapping_isdefaulttype_test.go`, example `mdl-examples/bug-tests/882-mapping-member-names.mdl`. upstream #882 | | `create validation rule …` parses and silently writes nothing, so a regex document cannot be bound to an attribute from MDL at all — every form (regex, range, required, unique, expression) reports success and leaves the model untouched | Three layers, one visible symptom. (1) The grammar rule existed with **no AST node, no visitor and no handler**. (2) Its shape was unimplementable: an `EXPRESSION` rule type Mendix does not have, an **inline** regex literal where Mendix stores a **reference to a document**, and strict `<`/`>` bounds Mendix cannot represent. (3) Underneath, `modelsdk/gen` bound `RegExRuleInfo`'s reference to BSON `RegularExpression` where Studio Pro stores `RegExIdentifier`, so the rule could not have been written even with the statement wired up | `modelsdk/gen/domainmodels/types.go` (`STORAGE-NAME OVERRIDE` on `initRegExRuleInfo` **and** `InitFromRaw`), `mdl/backend/modelsdk/domainmodel.go` (`ruleInfoFromGen`) + `domainmodel_write.go` (`ruleInfoToGen` now takes the whole rule), `sdk/domainmodel/domainmodel.go` (regex ref is a qualified NAME not an ID; Range gained the attribute-bound fields), `mdl/grammar/domains/MDLDomainModel.g4`, `mdl/ast/ast_validationrule.go`, `mdl/visitor/visitor_validationrule.go`, `mdl/executor/cmd_validationrules.go`, `mdl/executor/cmd_entities_describe.go` | **Correcting `modelsdk/gen` IS available** — this supersedes the previous row's "no `supplements.json` or `cmd/modelsdk-codegen` in this tree, so correcting gen is not currently available". Neither path has ever existed here (`git log --all` empty) and `/reference/` is gitignored, but four `STORAGE-NAME OVERRIDE` precedents already hand-patch `init`, and `TestGeneratedCodeIsFormatted` exists because gen gets hand-edited. **`generated/metamodel` is the arbiter**: `cmd/codegen` emits the storage name as the json tag (`json:"regExIdentifier"`) while gen kept the SDK name — an audit of all 570 cross-checkable types found **65 with at least one wrong key**, and the three with independent evidence (`RegularExpression.Expression`, `Attribute.GUID`, this one) were all flagged correctly. **Patch BOTH the encode and decode literal**: reverting only `init` gave a document that writes `RegularExpression` and reads `RegExIdentifier`, whose symptom is a confusing entity-rewrite refusal rather than a bad file. **The control is the proof**: same script, key reverted → `CE0135 "No regular expression specified"` on mxbuild 11.13; with the fix → 0 errors and `RegExIdentifier` on disk. **A rule type is not a rule** — `ruleInfoToGen(ruleType string)` could never rebuild a RegEx (its reference) or a Range (its bounds), so it was widened to the whole rule and the reader taught to carry the payload; a bare RuleInfo of the right `$Type` is a silent downgrade wearing the right name. **Check for a bound that is not a literal**: a Range may point at another ATTRIBUTE, which MDL cannot author and a literals-only reader would have dropped on the next rewrite. **Do not add a second spelling for something already authorable** — Required/Unique stay attribute constraints (`not null error '…'`), refused here with a pointer. **Fixing an unimplementable grammar is free when nothing could depend on it**: every old form parsed and did nothing, so the shape was replaced rather than preserved. Tests `modelsdk/gen/domainmodels/storagename_test.go`, `mdl/backend/modelsdk/validationrule_test.go`, `mdl/visitor/visitor_validationrule_test.go`, `mdl/executor/cmd_validationrules_test.go`, example `mdl-examples/doctype-tests/validation-rules.mdl` | +| An `@annotation` mxcli does not implement — `@size(600, 300)` — parses and does nothing, and layout authored through annotations silently differs from what was written | The grammar accepts any `@name`, and `extractMicroflowAnnotations`' switch had **no default arm**, so an unrecognised name was dropped in silence. Benign for an unimplemented annotation; NOT benign for a typo of an implemented one — `@postion(10, 20)` passed `check` and discarded the position | `mdl/ast/ast_microflow.go` (`ActivityAnnotations.UnknownNames`), `mdl/ast/annotations.go` (new: `StatementAnnotations`), `mdl/visitor/visitor_microflow_statements.go` (the `default:` arm), `mdl/executor/validate_microflow.go` (`checkUnknownAnnotations`, `knownActivityAnnotations`, MDL059), `mdl/executor/validate.go` (exec-enforced) | **A silent no-op is worse than an unimplemented feature** — the reporter filed "`@size` is ignored", but the same missing arm meant a typo of the annotation their whole workflow depends on threw away the layout with a clean `check`. Fix the class, not the instance. **Read the annotations generically, not through a type switch**: 15 statement types carry `Annotations`, and a hand-written switch that skips the 16th reintroduces the silent drop one level up — `StatementAnnotations` reflects, and `TestEveryActivityAnnotationsFieldIsNamedAnnotations` parses the AST package's own source for every `*ActivityAnnotations` field and asserts it is named so the accessor reaches it. **Pin a restated list against its source**: `knownActivityAnnotations` duplicates the visitor's case labels, so `TestKnownAnnotationsMatchTheVisitor` parses those labels out of the visitor and compares both directions — a name in one and not the other either rejects a working annotation or accepts a dead one. **Check a new rejection against the real corpus before believing it**: `scripts/check-skill-mdl.sh` over `.claude/skills/mendix` and `docs-site/src` (405 blocks) proved no false positives. Tests `mdl/ast/annotations_test.go`, `mdl/executor/validate_annotations_test.go`. upstream #884 | +| `mxcli lint` reports MPR008 "activities overlap" for two nodes that live on different canvases — one inside a loop, one outside — so generated layout is flagged for an overlap that cannot happen on screen | `collect` recursed into `LoopedActivity.ObjectCollection` and appended the children into the **same flat list** as the microflow's own canvas, then compared every pair. A LoopedActivity's children are positioned RELATIVE to the container; verified in the stored BSON — outer activity `200;230` (absolute) beside a loop child `141;130` (container-relative) | `mdl/linter/rules/mpr008_overlapping_activities.go` (`overlapPlanes`, one plane per canvas; the pair loop iterates per plane) | **A false positive on a layout rule is worse than silence** — the rule exists to be trusted about positions. **Extract the inline logic to make the fix testable**: the rule's own test file carried a note that the overlap logic "cannot be unit-tested without building a mock mpr.Reader", which is why the bug had no coverage; `overlapPlanes` is a pure function over the object tree and needs no reader. **Assert both directions** — the cross-canvas pair must stop being reported AND a genuine same-canvas pair at (300,200)/(310,210) must still be, or the fix trades a false positive for a false negative. **The reporter's own numbers did not reproduce**: (141,130) vs (134,230) is dy=100 against `activityBoxHeight = 60`, so that pair never fired; the mechanism was real but the quoted evidence was not, and a constructed pair was needed to demonstrate it. **A vanished fixture reads exactly like a fixed bug** — an intermediate "no violations" result was the scratch project having been reaped, not the fix working; the A/B was redone with a pre-fix binary against the same rebuilt project. Tests `mpr008_overlapping_activities_test.go`. upstream #884 | diff --git a/mdl-examples/bug-tests/884-annotations-and-lint-planes.mdl b/mdl-examples/bug-tests/884-annotations-and-lint-planes.mdl new file mode 100644 index 000000000..3da21db61 --- /dev/null +++ b/mdl-examples/bug-tests/884-annotations-and-lint-planes.mdl @@ -0,0 +1,80 @@ +-- Bug test for upstream issue #884 (two of the four reported problems). +-- +-- 1. UNKNOWN ANNOTATIONS WERE SILENTLY DROPPED. +-- The grammar accepts any @name and the visitor's switch had no default arm, +-- so an unrecognised annotation parsed and did nothing. The reporter found +-- this as "@size(600, 300) parses without error but produces no effect". +-- The sharper case is a TYPO of an annotation that does work: +-- +-- @postion(999, 111) -- check passed; the position was discarded +-- +-- Both are now MDL059 errors. Uncomment either line below to see it. +-- +-- 2. MPR008 COMPARED NODES ACROSS DIFFERENT CANVASES. +-- A LoopedActivity's children are positioned RELATIVE to the container, and +-- the rule flattened them into the same list as the microflow's own canvas, +-- so it reported overlaps that cannot happen on screen. +-- +-- Expected: `mxcli check` passes on this file, and `mxcli lint` reports MPR008 +-- for MF_SamePlane ONLY — never for MF_CrossCanvas. +-- +-- Verified on mxbuild 11.6.6. + +create module B884; +/ +create non-persistent entity B884.Rec ( Nm: string(100) ); +/ + +-- Cross-canvas: the outer node is at absolute (150,150); the loop child is at +-- container-relative (141,130). Different planes — MPR008 must stay silent. +create microflow B884.MF_CrossCanvas ( $Items: list of B884.Rec ) +returns boolean as $Ok +begin + @position(150, 150) + declare $Outer string = 'x'; + loop $it in $Items + begin + @position(141, 130) + declare $Inner string = 'y'; + end loop; + declare $Ok boolean = true; + return $Ok; +end; +/ + +-- Same canvas, 10px apart: MPR008 must STILL report this, or the scoping fix +-- traded a false positive for a false negative. +create microflow B884.MF_SamePlane ( $P: string ) +returns string as $R +begin + @position(300, 200) + declare $A string = $P; + @position(310, 210) + declare $R string = $A; + return $R; +end; +/ + +-- Every annotation mxcli implements, so the new rejection cannot fire on valid +-- input (checked against 405 real MDL blocks in the skills and docs corpora). +create microflow B884.MF_AllAnnotations ( $P: string ) +returns string as $R +begin + @position(200, 100) + @caption 'Set it' + @color Green + declare $A string = $P; + @annotation 'a free note' + @position(360, 100) + @anchor(from: right, to: left) + declare $R string = $A; + return $R; +end; +/ + +-- Rejected at check time (MDL059), so kept commented: +-- @size(600, 300) -- not implemented; container Size is still unauthorable +-- @postion(999, 111) -- typo of @position; used to discard the layout in silence +-- @curve(from: (40, -90), to: (-40, 90)) +-- -- parses today, and is NOT implemented: a hand-curved edge's bezier +-- control vectors are reset to "0;0" by any rewrite. Still open. diff --git a/mdl/ast/annotations.go b/mdl/ast/annotations.go new file mode 100644 index 000000000..2c807837b --- /dev/null +++ b/mdl/ast/annotations.go @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: Apache-2.0 + +package ast + +import "reflect" + +// StatementAnnotations returns the @-annotations attached to a microflow +// statement, or nil for a statement type that carries none. +// +// Read by reflection on purpose. Fifteen statement types have an `Annotations +// *ActivityAnnotations` field today, and a hand-written type switch over them +// would silently skip the sixteenth — which is exactly the failure this exists to +// catch, since the caller's job is to report annotations that were quietly +// dropped. TestEveryAnnotatedStatementIsReachable pins that the reflective read +// reaches every such type. (upstream #884) +func StatementAnnotations(s MicroflowStatement) *ActivityAnnotations { + if s == nil { + return nil + } + v := reflect.ValueOf(s) + for v.Kind() == reflect.Ptr { + if v.IsNil() { + return nil + } + v = v.Elem() + } + if v.Kind() != reflect.Struct { + return nil + } + f := v.FieldByName("Annotations") + if !f.IsValid() || f.Kind() != reflect.Ptr || f.IsNil() { + return nil + } + ann, _ := f.Interface().(*ActivityAnnotations) + return ann +} diff --git a/mdl/ast/annotations_test.go b/mdl/ast/annotations_test.go new file mode 100644 index 000000000..033adc94d --- /dev/null +++ b/mdl/ast/annotations_test.go @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: Apache-2.0 + +package ast + +import ( + "go/ast" + "go/parser" + "go/token" + "testing" +) + +// StatementAnnotations reads the Annotations field reflectively rather than +// through a type switch, because its whole job is to report annotations that +// were silently dropped — and a hand-written switch that silently skips a +// statement type reintroduces exactly that bug one level up. +// +// The accessor keys on one thing: a field NAMED "Annotations" whose type is +// *ActivityAnnotations. This test parses the package's own source for every +// struct field of that type and asserts each is named accordingly, so a type +// that spells it differently (or embeds it) fails here rather than silently +// falling out of annotation validation. (upstream #884) +func TestEveryActivityAnnotationsFieldIsNamedAnnotations(t *testing.T) { + fset := token.NewFileSet() + pkgs, err := parser.ParseDir(fset, ".", nil, 0) + if err != nil { + t.Fatalf("parse ast package: %v", err) + } + + type field struct{ typeName, fieldName, pos string } + var found []field + + for _, pkg := range pkgs { + for _, file := range pkg.Files { + ast.Inspect(file, func(n ast.Node) bool { + ts, ok := n.(*ast.TypeSpec) + if !ok { + return true + } + st, ok := ts.Type.(*ast.StructType) + if !ok { + return true + } + for _, f := range st.Fields.List { + star, ok := f.Type.(*ast.StarExpr) + if !ok { + continue + } + id, ok := star.X.(*ast.Ident) + if !ok || id.Name != "ActivityAnnotations" { + continue + } + if len(f.Names) == 0 { // embedded + found = append(found, field{ts.Name.Name, "", fset.Position(f.Pos()).String()}) + continue + } + for _, name := range f.Names { + found = append(found, field{ts.Name.Name, name.Name, fset.Position(name.Pos()).String()}) + } + } + return true + }) + } + } + + // A broken scan would make every assertion below pass vacuously. + if len(found) < 10 { + t.Fatalf("found only %d *ActivityAnnotations fields — the source scan is broken", len(found)) + } + + for _, f := range found { + if f.fieldName != "Annotations" { + t.Errorf("%s carries *ActivityAnnotations as %q (%s), but StatementAnnotations looks up "+ + "the field named \"Annotations\" — this type's annotations would be silently skipped "+ + "by validation, which is the bug #884 is about", f.typeName, f.fieldName, f.pos) + } + } +} + +// The accessor itself, against real statement types: one that carries +// annotations, one that does not, and the nil cases. +func TestStatementAnnotations(t *testing.T) { + ann := &ActivityAnnotations{UnknownNames: []string{"size"}} + + withAnnotations := []MicroflowStatement{ + &DeclareStmt{Annotations: ann}, + &ReturnStmt{Annotations: ann}, + &IfStmt{Annotations: ann}, + &LoopStmt{Annotations: ann}, + } + for _, s := range withAnnotations { + got := StatementAnnotations(s) + if got == nil { + t.Errorf("%T: got nil, want the annotations that were set", s) + continue + } + if len(got.UnknownNames) != 1 || got.UnknownNames[0] != "size" { + t.Errorf("%T: UnknownNames = %v, want [size]", s, got.UnknownNames) + } + } + + // A statement whose Annotations field is nil reads as nil, not as a panic. + if got := StatementAnnotations(&DeclareStmt{}); got != nil { + t.Errorf("unset Annotations = %v, want nil", got) + } + if got := StatementAnnotations(nil); got != nil { + t.Errorf("nil statement = %v, want nil", got) + } + if got := StatementAnnotations((*DeclareStmt)(nil)); got != nil { + t.Errorf("typed-nil statement = %v, want nil", got) + } +} diff --git a/mdl/ast/ast_microflow.go b/mdl/ast/ast_microflow.go index efafa833c..fe130d86c 100644 --- a/mdl/ast/ast_microflow.go +++ b/mdl/ast/ast_microflow.go @@ -211,6 +211,17 @@ type ActivityAnnotations struct { // populated on LoopStmt/WhileStmt. IteratorAnchor *FlowAnchors BodyTailAnchor *FlowAnchors + + // UnknownNames holds annotation names the visitor did not recognise, in + // source order, so validation can refuse them. + // + // The visitor's switch has no default: an unrecognised name used to be + // dropped in silence, which is benign for an annotation mxcli does not + // implement (@size) and NOT benign for a typo of one it does — `@postion(10, + // 20)` passed `check` and silently discarded the layout the author asked + // for. Layout is the whole point of these annotations, so a name that does + // nothing has to say so. (upstream #884) + UnknownNames []string } // ChangeItem represents a single assignment in CREATE/CHANGE: Attr = expr diff --git a/mdl/executor/validate.go b/mdl/executor/validate.go index 6a72e0737..2fd769ae3 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -1061,6 +1061,9 @@ var execEnforcedMicroflowRules = map[string]bool{ // built-ins (isNew/isSynced/isSyncing) were found missing and added — each // built at 0 errors — before this line was added. "MDL044": true, + // #884: an unknown annotation is silently dropped, so exec must refuse it too — + // otherwise `check` catches the typo and the write that follows does not. + "MDL059": true, } // validateMicroflowRules runs the MDL0xx microflow rule set (ValidateMicroflow) diff --git a/mdl/executor/validate_annotations_test.go b/mdl/executor/validate_annotations_test.go new file mode 100644 index 000000000..73f50222e --- /dev/null +++ b/mdl/executor/validate_annotations_test.go @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "go/ast" + "go/parser" + "go/token" + "strconv" + "testing" +) + +// knownActivityAnnotations restates the visitor's switch arms, and the two must +// not drift: a name in the visitor but not here is REJECTED though it works, and +// a name here but not in the visitor is ACCEPTED though it does nothing — which +// is the silent-drop bug MDL059 exists to end. +// +// Rather than trust the copy, parse extractMicroflowAnnotations' case labels out +// of the visitor's source and compare. (upstream #884) +func TestKnownAnnotationsMatchTheVisitor(t *testing.T) { + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "../visitor/visitor_microflow_statements.go", nil, 0) + if err != nil { + t.Fatalf("parse visitor: %v", err) + } + + visitorNames := map[string]bool{} + ast.Inspect(file, func(n ast.Node) bool { + fn, ok := n.(*ast.FuncDecl) + if !ok || fn.Name.Name != "extractMicroflowAnnotations" { + return true + } + ast.Inspect(fn.Body, func(m ast.Node) bool { + cc, ok := m.(*ast.CaseClause) + if !ok { + return true + } + for _, e := range cc.List { + lit, ok := e.(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + continue + } + if s, err := strconv.Unquote(lit.Value); err == nil { + visitorNames[s] = true + } + } + return true + }) + return false + }) + + // hasLaterActivityAnnotation lists a subset of the same names; the scan above + // is scoped to extractMicroflowAnnotations so it cannot pick those up twice. + if len(visitorNames) == 0 { + t.Fatal("no case labels found in extractMicroflowAnnotations — the scan is broken, " + + "and a broken scan makes this test pass vacuously") + } + + for name := range visitorNames { + if !knownActivityAnnotations[name] { + t.Errorf("the visitor implements @%s but knownActivityAnnotations omits it — "+ + "MDL059 would reject an annotation that actually works", name) + } + } + for name := range knownActivityAnnotations { + if !visitorNames[name] { + t.Errorf("knownActivityAnnotations lists @%s but the visitor has no case for it — "+ + "it would be accepted and silently do nothing", name) + } + } +} diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index fc1f140c8..8b8cef5f1 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -142,6 +142,7 @@ func (v *microflowValidator) checkDuplicateLoopVariables(body []ast.MicroflowSta // walkBody recursively walks microflow body statements looking for per-statement issues. func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { for _, s := range body { + v.checkUnknownAnnotations(s) switch stmt := s.(type) { case *ast.ValidationFeedbackStmt: if isEmptyMessage(stmt.Message) { @@ -1302,3 +1303,43 @@ func (v *microflowValidator) checkEnumSplitEmptyBranch(stmt *ast.EnumSplitStmt) "Add a `when (empty) then …` branch. It is required even when the attribute is `not null`. "+ "A branch may list several values (`when Open, (empty) then …`) if they share a path.") } + +// knownActivityAnnotations is the set the visitor implements. It is the visitor's +// own switch arms, restated: the two are pinned together by +// TestKnownAnnotationsMatchTheVisitor, because a name added to one and not the +// other either rejects a valid annotation or silently drops an invalid one. +var knownActivityAnnotations = map[string]bool{ + "position": true, + "caption": true, + "color": true, + "annotation": true, + "excluded": true, + "anchor": true, +} + +// checkUnknownAnnotations rejects an @annotation name the visitor does not +// implement. +// +// The grammar accepts any @name, and the visitor's switch had no default, so an +// unrecognised one was dropped in silence. That is benign for an annotation mxcli +// does not implement — @size(600, 300), which the #884 reporter found parsing +// without effect — and NOT benign for a typo of one it does: `@postion(10, 20)` +// passed `check` and discarded the layout the author asked for, on a workflow +// whose entire point is scripted canvas layout. +// +// An error rather than a warning: the statement's meaning silently differs from +// what was written, and warnings on a generated script are not read. (#884) +func (v *microflowValidator) checkUnknownAnnotations(s ast.MicroflowStatement) { + ann := ast.StatementAnnotations(s) + if ann == nil { + return + } + for _, name := range ann.UnknownNames { + v.addViolation("MDL059", linter.SeverityError, + fmt.Sprintf("unknown annotation `@%s` — it parses but does nothing, so whatever it was "+ + "meant to express is silently lost", name), + fmt.Sprintf("mxcli implements @position(x, y), @caption, @color, @annotation, @excluded and "+ + "@anchor on a microflow statement. If `@%s` is a typo of one of those, correct it; "+ + "container size and edge geometry are not authorable (upstream #884).", name)) + } +} diff --git a/mdl/linter/rules/mpr008_overlapping_activities.go b/mdl/linter/rules/mpr008_overlapping_activities.go index 39bd9b8e7..ff9b5f314 100644 --- a/mdl/linter/rules/mpr008_overlapping_activities.go +++ b/mdl/linter/rules/mpr008_overlapping_activities.go @@ -55,88 +55,115 @@ func (r *OverlappingActivitiesRule) Check(ctx *linter.LintContext) []linter.Viol continue } - type actInfo struct { - x, y int - caption string - } + planes := overlapPlanes(fullMF.ObjectCollection.Objects) - var activities []actInfo - var collect func(objects []microflows.MicroflowObject) - collect = func(objects []microflows.MicroflowObject) { - for _, obj := range objects { - switch act := obj.(type) { - case *microflows.ActionActivity: - p := act.GetPosition() - caption := act.Caption - if caption == "" { - caption = "(unnamed)" + // Check all pairs for overlapping positions, WITHIN a plane. + // Skip activities at the origin (0,0) — these are unpositioned/default. + reported := make(map[string]bool) + for _, activities := range planes { + for i := 0; i < len(activities); i++ { + for j := i + 1; j < len(activities); j++ { + a, b := activities[i], activities[j] + if (a.x == 0 && a.y == 0) || (b.x == 0 && b.y == 0) { + continue + } + dx := a.x - b.x + if dx < 0 { + dx = -dx } - activities = append(activities, actInfo{p.X, p.Y, caption}) - case *microflows.LoopedActivity: - p := act.GetPosition() - caption := act.Caption - if caption == "" { - caption = "(loop)" + dy := a.y - b.y + if dy < 0 { + dy = -dy } - activities = append(activities, actInfo{p.X, p.Y, caption}) - if act.ObjectCollection != nil { - collect(act.ObjectCollection.Objects) + if dx < activityBoxWidth && dy < activityBoxHeight { + key := fmt.Sprintf("%d,%d|%d,%d", a.x, a.y, b.x, b.y) + if reported[key] { + continue + } + reported[key] = true + violations = append(violations, linter.Violation{ + RuleID: r.ID(), + Severity: r.DefaultSeverity(), + Message: fmt.Sprintf( + "Activities '%s' (%d,%d) and '%s' (%d,%d) overlap in microflow '%s.%s'. "+ + "Each MDL statement that creates a canvas activity needs its own @position annotation.", + a.caption, a.x, a.y, b.caption, b.x, b.y, + mf.ModuleName, mf.Name, + ), + Location: linter.Location{ + Module: mf.ModuleName, + DocumentType: "microflow", + DocumentName: mf.Name, + DocumentID: mf.ID, + }, + Suggestion: "Add a separate @position(x, y) annotation before each statement. Use 190px spacing between activities.", + }) } - case *microflows.ExclusiveSplit: - p := act.GetPosition() - activities = append(activities, actInfo{p.X, p.Y, act.Caption}) - case *microflows.ExclusiveMerge: - p := act.GetPosition() - activities = append(activities, actInfo{p.X, p.Y, "(merge)"}) } } } - collect(fullMF.ObjectCollection.Objects) + } - // Check all pairs for overlapping positions. - // Skip activities at the origin (0,0) — these are unpositioned/default. - reported := make(map[string]bool) - for i := 0; i < len(activities); i++ { - for j := i + 1; j < len(activities); j++ { - a, b := activities[i], activities[j] - if (a.x == 0 && a.y == 0) || (b.x == 0 && b.y == 0) { - continue - } - dx := a.x - b.x - if dx < 0 { - dx = -dx + return violations +} + +// actInfo is one positioned node on a canvas. +type actInfo struct { + x, y int + caption string +} + +// overlapPlanes splits a microflow's objects into one plane per CANVAS, rather +// than flattening them into a single list. +// +// A LoopedActivity's children are positioned RELATIVE to the loop container, +// while everything on the microflow's own canvas is absolute. Flattening the two +// compares coordinates from different spaces and reports overlaps that cannot +// happen on screen — verified in the stored BSON: an outer activity at 200;230 +// alongside a loop child at 141;130, where 141;130 is measured from the loop's +// own frame. A false "these overlap" is worse than silence here, because the +// point of the rule is to be trusted about positions. (upstream #884) +// +// The container itself belongs to its PARENT's plane; only its children get a +// new one. +func overlapPlanes(objects []microflows.MicroflowObject) [][]actInfo { + var planes [][]actInfo + var collect func(objs []microflows.MicroflowObject) + collect = func(objs []microflows.MicroflowObject) { + var plane []actInfo + var nested [][]microflows.MicroflowObject + for _, obj := range objs { + switch act := obj.(type) { + case *microflows.ActionActivity: + caption := act.Caption + if caption == "" { + caption = "(unnamed)" } - dy := a.y - b.y - if dy < 0 { - dy = -dy + p := act.GetPosition() + plane = append(plane, actInfo{p.X, p.Y, caption}) + case *microflows.LoopedActivity: + caption := act.Caption + if caption == "" { + caption = "(loop)" } - if dx < activityBoxWidth && dy < activityBoxHeight { - key := fmt.Sprintf("%d,%d|%d,%d", a.x, a.y, b.x, b.y) - if reported[key] { - continue - } - reported[key] = true - violations = append(violations, linter.Violation{ - RuleID: r.ID(), - Severity: r.DefaultSeverity(), - Message: fmt.Sprintf( - "Activities '%s' (%d,%d) and '%s' (%d,%d) overlap in microflow '%s.%s'. "+ - "Each MDL statement that creates a canvas activity needs its own @position annotation.", - a.caption, a.x, a.y, b.caption, b.x, b.y, - mf.ModuleName, mf.Name, - ), - Location: linter.Location{ - Module: mf.ModuleName, - DocumentType: "microflow", - DocumentName: mf.Name, - DocumentID: mf.ID, - }, - Suggestion: "Add a separate @position(x, y) annotation before each statement. Use 190px spacing between activities.", - }) + p := act.GetPosition() + plane = append(plane, actInfo{p.X, p.Y, caption}) + if act.ObjectCollection != nil { + nested = append(nested, act.ObjectCollection.Objects) } + case *microflows.ExclusiveSplit: + p := act.GetPosition() + plane = append(plane, actInfo{p.X, p.Y, act.Caption}) + case *microflows.ExclusiveMerge: + p := act.GetPosition() + plane = append(plane, actInfo{p.X, p.Y, "(merge)"}) } } + planes = append(planes, plane) + for _, o := range nested { + collect(o) + } } - - return violations + collect(objects) + return planes } diff --git a/mdl/linter/rules/mpr008_overlapping_activities_test.go b/mdl/linter/rules/mpr008_overlapping_activities_test.go index 3964e89d9..eba85be69 100644 --- a/mdl/linter/rules/mpr008_overlapping_activities_test.go +++ b/mdl/linter/rules/mpr008_overlapping_activities_test.go @@ -6,6 +6,8 @@ import ( "testing" "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" ) // NOTE: The full Check() logic requires ctx.Reader().GetMicroflow() to read microflow @@ -40,3 +42,70 @@ func TestOverlappingActivitiesRule_Metadata(t *testing.T) { // overlapping activities using pairwise distance checks against internal heuristic constants // (activityBoxWidth, activityBoxHeight). Since the collect function is defined inline in // Check(), behavioral testing requires a real *mpr.Reader with positioned activities. + +// upstream #884. MPR008 flattened a loop's children into the same list as the +// microflow's own canvas and compared every pair. A LoopedActivity's children are +// positioned RELATIVE to the container, so that compares two different coordinate +// spaces and reports overlaps that cannot happen on screen. +// +// Verified against a real project before and after: an outer activity at +// (150,150) and a loop child at (141,130) were reported as overlapping by the +// pre-fix binary and are not by the fixed one, while a genuine same-canvas pair +// at (300,200)/(310,210) is still reported by both. +func TestOverlapPlanesSeparatesContainerCanvases(t *testing.T) { + inner := µflows.ActionActivity{} + inner.Position = model.Point{X: 141, Y: 130} + inner.Caption = "inner" + + loop := µflows.LoopedActivity{ + ObjectCollection: µflows.MicroflowObjectCollection{ + Objects: []microflows.MicroflowObject{inner}, + }, + } + loop.Position = model.Point{X: 520, Y: 230} + loop.Caption = "loop" + + outer := µflows.ActionActivity{} + outer.Position = model.Point{X: 150, Y: 150} + outer.Caption = "outer" + + planes := overlapPlanes([]microflows.MicroflowObject{outer, loop}) + + if len(planes) != 2 { + t.Fatalf("got %d planes, want 2 (the microflow canvas and the loop's own)", len(planes)) + } + + // The container sits on the parent's plane; only its children move. + captions := map[string]int{} + for i, plane := range planes { + for _, a := range plane { + captions[a.caption] = i + } + } + if captions["outer"] != captions["loop"] { + t.Errorf("the loop container must share the parent canvas with 'outer': outer=%d loop=%d", + captions["outer"], captions["loop"]) + } + if captions["inner"] == captions["outer"] { + t.Error("the loop's CHILD must not share a plane with the microflow's own canvas — " + + "its coordinates are relative to the container, so comparing them is meaningless") + } +} + +// A container with no children must not produce a phantom plane holding nothing, +// and a flat microflow must produce exactly one. +func TestOverlapPlanesFlatMicroflow(t *testing.T) { + a := µflows.ActionActivity{} + a.Position = model.Point{X: 300, Y: 200} + b := µflows.ActionActivity{} + b.Position = model.Point{X: 310, Y: 210} + + planes := overlapPlanes([]microflows.MicroflowObject{a, b}) + if len(planes) != 1 { + t.Fatalf("got %d planes, want 1", len(planes)) + } + if len(planes[0]) != 2 { + t.Errorf("got %d activities on the canvas, want 2 — both must still be compared "+ + "against each other, or the fix trades a false positive for a false negative", len(planes[0])) + } +} diff --git a/mdl/visitor/visitor_microflow_statements.go b/mdl/visitor/visitor_microflow_statements.go index 8cdefcdc0..093da4f2a 100644 --- a/mdl/visitor/visitor_microflow_statements.go +++ b/mdl/visitor/visitor_microflow_statements.go @@ -309,6 +309,13 @@ func extractMicroflowAnnotations(annotations []parser.IAnnotationContext) *ast.A hasAny = true } seenActivityMetadata = true + + default: + // Record rather than drop. The grammar accepts any @name, so this + // arm catches both an annotation mxcli does not implement (@size) + // and — the reason it matters — a typo of one it does. (#884) + result.UnknownNames = append(result.UnknownNames, ann.AnnotationName().GetText()) + hasAny = true } } From 3e38e0424d49c1982fed239ef66b470963f037dc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 17:38:22 +0000 Subject: [PATCH 2/7] Cache the reference model, not the whole reference project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cache entry was 34 MB because it stored whatever `mx create-project` produced — a whole blank Mendix app. Most of it is never read. Measured on Administration at 11.12.1: PackageRef.mpr 14 MB read widgets/ 9.6 MB never read themesource/ 6.4 MB never read theme-cache/ 2.1 MB never read (compiled CSS) javascriptsource/ 1.6 MB never read Both consumers take the .mpr and nothing beside it: SnapshotModule opens it, and PerformUpdate takes the reference's model from it while taking the module's bundled widgets from the .mpk — deliberately, because the reference project's widgets/ also holds the blank template's copies. Entries are now model-only (.mpr plus mprcontents/, kept in case a reference ever stays MPR v2). 34 MB -> 14 MB, and warm runs got faster because there is less to copy: the same diff went 13s -> 9s, with the same answer, 21 of 21 elements unchanged and 6 an upgrade would touch. The bound rises 6 -> 12 on the back of it. Twelve is what a six-module update sweep builds, base and target each, so a whole sweep now stays cached at ~170 MB instead of half of it being evicted. This is a constraint on future changes, and isModelFile says so: a cached reference is model-only, so anything that starts reading a sibling directory of the reference .mpr will see it on a cache miss and not on a hit — findings that come and go. The blank-project cache deliberately keeps its whole tree, since `mx module-import` reads all of it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JXnEgoc2NQP1Y2TWMCMXC4 --- .../mendix/download-marketplace-content.md | 2 +- cmd/mxcli/marketplace/refcache.go | 70 ++++++++++++++++--- cmd/mxcli/marketplace/refcache_test.go | 44 ++++++++++++ cmd/mxcli/marketplace/scratch.go | 4 +- docs-site/src/guides/marketplace.md | 29 +++++--- 5 files changed, 129 insertions(+), 20 deletions(-) diff --git a/.claude/skills/mendix/download-marketplace-content.md b/.claude/skills/mendix/download-marketplace-content.md index a64ebc666..5fb771cbe 100644 --- a/.claude/skills/mendix/download-marketplace-content.md +++ b/.claude/skills/mendix/download-marketplace-content.md @@ -263,7 +263,7 @@ Administration — installed 4.3.2 (Mendix 11.12.1) **Tell the user the first one is slow.** Answering needs a reference project — a blank app with the published module imported — and `--to` needs two. Measured -on Administration at 11.12.1: **~50s** the first time, **~13s** afterwards, once +on Administration at 11.12.1: **~47s** the first time, **~9s** afterwards, once `~/.mxcli/marketplace-refs/` holds the blank app and the built references. Run `diff` before `update` rather than instead of it: the `update` reuses the base reference the `diff` just built, so the pair costs little more than the `diff`. diff --git a/cmd/mxcli/marketplace/refcache.go b/cmd/mxcli/marketplace/refcache.go index c5b1099ed..72200e1d6 100644 --- a/cmd/mxcli/marketplace/refcache.go +++ b/cmd/mxcli/marketplace/refcache.go @@ -117,15 +117,19 @@ func safeKey(s string) string { // defaultRefCacheEntries bounds the finished-reference cache. // -// An entry is a whole Mendix project: 34 MB measured for Administration, the -// smallest module in a blank app. Updating six modules builds twelve references, -// so an unbounded cache is ~400 MB of a container that may have 2 GB free — and -// running out of disk mid-update is a far worse outcome than rebuilding a -// reference, because `marketplace update` does not roll back. +// An entry is the model alone (see isModelFile) — 14 MB measured for +// Administration at 11.12.1, against 34 MB for the whole reference project. +// Twelve is what a six-module update sweep builds, base and target each, so the +// default holds a whole sweep at ~170 MB and the `update` that follows a `diff` +// still hits. // -// The blank-project cache is deliberately NOT bounded: it holds one entry per -// Mendix version, and it is the one that pays off on every single build. -const defaultRefCacheEntries = 6 +// It is bounded at all because running out of disk part way through an update is +// a far worse outcome than rebuilding a reference: `marketplace update` does not +// roll back, so a failed write leaves the module already dropped. +// +// The blank-project cache is deliberately NOT bounded: one entry per Mendix +// version, and it is the one that pays off on every single build. +const defaultRefCacheEntries = 12 // refCacheMaxEntries reads the bound, honouring MXCLI_REF_CACHE_MAX. 0 disables // pruning for anyone with disk to spare. @@ -253,12 +257,56 @@ func publishToCache(buildDir, cacheDir string) error { return nil } +// isModelFile reports whether a path inside a reference project is part of the +// MODEL, which is the only thing a cached reference is ever read for. +// +// A reference project is a whole blank Mendix app, and most of it is bulk that +// nothing here touches. Measured on Administration at 11.12.1, a 34 MB entry: +// +// PackageRef.mpr 14 MB read +// widgets/ 9.6 MB never read +// themesource/ 6.4 MB never read +// theme-cache/ 2.1 MB never read (compiled CSS) +// javascriptsource/ 1.6 MB never read +// +// Both consumers take the .mpr and nothing beside it: SnapshotModule opens it, +// and PerformUpdate takes the reference's model from it while taking the +// module's bundled widgets from the .mpk — deliberately, because the reference +// project's widgets/ also holds the blank template's copies. +// +// THIS IS A CONSTRAINT ON FUTURE CHANGES. A cached reference is model-only, so +// anything that starts reading a sibling directory of the reference .mpr will +// see it on a cache miss and not on a hit — a difference that shows up as +// findings that come and go. Extend this filter in the same commit, or store +// the whole tree again. +// +// mprcontents/ is kept even though `mx module-import` always collapses the +// reference to MPR v1: the cost is nothing when it is absent, and the failure if +// that ever changes is an unreadable model rather than a slower run. +func isModelFile(rel string) bool { + if rel == "mprcontents" || strings.HasPrefix(rel, "mprcontents"+string(filepath.Separator)) { + return true + } + return !strings.ContainsRune(rel, filepath.Separator) && strings.HasSuffix(rel, ".mpr") +} + // copyTree copies a directory tree. Used both to seed a build from the cache and // to hand a caller its own copy, so the cached tree is never the one written to. // // Symlinks are copied as symlinks; a Mendix project has none, and following them // would let a crafted package escape the destination. func copyTree(src, dst string) error { + return copyTreeFiltered(src, dst, nil) +} + +// copyTreeModelOnly copies just the model, for the finished-reference cache. +// The blank-project cache deliberately does NOT use this: a blank app is the +// input to `mx module-import`, which reads the whole tree. +func copyTreeModelOnly(src, dst string) error { + return copyTreeFiltered(src, dst, isModelFile) +} + +func copyTreeFiltered(src, dst string, keep func(rel string) bool) error { return filepath.Walk(src, func(path string, info os.FileInfo, err error) error { if err != nil { return err @@ -274,6 +322,12 @@ func copyTree(src, dst string) error { if rel == completeMarker { return nil } + if keep != nil && !keep(rel) { + if info.IsDir() { + return filepath.SkipDir + } + return nil + } target := filepath.Join(dst, rel) switch { diff --git a/cmd/mxcli/marketplace/refcache_test.go b/cmd/mxcli/marketplace/refcache_test.go index 2111c4252..50e3985dc 100644 --- a/cmd/mxcli/marketplace/refcache_test.go +++ b/cmd/mxcli/marketplace/refcache_test.go @@ -273,3 +273,47 @@ func TestPruneDisabledByZero(t *testing.T) { t.Errorf("a malformed bound gave %d, want the default %d", got, defaultRefCacheEntries) } } + +// TestModelOnlyCopyKeepsWhatIsRead pins the filter that makes a reference entry +// ~14 MB instead of ~34 MB. The .mpr and mprcontents/ are the model; everything +// else in a reference project is bulk nothing reads. +// +// The negative half matters as much as the positive: if widgets/ started being +// copied again the cache would quietly double in size, and if the .mpr stopped +// being copied every hit would serve an unreadable reference. +func TestModelOnlyCopyKeepsWhatIsRead(t *testing.T) { + src, dst := t.TempDir(), filepath.Join(t.TempDir(), "out") + + mustWrite(t, filepath.Join(src, "PackageRef.mpr"), "model") + mustWrite(t, filepath.Join(src, "mprcontents", "nested", "unit.mxunit"), "unit") + mustWrite(t, filepath.Join(src, "widgets", "big.mpk"), "bulk") + mustWrite(t, filepath.Join(src, "themesource", "atlas", "style.scss"), "bulk") + mustWrite(t, filepath.Join(src, "theme-cache", "compiled.css"), "bulk") + mustWrite(t, filepath.Join(src, "javascriptsource", "a.js"), "bulk") + + if err := copyTreeModelOnly(src, dst); err != nil { + t.Fatalf("copyTreeModelOnly: %v", err) + } + + if got := readFile(t, filepath.Join(dst, "PackageRef.mpr")); got != "model" { + t.Errorf(".mpr = %q, want %q — a served entry would be unreadable", got, "model") + } + if got := readFile(t, filepath.Join(dst, "mprcontents", "nested", "unit.mxunit")); got != "unit" { + t.Errorf("mprcontents unit = %q, want %q", got, "unit") + } + for _, bulk := range []string{"widgets", "themesource", "theme-cache", "javascriptsource"} { + if _, err := os.Stat(filepath.Join(dst, bulk)); !os.IsNotExist(err) { + t.Errorf("%s/ was copied into the cache entry; nothing reads it", bulk) + } + } + + // And the unfiltered copy still takes everything — it is what seeds a blank + // project, which mx module-import reads in full. + full := filepath.Join(t.TempDir(), "full") + if err := copyTree(src, full); err != nil { + t.Fatalf("copyTree: %v", err) + } + if _, err := os.Stat(filepath.Join(full, "widgets", "big.mpk")); err != nil { + t.Error("the unfiltered copy dropped widgets/; a blank project needs its whole tree") + } +} diff --git a/cmd/mxcli/marketplace/scratch.go b/cmd/mxcli/marketplace/scratch.go index 5d9677e7c..5772be7c3 100644 --- a/cmd/mxcli/marketplace/scratch.go +++ b/cmd/mxcli/marketplace/scratch.go @@ -355,7 +355,9 @@ func CacheReference(versionID, mendixVersion, refDir, mpkPath string) { if err != nil { return } - if err := copyTree(refDir, filepath.Join(staging, entryProject)); err != nil { + // Model only: see isModelFile for what is dropped, why nothing reads it, and + // what that constrains. + if err := copyTreeModelOnly(refDir, filepath.Join(staging, entryProject)); err != nil { _ = os.RemoveAll(staging) return } diff --git a/docs-site/src/guides/marketplace.md b/docs-site/src/guides/marketplace.md index 4f30c79a5..4cdee5c87 100644 --- a/docs-site/src/guides/marketplace.md +++ b/docs-site/src/guides/marketplace.md @@ -206,10 +206,10 @@ the version being moved to. Two caches under `~/.mxcli/marketplace-refs/` keep that off the clock: -| Cache | Keyed by | Saves | -|---|---|---| -| `blank/` | Mendix version | the `mx create-project` in **every** reference build | -| `ref/` | published version UUID + Mendix version | the whole reference, on a repeat | +| Cache | Keyed by | Holds | Saves | +|---|---|---|---| +| `blank/` | Mendix version | a whole blank app (~37 MB) | the `mx create-project` in **every** reference build | +| `ref/` | published version UUID + Mendix version | the model only (~14 MB) | the whole reference, on a repeat | Measured on Administration (content 23513, 4.3.2 → 4.5.0, Mendix 11.12.1): @@ -217,20 +217,29 @@ Measured on Administration (content 23513, 4.3.2 → 4.5.0, Mendix 11.12.1): mxcli marketplace diff 23513 -p app.mpr --to 4.5.0 no cache 66s - cold cache 49s (blank app built once, reused by the second reference) - warm 13s + cold cache 47s (blank app built once, reused by the second reference) + warm 9s ``` +A `ref/` entry stores the `.mpr` (and `mprcontents/`) and nothing else. A +reference project is a whole blank Mendix app, but only its model is ever read — +`widgets/`, `themesource/`, `theme-cache/` and `javascriptsource/` account for +20 MB of 34 MB and are never opened, because an update takes the module's +bundled files from the `.mpk` rather than from the reference. Storing the model +alone made entries 58% smaller and warm runs faster, since less is copied. + The Mendix version is part of both keys, because a reference built at a different version reports Mendix's own conversions as local edits. An entry is only used once its completion marker is present and the project's version stamp has been re-checked on the way out, so a half-written or mislabelled entry is rebuilt rather than trusted. -A reference is about 34 MB, so `ref/` keeps the 6 most recently used entries and -evicts the rest — running out of disk part way through an update is worse than -rebuilding one, because `update` does not roll back. `blank/` is not bounded: it -holds one entry per Mendix version. +`ref/` keeps the 12 most recently used entries and evicts the rest — twelve is +what a six-module update sweep builds, so a whole sweep stays cached at ~170 MB. +It is bounded at all because running out of disk part way through an update is +worse than rebuilding an entry: `update` does not roll back, so a failed write +leaves the module already dropped. `blank/` is not bounded — one entry per +Mendix version. ```bash MXCLI_REF_CACHE_MAX=20 mxcli marketplace diff … # keep more (0 = keep everything) From 5716d8586adc75c4e22083f12835101faab50997 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 10:51:10 +0000 Subject: [PATCH 3/7] fix(devcontainer): install the PostgreSQL server, not just the client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mxcli run --local` needs a real database, and `--ensure-db` provisions one in-container by starting the local PostgreSQL service and creating the app role + database through a `sudo -u postgres` superuser. The generated Dockerfile installed `postgresql-client` only, so on a freshly built `mxcli init` dev container there was no service to start and no superuser — `--ensure-db` failed even though `psql` was on PATH, which made the container look correctly provisioned. Add the `postgresql` server package alongside the client, and assert it in `TestGenerateDockerfile_PostgresServer` for both the docker and podman variants. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EKdcQRPJZxTWq87SH34WYy --- .claude/skills/fix-issue.md | 1 + cmd/mxcli/tool_templates.go | 10 +++++++++- cmd/mxcli/tool_templates_test.go | 17 +++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index bcc4bc2e8..dd544a2f9 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -507,3 +507,4 @@ extracting `OffsetExpression`/`LimitExpression`. | An import activity's Range — Studio Pro's **All / First / Custom** — is absent from `DESCRIBE MICROFLOW`, and a `limit`/`offset` set in Studio Pro does not survive an mxcli round trip | Worse than "undescribed". `Microflows$ImportMappingCall.Range` is polymorphic — `ConstantRange{SingleObject}` (All/First) or `CustomRange{LimitExpression, OffsetExpression}` (Custom) — and mxcli wrote only the first and read only `SingleObject`. So **Custom was unrepresentable**, a bounded import became unbounded on any rewrite, and all three settings described identically, so describe→edit→exec silently changed the activity | `mdl/grammar/domains/MDLMicroflow.g4` (`importMappingRange`) + `MDLLexer.g4` (`FIRST`) + `MDLSettings.g4` (keyword list), `mdl/visitor/visitor_import_export_mapping.go`, `mdl/ast/ast_microflow.go`, `sdk/microflows/microflows_actions.go` (`RangeSingleObject`, `RangeSingleObjectOf`), `mdl/executor/cmd_microflows_builder_calls.go` (`addImportFromMappingAction`), `mdl/executor/cmd_microflows_format_action.go` (`formatImportMappingRange`), `mdl/backend/modelsdk/microflow_write.go` (`importMappingRangeToGen`) + `microflow_read_actions.go`, `sdk/mpr/writer_microflow_actions.go` (`importMappingRange`) + `parser_microflow_actions.go` | **The Range and the result variable's CARDINALITY are separate axes** — the first fix folded them and every `all` wrote a `ListType` against an object-rooted mapping, which mxbuild rejects with **CE0243** ("the mapping used to return 'List of X' but now returns 'X'"). Mendix's own `FeedbackModule.SUB_Feedback_PostToAppInsights` settles it: `ConstantRange{SingleObject:false}` against an **ObjectType** variable. The stored `VariableType` is the authority on cardinality; the range is its own flag (hence `RangeSingleObject *bool`, nil = "not authored, fall back"). **Count the write sites before declaring a serialization fixed** — the `ImportMappingCall` is built at THREE places (`importXmlActionToGen`, the REST `ResultHandlingMapping` case, and the legacy writer), and patching two let a limit reach the model while a `ConstantRange` was still written; extract one helper. **DESCRIBE must emit one of the three, never nothing** — silence re-enters the builder's inference, and an object-rooted mapping set to All (Studio Pro's default) comes back as First. **`first` is not `limit 1`**: one binds an OBJECT, the other a one-element LIST, so they cannot share syntax. **A platform rule you cannot author a positive case for is documentation, not validation** — Mendix rejects `offset` on a non-list mapping with **CE6100** while accepting `limit`, and mxcli cannot currently author a list-rooted import mapping (array-root mappings emit CE5015), so the constraint is documented rather than guessed at in a checker. Tests `mdl/executor/cmd_microflows_import_range_test.go`, `mdl/backend/modelsdk/microflow_import_range_test.go`, `sdk/mpr/parser_microflow_import_range_test.go`, example `mdl-examples/bug-tests/881-import-mapping-range.mdl`. upstream #881 | | `CREATE IMPORT MAPPING` is reported to "not preserve JSON member names": `total` becomes `Total`, `camelCase` becomes `CamelCase`, `__Value` becomes `__ValueItem`. The import then fails at runtime with `key not found: Path(QName(None,),None,)` at `MappingCache.storeValueMappingElement` | The capitalisation is **Mendix's own** and not the defect. A `JsonStructures$JsonElement` stores TWO names — `Path` (the raw JSON key, `(Object)\|uuid`, which the RUNTIME resolves by) and `ExposedName` (derived by capitalising the initial, and suffixing `Item` on an array's item object, which Studio Pro DISPLAYS). The real defect: mxcli's DESCRIBE emits `ExposedName` while its builder resolved only raw keys, so mxcli's own output did not round-trip — re-executing a DESCRIBE FABRICATED a path (`(Object)\|Total`; for arrays the `\|(Object)` item marker vanished entirely, leaving `(Object)\|EntityInstances\|__ValueItem`) and zeroed MaxOccurs. Export mappings had the identical bug. Separately, mxcli wrote `IsDefaultType` on every `ValueMappingElement` (a property only the OBJECT element types own) and cloned the JSON structure's `OriginalValue` — the SAMPLE value from the snippet — onto every mapping element, where Studio Pro leaves it empty | `mdl/executor/cmd_import_mappings.go` (`jsonSchemaIndex`, `newJSONSchemaIndex`, `resolve`, `memberNames`; `buildImportMappingElementModel` now returns an error), `mdl/executor/cmd_export_mappings.go` (same), `mdl/backend/modelsdk/mapping_write.go` + `sdk/mpr/writer_import_mapping.go` + `sdk/mpr/writer_export_mapping.go` (drop `IsDefaultType` from the value writers); `cmd_import_mappings.go` again for the `OriginalValue` clone | **A blank Mendix app is a free Studio-Pro-authored fixture** — `FeedbackModule.IMM_PostResponse` + `JSON_AppInsightsResponse` settle "what does Studio Pro actually write?" with no Studio Pro and no marketplace download: they store `ExposedName "Uuid"` against `Path "(Object)|uuid"`. Author the SAME mapping over the SAME structure in the SAME module with mxcli and diff the two units — the only variable left is which tool wrote it. **Run the reporter's repro as written before believing the diagnosis**: theirs produced paths byte-identical to the structure's, so their MDL was not the failing input; the corruption needed a DESCRIBE in the middle. **DESCRIBE emitting a different name than the parser accepts is a round-trip bug even when both are "correct"** — the fix is to accept BOTH spellings, not to change what DESCRIBE prints, because the exposed name is the one Studio Pro shows. **Never fabricate a path for an unresolved member**: the invented path passed `mxcli check` and surfaced only in mxbuild (CE5015) or at runtime, so the tool that wrote it reported success — refuse, and list the spellings that would have worked. **`generated/metamodel` decides property ownership**: `isDefaultType` is declared on `Import/ExportObjectMappingElement` and on neither `ValueMappingElement`, and per CLAUDE.md's overlay rule an extra property is the shape mxbuild tolerates and Studio Pro will not open — so a green build proves nothing here. **Count the samples before calling a difference a defect**: two Studio-Pro-authored mappings in a blank app (~15 value elements) all write `OriginalValue: ""` while their structures carry 17 non-empty samples, which is what makes "the sample belongs to the structure" a measurement rather than an opinion. **Running the reporter's repro is how you find out it is not a reproduction**: theirs PASSED at runtime on 11.6.6 and 11.13.0, so the runtime error they see needs something their standalone file does not carry — say so instead of claiming the fix. And **control the harness before believing its failure**: an early runtime run failed until the control (Studio Pro's own mapping, same harness) failed identically, which located the fault in the test's missing module-role grants, not in the mapping. Tests `mdl/executor/cmd_mappings_member_resolution_test.go`, `mdl/backend/modelsdk/mapping_isdefaulttype_test.go`, example `mdl-examples/bug-tests/882-mapping-member-names.mdl`. upstream #882 | | `create validation rule …` parses and silently writes nothing, so a regex document cannot be bound to an attribute from MDL at all — every form (regex, range, required, unique, expression) reports success and leaves the model untouched | Three layers, one visible symptom. (1) The grammar rule existed with **no AST node, no visitor and no handler**. (2) Its shape was unimplementable: an `EXPRESSION` rule type Mendix does not have, an **inline** regex literal where Mendix stores a **reference to a document**, and strict `<`/`>` bounds Mendix cannot represent. (3) Underneath, `modelsdk/gen` bound `RegExRuleInfo`'s reference to BSON `RegularExpression` where Studio Pro stores `RegExIdentifier`, so the rule could not have been written even with the statement wired up | `modelsdk/gen/domainmodels/types.go` (`STORAGE-NAME OVERRIDE` on `initRegExRuleInfo` **and** `InitFromRaw`), `mdl/backend/modelsdk/domainmodel.go` (`ruleInfoFromGen`) + `domainmodel_write.go` (`ruleInfoToGen` now takes the whole rule), `sdk/domainmodel/domainmodel.go` (regex ref is a qualified NAME not an ID; Range gained the attribute-bound fields), `mdl/grammar/domains/MDLDomainModel.g4`, `mdl/ast/ast_validationrule.go`, `mdl/visitor/visitor_validationrule.go`, `mdl/executor/cmd_validationrules.go`, `mdl/executor/cmd_entities_describe.go` | **Correcting `modelsdk/gen` IS available** — this supersedes the previous row's "no `supplements.json` or `cmd/modelsdk-codegen` in this tree, so correcting gen is not currently available". Neither path has ever existed here (`git log --all` empty) and `/reference/` is gitignored, but four `STORAGE-NAME OVERRIDE` precedents already hand-patch `init`, and `TestGeneratedCodeIsFormatted` exists because gen gets hand-edited. **`generated/metamodel` is the arbiter**: `cmd/codegen` emits the storage name as the json tag (`json:"regExIdentifier"`) while gen kept the SDK name — an audit of all 570 cross-checkable types found **65 with at least one wrong key**, and the three with independent evidence (`RegularExpression.Expression`, `Attribute.GUID`, this one) were all flagged correctly. **Patch BOTH the encode and decode literal**: reverting only `init` gave a document that writes `RegularExpression` and reads `RegExIdentifier`, whose symptom is a confusing entity-rewrite refusal rather than a bad file. **The control is the proof**: same script, key reverted → `CE0135 "No regular expression specified"` on mxbuild 11.13; with the fix → 0 errors and `RegExIdentifier` on disk. **A rule type is not a rule** — `ruleInfoToGen(ruleType string)` could never rebuild a RegEx (its reference) or a Range (its bounds), so it was widened to the whole rule and the reader taught to carry the payload; a bare RuleInfo of the right `$Type` is a silent downgrade wearing the right name. **Check for a bound that is not a literal**: a Range may point at another ATTRIBUTE, which MDL cannot author and a literals-only reader would have dropped on the next rewrite. **Do not add a second spelling for something already authorable** — Required/Unique stay attribute constraints (`not null error '…'`), refused here with a pointer. **Fixing an unimplementable grammar is free when nothing could depend on it**: every old form parsed and did nothing, so the shape was replaced rather than preserved. Tests `modelsdk/gen/domainmodels/storagename_test.go`, `mdl/backend/modelsdk/validationrule_test.go`, `mdl/visitor/visitor_validationrule_test.go`, `mdl/executor/cmd_validationrules_test.go`, example `mdl-examples/doctype-tests/validation-rules.mdl` | +| `mxcli run --local --ensure-db` fails on a freshly built `mxcli init` dev container — there is no PostgreSQL service to start and no `postgres` superuser — even though `psql` is on PATH | The generated Dockerfile installed `postgresql-client` **only**. `EnsureDatabase` starts a local service (`service postgresql start`) and provisions the role + database through `sudo -u postgres psql`, both of which need the **server** package. `psql` being present makes the container look correctly provisioned | `cmd/mxcli/tool_templates.go` (`generateDockerfile`) | Install `postgresql` alongside `postgresql-client`. **Generalisable**: when a feature shells out to a service, assert the *server* package in the image template, not the client that happens to satisfy a `LookPath` check — the CI/web image having the server is what hid this (`/usr/lib/postgresql/16` is present there but not in the generated dev container). Guarded by `TestGenerateDockerfile_PostgresServer`, which asserts the server package for both the docker and podman variants | diff --git a/cmd/mxcli/tool_templates.go b/cmd/mxcli/tool_templates.go index ea845d67d..871335075 100644 --- a/cmd/mxcli/tool_templates.go +++ b/cmd/mxcli/tool_templates.go @@ -352,7 +352,14 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ return `FROM mcr.microsoft.com/devcontainers/base:bookworm -# Install Adoptium JDK 21 (required by MxBuild), Node.js 22, and utility tools +# Install Adoptium JDK 21 (required by MxBuild), Node.js 22, and utility tools. +# +# The postgresql SERVER (not just -client) is required: the standalone runtime +# 'mxcli run --local' boots needs a real database, and 'mxcli run --local +# --ensure-db' provisions it in-container by starting the local service and +# creating the role + database through a 'sudo -u postgres' superuser. With only +# postgresql-client installed there is no service to start and no superuser, so +# --ensure-db fails on a fresh container. RUN apt-get update && apt-get install -y --no-install-recommends wget apt-transport-https gpg ca-certificates curl && \ wget -qO - https://packages.adoptium.net/artifactory/api/gpg/key/public | gpg --dearmor -o /etc/apt/keyrings/adoptium.gpg && \ echo "deb [signed-by=/etc/apt/keyrings/adoptium.gpg] https://packages.adoptium.net/artifactory/deb bookworm main" > /etc/apt/sources.list.d/adoptium.list && \ @@ -361,6 +368,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends wget apt-transp apt-get install -y --no-install-recommends \ temurin-21-jdk \ nodejs \ + postgresql \ postgresql-client \ kafkacat \ && apt-get clean \ diff --git a/cmd/mxcli/tool_templates_test.go b/cmd/mxcli/tool_templates_test.go index d044dc58f..3b6251f91 100644 --- a/cmd/mxcli/tool_templates_test.go +++ b/cmd/mxcli/tool_templates_test.go @@ -69,6 +69,23 @@ func TestGenerateDockerfile_Podman(t *testing.T) { } } +// TestGenerateDockerfile_PostgresServer guards the database prerequisite of +// `mxcli run --local`: --ensure-db starts a local PostgreSQL service and creates +// the role + database via a `sudo -u postgres` superuser, neither of which exists +// when only postgresql-client is installed. The client alone is easy to mistake +// for enough, so assert the server package specifically. +func TestGenerateDockerfile_PostgresServer(t *testing.T) { + for _, runtime := range []string{"docker", "podman"} { + df := generateDockerfile("MyApp", "App.mpr", runtime) + if !strings.Contains(df, "postgresql \\") { + t.Errorf("%s Dockerfile must install the postgresql SERVER package, not just postgresql-client — 'run --local --ensure-db' cannot provision a database without it", runtime) + } + if !strings.Contains(df, "postgresql-client") { + t.Errorf("%s Dockerfile should keep postgresql-client (psql is used to probe and provision)", runtime) + } + } +} + // TestGenerateDockerfile_PlaywrightArm64 guards the arm64 Playwright provisioning // fix: browsers must be installed via @playwright/cli's bundled playwright-core // (not a transient "npx playwright"), into a world-readable shared cache, with a From 223c072053d4cd932cd707b9ac6f0dfab1948f76 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 10:29:28 +0000 Subject: [PATCH 4/7] docs(proposal): solution-aware `mxcli init` for multi-project solutions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two existing proposals assume a repo where several .mpr projects live one level deep under a single workspace — warm-loop slice 5 (`run --solution`) and the multi-project tree view. Nothing creates that repo: `mxcli init` discovers only the first .mpr in a directory and writes a dev container per project, so initialising two projects yields two competing containers and VS Code attaches to one at a time. Propose `mxcli init --solution`: one root dev container whose forwardPorts follow slice 5's port-triple rule, a root agent context indexing the projects, per-project init without a nested dev container, a SessionStart hook covering every project, and a `mxcli.solution.yaml` skeleton for slice 5 to consume. Scoped deliberately to the repo-shape prerequisite. The orchestration half (manifest schema, `run --solution`, sibling-URL wiring) stays in slice 5 and is listed as a non-goal rather than duplicated. Records two findings for slice 5: `--constant` is now the primitive its constant wiring needs, and app-to-app links must use loopback, since an owner-gated hub answers a cookie-less OData call with a login page. Also documents a bug found while writing this up: the SessionStart hook marker is a substring match, so `init` in a second project sees the first project's hook, reports "already present", and silently never bootstraps the second app. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EKdcQRPJZxTWq87SH34WYy --- docs/11-proposals/README.md | 5 +- .../proposal-solution-aware-init.md | 308 ++++++++++++++++++ 2 files changed, 311 insertions(+), 2 deletions(-) create mode 100644 docs/11-proposals/proposal-solution-aware-init.md diff --git a/docs/11-proposals/README.md b/docs/11-proposals/README.md index a75fd6f3f..9344feb27 100644 --- a/docs/11-proposals/README.md +++ b/docs/11-proposals/README.md @@ -26,7 +26,7 @@ for display in this README): -## Active Proposals (88) +## Active Proposals (90) ### In Progress (partial) (11) @@ -45,7 +45,7 @@ for display in this README): | [Podman Support as Docker Alternative](PROPOSAL_podman_support.md) | Partial | Docker Desktop requires a paid subscription for larger organizations. | | [SHOW/DESCRIBE/USE Building Blocks](show-describe-building-blocks.md) | Partial | Document type: Pages$BuildingBlock (NOT Forms$BuildingBlock — the reader now | -### Proposed (36) +### Proposed (37) | Proposal | Status | Summary | |----------|--------|---------| @@ -80,6 +80,7 @@ for display in this README): | [SHOW/DESCRIBE Rules](show-describe-rules.md) | Proposed | Document type: microflows$rule | | [SHOW/DESCRIBE Scheduled Events](show-describe-scheduled-events.md) | Proposed | Document type: ScheduledEvents$ScheduledEvent | | [SHOW/DESCRIBE Support for Missing Pluggable (React) Widgets](show-describe-pluggable-widgets.md) | Proposed | Scope: Improve DESCRIBE PAGE/SNIPPET output for pluggable widgets that currently fall through to generic formatting | +| [Solution-aware `mxcli init` — one dev container for a multi-project solution](proposal-solution-aware-init.md) | Proposed | A Mendix solution increasingly spans several projects — a frontend and a | | [Unified mxcli & MDL Documentation Site](proposal-documentation-site.md) | Proposed | mxcli has extensive documentation — 119 markdown files, a complete language specification, architecture docs, examples, and user guides — bu | | [Version-Aware MDL and BSON Serialization](version-aware-mdl.md) | Proposed | The Mendix metamodel evolves across versions. | | [VS Code MDL Visualizations](PROPOSAL_vscode_visualizations.md) | Proposed | Add visual diagram previews to the VS Code MDL extension, enabling users to see entity-relationship diagrams, microflow flowcharts, page wir | diff --git a/docs/11-proposals/proposal-solution-aware-init.md b/docs/11-proposals/proposal-solution-aware-init.md new file mode 100644 index 000000000..f754c1f6a --- /dev/null +++ b/docs/11-proposals/proposal-solution-aware-init.md @@ -0,0 +1,308 @@ +--- +title: Solution-aware `mxcli init` — one dev container for a multi-project solution +status: proposed +date: 2026-08-11 +author: Generated with Claude Code +related: + - PROPOSAL_mxcli_dev_warm_loop.md + - PROPOSAL_multi_project_tree.md +--- + +# Proposal: Solution-aware `mxcli init` — one dev container for a multi-project solution + +**Status:** Proposed +**Date:** 2026-08-11 + +## Problem statement + +A Mendix **solution** increasingly spans several projects — a frontend and a +backend joined by OData, or a small service landscape. Two sibling proposals +already assume that shape: + +- [`PROPOSAL_mxcli_dev_warm_loop.md`](PROPOSAL_mxcli_dev_warm_loop.md) **slice 5** + proposes `mxcli run --solution` + `mxcli.solution.yaml` to boot N apps in one + container, and states the expected layout: *"projects live in subdirs of one + repo, e.g. `apps/web/Web.mpr`"*. +- [`PROPOSAL_multi_project_tree.md`](PROPOSAL_multi_project_tree.md) makes the VS + Code tree view multi-project, discovering `//.mpr` — + the same one-level-deep layout. + +Both take that repo layout as a given. **Nothing creates it.** `mxcli init` and +`mxcli new` are single-project throughout, so the container, the session +bootstrap, and the agent context for a solution have to be hand-assembled — and +`init` actively fights the layout, because re-running it inside a second project +writes a second, competing dev container. + +This is the missing prerequisite: slice 5 orchestrates the *runtimes*, the tree +view reads the *models*, and neither can happen until something produces a repo +whose container hosts every project at once. + +### Why this bites now + +Slice 5's premise is one container holding several apps. That is only reachable +if a single dev container is the workspace root. Today, initialising two projects +produces two dev containers in two sibling directories, and VS Code attaches to +one at a time — so the agent editing the frontend cannot see, build, or run the +backend it integrates with. + +### Current state — where the single-project assumption lives + +| Layer | File | Assumption | +|-------|------|-----------| +| **Project discovery** | `cmd/mxcli/init.go` `findMprFile()` | Returns the **first** `.mpr` in the directory, non-recursive | +| **Dev container** | `cmd/mxcli/init.go` (~L477) | `.devcontainer/` is written into the project dir — one per project | +| **Forwarded ports** | `cmd/mxcli/tool_templates.go` `generateDevcontainerJSON()` | `forwardPorts: [8080, 8090, 5432]` — one app's port triple | +| **Session bootstrap** | `cmd/mxcli/init_hook.go` `sessionStartHookCommand()` | Bakes one `-p `; marker-guarded on `"run --local --setup"`, so a second project's hook is **silently skipped** | +| **Agent context** | `cmd/mxcli/tool_templates.go` (`UniversalFiles`, `generateClaudeSettings`, …) | `CLAUDE.md` / `AGENTS.md` stamped with one `projectName` + `mprFile` | +| **CLI binary** | `cmd/mxcli/cmd_new.go` (step 5) | One `./mxcli` per project dir | + +**Encouraging finding:** the dev container is *already* nearly solution-agnostic. +`generateDevcontainerJSON` and `generateDockerfile` both accept an `mprPath` +parameter and **never reference it** — the image (JDK 21, Node 22, PostgreSQL, +Playwright, Claude Code) is entirely generic, and the only project-specific field +in `devcontainer.json` is `"name"` (plus the `PLAYWRIGHT_CLI_SESSION` label). So +this proposal is mostly about *where* files are written and *what* the hook +covers — not about new container machinery. + +## Non-goals — what this proposal does **not** cover + +The user-facing ask that prompted this was "one dev container **and** one run +orchestrator". The orchestrator half is **already proposed** and is deliberately +excluded here: + +| Concern | Where it lives | +|---------|----------------| +| `mxcli.solution.yaml` manifest, `mxcli run --solution`, port auto-allocation, per-app DB provisioning, registration under one `--hub-solution`, sibling-URL constant wiring | warm loop **slice 5** — do not duplicate | +| Multi-project tree view, per-project LSP, cross-project catalog queries | `PROPOSAL_multi_project_tree.md` | +| Per-preview sharing on the hub (external testers) | unproposed — see Open questions | + +This proposal is the **repo-shape prerequisite** for both. It ends where slice 5 +begins: once `mxcli init --solution` has produced the container and the manifest +skeleton, `run --solution` is what reads it. + +## Proposed design + +### Layout + +Adopt the layout both sibling proposals already assume — projects one level deep, +solution-level tooling at the root: + +``` +shop-suite/ # workspace root; the ONLY dev container + .devcontainer/ # generic image + every project's ports + .claude/ # one agent context, sees all projects + CLAUDE.md # solution-level, indexes the projects + mxcli.solution.yaml # skeleton; consumed by slice 5 + ./mxcli # one binary + backend/ + Backend.mpr + .claude/ # per-project skills/commands (no .devcontainer) + CLAUDE.md + frontend/ + Frontend.mpr + .claude/ + CLAUDE.md +``` + +Per-project `.claude/` and `CLAUDE.md` are **kept** — they carry project-specific +model context, and Claude Code composes nested context. What becomes +solution-scoped is exactly the container-shaped state: the dev container, the +session hook, the mxcli binary, and the manifest. + +### `mxcli init --solution` + +```bash +mxcli init --solution # discover *​/*.mpr from the root +mxcli init --solution -p backend/Backend.mpr -p frontend/Frontend.mpr +``` + +Behaviour: + +1. **Discover** projects — each `.mpr` one level deep (mirroring + `findAllMprPaths()` in the tree-view proposal, so discovery is one convention + across the two features). Explicit repeated `-p` overrides discovery. +2. **Write one root `.devcontainer/`** whose `forwardPorts` covers every project's + allocated triple, plus 5432 (see port assignment below). +3. **Write a root `CLAUDE.md`/`AGENTS.md`** that indexes the projects rather than + describing one — "this is a solution of N apps; each has its own CLAUDE.md". +4. **Run the existing per-project init** in each project dir for skills, commands, + and per-project context, but **suppress the dev container** there. This is the + one behavioural change to the existing path, and it wants an explicit flag + (`--no-devcontainer`) rather than an implicit mode, so the single-project path + is untouched. +5. **Emit one SessionStart hook covering every project** (see below). +6. **Write a `mxcli.solution.yaml` skeleton** — the projects, a solution name, and + commented-out placeholders for the inter-app constant wiring slice 5 defines. + `init` writes the skeleton; slice 5 owns the schema and the consumer. + +`mxcli new --solution ` is the greenfield companion, but is **deferred** — +`new` composes `create-project` + `theme` + `init` for one app, and multi-app +creation is a larger change with no user demand yet. `init --solution` on an +existing repo is the path that unblocks slice 5. + +### Port assignment + +Ports must agree between three artifacts: the dev container's `forwardPorts`, +whatever `run --solution` allocates, and any hand-run `run --local`. Slice 5 +already specifies the allocation rule — *"auto-allocates the port triples +(`8080/8090/6543`, `8081/8091/6544`, …)"* — indexed by project order. + +`init --solution` must therefore **use the same rule, not invent one**: forward +`8080+i / 8090+i / 6543+i` for each discovered project, plus 5432. The ordering +that defines `i` is the manifest's project order, which is why `init` writes the +manifest rather than leaving it to the user — the file is what makes the +assignment stable across `init` re-runs and `run --solution` boots. + +`portsAttributes` already covers `8080-8099` and `5432-5499` silently, so only +the `forwardPorts` list changes. + +### SessionStart hook for N projects + +`sessionStartHookCommand` currently emits: + +```sh +test -x ./mxcli && ./mxcli run --local --setup --ensure-db -p App.mpr || true +``` + +For a solution this must prepare **every** project's database and cache the shared +MxBuild/runtime once. Two options: + +- **(a) One command per project**, chained — works today with zero new CLI surface: + ```sh + test -x ./mxcli && for p in backend/Backend.mpr frontend/Frontend.mpr; do \ + ./mxcli run --local --setup --ensure-db -p "$p"; done || true + ``` +- **(b) `run --solution --setup`** — one invocation reading the manifest. Cleaner, + but it is slice 5 surface, so `init` would emit a command that does not exist yet. + +**Recommendation: (a) now, (b) when slice 5 lands.** The marker constant +(`sessionStartHookMarker = "run --local --setup"`) still matches (a), so +idempotency and the "never clobber a user's hooks" guarantee are preserved +unchanged. When slice 5 ships, the marker moves to `--setup` alone and the hook +is rewritten in place. + +Note the current failure mode this fixes: because the marker is a substring match, +running `init` in a second project today finds the first project's hook, concludes +"already present", and returns `changed=false` — the second app never bootstraps +and nothing says so. + +## BSON structure + +**Not applicable.** This feature touches no Mendix documents — it generates repo +scaffolding (`.devcontainer/`, `.claude/`, `CLAUDE.md`, `mxcli.solution.yaml`) and +reads `.mpr` files only to discover their paths and names. No parser or writer +changes, no `$Type` involved, so none of the storage-name hazards in CLAUDE.md +apply. + +## Proposed MDL syntax + +**None.** This is CLI surface (`mxcli init --solution`), not MDL. No grammar, AST, +visitor, or executor changes. + +## Implementation plan + +### Files to modify/create + +| File | Change | +|------|--------| +| `cmd/mxcli/init.go` | `--solution` / `--no-devcontainer` flags; `findMprFile` → `findSolutionProjects()` (one level deep, repeatable `-p` override); skip the `.devcontainer/` write under `--no-devcontainer`; drive the per-project init loop | +| `cmd/mxcli/tool_templates.go` | `generateDevcontainerJSON` takes the project list → `forwardPorts` from the slice-5 allocation rule; drop the unused `mprPath` parameter while touching the signature | +| `cmd/mxcli/init_hook.go` | `sessionStartHookCommand` takes `[]string` of mpr paths and emits the loop form; marker unchanged | +| `cmd/mxcli/init_solution.go` *(new)* | Solution discovery, port allocation, `mxcli.solution.yaml` skeleton writer, root `CLAUDE.md` generator | +| `cmd/mxcli/init_hook_test.go` | Multi-project hook: every project appears; re-running is idempotent; an existing single-project hook is upgraded rather than duplicated | +| `cmd/mxcli/tool_templates_test.go` | `forwardPorts` covers N triples and matches the slice-5 rule | +| `cmd/mxcli/init_solution_test.go` *(new)* | Discovery (one level deep, ignores nested/`deployment/` copies); no `.devcontainer/` written into project dirs; manifest skeleton shape | +| `docs-site/src/tools/devcontainer.md` | Document the solution layout | +| `.claude/skills/mendix/run-local.md` | Document running two apps side by side: the per-app port triples, the per-project defaults that already do not collide, and the loopback rule below | + +### Order of operations + +1. `findSolutionProjects()` + port allocation, with tests — pure functions, no I/O. +2. `--no-devcontainer` on the existing single-project path (no behaviour change by + default; makes step 4 possible). +3. Multi-project SessionStart hook + the silent-skip fix. +4. `--solution` orchestration: root container, root context, per-project init loop. +5. `mxcli.solution.yaml` skeleton — **schema agreed with slice 5 first**, since + slice 5 owns the consumer. +6. Docs. + +Steps 1–3 are independently useful and mergeable; the silent-skip fix in step 3 is +a bug fix that stands alone. + +## Version compatibility + +No Mendix version dependency — this generates repo scaffolding and never reads or +writes model content. No entry in `sdk/versions/mendix-{9,10,11}.yaml`, no +`checkFeature()` gate. + +The generated dev container inherits whatever `mxcli run --local` supports; each +project may target a **different Mendix version**, since MxBuild is cached per +version under `~/.mxcli/mxbuild/{version}` and resolved per project. That is worth +an explicit test — a solution spanning two Mendix minors is a realistic migration +scenario and the shared container must not assume one version. + +## Test plan + +No `mdl-examples/` scripts — there is no MDL surface. Coverage is Go tests plus one +integration check: + +| Test | Asserts | +|------|---------| +| `TestFindSolutionProjects` | One level deep; explicit `-p` overrides; `deployment/` and `node_modules/` ignored; deterministic order (the port allocation depends on it) | +| `TestSolutionPortAllocation` | Triples match slice 5's rule for N projects; `forwardPorts` contains every one plus 5432 | +| `TestSessionStartHook_MultiProject` | Every project appears; idempotent re-run; a pre-existing single-project hook is upgraded, not duplicated | +| `TestSessionStartHook_SecondProjectNotSilentlySkipped` | Regression for the marker substring-match bug | +| `TestInitSolution_NoNestedDevcontainers` | Exactly one `.devcontainer/` exists, at the root | +| `TestInitSolution_ManifestSkeleton` | Manifest lists every project and parses as the slice-5 schema | +| Integration (`-tags integration`) | `init --solution` on a two-project fixture, then `run --local` on each with the allocated ports, both reachable | + +Per the "verified at the layer the symptom lives in" rule, the container itself is +only assertable as generated output here — whether the image *builds* and both apps +*boot* inside it is a dev-container-level check that cannot run in CI today. It +should be recorded as a manual verification step on the PR, not asserted by a unit +test that would only prove the template string. + +## Open questions + +1. **Does the manifest schema belong here or in slice 5?** This proposal writes a + *skeleton*; slice 5 defines the fields and consumes it. If slice 5 is + implemented first, `init --solution` should simply emit its schema. Needs a + decision on ordering before step 5. +2. **Root `CLAUDE.md` vs per-project — how much duplication?** Nested context + composes, but the current generated `CLAUDE.md` is substantial. An index at the + root plus unchanged per-project files is proposed; whether the per-project files + should shrink is unresolved. +3. **`mxcli new --solution`** — deferred. Worth it only if greenfield multi-app + creation is a real workflow rather than a rare one. +4. **Existing single-project repos** — is there a migration path (`init --solution` + detecting an existing project-level `.devcontainer/` and offering to hoist it), + or is hand-editing acceptable for what should be a rare conversion? +5. **Hub previews for external testers.** Adjacent and currently unproposed: the + hub's `authorizePreview` is owner-only, so a solution's previews cannot be shared + with testers who are not the owner without dropping `--require-auth` for the + whole hub. A per-preview allow-list is the obvious shape. Out of scope here, but + it is the thing that makes a running solution *useful* to someone other than its + author. + +## Notes for slice 5 + +Two things learned while implementing the two-app workflow by hand, which slice 5's +constant wiring should absorb: + +- **The primitive already exists.** [`PROPOSAL_constant_values.md`](PROPOSAL_constant_values.md) + (accepted, shipped) defines one precedence chain — `--constant Module.Name=Value` + over the machine store over the configuration over the deployment default — with + the winning layer reported per constant, and `ApplyConstants` to change a value on + a running app. Slice 5's sibling-URL wiring should emit `--constant` values into + that chain rather than inventing a channel, and can rely on its refusal of names + the project does not declare. +- **`--runtime-setting MicroflowConstants=…` is still unguarded.** It replaces the + whole key, dropping every constant the deployment resolved, and the app then 530s + at the first microflow reading one. The constant chain gives users no reason to + reach for it, but nothing stops them; a refusal pointing at `--constant` would be + a cheap guard. +- **Intra-container wiring must use loopback, not the public subdomain.** Slice 5 + offers both. On a hub with `--require-auth` the public URL is owner-gated, and a + server-side OData call carries no session cookie — it receives an HTML login page + where it expected `$metadata`. The manifest should make loopback the default for + app-to-app links and reserve the subdomain for browser-facing values. From 91ce749f62bf3d65611d11a9b53a42be704706e1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 19:45:15 +0000 Subject: [PATCH 5/7] docs(proposal): mxcli developer panel for interaction-scoped runtime analysis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit analyze-runtime.md already covers joining logs, metrics, traces, and the catalog. What it cannot supply is the correlation key: which spans belong to the thing the user just did in the browser. That key exists only in the user's head and reaches the agent as prose, so the agent guesses at a time window and greps. Propose a panel injected into the app under development that captures the interaction — widget, page, time window, XHR, action — plus an in-process OTLP collector so spans are queryable without an external one, plus an mxcli MCP server exposing both to the agent. Deliberately avoids the unverified push API. Sending a prompt into a running Claude Code session needs a "post to session X" surface that is not exposed to an agent today and is outside mxcli's control, so v1 inverts the flow: the panel records, the agent pulls, and answers land in the Claude Code conversation. In-panel replies stay a purely additive follow-up. Two design points worth the space. Correlation via inbound W3C traceparent would be exact, but is unverified — outbound propagation is known to work, which is suggestive, not proof — so a window-based fallback ships first and a spike gates the precise path. And the panel must be gated on the viewer matching Backend.Owner rather than on preview reachability: external testers require --require-auth=false, which would otherwise expose the panel to everyone with the URL. Rejects shipping the panel as a JavaScript action: theme apply is deliberately model-free so it cannot break a build, and a panel written into the model would ship into production artifacts. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EKdcQRPJZxTWq87SH34WYy --- docs/11-proposals/README.md | 10 +- .../proposal-browser-dev-panel.md | 331 ++++++++++++++++++ 2 files changed, 337 insertions(+), 4 deletions(-) create mode 100644 docs/11-proposals/proposal-browser-dev-panel.md diff --git a/docs/11-proposals/README.md b/docs/11-proposals/README.md index 9344feb27..2599436f0 100644 --- a/docs/11-proposals/README.md +++ b/docs/11-proposals/README.md @@ -26,7 +26,7 @@ for display in this README): -## Active Proposals (90) +## Active Proposals (93) ### In Progress (partial) (11) @@ -45,7 +45,7 @@ for display in this README): | [Podman Support as Docker Alternative](PROPOSAL_podman_support.md) | Partial | Docker Desktop requires a paid subscription for larger organizations. | | [SHOW/DESCRIBE/USE Building Blocks](show-describe-building-blocks.md) | Partial | Document type: Pages$BuildingBlock (NOT Forms$BuildingBlock — the reader now | -### Proposed (37) +### Proposed (38) | Proposal | Status | Summary | |----------|--------|---------| @@ -56,6 +56,7 @@ for display in this README): | [Improving LLM Assistance for MDL Code Generation](PROPOSAL_llm_mdl_assistance.md) | Proposed | MDL (Mendix Definition Language) is a custom DSL that does not exist in LLM training data. | | [Large Source Files](refactor-large-files.md) | Proposed | This proposal addresses the refactoring of 6 large non-generated source files to improve maintainability and extensibility, especially for a | | [Mendix Automated Testing Pipeline](proposal-playwright-testing.md) | Proposed | Implementation Proposal for Claude Code | +| [mxcli developer panel — ask the agent about the interaction you just performed](proposal-browser-dev-panel.md) | Proposed | mxcli run --local produces four runtime signals — logs, metrics, traces, and the | | [mxcli Feature Request: Runtime Integration via M2EE Admin API](proposal-runtime-admin-port.md) | Proposed | During development with mxcli + Docker, we reverse-engineered the Mendix runtime's M2EE admin API | | [mxcli microflow debugger — breakpoints by name against a running runtime](PROPOSAL_microflow_debugger.md) | Proposed | Add first-class microflow-debugger support to mxcli: set breakpoints, inspect paused | | [mxcli tunnel-hub — GitHub authentication (hosted hub.mxcli.org)](PROPOSAL_hub_authentication.md) | Proposed | This proposal adds authentication to the multi-tenant tunnel-hub so a hosted | @@ -87,7 +88,7 @@ for display in this README): | [VS Code Search — Quick Pick + Workspace Symbol](PROPOSAL_vscode_search.md) | Proposed | Full-text search exists in mxcli (mxcli search) but is only accessible via the terminal. | | [Workflow Improvements: ALTER WORKFLOW + Cross-References](PROPOSAL_workflow_improvements.md) | Proposed | Workflow support in mxcli has full CREATE/DESCRIBE/DROP/SHOW coverage with 13 activity types and BSON round-trip fidelity. | -### Draft (40) +### Draft (41) | Proposal | Status | Summary | |----------|--------|---------| @@ -121,8 +122,9 @@ for display in this README): | [mxcli auth — Mendix Platform Authentication](PROPOSAL_platform_auth.md) | Draft | A growing set of mxcli features need to talk to Mendix platform APIs on behalf of the user: | | [mxcli catalog — Mendix Catalog Integration](PROPOSAL_catalog_integration.md) | Draft | ⚠️ TERMINOLOGY NOTE: This proposal covers the external Mendix Catalog service at catalog.mendix.com (CLI: mxcli catalog search), which is se | | [mxcli check — mine Mendix's diagnostics catalog to close the check↔mxbuild gap](PROPOSAL_check_diagnostics_catalog.md) | Draft | Relates to: PROPOSAL_check_mxbuild_gap_heuristics.md (one tactical slice of this — | -| [mxcli marketplace diff — detect local modification and plan an ID-preserving module upgrade](PROPOSAL_marketplace_module_upgrade.md) | Draft | Follows on from PROPOSAL_marketplace_modules.md, | +| [mxcli marketplace diff — detect local modification and plan a GUID-preserving module upgrade](PROPOSAL_marketplace_module_upgrade.md) | Draft | 2026-08-11 (DESCRIBE coverage measured — §7; GUID = database identity measured — §8) | | [mxcli Playground](mxcli-playground.md) | Draft | A public GitHub repository (mendixlabs/mxcli-playground) containing a ready-to-use Mendix project pre-configured with mxcli, Claude Code ski | +| [Owning the modelsdk/gen codegen — why the vendored generator cannot be adopted as-is](PROPOSAL_codegen_ownership.md) | Draft | Constraint set by the maintainer (2026-08-13): the reflection data is an | | [Playwright Session Reuse and Lifecycle Control](PROPOSAL_playwright_session_reuse.md) | Draft | Builds on proposal-playwright-cli.md, which | | [Project Brain — Persistent Knowledge and Session Scaffolding for Long-Term AI Collaboration](PROPOSAL_project_brain.md) | Draft | mxcli is developed with significant AI involvement, yet the project's knowledge infrastructure is built for static human reading rather than | | [RENAME with Reference Refactoring](PROPOSAL_rename_refactoring.md) | Draft | Renaming entities, microflows, pages, and modules is one of the most common refactoring operations. | diff --git a/docs/11-proposals/proposal-browser-dev-panel.md b/docs/11-proposals/proposal-browser-dev-panel.md new file mode 100644 index 000000000..75e38a158 --- /dev/null +++ b/docs/11-proposals/proposal-browser-dev-panel.md @@ -0,0 +1,331 @@ +--- +title: mxcli developer panel — ask the agent about the interaction you just performed +status: proposed +date: 2026-08-11 +author: Generated with Claude Code +related: + - PROPOSAL_mxcli_dev_warm_loop.md + - PROPOSAL_mcp_backend.md + - PROPOSAL_hub_authentication.md + - proposal-solution-aware-init.md +--- + +# Proposal: mxcli developer panel — ask the agent about the interaction you just performed + +**Status:** Proposed +**Date:** 2026-08-11 + +## Problem statement + +`mxcli run --local` produces four runtime signals — logs, metrics, traces, and the +model catalog — and [`analyze-runtime.md`](../../.claude/skills/mendix/analyze-runtime.md) +is the procedure for joining them. That skill works. What it cannot supply is the +**correlation key**: which of those spans belong to *the thing the user just did in +the browser*. + +Today that key exists only in the user's head, and reaches the agent as prose: + +> "I clicked Save on the customer form maybe a minute ago and it took ages — +> can you see what happened?" + +The agent then guesses at a time window, greps `runtime.log`, and hopes. The +information that would make the question precise — which widget, which page, which +XHR, at exactly which millisecond, producing exactly which span tree — was present +in the browser and thrown away. + +The proposal is a small panel inside the app under development that **captures that +context at the moment of the interaction** and makes it retrievable by the agent, so +"show me the flame chart caused by pressing this button" becomes an answerable +question rather than an approximate one. + +### Why now + +Three pieces landed that make this tractable, none of which existed when the warm +loop started: + +- `--trace-otlp` produces spans with real timestamps and parent IDs (the console + exporter omits both, so it cannot reconstruct a call tree at all). +- The tunnel-hub reverse-proxies every app response, giving a natural injection point + that requires nothing installed in the tester's browser. +- The hub has an authenticated owner identity (`Backend.Owner`), so a developer-only + surface can be gated on something real. + +## Design principle: the panel records, the agent pulls + +The obvious design is for the panel to send a prompt straight into the Claude Code +session and render the reply inline. **This proposal deliberately does not do that.** + +Pushing a prompt into a running session requires a "post a message to session X" +API. Such a surface may exist — `create_session` in the Claude Code Remote MCP +server references combining it with `send_message` and polling `list_events` — but +it is not exposed to an agent session today, and the in-agent `SendMessage` tool is +not something a web page can call. **It could not be verified while writing this +proposal, and it is outside mxcli's control.** + +So v1 inverts the flow: + +- The panel **records** an interaction into a store mxcli owns. +- The agent **pulls** it, on the user's ask, through an MCP tool. +- The answer appears **in the Claude Code conversation**, not in the panel. + +This loses the in-page reply, which is the most striking part of the pitch. It keeps +everything else, depends on nothing unverified, and leaves the push path as a +strictly additive follow-up (see Open questions). The user still asks in natural +language — they just ask in the place they are already working, and the panel's job +is to make "the interaction I just performed" a precise referent rather than a +description. + +## Architecture + +Three components, each independently useful: + +``` +Browser (app under test) Dev container +┌────────────────────────────┐ ┌──────────────────────────────────┐ +│ Mendix app │ │ Mendix runtime (JVM) │ +│ ┌──────────────────────┐ │ /xas/ ───▶ │ + OTel agent ──┐ │ +│ │ mxcli dev panel │ │ │ ▼ │ +│ │ · records clicks │ │ │ ┌───────────────────────────┐ │ +│ │ · captures XHRs │──┼── POST ─────▶│ │ mxcli run --local │ │ +│ │ · shows what it has │ │ /_mxcli/ │ │ · embedded OTLP collector│ │ +│ └──────────────────────┘ │ event │ │ · interaction store │ │ +└────────────────────────────┘ │ └─────────────┬─────────────┘ │ + ▲ │ │ MCP │ + │ injected by the hub │ ┌─────────────▼─────────────┐ │ + │ (ModifyResponse) │ │ Claude Code agent │ │ + └──────────────────────────────┼──│ get_interactions / trace │ │ + │ └───────────────────────────┘ │ + └──────────────────────────────────┘ +``` + +### 1. Embedded OTLP collector — `run --local --trace-collect` + +`--trace-otlp ` today points at a collector the user must run themselves. +`--trace-collect` instead starts an **in-process OTLP receiver** (http/protobuf on a +loopback port), points the runtime's agent at it, and keeps a bounded ring buffer of +spans in memory. + +This is the single highest-value piece and is **useful with no panel at all** — it +turns "spans go somewhere I have to set up" into "spans are queryable from mxcli", +which is what `analyze-runtime.md` currently sends people to Jaeger for. It should +ship first and alone. + +Bounded by span count and age (both flag-tunable), because the default filters still +let a busy transaction produce a lot of spans, and this buffer lives in the same +process as the dev loop. + +### 2. The panel + +A small self-contained script: a floating toggle, a list of recent interactions, and +a copyable reference for each. It records, per interaction: + +| Field | Source | +|-------|--------| +| Widget label / DOM path, page URL | click handler on `document`, capture phase | +| Timestamp window (start, end) | around the interaction's network activity | +| XHR requests fired | `fetch`/`XMLHttpRequest` wrapper, or `PerformanceObserver` | +| Mendix action name | the `/xas/` request payload | +| `traceparent` (if issued) | generated by the panel — see Correlation | + +It POSTs each to `/_mxcli/event` on the app's own origin, which mxcli serves +alongside the app. It does **not** talk to the agent, hold credentials, or render +agent output in v1. + +### 3. mxcli MCP server — `mxcli mcp serve` + +The agent-facing surface. Exposes the recorded interactions and the collector as MCP +tools: + +| Tool | Returns | +|------|---------| +| `list_interactions` | Recent interactions, newest first — label, page, time window, action | +| `get_interaction` | One interaction with its full captured detail | +| `get_trace` | The span tree for an interaction (or a raw time window), as a call tree with durations | +| `get_logs` | `runtime.log` lines within an interaction's window | + +Note the direction: [`PROPOSAL_mcp_backend.md`](PROPOSAL_mcp_backend.md) makes mxcli +an MCP **client** of Studio Pro's PED server. This is the **reverse** — mxcli as an +MCP *server* the agent connects to. They share no code and do not conflict, but the +naming should be kept clearly distinct (`mcp serve` vs the `--mcp` backend flag) or +the two will be confused permanently. + +## Correlation — the crux + +Tying a browser interaction to its server spans is the part that decides whether this +is precise or merely suggestive. Two mechanisms, and the proposal should implement +the fallback first because it always works. + +**Primary — W3C trace context.** The panel generates a `traceparent` per interaction +and attaches it to the outgoing XHR. If the runtime's OTel agent extracts inbound +trace context, every server span for that request becomes a child of the panel's +trace id, and correlation is exact. + +`analyze-runtime.md` records that *outbound* propagation works — "trace context (W3C +`traceparent`) crosses app boundaries automatically over `rest call`" — which shows +the agent's context plumbing is live, and standard OTel servlet instrumentation does +extract inbound headers by default. **This has not been verified against the Mendix +runtime, and nothing should be designed around it until it has.** It is also the +first thing to test, because if it works the rest of this section is unnecessary. + +Two caveats even when it works: Mendix may reject unexpected headers on `/xas/`, and +the panel adding a header changes the request the app sends — which must not alter +app behaviour. + +**Fallback — time window plus action name.** Record the interaction's start/end and +the `/xas/` action, then select spans overlapping that window on the matching service +name. Approximate: concurrent activity from another tester lands in the same window. +Good enough to answer "what did this button cost", not good enough to answer it on a +busy shared preview. The panel should say which mechanism produced a given answer +rather than presenting both as equally exact. + +## Panel injection — hub `ModifyResponse` + +The hub's app proxy (`cmd/mxcli/tunnelhub/server.go:135`) has a `Director` and no +`ModifyResponse` — it never touches response bodies today. Injection adds one: +rewrite `text/html` responses to include the panel's `