From b753edbc73c01de20acc89f0cac3ac9193a47e7e Mon Sep 17 00:00:00 2001 From: Gustavo Madeira Santana Date: Mon, 2 Mar 2026 02:15:55 -0500 Subject: [PATCH] diffs: remove standalone path footguns and add format passthrough regression --- extensions/diffs/src/store.test.ts | 17 ++--- extensions/diffs/src/store.ts | 89 +++++++++++++++------------ src/gateway/tools-invoke-http.test.ts | 40 ++++++++++++ 3 files changed, 98 insertions(+), 48 deletions(-) diff --git a/extensions/diffs/src/store.test.ts b/extensions/diffs/src/store.test.ts index 09850855ea7..d4e6aacd409 100644 --- a/extensions/diffs/src/store.test.ts +++ b/extensions/diffs/src/store.test.ts @@ -92,10 +92,11 @@ describe("DiffArtifactStore", () => { await expect(store.readHtml(artifact.id)).rejects.toThrow("escapes store root"); }); - it("allocates standalone file paths outside artifact metadata", async () => { - const filePath = store.allocateStandaloneFilePath(); - expect(filePath).toMatch(/preview\.png$/); - expect(filePath).toContain(rootDir); + it("creates standalone file artifacts with managed metadata", async () => { + const standalone = await store.createStandaloneFileArtifact(); + expect(standalone.filePath).toMatch(/preview\.png$/); + expect(standalone.filePath).toContain(rootDir); + expect(Date.parse(standalone.expiresAt)).toBeGreaterThan(Date.now()); }); it("expires standalone file artifacts using ttl metadata", async () => { @@ -127,8 +128,8 @@ describe("DiffArtifactStore", () => { const imagePath = store.allocateImagePath(artifact.id, "pdf"); expect(imagePath).toMatch(/preview\.pdf$/); - const standaloneImagePath = store.allocateStandaloneImagePath(); - expect(standaloneImagePath).toMatch(/preview\.png$/); + const standalone = await store.createStandaloneFileArtifact(); + expect(standalone.filePath).toMatch(/preview\.png$/); const updated = await store.updateImagePath(artifact.id, imagePath); expect(updated.filePath).toBe(imagePath); @@ -144,9 +145,9 @@ describe("DiffArtifactStore", () => { }); const artifactPdf = store.allocateFilePath(artifact.id, "pdf"); - const standalonePdf = store.allocateStandaloneFilePath("pdf"); + const standalonePdf = await store.createStandaloneFileArtifact({ format: "pdf" }); expect(artifactPdf).toMatch(/preview\.pdf$/); - expect(standalonePdf).toMatch(/preview\.pdf$/); + expect(standalonePdf.filePath).toMatch(/preview\.pdf$/); }); it("throttles cleanup sweeps across repeated artifact creation", async () => { diff --git a/extensions/diffs/src/store.ts b/extensions/diffs/src/store.ts index f529096d9dc..3e646188721 100644 --- a/extensions/diffs/src/store.ts +++ b/extensions/diffs/src/store.ts @@ -31,6 +31,8 @@ type StandaloneFileMeta = { filePath: string; }; +type ArtifactMetaFileName = "meta.json" | "file-meta.json"; + export class DiffArtifactStore { private readonly rootDir: string; private readonly logger?: PluginLogger; @@ -123,11 +125,6 @@ export class DiffArtifactStore { return path.join(this.artifactDir(id), `preview.${format}`); } - allocateStandaloneFilePath(format: DiffOutputFormat = "png"): string { - const id = crypto.randomBytes(10).toString("hex"); - return path.join(this.artifactDir(id), `preview.${format}`); - } - async createStandaloneFileArtifact( params: CreateStandaloneFileArtifactParams = {}, ): Promise<{ id: string; filePath: string; expiresAt: string }> { @@ -162,10 +159,6 @@ export class DiffArtifactStore { return this.allocateFilePath(id, format); } - allocateStandaloneImagePath(format: DiffOutputFormat = "png"): string { - return this.allocateStandaloneFilePath(format); - } - scheduleCleanup(): void { this.maybeCleanupExpired(); } @@ -237,60 +230,76 @@ export class DiffArtifactStore { return this.resolveWithinRoot(id); } - private metaPath(id: string): string { - return path.join(this.artifactDir(id), "meta.json"); - } - - private standaloneMetaPath(id: string): string { - return path.join(this.artifactDir(id), "file-meta.json"); - } - private async writeMeta(meta: DiffArtifactMeta): Promise { - await fs.writeFile(this.metaPath(meta.id), JSON.stringify(meta, null, 2), "utf8"); + await this.writeJsonMeta(meta.id, "meta.json", meta); } private async readMeta(id: string): Promise { - try { - const raw = await fs.readFile(this.metaPath(id), "utf8"); - return JSON.parse(raw) as DiffArtifactMeta; - } catch (error) { - if (isFileNotFound(error)) { - return null; - } - this.logger?.warn(`Failed to read diff artifact metadata for ${id}: ${String(error)}`); + const parsed = await this.readJsonMeta(id, "meta.json", "diff artifact"); + if (!parsed) { return null; } + return parsed as DiffArtifactMeta; } private async writeStandaloneMeta(meta: StandaloneFileMeta): Promise { - await fs.writeFile(this.standaloneMetaPath(meta.id), JSON.stringify(meta, null, 2), "utf8"); + await this.writeJsonMeta(meta.id, "file-meta.json", meta); } private async readStandaloneMeta(id: string): Promise { + const parsed = await this.readJsonMeta(id, "file-meta.json", "standalone diff"); + if (!parsed) { + return null; + } try { - const raw = await fs.readFile(this.standaloneMetaPath(id), "utf8"); - const parsed = JSON.parse(raw) as Partial; + const value = parsed as Partial; if ( - parsed.kind !== "standalone_file" || - typeof parsed.id !== "string" || - typeof parsed.createdAt !== "string" || - typeof parsed.expiresAt !== "string" || - typeof parsed.filePath !== "string" + value.kind !== "standalone_file" || + typeof value.id !== "string" || + typeof value.createdAt !== "string" || + typeof value.expiresAt !== "string" || + typeof value.filePath !== "string" ) { return null; } return { - kind: parsed.kind, - id: parsed.id, - createdAt: parsed.createdAt, - expiresAt: parsed.expiresAt, - filePath: this.normalizeStoredPath(parsed.filePath, "filePath"), + kind: value.kind, + id: value.id, + createdAt: value.createdAt, + expiresAt: value.expiresAt, + filePath: this.normalizeStoredPath(value.filePath, "filePath"), }; + } catch (error) { + this.logger?.warn(`Failed to normalize standalone diff metadata for ${id}: ${String(error)}`); + return null; + } + } + + private metaFilePath(id: string, fileName: ArtifactMetaFileName): string { + return path.join(this.artifactDir(id), fileName); + } + + private async writeJsonMeta( + id: string, + fileName: ArtifactMetaFileName, + data: unknown, + ): Promise { + await fs.writeFile(this.metaFilePath(id, fileName), JSON.stringify(data, null, 2), "utf8"); + } + + private async readJsonMeta( + id: string, + fileName: ArtifactMetaFileName, + context: string, + ): Promise { + try { + const raw = await fs.readFile(this.metaFilePath(id, fileName), "utf8"); + return JSON.parse(raw) as unknown; } catch (error) { if (isFileNotFound(error)) { return null; } - this.logger?.warn(`Failed to read standalone diff metadata for ${id}: ${String(error)}`); + this.logger?.warn(`Failed to read ${context} metadata for ${id}: ${String(error)}`); return null; } } diff --git a/src/gateway/tools-invoke-http.test.ts b/src/gateway/tools-invoke-http.test.ts index f87f00593a0..335cab6454d 100644 --- a/src/gateway/tools-invoke-http.test.ts +++ b/src/gateway/tools-invoke-http.test.ts @@ -123,6 +123,25 @@ vi.mock("../agents/openclaw-tools.js", () => { return { ok: true }; }, }, + { + name: "diffs_compat_test", + parameters: { + type: "object", + properties: { + mode: { type: "string" }, + fileFormat: { type: "string" }, + }, + additionalProperties: false, + }, + execute: async (_toolCallId: string, args: unknown) => { + const input = (args ?? {}) as Record; + return { + ok: true, + observedFormat: input.format, + observedFileFormat: input.fileFormat, + }; + }, + }, ]; return { @@ -546,4 +565,25 @@ describe("POST /tools/invoke", () => { expect(crashBody.error?.type).toBe("tool_error"); expect(crashBody.error?.message).toBe("tool execution failed"); }); + + it("passes deprecated format alias through invoke payloads even when schema omits it", async () => { + cfg = { + ...cfg, + agents: { + list: [{ id: "main", default: true, tools: { allow: ["diffs_compat_test"] } }], + }, + }; + + const res = await invokeToolAuthed({ + tool: "diffs_compat_test", + args: { mode: "file", format: "pdf" }, + sessionKey: "main", + }); + + expect(res.status).toBe(200); + const body = await res.json(); + expect(body.ok).toBe(true); + expect(body.result?.observedFormat).toBe("pdf"); + expect(body.result?.observedFileFormat).toBeUndefined(); + }); });