diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index e8c044cde..212851c50 100644 --- a/cmd/github-mcp-server/generate_docs.go +++ b/cmd/github-mcp-server/generate_docs.go @@ -265,8 +265,6 @@ 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) @@ -292,11 +290,7 @@ func writeToolDoc(buf *strings.Builder, tool inventory.ServerTool) { // Indent any continuation lines in the description to maintain markdown formatting description := indentMultilineDescription(prop.Description, " ") - 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) - } + 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 6b6ff7eaa..bd1df9ce1 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -45,7 +45,6 @@ runtime behavior (such as output formatting) won't appear here. - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) - `reviewers`: GitHub usernames or ORG/team-slug team reviewers to request reviews from (string[], 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 the user has already confirmed the action and the form would be redundant. (boolean, optional, conditional — visible when remote_mcp_ui_apps is enabled unless the client explicitly indicates it does not support io.modelcontextprotocol/ui) - `title`: PR title (string, required) - **get_me** - Get my user profile @@ -69,7 +68,6 @@ 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 the user has already confirmed the action and the form would be redundant. (boolean, optional, conditional — visible when remote_mcp_ui_apps is enabled unless the client explicitly indicates it does not support io.modelcontextprotocol/ui) - `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 373ead3b9..7bbeaddc3 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -39,7 +39,6 @@ The list below is generated from the Go source. It covers tool **inventory and s - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) - `reviewers`: GitHub usernames or ORG/team-slug team reviewers to request reviews from (string[], 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 the user has already confirmed the action and the form would be redundant. (boolean, optional, conditional — visible when remote_mcp_ui_apps is enabled unless the client explicitly indicates it does not support io.modelcontextprotocol/ui) - `title`: PR title (string, required) - **get_me** - Get my user profile @@ -63,7 +62,6 @@ 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 the user has already confirmed the action and the form would be redundant. (boolean, optional, conditional — visible when remote_mcp_ui_apps is enabled unless the client explicitly indicates it does not support io.modelcontextprotocol/ui) - `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 5de1b229b..3bd3404d4 100644 --- a/pkg/github/__toolsnaps__/create_pull_request.snap +++ b/pkg/github/__toolsnaps__/create_pull_request.snap @@ -49,10 +49,6 @@ }, "type": "array" }, - "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 the user has already confirmed the action and the form would be redundant.", - "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 9d80b14d5..da6a28688 100644 --- a/pkg/github/__toolsnaps__/issue_write.snap +++ b/pkg/github/__toolsnaps__/issue_write.snap @@ -96,10 +96,6 @@ "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 the user has already confirmed the action and the form would be redundant.", - "type": "boolean" - }, "state": { "description": "New state", "enum": [ diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 3af88f430..faa146be8 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1726,7 +1726,11 @@ const IssueWriteUIResourceURI = "ui://github-mcp-server/issue-write" // issueWriteFormParams are the parameters the issue_write MCP App form collects // and re-sends on submit. Any other parameter present on a call cannot be -// represented by the form. +// represented by the form. The form collects (and prefills) every parameter in +// the tool's current input schema, so hasNonFormParams against this set is a +// forward-compatibility safety net: a parameter added to the schema in the +// future but not yet wired into the form trips the check and bypasses the form +// so the supplied value isn't silently dropped. var issueWriteFormParams = map[string]struct{}{ "method": {}, "owner": {}, @@ -1742,28 +1746,9 @@ var issueWriteFormParams = map[string]struct{}{ "state": {}, "state_reason": {}, "duplicate_of": {}, - "show_ui": {}, "_ui_submitted": {}, } -// issueWriteHasNonFormParams reports whether the call carries any parameter the -// issue_write MCP App form cannot represent (anything outside issueWriteFormParams). -// The form collects (and prefills) every parameter in the tool's current input -// schema, so this is a forward-compatibility safety net: a parameter added to the -// schema in the future but not yet wired into the form trips this check and bypasses -// the UI so the supplied value isn't silently dropped. -func issueWriteHasNonFormParams(args map[string]any) bool { - for key, value := range args { - if value == nil { - continue - } - if _, ok := issueWriteFormParams[key]; !ok { - return true - } - } - return false -} - // issueWriteAwaitingFormResult builds the "awaiting form submission" stub // returned when issue_write hands off to the MCP App form. The body is shared // by IssueWrite and LegacyIssueWrite. The result is marked IsError=true so @@ -1919,17 +1904,6 @@ 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 the user has already confirmed the action and the form would be redundant.", - }, }, Required: []string{"method", "owner", "repo"}, }, @@ -1950,20 +1924,9 @@ Options are: return utils.NewToolResultError(err.Error()), nil, nil } - // 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), - // - 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 && showUI && !issueWriteHasNonFormParams(args) { + // Hand off to the interactive MCP App form unless this call must + // execute now (see shouldDeferToForm). + if shouldDeferToForm(ctx, deps, req, args, issueWriteFormParams) { issueNumber := 0 if method == "update" { n, numErr := RequiredInt(args, "issue_number") diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index cb432c701..e3dfe0fe1 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1674,86 +1674,6 @@ func Test_IssueWrite_MCPAppsFeature_UIGate(t *testing.T) { "labels should route through UI form") assert.True(t, result.IsError, "form-routing stub should be marked IsError so agents don't claim success") }) - - 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, "interactive form has been shown", - "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, "interactive form has been shown", - "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) { @@ -1766,8 +1686,6 @@ 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: false}, {name: "assignees present", args: map[string]any{"title": "t", "assignees": []any{"octocat"}}, want: false}, {name: "milestone present", args: map[string]any{"title": "t", "milestone": float64(2)}, want: false}, @@ -1783,7 +1701,7 @@ func Test_issueWriteHasNonFormParams(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - assert.Equal(t, tc.want, issueWriteHasNonFormParams(tc.args)) + assert.Equal(t, tc.want, hasNonFormParams(tc.args, issueWriteFormParams)) }) } } @@ -1797,7 +1715,7 @@ 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. The + // must trigger the safety-net bypass via hasNonFormParams. The // form currently collects every schema property, so this allowlist is // empty; add a property here only if it is added to the schema without // corresponding form support. diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index bf0eaa29b..9e2d36896 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -603,7 +603,6 @@ var pullRequestWriteFormParams = map[string]struct{}{ "draft": {}, "maintainer_can_modify": {}, "reviewers": {}, - "show_ui": {}, "_ui_submitted": {}, } @@ -621,34 +620,6 @@ var pullRequestUpdateFormParams = map[string]struct{}{ "_ui_submitted": {}, } -// pullRequestWriteHasNonFormParams reports whether the call carries any parameter -// the create_pull_request MCP App form cannot represent (anything outside -// pullRequestWriteFormParams). Such calls must bypass the UI form and execute -// directly so the supplied values aren't silently dropped. -func pullRequestWriteHasNonFormParams(args map[string]any) bool { - for key, value := range args { - if value == nil { - continue - } - if _, ok := pullRequestWriteFormParams[key]; !ok { - return true - } - } - return false -} - -func pullRequestUpdateHasNonFormParams(args map[string]any) bool { - for key, value := range args { - if value == nil { - continue - } - if _, ok := pullRequestUpdateFormParams[key]; !ok { - return true - } - } - return false -} - // CreatePullRequest creates a tool to create a new pull request. func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool { return NewTool( @@ -708,17 +679,6 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo Type: "string", }, }, - // 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 the user has already confirmed the action and the form would be redundant.", - }, }, Required: []string{"owner", "repo", "title", "head", "base"}, }, @@ -734,19 +694,9 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo return utils.NewToolResultError(err.Error()), nil, nil } - // 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), - // - 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 && showUI && !pullRequestWriteHasNonFormParams(args) { + // Hand off to the interactive MCP App form unless this call must + // execute now (see shouldDeferToForm). + if shouldDeferToForm(ctx, deps, req, args, pullRequestWriteFormParams) { return utils.NewToolResultAwaitingFormSubmission(fmt.Sprintf( "An interactive form has been shown to the user for creating a new pull request in %s/%s. "+ "STOP — do not call any other tools, do not respond as if the pull request was created, "+ @@ -965,8 +915,9 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo return utils.NewToolResultError(err.Error()), nil, nil } - uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted") - if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !pullRequestUpdateHasNonFormParams(args) { + // Hand off to the interactive MCP App form unless this call must + // execute now (see shouldDeferToForm). + if shouldDeferToForm(ctx, deps, req, args, pullRequestUpdateFormParams) { return utils.NewToolResultAwaitingFormSubmission(fmt.Sprintf( "An interactive form has been shown to the user for editing pull request #%d in %s/%s. "+ "STOP — do not call any other tools, do not respond as if the pull request was updated, "+ diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 0f372519e..92691fb64 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -2686,89 +2686,6 @@ 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, "interactive form has been shown", - "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, "interactive form has been shown", - "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") - }) } // Test_UpdatePullRequest_MCPAppsFeature_UIGate verifies the form-routing @@ -2876,9 +2793,7 @@ func Test_pullRequestWriteHasNonFormParams(t *testing.T) { want bool }{ {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, "reviewers": []any{"octocat"}, "show_ui": true, "_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: "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, "reviewers": []any{"octocat"}, "_ui_submitted": true}, want: false}, {name: "unknown param present", args: map[string]any{"title": "t", "unknown_param": "value"}, want: true}, {name: "nil value is ignored", args: map[string]any{"reviewers": nil}, want: false}, } @@ -2886,7 +2801,7 @@ func Test_pullRequestWriteHasNonFormParams(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - assert.Equal(t, tc.want, pullRequestWriteHasNonFormParams(tc.args)) + assert.Equal(t, tc.want, hasNonFormParams(tc.args, pullRequestWriteFormParams)) }) } } diff --git a/pkg/github/ui_capability.go b/pkg/github/ui_capability.go index f237df842..a850db0c9 100644 --- a/pkg/github/ui_capability.go +++ b/pkg/github/ui_capability.go @@ -33,3 +33,44 @@ func clientSupportsUI(ctx context.Context, req *mcp.CallToolRequest) bool { } return false } + +// uiSubmitted reports whether the call is itself an MCP App form submission. +// The form re-invokes its tool with _ui_submitted=true; such calls must execute +// rather than re-render the form. +func uiSubmitted(args map[string]any) bool { + submitted, _ := OptionalParam[bool](args, "_ui_submitted") + return submitted +} + +// hasNonFormParams reports whether the call carries any parameter the tool's MCP +// App form cannot represent (anything outside formParams). Such calls must +// bypass the form and execute directly so the supplied values aren't silently +// dropped. formParams is the set of parameters the form collects and re-sends +// on submit. +func hasNonFormParams(args map[string]any, formParams map[string]struct{}) bool { + for key, value := range args { + if value == nil { + continue + } + if _, ok := formParams[key]; !ok { + return true + } + } + return false +} + +// shouldDeferToForm is the single source of truth for the show/defer decision +// shared by the form-backed write tools (create_pull_request, +// update_pull_request, issue_write). It reports whether a call should be handed +// off to its MCP App form instead of executing now: defer only when MCP Apps +// are enabled, the client can render UI, the call is not itself a form +// submission, and every supplied parameter can be represented by the form +// (formParams is the tool's form-parameter allowlist). When it returns false +// the handler executes directly; the host may still render the tool's view, +// which renders the result rather than an input form. +func shouldDeferToForm(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args map[string]any, formParams map[string]struct{}) bool { + return deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && + clientSupportsUI(ctx, req) && + !uiSubmitted(args) && + !hasNonFormParams(args, formParams) +} diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index c8a9c21bc..9ecaca1f5 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -7,8 +7,6 @@ import ( "maps" "slices" "strings" - - "github.com/google/jsonschema-go/jsonschema" ) var ( @@ -408,97 +406,6 @@ 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 101f8ee94..5f4b37ef2 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -169,9 +169,8 @@ 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 and -// UI-capability-gated input-schema properties (e.g. show_ui) removed when -// the client cannot consume them. Useful for documentation generators and +// RegisterTools would expose them: with MCP Apps UI metadata stripped when +// the client cannot consume it. Useful for documentation generators and // diagnostics that need the same view of the tool surface the server would // register. // @@ -187,7 +186,6 @@ 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 } @@ -208,8 +206,7 @@ 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`) and UI-capability-gated input-schema -// properties (e.g. `show_ui`) are stripped from the registered tools when +// 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 // strip happens here (rather than at Build() time) so the per-request diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index bcdd70f00..3200bedde 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -4,12 +4,9 @@ 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" ) @@ -2022,267 +2019,6 @@ 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"} diff --git a/ui/src/apps/issue-write/App.tsx b/ui/src/apps/issue-write/App.tsx index 6372e2d50..95a549f28 100644 --- a/ui/src/apps/issue-write/App.tsx +++ b/ui/src/apps/issue-write/App.tsx @@ -24,6 +24,7 @@ import { } from "@primer/octicons-react"; import { AppProvider } from "../../components/AppProvider"; import { useMcpApp } from "../../hooks/useMcpApp"; +import { completedToolResult } from "../../lib/toolResult"; import { MarkdownEditor } from "../../components/MarkdownEditor"; interface IssueResult { @@ -435,10 +436,21 @@ function CreateIssueApp() { const [repoSearchLoading, setRepoSearchLoading] = useState(false); const [repoFilter, setRepoFilter] = useState(""); - const { app, error: appError, toolInput, callTool, hostContext, setModelContext, openLink } = useMcpApp({ + const { app, error: appError, toolInput, toolResult, callTool, hostContext, setModelContext, openLink } = useMcpApp({ appName: "github-mcp-server-issue-write", }); + // When the server created/updated the issue up-front instead of deferring to + // this form (e.g. the agent passed show_ui=false or parameters the form can't + // represent, such as labels/assignees/issue_fields), the host still renders + // this View and delivers the result via tool-result. Render that completed + // result as success so we never show a create/update form for an issue that + // is already done. The deferral sentinel (awaiting_user_submission) returns + // null here, keeping the form for the normal deferred flow. + // See github/copilot-mcp-core#1864. + const resultIssue = useMemo(() => completedToolResult(toolResult), [toolResult]); + const shownIssue = successIssue ?? resultIssue; + // Get method and issue_number from toolInput const method = (toolInput?.method as string) || "create"; const issueNumber = toolInput?.issue_number as number | undefined; @@ -1210,10 +1222,10 @@ function CreateIssueApp() { ); } - if (successIssue) { + if (shownIssue) { return ( completedToolResult(toolResult), [toolResult]); + const shownPR = successPR ?? resultPR; + useEffect(() => { setTitle(""); setBody(""); @@ -495,10 +505,10 @@ function EditPRApp() { } }, [title, body, owner, repo, pullNumber, baseBranch, initialValues, selectedReviewers, prState, isDraft, maintainerCanModify, callTool, setModelContext]); - if (successPR) { + if (shownPR) { return ( - + ); } diff --git a/ui/src/apps/pr-write/App.tsx b/ui/src/apps/pr-write/App.tsx index 769523d41..6975434be 100644 --- a/ui/src/apps/pr-write/App.tsx +++ b/ui/src/apps/pr-write/App.tsx @@ -27,6 +27,7 @@ import { } from "@primer/octicons-react"; import { AppProvider } from "../../components/AppProvider"; import { useMcpApp } from "../../hooks/useMcpApp"; +import { completedToolResult } from "../../lib/toolResult"; import { MarkdownEditor } from "../../components/MarkdownEditor"; interface PRResult { @@ -189,7 +190,7 @@ function CreatePRApp() { const [repoSearchLoading, setRepoSearchLoading] = useState(false); const [repoFilter, setRepoFilter] = useState(""); - const { app, error: appError, toolInput, callTool, hostContext, setModelContext, openLink } = useMcpApp({ + const { app, error: appError, toolInput, toolResult, callTool, hostContext, setModelContext, openLink } = useMcpApp({ appName: "github-mcp-server-create-pull-request", }); @@ -197,6 +198,16 @@ function CreatePRApp() { const repo = selectedRepo?.name || (toolInput?.repo as string) || ""; const [submittedTitle, setSubmittedTitle] = useState(""); + // When the server executed up-front instead of deferring to this form (e.g. + // the agent passed show_ui=false or parameters the form can't represent), the + // host still renders this View and delivers the created PR via tool-result. + // Treat that completed result as a success so we never show a "Create pull + // request" form for a PR that already exists. The deferral sentinel + // (awaiting_user_submission) returns null here, keeping the form for the + // normal deferred flow. See github/copilot-mcp-core#1864. + const resultPR = useMemo(() => completedToolResult(toolResult), [toolResult]); + const shownPR = successPR ?? resultPR; + // Reset all transient form/result state when toolInput changes (new invocation). // Without this, the SuccessView from a previous submit stays visible and stale // form values bleed through because the prefill effect below only sets when @@ -440,10 +451,10 @@ function CreatePRApp() { } }, [title, body, owner, repo, baseBranch, headBranch, isDraft, maintainerCanModify, selectedReviewers, toolInput, callTool, setModelContext]); - if (successPR) { + if (shownPR) { return ( - + ); } diff --git a/ui/src/hooks/useMcpApp.ts b/ui/src/hooks/useMcpApp.ts index cf386520f..e4146a234 100644 --- a/ui/src/hooks/useMcpApp.ts +++ b/ui/src/hooks/useMcpApp.ts @@ -69,6 +69,13 @@ export function useMcpApp({ app.ontoolinput = async (input) => { const args = (input.arguments ?? {}) as Record; setToolInput(args); + // A tool-input notification marks a new invocation, and the spec + // guarantees it is delivered before that invocation's tool-result. + // Drop any prior result so a completed result from a previous + // invocation can't leak into the new render (e.g. a stale success card + // showing over a fresh, still-deferred form). The current invocation's + // result, if any, arrives next via ontoolresult. + setToolResult(null); onToolInput?.(args); }; app.onhostcontextchanged = (params) => { diff --git a/ui/src/lib/toolResult.ts b/ui/src/lib/toolResult.ts new file mode 100644 index 000000000..e985537f4 --- /dev/null +++ b/ui/src/lib/toolResult.ts @@ -0,0 +1,37 @@ +import type { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; + +/** + * Returns the parsed JSON payload of a *completed* tool result, or `null` when + * the result is absent, an error, or the deferral sentinel (structured + * `status` of `"awaiting_user_submission"`, set by the server's + * `NewToolResultAwaitingFormSubmission`). + * + * Form-backed write Views (create_pull_request, issue_write, + * update_pull_request) use this to tell apart "the server deferred and is + * waiting for my form submission" from "the server already executed". Without + * it, a View renders its input form off in-app state alone and will show e.g. a + * "Create pull request" form for a PR that was already created up-front (when + * the agent passed `show_ui=false` or parameters the form can't represent). + * + * See github/copilot-mcp-core#1864 for the full show/defer state machine. + */ +export function completedToolResult>( + result: CallToolResult | null, +): T | null { + if (!result || result.isError) return null; + + const status = (result.structuredContent as { status?: string } | undefined) + ?.status; + if (status === "awaiting_user_submission") return null; + + const textContent = result.content?.find((c) => c.type === "text"); + if (!textContent || textContent.type !== "text" || !textContent.text) { + return null; + } + + try { + return JSON.parse(textContent.text) as T; + } catch { + return null; + } +}