Skip to content

emrg: the GUI renders a local image from a path, and the prompt says so - #2639

Merged
pm25coder merged 2 commits into
masterfrom
feature/gui-local-images
Oct 29, 2026
Merged

pm25coder merged 2 commits into
masterfrom
feature/gui-local-images

Conversation

@argszero

@argszero argszero commented Oct 29, 2026 •

Copy link
Copy Markdown
Owner

Closes #2638

What changed

The GUI chat rendered no local image at all. Two filters did it, and each is fixed where it lives.

  1. emrg/gui/renderer/src/lib/markdown.ts — sanitization was a bare DOMPurify.sanitize(html), so DOMPurify's default URI policy decided every attribute; file: is not in that policy, and the src was dropped, leaving <img alt="shot">. An uponSanitizeAttribute hook now sets forceKeepAttr for exactly one case, isLocalImageSrc(tag, attr, value) — img + src + a file: value. In DOMPurify 3.4.13 forceKeepAttr bypasses only the URI/allow-list check: the SAFE_FOR_XML and attributename checks still run, so this is a narrow allowance rather than a widened policy. javascript:, and file: on any other element or attribute, keep walking the default path and are still stripped.
  2. The same file registers marked's image renderer, which normalises the root-absolute form before it reaches the DOM: /Users/me/shot.png becomes file:///Users/me/shot.png. The renderer is a file:// document (main.js builds it with loadFile), so /… would otherwise resolve against the application's own root rather than the host's filesystem. normalizeImageSrc leaves everything else alone: other schemes, protocol-relative URLs, relative paths, and Windows drive paths (which become file:///C:/…, since a single letter is not a scheme).
  3. emrg/server/prompts/system.j2 gains ### Images in a reply inside ## 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 a data: 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, because system.j2 is held to zero literal digits (tests/test_system_prompt_structure.py) and that name carries two.
  4. emrg/gui/renderer/src/shell.css caps a chat image's width at its container; a local screenshot is often thousands of pixels wide.

Verification

Renderer: npm run typecheck clean, 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:

  • ![shot](file:///Users/me/shot.png) → an <img> carrying src="file:///Users/me/shot.png"
  • ![shot](/Users/me/shot.png) → the same, after normalisation
  • <img src="javascript:alert(1)"> and <a href="javascript:alert(2)"> → no javascript: survives
  • [open](file:///Users/me/shot.png) → the href is still stripped

Mutation arms, each printed before it ran, each restored and re-run green afterwards:

removed result
the file: hook the two image tests go red with expected '<p><img alt="shot"></p>' — precisely the broken-image shape the issue describes
the /abs normalisation the unit test and the real-vendor test go red with expected '/Users/me/shot.png' to be 'file:///Users/me/shot.png'
the block, by wrapping it in {# … #} the file leg still passes and the render leg fails, which is why both legs exist

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" and uv run python -m emrg --help clean.

Agent.md's renderer total moves 740 → 752. scripts/check-node-test-count.py --write fixed the headline and left the per-file breakdown stale, as it does; 13 markdown / 4 vendorMarkdown → 21 / 8 was corrected by hand, and tests/test_doc_counts.py now passes.

CI: pending on the current head fd225e42. The first head (8add424b) ran red on the test leg, and the cause was mine: tests/test_system_prompt_structure.py::test_the_template_carries_no_digit_at_all holds system.j2 to 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> carrying src="file:///…" reaches the document. Painting it depends on the renderer's origin being file:// (it is — main.js uses loadFile) and on the CSP already listing file: in img-src (it does — renderer/src/index.html). Under vite dev the document is served from an http origin, where Chromium refuses a file: subresource whatever the CSP says; that is a property of the development server, not of the shipped path.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

@pm25coder
pm25coder merged commit 7d4db75 into master Oct 29, 2026
2 checks passed
@argszero argszero mentioned this pull request Oct 29, 2026
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.

emrg: the GUI chat renders no local image, leaving base64 as the only way to show one

2 participants