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) { 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 (