Skip to content

Commit 4326e0b

Browse files
boazreicherboazreicher
committed
cr comments
1 parent eebd8a3 commit 4326e0b

2 files changed

Lines changed: 151 additions & 8 deletions

File tree

‎pkg/github/granular_tools_test.go‎

Lines changed: 145 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1313,7 +1313,11 @@ func TestGranularSetIssueFields(t *testing.T) {
13131313
"rationale": "Reflects the reported severity",
13141314
},
13151315
},
1316-
}).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(5)})),
1316+
}).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{
1317+
ID: gogithub.Ptr(int64(123)),
1318+
Number: gogithub.Ptr(5),
1319+
HTMLURL: gogithub.Ptr("https://github.com/owner/repo/issues/5"),
1320+
})),
13171321
}))
13181322

13191323
deps := BaseDeps{Client: restClient, GQLClient: gqlClient}
@@ -1335,6 +1339,9 @@ func TestGranularSetIssueFields(t *testing.T) {
13351339
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
13361340
require.NoError(t, err)
13371341
assert.False(t, result.IsError)
1342+
textContent := getTextResult(t, result)
1343+
assert.Contains(t, textContent.Text, `"id":"123"`)
1344+
assert.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/5")
13381345
})
13391346

13401347
t.Run("rationale too long returns error", func(t *testing.T) {
@@ -1396,7 +1403,11 @@ func TestGranularSetIssueFields(t *testing.T) {
13961403
"suggest": true,
13971404
},
13981405
},
1399-
}).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(5)})),
1406+
}).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{
1407+
ID: gogithub.Ptr(int64(123)),
1408+
Number: gogithub.Ptr(5),
1409+
HTMLURL: gogithub.Ptr("https://github.com/owner/repo/issues/5"),
1410+
})),
14001411
}))
14011412

14021413
deps := BaseDeps{Client: restClient, GQLClient: gqlClient}
@@ -1419,5 +1430,137 @@ func TestGranularSetIssueFields(t *testing.T) {
14191430
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
14201431
require.NoError(t, err)
14211432
assert.False(t, result.IsError)
1433+
textContent := getTextResult(t, result)
1434+
assert.Contains(t, textContent.Text, `"id":"123"`)
1435+
assert.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/5")
1436+
})
1437+
1438+
t.Run("successful set with single_select_option_id and rationale", func(t *testing.T) {
1439+
fieldsQuery := githubv4mock.NewQueryMatcher(
1440+
issueFieldsRepoQuery{},
1441+
map[string]any{
1442+
"owner": githubv4.String("owner"),
1443+
"name": githubv4.String("repo"),
1444+
},
1445+
githubv4mock.DataResponse(map[string]any{
1446+
"repository": map[string]any{
1447+
"issueFields": map[string]any{
1448+
"nodes": []any{
1449+
map[string]any{
1450+
"__typename": "IssueFieldSingleSelect",
1451+
"id": "FIELD_1",
1452+
"fullDatabaseId": "42",
1453+
"name": "Priority",
1454+
"dataType": "SINGLE_SELECT",
1455+
"visibility": "ALL",
1456+
"options": []any{
1457+
map[string]any{"id": "OPT_HIGH", "name": "High", "description": ""},
1458+
},
1459+
},
1460+
},
1461+
},
1462+
},
1463+
}),
1464+
)
1465+
gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(fieldsQuery))
1466+
1467+
restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
1468+
PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{
1469+
"issue_field_values": []any{
1470+
map[string]any{
1471+
"field_id": float64(42),
1472+
"value": "High",
1473+
"rationale": "Severe customer impact",
1474+
},
1475+
},
1476+
}).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{
1477+
ID: gogithub.Ptr(int64(123)),
1478+
Number: gogithub.Ptr(5),
1479+
HTMLURL: gogithub.Ptr("https://github.com/owner/repo/issues/5"),
1480+
})),
1481+
}))
1482+
1483+
deps := BaseDeps{Client: restClient, GQLClient: gqlClient}
1484+
serverTool := GranularSetIssueFields(translations.NullTranslationHelper)
1485+
handler := serverTool.Handler(deps)
1486+
1487+
request := createMCPRequest(map[string]any{
1488+
"owner": "owner",
1489+
"repo": "repo",
1490+
"issue_number": float64(5),
1491+
"fields": []any{
1492+
map[string]any{
1493+
"field_id": "FIELD_1",
1494+
"single_select_option_id": "OPT_HIGH",
1495+
"rationale": "Severe customer impact",
1496+
},
1497+
},
1498+
})
1499+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
1500+
require.NoError(t, err)
1501+
assert.False(t, result.IsError)
1502+
})
1503+
1504+
t.Run("successful set with number value and rationale", func(t *testing.T) {
1505+
fieldsQuery := githubv4mock.NewQueryMatcher(
1506+
issueFieldsRepoQuery{},
1507+
map[string]any{
1508+
"owner": githubv4.String("owner"),
1509+
"name": githubv4.String("repo"),
1510+
},
1511+
githubv4mock.DataResponse(map[string]any{
1512+
"repository": map[string]any{
1513+
"issueFields": map[string]any{
1514+
"nodes": []any{
1515+
map[string]any{
1516+
"__typename": "IssueFieldNumber",
1517+
"id": "FIELD_1",
1518+
"fullDatabaseId": "42",
1519+
"name": "Score",
1520+
"dataType": "NUMBER",
1521+
"visibility": "ALL",
1522+
},
1523+
},
1524+
},
1525+
},
1526+
}),
1527+
)
1528+
gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(fieldsQuery))
1529+
1530+
restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
1531+
PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{
1532+
"issue_field_values": []any{
1533+
map[string]any{
1534+
"field_id": float64(42),
1535+
"value": float64(7),
1536+
"rationale": "Initial estimate",
1537+
},
1538+
},
1539+
}).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{
1540+
ID: gogithub.Ptr(int64(123)),
1541+
Number: gogithub.Ptr(5),
1542+
HTMLURL: gogithub.Ptr("https://github.com/owner/repo/issues/5"),
1543+
})),
1544+
}))
1545+
1546+
deps := BaseDeps{Client: restClient, GQLClient: gqlClient}
1547+
serverTool := GranularSetIssueFields(translations.NullTranslationHelper)
1548+
handler := serverTool.Handler(deps)
1549+
1550+
request := createMCPRequest(map[string]any{
1551+
"owner": "owner",
1552+
"repo": "repo",
1553+
"issue_number": float64(5),
1554+
"fields": []any{
1555+
map[string]any{
1556+
"field_id": "FIELD_1",
1557+
"number_value": float64(7),
1558+
"rationale": "Initial estimate",
1559+
},
1560+
},
1561+
})
1562+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
1563+
require.NoError(t, err)
1564+
assert.False(t, result.IsError)
14221565
})
14231566
}

