M .beads/issues.jsonl => .beads/issues.jsonl +5 -0
@@ 1,8 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":"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","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.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}
A => +198 -0
@@ 0,0 1,198 @@
package core
import "fmt"
// CommentSide is which revision of a changed block a comment hangs off.
//
// A modified block exists twice — once as it was approved, once as proposed —
// and the two have different content, so "this sentence is wrong" has to say
// which sentence. Comments go on the new side, because that is the text under
// review; the old side exists for a block the proposal deletes outright, where
// there is no new text to point at.
type CommentSide string
const (
// SideNew anchors to the proposed revision of a block.
SideNew CommentSide = "new"
// SideOld anchors to the approved revision, for a deleted block.
SideOld CommentSide = "old"
)
// ParseCommentSide validates a side read back from Postgres or an API request.
func ParseCommentSide(s string) (CommentSide, error) {
switch CommentSide(s) {
case SideNew, SideOld:
return CommentSide(s), nil
}
return "", fmt.Errorf("%w: %q is not one of new|old", ErrInvalidCommentSide, s)
}
// CommentAnchor is where a comment is attached: one block of one document,
// named by what it says rather than by where it sits.
//
// Line numbers are useless here. Markdown reflows, so a one-word edit rewraps a
// paragraph and every line number below it moves; that is the same property
// that made a line-oriented differ unusable for prose and it makes a
// line-oriented comment anchor unusable for the same reason.
//
// The tuple is the one prosediff already emits for every block, which is why
// the anchoring model could be validated before it was committed to a schema.
type CommentAnchor struct {
// DocID is the archive's addressing key for the document — its frontmatter
// id when it has a unique well-formed one, otherwise its path. Anchoring on
// the id rather than the path is what makes a comment survive a rename.
DocID string
// HeadingPath is the enclosing headings, outermost first.
//
// It is deliberately not part of BlockHash: renaming a section would
// otherwise change the hash of every block beneath it and orphan every
// comment in that section at once, which is the failure mode most likely to
// happen on a real editing pass.
HeadingPath []string
// Index is the block's position among the blocks sharing its HeadingPath,
// 0-based — not its position in the document.
//
// prosediff numbers blocks document-globally, and that number is what a
// caller has in hand; it is converted here because a document-global index
// is destroyed by any insertion above it. Since the fallback exists
// precisely for the case where the block's content changed, an index that
// every unrelated edit invalidates would leave the fallback unable to fire
// exactly when it is needed.
Index int
// BlockHash is prosediff's structure-plus-normalized-content hash of the
// block as it read when the comment was written.
BlockHash string
// Side is which revision of the block was commented on.
Side CommentSide
}
// AnchorBlock is the part of a segmented block that anchoring reads.
//
// It exists so that core does not import prosediff. Dependency direction is
// strictly downward and the comment anchoring rules are domain logic, not diff
// logic — a caller passes prosediff's blocks through this shape.
type AnchorBlock struct {
HeadingPath []string
// Index is the block's position among blocks sharing HeadingPath, 0-based.
// Build a slice of these with [AnchorBlocks] rather than filling it by hand.
Index int
Hash string
}
// AnchorState is how well a comment's anchor still describes the revision being
// looked at. It is derived, never stored: a comment is not outdated in general,
// it is outdated *at a revision*, and a proposal branch moves under it as the
// agent revises.
type AnchorState string
const (
// AnchorExact means the commented block is still present verbatim.
AnchorExact AnchorState = "anchored"
// AnchorEdited means the block at the anchor's position is still there but
// its text has changed since the comment was written. The comment is shown
// against it, marked, so the reader can see the critique may no longer fit.
AnchorEdited AnchorState = "edited"
// AnchorOutdated means neither the content nor the position matched.
//
// The comment is kept and reported as outdated rather than relocated to a
// best guess. A comment moved to the wrong paragraph is worse than one
// admitting it lost its place: the reader cannot tell it is wrong.
AnchorOutdated AnchorState = "outdated"
)
// AnchorBlocks converts a revision's blocks, in document order, into the shape
// [ResolveAnchor] reads — numbering each block within its own heading path.
//
// hashes and paths are parallel slices in document order, which is what a
// caller holds after segmenting: pass block.Hash and block.HeadingPath.
func AnchorBlocks(hashes []string, paths [][]string) []AnchorBlock {
out := make([]AnchorBlock, len(hashes))
seen := make(map[string]int, len(hashes))
for i, h := range hashes {
var p []string
if i < len(paths) {
p = paths[i]
}
key := headingKey(p)
out[i] = AnchorBlock{HeadingPath: p, Index: seen[key], Hash: h}
seen[key]++
}
return out
}
// ResolveAnchor locates a comment's block in a revision, returning the index
// into blocks and how confident that answer is. The index is -1 when the anchor
// did not resolve.
//
// The order is content first, position second, give up third:
//
// 1. an identical block hash means the commented text is still there, wherever
// it now sits — content is the strongest evidence and survives reflow,
// renumbering and section renames;
// 2. failing that, the block at the same position under the same headings is
// taken to be the same block, edited;
// 3. failing both, the anchor is outdated.
//
// Step 1 can match more than once — a document may repeat a paragraph, and
// "TBD" appears verbatim in a dozen places — so the heading path breaks the tie
// and the nearest index breaks what the heading path does not. Without that,
// which duplicate a comment landed on would depend on document order.
func ResolveAnchor(a CommentAnchor, blocks []AnchorBlock) (int, AnchorState) {
if best := bestHashMatch(a, blocks); best >= 0 {
return best, AnchorExact
}
for i, b := range blocks {
if b.Index == a.Index && headingKey(b.HeadingPath) == headingKey(a.HeadingPath) {
return i, AnchorEdited
}
}
return -1, AnchorOutdated
}
// bestHashMatch returns the index of the block whose hash equals the anchor's,
// preferring one under the same headings and then the closest index. It returns
// -1 when no block has that hash.
//
// sameSectionBonus dominates any positional term, so a duplicate under the
// comment's own headings always beats a nearer one somewhere else: a comment on
// "TBD" under "Storage" belongs to Storage's TBD even when another section's
// sits at a closer index.
func bestHashMatch(a CommentAnchor, blocks []AnchorBlock) int {
const sameSectionBonus = 1 << 20
if a.BlockHash == "" {
return -1
}
want := headingKey(a.HeadingPath)
best, bestScore := -1, -1
for i, b := range blocks {
if b.Hash != a.BlockHash {
continue
}
score := -abs(b.Index - a.Index) // nearer is better
if headingKey(b.HeadingPath) == want {
score += sameSectionBonus
}
if score > bestScore {
best, bestScore = i, score
}
}
return best
}
// headingKey flattens a heading path to a comparable string. "\x00" is the
// separator because it cannot occur in a heading, so no two distinct paths
// collide the way " › " would for a heading containing that sequence.
func headingKey(path []string) string {
key := ""
for _, h := range path {
key += h + "\x00"
}
return key
}
func abs(n int) int {
if n < 0 {
return -n
}
return n
}
A => +181 -0
@@ 0,0 1,181 @@
package core
import (
"errors"
"testing"
)
// blocks builds a revision out of "heading/path|hash" shorthand, numbering each
// block within its heading path exactly as AnchorBlocks does.
func blocks(specs ...string) []AnchorBlock {
hashes := make([]string, len(specs))
paths := make([][]string, len(specs))
for i, s := range specs {
var path []string
hash := s
for j := len(s) - 1; j >= 0; j-- {
if s[j] == '|' {
hash = s[j+1:]
if head := s[:j]; head != "" {
path = split(head, '/')
}
break
}
}
hashes[i], paths[i] = hash, path
}
return AnchorBlocks(hashes, paths)
}
func split(s string, sep byte) []string {
var out []string
start := 0
for i := range len(s) {
if s[i] == sep {
out = append(out, s[start:i])
start = i + 1
}
}
return append(out, s[start:])
}
func anchor(headings []string, index int, hash string) CommentAnchor {
return CommentAnchor{DocID: "SPEC-0007", HeadingPath: headings, Index: index, BlockHash: hash, Side: SideNew}
}
// Content is the strongest evidence an anchor has. A paragraph that reflowed,
// or that a dozen insertions above pushed down the document, is still the same
// paragraph, and the comment on it must not be disturbed.
func TestAnchorFollowsContentWhereverItMoves(t *testing.T) {
rev := blocks("Storage|aaa", "Storage|bbb", "Storage|ccc")
a := anchor([]string{"Storage"}, 2, "ccc")
// Two blocks inserted above: the commented block is now at index 4.
moved := blocks("Storage|new1", "Storage|new2", "Storage|aaa", "Storage|bbb", "Storage|ccc")
for name, tc := range map[string]struct {
rev []AnchorBlock
want int
}{
"unchanged": {rev, 2},
"shifted by two": {moved, 4},
} {
got, state := ResolveAnchor(a, tc.rev)
if got != tc.want || state != AnchorExact {
t.Errorf("%s: ResolveAnchor = (%d, %s), want (%d, %s)", name, got, state, tc.want, AnchorExact)
}
}
}
// Renaming a section must not orphan the comments inside it. This is why
// HeadingPath is kept out of the block hash: the content is untouched, so the
// hash still matches and the comment survives a rename it had nothing to do
// with.
func TestSectionRenameDoesNotOrphanItsComments(t *testing.T) {
a := anchor([]string{"Storage"}, 1, "bbb")
renamed := blocks("Storage model|aaa", "Storage model|bbb")
got, state := ResolveAnchor(a, renamed)
if got != 1 || state != AnchorExact {
t.Errorf("ResolveAnchor = (%d, %s), want (1, %s)", got, state, AnchorExact)
}
}
// The fallback: the text was edited, so no hash matches, but a block still sits
// at that spot under those headings. The comment is shown against it and marked
// edited, because the critique may no longer fit the words.
func TestEditedBlockKeepsTheCommentAndSaysSo(t *testing.T) {
a := anchor([]string{"Storage"}, 1, "bbb")
edited := blocks("Storage|aaa", "Storage|bbb-rewritten", "Storage|ccc")
got, state := ResolveAnchor(a, edited)
if got != 1 || state != AnchorEdited {
t.Errorf("ResolveAnchor = (%d, %s), want (1, %s)", got, state, AnchorEdited)
}
}
// The index is within the heading path, not the document. An edit in an earlier
// section changes every document-global position below it, and the fallback
// exists precisely for blocks whose content changed — so a global index would
// fail exactly when it is needed.
func TestIndexIsRelativeToTheSectionNotTheDocument(t *testing.T) {
// The comment is on the second block of "Storage": document index 3.
a := anchor([]string{"Storage"}, 1, "bbb")
// A block is added to the earlier section and the commented block is
// rewritten, so only the positional fallback can fire.
rev := blocks("Intro|i1", "Intro|i2", "Intro|i3", "Storage|aaa", "Storage|bbb-rewritten")
got, state := ResolveAnchor(a, rev)
if state != AnchorEdited {
t.Fatalf("state = %s, want %s (a section-relative index survives an insertion above)", state, AnchorEdited)
}
if got != 4 {
t.Errorf("ResolveAnchor = %d, want 4 (the second Storage block, now at document index 4)", got)
}
}
// Nothing matched. The comment is kept and reported as outdated rather than
// relocated to a best guess: a comment on the wrong paragraph is worse than one
// that admits it lost its place, because the reader cannot tell it is wrong.
func TestNothingMatchedIsOutdatedNotRelocated(t *testing.T) {
a := anchor([]string{"Storage"}, 4, "bbb")
gone := blocks("Intro|i1", "Rationale|r1")
got, state := ResolveAnchor(a, gone)
if got != -1 || state != AnchorOutdated {
t.Errorf("ResolveAnchor = (%d, %s), want (-1, %s)", got, state, AnchorOutdated)
}
}
// A document repeats itself — "TBD" appears verbatim under half the headings —
// so a hash match alone does not identify a block. The comment's own section
// wins outright, however far the block moved inside it; otherwise which
// duplicate a comment landed on would be decided by document order.
func TestDuplicateContentIsDisambiguatedByItsSection(t *testing.T) {
rev := blocks("Intro|TBD", "Rationale|TBD", "Storage|s1", "Storage|s2", "Storage|s3", "Storage|TBD")
got, state := ResolveAnchor(anchor([]string{"Storage"}, 3, "TBD"), rev)
if got != 5 || state != AnchorExact {
t.Errorf("ResolveAnchor = (%d, %s), want (5, %s) — Storage's own TBD", got, state, AnchorExact)
}
// With no section of its own to prefer, the nearest index decides, and it
// does so deterministically rather than by walk order.
got, _ = ResolveAnchor(anchor([]string{"Gone"}, 0, "TBD"), rev)
if got != 0 {
t.Errorf("ResolveAnchor = %d, want 0 (nearest index when no section matches)", got)
}
}
// A comment on a block with no enclosing heading — the document preamble — is
// an ordinary case, not a missing value.
func TestPreambleBlocksAnchorLikeAnyOther(t *testing.T) {
rev := blocks("|p1", "|p2", "Storage|s1")
got, state := ResolveAnchor(anchor(nil, 1, "p2"), rev)
if got != 1 || state != AnchorExact {
t.Errorf("ResolveAnchor = (%d, %s), want (1, %s)", got, state, AnchorExact)
}
}
// Heading paths are compared whole. Two sections whose names concatenate to the
// same string are different sections, and a comment must not cross between them.
func TestHeadingPathsDoNotCollideByConcatenation(t *testing.T) {
rev := blocks("A/BC|x", "AB/C|y")
got, _ := ResolveAnchor(anchor([]string{"AB", "C"}, 0, "y"), rev)
if got != 1 {
t.Errorf("ResolveAnchor = %d, want 1; [A BC] and [AB C] are different sections", got)
}
}
func TestParseCommentSide(t *testing.T) {
for _, s := range []string{"new", "old"} {
if _, err := ParseCommentSide(s); err != nil {
t.Errorf("ParseCommentSide(%q): %v", s, err)
}
}
if _, err := ParseCommentSide("both"); !errors.Is(err, ErrInvalidCommentSide) {
t.Errorf("ParseCommentSide(both) error = %v, want ErrInvalidCommentSide", err)
}
}
M core/errors.go => core/errors.go +3 -0
@@ 79,4 79,7 @@ var (
// proposal: ids are Postgres sequence values starting at 1, so anything
// else is an unwritten row or an unset field rather than a proposal.
ErrInvalidProposalID = errors.New("invalid proposal id")
+
+ // ErrInvalidCommentSide is returned for a comment side outside new/old.
+ ErrInvalidCommentSide = errors.New("invalid comment side")
)
A => +318 -0
@@ 0,0 1,318 @@
package db
import (
"context"
"database/sql"
"errors"
"fmt"
"time"
"github.com/lib/pq"
"sourcecraft.dev/bigbes/sr-ht-spec/core"
)
// AuthorKind distinguishes the owner from an agent. It is stored rather than
// inferred from the author string, because "who may resolve a thread" turns on
// it: an agent marking its own critique resolved would defeat the auto-merge
// gate, and a rule that depends on parsing an identity string is a rule that
// stops holding the first time an agent is named after a person.
type AuthorKind string
const (
AuthorHuman AuthorKind = "human"
AuthorAgent AuthorKind = "agent"
)
// ParseAuthorKind validates an author kind read back from Postgres.
func ParseAuthorKind(s string) (AuthorKind, error) {
switch AuthorKind(s) {
case AuthorHuman, AuthorAgent:
return AuthorKind(s), nil
}
return "", fmt.Errorf("comment: %q is not one of human|agent", s)
}
// Comment is one message in a review thread: either a thread root carrying an
// anchor, or a reply to one.
//
// Anchor is set on a root and nil on a reply. A reply inherits its root's anchor
// rather than copying it — two copies of one anchor is two things that can
// disagree about where a thread is attached.
//
// Nothing here records whether the anchor still fits. That is a property of the
// revision being looked at, not of the comment, and the branch moves under it as
// the agent revises; callers derive it with core.ResolveAnchor.
type Comment struct {
ID int
ProposalID int
ParentID int // 0 for a thread root
Anchor *core.CommentAnchor
DocPath string // path as at comment time; empty on a reply
Body string
Author string
Kind AuthorKind
Session string // agent session; empty for a human
Created time.Time
Resolved *time.Time
}
// Root reports whether the comment starts a thread rather than replying to one.
func (c *Comment) Root() bool { return c.ParentID == 0 }
const commentSelect = `
SELECT id, proposal_id, COALESCE(parent_id, 0), COALESCE(doc_id, ''),
COALESCE(doc_path, ''), heading_path, COALESCE(block_index, 0),
COALESCE(block_hash, ''), COALESCE(side, ''), body, author, author_kind,
COALESCE(agent_session, ''), created, resolved
FROM comment`
func scanComment(sc rowScanner) (*Comment, error) {
var (
c Comment
docID string
headingPath pq.StringArray
blockIndex int
blockHash string
side string
kind string
resolved sql.NullTime
)
if err := sc.Scan(&c.ID, &c.ProposalID, &c.ParentID, &docID, &c.DocPath,
&headingPath, &blockIndex, &blockHash, &side, &c.Body, &c.Author, &kind,
&c.Session, &c.Created, &resolved); err != nil {
return nil, err
}
parsedKind, err := ParseAuthorKind(kind)
if err != nil {
return nil, fmt.Errorf("comment %d: %w", c.ID, err)
}
c.Kind = parsedKind
if docID != "" {
parsedSide, err := core.ParseCommentSide(side)
if err != nil {
return nil, fmt.Errorf("comment %d: %w", c.ID, err)
}
c.Anchor = &core.CommentAnchor{
DocID: docID,
HeadingPath: []string(headingPath),
Index: blockIndex,
BlockHash: blockHash,
Side: parsedSide,
}
}
if resolved.Valid {
t := resolved.Time
c.Resolved = &t
}
return &c, nil
}
// AddComment inserts a thread root: a comment anchored to one block of one
// document in a proposal. c.Anchor, c.DocPath, c.Body, c.Author and c.Kind must
// be set. ID, Created, ParentID and Resolved are ignored on input — a root is
// always born unresolved.
func (s *Store) AddComment(ctx context.Context, c *Comment) (*Comment, error) {
if c.Anchor == nil {
return nil, fmt.Errorf("add comment: a thread root needs an anchor (use ReplyComment for a reply)")
}
if err := validateAuthor(c); err != nil {
return nil, fmt.Errorf("add comment: %w", err)
}
if _, err := core.ParseCommentSide(string(c.Anchor.Side)); err != nil {
return nil, fmt.Errorf("add comment: %w", err)
}
if c.Anchor.DocID == "" || c.DocPath == "" {
return nil, fmt.Errorf("add comment: anchor needs both a document id and a path")
}
if c.Anchor.Index < 0 {
return nil, fmt.Errorf("add comment: block index %d is negative", c.Anchor.Index)
}
// A block before the first heading has no enclosing headings, which is an
// ordinary anchor and not a missing one. pq.Array sends a nil slice as SQL
// NULL, and a NULL heading_path beside a non-NULL doc_id is exactly the
// half-written anchor ck_comment_anchor refuses — so every comment on a
// document preamble would be unwritable. An empty non-nil slice sends '{}'.
headings := c.Anchor.HeadingPath
if headings == nil {
headings = []string{}
}
const q = `
INSERT INTO comment (proposal_id, doc_id, doc_path, heading_path, block_index,
block_hash, side, body, author, author_kind, agent_session, created)
VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12)
RETURNING id, created`
out := *c
out.ParentID = 0
out.Resolved = nil
err := s.q.QueryRowContext(ctx, q,
c.ProposalID, c.Anchor.DocID, c.DocPath, pq.Array(headings),
c.Anchor.Index, c.Anchor.BlockHash, string(c.Anchor.Side), c.Body,
c.Author, string(c.Kind), nullable(c.Session), time.Now().UTC(),
).Scan(&out.ID, &out.Created)
if err != nil {
return nil, fmt.Errorf("add comment: %w", err)
}
return &out, nil
}
// ReplyComment appends a reply to an existing thread. parentID must name a
// thread root; replying to a reply is refused rather than flattened, so that a
// thread is always one root plus its replies and no caller has to walk a chain
// to render one.
//
// The parent lookup and the insert run in one transaction: without it, a thread
// deleted between the two would leave a reply pointing at nothing, or the FK
// would fail with an error that says nothing about which rule was broken.
func (s *Store) ReplyComment(ctx context.Context, parentID int, c *Comment) (*Comment, error) {
if err := validateAuthor(c); err != nil {
return nil, fmt.Errorf("reply to comment %d: %w", parentID, err)
}
if c.Anchor != nil {
return nil, fmt.Errorf("reply to comment %d: a reply inherits its thread's anchor and must not carry one", parentID)
}
var out *Comment
err := s.InTx(ctx, func(tx *Store) error {
var (
parentOf sql.NullInt64
proposal int
)
err := tx.q.QueryRowContext(ctx,
`SELECT parent_id, proposal_id FROM comment WHERE id = $1`, parentID).
Scan(&parentOf, &proposal)
if errors.Is(err, sql.ErrNoRows) {
return ErrNotFound
}
if err != nil {
return err
}
if parentOf.Valid {
return fmt.Errorf("comment %d is itself a reply; threads are one level deep", parentID)
}
const q = `
INSERT INTO comment (proposal_id, parent_id, body, author, author_kind, agent_session, created)
VALUES ($1, $2, $3, $4, $5, $6, $7)
RETURNING id, created`
reply := *c
reply.ProposalID = proposal
reply.ParentID = parentID
reply.Resolved = nil
if err := tx.q.QueryRowContext(ctx, q, proposal, parentID, c.Body,
c.Author, string(c.Kind), nullable(c.Session), time.Now().UTC(),
).Scan(&reply.ID, &reply.Created); err != nil {
return err
}
out = &reply
return nil
})
if err != nil {
if errors.Is(err, ErrNotFound) {
return nil, ErrNotFound
}
return nil, fmt.Errorf("reply to comment %d: %w", parentID, err)
}
return out, nil
}
// validateAuthor enforces the provenance rule the ck_comment_provenance
// constraint also encodes, so a caller gets the rule by name rather than a
// constraint violation.
func validateAuthor(c *Comment) error {
if c.Body == "" {
return errors.New("body is required")
}
if c.Author == "" {
return errors.New("author is required")
}
switch c.Kind {
case AuthorAgent:
if c.Session == "" {
return errors.New("an agent comment needs a session: provenance is what makes one shared token auditable")
}
case AuthorHuman:
if c.Session != "" {
return errors.New("a human comment must not carry an agent session")
}
default:
return fmt.Errorf("author kind %q is not one of human|agent", c.Kind)
}
return nil
}
// ListComments returns every comment on a proposal — roots and replies together
// — oldest first, which is both thread order and the order the review page
// renders. Served by ix_comment_proposal.
func (s *Store) ListComments(ctx context.Context, proposalID int) ([]*Comment, error) {
q := commentSelect + ` WHERE proposal_id = $1 ORDER BY created, id`
rows, err := s.q.QueryContext(ctx, q, proposalID)
if err != nil {
return nil, fmt.Errorf("list comments of proposal %d: %w", proposalID, err)
}
defer rows.Close()
var out []*Comment
for rows.Next() {
c, err := scanComment(rows)
if err != nil {
return nil, fmt.Errorf("list comments of proposal %d: %w", proposalID, err)
}
out = append(out, c)
}
if err := rows.Err(); err != nil {
return nil, fmt.Errorf("list comments of proposal %d: %w", proposalID, err)
}
return out, nil
}
// ResolveComment marks a thread resolved, or reopens it when resolved is false.
// It returns ErrNotFound when no such thread exists.
//
// Only a root can be resolved — resolution is a property of the conversation,
// not of one message in it — and the `parent_id IS NULL` guard is what makes
// resolving a reply a miss rather than a silent write to the wrong row.
//
// This does not check who is asking. Agents may not resolve, and that rule lives
// in the service layer, which is where the caller's identity is known.
func (s *Store) ResolveComment(ctx context.Context, id int, resolved bool) error {
var at any
if resolved {
at = time.Now().UTC()
}
res, err := s.q.ExecContext(ctx,
`UPDATE comment SET resolved = $1 WHERE id = $2 AND parent_id IS NULL`, at, id)
if err != nil {
return fmt.Errorf("resolve comment %d: %w", id, err)
}
n, err := res.RowsAffected()
if err != nil {
return fmt.Errorf("resolve comment %d: %w", id, err)
}
if n == 0 {
return ErrNotFound
}
return nil
}
// HasUnresolvedComments reports whether a proposal has any open thread.
//
// This is the auto-merge gate's whole question. An unresolved comment suppresses
// a *policy* merge — the owner engaged with the proposal, so it must not slip
// through unattended — but never a manual approve, because a stale comment must
// not be able to wedge a proposal shut. Served by ix_comment_unresolved.
func (s *Store) HasUnresolvedComments(ctx context.Context, proposalID int) (bool, error) {
var exists bool
err := s.q.QueryRowContext(ctx, `
SELECT EXISTS (
SELECT 1 FROM comment
WHERE proposal_id = $1 AND parent_id IS NULL AND resolved IS NULL
)`, proposalID).Scan(&exists)
if err != nil {
return false, fmt.Errorf("unresolved comments of proposal %d: %w", proposalID, err)
}
return exists, nil
}
A => +349 -0
@@ 0,0 1,349 @@
package db
import (
"context"
"errors"
"testing"
"sourcecraft.dev/bigbes/sr-ht-spec/core"
)
func mkAnchor() *core.CommentAnchor {
return &core.CommentAnchor{
DocID: "SPEC-0007",
HeadingPath: []string{"Storage", "Trade-offs"},
Index: 2,
BlockHash: "b3a1f0",
Side: core.SideNew,
}
}
func mkComment(proposalID int) *Comment {
return &Comment{
ProposalID: proposalID,
Anchor: mkAnchor(),
DocPath: "specs/0007-storage.md",
Body: "This paragraph contradicts SPEC-0003.",
Author: "bigbes",
Kind: AuthorHuman,
}
}
// The anchor is the whole point of the table, so it must survive the round trip
// intact — including the heading path, which is a Postgres array and the one
// column a driver mismatch would quietly mangle.
func TestCommentAnchorRoundTrips(t *testing.T) {
s, _, cleanup := newTestStore(t)
defer cleanup()
ctx := context.Background()
space := mkSpace(t, s, ctx, "bigbes", "rfcs")
p := mkProposal(t, s, ctx, space.ID, "Add storage model section")
added, err := s.AddComment(ctx, mkComment(p.ID))
if err != nil {
t.Fatalf("AddComment: %v", err)
}
if added.ID == 0 || added.Created.IsZero() {
t.Fatalf("AddComment returned id %d created %v", added.ID, added.Created)
}
list, err := s.ListComments(ctx, p.ID)
if err != nil {
t.Fatalf("ListComments: %v", err)
}
if len(list) != 1 {
t.Fatalf("ListComments returned %d comments, want 1", len(list))
}
got := list[0]
if !got.Root() {
t.Errorf("comment is not a root; ParentID = %d", got.ParentID)
}
if got.Anchor == nil {
t.Fatal("comment came back with no anchor")
}
want := mkAnchor()
if got.Anchor.DocID != want.DocID || got.Anchor.Index != want.Index ||
got.Anchor.BlockHash != want.BlockHash || got.Anchor.Side != want.Side {
t.Errorf("anchor = %+v, want %+v", got.Anchor, want)
}
if len(got.Anchor.HeadingPath) != 2 ||
got.Anchor.HeadingPath[0] != "Storage" || got.Anchor.HeadingPath[1] != "Trade-offs" {
t.Errorf("heading path = %v, want [Storage Trade-offs]", got.Anchor.HeadingPath)
}
if got.Resolved != nil {
t.Errorf("a new thread is born unresolved, got resolved at %v", got.Resolved)
}
}
// A block at the space root, or before the first heading, has no enclosing
// headings at all. That is an ordinary anchor, not a missing value, and an empty
// array must not come back as a one-element array holding "".
func TestCommentOnAPreambleBlockHasAnEmptyHeadingPath(t *testing.T) {
s, _, cleanup := newTestStore(t)
defer cleanup()
ctx := context.Background()
space := mkSpace(t, s, ctx, "bigbes", "rfcs")
p := mkProposal(t, s, ctx, space.ID, "Rewrite the preamble")
c := mkComment(p.ID)
c.Anchor.HeadingPath = nil
c.Anchor.Index = 0
if _, err := s.AddComment(ctx, c); err != nil {
t.Fatalf("AddComment: %v", err)
}
list, err := s.ListComments(ctx, p.ID)
if err != nil {
t.Fatalf("ListComments: %v", err)
}
if got := list[0].Anchor.HeadingPath; len(got) != 0 {
t.Errorf("heading path = %v (len %d), want empty", got, len(got))
}
}
// A reply inherits its thread's anchor and its proposal. Carrying its own copy
// would be a second thing that can disagree about where the thread is attached.
func TestReplyInheritsTheThreadAndRefusesItsOwnAnchor(t *testing.T) {
s, _, cleanup := newTestStore(t)
defer cleanup()
ctx := context.Background()
space := mkSpace(t, s, ctx, "bigbes", "rfcs")
p := mkProposal(t, s, ctx, space.ID, "Add storage model section")
root, err := s.AddComment(ctx, mkComment(p.ID))
if err != nil {
t.Fatalf("AddComment: %v", err)
}
reply, err := s.ReplyComment(ctx, root.ID, &Comment{
Body: "Fixed in the next revision.",
Author: "claude-code/spec-writer",
Kind: AuthorAgent,
Session: "8fb9c9a4-b078-4af1-89eb-d97c522f9921",
})
if err != nil {
t.Fatalf("ReplyComment: %v", err)
}
if reply.ParentID != root.ID {
t.Errorf("reply parent = %d, want %d", reply.ParentID, root.ID)
}
if reply.ProposalID != p.ID {
t.Errorf("reply proposal = %d, want %d inherited from its thread", reply.ProposalID, p.ID)
}
if reply.Anchor != nil {
t.Errorf("reply carries an anchor %+v; it must inherit its root's", reply.Anchor)
}
// A reply that brings its own anchor is refused rather than having it
// dropped, so a caller cannot believe it anchored something.
_, err = s.ReplyComment(ctx, root.ID, &Comment{
Anchor: mkAnchor(), Body: "b", Author: "bigbes", Kind: AuthorHuman,
})
if err == nil {
t.Error("ReplyComment accepted a reply carrying its own anchor")
}
// Threads are one level deep: a review conversation between one human and
// one agent has no use for a tree, and a flat thread renders without a walk.
_, err = s.ReplyComment(ctx, reply.ID, &Comment{
Body: "c", Author: "bigbes", Kind: AuthorHuman,
})
if err == nil {
t.Error("ReplyComment threaded a reply under another reply")
}
if _, err := s.ReplyComment(ctx, 999999, &Comment{
Body: "d", Author: "bigbes", Kind: AuthorHuman,
}); !errors.Is(err, ErrNotFound) {
t.Errorf("reply to a missing thread = %v, want ErrNotFound", err)
}
}
// Provenance is not optional, on the same rule as proposal: one shared token
// still yields an audit trail because the identity strings say who did what. An
// agent comment with no session is unattributable, not merely incomplete.
func TestCommentProvenanceIsEnforced(t *testing.T) {
s, _, cleanup := newTestStore(t)
defer cleanup()
ctx := context.Background()
space := mkSpace(t, s, ctx, "bigbes", "rfcs")
p := mkProposal(t, s, ctx, space.ID, "Add storage model section")
for name, mutate := range map[string]func(*Comment){
"agent with no session": func(c *Comment) { c.Kind = AuthorAgent; c.Session = "" },
"human carrying session": func(c *Comment) { c.Kind = AuthorHuman; c.Session = "sess" },
"unknown author kind": func(c *Comment) { c.Kind = "robot" },
"no author": func(c *Comment) { c.Author = "" },
"empty body": func(c *Comment) { c.Body = "" },
} {
c := mkComment(p.ID)
mutate(c)
if _, err := s.AddComment(ctx, c); err == nil {
t.Errorf("%s: AddComment succeeded, want a refusal", name)
}
}
}
// Resolution is a property of the conversation, not of one message in it, so
// only a root resolves. The parent_id IS NULL guard is what makes resolving a
// reply a miss rather than a silent write to the wrong row.
func TestOnlyAThreadRootResolves(t *testing.T) {
s, _, cleanup := newTestStore(t)
defer cleanup()
ctx := context.Background()
space := mkSpace(t, s, ctx, "bigbes", "rfcs")
p := mkProposal(t, s, ctx, space.ID, "Add storage model section")
root, err := s.AddComment(ctx, mkComment(p.ID))
if err != nil {
t.Fatalf("AddComment: %v", err)
}
reply, err := s.ReplyComment(ctx, root.ID, &Comment{
Body: "ack", Author: "bigbes", Kind: AuthorHuman,
})
if err != nil {
t.Fatalf("ReplyComment: %v", err)
}
if err := s.ResolveComment(ctx, reply.ID, true); !errors.Is(err, ErrNotFound) {
t.Errorf("resolving a reply = %v, want ErrNotFound", err)
}
if err := s.ResolveComment(ctx, root.ID, true); err != nil {
t.Fatalf("ResolveComment: %v", err)
}
list, err := s.ListComments(ctx, p.ID)
if err != nil {
t.Fatalf("ListComments: %v", err)
}
for _, c := range list {
if c.Root() && c.Resolved == nil {
t.Error("root came back unresolved after being resolved")
}
if !c.Root() && c.Resolved != nil {
t.Error("reply carries its own resolution")
}
}
// Reopening is the same call, so a thread resolved by mistake is not a
// dead end.
if err := s.ResolveComment(ctx, root.ID, false); err != nil {
t.Fatalf("reopen: %v", err)
}
list, _ = s.ListComments(ctx, p.ID)
if list[0].Resolved != nil {
t.Error("reopened thread still carries a resolution timestamp")
}
}
// The auto-merge gate's whole question. An unresolved thread suppresses a policy
// merge — the owner engaged, so it must not slip through unattended — and a
// resolved one stops suppressing it, so a stale comment cannot wedge a proposal.
func TestHasUnresolvedCommentsDrivesTheAutoMergeGate(t *testing.T) {
s, _, cleanup := newTestStore(t)
defer cleanup()
ctx := context.Background()
space := mkSpace(t, s, ctx, "bigbes", "rfcs")
p := mkProposal(t, s, ctx, space.ID, "Add storage model section")
unresolved, err := s.HasUnresolvedComments(ctx, p.ID)
if err != nil {
t.Fatalf("HasUnresolvedComments: %v", err)
}
if unresolved {
t.Error("a proposal with no comments reports unresolved threads")
}
root, err := s.AddComment(ctx, mkComment(p.ID))
if err != nil {
t.Fatalf("AddComment: %v", err)
}
if unresolved, _ = s.HasUnresolvedComments(ctx, p.ID); !unresolved {
t.Error("an open thread must suppress policy auto-merge")
}
// A reply does not resolve anything: the agent answering a critique is not
// the owner accepting the answer.
if _, err := s.ReplyComment(ctx, root.ID, &Comment{
Body: "Fixed.", Author: "claude-code/spec-writer", Kind: AuthorAgent, Session: "sess",
}); err != nil {
t.Fatalf("ReplyComment: %v", err)
}
if unresolved, _ = s.HasUnresolvedComments(ctx, p.ID); !unresolved {
t.Error("an agent reply resolved the thread; only the owner may")
}
if err := s.ResolveComment(ctx, root.ID, true); err != nil {
t.Fatalf("ResolveComment: %v", err)
}
if unresolved, _ = s.HasUnresolvedComments(ctx, p.ID); unresolved {
t.Error("a resolved thread still suppresses auto-merge")
}
}
// Comments belong to their proposal and to nothing else: dropping the proposal
// takes its whole review conversation with it rather than leaving rows pointing
// at an id that no longer resolves.
func TestCommentsCascadeWithTheirProposal(t *testing.T) {
s, pool, cleanup := newTestStore(t)
defer cleanup()
ctx := context.Background()
space := mkSpace(t, s, ctx, "bigbes", "rfcs")
p := mkProposal(t, s, ctx, space.ID, "Add storage model section")
root, err := s.AddComment(ctx, mkComment(p.ID))
if err != nil {
t.Fatalf("AddComment: %v", err)
}
if _, err := s.ReplyComment(ctx, root.ID, &Comment{
Body: "ack", Author: "bigbes", Kind: AuthorHuman,
}); err != nil {
t.Fatalf("ReplyComment: %v", err)
}
if _, err := pool.ExecContext(ctx, `DELETE FROM proposal WHERE id = $1`, p.ID); err != nil {
t.Fatalf("delete proposal: %v", err)
}
var n int
if err := pool.QueryRowContext(ctx, `SELECT count(*) FROM comment`).Scan(&n); err != nil {
t.Fatalf("count comments: %v", err)
}
if n != 0 {
t.Errorf("%d comments outlived their proposal", n)
}
}
// The anchor columns are one value spread over six columns, so a half-written
// anchor must be unwritable rather than merely unwritten by this package's
// happy path. Go-level validation is not the guard here; the constraint is.
func TestHalfAnAnchorIsUnwritable(t *testing.T) {
s, pool, cleanup := newTestStore(t)
defer cleanup()
ctx := context.Background()
space := mkSpace(t, s, ctx, "bigbes", "rfcs")
p := mkProposal(t, s, ctx, space.ID, "Add storage model section")
for name, q := range map[string]string{
"doc id without a hash": `
INSERT INTO comment (proposal_id, doc_id, doc_path, heading_path, block_index, side, body, author, author_kind)
VALUES ($1, 'SPEC-0007', 'specs/a.md', ARRAY['Storage'], 0, 'new', 'b', 'bigbes', 'human')`,
"root with no anchor at all": `
INSERT INTO comment (proposal_id, body, author, author_kind)
VALUES ($1, 'b', 'bigbes', 'human')`,
"side outside new|old": `
INSERT INTO comment (proposal_id, doc_id, doc_path, heading_path, block_index, block_hash, side, body, author, author_kind)
VALUES ($1, 'SPEC-0007', 'specs/a.md', ARRAY['Storage'], 0, 'h', 'both', 'b', 'bigbes', 'human')`,
"negative block index": `
INSERT INTO comment (proposal_id, doc_id, doc_path, heading_path, block_index, block_hash, side, body, author, author_kind)
VALUES ($1, 'SPEC-0007', 'specs/a.md', ARRAY['Storage'], -1, 'h', 'new', 'b', 'bigbes', 'human')`,
} {
if _, err := pool.ExecContext(ctx, q, p.ID); err == nil {
t.Errorf("%s: the database accepted it", name)
}
}
}
M db/unit_test.go => db/unit_test.go +5 -8
@@ 368,15 368,12 @@ func TestSchemaMatchesMigration(t *testing.T) {
}
}
- // The design deliberately omits this one; adding it needs a design change,
- // not a quiet migration.
- for _, absent := range []string{"comment"} {
- if contains(created, absent) {
- t.Errorf("table %q is deliberately absent from v1", absent)
- }
- }
+ // `comment` was on an absent-list here through v1, so that adding it needed
+ // a design change rather than a quiet migration. Phase 5b is that design
+ // change: the anchoring model was settled against the built review UI, so
+ // the table is now required present like any other.
for _, want := range []string{"space", "document_id", "proposal", "agent_token",
- "index_stamp", "digest_mark", "project", "project_space"} {
+ "index_stamp", "digest_mark", "project", "project_space", "comment"} {
if !contains(created, want) {
t.Errorf("table %q is missing from schema.sql", want)
}
A => +88 -0
@@ 0,0 1,88 @@
-- +brant Up
-- Inline comments on a proposal, anchored to a block of a document.
--
-- The anchoring model was held back until the review UI existed (see the note
-- this migration removes from schema.sql), because a schema is the expensive
-- thing to get wrong here. What the built UI settled: comments live on
-- proposals, any block of a proposed document can carry one, and the anchor is
-- content-first.
--
-- The stored tuple is (doc_id, heading_path, block_index, block_hash). Line
-- numbers are deliberately absent: prose reflows, so a one-word edit moves
-- every line below it, which is the same property that made a line-oriented
-- differ useless for this corpus.
--
-- WHAT IS NOT STORED: whether a comment still fits. A comment is not outdated
-- in general — it is outdated *at a revision*, and a proposal branch moves
-- under it as the agent revises. Anchor state is derived at read time by
-- core.ResolveAnchor against the revision being displayed. A column here would
-- be a cache of a function of a moving input, and would be wrong every time the
-- agent pushed.
CREATE TABLE comment (
id SERIAL PRIMARY KEY,
proposal_id INTEGER NOT NULL REFERENCES proposal(id) ON DELETE CASCADE,
-- A reply. One level only: a thread is a root plus its replies, which is
-- what a review conversation with one human and one agent actually is.
-- Enforced in db/comment.go, since a CHECK cannot look at another row.
parent_id INTEGER REFERENCES comment(id) ON DELETE CASCADE,
-- The anchor. NULL on a reply, which inherits its root's rather than
-- carrying a copy that could drift from it.
doc_id TEXT, -- archive addressing key: "SPEC-0007", or the path
doc_path TEXT, -- path as at comment time; display, and which diff it belongs to
heading_path TEXT[], -- enclosing headings, outermost first
block_index INTEGER, -- position within heading_path, NOT within the document
block_hash TEXT, -- prosediff block hash when the comment was written
side TEXT, -- new | old ("old" only for a block the proposal deletes)
body TEXT NOT NULL,
author TEXT NOT NULL, -- owner username, or "claude-code/spec-writer"
author_kind TEXT NOT NULL, -- human | agent
agent_session TEXT,
created TIMESTAMPTZ NOT NULL DEFAULT now(),
-- Set when the owner resolves the thread. Agents may not resolve — an agent
-- marking its own critique resolved would defeat the auto-merge gate below
-- — which is enforced in the service layer, where the caller's identity is
-- known; this column only records that it happened.
resolved TIMESTAMPTZ,
CONSTRAINT ck_comment_body CHECK (length(btrim(body)) > 0),
CONSTRAINT ck_comment_author CHECK (length(author) > 0),
CONSTRAINT ck_comment_author_kind CHECK (author_kind IN ('human', 'agent')),
-- Provenance, on the same rule as proposal: the identity strings, not the
-- credential, are what identify who said what, so an agent comment without
-- a session is not a comment with a missing field — it is an unattributable
-- one. NOT NULL alone would accept the empty string and lose that.
CONSTRAINT ck_comment_provenance CHECK (
(author_kind = 'agent') = (agent_session IS NOT NULL AND length(agent_session) > 0)
),
-- A root carries the whole anchor and a reply carries none of it. Written
-- as one predicate over every anchor column so a half-populated anchor —
-- the shape a partial write would leave — cannot be stored at all.
CONSTRAINT ck_comment_anchor CHECK (
(parent_id IS NULL) = (doc_id IS NOT NULL)
AND (doc_id IS NULL) = (doc_path IS NULL)
AND (doc_id IS NULL) = (heading_path IS NULL)
AND (doc_id IS NULL) = (block_index IS NULL)
AND (doc_id IS NULL) = (block_hash IS NULL)
AND (doc_id IS NULL) = (side IS NULL)
),
CONSTRAINT ck_comment_side CHECK (side IS NULL OR side IN ('new', 'old')),
CONSTRAINT ck_comment_block_index CHECK (block_index IS NULL OR block_index >= 0),
-- Resolution is a property of the thread, not of one message in it.
CONSTRAINT ck_comment_resolved CHECK (resolved IS NULL OR parent_id IS NULL)
);
-- The review page reads a proposal's whole thread set in one go.
CREATE INDEX ix_comment_proposal ON comment (proposal_id, created);
-- Replies, by thread.
CREATE INDEX ix_comment_parent ON comment (parent_id) WHERE parent_id IS NOT NULL;
-- The auto-merge gate asks one question — "does this proposal have an
-- unresolved thread?" — on every policy merge, so it gets its own partial
-- index rather than scanning a proposal's comments to answer it.
CREATE INDEX ix_comment_unresolved ON comment (proposal_id)
WHERE parent_id IS NULL AND resolved IS NULL;
-- +brant Down
DROP TABLE comment;
M schema.sql => schema.sql +85 -6
@@ 10,9 10,9 @@
-- non-transactional merge path (git refs, then Postgres, then the index)
-- tolerable.
--
--- Deliberately absent: `comment` (inline comments are post-v1, and the
--- anchoring model should be settled by building the review UI before it is
--- committed to a schema).
+-- `comment` was deliberately absent until the review UI existed: the anchoring
+-- model had to be settled against a built UI before being committed to a
+-- schema. It was, in Phase 5b, and the table is below.
-- Spaces exist as repos; this table is for listing and index bookkeeping.
CREATE TABLE space (
@@ 131,9 131,10 @@ CREATE INDEX ix_project_space_space ON project_space (space_id);
--
-- These tables are written and read by core-go's webhook engine, which inserts
-- `NOW() at time zone 'utc'` — a `timestamp` WITHOUT time zone. So, unlike
--- spec's own tables above (which use TIMESTAMPTZ), everything below uses bare
--- `timestamp` to match pages.sr.ht and avoid timezone coercion. This is
--- deliberate; do not "fix" it to TIMESTAMPTZ.
+-- spec's own tables, the three tables in THIS SECTION use bare `timestamp` to
+-- match pages.sr.ht and avoid timezone coercion. This is deliberate; do not
+-- "fix" it to TIMESTAMPTZ. The rule is scoped to core-go's own tables: `comment`
+-- below is spec's, added later, and correctly uses TIMESTAMPTZ.
-- Users. spec.sr.ht is single-owner, but the webhook subscription is
-- user-scoped in the core-go convention, so a user row is the owner's identity
@@ 192,3 193,81 @@ CREATE TABLE gql_user_wh_delivery (
response_headers varchar,
response_status integer
);
+
+-- Inline comments on a proposal, anchored to a block of a document (added in
+-- migrations/0004_comment.sql).
+--
+-- The anchor is (doc_id, heading_path, block_index, block_hash) and is
+-- content-first: a block is found by its hash wherever it moved to, and only
+-- when that fails is position consulted. Line numbers are deliberately absent —
+-- prose reflows, so a one-word edit moves every line below it.
+--
+-- WHAT IS NOT STORED: whether a comment still fits. A comment is not outdated
+-- in general, it is outdated *at a revision*, and a proposal branch moves under
+-- it as the agent revises. Anchor state is derived at read time by
+-- core.ResolveAnchor. A column here would cache a function of a moving input
+-- and be wrong every time the agent pushed.
+CREATE TABLE comment (
+ id SERIAL PRIMARY KEY,
+ proposal_id INTEGER NOT NULL REFERENCES proposal(id) ON DELETE CASCADE,
+ -- A reply. One level only: a thread is a root plus its replies, which is
+ -- what a review conversation with one human and one agent actually is.
+ -- Enforced in db/comment.go, since a CHECK cannot look at another row.
+ parent_id INTEGER REFERENCES comment(id) ON DELETE CASCADE,
+
+ -- The anchor. NULL on a reply, which inherits its root's rather than
+ -- carrying a copy that could drift from it.
+ doc_id TEXT, -- archive addressing key: "SPEC-0007", or the path
+ doc_path TEXT, -- path as at comment time; display, and which diff it belongs to
+ heading_path TEXT[], -- enclosing headings, outermost first
+ block_index INTEGER, -- position within heading_path, NOT within the document
+ block_hash TEXT, -- prosediff block hash when the comment was written
+ side TEXT, -- new | old ("old" only for a block the proposal deletes)
+
+ body TEXT NOT NULL,
+ author TEXT NOT NULL, -- owner username, or "claude-code/spec-writer"
+ author_kind TEXT NOT NULL, -- human | agent
+ agent_session TEXT,
+ created TIMESTAMPTZ NOT NULL DEFAULT now(),
+ -- Set when the owner resolves the thread. Agents may not resolve — an agent
+ -- marking its own critique resolved would defeat the auto-merge gate — which
+ -- is enforced in the service layer, where the caller's identity is known;
+ -- this column only records that it happened.
+ resolved TIMESTAMPTZ,
+
+ CONSTRAINT ck_comment_body CHECK (length(btrim(body)) > 0),
+ CONSTRAINT ck_comment_author CHECK (length(author) > 0),
+ CONSTRAINT ck_comment_author_kind CHECK (author_kind IN ('human', 'agent')),
+ -- Provenance, on the same rule as proposal: the identity strings, not the
+ -- credential, are what identify who said what, so an agent comment without a
+ -- session is not a comment with a missing field — it is an unattributable
+ -- one. NOT NULL alone would accept the empty string and lose that.
+ CONSTRAINT ck_comment_provenance CHECK (
+ (author_kind = 'agent') = (agent_session IS NOT NULL AND length(agent_session) > 0)
+ ),
+ -- A root carries the whole anchor and a reply carries none of it. Written as
+ -- one predicate over every anchor column so a half-populated anchor — the
+ -- shape a partial write would leave — cannot be stored at all.
+ CONSTRAINT ck_comment_anchor CHECK (
+ (parent_id IS NULL) = (doc_id IS NOT NULL)
+ AND (doc_id IS NULL) = (doc_path IS NULL)
+ AND (doc_id IS NULL) = (heading_path IS NULL)
+ AND (doc_id IS NULL) = (block_index IS NULL)
+ AND (doc_id IS NULL) = (block_hash IS NULL)
+ AND (doc_id IS NULL) = (side IS NULL)
+ ),
+ CONSTRAINT ck_comment_side CHECK (side IS NULL OR side IN ('new', 'old')),
+ CONSTRAINT ck_comment_block_index CHECK (block_index IS NULL OR block_index >= 0),
+ -- Resolution is a property of the thread, not of one message in it.
+ CONSTRAINT ck_comment_resolved CHECK (resolved IS NULL OR parent_id IS NULL)
+);
+
+-- The review page reads a proposal's whole thread set in one go.
+CREATE INDEX ix_comment_proposal ON comment (proposal_id, created);
+-- Replies, by thread.
+CREATE INDEX ix_comment_parent ON comment (parent_id) WHERE parent_id IS NOT NULL;
+-- The auto-merge gate asks one question — "does this proposal have an
+-- unresolved thread?" — on every policy merge, so it gets its own partial index
+-- rather than scanning a proposal's comments to answer it.
+CREATE INDEX ix_comment_unresolved ON comment (proposal_id)
+ WHERE parent_id IS NULL AND resolved IS NULL;