Skip to content

Commit 04c8dcb

Browse files
mattdhollowayCopilot
mattdholloway
and
Copilot
authored
Skip MCP App form when issue/PR write carries non-form params (#2589)
* Skip MCP App form when issue/PR write carries non-form params 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> * 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> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2bd162a commit 04c8dcb

6 files changed

Lines changed: 259 additions & 36 deletions

File tree

‎pkg/github/issues.go‎

Lines changed: 52 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1763,6 +1763,36 @@ func searchIssuesHandler(ctx context.Context, deps ToolDependencies, args map[st
17631763
// IssueWriteUIResourceURI is the URI for the issue_write tool's MCP App UI resource.
17641764
const IssueWriteUIResourceURI = "ui://github-mcp-server/issue-write"
17651765

1766+
// issueWriteFormParams are the parameters the issue_write MCP App form collects
1767+
// and re-sends on submit. The form only supports title/body editing (plus the
1768+
// routing/identity fields), so any other parameter present on a call cannot be
1769+
// represented by the form.
1770+
var issueWriteFormParams = map[string]struct{}{
1771+
"method": {},
1772+
"owner": {},
1773+
"repo": {},
1774+
"title": {},
1775+
"body": {},
1776+
"issue_number": {},
1777+
"_ui_submitted": {},
1778+
}
1779+
1780+
// issueWriteHasNonFormParams reports whether the call carries any parameter the
1781+
// issue_write MCP App form cannot represent (anything outside issueWriteFormParams,
1782+
// e.g. labels, assignees, issue_fields or a state change). Such calls must bypass
1783+
// the UI form and execute directly so the supplied values aren't silently dropped.
1784+
func issueWriteHasNonFormParams(args map[string]any) bool {
1785+
for key, value := range args {
1786+
if value == nil {
1787+
continue
1788+
}
1789+
if _, ok := issueWriteFormParams[key]; !ok {
1790+
return true
1791+
}
1792+
}
1793+
return false
1794+
}
1795+
17661796
// IssueWrite is the FeatureFlagIssueFields-enabled variant of issue_write
17671797
// (with the issue_fields parameter). LegacyIssueWrite is served when the flag
17681798
// is off. Both register under the tool name "issue_write"; exactly one is
@@ -1908,26 +1938,22 @@ Options are:
19081938
return utils.NewToolResultError(err.Error()), nil, nil
19091939
}
19101940

1911-
// When MCP Apps are enabled and the client supports UI,
1912-
// check if this is a UI form submission. The UI sends _ui_submitted=true
1913-
// to distinguish form submissions from LLM calls.
1941+
// When MCP Apps are enabled and the client supports UI, route the
1942+
// call to the interactive form unless it is itself a form submission
1943+
// (the UI sends _ui_submitted=true) or it carries parameters the form
1944+
// cannot represent (e.g. labels, assignees or issue_fields). Those
1945+
// must be applied directly so their values aren't silently dropped.
19141946
uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted")
19151947

1916-
if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted {
1948+
if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !issueWriteHasNonFormParams(args) {
19171949
if method == "update" {
1918-
// Skip the UI form when a state change is requested because
1919-
// the form only handles title/body editing and would lose the
1920-
// state transition (e.g. closing or reopening the issue).
1921-
if _, hasState := args["state"]; !hasState {
1922-
issueNumber, numErr := RequiredInt(args, "issue_number")
1923-
if numErr != nil {
1924-
return utils.NewToolResultError("issue_number is required for update method"), nil, nil
1925-
}
1926-
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
1950+
issueNumber, numErr := RequiredInt(args, "issue_number")
1951+
if numErr != nil {
1952+
return utils.NewToolResultError("issue_number is required for update method"), nil, nil
19271953
}
1928-
} else {
1929-
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
1954+
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
19301955
}
1956+
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
19311957
}
19321958

19331959
title, err := OptionalParam[string](args, "title")
@@ -2144,26 +2170,22 @@ Options are:
21442170
return utils.NewToolResultError(err.Error()), nil, nil
21452171
}
21462172

2147-
// When MCP Apps are enabled and the client supports UI,
2148-
// check if this is a UI form submission. The UI sends _ui_submitted=true
2149-
// to distinguish form submissions from LLM calls.
2173+
// When MCP Apps are enabled and the client supports UI, route the
2174+
// call to the interactive form unless it is itself a form submission
2175+
// (the UI sends _ui_submitted=true) or it carries parameters the form
2176+
// cannot represent (e.g. labels, assignees or issue_fields). Those
2177+
// must be applied directly so their values aren't silently dropped.
21502178
uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted")
21512179

2152-
if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted {
2180+
if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !issueWriteHasNonFormParams(args) {
21532181
if method == "update" {
2154-
// Skip the UI form when a state change is requested because
2155-
// the form only handles title/body editing and would lose the
2156-
// state transition (e.g. closing or reopening the issue).
2157-
if _, hasState := args["state"]; !hasState {
2158-
issueNumber, numErr := RequiredInt(args, "issue_number")
2159-
if numErr != nil {
2160-
return utils.NewToolResultError("issue_number is required for update method"), nil, nil
2161-
}
2162-
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
2182+
issueNumber, numErr := RequiredInt(args, "issue_number")
2183+
if numErr != nil {
2184+
return utils.NewToolResultError("issue_number is required for update method"), nil, nil
21632185
}
2164-
} else {
2165-
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
2186+
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
21662187
}
2188+
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
21672189
}
21682190

21692191
title, err := OptionalParam[string](args, "title")

‎pkg/github/issues_test.go‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1700,6 +1700,129 @@ func Test_IssueWrite_MCPAppsFeature_UIGate(t *testing.T) {
17001700
assert.Contains(t, textContent.Text, "Ready to update issue #1",
17011701
"update without state should show UI form")
17021702
})
1703+
1704+
t.Run("UI client with issue_fields skips form and executes directly", func(t *testing.T) {
1705+
// The MCP App form does not collect or re-send issue_fields, so a call
1706+
// carrying them must bypass the form and apply the values directly.
1707+
fieldsClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
1708+
PostReposIssuesByOwnerByRepo: expectRequestBody(t, map[string]any{
1709+
"title": "Issue with fields",
1710+
"body": "",
1711+
"labels": []any{},
1712+
"assignees": []any{},
1713+
"issue_field_values": []any{
1714+
map[string]any{"field_id": float64(101), "value": "P1"},
1715+
},
1716+
}).andThen(
1717+
mockResponse(t, http.StatusCreated, &github.Issue{
1718+
Number: github.Ptr(125),
1719+
Title: github.Ptr("Issue with fields"),
1720+
HTMLURL: github.Ptr("https://github.com/owner/repo/issues/125"),
1721+
State: github.Ptr("open"),
1722+
}),
1723+
),
1724+
}))
1725+
fieldsGQLClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(
1726+
githubv4mock.NewQueryMatcher(
1727+
issueFieldWriteMetadataQuery{},
1728+
map[string]any{
1729+
"owner": githubv4.String("owner"),
1730+
"repo": githubv4.String("repo"),
1731+
},
1732+
githubv4mock.DataResponse(map[string]any{
1733+
"repository": map[string]any{
1734+
"issueFields": map[string]any{
1735+
"nodes": []any{
1736+
map[string]any{
1737+
"__typename": "IssueFieldSingleSelect",
1738+
"fullDatabaseId": "101",
1739+
"name": "Priority",
1740+
"dataType": "single_select",
1741+
"options": []any{
1742+
map[string]any{"fullDatabaseId": "9001", "name": "P1"},
1743+
},
1744+
},
1745+
},
1746+
},
1747+
},
1748+
}),
1749+
),
1750+
))
1751+
1752+
fieldsDeps := BaseDeps{
1753+
Client: fieldsClient,
1754+
GQLClient: fieldsGQLClient,
1755+
featureChecker: featureCheckerFor(MCPAppsFeatureFlag),
1756+
}
1757+
fieldsHandler := serverTool.Handler(fieldsDeps)
1758+
1759+
request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{
1760+
"method": "create",
1761+
"owner": "owner",
1762+
"repo": "repo",
1763+
"title": "Issue with fields",
1764+
"issue_fields": []any{
1765+
map[string]any{"field_name": "Priority", "field_option_name": "P1"},
1766+
},
1767+
})
1768+
result, err := fieldsHandler(ContextWithDeps(context.Background(), fieldsDeps), &request)
1769+
require.NoError(t, err)
1770+
1771+
textContent := getTextResult(t, result)
1772+
assert.NotContains(t, textContent.Text, "Ready to create an issue",
1773+
"issue_fields should skip UI form")
1774+
assert.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/125",
1775+
"issue_fields call should execute directly and return issue URL")
1776+
})
1777+
1778+
t.Run("UI client with labels skips form and executes directly", func(t *testing.T) {
1779+
// The form does not collect labels, so a call carrying them must bypass
1780+
// the form rather than silently drop them.
1781+
request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{
1782+
"method": "create",
1783+
"owner": "owner",
1784+
"repo": "repo",
1785+
"title": "Test",
1786+
"labels": []any{"bug"},
1787+
})
1788+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
1789+
require.NoError(t, err)
1790+
1791+
textContent := getTextResult(t, result)
1792+
assert.NotContains(t, textContent.Text, "Ready to create an issue",
1793+
"labels should skip UI form")
1794+
assert.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/1",
1795+
"labels call should execute directly and return issue URL")
1796+
})
1797+
}
1798+
1799+
func Test_issueWriteHasNonFormParams(t *testing.T) {
1800+
t.Parallel()
1801+
1802+
tests := []struct {
1803+
name string
1804+
args map[string]any
1805+
want bool
1806+
}{
1807+
{name: "no params", args: map[string]any{}, want: false},
1808+
{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},
1809+
{name: "labels present", args: map[string]any{"title": "t", "labels": []any{"bug"}}, want: true},
1810+
{name: "assignees present", args: map[string]any{"title": "t", "assignees": []any{"octocat"}}, want: true},
1811+
{name: "milestone present", args: map[string]any{"title": "t", "milestone": float64(2)}, want: true},
1812+
{name: "type present", args: map[string]any{"title": "t", "type": "Bug"}, want: true},
1813+
{name: "issue_fields present", args: map[string]any{"issue_fields": []any{map[string]any{"field_name": "Priority"}}}, want: true},
1814+
{name: "state present", args: map[string]any{"state": "closed"}, want: true},
1815+
{name: "state_reason present", args: map[string]any{"state_reason": "completed"}, want: true},
1816+
{name: "duplicate_of present", args: map[string]any{"duplicate_of": float64(7)}, want: true},
1817+
{name: "nil value is ignored", args: map[string]any{"issue_fields": nil}, want: false},
1818+
}
1819+
1820+
for _, tc := range tests {
1821+
t.Run(tc.name, func(t *testing.T) {
1822+
t.Parallel()
1823+
assert.Equal(t, tc.want, issueWriteHasNonFormParams(tc.args))
1824+
})
1825+
}
17031826
}
17041827

