Skip to content

Commit b2b4936

Browse files
boazreicherCopilotCopilotSamMorrowDrums
authored
Adding rationale for fields and labels in issues_granular (#2505)
* Adding rationle for fields and labels in issues_granular Co-authored-by: Copilot <copilot@github.com> * removed descriptions * test: update update_issue_labels toolsnap schema text * removed descriptions Co-authored-by: Copilot <copilot@github.com> * fixed snaps --------- Co-authored-by: Copilot <copilot@github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Sam Morrow <info@sam-morrow.com>
1 parent 0f0506d commit b2b4936

4 files changed

Lines changed: 447 additions & 35 deletions

File tree

‎pkg/github/__toolsnaps__/set_issue_fields.snap‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,11 @@
2727
"description": "The value to set for a number field",
2828
"type": "number"
2929
},
30+
"rationale": {
31+
"description": "One concise sentence explaining what specifically about the issue led you to choose this field value. State the concrete signal (e.g. 'Reports a crash when saving' → high priority).",
32+
"maxLength": 280,
33+
"type": "string"
34+
},
3035
"single_select_option_id": {
3136
"description": "The GraphQL node ID of the option to set for a single select field",
3237
"type": "string"

‎pkg/github/__toolsnaps__/update_issue_labels.snap‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,31 @@
1313
"type": "number"
1414
},
1515
"labels": {
16-
"description": "Labels to apply to this issue",
16+
"description": "Labels to apply to this issue.",
1717
"items": {
18-
"type": "string"
18+
"oneOf": [
19+
{
20+
"description": "Label name",
21+
"type": "string"
22+
},
23+
{
24+
"properties": {
25+
"name": {
26+
"description": "Label name",
27+
"type": "string"
28+
},
29+
"rationale": {
30+
"description": "One concise sentence explaining what specifically about the issue led you to choose this label. State the concrete signal (e.g. 'Reports a crash when saving' → bug).",
31+
"maxLength": 280,
32+
"type": "string"
33+
}
34+
},
35+
"required": [
36+
"name"
37+
],
38+
"type": "object"
39+
}
40+
]
1941
},
2042
"type": "array"
2143
},

‎pkg/github/granular_tools_test.go‎

Lines changed: 230 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -263,24 +263,124 @@ func TestGranularUpdateIssueAssignees(t *testing.T) {
263263
}
264264

265265
func TestGranularUpdateIssueLabels(t *testing.T) {
266-
client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
267-
PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{
268-
"labels": []any{"bug", "enhancement"},
269-
}).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(1)})),
270-
}))
271-
deps := BaseDeps{Client: client}
272-
serverTool := GranularUpdateIssueLabels(translations.NullTranslationHelper)
273-
handler := serverTool.Handler(deps)
266+
tests := []struct {
267+
name string
268+
requestArgs map[string]any
269+
expectedReq map[string]any
270+
}{
271+
{
272+
name: "labels as plain strings",
273+
requestArgs: map[string]any{
274+
"owner": "owner",
275+
"repo": "repo",
276+
"issue_number": float64(1),
277+
"labels": []any{"bug", "enhancement"},
278+
},
279+
expectedReq: map[string]any{
280+
"labels": []any{"bug", "enhancement"},
281+
},
282+
},
283+
{
284+
name: "label objects without rationale serialize as strings",
285+
requestArgs: map[string]any{
286+
"owner": "owner",
287+
"repo": "repo",
288+
"issue_number": float64(1),
289+
"labels": []any{
290+
map[string]any{"name": "bug"},
291+
"enhancement",
292+
},
293+
},
294+
expectedReq: map[string]any{
295+
"labels": []any{"bug", "enhancement"},
296+
},
297+
},
298+
{
299+
name: "mixed strings and label objects with rationale",
300+
requestArgs: map[string]any{
301+
"owner": "owner",
302+
"repo": "repo",
303+
"issue_number": float64(1),
304+
"labels": []any{
305+
"triage",
306+
map[string]any{"name": "bug", "rationale": " Reports a crash when saving "},
307+
map[string]any{"name": "frontend", "rationale": "Mentions the UI button"},
308+
},
309+
},
310+
expectedReq: map[string]any{
311+
"labels": []any{
312+
"triage",
313+
map[string]any{"name": "bug", "rationale": "Reports a crash when saving"},
314+
map[string]any{"name": "frontend", "rationale": "Mentions the UI button"},
315+
},
316+
},
317+
},
318+
}
274319

275-
request := createMCPRequest(map[string]any{
276-
"owner": "owner",
277-
"repo": "repo",
278-
"issue_number": float64(1),
279-
"labels": []string{"bug", "enhancement"},
280-
})
281-
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
282-
require.NoError(t, err)
283-
assert.False(t, result.IsError)
320+
for _, tc := range tests {
321+
t.Run(tc.name, func(t *testing.T) {
322+
client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
323+
PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, tc.expectedReq).
324+
andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(1)})),
325+
}))
326+
deps := BaseDeps{Client: client}
327+
serverTool := GranularUpdateIssueLabels(translations.NullTranslationHelper)
328+
handler := serverTool.Handler(deps)
329+
330+
request := createMCPRequest(tc.requestArgs)
331+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
332+
require.NoError(t, err)
333+
assert.False(t, result.IsError)
334+
})
335+
}
336+
}
337+
338+
func TestGranularUpdateIssueLabelsInvalidRationale(t *testing.T) {
339+
tests := []struct {
340+
name string
341+
requestArgs map[string]any
342+
expectedErrText string
343+
}{
344+
{
345+
name: "rationale too long",
346+
requestArgs: map[string]any{
347+
"owner": "owner",
348+
"repo": "repo",
349+
"issue_number": float64(1),
350+
"labels": []any{
351+
map[string]any{"name": "bug", "rationale": strings.Repeat("a", 281)},
352+
},
353+
},
354+
expectedErrText: "label rationale must be 280 characters or less",
355+
},
356+
{
357+
name: "label object missing name",
358+
requestArgs: map[string]any{
359+
"owner": "owner",
360+
"repo": "repo",
361+
"issue_number": float64(1),
362+
"labels": []any{
363+
map[string]any{"rationale": "no name provided"},
364+
},
365+
},
366+
expectedErrText: "each label object must have a 'name' string",
367+
},
368+
}
369+
370+
for _, tc := range tests {
371+
t.Run(tc.name, func(t *testing.T) {
372+
deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(nil))}
373+
serverTool := GranularUpdateIssueLabels(translations.NullTranslationHelper)
374+
handler := serverTool.Handler(deps)
375+
376+
request := createMCPRequest(tc.requestArgs)
377+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
378+
require.NoError(t, err)
379+
380+
errorContent := getErrorResult(t, result)
381+
assert.Contains(t, errorContent.Text, tc.expectedErrText)
382+
})
383+
}
284384
}
285385

