Fix delete:true on issue fields by calling deleteIssueFieldValue mutation#2755
Open
owenniblock wants to merge 1 commit into
Open
Fix delete:true on issue fields by calling deleteIssueFieldValue mutation#2755owenniblock wants to merge 1 commit into
owenniblock wants to merge 1 commit into
Conversation
owenniblock
added a commit
that referenced
this pull request
Jun 23, 2026
Multi-select issue fields ride on the existing custom-fields surface: - issue_write (consolidated) and set_issue_fields (granular) gain multi-select inputs (field_option_names / multi_select_option_ids) - list_issues field_filters gain a 'values' slot for multi-select filtering with AND semantics - list_issue_fields advertises multi_select in its description and surfaces multi-select definitions - Read paths (IssueFieldValueFragment, list_issues enrichment, etc.) decode multi-select values when an org has them The whole write surface is gated behind a new FF remote_mcp_issue_fields_multiselect. When the flag is off, the legacy variants of issue_write, list_issues, and list_issue_fields are served — same handler bodies, but their schemas and descriptions omit multi_select. Read paths stay unchanged: orgs that have dotcom-side multi-select enabled continue to see multi-select VALUES surfaced, matching what the dotcom UI shows. The granular tools (set_issue_fields) are not separately gated — they are already behind FeatureFlagIssuesGranular, which is itself a user-opt-in rollout flag. Double-gating adds complexity without proportionate benefit for users who have already accepted experimental territory. Per the user's preference for code duplication over interleaved branching: - IssueWrite and IssueWriteLegacy are full siblings (issues.go + issues_legacy_multiselect.go). The shared parser and resolver (optionalIssueWriteFields, resolveIssueRequestFieldValues) take a single multiSelectEnabled bool and reject multi-select inputs/fields when false — one contained branch point per helper, no FF checks threaded through the handler bodies. - ListIssues and ListIssuesLegacy share a buildListIssues helper that swaps in the right descriptions and adds/removes the field_filters[] values slot. - ListIssueFields and ListIssueFieldsLegacy share a buildListIssueFields helper that swaps the description (handler is identical). Snapshot naming follows the established convention: legacy variants own the canonical <name>.snap; MS-aware variants own <name>_ff_remote_mcp_issue_fields_multiselect.snap. Stacked on #2755 (the universal delete-fix half of the original PR). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a correctness gap in issue_write where delete: true could silently fail to clear the last remaining issue field value due to go-github’s omitempty behavior on IssueRequest.IssueFieldValues. The update flow now uses a GraphQL deleteIssueFieldValue mutation for deletions after the REST PATCH succeeds, ensuring field clears are applied even when REST would send {}.
Changes:
- Extend issue-field metadata resolution to capture each field’s GraphQL node ID alongside its REST database ID, and plumb a richer deletion list through the update path.
- Update
UpdateIssueto perform GraphQLdeleteIssueFieldValuemutations for requested deletions (avoids the RESTomitemptyno-op when the kept list is empty). - Add regression/unit tests that pin the
omitemptyserialization behavior and verify REST payload + GraphQL mutation behavior for delete-only and delete+set scenarios.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/issues.go | Carries field node IDs through resolution, updates UpdateIssue to clear deleted issue fields via GraphQL mutation after REST PATCH. |
| pkg/github/issues_granular.go | Adds DeleteIssueFieldValueInput type for the new GraphQL mutation call. |
| pkg/github/issues_delete_test.go | Adds tests covering omitempty behavior and verifying the delete mutation runs while REST PATCH omits issue_field_values when appropriate. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 0
iulia-b
reviewed
Jun 23, 2026
| return nil, nil, fmt.Errorf("issue field %q is missing fullDatabaseId", fieldInput.FieldName) | ||
| } | ||
|
|
||
| if fieldInput.Delete { |
Contributor
There was a problem hiding this comment.
We do have a delete type for IssueWrite request (see line46) it's how a field can be marked for deletion , and it is handled in th emerge and path. The idea of this tool is to call the updateIssue endpoint only, and do all the mutations for an issue in one operation.
Member
Author
There was a problem hiding this comment.
The problem (as far as I can see it) is that when the last issue field is deleted, we end up with: issue_field_values: [] but because we're using omitempty - the API request doesn't have issue_field_values and so the delete doesn't happen.
Another approach could be to update go_github so we're not using omitempty I think. As I say - I'm not yet great at Go so I could be wrong here 😆
Member
Author
There was a problem hiding this comment.
More details from some additional testing I did to double-check:
Verified just now against this repo on #2756:
| Action | Tool response | Server state |
|---|---|---|
Set Priority = P1 |
— | P1 |
main binary: issue_write delete: true |
success | still P1 — silent no-op |
| PR branch: same call | success | [] — cleared |
The dangerous bit is the identical success response — the model would happily report "cleared" when nothing changed.
Contributor
There was a problem hiding this comment.
to update go_github so we're not using omitempty I think.
I can't remember why but that was not an option when i went for this approach
when the last issue field is deleted, we end up with: issue_field_values: []
Not sure I follow. The current schema states, for issue_fields prop:
"issue_fields": {
"description": "Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', or 'delete: true'.", ...
so the call should look smt like: update_issue( issue_fields : [ "priority": { field_option_name: "P1"} , "impact": { delete: true }] )
iulia-b
reviewed
Jun 23, 2026
iulia-b
left a comment
iulia-b
left a comment
Contributor
There was a problem hiding this comment.
Unless I misunderstand the intention, I wouldn't opt for changing the way we delete fields in the updateIssue MCP tool
Collaborator
|
@iulia-b as you are on it please let me know if you validate/a fix is required :-D |
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>
owenniblock
force-pushed
the
owenniblock/issue-fields-delete-bug-fix
branch
from
8c16c3b to
6a07793
Compare
owenniblock
added a commit
that referenced
this pull request
Jun 23, 2026
Multi-select issue fields ride on the existing custom-fields surface: - issue_write (consolidated) and set_issue_fields (granular) gain multi-select inputs (field_option_names / multi_select_option_ids) - list_issues field_filters gain a 'values' slot for multi-select filtering with AND semantics - list_issue_fields advertises multi_select in its description and surfaces multi-select definitions - Read paths (IssueFieldValueFragment, list_issues enrichment, etc.) decode multi-select values when an org has them The write surface is gated behind a new FF remote_mcp_issue_fields_multiselect. When the flag is off, the legacy variants of issue_write, list_issues, and list_issue_fields are served — same handler bodies, but their schemas and descriptions omit multi_select. Read paths stay unchanged: orgs that have dotcom-side multi-select enabled continue to see multi-select VALUES surfaced, matching what the dotcom UI shows. The granular tools (set_issue_fields) are not separately gated — they are already behind FeatureFlagIssuesGranular, which is itself a user-opt-in rollout flag. Double-gating adds complexity without proportionate benefit for users who have already accepted experimental territory. Per the user's preference for code duplication over interleaved branching: - IssueWrite and IssueWriteLegacy are full siblings (issues.go + issues_legacy_multiselect.go). The shared parser and resolver (optionalIssueWriteFields, resolveIssueRequestFieldValues) take a single multiSelectEnabled bool and reject multi-select inputs/fields when false. - ListIssues / ListIssuesLegacy share a buildListIssues helper that swaps in the right descriptions and adds/removes the field_filters[] values slot. - ListIssueFields / ListIssueFieldsLegacy share a buildListIssueFields helper that swaps the description (handler is identical). Snapshot naming follows the established convention: legacy variants own the canonical <name>.snap; MS-aware variants own <name>_ff_remote_mcp_issue_fields_multiselect.snap. Stacked on #2755 (the universal delete-fix half of the original PR). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
REST PATCH alone can't clear an issue field value:
go-github'sIssueRequest.IssueFieldValuescarriesjson:"issue_field_values,omitempty", so Go's encoder strips an empty[]*IssueRequestFieldValuefrom the body. The current merge-and-PATCH flow filters the deleted entry out of the kept list and then assigns it back to the request; when the kept list ends up empty (e.g. you delete the only remaining field value), the entireissue_field_valueskey vanishes from the wire format and the server treats it as a no-op. The field's old value sticks around.Capture each field's GraphQL node ID alongside its database ID at resolve time, and after the REST PATCH succeeds run
deleteIssueFieldValueper deletion. The mutation is idempotent, per-field, and not subject to the omitempty silent-no-op. It also sidesteps the race window in the old fetch-merge-PATCH approach (where a concurrent edit between our fetch and our PATCH would be silently clobbered by REST's set semantics).Empirical confirmation of the bug
Running an equivalent flow on
main(mockingfetchExistingIssueFieldValuesto return one field, callingUpdateIssueasking to delete that field's value, capturing what reaches the PATCH endpoint) produces a literal PATCH body of{}. The server's update handler ingithub/github'sapp/api/issues.rbguards the entire field-values block ondata.include?(ISSUE_FIELD_VALUES)— key absent → nothing touched. Confirmed bug.Tests
Three new tests in
pkg/github/issues_delete_test.go:Test_IssueRequest_EmptyFieldValues_OmittedByJSON— pins the omitempty contract so we know ifgo-githubever drops the tag (at which point this fix could be revisited).Test_UpdateIssue_DeleteFieldValueRunsGraphQLMutation— deletes the only existing field value, captures the REST PATCH body, asserts it has noissue_field_valueskey, and asserts (via mutation matcher) that the GraphQL clear fires.Test_UpdateIssue_DeleteAndSetFieldsInSameCall— combined set-one-delete-another in a single call. REST PATCH carries only the set; GraphQL mutation fires for the deletion.Stack
This PR is the universal bug-fix half of #2659. The multi-select feature work (FF-gated) will be rebased on top of this and re-opened as a stacked PR once this lands.