diff --git a/CHANGELOG.md b/CHANGELOG.md index 670e47926262..c7587e99e9ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- **Diffs rendering:** render viewer and image output from one SSR preload, preserve language-pack highlighting through hydration, normalize language hints case-insensitively, skip identical before/after inputs with an explicit `changed` result, report truthful file-render and input errors, cache hash-pinned viewer runtimes, and prefer canonical file settings over stale aliases. (#100487) - **Remote browser reliability:** bound persistent Playwright tab enumeration by the existing remote CDP timeout budget and retire timed-out connection attempts so late completions cannot restore a stuck connection. (#80147, #58968) Thanks @HemantSudarshan and @KeaneYan. - **Control UI approval prompts:** keep stale resolve failures and busy-state cleanup from leaking across newer approvals or Gateway reconnects. (#98394) Thanks @haruaiclone-droid. - **Agent empty replies:** surface a visible failure when a completed interactive turn has no deliverable reply, including queued follow-ups, while preserving explicit silence, pending continuations, and committed side effects. (#100456) Thanks @mushuiyu886. diff --git a/docs/tools/diffs.md b/docs/tools/diffs.md index 66ecaca624ad..5f12b0a84f80 100644 --- a/docs/tools/diffs.md +++ b/docs/tools/diffs.md @@ -171,8 +171,11 @@ Without the pack, unsupported languages still render as readable plain text. See ## Output details contract +All successful results include `changed`: identical before/after input returns `false` without creating an artifact; rendered results return `true`. + + - `changed` - `artifactId` - `viewerUrl` - `viewerPath` @@ -185,6 +188,7 @@ Without the pack, unsupported languages still render as readable plain text. See + - `changed` - `artifactId` - `expiresAt` - `filePath` diff --git a/extensions/diffs-language-pack/src/plugin.ts b/extensions/diffs-language-pack/src/plugin.ts index c3405b54640b..e9c295974039 100644 --- a/extensions/diffs-language-pack/src/plugin.ts +++ b/extensions/diffs-language-pack/src/plugin.ts @@ -1,7 +1,9 @@ // Diffs Language Pack plugin module implements plugin behavior. import type { IncomingMessage, ServerResponse } from "node:http"; import type { OpenClawPluginApi } from "../api.js"; -import { VIEWER_ASSET_PREFIX, getServedViewerAsset } from "./viewer-assets.js"; +import { VIEWER_ASSET_PREFIX, VIEWER_RUNTIME_PATH, getServedViewerAsset } from "./viewer-assets.js"; + +const IMMUTABLE_ASSET_CACHE_CONTROL = "public, max-age=31536000, immutable"; export function registerDiffsLanguagePackPlugin(api: OpenClawPluginApi): void { api.registerHttpRoute({ @@ -30,7 +32,11 @@ function createDiffsLanguagePackHttpHandler() { } res.statusCode = 200; - setSharedHeaders(res, asset.contentType); + setSharedHeaders( + res, + asset.contentType, + parsed.pathname === VIEWER_RUNTIME_PATH ? IMMUTABLE_ASSET_CACHE_CONTROL : undefined, + ); if (req.method === "HEAD") { res.end(); } else { @@ -57,8 +63,12 @@ function respondText(res: ServerResponse, statusCode: number, body: string): voi res.end(body); } -function setSharedHeaders(res: ServerResponse, contentType: string): void { - res.setHeader("cache-control", "no-store, max-age=0"); +function setSharedHeaders( + res: ServerResponse, + contentType: string, + cacheControl = "no-store, max-age=0", +): void { + res.setHeader("cache-control", cacheControl); res.setHeader("content-type", contentType); res.setHeader("x-content-type-options", "nosniff"); res.setHeader("referrer-policy", "no-referrer"); diff --git a/extensions/diffs/README.md b/extensions/diffs/README.md index 84d76f7d3bbd..fdd80b32ff3e 100644 --- a/extensions/diffs/README.md +++ b/extensions/diffs/README.md @@ -20,6 +20,7 @@ It gives agents one tool, `diffs`, that can: The tool can return: +- `details.changed`: `false` when before/after inputs are identical and no artifact was rendered; `true` for rendered results - `details.viewerUrl`: a gateway URL that can be opened in the canvas - `details.filePath`: a local rendered artifact path when file rendering is requested - `details.fileFormat`: the rendered file format (`png` or `pdf`) diff --git a/extensions/diffs/openclaw.plugin.json b/extensions/diffs/openclaw.plugin.json index 277b2585cc40..a8b6eb4582af 100644 --- a/extensions/diffs/openclaw.plugin.json +++ b/extensions/diffs/openclaw.plugin.json @@ -145,8 +145,7 @@ }, "fileFormat": { "type": "string", - "enum": ["png", "pdf"], - "default": "png" + "enum": ["png", "pdf"] }, "format": { "type": "string", @@ -155,8 +154,7 @@ }, "fileQuality": { "type": "string", - "enum": ["standard", "hq", "print"], - "default": "standard" + "enum": ["standard", "hq", "print"] }, "fileScale": { "type": "number", diff --git a/extensions/diffs/skills/diffs/SKILL.md b/extensions/diffs/skills/diffs/SKILL.md index b049bd901c55..abf4802d98db 100644 --- a/extensions/diffs/skills/diffs/SKILL.md +++ b/extensions/diffs/skills/diffs/SKILL.md @@ -7,6 +7,8 @@ When you need to show edits as a real diff, prefer the `diffs` tool instead of w The `diffs` tool accepts either `before` + `after` text, or a unified `patch` string. +Check `details.changed`: identical before/after input returns `false` without creating an artifact; rendered results return `true`. + Use `mode=view` when you want an interactive gateway-hosted viewer. After the tool returns, use `details.viewerUrl` with the canvas tool via `canvas present` or `canvas navigate`. If the deployment uses a loopback trusted proxy (for example Tailscale Serve with `gateway.trustedProxies` including `127.0.0.1`), raw loopback viewer requests can fail closed without forwarded client-IP headers. In that topology, prefer `mode=file` / `mode=both`, or use a configured `viewerBaseUrl` / explicit proxy/public `baseUrl` when you need a shareable viewer URL. diff --git a/extensions/diffs/src/browser.test.ts b/extensions/diffs/src/browser.test.ts index ba1d9534728f..2b33dc54e9da 100644 --- a/extensions/diffs/src/browser.test.ts +++ b/extensions/diffs/src/browser.test.ts @@ -208,6 +208,81 @@ describe("PlaywrightDiffScreenshotter", () => { expect(pages).toHaveLength(1); expect(pages[0]?.screenshot).toHaveBeenCalledTimes(0); }); + + it("wraps browser launch failures with Chromium installation guidance", async () => { + launchMock.mockRejectedValue(new Error("launch failed")); + const screenshotter = new PlaywrightDiffScreenshotter({ + config: createConfig(), + browserIdleMs: 1_000, + }); + + await expect( + screenshotter.screenshotHtml({ + html: '
', + outputPath, + theme: "dark", + image: { + format: "png", + qualityPreset: "standard", + scale: 2, + maxWidth: 960, + maxPixels: 8_000_000, + }, + }), + ).rejects.toThrow("requires a Chromium-compatible browser"); + }); + + it("wraps new-page failures with Chromium installation guidance", async () => { + const browser = createMockBrowser([]); + browser.newPage.mockRejectedValue(new Error("page creation failed")); + launchMock.mockResolvedValue(browser); + const screenshotter = new PlaywrightDiffScreenshotter({ + config: createConfig(), + browserIdleMs: 1_000, + }); + + await expect( + screenshotter.screenshotHtml({ + html: '
', + outputPath, + theme: "dark", + image: { + format: "png", + qualityPreset: "standard", + scale: 2, + maxWidth: 960, + maxPixels: 8_000_000, + }, + }), + ).rejects.toThrow("requires a Chromium-compatible browser"); + }); + + it("preserves render errors after a browser page has opened", async () => { + const browser = createMockBrowser([]); + const page = createMockPage(); + page.waitForFunction.mockRejectedValue(new Error("hydration timeout")); + browser.newPage.mockResolvedValue(page); + launchMock.mockResolvedValue(browser); + const screenshotter = new PlaywrightDiffScreenshotter({ + config: createConfig(), + browserIdleMs: 1_000, + }); + + await expect( + screenshotter.screenshotHtml({ + html: '
', + outputPath, + theme: "dark", + image: { + format: "png", + qualityPreset: "standard", + scale: 2, + maxWidth: 960, + maxPixels: 8_000_000, + }, + }), + ).rejects.toThrow("hydration timeout"); + }); }); describe("diffs plugin registration", () => { @@ -428,6 +503,7 @@ describe("diffs plugin registration", () => { [ "When you need to show edits as a real diff, prefer the `diffs` tool instead of writing a manual summary.", "It accepts either `before` + `after` text or a unified `patch`.", + "Check `details.changed`: identical before/after input returns `false` without creating an artifact; rendered results return `true`.", "`mode=view` returns `details.viewerUrl` for canvas use; `mode=file` returns `details.filePath`; `mode=both` returns both.", "If you need to send the rendered file, use the `message` tool with `path` or `filePath`.", "Include `path` when you know the filename, and omit presentation overrides unless needed.", diff --git a/extensions/diffs/src/browser.ts b/extensions/diffs/src/browser.ts index b04a75d5724a..0891afd76d41 100644 --- a/extensions/diffs/src/browser.ts +++ b/extensions/diffs/src/browser.ts @@ -68,24 +68,33 @@ export class PlaywrightDiffScreenshotter implements DiffScreenshotter { theme: DiffTheme; image: DiffRenderOptions["image"]; }): Promise { - const lease = await acquireSharedBrowser({ - config: this.config, - idleMs: this.browserIdleMs, - }); + let lease: BrowserLease; + try { + lease = await acquireSharedBrowser({ + config: this.config, + idleMs: this.browserIdleMs, + }); + } catch (error) { + throw buildBrowserUnavailableError(error); + } let page: Awaited> | undefined; let currentScale = params.image.scale; const maxRetries = 2; try { for (let attempt = 0; attempt <= maxRetries; attempt += 1) { - page = await lease.browser.newPage({ - viewport: { - width: Math.max(Math.ceil(params.image.maxWidth + 240), 1200), - height: 900, - }, - deviceScaleFactor: currentScale, - colorScheme: params.theme, - }); + try { + page = await lease.browser.newPage({ + viewport: { + width: Math.max(Math.ceil(params.image.maxWidth + 240), 1200), + height: 900, + }, + deviceScaleFactor: currentScale, + colorScheme: params.theme, + }); + } catch (error) { + throw buildBrowserUnavailableError(error); + } await page.route("**/*", async (route) => { const requestUrl = route.request().url(); if (requestUrl === "about:blank" || requestUrl.startsWith("data:")) { @@ -276,15 +285,6 @@ export class PlaywrightDiffScreenshotter implements DiffScreenshotter { return params.outputPath; } throw new Error(IMAGE_SIZE_LIMIT_ERROR); - } catch (error) { - if (error instanceof Error && error.message === IMAGE_SIZE_LIMIT_ERROR) { - throw error; - } - const reason = formatErrorMessage(error); - throw new Error( - `Diff PNG/PDF rendering requires a Chromium-compatible browser. Set browser.executablePath or install Chrome/Chromium. ${reason}`, - { cause: error }, - ); } finally { await page?.close().catch(() => {}); await lease.release(); @@ -292,6 +292,14 @@ export class PlaywrightDiffScreenshotter implements DiffScreenshotter { } } +function buildBrowserUnavailableError(error: unknown): Error { + const reason = formatErrorMessage(error); + return new Error( + `Diff PNG/PDF rendering requires a Chromium-compatible browser. Set browser.executablePath or install Chrome/Chromium. ${reason}`, + { cause: error }, + ); +} + async function writeExternalArtifactFile(params: { outputPath: string; write: (tempPath: string) => Promise; @@ -449,6 +457,7 @@ function scheduleIdleBrowserClose(state: SharedBrowserState, idleMs: number): vo void closeSharedBrowser(); } }, idleMs); + state.idleTimer.unref(); } function clearIdleTimer(state: SharedBrowserState): void { diff --git a/extensions/diffs/src/config.test.ts b/extensions/diffs/src/config.test.ts index a9a756a5ed94..3dbaf619067a 100644 --- a/extensions/diffs/src/config.test.ts +++ b/extensions/diffs/src/config.test.ts @@ -62,7 +62,7 @@ function compileManifestConfigSchema() { schema: manifest.configSchema, value, applyDefaults: true, - }).ok; + }); } function requireRecord(value: unknown, label: string): Record { @@ -216,6 +216,20 @@ describe("resolveDiffsPluginDefaults", () => { ); }); + it("prefers an explicit canonical default value over a deprecated alias", () => { + expectFields( + resolveDiffsPluginDefaults({ + defaults: { + fileFormat: "png", + imageFormat: "pdf", + }, + }), + { + fileFormat: "png", + }, + ); + }); + it("accepts plugin-wide artifact TTL defaults", () => { expectFields( resolveDiffsPluginDefaults({ @@ -240,7 +254,7 @@ describe("resolveDiffsPluginDefaults", () => { ); }); - it("keeps loader-applied schema defaults from shadowing aliases and quality-derived defaults", () => { + it("keeps alias-only config values after manifest validation", () => { const validate = compileManifestConfigSchema(); const aliasOnly = { @@ -249,8 +263,11 @@ describe("resolveDiffsPluginDefaults", () => { imageQuality: "hq", }, }; - expect(validate(aliasOnly)).toBe(true); - expectFields(resolveDiffsPluginDefaults(aliasOnly), { + const validatedAliasOnly = validate(aliasOnly); + if (!validatedAliasOnly.ok) { + throw new Error("Expected alias-only config to pass manifest validation."); + } + expectFields(resolveDiffsPluginDefaults(validatedAliasOnly.value), { fileFormat: "pdf", fileQuality: "hq", fileScale: 2.5, @@ -262,8 +279,11 @@ describe("resolveDiffsPluginDefaults", () => { fileQuality: "hq", }, }; - expect(validate(qualityOnly)).toBe(true); - expectFields(resolveDiffsPluginDefaults(qualityOnly), { + const validatedQualityOnly = validate(qualityOnly); + if (!validatedQualityOnly.ok) { + throw new Error("Expected quality-only config to pass manifest validation."); + } + expectFields(resolveDiffsPluginDefaults(validatedQualityOnly.value), { fileQuality: "hq", fileScale: 2.5, fileMaxWidth: 1200, @@ -301,10 +321,10 @@ describe("diffs plugin schema surfaces", () => { it("rejects invalid viewerBaseUrl values at manifest-validation time too", () => { const validate = compileManifestConfigSchema(); - expect(validate({ viewerBaseUrl: "javascript:alert(1)" })).toBe(false); - expect(validate({ viewerBaseUrl: "https://example.com/openclaw?x=1" })).toBe(false); - expect(validate({ viewerBaseUrl: "https://example.com/openclaw#frag" })).toBe(false); - expect(validate({ viewerBaseUrl: "https://example.com/openclaw/" })).toBe(true); + expect(validate({ viewerBaseUrl: "javascript:alert(1)" }).ok).toBe(false); + expect(validate({ viewerBaseUrl: "https://example.com/openclaw?x=1" }).ok).toBe(false); + expect(validate({ viewerBaseUrl: "https://example.com/openclaw#frag" }).ok).toBe(false); + expect(validate({ viewerBaseUrl: "https://example.com/openclaw/" }).ok).toBe(true); }); it("preserves defaults and security for direct safeParse callers", () => { @@ -344,7 +364,7 @@ describe("diffs plugin schema surfaces", () => { expectFields(data.security, { allowRemoteViewer: true }); }); - it("canonicalizes alias-driven defaults for direct safeParse callers", () => { + it("resolves deprecated aliases before safeParse applies runtime defaults", () => { const parsed = requireRecord( diffsPluginConfigSchema.safeParse?.({ defaults: { diff --git a/extensions/diffs/src/config.ts b/extensions/diffs/src/config.ts index 0b0efec768a3..2e8b481257f8 100644 --- a/extensions/diffs/src/config.ts +++ b/extensions/diffs/src/config.ts @@ -144,15 +144,9 @@ const DiffsPluginJsonSchemaSource = z.strictObject({ wordWrap: z.boolean().default(DEFAULT_DIFFS_TOOL_DEFAULTS.wordWrap).optional(), background: z.boolean().default(DEFAULT_DIFFS_TOOL_DEFAULTS.background).optional(), theme: z.enum(DIFF_THEMES).default(DEFAULT_DIFFS_TOOL_DEFAULTS.theme).optional(), - fileFormat: z - .enum(DIFF_OUTPUT_FORMATS) - .default(DEFAULT_DIFFS_TOOL_DEFAULTS.fileFormat) - .optional(), + fileFormat: z.enum(DIFF_OUTPUT_FORMATS).optional(), format: z.enum(DIFF_OUTPUT_FORMATS).optional().describe("Deprecated alias for fileFormat."), - fileQuality: z - .enum(DIFF_IMAGE_QUALITY_PRESETS) - .default(DEFAULT_DIFFS_TOOL_DEFAULTS.fileQuality) - .optional(), + fileQuality: z.enum(DIFF_IMAGE_QUALITY_PRESETS).optional(), fileScale: z.number().min(1).max(4).optional(), fileMaxWidth: z.number().min(640).max(2400).optional(), imageFormat: z @@ -225,12 +219,8 @@ export const diffsPluginConfigSchema: OpenClawPluginConfigSchema = { function resolveConfiguredValue(options: { primary: T | undefined; aliases: Array; - schemaDefault?: T; }): T | undefined { const alias = options.aliases.find((value): value is T => value !== undefined); - if (alias !== undefined && options.primary === options.schemaDefault) { - return alias; - } return options.primary ?? alias; } @@ -257,14 +247,12 @@ export function resolveDiffsPluginDefaults(config: unknown): DiffToolDefaults { resolveConfiguredValue({ primary: defaults.fileQuality, aliases: [defaults.imageQuality], - schemaDefault: DEFAULT_DIFFS_TOOL_DEFAULTS.fileQuality, }), ); const profile = DEFAULT_IMAGE_QUALITY_PROFILES[fileQuality]; const fileFormat = resolveConfiguredValue({ primary: defaults.fileFormat, aliases: [defaults.imageFormat, defaults.format], - schemaDefault: DEFAULT_DIFFS_TOOL_DEFAULTS.fileFormat, }); const fileScale = resolveConfiguredValue({ primary: defaults.fileScale, diff --git a/extensions/diffs/src/http.ts b/extensions/diffs/src/http.ts index 620a8d05702a..38ba90a1c761 100644 --- a/extensions/diffs/src/http.ts +++ b/extensions/diffs/src/http.ts @@ -5,7 +5,7 @@ import type { PluginLogger } from "../api.js"; import { resolveRequestClientIp } from "../runtime-api.js"; import type { DiffArtifactStore } from "./store.js"; import { DIFF_ARTIFACT_ID_PATTERN, DIFF_ARTIFACT_TOKEN_PATTERN } from "./types.js"; -import { VIEWER_ASSET_PREFIX, getServedViewerAsset } from "./viewer-assets.js"; +import { VIEWER_ASSET_PREFIX, VIEWER_RUNTIME_PATH, getServedViewerAsset } from "./viewer-assets.js"; const VIEW_PREFIX = "/plugins/diffs/view/"; const VIEWER_MAX_FAILURES_PER_WINDOW = 40; @@ -23,6 +23,7 @@ const VIEWER_CONTENT_SECURITY_POLICY = [ "frame-ancestors 'self'", "object-src 'none'", ].join("; "); +const IMMUTABLE_ASSET_CACHE_CONTROL = "public, max-age=31536000, immutable"; export function createDiffsHttpHandler(params: { store: DiffArtifactStore; @@ -154,7 +155,11 @@ async function serveAsset( } res.statusCode = 200; - setSharedHeaders(res, asset.contentType); + setSharedHeaders( + res, + asset.contentType, + pathname === VIEWER_RUNTIME_PATH ? IMMUTABLE_ASSET_CACHE_CONTROL : undefined, + ); if (req.method === "HEAD") { res.end(); } else { @@ -174,8 +179,12 @@ function respondText(res: ServerResponse, statusCode: number, body: string): voi res.end(body); } -function setSharedHeaders(res: ServerResponse, contentType: string): void { - res.setHeader("cache-control", "no-store, max-age=0"); +function setSharedHeaders( + res: ServerResponse, + contentType: string, + cacheControl = "no-store, max-age=0", +): void { + res.setHeader("cache-control", cacheControl); res.setHeader("content-type", contentType); res.setHeader("x-content-type-options", "nosniff"); res.setHeader("referrer-policy", "no-referrer"); diff --git a/extensions/diffs/src/language-hints.test.ts b/extensions/diffs/src/language-hints.test.ts index 982f142da5a4..d26662355b30 100644 --- a/extensions/diffs/src/language-hints.test.ts +++ b/extensions/diffs/src/language-hints.test.ts @@ -19,6 +19,16 @@ describe("normalizeSupportedLanguageHint", () => { ]); }); + it("normalizes language hint casing", async () => { + await expect(normalizeHints(["Python", "TypeScript"])).resolves.toEqual([ + "python", + "typescript", + ]); + await expect( + normalizeSupportedLanguageHint("AbAp", { languagePackAvailable: true }), + ).resolves.toBe("abap"); + }); + it("normalizes common aliases to base viewer languages", async () => { await expect( normalizeHints(["ts", "c++", "c#", "bash", "dockerfile", "rb", "kt", "ps1"]), @@ -36,17 +46,7 @@ describe("normalizeSupportedLanguageHint", () => { it("keeps mainstream languages in the base viewer without the language pack", async () => { await expect( - normalizeHints([ - "ruby", - "swift", - "kotlin", - "r", - "dart", - "lua", - "powershell", - "xml", - "toml", - ]), + normalizeHints(["ruby", "swift", "kotlin", "r", "dart", "lua", "powershell", "xml", "toml"]), ).resolves.toEqual([ "ruby", "swift", diff --git a/extensions/diffs/src/language-hints.ts b/extensions/diffs/src/language-hints.ts index 05bd7676af49..1c31ee37767b 100644 --- a/extensions/diffs/src/language-hints.ts +++ b/extensions/diffs/src/language-hints.ts @@ -26,8 +26,8 @@ function normalizeOptionalString(value: unknown): string | undefined { if (typeof value !== "string") { return undefined; } - const trimmed = value.trim(); - return trimmed ? trimmed : undefined; + const normalized = value.trim().toLowerCase(); + return normalized ? normalized : undefined; } export async function normalizeSupportedLanguageHint( diff --git a/extensions/diffs/src/prompt-guidance.ts b/extensions/diffs/src/prompt-guidance.ts index 6e9e3415da50..fa962218e80b 100644 --- a/extensions/diffs/src/prompt-guidance.ts +++ b/extensions/diffs/src/prompt-guidance.ts @@ -2,6 +2,7 @@ export const DIFFS_AGENT_GUIDANCE = [ "When you need to show edits as a real diff, prefer the `diffs` tool instead of writing a manual summary.", "It accepts either `before` + `after` text or a unified `patch`.", + "Check `details.changed`: identical before/after input returns `false` without creating an artifact; rendered results return `true`.", "`mode=view` returns `details.viewerUrl` for canvas use; `mode=file` returns `details.filePath`; `mode=both` returns both.", "If you need to send the rendered file, use the `message` tool with `path` or `filePath`.", "Include `path` when you know the filename, and omit presentation overrides unless needed.", diff --git a/extensions/diffs/src/render-target.test.ts b/extensions/diffs/src/render-target.test.ts index 4e13fc7774db..5dab3c7e4291 100644 --- a/extensions/diffs/src/render-target.test.ts +++ b/extensions/diffs/src/render-target.test.ts @@ -72,7 +72,7 @@ describe("renderDiffDocument render targets", () => { expect(rendered.html).toContain("mock diff"); expect(rendered.imageHtml).toContain("mock diff"); - expect(preloadMultiFileDiffMock).toHaveBeenCalledTimes(2); + expect(preloadMultiFileDiffMock).toHaveBeenCalledTimes(1); }); it("renders only the image variant for patch image mode", async () => { diff --git a/extensions/diffs/src/render.preload.test.ts b/extensions/diffs/src/render.preload.test.ts new file mode 100644 index 000000000000..a7b22c3ded94 --- /dev/null +++ b/extensions/diffs/src/render.preload.test.ts @@ -0,0 +1,70 @@ +// Diffs tests cover shared SSR preload behavior. +import { disposeHighlighter } from "@pierre/diffs"; +import * as diffsSsr from "@pierre/diffs/ssr"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { DEFAULT_DIFFS_TOOL_DEFAULTS, resolveDiffImageRenderOptions } from "./config.js"; +import { renderDiffDocument } from "./render.js"; + +vi.mock("@pierre/diffs/ssr", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + preloadFileDiff: vi.fn(actual.preloadFileDiff), + preloadMultiFileDiff: vi.fn(actual.preloadMultiFileDiff), + }; +}); + +describe("renderDiffDocument SSR preloads", () => { + afterEach(async () => { + vi.clearAllMocks(); + await disposeHighlighter(); + }); + + it("preloads a before/after diff once for viewer and image output", async () => { + await renderDiffDocument( + { + kind: "before_after", + before: "const value = 1;\n", + after: "const value = 2;\n", + path: "src/example.ts", + }, + { + presentation: DEFAULT_DIFFS_TOOL_DEFAULTS, + image: resolveDiffImageRenderOptions({ defaults: DEFAULT_DIFFS_TOOL_DEFAULTS }), + expandUnchanged: false, + }, + "both", + ); + + expect(diffsSsr.preloadMultiFileDiff).toHaveBeenCalledTimes(1); + }); + + it("preloads each patch file once for viewer and image output", async () => { + const patch = [ + "diff --git a/a.ts b/a.ts", + "--- a/a.ts", + "+++ b/a.ts", + "@@ -1 +1 @@", + "-const a = 1;", + "+const a = 2;", + "diff --git a/b.ts b/b.ts", + "--- a/b.ts", + "+++ b/b.ts", + "@@ -1 +1 @@", + "-const b = 1;", + "+const b = 2;", + ].join("\n"); + + await renderDiffDocument( + { kind: "patch", patch }, + { + presentation: DEFAULT_DIFFS_TOOL_DEFAULTS, + image: resolveDiffImageRenderOptions({ defaults: DEFAULT_DIFFS_TOOL_DEFAULTS }), + expandUnchanged: false, + }, + "both", + ); + + expect(diffsSsr.preloadFileDiff).toHaveBeenCalledTimes(2); + }); +}); diff --git a/extensions/diffs/src/render.ts b/extensions/diffs/src/render.ts index 09fe4a4d0052..6bf3b5dd8628 100644 --- a/extensions/diffs/src/render.ts +++ b/extensions/diffs/src/render.ts @@ -24,6 +24,13 @@ const MAX_PATCH_TOTAL_LINES = 120_000; const VIEWER_LOADER_DOCUMENT_PATH = "../../assets/viewer.js"; const LANGUAGE_PACK_VIEWER_LOADER_DOCUMENT_PATH = "../../../diffs-language-pack/assets/viewer.js"; +export class DiffRenderInputError extends Error { + constructor(message: string) { + super(message); + this.name = "DiffRenderInputError"; + } +} + function escapeCssString(value: string): string { return value.replaceAll("\\", "\\\\").replaceAll('"', '\\"'); } @@ -65,11 +72,19 @@ function resolveBeforeAfterFileName(params: { return DEFAULT_FILE_NAME; } +function resolveDiffTypography(presentation: DiffRenderOptions["presentation"]): { + fontSize: number; + lineHeight: number; +} { + const fontSize = normalizeDiffFontSize(presentation.fontSize); + const lineSpacing = normalizeDiffLineSpacing(presentation.lineSpacing); + const lineHeight = Math.max(20, Math.round(fontSize * lineSpacing)); + return { fontSize, lineHeight }; +} + function buildDiffOptions(options: DiffRenderOptions): DiffViewerOptions { const fontFamily = escapeCssString(options.presentation.fontFamily); - const fontSize = normalizeDiffFontSize(options.presentation.fontSize); - const lineSpacing = normalizeDiffLineSpacing(options.presentation.lineSpacing); - const lineHeight = Math.max(20, Math.round(fontSize * lineSpacing)); + const { fontSize, lineHeight } = resolveDiffTypography(options.presentation); return { theme: { light: "pierre-light", @@ -201,6 +216,10 @@ function buildHtmlDocument(params: { bodyHtml: string; theme: DiffRenderOptions["presentation"]["theme"]; imageMaxWidth: number; + imageTypography: { + fontSize: number; + lineHeight: number; + }; runtimeMode: "viewer" | "image"; viewerRuntime: "base" | "language-pack"; }): string { @@ -208,6 +227,15 @@ function buildHtmlDocument(params: { params.viewerRuntime === "language-pack" ? LANGUAGE_PACK_VIEWER_LOADER_DOCUMENT_PATH : VIEWER_LOADER_DOCUMENT_PATH; + const imageTypographyCss = + params.runtimeMode === "image" + ? ` + .oc-frame[data-render-mode="image"] .oc-diff-host { + --diffs-font-size: ${params.imageTypography.fontSize}px; + --diffs-line-height: ${params.imageTypography.lineHeight}px; + } +` + : ""; return ` @@ -256,6 +284,7 @@ function buildHtmlDocument(params: { .oc-frame[data-render-mode="image"] { max-width: ${Math.max(640, Math.round(params.imageMaxWidth))}px; } +${imageTypographyCss} [data-openclaw-diff-root] { display: grid; @@ -364,54 +393,32 @@ async function renderBeforeAfterDiff( ...(lang ? { lang } : {}), }; const { viewerOptions, imageOptions } = buildRenderVariants({ options, target }); - const [viewerResult, imageResult] = await Promise.all([ - viewerOptions - ? preloadMultiFileDiffWithFallback({ - oldFile, - newFile, - options: viewerOptions, - }) - : Promise.resolve(undefined), - imageOptions - ? preloadMultiFileDiffWithFallback({ - oldFile, - newFile, - options: imageOptions, - }) - : Promise.resolve(undefined), - ]); - const [viewerPayload, imagePayload] = await Promise.all([ - viewerResult && viewerOptions - ? normalizeDiffViewerPayloadLanguages( - { - prerenderedHTML: viewerResult.prerenderedHTML, - oldFile: viewerResult.oldFile, - newFile: viewerResult.newFile, - options: viewerOptions, - langs: collectDiffPayloadLanguageHints({ - oldFile: viewerResult.oldFile, - newFile: viewerResult.newFile, - }), - }, - { languagePackAvailable }, - ) - : Promise.resolve(undefined), - imageResult && imageOptions - ? normalizeDiffViewerPayloadLanguages( - { - prerenderedHTML: imageResult.prerenderedHTML, - oldFile: imageResult.oldFile, - newFile: imageResult.newFile, - options: imageOptions, - langs: collectDiffPayloadLanguageHints({ - oldFile: imageResult.oldFile, - newFile: imageResult.newFile, - }), - }, - { languagePackAvailable }, - ) - : Promise.resolve(undefined), - ]); + const preloadOptions = viewerOptions ?? imageOptions; + if (!preloadOptions) { + throw new Error(`Unsupported diff render target: ${target}`); + } + const preloadResult = await preloadMultiFileDiffWithFallback({ + oldFile, + newFile, + options: preloadOptions, + }); + const normalizedPayload = await normalizeDiffViewerPayloadLanguages( + { + prerenderedHTML: preloadResult.prerenderedHTML, + oldFile: preloadResult.oldFile, + newFile: preloadResult.newFile, + options: preloadOptions, + langs: collectDiffPayloadLanguageHints({ + oldFile: preloadResult.oldFile, + newFile: preloadResult.newFile, + }), + }, + { languagePackAvailable }, + ); + const viewerPayload = viewerOptions + ? { ...normalizedPayload, options: viewerOptions } + : undefined; + const imagePayload = imageOptions ? { ...normalizedPayload, options: imageOptions } : undefined; const section = buildRenderedSection({ ...(viewerPayload ? { viewerPayload } : {}), ...(imagePayload ? { imagePayload } : {}), @@ -441,10 +448,12 @@ async function renderPatchDiff( .map((fileDiff) => normalizePatchFileLanguage(fileDiff, { languagePackAvailable })), ); if (files.length === 0) { - throw new Error("Patch input did not contain any file diffs."); + throw new DiffRenderInputError("Patch input did not contain any file diffs."); } if (files.length > MAX_PATCH_FILE_COUNT) { - throw new Error(`Patch input contains too many files (max ${MAX_PATCH_FILE_COUNT}).`); + throw new DiffRenderInputError( + `Patch input contains too many files (max ${MAX_PATCH_FILE_COUNT}).`, + ); } const totalLines = files.reduce((sum, fileDiff) => { const splitLines = Number.isFinite(fileDiff.splitLineCount) ? fileDiff.splitLineCount : 0; @@ -452,51 +461,37 @@ async function renderPatchDiff( return sum + Math.max(splitLines, unifiedLines, 0); }, 0); if (totalLines > MAX_PATCH_TOTAL_LINES) { - throw new Error(`Patch input is too large to render (max ${MAX_PATCH_TOTAL_LINES} lines).`); + throw new DiffRenderInputError( + `Patch input is too large to render (max ${MAX_PATCH_TOTAL_LINES} lines).`, + ); } const { viewerOptions, imageOptions } = buildRenderVariants({ options, target }); + const preloadOptions = viewerOptions ?? imageOptions; + if (!preloadOptions) { + throw new Error(`Unsupported diff render target: ${target}`); + } const sections = await Promise.all( files.map(async (fileDiff) => { - const [viewerResult, imageResult] = await Promise.all([ - viewerOptions - ? preloadFileDiffWithFallback({ - fileDiff, - options: viewerOptions, - }) - : Promise.resolve(undefined), - imageOptions - ? preloadFileDiffWithFallback({ - fileDiff, - options: imageOptions, - }) - : Promise.resolve(undefined), - ]); - - const [viewerPayload, imagePayload] = await Promise.all([ - viewerResult && viewerOptions - ? normalizeDiffViewerPayloadLanguages( - { - prerenderedHTML: viewerResult.prerenderedHTML, - fileDiff: viewerResult.fileDiff, - options: viewerOptions, - langs: collectDiffPayloadLanguageHints({ fileDiff: viewerResult.fileDiff }), - }, - { languagePackAvailable }, - ) - : Promise.resolve(undefined), - imageResult && imageOptions - ? normalizeDiffViewerPayloadLanguages( - { - prerenderedHTML: imageResult.prerenderedHTML, - fileDiff: imageResult.fileDiff, - options: imageOptions, - langs: collectDiffPayloadLanguageHints({ fileDiff: imageResult.fileDiff }), - }, - { languagePackAvailable }, - ) - : Promise.resolve(undefined), - ]); + const preloadResult = await preloadFileDiffWithFallback({ + fileDiff, + options: preloadOptions, + }); + const normalizedPayload = await normalizeDiffViewerPayloadLanguages( + { + prerenderedHTML: preloadResult.prerenderedHTML, + fileDiff: preloadResult.fileDiff, + options: preloadOptions, + langs: collectDiffPayloadLanguageHints({ fileDiff: preloadResult.fileDiff }), + }, + { languagePackAvailable }, + ); + const viewerPayload = viewerOptions + ? { ...normalizedPayload, options: viewerOptions } + : undefined; + const imagePayload = imageOptions + ? { ...normalizedPayload, options: imageOptions } + : undefined; return buildRenderedSection({ ...(viewerPayload ? { viewerPayload } : {}), @@ -537,6 +532,7 @@ export async function renderDiffDocument( ? await renderBeforeAfterDiff(input, options, target) : await renderPatchDiff(input, options, target); const viewerRuntime = rendered.usesLanguagePack ? "language-pack" : "base"; + const imageTypography = resolveDiffTypography(buildImageRenderOptions(options).presentation); return { ...(rendered.viewerBodyHtml @@ -546,6 +542,7 @@ export async function renderDiffDocument( bodyHtml: rendered.viewerBodyHtml, theme: options.presentation.theme, imageMaxWidth: options.image.maxWidth, + imageTypography, runtimeMode: "viewer", viewerRuntime, }), @@ -558,6 +555,7 @@ export async function renderDiffDocument( bodyHtml: rendered.imageBodyHtml, theme: options.presentation.theme, imageMaxWidth: options.image.maxWidth, + imageTypography, runtimeMode: "image", viewerRuntime, }), diff --git a/extensions/diffs/src/store.test.ts b/extensions/diffs/src/store.test.ts index 87ee6478cec5..a5ddc0b2b2e5 100644 --- a/extensions/diffs/src/store.test.ts +++ b/extensions/diffs/src/store.test.ts @@ -53,6 +53,8 @@ describe("DiffArtifactStore", () => { agentAccountId: "default", }); expect(await store.readHtml(artifact.id)).toBe("demo"); + await expect(store.getArtifact(artifact.id, "0".repeat(48))).resolves.toBeNull(); + await expect(store.getArtifact(artifact.id, "short")).resolves.toBeNull(); }); it("caps artifact expiry instead of throwing near the Date boundary", async () => { @@ -280,6 +282,7 @@ describe("createDiffsHttpHandler", () => { expect(res.statusCode).toBe(200); expect(res.body).toBe("viewer"); expect(res.getHeader("content-security-policy")).toContain("default-src 'none'"); + expect(res.getHeader("cache-control")).toBe("no-store, max-age=0"); }); it("rejects invalid tokens", async () => { @@ -321,6 +324,7 @@ describe("createDiffsHttpHandler", () => { expect(handled).toBe(true); expect(res.statusCode).toBe(200); expect(String(res.body)).toContain("./viewer-runtime.js?v="); + expect(res.getHeader("cache-control")).toBe("no-store, max-age=0"); }); it("serves the shared viewer runtime asset", async () => { @@ -337,6 +341,7 @@ describe("createDiffsHttpHandler", () => { expect(handled).toBe(true); expect(res.statusCode).toBe(200); expect(String(res.body)).toContain("openclawDiffsReady"); + expect(res.getHeader("cache-control")).toBe("public, max-age=31536000, immutable"); }); it.each([ diff --git a/extensions/diffs/src/store.ts b/extensions/diffs/src/store.ts index b6e7b95760fa..b33138d5cffd 100644 --- a/extensions/diffs/src/store.ts +++ b/extensions/diffs/src/store.ts @@ -3,7 +3,7 @@ import crypto from "node:crypto"; import fs from "node:fs/promises"; import path from "node:path"; import { MAX_DATE_TIMESTAMP_MS, timestampMsToIsoString } from "openclaw/plugin-sdk/number-runtime"; -import { root as fsRoot } from "openclaw/plugin-sdk/security-runtime"; +import { root as fsRoot, safeEqualSecret } from "openclaw/plugin-sdk/security-runtime"; import { normalizeOptionalString } from "openclaw/plugin-sdk/string-coerce-runtime"; import type { PluginLogger } from "../api.js"; import type { DiffArtifactContext, DiffArtifactMeta, DiffOutputFormat } from "./types.js"; @@ -94,7 +94,7 @@ export class DiffArtifactStore { if (!meta) { return null; } - if (meta.token !== token) { + if (!safeEqualSecret(token, meta.token)) { return null; } if (isExpired(meta)) { diff --git a/extensions/diffs/src/tool.test.ts b/extensions/diffs/src/tool.test.ts index b8805776173d..9bab645743d7 100644 --- a/extensions/diffs/src/tool.test.ts +++ b/extensions/diffs/src/tool.test.ts @@ -12,11 +12,16 @@ import { createDiffsTool } from "./tool.js"; import type { DiffRenderOptions } from "./types.js"; describe("diffs tool", () => { + let rootDir: string; let store: DiffArtifactStore; let cleanupRootDir: () => Promise; beforeEach(async () => { - ({ store, cleanup: cleanupRootDir } = await createDiffStoreHarness("openclaw-diffs-tool-")); + ({ + rootDir, + store, + cleanup: cleanupRootDir, + } = await createDiffStoreHarness("openclaw-diffs-tool-")); }); afterEach(async () => { @@ -42,6 +47,32 @@ describe("diffs tool", () => { expect(String(readDetails(result).viewerUrl)).toContain( "http://127.0.0.1:18789/plugins/diffs/view/", ); + expect(readDetails(result).changed).toBe(true); + }); + + it("short-circuits identical before/after input without creating an artifact", async () => { + const screenshotHtml = vi.fn(); + const tool = createToolWithScreenshotter(store, { screenshotHtml }); + + const result = await tool.execute?.("tool-identical", { + before: "same\n", + after: "same\n", + }); + + expect(readTextContent(result, 0)).toBe( + "Before and after are identical — no changes to render.", + ); + expect(readDetails(result)).toEqual({ + changed: false, + context: { + agentId: "main", + sessionId: "session-123", + messageChannel: "discord", + agentAccountId: "default", + }, + }); + expect(screenshotHtml).not.toHaveBeenCalled(); + await expect(fs.readdir(rootDir)).resolves.toEqual([]); }); it("uses configured viewerBaseUrl when tool input omits baseUrl", async () => { @@ -388,6 +419,29 @@ describe("diffs tool", () => { ).rejects.toThrow("patch exceeds maximum size"); }); + it("classifies patch render validation failures as tool input errors", async () => { + const tool = createDiffsTool({ + api: createApi(), + store, + defaults: DEFAULT_DIFFS_TOOL_DEFAULTS, + }); + + const error = await tool + .execute?.("tool-invalid-patch", { + patch: "not a unified patch", + mode: "view", + }) + .then( + () => undefined, + (caught: unknown) => caught, + ); + + expect(error).toMatchObject({ + name: "ToolInputError", + message: "Patch input did not contain any file diffs.", + }); + }); + it("rejects oversized before/after payloads", async () => { const tool = createDiffsTool({ api: createApi(), diff --git a/extensions/diffs/src/tool.ts b/extensions/diffs/src/tool.ts index 42918a3f4145..593ed28ab9a0 100644 --- a/extensions/diffs/src/tool.ts +++ b/extensions/diffs/src/tool.ts @@ -9,7 +9,7 @@ import type { Static } from "typebox"; import type { AnyAgentTool, OpenClawPluginApi, OpenClawPluginToolContext } from "../api.js"; import { PlaywrightDiffScreenshotter, type DiffScreenshotter } from "./browser.js"; import { resolveDiffImageRenderOptions } from "./config.js"; -import { renderDiffDocument } from "./render.js"; +import { DiffRenderInputError, renderDiffDocument } from "./render.js"; import type { DiffArtifactStore } from "./store.js"; import type { DiffArtifactContext, @@ -167,6 +167,20 @@ export function createDiffsTool(params: { const rawRecord = rawParams as Record; const artifactContext = buildArtifactContext(params.context); const input = normalizeDiffInput(toolParams); + if (input.kind === "before_after" && input.before === input.after) { + return { + content: [ + { + type: "text", + text: "Before and after are identical — no changes to render.", + }, + ], + details: { + changed: false, + ...(artifactContext ? { context: artifactContext } : {}), + }, + }; + } const mode = normalizeMode(toolParams.mode, params.defaults.mode); const theme = normalizeTheme(toolParams.theme, params.defaults.theme); const layout = normalizeLayout(toolParams.layout, params.defaults.layout); @@ -204,7 +218,12 @@ export function createDiffsTool(params: { languagePackAvailable: params.languagePackAvailable, }, renderTarget, - ); + ).catch((error: unknown) => { + if (error instanceof DiffRenderInputError) { + throw new PluginToolInputError(error.message); + } + throw error; + }); const screenshotter = params.screenshotter ?? new PlaywrightDiffScreenshotter({ config: params.api.config }); @@ -232,6 +251,7 @@ export function createDiffsTool(params: { ], details: buildArtifactDetails({ baseDetails: { + changed: true, ...(artifactFile.artifactId ? { artifactId: artifactFile.artifactId } : {}), ...(artifactFile.expiresAt ? { expiresAt: artifactFile.expiresAt } : {}), title: rendered.title, @@ -262,6 +282,7 @@ export function createDiffsTool(params: { }); const baseDetails = { + changed: true, artifactId: artifact.id, viewerUrl, viewerPath: artifact.viewerPath, diff --git a/extensions/diffs/src/viewer-client.test.ts b/extensions/diffs/src/viewer-client.test.ts index 55a847070fdb..1e2b34439263 100644 --- a/extensions/diffs/src/viewer-client.test.ts +++ b/extensions/diffs/src/viewer-client.test.ts @@ -192,6 +192,16 @@ describe("hydrateViewer", () => { }); }); +describe("resolveViewerLanguagePackAvailability", () => { + it("resolves defined and undefined build flags", async () => { + const { resolveViewerLanguagePackAvailability } = await import("./viewer-client.js"); + + expect(resolveViewerLanguagePackAvailability(true)).toBe(true); + expect(resolveViewerLanguagePackAvailability(false)).toBe(false); + expect(resolveViewerLanguagePackAvailability(undefined)).toBe(false); + }); +}); + describe("viewerState initialization", () => { beforeEach(() => { document.body.innerHTML = ""; diff --git a/extensions/diffs/src/viewer-client.ts b/extensions/diffs/src/viewer-client.ts index cd6810a08f54..a207aae392d1 100644 --- a/extensions/diffs/src/viewer-client.ts +++ b/extensions/diffs/src/viewer-client.ts @@ -10,6 +10,23 @@ import { normalizeDiffViewerPayloadLanguages } from "./language-hints.js"; import type { DiffViewerPayload, DiffLayout, DiffTheme } from "./types.js"; import { parseViewerPayloadJson } from "./viewer-payload.js"; +// oxlint-disable-next-line eslint/no-underscore-dangle -- Bundled builds replace this compile-time define identifier. +declare const __OPENCLAW_DIFFS_LANGUAGE_PACK__: boolean | undefined; + +// Build-time esbuild define; typeof guard keeps the module loadable where the +// define is absent (vitest/node), matching the __OPENCLAW_VERSION__ pattern. +function readInjectedLanguagePackFlag(): boolean | undefined { + return typeof __OPENCLAW_DIFFS_LANGUAGE_PACK__ === "boolean" + ? __OPENCLAW_DIFFS_LANGUAGE_PACK__ + : undefined; +} + +export function resolveViewerLanguagePackAvailability( + buildFlag: boolean | undefined = readInjectedLanguagePackFlag(), +): boolean { + return buildFlag === true; +} + type ViewerState = { theme: DiffTheme; layout: DiffLayout; @@ -290,10 +307,11 @@ export async function hydrateViewer(): Promise { // Rehydration replaces the current DOM card set; do not retain controllers // from a previous render because they can keep stale DOM references alive. controllers.length = 0; + const languagePackAvailable = resolveViewerLanguagePackAvailability(); const cards = await Promise.all( getCards().map(async ({ host, payload }) => ({ host, - payload: await normalizeDiffViewerPayloadLanguages(payload), + payload: await normalizeDiffViewerPayloadLanguages(payload, { languagePackAvailable }), })), ); const langs = new Set(); diff --git a/scripts/build-diffs-viewer-runtime.mjs b/scripts/build-diffs-viewer-runtime.mjs index ba701dc05091..720d50e498e0 100644 --- a/scripts/build-diffs-viewer-runtime.mjs +++ b/scripts/build-diffs-viewer-runtime.mjs @@ -16,10 +16,12 @@ const targets = { entry: "extensions/diffs/src/viewer-client.ts", output: "extensions/diffs/assets/viewer-runtime.js", shikiAlias: "scripts/diffs-shiki-curated.ts", + languagePackAvailable: false, }, full: { entry: "extensions/diffs/src/viewer-client.ts", output: "extensions/diffs-language-pack/assets/viewer-runtime.js", + languagePackAvailable: true, }, }; @@ -81,6 +83,7 @@ export async function buildDiffsViewerRuntime(targetName) { format: "esm", minify: true, define: { + __OPENCLAW_DIFFS_LANGUAGE_PACK__: String(target.languagePackAvailable), NaN: "Number.NaN", }, legalComments: "none", diff --git a/test/scripts/lint-suppressions.test.ts b/test/scripts/lint-suppressions.test.ts index fa9e7f050823..53cecdd37dd0 100644 --- a/test/scripts/lint-suppressions.test.ts +++ b/test/scripts/lint-suppressions.test.ts @@ -188,6 +188,7 @@ describe("production lint suppressions", () => { "extensions/browser/src/browser/pw-tools-core.interactions.ts|@typescript-eslint/no-implied-eval|2", "extensions/browser/src/cli/browser-cli-actions-input/register.files-downloads.ts|typescript/no-unnecessary-type-parameters|1", "extensions/browser/src/node-host/invoke-browser.ts|typescript/no-unnecessary-type-parameters|1", + "extensions/diffs/src/viewer-client.ts|eslint/no-underscore-dangle|1", "extensions/discord/src/outbound-adapter.test-harness.ts|typescript/no-unnecessary-type-parameters|1", "extensions/discord/src/test-support/provider.test-support.ts|typescript/no-unnecessary-type-parameters|1", "extensions/feishu/src/bitable.ts|typescript/no-unnecessary-type-parameters|1",