diff --git a/CHANGELOG.md b/CHANGELOG.md index d4e5bdc..b0a63bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,14 @@ skill in `.claude/skills/`) turns that section into a versioned heading. ## Unreleased +- **Fix: an ungrouped tool reports `group: null` to every reader.** `appduct_describe_tool` + dropped the key instead of reporting `null`, so it disagreed with `appduct_list_tools` about + the same tool. `ToolDescriptor.group` is + `string | undefined` again — it is the registration type, and registering `null` has always been + rejected — so the type app authors write against (`appduct/client`, the React Native SDK's + `getRegisteredTools()`) no longer admits a value that throws. Code reading a `tools.list` entry + takes the new `ListedToolDescriptor` (or `ToolsListEntry`, which adds `policy`), where `group` is + `string | null`; `appduct/client`'s `tools()` now returns those entries. - **Fix: the first Appduct command on a clean machine no longer fails with a bare `ENOENT`.** Nothing created the state directory before the auto-spawn path wrote into it: `~/.appduct` is created by `startDaemon`, but the spawn-lock and `daemon.log`'s fd are opened by the *parent* diff --git a/packages/appduct/README.md b/packages/appduct/README.md index 9490b1c..ed8c2ba 100644 --- a/packages/appduct/README.md +++ b/packages/appduct/README.md @@ -232,7 +232,7 @@ import { connect } from "appduct/client"; // auto-spawns the daemon and picks the single session, or pass { selector: "pixel-8" } to target one explicitly const app = await connect(); -await app.tools(); // ToolDescriptor[] +await app.tools(); // ListedToolDescriptor[]; an ungrouped tool's group is null const { total } = await app.call("sum", { a: 2, b: 3 }); const { payload } = await app.waitForEvent("checkout_done", { timeoutMs: 5_000 }); app.close(); diff --git a/packages/appduct/src/__tests__/app-client.test.ts b/packages/appduct/src/__tests__/app-client.test.ts index 213eb8f..dd22988 100644 --- a/packages/appduct/src/__tests__/app-client.test.ts +++ b/packages/appduct/src/__tests__/app-client.test.ts @@ -21,12 +21,15 @@ const streamAnswering = (result: unknown): DaemonStream => { describe("AppClient.tools()", () => { test("unwraps the `{ tools, total }` tools.list result", async () => { - const client = makeAppClient(streamAnswering({ tools: [toolEntry], total: 1 }), "s1"); - expect(await client.tools()).toEqual([toolEntry]); + const listed = { ...toolEntry, group: "diagnostics" }; + const client = makeAppClient(streamAnswering({ tools: [listed], total: 1 }), "s1"); + expect(await client.tools()).toEqual([listed]); }); test("still accepts a bare array from a daemon that predates `{ tools, total }`", async () => { const client = makeAppClient(streamAnswering([toolEntry]), "s1"); - expect(await client.tools()).toEqual([toolEntry]); + // That daemon predates tool groups too and sends no `group` key. Every entry carries one, so + // `tool.group === null` answers "ungrouped" here as it does anywhere else. + expect(await client.tools()).toEqual([{ ...toolEntry, group: null }]); }); }); diff --git a/packages/appduct/src/__tests__/mcp-daemon-fake.ts b/packages/appduct/src/__tests__/mcp-daemon-fake.ts index 0aae6a1..7b329ed 100644 --- a/packages/appduct/src/__tests__/mcp-daemon-fake.ts +++ b/packages/appduct/src/__tests__/mcp-daemon-fake.ts @@ -135,11 +135,15 @@ export const createFakeDaemon = (): FakeDaemon => { alias: options.alias, sessionId, setTools: (tools) => { - const entries = tools.map((tool) => ({ + const entries: FakeToolEntry[] = tools.map((tool) => ({ description: "A test tool.", policy: "allow" as const, ...tool, - })) as FakeToolEntry[]; + // The daemon normalises an ungrouped tool to `group: null` on every entry it serves + // (`daemon/daemon.ts`), so the fake does too. Without it these entries would carry a + // shape no daemon sends, and a reader that drops the key would pass here. + group: tool.group ?? null, + })); toolsByAlias.set(options.alias, entries); summary.toolCount = entries.length; diff --git a/packages/appduct/src/__tests__/mcp-server.test.ts b/packages/appduct/src/__tests__/mcp-server.test.ts index bed5cf2..9146aed 100644 --- a/packages/appduct/src/__tests__/mcp-server.test.ts +++ b/packages/appduct/src/__tests__/mcp-server.test.ts @@ -26,9 +26,10 @@ import { type CallToolResult, } from "@modelcontextprotocol/sdk/types.js"; -import { RPC_METHODS } from "@appduct/shared"; +import { RPC_METHODS, type ToolsListResult } from "@appduct/shared"; import { createMcpServer, type McpServerHandle } from "../mcp/server.js"; +import type { DaemonStream } from "../rpc/client.js"; import { createFakeDaemon, toolError, type FakeDaemon } from "./mcp-daemon-fake.js"; const mcpHandles: McpServerHandle[] = []; @@ -52,7 +53,7 @@ const BUILTIN_TOOL_NAMES = [ /** 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 * server has to a daemon and it is the fake's. */ -const startServerWithClient = async (daemon: FakeDaemon): Promise => { +const startServerWithClient = async (daemon: Pick): Promise => { const handle = await createMcpServer({ stateDir: "/nonexistent-state-dir", openStream: daemon.openStream, @@ -70,6 +71,30 @@ const startServerWithClient = async (daemon: FakeDaemon): Promise => { return client; }; +/** `daemon` as a daemon that predates tool groups serves it: `tools.list` comes back with no + * `groups` summary and no `group` key on any entry, which is the only input that tells a reader + * that normalises `group` apart from one that passes it through. */ +const openPreGroupsStream = (daemon: FakeDaemon) => { + return async (): Promise => { + const stream = await daemon.openStream(); + const { call } = stream; + + return { + ...stream, + call: async (method: string, params?: unknown): Promise => { + const result = await call(method, params); + + if (method !== RPC_METHODS.toolsList) { + return result; + } + + const { groups: _groups, tools, ...rest } = result as ToolsListResult; + return { ...rest, tools: tools.map(({ group: _group, ...entry }) => entry) } as TResult; + }, + }; + }; +}; + const callBuiltin = async (client: Client, name: string, args: Record): Promise => { return client.request({ method: "tools/call", params: { name, arguments: args } }, CallToolResultSchema); }; @@ -212,31 +237,8 @@ describe("mcp: appduct_list_tools", () => { test("against a daemon that predates groups, asking for a group fails instead of listing everything", async () => { const daemon = createFakeDaemon(); daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "ping" }]); - // An older daemon answers tools.list with no `groups` and ignores `group`. - const openOldStream = async () => { - const stream = await daemon.openStream(); - const call = stream.call; - return { - ...stream, - call: async (method: string, params?: unknown): Promise => { - const result = await call(method, params); - - if (method === RPC_METHODS.toolsList) { - const { groups: _groups, ...rest } = result as Record; - return rest as TResult; - } - return result; - }, - }; - }; - - const handle = await createMcpServer({ stateDir: "/nonexistent-state-dir", openStream: openOldStream, env: {} }); - mcpHandles.push(handle); - const [serverTransport, clientTransport] = InMemoryTransport.createLinkedPair(); - await handle.connect(serverTransport); - const client = new Client({ name: "test-client", version: "0.0.0" }); - await client.connect(clientTransport); + const client = await startServerWithClient({ openStream: openPreGroupsStream(daemon) }); const plain = await callBuiltin(client, "appduct_list_tools", {}); expect(plain.structuredContent).toMatchObject({ tools: [{ name: "ping" }] }); @@ -373,6 +375,35 @@ describe("mcp: appduct_describe_tool", () => { }); }); + test("reports an ungrouped tool's group as null, the value appduct_list_tools reports for it", async () => { + const daemon = createFakeDaemon(); + daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "echo", description: "Echoes its input." }]); + + const client = await startServerWithClient(daemon); + const result = await callBuiltin(client, "appduct_describe_tool", { name: "echo" }); + + expect(result.isError).not.toBe(true); + expect(result.structuredContent).toEqual({ + session: "pixel-8", + signature: "echo()", + policy: "allow", + name: "echo", + description: "Echoes its input.", + group: null, + }); + }); + + test("against a daemon that predates groups, an ungrouped tool's group is still null", async () => { + const daemon = createFakeDaemon(); + daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "ping", description: "Pings." }]); + + const client = await startServerWithClient({ openStream: openPreGroupsStream(daemon) }); + const result = await callBuiltin(client, "appduct_describe_tool", { name: "ping" }); + + expect(result.isError).not.toBe(true); + expect(result.structuredContent).toMatchObject({ name: "ping", group: null }); + }); + 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" }]); @@ -633,3 +664,24 @@ describe("mcp: appduct_call_tool", () => { expect(stale).toContain("unknown_session"); }); }); + +/** + * Everything above is only worth as much as the fake's fidelity to the daemon. The daemon + * normalises an ungrouped tool's `group` to `null` on every `tools.list` entry + * (`daemon/daemon.ts`), so a fake that left the key absent would let a reader that drops the key + * pass its tests against a shape no daemon ever sends. + */ +describe("mcp: the in-memory daemon fake", () => { + test("lists an ungrouped tool with a null group, the way the daemon serves one", async () => { + const daemon = createFakeDaemon(); + daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "echo" }, { name: "pay", group: "checkout" }]); + + const stream = await daemon.openStream(); + const result = await stream.call(RPC_METHODS.toolsList, { selector: "pixel-8" }); + + expect(result.tools.map((entry) => [entry.name, entry.group])).toEqual([ + ["echo", null], + ["pay", "checkout"], + ]); + }); +}); diff --git a/packages/appduct/src/cli/result-types.ts b/packages/appduct/src/cli/result-types.ts index 12504f7..dd85a85 100644 --- a/packages/appduct/src/cli/result-types.ts +++ b/packages/appduct/src/cli/result-types.ts @@ -10,8 +10,8 @@ import type { EffectivePolicyDecision, EventNotification, SessionSummary, - ToolDescriptor, ToolGroupSummary, + ToolsListEntry, ToolsListResult, } from "@appduct/shared"; @@ -167,9 +167,10 @@ export type ToolGroupsListing = { total: number; }; -/** `appduct tools`: a listing, a groups summary (`--groups`), or a single descriptor when a tool - * name resolved to a detail lookup. */ -export type ToolsCommandData = ToolsListing | ToolGroupsListing | ToolDescriptor; +/** `appduct tools`: a listing, a groups summary (`--groups`), or a single entry when a tool name + * resolved to a detail lookup. The detail form is the daemon's own listing entry, picked out of + * `tools.list` — not a registration, so its `group` is `null` for an ungrouped tool. */ +export type ToolsCommandData = ToolsListing | ToolGroupsListing | ToolsListEntry; /** `appduct invoke`: the tool's raw result payload, printed as-is. */ export type InvokeCommandData = unknown; diff --git a/packages/appduct/src/client/app-client.ts b/packages/appduct/src/client/app-client.ts index 6a23af4..162227c 100644 --- a/packages/appduct/src/client/app-client.ts +++ b/packages/appduct/src/client/app-client.ts @@ -13,7 +13,7 @@ import { RPC_METHODS, type EventNotification, type EventsSinceResult, - type ToolDescriptor, + type ListedToolDescriptor, type ToolsCallResult, type ToolsListResult, } from "@appduct/shared"; @@ -93,8 +93,9 @@ export type AppEvent = { export type AppClient = { readonly sessionId: string; - /** `tools.list` for this session. */ - tools(): Promise; + /** `tools.list` for this session. An entry spells an ungrouped tool's `group` as `null`, + * where a registration omits it. */ + tools(): Promise; /** `tools.call`; rejects with a {@link AppductError} whose `type` preserves the wire error * type verbatim (e.g. `"tool_timeout"`, `"policy_denied"`, `"session_suspended"`). */ @@ -164,7 +165,7 @@ export const makeAppClient = (stream: DaemonStream, sessionId: const client = { sessionId, - tools: async (): Promise => { + tools: async (): Promise => { try { // No `filter`/`limit`/`offset`: this client's public `tools()` contract is "every tool on // this session", unchanged by `tools.list`'s daemon-side paging (added for the CLI). @@ -173,7 +174,10 @@ export const makeAppClient = (stream: DaemonStream, sessionId: }); // Unlike the CLI, this client runs no daemon version check, so it can meet a daemon from // before `tools.list` returned `{ tools, total }` — one that still answers a bare array. - return Array.isArray(result) ? result : result.tools; + const entries = Array.isArray(result) ? result : result.tools; + // That daemon predates tool groups as well and sends no `group` key, so normalise it the + // way the daemon itself would. Callers are told every entry carries one. + return entries.map((entry) => ({ ...entry, group: entry.group ?? null })); } catch (error) { throw toAppductError(error); } diff --git a/packages/appduct/src/client/index.ts b/packages/appduct/src/client/index.ts index 3d3f658..57eb54f 100644 --- a/packages/appduct/src/client/index.ts +++ b/packages/appduct/src/client/index.ts @@ -23,4 +23,4 @@ export type { ToolMap, WaitForEventOptions, } from "./app-client.js"; -export type { AgentEndpoint, ErrorType, ToolDescriptor } from "@appduct/shared"; +export type { AgentEndpoint, ErrorType, ListedToolDescriptor, ToolDescriptor } from "@appduct/shared"; diff --git a/packages/appduct/src/commands/tools.ts b/packages/appduct/src/commands/tools.ts index 71545fc..7086a34 100644 --- a/packages/appduct/src/commands/tools.ts +++ b/packages/appduct/src/commands/tools.ts @@ -16,7 +16,7 @@ * unpaged registry, so a name lookup can never miss a tool that paging would have left off a page. */ -import { RPC_METHODS, type ToolDescriptor, type ToolsListResult } from "@appduct/shared"; +import { RPC_METHODS, type ToolsListEntry, type ToolsListResult } from "@appduct/shared"; import type { CliResult, ToolGroupsListing, ToolsCommandData, ToolsListing } from "../cli/result-types.js"; import { connectionError, usageError } from "../errors.js"; @@ -57,7 +57,9 @@ const listTools = ( ); }; -const findTool = (tools: ToolDescriptor[], name: string): ToolDescriptor | undefined => { +/** Over daemon *listing* entries, never registrations: an entry spells an ungrouped tool's group + * `null`, which `ToolDescriptor` does not admit. */ +const findTool = (tools: ToolsListEntry[], name: string): ToolsListEntry | undefined => { return tools.find((tool) => tool.name === name); }; diff --git a/packages/appduct/src/mcp/app-tools.ts b/packages/appduct/src/mcp/app-tools.ts index 1f6a043..b538734 100644 --- a/packages/appduct/src/mcp/app-tools.ts +++ b/packages/appduct/src/mcp/app-tools.ts @@ -19,8 +19,8 @@ import { summarizeToolDescription, TOOL_GROUP_PATTERN, type EffectivePolicyDecision, + type ListedToolDescriptor, type SessionsDescribeResult, - type ToolDescriptor, type ToolsListEntry, type ToolsListResult, } from "@appduct/shared"; @@ -119,7 +119,7 @@ export const CALL_TOOL_TOOL_DESCRIPTOR = { export type ResolvedAppTool = { sessionId: string; alias: string; - descriptor: ToolDescriptor; + descriptor: ListedToolDescriptor; policy: EffectivePolicyDecision; }; @@ -180,7 +180,7 @@ const asRequiredString = (value: unknown, field: string): string => { /** 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 => { +const toDescriptor = (entry: ToolsListEntry): ListedToolDescriptor => { return { name: entry.name, description: entry.description, @@ -188,7 +188,10 @@ const toDescriptor = (entry: ToolsListEntry): ToolDescriptor => { output_schema: entry.output_schema, annotations: entry.annotations, timeout_ms: entry.timeout_ms, - group: entry.group, + // Normalised the same way the listing normalises it, so `appduct_describe_tool` never + // disagrees with `appduct_list_tools` about whether a tool has a group. The `?? null` also + // covers a daemon that predates groups and omits the key entirely. + group: entry.group ?? null, }; }; diff --git a/packages/appduct/src/output.ts b/packages/appduct/src/output.ts index 6d9a76f..50b2465 100644 --- a/packages/appduct/src/output.ts +++ b/packages/appduct/src/output.ts @@ -4,8 +4,8 @@ import { renderToolSignature, summarizeToolDescription, type EventNotification, + type ListedToolDescriptor, type SessionSummary, - type ToolDescriptor, type ToolGroupSummary, type ToolsListEntry, } from "@appduct/shared"; @@ -153,7 +153,7 @@ const renderLsData = (colors: ColorPalette, data: LsCommandData, now: Date): str }; /** `renderToolsData` distinguishes the listing form of `ToolsCommandData` (`ToolsListing`, which - * carries `tools`/`total`) from the bare single-tool detail form purely by shape — a `ToolDescriptor` + * carries `tools`/`total`) from the bare single-tool detail form purely by shape — a single entry * never has a `tools` array of its own, so this never misclassifies either one. */ const isToolsListing = (data: ToolsCommandData): data is ToolsListing => { return typeof data === "object" && data !== null && Array.isArray((data as ToolsListing).tools); @@ -334,7 +334,11 @@ const renderToolSummaryTable = (colors: ColorPalette, data: ToolsListing): strin ]; }; -const renderToolDetail = (colors: ColorPalette, tool: ToolDescriptor, flags: GlobalFlags): string[] => { +const renderToolDetail = ( + colors: ColorPalette, + tool: ListedToolDescriptor, + flags: GlobalFlags, +): string[] => { return renderFields( colors.green(`Tool: ${tool.name}`), [ diff --git a/packages/shared/src/__tests__/tool-descriptor.test.ts b/packages/shared/src/__tests__/tool-descriptor.test.ts index bdfb93b..c5795ea 100644 --- a/packages/shared/src/__tests__/tool-descriptor.test.ts +++ b/packages/shared/src/__tests__/tool-descriptor.test.ts @@ -10,6 +10,7 @@ import { MAX_TOOL_TIMEOUT_MS, MIN_TOOL_TIMEOUT_MS, TOOL_NAME_PATTERN, + type ToolDescriptor, } from "../domains/tool-descriptor.js"; const valid = () => ({ @@ -196,6 +197,16 @@ describe("tool groups", () => { expect(isToolDescriptor({ ...valid(), group: null })).toBe(false); }); + test("the ToolDescriptor type rejects the null group that registration rejects", () => { + // `ToolDescriptor` is what an app author writes against (`appduct/client`, the React Native + // SDK's `getRegisteredTools`). A type that admits `group: null` promises a registration the + // guard below — and the `group-null` conformance vector — throws out. + // @ts-expect-error -- an ungrouped tool omits `group`; `null` is not a way to spell it. + const registered: ToolDescriptor = { ...valid(), group: null }; + + expect(isToolDescriptor(registered)).toBe(false); + }); + test("toolGroupMatches matches by segment: a parent includes its subgroups, never a longer name", () => { expect(toolGroupMatches("checkout", "checkout")).toBe(true); expect(toolGroupMatches("checkout/payment", "checkout")).toBe(true); diff --git a/packages/shared/src/domains/rpc.ts b/packages/shared/src/domains/rpc.ts index 0eeeb2d..41f0280 100644 --- a/packages/shared/src/domains/rpc.ts +++ b/packages/shared/src/domains/rpc.ts @@ -157,11 +157,12 @@ export type ToolsListParams = SessionSelectorParams & { offset?: number; }; -/** A `tools.list` entry: the tool's descriptor plus the policy decision (ARCHITECTURE.md §12) - * that would apply to it right now — resolved daemon-side (it needs `session.alias` and - * `config.policy`) so the MCP server knows which calls need an elicitation prompt without a - * second round trip. */ -export type ToolsListEntry = Omit & { +/** + * A tool as a *listing* describes it: every `ToolDescriptor` field, but with the listing's spelling + * of "no group". This is the type to read a `tools.list` entry with; `ToolDescriptor` is the + * registration side, and an app that registers `group: null` is rejected. + */ +export type ListedToolDescriptor = Omit & { /** * Always present on a listing: the tool's group, or `null` when it has none. Apps register an * ungrouped tool by *omitting* `group`; the daemon normalises that to `null` here so that entries @@ -169,6 +170,13 @@ export type ToolsListEntry = Omit & { * tool groups omits the key entirely, which callers normalise with `entry.group ?? null`. */ group: string | null; +}; + +/** A `tools.list` entry: the tool as the listing describes it plus the policy decision + * (ARCHITECTURE.md §12) that would apply to it right now — resolved daemon-side (it needs + * `session.alias` and `config.policy`) so the MCP server knows which calls need an elicitation + * prompt without a second round trip. */ +export type ToolsListEntry = ListedToolDescriptor & { policy: EffectivePolicyDecision; }; diff --git a/packages/shared/src/domains/tool-descriptor.ts b/packages/shared/src/domains/tool-descriptor.ts index 4a238e4..c175a1b 100644 --- a/packages/shared/src/domains/tool-descriptor.ts +++ b/packages/shared/src/domains/tool-descriptor.ts @@ -168,10 +168,15 @@ export type ToolDescriptor = { * ({@link ToolGroupSummary}), so one test — `entry.group === null` — answers "ungrouped" anywhere * in a listing. Consumers must therefore not probe `"group" in entry`. * + * This type is the *registration* side, and the one app authors write against (`appduct/client` + * re-exports it; the React Native SDK's `getRegisteredTools` returns it), so it admits only what + * registration admits: a string or nothing. A reader of a listing takes `ToolsListEntry`, whose + * own `group: string | null` carries the wire spelling. + * * Drives `tools.list`'s `group` filter and `groups` summary; never part of an MCP built-in's own * schema (the app tools surfaced through `appduct_list_tools` do report it, one per entry). */ - group?: string | null; + group?: string; }; const isJsonObject = (value: unknown): value is Record => {