From eacc2818d52f5cab3f66c6e1ad0818ed2a18191a Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 27 May 2026 16:47:43 +0200 Subject: [PATCH 1/4] feat: gate issue_write and get_issue behind remote_mcp_issue_fields flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports the gating from PR #2553 onto main (the original merge landed on a stack base that did not make it to main). Changes: - pkg/inventory: FeatureFlagDisable becomes []string (any-listed-on → hide). FeatureFlagEnable stays as a single string. This avoids the AND-of-enable semantics from the earlier proposal, which encoded dependencies rather than rollout knobs and had no real call site. Disable-OR is the case that does need the slice (LegacyIssueWrite below). - pkg/github/issues.go: split IssueWrite into IssueWrite (flag-enabled, exposes issue_fields) and LegacyIssueWrite (flag-disabled, omits it). Both register as 'issue_write'; mutually exclusive flag annotations pick exactly one at runtime. Refactored into a shared buildIssueWrite helper instead of duplicating the ~250-line tool definition. - pkg/github/issues.go: GetIssue field_values enrichment now requires the flag at runtime. The verbose REST IssueFieldValues is always cleared from the response. - Existing single-flag Disable call sites converted to slices. - New toolsnap variant issue_write_ff_remote_mcp_issue_fields.snap; the canonical issue_write.snap is owned by LegacyIssueWrite. - README + flag docs regenerated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 1 - docs/feature-flags.md | 22 ++- docs/insiders-features.md | 22 ++- pkg/github/__toolsnaps__/issue_write.snap | 36 ----- ...ssue_write_ff_remote_mcp_issue_fields.snap | 133 ++++++++++++++++++ pkg/github/csv_output_test.go | 2 +- pkg/github/issues.go | 70 ++++++--- pkg/github/issues_test.go | 54 +++---- pkg/github/pullrequests.go | 6 +- pkg/github/tools.go | 1 + pkg/github/tools_validation_test.go | 2 +- pkg/http/handler_test.go | 4 +- pkg/inventory/filters.go | 10 +- pkg/inventory/prompts.go | 6 +- pkg/inventory/registry_test.go | 10 +- pkg/inventory/resources.go | 6 +- pkg/inventory/server_tool.go | 6 +- 17 files changed, 286 insertions(+), 105 deletions(-) create mode 100644 pkg/github/__toolsnaps__/issue_write_ff_remote_mcp_issue_fields.snap diff --git a/README.md b/README.md index bec45b5da3..9082a3b642 100644 --- a/README.md +++ b/README.md @@ -855,7 +855,6 @@ The following sets of tools are available: - `assignees`: Usernames to assign to this issue (string[], optional) - `body`: Issue body content (string, optional) - `duplicate_of`: Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'. (number, optional) - - `issue_fields`: Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', or 'delete: true'. (object[], optional) - `issue_number`: Issue number to update (number, optional) - `labels`: Labels to apply to this issue (string[], optional) - `method`: Write operation to perform on a single issue. diff --git a/docs/feature-flags.md b/docs/feature-flags.md index afd6a52c74..74081b2869 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -56,7 +56,6 @@ runtime behavior (such as output formatting) won't appear here. - `assignees`: Usernames to assign to this issue (string[], optional) - `body`: Issue body content (string, optional) - `duplicate_of`: Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'. (number, optional) - - `issue_fields`: Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', or 'delete: true'. (object[], optional) - `issue_number`: Issue number to update (number, optional) - `labels`: Labels to apply to this issue (string[], optional) - `method`: Write operation to perform on a single issue. @@ -74,6 +73,27 @@ runtime behavior (such as output formatting) won't appear here. ### `remote_mcp_issue_fields` +- **issue_write** - Create or update issue + - **Required OAuth Scopes**: `repo` + - `assignees`: Usernames to assign to this issue (string[], optional) + - `body`: Issue body content (string, optional) + - `duplicate_of`: Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'. (number, optional) + - `issue_fields`: Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', or 'delete: true'. (object[], optional) + - `issue_number`: Issue number to update (number, optional) + - `labels`: Labels to apply to this issue (string[], optional) + - `method`: Write operation to perform on a single issue. + Options are: + - 'create' - creates a new issue. + - 'update' - updates an existing issue. + (string, required) + - `milestone`: Milestone number (number, optional) + - `owner`: Repository owner (string, required) + - `repo`: Repository name (string, required) + - `state`: New state (string, optional) + - `state_reason`: Reason for the state change. Ignored unless state is changed. (string, optional) + - `title`: Issue title (string, optional) + - `type`: Type of this issue. Only use if the repository has issue types configured. Use list_issue_types tool to get valid type values for the organization. If the repository doesn't support issue types, omit this parameter. (string, optional) + - **list_issue_fields** - List issue fields - **Required OAuth Scopes**: `repo`, `read:org` - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org` diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 6956a5eaee..881030f020 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -50,7 +50,6 @@ The list below is generated from the Go source. It covers tool **inventory and s - `assignees`: Usernames to assign to this issue (string[], optional) - `body`: Issue body content (string, optional) - `duplicate_of`: Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'. (number, optional) - - `issue_fields`: Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', or 'delete: true'. (object[], optional) - `issue_number`: Issue number to update (number, optional) - `labels`: Labels to apply to this issue (string[], optional) - `method`: Write operation to perform on a single issue. @@ -68,6 +67,27 @@ The list below is generated from the Go source. It covers tool **inventory and s ### `remote_mcp_issue_fields` +- **issue_write** - Create or update issue + - **Required OAuth Scopes**: `repo` + - `assignees`: Usernames to assign to this issue (string[], optional) + - `body`: Issue body content (string, optional) + - `duplicate_of`: Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'. (number, optional) + - `issue_fields`: Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', or 'delete: true'. (object[], optional) + - `issue_number`: Issue number to update (number, optional) + - `labels`: Labels to apply to this issue (string[], optional) + - `method`: Write operation to perform on a single issue. + Options are: + - 'create' - creates a new issue. + - 'update' - updates an existing issue. + (string, required) + - `milestone`: Milestone number (number, optional) + - `owner`: Repository owner (string, required) + - `repo`: Repository name (string, required) + - `state`: New state (string, optional) + - `state_reason`: Reason for the state change. Ignored unless state is changed. (string, optional) + - `title`: Issue title (string, optional) + - `type`: Type of this issue. Only use if the repository has issue types configured. Use list_issue_types tool to get valid type values for the organization. If the repository doesn't support issue types, omit this parameter. (string, optional) + - **list_issue_fields** - List issue fields - **Required OAuth Scopes**: `repo`, `read:org` - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org` diff --git a/pkg/github/__toolsnaps__/issue_write.snap b/pkg/github/__toolsnaps__/issue_write.snap index 6fb00d2490..a125864f04 100644 --- a/pkg/github/__toolsnaps__/issue_write.snap +++ b/pkg/github/__toolsnaps__/issue_write.snap @@ -29,42 +29,6 @@ "description": "Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'.", "type": "number" }, - "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'.", - "items": { - "additionalProperties": false, - "properties": { - "delete": { - "description": "Set to true to clear this field's current value on the issue. Cannot be combined with 'value' or 'field_option_name'.", - "enum": [ - true - ], - "type": "boolean" - }, - "field_name": { - "description": "Issue field name (case-insensitive). Must match a field returned by list_issue_fields for this repository or its organization.", - "type": "string" - }, - "field_option_name": { - "description": "Option name for single-select fields. Validated against the field's options before the API call. Cannot be combined with 'value' or 'delete'.", - "type": "string" - }, - "value": { - "description": "Value to set. Use for text, number, and date fields (date as YYYY-MM-DD). For single-select fields, prefer 'field_option_name' so the option is validated before the API call. Cannot be combined with 'field_option_name' or 'delete'.", - "type": [ - "string", - "number", - "boolean" - ] - } - }, - "required": [ - "field_name" - ], - "type": "object" - }, - "type": "array" - }, "issue_number": { "description": "Issue number to update", "type": "number" diff --git a/pkg/github/__toolsnaps__/issue_write_ff_remote_mcp_issue_fields.snap b/pkg/github/__toolsnaps__/issue_write_ff_remote_mcp_issue_fields.snap new file mode 100644 index 0000000000..6fb00d2490 --- /dev/null +++ b/pkg/github/__toolsnaps__/issue_write_ff_remote_mcp_issue_fields.snap @@ -0,0 +1,133 @@ +{ + "_meta": { + "ui": { + "resourceUri": "ui://github-mcp-server/issue-write", + "visibility": [ + "model", + "app" + ] + } + }, + "annotations": { + "title": "Create or update issue" + }, + "description": "Create a new or update an existing issue in a GitHub repository.", + "inputSchema": { + "properties": { + "assignees": { + "description": "Usernames to assign to this issue", + "items": { + "type": "string" + }, + "type": "array" + }, + "body": { + "description": "Issue body content", + "type": "string" + }, + "duplicate_of": { + "description": "Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'.", + "type": "number" + }, + "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'.", + "items": { + "additionalProperties": false, + "properties": { + "delete": { + "description": "Set to true to clear this field's current value on the issue. Cannot be combined with 'value' or 'field_option_name'.", + "enum": [ + true + ], + "type": "boolean" + }, + "field_name": { + "description": "Issue field name (case-insensitive). Must match a field returned by list_issue_fields for this repository or its organization.", + "type": "string" + }, + "field_option_name": { + "description": "Option name for single-select fields. Validated against the field's options before the API call. Cannot be combined with 'value' or 'delete'.", + "type": "string" + }, + "value": { + "description": "Value to set. Use for text, number, and date fields (date as YYYY-MM-DD). For single-select fields, prefer 'field_option_name' so the option is validated before the API call. Cannot be combined with 'field_option_name' or 'delete'.", + "type": [ + "string", + "number", + "boolean" + ] + } + }, + "required": [ + "field_name" + ], + "type": "object" + }, + "type": "array" + }, + "issue_number": { + "description": "Issue number to update", + "type": "number" + }, + "labels": { + "description": "Labels to apply to this issue", + "items": { + "type": "string" + }, + "type": "array" + }, + "method": { + "description": "Write operation to perform on a single issue.\nOptions are:\n- 'create' - creates a new issue.\n- 'update' - updates an existing issue.\n", + "enum": [ + "create", + "update" + ], + "type": "string" + }, + "milestone": { + "description": "Milestone number", + "type": "number" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "state": { + "description": "New state", + "enum": [ + "open", + "closed" + ], + "type": "string" + }, + "state_reason": { + "description": "Reason for the state change. Ignored unless state is changed.", + "enum": [ + "completed", + "not_planned", + "duplicate" + ], + "type": "string" + }, + "title": { + "description": "Issue title", + "type": "string" + }, + "type": { + "description": "Type of this issue. Only use if the repository has issue types configured. Use list_issue_types tool to get valid type values for the organization. If the repository doesn't support issue types, omit this parameter.", + "type": "string" + } + }, + "required": [ + "method", + "owner", + "repo" + ], + "type": "object" + }, + "name": "issue_write" +} \ No newline at end of file diff --git a/pkg/github/csv_output_test.go b/pkg/github/csv_output_test.go index 246902d498..b9ff2e3edc 100644 --- a/pkg/github/csv_output_test.go +++ b/pkg/github/csv_output_test.go @@ -42,7 +42,7 @@ func TestCSVOutputAppliesToFlagGatedListTools(t *testing.T) { enabledOnly := testCSVOutputTool("list_things", `[{"number":1}]`) enabledOnly.FeatureFlagEnable = FeatureFlagIssueFields disabledOnly := testCSVOutputTool("list_legacy_things", `[{"number":2}]`) - disabledOnly.FeatureFlagDisable = FeatureFlagIssueFields + disabledOnly.FeatureFlagDisable = []string{FeatureFlagIssueFields} tools := withCSVOutput([]inventory.ServerTool{enabledOnly, disabledOnly}) require.Len(t, tools, 2) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 87bfa08af8..ca0622b518 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -884,13 +884,16 @@ func GetIssue(ctx context.Context, client *github.Client, deps ToolDependencies, minimalIssue := convertToMinimalIssue(issue) - // Enrich with field_values via GraphQL for consistency with list_issues/search_issues - if issue != nil && issue.NodeID != nil && *issue.NodeID != "" { - gqlClient, err := deps.GetGQLClient(ctx) - if err == nil { - if fieldValuesByID, err := fetchIssueFieldValuesByNodeID(ctx, gqlClient, []*github.Issue{issue}); err == nil { - minimalIssue.FieldValues = fieldValuesByID[*issue.NodeID] - minimalIssue.IssueFieldValues = nil // Clear verbose REST format + // Always drop the verbose REST IssueFieldValues; only enrich with the GraphQL + // field_values view when the issue-fields feature flag is on. + minimalIssue.IssueFieldValues = nil + if deps.IsFeatureEnabled(ctx, FeatureFlagIssueFields) { + if issue != nil && issue.NodeID != nil && *issue.NodeID != "" { + gqlClient, err := deps.GetGQLClient(ctx) + if err == nil { + if fieldValuesByID, err := fetchIssueFieldValuesByNodeID(ctx, gqlClient, []*github.Issue{issue}); err == nil { + minimalIssue.FieldValues = fieldValuesByID[*issue.NodeID] + } } } } @@ -1331,7 +1334,7 @@ Options are: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil } }) - st.FeatureFlagDisable = FeatureFlagIssuesGranular + st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular} return st } @@ -1754,11 +1757,33 @@ func searchIssuesHandler(ctx context.Context, deps ToolDependencies, args map[st return callResult, nil } -// IssueWrite creates a tool to create a new or update an existing issue in a GitHub repository. // IssueWriteUIResourceURI is the URI for the issue_write tool's MCP App UI resource. const IssueWriteUIResourceURI = "ui://github-mcp-server/issue-write" +// IssueWrite is the FeatureFlagIssueFields-enabled variant of issue_write +// (with the issue_fields parameter). LegacyIssueWrite is served when the flag +// is off. Both register under the tool name "issue_write"; exactly one is +// active at a time via mutually exclusive feature-flag annotations. Delete the +// LegacyIssueWrite block (and the includeIssueFields parameter) when the flag +// is removed. func IssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { + st := buildIssueWrite(t, true) + st.FeatureFlagEnable = FeatureFlagIssueFields + st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular} + return st +} + +// LegacyIssueWrite is the FeatureFlagIssueFields-disabled variant of issue_write. +// It exposes the pre-issue-fields schema (no issue_fields parameter) and skips +// the custom field value resolution. Hidden whenever the granular toolset or +// the issue-fields flag is on. +func LegacyIssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { + st := buildIssueWrite(t, false) + st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular, FeatureFlagIssueFields} + return st +} + +func buildIssueWrite(t translations.TranslationHelperFunc, includeIssueFields bool) inventory.ServerTool { st := NewTool( ToolsetMetadataIssues, mcp.Tool{ @@ -1978,9 +2003,12 @@ Options are: return utils.NewToolResultError("duplicate_of can only be used when state_reason is 'duplicate'"), nil, nil } - issueFields, err := optionalIssueWriteFields(args) - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + var issueFields []issueWriteFieldInput + if includeIssueFields { + issueFields, err = optionalIssueWriteFields(args) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } } client, err := deps.GetClient(ctx) @@ -1993,9 +2021,13 @@ Options are: return utils.NewToolResultErrorFromErr("failed to get GraphQL client", err), nil, nil } - issueFieldValues, fieldIDsToDelete, err := resolveIssueRequestFieldValues(ctx, gqlClient, owner, repo, issueFields) - if err != nil { - return utils.NewToolResultError(fmt.Sprintf("failed to resolve issue_fields: %v", err)), nil, nil + var issueFieldValues []*github.IssueRequestFieldValue + var fieldIDsToDelete []int64 + if len(issueFields) > 0 { + issueFieldValues, fieldIDsToDelete, err = resolveIssueRequestFieldValues(ctx, gqlClient, owner, repo, issueFields) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to resolve issue_fields: %v", err)), nil, nil + } } switch method { @@ -2013,7 +2045,11 @@ Options are: return utils.NewToolResultError("invalid method, must be either 'create' or 'update'"), nil, nil } }) - st.FeatureFlagDisable = FeatureFlagIssuesGranular + if !includeIssueFields { + if schema, ok := st.Tool.InputSchema.(*jsonschema.Schema); ok { + delete(schema.Properties, "issue_fields") + } + } return st } @@ -2690,7 +2726,7 @@ func LegacyListIssues(t translations.TranslationHelperFunc) inventory.ServerTool } return result, nil, nil }) - st.FeatureFlagDisable = FeatureFlagIssueFields + st.FeatureFlagDisable = []string{FeatureFlagIssueFields} return st } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 3ca2ae9a75..2e37f08257 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -394,7 +394,9 @@ func Test_IssueRead_IFC_InsidersMode(t *testing.T) { } func Test_GetIssue_FieldValues(t *testing.T) { - // Verify that issue_field_values from the REST API are present in the returned object. + // Verify that issue_field_values from the REST API are NOT exposed when the + // remote_mcp_issue_fields flag is off. The raw REST format is always cleared; + // enriched field_values are only populated when the flag is on. serverTool := IssueRead(translations.NullTranslationHelper) mockIssueWithFields := &github.Issue{ @@ -457,24 +459,9 @@ func Test_GetIssue_FieldValues(t *testing.T) { err = json.Unmarshal([]byte(textContent.Text), &returnedIssue) require.NoError(t, err) - require.Len(t, returnedIssue.IssueFieldValues, 2, "expected two issue field values") - - first := returnedIssue.IssueFieldValues[0] - assert.Equal(t, int64(1001), first.IssueFieldID) - assert.Equal(t, "FV_node_1", first.NodeID) - assert.Equal(t, "single_select", first.DataType) - assert.Equal(t, "High", first.Value) - require.NotNil(t, first.SingleSelectOption) - assert.Equal(t, int64(42), first.SingleSelectOption.ID) - assert.Equal(t, "High", first.SingleSelectOption.Name) - assert.Equal(t, "red", first.SingleSelectOption.Color) - - second := returnedIssue.IssueFieldValues[1] - assert.Equal(t, int64(1002), second.IssueFieldID) - assert.Equal(t, "FV_node_2", second.NodeID) - assert.Equal(t, "text", second.DataType) - assert.Equal(t, "some text value", second.Value) - assert.Nil(t, second.SingleSelectOption) + // Flag is off: raw REST IssueFieldValues must be cleared, enriched FieldValues absent. + assert.Empty(t, returnedIssue.IssueFieldValues, "raw REST issue_field_values should not be exposed when flag is off") + assert.Empty(t, returnedIssue.FieldValues, "enriched field_values should not be present when flag is off") } func Test_AddIssueComment(t *testing.T) { @@ -1196,10 +1183,11 @@ func Test_SearchIssues_FieldValuesEnrichment(t *testing.T) { } func Test_CreateIssue(t *testing.T) { - // Verify tool definition once + // Verify tool definition once (flag-enabled variant snap) serverTool := IssueWrite(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagIssueFields, tool)) + require.Equal(t, FeatureFlagIssueFields, serverTool.FeatureFlagEnable) assert.Equal(t, "issue_write", tool.Name) assert.NotEmpty(t, tool.Description) @@ -2551,7 +2539,7 @@ func Test_LegacyListIssues_Definition(t *testing.T) { // owns list_issues_ff_.snap. require.NoError(t, toolsnaps.Test(tool.Name, tool)) require.Equal(t, "list_issues", tool.Name) - require.Equal(t, FeatureFlagIssueFields, serverTool.FeatureFlagDisable) + require.Equal(t, []string{FeatureFlagIssueFields}, serverTool.FeatureFlagDisable) require.Empty(t, serverTool.FeatureFlagEnable) props := tool.InputSchema.(*jsonschema.Schema).Properties @@ -2563,6 +2551,24 @@ func Test_LegacyListIssues_Definition(t *testing.T) { assert.NotContains(t, props, "field_filters", "legacy list_issues must not advertise field_filters") } +func Test_LegacyIssueWrite_Definition(t *testing.T) { + serverTool := LegacyIssueWrite(translations.NullTranslationHelper) + tool := serverTool.Tool + + // LegacyIssueWrite owns the canonical issue_write.snap; the + // FeatureFlagIssueFields-enabled variant owns issue_write_ff_.snap. + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + require.Equal(t, "issue_write", tool.Name) + require.Equal(t, []string{FeatureFlagIssuesGranular, FeatureFlagIssueFields}, serverTool.FeatureFlagDisable) + require.Empty(t, serverTool.FeatureFlagEnable) + + props := tool.InputSchema.(*jsonschema.Schema).Properties + assert.Contains(t, props, "method") + assert.Contains(t, props, "owner") + assert.Contains(t, props, "repo") + assert.NotContains(t, props, "issue_fields", "legacy issue_write must not advertise issue_fields") +} + func Test_LegacyListIssues_OmitsFieldValuesAndFilters(t *testing.T) { t.Parallel() @@ -2631,10 +2637,10 @@ func Test_LegacyListIssues_OmitsFieldValuesAndFilters(t *testing.T) { } func Test_UpdateIssue(t *testing.T) { - // Verify tool definition + // Verify tool definition (flag-enabled variant snap) serverTool := IssueWrite(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagIssueFields, tool)) assert.Equal(t, "issue_write", tool.Name) assert.NotEmpty(t, tool.Description) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index c298d875a1..3910a96b95 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1000,7 +1000,7 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo return utils.NewToolResultText(string(r)), nil, nil }) - st.FeatureFlagDisable = FeatureFlagPullRequestsGranular + st.FeatureFlagDisable = []string{FeatureFlagPullRequestsGranular} return st } @@ -1619,7 +1619,7 @@ Available methods: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", params.Method)), nil, nil } }) - st.FeatureFlagDisable = FeatureFlagPullRequestsGranular + st.FeatureFlagDisable = []string{FeatureFlagPullRequestsGranular} return st } @@ -2116,7 +2116,7 @@ func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.S }) return result, nil, err }) - st.FeatureFlagDisable = FeatureFlagPullRequestsGranular + st.FeatureFlagDisable = []string{FeatureFlagPullRequestsGranular} return st } diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 49edb00fff..d1d585b3fa 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -208,6 +208,7 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { ListIssueTypes(t), ListIssueFields(t), IssueWrite(t), + LegacyIssueWrite(t), AddIssueComment(t), SubIssueWrite(t), diff --git a/pkg/github/tools_validation_test.go b/pkg/github/tools_validation_test.go index 0a4a4eb7b0..1db85b2fc1 100644 --- a/pkg/github/tools_validation_test.go +++ b/pkg/github/tools_validation_test.go @@ -116,7 +116,7 @@ func TestNoDuplicateToolNames(t *testing.T) { // First pass: identify tools that have feature flags (mutually exclusive at runtime) for _, tool := range tools { - if tool.FeatureFlagEnable != "" || tool.FeatureFlagDisable != "" { + if tool.FeatureFlagEnable != "" || len(tool.FeatureFlagDisable) > 0 { featureFlagged[tool.Tool.Name] = true } } diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index 74e28a6e44..a36469133c 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -58,7 +58,9 @@ var _ scopes.FetcherInterface = allScopesFetcher{} func mockToolWithFeatureFlag(name, toolsetID string, readOnly bool, enableFlag, disableFlag string) inventory.ServerTool { tool := mockTool(name, toolsetID, readOnly) tool.FeatureFlagEnable = enableFlag - tool.FeatureFlagDisable = disableFlag + if disableFlag != "" { + tool.FeatureFlagDisable = []string{disableFlag} + } return tool } diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index e2effd8ca7..fd3579fa6f 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "slices" "sort" ) @@ -42,8 +43,8 @@ func (r *Inventory) checkFeatureFlag(ctx context.Context, flagName string) bool // installed when WithFeatureChecker received a non-nil checker). // // - If FeatureFlagEnable is set, the item is only allowed if the flag is enabled. -// - If FeatureFlagDisable is set, the item is excluded if the flag is enabled. -func featureFlagAllowed(ctx context.Context, checker FeatureFlagChecker, enableFlag, disableFlag string) bool { +// - If FeatureFlagDisable is non-empty, the item is excluded if any listed flag is enabled. +func featureFlagAllowed(ctx context.Context, checker FeatureFlagChecker, enableFlag string, disableFlags []string) bool { // Error semantics match the previous checkFeatureFlag helper: a checker // error is logged and treated as "flag not enabled". So an enable-flag // check on error excludes the tool, but a disable-flag check on error @@ -59,10 +60,7 @@ func featureFlagAllowed(ctx context.Context, checker FeatureFlagChecker, enableF if enableFlag != "" && !check(enableFlag) { return false } - if disableFlag != "" && check(disableFlag) { - return false - } - return true + return !slices.ContainsFunc(disableFlags, check) } // createFeatureFlagFilter returns a ToolFilter that gates tools on their diff --git a/pkg/inventory/prompts.go b/pkg/inventory/prompts.go index 648f20f9cd..d929578e83 100644 --- a/pkg/inventory/prompts.go +++ b/pkg/inventory/prompts.go @@ -11,9 +11,9 @@ type ServerPrompt struct { // FeatureFlagEnable specifies a feature flag that must be enabled for this prompt // to be available. If set and the flag is not enabled, the prompt is omitted. FeatureFlagEnable string - // FeatureFlagDisable specifies a feature flag that, when enabled, causes this prompt - // to be omitted. Used to disable prompts when a feature flag is on. - FeatureFlagDisable string + // FeatureFlagDisable specifies feature flags that, when any is enabled, cause this + // prompt to be omitted. Used to disable prompts when a feature flag is on. + FeatureFlagDisable []string } // NewServerPrompt creates a new ServerPrompt with toolset metadata. diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 75de9c574a..372f756023 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1047,7 +1047,9 @@ func TestMCPMethodConstants(t *testing.T) { func mockToolWithFlags(name string, toolsetID string, readOnly bool, enableFlag, disableFlag string) ServerTool { tool := mockTool(name, toolsetID, readOnly) tool.FeatureFlagEnable = enableFlag - tool.FeatureFlagDisable = disableFlag + if disableFlag != "" { + tool.FeatureFlagDisable = []string{disableFlag} + } return tool } @@ -1723,8 +1725,8 @@ func TestForMCPRequest_ToolsCall_FeatureFlaggedVariants(t *testing.T) { if len(availableOff) != 1 { t.Fatalf("Flag OFF: Expected 1 tool, got %d", len(availableOff)) } - if availableOff[0].FeatureFlagDisable != "consolidated_flag" { - t.Errorf("Flag OFF: Expected tool with FeatureFlagDisable, got FeatureFlagEnable=%q, FeatureFlagDisable=%q", + if len(availableOff[0].FeatureFlagDisable) != 1 || availableOff[0].FeatureFlagDisable[0] != "consolidated_flag" { + t.Errorf("Flag OFF: Expected tool with FeatureFlagDisable, got FeatureFlagEnable=%q, FeatureFlagDisable=%v", availableOff[0].FeatureFlagEnable, availableOff[0].FeatureFlagDisable) } @@ -1742,7 +1744,7 @@ func TestForMCPRequest_ToolsCall_FeatureFlaggedVariants(t *testing.T) { t.Fatalf("Flag ON: Expected 1 tool, got %d", len(availableOn)) } if availableOn[0].FeatureFlagEnable != "consolidated_flag" { - t.Errorf("Flag ON: Expected tool with FeatureFlagEnable, got FeatureFlagEnable=%q, FeatureFlagDisable=%q", + t.Errorf("Flag ON: Expected tool with FeatureFlagEnable, got FeatureFlagEnable=%q, FeatureFlagDisable=%v", availableOn[0].FeatureFlagEnable, availableOn[0].FeatureFlagDisable) } } diff --git a/pkg/inventory/resources.go b/pkg/inventory/resources.go index 6de037d584..2dd07ae0fe 100644 --- a/pkg/inventory/resources.go +++ b/pkg/inventory/resources.go @@ -19,9 +19,9 @@ type ServerResourceTemplate struct { // FeatureFlagEnable specifies a feature flag that must be enabled for this resource // to be available. If set and the flag is not enabled, the resource is omitted. FeatureFlagEnable string - // FeatureFlagDisable specifies a feature flag that, when enabled, causes this resource - // to be omitted. Used to disable resources when a feature flag is on. - FeatureFlagDisable string + // FeatureFlagDisable specifies feature flags that, when any is enabled, cause this + // resource to be omitted. Used to disable resources when a feature flag is on. + FeatureFlagDisable []string } // HasHandler returns true if this resource has a handler function. diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index 41d38b7ec2..326009b59f 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -64,9 +64,9 @@ type ServerTool struct { // to be available. If set and the flag is not enabled, the tool is omitted. FeatureFlagEnable string - // FeatureFlagDisable specifies a feature flag that, when enabled, causes this tool - // to be omitted. Used to disable tools when a feature flag is on. - FeatureFlagDisable string + // FeatureFlagDisable specifies feature flags that, when any is enabled, cause this + // tool to be omitted. Used to disable tools when a feature flag is on. + FeatureFlagDisable []string // Enabled is an optional function called at build/filter time to determine // if this tool should be available. If nil, the tool is considered enabled From 39dfc2fff01a970eaff00f25571df529ab653238 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 27 May 2026 17:08:54 +0200 Subject: [PATCH 2/4] refactor: inline IssueWrite/LegacyIssueWrite as full duplicates Replace the shared buildIssueWrite(includeIssueFields) helper with two fully duplicated tool definitions. When the FeatureFlagIssueFields flag is retired, LegacyIssueWrite can be deleted as a single function with no merge thinking required. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/issues.go | 258 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 228 insertions(+), 30 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index ca0622b518..50708d556a 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1763,27 +1763,10 @@ const IssueWriteUIResourceURI = "ui://github-mcp-server/issue-write" // IssueWrite is the FeatureFlagIssueFields-enabled variant of issue_write // (with the issue_fields parameter). LegacyIssueWrite is served when the flag // is off. Both register under the tool name "issue_write"; exactly one is -// active at a time via mutually exclusive feature-flag annotations. Delete the -// LegacyIssueWrite block (and the includeIssueFields parameter) when the flag -// is removed. +// active at a time via mutually exclusive feature-flag annotations. When the +// flag is removed, delete LegacyIssueWrite outright and drop the feature-flag +// fields on IssueWrite. func IssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { - st := buildIssueWrite(t, true) - st.FeatureFlagEnable = FeatureFlagIssueFields - st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular} - return st -} - -// LegacyIssueWrite is the FeatureFlagIssueFields-disabled variant of issue_write. -// It exposes the pre-issue-fields schema (no issue_fields parameter) and skips -// the custom field value resolution. Hidden whenever the granular toolset or -// the issue-fields flag is on. -func LegacyIssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { - st := buildIssueWrite(t, false) - st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular, FeatureFlagIssueFields} - return st -} - -func buildIssueWrite(t translations.TranslationHelperFunc, includeIssueFields bool) inventory.ServerTool { st := NewTool( ToolsetMetadataIssues, mcp.Tool{ @@ -2004,11 +1987,9 @@ Options are: } var issueFields []issueWriteFieldInput - if includeIssueFields { - issueFields, err = optionalIssueWriteFields(args) - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } + issueFields, err = optionalIssueWriteFields(args) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } client, err := deps.GetClient(ctx) @@ -2045,11 +2026,228 @@ Options are: return utils.NewToolResultError("invalid method, must be either 'create' or 'update'"), nil, nil } }) - if !includeIssueFields { - if schema, ok := st.Tool.InputSchema.(*jsonschema.Schema); ok { - delete(schema.Properties, "issue_fields") - } - } + st.FeatureFlagEnable = FeatureFlagIssueFields + st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular} + return st +} + +// LegacyIssueWrite is the FeatureFlagIssueFields-disabled variant of issue_write. +// It is a near-verbatim copy of IssueWrite minus the issue_fields schema +// property, the issue_fields handler block, and the related GraphQL field +// resolution. Kept as a full duplicate so removing the FeatureFlagIssueFields +// flag is a single-function delete. Hidden whenever the granular toolset or +// the issue-fields flag is on. +func LegacyIssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataIssues, + mcp.Tool{ + Name: "issue_write", + Description: t("TOOL_ISSUE_WRITE_DESCRIPTION", "Create a new or update an existing issue in a GitHub repository."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_ISSUE_WRITE_USER_TITLE", "Create or update issue"), + ReadOnlyHint: false, + }, + Meta: mcp.Meta{ + "ui": map[string]any{ + "resourceUri": IssueWriteUIResourceURI, + "visibility": []string{"model", "app"}, + }, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "method": { + Type: "string", + Description: `Write operation to perform on a single issue. +Options are: +- 'create' - creates a new issue. +- 'update' - updates an existing issue. +`, + Enum: []any{"create", "update"}, + }, + "owner": { + Type: "string", + Description: "Repository owner", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "issue_number": { + Type: "number", + Description: "Issue number to update", + }, + "title": { + Type: "string", + Description: "Issue title", + }, + "body": { + Type: "string", + Description: "Issue body content", + }, + "assignees": { + Type: "array", + Description: "Usernames to assign to this issue", + Items: &jsonschema.Schema{ + Type: "string", + }, + }, + "labels": { + Type: "array", + Description: "Labels to apply to this issue", + Items: &jsonschema.Schema{ + Type: "string", + }, + }, + "milestone": { + Type: "number", + Description: "Milestone number", + }, + "type": { + Type: "string", + Description: "Type of this issue. Only use if the repository has issue types configured. Use list_issue_types tool to get valid type values for the organization. If the repository doesn't support issue types, omit this parameter.", + }, + "state": { + Type: "string", + Description: "New state", + Enum: []any{"open", "closed"}, + }, + "state_reason": { + Type: "string", + Description: "Reason for the state change. Ignored unless state is changed.", + Enum: []any{"completed", "not_planned", "duplicate"}, + }, + "duplicate_of": { + Type: "number", + Description: "Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'.", + }, + }, + Required: []string{"method", "owner", "repo"}, + }, + }, + []scopes.Scope{scopes.Repo}, + func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + method, err := RequiredParam[string](args, "method") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + // When MCP Apps are enabled and the client supports UI, + // check if this is a UI form submission. The UI sends _ui_submitted=true + // to distinguish form submissions from LLM calls. + uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted") + + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted { + if method == "update" { + // Skip the UI form when a state change is requested because + // the form only handles title/body editing and would lose the + // state transition (e.g. closing or reopening the issue). + if _, hasState := args["state"]; !hasState { + issueNumber, numErr := RequiredInt(args, "issue_number") + if numErr != nil { + return utils.NewToolResultError("issue_number is required for update method"), nil, nil + } + return utils.NewToolResultText(fmt.Sprintf("Ready to update issue #%d in %s/%s. IMPORTANT: The issue has NOT been updated yet. Do NOT tell the user the issue was updated. The user MUST click Submit in the form to update it.", issueNumber, owner, repo)), nil, nil + } + } else { + return utils.NewToolResultText(fmt.Sprintf("Ready to create an issue in %s/%s. IMPORTANT: The issue has NOT been created yet. Do NOT tell the user the issue was created. The user MUST click Submit in the form to create it.", owner, repo)), nil, nil + } + } + + title, err := OptionalParam[string](args, "title") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + // Optional parameters + body, err := OptionalParam[string](args, "body") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + // Get assignees + assignees, err := OptionalStringArrayParam(args, "assignees") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + // Get labels + labels, err := OptionalStringArrayParam(args, "labels") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + // Get optional milestone + milestone, err := OptionalIntParam(args, "milestone") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + var milestoneNum int + if milestone != 0 { + milestoneNum = milestone + } + + // Get optional type + issueType, err := OptionalParam[string](args, "type") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + // Handle state, state_reason and duplicateOf parameters + state, err := OptionalParam[string](args, "state") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + stateReason, err := OptionalParam[string](args, "state_reason") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + duplicateOf, err := OptionalIntParam(args, "duplicate_of") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + if duplicateOf != 0 && stateReason != "duplicate" { + return utils.NewToolResultError("duplicate_of can only be used when state_reason is 'duplicate'"), nil, nil + } + + client, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } + + gqlClient, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GraphQL client", err), nil, nil + } + + switch method { + case "create": + result, err := CreateIssue(ctx, client, owner, repo, title, body, assignees, labels, milestoneNum, issueType, nil) + return result, nil, err + case "update": + issueNumber, err := RequiredInt(args, "issue_number") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + result, err := UpdateIssue(ctx, client, gqlClient, owner, repo, issueNumber, title, body, assignees, labels, milestoneNum, issueType, nil, nil, state, stateReason, duplicateOf) + return result, nil, err + default: + return utils.NewToolResultError("invalid method, must be either 'create' or 'update'"), nil, nil + } + }) + st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular, FeatureFlagIssueFields} return st } From eb1aed86880034fb87aeff2439f238d7591cd4ef Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 27 May 2026 17:16:10 +0200 Subject: [PATCH 3/4] test: add Test_GetIssue_FieldValues_FlagOn to cover the enrichment path Companion to Test_GetIssue_FieldValues: when remote_mcp_issue_fields is enabled, the GraphQL nodes() round-trip populates the enriched field_values while the raw REST issue_field_values stays cleared. Addresses the Copilot review suggestion on #2558. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/issues_test.go | 96 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 2e37f08257..c2be1984f7 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -464,6 +464,102 @@ func Test_GetIssue_FieldValues(t *testing.T) { assert.Empty(t, returnedIssue.FieldValues, "enriched field_values should not be present when flag is off") } +func Test_GetIssue_FieldValues_FlagOn(t *testing.T) { + // Verify the enriched field_values are populated via GraphQL when the + // remote_mcp_issue_fields flag is on, and the raw REST issue_field_values + // stays cleared. + serverTool := IssueRead(translations.NullTranslationHelper) + + mockIssueWithFields := &github.Issue{ + Number: github.Ptr(99), + NodeID: github.Ptr("I_node_99"), + Title: github.Ptr("Issue with field values"), + Body: github.Ptr("body"), + State: github.Ptr("open"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/99"), + User: &github.User{ + Login: github.Ptr("testuser"), + }, + IssueFieldValues: []*github.IssueFieldValue{ + { + IssueFieldID: 1001, + NodeID: "FV_node_1", + DataType: "single_select", + Value: "High", + }, + }, + } + + restClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockIssueWithFields), + }) + + gqlVars := map[string]any{ + "ids": []any{"I_node_99"}, + } + gqlResponse := githubv4mock.DataResponse(map[string]any{ + "nodes": []map[string]any{ + { + "id": "I_node_99", + "issueFieldValues": map[string]any{ + "nodes": []map[string]any{ + { + "__typename": "IssueFieldSingleSelectValue", + "field": map[string]any{"name": "priority"}, + "value": "P1", + }, + { + "__typename": "IssueFieldNumberValue", + "field": map[string]any{"name": "estimate"}, + "valueNumber": 2.5, + }, + }, + }, + }, + }, + }) + + const nodesQueryString = "query($ids:[ID!]!){nodes(ids: $ids){... on Issue{id,issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}}}}" + matcher := githubv4mock.NewQueryMatcher(nodesQueryString, gqlVars, gqlResponse) + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matcher)) + + cache := stubRepoAccessCache(nil, 15*time.Minute) + deps := BaseDeps{ + Client: mustNewGHClient(t, restClient), + GQLClient: gqlClient, + RepoAccessCache: cache, + featureChecker: featureCheckerFor(FeatureFlagIssueFields), + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "method": "get", + "owner": "owner", + "repo": "repo", + "issue_number": float64(99), + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.NotNil(t, result) + require.False(t, result.IsError, "expected result to not be an error") + + textContent := getTextResult(t, result) + + var returnedIssue MinimalIssue + err = json.Unmarshal([]byte(textContent.Text), &returnedIssue) + require.NoError(t, err) + + // Raw REST IssueFieldValues is always cleared, even when flag is on. + assert.Empty(t, returnedIssue.IssueFieldValues, "raw REST issue_field_values should not be exposed even when flag is on") + + // Enriched FieldValues comes from the GraphQL nodes() round-trip. + require.Len(t, returnedIssue.FieldValues, 2, "field_values should be populated from GraphQL when flag is on") + assert.Equal(t, "priority", returnedIssue.FieldValues[0].Field) + assert.Equal(t, "P1", returnedIssue.FieldValues[0].Value) + assert.Equal(t, "estimate", returnedIssue.FieldValues[1].Field) + assert.Equal(t, "2.5", returnedIssue.FieldValues[1].Value) +} + func Test_AddIssueComment(t *testing.T) { // Verify tool definition once serverTool := AddIssueComment(translations.NullTranslationHelper) From 0ee3ef68a0ce671ecb060c69cb3152f3a045eed4 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 28 May 2026 11:23:17 +0200 Subject: [PATCH 4/4] fix: make mergeIssueFieldValues output order deterministic mergeIssueFieldValues built the merged slice by iterating a Go map, which produces non-deterministic ordering and caused a flake in Test_UpdateIssue/partial_update_with_issue_fields_reconciled_by_names (introduced in #2551). Switch to an order-preserving merge: emit incoming entries first in their original order, then any existing entries (in their original order) whose field IDs weren't seen in incoming. Semantics (incoming wins, existing preserved) unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/issues.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 50708d556a..0469789812 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -460,16 +460,19 @@ func fetchExistingIssueFieldValues(ctx context.Context, gqlClient *githubv4.Clie // mergeIssueFieldValues returns a merged slice where incoming values override existing ones // for the same field ID, and existing fields not present in incoming are preserved. +// Ordering is deterministic: incoming entries first in their original order, followed by any +// existing entries (in their original order) whose field IDs weren't seen in incoming. func mergeIssueFieldValues(existing, incoming []*github.IssueRequestFieldValue) []*github.IssueRequestFieldValue { - merged := make(map[int64]*github.IssueRequestFieldValue, len(existing)+len(incoming)) - for _, v := range existing { - merged[v.FieldID] = v - } + seen := make(map[int64]struct{}, len(incoming)) + result := make([]*github.IssueRequestFieldValue, 0, len(existing)+len(incoming)) for _, v := range incoming { - merged[v.FieldID] = v + seen[v.FieldID] = struct{}{} + result = append(result, v) } - result := make([]*github.IssueRequestFieldValue, 0, len(merged)) - for _, v := range merged { + for _, v := range existing { + if _, ok := seen[v.FieldID]; ok { + continue + } result = append(result, v) } return result