From 76d0faef4274ff65fd087e0ff5fde0e39078a3d3 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 8 Jun 2025 04:02:04 +0100 Subject: [PATCH] image + analyze keeps the temp files --- src/tools/image.ts | 30 +------- tests/integration/image-tool.test.ts | 111 +++++++++++++++++++++------ tests/unit/tools/analyze.test.ts | 61 +++++++++++++++ tests/unit/tools/image.test.ts | 26 ++++--- 4 files changed, 163 insertions(+), 65 deletions(-) diff --git a/src/tools/image.ts b/src/tools/image.ts index 2a62327..18651ae 100644 --- a/src/tools/image.ts +++ b/src/tools/image.ts @@ -86,14 +86,8 @@ export async function imageToolHandler( const captureData = imageData; - // Determine which files to report as saved - if (tempDirUsed) { - // Temporary directory was used - don't include in saved_files - finalSavedFiles = []; - } else { - // User provided path or default save behavior - include in saved_files - finalSavedFiles = captureData.saved_files || []; - } + // Always report all saved files + finalSavedFiles = captureData.saved_files || []; if (input.question) { analysisAttempted = true; @@ -231,25 +225,5 @@ export async function imageToolHandler( isError: true, _meta: { backend_error_code: "UNEXPECTED_HANDLER_ERROR" }, }; - } finally { - // If we used a temporary directory, we need to clean up all files inside it and the directory itself. - if (tempDirUsed) { - logger.debug( - { tempDir: tempDirUsed }, - "Attempting to delete temporary capture directory.", - ); - try { - await fs.rm(tempDirUsed, { recursive: true, force: true }); - logger.info( - { tempDir: tempDirUsed }, - "Temporary capture directory deleted.", - ); - } catch (cleanupError) { - logger.warn( - { error: cleanupError, path: tempDirUsed }, - "Failed to delete temporary capture directory.", - ); - } - } } } diff --git a/tests/integration/image-tool.test.ts b/tests/integration/image-tool.test.ts index cbf356e..3f28908 100644 --- a/tests/integration/image-tool.test.ts +++ b/tests/integration/image-tool.test.ts @@ -82,7 +82,6 @@ describe("Image Tool Integration Tests", () => { describe("Output Handling", () => { it("should capture screen and return base64 data when no arguments are provided", async () => { // This test covers the user-reported bug where calling 'image' with no args caused a 'failed to write' error. - const rmSpy = vi.spyOn(fs, "rm"); // Mock resolveImagePath to return a temp directory mockResolveImagePath.mockResolvedValue({ @@ -112,14 +111,10 @@ describe("Image Tool Integration Tests", () => { // Verify the result is correct expect(result.isError).toBeUndefined(); - expect(result.saved_files).toEqual([]); // No persistent files + // Now returns the temp file in saved_files + expect(result.saved_files).toEqual([{ path: MOCK_SAVED_FILE_PATH, mime_type: "image/png" }]); const imageContent = result.content.find(c => c.type === "image"); expect(imageContent?.data).toBe("base64-no-args-test"); - - // Verify cleanup using the new method - expect(rmSpy).toHaveBeenCalledWith(MOCK_TEMP_DIR, { recursive: true, force: true }); - - rmSpy.mockRestore(); }); it("should return an error if the Swift CLI fails", async () => { @@ -204,8 +199,13 @@ describe("Image Tool Integration Tests", () => { expect.arrayContaining(["image", "--mode", "screen", "--screen-index", "0"]), mockContext.logger ); - // Since temp dir was used, saved_files should be empty - expect(result.saved_files).toEqual([]); + // Since temp dir was used, saved_files now contains the temp file + const mockResponse = mockSwiftCli.captureImage("screen", { + path: MOCK_SAVED_FILE_PATH, + format: "png", + item_label: "Display 0 (Index 0)" + }); + expect(result.saved_files).toEqual(mockResponse.data.saved_files); }); it("should handle screen:INDEX format (invalid index)", async () => { @@ -270,8 +270,12 @@ describe("Image Tool Integration Tests", () => { expect.arrayContaining(["image", "--mode", "screen", "--screen-index", "99"]), mockContext.logger ); - // Since temp dir was used, saved_files should be empty - expect(result.saved_files).toEqual([]); + // Since temp dir was used, saved_files now contains the temp file + expect(result.saved_files).toEqual([{ + path: MOCK_SAVED_FILE_PATH, + mime_type: "image/png", + item_label: "All Screens" + }]); }); it("should handle frontmost app_target (with warning)", async () => { @@ -536,7 +540,7 @@ describe("Image Tool Integration Tests", () => { vi.clearAllMocks(); }); - it("should analyze image and delete temp file when no path provided", async () => { + it("should analyze image and PRESERVE temp file when no path provided", async () => { const input: ImageInput = { question: "What is in this image?" }; // Mock resolveImagePath to return temp directory when question is asked @@ -562,8 +566,12 @@ describe("Image Tool Integration Tests", () => { const imageContent = result.content.find((item) => item.type === "image"); expect(imageContent).toBeUndefined(); - // saved_files should be empty (temp file was deleted) - expect(result.saved_files).toEqual([]); + // saved_files should now contain the temp file (preserved) + const MOCK_SAVED_FILES = mockSwiftCli.captureImage("screen", { + path: MOCK_SAVED_FILE_PATH, + format: "png" + }); + expect(result.saved_files).toEqual(MOCK_SAVED_FILES.data.saved_files); }); it("should analyze image and keep file when path is provided", async () => { @@ -684,11 +692,69 @@ describe("Image Tool Integration Tests", () => { const expectedAnalysisText = "Analysis for Window 1:\nFirst analysis.\n\nAnalysis for Window 2:\nSecond analysis."; expect(result.analysis_text).toBe(expectedAnalysisText); - // Verify that fs.rm was called to clean up the temporary directory - expect(fs.rm).toHaveBeenCalledWith(MOCK_TEMP_DIR, { recursive: true, force: true }); + // Verify saved files now contain all captured files + expect(result.saved_files).toEqual([ + { + path: `${MOCK_TEMP_DIR}/window1.png`, + mime_type: "image/png", + item_label: "Window 1" + }, + { + path: `${MOCK_TEMP_DIR}/window2.png`, + mime_type: "image/png", + item_label: "Window 2" + } + ]); + }); + + it("should NOT delete the temporary file when a question is asked", async () => { + const input: ImageInput = { question: "What is in this image?" }; - // Verify no saved files (temp files were cleaned up) - expect(result.saved_files).toEqual([]); + // Mock resolveImagePath to return a temporary directory path + mockResolveImagePath.mockResolvedValue({ + effectivePath: MOCK_TEMP_DIR, + tempDirUsed: MOCK_TEMP_DIR, + }); + + // Mock successful screen capture with one or more files + mockExecuteSwiftCli.mockResolvedValue({ + success: true, + data: { + saved_files: [ + { + path: `${MOCK_TEMP_DIR}/captured_image.png`, + mime_type: "image/png", + item_label: "Screen Capture" + } + ], + }, + messages: ["Captured 1 image"] + }); + + // Mock performAutomaticAnalysis with a successful response + mockPerformAutomaticAnalysis.mockResolvedValue({ + analysisText: "This is a mock analysis of the captured screen.", + modelUsed: "mock/test" + }); + + // Call imageToolHandler with a question but no path + const result = await imageToolHandler(input, mockContext); + + // Most important assertion: Verify that fs.rm was NOT called + expect(fs.rm).not.toHaveBeenCalled(); + + // Verify that result.saved_files is populated with the saved files from Swift CLI + expect(result.saved_files).toEqual([ + { + path: `${MOCK_TEMP_DIR}/captured_image.png`, + mime_type: "image/png", + item_label: "Screen Capture" + } + ]); + + // Additional verification: analysis was performed + expect(mockPerformAutomaticAnalysis).toHaveBeenCalled(); + expect(result.analysis_text).toBe("This is a mock analysis of the captured screen."); }); }); @@ -810,7 +876,6 @@ describe("Image Tool Integration Tests", () => { it("should NOT use PEEKABOO_DEFAULT_SAVE_PATH when question is provided", async () => { const MOCK_DEFAULT_PATH = "/default/save/path/for/question/test"; process.env.PEEKABOO_DEFAULT_SAVE_PATH = MOCK_DEFAULT_PATH; - const rmSpy = vi.spyOn(fs, "rm"); // Mock resolveImagePath to return temp directory when question is asked mockResolveImagePath.mockResolvedValue({ @@ -827,8 +892,8 @@ describe("Image Tool Integration Tests", () => { const result = await imageToolHandler({ question: "analyze this" }, mockContext); - // It should NOT have saved any files permanently - expect(result.saved_files).toEqual([]); + // It should now save files even with a question (files are preserved) + expect(result.saved_files).toEqual([{ path: MOCK_SAVED_FILE_PATH, mime_type: "image/png" }]); // The handler should not have used the default path // We can verify this by checking that the Swift CLI was called with the temp dir, not the default path @@ -836,12 +901,8 @@ describe("Image Tool Integration Tests", () => { expect.arrayContaining(["--path", MOCK_TEMP_DIR]), mockContext.logger ); - - // And the temp directory should have been cleaned up - expect(rmSpy).toHaveBeenCalledWith(MOCK_TEMP_DIR, { recursive: true, force: true }); delete process.env.PEEKABOO_DEFAULT_SAVE_PATH; - rmSpy.mockRestore(); }); }); diff --git a/tests/unit/tools/analyze.test.ts b/tests/unit/tools/analyze.test.ts index 59d35f0..c4f60cb 100644 --- a/tests/unit/tools/analyze.test.ts +++ b/tests/unit/tools/analyze.test.ts @@ -505,5 +505,66 @@ describe("Analyze Tool", () => { expect(result.isError).toBe(true); } }); + + it("should work with 'path' parameter as alias for 'image_path'", async () => { + process.env.PEEKABOO_AI_PROVIDERS = "ollama/llava"; + mockParseAIProviders.mockReturnValue([ + { provider: "ollama", model: "llava" }, + ]); + mockDetermineProviderAndModel.mockResolvedValue({ + provider: "ollama", + model: "llava", + }); + mockAnalyzeImageWithProvider.mockResolvedValue("Analysis complete"); + + const inputWithPath: AnalyzeToolInput = { + path: "/path/to/image.png", + question: "What is this?", + }; + + const result = await analyzeToolHandler(inputWithPath, mockContext); + + expect(mockReadImageAsBase64).toHaveBeenCalledWith("/path/to/image.png"); + expect(mockAnalyzeImageWithProvider).toHaveBeenCalledWith( + { provider: "ollama", model: "llava" }, + "/path/to/image.png", + MOCK_IMAGE_BASE64, + "What is this?", + mockLogger, + ); + expect(result.content[0].text).toBe("Analysis complete"); + expect(result.isError).toBeUndefined(); + }); + + it("should prioritize 'image_path' over 'path' when both are provided", async () => { + process.env.PEEKABOO_AI_PROVIDERS = "ollama/llava"; + mockParseAIProviders.mockReturnValue([ + { provider: "ollama", model: "llava" }, + ]); + mockDetermineProviderAndModel.mockResolvedValue({ + provider: "ollama", + model: "llava", + }); + mockAnalyzeImageWithProvider.mockResolvedValue("Analysis complete"); + + const inputWithBoth: AnalyzeToolInput = { + image_path: "/priority/image.png", + path: "/fallback/image.png", + question: "What is this?", + }; + + const result = await analyzeToolHandler(inputWithBoth, mockContext); + + expect(mockReadImageAsBase64).toHaveBeenCalledWith("/priority/image.png"); + expect(mockAnalyzeImageWithProvider).toHaveBeenCalledWith( + { provider: "ollama", model: "llava" }, + "/priority/image.png", + MOCK_IMAGE_BASE64, + "What is this?", + mockLogger, + ); + expect(result.content[0].text).toBe("Analysis complete"); + expect(result.isError).toBeUndefined(); + }); }); }); \ No newline at end of file diff --git a/tests/unit/tools/image.test.ts b/tests/unit/tools/image.test.ts index 38bbf18..ccabe8f 100644 --- a/tests/unit/tools/image.test.ts +++ b/tests/unit/tools/image.test.ts @@ -104,12 +104,12 @@ describe("Image Tool", () => { expect.objectContaining({ type: "image", data: "base64imagedata" }), ]), ); - expect(result.saved_files).toEqual([]); + expect(result.saved_files).toEqual(mockResponse.data.saved_files); expect(result.analysis_text).toBeUndefined(); expect(result.model_used).toBeUndefined(); - // Verify cleanup with fs.rm - expect(mockFsRm).toHaveBeenCalledWith(MOCK_TEMP_IMAGE_DIR, { recursive: true, force: true }); + // Verify no cleanup - files are preserved + expect(mockFsRm).not.toHaveBeenCalled(); }); it("should capture screen with format: 'data'", async () => { @@ -137,10 +137,10 @@ describe("Image Tool", () => { expect.objectContaining({ type: "image", data: "base64imagedata" }), ]), ); - expect(result.saved_files).toEqual([]); + expect(result.saved_files).toEqual(mockResponse.data.saved_files); - // Verify cleanup - expect(mockFsRm).toHaveBeenCalledWith(MOCK_TEMP_IMAGE_DIR, { recursive: true, force: true }); + // Verify no cleanup - files are preserved + expect(mockFsRm).not.toHaveBeenCalled(); }); it("should save file and return base64 when format: 'data' with path", async () => { @@ -420,7 +420,7 @@ describe("Image Tool", () => { process.env.PEEKABOO_AI_PROVIDERS = "ollama/llava:latest"; }); - it("should capture, analyze, and delete temp image if no path provided", async () => { + it("should capture, analyze, and PRESERVE temp image if no path provided", async () => { // Mock resolveImagePath to return temp directory when question is asked mockResolveImagePath.mockResolvedValue({ effectivePath: MOCK_TEMP_IMAGE_DIR, @@ -464,12 +464,13 @@ describe("Image Tool", () => { }), ]), ); - expect(result.saved_files).toEqual([]); + expect(result.saved_files).toEqual(mockCliResponse.data.saved_files); // No base64 in content when question is asked expect( result.content.some((item) => item.type === "image" && item.data), ).toBe(false); - expect(mockFsRm).toHaveBeenCalledWith(MOCK_TEMP_IMAGE_DIR, { recursive: true, force: true }); + // File is no longer removed even when no path provided + expect(mockFsRm).not.toHaveBeenCalled(); expect(result.isError).toBeUndefined(); }); @@ -540,7 +541,8 @@ describe("Image Tool", () => { ); expect(result.isError).toBe(true); expect(result.model_used).toBeUndefined(); - expect(mockFsRm).toHaveBeenCalledWith(MOCK_TEMP_IMAGE_DIR, { recursive: true, force: true }); + // File is no longer removed on analysis failure + expect(mockFsRm).not.toHaveBeenCalled(); }); it("should handle when AI analysis is not configured", async () => { @@ -697,8 +699,8 @@ describe("Image Tool", () => { "Analysis for Window 1:\nAnalysis for window 1.\n\nAnalysis for Window 2:\nAnalysis for window 2." ); - // Verify that the temporary directory is cleaned up - expect(mockFsRm).toHaveBeenCalledWith(MOCK_TEMP_IMAGE_DIR, { recursive: true, force: true }); + // Verify that the temporary directory is no longer cleaned up (files preserved) + expect(mockFsRm).not.toHaveBeenCalled(); }); });