Skip to content

Commit 6a07793

Browse files
owenniblockCopilot
owenniblock
and
Copilot
committed
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>
1 parent 9430064 commit 6a07793

3 files changed

Lines changed: 289 additions & 1 deletion

File tree

‎pkg/github/helper_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ const (
6666
PostReposIssuesSubIssuesByOwnerByRepoByIssueNumber = "POST /repos/{owner}/{repo}/issues/{issue_number}/sub_issues"
6767
DeleteReposIssuesSubIssueByOwnerByRepoByIssueNumber = "DELETE /repos/{owner}/{repo}/issues/{issue_number}/sub_issue"
6868
PatchReposIssuesSubIssuesPriorityByOwnerByRepoByIssueNumber = "PATCH /repos/{owner}/{repo}/issues/{issue_number}/sub_issues/priority"
69+
DeleteReposIssuesIssueFieldValueByOwnerByRepoByIssueNumber = "DELETE /repos/{owner}/{repo}/issues/{issue_number}/issue-field-values/{issue_field_id}"
6970

7071
// Pull request endpoints
7172
GetReposPullsByOwnerByRepo = "GET /repos/{owner}/{repo}/pulls"

‎pkg/github/issues.go‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2179,6 +2179,17 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4
21792179
issueRequest.Type = github.Ptr(issueType)
21802180
}
21812181

2182+
// fallbackDeleteFieldIDs holds field IDs we need to clear via the dedicated
2183+
// REST DELETE endpoint after the PATCH lands. This is the omitempty-trap
2184+
// fallback: when the merged list is empty AND we have deletions to make,
2185+
// `go-github`'s `omitempty` tag on IssueRequest.IssueFieldValues strips the
2186+
// empty slice from the JSON body, the dotcom REST handler's top-level
2187+
// `if data.include?(ISSUE_FIELD_VALUES)` guard short-circuits, and nothing
2188+
// gets cleared. When the merged list is non-empty the PATCH's set semantics
2189+
// handle the deletes implicitly (current - new), so no DELETE follow-up is
2190+
// needed in that path.
2191+
var fallbackDeleteFieldIDs []int64
2192+
21822193
if len(issueFieldValues) > 0 || len(fieldIDsToDelete) > 0 {
21832194
// The REST update endpoint uses "set" semantics — it overwrites all existing
21842195
// 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
22012212
}
22022213
merged = kept
22032214
}
2204-
issueRequest.IssueFieldValues = merged
2215+
if len(merged) == 0 && len(fieldIDsToDelete) > 0 {
2216+
// Omitempty trap: skip the IssueFieldValues assignment so we don't
2217+
// rely on a value that's about to be stripped from the JSON anyway,
2218+
// and clear each field via the dedicated DELETE endpoint after the
2219+
// PATCH lands.
2220+
fallbackDeleteFieldIDs = fieldIDsToDelete
2221+
} else {
2222+
issueRequest.IssueFieldValues = merged
2223+
}
22052224
}
22062225

22072226
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
22222241
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to update issue", resp, body), nil
22232242
}
22242243

