Skip to content

Commit eb3e783

Browse files
committed
add post_comments tool and comment group bundle
post_comments publishes a review in a single SetReview call: optional top-level message plus inline, range, file-level, and reply comments. Reply targets are validated against the change's existing comments so typos surface as errors instead of orphan threads; the resolved input inverts to Gerrit's unresolved wire field. Notify defaults to ALL. The comment capability group bundles get_change, get_change_comments, and post_comments -- the minimal read subset the flow needs to stand on its own. Include filters still never escalate beyond the enabled groups. Refs: #13, #15
1 parent a300f10 commit eb3e783

6 files changed

Lines changed: 446 additions & 18 deletions

File tree

internal/registry/registry.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,13 @@ func groupTools() map[config.Group][]string {
2929
tools.NameGetFileDiff,
3030
tools.NameGetChangeComments,
3131
},
32-
config.GroupComment: {}, // arrives with the comment group implementation
32+
// comment bundles the minimal read subset it needs: understanding the
33+
// change and the threads it replies to.
34+
config.GroupComment: {
35+
tools.NameGetChange,
36+
tools.NameGetChangeComments,
37+
tools.NamePostComments,
38+
},
3339
config.GroupTransition: {}, // arrives with the transition group implementation
3440
}
3541
}

internal/registry/registry_test.go

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,14 @@ import (
1313

1414
func cfg(groups []config.Group, include, exclude []string) *config.Config {
1515
return &config.Config{
16-
GerritURL: "https://gerrit.example.com",
17-
Username: "bot",
18-
Token: "s3cret",
19-
Groups: groups,
20-
IncludeTools: include,
21-
ExcludeTools: exclude,
22-
Projects: nil,
16+
GerritURL: "https://gerrit.example.com",
17+
Username: "bot",
18+
Token: "s3cret",
19+
Groups: groups,
20+
IncludeTools: include,
21+
ExcludeTools: exclude,
22+
Projects: nil,
23+
AllowForeignChanges: false,
2324
}
2425
}
2526

@@ -65,8 +66,26 @@ func Test_Resolve(t *testing.T) {
6566
want: allReadTools(),
6667
},
6768
{
68-
name: "write groups expose nothing yet",
69-
giveGroups: []config.Group{config.GroupComment, config.GroupTransition},
69+
name: "comment group bundles its read subset",
70+
giveGroups: []config.Group{config.GroupComment},
71+
giveInclude: nil,
72+
giveExclude: nil,
73+
want: []string{
74+
tools.NameGetChange,
75+
tools.NameGetChangeComments,
76+
tools.NamePostComments,
77+
},
78+
},
79+
{
80+
name: "read and comment union deduplicates",
81+
giveGroups: []config.Group{config.GroupRead, config.GroupComment},
82+
giveInclude: nil,
83+
giveExclude: nil,
84+
want: append(allReadTools(), tools.NamePostComments),
85+
},
86+
{
87+
name: "transition group exposes nothing yet",
88+
giveGroups: []config.Group{config.GroupTransition},
7089
giveInclude: nil,
7190
giveExclude: nil,
7291
want: nil,
@@ -102,7 +121,7 @@ func Test_Resolve(t *testing.T) {
102121
{
103122
name: "include never escalates beyond enabled groups",
104123
giveGroups: []config.Group{config.GroupComment},
105-
giveInclude: []string{tools.NameGetChange},
124+
giveInclude: []string{tools.NameSearchChanges},
106125
giveExclude: nil,
107126
want: nil,
108127
},

internal/tools/get-change_test.go

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -53,13 +53,14 @@ func session(t *testing.T, gerritHandler http.HandlerFunc) *mcp.ClientSession {
5353
t.Cleanup(srv.Close)
5454

5555
client, err := gerritclient.New(t.Context(), &config.Config{
56-
GerritURL: srv.URL,
57-
Username: "bot",
58-
Token: "s3cret",
59-
Groups: []config.Group{config.GroupRead},
60-
IncludeTools: nil,
61-
ExcludeTools: nil,
62-
Projects: nil,
56+
GerritURL: srv.URL,
57+
Username: "bot",
58+
Token: "s3cret",
59+
Groups: []config.Group{config.GroupRead},
60+
IncludeTools: nil,
61+
ExcludeTools: nil,
62+
Projects: nil,
63+
AllowForeignChanges: false,
6364
})
6465
require.NoError(t, err)
6566

@@ -103,6 +104,7 @@ func Test_GetChange(t *testing.T) {
103104

104105
assert.ElementsMatch(t, []string{
105106
"search_changes", "get_change", "list_change_files", "get_file_diff", "get_change_comments",
107+
"post_comments",
106108
}, names)
107109
})
108110

internal/tools/post-comments.go

Lines changed: 222 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,222 @@
1+
package tools
2+
3+
import (
4+
"context"
5+
"slices"
6+
"strings"
7+
8+
"dev.gaijin.team/go/golib/e"
9+
"dev.gaijin.team/go/golib/fields"
10+
gerrit "github.com/andygrunwald/go-gerrit"
11+
"github.com/modelcontextprotocol/go-sdk/mcp"
12+
13+
"dev.gaijin.team/go/go-gerrit-mcp/internal/gerritclient"
14+
"dev.gaijin.team/go/go-gerrit-mcp/internal/llmxml"
15+
)
16+
17+
var (
18+
errEmptyReview = e.New("nothing to post: provide a message, comments, or both")
19+
errInvalidNotify = e.New("invalid notify value, expected NONE, OWNER, OWNER_REVIEWERS, or ALL")
20+
errLineAndRange = e.New("comment cannot carry both line and start_line/end_line")
21+
errInvalidRange = e.New("end_line must be greater than or equal to start_line")
22+
errUnknownReplyTo = e.New("reply targets do not exist on the change")
23+
errCommentNoText = e.New("comment message must not be empty")
24+
errCommentNoFile = e.New("comment file must not be empty")
25+
)
26+
27+
type inlineComment struct {
28+
File string `json:"file" jsonschema:"File path; /COMMIT_MSG or /PATCHSET_LEVEL for non-file comments"`
29+
Message string `json:"message" jsonschema:"Comment text"`
30+
Line int `json:"line,omitempty" jsonschema:"1-based line, omit for a file-level comment"`
31+
StartLine int `json:"start_line,omitempty" jsonschema:"Multi-line comment start, used with end_line"`
32+
EndLine int `json:"end_line,omitempty" jsonschema:"Multi-line comment end, inclusive"`
33+
ReplyTo string `json:"reply_to,omitempty" jsonschema:"Comment id to reply to, from get_change_comments"`
34+
Resolved *bool `json:"resolved,omitempty" jsonschema:"Thread resolution intent; replies inherit when omitted"`
35+
}
36+
37+
type postCommentsInput struct {
38+
Change string `json:"change" jsonschema:"Change identifier: numeric ID, project~number, or Change-Id"`
39+
Message string `json:"message,omitempty" jsonschema:"Top-level review message"`
40+
Comments []inlineComment `json:"comments,omitempty" jsonschema:"Inline and file comments to publish"`
41+
Notify string `json:"notify,omitempty" jsonschema:"NONE, OWNER, OWNER_REVIEWERS, or ALL; default ALL"`
42+
}
43+
44+
func postComments(c *gerritclient.Client) Tool {
45+
return Tool{
46+
Name: NamePostComments,
47+
Register: func(s *mcp.Server) {
48+
mcp.AddTool(s, &mcp.Tool{
49+
Name: NamePostComments,
50+
Description: "Post a review to a Gerrit change in one call: optional top-level message " +
51+
"plus inline, range, file-level, and reply comments. Replies anchor to comment ids " +
52+
"from get_change_comments; setting resolved on a reply toggles the thread state. " +
53+
"Refused on changes not owned by the authenticated account unless the operator " +
54+
"disabled the own-changes restriction.",
55+
}, func(ctx context.Context, _ *mcp.CallToolRequest, in postCommentsInput,
56+
) (*mcp.CallToolResult, any, error) {
57+
input, err := buildReviewInput(ctx, c, in)
58+
if err != nil {
59+
return nil, nil, err
60+
}
61+
62+
if _, err := c.SetReview(ctx, in.Change, "", input); err != nil {
63+
return nil, nil, err
64+
}
65+
66+
return textResult(renderReviewAck(in)), nil, nil
67+
})
68+
},
69+
}
70+
}
71+
72+
func buildReviewInput(ctx context.Context, c *gerritclient.Client, in postCommentsInput) (*gerrit.ReviewInput, error) {
73+
if strings.TrimSpace(in.Message) == "" && len(in.Comments) == 0 {
74+
return nil, errEmptyReview
75+
}
76+
77+
notify := strings.ToUpper(strings.TrimSpace(in.Notify))
78+
if notify != "" && !slices.Contains([]string{"NONE", "OWNER", "OWNER_REVIEWERS", "ALL"}, notify) {
79+
return nil, errInvalidNotify.WithField("notify", in.Notify)
80+
}
81+
82+
comments, err := buildComments(in.Comments)
83+
if err != nil {
84+
return nil, err
85+
}
86+
87+
if err := validateReplyTargets(ctx, c, in); err != nil {
88+
return nil, err
89+
}
90+
91+
return &gerrit.ReviewInput{
92+
Message: in.Message,
93+
Comments: comments,
94+
Notify: notify,
95+
}, nil
96+
}
97+
98+
func buildComments(comments []inlineComment) (map[string][]gerrit.CommentInput, error) {
99+
if len(comments) == 0 {
100+
return nil, nil //nolint:nilnil // absent map means no inline comments in the review
101+
}
102+
103+
byFile := make(map[string][]gerrit.CommentInput, len(comments))
104+
105+
for _, comment := range comments {
106+
ci, err := buildComment(comment)
107+
if err != nil {
108+
return nil, err
109+
}
110+
111+
byFile[comment.File] = append(byFile[comment.File], ci)
112+
}
113+
114+
return byFile, nil
115+
}
116+
117+
func buildComment(comment inlineComment) (gerrit.CommentInput, error) {
118+
if strings.TrimSpace(comment.File) == "" {
119+
return gerrit.CommentInput{}, errCommentNoFile
120+
}
121+
122+
if strings.TrimSpace(comment.Message) == "" {
123+
return gerrit.CommentInput{}, errCommentNoText.WithField("file", comment.File)
124+
}
125+
126+
hasRange := comment.StartLine > 0 || comment.EndLine > 0
127+
128+
if comment.Line > 0 && hasRange {
129+
return gerrit.CommentInput{}, errLineAndRange.WithField("file", comment.File)
130+
}
131+
132+
if hasRange && comment.EndLine < comment.StartLine {
133+
return gerrit.CommentInput{}, errInvalidRange.WithFields(
134+
fields.F("file", comment.File),
135+
fields.F("start_line", comment.StartLine),
136+
fields.F("end_line", comment.EndLine),
137+
)
138+
}
139+
140+
ci := gerrit.CommentInput{
141+
Message: comment.Message,
142+
Line: comment.Line,
143+
InReplyTo: comment.ReplyTo,
144+
}
145+
146+
if hasRange {
147+
ci.Range = &gerrit.CommentRange{
148+
StartLine: comment.StartLine,
149+
StartCharacter: 0,
150+
EndLine: comment.EndLine,
151+
EndCharacter: 0,
152+
}
153+
}
154+
155+
if comment.Resolved != nil {
156+
unresolved := !*comment.Resolved
157+
158+
ci.Unresolved = &unresolved
159+
}
160+
161+
return ci, nil
162+
}
163+
164+
// validateReplyTargets refuses replies anchored to comment ids that do not
165+
// exist on the change, so typos surface as errors instead of orphan threads.
166+
func validateReplyTargets(ctx context.Context, c *gerritclient.Client, in postCommentsInput) error {
167+
var targets []string
168+
169+
for _, comment := range in.Comments {
170+
if comment.ReplyTo != "" {
171+
targets = append(targets, comment.ReplyTo)
172+
}
173+
}
174+
175+
if len(targets) == 0 {
176+
return nil
177+
}
178+
179+
existing, err := c.ListChangeComments(ctx, in.Change)
180+
if err != nil {
181+
return err
182+
}
183+
184+
known := map[string]bool{}
185+
186+
for _, comments := range existing {
187+
for _, comment := range comments {
188+
known[comment.ID] = true
189+
}
190+
}
191+
192+
var missing []string
193+
194+
for _, target := range targets {
195+
if !known[target] {
196+
missing = append(missing, target)
197+
}
198+
}
199+
200+
if len(missing) > 0 {
201+
return errUnknownReplyTo.WithFields(
202+
fields.F("change", in.Change),
203+
fields.F("ids", strings.Join(missing, ",")),
204+
)
205+
}
206+
207+
return nil
208+
}
209+
210+
func renderReviewAck(in postCommentsInput) string {
211+
notify := strings.ToUpper(strings.TrimSpace(in.Notify))
212+
if notify == "" {
213+
notify = "ALL"
214+
}
215+
216+
return llmxml.NewElement("review_posted",
217+
llmxml.Attr("change", in.Change),
218+
llmxml.Attr("message", strings.TrimSpace(in.Message) != ""),
219+
llmxml.Attr("comments", len(in.Comments)),
220+
llmxml.Attr("notify", notify),
221+
).String()
222+
}

0 commit comments

Comments
 (0)