M .beads/interactions.jsonl => .beads/interactions.jsonl +1 -0
@@ 8,3 8,4 @@
{"id":"int-efe1054b315e03fdc31f2535d4ccd1d1","kind":"field_change","created_at":"2026-07-24T16:54:06.613186Z","actor":"Eugene Blikh","issue_id":"spec-mfm","extra":{"field":"status","new_value":"closed","old_value":"in_progress","reason":"Inbox GET reads digest_mark to flag/count new-since-last-looked auto-merges + divider (pure read); explicit POST /inbox/seen advances the mark behind owner-only + same-origin guard. service.DigestMark/MarkDigestSeen added. Commit 865a21f."}}
{"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."}}
M .beads/issues.jsonl => .beads/issues.jsonl +2 -2
@@ 1,8 1,8 @@
{"_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":"open","priority":2,"issue_type":"task","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:57:05Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T18:57:05Z","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":"in_progress","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-24T18:57:37Z","started_at":"2026-07-24T18:57:37Z","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.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.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}
A => +331 -0
@@ 0,0 1,331 @@
package service
import (
"context"
"errors"
"fmt"
"time"
"sourcecraft.dev/bigbes/sr-ht-spec/authn"
"sourcecraft.dev/bigbes/sr-ht-spec/core"
"sourcecraft.dev/bigbes/sr-ht-spec/db"
"sourcecraft.dev/bigbes/sr-ht-spec/prosediff"
)
// Comment is one message in a review thread, as the surfaces above this layer
// need it. Agent reports the authoring agent's identity and is empty for the
// owner, which is the only distinction a reader needs — who said it.
type Comment struct {
ID int
ParentID int
Body string
Author string
Agent bool
Created time.Time
}
// Thread is one review conversation: a comment anchored to a block of a
// document, plus its replies in order.
//
// State and Block are filled in by [AnchorThreads] against a particular
// revision and are meaningless before it runs — State is the zero AnchorState
// and Block is 0, not -1. That is deliberate: anchor fit is a property of a
// revision, and a Thread that has not been resolved against one has no honest
// answer to give.
type Thread struct {
Root Comment
DocPath string
Anchor core.CommentAnchor
Replies []Comment
// Resolved is when the owner closed the thread, or nil while it is open.
// An open thread suppresses policy auto-merge; see [Service.autoMerges].
Resolved *time.Time
// State is how well the anchor still fits the revision it was resolved
// against.
State core.AnchorState
// Block is the index into that revision's blocks, or -1 when the anchor did
// not resolve.
Block int
}
// Open reports whether the thread still awaits the owner.
func (t Thread) Open() bool { return t.Resolved == nil }
// CommentRequest opens a review thread on one block of one document.
type CommentRequest struct {
Principal authn.Principal
Space core.SpaceRef
ProposalID int
// DocPath is the document's path on the proposal branch.
DocPath string
// Anchor names the block. Its DocID is the archive's addressing key for the
// document, which is what makes the thread survive a later rename.
Anchor core.CommentAnchor
Body string
}
// CommentOn opens a review thread anchored to a block of a proposed document.
//
// Owner-only. An agent may reply to a thread but may not start one: the review
// conversation exists so a human can direct an agent, and an agent opening
// threads on its own proposal would put unresolved threads — which suppress
// policy auto-merge — under the control of the thing the gate exists to hold
// back.
func (s *Service) CommentOn(ctx context.Context, req CommentRequest) (Thread, error) {
if !req.Principal.IsOwner() {
return Thread{}, fmt.Errorf("%w: %s may not open a review thread; that is the owner's", ErrForbidden, req.Principal)
}
if req.DocPath == "" {
return Thread{}, fmt.Errorf("%w: a comment must name the document it is on", ErrInvalid)
}
if req.Anchor.DocID == "" {
return Thread{}, fmt.Errorf("%w: a comment must carry the document id it anchors to", ErrInvalid)
}
if req.Body == "" {
return Thread{}, fmt.Errorf("%w: a comment needs a body", ErrInvalid)
}
if req.Anchor.Side == "" {
req.Anchor.Side = core.SideNew
}
// The proposal is read through the normal path so a comment on a missing or
// foreign proposal fails here rather than as a foreign-key violation.
if _, err := s.GetProposal(ctx, req.ProposalID); err != nil {
return Thread{}, err
}
row, err := s.store.AddComment(ctx, &db.Comment{
ProposalID: req.ProposalID,
Anchor: &req.Anchor,
DocPath: req.DocPath,
Body: req.Body,
Author: req.Principal.Owner,
Kind: db.AuthorHuman,
})
if err != nil {
return Thread{}, fmt.Errorf("service: comment on proposal %d: %w", req.ProposalID, err)
}
return threadView(row, nil), nil
}
// ReplyTo appends a reply to an existing thread, returning the stored reply.
//
// Both principals may reply: this is the loop's turn-taking — the owner
// critiques, the agent answers and revises. A reply never resolves the thread,
// so an agent answering a critique does not clear the auto-merge gate; only the
// owner accepting the answer does.
func (s *Service) ReplyTo(ctx context.Context, p authn.Principal, threadID int, body string) (Comment, error) {
if !p.CanRead() {
return Comment{}, fmt.Errorf("%w: %s may not comment", ErrForbidden, p)
}
if body == "" {
return Comment{}, fmt.Errorf("%w: a reply needs a body", ErrInvalid)
}
reply := &db.Comment{Body: body}
if p.IsAgent() {
if p.Agent == "" || p.Session == "" {
return Comment{}, fmt.Errorf("%w: an agent reply must carry its identity and session", ErrInvalid)
}
reply.Author, reply.Kind, reply.Session = p.Agent, db.AuthorAgent, p.Session
} else {
reply.Author, reply.Kind = p.Owner, db.AuthorHuman
}
row, err := s.store.ReplyComment(ctx, threadID, reply)
if errors.Is(err, db.ErrNotFound) {
return Comment{}, fmt.Errorf("%w: no review thread %d", ErrNotFound, threadID)
}
if err != nil {
return Comment{}, fmt.Errorf("service: reply to thread %d: %w", threadID, err)
}
return commentView(row), nil
}
// ResolveThread closes a review thread, or reopens it when resolved is false.
//
// Owner-only, and this is the rule the auto-merge gate rests on: an agent that
// could resolve the thread opened against its own proposal could clear the gate
// holding that proposal back, which is the one thing the gate is for.
func (s *Service) ResolveThread(ctx context.Context, p authn.Principal, threadID int, resolved bool) error {
if !p.IsOwner() {
return fmt.Errorf("%w: %s may not resolve a review thread; only the owner may", ErrForbidden, p)
}
err := s.store.ResolveComment(ctx, threadID, resolved)
if errors.Is(err, db.ErrNotFound) {
return fmt.Errorf("%w: no review thread %d", ErrNotFound, threadID)
}
if err != nil {
return fmt.Errorf("service: resolve thread %d: %w", threadID, err)
}
return nil
}
// Threads returns a proposal's review conversations, each with its replies in
// order, oldest thread first.
//
// The anchors are not resolved here: fit depends on which revision the caller
// is looking at, so it is [AnchorThreads] that answers it, against the documents
// the caller already read.
func (s *Service) Threads(ctx context.Context, p authn.Principal, proposalID int) ([]Thread, error) {
if !p.CanRead() {
return nil, fmt.Errorf("%w: %s may not read review threads", ErrForbidden, p)
}
rows, err := s.store.ListComments(ctx, proposalID)
if err != nil {
return nil, fmt.Errorf("service: threads of proposal %d: %w", proposalID, err)
}
replies := make(map[int][]Comment)
for _, r := range rows {
if !r.Root() {
replies[r.ParentID] = append(replies[r.ParentID], commentView(r))
}
}
var out []Thread
for _, r := range rows {
if r.Root() {
out = append(out, threadView(r, replies[r.ID]))
}
}
return out, nil
}
// AnchorThreads resolves every thread's anchor against a revision's documents,
// filling in State and Block.
//
// It lives here rather than in each surface for the reason [Service.Archive]
// does: the review page and the MCP tool must agree about whether a comment
// still fits, and two surfaces each segmenting and matching would agree only
// until one of them was changed. Each document is segmented at most once per
// side however many threads hang off it.
//
// A thread whose document is not in docs is outdated, not dropped. That happens
// when the agent's revision reverted the document to its base — it is no longer
// a changed document, so the review page never renders it — and a comment that
// silently vanished would look like one that was never made.
func AnchorThreads(threads []Thread, docs []ProposalDoc) []Thread {
byPath := make(map[string]ProposalDoc, len(docs))
for _, d := range docs {
byPath[d.Path] = d
}
type key struct {
path string
side core.CommentSide
}
segmented := make(map[key][]core.AnchorBlock)
out := make([]Thread, len(threads))
for i, t := range threads {
out[i] = t
out[i].Block, out[i].State = -1, core.AnchorOutdated
doc, ok := byPath[t.DocPath]
if !ok {
continue
}
k := key{t.DocPath, t.Anchor.Side}
blocks, done := segmented[k]
if !done {
src := doc.Proposed
if t.Anchor.Side == core.SideOld {
src = doc.Base
}
blocks = anchorBlocksOf(src)
segmented[k] = blocks
}
out[i].Block, out[i].State = core.ResolveAnchor(t.Anchor, blocks)
}
return out
}
// anchorBlocksOf segments a document and reduces it to what anchoring reads.
// A nil source — the base of a document the proposal adds — has no blocks, so
// every anchor against it is outdated, which is the honest answer.
func anchorBlocksOf(src []byte) []core.AnchorBlock {
if len(src) == 0 {
return nil
}
segs := prosediff.Segment(src)
hashes := make([]string, len(segs))
paths := make([][]string, len(segs))
for i, b := range segs {
hashes[i], paths[i] = b.Hash, b.HeadingPath
}
return core.AnchorBlocks(hashes, paths)
}
// AnchorOf builds the anchor for a block of a document, numbering it the way
// [core.AnchorBlocks] does.
//
// 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.
func AnchorOf(docID string, src []byte, ordinal int, side core.CommentSide) (core.CommentAnchor, error) {
segs := prosediff.Segment(src)
if ordinal < 0 || ordinal >= len(segs) {
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++
}
}
return core.CommentAnchor{
DocID: docID,
HeadingPath: blk.HeadingPath,
Index: index,
BlockHash: blk.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{
ID: c.ID,
ParentID: c.ParentID,
Body: c.Body,
Author: c.Author,
Agent: c.Kind == db.AuthorAgent,
Created: c.Created,
}
}
// threadView maps a stored root plus its replies onto the surface shape. Block
// is -1 until AnchorThreads runs: an unresolved anchor points at no block, and
// zero would point at the first one.
func threadView(root *db.Comment, replies []Comment) Thread {
t := Thread{
Root: commentView(root),
DocPath: root.DocPath,
Replies: replies,
Resolved: root.Resolved,
Block: -1,
}
if root.Anchor != nil {
t.Anchor = *root.Anchor
}
return t
}
A => +339 -0
@@ 0,0 1,339 @@
package service
import (
"context"
"errors"
"testing"
"sourcecraft.dev/bigbes/sr-ht-spec/core"
)
// autoMergeSetup opens a proposal that does not auto-merge yet — there is no
// policy at the time it is opened — and then installs one that covers it. The
// agent's next revision is therefore the first moment auto-merge can fire,
// which is exactly the window an open review thread has to hold shut.
func autoMergeSetup(t *testing.T) (*Service, context.Context, *Space, ProposeResult, string) {
t.Helper()
svc, _ := newTestService(t)
ctx := context.Background()
sp, err := svc.CreateSpace(ctx, fxSpace)
if err != nil {
t.Fatalf("CreateSpace: %v", err)
}
base, err := sp.Repo.ApprovedHead(ctx)
if err != nil {
t.Fatalf("ApprovedHead: %v", err)
}
res, err := svc.Propose(ctx, ProposeRequest{
Space: fxSpace,
Principal: agentPrincipal(),
Title: "firehose note",
IfMatch: base.String(),
Message: "add notes/a.md",
Writes: []DocumentWrite{{Path: "notes/a.md", Content: mdDoc("N-1", "A", "first body")}},
})
if err != nil {
t.Fatalf("Propose: %v", err)
}
if res.Merged {
t.Fatalf("proposal auto-merged with no policy installed")
}
// Widen the policy so the proposal's paths now qualify.
commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{
".spec.yml": []byte("review:\n auto_merge: [notes/**]\n"),
})
return svc, ctx, sp, res, base.String()
}
// revise is the agent's next push onto an open proposal, which is where
// auto-merge is re-evaluated.
func revise(t *testing.T, svc *Service, ctx context.Context, res ProposeResult, base, body string) ProposeResult {
t.Helper()
out, err := svc.Propose(ctx, ProposeRequest{
Space: fxSpace,
Principal: agentPrincipal(),
ProposalID: res.Proposal.ID,
IfMatch: base,
Message: "revise notes/a.md",
Writes: []DocumentWrite{{Path: "notes/a.md", Content: mdDoc("N-1", "A", body)}},
})
if err != nil {
t.Fatalf("revise: %v", err)
}
return out
}
func commentOn(t *testing.T, svc *Service, ctx context.Context, proposalID int) Thread {
t.Helper()
th, err := svc.CommentOn(ctx, CommentRequest{
Principal: ownerPrincipal(),
Space: fxSpace,
ProposalID: proposalID,
DocPath: "notes/a.md",
Anchor: core.CommentAnchor{
DocID: "N-1", HeadingPath: nil, Index: 0, BlockHash: "whatever", Side: core.SideNew,
},
Body: "This contradicts SPEC-0003.",
})
if err != nil {
t.Fatalf("CommentOn: %v", err)
}
return th
}
// The gate. Without a comment the agent's revision lands under policy; with an
// open thread it must not, because the owner has engaged with this proposal and
// it must not slip past them unattended on the agent's next push.
func TestOpenThreadSuppressesPolicyAutoMerge(t *testing.T) {
svc, ctx, _, res, base := autoMergeSetup(t)
commentOn(t, svc, ctx, res.Proposal.ID)
revised := revise(t, svc, ctx, res, base, "second body")
if revised.Merged {
t.Fatal("policy auto-merged a proposal with an open review thread")
}
if revised.Proposal.State != core.StateOpen {
t.Errorf("state = %s, want open", revised.Proposal.State)
}
}
// The control for the test above: the same revision, with nothing to hold it
// back, does land. Without this, a broken auto-merge would make the gate test
// pass for the wrong reason.
func TestPolicyAutoMergeStillFiresWithoutAThread(t *testing.T) {
svc, ctx, _, res, base := autoMergeSetup(t)
revised := revise(t, svc, ctx, res, base, "second body")
if !revised.Merged {
t.Fatal("policy did not auto-merge an uncommented proposal; the gate test above proves nothing")
}
if revised.Proposal.Approval != core.ApprovalPolicy {
t.Errorf("approval = %q, want policy", revised.Proposal.Approval)
}
}
// Resolving the thread lifts the gate: the owner has had their say, so the
// proposal returns to the firehose rather than needing a manual click.
func TestResolvingTheThreadRestoresAutoMerge(t *testing.T) {
svc, ctx, _, res, base := autoMergeSetup(t)
th := commentOn(t, svc, ctx, res.Proposal.ID)
if err := svc.ResolveThread(ctx, ownerPrincipal(), th.Root.ID, true); err != nil {
t.Fatalf("ResolveThread: %v", err)
}
revised := revise(t, svc, ctx, res, base, "second body")
if !revised.Merged {
t.Fatal("a resolved thread still suppressed auto-merge")
}
}
// An agent reply does not lift the gate. The agent answering a critique is not
// the owner accepting the answer, and if it were, the gate would be under the
// control of the thing it exists to hold back.
func TestAgentReplyDoesNotLiftTheGate(t *testing.T) {
svc, ctx, _, res, base := autoMergeSetup(t)
th := commentOn(t, svc, ctx, res.Proposal.ID)
if _, err := svc.ReplyTo(ctx, agentPrincipal(), th.Root.ID, "Fixed in this revision."); err != nil {
t.Fatalf("ReplyTo: %v", err)
}
revised := revise(t, svc, ctx, res, base, "second body")
if revised.Merged {
t.Fatal("an agent reply cleared the auto-merge gate")
}
}
// The gate is on policy auto-merge only. A comment nobody got round to
// resolving must not be able to wedge a proposal shut — the owner clicking
// approve is the judgement the thread was asking for.
func TestManualApproveIgnoresOpenThreads(t *testing.T) {
svc, ctx, _, res, _ := autoMergeSetup(t)
commentOn(t, svc, ctx, res.Proposal.ID)
merged, err := svc.MergeHuman(ctx, fxSpace, res.Proposal.ID)
if err != nil {
t.Fatalf("MergeHuman with an open thread: %v", err)
}
if merged.State != core.StateMerged {
t.Errorf("state = %s, want merged", merged.State)
}
if merged.Approval != core.ApprovalHuman {
t.Errorf("approval = %q, want human", merged.Approval)
}
}
// Who may do what. An agent may join a conversation but may neither start one
// nor declare it finished: both would hand it control of the gate.
func TestThreadAuthority(t *testing.T) {
svc, ctx, _, res, _ := autoMergeSetup(t)
if _, err := svc.CommentOn(ctx, CommentRequest{
Principal: agentPrincipal(), Space: fxSpace, ProposalID: res.Proposal.ID,
DocPath: "notes/a.md", Anchor: core.CommentAnchor{DocID: "N-1", Side: core.SideNew},
Body: "self-review",
}); !errors.Is(err, ErrForbidden) {
t.Errorf("agent opening a thread = %v, want ErrForbidden", err)
}
th := commentOn(t, svc, ctx, res.Proposal.ID)
reply, err := svc.ReplyTo(ctx, agentPrincipal(), th.Root.ID, "Acknowledged.")
if err != nil {
t.Fatalf("agent reply: %v", err)
}
if !reply.Agent {
t.Error("agent reply did not come back marked as an agent's")
}
if err := svc.ResolveThread(ctx, agentPrincipal(), th.Root.ID, true); !errors.Is(err, ErrForbidden) {
t.Errorf("agent resolving a thread = %v, want ErrForbidden", err)
}
threads, err := svc.Threads(ctx, ownerPrincipal(), res.Proposal.ID)
if err != nil {
t.Fatalf("Threads: %v", err)
}
if len(threads) != 1 {
t.Fatalf("Threads returned %d threads, want 1 (replies belong to their root)", len(threads))
}
if len(threads[0].Replies) != 1 || !threads[0].Replies[0].Agent {
t.Errorf("replies = %+v, want one agent reply", threads[0].Replies)
}
if !threads[0].Open() {
t.Error("thread reports closed; the agent must not have been able to resolve it")
}
}
// --- anchoring against a revision (no database) ---
const anchorDoc = `# Storage
The first paragraph of the storage section.
The second paragraph, which will be edited.
## Trade-offs
A trade-off paragraph.
`
func anchorFor(t *testing.T, src string, ordinal int) core.CommentAnchor {
t.Helper()
a, err := AnchorOf("SPEC-0007", []byte(src), ordinal, core.SideNew)
if err != nil {
t.Fatalf("AnchorOf(%d): %v", ordinal, err)
}
return a
}
// AnchorOf and AnchorBlocks must number blocks identically — one builds an
// anchor, the other resolves it, and a disagreement would put every comment on
// the wrong block of its own section.
func TestAnchorOfAgreesWithResolution(t *testing.T) {
src := []byte(anchorDoc)
blocks := anchorBlocksOf(src)
for ordinal := range blocks {
a := anchorFor(t, anchorDoc, ordinal)
got, state := core.ResolveAnchor(a, blocks)
if got != ordinal || state != core.AnchorExact {
t.Errorf("block %d: resolved to (%d, %s), want (%d, %s)",
ordinal, got, state, ordinal, core.AnchorExact)
}
}
if _, err := AnchorOf("SPEC-0007", src, len(blocks), core.SideNew); !errors.Is(err, ErrInvalid) {
t.Errorf("AnchorOf past the end = %v, want ErrInvalid", err)
}
}
// The three outcomes, against a revision the agent has since pushed: untouched
// text stays anchored, edited text keeps its comment and says it was edited,
// and text that is gone is reported outdated rather than moved onto a
// neighbouring paragraph.
func TestAnchorThreadsReportsFitAgainstARevision(t *testing.T) {
const revised = `# Storage
The first paragraph of the storage section.
The second paragraph, completely rewritten in the agent's revision.
## Trade-offs
A trade-off paragraph.
`
untouched := anchorFor(t, anchorDoc, 1) // "The first paragraph..."
edited := anchorFor(t, anchorDoc, 2) // "The second paragraph, which will be edited."
// An anchor to a block that the revision drops entirely.
const withExtra = anchorDoc + "\nA paragraph that the revision removes.\n"
removed := anchorFor(t, withExtra, 5)
threads := []Thread{
{DocPath: "specs/a.md", Anchor: untouched},
{DocPath: "specs/a.md", Anchor: edited},
{DocPath: "specs/a.md", Anchor: removed},
{DocPath: "specs/gone.md", Anchor: untouched},
}
docs := []ProposalDoc{{Path: "specs/a.md", Base: []byte(anchorDoc), Proposed: []byte(revised)}}
got := AnchorThreads(threads, docs)
want := []core.AnchorState{
core.AnchorExact,
core.AnchorEdited,
core.AnchorOutdated,
core.AnchorOutdated, // its document is not among the proposal's changes
}
for i := range want {
if got[i].State != want[i] {
t.Errorf("thread %d: state = %s, want %s", i, got[i].State, want[i])
}
}
if got[0].Block < 0 || got[1].Block < 0 {
t.Errorf("a resolved anchor must name a block: %d, %d", got[0].Block, got[1].Block)
}
if got[2].Block != -1 || got[3].Block != -1 {
t.Errorf("an outdated anchor must name no block: %d, %d", got[2].Block, got[3].Block)
}
// The input must not be mutated: a caller rendering two revisions would
// otherwise see the first one's answers on the second.
if threads[0].State != "" || threads[0].Block != 0 {
t.Error("AnchorThreads mutated its input")
}
}
// A thread whose document the proposal no longer changes — the agent reverted
// it — is outdated, not dropped. A comment that silently vanished would look
// like one that was never made.
func TestRevertedDocumentOutdatesItsThreadsRatherThanLosingThem(t *testing.T) {
threads := []Thread{{DocPath: "specs/a.md", Anchor: anchorFor(t, anchorDoc, 1)}}
got := AnchorThreads(threads, nil)
if len(got) != 1 {
t.Fatalf("AnchorThreads returned %d threads, want 1 kept", len(got))
}
if got[0].State != core.AnchorOutdated || got[0].Block != -1 {
t.Errorf("state/block = %s/%d, want outdated/-1", got[0].State, got[0].Block)
}
}
// A comment on the old side of a deleted block reads the base, not the proposed
// text — there is no proposed text for a block the change removes.
func TestOldSideAnchorsAgainstTheBase(t *testing.T) {
a := anchorFor(t, anchorDoc, 2)
a.Side = core.SideOld
got := AnchorThreads([]Thread{{DocPath: "specs/a.md", Anchor: a}},
[]ProposalDoc{{Path: "specs/a.md", Base: []byte(anchorDoc), Proposed: []byte("# Storage\n")}})
if got[0].State != core.AnchorExact {
t.Errorf("state = %s, want %s: an old-side anchor resolves against the base",
got[0].State, core.AnchorExact)
}
}
M service/propose.go => service/propose.go +24 -5
@@ 356,12 356,31 @@ func (s *Service) tryAutoMerge(ctx context.Context, sp *Space, row *db.Proposal)
// autoMerges reports whether every path the proposal changes may skip human
// review under the policy at the approved head.
//
-// It is fail-closed in two directions. An empty changed set is not auto-merged
-// (there is nothing to land), and any path that does not match is enough to
-// require a human: a proposal that touches one reviewed document is reviewed as
-// a whole, never split. The policy is read at the approved head because that is
-// where the merge lands and whose auto_merge patterns therefore govern it.
+// It is fail-closed in three directions. An open review thread stops it (see
+// below), an empty changed set is not auto-merged (there is nothing to land),
+// and any path that does not match is enough to require a human: a proposal
+// that touches one reviewed document is reviewed as a whole, never split. The
+// policy is read at the approved head because that is where the merge lands and
+// whose auto_merge patterns therefore govern it.
func (s *Service) autoMerges(ctx context.Context, sp *Space, row *db.Proposal) (bool, error) {
+ // An unresolved review thread means the owner engaged with this proposal,
+ // so it must not land unattended on the agent's next revision. This is
+ // checked first because it is the cheapest decisive question and the one
+ // most likely to be the answer: a commented proposal is, by definition, one
+ // a human already stopped to look at.
+ //
+ // It gates policy auto-merge only. MergeHuman does not consult it, because
+ // the owner clicking approve is the judgement the thread was asking for, and
+ // a comment nobody got round to resolving must not be able to wedge a
+ // proposal shut.
+ open, err := s.store.HasUnresolvedComments(ctx, row.ID)
+ if err != nil {
+ return false, err
+ }
+ if open {
+ return false, nil
+ }
+
head, err := sp.Repo.ApprovedHead(ctx)
if err != nil {
return false, err