From 09caa3e49c05ce4d832bf8301b6fbc1e74e8e960 Mon Sep 17 00:00:00 2001 From: Ivan Kondratyuk Date: Thu, 9 Jul 2026 17:08:19 +0300 Subject: [PATCH] fix(tools): getAccounts defaults to limit=100; getActor validates the UUID up front MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two dead-end traps found by a live audit of real assistant sessions: getAccounts: the backend default page is ~30 rows and the response has no has-more marker. On an actor with 20+ system accounts (Reactions, edges, Default, ...) the accounts created last — exactly the ones the caller just made and wants to see — fall off the first page, and the result reads as "the account was never created / the transaction did not work". This was the single most expensive support mystery in our project logs. The tool now requests limit=100 (the backend max) when the caller does not page explicitly; explicit paging is untouched. The Summary documents the backend default, the fact-only `total`, and the pair-access ghost (createAccount on a pair you cannot see succeeds but the account never appears in any listing — see createAccountPair). getActor: the backend answers 403 Access Denied for any malformed or truncated actorId, which reads as a permissions problem when it is a typo. A Resolve hook now validates the 36-char UUID form before the request and fails fast with the real reason plus the getActorByRef alternative. Verified live against mw (fresh binary over stdio JSON-RPC): - actor with 61 accounts: unpaged getAccounts returns all 40 visible fact rows (the old binary returned the backend 30-row page) - getActor("6bf39ec9"): instant format error instead of 403 Tests: scenario tests for the default/explicit limit and the UUID rejection/pass-through; the bound-arg test now binds a UUID-shaped id (real binds carry actor UUIDs). Full suite + vet green; discovery artifacts regenerated with no drift. Co-Authored-By: Claude Fable 5 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- .../mcp-server/internal/tools/accounts.go | 26 +++++++++- .../mcp-server/internal/tools/actors.go | 23 ++++++++- .../internal/tools/op_bound_test.go | 7 +-- .../internal/tools/scenario_test.go | 48 +++++++++++++++++++ 4 files changed, 98 insertions(+), 6 deletions(-) diff --git a/plugins/simulator/mcp-server/internal/tools/accounts.go b/plugins/simulator/mcp-server/internal/tools/accounts.go index 5410e9a..ff45362 100644 --- a/plugins/simulator/mcp-server/internal/tools/accounts.go +++ b/plugins/simulator/mcp-server/internal/tools/accounts.go @@ -1,5 +1,11 @@ package tools +import ( + "context" + + "github.com/corezoid/simulator-ai-plugin/plugins/simulator/mcp-server/internal/apiclient" +) + // accountTypes are the FORM_VALUE_ACCOUNT_TYPES — the account's VALUE type: // fact (the actual recorded value) | plan (a planned/budget value) | min | max | avg // (aggregates over children). Default fact. This is NOT an accounting category @@ -32,7 +38,10 @@ var accountOps = []Operation{ }, { Name: "getAccounts", Method: "GET", Path: "/accounts/{actorId}", - Summary: "List the accounts on an actor with their balances. Pass `from`/`to` to get each account's turnover/balance over that period (the account's movements summed within the window) — this is the account turnover for the period. The returned `amount` is the real balance value as a decimal (e.g. 1600 = 1600 USD); the currency `precision` only controls display rounding — do NOT divide by 10^precision.", + Summary: "List the accounts on an actor with their balances. Pass `from`/`to` to get each account's turnover/balance over that period (the account's movements summed within the window) — this is the account turnover for the period. The returned `amount` is the real balance value as a decimal (e.g. 1600 = 1600 USD); the currency `precision` only controls display rounding — do NOT divide by 10^precision. " + + "PAGINATION: the backend's default page is ~30 rows and the response carries NO has-more marker, so accounts created last (usually the ones you are looking for) silently fall off the first page — this tool therefore requests limit=100 (the backend max) when you don't pass `limit` yourself. If an actor may hold more than 100 accounts, page with `offset` until a short page. Note `total:true` counts only fact-type accounts. " + + "An account you KNOW exists (createAccount succeeded / \"already exist\") but that never appears in any page means you lack access to its (nameId, currencyId) PAIR — see createAccountPair.", + Resolve: defaultAccountsLimit, Params: []Param{ {Name: "actorId", In: InPath, Type: "string", Required: true, Desc: "Actor UUID."}, {Name: "accountType", In: InQuery, Type: "string", Enum: accountTypes, Desc: "Filter by account type."}, @@ -48,7 +57,7 @@ var accountOps = []Operation{ {Name: "withAggTypes", In: InQuery, Type: "boolean", Desc: "Include aggregated turnover by type in the response."}, {Name: "query", In: InQuery, Type: "string", Desc: "Search accounts by name."}, {Name: "total", In: InQuery, Type: "boolean", Desc: "Return the total count instead of the list."}, - {Name: "limit", In: InQuery, Type: "number", Desc: "Page size (max 100)."}, + {Name: "limit", In: InQuery, Type: "number", Desc: "Page size (max 100; defaults to 100 — the backend's own default is ~30 with no has-more marker)."}, {Name: "offset", In: InQuery, Type: "number", Desc: "Page offset."}, fieldFilterParam("id,accountName,currencyName,amount,availableAmount,incomeType,counterType"), {Name: "highPrecision", In: InQuery, Type: "boolean", Desc: "Return transaction sums with high precision."}, @@ -234,3 +243,16 @@ var accountOps = []Operation{ }, }, } + +// defaultAccountsLimit fills limit=100 (the backend max) when the caller does +// not page explicitly. The backend's own default page is ~30 rows and the +// response has no has-more marker, so accounts created last — usually exactly +// the ones the caller is trying to see — silently fell off the first page and +// read as "the account was never created". Callers that page explicitly are +// untouched. +func defaultAccountsLimit(_ context.Context, args map[string]any, _ *apiclient.Client) error { + if _, ok := args["limit"]; !ok { + args["limit"] = float64(100) + } + return nil +} diff --git a/plugins/simulator/mcp-server/internal/tools/actors.go b/plugins/simulator/mcp-server/internal/tools/actors.go index ca66c7d..5dd0d7f 100644 --- a/plugins/simulator/mcp-server/internal/tools/actors.go +++ b/plugins/simulator/mcp-server/internal/tools/actors.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "regexp" "github.com/corezoid/simulator-ai-plugin/plugins/simulator/mcp-server/internal/apiclient" ) @@ -108,8 +109,9 @@ var actorOps = []Operation{ "tens of thousands of tokens you rarely need. For a normal actor summary pass e.g. " + "filter=\"id,title,description,status,data,formId,formTitle,ownerId,createdAt,updatedAt,access\"; " + "omit `filter` only when you specifically need the form schema.", + Resolve: requireActorUUID, Params: []Param{ - {Name: "actorId", In: InPath, Type: "string", Required: true, Desc: "Actor UUID."}, + {Name: "actorId", In: InPath, Type: "string", Required: true, Desc: "Actor UUID (full 36-character form)."}, fieldFilterParam("id,title,description,status,data,formId,formTitle,ownerId,createdAt,updatedAt,access"), }, }, @@ -238,3 +240,22 @@ var actorOps = []Operation{ }, }, } + +// actorUUIDRe matches the full 36-character UUID form the backend expects in +// /actors/{actorId}. +var actorUUIDRe = regexp.MustCompile(`^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$`) + +// requireActorUUID rejects a malformed or truncated actorId before the request +// goes out: the backend answers 403 Access Denied for any non-UUID id, which +// reads as a permissions problem when it is actually a typo or a copy-paste of +// a shortened id. Failing fast with the real reason saves that dead end. +func requireActorUUID(_ context.Context, args map[string]any, _ *apiclient.Client) error { + id, _ := args["actorId"].(string) + if id == "" { + return nil // the required-parameter check reports the missing value + } + if !actorUUIDRe.MatchString(id) { + return fmt.Errorf("actorId %q is not a full actor UUID (36 chars, 8-4-4-4-12) — the backend would answer a misleading 403 Access Denied for it. Pass the complete UUID, or resolve the actor by its external key with getActorByRef(formId, ref)", id) + } + return nil +} diff --git a/plugins/simulator/mcp-server/internal/tools/op_bound_test.go b/plugins/simulator/mcp-server/internal/tools/op_bound_test.go index 7f447ab..ebc4d3b 100644 --- a/plugins/simulator/mcp-server/internal/tools/op_bound_test.go +++ b/plugins/simulator/mcp-server/internal/tools/op_bound_test.go @@ -13,7 +13,8 @@ import ( // the model never sets it and a Required param (actorId) is satisfied by the bind. func TestBoundArgHiddenAndInjected(t *testing.T) { op := opByName(t, "getActor") // GET /actors/{actorId}, actorId InPath + Required - bound := map[string]any{"actorId": "bound-1"} + // A real bind carries a full actor UUID (getActor validates the format). + bound := map[string]any{"actorId": "00000000-0000-4000-8000-0000000b0001"} // (1) hidden — the marshaled input schema has no actorId property. tool := mcp.NewTool(op.Name, toolOptions(op, bound)...) @@ -38,7 +39,7 @@ func TestBoundArgHiddenAndInjected(t *testing.T) { if res.IsError { t.Fatalf("unexpected error (bound actorId should satisfy Required): %+v", res.Content) } - if rec.path != "/actors/bound-1" { - t.Errorf("path = %s, want /actors/bound-1 (bound actorId injected)", rec.path) + if rec.path != "/actors/00000000-0000-4000-8000-0000000b0001" { + t.Errorf("path = %s, want /actors/00000000-0000-4000-8000-0000000b0001 (bound actorId injected)", rec.path) } } diff --git a/plugins/simulator/mcp-server/internal/tools/scenario_test.go b/plugins/simulator/mcp-server/internal/tools/scenario_test.go index 16c86e2..c43a281 100644 --- a/plugins/simulator/mcp-server/internal/tools/scenario_test.go +++ b/plugins/simulator/mcp-server/internal/tools/scenario_test.go @@ -3,6 +3,7 @@ package tools import ( "context" "encoding/json" + "fmt" "io" "net/http" "net/http/httptest" @@ -391,3 +392,50 @@ func TestQueryParam(t *testing.T) { t.Errorf("query = %q, want limit=5", rec.query) } } + +// TestGetAccountsDefaultsLimitTo100 verifies getAccounts requests limit=100 +// (the backend max) when the caller does not page explicitly: the backend's +// own default page is ~30 with no has-more marker, so accounts created last +// silently fell off the first page. An explicit limit is passed through. +func TestGetAccountsDefaultsLimitTo100(t *testing.T) { + c, rec := setup(t) + call(t, c, opByName(t, "getAccounts"), map[string]any{ + "actorId": "00000000-0000-4000-8000-0000000a0001", + }) + if rec.query != "limit=100" { + t.Errorf("query = %q, want limit=100 (default page must be the backend max)", rec.query) + } + + c2, rec2 := setup(t) + call(t, c2, opByName(t, "getAccounts"), map[string]any{ + "actorId": "00000000-0000-4000-8000-0000000a0001", + "limit": float64(10), + }) + if rec2.query != "limit=10" { + t.Errorf("query = %q, want limit=10 (explicit paging untouched)", rec2.query) + } +} + +// TestGetActorRejectsTruncatedUUID verifies a malformed/truncated actorId fails +// fast with an explanation instead of reaching the backend, which answers a +// misleading 403 Access Denied for any non-UUID id. +func TestGetActorRejectsTruncatedUUID(t *testing.T) { + c, _ := setup(t) + res := call(t, c, opByName(t, "getActor"), map[string]any{"actorId": "6bf39ec9"}) + if !res.IsError { + t.Fatal("expected an error for a truncated actorId, got success") + } + text := fmt.Sprintf("%+v", res.Content) + if !strings.Contains(text, "not a full actor UUID") { + t.Errorf("error must explain the UUID format problem, got: %s", text) + } + + c2, rec2 := setup(t) + res2 := call(t, c2, opByName(t, "getActor"), map[string]any{"actorId": "00000000-0000-4000-8000-0000000a0001"}) + if res2.IsError { + t.Fatalf("full UUID must pass validation, got: %+v", res2.Content) + } + if rec2.path != "/actors/00000000-0000-4000-8000-0000000a0001" { + t.Errorf("path = %q", rec2.path) + } +}