From 1d87679db6cccb1b4d79344934ce45f438f18df1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 13:25:58 +0000 Subject: [PATCH 1/2] fix(mcp): resolve a foldered document's module through its folder chain ALTER PAGE over the MCP backend failed for every page inside a folder: failed to open page for mutation: module not found: The uuid was the page's parent folder. resolvePageUnit passed page.ContainerID straight to GetModule, which only holds for a page at the module root. The backend already had the folder-aware answer, moduleNameForContainer; the page mutator now uses it, for saved pages and for pages created this session. The same direct lookup sat in UpdateConstant and UpdateWorkflow (CREATE OR MODIFY on a foldered constant or workflow failed the same way) and in forgetSessionConstant / forgetSessionEnum (DROP of a foldered document left it registered). Those now resolve through the same helper. Control: with the production change stashed, page_mutator_folder_test.go fails with "OpenPageForMutation: module not found: fPages" plus the three sibling failures; the module-root case passes either way. Fixes mendixlabs/mxcli#1593 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016ZCjbSMETkeDrr8bHi6LCD --- ...alter-page-in-folder-module-not-found.json | 1 + .../bug-patterns/duplicate-resolver-drift.md | 7 +- .../1593-mcp-alter-page-in-folder.mdl | 51 ++++ mdl/backend/mcp/constant.go | 8 +- mdl/backend/mcp/enumeration.go | 2 +- mdl/backend/mcp/page_mutator.go | 31 +-- mdl/backend/mcp/page_mutator_folder_test.go | 218 ++++++++++++++++++ mdl/backend/mcp/workflow.go | 6 +- 8 files changed, 301 insertions(+), 23 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-backend/2026-10-20-mcp-alter-page-in-folder-module-not-found.json create mode 100644 mdl-examples/bug-tests/1593-mcp-alter-page-in-folder.mdl create mode 100644 mdl/backend/mcp/page_mutator_folder_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-10-20-mcp-alter-page-in-folder-module-not-found.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-20-mcp-alter-page-in-folder-module-not-found.json new file mode 100644 index 0000000000..1618ab3d66 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-20-mcp-alter-page-in-folder-module-not-found.json @@ -0,0 +1 @@ +{"area": "mdl-backend", "date": "2026-10-20", "symptom": "`alter page` over the MCP backend (Studio Pro 11.12.5) fails for every page inside a folder with `failed to open page for mutation: module not found: ` — the uuid is the page's parent FOLDER — while the same statement on a module-root page succeeds; `check --references` passes and the trace shows no PED call before the error", "cause": "`resolvePageUnit` (MCP page mutator) resolved the module with `GetModule(page.ContainerID)`, and a page's ContainerID is its immediate parent, a folder for most Studio Pro pages. The backend already had the folder-aware answer, `moduleNameForContainer` (walks `resolveDocContainer` up the folder chain), used by CreatePage, DeleteConstant, the microflow mutator and others. `UpdateConstant` and `UpdateWorkflow` (CREATE OR MODIFY on a foldered document) and `forgetSessionConstant` / `forgetSessionEnum` (DROP cleanup; silently left the entry registered) had the same direct lookup", "file": "`mdl/backend/mcp/page_mutator.go` (`resolvePageUnit`), `mdl/backend/mcp/constant.go`, `mdl/backend/mcp/workflow.go`, `mdl/backend/mcp/enumeration.go`", "insight": "When an error says ` not found: ` and the id is NOT an id of that kind, it is the caller passing the wrong id — map the id to what it actually is (here a folder) before reading any resolver. For containers: `GetModule(x.ContainerID)` is correct only for a domain model or a folder's own walk; for any other document grep the package for that exact pattern — a folder-aware helper usually already exists and the bug is the call sites that bypass it (this is the MCP-backend recurrence of #976/#1071 in the executor). The modelsdk engine never had it, so running the bug-test MDL without `--mcp` proves nothing about the fix; the evidence is `mdl/backend/mcp/page_mutator_folder_test.go`, whose fake reader returns `module not found: ` for any non-module id exactly as the codec does. Control: production change stashed → `OpenPageForMutation: module not found: fPages` plus the three sibling failures. Repro `mdl-examples/bug-tests/1593-mcp-alter-page-in-folder.mdl`. Issue #1593", "refs": ["#1593", "#1071", "#976"]} diff --git a/docs-wiki/bug-patterns/duplicate-resolver-drift.md b/docs-wiki/bug-patterns/duplicate-resolver-drift.md index 8e7595641a..b78ddc922f 100644 --- a/docs-wiki/bug-patterns/duplicate-resolver-drift.md +++ b/docs-wiki/bug-patterns/duplicate-resolver-drift.md @@ -57,7 +57,12 @@ one reader, one renderer, one resolver — and taking the type set from *Per-doctype copies.* A behaviour implemented per document type drifts by construction. The folder clause is the clean example: every doctype's `FOLDER` handling had the same bug, and the report that named one of them read as a -doctype-specific defect. +doctype-specific defect. Its inverse — *which module owns this document?* — has +been answered by `GetModule(doc.ContainerID)` in one engine after another, which +is right only at the module root: the executor's reference checker (#1071), then +the MCP backend's page mutator and constant/workflow updates (#1593), each beside +a folder-aware resolver it did not call. The error names the folder's ID as a +"module not found", which is the tell. *Two walks over one tree.* The duplication does not need two *resolvers* — two traversals of the same structure drift the same way, and faster, because each diff --git a/mdl-examples/bug-tests/1593-mcp-alter-page-in-folder.mdl b/mdl-examples/bug-tests/1593-mcp-alter-page-in-folder.mdl new file mode 100644 index 0000000000..bd6976107d --- /dev/null +++ b/mdl-examples/bug-tests/1593-mcp-alter-page-in-folder.mdl @@ -0,0 +1,51 @@ +mdl 1; +-- ============================================================================ +-- ALTER PAGE over MCP failed for every page inside a folder (mendixlabs/mxcli#1593) +-- ============================================================================ +-- +-- Symptom (before fix), MCP backend against Studio Pro 11.12.5: +-- +-- failed to open page for mutation: module not found: +-- +-- for a page in a folder (one level or A/B/Pages), while the same statement on +-- a page at the module root succeeded. `check --references` passed in both +-- cases, and the trace showed no PED tool call before the error. +-- +-- Root cause: +-- The MCP page mutator resolved the page's module with +-- GetModule(page.ContainerID). A page's ContainerID is its immediate parent, +-- which for a foldered page is the FOLDER — the in the error was the +-- folder's ID. The backend already had a folder-aware resolver +-- (moduleNameForContainer, walking the folder chain up to the module); the +-- page mutator did not use it. UpdateConstant (CREATE OR MODIFY constant), +-- UpdateWorkflow (CREATE OR MODIFY workflow) and the session registries' +-- DROP cleanup had the same lookup and are fixed with it. +-- +-- WHAT THIS FILE PROVES, AND WHAT IT DOES NOT +-- Run with the default (modelsdk) backend, this script only shows the shape +-- is valid MDL and that the modelsdk engine handles it — that engine never +-- had the bug. The defect lived in the MCP backend, which needs a live +-- Studio Pro; it is covered by mdl/backend/mcp/page_mutator_folder_test.go +-- against a fake Studio Pro. To reproduce live, open the app in Studio Pro +-- and run this script with `--mcp `. + +create module "Bug1593"; + +create page "Bug1593"."RootPage" (Title: 'Root', Layout: Atlas_Core.Atlas_Default) { + dynamictext txtRoot (Content: 'root') +}; + +create page "Bug1593"."FolderPage" folder 'Pages' (Title: 'Folder', Layout: Atlas_Core.Atlas_Default) { + dynamictext txtFolder (Content: 'folder') +}; + +create page "Bug1593"."NestedPage" folder 'A/B/Pages' (Title: 'Nested', Layout: Atlas_Core.Atlas_Default) { + dynamictext txtNested (Content: 'nested') +}; + +-- control: module-root page always worked +alter page "Bug1593"."RootPage" { insert after txtRoot { dynamictext txtRoot2 (Content: 'root 2') } }; +-- reported: one folder deep +alter page "Bug1593"."FolderPage" { insert after txtFolder { dynamictext txtFolder2 (Content: 'folder 2') } }; +-- reported: nested folders +alter page "Bug1593"."NestedPage" { insert after txtNested { dynamictext txtNested2 (Content: 'nested 2') } }; diff --git a/mdl/backend/mcp/constant.go b/mdl/backend/mcp/constant.go index a00826386b..870384f92f 100644 --- a/mdl/backend/mcp/constant.go +++ b/mdl/backend/mcp/constant.go @@ -47,7 +47,7 @@ func (b *Backend) CreateConstant(c *model.Constant) error { // set it directly (same as an attribute's type), so a type change is rejected // rather than silently ignored. func (b *Backend) UpdateConstant(c *model.Constant) error { - mod, err := b.GetModule(c.ContainerID) + modName, err := b.moduleNameForContainer(c.ContainerID) if err != nil { return fmt.Errorf("resolve module for constant %q: %w", c.Name, err) } @@ -55,7 +55,7 @@ func (b *Backend) UpdateConstant(c *model.Constant) error { if err != nil { return err } - qn := mod.Name + "." + c.Name + qn := modName + "." + c.Name if live, err := b.liveConstantType(qn); err == nil && live != "" && live != pedType { return fmt.Errorf("constant %q: changing its type (%s -> %s) is not supported by the MCP backend — PED can't set the type element in place; drop and recreate, or edit in Studio Pro", c.Name, live, pedType) } @@ -66,7 +66,7 @@ func (b *Backend) UpdateConstant(c *model.Constant) error { if err := b.pedUpdateDoc(constantDocType, qn, ops...); err != nil { return err } - b.markDirty(mod.Name) + b.markDirty(modName) b.upsertSessionConstant(c) return b.pedCheckDocument(constantDocType, qn) } @@ -167,7 +167,7 @@ func (b *Backend) upsertSessionConstant(c *model.Constant) { func (b *Backend) forgetSessionConstant(moduleName, name string) { out := b.sessionConstants[:0] for _, c := range b.sessionConstants { - if mod, err := b.GetModule(c.ContainerID); err == nil && mod.Name == moduleName && c.Name == name { + if mod, err := b.moduleNameForContainer(c.ContainerID); err == nil && mod == moduleName && c.Name == name { continue } out = append(out, c) diff --git a/mdl/backend/mcp/enumeration.go b/mdl/backend/mcp/enumeration.go index c4e2364860..00569773c3 100644 --- a/mdl/backend/mcp/enumeration.go +++ b/mdl/backend/mcp/enumeration.go @@ -60,7 +60,7 @@ func (b *Backend) DeleteEnumeration(id model.ID) error { func (b *Backend) forgetSessionEnum(moduleName, name string) { out := b.sessionEnums[:0] for _, e := range b.sessionEnums { - if mod, err := b.GetModule(e.ContainerID); err == nil && mod.Name == moduleName && e.Name == name { + if mod, err := b.moduleNameForContainer(e.ContainerID); err == nil && mod == moduleName && e.Name == name { continue } out = append(out, e) diff --git a/mdl/backend/mcp/page_mutator.go b/mdl/backend/mcp/page_mutator.go index 370c0a7a06..2a234e8c41 100644 --- a/mdl/backend/mcp/page_mutator.go +++ b/mdl/backend/mcp/page_mutator.go @@ -35,26 +35,29 @@ func (b *Backend) OpenPageForMutation(unitID model.ID) (backend.PageMutator, err // resolvePageUnit maps a page unit ID to its module and page name (needed for the // pg_* tools, which are name-addressed). Saved pages come from the local reader; -// pages created over MCP this session come from sessionPages. +// pages created over MCP this session come from sessionPages. A page's ContainerID +// is its immediate parent — usually a folder, not the module — so the module is +// found by walking up the folder chain (mendixlabs/mxcli#1593). func (b *Backend) resolvePageUnit(unitID model.ID) (moduleName, pageName string, err error) { - for _, p := range b.sessionPages { - if p.ID == unitID { - mod, e := b.reader.GetModule(p.ContainerID) - if e != nil { - return "", "", e - } - return mod.Name, p.Name, nil + var p *pages.Page + for _, sp := range b.sessionPages { + if sp.ID == unitID { + p = sp + break } } - p, e := b.reader.GetPage(unitID) - if e != nil { - return "", "", fmt.Errorf("resolve page %s for mutation: %w", unitID, e) + if p == nil { + saved, e := b.reader.GetPage(unitID) + if e != nil { + return "", "", fmt.Errorf("resolve page %s for mutation: %w", unitID, e) + } + p = saved } - mod, e := b.reader.GetModule(p.ContainerID) + mod, e := b.moduleNameForContainer(p.ContainerID) if e != nil { - return "", "", e + return "", "", fmt.Errorf("resolve module of page %s: %w", p.Name, e) } - return mod.Name, p.Name, nil + return mod, p.Name, nil } type mcpPageMutator struct { diff --git a/mdl/backend/mcp/page_mutator_folder_test.go b/mdl/backend/mcp/page_mutator_folder_test.go new file mode 100644 index 0000000000..2fbd9f5664 --- /dev/null +++ b/mdl/backend/mcp/page_mutator_folder_test.go @@ -0,0 +1,218 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mcp + +import ( + "fmt" + "github.com/mendixlabs/mxcli/sdk/workflows" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// A page's ContainerID is its immediate parent, which for most Studio Pro pages is +// a folder. resolvePageUnit handed that ID straight to GetModule, so ALTER PAGE over +// MCP failed for every foldered page with "module not found: " before any +// pg_* call (mendixlabs/mxcli#1593). Root pages, whose parent IS the module, worked. +func TestResolvePageUnit_FolderedPages(t *testing.T) { + mod := &model.Module{Name: "MyFirstModule"} + mod.ID = "mod1" + folders := []*types.FolderInfo{ + {ID: "fA", ContainerID: "mod1", Name: "A"}, + {ID: "fB", ContainerID: "fA", Name: "B"}, + {ID: "fPages", ContainerID: "fB", Name: "Pages"}, + } + saved := map[model.ID]*pages.Page{} + for _, p := range []struct{ id, container, name string }{ + {"pRoot", "mod1", "RootPage"}, + {"pFolder", "fA", "FolderPage"}, + {"pNested", "fPages", "NestedPage"}, + } { + pg := &pages.Page{} + pg.ID = model.ID(p.id) + pg.ContainerID = model.ID(p.container) + pg.Name = p.name + saved[pg.ID] = pg + } + reader := &mock.MockBackend{ + // The local reader resolves only true module IDs, exactly as modelsdk does. + GetModuleFunc: func(id model.ID) (*model.Module, error) { + if id == mod.ID { + return mod, nil + } + return nil, fmt.Errorf("module not found: %s", id) + }, + ListFoldersFunc: func() ([]*types.FolderInfo, error) { return folders, nil }, + GetPageFunc: func(id model.ID) (*pages.Page, error) { + if p, ok := saved[id]; ok { + return p, nil + } + return nil, fmt.Errorf("page not found: %s", id) + }, + } + session := &pages.Page{} + session.ID = "pSession" + session.ContainerID = "fA" + session.Name = "SessionPage" + b := &Backend{reader: reader, sessionPages: []*pages.Page{session}} + + cases := []struct { + id model.ID + wantPage string + }{ + {"pRoot", "RootPage"}, // control: module-root page always resolved + {"pFolder", "FolderPage"}, // one folder deep (the reported case) + {"pNested", "NestedPage"}, // A/B/Pages + {"pSession", "SessionPage"}, // created over MCP this session, in a folder + } + for _, c := range cases { + mn, pn, err := b.resolvePageUnit(c.id) + if err != nil { + t.Errorf("resolvePageUnit(%s): %v", c.id, err) + continue + } + if mn != "MyFirstModule" || pn != c.wantPage { + t.Errorf("resolvePageUnit(%s) = (%q, %q), want (%q, %q)", c.id, mn, pn, "MyFirstModule", c.wantPage) + } + } + + // A page whose container chain is genuinely broken still errors. + saved["pOrphan"] = &pages.Page{} + saved["pOrphan"].ID = "pOrphan" + saved["pOrphan"].ContainerID = "nowhere" + if _, _, err := b.resolvePageUnit("pOrphan"); err == nil { + t.Error("orphaned page should fail to resolve") + } +} + +// End to end over a fake Studio Pro: the reporter's trace showed no PED call before +// the error. With the module resolved, the read goes out addressed by module name. +func TestOpenPageForMutation_FolderedPageReachesPgReadPage(t *testing.T) { + f := newFakePED(t, func(name string, _ map[string]any) (string, bool) { + if name == "pg_read_page" { + return `{"layout":"Atlas_Core.Atlas_Default","widgets":[]}`, false + } + return "{}", false + }) + mod := &model.Module{Name: "MyFirstModule"} + mod.ID = "mod1" + pg := &pages.Page{} + pg.ID = "p1" + pg.ContainerID = "fPages" + pg.Name = "Customer_Overview" + b := &Backend{ + client: f.connectClient(t), + reader: &mock.MockBackend{GetModuleFunc: func(id model.ID) (*model.Module, error) { + return nil, fmt.Errorf("module not found: %s", id) + }}, + sessionModules: []*model.Module{mod}, + sessionFolders: []*types.FolderInfo{{ID: "fPages", ContainerID: "mod1", Name: "Pages"}}, + sessionPages: []*pages.Page{pg}, + } + if _, err := b.OpenPageForMutation("p1"); err != nil { + t.Fatalf("OpenPageForMutation: %v", err) + } + call, ok := f.callByName("pg_read_page") + if !ok { + t.Fatal("pg_read_page was never called") + } + if call.Args["moduleName"] != "MyFirstModule" || call.Args["pageName"] != "Customer_Overview" { + t.Errorf("pg_read_page args = %v, want MyFirstModule.Customer_Overview", call.Args) + } +} + +// The same immediate-container lookup sat in the constant and workflow update paths +// (CREATE OR MODIFY on a foldered document) and in the session-registry cleanup. +func folderedBackend(t *testing.T, f *fakePED) *Backend { + t.Helper() + mod := &model.Module{Name: "M"} + mod.ID = "mod1" + b := &Backend{ + sessionModules: []*model.Module{mod}, + sessionFolders: []*types.FolderInfo{ + {ID: "f1", ContainerID: "mod1", Name: "Config"}, + {ID: "f2", ContainerID: "f1", Name: "Nested"}, + }, + } + if f != nil { + b.client = f.connectClient(t) + } + return b +} + +func documentNamesSent(f *fakePED) []string { + var out []string + for _, c := range f.calls { + if n, _ := c.Args["documentName"].(string); n != "" { + out = append(out, n) + } + } + return out +} + +func TestUpdateConstant_FolderedConstant(t *testing.T) { + f := newFakePED(t, func(name string, _ map[string]any) (string, bool) { + if name == "ped_check_errors" { + return "No errors found.", false + } + return "SUCCESS", false + }) + b := folderedBackend(t, f) + c := &model.Constant{Name: "ApiUrl", Type: model.ConstantDataType{Kind: "String"}, DefaultValue: "x"} + c.ID = "c1" + c.ContainerID = "f2" + if err := b.UpdateConstant(c); err != nil { + t.Fatalf("UpdateConstant: %v", err) + } + names := documentNamesSent(f) + if len(names) == 0 || names[0] != "M.ApiUrl" { + t.Errorf("PED calls addressed %v, want M.ApiUrl", names) + } +} + +func TestUpdateWorkflow_FolderedWorkflow(t *testing.T) { + f := newFakePED(t, func(name string, _ map[string]any) (string, bool) { + if name == "ped_check_errors" { + return "No errors found.", false + } + return "SUCCESS", false + }) + b := folderedBackend(t, f) + wf := &workflows.Workflow{ + Name: "WF", + Flow: &workflows.Flow{Activities: []workflows.WorkflowActivity{ + &workflows.StartWorkflowActivity{BaseWorkflowActivity: wfBase("Start", "Start")}, + &workflows.EndWorkflowActivity{BaseWorkflowActivity: wfBase("End", "End")}, + }}, + } + wf.ID = "wf1" + wf.ContainerID = "f1" + err := b.UpdateWorkflow(wf) + if err != nil && strings.Contains(err.Error(), "resolve module") { + t.Fatalf("UpdateWorkflow: %v", err) + } + names := documentNamesSent(f) + if len(names) == 0 || names[0] != "M.WF" { + t.Errorf("PED calls addressed %v, want M.WF", names) + } +} + +func TestForgetSessionDocuments_Foldered(t *testing.T) { + b := folderedBackend(t, nil) + c := &model.Constant{Name: "ApiUrl"} + c.ContainerID = "f2" + e := &model.Enumeration{Name: "Status"} + e.ContainerID = "f1" + b.sessionConstants = []*model.Constant{c} + b.sessionEnums = []*model.Enumeration{e} + b.forgetSessionConstant("M", "ApiUrl") + b.forgetSessionEnum("M", "Status") + if len(b.sessionConstants) != 0 || len(b.sessionEnums) != 0 { + t.Errorf("dropped foldered documents still registered: %d constant(s), %d enumeration(s)", + len(b.sessionConstants), len(b.sessionEnums)) + } +} diff --git a/mdl/backend/mcp/workflow.go b/mdl/backend/mcp/workflow.go index 7b53bfeb7c..40c2fe82f2 100644 --- a/mdl/backend/mcp/workflow.go +++ b/mdl/backend/mcp/workflow.go @@ -73,11 +73,11 @@ func (b *Backend) CreateWorkflow(wf *workflows.Workflow) error { // stable. exportLevel and overviewPage are not reapplied (no settable PED path, as // in ALTER WORKFLOW). func (b *Backend) UpdateWorkflow(wf *workflows.Workflow) error { - mod, err := b.GetModule(wf.ContainerID) + modName, err := b.moduleNameForContainer(wf.ContainerID) if err != nil { return fmt.Errorf("resolve module for workflow %q: %w", wf.Name, err) } - qn := mod.Name + "." + wf.Name + qn := modName + "." + wf.Name flowVal, err := b.mapWorkflowFlow(wf.Flow) if err != nil { @@ -192,7 +192,7 @@ func (b *Backend) UpdateWorkflow(wf *workflows.Workflow) error { if err := b.applyTimerBoundaryEvents(qn, wf, order); err != nil { return err } - b.markDirty(mod.Name) + b.markDirty(modName) b.upsertSessionWorkflow(wf) return b.pedCheckDocument(workflowDocType, qn) } From 0906ee5ca8c0670d7b4add517138de37bc9c8429 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 13:55:03 +0000 Subject: [PATCH 2/2] test(mcp): group imports in the foldered-document tests Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016ZCjbSMETkeDrr8bHi6LCD --- mdl/backend/mcp/page_mutator_folder_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mdl/backend/mcp/page_mutator_folder_test.go b/mdl/backend/mcp/page_mutator_folder_test.go index 2fbd9f5664..699bb9af80 100644 --- a/mdl/backend/mcp/page_mutator_folder_test.go +++ b/mdl/backend/mcp/page_mutator_folder_test.go @@ -4,7 +4,6 @@ package mcp import ( "fmt" - "github.com/mendixlabs/mxcli/sdk/workflows" "strings" "testing" @@ -12,6 +11,7 @@ import ( "github.com/mendixlabs/mxcli/mdl/types" "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/sdk/pages" + "github.com/mendixlabs/mxcli/sdk/workflows" ) // A page's ContainerID is its immediate parent, which for most Studio Pro pages is