From 47f48228e6c116f04fd00ce142852ea07a31d4b3 Mon Sep 17 00:00:00 2001 From: Derrick Hammer Date: Wed, 2 Sep 2026 06:24:40 +0000 Subject: [PATCH 1/2] fix(mcp): split invoke_tool into read/write/destructive dispatchers Replaces the single invoke_tool meta-tool with three typed invokers (invoke_read_tool, invoke_write_tool, invoke_destructive_tool), each admitting exactly one safety class with matching tool annotations. describe_tool names each tool's dispatcher via a new invokeTool field and the dispatchers enforce the class server-side. --- AGENTS.md | 6 +- README.md | 6 +- cmd/mcp-test-server/main.go | 2 +- docs/architecture.md | 3 +- docs/mcp-host-audit.md | 2 +- internal/mcp/adapter.go | 12 +- internal/mcp/annotations_wire_test.go | 39 ++- internal/mcp/apps_wire_visible_test.go | 10 +- internal/mcp/catalog.go | 22 +- internal/mcp/catalog_test.go | 6 +- internal/mcp/catalogsurface.go | 2 +- internal/mcp/core/model/catalog_types.go | 4 +- internal/mcp/core/model/protocol_model.go | 2 +- internal/mcp/curated.go | 2 +- internal/mcp/custom_tools.go | 2 +- internal/mcp/custom_tools_register.go | 2 +- internal/mcp/devtools_test.go | 8 +- internal/mcp/grok_discovery_test.go | 3 +- internal/mcp/sdk_official.go | 298 +++++++++++++------ internal/mcp/sdk_official_test.go | 131 +++++++- internal/mcp/toolforge/forge.go | 16 +- tests/sunpeak/mcp-e2e/account.test.ts | 2 +- tests/sunpeak/mcp-e2e/auth.test.ts | 4 +- tests/sunpeak/mcp-e2e/dns.test.ts | 6 +- tests/sunpeak/mcp-e2e/helpers.ts | 51 +++- tests/sunpeak/mcp-e2e/ipns.test.ts | 4 +- tests/sunpeak/mcp-e2e/meta-tools.test.ts | 24 +- tests/sunpeak/mcp-e2e/operations.test.ts | 4 +- tests/sunpeak/mcp-e2e/pins.test.ts | 8 +- tests/sunpeak/mcp-e2e/pins2.test.ts | 2 +- tests/sunpeak/mcp-e2e/real-tools.test.ts | 2 +- tests/sunpeak/mcp-e2e/resources.test.ts | 2 +- tests/sunpeak/mcp-e2e/tool-surface.test.ts | 15 +- tests/sunpeak/mcp-e2e/websites.test.ts | 2 +- tests/sunpeak/mcp-e2e/wizard.test.ts | 2 +- tests/sunpeak/tests/protocol-surface.test.ts | 22 +- 36 files changed, 520 insertions(+), 208 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f0178f8a..a51322eb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -149,7 +149,9 @@ tests/sunpeak/ MCP integration tests (driver `pinner mcp` over stdi - `catalog.go` — `ToolCatalog`: a two-tier tool surface. Curated, most-used tools are listed directly in `tools/list`; the rest of the catalog is served through progressive disclosure (`search_tools` → `describe_tool` → - `invoke_tool`). + the typed invoke dispatchers `invoke_read_tool` / `invoke_write_tool` / + `invoke_destructive_tool`, split by safety class so each dispatcher's MCP + annotations are truthful for platform directory validation). - `hostenv/` + `toolforge/` — the MCP surface is host-aware: tool descriptions, schemas, and variants are resolved against the connected host profile (platform/transport/auth) via feature gating (`hostenv.Feature` @@ -341,7 +343,7 @@ refused (human confirms via hand-off), and the handler additionally rejects **MCP surface:** `ens_point` / `ens_unpoint` are single-level catalog ops (category `ens`) and are **not** in `compiledCuratedToolNames`, so they stay behind progressive disclosure (`search_tools {query:"ens"}` → -`describe_tool` → `invoke_tool`) and never bloat `tools/list`. The +`describe_tool` → the typed invoke dispatchers) and never bloat `tools/list`. The `agent_guide` `ens_publish` flow and the `ens-publish` prompt (`internal/mcp/prompttemplates/ens_publish.tmpl`) steer an agent to them. diff --git a/README.md b/README.md index a12c8ed4..8a751db1 100644 --- a/README.md +++ b/README.md @@ -733,7 +733,7 @@ When running under an MCP client, the server exposes three meta-tools for progre |-----------|---------| | `search_tools` | Search the internal tool catalog by keyword or category (`core`, `admin`, `wizard`) | | `describe_tool` | Get the full JSON Schema input definition for a specific tool | -| `invoke_tool` | Execute a tool by name with arguments | +| `invoke_read_tool` / `invoke_write_tool` / `invoke_destructive_tool` | Execute a tool by name with arguments; the dispatchers are split by safety class (read-only / mutating / destructive) and `describe_tool` names the right one | **Wizard tools** (website onboarding, setup) are also available through the meta-tools. They use FSM-based sessions with a 30-minute TTL and a 100-session limit. @@ -900,7 +900,7 @@ pinner-cli/ ├── pkg/internal/mcp/ # MCP adapter (MCPCommand, sessions, resources, prompts) │ ├── adapter.go # MCPCommand build and stdio serving │ ├── catalog.go # Tool catalog with progressive disclosure -│ ├── meta_tools.go # search_tools, describe_tool, invoke_tool +│ ├── meta_tools.go # search_tools, describe_tool, typed invoke dispatchers │ ├── session.go # FSM-based wizard session store │ ├── wizard.go # Website and setup wizard MCP tools │ ├── resources.go # pinner:// resource handlers @@ -934,7 +934,7 @@ pinner-cli/ | `BillingAdminService` | Admin credits, price lines, pricing plans, subscribers | | `WebsiteAdminService` | Admin website block/unblock | -**MCP Adapter**: `pkg/internal/mcp/` adapts the urfave/cli command tree into an MCP server (`mcp.MCPCommand`). It exposes subcommands as MCP tools via progressive disclosure (only `search_tools`, `describe_tool`, `invoke_tool` are visible in `tools/list`). Tool invocation is in-process — no subprocess fork. The adapter injects `--agent` automatically for all invocations. Wizard sessions are FSM-based with TTL cleanup. Resources and prompts are registered via `ResourceProvidersFactory` and `WithPrompts()`. +**MCP Adapter**: `pkg/internal/mcp/` adapts the urfave/cli command tree into an MCP server (`mcp.MCPCommand`). It exposes subcommands as MCP tools via progressive disclosure (only `search_tools`, `describe_tool`, and the typed invoke dispatchers `invoke_read_tool`/`invoke_write_tool`/`invoke_destructive_tool` are visible in `tools/list`). Tool invocation is in-process — no subprocess fork. The adapter injects `--agent` automatically for all invocations. Wizard sessions are FSM-based with TTL cleanup. Resources and prompts are registered via `ResourceProvidersFactory` and `WithPrompts()`. **Output Formatting**: The `Output` interface provides methods for formatted output (`Print`, `Printf`, `PrintTable`, `PrintJSON`, etc.) with both human-readable and JSON implementations, selected by the `--json` flag. diff --git a/cmd/mcp-test-server/main.go b/cmd/mcp-test-server/main.go index 65d868e5..ce2190b3 100644 --- a/cmd/mcp-test-server/main.go +++ b/cmd/mcp-test-server/main.go @@ -1,7 +1,7 @@ // Command mcp-test-server boots the swagger-generated fake Pinner API // (internal/mcptest) on a localhost port. It is the upstream API double used // by the Sunpeak MCP end-to-end tests: the pinner MCP server is pointed at -// this endpoint so `invoke_tool` calls return real data instead of an +// this endpoint so the invoke dispatchers return real data instead of an // "authentication required" error. // // It is a test-only binary, not part of the production pinner command. diff --git a/docs/architecture.md b/docs/architecture.md index cf9ad4ec..d15c97c9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -172,7 +172,8 @@ the frontends. - `catalog.go` — `ToolCatalog`: the in-memory registry behind a **two-tier tool surface**. Curated, most-used tools are listed directly in `tools/list`; the remaining catalog is served through progressive disclosure - (`search_tools` → `describe_tool` → `invoke_tool`), keeping the initial tool + (`search_tools` → `describe_tool` → the typed invoke dispatchers + `invoke_read_tool`/`invoke_write_tool`/`invoke_destructive_tool`), keeping the initial tool surface small and the context budget predictable. - `catalogassembly.go` — `AssembleCatalogOps(deps *CatalogDepsBundle)` builds one catalog covering every domain (auth, account, vault, pins, websites, diff --git a/docs/mcp-host-audit.md b/docs/mcp-host-audit.md index f8743e36..0d415fad 100644 --- a/docs/mcp-host-audit.md +++ b/docs/mcp-host-audit.md @@ -474,7 +474,7 @@ It is **not** an MCP tool. Do not pass `` to: - `describe_tool` -- `invoke_tool` +- the typed invoke dispatchers (`invoke_read_tool` / `invoke_write_tool` / `invoke_destructive_tool`) It represents the host action of PUTing bytes to the minted URL. diff --git a/internal/mcp/adapter.go b/internal/mcp/adapter.go index 659f8aba..eaf9f6ba 100644 --- a/internal/mcp/adapter.go +++ b/internal/mcp/adapter.go @@ -89,7 +89,9 @@ var serverCardTools = []map[string]any{ {"name": "upload_file", "description": "Upload a file to IPFS"}, {"name": "search_tools", "description": "Search the tool catalog"}, {"name": "describe_tool", "description": "Get a tool's input schema"}, - {"name": "invoke_tool", "description": "Invoke a catalog tool"}, + {"name": "invoke_read_tool", "description": "Invoke a read-only catalog tool"}, + {"name": "invoke_write_tool", "description": "Invoke a mutating catalog tool"}, + {"name": "invoke_destructive_tool", "description": "Invoke a destructive catalog tool"}, } // serverCardHandler serves the static MCP server card used by directory @@ -766,7 +768,7 @@ func routeVaultSetupHandlers(catalog *ToolCatalog, create, restore model.PinnerT // mcpInstructionsBase is sent to MCP clients in the initialize response. const mcpInstructionsBase = `This server exposes a curated set of common Pinner tools directly, including upload, pin, list, status, download, vault, website, website/domain wizard tools, and the agent-facing out-of-band sign-in tools (auth_sso and auth_resume). Setup wizard tools are kept out of the curated direct list because they duplicate the auth_sso/vault_create/vault_restore flows for CLI-style onboarding; they never accept passwords or OTP over this channel and remain reachable via search_tools. -The tool surface is intentionally two-tier. The tools listed directly in tools/list are the curated, most-used surface. The rest of the catalog (see count below) is served through progressive disclosure and is NOT broken or missing: any tool not listed directly is reachable via search_tools -> describe_tool -> invoke_tool. If a tool you expect is absent from tools/list, search for it rather than assuming it is unavailable. A large catalog is deliberately kept off the direct list to keep the initial tool surface small and the context budget predictable. +The tool surface is intentionally two-tier. The tools listed directly in tools/list are the curated, most-used surface. The rest of the catalog (see count below) is served through progressive disclosure and is NOT broken or missing: any tool not listed directly is reachable via search_tools -> describe_tool -> invoke_read_tool/invoke_write_tool/invoke_destructive_tool (the describe_tool response names the typed dispatcher for each tool). If a tool you expect is absent from tools/list, search for it rather than assuming it is unavailable. A large catalog is deliberately kept off the direct list to keep the initial tool surface small and the context budget predictable. For authentication, prefer the out-of-band flow: call auth_sso, give the returned approval URL to the human, then poll auth_resume with the returned handle until it reports done. This avoids an invalid or missing API key blocking work. @@ -779,12 +781,12 @@ Common flows start here: - search: search_tools({ "query": "" }) - filter: search_tools({ "category": "vault", "query": "" }) -Some internal commands are human-only or read piped stdin; when an agent invokes one via invoke_tool, the server returns a structured needs_human redirect instead of blocking. Commands that prompt interactively are hidden from search_tools entirely. +Some internal commands are human-only or read piped stdin; when an agent invokes one via the invoke dispatchers, the server returns a structured needs_human redirect instead of blocking. Commands that prompt interactively are hidden from search_tools entirely. Less common CLI tools remain available through progressive disclosure: 1. search_tools({ "query": "..." }): Find tools by keyword. Returns matching names, descriptions, and categories. -2. describe_tool({ "name": "..." }): Get the full input schema for one internal tool. -3. invoke_tool({ "name": "...", "arguments": { ... } }): Execute one internal tool. +2. describe_tool({ "name": "..." }): Get the full input schema for one internal tool; the response carries invokeTool, the dispatcher that executes it. +3. invoke_read_tool / invoke_write_tool / invoke_destructive_tool({ "name": "...", "arguments": { ... } }): Execute one internal tool with the dispatcher named by invokeTool. The internal catalog has %d tools. Local path arguments refer to the MCP server host, not the remote agent's filesystem. Upload and vault copy therefore require a host-side file handoff. File attachments can use the directly visible upload_file (IPFS) and vault_put_file (vault) tools over the banner-visible source modes; Pinner fetches the temporary file URL locally and uses its existing authenticated TUS path. Large uploads use TUS internally; the SDK result includes an upload location for resume/status management. TUS is never anonymous. Vault cat returns bounded base64 JSON in agent mode and never writes raw bytes to the MCP transport.` diff --git a/internal/mcp/annotations_wire_test.go b/internal/mcp/annotations_wire_test.go index f470b830..07f8ff89 100644 --- a/internal/mcp/annotations_wire_test.go +++ b/internal/mcp/annotations_wire_test.go @@ -7,8 +7,13 @@ package mcp // booleans (readOnlyHint, destructiveHint, openWorldHint); a nil pointer // hint is emitted as null on the wire and fails validators. // - The progressive-disclosure meta-tools (search_tools/describe_tool/ -// invoke_tool) additionally carry top-level titles (the Claude directory -// submission requires them; annotations.title is only a legacy fallback). +// invoke_read_tool/invoke_write_tool/invoke_destructive_tool) additionally +// carry top-level titles (the Claude directory submission requires them; +// annotations.title is only a legacy fallback). +// - The typed invoke dispatchers carry truthfully split safety hints +// (read: readOnly=true; mutating: openWorld=true; destructive: +// destructive=true, openWorld=true) so no single MCP tool straddles the +// safe/unsafe boundary the directory validators reject. // - auth_status's hints declare the platform-required values (an out-of-band // sign-in email cannot be unsent, so readOnly=false / destructive=true). // - Every ui:// app view declares one exact HTTPS origin (_meta.ui.domain @@ -63,20 +68,36 @@ func TestWireAnnotationsOnMetaTools(t *testing.T) { require.NoError(t, err) titles := map[string]string{ - "search_tools": "Search tool catalog", - "describe_tool": "Describe a catalog tool", - "invoke_tool": "Invoke a catalog tool", + "search_tools": "Search tool catalog", + "describe_tool": "Describe a catalog tool", + "invoke_read_tool": "Invoke a read-only catalog tool", + "invoke_write_tool": "Invoke a mutating catalog tool", + "invoke_destructive_tool": "Invoke a destructive catalog tool", + } + wantHints := map[string][3]bool{ + // {readOnlyHint, destructiveHint, openWorldHint} + "search_tools": {false, false, false}, + "describe_tool": {false, false, false}, + "invoke_read_tool": {true, false, false}, + "invoke_write_tool": {false, false, true}, + "invoke_destructive_tool": {false, true, true}, } seen := map[string]bool{} for _, tool := range res.Tools { requireBoolHints(t, tool) seen[tool.Name] = true want, ok := titles[tool.Name] - if !ok { - continue + if ok { + require.Equal(t, want, tool.Title, "%s: top-level title required for directory submission", tool.Name) + require.Equal(t, want, tool.Annotations.Title, "%s: annotations.title mirrors the title", tool.Name) + } + if hints, ok := wantHints[tool.Name]; ok { + require.Equal(t, hints[0], tool.Annotations.ReadOnlyHint, "%s: readOnlyHint", tool.Name) + require.NotNil(t, tool.Annotations.DestructiveHint, "%s: destructiveHint present", tool.Name) + require.Equal(t, hints[1], *tool.Annotations.DestructiveHint, "%s: destructiveHint", tool.Name) + require.NotNil(t, tool.Annotations.OpenWorldHint, "%s: openWorldHint present", tool.Name) + require.Equal(t, hints[2], *tool.Annotations.OpenWorldHint, "%s: openWorldHint", tool.Name) } - require.Equal(t, want, tool.Title, "%s: top-level title required for directory submission", tool.Name) - require.Equal(t, want, tool.Annotations.Title, "%s: annotations.title mirrors the title", tool.Name) } for name := range titles { require.True(t, seen[name], "%s must be listed", name) diff --git a/internal/mcp/apps_wire_visible_test.go b/internal/mcp/apps_wire_visible_test.go index 1110e8c2..630220fc 100644 --- a/internal/mcp/apps_wire_visible_test.go +++ b/internal/mcp/apps_wire_visible_test.go @@ -148,11 +148,11 @@ func TestOfficialToolHandlerAnnotatesHandoffEndToEnd(t *testing.T) { require.Contains(t, uiText, "https://example.com/account/password/tok", "URL must be preserved") } -// TestInvokeToolAnnotatesAppBackedHandoff regresses the invoke_tool meta-path: +// TestInvokeToolAnnotatesAppBackedHandoff regresses the typed-invoke meta-path: // a non-DirectVisible, app-backed catalog tool (e.g. vault_create/vault_restore) -// is dispatched by the invoke_tool closure directly to the inner catalog +// is dispatched by the invoke_write_tool closure directly to the inner catalog // handler, so the outer officialToolHandler annotation (keyed on the wired name -// "invoke_tool") never sees the real tool. The closure must annotate with the +// "invoke_write_tool") never sees the real tool. The closure must annotate with the // resolved inner name so a text-only host still learns the companion app exists. func TestInvokeToolAnnotatesAppBackedHandoff(t *testing.T) { registerTestAppView(t, "vault_create", apps.AppViewInfo{ @@ -183,7 +183,7 @@ func TestInvokeToolAnnotatesAppBackedHandoff(t *testing.T) { cs := connectOfficialClient(t, srv) res, err := cs.CallTool(context.Background(), &mcp.CallToolParams{ - Name: "invoke_tool", + Name: "invoke_write_tool", Arguments: map[string]any{ "name": "vault_create", "arguments": map[string]any{}, @@ -193,7 +193,7 @@ func TestInvokeToolAnnotatesAppBackedHandoff(t *testing.T) { require.False(t, res.IsError) require.NotNil(t, res.Content, "expected text content") text := requireText(t, res) - require.Contains(t, text, "Create Vault", "companion-app title must annotate invoke_tool-dispatched hand-off") + require.Contains(t, text, "Create Vault", "companion-app title must annotate invoke_write_tool-dispatched hand-off") require.Contains(t, text, "is also available in Apps-capable clients", "text-only client expects fallback wording") require.Contains(t, text, "https://example.com/vault/create/tok", "raw URL must be preserved") } diff --git a/internal/mcp/catalog.go b/internal/mcp/catalog.go index e4594b5d..fea2338e 100644 --- a/internal/mcp/catalog.go +++ b/internal/mcp/catalog.go @@ -45,14 +45,20 @@ type ToolDetail struct { ReadOnly bool `json:"readOnlyHint"` Destructive bool `json:"destructiveHint"` Interaction model.Interaction `json:"interaction,omitempty"` - InputSchema json.RawMessage `json:"inputSchema"` + // InvokeTool names the typed invoke dispatcher that executes this tool + // (invoke_read_tool / invoke_write_tool / invoke_destructive_tool), so an + // agent never has to guess which safety class a tool belongs to when the + // typed dispatchers enforce the split. + InvokeTool string `json:"invokeTool,omitempty"` + InputSchema json.RawMessage `json:"inputSchema"` } // ToolCatalog is an in-memory registry of tools that are discovered through -// the meta-tools (search_tools, describe_tool, invoke_tool) instead of being -// listed directly in tools/list. This implements server-side progressive -// disclosure: the MCP client sees only 3 meta-tools, while the real tool -// catalog stays internal. +// the meta-tools (search_tools, describe_tool, and the typed invoke +// dispatchers invoke_read_tool / invoke_write_tool / invoke_destructive_tool) +// instead of being listed directly in tools/list. This implements server-side +// progressive disclosure: the MCP client sees only the meta-tools, while the +// real tool catalog stays internal. type ToolCatalog struct { mu sync.RWMutex tools map[string]*model.ToolEntry @@ -304,7 +310,7 @@ func isPrimaryTool(name string) bool { // Suggest returns up to max tool names close to the given (unknown) name, // ordered by ascending Levenshtein distance then name. It lets describe_tool -// and invoke_tool answer with "did you mean ...?" instead of a bare +// and the invoke dispatchers answer with "did you mean ...?" instead of a bare // unknown-tool error. Tools that Search deliberately hides — wizards and // interactive/human-only tools — are excluded so suggestions never surface a // tool the agent could not discover. Distance uses a local zero-dependency @@ -408,6 +414,7 @@ func (c *ToolCatalog) Describe(name string) (*ToolDetail, error) { ReadOnly: entry.ReadOnly, Destructive: entry.Destructive, Interaction: entry.Interaction, + InvokeTool: classifyEntry(entry).dispatcher(), InputSchema: entry.InputSchema, }, nil } @@ -435,6 +442,7 @@ func (c *ToolCatalog) DescribeFor(name string, profile *hostenv.PlatformProfile) ReadOnly: entry.ReadOnly, Destructive: entry.Destructive, Interaction: entry.Interaction, + InvokeTool: classifyEntry(entry).dispatcher(), InputSchema: entry.InputSchema, }, nil } @@ -525,7 +533,7 @@ func (c *ToolCatalog) Invoke(ctx context.Context, name string, args map[string]a return model.ToolResult{}, fmt.Errorf("unknown tool: %s", name) } if entry.Category == model.CategoryAdmin { - return model.ToolResult{IsError: true, Text: fmt.Sprintf("admin tool %s is not available through invoke_tool; use search_tools with category=admin to discover admin tools", name)}, nil + return model.ToolResult{IsError: true, Text: fmt.Sprintf("admin tool %s is not available through the invoke dispatchers; use search_tools with category=admin to discover admin tools", name)}, nil } log.Info("meta-tool invoke", zap.String("tool", name)) diff --git a/internal/mcp/catalog_test.go b/internal/mcp/catalog_test.go index e81162aa..28a7a5cf 100644 --- a/internal/mcp/catalog_test.go +++ b/internal/mcp/catalog_test.go @@ -45,7 +45,7 @@ func TestBuildInstructionsEmbedsCount(t *testing.T) { // The two-tier surface is documented so clients that read tools/list learn // an absent tool is reachable via discovery, not missing/broken. require.Contains(t, got, "intentionally two-tier") - require.Contains(t, got, "reachable via search_tools -> describe_tool -> invoke_tool") + require.Contains(t, got, "reachable via search_tools -> describe_tool -> invoke_read_tool/invoke_write_tool/invoke_destructive_tool") // The system prompt must direct agents to agent_guide and include the // publish/website flow so agents know how to create websites. require.Contains(t, got, "call agent_guide first") @@ -122,7 +122,7 @@ func TestStringSliceFlagEmitsArraySchema(t *testing.T) { // TestVaultRestoreInteractionStaysStdinInputThroughBuildCatalog asserts that // buildCatalog never reclassifies pinner_vault_restore away from stdin_input, // even when an OOB restore coordinator is wired. The Interaction enum drives -// the invoke_tool stdin gate (sdk_official.go), which switches on +// the invoke dispatchers' stdin gate (sdk_official.go), which switches on // entry.Interaction: if it became agent_safe, a --seed-stdin invocation would // fall through the switch and run io.ReadAll(os.Stdin), desyncing the stdio // MCP transport. The non-stdin OOB hand-off is already permitted by the @@ -184,7 +184,7 @@ func TestSSOToolsDiscoverableInCatalog(t *testing.T) { assert.Equal(t, 1, ssoCount, "auth_sso must be listed exactly once") assert.Equal(t, 1, resumeCount, "auth_resume must be listed exactly once") - // describe_tool / invoke_tool must also resolve them. auth_sso is an + // describe_tool / the invoke dispatchers must also resolve them. auth_sso is an // account-domain tool, so it surfaces under CategoryAccount. d, err := catalog.Describe("auth_sso") require.NoError(t, err) diff --git a/internal/mcp/catalogsurface.go b/internal/mcp/catalogsurface.go index dd3b55c3..06f16e8f 100644 --- a/internal/mcp/catalogsurface.go +++ b/internal/mcp/catalogsurface.go @@ -32,7 +32,7 @@ func startupProfile() hostenv.PlatformProfile { // This file is the bridge between the operation catalog (the compiler-backed // source of truth for MCP tool descriptions/schemas) and the legacy ToolCatalog // that drives the official MCP server's progressive-disclosure meta-tools -// (search_tools, describe_tool, invoke_tool). +// (search_tools, describe_tool, invoke_read_tool/invoke_write_tool/invoke_destructive_tool). // // The compiled catalog yields ToolDescriptors whose Description/InputSchema // come from the catalogops MCPTargets fallback and typed arg metadata, so CLI diff --git a/internal/mcp/core/model/catalog_types.go b/internal/mcp/core/model/catalog_types.go index 41c3338d..1c70f0cd 100644 --- a/internal/mcp/core/model/catalog_types.go +++ b/internal/mcp/core/model/catalog_types.go @@ -16,7 +16,7 @@ const ( ) // Interaction classifies how a tool behaves when invoked by an agent over the -// MCP channel (via invoke_tool). It lets the server steer agents away from +// MCP channel (via the typed invoke dispatchers). It lets the server steer agents away from // commands that would read drained stdin or block on a prompt, and instead // return a structured redirect so an agent never hangs on a deep command. // @@ -31,7 +31,7 @@ const ( InteractionAgentSafe Interaction = "agent_safe" // InteractionInteractive marks a tool that is purely human-facing (a // wizard/setup flow that prompts interactively). Agents should not invoke - // it; invoke_tool redirects, and search_tools hides it. + // it; the invoke dispatchers redirect, and search_tools hides it. InteractionInteractive Interaction = "interactive" ) diff --git a/internal/mcp/core/model/protocol_model.go b/internal/mcp/core/model/protocol_model.go index e0ade47f..5a1f6a64 100644 --- a/internal/mcp/core/model/protocol_model.go +++ b/internal/mcp/core/model/protocol_model.go @@ -159,7 +159,7 @@ func DescriptorFromTool(entry *ToolEntry) ToolDescriptor { // It lets a tool that is registered as a direct (tools/list) descriptor, such // as the out-of-band sign-in tools, ALSO be surfaced through progressive // discovery (search_tools/describe_tool) so both discovery surfaces stay in -// sync. The entry keeps its handler so invoke_tool can call it. +// sync. The entry keeps its handler so the typed invoke dispatchers can call it. func ToolEntryFromDescriptor(desc ToolDescriptor) *ToolEntry { return &ToolEntry{ Name: desc.Name, diff --git a/internal/mcp/curated.go b/internal/mcp/curated.go index 5b8df08b..727a22a3 100644 --- a/internal/mcp/curated.go +++ b/internal/mcp/curated.go @@ -8,7 +8,7 @@ package mcp // human-reviewable order. // // This is a deliberately small front door. The full tool catalog (~170 ops) -// remains behind the search_tools / describe_tool / invoke_tool progressive- +// remains behind the search_tools / describe_tool / typed-invoke progressive- // disclosure meta-tools. Everything listed here is either essential for first- // call orientation (auth_status), vault lifecycle entry points (vault_create, // vault_restore, vault_status), the vault's distinctive share primitive diff --git a/internal/mcp/custom_tools.go b/internal/mcp/custom_tools.go index 0ca1911a..41bab02a 100644 --- a/internal/mcp/custom_tools.go +++ b/internal/mcp/custom_tools.go @@ -577,7 +577,7 @@ func registerCustomTools(deps customToolDeps) error { // Always expose the agent guide so a model can orient to the primary flows // without probing each tool's description. It is both directly visible on // tools/list and indexed in the catalog so a cold-start host that follows - // search_tools(help) can resolve and read it via describe_tool / invoke_tool. + // search_tools(help) can resolve and read it via describe_tool / the typed invoke dispatchers. reg.add(customToolSpec{desc: NewAgentGuideDescriptor(), index: true, direct: true}) // Optionally expose the prompt templates, filtered to the surface so a diff --git a/internal/mcp/custom_tools_register.go b/internal/mcp/custom_tools_register.go index a4bc8e67..ca52c3a4 100644 --- a/internal/mcp/custom_tools_register.go +++ b/internal/mcp/custom_tools_register.go @@ -18,7 +18,7 @@ type customToolSpec struct { desc model.ToolDescriptor // index adds the tool to the ToolCatalog so it is discoverable through - // search_tools/describe_tool/invoke_tool. App views resolve their + // search_tools/describe_tool/typed-invoke dispatchers. App views resolve their // launchers against the catalog, so index always precedes app install. index bool diff --git a/internal/mcp/devtools_test.go b/internal/mcp/devtools_test.go index 2725f6be..d58b91e0 100644 --- a/internal/mcp/devtools_test.go +++ b/internal/mcp/devtools_test.go @@ -120,8 +120,8 @@ func TestDevRequestHandlerNilCapsSafe(t *testing.T) { require.False(t, res.IsError) } -// TestInvokeToolThreadsCaps guards the invoke_tool behavior dev tools rely on: -// when a catalog tool is invoked through the invoke_tool meta-tool, the calling +// TestInvokeToolThreadsCaps guards the invoke dispatcher behavior dev tools rely on: +// when a catalog tool is invoked through a typed invoke dispatcher, the calling // client's Caps must be threaded to the inner handler. It regresses a latent bug // where Caps (and thus the resolved host profile) was dropped on this path. func TestInvokeToolThreadsCaps(t *testing.T) { @@ -142,7 +142,7 @@ func TestInvokeToolThreadsCaps(t *testing.T) { defer cs.Close() res, err := cs.CallTool(context.Background(), &mcp.CallToolParams{ - Name: "invoke_tool", + Name: "invoke_write_tool", Arguments: map[string]any{ "name": "probe", "arguments": map[string]any{}, @@ -150,5 +150,5 @@ func TestInvokeToolThreadsCaps(t *testing.T) { }) require.NoError(t, err) require.False(t, res.IsError) - require.True(t, seenCaps, "invoke_tool must thread Caps (with resolved profile) to the inner handler") + require.True(t, seenCaps, "invoke_write_tool must thread Caps (with resolved profile) to the inner handler") } diff --git a/internal/mcp/grok_discovery_test.go b/internal/mcp/grok_discovery_test.go index e8f36e2d..cbdb46da 100644 --- a/internal/mcp/grok_discovery_test.go +++ b/internal/mcp/grok_discovery_test.go @@ -79,7 +79,8 @@ func TestGuideHintTargetIsResolvable(t *testing.T) { _, err := catalog.Describe("agent_guide") require.NoError(t, err) - // invoke_tool(agent_guide) must be invokable (registered, seeded handler). + // agent_guide must be invokable (registered, seeded handler); the typed + // invoke dispatchers call it through catalog.Get. _, ok := catalog.Get("agent_guide") require.True(t, ok, "agent_guide must be registered for invoke") } diff --git a/internal/mcp/sdk_official.go b/internal/mcp/sdk_official.go index 4644a901..7d4191cb 100644 --- a/internal/mcp/sdk_official.go +++ b/internal/mcp/sdk_official.go @@ -5,8 +5,9 @@ // registrations on the official MCP server, preserving Pinner's wire JSON // contract exactly: // -// - the three visible meta-tools (search_tools, describe_tool, -// invoke_tool) and their serialized schemas; +// - the visible meta-tools (search_tools, describe_tool, and the typed +// invoke dispatchers invoke_read_tool / invoke_write_tool / +// invoke_destructive_tool) and their serialized schemas; // - the progressive-disclosure catalog invocation behavior; // - pinner:// resource and resource-template URIs, MIME types and payloads; // - prompt names, arguments, roles, text and embedded resources. @@ -292,10 +293,12 @@ func registerTool(srv *sdk.Server, desc model.ToolDescriptor, handler model.Pinn return sdk.RegisterTool(srv, sdkHandlerDeps, desc) } -// RegisterOfficialMetaTools registers the three progressive-disclosure -// meta-tools (search_tools, describe_tool, invoke_tool) on an official-SDK -// server. The catalog itself stays hidden; the only tools visible via -// tools/list are these three, preserving progressive disclosure. +// RegisterOfficialMetaTools registers the progressive-disclosure meta-tools +// (search_tools, describe_tool, and the typed invoke dispatchers +// invoke_read_tool / invoke_write_tool / invoke_destructive_tool) on an +// official-SDK server. The catalog itself stays hidden; the tools visible via +// tools/list are these five, preserving progressive disclosure while keeping +// each invoke tool's hints truthful about the safety class it executes. func RegisterOfficialMetaTools(srv *sdk.Server, catalog *ToolCatalog, stdioMode bool, seedDrop *oob.SeedDrop, oobRestore *oob.OOBRestore, oobCreate *oob.OOBCreate) error { if srv == nil { return fmt.Errorf("nil official server") @@ -310,7 +313,7 @@ func RegisterOfficialMetaTools(srv *sdk.Server, catalog *ToolCatalog, stdioMode if err := registerOfficialDescribeTool(srv, catalog); err != nil { return err } - return registerOfficialInvokeTool(srv, catalog, stdioMode, seedDrop, oobRestore, oobCreate) + return registerOfficialInvokeTools(srv, catalog, stdioMode, seedDrop, oobRestore, oobCreate) } // metaToolSchema is a tiny SDK-neutral input schema builder for the static @@ -348,7 +351,8 @@ type describeToolInput struct { Name string `json:"name" jsonschema:"description=Tool name from search_tools result."` } -// invokeToolInput is the typed argument shape for invoke_tool. +// invokeToolInput is the typed argument shape for the typed invoke +// dispatchers (invoke_read_tool / invoke_write_tool / invoke_destructive_tool). type invokeToolInput struct { Name string `json:"name" jsonschema:"description=Tool name from search_tools result."` Arguments map[string]any `json:"arguments,omitempty" jsonschema:"description=Arguments object matching the tool's inputSchema."` @@ -372,7 +376,7 @@ func registerOfficialSearchTools(srv *sdk.Server, catalog *ToolCatalog) error { // Discovery workflow. This description documents the full search -> // describe -> invoke loop and the dual-surface policy (some file-I/O // tools are host-curated and not in this catalog). - discoveryNote := "Search the internal tool catalog by a single keyword. No boolean (AND/OR) syntax: pass one keyword at a time (e.g. 'pin', not 'pin OR upload'). Name matches are ranked exact, then starts-with, contains, then within-segment subsequence (a fuzzy abbreviation within a single word of the name), then whole-word description matches; tools that never match are omitted. Use the 'category' filter to narrow scope and 'limit' to cap results. Leave query empty or use 'help' for an onboarding listing of just the primary start-here tools, which also carries agent_guide for the full flows and a hint pointing at category browsing for a specific domain. Workflow: after discovering a tool here, call describe_tool(name) for its input schema, then invoke_tool(name, arguments). Capability and file-transfer tools are exposed directly on the tool surface AND indexed here, so they are discoverable by name (e.g. 'upload', 'capabilities'). Interactive wizard flows (category 'wizard') are excluded unless you filter for them specifically." + discoveryNote := "Search the internal tool catalog by a single keyword. No boolean (AND/OR) syntax: pass one keyword at a time (e.g. 'pin', not 'pin OR upload'). Name matches are ranked exact, then starts-with, contains, then within-segment subsequence (a fuzzy abbreviation within a single word of the name), then whole-word description matches; tools that never match are omitted. Use the 'category' filter to narrow scope and 'limit' to cap results. Leave query empty or use 'help' for an onboarding listing of just the primary start-here tools, which also carries agent_guide for the full flows and a hint pointing at category browsing for a specific domain. Workflow: after discovering a tool here, call describe_tool(name) for its input schema; the describe response carries an invokeTool field naming the typed dispatcher that executes it (invoke_read_tool for read-only tools, invoke_write_tool for mutating tools, invoke_destructive_tool for destructive tools — each dispatcher refuses out-of-class tools, so route by the named one). Capability and file-transfer tools are exposed directly on the tool surface AND indexed here, so they are discoverable by name (e.g. 'upload', 'capabilities'). Interactive wizard flows (category 'wizard') are excluded unless you filter for them specifically." desc := model.ToolDescriptor{ Name: "search_tools", @@ -464,98 +468,209 @@ func registerOfficialDescribeTool(srv *sdk.Server, catalog *ToolCatalog) error { return sdk.RegisterTool(srv, sdkHandlerDeps, desc) } -func registerOfficialInvokeTool(srv *sdk.Server, catalog *ToolCatalog, stdioMode bool, seedDrop *oob.SeedDrop, oobRestore *oob.OOBRestore, oobCreate *oob.OOBCreate) error { - schema := &metaToolSchema{} - schema.property("name", map[string]any{ - "type": "string", - "description": "Tool name from search_tools result", - }) - schema.property("arguments", map[string]any{ - "type": "object", - "description": "Arguments object matching the tool's inputSchema. Use describe_tool to see the schema.", - }) +// invokeClass identifies the safety class a typed invoke dispatcher admits. +// The platform directory validators (OpenAI/Claude) require a tool's hints to +// match what the tool CAN do, and explicitly reject a single catch-all tool +// that mixes safe and unsafe operations. name+arguments dispatch therefore +// lives in three typed tools, each admitted exactly one safety class, so the +// wire annotations are truthful and no dispatcher straddles a safety +// boundary. +type invokeClass int + +const ( + invokeClassRead invokeClass = iota + invokeClassWrite + invokeClassDestructive +) - desc := model.ToolDescriptor{ - Name: "invoke_tool", - Title: "Invoke a catalog tool", - Description: "Execute a tool by name with the given arguments. This is the third step of the discovery workflow: search_tools(name) to find a tool, describe_tool(name) for its input schema, then invoke_tool(name, arguments). The arguments object is validated against the tool's inputSchema returned by describe_tool.", - OpenWorldHint: false, - InputSchema: schema.raw(), +// classifyEntry maps a catalog entry's platform hints onto the typed-invoke +// safety class. The hint values (not the raw catalog Safety) drive the split +// because they are the platform-truthful classification — e.g. the auth_status +// override (an out-of-band sign-in email cannot be unsent) moves it into the +// destructive bucket despite its SafetyRead origin. A readOnly entry that +// still declares openWorld contradicts the read contract (validators reject +// readOnly+openWorld), so it is conservatively reachable only through the +// write dispatcher, whose hints cover it. +func classifyEntry(entry *model.ToolEntry) invokeClass { + switch { + case entry.Destructive: + return invokeClassDestructive + case entry.ReadOnly && !entry.OpenWorldHint: + return invokeClassRead + default: + return invokeClassWrite } +} - desc.Handler = model.PinnerToolHandler(func(ctx context.Context, request model.ToolRequest) (model.ToolResult, error) { - in, err := toolargs.DecodeToolArgs[invokeToolInput](request) - if err != nil { - return model.ToolResult{IsError: true, Text: err.Error()}, nil - } - if in.Name == "" { - return model.ToolResult{IsError: true, Text: "name is required"}, nil - } - toolArgs := in.Arguments - if toolArgs == nil { - toolArgs = map[string]any{} +// dispatcher is the MCP tool name of the typed invoke tool for this class. +func (c invokeClass) dispatcher() string { + switch c { + case invokeClassRead: + return "invoke_read_tool" + case invokeClassDestructive: + return "invoke_destructive_tool" + default: + return "invoke_write_tool" + } +} + +// registerOfficialInvokeTools registers the three typed invoke dispatchers. +// Each executes only catalog tools of its own safety class and refuses the +// rest with a pointer to the right dispatcher, keeping progressive discovery +// while making every MCP tool's annotations match its real capabilities. +func registerOfficialInvokeTools(srv *sdk.Server, catalog *ToolCatalog, stdioMode bool, seedDrop *oob.SeedDrop, oobRestore *oob.OOBRestore, oobCreate *oob.OOBCreate) error { + specs := []struct { + name string + title string + description string + class invokeClass + readOnly bool + destructive bool + openWorld bool + }{ + { + name: "invoke_read_tool", + title: "Invoke a read-only catalog tool", + description: "Execute a read-only catalog tool by name with the given arguments. This is the third step of the discovery workflow: search_tools(name) to find a tool, describe_tool(name) for its input schema (the describe response names the dispatcher to invoke), then invoke_read_tool(name, arguments) when the tool's hints are readOnlyHint=true. The dispatcher refuses non-read-only tools; use invoke_write_tool or invoke_destructive_tool for those. The arguments object is validated against the tool's inputSchema returned by describe_tool.", + class: invokeClassRead, + readOnly: true, + }, + { + name: "invoke_write_tool", + title: "Invoke a mutating catalog tool", + description: "Execute a state-mutating (but not destructive) catalog tool by name with the given arguments. This is the third step of the discovery workflow: search_tools(name) to find a tool, describe_tool(name) for its input schema (the describe response names the dispatcher to invoke), then invoke_write_tool(name, arguments) when the tool's hints are readOnlyHint=false and destructiveHint=false. The dispatcher refuses read-only and destructive tools; use invoke_read_tool or invoke_destructive_tool for those. The arguments object is validated against the tool's inputSchema returned by describe_tool.", + class: invokeClassWrite, + openWorld: true, + }, + { + name: "invoke_destructive_tool", + title: "Invoke a destructive catalog tool", + description: "Execute a destructive (irreversible / deletion) catalog tool by name with the given arguments. This is the third step of the discovery workflow: search_tools(name) to find a tool, describe_tool(name) for its input schema (the describe response names the dispatcher to invoke), then invoke_destructive_tool(name, arguments) when the tool's hints carry destructiveHint=true. Destructive operations additionally require human confirmation (the server returns a needs_human hand-off before running). The dispatcher refuses non-destructive tools; use invoke_read_tool or invoke_write_tool for those. The arguments object is validated against the tool's inputSchema returned by describe_tool.", + class: invokeClassDestructive, + destructive: true, + openWorld: true, + }, + } + + for _, spec := range specs { + schema := &metaToolSchema{} + schema.property("name", map[string]any{ + "type": "string", + "description": "Tool name from search_tools result", + }) + schema.property("arguments", map[string]any{ + "type": "object", + "description": "Arguments object matching the tool's inputSchema. Use describe_tool to see the schema.", + }) + + desc := model.ToolDescriptor{ + Name: spec.name, + Title: spec.title, + Description: spec.description, + ReadOnly: spec.readOnly, + Destructive: spec.destructive, + OpenWorldHint: spec.openWorld, + InputSchema: schema.raw(), } - entry, ok := catalog.Get(in.Name) - if !ok { - // Unknown tool: offer nearest names so the agent can recover - // without a separate search round-trip. - suggestions := catalog.Suggest(in.Name, 3) - resp := map[string]any{ - "error": fmt.Sprintf("unknown tool: %s", in.Name), - "suggest": suggestions, + + desc.Handler = model.PinnerToolHandler(func(ctx context.Context, request model.ToolRequest) (model.ToolResult, error) { + in, err := toolargs.DecodeToolArgs[invokeToolInput](request) + if err != nil { + return model.ToolResult{IsError: true, Text: err.Error()}, nil } - if len(suggestions) > 0 { - resp["message"] = "unknown tool. did you mean one of these?" + if in.Name == "" { + return model.ToolResult{IsError: true, Text: "name is required"}, nil + } + toolArgs := in.Arguments + if toolArgs == nil { + toolArgs = map[string]any{} + } + entry, ok := catalog.Get(in.Name) + if !ok { + // Unknown tool: offer nearest names so the agent can recover + // without a separate search round-trip. + suggestions := catalog.Suggest(in.Name, 3) + resp := map[string]any{ + "error": fmt.Sprintf("unknown tool: %s", in.Name), + "suggest": suggestions, + } + if len(suggestions) > 0 { + resp["message"] = "unknown tool. did you mean one of these?" + } + out, _ := json.Marshal(resp) + return model.ToolResult{IsError: true, Text: string(out)}, nil } - out, _ := json.Marshal(resp) - return model.ToolResult{IsError: true, Text: string(out)}, nil - } - // Admin tools are gated from invoke_tool, matching the search/describe - // gate: an agent that somehow knows an admin tool name by heart cannot - // invoke it. Admin tools are only discoverable via - // search_tools(category=admin) and are not invokable through this path. - if entry.Category == model.CategoryAdmin { - return model.ToolResult{IsError: true, Text: fmt.Sprintf("admin tool %s is not available through invoke_tool; use search_tools with category=admin to discover admin tools", in.Name)}, nil - } + // Safety-class gate: each dispatcher admits one class only. This is + // the server-side half of the typed-invoke split — the annotations + // claim a capability, and the handler enforces that the capability + // boundary actually holds at dispatch time. + if got := classifyEntry(entry); got != spec.class { + return model.ToolResult{IsError: true, Text: fmt.Sprintf("tool %s is a %s operation; call %s(name, arguments) instead", in.Name, classNoun(got), got.dispatcher())}, nil + } - // Steer agents away from commands they cannot run safely over the MCP - // channel, instead of letting them hang. A human-only (interactive) - // command always redirects. Everything else runs normally. - // - // Stdin-reading is a CLI-side concern only and is never gated here: a - // command whose action reads piped stdin (e.g. `vault restore - // --seed-stdin`) is a human/terminal mechanism that is not exposed - // through MCP. The agent-facing vault tools are the agent-safe OOB - // hand-offs (vaultSetupOps), which never touch os.Stdin. So the invoke - // gate only redirects interactive (human-only setup) tools. - switch entry.Interaction { - case model.InteractionInteractive: - return model.NeedsHumanResult(model.NeedsHuman{ - Reason: model.ReasonInteractiveOnly, - ResumeTool: "", - Detail: "This command is human-only (it prompts interactively) and has no agent-safe form. Run it via the CLI, or use the curated agent tool for the same workflow.", - }), nil - } + // Admin tools are gated from the invoke dispatchers, matching the + // search/describe gate: an agent that somehow knows an admin tool + // name by heart cannot invoke it. Admin tools are only discoverable + // via search_tools(category=admin) and are not invokable through + // this path. + if entry.Category == model.CategoryAdmin { + return model.ToolResult{IsError: true, Text: fmt.Sprintf("admin tool %s is not available through the invoke dispatchers; use search_tools with category=admin to discover admin tools", in.Name)}, nil + } - // Thread the calling client's Caps through to the inner tool so it can - // adapt per host (e.g. profile-aware dev_* tools). Caps was previously - // dropped here; every handler already nil-guards it. - result, err := entry.Handler(ctx, model.ToolRequest{Name: in.Name, Arguments: toolArgs, Caps: request.Caps}) - if err != nil { - return model.ToolResult{IsError: true, Text: err.Error()}, nil + // Steer agents away from commands they cannot run safely over the MCP + // channel, instead of letting them hang. A human-only (interactive) + // command always redirects. Everything else runs normally. + // + // Stdin-reading is a CLI-side concern only and is never gated here: a + // command whose action reads piped stdin (e.g. `vault restore + // --seed-stdin`) is a human/terminal mechanism that is not exposed + // through MCP. The agent-facing vault tools are the agent-safe OOB + // hand-offs (vaultSetupOps), which never touch os.Stdin. So the invoke + // gate only redirects interactive (human-only setup) tools. + switch entry.Interaction { + case model.InteractionInteractive: + return model.NeedsHumanResult(model.NeedsHuman{ + Reason: model.ReasonInteractiveOnly, + ResumeTool: "", + Detail: "This command is human-only (it prompts interactively) and has no agent-safe form. Run it via the CLI, or use the curated agent tool for the same workflow.", + }), nil + } + + // Thread the calling client's Caps through to the inner tool so it can + // adapt per host (e.g. profile-aware dev_* tools). Caps was previously + // dropped here; every handler already nil-guards it. + result, err := entry.Handler(ctx, model.ToolRequest{Name: in.Name, Arguments: toolArgs, Caps: request.Caps}) + if err != nil { + return model.ToolResult{IsError: true, Text: err.Error()}, nil + } + // The typed dispatcher routes to the inner catalog handler directly, + // so the outer adapter's annotation (keyed on the dispatcher name) + // never sees the real tool. Annotate here with the resolved inner + // name so companion-app metadata reaches text-only hosts for + // non-DirectVisible tools (e.g. vault_create/vault_restore) that are + // only reachable through the meta-tools. + annotateAppOnHandoff(in.Name, request.Caps, &result) + return result, nil + }) + + if err := sdk.RegisterTool(srv, sdkHandlerDeps, desc); err != nil { + return err } - // invoke_tool dispatches to the inner catalog handler directly, so the - // outer adapter's annotation (keyed on req.Params.Name == - // "invoke_tool") never sees the real tool. Annotate here with the - // resolved inner name so companion-app metadata reaches text-only hosts - // for non-DirectVisible tools (e.g. vault_create/vault_restore) that are - // only reachable through this meta-tool. - annotateAppOnHandoff(in.Name, request.Caps, &result) - return result, nil - }) + } + return nil +} - return sdk.RegisterTool(srv, sdkHandlerDeps, desc) +// classNoun names a safety class in human-readable error text. +func classNoun(c invokeClass) string { + switch c { + case invokeClassRead: + return "read-only" + case invokeClassDestructive: + return "destructive" + default: + return "mutating" + } } // RegisterOfficialDescriptor adds one Pinner-owned tool directly to tools/list. @@ -572,7 +687,8 @@ func RegisterOfficialDescriptor(srv *sdk.Server, desc model.ToolDescriptor) erro // RegisterOfficialCuratedTools exposes the catalog's directly-visible tools // (those with DirectVisible set) as standard tools/list tools. Remaining // catalog entries stay behind the progressive-disclosure meta-tools -// (search_tools / describe_tool / invoke_tool) which index the whole catalog. +// (search_tools / describe_tool / invoke_read_tool / invoke_write_tool / +// invoke_destructive_tool) which index the whole catalog. func RegisterOfficialCuratedTools(srv *sdk.Server, catalog *ToolCatalog) error { if srv == nil { return fmt.Errorf("nil official server") diff --git a/internal/mcp/sdk_official_test.go b/internal/mcp/sdk_official_test.go index a7b84e27..7ed13b14 100644 --- a/internal/mcp/sdk_official_test.go +++ b/internal/mcp/sdk_official_test.go @@ -18,11 +18,11 @@ import ( "github.com/stretchr/testify/require" "go.lumeweb.com/pinner-cli/internal/catalogops" - "go.lumeweb.com/pinner-cli/internal/mcp/core/session" "go.lumeweb.com/pinner-cli/internal/mcp/core/model" + "go.lumeweb.com/pinner-cli/internal/mcp/core/session" - "go.lumeweb.com/pinner-cli/internal/mcp/hostenv" "go.lumeweb.com/pinner-cli/internal/mcp/core/handoff" + "go.lumeweb.com/pinner-cli/internal/mcp/hostenv" "go.lumeweb.com/pinner-cli/internal/mcp/oob" "go.lumeweb.com/pinner-cli/internal/mcp/sdk" "go.lumeweb.com/pinner-cli/internal/mcp/vault" @@ -190,10 +190,12 @@ func TestOfficialMetaToolsListed(t *testing.T) { for _, tool := range res.Tools { names[tool.Name] = true } - require.Len(t, res.Tools, 3) + require.Len(t, res.Tools, 5) require.True(t, names["search_tools"], "search_tools listed") require.True(t, names["describe_tool"], "describe_tool listed") - require.True(t, names["invoke_tool"], "invoke_tool listed") + require.True(t, names["invoke_read_tool"], "invoke_read_tool listed") + require.True(t, names["invoke_write_tool"], "invoke_write_tool listed") + require.True(t, names["invoke_destructive_tool"], "invoke_destructive_tool listed") require.False(t, names["pinner_status"], "catalog tool must stay hidden") } @@ -317,7 +319,7 @@ func TestOfficialInvokeToolExecutesCatalog(t *testing.T) { cs := connectOfficialClient(t, srv) res, err := cs.CallTool(context.Background(), &mcp.CallToolParams{ - Name: "invoke_tool", + Name: "invoke_read_tool", Arguments: map[string]any{ "name": "pinner_status", "arguments": map[string]any{"json": true}, @@ -333,7 +335,7 @@ func TestOfficialInvokeToolUnknownIsError(t *testing.T) { cs := connectOfficialClient(t, srv) res, err := cs.CallTool(context.Background(), &mcp.CallToolParams{ - Name: "invoke_tool", + Name: "invoke_read_tool", Arguments: map[string]any{ "name": "pinner_does_not_exist", }, @@ -342,8 +344,113 @@ func TestOfficialInvokeToolUnknownIsError(t *testing.T) { require.True(t, res.IsError) } -// TestOfficialInvokeToolRedirectsInteractiveOnly verifies that invoke_tool -// steers agents away from interactive (human-only setup) tools by returning a +// TestOfficialInvokeClassGate pins the typed-invoke safety split: each +// dispatcher admits only its own class (read / mutating / destructive) and +// refuses out-of-class tools with a pointer to the right dispatcher. This is +// the platform requirement that no single MCP tool may mix safe and unsafe +// operations. +func TestOfficialInvokeClassGate(t *testing.T) { + catalog := NewToolCatalog() + catalog.Add(&model.ToolEntry{ + Name: "read_probe", + Description: "Read-only probe", + ReadOnly: true, + InputSchema: json.RawMessage(`{"type":"object"}`), + Handler: func(context.Context, model.ToolRequest) (model.ToolResult, error) { + return model.ToolResult{Text: "read"}, nil + }, + }) + catalog.Add(&model.ToolEntry{ + Name: "write_probe", + Description: "Mutating probe", + InputSchema: json.RawMessage(`{"type":"object"}`), + Handler: func(context.Context, model.ToolRequest) (model.ToolResult, error) { + return model.ToolResult{Text: "write"}, nil + }, + }) + run := false + catalog.Add(&model.ToolEntry{ + Name: "destroy_probe", + Description: "Destructive probe", + Destructive: true, + OpenWorldHint: true, + InputSchema: json.RawMessage(`{"type":"object"}`), + Handler: func(context.Context, model.ToolRequest) (model.ToolResult, error) { + run = true + return model.ToolResult{Text: "destroy"}, nil + }, + }) + + srv := sdk.NewServer(nil) + require.NoError(t, RegisterOfficialMetaTools(srv, catalog, false, nil, nil, nil)) + cs := connectOfficialClient(t, srv) + defer cs.Close() + + call := func(t *testing.T, dispatcher, name string) (bool, string) { + t.Helper() + res, err := cs.CallTool(context.Background(), &mcp.CallToolParams{ + Name: dispatcher, + Arguments: map[string]any{ + "name": name, + "arguments": map[string]any{}, + }, + }) + require.NoError(t, err) + return res.IsError, requireText(t, res) + } + + // Right dispatcher: the inner handler runs. + isErr, text := call(t, "invoke_read_tool", "read_probe") + require.False(t, isErr, text) + require.Equal(t, "read", text) + + isErr, text = call(t, "invoke_write_tool", "write_probe") + require.False(t, isErr, text) + require.Equal(t, "write", text) + + isErr, text = call(t, "invoke_destructive_tool", "destroy_probe") + require.False(t, isErr) + require.True(t, run) + + // Wrong dispatcher: refused with guidance, handler never runs. + isErr, text = call(t, "invoke_read_tool", "write_probe") + require.True(t, isErr) + require.Contains(t, text, "mutating operation") + require.Contains(t, text, "invoke_write_tool") + + isErr, text = call(t, "invoke_read_tool", "destroy_probe") + require.True(t, isErr) + require.Contains(t, text, "destructive operation") + require.Contains(t, text, "invoke_destructive_tool") + + isErr, text = call(t, "invoke_write_tool", "read_probe") + require.True(t, isErr) + require.Contains(t, text, "read-only operation") + require.Contains(t, text, "invoke_read_tool") + + // describe_tool names the dispatcher for each class so agents can route + // without guessing. + for _, tc := range []struct { + name, want string + }{ + {"read_probe", "invoke_read_tool"}, + {"write_probe", "invoke_write_tool"}, + {"destroy_probe", "invoke_destructive_tool"}, + } { + res, err := cs.CallTool(context.Background(), &mcp.CallToolParams{ + Name: "describe_tool", + Arguments: map[string]any{"name": tc.name}, + }) + require.NoError(t, err) + require.False(t, res.IsError) + var detail ToolDetail + require.NoError(t, json.Unmarshal([]byte(requireText(t, res)), &detail)) + require.Equal(t, tc.want, detail.InvokeTool, "%s must name its dispatcher", tc.name) + } +} + +// TestOfficialInvokeToolRedirectsInteractiveOnly verifies that the invoke +// dispatchers steer agents away from interactive (human-only setup) tools by returning a // needs_human hand-off instead of running them, while agent-safe tools // (including the OOB vault restore) run their handler directly. Stdin gating // is a CLI-side concern; the MCP invoke path does not perform stdin gating. @@ -380,7 +487,7 @@ func TestOfficialInvokeToolRedirectsInteractiveOnly(t *testing.T) { // Interactive tool -> needs_human redirect, handler not called. res, err := cs.CallTool(context.Background(), &mcp.CallToolParams{ - Name: "invoke_tool", + Name: "invoke_write_tool", Arguments: map[string]any{ "name": "pinner_setup", "arguments": map[string]any{}, @@ -397,7 +504,7 @@ func TestOfficialInvokeToolRedirectsInteractiveOnly(t *testing.T) { // Agent-safe tool (vault restore OOB) runs its handler, with no stdin // gating. res, err = cs.CallTool(context.Background(), &mcp.CallToolParams{ - Name: "invoke_tool", + Name: "invoke_write_tool", Arguments: map[string]any{ "name": "pinner_vault_restore", "arguments": map[string]any{}, @@ -449,7 +556,7 @@ func TestOfficialReadResourceTemplate(t *testing.T) { // buildCatalog (with an OOB restore coordinator wired) and asserts the stdin // gate still redirects it. This is the regression the hand-built-catalog tests // do not cover: buildCatalog previously reclassified pinner_vault_restore to -// agent_safe, which made the invoke_tool switch on entry.Interaction fall +// agent_safe, which made the invoke dispatchers' switch on entry.Interaction fall // through and run os.Stdin — desyncing the stdio transport — instead of // honoring the gate. The enum must stay stdin_input so the gate holds, while // the non-stdin OOB hand-off (bypassGate) remains reachable. @@ -495,7 +602,7 @@ func TestOfficialInvokeVaultRestoreRoutesAgentSafeHandoff(t *testing.T) { // A plain restore invoke (no seed on the channel) must reach the handler and // return a needs_human restore_url hand-off. res, err := cs.CallTool(context.Background(), &mcp.CallToolParams{ - Name: "invoke_tool", + Name: "invoke_write_tool", Arguments: map[string]any{ "name": vault.CompiledVaultRestoreToolName, "arguments": map[string]any{}, diff --git a/internal/mcp/toolforge/forge.go b/internal/mcp/toolforge/forge.go index 41f93a86..a036fa13 100644 --- a/internal/mcp/toolforge/forge.go +++ b/internal/mcp/toolforge/forge.go @@ -149,7 +149,7 @@ func copyMeta(m map[string]any) map[string]any { // the corresponding feature. var instructionsTemplate = template.Must(template.New("instructions").Parse(`This server exposes a curated set of common Pinner tools directly, including upload, pin, list, status, download, vault, website, website/domain wizard tools, and the agent-facing out-of-band sign-in tools (auth_sso and auth_resume). Setup wizard tools are kept out of the curated direct list because they duplicate the auth_sso/vault_create/vault_restore flows for CLI-style onboarding; they never accept passwords or OTP over this channel and remain reachable via search_tools. -The tool surface is intentionally two-tier. The tools listed directly in tools/list are the curated, most-used surface. The rest of the catalog (see count below) is served through progressive disclosure and is NOT broken or missing: any tool not listed directly is reachable via search_tools -> describe_tool -> invoke_tool. If a tool you expect is absent from tools/list, search for it rather than assuming it is unavailable. A large catalog is deliberately kept off the direct list to keep the initial tool surface small and the context budget predictable. +The tool surface is intentionally two-tier. The tools listed directly in tools/list are the curated, most-used surface. The rest of the catalog (see count below) is served through progressive disclosure and is NOT broken or missing: any tool not listed directly is reachable via search_tools -> describe_tool -> invoke_read_tool/invoke_write_tool/invoke_destructive_tool (the describe_tool response names the typed dispatcher for each tool). If a tool you expect is absent from tools/list, search for it rather than assuming it is unavailable. A large catalog is deliberately kept off the direct list to keep the initial tool surface small and the context budget predictable. For authentication, prefer the out-of-band flow: call auth_sso, give the returned approval URL to the human, then poll auth_resume with the returned handle until it reports done. This avoids an invalid or missing API key blocking work. @@ -162,7 +162,7 @@ Common flows start here: - search: search_tools({ "query": "" }) - filter: search_tools({ "category": "vault", "query": "" }) -Some internal commands are human-only or read piped stdin; when an agent invokes one via invoke_tool, the server returns a structured needs_human redirect instead of blocking. Commands that prompt interactively are hidden from search_tools entirely. +Some internal commands are human-only or read piped stdin; when an agent invokes one via the invoke dispatchers, the server returns a structured needs_human redirect instead of blocking. Commands that prompt interactively are hidden from search_tools entirely. The internal catalog has {{.ToolCount}} tools. {{if .FileHostInput}} @@ -179,12 +179,12 @@ Companion interactive pages (MCP Apps) may render alongside tool results when a // instructionsData is the template execution context. type instructionsData struct { - ToolCount int - FileHostInput bool - SourcePath bool - SourceMint bool - SourceRelay bool - MCPApps bool + ToolCount int + FileHostInput bool + SourcePath bool + SourceMint bool + SourceRelay bool + MCPApps bool } // buildInstructions returns the MCP server instructions for the given diff --git a/tests/sunpeak/mcp-e2e/account.test.ts b/tests/sunpeak/mcp-e2e/account.test.ts index ca709ed0..824690c9 100644 --- a/tests/sunpeak/mcp-e2e/account.test.ts +++ b/tests/sunpeak/mcp-e2e/account.test.ts @@ -3,7 +3,7 @@ import { invoke } from './helpers'; /** * Account domain tool (account_subscription) driven through the - * host-discovery contract: every call goes through invoke_tool (the + * host-discovery contract: every call goes through the typed invoke dispatchers (the * progressive-disclosure meta-tool) with { name, args } — the same path a * ChatGPT/Claude host uses to surface catalog tools. * diff --git a/tests/sunpeak/mcp-e2e/auth.test.ts b/tests/sunpeak/mcp-e2e/auth.test.ts index 87b5aa8b..80ff015c 100644 --- a/tests/sunpeak/mcp-e2e/auth.test.ts +++ b/tests/sunpeak/mcp-e2e/auth.test.ts @@ -11,11 +11,11 @@ test.describe.configure({ mode: 'serial' }); /** * Auth domain tools (auth_status / auth_login / auth_logout) driven through - * the host-discovery contract: every call goes through invoke_tool (the + * the host-discovery contract: every call goes through the typed invoke dispatchers (the * progressive-disclosure meta-tool) with { name, args } — the same path a * ChatGPT/Claude host uses to surface catalog tools. auth_status and * auth_logout are directly curated onto tools/list; auth_login deliberately - * lives only behind invoke_tool (tool-surface.test.ts enforces that). + * lives only behind the typed invoke tools (tool-surface.test.ts enforces that). * * STATE SAFETY (the tricky part): auth_login and auth_logout are LOCAL config * operations — they persist the edited credential to diff --git a/tests/sunpeak/mcp-e2e/dns.test.ts b/tests/sunpeak/mcp-e2e/dns.test.ts index 61793528..5d95a027 100644 --- a/tests/sunpeak/mcp-e2e/dns.test.ts +++ b/tests/sunpeak/mcp-e2e/dns.test.ts @@ -12,7 +12,7 @@ test.describe.configure({ mode: 'serial' }); /** * DNS domain tools (dns_zones_* / dns_records_*) driven through the - * host-discovery contract: every call goes through invoke_tool (the + * host-discovery contract: every call goes through the typed invoke dispatchers (the * progressive-disclosure meta-tool) with { name, args }, never by calling the * direct tool name. * @@ -52,7 +52,7 @@ test.describe.configure({ mode: 'serial' }); * - dns_zones_delete / dns_records_delete are SafetyDestructive; the MCP * dispatch layer refuses destructive ops invoked by a model actor with a * needs_human confirmation handoff BEFORE the handler runs. So through - * invoke_tool they always return the confirmation hand-off, not a delete. + * the invoke tool they always return the confirmation hand-off, not a delete. * This test locks that gate. */ @@ -94,7 +94,7 @@ test('dns_zones_list now contains the created zone', async ({ mcp }) => { test('dns_zones_get resolves the zone by domain name (round-trip)', async ({ mcp }) => { // Resolve the zone by its domain (resolveZoneID lists zones and matches - // .Domain), proving the full invoke_tool -> SDK -> HTTP -> fake chain. + // .Domain), proving the full invoke dispatchers -> SDK -> HTTP -> fake chain. const result = await invoke(mcp, 'dns_zones_get', { zone: Domain }); expect(isCleanSuccess(result)).toBe(true); diff --git a/tests/sunpeak/mcp-e2e/helpers.ts b/tests/sunpeak/mcp-e2e/helpers.ts index 552ce4db..2f8471d0 100644 --- a/tests/sunpeak/mcp-e2e/helpers.ts +++ b/tests/sunpeak/mcp-e2e/helpers.ts @@ -4,19 +4,58 @@ import type { McpFixture, CallToolResult } from 'sunpeak/test'; * Shared MCP-driver helpers for the Sunpeak e2e suite. * * Pinner's public MCP surface uses progressive disclosure: tools/list only - * advertises `search_tools`, `describe_tool`, and `invoke_tool`. Domain tools - * (`account_info`, `pins_list`, `dns_*`, …) are reachable ONLY by calling - * `invoke_tool` with `{ name, args }`. These helpers centralize that - * boilerplate so every per-tool test file stays declarative. + * advertises `search_tools`, `describe_tool`, and the typed invoke + * dispatchers (`invoke_read_tool` / `invoke_write_tool` / + * `invoke_destructive_tool`). Domain tools (`account_info`, `pins_list`, + * `dns_*`, …) are reachable ONLY through a typed dispatcher with + * `{ name, args }`. Each dispatcher enforces exactly one safety class + * (read-only / mutating / destructive), matching the platform directory rule + * that a single MCP tool must not mix safe and unsafe operations. These + * helpers centralize that routing so every per-tool test file stays + * declarative. */ -/** Call a domain tool through the progressive-disclosure invoke_tool path. */ +const invokeCache = new Map(); + +/** + * Resolve the typed invoke dispatcher for a catalog tool. describe_tool + * carries an `invokeTool` field naming the dispatcher for the tool's safety + * class; results are cached per fixture session. Unknown/unresolvable names + * default to the read dispatcher so the clean unknown-tool error path still + * flows through the meta-tools. + */ +async function resolveInvokeTool(mcp: McpFixture, name: string): Promise { + const cached = invokeCache.get(name); + if (cached) { + return cached; + } + const described = await describeTool(mcp, name); + if (described.isError !== true) { + // describe_tool returns the ToolDetail as canonical JSON on the text + // channel; parse it there (structuredContent is not used for meta-tool + // results). + let detail: { invokeTool?: unknown } | undefined; + try { + detail = JSON.parse(textOf(described)) as { invokeTool?: unknown }; + } catch { + detail = undefined; + } + if (typeof detail?.invokeTool === 'string' && detail.invokeTool) { + invokeCache.set(name, detail.invokeTool); + return detail.invokeTool; + } + } + return 'invoke_read_tool'; +} + +/** Call a domain tool through the typed progressive-disclosure invoke path. */ export async function invoke( mcp: McpFixture, name: string, args?: Record, ): Promise { - return mcp.callTool('invoke_tool', { name, arguments: args ?? {} }); + const dispatcher = await resolveInvokeTool(mcp, name); + return mcp.callTool(dispatcher, { name, arguments: args ?? {} }); } /** Concatenate all text blocks of a CallToolResult. */ diff --git a/tests/sunpeak/mcp-e2e/ipns.test.ts b/tests/sunpeak/mcp-e2e/ipns.test.ts index 6191b14e..b3aebd24 100644 --- a/tests/sunpeak/mcp-e2e/ipns.test.ts +++ b/tests/sunpeak/mcp-e2e/ipns.test.ts @@ -10,7 +10,7 @@ test.describe.configure({ mode: 'serial' }); /** * IPNS domain tools (ipns_keys_* / ipns_publish / ipns_republish / * ipns_resolve) driven through the host-discovery contract: every call goes - * through invoke_tool (the progressive-disclosure meta-tool) with { name, + * through the typed invoke dispatchers (the progressive-disclosure meta-tools) with { name, * args }, never by calling the direct tool name. * * CI-PENDING: this file is verified in CI (it drives tools through the real @@ -39,7 +39,7 @@ test.describe.configure({ mode: 'serial' }); * - ipns_republish takes `key-name` and returns {count, message}. * - ipns_keys_delete is SafetyDestructive; the MCP dispatch layer refuses * destructive ops invoked by a model actor with a needs_human - * confirmation handoff BEFORE the handler runs. Through invoke_tool it + * confirmation handoff BEFORE the handler runs. Through the invoke tool it * always returns the confirmation hand-off, not a delete. This locks the * gate. */ diff --git a/tests/sunpeak/mcp-e2e/meta-tools.test.ts b/tests/sunpeak/mcp-e2e/meta-tools.test.ts index 306395b0..37d983f5 100644 --- a/tests/sunpeak/mcp-e2e/meta-tools.test.ts +++ b/tests/sunpeak/mcp-e2e/meta-tools.test.ts @@ -2,19 +2,21 @@ import { test, expect } from 'sunpeak/test'; import { invoke, describeTool, searchTool, textOf } from './helpers'; /** - * Progressive-disclosure meta-tools: search_tools / describe_tool / invoke_tool - * and the host-curated orientation tools (capabilities / agent_guide). + * Progressive-disclosure meta-tools: search_tools / describe_tool and the + * typed invoke dispatchers (invoke_read_tool / invoke_write_tool / + * invoke_destructive_tool) plus the host-curated orientation tools + * (capabilities / agent_guide). * - * search_tools / describe_tool / invoke_tool operate over the FULL operation + * The meta-tools operate over the FULL operation * catalog (reachable by keyword / name); capabilities and agent_guide are * direct tools on the public surface. These tests lock the discovery contract: * ranked keyword search, input-schema introspection, the clean error paths of - * invoke_tool, and the structured orientation output of the two direct tools. + * the typed invoke tools, and the structured orientation output of the two direct tools. * * Keywords / structured shapes below were probed against the running server * (2026-08-22) before being locked. * - * NOTE on isCleanSuccess: it is tuned for API-touching invoke_tool results and + * NOTE on isCleanSuccess: it is tuned for API-touching invoke results and * flags the words "authenticated"/"authentication" as auth failures. Those words * legitimately appear inside catalog *descriptions* returned by search_tools / * describe_tool, so for those discovery tools we use `not.toBeError()` as the @@ -78,16 +80,16 @@ test('describe_tool returns the input schema', async ({ mcp }) => { expect(accountInfo).toHaveTextContent('"type":"object"'); }); -// ── invoke_tool: unknown + validation error paths ─────────────────── +// ── typed invoke tools: unknown + validation error paths ─────────── -test('invoke_tool with unknown name returns a clean error', async ({ mcp }) => { +test('invoke with unknown name returns a clean error', async ({ mcp }) => { const result = await invoke(mcp, '_definitely_not_a_real_tool_', {}); expect(result).toBeError(); // A clean error still carries explanatory text; it must not crash the session. expect(result).toHaveTextContent('unknown tool'); }); -test('invoke_tool with missing required arg returns a validation error', async ({ mcp }) => { +test('invoke with missing required arg returns a validation error', async ({ mcp }) => { const result = await invoke(mcp, 'pins_add', {}); expect(result).toBeError(); // pins_add requires cids; the validation failure names the missing field. @@ -175,7 +177,7 @@ test('agent_guide returns a coherent guide whose steps resolve to real tools', a } // Cross-surface invariant: every step tool the guide names resolves to a real - // tool in the catalog (advertised OR behind invoke_tool). describe_tool alone + // tool in the catalog (advertised OR behind the typed invoke tools). describe_tool alone // only resolves catalog operations, so host-curated direct tools the guide // legitimately references (e.g. capabilities) would be flagged "unknown". // A step is real if it is either on the advertised tools/list (covers direct @@ -189,14 +191,14 @@ test('agent_guide returns a coherent guide whose steps resolve to real tools', a }); // Sanity: the session is still healthy after the error-path tests (unknown tool -// and validation failure). We re-route a call through invoke_tool and confirm +// and validation failure). We re-route a call through the typed invoke tool and confirm // it still VALIDATES deterministically (clean arg-validation error, not a // crashed/hung session). We deliberately avoid an upstream-API-touching call // (e.g. account_info) here: the fake API is shared and its auth-ping route is // flaky under the parallel suite, which would make this session-health check // depend on an unrelated upstream double. test('session survives the error-path tests', async ({ mcp }) => { - // invoke_tool must still be responsive and validating after the earlier + // The invoke tool must still be responsive and validating after the earlier // unknown-tool and missing-arg errors — a clean validation error proves the // session did not crash or wedge. const result = await invoke(mcp, 'pins_add', {}); diff --git a/tests/sunpeak/mcp-e2e/operations.test.ts b/tests/sunpeak/mcp-e2e/operations.test.ts index f225a9f2..fe00bc49 100644 --- a/tests/sunpeak/mcp-e2e/operations.test.ts +++ b/tests/sunpeak/mcp-e2e/operations.test.ts @@ -3,12 +3,12 @@ import { invoke } from './helpers'; /** * Operations domain tools (operations_list / operations_get) driven through - * the host-discovery contract: every call goes through invoke_tool with + * the host-discovery contract: every call goes through the typed invoke dispatchers with * { name, args } — the same path a ChatGPT/Claude host uses. * * These read the seeded operations (internal/mcptest/account SeedOperations * seeds two deterministic rows: id 1 = completed pin, id 2 = running upload), - * proving the full invoke_tool -> MCP -> SDK -> fake-API chain returns real + * proving the full typed-invoke -> MCP -> SDK -> fake-API chain returns real * operation data (not a 501 stub or a generic error). */ test.describe.configure({ mode: 'serial' }); diff --git a/tests/sunpeak/mcp-e2e/pins.test.ts b/tests/sunpeak/mcp-e2e/pins.test.ts index d7a8a6e1..c17f8266 100644 --- a/tests/sunpeak/mcp-e2e/pins.test.ts +++ b/tests/sunpeak/mcp-e2e/pins.test.ts @@ -12,7 +12,7 @@ test.describe.configure({ mode: 'serial' }); /** * Pins domain tools (pins_add / pins_list / pins_status / pins_rm) driven - * through the host-discovery contract: every call goes through invoke_tool + * through the host-discovery contract: every call goes through the typed invoke dispatchers * (the progressive-disclosure meta-tool) with { name, args }, never by * calling the direct tool name. pins_add/list/status/rm are directly curated * onto tools/list, but this suite deliberately routes them through invoke to @@ -42,13 +42,13 @@ test.describe.configure({ mode: 'serial' }); * RequestID field), and the fake derives it as "req-". * - pins_status takes a `cid` (it looks a pin up by CID and returns its * status), NOT `request_id`. So the round-trip that proves the full - * invoke_tool -> SDK -> HTTP -> fake chain is pins_add(cid) -> + * invoke dispatchers -> SDK -> HTTP -> fake chain is pins_add(cid) -> * pins_status(cid). We still capture the request_id from pins_add (per the * spec) so it is available, but the tool resolves by CID. * - pins_rm is SafetyDestructive. The MCP dispatch layer refuses destructive * ops invoked by a model actor with a needs_human confirmation handoff * (Reason=confirmation) BEFORE the handler runs — unconditionally, even - * when `confirm:true` is passed. So through invoke_tool pins_rm cannot + * when `confirm:true` is passed. So through the invoke tool pins_rm cannot * actually delete; it always returns the confirmation hand-off. This test * locks that gate and asserts the pin survives. */ @@ -129,7 +129,7 @@ test('pins_list now contains the added pin', async ({ mcp }) => { test('pins_status resolves the added pin (round-trip)', async ({ mcp }) => { // Round-trip: the pin created by pins_add above must be resolvable back - // through the full invoke_tool -> SDK -> HTTP -> fake chain. pins_status + // through the full invoke dispatchers -> SDK -> HTTP -> fake chain. pins_status // resolves by `cid` (its catalog contract), NOT by `request_id`. // // The fake's GET /pins (internal/mcptest/ipfs/pins.go GetPins) honors the diff --git a/tests/sunpeak/mcp-e2e/pins2.test.ts b/tests/sunpeak/mcp-e2e/pins2.test.ts index 811664fd..6bc5fc16 100644 --- a/tests/sunpeak/mcp-e2e/pins2.test.ts +++ b/tests/sunpeak/mcp-e2e/pins2.test.ts @@ -12,7 +12,7 @@ test.describe.configure({ mode: 'serial' }); /** * Pins domain tools for pins_update + list filtering, driven through the - * host-discovery contract: every call goes through invoke_tool (the + * host-discovery contract: every call goes through the typed invoke dispatchers (the * progressive-disclosure meta-tool) with { name, args }, never by calling the * direct tool name. * diff --git a/tests/sunpeak/mcp-e2e/real-tools.test.ts b/tests/sunpeak/mcp-e2e/real-tools.test.ts index d5294c1b..9a5fea89 100644 --- a/tests/sunpeak/mcp-e2e/real-tools.test.ts +++ b/tests/sunpeak/mcp-e2e/real-tools.test.ts @@ -24,7 +24,7 @@ test('account_info returns the seeded account, not an auth error', async ({ mcp // must surface that account rather than an authentication failure. expect(result).toHaveTextContent('e2e@example.com'); - // invoke_tool returns the JSON both as text content and as structuredContent, + // The invoke dispatcher returns the JSON both as text content and as structuredContent, // so assert the structured shape directly. Assert only the field this test's // intent requires (the seeded email) rather than a full record snapshot, so // a schema addition to the account object does not break the test. diff --git a/tests/sunpeak/mcp-e2e/resources.test.ts b/tests/sunpeak/mcp-e2e/resources.test.ts index bfe2f099..111c2b9b 100644 --- a/tests/sunpeak/mcp-e2e/resources.test.ts +++ b/tests/sunpeak/mcp-e2e/resources.test.ts @@ -17,7 +17,7 @@ test.describe.configure({ mode: 'serial' }); * These are MCP RESOURCES (MCP resources/list + resources/read over stdio), * NOT tools — so they are driven through the mcp fixture's protocol * primitives (mcp.listResources() / mcp.readResource(uri)), never through - * invoke_tool (which dispatches catalog tools only). + * the typed invoke tools (which dispatch catalog tools only). * * The pinner resource set (internal/mcp/resources.go ResourceDescriptors): * static (resources/list) templates (resources/templates/list) diff --git a/tests/sunpeak/mcp-e2e/tool-surface.test.ts b/tests/sunpeak/mcp-e2e/tool-surface.test.ts index cbf95071..a523e6f9 100644 --- a/tests/sunpeak/mcp-e2e/tool-surface.test.ts +++ b/tests/sunpeak/mcp-e2e/tool-surface.test.ts @@ -3,8 +3,9 @@ import { test, expect } from 'sunpeak/test'; /** * Progressive-disclosure contract for pinner's public `tools/list` surface. * - * Pinner's MCP server hides the full operation catalog behind three meta-tools - * (search_tools / describe_tool / invoke_tool). The client-visible `tools/list` + * Pinner's MCP server hides the full operation catalog behind the meta-tools + * (search_tools / describe_tool / invoke_read_tool / invoke_write_tool / + * invoke_destructive_tool). The client-visible `tools/list` * advertises ONLY those meta-tools plus the host-curated direct tools * (compiledCuratedToolNames + the custom transport tools that register * DirectVisible=true). @@ -32,9 +33,15 @@ import { test, expect } from 'sunpeak/test'; * provides no signal and is not part of this contract. */ -const META_TOOLS = ['search_tools', 'describe_tool', 'invoke_tool']; +const META_TOOLS = [ + 'search_tools', + 'describe_tool', + 'invoke_read_tool', + 'invoke_write_tool', + 'invoke_destructive_tool', +]; -// Catalog tools that MUST live only behind invoke_tool and never leak into +// Catalog tools that MUST live only behind the typed invoke dispatchers and never leak into // tools/list. Guard rail: a name surfacing in tools/list that is in this list // (or carries one of these prefixes) is an accidental leak. const HIDDEN_BEHIND_INVOKE = [ diff --git a/tests/sunpeak/mcp-e2e/websites.test.ts b/tests/sunpeak/mcp-e2e/websites.test.ts index 9ac573c7..9efd5ed1 100644 --- a/tests/sunpeak/mcp-e2e/websites.test.ts +++ b/tests/sunpeak/mcp-e2e/websites.test.ts @@ -12,7 +12,7 @@ test.describe.configure({ mode: 'serial' }); /** * Website tools (websites_* / websites_domains_*) driven through the - * host-discovery contract: every call goes through invoke_tool with + * host-discovery contract: every call goes through the typed invoke dispatchers with * { name, args }, never by calling the direct tool name. * * CI-PENDING: this file is verified in CI (it drives tools through the real diff --git a/tests/sunpeak/mcp-e2e/wizard.test.ts b/tests/sunpeak/mcp-e2e/wizard.test.ts index e3c33466..c579422b 100644 --- a/tests/sunpeak/mcp-e2e/wizard.test.ts +++ b/tests/sunpeak/mcp-e2e/wizard.test.ts @@ -11,7 +11,7 @@ test.describe.configure({ mode: 'serial' }); /** * Wizard FSM session lifecycle (websites_*), driven through the - * host-discovery contract: every call goes through invoke_tool with + * host-discovery contract: every call goes through the typed invoke dispatchers with * { name, args }, never by calling the direct tool name. * * CONTRACT (from internal/mcp/wizard/wizard.go, marshalWizardResponse at diff --git a/tests/sunpeak/tests/protocol-surface.test.ts b/tests/sunpeak/tests/protocol-surface.test.ts index 6d2e3096..e82e6cfb 100644 --- a/tests/sunpeak/tests/protocol-surface.test.ts +++ b/tests/sunpeak/tests/protocol-surface.test.ts @@ -15,14 +15,20 @@ test('server exposes the progressive-disclosure tool surface', async ({ mcp }) = const tools = await mcp.listTools(); const names = tools.map((t) => t.name); - // The three always-visible adapter tools are the discovery entry points. - for (const name of ['search_tools', 'describe_tool', 'invoke_tool']) { + // The always-visible adapter tools are the discovery entry points. + for (const name of [ + 'search_tools', + 'describe_tool', + 'invoke_read_tool', + 'invoke_write_tool', + 'invoke_destructive_tool', + ]) { expect(names).toContain(name); } - // The catalog surface is larger than the always-visible trio (auth, vault, - // upload, account ops, etc.). - expect(tools.length).toBeGreaterThan(3); + // The catalog surface is larger than the always-visible meta set (auth, + // vault, upload, account ops, etc.). + expect(tools.length).toBeGreaterThan(5); }); test('every advertised tool carries a valid JSON schema with required args', async ({ mcp }) => { @@ -55,7 +61,7 @@ test('describe_tool returns a schema for a known catalog tool', async ({ mcp }) }); test('unauthenticated call fails cleanly with a machine-readable error, not a crash', async ({ mcp }) => { - // `invoke_tool` dispatches to a real catalog operation. Without an authed + // The typed invoke dispatchers route to a real catalog operation. Without an authed // session it must fail with isError:true and a useful message rather than // hang or hard-crash the server process. Use a real registered tool // (account_info) so we exercise the auth check, not the unknown-tool path. @@ -65,11 +71,11 @@ test('unauthenticated call fails cleanly with a machine-readable error, not a cr // authenticated, so `account_info` legitimately succeeds and the // unauthenticated-error contract cannot be exercised. Skip in that case // rather than fail the run; CI has no token and still asserts the gate. - const probe = await mcp.callTool('invoke_tool', { name: 'account_info', args: {} }); + const probe = await mcp.callTool('invoke_read_tool', { name: 'account_info', args: {} }); if (!probe.isError) { test.skip(true, 'environment is already authenticated; cannot exercise the unauthenticated path'); } - const result = await mcp.callTool('invoke_tool', { name: 'account_info', args: {} }); + const result = await mcp.callTool('invoke_read_tool', { name: 'account_info', args: {} }); expect(result.isError).toBe(true); const text = result.content?.map((c) => c.text ?? '').join('') ?? ''; // The actual error is `authentication failed: not authenticated: no auth From 65453bdb50e4ecfab99a52e80403756844b88fe9 Mon Sep 17 00:00:00 2001 From: Derrick Hammer Date: Wed, 2 Sep 2026 06:55:30 +0000 Subject: [PATCH 2/2] fix(mcp): align invoke dispatcher descriptions with admission rules --- internal/mcp/sdk_official.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/mcp/sdk_official.go b/internal/mcp/sdk_official.go index 7d4191cb..5514c875 100644 --- a/internal/mcp/sdk_official.go +++ b/internal/mcp/sdk_official.go @@ -531,21 +531,21 @@ func registerOfficialInvokeTools(srv *sdk.Server, catalog *ToolCatalog, stdioMod { name: "invoke_read_tool", title: "Invoke a read-only catalog tool", - description: "Execute a read-only catalog tool by name with the given arguments. This is the third step of the discovery workflow: search_tools(name) to find a tool, describe_tool(name) for its input schema (the describe response names the dispatcher to invoke), then invoke_read_tool(name, arguments) when the tool's hints are readOnlyHint=true. The dispatcher refuses non-read-only tools; use invoke_write_tool or invoke_destructive_tool for those. The arguments object is validated against the tool's inputSchema returned by describe_tool.", + description: "Execute a read-only catalog tool by name with the given arguments. This is the third step of the discovery workflow: search_tools(name) to find a tool, describe_tool(name) for its input schema (the describe response names the dispatcher to invoke), then invoke_read_tool(name, arguments) for any tool whose describe response names invoke_read_tool — read-only tools with readOnlyHint=true and no open-world interaction. The dispatcher refuses non-read-only tools; use invoke_write_tool or invoke_destructive_tool for those. The arguments object is validated against the tool's inputSchema returned by describe_tool.", class: invokeClassRead, readOnly: true, }, { name: "invoke_write_tool", title: "Invoke a mutating catalog tool", - description: "Execute a state-mutating (but not destructive) catalog tool by name with the given arguments. This is the third step of the discovery workflow: search_tools(name) to find a tool, describe_tool(name) for its input schema (the describe response names the dispatcher to invoke), then invoke_write_tool(name, arguments) when the tool's hints are readOnlyHint=false and destructiveHint=false. The dispatcher refuses read-only and destructive tools; use invoke_read_tool or invoke_destructive_tool for those. The arguments object is validated against the tool's inputSchema returned by describe_tool.", + description: "Execute a state-mutating (but not destructive, and generally not read-only) catalog tool by name with the given arguments. This is the third step of the discovery workflow: search_tools(name) to find a tool, describe_tool(name) for its input schema (the describe response names the dispatcher to invoke), then invoke_write_tool(name, arguments) for any tool whose describe response names invoke_write_tool — every tool that is neither read-only (invoke_read_tool) nor destructive (invoke_destructive_tool). The dispatcher refuses read-only and destructive tools; use invoke_read_tool or invoke_destructive_tool for those. The arguments object is validated against the tool's inputSchema returned by describe_tool.", class: invokeClassWrite, openWorld: true, }, { name: "invoke_destructive_tool", title: "Invoke a destructive catalog tool", - description: "Execute a destructive (irreversible / deletion) catalog tool by name with the given arguments. This is the third step of the discovery workflow: search_tools(name) to find a tool, describe_tool(name) for its input schema (the describe response names the dispatcher to invoke), then invoke_destructive_tool(name, arguments) when the tool's hints carry destructiveHint=true. Destructive operations additionally require human confirmation (the server returns a needs_human hand-off before running). The dispatcher refuses non-destructive tools; use invoke_read_tool or invoke_write_tool for those. The arguments object is validated against the tool's inputSchema returned by describe_tool.", + description: "Execute a destructive (irreversible / deletion) catalog tool by name with the given arguments. This is the third step of the discovery workflow: search_tools(name) to find a tool, describe_tool(name) for its input schema (the describe response names the dispatcher to invoke), then invoke_destructive_tool(name, arguments) for any tool whose describe response names invoke_destructive_tool — tools whose hints carry destructiveHint=true. Destructive operations additionally require human confirmation (the server returns a needs_human hand-off before running). The dispatcher refuses non-destructive tools; use invoke_read_tool or invoke_write_tool for those. The arguments object is validated against the tool's inputSchema returned by describe_tool.", class: invokeClassDestructive, destructive: true, openWorld: true,