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>

Author: owenniblock

pkg/github/issues.go

@@ -2216,8 +2216,20 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4
 			// 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.
-			fallbackDeleteFieldIDs = fieldIDsToDelete
+			// 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
 		}
@@ -2243,22 +2255,45 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4
 
 	// 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's idempotent, takes
-	// the integer field ID, and the URL is the standard `/repos/{owner}/{repo}`
-	// pattern (no need for the numeric repository ID despite what the internal
-	// route in app/api/issue_field_values.rb suggests; the public API rewrites
-	// to it).
-	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 {
-			return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to build DELETE request for issue field value", nil, err), nil
+	// 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()
 		}
-		delResp, err := client.Do(req, nil)
-		if err != nil {
-			return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to clear issue field value", delResp, err), nil
+		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
 		}
-		_ = delResp.Body.Close()
 	}
 
 	// Use GraphQL API for state updates

pkg/github/issues_delete_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"io"
 	"net/http"
+	"strings"
 	"sync"
 	"testing"
 
@@ -246,3 +247,213 @@ func Test_UpdateIssue_DeleteOneOfManyUsesSetSemantics(t *testing.T) {
 	require.Empty(t, deletePaths,
 		"no DELETE call should fire when the kept set is non-empty — the PATCH's set semantics clear the deleted field on the server side")
 }
+
+// Test_UpdateIssue_DeleteAbsentFieldIsNoOp verifies that asking to delete a
+// field that isn't currently set on the issue does not fire a DELETE request
+// to the dedicated endpoint (which would 404). This preserves the pre-fix
+// behaviour of treating "delete a field that isn't set" as a silent no-op —
+// important because callers often use delete:true idempotently ("ensure
+// field X is cleared"), and the second invocation should succeed not error.
+func Test_UpdateIssue_DeleteAbsentFieldIsNoOp(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"),
+	}
+
+	var (
+		mu                sync.Mutex
+		capturedPatchBody []byte
+		deletePaths       []string
+	)
+
+	restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
+		PatchReposIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) {
+			body, _ := io.ReadAll(r.Body)
+			mu.Lock()
+			capturedPatchBody = body
+			mu.Unlock()
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusOK)
+			_ = json.NewEncoder(w).Encode(mockIssue)
+		},
+		DeleteReposIssuesIssueFieldValueByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) {
+			mu.Lock()
+			deletePaths = append(deletePaths, r.URL.Path)
+			mu.Unlock()
+			// Fail loudly: if we get here, the fix is wrong.
+			w.WriteHeader(http.StatusNotFound)
+		},
+	}))
+
+	// Issue has no field values at all.
+	existingFieldsResponse := githubv4mock.DataResponse(map[string]any{
+		"repository": map[string]any{
+			"issue": map[string]any{
+				"issueFieldValues": map[string]any{
+					"nodes": []any{},
+				},
+			},
+		},
+	})
+
+	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,
+		),
+	))
+
+	result, err := UpdateIssue(
+		context.Background(),
+		restClient,
+		gqlClient,
+		"owner", "repo", 42,
+		"", "", nil, nil, 0, "",
+		nil,
+		[]int64{101}, // ask to delete a field that isn't set
+		"", "", 0,
+	)
+	require.NoError(t, err)
+	if result.IsError {
+		t.Fatalf("expected non-error result, got: %s", getTextResult(t, result).Text)
+	}
+
+	mu.Lock()
+	defer mu.Unlock()
+	require.NotContains(t, string(capturedPatchBody), "issue_field_values",
+		"PATCH body must not carry issue_field_values when nothing changed")
+	require.Empty(t, deletePaths,
+		"no DELETE call should fire for a field that isn't present on the issue — preserves the pre-fix silent-no-op behaviour and avoids a guaranteed 404")
+}
+
+// Test_UpdateIssue_DeleteFallbackContinuesOnPartialFailure verifies that when
+// one DELETE in the fallback loop fails, the remaining DELETEs still fire and
+// the aggregated error names the failed and succeeded field IDs. The pre-fix
+// loop short-circuited on the first failure, which left the caller blind to
+// which deletions had landed.
+func Test_UpdateIssue_DeleteFallbackContinuesOnPartialFailure(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"),
+	}
+
+	var (
+		mu          sync.Mutex
+		deletePaths []string
+	)
+
+	restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
+		PatchReposIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusOK)
+			_ = json.NewEncoder(w).Encode(mockIssue)
+		},
+		DeleteReposIssuesIssueFieldValueByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) {
+			mu.Lock()
+			deletePaths = append(deletePaths, r.URL.Path)
+			mu.Unlock()
+			// Field 202 returns 500; fields 101 and 303 succeed. We expect
+			// all three calls to fire even though the middle one errors, and
+			// the final tool result must mention 202 as failed and 101/303
+			// as cleared.
+			if strings.HasSuffix(r.URL.Path, "/202") {
+				w.WriteHeader(http.StatusInternalServerError)
+				_, _ = w.Write([]byte(`{"message":"simulated failure"}`))
+				return
+			}
+			w.WriteHeader(http.StatusNoContent)
+		},
+	}))
+
+	existingFieldsResponse := githubv4mock.DataResponse(map[string]any{
+		"repository": map[string]any{
+			"issue": map[string]any{
+				"issueFieldValues": map[string]any{
+					"nodes": []any{
+						map[string]any{
+							"__typename": "IssueFieldSingleSelectValue",
+							"field":      map[string]any{"fullDatabaseId": "101", "name": "Priority"},
+							"value":      "P1",
+						},
+						map[string]any{
+							"__typename": "IssueFieldSingleSelectValue",
+							"field":      map[string]any{"fullDatabaseId": "202", "name": "Visibility"},
+							"value":      "High",
+						},
+						map[string]any{
+							"__typename": "IssueFieldSingleSelectValue",
+							"field":      map[string]any{"fullDatabaseId": "303", "name": "Impact"},
+							"value":      "Critical",
+						},
+					},
+				},
+			},
+		},
+	})
+
+	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,
+		),
+	))
+
+	result, err := UpdateIssue(
+		context.Background(),
+		restClient,
+		gqlClient,
+		"owner", "repo", 42,
+		"", "", nil, nil, 0, "",
+		nil,
+		[]int64{101, 202, 303},
+		"", "", 0,
+	)
+	require.NoError(t, err)
+	require.True(t, result.IsError, "expected an error result because field 202 failed")
+
+	mu.Lock()
+	defer mu.Unlock()
+	// All three DELETEs must have fired — the middle failure must not short-circuit the third.
+	require.Len(t, deletePaths, 3,
+		"all three DELETE calls should fire even though one fails; got paths: %v", deletePaths)
+
+	resultText := getTextResult(t, result).Text
+	require.Contains(t, resultText, "failed=[202]",
+		"error must name the failed field ID so the caller can retry it; got: %s", resultText)
+	require.Contains(t, resultText, "cleared=[101 303]",
+		"error must name the cleared field IDs so the caller knows what's already done; got: %s", resultText)
+}