fix: report an ungrouped tool's group as null to every reader (#86)

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*

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();

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 }]);
   });
 });

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;

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<Client> => {
+const startServerWithClient = async (daemon: Pick<FakeDaemon, "openStream">): Promise<Client> => {
   const handle = await createMcpServer({
     stateDir: "/nonexistent-state-dir",
     openStream: daemon.openStream,
@@ -70,6 +71,30 @@ const startServerWithClient = async (daemon: FakeDaemon): Promise<Client> => {
   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<DaemonStream> => {
+    const stream = await daemon.openStream();
+    const { call } = stream;
+
+    return {
+      ...stream,
+      call: async <TResult>(method: string, params?: unknown): Promise<TResult> => {
+        const result = await call<TResult>(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<string, unknown>): Promise<CallToolResult> => {
   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 <TResult>(method: string, params?: unknown): Promise<TResult> => {
-          const result = await call<TResult>(method, params);
-
-          if (method === RPC_METHODS.toolsList) {
-            const { groups: _groups, ...rest } = result as Record<string, unknown>;
-            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<ToolsListResult>(RPC_METHODS.toolsList, { selector: "pixel-8" });
+
+    expect(result.tools.map((entry) => [entry.name, entry.group])).toEqual([
+      ["echo", null],
+      ["pay", "checkout"],
+    ]);
+  });
+});

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;

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<TPayload = unknown> = {
 export type AppClient<TTools = ToolMap> = {
   readonly sessionId: string;
 
-  /** `tools.list` for this session. */
-  tools(): Promise<ToolDescriptor[]>;
+  /** `tools.list` for this session. An entry spells an ungrouped tool's `group` as `null`,
+   * where a registration omits it. */
+  tools(): Promise<ListedToolDescriptor[]>;
 
   /** `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 = <TTools = ToolMap>(stream: DaemonStream, sessionId:
   const client = {
     sessionId,
 
-    tools: async (): Promise<ToolDescriptor[]> => {
+    tools: async (): Promise<ListedToolDescriptor[]> => {
       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 = <TTools = ToolMap>(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);
       }

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";

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);
 };
 

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,15 +180,18 @@ 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,
     input_schema: entry.input_schema,
     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,
   };
 };
 

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}`),
     [

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);