mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-24 00:11:31 +00:00
Agents: preserve bootstrap paths and expose failed mutations
This commit is contained in:
committed by
Gustavo Madeira Santana
parent
bc299ae17e
commit
ad13597df4
@@ -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");
|
||||
});
|
||||
|
||||
@@ -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",
|
||||
},
|
||||
]);
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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.
|
||||
|
||||
47
src/agents/system-prompt-report.test.ts
Normal file
47
src/agents/system-prompt-report.test.ts
Normal file
@@ -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>): 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);
|
||||
});
|
||||
});
|
||||
@@ -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<string, string>();
|
||||
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 {
|
||||
|
||||
Reference in New Issue
Block a user