Skip to content

Commit 935b77a

Browse files
fix(github): preserve MCP App UI data and dispatch scopes
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 2c6d082 commit 935b77a

11 files changed

Lines changed: 218 additions & 32 deletions

File tree

‎pkg/github/__toolsnaps__/ui_get.snap‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,12 +53,16 @@
5353
"items": {
5454
"additionalProperties": false,
5555
"properties": {
56+
"avatar_url": {
57+
"type": "string"
58+
},
5659
"login": {
5760
"type": "string"
5861
}
5962
},
6063
"required": [
61-
"login"
64+
"login",
65+
"avatar_url"
6266
],
6367
"type": "object"
6468
},
@@ -394,12 +398,16 @@
394398
"items": {
395399
"additionalProperties": false,
396400
"properties": {
401+
"avatar_url": {
402+
"type": "string"
403+
},
397404
"login": {
398405
"type": "string"
399406
}
400407
},
401408
"required": [
402-
"login"
409+
"login",
410+
"avatar_url"
403411
],
404412
"type": "object"
405413
},

‎pkg/github/typed_copilot_ui_outputs.go‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ func normalizeUIGetArguments(raw json.RawMessage) (json.RawMessage, error) {
158158
if err != nil || args == nil {
159159
return raw, err
160160
}
161+
delete(args, "_compat_method")
161162
method, err := RequiredParam[string](args, "method")
162163
if err != nil {
163164
return nil, &inventory.ToolInputError{Message: err.Error()}
@@ -179,13 +180,7 @@ func normalizeUIGetArguments(raw json.RawMessage) (json.RawMessage, error) {
179180
}
180181
}
181182
default:
182-
args["method"] = "labels"
183-
args["_compat_method"] = method
184-
if _, exists := args["repo"]; exists {
185-
if _, err := RequiredParam[string](args, "repo"); err != nil {
186-
args["repo"] = ""
187-
}
188-
}
183+
return nil, &inventory.ToolInputError{Message: fmt.Sprintf("unknown method: %s", method)}
189184
}
190185
return json.Marshal(args)
191186
}
@@ -206,7 +201,8 @@ type UIGetLabelOutput struct {
206201
}
207202

208203
type UIGetAssigneeOutput struct {
209-
Login string `json:"login"`
204+
Login string `json:"login"`
205+
AvatarURL string `json:"avatar_url"`
210206
}
211207

