Skip to content

Dashboard: render tool-result images inline#390

Merged
m-aebrer merged 9 commits into
aebrer:masterfrom
maxscheurer:feature/issue-389-dashboard-tool-result-images
Jul 24, 2026
Merged

Dashboard: render tool-result images inline#390
m-aebrer merged 9 commits into
aebrer:masterfrom
maxscheurer:feature/issue-389-dashboard-tool-result-images

Conversation

@maxscheurer

Copy link
Copy Markdown
Contributor

Closes #389

Render tool-result image content blocks (e.g. read on a PNG/JPG/GIF/WebP) inline in the dashboard transcript, independent of whether the active model supports vision. Currently the dashboard reducer flattens tool results to text only and silently drops image blocks.

Implementation plan posted as a comment below.

@maxscheurer

Copy link
Copy Markdown
Contributor Author

Implementation Plan

Problem

The dashboard silently discards image content blocks returned by tools. read on an image returns [{type:"text",…}, {type:"image", data, mimeType}], but the dashboard reducer's contentToText() (packages/dashboard/src/client/state/reducer.ts:181-188) keeps only parts with a string .text and drops the image block on all three paths (tool_execution_update, tool_execution_end, and replay messagesToEntries). ToolEntry has no image field, and transcript.tsx has no <img> render path. The human viewing the dashboard should always see the image, regardless of model vision support.

Key design decision — do NOT send image bytes over SSE

This is the crux. event-hub.ts enforces DEFAULT_EVENT_BYTES = 1 MB per frame; any larger frame is dropped and replaced with a dashboard_resync barrier (publish(), lines 116-126). projectDashboardEvent (lines 70-92) has no tool_execution_end case, so it currently carries the full base64 result.content. Since read resizes images to a base64 cap of 4.5 MB (utils/image-resize.ts), a large image today would blow the 1 MB frame limit and drop the entire tool-result event — worse than the current bug.

Chosen approach (robust, reuses existing infra): strip image data from the live SSE frame and have the client lazy-load the bytes over HTTP, which has no 1 MB cap.

  • Server (projectDashboardEvent): add a tool_execution_end case that replaces each image block's data with a lightweight marker (keep type:"image", mimeType, drop/blank data, add hasImage/omission flag). This keeps the SSE frame small and eliminates the oversized-event drop and replay-buffer bloat.
  • Client: when a tool result reports image blocks with stripped data, lazily GET /api/runtimes/:key/messages (server.ts:527 — already returns full messages incl. image blocks) and extract the image(s) by toolCallId. Replay/refresh naturally works because messagesToEntries reconstructs from that same endpoint.

(Alternative considered and rejected for the plan: raising eventBytes for tool frames — inflates the 8 MB replay buffer / 3 MB replay budget and risks evicting transcript history. Documented in the issue assessment.)

Deliverables

  1. Reducer (reducer.ts): replace contentToText() usage on tool-result paths with a new contentToParts() helper returning { text: string; images: { data?: string; mimeType: string }[] }. Add images?: { data?: string; mimeType: string; pending?: boolean }[] to the ToolEntry interface (lines 51-63). Populate it in tool_execution_update, tool_execution_end, and the replay messagesToEntries branch (lines 259-266).
  2. Lazy image fetch (client store/api): on encountering pending images (data stripped by SSE projection), fetch full content via the messages endpoint and merge data into the matching ToolEntry. Reuse the existing api.ts client + store patterns.
  3. Server projection (event-hub.ts): add the tool_execution_end image-stripping case to projectDashboardEvent.
  4. Transcript render (transcript.tsx): render entry.images as sanitized data-URI <img> inside the tool-result card. Make it generic across tools (any tool with image blocks), not gated on toolName === "read". Allowlist mimeType to image/png|jpeg|gif|webp; render only as data: URI <img> (no raw HTML sink). Add supporting CSS in styles/app.css with bounded max dimensions.
  5. HTML export parity — VERIFY + TEST ONLY. export-html/template.js already has a case 'read' that calls renderResultImages() emitting <img src="data:…"> (and the same for messages). Confirm it works end-to-end and add a regression test; do not rebuild.

