Skip to content

Commit 1b62471

Browse files
committed
include the caller's drafts in comment threads
get_change_comments read only published comments, while the Gerrit UI merges the viewer's unpublished drafts into thread state client-side. An agent working for a user mid-review would declare every thread resolved while the user's change screen showed dozens reopened by their own pending drafts — both technically right, the agent wrong about what the user sees. The tool now fetches the caller's drafts alongside published comments and merges them into reconstruction: draft comments render with draft="true" and the authenticated account as author (Gerrit omits author on drafts), the result carries a drafts count, and thread resolved state matches the caller's UI. Thread building now mirrors Gerrit 3.13 CommentThreads exactly: comments pool change-wide by id instead of per file, so reply chains survive file renames; reply branches merge chronologically with comment-id tiebreak, which also decides the resolved state of threads whose tail comments share a timestamp. Comments unreachable through reference cycles surface as single-comment threads instead of being dropped. Fixes: #42
1 parent 1cf57b3 commit 1b62471

8 files changed

Lines changed: 349 additions & 58 deletions

internal/gerritclient/changes.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ var (
1919
ErrListFiles = e.New("list change files")
2020
ErrGetDiff = e.New("get file diff")
2121
ErrListComments = e.New("list change comments")
22+
ErrListDrafts = e.New("list draft comments")
2223

2324
// ErrProjectScope reports an operation refused by the project allowlist.
2425
ErrProjectScope = e.New("change is outside the configured project scope")
@@ -288,3 +289,23 @@ func (c *Client) ListChangeComments(ctx context.Context, changeID string) (map[s
288289

289290
return *comments, nil
290291
}
292+
293+
// ListChangeDrafts lists the calling account's unpublished draft comments
294+
// across all revisions of a change, grouped by file path. Gerrit never
295+
// exposes other accounts' drafts.
296+
func (c *Client) ListChangeDrafts(ctx context.Context, changeID string) (map[string][]gerrit.CommentInfo, error) {
297+
if err := c.checkProjectScope(ctx, changeID); err != nil {
298+
return nil, ErrListDrafts.Wrap(err)
299+
}
300+
301+
drafts, resp, err := c.gerrit.Changes.ListChangeDrafts(ctx, changeID)
302+
if err != nil {
303+
return nil, ErrListDrafts.Wrap(apiError(resp, err), fields.F("change", changeID))
304+
}
305+
306+
if drafts == nil {
307+
return nil, ErrListDrafts.Wrap(errEmptyResponse, fields.F("change", changeID))
308+
}
309+
310+
return *drafts, nil
311+
}

internal/tools/get-change-comments.go

Lines changed: 173 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,11 @@ func getChangeComments(c *gerritclient.Client) Tool {
3737
Description: "List a Gerrit change's inline review comments — the code discussion " +
3838
"anchored to files and lines, distinct from the change messages get_change " +
3939
"shows — grouped by file and reconstructed into threads with their resolved " +
40-
"state. Comment ids are the reply anchors for post_comments; filter " +
41-
"status=unresolved to see what still needs action.",
40+
"state. The calling account's unpublished draft comments are included, marked " +
41+
"draft=\"true\" and invisible to anyone else; thread state accounts for them, " +
42+
"matching what this account's Gerrit UI shows. Comment ids are the reply " +
43+
"anchors for post_comments; filter status=unresolved to see what still needs " +
44+
"action.",
4245
}, func(ctx context.Context, _ *mcp.CallToolRequest, in getChangeCommentsInput,
4346
) (*mcp.CallToolResult, any, error) {
4447
if in.Status == "" {
@@ -49,93 +52,199 @@ func getChangeComments(c *gerritclient.Client) Tool {
4952
return nil, nil, errInvalidStatus.WithField("status", in.Status)
5053
}
5154

52-
comments, err := c.ListChangeComments(ctx, in.Change)
55+
published, err := c.ListChangeComments(ctx, in.Change)
5356
if err != nil {
5457
return nil, nil, err
5558
}
5659

57-
return textResult(renderComments(in, comments)), nil, nil
60+
drafts, err := c.ListChangeDrafts(ctx, in.Change)
61+
if err != nil {
62+
return nil, nil, err
63+
}
64+
65+
all := flattenComments(published, drafts, accountLabel(c.Self()))
66+
67+
return textResult(renderComments(in, buildThreads(all), draftCount(all))), nil, nil
5868
})
5969
},
6070
}
6171
}
6272

