fix(describe): emit workflow annotations as comments, not statements (mendixlabs/mxcli#1007)

.claude/skills/fix-issue.md

@@ -759,6 +759,7 @@ extracting `OffsetExpression`/`LimitExpression`.
 | After `mxcli test … --local`, an app another `mxcli run --local` is serving goes blank while still answering HTTP 200 (~1.7 KB, the Mendix SPA shell); the runtime log shows `Connector: 404 - file not found for file: dist%2Findex.js` and `deployment/web/dist` is gone | `cmd/mxcli/testrunner/localapp_options.go`, `cmd/mxcli/testrunner/runner_local.go` (`localTestDeployDir`), `cmd/mxcli/testrunner/runner.go` (`checkScratchDeploymentExists`) | A local test run already used its own ports and its own `<project>_test` database — the code comment says why, verbatim — but shared the **deployment directory**, which is the one the *browser* reads. A headless test boot does not bundle the web client, so its build left the running app serving the shell over a 404: tests pass, run keeps running, app is blank, nothing reported at either end. **Detection was not the fix**: the two processes use different ports by design, so no port check can see it, and a lock file would only turn a silent blanking into a refusal. The test boot now builds into `<project>/.mxcli/deployment-test/` — gitignored, already where the test runtime log lives — which makes the collision impossible. Note booting a runtime against the shared directory damages it even **without** a rebuild (the packaging step removes the bundle — FINDINGS §35, `ReportLostWebClientBundle`), so "reuse the dev loop's tree read-only" is not an alternative. Consequence to wire: `--skip-build` used to mean "reuse deployment/" and now has nothing until tests have run once, so it is refused with the reason rather than failing inside the runtime boot against a path the user never chose. Reported as mxcli-formula1 FINDINGS §62 |
 | A widget keyword the grammar accepts is absent from `mxcli syntax page widgets`, so it is concluded not to exist and worked around at length (reported for `tabcontainer`, which cost two days and five hand-rolled pages) | `cmd/mxcli/syntax/features_page.go`, `cmd/mxcli/syntax/widget_keywords_drift_test.go` | The lesson the reporter drew — "absence from the documentation is not absence from the grammar" — is true and is a bad thing for the docs to require. `TestEveryWidgetKeywordIsInAPageSyntaxTopic` makes it false instead: it reads the `widgetTypeV3` rule out of the **committed** `.g4` (only the *generated parser* is uncommitted, and the grammar is the authority the reporter was told to consult) and fails when a keyword appears in no `page.*` topic. It found **18**, not one. Exemptions go in `documentedElsewhere` **with the topic that owns them** — layout constructs (`scrollcontainer`, `region`, `navigationtree`, `menubar`, `placeholder`) and pluggable-widget object-list keywords (`group`, `series`, `marker`, …) are not page widgets; an entry with no home is the same defect. The guard carries its own vacuity control: a keyword that does not exist must not match, and one that does must. **Do not document a keyword without running it** — probing all 18 on 11.13 found four the parser accepts and the *default engine refuses* (`statictext`, `staticimage`, `dynamicimage`, `dropdown` → "widget *pages.X not yet supported by the modelsdk engine"), two refused on both engines (`referenceselector`, `legacydatagrid`), and one whose bare form emits **CE0463** (`image`). Reported as mxcli-formula1 FINDINGS §69 |
 | `DESCRIBE WORKFLOW` output fails `mxcli check` — `mismatched input '[%UserRole_Banker%]' expecting ';'` on a `targeting users xpath` line, and every later statement cascades | The emitter wrote `fmt.Sprintf("… '%s'", v)` — quotes in the format string, escaping (if any) at the call site. **6 of 23 emit sites did not escape**: both xpath variants, the user-task caption, the workflow-level due date, and both outcome values | `mdl/executor/cmd_workflows.go` (all 23 sites), `mdl/executor/identifier_quoting.go` (`mdlQuoted`) | **Return the quotes WITH the escaping** — `mdlQuoted(s)` yields `'…''…'` complete, so an unescaped emit cannot be written by omission. Escaping at the call site is one thing to remember per site, and the count of sites only grows. Assert the emitted MDL **parses** (wrap the fragment in a minimal `create workflow … end workflow;` and run `visitor.Build`), never that it contains a particular escape — a substring assertion encodes the very escape under test, so it passes for the wrong reason. Route the harness through `formatWorkflowActivities`, not the per-activity formatter: the statement terminator is appended by the caller, so calling the formatter directly produces unparseable output for reasons unrelated to the bug. Emit tests only cover positions the test constructs, so add a **source scan** for a literal `'%s'` in the describers — the real failure mode is a *new* site added later, and `mdlQuoted` carrying its own quotes is what makes that scan sound. The reported symptom was the XPath (its payload is full of quoted constraints, so *every* XPath-targeted user task hits it); the caption one needs only an apostrophe — `Manager's review`. mendixlabs/mxcli#1006 |
+| `DESCRIBE WORKFLOW` output is refused by `mxcli check` with **MDL-WF04** — "a standalone `annotation` … produces a model Mendix cannot load"; 13 errors from one unmodified describe of a 23-activity workflow | The describer emitted `annotation '<text>';` — the exact construct MDL-WF04 exists to refuse, and that `execCreateWorkflow` refuses again. Two parts of the tree disagreed **in comments**: the emitter said "emitted as a parseable MDL statement so it survives round-trips", the validator said it cannot be loaded. The emitter's comment was the stale one | `mdl/executor/cmd_workflows.go` (`formatAnnotation` + the `WorkflowAnnotationActivity` branch), refusals in `mdl/executor/validate_workflow.go` and `cmd_workflows_write.go` | **When a describer and a validator disagree, one of them has a stale comment — read both before choosing a side.** The reporter framed these as canvas annotations; they are not. `formatAnnotation` is called at 10 sites, always with an activity's **attached** `Annotation`, and describe converted attached → standalone, which is the refused form. That distinction changes the fix: the issue's "support writing annotations" needs a grammar change, because `MDLWorkflow.g4` has only the standalone `workflowAnnotationStmt` — **no MDL input can express an attached annotation today**, even though the write path stores one. So a comment loses nothing that was reachable. Note the asymmetry that caused this: the **microflow** domain has `@annotation 'text'` as an activity prefix and round-trips it properly; giving workflow activities the same prefix is the non-lossy fix, and is a feature, not this bug. Two traps: a `--` comment runs to end of line, so a multi-line annotation must be prefixed **per line** or the tail becomes stray tokens (the same failure the statement form had), and the standalone branch must set `isComment` or the terminator logic appends `;` to a comment. Assert the emitted MDL **parses AND passes ValidateWorkflow AND still contains the text** — checking only that the keyword is gone also passes for an emit that dropped the annotation entirely. mendixlabs/mxcli#1007 |
 | Every `IMAGE` widget mxcli authors on Mendix 11.13 fails `mx check` with **CE0463** "the definition of this widget has changed", including the exact `pluggablewidget` form an existing bug-test documents as fixed. `mxcli fix widgets` clears it | `mdl/executor/widget_engine.go` (`hiddenUnnamedProperties`, the mapping loop) | **Case B**, established before any hypothesis (diagnose-ce0463 Step 0): the baseline blank project ships **10 Studio Pro-authored widgets of the same widget id** and reports **0** CE0463; one mxcli-authored Image makes it 1. So the tool is the variable — and those 10 are a known-good reference in the same project, better than a template extraction. The exhaustive path diff came back with **every path present on both sides** and four differing values, two of them content; the `Type` (PropertyTypes schema) subtree and the `TypePointer`→`PropertyKey` mapping were identical. **Two hypotheses tested and falsified, which is why they are recorded**: (a) *key order* — mxcli's node is not in the reference's alphabetical order, a documented CE0463 cause, but the same page's mxcli-authored Datagrid and Badge share that order and pass; (b) *the width value* — authoring `Width: 48` explicitly passes, so 48 is not rejected. The cause is the interaction: the widget **hides `width` when `widthUnit` is "auto"**, and a hidden property must hold its DECLARED default. The engine *skipped* the mapping for a hidden property, which leaves the widget **template's captured value** — image.json holds 48 while its own `ValueType.DefaultValue` says 100. mxcli's own **MDL-WIDGET10** already said so verbatim ("a non-default value there fails the build with CE0463 (the default is \"100\")"), so `check` refused what `exec` emitted: the fix makes the writer read `widgetPropertyDefaults`, the checker's own source, so the two cannot disagree again. The engine's comment already stated the invariant correctly — "the hidden ones **at their default**, so hidden means default-valued, not absent" — and skipping only coincides with that when the template happens to be at the default. Where no default can be looked up (a datasource has none) it still skips, so #956's File Uploader pruning is unchanged. **Control**: stub the `SetPrimitive` and CE0463 returns end-to-end. Follow-on the fix *revealed* (CE0463 was masking it): the default `ImageType: image` needs an image-collection entry MDL cannot name, so the bare form builds to Mendix's own "No image selected." — now **MDL-WIDGET22** at check time, and it found the same breakage in four of the repo's own examples. Reported as mxcli-formula1 FINDINGS §69/§142 |
 | A `RETRIEVE … WHERE a = $Var/Attr AND b = $Var/Attr` passes `mxcli check` and the build fails **CE0161** "Error(s) in XPath constraint" — but the same statement with **literals** on both sides of the same uppercase `AND` builds fine | `mdl/visitor/xpath_operators.go` (new, `NormalizeXPathOperators`), called from `mdl/visitor/xpath_format.go` | XPath 1.0 spells `and`/`or`/`not` in **lower case only**; MDL's lexer accepts any case (`AND: A N D;`). mxcli lowercased the operator on **one of two rendering paths**: `expressionToXPath` does it while walking the parse tree, but `buildRetrieveWhereExpression` freezes the **raw source** whenever the clause contains a `/` — which every variable path has — and `expressionToXPath`'s `SourceExpr` case hands that text back verbatim. So the casing survived exactly when a path was present, which is why the reporter's literal-only reproducer built cleanly and the report looked not-reproducible. **The correlation was the whole difficulty**: a workaround found under time pressure records what you changed, not what was wrong. Fixed at `FormatXPathConstraint`, the single choke point all three constraint writers share (retrieve, page data source, entity access rule — the latter two measured as broken the same way before the fix), and **before** its width test, because the short branch returns the caller's own bytes. The replacement is token-based and literal-aware for two reasons that are each a worse bug than the one being fixed: rewriting inside `'A AND B'` silently changes which rows match, and an identifier that merely contains the letters (`Brand`, `Andrew`, `NOTES`, `Order_Andon`, `Module.Handover`) is not an operator. `div`/`mod` are deliberately excluded — nothing in MDL emits them, and a rewrite nothing needs can only be wrong. Example `mdl-examples/bug-tests/f1-80-xpath-operator-case.mdl`. Unrelated and pre-existing, found alongside: `$currentUser/...` in a **page** data source constraint is CE0161 regardless of operator case. Reported as mxcli-formula1 FINDINGS §80 |
 | An `image` widget mxcli wrote shows nothing, and `mx check` fails with **"No image selected."**; and a `describe` → rename → `exec` copy of an Atlas layout (or any page with a brand image) comes out with the image gone, describe having emitted `image staticImage1 (Responsive: false)` with no reference at all | `mdl/executor/widget_engine.go` (the `image` operation), `mdl/executor/widget_defs.go` + `mdl/executor/widget_registry.go` (`operationForType`, `defaultKnownOperations`), `mdl/backend/widgetobj/builder.go` (`SetImage`, `setImageValue`), `mdl/backend/mutation.go` + `mdl/backend/mcp/widget.go`, `{modelsdk,sdk}/widgets/definitions/image.def.json` (`imageObject` → `Image`), `mdl/executor/cmd_pages_describe_pluggable.go` (`extractCustomWidgetPropertyImage`) + `cmd_pages_describe_output.go` (`describeImageWidgetProps`) | MDL had no spelling for **which** image an image widget shows, so its default source (`ImageType: image`, an image collection entry) could never be satisfied and describe had nothing to emit — mxcli-formula1 FINDINGS §142. The name is three parts like an icon reference, `Module.Collection.Image`, stored as a plain string on the `WidgetValue`'s `Image` key. Both halves are needed: with only the write, a round trip still loses it. `setImageValue` **replaces** the existing `Image` key and never adds one — adding a key the widget definition does not declare is the CE0463 shape. Controls: stub `setImageValue` → `TestSetImageValue_SetsTheQualifiedName` fails; stub the describe emission → `TestDescribeImageWidget_EmitsTheImageReference` fails |

mdl/executor/cmd_workflows.go

@@ -235,13 +235,50 @@ func describeWorkflowToString(ctx *ExecContext, name ast.QualifiedName) (string,
 	return strings.Join(lines, "\n"), nil, nil
 }
 
-// formatAnnotation returns an ANNOTATION statement for a workflow activity annotation.
-// The annotation is emitted as a parseable MDL statement so it survives round-trips.
+// formatAnnotation renders an activity's annotation as MDL comment lines.
+//
+// It used to emit `annotation '<text>';`, and its own doc comment claimed that
+// statement "survives round-trips". It does not, and has not since MDL-WF04: a
+// standalone `annotation` in a workflow body is refused at check time AND by
+// execCreateWorkflow, because Mendix constructs every child of the activity flow
+// with a Flow parent and no annotation type takes one — the written unit cannot
+// be LOADED, so Studio Pro will not open the project. The describer was emitting
+// the one construct the writer refuses, and a 23-activity workflow produced 13
+// MDL-WF04 errors from unmodified DESCRIBE output (mendixlabs/mxcli#1007).
+//
+// A comment is the honest emit today, not a workaround. The annotation being
+// re-emitted here is ATTACHED to an activity, and although the write path stores
+// an attached annotation (addActivityBaseFields), no MDL input can produce one:
+// MDLWorkflow.g4 has only the standalone `workflowAnnotationStmt`. So the text is
+// unwritable either way, and carrying it as a comment at least keeps it in front
+// of whoever edits the script. The `annotation:` marker says what the line was.
+//
+// The microflow domain does have an attached form (`@annotation 'text'`, see
+// MDLMicroflow.g4) and it round-trips properly. Giving workflow activities the
+// same prefix is the fix that would preserve the annotation rather than
+// commenting it out; it is a grammar change, and deliberately not bundled here.
 func formatAnnotation(annotation string, indent string) string {
 	if annotation == "" {
 		return ""
 	}
-	return fmt.Sprintf("%sannotation %s;", indent, mdlQuoted(annotation))
+	return annotationComment(annotation, indent)
+}
+
+// annotationComment renders text as one or more `-- annotation:` lines. An
+// annotation may contain newlines, and a `--` comment runs to end of line, so a
+// multi-line note has to be prefixed line by line or everything after the first
+// newline becomes stray tokens — the same failure the statement form had.
+func annotationComment(text, indent string) string {
+	lines := strings.Split(text, "\n")
+	for i, l := range lines {
+		l = strings.TrimRight(l, "\r")
+		if i == 0 {
+			lines[i] = indent + "-- annotation: " + l
+			continue
+		}
+		lines[i] = indent + "--   " + l
+	}
+	return strings.Join(lines, "\n")
 }
 
 // boundaryEventKeyword maps an EventType string to the MDL BOUNDARY EVENT keyword sequence.
@@ -360,12 +397,15 @@ func formatWorkflowActivities(flow *workflows.Flow, indent string) []string {
 			// Skip - auto-generated by Mendix, implicit in MDL syntax
 			continue
 		case *workflows.WorkflowAnnotationActivity:
-			// Standalone annotation (sticky note) - emit as ANNOTATION statement
-			if a.Description != "" {
-				actLines = []string{fmt.Sprintf("%sannotation %s", indent, mdlQuoted(a.Description))}
-			} else {
+			// A standalone annotation (sticky note) read back from the model. Emitted
+			// as a comment for the same reason as an attached one: the `annotation`
+			// statement it used to produce is refused by MDL-WF04 and by exec, so the
+			// describe output could not be re-run (mendixlabs/mxcli#1007).
+			if a.Description == "" {
 				continue
 			}
+			isComment = true
+			actLines = []string{annotationComment(a.Description, indent)}
 		case *workflows.GenericWorkflowActivity:
 			isComment = true
 			caption := a.Caption