From 239406d4545f57f9e528c9d17716b0c64c5a0286 Mon Sep 17 00:00:00 2001 From: Travis Gockel Date: Fri, 14 Aug 2026 18:04:15 -0600 Subject: [PATCH 1/2] fix(issues): allow delete:false in issue_write issue_fields The `delete` property of issue_write's issue_fields items was declared with Enum: []any{true}, making true its only legal value. The property is optional, but a client that fills every property of a schema -- common, since OpenAI-style strict function calling requires every property to appear in `required` -- had no way to express "not deleting this field": there is no false in the enum and no null in the type. `value` offers no alternative either, being typed ["string","number","boolean"] with no null. The MCP Go SDK validates arguments against the resolved input schema before the handler runs, so delete: false was rejected at schema validation and never reached optionalIssueWriteFields. Such clients sent delete: true alongside a value instead and hit the handler's mutual-exclusion check, so issue_write could never set an issue field for them. Remove the enum so false is a legal no-op, and document that omitting the property or setting it to false leaves the field unchanged. No handler change is needed: the code already branches on `if deleteField`, so false falls through to the normal value path, and the mutual-exclusion check for delete: true still applies. Add tests for optionalIssueWriteFields, which had none. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/github/__toolsnaps__/issue_write.snap | 5 +- pkg/github/issues.go | 5 +- pkg/github/issues_test.go | 57 +++++++++++++++++++++++ 3 files changed, 61 insertions(+), 6 deletions(-) diff --git a/pkg/github/__toolsnaps__/issue_write.snap b/pkg/github/__toolsnaps__/issue_write.snap index 10efb6c6df..5211e7cb29 100644 --- a/pkg/github/__toolsnaps__/issue_write.snap +++ b/pkg/github/__toolsnaps__/issue_write.snap @@ -37,10 +37,7 @@ "additionalProperties": false, "properties": { "delete": { - "description": "Set to true to clear this field's current value on the issue. Cannot be combined with 'value' or 'field_option_name'.", - "enum": [ - true - ], + "description": "Set to true to clear this field's current value on the issue. Cannot be combined with 'value' or 'field_option_name'. Omit this property, or set it to false, to leave the field's current value unchanged.", "type": "boolean" }, "field_name": { diff --git a/pkg/github/issues.go b/pkg/github/issues.go index b53ee6d596..df6676b32f 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -2447,9 +2447,10 @@ Options are: }, "delete": { Type: "boolean", - Enum: []any{true}, Description: "Set to true to clear this field's current value on the " + - "issue. Cannot be combined with 'value' or 'field_option_name'.", + "issue. Cannot be combined with 'value' or 'field_option_name'. " + + "Omit this property, or set it to false, to leave the field's " + + "current value unchanged.", }, }, Required: []string{"field_name"}, diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index fb747a1283..0b2cccf11b 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -2115,6 +2115,63 @@ func Test_issueWriteHasNonFormParams(t *testing.T) { } } +// Test_optionalIssueWriteFields covers parsing of issue_write's issue_fields +// items. The delete:false cases matter because the schema deliberately does not +// constrain 'delete' to a single value: clients that populate every property of +// a schema need a way to say "not deleting", and false must be a no-op that +// falls through to the normal value path. +func Test_optionalIssueWriteFields(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + item map[string]any + want issueWriteFieldInput + wantErr string + }{ + { + name: "delete false alongside a value sets the value", + item: map[string]any{"field_name": "Start date", "value": "2026-08-14", "delete": false, "field_option_name": ""}, + want: issueWriteFieldInput{FieldName: "Start date", Value: "2026-08-14"}, + }, + { + name: "delete true alone clears the field", + item: map[string]any{"field_name": "Start date", "delete": true}, + want: issueWriteFieldInput{FieldName: "Start date", Delete: true}, + }, + { + name: "delete omitted with field_option_name", + item: map[string]any{"field_name": "Priority", "field_option_name": "High"}, + want: issueWriteFieldInput{FieldName: "Priority", FieldOptionName: "High"}, + }, + { + name: "delete true with a value is rejected", + item: map[string]any{"field_name": "Start date", "value": "2026-08-14", "delete": true}, + wantErr: "cannot specify 'delete' together with 'value' or 'field_option_name'", + }, + { + name: "delete false with nothing to set is rejected", + item: map[string]any{"field_name": "Start date", "delete": false}, + wantErr: "must specify either value or field_option_name", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := optionalIssueWriteFields(map[string]any{"issue_fields": []any{tc.item}}) + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + return + } + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, tc.want, got[0]) + }) + } +} + // Test_issueWriteSchemaClassification fails when a schema property is added // without classifying it as either form-resendable (issueWriteFormParams) or // known-non-form (knownNonForm below). Without this guard, an unclassified From 275ad336139e05da178a5adab362add0979a535c Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Mon, 17 Aug 2026 15:21:29 +0200 Subject: [PATCH 2/2] fix(issues): preserve delete field semantics Clarify that delete:false is ignored, retain mutual exclusion for delete:true, and reject invalid delete types. Add focused schema and handler regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- pkg/github/__toolsnaps__/issue_write.snap | 6 +-- pkg/github/issues.go | 14 +++--- pkg/github/issues_test.go | 52 +++++++++++++++++++---- 3 files changed, 55 insertions(+), 17 deletions(-) diff --git a/pkg/github/__toolsnaps__/issue_write.snap b/pkg/github/__toolsnaps__/issue_write.snap index 5211e7cb29..d4968c4f2f 100644 --- a/pkg/github/__toolsnaps__/issue_write.snap +++ b/pkg/github/__toolsnaps__/issue_write.snap @@ -37,7 +37,7 @@ "additionalProperties": false, "properties": { "delete": { - "description": "Set to true to clear this field's current value on the issue. Cannot be combined with 'value' or 'field_option_name'. Omit this property, or set it to false, to leave the field's current value unchanged.", + "description": "Set to true to clear this field's current value on the issue. When false or omitted, this property is ignored. Cannot be true when 'value' or 'field_option_name' is provided.", "type": "boolean" }, "field_name": { @@ -45,11 +45,11 @@ "type": "string" }, "field_option_name": { - "description": "Option name for single-select fields. Validated against the field's options before the API call. Cannot be combined with 'value' or 'delete'.", + "description": "Option name for single-select fields. Validated against the field's options before the API call. Cannot be combined with 'value' or 'delete: true'.", "type": "string" }, "value": { - "description": "Value to set. Use for text, number, and date fields (date as YYYY-MM-DD). For single-select fields, prefer 'field_option_name' so the option is validated before the API call. Cannot be combined with 'field_option_name' or 'delete'.", + "description": "Value to set. Use for text, number, and date fields (date as YYYY-MM-DD). For single-select fields, prefer 'field_option_name' so the option is validated before the API call. Cannot be combined with 'field_option_name' or 'delete: true'.", "type": [ "string", "number", diff --git a/pkg/github/issues.go b/pkg/github/issues.go index df6676b32f..b8b24070a3 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -265,7 +265,10 @@ func optionalIssueWriteFields(args map[string]any) ([]issueWriteFieldInput, erro return nil, err } - deleteField, _ := OptionalParam[bool](itemMap, "delete") + deleteField, err := OptionalParam[bool](itemMap, "delete") + if err != nil { + return nil, err + } value, hasValue := itemMap["value"] if hasValue && value == nil { return nil, fmt.Errorf("value cannot be null for field %q", fieldName) @@ -2437,20 +2440,19 @@ Options are: Description: "Value to set. Use for text, number, and date fields " + "(date as YYYY-MM-DD). For single-select fields, prefer " + "'field_option_name' so the option is validated before the API " + - "call. Cannot be combined with 'field_option_name' or 'delete'.", + "call. Cannot be combined with 'field_option_name' or 'delete: true'.", }, "field_option_name": { Type: "string", Description: "Option name for single-select fields. Validated against " + "the field's options before the API call. Cannot be combined with " + - "'value' or 'delete'.", + "'value' or 'delete: true'.", }, "delete": { Type: "boolean", Description: "Set to true to clear this field's current value on the " + - "issue. Cannot be combined with 'value' or 'field_option_name'. " + - "Omit this property, or set it to false, to leave the field's " + - "current value unchanged.", + "issue. When false or omitted, this property is ignored. Cannot " + + "be true when 'value' or 'field_option_name' is provided.", }, }, Required: []string{"field_name"}, diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 0b2cccf11b..fcd74bf2e3 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1840,8 +1840,8 @@ func Test_CreateIssue(t *testing.T) { "repo": "repo", "title": "Issue with fields", "issue_fields": []any{ - map[string]any{"field_name": "Priority", "field_option_name": "P1"}, - map[string]any{"field_name": "Customer", "value": "Acme"}, + map[string]any{"field_name": "Priority", "field_option_name": "P1", "delete": false}, + map[string]any{"field_name": "Customer", "value": "Acme", "delete": false}, }, }, expectError: false, @@ -1884,6 +1884,21 @@ func Test_CreateIssue(t *testing.T) { expectError: false, expectedErrMsg: "cannot specify both value and field_option_name", }, + { + name: "issue_fields rejects delete true with value", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}), + requestArgs: map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Invalid fields", + "issue_fields": []any{ + map[string]any{"field_name": "Start date", "value": "2026-08-14", "delete": true}, + }, + }, + expectError: false, + expectedErrMsg: "cannot specify 'delete' together with 'value' or 'field_option_name'", + }, } for _, tc := range tests { @@ -2115,11 +2130,17 @@ func Test_issueWriteHasNonFormParams(t *testing.T) { } } -// Test_optionalIssueWriteFields covers parsing of issue_write's issue_fields -// items. The delete:false cases matter because the schema deliberately does not -// constrain 'delete' to a single value: clients that populate every property of -// a schema need a way to say "not deleting", and false must be a no-op that -// falls through to the normal value path. +func Test_IssueWriteIssueFieldsDeleteSchema(t *testing.T) { + t.Parallel() + + inputSchema := IssueWrite(translations.NullTranslationHelper).Tool.InputSchema.(*jsonschema.Schema) + deleteSchema := inputSchema.Properties["issue_fields"].Items.Properties["delete"] + + assert.Equal(t, "boolean", deleteSchema.Type) + assert.Empty(t, deleteSchema.Enum) + assert.Contains(t, deleteSchema.Description, "When false or omitted, this property is ignored") +} + func Test_optionalIssueWriteFields(t *testing.T) { t.Parallel() @@ -2130,10 +2151,15 @@ func Test_optionalIssueWriteFields(t *testing.T) { wantErr string }{ { - name: "delete false alongside a value sets the value", + name: "delete false alongside a value is ignored", item: map[string]any{"field_name": "Start date", "value": "2026-08-14", "delete": false, "field_option_name": ""}, want: issueWriteFieldInput{FieldName: "Start date", Value: "2026-08-14"}, }, + { + name: "delete false alongside field_option_name is ignored", + item: map[string]any{"field_name": "Priority", "field_option_name": "High", "delete": false}, + want: issueWriteFieldInput{FieldName: "Priority", FieldOptionName: "High"}, + }, { name: "delete true alone clears the field", item: map[string]any{"field_name": "Start date", "delete": true}, @@ -2149,11 +2175,21 @@ func Test_optionalIssueWriteFields(t *testing.T) { item: map[string]any{"field_name": "Start date", "value": "2026-08-14", "delete": true}, wantErr: "cannot specify 'delete' together with 'value' or 'field_option_name'", }, + { + name: "delete true with field_option_name is rejected", + item: map[string]any{"field_name": "Priority", "field_option_name": "High", "delete": true}, + wantErr: "cannot specify 'delete' together with 'value' or 'field_option_name'", + }, { name: "delete false with nothing to set is rejected", item: map[string]any{"field_name": "Start date", "delete": false}, wantErr: "must specify either value or field_option_name", }, + { + name: "delete with invalid type is rejected", + item: map[string]any{"field_name": "Start date", "delete": "false"}, + wantErr: "parameter delete is not of type bool", + }, } for _, tc := range tests {