Acceptance criteria

  • Reading a PNG/JPG/GIF/WebP via read renders the image inline in the dashboard tool card (not just the text note).
  • Works on text-only models (image omitted from LLM context but shown in dashboard).
  • Image survives SSE replay/resync (refresh mid-session → image still renders) via the messages HTTP endpoint.
  • Large images do not trip the 1 MB eventBytes drop (bytes travel over HTTP, not SSE).
  • No XSS: base64 validated, mimeType allowlisted to image types, rendered only as data-URI <img>.
  • Exported HTML transcript embeds the image (verified + regression-tested).

Files to create or modify

  • packages/dashboard/src/client/state/reducer.tscontentToParts(), ToolEntry.images, populate on all 3 paths.
  • packages/dashboard/src/client/state/store.ts + client/api.ts — lazy image fetch/merge.
  • packages/dashboard/src/server/event-hub.tstool_execution_end projection case.
  • packages/dashboard/src/client/components/transcript.tsx<img> render path (generic, sanitized).
  • packages/dashboard/src/client/styles/app.css — bounded image styles.
  • (Verify) packages/coding-agent/src/core/export-html/template.js — image render already present.

Testing approach

  • test/reducer.test.tscontentToParts() extracts text + images; ToolEntry.images populated on tool_execution_end, tool_execution_update, and replay; text-only note preserved alongside image.
  • test/event-hub.test.tsprojectDashboardEvent strips image data from tool_execution_end, keeps mimeType/marker, and the resulting frame stays under eventBytes (no oversized-event resync for a large image).
  • test/client/api.test.ts / test/client/store.test.ts — lazy fetch merges image bytes into the correct ToolEntry by toolCallId.
  • test/client/screens.test.tsx — transcript renders a sanitized data-URI <img> for a tool result with an image; disallowed mimeType is not rendered; generic across tool names.
  • Export-html regression test — HTML export embeds a tool-result image as a data: URI.

Risks and open questions

  • Lazy-fetch UX: brief delay between text note and image appearing on live events; acceptable. Confirm no duplicate fetches (dedupe by toolCallId).
  • Marker shape: exact stripped-image marker field name (pending vs omitted vs hasImage) — settle during implementation; keep it internal to reducer/store.
  • Genericity: ensure the transcript path handles multiple images and multiple image-returning tools, not just read.
  • Security: strict base64 validation + mimeType allowlist; never route image data through an HTML-injection sink (the existing HighlightedPre/MarkdownBody use DOMPurify; the <img> path must be a plain data-URI attribute).

Plan created by mach6

Tools that return image content blocks (e.g. read on a PNG/JPG/GIF/WebP)
previously showed only the text note in the dashboard; the reducer flattened
tool results to text and silently dropped every image block. Surface those
images inline so the human viewing the dashboard always sees them, independent
of whether the active model supports vision.

- reducer: add ToolResultImage type and ToolEntry.images; new contentToParts()
  helper (mime allowlist excluding SVG, base64 validation) populating images on
  the live tool_execution_update/end paths and the replay messagesToEntries path
- transcript: render entry.images as sanitized data-URI <img>, generic across
  all tools, with a defense-in-depth mime re-check; bounded via CSS
- large images (>1MB) ride the existing oversized_event -> resync -> messages
  recovery path, so the SSE eventBytes budget is never blown
- HTML export already embedded tool-result images; add a regression test
- tests: reducer image extraction on all paths, transcript render + mime
  allowlist, and HTML export image parity
@maxscheurer

Copy link
Copy Markdown
Contributor Author

Progress Update

Implemented inline rendering of tool-result images in the dashboard transcript.

Design change vs. the plan (simpler, lower-risk)

The posted plan chose to strip image bytes from the SSE frame and lazy-fetch them over HTTP (new endpoint + store fetch logic). While implementing, a closer read of store.ts showed that path is unnecessary: when a tool_execution_end frame exceeds the 1 MB eventBytes budget, publish() already drops it and emits a dashboard_resync barrier, which drives the client through /api/resyncmessagesToEntries — and the messages endpoint carries full image blocks (no size cap). So making messagesToEntries extract images makes large images render for free, with no SSE stripping, no lazy-fetch, and no new endpoint.

Net result — same acceptance criteria, far less surface area and risk:

  • Small images (< 1 MB base64) stream inline via the live event.
  • Large images ride the pre-existing oversized-event → resync → messages recovery path.
  • The eventBytes budget is never blown and the replay buffer is not inflated (we did not raise the limit).
  • Subagent tool images are covered for free (same reducer + subagent resync path).

