Merge pull request #220 from yscraft/docs/mxcli-dev-contributor-commands

docs: add mxcli-dev contributor command namespace and /mxcli-dev:review

Author: ako

.claude/commands/mxcli-dev/review.md

@@ -0,0 +1,42 @@
+# /mxcli-dev:review — PR Review
+
+Run a structured review of the current branch's changes against the CLAUDE.md
+checklist, then check the recurring findings table below for patterns that have
+burned us before.
+
+## Steps
+
+1. Run `gh pr view` and `gh pr diff` (or `git diff main...HEAD`) to read the change.
+2. Work through the CLAUDE.md "PR / Commit Review Checklist" in full.
+3. Then check every row in the Recurring Findings table below — flag any match.
+4. Report: blockers first, then moderate issues, then minor. Include a concrete fix
+   option for every blocker (not just "this is wrong").
+5. After the review: **add a row** to the Recurring Findings table for any new
+   pattern not already covered.
+
+---
+
+## Recurring Findings
+
+Patterns caught in real reviews. Each row is a class of mistake worth checking
+proactively. Add a row after every review that surfaces something new.
+
+| # | Finding | Category | Canonical fix |
+|---|---------|----------|---------------|
+| 1 | Formatter emits a keyword not present in `MDLParser.g4` → DESCRIBE output won't re-parse (e.g. `RANGE(...)`) | DESCRIBE roundtrip | Grep grammar before assuming a keyword is valid; if construct can't be expressed yet, emit `-- TypeName(field=value) — not yet expressible in MDL` |
+| 2 | Output uses `$currentObject/Attr` prefix — non-idiomatic; Studio Pro uses bare attribute names | Idiomatic output | Verify against a real Studio Pro BSON sample before choosing a prefix convention |
+| 3 | Malformed BSON field (missing key, wrong type) produces silent garbage output (e.g. `RANGE($x, , )`) | Error handling | Default missing numeric fields to `"0"`; or emit `-- malformed <TypeName>` rather than broken MDL |
+| 4 | No DESCRIBE roundtrip test — grammar gap went undetected until human review | Test coverage | Add roundtrip test: format struct → MDL string → parse → confirm no error |
+| 5 | Hardcoded personal path in committed file (e.g. `/c/Users/Ylber.Sadiku/...`) | Docs quality | Use bare commands (`go test ./...`) without absolute paths in any committed doc or skill |
+| 6 | Docs-only PR cites an unmerged PR as a "model example" — cited PR had blockers | Docs quality | Only cite merged, verified PRs; or annotate with known gaps if citing in-flight work |
+| 7 | Skill/doc table references a function that doesn't exist (e.g. `formatActionStatement()` vs `formatAction()`) | Docs quality | Grep function names before writing: `grep -r "func formatA" mdl/executor/` |
+| 8 | "Always X" rule is too absolute for trivial edge cases (e.g. "always write failing test first" for one-char typos) | Docs quality | Soften to "prefer X" or add an exception clause; include the reasoning so readers can judge edge cases |
+
+---
+
+## After Every Review
+
+- [ ] All blockers have a concrete fix option stated.
+- [ ] Recurring Findings table updated with any new pattern.
+- [ ] If docs-only PR: every function name, path, and PR reference verified against
+      live code before approving.

.claude/skills/fix-issue.md

