Images render for the author who wrote them and the reviewer who receives them - #16
Merged
Merged
Conversation
A relative image src has always been resolved against the document origin — attn://app, or the Vite dev server — which is not where the markdown file lives, so `` rendered as a broken glyph and only absolute https:/data: URLs ever appeared. The plumbing was already there: the attn:// handler serves any local file from attn://localhost/<abs-path>, which is what ImageViewer and MediaPlayer use. Nothing was pointing markdown images at it. resolveImageSrc() maps an authored src onto that URL, and an `image` NodeView applies it to the DISPLAYED src only. node.attrs.src is never written, which is the whole round-trip guarantee — `image` has no serializer override, so the attr goes to disk verbatim on every save. Encoding is the sharp edge: markdown-it has already percent-encoded the src, and the Rust handler truncates the path at the first '?' or '#' before decoding it. So each segment is decoded once, re-split (an encoded '/' IS a separator to the handler), normalised with '..' clamped at the root, and re-encoded per segment. The view is registered only where a resolver is supplied. The hosted app and the reviewer reading an owner's snapshot pass none — their relative srcs name files on someone else's disk — and keep stock toDOM, byte-identical DOM included. Workspace-relative assets in the hosted app are a separate follow-up. An asset that will not load gets a paper/ink card — an eyebrow, the alt text in serif, the filename in mono, announced as one image — in place of the platform's broken-image glyph. It says "didn't load", not "not found", because a present file with a MIME the webview declines fires the same event. Closes attn-cgev. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q6b8P61pFqkaCSmM83FkWn
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The absolute-URL fixture points at https://example.com/pixel.png, which 404s. That is deliberate — the E2E assertion is that the exact string survives the resolver untouched, and anchoring it on a live host would fail offline and in CI — but the surrounding prose read as a promise that the image would render, so the placeholder card looked like the bug the fixture exists to disprove. Say what the card means, and where to look instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q6b8P61pFqkaCSmM83FkWn
The image NodeView was registered only where a resolver existed, to keep hosted DOM byte-identical. The consequence: hosted fell back to the stock `toDOM` and rendered the platform's broken-image glyph — the exact thing this feature replaces — while native showed the card. Hosted cannot load a relative local asset at all: no filesystem behind `./diagram.png`, and no `https:` in its `img-src`. So the failure there is permanent until workspace-relative assets land, which makes it the failure most worth explaining, not least. Register the view everywhere; surfaces with no resolver render the authored src verbatim, as before, and differ from stock only in what they show when it fails. The reviewer-snapshot editor still gets no resolver — its relative srcs name files on the owner's disk — but now gets the card too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q6b8P61pFqkaCSmM83FkWn
Three ways the local collab harness reported success it had not earned. A stray relay on 8787 exposed all three at once, and between them the user saw only "the reviewer window never opens the shared doc". start_relay polled /health for a 200, which ANY process on the port answers. wrangler survives its own EADDRINUSE, so `kill -0 $RELAY_PID` stayed true while the runtime that failed to bind sat there dead — the harness ran both daemons against a stranger's relay and its unrelated Durable Object state. Refuse to start on a held port, naming the process holding it, and treat "Address already in use" in the log as fatal. `attn review join` printed "join request sent to the running attn daemon" and exited 0 once the socket WRITE succeeded; the daemon parsed the invite afterwards, so every later failure was invisible. Add ReviewJoinWait, which replies only after the join has run, and ReviewManager::join_blocking, which does the same work as the Join command arm and additionally returns the outcome. The CLI now names the room it joined, or exits non-zero saying why it could not. dev-collab offered exactly one invite paste and fell through to "Ctrl+C to stop" on failure, so a bad first attempt meant restarting everything. Loop until a join succeeds or the user submits an empty line. Fixes attn-1kvp, attn-q8gs, attn-0cnt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q6b8P61pFqkaCSmM83FkWn
First half of attn-udu8. A reviewer sees the placeholder card for every image because share_lifecycle.rs:632 lets a share carry only md/markdown/html/htm, so the bytes never leave the owner's machine. Before any of them can, something has to decide which are safe to send. This module is that gate, and is written as one rather than as a path helper. The document being scanned is often agent-authored — nobody read every line — so `` is a realistic thing to find in it. Every src is refused unless it is provably a plain, allowlisted image file inside the directory the user chose to share. Deliberately stricter than resolveImageSrc in markdown-layer.ts, which resolves srcs for DISPLAY on the machine that already holds the files: showing someone a file they can already open costs nothing, transmitting it is irreversible. So a filesystem-absolute src is displayed but never sent, and a src escaping the share root is displayed but never sent. Three details carry the weight. Symlinks are refused BEFORE canonicalising, because canonicalize() follows the link and would report the target as an ordinary file inside the root. An encoded %2F is split as a separator, because it becomes one before the file is opened. And srcs come from comrak, not a regex, so a path inside a code fence never causes a file read. Skips are recorded with a reason rather than dropped — an image silently missing from a review is the thing this feature exists to stop.
Second half of attn-udu8's owner side. Every markdown target is scanned before the selection is recorded — a wire path is only minted for a path in selected_paths, so an asset outside it publishes fine and then has no name the reviewer can look it up by — and each surviving image is published as a DocType::Asset snapshot down the same envelope path as a document. The relay still sees only ciphertext. The share root is what the user actually chose: a folder share confines to the folder, a single-file share to that file's own directory. An image that fails to publish is logged and skipped rather than added to publish_errors. A share fails when a DOCUMENT cannot be sent; losing a picture must not cost the reviewer the review. Skipped srcs are logged with the reason from review::assets, so "why is this image missing" is answerable from the daemon log rather than by guessing.
Workspace manifest hydration failed for every v3 reviewer, so no reviewer ever saw a multi-file workspace — or, since attn-udu8, any image. The check that gated it loaded the ROOM SECRET, on the stated belief that "every joined/owned room persists the invite secret before starting its transport". That was never true of a v3 join: the key tree is one-way (room_secret -> root_key -> read_capability_key -> leaves) and a reviewer only ever holds a leaf, so the check could not pass for the one party it was checking. It was also asking the wrong question. The manifest FileId it compared travels inside every manifest snapshot a reviewer receives, so knowing it is not evidence of having authored anything — a secret-derived identifier is a name, not a signature. What needs proving is authorship, so prove it: accept the manifest when the OWNER signed the event carrying it. Deliberately NOT re-deriving that FileId from read_capability_key. It would unbreak hydration in about one line and make forgery strictly easier, since every reviewer holds that key and could then mint a manifest that passes. The owner's public key rides in the invite fragment, which is the one channel that already carries room-opening secrets and, being a fragment, never reaches the relay — the party a pinned key exists to defend against. The joiner verifies the existing grant against it locally rather than trusting the relay's verdict, and pins it in RoomAccessV3. Verification is a full verify_event, not a signingKeyId compare, because replay re-verifies nothing of its own and a check that only held live would evaporate on restart. Both fragment parsers move together. Each re-renders through its own composer and compares, so a disagreement about field order would make each side reject the other's invites; PARITY_VIEW_FRAGMENT / PARITY_COMMENT_FRAGMENT are asserted verbatim on both sides, which nothing previously pinned. Note the skew is asymmetric: old invites still parse, but an OLD client meets `owner=` as an unknown field and refuses, so owner and reviewer builds move together. Rooms with no pinned key — owners, v2 joiners — keep the original check, and fail closed when they have neither. Also lets an asset carry a manifest entry (attn-udu8): assets were refused there only because nothing but a document had ever been a share target, and bytes with no binding are indistinguishable from bytes never sent. Fixes attn-lb7p, attn-lb7p.1, attn-lb7p.2, attn-lb7p.3, attn-yhv8.
The last mile of attn-udu8, and the original report: "we don't render the images when they are referenced". Owners were fixed first, then the manifest binding that carried assets (attn-lb7p); this is the reviewer finally seeing pixels instead of a placeholder. The bytes were already arriving. They stopped in the frontend store, which refuses inline assets on the applyEvent path — deliberately, because that method is shared with the hosted session where the payload is still sender-supplied and a manifest may be parked awaiting its entries. Widening that gate would have admitted assets on the untrusted lane too. So use the front door instead. ReviewUpdate::SnapshotCreated carried a pair of ids and had no production emitter at all — it was constructed only under cfg(test) — even though types.ts documents window.__attn__.reviewSnapshot as exactly this route. It now carries the snapshot, and the daemon emits one for each asset after rehydrate_snapshot_event has discarded the sender's value, re-read the blob locally, and checked its length and content hash against the signed BlobRef. Replay emits them too, or images would vanish on restart while a fresh join showed them. Reviewer-side resolution is its own module rather than a reuse of resolveImageSrc, which maps srcs onto `attn://localhost/<abs-path>` — on a reviewer's machine that is nothing, or an unrelated file at the same path. Document and asset wire paths both come from selected_share_wire_path, so a src resolved against the document's own path lands on the asset's key. One rule deliberately differs from the local resolver: `..` that would climb above the share root returns null rather than clamping, since clamping would let `../../chart.svg` resolve onto a root-level asset nobody referenced. data: rather than blob: — no decode (the bytes are already base64url), no object-URL lifetime to get wrong against Editor's full docView rebuild, and no dependency on what a custom-scheme origin permits. A src with no matching asset stays null so the card shows. A reviewer cannot tell "skipped by policy" from "not yet arrived", and inventing a URL for either would be worse than saying so. Verified live: reviewer joins a share of tests/fixtures/images.md and decodes 96x64, 96x64, 64x64 and 120x64; the remote and missing srcs keep their cards; data-src still holds the authored string on every node. Native only. The hosted session zeroes asset bytes after hashing them by design; carrying them into its IndexedDB replay is a separate change.
angusbezzina
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Started as "we don't render the images when they are referenced" (
attn-cgev). Finishing it meant fixing three things that turned out to be worse than the original bug.Closes
attn-cgev,attn-udu8,attn-lb7p(+.1/.2/.3),attn-yhv8,attn-1kvp,attn-q8gs,attn-0cnt.The original bug
never rendered. The image node put the authored src straight into<img src>, and the native document is served fromattn://app, so a relative src resolved against the app origin instead of the markdown file's directory.The plumbing already existed — the
attn://handler serves any local file fromattn://localhost/<abs-path>, whichImageVieweralready used. Nothing pointed markdown images at it.Fix:
resolveImageSrc(docPath, src)plus animageNodeView that applies it to the displayed src only.node.attrs.srcis never written — that is the round-trip guarantee, sinceimagehas no serializer override and the attr goes to disk verbatim on every save. A byte-exact serializer test over 13 src forms pins it.The placeholder card ("Image didn't load" + alt + filename) replaces the platform's broken-image glyph, and ships on every surface — a graceful state on only one of two surfaces is a half-landed feature.
Then: reviewers saw none of it
A reviewer has the document's text and nothing else, so every relative src failed on their machine. Three separate causes, in order of discovery:
1. A share couldn't carry an image at all.
share_lifecycle.rsadmitted onlymd/markdown/html/htm. Now referenced images are published asDocType::Assetsnapshots inside the encrypted envelope — the relay stays content-blind.New
src/review/assets.rsdecides which files may be sent, and is written as a security boundary rather than a path helper: the documents being scanned are often agent-authored, sois a realistic thing to find in one. Symlinks are refused beforecanonicalize()(which would follow the link and report the target as an innocent file inside the root);%2Fis split as a separator (it becomes one before the file is opened); srcs come from comrak, so a path inside a code fence never causes a file read.2. Manifest hydration failed for 100% of reviewers. Assets bind through the workspace manifest, and the check gating it loaded the room secret — which only owners hold. The v3 key tree is one-way (
room_secret → root_key → read_capability_key → leaves), so no reviewer could ever satisfy it. This meant no reviewer had ever seen a multi-file workspace, images aside.It was also the wrong question: that FileId travels inside every manifest a reviewer receives, so knowing it proves nothing. A secret-derived identifier is a name, not a signature. The manifest is now accepted when the owner signed it, verified against a key pinned from the invite — the standard resolution for shared-key group systems, where a shared key proves membership but never authorship.
Deliberately not re-derived from
read_capability_key: that unbreaks hydration in one line and makes forgery easier, since every reviewer holds that key.3. The bytes stopped in the frontend store.
applyEventrefuses inline assets on purpose — it is shared with the hosted session, where payloads are still sender-supplied. Rather than widen that gate, assets now arrive throughwindow.__attn__.reviewSnapshot, whichtypes.tsalready documented as this exact route and which had no production emitter at all (constructed only undercfg(test)). What travels it has been throughrehydrate_snapshot_event: sender's value discarded, blob re-read locally, length and content hash checked against the signedBlobRef.Verified live, not inferred
Reviewer joins a share of
tests/fixtures/images.mdand decodes 96×64, 96×64, 64×64, 120×64. The two remaining cards are correct — a remote URL and a missing file, neither ever transmitted.data-srcstill holds the authored string on every node.The manifest tests are mutation-verified: removing the
verify_eventcall kills exactly the forgery and tamper cases and leaves the other six passing.Collab harness (found while testing)
Three ways it reported success it hadn't earned. A stray relay on 8787 exposed all of them at once, and between them the visible symptom was only "the reviewer window never opens".
start_relaypolled/health, which any process on the port answers — and wrangler survives its ownEADDRINUSE, so the liveness check stayed true while the runtime that failed to bind sat there dead.attn review joinprinted success once the socket write succeeded; the daemon parsed the invite afterwards, so every later failure was invisible. Now waits for the real outcome and exits non-zero.dev-collab.shoffered exactly one invite paste, with no retry.Compatibility — read before merging
The v3 invite fragment gained
&owner=, and the skew is asymmetric. Old invites still parse (Option+ fail-closed fallback), but an old client meetsowner=as an unknown field and refuses the invite outright — both parsers have an allowlist and a canonical re-render gate. Owner and reviewer builds must ship together.Rust↔TS fragment parity was previously enforced by nothing; it is now pinned by shared literals asserted verbatim on both sides.
Scope limits, deliberate
/appcannot render local images at all (no filesystem, and nohttps:in itsimg-src). It shows the honest card.Gates
cargo fmt --check·clippy --all-targets -D warnings· 588 + 701 Rust tests · 137 web test files · both builds · route-bundle gate · binary-size gate.scripts/test-e2e.shstill exits 1 on two pre-existing Suite 2 failures ("Navigate to basic.md", "Breadcrumb shows nested path"), reproduced on a clean stashed tree at this branch point. Unrelated, and the script is not part of CI. Worth its own issue.Follow-up worth filing
The manifest was the only authorship check anywhere in the snapshot path. Ordinary document and asset snapshots still accept any registered device as author. The same owner-signature mechanism generalizes.