Changes

  • reducer.ts — added ToolResultImage type + ToolEntry.images; new contentToParts() helper (mime allowlist excluding SVG + base64 validation) populating images on tool_execution_update, tool_execution_end, and the replay messagesToEntries toolResult path.
  • transcript.tsxToolResultImages component renders sanitized data: URI <img>, generic across all tools, with a defense-in-depth mime re-check.
  • app.css — bounded .tool-image styles (max-height 480px).
  • HTML export already embedded tool-result images (template.js case 'read'renderResultImages()); confirmed + added a regression test.

Acceptance criteria

  • read on PNG/JPG/GIF/WebP renders inline in the tool card
  • Works on text-only models (image omitted from LLM context, shown in dashboard)
  • Survives SSE replay/resync (refresh mid-session)
  • Large images bounded — no eventBytes breach; render dims capped
  • No XSS — base64 validated, mime allowlisted (no SVG), data-URI <img> only
  • Exported HTML transcript embeds the image (verified + regression-tested)

Tests

  • reducer.test.ts — image extraction on live end/update + replay paths; text-only stays imageless; SVG + malformed base64 dropped.
  • screens.test.tsx — inline <img> data-URI render; generic across tool names; no img when absent; disallowed mime not rendered.
  • export-html-images.test.ts (new) — export pipeline round-trips the image block, and the real renderResultImages emits the data: URI <img>.

All green: dashboard vitest + browser suites and coding-agent suite pass; tsgo + biome clean.

Commit: 85215a7


Progress tracked by mach6

@maxscheurer
maxscheurer marked this pull request as ready for review July 21, 2026 13:13
@maxscheurer

Copy link
Copy Markdown
Contributor Author

Code Review

Critical

None.

Important

  1. Exported tool-result images permit stored HTML injection (confidence 99) — packages/coding-agent/src/core/export-html/template.js:874-884,1373-1378. mimeType and data are interpolated into an HTML string without the dashboard's MIME allowlist or base64 validation, then inserted via innerHTML. A crafted/imported session or extension tool result can break out of src and execute script when the export is opened. Validate exact raster MIME types and base64 before rendering, preferably assign img.src through DOM APIs, apply the same rules to message images, and add malicious-value tests.

  2. Image recovery has unbounded HTTP, memory, and decode costs (confidence 96) — packages/dashboard/src/client/state/reducer.ts:214-245, packages/dashboard/src/client/components/transcript.tsx:364-373, packages/dashboard/src/client/styles/app.css:795-803. The SSE cap only diverts oversized events to /api/resync; the snapshot can still transfer and retain every image-bearing message, while visible cards eagerly create data URIs and decoded surfaces. CSS dimensions limit layout, not bytes or decoded pixels, and generic tools need not honor read resizing. Add enforceable per-image/aggregate limits and lazy rendering or a bounded image transport with an omission state.

  3. The core oversized-event recovery design is not tested end to end (confidence 98) — packages/dashboard/test/reducer.test.ts:115. The test named “resync recovery” calls messagesToEntries() directly and does not exercise oversized tool_execution_enddashboard_resync/api/resync → store hydration. Add an integration test with a deliberately small eventBytes limit that verifies the barrier, persisted image snapshot, and hydrated image bytes.

  4. The export test bypasses actual rendering dispatch (confidence 99) — packages/coding-agent/test/export-html-images.test.ts:135-158. Regex extraction plus new Function tests a detached helper, so it stays green if renderToolCall() never invokes that helper and does not cover the generated artifact. Load the exported page in the established browser-test setup and assert the rendered DOM for both read and a non-read image-producing tool.