@@ -0,0 +1,77 @@
+# Fix Issue Skill
+
+A fast-path workflow for diagnosing and fixing bugs in mxcli. Each fix appends
+to the symptom table below, so the next similar issue costs fewer reads.
+
+## How to Use
+
+1. Match the issue symptom to a row in the table — go straight to that file.
+2. Follow the fix pattern for that row.
+3. Write a failing test first, then implement.
+4. After the fix: **add a new row** to the table if the symptom is not already covered.
+
+---
+
+## Symptom → Layer → File Table
+
+| Symptom | Root cause layer | First file to open | Fix pattern |
+|---------|-----------------|-------------------|-------------|
+| `DESCRIBE` shows `$var = LIST OPERATION ...;` | Missing parser case | `sdk/mpr/parser_microflow.go` → `parseListOperation()` | Add `case "Microflows$XxxType":` returning the correct struct |
+| `DESCRIBE` shows `$var = ACTION ...;` | Missing formatter case | `mdl/executor/cmd_microflows_format_action.go` → `formatActionStatement()` | Add `case *microflows.XxxAction:` with `fmt.Sprintf` output |
+| `DESCRIBE` shows `$var = LIST OPERATION %T;` (with type name) | Missing formatter case | `mdl/executor/cmd_microflows_format_action.go` → `formatListOperation()` | Add `case *microflows.XxxOperation:` before the `default` |
+| Compile error: `undefined: microflows.XxxOperation` | Missing SDK struct | `sdk/microflows/microflows_actions.go` | Add struct + `func (XxxOperation) isListOperation() {}` marker |
+| `TypeCacheUnknownTypeException` in Studio Pro | Wrong `$Type` storage name in BSON write | `sdk/mpr/writer_microflow.go` | Check the storage name table in CLAUDE.md; verify against `reference/mendixmodellib/reflection-data/` |
+| CE0066 "Entity access is out of date" | MemberAccess added to wrong entity | `sdk/mpr/writer_domainmodel.go` | MemberAccess must only be on the FROM entity (`ParentPointer`), not the TO entity — see CLAUDE.md association semantics |
+| CE0463 "widget definition changed" | Object property structure doesn't match Type PropertyTypes | `sdk/widgets/templates/` | Re-extract template from Studio Pro; see `sdk/widgets/templates/README.md` |
+| Parser returns `nil` for a known BSON type | Unhandled `default` in a `parseXxx()` switch | `sdk/mpr/parser_microflow.go` or `parser_page.go` | Find the switch by grepping for `default: return nil`; add the missing case |
+| MDL check gives "unexpected token" on valid-looking syntax | Grammar missing rule or token | `mdl/grammar/MDLParser.g4` + `MDLLexer.g4` | Add rule/token, run `make grammar` |
+| CE7054 "parameters updated" / CE7067 "does not support body entity" after `SEND REST REQUEST` | `addSendRestRequestAction` emitted wrong BSON: all params as query params, BodyVariable set for JSON bodies | `mdl/executor/cmd_microflows_builder_calls.go` → `addSendRestRequestAction` | Look up operation via `fb.restServices`; route path/query params with `buildRestParameterMappings`; suppress BodyVariable for JSON/TEMPLATE/FILE via `shouldSetBodyVariable` |
+
+---
+
+## TDD Protocol
+
+Always follow this order — never implement before the test exists:
+
+```
+Step 1: Write a failing unit test (parser test or formatter test)
+Step 2: Confirm it fails to compile or fails at runtime
+Step 3: Implement the minimum code to make it pass
+Step 4: Run: /c/Users/Ylber.Sadiku/go/go/bin/go test ./mdl/executor/... ./sdk/mpr/...
+Step 5: Add the symptom row to the table above if not already present
+```
+
+Parser tests go in `sdk/mpr/parser_<domain>_test.go`.
+Formatter tests go in `mdl/executor/cmd_<domain>_format_<area>_test.go`.
+
+---
+
+## Issue #212 — Reference Fix (seeding example)
+
+**Symptom:** `DESCRIBE MICROFLOW` showed `$var = LIST OPERATION ...;` for
+`Microflows$Find`, `Microflows$Filter`, `Microflows$ListRange`.
+
+**Root cause:** `parseListOperation()` in `sdk/mpr/parser_microflow.go` had no
+cases for these three BSON types — they fell to `default: return nil`.
+
+**Files changed:**
+| File | Change |
+|------|--------|
+| `sdk/microflows/microflows_actions.go` | Added `FindByAttributeOperation`, `FilterByAttributeOperation`, `RangeOperation` |
+| `sdk/mpr/parser_microflow.go` | Added 3 parser cases |
+| `mdl/executor/cmd_microflows_format_action.go` | Added 3 formatter cases |
+| `mdl/executor/cmd_microflows_format_listop_test.go` | Added 4 formatter tests |
+| `sdk/mpr/parser_listoperation_test.go` | New file, 4 parser tests |
+
+**Key insight:** `Microflows$ListRange` stores offset/limit inside a nested
+`CustomRange` map — must cast `raw["CustomRange"].(map[string]any)` before
+extracting `OffsetExpression`/`LimitExpression`.
+
+---
+
+## After Every Fix — Checklist
+
+- [ ] Failing test written before implementation
+- [ ] `go test ./mdl/executor/... ./sdk/mpr/...` passes
+- [ ] New symptom row added to the table above (if not already covered)
+- [ ] PR title: `fix: <one-line description matching the symptom>`

