diff --git a/docs/tools/exec-approvals-advanced.md b/docs/tools/exec-approvals-advanced.md index 1394420f767..9c3d1dc8900 100644 --- a/docs/tools/exec-approvals-advanced.md +++ b/docs/tools/exec-approvals-advanced.md @@ -162,11 +162,16 @@ Approval-backed interpreter/runtime runs are intentionally conservative: When approvals are required, the exec tool returns immediately with an approval id. Use that id to correlate later approved-run system events (`Exec finished`, and `Exec running` when configured). If no decision arrives before the timeout, the request is treated as an approval timeout and -surfaced as a terminal denial rather than an agent-waking system event. +surfaced as a terminal host-command denial. For main-agent async approvals with an originating +session, OpenClaw also resumes that session with an internal followup so the agent observes that +the command did not run instead of later repairing a missing result. ### Followup delivery behavior After an approved async exec finishes, OpenClaw sends a followup `agent` turn to the same session. +Denied async approvals use the same main-session followup path for the denial status, but they do +not register elevated runtime handoffs and they do not run the command. Denials without a resumable +main session are either suppressed or reported through a safe direct route when one exists. - If a valid external delivery target exists (deliverable channel plus target `to`), followup delivery uses that channel. - In webchat-only or internal-session flows with no external target, followup delivery stays session-only (`deliver: false`). diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index f9c8f39eea5..a1a0af55184 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -443,9 +443,13 @@ Exec lifecycle is surfaced as system messages: - `Exec finished`. These are posted to the agent's session after the node reports the event. -Denied exec approvals are terminal: OpenClaw can report the denial to the -operator or direct chat route, but it does not post `Exec denied` back into the -agent session or wake agent work. +Denied exec approvals are terminal for the host command itself: the command +does not run. For main-agent async approvals with an originating session, +OpenClaw posts the denial back into that session as an internal followup so the +agent can stop waiting on the async command and avoid a missing-result repair. +If there is no session or the session cannot be resumed, OpenClaw can still +report a concise denial to the operator or direct chat route. Denials for +subagent sessions are not posted back into the subagent. Gateway-host exec approvals emit the same lifecycle events when the command finishes (and optionally when running longer than the threshold). Approval-gated execs reuse the approval id as the `runId` in these @@ -453,11 +457,12 @@ messages for easy correlation. ## Denied approval behavior -When an async exec approval is denied, OpenClaw treats the request as terminal. -It can show a concise denial to the operator or direct chat route, but it does -not send denial guidance back through the agent session. That keeps a denied -command from becoming another model turn and prevents the agent from reusing -output from an earlier run of the same command. +When an async exec approval is denied, OpenClaw treats the host command as +terminal and fail-closed. For main-agent sessions, the denial is delivered as an +internal session followup that tells the agent the async command did not run. +That preserves transcript continuity without exposing stale command output. If +session delivery is unavailable, OpenClaw falls back to a concise operator or +direct-chat denial when a safe route exists. ## Implications diff --git a/src/agents/bash-tools.exec-approval-followup.test.ts b/src/agents/bash-tools.exec-approval-followup.test.ts index 61a912a4c54..44b48e825f4 100644 --- a/src/agents/bash-tools.exec-approval-followup.test.ts +++ b/src/agents/bash-tools.exec-approval-followup.test.ts @@ -113,17 +113,26 @@ describe("exec approval followup", () => { expect(sendMessage).not.toHaveBeenCalled(); }); - it("suppresses denied followups for normal sessions", async () => { - await expect( - sendExecApprovalFollowup({ - approvalId: "req-denied-main", - sessionKey: "agent:main:main", - turnSourceChannel: "webchat", - resultText: "Exec denied (gateway id=req-denied-main, user-denied): uname -a", - }), - ).resolves.toBe(false); + it("routes denied followups through the originating main session", async () => { + await sendExecApprovalFollowup({ + approvalId: "req-denied-main", + sessionKey: "agent:main:main", + turnSourceChannel: "webchat", + resultText: "Exec denied (gateway id=req-denied-main, user-denied): uname -a", + }); - expect(callGatewayTool).not.toHaveBeenCalled(); + const agentArgs = expectGatewayAgentFollowup({ + sessionKey: "agent:main:main", + deliver: false, + channel: "webchat", + idempotencyKey: "exec-approval-followup:req-denied-main", + }); + expect(agentArgs.message).toContain("An async command did not run."); + expect(agentArgs.message).toContain( + "Exec denied (gateway id=req-denied-main, user-denied): uname -a", + ); + expect(agentArgs.message).not.toContain("missing tool result"); + expect(agentArgs.message).not.toContain("transcript repair"); expect(sendMessage).not.toHaveBeenCalled(); }); @@ -287,7 +296,9 @@ describe("exec approval followup", () => { }); }); - it("uses safe direct denied copy without resuming the session", async () => { + it("falls back to safe direct denied copy when session resume fails", async () => { + vi.mocked(callGatewayTool).mockRejectedValueOnce(new Error("session missing")); + await sendExecApprovalFollowup({ approvalId: "req-denied-resume-failed", sessionKey: "agent:main:telegram:-100123", @@ -302,10 +313,12 @@ describe("exec approval followup", () => { content: "Command did not run: approval timed out.", idempotencyKey: "exec-approval-followup:req-denied-resume-failed", }); - expect(callGatewayTool).not.toHaveBeenCalled(); + expect(callGatewayTool).toHaveBeenCalledTimes(1); }); - it("uses safe direct denied copy for nested-parentheses denial metadata without resuming the session", async () => { + it("falls back to safe direct denied copy for nested-parentheses denial metadata", async () => { + vi.mocked(callGatewayTool).mockRejectedValueOnce(new Error("session missing")); + await sendExecApprovalFollowup({ approvalId: "req-denied-resume-failed-nested", sessionKey: "agent:main:telegram:-100123", @@ -321,7 +334,7 @@ describe("exec approval followup", () => { content: "Command did not run: approval timed out.", idempotencyKey: "exec-approval-followup:req-denied-resume-failed-nested", }); - expect(callGatewayTool).not.toHaveBeenCalled(); + expect(callGatewayTool).toHaveBeenCalledTimes(1); }); it("suppresses denied followups for subagent sessions", async () => { diff --git a/src/agents/bash-tools.exec-approval-followup.ts b/src/agents/bash-tools.exec-approval-followup.ts index 1228d7f790a..5982c011035 100644 --- a/src/agents/bash-tools.exec-approval-followup.ts +++ b/src/agents/bash-tools.exec-approval-followup.ts @@ -292,21 +292,12 @@ export async function sendExecApprovalFollowup( ? normalizedTurnSourceChannel : undefined; - if (isDenied) { - if (!sessionKey || shouldSuppressExecDeniedFollowup(sessionKey)) { - return false; - } - return await sendDirectFollowupFallback({ - approvalId: params.approvalId, - deliveryTarget, - resultText, - sessionError: null, - allowDenied: true, - }); - } - let sessionError: unknown = null; + if (isDenied && (!sessionKey || shouldSuppressExecDeniedFollowup(sessionKey))) { + return false; + } + if (sessionKey && params.direct !== true) { try { const agentArgs = buildAgentFollowupArgs({ @@ -342,6 +333,24 @@ export async function sendExecApprovalFollowup( } } + if (isDenied) { + if ( + await sendDirectFollowupFallback({ + approvalId: params.approvalId, + deliveryTarget, + resultText, + sessionError, + allowDenied: true, + }) + ) { + return true; + } + if (sessionError) { + throw new Error(`Session followup failed: ${formatUnknownError(sessionError)}`); + } + return false; + } + if ( await sendDirectFollowupFallback({ approvalId: params.approvalId, diff --git a/src/agents/bash-tools.exec-host-gateway.test.ts b/src/agents/bash-tools.exec-host-gateway.test.ts index 48d85f4bdaf..f633d3af0e9 100644 --- a/src/agents/bash-tools.exec-host-gateway.test.ts +++ b/src/agents/bash-tools.exec-host-gateway.test.ts @@ -332,6 +332,7 @@ describe("processGatewayAllowlist", () => { askFallback: "full" | "allowlist"; approvedByAsk: boolean; }) { + buildExecApprovalFollowupTargetMock.mockImplementation((value) => value); resolveExecHostApprovalContextMock.mockReturnValue({ approvals: { allowlist: [], file: { version: 1, agents: {} } }, hostSecurity: params.security, @@ -355,6 +356,7 @@ describe("processGatewayAllowlist", () => { security: params.security, ask: "always", strictInlineEval: true, + sessionKey: "agent:main:main", }); } @@ -1117,7 +1119,12 @@ EOF`, expect(result.pendingResult?.details.status).toBe("approval-pending"); await vi.waitFor(() => { expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledWith( - null, + expect.objectContaining({ + approvalId: "req-1", + sessionKey: "agent:main:main", + turnSourceChannel: undefined, + direct: false, + }), "Exec denied (gateway id=req-1, approval-timeout): python3 -c 'print(1)'", ); }); @@ -1135,7 +1142,12 @@ EOF`, expect(result.pendingResult?.details.status).toBe("approval-pending"); await vi.waitFor(() => { expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledWith( - null, + expect.objectContaining({ + approvalId: "req-1", + sessionKey: "agent:main:main", + turnSourceChannel: undefined, + direct: false, + }), "Exec denied (gateway id=req-1, approval-timeout): python3 -c 'print(1)'", ); }); diff --git a/src/agents/bash-tools.exec-host-node.test.ts b/src/agents/bash-tools.exec-host-node.test.ts index cb2774f6960..761302a603b 100644 --- a/src/agents/bash-tools.exec-host-node.test.ts +++ b/src/agents/bash-tools.exec-host-node.test.ts @@ -168,7 +168,7 @@ vi.mock("./bash-tools.exec-host-shared.js", () => ({ buildDefaultExecApprovalRequestArgs: vi.fn(() => ({})), createAndRegisterDefaultExecApprovalRequest: createAndRegisterDefaultExecApprovalRequestMock, shouldResolveExecApprovalUnavailableInline: vi.fn(() => false), - buildExecApprovalFollowupTarget: vi.fn(() => ({ approvalId: "approval-1" })), + buildExecApprovalFollowupTarget: vi.fn((value) => value), resolveApprovalDecisionOrUndefined: resolveApprovalDecisionOrUndefinedMock, createExecApprovalDecisionState: createExecApprovalDecisionStateMock, enforceStrictInlineEvalApprovalBoundary: enforceStrictInlineEvalApprovalBoundaryMock, @@ -1365,7 +1365,10 @@ describe("executeNodeHostCommand", () => { expect(autoReviewer).not.toHaveBeenCalled(); await vi.waitFor(() => { expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledWith( - { approvalId: "approval-1" }, + expect.objectContaining({ + approvalId: "approval-1", + sessionKey: "requested-session", + }), "Exec denied (node=node-1 id=approval-1, approval-timeout): bun ./script.ts", ); }); @@ -1638,7 +1641,10 @@ describe("executeNodeHostCommand", () => { expect(autoReviewer).not.toHaveBeenCalled(); await vi.waitFor(() => { expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledWith( - { approvalId: "approval-1" }, + expect.objectContaining({ + approvalId: "approval-1", + sessionKey: "requested-session", + }), "Exec denied (node=node-1 id=approval-1, approval-timeout): echo 'unterminated", ); }); @@ -1693,7 +1699,10 @@ describe("executeNodeHostCommand", () => { expect(result.details?.status).toBe("approval-pending"); await vi.waitFor(() => { expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledWith( - { approvalId: "approval-1" }, + expect.objectContaining({ + approvalId: "approval-1", + sessionKey: "requested-session", + }), "Exec denied (node=node-1 id=approval-1, approval-timeout): bun ./script.ts", ); }); @@ -2057,7 +2066,10 @@ describe("executeNodeHostCommand", () => { expect(result.details?.status).toBe("approval-pending"); await vi.waitFor(() => { expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledWith( - { approvalId: "approval-1" }, + expect.objectContaining({ + approvalId: "approval-1", + sessionKey: "requested-session", + }), "Exec denied (node=node-1 id=approval-1, approval-timeout): python3 -c 'print(1)'", ); }); diff --git a/src/agents/bash-tools.exec.approval-id.test.ts b/src/agents/bash-tools.exec.approval-id.test.ts index 4eb05eb52b8..58a868ce0e0 100644 --- a/src/agents/bash-tools.exec.approval-id.test.ts +++ b/src/agents/bash-tools.exec.approval-id.test.ts @@ -1141,7 +1141,7 @@ describe("exec approvals", () => { expect(agentCalls[0]?.message).toContain("webchat-ok"); }); - it("does not spawn a gateway followup agent when approval is denied", async () => { + it("routes denied approval status through the originating session", async () => { const agentCalls: Array> = []; vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => { @@ -1172,8 +1172,19 @@ describe("exec approvals", () => { }); expect(result.details.status).toBe("approval-pending"); - await new Promise((resolve) => setTimeout(resolve, 25)); - expect(agentCalls).toHaveLength(0); + const approvalId = (result.details as { approvalId?: string }).approvalId; + expect(typeof approvalId).toBe("string"); + await expect.poll(() => agentCalls.length, { timeout: 3000, interval: 1 }).toBe(1); + expectRecordFields(agentCalls[0], { + sessionKey: "agent:main:main", + deliver: false, + idempotencyKey: `exec-approval-followup:${approvalId}`, + }); + expect(agentCalls[0]?.message).toContain("An async command did not run."); + expect(agentCalls[0]?.message).toContain( + `Exec denied (gateway id=${approvalId}, user-denied): echo ok`, + ); + expect(sendMessage).not.toHaveBeenCalled(); }); it("requires a separate approval for each elevated command after allow-once", async () => {