From ccad8f8996198fca756d70a149c95a0aaa9feefa Mon Sep 17 00:00:00 2001 From: Kelsey Myers <52179263+kelsey-myers@users.noreply.github.com> Date: Tue, 26 May 2026 15:27:40 -0700 Subject: [PATCH 1/4] feat: gate issue_write and get_issue behind remote_mcp_issue_fields flag - Extend FeatureFlagEnable/Disable fields from string to []string (AND semantics for enable, OR semantics for disable) so a tool can require multiple flags simultaneously. - Add LegacyIssueWrite: 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. Both this and IssueWrite register under the tool name 'issue_write'; exactly one is active at a time via mutually exclusive flag annotations. - Gate IssueWrite (the flag-enabled variant) with FeatureFlagEnable so it is only served when remote_mcp_issue_fields is on. - Gate the get_issue field_values enrichment with a runtime IsFeatureEnabled check so the GraphQL round-trip is skipped when the flag is off. - Add issue_write.snap (legacy) and issue_write_ff_remote_mcp_issue_fields.snap (flag-enabled) toolsnaps following the convention established by PR #2520. - Modernise featureFlagAllowed disableFlags loop to slices.ContainsFunc. --- README.md | 1 - docs/feature-flags.md | 22 +- docs/insiders-features.md | 22 +- pkg/github/__toolsnaps__/issue_write.snap | 51 ---- ...ssue_write_ff_remote_mcp_issue_fields.snap | 148 +++++++++++ pkg/github/csv_output_test.go | 4 +- pkg/github/granular_tools_test.go | 6 +- pkg/github/issue_fields.go | 2 +- pkg/github/issues.go | 235 +++++++++++++++++- pkg/github/issues_granular.go | 16 +- pkg/github/issues_test.go | 31 ++- pkg/github/pullrequests.go | 6 +- pkg/github/pullrequests_granular.go | 18 +- pkg/github/tools.go | 1 + pkg/github/tools_validation_test.go | 2 +- pkg/http/handler_test.go | 10 +- pkg/inventory/filters.go | 14 +- pkg/inventory/prompts.go | 10 +- pkg/inventory/registry_test.go | 16 +- pkg/inventory/resources.go | 10 +- pkg/inventory/server_tool.go | 12 +- 21 files changed, 508 insertions(+), 129 deletions(-) create mode 100644 pkg/github/__toolsnaps__/issue_write_ff_remote_mcp_issue_fields.snap diff --git a/README.md b/README.md index 1b99bb6752..b387b61f15 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. Each item requires 'field_name' and exactly one of 'value' or 'field_option_name'. (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 2d00e6ad90..95e31c9f50 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. Each item requires 'field_name' and exactly one of 'value' or 'field_option_name'. (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. Each item requires 'field_name' and exactly one of 'value' or 'field_option_name'. (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 c102138dfa..bb9d664eae 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. Each item requires 'field_name' and exactly one of 'value' or 'field_option_name'. (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. Each item requires 'field_name' and exactly one of 'value' or 'field_option_name'. (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 2248dad907..a125864f04 100644 --- a/pkg/github/__toolsnaps__/issue_write.snap +++ b/pkg/github/__toolsnaps__/issue_write.snap @@ -29,57 +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. Each item requires 'field_name' and exactly one of 'value' or 'field_option_name'.", - "items": { - "additionalProperties": false, - "oneOf": [ - { - "not": { - "required": [ - "field_option_name" - ] - }, - "required": [ - "value" - ] - }, - { - "not": { - "required": [ - "value" - ] - }, - "required": [ - "field_option_name" - ] - } - ], - "properties": { - "field_name": { - "description": "Issue field name", - "type": "string" - }, - "field_option_name": { - "description": "Option name for single-select fields — validates the option exists in the field definition before setting it.", - "type": "string" - }, - "value": { - "description": "Value to set. For single-select fields, prefer 'field_option_name' to validate the option exists first.", - "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..2248dad907 --- /dev/null +++ b/pkg/github/__toolsnaps__/issue_write_ff_remote_mcp_issue_fields.snap @@ -0,0 +1,148 @@ +{ + "_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. Each item requires 'field_name' and exactly one of 'value' or 'field_option_name'.", + "items": { + "additionalProperties": false, + "oneOf": [ + { + "not": { + "required": [ + "field_option_name" + ] + }, + "required": [ + "value" + ] + }, + { + "not": { + "required": [ + "value" + ] + }, + "required": [ + "field_option_name" + ] + } + ], + "properties": { + "field_name": { + "description": "Issue field name", + "type": "string" + }, + "field_option_name": { + "description": "Option name for single-select fields — validates the option exists in the field definition before setting it.", + "type": "string" + }, + "value": { + "description": "Value to set. For single-select fields, prefer 'field_option_name' to validate the option exists first.", + "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..fa1c7651c1 100644 --- a/pkg/github/csv_output_test.go +++ b/pkg/github/csv_output_test.go @@ -40,9 +40,9 @@ func TestCSVOutputAppliedToDefaultListTools(t *testing.T) { func TestCSVOutputAppliesToFlagGatedListTools(t *testing.T) { enabledOnly := testCSVOutputTool("list_things", `[{"number":1}]`) - enabledOnly.FeatureFlagEnable = FeatureFlagIssueFields + enabledOnly.FeatureFlagEnable = []string{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/granular_tools_test.go b/pkg/github/granular_tools_test.go index 88bd560b4f..834a63bf06 100644 --- a/pkg/github/granular_tools_test.go +++ b/pkg/github/granular_tools_test.go @@ -20,7 +20,7 @@ import ( func granularToolsForToolset(toolsetID inventory.ToolsetID, featureFlag string) []inventory.ServerTool { var result []inventory.ServerTool for _, tool := range AllTools(translations.NullTranslationHelper) { - if tool.Toolset.ID == toolsetID && tool.FeatureFlagEnable == featureFlag { + if tool.Toolset.ID == toolsetID && len(tool.FeatureFlagEnable) > 0 && tool.FeatureFlagEnable[0] == featureFlag { result = append(result, tool) } } @@ -94,7 +94,7 @@ func TestIssuesGranularToolset(t *testing.T) { t.Run("all granular tools have correct feature flag", func(t *testing.T) { for _, tool := range granularToolsForToolset(ToolsetMetadataIssues.ID, FeatureFlagIssuesGranular) { - assert.Equal(t, FeatureFlagIssuesGranular, tool.FeatureFlagEnable, "tool %s", tool.Tool.Name) + assert.Equal(t, []string{FeatureFlagIssuesGranular}, tool.FeatureFlagEnable, "tool %s", tool.Tool.Name) } }) } @@ -129,7 +129,7 @@ func TestPullRequestsGranularToolset(t *testing.T) { t.Run("all granular tools have correct feature flag", func(t *testing.T) { for _, tool := range granularToolsForToolset(ToolsetMetadataPullRequests.ID, FeatureFlagPullRequestsGranular) { - assert.Equal(t, FeatureFlagPullRequestsGranular, tool.FeatureFlagEnable, "tool %s", tool.Tool.Name) + assert.Equal(t, []string{FeatureFlagPullRequestsGranular}, tool.FeatureFlagEnable, "tool %s", tool.Tool.Name) } }) } diff --git a/pkg/github/issue_fields.go b/pkg/github/issue_fields.go index 1eabbc02f3..6d8bb9b964 100644 --- a/pkg/github/issue_fields.go +++ b/pkg/github/issue_fields.go @@ -157,7 +157,7 @@ func ListIssueFields(t translations.TranslationHelperFunc) inventory.ServerTool return utils.NewToolResultText(string(r)), nil, nil }) - st.FeatureFlagEnable = FeatureFlagIssueFields + st.FeatureFlagEnable = []string{FeatureFlagIssueFields} return st } diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 4cc3540294..fdfabaa581 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -759,13 +759,17 @@ 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 + // Enrich with field_values via GraphQL for consistency with list_issues/search_issues. + // Gated behind FeatureFlagIssueFields so the GraphQL round-trip is only paid when the + // feature is active. + 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] + minimalIssue.IssueFieldValues = nil // Clear verbose REST format + } } } } @@ -1206,7 +1210,7 @@ Options are: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil } }) - st.FeatureFlagDisable = FeatureFlagIssuesGranular + st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular} return st } @@ -1629,10 +1633,17 @@ 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. It exposes +// the full schema including the issue_fields parameter for setting custom issue field +// values. When the flag is off, LegacyIssueWrite is served instead. Both registrations +// share the tool name "issue_write" and rely on the inventory's feature-flag filter to +// make exactly one active at a time. +// Delete this comment (and the Legacy* block below) when the flag is removed and fold +// issue_fields back into a single registration. + func IssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { st := NewTool( ToolsetMetadataIssues, @@ -1886,7 +1897,207 @@ Options are: return utils.NewToolResultError("invalid method, must be either 'create' or 'update'"), nil, nil } }) - st.FeatureFlagDisable = FeatureFlagIssuesGranular + st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular} + st.FeatureFlagEnable = []string{FeatureFlagIssueFields} + 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, so the request does not depend on server-side +// issue_fields GraphQL features. Both this and IssueWrite register under the tool name +// "issue_write"; exactly one is active for any given request thanks to mutually +// exclusive FeatureFlagEnable / FeatureFlagDisable annotations. +// Delete this function (and the Legacy* block) when FeatureFlagIssueFields is removed. +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 + } + + uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted") + + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted { + if method == "update" { + 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 + } + body, err := OptionalParam[string](args, "body") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + assignees, err := OptionalStringArrayParam(args, "assignees") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + labels, err := OptionalStringArrayParam(args, "labels") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + milestone, err := OptionalIntParam(args, "milestone") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + var milestoneNum int + if milestone != 0 { + milestoneNum = milestone + } + issueType, err := OptionalParam[string](args, "type") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + 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, 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 } @@ -2339,7 +2550,7 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { } return result, nil, nil }) - st.FeatureFlagEnable = FeatureFlagIssueFields + st.FeatureFlagEnable = []string{FeatureFlagIssueFields} return st } @@ -2542,7 +2753,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_granular.go b/pkg/github/issues_granular.go index 9e789c6d16..dd535b87d6 100644 --- a/pkg/github/issues_granular.go +++ b/pkg/github/issues_granular.go @@ -103,7 +103,7 @@ func issueUpdateTool( return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureFlagEnable = []string{FeatureFlagIssuesGranular} return st } @@ -187,7 +187,7 @@ func GranularCreateIssue(t translations.TranslationHelperFunc) inventory.ServerT return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureFlagEnable = []string{FeatureFlagIssuesGranular} return st } @@ -432,7 +432,7 @@ func GranularUpdateIssueLabels(t translations.TranslationHelperFunc) inventory.S return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureFlagEnable = []string{FeatureFlagIssuesGranular} return st } @@ -610,7 +610,7 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureFlagEnable = []string{FeatureFlagIssuesGranular} return st } @@ -719,7 +719,7 @@ func GranularAddSubIssue(t translations.TranslationHelperFunc) inventory.ServerT return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureFlagEnable = []string{FeatureFlagIssuesGranular} return st } @@ -788,7 +788,7 @@ func GranularRemoveSubIssue(t translations.TranslationHelperFunc) inventory.Serv return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureFlagEnable = []string{FeatureFlagIssuesGranular} return st } @@ -873,7 +873,7 @@ func GranularReprioritizeSubIssue(t translations.TranslationHelperFunc) inventor return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureFlagEnable = []string{FeatureFlagIssuesGranular} return st } @@ -1131,6 +1131,6 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureFlagEnable = []string{FeatureFlagIssuesGranular} return st } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 1634b124e7..dd197c45a7 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1199,7 +1199,8 @@ func Test_CreateIssue(t *testing.T) { // Verify tool definition once 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, []string{FeatureFlagIssueFields}, serverTool.FeatureFlagEnable) assert.Equal(t, "issue_write", tool.Name) assert.NotEmpty(t, tool.Description) @@ -1443,6 +1444,28 @@ func Test_CreateIssue(t *testing.T) { } } +func Test_LegacyIssueWrite_Definition(t *testing.T) { + serverTool := LegacyIssueWrite(translations.NullTranslationHelper) + tool := serverTool.Tool + + // LegacyIssueWrite claims the base tool name "issue_write" and produces the + // FeatureFlagIssueFields-disabled schema (no issue_fields). It 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.Contains(t, props, "title") + assert.Contains(t, props, "body") + assert.NotContains(t, props, "issue_fields", "legacy issue_write must not advertise issue_fields") +} + // Test_IssueWrite_MCPAppsFeature_UIGate verifies the MCP Apps feature UI gate // behavior: UI clients get a form message, non-UI clients execute directly. func Test_IssueWrite_MCPAppsFeature_UIGate(t *testing.T) { @@ -1624,7 +1647,7 @@ func Test_ListIssues(t *testing.T) { serverTool := ListIssues(translations.NullTranslationHelper) tool := serverTool.Tool require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagIssueFields, tool)) - require.Equal(t, FeatureFlagIssueFields, serverTool.FeatureFlagEnable) + require.Equal(t, []string{FeatureFlagIssueFields}, serverTool.FeatureFlagEnable) assert.Equal(t, "list_issues", tool.Name) assert.NotEmpty(t, tool.Description) @@ -2551,7 +2574,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 @@ -2634,7 +2657,7 @@ func Test_UpdateIssue(t *testing.T) { // Verify tool definition 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/pullrequests_granular.go b/pkg/github/pullrequests_granular.go index 30d7f78d62..050528ab52 100644 --- a/pkg/github/pullrequests_granular.go +++ b/pkg/github/pullrequests_granular.go @@ -103,7 +103,7 @@ func prUpdateTool( return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureFlagEnable = []string{FeatureFlagPullRequestsGranular} return st } @@ -272,7 +272,7 @@ func GranularUpdatePullRequestDraftState(t translations.TranslationHelperFunc) i return utils.NewToolResultText("pull request marked as ready for review"), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureFlagEnable = []string{FeatureFlagPullRequestsGranular} return st } @@ -347,7 +347,7 @@ func GranularRequestPullRequestReviewers(t translations.TranslationHelperFunc) i return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureFlagEnable = []string{FeatureFlagPullRequestsGranular} return st } @@ -416,7 +416,7 @@ func GranularCreatePullRequestReview(t translations.TranslationHelperFunc) inven return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureFlagEnable = []string{FeatureFlagPullRequestsGranular} return st } @@ -480,7 +480,7 @@ func GranularSubmitPendingPullRequestReview(t translations.TranslationHelperFunc return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureFlagEnable = []string{FeatureFlagPullRequestsGranular} return st } @@ -535,7 +535,7 @@ func GranularDeletePendingPullRequestReview(t translations.TranslationHelperFunc return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureFlagEnable = []string{FeatureFlagPullRequestsGranular} return st } @@ -646,7 +646,7 @@ func GranularAddPullRequestReviewComment(t translations.TranslationHelperFunc) i return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureFlagEnable = []string{FeatureFlagPullRequestsGranular} return st } @@ -690,7 +690,7 @@ func GranularResolveReviewThread(t translations.TranslationHelperFunc) inventory return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureFlagEnable = []string{FeatureFlagPullRequestsGranular} return st } @@ -734,6 +734,6 @@ func GranularUnresolveReviewThread(t translations.TranslationHelperFunc) invento return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureFlagEnable = []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..129ba6bb04 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 len(tool.FeatureFlagEnable) > 0 || 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..1a286301db 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -57,8 +57,12 @@ 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 enableFlag != "" { + tool.FeatureFlagEnable = []string{enableFlag} + } + if disableFlag != "" { + tool.FeatureFlagDisable = []string{disableFlag} + } return tool } @@ -654,7 +658,7 @@ func TestStaticInventoryPreservesPerRequestFeatureVariants(t *testing.T) { available := inv.AvailableTools(ctx) require.Len(t, available, 1) assert.Equal(t, "list_issues", available[0].Tool.Name) - assert.Equal(t, github.FeatureFlagCSVOutput, available[0].FeatureFlagEnable) + assert.Equal(t, []string{github.FeatureFlagCSVOutput}, available[0].FeatureFlagEnable) } // TestContentTypeHandling verifies that the MCP StreamableHTTP handler diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index e2effd8ca7..c069b3a9e8 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "slices" "sort" ) @@ -43,7 +44,7 @@ func (r *Inventory) checkFeatureFlag(ctx context.Context, flagName string) bool // // - 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 { +func featureFlagAllowed(ctx context.Context, checker FeatureFlagChecker, enableFlags, 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 @@ -56,13 +57,12 @@ func featureFlagAllowed(ctx context.Context, checker FeatureFlagChecker, enableF } return enabled } - if enableFlag != "" && !check(enableFlag) { - return false - } - if disableFlag != "" && check(disableFlag) { - return false + for _, flag := range enableFlags { + if !check(flag) { + 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..b7ee9555a5 100644 --- a/pkg/inventory/prompts.go +++ b/pkg/inventory/prompts.go @@ -8,12 +8,12 @@ type ServerPrompt struct { Handler mcp.PromptHandler // Toolset identifies which toolset this prompt belongs to Toolset ToolsetMetadata - // 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 + // FeatureFlagEnable specifies feature flags that must all be enabled for this prompt + // to be available. If any flag is not enabled, the prompt is omitted. + FeatureFlagEnable []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 + 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..e40777afc8 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1046,8 +1046,12 @@ func TestMCPMethodConstants(t *testing.T) { // mockToolWithFlags creates a ServerTool with feature flags for testing func mockToolWithFlags(name string, toolsetID string, readOnly bool, enableFlag, disableFlag string) ServerTool { tool := mockTool(name, toolsetID, readOnly) - tool.FeatureFlagEnable = enableFlag - tool.FeatureFlagDisable = disableFlag + if enableFlag != "" { + tool.FeatureFlagEnable = []string{enableFlag} + } + if disableFlag != "" { + tool.FeatureFlagDisable = []string{disableFlag} + } return tool } @@ -1163,7 +1167,7 @@ func TestFeatureFlagResources(t *testing.T) { { Template: mcp.ResourceTemplate{Name: "needs_flag", URITemplate: "uri2"}, Toolset: testToolsetMetadata("toolset1"), - FeatureFlagEnable: "my_feature", + FeatureFlagEnable: []string{"my_feature"}, }, } @@ -1188,7 +1192,7 @@ func TestFeatureFlagPrompts(t *testing.T) { { Prompt: mcp.Prompt{Name: "needs_flag"}, Toolset: testToolsetMetadata("toolset1"), - FeatureFlagEnable: "my_feature", + FeatureFlagEnable: []string{"my_feature"}, }, } @@ -1723,7 +1727,7 @@ 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" { + if len(availableOff[0].FeatureFlagDisable) == 0 || availableOff[0].FeatureFlagDisable[0] != "consolidated_flag" { t.Errorf("Flag OFF: Expected tool with FeatureFlagDisable, got FeatureFlagEnable=%q, FeatureFlagDisable=%q", availableOff[0].FeatureFlagEnable, availableOff[0].FeatureFlagDisable) } @@ -1741,7 +1745,7 @@ func TestForMCPRequest_ToolsCall_FeatureFlaggedVariants(t *testing.T) { if len(availableOn) != 1 { t.Fatalf("Flag ON: Expected 1 tool, got %d", len(availableOn)) } - if availableOn[0].FeatureFlagEnable != "consolidated_flag" { + if len(availableOn[0].FeatureFlagEnable) == 0 || availableOn[0].FeatureFlagEnable[0] != "consolidated_flag" { t.Errorf("Flag ON: Expected tool with FeatureFlagEnable, got FeatureFlagEnable=%q, FeatureFlagDisable=%q", availableOn[0].FeatureFlagEnable, availableOn[0].FeatureFlagDisable) } diff --git a/pkg/inventory/resources.go b/pkg/inventory/resources.go index 6de037d584..b2d16ec783 100644 --- a/pkg/inventory/resources.go +++ b/pkg/inventory/resources.go @@ -16,12 +16,12 @@ type ServerResourceTemplate struct { HandlerFunc ResourceHandlerFunc // Toolset identifies which toolset this resource belongs to Toolset ToolsetMetadata - // 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 + // FeatureFlagEnable specifies feature flags that must all be enabled for this resource + // to be available. If any flag is not enabled, the resource is omitted. + FeatureFlagEnable []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 + 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..bead29a873 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -60,13 +60,13 @@ type ServerTool struct { // and handlers are only created when needed. HandlerFunc HandlerFunc - // FeatureFlagEnable specifies a feature flag that must be enabled for this tool - // to be available. If set and the flag is not enabled, the tool is omitted. - FeatureFlagEnable string + // FeatureFlagEnable specifies feature flags that must all be enabled for this tool + // to be available. If any 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 488b60974cd52fb95493160ee178dee0ffd3d9d8 Mon Sep 17 00:00:00 2001 From: Kelsey Myers <52179263+kelsey-myers@users.noreply.github.com> Date: Tue, 26 May 2026 16:01:18 -0700 Subject: [PATCH 2/4] address comments --- pkg/github/granular_tools_test.go | 4 +- pkg/github/issues_granular.go | 2 +- pkg/inventory/registry_test.go | 63 +++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/pkg/github/granular_tools_test.go b/pkg/github/granular_tools_test.go index 834a63bf06..9232827e09 100644 --- a/pkg/github/granular_tools_test.go +++ b/pkg/github/granular_tools_test.go @@ -94,7 +94,7 @@ func TestIssuesGranularToolset(t *testing.T) { t.Run("all granular tools have correct feature flag", func(t *testing.T) { for _, tool := range granularToolsForToolset(ToolsetMetadataIssues.ID, FeatureFlagIssuesGranular) { - assert.Equal(t, []string{FeatureFlagIssuesGranular}, tool.FeatureFlagEnable, "tool %s", tool.Tool.Name) + assert.Contains(t, tool.FeatureFlagEnable, FeatureFlagIssuesGranular, "tool %s should require the granular flag", tool.Tool.Name) } }) } @@ -129,7 +129,7 @@ func TestPullRequestsGranularToolset(t *testing.T) { t.Run("all granular tools have correct feature flag", func(t *testing.T) { for _, tool := range granularToolsForToolset(ToolsetMetadataPullRequests.ID, FeatureFlagPullRequestsGranular) { - assert.Equal(t, []string{FeatureFlagPullRequestsGranular}, tool.FeatureFlagEnable, "tool %s", tool.Tool.Name) + assert.Contains(t, tool.FeatureFlagEnable, FeatureFlagPullRequestsGranular, "tool %s should require the granular flag", tool.Tool.Name) } }) } diff --git a/pkg/github/issues_granular.go b/pkg/github/issues_granular.go index dd535b87d6..0c496d8b2a 100644 --- a/pkg/github/issues_granular.go +++ b/pkg/github/issues_granular.go @@ -1131,6 +1131,6 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = []string{FeatureFlagIssuesGranular} + st.FeatureFlagEnable = []string{FeatureFlagIssuesGranular, FeatureFlagIssueFields} return st } diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index e40777afc8..a41897d1f2 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1145,6 +1145,69 @@ func TestFeatureFlagBoth(t *testing.T) { } } +func TestFeatureFlagMultipleEnableFlags(t *testing.T) { + // Tool requires BOTH flag_a AND flag_b (AND semantics). + tool := mockTool("dual_enable_tool", "toolset1", true) + tool.FeatureFlagEnable = []string{"flag_a", "flag_b"} + tools := []ServerTool{tool} + + // Neither flag enabled → excluded + checkerNone := func(_ context.Context, _ string) (bool, error) { return false, nil } + regNone := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checkerNone)) + if len(regNone.AvailableTools(context.Background())) != 0 { + t.Error("Tool should be excluded when no enable flags are on") + } + + // Only flag_a enabled → excluded (flag_b still missing) + checkerOnlyA := func(_ context.Context, flag string) (bool, error) { return flag == "flag_a", nil } + regOnlyA := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checkerOnlyA)) + if len(regOnlyA.AvailableTools(context.Background())) != 0 { + t.Error("Tool should be excluded when only one of two enable flags is on") + } + + // Both flags enabled → included + checkerBoth := func(_ context.Context, _ string) (bool, error) { return true, nil } + regBoth := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checkerBoth)) + if len(regBoth.AvailableTools(context.Background())) != 1 { + t.Error("Tool should be included when all enable flags are on") + } +} + +func TestFeatureFlagMultipleDisableFlags(t *testing.T) { + // Tool is blocked when EITHER kill_a OR kill_b is enabled (OR semantics). + tool := mockTool("dual_disable_tool", "toolset1", true) + tool.FeatureFlagDisable = []string{"kill_a", "kill_b"} + tools := []ServerTool{tool} + + // Neither kill flag on → included + checkerNone := func(_ context.Context, _ string) (bool, error) { return false, nil } + regNone := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checkerNone)) + if len(regNone.AvailableTools(context.Background())) != 1 { + t.Error("Tool should be included when no disable flags are on") + } + + // Only kill_a on → excluded + checkerOnlyA := func(_ context.Context, flag string) (bool, error) { return flag == "kill_a", nil } + regOnlyA := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checkerOnlyA)) + if len(regOnlyA.AvailableTools(context.Background())) != 0 { + t.Error("Tool should be excluded when the first disable flag is on") + } + + // Only kill_b on → excluded + checkerOnlyB := func(_ context.Context, flag string) (bool, error) { return flag == "kill_b", nil } + regOnlyB := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checkerOnlyB)) + if len(regOnlyB.AvailableTools(context.Background())) != 0 { + t.Error("Tool should be excluded when the second disable flag is on") + } + + // Both kill flags on → excluded + checkerBoth := func(_ context.Context, _ string) (bool, error) { return true, nil } + regBoth := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checkerBoth)) + if len(regBoth.AvailableTools(context.Background())) != 0 { + t.Error("Tool should be excluded when all disable flags are on") + } +} + func TestFeatureFlagError(t *testing.T) { tools := []ServerTool{ mockToolWithFlags("needs_flag", "toolset1", true, "my_feature", ""), From 1f35c64512292e8d0f2dfe2dae2262eaaab08308 Mon Sep 17 00:00:00 2001 From: Kelsey Myers <52179263+kelsey-myers@users.noreply.github.com> Date: Tue, 26 May 2026 16:11:17 -0700 Subject: [PATCH 3/4] always clear raw IssueFieldValues from get_issue response --- pkg/github/issues.go | 4 ++-- pkg/github/issues_test.go | 26 +++++++------------------- 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index fdfabaa581..9ede2e4fd1 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -761,14 +761,14 @@ func GetIssue(ctx context.Context, client *github.Client, deps ToolDependencies, // Enrich with field_values via GraphQL for consistency with list_issues/search_issues. // Gated behind FeatureFlagIssueFields so the GraphQL round-trip is only paid when the - // feature is active. + // feature is active. Always clear the verbose REST IssueFieldValues regardless. + 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] - minimalIssue.IssueFieldValues = nil // Clear verbose REST format } } } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index dd197c45a7..6637454dc9 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -394,7 +394,10 @@ 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 (tested via + // featureCheckerFor in Test_CreateIssue and the smoke tests). serverTool := IssueRead(translations.NullTranslationHelper) mockIssueWithFields := &github.Issue{ @@ -457,24 +460,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) { From 23cf776e5b714b1b6a6de40e7b136405e5561b42 Mon Sep 17 00:00:00 2001 From: Kelsey Myers <52179263+kelsey-myers@users.noreply.github.com> Date: Tue, 26 May 2026 16:20:32 -0700 Subject: [PATCH 4/4] clean up verbose comments on IssueWrite and LegacyIssueWrite --- pkg/github/issues.go | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 9ede2e4fd1..112984a795 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -759,9 +759,8 @@ 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. - // Gated behind FeatureFlagIssueFields so the GraphQL round-trip is only paid when the - // feature is active. Always clear the verbose REST IssueFieldValues regardless. + // Enrich with field_values via GraphQL when the flag is on; always clear the verbose + // REST IssueFieldValues. minimalIssue.IssueFieldValues = nil if deps.IsFeatureEnabled(ctx, FeatureFlagIssueFields) { if issue != nil && issue.NodeID != nil && *issue.NodeID != "" { @@ -1636,13 +1635,8 @@ func searchIssuesHandler(ctx context.Context, deps ToolDependencies, args map[st // 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. It exposes -// the full schema including the issue_fields parameter for setting custom issue field -// values. When the flag is off, LegacyIssueWrite is served instead. Both registrations -// share the tool name "issue_write" and rely on the inventory's feature-flag filter to -// make exactly one active at a time. -// Delete this comment (and the Legacy* block below) when the flag is removed and fold -// issue_fields back into a single registration. +// IssueWrite is the FeatureFlagIssueFields-enabled variant of issue_write (with issue_fields). +// LegacyIssueWrite is served when the flag is off. Delete both when the flag is removed. func IssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { st := NewTool( @@ -1902,13 +1896,8 @@ Options are: 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, so the request does not depend on server-side -// issue_fields GraphQL features. Both this and IssueWrite register under the tool name -// "issue_write"; exactly one is active for any given request thanks to mutually -// exclusive FeatureFlagEnable / FeatureFlagDisable annotations. -// Delete this function (and the Legacy* block) when FeatureFlagIssueFields is removed. +// LegacyIssueWrite is the FeatureFlagIssueFields-disabled variant of issue_write (no issue_fields). +// IssueWrite is served when the flag is on. Delete both when the flag is removed. func LegacyIssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { st := NewTool( ToolsetMetadataIssues,