Skip to content

Commit 0bc4ae4

Browse files
fix: address PR review on issue-fields gating
- docs generator: install a no-flags feature checker so README reflects the default user experience (tools enabled with no special flags), fixing duplicate `list_issues` and removing granular/flagged-only tools that were never meant to appear in the default docs. - csv_output: drop the FeatureFlagEnable/Disable exclusion in isCSVOutputTool. Wrapping happens before the per-request flag filter picks the live variant, so flag-gated list_* tools wrap safely; this restores CSV conversion for `list_issues` and enables it for `list_issue_fields` when both flags are on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent f68771e commit 0bc4ae4

4 files changed

Lines changed: 41 additions & 193 deletions

File tree

README.md

Lines changed: 0 additions & 187 deletions
Original file line numberDiff line numberDiff line change
@@ -829,21 +829,6 @@ The following sets of tools are available:
829829
- `owner`: Repository owner (string, required)
830830
- `repo`: Repository name (string, required)
831831

832-
- **add_sub_issue** - Add Sub-Issue
833-
- **Required OAuth Scopes**: `repo`
834-
- `issue_number`: The parent issue number (number, required)
835-
- `owner`: Repository owner (username or organization) (string, required)
836-
- `replace_parent`: If true, reparent the sub-issue if it already has a parent (boolean, optional)
837-
- `repo`: Repository name (string, required)
838-
- `sub_issue_id`: The ID of the sub-issue to add. ID is not the same as issue number (number, required)
839-
840-
- **create_issue** - Create Issue
841-
- **Required OAuth Scopes**: `repo`
842-
- `body`: Issue body content (optional) (string, optional)
843-
- `owner`: Repository owner (username or organization) (string, required)
844-
- `repo`: Repository name (string, required)
845-
- `title`: Issue title (string, required)
846-
847832
- **get_label** - Get a specific label from a repository
848833
- **Required OAuth Scopes**: `repo`
849834
- `name`: Label name. (string, required)
@@ -885,30 +870,11 @@ The following sets of tools are available:
885870
- `title`: Issue title (string, optional)
886871
- `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)
887872

888-
- **list_issue_fields** - List issue fields
889-
- **Required OAuth Scopes**: `repo`, `read:org`
890-
- **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org`
891-
- `owner`: The account owner of the repository or organization. The name is not case sensitive. (string, required)
892-
- `repo`: The name of the repository. When provided, returns fields for this specific repository (inherited from its organization). When omitted, returns org-level fields directly. (string, optional)
893-
894873
- **list_issue_types** - List available issue types
895874
- **Required OAuth Scopes**: `read:org`
896875
- **Accepted OAuth Scopes**: `admin:org`, `read:org`, `write:org`
897876
- `owner`: The organization owner of the repository (string, required)
898877

899-
- **list_issues** - List issues
900-
- **Required OAuth Scopes**: `repo`
901-
- `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional)
902-
- `direction`: Order direction. If provided, the 'orderBy' also needs to be provided. (string, optional)
903-
- `field_filters`: Filter by custom issue field values. Each entry takes a field_name and a value; the server looks up the field and coerces the value to its type (single-select option name, text, number, or YYYY-MM-DD date). (object[], optional)
904-
- `labels`: Filter by labels (string[], optional)
905-
- `orderBy`: Order issues by field. If provided, the 'direction' also needs to be provided. (string, optional)
906-
- `owner`: Repository owner (string, required)
907-
- `perPage`: Results per page for pagination (min 1, max 100) (number, optional)
908-
- `repo`: Repository name (string, required)
909-
- `since`: Filter by date (ISO 8601 timestamp) (string, optional)
910-
- `state`: Filter by state, by default both open and closed issues are returned when not provided (string, optional)
911-
912878
- **list_issues** - List issues
913879
- **Required OAuth Scopes**: `repo`
914880
- `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional)
@@ -921,22 +887,6 @@ The following sets of tools are available:
921887
- `since`: Filter by date (ISO 8601 timestamp) (string, optional)
922888
- `state`: Filter by state, by default both open and closed issues are returned when not provided (string, optional)
923889

