diff --git a/CHANGELOG.md b/CHANGELOG.md index da2b039241..bed2c0e35e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,27 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. request for an administrator to switch on a work app, grant it to a Bot, or connect a Shared app's team account is kept, with who answered it and when. It starts empty and nothing is backfilled. +### A Bot with an app or web research can use its browser in a conversation again + +A Bot that held any app or research tool, such as Parallel Search, could not open a page, click or +type in a conversation: every browser and computer step came back as unavailable, and the Bot +fetched the page some other way instead. Its browser now works beside those tools, as it does for a +Bot that holds none. Routines and other unattended turns are unchanged. + +### An app a Bot could not use asks to be connected once + +When a Bot reached for an app the person had not connected, such as Airtable, the conversation drew +the Connect card twice, and still did after a reload. The built-in LangGraph Bot now names the message +its tool calls belong to the way the conversation store does, so a conversation no longer keeps a +second copy of every tool call, and a conversation that already has one draws each call once. + +### An app's steps are named for the app, never for the service behind it + +A conversation named a Composio app's work after the broker and the vendor's own prefix: "Used +Composio Airtable", then "Airtable list bases". It now says "Used Airtable", and each step reads as +the action, "List bases", with the app beside it. An Arcade app is named the same way, as the app +rather than `arcade-Gmail`. + ### Plainer words, and one word for each thing OpenBot now says **Bot** everywhere a person reads it, where screens used to mix Bot, agent, coworker diff --git a/agent-langgraph/src/stream.ts b/agent-langgraph/src/stream.ts index dbc9eb7da2..d6a2ec265a 100644 --- a/agent-langgraph/src/stream.ts +++ b/agent-langgraph/src/stream.ts @@ -143,14 +143,17 @@ export async function streamRun( | undefined; // Prose and tool calls cannot interleave inside one message. closeText(); + let parentMessageId: string | undefined; for (const message of output?.messages ?? []) { const id = message.tool_call_id ?? ""; const call = pending.get(id); if (!call) continue; + parentMessageId ??= stepMessageId(id); send({ type: "TOOL_CALL_START", toolCallId: id, toolCallName: call.name, + parentMessageId, } as BaseEvent); send({ type: "TOOL_CALL_ARGS", @@ -183,11 +186,14 @@ export async function streamRun( * because an empty run is not an error by the protocol, nothing says so. No result is sent * with them; producing it is the surface's half, and it begins the next run holding it. */ + let drainedParent: string | undefined; for (const [id, call] of pending) { + drainedParent ??= stepMessageId(id); send({ type: "TOOL_CALL_START", toolCallId: id, toolCallName: call.name, + parentMessageId: drainedParent, } as BaseEvent); send({ type: "TOOL_CALL_ARGS", @@ -235,3 +241,12 @@ export async function streamRun( } as BaseEvent); } } + +/** + * The message a step's tool calls belong to, named the way the conversation store names it: for the + * step's first call. Left unnamed, the surface named each call's message after the call itself, a + * different id from the stored one, and sent its copy back on the next turn as a second message. + */ +function stepMessageId(firstToolCallId: string): string { + return `assistant-${firstToolCallId}`; +} diff --git a/agent-langgraph/tests/stream.test.ts b/agent-langgraph/tests/stream.test.ts index 5b749767a7..25bae5c8bc 100644 --- a/agent-langgraph/tests/stream.test.ts +++ b/agent-langgraph/tests/stream.test.ts @@ -184,6 +184,55 @@ describe("a run that does produce something is left alone", () => { false, ); }); + + test("the calls of one step name the message the store keeps them in", async () => { + // The store keeps a step's calls in one message named for the first call. Without a parent the + // surface named each after itself, and sent that copy back next turn as a second message. + const sent = await collect([ + { + event: "on_chat_model_end", + data: { + output: { + tool_calls: [ + { id: "call_a", name: "list_bases", args: {} }, + { id: "call_b", name: "get_schema", args: {} }, + ], + }, + }, + }, + { + event: "on_chain_end", + name: "tools", + data: { + output: { + messages: [ + { tool_call_id: "call_a", content: "bases" }, + { tool_call_id: "call_b", content: "schema" }, + ], + }, + }, + }, + ]); + const starts = sent.filter((event) => event.type === "TOOL_CALL_START"); + expect(starts.map((event) => event.parentMessageId)).toEqual([ + "assistant-call_a", + "assistant-call_a", + ]); + }); + + test("a call the surface owns names its message the same way", async () => { + const sent = await collect([ + { + event: "on_chat_model_end", + data: { + output: { + tool_calls: [{ id: "call_1", name: "computer_navigate", args: {} }], + }, + }, + }, + ]); + expect(sent[0]).toMatchObject({ parentMessageId: "assistant-call_1" }); + }); }); describe("message boundaries across a multi-turn run", () => { diff --git a/app/src/components/channels/channel-chat.tsx b/app/src/components/channels/channel-chat.tsx index 3bd769d2e2..06bb23cdb9 100644 --- a/app/src/components/channels/channel-chat.tsx +++ b/app/src/components/channels/channel-chat.tsx @@ -124,17 +124,43 @@ export function channelHistoryNotice({ * A shorter read can still contain missing turns after unreadable rows are filtered out. Without a * following shared anchor, append the missing tail: the store cannot place it among local-only rows. * Return the original array when nothing was added so refreshes can wait for the store to catch up. + * + * Exported for its tests. */ -function mergeStoredMessages(local: Message[], stored: Message[]): Message[] { +export function mergeStoredMessages( + local: Message[], + stored: Message[], +): Message[] { const localIds = new Set(local.map((message) => message.id)); + /* + * The store can name a tool call's message differently from the live turn — `assistant-call_1` + * where the stream said `call_1` — so a stored message whose every call the live turn already + * holds is that live message, not a missing one. Added as missing, its card was drawn twice. + */ + const localByCall = new Map(); + for (const message of local) { + if (message.role !== "assistant") continue; + for (const call of message.toolCalls ?? []) + localByCall.set(call.id, message.id); + } + const heldAs = (message: Message): string | undefined => { + if (localIds.has(message.id)) return message.id; + if (message.role !== "assistant" || !message.toolCalls?.length) return; + const holders = new Set( + message.toolCalls.map((call) => localByCall.get(call.id)), + ); + const [holder] = holders; + return holders.size === 1 ? holder : undefined; + }; const seenStored = new Set(); const before = new Map(); let pending: Message[] = []; for (const message of stored) { if (seenStored.has(message.id)) continue; seenStored.add(message.id); - if (localIds.has(message.id)) { - if (pending.length > 0) before.set(message.id, pending); + const held = heldAs(message); + if (held !== undefined) { + if (pending.length > 0) before.set(held, pending); pending = []; } else { pending.push(message); diff --git a/app/src/components/channels/tool-activity.tsx b/app/src/components/channels/tool-activity.tsx index ff0c739336..7e01601da7 100644 --- a/app/src/components/channels/tool-activity.tsx +++ b/app/src/components/channels/tool-activity.tsx @@ -10,7 +10,12 @@ import { useCallback, useContext, useMemo, useSyncExternalStore } from "react"; import { Streamdown } from "streamdown"; import { PendingApprovals } from "@/components/approvals/inline-approval"; import { markdownComponents } from "@/lib/markdown"; -import { humanise, ownToolLabel, readToolName } from "@/lib/plugins/tool-name"; +import { + connectorName, + humanise, + ownToolLabel, + readToolName, +} from "@/lib/plugins/tool-name"; import { asText, forDisplay, @@ -196,15 +201,6 @@ function hostOf(url: string): string { } } -/** "google-drive" as "Google Drive". */ -function connectorName(key: string): string { - return key - .split(/[-_]/) - .filter(Boolean) - .map((word) => word[0]?.toUpperCase() + word.slice(1)) - .join(" "); -} - /** A value from a tool's arguments, short enough for one line. */ function inputValue(value: unknown): string { const text = typeof value === "string" ? value : JSON.stringify(value); diff --git a/app/src/lib/plugins/tool-name.ts b/app/src/lib/plugins/tool-name.ts index 3c23d69ad6..0fcb5630c1 100644 --- a/app/src/lib/plugins/tool-name.ts +++ b/app/src/lib/plugins/tool-name.ts @@ -39,6 +39,22 @@ export function readToolName(name: string, running = false): ToolName { */ const role = researchRoleOfTool(server, tool); if (role) return { label: RESEARCH_ROLE_LABELS[role] }; + /* + * A BROKERED APP IS NAMED ONCE, AS THE APP, BESIDE THE ACTION. Composio publishes + * `__`, so its app leads every action and reads as a vendor's prefix + * ("Airtable list bases"); Arcade has already dropped its toolkit from the name. Either way the + * action starts at the verb and the app is said once, in its own name. + */ + if (broker) { + const appWords = wordsOf(broker); + const toolWords = wordsOf(tool); + const leads = appWords.every((word, index) => toolWords[index] === word); + const action = leads ? toolWords.slice(appWords.length) : toolWords; + return { + label: action.length > 0 ? humanise(action.join(" ")) : humanise(tool), + detail: connectorName(server), + }; + } const label = humanise(tool); /* @@ -64,17 +80,20 @@ export function readToolName(name: string, running = false): ToolName { * a known limit of this rule, not a bug to chase with a word list. */ const labelWords = label.toLowerCase().split(" ").map(singular); - /* - * A BROKERED APP IS SEARCHED FOR IN THE WHOLE LABEL, VERB INCLUDED. The verb exclusion above - * exists for a server whose name collides with a verb by accident, and a broker's apps cannot - * produce that collision: Composio publishes `__`, so the app always leads and - * is never the verb. Excluding the first word here would put "googlecalendar" beside a label - * that opens with the word. - */ - const wordsActedOn = broker ? labelWords : labelWords.slice(1); - const serverWords = wordsOf(broker ?? server).map(singular); + const wordsActedOn = labelWords.slice(1); + const serverWords = wordsOf(server).map(singular); const named = containsPhrase(wordsActedOn, serverWords); - return named ? { label } : { label, detail: broker ?? server }; + return named ? { label } : { label, detail: server }; +} + +/** + * The connector a server key names, the way a person would say it: "google-drive" as "Google + * Drive", and a broker's "composio-airtable" or "arcade-Gmail" as the app alone. + */ +export function connectorName(server: string): string { + return wordsOf(brokeredApp(server) ?? server) + .map((word) => word[0]?.toUpperCase() + word.slice(1)) + .join(" "); } /** @@ -102,7 +121,7 @@ export function ownToolLabel(name: string, running = false): string | null { /** * The app behind a brokered server key, or null for a server this deployment dials itself. * - * A brokered server is keyed `-` — `composio-gmail` — because the broker is how the + * A brokered server is keyed `-` — `composio-gmail`, `arcade-Gmail` — because the broker is how the * credential is held and two brokers could both offer Gmail. Neither of those is the reader's * business: they asked their Bot about their email, not about which company holds the grant. This * is the same argument the research relabelling above makes, and the answer is the same — which @@ -111,12 +130,17 @@ export function ownToolLabel(name: string, running = false): string | null { * Only the prefix is removed, never the app. "Gmail" is what was acted upon and stays legible. */ function brokeredApp(server: string): string | null { - const prefix = "composio-"; - if (!server.startsWith(prefix)) return null; - const app = server.slice(prefix.length); - return app.length > 0 ? app : null; + for (const prefix of BROKER_PREFIXES) { + if (!server.startsWith(prefix)) continue; + const app = server.slice(prefix.length); + return app.length > 0 ? app : null; + } + return null; } +/** The brokers whose apps this deployment keys `-`. See `server/src/plugins/access.ts`. */ +const BROKER_PREFIXES = ["composio-", "arcade-"] as const; + /** * `routines` and `routine` are the same word for this purpose. * diff --git a/app/tests/merge-stored-messages.test.ts b/app/tests/merge-stored-messages.test.ts new file mode 100644 index 0000000000..2357317c7c --- /dev/null +++ b/app/tests/merge-stored-messages.test.ts @@ -0,0 +1,88 @@ +import { describe, expect, test } from "bun:test"; +import type { Message } from "@ag-ui/core"; +import { mergeStoredMessages } from "../src/components/channels/channel-chat"; + +const call = (id: string) => ({ + id, + type: "function" as const, + function: { + name: "mcp__composio-airtable__AIRTABLE_LIST_BASES", + arguments: "{}", + }, +}); + +const question: Message = { + id: "user-1", + role: "user", + content: "What bases do I have?", +}; +const result: Message = { + id: "result-1", + role: "tool", + toolCallId: "call_1", + content: "Refused. Connect Airtable.", +}; +const answer: Message = { + id: "answer-1", + role: "assistant", + content: "Airtable is not connected.", +}; + +describe("mergeStoredMessages", () => { + test("a stored tool call the live turn already holds under another id is not added again", () => { + const local: Message[] = [ + question, + { id: "call_1", role: "assistant", toolCalls: [call("call_1")] }, + result, + answer, + ]; + const stored: Message[] = [ + question, + { + id: "assistant-call_1", + role: "assistant", + toolCalls: [call("call_1")], + }, + result, + answer, + ]; + expect(mergeStoredMessages(local, stored)).toBe(local); + }); + + test("a missing stored message still lands before the live message holding the same call", () => { + const local: Message[] = [ + { id: "call_1", role: "assistant", toolCalls: [call("call_1")] }, + result, + ]; + const stored: Message[] = [ + question, + { + id: "assistant-call_1", + role: "assistant", + toolCalls: [call("call_1")], + }, + result, + ]; + expect(mergeStoredMessages(local, stored).map((m) => m.id)).toEqual([ + "user-1", + "call_1", + "result-1", + ]); + }); + + test("a stored tool call the live turn does not hold is still added", () => { + const local: Message[] = [question]; + const stored: Message[] = [ + question, + { + id: "assistant-call_2", + role: "assistant", + toolCalls: [call("call_2")], + }, + ]; + expect(mergeStoredMessages(local, stored).map((m) => m.id)).toEqual([ + "user-1", + "assistant-call_2", + ]); + }); +}); diff --git a/app/tests/tool-activity.test.tsx b/app/tests/tool-activity.test.tsx index 529e470613..123ad4794c 100644 --- a/app/tests/tool-activity.test.tsx +++ b/app/tests/tool-activity.test.tsx @@ -245,6 +245,23 @@ test("the summary says what was done, never which provider searched", () => { ).toBe("Used Google Drive"); }); +test("a brokered app is summarised as the app, never with its broker's name", () => { + const steps = [ + call("l", "mcp__composio-airtable__AIRTABLE_LIST_BASES", {}, '"x"'), + call("s", "mcp__composio-airtable__AIRTABLE_GET_BASE_SCHEMA", {}, '"x"'), + ].map(stepDetails); + expect(summarise(steps)).toBe("Used Airtable"); + expect(steps.map((step) => step.label)).toEqual([ + "List bases", + "Get base schema", + ]); + expect( + summarise( + [call("a", "mcp__arcade-Gmail__ListEmails", {}, '"x"')].map(stepDetails), + ), + ).toBe("Used Gmail"); +}); + test("a refusal and a failure each count as an issue on the folded line", () => { const group: ToolGroup = { kind: "tools", diff --git a/app/tests/tool-name.test.ts b/app/tests/tool-name.test.ts index 2f67894305..1a29fccd3e 100644 --- a/app/tests/tool-name.test.ts +++ b/app/tests/tool-name.test.ts @@ -68,34 +68,35 @@ describe("naming a tool call", () => { }); }); - test("a brokered app never puts the broker on screen", () => { + test("a brokered app's action is named without any vendor's prefix", () => { // Composio is how this deployment reaches Gmail, which is plumbing: the reader asked their Bot - // about their email, not about which broker holds the grant. The app also names itself in every - // tool it publishes, so "Gmail fetch emails" already says which mailbox was read. + // about their email, not about which broker holds the grant. Composio also prefixes every tool + // with the app, so the app is dropped from the action and named once, beside it. expect(readToolName("mcp__composio-gmail__GMAIL_FETCH_EMAILS")).toEqual({ - label: "Gmail fetch emails", - }); - expect(readToolName("mcp__composio-slack__SLACK_SEND_EMAIL")).toEqual({ - label: "Slack send email", + label: "Fetch emails", + detail: "Gmail", }); + expect(readToolName("mcp__composio-airtable__AIRTABLE_LIST_BASES")).toEqual( + { label: "List bases", detail: "Airtable" }, + ); + expect( + readToolName("mcp__composio-googlecalendar__GOOGLECALENDAR_CREATE_EVENT"), + ).toEqual({ label: "Create event", detail: "Googlecalendar" }); }); - test("a brokered app the label does not name is kept, without its broker", () => { - // The app still belongs on screen when the action does not say it — but as the app, never as - // `composio-gmail`. This is what proves the prefix is stripped rather than the detail deleted. + test("an action that does not lead with its app is kept whole, beside the app", () => { expect(readToolName("mcp__composio-gmail__FETCH_EMAILS")).toEqual({ label: "Fetch emails", - detail: "gmail", + detail: "Gmail", }); }); - test("a brokered app named by the label's first word is still dropped", () => { - // Composio writes `__`, so the app leads and is never the verb. The rule that - // protects `posts`/`post_message` excludes the first word from the search, and applying it here - // would put "googlecalendar" beside a label that opens with it. - expect( - readToolName("mcp__composio-googlecalendar__GOOGLECALENDAR_CREATE_EVENT"), - ).toEqual({ label: "Googlecalendar create event" }); + test("an Arcade app is named as the app, never as arcade-", () => { + // Arcade already strips its toolkit from the action, so only the broker prefix is left to drop. + expect(readToolName("mcp__arcade-Gmail__ListEmails")).toEqual({ + label: "List emails", + detail: "Gmail", + }); }); test("a component the app registered is left alone", () => { diff --git a/server/tests/headless-tools.test.ts b/server/tests/headless-tools.test.ts index b15558a0d7..db4a7c218d 100644 --- a/server/tests/headless-tools.test.ts +++ b/server/tests/headless-tools.test.ts @@ -11,6 +11,7 @@ import { parseApprovalContinuation } from "../src/approvals/types"; import type { AuditEventInput } from "../src/audit"; import { createComputerGateway } from "../src/computer/gateway"; import * as headless from "../src/computer/headless-tools"; +import { HeadlessToolsMiddleware } from "@openbot/toolkit/headless-tools"; import { createTurnRunner, type RunnerLike } from "@openbot/toolkit/run-turn"; import { serverTurnAdmission } from "../src/agents/turn-admission"; @@ -523,6 +524,62 @@ test("remote backend tool results are persisted without executing the client too ).toHaveLength(1); }); +test("a computer tool the caller offers is left to the caller when OpenBot adds plugin tools beside it", async () => { + const remote = remoteTurn((input) => + lifecycle( + input, + toolCall("computer_navigate", '{"url":"https://example.com"}'), + ), + ); + let executions = 0; + const middleware = new HeadlessToolsMiddleware([ + { + definition: { + name: "web_search", + description: "Search the public web", + parameters: { type: "object", properties: {} }, + }, + execute: async () => { + executions += 1; + return "Results"; + }, + }, + ]); + const events: BaseEvent[] = []; + await new Promise((resolve, reject) => + middleware + .run( + { + threadId: "thread", + runId: "run", + messages: [], + state: {}, + context: [], + forwardedProps: {}, + tools: [ + { + name: "computer_navigate", + description: "Open a page in the Bot's browser", + parameters: { type: "object", properties: {} }, + }, + ], + }, + remote.agent, + ) + .subscribe({ + next: (event) => events.push(event), + error: reject, + complete: resolve, + }), + ); + expect(executions).toBe(0); + expect(remote.requests).toHaveLength(1); + expect( + events.filter((event) => event.type === EventType.TOOL_CALL_RESULT), + ).toHaveLength(0); + expect(events.at(-1)?.type).toBe(EventType.RUN_FINISHED); +}); + test("unknown computer tool calls become error results rather than commands", async () => { const remote = remoteTurn((input, index) => lifecycle( diff --git a/toolkit/src/headless-tools.ts b/toolkit/src/headless-tools.ts index 58958a2f92..abfbf2ff53 100644 --- a/toolkit/src/headless-tools.ts +++ b/toolkit/src/headless-tools.ts @@ -73,6 +73,12 @@ export class HeadlessToolsMiddleware extends Middleware { const tools = new Map( this.tools.map((tool) => [tool.definition.name, tool]), ); + // A browser chat offers its own computer tools; a plugin added beside them must not answer those. + const callerOffers = new Set( + originalInput.tools + .map((tool) => tool.name) + .filter((name) => !tools.has(name)), + ); const executed = new Set(); let subscription: Subscription | undefined; let started = false; @@ -161,7 +167,8 @@ export class HeadlessToolsMiddleware extends Middleware { const owned = pending.filter( (call) => tools.has(call.function.name) || - call.function.name.startsWith("computer_"), + (call.function.name.startsWith("computer_") && + !callerOffers.has(call.function.name)), ); if (owned.length === 0) { subscriber.next(terminal);