Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion agent-ag2/tests/test_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
16 changes: 16 additions & 0 deletions app/src/lib/computers/call.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down
25 changes: 21 additions & 4 deletions app/src/lib/copilot/approval-tools.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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<T extends Record<string, unknown>>(
tool: ReactFrontendTool<T>,
Expand All @@ -43,7 +55,8 @@ export function useFrontendTool<T extends Record<string, unknown>>(
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) => {
Expand All @@ -61,14 +74,18 @@ export function useFrontendTool<T extends Record<string, unknown>>(
: {}),
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<typeof frontendApprovalSnapshot>;
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 {
Expand Down
136 changes: 136 additions & 0 deletions app/tests/approval-tools.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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<string, unknown>) => {
looked.push(args);
return lookOutcome;
},
});
return null;
}

async function navigateWith(
answer: (path: string) => Promise<Response>,
): Promise<{ result: unknown; paths: string[] }> {
const paths: string[] = [];
const originalFetch = global.fetch;
global.fetch = Object.assign(
async (path: Parameters<typeof fetch>[0]) => {
paths.push(String(path));
return answer(String(path));
},
{ preconnect: originalFetch.preconnect },
);
try {
render(
<QueryClientProvider client={new QueryClient()}>
<LookTools />
</QueryClientProvider>,
);
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.",
});
});
23 changes: 19 additions & 4 deletions server/src/approvals/types.ts
Original file line number Diff line number Diff line change
@@ -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 =
Expand Down Expand Up @@ -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.");
Expand Down Expand Up @@ -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,
Expand Down
12 changes: 11 additions & 1 deletion server/src/computer/routes.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -1030,6 +1031,13 @@ function errorBody(error: unknown): Record<string, unknown> {
...(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 }
: {}),
};
}

Expand All @@ -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 (
Expand Down
Loading
Loading