Skip to content

Add multi_select issue field support (gated behind remote_mcp_issue_fields_multiselect)#2659

Open
owenniblock wants to merge 4 commits into
mainfrom
owenniblock/multi-select-issue-fields
Open

Add multi_select issue field support (gated behind remote_mcp_issue_fields_multiselect)#2659
owenniblock wants to merge 4 commits into
mainfrom
owenniblock/multi-select-issue-fields

Conversation

@owenniblock

@owenniblock owenniblock commented Jun 10, 2026

Copy link
Copy Markdown
Member

Summary

Adds multi-select issue field support across the consolidated and granular issue tools, gated behind a new feature flag remote_mcp_issue_fields_multiselect. When the flag is off, the existing single-select behaviour is preserved.

What's gated behind the FF

  • issue_writefield_option_names slot on issue_fields[] items + multi_select resolver
  • list_issuesfield_filters[].values slot for multi-select option filtering (AND semantics)
  • list_issue_fields — description mentions multi_select

What's not gated

  • The granular set_issue_fields tool — already behind FeatureFlagIssuesGranular, no double-gating
  • Read paths (IssueFieldValueFragment, list_issues enrichment, etc.) — orgs with dotcom-side multi-select enabled continue to see multi-select VALUES surfaced, matching the dotcom UI

Structure

Per the user's preference, code duplication is preferred over interleaved FF branches:

  • IssueWrite / IssueWriteLegacy are full siblings (issues.go + issues_legacy_multiselect.go)
  • ListIssues / ListIssuesLegacy share a buildListIssues helper
  • ListIssueFields / ListIssueFieldsLegacy share a buildListIssueFields helper
  • Shared parser/resolver helpers (optionalIssueWriteFields, resolveIssueRequestFieldValues, parseRawFieldFilters) take a single multiSelectEnabled bool — one contained branch point per helper, no FF checks threaded through handler bodies

Snapshot convention: legacy variants own the canonical <name>.snap; MS-aware own <name>_ff_remote_mcp_issue_fields_multiselect.snap.

Stacked on #2755

This PR is stacked on #2755 — Fix delete:true on issue fields by calling deleteIssueFieldValue mutation. Please review and merge that one first.

The original PR contained both the universal delete-bug fix and the multi-select feature; they were split for cleaner review. The base of this PR is set to the delete-fix branch so the diff shown reflects only the multi-select work.

@owenniblock owenniblock marked this pull request as ready for review June 12, 2026 12:44
@owenniblock owenniblock requested a review from a team as a code owner June 12, 2026 12:44
Copilot AI review requested due to automatic review settings June 12, 2026 12:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds end-to-end support for multi_select issue fields across the MCP server’s issue-field surfaces (list field definitions, set/clear values via issue_write, and read/filter via list_issues), aligning behavior and schema with the existing single-select plumbing and ensuring GraphQL feature-gated fragments are actually exercised.

Changes:

  • Threaded multi_select through GraphQL fragments + minimal output mapping (including MinimalFieldValue.Values).
  • Extended issue_write to accept field_option_names: []string for multi-select, with option validation + improved delete semantics.
  • Extended list_issues field filtering to accept values: []string for multi-select (AND semantics) and updated toolsnaps/docs/tests accordingly.
Show a summary per file
File Description
pkg/github/minimal_types.go Adds multi-select support in GraphQL→minimal field-value conversion (populates Values).
pkg/github/issues.go Adds multi-select fragments, issue_write parsing/validation, list_issues multi-select filtering, and GraphQL-backed delete-on-clear behavior.
pkg/github/issues_test.go Updates hardcoded GraphQL query string expectations and error-message assertions for new fragments/validation.
pkg/github/issues_multiselect_test.go New targeted tests for multi-select parsing, filter resolution, fragment conversion, and delete mutation behavior.
pkg/github/issues_granular.go Introduces DeleteIssueFieldValueInput type used by the new delete mutation path.
pkg/github/issue_fields.go Extends list_issue_fields to surface multi_select field definitions and options.
pkg/github/toolsnaps/list_issues_ff_remote_mcp_issue_fields.snap Updates schema snapshot for new field_filters[].values and revised description/required fields.
pkg/github/toolsnaps/list_issue_fields.snap Updates schema snapshot to include multi_select in description/output expectations.
pkg/github/toolsnaps/issue_write_ff_remote_mcp_issue_fields.snap Updates schema snapshot to include field_option_names and updated mutual-exclusion wording.
docs/insiders-features.md Regenerates tool docs reflecting new issue_write.issue_fields[].field_option_names and list_issues.field_filters[].values.
docs/feature-flags.md Regenerates feature-flag inventory/docs to reflect the updated tool schemas/descriptions.

Copilot's findings

  • Files reviewed: 11/11 changed files
  • Comments generated: 3

