Polish issue_fields write path: clearer empty-array error, drop dead field, add combined test

Three small fixes from code review:

1. optionalIssueWriteFields now returns a targeted error when
   field_option_names is present but empty, pointing the caller at
   delete:true. The generic 'must specify one of' message was
   confusing because the caller did specify field_option_names.

2. Drop the dead MultiSelectOptionIDs field from IssueFieldValueFilter.
   resolveFieldFilters only ever sets MultiSelectOptionValues; the IDs
   variant was declared but never wired.

3. Add Test_UpdateIssue_DeleteAndSetFieldsInSameCall covering a single
   UpdateIssue call that deletes one multi_select field and sets another
   field in the same request. Asserts the REST PATCH carries only the
   kept set (no null clearing) and that the deleteIssueFieldValue
   mutation fires for the deleted field with the right node IDs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: owenniblock

pkg/github/issues.go

@@ -295,7 +295,8 @@ func optionalIssueWriteFields(args map[string]any) ([]issueWriteFieldInput, erro
 		}
 
 		var fieldOptionNames []string
-		if rawNames, hasNames := itemMap["field_option_names"]; hasNames && rawNames != nil {
+		_, hasNamesKey := itemMap["field_option_names"]
+		if rawNames := itemMap["field_option_names"]; hasNamesKey && rawNames != nil {
 			switch v := rawNames.(type) {
 			case []string:
 				fieldOptionNames = v
@@ -341,6 +342,9 @@ func optionalIssueWriteFields(args map[string]any) ([]issueWriteFieldInput, erro
 			setters++
 		}
 		if setters == 0 {
+			if hasNamesKey {
+				return nil, fmt.Errorf("issue field %q has empty field_option_names — use 'delete: true' to clear the field", fieldName)
+			}
 			return nil, fmt.Errorf("issue field %q must specify one of value, field_option_name, or field_option_names", fieldName)
 		}
 		if setters > 1 {
@@ -688,15 +692,14 @@ type ListIssuesQueryTypeWithLabelsWithSince struct {
 
 // IssueFieldValueFilter mirrors the GraphQL IssueFieldValueFilter input. Exactly one typed value
 // field should be set per filter (the monolith resolver rejects multiple). For multi_select fields,
-// MultiSelectOptionValues / MultiSelectOptionIDs use all-of (AND) semantics: an issue matches only
-// if it has every listed option set on the field. To match any-of, send separate filter calls.
+// MultiSelectOptionValues uses all-of (AND) semantics: an issue matches only if it has every listed
+// option set on the field. To match any-of, send separate filter calls.
 type IssueFieldValueFilter struct {
 	FieldName               githubv4.String    `json:"fieldName"`
 	TextValue               *githubv4.String   `json:"textValue,omitempty"`
 	DateValue               *githubv4.String   `json:"dateValue,omitempty"`
 	NumberValue             *githubv4.Float    `json:"numberValue,omitempty"`
 	SingleSelectOptionValue *githubv4.String   `json:"singleSelectOptionValue,omitempty"`
-	MultiSelectOptionIDs    *[]githubv4.ID     `json:"multiSelectOptionIds,omitempty"`
 	MultiSelectOptionValues *[]githubv4.String `json:"multiSelectOptionValues,omitempty"`
 }
 

pkg/github/issues_multiselect_test.go

@@ -243,9 +243,10 @@ func Test_optionalIssueWriteFields_MultiSelect(t *testing.T) {
 			},
 		})
 		require.Error(t, err)
-		// An empty slice is treated the same as "neither value/option provided" — the
-		// caller has to pick something. delete:true is the right way to clear a field.
-		assert.Contains(t, err.Error(), "must specify one of")
+		// An empty slice is a common "clear the field" guess; nudge callers to delete:true
+		// so the GraphQL deletion mutation runs instead of an unintentional no-op.
+		assert.Contains(t, err.Error(), "empty field_option_names")
+		assert.Contains(t, err.Error(), "delete: true")
 	})
 
 	t.Run("rejects field_option_names + value combo", func(t *testing.T) {
@@ -528,3 +529,144 @@ func Test_UpdateIssue_DeleteFieldValueRunsGraphQLMutation(t *testing.T) {
 }
 
 // (intentionally no further helpers)
+
+// Test_UpdateIssue_DeleteAndSetFieldsInSameCall verifies that a single UpdateIssue
+// can delete one issue field while setting another in the same request: the REST PATCH
+// must carry only the set operation (no null clearing for the deleted field), and the
+// deleteIssueFieldValue GraphQL mutation must fire for the deletion.
+func Test_UpdateIssue_DeleteAndSetFieldsInSameCall(t *testing.T) {
+	t.Parallel()
+
+	mockIssue := &gogithub.Issue{
+		Number:  gogithub.Ptr(42),
+		Title:   gogithub.Ptr("Test issue"),
+		State:   gogithub.Ptr("open"),
+		HTMLURL: gogithub.Ptr("https://github.com/owner/repo/issues/42"),
+	}
+
+	// Verify the PATCH body carries only the kept set operation. Crucially, it must
+	// NOT include the deleted Components field (omitting != sending null), and it
+	// must NOT double-clear the deleted field — the GraphQL mutation owns that path.
+	restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
+		PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{
+			"issue_field_values": []any{
+				map[string]any{"field_id": float64(202), "value": "P2"},
+			},
+		}).andThen(
+			mockResponse(t, http.StatusOK, mockIssue),
+		),
+	}))
+
+	// Existing issue carries both fields. After merge + deletion filter, only the
+	// Priority "set" entry should remain in the REST payload.
+	existingFieldsResponse := githubv4mock.DataResponse(map[string]any{
+		"repository": map[string]any{
+			"issue": map[string]any{
+				"issueFieldValues": map[string]any{
+					"nodes": []any{
+						map[string]any{
+							"__typename": "IssueFieldMultiSelectValue",
+							"field": map[string]any{
+								"fullDatabaseId": "101",
+								"name":           "Components",
+							},
+							"options": []any{
+								map[string]any{"name": "Auth"},
+								map[string]any{"name": "Billing"},
+							},
+						},
+						map[string]any{
+							"__typename": "IssueFieldSingleSelectValue",
+							"field": map[string]any{
+								"fullDatabaseId": "202",
+								"name":           "Priority",
+							},
+							"value": "P1",
+						},
+					},
+				},
+			},
+		},
+	})
+
+	issueIDResponse := githubv4mock.DataResponse(map[string]any{
+		"repository": map[string]any{
+			"issue": map[string]any{"id": "I_node_42"},
+		},
+	})
+
+	// The matcher fails the test if the mutation doesn't fire with the right
+	// (issueId, fieldId) pair — proves the delete side of the combined update ran.
+	deleteResponse := githubv4mock.DataResponse(map[string]any{
+		"deleteIssueFieldValue": map[string]any{
+			"issue": map[string]any{"number": 42},
+		},
+	})
+
+	gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(
+		githubv4mock.NewQueryMatcher(
+			struct {
+				Repository struct {
+					Issue struct {
+						IssueFieldValues struct {
+							Nodes []IssueFieldValueFragment
+						} `graphql:"issueFieldValues(first: 25)"`
+					} `graphql:"issue(number: $number)"`
+				} `graphql:"repository(owner: $owner, name: $repo)"`
+			}{},
+			map[string]any{
+				"owner":  githubv4.String("owner"),
+				"repo":   githubv4.String("repo"),
+				"number": githubv4.Int(42),
+			},
+			existingFieldsResponse,
+		),
+		githubv4mock.NewQueryMatcher(
+			struct {
+				Repository struct {
+					Issue struct {
+						ID githubv4.ID
+					} `graphql:"issue(number: $issueNumber)"`
+				} `graphql:"repository(owner: $owner, name: $repo)"`
+			}{},
+			map[string]any{
+				"owner":       githubv4.String("owner"),
+				"repo":        githubv4.String("repo"),
+				"issueNumber": githubv4.Int(42),
+			},
+			issueIDResponse,
+		),
+		githubv4mock.NewMutationMatcher(
+			struct {
+				DeleteIssueFieldValue struct {
+					Issue struct {
+						Number githubv4.Int
+					}
+				} `graphql:"deleteIssueFieldValue(input: $input)"`
+			}{},
+			DeleteIssueFieldValueInput{
+				IssueID: githubv4.ID("I_node_42"),
+				FieldID: githubv4.ID("IFMS_node_101"),
+			},
+			nil,
+			deleteResponse,
+		),
+	))
+
+	result, err := UpdateIssue(
+		context.Background(),
+		restClient,
+		gqlClient,
+		"owner", "repo", 42,
+		"", "", nil, nil, 0, "",
+		[]*gogithub.IssueRequestFieldValue{
+			{FieldID: 202, Value: "P2"},
+		},
+		[]fieldDeletion{{DatabaseID: 101, NodeID: githubv4.ID("IFMS_node_101")}},
+		"", "", 0,
+	)
+	require.NoError(t, err)
+	if result.IsError {
+		t.Fatalf("expected non-error result, got: %s", getTextResult(t, result).Text)
+	}
+}