From ce1cd26fbcb04e2a9072efc89290065abc1bf20d Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 5 Apr 2026 20:36:40 +0100 Subject: [PATCH] refactor: align agent tool params with upstream pi --- docs/pi.md | 2 +- ...pi-tool-definition-adapter.logging.test.ts | 20 +- ...aliases-schemas-without-dropping-e.test.ts | 48 +--- ...aliases-schemas-without-dropping-f.test.ts | 105 ++++---- ...aliases-schemas-without-dropping-g.test.ts | 4 +- src/agents/pi-tools.host-edit.ts | 9 +- src/agents/pi-tools.params.test.ts | 58 ++--- src/agents/pi-tools.params.ts | 240 ++---------------- .../pi-tools.read.host-edit-recovery.test.ts | 19 +- src/agents/pi-tools.read.ts | 52 ++-- src/agents/pi-tools.ts | 10 +- 11 files changed, 162 insertions(+), 405 deletions(-) diff --git a/docs/pi.md b/docs/pi.md index 09cb9ac819b..72d6744c0fd 100644 --- a/docs/pi.md +++ b/docs/pi.md @@ -503,7 +503,7 @@ if (sandboxRoot) { - Refusal magic string scrubbing - Turn validation for consecutive roles -- Claude Code parameter compatibility +- Strict upstream Pi tool parameter validation ### Google/Gemini diff --git a/src/agents/pi-tool-definition-adapter.logging.test.ts b/src/agents/pi-tool-definition-adapter.logging.test.ts index 269a1a3cad0..8869772977a 100644 --- a/src/agents/pi-tool-definition-adapter.logging.test.ts +++ b/src/agents/pi-tool-definition-adapter.logging.test.ts @@ -13,8 +13,8 @@ vi.mock("../logger.js", () => ({ })); let toToolDefinitions: typeof import("./pi-tool-definition-adapter.js").toToolDefinitions; -let wrapToolParamNormalization: typeof import("./pi-tools.params.js").wrapToolParamNormalization; -let CLAUDE_PARAM_GROUPS: typeof import("./pi-tools.params.js").CLAUDE_PARAM_GROUPS; +let wrapToolParamValidation: typeof import("./pi-tools.params.js").wrapToolParamValidation; +let REQUIRED_PARAM_GROUPS: typeof import("./pi-tools.params.js").REQUIRED_PARAM_GROUPS; let logError: typeof import("../logger.js").logError; type ToolExecute = ReturnType< @@ -25,7 +25,7 @@ const extensionContext = {} as Parameters[4]; describe("pi tool definition adapter logging", () => { beforeAll(async () => { ({ toToolDefinitions } = await import("./pi-tool-definition-adapter.js")); - ({ wrapToolParamNormalization, CLAUDE_PARAM_GROUPS } = await import("./pi-tools.params.js")); + ({ wrapToolParamValidation, REQUIRED_PARAM_GROUPS } = await import("./pi-tools.params.js")); ({ logError } = await import("../logger.js")); }); @@ -41,8 +41,12 @@ describe("pi tool definition adapter logging", () => { description: "edits files", parameters: Type.Object({ path: Type.String(), - oldText: Type.String(), - newText: Type.String(), + edits: Type.Array( + Type.Object({ + oldText: Type.String(), + newText: Type.String(), + }), + ), }), execute: async () => ({ content: [{ type: "text" as const, text: "ok" }], @@ -50,7 +54,7 @@ describe("pi tool definition adapter logging", () => { }), } satisfies AgentTool; - const tool = wrapToolParamNormalization(baseTool, CLAUDE_PARAM_GROUPS.edit); + const tool = wrapToolParamValidation(baseTool, REQUIRED_PARAM_GROUPS.edit); const [def] = toToolDefinitions([tool]); if (!def) { throw new Error("missing tool definition"); @@ -60,7 +64,7 @@ describe("pi tool definition adapter logging", () => { expect(logError).toHaveBeenCalledWith( expect.stringContaining( - '[tools] edit failed: Missing required parameters: oldText alias, newText alias (received: path). Supply correct parameters before retrying. raw_params={"path":"notes.txt"}', + '[tools] edit failed: Missing required parameter: edits (received: path). Supply correct parameters before retrying. raw_params={"path":"notes.txt"}', ), ); }); @@ -86,7 +90,7 @@ describe("pi tool definition adapter logging", () => { execute, } satisfies AgentTool; - const tool = wrapToolParamNormalization(baseTool, CLAUDE_PARAM_GROUPS.edit); + const tool = wrapToolParamValidation(baseTool, REQUIRED_PARAM_GROUPS.edit); const [def] = toToolDefinitions([tool]); if (!def) { throw new Error("missing tool definition"); diff --git a/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-e.test.ts b/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-e.test.ts index acc12200838..bbb13569520 100644 --- a/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-e.test.ts +++ b/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-e.test.ts @@ -1,41 +1,12 @@ import type { AgentTool } from "@mariozechner/pi-agent-core"; import { Type } from "@sinclair/typebox"; import { describe, expect, it, vi } from "vitest"; -import { - patchToolSchemaForClaudeCompatibility, - wrapToolParamNormalization, -} from "./pi-tools.params.js"; +import { REQUIRED_PARAM_GROUPS, wrapToolParamValidation } from "./pi-tools.params.js"; import { cleanToolSchemaForGemini } from "./pi-tools.schema.js"; describe("createOpenClawCodingTools", () => { - describe("Claude/Gemini alias support", () => { - it("adds Claude-style aliases to schemas without dropping metadata", () => { - const base: AgentTool = { - name: "write", - label: "write", - description: "test", - parameters: Type.Object({ - path: Type.String({ description: "Path" }), - content: Type.String({ description: "Body" }), - }), - execute: vi.fn(), - }; - - const patched = patchToolSchemaForClaudeCompatibility(base); - const params = patched.parameters as { - additionalProperties?: unknown; - properties?: Record; - required?: string[]; - }; - const props = params.properties ?? {}; - - expect(props.file_path).toEqual(props.path); - expect(params.additionalProperties).toBe(false); - expect(params.required ?? []).not.toContain("path"); - expect(params.required ?? []).not.toContain("file_path"); - }); - - it("normalizes file_path to path and enforces required groups at runtime", async () => { + describe("Gemini cleanup and strict param validation", () => { + it("enforces canonical path/content at runtime", async () => { const execute = vi.fn(async (_id, args) => args); const tool: AgentTool = { name: "write", @@ -48,12 +19,9 @@ describe("createOpenClawCodingTools", () => { execute, }; - const wrapped = wrapToolParamNormalization(tool, [ - { keys: ["path", "file_path"], label: "path (path or file_path)" }, - { keys: ["content"], label: "content" }, - ]); + const wrapped = wrapToolParamValidation(tool, REQUIRED_PARAM_GROUPS.write); - await wrapped.execute("tool-1", { file_path: "foo.txt", content: "x" }); + await wrapped.execute("tool-1", { path: "foo.txt", content: "x" }); expect(execute).toHaveBeenCalledWith( "tool-1", { path: "foo.txt", content: "x" }, @@ -67,14 +35,14 @@ describe("createOpenClawCodingTools", () => { await expect(wrapped.execute("tool-2", { content: "x" })).rejects.toThrow( /Supply correct parameters before retrying\./, ); - await expect(wrapped.execute("tool-3", { file_path: " ", content: "x" })).rejects.toThrow( + await expect(wrapped.execute("tool-3", { path: " ", content: "x" })).rejects.toThrow( /Missing required parameter/, ); - await expect(wrapped.execute("tool-3", { file_path: " ", content: "x" })).rejects.toThrow( + await expect(wrapped.execute("tool-3", { path: " ", content: "x" })).rejects.toThrow( /Supply correct parameters before retrying\./, ); await expect(wrapped.execute("tool-4", {})).rejects.toThrow( - /Missing required parameters: path \(path or file_path\), content/, + /Missing required parameters: path, content/, ); await expect(wrapped.execute("tool-4", {})).rejects.toThrow( /Supply correct parameters before retrying\./, diff --git a/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-f.test.ts b/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-f.test.ts index 4caf5936bf7..1e7bc4b2d0e 100644 --- a/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-f.test.ts +++ b/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-f.test.ts @@ -8,26 +8,25 @@ import { createOpenClawCodingTools } from "./pi-tools.js"; import { expectReadWriteEditTools } from "./test-helpers/pi-tools-fs-helpers.js"; describe("createOpenClawCodingTools", () => { - it("accepts Claude Code parameter aliases for read/write/edit", async () => { - const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-alias-")); + it("accepts canonical parameters for read/write/edit", async () => { + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-canonical-")); try { const tools = createOpenClawCodingTools({ workspaceDir: tmpDir }); const { readTool, writeTool, editTool } = expectReadWriteEditTools(tools); - const filePath = "alias-test.txt"; - await writeTool?.execute("tool-alias-1", { - file_path: filePath, + const filePath = "canonical-test.txt"; + await writeTool?.execute("tool-canonical-1", { + path: filePath, content: "hello world", }); - await editTool?.execute("tool-alias-2", { - file_path: filePath, - old_string: "world", - new_string: "universe", + await editTool?.execute("tool-canonical-2", { + path: filePath, + edits: [{ oldText: "world", newText: "universe" }], }); - const result = await readTool?.execute("tool-alias-3", { - file_path: filePath, + const result = await readTool?.execute("tool-canonical-3", { + path: filePath, }); const textBlocks = result?.content?.filter((block) => block.type === "text") as @@ -40,62 +39,59 @@ describe("createOpenClawCodingTools", () => { } }); - it("accepts broader file/edit alias variants", async () => { - const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-alias-broad-")); + it("rejects legacy alias parameters", async () => { + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-legacy-alias-")); try { const tools = createOpenClawCodingTools({ workspaceDir: tmpDir }); const { readTool, writeTool, editTool } = expectReadWriteEditTools(tools); - await writeTool?.execute("tool-alias-broad-1", { - file: "broad-alias.txt", - content: "hello old value", - }); + await expect( + writeTool?.execute("tool-legacy-write", { + file: "legacy.txt", + content: "hello old value", + }), + ).rejects.toThrow(/Missing required parameter: path/); - await editTool?.execute("tool-alias-broad-2", { - filePath: "broad-alias.txt", - old_text: "old", - newString: "new", - }); + await expect( + editTool?.execute("tool-legacy-edit", { + filePath: "legacy.txt", + old_text: "old", + newString: "new", + }), + ).rejects.toThrow(/Missing required parameters: path, edits/); - const result = await readTool?.execute("tool-alias-broad-3", { - file: "broad-alias.txt", - }); - - const textBlocks = result?.content?.filter((block) => block.type === "text") as - | Array<{ text?: string }> - | undefined; - const combinedText = textBlocks?.map((block) => block.text ?? "").join("\n"); - expect(combinedText).toContain("hello new value"); + await expect( + readTool?.execute("tool-legacy-read", { + file_path: "legacy.txt", + }), + ).rejects.toThrow(/Missing required parameter: path/); } finally { await fs.rm(tmpDir, { recursive: true, force: true }); } }); - it("coerces structured content blocks for write", async () => { + it("rejects structured content blocks for write", async () => { const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-structured-write-")); try { const tools = createOpenClawCodingTools({ workspaceDir: tmpDir }); const writeTool = tools.find((tool) => tool.name === "write"); expect(writeTool).toBeDefined(); - await writeTool?.execute("tool-structured-write", { - path: "structured-write.js", - content: [ - { type: "text", text: "const path = require('path');\n" }, - { type: "input_text", text: "const root = path.join(process.env.HOME, 'clawd');\n" }, - ], - }); - - const written = await fs.readFile(path.join(tmpDir, "structured-write.js"), "utf8"); - expect(written).toBe( - "const path = require('path');\nconst root = path.join(process.env.HOME, 'clawd');\n", - ); + await expect( + writeTool?.execute("tool-structured-write", { + path: "structured-write.js", + content: [ + { type: "text", text: "const path = require('path');\n" }, + { type: "input_text", text: "const root = path.join(process.env.HOME, 'clawd');\n" }, + ], + }), + ).rejects.toThrow(/Missing required parameter: content/); } finally { await fs.rm(tmpDir, { recursive: true, force: true }); } }); - it("coerces structured old/new text blocks for edit", async () => { + it("rejects structured edit payloads", async () => { const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-structured-edit-")); try { const filePath = path.join(tmpDir, "structured-edit.js"); @@ -105,14 +101,17 @@ describe("createOpenClawCodingTools", () => { const editTool = tools.find((tool) => tool.name === "edit"); expect(editTool).toBeDefined(); - await editTool?.execute("tool-structured-edit", { - file_path: "structured-edit.js", - old_string: [{ type: "text", text: "old" }], - new_string: [{ kind: "text", value: "new" }], - }); - - const edited = await fs.readFile(filePath, "utf8"); - expect(edited).toBe("const value = 'new';\n"); + await expect( + editTool?.execute("tool-structured-edit", { + path: "structured-edit.js", + edits: [ + { + oldText: [{ type: "text", text: "old" }], + newText: [{ kind: "text", value: "new" }], + }, + ], + }), + ).rejects.toThrow(/Missing required parameter: edits/); } finally { await fs.rm(tmpDir, { recursive: true, force: true }); } diff --git a/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-g.test.ts b/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-g.test.ts index b8bb6bea971..d0edc8252ea 100644 --- a/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-g.test.ts +++ b/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-g.test.ts @@ -27,7 +27,7 @@ function extractToolText(result: unknown): string { } describe("createOpenClawCodingTools read behavior", () => { - it("applies sandbox path guards to file_path alias", async () => { + it("applies sandbox path guards to canonical path", async () => { const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-sbx-")); const outsidePath = path.join(os.tmpdir(), "openclaw-outside.txt"); await fs.writeFile(outsidePath, "outside", "utf8"); @@ -36,7 +36,7 @@ describe("createOpenClawCodingTools read behavior", () => { root: tmpDir, bridge: createHostSandboxFsBridge(tmpDir), }); - await expect(readTool.execute("sandbox-1", { file_path: outsidePath })).rejects.toThrow( + await expect(readTool.execute("sandbox-1", { path: outsidePath })).rejects.toThrow( /sandbox root/i, ); } finally { diff --git a/src/agents/pi-tools.host-edit.ts b/src/agents/pi-tools.host-edit.ts index aaa7b367f97..b3fee70e35e 100644 --- a/src/agents/pi-tools.host-edit.ts +++ b/src/agents/pi-tools.host-edit.ts @@ -1,7 +1,7 @@ import os from "node:os"; import path from "node:path"; import type { AgentToolResult, AgentToolUpdateCallback } from "@mariozechner/pi-agent-core"; -import { normalizeToolParams } from "./pi-tools.params.js"; +import { getToolParamsRecord } from "./pi-tools.params.js"; import type { AnyAgentTool } from "./pi-tools.types.js"; type EditToolRecoveryOptions = { @@ -61,12 +61,9 @@ function readEditReplacements(record: Record | undefined): Edit } function readEditToolParams(params: unknown): EditToolParams { - const normalized = normalizeToolParams(params); - const record = - normalized ?? - (params && typeof params === "object" ? (params as Record) : undefined); + const record = getToolParamsRecord(params); return { - pathParam: readStringParam(record, "path", "file_path", "filePath", "file"), + pathParam: readStringParam(record, "path"), edits: readEditReplacements(record), }; } diff --git a/src/agents/pi-tools.params.test.ts b/src/agents/pi-tools.params.test.ts index 0c426f94c62..caa7fdc2a9c 100644 --- a/src/agents/pi-tools.params.test.ts +++ b/src/agents/pi-tools.params.test.ts @@ -1,46 +1,32 @@ -import { Type } from "@sinclair/typebox"; import { describe, expect, it, vi } from "vitest"; import { assertRequiredParams, - CLAUDE_PARAM_GROUPS, - patchToolSchemaForClaudeCompatibility, - wrapToolParamNormalization, + REQUIRED_PARAM_GROUPS, + getToolParamsRecord, + wrapToolParamValidation, } from "./pi-tools.params.js"; describe("assertRequiredParams", () => { - it("patches Claude-compatible file tool schemas to disallow unknown parameters", () => { - const patched = patchToolSchemaForClaudeCompatibility({ - name: "read", - label: "read", - description: "read a file", - parameters: Type.Object({ - path: Type.String(), - offset: Type.Optional(Type.Number()), - limit: Type.Optional(Type.Number()), - }), - execute: vi.fn(), - }); - - expect((patched.parameters as { additionalProperties?: unknown }).additionalProperties).toBe( - false, - ); + it("returns object params unchanged", () => { + const params = { path: "test.txt" }; + expect(getToolParamsRecord(params)).toBe(params); }); it("includes received keys in error when some params are present but content is missing", () => { expect(() => assertRequiredParams( - { file_path: "test.txt" }, + { path: "test.txt" }, [ - { keys: ["path", "file_path"], label: "path alias" }, + { keys: ["path"], label: "path" }, { keys: ["content"], label: "content" }, ], "write", ), - ).toThrow(/\(received: file_path\)/); + ).toThrow(/\(received: path\)/); }); - it("shows normalized key in hint when called through wrapToolParamNormalization (file_path alias -> path)", async () => { - const tool = wrapToolParamNormalization( + it("does not normalize legacy aliases during validation", async () => { + const tool = wrapToolParamValidation( { name: "write", label: "write", @@ -48,24 +34,24 @@ describe("assertRequiredParams", () => { parameters: {}, execute: vi.fn(), }, - CLAUDE_PARAM_GROUPS.write, + REQUIRED_PARAM_GROUPS.write, ); await expect( tool.execute("id", { file_path: "test.txt" }, new AbortController().signal, vi.fn()), - ).rejects.toThrow(/\(received: path\)/); + ).rejects.toThrow(/\(received: file_path\)/); }); it("excludes null and undefined values from received hint", () => { expect(() => assertRequiredParams( - { file_path: "test.txt", content: null }, + { path: "test.txt", content: null }, [ - { keys: ["path", "file_path"], label: "path alias" }, + { keys: ["path"], label: "path" }, { keys: ["content"], label: "content" }, ], "write", ), - ).toThrow(/\(received: file_path\)[^,]/); + ).toThrow(/\(received: path\)[^,]/); }); it("shows empty-string values for present params that still fail validation", () => { @@ -73,7 +59,7 @@ describe("assertRequiredParams", () => { assertRequiredParams( { path: "/tmp/a.txt", content: " " }, [ - { keys: ["path", "file_path"], label: "path alias" }, + { keys: ["path"], label: "path" }, { keys: ["content"], label: "content" }, ], "write", @@ -82,7 +68,7 @@ describe("assertRequiredParams", () => { }); it("shows wrong-type values for present params that still fail validation", async () => { - const tool = wrapToolParamNormalization( + const tool = wrapToolParamValidation( { name: "write", label: "write", @@ -90,12 +76,12 @@ describe("assertRequiredParams", () => { parameters: {}, execute: vi.fn(), }, - CLAUDE_PARAM_GROUPS.write, + REQUIRED_PARAM_GROUPS.write, ); await expect( tool.execute( "id", - { file_path: "test.txt", content: { unexpected: true } }, + { path: "test.txt", content: { unexpected: true } }, new AbortController().signal, vi.fn(), ), @@ -107,7 +93,7 @@ describe("assertRequiredParams", () => { assertRequiredParams( { path: "/tmp/a.txt", extra: "yes" }, [ - { keys: ["path", "file_path"], label: "path alias" }, + { keys: ["path"], label: "path" }, { keys: ["content"], label: "content" }, ], "write", @@ -133,7 +119,7 @@ describe("assertRequiredParams", () => { assertRequiredParams( { path: "a.txt", content: "hello" }, [ - { keys: ["path", "file_path"], label: "path alias" }, + { keys: ["path"], label: "path" }, { keys: ["content"], label: "content" }, ], "write", diff --git a/src/agents/pi-tools.params.ts b/src/agents/pi-tools.params.ts index d7120acd050..0520621ba1a 100644 --- a/src/agents/pi-tools.params.ts +++ b/src/agents/pi-tools.params.ts @@ -46,141 +46,21 @@ function formatReceivedParamHint( return received.length > 0 ? ` (received: ${received.join(", ")})` : ""; } -export const CLAUDE_PARAM_GROUPS = { - read: [{ keys: ["path", "file_path", "filePath", "file"], label: "path alias" }], - write: [ - { keys: ["path", "file_path", "filePath", "file"], label: "path alias" }, - { keys: ["content"], label: "content" }, - ], - edit: [ - { keys: ["path", "file_path", "filePath", "file"], label: "path alias" }, - { - keys: ["oldText", "old_string", "old_text", "oldString"], - label: "oldText alias", - validator: hasValidEditReplacements, - }, - { - keys: ["newText", "new_string", "new_text", "newString"], - label: "newText alias", - allowEmpty: true, - validator: hasValidEditReplacements, - }, - ], -} as const; - -type ClaudeParamAlias = { - original: string; - alias: string; -}; - -const CLAUDE_PARAM_ALIASES: ClaudeParamAlias[] = [ - { original: "path", alias: "file_path" }, - { original: "path", alias: "filePath" }, - { original: "path", alias: "file" }, - { original: "oldText", alias: "old_string" }, - { original: "oldText", alias: "old_text" }, - { original: "oldText", alias: "oldString" }, - { original: "newText", alias: "new_string" }, - { original: "newText", alias: "new_text" }, - { original: "newText", alias: "newString" }, -]; - type EditReplacement = { oldText: string; newText: string; }; -function extractStructuredText(value: unknown, depth = 0): string | undefined { - if (depth > 6) { - return undefined; - } - if (typeof value === "string") { - return value; - } - if (Array.isArray(value)) { - const parts = value - .map((entry) => extractStructuredText(entry, depth + 1)) - .filter((entry): entry is string => typeof entry === "string"); - return parts.length > 0 ? parts.join("") : undefined; - } +function isValidEditReplacement(value: unknown): value is EditReplacement { if (!value || typeof value !== "object") { - return undefined; + return false; } const record = value as Record; - if (typeof record.text === "string") { - return record.text; - } - if (typeof record.content === "string") { - return record.content; - } - if (Array.isArray(record.content)) { - return extractStructuredText(record.content, depth + 1); - } - if (Array.isArray(record.parts)) { - return extractStructuredText(record.parts, depth + 1); - } - if (typeof record.value === "string" && record.value.length > 0) { - const type = typeof record.type === "string" ? record.type.toLowerCase() : ""; - const kind = typeof record.kind === "string" ? record.kind.toLowerCase() : ""; - if (type.includes("text") || kind === "text") { - return record.value; - } - } - return undefined; -} - -function normalizeTextLikeParam(record: Record, key: string) { - const value = record[key]; - if (typeof value === "string") { - return; - } - const extracted = extractStructuredText(value); - if (typeof extracted === "string") { - record[key] = extracted; - } -} - -function normalizeEditReplacement(value: unknown): EditReplacement | undefined { - if (!value || typeof value !== "object") { - return undefined; - } - const normalized = { ...(value as Record) }; - normalizeClaudeParamAliases(normalized); - normalizeTextLikeParam(normalized, "oldText"); - normalizeTextLikeParam(normalized, "newText"); - if (typeof normalized.oldText !== "string" || normalized.oldText.trim().length === 0) { - return undefined; - } - if (typeof normalized.newText !== "string") { - return undefined; - } - return { - oldText: normalized.oldText, - newText: normalized.newText, - }; -} - -function normalizeEditReplacements(record: Record) { - const replacements: EditReplacement[] = []; - if (Array.isArray(record.edits)) { - for (const entry of record.edits) { - const normalized = normalizeEditReplacement(entry); - if (normalized) { - replacements.push(normalized); - } - } - } - if (typeof record.oldText === "string" && record.oldText.trim().length > 0) { - if (typeof record.newText === "string") { - replacements.push({ - oldText: record.oldText, - newText: record.newText, - }); - } - } - if (replacements.length > 0) { - record.edits = replacements; - } + return ( + typeof record.oldText === "string" && + record.oldText.trim().length > 0 && + typeof record.newText === "string" + ); } function hasValidEditReplacements(record: Record): boolean { @@ -188,89 +68,24 @@ function hasValidEditReplacements(record: Record): boolean { return ( Array.isArray(edits) && edits.length > 0 && - edits.every((entry) => normalizeEditReplacement(entry) !== undefined) + edits.every((entry) => isValidEditReplacement(entry)) ); } -function normalizeClaudeParamAliases(record: Record) { - for (const { original, alias } of CLAUDE_PARAM_ALIASES) { - if (alias in record && !(original in record)) { - record[original] = record[alias]; - } - delete record[alias]; - } -} +export const REQUIRED_PARAM_GROUPS = { + read: [{ keys: ["path"], label: "path" }], + write: [ + { keys: ["path"], label: "path" }, + { keys: ["content"], label: "content" }, + ], + edit: [ + { keys: ["path"], label: "path" }, + { keys: ["edits"], label: "edits", validator: hasValidEditReplacements }, + ], +} as const; -function addClaudeParamAliasesToSchema(params: { - properties: Record; - required: string[]; -}): boolean { - let changed = false; - for (const { original, alias } of CLAUDE_PARAM_ALIASES) { - if (!(original in params.properties)) { - continue; - } - if (!(alias in params.properties)) { - params.properties[alias] = params.properties[original]; - changed = true; - } - const idx = params.required.indexOf(original); - if (idx !== -1) { - params.required.splice(idx, 1); - changed = true; - } - } - return changed; -} - -// Normalize tool parameters from Claude Code conventions to pi-coding-agent conventions. -// Claude Code uses file_path/old_string/new_string while pi-coding-agent uses path/oldText/newText. -// This prevents models trained on Claude Code from getting stuck in tool-call loops. -export function normalizeToolParams(params: unknown): Record | undefined { - if (!params || typeof params !== "object") { - return undefined; - } - const record = params as Record; - const normalized = { ...record }; - normalizeClaudeParamAliases(normalized); - // Some providers/models emit text payloads as structured blocks instead of raw strings. - // Normalize these for write/edit so content matching and writes stay deterministic. - normalizeTextLikeParam(normalized, "content"); - normalizeTextLikeParam(normalized, "oldText"); - normalizeTextLikeParam(normalized, "newText"); - normalizeEditReplacements(normalized); - return normalized; -} - -export function patchToolSchemaForClaudeCompatibility(tool: AnyAgentTool): AnyAgentTool { - const schema = - tool.parameters && typeof tool.parameters === "object" - ? (tool.parameters as Record) - : undefined; - - if (!schema || !schema.properties || typeof schema.properties !== "object") { - return tool; - } - - const properties = { ...(schema.properties as Record) }; - const required = Array.isArray(schema.required) - ? schema.required.filter((key): key is string => typeof key === "string") - : []; - const changed = addClaudeParamAliasesToSchema({ properties, required }); - - if (!changed) { - return tool; - } - - return { - ...tool, - parameters: { - ...schema, - additionalProperties: "additionalProperties" in schema ? schema.additionalProperties : false, - properties, - required, - }, - }; +export function getToolParamsRecord(params: unknown): Record | undefined { + return params && typeof params === "object" ? (params as Record) : undefined; } export function assertRequiredParams( @@ -314,23 +129,18 @@ export function assertRequiredParams( } } -// Generic wrapper to normalize parameters for any tool. -export function wrapToolParamNormalization( +export function wrapToolParamValidation( tool: AnyAgentTool, requiredParamGroups?: readonly RequiredParamGroup[], ): AnyAgentTool { - const patched = patchToolSchemaForClaudeCompatibility(tool); return { - ...patched, + ...tool, execute: async (toolCallId, params, signal, onUpdate) => { - const normalized = normalizeToolParams(params); - const record = - normalized ?? - (params && typeof params === "object" ? (params as Record) : undefined); + const record = getToolParamsRecord(params); if (requiredParamGroups?.length) { assertRequiredParams(record, requiredParamGroups, tool.name); } - return tool.execute(toolCallId, normalized ?? params, signal, onUpdate); + return tool.execute(toolCallId, params, signal, onUpdate); }, }; } diff --git a/src/agents/pi-tools.read.host-edit-recovery.test.ts b/src/agents/pi-tools.read.host-edit-recovery.test.ts index 9648c5873de..f6679138897 100644 --- a/src/agents/pi-tools.read.host-edit-recovery.test.ts +++ b/src/agents/pi-tools.read.host-edit-recovery.test.ts @@ -103,7 +103,7 @@ describe("edit tool recovery hardening", () => { await expect( tool.execute( "call-1", - { path: filePath, oldText: "missing", newText: "replacement" }, + { path: filePath, edits: [{ oldText: "missing", newText: "replacement" }] }, undefined, ), ).rejects.toThrow(/Current file contents:\nactual current content/); @@ -126,8 +126,12 @@ describe("edit tool recovery hardening", () => { "call-1", { path: filePath, - oldText: 'const value = "foo";\n', - newText: 'const value = "foobar";\n', + edits: [ + { + oldText: 'const value = "foo";\n', + newText: 'const value = "foobar";\n', + }, + ], }, undefined, ); @@ -154,7 +158,10 @@ describe("edit tool recovery hardening", () => { await expect( tool.execute( "call-1", - { path: filePath, oldText: "missing", newText: "replacement already present" }, + { + path: filePath, + edits: [{ oldText: "missing", newText: "replacement already present" }], + }, undefined, ), ).rejects.toThrow("Simulated post-write failure"); @@ -175,7 +182,7 @@ describe("edit tool recovery hardening", () => { }); const result = await tool.execute( "call-1", - { path: filePath, oldText: "delete me", newText: "" }, + { path: filePath, edits: [{ oldText: "delete me", newText: "" }] }, undefined, ); @@ -235,7 +242,7 @@ describe("edit tool recovery hardening", () => { }); const result = await tool.execute( "call-1", - { path: filePath, oldText: "old text", newText: "new text" }, + { path: filePath, edits: [{ oldText: "old text", newText: "new text" }] }, undefined, ); diff --git a/src/agents/pi-tools.read.ts b/src/agents/pi-tools.read.ts index c78a865f52e..91050b20699 100644 --- a/src/agents/pi-tools.read.ts +++ b/src/agents/pi-tools.read.ts @@ -16,11 +16,10 @@ import type { ImageSanitizationLimits } from "./image-sanitization.js"; import { toRelativeWorkspacePath } from "./path-policy.js"; import { wrapEditToolWithRecovery } from "./pi-tools.host-edit.js"; import { - CLAUDE_PARAM_GROUPS, + REQUIRED_PARAM_GROUPS, assertRequiredParams, - normalizeToolParams, - patchToolSchemaForClaudeCompatibility, - wrapToolParamNormalization, + getToolParamsRecord, + wrapToolParamValidation, } from "./pi-tools.params.js"; import type { AnyAgentTool } from "./pi-tools.types.js"; import { assertSandboxPath } from "./sandbox-paths.js"; @@ -28,15 +27,14 @@ import type { SandboxFsBridge } from "./sandbox/fs-bridge.js"; import { sanitizeToolResultImages } from "./tool-images.js"; export { - CLAUDE_PARAM_GROUPS, + REQUIRED_PARAM_GROUPS, assertRequiredParams, - normalizeToolParams, - patchToolSchemaForClaudeCompatibility, - wrapToolParamNormalization, + getToolParamsRecord, + wrapToolParamValidation, } from "./pi-tools.params.js"; // NOTE(steipete): Upstream read now does file-magic MIME detection; we keep the wrapper -// to normalize payloads and sanitize oversized images before they hit providers. +// to sanitize oversized images before they hit providers. type ToolContentBlock = AgentToolResult["content"][number]; type ImageContentBlock = Extract; type TextContentBlock = Extract; @@ -509,16 +507,13 @@ export function wrapToolMemoryFlushAppendOnlyWrite( ...tool, description: `${tool.description} During memory flush, this tool may only append to ${options.relativePath}.`, execute: async (toolCallId, args, signal, onUpdate) => { - const normalized = normalizeToolParams(args); - const record = - normalized ?? - (args && typeof args === "object" ? (args as Record) : undefined); - assertRequiredParams(record, CLAUDE_PARAM_GROUPS.write, tool.name); + const record = getToolParamsRecord(args); + assertRequiredParams(record, REQUIRED_PARAM_GROUPS.write, tool.name); const filePath = typeof record?.path === "string" && record.path.trim() ? record.path : undefined; const content = typeof record?.content === "string" ? record.content : undefined; if (!filePath || content === undefined) { - return tool.execute(toolCallId, normalized ?? args, signal, onUpdate); + return tool.execute(toolCallId, args, signal, onUpdate); } const resolvedPath = resolveToolPathAgainstWorkspaceRoot({ @@ -561,10 +556,7 @@ export function wrapToolWorkspaceRootGuardWithOptions( return { ...tool, execute: async (toolCallId, args, signal, onUpdate) => { - const normalized = normalizeToolParams(args); - const record = - normalized ?? - (args && typeof args === "object" ? (args as Record) : undefined); + const record = getToolParamsRecord(args); const filePath = record?.path; if (typeof filePath === "string" && filePath.trim()) { const sandboxPath = mapContainerPathToWorkspaceRoot({ @@ -574,7 +566,7 @@ export function wrapToolWorkspaceRootGuardWithOptions( }); await assertSandboxPath({ filePath: sandboxPath, cwd: root, root }); } - return tool.execute(toolCallId, normalized ?? args, signal, onUpdate); + return tool.execute(toolCallId, args, signal, onUpdate); }, }; } @@ -600,7 +592,7 @@ export function createSandboxedWriteTool(params: SandboxToolParams) { const base = createWriteTool(params.root, { operations: createSandboxWriteOperations(params), }) as unknown as AnyAgentTool; - return wrapToolParamNormalization(base, CLAUDE_PARAM_GROUPS.write); + return wrapToolParamValidation(base, REQUIRED_PARAM_GROUPS.write); } export function createSandboxedEditTool(params: SandboxToolParams) { @@ -612,14 +604,14 @@ export function createSandboxedEditTool(params: SandboxToolParams) { readFile: async (absolutePath: string) => (await params.bridge.readFile({ filePath: absolutePath, cwd: params.root })).toString("utf8"), }); - return wrapToolParamNormalization(withRecovery, CLAUDE_PARAM_GROUPS.edit); + return wrapToolParamValidation(withRecovery, REQUIRED_PARAM_GROUPS.edit); } export function createHostWorkspaceWriteTool(root: string, options?: { workspaceOnly?: boolean }) { const base = createWriteTool(root, { operations: createHostWriteOperations(root, options), }) as unknown as AnyAgentTool; - return wrapToolParamNormalization(base, CLAUDE_PARAM_GROUPS.write); + return wrapToolParamValidation(base, REQUIRED_PARAM_GROUPS.write); } export function createHostWorkspaceEditTool(root: string, options?: { workspaceOnly?: boolean }) { @@ -630,26 +622,22 @@ export function createHostWorkspaceEditTool(root: string, options?: { workspaceO root, readFile: (absolutePath: string) => fs.readFile(absolutePath, "utf-8"), }); - return wrapToolParamNormalization(withRecovery, CLAUDE_PARAM_GROUPS.edit); + return wrapToolParamValidation(withRecovery, REQUIRED_PARAM_GROUPS.edit); } export function createOpenClawReadTool( base: AnyAgentTool, options?: OpenClawReadToolOptions, ): AnyAgentTool { - const patched = patchToolSchemaForClaudeCompatibility(base); return { - ...patched, + ...base, execute: async (toolCallId, params, signal) => { - const normalized = normalizeToolParams(params); - const record = - normalized ?? - (params && typeof params === "object" ? (params as Record) : undefined); - assertRequiredParams(record, CLAUDE_PARAM_GROUPS.read, base.name); + const record = getToolParamsRecord(params); + assertRequiredParams(record, REQUIRED_PARAM_GROUPS.read, base.name); const result = await executeReadWithAdaptivePaging({ base, toolCallId, - args: (normalized ?? params ?? {}) as Record, + args: record ?? {}, signal, maxBytes: resolveAdaptiveReadMaxBytes(options), }); diff --git a/src/agents/pi-tools.ts b/src/agents/pi-tools.ts index 6dc77ad5b08..a095db9329a 100644 --- a/src/agents/pi-tools.ts +++ b/src/agents/pi-tools.ts @@ -38,12 +38,11 @@ import { createSandboxedEditTool, createSandboxedReadTool, createSandboxedWriteTool, - normalizeToolParams, - patchToolSchemaForClaudeCompatibility, + getToolParamsRecord, wrapToolMemoryFlushAppendOnlyWrite, wrapToolWorkspaceRootGuard, wrapToolWorkspaceRootGuardWithOptions, - wrapToolParamNormalization, + wrapToolParamValidation, } from "./pi-tools.read.js"; import { cleanToolSchemaForGemini, normalizeToolParameters } from "./pi-tools.schema.js"; import type { AnyAgentTool } from "./pi-tools.types.js"; @@ -234,9 +233,8 @@ export function resolveToolLoopDetectionConfig(params: { export const __testing = { cleanToolSchemaForGemini, - normalizeToolParams, - patchToolSchemaForClaudeCompatibility, - wrapToolParamNormalization, + getToolParamsRecord, + wrapToolParamValidation, assertRequiredParams, applyModelProviderToolPolicy, } as const;