From ad13597df4e9c664199ef0992b47bf7e477b278e Mon Sep 17 00:00:00 2001 From: Bruno Skvorc Date: Sat, 14 Feb 2026 10:27:11 +0100 Subject: [PATCH] Agents: preserve bootstrap paths and expose failed mutations --- src/agents/bootstrap-files.e2e.test.ts | 4 +- ...ers.buildbootstrapcontextfiles.e2e.test.ts | 2 +- src/agents/pi-embedded-helpers/bootstrap.ts | 4 +- .../run/payloads.e2e.test.ts | 85 +++++++++++++------ src/agents/pi-embedded-runner/run/payloads.ts | 39 +++++++-- .../pi-embedded-subscribe.handlers.tools.ts | 3 + src/agents/system-prompt-report.test.ts | 47 ++++++++++ src/agents/system-prompt-report.ts | 16 +++- 8 files changed, 165 insertions(+), 35 deletions(-) create mode 100644 src/agents/system-prompt-report.test.ts diff --git a/src/agents/bootstrap-files.e2e.test.ts b/src/agents/bootstrap-files.e2e.test.ts index 4cf0941e6a2..eee80fadc1f 100644 --- a/src/agents/bootstrap-files.e2e.test.ts +++ b/src/agents/bootstrap-files.e2e.test.ts @@ -53,7 +53,9 @@ describe("resolveBootstrapContextForRun", () => { const workspaceDir = await makeTempWorkspace("openclaw-bootstrap-"); const result = await resolveBootstrapContextForRun({ workspaceDir }); - const extra = result.contextFiles.find((file) => file.path === "EXTRA.md"); + const extra = result.contextFiles.find( + (file) => file.path === path.join(workspaceDir, "EXTRA.md"), + ); expect(extra?.content).toBe("extra"); }); diff --git a/src/agents/pi-embedded-helpers.buildbootstrapcontextfiles.e2e.test.ts b/src/agents/pi-embedded-helpers.buildbootstrapcontextfiles.e2e.test.ts index 4139bf31984..a646098c250 100644 --- a/src/agents/pi-embedded-helpers.buildbootstrapcontextfiles.e2e.test.ts +++ b/src/agents/pi-embedded-helpers.buildbootstrapcontextfiles.e2e.test.ts @@ -14,7 +14,7 @@ describe("buildBootstrapContextFiles", () => { const files = [makeFile({ missing: true, content: undefined })]; expect(buildBootstrapContextFiles(files)).toEqual([ { - path: DEFAULT_AGENTS_FILENAME, + path: "/tmp/AGENTS.md", content: "[MISSING] Expected at: /tmp/AGENTS.md", }, ]); diff --git a/src/agents/pi-embedded-helpers/bootstrap.ts b/src/agents/pi-embedded-helpers/bootstrap.ts index 725324be9fb..7c141a09b7f 100644 --- a/src/agents/pi-embedded-helpers/bootstrap.ts +++ b/src/agents/pi-embedded-helpers/bootstrap.ts @@ -168,7 +168,7 @@ export function buildBootstrapContextFiles( for (const file of files) { if (file.missing) { result.push({ - path: file.name, + path: file.path, content: `[MISSING] Expected at: ${file.path}`, }); continue; @@ -183,7 +183,7 @@ export function buildBootstrapContextFiles( ); } result.push({ - path: file.name, + path: file.path, content: trimmed.content, }); } 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 bac074e0181..ae7a73513e4 100644 --- a/src/agents/pi-embedded-runner/run/payloads.e2e.test.ts +++ b/src/agents/pi-embedded-runner/run/payloads.e2e.test.ts @@ -229,7 +229,56 @@ describe("buildEmbeddedRunPayloads", () => { expect(payloads[0]?.text).toContain("code 1"); }); - it("suppresses recoverable tool errors containing 'required'", () => { + it("suppresses recoverable tool errors containing 'required' for non-mutating tools", () => { + const payloads = buildEmbeddedRunPayloads({ + assistantTexts: [], + toolMetas: [], + lastAssistant: undefined, + lastToolError: { toolName: "browser", error: "url required" }, + sessionKey: "session:telegram", + inlineToolResultsAllowed: false, + verboseLevel: "off", + reasoningLevel: "off", + toolResultFormat: "plain", + }); + + // Recoverable errors should not be sent to the user + expect(payloads).toHaveLength(0); + }); + + it("suppresses recoverable tool errors containing 'missing' for non-mutating tools", () => { + const payloads = buildEmbeddedRunPayloads({ + assistantTexts: [], + toolMetas: [], + lastAssistant: undefined, + lastToolError: { toolName: "browser", error: "url missing" }, + sessionKey: "session:telegram", + inlineToolResultsAllowed: false, + verboseLevel: "off", + reasoningLevel: "off", + toolResultFormat: "plain", + }); + + expect(payloads).toHaveLength(0); + }); + + it("suppresses recoverable tool errors containing 'invalid' for non-mutating tools", () => { + const payloads = buildEmbeddedRunPayloads({ + assistantTexts: [], + toolMetas: [], + lastAssistant: undefined, + lastToolError: { toolName: "browser", error: "invalid parameter: url" }, + sessionKey: "session:telegram", + inlineToolResultsAllowed: false, + verboseLevel: "off", + reasoningLevel: "off", + toolResultFormat: "plain", + }); + + expect(payloads).toHaveLength(0); + }); + + it("shows recoverable tool errors for mutating tools", () => { const payloads = buildEmbeddedRunPayloads({ assistantTexts: [], toolMetas: [], @@ -242,16 +291,17 @@ describe("buildEmbeddedRunPayloads", () => { toolResultFormat: "plain", }); - // Recoverable errors should not be sent to the user - expect(payloads).toHaveLength(0); + expect(payloads).toHaveLength(1); + expect(payloads[0]?.isError).toBe(true); + expect(payloads[0]?.text).toContain("required"); }); - it("suppresses recoverable tool errors containing 'missing'", () => { + it("shows mutating tool errors even when assistant output exists", () => { const payloads = buildEmbeddedRunPayloads({ - assistantTexts: [], + assistantTexts: ["Done."], toolMetas: [], - lastAssistant: undefined, - lastToolError: { toolName: "message", error: "messageId missing" }, + lastAssistant: { stopReason: "end_turn" } as AssistantMessage, + lastToolError: { toolName: "write", error: "file missing" }, sessionKey: "session:telegram", inlineToolResultsAllowed: false, verboseLevel: "off", @@ -259,23 +309,10 @@ describe("buildEmbeddedRunPayloads", () => { toolResultFormat: "plain", }); - expect(payloads).toHaveLength(0); - }); - - it("suppresses recoverable tool errors containing 'invalid'", () => { - const payloads = buildEmbeddedRunPayloads({ - assistantTexts: [], - toolMetas: [], - lastAssistant: undefined, - lastToolError: { toolName: "message", error: "invalid parameter: to" }, - sessionKey: "session:telegram", - inlineToolResultsAllowed: false, - verboseLevel: "off", - reasoningLevel: "off", - toolResultFormat: "plain", - }); - - expect(payloads).toHaveLength(0); + expect(payloads).toHaveLength(2); + expect(payloads[0]?.text).toBe("Done."); + expect(payloads[1]?.isError).toBe(true); + expect(payloads[1]?.text).toContain("missing"); }); it("shows non-recoverable tool errors to the user", () => { diff --git a/src/agents/pi-embedded-runner/run/payloads.ts b/src/agents/pi-embedded-runner/run/payloads.ts index 440f7eaed48..ac453feb06a 100644 --- a/src/agents/pi-embedded-runner/run/payloads.ts +++ b/src/agents/pi-embedded-runner/run/payloads.ts @@ -21,6 +21,35 @@ import { type ToolMetaEntry = { toolName: string; meta?: string }; +const MUTATING_TOOL_NAMES = new Set([ + "write", + "edit", + "apply_patch", + "exec", + "bash", + "process", + "message", + "sessions_send", + "cron", + "gateway", + "canvas", + "nodes", + "session_status", +]); + +function isLikelyMutatingTool(toolName: string): boolean { + const normalized = toolName.trim().toLowerCase(); + if (!normalized) { + return false; + } + return ( + MUTATING_TOOL_NAMES.has(normalized) || + normalized.endsWith("_actions") || + normalized.startsWith("message_") || + normalized.includes("send") + ); +} + export function buildEmbeddedRunPayloads(params: { assistantTexts: string[]; toolMetas: ToolMetaEntry[]; @@ -223,12 +252,12 @@ export function buildEmbeddedRunPayloads(params: { errorLower.includes("must have") || errorLower.includes("needs") || errorLower.includes("requires"); + const isMutatingToolError = isLikelyMutatingTool(params.lastToolError.toolName); + const shouldShowToolError = isMutatingToolError || (!hasUserFacingReply && !isRecoverableError); - // Show tool errors only when: - // 1. There's no user-facing reply AND the error is not recoverable - // Recoverable errors (validation, missing params) are already in the model's context - // and shouldn't be surfaced to users since the model should retry. - if (!hasUserFacingReply && !isRecoverableError) { + // Always surface mutating tool failures so we do not silently confirm actions that did not happen. + // Otherwise, keep the previous behavior and only surface non-recoverable failures when no reply exists. + if (shouldShowToolError) { const toolSummary = formatToolAggregate( params.lastToolError.toolName, params.lastToolError.meta ? [params.lastToolError.meta] : undefined, diff --git a/src/agents/pi-embedded-subscribe.handlers.tools.ts b/src/agents/pi-embedded-subscribe.handlers.tools.ts index ba2151be8da..caa7691eed6 100644 --- a/src/agents/pi-embedded-subscribe.handlers.tools.ts +++ b/src/agents/pi-embedded-subscribe.handlers.tools.ts @@ -178,6 +178,9 @@ export async function handleToolExecutionEnd( meta, error: errorMessage, }; + } else { + // Keep the latest tool outcome authoritative; successful retries should clear prior failures. + ctx.state.lastToolError = undefined; } // Commit messaging tool text on success, discard on error. diff --git a/src/agents/system-prompt-report.test.ts b/src/agents/system-prompt-report.test.ts new file mode 100644 index 00000000000..c2737865b6e --- /dev/null +++ b/src/agents/system-prompt-report.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from "vitest"; +import type { WorkspaceBootstrapFile } from "./workspace.js"; +import { buildSystemPromptReport } from "./system-prompt-report.js"; + +function makeBootstrapFile(overrides: Partial): WorkspaceBootstrapFile { + return { + name: "AGENTS.md", + path: "/tmp/workspace/AGENTS.md", + content: "alpha", + missing: false, + ...overrides, + }; +} + +describe("buildSystemPromptReport", () => { + it("counts injected chars when injected file paths are absolute", () => { + const file = makeBootstrapFile({ path: "/tmp/workspace/policies/AGENTS.md" }); + const report = buildSystemPromptReport({ + source: "run", + generatedAt: 0, + bootstrapMaxChars: 20_000, + systemPrompt: "system", + bootstrapFiles: [file], + injectedFiles: [{ path: "/tmp/workspace/policies/AGENTS.md", content: "trimmed" }], + skillsPrompt: "", + tools: [], + }); + + expect(report.injectedWorkspaceFiles[0]?.injectedChars).toBe("trimmed".length); + }); + + it("keeps legacy basename matching for injected files", () => { + const file = makeBootstrapFile({ path: "/tmp/workspace/policies/AGENTS.md" }); + const report = buildSystemPromptReport({ + source: "run", + generatedAt: 0, + bootstrapMaxChars: 20_000, + systemPrompt: "system", + bootstrapFiles: [file], + injectedFiles: [{ path: "AGENTS.md", content: "trimmed" }], + skillsPrompt: "", + tools: [], + }); + + expect(report.injectedWorkspaceFiles[0]?.injectedChars).toBe("trimmed".length); + }); +}); diff --git a/src/agents/system-prompt-report.ts b/src/agents/system-prompt-report.ts index 4f4b43fb06f..5783202e101 100644 --- a/src/agents/system-prompt-report.ts +++ b/src/agents/system-prompt-report.ts @@ -1,4 +1,5 @@ import type { AgentTool } from "@mariozechner/pi-agent-core"; +import path from "node:path"; import type { SessionSystemPromptReport } from "../config/sessions/types.js"; import type { EmbeddedContextFile } from "./pi-embedded-helpers.js"; import type { WorkspaceBootstrapFile } from "./workspace.js"; @@ -40,10 +41,21 @@ function buildInjectedWorkspaceFiles(params: { injectedFiles: EmbeddedContextFile[]; bootstrapMaxChars: number; }): SessionSystemPromptReport["injectedWorkspaceFiles"] { - const injectedByName = new Map(params.injectedFiles.map((f) => [f.path, f.content])); + const injectedByPath = new Map(params.injectedFiles.map((f) => [f.path, f.content])); + const injectedByBaseName = new Map(); + for (const file of params.injectedFiles) { + const normalizedPath = file.path.replace(/\\/g, "/"); + const baseName = path.posix.basename(normalizedPath); + if (!injectedByBaseName.has(baseName)) { + injectedByBaseName.set(baseName, file.content); + } + } return params.bootstrapFiles.map((file) => { const rawChars = file.missing ? 0 : (file.content ?? "").trimEnd().length; - const injected = injectedByName.get(file.name); + const injected = + injectedByPath.get(file.path) ?? + injectedByPath.get(file.name) ?? + injectedByBaseName.get(file.name); const injectedChars = injected ? injected.length : 0; const truncated = !file.missing && rawChars > params.bootstrapMaxChars; return {