Suggestions

  1. Final results can retain stale images from partial updates (confidence 100) — packages/dashboard/src/client/state/reducer.ts:508-533. Images are assigned only when the newest parsed list is non-empty. An image-bearing update followed by a text-only, malformed, disallowed, string, or empty authoritative final result leaves the old image visible. Treat replacement payloads as authoritative and clear entry.images when the replacement contains no valid images; add a lifecycle regression test.

  2. Oversized partial images cannot be recovered by resync (confidence 93) — packages/dashboard/src/client/state/reducer.ts:508-518, packages/dashboard/src/server/event-hub.ts:113-119. The reducer supports image-bearing tool_execution_update, but an oversized update is replaced by a resync barrier while partial data is absent from persisted messages. If the final result does not repeat the image, it is silently lost. Either make such payloads retrievable, guarantee final repetition, or represent the omission explicitly.

  3. HTML export drops images from non-read tools (confidence 100) — packages/coding-agent/src/core/export-html/template.js:863-1000. renderResultImages() is only invoked by the read branch, while the dashboard behavior and issue scope are generic. Append sanitized result images from the common tool-result path and cover a custom/non-read tool in the export regression test.

  4. The four supported raster formats are not covered through the complete dashboard pipeline (confidence 94) — packages/dashboard/test/client/screens.test.tsx:3968. PNG, JPEG, and WebP are split across reducer/component tests and GIF is absent; because reducer and renderer have separate allowlists, drift can pass. Add a parameterized event/message → reducer → transcript test for PNG, JPEG, GIF, and WebP.

  5. Reuse the existing dashboard image DTO (confidence 96) — packages/dashboard/src/client/state/reducer.ts:51-80. ToolResultImage duplicates the existing { data, mimeType } ImageAttachmentDto. Reusing it, and typing the component as NonNullable<ToolEntry["images"]>, removes an exported duplicate type.

  6. Centralize normalized tool-content assignment (confidence 95) — packages/dashboard/src/client/state/reducer.ts:322-328,508-533. Hydration, update, and final paths repeat contentToParts() plus field assignment. A small applyToolContent(entry, content) helper would reduce drift and give the stale-image replacement semantics one home.

  7. Memoize renderable images (confidence 92) — packages/dashboard/src/client/components/transcript.tsx:364-370. The accessor filters into a new array separately for Show and For; use the already-imported createMemo() for the derived list.

  8. Trim brittle or redundant test mechanics (confidence 93-99) — packages/coding-agent/test/export-html-images.test.ts:113-158, packages/dashboard/test/client/screens.test.tsx:3980-4008. Once the export test loads the generated page, remove regex/eval machinery, existsSync and non-empty assertions implied by later reads, and simplify screen selectors to assert the final image attribute directly.

  9. Make the CSS comment match generic behavior (confidence 99) — packages/dashboard/src/client/styles/app.css:795-799. The comment says bytes are resize-capped by read, which is not true for arbitrary image-producing tools. Describe only the layout bound the CSS actually provides.

Strengths

  • Dashboard extraction and rendering are generic across tool names and independent of model vision support.
  • Live final events, refresh hydration, parent resync, and subagent snapshots converge on the same reducer parsing path.
  • The dashboard path uses a raster MIME allowlist, plausible-base64 validation, a plain DOM src attribute, and a second render-time MIME gate.
  • The existing oversized final-event recovery chain is coherent: the event hub emits a resync barrier and hydration can reconstruct persisted full messages without raising replay-buffer limits.
  • Tests already cover live end/update extraction, hydration, text-only results, malformed/SVG rejection, exact data URIs, generic dashboard tools, and export payload round-tripping.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier


Reviewed by mach6

@maxscheurer

Copy link
Copy Markdown
Contributor Author

Review Assessment

Review comment

Classifications

