Skip to content

Commit 50a0461

Browse files
Copilotmattdholloway
authored andcommitted
Skip MCP Apps UI form when update includes a state change
When using issue_write with method "update" and a state parameter (e.g. "closed"), the MCP Apps UI form was incorrectly shown. The form only handles title/body editing and would lose the state transition. Now when a state change is requested, the UI form is skipped and the update executes directly. Fixes #798 Co-authored-by: mattdholloway <918573+mattdholloway@users.noreply.github.com>
1 parent 7848af8 commit 50a0461

File tree

2 files changed

+118
-5
lines changed

2 files changed

+118
-5
lines changed

pkg/github/issues.go

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1080,13 +1080,20 @@ Options are:
10801080

10811081
if deps.GetFlags(ctx).InsidersMode && clientSupportsUI(ctx, req) && !uiSubmitted {
10821082
if method == "update" {
1083-
issueNumber, numErr := RequiredInt(args, "issue_number")
1084-
if numErr != nil {
1085-
return utils.NewToolResultError("issue_number is required for update method"), nil, nil
1083+
// Skip the UI form when a state change is requested because
1084+
// the form only handles title/body editing and would lose the
1085+
// state transition (e.g. closing or reopening the issue).
1086+
state, _ := OptionalParam[string](args, "state")
1087+
if state == "" {
1088+
issueNumber, numErr := RequiredInt(args, "issue_number")
1089+
if numErr != nil {
1090+
return utils.NewToolResultError("issue_number is required for update method"), nil, nil
1091+
}
1092+
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
10861093
}
1087-
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
1094+
} else {
1095+
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
10881096
}
1089-
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
10901097
}
10911098

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

pkg/github/issues_test.go

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1000,6 +1000,112 @@ func Test_IssueWrite_InsidersMode_UIGate(t *testing.T) {
10001000
assert.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/1",
10011001
"non-UI client should execute directly")
10021002
})
1003+
1004+
t.Run("UI client with state change skips form and executes directly", func(t *testing.T) {
1005+
mockBaseIssue := &github.Issue{
1006+
Number: github.Ptr(1),
1007+
Title: github.Ptr("Test"),
1008+
State: github.Ptr("open"),
1009+
HTMLURL: github.Ptr("https://github.com/owner/repo/issues/1"),
1010+
}
1011+
issueIDQueryResponse := githubv4mock.DataResponse(map[string]any{
1012+
"repository": map[string]any{
1013+
"issue": map[string]any{
1014+
"id": "I_kwDOA0xdyM50BPaO",
1015+
},
1016+
},
1017+
})
1018+
closeSuccessResponse := githubv4mock.DataResponse(map[string]any{
1019+
"closeIssue": map[string]any{
1020+
"issue": map[string]any{
1021+
"id": "I_kwDOA0xdyM50BPaO",
1022+
"number": 1,
1023+
"url": "https://github.com/owner/repo/issues/1",
1024+
"state": "CLOSED",
1025+
},
1026+
},
1027+
})
1028+
completedReason := IssueClosedStateReasonCompleted
1029+
1030+
closeClient := github.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
1031+
PatchReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockBaseIssue),
1032+
}))
1033+
closeGQLClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(
1034+
githubv4mock.NewQueryMatcher(
1035+
struct {
1036+
Repository struct {
1037+
Issue struct {
1038+
ID githubv4.ID
1039+
} `graphql:"issue(number: $issueNumber)"`
1040+
} `graphql:"repository(owner: $owner, name: $repo)"`
1041+
}{},
1042+
map[string]any{
1043+
"owner": githubv4.String("owner"),
1044+
"repo": githubv4.String("repo"),
1045+
"issueNumber": githubv4.Int(1),
1046+
},
1047+
issueIDQueryResponse,
1048+
),
1049+
githubv4mock.NewMutationMatcher(
1050+
struct {
1051+
CloseIssue struct {
1052+
Issue struct {
1053+
ID githubv4.ID
1054+
Number githubv4.Int
1055+
URL githubv4.String
1056+
State githubv4.String
1057+
}
1058+
} `graphql:"closeIssue(input: $input)"`
1059+
}{},
1060+
CloseIssueInput{
1061+
IssueID: "I_kwDOA0xdyM50BPaO",
1062+
StateReason: &completedReason,
1063+
},
1064+
nil,
1065+
closeSuccessResponse,
1066+
),
1067+
))
1068+
1069+
closeDeps := BaseDeps{
1070+
Client: closeClient,
1071+
GQLClient: closeGQLClient,
1072+
Flags: FeatureFlags{InsidersMode: true},
1073+
}
1074+
closeHandler := serverTool.Handler(closeDeps)
1075+
1076+
request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{
1077+
"method": "update",
1078+
"owner": "owner",
1079+
"repo": "repo",
1080+
"issue_number": float64(1),
1081+
"state": "closed",
1082+
"state_reason": "completed",
1083+
})
1084+
result, err := closeHandler(ContextWithDeps(context.Background(), closeDeps), &request)
1085+
require.NoError(t, err)
1086+
1087+
textContent := getTextResult(t, result)
1088+
assert.NotContains(t, textContent.Text, "Ready to update issue",
1089+
"state change should skip UI form")
1090+
assert.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/1",
1091+
"state change should execute directly and return issue URL")
1092+
})
1093+
1094+
t.Run("UI client update without state change returns form message", func(t *testing.T) {
1095+
request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{
1096+
"method": "update",
1097+
"owner": "owner",
1098+
"repo": "repo",
1099+
"issue_number": float64(1),
1100+
"title": "New Title",
1101+
})
1102+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
1103+
require.NoError(t, err)
1104+
1105+
textContent := getTextResult(t, result)
1106+
assert.Contains(t, textContent.Text, "Ready to update issue #1",
1107+
"update without state should show UI form")
1108+
})
10031109
}
10041110

10051111
func Test_ListIssues(t *testing.T) {

0 commit comments

Comments
 (0)