From 4207ca2eb8816e4bab47afe2a55f18f7661e7771 Mon Sep 17 00:00:00 2001 From: seonang Date: Thu, 2 Apr 2026 02:20:28 +0000 Subject: [PATCH] Fix Telegram exec approval delivery and auto-resume fallback --- CHANGELOG.md | 1 + .../bash-tools.exec-approval-followup.test.ts | 44 ++++++++ .../bash-tools.exec-approval-followup.ts | 106 ++++++++++++------ .../bash-tools.exec-host-gateway.test.ts | 31 ++++- src/agents/bash-tools.exec-host-gateway.ts | 4 +- src/agents/bash-tools.exec-host-node.ts | 2 +- 6 files changed, 149 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f7e373d10c8..a9b41694173 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,7 @@ Docs: https://docs.openclaw.ai - Plugins/install: accept JSON5 syntax in `openclaw.plugin.json` and bundle `plugin.json` manifests during install/validation, so third-party plugins with trailing commas, comments, or unquoted keys no longer fail to install. (#59084) Thanks @singleGanghood. - Telegram/exec approvals: rewrite shared `/approve … allow-always` callback payloads to `/approve … always` before Telegram button rendering so plugin approval IDs still fit Telegram's `callback_data` limit and keep the Allow Always action visible. (#59217) Thanks @jameslcowan. - Cron/exec timeouts: surface timed-out `exec` and `bash` failures in isolated cron runs even when `verbose: off`, including custom session-target cron jobs, so scheduled runs stop failing silently. (#58247) Thanks @skainguyen1412. +- Telegram/exec approvals: fall back to the origin session key for async approval followups and keep resume-failure status delivery sanitized so Telegram followups still land without leaking raw exec metadata. (#59351) Thanks @seonang. ## 2026.4.2 diff --git a/src/agents/bash-tools.exec-approval-followup.test.ts b/src/agents/bash-tools.exec-approval-followup.test.ts index 496a0a0ad19..916bf4c0416 100644 --- a/src/agents/bash-tools.exec-approval-followup.test.ts +++ b/src/agents/bash-tools.exec-approval-followup.test.ts @@ -138,6 +138,28 @@ describe("exec approval followup", () => { expect(callGatewayTool).not.toHaveBeenCalled(); }); + it("falls back to sanitized direct delivery when session resume fails", async () => { + vi.mocked(callGatewayTool).mockRejectedValueOnce(new Error("session missing")); + + await sendExecApprovalFollowup({ + approvalId: "req-session-resume-failed", + sessionKey: "agent:main:discord:channel:123", + turnSourceChannel: "discord", + turnSourceTo: "123", + turnSourceAccountId: "default", + turnSourceThreadId: "456", + resultText: + "Exec finished (gateway id=req-session-resume-failed, session=sess_1, code 0)\nall good", + }); + + expect(sendMessage).toHaveBeenCalledWith( + expect.objectContaining({ + content: "Automatic session resume failed, so sending the status directly.\n\nall good", + idempotencyKey: "exec-approval-followup:req-session-resume-failed", + }), + ); + }); + it("uses a generic summary when a no-session completion has no user-visible output", async () => { await sendExecApprovalFollowup({ approvalId: "req-no-session-empty", @@ -156,6 +178,28 @@ describe("exec approval followup", () => { ); }); + it("uses safe 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", + turnSourceChannel: "telegram", + turnSourceTo: "-100123", + turnSourceAccountId: "default", + turnSourceThreadId: "789", + resultText: "Exec denied (gateway id=req-denied-resume-failed, approval-timeout): uname -a", + }); + + expect(sendMessage).toHaveBeenCalledWith( + expect.objectContaining({ + content: + "Automatic session resume failed, so sending the status directly.\n\nCommand did not run: approval timed out.", + idempotencyKey: "exec-approval-followup:req-denied-resume-failed", + }), + ); + }); + it("suppresses denied followups for subagent sessions", async () => { await expect( sendExecApprovalFollowup({ diff --git a/src/agents/bash-tools.exec-approval-followup.ts b/src/agents/bash-tools.exec-approval-followup.ts index 557206e3748..49e437da86f 100644 --- a/src/agents/bash-tools.exec-approval-followup.ts +++ b/src/agents/bash-tools.exec-approval-followup.ts @@ -2,7 +2,11 @@ import { resolveExternalBestEffortDeliveryTarget } from "../infra/outbound/best- import { sendMessage } from "../infra/outbound/message.js"; import { isCronSessionKey, isSubagentSessionKey } from "../sessions/session-key-utils.js"; import { isGatewayMessageChannel, normalizeMessageChannel } from "../utils/message-channel.js"; -import { isExecDeniedResultText, parseExecApprovalResultText } from "./exec-approval-result.js"; +import { + formatExecDeniedUserMessage, + isExecDeniedResultText, + parseExecApprovalResultText, +} from "./exec-approval-result.js"; import { sanitizeUserFacingText } from "./pi-embedded-helpers/errors.js"; import { callGatewayTool } from "./tools/gateway.js"; @@ -32,6 +36,20 @@ function buildExecDeniedFollowupPrompt(resultText: string): string { ].join("\n"); } +function formatUnknownError(error: unknown): string { + if (error instanceof Error) { + return error.message; + } + if (typeof error === "string") { + return error; + } + try { + return JSON.stringify(error); + } catch { + return "unknown error"; + } +} + export function buildExecApprovalFollowupPrompt(resultText: string): string { const trimmed = resultText.trim(); if (isExecDeniedResultText(trimmed)) { @@ -56,13 +74,16 @@ function shouldSuppressExecDeniedFollowup(sessionKey: string | undefined): boole return isSubagentSessionKey(sessionKey) || isCronSessionKey(sessionKey); } -function formatDirectExecApprovalFollowupText(resultText: string): string | null { +function formatDirectExecApprovalFollowupText( + resultText: string, + opts: { allowDenied?: boolean } = {}, +): string | null { const parsed = parseExecApprovalResultText(resultText); if (parsed.kind === "other" && !parsed.raw) { return null; } if (parsed.kind === "denied") { - return null; + return opts.allowDenied ? formatExecDeniedUserMessage(parsed.raw) : null; } if (parsed.kind === "finished") { @@ -91,6 +112,10 @@ function formatDirectExecApprovalFollowupText(resultText: string): string | null return sanitizeUserFacingText(parsed.raw, { errorContext: true }).trim() || null; } +function buildSessionResumeFallbackPrefix(): string { + return "Automatic session resume failed, so sending the status directly.\n\n"; +} + export async function sendExecApprovalFollowup( params: ExecApprovalFollowupParams, ): Promise { @@ -116,55 +141,66 @@ export async function sendExecApprovalFollowup( ? normalizedTurnSourceChannel : undefined; + let sessionError: unknown = null; + if (sessionKey) { - await callGatewayTool( - "agent", - { timeoutMs: 60_000 }, - { - sessionKey, - message: buildExecApprovalFollowupPrompt(resultText), - deliver: deliveryTarget.deliver, - ...(deliveryTarget.deliver ? { bestEffortDeliver: true as const } : {}), - channel: deliveryTarget.deliver ? deliveryTarget.channel : sessionOnlyOriginChannel, - to: deliveryTarget.deliver - ? deliveryTarget.to - : sessionOnlyOriginChannel - ? params.turnSourceTo - : undefined, - accountId: deliveryTarget.deliver - ? deliveryTarget.accountId - : sessionOnlyOriginChannel - ? params.turnSourceAccountId - : undefined, - threadId: deliveryTarget.deliver - ? deliveryTarget.threadId - : sessionOnlyOriginChannel - ? params.turnSourceThreadId - : undefined, - idempotencyKey: `exec-approval-followup:${params.approvalId}`, - }, - { expectFinal: true }, - ); - return true; + try { + await callGatewayTool( + "agent", + { timeoutMs: 60_000 }, + { + sessionKey, + message: buildExecApprovalFollowupPrompt(resultText), + deliver: deliveryTarget.deliver, + ...(deliveryTarget.deliver ? { bestEffortDeliver: true as const } : {}), + channel: deliveryTarget.deliver ? deliveryTarget.channel : sessionOnlyOriginChannel, + to: deliveryTarget.deliver + ? deliveryTarget.to + : sessionOnlyOriginChannel + ? params.turnSourceTo + : undefined, + accountId: deliveryTarget.deliver + ? deliveryTarget.accountId + : sessionOnlyOriginChannel + ? params.turnSourceAccountId + : undefined, + threadId: deliveryTarget.deliver + ? deliveryTarget.threadId + : sessionOnlyOriginChannel + ? params.turnSourceThreadId + : undefined, + idempotencyKey: `exec-approval-followup:${params.approvalId}`, + }, + { expectFinal: true }, + ); + return true; + } catch (err) { + sessionError = err; + } } - const directText = formatDirectExecApprovalFollowupText(resultText); + const directText = formatDirectExecApprovalFollowupText(resultText, { + allowDenied: sessionError !== null, + }); if (deliveryTarget.deliver && directText) { + const prefix = sessionError ? buildSessionResumeFallbackPrefix() : ""; await sendMessage({ channel: deliveryTarget.channel, to: deliveryTarget.to ?? "", accountId: deliveryTarget.accountId, threadId: deliveryTarget.threadId, - content: directText, + content: `${prefix}${directText}`, agentId: undefined, idempotencyKey: `exec-approval-followup:${params.approvalId}`, }); return true; } + if (sessionError) { + throw new Error(`Session followup failed: ${formatUnknownError(sessionError)}`); + } if (isDenied) { return false; } - throw new Error("Session key or deliverable origin route is required"); } diff --git a/src/agents/bash-tools.exec-host-gateway.test.ts b/src/agents/bash-tools.exec-host-gateway.test.ts index 96d1ed7fc88..7834d3fe7ad 100644 --- a/src/agents/bash-tools.exec-host-gateway.test.ts +++ b/src/agents/bash-tools.exec-host-gateway.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const createAndRegisterDefaultExecApprovalRequestMock = vi.hoisted(() => vi.fn()); const buildExecApprovalPendingToolResultMock = vi.hoisted(() => vi.fn()); +const buildExecApprovalFollowupTargetMock = vi.hoisted(() => vi.fn(() => null)); vi.mock("../infra/exec-approvals.js", () => ({ evaluateShellAllowlist: vi.fn(() => ({ @@ -20,6 +21,7 @@ vi.mock("../infra/exec-approvals.js", () => ({ recordAllowlistUse: vi.fn(), resolveApprovalAuditCandidatePath: vi.fn(() => null), resolveAllowAlwaysPatterns: vi.fn(() => []), + resolveExecApprovalAllowedDecisions: vi.fn(() => ["allow-once", "allow-always", "deny"]), addAllowlistEntry: vi.fn(), addDurableCommandApproval: vi.fn(), })); @@ -39,7 +41,7 @@ vi.mock("./bash-tools.exec-host-shared.js", () => ({ })), buildDefaultExecApprovalRequestArgs: vi.fn(() => ({})), buildHeadlessExecApprovalDeniedMessage: vi.fn(() => "denied"), - buildExecApprovalFollowupTarget: vi.fn(() => null), + buildExecApprovalFollowupTarget: buildExecApprovalFollowupTargetMock, buildExecApprovalPendingToolResult: buildExecApprovalPendingToolResultMock, createExecApprovalDecisionState: vi.fn(() => ({ baseDecision: { timedOut: false }, @@ -83,6 +85,8 @@ describe("processGatewayAllowlist", () => { beforeEach(async () => { vi.resetModules(); buildExecApprovalPendingToolResultMock.mockReset(); + buildExecApprovalFollowupTargetMock.mockReset(); + buildExecApprovalFollowupTargetMock.mockReturnValue(null); buildExecApprovalPendingToolResultMock.mockReturnValue({ details: { status: "approval-pending" }, content: [], @@ -121,4 +125,29 @@ describe("processGatewayAllowlist", () => { expect(createAndRegisterDefaultExecApprovalRequestMock).toHaveBeenCalledTimes(1); expect(result.pendingResult?.details.status).toBe("approval-pending"); }); + + it("uses sessionKey for followups when notifySessionKey is absent", async () => { + await processGatewayAllowlist({ + command: "echo ok", + workdir: process.cwd(), + env: process.env as Record, + pty: false, + defaultTimeoutSec: 30, + security: "allowlist", + ask: "off", + safeBins: new Set(), + safeBinProfiles: {}, + warnings: [], + approvalRunningNoticeMs: 0, + maxOutput: 1000, + pendingMaxOutput: 1000, + sessionKey: "agent:main:telegram:direct:123", + }); + + expect(buildExecApprovalFollowupTargetMock).toHaveBeenCalledWith( + expect.objectContaining({ + sessionKey: "agent:main:telegram:direct:123", + }), + ); + }); }); diff --git a/src/agents/bash-tools.exec-host-gateway.ts b/src/agents/bash-tools.exec-host-gateway.ts index c4b0122b526..3a9897cccd9 100644 --- a/src/agents/bash-tools.exec-host-gateway.ts +++ b/src/agents/bash-tools.exec-host-gateway.ts @@ -270,7 +270,7 @@ export async function processGatewayAllowlist( typeof params.timeoutSec === "number" ? params.timeoutSec : params.defaultTimeoutSec; const followupTarget = buildExecApprovalFollowupTarget({ approvalId, - sessionKey: params.notifySessionKey, + sessionKey: params.notifySessionKey ?? params.sessionKey, turnSourceChannel: params.turnSourceChannel, turnSourceTo: params.turnSourceTo, turnSourceAccountId: params.turnSourceAccountId, @@ -364,7 +364,7 @@ export async function processGatewayAllowlist( notifyOnExit: false, notifyOnExitEmptySuccess: false, scopeKey: params.scopeKey, - sessionKey: params.notifySessionKey, + sessionKey: params.notifySessionKey ?? params.sessionKey, timeoutSec: effectiveTimeout, }); } catch { diff --git a/src/agents/bash-tools.exec-host-node.ts b/src/agents/bash-tools.exec-host-node.ts index 418d28ec84e..e608820e011 100644 --- a/src/agents/bash-tools.exec-host-node.ts +++ b/src/agents/bash-tools.exec-host-node.ts @@ -312,7 +312,7 @@ export async function executeNodeHostCommand( } else { const followupTarget = execHostShared.buildExecApprovalFollowupTarget({ approvalId, - sessionKey: params.notifySessionKey, + sessionKey: params.notifySessionKey ?? params.sessionKey, turnSourceChannel: params.turnSourceChannel, turnSourceTo: params.turnSourceTo, turnSourceAccountId: params.turnSourceAccountId,