From e54bc7888f7c9097c9250c376ab50ced095eeaad Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 3 Jun 2026 10:58:10 +0100 Subject: [PATCH 1/5] Add explicit show_ui parameter to UI-enabled write tools Today the server decides whether to route issue_write and create_pull_request through the MCP App form using two implicit signals: _ui_submitted (set by the form on submit) and a heuristic that bypasses the form when the call carries any parameter the form cannot represent (labels, assignees, issue_fields, state, reviewers, etc.). The model had no first-class, documented way to say "execute directly, do not show a form". Add a show_ui boolean parameter to the input schema of IssueWrite, LegacyIssueWrite, and CreatePullRequest. It defaults to true and is visible only to clients that advertise MCP App UI support: the strip happens per-request in inventory.ToolsForRegistration via a new stripUIOnlySchemaProperties helper, gated by the same predicate that already strips _meta.ui (shouldStripMCPAppsMetadata). The two strips share one decision so the schema and metadata stay in lock-step. Form-routing predicate becomes: MCPApps FF on && client supports UI && !_ui_submitted && show_ui && !hasNonFormParams show_ui=false is a new explicit way for the model to opt out. The existing non-form-param auto-bypass stays as a safety net, and the React forms keep sending _ui_submitted=true on submit unchanged. get_me is out of scope because its UI is pure client-side card rendering with no server-side gating to replace. The current strip gate ("strip when FF is off OR capability explicitly absent") mirrors today's _meta.ui behavior exactly, including the "capability unknown" case. For stdio that means UI-capable schemas are exposed to any FF-enabled client. The handler-side clientSupportsUI check still gates form execution at call time, so it is functionally a no-op for non-UI stdio clients. A separate follow-up will tighten the gate to "strip on unknown too" and wire an InitializedHandler in stdio to re-register the un-stripped surface only after a UI-capable client has advertised; the two changes must ship together to avoid breaking stdio. docs/feature-flags.md and docs/insiders-features.md include an unrelated "reviewers" description update picked up by script/generate-docs from commit 2bd162ac ("fix: support team pull request reviewers"), which updated the source schema but did not regenerate docs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/feature-flags.md | 4 +- docs/insiders-features.md | 2 + .../__toolsnaps__/create_pull_request.snap | 4 + pkg/github/__toolsnaps__/issue_write.snap | 4 + ...ssue_write_ff_remote_mcp_issue_fields.snap | 4 + pkg/github/issues.go | 51 +++- pkg/github/issues_test.go | 82 ++++++ pkg/github/pullrequests.go | 25 +- pkg/github/pullrequests_test.go | 85 ++++++ pkg/inventory/builder.go | 76 +++++ pkg/inventory/registry.go | 13 +- pkg/inventory/registry_test.go | 267 +++++++++++++++++- 12 files changed, 583 insertions(+), 34 deletions(-) diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 0b75a61bac..4caf0cb111 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) - `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) - `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 881030f020..3cb898317b 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) - `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) - `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 a8a94ce690..5442aeda30 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 a125864f04..995bce120f 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 6fb00d2490..83896180cf 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 6e9cdae53b..f02d5e2fa9 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,15 @@ 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 / README) so the UI-capable + // surface is fully documented. + "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 +1949,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 +2166,15 @@ 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 / README) so the UI-capable + // surface is fully documented. + "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 +2196,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 d794ad1679..39a4c78f9e 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1794,6 +1794,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 +1886,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}, diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 05028850d7..f28f5d5460 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,15 @@ 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 / README) so the UI-capable + // surface is fully documented. + "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 +653,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 aff71e4c1a..f9c028d05c 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}, } diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 9ecaca1f57..9e59807d52 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,80 @@ 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 README document the full UI-capable +// surface) 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 +} + +// 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 b8a70a3420..101f8ee944 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 20b1fb718c..cca998e3be 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,250 @@ 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") +} + +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 +2450,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 } From 76b9cfbb6f9e436093de5c6cb29d19e595b35b03 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 3 Jun 2026 14:01:40 +0100 Subject: [PATCH 2/5] Clarify where show_ui appears in generated docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code comments next to the show_ui schema entries (and the uiOnlySchemaProperties allowlist) said the property is documented in "toolsnaps / README". README is generated from the stripped (non-UI) schema, so show_ui is not actually in it — it only appears in toolsnaps and the feature-flag / insiders docs. Reword the comments to match reality. Comment-only change; no behavior or test impact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/issues.go | 12 ++++++++---- pkg/github/pullrequests.go | 6 ++++-- pkg/inventory/builder.go | 5 +++-- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index f02d5e2fa9..0e048c9da5 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1922,8 +1922,10 @@ Options are: // 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 / README) so the UI-capable - // surface is fully documented. + // 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.", @@ -2169,8 +2171,10 @@ Options are: // 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 / README) so the UI-capable - // surface is fully documented. + // 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.", diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index f28f5d5460..63338ca636 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -631,8 +631,10 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo // 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 / README) so the UI-capable - // surface is fully documented. + // 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.", diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 9e59807d52..80ddab6d66 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -410,8 +410,9 @@ func stripMCPAppsMetadata(tools []ServerTool) []ServerTool { // 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 README document the full UI-capable -// surface) and are stripped per-request when the same gate that hides +// 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 From 0f2ad87373ecfd7b6e4630491d11ed6cdaae3c32 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 3 Jun 2026 14:03:46 +0100 Subject: [PATCH 3/5] Guard issue_write/create_pull_request schemas against UI-gating desync The form-routing logic depends on a hand-maintained classification of each schema property into form-resendable vs known-non-form. A new property added without updating the classification would silently shift UI gating behavior (e.g. a form-incompatible param wouldn't trigger the safety-net bypass). Add Test_issueWriteSchemaClassification and Test_createPullRequestSchemaClassification that enumerate each tool's InputSchema.Properties and require every property to be classified as exactly one of: - form-resendable (member of issueWriteFormParams / pullRequestWriteFormParams) - known-non-form (test-local allowlist) A future schema addition without classification fails the test with a message pointing at the exact set the contributor needs to update. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/issues_test.go | 51 +++++++++++++++++++++++++++++++++ pkg/github/pullrequests_test.go | 26 +++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 39a4c78f9e..ca8dbf28a1 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" @@ -1907,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_test.go b/pkg/github/pullrequests_test.go index f9c028d05c..ae8f3c66c8 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -2615,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() From 0bfed9ae042e98d976b6c1994b2b6644933f3200 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 3 Jun 2026 14:19:28 +0100 Subject: [PATCH 4/5] Mark conditional schema parameters in generated docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously `show_ui` was listed in docs/feature-flags.md and docs/insiders-features.md alongside ordinary parameters with no indication that it is hidden from clients without MCP App UI support. A reader scanning the parameter list would assume it is always available. Add a programmatic conditional-property mechanism: - `inventory.ConditionalSchemaPropertyDescriptions()` exposes a map[propertyName]conditionDescription derived from the same uiOnlySchemaProperties allowlist that drives the per-request strip in ToolsForRegistration. Single source of truth. - The doc generator (writeToolDoc) consults this map and appends "conditional — " to the parameter's parenthesised type/required suffix. Example rendered output: - `show_ui`: Whether to render the MCP App form... (boolean, optional, conditional — only visible to clients that advertise MCP App UI support) A small test (TestConditionalSchemaPropertyDescriptions) ensures every entry in uiOnlySchemaProperties has a description, so a future stripped property addition can't silently lose its doc marker. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/github-mcp-server/generate_docs.go | 8 +++++++- docs/feature-flags.md | 4 ++-- docs/insiders-features.md | 4 ++-- pkg/inventory/builder.go | 16 ++++++++++++++++ pkg/inventory/registry_test.go | 17 +++++++++++++++++ 5 files changed, 44 insertions(+), 5 deletions(-) diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index 78ed8361a8..74977c1a37 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 4caf0cb111..4e7e0f40d3 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -44,7 +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) + - `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 @@ -67,7 +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) + - `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/docs/insiders-features.md b/docs/insiders-features.md index 3cb898317b..dc4037f0a3 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -38,7 +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) + - `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 @@ -61,7 +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) + - `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/inventory/builder.go b/pkg/inventory/builder.go index 80ddab6d66..68d4a692bb 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -418,6 +418,22 @@ 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 = "only visible to clients that advertise MCP App UI support" + 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) diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index cca998e3be..bcdd70f000 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -2182,6 +2182,23 @@ func TestStripUIOnlySchemaProperties(t *testing.T) { "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 From 5d2bc01efea78e81e27c74cdcd89c8af9f0ec90f Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 3 Jun 2026 14:29:15 +0100 Subject: [PATCH 5/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- pkg/inventory/builder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 68d4a692bb..c8a9c21bc9 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -426,7 +426,7 @@ var uiOnlySchemaProperties = []string{ // surface — entries here must correspond to a strip rule in // ToolsForRegistration. func ConditionalSchemaPropertyDescriptions() map[string]string { - const uiOnlyCondition = "only visible to clients that advertise MCP App UI support" + 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