Finding Classification Reasoning
1. Export image HTML injection genuine template.js interpolates unvalidated mimeType and data into markup later assigned through innerHTML; an attribute-breaking MIME can create an event handler. Export MIME/base64 safety is part of the stated acceptance criteria.
2. Unbounded image recovery costs deferred This is real hardening work for generic tools, but read currently resize-caps/omits large images, the transcript renders a 150-entry window, and SSE frames are bounded. Add transport/aggregate limits in a follow-up issue rather than block this PR.
3. Missing end-to-end oversized recovery test false-positive Existing event-hub and server tests cover oversized event → barrier → snapshot behavior, store tests cover hydration, and the new reducer test covers image extraction from hydrated messages. The new behavior is covered across these layers.
4. Export test bypasses dispatch nitpick Regex/new Function is brittle and does not prove dispatch, but it does execute the real helper body. A browser-level test would be better test design, not a separate correctness defect.
5. Stale partial images after final result genuine The final handler only assigns images when the final list is non-empty, so a prior partial image survives a text-only, invalid, empty, or string authoritative result.
6. Oversized partial images cannot resync nitpick Partial updates are transient and best-effort by design; authoritative finals are persisted. Clearing state correctly on the final event addresses the practical asymmetry.
7. Export drops non-read tool images genuine renderResultImages() is called only in the read branch, while dashboard rendering and the PR's parity/genericity goals apply to any image-producing tool.
8. Four formats lack full-pipeline tests nitpick The rendering path is identical across the allowlist; current tests cover PNG/WebP rendering, JPEG extraction, and rejected SVG. GIF coverage would improve completeness but is not a distinct defect.
9. Reuse ImageAttachmentDto nitpick The shapes match, but a separate sanitized render-state type has defensible semantics and documentation. This is a DRY preference.
10. Centralize content assignment nitpick A helper could reduce duplication, but the repeated code is small; the stale-state bug can be fixed directly without requiring the refactor.
11. Memoize image filtering nitpick This is a micro-optimization over a tiny image array and has no correctness impact.
12. Trim test mechanics nitpick The suggested cleanup improves test readability but does not alter production behavior. It substantially overlaps finding 4.
13. Inaccurate generic CSS comment nitpick The comment wrongly assumes every image is resize-capped by read; custom tools need not be. This is a valid one-line documentation cleanup.

Action Plan

  1. Fix finding 1: sanitize exported tool/message images with the exact raster MIME allowlist and strict base64 validation; avoid unsafe raw attribute interpolation; add malicious-input regressions.
  2. Fix finding 7: render sanitized result images from the common export tool-result path and test a non-read image-producing tool.
  3. Fix finding 5: make tool_execution_end authoritative by clearing images for string, empty, invalid, or text-only finals; add an image-update → text-final lifecycle test.
  4. Optionally include the trivial comment correction from finding 13 while touching the affected files.
  5. Create a follow-up issue for finding 2 covering bounded/lazy image transport, per-image and aggregate limits, and explicit omission behavior for arbitrary tools.

Fix before merge: findings 1, 5, and 7.


Assessment by mach6

@maxscheurer

Copy link
Copy Markdown
Contributor Author

Progress Update

Fixed the three genuine review findings selected for this batch:

  • Finding 1 — export image sanitization: exported tool-result and message images now require an exact PNG/JPEG/GIF/WebP MIME allowlist and strict base64 validation before a data URI is rendered. Crafted MIME values, SVG, malformed base64, and markup payloads are rejected.
  • Finding 5 — stale partial images: content-bearing tool updates and authoritative final results now replace image state consistently, clearing earlier images when the newer snapshot/result has none. Events with no replacement content preserve the existing fallback behavior.
  • Finding 7 — generic export parity: sanitized tool-result images are appended from the common export rendering path, so custom and other non-read tools retain images in exported transcripts.

Added dispatch-level export regressions for valid read, custom tools, message images, and malicious values, plus reducer lifecycle tests covering image updates followed by text, string, invalid, empty, or absent final content.

Verification completed:

  • targeted tests: 48 passed
  • pre-commit full suite: 4,882 passed, 0 failed, 701 skipped
  • npm run check
  • npm run build
  • workspace-link verification
  • git diff --check

Commit: c1181d9


Progress tracked by mach6

@m-aebrer

m-aebrer commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

unblocked since #388 merge (in a moment)

Will likely need some reconciliation with the latest master and then confirmation that it still works. Once confirmed good tag me for review and I'll publish :)

@maxscheurer

Copy link
Copy Markdown
Contributor Author

Progress Update