‎pkg/github/issues_granular.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"encoding/json"
66
"fmt"
77
"maps"
8-
"strconv"
98
"strings"
109

1110
ghErrors "github.com/github/github-mcp-server/pkg/errors"
@@ -1157,16 +1156,17 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv
11571156

11581157
// issueFieldValueRESTRequest is the JSON body shape accepted by the REST
11591158
// update-issue endpoint when setting issue field values with rationale or
1160-
// suggestion. The endpoint expects integer database IDs and a single string
1161-
// value per field. Used as a fallback to the GraphQL setIssueFieldValue
1159+
// suggestion. The endpoint expects integer database IDs and a value whose
1160+
// JSON type matches the field's data type (string for text/date/single_select,
1161+
// number for number). Used as a fallback to the GraphQL setIssueFieldValue
11621162
// mutation, which does not (yet) support rationale or suggest.
11631163
type issueFieldValueRESTRequest struct {
11641164
IssueFieldValues []issueFieldValueRESTEntry `json:"issue_field_values"`
11651165
}
11661166

11671167
type issueFieldValueRESTEntry struct {
11681168
FieldID int64 `json:"field_id"`
1169-
Value string `json:"value"`
1169+
Value any `json:"value"`
11701170
Rationale string `json:"rationale,omitempty"`
11711171
Suggest bool `json:"suggest,omitempty"`
11721172
}
@@ -1215,12 +1215,12 @@ func setIssueFieldsViaREST(
12151215
return utils.NewToolResultError(fmt.Sprintf("could not resolve database ID for field %q", def.Name)), nil, nil
12161216
}
12171217

1218-
var value string
1218+
var value any
12191219
switch {
12201220
case f.TextValue != nil:
12211221
value = string(*f.TextValue)
12221222
case f.NumberValue != nil:
1223-
value = strconv.FormatFloat(float64(*f.NumberValue), 'f', -1, 64)
1223+
value = float64(*f.NumberValue)
12241224
case f.DateValue != nil:
12251225
value = string(*f.DateValue)
12261226
case f.SingleSelectOptionID != nil:

0 commit comments

Comments
 (0)