diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index 78ed8361a..74977c1a3 100644 --- a/cmd/github-mcp-server/generate_docs.go +++ b/cmd/github-mcp-server/generate_docs.go @@ -257,6 +257,8 @@ func writeToolDoc(buf *strings.Builder, tool inventory.ServerTool) { } sort.Strings(paramNames) + conditional := inventory.ConditionalSchemaPropertyDescriptions() + for i, propName := range paramNames { prop := schema.Properties[propName] required := slices.Contains(schema.Required, propName) @@ -282,7 +284,11 @@ func writeToolDoc(buf *strings.Builder, tool inventory.ServerTool) { // Indent any continuation lines in the description to maintain markdown formatting description := indentMultilineDescription(prop.Description, " ") - fmt.Fprintf(buf, " - `%s`: %s (%s, %s)", propName, description, typeStr, requiredStr) + if cond, isConditional := conditional[propName]; isConditional { + fmt.Fprintf(buf, " - `%s`: %s (%s, %s, conditional — %s)", propName, description, typeStr, requiredStr, cond) + } else { + fmt.Fprintf(buf, " - `%s`: %s (%s, %s)", propName, description, typeStr, requiredStr) + } if i < len(paramNames)-1 { buf.WriteString("\n") } diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 0b75a61ba..4e7e0f40d 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -44,6 +44,7 @@ runtime behavior (such as output formatting) won't appear here. - `maintainer_can_modify`: Allow maintainer edits (boolean, optional) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) + - `show_ui`: Whether to render the MCP App form instead of executing the request immediately. Defaults to true. Set to false to skip the form and execute directly — useful when you have all required values (especially ones the form does not collect, like reviewers) and the user has already confirmed the action. (boolean, optional, conditional — only visible to clients that advertise MCP App UI support) - `title`: PR title (string, required) - **get_me** - Get my user profile @@ -66,6 +67,7 @@ runtime behavior (such as output formatting) won't appear here. - `milestone`: Milestone number (number, optional) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) + - `show_ui`: Whether to render the MCP App form instead of executing the request immediately. Defaults to true. Set to false to skip the form and execute directly — useful when you have all required values (especially ones the form does not collect, like labels, assignees, milestone, type, or state changes) and the user has already confirmed the action. (boolean, optional, conditional — only visible to clients that advertise MCP App UI support) - `state`: New state (string, optional) - `state_reason`: Reason for the state change. Ignored unless state is changed. (string, optional) - `title`: Issue title (string, optional) @@ -240,7 +242,7 @@ runtime behavior (such as output formatting) won't appear here. - `owner`: Repository owner (username or organization) (string, required) - `pullNumber`: The pull request number (number, required) - `repo`: Repository name (string, required) - - `reviewers`: GitHub usernames to request reviews from (string[], required) + - `reviewers`: GitHub usernames or ORG/team-slug team reviewers to request reviews from (string[], required) - **resolve_review_thread** - Resolve Review Thread - **Required OAuth Scopes**: `repo` diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 881030f02..dc4037f0a 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -38,6 +38,7 @@ The list below is generated from the Go source. It covers tool **inventory and s - `maintainer_can_modify`: Allow maintainer edits (boolean, optional) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) + - `show_ui`: Whether to render the MCP App form instead of executing the request immediately. Defaults to true. Set to false to skip the form and execute directly — useful when you have all required values (especially ones the form does not collect, like reviewers) and the user has already confirmed the action. (boolean, optional, conditional — only visible to clients that advertise MCP App UI support) - `title`: PR title (string, required) - **get_me** - Get my user profile @@ -60,6 +61,7 @@ The list below is generated from the Go source. It covers tool **inventory and s - `milestone`: Milestone number (number, optional) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) + - `show_ui`: Whether to render the MCP App form instead of executing the request immediately. Defaults to true. Set to false to skip the form and execute directly — useful when you have all required values (especially ones the form does not collect, like labels, assignees, milestone, type, or state changes) and the user has already confirmed the action. (boolean, optional, conditional — only visible to clients that advertise MCP App UI support) - `state`: New state (string, optional) - `state_reason`: Reason for the state change. Ignored unless state is changed. (string, optional) - `title`: Issue title (string, optional) diff --git a/pkg/github/__toolsnaps__/create_pull_request.snap b/pkg/github/__toolsnaps__/create_pull_request.snap index a8a94ce69..5442aeda3 100644 --- a/pkg/github/__toolsnaps__/create_pull_request.snap +++ b/pkg/github/__toolsnaps__/create_pull_request.snap @@ -42,6 +42,10 @@ "description": "Repository name", "type": "string" }, + "show_ui": { + "description": "Whether to render the MCP App form instead of executing the request immediately. Defaults to true. Set to false to skip the form and execute directly — useful when you have all required values (especially ones the form does not collect, like reviewers) and the user has already confirmed the action.", + "type": "boolean" + }, "title": { "description": "PR title", "type": "string" diff --git a/pkg/github/__toolsnaps__/issue_write.snap b/pkg/github/__toolsnaps__/issue_write.snap index a125864f0..995bce120 100644 --- a/pkg/github/__toolsnaps__/issue_write.snap +++ b/pkg/github/__toolsnaps__/issue_write.snap @@ -60,6 +60,10 @@ "description": "Repository name", "type": "string" }, + "show_ui": { + "description": "Whether to render the MCP App form instead of executing the request immediately. Defaults to true. Set to false to skip the form and execute directly — useful when you have all required values (especially ones the form does not collect, like labels, assignees, milestone, type, or state changes) and the user has already confirmed the action.", + "type": "boolean" + }, "state": { "description": "New state", "enum": [ 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 index 6fb00d249..83896180c 100644 --- a/pkg/github/__toolsnaps__/issue_write_ff_remote_mcp_issue_fields.snap +++ b/pkg/github/__toolsnaps__/issue_write_ff_remote_mcp_issue_fields.snap @@ -96,6 +96,10 @@ "description": "Repository name", "type": "string" }, + "show_ui": { + "description": "Whether to render the MCP App form instead of executing the request immediately. Defaults to true. Set to false to skip the form and execute directly — useful when you have all required values (especially ones the form does not collect, like labels, assignees, milestone, type, issue_fields, or state changes) and the user has already confirmed the action.", + "type": "boolean" + }, "state": { "description": "New state", "enum": [ diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 6e9cdae53..0e048c9da 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1774,6 +1774,7 @@ var issueWriteFormParams = map[string]struct{}{ "title": {}, "body": {}, "issue_number": {}, + "show_ui": {}, "_ui_submitted": {}, } @@ -1918,6 +1919,17 @@ Options are: Required: []string{"field_name"}, }, }, + // show_ui is hidden from clients that do not advertise MCP App + // UI support. The strip happens per-request in + // inventory.ToolsForRegistration; it is present in the static + // schema (and therefore in toolsnaps and the feature-flag / + // insiders docs) so the UI-capable surface is fully + // documented. It is intentionally not in the main README, + // which renders the stripped (non-UI) schema. + "show_ui": { + Type: "boolean", + Description: "Whether to render the MCP App form instead of executing the request immediately. Defaults to true. Set to false to skip the form and execute directly — useful when you have all required values (especially ones the form does not collect, like labels, assignees, milestone, type, issue_fields, or state changes) and the user has already confirmed the action.", + }, }, Required: []string{"method", "owner", "repo"}, }, @@ -1939,13 +1951,19 @@ Options are: } // When MCP Apps are enabled and the client supports UI, route the - // call to the interactive form unless it is itself a form submission - // (the UI sends _ui_submitted=true) or it carries parameters the form - // cannot represent (e.g. labels, assignees or issue_fields). Those - // must be applied directly so their values aren't silently dropped. + // call to the interactive form unless: + // - it is itself a form submission (the UI sends _ui_submitted=true), + // - the caller explicitly asked to skip the UI (show_ui=false), or + // - it carries parameters the form cannot represent (e.g. labels, + // assignees or issue_fields). Those must be applied directly so + // their values aren't silently dropped. uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted") + showUI, err := OptionalBoolParamWithDefault(args, "show_ui", true) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !issueWriteHasNonFormParams(args) { + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && showUI && !issueWriteHasNonFormParams(args) { if method == "update" { issueNumber, numErr := RequiredInt(args, "issue_number") if numErr != nil { @@ -2150,6 +2168,17 @@ Options are: Type: "number", Description: "Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'.", }, + // show_ui is hidden from clients that do not advertise MCP App + // UI support. The strip happens per-request in + // inventory.ToolsForRegistration; it is present in the static + // schema (and therefore in toolsnaps and the feature-flag / + // insiders docs) so the UI-capable surface is fully + // documented. It is intentionally not in the main README, + // which renders the stripped (non-UI) schema. + "show_ui": { + Type: "boolean", + Description: "Whether to render the MCP App form instead of executing the request immediately. Defaults to true. Set to false to skip the form and execute directly — useful when you have all required values (especially ones the form does not collect, like labels, assignees, milestone, type, or state changes) and the user has already confirmed the action.", + }, }, Required: []string{"method", "owner", "repo"}, }, @@ -2171,13 +2200,19 @@ Options are: } // When MCP Apps are enabled and the client supports UI, route the - // call to the interactive form unless it is itself a form submission - // (the UI sends _ui_submitted=true) or it carries parameters the form - // cannot represent (e.g. labels, assignees or issue_fields). Those - // must be applied directly so their values aren't silently dropped. + // call to the interactive form unless: + // - it is itself a form submission (the UI sends _ui_submitted=true), + // - the caller explicitly asked to skip the UI (show_ui=false), or + // - it carries parameters the form cannot represent (e.g. labels, + // assignees or issue_fields). Those must be applied directly so + // their values aren't silently dropped. uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted") + showUI, err := OptionalBoolParamWithDefault(args, "show_ui", true) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !issueWriteHasNonFormParams(args) { + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && showUI && !issueWriteHasNonFormParams(args) { if method == "update" { issueNumber, numErr := RequiredInt(args, "issue_number") if numErr != nil { diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index d794ad167..ca8dbf28a 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -15,6 +15,7 @@ import ( "github.com/github/github-mcp-server/internal/toolsnaps" "github.com/github/github-mcp-server/pkg/http/headers" transportpkg "github.com/github/github-mcp-server/pkg/http/transport" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v87/github" "github.com/google/jsonschema-go/jsonschema" @@ -1794,6 +1795,86 @@ func Test_IssueWrite_MCPAppsFeature_UIGate(t *testing.T) { assert.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/1", "labels call should execute directly and return issue URL") }) + + t.Run("UI client with show_ui=false skips form and executes directly", func(t *testing.T) { + // show_ui=false is the explicit, model-facing way to opt out of the + // form. It must bypass the form even when every other condition would + // route the call there (UI capability, MCP Apps flag on, no + // _ui_submitted, only form params present). + request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Test", + "show_ui": false, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + assert.NotContains(t, textContent.Text, "Ready to create an issue", + "show_ui=false should skip UI form") + assert.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/1", + "show_ui=false call should execute directly and return issue URL") + }) + + t.Run("UI client with show_ui=true returns form message", func(t *testing.T) { + // show_ui=true is the explicit, redundant-with-the-default way to ask + // for the form. It must still route through the form and must not be + // treated as a non-form parameter that would trigger the safety-net + // bypass. + request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Test", + "show_ui": true, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, "Ready to create an issue", + "show_ui=true should still route through the form") + }) + + t.Run("UI client with show_ui=false and _ui_submitted=true executes directly", func(t *testing.T) { + // _ui_submitted and show_ui=false are two ways to say "execute + // directly". When both are set there must be no conflict — the call + // still executes directly. + request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Test", + "show_ui": false, + "_ui_submitted": true, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/1", + "show_ui=false + _ui_submitted should execute directly") + }) + + t.Run("non-UI client with show_ui=false executes directly (no regression)", func(t *testing.T) { + // show_ui is irrelevant when the client does not support UI; the call + // must execute directly exactly as it does today. + request := createMCPRequest(map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Test", + "show_ui": false, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/1", + "non-UI client should execute directly regardless of show_ui") + }) } func Test_issueWriteHasNonFormParams(t *testing.T) { @@ -1806,6 +1887,8 @@ func Test_issueWriteHasNonFormParams(t *testing.T) { }{ {name: "no params", args: map[string]any{}, want: false}, {name: "only form params", args: map[string]any{"method": "create", "owner": "o", "repo": "r", "title": "t", "body": "b", "issue_number": float64(1), "_ui_submitted": true}, want: false}, + {name: "show_ui true is a form param", args: map[string]any{"title": "t", "show_ui": true}, want: false}, + {name: "show_ui false is a form param", args: map[string]any{"title": "t", "show_ui": false}, want: false}, {name: "labels present", args: map[string]any{"title": "t", "labels": []any{"bug"}}, want: true}, {name: "assignees present", args: map[string]any{"title": "t", "assignees": []any{"octocat"}}, want: true}, {name: "milestone present", args: map[string]any{"title": "t", "milestone": float64(2)}, want: true}, @@ -1825,6 +1908,56 @@ func Test_issueWriteHasNonFormParams(t *testing.T) { } } +// Test_issueWriteSchemaClassification fails when a schema property is added +// without classifying it as either form-resendable (issueWriteFormParams) or +// known-non-form (knownNonForm below). Without this guard, an unclassified +// property would silently flip UI gating: form-incompatible fields would +// stop tripping the safety-net bypass and the form would drop their values. +func Test_issueWriteSchemaClassification(t *testing.T) { + t.Parallel() + + // Schema properties the MCP App form cannot represent — their presence + // must trigger the safety-net bypass via issueWriteHasNonFormParams. + knownNonForm := map[string]struct{}{ + "assignees": {}, + "labels": {}, + "milestone": {}, + "type": {}, + "state": {}, + "state_reason": {}, + "duplicate_of": {}, + "issue_fields": {}, // only on the FF-enabled IssueWrite variant + } + + cases := []struct { + name string + tool inventory.ServerTool + }{ + {name: "IssueWrite", tool: IssueWrite(translations.NullTranslationHelper)}, + {name: "LegacyIssueWrite", tool: LegacyIssueWrite(translations.NullTranslationHelper)}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + schema, ok := tc.tool.Tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + + for prop := range schema.Properties { + _, isForm := issueWriteFormParams[prop] + _, isNonForm := knownNonForm[prop] + + assert.Falsef(t, isForm && isNonForm, + "property %q is classified as both form-resendable and non-form — pick one", prop) + assert.Truef(t, isForm || isNonForm, + "property %q in %s schema is unclassified — add it to issueWriteFormParams (pkg/github/issues.go) "+ + "if the MCP App form can carry it on submit, otherwise add it to the knownNonForm allowlist in this test", + prop, tc.name) + } + }) + } +} + func Test_ListIssues(t *testing.T) { // Verify tool definition serverTool := ListIssues(translations.NullTranslationHelper) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 05028850d..63338ca63 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -556,6 +556,7 @@ var pullRequestWriteFormParams = map[string]struct{}{ "base": {}, "draft": {}, "maintainer_can_modify": {}, + "show_ui": {}, "_ui_submitted": {}, } @@ -627,6 +628,17 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo Type: "boolean", Description: "Allow maintainer edits", }, + // show_ui is hidden from clients that do not advertise MCP App + // UI support. The strip happens per-request in + // inventory.ToolsForRegistration; it is present in the static + // schema (and therefore in toolsnaps and the feature-flag / + // insiders docs) so the UI-capable surface is fully + // documented. It is intentionally not in the main README, + // which renders the stripped (non-UI) schema. + "show_ui": { + Type: "boolean", + Description: "Whether to render the MCP App form instead of executing the request immediately. Defaults to true. Set to false to skip the form and execute directly — useful when you have all required values (especially ones the form does not collect, like reviewers) and the user has already confirmed the action.", + }, }, Required: []string{"owner", "repo", "title", "head", "base"}, }, @@ -643,13 +655,18 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo } // When MCP Apps are enabled and the client supports UI, route the - // call to the interactive form unless it is itself a form submission - // (the UI sends _ui_submitted=true) or it carries parameters the form - // cannot represent. Those must be applied directly so their values - // aren't silently dropped. + // call to the interactive form unless: + // - it is itself a form submission (the UI sends _ui_submitted=true), + // - the caller explicitly asked to skip the UI (show_ui=false), or + // - it carries parameters the form cannot represent. Those must be + // applied directly so their values aren't silently dropped. uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted") + showUI, err := OptionalBoolParamWithDefault(args, "show_ui", true) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !pullRequestWriteHasNonFormParams(args) { + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && showUI && !pullRequestWriteHasNonFormParams(args) { return utils.NewToolResultText(fmt.Sprintf("Ready to create a pull request in %s/%s. IMPORTANT: The PR has NOT been created yet. Do NOT tell the user the PR was created. The user MUST click Submit in the form to create it.", owner, repo)), nil, nil } diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index aff71e4c1..ae8f3c66c 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -2506,6 +2506,89 @@ func Test_CreatePullRequest_MCPAppsFeature_UIGate(t *testing.T) { assert.Contains(t, textContent.Text, "https://github.com/owner/repo/pull/42", "non-form param call should execute directly and return PR URL") }) + + t.Run("UI client with show_ui=false skips form and executes directly", func(t *testing.T) { + // show_ui=false is the explicit, model-facing way to opt out of the + // form. It must bypass the form even when every other condition would + // route the call there (UI capability, MCP Apps flag on, no + // _ui_submitted, only form params present). + request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{ + "owner": "owner", + "repo": "repo", + "title": "Test PR", + "head": "feature", + "base": "main", + "show_ui": false, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + assert.NotContains(t, textContent.Text, "Ready to create a pull request", + "show_ui=false should skip UI form") + assert.Contains(t, textContent.Text, "https://github.com/owner/repo/pull/42", + "show_ui=false call should execute directly and return PR URL") + }) + + t.Run("UI client with show_ui=true returns form message", func(t *testing.T) { + // show_ui=true must still route through the form and must not be + // treated as a non-form parameter that would trigger the safety-net + // bypass. + request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{ + "owner": "owner", + "repo": "repo", + "title": "Test PR", + "head": "feature", + "base": "main", + "show_ui": true, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, "Ready to create a pull request", + "show_ui=true should still route through the form") + }) + + t.Run("UI client with show_ui=false and _ui_submitted=true executes directly", func(t *testing.T) { + // _ui_submitted and show_ui=false are two ways to say "execute + // directly". When both are set there must be no conflict — the call + // still executes directly. + request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{ + "owner": "owner", + "repo": "repo", + "title": "Test PR", + "head": "feature", + "base": "main", + "show_ui": false, + "_ui_submitted": true, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, "https://github.com/owner/repo/pull/42", + "show_ui=false + _ui_submitted should execute directly") + }) + + t.Run("non-UI client with show_ui=false executes directly (no regression)", func(t *testing.T) { + // show_ui is irrelevant when the client does not support UI; the call + // must execute directly exactly as it does today. + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "title": "Test PR", + "head": "feature", + "base": "main", + "show_ui": false, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, "https://github.com/owner/repo/pull/42", + "non-UI client should execute directly regardless of show_ui") + }) } func Test_pullRequestWriteHasNonFormParams(t *testing.T) { @@ -2518,6 +2601,8 @@ func Test_pullRequestWriteHasNonFormParams(t *testing.T) { }{ {name: "no params", args: map[string]any{}, want: false}, {name: "only form params", args: map[string]any{"owner": "o", "repo": "r", "title": "t", "body": "b", "head": "h", "base": "b", "draft": true, "maintainer_can_modify": false, "_ui_submitted": true}, want: false}, + {name: "show_ui true is a form param", args: map[string]any{"title": "t", "show_ui": true}, want: false}, + {name: "show_ui false is a form param", args: map[string]any{"title": "t", "show_ui": false}, want: false}, {name: "unknown param present", args: map[string]any{"title": "t", "reviewers": []any{"octocat"}}, want: true}, {name: "nil value is ignored", args: map[string]any{"reviewers": nil}, want: false}, } @@ -2530,6 +2615,32 @@ func Test_pullRequestWriteHasNonFormParams(t *testing.T) { } } +// Test_createPullRequestSchemaClassification fails when a schema property is +// added without classifying it as either form-resendable +// (pullRequestWriteFormParams) or known-non-form (knownNonForm below). +// Today every property is form-resendable, so knownNonForm is empty. +func Test_createPullRequestSchemaClassification(t *testing.T) { + t.Parallel() + + knownNonForm := map[string]struct{}{} + + tool := CreatePullRequest(translations.NullTranslationHelper) + schema, ok := tool.Tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + + for prop := range schema.Properties { + _, isForm := pullRequestWriteFormParams[prop] + _, isNonForm := knownNonForm[prop] + + assert.Falsef(t, isForm && isNonForm, + "property %q is classified as both form-resendable and non-form — pick one", prop) + assert.Truef(t, isForm || isNonForm, + "property %q in create_pull_request schema is unclassified — add it to pullRequestWriteFormParams "+ + "(pkg/github/pullrequests.go) if the MCP App form can carry it on submit, otherwise add it to "+ + "the knownNonForm allowlist in this test", prop) + } +} + func TestCreateAndSubmitPullRequestReview(t *testing.T) { t.Parallel() diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 9ecaca1f5..c8a9c21bc 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -7,6 +7,8 @@ import ( "maps" "slices" "strings" + + "github.com/google/jsonschema-go/jsonschema" ) var ( @@ -406,6 +408,97 @@ func stripMCPAppsMetadata(tools []ServerTool) []ServerTool { return result } +// uiOnlySchemaProperties lists input-schema property names that should only +// be visible to clients that advertise MCP Apps UI support. They live on the +// static schema (so toolsnaps and the feature-flag / insiders docs document +// the full UI-capable surface; the main README renders the stripped +// non-UI schema) and are stripped per-request when the same gate that hides +// _meta.ui is true. +var uiOnlySchemaProperties = []string{ + "show_ui", // explicit "render the MCP App form" toggle on form-backed write tools +} + +// ConditionalSchemaPropertyDescriptions returns a map of schema property name +// to a human-readable description of the condition under which the property +// is visible to clients. The doc generator uses this to annotate conditional +// parameters so readers can see at a glance which fields are not always +// available. This is the single source of truth for the conditional-property +// surface — entries here must correspond to a strip rule in +// ToolsForRegistration. +func ConditionalSchemaPropertyDescriptions() map[string]string { + const uiOnlyCondition = "visible when remote_mcp_ui_apps is enabled unless the client explicitly indicates it does not support io.modelcontextprotocol/ui" + out := make(map[string]string, len(uiOnlySchemaProperties)) + for _, name := range uiOnlySchemaProperties { + out[name] = uiOnlyCondition + } + return out +} + +// stripUIOnlySchemaProperties removes UI-capability-gated input-schema +// properties (currently just "show_ui") from each tool's static schema. +// Tools whose InputSchema is not a *jsonschema.Schema (e.g. json.RawMessage) +// are passed through untouched — no such tool currently declares a gated +// property, and inferring intent from an opaque schema is not safe. +// Tools without any gated property are returned as-is so we only allocate +// when a change is actually made (mirrors the stripMetaKeys pattern). +func stripUIOnlySchemaProperties(tools []ServerTool) []ServerTool { + result := make([]ServerTool, 0, len(tools)) + for _, tool := range tools { + if stripped := stripSchemaProperties(tool, uiOnlySchemaProperties); stripped != nil { + result = append(result, *stripped) + } else { + result = append(result, tool) + } + } + return result +} + +// stripSchemaProperties removes the named keys from tool.Tool.InputSchema's +// Properties map (and Required list, if present) and returns a modified copy. +// Returns nil when the schema is not a *jsonschema.Schema or no listed key +// is present, signalling no change. +func stripSchemaProperties(tool ServerTool, keys []string) *ServerTool { + if tool.Tool.InputSchema == nil || len(keys) == 0 { + return nil + } + schema, ok := tool.Tool.InputSchema.(*jsonschema.Schema) + if !ok || schema == nil || len(schema.Properties) == 0 { + return nil + } + + hasKey := false + for _, key := range keys { + if _, exists := schema.Properties[key]; exists { + hasKey = true + break + } + } + if !hasKey { + return nil + } + + toolCopy := tool + schemaCopy := *schema + newProps := make(map[string]*jsonschema.Schema, len(schema.Properties)) + for k, v := range schema.Properties { + if !slices.Contains(keys, k) { + newProps[k] = v + } + } + schemaCopy.Properties = newProps + if len(schemaCopy.Required) > 0 { + newRequired := make([]string, 0, len(schemaCopy.Required)) + for _, r := range schemaCopy.Required { + if !slices.Contains(keys, r) { + newRequired = append(newRequired, r) + } + } + schemaCopy.Required = newRequired + } + toolCopy.Tool.InputSchema = &schemaCopy + return &toolCopy +} + // stripMetaKeys removes the specified Meta keys from a single tool. // Returns a modified copy if changes were made, nil otherwise. func stripMetaKeys(tool ServerTool, keys []string) *ServerTool { diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index b8a70a342..101f8ee94 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -169,8 +169,9 @@ func (r *Inventory) ToolsetDescriptions() map[ToolsetID]string { } // ToolsForRegistration returns AvailableTools(ctx) post-processed exactly as -// RegisterTools would expose them: with MCP Apps UI metadata stripped when -// the client cannot consume it. Useful for documentation generators and +// RegisterTools would expose them: with MCP Apps UI metadata stripped and +// UI-capability-gated input-schema properties (e.g. show_ui) removed when +// the client cannot consume them. Useful for documentation generators and // diagnostics that need the same view of the tool surface the server would // register. // @@ -186,6 +187,7 @@ func (r *Inventory) ToolsForRegistration(ctx context.Context) []ServerTool { tools := r.AvailableTools(ctx) if shouldStripMCPAppsMetadata(ctx, r.checkFeatureFlag(ctx, mcpAppsFeatureFlag)) { tools = stripMCPAppsMetadata(tools) + tools = stripUIOnlySchemaProperties(tools) } return tools } @@ -206,9 +208,10 @@ func shouldStripMCPAppsMetadata(ctx context.Context, featureFlagEnabled bool) bo // RegisterTools registers all available tools with the server using the provided dependencies. // The context is used for feature flag evaluation and client capability checks. // -// MCP Apps UI metadata (`_meta.ui`) is stripped from the registered tools -// when either the MCP Apps feature flag is not enabled for this request, or -// the client did not advertise the io.modelcontextprotocol/ui extension. The +// MCP Apps UI metadata (`_meta.ui`) and UI-capability-gated input-schema +// properties (e.g. `show_ui`) are stripped from the registered tools when +// either the MCP Apps feature flag is not enabled for this request, or the +// client did not advertise the io.modelcontextprotocol/ui extension. The // strip happens here (rather than at Build() time) so the per-request // context is in scope — HTTP feature checkers that read insiders mode or // user identity from ctx would otherwise see context.Background() and diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 20b1fb718..bcdd70f00 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -4,9 +4,12 @@ import ( "context" "encoding/json" "fmt" + "maps" + "slices" "testing" ghcontext "github.com/github/github-mcp-server/pkg/context" + "github.com/google/jsonschema-go/jsonschema" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/require" ) @@ -2019,6 +2022,267 @@ func TestStripMCPAppsMetadata(t *testing.T) { require.Nil(t, result[2].Tool.Meta) } +// mockToolWithSchema creates a ServerTool with the given *jsonschema.Schema as +// InputSchema. Used to exercise schema-based strip helpers. +func mockToolWithSchema(name string, toolsetID string, schema *jsonschema.Schema) ServerTool { + return NewServerTool( + mcp.Tool{ + Name: name, + Annotations: &mcp.ToolAnnotations{ + ReadOnlyHint: true, + }, + InputSchema: schema, + }, + testToolsetMetadata(toolsetID), + func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return nil, nil + }, + ) +} + +func TestStripSchemaProperties(t *testing.T) { + tests := []struct { + name string + schema any + keys []string + expectChange bool + wantProperties []string // property names expected to remain (order-independent) + wantRequired []string // required fields expected to remain (order-independent) + }{ + { + name: "nil schema - no change", + schema: nil, + keys: []string{"show_ui"}, + expectChange: false, + }, + { + name: "RawMessage schema - skipped (not a *jsonschema.Schema)", + schema: json.RawMessage(`{"type":"object","properties":{"show_ui":{"type":"boolean"}}}`), + keys: []string{"show_ui"}, + expectChange: false, + }, + { + name: "schema without the key - no change", + schema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": {Type: "string"}, + }, + }, + keys: []string{"show_ui"}, + expectChange: false, + }, + { + name: "empty keys list - no change", + schema: &jsonschema.Schema{Type: "object", Properties: map[string]*jsonschema.Schema{"show_ui": {Type: "boolean"}}}, + keys: []string{}, + expectChange: false, + }, + { + name: "schema with the key - stripped, others preserved", + schema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": {Type: "string"}, + "repo": {Type: "string"}, + "show_ui": {Type: "boolean"}, + }, + Required: []string{"owner", "repo"}, + }, + keys: []string{"show_ui"}, + expectChange: true, + wantProperties: []string{"owner", "repo"}, + wantRequired: []string{"owner", "repo"}, + }, + { + name: "key in required list is also stripped", + schema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": {Type: "string"}, + "show_ui": {Type: "boolean"}, + }, + Required: []string{"owner", "show_ui"}, + }, + keys: []string{"show_ui"}, + expectChange: true, + wantProperties: []string{"owner"}, + wantRequired: []string{"owner"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tool := NewServerTool( + mcp.Tool{ + Name: "test", + Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, + InputSchema: tt.schema, + }, + testToolsetMetadata("toolset1"), + func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return nil, nil + }, + ) + + result := stripSchemaProperties(tool, tt.keys) + + if !tt.expectChange { + require.Nil(t, result, "expected no change but got result") + return + } + + require.NotNil(t, result, "expected change but got nil") + schema, ok := result.Tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "result schema should remain *jsonschema.Schema") + require.ElementsMatch(t, tt.wantProperties, slices.Collect(maps.Keys(schema.Properties))) + require.ElementsMatch(t, tt.wantRequired, schema.Required) + + // Original schema must not be mutated. + origSchema := tt.schema.(*jsonschema.Schema) + _, stillThere := origSchema.Properties["show_ui"] + require.True(t, stillThere || !slices.Contains(tt.keys, "show_ui"), "original schema should not be mutated") + }) + } +} + +func TestStripUIOnlySchemaProperties(t *testing.T) { + tools := []ServerTool{ + mockToolWithSchema("with_show_ui", "toolset1", &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": {Type: "string"}, + "show_ui": {Type: "boolean"}, + }, + }), + mockToolWithSchema("without_show_ui", "toolset1", &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": {Type: "string"}, + }, + }), + mockTool("raw_schema_tool", "toolset1", true), // InputSchema is json.RawMessage + } + + result := stripUIOnlySchemaProperties(tools) + require.Len(t, result, 3) + + stripped := result[0].Tool.InputSchema.(*jsonschema.Schema) + require.NotContains(t, stripped.Properties, "show_ui", + "show_ui should be stripped from a tool that declares it") + require.Contains(t, stripped.Properties, "owner", + "other properties on the same schema must be preserved") + + // Tool without show_ui: same value returned (no allocation), schema untouched. + require.Same(t, tools[1].Tool.InputSchema, result[1].Tool.InputSchema, + "tools without the gated property must be returned unchanged") + + // Tool with an opaque (json.RawMessage) schema: passed through untouched. + require.Equal(t, tools[2].Tool.InputSchema, result[2].Tool.InputSchema, + "tools with a non-*jsonschema.Schema input schema must be passed through") +} + +// TestConditionalSchemaPropertyDescriptions ensures every property that +// inventory strips per-request also has a human-readable condition the doc +// generator can render. A future addition to uiOnlySchemaProperties that +// forgets to wire a description through will fail here. +func TestConditionalSchemaPropertyDescriptions(t *testing.T) { + t.Parallel() + + descs := ConditionalSchemaPropertyDescriptions() + require.NotEmpty(t, descs, "expected at least show_ui to be advertised as conditional") + + for _, name := range uiOnlySchemaProperties { + desc, ok := descs[name] + require.Truef(t, ok, "ui-only property %q must have a conditional description", name) + require.NotEmptyf(t, desc, "conditional description for %q must be non-empty", name) + } +} + +func TestToolsForRegistration_StripsShowUIUnderSameGate(t *testing.T) { + // A tool whose schema declares both `_meta.ui` and `show_ui`. The strip + // for both must fire — or not — together, governed by the same gate + // already covered by TestShouldStripMCPAppsMetadata. + makeTool := func() ServerTool { + st := mockToolWithSchema("ui_tool", "toolset1", &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": {Type: "string"}, + "show_ui": {Type: "boolean"}, + }, + }) + st.Tool.Meta = map[string]any{ + "ui": map[string]any{"resourceUri": "ui://example"}, + "description": "kept", + } + return st + } + + mcpAppsChecker := func(_ context.Context, flag string) (bool, error) { + return flag == mcpAppsFeatureFlag, nil + } + + tests := []struct { + name string + ctx context.Context + ffOn bool + wantShowUI bool // expect show_ui to remain in registered schema + wantUIMeta bool // expect _meta.ui to remain on registered tool + }{ + { + name: "FF off, capability unknown -> both stripped", + ctx: context.Background(), + ffOn: false, + wantShowUI: false, + wantUIMeta: false, + }, + { + name: "FF on, capability unknown -> both kept", + ctx: context.Background(), + ffOn: true, + wantShowUI: true, + wantUIMeta: true, + }, + { + name: "FF on, capability present -> both kept", + ctx: ghcontext.WithUISupport(context.Background(), true), + ffOn: true, + wantShowUI: true, + wantUIMeta: true, + }, + { + name: "FF on, capability explicitly absent -> both stripped", + ctx: ghcontext.WithUISupport(context.Background(), false), + ffOn: true, + wantShowUI: false, + wantUIMeta: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + builder := NewBuilder().SetTools([]ServerTool{makeTool()}).WithToolsets([]string{"all"}) + if tc.ffOn { + builder = builder.WithFeatureChecker(mcpAppsChecker) + } + reg := mustBuild(t, builder) + + registered := reg.ToolsForRegistration(tc.ctx) + require.Len(t, registered, 1) + schema, ok := registered[0].Tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok) + + _, hasShowUI := schema.Properties["show_ui"] + require.Equal(t, tc.wantShowUI, hasShowUI, + "show_ui presence in registered schema should match strip gate") + + _, hasUIMeta := registered[0].Tool.Meta["ui"] + require.Equal(t, tc.wantUIMeta, hasUIMeta, + "_meta.ui presence on registered tool should match strip gate") + }) + } +} + func TestStripMetaKeys_MultipleKeys(t *testing.T) { // This test verifies the mechanism works for multiple keys keys := []string{"ui", "experimental_feature", "beta"} @@ -2203,23 +2467,17 @@ func TestCreateExcludeToolsFilter(t *testing.T) { // captureRegisteredTools mirrors RegisterTools' per-request strip behavior so // tests can verify what the wire sees, without requiring tools to have real -// handlers (RegisterTools panics on tools without HandlerFunc). +// handlers (RegisterTools panics on tools without HandlerFunc). It delegates +// to ToolsForRegistration so any future strip added there is picked up +// automatically. func captureRegisteredTools(ctx context.Context, t *testing.T, reg *Inventory) []*mcp.Tool { t.Helper() - tools := reg.AvailableTools(ctx) - out := make([]*mcp.Tool, 0, len(tools)) - for i := range tools { - toolCopy := tools[i].Tool + forReg := reg.ToolsForRegistration(ctx) + out := make([]*mcp.Tool, 0, len(forReg)) + for i := range forReg { + toolCopy := forReg[i].Tool out = append(out, &toolCopy) } - if shouldStripMCPAppsMetadata(ctx, reg.checkFeatureFlag(ctx, mcpAppsFeatureFlag)) { - for _, tt := range out { - delete(tt.Meta, "ui") - if len(tt.Meta) == 0 { - tt.Meta = nil - } - } - } return out }