diff --git a/CHANGELOG.md b/CHANGELOG.md index b8a244ec..708b0596 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,36 @@ package versions for a release. ## Unreleased +- **Breaking (MCP): the app's tools are no longer listed as MCP tools.** An agent reaches them + through three built-ins that mirror the CLI: `appduct_list_tools` (one-line signatures and + each tool's policy, with `filter`/`limit`/`offset`, like `appduct tools`), + `appduct_describe_tool` (one tool's full schema, like `appduct tools `) and + `appduct_call_tool` (`{ selector?, name, args?, timeoutMs? }`, like `appduct invoke`). + `tools/list` is now a fixed set of built-ins, so an app with hundreds of tools adds three + definitions to an agent's context, not hundreds. `appduct_list_tools` returns 50 tools at a time + unless given `limit`. `selector` takes a session alias or id; a call is routed by session id, so + it fails with `unknown_session` rather than reaching a new device that took over a departed + device's alias. Unknown parameters are rejected (`invalid_request`). `timeoutMs` can only + shorten the tool's own deadline, since the app stops a tool at its declared timeout; a longer + one, or one outside 1000–600000, is rejected rather than clamped. What goes away: + - Calling an app tool by its own name through `tools/call`. It now returns `tool_not_found`, + pointing at `appduct_list_tools` and `appduct_call_tool`. + - `__` namespacing. With several devices connected, pass `selector` (the + session alias or id) instead. + - `notifications/tools/list_changed`, and the `listChanged` capability. + - MCP-level `outputSchema` enforcement and schema degradation: schemas reach the agent as + data through `appduct_describe_tool`, exactly as registered, whatever their root type. The + React Native SDK no longer warns about non-object output schemas. + - MCP client permission rules that named individual app tools (for example + `mcp__appduct__seed_cart`) no longer match anything; the client's permission now covers + `appduct_call_tool` as a whole, so "always allow" there approves every app tool. To keep a + person approving destructive calls, set `policy.destructive` to `"prompt"` (it covers tools + annotated `destructiveHint: true`). `"prompt"`-policy + consent itself is unchanged: it is asked per call, via elicitation. + - The React Native SDK's input-schema warning now fires only for a root `type` that rules out + an object (`z.string()`, `z.array(...)`), not for unions or intersections of objects. + - `@appduct/shared` no longer exports `isObjectRootedSchema`. + - **Breaking (MCP): `"prompt"`-policy consent is elicitation-only.** The Claude Code-specific fallback is gone: `tools/list` no longer emits `_meta["anthropic/requiresUserInteraction"]`, and the MCP server no longer sends `consent: "client"`. A `"prompt"` tool called from an MCP client @@ -18,8 +48,9 @@ package versions for a release. the elicitation prompt instead; only a client that relied on the flag without supporting elicitation loses access. To fix that, use a client that supports elicitation, or set the tool's policy to `"allow"` in `config.json` — which removes the gate for every caller, including the - CLI — and restart the daemon (`appduct daemon stop`; the next command starts it again), since - `config.json` is read once at daemon start. + CLI — and restart the daemon (`appduct daemon stop`), since `config.json` is read once at daemon + start. The restart disconnects every device, which then has to link again, and a running + `appduct mcp` loses its daemon connection, so restart the MCP server in your client too. - A daemon with this change that receives `consent: "client"` from an older MCP server treats it as no consent, so the call is denied and audited as `no_consent_channel`. - `@appduct/shared`: `ToolsCallParams.consent` and the audit record's `consent` narrow to diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 6c5db965..8ba3fee7 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -461,11 +461,31 @@ belong here: `appduct mcp` starts a **stdio** MCP server (SDK: `@modelcontextprotocol/sdk`) that proxies daemon RPC (auto-spawning the daemon like any client): -- `tools/list` mirrors the live registry. One session → tools under their own names; - several → namespaced `__`. Registry and session changes emit - `notifications/tools/list_changed`, so an agent's tool list tracks the device. -- Tool calls, progress frames, errors (with their `type` preserved), and descriptor - annotations all map through verbatim. Two semantics the MCP surface does add: +- `tools/list` is a fixed set of built-in tools. The app's tools are never listed as MCP + tools of their own; an agent reaches them through three built-ins that mirror the CLI (§10): + `appduct_list_tools` (`appduct tools`: one-line signatures from `renderToolSignature`, each + tool's effective policy, with `filter`/`limit`/`offset` passed through to `tools.list` and + `limit` defaulting to 50), `appduct_describe_tool` (`appduct tools `: the whole + descriptor), and `appduct_call_tool` (`appduct invoke`: `{ selector?, name, args?, timeoutMs? }`). + `timeoutMs` can only shorten the tool's own deadline, since the `tool_call` frame carries no + deadline and the app stops the handler at its declared one (`docs/PROTOCOL.md` §5); a longer value, or one outside + 1000–600000 ms, is rejected rather than clamped. A client cancel that arrives while the consent + prompt is open stops the call before `tools.call`, even if the prompt is then accepted. Each + takes the same `selector` as the CLI (alias or session id) and resolves it with + `sessions.describe` first; every later daemon call for that request — `tools.list`, + `tools.call`, the progress subscription, a cancel — names the session by **id**. The daemon + gives a departed session's alias to the next device of the same model, so routing by alias + could run a call, one the user may already have approved, on a different device; by id it fails + with `unknown_session` instead. Results name the session by alias. Unknown parameters are + rejected with `invalid_request` rather than dropped, and `null` counts as absent. + A registry of hundreds of tools therefore costs a client three tool definitions, and nothing + about the registry or the session set changes `tools/list`: the server advertises no + `listChanged` capability and never sends `notifications/tools/list_changed`. Schemas travel as + data inside a tool result rather than as MCP `Tool.inputSchema`/`outputSchema`, so MCP's + object-rooted rule for those fields no longer applies to app schemas. +- Tool call results, progress frames and errors (with their `type` preserved) map through + verbatim: a JSON object result is returned as `structuredContent` as well as text, any other + value as text only. Two semantics the MCP surface does add: `"prompt"`-policy consent (§12) — one channel, elicitation (issue #10), used whenever the client declared the `elicitation` capability at `initialize`: a `"prompt"`-policy call sends one `elicitation/create` request naming the tool, the session alias, and the call's arguments, and an `action: "accept"` reply becomes @@ -768,7 +788,7 @@ deviations): list of everything that changes the registry entry — `name`, `description`, `timeoutMs` (app-side only, but part of the entry), stringified `annotations`, the exported input/output JSON Schemas, and `enabled` — so a re-render never emits a - `tool_registry_delta` pair or an agent-side `notifications/tools/list_changed`. Schemas + `tool_registry_delta` pair. Schemas are compared by identity first and re-exported only when the identity changed (hoisted/memoized schemas never re-export; an inline `z.object({…})` re-exports once per render and still matches by shape). A schema that exports no JSON Schema (zod 3, plain @@ -821,9 +841,11 @@ deviations): plain-object rule. The `jsonSchema` half of a pair and every converter result are held to that same rule, so the forms cannot diverge in what they will publish. - Separately from all of this, an **input schema should be object-typed at its root** to be - usable over MCP — a root `enum`/`const`/`$ref`/`anyOf` is legal JSON Schema but leaves the - agent with no named arguments (issue #34). This is documented, not enforced. + Separately from all of this, an **input schema has to accept a JSON object**, because + `tools.call`'s `args` always are one: a root `type` that rules an object out can never be + satisfied (issue #34), and the React Native SDK dev-warns about it. A root `anyOf`/`oneOf`/ + `allOf` of objects is callable, though its signature renders as `(...)`. This is warned about, + not enforced. Every way a slot can end up with no shape — a missing exporter, an exporter that throws or returns a non-object, a paired converter that does either — takes the same route: throw in @@ -977,6 +999,7 @@ named-pipe path `\\.\pipe\appduct-` behind the same client API. pin sets; the anchor-CA design is a future option). - Web/browser client (safe no-op stub only). - Multiple endpoint candidates in the bootstrap payload. -- A tool whose `input_schema` is not object-rooted is listed but not usefully callable over MCP, - because MCP tool arguments are always an object (§9). Wrapping such arguments so the tool stays - callable is tracked in [issue #34](https://github.com/callstackincubator/appduct/issues/34). +- A tool whose `input_schema` root `type` rules out an object (`"string"`, `"array"`, ...) is + listed but not callable, because `tools.call`'s `args` are always a JSON object (§5). Wrapping + such arguments so the tool stays callable is tracked in + [issue #34](https://github.com/callstackincubator/appduct/issues/34). diff --git a/docs/PROTOCOL.md b/docs/PROTOCOL.md index eb64b5bd..303fafa3 100644 --- a/docs/PROTOCOL.md +++ b/docs/PROTOCOL.md @@ -260,7 +260,8 @@ the tool, without `input_schema`/`output_schema`, so agents see a shapeless (`{} the app-side SDK throws on that in development rather than letting it ship silently. The daemon never inspects a schema's internals — only that it is a JSON object. -`annotations` map 1:1 to MCP tool annotations and drive the daemon's policy engine +`annotations` are shown to agents as-is (`appduct tools `, `appduct_describe_tool` over +MCP) and drive the daemon's policy engine (`docs/ARCHITECTURE.md` §12): `destructiveHint: true` routes a call through `policy.destructive` instead of `policy.default`. @@ -277,7 +278,8 @@ is snake_case here like every other protocol-defined descriptor field, while the layers. A camelCase key on this descriptor is an unknown extra, not a deadline. It is the app's *explicit* per-tool value only — never an app-wide default such as `defaultToolTimeoutMs`. Older apps omit the field entirely and keep the daemon's 10 s default, so it is safe to add in either direction. It -is a daemon-side scheduling hint and is never emitted on the MCP `Tool` JSON. +is a daemon-side scheduling hint; agents see it through `appduct tools ` and +`appduct_describe_tool`. ## 6. Session state machine @@ -355,7 +357,7 @@ types also establish these details: - `link.create` also accepts `addressOverride` (forces the bootstrap payload's advertised address — used by the emulator/simulator fast path to force `127.0.0.1`). - `tools.call`'s result carries a `callId` alongside `result`, so a caller juggling - several in-flight calls (the MCP server proxying concurrent `tools/call` requests) can + several in-flight calls (the MCP server running concurrent `appduct_call_tool` requests) can match `tool_call_progress`/`tool_call_finished` events back to the call that produced them without guessing from data shape. - `tools.cancel({ selector?, callId, reason? })` sends `tool_cancel` (above) to the diff --git a/docs/SECURITY.md b/docs/SECURITY.md index d468793d..2533723e 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -284,9 +284,10 @@ tool with no purpose outside a local dev loop, say. It just shouldn't be the exa app copies for hardening. **Consequence for agents and E2E flows:** because registration is the app-side allowlist, -`tools/list` legitimately differs per build artifact. A CI testing build may expose a +the tool set legitimately differs per build artifact. A CI testing build may expose a different tool set than a local dev build or a hardened production build. Automated flows -should discover tools via `tools/list` rather than assume a fixed set is always present. +should discover tools (`appduct tools`, or `appduct_list_tools` over MCP) rather than assume +a fixed set is always present. ## Key handling rules @@ -351,7 +352,11 @@ not as the mechanism that keeps a destructive tool out of reach of a hostile one `policy.tools["/"]` overrides) to `"deny"` for anything you don't want an arbitrary caller invoking against a production build. Every `tools.call` — CLI, MCP, and `appduct/client` alike — is evaluated against this before it ever reaches the app; - a denial returns `policy_denied` and never sends a `tool_call` frame. `"prompt"` requires a human gate + a denial returns `policy_denied` and never sends a `tool_call` frame. Over MCP, the client's own + permission prompt covers `appduct_call_tool` as a whole rather than each app tool, so an operator + who "always allows" it has approved every app tool; `policy.destructive: "prompt"` is how to keep + a human approving each call to a tool annotated `destructiveHint: true` (an unannotated tool + falls under `policy.default`). `"prompt"` requires a human gate and fails closed everywhere one can't be guaranteed: today the only implemented gate is an MCP client that declares the `elicitation` capability, which receives an `elicitation/create` prompt for each call; the CLI and every other client are denied outright diff --git a/docs/TOOLS.md b/docs/TOOLS.md index b41da8a6..0fe2b672 100644 --- a/docs/TOOLS.md +++ b/docs/TOOLS.md @@ -20,7 +20,7 @@ An agent can only use a tool it can see the shape of, so every form below except A Standard Schema does not have to be a plain object: arktype's `Type` is callable, and is detected the same way (anything carrying `~standard.validate`). -Whatever form you use, an **input schema must be object-typed at its root** to be callable over MCP — a root `enum`, `const`, `$ref`, or `anyOf` is legal JSON Schema but leaves the agent with no named arguments to pass. +Whatever form you use, the **input schema must accept a JSON object**, because a call's arguments always are one — see [Make the input schema accept an object](#make-the-input-schema-accept-an-object). Appduct has no third-party runtime dependencies and does not bundle a JSON Schema validator, so a raw JSON Schema describes the tool for the agent but never enforces anything. Use a pair when you want both a real shape *and* real validation. @@ -87,7 +87,7 @@ All of these throw a `TypeError` at registration naming what to fix. ## Registration is per mount, not per render -The hook registers once when the component mounts and re-registers only when something that changes the registration itself changed: `name`, `description`, the exported input/output JSON Schemas, `annotations`, `timeoutMs`, or `enabled`. Re-rendering the component — including on every keystroke of some unrelated state — sends nothing over the wire and does not make agents re-fetch `tools/list`. +The hook registers once when the component mounts and re-registers only when something that changes the registration itself changed: `name`, `description`, the exported input/output JSON Schemas, `annotations`, `timeoutMs`, or `enabled`. Re-rendering the component — including on every keystroke of some unrelated state — sends nothing over the wire. **Your handler is always fresh.** The hook registers a stable wrapper that forwards to the handler from the latest render, so a handler that closes over component state sees the current value on the next call without being re-registered and without `useRef` workarounds: @@ -113,29 +113,13 @@ Because exportable schemas are compared by their *exported* JSON Schema, the reg **`deps` is an optional, advanced override.** Passing it replaces the derived key entirely with `useEffect`'s own semantics (`enabled` is still appended), which is occasionally useful — for example, forcing a re-registration on something the descriptor doesn't capture. Most call sites should simply omit it. Pass it consistently if you pass it at all: alternating between passing `deps` and omitting it changes the dependency-array length between renders, which React warns about, exactly as it does for a hand-written `useEffect`. -## Keep both schemas object-rooted +## Make the input schema accept an object -MCP's tool wire shape requires `inputSchema.type` and `outputSchema.type` to be the literal `"object"`, so `z.object({ ... })` (also `.passthrough()`/`z.looseObject(...)` and `z.record(...)`) is the only shape that survives to an agent intact. Anything else cannot be represented: +A tool call always passes its arguments as a JSON object. An `inputSchema` whose root type is something else — `z.string()`, `z.number()`, `z.array(...)` — can never be satisfied, and registering one logs a dev warning naming the tool. Wrap the value instead: `inputSchema: z.object({ sku: z.string() })` rather than `z.string()`. -| Construct | Exports as | Object-rooted? | -| --- | --- | --- | -| `z.object({ ... })`, `.passthrough()`, `z.record(...)` | `type: "object"` | yes | -| `z.array(...)` | `type: "array"` | no | -| `z.string()`, `z.number()`, `z.boolean()`, `z.null()` | `type: "string"` etc. | no | -| `z.union([...])`, `z.object(...).nullable()` | `anyOf` | no — no root `type` at all | -| `z.discriminatedUnion(...)` | `oneOf` | no, even when every branch is an object | -| `z.intersection(a, b)` | `allOf` | no, even when both sides are objects | +Unions and intersections of objects work: `z.union([...])`, `z.discriminatedUnion(...)` and `z.intersection(a, b)` export with no root `type`, and an object argument can still match one of their branches. The one-line signature in `appduct tools` shows their arguments as `(...)`, though, so an agent has to read the full schema (`appduct tools `, or `appduct_describe_tool` over MCP) before it can call them. A single `z.object(...)` gives agents named arguments straight from the listing. -A client validates the *whole* `tools/list` result, so one such schema would otherwise leave the agent with zero tools from your app. Appduct degrades it instead: - -| Schema | What Appduct does | -| --- | --- | -| `outputSchema` MCP cannot accept | Drops it from `tools/list`. The tool stays listed and callable; its result arrives as JSON text, with no schema describing it (agents still get `structuredContent` when the result happens to be a JSON object, they just have nothing to validate it against). | -| `inputSchema` MCP cannot accept | Replaces it with a permissive empty object schema, so agents cannot see the tool's real arguments. MCP arguments are always an object, so the tool is not usefully callable this way. | - -Both log a dev warning naming the tool when it registers. That warning is a best-effort hint covering the root type only, which is everything zod itself can produce; MCP rejects a little more than that (a `properties` entry that is not an object subschema, such as the `{ a: true }` shorthand, or a `required` that is not an array), and those slip past it. **The authoritative signal is the `appduct mcp:` notice on the MCP server's stderr** — it names the tool and quotes the SDK's own reason for rejecting the schema. - -Wrap the value instead — `outputSchema: z.object({ todos: z.array(z.string()) })` rather than `z.array(z.string())` — and agents get the full shape, described and validated. `appduct invoke`, `--json` output, and the JS client are unaffected either way: they carry the real schema and the raw result. +`outputSchema` has no such limit. A result can be any JSON value, and agents see the schema exactly as you wrote it. ## Long-running tools diff --git a/packages/appduct/README.md b/packages/appduct/README.md index cd96ca9b..e588ca12 100644 --- a/packages/appduct/README.md +++ b/packages/appduct/README.md @@ -166,9 +166,21 @@ An MCP server is usually launched with a working directory you don't control, so } ``` -Once configured, the connected app's tools appear as MCP tools automatically: `tools/list` mirrors the live registry (namespaced `__` when more than one session is active), and `tools/call` proxies straight to the app with progress and errors preserved. +Once configured, an agent reaches the connected app's tools through three built-in tools that work like the CLI: -Four built-in tools cover what an agent can't do through the app's own registry. `appduct_connect` mints a link and, by default, delivers it to whichever `android`/`ios-sim` device it detects — pass `target`/`device` to choose, or `target: "none"` to force the human flow — falling back to a QR code, plus instructions to show it, only when there's nothing to deliver to. Delivering to `android` (chosen or detected) needs `appId`, resolved the same way as `--app-id` (see [Delivering the link to a device](#delivering-the-link-to-a-device)); passing it with `target: "ios-sim"` or `"none"` is an error. `appduct_wait_for_session` then waits for that session to be claimed. `target: "ios-device"` reaches a paired physical iPhone or iPad, with `appId` and the [prerequisites above](#--open-ios-device-experimental) — it's experimental and never auto-detected, so an agent has to ask for it by name. +| Tool | Does what | CLI equivalent | +| --- | --- | --- | +| `appduct_list_tools` | Lists the app's tools as one-line signatures, with each tool's policy. Returns 50 at a time unless given `limit`; takes `filter` and `offset`. | `appduct tools` | +| `appduct_describe_tool` | Shows one tool's full input and output schema. | `appduct tools ` | +| `appduct_call_tool` | Calls a tool by `name` with `args`, with progress and errors preserved. | `appduct invoke` | + +The app's tools don't show up as MCP tools of their own. An app with hundreds of tools still adds only these three to the agent's tool list, and that list doesn't change when tools register or a device connects. With more than one device connected, each of the three needs `selector`: the session alias or id from `appduct ls`. + +Your MCP client asks permission for `appduct_call_tool` as a single tool, so choosing "always allow" there approves every tool the app registers, destructive ones included. To keep a person approving those calls, set `policy.destructive` to `"prompt"` in the state directory's `config.json`. Each call to a tool marked `destructiveHint` then shows an approval prompt in clients that support it, and is denied in clients that don't. A destructive tool without that annotation falls under `policy.default` instead. + +Set this before you connect: the daemon reads `config.json` only when it starts, so changing it later takes `appduct daemon stop`. That disconnects every device, which then has to link again, and leaves a running `appduct mcp` without a daemon, so restart the MCP server in your client as well. + +Four more built-in tools cover what the app's own tools can't. `appduct_connect` mints a link and, by default, delivers it to whichever `android`/`ios-sim` device it detects — pass `target`/`device` to choose, or `target: "none"` to force the human flow — falling back to a QR code, plus instructions to show it, only when there's nothing to deliver to. Delivering to `android` (chosen or detected) needs `appId`, resolved the same way as `--app-id` (see [Delivering the link to a device](#delivering-the-link-to-a-device)); passing it with `target: "ios-sim"` or `"none"` is an error. `appduct_wait_for_session` then waits for that session to be claimed. `target: "ios-device"` reaches a paired physical iPhone or iPad, with `appId` and the [prerequisites above](#--open-ios-device-experimental) — it's experimental and never auto-detected, so an agent has to ask for it by name. The other two give an agent a pull surface over `postEvent()`-pushed app events: `appduct_events` drains everything retained since a cursor, and `appduct_wait_for_event` blocks for a matching event (checking what's already retained before waiting live), rejecting with `tool_timeout` if none arrives in time. diff --git a/packages/appduct/src/__tests__/__snapshots__/mcp-server.test.ts.snap b/packages/appduct/src/__tests__/__snapshots__/mcp-server.test.ts.snap index 769bda1e..560efe2a 100644 --- a/packages/appduct/src/__tests__/__snapshots__/mcp-server.test.ts.snap +++ b/packages/appduct/src/__tests__/__snapshots__/mcp-server.test.ts.snap @@ -1,9 +1,37 @@ // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html -exports[`mcp: tools/list and tools/call > the generated MCP tool list (built-ins + one proxied tool) matches the locked mapping 1`] = ` +exports[`mcp: tools/list > the built-in tool shapes match the locked snapshot 1`] = ` [ { - "annotations": undefined, + "inputSchema": { + "additionalProperties": false, + "properties": { + "args": { + "type": "object", + }, + "name": { + "minLength": 1, + "type": "string", + }, + "selector": { + "description": "Session alias or id. Omit to target the sole active/suspended session.", + "minLength": 1, + "type": "string", + }, + "timeoutMs": { + "maximum": 600000, + "minimum": 1000, + "type": "integer", + }, + }, + "required": [ + "name", + ], + "type": "object", + }, + "name": "appduct_call_tool", + }, + { "inputSchema": { "additionalProperties": false, "properties": { @@ -33,10 +61,29 @@ exports[`mcp: tools/list and tools/call > the generated MCP tool list (built-ins "type": "object", }, "name": "appduct_connect", - "outputSchema": undefined, }, { - "annotations": undefined, + "inputSchema": { + "additionalProperties": false, + "properties": { + "name": { + "minLength": 1, + "type": "string", + }, + "selector": { + "description": "Session alias or id. Omit to target the sole active/suspended session.", + "minLength": 1, + "type": "string", + }, + }, + "required": [ + "name", + ], + "type": "object", + }, + "name": "appduct_describe_tool", + }, + { "inputSchema": { "additionalProperties": false, "properties": { @@ -76,10 +123,34 @@ exports[`mcp: tools/list and tools/call > the generated MCP tool list (built-ins "type": "object", }, "name": "appduct_events", - "outputSchema": undefined, }, { - "annotations": undefined, + "inputSchema": { + "additionalProperties": false, + "properties": { + "filter": { + "maxLength": 256, + "type": "string", + }, + "limit": { + "exclusiveMinimum": 0, + "type": "integer", + }, + "offset": { + "minimum": 0, + "type": "integer", + }, + "selector": { + "description": "Session alias or id. Omit to target the sole active/suspended session.", + "minLength": 1, + "type": "string", + }, + }, + "type": "object", + }, + "name": "appduct_list_tools", + }, + { "inputSchema": { "additionalProperties": false, "properties": { @@ -115,10 +186,8 @@ exports[`mcp: tools/list and tools/call > the generated MCP tool list (built-ins "type": "object", }, "name": "appduct_wait_for_event", - "outputSchema": undefined, }, { - "annotations": undefined, "inputSchema": { "additionalProperties": false, "properties": { @@ -136,20 +205,6 @@ exports[`mcp: tools/list and tools/call > the generated MCP tool list (built-ins "type": "object", }, "name": "appduct_wait_for_session", - "outputSchema": undefined, - }, - { - "annotations": undefined, - "inputSchema": { - "properties": { - "text": { - "type": "string", - }, - }, - "type": "object", - }, - "name": "echo", - "outputSchema": undefined, }, ] `; diff --git a/packages/appduct/src/__tests__/call-timeouts.test.ts b/packages/appduct/src/__tests__/call-timeouts.test.ts index d53ca86b..cfd31674 100644 --- a/packages/appduct/src/__tests__/call-timeouts.test.ts +++ b/packages/appduct/src/__tests__/call-timeouts.test.ts @@ -16,8 +16,6 @@ import { MAX_CALL_TIMEOUT_MS, MIN_CALL_TIMEOUT_MS, } from "../daemon/calls.js"; -import { createMcpToolMapper } from "../mcp/tool-mapping.js"; -import { namespacedToolsSnapshotKey } from "../mcp/tool-namespace.js"; describe("clampTimeout", () => { test("falls back to the daemon default when unset or not finite", () => { @@ -85,52 +83,3 @@ describe("callers that do not know the effective deadline", () => { expect(transportTimeoutForToolCall(500)).toBe(MIN_CALL_TIMEOUT_MS + CALL_TRANSPORT_TIMEOUT_SLACK_MS); }); }); - -const namespacedTool = (timeoutMs?: number) => ({ - mcpName: "slow-login", - selector: "pixel-8", - descriptor: { - name: "slow-login", - description: "Signs in.", - ...(timeoutMs ? { timeout_ms: timeoutMs } : {}), - }, - policy: "allow" as const, -}); - -describe("toMcpTool", () => { - const toMcpTool = createMcpToolMapper(() => {}); - - test("never emits a timeout on the MCP tool, even for a tool that declares one", () => { - const mapped = toMcpTool(namespacedTool(60_000)); - - // The deadline is a daemon-side scheduling hint, not part of the MCP `Tool` contract. This - // guards against a future refactor swapping the explicit field mapping for a spread — under - // either spelling. - expect("timeout_ms" in mapped).toBe(false); - expect("timeoutMs" in mapped).toBe(false); - expect(Object.keys(mapped).sort()).toEqual(["description", "inputSchema", "name"]); - }); - - test("maps a tool that declares one identically to a tool that does not", () => { - expect(toMcpTool(namespacedTool(60_000))).toEqual(toMcpTool(namespacedTool())); - }); -}); - -describe("namespacedToolsSnapshotKey", () => { - test("ignores the timeout, so a timeout-only re-registration fires no list_changed", () => { - // The key exists to decide whether to tell an MCP client its tool list moved. Since the - // deadline never reaches the `Tool` JSON, changing only that leaves the client's view - // identical — firing `list_changed` would just make it re-fetch the same list. - expect(namespacedToolsSnapshotKey([namespacedTool(60_000)])).toBe( - namespacedToolsSnapshotKey([namespacedTool(20_000)]), - ); - expect(namespacedToolsSnapshotKey([namespacedTool(60_000)])).toBe( - namespacedToolsSnapshotKey([namespacedTool()]), - ); - }); - - test("still reacts to a change a client can actually see", () => { - const renamed = { ...namespacedTool(60_000), mcpName: "slow-login-2" }; - expect(namespacedToolsSnapshotKey([namespacedTool(60_000)])).not.toBe(namespacedToolsSnapshotKey([renamed])); - }); -}); diff --git a/packages/appduct/src/__tests__/e2e/mcp.e2e.test.ts b/packages/appduct/src/__tests__/e2e/mcp.e2e.test.ts index 76edf5f0..0f720072 100644 --- a/packages/appduct/src/__tests__/e2e/mcp.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/mcp.e2e.test.ts @@ -1,7 +1,7 @@ /** * E2E scenario: MCP. An MCP client over stdio against a real `appduct mcp` *subprocess* * (consolidating `mcp-server.integration.test.ts`'s in-process coverage at the subprocess level): - * list/call/list_changed with the fake app. + * listing, describing and calling the fake app's tools through the built-ins. */ import { afterEach, describe, expect, test } from "vitest"; @@ -26,20 +26,9 @@ import { afterEach(cleanupAfterEach); -const BUILTIN_TOOL_NAMES = new Set([ - "appduct_connect", - "appduct_wait_for_session", - "appduct_events", - "appduct_wait_for_event", -]); - -const withoutBuiltinTools = (tools: T[]): T[] => { - return tools.filter((tool) => !BUILTIN_TOOL_NAMES.has(tool.name)); -}; - describe("e2e: mcp (real stdio subprocess)", () => { test( - "tools/list, tools/call, and list_changed against a real `appduct mcp` subprocess", + "list, describe and call app tools through the built-ins against a real `appduct mcp` subprocess", async () => { const { stateDir } = await makeTempStateDir({ scheme: "appduct-mcp-e2e" }); // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back @@ -80,42 +69,78 @@ describe("e2e: mcp (real stdio subprocess)", () => { await client.connect(transport); const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - const proxiedTools = withoutBuiltinTools(listed.tools); - expect(proxiedTools).toHaveLength(1); - expect(proxiedTools[0]!.name).toBe("echo"); - expect(proxiedTools[0]!.inputSchema).toEqual({ type: "object", properties: { text: { type: "string" } } }); + const listedNames = listed.tools.map((tool) => tool.name); + expect(listedNames).toContain("appduct_list_tools"); + expect(listedNames).not.toContain("echo"); + + const appTools = await client.request( + { method: "tools/call", params: { name: "appduct_list_tools", arguments: {} } }, + CallToolResultSchema, + ); + expect(appTools.structuredContent).toEqual({ + session: alias, + total: 1, + limit: 50, + tools: [{ name: "echo", signature: "echo(text?: string)", summary: "Echoes its input.", policy: "allow" }], + }); + + const described = await client.request( + { method: "tools/call", params: { name: "appduct_describe_tool", arguments: { name: "echo" } } }, + CallToolResultSchema, + ); + expect(described.structuredContent).toMatchObject({ + name: "echo", + input_schema: { type: "object", properties: { text: { type: "string" } } }, + }); app.answerCalls((call) => ({ result: { echoed: (call.args as Record).text } })); const called = await client.request( - { method: "tools/call", params: { name: "echo", arguments: { text: "hello-mcp" } } }, + { + method: "tools/call", + params: { name: "appduct_call_tool", arguments: { name: "echo", args: { text: "hello-mcp" } } }, + }, CallToolResultSchema, ); expect(called.isError).not.toBe(true); expect(called.structuredContent).toEqual({ echoed: "hello-mcp" }); - // A second device connecting flips namespacing and fires list_changed. + // A second device connecting changes nothing in tools/list; its tools are reached with a + // selector instead. let listChangedCount = 0; client.setNotificationHandler(ToolListChangedNotificationSchema, () => { listChangedCount += 1; }); + const secondEvents = await subscribeToEvents(stateDir); + const secondToolsChanged = secondEvents.waitFor("tools_changed"); const secondLink = await mintLink(stateDir); const secondApp = new FakeAppClient(port, pinnedKeys); const secondAck = await secondApp.claim(secondLink, { model: "iPhone 15" }); - secondApp.registerTools([{ name: "echo" }]); - - const deadline = Date.now() + 5000; - while (listChangedCount === 0 && Date.now() < deadline) { - await new Promise((resolve) => setTimeout(resolve, 25)); - } - expect(listChangedCount).toBeGreaterThan(0); - - const namespacedListing = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - const namespacedNames = withoutBuiltinTools(namespacedListing.tools) - .map((tool) => tool.name) - .sort(); - expect(namespacedNames).toEqual([`${alias}__echo`, `${secondAck.alias}__echo`].sort()); + secondApp.registerTools([{ name: "whoami", description: "Names the device." }]); + await secondToolsChanged; + secondEvents.close(); + secondApp.answerCalls(() => ({ result: { device: "iphone" } })); + + const relisted = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); + expect(relisted.tools.map((tool) => tool.name)).toEqual(listedNames); + expect(listChangedCount).toBe(0); + + const ambiguous = await client.request( + { method: "tools/call", params: { name: "appduct_list_tools", arguments: {} } }, + CallToolResultSchema, + ); + expect(ambiguous.isError).toBe(true); + expect((ambiguous.content[0] as { text: string }).text).toContain("ambiguous_session"); + + const secondCall = await client.request( + { + method: "tools/call", + params: { name: "appduct_call_tool", arguments: { selector: secondAck.alias, name: "whoami" } }, + }, + CallToolResultSchema, + ); + expect(secondCall.structuredContent).toEqual({ device: "iphone" }); app.close(); secondApp.close(); diff --git a/packages/appduct/src/__tests__/mcp-daemon-fake.ts b/packages/appduct/src/__tests__/mcp-daemon-fake.ts index 87d60de2..375b078a 100644 --- a/packages/appduct/src/__tests__/mcp-daemon-fake.ts +++ b/packages/appduct/src/__tests__/mcp-daemon-fake.ts @@ -3,19 +3,18 @@ * are about the MCP server and nothing else. * * `createMcpServer` talks to the daemon only through `DaemonStream` (`rpc/client.ts`), which is - * four functions: `call`, `onNotification`, `onClose`, `close`. Name mapping, output-schema - * degradation, `__` namespacing and `list_changed` are decided entirely from what - * comes back over those four — a real daemon adds a pidfile, a self-signed certificate, a wss - * listener and a scripted app on a WebSocket, none of which any of those behaviours depends on, - * and all of which can fail on their own. The cases that genuinely exercise the real transport - * (progress correlation over a second stream, cancellation, real policy denial) still run against - * a real daemon in `mcp-server.integration.test.ts`. + * four functions: `call`, `onNotification`, `onClose`, `close`. The built-in tools' behaviour is + * decided entirely from what comes back over those four — a real daemon adds a pidfile, a + * self-signed certificate, a wss listener and a scripted app on a WebSocket, none of which that + * behaviour depends on, and all of which can fail on their own. The cases that genuinely exercise + * the real transport (progress correlation over a second stream, cancellation, real policy denial) + * still run against a real daemon in `mcp-server.integration.test.ts`. * - * This fake answers the four methods the server actually calls — `sessions.list`, `tools.list`, - * `tools.call`, `events.subscribe` — and can push `event` notifications, which is how - * `list_changed` is driven. Anything else throws, loudly, rather than returning a plausible - * nothing: a silently-answered method the server did not expect would make a test pass for the - * wrong reason. + * This fake answers the methods the server actually calls — `sessions.list`, `sessions.describe`, + * `tools.list`, `tools.call`, `events.subscribe` — resolving selectors by alias or session id with + * the daemon's own rules, and can push `event` notifications. It does not validate params the way + * the daemon does. Anything else throws, loudly, rather than returning a plausible nothing: a + * silently-answered method the server did not expect would make a test pass for the wrong reason. */ import { @@ -87,6 +86,33 @@ export const createFakeDaemon = (): FakeDaemon => { } }; + /** The daemon's selector rules (`daemon/sessions.ts`'s `resolveSession`): an alias or session + * id, or with none given, the sole live session. */ + const resolveSession = (selector: string | undefined): SessionSummary => { + if (selector !== undefined) { + const match = sessions.find((session) => session.sessionId === selector || session.alias === selector); + + if (!match) { + throw toolError("unknown_session", `No session matches "${selector}".`); + } + + return match; + } + + if (sessions.length === 0) { + throw toolError("no_session", "No active or suspended session, and none was specified."); + } + + if (sessions.length > 1) { + throw toolError( + "ambiguous_session", + `Multiple sessions are live (${sessions.map((session) => session.alias).join(", ")}); specify a selector.`, + ); + } + + return sessions[0]!; + }; + const addSession = (options: FakeSessionOptions): FakeSession => { const sessionId = options.sessionId ?? `session-${options.alias}`; const summary: SessionSummary = { @@ -148,21 +174,36 @@ export const createFakeDaemon = (): FakeDaemon => { return sessions.map((session) => ({ ...session })) as TResult; } - if (method === RPC_METHODS.toolsList) { + if (method === RPC_METHODS.sessionsDescribe) { const selector = (params as { selector?: string } | undefined)?.selector; - const entries = selector === undefined ? undefined : toolsByAlias.get(selector); + return { ...resolveSession(selector) } as TResult; + } - if (!entries) { - throw toolError("unknown_session", `No session matches "${selector}".`); - } + if (method === RPC_METHODS.toolsList) { + const { selector, filter, limit, offset } = (params ?? {}) as { + selector?: string; + filter?: string; + limit?: number; + offset?: number; + }; + const entries = toolsByAlias.get(resolveSession(selector).alias)!; - // The daemon's `{ tools, total }` shape, sorted by name as the daemon sorts its registry. - // The server asks for the unpaged, unfiltered listing, so `total` is the whole registry. - const tools = entries + // The daemon's `{ tools, total }` shape: sorted by name as the daemon sorts its registry, + // `filter`ed on name and description, `total` counted before paging. + const lowerFilter = filter?.toLowerCase(); + const matching = entries .map((entry) => ({ ...entry })) - .sort((a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0)); - - return { tools, total: tools.length } as TResult; + .sort((a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0)) + .filter( + (entry) => + lowerFilter === undefined || + entry.name.toLowerCase().includes(lowerFilter) || + entry.description.toLowerCase().includes(lowerFilter), + ); + const start = offset ?? 0; + const tools = matching.slice(start, limit === undefined ? undefined : start + limit); + + return { tools, total: matching.length } as TResult; } if (method === RPC_METHODS.toolsCall) { @@ -171,7 +212,7 @@ export const createFakeDaemon = (): FakeDaemon => { name: string; args: Record; }; - const handler = selector === undefined ? undefined : handlersByAlias.get(selector)?.get(name); + const handler = handlersByAlias.get(resolveSession(selector).alias)?.get(name); if (!handler) { throw toolError("tool_not_found", `Tool "${name}" is not registered.`); diff --git a/packages/appduct/src/__tests__/mcp-server.integration.test.ts b/packages/appduct/src/__tests__/mcp-server.integration.test.ts index 5d72c67c..32f9b1a7 100644 --- a/packages/appduct/src/__tests__/mcp-server.integration.test.ts +++ b/packages/appduct/src/__tests__/mcp-server.integration.test.ts @@ -8,9 +8,9 @@ * Only what genuinely needs the real transport lives here: a declared `timeout_ms` surviving the * whole round trip, progress correlation over the second daemon stream, cancellation reaching the * app as `tool_cancel`, the `appduct_connect`/`appduct_wait_for_session` delivery paths, the - * events tools, the `appduct://sessions` resource, stdout purity, and version drift. The server's - * own mapping decisions — tool names, output-schema degradation, namespacing, `list_changed` — - * moved to `mcp-server.test.ts`, which runs them against an in-memory daemon. + * events tools, the `appduct://sessions` resource, stdout purity, and version drift. The + * list/describe/call built-ins' own behaviour lives in `mcp-server.test.ts`, which runs them + * against an in-memory daemon. */ import { mkdir, mkdtemp, writeFile } from "node:fs/promises"; @@ -222,20 +222,6 @@ const snapshotTools = async ( await toolsChanged; }; -const BUILTIN_TOOL_NAMES = new Set([ - "appduct_connect", - "appduct_wait_for_session", - "appduct_events", - "appduct_wait_for_event", -]); - -/** Every `tools/list` response always includes the two built-in management tools alongside - * whatever proxied device tools are live; tests that care only about the proxied tools filter - * them out here rather than repeating the same two names everywhere. */ -const withoutBuiltinTools = (tools: T[]): T[] => { - return tools.filter((tool) => !BUILTIN_TOOL_NAMES.has(tool.name)); -}; - /** `xcrun`/`adb` stub reporting an empty machine. `appduct_connect` auto-detects a delivery * target when none is given, so without an injected `exec` these tests would shell out to the real * toolchain and behave differently depending on whether the developer running them happens to have @@ -276,7 +262,7 @@ const connectInMemoryClient = async (handle: McpServerHandle): Promise = return client; }; -describe("mcp: tools/list and tools/call", () => { +describe("mcp: calling app tools", () => { test("a tool declaring a timeoutMs above the daemon default gets it, over MCP, end to end (issue #25)", async () => { const { daemon, stateDir, port } = await startTestDaemon(); const app = await claimApp(daemon, port); @@ -291,15 +277,16 @@ describe("mcp: tools/list and tools/call", () => { const handle = await createMcpHandle(stateDir); const client = await connectInMemoryClient(handle); - // The deadline is a daemon-side scheduling hint, never part of the MCP tool contract. - const listed = await client.request( - { method: "tools/list", params: {} }, - ListToolsResultSchema, + // The app tool is not an MCP tool of its own; its declared deadline is visible to an agent + // through appduct_describe_tool, and is what appduct_call_tool runs it under. + const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); + expect(listed.tools.map((tool) => tool.name)).not.toContain("slow-login"); + + const described = await client.request( + { method: "tools/call", params: { name: "appduct_describe_tool", arguments: { name: "slow-login" } } }, + CallToolResultSchema, ); - const proxied = withoutBuiltinTools(listed.tools)[0]!; - expect(proxied.name).toBe("slow-login"); - expect("timeout_ms" in proxied).toBe(false); - expect("timeoutMs" in proxied).toBe(false); + expect(described.structuredContent).toMatchObject({ name: "slow-login", timeout_ms: 20_000 }); app.socket.on("message", (data) => { const msg = JSON.parse(data.toString("utf8")) as Record; @@ -319,7 +306,7 @@ describe("mcp: tools/list and tools/call", () => { }); const called = await client.request( - { method: "tools/call", params: { name: "slow-login", arguments: {} } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "slow-login", args: {} } } }, CallToolResultSchema, // Above the MCP server's own derived transport timeout (20 s + 5 s slack) so this client's // watchdog can never be what the assertion actually measures. @@ -332,6 +319,32 @@ describe("mcp: tools/list and tools/call", () => { app.socket.close(); }, 45_000); + test("appduct_list_tools passes filter/limit/offset to the real daemon, which rejects bad values as it does for the CLI", async () => { + const { daemon, stateDir, port } = await startTestDaemon(); + const app = await claimApp(daemon, port); + await snapshotTools(daemon, app, [{ name: "cart_add" }, { name: "cart_clear" }, { name: "login" }]); + + const handle = await createMcpHandle(stateDir); + const client = await connectInMemoryClient(handle); + + const page = await client.request( + { method: "tools/call", params: { name: "appduct_list_tools", arguments: { filter: "cart", limit: 1 } } }, + CallToolResultSchema, + ); + expect(page.structuredContent).toMatchObject({ total: 2, limit: 1, tools: [{ name: "cart_add" }] }); + + for (const bad of [{ limit: 0 }, { offset: -1 }, { limit: 1.5 }]) { + const rejected = await client.request( + { method: "tools/call", params: { name: "appduct_list_tools", arguments: bad } }, + CallToolResultSchema, + ); + expect(rejected.isError).toBe(true); + expect((rejected.content[0] as { text: string }).text).toContain("invalid_request"); + } + + app.socket.close(); + }); + test("tool_call_progress frames map to MCP progress notifications when the client sends a progressToken", async () => { const { daemon, stateDir, port } = await startTestDaemon(); const app = await claimApp(daemon, port); @@ -358,7 +371,7 @@ describe("mcp: tools/list and tools/call", () => { const progressUpdates: Array<{ progress: number; message?: string }> = []; const called = await client.request( - { method: "tools/call", params: { name: "slow", arguments: {} } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "slow", args: {} } } }, CallToolResultSchema, { onprogress: (progress) => { @@ -397,10 +410,10 @@ describe("mcp: tools/list and tools/call", () => { const controller = new AbortController(); const callPromise = client .request( - { method: "tools/call", params: { name: "slow", arguments: {} } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "slow", args: {} } } }, CallToolResultSchema, // `onprogress` is what makes the SDK attach a progressToken — required for the server's - // progress-correlation path (mcp/server.ts's callProxiedTool) to ever learn `callId`. + // progress-correlation path (mcp/server.ts's callAppTool) to ever learn `callId`. { onprogress: () => {}, signal: controller.signal }, ) .catch(() => { diff --git a/packages/appduct/src/__tests__/mcp-server.test.ts b/packages/appduct/src/__tests__/mcp-server.test.ts index a1b8a0ea..df64cb6d 100644 --- a/packages/appduct/src/__tests__/mcp-server.test.ts +++ b/packages/appduct/src/__tests__/mcp-server.test.ts @@ -1,17 +1,17 @@ /** * The MCP server's own behaviour (ARCHITECTURE.md §9), against an in-memory daemon - * (`mcp-daemon-fake.ts`) rather than a real one: tool-name mapping, output-schema degradation - * (issue #26), `__` namespacing and `notifications/tools/list_changed`. + * (`mcp-daemon-fake.ts`) rather than a real one: the fixed `tools/list`, and the + * `appduct_list_tools` / `appduct_describe_tool` / `appduct_call_tool` built-ins an agent reaches + * the app's tools through. * - * None of that depends on the transport underneath. These cases used to boot a real daemon — a - * pidfile, a self-signed certificate, a wss listener — and script a fake app over a real - * WebSocket, to assert on a JSON schema the server rewrote on its way out. The server is driven by - * the SDK's own `Client` over `InMemoryTransport` exactly as before, so what the client sees is - * still what a real client would see; only the daemon behind it is a fake. + * None of that depends on the transport underneath. The server is driven by the SDK's own `Client` + * over `InMemoryTransport`, so what the client sees is what a real client would see; only the + * daemon behind it is a fake. * - * `mcp-server.integration.test.ts` keeps everything that genuinely needs the real thing: progress - * correlation over the second daemon stream, cancellation, the `appduct_connect` delivery paths, - * the resource, stdout purity, and version drift. + * `mcp-server.integration.test.ts` keeps everything that genuinely needs the real thing: a + * declared deadline surviving the round trip, progress correlation over the second daemon stream, + * cancellation, the `appduct_connect` delivery paths, the resource, stdout purity, and version + * drift. */ import { afterEach, describe, expect, test } from "vitest"; @@ -20,10 +20,14 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; import { CallToolResultSchema, + ElicitRequestSchema, ListToolsResultSchema, ToolListChangedNotificationSchema, + type CallToolResult, } from "@modelcontextprotocol/sdk/types.js"; +import { RPC_METHODS } from "@appduct/shared"; + import { createMcpServer, type McpServerHandle } from "../mcp/server.js"; import { createFakeDaemon, toolError, type FakeDaemon } from "./mcp-daemon-fake.js"; @@ -35,18 +39,15 @@ afterEach(async () => { } }); -const BUILTIN_TOOL_NAMES = new Set([ +const BUILTIN_TOOL_NAMES = [ + "appduct_call_tool", "appduct_connect", - "appduct_wait_for_session", + "appduct_describe_tool", "appduct_events", + "appduct_list_tools", "appduct_wait_for_event", -]); - -/** Every `tools/list` response always includes the built-in management tools alongside whatever - * proxied device tools are live; tests that care only about the proxied tools filter them here. */ -const withoutBuiltinTools = (tools: T[]): T[] => { - return tools.filter((tool) => !BUILTIN_TOOL_NAMES.has(tool.name)); -}; + "appduct_wait_for_session", +]; /** Starts an MCP server over `daemon` and connects an SDK `Client` to it in-process. `stateDir` is * never touched: nothing here reaches the filesystem, because `openStream` is the only path the @@ -69,215 +70,311 @@ const startServerWithClient = async (daemon: FakeDaemon): Promise => { return client; }; -describe("mcp: tools/list and tools/call", () => { - test("a fake app's registered tools appear in tools/list with schemas and round-trip through tools/call", async () => { +const callBuiltin = async (client: Client, name: string, args: Record): Promise => { + return client.request({ method: "tools/call", params: { name, arguments: args } }, CallToolResultSchema); +}; + +const errorText = (result: CallToolResult): string => { + expect(result.isError).toBe(true); + return (result.content[0] as { text: string }).text; +}; + +describe("mcp: tools/list", () => { + test("lists exactly the fixed built-ins, never the app's own tools", async () => { const daemon = createFakeDaemon(); - const app = daemon.addSession({ alias: "pixel-8" }); - app.setTools([ - { - name: "echo", - description: "Echoes its input.", - input_schema: { type: "object", properties: { text: { type: "string" } } }, - }, - ]); - app.onCall("echo", (args) => ({ echoed: args.text })); + daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "echo" }, { name: "seed_cart" }]); + + const client = await startServerWithClient(daemon); + const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); + expect(listed.tools.map((tool) => tool.name).sort()).toEqual(BUILTIN_TOOL_NAMES); + }); + + test("the built-in tool shapes match the locked snapshot", async () => { + const daemon = createFakeDaemon(); const client = await startServerWithClient(daemon); const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - const proxiedTools = withoutBuiltinTools(listed.tools); - expect(proxiedTools).toHaveLength(1); - expect(proxiedTools[0]!.name).toBe("echo"); - expect(proxiedTools[0]!.description).toBe("Echoes its input."); - expect(proxiedTools[0]!.inputSchema).toEqual({ type: "object", properties: { text: { type: "string" } } }); - - const called = await client.request( - { method: "tools/call", params: { name: "echo", arguments: { text: "hello" } } }, - CallToolResultSchema, - ); + // Schemas only, sorted: free-text descriptions would make this snapshot brittle against + // unrelated wording tweaks. + const shapes = listed.tools + .map((tool) => ({ name: tool.name, inputSchema: tool.inputSchema })) + .sort((a, b) => a.name.localeCompare(b.name)); - expect(called.isError).not.toBe(true); - expect(called.structuredContent).toEqual({ echoed: "hello" }); + expect(shapes).toMatchSnapshot(); }); - test("a non-object output schema does not break tools/list: both tools list and both stay callable", async () => { + test("never fires list_changed or advertises it: sessions and registries changing leave tools/list alone", async () => { const daemon = createFakeDaemon(); - const app = daemon.addSession({ alias: "pixel-8" }); - app.setTools([ + daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "echo" }]); + + const client = await startServerWithClient(daemon); + expect(client.getServerCapabilities()?.tools?.listChanged).not.toBe(true); + + let listChangedCount = 0; + client.setNotificationHandler(ToolListChangedNotificationSchema, () => { + listChangedCount += 1; + }); + + daemon.addSession({ alias: "iphone-15" }).setTools([{ name: "echo" }]); + await new Promise((resolve) => setTimeout(resolve, 100)); + + expect(listChangedCount).toBe(0); + }); + + test("calling an app tool directly by name points the agent at the built-ins", async () => { + const daemon = createFakeDaemon(); + daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "echo" }]); + + const client = await startServerWithClient(daemon); + const text = errorText(await callBuiltin(client, "echo", {})); + + expect(text).toContain("tool_not_found"); + expect(text).toContain("appduct_list_tools"); + expect(text).toContain("appduct_call_tool"); + }); +}); + +describe("mcp: appduct_list_tools", () => { + test("returns one signature, summary and policy per tool, sorted by name, for the sole session", async () => { + const daemon = createFakeDaemon(); + daemon.addSession({ alias: "pixel-8" }).setTools([ { - name: "get-profile", - description: "Returns the profile.", - output_schema: { + name: "seed_cart", + description: "Seeds the cart.\nLonger explanation that stays out of the listing.", + input_schema: { type: "object", - properties: { name: { type: "string" } }, - required: ["name"], - additionalProperties: false, - }, - }, - { - // `z.array(z.string())`: MCP's `Tool.outputSchema.type` is the literal `"object"`, so - // before issue #26 this single entry made the client reject the whole list. - name: "list-todos", - description: "Returns the todos.", - output_schema: { type: "array", items: { type: "string" } }, - }, - { - // `z.union([z.object(...), z.object(...)])`: `anyOf` with no root `type`, so MCP rejects - // it even though every branch — and every result — is an object. - name: "get-status", - description: "Returns one of two shapes.", - output_schema: { - anyOf: [ - { type: "object", properties: { ok: { type: "boolean" } }, required: ["ok"] }, - { type: "object", properties: { error: { type: "string" } }, required: ["error"] }, - ], + properties: { items: { type: "integer" }, sku: { type: "string" } }, + required: ["items"], }, + output_schema: { type: "object", properties: { added: { type: "integer" } }, required: ["added"] }, + annotations: { destructiveHint: true }, + policy: "prompt", }, + { name: "echo", description: "Echoes its input." }, ]); - app.onCall("get-profile", () => ({ name: "Ada" })); - app.onCall("list-todos", () => ["write tests", "ship it"]); - app.onCall("get-status", () => ({ ok: true })); const client = await startServerWithClient(daemon); + const result = await callBuiltin(client, "appduct_list_tools", {}); + + expect(result.isError).not.toBe(true); + expect(result.structuredContent).toEqual({ + session: "pixel-8", + total: 2, + limit: 50, + tools: [ + { name: "echo", signature: "echo()", summary: "Echoes its input.", policy: "allow" }, + { + name: "seed_cart", + signature: "seed_cart(items: int, sku?: string) -> { added: int }", + summary: "Seeds the cart.", + policy: "prompt", + annotations: { destructiveHint: true }, + }, + ], + }); + }); + + test("forwards filter/limit/offset to the daemon, echoes them, and reports total before paging", async () => { + const daemon = createFakeDaemon(); + daemon + .addSession({ alias: "pixel-8" }) + .setTools([{ name: "cart_add" }, { name: "cart_clear" }, { name: "cart_remove" }, { name: "login" }]); - // The SDK's own `listTools`, so the result goes through `ListToolsResultSchema` *and* caches - // the output schemas `callTool` below enforces — exactly what a real client does. - const listed = await client.listTools(); - const proxiedTools = withoutBuiltinTools(listed.tools); - expect(proxiedTools.map((tool) => tool.name).sort()).toEqual(["get-profile", "get-status", "list-todos"]); - - const objectTool = proxiedTools.find((tool) => tool.name === "get-profile")!; - const arrayTool = proxiedTools.find((tool) => tool.name === "list-todos")!; - const unionTool = proxiedTools.find((tool) => tool.name === "get-status")!; - expect(objectTool.outputSchema).toEqual({ - type: "object", - properties: { name: { type: "string" } }, - required: ["name"], - additionalProperties: false, + const client = await startServerWithClient(daemon); + const result = await callBuiltin(client, "appduct_list_tools", { filter: "CART", limit: 1, offset: 1 }); + + expect(result.structuredContent).toMatchObject({ + session: "pixel-8", + total: 3, + filter: "CART", + limit: 1, + offset: 1, + tools: [{ name: "cart_clear" }], + }); + expect(daemon.calls().filter((call) => call.method === RPC_METHODS.toolsList).at(-1)?.params).toEqual({ + selector: "session-pixel-8", + filter: "CART", + limit: 1, + offset: 1, }); - expect(arrayTool.outputSchema).toBeUndefined(); - expect(unionTool.outputSchema).toBeUndefined(); + }); - const profile = await client.callTool({ name: "get-profile", arguments: {} }); - expect(profile.isError).not.toBe(true); - expect(profile.structuredContent).toEqual({ name: "Ada" }); + test("with several sessions, needs a selector, which may be an alias or a session id", async () => { + const daemon = createFakeDaemon(); + daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "android_only" }]); + daemon.addSession({ alias: "iphone-15", sessionId: "session-ios" }).setTools([{ name: "ios_only" }]); - // The dropped schema means no `structuredContent` is required or expected; the value still - // reaches the agent as JSON text. - const todos = await client.callTool({ name: "list-todos", arguments: {} }); - expect(todos.isError).not.toBe(true); - expect(todos.structuredContent).toBeUndefined(); - expect(todos.content).toEqual([{ type: "text", text: JSON.stringify(["write tests", "ship it"]) }]); + const client = await startServerWithClient(daemon); - // A dropped schema never turns a good result into an error: the union tool's result *is* an - // object, so it still travels as `structuredContent` — the client just has no schema to - // validate it against, which is allowed. - const status = await client.callTool({ name: "get-status", arguments: {} }); - expect(status.isError).not.toBe(true); - expect(status.structuredContent).toEqual({ ok: true }); - expect(status.content).toEqual([{ type: "text", text: JSON.stringify({ ok: true }) }]); + const ambiguous = errorText(await callBuiltin(client, "appduct_list_tools", {})); + expect(ambiguous).toContain("ambiguous_session"); + expect(ambiguous).toContain("pixel-8"); + expect(ambiguous).toContain("iphone-15"); + + const byAlias = await callBuiltin(client, "appduct_list_tools", { selector: "pixel-8" }); + expect(byAlias.structuredContent).toMatchObject({ session: "pixel-8", tools: [{ name: "android_only" }] }); + + // A session id resolves to the same session, and the result still names it by alias. + const byId = await callBuiltin(client, "appduct_list_tools", { selector: "session-ios" }); + expect(byId.structuredContent).toMatchObject({ session: "iphone-15", tools: [{ name: "ios_only" }] }); }); - test("an object output schema paired with a non-object result is a tool_output_validation_error, not a client protocol error", async () => { + test("without a limit, returns the first 50 and a total that says how many more there are", async () => { const daemon = createFakeDaemon(); - const app = daemon.addSession({ alias: "pixel-8" }); - app.setTools([ - { name: "lies", description: "Claims an object, returns a number.", output_schema: { type: "object" } }, - ]); - app.onCall("lies", () => 42); + daemon + .addSession({ alias: "pixel-8" }) + .setTools(Array.from({ length: 60 }, (_, index) => ({ name: `tool_${String(index).padStart(2, "0")}` }))); const client = await startServerWithClient(daemon); - await client.listTools(); + const result = await callBuiltin(client, "appduct_list_tools", {}); + const listing = result.structuredContent as { total: number; limit: number; tools: unknown[] }; - // `callTool` (not raw `request`) so the SDK's "has an output schema but did not return - // structured content" guard is live: an `isError` result is the one shape it accepts. - const result = await client.callTool({ name: "lies", arguments: {} }); + expect(listing.total).toBe(60); + expect(listing.limit).toBe(50); + expect(listing.tools).toHaveLength(50); + }); + + test("caps each summary at the CLI's 120 characters, so a long one-line description can't flood a page", async () => { + const daemon = createFakeDaemon(); + daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "verbose", description: "x".repeat(4000) }]); + + const client = await startServerWithClient(daemon); + const result = await callBuiltin(client, "appduct_list_tools", {}); + const [tool] = (result.structuredContent as { tools: Array<{ summary: string }> }).tools; - expect(result.isError).toBe(true); - expect(result.structuredContent).toBeUndefined(); - expect((result.content as Array<{ text: string }>)[0]!.text).toContain("tool_output_validation_error"); - expect((result.content as Array<{ text: string }>)[0]!.text).toContain("a number"); + expect(Array.from(tool!.summary)).toHaveLength(121); + expect(tool!.summary.endsWith("…")).toBe(true); }); - // The issue's own example of a result that breaks the `structuredContent` contract. - test("an object output schema paired with a null result is a tool_output_validation_error", async () => { + test("with no session at all, says so", async () => { + const client = await startServerWithClient(createFakeDaemon()); + expect(errorText(await callBuiltin(client, "appduct_list_tools", {}))).toContain("no_session"); + }); + + test("an empty selector or an unknown key is an invalid request; null optional fields count as absent", async () => { const daemon = createFakeDaemon(); - const app = daemon.addSession({ alias: "pixel-8" }); - app.setTools([ - { name: "nullish", description: "Claims an object, returns null.", output_schema: { type: "object" } }, - ]); - app.onCall("nullish", () => null); + daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "echo" }]); const client = await startServerWithClient(daemon); - await client.listTools(); - const result = await client.callTool({ name: "nullish", arguments: {} }); - - expect(result.isError).toBe(true); - const text = (result.content as Array<{ text: string }>)[0]!.text; - expect(text).toContain("tool_output_validation_error"); - expect(text).toContain("returned null"); - // Never the raw `typeof` wording: "a object" / "a undefined" would read as a bug in the tool. - expect(text).not.toContain("a undefined"); - expect(text).not.toContain("a object"); + expect(errorText(await callBuiltin(client, "appduct_list_tools", { selector: "" }))).toContain("invalid_request"); + + const unknownKey = errorText(await callBuiltin(client, "appduct_list_tools", { search: "echo" })); + expect(unknownKey).toContain("invalid_request"); + expect(unknownKey).toContain('"search"'); + + const nulls = await callBuiltin(client, "appduct_list_tools", { selector: null, filter: null, limit: null, offset: null }); + expect(nulls.structuredContent).toMatchObject({ session: "pixel-8", tools: [{ name: "echo" }] }); }); +}); - test("the generated MCP tool list (built-ins + one proxied tool) matches the locked mapping", async () => { +describe("mcp: appduct_describe_tool", () => { + test("returns the whole descriptor, its signature and policy, with every schema exactly as registered", async () => { const daemon = createFakeDaemon(); - const app = daemon.addSession({ alias: "pixel-8" }); - app.setTools([ + daemon.addSession({ alias: "pixel-8" }).setTools([ { - name: "echo", - description: "Echoes its input.", - input_schema: { type: "object", properties: { text: { type: "string" } } }, + name: "list_todos", + description: "Returns the todos.", + // Not object-rooted: MCP's own `Tool.outputSchema` could never carry this, but as data it + // reaches the agent intact. + output_schema: { type: "array", items: { type: "string" } }, + annotations: { readOnlyHint: true }, + timeout_ms: 30_000, }, ]); const client = await startServerWithClient(daemon); + const result = await callBuiltin(client, "appduct_describe_tool", { name: "list_todos" }); + + expect(result.isError).not.toBe(true); + expect(result.structuredContent).toEqual({ + session: "pixel-8", + signature: "list_todos() -> string[]", + policy: "allow", + name: "list_todos", + description: "Returns the todos.", + output_schema: { type: "array", items: { type: "string" } }, + annotations: { readOnlyHint: true }, + timeout_ms: 30_000, + }); + }); - const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - // Names only, sorted: full descriptors (incl. built-ins' free-text descriptions) would make this - // snapshot brittle against unrelated wording tweaks; the shape/schema mapping is what's locked. - const shapes = listed.tools - .map((tool) => ({ - name: tool.name, - inputSchema: tool.inputSchema, - outputSchema: tool.outputSchema, - annotations: tool.annotations, - })) - .sort((a, b) => a.name.localeCompare(b.name)); + test("an unknown tool is tool_not_found and points at appduct_list_tools", async () => { + const daemon = createFakeDaemon(); + daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "echo" }]); - expect(shapes).toMatchSnapshot(); + const client = await startServerWithClient(daemon); + const text = errorText(await callBuiltin(client, "appduct_describe_tool", { name: "ech" })); + + expect(text).toContain("tool_not_found"); + expect(text).toContain("pixel-8"); + expect(text).toContain("appduct_list_tools"); }); - test("a tool without an input_schema gets a permissive object schema", async () => { + test("a missing name is an invalid request", async () => { const daemon = createFakeDaemon(); - daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "no-schema" }]); + daemon.addSession({ alias: "pixel-8" }); const client = await startServerWithClient(daemon); + expect(errorText(await callBuiltin(client, "appduct_describe_tool", {}))).toContain("invalid_request"); + }); +}); - const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - const proxiedTools = withoutBuiltinTools(listed.tools); - expect(proxiedTools[0]!.inputSchema).toEqual({ type: "object", additionalProperties: true }); +describe("mcp: appduct_call_tool", () => { + test("round-trips args and returns an object result as structuredContent", async () => { + const daemon = createFakeDaemon(); + const app = daemon.addSession({ alias: "pixel-8" }); + app.setTools([{ name: "echo", input_schema: { type: "object", properties: { text: { type: "string" } } } }]); + app.onCall("echo", (args) => ({ echoed: args.text })); + + const client = await startServerWithClient(daemon); + const result = await callBuiltin(client, "appduct_call_tool", { name: "echo", args: { text: "hello" } }); + + expect(result.isError).not.toBe(true); + expect(result.structuredContent).toEqual({ echoed: "hello" }); + expect(result.content).toEqual([{ type: "text", text: JSON.stringify({ echoed: "hello" }) }]); }); - test("annotations map verbatim onto the MCP tool", async () => { + test("a non-object result travels as JSON text, whatever the tool's output schema declares", async () => { const daemon = createFakeDaemon(); - daemon.addSession({ alias: "pixel-8" }).setTools([ - { - name: "destructive-tool", - description: "Deletes things.", - annotations: { destructiveHint: true, readOnlyHint: false }, - }, + const app = daemon.addSession({ alias: "pixel-8" }); + app.setTools([ + { name: "list_todos", output_schema: { type: "array", items: { type: "string" } } }, + // No longer an MCP-level contract violation: the call tool declares no output schema, so a + // client has nothing to enforce, and the app's own validation is the only check. + { name: "claims_object", output_schema: { type: "object" } }, ]); + app.onCall("list_todos", () => ["write tests", "ship it"]); + app.onCall("claims_object", () => 42); const client = await startServerWithClient(daemon); - const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - const proxiedTools = withoutBuiltinTools(listed.tools); - expect(proxiedTools[0]!.annotations).toEqual({ destructiveHint: true, readOnlyHint: false }); + const todos = await callBuiltin(client, "appduct_call_tool", { name: "list_todos" }); + expect(todos.isError).not.toBe(true); + expect(todos.structuredContent).toBeUndefined(); + expect(todos.content).toEqual([{ type: "text", text: JSON.stringify(["write tests", "ship it"]) }]); + + const number = await callBuiltin(client, "appduct_call_tool", { name: "claims_object" }); + expect(number.isError).not.toBe(true); + expect(number.content).toEqual([{ type: "text", text: "42" }]); }); - test("an app tool_error's type and message are preserved in the MCP error content, not thrown as a protocol error", async () => { + test("omitted args reach the tool as an empty object; non-object args are an invalid request", async () => { + const daemon = createFakeDaemon(); + const app = daemon.addSession({ alias: "pixel-8" }); + app.setTools([{ name: "ping" }]); + app.onCall("ping", (args) => ({ received: args })); + + const client = await startServerWithClient(daemon); + + const omitted = await callBuiltin(client, "appduct_call_tool", { name: "ping" }); + expect(omitted.structuredContent).toEqual({ received: {} }); + + const invalid = errorText(await callBuiltin(client, "appduct_call_tool", { name: "ping", args: [1, 2] })); + expect(invalid).toContain("invalid_request"); + }); + + test("an app tool_error's type and message are preserved in the error content, not thrown as a protocol error", async () => { const daemon = createFakeDaemon(); const app = daemon.addSession({ alias: "pixel-8" }); app.setTools([{ name: "boom" }]); @@ -286,61 +383,179 @@ describe("mcp: tools/list and tools/call", () => { }); const client = await startServerWithClient(daemon); + const text = errorText(await callBuiltin(client, "appduct_call_tool", { name: "boom" })); - const result = await client.request( - { method: "tools/call", params: { name: "boom", arguments: {} } }, - CallToolResultSchema, - ); - - expect(result.isError).toBe(true); - const text = (result.content[0] as { text: string }).text; expect(text).toContain("tool_execution_error"); expect(text).toContain("boom failed"); }); - test("calling an unregistered tool returns tool_not_found error content", async () => { + test("an unregistered tool is tool_not_found and never reaches the daemon's tools.call", async () => { const daemon = createFakeDaemon(); + daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "echo" }]); + const client = await startServerWithClient(daemon); + const text = errorText(await callBuiltin(client, "appduct_call_tool", { name: "does-not-exist" })); - const result = await client.request( - { method: "tools/call", params: { name: "does-not-exist", arguments: {} } }, - CallToolResultSchema, + expect(text).toContain("tool_not_found"); + expect(daemon.calls().some((call) => call.method === RPC_METHODS.toolsCall)).toBe(false); + }); + + test("the selector routes the call to that session when several share a tool name", async () => { + const daemon = createFakeDaemon(); + const android = daemon.addSession({ alias: "pixel-8" }); + android.setTools([{ name: "whoami" }]); + android.onCall("whoami", () => ({ platform: "android" })); + const ios = daemon.addSession({ alias: "iphone-15" }); + ios.setTools([{ name: "whoami" }]); + ios.onCall("whoami", () => ({ platform: "ios" })); + + const client = await startServerWithClient(daemon); + + expect(errorText(await callBuiltin(client, "appduct_call_tool", { name: "whoami" }))).toContain( + "ambiguous_session", ); - expect(result.isError).toBe(true); - expect((result.content[0] as { text: string }).text).toContain("tool_not_found"); + const result = await callBuiltin(client, "appduct_call_tool", { selector: "iphone-15", name: "whoami" }); + expect(result.structuredContent).toEqual({ platform: "ios" }); }); -}); -describe("mcp: namespacing and list_changed", () => { - test("a single live session exposes tools under their own names; a second flips to __ and fires list_changed", async () => { + test("runs under the tool's own deadline; timeoutMs can shorten it but never extend it", async () => { const daemon = createFakeDaemon(); - const appA = daemon.addSession({ alias: "pixel-8", deviceModel: "Pixel 8" }); - appA.setTools([{ name: "echo" }]); + const app = daemon.addSession({ alias: "pixel-8" }); + app.setTools([{ name: "slow", timeout_ms: 30_000 }, { name: "undeclared" }]); + app.onCall("slow", () => ({ ok: true })); + app.onCall("undeclared", () => ({ ok: true })); const client = await startServerWithClient(daemon); + const sentTimeouts = () => + daemon + .calls() + .filter((call) => call.method === RPC_METHODS.toolsCall) + .map((call) => (call.params as { timeoutMs?: number }).timeoutMs); + + await callBuiltin(client, "appduct_call_tool", { name: "slow" }); + await callBuiltin(client, "appduct_call_tool", { name: "slow", timeoutMs: 5_000 }); + await callBuiltin(client, "appduct_call_tool", { name: "undeclared" }); + expect(sentTimeouts()).toEqual([30_000, 5_000, 10_000]); + + // The app stops the tool at its own deadline, so a longer one is refused up front instead of + // timing out at the same point anyway. + const longer = errorText(await callBuiltin(client, "appduct_call_tool", { name: "slow", timeoutMs: 45_000 })); + expect(longer).toContain("invalid_request"); + expect(longer).toContain("30000"); + const longerThanDefault = errorText( + await callBuiltin(client, "appduct_call_tool", { name: "undeclared", timeoutMs: 20_000 }), + ); + expect(longerThanDefault).toContain("10000"); + + // Out of range, or not whole milliseconds, is rejected rather than silently clamped. + for (const timeoutMs of [999, 600_001, 1_500.5]) { + const invalid = errorText(await callBuiltin(client, "appduct_call_tool", { name: "slow", timeoutMs })); + expect(invalid).toContain("invalid_request"); + } + expect(sentTimeouts()).toEqual([30_000, 5_000, 10_000]); + }); - const singleSessionListing = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - expect(withoutBuiltinTools(singleSessionListing.tools).map((tool) => tool.name)).toEqual(["echo"]); + test("a call the client cancels while its consent prompt is open never reaches the app, even if the user then accepts", async () => { + const daemon = createFakeDaemon(); + const app = daemon.addSession({ alias: "pixel-8" }); + app.setTools([{ name: "wipe", policy: "prompt" }]); + let ran = 0; + app.onCall("wipe", () => { + ran += 1; + return { wiped: true }; + }); - let listChangedCount = 0; - client.setNotificationHandler(ToolListChangedNotificationSchema, () => { - listChangedCount += 1; + const handle = await createMcpServer({ + stateDir: "/nonexistent-state-dir", + openStream: daemon.openStream, + scheme: "appduct", + env: {}, }); + mcpHandles.push(handle); + const [serverTransport, clientTransport] = InMemoryTransport.createLinkedPair(); + await handle.connect(serverTransport); - const appB = daemon.addSession({ alias: "iphone-15", deviceModel: "iPhone 15" }); - appB.setTools([{ name: "echo" }]); + let promptShown!: () => void; + const prompted = new Promise((resolve) => { + promptShown = resolve; + }); + let answerPrompt!: (answer: { action: "accept" }) => void; + const answer = new Promise<{ action: "accept" }>((resolve) => { + answerPrompt = resolve; + }); - // The daemon event that flips the namespacing (appB's own tools_changed) is pushed - // synchronously; give the MCP server's own async refresh + notify a beat to catch up. - await new Promise((resolve) => setTimeout(resolve, 100)); + const client = new Client({ name: "test-client", version: "0.0.0" }, { capabilities: { elicitation: {} } }); + client.setRequestHandler(ElicitRequestSchema, async () => { + promptShown(); + return answer; + }); + await client.connect(clientTransport); + + const controller = new AbortController(); + const call = client.request( + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "wipe" } } }, + CallToolResultSchema, + { signal: controller.signal }, + ); + call.catch(() => {}); + + await prompted; + controller.abort(); + // Let the cancel notification reach the server before the user answers the stale prompt. + await new Promise((resolve) => setTimeout(resolve, 20)); + answerPrompt({ action: "accept" }); + await expect(call).rejects.toThrow(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + expect(ran).toBe(0); + expect(daemon.calls().some((entry) => entry.method === RPC_METHODS.toolsCall)).toBe(false); + }); - expect(listChangedCount).toBeGreaterThan(0); + test("a misspelled parameter is rejected instead of running the tool without it", async () => { + const daemon = createFakeDaemon(); + const app = daemon.addSession({ alias: "pixel-8" }); + app.setTools([{ name: "echo" }]); + app.onCall("echo", (args) => ({ received: args })); + + const client = await startServerWithClient(daemon); + + for (const misspelled of [{ arguments: { text: "hi" } }, { text: "hi" }, { timeout_ms: 30_000 }]) { + const text = errorText(await callBuiltin(client, "appduct_call_tool", { name: "echo", ...misspelled })); + expect(text).toContain("invalid_request"); + expect(text).toContain("selector, name, args, timeoutMs"); + } + expect(daemon.calls().some((call) => call.method === RPC_METHODS.toolsCall)).toBe(false); - const multiSessionListing = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - const names = withoutBuiltinTools(multiSessionListing.tools) - .map((tool) => tool.name) - .sort(); - expect(names).toEqual([`${appA.alias}__echo`, `${appB.alias}__echo`].sort()); + // null for an optional field is not a misspelling. + const nulls = await callBuiltin(client, "appduct_call_tool", { selector: null, name: "echo", args: null, timeoutMs: null }); + expect(nulls.structuredContent).toEqual({ received: {} }); + }); + + test("a call is routed by session id, so it never lands on a new device that inherited the alias", async () => { + const daemon = createFakeDaemon(); + const original = daemon.addSession({ alias: "pixel-8", sessionId: "sess-old" }); + original.setTools([{ name: "whoami" }]); + original.onCall("whoami", () => ({ ranOn: "sess-old" })); + + const client = await startServerWithClient(daemon); + await callBuiltin(client, "appduct_call_tool", { selector: "pixel-8", name: "whoami" }); + + // Every daemon call after resolving the selector names the session by id. + const routed = daemon + .calls() + .filter((call) => call.method === RPC_METHODS.toolsList || call.method === RPC_METHODS.toolsCall) + .map((call) => (call.params as { selector?: string }).selector); + expect(new Set(routed)).toEqual(new Set(["sess-old"])); + + // The device goes away and a new one of the same model takes over its alias. + daemon.removeSession("pixel-8"); + const replacement = daemon.addSession({ alias: "pixel-8", sessionId: "sess-new" }); + replacement.setTools([{ name: "whoami" }]); + replacement.onCall("whoami", () => ({ ranOn: "sess-new" })); + + // Naming the old session by id fails, instead of reaching the replacement. + const stale = errorText(await callBuiltin(client, "appduct_call_tool", { selector: "sess-old", name: "whoami" })); + expect(stale).toContain("unknown_session"); }); }); diff --git a/packages/appduct/src/__tests__/policy-and-audit.integration.test.ts b/packages/appduct/src/__tests__/policy-and-audit.integration.test.ts index 7880971b..37a4216a 100644 --- a/packages/appduct/src/__tests__/policy-and-audit.integration.test.ts +++ b/packages/appduct/src/__tests__/policy-and-audit.integration.test.ts @@ -382,10 +382,10 @@ describe("policy: prompt without elicitation", () => { const client = await connectClientWithoutElicitation(mcpHandle, { name: "claude-code", version: "2.1.199" }); const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - expect(listed.tools.find((tool) => tool.name === "echo")?._meta).toBeUndefined(); + expect(listed.tools.some((tool) => tool._meta !== undefined)).toBe(false); const result = await client.request( - { method: "tools/call", params: { name: "echo", arguments: {} } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "echo", args: {} } } }, CallToolResultSchema, ); expect(result.isError).toBe(true); @@ -461,7 +461,7 @@ describe("policy: prompt without elicitation", () => { const client = await connectClientWithoutElicitation(mcpHandle); const result = await client.request( - { method: "tools/call", params: { name: "echo", arguments: {} } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "echo", args: {} } } }, CallToolResultSchema, ); expect(result.isError).toBe(true); @@ -496,7 +496,7 @@ describe("policy: prompt without elicitation", () => { app.socket.close(); }); - test('policy "deny" is still denied for an MCP client, and no _meta is emitted for a "deny" tool', async () => { + test('policy "deny" is still denied for an MCP client', async () => { const { daemon, port, stateDir } = await startTestDaemon({ policy: { tools: { "pixel-8/echo": "deny" } } }); const app = await claimApp(daemon, port, "Pixel 8"); await snapshotTools(daemon, app, [{ name: "echo" }]); @@ -510,11 +510,8 @@ describe("policy: prompt without elicitation", () => { mcpHandles.push(mcpHandle); const client = await connectClientWithoutElicitation(mcpHandle); - const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - expect(listed.tools.find((tool) => tool.name === "echo")?._meta).toBeUndefined(); - const result = await client.request( - { method: "tools/call", params: { name: "echo", arguments: {} } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "echo", args: {} } } }, CallToolResultSchema, ); expect(result.isError).toBe(true); @@ -550,11 +547,8 @@ describe("policy: prompt without elicitation", () => { mcpHandles.push(mcpHandle); const client = await connectClientWithoutElicitation(mcpHandle); - const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - expect(listed.tools.find((tool) => tool.name === "echo")?._meta).toBeUndefined(); - const result = await client.request( - { method: "tools/call", params: { name: "echo", arguments: {} } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "echo", args: {} } } }, CallToolResultSchema, ); expect(result.isError).not.toBe(true); @@ -583,7 +577,7 @@ describe("policy: prompt via MCP elicitation (issue #10)", () => { return client; }; - test('tools/list never emits _meta for a "prompt" tool — consent is asked at call time, not flagged at listing time', async () => { + test('appduct_list_tools reports a "prompt" tool\'s policy, and nothing is flagged at listing time — consent is asked at call time', async () => { const { daemon, port, stateDir } = await startTestDaemon({ policy: { tools: { "pixel-8/echo": "prompt" } } }); const app = await claimApp(daemon, port, "Pixel 8"); await snapshotTools(daemon, app, [{ name: "echo" }]); @@ -593,7 +587,13 @@ describe("policy: prompt via MCP elicitation (issue #10)", () => { const client = await connectElicitationClient(mcpHandle, () => ({ action: "accept" })); const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - expect(listed.tools.find((tool) => tool.name === "echo")?._meta).toBeUndefined(); + expect(listed.tools.some((tool) => tool._meta !== undefined)).toBe(false); + + const appTools = await client.request( + { method: "tools/call", params: { name: "appduct_list_tools", arguments: {} } }, + CallToolResultSchema, + ); + expect(appTools.structuredContent).toMatchObject({ tools: [{ name: "echo", policy: "prompt" }] }); app.socket.close(); }); @@ -620,7 +620,7 @@ describe("policy: prompt via MCP elicitation (issue #10)", () => { }); const result = await client.request( - { method: "tools/call", params: { name: "echo", arguments: { text: "hi there" } } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "echo", args: { text: "hi there" } } } }, CallToolResultSchema, ); expect(result.isError).not.toBe(true); @@ -663,7 +663,7 @@ describe("policy: prompt via MCP elicitation (issue #10)", () => { const client = await connectElicitationClient(mcpHandle, () => ({ action: "accept" })); const result = await client.request( - { method: "tools/call", params: { name: "boom", arguments: {} } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "boom", args: {} } } }, CallToolResultSchema, ); expect(result.isError).toBe(true); @@ -693,7 +693,7 @@ describe("policy: prompt via MCP elicitation (issue #10)", () => { const client = await connectElicitationClient(mcpHandle, () => ({ action })); const result = await client.request( - { method: "tools/call", params: { name: "echo", arguments: {} } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "echo", args: {} } } }, CallToolResultSchema, ); expect(result.isError).toBe(true); @@ -735,7 +735,7 @@ describe("policy: prompt via MCP elicitation (issue #10)", () => { await client.connect(clientTransport); const result = await client.request( - { method: "tools/call", params: { name: "echo", arguments: {} } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "echo", args: {} } } }, CallToolResultSchema, ); expect(result.isError).toBe(true); @@ -782,7 +782,7 @@ describe("policy: prompt via MCP elicitation (issue #10)", () => { await client.connect(clientTransport); const result = await client.request( - { method: "tools/call", params: { name: "echo", arguments: {} } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "echo", args: {} } } }, CallToolResultSchema, ); expect(result.isError).toBe(true); @@ -861,7 +861,7 @@ describe("audit: one line per tools.call attempt", () => { await client.connect(clientTransport); const mcpResult = await client.request( - { method: "tools/call", params: { name: "echo", arguments: {} } }, + { method: "tools/call", params: { name: "appduct_call_tool", arguments: { name: "echo", args: {} } } }, CallToolResultSchema, ); expect(mcpResult.isError).not.toBe(true); diff --git a/packages/appduct/src/__tests__/tool-mapping.test.ts b/packages/appduct/src/__tests__/tool-mapping.test.ts deleted file mode 100644 index d0fcf570..00000000 --- a/packages/appduct/src/__tests__/tool-mapping.test.ts +++ /dev/null @@ -1,286 +0,0 @@ -/** - * The MCP tool mapper's schema gate (issue #26). A client validates the whole `tools/list` result - * against the SDK's `ToolSchema`, so a single entry it rejects takes the entire list down. Every - * fixture below is asserted against that same `ToolSchema` in the first describe block, so these - * tests cannot quietly drift from what the SDK actually accepts if the pinned version changes. - */ - -import { describe, expect, test, vi } from "vitest"; - -import { ToolSchema } from "@modelcontextprotocol/sdk/types.js"; - -import type { ToolDescriptor, ToolSchemaDescriptor } from "@appduct/shared"; - -import { createMcpToolMapper, emitsMcpOutputSchema, type McpToolMapper } from "../mcp/tool-mapping.js"; -import type { NamespacedTool } from "../mcp/tool-namespace.js"; - -const EMPTY_OBJECT_SCHEMA = { type: "object", additionalProperties: true }; - -/** What zod v4.4.3's exporter emits, verified against the real zod, by construct. */ -const OBJECT_SCHEMA: ToolSchemaDescriptor = { - type: "object", - properties: { echoed: { type: "string" } }, - required: ["echoed"], - additionalProperties: false, -}; -/** `z.object({}).passthrough()` / `z.looseObject({})`. */ -const PASSTHROUGH_SCHEMA: ToolSchemaDescriptor = { type: "object", properties: {}, additionalProperties: {} }; -/** `z.record(z.string(), z.number())`. */ -const RECORD_SCHEMA: ToolSchemaDescriptor = { - type: "object", - propertyNames: { type: "string" }, - additionalProperties: { type: "number" }, -}; -const ARRAY_SCHEMA: ToolSchemaDescriptor = { type: "array", items: { type: "string" } }; -const STRING_SCHEMA: ToolSchemaDescriptor = { type: "string" }; -const NUMBER_SCHEMA: ToolSchemaDescriptor = { type: "number" }; -const BOOLEAN_SCHEMA: ToolSchemaDescriptor = { type: "boolean" }; -const NULL_SCHEMA: ToolSchemaDescriptor = { type: "null" }; -/** `z.union([z.object(...), z.object(...)])`, and equally `z.object(...).nullable()` — `anyOf`, - * no root `type`. */ -const UNION_SCHEMA: ToolSchemaDescriptor = { anyOf: [{ type: "object" }, { type: "object" }] }; -/** `z.discriminatedUnion(...)` of objects — `oneOf`, still no root `type`. */ -const DISCRIMINATED_UNION_SCHEMA: ToolSchemaDescriptor = { oneOf: [{ type: "object" }, { type: "object" }] }; -/** `z.intersection(z.object(...), z.object(...))` — `allOf`, still no root `type`. */ -const INTERSECTION_SCHEMA: ToolSchemaDescriptor = { allOf: [{ type: "object" }, { type: "object" }] }; - -/** Not zod exports — hand-written or third-party-exporter shapes that are object-*rooted* yet - * still rejected, which is why the gate is the SDK schema rather than a `type === "object"` - * check. */ -const BOOLEAN_SUBSCHEMA: ToolSchemaDescriptor = { type: "object", properties: { a: true } }; -const STRING_SUBSCHEMA: ToolSchemaDescriptor = { type: "object", properties: { a: "string" } }; -const SCALAR_REQUIRED: ToolSchemaDescriptor = { type: "object", required: "name" }; -const NULLABLE_OBJECT_TYPE: ToolSchemaDescriptor = { type: ["object", "null"] }; - -const ACCEPTED: Array<[string, ToolSchemaDescriptor]> = [ - ["object", OBJECT_SCHEMA], - ["passthrough object", PASSTHROUGH_SCHEMA], - ["record", RECORD_SCHEMA], -]; - -const REJECTED: Array<[string, ToolSchemaDescriptor]> = [ - ["array", ARRAY_SCHEMA], - ["string", STRING_SCHEMA], - ["number", NUMBER_SCHEMA], - ["boolean", BOOLEAN_SCHEMA], - ["null", NULL_SCHEMA], - ["union (anyOf)", UNION_SCHEMA], - ["discriminated union of objects (oneOf)", DISCRIMINATED_UNION_SCHEMA], - ["intersection of objects (allOf)", INTERSECTION_SCHEMA], - ["object with a boolean subschema", BOOLEAN_SUBSCHEMA], - ["object with a string subschema", STRING_SUBSCHEMA], - ["object with a scalar required", SCALAR_REQUIRED], - ['type ["object", "null"]', NULLABLE_OBJECT_TYPE], -]; - -const namespacedTool = (descriptor: Partial, selector = "pixel-8"): NamespacedTool => ({ - mcpName: descriptor.name ?? "tool", - selector, - descriptor: { name: "tool", description: "A test tool.", ...descriptor }, - policy: "allow", -}); - -/** A mapper whose notices are captured instead of printed — no console spying, no module state. */ -const mapperWithNotices = (): { map: McpToolMapper; notices: string[] } => { - const notices: string[] = []; - return { map: createMcpToolMapper((message) => notices.push(message)), notices }; -}; - -const map = (descriptor: Partial) => mapperWithNotices().map(namespacedTool(descriptor)); - -describe("the fixtures match what the pinned MCP SDK accepts", () => { - test.each(ACCEPTED)("ToolSchema accepts a %s schema in both slots", (_label, schema) => { - expect(ToolSchema.safeParse({ name: "probe", inputSchema: schema }).success).toBe(true); - expect( - ToolSchema.safeParse({ name: "probe", inputSchema: { type: "object" }, outputSchema: schema }).success, - ).toBe(true); - }); - - test.each(REJECTED)("ToolSchema rejects a %s schema in both slots", (_label, schema) => { - expect(ToolSchema.safeParse({ name: "probe", inputSchema: schema }).success).toBe(false); - expect( - ToolSchema.safeParse({ name: "probe", inputSchema: { type: "object" }, outputSchema: schema }).success, - ).toBe(false); - }); -}); - -describe("outputSchema is emitted only when the SDK would accept it", () => { - test.each(ACCEPTED)("keeps a %s output schema verbatim", (_label, schema) => { - expect(map({ output_schema: schema }).outputSchema).toEqual(schema); - expect(emitsMcpOutputSchema(schema)).toBe(true); - }); - - test.each(REJECTED)("omits a %s output schema", (_label, schema) => { - const mapped = map({ output_schema: schema }); - - expect(mapped).not.toHaveProperty("outputSchema"); - expect(mapped.outputSchema).toBeUndefined(); - expect(emitsMcpOutputSchema(schema)).toBe(false); - }); - - test("the tool itself stays listed, named and described when its output schema is dropped", () => { - const mapped = map({ name: "list-todos", description: "Lists todos.", output_schema: ARRAY_SCHEMA }); - - expect(mapped.name).toBe("list-todos"); - expect(mapped.description).toBe("Lists todos."); - expect(mapped.inputSchema).toEqual(EMPTY_OBJECT_SCHEMA); - }); - - test("omits outputSchema when the descriptor has none", () => { - expect(map({}).outputSchema).toBeUndefined(); - expect(emitsMcpOutputSchema(undefined)).toBe(false); - }); - - test("every emitted tool passes the SDK's own ToolSchema", () => { - for (const [, schema] of [...ACCEPTED, ...REJECTED]) { - const mapped = map({ name: "probe", input_schema: schema, output_schema: schema }); - - expect(ToolSchema.safeParse(mapped).success).toBe(true); - } - }); -}); - -describe("inputSchema always ends up something MCP accepts", () => { - test.each(ACCEPTED)("keeps a %s input schema verbatim", (_label, schema) => { - expect(map({ input_schema: schema }).inputSchema).toEqual(schema); - }); - - test.each(REJECTED)("falls back to the permissive empty object schema for a %s input schema", (_label, schema) => { - expect(map({ input_schema: schema }).inputSchema).toEqual(EMPTY_OBJECT_SCHEMA); - }); - - test("falls back to the permissive empty object schema when there is no input schema", () => { - expect(map({}).inputSchema).toEqual(EMPTY_OBJECT_SCHEMA); - }); -}); - -describe("degradation notices", () => { - test("warns once per tool however often tools/list is answered", () => { - const { map: mapper, notices } = mapperWithNotices(); - const tool = namespacedTool({ name: "list-todos", output_schema: ARRAY_SCHEMA }); - - mapper(tool); - mapper(tool); - mapper(tool); - - expect(notices).toHaveLength(1); - expect(notices[0]).toContain("list-todos"); - expect(notices[0]).toContain("output schema"); - }); - - test("names the tool as the agent sees it, and says why MCP rejected the schema", () => { - const { map: mapper, notices } = mapperWithNotices(); - - mapper({ - mcpName: "pixel-8__lies", - selector: "pixel-8", - descriptor: { name: "lies", description: "d", output_schema: SCALAR_REQUIRED }, - policy: "allow", - }); - - expect(notices[0]).toContain("pixel-8__lies"); - expect(notices[0]).toContain("required"); - }); - - test("two sessions exposing the same broken tool each get a notice", () => { - const { map: mapper, notices } = mapperWithNotices(); - - mapper(namespacedTool({ name: "list-todos", output_schema: ARRAY_SCHEMA }, "pixel-8")); - mapper(namespacedTool({ name: "list-todos", output_schema: ARRAY_SCHEMA }, "iphone-16")); - - expect(notices).toHaveLength(2); - }); - - test("re-registering the same tool with a differently broken schema warns again", () => { - const { map: mapper, notices } = mapperWithNotices(); - - mapper(namespacedTool({ name: "list-todos", output_schema: ARRAY_SCHEMA })); - mapper(namespacedTool({ name: "list-todos", output_schema: STRING_SCHEMA })); - - expect(notices).toHaveLength(2); - }); - - test("the single-to-multi session flip, which rewrites mcpName, does not re-warn", () => { - const { map: mapper, notices } = mapperWithNotices(); - const descriptor = { name: "list-todos", description: "d", output_schema: ARRAY_SCHEMA }; - - mapper({ mcpName: "list-todos", selector: "pixel-8", descriptor, policy: "allow" }); - mapper({ mcpName: "pixel-8__list-todos", selector: "pixel-8", descriptor, policy: "allow" }); - - expect(notices).toHaveLength(1); - }); - - test("warns separately for the input and the output side of one tool", () => { - const { map: mapper, notices } = mapperWithNotices(); - - mapper(namespacedTool({ name: "a", input_schema: STRING_SCHEMA, output_schema: ARRAY_SCHEMA })); - - expect(notices).toHaveLength(2); - expect(notices.filter((notice) => notice.includes("input schema"))).toHaveLength(1); - expect(notices.filter((notice) => notice.includes("output schema"))).toHaveLength(1); - }); - - test("remembers a bounded number of notices, so a schema built from live data cannot grow it forever", () => { - const { map: mapper, notices } = mapperWithNotices(); - - // Each iteration is a *distinct* broken schema for the same tool, which is what an app - // building a schema from fetched rows would produce. Far past the 256-key cap. - for (let index = 0; index < 400; index += 1) { - mapper(namespacedTool({ name: "from-live-data", output_schema: { type: "string", const: `v${index}` } })); - } - - expect(notices).toHaveLength(400); - - // The oldest keys have been evicted, so the very first schema warns again... - mapper(namespacedTool({ name: "from-live-data", output_schema: { type: "string", const: "v0" } })); - expect(notices).toHaveLength(401); - - // ...while a recent one is still remembered and stays quiet. - mapper(namespacedTool({ name: "from-live-data", output_schema: { type: "string", const: "v399" } })); - expect(notices).toHaveLength(401); - }); - - test("the default sink writes to stderr with the same prefix as the rest of the MCP server", () => { - const stderr = vi.spyOn(console, "error").mockImplementation(() => {}); - - try { - createMcpToolMapper()(namespacedTool({ name: "list-todos", output_schema: ARRAY_SCHEMA })); - - expect(stderr).toHaveBeenCalledTimes(1); - expect(String(stderr.mock.calls[0]![0])).toMatch(/^appduct mcp: /); - } finally { - stderr.mockRestore(); - } - }); - - test("stays silent for accepted and absent schemas", () => { - const { map: mapper, notices } = mapperWithNotices(); - - mapper(namespacedTool({ name: "quiet", input_schema: OBJECT_SCHEMA, output_schema: RECORD_SCHEMA })); - mapper(namespacedTool({ name: "also-quiet" })); - - expect(notices).toEqual([]); - }); -}); - -describe("unrelated mapping is unchanged", () => { - test('annotations still map alongside a dropped output schema, and a "prompt" tool carries no _meta', () => { - const { map: mapper } = mapperWithNotices(); - const mapped = mapper({ - mcpName: "pixel-8__list-todos", - selector: "pixel-8", - descriptor: { - name: "list-todos", - description: "Lists todos.", - output_schema: ARRAY_SCHEMA, - annotations: { readOnlyHint: true }, - }, - policy: "prompt", - }); - - expect(mapped.name).toBe("pixel-8__list-todos"); - expect(mapped.annotations).toEqual({ readOnlyHint: true }); - expect(mapped).not.toHaveProperty("_meta"); - expect(mapped.outputSchema).toBeUndefined(); - }); -}); diff --git a/packages/appduct/src/cli/create-cli.ts b/packages/appduct/src/cli/create-cli.ts index 093a98db..2d9fa5d9 100644 --- a/packages/appduct/src/cli/create-cli.ts +++ b/packages/appduct/src/cli/create-cli.ts @@ -88,7 +88,7 @@ export const createCli = () => { cli.command("revoke [selector]", "Revoke a session."); cli - .command("mcp", "Start a stdio MCP server that proxies connected apps' tools to MCP clients.") + .command("mcp", "Start a stdio MCP server that gives MCP clients access to connected apps' tools.") .option( "--scheme ", "Deep-link URI scheme for appduct_connect (also: APPDUCT_SCHEME).", diff --git a/packages/appduct/src/daemon/daemon.ts b/packages/appduct/src/daemon/daemon.ts index c2209a69..199e5238 100644 --- a/packages/appduct/src/daemon/daemon.ts +++ b/packages/appduct/src/daemon/daemon.ts @@ -10,6 +10,7 @@ import { rm } from "node:fs/promises"; import { + MAX_TOOLS_FILTER_LENGTH, RPC_METHODS, EVENT_KINDS, type EventKind, @@ -153,9 +154,6 @@ const asSelectorParams = (params: unknown): { selector?: string } => { return { selector }; }; -/** `tools.list`'s `filter` string cap (ARCHITECTURE.md §5) — generous for a name/description - * substring search, small enough that a malicious/buggy caller can't use it to bloat a request. */ -const MAX_TOOLS_FILTER_LENGTH = 256; const asToolsListParams = (params: unknown): ToolsListParams => { const { selector } = asSelectorParams(params); diff --git a/packages/appduct/src/mcp/app-tools.ts b/packages/appduct/src/mcp/app-tools.ts new file mode 100644 index 00000000..7cef4d78 --- /dev/null +++ b/packages/appduct/src/mcp/app-tools.ts @@ -0,0 +1,323 @@ +/** + * The built-in `appduct_list_tools` / `appduct_describe_tool` / `appduct_call_tool` MCP tools + * (ARCHITECTURE.md §9). The app's own tools are never listed as MCP tools. An agent reaches them + * the way the CLI does: compact signatures first (`appduct tools`), one full schema on demand + * (`appduct tools `), then a call by name (`appduct invoke`). A registry of hundreds of tools + * therefore costs a client three fixed tool definitions, and `tools/list` never changes while an + * agent works. + * + * Every lookup goes to the daemon live, with no caching: the session and its registry can change + * between any two calls, and the daemon is the single source of truth for both. + */ + +import { + MAX_TOOLS_FILTER_LENGTH, + MAX_TOOL_TIMEOUT_MS, + MIN_TOOL_TIMEOUT_MS, + RPC_METHODS, + renderToolSignature, + summarizeToolDescription, + type EffectivePolicyDecision, + type SessionsDescribeResult, + type ToolDescriptor, + type ToolsListEntry, + type ToolsListResult, +} from "@appduct/shared"; + +import { clampTimeout } from "../daemon/calls.js"; +import { McpBuiltinToolError } from "./connect-tool.js"; +import type { DaemonCall } from "./daemon-tools.js"; + +export const LIST_TOOLS_TOOL_NAME = "appduct_list_tools"; +export const DESCRIBE_TOOL_TOOL_NAME = "appduct_describe_tool"; +export const CALL_TOOL_TOOL_NAME = "appduct_call_tool"; + +/** Applied when `appduct_list_tools` gets no `limit`, so the first call on a large app returns a + * page, not the whole registry; `total` tells the agent how much it left out. */ +export const DEFAULT_LIST_TOOLS_LIMIT = 50; + +const SELECTOR_PROPERTY = { + type: "string", + minLength: 1, + description: "Session alias or id. Omit to target the sole active/suspended session.", +} as const; + +const NAME_PROPERTY = { type: "string", minLength: 1 } as const; + +export const LIST_TOOLS_TOOL_DESCRIPTOR = { + name: LIST_TOOLS_TOOL_NAME, + description: + "List the tools the connected app registered, as one-line signatures " + + "(`name(param: type, optional?: type) -> result`) with the first line of each description " + + "and the tool's effective policy. Start here: the app's tools are not MCP tools of their own. " + + "filter is a case-insensitive substring match on name and description; limit (default " + + `${DEFAULT_LIST_TOOLS_LIMIT}) and offset page the name-sorted list, and total counts every ` + + "match before paging, so page on with offset when total is larger. Use appduct_describe_tool for " + + "one tool's full input/output schema, then appduct_call_tool to run it. A tool with policy " + + '"prompt" asks the user to approve each call; one with policy "deny" cannot be called.', + inputSchema: { + type: "object", + properties: { + selector: SELECTOR_PROPERTY, + filter: { type: "string", maxLength: MAX_TOOLS_FILTER_LENGTH }, + limit: { type: "integer", exclusiveMinimum: 0 }, + offset: { type: "integer", minimum: 0 }, + }, + additionalProperties: false, + }, +} as const; + +export const DESCRIBE_TOOL_TOOL_DESCRIPTOR = { + name: DESCRIBE_TOOL_TOOL_NAME, + description: + "Show one of the connected app's tools in full: description, input_schema (JSON Schema for " + + "appduct_call_tool's args), output_schema, annotations, timeout and effective policy. Find " + + "names with appduct_list_tools.", + inputSchema: { + type: "object", + properties: { + selector: SELECTOR_PROPERTY, + name: NAME_PROPERTY, + }, + required: ["name"], + additionalProperties: false, + }, +} as const; + +export const CALL_TOOL_TOOL_DESCRIPTOR = { + name: CALL_TOOL_TOOL_NAME, + description: + "Call one of the connected app's tools by name. args must match the tool's input_schema " + + "(see appduct_describe_tool); omit it for a tool that takes no input. The call runs under the " + + "tool's own deadline (timeout_ms in appduct_describe_tool, 10000 ms if it declares none); " + + "timeoutMs can only shorten it, since the app stops the tool at its own deadline. Returns " + + 'the tool\'s result as JSON. A tool with policy "prompt" ' + + "asks the user to approve the call first, and fails if they decline or this client cannot ask.", + inputSchema: { + type: "object", + properties: { + selector: SELECTOR_PROPERTY, + name: NAME_PROPERTY, + args: { type: "object" }, + timeoutMs: { type: "integer", minimum: MIN_TOOL_TIMEOUT_MS, maximum: MAX_TOOL_TIMEOUT_MS }, + }, + required: ["name"], + additionalProperties: false, + }, +} as const; + +/** One app tool resolved against a live session, with everything `appduct_call_tool` needs to + * run it: the session id the call, its progress subscription and any cancel are routed by, the + * alias shown to people, the descriptor (for its name and declared deadline), and the effective + * policy (for whether to ask for consent). */ +export type ResolvedAppTool = { + sessionId: string; + alias: string; + descriptor: ToolDescriptor; + policy: EffectivePolicyDecision; +}; + +type ResolvedSession = { sessionId: string; alias: string }; + +export type CallToolArgs = { + selector?: string; + name: string; + args: Record; + timeoutMs?: number; +}; + +const asRecord = (value: unknown): Record => { + if (typeof value !== "object" || value === null || Array.isArray(value)) { + return {}; + } + + return value as Record; +}; + +/** Every key an agent sends must be one the tool declares. A misspelled key (`arguments` for + * `args`, `timeout_ms` for `timeoutMs`) would otherwise be dropped silently, and the tool would run + * without what the agent meant to pass. */ +const rejectUnknownKeys = (args: Record, tool: string, allowed: readonly string[]): void => { + const unknown = Object.keys(args).filter((key) => !allowed.includes(key)); + + if (unknown.length > 0) { + throw new McpBuiltinToolError( + "invalid_request", + `${tool} does not take ${unknown.map((key) => `"${key}"`).join(", ")}. It takes: ${allowed.join(", ")}.`, + ); + } +}; + +/** `null` counts as absent for every optional field: some clients fill unset optional + * parameters with `null` rather than leaving them out. */ +const asOptionalString = (value: unknown, field: string): string | undefined => { + if (value === undefined || value === null) { + return undefined; + } + + if (typeof value !== "string" || value.length === 0) { + throw new McpBuiltinToolError("invalid_request", `"${field}" must be a non-empty string.`); + } + + return value; +}; + +const asRequiredString = (value: unknown, field: string): string => { + const parsed = asOptionalString(value, field); + + if (parsed === undefined) { + throw new McpBuiltinToolError("invalid_request", `"${field}" is required.`); + } + + return parsed; +}; + +/** Explicit pick, so a non-descriptor field on `ToolsListEntry` (today `policy`) is reported once, + * on its own key, rather than twice. */ +const toDescriptor = (entry: ToolsListEntry): ToolDescriptor => { + return { + name: entry.name, + description: entry.description, + input_schema: entry.input_schema, + output_schema: entry.output_schema, + annotations: entry.annotations, + timeout_ms: entry.timeout_ms, + }; +}; + +/** Resolves `selector` (an alias, a session id, or nothing) to one concrete session first, with + * the daemon's own rules and errors (`no_session`, `ambiguous_session`, `unknown_session`). + * Everything after that is routed by the **session id**, never the alias: the daemon frees an + * alias when a session ends and gives it to the next device of the same model, so routing by alias + * could land a call — one the user may already have approved — on a different device. If the + * session goes away mid-call, the next daemon call fails with `unknown_session` instead. */ +const resolveSession = async (call: DaemonCall, selector: string | undefined): Promise => { + const session = await call(RPC_METHODS.sessionsDescribe, { selector }); + return { sessionId: session.sessionId, alias: session.alias }; +}; + +const findTool = async (call: DaemonCall, selector: string | undefined, name: string) => { + const session = await resolveSession(call, selector); + // The whole registry, never a filtered page, so a name lookup cannot miss a tool that paging + // would have left out. + const { tools } = await call(RPC_METHODS.toolsList, { selector: session.sessionId }); + const entry = tools.find((tool) => tool.name === name); + + if (!entry) { + throw new McpBuiltinToolError( + "tool_not_found", + `Tool "${name}" is not registered on session "${session.alias}". Use ${LIST_TOOLS_TOOL_NAME} (optionally with filter) to find it.`, + ); + } + + return { session, entry }; +}; + +export const handleListToolsTool = async (rawArgs: unknown, call: DaemonCall) => { + const args = asRecord(rawArgs); + rejectUnknownKeys(args, LIST_TOOLS_TOOL_NAME, ["selector", "filter", "limit", "offset"]); + const selector = asOptionalString(args.selector, "selector"); + const session = await resolveSession(call, selector); + + // `filter`/`limit`/`offset` are validated by the daemon, which rejects a bad value with + // `invalid_request` exactly as it does for the CLI. + const params = { + ...(args.filter !== undefined && args.filter !== null ? { filter: args.filter } : {}), + limit: args.limit ?? DEFAULT_LIST_TOOLS_LIMIT, + ...(args.offset !== undefined && args.offset !== null ? { offset: args.offset } : {}), + }; + const result = await call(RPC_METHODS.toolsList, { selector: session.sessionId, ...params }); + + return { + session: session.alias, + total: result.total, + ...params, + tools: result.tools.map((entry) => ({ + name: entry.name, + signature: renderToolSignature(entry), + summary: summarizeToolDescription(entry.description), + policy: entry.policy, + ...(entry.annotations ? { annotations: entry.annotations } : {}), + })), + }; +}; + +export const handleDescribeToolTool = async (rawArgs: unknown, call: DaemonCall) => { + const args = asRecord(rawArgs); + rejectUnknownKeys(args, DESCRIBE_TOOL_TOOL_NAME, ["selector", "name"]); + const selector = asOptionalString(args.selector, "selector"); + const name = asRequiredString(args.name, "name"); + const { session, entry } = await findTool(call, selector, name); + + return { + session: session.alias, + signature: renderToolSignature(entry), + policy: entry.policy, + ...toDescriptor(entry), + }; +}; + +export const parseCallToolArgs = (rawArgs: unknown): CallToolArgs => { + const args = asRecord(rawArgs); + rejectUnknownKeys(args, CALL_TOOL_TOOL_NAME, ["selector", "name", "args", "timeoutMs"]); + const selector = asOptionalString(args.selector, "selector"); + const name = asRequiredString(args.name, "name"); + const toolArgs = args.args ?? {}; + + if (typeof toolArgs !== "object" || Array.isArray(toolArgs)) { + throw new McpBuiltinToolError("invalid_request", '"args" must be an object.'); + } + + const timeoutMs = args.timeoutMs ?? undefined; + + // Rejected rather than clamped: the daemon would silently clamp an out-of-range deadline, and + // an agent that asked for an hour should learn it gets ten minutes before the call starts. + if ( + timeoutMs !== undefined && + (typeof timeoutMs !== "number" || + !Number.isInteger(timeoutMs) || + timeoutMs < MIN_TOOL_TIMEOUT_MS || + timeoutMs > MAX_TOOL_TIMEOUT_MS) + ) { + throw new McpBuiltinToolError( + "invalid_request", + `"timeoutMs" must be an integer number of milliseconds from ${MIN_TOOL_TIMEOUT_MS} to ${MAX_TOOL_TIMEOUT_MS}.`, + ); + } + + return { + selector, + name, + args: toolArgs as Record, + timeoutMs: timeoutMs as number | undefined, + }; +}; + +/** + * The deadline a call runs under: the tool's own (`clampTimeout` folds in the 10 s default for a + * tool that declares none), or a shorter one the caller asked for. Never longer: the `tool_call` + * frame carries no deadline, so the app stops the handler at the tool's own deadline whatever the + * daemon waits for (docs/PROTOCOL.md), and a longer caller deadline would only turn a clear + * `tool_timeout` into the same timeout after a retry that ran the tool again. + */ +export const resolveCallDeadline = (tool: ResolvedAppTool, requestedTimeoutMs: number | undefined): number => { + const toolDeadline = clampTimeout(tool.descriptor.timeout_ms); + + if (requestedTimeoutMs !== undefined && requestedTimeoutMs > toolDeadline) { + throw new McpBuiltinToolError( + "invalid_request", + `"timeoutMs" can only shorten "${tool.descriptor.name}"'s own deadline of ${toolDeadline} ms: the app stops the tool at that deadline. Raising it takes a larger timeoutMs in the tool's registration.`, + ); + } + + return requestedTimeoutMs ?? toolDeadline; +}; + +export const resolveAppTool = async ( + call: DaemonCall, + selector: string | undefined, + name: string, +): Promise => { + const { session, entry } = await findTool(call, selector, name); + return { ...session, descriptor: toDescriptor(entry), policy: entry.policy }; +}; diff --git a/packages/appduct/src/mcp/connect-tool.ts b/packages/appduct/src/mcp/connect-tool.ts index 580a0536..99da0b80 100644 --- a/packages/appduct/src/mcp/connect-tool.ts +++ b/packages/appduct/src/mcp/connect-tool.ts @@ -115,7 +115,7 @@ export const WAIT_FOR_SESSION_TOOL_DESCRIPTOR = { "as the device connects (or immediately if it already has); rejects with tool_timeout if " + "timeoutMs elapses first. If appduct_connect returned a QR instead of delivering the link, " + "show that QR to the user and ask them to scan it before calling this — it produces no output " + - "while it waits.", + "while it waits. Once it resolves, find the app's tools with appduct_list_tools.", inputSchema: { type: "object", properties: { @@ -129,10 +129,10 @@ export const WAIT_FOR_SESSION_TOOL_DESCRIPTOR = { /** Errors from the built-in management tools reuse the daemon's wire `ErrorType` union * (`RpcApplicationError`-style: `{ type, message }`) so the MCP server's generic error-content - * mapping (see `server.ts`) handles them the same way it handles a proxied device tool's error. */ + * mapping (see `server.ts`) handles them the same way it handles an app tool's error. */ export class McpBuiltinToolError extends Error { constructor( - readonly type: "invalid_request" | "tool_timeout" | "tool_execution_error", + readonly type: "invalid_request" | "tool_not_found" | "tool_timeout" | "tool_cancelled" | "tool_execution_error", message: string, ) { super(message); diff --git a/packages/appduct/src/mcp/daemon-tools.ts b/packages/appduct/src/mcp/daemon-tools.ts index a0b7c41b..bfebb384 100644 --- a/packages/appduct/src/mcp/daemon-tools.ts +++ b/packages/appduct/src/mcp/daemon-tools.ts @@ -1,44 +1,4 @@ -/** - * Fetches the effective (possibly namespaced) MCP tool list live from the daemon - * (ARCHITECTURE.md §9): `sessions.list` for the live sessions, then `tools.list` per session. - * There is no caching here — every `tools/list` request and every list-changed check re-fetches, - * since the daemon's session/registry state is the single source of truth and can change between - * any two calls. - */ - -import { - RPC_METHODS, - type SessionsListResult, - type ToolsListEntry, - type ToolsListResult, -} from "@appduct/shared"; - -import { DaemonRpcError } from "../rpc/client.js"; -import { buildNamespacedTools, type NamespacedTool } from "./tool-namespace.js"; - /** A method call against the daemon RPC — satisfied by both `callDaemon` (bound to a method+params) - * and `DaemonStream.call` from `rpc/client.ts`. */ + * and `DaemonStream.call` from `rpc/client.ts`. The MCP server's built-in tools take one of these + * rather than a whole stream, so they can be tested against a fake. */ export type DaemonCall = (method: string, params?: unknown) => Promise; - -export const fetchEffectiveTools = async (call: DaemonCall): Promise => { - const sessions = await call(RPC_METHODS.sessionsList); - const toolsByAlias = new Map(); - - for (const session of sessions) { - try { - const { tools } = await call(RPC_METHODS.toolsList, { selector: session.alias }); - toolsByAlias.set(session.alias, tools); - } catch (error) { - // The session can transition (revoke/expire) between `sessions.list` and this per-session - // `tools.list` call; treat it as having no tools rather than failing the whole listing. - if (error instanceof DaemonRpcError) { - toolsByAlias.set(session.alias, []); - continue; - } - - throw error; - } - } - - return buildNamespacedTools(sessions, toolsByAlias); -}; diff --git a/packages/appduct/src/mcp/server.ts b/packages/appduct/src/mcp/server.ts index 7c527c03..ee1446ad 100644 --- a/packages/appduct/src/mcp/server.ts +++ b/packages/appduct/src/mcp/server.ts @@ -1,12 +1,14 @@ /** - * The `appduct mcp` stdio MCP server (ARCHITECTURE.md §9): a thin proxy that maps the daemon RPC - * surface (`rpc/client.ts`, same auto-spawning client the CLI uses) onto MCP's `tools/list`, - * `tools/call`, `notifications/tools/list_changed`, progress notifications, and one resource + * The `appduct mcp` stdio MCP server (ARCHITECTURE.md §9): a thin proxy over the daemon RPC surface + * (`rpc/client.ts`, same auto-spawning client the CLI uses). `tools/list` is a fixed set of + * built-in tools; the app's own tools are reached through `appduct_list_tools`, + * `appduct_describe_tool` and `appduct_call_tool` (`app-tools.ts`), mirroring the CLI, never + * listed as MCP tools themselves. Also progress notifications, cancellation, and one resource * (`appduct://sessions`). All logging here goes to stderr only — stdout is reserved for the MCP * transport's protocol frames. * * One persistent daemon connection (`stream`) is used for everything except progress-correlated - * `tools.call`s, which get their own short-lived connection (see `callProxiedTool` below) so the + * `tools.call`s, which get their own short-lived connection (see `callAppTool` below) so the * `tool_call_started` event that reveals the call's `callId` is unambiguous. * * This is also where the `"prompt"`-policy consent channel lives (ARCHITECTURE.md §12): @@ -27,10 +29,10 @@ import { type CallToolResult, } from "@modelcontextprotocol/sdk/types.js"; -import { RPC_METHODS, type EventKind, type EventNotification, type SessionsListResult, type ToolsCallResult } from "@appduct/shared"; +import { RPC_METHODS, type EventNotification, type SessionsListResult, type ToolsCallResult } from "@appduct/shared"; import type { ExecFn } from "../cli/open-target.js"; -import { clampTimeout, deriveCallTransportTimeoutMs } from "../daemon/calls.js"; +import { deriveCallTransportTimeoutMs } from "../daemon/calls.js"; import { getPackageVersion } from "../package-version.js"; import { DaemonRpcError, @@ -40,6 +42,20 @@ import { type SpawnFn, type VersionCheckOptions, } from "../rpc/client.js"; +import { + CALL_TOOL_TOOL_DESCRIPTOR, + CALL_TOOL_TOOL_NAME, + DESCRIBE_TOOL_TOOL_DESCRIPTOR, + DESCRIBE_TOOL_TOOL_NAME, + handleDescribeToolTool, + handleListToolsTool, + LIST_TOOLS_TOOL_DESCRIPTOR, + LIST_TOOLS_TOOL_NAME, + parseCallToolArgs, + resolveAppTool, + resolveCallDeadline, + type ResolvedAppTool, +} from "./app-tools.js"; import { CONNECT_TOOL_DESCRIPTOR, CONNECT_TOOL_NAME, @@ -49,7 +65,6 @@ import { WAIT_FOR_SESSION_TOOL_DESCRIPTOR, WAIT_FOR_SESSION_TOOL_NAME, } from "./connect-tool.js"; -import { fetchEffectiveTools } from "./daemon-tools.js"; import { EVENTS_TOOL_DESCRIPTOR, EVENTS_TOOL_NAME, @@ -58,49 +73,15 @@ import { WAIT_FOR_EVENT_TOOL_DESCRIPTOR, WAIT_FOR_EVENT_TOOL_NAME, } from "./events-tool.js"; -import { createMcpToolMapper, emitsMcpOutputSchema } from "./tool-mapping.js"; -import { findNamespacedTool, namespacedToolsSnapshotKey, type NamespacedTool } from "./tool-namespace.js"; const packageVersion = getPackageVersion(); export const SESSIONS_RESOURCE_URI = "appduct://sessions"; -/** Event kinds whose arrival can change the effective (namespaced or not) tool list — including - * the single↔multi namespacing flip itself, which is driven by session count, not tool count. */ -const LIST_CHANGE_EVENT_KINDS: readonly EventKind[] = [ - "tools_changed", - "session_claimed", - "session_revoked", - "session_expired", - "session_suspended", - "session_resumed", -]; - const isPlainObject = (value: unknown): value is Record => { return typeof value === "object" && value !== null && !Array.isArray(value); }; -/** The *shape* of a rejected tool result, for the error message only — never the value itself, - * which may be large or carry app data that does not belong in an agent-visible error string. */ -const describeJsonValue = (value: unknown): string => { - if (value === null) { - return "null"; - } - - // Defensive: the daemon rejects a `tool_result` frame with no `result`, so nothing on today's - // wire path yields `undefined` here. Spelled out anyway so a future one reads as "returned no - // value" rather than the `typeof` wording, "returned a undefined". - if (value === undefined) { - return "no value"; - } - - if (Array.isArray(value)) { - return "an array"; - } - - return `a ${typeof value}`; -}; - /** * Whether the connected MCP client declared the `elicitation` capability at `initialize` * (ARCHITECTURE.md §12 / issue #10) — the only `"prompt"`-policy consent channel, checked fresh on @@ -136,14 +117,14 @@ const formatElicitationArgsPreview = (args: Record): string => /** Names the session alias, the tool, and the actual arguments — the human answering this prompt * has nothing else to go on (ARCHITECTURE.md §12 / issue #10). */ -const buildElicitationMessage = (tool: NamespacedTool, args: Record): string => { +const buildElicitationMessage = (tool: ResolvedAppTool, args: Record): string => { return ( - `Appduct: allow the MCP tool "${tool.descriptor.name}" to run on session "${tool.selector}"? ` + + `Appduct: allow the app tool "${tool.descriptor.name}" to run on session "${tool.alias}" (${tool.sessionId})? ` + `Arguments: ${formatElicitationArgsPreview(args)}` ); }; -/** Thrown from `callProxiedTool` when the human declined or cancelled an elicitation prompt (or it +/** Thrown from `callAppTool` when the human declined or cancelled an elicitation prompt (or it * timed out), so the outer `tools/call` handler can turn it into an ordinary MCP tool **result** * with `isError: true` — never a thrown protocol-level error — matching every other error path in * this file (`toolErrorContentFromError` below): the agent reading the result must be able to @@ -169,7 +150,7 @@ class ElicitationDeclinedError extends Error { */ const requestElicitationConsent = async ( server: Server, - tool: NamespacedTool, + tool: ResolvedAppTool, args: Record, // Injectable so tests can exercise the timeout branch without an actual 10-minute wait; every // production caller passes `ELICITATION_TIMEOUT_MS` (via `CreateMcpServerOptions.elicitationTimeoutMs`). @@ -194,13 +175,13 @@ const requestElicitationConsent = async ( return { type: "declined", - message: `The user ${result.action === "cancel" ? "cancelled" : "declined"} the request to call "${tool.descriptor.name}" on session "${tool.selector}".`, + message: `The user ${result.action === "cancel" ? "cancelled" : "declined"} the request to call "${tool.descriptor.name}" on session "${tool.alias}".`, }; } catch (error) { if (error instanceof McpError && error.code === ErrorCode.RequestTimeout) { return { type: "declined", - message: `Timed out after ${timeoutMs / 1000}s waiting for a human to respond to the consent prompt for "${tool.descriptor.name}" on session "${tool.selector}".`, + message: `Timed out after ${timeoutMs / 1000}s waiting for a human to respond to the consent prompt for "${tool.descriptor.name}" on session "${tool.alias}".`, }; } @@ -221,7 +202,7 @@ const toolSuccessContent = (result: unknown): CallToolResult => { return { content }; }; -/** Errors from a tool call — proxied device tool or built-in management tool alike — become MCP +/** Errors from a tool call — an app tool run through `appduct_call_tool` or a built-in alike — become MCP * tool-error *content*, never a thrown protocol-level error: the preserved * `type` and `message` are put in the text so an agent reading the result can branch on them. */ const toolErrorContent = (type: string, message: string, details?: unknown): CallToolResult => { @@ -255,38 +236,12 @@ const toolErrorContentFromError = (error: unknown): CallToolResult => { return toolErrorContent("tool_execution_error", "An unexpected error occurred."); }; -/** - * The success path for a *proxied* device tool (issue #26). An MCP client requires - * `structuredContent` on every successful call to a tool whose `tools/list` entry carried an - * `outputSchema`, so this shares `emitsMcpOutputSchema` with the mapping layer: the two decisions - * are the same predicate and cannot drift. When a schema *was* advertised and the result is not an - * object anyway (reachable only when the schema's validator is looser than its declared shape, - * `tool-invocation.ts`), the call fails as `tool_output_validation_error` rather than as an opaque - * client-side protocol error. A tool whose schema was dropped, or that has none, keeps the - * opportunistic behaviour: text content always, plus `structuredContent` when the result happens - * to be an object — which is allowed, since the client has no schema to validate it against. - */ -const proxiedToolResultContent = (tool: NamespacedTool, result: unknown): CallToolResult => { - if (!emitsMcpOutputSchema(tool.descriptor.output_schema)) { - return toolSuccessContent(result); - } - - if (!isPlainObject(result)) { - return toolErrorContent( - "tool_output_validation_error", - `Tool "${tool.mcpName}" declares an object output schema but returned ${describeJsonValue(result)}.`, - ); - } - - return toolSuccessContent(result); -}; - /** * How this server opens a daemon connection. Both the startup stream and each short-lived * progress stream go through it, so a caller can put something other than a real daemon on the * other end. The only production implementation is {@link openDaemonStream}; the seam exists so - * the behaviour that is purely this module's own — name mapping, schema degradation, consent, - * namespacing, `list_changed` — can be tested without a TLS listener, a pidfile and a + * the behaviour that is purely this module's own — the built-in tools, consent, progress and + * cancellation — can be tested without a TLS listener, a pidfile and a * subprocess, none of which those behaviours depend on. */ export type OpenDaemonStreamFn = (options: { @@ -360,14 +315,15 @@ export const createMcpServer = async (options: CreateMcpServerOptions): Promise< const server = new Server( { name: "appduct", version: packageVersion }, - { capabilities: { tools: { listChanged: true }, resources: {} } }, + // `tools/list` is a fixed set of built-ins, so there is never a list change to announce. + { capabilities: { tools: {}, resources: {} } }, ); /** * Resolves the `consent` param for one `"prompt"`-policy `tools.call` (ARCHITECTURE.md §12), - * shared by both call paths below so the logic exists exactly once. Recomputes the tool's live - * policy's implications at call time — never trusts anything cached from a prior `tools/list` - * snapshot. + * shared by both call paths below so the logic exists exactly once. Works from the policy the + * daemon reported when `appduct_call_tool` resolved the tool for this call, never from an + * earlier `appduct_list_tools` result. * * Elicitation (issue #10) is the only channel. A client that didn't declare the capability gets * no consent marker, so the daemon denies the call with reason `no_consent_channel`. An @@ -376,7 +332,7 @@ export const createMcpServer = async (options: CreateMcpServerOptions): Promise< * "no consent obtained". */ const resolveToolCallConsent = async ( - tool: NamespacedTool, + tool: ResolvedAppTool, args: Record, ): Promise<"elicitation" | undefined> => { if (tool.policy !== "prompt" || !clientSupportsElicitation(server)) { @@ -404,13 +360,11 @@ export const createMcpServer = async (options: CreateMcpServerOptions): Promise< return undefined; }; - // One mapper per server: it owns the dedup for the "this schema had to be degraded" stderr - // notices, which would otherwise repeat on every `tools/list` and every list-changed refresh. - const mapToMcpTool = createMcpToolMapper(); - - const callProxiedTool = async ( - tool: NamespacedTool, + const callAppTool = async ( + tool: ResolvedAppTool, args: Record, + /** The caller's `timeoutMs`, overriding the tool's declared deadline when set. */ + requestedTimeoutMs: number | undefined, progressToken: string | number | undefined, sendNotification: (notification: unknown) => Promise, // The MCP SDK aborts this automatically on an inbound `notifications/cancelled` for this @@ -419,23 +373,30 @@ export const createMcpServer = async (options: CreateMcpServerOptions): Promise< // `callId` to cancel by until it has already resolved. signal: AbortSignal, ): Promise => { + // The deadline this call runs under, resolved once (from the `tools.list` snapshot the tool was + // matched against, or a shorter one the caller asked for) and sent explicitly rather than left + // to the daemon's `?? tool.timeout_ms` fallback, so the watchdog below can never be sized off a + // different read: the daemon consults its live registry, which a re-registration between that + // snapshot and this call could have moved, and a watchdog built for the older value would fire + // first and mask the daemon's real `tool_timeout` (issue #25). Checked before consent, so a + // bad `timeoutMs` never costs the user a prompt. + const timeoutMs = resolveCallDeadline(tool, requestedTimeoutMs); + const transportTimeoutMs = deriveCallTransportTimeoutMs(timeoutMs); + const consent = await resolveToolCallConsent(tool, args); - // The deadline this call runs under, resolved once from the `tools.list` snapshot it was - // matched against, and sent explicitly rather than left to the daemon's `?? tool.timeout_ms` - // fallback. `clampTimeout` folds in the 10 s default for a tool that declares nothing, so - // there is exactly one number here and the watchdog below can never be sized off a different - // read: the daemon consults its live registry, which a re-registration between that snapshot - // and this call could have moved, and a watchdog built for the older value would fire first - // and mask the daemon's real `tool_timeout` (issue #25). - const timeoutMs = clampTimeout(tool.descriptor.timeout_ms); - const transportTimeoutMs = deriveCallTransportTimeoutMs(timeoutMs); + // The consent prompt can stay open for minutes, and the client may cancel the request while it + // does. Nothing has reached the app yet, so a cancel that arrived in the meantime must stop the + // call here — otherwise a user accepting a stale prompt would run a call its client gave up on. + if (signal.aborted) { + throw new McpBuiltinToolError("tool_cancelled", `The call to "${tool.descriptor.name}" was cancelled before it started.`); + } if (progressToken === undefined) { const result = await stream.call( RPC_METHODS.toolsCall, { - selector: tool.selector, + selector: tool.sessionId, name: tool.descriptor.name, args, caller: "mcp", @@ -448,20 +409,24 @@ export const createMcpServer = async (options: CreateMcpServerOptions): Promise< return result.result; } - // A dedicated connection carries only this one in-flight `tools.call`, so the first - // `tool_call_started` it sees for this tool name is unambiguously this call — the daemon only - // reveals `callId` once the call is already in flight (ARCHITECTURE.md §5's `ToolsCallResult` - // doc comment), so this is the only way to correlate it before the call finishes. + // A dedicated connection carries only this one in-flight `tools.call`, and the first + // `tool_call_started` it sees for this tool name on this session is taken to be this call — the + // daemon only reveals `callId` once the call is already in flight (ARCHITECTURE.md §5's + // `ToolsCallResult` doc comment), so this is the only way to correlate it before the call + // finishes. It is not airtight: two concurrent calls of the same tool on the same session (two + // agents, or MCP and the CLI) can each take the other's `callId`, and then progress and a cancel + // go to the wrong call. const progressStream = await openStream({ stateDir: options.stateDir, spawn: options.spawn }); try { await progressStream.call(RPC_METHODS.eventsSubscribe, { - sessionSelector: tool.selector, + sessionSelector: tool.sessionId, kinds: ["tool_call_started", "tool_call_progress"], }); let callId: string | undefined; - let cancelRequested = signal.aborted; + // Re-read: the signal can abort while the progress stream above was opening. + let cancelRequested: boolean = signal.aborted; const maybeCancel = (): void => { if (callId === undefined || !cancelRequested) { @@ -469,9 +434,9 @@ export const createMcpServer = async (options: CreateMcpServerOptions): Promise< } progressStream - .call(RPC_METHODS.toolsCancel, { selector: tool.selector, callId, reason: "mcp_client_cancelled" }) + .call(RPC_METHODS.toolsCancel, { selector: tool.sessionId, callId, reason: "mcp_client_cancelled" }) .catch((error: unknown) => { - console.error("appduct mcp: failed to cancel a proxied tool call:", error); + console.error("appduct mcp: failed to cancel an app tool call:", error); }); }; @@ -514,7 +479,7 @@ export const createMcpServer = async (options: CreateMcpServerOptions): Promise< const result = await progressStream.call( RPC_METHODS.toolsCall, { - selector: tool.selector, + selector: tool.sessionId, name: tool.descriptor.name, args, caller: "mcp", @@ -535,15 +500,15 @@ export const createMcpServer = async (options: CreateMcpServerOptions): Promise< }; server.setRequestHandler(ListToolsRequestSchema, async () => { - const tools = await fetchEffectiveTools(stream.call); - return { tools: [ CONNECT_TOOL_DESCRIPTOR, WAIT_FOR_SESSION_TOOL_DESCRIPTOR, + LIST_TOOLS_TOOL_DESCRIPTOR, + DESCRIBE_TOOL_TOOL_DESCRIPTOR, + CALL_TOOL_TOOL_DESCRIPTOR, EVENTS_TOOL_DESCRIPTOR, WAIT_FOR_EVENT_TOOL_DESCRIPTOR, - ...tools.map(mapToMcpTool), ], }; }); @@ -591,22 +556,33 @@ export const createMcpServer = async (options: CreateMcpServerOptions): Promise< ); } - const tools = await fetchEffectiveTools(stream.call); - const tool = findNamespacedTool(tools, name); + if (name === LIST_TOOLS_TOOL_NAME) { + return toolSuccessContent(await handleListToolsTool(args, stream.call)); + } - if (!tool) { - return toolErrorContent("tool_not_found", `Tool "${name}" is not registered.`); + if (name === DESCRIBE_TOOL_TOOL_NAME) { + return toolSuccessContent(await handleDescribeToolTool(args, stream.call)); } - return proxiedToolResultContent( - tool, - await callProxiedTool( - tool, - args, - progressToken, - extra.sendNotification as (notification: unknown) => Promise, - extra.signal, - ), + if (name === CALL_TOOL_TOOL_NAME) { + const callArgs = parseCallToolArgs(args); + const tool = await resolveAppTool(stream.call, callArgs.selector, callArgs.name); + + return toolSuccessContent( + await callAppTool( + tool, + callArgs.args, + callArgs.timeoutMs, + progressToken, + extra.sendNotification as (notification: unknown) => Promise, + extra.signal, + ), + ); + } + + return toolErrorContent( + "tool_not_found", + `"${name}" is not an Appduct MCP tool. Use ${LIST_TOOLS_TOOL_NAME} to find the app's tools and ${CALL_TOOL_TOOL_NAME} to run one.`, ); } catch (error) { return toolErrorContentFromError(error); @@ -638,53 +614,7 @@ export const createMcpServer = async (options: CreateMcpServerOptions): Promise< }; }); - // --- notifications/tools/list_changed --- - - let lastSnapshotKey: string | undefined; - let closed = false; - - const refreshAndMaybeNotifyListChanged = async (): Promise => { - const tools = await fetchEffectiveTools(stream.call); - - if (closed) { - return; - } - - const key = namespacedToolsSnapshotKey(tools); - const changed = lastSnapshotKey !== undefined && key !== lastSnapshotKey; - lastSnapshotKey = key; - - if (changed) { - await server.sendToolListChanged(); - } - }; - - const unsubscribeFromDaemonEvents = stream.onNotification((payload) => { - const event = payload as EventNotification; - - if (LIST_CHANGE_EVENT_KINDS.includes(event.kind)) { - refreshAndMaybeNotifyListChanged().catch((error: unknown) => { - if (closed) { - // Expected: the shared connection closed (server shutting down) while a refresh - // triggered by the last few daemon events was still in flight. - return; - } - - // A failed refresh/notify must never crash the process — the next qualifying event (or the - // client's own next `tools/list`) will simply see the current state instead. - console.error("appduct mcp: failed to refresh the tool list after a daemon event:", error); - }); - } - }); - - await stream.call(RPC_METHODS.eventsSubscribe, { kinds: LIST_CHANGE_EVENT_KINDS as EventKind[] }); - // Establish the baseline before any daemon event can race a real change in — the guard above - // (`lastSnapshotKey !== undefined`) means this first call only seeds state, never notifies. - await refreshAndMaybeNotifyListChanged(); - const close = async (): Promise => { - closed = true; - unsubscribeFromDaemonEvents(); stream.close(); }; diff --git a/packages/appduct/src/mcp/tool-mapping.ts b/packages/appduct/src/mcp/tool-mapping.ts deleted file mode 100644 index 16e4438d..00000000 --- a/packages/appduct/src/mcp/tool-mapping.ts +++ /dev/null @@ -1,190 +0,0 @@ -/** - * Maps a `NamespacedTool` (daemon `ToolDescriptor` + resolved external name) onto the MCP SDK's - * `Tool` wire shape (ARCHITECTURE.md §9/§7): `description` maps directly and `annotations` map - * verbatim, while `input_schema`/`output_schema` are emitted only if the SDK's own `ToolSchema` - * accepts them (issue #26) — a rejected `input_schema` is replaced with an empty, permissive - * object schema and a rejected `output_schema` is dropped entirely. Either way the tool stays - * listed and callable: one app-side schema MCP cannot represent must never take down the whole - * `tools/list`. - * - * The gate is the SDK schema itself rather than a hand-rolled `type === "object"` check, because - * MCP constrains more than the root type — `properties` must be a record of *object* subschemas - * (the JSON Schema shorthand `properties: { a: true }` is rejected) and `required` must be an - * array. Any of those makes a client reject the entire `tools/list` result, so the only safe - * predicate is the one the client will actually apply. - */ - -import { ToolSchema } from "@modelcontextprotocol/sdk/types.js"; - -import type { ToolSchemaDescriptor } from "@appduct/shared"; - -import type { NamespacedTool } from "./tool-namespace.js"; - -/** MCP requires `inputSchema.type === "object"`; an app that registered a tool without a schema - * gets the most permissive possible one rather than an MCP-invalid `{}`. */ -const EMPTY_OBJECT_SCHEMA = { type: "object", additionalProperties: true } as const; - -export type McpToolSchema = { - name: string; - description: string; - inputSchema: Record; - outputSchema?: Record; - annotations?: Record; -}; - -type SchemaSlot = "input" | "output"; - -/** - * Why the SDK's `ToolSchema` would reject `schema` in this slot, or `undefined` when it accepts - * it. The probe wraps the schema in an otherwise-minimal valid `Tool` so the only thing under test - * is the schema; the returned reason is the first issue, with the leading `inputSchema`/ - * `outputSchema` path segment stripped so the message reads from the app author's point of view. - */ -const mcpSchemaRejection = (schema: ToolSchemaDescriptor, slot: SchemaSlot): string | undefined => { - const probe = - slot === "input" - ? { name: "probe", inputSchema: schema } - : { name: "probe", inputSchema: EMPTY_OBJECT_SCHEMA, outputSchema: schema }; - - const parsed = ToolSchema.safeParse(probe); - - if (parsed.success) { - return undefined; - } - - const issue = parsed.error.issues[0]; - - if (!issue) { - return "it does not match MCP's tool schema"; - } - - const path = issue.path.slice(1).join("."); - - return path.length > 0 ? `${path}: ${issue.message}` : issue.message; -}; - -/** - * Whether `tools/list` will carry an `outputSchema` for this descriptor. `server.ts`'s success - * path shares this predicate so the "did we advertise a schema?" and "must this result carry - * `structuredContent`?" decisions can never disagree — a client demands the latter exactly when - * it saw the former. - */ -export const emitsMcpOutputSchema = (schema: ToolSchemaDescriptor | undefined): boolean => { - return schema !== undefined && mcpSchemaRejection(schema, "output") === undefined; -}; - -/** stderr only — stdout carries the MCP transport's protocol frames (ARCHITECTURE.md §9). */ -const defaultWarn = (message: string): void => { - console.error(`appduct mcp: ${message}`); -}; - -/** - * Upper bound on remembered notices. A key embeds the offending schema, and an app is free to - * build a schema from live data (a `z.enum` over rows it just fetched), so an unbounded set would - * grow with every distinct broken schema for the life of the process. Past the cap the oldest key - * is evicted, which at worst re-prints a notice the operator has already seen. - */ -const MAX_REMEMBERED_NOTICES = 256; - -export type McpToolMapper = (tool: NamespacedTool) => McpToolSchema; - -/** - * Builds a mapper with its own degradation-notice dedup, so nothing here is module state and no - * reset hook has to be exported for tests: a server owns one mapper for its lifetime, and a test - * owns one per case. `warn` defaults to stderr and exists so a test can capture notices without - * spying on the console. - * - * Notices dedupe on session + tool name + the offending schema, because `tools/list` is - * re-answered on every client request and on every `notifications/tools/list_changed` refresh. Two - * devices exposing the same broken tool each get their own notice, and re-registering a tool with - * a *differently* broken schema warns again; only the same problem on the same tool of the same - * session stays quiet. The session's `selector` is the key rather than the namespaced `mcpName` so - * the single↔multi session flip, which rewrites every `mcpName`, does not re-warn about tools - * nothing changed about. - */ -export const createMcpToolMapper = (warn: (message: string) => void = defaultWarn): McpToolMapper => { - const warned = new Set(); - - const warnOnce = (tool: NamespacedTool, slot: SchemaSlot, schema: ToolSchemaDescriptor, message: string): void => { - const key = `${tool.selector}\u0000${tool.descriptor.name}\u0000${slot}\u0000${JSON.stringify(schema)}`; - - if (warned.has(key)) { - return; - } - - // A `Set` iterates in insertion order, so the first key is the oldest. - if (warned.size >= MAX_REMEMBERED_NOTICES) { - const oldest = warned.values().next(); - - if (!oldest.done) { - warned.delete(oldest.value); - } - } - - warned.add(key); - warn(message); - }; - - const mapInputSchema = (tool: NamespacedTool): Record => { - const schema = tool.descriptor.input_schema; - - if (schema === undefined) { - return EMPTY_OBJECT_SCHEMA; - } - - const rejection = mcpSchemaRejection(schema, "input"); - - if (rejection === undefined) { - return schema; - } - - warnOnce( - tool, - "input", - schema, - `Tool "${tool.mcpName}" declares an input schema MCP cannot accept (${rejection}); MCP tool ` + - "arguments are always an object, so the schema was replaced with a permissive empty object " + - "schema and agents cannot see the tool's real arguments. Wrap the input in an object schema.", - ); - - return EMPTY_OBJECT_SCHEMA; - }; - - const mapOutputSchema = (tool: NamespacedTool): Record | undefined => { - const schema = tool.descriptor.output_schema; - - if (schema === undefined) { - return undefined; - } - - const rejection = mcpSchemaRejection(schema, "output"); - - if (rejection === undefined) { - return schema; - } - - warnOnce( - tool, - "output", - schema, - `Tool "${tool.mcpName}" declares an output schema MCP cannot accept (${rejection}); it was ` + - "dropped from tools/list, so agents get the result without a schema to validate it against. " + - "Wrap the output in an object schema to give agents a described, structured result.", - ); - - return undefined; - }; - - return (tool) => { - const { descriptor } = tool; - const outputSchema = mapOutputSchema(tool); - - return { - name: tool.mcpName, - description: descriptor.description, - inputSchema: mapInputSchema(tool), - ...(outputSchema ? { outputSchema } : {}), - ...(descriptor.annotations ? { annotations: { ...descriptor.annotations } } : {}), - }; - }; -}; diff --git a/packages/appduct/src/mcp/tool-namespace.ts b/packages/appduct/src/mcp/tool-namespace.ts deleted file mode 100644 index fbebcc11..00000000 --- a/packages/appduct/src/mcp/tool-namespace.ts +++ /dev/null @@ -1,95 +0,0 @@ -/** - * Builds the effective MCP tool list from live daemon sessions and their tool registries - * (ARCHITECTURE.md §9): exactly one live session exposes tools under their - * own names; two or more namespace every tool as `__` so names never collide across - * devices. - * - * The `__` separator is unambiguous to split back apart even though a tool's own name - * (`[a-zA-Z0-9_-]{1,64}`, `daemon/registry.ts`) may itself contain underscores: aliases - * (`daemon/sessions.ts`'s `slugifyDeviceModel`/`dedupeAlias`) only ever contain lowercase - * alphanumerics and hyphens, so the alias half of a namespaced name never itself contains `__` — - * the first occurrence of `__` in a namespaced name is always the alias/tool-name boundary. - */ - -import type { EffectivePolicyDecision, SessionSummary, ToolDescriptor, ToolsListEntry } from "@appduct/shared"; - -export const TOOL_NAMESPACE_SEPARATOR = "__"; - -export type NamespacedTool = { - /** The name exposed to MCP clients: `descriptor.name` verbatim when exactly one session is live, - * else `__`. */ - mcpName: string; - /** The session alias `tools.call`'s `selector` should target for this tool. */ - selector: string; - descriptor: ToolDescriptor; - /** The effective policy decision for this tool right now (ARCHITECTURE.md §12), as resolved by - * the daemon's `tools.list` — carried through so the MCP server knows whether to ask for - * consent via elicitation before `tools.call`, without a second round trip. */ - policy: EffectivePolicyDecision; -}; - -/** Builds the namespaced (or not) tool list for the current set of live sessions. `toolsByAlias` - * is looked up by `session.alias` (not `sessionId`) to match `tools.list`'s `selector` semantics. */ -export const buildNamespacedTools = ( - sessions: readonly SessionSummary[], - toolsByAlias: ReadonlyMap, -): NamespacedTool[] => { - const namespaced = sessions.length > 1; - const tools: NamespacedTool[] = []; - - for (const session of sessions) { - for (const entry of toolsByAlias.get(session.alias) ?? []) { - // Explicit pick (not a `{ policy, ...descriptor }` rest-spread) so a future non-descriptor - // field added to `ToolsListEntry` can't silently leak into `descriptor` and from there into - // the MCP tool surface and the snapshot key below. - const descriptor: ToolDescriptor = { - name: entry.name, - description: entry.description, - input_schema: entry.input_schema, - output_schema: entry.output_schema, - annotations: entry.annotations, - timeout_ms: entry.timeout_ms, - }; - - tools.push({ - mcpName: namespaced ? `${session.alias}${TOOL_NAMESPACE_SEPARATOR}${descriptor.name}` : descriptor.name, - selector: session.alias, - descriptor, - policy: entry.policy, - }); - } - } - - return tools; -}; - -export const findNamespacedTool = ( - tools: readonly NamespacedTool[], - mcpName: string, -): NamespacedTool | undefined => { - return tools.find((tool) => tool.mcpName === mcpName); -}; - -/** A stable, order-independent key used to detect whether the effective tool list actually - * changed (name, target session, and the descriptor fields a client can see) — used to decide - * whether to fire `notifications/tools/list_changed` rather than on every qualifying daemon event. - * - * `timeout_ms` is deliberately excluded: it never reaches the MCP `Tool` JSON (`tool-mapping.ts`), - * so an app re-registering a tool with only a different deadline changes nothing a client could - * observe, and telling it the list changed would just make it re-fetch an identical list. */ -export const namespacedToolsSnapshotKey = (tools: readonly NamespacedTool[]): string => { - const sorted = tools - .map((tool) => { - const { timeout_ms: _timeoutMs, ...clientVisibleDescriptor } = tool.descriptor; - - return { - name: tool.mcpName, - selector: tool.selector, - descriptor: clientVisibleDescriptor, - policy: tool.policy, - }; - }) - .sort((a, b) => a.name.localeCompare(b.name)); - - return JSON.stringify(sorted); -}; diff --git a/packages/appduct/src/output.ts b/packages/appduct/src/output.ts index 33377132..b7e0e7fa 100644 --- a/packages/appduct/src/output.ts +++ b/packages/appduct/src/output.ts @@ -2,6 +2,7 @@ import pc from "picocolors"; import { formatAgentWebSocketUrl, renderToolSignature, + summarizeToolDescription, type EventNotification, type SessionSummary, type ToolDescriptor, @@ -155,25 +156,6 @@ const isToolsListing = (data: ToolsCommandData): data is ToolsListing => { return typeof data === "object" && data !== null && Array.isArray((data as ToolsListing).tools); }; -/** First line of a tool's description, trimmed and capped — the summary listing shows only this, - * not the full (possibly multi-line, up to `MAX_TOOL_DESCRIPTION_LENGTH`) text; `tools `/ - * `--full` still show it in full. */ -const MAX_LISTED_DESCRIPTION_LENGTH = 120; - -const summarizeDescription = (description: string): string => { - // Any line break ends the first line (`\r` alone included), and remaining control characters - // are dropped: the description is app-supplied text printed straight to a terminal. - const firstLine = (description.split(/\r\n|[\n\r\u2028\u2029]/u)[0] ?? "") - .replace(/\p{Cc}/gu, "") - .trim(); - // Cut by code point, never through the middle of a surrogate pair. - const codePoints = Array.from(firstLine); - - return codePoints.length > MAX_LISTED_DESCRIPTION_LENGTH - ? `${codePoints.slice(0, MAX_LISTED_DESCRIPTION_LENGTH).join("")}…` - : firstLine; -}; - /** "No tools registered"/"No tools match" for an empty listing (compact or `--full` — both share * this line, only the header differs). */ const renderEmptyToolsLine = (data: ToolsListing): string => { @@ -210,7 +192,7 @@ const renderToolSummaryTable = (colors: ColorPalette, data: ToolsListing): strin colors.green("Tools"), ...data.tools.flatMap((tool) => { const tag = tool.policy === "allow" ? "" : ` [${tool.policy}]`; - return [` ${renderToolSignature(tool)}${tag}`, ` ${summarizeDescription(tool.description)}`]; + return [` ${renderToolSignature(tool)}${tag}`, ` ${summarizeToolDescription(tool.description)}`]; }), ...renderTruncationLine(data), "", diff --git a/packages/react-native/README.md b/packages/react-native/README.md index ca843398..3b2a51f1 100644 --- a/packages/react-native/README.md +++ b/packages/react-native/README.md @@ -83,7 +83,7 @@ Mount it near app startup, or register from a module that loads then. The host c The hook registers once per mount and re-registers only when the registration itself changes, routing every call through the latest render's handler — so `deps` is an optional override, not something each call site has to get right. See [Registration is per mount, not per render](https://github.com/callstackincubator/appduct/blob/main/docs/TOOLS.md#registration-is-per-mount-not-per-render). -Keep both schemas object-rooted: MCP requires it, and a schema that isn't degrades gracefully rather than taking your whole tool list down — see [Keep both schemas object-rooted](https://github.com/callstackincubator/appduct/blob/main/docs/TOOLS.md#keep-both-schemas-object-rooted). A call gets 10 seconds unless the registration declares `timeoutMs` — see [Long-running tools](https://github.com/callstackincubator/appduct/blob/main/docs/TOOLS.md#long-running-tools). +Make `inputSchema` accept an object: a call's arguments are always a JSON object — see [Make the input schema accept an object](https://github.com/callstackincubator/appduct/blob/main/docs/TOOLS.md#make-the-input-schema-accept-an-object). A call gets 10 seconds unless the registration declares `timeoutMs` — see [Long-running tools](https://github.com/callstackincubator/appduct/blob/main/docs/TOOLS.md#long-running-tools). To keep a destructive tool out of some build variants, pass `{ enabled }` rather than wrapping the hook in an `if` — see [Gating a tool by build variant](https://github.com/callstackincubator/appduct/blob/main/docs/SECURITY.md#gating-a-tool-by-build-variant). @@ -142,7 +142,7 @@ Omit the session selector when only one session is active; pass an alias or sess ## Going further - [Trust modes](https://github.com/callstackincubator/appduct/blob/main/docs/SECURITY.md#trust-modes) and [Configuring trust](https://github.com/callstackincubator/appduct/blob/main/docs/SECURITY.md#configuring-trust) — pins, plugin options, bare-RN native keys. -- [Registering tools](https://github.com/callstackincubator/appduct/blob/main/docs/TOOLS.md) — schema forms, what re-registers, MCP's object-rooted requirement, `timeoutMs`. +- [Registering tools](https://github.com/callstackincubator/appduct/blob/main/docs/TOOLS.md) — schema forms, what re-registers, input schemas that accept an object, `timeoutMs`. - [Gating a tool by build variant](https://github.com/callstackincubator/appduct/blob/main/docs/SECURITY.md#gating-a-tool-by-build-variant) — `enabled`, and why `__DEV__` is wrong here. - [Build variants](https://github.com/callstackincubator/appduct/blob/main/docs/BUILD-VARIANTS.md) — `APPDUCT_ENABLED`, autolinking exclusion, compiling Appduct out of production builds. - [What a build without the native module does](https://github.com/callstackincubator/appduct/blob/main/docs/SECURITY.md#what-a-build-without-the-native-module-does). diff --git a/packages/react-native/src/__tests__/schema.test.ts b/packages/react-native/src/__tests__/schema.test.ts index b22ab503..f1a08bc3 100644 --- a/packages/react-native/src/__tests__/schema.test.ts +++ b/packages/react-native/src/__tests__/schema.test.ts @@ -788,40 +788,28 @@ const objectShape = { type: "object" } as const; const warningsMentioning = (warnings: string[][], needle: string): string[][] => warnings.filter((args) => args.some((arg) => arg.includes(needle))); -describe("toToolDescriptor: schemas MCP cannot represent (issue #26)", () => { - test("warns once for a non-object-rooted output schema but still carries the real schema", () => { - const definition = { - name: "non-object-output", - description: "d", - outputSchema: normalizeToolSchema( - withExportedShape(objectShape, { - type: "array", - items: { type: "string" }, - }), - "l", - ), - }; - +describe("toToolDescriptor: non-object-rooted schemas", () => { + test("a non-object-rooted output schema is carried as-is, with no warning", () => { const warnings = withWarningsCaptured(() => { - const first = toToolDescriptor(definition); - const second = toToolDescriptor(definition); - - // The descriptor is unchanged: the CLI, the JS client and app-side result validation all - // keep the real schema; only the MCP surface degrades. - expect(first.output_schema).toEqual({ - type: "array", - items: { type: "string" }, + const descriptor = toToolDescriptor({ + name: "non-object-output", + description: "d", + outputSchema: normalizeToolSchema( + withExportedShape(objectShape, { + type: "array", + items: { type: "string" }, + }), + "l", + ), }); - expect(second.output_schema).toEqual({ + + expect(descriptor.output_schema).toEqual({ type: "array", items: { type: "string" }, }); }); - const relevant = warningsMentioning(warnings, "non-object-output"); - expect(relevant).toHaveLength(1); - expect(relevant[0]!.join(" ")).toContain("output"); - expect(relevant[0]!.join(" ")).toContain("no schema describing it"); + expect(warningsMentioning(warnings, "non-object-output")).toEqual([]); }); test("warns once for a non-object-rooted input schema", () => { @@ -841,23 +829,47 @@ describe("toToolDescriptor: schemas MCP cannot represent (issue #26)", () => { const relevant = warningsMentioning(warnings, "non-object-input"); expect(relevant).toHaveLength(1); expect(relevant[0]!.join(" ")).toContain("input"); + expect(relevant[0]!.join(" ")).toContain("no call can satisfy it"); }); test.each([ - ["union-output", "anyOf"], - ["discriminated-union-output", "oneOf"], - ["intersection-output", "allOf"], + ["union-input", "anyOf"], + ["discriminated-union-input", "oneOf"], + ["intersection-input", "allOf"], ])( - "a %s export (%s, no root type) warns even though every branch is an object", + "a %s export (%s, no root type) stays silent: its branches can be objects", (toolName, keyword) => { const warnings = withWarningsCaptured(() => { toToolDescriptor({ name: toolName, description: "d", - outputSchema: normalizeToolSchema( - withExportedShape(objectShape, { - [keyword]: [{ type: "object" }, { type: "object" }], - }), + inputSchema: normalizeToolSchema( + withExportedShape( + { [keyword]: [{ type: "object" }, { type: "object" }] }, + objectShape, + ), + "l", + ), + }); + }); + + expect(warningsMentioning(warnings, toolName)).toEqual([]); + }, + ); + + test.each([ + ["array-input", { type: "array", items: { type: "string" } }], + ["number-input", { type: "number" }], + ["nullable-string-input", { type: ["string", "null"] }], + ])( + "a %s root that rules out an object warns", + (toolName, schema) => { + const warnings = withWarningsCaptured(() => { + toToolDescriptor({ + name: toolName, + description: "d", + inputSchema: normalizeToolSchema( + withExportedShape(schema, objectShape), "l", ), }); diff --git a/packages/react-native/src/schema.ts b/packages/react-native/src/schema.ts index 5f21b7b8..50d1dfc3 100644 --- a/packages/react-native/src/schema.ts +++ b/packages/react-native/src/schema.ts @@ -1,6 +1,5 @@ import { clampToolTimeoutMs, - isObjectRootedSchema, MAX_TOOL_TIMEOUT_MS, MIN_TOOL_TIMEOUT_MS, type StandardSchemaV1, @@ -72,50 +71,45 @@ const reportShapelessSchema = ( return undefined; }; -/** Dedupes the two dev warnings below across repeated registrations of the same tool name. */ -const nonObjectOutputWarningsSeen = new Set(); +/** Dedupes the dev warning below across repeated registrations of the same tool name. */ const nonObjectInputWarningsSeen = new Set(); /** - * Issue #26: MCP's `Tool` wire shape requires both `inputSchema.type` and `outputSchema.type` to be - * the literal `"object"`, so a schema exported as anything else (`z.array`, `z.string`, a - * `z.union`'s `anyOf`, a `z.discriminatedUnion`'s `oneOf` or a `z.intersection`'s `allOf` — the - * last two even when every branch is an object) cannot be put on the wire as-is. The tool is still - * registered and still callable, and the descriptor still carries the real schema for the CLI and - * the JS client; only the MCP surface degrades. Warn at registration time so an app author learns - * it here rather than from an agent. + * A tool call always carries its `args` as a JSON object (the daemon rejects anything else), so an + * input schema whose root `type` rules an object out (`z.string`, `z.number`, `z.array`, ...) can + * never be satisfied by a call. Warn at registration time so an app author learns it here rather + * than from an agent. The tool is still registered, and the descriptor still carries the schema as + * exported. * - * This is a best-effort dev-time hint, not the authority. The MCP server makes the real decision - * by parsing the composed tool with the SDK's own `ToolSchema` (`mcp/tool-mapping.ts`), which - * rejects a little more than the root-type check available here — this package cannot depend on - * the MCP SDK. Every shape zod itself can export is covered by the check below. + * A schema with no root `type` — a `z.union`'s `anyOf`, a `z.discriminatedUnion`'s `oneOf`, a + * `z.intersection`'s `allOf` — is left alone: its branches can be objects, and args are validated + * app-side by the schema itself. An output schema has no constraint at all: a result can be any + * JSON value. */ -const warnNonObjectRootedSchema = ( +const rulesOutObjectArgs = (schema: ToolSchemaDescriptor): boolean => { + const { type } = schema; + + if (typeof type === "string") { + return type !== "object"; + } + + return Array.isArray(type) && !type.includes("object"); +}; + +const warnNonObjectRootedInputSchema = ( toolName: string, - mode: "input" | "output", schema: ToolSchemaDescriptor, ): void => { - const seen = - mode === "output" - ? nonObjectOutputWarningsSeen - : nonObjectInputWarningsSeen; - - if (seen.has(toolName)) { + if (nonObjectInputWarningsSeen.has(toolName)) { return; } - seen.add(toolName); - - const consequence = - mode === "output" - ? "MCP drops it from tools/list, so agents get the result with no schema describing it." - : "MCP replaces it with a permissive empty object schema, so agents cannot see the tool's " + - "real arguments."; + nonObjectInputWarningsSeen.add(toolName); logger.devWarn( - `Tool "${toolName}" exports a JSON Schema for its ${mode} that is not rooted at ` + - `type "object" (got ${JSON.stringify(schema.type ?? null)}). ${consequence} ` + - `Wrap the ${mode} in an object schema (for example z.object({ result: ... })) to keep the ` + - "full shape over MCP.", + `Tool "${toolName}" exports a JSON Schema for its input that is not rooted at ` + + `type "object" (got ${JSON.stringify(schema.type ?? null)}). Tool calls always pass ` + + "their args as a JSON object, so no call can satisfy it. Wrap the input in an object " + + "schema (for example z.object({ value: ... })).", ); }; @@ -563,12 +557,8 @@ export const toToolDescriptor = ( definition.name, ); - if (inputSchema !== undefined && !isObjectRootedSchema(inputSchema)) { - warnNonObjectRootedSchema(definition.name, "input", inputSchema); - } - - if (outputSchema !== undefined && !isObjectRootedSchema(outputSchema)) { - warnNonObjectRootedSchema(definition.name, "output", outputSchema); + if (inputSchema !== undefined && rulesOutObjectArgs(inputSchema)) { + warnNonObjectRootedInputSchema(definition.name, inputSchema); } return { diff --git a/packages/react-native/src/useAppductTool.ts b/packages/react-native/src/useAppductTool.ts index aec55484..44f97df3 100644 --- a/packages/react-native/src/useAppductTool.ts +++ b/packages/react-native/src/useAppductTool.ts @@ -126,7 +126,7 @@ export function createUseAppductTool( * only when something that changes the registry entry changed — `name`, `description`, the * exported input/output JSON Schemas, `annotations`, `timeoutMs`, or `options.enabled`. Omitting * `deps` is therefore the correct, cheap default: re-rendering the hosting component does not - * produce `tool_registry_delta` traffic or agent-side `tools/list_changed` notifications. + * produce `tool_registry_delta` traffic. * (`timeoutMs` is app-side only — the daemon never sees it — but it is part of the entry, so a * change to it has to reach the registry.) * diff --git a/packages/shared/src/__tests__/tool-descriptor.test.ts b/packages/shared/src/__tests__/tool-descriptor.test.ts index d579cca9..b343f6f7 100644 --- a/packages/shared/src/__tests__/tool-descriptor.test.ts +++ b/packages/shared/src/__tests__/tool-descriptor.test.ts @@ -2,7 +2,6 @@ import { describe, expect, test } from "vitest"; import { clampToolTimeoutMs, - isObjectRootedSchema, isToolDescriptor, MAX_TOOL_DESCRIPTION_LENGTH, MAX_TOOL_TIMEOUT_MS, @@ -132,68 +131,6 @@ describe("isToolDescriptor", () => { }); }); -/** - * The root-type gate behind the app-side dev warning for issue #26. Deliberately *not* the whole - * MCP rule — the server re-checks with the SDK's own `ToolSchema` (`mcp/tool-mapping.ts`), which - * this package cannot import — so the last case below pins that narrowness as intended, not a bug. - */ -describe("isObjectRootedSchema", () => { - test('accepts a schema rooted at the literal type "object"', () => { - expect(isObjectRootedSchema({ type: "object" })).toBe(true); - expect( - isObjectRootedSchema({ - type: "object", - properties: { echoed: { type: "string" } }, - required: ["echoed"], - additionalProperties: false, - }), - ).toBe(true); - }); - - test("accepts the shapes zod exports for a passthrough object and a record", () => { - expect(isObjectRootedSchema({ type: "object", properties: {}, additionalProperties: {} })).toBe(true); - expect( - isObjectRootedSchema({ - type: "object", - propertyNames: { type: "string" }, - additionalProperties: { type: "number" }, - }), - ).toBe(true); - }); - - test.each([ - ["array", { type: "array", items: { type: "string" } }], - ["string", { type: "string" }], - ["number", { type: "number" }], - ["boolean", { type: "boolean" }], - ["null", { type: "null" }], - ])("rejects a %s schema", (_label, schema) => { - expect(isObjectRootedSchema(schema)).toBe(false); - }); - - test.each([ - ["union (anyOf)", { anyOf: [{ type: "object" }, { type: "object" }] }], - ["discriminated union (oneOf)", { oneOf: [{ type: "object" }, { type: "object" }] }], - ["intersection (allOf)", { allOf: [{ type: "object" }, { type: "object" }] }], - ])("rejects a %s schema even though every branch is an object", (_label, schema) => { - expect(isObjectRootedSchema(schema)).toBe(false); - }); - - test('rejects a union type that merely includes "object"', () => { - expect(isObjectRootedSchema({ type: ["object", "null"] })).toBe(false); - }); - - test("rejects a schema with no type at all, and undefined", () => { - expect(isObjectRootedSchema({})).toBe(false); - expect(isObjectRootedSchema(undefined)).toBe(false); - }); - - test("is only the root-type gate: object-rooted shapes MCP still rejects pass here", () => { - expect(isObjectRootedSchema({ type: "object", properties: { a: true } })).toBe(true); - expect(isObjectRootedSchema({ type: "object", required: "name" })).toBe(true); - }); -}); - describe("clampToolTimeoutMs", () => { test("passes an in-range value through, truncated to whole milliseconds", () => { expect(clampToolTimeoutMs(60_000)).toBe(60_000); diff --git a/packages/shared/src/domains/rpc.ts b/packages/shared/src/domains/rpc.ts index 2597640d..44736765 100644 --- a/packages/shared/src/domains/rpc.ts +++ b/packages/shared/src/domains/rpc.ts @@ -139,6 +139,10 @@ export type SessionsRevokeResult = { ok: true }; // --- tools.list / tools.call --- +/** `tools.list`'s `filter` string cap (ARCHITECTURE.md §5) — generous for a name/description + * substring search, small enough that a malicious/buggy caller can't use it to bloat a request. */ +export const MAX_TOOLS_FILTER_LENGTH = 256; + export type ToolsListParams = SessionSelectorParams & { /** Case-insensitive substring match against name and description. */ filter?: string; @@ -197,7 +201,7 @@ export type ToolsCallResult = { result: unknown; /** The `tool_call`/`tool_call_progress`/`tool_call_finished` correlation id (ARCHITECTURE.md * §7's `call_…` id), exposed so a caller with several in-flight `tools.call`s (e.g. the MCP - * server proxying concurrent `tools/call` requests) can match its own call to the progress events + * server running concurrent `appduct_call_tool` requests) can match its own call to the progress events * it sees on `events.subscribe` without guessing from data shape. */ callId: string; }; diff --git a/packages/shared/src/domains/tool-descriptor.ts b/packages/shared/src/domains/tool-descriptor.ts index 95f21605..3b297f19 100644 --- a/packages/shared/src/domains/tool-descriptor.ts +++ b/packages/shared/src/domains/tool-descriptor.ts @@ -124,22 +124,3 @@ export const isToolDescriptor = (value: unknown): value is ToolDescriptor => { return true; }; - -/** - * Whether an exported JSON Schema is rooted at the literal `type: "object"`. - * - * MCP's `Tool` wire shape (`@modelcontextprotocol/sdk`) declares both `inputSchema.type` and - * `outputSchema.type` as `z.literal("object")` and clients validate the whole `tools/list` result, - * so a single schema rooted at anything else (`z.array`, `z.string`, a `z.union`'s `anyOf`, a - * `z.discriminatedUnion`'s `oneOf` or a `z.intersection`'s `allOf` — the last two even when every - * branch is an object) makes the client reject the *entire* list. - * - * This is the *cheap* gate, for the app-side dev warning in `@appduct/react-native`, which - * cannot depend on the MCP SDK: it catches every shape zod can actually export. MCP constrains - * more than the root type (`properties` must be a record of object subschemas, `required` must be - * an array), so the MCP server's own decision to emit or drop a schema is made by parsing the - * composed tool with the SDK's `ToolSchema` — see `mcp/tool-mapping.ts` — never by this predicate. - */ -export const isObjectRootedSchema = (schema: ToolSchemaDescriptor | undefined): boolean => { - return schema !== undefined && schema.type === "object"; -}; diff --git a/packages/shared/src/domains/tool-signature.ts b/packages/shared/src/domains/tool-signature.ts index e63cec13..5ce1e036 100644 --- a/packages/shared/src/domains/tool-signature.ts +++ b/packages/shared/src/domains/tool-signature.ts @@ -230,11 +230,9 @@ const renderParamEntry = ( }; /** The `(...)` params group. An absent `input_schema` is `()`: the SDKs omit it for a tool that - * takes no input, and the MCP server maps it to an empty object schema (`tool-mapping.ts`). Unlike - * {@link renderObjectType}, a present `input_schema` not rooted at `type: "object"` is always - * `(...)` — MCP requires an object-rooted input schema (`tool-descriptor.ts`'s - * `isObjectRootedSchema`), so anything else means this renderer cannot describe the call's - * arguments, not that there are none. */ + * takes no input. Unlike {@link renderObjectType}, a present `input_schema` not rooted at + * `type: "object"` is always `(...)` — a call's args are always a JSON object, so anything else + * means this renderer cannot describe the call's arguments, not that there are none. */ const renderParams = (inputSchema: ToolSchemaDescriptor | undefined): string => { if (inputSchema === undefined) { return "()"; @@ -276,3 +274,22 @@ export const renderToolSignature = ( return `${name}(...)`; } }; + +/** The listing's per-tool summary length, in code points. The full description (up to + * `MAX_TOOL_DESCRIPTION_LENGTH`) stays available from a single-tool lookup. */ +export const MAX_TOOL_SUMMARY_LENGTH = 120; + +/** + * A tool description's first line, for a listing (`appduct tools`, `appduct_list_tools`): any line + * break ends it (`\r` alone included), remaining control characters are dropped since the text is + * app-supplied and may be printed straight to a terminal, and it is capped at + * {@link MAX_TOOL_SUMMARY_LENGTH} code points, never cut through a surrogate pair. + */ +export const summarizeToolDescription = (description: string): string => { + const firstLine = (description.split(/\r\n|[\n\r\u2028\u2029]/u)[0] ?? "").replace(/\p{Cc}/gu, "").trim(); + const codePoints = Array.from(firstLine); + + return codePoints.length > MAX_TOOL_SUMMARY_LENGTH + ? `${codePoints.slice(0, MAX_TOOL_SUMMARY_LENGTH).join("")}…` + : firstLine; +}; diff --git a/skills/appduct/SKILL.md b/skills/appduct/SKILL.md index 1fccbd39..361b0e7e 100644 --- a/skills/appduct/SKILL.md +++ b/skills/appduct/SKILL.md @@ -124,9 +124,20 @@ management tools instead of the CLI commands above. **Call `appduct_connect` with no arguments.** It auto-detects a booted iOS simulator or attached Android device and delivers the link straight to it — no human involved. A result with `delivered: true` is done; go on to `appduct_wait_for_session({ sessionId })`, -which blocks until the device connects (or returns immediately if it already has). After -that the app's own tools appear directly in `tools/list` — call them with `tools/call` -like any other MCP tool. +which blocks until the device connects (or returns immediately if it already has). + +The app's own tools are not MCP tools of their own. Reach them through three built-ins that +mirror the CLI: + +1. `appduct_list_tools` lists them as one-line signatures with each tool's policy (like + `appduct tools`). On a large app, narrow with `filter`, or page with `limit`/`offset`. +2. `appduct_describe_tool({ name })` shows one tool's full input and output schema (like + `appduct tools `). +3. `appduct_call_tool({ name, args })` runs it (like `appduct invoke`). + +With more than one device connected, pass `selector` (the session alias or id) to each of them. A +tool with policy `"prompt"` asks the user to approve every call; if the user declines, don't +retry it on your own. Pass `target: "android"` / `"ios-sim"` (plus `device` — an adb serial or simulator udid) only to override that choice, e.g. when several devices are up and the result said so. @@ -164,7 +175,7 @@ the `wss://` port or the daemon's key is being rotated. ## Declaring tools The app must register tools before `appduct tools` / `appduct invoke` (or MCP -`tools/call`) can do anything useful. Register with `registerTool` or `useAppductTool`: +`appduct_call_tool`) can do anything useful. Register with `registerTool` or `useAppductTool`: ```ts import { registerTool } from "@appduct/react-native"; @@ -191,12 +202,13 @@ registerTool({ | A raw JSON Schema object (no `~standard`, at least one JSON Schema keyword) | **no** — args pass through | the object, verbatim | A bare Zod 3 / plain valibot schema (Standard Schema, no exporter) **throws in `__DEV__`**: -it would otherwise register a shapeless tool that `tools/list` reports as taking any +it would otherwise register a shapeless tool that `appduct tools` reports as taking any object. Pair it, or pass raw JSON Schema. -An input schema must be **object-typed at its root** to be callable over MCP: a root -`enum`, `const`, `$ref` or `anyOf` is legal JSON Schema but gives the agent no named -arguments to pass (issue #34). +An input schema must **accept a JSON object**, since a call's args always are one: a root +`type` of string, number or array can never be satisfied (issue #34). A root `anyOf`/`oneOf` +of objects works, but its signature shows as `(...)`, so read the full schema +(`appduct tools `) before calling it. ## Notes