Fix delete:true on issue fields by calling deleteIssueFieldValue mutation

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

@@ -2179,6 +2179,17 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4
 		issueRequest.Type = github.Ptr(issueType)
 	}
 
+	// fallbackDeleteFieldIDs holds field IDs we need to clear via the dedicated
+	// REST DELETE endpoint after the PATCH lands. This is the omitempty-trap
+	// fallback: when the merged list is empty AND we have deletions to make,
+	// `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 short-circuits, and nothing
+	// gets cleared. When the merged list is non-empty the PATCH's set semantics
+	// handle the deletes implicitly (current - new), so no DELETE follow-up is
+	// needed in that path.
+	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
@@ -2201,7 +2212,27 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4
 			}
 			merged = kept
 		}
-		issueRequest.IssueFieldValues = merged
+		if len(merged) == 0 && len(fieldIDsToDelete) > 0 {
+			// Omitempty trap: skip the IssueFieldValues assignment so we don't
+			// rely on a value that's about to be stripped from the JSON anyway,
+			// and clear each field via the dedicated DELETE endpoint after the
+			// PATCH lands. Only queue a DELETE for fields actually present on
+			// the issue — the dotcom DELETE endpoint returns 404 for absent
+			// values, and we want to preserve the pre-fix behaviour of treating
+			// "delete a field that isn't set" as a silent no-op (which is what
+			// happens when the user idempotently re-runs the same call).
+			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)
@@ -2222,6 +2253,49 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4
 		return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to update issue", resp, body), nil
 	}
 
+	// Clear any fields whose deletion the PATCH couldn't carry. See the
+	// fallbackDeleteFieldIDs declaration above for the omitempty trap details.
+	// We hit the dedicated DELETE endpoint per field — it takes the integer
+	// field ID, and the URL is the standard `/repos/{owner}/{repo}` pattern.
+	//
+	// Per-field errors don't short-circuit: each DELETE is attempted, and any
+	// failures are aggregated into a single error response that names the
+	// successful and failed field IDs. This avoids leaving the caller blind
+	// to which deletions landed when one of several fails (e.g. transient
+	// 5xx on field 2 of 3 — without aggregation, field 1 is already gone and
+	// field 3 was never attempted, but the caller only sees a generic error).
+	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