CLAUDE.md

@@ -224,6 +224,11 @@ Available namespaces: `DomainModels`, `Enumerations`, `Microflows`, `Pages`, `Mo
 
 When reviewing pull requests or validating work before commit, verify these items:
 
+### Bug fixes
+- [ ] **Fix-issue skill consulted** — read `.claude/skills/fix-issue.md` before diagnosing; match symptom to table before opening files
+- [ ] **Symptom table updated** — new symptom/layer/file mapping added to `.claude/skills/fix-issue.md` if not already covered
+- [ ] **Test written first** — failing test exists before implementation (parser test in `sdk/mpr/`, formatter test in `mdl/executor/`)
+
 ### Overlap & duplication
 - [ ] Check `docs/11-proposals/` for existing proposals covering the same functionality
 - [ ] Search the codebase for existing implementations (grep for key function names, command names, types)
@@ -356,11 +361,22 @@ mxcli new MyApp --version 10.24.0 --output-dir ./projects/my-app
 
 Steps performed: downloads MxBuild → `mx create-project` → `mxcli init` → downloads correct Linux mxcli binary for devcontainer. The result is a ready-to-open project with `.devcontainer/`, AI tooling, and a working `./mxcli` binary.
 
+### Slash Command Namespaces
+
+Commands in `.claude/commands/` are organised by audience:
+
+| Namespace | Folder | Invoked as | Purpose |
+|-----------|--------|------------|---------|
+| `mendix:` | `.claude/commands/mendix/` | `/mendix:lint` | mxcli **user** commands — synced to Mendix projects via `mxcli init` |
+| `mxcli-dev:` | `.claude/commands/mxcli-dev/` | `/mxcli-dev:review` | **Contributor** commands — this repo only, never synced to user projects |
+
+Both namespaces are discoverable by typing `/mxcli` in Claude Code. Add new contributor tooling (review workflows, debugging helpers, etc.) under `mxcli-dev/`. Add commands intended for Mendix project users under `mendix/`.
+
 ### mxcli init
 
 `mxcli init` creates a `.claude/` folder with skills, commands, CLAUDE.md, and VS Code MDL extension in a target Mendix project. Source of truth for synced assets:
 - Skills: `reference/mendix-repl/templates/.claude/skills/`
-- Commands: `.claude/commands/mendix/`
+- Commands: `.claude/commands/mendix/` (the `mxcli-dev/` folder is **not** synced)
 - VS Code extension: `vscode-mdl/vscode-mdl-*.vsix`
 
 Build-time sync: `make build` syncs everything automatically. Individual targets: `make sync-skills`, `make sync-commands`, `make sync-vsix`.

mdl/executor/cmd_microflows_builder.go

@@ -33,6 +33,7 @@ type flowBuilder struct {
 	reader              *mpr.Reader              // For looking up page/microflow references
 	hierarchy           *ContainerHierarchy      // For resolving container IDs to module names
 	pendingAnnotations  *ast.ActivityAnnotations // Pending annotations to attach to next activity
+	restServices        []*model.ConsumedRestService // Cached REST services for parameter classification
 }
 
 // addError records a validation error during flow building.

mdl/executor/cmd_microflows_builder_calls.go

@@ -769,6 +769,16 @@ func (fb *flowBuilder) addSendRestRequestAction(s *ast.SendRestRequestStmt) mode
 	// Build operation reference: Module.Service.Operation
 	operationQN := s.Operation.String()
 
+	// Look up the operation definition to classify parameters and body kind.
+	// s.Operation.Module = "MfTest", s.Operation.Name = "RC_TestApi.PostJsonTemplate"
+	var opDef *model.RestClientOperation
+	if fb.restServices != nil && s.Operation.Module != "" && strings.Contains(s.Operation.Name, ".") {
+		dotIdx := strings.Index(s.Operation.Name, ".")
+		serviceName := s.Operation.Name[:dotIdx]
+		opName := s.Operation.Name[dotIdx+1:]
+		opDef = lookupRestOperation(fb.restServices, serviceName, opName)
+	}
+
 	// Build OutputVariable
 	var outputVar *microflows.RestOutputVar
 	if s.OutputVariable != "" {
@@ -778,29 +788,20 @@ func (fb *flowBuilder) addSendRestRequestAction(s *ast.SendRestRequestStmt) mode
 		}
 	}
 
-	// Build BodyVariable
+	// Build BodyVariable only for EXPORT_MAPPING body kind.
+	// For JSON / TEMPLATE / FILE bodies, the body expression lives on the
+	// operation definition itself and must NOT be set here (CE7067).
 	var bodyVar *microflows.RestBodyVar
-	if s.BodyVariable != "" {
+	if s.BodyVariable != "" && shouldSetBodyVariable(opDef) {
 		bodyVar = &microflows.RestBodyVar{
 			BaseElement:  model.BaseElement{ID: model.ID(mpr.GenerateID())},
 			VariableName: s.BodyVariable,
 		}
 	}
 
-	// Build parameter mappings from WITH clause
-	var paramMappings []*microflows.RestParameterMapping
-	var queryParamMappings []*microflows.RestQueryParameterMapping
-	for _, p := range s.Parameters {
-		// Determine if path or query param by convention:
-		// the executor can't distinguish at this level, so we emit both
-		// and let the BSON field names sort it out. For now, emit as
-		// query parameter mappings (most common use case).
-		queryParamMappings = append(queryParamMappings, &microflows.RestQueryParameterMapping{
-			Parameter: operationQN + "." + p.Name,
-			Value:     p.Expression,
-			Included:  "Yes",
-		})
-	}
+	// Build parameter mappings, routing to ParameterMappings (path) or
+	// QueryParameterMappings (query) based on the operation definition.
+	paramMappings, queryParamMappings := buildRestParameterMappings(s.Parameters, opDef, operationQN)
 
 	// RestOperationCallAction does not support custom error handling (CE6035).
 	// ON ERROR clauses in the MDL are silently ignored for this action type.
@@ -831,6 +832,84 @@ func (fb *flowBuilder) addSendRestRequestAction(s *ast.SendRestRequestStmt) mode
 	return activity.ID
 }
 
+// lookupRestOperation finds a specific operation in a consumed REST service list.
+func lookupRestOperation(services []*model.ConsumedRestService, serviceName, opName string) *model.RestClientOperation {
+	for _, svc := range services {
+		if svc.Name != serviceName {
+			continue
+		}
+		for _, op := range svc.Operations {
+			if op.Name == opName {
+				return op
+			}
+		}
+	}
+	return nil
+}
+
+// shouldSetBodyVariable returns true if a BodyVariable BSON field should be
+// emitted for a call to the given operation.
+// For JSON, TEMPLATE, and FILE body kinds, the body expression lives on the
+// operation definition and must not be overridden by a BodyVariable (CE7067).
+// For EXPORT_MAPPING, the caller provides an entity to export via BodyVariable.
+// When the operation definition is unknown (nil), we preserve old behaviour and
+// set BodyVariable so the caller's intent is not silently dropped.
+func shouldSetBodyVariable(op *model.RestClientOperation) bool {
+	if op == nil {
+		return true // unknown operation — preserve caller intent
+	}
+	switch op.BodyType {
+	case "JSON", "TEMPLATE", "FILE":
+		return false
+	default:
+		// EXPORT_MAPPING or empty (no body) — only set if EXPORT_MAPPING
+		return op.BodyType == "EXPORT_MAPPING"
+	}
+}
+
+// buildRestParameterMappings splits parameter bindings from a SEND REST REQUEST
+// WITH clause into path parameter mappings and query parameter mappings,
+// using the operation definition to determine which is which.
+// When op is nil (operation not found), all parameters fall back to query
+// parameter mappings (preserves old behaviour).
+func buildRestParameterMappings(
+	params []ast.SendRestParamDef,
+	op *model.RestClientOperation,
+	operationQN string,
+) ([]*microflows.RestParameterMapping, []*microflows.RestQueryParameterMapping) {
+	if len(params) == 0 {
+		return nil, nil
+	}
+
+	// Build lookup sets from the operation definition.
+	pathParamSet := map[string]bool{}
+	if op != nil {
+		for _, p := range op.Parameters {
+			pathParamSet[p.Name] = true
+		}
+	}
+
+	var pathMappings []*microflows.RestParameterMapping
+	var queryMappings []*microflows.RestQueryParameterMapping
+
+	for _, p := range params {
+		if pathParamSet[p.Name] {
+			pathMappings = append(pathMappings, &microflows.RestParameterMapping{
+				Parameter: operationQN + "." + p.Name,
+				Value:     p.Expression,
+			})
+		} else {
+			queryMappings = append(queryMappings, &microflows.RestQueryParameterMapping{
+				Parameter: operationQN + "." + p.Name,
+				Value:     p.Expression,
+				Included:  "Yes",
+			})
+		}
+	}
+
+	return pathMappings, queryMappings
+}
+
 // addExecuteDatabaseQueryAction creates an EXECUTE DATABASE QUERY statement.
 func (fb *flowBuilder) addExecuteDatabaseQueryAction(s *ast.ExecuteDatabaseQueryStmt) model.ID {
 	// DynamicQuery is a Mendix expression — string literals need single quotes

mdl/executor/cmd_microflows_builder_control.go

@@ -287,6 +287,7 @@ func (fb *flowBuilder) addLoopStatement(s *ast.LoopStmt) model.ID {
 		measurer:     fb.measurer,     // Share measurer
 		reader:       fb.reader,       // Share reader
 		hierarchy:    fb.hierarchy,    // Share hierarchy
+		restServices: fb.restServices, // Share REST services for parameter classification
 	}
 
 	// Process loop body statements and connect them with flows
@@ -360,6 +361,7 @@ func (fb *flowBuilder) addWhileStatement(s *ast.WhileStmt) model.ID {
 		measurer:     fb.measurer,
 		reader:       fb.reader,
 		hierarchy:    fb.hierarchy,
+		restServices: fb.restServices,
 	}
 
 	var lastBodyID model.ID

mdl/executor/cmd_microflows_builder_flows.go

@@ -66,6 +66,7 @@ func (fb *flowBuilder) addErrorHandlerFlow(sourceActivityID model.ID, sourceX in
 		measurer:     fb.measurer,
 		reader:       fb.reader,
 		hierarchy:    fb.hierarchy,
+		restServices: fb.restServices,
 	}
 
 	var lastErrID model.ID