Enforce exactly one value key per field in set_issue_fields#2339
Merged
mattdholloway merged 2 commits intoadd-set-issue-fields-toolfrom Apr 16, 2026
Merged
Conversation
…ests 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>
Copilot
AI
changed the title
[WIP] Fix code based on review comments
Enforce exactly one value key per field in set_issue_fields
Apr 16, 2026
mattdholloway
merged commit 8cfd9ad
into
add-set-issue-fields-tool
13 of 16 checks passed
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens runtime validation for the set_issue_fields granular tool so each fields[] element must specify exactly one value input (or delete:true), rejecting ambiguous combinations that previously slipped through.
Changes:
- Replace the prior “at least one value” boolean check with a
valueCountenforcement that errors when multiple values are provided. - Add test cases to verify rejection of multiple value keys and value+
delete:truecombinations.
Show a summary per file
| File | Description |
|---|---|
pkg/github/issues_granular.go |
Enforces “exactly one” value key (or delete:true) per field entry via valueCount validation. |
pkg/github/granular_tools_test.go |
Adds coverage for newly-rejected ambiguous input combinations. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 2
Comment on lines
+734
to
763
| // Count how many value keys are present; exactly one is required. | ||
| valueCount := 0 | ||
|
|
||
| if v, err := OptionalParam[string](fieldMap, "text_value"); err == nil && v != "" { | ||
| input.TextValue = githubv4.NewString(githubv4.String(v)) | ||
| hasValue = true | ||
| valueCount++ | ||
| } | ||
| if v, err := OptionalParam[float64](fieldMap, "number_value"); err == nil { | ||
| if _, exists := fieldMap["number_value"]; exists { | ||
| gqlFloat := githubv4.Float(v) | ||
| input.NumberValue = &gqlFloat | ||
| hasValue = true | ||
| valueCount++ | ||
| } | ||
| } | ||
| if v, err := OptionalParam[string](fieldMap, "date_value"); err == nil && v != "" { | ||
| input.DateValue = githubv4.NewString(githubv4.String(v)) | ||
| hasValue = true | ||
| valueCount++ | ||
| } | ||
| if v, err := OptionalParam[string](fieldMap, "single_select_option_id"); err == nil && v != "" { | ||
| optionID := githubv4.ID(v) | ||
| input.SingleSelectOptionID = &optionID | ||
| hasValue = true | ||
| valueCount++ | ||
| } | ||
| if _, exists := fieldMap["delete"]; exists { | ||
| del, err := OptionalParam[bool](fieldMap, "delete") | ||
| if err == nil && del { | ||
| deleteVal := githubv4.Boolean(true) | ||
| input.Delete = &deleteVal | ||
| hasValue = true | ||
| valueCount++ | ||
| } |
There was a problem hiding this comment.
valueCount only increments when a value parses successfully (and for strings, is non-empty). As a result, requests that include multiple value keys can still slip through when one of the keys is present but invalid/empty (e.g., text_value:"" + number_value:42, or text_value:"hello" + number_value:"42"), which undermines the “exactly one key” validation and can also produce confusing errors. Consider counting based on key presence (e.g., check fieldMap for the key first) and returning a specific type/empty-value error when a provided key can’t be used, then enforcing exactly one among the provided keys (plus delete:true).
See below for a potential fix:
if _, exists := fieldMap["text_value"]; exists {
valueCount++
v, err := OptionalParam[string](fieldMap, "text_value")
if err != nil {
return utils.NewToolResultError("text_value must be a string"), nil, nil
}
if v == "" {
return utils.NewToolResultError("text_value must be a non-empty string"), nil, nil
}
input.TextValue = githubv4.NewString(githubv4.String(v))
}
if _, exists := fieldMap["number_value"]; exists {
valueCount++
v, err := OptionalParam[float64](fieldMap, "number_value")
if err != nil {
return utils.NewToolResultError("number_value must be a number"), nil, nil
}
gqlFloat := githubv4.Float(v)
input.NumberValue = &gqlFloat
}
if _, exists := fieldMap["date_value"]; exists {
valueCount++
v, err := OptionalParam[string](fieldMap, "date_value")
if err != nil {
return utils.NewToolResultError("date_value must be a string"), nil, nil
}
if v == "" {
return utils.NewToolResultError("date_value must be a non-empty string"), nil, nil
}
input.DateValue = githubv4.NewString(githubv4.String(v))
}
if _, exists := fieldMap["single_select_option_id"]; exists {
valueCount++
v, err := OptionalParam[string](fieldMap, "single_select_option_id")
if err != nil {
return utils.NewToolResultError("single_select_option_id must be a string"), nil, nil
}
if v == "" {
return utils.NewToolResultError("single_select_option_id must be a non-empty string"), nil, nil
}
optionID := githubv4.ID(v)
input.SingleSelectOptionID = &optionID
}
if _, exists := fieldMap["delete"]; exists {
valueCount++
del, err := OptionalParam[bool](fieldMap, "delete")
if err != nil {
return utils.NewToolResultError("delete must be a boolean"), nil, nil
}
if !del {
return utils.NewToolResultError("delete must be true when provided"), nil, nil
}
deleteVal := githubv4.Boolean(true)
input.Delete = &deleteVal
Comment on lines
+911
to
+947
| t.Run("multiple value keys returns error", func(t *testing.T) { | ||
| deps := BaseDeps{} | ||
| serverTool := GranularSetIssueFields(translations.NullTranslationHelper) | ||
| handler := serverTool.Handler(deps) | ||
|
|
||
| request := createMCPRequest(map[string]any{ | ||
| "owner": "owner", | ||
| "repo": "repo", | ||
| "issue_number": float64(5), | ||
| "fields": []any{ | ||
| map[string]any{"field_id": "FIELD_1", "text_value": "hello", "number_value": float64(42)}, | ||
| }, | ||
| }) | ||
| result, err := handler(ContextWithDeps(context.Background(), deps), &request) | ||
| require.NoError(t, err) | ||
| textContent := getTextResult(t, result) | ||
| assert.Contains(t, textContent.Text, "each field must have exactly one value") | ||
| }) | ||
|
|
||
| t.Run("value key with delete returns error", func(t *testing.T) { | ||
| deps := BaseDeps{} | ||
| serverTool := GranularSetIssueFields(translations.NullTranslationHelper) | ||
| handler := serverTool.Handler(deps) | ||
|
|
||
| request := createMCPRequest(map[string]any{ | ||
| "owner": "owner", | ||
| "repo": "repo", | ||
| "issue_number": float64(5), | ||
| "fields": []any{ | ||
| map[string]any{"field_id": "FIELD_1", "text_value": "hello", "delete": true}, | ||
| }, | ||
| }) | ||
| result, err := handler(ContextWithDeps(context.Background(), deps), &request) | ||
| require.NoError(t, err) | ||
| textContent := getTextResult(t, result) | ||
| assert.Contains(t, textContent.Text, "each field must have exactly one value") | ||
| }) |
There was a problem hiding this comment.
The new tests cover multiple valid value keys, but they don’t cover cases where a second value key is present but invalid/empty (which currently won’t increment valueCount and therefore won’t trigger the “exactly one value” error). Adding a test like text_value:"hello" + number_value:"42" (string) and/or text_value:"" + number_value:42 would catch this and protect the stricter validation behavior described in the PR.
SamMorrowDrums
pushed a commit
that referenced
this pull request
Apr 16, 2026
* 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tightens
set_issue_fieldsvalidation to reject ambiguous input where multiple value keys (or value +delete:true) are provided for a single field element.Why
Review feedback on #2338: the handler documented "exactly one" value key per field but only enforced "at least one", silently accepting ambiguous combinations like
text_value+number_valuethat would produce unpredictable GraphQL mutations.What changed
hasValueflag withvalueCountcounter; returns error when count > 1text_value+number_valueandtext_value+delete:trueMCP impact
Prompts tested (tool changes only)
Security / limits
Tool renaming
deprecated_tool_aliases.goNote: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
./script/lint./script/testDocs
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
golangci-lint.run/usr/bin/curl curl -sSfL REDACTED ux-amd64/pkg/too/tmp/go-build2054917994/b156/vet.cfg conf�� 0.1-go1.25.0.linux-amd64/src/run-I --global ux-amd64/pkg/tool/linux_amd64/vet credential.helpe/home/REDACTED/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25.0.linux-amd64/pkg/tool/linux_amd64/vet(dns block)raw.ghe.example.com/tmp/go-build1945429804/b366/oauth.test /tmp/go-build1945429804/b366/oauth.test -test.testlogfile=/tmp/go-build1945429804/b366/testlog.txt -test.paniconexit0 -test.timeout=10m0s -buildid kPj6BkeRJbvDEDQyF5jF/kPj6BkeRJbvDEDQyF5jF -goversion go1.25.0 -c=4 -race -nolocalimports -importcfg ortc�� 0.1-go1.25.0.linux-amd64/src/net-I ver/github-mcp-server/cmd/github/tmp/go-build1945429804/b160/ ux-amd64/pkg/tool/linux_amd64/vet ux-amd64/src/run/home/REDACTED/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25.0.linux-amd64/pkg/too-trimpath ux-amd64/src/net-unsafeptr=false ux-amd64/src/run-unreachable=false ux-amd64/pkg/too/tmp/go-build1945429804/b311/vet.cfg(dns block)raw.mycompanygithub.com/tmp/go-build1945429804/b404/utils.test /tmp/go-build1945429804/b404/utils.test -test.testlogfile=/tmp/go-build1945429804/b404/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1945429804/b330/vet.cfg g_.a t ux-amd64/pkg/tool/linux_amd64/vet pMQuNTHXA --global t ux-amd64/pkg/too/tmp/go-build1945429804/b302/vet.cfg 0333�� rg/toolchain@v0.0.1-go1.25.0.lin-p t ux-amd64/pkg/tool/linux_amd64/compile W0hyIh8Ll SQdUDQPIO ux-amd64/pkg/too/tmp/go-build1945429804/b343/_pkg_.a ux-amd64/pkg/too-trimpath(dns block)raw.myghe.com/tmp/go-build1945429804/b404/utils.test /tmp/go-build1945429804/b404/utils.test -test.testlogfile=/tmp/go-build1945429804/b404/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1945429804/b330/vet.cfg g_.a t ux-amd64/pkg/tool/linux_amd64/vet pMQuNTHXA --global t ux-amd64/pkg/too/tmp/go-build1945429804/b302/vet.cfg 0333�� rg/toolchain@v0.0.1-go1.25.0.lin-p t ux-amd64/pkg/tool/linux_amd64/compile W0hyIh8Ll SQdUDQPIO ux-amd64/pkg/too/tmp/go-build1945429804/b343/_pkg_.a ux-amd64/pkg/too-trimpath(dns block)raw.notgithub.com/tmp/go-build1945429804/b404/utils.test /tmp/go-build1945429804/b404/utils.test -test.testlogfile=/tmp/go-build1945429804/b404/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1945429804/b330/vet.cfg g_.a t ux-amd64/pkg/tool/linux_amd64/vet pMQuNTHXA --global t ux-amd64/pkg/too/tmp/go-build1945429804/b302/vet.cfg 0333�� rg/toolchain@v0.0.1-go1.25.0.lin-p t ux-amd64/pkg/tool/linux_amd64/compile W0hyIh8Ll SQdUDQPIO ux-amd64/pkg/too/tmp/go-build1945429804/b343/_pkg_.a ux-amd64/pkg/too-trimpath(dns block)If you need me to access, download, or install something from one of these locations, you can either: