mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-20 06:20:55 +00:00
fix: scope mutating tool error clearing
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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: [],
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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[];
|
||||
|
||||
@@ -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<string, { startTime: number; args: unknown }>();
|
||||
|
||||
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<string, unknown> | undefined {
|
||||
return value && typeof value === "object" ? (value as Record<string, unknown>) : 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.
|
||||
|
||||
@@ -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<string, string | undefined>;
|
||||
toolMetaById: Map<string, ToolCallSummary>;
|
||||
toolSummaryById: Set<string>;
|
||||
lastToolError?: ToolErrorSummary;
|
||||
|
||||
|
||||
@@ -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<typeof subscribeEmbeddedPiSession>[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<typeof subscribeEmbeddedPiSession>[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 = {
|
||||
|
||||
Reference in New Issue
Block a user