924-
- **remove_sub_issue** - Remove Sub-Issue
925-
- **Required OAuth Scopes**: `repo`
926-
- `issue_number`: The parent issue number (number, required)
927-
- `owner`: Repository owner (username or organization) (string, required)
928-
- `repo`: Repository name (string, required)
929-
- `sub_issue_id`: The ID of the sub-issue to remove. ID is not the same as issue number (number, required)
930-
931-
- **reprioritize_sub_issue** - Reprioritize Sub-Issue
932-
- **Required OAuth Scopes**: `repo`
933-
- `after_id`: The ID of the sub-issue to place this after (either after_id OR before_id should be specified) (number, optional)
934-
- `before_id`: The ID of the sub-issue to place this before (either after_id OR before_id should be specified) (number, optional)
935-
- `issue_number`: The parent issue number (number, required)
936-
- `owner`: Repository owner (username or organization) (string, required)
937-
- `repo`: Repository name (string, required)
938-
- `sub_issue_id`: The ID of the sub-issue to reorder. ID is not the same as issue number (number, required)
939-
940890
- **search_issues** - Search issues
941891
- **Required OAuth Scopes**: `repo`
942892
- `order`: Sort order (string, optional)
@@ -947,13 +897,6 @@ The following sets of tools are available:
947897
- `repo`: Optional repository name. If provided with owner, only issues for this repository are listed. (string, optional)
948898
- `sort`: Sort field by number of matches of categories, defaults to best match (string, optional)
949899

950-
- **set_issue_fields** - Set Issue Fields
951-
- **Required OAuth Scopes**: `repo`
952-
- `fields`: Array of issue field values to set. Each element must have a 'field_id' (string, the GraphQL node ID of the field) and exactly one value field: 'text_value' for text fields, 'number_value' for number fields, 'date_value' (ISO 8601 date string) for date fields, or 'single_select_option_id' (the GraphQL node ID of the option) for single select fields. Set 'delete' to true to remove a field value. (object[], required)
953-
- `issue_number`: The issue number to update (number, required)
954-
- `owner`: Repository owner (username or organization) (string, required)
955-
- `repo`: Repository name (string, required)
956-
957900
- **sub_issue_write** - Change sub-issue
958901
- **Required OAuth Scopes**: `repo`
959902
- `after_id`: The ID of the sub-issue to be prioritized after (either after_id OR before_id should be specified) (number, optional)
@@ -970,57 +913,6 @@ The following sets of tools are available:
970913
- `repo`: Repository name (string, required)
971914
- `sub_issue_id`: The ID of the sub-issue to add. ID is not the same as issue number (number, required)
972915

973-
- **update_issue_assignees** - Update Issue Assignees
974-
- **Required OAuth Scopes**: `repo`
975-
- `assignees`: GitHub usernames to assign to this issue (string[], required)
976-
- `issue_number`: The issue number to update (number, required)
977-
- `owner`: Repository owner (username or organization) (string, required)
978-
- `repo`: Repository name (string, required)
979-
980-
- **update_issue_body** - Update Issue Body
981-
- **Required OAuth Scopes**: `repo`
982-
- `body`: The new body content for the issue (string, required)
983-
- `issue_number`: The issue number to update (number, required)
984-
- `owner`: Repository owner (username or organization) (string, required)
985-
- `repo`: Repository name (string, required)
986-
987-
- **update_issue_labels** - Update Issue Labels
988-
- **Required OAuth Scopes**: `repo`
989-
- `issue_number`: The issue number to update (number, required)
990-
- `labels`: Labels to apply to this issue. ([], required)
991-
- `owner`: Repository owner (username or organization) (string, required)
992-
- `repo`: Repository name (string, required)
993-
994-
- **update_issue_milestone** - Update Issue Milestone
995-
- **Required OAuth Scopes**: `repo`
996-
- `issue_number`: The issue number to update (number, required)
997-
- `milestone`: The milestone number to set on the issue (integer, required)
998-
- `owner`: Repository owner (username or organization) (string, required)
999-
- `repo`: Repository name (string, required)
1000-
1001-
- **update_issue_state** - Update Issue State
1002-
- **Required OAuth Scopes**: `repo`
1003-
- `issue_number`: The issue number to update (number, required)
1004-
- `owner`: Repository owner (username or organization) (string, required)
1005-
- `repo`: Repository name (string, required)
1006-
- `state`: The new state for the issue (string, required)
1007-
- `state_reason`: The reason for the state change (only for closed state) (string, optional)
1008-
1009-
- **update_issue_title** - Update Issue Title
1010-
- **Required OAuth Scopes**: `repo`
1011-
- `issue_number`: The issue number to update (number, required)
1012-
- `owner`: Repository owner (username or organization) (string, required)
1013-
- `repo`: Repository name (string, required)
1014-
- `title`: The new title for the issue (string, required)
1015-
1016-
- **update_issue_type** - Update Issue Type
1017-
- **Required OAuth Scopes**: `repo`
1018-
- `issue_number`: The issue number to update (number, required)
1019-
- `issue_type`: The issue type to set (string, required)
1020-
- `owner`: Repository owner (username or organization) (string, required)
1021-
- `rationale`: One concise sentence explaining what specifically about the issue led you to choose this type. State the concrete signal (e.g. 'Reports a crash when saving' → bug, 'Asks for dark mode support' → feature). (string, optional)
1022-
- `repo`: Repository name (string, required)
1023-
1024916
</details>
1025917

