diff --git a/src/tools/look-at/image-converter.test.ts b/src/tools/look-at/image-converter.test.ts index 37c9cdf2a..8acab97b6 100644 --- a/src/tools/look-at/image-converter.test.ts +++ b/src/tools/look-at/image-converter.test.ts @@ -1,11 +1,11 @@ import { describe, expect, test, mock, beforeEach } from "bun:test" import { existsSync, mkdtempSync, writeFileSync, unlinkSync, rmSync } from "node:fs" import { tmpdir } from "node:os" -import { join } from "node:path" +import { dirname, join } from "node:path" const originalChildProcess = await import("node:child_process") -const execFileSyncMock = mock((_command: string, _args: string[]) => "") +const execFileSyncMock = mock((_command: string, _args: string[], _options?: unknown) => "") const execSyncMock = mock(() => { throw new Error("execSync should not be called") }) @@ -16,7 +16,44 @@ mock.module("node:child_process", () => ({ execSync: execSyncMock, })) -const { convertImageToJpeg } = await import("./image-converter") +const { convertImageToJpeg, cleanupConvertedImage } = await import("./image-converter") + +function writeConvertedOutput(command: string, args: string[]): void { + if (command === "sips") { + const outIndex = args.indexOf("--out") + const outputPath = outIndex >= 0 ? args[outIndex + 1] : undefined + if (outputPath) { + writeFileSync(outputPath, "jpeg") + } + return + } + + if (command === "convert") { + writeFileSync(args[2], "jpeg") + return + } + + if (command === "magick") { + writeFileSync(args[2], "jpeg") + } +} + +function withMockPlatform(platform: NodeJS.Platform, run: () => TValue): TValue { + const originalPlatform = process.platform + Object.defineProperty(process, "platform", { + value: platform, + configurable: true, + }) + + try { + return run() + } finally { + Object.defineProperty(process, "platform", { + value: originalPlatform, + configurable: true, + }) + } +} describe("image-converter command execution safety", () => { beforeEach(() => { @@ -30,13 +67,7 @@ describe("image-converter command execution safety", () => { writeFileSync(inputPath, "fake-heic-data") execFileSyncMock.mockImplementation((command: string, args: string[]) => { - if (command === "sips") { - const outIndex = args.indexOf("--out") - const outputPath = outIndex >= 0 ? args[outIndex + 1] : undefined - if (outputPath) writeFileSync(outputPath, "jpeg") - } else if (command === "convert") { - writeFileSync(args[1], "jpeg") - } + writeConvertedOutput(command, args) return "" }) @@ -48,7 +79,10 @@ describe("image-converter command execution safety", () => { const [firstCommand, firstArgs] = execFileSyncMock.mock.calls[0] as [string, string[]] expect(typeof firstCommand).toBe("string") expect(Array.isArray(firstArgs)).toBe(true) + expect(["sips", "convert", "magick"]).toContain(firstCommand) + expect(firstArgs).toContain("--") expect(firstArgs).toContain(inputPath) + expect(firstArgs.indexOf("--") < firstArgs.indexOf(inputPath)).toBe(true) expect(firstArgs.join(" ")).not.toContain(`\"${inputPath}\"`) expect(existsSync(outputPath)).toBe(true) @@ -57,4 +91,102 @@ describe("image-converter command execution safety", () => { if (existsSync(inputPath)) unlinkSync(inputPath) rmSync(testDir, { recursive: true, force: true }) }) + + test("removes temporary conversion directory during cleanup", () => { + const testDir = mkdtempSync(join(tmpdir(), "img-converter-cleanup-test-")) + const inputPath = join(testDir, "photo.heic") + writeFileSync(inputPath, "fake-heic-data") + + execFileSyncMock.mockImplementation((command: string, args: string[]) => { + writeConvertedOutput(command, args) + return "" + }) + + const outputPath = convertImageToJpeg(inputPath, "image/heic") + const conversionDirectory = dirname(outputPath) + + expect(existsSync(conversionDirectory)).toBe(true) + + cleanupConvertedImage(outputPath) + + expect(existsSync(conversionDirectory)).toBe(false) + + if (existsSync(inputPath)) unlinkSync(inputPath) + rmSync(testDir, { recursive: true, force: true }) + }) + + test("uses magick command on non-darwin platforms to avoid convert.exe collision", () => { + withMockPlatform("linux", () => { + const testDir = mkdtempSync(join(tmpdir(), "img-converter-platform-test-")) + const inputPath = join(testDir, "photo.heic") + writeFileSync(inputPath, "fake-heic-data") + + execFileSyncMock.mockImplementation((command: string, args: string[]) => { + if (command === "magick") { + writeFileSync(args[2], "jpeg") + } + return "" + }) + + const outputPath = convertImageToJpeg(inputPath, "image/heic") + + const [command, args] = execFileSyncMock.mock.calls[0] as [string, string[]] + expect(command).toBe("magick") + expect(args).toContain("--") + expect(args.indexOf("--") < args.indexOf(inputPath)).toBe(true) + expect(existsSync(outputPath)).toBe(true) + + cleanupConvertedImage(outputPath) + if (existsSync(inputPath)) unlinkSync(inputPath) + rmSync(testDir, { recursive: true, force: true }) + }) + }) + + test("applies timeout when executing conversion commands", () => { + const testDir = mkdtempSync(join(tmpdir(), "img-converter-timeout-test-")) + const inputPath = join(testDir, "photo.heic") + writeFileSync(inputPath, "fake-heic-data") + + execFileSyncMock.mockImplementation((command: string, args: string[]) => { + writeConvertedOutput(command, args) + return "" + }) + + const outputPath = convertImageToJpeg(inputPath, "image/heic") + + const options = execFileSyncMock.mock.calls[0]?.[2] as { timeout?: number } | undefined + expect(options).toBeDefined() + expect(typeof options?.timeout).toBe("number") + expect((options?.timeout ?? 0) > 0).toBe(true) + + cleanupConvertedImage(outputPath) + if (existsSync(inputPath)) unlinkSync(inputPath) + rmSync(testDir, { recursive: true, force: true }) + }) + + test("attaches temporary output path to conversion errors", () => { + withMockPlatform("linux", () => { + const testDir = mkdtempSync(join(tmpdir(), "img-converter-failure-test-")) + const inputPath = join(testDir, "photo.heic") + writeFileSync(inputPath, "fake-heic-data") + + execFileSyncMock.mockImplementation(() => { + throw new Error("conversion process failed") + }) + + const runConversion = () => convertImageToJpeg(inputPath, "image/heic") + expect(runConversion).toThrow("No image conversion tool available") + + try { + runConversion() + } catch (error) { + const conversionError = error as Error & { temporaryOutputPath?: string } + expect(conversionError.temporaryOutputPath).toBeDefined() + expect(conversionError.temporaryOutputPath?.endsWith("converted.jpg")).toBe(true) + } + + if (existsSync(inputPath)) unlinkSync(inputPath) + rmSync(testDir, { recursive: true, force: true }) + }) + }) }) diff --git a/src/tools/look-at/image-converter.ts b/src/tools/look-at/image-converter.ts index e95237ba3..fe3b6be05 100644 --- a/src/tools/look-at/image-converter.ts +++ b/src/tools/look-at/image-converter.ts @@ -1,7 +1,7 @@ import { execFileSync } from "node:child_process" -import { existsSync, mkdtempSync, unlinkSync, writeFileSync, readFileSync } from "node:fs" +import { existsSync, mkdtempSync, readFileSync, rmSync, unlinkSync, writeFileSync } from "node:fs" import { tmpdir } from "node:os" -import { join } from "node:path" +import { dirname, join } from "node:path" import { log } from "../../shared" const SUPPORTED_FORMATS = new Set([ @@ -32,6 +32,8 @@ const UNSUPPORTED_FORMATS = new Set([ "image/x-photoshop", ]) +const CONVERSION_TIMEOUT_MS = 30_000 + export function needsConversion(mimeType: string): boolean { if (SUPPORTED_FORMATS.has(mimeType)) { return false @@ -57,9 +59,10 @@ export function convertImageToJpeg(inputPath: string, mimeType: string): string try { if (process.platform === "darwin") { try { - execFileSync("sips", ["-s", "format", "jpeg", inputPath, "--out", outputPath], { + execFileSync("sips", ["-s", "format", "jpeg", "--", inputPath, "--out", outputPath], { stdio: "pipe", encoding: "utf-8", + timeout: CONVERSION_TIMEOUT_MS, }) if (existsSync(outputPath)) { @@ -72,9 +75,11 @@ export function convertImageToJpeg(inputPath: string, mimeType: string): string } try { - execFileSync("convert", [inputPath, outputPath], { + const imagemagickCommand = process.platform === "darwin" ? "convert" : "magick" + execFileSync(imagemagickCommand, ["--", inputPath, outputPath], { stdio: "pipe", encoding: "utf-8", + timeout: CONVERSION_TIMEOUT_MS, }) if (existsSync(outputPath)) { @@ -97,6 +102,11 @@ export function convertImageToJpeg(inputPath: string, mimeType: string): string unlinkSync(outputPath) } } catch {} + + if (error instanceof Error) { + const conversionError = error as Error & { temporaryOutputPath?: string } + conversionError.temporaryOutputPath = outputPath + } throw error } @@ -104,10 +114,15 @@ export function convertImageToJpeg(inputPath: string, mimeType: string): string export function cleanupConvertedImage(filePath: string): void { try { + const tempDirectory = dirname(filePath) if (existsSync(filePath)) { unlinkSync(filePath) log(`[image-converter] Cleaned up temporary file: ${filePath}`) } + if (existsSync(tempDirectory)) { + rmSync(tempDirectory, { recursive: true, force: true }) + log(`[image-converter] Cleaned up temporary directory: ${tempDirectory}`) + } } catch (error) { log(`[image-converter] Failed to cleanup ${filePath}: ${error}`) } diff --git a/src/tools/look-at/tools.ts b/src/tools/look-at/tools.ts index a4c67f051..423b1f91a 100644 --- a/src/tools/look-at/tools.ts +++ b/src/tools/look-at/tools.ts @@ -20,6 +20,24 @@ import { cleanupConvertedImage, } from "./image-converter" +function getTemporaryConversionPath(error: unknown): string | null { + if (!(error instanceof Error)) { + return null + } + + const temporaryOutputPath = Reflect.get(error, "temporaryOutputPath") + if (typeof temporaryOutputPath === "string" && temporaryOutputPath.length > 0) { + return temporaryOutputPath + } + + const temporaryDirectory = Reflect.get(error, "temporaryDirectory") + if (typeof temporaryDirectory === "string" && temporaryDirectory.length > 0) { + return temporaryDirectory + } + + return null +} + export { normalizeArgs, validateArgs } from "./look-at-arguments" export function createLookAt(ctx: PluginInput): ToolDefinition { @@ -48,6 +66,7 @@ export function createLookAt(ctx: PluginInput): ToolDefinition { let mimeType: string let filePart: { type: "file"; mime: string; url: string; filename: string } let tempFilePath: string | null = null + let tempConversionPath: string | null = null let tempFilesToCleanup: string[] = [] try { @@ -85,10 +104,15 @@ export function createLookAt(ctx: PluginInput): ToolDefinition { log(`[look_at] Detected unsupported format: ${mimeType}, converting to JPEG...`) try { tempFilePath = convertImageToJpeg(filePath, mimeType) + tempConversionPath = tempFilePath actualFilePath = tempFilePath mimeType = "image/jpeg" log(`[look_at] Conversion successful: ${tempFilePath}`) } catch (conversionError) { + const failedConversionPath = getTemporaryConversionPath(conversionError) + if (failedConversionPath) { + tempConversionPath = failedConversionPath + } log(`[look_at] Conversion failed: ${conversionError}`) return `Error: Failed to convert image format. ${conversionError}` } @@ -194,10 +218,14 @@ Original error: ${createResult.error}` log(`[look_at] Got response, length: ${responseText.length}`) return responseText } finally { - if (tempFilePath) { + if (tempConversionPath) { + cleanupConvertedImage(tempConversionPath) + } else if (tempFilePath) { cleanupConvertedImage(tempFilePath) } - tempFilesToCleanup.forEach(file => cleanupConvertedImage(file)) + tempFilesToCleanup.forEach(file => { + cleanupConvertedImage(file) + }) } }, })