17051828
func Test_ListIssues(t *testing.T) {

‎pkg/github/pullrequests.go‎

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -544,6 +544,37 @@ func GetPullRequestReviews(ctx context.Context, client *github.Client, deps Tool
544544
// PullRequestWriteUIResourceURI is the URI for the create_pull_request tool's MCP App UI resource.
545545
const PullRequestWriteUIResourceURI = "ui://github-mcp-server/pr-write"
546546

547+
// pullRequestWriteFormParams are the parameters the create_pull_request MCP App
548+
// form collects and re-sends on submit. Any other parameter present on a call
549+
// cannot be represented by the form.
550+
var pullRequestWriteFormParams = map[string]struct{}{
551+
"owner": {},
552+
"repo": {},
553+
"title": {},
554+
"body": {},
555+
"head": {},
556+
"base": {},
557+
"draft": {},
558+
"maintainer_can_modify": {},
559+
"_ui_submitted": {},
560+
}
561+
562+
// pullRequestWriteHasNonFormParams reports whether the call carries any parameter
563+
// the create_pull_request MCP App form cannot represent (anything outside
564+
// pullRequestWriteFormParams). Such calls must bypass the UI form and execute
565+
// directly so the supplied values aren't silently dropped.
566+
func pullRequestWriteHasNonFormParams(args map[string]any) bool {
567+
for key, value := range args {
568+
if value == nil {
569+
continue
570+
}
571+
if _, ok := pullRequestWriteFormParams[key]; !ok {
572+
return true
573+
}
574+
}
575+
return false
576+
}
577+
547578
// CreatePullRequest creates a tool to create a new pull request.
548579
func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool {
549580
return NewTool(
@@ -611,12 +642,14 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
611642
return utils.NewToolResultError(err.Error()), nil, nil
612643
}
613644

614-
// When MCP Apps are enabled and the client supports UI,
615-
// check if this is a UI form submission. The UI sends _ui_submitted=true
616-
// to distinguish form submissions from LLM calls.
645+
// When MCP Apps are enabled and the client supports UI, route the
646+
// call to the interactive form unless it is itself a form submission
647+
// (the UI sends _ui_submitted=true) or it carries parameters the form
648+
// cannot represent. Those must be applied directly so their values
649+
// aren't silently dropped.
617650
uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted")
618651

619-
if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted {
652+
if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !pullRequestWriteHasNonFormParams(args) {
620653
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
621654
}
622655

‎pkg/github/pullrequests_test.go‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2485,6 +2485,49 @@ func Test_CreatePullRequest_MCPAppsFeature_UIGate(t *testing.T) {
24852485
assert.Contains(t, textContent.Text, "https://github.com/owner/repo/pull/42",
24862486
"non-UI client should execute directly")
24872487
})
2488+
2489+
t.Run("UI client with non-form param skips form and executes directly", func(t *testing.T) {
2490+
// A parameter the form does not collect must bypass the form rather than
2491+
// be silently dropped.
2492+
request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{
2493+
"owner": "owner",
2494+
"repo": "repo",
2495+
"title": "Test PR",
2496+
"head": "feature",
2497+
"base": "main",
2498+
"reviewers": []any{"octocat"},
2499+
})
2500+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
2501+
require.NoError(t, err)
2502+
2503+
textContent := getTextResult(t, result)
2504+
assert.NotContains(t, textContent.Text, "Ready to create a pull request",
2505+
"non-form param should skip UI form")
2506+
assert.Contains(t, textContent.Text, "https://github.com/owner/repo/pull/42",
2507+
"non-form param call should execute directly and return PR URL")
2508+
})
2509+
}
2510+
2511+
func Test_pullRequestWriteHasNonFormParams(t *testing.T) {
2512+
t.Parallel()
2513+
2514+
tests := []struct {
2515+
name string
2516+
args map[string]any
2517+
want bool
2518+
}{
2519+
{name: "no params", args: map[string]any{}, want: false},
2520+
{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},
2521+
{name: "unknown param present", args: map[string]any{"title": "t", "reviewers": []any{"octocat"}}, want: true},
2522+
{name: "nil value is ignored", args: map[string]any{"reviewers": nil}, want: false},
2523+
}
2524+
2525+
for _, tc := range tests {
2526+
t.Run(tc.name, func(t *testing.T) {
2527+
t.Parallel()
2528+
assert.Equal(t, tc.want, pullRequestWriteHasNonFormParams(tc.args))
2529+
})
2530+
}
24882531
}
24892532

24902533
func TestCreateAndSubmitPullRequestReview(t *testing.T) {

‎ui/src/apps/issue-write/App.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ function CreateIssueApp() {
152152

153153
try {
154154
const params: Record<string, unknown> = {
155+
...(toolInput as Record<string, unknown> | undefined),
155156
method: isUpdateMode ? "update" : "create",
156157
owner,
157158
repo,
@@ -204,7 +205,7 @@ function CreateIssueApp() {
204205
} finally {
205206
setIsSubmitting(false);
206207
}
207-
}, [title, body, owner, repo, isUpdateMode, issueNumber, callTool, setModelContext]);
208+
}, [title, body, owner, repo, isUpdateMode, issueNumber, toolInput, callTool, setModelContext]);
208209

209210
const body_node = (() => {
210211
if (appError) {

‎ui/src/apps/pr-write/App.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ function CreatePRApp() {
156156

157157
try {
158158
const result = await callTool("create_pull_request", {
159+
...(toolInput as Record<string, unknown> | undefined),
159160
owner, repo,
160161
title: title.trim(),
161162
body: body.trim(),
@@ -193,7 +194,7 @@ function CreatePRApp() {
193194
} finally {
194195
setIsSubmitting(false);
195196
}
196-
}, [title, body, owner, repo, head, base, isDraft, maintainerCanModify, callTool, setModelContext]);
197+
}, [title, body, owner, repo, head, base, isDraft, maintainerCanModify, toolInput, callTool, setModelContext]);
197198

198199
if (successPR) {
199200
return (

0 commit comments

Comments
 (0)