diff --git a/README.md b/README.md index bec45b5da..9082a3b64 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 afd6a52c7..74081b286 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 6956a5eae..881030f02 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 6fb00d249..a125864f0 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 000000000..6fb00d249 --- /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 246902d49..b9ff2e3ed 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 87bfa08af..50708d556 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,10 +1757,15 @@ 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. When the +// flag is removed, delete LegacyIssueWrite outright and drop the feature-flag +// fields on IssueWrite. func IssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { st := NewTool( ToolsetMetadataIssues, @@ -1978,7 +1986,8 @@ Options are: return utils.NewToolResultError("duplicate_of can only be used when state_reason is 'duplicate'"), nil, nil } - issueFields, err := optionalIssueWriteFields(args) + var issueFields []issueWriteFieldInput + issueFields, err = optionalIssueWriteFields(args) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -1993,9 +2002,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 +2026,228 @@ Options are: return utils.NewToolResultError("invalid method, must be either 'create' or 'update'"), nil, nil } }) - st.FeatureFlagDisable = FeatureFlagIssuesGranular + 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 } @@ -2690,7 +2924,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 3ca2ae9a7..c2be1984f 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,105 @@ 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_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) { @@ -1196,10 +1279,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 +2635,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 +2647,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 +2733,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 c298d875a..3910a96b9 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 49edb00ff..d1d585b3f 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 0a4a4eb7b..1db85b2fc 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 74e28a6e4..a36469133 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 e2effd8ca..fd3579fa6 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 648f20f9c..d929578e8 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 75de9c574..372f75602 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 6de037d58..2dd07ae0f 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 41d38b7ec..326009b59 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