diff --git a/.changeset/fix-file-operation-error-handling.md b/.changeset/fix-file-operation-error-handling.md new file mode 100644 index 00000000..a13a71c3 --- /dev/null +++ b/.changeset/fix-file-operation-error-handling.md @@ -0,0 +1,5 @@ +--- +"@perstack/runtime": patch +--- + +Handle file read errors gracefully in resolving PDF and image file states diff --git a/CODE_REVIEW_REPORT.md b/CODE_REVIEW_REPORT.md index 7f5bee17..5fffa323 100644 --- a/CODE_REVIEW_REPORT.md +++ b/CODE_REVIEW_REPORT.md @@ -18,7 +18,7 @@ Overall, the codebase is well-structured with strong architectural decisions. Th | βœ… Verified | 5 | Confirmed not an issue / working correctly | | πŸ“ Documented | 1 | Behavior documented, no code change needed | | ⏸️ Deferred | 8 | Low priority / E2E scope / future work | -| πŸ”΄ Open | 5 | Runtime package issues (2025-12-03) | +| πŸ”΄ Open | 4 | Runtime package issues (2025-12-03) | | 🟑 Low Prio | 4 | Runtime minor issues (2025-12-03) | --- @@ -698,31 +698,16 @@ experts[expertKey] = toRuntimeExpert(expert) // Mutates argument **Status**: πŸ”΄ **Open** +**Status**: βœ… **Fixed** β€” Commit `796f981` + **Category**: Potential Bug **Severity**: Minor -**Location**: `packages/runtime/src/states/resolving-pdf-file.ts:31`, `resolving-image-file.ts:31` - -**Issue**: `readFile(path)` can throw if file doesn't exist or is inaccessible, but errors are not caught: - -```typescript -const file = await readFile(path).then((buffer) => ({ - encodedData: buffer.toString("base64"), - // ... -})) -``` +**Location**: `packages/runtime/src/states/resolving-pdf-file.ts`, `resolving-image-file.ts` -**Impact**: File read errors will crash the runtime instead of being fed back to the LLM. +**Issue**: `readFile(path)` can throw if file doesn't exist or is inaccessible. -**Recommendation**: Wrap in try-catch and return error message to LLM: -```typescript -try { - const buffer = await readFile(path) - // ... -} catch (error) { - files.push({ type: "textPart", text: `Error reading file: ${error.message}` }) -} -``` +**Resolution**: Added try-catch blocks to handle file read errors gracefully. Errors are now returned as text parts to the LLM instead of crashing. --- @@ -917,7 +902,7 @@ const metaInstruction = dedent` | #34 | Delegate expert not found error | πŸ”΄ Open | | #35 | Skill object mutation | πŸ”΄ Open | | #36 | experts object mutation | πŸ”΄ Open | -| #37 | File operation error handling | πŸ”΄ Open | +| #37 | File operation error handling | βœ… Fixed | | #38 | RunSetting schema validation | πŸ”΄ Open | | #39 | closeSkillManagers failure handling | βœ… Fixed | | #40 | EventEmitter listener errors | 🟑 Low priority | @@ -945,7 +930,6 @@ const metaInstruction = dedent` **Minor (Should Fix)**: - **#34**: Delegate expert not found β€” improve error message - **#35, #36**: Object mutation side effects β€” defensive copying recommended -- **#37**: File read errors unhandled β€” should feed back to LLM - **#38**: RunSetting not validated β€” use Zod schema **By Design / Needs Clarification**: @@ -972,3 +956,4 @@ const metaInstruction = dedent` | `5490ebb` | Refactor: Split SkillManager into separate classes | | `ced1aa6` | Fix: maxSteps off-by-one error in finishing step | | `dac71b5` | Fix: Handle individual close failures in closeSkillManagers | +| `796f981` | Fix: Handle file read errors gracefully in resolving file states | diff --git a/packages/runtime/src/states/resolving-image-file.test.ts b/packages/runtime/src/states/resolving-image-file.test.ts index 31c5b3d3..a7ba693e 100644 --- a/packages/runtime/src/states/resolving-image-file.test.ts +++ b/packages/runtime/src/states/resolving-image-file.test.ts @@ -1,16 +1,22 @@ import { createId } from "@paralleldrive/cuid2" -import { describe, expect, it, vi } from "vitest" +import { readFile } from "node:fs/promises" +import { beforeEach, describe, expect, it, vi } from "vitest" import { createCheckpoint, createRunSetting, createStep } from "../../test/run-params.js" import { StateMachineLogics } from "../index.js" vi.mock("node:fs/promises", () => ({ - readFile: vi.fn().mockImplementation(() => { - return Promise.resolve(Buffer.from("encoded_image")) - }), + readFile: vi.fn(), })) +const mockReadFile = vi.mocked(readFile) + describe("@perstack/runtime: StateMachineLogic['ResolvingImageFile']", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + it("processes image file tool result correctly", async () => { + mockReadFile.mockResolvedValue(Buffer.from("encoded_image")) const imageInfo = { path: "/test/image.png", mimeType: "image/png", @@ -77,4 +83,50 @@ describe("@perstack/runtime: StateMachineLogic['ResolvingImageFile']", () => { ], }) }) + + it("handles file read error gracefully", async () => { + mockReadFile.mockRejectedValue(new Error("ENOENT: no such file or directory")) + const imageInfo = { + path: "/nonexistent.png", + mimeType: "image/png", + size: 1024, + } + const setting = createRunSetting() + const checkpoint = createCheckpoint() + const step = createStep({ + toolCall: { + id: "tc_123", + skillName: "@perstack/base", + toolName: "readImageFile", + args: { path: "/nonexistent.png" }, + }, + toolResult: { + id: "tr_123", + skillName: "@perstack/base", + toolName: "readImageFile", + result: [ + { + type: "textPart" as const, + text: JSON.stringify(imageInfo), + id: createId(), + }, + ], + }, + }) + const result = await StateMachineLogics.ResolvingImageFile({ + setting, + checkpoint, + step, + eventListener: async () => {}, + skillManagers: {}, + }) + expect(result.type).toBe("finishToolCall") + if (result.type !== "finishToolCall") throw new Error("Unexpected event type") + const toolResultPart = result.newMessages[0].contents[0] + if (toolResultPart.type !== "toolResultPart") throw new Error("Unexpected part type") + expect(toolResultPart.contents[0]).toMatchObject({ + type: "textPart", + text: expect.stringContaining('Failed to read image file "/nonexistent.png"'), + }) + }) }) diff --git a/packages/runtime/src/states/resolving-image-file.ts b/packages/runtime/src/states/resolving-image-file.ts index 84f21162..3c06a6a9 100644 --- a/packages/runtime/src/states/resolving-image-file.ts +++ b/packages/runtime/src/states/resolving-image-file.ts @@ -28,16 +28,19 @@ export async function resolvingImageFileLogic({ continue } const { path, mimeType, size } = imageInfo - const file = await readFile(path).then((buffer) => ({ - encodedData: buffer.toString("base64"), - mimeType, - size, - })) - files.push({ - type: "imageInlinePart", - encodedData: file.encodedData, - mimeType: file.mimeType, - }) + try { + const buffer = await readFile(path) + files.push({ + type: "imageInlinePart", + encodedData: buffer.toString("base64"), + mimeType, + }) + } catch (error) { + files.push({ + type: "textPart", + text: `Failed to read image file "${path}": ${error instanceof Error ? error.message : String(error)}`, + }) + } } return finishToolCall(setting, checkpoint, { newMessages: [ diff --git a/packages/runtime/src/states/resolving-pdf-file.test.ts b/packages/runtime/src/states/resolving-pdf-file.test.ts index 46c40f2d..05bbdbff 100644 --- a/packages/runtime/src/states/resolving-pdf-file.test.ts +++ b/packages/runtime/src/states/resolving-pdf-file.test.ts @@ -1,16 +1,22 @@ import { createId } from "@paralleldrive/cuid2" -import { describe, expect, it, vi } from "vitest" +import { readFile } from "node:fs/promises" +import { beforeEach, describe, expect, it, vi } from "vitest" import { createCheckpoint, createRunSetting, createStep } from "../../test/run-params.js" import { StateMachineLogics } from "../index.js" vi.mock("node:fs/promises", () => ({ - readFile: vi.fn().mockImplementation(() => { - return Promise.resolve(Buffer.from("encoded_pdf_content")) - }), + readFile: vi.fn(), })) +const mockReadFile = vi.mocked(readFile) + describe("@perstack/runtime: StateMachineLogic['ResolvingPdfFile']", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + it("processes PDF file tool result correctly", async () => { + mockReadFile.mockResolvedValue(Buffer.from("encoded_pdf_content")) const pdfInfo = { path: "/test.pdf", mimeType: "application/pdf", @@ -88,4 +94,49 @@ describe("@perstack/runtime: StateMachineLogic['ResolvingPdfFile']", () => { ], }) }) + + it("handles file read error gracefully", async () => { + mockReadFile.mockRejectedValue(new Error("ENOENT: no such file or directory")) + const pdfInfo = { + path: "/nonexistent.pdf", + mimeType: "application/pdf", + size: 2048, + } + const setting = createRunSetting() + const checkpoint = createCheckpoint() + const step = createStep({ + toolCall: { + id: "tc_123", + skillName: "@perstack/base", + toolName: "readPdfFile", + args: { path: "/nonexistent.pdf" }, + }, + toolResult: { + id: "tr_123", + skillName: "@perstack/base", + toolName: "readPdfFile", + result: [ + { + type: "textPart" as const, + text: JSON.stringify(pdfInfo), + id: createId(), + }, + ], + }, + }) + const result = await StateMachineLogics.ResolvingPdfFile({ + setting, + checkpoint, + step, + eventListener: async () => {}, + skillManagers: {}, + }) + expect(result.type).toBe("finishToolCall") + if (result.type !== "finishToolCall") throw new Error("Unexpected event type") + const userMessage = result.newMessages[1] + expect(userMessage.contents[0]).toMatchObject({ + type: "textPart", + text: expect.stringContaining('Failed to read PDF file "/nonexistent.pdf"'), + }) + }) }) diff --git a/packages/runtime/src/states/resolving-pdf-file.ts b/packages/runtime/src/states/resolving-pdf-file.ts index 8aa6b8d5..fafa3d8e 100644 --- a/packages/runtime/src/states/resolving-pdf-file.ts +++ b/packages/runtime/src/states/resolving-pdf-file.ts @@ -28,16 +28,19 @@ export async function resolvingPdfFileLogic({ continue } const { path, mimeType, size } = pdfInfo - const file = await readFile(path).then((buffer) => ({ - encodedData: buffer.toString("base64"), - mimeType, - size, - })) - files.push({ - type: "fileInlinePart", - encodedData: file.encodedData, - mimeType: file.mimeType, - }) + try { + const buffer = await readFile(path) + files.push({ + type: "fileInlinePart", + encodedData: buffer.toString("base64"), + mimeType, + }) + } catch (error) { + files.push({ + type: "textPart", + text: `Failed to read PDF file "${path}": ${error instanceof Error ? error.message : String(error)}`, + }) + } } return finishToolCall(setting, checkpoint, { newMessages: [