mirror of https://github.com/openclaw/openclaw.git
Fix Telegram exec approval delivery and auto-resume fallback
This commit is contained in:
parent
b5161042b7
commit
4207ca2eb8
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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({
|
||||
|
|
|
|||
|
|
@ -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<boolean> {
|
||||
|
|
@ -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");
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<string, string>,
|
||||
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",
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Reference in New Issue