Review — end-to-end ✅ (approved after fix pushed)

What this ships

Targeted follow-up to PR #1109 / #1117 — fixes the two remaining bugs in #1095:

1. Composer tray now shows image thumbnails instead of paperclip chips. renderTray() checks _IMAGE_EXTS per file, builds a URL.createObjectURL(f) blob URL, renders an <img class="attach-thumb"> chip, and revokes the blob on remove. Non-image files keep the paperclip chip. (static/ui.js:3239-3257, static/style.css:644-647)
2. Chat history images load correctly — switched from api/media?path=FILENAME (404, expects absolute filesystem path) to api/file/raw?session_id=SID&path=FILENAME (resolves filename against the session's workspace, which is exactly where api/upload writes). (static/ui.js:2257-2266)

Traced against upstream hermes-agent

Pulled fresh nousresearch/hermes-agent tarball. Both fixes are pure WebUI-frontend changes (static/ui.js + static/style.css) with no agent-side coupling. The backend api/file/raw handler at api/routes.py:2220-2272 is webui-internal.

What I caught — accidental deletion of uploadPendingFiles early-return guard

The original PR diff removed the line

if(!S.pendingFiles.length||!S.session)return[];

from the top of uploadPendingFiles. Verified against master (git show origin/master:static/ui.js | sed -n '3245,3260p') — the guard was at line 3246 prior to this PR, and the deletion is not mentioned anywhere in the PR description. Mixed into the renderTray refactor diff context, presumably accidental.

Consequence: when uploadPendingFiles is called with S.session === null (transient state during session load, error recovery, or any broken init path), the line fd.append('session_id', S.session.session_id) at static/ui.js:3266 throws TypeError: Cannot read properties of null (reading 'session_id') instead of returning the empty array gracefully.

What I pushed — 8f8b968

Restored the one-line guard at the top of uploadPendingFiles. No other behaviour change. JS syntax clean. Full suite: 2540 passed.

End-to-end trace

api/file/raw endpoint (highest-stakes — change to a file-serving endpoint usage)

Verified at api/routes.py:2220-2272:

• Requires session_id query parameter (line 2222-2224).
• Loads session via get_session(sid) — handles missing sessions with 404 (line 2226-2228).
• Resolves the path parameter via safe_resolve(Path(s.workspace), rel) (line 2231) — path-traversal-safe: safe_resolve rejects relative paths that escape the workspace root.
• Forces Content-Disposition: attachment for dangerous MIME types (text/html, application/xhtml+xml, image/svg+xml) at line 2246-2256 to prevent XSS via uploaded HTML/SVG.
• For inline previews via ?inline=1, additionally sets Content-Security-Policy: sandbox allow-scripts at line 2269 — defense-in-depth even if accessed outside the iframe.

The PR's URL construction:

const imgUrl='api/file/raw?session_id='+encodeURIComponent(_attachSid)+'&path='+encodeURIComponent(fname);

Both query params are encodeURIComponentd. The rendered <img src="${esc(imgUrl)}"> then escapes the URL again for HTML attribute context. ✅

Composer tray thumbnail (renderTray)

static/ui.js:3239-3257:

if(_IMAGE_EXTS.test(f.name)){
  const blobUrl=URL.createObjectURL(f);
  chip.className='attach-chip attach-chip--image';
  chip.dataset.blobUrl=blobUrl;
  chip.innerHTML=`<img class="attach-thumb" src="${esc(blobUrl)}" alt="${esc(f.name)}" title="${esc(f.name)}"><button title="${t('remove_title')}">${li('x',12)}</button>`;
} else {
  chip.innerHTML=`${li('paperclip',12)} ${esc(f.name)} <button title="${t('remove_title')}">${li('x',12)}</button>`;
}
chip.querySelector('button').onclick=()=>{
  if(chip.dataset.blobUrl) URL.revokeObjectURL(chip.dataset.blobUrl);
  S.pendingFiles.splice(i,1);renderTray();
};

• f.name is escaped via esc() for both alt and title. ✅
• blobUrl (a blob: URI generated by the browser) is esc()d in the src attribute. ✅
• _IMAGE_EXTS is module-scope at static/ui.js:53, shared with renderMessages and renderMd. Reuse is correct.

XSS audit

• All user-controlled strings (f.name, fname) flow through esc() before reaching innerHTML. ✅
• The thumbnail path uses URL.createObjectURL, which produces opaque blob:<origin>/<uuid> URLs — not user-controllable.
• The chat-history imgUrl parameters use encodeURIComponent AND the result is escaped again. Belt-and-braces.

Other audit — things that are correct already

• CI green: ✅ test (3.11), ✅ test (3.12), ✅ test (3.13).
• safe_resolve path-traversal: an attacker submitting ?path=../../../etc/passwd is rejected at the workspace boundary inside _handle_file_raw.
• AVIF support: _IMAGE_EXTS already includes avif from a prior PR; new tray code inherits it.
• addFiles deduplication: existing if(!S.pendingFiles.find(p=>p.name===f.name)) prevents duplicate file objects. Combined with the new URL.createObjectURL per-file, dedup also avoids creating duplicate blob URLs for re-dropped files.
• _IMAGE_EXTS module-scope hoist: was done in v0.50.221 (#1117 absorb). The PR correctly uses it from the new call site.

Edge-case trace

Tests

• PR's tests (test_issue1095_pasted_images.py): 14 tests across TestComposerTrayThumbnails (8) + TestChatHistoryImageRendering (6). All pass.
• CI on PR: green on 3.11/3.12/3.13.
• Local full suite (after my fix): 2540 passed, 47 skipped, 0 PR-related failures.
• The PR-claimed count of 2588 vs. local 2540 is environment-dependent — onboarding-network integration tests skip on macOS.

Minor observations (non-blocking)

• Blob URL leak on upload-then-send path: when uploadPendingFiles() resets S.pendingFiles=[] and calls renderTray() (line 3278), the blob URLs that were attached to the now-removed chips are NOT revoked. The browser does GC the underlying File data eventually, but URL.createObjectURL references can accumulate. For a session with many uploads, this leaks. A small fix would be: before clearing pendingFiles, iterate the tray's [data-blob-url] chips and URL.revokeObjectURL() each. Not a blocker — magnitude is small (one per upload), browser GCs the underlying data, and it's bounded by session lifetime. Suggest as a follow-up.
• The PR diff also removed a blank line between addFiles and uploadPendingFiles — purely cosmetic, harmless.
• chip.dataset.blobUrl=blobUrl — modern browsers store dataset values as strings; the blob: URL is a string. ✅ no JSON-stringify oddity.

Recommendation

Approved (after fix pushed). Two real bugs in #1095 cleanly fixed, the new api/file/raw endpoint usage is path-traversal-safe and properly escaped, the blob URL handling has correct revocation on the explicit-remove path. The accidental deletion of the uploadPendingFiles guard was caught and restored in commit 8f8b968. No cross-tool agent regression. Parked at approval — ready for the release agent's merge/tag pipeline.

Re-review of new commits ✅ (re-approve, latest state clean)

Two commits landed after my prior approval at 8f8b968:

• f15406f — feat(ux): small thumbnails in chat + lightbox overlay on click
• d182deb — fix(tests): update assertions for lightbox

This re-review covers only the delta from 8f8b968..HEAD. The earlier approval scope (composer thumbnails + api/file/raw switch + my pushed guard restore) is unchanged.

What this ships (delta)

• 120×90 fixed-size chat thumbnails instead of full-bleed max-width:480px — chat rows are tighter, multiple images can sit on one line. (static/style.css:618-619)
• Lightbox overlay on click — new _openImgLightbox(src, alt) / _closeImgLightbox(lb) pair at static/ui.js:54-81. All 5 image render sites switched from onclick="this.classList.toggle('msg-media-img--full')" (CSS class toggle on the img itself) to onclick="_openImgLightbox(this.src,this.alt)" (full-screen overlay).
• Three test files updated to assert the new behaviour — test_issue1095_pasted_images.py, test_issues_853_857.py, test_media_inline.py. The .msg-media-img--full class assertions were replaced with _openImgLightbox / .img-lightbox assertions, plus the thumbnail width assertion bumped from max-width → width:120px.

Security audit (delta)

Inline onclick handler — no XSS surface

All 5 occurrences write the literal string _openImgLightbox(this.src,this.alt) into the onclick= attribute. Verified by grep:

849:  ... onclick="_openImgLightbox(this.src,this.alt)">`
928:  ... onclick="_openImgLightbox(this.src,this.alt)">`
992:  ... onclick="_openImgLightbox(this.src,this.alt)">`
999:  ... onclick="_openImgLightbox(this.src,this.alt)">`
2296: ... onclick="_openImgLightbox(this.src,this.alt)">`

this.src and this.alt are runtime DOM property reads, not template substitutions. The user-controlled values flow into src= and alt= attributes (both esc()'d in their respective interpolations), and at click time the browser provides their parsed values to the handler. No string concatenation breakout possible.

CSP impact: existing script-src 'unsafe-inline' already permitted inline event handlers, so no new directive is needed. The PR is moving from one inline handler to another — net neutral.

Lightbox internals

static/ui.js:54-81:

function _openImgLightbox(src, alt) {
  const lb = document.createElement('div');
  lb.className = 'img-lightbox';
  lb.setAttribute('role', 'dialog');
  lb.setAttribute('aria-label', alt || 'Image');
  const img = document.createElement('img');
  img.src = src;            // property assignment, not innerHTML
  img.alt = alt || '';      // property assignment, not innerHTML
  img.onclick = e => e.stopPropagation();
  ...
  lb.onclick = () => _closeImgLightbox(lb);
  document.body.appendChild(lb);
  lb._escHandler = e => { if(e.key==='Escape') _closeImgLightbox(lb); };
  document.addEventListener('keydown', lb._escHandler);
}

• img.src = src and img.alt = alt are property setters, not innerHTML assignments — no HTML parsing path. A javascript: URL in src is not honoured by <img> elements (browsers reject non-fetch schemes). ✅
• setAttribute('role', 'dialog') and setAttribute('aria-label', alt) — setAttribute on these attributes is plain-text. ✅
• The escape-key handler is stored on lb._escHandler so _closeImgLightbox can removeEventListener the same function reference (avoids the common "anonymous function can't be removed" leak). ✅
• _closeImgLightbox guards with if(!lb || !lb.parentNode) return; — double-close is safe.

Behavioural harness

11/11 assertions pass with a Node + DOM-shim harness:

Test1 lightbox created: PASS
  role: PASS
  aria-label: PASS
  has img child: PASS
  has close button: PASS
  esc handler registered: PASS
Test2 img click stops propagation: PASS
Test3 escape closes lightbox: PASS
  esc handler removed: PASS
Test4 backdrop click closes: PASS
Test5 no alt → aria-label fallback: PASS
Test6 double-close safe (no throw): PASS

This proves: the dialog is created with proper ARIA, the image-click stops bubbling so the lightbox stays open when interacting with the image, the escape key closes AND deregisters its handler, the backdrop click closes, missing alt falls back to "Image", and double-close is idempotent.

Edge-case trace (delta)

Other audit — confirmed correct

• Animation cleanup: setTimeout(120ms) removes the element after the reverse animation. Multiple clicks of close → multiple setTimeouts, but they all guard via lb.parentNode && before removeChild. ✅
• Memory: each lightbox has at most one keydown listener registered, removed in _closeImgLightbox. No leak.
• CSS: new .img-lightbox rule uses position:fixed;inset:0;z-index:9999 — covers the entire viewport, sits above all other UI including modals (other modals use lower z-index per existing code).
• Existing test compatibility: the old test at test_msg_media_img_full_class_defined was updated to assert .img-lightbox instead of .msg-media-img--full — correct migration.
• Visual regression: chat images now appear at fixed 120×90 vs. responsive max-width:480px. Some users may notice. The lightbox compensates for inspecting full-size. Pattern matches Slack/Discord image previews.
• My pushed fix 8f8b968 is still in the PR: verified the if(!S.pendingFiles.length||!S.session)return[]; guard remains at static/ui.js:3260.

Tests (delta)

• CI on latest commit (d182deb): ✅ test (3.11), ✅ test (3.12), ✅ test (3.13).
• Local full suite: 2540 passed, 47 skipped, 0 PR-related failures.
• Three test files migrated to assert lightbox behaviour; assertions are precise (e.g. width:120px in rule, _openImgLightbox in body, .img-lightbox in css).
• Behavioural harness covers all 6 happy/edge paths for the new lightbox functions.

Minor observations (still non-blocking)

• The blob-URL leak on the upload-then-send path that I noted in the prior review is unchanged — still a small memory leak per upload, browser GCs eventually. Suggested as a follow-up.
• Lightbox could trap focus inside the dialog for full keyboard accessibility (screenreader users currently lose focus context when Escape closes). Not a blocker — the role/aria-label is already there, and the close button is keyboard-reachable. Could be tightened later.
• Programmatic re-open (without close) stacks lightboxes; only the most recently opened gets escape-keyed closed via its own handler, but others still have their handlers attached. Not user-reachable — the lightbox covers the page so the user can't click another image to trigger a second open. Worth noting for future refactors.

Recommendation

Approved (fresh approval on the new commits). Lightbox is a clean UX upgrade — semantic dialog markup, ARIA labelling, property-based DOM construction (no innerHTML parse), idempotent close, escape and backdrop click both close, behavioural harness confirms 11/11 expected behaviours. No new XSS surface (inline onclick handlers are literal strings; user-controlled values reach via DOM properties at runtime). My earlier guard restoration (8f8b968) is still in place. Parked at approval — ready for the release agent's merge/tag pipeline.