Comment thread pkg/github/issues.go Outdated
Comment thread pkg/github/issues.go
Comment thread pkg/github/issues.go
Comment on lines +3352 to +3363
func matchOption(field IssueField, value string) (string, error) {
for _, o := range field.Options {
if strings.EqualFold(o.Name, value) {
return o.Name, nil
}
}
optionNames := make([]string, 0, len(field.Options))
for _, o := range field.Options {
optionNames = append(optionNames, o.Name)
}
return "", fmt.Errorf("field_filters: %q is not a valid option for %q. Valid options: %s", value, field.Name, strings.Join(optionNames, ", "))
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — trimming both sides now, with a regression test.

@akenneth akenneth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work! left a few questions I am unsure about.

Also tagging @iulia-b and @kelsey-myers for extra 👀 since you are familiar with this part

Comment thread pkg/github/issues.go Outdated
type issueFieldWriteMetadataNode struct {
TypeName githubv4.String `graphql:"__typename"`
IssueFieldText struct {
ID githubv4.ID

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? this wasn't here before for other types right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's new. The delete: true path now calls the deleteIssueFieldValue GraphQL mutation, which takes the field's node id as input — and delete can target any field type, so we grab id on every fragment. Before this PR there was no GraphQL delete, so we never needed the node id.

Comment thread pkg/github/issues.go Outdated
}

func resolveIssueRequestFieldValues(ctx context.Context, gqlClient *githubv4.Client, owner, repo string, issueFields []issueWriteFieldInput) ([]*github.IssueRequestFieldValue, []int64, error) {
// fieldDeletion carries both identifiers needed to clear an issue field value:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this change is needed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so — we added field_option_names as a third value setter, so the old 2-way value vs field_option_name mutual-exclusion check had to become 3-way. Happy to simplify if you can see a cleaner way though.

Comment thread pkg/github/issues.go Outdated
// REST IssueField#build_value_attributes for multi_select expects an array of option names.
resolvedValue = resolvedNames
default:
// Raw value path. Reject it for multi_select so callers get a clear error

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i think we can reduce the comments here, the if is clear enough

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍🏼

Comment thread pkg/github/issues.go Outdated
value = string(node.SingleSelectValue.Value)
case "IssueFieldMultiSelectValue":
fieldIDStr = node.MultiSelectValue.Field.FullDatabaseIDStr()
// REST IssueField#build_value_attributes for multi_select expects an array

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍🏼

Comment thread pkg/github/issues.go

var q searchIssuesNodesQuery
if err := gqlClient.Query(ctx, &q, map[string]any{"ids": ids}); err != nil {
ctxWithFeatures := ghcontext.WithGraphQLFeatures(ctx, "issue_fields", "repo_issue_fields")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latent bug fix — fetchIssueFieldValuesByNodeID was missing the ghcontext.WithGraphQLFeatures("issue_fields", "repo_issue_fields") wrap, so without it the new fragment would have silently no-op'd on dotcom even with the right feature flags enabled.

Interesting! What was broken here? is it searching with fields?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not search/filtering — it's the read enrichment. fetchIssueFieldValuesByNodeID backs the field_values you get on get_issue and search_issues output. It was missing the WithGraphQLFeatures("issue_fields", "repo_issue_fields") wrap, so on dotcom the new IssueFieldValues fragment would just come back empty even with the flags on — field values silently wouldn't show up. Adding the wrap makes it actually resolve.

Comment thread pkg/github/issues.go Outdated
// Clear any fields marked with delete:true via the GraphQL deleteIssueFieldValue
// mutation. The REST PATCH above can't do this reliably — Go's omitempty drops an
// empty issue_field_values array, leaving the old values intact.
if len(fieldsToDelete) > 0 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am confused, wasn't this code already in place?
what were we doing with the previous fieldIDsToDelete ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question — it looked in place but it didn't actually work. Previously resolveIssueRequestFieldValues returned []int64 fieldIDsToDelete and we leaned on the REST PATCH to clear them by sending an empty value — but Go's omitempty drops the empty issue_field_values array, so the clear silently no-op'd and the old values stuck around. This PR swaps that for an explicit deleteIssueFieldValue GraphQL mutation per field, so fieldDeletion now carries both the db id (to keep it out of the REST payload) and the node id (for the mutation). So delete: true actually clears now.

@owenniblock owenniblock force-pushed the owenniblock/multi-select-issue-fields branch from 786a2d1 to c726b68 Compare June 23, 2026 10:21
@owenniblock owenniblock changed the base branch from main to owenniblock/issue-fields-delete-bug-fix June 23, 2026 10:21
@owenniblock owenniblock changed the title Add multi_select issue field support Add multi_select issue field support (gated behind remote_mcp_issue_fields_multiselect) Jun 23, 2026
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 owenniblock force-pushed the owenniblock/issue-fields-delete-bug-fix branch from 8c16c3b to 6a07793 Compare June 23, 2026 12:48
@owenniblock owenniblock force-pushed the owenniblock/multi-select-issue-fields branch 3 times, most recently from 218443c to 5cd0dff Compare June 24, 2026 10:32
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>
@owenniblock owenniblock force-pushed the owenniblock/issue-fields-delete-bug-fix branch from b399b6c to 3b22154 Compare June 24, 2026 10:33
@owenniblock owenniblock force-pushed the owenniblock/multi-select-issue-fields branch from 5cd0dff to 995419a Compare June 24, 2026 10:34
Trim the doc comments on UpdateIssue's omitempty-trap region + test
file to the bare minimum needed to understand the non-obvious behaviour
(why the DELETE fallback exists, why we filter to existing IDs, why
errors are aggregated). Drop restated context and step-by-step
explanations.

No behaviour change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@owenniblock owenniblock force-pushed the owenniblock/multi-select-issue-fields branch from 995419a to a02f4d3 Compare June 25, 2026 15:26
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>
@owenniblock owenniblock reopened this Jun 26, 2026
Base automatically changed from owenniblock/issue-fields-delete-bug-fix to main June 26, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants