Fix delete:true on issue fields by calling deleteIssueFieldValue mutation (#2755)

* Fall back to REST DELETE when the PATCH can't carry an issue field clear

The dotcom REST update endpoint uses set semantics for issue_field_values:
sending {"issue_field_values": [...]} overwrites the whole list, and
anything not in the new list is treated as a deletion. UpdateIssue
already exploits this via merge-and-filter — delete:true on a field
removes it from the kept list and the server clears it as a side effect.

That breaks for one specific case: when the kept list ends up empty
(e.g. you're deleting the only remaining field value, or every field in
one call), go-github's omitempty tag on
`IssueRequest.IssueFieldValues` strips the empty slice from the JSON
body. The dotcom REST handler's top-level
`if data.include?(ISSUE_FIELD_VALUES)` guard then short-circuits — the
key isn't in the payload, so the whole block is skipped and the field
keeps its old value. The MCP tool returns success regardless, so a
coding agent would happily report the field as cleared.

Detect this case in UpdateIssue and fall back to the dedicated REST
DELETE endpoint per field: DELETE /repos/{owner}/{repo}/issues/{number}/
issue-field-values/{field_id}. The endpoint is idempotent, takes the
integer field ID (no GraphQL node ID needed), and is the same one the
public OpenAPI documents.

Empirical confirmation on #2756:

- Set Priority=P1 via REST
- Before the fix: MCP issue_write delete:true returns success, PATCH body
  on the wire is literally {}, Priority remains P1 (silent no-op).
- After the fix: MCP issue_write delete:true returns success, PATCH body
  is still {}, but the follow-up DELETE clears the field. Priority is
  cleared as expected.

Three new tests in issues_delete_test.go cover:

- The omitempty contract (so we know if go-github ever drops the tag)
- The 1-of-1 fallback path (PATCH body has no issue_field_values, DELETE
  fires)
- The N-1 set-semantics path (kept list is non-empty, PATCH alone
  handles it, no DELETE call)

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

* Address review: handle delete-absent-field and DELETE partial failures

Two issues surfaced in code review of the original delete fix:

1. Asking to delete a field that isn't set on the issue used to be a
   silent no-op (PATCH body is {} after omitempty stripping, server
   skips the issue_field_values block, tool returns success). The fix
   turned it into a hard error because the fallback DELETE loop fires
   unconditionally for every field ID in fieldIDsToDelete, and the
   dotcom DELETE endpoint returns 404 when the field has no value to
   delete. This breaks idempotent 'ensure field X is cleared' callers
   that may re-run the same delete on a field that's already been
   cleared.

   Fix: before queueing the fallback DELETEs, filter
   fallbackDeleteFieldIDs down to IDs that actually appeared in the
   existing field values. This preserves the pre-fix silent-no-op
   behaviour and avoids a guaranteed 404.

2. The DELETE loop returned on the first error. If a caller asks to
   clear three fields and the second fails (transient 5xx, rate limit,
   etc.), the first is gone, the third is never attempted, and the
   user gets a generic error with no indication of which field failed
   or which had already succeeded. This is unrecoverable from the
   caller side.

   Fix: continue on per-field errors, accumulate the failed and
   succeeded IDs, and return a single aggregated error naming both
   sets so callers can retry the right fields.

Tests:

- Test_UpdateIssue_DeleteAbsentFieldIsNoOp: existing field values is
  empty, fieldIDsToDelete=[101]. Asserts no DELETE call fires (the
  mock returns 404 if it does, which would fail the test) and the
  tool returns success.

- Test_UpdateIssue_DeleteFallbackContinuesOnPartialFailure: three
  fields exist, all three are deleted. Mock returns 500 for the
  middle DELETE, 204 for the others. Asserts all three DELETEs
  fired (the middle failure didn't short-circuit the third) and the
  error result names failed=[202] and cleared=[101 303].

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

* Slim down code comments in delete-fix region

Trim the doc comments on UpdateIssue's omitempty-trap region + test
file to the bare minimum needed to understand the non-obvious behaviour
(why the DELETE fallback exists, why we filter to existing IDs, why
errors are aggregated). Drop restated context and step-by-step
explanations.

No behaviour change.

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

---------

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

Author: owenniblock

pkg/github/helper_test.go

@@ -66,6 +66,7 @@ const (
 	PostReposIssuesSubIssuesByOwnerByRepoByIssueNumber          = "POST /repos/{owner}/{repo}/issues/{issue_number}/sub_issues"
 	DeleteReposIssuesSubIssueByOwnerByRepoByIssueNumber         = "DELETE /repos/{owner}/{repo}/issues/{issue_number}/sub_issue"
 	PatchReposIssuesSubIssuesPriorityByOwnerByRepoByIssueNumber = "PATCH /repos/{owner}/{repo}/issues/{issue_number}/sub_issues/priority"
+	DeleteReposIssuesIssueFieldValueByOwnerByRepoByIssueNumber  = "DELETE /repos/{owner}/{repo}/issues/{issue_number}/issue-field-values/{issue_field_id}"
 
 	// Pull request endpoints
 	GetReposPullsByOwnerByRepo                                = "GET /repos/{owner}/{repo}/pulls"

pkg/github/issues.go

@@ -2148,10 +2148,13 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4
 		issueRequest.Type = github.Ptr(issueType)
 	}
 
+	// Field IDs to clear via DELETE after the PATCH. See the post-PATCH loop
+	// for why we can't just rely on REST set semantics.
+	var fallbackDeleteFieldIDs []int64
+
 	if len(issueFieldValues) > 0 || len(fieldIDsToDelete) > 0 {
-		// The REST update endpoint uses "set" semantics — it overwrites all existing
-		// field values with whatever is sent. Fetch the current values first, merge in
-		// the new values, then remove any explicitly deleted fields.
+		// REST PATCH uses set semantics, so fetch existing values, merge in
+		// the new ones, then drop anything explicitly deleted.
 		existing, err := fetchExistingIssueFieldValues(ctx, gqlClient, owner, repo, issueNumber)
 		if err != nil {
 			return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to fetch existing issue field values", err), nil
@@ -2170,7 +2173,22 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4
 			}
 			merged = kept
 		}
-		issueRequest.IssueFieldValues = merged
+		if len(merged) == 0 && len(fieldIDsToDelete) > 0 {
+			// Only queue DELETEs for fields actually present — the endpoint
+			// returns 404 otherwise, and "delete a field that isn't set" should
+			// stay a no-op (callers often invoke delete:true idempotently).
+			existingIDs := make(map[int64]bool, len(existing))
+			for _, e := range existing {
+				existingIDs[e.FieldID] = true
+			}
+			for _, id := range fieldIDsToDelete {
+				if existingIDs[id] {
+					fallbackDeleteFieldIDs = append(fallbackDeleteFieldIDs, id)
+				}
+			}
+		} else {
+			issueRequest.IssueFieldValues = merged
+		}
 	}
 
 	updatedIssue, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueRequest)
@@ -2191,6 +2209,43 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4
 		return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to update issue", resp, body), nil
 	}
 
+	// Per-field DELETE fallback. The PATCH can't clear field values when the
+	// merged set is empty — go-github's `omitempty` strips the empty slice
+	// and the dotcom REST handler skips its issue_field_values block when the
+	// key is absent. Errors are aggregated (not short-circuited) so callers
+	// can see which fields succeeded and which need retry.
+	if len(fallbackDeleteFieldIDs) > 0 {
+		var failedIDs, succeededIDs []int64
+		var firstFailureErr error
+		var firstFailureResp *github.Response
+		for _, fieldID := range fallbackDeleteFieldIDs {
+			path := fmt.Sprintf("repos/%s/%s/issues/%d/issue-field-values/%d", owner, repo, issueNumber, fieldID)
+			req, err := client.NewRequest(ctx, http.MethodDelete, path, nil)
+			if err != nil {
+				failedIDs = append(failedIDs, fieldID)
+				if firstFailureErr == nil {
+					firstFailureErr = err
+				}
+				continue
+			}
+			delResp, err := client.Do(req, nil)
+			if err != nil {
+				failedIDs = append(failedIDs, fieldID)
+				if firstFailureErr == nil {
+					firstFailureErr = err
+					firstFailureResp = delResp
+				}
+				continue
+			}
+			succeededIDs = append(succeededIDs, fieldID)
+			_ = delResp.Body.Close()
+		}
+		if len(failedIDs) > 0 {
+			msg := fmt.Sprintf("failed to clear issue field values: failed=%v, cleared=%v", failedIDs, succeededIDs)
+			return ghErrors.NewGitHubAPIErrorResponse(ctx, msg, firstFailureResp, firstFailureErr), nil
+		}
+	}
+
 	// Use GraphQL API for state updates
 	if state != "" {
 		// Mandate specifying duplicateOf when trying to close as duplicate