1026918
<details>
@@ -1173,19 +1065,6 @@ The following sets of tools are available:
11731065
- `startSide`: For multi-line comments, the starting side of the diff that the comment applies to. LEFT indicates the previous state, RIGHT indicates the new state (string, optional)
11741066
- `subjectType`: The level at which the comment is targeted (string, required)
11751067

1176-
- **add_pull_request_review_comment** - Add Pull Request Review Comment
1177-
- **Required OAuth Scopes**: `repo`
1178-
- `body`: The comment body (string, required)
1179-
- `line`: The line number in the diff to comment on (optional) (number, optional)
1180-
- `owner`: Repository owner (username or organization) (string, required)
1181-
- `path`: The relative path of the file to comment on (string, required)
1182-
- `pullNumber`: The pull request number (number, required)
1183-
- `repo`: Repository name (string, required)
1184-
- `side`: The side of the diff to comment on (optional) (string, optional)
1185-
- `startLine`: The start line of a multi-line comment (optional) (number, optional)
1186-
- `startSide`: The start side of a multi-line comment (optional) (string, optional)
1187-
- `subjectType`: The subject type of the comment (string, required)
1188-
11891068
- **add_reply_to_pull_request_comment** - Add reply to pull request comment
11901069
- **Required OAuth Scopes**: `repo`
11911070
- `body`: The text of the reply (string, required)
@@ -1205,21 +1084,6 @@ The following sets of tools are available:
12051084
- `repo`: Repository name (string, required)
12061085
- `title`: PR title (string, required)
12071086

1208-
- **create_pull_request_review** - Create Pull Request Review
1209-
- **Required OAuth Scopes**: `repo`
1210-
- `body`: The review body text (optional) (string, optional)
1211-
- `commitID`: The SHA of the commit to review (optional, defaults to latest) (string, optional)
1212-
- `event`: The review action to perform. If omitted, creates a pending review. (string, optional)
1213-
- `owner`: Repository owner (username or organization) (string, required)
1214-
- `pullNumber`: The pull request number (number, required)
1215-
- `repo`: Repository name (string, required)
1216-
1217-
- **delete_pending_pull_request_review** - Delete Pending Pull Request Review
1218-
- **Required OAuth Scopes**: `repo`
1219-
- `owner`: Repository owner (username or organization) (string, required)
1220-
- `pullNumber`: The pull request number (number, required)
1221-
- `repo`: Repository name (string, required)
1222-
12231087
- **list_pull_requests** - List pull requests
12241088
- **Required OAuth Scopes**: `repo`
12251089
- `base`: Filter by base branch (string, optional)
@@ -1272,17 +1136,6 @@ The following sets of tools are available:
12721136
- `repo`: Repository name (string, required)
12731137
- `threadId`: The node ID of the review thread (e.g., PRRT_kwDOxxx). Required for resolve_thread and unresolve_thread methods. Get thread IDs from pull_request_read with method get_review_comments. (string, optional)
12741138

