diff --git a/CHANGELOG.md b/CHANGELOG.md index 316d67ec62f..7828bc333e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Agents: keep unresolved mutating tool failures visible until the same action retry succeeds, scope mutation-error surfacing to mutating calls (including `session_status` model changes), and dedupe duplicate failure warnings in outbound replies. (#16131) Thanks @Swader. - Agents: classify external timeout aborts during compaction the same as internal timeouts, preventing unnecessary auth-profile rotation and preserving compaction-timeout snapshot fallback behavior. (#9855) Thanks @mverrilli. - Sessions/Agents: harden transcript path resolution for mismatched agent context by preserving explicit store roots and adding safe absolute-path fallback to the correct agent sessions directory. (#16288) Thanks @robbyczgw-cla. - BlueBubbles: include sender identity in group chat envelopes and pass clean message text to the agent prompt, aligning with iMessage/Signal formatting. (#16210) Thanks @zerone0x. diff --git a/src/agents/pi-embedded-runner/run/payloads.e2e.test.ts b/src/agents/pi-embedded-runner/run/payloads.e2e.test.ts index ae7a73513e4..f9fd438e056 100644 --- a/src/agents/pi-embedded-runner/run/payloads.e2e.test.ts +++ b/src/agents/pi-embedded-runner/run/payloads.e2e.test.ts @@ -315,6 +315,66 @@ describe("buildEmbeddedRunPayloads", () => { expect(payloads[1]?.text).toContain("missing"); }); + it("does not treat session_status read failures as mutating when explicitly flagged", () => { + const payloads = buildEmbeddedRunPayloads({ + assistantTexts: ["Status loaded."], + toolMetas: [], + lastAssistant: { stopReason: "end_turn" } as AssistantMessage, + lastToolError: { + toolName: "session_status", + error: "model required", + mutatingAction: false, + }, + sessionKey: "session:telegram", + inlineToolResultsAllowed: false, + verboseLevel: "off", + reasoningLevel: "off", + toolResultFormat: "plain", + }); + + expect(payloads).toHaveLength(1); + expect(payloads[0]?.text).toBe("Status loaded."); + }); + + it("dedupes identical tool warning text already present in assistant output", () => { + const seed = buildEmbeddedRunPayloads({ + assistantTexts: [], + toolMetas: [], + lastAssistant: undefined, + lastToolError: { + toolName: "write", + error: "file missing", + mutatingAction: true, + }, + sessionKey: "session:telegram", + inlineToolResultsAllowed: false, + verboseLevel: "off", + reasoningLevel: "off", + toolResultFormat: "plain", + }); + const warningText = seed[0]?.text; + expect(warningText).toBeTruthy(); + + const payloads = buildEmbeddedRunPayloads({ + assistantTexts: [warningText ?? ""], + toolMetas: [], + lastAssistant: { stopReason: "end_turn" } as AssistantMessage, + lastToolError: { + toolName: "write", + error: "file missing", + mutatingAction: true, + }, + sessionKey: "session:telegram", + inlineToolResultsAllowed: false, + verboseLevel: "off", + reasoningLevel: "off", + toolResultFormat: "plain", + }); + + expect(payloads).toHaveLength(1); + expect(payloads[0]?.text).toBe(warningText); + }); + it("shows non-recoverable tool errors to the user", () => { const payloads = buildEmbeddedRunPayloads({ assistantTexts: [], diff --git a/src/agents/pi-embedded-runner/run/payloads.ts b/src/agents/pi-embedded-runner/run/payloads.ts index ac453feb06a..3dd4411ba31 100644 --- a/src/agents/pi-embedded-runner/run/payloads.ts +++ b/src/agents/pi-embedded-runner/run/payloads.ts @@ -54,7 +54,13 @@ export function buildEmbeddedRunPayloads(params: { assistantTexts: string[]; toolMetas: ToolMetaEntry[]; lastAssistant: AssistantMessage | undefined; - lastToolError?: { toolName: string; meta?: string; error?: string }; + lastToolError?: { + toolName: string; + meta?: string; + error?: string; + mutatingAction?: boolean; + actionFingerprint?: string; + }; config?: OpenClawConfig; sessionKey: string; provider?: string; @@ -252,7 +258,8 @@ export function buildEmbeddedRunPayloads(params: { errorLower.includes("must have") || errorLower.includes("needs") || errorLower.includes("requires"); - const isMutatingToolError = isLikelyMutatingTool(params.lastToolError.toolName); + const isMutatingToolError = + params.lastToolError.mutatingAction ?? isLikelyMutatingTool(params.lastToolError.toolName); const shouldShowToolError = isMutatingToolError || (!hasUserFacingReply && !isRecoverableError); // Always surface mutating tool failures so we do not silently confirm actions that did not happen. @@ -264,10 +271,23 @@ export function buildEmbeddedRunPayloads(params: { { markdown: useMarkdown }, ); const errorSuffix = params.lastToolError.error ? `: ${params.lastToolError.error}` : ""; - replyItems.push({ - text: `⚠️ ${toolSummary} failed${errorSuffix}`, - isError: true, - }); + const warningText = `⚠️ ${toolSummary} failed${errorSuffix}`; + const normalizedWarning = normalizeTextForComparison(warningText); + const duplicateWarning = normalizedWarning + ? replyItems.some((item) => { + if (!item.text) { + return false; + } + const normalizedExisting = normalizeTextForComparison(item.text); + return normalizedExisting.length > 0 && normalizedExisting === normalizedWarning; + }) + : false; + if (!duplicateWarning) { + replyItems.push({ + text: warningText, + isError: true, + }); + } } } diff --git a/src/agents/pi-embedded-runner/run/types.ts b/src/agents/pi-embedded-runner/run/types.ts index dc05d0a1286..2d22e0a953f 100644 --- a/src/agents/pi-embedded-runner/run/types.ts +++ b/src/agents/pi-embedded-runner/run/types.ts @@ -33,7 +33,13 @@ export type EmbeddedRunAttemptResult = { assistantTexts: string[]; toolMetas: Array<{ toolName: string; meta?: string }>; lastAssistant: AssistantMessage | undefined; - lastToolError?: { toolName: string; meta?: string; error?: string }; + lastToolError?: { + toolName: string; + meta?: string; + error?: string; + mutatingAction?: boolean; + actionFingerprint?: string; + }; didSendViaMessagingTool: boolean; messagingToolSentTexts: string[]; messagingToolSentTargets: MessagingToolSend[]; diff --git a/src/agents/pi-embedded-subscribe.handlers.tools.ts b/src/agents/pi-embedded-subscribe.handlers.tools.ts index caa7691eed6..4f22244181c 100644 --- a/src/agents/pi-embedded-subscribe.handlers.tools.ts +++ b/src/agents/pi-embedded-subscribe.handlers.tools.ts @@ -1,6 +1,9 @@ import type { AgentEvent } from "@mariozechner/pi-agent-core"; import type { PluginHookAfterToolCallEvent } from "../plugins/types.js"; -import type { EmbeddedPiSubscribeContext } from "./pi-embedded-subscribe.handlers.types.js"; +import type { + EmbeddedPiSubscribeContext, + ToolCallSummary, +} from "./pi-embedded-subscribe.handlers.types.js"; import { emitAgentEvent } from "../infra/agent-events.js"; import { getGlobalHookRunner } from "../plugins/hook-runner-global.js"; import { normalizeTextForComparison } from "./pi-embedded-helpers.js"; @@ -17,6 +20,161 @@ import { normalizeToolName } from "./tool-policy.js"; /** Track tool execution start times and args for after_tool_call hook */ const toolStartData = new Map(); + +const READ_ONLY_ACTIONS = new Set([ + "get", + "list", + "read", + "status", + "show", + "fetch", + "search", + "query", + "view", + "poll", + "log", + "inspect", + "check", + "probe", +]); +const PROCESS_MUTATING_ACTIONS = new Set(["write", "send_keys", "submit", "paste", "kill"]); +const MESSAGE_MUTATING_ACTIONS = new Set([ + "send", + "reply", + "thread_reply", + "threadreply", + "edit", + "delete", + "react", + "pin", + "unpin", +]); + +function asRecord(value: unknown): Record | undefined { + return value && typeof value === "object" ? (value as Record) : undefined; +} + +function normalizeActionName(value: unknown): string | undefined { + if (typeof value !== "string") { + return undefined; + } + const normalized = value + .trim() + .toLowerCase() + .replace(/[\s-]+/g, "_"); + return normalized || undefined; +} + +function normalizeFingerprintValue(value: unknown): string | undefined { + if (typeof value !== "string") { + return undefined; + } + const normalized = value.trim(); + return normalized ? normalized.toLowerCase() : undefined; +} + +function isMutatingToolCall(toolName: string, args: unknown): boolean { + const normalized = toolName.trim().toLowerCase(); + const record = asRecord(args); + const action = normalizeActionName(record?.action); + + switch (normalized) { + case "write": + case "edit": + case "apply_patch": + case "exec": + case "bash": + case "sessions_send": + return true; + case "process": + return action != null && PROCESS_MUTATING_ACTIONS.has(action); + case "message": + return ( + (action != null && MESSAGE_MUTATING_ACTIONS.has(action)) || + typeof record?.content === "string" || + typeof record?.message === "string" + ); + case "session_status": + return typeof record?.model === "string" && record.model.trim().length > 0; + default: { + if (normalized === "cron" || normalized === "gateway" || normalized === "canvas") { + return action == null || !READ_ONLY_ACTIONS.has(action); + } + if (normalized === "nodes") { + return action == null || action !== "list"; + } + if (normalized.endsWith("_actions")) { + return action == null || !READ_ONLY_ACTIONS.has(action); + } + if (normalized.startsWith("message_") || normalized.includes("send")) { + return true; + } + return false; + } + } +} + +function buildActionFingerprint( + toolName: string, + args: unknown, + meta?: string, +): string | undefined { + if (!isMutatingToolCall(toolName, args)) { + return undefined; + } + const normalizedTool = toolName.trim().toLowerCase(); + const record = asRecord(args); + const action = normalizeActionName(record?.action); + const parts = [`tool=${normalizedTool}`]; + if (action) { + parts.push(`action=${action}`); + } + for (const key of [ + "path", + "filePath", + "oldPath", + "newPath", + "to", + "target", + "messageId", + "sessionKey", + "jobId", + "id", + "model", + ]) { + const value = normalizeFingerprintValue(record?.[key]); + if (value) { + parts.push(`${key.toLowerCase()}=${value}`); + } + } + const normalizedMeta = meta?.trim().replace(/\s+/g, " ").toLowerCase(); + if (normalizedMeta) { + parts.push(`meta=${normalizedMeta}`); + } + return parts.join("|"); +} + +function buildToolCallSummary(toolName: string, args: unknown, meta?: string): ToolCallSummary { + const actionFingerprint = buildActionFingerprint(toolName, args, meta); + return { + meta, + mutatingAction: actionFingerprint != null, + actionFingerprint, + }; +} + +function sameToolAction( + existing: { toolName: string; meta?: string; actionFingerprint?: string }, + toolName: string, + meta?: string, + actionFingerprint?: string, +): boolean { + if (existing.actionFingerprint && actionFingerprint) { + return existing.actionFingerprint === actionFingerprint; + } + return existing.toolName === toolName && (existing.meta ?? "") === (meta ?? ""); +} + function extendExecMeta(toolName: string, args: unknown, meta?: string): string | undefined { const normalized = toolName.trim().toLowerCase(); if (normalized !== "exec" && normalized !== "bash") { @@ -70,7 +228,7 @@ export async function handleToolExecutionStart( } const meta = extendExecMeta(toolName, args, inferToolMetaFromArgs(toolName, args)); - ctx.state.toolMetaById.set(toolCallId, meta); + ctx.state.toolMetaById.set(toolCallId, buildToolCallSummary(toolName, args, meta)); ctx.log.debug( `embedded run tool start: runId=${ctx.params.runId} tool=${toolName} toolCallId=${toolCallId}`, ); @@ -167,7 +325,8 @@ export async function handleToolExecutionEnd( const result = evt.result; const isToolError = isError || isToolResultError(result); const sanitizedResult = sanitizeToolResult(result); - const meta = ctx.state.toolMetaById.get(toolCallId); + const callSummary = ctx.state.toolMetaById.get(toolCallId); + const meta = callSummary?.meta; ctx.state.toolMetas.push({ toolName, meta }); ctx.state.toolMetaById.delete(toolCallId); ctx.state.toolSummaryById.delete(toolCallId); @@ -177,10 +336,18 @@ export async function handleToolExecutionEnd( toolName, meta, error: errorMessage, + mutatingAction: callSummary?.mutatingAction, + actionFingerprint: callSummary?.actionFingerprint, }; - } else { - // Keep the latest tool outcome authoritative; successful retries should clear prior failures. - ctx.state.lastToolError = undefined; + } else if (ctx.state.lastToolError) { + // Keep unresolved mutating failures until the same action succeeds. + if (ctx.state.lastToolError.mutatingAction) { + if (sameToolAction(ctx.state.lastToolError, toolName, meta, callSummary?.actionFingerprint)) { + ctx.state.lastToolError = undefined; + } + } else { + ctx.state.lastToolError = undefined; + } } // Commit messaging tool text on success, discard on error. diff --git a/src/agents/pi-embedded-subscribe.handlers.types.ts b/src/agents/pi-embedded-subscribe.handlers.types.ts index 4dd091ab55a..aa70dc4e912 100644 --- a/src/agents/pi-embedded-subscribe.handlers.types.ts +++ b/src/agents/pi-embedded-subscribe.handlers.types.ts @@ -20,12 +20,20 @@ export type ToolErrorSummary = { toolName: string; meta?: string; error?: string; + mutatingAction?: boolean; + actionFingerprint?: string; +}; + +export type ToolCallSummary = { + meta?: string; + mutatingAction: boolean; + actionFingerprint?: string; }; export type EmbeddedPiSubscribeState = { assistantTexts: string[]; toolMetas: Array<{ toolName?: string; meta?: string }>; - toolMetaById: Map; + toolMetaById: Map; toolSummaryById: Set; lastToolError?: ToolErrorSummary; diff --git a/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.subscribeembeddedpisession.e2e.test.ts b/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.subscribeembeddedpisession.e2e.test.ts index 42c0158af43..cc18dcfcd67 100644 --- a/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.subscribeembeddedpisession.e2e.test.ts +++ b/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.subscribeembeddedpisession.e2e.test.ts @@ -318,6 +318,100 @@ describe("subscribeEmbeddedPiSession", () => { expect(payloads[0]?.mediaUrls).toEqual(["https://example.com/a.png"]); }); + it("keeps unresolved mutating failure when an unrelated tool succeeds", () => { + let handler: ((evt: unknown) => void) | undefined; + const session: StubSession = { + subscribe: (fn) => { + handler = fn; + return () => {}; + }, + }; + + const subscription = subscribeEmbeddedPiSession({ + session: session as unknown as Parameters[0]["session"], + runId: "run-tools-1", + sessionKey: "test-session", + }); + + handler?.({ + type: "tool_execution_start", + toolName: "write", + toolCallId: "w1", + args: { path: "/tmp/demo.txt", content: "next" }, + }); + handler?.({ + type: "tool_execution_end", + toolName: "write", + toolCallId: "w1", + isError: true, + result: { error: "disk full" }, + }); + expect(subscription.getLastToolError()?.toolName).toBe("write"); + + handler?.({ + type: "tool_execution_start", + toolName: "read", + toolCallId: "r1", + args: { path: "/tmp/demo.txt" }, + }); + handler?.({ + type: "tool_execution_end", + toolName: "read", + toolCallId: "r1", + isError: false, + result: { text: "ok" }, + }); + + expect(subscription.getLastToolError()?.toolName).toBe("write"); + }); + + it("clears unresolved mutating failure when the same action succeeds", () => { + let handler: ((evt: unknown) => void) | undefined; + const session: StubSession = { + subscribe: (fn) => { + handler = fn; + return () => {}; + }, + }; + + const subscription = subscribeEmbeddedPiSession({ + session: session as unknown as Parameters[0]["session"], + runId: "run-tools-2", + sessionKey: "test-session", + }); + + handler?.({ + type: "tool_execution_start", + toolName: "write", + toolCallId: "w1", + args: { path: "/tmp/demo.txt", content: "next" }, + }); + handler?.({ + type: "tool_execution_end", + toolName: "write", + toolCallId: "w1", + isError: true, + result: { error: "disk full" }, + }); + expect(subscription.getLastToolError()?.toolName).toBe("write"); + + handler?.({ + type: "tool_execution_start", + toolName: "write", + toolCallId: "w2", + args: { path: "/tmp/demo.txt", content: "retry" }, + }); + handler?.({ + type: "tool_execution_end", + toolName: "write", + toolCallId: "w2", + isError: false, + result: { ok: true }, + }); + + expect(subscription.getLastToolError()).toBeUndefined(); + }); + it("emits lifecycle:error event on agent_end when last assistant message was an error", async () => { let handler: ((evt: unknown) => void) | undefined; const session: StubSession = {