Skip to content

Enforce exactly one value key per field in set_issue_fields #2339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattdholloway merged 2 commits into from
Apr 16, 2026
+49 −8
Merged

Enforce exactly one value key per field in set_issue_fields #2339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ac1762
Initial plan
Copilot Apr 16, 2026
007716b
Enforce exactly one value key per field in set_issue_fields and add t…
Copilot Apr 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions pkg/github/granular_tools_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -907,4 +907,42 @@ func TestGranularSetIssueFields(t *testing.T) {
textContent := getTextResult(t, result)
assert.Contains(t, textContent.Text, "each field must have a value")
})

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")
})
Comment on lines +911 to +947
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot generated this review using guidance from repository custom instructions.
}
19 changes: 11 additions & 8 deletions pkg/github/issues_granular.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,41 +731,44 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv
FieldID: githubv4.ID(fieldID),
}

// Check for exactly one value type (or delete)
hasValue := false
// 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++
}
Comment on lines +734 to 763
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.
}

if !hasValue {
if valueCount == 0 {
return utils.NewToolResultError("each field must have a value (text_value, number_value, date_value, single_select_option_id) or delete: true"), nil, nil
}
if valueCount > 1 {
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
}

issueFields = append(issueFields, input)
}
Expand Down
Loading