From 3094759f35ea304272e9c55adf611c7f7388883f Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 1 Jun 2026 16:42:03 +0100 Subject: [PATCH 1/2] Skip MCP App form when issue/PR write carries non-form params MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When MCP Apps are enabled and the client supports UI, issue_write and create_pull_request route the call to an interactive form. The form only collects a subset of fields and rebuilds the submit payload from scratch, so any parameter it cannot represent was silently dropped — e.g. labels, assignees, milestone, type, state and issue_fields (priority) for issue_write. Skip the form and execute directly whenever the call carries a parameter outside the set the form collects and re-sends. This generalizes the previous state-only guard and is robust to future parameter additions (an unrecognized param now bypasses the form rather than being lost). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/issues.go | 82 +++++++++++++-------- pkg/github/issues_test.go | 123 ++++++++++++++++++++++++++++++++ pkg/github/pullrequests.go | 41 +++++++++-- pkg/github/pullrequests_test.go | 43 +++++++++++ 4 files changed, 255 insertions(+), 34 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 046978981..6e9cdae53 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1763,6 +1763,36 @@ func searchIssuesHandler(ctx context.Context, deps ToolDependencies, args map[st // IssueWriteUIResourceURI is the URI for the issue_write tool's MCP App UI resource. const IssueWriteUIResourceURI = "ui://github-mcp-server/issue-write" +// issueWriteFormParams are the parameters the issue_write MCP App form collects +// and re-sends on submit. The form only supports title/body editing (plus the +// routing/identity fields), so any other parameter present on a call cannot be +// represented by the form. +var issueWriteFormParams = map[string]struct{}{ + "method": {}, + "owner": {}, + "repo": {}, + "title": {}, + "body": {}, + "issue_number": {}, + "_ui_submitted": {}, +} + +// issueWriteHasNonFormParams reports whether the call carries any parameter the +// issue_write MCP App form cannot represent (anything outside issueWriteFormParams, +// e.g. labels, assignees, issue_fields or a state change). Such calls must bypass +// the UI form and execute directly so the supplied values aren'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 +} + // IssueWrite is the FeatureFlagIssueFields-enabled variant of issue_write // (with the issue_fields parameter). LegacyIssueWrite is served when the flag // is off. Both register under the tool name "issue_write"; exactly one is @@ -1908,26 +1938,22 @@ Options are: return utils.NewToolResultError(err.Error()), nil, nil } - // When MCP Apps are enabled and the client supports UI, - // check if this is a UI form submission. The UI sends _ui_submitted=true - // to distinguish form submissions from LLM calls. + // 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 { + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !issueWriteHasNonFormParams(args) { if method == "update" { - // Skip the UI form when a state change is requested because - // the form only handles title/body editing and would lose the - // state transition (e.g. closing or reopening the issue). - if _, hasState := args["state"]; !hasState { - issueNumber, numErr := RequiredInt(args, "issue_number") - if numErr != nil { - return utils.NewToolResultError("issue_number is required for update method"), nil, nil - } - return utils.NewToolResultText(fmt.Sprintf("Ready to update issue #%d in %s/%s. IMPORTANT: The issue has NOT been updated yet. Do NOT tell the user the issue was updated. The user MUST click Submit in the form to update it.", issueNumber, owner, repo)), nil, nil + issueNumber, numErr := RequiredInt(args, "issue_number") + if numErr != nil { + return utils.NewToolResultError("issue_number is required for update method"), nil, nil } - } else { - return utils.NewToolResultText(fmt.Sprintf("Ready to create an issue in %s/%s. IMPORTANT: The issue has NOT been created yet. Do NOT tell the user the issue was created. The user MUST click Submit in the form to create it.", owner, repo)), nil, nil + return utils.NewToolResultText(fmt.Sprintf("Ready to update issue #%d in %s/%s. IMPORTANT: The issue has NOT been updated yet. Do NOT tell the user the issue was updated. The user MUST click Submit in the form to update it.", issueNumber, owner, repo)), nil, nil } + return utils.NewToolResultText(fmt.Sprintf("Ready to create an issue in %s/%s. IMPORTANT: The issue has NOT been created yet. Do NOT tell the user the issue was created. The user MUST click Submit in the form to create it.", owner, repo)), nil, nil } title, err := OptionalParam[string](args, "title") @@ -2144,26 +2170,22 @@ Options are: return utils.NewToolResultError(err.Error()), nil, nil } - // When MCP Apps are enabled and the client supports UI, - // check if this is a UI form submission. The UI sends _ui_submitted=true - // to distinguish form submissions from LLM calls. + // 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 { + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !issueWriteHasNonFormParams(args) { if method == "update" { - // Skip the UI form when a state change is requested because - // the form only handles title/body editing and would lose the - // state transition (e.g. closing or reopening the issue). - if _, hasState := args["state"]; !hasState { - issueNumber, numErr := RequiredInt(args, "issue_number") - if numErr != nil { - return utils.NewToolResultError("issue_number is required for update method"), nil, nil - } - return utils.NewToolResultText(fmt.Sprintf("Ready to update issue #%d in %s/%s. IMPORTANT: The issue has NOT been updated yet. Do NOT tell the user the issue was updated. The user MUST click Submit in the form to update it.", issueNumber, owner, repo)), nil, nil + issueNumber, numErr := RequiredInt(args, "issue_number") + if numErr != nil { + return utils.NewToolResultError("issue_number is required for update method"), nil, nil } - } else { - return utils.NewToolResultText(fmt.Sprintf("Ready to create an issue in %s/%s. IMPORTANT: The issue has NOT been created yet. Do NOT tell the user the issue was created. The user MUST click Submit in the form to create it.", owner, repo)), nil, nil + return utils.NewToolResultText(fmt.Sprintf("Ready to update issue #%d in %s/%s. IMPORTANT: The issue has NOT been updated yet. Do NOT tell the user the issue was updated. The user MUST click Submit in the form to update it.", issueNumber, owner, repo)), nil, nil } + return utils.NewToolResultText(fmt.Sprintf("Ready to create an issue in %s/%s. IMPORTANT: The issue has NOT been created yet. Do NOT tell the user the issue was created. The user MUST click Submit in the form to create it.", owner, repo)), nil, nil } title, err := OptionalParam[string](args, "title") diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index b04370976..d794ad167 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1700,6 +1700,129 @@ func Test_IssueWrite_MCPAppsFeature_UIGate(t *testing.T) { assert.Contains(t, textContent.Text, "Ready to update issue #1", "update without state should show UI form") }) + + t.Run("UI client with issue_fields skips form and executes directly", func(t *testing.T) { + // The MCP App form does not collect or re-send issue_fields, so a call + // carrying them must bypass the form and apply the values directly. + fieldsClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesByOwnerByRepo: expectRequestBody(t, map[string]any{ + "title": "Issue with fields", + "body": "", + "labels": []any{}, + "assignees": []any{}, + "issue_field_values": []any{ + map[string]any{"field_id": float64(101), "value": "P1"}, + }, + }).andThen( + mockResponse(t, http.StatusCreated, &github.Issue{ + Number: github.Ptr(125), + Title: github.Ptr("Issue with fields"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/125"), + State: github.Ptr("open"), + }), + ), + })) + fieldsGQLClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + issueFieldWriteMetadataQuery{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issueFields": map[string]any{ + "nodes": []any{ + map[string]any{ + "__typename": "IssueFieldSingleSelect", + "fullDatabaseId": "101", + "name": "Priority", + "dataType": "single_select", + "options": []any{ + map[string]any{"fullDatabaseId": "9001", "name": "P1"}, + }, + }, + }, + }, + }, + }), + ), + )) + + fieldsDeps := BaseDeps{ + Client: fieldsClient, + GQLClient: fieldsGQLClient, + featureChecker: featureCheckerFor(MCPAppsFeatureFlag), + } + fieldsHandler := serverTool.Handler(fieldsDeps) + + request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Issue with fields", + "issue_fields": []any{ + map[string]any{"field_name": "Priority", "field_option_name": "P1"}, + }, + }) + result, err := fieldsHandler(ContextWithDeps(context.Background(), fieldsDeps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + assert.NotContains(t, textContent.Text, "Ready to create an issue", + "issue_fields should skip UI form") + assert.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/125", + "issue_fields call should execute directly and return issue URL") + }) + + t.Run("UI client with labels skips form and executes directly", func(t *testing.T) { + // The form does not collect labels, so a call carrying them must bypass + // the form rather than silently drop them. + request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Test", + "labels": []any{"bug"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + assert.NotContains(t, textContent.Text, "Ready to create an issue", + "labels should skip UI form") + assert.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/1", + "labels call should execute directly and return issue URL") + }) +} + +func Test_issueWriteHasNonFormParams(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args map[string]any + want bool + }{ + {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: "labels present", args: map[string]any{"title": "t", "labels": []any{"bug"}}, want: true}, + {name: "assignees present", args: map[string]any{"title": "t", "assignees": []any{"octocat"}}, want: true}, + {name: "milestone present", args: map[string]any{"title": "t", "milestone": float64(2)}, want: true}, + {name: "type present", args: map[string]any{"title": "t", "type": "Bug"}, want: true}, + {name: "issue_fields present", args: map[string]any{"issue_fields": []any{map[string]any{"field_name": "Priority"}}}, want: true}, + {name: "state present", args: map[string]any{"state": "closed"}, want: true}, + {name: "state_reason present", args: map[string]any{"state_reason": "completed"}, want: true}, + {name: "duplicate_of present", args: map[string]any{"duplicate_of": float64(7)}, want: true}, + {name: "nil value is ignored", args: map[string]any{"issue_fields": nil}, want: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, issueWriteHasNonFormParams(tc.args)) + }) + } } func Test_ListIssues(t *testing.T) { diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 7f1751b97..05028850d 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -544,6 +544,37 @@ func GetPullRequestReviews(ctx context.Context, client *github.Client, deps Tool // PullRequestWriteUIResourceURI is the URI for the create_pull_request tool's MCP App UI resource. const PullRequestWriteUIResourceURI = "ui://github-mcp-server/pr-write" +// pullRequestWriteFormParams are the parameters the create_pull_request MCP App +// form collects and re-sends on submit. Any other parameter present on a call +// cannot be represented by the form. +var pullRequestWriteFormParams = map[string]struct{}{ + "owner": {}, + "repo": {}, + "title": {}, + "body": {}, + "head": {}, + "base": {}, + "draft": {}, + "maintainer_can_modify": {}, + "_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 +} + // CreatePullRequest creates a tool to create a new pull request. func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool { return NewTool( @@ -611,12 +642,14 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo return utils.NewToolResultError(err.Error()), nil, nil } - // When MCP Apps are enabled and the client supports UI, - // check if this is a UI form submission. The UI sends _ui_submitted=true - // to distinguish form submissions from LLM calls. + // 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 { + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !pullRequestWriteHasNonFormParams(args) { return utils.NewToolResultText(fmt.Sprintf("Ready to create a pull request in %s/%s. IMPORTANT: The PR has NOT been created yet. Do NOT tell the user the PR was created. The user MUST click Submit in the form to create it.", owner, repo)), nil, nil } diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 0faee23e2..aff71e4c1 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -2485,6 +2485,49 @@ func Test_CreatePullRequest_MCPAppsFeature_UIGate(t *testing.T) { assert.Contains(t, textContent.Text, "https://github.com/owner/repo/pull/42", "non-UI client should execute directly") }) + + t.Run("UI client with non-form param skips form and executes directly", func(t *testing.T) { + // A parameter the form does not collect must bypass the form rather than + // be silently dropped. + request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{ + "owner": "owner", + "repo": "repo", + "title": "Test PR", + "head": "feature", + "base": "main", + "reviewers": []any{"octocat"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + assert.NotContains(t, textContent.Text, "Ready to create a pull request", + "non-form param should skip UI form") + assert.Contains(t, textContent.Text, "https://github.com/owner/repo/pull/42", + "non-form param call should execute directly and return PR URL") + }) +} + +func Test_pullRequestWriteHasNonFormParams(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args map[string]any + 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, "_ui_submitted": true}, want: false}, + {name: "unknown param present", args: map[string]any{"title": "t", "reviewers": []any{"octocat"}}, want: true}, + {name: "nil value is ignored", args: map[string]any{"reviewers": nil}, want: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, pullRequestWriteHasNonFormParams(tc.args)) + }) + } } func TestCreateAndSubmitPullRequestReview(t *testing.T) { From 95b05a86c3e4a51ebce9841b7e6b0479a81f853b Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 1 Jun 2026 16:47:06 +0100 Subject: [PATCH 2/2] Forward original tool params on MCP App form submit The issue-write and pr-write forms rebuilt their submit payload from scratch, so any parameter the form does not render was dropped on submit. Spread the original toolInput first and override only the edited fields, so unsupported params (e.g. issue_fields, labels, state) are preserved when the user submits the form. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ui/src/apps/issue-write/App.tsx | 3 ++- ui/src/apps/pr-write/App.tsx | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/ui/src/apps/issue-write/App.tsx b/ui/src/apps/issue-write/App.tsx index 863543fc1..fedb7f24f 100644 --- a/ui/src/apps/issue-write/App.tsx +++ b/ui/src/apps/issue-write/App.tsx @@ -152,6 +152,7 @@ function CreateIssueApp() { try { const params: Record = { + ...(toolInput as Record | undefined), method: isUpdateMode ? "update" : "create", owner, repo, @@ -204,7 +205,7 @@ function CreateIssueApp() { } finally { setIsSubmitting(false); } - }, [title, body, owner, repo, isUpdateMode, issueNumber, callTool, setModelContext]); + }, [title, body, owner, repo, isUpdateMode, issueNumber, toolInput, callTool, setModelContext]); const body_node = (() => { if (appError) { diff --git a/ui/src/apps/pr-write/App.tsx b/ui/src/apps/pr-write/App.tsx index bfefdbede..abbeacb12 100644 --- a/ui/src/apps/pr-write/App.tsx +++ b/ui/src/apps/pr-write/App.tsx @@ -156,6 +156,7 @@ function CreatePRApp() { try { const result = await callTool("create_pull_request", { + ...(toolInput as Record | undefined), owner, repo, title: title.trim(), body: body.trim(), @@ -193,7 +194,7 @@ function CreatePRApp() { } finally { setIsSubmitting(false); } - }, [title, body, owner, repo, head, base, isDraft, maintainerCanModify, callTool, setModelContext]); + }, [title, body, owner, repo, head, base, isDraft, maintainerCanModify, toolInput, callTool, setModelContext]); if (successPR) { return (