diff --git a/CHANGELOG.md b/CHANGELOG.md index 06ba50531..a116f2b4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A Bot running its own loop is told a vendor's error is the vendor's + +A vendor that says no by answering with an error, the way an MCP server refuses a call, reached a Bot +running here as "The vendor reported an error: …", and reached a Bot calling tools back from its own +process, such as the LangGraph Bots, as the bare sentence. Those Bots pass the answer on as they +receive it, so their model read something like Google's "The caller does not have permission" as an +ordinary result, and could tell the person they had no access rather than that the vendor had refused. +Both kinds of Bot are now told the same thing. A result that is not an error, and this deployment's +own refusals, read as before. ### A long Composio result or failure is cut between characters, not through an emoji A Composio action's answer over 20,000 characters, and a failure sentence as long, were cut by UTF-16 diff --git a/server/src/app.ts b/server/src/app.ts index 9598f67da..78ce54848 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -60,7 +60,7 @@ import { MAX_PAGE, type PeopleStore } from "./people/store"; import type { ComposioBroker } from "./plugins/broker"; import { createPluginRoutes } from "./plugins/routes"; import { isDeploymentFault, type PluginStore } from "./plugins/store"; -import { REFUSAL_MARKER } from "./plugins/tools"; +import { REFUSAL_MARKER, vendorAnswer } from "./plugins/tools"; import { createRoutineRoutes, type RoutineStore } from "./routines/routes"; import type { RoutineRunner } from "./routines/runner"; import type { IntentRouter } from "./routing/classify"; @@ -1382,7 +1382,12 @@ export function createApp( actorId: verdict.actorId, ...(verdict.initiator ? { initiator: verdict.initiator } : {}), }); - return context.json({ text: result.text, isError: result.isError }); + // Worded by the helper the in-process door uses, so a framework Bot's model reads a vendor's + // error as the vendor's and not as a result. Neither Bot words it on its way through. + return context.json({ + text: vendorAnswer(result), + isError: result.isError, + }); } catch (error) { /* * A refusal is an answer, not a failure: the Bot says what was blocked and carries on. The diff --git a/server/src/plugins/tools.ts b/server/src/plugins/tools.ts index 905a815fa..e6e5c09bb 100644 --- a/server/src/plugins/tools.ts +++ b/server/src/plugins/tools.ts @@ -32,6 +32,30 @@ import { */ export const REFUSAL_MARKER = "Refused."; +/** + * What a model is told a vendor answered: its result as written, or its error named as one. + * + * `isError` used to be dropped, and it cost a diagnosis. Google refused the Drive MCP server with + * `isError: true` and the text "The caller does not have permission"; the model received that as an + * ordinary result, believed it, and told the person it had no access to their Drive — which read as + * the Bot being confused rather than as the vendor refusing. + * + * The prefix is the vendor's, and says so. It is deliberately NOT `REFUSAL_MARKER`: that one means + * this deployment declined, and the transcript draws it as a boundary holding. A vendor saying no is + * a different fact with a different fix, and collapsing the two would make a misconfigured connector + * look like a policy working correctly. + * + * One function for both doors to one store — {@link grantedTools} for a Bot running here, and + * `/api/agent-tools/call` for a Bot running its own loop — because the second answered with the bare + * text, and neither framework Bot words an `isError` answer on its way through. Which door a Bot + * arrives at is a deployment topology decision, not a decision about what its model is told. + */ +export function vendorAnswer(result: { text: string; isError: boolean }) { + return result.isError + ? `The vendor reported an error: ${result.text}` + : result.text; +} + export type GrantedTool = { name: string; description: string; @@ -187,22 +211,7 @@ export async function grantedTools(options: { actorId, ...(initiator ? { initiator } : {}), }); - /* - * A vendor's error is named as one, not handed over as content. - * - * `isError` used to be dropped here, and it cost a diagnosis. Google refused the Drive MCP - * server with `isError: true` and the text "The caller does not have permission"; the model - * received that as an ordinary result, believed it, and told the person it had no access to - * their Drive — which read as the Bot being confused rather than as the vendor refusing. - * - * The prefix is the vendor's, and says so. It is deliberately NOT `REFUSAL_MARKER`: that one - * means this deployment declined, and the transcript draws it as a boundary holding. A vendor - * saying no is a different fact with a different fix, and collapsing the two would make a - * misconfigured connector look like a policy working correctly. - */ - return result.isError - ? `The vendor reported an error: ${result.text}` - : result.text; + return vendorAnswer(result); } catch (error) { if (error instanceof PluginRefusedError) { return `${REFUSAL_MARKER} ${error.message}`; diff --git a/server/tests/agent-callback-token.test.ts b/server/tests/agent-callback-token.test.ts index 76be1cb31..64b1190f2 100644 --- a/server/tests/agent-callback-token.test.ts +++ b/server/tests/agent-callback-token.test.ts @@ -496,4 +496,104 @@ describe("the tool-call route a callback token guards", () => { await toolResult(new Error("The caller does not have permission.")), ).toContain("The caller does not have permission."); }); + + /** + * A VENDOR THAT ANSWERED WITH AN ERROR, rather than one that threw, which is how an MCP server says + * no: `{ isError: true }` and a sentence, resolved and not thrown. + * + * The in-process door names that sentence as the vendor's (`plugins/tools.ts`), because handing it + * over as content already cost a diagnosis: Google's "The caller does not have permission" read as + * a result, and the model told the person it had no access to their Drive. Neither framework Bot + * words it on its way through — the LangGraph Bot passes an `isError` answer on untouched, and + * the Python one reads only `text` — so what this route writes is what the model reads. + */ + function storeAnswering(result: { text: string; isError: boolean }) { + return { + callTool: async () => ({ ...result, truncated: false }), + listForAgent: async () => ({ + tools: [ + { + toolName: "mcp__linear__LINEAR_CREATE_ISSUE", + ref: "linear/LINEAR_CREATE_ISSUE", + description: "Create an issue.", + inputSchema: { type: "object" }, + }, + ], + }), + } as unknown as PluginStore; + } + + /** Both doors' answers to one call against the same store: the callback route's, and the in-process one's. */ + async function bothDoors(store: PluginStore) { + const response = await createApp( + config, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + store, + ).request("http://openbot.local/api/agent-tools/call", { + method: "POST", + headers: { + "content-type": "application/json", + "x-openbot-agent-token": DEPLOYMENT_TOKEN, + }, + body: JSON.stringify({ + name: "mcp__linear__LINEAR_CREATE_ISSUE", + args: {}, + run: mintRunAssertion( + { botId: "knowledge", actorId: "usr_7", runId: "run_1" }, + config.keyEncryptionKey, + ), + }), + }); + expect(response.status).toBe(200); + const callback = (await response.json()) as { + text: string; + isError: boolean; + }; + + const { grantedTools } = await import("../src/plugins/tools"); + const [tool] = await grantedTools({ + store, + botId: "knowledge", + actorId: "usr_7", + }); + const inProcess = await tool?.execute({}); + return { callback, inProcess }; + } + + test("a vendor's error answer is named as the vendor's, the way the in-process door names it", async () => { + const { callback, inProcess } = await bothDoors( + storeAnswering({ + text: "The caller does not have permission.", + isError: true, + }), + ); + + expect(callback.isError).toBe(true); + // One store, one answer: which door a Bot comes through is topology, not what its model is told. + expect(callback.text).toBe(inProcess); + expect(callback.text).toBe( + "The vendor reported an error: The caller does not have permission.", + ); + }); + + test("a vendor's result that is not an error reaches the model as the vendor wrote it", async () => { + const { callback, inProcess } = await bothDoors( + storeAnswering({ text: "Created LIN-42.", isError: false }), + ); + + expect(callback).toEqual({ text: "Created LIN-42.", isError: false }); + expect(callback.text).toBe(inProcess); + }); });