From 6a07793e7bac57e72e88a8acf8187408967e1b88 Mon Sep 17 00:00:00 2001 From: Owen Niblock Date: Tue, 23 Jun 2026 13:48:33 +0100 Subject: [PATCH 1/2] Fall back to REST DELETE when the PATCH can't carry an issue field clear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 github/github-mcp-server#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> --- pkg/github/helper_test.go | 1 + pkg/github/issues.go | 41 ++++- pkg/github/issues_delete_test.go | 248 +++++++++++++++++++++++++++++++ 3 files changed, 289 insertions(+), 1 deletion(-) create mode 100644 pkg/github/issues_delete_test.go diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index 2ad173679..e4f00f399 100644 --- a/pkg/github/helper_test.go +++ b/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" diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 5479f3579..519248645 100644 --- a/pkg/github/issues.go +++ b/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,15 @@ 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. + fallbackDeleteFieldIDs = fieldIDsToDelete + } else { + issueRequest.IssueFieldValues = merged + } } updatedIssue, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueRequest) @@ -2222,6 +2241,26 @@ 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'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 + } + delResp, err := client.Do(req, nil) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to clear issue field value", delResp, err), nil + } + _ = delResp.Body.Close() + } + // Use GraphQL API for state updates if state != "" { // Mandate specifying duplicateOf when trying to close as duplicate diff --git a/pkg/github/issues_delete_test.go b/pkg/github/issues_delete_test.go new file mode 100644 index 000000000..9c7427ab4 --- /dev/null +++ b/pkg/github/issues_delete_test.go @@ -0,0 +1,248 @@ +package github + +import ( + "context" + "encoding/json" + "io" + "net/http" + "sync" + "testing" + + "github.com/github/github-mcp-server/internal/githubv4mock" + gogithub "github.com/google/go-github/v87/github" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Test_IssueRequest_EmptyFieldValues_OmittedByJSON pins the omitempty behaviour +// that motivates the DELETE-endpoint fallback in UpdateIssue. If go-github's +// IssueRequest ever drops the `omitempty` tag from `issue_field_values`, this +// test will fail — at which point the fallback could potentially be revisited. +// Until then, an empty `[]*IssueRequestFieldValue{}` is serialised as nothing +// at all, so the REST PATCH alone can never clear a field's last value via +// the set-semantics path. +func Test_IssueRequest_EmptyFieldValues_OmittedByJSON(t *testing.T) { + t.Parallel() + + req := &gogithub.IssueRequest{ + Title: gogithub.Ptr("still here"), + IssueFieldValues: []*gogithub.IssueRequestFieldValue{}, + } + body, err := json.Marshal(req) + require.NoError(t, err) + + assert.NotContains(t, string(body), "issue_field_values", + "empty IssueFieldValues should be dropped by omitempty — this is why the REST PATCH alone can't clear field values when the merged list ends up empty, and why we fall back to the dedicated DELETE endpoint") + assert.Contains(t, string(body), `"title":"still here"`, + "sanity check: other fields still serialise") +} + +// Test_UpdateIssue_DeleteLastFieldValueCallsDeleteEndpoint: regression test for +// the delete:true bug. When the kept set after merge + filter ends up empty +// (e.g. deleting the only remaining field value), the PATCH alone cannot carry +// the deletion intent because go-github strips the empty issue_field_values +// slice via omitempty. UpdateIssue follows up with a per-field DELETE to the +// dedicated `/repos/{owner}/{repo}/issues/{number}/issue-field-values/{id}` +// endpoint. +// +// Asserts both halves: +// +// - the PATCH body does NOT carry an `issue_field_values` key (we don't want +// to double-clear or rely on a value omitempty is about to strip) +// - a DELETE for the field ID fires after the PATCH +func Test_UpdateIssue_DeleteLastFieldValueCallsDeleteEndpoint(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() + w.WriteHeader(http.StatusNoContent) + }, + })) + + // Existing field values for the merge step. Returning only the field about + // to be deleted is the worst case: the kept list ends up empty and the + // fallback DELETE is the only thing that can clear it. + 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", + }, + }, + }, + }, + }, + }) + + 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}, + "", "", 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", + "REST PATCH body must not carry issue_field_values when the kept set is empty (PATCH body was: %s)", string(capturedPatchBody)) + require.Equal(t, []string{"/repos/owner/repo/issues/42/issue-field-values/101"}, deletePaths, + "expected exactly one DELETE call to the dedicated endpoint for field id 101") +} + +// Test_UpdateIssue_DeleteOneOfManyUsesSetSemantics verifies that when the kept +// set after merge + filter is non-empty (deleting 1 of N existing fields), the +// PATCH carries the kept fields and the dotcom REST handler's set semantics do +// the deletion implicitly — no fallback DELETE call is needed. +func Test_UpdateIssue_DeleteOneOfManyUsesSetSemantics(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: expectRequestBody(t, map[string]any{ + "issue_field_values": []any{ + map[string]any{"field_id": float64(202), "value": "High"}, + }, + }).andThen( + mockResponse(t, http.StatusOK, mockIssue), + ), + DeleteReposIssuesIssueFieldValueByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + deletePaths = append(deletePaths, r.URL.Path) + mu.Unlock() + 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": "Impact"}, + "value": "High", + }, + }, + }, + }, + }, + }) + + 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}, + "", "", 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.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") +} From 3b2215482f1605d5c92394231eef7f9081e56edb Mon Sep 17 00:00:00 2001 From: Owen Niblock Date: Wed, 24 Jun 2026 11:27:37 +0100 Subject: [PATCH 2/2] 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> --- pkg/github/issues.go | 67 +++++++--- pkg/github/issues_delete_test.go | 211 +++++++++++++++++++++++++++++++ 2 files changed, 262 insertions(+), 16 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 519248645..100178a77 100644 --- a/pkg/github/issues.go +++ b/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 diff --git a/pkg/github/issues_delete_test.go b/pkg/github/issues_delete_test.go index 9c7427ab4..4661339f8 100644 --- a/pkg/github/issues_delete_test.go +++ b/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, _ *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) +}