From 1c26b664626b3113c616566fb8d802a01b2d3a6f Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Tue, 9 Jun 2026 12:47:42 +0200 Subject: [PATCH] Surface Guardian reviews as tool calls --- src/CodexEventHandler.ts | 33 +++- src/CodexToolCallMapper.ts | 141 ++++++++++++++++++ ...proval-review-completed-without-start.json | 46 ++++++ .../data/guardian-approval-review-flow.json | 94 ++++++++++++ .../guardian-approval-review-events.test.ts | 117 +++++++++++++++ 5 files changed, 428 insertions(+), 3 deletions(-) create mode 100644 src/__tests__/CodexACPAgent/data/guardian-approval-review-completed-without-start.json create mode 100644 src/__tests__/CodexACPAgent/data/guardian-approval-review-flow.json create mode 100644 src/__tests__/CodexACPAgent/guardian-approval-review-events.test.ts diff --git a/src/CodexEventHandler.ts b/src/CodexEventHandler.ts index 70c31314..e074e887 100644 --- a/src/CodexEventHandler.ts +++ b/src/CodexEventHandler.ts @@ -15,8 +15,11 @@ import type { ConfigWarningNotification, ErrorNotification, GuardianWarningNotification, + ItemGuardianApprovalReviewCompletedNotification, + ItemGuardianApprovalReviewStartedNotification, ItemCompletedNotification, - ItemStartedNotification, ThreadItem, + ItemStartedNotification, + ThreadItem, ModelReroutedNotification, ThreadTokenUsageUpdatedNotification, TurnPlanUpdatedNotification, @@ -28,6 +31,8 @@ import { createCommandExecutionUpdate, createDynamicToolCallUpdate, createFileChangeUpdate, + createGuardianApprovalReviewToolCall, + createGuardianApprovalReviewToolCallUpdate, createMcpRawInput, createMcpRawOutput, createFuzzyFileSearchComplete, @@ -45,6 +50,7 @@ export class CodexEventHandler { private readonly sessionState: SessionState; private failure: RequestError | null = null; private readonly activeFuzzyFileSearchSessions = new Set(); + private readonly activeGuardianApprovalReviews = new Set(); constructor(connection: acp.AgentSideConnection, sessionState: SessionState) { this.connection = connection; @@ -124,6 +130,10 @@ export class CodexEventHandler { return this.createWarningEvent(notification.params); case "guardianWarning": return this.createGuardianWarningEvent(notification.params); + case "item/autoApprovalReview/started": + return this.handleGuardianApprovalReviewStarted(notification.params); + case "item/autoApprovalReview/completed": + return this.handleGuardianApprovalReviewCompleted(notification.params); case "thread/compacted": return { sessionUpdate: "agent_message_chunk", @@ -140,8 +150,6 @@ export class CodexEventHandler { return this.handleFuzzyFileSearchSessionCompleted(notification.params); // ignored events case "command/exec/outputDelta": - case "item/autoApprovalReview/started": - case "item/autoApprovalReview/completed": case "hook/started": case "hook/completed": case "item/reasoning/summaryTextDelta": @@ -486,4 +494,23 @@ export class CodexEventHandler { this.activeFuzzyFileSearchSessions.delete(toolCallId); return createFuzzyFileSearchComplete(params); } + + private handleGuardianApprovalReviewStarted( + params: ItemGuardianApprovalReviewStartedNotification + ): UpdateSessionEvent { + if (this.activeGuardianApprovalReviews.has(params.reviewId)) { + return createGuardianApprovalReviewToolCallUpdate(params); + } + this.activeGuardianApprovalReviews.add(params.reviewId); + return createGuardianApprovalReviewToolCall(params); + } + + private handleGuardianApprovalReviewCompleted( + params: ItemGuardianApprovalReviewCompletedNotification + ): UpdateSessionEvent { + if (this.activeGuardianApprovalReviews.delete(params.reviewId)) { + return createGuardianApprovalReviewToolCallUpdate(params); + } + return createGuardianApprovalReviewToolCall(params); + } } diff --git a/src/CodexToolCallMapper.ts b/src/CodexToolCallMapper.ts index e1be3fdf..06454aab 100644 --- a/src/CodexToolCallMapper.ts +++ b/src/CodexToolCallMapper.ts @@ -13,6 +13,12 @@ import type { CommandExecutionStatus, DynamicToolCallStatus, FileUpdateChange, + GuardianApprovalReview, + GuardianApprovalReviewAction, + GuardianApprovalReviewStatus, + GuardianCommandSource, + ItemGuardianApprovalReviewCompletedNotification, + ItemGuardianApprovalReviewStartedNotification, McpToolCallError, McpToolCallResult, McpToolCallStatus, @@ -24,6 +30,9 @@ import {logger} from "./Logger"; type CodexItemStatus = CommandExecutionStatus | PatchApplyStatus | McpToolCallStatus | DynamicToolCallStatus; type AcpToolCallStatus = "pending" | "in_progress" | "completed" | "failed"; +type GuardianApprovalReviewNotification = + | ItemGuardianApprovalReviewStartedNotification + | ItemGuardianApprovalReviewCompletedNotification; function toAcpStatus(status: CodexItemStatus): AcpToolCallStatus { switch (status) { @@ -143,6 +152,36 @@ export function createMcpRawOutput( }; } +export function guardianApprovalReviewToolCallId(reviewId: string): string { + return `guardian_assessment:${reviewId}`; +} + +export function createGuardianApprovalReviewToolCall( + event: GuardianApprovalReviewNotification, +): UpdateSessionEvent { + return { + sessionUpdate: "tool_call", + toolCallId: guardianApprovalReviewToolCallId(event.reviewId), + kind: "think", + title: "Guardian Review", + status: toAcpGuardianApprovalReviewStatus(event.review.status), + content: createGuardianApprovalReviewContent(event.review, event.action), + rawInput: event as unknown as Record, + }; +} + +export function createGuardianApprovalReviewToolCallUpdate( + event: GuardianApprovalReviewNotification, +): UpdateSessionEvent { + return { + sessionUpdate: "tool_call_update", + toolCallId: guardianApprovalReviewToolCallId(event.reviewId), + status: toAcpGuardianApprovalReviewStatus(event.review.status), + content: createGuardianApprovalReviewContent(event.review, event.action), + rawOutput: event as unknown as Record, + }; +} + export function fuzzyFileSearchToolCallId(sessionId: string): string { return `fuzzyFileSearch.${sessionId}`; } @@ -257,6 +296,108 @@ function createSearchTitle(query: string | null, path: string | null): string { return "Search"; } +function toAcpGuardianApprovalReviewStatus(status: GuardianApprovalReviewStatus): AcpToolCallStatus { + switch (status) { + case "inProgress": + return "in_progress"; + case "approved": + return "completed"; + case "denied": + case "aborted": + case "timedOut": + return "failed"; + } +} + +function createGuardianApprovalReviewContent( + review: GuardianApprovalReview, + action: GuardianApprovalReviewAction, +): ToolCallContent[] { + const lines = [`Status: ${formatGuardianApprovalReviewStatus(review.status)}`]; + const actionSummary = createGuardianApprovalReviewActionSummary(action); + if (actionSummary) { + lines.push(`Action: ${actionSummary}`); + } + if (review.riskLevel) { + lines.push(`Risk: ${review.riskLevel}`); + } + if (review.rationale?.trim()) { + lines.push(`Rationale: ${review.rationale}`); + } + + return [{ + type: "content", + content: { + type: "text", + text: lines.join("\n"), + }, + }]; +} + +function formatGuardianApprovalReviewStatus(status: GuardianApprovalReviewStatus): string { + switch (status) { + case "inProgress": + return "In progress"; + case "approved": + return "Approved"; + case "denied": + return "Denied"; + case "aborted": + return "Aborted"; + case "timedOut": + return "Timed out"; + } +} + +function createGuardianApprovalReviewActionSummary(action: GuardianApprovalReviewAction): string | null { + switch (action.type) { + case "command": + return `${guardianCommandSourceLabel(action.source)} ${action.command}`; + case "execve": { + const command = action.argv.length > 0 ? action.argv : [action.program]; + return `${guardianCommandSourceLabel(action.source)} ${shellJoin(command)}`; + } + case "applyPatch": + if (action.files.length === 1) { + return `apply_patch touching ${action.files[0]}`; + } + return `apply_patch touching ${action.files.length} files`; + case "networkAccess": { + const label = action.target.length > 0 ? action.target : action.host; + return `network access to ${label}`; + } + case "mcpToolCall": { + const label = action.connectorName ?? action.server; + return `MCP ${action.toolName} on ${label}`; + } + case "requestPermissions": + return action.reason ?? "request additional permissions"; + } +} + +function guardianCommandSourceLabel(source: GuardianCommandSource): string { + switch (source) { + case "shell": + return "shell"; + case "unifiedExec": + return "exec"; + } +} + +function shellJoin(args: string[]): string { + return args.map(shellQuote).join(" "); +} + +function shellQuote(arg: string): string { + if (arg.length === 0) { + return "''"; + } + if (/^[A-Za-z0-9_/:=+.,@%-]+$/.test(arg)) { + return arg; + } + return `'${arg.replace(/'/g, `'\\''`)}'`; +} + async function createPatchContent(change: FileUpdateChange): Promise { try { switch (change.kind.type) { diff --git a/src/__tests__/CodexACPAgent/data/guardian-approval-review-completed-without-start.json b/src/__tests__/CodexACPAgent/data/guardian-approval-review-completed-without-start.json new file mode 100644 index 00000000..3f9dd03d --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/guardian-approval-review-completed-without-start.json @@ -0,0 +1,46 @@ +{ + "method": "sessionUpdate", + "args": [ + { + "sessionId": "test-session-id", + "update": { + "sessionUpdate": "tool_call", + "toolCallId": "guardian_assessment:review-orphaned", + "kind": "think", + "title": "Guardian Review", + "status": "failed", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": "Status: Denied\nAction: network access to api.example.com\nRisk: high\nRationale: The network target is not permitted." + } + } + ], + "rawInput": { + "threadId": "test-session-id", + "turnId": "turn-1", + "startedAtMs": 1000, + "completedAtMs": 1800, + "reviewId": "review-orphaned", + "targetItemId": null, + "decisionSource": "agent", + "review": { + "status": "denied", + "riskLevel": "high", + "userAuthorization": "low", + "rationale": "The network target is not permitted." + }, + "action": { + "type": "networkAccess", + "target": "", + "host": "api.example.com", + "protocol": "https", + "port": 443 + } + } + } + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/data/guardian-approval-review-flow.json b/src/__tests__/CodexACPAgent/data/guardian-approval-review-flow.json new file mode 100644 index 00000000..d92e765e --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/guardian-approval-review-flow.json @@ -0,0 +1,94 @@ +{ + "method": "sessionUpdate", + "args": [ + { + "sessionId": "test-session-id", + "update": { + "sessionUpdate": "tool_call", + "toolCallId": "guardian_assessment:review-1", + "kind": "think", + "title": "Guardian Review", + "status": "in_progress", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": "Status: In progress\nAction: exec /bin/ls -l\nRisk: medium\nRationale: Checking whether this command should run automatically." + } + } + ], + "rawInput": { + "threadId": "test-session-id", + "turnId": "turn-1", + "startedAtMs": 1000, + "reviewId": "review-1", + "targetItemId": "command-1", + "review": { + "status": "inProgress", + "riskLevel": "medium", + "userAuthorization": "unknown", + "rationale": "Checking whether this command should run automatically." + }, + "action": { + "type": "execve", + "source": "unifiedExec", + "program": "/bin/ls", + "argv": [ + "/bin/ls", + "-l" + ], + "cwd": "/test/project" + } + } + } + } + ] +} +{ + "method": "sessionUpdate", + "args": [ + { + "sessionId": "test-session-id", + "update": { + "sessionUpdate": "tool_call_update", + "toolCallId": "guardian_assessment:review-1", + "status": "completed", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": "Status: Approved\nAction: exec /bin/ls -l\nRisk: low\nRationale: The command only lists files." + } + } + ], + "rawOutput": { + "threadId": "test-session-id", + "turnId": "turn-1", + "startedAtMs": 1000, + "completedAtMs": 1500, + "reviewId": "review-1", + "targetItemId": "command-1", + "decisionSource": "agent", + "review": { + "status": "approved", + "riskLevel": "low", + "userAuthorization": "medium", + "rationale": "The command only lists files." + }, + "action": { + "type": "execve", + "source": "unifiedExec", + "program": "/bin/ls", + "argv": [ + "/bin/ls", + "-l" + ], + "cwd": "/test/project" + } + } + } + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/guardian-approval-review-events.test.ts b/src/__tests__/CodexACPAgent/guardian-approval-review-events.test.ts new file mode 100644 index 00000000..820a69f5 --- /dev/null +++ b/src/__tests__/CodexACPAgent/guardian-approval-review-events.test.ts @@ -0,0 +1,117 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import type { SessionState } from "../../CodexAcpServer"; +import type { ServerNotification } from "../../app-server"; +import { + createCodexMockTestFixture, + createTestSessionState, + setupPromptAndSendNotifications, + type CodexMockTestFixture, +} from "../acp-test-utils"; +import { AgentMode } from "../../AgentMode"; + +describe("CodexEventHandler - Guardian approval review events", () => { + let mockFixture: CodexMockTestFixture; + const sessionId = "test-session-id"; + + beforeEach(() => { + mockFixture = createCodexMockTestFixture(); + vi.clearAllMocks(); + }); + + const sessionState: SessionState = createTestSessionState({ + sessionId, + currentModelId: "model-id[effort]", + agentMode: AgentMode.DEFAULT_AGENT_MODE, + }); + + it("maps Guardian review start and completion to a think tool call flow", async () => { + const started: ServerNotification = { + method: "item/autoApprovalReview/started", + params: { + threadId: sessionId, + turnId: "turn-1", + startedAtMs: 1000, + reviewId: "review-1", + targetItemId: "command-1", + review: { + status: "inProgress", + riskLevel: "medium", + userAuthorization: "unknown", + rationale: "Checking whether this command should run automatically.", + }, + action: { + type: "execve", + source: "unifiedExec", + program: "/bin/ls", + argv: ["/bin/ls", "-l"], + cwd: "/test/project", + }, + }, + }; + const completed: ServerNotification = { + method: "item/autoApprovalReview/completed", + params: { + threadId: sessionId, + turnId: "turn-1", + startedAtMs: 1000, + completedAtMs: 1500, + reviewId: "review-1", + targetItemId: "command-1", + decisionSource: "agent", + review: { + status: "approved", + riskLevel: "low", + userAuthorization: "medium", + rationale: "The command only lists files.", + }, + action: { + type: "execve", + source: "unifiedExec", + program: "/bin/ls", + argv: ["/bin/ls", "-l"], + cwd: "/test/project", + }, + }, + }; + + await setupPromptAndSendNotifications(mockFixture, sessionId, sessionState, [started, completed]); + + await expect(mockFixture.getAcpConnectionDump([])).toMatchFileSnapshot( + "data/guardian-approval-review-flow.json" + ); + }); + + it("creates a completed Guardian review tool call when the start event was missed", async () => { + const completed: ServerNotification = { + method: "item/autoApprovalReview/completed", + params: { + threadId: sessionId, + turnId: "turn-1", + startedAtMs: 1000, + completedAtMs: 1800, + reviewId: "review-orphaned", + targetItemId: null, + decisionSource: "agent", + review: { + status: "denied", + riskLevel: "high", + userAuthorization: "low", + rationale: "The network target is not permitted.", + }, + action: { + type: "networkAccess", + target: "", + host: "api.example.com", + protocol: "https", + port: 443, + }, + }, + }; + + await setupPromptAndSendNotifications(mockFixture, sessionId, sessionState, [completed]); + + await expect(mockFixture.getAcpConnectionDump([])).toMatchFileSnapshot( + "data/guardian-approval-review-completed-without-start.json" + ); + }); +});