Sync ako/mxcli: bundled skills adopt the Agent Skills standard, plus microflow-layout and OData fixes

.claude/skills/fix-issue.md

@@ -608,3 +608,7 @@ extracting `OffsetExpression`/`LimitExpression`.
 | `DELETE_BEHAVIOR PREVENT` (or `DELETE_IF_NO_REFERENCES`, or `DELETE_AND_REFERENCES`) reports `Modified association` and stores `DeleteMeButKeepReferences`, destroying whatever the association had — and `DESCRIBE ASSOCIATION` emits `delete_behavior DELETE_CASCADE`, which the parser then rejects | `buildDeleteBehavior` read only `db.CASCADE()`; the other four of the rule's five tokens fell through to the zero value `ast.DeleteKeepReferences`. A **legal** behaviour, so nothing below could tell it had been substituted. Underneath it, `ALTER ASSOCIATION SET` built the stored value as `DeleteBehaviorType(s.DeleteBehavior.String())`, and `String()` spells prevent `"DeleteIfNoReferences"` where Mendix writes `"DeleteMeIfNoReferences"` — so fixing the visitor alone put an **out-of-domain enum** on disk | `mdl/visitor/visitor_helpers.go` (`buildDeleteBehavior`), then `mdl/executor/cmd_associations.go` (`storageDeleteBehavior`) | Read **every** token the grammar rule admits — a `default:` arm returning a plausible value turns a parse gap into silent data loss. Route all storage conversions through one function; a `String()` returning storage-ish names is a trap, not an encoder. Emit from DESCRIBE only spellings the lexer has (`DELETE_AND_REFERENCES`, not `DELETE_CASCADE`) and prove it by feeding the output back through `visitor.Build`. **`mx check` is not the oracle here**: measured on mxbuild 11.6.6, a project carrying `DeleteIfNoReferences` builds with 0 errors exactly like a correct one — only Studio Pro refuses it, so assert the stored value is one of Mendix's three, not merely the one you asked for (upstream #901) |
 | `CREATE OR REPLACE WORKFLOW` silently deletes a boundary event (timer + its whole handler flow) or an event sub-process that the script does not restate — `exec` reports "Created workflow" and exit 0, and `mx check` afterwards reports 0 errors because the result is a valid workflow that simply no longer does what it did. `DESCRIBE WORKFLOW` does not show the construct either, so nothing reveals the loss | Two separate defects. (1) The **default (modelsdk) engine's workflow reader had no boundary-event support at all** — `grep BoundaryEvent mdl/backend/modelsdk/workflow_read.go` → 0, while `sdk/mpr/parser_workflow.go` → 17. Both engines *write* them, so a boundary event mxcli itself had just written read back as absent. (2) A rewrite rebuilds from the statement, so anything unstated is dropped, and no guard existed — unlike the queued-call and validation-rule paths | `mdl/backend/modelsdk/workflow_read.go` (`boundaryEventsFromGen`, wired into the 6 gen types with `BoundaryEventsItems`); `mdl/executor/validate_workflow_rewrite.go` (`checkNoDroppedWorkflowConstructs`, called from `cmd_workflows_write.go`); `mdl/executor/cmd_workflows.go` (clause order) | **Read the stored side from the RAW unit, not the semantic model** — the reader is what was blind, and a guard sharing its blind spot cannot see what it protects; `GetRawUnit` also covers constructs no engine models yet (event sub-processes exist in `modelsdk/gen` but have no semantic type, reader or writer anywhere). Match the `$Type` by **substring** so all three timer variants and any later one are caught. Authorable constructs get the queue treatment (restate them and the rewrite proceeds); unauthorable ones refuse outright. **Fixing a reader can expose a describer bug**: with boundary events finally readable, `describe → exec` stopped parsing, because the call-microflow describer emitted `boundary event` BEFORE `outcomes` and the grammar requires the reverse — pre-existing on legacy, invisible while the default engine emitted neither. Always re-run a describe→re-exec round trip after widening a reader. Issue #948 |
 | A workflow that calls a microflow existing nowhere passes `mxcli check --references` with "All references valid" and is written by `exec` with exit 0; only native `mx check` catches it (**CE1613** "The selected microflow ... no longer exists"). The identical mistake inside a plain microflow body IS caught | `validateWithContext`'s `CreateWorkflowStmt` case called only `validateWorkflowParameterMappings`, whose own comment defers a target that is not in the project to "the missing-reference check" — **which was never written**. `validateFlowBodyReferences` (which does catch it) is wired only to `CreateMicroflowStmt`/`CreateNanoflowStmt`. `AlterWorkflowStmt` had **no case at all** and fell to `default: skip validation` | `mdl/executor/validate_workflow_refs.go` (`validateWorkflowReferences`, `validateWorkflowStatementRefs`, `validateAlterWorkflowRefs`); `mdl/executor/validate.go` (both switch cases, `scriptContext.workflows`, `collectDefinitions`/`collectSingle`/`allNames`); `mdl/executor/helpers.go` (`buildWorkflowQualifiedNames`); exec-side guards in `cmd_workflows_write.go` + `cmd_alter_workflow.go` | **The gap is never one reference kind** — probe them all before fixing. Here *every* reference in a workflow was unvalidated: call microflow, call workflow, user task page, targeting microflow, context entity, and the workflow's own module (checked for a microflow, not for a workflow — and `exec` auto-creates a module, so `check` was the only guard against a typo silently making one). **`check` and `exec` run different passes**: `check --references` runs `validateProgram`, `exec` does not, so a validator wired only into the switch leaves exec writing the broken model — the guard must ALSO be called from the statement handler, before `findOrCreateModule` (same shape as #833). Pass `nil` for the scriptContext there: exec applies statements one at a time, so an earlier statement's output is already in the project. **The real risk is false positives** — verify by diffing error counts against a baseline binary over every workflow script in `mdl-examples/`, with the referenced modules actually present, and exempt `System.*` (`isBuiltinModuleEntity`) or every built-in target is reported missing. Issue #943 |
+| `CREATE OR MODIFY ODATA CLIENT` reports `Unchanged OData client` after the contract file was refreshed from the running backend — `SHOW CONTRACT ENTITIES` keeps listing the old entity types, and the `CREATE OR MODIFY EXTERNAL ENTITIES` that follows imports the old shape with no warning. `DROP ODATA CLIENT` + recreate is the only way out, and it invalidates the client ID the existing external entities point at | The modify branch updated every property **except the cached contract**. Only the create path fetched `$metadata`; `svc.Metadata` / `svc.MetadataHash` kept the snapshot taken when the client was first created. The same branch also stored `stmt.MetadataUrl` raw, skipping the `NormalizeURL` the create path applies, so a relative `./contracts/x.xml` ended up as a URL neither Studio Pro nor `fetchODataMetadata` can open | `mdl/executor/cmd_odata.go` (`refreshCachedMetadata`, `normalizeMetadataURL`, `contractSummary`, called from the `stmt.CreateOrModify` branch of `createODataClient`); test `mdl/executor/cmd_odata_metadata_refresh_test.go`; repro `mdl-examples/bug-tests/odata-client-stale-contract-on-modify.mdl` | **Refresh from the *effective* URL, not the statement's** — a `CREATE OR MODIFY` that omits `MetadataUrl` still has to converge the cache, or the idiomatic re-run silently keeps the stale contract. **A failed fetch must keep what is cached** (warning, not error): losing a contract that was merely unreachable is worse than serving a stale one, and there is nothing to fall back on. **Leave `svc` untouched when the hash matches**, or ADR-0008 elision breaks and every re-run churns the document — the unchanged-contract control is what proves this, and it passes both before and after the fix, so it is the half of the test that keeps the other half honest. `ALTER ODATA CLIENT SET MetadataUrl` has the same staleness and is deliberately **not** fixed here: it carries no design-time credentials, so a blind re-fetch of an authenticated URL would warn where it used to be silent. `mx check` is no oracle for any of this — the stale project builds with 0 errors, because a client caching an old contract is perfectly valid |
+| A microflow rewritten by `CREATE OR MODIFY MICROFLOW` is drawn with its **StartEvent stranded** — every activity where the script asked, the start left behind, joined to its own first activity by a long mostly-empty line across the canvas. Reproduces on every rewrite, and copying a working script's `@position` pattern does not help | The fix for the OPPOSITE report. #884 was a describe→exec round-trip MOVING a hand-placed start (145;200 → 100;200), fixed by carrying the stored position over on every rewrite; that then **pinned** the start of every rewritten flow, so it no longer followed activities the same script had just moved. Both reports are real and neither is answerable without asking where the stored value came from | `mdl/executor/cmd_microflows_start_position.go` (`authoredStartPosition`, `derivedStartPosition`, `startAnnotationLines`), `mdl/executor/cmd_microflows_create.go` (`storedStartPosition`), `mdl/executor/cmd_microflows_builder_graph.go` (StartEvent construction) | Carry the stored position over **only when it is not where the layout would have put it** — a start at `first.X − spacing` on `first.Y` is mxcli's own arithmetic handed back and carries no intent, so re-derive it; anywhere else, a person put it there, so keep it. Add `@start(x, y)` on the first statement (the `@merge` precedent — it positions the other node with no statement of its own) so the position can be *stated* rather than inferred, and have DESCRIBE emit it **only for a non-derived start**, which is what makes the round-trip exact without pinning every described flow. **`mx check` is not the oracle, and neither is a green build**: the Mendix model carries no geometry rules, so a stranded start is a valid document — 0 errors on mxbuild 11.13.0 before and after. Verify by reading the coordinates back off the stored document. **Emit from BOTH describers**: `formatMicroflowActivities` and `formatMicroflowActivitiesWithSourceMap` are near-duplicates, and the first cut of this fix patched only the second, so `describe microflow` silently dropped the line. Controls: reverting the narrowing reproduces the stranded start, reverting the builder arm reproduces "@start does nothing", reverting either describer reproduces the lossy round-trip. Repro `mdl-examples/bug-tests/951-microflow-start-event-position.mdl`. Issue #951 |
+| `SPLIT TYPE … END SPLIT` followed by any statement draws that statement **on top of the merge** in Studio Pro, joined by a zero-length sequence flow, and the merge itself sits far to the right of the branches it joins (branches at x=720, merge at x=1480). `CASE` and `IF/ELSE` with the same graph shape lay out correctly. `mx check` reports 0 errors either way | Two defects in `addStructuredInheritanceSplit`. (1) It ended with `fb.posX = mergeX` — the merge's own centre — where `addEnumSplit` steps to `mergeX + HorizontalSpacing/2` and `addIfStatement` to `mergeX + MergeSize + HorizontalSpacing/2`; three builders, three conventions, and the type split's was zero. (2) Branch width came from `measureStatements(appendInheritanceBodies(s))`, which concatenated every branch body into ONE list and measured it as a single left-to-right run — so the merge slid right by an activity-plus-spacing per **extra branch**, not per branch *width*. `layout.go`'s own `measureInheritanceSplitStatement` always took the max, so the builder disagreed with its measurer | `mdl/executor/cmd_microflows_builder_actions.go` (`addStructuredInheritanceSplit`; `appendInheritanceBodies` → `inheritanceBranchBodies`); tests `mdl/executor/cmd_microflows_builder_split_geometry_test.go`; repro `mdl-examples/bug-tests/953-split-type-merge-overlap.mdl` | **Geometry is invisible to every automatic check below Studio Pro** — the model is valid, so `mx check` passes, the build passes, and `describe` output looks right until you read the coordinates. `describe` printing `@position`/`@merge` IS the headless oracle: diff those, don't open the modeler. **The split builders had zero positional coverage** before this (grep `Position` in the enum-split and inheritance-split tests: nothing), which is the same gap behind the loop-box sizing bugs #790 and #884. **Pick the IF convention, not the CASE one**: the constants are centre-to-centre for a 40px edge gap, and `CASE`'s `+HorizontalSpacing/2` leaves a following activity's left edge exactly touching the merge (measured: merge 890, activity 970, both edges at 910) — a lesser pre-existing nit deliberately left alone, because changing it re-lays-out every enum split ever written. **`@merge(x,y)` was not a workaround**: `fb.posX = mergeX` read the overridden value, so moving the merge by hand moved the stacked element with it. A fix needs a control that the neighbouring construct did NOT move — `TestEnumSplitGeometryIsUnchanged` — or "the type split matches CASE now" can be achieved by breaking CASE. Issue #953 |
+| A bundled skill renders with a stray `---` fence and a duplicated `name:` at the top of its body, and `mxcli init` ships it that way. `TestEmbeddedSkillsCarryAgentSkillsFrontmatter` passes — the frontmatter it checks is valid | The #906 migration **prepended** a `name`/`description` block to every skill, including the one file that already had one (`custom-widgets`, under its pre-rename name `mendix-custom-widgets`). Everything after the FIRST `---` block is body, so the old block became visible text. The test's `frontmatter.FindSubmatch` matches the first block and validates it correctly, which is precisely why it is blind to a second one | `.claude/skills/mendix/custom-widgets/SKILL.md` (two blocks merged into one, keeping the old block's trigger words — child slots, `.def.json`, engine internals); guard added to `cmd/mxcli/init_skills_standard_test.go` | **A first-block check cannot see a rename-and-prepend**, and that is the exact shape of a migration that renames things — so a bulk frontmatter edit needs a guard against a SECOND block, not a better check of the first. **Measure the pre-state before a bulk prepend**: 1 of 68 files already had frontmatter, which was known and written down, and the migration script still did not branch on it. **Do not attribute a defect to the change you are looking at** — this was reported against PR #222, whose diff touches only the `description:` inside the first block and shows the stray `---` as unchanged context; `git log -S 'name: mendix-custom-widgets'` settles it in one command. Two adjacent traps from the same report: `make test` did not depend on `sync-all` while `make build` did, so a bare `go build` left the generated `cmd/mxcli/skills` stale and six tests failed — and the failure message named the `go:embed` directive, which was fine, instead of the missing build step. A **differential across refs is only as sound as the procedure both sides share**: the same stale build made main fail 4 and the branch fail 6, which looked like a regression and was a test-count difference. mxcli-formula1 finding 68 |