1275-
- **request_pull_request_reviewers** - Request Pull Request Reviewers
1276-
- **Required OAuth Scopes**: `repo`
1277-
- `owner`: Repository owner (username or organization) (string, required)
1278-
- `pullNumber`: The pull request number (number, required)
1279-
- `repo`: Repository name (string, required)
1280-
- `reviewers`: GitHub usernames to request reviews from (string[], required)
1281-
1282-
- **resolve_review_thread** - Resolve Review Thread
1283-
- **Required OAuth Scopes**: `repo`
1284-
- `threadID`: The node ID of the review thread to resolve (e.g., PRRT_kwDOxxx) (string, required)
1285-
12861139
- **search_pull_requests** - Search pull requests
12871140
- **Required OAuth Scopes**: `repo`
12881141
- `order`: Sort order (string, optional)
@@ -1293,18 +1146,6 @@ The following sets of tools are available:
12931146
- `repo`: Optional repository name. If provided with owner, only pull requests for this repository are listed. (string, optional)
12941147
- `sort`: Sort field by number of matches of categories, defaults to best match (string, optional)
12951148

1296-
- **submit_pending_pull_request_review** - Submit Pending Pull Request Review
1297-
- **Required OAuth Scopes**: `repo`
1298-
- `body`: The review body text (optional) (string, optional)
1299-
- `event`: The review action to perform (string, required)
1300-
- `owner`: Repository owner (username or organization) (string, required)
1301-
- `pullNumber`: The pull request number (number, required)
1302-
- `repo`: Repository name (string, required)
1303-
1304-
- **unresolve_review_thread** - Unresolve Review Thread
1305-
- **Required OAuth Scopes**: `repo`
1306-
- `threadID`: The node ID of the review thread to unresolve (e.g., PRRT_kwDOxxx) (string, required)
1307-
13081149
- **update_pull_request** - Edit pull request
13091150
- **Required OAuth Scopes**: `repo`
13101151
- `base`: New base branch name (string, optional)
@@ -1318,41 +1159,13 @@ The following sets of tools are available:
13181159
- `state`: New state (string, optional)
13191160
- `title`: New title (string, optional)
13201161

1321-
- **update_pull_request_body** - Update Pull Request Body
1322-
- **Required OAuth Scopes**: `repo`
1323-
- `body`: The new body content for the pull request (string, required)
1324-
- `owner`: Repository owner (username or organization) (string, required)
1325-
- `pullNumber`: The pull request number (number, required)
1326-
- `repo`: Repository name (string, required)
1327-
13281162
- **update_pull_request_branch** - Update pull request branch
13291163
- **Required OAuth Scopes**: `repo`
13301164
- `expectedHeadSha`: The expected SHA of the pull request's HEAD ref (string, optional)
13311165
- `owner`: Repository owner (string, required)
13321166
- `pullNumber`: Pull request number (number, required)
13331167
- `repo`: Repository name (string, required)
13341168

1335-
- **update_pull_request_draft_state** - Update Pull Request Draft State
1336-
- **Required OAuth Scopes**: `repo`
1337-
- `draft`: Set to true to convert to draft, false to mark as ready for review (boolean, required)
1338-
- `owner`: Repository owner (username or organization) (string, required)
1339-
- `pullNumber`: The pull request number (number, required)
1340-
- `repo`: Repository name (string, required)
1341-
1342-
- **update_pull_request_state** - Update Pull Request State
1343-
- **Required OAuth Scopes**: `repo`
1344-
- `owner`: Repository owner (username or organization) (string, required)
1345-
- `pullNumber`: The pull request number (number, required)
1346-
- `repo`: Repository name (string, required)
1347-
- `state`: The new state for the pull request (string, required)
1348-
1349-
- **update_pull_request_title** - Update Pull Request Title
1350-
- **Required OAuth Scopes**: `repo`
1351-
- `owner`: Repository owner (username or organization) (string, required)
1352-
- `pullNumber`: The pull request number (number, required)
1353-
- `repo`: Repository name (string, required)
1354-
- `title`: The new title for the pull request (string, required)
1355-
13561169
</details>
13571170

13581171
<details>

