diff --git a/config/skills/comms-playbook/SKILL.md b/config/skills/comms-playbook/SKILL.md index 36a64c2a..059c5856 100644 --- a/config/skills/comms-playbook/SKILL.md +++ b/config/skills/comms-playbook/SKILL.md @@ -56,7 +56,10 @@ Pick the destination that puts the message in front of exactly who needs it: to a direct channel between them instead of spraying a shared channel. Fewer readers, less noise, no muted-topic guesswork. Use a DM for a two-party clarification; use the shared channel when the outcome is something the lane - should be able to read later. + should be able to read later. Agents heavily prefer DMs for two-party + coordination: open or resume a peer DM with `comms_open_dm{peer_handle}`, or + use `comms_dm{peer_handle, topic, text}` to open-and-post in one call. Like any + post it names a topic, and a new topic needs `create_topic: true`. - **Report UP, delegate DOWN.** Results and asks that need your parent go to the parent; work you hand to a report goes down to it. Keep a topic to one direction of flow where you can. diff --git a/packages/compass-agent/AGENTS.md b/packages/compass-agent/AGENTS.md index 0c8ccf12..0d4950bb 100644 --- a/packages/compass-agent/AGENTS.md +++ b/packages/compass-agent/AGENTS.md @@ -36,7 +36,7 @@ restatement of the role prompt. ## The comms toolset -Five native comms tools ship (`src/comms.ts`), none of them ask-answering: +Seven native comms tools ship (`src/comms.ts`), none of them ask-answering: - `comms_post_message` — post a markdown message to a channel topic. - `comms_post_ask` — raise a structured ask (async; the answer arrives on a @@ -44,6 +44,8 @@ Five native comms tools ship (`src/comms.ts`), none of them ask-answering: - `comms_list_messages` — read a channel's recent messages. - `compass_roster` — list the agent's neighborhood/subtree/owner roster. - `compass_set_status` — set the agent's presence activity. +- `comms_open_dm` — resolve-or-create a two-party DM channel with a peer by handle. +- `comms_dm` — open (resolve-or-create) a peer DM and post a message to it in one call. `comms_post_ask` mints each `AskOption.id` as the option's zero-based index (a decimal string) — the native SDK ask option carries no id, and the id is the diff --git a/packages/compass-agent/src/cli.test.ts b/packages/compass-agent/src/cli.test.ts index 900506fa..98bddc42 100644 --- a/packages/compass-agent/src/cli.test.ts +++ b/packages/compass-agent/src/cli.test.ts @@ -2571,7 +2571,7 @@ describe("main wires the mounted agent-config into createAgentSession", () => { ...createLifecycleTools(new LifecycleBroker(fakeTransport)), ...createForgeTools(new ForgeBroker(fakeTransport)), ]; - expect(natives).toHaveLength(17); + expect(natives).toHaveLength(19); for (const tool of natives) { expect({ name: tool.name, arity: tool.execute.length }).toEqual({ name: tool.name, @@ -2621,7 +2621,7 @@ describe("main wires the mounted agent-config into createAgentSession", () => { // natives are ALWAYS merged in (RIG-1741/RIG-2672) — so customTools carries // exactly those, and never a discovered MCP tool. expect(toolNames(seen[0].customTools)).toContain("agents_spawn_peer"); - expect(seen[0].customTools).toHaveLength(17); + expect(seen[0].customTools).toHaveLength(19); expect(seen[0].enableMCP).toBe(false); }); @@ -2654,7 +2654,7 @@ describe("main wires the mounted agent-config into createAgentSession", () => { // (RIG-1741/RIG-2672) — so customTools is exactly the comms/lifecycle/forge // natives. expect(toolNames(seen[0].customTools)).toContain("comms_post_message"); - expect(seen[0].customTools).toHaveLength(17); + expect(seen[0].customTools).toHaveLength(19); }); // ── RIG-1732 T10: COMPASS_ROLE → prompts//SYSTEM.md → customSystemPrompt ── diff --git a/packages/compass-agent/src/comms.test.ts b/packages/compass-agent/src/comms.test.ts index dad244e0..882dd3fe 100644 --- a/packages/compass-agent/src/comms.test.ts +++ b/packages/compass-agent/src/comms.test.ts @@ -17,7 +17,9 @@ import { CommsBroker, type CommsTransport, createCommsTools, + dmParameters, listParameters, + openDmParameters, postAskParameters, postParameters, } from "./comms"; @@ -26,6 +28,7 @@ import { AskOptionSchema, AskQuestionSchema, AskSchema, + ChannelSchema, CommsCallErrorSchema, type CommsCallRequest, CommsCallRequestSchema, @@ -37,6 +40,7 @@ import { type Message, MessageBlockSchema, MessageSchema, + OpenDMResponseSchema, PostMessageResponseSchema, type RosterEntry, RosterEntrySchema, @@ -55,6 +59,38 @@ class FakeTransport implements CommsTransport { } } +// The composite `comms_dm` issues two calls (open then post); this fake returns +// canned results in call order and records every request, so ordering and the +// per-leg wire shape are both assertable. +class SequencedTransport implements CommsTransport { + readonly requests: CommsCallRequest[] = []; + #next = 0; + constructor(private readonly results: CommsCallResult[]) {} + async comms(req: CommsCallRequest): Promise { + this.requests.push(req); + const r = this.results[this.#next++]; + if (!r) + throw new Error("SequencedTransport: no canned result for this call"); + return r; + } +} + +function openDmResult(channelName: string, created: boolean): CommsCallResult { + return create(CommsCallResultSchema, { + callId: "call-1", + result: { + case: "openDm", + value: create(OpenDMResponseSchema, { + channel: create(ChannelSchema, { + id: `id-${channelName}`, + name: channelName, + }), + created, + }), + }, + }); +} + function postResult(id: string, topicId: string): CommsCallResult { return create(CommsCallResultSchema, { callId: "call-1", @@ -270,7 +306,7 @@ describe("CommsBroker", () => { }); describe("createCommsTools", () => { - test("exposes exactly the five comms tools and never an ask-answering one", () => { + test("exposes exactly the seven comms tools and never an ask-answering one", () => { const tools = createCommsTools( new CommsBroker(new FakeTransport(postResult("m", "c"))), ); @@ -280,6 +316,8 @@ describe("createCommsTools", () => { "comms_list_messages", "compass_roster", "compass_set_status", + "comms_open_dm", + "comms_dm", ]); expect(tools.every((t) => t.label.length > 0)).toBe(true); // `approval` decides which modes auto-approve the call. A silent flip of @@ -299,6 +337,10 @@ describe("createCommsTools", () => { // only surface as a confusing validation failure at call time. expect(byName("comms_post_message").parameters).toBe(postParameters); expect(byName("comms_post_ask").parameters).toBe(postAskParameters); + expect(byName("comms_open_dm").approval).toBe("write"); + expect(byName("comms_dm").approval).toBe("write"); + expect(byName("comms_open_dm").parameters).toBe(openDmParameters); + expect(byName("comms_dm").parameters).toBe(dmParameters); }); }); @@ -462,6 +504,79 @@ describe("comms parameter schemas", () => { expect(listParameters.get("limit").expression).toContain(">= 1"); expect(listParameters.get("limit").description).toContain("default 50"); }); + + // open_dm requires a non-blank peer_handle — the same `.narrow` idiom. + test("open_dm requires a non-blank peer_handle", () => { + expect(rejects(openDmParameters, {})).toBe(true); + expect(rejects(openDmParameters, { peer_handle: "" })).toBe(true); + expect(rejects(openDmParameters, { peer_handle: " " })).toBe(true); + expect(rejects(openDmParameters, { peer_handle: "@bob" })).toBe(false); + }); + + // dm requires non-blank peer_handle, text, and topic; topic ≤120; + // create_topic optional boolean. + test("dm requires non-blank peer_handle, text, topic (≤120) and an optional create_topic boolean", () => { + expect(rejects(dmParameters, { topic: "t", text: "hi" })).toBe(true); + expect(rejects(dmParameters, { peer_handle: "@b", topic: "t" })).toBe(true); + expect(rejects(dmParameters, { peer_handle: "@b", text: "hi" })).toBe(true); + expect( + rejects(dmParameters, { peer_handle: " ", topic: "t", text: "hi" }), + ).toBe(true); + expect( + rejects(dmParameters, { peer_handle: "@b", topic: " ", text: "hi" }), + ).toBe(true); + expect( + rejects(dmParameters, { peer_handle: "@b", topic: "t", text: " " }), + ).toBe(true); + expect( + rejects(dmParameters, { + peer_handle: "@b", + topic: "x".repeat(121), + text: "hi", + }), + ).toBe(true); + expect( + rejects(dmParameters, { + peer_handle: "@b", + topic: "x".repeat(120), + text: "hi", + }), + ).toBe(false); + expect( + rejects(dmParameters, { + peer_handle: "@b", + topic: "t", + text: "hi", + create_topic: "yes", + }), + ).toBe(true); + expect( + rejects(dmParameters, { + peer_handle: "@b", + topic: "t", + text: "hi", + create_topic: true, + }), + ).toBe(false); + expect( + rejects(dmParameters, { peer_handle: "@b", topic: "t", text: "hi" }), + ).toBe(false); + }); + + // The DM tools' `.narrow` rules (non-blank, the create_topic gate) do not + // survive into the JSON Schema the model is shown, so the descriptions are + // the only place a caller reads them — asserted here so dropping the rule + // from a description reddens rather than silently blinding the model. + test("open_dm/dm descriptions carry the rules unrepresentable in JSON Schema", () => { + // peer_handle: the semantic miss rule (a blank/unknown/cross-owner handle + // is rejected) lives only here. + expect(openDmParameters.get("peer_handle").description).toContain("error"); + expect(dmParameters.get("peer_handle").description).toContain("error"); + // text: the non-blank rule the `.narrow` enforces but the schema cannot show. + expect(dmParameters.get("text").description).toContain("blank"); + // topic: the create_topic gate that turns a name-miss from a mint into an error. + expect(dmParameters.get("topic").description).toContain("create_topic"); + }); }); describe("comms_post_message", () => { @@ -734,6 +849,186 @@ describe("comms_post_message", () => { }); }); +describe("comms_open_dm", () => { + test("puts an open_dm call on the wire carrying the peer handle", async () => { + const transport = new FakeTransport(openDmResult("dm--alice--bob", true)); + const open = tool(new CommsBroker(transport), "comms_open_dm"); + + await exec(open, "tc-1", { peer_handle: "@bob" }); + + const req = transport.requests[0]; + expect(req?.callId).toBe("tc-1"); + expect(req?.call.case).toBe("openDm"); + if (req?.call.case !== "openDm") throw new Error("expected an openDm call"); + expect(req.call.value.peerHandle).toBe("@bob"); + }); + + test("renders Opened plus the DM channel name when created", async () => { + const transport = new FakeTransport(openDmResult("dm--alice--bob", true)); + const open = tool(new CommsBroker(transport), "comms_open_dm"); + + const text = textOf(await exec(open, "tc-1", { peer_handle: "@bob" })); + expect(text).toContain("Opened"); + expect(text).toContain("dm--alice--bob"); + }); + + test("renders Resumed when the DM already existed", async () => { + const transport = new FakeTransport(openDmResult("dm--alice--bob", false)); + const open = tool(new CommsBroker(transport), "comms_open_dm"); + + const text = textOf(await exec(open, "tc-1", { peer_handle: "@bob" })); + expect(text).toContain("Resumed"); + expect(text).toContain("dm--alice--bob"); + }); + + test("an error result throws carrying the code and the detail", async () => { + const transport = new FakeTransport( + errorResult("not_found", "no such peer"), + ); + const open = tool(new CommsBroker(transport), "comms_open_dm"); + + const err = await exec(open, "tc-1", { peer_handle: "@ghost" }).then( + () => undefined, + (e: unknown) => e as Error, + ); + expect(err).toBeInstanceOf(Error); + expect(err?.message).toContain("not_found"); + expect(err?.message).toContain("no such peer"); + }); + + test("a wrong result case is a protocol violation and throws", async () => { + const transport = new FakeTransport(postResult("m-1", "t-1")); + const open = tool(new CommsBroker(transport), "comms_open_dm"); + + await expect(exec(open, "tc-1", { peer_handle: "@bob" })).rejects.toThrow( + /comms_open_dm/, + ); + }); +}); + +describe("comms_dm", () => { + test("opens then posts, carrying the resolved DM name and topic on the post leg", async () => { + const transport = new SequencedTransport([ + openDmResult("dm--a--b", true), + postResult("m-1", "t-1"), + ]); + const dm = tool(new CommsBroker(transport), "comms_dm"); + + await exec(dm, "tc-1", { + peer_handle: "@b", + topic: "planning", + text: "hi", + }); + + expect(transport.requests).toHaveLength(2); + const open = transport.requests[0]?.call; + if (open?.case !== "openDm") throw new Error("expected an openDm call"); + expect(open.value.peerHandle).toBe("@b"); + const post = transport.requests[1]?.call; + if (post?.case !== "post") throw new Error("expected a post call"); + expect(post.value.container).toEqual({ + case: "channelId", + value: "dm--a--b", + }); + expect(post.value.topic).toEqual({ case: "topicName", value: "planning" }); + expect(post.value.blocks[0]?.block).toEqual({ case: "text", value: "hi" }); + }); + + test("mints the idempotency key on the post leg only", async () => { + const transport = new SequencedTransport([ + openDmResult("dm--a--b", true), + postResult("m-1", "t-1"), + ]); + const broker = new CommsBroker(transport); + const dm = tool(broker, "comms_dm"); + + await exec(dm, "tc-9", { peer_handle: "@b", topic: "t", text: "hi" }); + + const post = transport.requests[1]?.call; + if (post?.case !== "post") throw new Error("expected a post call"); + expect(post.value.clientRequestId).toBe(broker.idempotencyKey("tc-9")); + expect(post.value.clientRequestId).not.toBe("tc-9"); + }); + + test("create_topic defaults false and passes through on the post leg", async () => { + const transport = new SequencedTransport([ + openDmResult("dm--a--b", true), + postResult("m-1", "t-1"), + ]); + const dm = tool(new CommsBroker(transport), "comms_dm"); + await exec(dm, "tc-1", { peer_handle: "@b", topic: "t", text: "hi" }); + const post0 = transport.requests[1]?.call; + if (post0?.case !== "post") throw new Error("expected a post call"); + expect(post0.value.createTopic).toBe(false); + + const transport2 = new SequencedTransport([ + openDmResult("dm--a--b", true), + postResult("m-2", "t-2"), + ]); + const dm2 = tool(new CommsBroker(transport2), "comms_dm"); + await exec(dm2, "tc-2", { + peer_handle: "@b", + topic: "t", + text: "hi", + create_topic: true, + }); + const post1 = transport2.requests[1]?.call; + if (post1?.case !== "post") throw new Error("expected a post call"); + expect(post1.value.createTopic).toBe(true); + }); + + test("a first-leg failure short-circuits before the post leg", async () => { + const transport = new SequencedTransport([ + errorResult("not_found", "no such peer"), + ]); + const dm = tool(new CommsBroker(transport), "comms_dm"); + + const err = await exec(dm, "tc-1", { + peer_handle: "@ghost", + topic: "t", + text: "hi", + }).then( + () => undefined, + (e: unknown) => e as Error, + ); + expect(err?.message).toContain("not_found"); + expect(transport.requests).toHaveLength(1); + }); + + test("a second-leg failure renders in-band after the open leg", async () => { + const transport = new SequencedTransport([ + openDmResult("dm--a--b", true), + errorResult("not_found", "no such topic"), + ]); + const dm = tool(new CommsBroker(transport), "comms_dm"); + + const err = await exec(dm, "tc-1", { + peer_handle: "@b", + topic: "missing", + text: "hi", + }).then( + () => undefined, + (e: unknown) => e as Error, + ); + expect(err?.message).toContain("not_found"); + expect(transport.requests).toHaveLength(2); + }); + + test("the confirmation names the posted message id and the DM channel", async () => { + const transport = new SequencedTransport([ + openDmResult("dm--a--b", true), + postResult("m-42", "t-1"), + ]); + const dm = tool(new CommsBroker(transport), "comms_dm"); + + const text = textOf( + await exec(dm, "tc-1", { peer_handle: "@b", topic: "t", text: "hi" }), + ); + expect(text).toContain("m-42"); + expect(text).toContain("dm--a--b"); + }); +}); + describe("comms_list_messages", () => { test("puts a list call on the wire", async () => { const transport = new FakeTransport(listResult()); diff --git a/packages/compass-agent/src/comms.ts b/packages/compass-agent/src/comms.ts index 37fe9ae5..9e81f1e5 100644 --- a/packages/compass-agent/src/comms.ts +++ b/packages/compass-agent/src/comms.ts @@ -50,8 +50,8 @@ // `ask_answer` block on the deliver lane, rendered to the model on a subsequent // turn. See packages/compass-agent/AGENTS.md for the package contract. // -// Five tools ship: post, post_ask, list, roster, and set_status; search is -// deferred (OQ-3). +// Seven tools ship: post, post_ask, list, roster, set_status, open_dm, and dm; +// search is deferred (OQ-3). import type { AgentTool } from "@oh-my-pi/pi-agent-core"; // `arktype` is pinned exact in package.json to whatever the SDK resolves @@ -73,6 +73,7 @@ import { ListMessagesRequestSchema, type Message, MessageBlockSchema, + OpenDMRequestSchema, PostMessageRequestSchema, type RosterEntry, RosterScope, @@ -289,6 +290,36 @@ export const setStatusParameters = type({ ), }); +/** Exported so a test can validate the wire contract the agent loop enforces. */ +export const openDmParameters = type({ + peer_handle: type("string") + .narrow((s, ctx) => s.trim().length > 0 || ctx.mustBe("non-blank")) + .describe( + "The peer agent's handle to open a DM with; unknown or cross-owner is an error", + ), +}); + +/** Exported so a test can validate the wire contract the agent loop enforces. */ +export const dmParameters = type({ + peer_handle: type("string") + .narrow((s, ctx) => s.trim().length > 0 || ctx.mustBe("non-blank")) + .describe( + "The peer agent's handle to DM; unknown or cross-owner is an error", + ), + text: type("string") + .narrow((s, ctx) => s.trim().length > 0 || ctx.mustBe("non-blank")) + .describe("Markdown message body; must not be blank"), + topic: type("string") + .narrow((s, ctx) => s.trim().length > 0 || ctx.mustBe("non-blank")) + .narrow((s, ctx) => s.length <= 120 || ctx.mustBe("at most 120 characters")) + .describe( + "Named conversation within the DM; a name-miss is an error unless create_topic is true", + ), + "create_topic?": type("boolean").describe( + "When true, an unknown topic name is created; when false (default) a topic name-miss is an error", + ), +}); + /** * The `Error` a non-matching `CommsCallResult` deserves — both shapes are tool * failures under the OMP contract ("throw an error when a tool fails"): @@ -353,7 +384,7 @@ function presenceLabel(presence: AgentPresence): string { } /** - * The native comms tool set. Five tools; never an ask-answering one. + * The native comms tool set. Seven tools; never an ask-answering one. * * Wired into the container entrypoint by `cli.ts main()` (RIG-1741): the tools * are merged into the session's `customTools` and so register as `#withNatives` @@ -839,5 +870,124 @@ export function createCommsTools(broker: CommsBroker): AgentTool[] { }, }; - return [postMessage, postAsk, listMessages, roster, setStatus]; + const commsOpenDm: AgentTool = { + name: "comms_open_dm", + label: "Open peer DM", + approval: "write", + description: + "Resolve-or-create a two-party DM channel with a peer by handle and " + + "return the DM channel name to post into. Idempotent: a DM that already " + + "exists is resumed, not duplicated.", + parameters: openDmParameters, + execute: async (toolCallId, params) => { + const result = await broker.call( + create(CommsCallRequestSchema, { + callId: toolCallId, + call: { + case: "openDm", + value: create(OpenDMRequestSchema, { + peerHandle: params.peer_handle, + }), + }, + }), + ); + if (result.result.case !== "openDm") + throw commsFailure(result, "comms_open_dm", "open_dm"); + const { channel, created } = result.result.value; + if (!channel) + throw new Error( + "comms_open_dm: protocol violation — open_dm result carried no channel", + ); + // The DM name (dm----) is id-shaped, interpolated into a plain + // confirmation line the model reads as authoritative output — attr-guarded + // exactly like the post return's ids. + const verb = created ? "Opened" : "Resumed"; + return { + content: [ + { type: "text", text: `${verb} DM channel ${attr(channel.name)}.` }, + ], + }; + }, + }; + + const commsDm: AgentTool = { + name: "comms_dm", + label: "Direct-message a peer", + approval: "write", + description: + "Open (resolve-or-create) a peer DM by handle and post a markdown " + + "message to it in one call. topic names the conversation within the DM; " + + "a new topic needs create_topic: true (otherwise a topic name-miss is an " + + "error, not a silent create).", + parameters: dmParameters, + execute: async (toolCallId, params) => { + // Leg 1: resolve-or-create the DM channel. OpenDMRequest carries no + // client_request_id — only the post leg is a write that needs dedup. + const opened = await broker.call( + create(CommsCallRequestSchema, { + callId: toolCallId, + call: { + case: "openDm", + value: create(OpenDMRequestSchema, { + peerHandle: params.peer_handle, + }), + }, + }), + ); + if (opened.result.case !== "openDm") + throw commsFailure(opened, "comms_dm", "open_dm"); + const channel = opened.result.value.channel; + if (!channel) + throw new Error( + "comms_dm: protocol violation — open_dm result carried no channel", + ); + // Leg 2: post into the resolved DM by NAME (the comms edge resolves the + // name → id within the caller-visible set, exactly as comms_post_message). + // The idempotency key rides THIS leg (the write), broker-scoped. + const posted = await broker.call( + create(CommsCallRequestSchema, { + callId: toolCallId, + call: { + case: "post", + value: create(PostMessageRequestSchema, { + container: { case: "channelId", value: channel.name }, + blocks: [ + create(MessageBlockSchema, { + block: { case: "text", value: params.text }, + }), + ], + topic: { case: "topicName", value: params.topic }, + createTopic: params.create_topic ?? false, + clientRequestId: broker.idempotencyKey(toolCallId), + }), + }, + }), + ); + if (posted.result.case !== "post") + throw commsFailure(posted, "comms_dm", "post"); + const message = posted.result.value.message; + if (!message) + throw new Error( + "comms_dm: protocol violation — post result carried no message", + ); + return { + content: [ + { + type: "text", + text: `Posted message ${attr(message.id)} to DM ${attr(channel.name)} topic ${attr(message.topicId)}.`, + }, + ], + }; + }, + }; + + return [ + postMessage, + postAsk, + listMessages, + roster, + setStatus, + commsOpenDm, + commsDm, + ]; } diff --git a/packages/compass-agent/src/compassv1.ts b/packages/compass-agent/src/compassv1.ts index bdeb72db..2a3bd2e7 100644 --- a/packages/compass-agent/src/compassv1.ts +++ b/packages/compass-agent/src/compassv1.ts @@ -173,6 +173,10 @@ export { AskQuestionAnswerSchema, AskQuestionSchema, AskSchema, + // The full channel message an `OpenDMResponse` wraps — the DM tools render + // its `name` (dm----) and tests build fixtures from it. + type Channel, + ChannelSchema, // The roster read payloads the agent's `compass_roster` tool constructs: the // request names a `scope` (RosterScope) and, for an agent caller, omits the // session-resolved `agentAccountId`; the response carries the RosterEntry @@ -202,6 +206,13 @@ export { MessageSchema, type MessageUpdated, MessageUpdatedSchema, + // The peer-DM open call payload pair (peer-DM record): OpenDMRequest names a + // peer by handle; OpenDMResponse carries the resolved DM `Channel` (above) + // plus a `created` flag distinguishing a mint from a resume. + type OpenDMRequest, + OpenDMRequestSchema, + type OpenDMResponse, + OpenDMResponseSchema, type PostMessageRequest, PostMessageRequestSchema, type PostMessageResponse,