From d41832bdaafc2fadbad3134eca8af412f270ec31 Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 31 Aug 2026 18:53:12 +0000 Subject: [PATCH] fix(describe): emit workflow annotations as comments, not statements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DESCRIBE WORKFLOW emitted `annotation '';` — the exact construct MDL-WF04 exists to refuse, and that execCreateWorkflow refuses again. So the describer produced output mxcli's own checker rejects: 13 MDL-WF04 errors from one unmodified describe of a 23-activity workflow. Two parts of the tree disagreed, each stating its position in a comment. The emitter: "emitted as a parseable MDL statement so it survives round-trips." The validator: it "produces a model Mendix cannot load (the annotation is placed in the activity flow, which accepts only flow elements)". The emitter's comment was the stale one. The reporter read these as canvas annotations. They are not: formatAnnotation is called at 10 sites, always with an activity's ATTACHED Annotation, and the describer converted attached to standalone — which is the refused form. That matters for the fix, because MDLWorkflow.g4 has only the standalone workflowAnnotationStmt: no MDL input can express an attached annotation, even though the write path stores one. Commenting it out therefore loses nothing that was reachable. Both emit paths become comments — the attached one and the standalone WorkflowAnnotationActivity read back from a model. The standalone branch also has to mark itself a comment, or the terminator logic appends `;`. The microflow domain already has an attached form (`@annotation 'text'`) that round-trips properly. Giving workflow activities the same prefix would preserve the annotation instead of commenting it out; that is a grammar change and deliberately not bundled here. Tests assert the emitted MDL parses, passes ValidateWorkflow, AND still contains the text — the last one because dropping the annotation entirely would satisfy the first two. Reported as mendixlabs/mxcli#1007. Co-Authored-By: Claude Opus 5 --- .claude/skills/fix-issue.md | 1 + mdl/executor/cmd_workflows.go | 54 ++++++- .../issue1007_annotation_emit_test.go | 140 ++++++++++++++++++ 3 files changed, 188 insertions(+), 7 deletions(-) create mode 100644 mdl/executor/issue1007_annotation_emit_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 955b0735c..2c8b025ac 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -759,3 +759,4 @@ 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 `_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 `/.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 '';` — 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 | diff --git a/mdl/executor/cmd_workflows.go b/mdl/executor/cmd_workflows.go index e9d3b1da1..2b48da69e 100644 --- a/mdl/executor/cmd_workflows.go +++ b/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 '';`, 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 diff --git a/mdl/executor/issue1007_annotation_emit_test.go b/mdl/executor/issue1007_annotation_emit_test.go new file mode 100644 index 000000000..3ee58aea7 --- /dev/null +++ b/mdl/executor/issue1007_annotation_emit_test.go @@ -0,0 +1,140 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/sdk/workflows" +) + +// mendixlabs/mxcli#1007 — DESCRIBE WORKFLOW emitted `annotation '';`, the +// one construct MDL-WF04 exists to refuse. The describer and the validator +// disagreed, each stating its position in a comment, and the describer's was the +// stale one. +// +// The assertion is the full contract, not just "no annotation statement": the +// emitted MDL must PARSE and must pass ValidateWorkflow. Checking only that the +// keyword is gone would also pass for an emit that dropped the text entirely. + +// describeAndValidate emits the activities through the real describe path, wraps +// them in the smallest containing workflow, and returns the parse errors and the +// validator's rule IDs. +func describeAndValidate(t *testing.T, acts ...workflows.WorkflowActivity) (src string, parseErrs []string, ruleIDs []string) { + t.Helper() + lines := formatWorkflowActivities(&workflows.Flow{Activities: acts}, " ") + src = "create workflow M.WF\n parameter $WorkflowContext: M.E\nbegin\n" + + strings.Join(lines, "\n") + "\nend workflow;" + prog, errs := visitor.Build(src) + for _, e := range errs { + parseErrs = append(parseErrs, e.Error()) + } + if len(parseErrs) > 0 { + return src, parseErrs, nil + } + for _, stmt := range prog.Statements { + if wf, ok := stmt.(*ast.CreateWorkflowStmt); ok { + for _, v := range ValidateWorkflow(wf) { + ruleIDs = append(ruleIDs, v.RuleID) + } + } + } + return src, nil, ruleIDs +} + +// An annotation ATTACHED to an activity — the reporter's case. 13 of these came +// out of one 23-activity workflow. +func TestDescribeWorkflow_AttachedAnnotationPassesOwnCheck(t *testing.T) { + jump := &workflows.JumpToActivity{TargetActivity: "Review"} + jump.Name = "j1" + jump.Annotation = "Source: Receive + Set busState = New Opening" + + src, parseErrs, rules := describeAndValidate(t, jump) + if parseErrs != nil { + t.Fatalf("emitted MDL does not parse: %v\n%s", parseErrs, src) + } + for _, r := range rules { + if r == "MDL-WF04" { + t.Errorf("describe output still trips MDL-WF04:\n%s", src) + } + } + // The text must survive as something a reader can see — dropping it silently + // would satisfy the two checks above. + if !strings.Contains(src, "Source: Receive + Set busState = New Opening") { + t.Errorf("the annotation text was dropped:\n%s", src) + } + if strings.Contains(src, "annotation '") { + t.Errorf("still emitting an `annotation` statement:\n%s", src) + } +} + +// A standalone annotation (a canvas sticky note) read back from the model, which +// goes through a different branch of formatWorkflowActivities and had the same +// bug. Note the terminator: the branch must mark itself a comment, or `;` gets +// appended to the last line. +func TestDescribeWorkflow_StandaloneAnnotationPassesOwnCheck(t *testing.T) { + ann := &workflows.WorkflowAnnotationActivity{Description: "sticky note on the canvas"} + + src, parseErrs, rules := describeAndValidate(t, ann) + if parseErrs != nil { + t.Fatalf("emitted MDL does not parse: %v\n%s", parseErrs, src) + } + for _, r := range rules { + if r == "MDL-WF04" { + t.Errorf("describe output still trips MDL-WF04:\n%s", src) + } + } + if !strings.Contains(src, "sticky note on the canvas") { + t.Errorf("the annotation text was dropped:\n%s", src) + } + if strings.Contains(src, "; --") || strings.Contains(src, "note;") { + t.Errorf("a terminator was appended to a comment line:\n%s", src) + } +} + +// An annotation may contain newlines, and `--` runs to end of line — so every +// line needs its own prefix or the tail becomes stray tokens, which is the same +// failure mode the statement form had. +func TestDescribeWorkflow_MultiLineAnnotationCommentsEveryLine(t *testing.T) { + jump := &workflows.JumpToActivity{TargetActivity: "Review"} + jump.Name = "j1" + jump.Annotation = "first line\nsecond line\nthird line" + + src, parseErrs, _ := describeAndValidate(t, jump) + if parseErrs != nil { + t.Fatalf("multi-line annotation does not parse: %v\n%s", parseErrs, src) + } + for _, want := range []string{"first line", "second line", "third line"} { + if !strings.Contains(src, want) { + t.Errorf("%q missing from:\n%s", want, src) + } + } + for _, line := range strings.Split(src, "\n") { + for _, part := range []string{"second line", "third line"} { + if strings.Contains(line, part) && !strings.Contains(line, "--") { + t.Errorf("continuation line is not commented: %q", line) + } + } + } +} + +// The control for the whole change: an activity with no annotation must emit +// exactly what it did before, with no stray comment line. +func TestDescribeWorkflow_NoAnnotationEmitsNoComment(t *testing.T) { + jump := &workflows.JumpToActivity{TargetActivity: "Review"} + jump.Name = "j1" + + src, parseErrs, rules := describeAndValidate(t, jump) + if parseErrs != nil { + t.Fatalf("parse: %v\n%s", parseErrs, src) + } + if len(rules) > 0 { + t.Errorf("unexpected violations %v for a plain jump:\n%s", rules, src) + } + if strings.Contains(src, "annotation") { + t.Errorf("emitted an annotation for an activity that has none:\n%s", src) + } +}