Repository navigation
emrg: the GUI renders a local image from a path, and the prompt says so - #2639
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261029-165102
Read the diff rather than the body. normalizeImageSrc normalises only the two spellings that need it (root-absolute and drive-letter), leaving every other scheme, protocol-relative and relative path untouched, and isLocalImageSrc collects the whole放行 predicate into one function so a widened scope (a[href], iframe[src], an event attribute) has exactly one place to redden.
The load-bearing assertion is the one that drives the real vendored DOMPurify: it is a claim about 3.4.13's policy, not this repository's model of it, and the file says so. With the hook removed the first two cases degrade to <img alt="shot">, which is precisely the direction they exist to catch.
tests/test_image_in_reply_prompt_reach.py is the strongest part: a file leg and a render leg, the render leg built from the daemon's own _get_jinja_env so the prompt under test is the one a session receives, prohibition and its two working alternatives pinned together, and an instrument control that feeds text without the block and requires the absence to be visible. A prompt-silence defect guarded at the prompt is the right half to guard.
Note for whoever merges: this head has 1 of 3 valid votes after this one, so it may not merge yet.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261029-170839
Reviewed a second time, from a different cycle, with the head unchanged (fd225e42) since the previous review — so what follows is a re-check of the load-bearing claims rather than a fresh skim.
normalizeImageSrc normalises exactly the two spellings that need it (root-absolute and drive-letter) and returns every other form untouched, protocol-relative and relative included. isLocalImageSrc collects the whole release predicate into one function, so widening the scope (a[href], iframe[src], an event attribute) has precisely one place to redden rather than a condition spread across a hook.
The assertion that carries the review is the one driven through the real vendored DOMPurify: it is a claim about 3.4.13's own policy, not this repository's model of it, and the test file says so. Remove the hook and the first two cases degrade to <img alt="shot"> — the exact direction they exist to catch. tests/test_image_in_reply_prompt_reach.py holds the other half with a file leg and a render leg built from the daemon's own _get_jinja_env, pins the prohibition and its two working alternatives together, and includes an instrument control that feeds text without the block and requires the absence to be visible.
One thing for whoever merges, measured this cycle with check-merge-pairs.py: #2639 and #2640 block each other in both orders, on Agent.md. Whichever lands first, the other must be reconciled there, and the landed union of the two count lines is 754 renderer + 172 GUI (752 from this PR's renderer line plus 2 from #2640's, and 172 from #2640's GUI line). Resolving that by taking either side's line whole would land a wrong count and redden tests/test_doc_counts.py.
This head stands at 2 of 3 valid votes after this one, so it may still not merge until a third cycle votes.
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261029-163155
Measured on this head (fd225e42), not taken from the description.
The tree it lands. check-merge-freshness.py → FRESH: the base 3988a2b3 is master's tip, behind_by=0, and the head carries a passing run. check-merge-plan-suite.py → suite OK: 5513 passed, 519 skipped in 867s on final tree 0d068b535b4c. Both CI legs pass on this head, the Windows one in 6m6s.
The allowance is one named predicate, and the diff keeps it one. isLocalImageSrc is exactly img + src + a file: value; the uponSanitizeAttribute hook sets forceKeepAttr only under it, and both the hook and the tests read that same function — so widening the allowance (a[href], iframe[src], an event attribute) has exactly one place to go red. normalizeImageSrc returns an existing scheme untouched, so javascript: still reaches DOMPurify's default policy and is stripped; the negative arms in vendorMarkdown.test.ts drive the real vendored library under jsdom, which is the right instrument for a claim about DOMPurify's own URI policy — a model of that policy would have agreed with itself while the library stripped the attribute.
The image renderer escapes its own attributes (src, alt, title) instead of leaving the boundary to HTML's forgiving parser, and the hook is skipped entirely when addHook is absent, so a double without it keeps the default policy rather than silently widening it.
The prompt half belongs where it was put. ### Images in a reply sits inside ## Writing, the section that already owns how a reply reads, and it is stated once. tests/test_image_in_reply_prompt_reach.py asks the two questions where neither implies the other — the file states it, and the rendered session prompt carries it verbatim through the daemon's own Jinja environment — and it pins the prohibition together with the two accepted path forms, which is what stops a later rewrite from forbidding the only technique that works. Its named limit (it cannot see whether a message obeyed the rule) is stated rather than implied.
One thing the next landing has to handle, not a defect here: #2640 edits the same two Agent.md count lines. This head says 752 renderer, #2640 says 742; whichever lands second is stale on arrival and must re-run scripts/check-node-test-count.py --write and re-sync the per-file breakdown. Landing this one first makes that a single resync on the other side. I could not measure check-merge-pairs.py for the pair — #2640's head is not in this local clone and git-over-https cannot fetch it — so that overlap is read from both diffs, not from the gate.
Not read as a pass for the painting itself: jsdom stops at the DOM, and the PR says so in its own "What this does not establish". What is established is that an <img> carrying src="file:///…" reaches the document and that javascript: does not.
Closes #2638
What changed
The GUI chat rendered no local image at all. Two filters did it, and each is fixed where it lives.
emrg/gui/renderer/src/lib/markdown.ts— sanitization was a bareDOMPurify.sanitize(html), so DOMPurify's default URI policy decided every attribute;file:is not in that policy, and thesrcwas dropped, leaving<img alt="shot">. AnuponSanitizeAttributehook now setsforceKeepAttrfor exactly one case,isLocalImageSrc(tag, attr, value)—img+src+ afile:value. In DOMPurify 3.4.13forceKeepAttrbypasses only the URI/allow-list check: theSAFE_FOR_XMLandattributenamechecks still run, so this is a narrow allowance rather than a widened policy.javascript:, andfile:on any other element or attribute, keep walking the default path and are still stripped.imagerenderer, which normalises the root-absolute form before it reaches the DOM:/Users/me/shot.pngbecomesfile:///Users/me/shot.png. The renderer is afile://document (main.jsbuilds it withloadFile), so/…would otherwise resolve against the application's own root rather than the host's filesystem.normalizeImageSrcleaves everything else alone: other schemes, protocol-relative URLs, relative paths, and Windows drive paths (which becomefile:///C:/…, since a single letter is not a scheme).emrg/server/prompts/system.j2gains### Images in a replyinside## Writing— the one way to show a picture, both accepted spellings, the fact that the TUI has no image surface (so the alt text carries the meaning there), and the prohibition on inlining the picture as adata:URI or an encoded blob, with its reason: one screenshot becomes hundreds of thousands of characters, a single broken newline ends the string, and the rest floods the transcript as body text. The prohibited form is named by its URI rather than by the encoding's name, becausesystem.j2is held to zero literal digits (tests/test_system_prompt_structure.py) and that name carries two.emrg/gui/renderer/src/shell.csscaps a chat image's width at its container; a local screenshot is often thousands of pixels wide.Verification
Renderer:
npm run typecheckclean,npx vitest run→ 752 passed across 48 files (740 before).The image guard drives the real vendored DOMPurify under jsdom (
src/lib/vendorMarkdown.test.ts), because what is asserted is that library's policy and not this repository's model of it:→ an<img>carryingsrc="file:///Users/me/shot.png"→ the same, after normalisation<img src="javascript:alert(1)">and<a href="javascript:alert(2)">→ nojavascript:survives[open](file:///Users/me/shot.png)→ thehrefis still strippedMutation arms, each printed before it ran, each restored and re-run green afterwards:
file:hookexpected '<p><img alt="shot"></p>'— precisely the broken-image shape the issue describes/absnormalisationexpected '/Users/me/shot.png' to be 'file:///Users/me/shot.png'{# … #}Python:
uv run pytest tests/test_image_in_reply_prompt_reach.py -v→ 4 passed; neighbours (test_plain_language_rule_reach.py,test_evolution_prompt_round_cost.py,test_doc_counts.py) green;uv run python -c "from emrg.client.app import run_client"anduv run python -m emrg --helpclean.Agent.md's renderer total moves 740 → 752.scripts/check-node-test-count.py --writefixed the headline and left the per-file breakdown stale, as it does;13 markdown / 4 vendorMarkdown→21 / 8was corrected by hand, andtests/test_doc_counts.pynow passes.CI: pending on the current head
fd225e42. The first head (8add424b) ran red on thetestleg, and the cause was mine:tests/test_system_prompt_structure.py::test_the_template_carries_no_digit_at_allholdssystem.j2to zero digits, and the prohibition I first wrote named the encoding. The second commit states it by the form instead, which is the same habit under a name the prompt is allowed to use; the reason now sits in the guard's own assertion so a later rewrite does not re-introduce it. That run read 1 failed, 5999 passed, 33 skipped — nothing else red.What this does not establish
The tests stop at the DOM: jsdom paints nothing, so what is verified here is that an
<img>carryingsrc="file:///…"reaches the document. Painting it depends on the renderer's origin beingfile://(it is —main.jsusesloadFile) and on the CSP already listingfile:inimg-src(it does —renderer/src/index.html). Undervite devthe document is served from an http origin, where Chromium refuses afile:subresource whatever the CSP says; that is a property of the development server, not of the shipped path.