Skip to content

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

Merged
V3RON merged 10 commits into
mainfrom
issue-86-ungrouped-tool-describe-omits-group-publ
Sep 22, 2026
Merged

V3RON merged 10 commits into
mainfrom
issue-86-ungrouped-tool-describe-omits-group-publ

Conversation

@V3RON

@V3RON V3RON commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Closes #86

What changed

"No group" now has one spelling per direction. appduct_describe_tool reports group: null for an ungrouped tool, the value
appduct_list_tools already reported, because toDescriptor normalises with ?? null like the
listing does. ToolDescriptor.group is string | undefined again, so the type an app author
writes against admits exactly what registration admits. The listing spelling gets its own name,
ListedToolDescriptor (Omit<ToolDescriptor, "group"> & { group: string | null }), which
ToolsListEntry now extends and which the readers of a tools.list entry take. The MCP tests'
daemon fake normalises group the way the daemon does, and loses the cast that was hiding the
mismatch.

The issue named three readers; the compiler found a fourth, appduct/client's AppClient.tools(),
which returns daemon list entries and was typed ToolDescriptor[]. It now returns
ListedToolDescriptor[], and appduct/client re-exports that type. That is the one public-API
signature change here, and it is the same fix: it stops promising group?: string for a value the
daemon sends as null.

Why a named type rather than Omit<ToolsListEntry, "policy"> inline: four call sites
(toDescriptor, ResolvedAppTool.descriptor, renderToolDetail, AppClient.tools), and one of
them is public, so users need something to name.

Acceptance criteria

# Criterion Test Tier
1 appduct_describe_tool reports group: null for an ungrouped tool, including against a daemon that predates groups and sends no group key mcp-server.test.ts — "reports an ungrouped tool's group as null, the value appduct_list_tools reports for it" and "against a daemon that predates groups, an ungrouped tool's group is still null" unit
2 The daemon fake's tools.list entries carry group: null for an ungrouped tool, as the daemon's do, with no cast to make them fit mcp-server.test.ts — "lists an ungrouped tool with a null group, the way the daemon serves one" unit
3 The public ToolDescriptor type rejects group: null, which registration already rejects tool-descriptor.test.ts — "the ToolDescriptor type rejects the null group that registration rejects" unit
4 appduct/client's tools() reports group: null on every entry, including from the pre-{ tools, total } daemon it tolerates app-client.test.ts — "still accepts a bare array from a daemon that predates { tools, total }" unit

Criterion 3 is a compile-time criterion: its @ts-expect-error was unused while the type still
admitted null, so pnpm typecheck is what went from red to green on it. Vitest cannot observe it
— types are erased before the test runs.

E2E evidence

not applicable (no runtime behaviour change on device)

No SDK, daemon or protocol behaviour changes. The narrowed ToolDescriptor.group only removes a
value registration already threw on, and the two ?? null normalisations change what an MCP
reader and a test-runner client report, not what any device does or sends.

Checklist

  • CHANGELOG.md has a line under Unreleased, or the change is not user-visible
  • User-facing docs updated for every surface the change touches (writing-user-docs skill), or the change is not user-visible — packages/appduct/README.md's appduct/client example names the new return type. The React Native README's getRegisteredToolsToolDescriptor[] is unchanged and still correct: that is the registration side. pnpm check:links passes.
  • No new import past a module's index.ts; no new direct node:* I/O outside an adapter
  • Simplification checklist from the architecture skill applied, exceptions explained above
  • docs/ARCHITECTURE.md updated if a surface it describes changed — §5's tools.list row already describes the wire shape, which is unchanged

Out of scope

none

Status

Implement: done Review: changes addressed E2E: not applicable Ready: no

`toDescriptor` now normalises with `?? null` the way the listing does, so
`appduct_describe_tool` and `appduct_call_tool` stop dropping the key.

2 failing -> 1 failing
#86)

`setTools` normalises `group` instead of casting `Partial<ToolDescriptor>` into
`ToolsListEntry`, so the fake stops hiding the mismatch the entry type exists to raise.

1 failing -> 0 failing (typecheck still red on the ToolDescriptor type)
…#86)

`group` is `string | undefined` again on the registration type, so `appduct/client` and the
React Native SDK stop advertising a `null` that `isToolDescriptor` throws out. The listing
spelling moves to `ListedToolDescriptor`, which `ToolsListEntry` extends and which the four
readers of a `tools.list` entry now take.

typecheck red -> green; 0 failing

@V3RON V3RON left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment: 0 blocker, 2 should-fix, 1 nit. Spec: issue #86 plus the PR body for the two extras (the AppClient.tools() retype and ListedToolDescriptor); the changelog merges cleanly onto main's new header with a single ## Unreleased.
Fix first: AppClient.tools() now promises group: string | null on every entry but passes a pre-groups daemon's entries through with no key, while the MCP path in this same diff normalises with ?? null for exactly that daemon.

