From 7dcfb449c2eb9443b295b8f03fb816c8f1bececa Mon Sep 17 00:00:00 2001 From: Travelguest Date: Sun, 12 Apr 2026 10:28:54 +0800 Subject: [PATCH] refactor(coding): migrate write_file and str_replace to structured tool-result Align the two remaining legacy tools with the StructuredToolResult architecture introduced in recent commits. Both tools now use `okToolResult()` / `errorToolResult()` from `./tool-result` and `ensureAbsolutePath()` from `./tool-utils`. **write_file changes:** - Add absolute path validation (INVALID_PATH) - Add try/catch with structured error (WRITE_FAILED) - Auto-create parent directories when missing - Return structured success with `{ path, bytes }` **str_replace changes:** - Add absolute path validation (INVALID_PATH) - Replace ad-hoc `{ ok, error }` returns with structured results - Add error codes: FILE_NOT_FOUND, INVALID_ARGUMENT, NOT_FOUND, WRITE_FAILED - Remove unnecessary mkdir logic (str_replace operates on existing files) - Wrap file.write() in try/catch **Test updates:** - Update write-file tests to match structured result shape - Add test: creates parent directories automatically - Add test: returns INVALID_PATH for relative paths - Update str-replace tests to match structured result shape - Add test: returns INVALID_PATH for relative paths --- .../tools/__tests__/str-replace.test.ts | 42 ++++++++--------- src/coding/tools/__tests__/write-file.test.ts | 31 ++++++++++-- src/coding/tools/str-replace.ts | 47 +++++++++++++------ src/coding/tools/write-file.ts | 32 +++++++++++-- 4 files changed, 108 insertions(+), 44 deletions(-) diff --git a/src/coding/tools/__tests__/str-replace.test.ts b/src/coding/tools/__tests__/str-replace.test.ts index bc97860..a749fc0 100644 --- a/src/coding/tools/__tests__/str-replace.test.ts +++ b/src/coding/tools/__tests__/str-replace.test.ts @@ -28,11 +28,9 @@ describe("strReplaceTool", () => { new: "y", }); - expect(result).toEqual({ + expect(result).toMatchObject({ ok: true, - path: filePath, - replacements: 2, - changed: true, + data: { path: filePath, replacements: 2, changed: true }, }); await expect(readFile(filePath, "utf8")).resolves.toBe("a y b y c\n"); }); @@ -49,11 +47,9 @@ describe("strReplaceTool", () => { count: 1, }); - expect(result).toEqual({ + expect(result).toMatchObject({ ok: true, - path: filePath, - replacements: 1, - changed: true, + data: { path: filePath, replacements: 1, changed: true }, }); await expect(readFile(filePath, "utf8")).resolves.toBe("ONE two one three\n"); }); @@ -70,11 +66,9 @@ describe("strReplaceTool", () => { count: 0, }); - expect(result).toEqual({ + expect(result).toMatchObject({ ok: true, - path: filePath, - replacements: 0, - changed: false, + data: { path: filePath, replacements: 0, changed: false }, }); await expect(readFile(filePath, "utf8")).resolves.toBe("abc\n"); }); @@ -89,10 +83,7 @@ describe("strReplaceTool", () => { new: "b", }); - expect(result).toEqual({ - ok: false, - error: `File ${filePath} does not exist.`, - }); + expect(result).toMatchObject({ ok: false, code: "FILE_NOT_FOUND" }); }); test("returns error when old is empty", async () => { @@ -106,10 +97,7 @@ describe("strReplaceTool", () => { new: "y", }); - expect(result).toEqual({ - ok: false, - error: "`old` must be a non-empty string.", - }); + expect(result).toMatchObject({ ok: false, code: "INVALID_ARGUMENT" }); }); test("returns error when old is not found", async () => { @@ -123,9 +111,17 @@ describe("strReplaceTool", () => { new: "yes", }); - expect(result).toEqual({ - ok: false, - error: `No occurrences of 'old' found in ${filePath}.`, + expect(result).toMatchObject({ ok: false, code: "NOT_FOUND" }); + }); + + test("returns error for relative path", async () => { + const result = await strReplaceTool.invoke({ + description: "Relative path test", + path: "relative/file.txt", + old: "a", + new: "b", }); + + expect(result).toMatchObject({ ok: false, code: "INVALID_PATH" }); }); }); diff --git a/src/coding/tools/__tests__/write-file.test.ts b/src/coding/tools/__tests__/write-file.test.ts index c1ba944..ede488d 100644 --- a/src/coding/tools/__tests__/write-file.test.ts +++ b/src/coding/tools/__tests__/write-file.test.ts @@ -26,7 +26,7 @@ describe("writeFileTool", () => { content: "hello\nworld\n", }); - expect(result).toBeUndefined(); + expect(result).toMatchObject({ ok: true, data: { path: filePath, bytes: 12 } }); await expect(readFile(filePath, "utf8")).resolves.toBe("hello\nworld\n"); }); @@ -34,12 +34,13 @@ describe("writeFileTool", () => { const filePath = join(tempDir, "mutable.txt"); await writeFile(filePath, "before\n"); - await writeFileTool.invoke({ + const result = await writeFileTool.invoke({ description: "Overwrite file", path: filePath, content: "after\n", }); + expect(result).toMatchObject({ ok: true }); await expect(readFile(filePath, "utf8")).resolves.toBe("after\n"); }); @@ -48,12 +49,36 @@ describe("writeFileTool", () => { await mkdir(subDir, { recursive: true }); const filePath = join(subDir, "deep.txt"); - await writeFileTool.invoke({ + const result = await writeFileTool.invoke({ description: "Write nested file", path: filePath, content: "nested\n", }); + expect(result).toMatchObject({ ok: true }); await expect(readFile(filePath, "utf8")).resolves.toBe("nested\n"); }); + + test("creates parent directories when they do not exist", async () => { + const filePath = join(tempDir, "a", "b", "c", "deep.txt"); + + const result = await writeFileTool.invoke({ + description: "Write deeply nested file", + path: filePath, + content: "deep content\n", + }); + + expect(result).toMatchObject({ ok: true, data: { path: filePath } }); + await expect(readFile(filePath, "utf8")).resolves.toBe("deep content\n"); + }); + + test("returns error for relative path", async () => { + const result = await writeFileTool.invoke({ + description: "Relative path test", + path: "relative/path.txt", + content: "should fail", + }); + + expect(result).toMatchObject({ ok: false, code: "INVALID_PATH" }); + }); }); diff --git a/src/coding/tools/str-replace.ts b/src/coding/tools/str-replace.ts index 3d7a4bb..b16a1a9 100644 --- a/src/coding/tools/str-replace.ts +++ b/src/coding/tools/str-replace.ts @@ -1,10 +1,10 @@ -import { exists, mkdir } from "node:fs/promises"; -import { parse } from "node:path"; - import z from "zod"; import { defineTool } from "@/foundation"; +import { errorToolResult, okToolResult } from "./tool-result"; +import { ensureAbsolutePath } from "./tool-utils"; + export const strReplaceTool = defineTool({ name: "str_replace", description: "Replace occurrences of a substring in a file. Make sure the `old` is unique in the file.", @@ -23,22 +23,32 @@ export const strReplaceTool = defineTool({ .optional(), }), invoke: async ({ path, old, new: replacement, count }) => { + const absolute = ensureAbsolutePath(path); + if (!absolute.ok) { + return errorToolResult(absolute.error, "INVALID_PATH", { path }); + } + const file = Bun.file(path); if (!(await file.exists())) { - return { ok: false as const, error: `File ${path} does not exist.` }; + return errorToolResult(`File ${path} does not exist.`, "FILE_NOT_FOUND", { path }); } if (old.length === 0) { - return { ok: false as const, error: "`old` must be a non-empty string." }; + return errorToolResult("`old` must be a non-empty string.", "INVALID_ARGUMENT", { path }); } const text = await file.text(); const maxReplacements = count ?? Number.POSITIVE_INFINITY; if (maxReplacements === 0) { - return { ok: true as const, path, replacements: 0, changed: false as const }; + return okToolResult(`No replacements requested (count=0) in ${path}`, { + path, + replacements: 0, + changed: false, + }); } + // Count actual occurrences up to the limit let replacements = 0; let idx = 0; while (replacements < maxReplacements) { @@ -49,7 +59,7 @@ export const strReplaceTool = defineTool({ } if (replacements === 0) { - return { ok: false as const, error: `No occurrences of 'old' found in ${path}.` }; + return errorToolResult(`No occurrences of 'old' found in ${path}.`, "NOT_FOUND", { path }); } let updated: string; @@ -65,16 +75,23 @@ export const strReplaceTool = defineTool({ } if (updated === text) { - return { ok: true as const, path, replacements: 0, changed: false as const }; + return okToolResult(`No effective changes in ${path}`, { + path, + replacements: 0, + changed: false, + }); } - // Make sure the parent directory exists - const parentDir = parse(path).dir; - if (!(await exists(parentDir))) { - await mkdir(parentDir, { recursive: true }); + try { + await file.write(updated); + return okToolResult(`Replaced ${replacements} occurrence(s) in ${path}`, { + path, + replacements, + changed: true, + }); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + return errorToolResult(`Failed to write replacement to ${path}`, "WRITE_FAILED", { path, message }); } - - await file.write(updated); - return { ok: true as const, path, replacements, changed: true as const }; }, }); diff --git a/src/coding/tools/write-file.ts b/src/coding/tools/write-file.ts index 55b9518..9293e67 100644 --- a/src/coding/tools/write-file.ts +++ b/src/coding/tools/write-file.ts @@ -1,10 +1,16 @@ +import { exists, mkdir } from "node:fs/promises"; +import { parse } from "node:path"; + import z from "zod"; import { defineTool } from "@/foundation"; +import { errorToolResult, okToolResult } from "./tool-result"; +import { ensureAbsolutePath } from "./tool-utils"; + export const writeFileTool = defineTool({ name: "write_file", - description: "Write to a file at an absolute path.", + description: "Write to a file at an absolute path. Creates parent directories if they do not exist.", parameters: z.object({ description: z .string() @@ -13,7 +19,27 @@ export const writeFileTool = defineTool({ content: z.string().describe("The content to write to the file."), }), invoke: async ({ path, content }) => { - const file = Bun.file(path); - await file.write(content); + const absolute = ensureAbsolutePath(path); + if (!absolute.ok) { + return errorToolResult(absolute.error, "INVALID_PATH", { path }); + } + + try { + // Ensure parent directory exists + const parentDir = parse(path).dir; + if (!(await exists(parentDir))) { + await mkdir(parentDir, { recursive: true }); + } + + const file = Bun.file(path); + await file.write(content); + return okToolResult(`Successfully wrote ${content.length} chars to ${path}`, { + path, + bytes: content.length, + }); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + return errorToolResult(`Failed to write file: ${path}`, "WRITE_FAILED", { path, message }); + } }, });