286386
func TestGranularUpdateIssueMilestone(t *testing.T) {
@@ -1034,4 +1134,117 @@ func TestGranularSetIssueFields(t *testing.T) {
10341134
textContent := getTextResult(t, result)
10351135
assert.Contains(t, textContent.Text, "each field must have exactly one value")
10361136
})
1137+
1138+
t.Run("successful set with text value and rationale", func(t *testing.T) {
1139+
matchers := []githubv4mock.Matcher{
1140+
githubv4mock.NewQueryMatcher(
1141+
struct {
1142+
Repository struct {
1143+
Issue struct {
1144+
ID githubv4.ID
1145+
} `graphql:"issue(number: $issueNumber)"`
1146+
} `graphql:"repository(owner: $owner, name: $repo)"`
1147+
}{},
1148+
map[string]any{
1149+
"owner": githubv4.String("owner"),
1150+
"repo": githubv4.String("repo"),
1151+
"issueNumber": githubv4.Int(5),
1152+
},
1153+
githubv4mock.DataResponse(map[string]any{
1154+
"repository": map[string]any{
1155+
"issue": map[string]any{"id": "ISSUE_123"},
1156+
},
1157+
}),
1158+
),
1159+
githubv4mock.NewMutationMatcher(
1160+
struct {
1161+
SetIssueFieldValue struct {
1162+
Issue struct {
1163+
ID githubv4.ID
1164+
Number githubv4.Int
1165+
URL githubv4.String
1166+
}
1167+
IssueFieldValues []struct {
1168+
TextValue struct {
1169+
Value string
1170+
} `graphql:"... on IssueFieldTextValue"`
1171+
SingleSelectValue struct {
1172+
Name string
1173+
} `graphql:"... on IssueFieldSingleSelectValue"`
1174+
DateValue struct {
1175+
Value string
1176+
} `graphql:"... on IssueFieldDateValue"`
1177+
NumberValue struct {
1178+
Value float64
1179+
} `graphql:"... on IssueFieldNumberValue"`
1180+
}
1181+
} `graphql:"setIssueFieldValue(input: $input)"`
1182+
}{},
1183+
SetIssueFieldValueInput{
1184+
IssueID: githubv4.ID("ISSUE_123"),
1185+
IssueFields: []IssueFieldCreateOrUpdateInput{
1186+
{
1187+
FieldID: githubv4.ID("FIELD_1"),
1188+
TextValue: githubv4.NewString(githubv4.String("hello")),
1189+
Rationale: githubv4.NewString(githubv4.String("Reflects the reported severity")),
1190+
},
1191+
},
1192+
},
1193+
nil,
1194+
githubv4mock.DataResponse(map[string]any{
1195+
"setIssueFieldValue": map[string]any{
1196+
"issue": map[string]any{
1197+
"id": "ISSUE_123",
1198+
"number": 5,
1199+
"url": "https://github.com/owner/repo/issues/5",
1200+
},
1201+
},
1202+
}),
1203+
),
1204+
}
1205+
1206+
gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matchers...))
1207+
deps := BaseDeps{GQLClient: gqlClient}
1208+
serverTool := GranularSetIssueFields(translations.NullTranslationHelper)
1209+
handler := serverTool.Handler(deps)
1210+
1211+
request := createMCPRequest(map[string]any{
1212+
"owner": "owner",
1213+
"repo": "repo",
1214+
"issue_number": float64(5),
1215+
"fields": []any{
1216+
map[string]any{
1217+
"field_id": "FIELD_1",
1218+
"text_value": "hello",
1219+
"rationale": " Reflects the reported severity ",
1220+
},
1221+
},
1222+
})
1223+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
1224+
require.NoError(t, err)
1225+
assert.False(t, result.IsError)
1226+
})
1227+
1228+
t.Run("rationale too long returns error", func(t *testing.T) {
1229+
deps := BaseDeps{}
1230+
serverTool := GranularSetIssueFields(translations.NullTranslationHelper)
1231+
handler := serverTool.Handler(deps)
1232+
1233+
request := createMCPRequest(map[string]any{
1234+
"owner": "owner",
1235+
"repo": "repo",
1236+
"issue_number": float64(5),
1237+
"fields": []any{
1238+
map[string]any{
1239+
"field_id": "FIELD_1",
1240+
"text_value": "hello",
1241+
"rationale": strings.Repeat("a", 281),
1242+
},
1243+
},
1244+
})
1245+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
1246+
require.NoError(t, err)
1247+
textContent := getTextResult(t, result)
1248+
assert.Contains(t, textContent.Text, "field rationale must be 280 characters or less")
1249+
})
10371250
}

0 commit comments

Comments
 (0)