cmd/github-mcp-server/generate_docs.go

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@ func init() {
2929
rootCmd.AddCommand(generateDocsCmd)
3030
}
3131

32+
// noFeatureFlagsChecker reports every feature flag as disabled. It models the
33+
// default user experience used by the generated documentation.
34+
func noFeatureFlagsChecker(_ context.Context, _ string) (bool, error) {
35+
return false, nil
36+
}
37+
3238
func generateAllDocs() error {
3339
for _, doc := range []struct {
3440
path string
@@ -51,9 +57,16 @@ func generateReadmeDocs(readmePath string) error {
5157
// Create translation helper
5258
t, _ := translations.TranslationHelper()
5359

54-
// (not available to regular users) while including tools with FeatureFlagDisable.
60+
// The README documents the default user experience: tools that are
61+
// enabled with no special flags set. Installing a checker that reports
62+
// every flag as disabled excludes tools gated by FeatureFlagEnable and
63+
// keeps the legacy variants of tools gated by FeatureFlagDisable, so
64+
// flag-gated duplicates don't appear twice.
5565
// Build() can only fail if WithTools specifies invalid tools - not used here
56-
r, _ := github.NewInventory(t).WithToolsets([]string{"all"}).Build()
66+
r, _ := github.NewInventory(t).
67+
WithToolsets([]string{"all"}).
68+
WithFeatureChecker(noFeatureFlagsChecker).
69+
Build()
5770

5871
// Generate toolsets documentation
5972
toolsetsDoc := generateToolsetsDoc(r)

pkg/github/csv_output.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,16 @@ func withCSVOutput(tools []inventory.ServerTool) []inventory.ServerTool {
5656
return tools
5757
}
5858

59+
// isCSVOutputTool reports whether the given tool should have its handler
60+
// wrapped to honor the csv_output feature flag. Wrapping happens at slice
61+
// construction time, before the per-request feature-flag filter chooses which
62+
// variant of a flag-gated tool to register, so flag-gated list_* tools are
63+
// included on equal footing — only the live variant ever runs at request time.
5964
func isCSVOutputTool(tool inventory.ServerTool) bool {
6065
if !tool.Toolset.Default {
6166
return false
6267
}
63-
if !strings.HasPrefix(tool.Tool.Name, "list_") {
64-
return false
65-
}
66-
return tool.FeatureFlagEnable == "" && tool.FeatureFlagDisable == ""
68+
return strings.HasPrefix(tool.Tool.Name, "list_")
6769
}
6870

6971
func wrapHandlerWithCSVOutput(next inventory.HandlerFunc) inventory.HandlerFunc {

pkg/github/csv_output_test.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,26 @@ func TestCSVOutputAppliedToDefaultListTools(t *testing.T) {
3838
}
3939
}
4040

41+
func TestCSVOutputAppliesToFlagGatedListTools(t *testing.T) {
42+
enabledOnly := testCSVOutputTool("list_things", `[{"number":1}]`)
43+
enabledOnly.FeatureFlagEnable = FeatureFlagIssueFields
44+
disabledOnly := testCSVOutputTool("list_legacy_things", `[{"number":2}]`)
45+
disabledOnly.FeatureFlagDisable = FeatureFlagIssueFields
46+
47+
tools := withCSVOutput([]inventory.ServerTool{enabledOnly, disabledOnly})
48+
require.Len(t, tools, 2)
49+
50+
// Both flag-gated variants get the CSV wrapper; the per-request flag filter
51+
// decides which one actually registers, and the runtime csv_output check
52+
// decides whether the wrapper converts the response.
53+
deps := newCSVOutputTestDeps(true)
54+
for _, tool := range tools {
55+
result, err := tool.Handler(deps)(ContextWithDeps(context.Background(), deps), testCSVOutputRequest())
56+
require.NoError(t, err)
57+
assert.Contains(t, textResult(t, result), "number\n")
58+
}
59+
}
60+
4161
func TestCSVOutputOnlyAppliesToDefaultToolsets(t *testing.T) {
4262
nonDefaultListTool := testCSVOutputToolWithToolset("list_discussions", `[{"number":1}]`, ToolsetMetadataDiscussions)
4363

0 commit comments

Comments
 (0)