From 42001e78ef5c542c3a4e49000b4bab8945bb9fc9 Mon Sep 17 00:00:00 2001 From: Graeme Foster <80714+GraemeF@users.noreply.github.com> Date: Sat, 1 Aug 2026 09:13:56 +0100 Subject: [PATCH] A session id is the host's to supply, so the model is no longer shown one (comms-tg70) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/agent-experience.md principle 1: a human does not type their session id into the compose box. `session_id` was declared on seven tool schemas, so the model saw a parameter it has no way to fill — the value comes from Claude Code's PreToolUse hook, or from a non-CC host that stamps it into `arguments` itself. Neither route ever needed it advertised. What the tool ADVERTISES and what it ACCEPTS are now separate. `hostSuppliedArgs` names the arguments a host stamps in out of band; the guard in `registerTools` admits `properties ∪ hostSuppliedArgs` while the unknown-argument error still names only the advertised ones — an error listing `session_id` as valid would put the parameter back on the surface this removes it from. The relaxation is per-tool: a tool listing nothing still rejects `session_id`, and every tool still rejects everything else. Every supply channel is untouched. The boot-env feeder, the hook injection, and `readSessionId`'s read-back all work as before, which the existing tools-session suite proves — those tests pass `session_id` in `arguments` and assert the mint, and they are now tests of an undeclared argument being read. Seven tools carry it, not the five in the hook matcher: subscribe and unsubscribe are the identity route for a listen-first non-CC seat, and a host stamping the argument on a tool CC never stamps must not be told it is unknown. The instructions block goes too. `COMMY_INSTRUCTIONS` is injected into every agent's context verbatim, and it told the model to pass its conversation's session id. Removing the schema property while leaving that paragraph would have shipped a surface that instructs the model to send an argument no tool advertises. The contract a non-CC host implements now lives only in docs/self-hosting.md, which its operator reads. The manifest test asserts the same rule one seam over: a tool whose adapter path reaches `boundHttp` must RECEIVE `session_id`, not declare it. That rule is traced from a source scan, and a scan that stops matching satisfies every "no tool violates this" assertion by looking at nothing — so the receiving set is pinned at seven, and the advertise side is traced separately and asserted empty. Verified by mutation: renaming the marker fails the pin, and returning a `session_id` property to a schema fails the advertise-side rule. --- clients/claude-code/README.md | 4 - clients/claude-code/hooks-manifest.test.ts | 137 +++++++++++++++++---- docs/agent-experience.md | 3 - docs/self-hosting.md | 8 +- packages/mcp/mcp-server.test.ts | 20 ++- packages/mcp/mcp-server.ts | 13 +- packages/mcp/tools-session.test.ts | 87 +++++++++++++ packages/mcp/tools.test.ts | 38 +++++- packages/mcp/tools.ts | 69 ++++++++--- 9 files changed, 320 insertions(+), 59 deletions(-) diff --git a/clients/claude-code/README.md b/clients/claude-code/README.md index 9324770..2795c5e 100644 --- a/clients/claude-code/README.md +++ b/clients/claude-code/README.md @@ -194,10 +194,6 @@ The mechanics block defines: - **Tools.** `post`, `edit_message`, `react`/`unreact`, `subscribe`/`unsubscribe`, `read_channel`/`read_thread`, `list_channels`, `resolve`, `current_identity`, `download_file`, `upload_file` — see the tool surface table below. -- **`session_id`.** Pass it on `post`, `edit_message`, `react`, `unreact`, - and `current_identity`. **Must be a UUID** (e.g. `crypto.randomUUID()`); - malformed values are rejected as if the field were missing. - CC's PreToolUse hook injects the harness session UUID automatically. ## Inbound event format diff --git a/clients/claude-code/hooks-manifest.test.ts b/clients/claude-code/hooks-manifest.test.ts index 4cb78dc..ccfd326 100644 --- a/clients/claude-code/hooks-manifest.test.ts +++ b/clients/claude-code/hooks-manifest.test.ts @@ -15,7 +15,7 @@ import hooksManifest from './hooks/hooks.json' * So the derivation is traced from there instead, which is also what * `comms-tww6` specifies: * - * A TOOL WHOSE ADAPTER PATH REACHES `boundHttp` MUST DECLARE `session_id` + * A TOOL WHOSE ADAPTER PATH REACHES `boundHttp` MUST RECEIVE `session_id` * AND BE IN THE `hooks.json` MATCHER. * * That is a stronger rule than the old one. The old test could only catch a @@ -23,12 +23,27 @@ import hooksManifest from './hooks/hooks.json' * see a tool that reached `boundHttp` while appearing in neither set — which is * exactly the live P1 that `comms-tww6` is open about. * + * RECEIVES, NOT DECLARES (comms-tg70). The rule used to say DECLARE, and traced + * a `session_id` property on the tool's advertised `inputSchema`. No tool + * declares one now: `session_id` is host plumbing the model has no way to fill, + * so it is supplied into `arguments` and accepted by the guard without being + * advertised (`ToolDef.hostSuppliedArgs`). What a bound-path tool must have is + * unchanged in substance — the id has to REACH it — so the trace moved to the + * accept-side marker. + * + * THAT MOVE IS WHY {@link SESSION_ID_RECEIVING_TOOLS} EXISTS. A test that + * derives a property from a source scan goes green when the scan stops matching + * — every rule below reads "no tool violates it" and a scan finding nothing + * satisfies all of them by looking at nothing. Pinning the receiving set makes + * the scan itself the thing under test: rename the marker and the pin fails + * loudly instead of the suite passing quietly. + * * KNOWN VIOLATIONS ARE NAMED, NOT PAPERED OVER. Three tools violate the rule at - * HEAD (see `TWW6_EXCEPTIONS`). Fixing them means adding `session_id` to their - * schemas, which is `comms-tg70`'s ground and out of scope here. Encoding the - * real rule with a visible exception list beats asserting a weaker rule that - * passes: the day `comms-tww6` lands, its author deletes entries from that list - * and this test proves the fix. + * HEAD (see `TWW6_EXCEPTIONS`). Fixing them means giving them the host-supplied + * `session_id`, which is out of scope here. Encoding the real rule with a + * visible exception list beats asserting a weaker rule that passes: the day + * `comms-tww6` lands, its author deletes entries from that list and this test + * proves the fix. */ /** @@ -100,7 +115,7 @@ const BOUND_INBOX_VERBS = ['subscribe', 'subscriptions', 'unsubscribe'] as const const G5ZH3_MATCHER_PENDING = [] as const /** - * Tools that reach `boundHttp` while declaring no `session_id` and sitting + * Tools that reach `boundHttp` while receiving no `session_id` and sitting * outside the matcher — the open P1 `comms-tww6`. They run under whatever seat * an EARLIER call happened to bind, so their attribution is inherited by * accident of ordering rather than established by the call itself. @@ -110,6 +125,26 @@ const G5ZH3_MATCHER_PENDING = [] as const */ const TWW6_EXCEPTIONS = ['resolve_thread', 'set_channel_description', 'unresolve_thread'] as const +/** + * Every tool that accepts a host-supplied `session_id`. SEVEN, which is wider + * than the five in the PreToolUse matcher: `subscribe` and `unsubscribe` are + * here for a non-CC ephemeral host that supplies the UUID itself, and a + * listen-first seat reaches an identity through no other tool. Do not read a + * statement about one of these sets as a statement about the other. + * + * Pinned, not derived, and that is the point — see the file header. This set is + * what proves the scan below still sees anything at all. + */ +const SESSION_ID_RECEIVING_TOOLS = [ + 'current_identity', + 'edit_message', + 'post', + 'react', + 'subscribe', + 'unreact', + 'unsubscribe', +] as const + /** Enclosing declaration names in the adapter source that call `boundHttp()`. */ function adapterVerbsReachingBoundHttp(source: string): ReadonlySet { const reaching = new Set() @@ -135,24 +170,37 @@ function adapterVerbsReachingBoundHttp(source: string): ReadonlySet { interface ToolFacts { readonly verbs: ReadonlySet readonly inboxVerbs: ReadonlySet - readonly declaresSessionId: boolean + readonly receivesSessionId: boolean + readonly advertisesSessionId: boolean } /** - * Per-tool: which publisher verbs and which inbox verbs its handler calls, and - * whether it declares `session_id`. + * Per-tool: which publisher verbs and which inbox verbs its handler calls, + * whether it accepts a host-supplied `session_id`, and whether it advertises + * one on its `inputSchema`. The last is traced only so the assertions can show + * it is nowhere — the two facts are separate and stay separately measured. */ function toolFactsFromToolsSource(source: string): ReadonlyMap { const facts = new Map< string, - { verbs: Set; inboxVerbs: Set; declaresSessionId: boolean } + { + verbs: Set + inboxVerbs: Set + receivesSessionId: boolean + advertisesSessionId: boolean + } >() let current: string | undefined for (const line of source.split('\n')) { const named = line.match(/^ {6}name: '([a-z_]+)',$/)?.[1] if (named !== undefined) { current = named - facts.set(named, { verbs: new Set(), inboxVerbs: new Set(), declaresSessionId: false }) + facts.set(named, { + verbs: new Set(), + inboxVerbs: new Set(), + receivesSessionId: false, + advertisesSessionId: false, + }) } const entry = current === undefined ? undefined : facts.get(current) if (entry === undefined) continue @@ -167,7 +215,11 @@ function toolFactsFromToolsSource(source: string): ReadonlyMap [name, { ...e, verbs: e.verbs, inboxVerbs: e.inboxVerbs }]), @@ -210,13 +262,39 @@ test('the set of adapter declarations reaching boundHttp is the pinned one', asy ]) }) -test('every tool whose adapter path reaches boundHttp declares session_id', async () => { +// The pin that keeps the four rules below from holding by not looking. Each of +// them reads "no tool violates this", which a scan that matches nothing +// satisfies trivially — so assert first that the scan finds the set it is +// supposed to find. +test('the tools accepting a host-supplied session_id are exactly the pinned seven', async () => { + const facts = toolFactsFromToolsSource(await toolsSource()) + const receiving = [...facts] + .filter(([, f]) => f.receivesSessionId) + .map(([name]) => name) + .sort() + expect(receiving).toEqual([...SESSION_ID_RECEIVING_TOOLS]) +}) + +// comms-tg70: `session_id` is supplied, never advertised. A human does not type +// their session id into the compose box (docs/agent-experience.md principle 1), +// and the model has no way to fill the field, so putting it on the schema only +// ever put plumbing on the agent's surface. +test('no tool advertises session_id on its inputSchema', async () => { + const facts = toolFactsFromToolsSource(await toolsSource()) + const advertising = [...facts] + .filter(([, f]) => f.advertisesSessionId) + .map(([name]) => name) + .sort() + expect(advertising).toEqual([]) +}) + +test('every tool whose adapter path reaches boundHttp receives session_id', async () => { const facts = toolFactsFromToolsSource(await toolsSource()) const offenders = [...facts] .filter( ([, f]) => [...f.verbs].some((v) => (BOUND_VERBS as ReadonlyArray).includes(v)) && - !f.declaresSessionId, + !f.receivesSessionId, ) .map(([name]) => name) .sort() @@ -259,7 +337,7 @@ test('comms-tww6: the known unstamped bound-path tools are exactly the recorded const facts = toolFactsFromToolsSource(await toolsSource()) const boundHttpVerbs = new Set(['resolveThread', 'unresolveThread', 'setChannelDescription']) const unstamped = [...facts] - .filter(([, f]) => [...f.verbs].some((v) => boundHttpVerbs.has(v)) && !f.declaresSessionId) + .filter(([, f]) => [...f.verbs].some((v) => boundHttpVerbs.has(v)) && !f.receivesSessionId) .map(([name]) => name) .sort() expect(unstamped).toEqual([...TWW6_EXCEPTIONS]) @@ -302,9 +380,7 @@ test('adapterVerbsReachingBoundHttp ignores commented-out mentions of boundHttp' test('toolFactsFromToolsSource attributes verbs and session_id to the enclosing tool', () => { const synthetic = ` name: 'alpha', - inputSchema: { - properties: { session_id: sessionIdField }, - }, + hostSuppliedArgs: hostSuppliedSessionId, handler: async (args) => { await run(adapter.publisher.post(channel, body)) }, @@ -316,24 +392,41 @@ test('toolFactsFromToolsSource attributes verbs and session_id to the enclosing handler: async () => { await run(adapter.inbox.subscribe(target)) }, + name: 'delta', + inputSchema: { + properties: { + session_id: sessionIdField, + }, + }, ` const facts = toolFactsFromToolsSource(synthetic) expect(facts.get('alpha')).toEqual({ verbs: new Set(['post']), inboxVerbs: new Set(), - declaresSessionId: true, + receivesSessionId: true, + advertisesSessionId: false, }) expect(facts.get('beta')).toEqual({ verbs: new Set(), inboxVerbs: new Set(), - declaresSessionId: false, + receivesSessionId: false, + advertisesSessionId: false, }) // A read through the inbox is still traced as an inbox verb here; whether it // BINDS is decided by `BOUND_INBOX_VERBS`, not by the receiver. expect(facts.get('gamma')).toEqual({ verbs: new Set(), inboxVerbs: new Set(['subscribe']), - declaresSessionId: false, + receivesSessionId: false, + advertisesSessionId: false, + }) + // The advertise-side trace catches a schema property coming back, and does + // not confuse it with the accept-side marker. + expect(facts.get('delta')).toEqual({ + verbs: new Set(), + inboxVerbs: new Set(), + receivesSessionId: false, + advertisesSessionId: true, }) }) diff --git a/docs/agent-experience.md b/docs/agent-experience.md index 4e56dfa..ed6f760 100644 --- a/docs/agent-experience.md +++ b/docs/agent-experience.md @@ -110,9 +110,6 @@ identity are the same moment. Places the current implementation fails this reference. -- **`session_id` is a parameter on seven tool schemas** (`packages/mcp/tools.ts`). - A human does not type their session id into the compose box. Plumbing has - surfaced in the agent-visible surface. Principle 1. - **Full message content is pushed into the agent's turn** (`packages/mcp/events.ts`, and the inbound format in the plugin README). The agent has paid for the content before deciding it was relevant. diff --git a/docs/self-hosting.md b/docs/self-hosting.md index 4aad6e8..739456f 100644 --- a/docs/self-hosting.md +++ b/docs/self-hosting.md @@ -242,7 +242,13 @@ Two supported ways to avoid it, both already documented above: - Set `COMMY_BOT_NAME` for a persistent identity — the session id is irrelevant in that mode, and the bot subscribes under its own stable principal. - Pass a UUID `session_id` in the tool-call arguments, which is the binding an - ephemeral non-CC host has (`docs/claude-channel-inbound-contract.md`). + ephemeral non-CC host has (`docs/claude-channel-inbound-contract.md`). The + tools accepting it are `post`, `edit_message`, `react`, `unreact`, + `current_identity`, `subscribe` and `unsubscribe`. It is **not** advertised on + their `inputSchema` — a session id is the host's to supply, not something a + model could fill, so it is documented here rather than shown to the agent. + Anything that fails UUID validation is treated as missing: the server returns + the unbound-stub error rather than minting a malformed `cc-*` identity. Claude Code seats are unaffected: the plugin injects `CLAUDE_CODE_SESSION_ID` into the MCP child's environment at spawn, so the id is known before the seat's diff --git a/packages/mcp/mcp-server.test.ts b/packages/mcp/mcp-server.test.ts index c2006ad..db19af4 100644 --- a/packages/mcp/mcp-server.test.ts +++ b/packages/mcp/mcp-server.test.ts @@ -69,18 +69,34 @@ test('initialize response declares a tools capability that supports list changes } }) -test('initialize response carries instructions explaining the session_id contract', async () => { +test('initialize response carries instructions covering the tool surface', async () => { const { client, close } = await pairAndConnect() try { const instructions = client.getInstructions() expect(instructions).toBeDefined() - expect(instructions).toMatch(/session_id/) expect(instructions).toMatch(/post|react|unreact|current_identity/) } finally { await close() } }) +// The instructions block is read by the model, so it is the agent-visible +// surface in prose (comms-tg70). It used to carry a `session_id` paragraph +// telling the agent to pass its conversation's session id — plumbing a host +// supplies, not something an agent has or could produce. Removing the schema +// property while leaving that paragraph would have left the model instructed to +// send an argument no tool advertises. +test('initialize instructions never ask the agent for a session id', async () => { + const { client, close } = await pairAndConnect() + try { + const instructions = client.getInstructions() + expect(instructions).not.toMatch(/session_id/) + expect(instructions).not.toMatch(/session id/i) + } finally { + await close() + } +}) + test('initialize instructions give substrate-coexistence guidance without naming a specific peer substrate', async () => { const { client, close } = await pairAndConnect() try { diff --git a/packages/mcp/mcp-server.ts b/packages/mcp/mcp-server.ts index 9472b8b..03ec20f 100644 --- a/packages/mcp/mcp-server.ts +++ b/packages/mcp/mcp-server.ts @@ -12,13 +12,18 @@ export const PLUGIN_VERSION = '0.22.0' * Echoed to every connected MCP client via the server `instructions:` * field. Substrate-general mechanics only — channel naming + discovery, * topic discipline, subscription discipline, - * clickable-permalink rendering, tool cheat sheet, and - * the `session_id` contract. Deliberately carries + * clickable-permalink rendering, and a tool cheat sheet. + * Deliberately carries * no operator-specific assumptions (named peer substrates, issue * trackers, internal ids) — those belong in an operator's own context, * not in guidance shipped to every adopter. Etiquette — how * to communicate *well* on the substrate — ships separately as the * `using-commy` skill so it stays opt-in rather than always-on. + * + * Nothing here asks the agent for a `session_id`. This block is read by the + * model, so it is the agent-visible surface in prose, and a session id is the + * host's to supply (see `ToolDef.hostSuppliedArgs`). The contract a non-CC + * host implements is in `docs/self-hosting.md`, which its operator reads. */ const COMMY_INSTRUCTIONS = `**Substrate.** commy is the inter-agent channel: agents and humans coordinate here. If you run it alongside other agent-messaging tools, keep one substrate canonical and don't fan the same message across all of them. @@ -30,9 +35,7 @@ const COMMY_INSTRUCTIONS = `**Substrate.** commy is the inter-agent channel: age **Links.** Every ref the substrate hands you carries a ready-to-click \`permalink\` — on \`post\` results, \`read_channel\`/\`read_thread\` messages (message \`permalink\` plus \`channel.permalink\` and \`thread.permalink\`), \`list_channels\` (channel \`permalink\`), and inbound \`\` frames (\`permalink\` / \`channel_permalink\` / \`thread_permalink\` meta, \`target_permalink\` on reaction frames). **Whenever you show a human a message, channel, or topic reference, render it as that clickable permalink — never a bare name or numeric id.** A human can click a permalink straight to the message; a bare \`#channel > topic\` or message number makes them hunt. When you hold only a message id (e.g. one cited elsewhere, with no permalink to hand), \`message_link(message_id, channel_name?, thread?)\` returns its \`{permalink}\`. -**Tools.** \`post\` (channel; optionally thread or reply), \`react\`/\`unreact\` (emoji on a message), \`subscribe\`/\`unsubscribe\` (live target), \`read_channel\`/\`read_thread\` (history within a range), \`list_channels\` (enumerate channels in the realm), \`message_link\` (canonical permalink for a message id), \`resolve\` (identity by name), \`current_identity\` (passive — never acquires), \`download_file\` (fetch a \`/user_uploads/...\` attachment to a temp file — rooted under the operator-set \`COMMY_DOWNLOAD_DIR\` when configured so it lands somewhere you can Read, else \`$TMPDIR\`; use Read on the returned path to view images), \`upload_file\` (upload a local file by absolute path; returns a \`reference\` string to embed in a \`post\` body). - -**session_id.** Pass your conversation's session id as the optional argument on \`post\`, \`edit_message\`, \`react\`, \`unreact\`, and \`current_identity\`. **Must be a UUID** (e.g. \`crypto.randomUUID()\`); anything else is rejected as malformed and the call routes to the "missing session_id" error rather than silently minting a \`cc-\` identity. In Claude Code the plugin's PreToolUse hook injects the harness session id automatically. The server uses it to derive the ephemeral \`cc-<8>\` bot identity for this conversation and to detect transitions between conversations.` +**Tools.** \`post\` (channel; optionally thread or reply), \`react\`/\`unreact\` (emoji on a message), \`subscribe\`/\`unsubscribe\` (live target), \`read_channel\`/\`read_thread\` (history within a range), \`list_channels\` (enumerate channels in the realm), \`message_link\` (canonical permalink for a message id), \`resolve\` (identity by name), \`current_identity\` (passive — never acquires), \`download_file\` (fetch a \`/user_uploads/...\` attachment to a temp file — rooted under the operator-set \`COMMY_DOWNLOAD_DIR\` when configured so it lands somewhere you can Read, else \`$TMPDIR\`; use Read on the returned path to view images), \`upload_file\` (upload a local file by absolute path; returns a \`reference\` string to embed in a \`post\` body).` /** * Construct the commy MCP server with the capabilities the plugin diff --git a/packages/mcp/tools-session.test.ts b/packages/mcp/tools-session.test.ts index f5956f6..ff83353 100644 --- a/packages/mcp/tools-session.test.ts +++ b/packages/mcp/tools-session.test.ts @@ -525,3 +525,90 @@ test('post with cwd from a non-project directory falls back to bare cc-<8>', () }), ), )) + +// --- session_id is supplied, not advertised (comms-tg70) --- +// +// The declaration the model reads and the argument the server reads are +// separate things. `session_id` is on no advertised `inputSchema` — a host +// stamps it into `arguments` out of band, so the model never sees a field it +// has no way to fill. The tests above all exercise this already; the three +// below state the property directly, because the argument check derives its +// accepted set from the ADVERTISED one and would otherwise reject the very +// supply channel every test here depends on. + +test('post accepts a host-supplied session_id that no schema advertises', () => + Effect.runPromise( + Effect.scoped( + Effect.gen(function* () { + const rig = yield* buildSessionRig() + const listed = yield* Effect.promise(() => rig.client.listTools()) + const post = listed.tools.find((tool) => tool.name === 'post') + const properties = (post?.inputSchema as { properties?: Readonly> }) + .properties + expect(Object.hasOwn(properties ?? {}, 'session_id')).toBe(false) + + const result = yield* Effect.promise(() => + rig.client.callTool({ + name: 'post', + arguments: { + channel_name: 'home', + body: 'stamped by the host, not typed by the model', + session_id: SID_A, + }, + }), + ) + expect(result.isError).toBeFalsy() + const current = yield* rig.adapter.identity.currentIdentity() + expect(current.name).toBe(decodeDisplayNameSync('cc-aaaaaaaa')) + }), + ), + )) + +test('a tool that takes no host-supplied session_id still rejects one', () => + Effect.runPromise( + Effect.scoped( + Effect.gen(function* () { + // The relaxation is per-tool, not a hole in the check. `read_thread` is + // outside the hook matcher and binds nothing, so a `session_id` reaching + // it is a caller error and still says so. + const rig = yield* buildSessionRig() + const error = yield* Effect.flip( + Effect.tryPromise({ + try: () => + rig.client.callTool({ + name: 'read_thread', + arguments: { channel_name: 'home', thread: 'orientation', session_id: SID_A }, + }), + catch: (e) => e as { message: string }, + }), + ) + expect(error.message).toContain('session_id') + }), + ), + )) + +test('a genuinely unknown argument is still rejected on a tool that takes session_id', () => + Effect.runPromise( + Effect.scoped( + Effect.gen(function* () { + const rig = yield* buildSessionRig() + const error = yield* Effect.flip( + Effect.tryPromise({ + try: () => + rig.client.callTool({ + name: 'post', + arguments: { + channel_name: 'home', + body: 'hello', + session_id: SID_A, + thread_name: 'oops', + }, + }), + catch: (e) => e as { message: string }, + }), + ) + expect(error.message).toContain('thread_name') + expect(error.message).not.toContain('session_id') + }), + ), + )) diff --git a/packages/mcp/tools.test.ts b/packages/mcp/tools.test.ts index eab0a49..0f07904 100644 --- a/packages/mcp/tools.test.ts +++ b/packages/mcp/tools.test.ts @@ -150,7 +150,7 @@ const withRigAndCache = ( return rig }) -test('tools/list advertises current_identity with optional session_id', () => +test('tools/list advertises current_identity with optional cwd', () => Effect.runPromise( Effect.scoped( Effect.gen(function* () { @@ -162,13 +162,43 @@ test('tools/list advertises current_identity with optional session_id', () => expect(tool?.inputSchema).toMatchObject({ type: 'object', properties: { - session_id: { type: 'string', description: expect.any(String) }, + cwd: { type: 'string', description: expect.any(String) }, }, additionalProperties: false, }) - // session_id is optional (not in required[]). + // cwd is optional (not in required[]). const inputSchema = tool?.inputSchema as { required?: ReadonlyArray } - expect(inputSchema.required ?? []).not.toContain('session_id') + expect(inputSchema.required ?? []).not.toContain('cwd') + }), + ), + )) + +// A human does not type their session id into the compose box +// (docs/agent-experience.md principle 1). `session_id` is host plumbing: the +// PreToolUse hook stamps it into `arguments` outside the model's view, and a +// non-CC host supplies it the same way. Neither needs the parameter ADVERTISED, +// and advertising it puts a field on the agent's surface that nothing an agent +// knows could fill. +// +// Stated over every tool, not just the seven that used to declare it, so a tool +// gaining the property later fails here rather than passing by not being looked +// at. +test('tools/list advertises session_id on no tool', () => + Effect.runPromise( + Effect.scoped( + Effect.gen(function* () { + const rig = yield* withRig((_adapter, ensureBound) => ensureBound().pipe(Effect.asVoid)) + const result = yield* Effect.promise(() => rig.client.listTools()) + expect(result.tools.length).toBeGreaterThan(0) + const advertising = result.tools + .filter((tool) => { + const schema = tool.inputSchema as { + properties?: Readonly> + } + return Object.hasOwn(schema.properties ?? {}, 'session_id') + }) + .map((tool) => tool.name) + expect(advertising).toEqual([]) }), ), )) diff --git a/packages/mcp/tools.ts b/packages/mcp/tools.ts index 4cc8f18..09a5635 100644 --- a/packages/mcp/tools.ts +++ b/packages/mcp/tools.ts @@ -63,6 +63,22 @@ interface ToolDef { readonly name: string readonly description: string readonly inputSchema: ToolInputSchema + /** + * Arguments the MCP HOST stamps into `arguments` out of band — accepted by + * the guard in `registerTools`, never advertised in `inputSchema`. + * + * The two are separate on purpose. `session_id` is per-conversation host + * plumbing: Claude Code's PreToolUse hook injects it before the call leaves + * the client, and a non-CC host supplies it the same way. Neither route + * needs the model to see a parameter, and advertising one put a field on the + * agent's surface that nothing an agent knows could fill + * (docs/agent-experience.md principle 1). + * + * Declaring it here rather than widening the guard globally keeps the + * relaxation per-tool: a tool that lists nothing still rejects `session_id`, + * and every tool still rejects everything else. + */ + readonly hostSuppliedArgs?: ReadonlyArray readonly handler: (args: Readonly>) => Promise } @@ -426,9 +442,10 @@ const rangeSchemaFields = { * fields inside the handler via the `decode*` decoders in `core/ports.ts`. * * The MCP-advertised `inputSchema` on each `ToolDef` stays hand-written: it is - * what `tools/list` exposes to clients and what the central unknown-argument - * guard in `registerTools` checks against. The schemas here govern the - * handler-side parse only. + * what `tools/list` exposes to clients, and — together with that tool's + * `hostSuppliedArgs` — what the central unknown-argument guard in + * `registerTools` checks against. The schemas here govern the handler-side + * parse only. */ const RangeArgs = { since: Schema.optional(Schema.Number), @@ -486,11 +503,19 @@ const UploadFileArgs = Schema.Struct({ path: Schema.String }) const buildToolDefs = (deps: RegisterToolsDeps, cache: InternalCache): ReadonlyArray => { const { adapter, identityCache, narrowSet } = deps const projectForCwd = deps.projectForCwd ?? (() => Effect.succeed(undefined)) - const sessionIdField = { - type: 'string', - description: - "Per-conversation identifier (UUID). In Claude Code, the plugin's PreToolUse hook injects this from the harness session id; non-CC MCP clients must supply a UUID (e.g. via crypto.randomUUID()). Anything that fails UUID validation is treated as missing — the server returns the unbound-stub error rather than minting a malformed cc-* identity. Drives ephemeral identity minting.", - } as const + /** + * The tools that accept a host-supplied `session_id` (see + * {@link ToolDef.hostSuppliedArgs}). SEVEN tools carry it, not the five in + * Claude Code's PreToolUse matcher: `subscribe` and `unsubscribe` are here + * for the non-CC ephemeral host that supplies the UUID itself, which is a + * listen-first seat's only route to an identity. A host stamping the arg on + * a tool CC never stamps must not be told the argument is unknown. + * + * The value is a UUID. A raw value that isn't routes through the cache's + * unbound stub rather than minting a malformed `cc--` + * identity — see `readSessionId` below. + */ + const hostSuppliedSessionId: ReadonlyArray = ['session_id'] const cwdField = { type: 'string', description: @@ -591,9 +616,10 @@ const buildToolDefs = (deps: RegisterToolsDeps, cache: InternalCache): ReadonlyA 'Return the identity this session is bound to. Passive — never triggers acquire. Returns {state: "unbound", identity: null} for ephemeral sessions that have not yet performed an attribution-producing action; {state: "bound", identity: {id, name, kind}, recent_threads?} once acquire has resolved. recent_threads is a best-effort orientation list of {channel, thread, last_post_ts, last_post_body} for the threads this identity most recently posted in; it is omitted when the enrichment lookup fails — the binding self-check itself never fails on it.', inputSchema: { type: 'object', - properties: { session_id: sessionIdField, cwd: cwdField }, + properties: { cwd: cwdField }, additionalProperties: false, }, + hostSuppliedArgs: hostSuppliedSessionId, handler: async (args): Promise => { const run = runFor(args) // Passive: reads whether this session is bound, never binds. The @@ -712,12 +738,12 @@ const buildToolDefs = (deps: RegisterToolsDeps, cache: InternalCache): ReadonlyA type: 'string', description: 'Message id of a prior message to reply to', }, - session_id: sessionIdField, cwd: cwdField, }, required: ['channel_name', 'body'], additionalProperties: false, }, + hostSuppliedArgs: hostSuppliedSessionId, handler: async (args) => { const run = runFor(args) const ref = await run( @@ -775,12 +801,12 @@ const buildToolDefs = (deps: RegisterToolsDeps, cache: InternalCache): ReadonlyA type: 'string', description: 'Thread / topic name (for thread-scoped messages)', }, - session_id: sessionIdField, cwd: cwdField, }, required: ['message_id', 'body'], additionalProperties: false, }, + hostSuppliedArgs: hostSuppliedSessionId, handler: async (args) => { const run = runFor(args) const ref = await run( @@ -813,12 +839,12 @@ const buildToolDefs = (deps: RegisterToolsDeps, cache: InternalCache): ReadonlyA type: 'string', description: 'Thread / topic name (for thread-scoped messages)', }, - session_id: sessionIdField, cwd: cwdField, }, required: ['message_id', 'emoji'], additionalProperties: false, }, + hostSuppliedArgs: hostSuppliedSessionId, handler: async (args) => { const run = runFor(args) const { ref, threadName } = await run( @@ -855,12 +881,12 @@ const buildToolDefs = (deps: RegisterToolsDeps, cache: InternalCache): ReadonlyA emoji: { type: 'string', description: 'Substrate-native emoji name' }, channel_name: { type: 'string', description: 'Channel name (required on cache miss)' }, thread: { type: 'string', description: 'Thread / topic name' }, - session_id: sessionIdField, cwd: cwdField, }, required: ['message_id', 'emoji'], additionalProperties: false, }, + hostSuppliedArgs: hostSuppliedSessionId, handler: async (args) => { const run = runFor(args) await run( @@ -885,12 +911,12 @@ const buildToolDefs = (deps: RegisterToolsDeps, cache: InternalCache): ReadonlyA description: 'Subscribe-target token: "", "/", or "new-topics:"', }, - session_id: sessionIdField, cwd: cwdField, }, required: ['target'], additionalProperties: false, }, + hostSuppliedArgs: hostSuppliedSessionId, handler: async (args) => { const run = runFor(args) await run( @@ -937,12 +963,12 @@ const buildToolDefs = (deps: RegisterToolsDeps, cache: InternalCache): ReadonlyA description: 'Subscribe-target token: "", "/", or "new-topics:"', }, - session_id: sessionIdField, cwd: cwdField, }, required: ['target'], additionalProperties: false, }, + hostSuppliedArgs: hostSuppliedSessionId, handler: async (args) => { const run = runFor(args) await run( @@ -1326,11 +1352,18 @@ export const registerTools = (server: Server, deps: RegisterToolsDeps): ToolsCac throw new Error(`unknown tool: ${request.params.name}`) } const args = (request.params.arguments ?? {}) as Readonly> - const knownKeys = new Set(Record.keys(def.inputSchema.properties)) - const unknownKeys = Record.keys(args).filter((k) => !knownKeys.has(k)) + // Two sets, and the difference between them is the point. What the tool + // ADVERTISES is what a caller may write; what it ACCEPTS additionally + // includes the arguments the host stamps in out of band. So the guard + // admits `hostSuppliedArgs` while the error still names only the advertised + // ones — an unknown-argument message that listed `session_id` as valid + // would put the parameter back on the surface this removed it from. + const advertisedKeys = Record.keys(def.inputSchema.properties) + const acceptedKeys = new Set([...advertisedKeys, ...(def.hostSuppliedArgs ?? [])]) + const unknownKeys = Record.keys(args).filter((k) => !acceptedKeys.has(k)) if (unknownKeys.length > 0) { throw new Error( - `${def.name}: unknown argument(s): ${unknownKeys.join(', ')}. Valid arguments: ${[...knownKeys].join(', ')}`, + `${def.name}: unknown argument(s): ${unknownKeys.join(', ')}. Valid arguments: ${advertisedKeys.join(', ')}`, ) } try {