Reconciled this branch with the latest master (which now includes the SSE-pushed fleet snapshot rework, PR #388) and confirmed the image feature still works end to end.

Reconciliation

  • Merged origin/master into the branch. All seven files this PR touches were also reworked by Dashboard: eliminate REST fleet refresh with SSE-pushed snapshots #388 (reducer, transcript, app.css, export-html template + tests), but the merge auto-resolved with zero conflicts and the image code survived intact.
  • Build clean; targeted dashboard + export suites pass (reducer, event-hub, server, client screens/store, export-html-images), and the full pre-commit suite passed (4965 passed, 0 failed).

Compatibility with the SSE-snapshot rework — confirmed

  • All transcript-building paths route through the image-aware messagesToEntries — active hydrate, subagent hydrate, /api/resync snapshot, and subagent snapshot — so images render after any resync/hydrate.
  • The SSE-pushed fleet_snapshot event carries no messages (lightweight FleetRuntimeSnapshotDto: state, background agents, timestamps, truncated preview only), so it transports no image bytes — no interaction with the 1 MB frame budget.
  • Full-message hydration stays on HTTP (/api/resync), which has no SSE frame cap, so large images continue to ride the pre-existing oversized-event -> resync -> HTTP hydrate path.

Additional fix

  • Corrected the .tool-image CSS comment (review finding 13): it wrongly claimed all image bytes are resize-capped by read, which is untrue for generic image-producing tools. Comment now describes only the layout bound it actually provides.

Deferred work

  • Review finding 2 (bounded/lazy image transport, per-image + aggregate limits, explicit omission state, low-data mode) is intentionally out of scope here and tracked in the follow-up issue for a dashboard low-data image mode.

Ready for review.

Commit: 1b3dd53 (CSS comment fix), on top of the master reconciliation merge d3d55bc.


Progress tracked by mach6

@maxscheurer

Copy link
Copy Markdown
Contributor Author

Code Review (re-review after master reconciliation)

This is a second review pass, run after the branch was reconciled with master (PR #388, the SSE-pushed fleet snapshot rework). The prior review's genuine findings (1, 5, 7) and the bonus CSS comment fix (13) were all confirmed resolved; finding 2 remains legitimately deferred to follow-up issue #398. The reconciliation itself introduced no regressions.

Critical

None.

Important

None.

Suggestions

  1. Mixed valid + invalid image arrays are untested (confidence 82) — packages/dashboard/src/client/state/reducer.ts (contentToParts, ~226-246), mirrored in packages/coding-agent/src/core/export-html/template.js (renderResultImages, ~928) and packages/dashboard/src/client/components/transcript.tsx (ToolResultImages, ~365). Every test uses either a single valid image (all-valid) or only invalid images (all-invalid → undefined). No test covers a content array with one allowlisted image plus one disallowed/malformed block. A regression that bailed on the first bad block (e.g. .every() instead of per-item filter, or an early continue/return) would hide legitimate images yet still pass both existing shapes. Add a test asserting content: [text, validPng, svg, malformedPng] yields exactly images: [validPng] in the reducer, one <img> in template.js, and one img.tool-image in the transcript. Worth also asserting two valid images render as two tags (multi-image only exercised implicitly).

  2. Extract a shared applyResultSnapshot helper (confidence 88) — packages/dashboard/src/client/state/reducer.ts:508-548. The tool_execution_update and tool_execution_end branches contain byte-for-byte identical snapshot logic (string → clear images; ?.content → authoritative set/clear; else preserve), differing only in the local variable name. A small helper removes the duplication with no behavior change; the end case keeps its extra status/details/endedAt assignments outside the helper. Well-guarded by the new reducer test suite.

  3. Memoize renderable() in ToolResultImages (confidence 85) — packages/dashboard/src/client/components/transcript.tsx:364-376. The plain accessor runs .filter() on every read and is read twice per render (Show when + For each). Wrap in createMemo (already imported and used elsewhere in this file) to compute once. Micro-optimization over a tiny array; behavior-preserving.

  4. Redundant early-return in renderResultImages (confidence 82) — packages/coding-agent/src/core/export-html/template.js:924-930. The if (images.length === 0) return '' guard is subsumed by the subsequent if (tags.length === 0) return '' (mapping over an empty array is a no-op). Drop the first check.

Strengths

  • Reconciliation is clean. Every transcript-building path (active hydrate, subagent hydrate, /api/resync snapshot, subagent disk snapshot, and message_end replace) routes through the image-aware messagesToEntries. The SSE FleetRuntimeSnapshotDto carries no messages, so no image bytes touch the 1 MB frame budget; full bytes ride the uncapped HTTP /api/resync path.
  • XSS surface is closed. Dashboard has two independent gates (reducer allowlist + base64 validation, then a transcript re-check), renders only via a data: URI on the src attribute (no HTML sink), and excludes SVG. Export template.js now validates through sanitizeImageDataUri (exact MIME allowlist + strict base64) for both tool-result and message images before interpolation.
  • Clearing/preservation state machine is correct in all orderings and thoroughly tested (text-after-image, string-after-image, empty-array, all-invalid, no-result fallback, update-supersedes-update). The messagesToEntries replay path correctly never needs to clear (entries are freshly built).
  • isLikelyBase64 is safe on large payloads — the regex has no nested quantifiers (linear, no catastrophic backtracking), and Buffer.toString("base64") output isn't falsely rejected.
  • Export test harness genuinely proves dispatch — it evaluates the real shipped template.js and drives renderToolCall/renderEntry, covering both read and a non-read custom tool through the generic post-switch image path; brittleness points fail loudly rather than false-green.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier


Reviewed by mach6

@maxscheurer

Copy link
Copy Markdown
Contributor Author

Review Assessment

Review comment

This assessment independently verified each finding against the actual source. Notably, the assessor confirmed all three image-filtering layers are genuinely per-item (loop-push / .map().filter(Boolean) / .filter()) — there is no first-bad-block bailout, so the code is correct today. The prior-cycle genuine findings (1, 5, 7) remain resolved and the master reconciliation introduced no regressions.

Classifications

Finding Classification Reasoning
1. Mixed valid+invalid image arrays untested genuine (low severity) Code is correct (per-item in all three layers), but no test covers a valid+invalid mix or a two-valid multi-render. Per the completeness rule, test gaps for new code should ship with the PR. Cheap to add.
2. Extract applyResultSnapshot helper nitpick The tool_execution_update/end snapshot blocks are byte-for-byte identical modulo variable name — real duplication, but a pure maintainability refactor with no correctness impact.
3. Memoize renderable() nitpick renderable() filters twice per render; correct observation, but the array is tiny and there's no correctness issue. Marginal cleanup.
4. Redundant early-return in renderResultImages nitpick The images.length===0 guard is subsumed by the later tags.length===0 guard. Correct, but a harmless stylistic micro-exit.

Action Plan

  1. Finding 1 — add mixed valid+invalid image tests (low severity, but ship with the PR per the completeness rule). Cover all three layers so a future first-bad-block regression can't silently hide surviving valid images:
    • reducer.test.ts: content array with one allowlisted PNG plus one SVG/malformed block → assert images contains exactly the valid one (not undefined).
    • export-html-images.test.ts: mixed array → assert exactly one <img class="tool-image">, invalid block absent.
    • screens.test.tsx: two valid images → assert two img.tool-image (multi-render); optionally a mixed array asserting only the valid one renders.
  2. Findings 2–4 — optional polish, not merge-blocking. Finding 2 (shared snapshot helper) is the most defensible given small future-drift risk, but none affect correctness. Apply at author's discretion.

Fix before merge: Finding 1 (tests). No new tracking issues required — findings 2–4 are optional and finding 2's deferred hardening work is already tracked in issue #398.


Assessment by mach6

Add per-item filtering coverage across the reducer, transcript, and HTML
export layers: a valid image mixed with disallowed/malformed siblings must
still surface the valid one, and multiple valid images must all render.
Guards against a future regression that bails on the first bad block.
@maxscheurer

Copy link
Copy Markdown
Contributor Author

Progress Update

Addressed the one genuine finding from the re-review (finding 1 — mixed valid+invalid image arrays untested). The image-filtering code was already correct (per-item in all three layers), so this is coverage that guards against a future first-bad-block regression.

Added tests across all three layers:

  • reducer.test.ts — a mixed content array (text + SVG + valid PNG + malformed PNG) surfaces exactly the valid PNG; plus a multiple-valid-images case (PNG + WebP).
  • screens.test.tsx — mixed allowed/disallowed images render only the single allowed <img>; plus a two-valid-image multi-render assertion.
  • export-html-images.test.ts — new case (d): a mixed tool-result array driven through the real renderToolCall dispatch emits exactly one sanitized <img> and leaks no SVG/malformed markup.

The three simplifier suggestions (shared applyResultSnapshot helper, createMemo on renderable(), redundant early-return) were assessed as nitpicks and left as optional polish.

Verification:

  • targeted suites: reducer (43) + export-html-images (8) + screens (136) all green
  • full pre-commit suite: 4970 passed, 0 failed, 706 skipped
  • biome clean

Commit: 2b89a46


Progress tracked by mach6

@m-aebrer
m-aebrer merged commit 2103fbd into aebrer:master Jul 24, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dashboard discards image content blocks from tool results, so images never render inline

2 participants