From 9e52cd0747a99a092cb1be6c2c5f081e1eae1a7e Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 1 Oct 2026 17:25:12 +0000 Subject: [PATCH 1/5] refactor(check): one script's check returns its exit code instead of exiting The check command's body becomes runCheckFile, which returns the exit code where it called os.Exit. Behaviour is unchanged for the one file check takes; it lets a caller check several files and combine their codes (ako/mxcli#905). Co-Authored-By: Claude Opus 5.5 --- cmd/mxcli/cmd_check.go | 554 +++++++++++++++++++++-------------------- 1 file changed, 280 insertions(+), 274 deletions(-) diff --git a/cmd/mxcli/cmd_check.go b/cmd/mxcli/cmd_check.go index 0b1a5d227f..3743e7a4d1 100644 --- a/cmd/mxcli/cmd_check.go +++ b/cmd/mxcli/cmd_check.go @@ -105,311 +105,317 @@ Examples: `, Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - filePath := args[0] - projectPath, _ := cmd.Flags().GetString("project") - // A project makes reference resolution possible, so it runs. It used to - // need --references as well, which meant `mxcli check script.mdl -p - // app.mpr` printed an unqualified "Check passed!" having resolved - // nothing — icons, entity and page references all silently unchecked. - // Someone who hands the command a project has said what they want; the - // flag stays accepted so existing invocations and scripts keep working. - checkRefs, _ := cmd.Flags().GetBool("references") - checkRefs = checkRefs || projectPath != "" - postMigration, _ := cmd.Flags().GetBool("post-migration") - depPolicy := deprecationPolicy(cmd) - format := resolveFormat(cmd, "text") - isStructured := format != "" && format != "text" - - outputFormat := linter.OutputFormat(format) - formatter := linter.GetFormatter(outputFormat, !isStructured) - - // In a structured format the payload is ONE document on stdout, emitted - // once at the end (or at the first failing phase). Each phase used to - // format its own violations to stderr, so `check --format json` put - // nothing parseable on stdout — only the executor's "Connected to:" - // chatter — and a run reaching several phases wrote several documents. - var structured []linter.Violation - finish := func(code int) { - if isStructured { - formatter.Format(structured, os.Stdout) - } - if code != 0 { - os.Exit(code) - } + if code := runCheckFile(cmd, args[0]); code != 0 { + os.Exit(code) } + }, +} - // Read the script (a path, or "-" for stdin) - content, err := readMDLSource(filePath) - if err != nil { - fmt.Fprintf(os.Stderr, "Error reading file: %v\n", err) - os.Exit(1) +// runCheckFile checks one script and returns the exit code: 0 when it passed. +func runCheckFile(cmd *cobra.Command, filePath string) int { + projectPath, _ := cmd.Flags().GetString("project") + // A project makes reference resolution possible, so it runs. It used to + // need --references as well, which meant `mxcli check script.mdl -p + // app.mpr` printed an unqualified "Check passed!" having resolved + // nothing — icons, entity and page references all silently unchecked. + // Someone who hands the command a project has said what they want; the + // flag stays accepted so existing invocations and scripts keep working. + checkRefs, _ := cmd.Flags().GetBool("references") + checkRefs = checkRefs || projectPath != "" + postMigration, _ := cmd.Flags().GetBool("post-migration") + depPolicy := deprecationPolicy(cmd) + format := resolveFormat(cmd, "text") + isStructured := format != "" && format != "text" + + outputFormat := linter.OutputFormat(format) + formatter := linter.GetFormatter(outputFormat, !isStructured) + + // In a structured format the payload is ONE document on stdout, emitted + // once at the end (or at the first failing phase). Each phase used to + // format its own violations to stderr, so `check --format json` put + // nothing parseable on stdout — only the executor's "Connected to:" + // chatter — and a run reaching several phases wrote several documents. + var structured []linter.Violation + finish := func(code int) int { + if isStructured { + formatter.Format(structured, os.Stdout) } - - // Parse the script - if !isStructured { - fmt.Printf("Checking syntax: %s\n", mdlSourceLabel(filePath)) + return code + } + + // Read the script (a path, or "-" for stdin) + content, err := readMDLSource(filePath) + if err != nil { + fmt.Fprintf(os.Stderr, "Error reading file: %v\n", err) + return 1 + } + + // Parse the script + if !isStructured { + fmt.Printf("Checking syntax: %s\n", mdlSourceLabel(filePath)) + } + + // A .test.mdl / .test.md file is not top-level MDL: each block is a + // microflow body. Render it as the microflows it becomes, on the source's + // own lines, so every rule below applies to what the author actually wrote + // (mendixlabs/mxcli#1103). + source := string(content) + var testProblems []linter.Violation + if testrunner.IsTestFile(filePath) { + checked, terr := testrunner.CheckSource(source, filePath) + if terr != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", terr) + return 1 + } + source = checked.MDL + // A rendering with nothing in it means the file declares no @test + // block. That is not #618's "the parser could not begin reading it" + // — the parser was handed an empty rendering, not the author's text + // — so it gets its own message rather than one quoting a line that + // was never parsed. + if strings.TrimSpace(source) == "" && strings.TrimSpace(string(content)) != "" { + fmt.Fprintln(os.Stderr, noTestsDeclaredError(mdlSourceLabel(filePath))) + return 1 } + for _, p := range checked.Problems { + testProblems = append(testProblems, linter.Violation{ + RuleID: "MDL-TEST01", + Severity: linter.SeverityError, + Message: fmt.Sprintf("test %q: %s", p.Test, p.Message), + Location: linter.Location{DocumentType: "test", DocumentName: p.Test}, + }) + } + } - // A .test.mdl / .test.md file is not top-level MDL: each block is a - // microflow body. Render it as the microflows it becomes, on the source's - // own lines, so every rule below applies to what the author actually wrote - // (mendixlabs/mxcli#1103). - source := string(content) - var testProblems []linter.Violation - if testrunner.IsTestFile(filePath) { - checked, terr := testrunner.CheckSource(source, filePath) - if terr != nil { - fmt.Fprintf(os.Stderr, "Error: %v\n", terr) - os.Exit(1) - } - source = checked.MDL - // A rendering with nothing in it means the file declares no @test - // block. That is not #618's "the parser could not begin reading it" - // — the parser was handed an empty rendering, not the author's text - // — so it gets its own message rather than one quoting a line that - // was never parsed. - if strings.TrimSpace(source) == "" && strings.TrimSpace(string(content)) != "" { - fmt.Fprintln(os.Stderr, noTestsDeclaredError(mdlSourceLabel(filePath))) - os.Exit(1) - } - for _, p := range checked.Problems { - testProblems = append(testProblems, linter.Violation{ - RuleID: "MDL-TEST01", + prog, errs := visitor.Build(source) + if len(errs) > 0 { + if isStructured { + var parseViolations []linter.Violation + for _, parseErr := range errs { + parseViolations = append(parseViolations, linter.Violation{ + RuleID: "MDL-SYNTAX", Severity: linter.SeverityError, - Message: fmt.Sprintf("test %q: %s", p.Test, p.Message), - Location: linter.Location{DocumentType: "test", DocumentName: p.Test}, + Message: parseErr.Error(), }) } - } - - prog, errs := visitor.Build(source) - if len(errs) > 0 { - if isStructured { - var parseViolations []linter.Violation - for _, parseErr := range errs { - parseViolations = append(parseViolations, linter.Violation{ - RuleID: "MDL-SYNTAX", - Severity: linter.SeverityError, - Message: parseErr.Error(), - }) - } - structured = append(structured, parseViolations...) - finish(1) - } else { - fmt.Fprintf(os.Stderr, "Syntax errors found:\n") - for _, err := range errs { - fmt.Fprintf(os.Stderr, " - %v\n", err) - } - // Hint: if script contains IMPORT/QUERY with single $ but not $$, suggest dollar-quoting - src := source - if (strings.Contains(src, "IMPORT") || strings.Contains(src, "import")) && - (strings.Contains(src, "QUERY") || strings.Contains(src, "query")) && - strings.Contains(src, "$") && !strings.Contains(src, "$$") { - fmt.Fprintf(os.Stderr, "\nHint: SQL queries in IMPORT statements should use dollar-quoting ($$...$$) instead of single quotes.\n") - fmt.Fprintf(os.Stderr, " Example: IMPORT FROM alias QUERY $$SELECT * FROM table$$ INTO Module.Entity MAP (...)\n") - } + structured = append(structured, parseViolations...) + return finish(1) + } else { + fmt.Fprintf(os.Stderr, "Syntax errors found:\n") + for _, err := range errs { + fmt.Fprintf(os.Stderr, " - %v\n", err) + } + // Hint: if script contains IMPORT/QUERY with single $ but not $$, suggest dollar-quoting + src := source + if (strings.Contains(src, "IMPORT") || strings.Contains(src, "import")) && + (strings.Contains(src, "QUERY") || strings.Contains(src, "query")) && + strings.Contains(src, "$") && !strings.Contains(src, "$$") { + fmt.Fprintf(os.Stderr, "\nHint: SQL queries in IMPORT statements should use dollar-quoting ($$...$$) instead of single quotes.\n") + fmt.Fprintf(os.Stderr, " Example: IMPORT FROM alias QUERY $$SELECT * FROM table$$ INTO Module.Entity MAP (...)\n") } - os.Exit(1) - } - // Zero statements from non-empty input is not an empty script: the parser - // never got into the file. Both gates refuse it (ako/mxcli#618). - // - // `source`, not `content`: for a test file they differ, and the message - // names a line from whichever text the parser was actually given. - if line, bad := unparsableInput(source, len(prog.Statements)); bad { - fmt.Fprintln(os.Stderr, unparsableInputError(filePath, line)) - os.Exit(1) } - if !isStructured { - fmt.Printf("✓ Syntax OK (%d statements)\n", len(prog.Statements)) + return 1 + } + // Zero statements from non-empty input is not an empty script: the parser + // never got into the file. Both gates refuse it (ako/mxcli#618). + // + // `source`, not `content`: for a test file they differ, and the message + // names a line from whichever text the parser was actually given. + if line, bad := unparsableInput(source, len(prog.Statements)); bad { + fmt.Fprintln(os.Stderr, unparsableInputError(filePath, line)) + return 1 + } + if !isStructured { + fmt.Printf("✓ Syntax OK (%d statements)\n", len(prog.Statements)) + } + + // Every semantic check lives in executor.ValidateProgram, so `mxcli exec` + // refuses exactly what `mxcli check` reports. Adding a check there gives + // both commands it at once. + violations := append(testProblems, executor.ValidateProgram(prog, projectPath)...) + violations = executor.ApplyDeprecationPolicy(violations, depPolicy) + + if isStructured { + // Always emit structured output (even when clean) + structured = append(structured, violations...) + } else if len(violations) > 0 { + fmt.Fprintln(os.Stderr) + formatter.Format(violations, os.Stderr) + } + + if len(violations) > 0 { + summary := linter.Summarize(violations) + if summary.Errors > 0 { + return finish(1) } + } - // Every semantic check lives in executor.ValidateProgram, so `mxcli exec` - // refuses exactly what `mxcli check` reports. Adding a check there gives - // both commands it at once. - violations := append(testProblems, executor.ValidateProgram(prog, projectPath)...) - violations = executor.ApplyDeprecationPolicy(violations, depPolicy) - - if isStructured { - // Always emit structured output (even when clean) - structured = append(structured, violations...) - } else if len(violations) > 0 { - fmt.Fprintln(os.Stderr) - formatter.Format(violations, os.Stderr) + // If reference checking requested + if checkRefs { + if projectPath == "" { + fmt.Fprintln(os.Stderr, "Error: --project (-p) is required for reference checking") + return 1 } - if len(violations) > 0 { - summary := linter.Summarize(violations) - if summary.Errors > 0 { - finish(1) - } + if !isStructured { + fmt.Printf("\nValidating references against: %s\n", projectPath) + fmt.Printf("(Note: References to objects created within the script are skipped)\n") } - - // If reference checking requested - if checkRefs { - if projectPath == "" { - fmt.Fprintln(os.Stderr, "Error: --project (-p) is required for reference checking") - os.Exit(1) + exec, logger := newLoggedExecutorTo("check", progressSink(format)) + defer logger.Close() + defer exec.Close() + + // Connect to project + connectProg, _ := visitor.Build(fmt.Sprintf("CONNECT LOCAL '%s'", visitor.QuoteString(projectPath))) + for _, stmt := range connectProg.Statements { + if err := exec.Execute(stmt); err != nil { + fmt.Fprintf(os.Stderr, "Error connecting: %v\n", err) + return 1 } + } - if !isStructured { - fmt.Printf("\nValidating references against: %s\n", projectPath) - fmt.Printf("(Note: References to objects created within the script are skipped)\n") - } - exec, logger := newLoggedExecutorTo("check", progressSink(format)) - defer logger.Close() - defer exec.Close() - - // Connect to project - connectProg, _ := visitor.Build(fmt.Sprintf("CONNECT LOCAL '%s'", visitor.QuoteString(projectPath))) - for _, stmt := range connectProg.Statements { - if err := exec.Execute(stmt); err != nil { - fmt.Fprintf(os.Stderr, "Error connecting: %v\n", err) - os.Exit(1) - } - } - - // Validate the program (considers objects defined within the script) - validationErrors, refWarnings := exec.ValidateProgramWithWarnings(prog) - - // Check for project conflicts: plain CREATE where the document already exists - validationErrors = append(validationErrors, exec.CheckProjectConflicts(prog)...) - - // Unresolved references in EXCLUDED documents: reported, never - // failing the run — Mendix does not validate excluded documents. - // In structured mode they join the error list (one document, not two) - // or are emitted on their own when there is nothing else. - var warnViolations []linter.Violation + // Validate the program (considers objects defined within the script) + validationErrors, refWarnings := exec.ValidateProgramWithWarnings(prog) + + // Check for project conflicts: plain CREATE where the document already exists + validationErrors = append(validationErrors, exec.CheckProjectConflicts(prog)...) + + // Unresolved references in EXCLUDED documents: reported, never + // failing the run — Mendix does not validate excluded documents. + // In structured mode they join the error list (one document, not two) + // or are emitted on their own when there is nothing else. + var warnViolations []linter.Violation + for _, w := range refWarnings { + warnViolations = append(warnViolations, linter.Violation{ + RuleID: "MDL-REF", + Severity: linter.SeverityWarning, + Message: w, + }) + } + if len(refWarnings) > 0 && !isStructured { + fmt.Fprintf(os.Stderr, "Reference warnings:\n") for _, w := range refWarnings { - warnViolations = append(warnViolations, linter.Violation{ - RuleID: "MDL-REF", - Severity: linter.SeverityWarning, - Message: w, - }) - } - if len(refWarnings) > 0 && !isStructured { - fmt.Fprintf(os.Stderr, "Reference warnings:\n") - for _, w := range refWarnings { - fmt.Fprintf(os.Stderr, " %s\n", w) - } - } else if len(warnViolations) > 0 && len(validationErrors) == 0 { - structured = append(structured, warnViolations...) - } - - if len(validationErrors) > 0 { - if isStructured { - refViolations := warnViolations - for _, err := range validationErrors { - refViolations = append(refViolations, linter.Violation{ - RuleID: "MDL-REF", - Severity: linter.SeverityError, - Message: err.Error(), - }) - } - structured = append(structured, refViolations...) - } else { - fmt.Fprintf(os.Stderr, "Reference errors:\n") - for _, err := range validationErrors { - fmt.Fprintf(os.Stderr, " %v\n", err) - } - fmt.Fprintf(os.Stderr, "\n✗ %d reference error(s) found\n", len(validationErrors)) - } - finish(1) - } - if !isStructured { - fmt.Printf("✓ All references valid\n") + fmt.Fprintf(os.Stderr, " %s\n", w) } + } else if len(warnViolations) > 0 && len(validationErrors) == 0 { + structured = append(structured, warnViolations...) + } - // The catalog-backed tier: the checks whose answers only exist once a - // project is connected. It runs after the reference check because a - // script naming things that do not exist has a more basic problem - // than a mistyped operand — and because building the catalog for a - // run that already failed is wasted work. - // - // Like every other violation this command emits, only an error - // severity fails the run. Warnings and hints are advice, and a - // checker whose first outing turns advice into a broken build is a - // checker people turn off. - // - // MDL087 is what this script REMOVES from the project, which nothing - // reported until now (ako/mxcli#562). `create or modify entity` - // rebuilds the entity from the statement, so a member the script does - // not restate is deleted — and the loss only becomes visible slices - // later, as a CE1613 on whatever still binds it. exec prints the same - // list, but only as it applies the statement; by then it is gone. - // - // Expression type checking is the other half: the rules that need an - // attribute's type, an enumeration's cases or a microflow's return - // type. The scope-local tier already ran in the unconditional pass. - // - // The flow verdicts are what exec would refuse (ako/mxcli#876): a - // `create or modify` of a stored flow whose change the splice cannot - // make is refused under mdl 1 and rebuilt with MDL-V1-REBUILD under - // mdl 0, and an alter whose patch fails is refused under both. They - // run the verdict exec and diff run, so the three agree. - projectViolations := exec.CheckEntityMemberDrops(prog) - projectViolations = append(projectViolations, exec.TypeCheckProgram(prog)...) - projectViolations = append(projectViolations, exec.CheckFlowVerdicts(prog)...) - if len(projectViolations) > 0 { - if isStructured { - structured = append(structured, projectViolations...) - } else { - fmt.Fprintln(os.Stderr) - formatter.Format(projectViolations, os.Stderr) + if len(validationErrors) > 0 { + if isStructured { + refViolations := warnViolations + for _, err := range validationErrors { + refViolations = append(refViolations, linter.Violation{ + RuleID: "MDL-REF", + Severity: linter.SeverityError, + Message: err.Error(), + }) } - if linter.Summarize(projectViolations).Errors > 0 { - finish(1) + structured = append(structured, refViolations...) + } else { + fmt.Fprintf(os.Stderr, "Reference errors:\n") + for _, err := range validationErrors { + fmt.Fprintf(os.Stderr, " %v\n", err) } - } else if !isStructured { - fmt.Printf("✓ Expression types OK, no unstated member drops, no flow change exec would refuse\n") + fmt.Fprintf(os.Stderr, "\n✗ %d reference error(s) found\n", len(validationErrors)) } + return finish(1) + } + if !isStructured { + fmt.Printf("✓ All references valid\n") } - // Post-migration scan: walk the project for native widgets that - // have pluggable replacements (Studio Pro does not auto-migrate - // these on a Mendix major-version upgrade). - if postMigration { - if projectPath == "" { - fmt.Fprintln(os.Stderr, "Error: --project (-p) is required for --post-migration") - os.Exit(1) - } - if !isStructured { - fmt.Printf("\nScanning project for legacy native widgets: %s\n", projectPath) - } - legacyViolations, err := scanLegacyWidgets(projectPath) - if err != nil { - fmt.Fprintf(os.Stderr, "Error scanning project: %v\n", err) - os.Exit(1) - } + // The catalog-backed tier: the checks whose answers only exist once a + // project is connected. It runs after the reference check because a + // script naming things that do not exist has a more basic problem + // than a mistyped operand — and because building the catalog for a + // run that already failed is wasted work. + // + // Like every other violation this command emits, only an error + // severity fails the run. Warnings and hints are advice, and a + // checker whose first outing turns advice into a broken build is a + // checker people turn off. + // + // MDL087 is what this script REMOVES from the project, which nothing + // reported until now (ako/mxcli#562). `create or modify entity` + // rebuilds the entity from the statement, so a member the script does + // not restate is deleted — and the loss only becomes visible slices + // later, as a CE1613 on whatever still binds it. exec prints the same + // list, but only as it applies the statement; by then it is gone. + // + // Expression type checking is the other half: the rules that need an + // attribute's type, an enumeration's cases or a microflow's return + // type. The scope-local tier already ran in the unconditional pass. + // + // The flow verdicts are what exec would refuse (ako/mxcli#876): a + // `create or modify` of a stored flow whose change the splice cannot + // make is refused under mdl 1 and rebuilt with MDL-V1-REBUILD under + // mdl 0, and an alter whose patch fails is refused under both. They + // run the verdict exec and diff run, so the three agree. + projectViolations := exec.CheckEntityMemberDrops(prog) + projectViolations = append(projectViolations, exec.TypeCheckProgram(prog)...) + projectViolations = append(projectViolations, exec.CheckFlowVerdicts(prog)...) + if len(projectViolations) > 0 { if isStructured { - structured = append(structured, legacyViolations...) - } else if len(legacyViolations) > 0 { - fmt.Fprintln(os.Stderr) - formatter.Format(legacyViolations, os.Stderr) - fmt.Fprintf(os.Stderr, "\n✗ %d legacy widget(s) found\n", len(legacyViolations)) + structured = append(structured, projectViolations...) } else { - fmt.Printf("✓ No legacy native widgets found\n") + fmt.Fprintln(os.Stderr) + formatter.Format(projectViolations, os.Stderr) } - if len(legacyViolations) > 0 { - summary := linter.Summarize(legacyViolations) - if summary.Errors > 0 { - finish(1) - } + if linter.Summarize(projectViolations).Errors > 0 { + return finish(1) } + } else if !isStructured { + fmt.Printf("✓ Expression types OK, no unstated member drops, no flow change exec would refuse\n") + } + } + + // Post-migration scan: walk the project for native widgets that + // have pluggable replacements (Studio Pro does not auto-migrate + // these on a Mendix major-version upgrade). + if postMigration { + if projectPath == "" { + fmt.Fprintln(os.Stderr, "Error: --project (-p) is required for --post-migration") + return 1 } - - finish(0) if !isStructured { - fmt.Println("\nCheck passed!") - // Qualify the verdict when nothing was resolved against a model. A - // bare "Check passed!" reads as more than it is: without a project - // no icon, entity, page or microflow name in the script has been - // looked up, and those are exactly what this command gets reached - // for. Saying so beats leaving the reader to infer it. - if !checkRefs { - fmt.Println(" (no project given — icon, entity, page and microflow references were") - fmt.Println(" not resolved; re-run with -p for full coverage)") + fmt.Printf("\nScanning project for legacy native widgets: %s\n", projectPath) + } + legacyViolations, err := scanLegacyWidgets(projectPath) + if err != nil { + fmt.Fprintf(os.Stderr, "Error scanning project: %v\n", err) + return 1 + } + if isStructured { + structured = append(structured, legacyViolations...) + } else if len(legacyViolations) > 0 { + fmt.Fprintln(os.Stderr) + formatter.Format(legacyViolations, os.Stderr) + fmt.Fprintf(os.Stderr, "\n✗ %d legacy widget(s) found\n", len(legacyViolations)) + } else { + fmt.Printf("✓ No legacy native widgets found\n") + } + if len(legacyViolations) > 0 { + summary := linter.Summarize(legacyViolations) + if summary.Errors > 0 { + return finish(1) } } - }, + } + + finish(0) + if !isStructured { + fmt.Println("\nCheck passed!") + // Qualify the verdict when nothing was resolved against a model. A + // bare "Check passed!" reads as more than it is: without a project + // no icon, entity, page or microflow name in the script has been + // looked up, and those are exactly what this command gets reached + // for. Saying so beats leaving the reader to infer it. + if !checkRefs { + fmt.Println(" (no project given — icon, entity, page and microflow references were") + fmt.Println(" not resolved; re-run with -p for full coverage)") + } + } + return 0 } + From 3d8d53548a335f6e891e9f41a377a338dec7c5f5 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 1 Oct 2026 17:25:43 +0000 Subject: [PATCH 2/5] feat(check,fmt): read several files as one script set; report and pair stub-then-real flows (#905) check and fmt --upgrade take several files, run in the order given. A flow two create-or-modify statements of the set declare - a stub, then the real flow - is reported as MDL-STUB01 (a warning) naming both statements, and fmt --upgrade decides the language header for the files sharing such a flow together: all take it or none does. Decided file by file, the stub's file was declined the header while the real flow's file took it, and run in order the mdl 0 stub rebuilt the real flow every run while the mdl 1 real statement was refused, with mx check clean. One file alone is unchanged. Co-Authored-By: Claude Opus 5.5 --- .../skills/fix-issue/findings/cmd-mxcli.jsonl | 1 + CHANGELOG.md | 1 + cmd/mxcli/cmd_check.go | 73 +++- cmd/mxcli/cmd_fmt.go | 314 +++++++++++++----- cmd/mxcli/script_set.go | 236 +++++++++++++ cmd/mxcli/script_set_test.go | 215 ++++++++++++ docs-site/src/appendixes/error-messages.md | 15 + 7 files changed, 761 insertions(+), 94 deletions(-) create mode 100644 cmd/mxcli/script_set.go create mode 100644 cmd/mxcli/script_set_test.go diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl index b5ea9ead1d..1a059d139c 100644 --- a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl +++ b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl @@ -133,3 +133,4 @@ {"area": "cmd/mxcli", "date": "2026-09-28", "symptom": "`mxcli check x.test.mdl` reported an MDL-DEPR001 warning (`create or replace` is deprecated) on the doc comment of every @test block, for a spelling the author never wrote; the conformance gate (#756) counted 70-odd of them across mdl-examples' test files", "cause": "testrunner.CheckSource renders each test block as a microflow so the top-level grammar can check it, and its wrapper text was `CREATE OR REPLACE MICROFLOW …`, placed on the doc-comment line. The deprecation registry landed after the wrapper, and nothing checked mxcli's own generated MDL against it", "file": "`cmd/mxcli/testrunner/check_source.go` (wrapper spelling), test `TestCheckSourceWrapperIsCanonical` in `check_source_test.go`", "insight": "**Generated MDL is subject to the deprecation registry too**, and a warning on generated text lands on the author's line numbers, where it looks like their mistake. The other generators in testrunner (generator.go, endpoint.go) still write `create or replace` into scripts that are executed, not checked, so they warn nowhere a user sees — left alone. Proof: the new test fails with MDL-DEPR001 on line 1 of the rendering before the one-word fix.", "refs": ["ako/mxcli#756"]} {"date": "2026-09-30", "area": "cmd/mxcli", "symptom": "`mxcli fmt --upgrade tests/csv-import.test.mdl` failed with `no viable alternative at input '/**\\n * @test …'` on a file `mxcli check` passes, so the migration tool could not reach a project's test suite; separately `mxcli check x.test.md` reported a syntax error on every valid markdown test block whose doc comment spans lines.", "cause": "fmt parsed the file as top-level MDL, but a test file is `/** @test */` doc comments and `/` separators around microflow bodies; only check rendered it (testrunner.CheckSource). The markdown parser counted a fence block's lines from the ```mdl-test line instead of the line after it, so BodyLine was one early and the rendering overwrote the doc comment's last line with the body.", "fix": "testrunner.UpgradeSource upgrades CheckSource's line-preserving rendering and maps it back line for line: a line the rendering kept verbatim takes the upgraded text, every other line stays the author's, and a rewrite reaching a wrapper line or changing the line count is refused. fmt routes test files there (plain fmt on one is refused with a pointer to --upgrade). parseMarkdownTests passes blockStart+1 as the chunk line.", "insight": "A test file takes no language header: neither check's rendering nor either runner generator reads a `mdl 1;` line, and one in front of the first @test makes that test's doc comment non-leading, so the runner silently drops it. --header therefore adds no header to a test file and says so, rather than applying mdl 1 rewrites the runner would execute as mdl 0. The markdown BodyLine bug was found only because the upgrade's corpus test re-parsed the rendering of mdl-examples/doctype-tests/microflow-spec.test.md; it also shrank the conformance allowlist.", "issue": "ako/mxcli#837", "file": "cmd/mxcli/testrunner/upgrade_source.go, cmd/mxcli/testrunner/parser.go (parseMarkdownTests), cmd/mxcli/cmd_fmt.go", "test": "cmd/mxcli/testrunner/upgrade_source_test.go, TestCheckSourceMarkdownBodyLine, cmd/mxcli/cmd_fmt_upgrade_test.go TestFmtUpgrade_TestFile"} {"date": "2026-10-01", "area": "cmd/mxcli", "symptom": "The LSP's CREATE MICROFLOW / CREATE NANOFLOW / CREATE ENUMERATION snippet completions inserted MDL that does not parse under any language version (`missing '(' at 'BEGIN'`, a quoted enumeration value name), the CONSTANT snippet the deprecated clause form (MDL-DEPR136), and PAGE/SNIPPET a statement mdl 1 refuses for its missing `;`; completion also offered alias-only keywords (SHOW_PAGE, DELETE_BEHAVIOR, DEFINE) and the `show entities` listings after SHOW.", "cause": "The snippets and keyword lists were hand-written once and never parsed; the grammar moved on (R2 property lists, R8 words, `list` for `show`) and nothing tied the completion text to it. The keyword list is generated from the lexer alone, which cannot tell an alias token from a canonical one: that is said by the `/* @alias MDL-DEPRnnn */` markers in the parser grammar.", "fix": "Snippets rewritten to canonical mdl 1. cmd/gen-completions reads the parser grammar too and drops a token every parser-rule use of which carries an @alias marker (the `keyword` rule, which lists tokens usable as names, does not count). `list` gets the listings; `show` offers page/message/home page.", "insight": "A completion text is MDL that ships in the binary, so it is held to what docs are held to: TestCompletionSnippetsAreMdl1 parses every snippet, expanded with its defaults, under `mdl 1;` and requires no deprecation. Deciding alias-only from the markers is the registry's own data; a token-swap rewrite word (`snippet`, `column`, `comment`) is NOT alias-only \u2014 those words stay canonical elsewhere \u2014 and neither is SHOW (show page, show message).", "issue": "ako/mxcli#714 (decision 5)", "file": "cmd/mxcli/lsp_completion.go, cmd/gen-completions/aliases.go", "test": "cmd/mxcli/lsp_mdl1_test.go TestCompletionSnippetsAreMdl1, TestCompletionOffersNoDeprecatedSpelling, TestCompletionListAndShowContinueIntoMdl1; cmd/gen-completions/aliases_test.go"} +{"date": "2026-10-01", "area": "cmd/mxcli", "symptom": "A stub-then-real script set (two files each with `create or modify microflow X`) upgraded with `fmt --upgrade -p` one file at a time ended up split: the stub's file declined the header (MDL-V1-REBUILD) and the real flow's file took it. Run in order, the mdl 0 stub rebuilt the stored real flow and the mdl 1 real statement was refused, every run; mx check 0 errors, the app running the placeholder.", "cause": "Each command judged one file against the model as stored; check/fmt took exactly one file, so nothing could see a flow declared twice across the set, and a per-file header decision is wrong for a pair whose files must agree.", "fix": "check and fmt accept several files as one script set (cmd/mxcli/script_set.go): findFlowRedeclarations finds a flow declared by two create-or-modify statements; check warns MDL-STUB01 naming both; fmt --upgrade groups files sharing such a flow and declines the header for all of them when any one cannot take it (canTakeHeader = the per-file verdict). One file alone is unchanged.", "insight": "A per-file verdict is only right when the files are independent; the run is the unit when two files write the same document. The symptom hides because each half is individually correct (decline is right for the stub, header is right for the real file) - only the combination is wrong, so test the set, with the single-file decision as the control.", "issue": "ako/mxcli#905", "file": "cmd/mxcli/script_set.go", "test": "cmd/mxcli/script_set_test.go TestFmtUpgrade_ScriptSetDecidesStubThenRealHeaderTogether, TestCheck_ScriptSetWarnsOnStubThenReal"} diff --git a/CHANGELOG.md b/CHANGELOG.md index 8bfd4d0433..8a9513a234 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Changed +- **`check` and `fmt --upgrade` take several files and read them as one script set** (ako/mxcli#905) — a flow declared by two `create or modify` statements of the set, a stub followed by the real flow, is reported as **MDL-STUB01** (a warning), and `fmt --upgrade [-p]` decides the language header for the files sharing such a flow together: added to all or to none. Upgraded one file at a time, the stub's file could be declined the header (exec would refuse its change to the stored real flow) while the real flow's file took it; run in order, the mdl 0 stub then rebuilt the real flow and the mdl 1 real statement was refused, on every run, with `mx check` clean. One file alone behaves as before. **Migrating a script set:** run `fmt --upgrade -w -p app.mpr` over all of its files at once, or drop the stub — a self-recursive flow needs none since #843. - **`mdl 1` is frozen** (ako/mxcli#714 freeze, ako/mxcli#840) — the beta language is a contract: the `MDL-LANG01` "preview: may still change" warning is gone, and a later change of meaning needs `mdl 2`. **`describe` writes `mdl 1`**: every description, from the statement and from `mxcli describe`, starts with `mdl 1;` and runs back as it is; descriptions concatenated into one file repeat the header, which the parser now accepts when it names the same version (a header naming another version is refused). `mxcli describe`, `mxcli context` and `mxcli diff-local` take `--mdl 0|1` (default 1); `--mdl 0` writes the alpha language with no header. Describe never writes a deprecated spelling in either version: list operations and aggregates are their statements (`$n = count $L;`, not `count($L)`), and a chart series' template parameters are in `( )`. **`fmt --upgrade` adds the header by default** (`--header=false` declines it); plain `fmt` never adds one. **The REPL and `-c` start in `mdl 1`**: input without a header is read as `mdl 1`, `--mdl 0` starts them in `mdl 0`, and in the REPL `mdl 0;` / `mdl 1;` switches the session; session commands and a missing `;` after the last statement stay allowed there. **Script files are unchanged**: `exec`, `check` and `fmt` read a headerless file as `mdl 0` (ADR-0011). **Migrating:** nothing breaks for a script file. A one-liner or REPL habit that relied on `mdl 0` (`show entity X`, a backslash escape, `limit 1` as an object) now needs `--mdl 0`, or the `mdl 1` spelling. `mdl 0` is documented on the "Language versions and migration" page only. - **`check` warns when a doc comment is lost** (ako/mxcli#877) — a `/** … */` doc comment before a statement that cannot store documentation (`drop`, `grant`, `revoke`, `set`, `alter`, `create module`, …) is ignored. `check` and `exec` now warn **MDL089**, naming the next statement that could have taken it. mxcli-rest wrote `drop microflow if exists X;` between six flows' comments and their `create`s, and the documentation was gone with every other tool clean. - **`check -p` reports what `exec` would refuse in a flow, and `fmt --upgrade --header -p` keeps the header off a script it would stop** (ako/mxcli#876) — a `create or modify microflow|nanoflow` of a stored flow whose change cannot be spliced in is an `MDL-V1-REBUILD` error with exec's message under `mdl 1`, and exec's `MDL-V1-REBUILD` warning without the header; an `alter microflow|nanoflow` exec would refuse is `MDL090`. `check`, `diff` and `exec` share one verdict function, so they cannot disagree. A statement on a flow an earlier statement of the script changes, or that only builds once an earlier statement has run, is not predicted. `fmt --upgrade --header -p` names each statement that would be refused and leaves the header off that file (`--force-header` adds it anyway); mxcli-rest's corpus checked clean after the upgrade and then stopped executing. diff --git a/cmd/mxcli/cmd_check.go b/cmd/mxcli/cmd_check.go index 3743e7a4d1..7f892f8fe4 100644 --- a/cmd/mxcli/cmd_check.go +++ b/cmd/mxcli/cmd_check.go @@ -15,7 +15,7 @@ import ( ) var checkCmd = &cobra.Command{ - Use: "check ", + Use: "check [file...]", Short: "Check an MDL script for errors without executing it", Long: `Check an MDL script file for syntax errors and optionally validate references. @@ -73,6 +73,16 @@ This is the verdict "mxcli diff" reports as "Refused:", computed by the same code. A statement on a flow an earlier statement of the script changes, or whose flow does not build until an earlier statement has run, is not predicted. +Several files are checked as one script set, run in the order given: each +file is checked as it would be alone, and the set is first read as a whole for +two "create or modify" statements declaring the same flow — a placeholder +("stub") followed by the real flow. That is reported as an MDL-STUB01 warning +naming both statements: run in order, the stub replaces the real flow before +the real statement restores it, on every run, and with the two under different +language headers the real statement can be refused while the stub's rebuild +goes through. Since #843 a self-recursive flow is created in one statement, so +the stub can be dropped. One file alone is not read this way. + Output includes structured rule IDs (MDL prefix for reference and script rules, E0xx for expression type rules) for each validation issue. @@ -100,12 +110,15 @@ Examples: mxcli check script.mdl --format json mxcli check script.mdl -p app.mpr --format sarif > results.sarif + # Check a script set, run in this order + mxcli check 10-domain.mdl 20-flows.mdl 30-pages.mdl -p app.mpr + # Read the script from stdin cat script.mdl | mxcli check - `, - Args: cobra.ExactArgs(1), + Args: cobra.MinimumNArgs(1), Run: func(cmd *cobra.Command, args []string) { - if code := runCheckFile(cmd, args[0]); code != 0 { + if code := runCheckFiles(cmd, args); code != 0 { os.Exit(code) } }, @@ -419,3 +432,57 @@ func runCheckFile(cmd *cobra.Command, filePath string) int { return 0 } +// runCheckFiles checks each script of a run and returns the worst exit code. +// One file is checked exactly as before. Several files are a script set run +// in order (ako/mxcli#905): before the files are checked one by one, the set +// is read as a whole for what no single file shows — a flow two of its +// `create or modify` statements declare, the stub-then-real pattern +// (MDL-STUB01, a warning). +func runCheckFiles(cmd *cobra.Command, files []string) int { + if len(files) == 1 { + return runCheckFile(cmd, files[0]) + } + for _, f := range files { + if f == stdinPath { + fmt.Fprintln(os.Stderr, "Error: '-' (stdin) checks one script; it cannot be one of several files") + return 1 + } + } + if format := resolveFormat(cmd, "text"); format != "" && format != "text" { + fmt.Fprintf(os.Stderr, "Error: --format %s writes one document for one script; check the files one at a time\n", format) + return 1 + } + if v := stubThenRealViolations(findFlowRedeclarations(parseScriptSet(files))); len(v) > 0 { + fmt.Printf("Checking the script set: %d files\n", len(files)) + linter.GetFormatter(linter.OutputFormat("text"), true).Format(v, os.Stderr) + fmt.Fprintln(os.Stderr) + } + worst := 0 + for i, f := range files { + if i > 0 { + fmt.Println() + } + if code := runCheckFile(cmd, f); code > worst { + worst = code + } + } + return worst +} + +// parseScriptSet reads and parses each file for the set-level checks. A file +// that cannot be read or parsed is left out (Prog nil): its own check reports +// that. +func parseScriptSet(files []string) []setScript { + var out []setScript + for _, f := range files { + sc := setScript{Path: f} + if data, err := os.ReadFile(f); err == nil && !testrunner.IsTestFile(f) { + sc.Source = string(data) + if prog, errs := visitor.Build(sc.Source); len(errs) == 0 { + sc.Prog = prog + } + } + out = append(out, sc) + } + return out +} diff --git a/cmd/mxcli/cmd_fmt.go b/cmd/mxcli/cmd_fmt.go index e406877ec3..d5679c473b 100644 --- a/cmd/mxcli/cmd_fmt.go +++ b/cmd/mxcli/cmd_fmt.go @@ -23,7 +23,7 @@ import ( ) var fmtCmd = &cobra.Command{ - Use: "fmt [file.mdl | -]", + Use: "fmt [file.mdl... | -]", Short: "Format an MDL file", Long: `Format an MDL script file with consistent styling: - Lowercase MDL keywords, the canonical case. Only words the parse tree shows @@ -86,6 +86,17 @@ Upgrading (--upgrade): stored. Without -p the script is left as written and fmt prints a note (MDL067) for each flow with a bare commit. + Several files are upgraded as one script set, run in the order given. When + two of their "create or modify" statements declare the same flow — a + placeholder ("stub") followed by the real flow — fmt reports the pair + (MDL-STUB01) and decides the header for the files that share the flow + together: all of them take it, or none does. Decided file by file, the stub + could stay mdl 0 (exec would refuse its change to the stored real flow) + while the real flow's file took the header; run in order, the stub then + rebuilt the real flow on every run and the mdl 1 real statement was + refused. Since #843 a self-recursive flow is created in one statement, so + the better fix is to drop the stub. + A test file (.test.mdl, .test.md) is upgraded the way check reads it: the statements in its blocks are rewritten, and its doc comments (@test, @expect, …), separators and prose are kept byte for byte. It takes no @@ -95,10 +106,10 @@ Upgrading (--upgrade): mxcli fmt --upgrade -w script.mdl mxcli fmt --upgrade --header -w script.mdl mxcli fmt --upgrade --header -w -p app.mpr script.mdl + mxcli fmt --upgrade -w -p app.mpr scripts/*.mdl `, - Args: cobra.MaximumNArgs(1), + Args: cobra.ArbitraryArgs, RunE: func(cmd *cobra.Command, args []string) error { - writeInPlace, _ := cmd.Flags().GetBool("write") doUpgrade, _ := cmd.Flags().GetBool("upgrade") addHeader, _ := cmd.Flags().GetBool("header") if cmd.Flags().Changed("header") && !doUpgrade { @@ -107,111 +118,131 @@ Upgrading (--upgrade): if force, _ := cmd.Flags().GetBool("force-header"); force && (!doUpgrade || cmd.Flags().Changed("header") && !addHeader) { return fmt.Errorf("--force-header needs --upgrade, with the header") } - - // Determine source: stdin when no arg or "-" is passed. - fromStdin := len(args) == 0 || args[0] == "-" - filePath := "" - if !fromStdin { - filePath = args[0] + if len(args) > 1 { + return fmtScriptSet(cmd, args) } + return fmtFile(cmd, args, "") + }, +} - if writeInPlace && fromStdin { - return fmt.Errorf("-w cannot be used with stdin") - } +// fmtFile formats or upgrades one script: args holds its path, or nothing or +// "-" for stdin. declineHeader, when set, keeps the language header off the +// file and says why: the header of a script set's stub-then-real pair is +// decided for its files together (ako/mxcli#905). +func fmtFile(cmd *cobra.Command, args []string, declineHeader string) error { + writeInPlace, _ := cmd.Flags().GetBool("write") + doUpgrade, _ := cmd.Flags().GetBool("upgrade") + addHeader, _ := cmd.Flags().GetBool("header") + + // Determine source: stdin when no arg or "-" is passed. + fromStdin := len(args) == 0 || args[0] == "-" + filePath := "" + if !fromStdin { + filePath = args[0] + } + + if writeInPlace && fromStdin { + return fmt.Errorf("-w cannot be used with stdin") + } + + var data []byte + var err error + if fromStdin { + data, err = io.ReadAll(os.Stdin) + } else { + data, err = os.ReadFile(filePath) + } + if err != nil { + return fmt.Errorf("failed to read input: %w", err) + } + + label := filePath + if fromStdin { + label = "" + } - var data []byte - var err error - if fromStdin { - data, err = io.ReadAll(os.Stdin) - } else { - data, err = os.ReadFile(filePath) + // A .test.mdl / .test.md file is not top-level MDL: its blocks are + // microflow bodies behind `/** @test … */` doc comments. --upgrade reads + // it the way check does (ako/mxcli#837); the layout formatter does not + // know the format, so it is not let loose on one. + if !fromStdin && testrunner.IsTestFile(filePath) { + if !doUpgrade { + return fmt.Errorf("%s is a test file: fmt formats top-level MDL scripts, and would not keep a test "+ + "file's doc comments and separators; use `mxcli fmt --upgrade` to upgrade its statements", label) + } + opts := upgrade.DefaultOptions() + if cmd.Flags().Changed("header") { + opts.AddHeader = addHeader } + res, headerSkipped, err := testrunner.UpgradeSource(string(data), filePath, opts) if err != nil { - return fmt.Errorf("failed to read input: %w", err) + return fmt.Errorf("%s: %w", label, err) } - - label := filePath - if fromStdin { - label = "" + reportUpgrade(cmd.ErrOrStderr(), label, res) + if headerSkipped { + fmt.Fprintf(cmd.ErrOrStderr(), "%s: no language header added: a test file takes no language header yet "+ + "(check and the test runner read its blocks as mdl 0), so its header-gated constructs were left as they are\n", label) } + return writeFmtResult(cmd, filePath, writeInPlace, string(data), res.Source, true) + } - // A .test.mdl / .test.md file is not top-level MDL: its blocks are - // microflow bodies behind `/** @test … */` doc comments. --upgrade reads - // it the way check does (ako/mxcli#837); the layout formatter does not - // know the format, so it is not let loose on one. - if !fromStdin && testrunner.IsTestFile(filePath) { - if !doUpgrade { - return fmt.Errorf("%s is a test file: fmt formats top-level MDL scripts, and would not keep a test "+ - "file's doc comments and separators; use `mxcli fmt --upgrade` to upgrade its statements", label) - } - opts := upgrade.DefaultOptions() - if cmd.Flags().Changed("header") { - opts.AddHeader = addHeader - } - res, headerSkipped, err := testrunner.UpgradeSource(string(data), filePath, opts) - if err != nil { - return fmt.Errorf("%s: %w", label, err) - } - reportUpgrade(cmd.ErrOrStderr(), label, res) - if headerSkipped { - fmt.Fprintf(cmd.ErrOrStderr(), "%s: no language header added: a test file takes no language header yet "+ - "(check and the test runner read its blocks as mdl 0), so its header-gated constructs were left as they are\n", label) - } - return writeFmtResult(cmd, filePath, writeInPlace, string(data), res.Source, true) + // Reject unparseable input so automation scripts can detect failures. + // Two failure modes: + // 1. ANTLR reports explicit parse errors (structural violations). + // 2. ANTLR silently skips unrecognised tokens — detected when no + // statements were produced from non-blank, non-comment content. + prog, errs := visitor.Build(string(data)) + if len(errs) > 0 { + var msgs []string + for _, e := range errs { + msgs = append(msgs, e.Error()) } + return fmt.Errorf("syntax errors in %s:\n%s", label, strings.Join(msgs, "\n")) + } + if prog != nil && len(prog.Statements) == 0 && hasSubstantiveContent(string(data)) { + return fmt.Errorf("no valid MDL statements found in %s", label) + } - // Reject unparseable input so automation scripts can detect failures. - // Two failure modes: - // 1. ANTLR reports explicit parse errors (structural violations). - // 2. ANTLR silently skips unrecognised tokens — detected when no - // statements were produced from non-blank, non-comment content. - prog, errs := visitor.Build(string(data)) - if len(errs) > 0 { - var msgs []string - for _, e := range errs { - msgs = append(msgs, e.Error()) - } - return fmt.Errorf("syntax errors in %s:\n%s", label, strings.Join(msgs, "\n")) + var formatted string + if doUpgrade { + opts := upgrade.DefaultOptions() + if cmd.Flags().Changed("header") { + opts.AddHeader = addHeader } - if prog != nil && len(prog.Statements) == 0 && hasSubstantiveContent(string(data)) { - return fmt.Errorf("no valid MDL statements found in %s", label) + if declineHeader != "" { + opts.AddHeader = false } - - var formatted string - if doUpgrade { - opts := upgrade.DefaultOptions() - if cmd.Flags().Changed("header") { - opts.AddHeader = addHeader + closeProject, err := openUpgradeProject(cmd, &opts) + if err != nil { + return err + } + defer closeProject() + res, err := upgrade.Upgrade(string(data), opts) + if err != nil { + // The header is the default since the freeze (ako/mxcli#714): + // name the way to upgrade the spellings without it, which a + // plain `fmt --upgrade` did before. + var blocked *upgrade.HeaderBlockedError + if errors.As(err, &blocked) && !cmd.Flags().Changed("header") { + return fmt.Errorf("%s: %w\n`mxcli fmt --upgrade --header=false` upgrades the spellings without the header", label, err) } - closeProject, err := openUpgradeProject(cmd, &opts) - if err != nil { + return fmt.Errorf("%s: %w", label, err) + } + if res.HeaderAdded { + if res, err = declineRefusedHeader(cmd, label, string(data), opts, res); err != nil { return err } - defer closeProject() - res, err := upgrade.Upgrade(string(data), opts) - if err != nil { - // The header is the default since the freeze (ako/mxcli#714): - // name the way to upgrade the spellings without it, which a - // plain `fmt --upgrade` did before. - var blocked *upgrade.HeaderBlockedError - if errors.As(err, &blocked) && !cmd.Flags().Changed("header") { - return fmt.Errorf("%s: %w\n`mxcli fmt --upgrade --header=false` upgrades the spellings without the header", label, err) - } - return fmt.Errorf("%s: %w", label, err) - } - if res.HeaderAdded { - if res, err = declineRefusedHeader(cmd, label, string(data), opts, res); err != nil { - return err - } - } - reportUpgrade(cmd.ErrOrStderr(), label, res) - formatted = res.Source - } else { - formatted = formatter.Format(string(data)) } + reportUpgrade(cmd.ErrOrStderr(), label, res) + if declineHeader != "" { + fmt.Fprintf(cmd.ErrOrStderr(), "%s: no language header added: %s\n", label, declineHeader) + } + formatted = res.Source + } else { + formatted = formatter.Format(string(data)) + } - return writeFmtResult(cmd, filePath, writeInPlace, string(data), formatted, doUpgrade) - }, + return writeFmtResult(cmd, filePath, writeInPlace, string(data), formatted, doUpgrade) } // openUpgradeProject opens the -p project read-only for the upgrade to read @@ -381,3 +412,104 @@ func hasSubstantiveContent(s string) bool { } return false } + +// fmtScriptSet formats or upgrades several scripts, each as fmtFile would one, +// and returns every file's error. With --upgrade the files are read as one set +// first (ako/mxcli#905): a flow two of its `create or modify` statements +// declare — the stub-then-real pattern — is reported (MDL-STUB01), and the +// language header of the files sharing such a flow is decided for them +// together: added to all of them or to none. Decided file by file, the stub's +// file was declined the header (exec would refuse its change to the stored +// real flow) while the real flow's file took it; run in order, the mdl 0 stub +// then rebuilt the real flow and the mdl 1 real statement was refused, on +// every run, with mx check clean. +func fmtScriptSet(cmd *cobra.Command, files []string) error { + for _, f := range files { + if f == stdinPath { + return fmt.Errorf("'-' (stdin) is one script; it cannot be one of several files") + } + } + decline := map[string]string{} + if doUpgrade, _ := cmd.Flags().GetBool("upgrade"); doUpgrade { + scripts := parseScriptSet(files) + redecls := findFlowRedeclarations(scripts) + w := cmd.ErrOrStderr() + for _, v := range stubThenRealViolations(redecls) { + fmt.Fprintf(w, "warning: %s [%s]\n", v.Message, v.RuleID) + } + if addHeader, _ := cmd.Flags().GetBool("header"); addHeader && len(redecls) > 0 { + decline = decideSetHeaders(cmd, scripts, redecls) + } + } + var errs []error + for _, f := range files { + if err := fmtFile(cmd, []string{f}, decline[f]); err != nil { + errs = append(errs, err) + } + } + return errors.Join(errs...) +} + +// decideSetHeaders returns, for each file that has to stay off the header +// because a file it shares a redeclared flow with cannot take it, the reason +// to print. A group whose files can all take the header, or none of them, is +// left to the per-file decision, which then agrees. +func decideSetHeaders(cmd *cobra.Command, scripts []setScript, redecls []flowRedeclaration) map[string]string { + src := map[string]string{} + var files []string + for _, sc := range scripts { + src[sc.Path] = sc.Source + files = append(files, sc.Path) + } + decline := map[string]string{} + for _, group := range fileGroups(files, redecls) { + var can, cannot []string + for _, f := range group { + if canTakeHeader(cmd, src[f]) { + can = append(can, f) + } else { + cannot = append(cannot, f) + } + } + if len(can) == 0 || len(cannot) == 0 { + continue + } + for _, f := range can { + decline[f] = fmt.Sprintf("it declares a flow that %s also declares, and that file cannot take the "+ + "header; a stub-then-real pair under different headers lets the mdl 0 statement rebuild the flow "+ + "on every run while the mdl 1 one is refused, so the header is decided for the files together "+ + "(%s)", strings.Join(cannot, ", "), StubThenRealRule) + } + } + return decline +} + +// canTakeHeader reports whether fmt --upgrade, run on src alone, would leave +// it under the language header: it has one already, or the upgrade can add +// one (no construct blocks it) and, with -p, exec would refuse none of its +// statements under it (or --force-header overrides that). +func canTakeHeader(cmd *cobra.Command, src string) bool { + if _, written := langver.ScanWrittenHeader(src); written { + return true + } + opts := upgrade.DefaultOptions() + opts.AddHeader = true + closeProject, err := openUpgradeProject(cmd, &opts) + if err != nil { + return false + } + defer closeProject() + res, err := upgrade.Upgrade(src, opts) + if err != nil { + return false + } + projectPath, _ := cmd.Flags().GetString("project") + if !res.HeaderAdded || projectPath == "" { + return true + } + if force, _ := cmd.Flags().GetBool("force-header"); force { + return true + } + refusals, err := headerRefusals(projectPath, res.Source) + return err == nil && len(refusals) == 0 +} diff --git a/cmd/mxcli/script_set.go b/cmd/mxcli/script_set.go new file mode 100644 index 0000000000..c9b0cafc8e --- /dev/null +++ b/cmd/mxcli/script_set.go @@ -0,0 +1,236 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "regexp" + "sort" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/langver" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// StubThenRealRule is the warning for a script set that declares one flow +// twice with `create or modify` (ako/mxcli#905): a placeholder ("stub") that +// earlier scripts can reference, then the real flow. Run in order, the stub +// rebuilds whatever is stored before the real statement restores it — on +// every run. When the two files end up under different language versions the +// mdl 0 stub still rebuilds the real flow while the mdl 1 real statement may +// be refused, and the project keeps running the placeholder with mx check +// clean. Since #843 a self-recursive flow is created in one statement, so the +// stub is no longer needed. +const StubThenRealRule = "MDL-STUB01" + +// setScript is one parsed script of a run over several files. +type setScript struct { + Path string + Source string + Prog *ast.Program +} + +// flowDeclSite is one `create or modify microflow|nanoflow` in a script set. +type flowDeclSite struct { + File string + Line int // 1-based; 0 when the statement could not be located in the source + Nanoflow bool + Version langver.Version +} + +// flowRedeclaration is a flow that a script set declares more than once with +// `create or modify`, in source order (files in the order given). +type flowRedeclaration struct { + Flow string + Sites []flowDeclSite +} + +// files returns the distinct files of the redeclaration, in order. +func (r flowRedeclaration) files() []string { + var out []string + seen := map[string]bool{} + for _, s := range r.Sites { + if !seen[s.File] { + seen[s.File] = true + out = append(out, s.File) + } + } + return out +} + +// mixedVersions reports whether the sites are written in different language +// versions — the pair #905 found, where the mdl 0 stub rebuilds the real flow +// and the mdl 1 real statement is refused. +func (r flowRedeclaration) mixedVersions() bool { + for _, s := range r.Sites[1:] { + if s.Version != r.Sites[0].Version { + return true + } + } + return false +} + +// findFlowRedeclarations returns every flow that two or more `create or +// modify microflow|nanoflow` statements of the set declare, whether in +// different files or twice in one. Microflows and nanoflows share one +// namespace, and Mendix document names are matched case-insensitively here, +// as the executor's lookups do. The result is ordered by first appearance. +func findFlowRedeclarations(scripts []setScript) []flowRedeclaration { + byKey := map[string]*flowRedeclaration{} + var order []string + for _, sc := range scripts { + if sc.Prog == nil { + continue + } + seenInFile := map[string]int{} + for _, stmt := range sc.Prog.Statements { + var name ast.QualifiedName + nano := false + switch s := stmt.(type) { + case *ast.CreateMicroflowStmt: + if !s.CreateOrModify { + continue + } + name = s.Name + case *ast.CreateNanoflowStmt: + if !s.CreateOrModify { + continue + } + name, nano = s.Name, true + default: + continue + } + key := strings.ToLower(name.String()) + r, ok := byKey[key] + if !ok { + r = &flowRedeclaration{Flow: name.String()} + byKey[key] = r + order = append(order, key) + } + nth := seenInFile[key] + seenInFile[key] = nth + 1 + r.Sites = append(r.Sites, flowDeclSite{ + File: sc.Path, + Line: flowDeclLine(sc.Source, name, nth), + Nanoflow: nano, + Version: sc.Prog.LanguageVersion, + }) + } + } + var out []flowRedeclaration + for _, k := range order { + if r := byKey[k]; len(r.Sites) > 1 { + out = append(out, *r) + } + } + return out +} + +// flowDeclLine finds the line of the nth (0-based) `create or modify|replace +// microflow|nanoflow ` in src. The AST carries no statement positions; +// a statement whose keywords are split over lines is not located (0). +func flowDeclLine(src string, name ast.QualifiedName, nth int) int { + ident := func(s string) string { return `(?:"` + regexp.QuoteMeta(s) + `"|` + regexp.QuoteMeta(s) + `)` } + re, err := regexp.Compile(`(?i)\bcreate\s+or\s+(?:modify|replace)\s+(?:microflow|nanoflow)\s+` + + ident(name.Module) + `\s*\.\s*` + ident(name.Name) + `(?:[^\w]|$)`) + if err != nil { + return 0 + } + for i, line := range strings.Split(src, "\n") { + if re.MatchString(line) { + if nth == 0 { + return i + 1 + } + nth-- + } + } + return 0 +} + +// site names a declaration for a message: file:line. +func (s flowDeclSite) String() string { + if s.Line == 0 { + return s.File + } + return fmt.Sprintf("%s:%d", s.File, s.Line) +} + +// stubThenRealViolations is check's warning for each redeclared flow of a +// script set: both statements named, and the advice to drop the stub. +func stubThenRealViolations(redecls []flowRedeclaration) []linter.Violation { + var out []linter.Violation + for _, r := range redecls { + var sites []string + for _, s := range r.Sites { + sites = append(sites, fmt.Sprintf("%s (%s)", s, s.Version)) + } + msg := fmt.Sprintf("%s is declared by %d `create or modify` statements in this script set: %s. "+ + "Run in order, the first replaces what the last one stored, on every run", + r.Flow, len(r.Sites), strings.Join(sites, ", ")) + if r.mixedVersions() { + msg += "; and under different language versions the mdl 0 one rebuilds the flow while the mdl 1 one " + + "can be refused, leaving the project running the placeholder with mx check clean — at the least give " + + "them the same header (`mxcli fmt --upgrade -w -p app.mpr` over all the files decides it for them together)" + } + msg += ". A self-recursive flow no longer needs a placeholder (#843): drop the stub and keep the real statement" + mod, doc := r.Flow, r.Flow + if i := strings.IndexByte(r.Flow, '.'); i > 0 { + mod, doc = r.Flow[:i], r.Flow[i+1:] + } + dt := "microflow" + if r.Sites[0].Nanoflow { + dt = "nanoflow" + } + out = append(out, linter.Violation{ + RuleID: StubThenRealRule, + Severity: linter.SeverityWarning, + Message: msg, + Location: linter.Location{Module: mod, DocumentType: dt, DocumentName: doc}, + }) + } + return out +} + +// fileGroups joins the files that share a redeclared flow into groups +// (transitively), each listed in the order the files were given. A group of +// one file — a flow declared twice in the same file — is not returned: one +// file has one header. +func fileGroups(files []string, redecls []flowRedeclaration) [][]string { + parent := map[string]string{} + var find func(string) string + find = func(f string) string { + if p, ok := parent[f]; ok && p != f { + r := find(p) + parent[f] = r + return r + } + parent[f] = f + return f + } + for _, r := range redecls { + fs := r.files() + for _, f := range fs[1:] { + parent[find(f)] = find(fs[0]) + } + } + idx := map[string]int{} + for i, f := range files { + idx[f] = i + } + byRoot := map[string][]string{} + for f := range parent { + root := find(f) + byRoot[root] = append(byRoot[root], f) + } + var out [][]string + for _, g := range byRoot { + if len(g) < 2 { + continue + } + sort.Slice(g, func(i, j int) bool { return idx[g[i]] < idx[g[j]] }) + out = append(out, g) + } + sort.Slice(out, func(i, j int) bool { return idx[out[i][0]] < idx[out[j][0]] }) + return out +} diff --git a/cmd/mxcli/script_set_test.go b/cmd/mxcli/script_set_test.go new file mode 100644 index 0000000000..940b20cd74 --- /dev/null +++ b/cmd/mxcli/script_set_test.go @@ -0,0 +1,215 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/executor" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// writeScript writes src to dir/name and returns the path. +func writeScript(t *testing.T, dir, name, src string) string { + t.Helper() + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte(src), 0o644); err != nil { + t.Fatal(err) + } + return p +} + +// captureStd runs fn with os.Stdout and os.Stderr captured, returning both. +func captureStd(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + so, se := os.Stdout, os.Stderr + os.Stdout, os.Stderr = w, w + done := make(chan string) + go func() { b, _ := io.ReadAll(r); done <- string(b) }() + defer func() { os.Stdout, os.Stderr = so, se }() + fn() + _ = w.Close() + os.Stdout, os.Stderr = so, se + return <-done +} + +const ( + setStub = "-- the placeholder\ncreate or modify microflow MyFirstModule.Export ()\nbegin\n log info node 'X' 'stub';\nend;\n" + setReal = "-- the real flow\ncreate or modify microflow MyFirstModule.Export ()\nbegin\n log info node 'X' 'real';\n log info node 'X' 'done';\nend;\n" + setOther = "create or modify microflow MyFirstModule.Other ()\nbegin\n log info node 'X' 'other';\nend;\n" +) + +func TestFindFlowRedeclarations(t *testing.T) { + parse := func(path, src string) setScript { + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatal(errs[0]) + } + return setScript{Path: path, Source: src, Prog: prog} + } + twice := "create or modify nanoflow M.N () begin end;\n\nCREATE OR REPLACE NANOFLOW M.\"N\" () begin end;\n" + got := findFlowRedeclarations([]setScript{ + parse("a.mdl", setStub), + parse("b.mdl", "mdl 1;\n"+setReal), + parse("c.mdl", setOther), + parse("d.mdl", twice), + parse("e.mdl", "create microflow M.Plain () begin end;\n"), + parse("f.mdl", "create microflow M.Plain () begin end;\n"), + }) + if len(got) != 2 { + t.Fatalf("want MyFirstModule.Export and M.N, got %+v", got) + } + if r := got[0]; r.Flow != "MyFirstModule.Export" || len(r.Sites) != 2 || + r.Sites[0].String() != "a.mdl:2" || r.Sites[1].String() != "b.mdl:3" || !r.mixedVersions() { + t.Errorf("the cross-file pair: %+v", r) + } + if r := got[1]; r.Flow != "M.N" || len(r.Sites) != 2 || r.Sites[0].String() != "d.mdl:1" || + r.Sites[1].String() != "d.mdl:3" || r.mixedVersions() || !r.Sites[0].Nanoflow { + t.Errorf("the in-file pair: %+v", r) + } + // One file declaring a flow twice has one header: nothing to decide together. + groups := fileGroups([]string{"a.mdl", "b.mdl", "c.mdl", "d.mdl"}, got) + if len(groups) != 1 || strings.Join(groups[0], ",") != "a.mdl,b.mdl" { + t.Errorf("file groups: %v", groups) + } +} + +// ako/mxcli#905: `check` over a script set warns when two of its `create or +// modify` statements declare one flow — a stub-then-real pair — naming both +// and saying to drop the stub. Controls: a set without a redeclaration, and +// one file alone (single-file behaviour is unchanged). +func TestCheck_ScriptSetWarnsOnStubThenReal(t *testing.T) { + // cobra keeps flag values between runs in one process: no project, text. + _ = rootCmd.PersistentFlags().Set("project", "") + if f := checkCmd.Flags().Lookup("format"); f != nil { + _ = f.Value.Set(f.DefValue) + } + dir := t.TempDir() + stub := writeScript(t, dir, "1-stub.mdl", setStub) + real := writeScript(t, dir, "2-real.mdl", setReal) + other := writeScript(t, dir, "3-other.mdl", setOther) + + var code int + out := captureStd(t, func() { code = runCheckFiles(checkCmd, []string{stub, real}) }) + if code != 0 { + t.Fatalf("a warning failed the run (code %d):\n%s", code, out) + } + for _, want := range []string{StubThenRealRule, "MyFirstModule.Export", stub + ":2", real + ":2", "drop the stub", "#843"} { + if !strings.Contains(out, want) { + t.Errorf("want %q in check's output:\n%s", want, out) + } + } + if !strings.Contains(out, "Checking syntax: "+stub) || !strings.Contains(out, "Checking syntax: "+real) { + t.Errorf("each file is still checked:\n%s", out) + } + + out = captureStd(t, func() { code = runCheckFiles(checkCmd, []string{stub, other}) }) + if code != 0 || strings.Contains(out, StubThenRealRule) { + t.Errorf("control, no redeclaration: code %d\n%s", code, out) + } + both := writeScript(t, dir, "both.mdl", setStub+setReal) + out = captureStd(t, func() { code = runCheckFiles(checkCmd, []string{both}) }) + if code != 0 || strings.Contains(out, StubThenRealRule) { + t.Errorf("control, one file: code %d\n%s", code, out) + } +} + +// ako/mxcli#905: `fmt --upgrade -p` over a stub-then-real script set decided +// the header file by file: the stub's file was declined it (exec would refuse +// its change to the stored real flow) and the real flow's file took it. Run in +// order, the mdl 0 stub then rebuilt the real flow and the mdl 1 real +// statement was refused, every run. Over the set, the header is decided for +// the pair together: here neither file takes it, and the pair is reported. +// Controls: the real file alone takes the header (single-file behaviour is +// unchanged), and so does it in a set without a redeclaration. +func TestFmtUpgrade_ScriptSetDecidesStubThenRealHeaderTogether(t *testing.T) { + src := filepath.Join("..", "..", "testdata", "pedapp") + if _, err := os.Stat(filepath.Join(src, "PedApp.mpr")); err != nil { + t.Skipf("PedApp fixture not found: %v", err) + } + dir := t.TempDir() + if err := copyTree(src, dir); err != nil { + t.Fatal(err) + } + mpr := filepath.Join(dir, "PedApp.mpr") + // The stored real flow: a loop body the stub's statement would change, + // which the splice cannot do, so the stub is refused under mdl 1. + const real = "create or modify microflow MyFirstModule.Grow ($Items: List of System.User)\nreturns String as $Out\nbegin\n declare $Out String = '';\n" + + " loop $U in $Items\n begin\n set $Out = $Out + ',';\n set $Out = $Out + $U/Name;\n end loop;\n return $Out;\nend;\n" + const stub = "create or modify microflow MyFirstModule.Grow ($Items: List of System.User)\nreturns String as $Out\nbegin\n declare $Out String = '';\n loop $U in $Items\n begin\n set $Out = $Out + ',';\n end loop;\n return $Out;\nend;\n" + exe := executor.New(io.Discard) + exe.SetBackendFactory(newBackendFactory()) + prog, errs := visitor.Build(fmt.Sprintf("connect local '%s';\n%s", visitor.QuoteString(mpr), real)) + if len(errs) > 0 { + t.Fatal(errs[0]) + } + if err := exe.ExecuteProgram(prog); err != nil { + t.Fatalf("store the real flow: %v", err) + } + _ = exe.Close() + + // Control: the real file alone takes the header. + alone := writeScript(t, t.TempDir(), "real.mdl", real) + if out, err := runFmt(t, "--upgrade", "-w", "-p", mpr, alone); err != nil { + t.Fatalf("the real file alone: %v\n%s", err, out) + } + if got, _ := os.ReadFile(alone); !strings.HasPrefix(string(got), "mdl 1;\n") { + t.Fatalf("control: the real file alone did not take the header:\n%s", got) + } + + // Control: in a set without a redeclaration it takes it too. + cdir := t.TempDir() + creal := writeScript(t, cdir, "2-real.mdl", real) + cother := writeScript(t, cdir, "3-other.mdl", setOther) + out, err := runFmt(t, "--upgrade", "-w", "-p", mpr, creal, cother) + if err != nil { + t.Fatalf("control set: %v\n%s", err, out) + } + if strings.Contains(out, StubThenRealRule) { + t.Errorf("control set reported a pair:\n%s", out) + } + for _, p := range []string{creal, cother} { + if got, _ := os.ReadFile(p); !strings.HasPrefix(string(got), "mdl 1;\n") { + t.Errorf("control set: %s did not take the header:\n%s", p, got) + } + } + + sdir := t.TempDir() + sstub := writeScript(t, sdir, "1-stub.mdl", stub) + sreal := writeScript(t, sdir, "2-real.mdl", real) + out, err = runFmt(t, "--upgrade", "-w", "-p", mpr, sstub, sreal) + if err != nil { + t.Fatalf("the stub-then-real set: %v\n%s", err, out) + } + if got, _ := os.ReadFile(sstub); string(got) != stub { + t.Errorf("the stub took the header exec would refuse it under:\n%s", got) + } + if got, _ := os.ReadFile(sreal); string(got) != real { + t.Errorf("the pair was split: the real file took the header its stub cannot take:\n%s", got) + } + for _, want := range []string{StubThenRealRule, "MyFirstModule.Grow", sstub + ":1", sreal + ":1", "decided for the files together", "drop the stub"} { + if !strings.Contains(out, want) { + t.Errorf("want %q in fmt's report:\n%s", want, out) + } + } + + // --force-header overrides the refusal for both: the pair stays together. + out, err = runFmt(t, "--upgrade", "--force-header", "-w", "-p", mpr, sstub, sreal) + if err != nil { + t.Fatalf("--force-header: %v\n%s", err, out) + } + for _, p := range []string{sstub, sreal} { + if got, _ := os.ReadFile(p); !strings.HasPrefix(string(got), "mdl 1;\n") { + t.Errorf("--force-header: %s did not take the header:\n%s", p, got) + } + } +} diff --git a/docs-site/src/appendixes/error-messages.md b/docs-site/src/appendixes/error-messages.md index 9d40449ed5..f608b87601 100644 --- a/docs-site/src/appendixes/error-messages.md +++ b/docs-site/src/appendixes/error-messages.md @@ -142,6 +142,21 @@ iterator name exists, so mxbuild rejects this with CE0109 "Undefined variable The rule keys on **scope, not on the name**. `$item` is perfectly valid in a predicate when it is the enclosing loop's iterator, which is how the O(N) lookup idiom is written — inside `loop $item in $L`, `find($Others, Key = $item/Key)` navigates the loop's variable and is not flagged. +### MDL-STUB01: One flow declared twice in a script set + +``` +MyFirstModule.Export is declared by 2 `create or modify` statements in this script +set: 13-actions.mdl:40 (mdl 0), 30-export.mdl:12 (mdl 1). Run in order, the first +replaces what the last one stored, on every run; ... A self-recursive flow no +longer needs a placeholder (#843): drop the stub and keep the real statement [MDL-STUB01] +``` + +**Cause:** A script set — `mxcli check` or `mxcli fmt --upgrade` given several files, read as one run in the order given — declares one microflow or nanoflow with two `create or modify` statements: a placeholder ("stub") that earlier scripts can reference, followed by the real flow. Run in order, the stub replaces the stored real flow and the real statement restores it, on every run. Under different language headers it is worse: the mdl 0 stub rebuilds the real flow, the mdl 1 real statement can be refused (its change cannot be spliced into the stub), and the project keeps running the placeholder with `mx check` reporting nothing (ako/mxcli#905). + +**Solution:** Drop the stub. Since #843 a self-recursive flow is created in one statement, and a script that only references the flow does not need it to exist until it runs. If the stub has to stay, give both files the same header: `mxcli fmt --upgrade -w -p app.mpr` over **all** the files decides the header for the pair together — added to both or to neither — where upgrading the files one at a time could split it. + +It is a warning. One file checked alone is not read as a set. + ### MDL-DEPRnnn: Deprecated spelling ``` From 87ed9015d9383ae22127a2a2bc4a85cfb54ce814 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 1 Oct 2026 17:33:07 +0000 Subject: [PATCH 3/5] fix(fmt): a pinned mdl 0 file holds its stub-then-real partner back; an already-headed file is reported, not declined (#905) canTakeHeader read any written header as 'already under it', so a stub pinned `mdl 0;` let its real file take `mdl 1;` and the pair split again. A file already carrying the header cannot be declined it (fmt never removes one); fmt now says the pair stays split instead of 'no language header added'. Co-Authored-By: Claude Opus 5.5 --- .../skills/fix-issue/findings/cmd-mxcli.jsonl | 1 + cmd/mxcli/cmd_fmt.go | 17 ++++-- cmd/mxcli/script_set_test.go | 52 +++++++++++++++++++ 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl index 1a059d139c..17a066416f 100644 --- a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl +++ b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl @@ -134,3 +134,4 @@ {"date": "2026-09-30", "area": "cmd/mxcli", "symptom": "`mxcli fmt --upgrade tests/csv-import.test.mdl` failed with `no viable alternative at input '/**\\n * @test …'` on a file `mxcli check` passes, so the migration tool could not reach a project's test suite; separately `mxcli check x.test.md` reported a syntax error on every valid markdown test block whose doc comment spans lines.", "cause": "fmt parsed the file as top-level MDL, but a test file is `/** @test */` doc comments and `/` separators around microflow bodies; only check rendered it (testrunner.CheckSource). The markdown parser counted a fence block's lines from the ```mdl-test line instead of the line after it, so BodyLine was one early and the rendering overwrote the doc comment's last line with the body.", "fix": "testrunner.UpgradeSource upgrades CheckSource's line-preserving rendering and maps it back line for line: a line the rendering kept verbatim takes the upgraded text, every other line stays the author's, and a rewrite reaching a wrapper line or changing the line count is refused. fmt routes test files there (plain fmt on one is refused with a pointer to --upgrade). parseMarkdownTests passes blockStart+1 as the chunk line.", "insight": "A test file takes no language header: neither check's rendering nor either runner generator reads a `mdl 1;` line, and one in front of the first @test makes that test's doc comment non-leading, so the runner silently drops it. --header therefore adds no header to a test file and says so, rather than applying mdl 1 rewrites the runner would execute as mdl 0. The markdown BodyLine bug was found only because the upgrade's corpus test re-parsed the rendering of mdl-examples/doctype-tests/microflow-spec.test.md; it also shrank the conformance allowlist.", "issue": "ako/mxcli#837", "file": "cmd/mxcli/testrunner/upgrade_source.go, cmd/mxcli/testrunner/parser.go (parseMarkdownTests), cmd/mxcli/cmd_fmt.go", "test": "cmd/mxcli/testrunner/upgrade_source_test.go, TestCheckSourceMarkdownBodyLine, cmd/mxcli/cmd_fmt_upgrade_test.go TestFmtUpgrade_TestFile"} {"date": "2026-10-01", "area": "cmd/mxcli", "symptom": "The LSP's CREATE MICROFLOW / CREATE NANOFLOW / CREATE ENUMERATION snippet completions inserted MDL that does not parse under any language version (`missing '(' at 'BEGIN'`, a quoted enumeration value name), the CONSTANT snippet the deprecated clause form (MDL-DEPR136), and PAGE/SNIPPET a statement mdl 1 refuses for its missing `;`; completion also offered alias-only keywords (SHOW_PAGE, DELETE_BEHAVIOR, DEFINE) and the `show entities` listings after SHOW.", "cause": "The snippets and keyword lists were hand-written once and never parsed; the grammar moved on (R2 property lists, R8 words, `list` for `show`) and nothing tied the completion text to it. The keyword list is generated from the lexer alone, which cannot tell an alias token from a canonical one: that is said by the `/* @alias MDL-DEPRnnn */` markers in the parser grammar.", "fix": "Snippets rewritten to canonical mdl 1. cmd/gen-completions reads the parser grammar too and drops a token every parser-rule use of which carries an @alias marker (the `keyword` rule, which lists tokens usable as names, does not count). `list` gets the listings; `show` offers page/message/home page.", "insight": "A completion text is MDL that ships in the binary, so it is held to what docs are held to: TestCompletionSnippetsAreMdl1 parses every snippet, expanded with its defaults, under `mdl 1;` and requires no deprecation. Deciding alias-only from the markers is the registry's own data; a token-swap rewrite word (`snippet`, `column`, `comment`) is NOT alias-only \u2014 those words stay canonical elsewhere \u2014 and neither is SHOW (show page, show message).", "issue": "ako/mxcli#714 (decision 5)", "file": "cmd/mxcli/lsp_completion.go, cmd/gen-completions/aliases.go", "test": "cmd/mxcli/lsp_mdl1_test.go TestCompletionSnippetsAreMdl1, TestCompletionOffersNoDeprecatedSpelling, TestCompletionListAndShowContinueIntoMdl1; cmd/gen-completions/aliases_test.go"} {"date": "2026-10-01", "area": "cmd/mxcli", "symptom": "A stub-then-real script set (two files each with `create or modify microflow X`) upgraded with `fmt --upgrade -p` one file at a time ended up split: the stub's file declined the header (MDL-V1-REBUILD) and the real flow's file took it. Run in order, the mdl 0 stub rebuilt the stored real flow and the mdl 1 real statement was refused, every run; mx check 0 errors, the app running the placeholder.", "cause": "Each command judged one file against the model as stored; check/fmt took exactly one file, so nothing could see a flow declared twice across the set, and a per-file header decision is wrong for a pair whose files must agree.", "fix": "check and fmt accept several files as one script set (cmd/mxcli/script_set.go): findFlowRedeclarations finds a flow declared by two create-or-modify statements; check warns MDL-STUB01 naming both; fmt --upgrade groups files sharing such a flow and declines the header for all of them when any one cannot take it (canTakeHeader = the per-file verdict). One file alone is unchanged.", "insight": "A per-file verdict is only right when the files are independent; the run is the unit when two files write the same document. The symptom hides because each half is individually correct (decline is right for the stub, header is right for the real file) - only the combination is wrong, so test the set, with the single-file decision as the control.", "issue": "ako/mxcli#905", "file": "cmd/mxcli/script_set.go", "test": "cmd/mxcli/script_set_test.go TestFmtUpgrade_ScriptSetDecidesStubThenRealHeaderTogether, TestCheck_ScriptSetWarnsOnStubThenReal"} +{"date": "2026-10-01", "area": "cmd/mxcli", "symptom": "fmt --upgrade over a stub-then-real set still split the pair when the stub's file carried a written `mdl 0;`: the real file took `mdl 1;`. With the real file already `mdl 1;`, fmt printed 'no language header added' about it while the pair stayed split.", "cause": "canTakeHeader returned true for ANY written header (langver.ScanWrittenHeader's bool), reading a pinned `mdl 0;` as 'already has the header'; and the group decline was applied to a file that already carries the header, which fmt never removes.", "fix": "canTakeHeader: a written header can take it only when it is langver.Latest. decideSetHeaders: a file already under the header is not declined; fmt says the pair stays under different headers and to drop the stub or take the header off.", "insight": "ScanWrittenHeader's bool means 'a header is written', not 'the header is mdl 1'; a written mdl 0 pin is the strongest 'cannot take it' there is. Test the group decision with every header state of each file, not only headerless ones.", "issue": "ako/mxcli#905", "file": "cmd/mxcli/cmd_fmt.go", "test": "cmd/mxcli/script_set_test.go TestFmtUpgrade_ScriptSetPinnedMdl0StubHoldsTheRealFileBack, TestFmtUpgrade_ScriptSetAlreadySplitPairIsReportedAsSplit"} diff --git a/cmd/mxcli/cmd_fmt.go b/cmd/mxcli/cmd_fmt.go index d5679c473b..540c736449 100644 --- a/cmd/mxcli/cmd_fmt.go +++ b/cmd/mxcli/cmd_fmt.go @@ -475,6 +475,15 @@ func decideSetHeaders(cmd *cobra.Command, scripts []setScript, redecls []flowRed continue } for _, f := range can { + // fmt never removes a written header: a file already under it + // cannot be held back, and the pair stays split until the stub + // goes or the header is taken off by hand. + if v, written := langver.ScanWrittenHeader(src[f]); written { + fmt.Fprintf(cmd.ErrOrStderr(), "%s: already has the %s header, which fmt does not remove, while %s "+ + "cannot take it: the stub-then-real pair stays under different headers (%s); drop the stub, "+ + "or take the header off this file\n", f, v, strings.Join(cannot, ", "), StubThenRealRule) + continue + } decline[f] = fmt.Sprintf("it declares a flow that %s also declares, and that file cannot take the "+ "header; a stub-then-real pair under different headers lets the mdl 0 statement rebuild the flow "+ "on every run while the mdl 1 one is refused, so the header is decided for the files together "+ @@ -485,12 +494,14 @@ func decideSetHeaders(cmd *cobra.Command, scripts []setScript, redecls []flowRed } // canTakeHeader reports whether fmt --upgrade, run on src alone, would leave -// it under the language header: it has one already, or the upgrade can add +// it under the language header: it has it written already, or the upgrade can add // one (no construct blocks it) and, with -p, exec would refuse none of its // statements under it (or --force-header overrides that). func canTakeHeader(cmd *cobra.Command, src string) bool { - if _, written := langver.ScanWrittenHeader(src); written { - return true + // A written header is the file's version: `mdl 0;` pins it, and + // --upgrade leaves it there. + if v, written := langver.ScanWrittenHeader(src); written { + return v == langver.Latest } opts := upgrade.DefaultOptions() opts.AddHeader = true diff --git a/cmd/mxcli/script_set_test.go b/cmd/mxcli/script_set_test.go index 940b20cd74..fe7f827338 100644 --- a/cmd/mxcli/script_set_test.go +++ b/cmd/mxcli/script_set_test.go @@ -213,3 +213,55 @@ func TestFmtUpgrade_ScriptSetDecidesStubThenRealHeaderTogether(t *testing.T) { } } } + +// ako/mxcli#905 (review): a written header is the file's version, not a +// header it has already taken. A stub pinned `mdl 0;` keeps mdl 0 under +// --upgrade, so its real flow's file must be declined the header as well — +// it was counted as "can take it" and the pair came out split, mdl 0 stub +// before mdl 1 real. Control: a written `mdl 1;` stub does not hold the real +// file back. +func TestFmtUpgrade_ScriptSetPinnedMdl0StubHoldsTheRealFileBack(t *testing.T) { + dir := t.TempDir() + stub := writeScript(t, dir, "1-stub.mdl", "mdl 0;\n"+setStub) + real := writeScript(t, dir, "2-real.mdl", setReal) + out, err := runFmt(t, "--upgrade", "-w", stub, real) + if err != nil { + t.Fatalf("%v\n%s", err, out) + } + if got, _ := os.ReadFile(real); string(got) != setReal { + t.Errorf("the pair was split: the real file took the header its mdl 0 stub keeps off:\n%s\n%s", got, out) + } + if !strings.Contains(out, "decided for the files together") { + t.Errorf("want the decline reported:\n%s", out) + } + + cdir := t.TempDir() + cstub := writeScript(t, cdir, "1-stub.mdl", "mdl 1;\n"+setStub) + creal := writeScript(t, cdir, "2-real.mdl", setReal) + if out, err := runFmt(t, "--upgrade", "-w", cstub, creal); err != nil { + t.Fatalf("control: %v\n%s", err, out) + } + if got, _ := os.ReadFile(creal); !strings.HasPrefix(string(got), "mdl 1;\n") { + t.Errorf("control: a written mdl 1 stub held the real file back:\n%s", got) + } +} + +// ako/mxcli#905 (review): fmt never removes a written header, so a file of +// the group that already carries `mdl 1;` cannot be "declined" it. fmt said +// "no language header added" about it while the pair stayed split; it must +// say the pair stays under different headers instead. +func TestFmtUpgrade_ScriptSetAlreadySplitPairIsReportedAsSplit(t *testing.T) { + dir := t.TempDir() + stub := writeScript(t, dir, "1-stub.mdl", "mdl 0;\n"+setStub) + real := writeScript(t, dir, "2-real.mdl", "mdl 1;\n"+setReal) + out, err := runFmt(t, "--upgrade", "-w", stub, real) + if err != nil { + t.Fatalf("%v\n%s", err, out) + } + if strings.Contains(out, real+": no language header added") { + t.Errorf("fmt claims it kept the header off a file that already has it:\n%s", out) + } + if !strings.Contains(out, real+": already has the mdl 1 header") { + t.Errorf("want the split pair named:\n%s", out) + } +} From f47f6c74bae35aad41e061ddb38786f6976dedbf Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 1 Oct 2026 17:37:50 +0000 Subject: [PATCH 4/5] fix(flow-modify): a message written as an expression matches its stored '{1}' template (#905) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A log, show message or validation feedback whose message is an expression (`'failed for ' + $User/Name`) is stored as the template '{1}' with the expression as its parameter, and describe prints that form. declaredMatches compared the two spellings as different statements, so wherever the built comparison does not decide (a spliced or Studio Pro-drawn flow) the statement diff replaced the activity on every run: absorbed by the write elision on the main path, refused under mdl 1 when the message sits in a stored activity's error handler ("cannot replace … it has an error handler"). That was the second run of CapTrack's ACT_Export_Excel once the splice could grow it. matchValue normalises the three statements to the builder's stored form. Co-Authored-By: Claude Opus 5.5 --- mdl/executor/flow_declared_match.go | 51 ++++++++++++++++++++ mdl/executor/flow_message_respelling_test.go | 48 ++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 mdl/executor/flow_message_respelling_test.go diff --git a/mdl/executor/flow_declared_match.go b/mdl/executor/flow_declared_match.go index 2e5b79b0c7..7f17de2cdf 100644 --- a/mdl/executor/flow_declared_match.go +++ b/mdl/executor/flow_declared_match.go @@ -129,6 +129,9 @@ func matchValue(d, s reflect.Value, mode matchMode) bool { if d.Type() == dataTypeStructType { d, s = storedFlowDataType(d), storedFlowDataType(s) } + if messageStructTypes[d.Type()] { + d, s = messageAsTemplate(d), messageAsTemplate(s) + } geo := geometryFields[d.Type()] for i := 0; i < d.NumField(); i++ { df := d.Field(i) @@ -204,6 +207,54 @@ func storedFlowDataType(v reflect.Value) reflect.Value { return reflect.ValueOf(dt) } +// messageStructTypes are the statements whose message the builder stores as +// a template: a string literal as its text, any other expression as '{1}' +// with the expression as the first parameter. +var messageStructTypes = map[reflect.Type]bool{ + reflect.TypeOf(ast.LogStmt{}): true, + reflect.TypeOf(ast.ShowMessageStmt{}): true, + reflect.TypeOf(ast.ValidationFeedbackStmt{}): true, +} + +// messageAsTemplate returns v, a statement with a message, with its message +// in the form the builder stores it (addLogMessageAction, +// addShowMessageAction, addValidationFeedbackAction): `'failed for ' + $Name` +// is the template '{1}' with ({1} = 'failed for ' + $Name), which describe +// prints — so the two spellings are one activity (ako/mxcli#905). A log +// that states its own parameters keeps its message as written, as the +// builder does. +func messageAsTemplate(v reflect.Value) reflect.Value { + templated := func(msg ast.Expression) bool { + if msg == nil { + return false + } + lit, ok := msg.(*ast.LiteralExpr) + return !ok || lit.Kind != ast.LiteralString + } + placeholder := &ast.LiteralExpr{Kind: ast.LiteralString, Value: "{1}"} + switch st := v.Interface().(type) { + case ast.LogStmt: + if len(st.Template) == 0 && templated(st.Message) { + st.Template = []ast.TemplateParam{{Index: 1, Value: st.Message}} + st.Message = placeholder + } + return reflect.ValueOf(st) + case ast.ShowMessageStmt: + if templated(st.Message) { + st.TemplateArgs = append([]ast.Expression{st.Message}, st.TemplateArgs...) + st.Message = placeholder + } + return reflect.ValueOf(st) + case ast.ValidationFeedbackStmt: + if templated(st.Message) { + st.TemplateArgs = append([]ast.Expression{st.Message}, st.TemplateArgs...) + st.Message = placeholder + } + return reflect.ValueOf(st) + } + return v +} + var expressionType = reflect.TypeOf((*ast.Expression)(nil)).Elem() // sameMendixExpression compares two Mendix expressions of a statement by diff --git a/mdl/executor/flow_message_respelling_test.go b/mdl/executor/flow_message_respelling_test.go new file mode 100644 index 0000000000..e07682b187 --- /dev/null +++ b/mdl/executor/flow_message_respelling_test.go @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import "testing" + +// ako/mxcli#905: a message written as an expression — `log … 'failed for ' + +// $User/Name` — is stored as the template '{1}' with the expression as its +// parameter, and describe prints it so. The two spellings are one activity; +// compared as written they were two, so a statement re-run against a flow the +// splice had grown saw its activity changed. Outside an error handler that is a +// replace the write elision absorbs; for an activity whose handler holds the +// message (CapTrack ACT_Export_Excel) it was refused on every run after the +// first ("it has an error handler, which would be left with no activity"). +func TestDeclaredMatches_MessageExpressionIsItsTemplate(t *testing.T) { + stored := parseFlowBody(t, ` log node 'N' '{1}' with ({1} = 'v ' + $In); + show message '{1}' type Warning with ({1} = 'Hi ' + $In); + validation feedback $In/Name message '{1}' with ({1} = 'bad ' + $In);`) + for _, c := range []struct { + name, body string + want bool + }{ + {"as written", ` log info node 'N' 'v ' + $In; + show message 'Hi ' + $In type Warning; + validation feedback $In/Name message 'bad ' + $In;`, true}, + {"as described", ` log node 'N' '{1}' with ({1} = 'v ' + $In); + show message '{1}' type Warning with ({1} = 'Hi ' + $In); + validation feedback $In/Name message '{1}' with ({1} = 'bad ' + $In);`, true}, + // Controls: a real change is still one. + {"control: another log text", ` log info node 'N' 'w ' + $In; + show message 'Hi ' + $In type Warning; + validation feedback $In/Name message 'bad ' + $In;`, false}, + {"control: another message text", ` log info node 'N' 'v ' + $In; + show message 'Ho ' + $In type Warning; + validation feedback $In/Name message 'bad ' + $In;`, false}, + {"control: another feedback text", ` log info node 'N' 'v ' + $In; + show message 'Hi ' + $In type Warning; + validation feedback $In/Name message 'worse ' + $In;`, false}, + {"control: the template as a literal", ` log info node 'N' '{1}'; + show message 'Hi ' + $In type Warning; + validation feedback $In/Name message 'bad ' + $In;`, false}, + } { + declared := parseFlowBody(t, c.body) + if got := declaredMatches(declared.Body, stored.Body); got != c.want { + t.Errorf("%s: declaredMatches = %v, want %v", c.name, got, c.want) + } + } +} From d245209cfee75027c5ad4674604dd423cda97832 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 1 Oct 2026 17:37:50 +0000 Subject: [PATCH 5/5] fix(flow-modify): a new activity's error handler may end in its own return when spliced (#905) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under mdl 1, `create or modify` refused to grow a stored flow by an activity whose custom error handler ends in `return` ("an error handler in the fragment ends at an end event of its own … a return inside an error handler is not spliced yet"), as an insert or a replace, on every run; mdl 0 rebuilt the flow. On CapTrack this left a stub-then-real pair with the placeholder stored. The handler is built by a child flowBuilder whose returnEndIDs never reached the parent, so cutFragment took the handler's return end event for one the builder added. addErrorHandlerFlow now carries them up: the return is a new end event of the flow, placed where the builder drew it relative to the fragment, like a guard clause's (#888), and checkRoom/checkBranches refuse where it would be drawn over or across stored content. Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .../skills/mendix/write-microflows/SKILL.md | 4 +- .../skills/mendix/write-nanoflows/SKILL.md | 3 +- mdl/executor/cmd_alter_flow.go | 13 +- mdl/executor/cmd_alter_flow_cut_test.go | 11 +- .../cmd_alter_flow_handler_return_test.go | 67 +++++ mdl/executor/cmd_microflows_builder_flows.go | 11 + .../flow_splice_handler_return_test.go | 242 ++++++++++++++++++ 8 files changed, 336 insertions(+), 16 deletions(-) create mode 100644 mdl/executor/cmd_alter_flow_handler_return_test.go create mode 100644 mdl/roundtrip/flow_splice_handler_return_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 03966f81c2..db4ac13631 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -790,3 +790,4 @@ {"date": "2026-10-01", "area": "mdl/executor", "symptom": "ako/mxcli#887 (rehearsal 2, N1): `create or modify microflow|nanoflow` of a stored, foldered flow with NO folder clause moved it to the module root (`Moved microflow: ...`), both language versions; `mxcli diff` said 'moved to the module root'. With a separate organise step (`move ... to folder`) every run moved the flows out and back: 19 flows a run in mxcli-rest, 47-55 in formula1, the .mpr rewritten each time although no unit's content changed.", "cause": "The splice path (modifyFlowInPlace) compared the declared folder with the stored folder and moved on any difference, treating an empty clause as 'the module root'. Every other create-or-modify path follows document_placement.go's rule that no folder clause leaves the document where it is (containerForDocument / resolveRequestedFolder, or `if s.Folder == \"\" { container = existing }`); the flow rebuild path did too. spliceVerdict in cmd_diff.go copied the same comparison.", "file": "mdl/executor/cmd_flow_modify.go, mdl/executor/cmd_diff.go", "fix": "movesFolder(declared, stored) = declared != \"\" && declared != stored, used by modifyFlowInPlace (which now resolves via resolveRequestedFolder) and by spliceVerdict. Audited every resolveFolder/applyDocumentFolder caller (pages, snippets, rules, enumerations, constants, json structures, mappings, message definitions, business events, published/consumed REST, OData, agent-editor documents, workflows, queues, ...): all already keep the stored container when the clause is absent.", "test": "mdl/roundtrip/flow_modify_folder_test.go TestSpliceRerun_NoFolderClauseKeepsTheFlowFiled (integration, parity suite; Studio Pro-authored PedApp Administration.ChangeMyPassword and Atlas_Web_Content.DS_LoginContext, both headers: no Moved report, folder kept, exec 2 writes nothing, organised re-run leaves the .mpr byte-identical, diff reports no move; control: a folder clause naming another folder still moves). Revert check: all four subject/header pairs fail with 'moved the flow out of' and the organised re-run rewrites the .mpr.", "insight": "A move that is undone in the same run is invisible to a unit snapshot (contents and final container are the same) and does not move LastTransactionID; only the .mpr bytes show it. When a second path implements a rule a helper already encodes (no clause = leave it), route it through the helper rather than re-deriving the comparison."} {"date": "2026-10-01", "area": "mdl/executor", "symptom": "ako/mxcli#890 (rehearsal 2, R-rep): re-running a settled script wrote nothing (units_written=0) but still printed \"Granted access on ...\", \"Set project security level to ...\", \"Added module roles ... to user role ...\", \"Updated ... settings\" / \"Updated configuration ...\", and `move ... to folder` printed \"Moved ... to new location\"; the output could not serve as the #859 'second run reports Unchanged' gate. Also: a second `move` of the same document in one session failed \"microflow not found\".", "cause": "Those handlers printed their sentence with fmt.Fprintf after the backend call instead of going through ReportMutation's write-elision evidence (WriteStats offered vs written). Grants/revokes inside a program run are deferred (#872 accessRuleRun), so even ReportMutation could not see their write at statement time. The typed movers changed a container without invalidating the cached hierarchy.", "file": "mdl/executor/report_mutation.go, mdl/executor/access_rule_run.go, mdl/executor/cmd_security_write.go, mdl/executor/cmd_settings.go, mdl/executor/cmd_move.go", "fix": "ExecContext.reportWrite(unchanged, sentence...) prints the sentence or `Unchanged ` (through the run tally) on the ReportMutation evidence rule; used for project security level/demo users/strict mode/guest access, alter user role module roles, settings section/configuration/constant updates. Access-rule reports go through reportAccessRule: held on the open accessRuleRun and printed at its flush, Unchanged when the flush offered and elided (notices like 'No access rules found' print regardless). execMove short-circuits a document already in the target container (alreadyPlaced -> Unchanged) and invalidates the hierarchy after every move.", "test": "mdl/executor/noop_reporting_pedapp_test.go TestNoopRerun_ReportsUnchanged (PedApp, per statement: run 1 reports its write = control; run 2 writes no file and reports Unchanged, for grant, security level, demo users, strict mode, user role module roles, settings runtime, configuration (alter and create or modify), move) and TestNoopRerun_ProgramReportsUnchanged (program run incl. a revoke+grant reset; run 2 writes nothing and reports no write verb; run 1's net-nothing reset reports no write). Revert check: every case fails with the write sentence on run 2; the move case with 'microflow not found'.", "insight": "A report printed after a backend call is a claim about storage the handler cannot make on its own; route every write report through the write-stats evidence, and where writes are deferred, defer the report with them. The output only becomes an idempotency gate when no statement prints a write verb by construction."} {"date": "2026-10-01", "area": "mdl/executor", "symptom": "ako/mxcli#840 (mxcli-ledger, finding 162): `mxcli describe` (no header, no option) wrote mdl 0 spellings that mdl 1 refuses, and its microflow output held `$N = count($Hits)` / `$x = find($L, …)` — call forms registered as deprecated (MDL-DEPR003/004), so subcommand output was neither mdl 1-runnable nor mdl 0-clean. At the freeze a second instance surfaced on TestApp: describe of a chart series' text template wrote `staticTooltipHoverTextParams: [{1} = X]`, the bracketed form MDL-DEPR124 deprecates, under both versions.", "cause": "formatListOperation / the AggregateListAction case gated the statement form on describeLanguage >= V1 and fell back to the call form for mdl 0, although the statement form parses with the same meaning and no warning under mdl 0. The object-list describer (cmd_pages_describe_objectlist.go) wrote a TextTemplate's Params as \"[\" + … + \"]\". The roundtrip test that should have caught both (describeUsesCanonicalSpellings) filtered deprecations to an R8 allowlist ('describe keeps them under mdl 0'), so any code outside the list was invisible.", "file": "mdl/executor/cmd_microflows_format_action.go, mdl/executor/cmd_pages_describe_objectlist.go, mdl/roundtrip/describe_canonical_spelling_test.go", "fix": "Describe writes the List operation / Aggregate list statement in every language; only an activity the statement cannot express falls back to the call. Object-list template parameters are written in ( ). The canonical-spelling roundtrip test now checks EVERY registered deprecation under both describe languages (describeAs V1 and V0) on PedApp and TestApp, with no code filter. Freeze: langver.Frozen = V1, every describe output starts with `mdl 1;`, `--mdl 0|1` on describe/context/diff-local.", "test": "mdl/executor/cmd_microflows_format_list_activity_test.go TestDescribeListActivityUnderMdl1 (both versions, plus the call-form control warning under mdl 0); mdl/roundtrip/describe_canonical_spelling_test.go TestTestAppDescribeUsesCanonicalSpellings (failed on Snip_TaskDashboard_Numbers & 2 more with MDL-DEPR124 before the objectlist fix).", "insight": "A test that filters warnings to a list of 'codes this test owns' hides every code added later; a 'never emits X' property must check the whole registry, and in every output language the command can be asked for."} +{"date": "2026-10-01", "area": "mdl/executor", "symptom": "ako/mxcli#905 part 1 (#897 item 4, rehearsal 3 G2): under `mdl 1`, `create or modify microflow` that grows a stored flow by an activity whose custom error handler ends in its own `return` (`$Ok = call microflow … on error begin log …; return; end error;`) was refused on every run, as an insert or a replace: \"an error handler in the fragment ends at an end event of its own … a return inside an error handler is not spliced yet\". Created fresh the statement worked; mdl 0 rebuilt (MDL-V1-REBUILD). On CapTrack the stub-then-real pair (13-actions stub, 30-export real ACT_Export_Excel) left the placeholder stored on every run with mx check clean. Once spliced, the second run of the real CapTrack script was refused: \"replace $Written: cannot replace the ActionActivity …: it has an error handler\".", "cause": "addErrorHandlerFlow (cmd_microflows_builder_flows.go) builds a handler body with a child flowBuilder and merged its objects and flows into the parent, but not its returnEndIDs, so cutFragment saw the handler's return end event as one the builder added and refused it. Second defect, exposed by the grow: a log/show message/validation feedback message written as an expression (`'failed for ' + $User/Name`) is stored as template '{1}' with the expression as parameter, and describe prints that form; declaredMatches compared the two spellings as different statements. Where builtAsStored is false (a spliced or Studio Pro-drawn flow), the statement diff then replaced the activity: absorbed by write elision on the main path, but refused when the message sits in a stored activity's error handler.", "file": "mdl/executor/cmd_microflows_builder_flows.go, mdl/executor/flow_declared_match.go", "fix": "addErrorHandlerFlow copies errBuilder.returnEndIDs into the parent's (lastReturnEndID untouched), so a handler's return is a new end event of the flow like a guard's (#888); placement/room checks (checkRoom/checkBranches) apply unchanged and refuse where the handler's return branch would cross a stored flow. matchValue normalises LogStmt/ShowMessageStmt/ValidationFeedbackStmt (messageAsTemplate) to the builder's stored form: a non-literal message becomes '{1}' with the expression as first parameter (a log stating its own `with (...)` keeps its message, as the builder does).", "test": "mdl/executor/cmd_alter_flow_handler_return_test.go TestCutFragment_HandlerReturnIsANewEndEvent; mdl/executor/flow_message_respelling_test.go (with controls); mdl/roundtrip/flow_splice_handler_return_test.go TestSpliceRerun_GrowByHandlerReturn (replace/insert under mdl 1, insert under mdl 0, verdict agreement check/diff/exec, twice-exec, a changed-flow control), TestSpliceRerun_GrowStudioProFlowByHandlerReturn (PedApp ShowPasswordForm, all stored IDs kept, description fixed point), TestSpliceRerun_HandlerReturnWithNoRoomIsRefused. Revert checks: returnEndIDs not copied -> every grow refused 'not spliced yet' (check predicts it); messageAsTemplate off -> second run refused 'replace $Ok … it has an error handler'. CapTrack copy: fmt --upgrade -p 13+30, exec 13, 30, 30 -> real flow stored, third run 0 units; mx check identical to baseline (0 errors). PedApp repros mx check identical to baseline.", "insight": "Child builders (error handler, loop) each keep builder state the parent's consumers rely on; when a new piece of builder state is added for the splice (returnEndIDs, #888), enumerate the child builders and decide per child whether it propagates. A grow test that only re-runs on an mxcli-authored flow can pass because builtAsStored short-circuits the statement diff; the respelling only surfaced on the real project and the Studio Pro-drawn flow, where the statement diff decides."} diff --git a/.claude/skills/mendix/write-microflows/SKILL.md b/.claude/skills/mendix/write-microflows/SKILL.md index c2c1358cdc..6b8d498a0e 100644 --- a/.claude/skills/mendix/write-microflows/SKILL.md +++ b/.claude/skills/mendix/write-microflows/SKILL.md @@ -41,8 +41,8 @@ Choose the mode by who owns the microflow ([choose-edit-mode](../choose-edit-mod (or fresh `describe` output) and re-run `create or modify`. - **Authored in Studio Pro:** prefer `alter microflow X { insert/replace/drop … }` (targets from `describe microflow X with handles`). `create or modify` of `describe` - output patches too: unchanged writes nothing; a statement, guard clause, `return` value, `if` - condition, header clause, parameter (added/retyped; removed only if unused) or stated `@position`/`@start` change is + output patches too: unchanged writes nothing; a statement (even one whose handler returns), guard + clause, `return` value, `if` condition, header clause, parameter (added/retyped; removed only if unused) or stated `@position`/`@start` change is patched in place (a move keeps the node's flows). A redrawn `@anchor`/`@curve`, loop body, error handler or other `return` added/taken away rebuilds under mdl 0 (`MDL-V1-REBUILD`: IDs renumbered, merges and curves lost) and is refused under `mdl 1;`. diff --git a/.claude/skills/mendix/write-nanoflows/SKILL.md b/.claude/skills/mendix/write-nanoflows/SKILL.md index fdb0b856ed..38acc40c5e 100644 --- a/.claude/skills/mendix/write-nanoflows/SKILL.md +++ b/.claude/skills/mendix/write-nanoflows/SKILL.md @@ -28,7 +28,8 @@ Choose the mode by who owns the nanoflow ([choose-edit-mode](../choose-edit-mode output also works as a patch: an unchanged definition writes nothing, and an inserted, replaced or dropped statement (at the top level or in an `if` branch) is spliced in, leaving every other node, merge and curve as stored — a guard clause (`if … then return; - end if;`) too, its return a new end event; a changed `return` value or `if` condition is set on + end if;`) too, its return a new end event, and so is the `return` ending a new activity's + error handler; a changed `return` value or `if` condition is set on the stored end event or decision, and a body without a trailing `return` means the stored end. The header (documentation, return type, parameters added, retyped or — when unused — removed) is set on the stored document, and a stated `@position` or `@start` moves the diff --git a/mdl/executor/cmd_alter_flow.go b/mdl/executor/cmd_alter_flow.go index 57b945d274..d57437f400 100644 --- a/mdl/executor/cmd_alter_flow.go +++ b/mdl/executor/cmd_alter_flow.go @@ -416,9 +416,10 @@ func (a *alterFlowContext) buildFragment(ctx *ExecContext, body []ast.MicroflowS // An end event a `return` in the fragment drew (returns) is part of the // fragment: a guard clause's return ends its own path, which branches off the // rest of the flow, and is written as a new end event of the flow where the -// builder drew it relative to the fragment (ako/mxcli#888). Any other end event -// is one the builder adds to end an error handler at the fragment's end — -// with the handler's own return, or the flow's default value — and is refused. +// builder drew it relative to the fragment (ako/mxcli#888). So is the return +// that ends an error handler of the fragment (ako/mxcli#905). Any other end +// event is one the builder adds to end a handler that states no return, with +// the flow's default value, and is refused. func cutFragment(oc *microflows.MicroflowObjectCollection, fallThrough model.ID, returns map[model.ID]bool) (*backend.MicroflowFragment, error) { var start, end microflows.MicroflowObject for _, obj := range oc.Objects { @@ -430,10 +431,8 @@ func cutFragment(oc *microflows.MicroflowObjectCollection, fallThrough model.ID, case obj.GetID() == fallThrough: end = obj case !returns[obj.GetID()]: - // A handler's return draws this end event too, so the - // refusal must not ask for one. - return nil, fmt.Errorf("an error handler in the fragment ends at an end event of its own; an inserted " + - "error handler has to rejoin the rest of the flow (a return inside an error handler is not spliced yet)") + return nil, fmt.Errorf("an error handler in the fragment ends at an end event the script does not state; " + + "an inserted error handler has to rejoin the rest of the flow or end in a return of its own") } } } diff --git a/mdl/executor/cmd_alter_flow_cut_test.go b/mdl/executor/cmd_alter_flow_cut_test.go index ec105a2487..18836fe66f 100644 --- a/mdl/executor/cmd_alter_flow_cut_test.go +++ b/mdl/executor/cmd_alter_flow_cut_test.go @@ -11,10 +11,9 @@ import ( ) // ako/mxcli#888: an end event in a fragment that no `return` drew is one the -// builder added to end an error handler, and the splice refuses it. The -// refusal must not tell the user to "end that path with a return": a handler -// that does state one (`on error … { return false; }`) draws exactly this end -// event, so that advice cannot be followed. +// builder added to end an error handler, and the splice refuses it, naming the +// handler. (A handler that states its own return draws an end event the cut +// keeps since ako/mxcli#905: TestCutFragment_HandlerReturnIsANewEndEvent.) func TestCutFragment_HandlerEndRefusalDoesNotAskForAReturn(t *testing.T) { start := µflows.StartEvent{BaseMicroflowObject: microflows.BaseMicroflowObject{BaseElement: model.BaseElement{ID: "start"}}} act := µflows.ActionActivity{BaseActivity: microflows.BaseActivity{BaseMicroflowObject: microflows.BaseMicroflowObject{BaseElement: model.BaseElement{ID: "act"}}}} @@ -30,7 +29,7 @@ func TestCutFragment_HandlerEndRefusalDoesNotAskForAReturn(t *testing.T) { if !strings.Contains(err.Error(), "error handler") { t.Errorf("the refusal does not name the error handler: %v", err) } - if strings.Contains(err.Error(), "end that path with a return") || strings.Contains(err.Error(), "states no return") { - t.Errorf("the refusal asks for a return, which a handler that returns already states: %v", err) + if strings.Contains(err.Error(), "not spliced yet") { + t.Errorf("the refusal says a handler's return is not spliced, which it is: %v", err) } } diff --git a/mdl/executor/cmd_alter_flow_handler_return_test.go b/mdl/executor/cmd_alter_flow_handler_return_test.go new file mode 100644 index 0000000000..4320918b54 --- /dev/null +++ b/mdl/executor/cmd_alter_flow_handler_return_test.go @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// ako/mxcli#905 (#897 item 4, rehearsal 3 class G2): a fragment whose custom +// error handler ends in its own `return` was refused by the splice — "an error +// handler in the fragment ends at an end event of its own … a return inside an +// error handler is not spliced yet" — because the handler is built by a child +// builder whose returns never reached the fragment's returnEndIDs. The +// handler's end event is a return the script states, so it is part of the +// fragment exactly like a guard clause's (#888): a new end event of the flow. +func TestCutFragment_HandlerReturnIsANewEndEvent(t *testing.T) { + body := []ast.MicroflowStatement{ + &ast.LogStmt{ + Level: ast.LogInfo, + Message: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "risky"}, + ErrorHandling: &ast.ErrorHandlingClause{ + Type: ast.ErrorHandlingCustom, + Body: []ast.MicroflowStatement{ + &ast.LogStmt{Level: ast.LogError, Message: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "failed"}}, + &ast.ReturnStmt{}, + }, + }, + }, + &ast.LogStmt{Level: ast.LogInfo, Message: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "done"}}, + } + fb := &flowBuilder{posX: 200, posY: 200, baseY: 200, spacing: HorizontalSpacing} + oc := fb.buildFlowGraph(body, nil) + if errs := fb.GetErrors(); len(errs) > 0 { + t.Fatalf("build: %v", errs) + } + if fb.endsWithReturn { + t.Fatal("a return inside an error handler does not end the fragment's main path") + } + frag, err := cutFragment(oc, fb.fallThroughEndID, fb.returnEndIDs) + if err != nil { + t.Fatalf("the handler's return was refused: %v", err) + } + var ends int + for _, obj := range frag.Objects { + if _, ok := obj.(*microflows.EndEvent); ok { + ends++ + } + } + if ends != 1 { + t.Errorf("%d end events in the fragment, want the handler's return alone", ends) + } + var errFlow bool + for _, f := range frag.Flows { + if f.IsErrorHandler { + errFlow = true + } + } + if !errFlow { + t.Error("the fragment lost its error-handler flow") + } + if frag.Exit == "" { + t.Error("the fragment does not lead on to the rest of the flow") + } +} diff --git a/mdl/executor/cmd_microflows_builder_flows.go b/mdl/executor/cmd_microflows_builder_flows.go index 8a49574f20..84e8e04589 100644 --- a/mdl/executor/cmd_microflows_builder_flows.go +++ b/mdl/executor/cmd_microflows_builder_flows.go @@ -815,6 +815,17 @@ func (fb *flowBuilder) addErrorHandlerFlow(sourceActivityID model.ID, sourceX in if fb.annotationsByLabel == nil { fb.annotationsByLabel = errBuilder.annotationsByLabel } + // A `return` in the handler drew its end event into this collection too, + // so it is one of this builder's returns: the splice keeps it as a new end + // event of the flow rather than refusing it as an end the builder added + // (ako/mxcli#905). lastReturnEndID is left alone — it is the fallback + // target of the main path's own handlers. + for id := range errBuilder.returnEndIDs { + if fb.returnEndIDs == nil { + fb.returnEndIDs = map[model.ID]bool{} + } + fb.returnEndIDs[id] = true + } // If the error handler ends with RAISE ERROR or RETURN, it terminates there. // Otherwise, return the last activity ID so caller can create a merge. diff --git a/mdl/roundtrip/flow_splice_handler_return_test.go b/mdl/roundtrip/flow_splice_handler_return_test.go new file mode 100644 index 0000000000..8a090b57e4 --- /dev/null +++ b/mdl/roundtrip/flow_splice_handler_return_test.go @@ -0,0 +1,242 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build integration + +package roundtrip + +import ( + "strings" + "testing" +) + +// ako/mxcli#905 (#897 item 4, rehearsal 3 class G2): under mdl 1, `create or +// modify` refused to splice a new activity whose custom error handler ends in +// its own `return` — "an error handler in the fragment ends at an end event of +// its own … a return inside an error handler is not spliced yet" — on every +// run. Created fresh, the same statement worked. On CapTrack this made a +// stub-then-real script set leave the export flow as its placeholder. +// +// The handler's return is a new end event of the flow, drawn where the builder +// drew it relative to the fragment, like a guard clause's (#888). Named +// TestSpliceRerun_ so that it runs in the parity suite (#870). + +const handlerHelper = `create or modify microflow MyFirstModule.EH_Helper () returns Boolean as $Ok +begin + return true; +end; +` + +// An mxcli-authored stub grows by an activity whose handler returns, under +// both language versions — once as a replace of the stub's activity, once as an +// insert before a kept one — and the second run of the same script writes +// nothing. +func TestSpliceRerun_GrowByHandlerReturn(t *testing.T) { + h := newHarness(t) + defer h.close() + if err := h.exec("mdl 1;\n" + handlerHelper); err != nil { + t.Fatalf("helper: %v\n%s", err, h.out.String()) + } + + for _, c := range []struct { + name, header, stub, grown, spliced string + ends int + }{ + {"replace, mdl 1", "mdl 1;\n", + `create or modify microflow MyFirstModule.EH_Replace1 () +begin + log info node 'EH' 'stub'; +end; +`, `create or modify microflow MyFirstModule.EH_Replace1 () +begin + $Ok = call microflow MyFirstModule.EH_Helper() on error begin + log error node 'EH' 'failed'; + return; + end error; + log info node 'EH' 'done'; +end; +`, "(spliced: 1 replaced)", 2}, + {"insert, mdl 1, a returned value", "mdl 1;\n", + `create or modify microflow MyFirstModule.EH_Insert1 () returns Boolean as $R +begin + log info node 'EH' 'done'; + return true; +end; +`, `create or modify microflow MyFirstModule.EH_Insert1 () returns Boolean as $R +begin + $Ok = call microflow MyFirstModule.EH_Helper() on error without rollback begin + log error node 'EH' 'failed for ' + $currentUser/Name; + return false; + end error; + log info node 'EH' 'done'; + return true; +end; +`, "(spliced: 1 inserted)", 2}, + {"insert, mdl 0", "", + `create or modify microflow MyFirstModule.EH_Insert0 () +begin + log info node 'EH' 'done'; +end; +`, `create or modify microflow MyFirstModule.EH_Insert0 () +begin + $Ok = call microflow MyFirstModule.EH_Helper() on error begin + log error node 'EH' 'failed'; + return; + end error; + log info node 'EH' 'done'; +end; +`, "(spliced: 1 inserted)", 2}, + } { + t.Run(c.name, func(t *testing.T) { + // verdictsOf restores the fixture after a write, so the helper + // and the stub are stated again after it. + stored := func() { + if err := h.exec("mdl 1;\n" + handlerHelper + c.stub); err != nil { + t.Fatalf("stub: %v\n%s", err, h.out.String()) + } + } + stored() + // check -p agrees with exec (#892): no refusal, no rebuild. + if v := h.verdictsOf(c.header + handlerHelper + c.grown); agree(t, c.header != "", v) || v.checkError || v.checkWarning { + t.Errorf("check/diff/exec predict a refusal or a rebuild: %+v", v) + } + stored() + name := strings.Fields(strings.SplitN(c.stub, ".", 2)[1])[0] + stub := h.flowUnit(t, name) + + if err := h.exec(c.header + c.grown); err != nil { + t.Fatalf("grow: %v\n%s", err, h.out.String()) + } + if !strings.Contains(h.out.String(), c.spliced) { + t.Errorf("want %s, got:\n%s", c.spliced, h.out.String()) + } + grown := h.flowUnit(t, name) + if strings.Contains(c.spliced, "inserted") { + requireKept(t, stub, grown, "") + } + if got := countType(t, grown, "Microflows$EndEvent"); got != c.ends { + t.Errorf("%d end events after the grow, want %d", got, c.ends) + } + described := h.mustDescribeMdl0(t, "microflow MyFirstModule."+name) + for _, want := range []string{"on error", "log error node 'EH'", "return", "'EH' 'done'"} { + if !strings.Contains(described, want) { + t.Errorf("the grown flow does not state %q:\n%s", want, described) + } + } + + // The twice-exec rule. + settled := h.snapshot() + if err := h.exec(c.header + c.grown); err != nil { + t.Fatalf("second run: %v\n%s", err, h.out.String()) + } + if changed := settled.diff(h.snapshot()); len(changed) != 0 { + t.Errorf("the second run wrote: %s\n%s", strings.Join(changed, "; "), h.out.String()) + } + // Control: a change after the new activity is a change, and is + // written. (A change inside a stored handler is a replace of + // its activity, which the splice does not make.) + changed := strings.Replace(c.grown, "'EH' 'done'", "'EH' 'finished'", 1) + if err := h.exec(c.header + changed); err != nil { + t.Fatalf("changed flow: %v\n%s", err, h.out.String()) + } + if len(settled.diff(h.snapshot())) == 0 { + t.Errorf("a changed log message wrote nothing:\n%s", h.out.String()) + } + }) + } +} + +// A Studio Pro-authored flow grows by an activity whose handler returns: every +// stored element stays, the handler's return is a new end event, and the +// second run — and the grown flow's own description — writes nothing. The +// handler's message is an expression, which is stored as '{1}' with the +// expression as its parameter: the script's spelling and describe's are one +// activity, or the re-run would see the activity changed and refuse it. +func TestSpliceRerun_GrowStudioProFlowByHandlerReturn(t *testing.T) { + h := newHarness(t) + defer h.close() + if err := h.exec("mdl 1;\n" + handlerHelper); err != nil { + t.Fatalf("helper: %v\n%s", err, h.out.String()) + } + + const target = "microflow Administration.ShowPasswordForm" + described := withoutLayout(h.mustDescribeMdl0(t, target)) + const show = " show page Administration.ChangePasswordForm(" + const call = " $Ok = call microflow MyFirstModule.EH_Helper() on error begin\n" + + " log error node 'Pwd' 'no password form for ' + $Account/FullName;\n" + + " return;\n" + + " end error;\n" + edited := strings.Replace(described, show, call+show, 1) + if edited == described { + t.Fatalf("describe output changed shape:\n%s", described) + } + if v := h.verdictsOf("mdl 1;\n" + handlerHelper + edited); agree(t, true, v) || v.checkError { + t.Errorf("check/diff/exec predict a refusal: %+v", v) + } + if err := h.exec("mdl 1;\n" + handlerHelper); err != nil { + t.Fatalf("helper: %v\n%s", err, h.out.String()) + } + before := h.flowUnit(t, "ShowPasswordForm") + if err := h.exec("mdl 1;\n" + edited); err != nil { + t.Fatalf("grow: %v\n%s", err, h.out.String()) + } + if !strings.Contains(h.out.String(), "(spliced: 1 inserted)") { + t.Errorf("want one insert, got:\n%s", h.out.String()) + } + after := h.flowUnit(t, "ShowPasswordForm") + requireKept(t, before, after, "") + if got := countType(t, after, "Microflows$EndEvent"); got != 2 { + t.Errorf("%d end events after the grow, want 2", got) + } + + settled := h.snapshot() + if err := h.exec("mdl 1;\n" + edited); err != nil { + t.Fatalf("second run: %v\n%s", err, h.out.String()) + } + if changed := settled.diff(h.snapshot()); len(changed) != 0 { + t.Errorf("the second run wrote: %s\n%s", strings.Join(changed, "; "), h.out.String()) + } + again := h.mustDescribeMdl0(t, target) + if err := h.exec("mdl 1;\n" + again); err != nil { + t.Fatalf("re-exec of the description: %v\n%s", err, h.out.String()) + } + if changed := settled.diff(h.snapshot()); len(changed) != 0 { + t.Errorf("the description of the grown flow wrote: %s", strings.Join(changed, "; ")) + } +} + +// Where the handler's return would be drawn across a stored flow there is no +// free room, and the grow is refused with that reason — by check as by exec — +// and writes nothing: the insert stretches the stored handler of the activity +// before it across the gap the new handler goes in. +func TestSpliceRerun_HandlerReturnWithNoRoomIsRefused(t *testing.T) { + h := newHarness(t) + defer h.close() + if err := h.exec("mdl 1;\n" + handlerHelper); err != nil { + t.Fatalf("helper: %v\n%s", err, h.out.String()) + } + const flow = `create or modify microflow MyFirstModule.EH_Room () +begin + log info node 'R' 'a' on error begin + log error node 'R' 'x'; + log error node 'R' 'y'; + return; + end error; + log info node 'R' 'b'; +end; +` + if err := h.exec("mdl 1;\n" + flow); err != nil { + t.Fatalf("create: %v\n%s", err, h.out.String()) + } + grown := strings.Replace(flow, " log info node 'R' 'b';\n", + " $Ok = call microflow MyFirstModule.EH_Helper() on error begin\n log error node 'R' 'failed';\n return;\n end error;\n log info node 'R' 'b';\n", 1) + v := h.verdictsOf("mdl 1;\n" + handlerHelper + grown) + if !agree(t, true, v) || !v.checkError { + t.Fatalf("want the grow refused by check, diff and exec: %+v", v) + } + if !strings.Contains(v.execMessage, "no free room for the return") { + t.Errorf("the refusal does not say there is no room:\n%s", v.execMessage) + } + if strings.Contains(v.execMessage, "not spliced yet") { + t.Errorf("the refusal still says a handler's return is not spliced:\n%s", v.execMessage) + } +}