Skip to content

Commit cc6be4e

Browse files
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>
1 parent 436dfe9 commit cc6be4e

3 files changed

Lines changed: 55 additions & 17 deletions

File tree

pkg/github/__toolsnaps__/issue_write.snap

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,19 +37,19 @@
3737
"additionalProperties": false,
3838
"properties": {
3939
"delete": {
40-
"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.",
40+
"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.",
4141
"type": "boolean"
4242
},
4343
"field_name": {
4444
"description": "Issue field name (case-insensitive). Must match a field returned by list_issue_fields for this repository or its organization.",
4545
"type": "string"
4646
},
4747
"field_option_name": {
48-
"description": "Option name for single-select fields. Validated against the field's options before the API call. Cannot be combined with 'value' or 'delete'.",
48+
"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'.",
4949
"type": "string"
5050
},
5151
"value": {
52-
"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'.",
52+
"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'.",
5353
"type": [
5454
"string",
5555
"number",

pkg/github/issues.go

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,10 @@ func optionalIssueWriteFields(args map[string]any) ([]issueWriteFieldInput, erro
265265
return nil, err
266266
}
267267

268-
deleteField, _ := OptionalParam[bool](itemMap, "delete")
268+
deleteField, err := OptionalParam[bool](itemMap, "delete")
269+
if err != nil {
270+
return nil, err
271+
}
269272
value, hasValue := itemMap["value"]
270273
if hasValue && value == nil {
271274
return nil, fmt.Errorf("value cannot be null for field %q", fieldName)
@@ -2277,20 +2280,19 @@ Options are:
22772280
Description: "Value to set. Use for text, number, and date fields " +
22782281
"(date as YYYY-MM-DD). For single-select fields, prefer " +
22792282
"'field_option_name' so the option is validated before the API " +
2280-
"call. Cannot be combined with 'field_option_name' or 'delete'.",
2283+
"call. Cannot be combined with 'field_option_name' or 'delete: true'.",
22812284
},
22822285
"field_option_name": {
22832286
Type: "string",
22842287
Description: "Option name for single-select fields. Validated against " +
22852288
"the field's options before the API call. Cannot be combined with " +
2286-
"'value' or 'delete'.",
2289+
"'value' or 'delete: true'.",
22872290
},
22882291
"delete": {
22892292
Type: "boolean",
22902293
Description: "Set to true to clear this field's current value on the " +
2291-
"issue. Cannot be combined with 'value' or 'field_option_name'. " +
2292-
"Omit this property, or set it to false, to leave the field's " +
2293-
"current value unchanged.",
2294+
"issue. When false or omitted, this property is ignored. Cannot " +
2295+
"be true when 'value' or 'field_option_name' is provided.",
22942296
},
22952297
},
22962298
Required: []string{"field_name"},

pkg/github/issues_test.go

Lines changed: 44 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1823,8 +1823,8 @@ func Test_CreateIssue(t *testing.T) {
18231823
"repo": "repo",
18241824
"title": "Issue with fields",
18251825
"issue_fields": []any{
1826-
map[string]any{"field_name": "Priority", "field_option_name": "P1"},
1827-
map[string]any{"field_name": "Customer", "value": "Acme"},
1826+
map[string]any{"field_name": "Priority", "field_option_name": "P1", "delete": false},
1827+
map[string]any{"field_name": "Customer", "value": "Acme", "delete": false},
18281828
},
18291829
},
18301830
expectError: false,
@@ -1867,6 +1867,21 @@ func Test_CreateIssue(t *testing.T) {
18671867
expectError: false,
18681868
expectedErrMsg: "cannot specify both value and field_option_name",
18691869
},
1870+
{
1871+
name: "issue_fields rejects delete true with value",
1872+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}),
1873+
requestArgs: map[string]any{
1874+
"method": "create",
1875+
"owner": "owner",
1876+
"repo": "repo",
1877+
"title": "Invalid fields",
1878+
"issue_fields": []any{
1879+
map[string]any{"field_name": "Start date", "value": "2026-08-14", "delete": true},
1880+
},
1881+
},
1882+
expectError: false,
1883+
expectedErrMsg: "cannot specify 'delete' together with 'value' or 'field_option_name'",
1884+
},
18701885
}
18711886

18721887
for _, tc := range tests {
@@ -2098,11 +2113,17 @@ func Test_issueWriteHasNonFormParams(t *testing.T) {
20982113
}
20992114
}
21002115

2101-
// Test_optionalIssueWriteFields covers parsing of issue_write's issue_fields
2102-
// items. The delete:false cases matter because the schema deliberately does not
2103-
// constrain 'delete' to a single value: clients that populate every property of
2104-
// a schema need a way to say "not deleting", and false must be a no-op that
2105-
// falls through to the normal value path.
2116+
func Test_IssueWriteIssueFieldsDeleteSchema(t *testing.T) {
2117+
t.Parallel()
2118+
2119+
inputSchema := IssueWrite(translations.NullTranslationHelper).Tool.InputSchema.(*jsonschema.Schema)
2120+
deleteSchema := inputSchema.Properties["issue_fields"].Items.Properties["delete"]
2121+
2122+
assert.Equal(t, "boolean", deleteSchema.Type)
2123+
assert.Empty(t, deleteSchema.Enum)
2124+
assert.Contains(t, deleteSchema.Description, "When false or omitted, this property is ignored")
2125+
}
2126+
21062127
func Test_optionalIssueWriteFields(t *testing.T) {
21072128
t.Parallel()
21082129

@@ -2113,10 +2134,15 @@ func Test_optionalIssueWriteFields(t *testing.T) {
21132134
wantErr string
21142135
}{
21152136
{
2116-
name: "delete false alongside a value sets the value",
2137+
name: "delete false alongside a value is ignored",
21172138
item: map[string]any{"field_name": "Start date", "value": "2026-08-14", "delete": false, "field_option_name": ""},
21182139
want: issueWriteFieldInput{FieldName: "Start date", Value: "2026-08-14"},
21192140
},
2141+
{
2142+
name: "delete false alongside field_option_name is ignored",
2143+
item: map[string]any{"field_name": "Priority", "field_option_name": "High", "delete": false},
2144+
want: issueWriteFieldInput{FieldName: "Priority", FieldOptionName: "High"},
2145+
},
21202146
{
21212147
name: "delete true alone clears the field",
21222148
item: map[string]any{"field_name": "Start date", "delete": true},
@@ -2132,11 +2158,21 @@ func Test_optionalIssueWriteFields(t *testing.T) {
21322158
item: map[string]any{"field_name": "Start date", "value": "2026-08-14", "delete": true},
21332159
wantErr: "cannot specify 'delete' together with 'value' or 'field_option_name'",
21342160
},
2161+
{
2162+
name: "delete true with field_option_name is rejected",
2163+
item: map[string]any{"field_name": "Priority", "field_option_name": "High", "delete": true},
2164+
wantErr: "cannot specify 'delete' together with 'value' or 'field_option_name'",
2165+
},
21352166
{
21362167
name: "delete false with nothing to set is rejected",
21372168
item: map[string]any{"field_name": "Start date", "delete": false},
21382169
wantErr: "must specify either value or field_option_name",
21392170
},
2171+
{
2172+
name: "delete with invalid type is rejected",
2173+
item: map[string]any{"field_name": "Start date", "delete": "false"},
2174+
wantErr: "parameter delete is not of type bool",
2175+
},
21402176
}
21412177

21422178
for _, tc := range tests {

0 commit comments

Comments
 (0)