a73x

deb80501

Design comment resolution anchored to revisions

a73x   2026-08-09 16:27

Commit message
Design comment resolution anchored to revisions

Inline comments have no identity in derived state, so nothing can name the
comment being resolved; carrying the event commit OID through fixes that
with no migration. Resolve and reopen become ordinary events, so existing
Lamport ordering handles concurrency and disagreement stays in the record.

Adds patch diff --answers, which resolves a resolution to the interdiff
that answered it, scoped to the comment's file. This is the thing a pull
request cannot offer once the branch has been force-pushed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docs/superpowers/specs/2026-08-09-comment-resolution-design.md
Old New
@@ -0,0 +1,153 @@
1 # Comment Resolution — Design
2
3 Date: 2026-08-09
4 Status: Draft. Depends on `2026-08-09-patch-revision-refs-design.md`.
5
6 ## Purpose
7
8 Review feedback is preserved today but not usable across rounds. An inline
9 comment is anchored to the revision it was written on (`state.rs:197`), so it
10 never drifts, but nothing records whether it was ever answered. With five threads
11 across four revisions, a reviewer re-reads all five every round to find the two
12 that still matter.
13
14 That is the same tax the branch workflow imposes, arriving through a different
15 door. Under a pull request the correspondence between feedback and the change
16 answering it is lost at the force-push, and authors reconstruct it with
17 `fix(pr): address review` commits that then pollute the history. The patch model
18 can keep the correspondence, and — because revisions are immutable and
19 interdiffs are rebase-aware — can *show* it rather than assert it.
20
21 ## Blocker: inline comments have no identity
22
23 `Comment` carries `commit_id: Oid`, the OID of the event commit that created it
24 (`state.rs:83`). `InlineComment` does not (`state.rs:190-198`). There is
25 therefore no way to name the comment being resolved.
26
27 No migration is needed to fix this. State is derived by walking the event DAG, so
28 the event commit's OID is in hand when the comment is applied — it is simply
29 discarded. Carrying it through populates the field for every existing comment on
30 the next state rebuild.
31
32 Comments are addressed by OID prefix, like patch and issue ids, with the same
33 ambiguous-prefix error.
34
35 ## Model
36
37 Two events:
38
39 ```rust
40 PatchCommentResolve { comment: String, revision: Option<u32> }
41 PatchCommentReopen { comment: String }
42 ```
43
44 `revision` is the revision the resolution is claimed against, defaulting to the
45 patch's latest at the time the event is written.
46
47 Derived state gains a field:
48
49 ```rust
50 pub struct InlineComment {
51 // ...
52 pub commit_id: Oid,
53 pub resolved: Option<Resolution>,
54 }
55
56 pub struct Resolution {
57 pub by: Author,
58 pub revision: Option<u32>,
59 pub timestamp: String,
60 }
61 ```
62
63 ### Who may resolve
64
65 Anyone. The event is signed and attribution is in the record, so the interesting
66 distinction is displayed rather than enforced: a resolution by the patch author
67 is a *claim*, one by the comment's author is a *confirmation*, and the UI says
68 which. Building permissions into this would add a rule with no threat behind it —
69 the parties are collaborators, and disagreement has a representation already.
70
71 Disagreement is `PatchCommentReopen`. Both events stay in the record with their
72 authors, so "the author said this was fixed and the reviewer disagreed" is
73 history rather than a lost state transition.
74
75 ### Ordering
76
77 Resolve and reopen are ordinary events, ordered by the existing `(clock, oid)`
78 rule. Last writer wins per comment. Two people resolving concurrently converge on
79 the same answer on every machine, which is the same guarantee every other event
80 already has. No new conflict machinery.
81
82 ### Revisions do not auto-resolve or auto-reopen
83
84 Recording a new revision does not change any comment's resolution state, even if
85 that revision touches the lines a resolved comment anchored to. Deriving state
86 transitions from graph observation would surprise both parties — the author
87 would see their claim silently revoked, and the reviewer would see threads
88 reopen without anyone reopening them.
89
90 Where a later revision does touch a resolved comment's lines, that is surfaced as
91 a *hint* alongside the thread, not a state change.
92
93 ## Showing the answer
94
95 The feature that a pull request cannot offer:
96
97 ```console
98 $ git-collab patch diff a1b2c3d4 --answers 7f3a9c2
99 ```
100
101 Resolves to the interdiff from the comment's revision to its resolution's
102 revision, scoped to the file the comment anchored to. Because revision refs are
103 immutable and the interdiff is rebase-aware, this works after the author has
104 squashed, amended and rebased — the exact conditions under which GitHub has
105 nothing left to diff.
106
107 This is the dependency on the revision-refs design. Built against today's flat
108 tree-to-tree `interdiff` (`patch.rs:501-525`), the output would include every
109 upstream commit rebased over and would be worse than useless for answering a
110 one-line comment.
111
112 Where the comment carries no revision — data written before reviews were
113 revision-scoped — there is no from-side and the command reports that rather than
114 guessing.
115
116 ## Surfaces
117
118 - `patch show`: unresolved threads first, resolved ones collapsed to one line
119 naming who resolved them and at which revision.
120 - `patch log`: each revision annotated with how many comments it was claimed to
121 resolve.
122 - `patch list` and the dashboard: unresolved count per patch, since "has
123 unanswered feedback" is the state a reviewer filters on.
124 - Web UI: same grouping as `patch show`.
125
126 ## Out of scope
127
128 - **Thread comments.** `Comment` has no revision (`state.rs:77-84`) and no anchor;
129 it is conversation, not an actionable item. If it turns out reviewers use
130 thread comments for actionable feedback, revisit — but do not build it on
131 speculation.
132 - **Reviews.** A review already has a lifecycle: one current vote per author per
133 revision, with a new vote superseding the old (`state.rs:633-642`). Reviews
134 resolve themselves by being re-cast and need nothing here.
135 - Requiring all comments resolved before a patch may merge. Merge is detected
136 from the graph, not gated (`state.rs:460`), and gating it would need an
137 enforcement point that does not exist and that the design test would reject.
138
139 ## Testing
140
141 - A comment resolved on r1 and a patch revised to r3 shows as resolved at r1.
142 - Reopening after resolving leaves both events in the record with their authors.
143 - Concurrent resolve and reopen from two clones converge identically on both.
144 - `--answers` on a comment resolved across a rebase shows only the author's
145 changes, not upstream churn.
146 - `--answers` on a comment with no revision reports the absence rather than
147 emitting a diff.
148 - Recording a new revision that touches a resolved comment's lines leaves it
149 resolved, and surfaces the hint.
150 - Resolving by ambiguous OID prefix errors the same way patch and issue prefixes
151 do.
152 - Inline comments written before this change gain their `commit_id` on rebuild,
153 with no migration step.