Skip to content

Commit 569a48d

Browse files
Copilotmattdholloway
authored andcommitted
Enforce exactly one value key per field in set_issue_fields (#2339)
* Initial plan * Enforce exactly one value key per field in set_issue_fields and add tests Address review feedback: - Change validation to count value keys and reject when multiple are provided (e.g., text_value + number_value, or text_value + delete). - Add unit tests for multiple value keys and value + delete scenarios. - Run generate-docs (no doc changes needed; README was already current). Agent-Logs-Url: https://github.com/github/github-mcp-server/sessions/7e89edb3-5315-42dd-bfa1-6c962f1ba137 Co-authored-by: mattdholloway <918573+mattdholloway@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mattdholloway <918573+mattdholloway@users.noreply.github.com>
1 parent fc7a7dc commit 569a48d

File tree

2 files changed

+49
-8
lines changed

2 files changed

+49
-8
lines changed

pkg/github/granular_tools_test.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -907,4 +907,42 @@ func TestGranularSetIssueFields(t *testing.T) {
907907
textContent := getTextResult(t, result)
908908
assert.Contains(t, textContent.Text, "each field must have a value")
909909
})
910+
911+
t.Run("multiple value keys returns error", func(t *testing.T) {
912+
deps := BaseDeps{}
913+
serverTool := GranularSetIssueFields(translations.NullTranslationHelper)
914+
handler := serverTool.Handler(deps)
915+
916+
request := createMCPRequest(map[string]any{
917+
"owner": "owner",
918+
"repo": "repo",
919+
"issue_number": float64(5),
920+
"fields": []any{
921+
map[string]any{"field_id": "FIELD_1", "text_value": "hello", "number_value": float64(42)},
922+
},
923+
})
924+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
925+
require.NoError(t, err)
926+
textContent := getTextResult(t, result)
927+
assert.Contains(t, textContent.Text, "each field must have exactly one value")
928+
})
929+
930+
t.Run("value key with delete returns error", func(t *testing.T) {
931+
deps := BaseDeps{}
932+
serverTool := GranularSetIssueFields(translations.NullTranslationHelper)
933+
handler := serverTool.Handler(deps)
934+
935+
request := createMCPRequest(map[string]any{
936+
"owner": "owner",
937+
"repo": "repo",
938+
"issue_number": float64(5),
939+
"fields": []any{
940+
map[string]any{"field_id": "FIELD_1", "text_value": "hello", "delete": true},
941+
},
942+
})
943+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
944+
require.NoError(t, err)
945+
textContent := getTextResult(t, result)
946+
assert.Contains(t, textContent.Text, "each field must have exactly one value")
947+
})
910948
}

pkg/github/issues_granular.go

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -731,41 +731,44 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv
731731
FieldID: githubv4.ID(fieldID),
732732
}
733733

734-
// Check for exactly one value type (or delete)
735-
hasValue := false
734+
// Count how many value keys are present; exactly one is required.
735+
valueCount := 0
736736

737737
if v, err := OptionalParam[string](fieldMap, "text_value"); err == nil && v != "" {
738738
input.TextValue = githubv4.NewString(githubv4.String(v))
739-
hasValue = true
739+
valueCount++
740740
}
741741
if v, err := OptionalParam[float64](fieldMap, "number_value"); err == nil {
742742
if _, exists := fieldMap["number_value"]; exists {
743743
gqlFloat := githubv4.Float(v)
744744
input.NumberValue = &gqlFloat
745-
hasValue = true
745+
valueCount++
746746
}
747747
}
748748
if v, err := OptionalParam[string](fieldMap, "date_value"); err == nil && v != "" {
749749
input.DateValue = githubv4.NewString(githubv4.String(v))
750-
hasValue = true
750+
valueCount++
751751
}
752752
if v, err := OptionalParam[string](fieldMap, "single_select_option_id"); err == nil && v != "" {
753753
optionID := githubv4.ID(v)
754754
input.SingleSelectOptionID = &optionID
755-
hasValue = true
755+
valueCount++
756756
}
757757
if _, exists := fieldMap["delete"]; exists {
758758
del, err := OptionalParam[bool](fieldMap, "delete")
759759
if err == nil && del {
760760
deleteVal := githubv4.Boolean(true)
761761
input.Delete = &deleteVal
762-
hasValue = true
762+
valueCount++
763763
}
764764
}
765765

766-
if !hasValue {
766+
if valueCount == 0 {
767767
return utils.NewToolResultError("each field must have a value (text_value, number_value, date_value, single_select_option_id) or delete: true"), nil, nil
768768
}
769+
if valueCount > 1 {
770+
return utils.NewToolResultError("each field must have exactly one value (text_value, number_value, date_value, single_select_option_id) or delete: true, but multiple were provided"), nil, nil
771+
}
769772

770773
issueFields = append(issueFields, input)
771774
}

0 commit comments

Comments
 (0)