M .beads/interactions.jsonl => .beads/interactions.jsonl +2 -0
@@ 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."}}
M .beads/issues.jsonl => .beads/issues.jsonl +2 -2
@@ 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}
M scss/main.scss => scss/main.scss +154 -0
@@ 116,6 116,12 @@
// block per change, and inline <del>/<ins> 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
+ // <details> 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);
+ }
+}
M => +20 -35
@@ 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{
A => +412 -0
@@ 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
}
A => +478 -0
@@ 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, `<img src=x onerror="alert(1)">`)
body := get(t, h, "/~bigbes/rfcs/p/7", "bigbes").Body.String()
if strings.Contains(body, "<img src=x") {
t.Fatalf("an unescaped comment body reached the page; body:\n%s", body)
}
if !strings.Contains(body, "<img src=x") {
t.Errorf("the escaped comment body is missing; body:\n%s", body)
}
}
// TestOwnerOpensAThreadOnABlock proves the compose POST anchors through
// service.AnchorOf and redirects to what was just written.
func TestOwnerOpensAThreadOnABlock(t *testing.T) {
h, r := commentServer(t)
anchor, err := service.AnchorOf(commentDocID, []byte(commentProposed), 1, core.SideNew)
if err != nil {
t.Fatalf("AnchorOf: %v", err)
}
rec := postForm(t, h, "/~bigbes/rfcs/p/7/comment", "bigbes", "https://spec.example", url.Values{
"doc": {"specs/0007-storage.md"},
"block": {"1"},
"side": {"new"},
"hash": {anchor.BlockHash},
"body": {"say why"},
})
if rec.Code != http.StatusSeeOther {
t.Fatalf("status = %d, want 303; body:\n%s", rec.Code, rec.Body)
}
if len(r.threads[7]) != 1 {
t.Fatalf("%d threads stored, want 1", len(r.threads[7]))
}
got := r.threads[7][0]
if got.Anchor.BlockHash != anchor.BlockHash {
t.Errorf("stored anchor hash = %q, want the block's %q", got.Anchor.BlockHash, anchor.BlockHash)
}
if got.Anchor.Index != anchor.Index || got.Anchor.DocID != anchor.DocID {
t.Errorf("stored anchor = %+v, want service.AnchorOf's %+v", got.Anchor, anchor)
}
if loc := rec.Header().Get("Location"); loc != "/~bigbes/rfcs/p/7#thread-1" {
t.Errorf("redirect = %q, want the new thread on the proposal page", loc)
}
}
// TestCommentOnAStaleBlockIsRefused proves the form's block hash is a guard, not
// decoration: if the agent revised the document while the page was open, the
// comment is refused rather than attached to whatever moved into that position.
func TestCommentOnAStaleBlockIsRefused(t *testing.T) {
h, r := commentServer(t)
rec := postForm(t, h, "/~bigbes/rfcs/p/7/comment", "bigbes", "https://spec.example", url.Values{
"doc": {"specs/0007-storage.md"},
"block": {"1"},
"side": {"new"},
"hash": {"a-hash-from-a-revision-that-is-gone"},
"body": {"say why"},
})
if rec.Code != http.StatusConflict {
t.Fatalf("status = %d, want 409 for a block that moved under the form", rec.Code)
}
if len(r.threads[7]) != 0 {
t.Errorf("a comment was stored against a stale block: %+v", r.threads[7])
}
}
// The hash is required rather than checked-when-present, so that a template
// refactor dropping the hidden field fails here instead of silently disabling
// the staleness guard above — the comment would still store a coherent anchor,
// just not against the block the reviewer was actually reading.
func TestCommentWithoutABlockHashIsRefused(t *testing.T) {
h, r := commentServer(t)
rec := postForm(t, h, "/~bigbes/rfcs/p/7/comment", "bigbes", "https://spec.example", url.Values{
"doc": {"specs/0007-storage.md"},
"block": {"1"},
"side": {"new"},
"body": {"say why"},
})
if rec.Code != http.StatusBadRequest {
t.Fatalf("status = %d, want 400 when the form carries no block hash", rec.Code)
}
if len(r.threads[7]) != 0 {
t.Errorf("a comment was stored with no staleness guard: %+v", r.threads[7])
}
}
// TestAgentMayNotOpenAThread proves the owner-only rule is the service's and
// this page surfaces it: an agent posting the form directly gets a 403.
// The request is deliberately well-formed, block hash included: the refusal
// under test is the authority one, and a form the handler rejects before it
// ever reaches service.CommentOn would prove nothing about who may open a
// thread.
func TestAgentMayNotOpenAThread(t *testing.T) {
h, r := commentServer(t)
anchor, err := service.AnchorOf(commentDocID, []byte(commentProposed), 1, core.SideNew)
if err != nil {
t.Fatalf("AnchorOf: %v", err)
}
req := httptest.NewRequest(http.MethodPost, "/~bigbes/rfcs/p/7/comment",
strings.NewReader(url.Values{
"doc": {"specs/0007-storage.md"}, "block": {"1"}, "side": {"new"},
"hash": {anchor.BlockHash}, "body": {"mine now"},
}.Encode()))
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
req.Header.Set("Authorization", "Bearer "+agentTk)
req.Header.Set("Origin", "https://spec.example")
rec := httptest.NewRecorder()
h.ServeHTTP(rec, req)
if rec.Code != http.StatusForbidden {
t.Fatalf("status = %d, want 403 for an agent opening a thread", rec.Code)
}
if len(r.threads[7]) != 0 {
t.Errorf("the thread was opened despite the refusal: %+v", r.threads[7])
}
}
// TestAgentMayReply proves the other half of that rule: replying is how the
// agent answers a critique, and it is allowed.
func TestAgentMayReply(t *testing.T) {
h, r := commentServer(t)
th := seedThread(t, r, 7, 1, "please reword this")
req := httptest.NewRequest(http.MethodPost, "/~bigbes/rfcs/p/7/reply",
strings.NewReader(url.Values{"thread": {strconv.Itoa(th.Root.ID)}, "body": {"reworded"}}.Encode()))
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
req.Header.Set("Authorization", "Bearer "+agentTk)
req.Header.Set("Origin", "https://spec.example")
rec := httptest.NewRecorder()
h.ServeHTTP(rec, req)
if rec.Code != http.StatusSeeOther {
t.Fatalf("status = %d, want 303; body:\n%s", rec.Code, rec.Body)
}
if len(th.Replies) != 1 || th.Replies[0].Body != "reworded" {
t.Fatalf("replies = %+v, want the agent's answer", th.Replies)
}
if !th.Replies[0].Agent {
t.Errorf("the reply is not attributed to the agent: %+v", th.Replies[0])
}
}
// TestAgentMayNotResolve proves an agent cannot clear the gate its own proposal
// is held by.
func TestAgentMayNotResolve(t *testing.T) {
h, r := commentServer(t)
th := seedThread(t, r, 7, 1, "please reword this")
req := httptest.NewRequest(http.MethodPost, "/~bigbes/rfcs/p/7/resolve",
strings.NewReader(url.Values{"thread": {strconv.Itoa(th.Root.ID)}, "resolved": {"1"}}.Encode()))
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
req.Header.Set("Authorization", "Bearer "+agentTk)
req.Header.Set("Origin", "https://spec.example")
rec := httptest.NewRecorder()
h.ServeHTTP(rec, req)
if rec.Code != http.StatusForbidden {
t.Fatalf("status = %d, want 403 for an agent resolving", rec.Code)
}
if !th.Open() {
t.Errorf("the thread was resolved despite the refusal")
}
}
// TestOwnerResolvesAndReopens proves the resolve control is a toggle, and that
// the page then offers the other direction.
func TestOwnerResolvesAndReopens(t *testing.T) {
h, r := commentServer(t)
th := seedThread(t, r, 7, 1, "please reword this")
form := url.Values{"thread": {strconv.Itoa(th.Root.ID)}, "resolved": {"1"}}
if rec := postForm(t, h, "/~bigbes/rfcs/p/7/resolve", "bigbes", "https://spec.example", form); rec.Code != http.StatusSeeOther {
t.Fatalf("status = %d, want 303; body:\n%s", rec.Code, rec.Body)
}
if th.Open() {
t.Fatalf("the thread is still open after the owner resolved it")
}
body := get(t, h, "/~bigbes/rfcs/p/7", "bigbes").Body.String()
if !strings.Contains(body, "Reopen") {
t.Errorf("a resolved thread offers no way back; body:\n%s", body)
}
form.Set("resolved", "0")
if rec := postForm(t, h, "/~bigbes/rfcs/p/7/resolve", "bigbes", "https://spec.example", form); rec.Code != http.StatusSeeOther {
t.Fatalf("reopen status = %d, want 303", rec.Code)
}
if !th.Open() {
t.Errorf("the thread did not reopen")
}
}
// TestCommentRefusedCrossOrigin proves the comment writes carry the same CSRF
// defense as approve and reject.
func TestCommentRefusedCrossOrigin(t *testing.T) {
h, r := commentServer(t)
rec := postForm(t, h, "/~bigbes/rfcs/p/7/comment", "bigbes", "https://evil.example", url.Values{
"doc": {"specs/0007-storage.md"}, "block": {"1"}, "side": {"new"}, "body": {"forged"},
})
if rec.Code != http.StatusForbidden {
t.Fatalf("status = %d, want 403 for a cross-origin comment", rec.Code)
}
if len(r.threads[7]) != 0 {
t.Errorf("a forged comment was stored: %+v", r.threads[7])
}
}
// TestCommentThroughAnotherSpacesURLIs404 proves a comment POST is held to the
// same rule as the page it comes from: the proposal id is global, but the link
// names a space, and answering for the wrong one would let one space's URL write
// into another's review.
func TestCommentThroughAnotherSpacesURLIs404(t *testing.T) {
h, r := commentServer(t)
rec := postForm(t, h, "/~bigbes/other/p/7/comment", "bigbes", "https://spec.example", url.Values{
"doc": {"specs/0007-storage.md"}, "block": {"1"}, "side": {"new"}, "body": {"wrong space"},
})
if rec.Code != http.StatusNotFound {
t.Fatalf("status = %d, want 404 through another space's URL", rec.Code)
}
if len(r.threads[7]) != 0 {
t.Errorf("a comment was stored through the wrong space's URL: %+v", r.threads[7])
}
}
// TestAnonymousMayNotComment proves the comment writes are behind the read ACL
// as well as behind the service's owner rule: a viewer who may not read the
// proposal cannot write to its review either.
func TestAnonymousMayNotComment(t *testing.T) {
h, r := commentServer(t)
rec := postForm(t, h, "/~bigbes/rfcs/p/7/comment", "", "https://spec.example", url.Values{
"doc": {"specs/0007-storage.md"}, "block": {"1"}, "side": {"new"}, "body": {"uninvited"},
})
if rec.Code != http.StatusForbidden {
t.Fatalf("status = %d, want 403 for an anonymous comment", rec.Code)
}
if len(r.threads[7]) != 0 {
t.Errorf("an anonymous comment was stored: %+v", r.threads[7])
}
}
// TestReplyToAnotherProposalsThreadIs404 proves the thread id in a form is
// checked against the proposal in the URL: a global id must not let a reply land
// on a conversation the reviewer was never looking at.
func TestReplyToAnotherProposalsThreadIs404(t *testing.T) {
h, r := commentServer(t)
other := openProposal()
other.ID = 8
seedProposal(r, other, commentDocs())
th := seedThread(t, r, 8, 1, "on the other proposal")
rec := postForm(t, h, "/~bigbes/rfcs/p/7/reply", "bigbes", "https://spec.example",
url.Values{"thread": {strconv.Itoa(th.Root.ID)}, "body": {"misdirected"}})
if rec.Code != http.StatusNotFound {
t.Fatalf("status = %d, want 404 for a thread on another proposal", rec.Code)
}
if len(th.Replies) != 0 {
t.Errorf("the reply landed on the other proposal's thread: %+v", th.Replies)
}
}
M web/diff.go => web/diff.go +363 -47
@@ 1,11 1,17 @@
package web
import (
+ "bytes"
+ "crypto/sha256"
+ "encoding/hex"
"fmt"
"html/template"
+ "log"
"strings"
+ "sourcecraft.dev/bigbes/sr-ht-spec/core"
"sourcecraft.dev/bigbes/sr-ht-spec/prosediff"
+ "sourcecraft.dev/bigbes/sr-ht-spec/service"
)
// inlineSimilarityThreshold is the Phase 0 verdict's presentation switch: a
@@ 20,6 26,11 @@ import (
// itself; this is where the decision is made.
const inlineSimilarityThreshold = 0.75
+// contextPreviewRunes bounds the one-line preview a collapsed unchanged block
+// shows: long enough to recognise the paragraph, short enough that a screen of
+// them still reads as a list of blocks rather than as the document itself.
+const contextPreviewRunes = 90
+
// diffView is the whole rendered diff of one document, ready for the proposal
// template. Unchanged reports the degenerate case — a proposal that touches a
// document without changing it — so the page can say so rather than show an
@@ 28,21 39,59 @@ type diffView struct {
HTML template.HTML
Stats prosediff.Stats
Unchanged bool
+
+ // Unplaced are this document's threads that no rendered block claimed. The
+ // page shows them in its own area: a comment whose anchor is lost, or whose
+ // block this diff does not render, must still be visible somewhere.
+ Unplaced []service.Thread
+}
+
+// docDiff is one document's review: the two revisions to compare, the identity
+// its comments anchor to, the threads already resolved against this revision,
+// and who may act on them.
+type docDiff struct {
+ // DocID is the document's anchoring key — see docIDFor.
+ DocID string
+ // Path is the document's path on the proposal branch, which the compose form
+ // posts back so the anchor is rebuilt against the branch, not the form.
+ Path string
+ // Base is the approved content the proposal was made against; Proposed is
+ // what the branch says now.
+ Base, Proposed []byte
+ // Threads are this document's threads, already run through
+ // service.AnchorThreads: State and Block are meaningless before that.
+ Threads []service.Thread
+ Controls reviewControls
+}
+
+// blockKey identifies a block of one revision of one document: which side it is
+// on and its position in that side's segmentation. It is the only key a thread
+// is placed by — see docRenderer.
+type blockKey struct {
+ side core.CommentSide
+ ordinal int
}
// renderDocDiff diffs the approved (old) and proposed (new) source of one
-// document and renders it to HTML.
+// document and renders it to HTML, with every block's review threads and, for
+// the owner, the form that opens a new one.
//
// It walks prosediff's block-change model rather than its text renderer: the
// text renderer is for a terminal, and the review page needs headings grouped,
// word edits marked with <ins>/<del>, 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(`</div><div class="ph-blocks">`)
inHunk = true
}
- writeBlock(&b, c)
+ r.writeBlock(&b, c)
}
closeHunk()
b.WriteString(`</div>`)
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, `<div class="ph-block ph-move"><span class="ph-label">%s moved here (was line %d)</span></div>`,
- 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, `<div class="ph-block ph-move"><span class="ph-label">%s moved away (now line %d)</span></div>`,
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
-// <pre> 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, `<div class="ph-block %s"><span class="ph-label">%s</span>`,
- 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, `<div class="ph-block %s"%s><span class="ph-label">%s</span>`,
+ class, idAttr(anchor, commentable), template.HTMLEscapeString(label))
+ writeChangedBody(b, c)
+ if commentable {
+ r.writeComments(b, anchor, key, threads)
+ }
b.WriteString(`</div>`)
}
-// 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, `<details class="ph-block ph-context"%s%s><summary class="ph-context-label">%s</summary>`,
+ 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(`</details>`)
+}
+
+// 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, `<div class="ph-block ph-modify"><span class="ph-label">%s</span><pre class="ph-code">`,
- template.HTMLEscapeString(label))
+ b.WriteString(`<pre class="ph-code">`)
writeLineSpans(b, c.Lines)
- b.WriteString(`</pre></div>`)
+ b.WriteString(`</pre>`)
return
}
-
- if c.Similarity >= inlineSimilarityThreshold {
- fmt.Fprintf(b, `<div class="ph-block ph-modify"><span class="ph-label">%s</span><div class="ph-body ph-inline">`,
- template.HTMLEscapeString(label))
+ if !twoColumn(c) {
+ b.WriteString(`<div class="ph-body ph-inline">`)
writeInlineSpans(b, c.Words)
- b.WriteString(`</div></div>`)
+ b.WriteString(`</div>`)
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, `<div class="ph-block ph-modify ph-columns"><span class="ph-label">%s (rewritten)</span><div class="ph-cols"><div class="ph-col ph-old">`,
- template.HTMLEscapeString(label))
+ b.WriteString(`<div class="ph-cols"><div class="ph-col ph-old">`)
writeColumnSpans(b, c.Words, true)
b.WriteString(`</div><div class="ph-col ph-new">`)
writeColumnSpans(b, c.Words, false)
- b.WriteString(`</div></div></div>`)
+ b.WriteString(`</div></div>`)
+}
+
+// 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 <pre> when it is not prose.
M web/diff_internal_test.go => web/diff_internal_test.go +230 -5
@@ 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 <script>alert(1)</script> text here.\n")
- v := renderDocDiff(old, nw)
+ v := render(old, nw)
html := string(v.HTML)
if strings.Contains(html, "<script>") {
t.Fatalf("unescaped <script> reached the output; got:\n%s", html)
@@ 74,12 93,27 @@ func TestRenderDocDiffEscapesContent(t *testing.T) {
}
}
+// TestRenderDocDiffEscapesUnchangedContent proves the context blocks the review
+// now renders are escaped too — both the collapsed preview and the body.
+func TestRenderDocDiffEscapesUnchangedContent(t *testing.T) {
+ const untouched = "A <b>bold</b> claim nobody edited.\n"
+ old := []byte("# Title\n\n" + untouched + "\nfirst.\n")
+ nw := []byte("# Title\n\n" + untouched + "\nsecond.\n")
+ html := string(render(old, nw).HTML)
+ if strings.Contains(html, "<b>bold</b>") {
+ t.Fatalf("unescaped markup from an unchanged block reached the output; got:\n%s", html)
+ }
+ if want := strings.Count(html, "<b>bold</b>"); want != 2 {
+ t.Errorf("escaped markup appears %d times, want 2 (the preview and the body); got:\n%s", want, html)
+ }
+}
+
// TestRenderDocDiffInsertAndDelete proves an added and a removed block are each
// shown whole, labelled.
func TestRenderDocDiffInsertAndDelete(t *testing.T) {
old := []byte("# Title\n\nKept paragraph.\n\nDoomed paragraph.\n")
nw := []byte("# Title\n\nKept paragraph.\n\nBrand new paragraph.\n")
- v := renderDocDiff(old, nw)
+ v := render(old, nw)
html := string(v.HTML)
if !strings.Contains(html, "ph-insert") {
t.Errorf("missing an inserted block; got:\n%s", html)
@@ 88,3 122,194 @@ func TestRenderDocDiffInsertAndDelete(t *testing.T) {
t.Errorf("missing a deleted block; got:\n%s", html)
}
}
+
+// TestUnchangedBlocksRenderAsSubordinateContext proves the reversal this phase
+// makes: an unchanged block is on the page — every block of a proposed document
+// must be commentable — but as collapsed context, so a changed block still reads
+// as the thing that changed.
+func TestUnchangedBlocksRenderAsSubordinateContext(t *testing.T) {
+ old := []byte("# Title\n\nUntouched paragraph.\n\nOld wording here.\n")
+ nw := []byte("# Title\n\nUntouched paragraph.\n\nNew wording here.\n")
+ html := string(render(old, nw).HTML)
+
+ if !strings.Contains(html, "Untouched paragraph.") {
+ t.Fatalf("the unchanged block is not on the page; got:\n%s", html)
+ }
+ if !strings.Contains(html, `<details class="ph-block ph-context"`) {
+ t.Errorf("the unchanged block is not rendered as collapsed context; got:\n%s", html)
+ }
+ // The changed block must stay plainly changed: its own class, its inline
+ // marks, and no <details> wrapper hiding it behind a click.
+ if !strings.Contains(html, `<div class="ph-block ph-modify"`) {
+ t.Errorf("the changed block lost its own presentation; got:\n%s", html)
+ }
+ if !strings.Contains(html, "<ins>New</ins>") {
+ t.Errorf("the changed block lost its inline marks; got:\n%s", html)
+ }
+}
+
+// TestEveryRenderedBlockCarriesAnID proves a comment has something to point at
+// and a link has something to scroll to, on changed and unchanged blocks alike.
+func TestEveryRenderedBlockCarriesAnID(t *testing.T) {
+ old := []byte("# Title\n\nUntouched paragraph.\n\nOld wording here.\n")
+ nw := []byte("# Title\n\nUntouched paragraph.\n\nNew wording here.\n")
+ html := string(render(old, nw).HTML)
+
+ ids := blockIDs(html)
+ // heading, untouched paragraph, modified paragraph.
+ if len(ids) != 3 {
+ t.Fatalf("rendered %d block ids, want 3; got:\n%s", len(ids), html)
+ }
+ seen := map[string]bool{}
+ for _, id := range ids {
+ if seen[id] {
+ t.Errorf("duplicate block id %q; got:\n%s", id, html)
+ }
+ seen[id] = true
+ }
+}
+
+// TestBlockIDIsStableAcrossARerender proves the id is a function of the block,
+// not of anything the renderer accumulates as it goes.
+func TestBlockIDIsStableAcrossARerender(t *testing.T) {
+ old := []byte("# Title\n\nUntouched paragraph.\n\nOld wording here.\n")
+ nw := []byte("# Title\n\nUntouched paragraph.\n\nNew wording here.\n")
+
+ first := blockIDs(string(render(old, nw).HTML))
+ second := blockIDs(string(render(old, nw).HTML))
+ if len(first) == 0 || !equalStrings(first, second) {
+ t.Fatalf("ids changed across a re-render:\n%v\n%v", first, second)
+ }
+}
+
+// TestBlockIDSurvivesAnUnrelatedEdit proves an id names its own block: editing
+// a different section of the document leaves it alone, so a link written down
+// yesterday still lands on the paragraph it was written for.
+func TestBlockIDSurvivesAnUnrelatedEdit(t *testing.T) {
+ const base = "# Storage\n\nThe block under test.\n\n## Trade-offs\n\nSomething else entirely.\n"
+ const first = base + "\nA tail paragraph.\n"
+ const second = "# Storage\n\nThe block under test.\n\n## Trade-offs\n\nSomething else, rewritten.\n\nA tail paragraph.\n"
+
+ before := blockIDs(string(render([]byte(base), []byte(first)).HTML))
+ after := blockIDs(string(render([]byte(base), []byte(second)).HTML))
+ if len(before) < 2 || len(after) < 2 {
+ t.Fatalf("expected several blocks, got %d and %d", len(before), len(after))
+ }
+ // Index 1 is "The block under test." in both renders; the edit is two blocks
+ // further down, under a different heading.
+ if before[1] != after[1] {
+ t.Errorf("an edit elsewhere in the document changed a block's id: %q vs %q", before[1], after[1])
+ }
+}
+
+// TestThreadRendersOnItsOwnBlock proves an anchored comment is drawn against
+// the block it anchors to and nowhere else.
+func TestThreadRendersOnItsOwnBlock(t *testing.T) {
+ const src = "# Storage\n\nThe commented paragraph.\n\nAn innocent bystander.\n"
+ anchor, err := service.AnchorOf("SPEC-0007", []byte(src), 1, core.SideNew)
+ if err != nil {
+ t.Fatalf("AnchorOf: %v", err)
+ }
+ view := renderDocDiff(docDiff{
+ DocID: "SPEC-0007", Path: "specs/a.md",
+ Base: []byte("# Storage\n\nThe commented paragraph.\n"),
+ Proposed: []byte(src),
+ Threads: []service.Thread{{
+ Root: service.Comment{ID: 4, Author: "bigbes", Body: "this is wrong"},
+ Anchor: anchor, State: core.AnchorExact, Block: 1,
+ }},
+ })
+ html := string(view.HTML)
+ if len(view.Unplaced) != 0 {
+ t.Fatalf("an anchored thread came back unplaced: %+v", view.Unplaced)
+ }
+
+ // The comment must sit inside the block it anchors to: the bystander block
+ // opens after it, so the comment body has to appear before that boundary.
+ comment := strings.Index(html, "this is wrong")
+ bystander := strings.Index(html, "An innocent bystander")
+ if comment < 0 {
+ t.Fatalf("the comment is not on the page; got:\n%s", html)
+ }
+ if bystander > 0 && comment > bystander {
+ t.Errorf("the comment rendered after the following block, not on its own; got:\n%s", html)
+ }
+}
+
+// TestOutdatedThreadIsHandedBackNotAttached proves a comment whose anchor is
+// lost is never drawn against some other block. It comes back for the page to
+// show in its own area — visible, and attached to nothing.
+func TestOutdatedThreadIsHandedBackNotAttached(t *testing.T) {
+ view := renderDocDiff(docDiff{
+ DocID: "SPEC-0007", Path: "specs/a.md",
+ Base: []byte("# Storage\n\nOne paragraph.\n"),
+ Proposed: []byte("# Storage\n\nOne paragraph, edited.\n"),
+ Threads: []service.Thread{{
+ Root: service.Comment{ID: 9, Author: "bigbes", Body: "orphaned critique"},
+ State: core.AnchorOutdated, Block: -1,
+ }},
+ })
+ if len(view.Unplaced) != 1 || view.Unplaced[0].Root.ID != 9 {
+ t.Fatalf("Unplaced = %+v, want the outdated thread", view.Unplaced)
+ }
+ if strings.Contains(string(view.HTML), "orphaned critique") {
+ t.Errorf("an outdated comment was rendered against a block; got:\n%s", view.HTML)
+ }
+}
+
+// TestThreadOnAnUnrenderedBlockIsHandedBack proves the invariant the renderer
+// is built on: a thread comes out either on its block or in Unplaced, never
+// neither. An old-side anchor that still resolves against the base but whose
+// block the diff no longer renders — the agent restored the deleted paragraph —
+// is the case that would otherwise vanish.
+func TestThreadOnAnUnrenderedBlockIsHandedBack(t *testing.T) {
+ const src = "# Storage\n\nThe restored paragraph.\n"
+ anchor, err := service.AnchorOf("SPEC-0007", []byte(src), 1, core.SideOld)
+ if err != nil {
+ t.Fatalf("AnchorOf: %v", err)
+ }
+ view := renderDocDiff(docDiff{
+ DocID: "SPEC-0007", Path: "specs/a.md",
+ Base: []byte(src),
+ Proposed: []byte(src + "\nAnd a new one.\n"),
+ Threads: []service.Thread{{
+ Root: service.Comment{ID: 3, Body: "written when this was deleted"},
+ Anchor: anchor, State: core.AnchorExact, Block: 1,
+ }},
+ })
+ if len(view.Unplaced) != 1 {
+ t.Fatalf("Unplaced = %+v, want the thread whose block is not rendered", view.Unplaced)
+ }
+ if strings.Contains(string(view.HTML), "written when this was deleted") {
+ t.Errorf("the thread was rendered against some other block; got:\n%s", view.HTML)
+ }
+}
+
+// TestUnchangedDocumentKeepsItsThreads proves nothing is lost in the degenerate
+// case: a document whose diff renders no blocks at all still hands its threads
+// back rather than swallowing them.
+func TestUnchangedDocumentKeepsItsThreads(t *testing.T) {
+ src := []byte("# Storage\n\nOne paragraph.\n")
+ view := renderDocDiff(docDiff{
+ DocID: "SPEC-0007", Path: "specs/a.md", Base: src, Proposed: src,
+ Threads: []service.Thread{{Root: service.Comment{ID: 1, Body: "still here"}, State: core.AnchorExact, Block: 1}},
+ })
+ if !view.Unchanged {
+ t.Fatalf("Unchanged = false for identical input")
+ }
+ if len(view.Unplaced) != 1 {
+ t.Errorf("Unplaced = %+v, want the thread of an unrendered document", view.Unplaced)
+ }
+}
+
+func equalStrings(a, b []string) bool {
+ if len(a) != len(b) {
+ return false
+ }
+ for i := range a {
+ if a[i] != b[i] {
+ return false
+ }
+ }
+ return true
+}
M web/proposal.go => web/proposal.go +59 -6
@@ 31,6 31,16 @@ type proposalData struct {
// Approved / Rejected phrase the outcome for a terminal proposal.
Merged bool
Rejected bool
+
+ // Lost are the review threads no block on this page claimed: an anchor that
+ // did not resolve, or one on a document this proposal no longer changes. They
+ // are rendered in their own area rather than dropped — a comment that
+ // silently vanished would look like one that was never made — and never
+ // against a block that is merely nearby.
+ Lost []threadPanel
+ // Unresolved is how many threads still await the owner. An open thread
+ // suppresses policy auto-merge, so the count is why a proposal is still here.
+ Unresolved int
}
// proposalDocDiff is one document's rendered diff on the review page.
@@ 92,18 102,59 @@ func (s *Server) handleProposal(w http.ResponseWriter, r *http.Request) {
s.fail(w, r, err)
return
}
+
+ principal := authn.PrincipalFromContext(r.Context())
+ threads, err := s.reader.Threads(r.Context(), principal, id)
+ if err != nil {
+ s.fail(w, r, err)
+ return
+ }
+ // Anchor fit is a property of the revision on screen, so it is resolved
+ // against the documents this page is about to render and nowhere else. Doing
+ // it here rather than in each renderer also means the state a thread reports
+ // and the blocks the diff draws describe the same bytes.
+ threads = service.AnchorThreads(threads, docs)
+ byDoc := make(map[string][]service.Thread, len(docs))
+ for _, t := range threads {
+ byDoc[t.DocPath] = append(byDoc[t.DocPath], t)
+ }
+
+ // The controls follow the service's two authorities: opening and resolving
+ // are the owner's, replying is any reader's. Both are offered only while the
+ // proposal is open — a merged or rejected proposal's conversation is history,
+ // and there is no auto-merge left for a thread to gate.
+ open := p.State == core.StateOpen
+ owner := principal.IsOwner()
+ controls := reviewControls{
+ Owner: owner && open,
+ Reply: open,
+ ActionBase: proposalHref(p),
+ }
+
views := make([]proposalDocDiff, 0, len(docs))
+ var lost []service.Thread
for _, d := range docs {
// A new document diffs against nothing, which renders as an all-inserted
// block set — the same renderer, so the page has one code path.
- views = append(views, proposalDocDiff{
- Path: d.Path,
- New: d.New,
- Diff: renderDocDiff(d.Base, d.Proposed),
+ view := renderDocDiff(docDiff{
+ DocID: docIDFor(d.Path, d.Proposed),
+ Path: d.Path,
+ Base: d.Base,
+ Proposed: d.Proposed,
+ Threads: byDoc[d.Path],
+ Controls: controls,
})
+ lost = append(lost, view.Unplaced...)
+ delete(byDoc, d.Path)
+ views = append(views, proposalDocDiff{Path: d.Path, New: d.New, Diff: view})
+ }
+ // Whatever is left belongs to a document this proposal no longer changes —
+ // the agent reverted it — so no renderer ever saw those threads. They are as
+ // lost as an unresolved anchor, and just as visible.
+ for _, ts := range byDoc {
+ lost = append(lost, ts...)
}
- owner := authn.PrincipalFromContext(r.Context()).IsOwner()
vd := s.chrome(r)
vd.Title = fmt.Sprintf("Proposal #%d — %s", p.ID, p.Title)
vd.Data = proposalData{
@@ 111,9 162,11 @@ func (s *Server) handleProposal(w http.ResponseWriter, r *http.Request) {
SpaceHref: "/" + ref.String(),
StateBadge: stateBadge(p.State),
Docs: views,
- CanApprove: owner && p.State == core.StateOpen,
+ CanApprove: owner && open,
Merged: p.State == core.StateMerged,
Rejected: p.State == core.StateRejected,
+ Lost: lostPanels(lost, controls),
+ Unresolved: unresolvedThreads(threads),
}
s.render(w, http.StatusOK, "proposal", vd)
}
M web/reader.go => web/reader.go +30 -0
@@ 4,6 4,7 @@ import (
"context"
"time"
+ "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/search"
@@ 69,6 70,19 @@ type Reader interface {
Approve(ctx context.Context, ref core.SpaceRef, id int) (service.Proposal, error)
Reject(ctx context.Context, ref core.SpaceRef, id int) (service.Proposal, error)
+ // Threads returns a proposal's review conversations with their replies, and
+ // the three comment writes the review page makes.
+ //
+ // These carry the principal explicitly rather than reading it from the
+ // context the way the read plane does, because the authority they turn on is
+ // not the read ACL: only the owner may open or resolve a thread, while owner
+ // and agent alike may reply. service/ decides that and answers ErrForbidden;
+ // this package passes the principal through and surfaces the refusal.
+ Threads(ctx context.Context, p authn.Principal, proposalID int) ([]service.Thread, error)
+ CommentOn(ctx context.Context, req service.CommentRequest) (service.Thread, error)
+ ReplyTo(ctx context.Context, p authn.Principal, threadID int, body string) (service.Comment, error)
+ ResolveThread(ctx context.Context, p authn.Principal, threadID int, resolved bool) error
+
// Inbox is every open proposal on the instance, newest first — the review
// queue. Digest is the recently policy-merged proposals, the firehose a human
// sees after the fact.
@@ 151,6 165,22 @@ func (r serviceReader) Reject(ctx context.Context, ref core.SpaceRef, id int) (s
return r.svc.Reject(ctx, ref, id)
}
+func (r serviceReader) Threads(ctx context.Context, p authn.Principal, proposalID int) ([]service.Thread, error) {
+ return r.svc.Threads(ctx, p, proposalID)
+}
+
+func (r serviceReader) CommentOn(ctx context.Context, req service.CommentRequest) (service.Thread, error) {
+ return r.svc.CommentOn(ctx, req)
+}
+
+func (r serviceReader) ReplyTo(ctx context.Context, p authn.Principal, threadID int, body string) (service.Comment, error) {
+ return r.svc.ReplyTo(ctx, p, threadID, body)
+}
+
+func (r serviceReader) ResolveThread(ctx context.Context, p authn.Principal, threadID int, resolved bool) error {
+ return r.svc.ResolveThread(ctx, p, threadID, resolved)
+}
+
func (r serviceReader) Inbox(ctx context.Context) ([]service.Proposal, error) {
return r.svc.InboxProposals(ctx)
}
M web/router.go => web/router.go +3 -0
@@ 50,6 50,9 @@ func (s *Server) Register(r chi.Router) {
r.Get("/~{owner}/{space}/p/{id}", s.handleProposal)
r.Post("/~{owner}/{space}/p/{id}/approve", s.handleProposalApprove)
r.Post("/~{owner}/{space}/p/{id}/reject", s.handleProposalReject)
+ r.Post("/~{owner}/{space}/p/{id}/comment", s.handleProposalComment)
+ r.Post("/~{owner}/{space}/p/{id}/reply", s.handleProposalReply)
+ r.Post("/~{owner}/{space}/p/{id}/resolve", s.handleProposalResolve)
r.Get("/~{owner}/{space}", s.handleSpace)
r.Get("/~{owner}/{space}/*", s.handleDocument)
M web/templates.go => web/templates.go +23 -2
@@ 48,17 48,38 @@ var funcMap = template.FuncMap{
// pageNames are the content templates; each is parsed with layout.html.
var pageNames = []string{"index", "space", "document", "search", "error", "proposal", "inbox"}
-// pages maps a page name to its parsed template set (layout + that page).
+// pages maps a page name to its parsed template set (layout + partials + that
+// page). threads.html is parsed into every set rather than only into the
+// proposal page's: it defines review-thread markup and nothing else, and a
+// partial that only some sets know about is a lookup that fails on the page
+// that later needs it.
var pages = func() map[string]*template.Template {
m := make(map[string]*template.Template, len(pageNames))
for _, name := range pageNames {
t := template.New("layout.html").Funcs(funcMap)
- t = template.Must(t.ParseFS(tmplFS, "templates/layout.html", "templates/"+name+".html"))
+ t = template.Must(t.ParseFS(tmplFS,
+ "templates/layout.html", "templates/threads.html", "templates/"+name+".html"))
m[name] = t
}
return m
}()
+// blockThreadsTmpl is the per-block comment markup, taken out of the proposal
+// page's own set so the diff renderer — which builds its HTML in Go and cannot
+// reach a page template through the usual {{template}} call — and the page
+// itself cannot drift into two spellings of a thread.
+//
+// A missing define is a build-time mistake in this package, so it panics at
+// init the way template.Must does, rather than yielding a page with the
+// comments silently absent.
+var blockThreadsTmpl = func() *template.Template {
+ t := pages["proposal"].Lookup("blockthreads")
+ if t == nil {
+ panic(`web: templates/threads.html does not define "blockthreads"`)
+ }
+ return t
+}()
+
// render executes a page into a buffer first, so a template error yields a
// clean 500 rather than a half-written response. On success it writes the
// status and the buffered HTML.
M web/templates/proposal.html => web/templates/proposal.html +32 -0
@@ 23,6 23,16 @@
</small>
</p>
+ {{if .Data.Unresolved}}
+ <p class="text-muted">
+ <small>
+ <span class="badge badge-warning">{{.Data.Unresolved}} unresolved</span>
+ Policy auto-merge is suppressed while a review thread is open; approving
+ by hand still works.
+ </small>
+ </p>
+ {{end}}
+
{{if .Data.CanApprove}}
<div class="proposal-actions">
<form method="POST" action="/{{$p.Space}}/p/{{$p.ID}}/approve" class="d-inline">
@@ 59,6 69,28 @@
{{else}}
<p class="text-muted">This proposal changes no documents.</p>
{{end}}
+
+ {{/*
+ Comments whose block is gone. They are shown here, whole, rather than
+ moved onto whatever block now sits nearest: a critique read against the
+ wrong paragraph is worse than one that admits it lost its place, because
+ the reader cannot tell it is wrong.
+ */}}
+ {{if .Data.Lost}}
+ <div class="proposal-doc ph-lost">
+ <h4 class="h6">
+ Comments that lost their anchor
+ <span class="badge badge-warning">{{len .Data.Lost}}</span>
+ </h4>
+ <p class="text-muted">
+ <small>
+ The blocks these were written about are no longer in the proposed
+ revision. They are kept as written and attached to nothing.
+ </small>
+ </p>
+ {{range .Data.Lost}}{{template "thread" .}}{{end}}
+ </div>
+ {{end}}
</div>
</div>
{{end}}
A web/templates/threads.html => web/templates/threads.html +79 -0
@@ 0,0 1,79 @@
+{{/*
+ The review-thread markup, defined once and parsed into every page set.
+
+ The diff renderer (web/diff.go) executes "blockthreads" into the HTML it
+ builds for one block, and the proposal page renders "thread" directly for the
+ comments whose anchor no longer names a block. Both go through html/template
+ rather than string building because a comment body is prose someone else
+ wrote: contextual auto-escaping is what keeps it text.
+*/}}
+
+{{define "thread"}}
+<article class="ph-thread{{if .Resolved}} ph-thread-resolved{{end}}" id="thread-{{.ID}}">
+ <header class="ph-thread-head">
+ <span class="ph-author">{{.Author}}</span>
+ {{if .Agent}}<span class="badge badge-secondary">agent</span>{{end}}
+ <span class="ph-when">{{.When}}</span>
+ {{if .Resolved}}<span class="badge badge-success">resolved</span>{{end}}
+ {{if .Note}}<span class="badge badge-warning" title="{{.NoteWhy}}">{{.Note}}</span>{{end}}
+ {{if .DocPath}}<code class="ph-when">{{.DocPath}}</code>{{end}}
+ </header>
+ <div class="ph-comment-body">{{.Body}}</div>
+
+ {{range .Replies}}
+ <div class="ph-reply">
+ <header class="ph-thread-head">
+ <span class="ph-author">{{.Author}}</span>
+ {{if .Agent}}<span class="badge badge-secondary">agent</span>{{end}}
+ <span class="ph-when">{{.When}}</span>
+ </header>
+ <div class="ph-comment-body">{{.Body}}</div>
+ </div>
+ {{end}}
+
+ <div class="ph-thread-actions">
+ {{if .CanReply}}
+ <details class="ph-form">
+ <summary>Reply</summary>
+ <form method="POST" action="{{.ActionBase}}/reply">
+ <input type="hidden" name="thread" value="{{.ID}}">
+ <textarea class="form-control" name="body" rows="3" required
+ placeholder="Answer the critique"></textarea>
+ <button type="submit" class="btn btn-sm btn-primary">Reply</button>
+ </form>
+ </details>
+ {{end}}
+ {{if .CanResolve}}
+ <form method="POST" action="{{.ActionBase}}/resolve" class="ph-form">
+ <input type="hidden" name="thread" value="{{.ID}}">
+ <input type="hidden" name="resolved" value="{{if .Resolved}}0{{else}}1{{end}}">
+ <button type="submit" class="btn btn-sm btn-outline-secondary">
+ {{if .Resolved}}Reopen{{else}}Resolve{{end}}
+ </button>
+ </form>
+ {{end}}
+ </div>
+</article>
+{{end}}
+
+{{define "blockthreads"}}
+{{if .Threads}}
+<div class="ph-threads">
+ {{range .Threads}}{{template "thread" .}}{{end}}
+</div>
+{{end}}
+{{if .Compose}}
+<details class="ph-form ph-compose">
+ <summary>Comment on this block</summary>
+ <form method="POST" action="{{.Compose.ActionBase}}/comment">
+ <input type="hidden" name="doc" value="{{.Compose.DocPath}}">
+ <input type="hidden" name="block" value="{{.Compose.Ordinal}}">
+ <input type="hidden" name="side" value="{{.Compose.Side}}">
+ <input type="hidden" name="hash" value="{{.Compose.Hash}}">
+ <textarea class="form-control" name="body" rows="3" required
+ placeholder="What is wrong with this block?"></textarea>
+ <button type="submit" class="btn btn-sm btn-primary">Comment</button>
+ </form>
+</details>
+{{end}}
+{{end}}
M web/web_test.go => web/web_test.go +91 -0
@@ 122,6 122,14 @@ type fakeReader struct {
// timestamp and whether one has been set. MarkDigestSeen moves them.
mark time.Time
marked bool
+
+ // threads back the Phase 5b comment tests: review threads by proposal id,
+ // with ids handed out by nextThread. The authority rules below are service/'s
+ // and are restated here only so the fake refuses what the real service
+ // refuses — a page that draws a control the service would reject would pass
+ // against a fake that accepts everything.
+ threads map[int][]*service.Thread
+ nextThread int
}
func newFakeReader() *fakeReader {
@@ 130,6 138,7 @@ func newFakeReader() *fakeReader {
head: headRev,
proposals: map[int]service.Proposal{},
diffs: map[int][]service.ProposalDoc{},
+ threads: map[int][]*service.Thread{},
}
}
@@ 257,6 266,88 @@ func (f *fakeReader) Reject(_ context.Context, _ core.SpaceRef, id int) (service
return p, nil
}
+func (f *fakeReader) Threads(_ context.Context, p authn.Principal, proposalID int) ([]service.Thread, error) {
+ if !p.CanRead() {
+ return nil, fmt.Errorf("%w: %s may not read review threads", service.ErrForbidden, p)
+ }
+ out := make([]service.Thread, 0, len(f.threads[proposalID]))
+ for _, t := range f.threads[proposalID] {
+ out = append(out, *t)
+ }
+ return out, nil
+}
+
+func (f *fakeReader) CommentOn(_ context.Context, req service.CommentRequest) (service.Thread, error) {
+ if !req.Principal.IsOwner() {
+ return service.Thread{}, fmt.Errorf("%w: %s may not open a review thread", service.ErrForbidden, req.Principal)
+ }
+ f.nextThread++
+ t := &service.Thread{
+ Root: service.Comment{
+ ID: f.nextThread,
+ Body: req.Body,
+ Author: req.Principal.Owner,
+ Created: time.Now(),
+ },
+ DocPath: req.DocPath,
+ Anchor: req.Anchor,
+ Block: -1,
+ }
+ f.threads[req.ProposalID] = append(f.threads[req.ProposalID], t)
+ return *t, nil
+}
+
+func (f *fakeReader) ReplyTo(_ context.Context, p authn.Principal, threadID int, body string) (service.Comment, error) {
+ if !p.CanRead() {
+ return service.Comment{}, fmt.Errorf("%w: %s may not comment", service.ErrForbidden, p)
+ }
+ t := f.thread(threadID)
+ if t == nil {
+ return service.Comment{}, fmt.Errorf("%w: no review thread %d", service.ErrNotFound, threadID)
+ }
+ f.nextThread++
+ author, agent := p.Owner, false
+ if p.IsAgent() {
+ author, agent = p.Agent, true
+ }
+ reply := service.Comment{
+ ID: f.nextThread, ParentID: threadID, Body: body,
+ Author: author, Agent: agent, Created: time.Now(),
+ }
+ t.Replies = append(t.Replies, reply)
+ return reply, nil
+}
+
+func (f *fakeReader) ResolveThread(_ context.Context, p authn.Principal, threadID int, resolved bool) error {
+ if !p.IsOwner() {
+ return fmt.Errorf("%w: %s may not resolve a review thread", service.ErrForbidden, p)
+ }
+ t := f.thread(threadID)
+ if t == nil {
+ return fmt.Errorf("%w: no review thread %d", service.ErrNotFound, threadID)
+ }
+ if !resolved {
+ t.Resolved = nil
+ return nil
+ }
+ now := time.Now()
+ t.Resolved = &now
+ return nil
+}
+
+// thread finds a root thread by id across every proposal, the way a global
+// thread id resolves in the real store.
+func (f *fakeReader) thread(id int) *service.Thread {
+ for _, ts := range f.threads {
+ for _, t := range ts {
+ if t.Root.ID == id {
+ return t
+ }
+ }
+ }
+ return nil
+}
+
func (f *fakeReader) Inbox(_ context.Context) ([]service.Proposal, error) {
var out []service.Proposal
for _, p := range f.proposals {