diff --git a/docs/feature-flags.md b/docs/feature-flags.md index a552e71a04..95e31c9f50 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -73,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` @@ -177,6 +198,7 @@ runtime behavior (such as output formatting) won't appear here. - **update_issue_type** - Update Issue Type - **Required OAuth Scopes**: `repo` + - `is_suggestion`: If true, propose the issue type change instead of applying it. Defaults to false, which applies the change to the issue. (boolean, optional) - `issue_number`: The issue number to update (number, required) - `issue_type`: The issue type to set (string, required) - `owner`: Repository owner (username or organization) (string, required) diff --git a/docs/insiders-features.md b/docs/insiders-features.md index c221b87580..bb9d664eae 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -67,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_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..9232827e09 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.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, 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/issue_fields.go b/pkg/github/issue_fields.go index a7b7c429de..6d8bb9b964 100644 --- a/pkg/github/issue_fields.go +++ b/pkg/github/issue_fields.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "strconv" ghcontext "github.com/github/github-mcp-server/pkg/context" ghErrors "github.com/github/github-mcp-server/pkg/errors" @@ -19,6 +20,7 @@ import ( // IssueField represents a repository issue field definition. type IssueField struct { ID string `json:"id"` + DatabaseID int64 `json:"full_database_id,omitempty"` Name string `json:"name"` Description string `json:"description,omitempty"` DataType string `json:"data_type"` @@ -37,36 +39,42 @@ type IssueSingleSelectFieldOption struct { // issueFieldNode is the GraphQL fragment for a single issue field in the IssueFields union. // Only the fragment matching __typename is populated; read from the matching fragment. +// fullDatabaseId (BigInt scalar, returned as string) is fetched on each concrete type because +// shurcooL/githubv4 does not support interface fragments at the top level of a union. type issueFieldNode struct { TypeName githubv4.String `graphql:"__typename"` IssueFieldText struct { - ID githubv4.ID - Name githubv4.String - Description githubv4.String - DataType githubv4.String - Visibility githubv4.String + ID githubv4.ID + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + Name githubv4.String + Description githubv4.String + DataType githubv4.String + Visibility githubv4.String } `graphql:"... on IssueFieldText"` IssueFieldNumber struct { - ID githubv4.ID - Name githubv4.String - Description githubv4.String - DataType githubv4.String - Visibility githubv4.String + ID githubv4.ID + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + Name githubv4.String + Description githubv4.String + DataType githubv4.String + Visibility githubv4.String } `graphql:"... on IssueFieldNumber"` IssueFieldDate struct { - ID githubv4.ID - Name githubv4.String - Description githubv4.String - DataType githubv4.String - Visibility githubv4.String + ID githubv4.ID + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + Name githubv4.String + Description githubv4.String + DataType githubv4.String + Visibility githubv4.String } `graphql:"... on IssueFieldDate"` IssueFieldSingleSelect struct { - ID githubv4.ID - Name githubv4.String - Description githubv4.String - DataType githubv4.String - Visibility githubv4.String - Options []struct { + ID githubv4.ID + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + Name githubv4.String + Description githubv4.String + DataType githubv4.String + Visibility githubv4.String + Options []struct { ID githubv4.ID Name githubv4.String Description githubv4.String @@ -149,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 } @@ -200,6 +208,7 @@ func issueFieldsFromNodes(nodes []issueFieldNode) []IssueField { } f = IssueField{ ID: fmt.Sprintf("%v", node.IssueFieldSingleSelect.ID), + DatabaseID: parseFullDatabaseID(string(node.IssueFieldSingleSelect.FullDatabaseID)), Name: string(node.IssueFieldSingleSelect.Name), Description: string(node.IssueFieldSingleSelect.Description), DataType: string(node.IssueFieldSingleSelect.DataType), @@ -209,6 +218,7 @@ func issueFieldsFromNodes(nodes []issueFieldNode) []IssueField { case "IssueFieldText": f = IssueField{ ID: fmt.Sprintf("%v", node.IssueFieldText.ID), + DatabaseID: parseFullDatabaseID(string(node.IssueFieldText.FullDatabaseID)), Name: string(node.IssueFieldText.Name), Description: string(node.IssueFieldText.Description), DataType: string(node.IssueFieldText.DataType), @@ -217,6 +227,7 @@ func issueFieldsFromNodes(nodes []issueFieldNode) []IssueField { case "IssueFieldNumber": f = IssueField{ ID: fmt.Sprintf("%v", node.IssueFieldNumber.ID), + DatabaseID: parseFullDatabaseID(string(node.IssueFieldNumber.FullDatabaseID)), Name: string(node.IssueFieldNumber.Name), Description: string(node.IssueFieldNumber.Description), DataType: string(node.IssueFieldNumber.DataType), @@ -225,6 +236,7 @@ func issueFieldsFromNodes(nodes []issueFieldNode) []IssueField { case "IssueFieldDate": f = IssueField{ ID: fmt.Sprintf("%v", node.IssueFieldDate.ID), + DatabaseID: parseFullDatabaseID(string(node.IssueFieldDate.FullDatabaseID)), Name: string(node.IssueFieldDate.Name), Description: string(node.IssueFieldDate.Description), DataType: string(node.IssueFieldDate.DataType), @@ -237,3 +249,16 @@ func issueFieldsFromNodes(nodes []issueFieldNode) []IssueField { } return fields } + +// parseFullDatabaseID converts a BigInt scalar string (e.g. "12345") to int64. +// Returns 0 if the string is empty or cannot be parsed. +func parseFullDatabaseID(s string) int64 { + if s == "" { + return 0 + } + n, err := strconv.ParseInt(s, 10, 64) + if err != nil { + return 0 + } + return n +} diff --git a/pkg/github/issue_fields_test.go b/pkg/github/issue_fields_test.go index 238c0455b2..2c2b26ee2a 100644 --- a/pkg/github/issue_fields_test.go +++ b/pkg/github/issue_fields_test.go @@ -75,12 +75,13 @@ func Test_ListIssueFields(t *testing.T) { "issueFields": map[string]any{ "nodes": []any{ map[string]any{ - "__typename": "IssueFieldText", - "id": "IFT_1", - "name": "DRI", - "description": "Directly responsible individual", - "dataType": "TEXT", - "visibility": "ORG_ONLY", + "__typename": "IssueFieldText", + "id": "IFT_1", + "fullDatabaseId": "42", + "name": "DRI", + "description": "Directly responsible individual", + "dataType": "TEXT", + "visibility": "ORG_ONLY", }, }, }, @@ -89,6 +90,7 @@ func Test_ListIssueFields(t *testing.T) { expectedFields: []IssueField{ { ID: "IFT_1", + DatabaseID: 42, Name: "DRI", Description: "Directly responsible individual", DataType: "TEXT", @@ -107,12 +109,13 @@ func Test_ListIssueFields(t *testing.T) { "issueFields": map[string]any{ "nodes": []any{ map[string]any{ - "__typename": "IssueFieldSingleSelect", - "id": "IFSS_1", - "name": "Priority", - "description": "Level of importance", - "dataType": "SINGLE_SELECT", - "visibility": "ALL", + "__typename": "IssueFieldSingleSelect", + "id": "IFSS_1", + "fullDatabaseId": "99", + "name": "Priority", + "description": "Level of importance", + "dataType": "SINGLE_SELECT", + "visibility": "ALL", "options": []any{ map[string]any{ "id": "OPT_1", @@ -133,6 +136,7 @@ func Test_ListIssueFields(t *testing.T) { expectedFields: []IssueField{ { ID: "IFSS_1", + DatabaseID: 99, Name: "Priority", Description: "Level of importance", DataType: "SINGLE_SELECT", @@ -165,18 +169,19 @@ func Test_ListIssueFields(t *testing.T) { "issueFields": map[string]any{ "nodes": []any{ map[string]any{ - "__typename": "IssueFieldText", - "id": "IFT_1", - "name": "DRI", - "dataType": "TEXT", - "visibility": "ORG_ONLY", + "__typename": "IssueFieldText", + "id": "IFT_1", + "fullDatabaseId": "77", + "name": "DRI", + "dataType": "TEXT", + "visibility": "ORG_ONLY", }, }, }, }, }), expectedFields: []IssueField{ - {ID: "IFT_1", Name: "DRI", DataType: "TEXT", Visibility: "ORG_ONLY"}, + {ID: "IFT_1", DatabaseID: 77, Name: "DRI", DataType: "TEXT", Visibility: "ORG_ONLY"}, }, }, { @@ -190,18 +195,19 @@ func Test_ListIssueFields(t *testing.T) { "issueFields": map[string]any{ "nodes": []any{ map[string]any{ - "__typename": "IssueFieldNumber", - "id": "IFN_1", - "name": "Engineering Staffing", - "dataType": "NUMBER", - "visibility": "ORG_ONLY", + "__typename": "IssueFieldNumber", + "id": "IFN_1", + "fullDatabaseId": "101", + "name": "Engineering Staffing", + "dataType": "NUMBER", + "visibility": "ORG_ONLY", }, }, }, }, }), expectedFields: []IssueField{ - {ID: "IFN_1", Name: "Engineering Staffing", DataType: "NUMBER", Visibility: "ORG_ONLY"}, + {ID: "IFN_1", DatabaseID: 101, Name: "Engineering Staffing", DataType: "NUMBER", Visibility: "ORG_ONLY"}, }, }, { @@ -215,18 +221,19 @@ func Test_ListIssueFields(t *testing.T) { "issueFields": map[string]any{ "nodes": []any{ map[string]any{ - "__typename": "IssueFieldDate", - "id": "IFD_1", - "name": "Target Date", - "dataType": "DATE", - "visibility": "ORG_ONLY", + "__typename": "IssueFieldDate", + "id": "IFD_1", + "fullDatabaseId": "202", + "name": "Target Date", + "dataType": "DATE", + "visibility": "ORG_ONLY", }, }, }, }, }), expectedFields: []IssueField{ - {ID: "IFD_1", Name: "Target Date", DataType: "DATE", Visibility: "ORG_ONLY"}, + {ID: "IFD_1", DatabaseID: 202, Name: "Target Date", DataType: "DATE", Visibility: "ORG_ONLY"}, }, }, { @@ -284,6 +291,7 @@ func Test_ListIssueFields(t *testing.T) { require.Equal(t, len(tc.expectedFields), len(returnedFields)) for i, expected := range tc.expectedFields { assert.Equal(t, expected.ID, returnedFields[i].ID) + assert.Equal(t, expected.DatabaseID, returnedFields[i].DatabaseID) assert.Equal(t, expected.Name, returnedFields[i].Name) assert.Equal(t, expected.DataType, returnedFields[i].DataType) assert.Equal(t, expected.Visibility, returnedFields[i].Visibility) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 0074bbd581..112984a795 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -37,6 +37,14 @@ type CloseIssueInput struct { // Used to extend the functionality of the githubv4 library to support closing issues as duplicates. type IssueClosedStateReason string +// issueWriteFieldInput is a user-friendly issue field input for issue_write. +// Field IDs and option IDs are resolved internally before calling the REST API. +type issueWriteFieldInput struct { + FieldName string + Value any + FieldOptionName string +} + const ( IssueClosedStateReasonCompleted IssueClosedStateReason = "COMPLETED" IssueClosedStateReasonDuplicate IssueClosedStateReason = "DUPLICATE" @@ -105,6 +113,46 @@ func getCloseStateReason(stateReason string) IssueClosedStateReason { } } +// issueFieldWriteMetadataNode queries only the fields needed to resolve a write: the field's +// fullDatabaseId (BigInt scalar, returned as string) plus its name and data type for validation. +// shurcooL/githubv4 cannot use interface-level fragments at union top-level, so we repeat +// fullDatabaseId on each concrete type; all four implement IssueFieldCommon. +type issueFieldWriteMetadataNode struct { + TypeName githubv4.String `graphql:"__typename"` + IssueFieldText struct { + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + Name githubv4.String + DataType githubv4.String + } `graphql:"... on IssueFieldText"` + IssueFieldNumber struct { + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + Name githubv4.String + DataType githubv4.String + } `graphql:"... on IssueFieldNumber"` + IssueFieldDate struct { + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + Name githubv4.String + DataType githubv4.String + } `graphql:"... on IssueFieldDate"` + IssueFieldSingleSelect struct { + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + Name githubv4.String + DataType githubv4.String + Options []struct { + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + Name githubv4.String + } + } `graphql:"... on IssueFieldSingleSelect"` +} + +type issueFieldWriteMetadataQuery struct { + Repository struct { + IssueFields struct { + Nodes []issueFieldWriteMetadataNode + } `graphql:"issueFields(first: 100)"` + } `graphql:"repository(owner: $owner, name: $repo)"` +} + // IssueFieldRef resolves the name of an issue field across its concrete types. // IssueFields is a union of IssueFieldDate, IssueFieldNumber, IssueFieldSingleSelect, IssueFieldText, // so we have to ask for `name` on each member. @@ -153,6 +201,155 @@ type IssueFieldValueFragment struct { } `graphql:"... on IssueFieldTextValue"` } +func optionalIssueWriteFields(args map[string]any) ([]issueWriteFieldInput, error) { + issueFieldsRaw, exists := args["issue_fields"] + if !exists { + return nil, nil + } + + var inputMaps []map[string]any + switch v := issueFieldsRaw.(type) { + case []any: + for _, item := range v { + itemMap, ok := item.(map[string]any) + if !ok { + return nil, fmt.Errorf("each issue_fields item must be an object") + } + inputMaps = append(inputMaps, itemMap) + } + case []map[string]any: + inputMaps = v + default: + return nil, fmt.Errorf("issue_fields must be an array") + } + + issueFields := make([]issueWriteFieldInput, 0, len(inputMaps)) + for _, itemMap := range inputMaps { + fieldName, err := RequiredParam[string](itemMap, "field_name") + if err != nil || strings.TrimSpace(fieldName) == "" { + return nil, fmt.Errorf("field_name is required for each issue_fields item") + } + + fieldOptionName, err := OptionalParam[string](itemMap, "field_option_name") + if err != nil { + return nil, err + } + + value, hasValue := itemMap["value"] + if hasValue && value == nil { + return nil, fmt.Errorf("value cannot be null for field %q", fieldName) + } + + if hasValue && fieldOptionName != "" { + return nil, fmt.Errorf("issue field %q cannot specify both value and field_option_name", fieldName) + } + + if !hasValue && fieldOptionName == "" { + return nil, fmt.Errorf("issue field %q must specify either value or field_option_name", fieldName) + } + + issueFields = append(issueFields, issueWriteFieldInput{ + FieldName: fieldName, + Value: value, + FieldOptionName: fieldOptionName, + }) + } + + return issueFields, nil +} + +func resolveIssueRequestFieldValues(ctx context.Context, gqlClient *githubv4.Client, owner, repo string, issueFields []issueWriteFieldInput) ([]*github.IssueRequestFieldValue, error) { + if len(issueFields) == 0 { + return nil, nil + } + + ctxWithFeatures := ghcontext.WithGraphQLFeatures(ctx, "issue_fields", "repo_issue_fields") + var query issueFieldWriteMetadataQuery + vars := map[string]any{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + } + if err := gqlClient.Query(ctxWithFeatures, &query, vars); err != nil { + return nil, fmt.Errorf("failed to query issue fields metadata: %w", err) + } + + // Build name → node map, dispatching on concrete type to extract name. + fieldByName := make(map[string]issueFieldWriteMetadataNode, len(query.Repository.IssueFields.Nodes)) + for _, node := range query.Repository.IssueFields.Nodes { + var name string + switch string(node.TypeName) { + case "IssueFieldText": + name = string(node.IssueFieldText.Name) + case "IssueFieldNumber": + name = string(node.IssueFieldNumber.Name) + case "IssueFieldDate": + name = string(node.IssueFieldDate.Name) + case "IssueFieldSingleSelect": + name = string(node.IssueFieldSingleSelect.Name) + default: + continue + } + fieldByName[strings.ToLower(strings.TrimSpace(name))] = node + } + + resolved := make([]*github.IssueRequestFieldValue, 0, len(issueFields)) + for _, fieldInput := range issueFields { + node, ok := fieldByName[strings.ToLower(strings.TrimSpace(fieldInput.FieldName))] + if !ok { + return nil, fmt.Errorf("issue field %q was not found in %s/%s", fieldInput.FieldName, owner, repo) + } + + var fullDatabaseIDStr, dataType string + switch string(node.TypeName) { + case "IssueFieldText": + fullDatabaseIDStr = string(node.IssueFieldText.FullDatabaseID) + dataType = string(node.IssueFieldText.DataType) + case "IssueFieldNumber": + fullDatabaseIDStr = string(node.IssueFieldNumber.FullDatabaseID) + dataType = string(node.IssueFieldNumber.DataType) + case "IssueFieldDate": + fullDatabaseIDStr = string(node.IssueFieldDate.FullDatabaseID) + dataType = string(node.IssueFieldDate.DataType) + case "IssueFieldSingleSelect": + fullDatabaseIDStr = string(node.IssueFieldSingleSelect.FullDatabaseID) + dataType = string(node.IssueFieldSingleSelect.DataType) + } + + fieldID := parseFullDatabaseID(fullDatabaseIDStr) + if fieldID == 0 { + return nil, fmt.Errorf("issue field %q is missing fullDatabaseId", fieldInput.FieldName) + } + + resolvedValue := fieldInput.Value + if fieldInput.FieldOptionName != "" { + if !strings.EqualFold(dataType, "single_select") { + return nil, fmt.Errorf("issue field %q is %q, so field_option_name cannot be used", fieldInput.FieldName, dataType) + } + + optionFound := false + for _, option := range node.IssueFieldSingleSelect.Options { + if strings.EqualFold(strings.TrimSpace(string(option.Name)), strings.TrimSpace(fieldInput.FieldOptionName)) { + // REST API expects the option name, not the ID + resolvedValue = string(option.Name) + optionFound = true + break + } + } + + if !optionFound { + return nil, fmt.Errorf("issue field option %q was not found for field %q", fieldInput.FieldOptionName, fieldInput.FieldName) + } + } + + resolved = append(resolved, &github.IssueRequestFieldValue{ + FieldID: fieldID, + Value: resolvedValue, + }) + } + + return resolved, nil +} + // IssueFragment represents a fragment of an issue node in the GraphQL API. type IssueFragment struct { Number githubv4.Int @@ -562,6 +759,20 @@ func GetIssue(ctx context.Context, client *github.Client, deps ToolDependencies, minimalIssue := convertToMinimalIssue(issue) + // 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 != "" { + gqlClient, err := deps.GetGQLClient(ctx) + if err == nil { + if fieldValuesByID, err := fetchIssueFieldValuesByNodeID(ctx, gqlClient, []*github.Issue{issue}); err == nil { + minimalIssue.FieldValues = fieldValuesByID[*issue.NodeID] + } + } + } + } + return MarshalledTextResult(minimalIssue), nil } @@ -998,7 +1209,7 @@ Options are: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil } }) - st.FeatureFlagDisable = FeatureFlagIssuesGranular + st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular} return st } @@ -1266,7 +1477,7 @@ func parseRepositoryURL(repoURL string) (string, string, bool) { // SearchIssueResult wraps a REST search hit with its custom issue field values, fetched in a follow-up GraphQL nodes() query. type SearchIssueResult struct { *github.Issue - FieldValues []MinimalIssueFieldValue `json:"field_values,omitempty"` + FieldValues []MinimalFieldValue `json:"field_values,omitempty"` } // MarshalJSON serializes SearchIssueResult, suppressing the raw issue_field_values from the @@ -1315,7 +1526,7 @@ type searchIssuesNodesQuery struct { // fetchIssueFieldValuesByNodeID runs one GraphQL nodes() query for the given REST issues and // returns a map of node_id -> flattened field values. Issues without a node_id are skipped, and // an empty result set short-circuits the round-trip. -func fetchIssueFieldValuesByNodeID(ctx context.Context, gqlClient *githubv4.Client, issues []*github.Issue) (map[string][]MinimalIssueFieldValue, error) { +func fetchIssueFieldValuesByNodeID(ctx context.Context, gqlClient *githubv4.Client, issues []*github.Issue) (map[string][]MinimalFieldValue, error) { ids := make([]githubv4.ID, 0, len(issues)) for _, iss := range issues { if iss == nil || iss.NodeID == nil || *iss.NodeID == "" { @@ -1332,15 +1543,15 @@ func fetchIssueFieldValuesByNodeID(ctx context.Context, gqlClient *githubv4.Clie return nil, err } - result := make(map[string][]MinimalIssueFieldValue, len(q.Nodes)) + result := make(map[string][]MinimalFieldValue, len(q.Nodes)) for _, n := range q.Nodes { idStr, ok := n.Issue.ID.(string) if !ok || idStr == "" { continue } - vals := make([]MinimalIssueFieldValue, 0, len(n.Issue.IssueFieldValues.Nodes)) + vals := make([]MinimalFieldValue, 0, len(n.Issue.IssueFieldValues.Nodes)) for _, fv := range n.Issue.IssueFieldValues.Nodes { - if m, ok := fragmentToMinimalIssueFieldValue(fv); ok { + if m, ok := fragmentToMinimalFieldValue(fv); ok { vals = append(vals, m) } } @@ -1378,8 +1589,8 @@ func searchIssuesHandler(ctx context.Context, deps ToolDependencies, args map[st return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, errorPrefix, resp, body), nil } - var fieldValuesByID map[string][]MinimalIssueFieldValue - if deps.IsFeatureEnabled(ctx, FeatureFlagIssueFields) && len(result.Issues) > 0 { + var fieldValuesByID map[string][]MinimalFieldValue + if len(result.Issues) > 0 { gqlClient, err := deps.GetGQLClient(ctx) if err != nil { return utils.NewToolResultErrorFromErr(errorPrefix+": failed to get GitHub GraphQL client", err), nil @@ -1421,10 +1632,12 @@ 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 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( ToolsetMetadataIssues, @@ -1509,6 +1722,39 @@ Options are: Type: "number", Description: "Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'.", }, + "issue_fields": { + Type: "array", + Description: "Issue field values to set. Each item requires 'field_name' and exactly one of 'value' or 'field_option_name'.", + Items: &jsonschema.Schema{ + Type: "object", + AdditionalProperties: &jsonschema.Schema{Not: &jsonschema.Schema{}}, + Properties: map[string]*jsonschema.Schema{ + "field_name": { + Type: "string", + Description: "Issue field name", + }, + "value": { + Types: []string{"string", "number", "boolean"}, + Description: "Value to set. For single-select fields, prefer 'field_option_name' to validate the option exists first.", + }, + "field_option_name": { + Type: "string", + Description: "Option name for single-select fields — validates the option exists in the field definition before setting it.", + }, + }, + Required: []string{"field_name"}, + OneOf: []*jsonschema.Schema{ + { + Required: []string{"value"}, + Not: &jsonschema.Schema{Required: []string{"field_option_name"}}, + }, + { + Required: []string{"field_option_name"}, + Not: &jsonschema.Schema{Required: []string{"value"}}, + }, + }, + }, + }, }, Required: []string{"method", "owner", "repo"}, }, @@ -1610,6 +1856,11 @@ Options are: return utils.NewToolResultError("duplicate_of can only be used when state_reason is 'duplicate'"), nil, nil } + issueFields, err := optionalIssueWriteFields(args) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + client, err := deps.GetClient(ctx) if err != nil { return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil @@ -1620,36 +1871,237 @@ Options are: return utils.NewToolResultErrorFromErr("failed to get GraphQL client", err), nil, nil } + issueFieldValues, 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 { case "create": - result, err := CreateIssue(ctx, client, owner, repo, title, body, assignees, labels, milestoneNum, issueType) + result, err := CreateIssue(ctx, client, owner, repo, title, body, assignees, labels, milestoneNum, issueType, issueFieldValues) 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, state, stateReason, duplicateOf) + result, err := UpdateIssue(ctx, client, gqlClient, owner, repo, issueNumber, title, body, assignees, labels, milestoneNum, issueType, issueFieldValues, state, stateReason, duplicateOf) return result, nil, err default: 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 } -func CreateIssue(ctx context.Context, client *github.Client, owner string, repo string, title string, body string, assignees []string, labels []string, milestoneNum int, issueType string) (*mcp.CallToolResult, error) { +// 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, + 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 +} + +func CreateIssue(ctx context.Context, client *github.Client, owner string, repo string, title string, body string, assignees []string, labels []string, milestoneNum int, issueType string, issueFieldValues []*github.IssueRequestFieldValue) (*mcp.CallToolResult, error) { if title == "" { return utils.NewToolResultError("missing required parameter: title"), nil } // Create the issue request issueRequest := &github.IssueRequest{ - Title: github.Ptr(title), - Body: github.Ptr(body), - Assignees: &assignees, - Labels: &labels, + Title: github.Ptr(title), + Body: github.Ptr(body), + Assignees: &assignees, + Labels: &labels, + IssueFieldValues: issueFieldValues, } if milestoneNum != 0 { @@ -1692,7 +2144,7 @@ func CreateIssue(ctx context.Context, client *github.Client, owner string, repo return utils.NewToolResultText(string(r)), nil } -func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4.Client, owner string, repo string, issueNumber int, title string, body string, assignees []string, labels []string, milestoneNum int, issueType string, state string, stateReason string, duplicateOf int) (*mcp.CallToolResult, error) { +func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4.Client, owner string, repo string, issueNumber int, title string, body string, assignees []string, labels []string, milestoneNum int, issueType string, issueFieldValues []*github.IssueRequestFieldValue, state string, stateReason string, duplicateOf int) (*mcp.CallToolResult, error) { // Create the issue request with only provided fields issueRequest := &github.IssueRequest{} @@ -1721,6 +2173,10 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 issueRequest.Type = github.Ptr(issueType) } + if len(issueFieldValues) > 0 { + issueRequest.IssueFieldValues = issueFieldValues + } + updatedIssue, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueRequest) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, @@ -2083,7 +2539,7 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { } return result, nil, nil }) - st.FeatureFlagEnable = FeatureFlagIssueFields + st.FeatureFlagEnable = []string{FeatureFlagIssueFields} return st } @@ -2286,7 +2742,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..0c496d8b2a 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, FeatureFlagIssueFields} return st } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 3bac597225..6637454dc9 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -393,6 +393,78 @@ 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 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{ + Number: github.Ptr(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", + SingleSelectOption: &github.IssueFieldValueSingleSelectOption{ + ID: 42, + Name: "High", + Color: "red", + }, + }, + { + IssueFieldID: 1002, + NodeID: "FV_node_2", + DataType: "text", + Value: "some text value", + }, + }, + } + + mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockIssueWithFields), + }) + + cache := stubRepoAccessCache(nil, 15*time.Minute) + flags := stubFeatureFlags(map[string]bool{"lockdown-mode": false}) + deps := BaseDeps{ + Client: mustNewGHClient(t, mockedClient), + GQLClient: defaultGQLClient, + RepoAccessCache: cache, + Flags: flags, + } + 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) + + textContent := getTextResult(t, result) + + var returnedIssue MinimalIssue + err = json.Unmarshal([]byte(textContent.Text), &returnedIssue) + require.NoError(t, err) + + // 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) { // Verify tool definition once serverTool := AddIssueComment(translations.NullTranslationHelper) @@ -1103,7 +1175,7 @@ func Test_SearchIssues_FieldValuesEnrichment(t *testing.T) { require.Equal(t, 2, *response.Total) require.Len(t, response.Items, 2) assert.Equal(t, 42, *response.Items[0].Number) - assert.Equal(t, []MinimalIssueFieldValue{ + assert.Equal(t, []MinimalFieldValue{ {Field: "priority", Value: "P1"}, {Field: "estimate", Value: "2.5"}, }, response.Items[0].FieldValues) @@ -1115,7 +1187,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) @@ -1128,6 +1201,7 @@ func Test_CreateIssue(t *testing.T) { assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "labels") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "milestone") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "type") + assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "issue_fields") assert.ElementsMatch(t, tool.InputSchema.(*jsonschema.Schema).Required, []string{"method", "owner", "repo"}) // Setup mock issue for success case @@ -1144,12 +1218,13 @@ func Test_CreateIssue(t *testing.T) { } tests := []struct { - name string - mockedClient *http.Client - requestArgs map[string]any - expectError bool - expectedIssue *github.Issue - expectedErrMsg string + name string + mockedClient *http.Client + mockedGQLClient *http.Client + requestArgs map[string]any + expectError bool + expectedIssue *github.Issue + expectedErrMsg string }{ { name: "successful issue creation with all fields", @@ -1204,6 +1279,77 @@ func Test_CreateIssue(t *testing.T) { State: github.Ptr("open"), }, }, + { + name: "successful issue creation with issue fields reconciled by names", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesByOwnerByRepo: expectRequestBody(t, map[string]any{ + "title": "Issue with fields", + "body": "", + "labels": []any{}, + "assignees": []any{}, + "issue_field_values": []any{ + map[string]any{"field_id": float64(101), "value": "P1"}, + map[string]any{"field_id": float64(102), "value": "Acme"}, + }, + }).andThen( + mockResponse(t, http.StatusCreated, &github.Issue{ + Number: github.Ptr(125), + Title: github.Ptr("Issue with fields"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/125"), + State: github.Ptr("open"), + }), + ), + }), + mockedGQLClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + issueFieldWriteMetadataQuery{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issueFields": map[string]any{ + "nodes": []any{ + map[string]any{ + "__typename": "IssueFieldSingleSelect", + "fullDatabaseId": "101", + "name": "Priority", + "dataType": "single_select", + "options": []any{ + map[string]any{"fullDatabaseId": "9001", "name": "P1"}, + }, + }, + map[string]any{ + "__typename": "IssueFieldText", + "fullDatabaseId": "102", + "name": "Customer", + "dataType": "text", + }, + }, + }, + }, + }), + ), + ), + requestArgs: map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Issue with fields", + "issue_fields": []any{ + map[string]any{"field_name": "Priority", "field_option_name": "P1"}, + map[string]any{"field_name": "Customer", "value": "Acme"}, + }, + }, + expectError: false, + expectedIssue: &github.Issue{ + Number: github.Ptr(125), + Title: github.Ptr("Issue with fields"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/125"), + State: github.Ptr("open"), + }, + }, { name: "issue creation fails", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -1221,13 +1367,32 @@ func Test_CreateIssue(t *testing.T) { expectError: false, expectedErrMsg: "missing required parameter: title", }, + { + name: "issue_fields rejects both value and field_option_name", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}), + requestArgs: map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Invalid fields", + "issue_fields": []any{ + map[string]any{"field_name": "Priority", "value": "P1", "field_option_name": "P1"}, + }, + }, + expectError: false, + expectedErrMsg: "cannot specify both value and field_option_name", + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := mustNewGHClient(t, tc.mockedClient) - gqlClient := githubv4.NewClient(nil) + gqlHTTPClient := tc.mockedGQLClient + if gqlHTTPClient == nil { + gqlHTTPClient = githubv4mock.NewMockedHTTPClient() + } + gqlClient := githubv4.NewClient(gqlHTTPClient) deps := BaseDeps{ Client: client, GQLClient: gqlClient, @@ -1267,6 +1432,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) { @@ -1448,7 +1635,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) @@ -1811,9 +1998,9 @@ func Test_ListIssues(t *testing.T) { // (including float formatting); #789 has no field values. switch issue.Number { case 123: - assert.Equal(t, []MinimalIssueFieldValue{{Field: "priority", Value: "P1"}}, issue.FieldValues) + assert.Equal(t, []MinimalFieldValue{{Field: "priority", Value: "P1"}}, issue.FieldValues) case 456: - assert.Equal(t, []MinimalIssueFieldValue{ + assert.Equal(t, []MinimalFieldValue{ {Field: "due", Value: "2026-06-01"}, {Field: "estimate", Value: "2.5"}, {Field: "notes", Value: "needs triage"}, @@ -2375,7 +2562,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 @@ -2458,7 +2645,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) @@ -2475,6 +2662,7 @@ func Test_UpdateIssue(t *testing.T) { assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "state") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "state_reason") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "duplicate_of") + assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "issue_fields") assert.ElementsMatch(t, tool.InputSchema.(*jsonschema.Schema).Required, []string{"method", "owner", "repo"}) // Mock issues for reuse across test cases @@ -2586,6 +2774,63 @@ func Test_UpdateIssue(t *testing.T) { expectError: false, expectedIssue: mockUpdatedIssue, }, + { + name: "partial update with issue fields reconciled by names", + mockedRESTClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{ + "issue_field_values": []any{ + map[string]any{"field_id": float64(101), "value": "P1"}, + map[string]any{"field_id": float64(102), "value": "Acme"}, + }, + "title": "Updated Title", + }).andThen( + mockResponse(t, http.StatusOK, mockUpdatedIssue), + ), + }), + mockedGQLClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + issueFieldWriteMetadataQuery{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issueFields": map[string]any{ + "nodes": []any{ + map[string]any{ + "__typename": "IssueFieldSingleSelect", + "fullDatabaseId": "101", + "name": "Priority", + "dataType": "single_select", + "options": []any{map[string]any{"fullDatabaseId": "9001", "name": "P1"}}, + }, + map[string]any{ + "__typename": "IssueFieldText", + "fullDatabaseId": "102", + "name": "Customer", + "dataType": "text", + }, + }, + }, + }, + }), + ), + ), + requestArgs: map[string]any{ + "method": "update", + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + "title": "Updated Title", + "issue_fields": []any{ + map[string]any{"field_name": "Priority", "field_option_name": "P1"}, + map[string]any{"field_name": "Customer", "value": "Acme"}, + }, + }, + expectError: false, + expectedIssue: mockUpdatedIssue, + }, { name: "issue not found when updating non-state fields only", mockedRESTClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ diff --git a/pkg/github/minimal_types.go b/pkg/github/minimal_types.go index 02309db45b..5ad7656f06 100644 --- a/pkg/github/minimal_types.go +++ b/pkg/github/minimal_types.go @@ -220,6 +220,31 @@ type MinimalReactions struct { Eyes int `json:"eyes"` } +// MinimalIssueFieldValueSingleSelectOption is the trimmed output type for a single-select option of an issue field value. +type MinimalIssueFieldValueSingleSelectOption struct { + ID int64 `json:"id"` + Name string `json:"name"` + Color string `json:"color"` +} + +// MinimalIssueFieldValue is the trimmed output type for a custom field value attached to an issue, +// populated from REST API responses (e.g. get_issue). For GraphQL-sourced field values see MinimalFieldValue. +type MinimalIssueFieldValue struct { + IssueFieldID int64 `json:"issue_field_id,omitempty"` + NodeID string `json:"node_id,omitempty"` + DataType string `json:"data_type,omitempty"` + Value any `json:"value,omitempty"` + SingleSelectOption *MinimalIssueFieldValueSingleSelectOption `json:"single_select_option,omitempty"` +} + +// MinimalFieldValue is the trimmed output type for a custom field value resolved via GraphQL +// (e.g. list_issues, search_issues). Single-value variants populate Value; Values is reserved for multi-select. +type MinimalFieldValue struct { + Field string `json:"field"` + Value string `json:"value,omitempty"` + Values []string `json:"values,omitempty"` +} + // MinimalIssue is the trimmed output type for issue objects to reduce verbosity. type MinimalIssue struct { Number int `json:"number"` @@ -242,15 +267,8 @@ type MinimalIssue struct { ClosedAt string `json:"closed_at,omitempty"` ClosedBy string `json:"closed_by,omitempty"` IssueType string `json:"issue_type,omitempty"` - FieldValues []MinimalIssueFieldValue `json:"field_values,omitempty"` -} - -// MinimalIssueFieldValue is the trimmed output type for a custom issue field value. -// Single-value variants (date, number, single-select, text) populate Value. Values is reserved for multi-select. -type MinimalIssueFieldValue struct { - Field string `json:"field"` - Value string `json:"value,omitempty"` - Values []string `json:"values,omitempty"` + IssueFieldValues []MinimalIssueFieldValue `json:"issue_field_values,omitempty"` + FieldValues []MinimalFieldValue `json:"field_values,omitempty"` } // MinimalIssuesResponse is the trimmed output for a paginated list of issues. @@ -435,6 +453,26 @@ func convertToMinimalIssue(issue *github.Issue) MinimalIssue { m.IssueType = issueType.GetName() } + for _, fv := range issue.IssueFieldValues { + if fv == nil { + continue + } + mfv := MinimalIssueFieldValue{ + IssueFieldID: fv.IssueFieldID, + NodeID: fv.NodeID, + DataType: fv.DataType, + Value: fv.Value, + } + if opt := fv.SingleSelectOption; opt != nil { + mfv.SingleSelectOption = &MinimalIssueFieldValueSingleSelectOption{ + ID: opt.ID, + Name: opt.Name, + Color: opt.Color, + } + } + m.IssueFieldValues = append(m.IssueFieldValues, mfv) + } + if r := issue.Reactions; r != nil { m.Reactions = &MinimalReactions{ TotalCount: r.GetTotalCount(), @@ -471,7 +509,7 @@ func fragmentToMinimalIssue(fragment IssueFragment) MinimalIssue { } for _, fv := range fragment.IssueFieldValues.Nodes { - if mfv, ok := fragmentToMinimalIssueFieldValue(fv); ok { + if mfv, ok := fragmentToMinimalFieldValue(fv); ok { m.FieldValues = append(m.FieldValues, mfv) } } @@ -479,32 +517,32 @@ func fragmentToMinimalIssue(fragment IssueFragment) MinimalIssue { return m } -// fragmentToMinimalIssueFieldValue flattens the union value fragment into a single +// fragmentToMinimalFieldValue flattens the union value fragment into a single // {field, value} pair. Returns ok=false if the typename is unrecognised. -func fragmentToMinimalIssueFieldValue(fv IssueFieldValueFragment) (MinimalIssueFieldValue, bool) { +func fragmentToMinimalFieldValue(fv IssueFieldValueFragment) (MinimalFieldValue, bool) { switch fv.TypeName { case "IssueFieldDateValue": - return MinimalIssueFieldValue{ + return MinimalFieldValue{ Field: fv.DateValue.Field.Name(), Value: string(fv.DateValue.Value), }, true case "IssueFieldNumberValue": - return MinimalIssueFieldValue{ + return MinimalFieldValue{ Field: fv.NumberValue.Field.Name(), Value: strconv.FormatFloat(float64(fv.NumberValue.Value), 'f', -1, 64), }, true case "IssueFieldSingleSelectValue": - return MinimalIssueFieldValue{ + return MinimalFieldValue{ Field: fv.SingleSelectValue.Field.Name(), Value: string(fv.SingleSelectValue.Value), }, true case "IssueFieldTextValue": - return MinimalIssueFieldValue{ + return MinimalFieldValue{ Field: fv.TextValue.Field.Name(), Value: string(fv.TextValue.Value), }, true } - return MinimalIssueFieldValue{}, false + return MinimalFieldValue{}, false } func convertToMinimalIssuesResponse(fragment IssueQueryFragment) MinimalIssuesResponse { 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..a41897d1f2 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 } @@ -1141,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", ""), @@ -1163,7 +1230,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 +1255,7 @@ func TestFeatureFlagPrompts(t *testing.T) { { Prompt: mcp.Prompt{Name: "needs_flag"}, Toolset: testToolsetMetadata("toolset1"), - FeatureFlagEnable: "my_feature", + FeatureFlagEnable: []string{"my_feature"}, }, } @@ -1723,7 +1790,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 +1808,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