sessionId,

tools: async (): Promise<ToolDescriptor[]> => {
tools: async (): Promise<ListedToolDescriptor[]> => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should-fix. The return type now promises group: string | null on every entry and the doc comment on line 96 says so, but this returns the daemon's entries as-is, and this method explicitly tolerates a daemon that predates { tools, total } (line 175: no version check) — one that also predates groups and sends no group key. Scenario: an 0.10 daemon is still running, const [tool] = await app.tools() on an ungrouped tool, then the documented test tool.group === null is false, and tool.group !== null && tool.group.startsWith("checkout") throws TypeError on undefined. toDescriptor in mcp/app-tools.ts adds ?? null in this diff for exactly this daemon; this path does not, so the same case is handled two ways. Map the entries with group: entry.group ?? null and change the "still accepts a bare array" test in app-client.test.ts (whose toolEntry has no group and currently pins the pass-through) to expect group: null.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved. tools() now maps every entry with group: entry.group ?? null (app-client.ts:180), and the "still accepts a bare array" test pins it: toolEntry has no group, and toEqual([{ ...toolEntry, group: null }]) fails against the old pass-through, since toEqual only forgives undefined, not a missing key against null.

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[]>;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should-fix. packages/appduct/README.md:235 still documents this as await app.tools(); // ToolDescriptor[]. A user who annotates from the README, const tools: ToolDescriptor[] = await app.tools(), now gets TS2322: group: string | null is not assignable to group?: string. That README is the surface the PR template's docs checklist item names for an SDK API change (the template landed on main after this branch was cut, so the PR body does not carry the item). Change the comment to ListedToolDescriptor[] and say in one clause that group is null for an ungrouped tool.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved. packages/appduct/README.md:235 now reads ListedToolDescriptor[]; an ungrouped tool's group is null, and appduct/client exports the type (client/index.ts:26), so an annotation copied from the example compiles.

// Normalised the same way the listing normalises it, so `appduct_describe_tool` and
// `appduct_call_tool` never disagree 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,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. At head nothing fails without the ?? null here: the criterion-1 test is green from the fake's own normalisation, so group: entry.group would pass every test too, and the pre-groups daemon this comment says it covers is the only input that exercises the line. The "against a daemon that predates groups" test in mcp-server.test.ts (line 212) already strips keys from the fake's result; the same wrapper, dropping group from each entry, would pin this for appduct_describe_tool.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved. openPreGroupsStream strips group from every tools.list entry, and "against a daemon that predates groups, an ungrouped tool's group is still null" fails without the ?? null here: toDescriptor would return group: undefined, which toMatchObject({ group: null }) does not accept.

Two red tests from the review of #87: `appduct/client`'s `tools()` passing a pre-groups
daemon's entries straight through, and `appduct_describe_tool` doing the same. The second is
green already; the pre-groups stream wrapper is what makes it fail without the `?? null`.

1 failing
)

`tools()` normalises the entries it hands back, so the pre-`{ tools, total }` daemon it
already tolerates cannot leave a caller with `group` missing where the type promises `null`.

1 failing -> 0 failing
The README's example still annotated it `ToolDescriptor[]`, which no longer compiles.

@V3RON V3RON left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment: 0 blocker, 1 should-fix, 0 nit. Spec: issue #86 plus the PR body for the AppClient.tools() retype and ListedToolDescriptor; all three round-1 threads are resolved, each with a test that fails without its fix, and the merge from main is identical to a clean merge-tree of its parents with a single ## Unreleased.
Fix first: the changelog says appduct_call_tool echoes a descriptor back; it returns only the tool's result, so drop that clause.

Comment thread CHANGELOG.md Outdated
## Unreleased

- **Fix: an ungrouped tool reports `group: null` to every reader.** `appduct_describe_tool` and
the descriptor `appduct_call_tool` echoes back dropped the key instead of reporting `null`, so

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should-fix. appduct_call_tool never returns a descriptor: callAppTool in mcp/server.ts returns result.result, the tool's own payload, ToolsCallResult carries only result and callId, and ResolvedAppTool.descriptor is read for name and timeout_ms alone (server.ts:400, :483, app-tools.ts:329). Scenario: an agent author reads this line, expects group in appduct_call_tool's output, and reads a key that is never there. Drop the clause so the line names the one reader that was wrong: appduct_describe_tool dropped the key instead of reporting null, so it disagreed with appduct_list_tools about the same tool. The PR body's criterion 1 and the toDescriptor comment (app-tools.ts:191) make the same claim; not user-facing, but worth the same edit.

@V3RON
V3RON marked this pull request as ready for review September 22, 2026 19:35
@V3RON
V3RON merged commit 8a76aca into main Sep 22, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ungrouped tool: describe omits group; public type admits null

1 participant