212208
type UIGetMilestoneOutput struct {

‎pkg/github/typed_copilot_ui_outputs_test.go‎

Lines changed: 79 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,18 @@ import (
55
"encoding/json"
66
"io"
77
"net/http"
8+
"net/http/httptest"
89
"reflect"
910
"strings"
1011
"testing"
1112

1213
ghcontext "github.com/github/github-mcp-server/pkg/context"
14+
"github.com/github/github-mcp-server/pkg/http/middleware"
15+
"github.com/github/github-mcp-server/pkg/http/oauth"
1316
"github.com/github/github-mcp-server/pkg/inventory"
17+
"github.com/github/github-mcp-server/pkg/scopes"
1418
"github.com/github/github-mcp-server/pkg/translations"
19+
"github.com/github/github-mcp-server/pkg/utils"
1520
"github.com/google/jsonschema-go/jsonschema"
1621
"github.com/modelcontextprotocol/go-sdk/mcp"
1722
"github.com/shurcooL/githubv4"
@@ -274,12 +279,12 @@ func TestTypedUIGetNonemptyWireOutputs(t *testing.T) {
274279
structured string
275280
}{
276281
{method: "labels", args: map[string]any{"method": "labels", "owner": "owner", "repo": "repo"}, structured: `{"method":"labels","labels":{"labels":[{"id":"LABEL_1","name":"bug","color":"d73a4a","description":"Something isn't working"}],"totalCount":1,"has_more":false}}`},
277-
{method: "assignees", args: map[string]any{"method": "assignees", "owner": "owner", "repo": "repo"}, structured: `{"method":"assignees","assignees":{"assignees":[{"login":"octocat"}],"totalCount":1,"has_more":false}}`},
282+
{method: "assignees", args: map[string]any{"method": "assignees", "owner": "owner", "repo": "repo"}, structured: `{"method":"assignees","assignees":{"assignees":[{"login":"octocat","avatar_url":"https://example.com/avatar.png"}],"totalCount":1,"has_more":false}}`},
278283
{method: "milestones", args: map[string]any{"method": "milestones", "owner": "owner", "repo": "repo"}, structured: `{"method":"milestones","milestones":{"milestones":[{"number":4,"title":"v1","description":"First release","state":"open","open_issues":2,"due_on":"2026-12-31"}],"totalCount":1,"has_more":false}}`},
279284
{method: "issue_types", args: map[string]any{"method": "issue_types", "owner": "owner"}, structured: `{"method":"issue_types","issue_types":[{"id":12,"name":"Bug","description":"A bug","color":"red"},null]}`},
280285
{method: "branches", args: map[string]any{"method": "branches", "owner": "owner", "repo": "repo"}, structured: `{"method":"branches","branches":{"branches":[{"name":"main","sha":"abc123","protected":true}],"totalCount":1,"has_more":false}}`},
281286
{method: "issue_fields", args: map[string]any{"method": "issue_fields", "owner": "owner", "repo": "repo"}, structured: `{"method":"issue_fields","issue_fields":{"fields":[{"id":"FIELD_1","name":"Priority","data_type":"text","description":"Issue priority"}],"totalCount":1}}`},
282-
{method: "reviewers", args: map[string]any{"method": "reviewers", "owner": "owner", "repo": "repo"}, structured: `{"method":"reviewers","reviewers":{"users":[{"login":"octocat"}],"teams":[{"slug":"docs","name":"Docs","org":"owner"}],"totalCount":2,"has_more":false}}`},
287+
{method: "reviewers", args: map[string]any{"method": "reviewers", "owner": "owner", "repo": "repo"}, structured: `{"method":"reviewers","reviewers":{"users":[{"login":"octocat","avatar_url":"https://example.com/avatar.png"}],"teams":[{"slug":"docs","name":"Docs","org":"owner"}],"totalCount":2,"has_more":false}}`},
283288
}
284289
for _, tc := range tests {
285290
t.Run(tc.method, func(t *testing.T) {
@@ -337,6 +342,8 @@ func TestTypedCopilotUIErrorsPreserveLegacyText(t *testing.T) {
337342
{name: "request_copilot_review", args: map[string]any{"repo": "repo", "pullNumber": 7}, text: "missing required parameter: owner"},
338343
{name: "ui_get", args: map[string]any{"method": "labels", "owner": "owner"}, text: "missing required parameter: repo"},
339344
{name: "ui_get", args: map[string]any{"method": "unknown", "owner": "owner"}, text: "unknown method: unknown"},
345+
{name: "ui_get", args: map[string]any{"method": "unknown", "owner": "owner", "repo": true}, text: "unknown method: unknown"},
346+
{name: "ui_get", args: map[string]any{"method": "unknown"}, text: "missing required parameter: owner"},
340347
{name: "ui_get", args: map[string]any{"method": true, "owner": "owner"}, text: "parameter method is not of type string"},
341348
}
342349
for _, protocol := range protocols {
@@ -354,20 +361,88 @@ func TestTypedCopilotUIErrorsPreserveLegacyText(t *testing.T) {
354361
}
355362
}
356363

364+
func TestTypedUIGetHTTPScopesAndDispatch(t *testing.T) {
365+
registry, err := inventory.NewBuilder().SetTools([]inventory.ServerTool{
366+
UIGet(translations.NullTranslationHelper),
367+
}).WithToolsets([]string{"all"}).Build()
368+
require.NoError(t, err)
369+
scopes.SetToolScopeMapFromInventory(registry)
370+
t.Cleanup(func() { scopes.SetGlobalToolScopeMap(nil) })
371+
372+
for _, protocol := range []string{inventory.ProtocolVersionMultiRoundTrip, "2025-11-25", "", "unknown"} {
373+
t.Run("protocol="+protocol, func(t *testing.T) {
374+
session, _ := typedCopilotUISession(t, typedCopilotUIClient(t), protocol)
375+
for _, tc := range []struct {
376+
name string
377+
args map[string]any
378+
scopes []string
379+
wantStatus int
380+
wantError string
381+
}{
382+
{name: "unknown method does not challenge", args: map[string]any{"method": "unknown", "owner": "owner", "repo": "repo"}, scopes: []string{"gist"}, wantStatus: http.StatusOK, wantError: "unknown method: unknown"},
383+
{name: "unknown method validates owner first", args: map[string]any{"method": "unknown", "repo": "repo"}, scopes: []string{"gist"}, wantStatus: http.StatusOK, wantError: "missing required parameter: owner"},
384+
{name: "labels cannot be overridden to issue types", args: map[string]any{"method": "labels", "owner": "owner", "repo": "repo", "_compat_method": "issue_types"}, scopes: []string{"repo"}, wantStatus: http.StatusOK},
385+
{name: "labels cannot be overridden to reviewers", args: map[string]any{"method": "labels", "owner": "owner", "repo": "repo", "_compat_method": "reviewers"}, scopes: []string{"repo"}, wantStatus: http.StatusOK},
386+
{name: "issue types still require read org", args: map[string]any{"method": "issue_types", "owner": "owner", "_compat_method": "labels"}, scopes: []string{"repo"}, wantStatus: http.StatusForbidden},
387+
{name: "labels still require repo", args: map[string]any{"method": "labels", "owner": "owner", "repo": "repo"}, scopes: []string{"gist"}, wantStatus: http.StatusForbidden},
388+
} {
389+
t.Run(tc.name, func(t *testing.T) {
390+
raw, err := json.Marshal(tc.args)
391+
require.NoError(t, err)
392+
called := false
393+
handler := middleware.WithScopeChallenge(&oauth.Config{}, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
394+
called = true
395+
result, err := session.CallTool(r.Context(), &mcp.CallToolParams{Name: "ui_get", Arguments: tc.args})
396+
require.NoError(t, err)
397+
if tc.wantError != "" {
398+
require.True(t, result.IsError)
399+
assert.Equal(t, tc.wantError, getTextResult(t, result).Text)
400+
assert.Nil(t, result.StructuredContent)
401+
} else {
402+
require.False(t, result.IsError)
403+
if protocol == inventory.ProtocolVersionMultiRoundTrip {
404+
assert.JSONEq(t, `{"method":"labels","labels":{"labels":[],"totalCount":0,"has_more":false}}`, getTextResult(t, result).Text)
405+
} else {
406+
assert.Equal(t, `{"has_more":false,"labels":[],"totalCount":0}`, getTextResult(t, result).Text)
407+
}
408+
}
409+
require.NoError(t, json.NewEncoder(w).Encode(result))
410+
}))
411+
request := httptest.NewRequest(http.MethodPost, "/mcp", nil)
412+
ctx := ghcontext.WithTokenInfo(request.Context(), &ghcontext.TokenInfo{TokenType: utils.TokenTypeOAuthAccessToken})
413+
ctx = ghcontext.WithTokenScopes(ctx, tc.scopes)
414+
ctx = ghcontext.WithMCPMethodInfo(ctx, &ghcontext.MCPMethodInfo{
415+
Method: "tools/call", ItemName: "ui_get", RawArguments: raw, ProtocolVersion: protocol,
416+
})
417+
response := httptest.NewRecorder()
418+
handler.ServeHTTP(response, request.WithContext(ctx))
419+
assert.Equal(t, tc.wantStatus, response.Code)
420+
assert.Equal(t, tc.wantStatus == http.StatusOK, called)
421+
if tc.wantStatus == http.StatusOK {
422+
assert.Empty(t, response.Header().Get("WWW-Authenticate"))
423+
} else {
424+
assert.Contains(t, response.Header().Get("WWW-Authenticate"), "insufficient_scope")
425+
}
426+
})
427+
}
428+
})
429+
}
430+
}
431+
357432
func TestTypedUIGetUnionOutputs(t *testing.T) {
358433
cases := []struct {
359434
method string
360435
raw string
361436
typed string
362437
}{
363438
{method: "labels", raw: `{"labels":[{"id":"L","name":"bug","color":"red","description":""}],"totalCount":1,"has_more":false}`, typed: `{"method":"labels","labels":{"labels":[{"id":"L","name":"bug","color":"red","description":""}],"totalCount":1,"has_more":false}}`},
364-
{method: "assignees", raw: `{"assignees":[{"login":"octocat","avatar_url":"https://example.com/avatar.png"}],"totalCount":1,"has_more":false}`, typed: `{"method":"assignees","assignees":{"assignees":[{"login":"octocat"}],"totalCount":1,"has_more":false}}`},
439+
{method: "assignees", raw: `{"assignees":[{"login":"octocat","avatar_url":"https://example.com/avatar.png"}],"totalCount":1,"has_more":false}`, typed: `{"method":"assignees","assignees":{"assignees":[{"login":"octocat","avatar_url":"https://example.com/avatar.png"}],"totalCount":1,"has_more":false}}`},
365440
{method: "milestones", raw: `{"milestones":[{"number":4,"title":"v1","description":"","state":"open","open_issues":2,"due_on":""}],"totalCount":1,"has_more":false}`, typed: `{"method":"milestones","milestones":{"milestones":[{"number":4,"title":"v1","description":"","state":"open","open_issues":2,"due_on":""}],"totalCount":1,"has_more":false}}`},
366441
{method: "issue_types", raw: `[{"id":1,"name":"Bug"}]`, typed: `{"method":"issue_types","issue_types":[{"id":1,"name":"Bug"}]}`},
367442
{method: "issue_types", raw: `null`, typed: `{"method":"issue_types","issue_types":null}`},
368443
{method: "branches", raw: `{"branches":[{"name":"main","sha":"abc","protected":true}],"totalCount":1,"has_more":false}`, typed: `{"method":"branches","branches":{"branches":[{"name":"main","sha":"abc","protected":true}],"totalCount":1,"has_more":false}}`},
369444
{method: "issue_fields", raw: `{"fields":[{"id":"F","name":"Priority","data_type":"single_select","description":"","options":[]}],"totalCount":1}`, typed: `{"method":"issue_fields","issue_fields":{"fields":[{"id":"F","name":"Priority","data_type":"single_select","description":"","options":[]}],"totalCount":1}}`},
370-
{method: "reviewers", raw: `{"users":[{"login":"octocat","avatar_url":""}],"teams":[{"slug":"docs","name":"Docs","org":"owner"}],"totalCount":2,"has_more":false}`, typed: `{"method":"reviewers","reviewers":{"users":[{"login":"octocat"}],"teams":[{"slug":"docs","name":"Docs","org":"owner"}],"totalCount":2,"has_more":false}}`},
445+
{method: "reviewers", raw: `{"users":[{"login":"octocat","avatar_url":""}],"teams":[{"slug":"docs","name":"Docs","org":"owner"}],"totalCount":2,"has_more":false}`, typed: `{"method":"reviewers","reviewers":{"users":[{"login":"octocat","avatar_url":""}],"teams":[{"slug":"docs","name":"Docs","org":"owner"}],"totalCount":2,"has_more":false}}`},
371446
}
372447
schema, err := uiGetOutputSchema().Resolve(nil)
373448
require.NoError(t, err)

‎pkg/github/ui_tools.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,6 @@ func UIGet(t translations.TranslationHelperFunc) inventory.ServerTool {
7474
if err != nil {
7575
return utils.NewToolResultError(err.Error()), &UIGetOutput{Method: "labels"}, nil
7676
}
77-
if compatibilityMethod, ok := args["_compat_method"].(string); ok {
78-
method = compatibilityMethod
79-
}
80-
8177
owner, err := RequiredParam[string](args, "owner")
8278
if err != nil {
8379
return utils.NewToolResultError(err.Error()), &UIGetOutput{Method: uiGetSchemaMethod(method)}, nil

‎ui/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
"build": "node scripts/build.mjs",
1212
"dev": "npm run build",
1313
"typecheck": "tsc --noEmit",
14+
"test": "node --experimental-strip-types --test src/lib/*.test.ts",
1415
"clean": "rm -rf dist"
1516
},
1617
"dependencies": {

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

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
import { AppProvider } from "../../components/AppProvider";
2626
import { useMcpApp } from "../../hooks/useMcpApp";
2727
import { completedToolResult } from "../../lib/toolResult";
28+
import { parseUIGetResult } from "../../lib/uiGetResult";
2829
import { MarkdownEditor } from "../../components/MarkdownEditor";
2930

3031
interface IssueResult {
@@ -552,7 +553,7 @@ function CreateIssueApp() {
552553
(c: { type: string }) => c.type === "text"
553554
);
554555
if (textContent && "text" in textContent) {
555-
const data = JSON.parse(textContent.text as string);
556+
const data = parseUIGetResult("labels", textContent.text as string);
556557
const labels = (data.labels || []).map(
557558
(l: { name: string; color: string; id: string }) => ({
558559
id: l.id || l.name,
@@ -579,7 +580,7 @@ function CreateIssueApp() {
579580
(c: { type: string }) => c.type === "text"
580581
);
581582
if (textContent && "text" in textContent) {
582-
const data = JSON.parse(textContent.text as string);
583+
const data = parseUIGetResult("assignees", textContent.text as string);
583584
const assignees = (data.assignees || []).map(
584585
(a: { login: string }) => ({
585586
id: a.login,
@@ -605,7 +606,7 @@ function CreateIssueApp() {
605606
(c: { type: string }) => c.type === "text"
606607
);
607608
if (textContent && "text" in textContent) {
608-
const data = JSON.parse(textContent.text as string);
609+
const data = parseUIGetResult("milestones", textContent.text as string);
609610
const milestones = (data.milestones || []).map(
610611
(m: { number: number; title: string; description: string }) => ({
611612
id: String(m.number),
@@ -633,10 +634,8 @@ function CreateIssueApp() {
633634
(c: { type: string }) => c.type === "text"
634635
);
635636
if (textContent && "text" in textContent) {
636-
const data = JSON.parse(textContent.text as string);
637-
// ui_get returns array directly or wrapped in issue_types/types
638-
const typesArray = Array.isArray(data) ? data : (data.issue_types || data.types || []);
639-
const types = typesArray.map(
637+
const typesArray = parseUIGetResult("issue_types", textContent.text as string);
638+
const types = typesArray.filter(t => t !== null).map(
640639
(t: { id: number; name: string; description?: string } | string) => {
641640
if (typeof t === "string") {
642641
return { id: t, text: t };
@@ -663,7 +662,7 @@ function CreateIssueApp() {
663662
(c: { type: string }) => c.type === "text"
664663
);
665664
if (textContent && "text" in textContent) {
666-
const data = JSON.parse(textContent.text as string);
665+
const data = parseUIGetResult("issue_fields", textContent.text as string);
667666
const fields = (data.fields || [])
668667
.map(
669668
(field: {

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import {
2626
import { AppProvider } from "../../components/AppProvider";
2727
import { useMcpApp } from "../../hooks/useMcpApp";
2828
import { completedToolResult } from "../../lib/toolResult";
29+
import { parseUIGetResult } from "../../lib/uiGetResult";
2930
import { MarkdownEditor } from "../../components/MarkdownEditor";
3031

3132
interface PRResult {
@@ -392,8 +393,8 @@ function EditPRApp() {
392393
if (result && !result.isError && result.content) {
393394
const textContent = result.content.find((c: { type: string }) => c.type === "text");
394395
if (textContent && "text" in textContent) {
395-
const data = JSON.parse(textContent.text as string);
396-
const branches = (data.branches || data || []).map(
396+
const data = parseUIGetResult("branches", textContent.text as string);
397+
const branches = (data.branches || []).map(
397398
(b: { name: string; protected?: boolean }) => ({ name: b.name, protected: b.protected || false })
398399
);
399400
setAvailableBranches(branches);
@@ -416,7 +417,7 @@ function EditPRApp() {
416417
if (result && !result.isError && result.content) {
417418
const textContent = result.content.find((c: { type: string }) => c.type === "text");
418419
if (textContent && "text" in textContent) {
419-
const data = JSON.parse(textContent.text as string);
420+
const data = parseUIGetResult("reviewers", textContent.text as string);
420421
const users = (data.users || []).map(
421422
(u: { login: string; avatar_url?: string }) => ({
422423
kind: "user" as const,

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import {
2828
import { AppProvider } from "../../components/AppProvider";
2929
import { useMcpApp } from "../../hooks/useMcpApp";
3030
import { completedToolResult } from "../../lib/toolResult";
31+
import { parseUIGetResult } from "../../lib/uiGetResult";
3132
import { MarkdownEditor } from "../../components/MarkdownEditor";
3233

3334
interface PRResult {
@@ -335,8 +336,8 @@ function CreatePRApp() {
335336
if (result && !result.isError && result.content) {
336337
const textContent = result.content.find((c: { type: string }) => c.type === "text");
337338
if (textContent && "text" in textContent) {
338-
const data = JSON.parse(textContent.text as string);
339-
const branches = (data.branches || data || []).map(
339+
const data = parseUIGetResult("branches", textContent.text as string);
340+
const branches = (data.branches || []).map(
340341
(b: { name: string; protected?: boolean }) => ({ name: b.name, protected: b.protected || false })
341342
);
342343
setAvailableBranches(branches);
@@ -363,7 +364,7 @@ function CreatePRApp() {
363364
if (result && !result.isError && result.content) {
364365
const textContent = result.content.find((c: { type: string }) => c.type === "text");
365366
if (textContent && "text" in textContent) {
366-
const data = JSON.parse(textContent.text as string);
367+
const data = parseUIGetResult("reviewers", textContent.text as string);
367368
const users = (data.users || []).map(
368369
(u: { login: string; avatar_url?: string }) => ({
369370
kind: "user" as const,

0 commit comments

Comments
 (0)