From 472bcb1fc9951aa77ed2601557d93c21c1a46616 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Fri, 24 Jul 2026 22:59:17 +0300 Subject: [PATCH] feat(web): commentable prose diff with honest anchor state (spec-by6.3.3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverses the diff view's founding rule. renderDocDiff skipped ChangeEqual outright — "the review shows only what changed" — but any block of a proposed document must be commentable, so unchanged blocks now render as collapsed, dimmed context. Changed blocks keep their border, tint and full body, so the page still reads as a diff at a glance rather than as a document dump. A context block carrying a comment renders open. ChangeMoveIn now shows its text too, since a comment control on invisible text is a control on nothing; ChangeMoveOut stays a bare marker and is deliberately not commentable, because the same paragraph is anchorable at its move-in position and two anchors for one paragraph is the bug that avoids. Every rendered block carries id="b-<16 hex>", hashed from the whole anchor tuple. Not the page ordinal: an ordinal renumbers on any insertion above it, so a saved link would silently scroll to a neighbouring paragraph, whereas including the block hash makes a stale link resolve to nothing instead. Threads are placed by anchor and by nothing else. Anything no rendered block claims — an outdated anchor, an old-side anchor whose block the diff no longer draws, a document the proposal no longer changes — is collected into a page-level "comments that lost their anchor" area. Never dropped, never moved onto a neighbour: a comment reads as authoritative about the block it sits beside, so attaching it to the wrong one is worse than admitting it lost its place. An edited anchor is drawn on its block and badged. The comment form's anchor is built at submit time from the branch as it now reads, through service.AnchorOf — hand-rolling the ordinal conversion here would put the browser's comments on different blocks than the MCP tool's, which is the one way two surfaces of one conversation disagree without either looking broken. The form's block hash guards it: a block that moved while the page sat open is a 409, not a comment attached to whatever took its place. That hash is required rather than checked-when-present. Skipping the guard for a form that omits it would let a later template refactor drop the hidden field and disable the staleness check silently, with every test still green. Authority is surfaced, not re-implemented: compose and resolve are the owner's because service says so and ErrForbidden becomes a 403. spec-by6.3.3 --- .beads/interactions.jsonl | 2 + .beads/issues.jsonl | 4 +- scss/main.scss | 154 ++++++++++++ service/comment.go | 55 ++--- web/comments.go | 412 +++++++++++++++++++++++++++++++ web/comments_test.go | 478 ++++++++++++++++++++++++++++++++++++ web/diff.go | 410 +++++++++++++++++++++++++++---- web/diff_internal_test.go | 235 +++++++++++++++++- web/proposal.go | 65 ++++- web/reader.go | 30 +++ web/router.go | 3 + web/templates.go | 25 +- web/templates/proposal.html | 32 +++ web/templates/threads.html | 79 ++++++ web/web_test.go | 91 +++++++ 15 files changed, 1978 insertions(+), 97 deletions(-) create mode 100644 web/comments.go create mode 100644 web/comments_test.go create mode 100644 web/templates/threads.html diff --git a/.beads/interactions.jsonl b/.beads/interactions.jsonl index 40a09463a2bc63c6b4624665f0c9a7a28a525f36..c40cf4e69af7b11e18c1a2e7c99051588a21eba8 100644 --- a/.beads/interactions.jsonl +++ b/.beads/interactions.jsonl @@ -9,3 +9,5 @@ {"id":"int-f4f6563f5207770848db3bf66247c0db","kind":"field_change","created_at":"2026-07-24T16:59:33.671408Z","actor":"Eugene Blikh","issue_id":"spec-ejq.1","extra":{"field":"status","new_value":"closed","old_value":"in_progress","reason":"authn.Principal.CanRead() defined once; graph.gate, web.mayRead, mcpsrv.Gate routed through it. coreauth left independent (different semantic). Unit test covers owner/agent true, anon/zero/unknown false. Commit pending push."}} {"id":"int-d8d8556e8412e54680f206d5867f2370","kind":"field_change","created_at":"2026-07-24T18:12:41.580051Z","actor":"Eugene Blikh","issue_id":"spec-wcr","extra":{"field":"status","new_value":"closed","old_value":"in_progress","reason":"All 7 items dispositioned: 2 fixed (#1,#2), 1 deleted (#4), 2 wontfix (#3,#5), 2 moved to Phase 5 (spec-by6.1, spec-by6.2)."}} {"id":"int-2e1293e1ef51cf8c03a3c538d807fdae","kind":"field_change","created_at":"2026-07-24T19:08:44.914555Z","actor":"Eugene Blikh","issue_id":"spec-by6.3.1","extra":{"field":"status","new_value":"closed","old_value":"in_progress","reason":"core.CommentAnchor + ResolveAnchor with 9 tests; migrations/0004_comment.sql + schema.sql; db/comment.go with 8 Postgres-backed tests, all green against real PG 16."}} +{"id":"int-06d2c1ce7f403e7cbb46c711877f5a14","kind":"field_change","created_at":"2026-07-24T19:15:24.165993Z","actor":"Eugene Blikh","issue_id":"spec-by6.3.2","extra":{"field":"status","new_value":"closed","old_value":"in_progress","reason":"service.CommentOn/ReplyTo/ResolveThread/Threads + AnchorThreads/AnchorOf; auto-merge gate in autoMerges. 10 tests, PG-backed ones green against real PG 16."}} +{"id":"int-70f93fbf5d69de0ca9f334d61c671867","kind":"field_change","created_at":"2026-07-24T19:44:55.309486Z","actor":"Eugene Blikh","issue_id":"spec-by6.3.4","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"spec_comment: list threads with anchor state + reply; no-resolve enforced by interface. 9 tests green, no DB needed. Committed f82d90a."}} diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index e5a193f0b23cc6b7dd938849f0c06b7513004827..570a43840283bd171518b1588546f2b47d15781e 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -1,13 +1,13 @@ {"_type":"issue","id":"spec-zqb","title":"Phase 3: write plane — agents propose","description":"The agent half of the loop, and the service's whole premise: an agent proposes a document change, gets back a link, a human approves in a browser. Until this ships, agents can only read.","design":"Scope: proposals (branch proposals/\u003cid\u003e + Postgres row, state open-\u003emerged|rejected); the If-Match tree-splice merge with the four staleness cases and the ancestry check gitx flagged; the single agent token + mandatory provenance trailers; spec_propose / spec_comment over REST + MCP; every write response returns {proposal, url} so the agent can hand over a link. Prereqs found during Phases 1-2: (1) service.ListProposals(space, state) — graph proposal listing is stubbed and db only has ListProposalsByState; (2) IsAncestor(proposalHead, H) check before Merge so an already-merged proposal reports 'already merged' not a confusing 409; (3) auto-merge policy evaluation (.spec.yml AutoMerges) which the reconciler's 'approval=policy' inference also wants. Dispatch like Phases 1-2: proposal orchestration in service/ first and committed, then surface tools fan out. This is where the merge model and reconciler first run under real proposals rather than on paper.","status":"closed","priority":1,"issue_type":"feature","assignee":"Eugene Blikh","owner":"bigbes@gmail.com","created_at":"2026-07-23T04:08:58Z","created_by":"Eugene Blikh","updated_at":"2026-07-23T05:29:29Z","started_at":"2026-07-23T04:52:59Z","closed_at":"2026-07-23T05:29:29Z","close_reason":"Write plane shipped: service.Propose/Merge/Reject/ListProposals with provenance trailers, auto-merge policy, If-Match staleness + already-merged ancestry check; surfaces graph (Proposals port), mcpsrv spec_propose, and new api/ REST PUT — every write returns {proposal, url}. Validated end-to-end against Postgres. spec_comment deliberately deferred to Phase 5 (spec-by6), which gates the comment schema on the Phase 4 review UI; the approve/reject browser UI is Phase 4 (spec-3vz).","dependencies":[{"issue_id":"spec-zqb","depends_on_id":"spec-ejq","type":"parent-child","created_at":"2026-07-23T07:09:24Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":1,"comment_count":0} {"_type":"issue","id":"spec-ejq","title":"spec.sr.ht — reviewable document storage for humans and agents","description":"A third custom Go service on the self-hosted SourceHut instance: agents propose documents, a human reviews and curates, agents read the approved text. One loop — bot produces, human curates, bots consume. Runs at https://spec.srht.bigb.es, deployed on phoebe as srht-spec-1. Code: git.sourcecraft.dev/bigbes/sr-ht-spec. Design: docs/DESIGN.md.","design":"DONE and deployed (Phases 0-2): core domain, gitx (bare repos + id-keyed tree-splice merge), db (Postgres schema + global ID registry), authn (unified-login cookie + agent tokens + provenance), the three receive hooks + hook RPC + daemon, service layer + reconciler, doc (warren vault/render on git objects), search (one global bleve index, per-line ru/en routing), projects (saved filter, not container), web read UI, mcpsrv read tools, graph read schema, prosediff (Phase 0 gate PASSED), specsrht space create/list, push-\u003ereindex. Live corpus: ~bigbes/rfcs holds SPEC-0001 and NOTE-0001. Verified end-to-end: validating push path, fail-closed reads, id-addressing, bilingual search, GraphQL. NOT DONE: the agent half of the loop (write plane), review UI, comments.","status":"open","priority":1,"issue_type":"epic","owner":"bigbes@gmail.com","created_at":"2026-07-23T04:08:44Z","created_by":"Eugene Blikh","updated_at":"2026-07-23T04:08:44Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"spec-by6.3.3","title":"web: commentable diff — context blocks, anchor ids, comment UI","description":"renderDocDiff currently skips ChangeEqual ('the review shows only what changed', web/diff.go:61). It must now render unchanged blocks as commentable context — collapsed or dimmed so the diff still reads as a diff — because the settled scope is that any block of a proposed document can be commented on. Every rendered block gains a stable DOM id derived from the anchor tuple. Then the comment UI itself: existing comments rendered beside their block with their anchor state (moved / outdated shown honestly), a compose form, reply and resolve. Threading is flat per anchor.","status":"open","priority":2,"issue_type":"task","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:57:14Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T18:57:14Z","dependencies":[{"issue_id":"spec-by6.3.3","depends_on_id":"spec-by6.3.2","type":"blocks","created_at":"2026-07-24T21:57:29Z","created_by":"Eugene Blikh","metadata":"{}"},{"issue_id":"spec-by6.3.3","depends_on_id":"spec-by6.3","type":"parent-child","created_at":"2026-07-24T21:57:14Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":1,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"spec-by6.3.2","title":"service: comment API + policy auto-merge gate","description":"service.Comment type, Comment/ListComments/ReplyComment/ResolveComment, and the anchor resolution pass that takes a revision's prosediff blocks and reports each comment as anchored, moved or outdated. Wire the merge gate: an unresolved comment suppresses policy auto-merge (not manual approve) — the check belongs beside the existing auto-merge policy evaluation, and needs a test that a commented proposal stops auto-merging while Merge with human approval still succeeds.","status":"in_progress","priority":2,"issue_type":"task","assignee":"Eugene Blikh","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:57:05Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T19:09:52Z","started_at":"2026-07-24T19:09:52Z","dependencies":[{"issue_id":"spec-by6.3.2","depends_on_id":"spec-by6.3","type":"parent-child","created_at":"2026-07-24T21:57:05Z","created_by":"Eugene Blikh","metadata":"{}"},{"issue_id":"spec-by6.3.2","depends_on_id":"spec-by6.3.1","type":"blocks","created_at":"2026-07-24T21:57:28Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":1,"dependent_count":2,"comment_count":0} +{"_type":"issue","id":"spec-by6.3.2","title":"service: comment API + policy auto-merge gate","description":"service.Comment type, Comment/ListComments/ReplyComment/ResolveComment, and the anchor resolution pass that takes a revision's prosediff blocks and reports each comment as anchored, moved or outdated. Wire the merge gate: an unresolved comment suppresses policy auto-merge (not manual approve) — the check belongs beside the existing auto-merge policy evaluation, and needs a test that a commented proposal stops auto-merging while Merge with human approval still succeeds.","status":"closed","priority":2,"issue_type":"task","assignee":"Eugene Blikh","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:57:05Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T19:15:24Z","started_at":"2026-07-24T19:09:52Z","closed_at":"2026-07-24T19:15:24Z","close_reason":"service.CommentOn/ReplyTo/ResolveThread/Threads + AnchorThreads/AnchorOf; auto-merge gate in autoMerges. 10 tests, PG-backed ones green against real PG 16.","dependencies":[{"issue_id":"spec-by6.3.2","depends_on_id":"spec-by6.3","type":"parent-child","created_at":"2026-07-24T21:57:05Z","created_by":"Eugene Blikh","metadata":"{}"},{"issue_id":"spec-by6.3.2","depends_on_id":"spec-by6.3.1","type":"blocks","created_at":"2026-07-24T21:57:28Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":1,"dependent_count":2,"comment_count":0} {"_type":"issue","id":"spec-by6.3.1","title":"Comment anchors: core type + db schema","description":"Foundation wave. core.CommentAnchor (doc_id, heading_path, ordinal, hash) plus the anchor-state vocabulary (anchored | moved | outdated) and the resolution function, kept free of a prosediff import so core stays dependency-free — resolution takes the block fields it needs, not the Block type. migrations/0004_comment.sql + schema.sql: the comment table storing the tuple, threading via parent_id, author identity + agent provenance (mirroring proposal's ck_proposal_provenance rule), and resolved timestamp. Anchor state is NOT a column — it is derived per revision at read time. db/comment.go with the insert/list/resolve queries and a test that a reflow keeps the comment and a deletion outdates it.","status":"closed","priority":2,"issue_type":"task","assignee":"Eugene Blikh","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:57:02Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T19:08:45Z","started_at":"2026-07-24T18:57:37Z","closed_at":"2026-07-24T19:08:45Z","close_reason":"core.CommentAnchor + ResolveAnchor with 9 tests; migrations/0004_comment.sql + schema.sql; db/comment.go with 8 Postgres-backed tests, all green against real PG 16.","dependencies":[{"issue_id":"spec-by6.3.1","depends_on_id":"spec-by6.3","type":"parent-child","created_at":"2026-07-24T21:57:01Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":1,"comment_count":0} {"_type":"issue","id":"spec-by6.3","title":"Phase 5b: inline comments on proposals","description":"Close the propose-\u003ecritique-\u003erevise loop: the owner comments on blocks of a proposed document, the agent reads the comments, replies, and pushes a revision. Anchors re-resolve against the new revision rather than being lost.\n\nDesign gate cleared 2026-07-24: DESIGN.md deferred the comment schema until the Phase 4 review UI existed. It exists, and three facts from it shape the work: the review page renders only changed blocks (web/diff.go:61); the read view uses a different block model (doc.Renderer/goldmark, web/handlers.go:437) from the review view (prosediff.Segment); no rendered block carries a DOM id.","design":"ANCHOR MODEL (as DESIGN.md:793-808 specifies, implemented not revisited):\ntuple = (doc_id, heading_path, block_ordinal, block_hash). Resolve against a\nrevision by block_hash first; fall back to heading_path + ordinal; when both\nfail mark the comment OUTDATED rather than relocating it. heading_path is\ndeliberately NOT part of the hash, so renaming a section does not dirty every\nblock beneath it and orphan its comments at once. Anchor on doc_id, not path,\nso comments survive renames.\n\nANCHOR STATE IS DERIVED, NOT STORED. A comment is not outdated in general, it\nis outdated AT a revision, and a proposal branch moves under it. The DB stores\nthe tuple; resolution runs at read time against the revision being displayed.\n\nSIDE: a comment anchors to the NEW side of a modified block (that is the text\nunder review) and to the OLD side of a pure deletion.\n\nSCOPE DECISIONS (settled with owner 2026-07-24):\n1. Surface: the review page, ALL blocks — not only changed ones. This changes\n the diff view rule: renderDocDiff currently skips ChangeEqual entirely, and\n must instead render unchanged blocks as commentable context. Comments on\n approved documents (the read view) stay out of scope: that needs prosediff\n segmentation of the read path plus a goldmark\u003c-\u003eprosediff block mapping.\n2. Merge gate: an unresolved comment suppresses POLICY AUTO-MERGE only. Manual\n approve still works — human judgement wins. Rationale: a comment means the\n owner engaged, so the proposal must not slip through unattended; but a stale\n comment must not be able to wedge a proposal.\n3. Agents: read + reply + revise via spec_comment. Agents may NOT resolve —\n an agent marking its own critique resolved defeats the gate.","acceptance_criteria":"A comment survives a revision that reflows its paragraph (hash miss, heading-path+ordinal hit) and is reported as moved, not lost. A comment whose block is deleted is reported outdated, never relocated. An unresolved comment prevents policy auto-merge and does not prevent manual approve. An agent can read and reply to comments on its own proposal but cannot resolve one.","status":"open","priority":2,"issue_type":"feature","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:56:37Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T18:56:37Z","dependencies":[{"issue_id":"spec-by6.3","depends_on_id":"spec-by6","type":"parent-child","created_at":"2026-07-24T21:56:36Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"spec-43w","title":"Stale apk pins: paste.sr.ht 0.19.3-r0 and hub.sr.ht 0.29.4-r0 aged off the mirror","description":"Pre-existing, not caused by spec.sr.ht — but a FULL stack rebuild (post_push) would fail on these two apk layers and leave the stack un-deployable (the 2026-07-11 incident pattern). The self-hosted append-only repo.bigb.es mirror only preserves what it has seen, and it started after these aged off; old versions are unrecoverable.","design":"spec.sr.ht was deployed AROUND this with labng push --no-restart + a single-service 'docker compose build spec', so nothing else rebuilt. To fix properly: bump the two pins in versions.env to what the mirror currently ships (paste 0.19.4-r0, hub 0.31.1-r0 as of 2026-07-22). hub 0.29.4-\u003e0.31.1 crosses two minors with forward-only migrations — pg_dumpall first per the stack CLAUDE.md. Kept separate from the spec deploy deliberately.","status":"closed","priority":2,"issue_type":"bug","owner":"bigbes@gmail.com","created_at":"2026-07-23T04:09:54Z","created_by":"Eugene Blikh","updated_at":"2026-07-23T04:52:46Z","closed_at":"2026-07-23T04:52:46Z","close_reason":"Stale apk pins already bumped in the stack versions.env (paste 0.19.4-r0, hub 0.31.1-r0); fixed out of band.","dependencies":[{"issue_id":"spec-43w","depends_on_id":"spec-ejq","type":"parent-child","created_at":"2026-07-23T07:09:54Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"spec-ar4","title":"Restart other SourceHut services so spec.sr.ht appears in their nav","description":"The switcher entry is config-driven: each service builds its own nav from config sections ending in .sr.ht. spec.sr.ht is in the shared config.ini now, but the OTHER services (git, meta, todo, ...) only pick up the new entry on restart. Not done unprompted because it briefly bounces every service. On phoebe: docker compose restart \u003cthe sourcehut services\u003e.","status":"open","priority":2,"issue_type":"task","owner":"bigbes@gmail.com","created_at":"2026-07-23T04:09:53Z","created_by":"Eugene Blikh","updated_at":"2026-07-23T04:09:53Z","dependencies":[{"issue_id":"spec-ar4","depends_on_id":"spec-ejq","type":"parent-child","created_at":"2026-07-23T07:09:53Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"spec-3vz","title":"Phase 4: review plane — approve/reject in a browser","description":"Proposal pages at stable URLs (returned by every write), the inbox, prose diff, approve (merges immediately) / reject, status lifecycle, and the digest of policy-merged firehose content.","design":"Productionise prosediff (the Phase 0 spike) into the review UI. CRITICAL requirement from the Phase 0 verdict: 13% of real prose modifications shred into interleaved fragments (similarity \u003c= 0.73), so the UI MUST switch to a two-column old/new view below ~0.75 and inline word diffs above it — building only the inline renderer makes one review in eight unreadable. The prosediff package already emits the (Hash, HeadingPath, Ordinal) comment-anchor tuple. Reads a proposal branch via service.ReadDocumentAtRef (the deliberately-awkward bypass), never the normal read path.","status":"closed","priority":2,"issue_type":"feature","assignee":"Eugene Blikh","owner":"bigbes@gmail.com","created_at":"2026-07-23T04:09:25Z","created_by":"Eugene Blikh","updated_at":"2026-07-23T06:52:55Z","started_at":"2026-07-23T06:31:53Z","closed_at":"2026-07-23T06:52:55Z","close_reason":"Review plane shipped: proposal page at /~owner/space/p/\u003cid\u003e with the prose diff (inline word-diff above 0.75 similarity, two-column old/new below — the Phase 0 verdict's hard requirement, in web/diff.go), approve(merge)/reject as owner-only CSRF-guarded POSTs, status badges, and the /inbox review queue + policy-merged digest. service.ProposalDiff/InboxProposals/DigestProposals/MergeHuman back it; web.Reader extended. Diff reads the proposal branch by resolving its tip to a sha (pinned read) rather than the ReadDocumentAtRef bypass — cleaner and immutable. Validated: diff renderer unit tests (inline/two-column/escaping), proposal-page + approve/reject handler tests (CSRF, owner-only, stale-\u003e409, wrong-space-\u003e404), inbox tests, and PG-backed service tests. Follow-up: digest_mark 'since last seen' tracking.","dependencies":[{"issue_id":"spec-3vz","depends_on_id":"spec-ejq","type":"parent-child","created_at":"2026-07-23T07:09:25Z","created_by":"Eugene Blikh","metadata":"{}"},{"issue_id":"spec-3vz","depends_on_id":"spec-zqb","type":"blocks","created_at":"2026-07-23T07:09:26Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0} -{"_type":"issue","id":"spec-by6.3.4","title":"mcpsrv: spec_comment tool (read + reply, no resolve)","description":"The agent half of the loop. spec_comment lets an agent list the comments on a proposal it authored and reply to them; it must NOT resolve one, since an agent marking its own critique resolved defeats the auto-merge gate. Reuses the Phase 3 provenance path (agent + agent_session) for reply authorship, and the existing owner+agents read gate from spec-jjo. mcpsrv/mcpsrv.go:39 still documents spec_comment as deliberately absent — update that comment when it lands.","status":"open","priority":3,"issue_type":"task","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:57:18Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T18:57:18Z","dependencies":[{"issue_id":"spec-by6.3.4","depends_on_id":"spec-by6.3.2","type":"blocks","created_at":"2026-07-24T21:57:30Z","created_by":"Eugene Blikh","metadata":"{}"},{"issue_id":"spec-by6.3.4","depends_on_id":"spec-by6.3","type":"parent-child","created_at":"2026-07-24T21:57:17Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":1,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"spec-by6.3.4","title":"mcpsrv: spec_comment tool (read + reply, no resolve)","description":"The agent half of the loop. spec_comment lets an agent list the comments on a proposal it authored and reply to them; it must NOT resolve one, since an agent marking its own critique resolved defeats the auto-merge gate. Reuses the Phase 3 provenance path (agent + agent_session) for reply authorship, and the existing owner+agents read gate from spec-jjo. mcpsrv/mcpsrv.go:39 still documents spec_comment as deliberately absent — update that comment when it lands.","notes":"Implemented. Divergence from this issue's original text, accepted: reads are NOT narrowed to 'a proposal the agent authored'. Agent identity is self-declared (X-Agent/X-Agent-Session headers) and all agents share one token, so an authorship check would gate on a string the caller chooses — stricter on paper than the owner+agents read plane (spec-jjo) it sits in, while enforcing nothing. Revisit if/when per-agent token scoping lands (spec-by6 backlog).\n\nNo-resolve is enforced by TYPE: Commenter names only Threads/ReplyTo/GetProposal/ProposalDiff, so service.CommentOn and service.ResolveThread are unreachable from the handler however service/ grows. Writer is now Proposer+Commenter.\n\nExtra beyond the issue: replying requires proposal+thread and checks membership, because a thread id is global and service.ReplyTo takes only the id — a mistyped id would otherwise reply on a stranger's proposal.","status":"open","priority":3,"issue_type":"task","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:57:18Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T19:44:54Z","dependencies":[{"issue_id":"spec-by6.3.4","depends_on_id":"spec-by6.3.2","type":"blocks","created_at":"2026-07-24T21:57:30Z","created_by":"Eugene Blikh","metadata":"{}"},{"issue_id":"spec-by6.3.4","depends_on_id":"spec-by6.3","type":"parent-child","created_at":"2026-07-24T21:57:17Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":1,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"spec-by6.2","title":"Attachments: image embeds render as visibly missing","description":"gitx walks .md only, so an archive built from a git tree has no attachment index and ![[diagram.png]] resolves to a visibly-missing link. doc.FromPages already accepts an assets map — the gap is producing one. Two options: (a) gitx.WalkBlobs + an asset index threaded through service.Archive, plus a read-plane route serving the blob; (b) adopt Mermaid-by-convention and document that binary attachments are out of scope. Carried over from spec-wcr item 7 (triaged 2026-07-24: a feature, not a latent bug).","status":"open","priority":3,"issue_type":"feature","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:12:24Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T18:12:24Z","dependencies":[{"issue_id":"spec-by6.2","depends_on_id":"spec-by6","type":"parent-child","created_at":"2026-07-24T21:12:24Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"spec-45j","title":"Phase 5a-2: webhook tables, SDL, resolvers, delivery on proposal open/merge/reject","description":"On top of 5a-1: gql_proposal_wh_sub/_delivery tables + migration, WebhookSubscription/Delivery/Payload SDL, create/delete mutations + webhooks queries, and fire via webhooks.Schedule on proposal lifecycle events (service event hook -\u003e queue). Reference pages.sr.ht webhooks/webhooks.go + graph/model/webhooks.go.","status":"closed","priority":3,"issue_type":"feature","owner":"bigbes@gmail.com","created_at":"2026-07-24T02:05:29Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T09:50:16Z","closed_at":"2026-07-24T09:50:16Z","close_reason":"Webhook tables/SDL/resolvers/delivery all shipped and verified end-to-end.","dependencies":[{"issue_id":"spec-45j","depends_on_id":"spec-3m9","type":"parent-child","created_at":"2026-07-24T05:05:30Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"spec-8y4","title":"Phase 5a-1: adopt core-go graph conventions (directives, database.Model, cursors, auth+user model)","description":"Retrofit spec's minimal hand-written graph onto core-go's stack, the prerequisite for GraphQL-native webhooks. Per pages.sr.ht: @access/@private directives + impls, core-go database.Model for space/document/proposal/project/search types, cursor pagination, core-go auth.Middleware + a single-owner user table. Blueprint from ~/data/home/tmp/pages.sr.ht.","status":"closed","priority":3,"issue_type":"feature","owner":"bigbes@gmail.com","created_at":"2026-07-24T02:05:29Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T09:50:17Z","closed_at":"2026-07-24T09:50:17Z","close_reason":"Adopted core-go server stack for /query (faithful path); the convention pieces webhooks needed (auth/database/server context, database.Model for webhook models, gqlgen) are in.","dependencies":[{"issue_id":"spec-8y4","depends_on_id":"spec-3m9","type":"parent-child","created_at":"2026-07-24T05:05:29Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} diff --git a/scss/main.scss b/scss/main.scss index cc50c75fd77d0b735d35b7611a5032aaeac6fabb..7431c8be0e9acb2ea038d213afba647ac23fbb11 100644 --- a/scss/main.scss +++ b/scss/main.scss @@ -116,6 +116,12 @@ // block per change, and inline / spans or a two-column view depending // on how badly the block was rewritten. Colours follow Bootstrap's success and // danger so added and removed read the same here as everywhere else. +// +// Every block of the proposed document is on the page, because every block can +// be commented on — so the visual weight is what keeps this a diff rather than a +// document dump. A changed block is bordered, tinted and full size; an unchanged +// one is a collapsed, dimmed, borderless line. That contrast is load-bearing: +// remove it and a reviewer can no longer see at a glance what the agent did. .prosediff { font-size: 0.95rem; @@ -176,6 +182,47 @@ font-style: italic; } + // Unchanged text: present, addressable, and deliberately quiet. It is a + //
collapsed to one line, so a long untouched section costs a few + // dim rows rather than screens of prose between the changes. + .ph-context { + padding: 0; + margin: 0; + border-left-color: transparent; + color: $gray-600; + + @media (prefers-color-scheme: dark) { + color: $gray-500; + } + + > .ph-context-label { + padding: 0.1rem 0.6rem; + font-size: 0.85rem; + cursor: pointer; + list-style-position: outside; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + } + + // Expanded, it indents under its own summary and stays uncoloured: it is + // still context, even while being read. + > .ph-body, + > .ph-code, + > .ph-threads, + > .ph-form { + margin-left: 1.2rem; + } + + &[open] > .ph-context-label { + color: $gray-700; + + @media (prefers-color-scheme: dark) { + color: $gray-400; + } + } + } + // Inline marks: a deletion is struck through in danger, an insertion is // underlined in success. Both keep a faint background so a one-word change is // visible without reading the colour. @@ -238,3 +285,110 @@ } } } + +// ---- Review threads ------------------------------------------------------- +// +// Rendered both inside a diff block (web/diff.go executes "blockthreads" into +// the block's markup) and, for comments whose anchor is lost, in the page's own +// area — so these styles are outside .prosediff and apply in both places. + +.ph-threads { + margin: 0.35rem 0 0.15rem; +} + +.ph-thread { + margin: 0.35rem 0; + padding: 0.4rem 0.6rem; + border-left: 3px solid $info; + background: rgba($info, 0.06); + border-radius: 2px; + + // A resolved thread is history: it stays readable and stays where it was, but + // stops asking for attention the way an open one does. + &.ph-thread-resolved { + border-left-color: $gray-400; + background: transparent; + opacity: 0.7; + } +} + +.ph-thread-head { + font-size: 0.8rem; + margin-bottom: 0.2rem; + + .ph-author { + font-weight: 700; + } + + .ph-when { + color: $gray-600; + margin-left: 0.35rem; + + @media (prefers-color-scheme: dark) { + color: $gray-400; + } + } + + .badge { + margin-left: 0.35rem; + } +} + +// A comment is prose someone typed, newlines included; the server escapes it and +// the browser must not then reflow it into one paragraph. +.ph-comment-body { + white-space: pre-wrap; + word-break: break-word; +} + +.ph-reply { + margin-top: 0.4rem; + padding-left: 0.6rem; + border-left: 2px solid $gray-400; + + @media (prefers-color-scheme: dark) { + border-left-color: $gray-700; + } +} + +.ph-thread-actions { + display: flex; + flex-wrap: wrap; + gap: 0.5rem; + margin-top: 0.4rem; +} + +// The compose and reply forms fold away behind a summary: the review page shows +// one control per block, and a textarea per block would bury the diff. +.ph-form { + font-size: 0.85rem; + + > summary { + cursor: pointer; + color: $gray-600; + + @media (prefers-color-scheme: dark) { + color: $gray-400; + } + } + + textarea { + margin: 0.3rem 0; + } +} + +.ph-compose { + margin: 0.2rem 0 0.4rem; +} + +// Comments whose block is gone. The warning border is the point: they are shown +// because dropping them would hide a critique, and marked because they no longer +// describe anything on this page. +.ph-lost { + border-top-color: $warning; + + .ph-thread { + border-left-color: $warning; + background: rgba($warning, 0.06); + } +} diff --git a/service/comment.go b/service/comment.go index bd49fd4d950a64ffe897b3d3155b9863d565b1a1..5a79fdf268130a44c118acb43b81b8bb54da8986 100644 --- a/service/comment.go +++ b/service/comment.go @@ -256,51 +256,36 @@ func anchorBlocksOf(src []byte) []core.AnchorBlock { return core.AnchorBlocks(hashes, paths) } -// AnchorOf builds the anchor for a block of a document, numbering it the way -// [core.AnchorBlocks] does. +// AnchorOf builds the anchor for a block of a document. // -// A surface offering a "comment on this block" control has a document and a -// position in prosediff's document-global block order; the anchor needs the -// position within the block's own heading path instead. Converting here means -// the web form and the MCP tool cannot each get the numbering subtly different, -// which would put their comments on different blocks of the same document. +// A surface offering a "comment on this block" control has a position in +// prosediff's document-global block order; the anchor needs the position within +// the block's own heading path instead. Converting here means the web form and +// the MCP tool cannot each get the numbering subtly different, which would put +// their comments on different blocks of the same document. +// +// It reads that numbering out of [core.AnchorBlocks] — the same call +// [AnchorThreads] resolves against — rather than recomputing it. A second +// implementation of "which block is this within its section" would agree with +// the first only for as long as nobody edited either, and the failure it would +// eventually produce is a comment stored against one block and displayed +// against another. func AnchorOf(docID string, src []byte, ordinal int, side core.CommentSide) (core.CommentAnchor, error) { - segs := prosediff.Segment(src) - if ordinal < 0 || ordinal >= len(segs) { + blocks := anchorBlocksOf(src) + if ordinal < 0 || ordinal >= len(blocks) { return core.CommentAnchor{}, fmt.Errorf("%w: block %d is outside the document's %d blocks", - ErrInvalid, ordinal, len(segs)) - } - blk := segs[ordinal] - - // The within-section index is a count of preceding blocks sharing the - // heading path, which is exactly what AnchorBlocks assigns. - index := 0 - for _, prior := range segs[:ordinal] { - if sameHeadingPath(prior.HeadingPath, blk.HeadingPath) { - index++ - } + ErrInvalid, ordinal, len(blocks)) } + b := blocks[ordinal] return core.CommentAnchor{ DocID: docID, - HeadingPath: blk.HeadingPath, - Index: index, - BlockHash: blk.Hash, + HeadingPath: b.HeadingPath, + Index: b.Index, + BlockHash: b.Hash, Side: side, }, nil } -func sameHeadingPath(a, b []string) bool { - if len(a) != len(b) { - return false - } - for i := range a { - if a[i] != b[i] { - return false - } - } - return true -} - // commentView maps a stored comment onto the surface shape. func commentView(c *db.Comment) Comment { return Comment{ diff --git a/web/comments.go b/web/comments.go new file mode 100644 index 0000000000000000000000000000000000000000..c705208007747a1b1646d1aa9158f4c79eac17c7 --- /dev/null +++ b/web/comments.go @@ -0,0 +1,412 @@ +package web + +import ( + "net/http" + "sort" + "strconv" + "strings" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/doc" + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +// commentTimeFormat is how a comment's timestamp reads on the page. Minutes are +// the finest unit a review conversation cares about. +const commentTimeFormat = "2006-01-02 15:04" + +// reviewControls is who may do what to the review threads on one page. +// +// The two authorities are kept apart because the service keeps them apart: only +// the owner may open or resolve a thread, while owner and agent alike may +// reply. This struct decides which controls are *drawn*; it is not the check — +// service.CommentOn and service.ResolveThread refuse a non-owner themselves and +// this page surfaces their ErrForbidden. Drawing a control the service would +// refuse is the failure this prevents, not privilege escalation. +type reviewControls struct { + // Owner draws the compose form and the resolve/reopen control. + Owner bool + // Reply draws the reply form. + Reply bool + // ActionBase is the proposal's URL, which every form posts under. + ActionBase string +} + +// threadPanel is one review thread as the page renders it: the root comment, +// its replies, and the controls the viewer is allowed. +type threadPanel struct { + ID int + Author string + Agent bool + Body string + When string + Replies []replyLine + Resolved bool + + // Note is a short badge naming an anchor that no longer fits exactly, and + // NoteWhy is its tooltip. Both are empty for core.AnchorExact: a comment that + // still sits on the text it was written about needs no annotation. + Note string + NoteWhy string + + // DocPath is set only for a thread the page could not place on a block, where + // the document it belonged to is the only remaining locator. A placed thread + // leaves it empty — the document heading it renders under already says it. + DocPath string + + CanReply bool + CanResolve bool + ActionBase string +} + +// replyLine is one reply under a root comment. Threading is flat by design: +// there is a root and there are answers to it, and nothing nests deeper. +type replyLine struct { + Author string + Agent bool + Body string + When string +} + +// composeForm is the "comment on this block" form's hidden state. +// +// It carries the block's document-global prosediff ordinal rather than a +// pre-built anchor: building the anchor is service.AnchorOf's job, and doing it +// at submit time re-reads the branch, so a form drawn against a revision the +// agent has since replaced is caught by Hash rather than silently anchored to +// whatever now sits at that ordinal. +type composeForm struct { + ActionBase string + DocPath string + Ordinal int + Side core.CommentSide + Hash string +} + +// blockComments is what the "blockthreads" template renders for one block: the +// threads already on it and, for the owner, the form that opens a new one. +type blockComments struct { + Threads []threadPanel + Compose *composeForm +} + +// threadPanelOf maps a resolved service.Thread onto the page's shape. +// +// The anchor state is reported, never hidden. core.AnchorEdited means the block +// is still where the comment pointed but its text has moved on, so the critique +// may no longer fit — the reader has to be told that, because a stale critique +// read as a current one is worse than no critique. core.AnchorOutdated means the +// anchor is lost entirely; such a thread is never drawn against a block at all, +// only in the page's unplaced area. +func threadPanelOf(t service.Thread, c reviewControls) threadPanel { + p := threadPanel{ + ID: t.Root.ID, + Author: t.Root.Author, + Agent: t.Root.Agent, + Body: t.Root.Body, + When: t.Root.Created.Format(commentTimeFormat), + Resolved: !t.Open(), + CanReply: c.Reply, + CanResolve: c.Owner, + ActionBase: c.ActionBase, + } + for _, r := range t.Replies { + p.Replies = append(p.Replies, replyLine{ + Author: r.Author, + Agent: r.Agent, + Body: r.Body, + When: r.Created.Format(commentTimeFormat), + }) + } + switch t.State { + case core.AnchorEdited: + p.Note = "block edited since" + p.NoteWhy = "The block is still here but its text changed after this comment was written." + case core.AnchorOutdated: + p.Note = "anchor lost" + p.NoteWhy = "The block this comment was written about is no longer in the proposed revision." + } + return p +} + +// lostPanels renders the threads no block claimed, in a stable order. +// +// They carry their document path because that is all that is left of where they +// pointed, and they are sorted so two renders of the same page agree: the +// threads arrive grouped per document from a map walk, which has no order of its +// own. +func lostPanels(threads []service.Thread, c reviewControls) []threadPanel { + out := make([]threadPanel, 0, len(threads)) + for _, t := range threads { + p := threadPanelOf(t, c) + p.DocPath = t.DocPath + out = append(out, p) + } + sort.Slice(out, func(i, j int) bool { + if out[i].DocPath != out[j].DocPath { + return out[i].DocPath < out[j].DocPath + } + return out[i].ID < out[j].ID + }) + return out +} + +// docIDFor derives a document's anchoring key: its frontmatter id when it has a +// well-formed one, otherwise its path without the extension. That is the +// archive's addressing rule (doc/scan.go), and anchoring on the id rather than +// the path is what lets a comment survive a rename. +// +// The archive also refuses an id that two documents claim; that contest cannot +// be judged here, because a review holds only the documents the proposal changes +// and not the whole revision they sit in. The consequence is bounded: the anchor +// carries the document path as well, and it is the path that service.AnchorThreads +// resolves a thread by. +func docIDFor(path string, src []byte) string { + front, _ := doc.ParseFront(src) + if core.ValidateDocID(front.ID) == nil { + return front.ID + } + return strings.TrimSuffix(path, core.DocExt) +} + +// ---- handlers ------------------------------------------------------------- + +// handleProposalComment opens a review thread on one block of a proposed +// document. +// +// The anchor is built here, at submit time, from the branch as it now reads — +// not from anything the form carries — because a form drawn ten minutes ago +// describes a revision the agent may have replaced since. The form's block hash +// is the guard on that: if the block at the submitted ordinal no longer hashes +// to what the reviewer was looking at, the comment is refused rather than +// attached to whatever moved into that position. +// +// service.AnchorOf does the ordinal conversion. Hand-rolling it here would put +// the browser's comments on different blocks than the MCP tool's, which is the +// one way two surfaces of the same conversation can disagree without either +// looking broken. +func (s *Server) handleProposalComment(w http.ResponseWriter, r *http.Request) { + p, ok := s.commentPost(w, r) + if !ok { + return + } + + docPath := r.PostFormValue("doc") + body := strings.TrimSpace(r.PostFormValue("body")) + ordinal, err := strconv.Atoi(r.PostFormValue("block")) + if err != nil || ordinal < 0 { + s.renderError(w, r, http.StatusBadRequest, "that comment names no block") + return + } + side, err := core.ParseCommentSide(r.PostFormValue("side")) + if err != nil { + s.renderError(w, r, http.StatusBadRequest, err.Error()) + return + } + if body == "" { + s.renderError(w, r, http.StatusBadRequest, "a comment needs a body") + return + } + + docs, err := s.reader.ProposalDiff(r.Context(), p) + if err != nil { + s.fail(w, r, err) + return + } + src, found := sourceOfSide(docs, docPath, side) + if !found { + s.renderError(w, r, http.StatusNotFound, "this proposal does not change that document") + return + } + + anchor, err := service.AnchorOf(docIDFor(docPath, src), src, ordinal, side) + if err != nil { + s.fail(w, r, err) + return + } + // The hash is required, not merely checked when present. Skipping the guard + // for a submission that omits it would mean a later template refactor that + // dropped the hidden field disabled the staleness check silently, with every + // test still passing — the comment would still store a coherent anchor, just + // not the block the reviewer was reading. + switch want := r.PostFormValue("hash"); { + case want == "": + s.renderError(w, r, http.StatusBadRequest, "that comment names no block revision") + return + case want != anchor.BlockHash: + s.renderError(w, r, http.StatusConflict, + "that block changed since this page was loaded; reload the proposal and comment again") + return + } + + thread, err := s.reader.CommentOn(r.Context(), service.CommentRequest{ + Principal: authn.PrincipalFromContext(r.Context()), + Space: p.Space, + ProposalID: p.ID, + DocPath: docPath, + Anchor: anchor, + Body: body, + }) + if err != nil { + s.fail(w, r, err) + return + } + s.backToThread(w, r, p, thread.Root.ID) +} + +// handleProposalReply appends a reply to an existing thread. Both principals may +// reply — that is the loop's turn-taking, the owner critiques and the agent +// answers — and service.ReplyTo is what says so. +func (s *Server) handleProposalReply(w http.ResponseWriter, r *http.Request) { + p, ok := s.commentPost(w, r) + if !ok { + return + } + threadID, ok := s.threadOfProposal(w, r, p.ID) + if !ok { + return + } + body := strings.TrimSpace(r.PostFormValue("body")) + if body == "" { + s.renderError(w, r, http.StatusBadRequest, "a reply needs a body") + return + } + if _, err := s.reader.ReplyTo(r.Context(), authn.PrincipalFromContext(r.Context()), threadID, body); err != nil { + s.fail(w, r, err) + return + } + s.backToThread(w, r, p, threadID) +} + +// handleProposalResolve closes a thread, or reopens it when the form says so. +// +// Owner-only, enforced by service.ResolveThread rather than re-stated here: an +// agent that could resolve the thread opened against its own proposal could +// clear the auto-merge gate that thread exists to hold shut. +func (s *Server) handleProposalResolve(w http.ResponseWriter, r *http.Request) { + p, ok := s.commentPost(w, r) + if !ok { + return + } + threadID, ok := s.threadOfProposal(w, r, p.ID) + if !ok { + return + } + resolved := r.PostFormValue("resolved") == "1" + who := authn.PrincipalFromContext(r.Context()) + if err := s.reader.ResolveThread(r.Context(), who, threadID, resolved); err != nil { + s.fail(w, r, err) + return + } + s.backToThread(w, r, p, threadID) +} + +// commentPost is the prologue every comment POST shares: the cross-site guard, +// read authority, a parsed form, and the proposal the URL names — refusing one +// that belongs to another space for the same reason the review page does, that +// the id is global but the link names its space. +// +// The one authority it checks is the read ACL — these handlers go on to read the +// proposal branch to place an anchor and to list a proposal's threads, and that +// is a read like any other. Which principal may *write* what is deliberately not +// restated: opening and resolving are the owner's alone and replying is not, the +// service knows both rules, and a second copy here would be a second place for +// them to be wrong. +func (s *Server) commentPost(w http.ResponseWriter, r *http.Request) (service.Proposal, bool) { + if !s.sameOrigin(r) { + s.renderError(w, r, http.StatusForbidden, "this request did not originate from this site") + return service.Proposal{}, false + } + if !mayRead(r) { + s.renderError(w, r, http.StatusForbidden, "you may not read this proposal") + return service.Proposal{}, false + } + if err := r.ParseForm(); err != nil { + s.renderError(w, r, http.StatusBadRequest, "malformed form submission") + return service.Proposal{}, false + } + ref, err := spaceRefFrom(r) + if err != nil { + s.fail(w, r, err) + return service.Proposal{}, false + } + id, ok := proposalIDFrom(r) + if !ok { + s.renderError(w, r, http.StatusNotFound, "no such proposal") + return service.Proposal{}, false + } + p, err := s.reader.GetProposal(r.Context(), id) + if err != nil { + s.fail(w, r, err) + return service.Proposal{}, false + } + if p.Space != ref { + s.renderError(w, r, http.StatusNotFound, "no such proposal in this space") + return service.Proposal{}, false + } + return p, true +} + +// threadOfProposal reads the "thread" form field and checks that it names a root +// thread of *this* proposal. +// +// Thread ids are global while the URL names one proposal, so without this a form +// could carry another proposal's thread id and have the reply land somewhere the +// reviewer was never looking. Matching against the roots also keeps threading +// flat: a reply's id is not a root, so it cannot be replied to. +func (s *Server) threadOfProposal(w http.ResponseWriter, r *http.Request, proposalID int) (int, bool) { + threadID, err := strconv.Atoi(r.PostFormValue("thread")) + if err != nil || threadID <= 0 { + s.renderError(w, r, http.StatusBadRequest, "that action names no review thread") + return 0, false + } + threads, err := s.reader.Threads(r.Context(), authn.PrincipalFromContext(r.Context()), proposalID) + if err != nil { + s.fail(w, r, err) + return 0, false + } + for _, t := range threads { + if t.Root.ID == threadID { + return threadID, true + } + } + s.renderError(w, r, http.StatusNotFound, "no such review thread on this proposal") + return 0, false +} + +// backToThread redirects to the proposal page, scrolled to the thread that was +// just written, so a reload does not re-submit and the reviewer lands on what +// they said rather than at the top of a long diff. +func (s *Server) backToThread(w http.ResponseWriter, r *http.Request, p service.Proposal, threadID int) { + http.Redirect(w, r, proposalHref(p)+"#thread-"+strconv.Itoa(threadID), http.StatusSeeOther) +} + +// sourceOfSide returns the revision of a document a comment on one side anchors +// against: the proposed text for a comment on the new side, the base for one on +// a block the proposal deletes. +func sourceOfSide(docs []service.ProposalDoc, path string, side core.CommentSide) ([]byte, bool) { + for _, d := range docs { + if d.Path != path { + continue + } + if side == core.SideOld { + return d.Base, d.Base != nil + } + return d.Proposed, true + } + return nil, false +} + +// unresolvedThreads counts the threads still awaiting the owner. It is what the +// page states next to the approve control: an open thread suppresses policy +// auto-merge, so the count explains why a proposal is sitting here. +func unresolvedThreads(threads []service.Thread) int { + n := 0 + for _, t := range threads { + if t.Open() { + n++ + } + } + return n +} diff --git a/web/comments_test.go b/web/comments_test.go new file mode 100644 index 0000000000000000000000000000000000000000..70f305f19c3319d9df9b87b461afc9a675203588 --- /dev/null +++ b/web/comments_test.go @@ -0,0 +1,478 @@ +package web + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "strings" + "testing" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +// The document under review: two paragraphs under one heading, one of which the +// proposal edits. Block 1 is "The first paragraph."; block 2 is the edited one. +const ( + commentBase = "# Storage\n\nThe first paragraph.\n\nThe second paragraph.\n" + commentProposed = "# Storage\n\nThe first paragraph.\n\nThe second paragraph, revised.\n" + // It has no frontmatter, so its anchoring key is its path minus the + // extension — the archive's rule for a document with no well-formed id. + commentDocID = "specs/0007-storage" +) + +func commentDocs() []service.ProposalDoc { + return []service.ProposalDoc{{ + Path: "specs/0007-storage.md", + Base: []byte(commentBase), + Proposed: []byte(commentProposed), + }} +} + +// seedThread hangs a thread off one block of the proposed document, anchored the +// way the service would anchor it — through service.AnchorOf, so the test cannot +// disagree with production about which block it named. +func seedThread(t *testing.T, r *fakeReader, proposalID, ordinal int, body string) *service.Thread { + t.Helper() + anchor, err := service.AnchorOf(commentDocID, []byte(commentProposed), ordinal, core.SideNew) + if err != nil { + t.Fatalf("AnchorOf: %v", err) + } + r.nextThread++ + th := &service.Thread{ + Root: service.Comment{ID: r.nextThread, Author: "bigbes", Body: body}, + DocPath: "specs/0007-storage.md", + Anchor: anchor, + Block: -1, + } + r.threads[proposalID] = append(r.threads[proposalID], th) + return th +} + +// postForm issues a form POST with a urlencoded body, as a browser would. +func postForm(t *testing.T, h http.Handler, target, user, origin string, form url.Values) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(http.MethodPost, target, strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + if user != "" { + login(req, user) + } + if origin != "" { + req.Header.Set("Origin", origin) + } + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + return rec +} + +// commentServer is a fake reader holding one open proposal with one changed +// document, plus the handler. +func commentServer(t *testing.T) (http.Handler, *fakeReader) { + t.Helper() + r := newFakeReader() + seedProposal(r, openProposal(), commentDocs()) + h, reader, _ := testServerWith(t, r) + return h, reader +} + +// TestDocIDForFollowsTheArchiveAddressingRule proves a comment anchors to the +// document's frontmatter id when it has a well-formed one — which is what lets +// the comment survive a rename — and to its path when it does not. +func TestDocIDForFollowsTheArchiveAddressingRule(t *testing.T) { + withID := []byte("---\nid: SPEC-0007\ntitle: Storage\n---\n\n# Storage\n") + if got := docIDFor("specs/0007-storage.md", withID); got != "SPEC-0007" { + t.Errorf("docIDFor = %q, want the frontmatter id", got) + } + if got := docIDFor("notes/plain.md", []byte("# Just a note\n")); got != "notes/plain" { + t.Errorf("docIDFor = %q, want the path without its extension", got) + } +} + +// TestProposalPageDrawsComposeFormsForTheOwner proves the owner can start a +// thread on a block — including on one the proposal did not change, which is +// the whole point of rendering unchanged blocks. +func TestProposalPageDrawsComposeFormsForTheOwner(t *testing.T) { + h, _ := commentServer(t) + body := get(t, h, "/~bigbes/rfcs/p/7", "bigbes").Body.String() + + if !strings.Contains(body, "/p/7/comment") { + t.Fatalf("the owner has no compose form; body:\n%s", body) + } + // One form per rendered block: heading, unchanged paragraph, changed one. + if n := strings.Count(body, `action="/~bigbes/rfcs/p/7/comment"`); n != 3 { + t.Errorf("%d compose forms, want one per rendered block (3); body:\n%s", n, body) + } + if !strings.Contains(body, `name="side" value="new"`) { + t.Errorf("the compose form does not name the side it anchors to; body:\n%s", body) + } +} + +// TestProposalPageHidesOwnerControlsFromAnAgent proves a principal who may read +// and reply is not offered the two controls that are the owner's: opening a +// thread and resolving one. +func TestProposalPageHidesOwnerControlsFromAnAgent(t *testing.T) { + h, r := commentServer(t) + seedThread(t, r, 7, 1, "please reword this") + + body := getAgent(t, h, "/~bigbes/rfcs/p/7", agentTk).Body.String() + if !strings.Contains(body, "please reword this") { + t.Fatalf("an agent cannot see the comments on its own proposal; body:\n%s", body) + } + if strings.Contains(body, "/p/7/comment") { + t.Errorf("an agent was offered the compose form; body:\n%s", body) + } + if strings.Contains(body, "/p/7/resolve") { + t.Errorf("an agent was offered the resolve control; body:\n%s", body) + } + if !strings.Contains(body, "/p/7/reply") { + t.Errorf("an agent was not offered the reply form, which is its half of the loop; body:\n%s", body) + } +} + +// TestProposalPageMarksAnEditedAnchor proves a comment whose block has changed +// since it was written is shown — the reviewer still needs it — and marked, so +// nobody reads a stale critique as a current one. +func TestProposalPageMarksAnEditedAnchor(t *testing.T) { + h, r := commentServer(t) + // Anchored to the second paragraph as it read before the revision, so the + // hash misses and heading-path + index carries it: edited, not lost. + anchor, err := service.AnchorOf(commentDocID, []byte(commentBase), 2, core.SideNew) + if err != nil { + t.Fatalf("AnchorOf: %v", err) + } + r.nextThread++ + r.threads[7] = append(r.threads[7], &service.Thread{ + Root: service.Comment{ID: r.nextThread, Author: "bigbes", Body: "the tense is wrong"}, + DocPath: "specs/0007-storage.md", + Anchor: anchor, + Block: -1, + }) + + body := get(t, h, "/~bigbes/rfcs/p/7", "bigbes").Body.String() + if !strings.Contains(body, "the tense is wrong") { + t.Fatalf("an edited-anchor comment is not on the page; body:\n%s", body) + } + if !strings.Contains(body, "block edited since") { + t.Errorf("an edited anchor is shown as if it still fitted; body:\n%s", body) + } + if strings.Contains(body, "lost their anchor") { + t.Errorf("an edited anchor was demoted to lost; body:\n%s", body) + } +} + +// TestProposalPageKeepsAnOutdatedComment proves a comment whose block is gone is +// still on the page, in the area that says so, and is not attached to whatever +// block happens to be nearby. +func TestProposalPageKeepsAnOutdatedComment(t *testing.T) { + h, r := commentServer(t) + // Anchored to a paragraph that exists in neither revision on the page. + const gone = commentBase + "\nA paragraph the revision dropped.\n" + anchor, err := service.AnchorOf(commentDocID, []byte(gone), 3, core.SideNew) + if err != nil { + t.Fatalf("AnchorOf: %v", err) + } + r.nextThread++ + r.threads[7] = append(r.threads[7], &service.Thread{ + Root: service.Comment{ID: r.nextThread, Author: "bigbes", Body: "this claim is unsupported"}, + DocPath: "specs/0007-storage.md", + Anchor: anchor, + Block: -1, + }) + + body := get(t, h, "/~bigbes/rfcs/p/7", "bigbes").Body.String() + if !strings.Contains(body, "this claim is unsupported") { + t.Fatalf("an outdated comment was dropped from the page; body:\n%s", body) + } + lost := strings.Index(body, "Comments that lost their anchor") + if lost < 0 { + t.Fatalf("no area for comments whose anchor is lost; body:\n%s", body) + } + // It belongs to that area and to no block: the diff ends before it. + if strings.Index(body, "this claim is unsupported") < lost { + t.Errorf("an outdated comment was rendered against a block; body:\n%s", body) + } +} + +// TestProposalPageKeepsCommentsOnARevertedDocument proves a thread whose +// document the proposal no longer changes — so no diff renders it at all — is +// still shown rather than silently disappearing. +func TestProposalPageKeepsCommentsOnARevertedDocument(t *testing.T) { + h, r := commentServer(t) + r.nextThread++ + r.threads[7] = append(r.threads[7], &service.Thread{ + Root: service.Comment{ID: r.nextThread, Author: "bigbes", Body: "comment on a reverted file"}, + DocPath: "specs/reverted.md", + Block: -1, + }) + + body := get(t, h, "/~bigbes/rfcs/p/7", "bigbes").Body.String() + if !strings.Contains(body, "comment on a reverted file") { + t.Fatalf("a comment on a document no longer changed was dropped; body:\n%s", body) + } + if !strings.Contains(body, "specs/reverted.md") { + t.Errorf("the lost comment does not say which document it came from; body:\n%s", body) + } +} + +// TestCommentBodyIsEscaped proves a comment is text: markup in a body cannot +// become markup on the page. +func TestCommentBodyIsEscaped(t *testing.T) { + h, r := commentServer(t) + seedThread(t, r, 7, 1, ``) + + body := get(t, h, "/~bigbes/rfcs/p/7", "bigbes").Body.String() + if strings.Contains(body, "/, and — the load-bearing part — the // two-column fallback for shredded blocks. Every piece of document content is // HTML-escaped before it reaches the output; the only markup this produces is -// its own structure. -func renderDocDiff(oldSrc, newSrc []byte) diffView { - d := prosediff.Compare(oldSrc, newSrc) +// its own structure and the thread markup html/template escapes for it. +func renderDocDiff(in docDiff) diffView { + d := prosediff.Compare(in.Base, in.Proposed) view := diffView{Stats: d.Stats, Unchanged: !d.Stats.Changed()} + + r := newDocRenderer(in, d) if view.Unchanged { + // Nothing is rendered, so nothing can hold a thread. Handing them all + // back keeps the invariant this renderer is built on: every thread comes + // out either attached to a block or in Unplaced, and never neither. + view.Unplaced = r.unplaced() return view } @@ -58,9 +107,6 @@ func renderDocDiff(oldSrc, newSrc []byte) diffView { } for _, c := range d.Changes { - if c.Kind == prosediff.ChangeEqual { - continue // the review shows only what changed - } blk := c.New if blk == nil { blk = c.Old @@ -77,83 +123,353 @@ func renderDocDiff(oldSrc, newSrc []byte) diffView { b.WriteString(`
`) inHunk = true } - writeBlock(&b, c) + r.writeBlock(&b, c) } closeHunk() b.WriteString(`
`) view.HTML = template.HTML(b.String()) + view.Unplaced = r.unplaced() return view } -// writeBlock renders one changed block. Insert/delete/move show the whole -// block; a modify chooses among a code line diff, an inline word diff and the -// two-column view, on the rules the design pins. -func writeBlock(b *strings.Builder, c prosediff.BlockChange) { +// docRenderer renders the blocks of one document's diff. It holds the anchor +// numbering of both revisions and the threads still waiting for a block: a +// block claims its threads as it is written, and whatever is left over at the +// end never had a block on the page. +type docRenderer struct { + in docDiff + // anchors is each side's blocks, numbered within their heading path. + anchors map[core.CommentSide][]core.AnchorBlock + pending map[blockKey][]service.Thread +} + +func newDocRenderer(in docDiff, d *prosediff.Diff) *docRenderer { + r := &docRenderer{ + in: in, + anchors: map[core.CommentSide][]core.AnchorBlock{ + core.SideNew: blockAnchors(d.NewBlocks), + core.SideOld: blockAnchors(d.OldBlocks), + }, + pending: make(map[blockKey][]service.Thread, len(in.Threads)), + } + // A thread is placed by (side, block ordinal) and by nothing else. The + // anchor resolution already decided which block it belongs to, and any + // second-guessing here would be the one thing the anchor model forbids: a + // comment quietly moved onto a neighbouring paragraph. A thread whose anchor + // did not resolve (Block < 0) is never placed at all. + for _, t := range in.Threads { + if t.Block < 0 { + continue + } + k := blockKey{sideOf(t.Anchor.Side), t.Block} + r.pending[k] = append(r.pending[k], t) + } + return r +} + +// unplaced reports the threads no block claimed. It walks the input rather than +// the leftover map so the order is the one the service listed them in, not a +// map's. +func (r *docRenderer) unplaced() []service.Thread { + out := make([]service.Thread, 0, len(r.pending)) + for _, t := range r.in.Threads { + if t.Block < 0 { + out = append(out, t) + continue + } + k := blockKey{sideOf(t.Anchor.Side), t.Block} + if _, still := r.pending[k]; still { + out = append(out, t) + } + } + return out +} + +// target returns the anchor of the block a change offers to comment on, and +// whether it offers one at all. +// +// A comment goes on the new side, which is the text under review; the old side +// is for a block the proposal deletes, where there is no new text to point at. +// A move-out offers nothing: it is a pointer to text that is rendered at its +// new position, and anchoring it here would give one paragraph two places to be +// commented on. +func (r *docRenderer) target(c prosediff.BlockChange) (core.CommentAnchor, blockKey, bool) { + var side core.CommentSide + var blk *prosediff.Block switch c.Kind { - case prosediff.ChangeInsert: - writeWholeBlock(b, "ph-insert", "added", c.New) - case prosediff.ChangeDelete: - writeWholeBlock(b, "ph-delete", "removed", c.Old) - case prosediff.ChangeMoveIn: - fmt.Fprintf(b, `
%s moved here (was line %d)
`, - template.HTMLEscapeString(c.New.Label()), c.Old.StartLine) case prosediff.ChangeMoveOut: + return core.CommentAnchor{}, blockKey{}, false + case prosediff.ChangeDelete: + side, blk = core.SideOld, c.Old + default: + side, blk = core.SideNew, c.New + } + blocks := r.anchors[side] + if blk == nil || blk.Ordinal < 0 || blk.Ordinal >= len(blocks) { + return core.CommentAnchor{}, blockKey{}, false + } + ab := blocks[blk.Ordinal] + anchor := core.CommentAnchor{ + DocID: r.in.DocID, + HeadingPath: ab.HeadingPath, + Index: ab.Index, + BlockHash: ab.Hash, + Side: side, + } + return anchor, blockKey{side, blk.Ordinal}, true +} + +// writeBlock renders one block of the diff and the comment UI hanging off it. +// Insert/delete/move show the whole block; a modify chooses among a code line +// diff, an inline word diff and the two-column view, on the rules the design +// pins; an unchanged block renders as collapsed context. +func (r *docRenderer) writeBlock(b *strings.Builder, c prosediff.BlockChange) { + if c.Kind == prosediff.ChangeMoveOut { fmt.Fprintf(b, `
%s moved away (now line %d)
`, template.HTMLEscapeString(c.Old.Label()), c.New.StartLine) - case prosediff.ChangeModify: - writeModify(b, c) + return + } + + anchor, key, commentable := r.target(c) + var threads []service.Thread + if commentable { + threads = r.pending[key] + delete(r.pending, key) + } + + if c.Kind == prosediff.ChangeEqual { + r.writeContext(b, c, anchor, key, threads, commentable) + return } -} -// writeWholeBlock renders an inserted or deleted block: its whole text, in a -// 
 for a non-prose kind so code keeps its wrapping, and reflowed prose
-// otherwise.
-func writeWholeBlock(b *strings.Builder, class, verb string, blk *prosediff.Block) {
-	label := verb + " " + blk.Label()
-	fmt.Fprintf(b, `
%s`,
-		class, template.HTMLEscapeString(label))
-	writeBody(b, blk.Text, blk.Kind.Prose())
+	class, label := changedPresentation(c)
+	// A block the anchor numbering does not cover is still shown — losing
+	// document content would be worse than losing its comment affordance — it
+	// just carries no id and offers no form. Nothing in prosediff produces one
+	// today; this is the branch that keeps that assumption from being fatal.
+	fmt.Fprintf(b, `
%s`,
+		class, idAttr(anchor, commentable), template.HTMLEscapeString(label))
+	writeChangedBody(b, c)
+	if commentable {
+		r.writeComments(b, anchor, key, threads)
+	}
 	b.WriteString(`
`)
 }
 
-// writeModify renders a modified block. A code, frontmatter or HTML block has a
-// line-oriented edit script and renders line by line; a prose block has a word
-// edit script and renders inline when it stayed similar enough to follow, and
-// two-column when it did not.
-func writeModify(b *strings.Builder, c prosediff.BlockChange) {
-	label := "changed " + c.New.Label()
+// idAttr renders the id attribute of a commentable block, and nothing at all
+// for one that carries no anchor: an empty id="" is a fragment that matches
+// every such block at once.
+func idAttr(a core.CommentAnchor, commentable bool) string {
+	if !commentable {
+		return ""
+	}
+	return ` id="` + blockDOMID(a) + `"`
+}
+
+// writeContext renders an unchanged block.
+//
+// The rule used to be that the review shows only what changed, and it is now
+// that any block of a proposed document can be commented on — a reviewer's
+// objection is as often to the paragraph the agent left alone as to the one it
+// touched, and a block that is not on the page cannot be pointed at. So
+// unchanged text is rendered, but subordinate: collapsed behind a one-line
+// preview, so a screenful of context never competes with the marked-up blocks
+// and the page still reads at a glance as a diff. A block that already carries a
+// comment opens by default — it is no longer merely context once someone has
+// said something about it.
+func (r *docRenderer) writeContext(b *strings.Builder, c prosediff.BlockChange,
+	anchor core.CommentAnchor, key blockKey, threads []service.Thread, commentable bool) {
+
+	open := ""
+	if len(threads) > 0 {
+		open = " open"
+	}
+	fmt.Fprintf(b, `
%s`,
+		idAttr(anchor, commentable), open, template.HTMLEscapeString(blockPreview(c.New)))
+	writeBody(b, c.New.Text, c.New.Kind.Prose())
+	if commentable {
+		r.writeComments(b, anchor, key, threads)
+	}
+	b.WriteString(`
`)
+}
+
+// writeComments appends one block's threads and, for the owner, the form that
+// opens a new one.
+//
+// It renders through html/template rather than by hand because everything here
+// is prose someone else wrote; contextual auto-escaping is what keeps a comment
+// body text. The template is executed into a buffer first, for the reason
+// Server.render uses one: a template that fails halfway must not leave its
+// half-written markup inside the diff.
+func (r *docRenderer) writeComments(b *strings.Builder, anchor core.CommentAnchor,
+	key blockKey, threads []service.Thread) {
+
+	data := blockComments{}
+	for _, t := range threads {
+		data.Threads = append(data.Threads, threadPanelOf(t, r.in.Controls))
+	}
+	if r.in.Controls.Owner {
+		data.Compose = &composeForm{
+			ActionBase: r.in.Controls.ActionBase,
+			DocPath:    r.in.Path,
+			Ordinal:    key.ordinal,
+			Side:       key.side,
+			Hash:       anchor.BlockHash,
+		}
+	}
+	if len(data.Threads) == 0 && data.Compose == nil {
+		return
+	}
+
+	var buf bytes.Buffer
+	if err := blockThreadsTmpl.Execute(&buf, data); err != nil {
+		log.Printf("web: rendering comments on %s: %v", r.in.Path, err)
+		return
+	}
+	b.Write(buf.Bytes())
+}
+
+// changedPresentation is the class and the label of a changed block, decided
+// together: the two-column fallback changes both, and deciding it twice is how
+// a block ends up labelled "rewritten" while rendering inline.
+func changedPresentation(c prosediff.BlockChange) (class, label string) {
+	switch c.Kind {
+	case prosediff.ChangeInsert:
+		return "ph-insert", "added " + c.New.Label()
+	case prosediff.ChangeDelete:
+		return "ph-delete", "removed " + c.Old.Label()
+	case prosediff.ChangeMoveIn:
+		return "ph-move", fmt.Sprintf("%s moved here (was line %d)", c.New.Label(), c.Old.StartLine)
+	}
+
+	label = "changed " + c.New.Label()
 	if c.StructureOnly {
 		label = fmt.Sprintf("changed %s → %s (structure)", c.Old.Label(), c.New.Label())
 	}
 	if c.Moved {
 		label += fmt.Sprintf(" (moved from line %d)", c.Old.StartLine)
 	}
+	if twoColumn(c) {
+		return "ph-modify ph-columns", label + " (rewritten)"
+	}
+	return "ph-modify", label
+}
+
+// twoColumn reports whether a modified block falls back to the old/new columns:
+// a prose block (no line script) rewritten past the point where inline marks
+// stay readable.
+func twoColumn(c prosediff.BlockChange) bool {
+	return len(c.Lines) == 0 && c.Similarity < inlineSimilarityThreshold
+}
+
+// writeChangedBody renders the body of a changed block. A code, frontmatter or
+// HTML block has a line-oriented edit script and renders line by line; a prose
+// block has a word edit script and renders inline when it stayed similar enough
+// to follow, and two-column when it did not. An inserted, deleted or moved block
+// has no edit script and shows its whole text.
+//
+// A move-in used to render as a bare "moved here" marker. It shows its text now
+// because it is commentable, and a comment control on text the reviewer cannot
+// see is a control on nothing.
+func writeChangedBody(b *strings.Builder, c prosediff.BlockChange) {
+	switch c.Kind {
+	case prosediff.ChangeInsert, prosediff.ChangeMoveIn:
+		writeBody(b, c.New.Text, c.New.Kind.Prose())
+		return
+	case prosediff.ChangeDelete:
+		writeBody(b, c.Old.Text, c.Old.Kind.Prose())
+		return
+	}
 
 	if len(c.Lines) > 0 {
-		fmt.Fprintf(b, `
%s
`,
-			template.HTMLEscapeString(label))
+		b.WriteString(`
`)
 		writeLineSpans(b, c.Lines)
-		b.WriteString(`
`)
+		b.WriteString(`
`) return } - - if c.Similarity >= inlineSimilarityThreshold { - fmt.Fprintf(b, `
%s
`, - template.HTMLEscapeString(label)) + if !twoColumn(c) { + b.WriteString(`
`) writeInlineSpans(b, c.Words) - b.WriteString(`
`) + b.WriteString(`
`) return } // The two-column fallback: the block was rewritten enough that inline marks // would shred it. The old column keeps deletions, the new keeps insertions, // each still marked, so a reviewer reads two coherent paragraphs side by side. - fmt.Fprintf(b, `
%s (rewritten)
`, - template.HTMLEscapeString(label)) + b.WriteString(`
`) writeColumnSpans(b, c.Words, true) b.WriteString(`
`) writeColumnSpans(b, c.Words, false) - b.WriteString(`
`) + b.WriteString(`
`) +} + +// blockAnchors numbers a revision's blocks the way the anchor model does: +// within their own heading path, not document-globally. core.AnchorBlocks is +// the single spelling of that numbering — service.AnchorOf reproduces it for +// the anchor a comment stores — so the id a block carries here and the anchor +// the form posts back cannot drift apart. +func blockAnchors(blocks []prosediff.Block) []core.AnchorBlock { + hashes := make([]string, len(blocks)) + paths := make([][]string, len(blocks)) + for i, b := range blocks { + hashes[i], paths[i] = b.Hash, b.HeadingPath + } + return core.AnchorBlocks(hashes, paths) +} + +// blockDOMID is the id attribute a rendered block carries: a digest of its +// anchor tuple. +// +// Not its position on the page. An ordinal id renumbers whenever anything above +// it is inserted, so a link saved from one revision would silently scroll to a +// different paragraph in the next — exactly the relocation the anchor model +// refuses to do for comments, and the reader could not tell it had happened. +// The digest covers the whole tuple, block hash included, so a link into a block +// that has since been rewritten resolves to nothing at all rather than to its +// neighbour. The index keeps two blocks that repeat the same text under the same +// headings — "TBD" is written a dozen times in a real corpus — from sharing an +// id. +func blockDOMID(a core.CommentAnchor) string { + h := sha256.New() + // NUL separates the parts because it cannot occur in a heading, a path or a + // hash, so no two distinct tuples can hash the same byte string. + write := func(s string) { + h.Write([]byte(s)) + h.Write([]byte{0}) + } + write(a.DocID) + write(string(a.Side)) + for _, seg := range a.HeadingPath { + write(seg) + } + write(fmt.Sprint(a.Index)) + write(a.BlockHash) + return "b-" + hex.EncodeToString(h.Sum(nil))[:16] +} + +// blockPreview is the one line a collapsed context block shows: its text with +// whitespace collapsed and truncated, or its structural label when it has no +// text to show (a thematic break, an empty block). +func blockPreview(blk *prosediff.Block) string { + text := strings.Join(strings.Fields(blk.Text), " ") + if text == "" { + return blk.Label() + } + if runes := []rune(text); len(runes) > contextPreviewRunes { + return string(runes[:contextPreviewRunes]) + "…" + } + return text +} + +// sideOf defaults an empty side to the new one, the way service.CommentOn does +// when it stores a comment: an anchor read back without a side is a comment on +// the text under review. +func sideOf(s core.CommentSide) core.CommentSide { + if s == core.SideOld { + return core.SideOld + } + return core.SideNew } // writeBody renders whole-block text: escaped, in a 
 when it is not prose.
diff --git a/web/diff_internal_test.go b/web/diff_internal_test.go
index fe9b7559988731e9c43bcb454e363388d15a62da..a1393623b275b4a43bdef03ccc650d4eab38a789 100644
--- a/web/diff_internal_test.go
+++ b/web/diff_internal_test.go
@@ -3,13 +3,32 @@ package web
 import (
 	"strings"
 	"testing"
+
+	"sourcecraft.dev/bigbes/sr-ht-spec/core"
+	"sourcecraft.dev/bigbes/sr-ht-spec/service"
 )
 
+// render is renderDocDiff for a document nobody has commented on, which is what
+// the presentation tests are about.
+func render(oldSrc, newSrc []byte) diffView {
+	return renderDocDiff(docDiff{DocID: "SPEC-0007", Path: "specs/0007-storage.md", Base: oldSrc, Proposed: newSrc})
+}
+
+// blockIDs pulls the rendered block ids out of a diff, in document order, so a
+// test can compare two renders without depending on the digest itself.
+func blockIDs(html string) []string {
+	var ids []string
+	for _, rest := range strings.Split(html, ` id="b-`)[1:] {
+		ids = append(ids, "b-"+rest[:strings.IndexByte(rest, '"')])
+	}
+	return ids
+}
+
 // TestRenderDocDiffUnchanged proves a proposal that does not change a document
 // is reported as unchanged rather than as an empty diff.
 func TestRenderDocDiffUnchanged(t *testing.T) {
 	src := []byte("# Title\n\nOne paragraph.\n")
-	v := renderDocDiff(src, src)
+	v := render(src, src)
 	if !v.Unchanged {
 		t.Fatalf("Unchanged = false, want true for identical input")
 	}
@@ -23,7 +42,7 @@ func TestRenderDocDiffUnchanged(t *testing.T) {
 func TestRenderDocDiffInlineWordChange(t *testing.T) {
 	old := []byte("# Title\n\nThe quick brown fox jumps over the lazy dog.\n")
 	nw := []byte("# Title\n\nThe quick red fox jumps over the lazy dog.\n")
-	v := renderDocDiff(old, nw)
+	v := render(old, nw)
 	if v.Unchanged {
 		t.Fatalf("Unchanged = true, want a change")
 	}
@@ -49,7 +68,7 @@ func TestRenderDocDiffTwoColumnBelowThreshold(t *testing.T) {
 		"contentious debate that lasted well into the evening.\n")
 	nw := []byte("# Title\n\nThe committee rejected the annual budget after a brief and " +
 		"quiet discussion that ended early in the afternoon.\n")
-	v := renderDocDiff(old, nw)
+	v := render(old, nw)
 	html := string(v.HTML)
 	if !strings.Contains(html, "ph-columns") {
 		t.Fatalf("a wholesale rewrite did not render as two columns; got:\n%s", html)
@@ -64,7 +83,7 @@ func TestRenderDocDiffTwoColumnBelowThreshold(t *testing.T) {
 func TestRenderDocDiffEscapesContent(t *testing.T) {
 	old := []byte("# Title\n\nplain text here.\n")
 	nw := []byte("# Title\n\nplain  text here.\n")
-	v := renderDocDiff(old, nw)
+	v := render(old, nw)
 	html := string(v.HTML)
 	if strings.Contains(html, "