2244+
// Clear any fields whose deletion the PATCH couldn't carry. See the
2245+
// fallbackDeleteFieldIDs declaration above for the omitempty trap details.
2246+
// We hit the dedicated DELETE endpoint per field — it's idempotent, takes
2247+
// the integer field ID, and the URL is the standard `/repos/{owner}/{repo}`
2248+
// pattern (no need for the numeric repository ID despite what the internal
2249+
// route in app/api/issue_field_values.rb suggests; the public API rewrites
2250+
// to it).
2251+
for _, fieldID := range fallbackDeleteFieldIDs {
2252+
path := fmt.Sprintf("repos/%s/%s/issues/%d/issue-field-values/%d", owner, repo, issueNumber, fieldID)
2253+
req, err := client.NewRequest(ctx, http.MethodDelete, path, nil)
2254+
if err != nil {
2255+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to build DELETE request for issue field value", nil, err), nil
2256+
}
2257+
delResp, err := client.Do(req, nil)
2258+
if err != nil {
2259+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to clear issue field value", delResp, err), nil
2260+
}
2261+
_ = delResp.Body.Close()
2262+
}
2263+
22252264
// Use GraphQL API for state updates
22262265
if state != "" {
22272266
// Mandate specifying duplicateOf when trying to close as duplicate

‎pkg/github/issues_delete_test.go‎

Lines changed: 248 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,248 @@
1+
package github
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"io"
7+
"net/http"
8+
"sync"
9+
"testing"
10+
11+
"github.com/github/github-mcp-server/internal/githubv4mock"
12+
gogithub "github.com/google/go-github/v87/github"
13+
"github.com/shurcooL/githubv4"
14+
"github.com/stretchr/testify/assert"
15+
"github.com/stretchr/testify/require"
16+
)
17+
18+
// Test_IssueRequest_EmptyFieldValues_OmittedByJSON pins the omitempty behaviour
19+
// that motivates the DELETE-endpoint fallback in UpdateIssue. If go-github's
20+
// IssueRequest ever drops the `omitempty` tag from `issue_field_values`, this
21+
// test will fail — at which point the fallback could potentially be revisited.
22+
// Until then, an empty `[]*IssueRequestFieldValue{}` is serialised as nothing
23+
// at all, so the REST PATCH alone can never clear a field's last value via
24+
// the set-semantics path.
25+
func Test_IssueRequest_EmptyFieldValues_OmittedByJSON(t *testing.T) {
26+
t.Parallel()
27+
28+
req := &gogithub.IssueRequest{
29+
Title: gogithub.Ptr("still here"),
30+
IssueFieldValues: []*gogithub.IssueRequestFieldValue{},
31+
}
32+
body, err := json.Marshal(req)
33+
require.NoError(t, err)
34+
35+
assert.NotContains(t, string(body), "issue_field_values",
36+
"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")
37+
assert.Contains(t, string(body), `"title":"still here"`,
38+
"sanity check: other fields still serialise")
39+
}
40+
41+
// Test_UpdateIssue_DeleteLastFieldValueCallsDeleteEndpoint: regression test for
42+
// the delete:true bug. When the kept set after merge + filter ends up empty
43+
// (e.g. deleting the only remaining field value), the PATCH alone cannot carry
44+
// the deletion intent because go-github strips the empty issue_field_values
45+
// slice via omitempty. UpdateIssue follows up with a per-field DELETE to the
46+
// dedicated `/repos/{owner}/{repo}/issues/{number}/issue-field-values/{id}`
47+
// endpoint.
48+
//
49+
// Asserts both halves:
50+
//
51+
// - the PATCH body does NOT carry an `issue_field_values` key (we don't want
52+
// to double-clear or rely on a value omitempty is about to strip)
53+
// - a DELETE for the field ID fires after the PATCH
54+
func Test_UpdateIssue_DeleteLastFieldValueCallsDeleteEndpoint(t *testing.T) {
55+
t.Parallel()
56+
57+
mockIssue := &gogithub.Issue{
58+
Number: gogithub.Ptr(42),
59+
Title: gogithub.Ptr("Test issue"),
60+
State: gogithub.Ptr("open"),
61+
HTMLURL: gogithub.Ptr("https://github.com/owner/repo/issues/42"),
62+
}
63+
64+
var (
65+
mu sync.Mutex
66+
capturedPatchBody []byte
67+
deletePaths []string
68+
)
69+
70+
restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
71+
PatchReposIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) {
72+
body, _ := io.ReadAll(r.Body)
73+
mu.Lock()
74+
capturedPatchBody = body
75+
mu.Unlock()
76+
w.Header().Set("Content-Type", "application/json")
77+
w.WriteHeader(http.StatusOK)
78+
_ = json.NewEncoder(w).Encode(mockIssue)
79+
},
80+
DeleteReposIssuesIssueFieldValueByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) {
81+
mu.Lock()
82+
deletePaths = append(deletePaths, r.URL.Path)
83+
mu.Unlock()
84+
w.WriteHeader(http.StatusNoContent)
85+
},
86+
}))
87+
88+
// Existing field values for the merge step. Returning only the field about
89+
// to be deleted is the worst case: the kept list ends up empty and the
90+
// fallback DELETE is the only thing that can clear it.
91+
existingFieldsResponse := githubv4mock.DataResponse(map[string]any{
92+
"repository": map[string]any{
93+
"issue": map[string]any{
94+
"issueFieldValues": map[string]any{
95+
"nodes": []any{
96+
map[string]any{
97+
"__typename": "IssueFieldSingleSelectValue",
98+
"field": map[string]any{
99+
"fullDatabaseId": "101",
100+
"name": "Priority",
101+
},
102+
"value": "P1",
103+
},
104+
},
105+
},
106+
},
107+
},
108+
})
109+
110+
gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(
111+
githubv4mock.NewQueryMatcher(
112+
struct {
113+
Repository struct {
114+
Issue struct {
115+
IssueFieldValues struct {
116+
Nodes []IssueFieldValueFragment
117+
} `graphql:"issueFieldValues(first: 25)"`
118+
} `graphql:"issue(number: $number)"`
119+
} `graphql:"repository(owner: $owner, name: $repo)"`
120+
}{},
121+
map[string]any{
122+
"owner": githubv4.String("owner"),
123+
"repo": githubv4.String("repo"),
124+
"number": githubv4.Int(42),
125+
},
126+
existingFieldsResponse,
127+
),
128+
))
129+
130+
result, err := UpdateIssue(
131+
context.Background(),
132+
restClient,
133+
gqlClient,
134+
"owner", "repo", 42,
135+
"", "", nil, nil, 0, "",
136+
nil,
137+
[]int64{101},
138+
"", "", 0,
139+
)
140+
require.NoError(t, err)
141+
if result.IsError {
142+
t.Fatalf("expected non-error result, got: %s", getTextResult(t, result).Text)
143+
}
144+
145+
mu.Lock()
146+
defer mu.Unlock()
147+
require.NotContains(t, string(capturedPatchBody), "issue_field_values",
148+
"REST PATCH body must not carry issue_field_values when the kept set is empty (PATCH body was: %s)", string(capturedPatchBody))
149+
require.Equal(t, []string{"/repos/owner/repo/issues/42/issue-field-values/101"}, deletePaths,
150+
"expected exactly one DELETE call to the dedicated endpoint for field id 101")
151+
}
152+
153+
// Test_UpdateIssue_DeleteOneOfManyUsesSetSemantics verifies that when the kept
154+
// set after merge + filter is non-empty (deleting 1 of N existing fields), the
155+
// PATCH carries the kept fields and the dotcom REST handler's set semantics do
156+
// the deletion implicitly — no fallback DELETE call is needed.
157+
func Test_UpdateIssue_DeleteOneOfManyUsesSetSemantics(t *testing.T) {
158+
t.Parallel()
159+
160+
mockIssue := &gogithub.Issue{
161+
Number: gogithub.Ptr(42),
162+
Title: gogithub.Ptr("Test issue"),
163+
State: gogithub.Ptr("open"),
164+
HTMLURL: gogithub.Ptr("https://github.com/owner/repo/issues/42"),
165+
}
166+
167+
var (
168+
mu sync.Mutex
169+
deletePaths []string
170+
)
171+
172+
restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
173+
PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{
174+
"issue_field_values": []any{
175+
map[string]any{"field_id": float64(202), "value": "High"},
176+
},
177+
}).andThen(
178+
mockResponse(t, http.StatusOK, mockIssue),
179+
),
180+
DeleteReposIssuesIssueFieldValueByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) {
181+
mu.Lock()
182+
deletePaths = append(deletePaths, r.URL.Path)
183+
mu.Unlock()
184+
w.WriteHeader(http.StatusNoContent)
185+
},
186+
}))
187+
188+
existingFieldsResponse := githubv4mock.DataResponse(map[string]any{
189+
"repository": map[string]any{
190+
"issue": map[string]any{
191+
"issueFieldValues": map[string]any{
192+
"nodes": []any{
193+
map[string]any{
194+
"__typename": "IssueFieldSingleSelectValue",
195+
"field": map[string]any{"fullDatabaseId": "101", "name": "Priority"},
196+
"value": "P1",
197+
},
198+
map[string]any{
199+
"__typename": "IssueFieldSingleSelectValue",
200+
"field": map[string]any{"fullDatabaseId": "202", "name": "Impact"},
201+
"value": "High",
202+
},
203+
},
204+
},
205+
},
206+
},
207+
})
208+
209+
gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(
210+
githubv4mock.NewQueryMatcher(
211+
struct {
212+
Repository struct {
213+
Issue struct {
214+
IssueFieldValues struct {
215+
Nodes []IssueFieldValueFragment
216+
} `graphql:"issueFieldValues(first: 25)"`
217+
} `graphql:"issue(number: $number)"`
218+
} `graphql:"repository(owner: $owner, name: $repo)"`
219+
}{},
220+
map[string]any{
221+
"owner": githubv4.String("owner"),
222+
"repo": githubv4.String("repo"),
223+
"number": githubv4.Int(42),
224+
},
225+
existingFieldsResponse,
226+
),
227+
))
228+
229+
result, err := UpdateIssue(
230+
context.Background(),
231+
restClient,
232+
gqlClient,
233+
"owner", "repo", 42,
234+
"", "", nil, nil, 0, "",
235+
nil,
236+
[]int64{101},
237+
"", "", 0,
238+
)
239+
require.NoError(t, err)
240+
if result.IsError {
241+
t.Fatalf("expected non-error result, got: %s", getTextResult(t, result).Text)
242+
}
243+
244+
mu.Lock()
245+
defer mu.Unlock()
246+
require.Empty(t, deletePaths,
247+
"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")
248+
}

0 commit comments

Comments
 (0)