63-
// thread is one comment thread: the root comment and its replies in
64-
// chronological order. Its resolved state is the state of the last comment.
65-
type thread struct {
66-
comments []gerrit.CommentInfo
67-
unresolved bool
73+
// changeComment is one comment of a change annotated with the file path it
74+
// anchors to and whether it is the caller's unpublished draft.
75+
type changeComment struct {
76+
gerrit.CommentInfo
77+
78+
path string
79+
authorLabel string
80+
draft bool
81+
}
82+
83+
// flattenComments merges the published and draft per-file comment maps into
84+
// one change-wide pool. Draft entries carry no author in Gerrit's response —
85+
// they are always the caller's — so they are labeled as self.
86+
func flattenComments(published, drafts map[string][]gerrit.CommentInfo, selfLabel string) []changeComment {
87+
var all []changeComment
88+
89+
for path, comments := range published {
90+
for _, ci := range comments {
91+
all = append(all, changeComment{
92+
CommentInfo: ci, path: path, authorLabel: accountLabel(ci.Author), draft: false,
93+
})
94+
}
95+
}
96+
97+
for path, comments := range drafts {
98+
for _, ci := range comments {
99+
all = append(all, changeComment{
100+
CommentInfo: ci, path: path, authorLabel: selfLabel, draft: true,
101+
})
102+
}
103+
}
104+
105+
return all
68106
}
69107

70-
// buildThreads reconstructs threads from a flat comment list. Comments are
71-
// ordered chronologically; each reply chain is walked to its root, and a
72-
// reply whose parent is missing starts its own thread.
73-
func buildThreads(comments []gerrit.CommentInfo) []thread {
74-
ordered := make([]gerrit.CommentInfo, len(comments))
75-
copy(ordered, comments)
108+
func draftCount(all []changeComment) int {
109+
count := 0
76110

77-
slices.SortStableFunc(ordered, compareUpdated)
111+
for _, c := range all {
112+
if c.draft {
113+
count++
114+
}
115+
}
78116

79-
parent := make(map[string]string, len(ordered))
80-
for _, comment := range ordered {
81-
parent[comment.ID] = comment.InReplyTo
117+
return count
118+
}
119+
120+
// thread is one comment thread in Gerrit's order: root first, reply branches
121+
// merged chronologically with comment-id tiebreak. Its resolved state is the
122+
// state of the thread's last comment — drafts included, which is exactly the
123+
// state the caller's change screen shows.
124+
type thread struct {
125+
comments []changeComment
126+
unresolved bool
127+
}
128+
129+
// buildThreads reconstructs threads the way Gerrit 3.13 does
130+
// (CommentThreads): one change-wide pool keyed by comment id — reply chains
131+
// may cross file paths when a file was renamed between patch sets — where a
132+
// comment whose parent is absent from the pool roots its own thread.
133+
func buildThreads(all []changeComment) []thread {
134+
byID := make(map[string]changeComment, len(all))
135+
for _, c := range all {
136+
byID[c.ID] = c
82137
}
83138

84139
var (
85-
roots []string
86-
grouped = map[string][]gerrit.CommentInfo{}
140+
roots []changeComment
141+
children = map[string][]changeComment{}
87142
)
88143

89-
for _, comment := range ordered {
90-
root := rootOf(parent, comment.ID)
91-
if _, ok := grouped[root]; !ok {
92-
roots = append(roots, root)
144+
for _, c := range all {
145+
if c.InReplyTo != "" {
146+
if _, ok := byID[c.InReplyTo]; ok {
147+
children[c.InReplyTo] = append(children[c.InReplyTo], c)
148+
continue
149+
}
93150
}
94151

95-
grouped[root] = append(grouped[root], comment)
152+
roots = append(roots, c)
96153
}
97154

155+
slices.SortStableFunc(roots, compareComments)
156+
98157
threads := make([]thread, 0, len(roots))
158+
seen := 0
99159

100160
for _, root := range roots {
101-
group := grouped[root]
102-
last := group[len(group)-1]
161+
members := expandThread(root, children)
162+
163+
seen += len(members)
164+
165+
last := members[len(members)-1]
103166

104167
threads = append(threads, thread{
105-
comments: group,
168+
comments: members,
106169
unresolved: last.Unresolved != nil && *last.Unresolved,
107170
})
108171
}
109172

173+
// A reference cycle in malformed data leaves comments unreachable from
174+
// any root; surface them as single-comment threads instead of dropping
175+
// them silently.
176+
if seen < len(all) {
177+
threads = append(threads, orphanThreads(all, threads)...)
178+
}
179+
110180
return threads
111181
}
112182

113-
func compareUpdated(a, b gerrit.CommentInfo) int {
183+
// expandThread walks the reply tree from the root, always emitting the
184+
// earliest unvisited comment next, so parallel reply branches merge
185+
// chronologically while every parent stays ahead of its children.
186+
func expandThread(root changeComment, children map[string][]changeComment) []changeComment {
187+
var members []changeComment
188+
189+
frontier := []changeComment{root}
190+
191+
for len(frontier) > 0 {
192+
next := 0
193+
194+
for i := 1; i < len(frontier); i++ {
195+
if compareComments(frontier[i], frontier[next]) < 0 {
196+
next = i
197+
}
198+
}
199+
200+
c := frontier[next]
201+
202+
frontier = append(frontier[:next], frontier[next+1:]...)
203+
members = append(members, c)
204+
frontier = append(frontier, children[c.ID]...)
205+
}
206+
207+
return members
208+
}
209+
210+
// orphanThreads returns single-comment threads for comments that no built
211+
// thread contains.
212+
func orphanThreads(all []changeComment, threads []thread) []thread {
213+
reached := map[string]bool{}
214+
215+
for _, t := range threads {
216+
for _, c := range t.comments {
217+
reached[c.ID] = true
218+
}
219+
}
220+
221+
var orphans []thread
222+
223+
for _, c := range all {
224+
if !reached[c.ID] {
225+
orphans = append(orphans, thread{
226+
comments: []changeComment{c},
227+
unresolved: c.Unresolved != nil && *c.Unresolved,
228+
})
229+
}
230+
}
231+
232+
return orphans
233+
}
234+
235+
// compareComments orders comments chronologically, breaking ties (and absent
236+
// timestamps) by comment id — Gerrit's ordering, deterministic across runs.
237+
func compareComments(a, b changeComment) int {
114238
switch {
115239
case a.Updated == nil || b.Updated == nil:
116-
return 0
117240
case a.Updated.Before(b.Updated.Time):
118241
return -1
119242
case b.Updated.Before(a.Updated.Time):
120243
return 1
121244
default:
122-
return 0
123245
}
124-
}
125-
126-
// rootOf walks a reply chain to its root; the iteration cap guards against
127-
// reference cycles in malformed data.
128-
func rootOf(parent map[string]string, id string) string {
129-
for range len(parent) {
130-
up, ok := parent[id]
131-
if !ok || up == "" {
132-
return id
133-
}
134246

135-
id = up
136-
}
137-
138-
return id
247+
return strings.Compare(a.ID, b.ID)
139248
}
140249

141250
func matchesStatus(t thread, status string) bool {
@@ -149,7 +258,17 @@ func matchesStatus(t thread, status string) bool {
149258
}
150259
}
151260

152-
func renderComments(in getChangeCommentsInput, byFile map[string][]gerrit.CommentInfo) string {
261+
// renderComments groups threads under the file path of their root comment,
262+
// in sorted path order.
263+
func renderComments(in getChangeCommentsInput, threads []thread, drafts int) string {
264+
byFile := map[string][]thread{}
265+
266+
for _, t := range threads {
267+
path := t.comments[0].path
268+
269+
byFile[path] = append(byFile[path], t)
270+
}
271+
153272
paths := make([]string, 0, len(byFile))
154273
for path := range byFile {
155274
paths = append(paths, path)
@@ -163,11 +282,9 @@ func renderComments(in getChangeCommentsInput, byFile map[string][]gerrit.Commen
163282
)
164283

165284
for _, path := range paths {
166-
threads := buildThreads(byFile[path])
167-
168285
var fileThreads []string
169286

170-
for _, t := range threads {
287+
for _, t := range byFile[path] {
171288
if !matchesStatus(t, in.Status) {
172289
continue
173290
}
@@ -189,6 +306,7 @@ func renderComments(in getChangeCommentsInput, byFile map[string][]gerrit.Commen
189306
llmxml.Attr("change", in.Change),
190307
llmxml.Attr("filter", in.Status),
191308
llmxml.Attr("threads", threadCount),
309+
llmxml.Attr("drafts", drafts),
192310
)
193311

194312
if len(rendered) == 0 {
@@ -204,9 +322,13 @@ func renderThread(t thread) string {
204322
for _, comment := range t.comments {
205323
el := llmxml.NewElement("comment",
206324
llmxml.Attr("id", comment.ID),
207-
llmxml.Attr("author", accountLabel(comment.Author)),
325+
llmxml.Attr("author", comment.authorLabel),
208326
)
209327

328+
if comment.draft {
329+
el.Attr(llmxml.Attr("draft", true))
330+
}
331+
210332
if comment.Updated != nil {
211333
el.Attr(llmxml.Attr("date", timestamp(*comment.Updated)))
212334
}

0 commit comments

Comments
 (0)