From d0ecf750d777f41a5c56c3c398fe0ff37e337f2d Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Fri, 26 Jun 2026 11:25:44 +0200 Subject: [PATCH 1/4] fix(ui): render success view when an MCP App tool executed up-front MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The create_pull_request / issue_write / update_pull_request Views decided form-vs-success from in-app submit state only, ignoring the tool-result the host delivers on render. Per the MCP Apps 2026-01-26 spec the host renders a View whenever the tool carries _meta.ui.resourceUri — independent of whether the server deferred or executed. So when the server executed up-front (e.g. show_ui=false, or parameters the form can't represent) the View still showed its "Create pull request" input form over an already-created PR, which reads as a bug (it even shows a PR number). Drive the Views off the result instead: a new shared completedToolResult() helper returns parsed data only for a genuine completed success, and returns null for the awaiting_user_submission deferral sentinel, errors, or no result. Each write View now shows its success card when that completed result is present, so the form is only ever shown while the action is genuinely deferred. Reconciles the show/defer state machine at the View (decision layer that the host result feeds). See github/copilot-mcp-core#1864. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ui/src/apps/issue-write/App.tsx | 18 +++++++++++++--- ui/src/apps/pr-edit/App.tsx | 16 +++++++++++--- ui/src/apps/pr-write/App.tsx | 17 ++++++++++++--- ui/src/lib/toolResult.ts | 37 +++++++++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 9 deletions(-) create mode 100644 ui/src/lib/toolResult.ts diff --git a/ui/src/apps/issue-write/App.tsx b/ui/src/apps/issue-write/App.tsx index 6372e2d503..95a549f28b 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 769523d41b..6975434bea 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/lib/toolResult.ts b/ui/src/lib/toolResult.ts new file mode 100644 index 0000000000..e985537f49 --- /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; + } +} From 09a7a78d4f0d05f4b512ea176df41bcd4c0850f5 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Fri, 26 Jun 2026 11:36:57 +0200 Subject: [PATCH 2/4] fix(ui): scope tool-result to the current invocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback: the write Views derive their success card from `toolResult`, but it wasn't cleared when a new invocation arrived (only the in-app `successPR`/`successIssue` was reset on `toolInput` change). A completed result from a previous invocation could briefly render a stale success card over the next, still-deferred form. Clear `toolResult` whenever a new `tool-input` notification arrives. The spec guarantees `tool-input` precedes that invocation's `tool-result`, so this scopes the result to the current invocation centrally in the hook — fixing all three Views without per-app invocation keys. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ui/src/hooks/useMcpApp.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ui/src/hooks/useMcpApp.ts b/ui/src/hooks/useMcpApp.ts index cf386520f0..e4146a234f 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) => { From e2207ac23b8015adfc2f1e8092df3607727a857e Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Fri, 26 Jun 2026 15:11:00 +0200 Subject: [PATCH 3/4] =?UTF-8?q?refactor(mcp-apps):=20remove=20show=5Fui=20?= =?UTF-8?q?=E2=80=94=20it=20can't=20suppress=20app=20rendering?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit show_ui promised "skip the form and execute directly", but it can't deliver: the host renders an MCP App for any tool that carries _meta.ui.resourceUri, and the 2026-01-26 MCP Apps spec has no per-call/per-result way to opt out of rendering. show_ui only flipped the server's defer decision, so show_ui=false created the PR/issue up-front yet the host still rendered the app — exactly the contradiction this work set out to fix. And show_ui is only ever exposed to clients that support UI, i.e. precisely the clients that always render the app. Remove it entirely: - Drop the show_ui schema property, the form-param allowlist entry, and the showUI term from the defer predicate in create_pull_request and issue_write. The gate is now FF && clientSupportsUI && !_ui_submitted && !hasNonFormParams. - Delete the now-unused UI-only schema-property strip machinery in pkg/inventory (uiOnlySchemaProperties, stripUIOnlySchemaProperties, stripSchemaProperties) and the exported ConditionalSchemaPropertyDescriptions, which existed solely to surface show_ui to UI-capable clients. _meta.ui stripping is untouched. - Drop the conditional-property annotation from the docs generator. - Update toolsnaps, generated docs, and tests. With the up-front-execution Views now rendering the result (success card), the remaining contract is simple: when MCP Apps are enabled the form is the path, and the form is only shown while the action is genuinely deferred. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/github-mcp-server/generate_docs.go | 8 +- docs/feature-flags.md | 2 - docs/insiders-features.md | 2 - .../__toolsnaps__/create_pull_request.snap | 4 - pkg/github/__toolsnaps__/issue_write.snap | 4 - pkg/github/issues.go | 21 +- pkg/github/issues_test.go | 82 ------ pkg/github/pullrequests.go | 21 +- pkg/github/pullrequests_test.go | 87 +----- pkg/inventory/builder.go | 93 ------ pkg/inventory/registry.go | 9 +- pkg/inventory/registry_test.go | 264 ------------------ 12 files changed, 9 insertions(+), 588 deletions(-) diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index e8c044cde1..212851c50d 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 6b6ff7eaa4..bd1df9ce1b 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 373ead3b92..7bbeaddc3a 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 5de1b229b9..3bd3404d49 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 9d80b14d5a..da6a286887 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 3af88f4306..f9aecaa6ee 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1742,7 +1742,6 @@ var issueWriteFormParams = map[string]struct{}{ "state": {}, "state_reason": {}, "duplicate_of": {}, - "show_ui": {}, "_ui_submitted": {}, } @@ -1919,17 +1918,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"}, }, @@ -1952,18 +1940,13 @@ 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), - // - the caller explicitly asked to skip the UI (show_ui=false), or + // - 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. 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) { + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !issueWriteHasNonFormParams(args) { 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 cb432c7016..59e47bad77 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}, diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index bf0eaa29b6..240d0b2fd6 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": {}, } @@ -708,17 +707,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"}, }, @@ -736,17 +724,12 @@ 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), - // - the caller explicitly asked to skip the UI (show_ui=false), or + // - 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. 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) { + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !pullRequestWriteHasNonFormParams(args) { 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, "+ diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 0f372519e5..80308e6842 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}, } diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index c8a9c21bc9..9ecaca1f57 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 101f8ee944..5f4b37ef28 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 bcdd70f000..3200bedde6 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"} From 8b2363d31eba31600f3bfee6ad76d2af8a9f755e Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Fri, 26 Jun 2026 15:20:00 +0200 Subject: [PATCH 4/4] refactor(mcp-apps): centralize the show/defer decision (single source of truth) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The defer-to-form predicate was triplicated across create_pull_request, update_pull_request, and issue_write, each with its own near-identical *HasNonFormParams function. As more MCP App tools are added this duplication would grow and the copies could silently drift. Extract one shared gate in ui_capability.go: - shouldDeferToForm(ctx, deps, req, args, formParams) — the single show/defer decision (MCP Apps enabled, client supports UI, not a form submission, and no non-form params). - hasNonFormParams(args, formParams) — one generic helper replacing the three per-tool functions. - uiSubmitted(args) — small shared predicate. Each handler is now a one-line `if shouldDeferToForm(...) { return awaiting }`. The per-tool form-parameter allowlists and the user-facing messages stay per-tool (that is the genuine per-tool config). Pure refactor — behavior unchanged; existing tests now exercise the generic helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/issues.go | 36 ++++++--------------------- pkg/github/issues_test.go | 4 +-- pkg/github/pullrequests.go | 44 +++++---------------------------- pkg/github/pullrequests_test.go | 2 +- pkg/github/ui_capability.go | 41 ++++++++++++++++++++++++++++++ 5 files changed, 58 insertions(+), 69 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index f9aecaa6ee..faa146be84 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": {}, @@ -1745,24 +1749,6 @@ var issueWriteFormParams = map[string]struct{}{ "_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 @@ -1938,15 +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), 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") - - if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !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 59e47bad77..e3dfe0fe1d 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1701,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)) }) } } @@ -1715,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 240d0b2fd6..9e2d368962 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -620,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( @@ -722,14 +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), 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") - - if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !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, "+ @@ -948,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 80308e6842..92691fb641 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -2801,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 f237df8424..a850db0c95 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) +}