Skip to content

Comment on shared HTML documents (attn-61t) - #14

Draft
angusbezzina wants to merge 18 commits into
mainfrom
angus/html-commenting
Draft

Comment on shared HTML documents (attn-61t)#14
angusbezzina wants to merge 18 commits into
mainfrom
angus/html-commenting

Conversation

@angusbezzina

@angusbezzina angusbezzina commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Comment on shared HTML documents — text ranges and whole elements, including the cell ‹ row ‹ table scope chain — on both the hosted reviewer and the native window. Comments ride the same end-to-end-encrypted event path markdown already uses, so the rail, replies, and resolve work unchanged.

Closes the attn-61t epic (9/9 children). Follow-ups live under attn-mz25.

Two decisions worth reviewing first

Design B is superseded (amendments.md #19). The June plan called for serving the shared document from a distinct localhost origin. That cannot work for the hosted browser reviewer, which has no Rust process — it renders decrypted bytes via srcdoc, so there is no server, port, or origin to serve from. Building it would have shipped commenting to the native daemon and left every browser-side reviewer out.

Instead the frame keeps its opaque-origin sandbox and the runtime is injected into the HTML source: one code path for both surfaces, and the untrusted document is denied the storage and same-origin reach a real origin would have granted it. The channel is a hello → MessageChannel handshake bound on event.source, since origin checking is meaningless on an opaque frame (event.origin is the string "null").

The document frame is untrusted (amendments.md #20). It may propose anchors and report geometry; it never creates or mutates review state. Comment bodies and the submit action stay in shell-owned UI, so the worst a hostile page can do is misdescribe a proposal you see before committing.

Rust never parses HTML — anchors are opaque blobs, resolved client-side in the frame — which keeps a headless HTML parser out of the 40 MiB binary budget. HTML anchors are therefore "unverified-by-authority", and client resolution reports are local-only: they mint no durable event and reach no peer, because an HTML anchor resolves against this client's DOM and peers can legitimately disagree.

Scope note

Phase 7 was resolved by scoping rather than code. It assumed HTML needed to join the ProseMirror collab room, but live co-typing of HTML is an explicit non-goal (§8) and the seeding effect already skips it correctly. What actually mattered — reviewers receive the document and anchors converge — is handled by the content-agnostic relay path, verified with an envelope round-trip test.

Hardening after adversarial review

Three independent reviewers swept the full diff. The architecture held, but eleven real defects came out and are fixed here:

  • Commenting was broken on non-ASCII documents. Producers sliced strings by characters against wire caps measured in bytes, so a CJK/emoji selection overran the parser caps and the whole message was dropped silently. Byte-offset mapping also counted an emoji as 6 bytes instead of 4, skewing every anchor to its right.
  • Trust-boundary gap. Inbound events never ran HtmlAnchor::validate() — a modified client with valid room keys could sync unbounded selector blobs into every peer's events.jsonl and doc frame. Now enforced at authorize_event. HTML-anchored suggestions (unresolvable by design) are refused at creation.
  • The hosted validator rejected the annotation capability outright, so a hosted reviewer joining a hosted-published HTML share could not hydrate the document at all; a second site dropped the capability even once parsed, leaving live joiners silently read-only until a refresh.
  • Injection robustness. The runtime spliced at the first </body> — including one inside a comment or script string — used String.replace (whose $' patterns could splice document text into the script), and had no double-injection guard.

Also fixed here

069ca18 — dialog content overflowing its frame horizontally. The shared dialog body was a bare grid, whose single implicit column sizes to max-content; one unbreakable font-mono path widened it past the frame and overflow-hidden clipped every control (measured: a 542px column in a 446px content box). Affects all four dialogs on the primitive. The mirror-image vertical bug was fixed and guarded previously, but that guard asserted nothing horizontal — now it does, and the new guard is verified to fail without the fix.

Verification

Full Rust suite, 123 web test files, svelte-check clean across 1579 files, clippy and cargo fmt clean, 12/12 Playwright in real Chromium against a genuinely opaque-origin sandboxed iframe, and the dual-instance E2E (task test:html-annotation) passing 9/9 over a real relay.

Markdown review was regression-checked empirically rather than argued: the review-surface suite produces a failure profile identical to the merge-base (34 pass / 9 pre-existing failures), so nothing here changed it.

What still needs a human

Tracked under attn-mz25 — none of it blocks reading the diff, but the first two should land before merge:

  1. attn-kio6 — security sign-off. Annotating requires allow-scripts, so hosted reviewers get page scripts enabled while annotating (they are fully disabled today). The frame stays opaque-origin and read-only viewing is unchanged, but this posture change wants a second opinion. See html-annotation.md §4.
  2. attn-qglc — interactive verification. The runtime and bridge have real-browser coverage and the E2E passes, but nobody has yet driven the UI by hand: selection-pill feel, composer placement, card alignment against live frame geometry. tests/fixtures/annotation-smoke.html is shaped for exactly that pass.
  3. attn-mz25.1 — deny_unknown_fields on SnapshotPlaintext means older native builds hard-fail on any newly published HTML share. Needs a schema-evolution decision, not a patch.
  4. attn-7jj3 (P3) — anchor calibration corpus; the resolver's confidence weights are currently unvalidated estimates. Plus a design decision on suggestions for HTML.

🤖 Generated with Claude Code

angusbezzina and others added 12 commits August 10, 2026 11:42
@angusbezzina @claude
feat(review): HTML annotation substrate + doc-frame runtime (attn-61t)
e88f41b 
Phases 0-3 of the HTML document annotation epic: give shared HTML
documents a comment anchoring substrate and the in-frame runtime that
resolves it. Read-only HTML sharing already worked (attn-qgd); this is
the commenting half.

Design (planning/collab/html-annotation.md, amendments #19/#20):

The June-locked "Design B" — serve the shared document from a distinct
localhost origin so the frame has a checkable origin — is superseded. It
cannot work for the hosted browser reviewer, which has no Rust process:
it renders decrypted bytes via srcdoc, so there is no server, port, or
origin to serve from. Adopting it would have left every browser-side
reviewer unable to comment.

Instead the frame keeps its opaque-origin sandbox and the runtime is
injected into the HTML source — a pure content transform, so native and
hosted run byte-identical code. The channel is a hello -> MessageChannel
handshake bound on event.source; origin checking is meaningless on an
opaque frame and unnecessary once traffic moves to a private port.
Retaining the opaque origin also denies the untrusted document the
storage and same-origin capability a real origin would have granted it.

The document frame is untrusted: it may propose anchors and report
geometry, never create or mutate review state. Comment bodies and the
submit action stay in shell-owned UI, so the worst a hostile page can do
is misdescribe a proposal the user sees before committing.

Landed:

- Rust: HtmlAnchor W3C selector layer (CssSelector + ranked fallbacks,
  TextQuote, TextPosition, RangeSelector) plus an agent-context block, so
  a comment is actionable to a coding agent that never saw the document.
  Anchor.html is serde-optional; markdown wire bytes are unchanged.
  HtmlAnchor::validate bounds every field at the trust boundary.
- Rust: SnapshotAnnotation::HtmlSelectorsV1 capability; bootstrap now
  publishes HTML with it rather than (DocType::Html, None). HTML still
  never carries a Rust-built anchorIndex — that would need a headless
  HTML parser in the binary and the size gate forbids it.
- Rust: the markdown resolver refuses HTML anchors outright rather than
  landing them somewhere plausible and wrong; manager validates HTML
  anchors before persisting or syncing them.
- Web: doc-runtime/ (selector generation + resolution, CSS Custom
  Highlight API text layer, element overlays with inert fills, persistent
  comment pins, scope breadcrumb), bundled to an injectable IIFE.
- Web: doc-protocol.ts with a validating parser for every inbound frame
  message, and HtmlAnnotationBridge for the handshake and coordinates.
- Web: HtmlViewer gains annotate/onBridge. Annotating needs allow-scripts,
  so hosted reviewers get page scripts enabled while annotating; the frame
  stays opaque-origin, and read-only viewing is unchanged.

Tests: 557 Rust, 97 web test files, all green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@angusbezzina @claude
test(review): real-browser tests for the HTML annotation runtime (att…
102e4f8 
…n-ges)

The runtime cannot be tested without a browser engine: it depends on live
Range geometry, getClientRects, document.getSelection, the CSS Custom
Highlight API, and on postMessage/MessageChannel behaving as they do
across a genuinely opaque-origin sandboxed iframe — which is the whole
premise of the design.

The spec stands up a shell harness that injects the runtime into a srcdoc
frame with sandbox="allow-scripts" and no allow-same-origin, performs the
real hello -> MessageChannel handshake, and asserts against what the frame
reports back. Covers the handshake, a text proposal carrying every
selector layer, exact resolution, re-anchoring by quote after the document
shifts (the reason all layers are written at creation time), stale
detection when the content is gone, the cell/row/table scope chain,
element overlay + persistent pin with a pointer-events:none fill, and
resilience to malformed shell messages.

8 passing. Run with `npm run test:e2e:html-annotation`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@angusbezzina @claude
chore(beads): close attn-05x/80u/yob/17g/ges; record design deviation
b4e094b 
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@angusbezzina @claude
feat(review): comment on shared HTML documents end to end (attn-lon, …
db85b03 
…attn-08r)

Phases 5 and 6. The annotation runtime now reaches the user: a shared HTML
snapshot that declares the client-side annotation capability renders with
the runtime injected, mounts the comment margin beside it, and round-trips
comments through the same encrypted event path markdown uses.

Phase 6 — IPC contract:

ReportHtmlAnchorResolution carries the document frame's verdict to the
daemon, which finds the event's fileId and emits AnchorResolutionChanged so
the rail can show position and confidence. Deliberately local-only: unlike
ResolveAnchor it mints no durable event and reaches no peer, because an
HTML anchor resolves against *this* client's rendered DOM. Two peers can
legitimately disagree, and propagating one peer's view would overwrite the
other's correct one.

ExactReason and RemappedReason gain ClientResolved rather than reusing a
markdown reason. The existing variants each name a step this resolver ran,
and for HTML none of them did — labelling a frame-reported match as
base_hash_match would assert something the daemon cannot know.

Phase 5 — shell wiring:

ReviewMargin gains an optional anchorTops record, consulted only when there
is no ProseMirror view. An HTML document renders in a cross-origin frame
whose DOM the shell cannot touch, so the frame reports its own geometry and
the shell forwards it. The markdown path is untouched: with a view present,
every code path is exactly what it was.

HtmlCommentComposer is a sibling of CommentComposer rather than a variant.
That component is built around a ProseMirror view — quote via textBetween,
anchor tracking via coordsAtPos, anchor construction from the live
selection — none of which exists here. Keeping them apart leaves the
markdown authoring path, which everything else in review depends on,
completely unmodified.

Also carries `annotation` through the two snapshot assembly sites that were
silently dropping it; without that the reviewer always saw a read-only
document no matter what the owner published.

Tests: 557 Rust, 97 web test files, all green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@angusbezzina @claude
test(review): exercise the shipped bridge, not a stand-in (attn-08r)
bbf8650 
The existing spec drove the protocol with an inline harness. That proved the
document side works but said nothing about the code the app ships, so a
regression in HtmlAnnotationBridge or injectDocRuntime would have gone
unnoticed. These four cases bundle the real modules into the page.

Covers the handshake through the shipped injector, viewport→shell coordinate
conversion (with the frame deliberately offset so the two spaces cannot
coincide by accident), rejection of a forged hello that did not come from
this exact frame — the event.source binding that stands in for an origin
check an opaque frame cannot provide — and the queue-before-handshake path,
since the shell renders threads well before the frame finishes booting.

12 passing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@angusbezzina @claude
feat(review): wire HTML annotation into the native shell too (attn-08r)
1df07e2 
The hosted reviewer got the full treatment first; this brings the native
window to parity so the feature exists on both surfaces rather than only in
the browser build.

Same shape as BrowserReviewApp: the bridge state mirrors what the document
frame reports, threads become renderable anchors keyed by thread id, frame
geometry becomes ReviewMargin card tops, and proposals open
HtmlCommentComposer. The native margin already mounted unconditionally via
rightRailPlaceholder, so it only needed the geometry source and a null view
for HTML — the markdown path keeps its ProseMirror view and every code path
it had.

Also adds an envelope round-trip test for an HTML-anchored comment. That is
the convergence-critical path: the anchor is opaque to Rust, so nothing
downstream would notice a dropped or reordered selector — the comment would
simply land somewhere else on the peer's screen, or nowhere.

Tests: 558 Rust, 12 Playwright, 0 svelte-check errors.

Note: web/src/lib/review/review-drift-check.test.ts is flaky (~1 in 3) on a
60ms timing race in the test itself. Reproduced independently of these
changes; untouched here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@angusbezzina @claude
test(review): dual-instance E2E for HTML annotation (attn-7ev)
cd107cd 
Sibling to test-html-share-e2e.sh, which proves an HTML document reaches a
reviewer read-only. This proves the half that makes it a review surface: the
annotation capability survives publish → encrypt → relay → decrypt, the
runtime is spliced into the reviewer's frame, and the comment margin mounts.

Two assertions are worth calling out. The capability check exists because
losing it in transit is invisible — the reviewer silently falls back to the
read-only viewer, which looks identical until you try to comment. The
sandbox check asserts the frame gained allow-scripts but NOT
allow-same-origin, since the whole isolation argument in amendments #19
rests on the document staying on an opaque origin.

The assertions are deliberately shell-observable rather than reaching into
the frame: the automation bridge evaluates in the shell's context and cannot
cross into a sandboxed cross-origin document. The frame's own behavior is
covered in a real browser by web/e2e/html-annotation-runtime.spec.ts.

Adds task test:html-annotation and task test:html-annotation:runtime.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@angusbezzina @claude
style: rustfmt manager.rs after the Phase 6 command additions
5c94656 
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@angusbezzina @claude
chore(beads): close attn-61t; file security-review, verification, and…
0385917 
… calibration follow-ups

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@angusbezzina @claude
fix(review): harden HTML annotation after adversarial review (attn-mz25)
4fee2c5 
Post-hoc review of attn-61t (3 independent passes over the full diff)
surfaced these; all verified by test before fixing.

Frame runtime and injection:
- positionAt counted astral chars as 6 bytes (per-code-unit byteLength on
  lone surrogates); every offset right of an emoji skewed. Walk code points.
- Producers sliced by CHARS against wire caps in BYTES, so selections in
  CJK/emoji documents blew the parser caps and the whole message was
  silently dropped (commenting dead on non-ASCII docs). New clampText
  enforces both bounds without splitting surrogate pairs; applied to
  quote/scopePreview/preview/title; prefix/suffix now cut by code points.
- Normalized-quote tier probed RAW text with 24 normalized chars, failing
  exactly when the cosmetic edit sat in the probe, and sized the range with
  the collapsed-whitespace length. normalizeTextWithMap maps the normalized
  match back to exact raw offsets.
- injectDocRuntime spliced at the FIRST </body> — including one inside a
  comment, script string, or attribute (runtime dead or document
  corrupted). Now splices before the LAST, skips documents already carrying
  the runtime, and concatenates instead of String.replace (whose $'
  patterns could splice document text into the script). Runtime also guards
  itself with a window global against double boot. build-doc-runtime
  escapes </script and <!-- in the bundle and syntax-checks the result.

Trust boundary (Rust):
- Inbound events never ran HtmlAnchor::validate() — a modified client with
  valid room keys could sync an unbounded selector blob to every peer's
  events.jsonl and doc frame. authorize_event now validates CommentCreated
  and SuggestionCreated anchors (InboundError::InvalidAnchor, test).
- CreateSuggestion accepted html-anchored drafts that the apply pipeline
  can never resolve (v1 non-goal, §8); now refused at creation.

Hosted shell:
- validateSnapshotPlaintext's html arm required EXACT keys, so a snapshot
  carrying the annotation capability failed validation outright — a hosted
  joiner could not hydrate a hosted-published HTML share at all. The arm
  now accepts and PRESERVES annotation (known value only, html only).
- hydrateSnapshot dropped annotation even once parsed — live-joining
  reviewers landed read-only until a refresh replayed the log. Now copied;
  regression-tested through the real validator + session.
- Hosted resolution reports called a native-only IPC that no-ops in the
  browser; verdicts now apply straight to the review store (local-only by
  design), with client_resolved added to the TS ResolvedAnchor unions to
  match the Rust wire.
- Both composers bound LIVE snapshot identity to an anchor whose offsets
  were measured at selection time; identity is now captured at open and the
  composer closes on republish/file switch.

Verified: cargo test green (incl. new inbound rejection test), fmt+check
clean; web 98/98 test files (10 new text-bounds + 4 new inject cases +
annotation validator cases), svelte-check 0 errors; Playwright 12/12; the
dual-instance E2E (task test:html-annotation) run for the first time and
passing 9/9 against the rebuilt binary. Markdown review regression-checked:
scripts/test-review-e2e.sh matches the merge-base failure profile exactly
(34 PASS / 9 pre-existing FAIL; the scroll-tracking delta reproduced as an
environmental rAF-throttling flake, tracks correctly in live probes).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@angusbezzina @claude
test(fixtures): an HTML document worth smoke-testing annotation against
4f86651 
sample.html is a centered card — no table, no scroll, one heading. It
proves the viewer renders, but it cannot exercise the parts of annotation
that only a human can judge.

This fixture is shaped for that pass: long enough to scroll (margin cards
tracking their anchors), a real table with a header row (the cell < row <
table scope breadcrumb), a list/code block/blockquote (each its own gutter
scope), and one sentence repeated verbatim in two places so prefix/suffix
disambiguation can be watched keeping two comments on their own paragraphs.
Self-contained — no remote fonts — so the smoke test never depends on the
network. Inline script retained deliberately: annotating runs with scripts
enabled (html-annotation.md §4).

Verified over the real relay: shares, reaches the reviewer with the
annotation capability, and the runtime injects with the table intact.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@angusbezzina @claude
fix(ui): stop dialog content overflowing its frame horizontally (attn…
069ca18 
…-mz25.2)

The shared dialog body was `grid gap-4 p-6`. A bare grid has ONE implicit
column sized `auto` = `minmax(auto, max-content)`, and inside the
ScrollArea that resolves to the widest child's max-content width rather
than the dialog's own. One unbreakable string — the font-mono project
path in the share modal — therefore widened the whole column past the
frame, where `overflow-hidden` silently clipped it: 'Select visible' cut
mid-word, the file-type labels gone entirely, the create button sliced in
half. Measured in the running app: a 542px column inside a 446px content
box, 32 elements up to 70px past the right edge.

`grid-cols-[minmax(0,1fr)]` resolves the track against the dialog and
lets children shrink below min-content, which is precisely what gives the
`truncate`/`min-w-0` already on those children a definite width to work
against. Same measurement after: 447.5px column, zero overflow, the path
ellipsising as intended. The share dialog's project-root row also gains
`min-w-0`, since a flex item defaults to `min-width:auto` and refuses to
shrink below its nowrap min-content no matter what `truncate` says.

Fixes all four dialogs on the primitive (Settings, Share, NamePrompt,
ReviewExitConfirm). Vertical scrolling was already correct and stays so:
verified the body scrolls its full 301px when the frame is squashed to
240px, with no horizontal overflow at that size either.

The mirror-image VERTICAL bug was fixed and guarded in attn-11g4.1.1, but
that guard asserted nothing about the horizontal axis, which is how this
one reached a user. dialog-scroll.spec.ts now covers both: a width sweep
as a blast-radius check, plus an unbreakable-string case that reproduces
the real trigger. Verified honest — reverting the fix fails that test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
attn Ready Ready Preview Aug 13, 2026 5:51pm

Request Review

@angusbezzina @claude
fix(dev): dev-collab readiness probe assumed a markdown fixture
7e34f50 
`wait_for_dual 'h1'` is a fine proxy for "the window rendered" only while
the fixture is markdown, which renders into the SHELL's own DOM. An HTML
document renders inside a sandboxed, opaque-origin iframe (src=attn://…
for a local file, srcdoc for a received snapshot), so its <h1> lives in
the frame's document and the automation bridge — which evaluates in the
shell's context — can never see it.

So `FIXTURE_PATH=…​.html scripts/dev-collab.sh` timed out after 20s on a
window that had rendered perfectly, and tore the whole harness down
before the user could click Share. Probed to confirm rather than assume:
with the smoke fixture open the shell reports 0 `h1` elements and 1
`[data-slot="html-viewer"]`.

The library gains `dual_ready_selector` (fixture path → the selector that
proves it rendered) and `wait_for_dual_fixtures`, which waits for each
window against ITS OWN fixture's selector — the owner may be on .html
while the reviewer is still on .md, so one selector for both cannot work.
`start_dual` stashes the reviewer fixture it actually booted so the two
calls cannot disagree. `wait_for_dual` is untouched for existing callers.

Verified: the failing command now reaches "Daemons running" and stays up;
the default markdown path still boots; test:dual passes 10/0/0; the
html-annotation E2E passes 9/9.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@angusbezzina @claude
fix(ui): native icon resolver looped Svelte effects and blanked every…
cca1204 
… document

subscribeIconPack deliberately emits its current value synchronously on
subscribe (icon-pack.ts:71). nativeFileIconResolver forwarded that first
emission straight to the subscriber, so FileTree's `iconRevision += 1`
ran while the very effect that registered it was still executing. The
render reads iconRevision (FileTree.svelte:145,150), so Svelte re-ran the
subscription effect until it tripped effect_update_depth_exceeded — which
kills the reactive graph. The editor then mounts a single empty paragraph
and NO document renders at all, markdown included.

Bisected to 2aa663d. The same commit had already found and fixed this
exact hazard in the HOSTED registry, whose comment describes the native
failure precisely — 'Notifying here would write every recursive
FileTree's revision from inside its own subscription effect'. The native
resolver was simply left behind, so this mirrors that fix: track the
selected pack, keep kicking off the lazy load on the initial emission,
and only notify on a genuine change. Later loads still notify through
the async path, so icons still repaint (verified: 13 icons across 8 tree
items after the change).

Not the HTML commenting epic: 5bb122b measures healthy, 2aa663d does not.

Verified against the pre-epic baseline exactly — ProseMirror back to 17
children, 1 h1, zero effect errors (was 1 child, 2 errors). The HTML
annotation E2E goes from 6 failures to 12/12 including the owner-side
assertions; svelte-check clean over 1592 files; 125 web test files pass.
The review-surface suite's 13 remaining failures are the known
rail/gutter cluster that predates this branch, untouched by icons.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@angusbezzina