diff --git a/CHANGELOG.md b/CHANGELOG.md index 500cb41634..a75021151e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -162,6 +162,14 @@ the field, the page and the button: a card number field, a checkout page or a ne still goes to you, whatever is typed. A news wire (business, news or press wire) on a link or button no longer counts as wiring money; wiring money still does. +### A page visit an approval rule covers now asks you + +With an approval rule that asks before a Bot opens, reads or scrolls a page, the Bot was told the +action "was not started from a conversation that can ask you", did nothing, and said it could not get +an approval in this chat. The visit now waits on the approval card in the conversation, like a click +or a command does: Allow once opens the page and the Bot carries on, Deny tells the Bot it was +declined. Page visits no rule asks about still go straight to the computer. + ### A Bot's browser step is no longer refused while it is being saved Before your browser runs a click or a typed entry for a Bot, the server checks it against the copy diff --git a/agent-ag2/tests/test_main.py b/agent-ag2/tests/test_main.py index 93cf26a8c3..e3824d90d0 100644 --- a/agent-ag2/tests/test_main.py +++ b/agent-ag2/tests/test_main.py @@ -190,7 +190,11 @@ def test_an_anthropic_key_uses_the_official_endpoint_when_compose_sets_a_blank_u async def respond(transport, request): body = json.loads(request.content) - seen.append((str(request.url), request.headers.get("x-api-key"), body["model"])) + # Anthropic's SDK sends the key as `x-api-key` up to 1.x and as `Authorization: Bearer` from + # 2.x; either is the key reaching Anthropic, which is what this test is about. + bearer = request.headers.get("authorization") or "" + key = request.headers.get("x-api-key") or bearer.removeprefix("Bearer ") or None + seen.append((str(request.url), key, body["model"])) return httpx2.Response( 200, request=request, diff --git a/app/src/lib/computers/call.ts b/app/src/lib/computers/call.ts index 52a333ec41..01183616da 100644 --- a/app/src/lib/computers/call.ts +++ b/app/src/lib/computers/call.ts @@ -111,6 +111,22 @@ export function failedComputerCall( : "The computer is still starting. This call was not performed. Try it again in a moment.", }; } + /* + * A rule wants to ask about this call, and it went straight to the computer without its + * conversation. Nothing was done. `useFrontendTool` in approval-tools sends it again through the + * approval route, so this result only reaches the Bot from a surface with no conversation at all. + */ + if (body?.needsConversation === true) { + return { + ok: false, + needsConversation: true, + notPerformed: true, + reason: + typeof body.error === "string" + ? body.error + : "This action needs your approval in a chat with this Bot.", + }; + } return { ok: false, reason: typeof body?.error === "string" ? body.error : "That did not work.", diff --git a/app/src/lib/copilot/approval-tools.tsx b/app/src/lib/copilot/approval-tools.tsx index ca7014b84e..ca3eeef760 100644 --- a/app/src/lib/copilot/approval-tools.tsx +++ b/app/src/lib/copilot/approval-tools.tsx @@ -13,6 +13,11 @@ import { frontendApprovalSnapshot } from "./approval-context"; type RenderProps = { toolCallId: string; [key: string]: unknown }; +const needsConversation = (outcome: unknown) => + !!outcome && + typeof outcome === "object" && + (outcome as { needsConversation?: unknown }).needsConversation === true; + const writes = new Set([ "computer_click", "computer_type", @@ -32,6 +37,13 @@ const writes = new Set([ * handler kept the switch as it was on first render, usually still loading. Every change then went * straight to the computer outside any run, and the gate could only answer "this action needs an * authenticated run". Nothing in this handler may depend on render-time state for that reason. + * + * A LOOK GOES STRAIGHT TO THE COMPUTER, AND COMES BACK THROUGH THE GATE WHEN A RULE ASKS ABOUT IT. A + * navigation or a snapshot is usually allowed, and sending each one with the whole conversation would + * be paid on every step of every page. A rule can still ask about one ("ask before opening pages"), + * and that call arrives with no conversation to ask in, so the computer answers `needsConversation` + * having done nothing. It is sent again here through the gate, where the person is asked; before + * this, the Bot was handed the refusal and could only apologise for a card nobody was shown. */ export function useFrontendTool>( tool: ReactFrontendTool, @@ -43,7 +55,8 @@ export function useFrontendTool>( const render = tool.render; useSDKFrontendTool({ ...tool, - ...(writes.has(tool.name) + // Every computer tool, not only changes: a look a rule asks about waits on the same card. + ...(tool.name.startsWith("computer_") ? { // A component, so the approval card can use hooks; the SDK keeps this function forever. render: (props: RenderProps) => { @@ -61,14 +74,18 @@ export function useFrontendTool>( : {}), handler: tool.handler ? async (args, context) => { - if (!writes.has(tool.name)) return tool.handler?.(args, context); + let direct: unknown; + if (!writes.has(tool.name)) { + direct = await tool.handler?.(args, context); + if (!needsConversation(direct)) return direct; + } let snapshot: ReturnType; try { snapshot = frontendApprovalSnapshot(context); } catch { // A surface with no observed conversation. The server still gates the action; it can - // run it when nothing asks, and says why when something does. - return tool.handler?.(args, context); + // run it when nothing asks, and says why when something does. A look already went. + return direct ?? tool.handler?.(args, context); } let response: Response; try { diff --git a/app/tests/approval-tools.test.tsx b/app/tests/approval-tools.test.tsx index 77c881efbc..35ceb82d63 100644 --- a/app/tests/approval-tools.test.tsx +++ b/app/tests/approval-tools.test.tsx @@ -3,6 +3,7 @@ import * as ReactCoreV2 from "@copilotkit/react-core/v2"; import { GlobalRegistrator } from "@happy-dom/global-registrator"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { cleanup, render } from "@testing-library/react"; +import { failedComputerCall } from "@/lib/computers/call"; import { observeApprovalAgent } from "@/lib/copilot/approval-context"; import { useFrontendTool } from "@/lib/copilot/approval-tools"; @@ -273,3 +274,138 @@ test("a gate that cannot be reached, or answers with no result, is still a resul await clickAgainstGate(async () => Response.json({}, { status: 200 })), ).toEqual({ ok: false, reason: "The computer action returned no result." }); }); + +/* + * A look at a page a rule asks about. + * + * A navigation goes straight to the computer, without the conversation the approval card is drawn + * in, so a rule like "ask before opening pages" used to leave the gate nowhere to ask. The Bot was + * handed the refusal and told the person "this browser action requires an approval flow that isn't + * available in this chat". The computer now answers `needsConversation`, and the call goes again + * through the gate, where it waits on the card like any change. + */ +let lookOutcome: unknown; +const looked: unknown[] = []; +function LookTools() { + useFrontendTool({ + name: "computer_navigate", + description: "Open a page", + handler: async (args: Record) => { + looked.push(args); + return lookOutcome; + }, + }); + return null; +} + +async function navigateWith( + answer: (path: string) => Promise, +): Promise<{ result: unknown; paths: string[] }> { + const paths: string[] = []; + const originalFetch = global.fetch; + global.fetch = Object.assign( + async (path: Parameters[0]) => { + paths.push(String(path)); + return answer(String(path)); + }, + { preconnect: originalFetch.preconnect }, + ); + try { + render( + + + , + ); + const tool = registered.find((entry) => entry.name === "computer_navigate"); + const toolCall = { + id: "navigate-call", + type: "function" as const, + function: { + name: "computer_navigate", + arguments: '{"url":"https://example.com"}', + }, + }; + const subscribers: { + onRunInitialized?: (input: { input: unknown }) => void; + }[] = []; + const agent = { + threadId: "thread", + state: {}, + messages: [ + { id: "u1", role: "user", content: "Open example.com" }, + { id: "a1", role: "assistant", content: "", toolCalls: [toolCall] }, + ], + subscribe: (subscriber: (typeof subscribers)[number]) => { + subscribers.push(subscriber); + return { unsubscribe: () => undefined }; + }, + }; + const stop = observeApprovalAgent(agent as never); + for (const subscriber of subscribers) + subscriber.onRunInitialized?.({ + input: { + runId: "run-1", + threadId: "thread", + messages: agent.messages, + tools: [], + context: [], + state: {}, + forwardedProps: {}, + }, + }); + try { + const result = await tool?.handler?.({ url: "https://example.com" }, { + agent, + toolCall, + } as never); + return { result, paths }; + } finally { + stop(); + } + } finally { + global.fetch = originalFetch; + } +} + +test("a look a rule asks about goes again through the gate, with its conversation", async () => { + looked.length = 0; + stopped.length = 0; + lookOutcome = failedComputerCall(409, { + error: "This action needs your approval.", + needsConversation: true, + notPerformed: true, + }); + const { result, paths } = await navigateWith(async () => + Response.json({ waiting: true, approvalId: "approval-2" }, { status: 202 }), + ); + expect(looked).toHaveLength(1); + expect(paths).toEqual(["/api/approvals/computer/default"]); + expect(result).toEqual({ waiting: true }); + expect(stopped).toHaveLength(1); +}); + +test("a look nothing asks about goes straight to the computer and nowhere else", async () => { + looked.length = 0; + lookOutcome = { ok: true, title: "Example Domain" }; + const { result, paths } = await navigateWith(async () => { + throw new Error("The gate should not be called."); + }); + expect(looked).toHaveLength(1); + expect(paths).toEqual([]); + expect(result).toEqual({ ok: true, title: "Example Domain" }); +}); + +test("needing a conversation is not read as stale refs", () => { + expect( + failedComputerCall(409, { + error: "This action needs your approval.", + needsConversation: true, + notPerformed: true, + }), + ).toEqual({ + ok: false, + needsConversation: true, + notPerformed: true, + reason: "This action needs your approval.", + }); +}); diff --git a/server/src/approvals/types.ts b/server/src/approvals/types.ts index 048c17e47d..1a2b69ba4a 100644 --- a/server/src/approvals/types.ts +++ b/server/src/approvals/types.ts @@ -1,11 +1,11 @@ import { createHash } from "node:crypto"; import { RunAgentInputSchema } from "@ag-ui/core/schemas"; -import type { AuditInitiator } from "../audit"; import { type ApprovalContinuation, currentApprovalContext, withApprovalContext, } from "@openbot/toolkit/continuation"; +import type { AuditInitiator } from "../audit"; /** `handled` answers a hand-off: the person did it themselves, so it is never executed for them. */ export type ApprovalDecision = @@ -223,6 +223,23 @@ export class ApprovalRefusedError extends Error { this.name = "ApprovalRefusedError"; } } +/** + * The policy wants to ask, and the call arrived with no conversation to ask in. + * + * A browser sends a Bot's look at a page (a navigation, a snapshot) straight to the computer; only its + * changes travel through the approval route that carries the conversation. A rule can still ask about + * a look. Marked apart from other refusals so the computer routes can say so, and the browser can send + * the same call again through that route, where the person is asked, instead of handing the Bot a + * refusal it can only apologise for. + */ +export class ApprovalNeedsConversationError extends ApprovalRefusedError { + constructor() { + super( + "This action needs your approval, but it was not started from a conversation that can ask you. Nothing was done. Ask for it in a chat with this Bot, or change your rules in Settings → Approvals.", + ); + this.name = "ApprovalNeedsConversationError"; + } +} export class ApprovalNotFoundError extends Error { constructor() { super("This approval could not be found."); @@ -304,9 +321,7 @@ export function approvalAction(candidate: ApprovalCandidate): ApprovalAction { !continuation.threadId || !continuation.toolCallId ) - throw new ApprovalRefusedError( - "This action needs your approval, but it was not started from a conversation that can ask you. Nothing was done. Ask for it in a chat with this Bot, or change your rules in Settings → Approvals.", - ); + throw new ApprovalNeedsConversationError(); if ( ![ candidate.actorId, diff --git a/server/src/computer/routes.ts b/server/src/computer/routes.ts index 512e3ec8ec..55df55ddc0 100644 --- a/server/src/computer/routes.ts +++ b/server/src/computer/routes.ts @@ -1,7 +1,8 @@ +import { downloadHeaders } from "@openbot/shared/file-download"; import type { Context, MiddlewareHandler } from "hono"; import { Hono } from "hono"; -import { downloadHeaders } from "@openbot/shared/file-download"; import type { BotAccessCheck } from "../agents/profile-policy"; +import { ApprovalNeedsConversationError } from "../approvals/types"; import type { AuditReader } from "../audit"; import type { AppVariables } from "../auth/guards"; import { requireAdmin } from "../auth/guards"; @@ -1030,6 +1031,13 @@ function errorBody(error: unknown): Record { ...(error instanceof ComputerStillStartingError ? { starting: true, notPerformed: true } : {}), + /* + * A rule asks about this call and it came without the conversation the asking happens in. Marked + * so the browser sends the same call again through the approval route, which carries it. + */ + ...(error instanceof ApprovalNeedsConversationError + ? { needsConversation: true, notPerformed: true } + : {}), }; } @@ -1053,6 +1061,8 @@ function statusFor(error: unknown): 400 | 403 | 404 | 409 | 413 | 500 | 503 { // another snapshot. Not 503, which says the computer is unavailable and sends an operator hunting a // container that is running perfectly. if (error instanceof ElementNotFoundError) return 409; + // Nothing is broken here either: the call has to come again with its conversation first. + if (error instanceof ApprovalNeedsConversationError) return 409; // A person holding the wheel, or a person driving before taking it. Nothing is broken; the caller // has to wait or take control first, and 409 is how both of those are already reported. if ( diff --git a/server/tests/approval-needs-conversation.test.ts b/server/tests/approval-needs-conversation.test.ts new file mode 100644 index 0000000000..72dcdea197 --- /dev/null +++ b/server/tests/approval-needs-conversation.test.ts @@ -0,0 +1,93 @@ +import { afterEach, expect, test } from "bun:test"; +import type { MiddlewareHandler } from "hono"; +import { approvalAction } from "../src/approvals/types"; +import type { AuditEventInput, AuditStore } from "../src/audit"; +import type { AppVariables, AuthenticatedActor } from "../src/auth/guards"; +import { createComputerGateway } from "../src/computer/gateway"; +import type { PolicyStore } from "../src/computer/policy-store"; +import type { ComputerProvider } from "../src/computer/provider"; +import { createComputerRoutes } from "../src/computer/routes"; + +/** + * A look at a page that a rule asks about, sent without its conversation. + * + * The browser sends a navigation straight to the computer; only changes travel with the conversation + * the approval card is drawn in. With a rule like "ask before opening pages", the gate wanted to ask + * and had nowhere to, so the route answered a bare 500 with the refusal text, and the Bot told the + * person "this browser action requires an approval flow that isn't available in this chat". The route + * now says what the browser has to do, so it can send the same call again with the conversation. + * + * The gate here asks the way the approval service does, through `approvalAction`, and the computer is + * a real listener so the test can show nothing reached it. + */ + +const servers: Array<{ stop: (force?: boolean) => void }> = []; + +afterEach(() => { + for (const server of servers.splice(0)) server.stop(true); +}); + +test("a navigation a rule asks about comes back as needing its conversation, not done", async () => { + let computerCalls = 0; + const computer = Bun.serve({ + port: 0, + fetch: async () => { + computerCalls++; + return Response.json({ url: "https://example.com/", title: "Example" }); + }, + }); + servers.push(computer); + const provider: ComputerProvider = { + name: "test", + isolation: "per-bot", + locate: async () => `http://127.0.0.1:${computer.port}`, + status: async (botId) => ({ botId, state: "ready" }), + stop: async () => ({ wasRunning: true }), + reset: async () => ({ cleared: true }), + list: async () => [], + }; + const gateway = createComputerGateway({ + provider, + auditStore: { + insert: async (_event: AuditEventInput) => undefined, + } as unknown as AuditStore, + policy: () => ({ mode: "enforce", deny: [], allow: ["true"] }), + token: "computer-token-for-this-test", + // A rule that asks: the approval service turns the candidate into an action to wait on. + approvalGate: async (candidate) => { + approvalAction(candidate); + return undefined; + }, + }); + const actor: AuthenticatedActor = { + id: "user-1", + email: "member@openbot.test", + role: "user", + }; + const asActor: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, + ) => { + context.set("actor", actor); + await next(); + }; + const app = createComputerRoutes( + gateway, + {} as PolicyStore, + asActor, + async () => true, + ); + + const response = await app.request("http://openbot.test/bot-1/navigate", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: "https://example.com/" }), + }); + + expect(response.status).toBe(409); + expect(await response.json()).toMatchObject({ + needsConversation: true, + notPerformed: true, + }); + expect(computerCalls).toBe(0); +});