Skip to content

Commit fad6295

Browse files
kerobbiSamMorrowDrums
authored andcommitted
retain typed output fidelity
1 parent c0de09a commit fad6295

14 files changed

Lines changed: 304 additions & 95 deletions

‎pkg/github/__toolsnaps__/get_dependabot_alert.snap‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,13 @@
100100
"security_advisory": {
101101
"additionalProperties": false,
102102
"properties": {
103+
"classification": {
104+
"description": "GitHub advisory classification, such as malware or general.",
105+
"type": [
106+
"null",
107+
"string"
108+
]
109+
},
103110
"cve_id": {
104111
"type": [
105112
"null",

‎pkg/github/__toolsnaps__/get_notification_details.snap‎

Lines changed: 9 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -35,23 +35,7 @@
3535
]
3636
},
3737
"reason": {
38-
"enum": [
39-
"approval_requested",
40-
"assign",
41-
"author",
42-
"ci_activity",
43-
"comment",
44-
"invitation",
45-
"manual",
46-
"member_feature_requested",
47-
"mention",
48-
"review_requested",
49-
"security_alert",
50-
"security_advisory_credit",
51-
"state_change",
52-
"subscribed",
53-
"team_mention"
54-
],
38+
"description": "Notification reason reported by GitHub; new provider values are preserved.",
5539
"type": [
5640
"null",
5741
"string"
@@ -106,16 +90,14 @@
10690
]
10791
},
10892
"type": {
109-
"enum": [
110-
"CheckSuite",
111-
"Commit",
112-
"Discussion",
113-
"Issue",
114-
"PullRequest",
115-
"Release",
116-
"RepositoryInvitation",
117-
"SecurityAdvisory"
118-
],
93+
"description": "Subject type reported by GitHub; new provider values are preserved.",
94+
"type": [
95+
"null",
96+
"string"
97+
]
98+
},
99+
"url": {
100+
"description": "API reference to the underlying notification subject.",
119101
"type": [
120102
"null",
121103
"string"

‎pkg/github/__toolsnaps__/get_secret_scanning_alert.snap‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,26 @@
7676
"object"
7777
]
7878
},
79+
"has_more_locations": {
80+
"description": "Whether additional secret locations exist beyond the first location.",
81+
"type": [
82+
"null",
83+
"boolean"
84+
]
85+
},
7986
"html_url": {
8087
"type": [
8188
"null",
8289
"string"
8390
]
8491
},
92+
"is_base64_encoded": {
93+
"description": "Whether the detected secret is base64 encoded.",
94+
"type": [
95+
"null",
96+
"boolean"
97+
]
98+
},
8599
"number": {
86100
"type": [
87101
"null",

‎pkg/github/__toolsnaps__/list_dependabot_alerts.snap‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,13 @@
130130
"security_advisory": {
131131
"additionalProperties": false,
132132
"properties": {
133+
"classification": {
134+
"description": "GitHub advisory classification, such as malware or general.",
135+
"type": [
136+
"null",
137+
"string"
138+
]
139+
},
133140
"cve_id": {
134141
"type": [
135142
"null",

‎pkg/github/__toolsnaps__/list_notifications.snap‎

Lines changed: 9 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -65,23 +65,7 @@
6565
]
6666
},
6767
"reason": {
68-
"enum": [
69-
"approval_requested",
70-
"assign",
71-
"author",
72-
"ci_activity",
73-
"comment",
74-
"invitation",
75-
"manual",
76-
"member_feature_requested",
77-
"mention",
78-
"review_requested",
79-
"security_alert",
80-
"security_advisory_credit",
81-
"state_change",
82-
"subscribed",
83-
"team_mention"
84-
],
68+
"description": "Notification reason reported by GitHub; new provider values are preserved.",
8569
"type": [
8670
"null",
8771
"string"
@@ -136,16 +120,14 @@
136120
]
137121
},
138122
"type": {
139-
"enum": [
140-
"CheckSuite",
141-
"Commit",
142-
"Discussion",
143-
"Issue",
144-
"PullRequest",
145-
"Release",
146-
"RepositoryInvitation",
147-
"SecurityAdvisory"
148-
],
123+
"description": "Subject type reported by GitHub; new provider values are preserved.",
124+
"type": [
125+
"null",
126+
"string"
127+
]
128+
},
129+
"url": {
130+
"description": "API reference to the underlying notification subject.",
149131
"type": [
150132
"null",
151133
"string"

‎pkg/github/__toolsnaps__/list_secret_scanning_alerts.snap‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,12 +107,26 @@
107107
"object"
108108
]
109109
},
110+
"has_more_locations": {
111+
"description": "Whether additional secret locations exist beyond the first location.",
112+
"type": [
113+
"null",
114+
"boolean"
115+
]
116+
},
110117
"html_url": {
111118
"type": [
112119
"null",
113120
"string"
114121
]
115122
},
123+
"is_base64_encoded": {
124+
"description": "Whether the detected secret is base64 encoded.",
125+
"type": [
126+
"null",
127+
"boolean"
128+
]
129+
},
116130
"number": {
117131
"type": [
118132
"null",

‎pkg/github/__toolsnaps__/repository_ruleset_read.snap‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,10 @@
282282
"before_sha": {
283283
"type": "string"
284284
},
285+
"evaluation_result": {
286+
"description": "Result of rules evaluated without enforcement, distinct from the enforced result.",
287+
"type": "string"
288+
},
285289
"id": {
286290
"type": "integer"
287291
},
@@ -324,7 +328,10 @@
324328
"additionalProperties": false,
325329
"properties": {
326330
"id": {
327-
"type": "integer"
331+
"type": [
332+
"null",
333+
"integer"
334+
]
328335
},
329336
"name": {
330337
"type": "string"

‎pkg/github/discussion_notification_contracts_test.go‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -173,10 +173,9 @@ func TestNotificationProjectionTrimsHypermedia(t *testing.T) {
173173
LatestCommentURL: new("https://api.github.com/repos/owner/repo/issues/comments/2"),
174174
},
175175
}
176-
// The pre-quality DTO exposed these API links; legacy output still does.
177176
before := `{"id":"123","subject":{"title":"Title","url":"https://api.github.com/repos/owner/repo/issues/1","latest_comment_url":"https://api.github.com/repos/owner/repo/issues/comments/2","type":"Issue"},"url":"https://api.github.com/notifications/threads/123"}`
178177
after := mustMarshalJSON(t, notificationOutput(notification))
179-
assert.JSONEq(t, `{"id":"123","subject":{"title":"Title","type":"Issue"}}`, after)
178+
assert.JSONEq(t, `{"id":"123","subject":{"title":"Title","type":"Issue","url":"https://api.github.com/repos/owner/repo/issues/1"}}`, after)
180179
assert.Less(t, len(after), len(before))
181180
assert.Contains(t, mustMarshalJSON(t, notification), "latest_comment_url", "legacy formatter must retain its existing links")
182181
t.Logf("populated notification projection: %d -> %d JSON bytes", len(before), len(after))
@@ -240,17 +239,32 @@ func TestDiscussionEmptyCollections(t *testing.T) {
240239
}
241240
}
242241

243-
func TestNotificationOutputEnumsAndHypermediaSchemas(t *testing.T) {
242+
func TestNotificationOutputProviderValuesAndReferenceSchema(t *testing.T) {
244243
schema := notificationOutputSchema(false)
245244
assert.NotContains(t, schema.Properties, "url")
246-
assert.NotContains(t, schema.Properties["subject"].Properties, "url")
245+
assert.Contains(t, schema.Properties["subject"].Properties, "url")
247246
assert.NotContains(t, schema.Properties["subject"].Properties, "latest_comment_url")
248247
assert.Contains(t, schema.Properties["repository"].Properties, "html_url")
249248
resolved, err := schema.Resolve(nil)
250249
require.NoError(t, err)
251-
assert.NoError(t, resolved.Validate(map[string]any{"reason": "mention", "subject": map[string]any{"type": "Discussion"}}))
252-
assert.Error(t, resolved.Validate(map[string]any{"reason": "invalid"}))
253-
assert.Error(t, resolved.Validate(map[string]any{"subject": map[string]any{"type": "invalid"}}))
250+
assert.Empty(t, schema.Properties["reason"].Enum)
251+
assert.Empty(t, schema.Properties["subject"].Properties["type"].Enum)
252+
for _, reason := range []string{
253+
"agent_session_finished", "approval_requested", "assign", "author", "ci_activity",
254+
"comment", "invitation", "manual", "member_feature_requested", "mention",
255+
"review_requested", "security_advisory_credit", "security_alert",
256+
"state_change", "subscribed", "team_mention", "future_reason",
257+
} {
258+
assert.NoError(t, resolved.Validate(map[string]any{"reason": reason}), reason)
259+
}
260+
for _, subjectType := range []string{
261+
"CheckSuite", "Commit", "Discussion", "Issue", "PullRequest", "Release",
262+
"RepositoryInvitation", "SecurityAdvisory", "FutureSubject",
263+
} {
264+
assert.NoError(t, resolved.Validate(map[string]any{"subject": map[string]any{"type": subjectType}}), subjectType)
265+
}
266+
assert.Error(t, resolved.Validate(map[string]any{"reason": 42}))
267+
assert.Error(t, resolved.Validate(map[string]any{"subject": map[string]any{"type": 42}}))
254268
subscription := discussionNotificationOutputSchema[NotificationSubscriptionOutput]()
255269
for _, field := range []string{"url", "thread_url", "repository_url"} {
256270
assert.NotContains(t, subscription.Properties, field)

‎pkg/github/discussion_notification_outputs.go‎

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@ import (
1010

1111
type NotificationSubjectOutput struct {
1212
Title *string `json:"title,omitempty"`
13-
Type *string `json:"type,omitempty"`
13+
Type *string `json:"type,omitempty" jsonschema:"Subject type reported by GitHub; new provider values are preserved."`
14+
URL *string `json:"url,omitempty" jsonschema:"API reference to the underlying notification subject."`
1415
}
1516

1617
type NotificationRepositoryOutput struct {
@@ -25,7 +26,7 @@ type NotificationOutput struct {
2526
ID *string `json:"id,omitempty"`
2627
Repository *NotificationRepositoryOutput `json:"repository,omitempty"`
2728
Subject *NotificationSubjectOutput `json:"subject,omitempty"`
28-
Reason *string `json:"reason,omitempty"`
29+
Reason *string `json:"reason,omitempty" jsonschema:"Notification reason reported by GitHub; new provider values are preserved."`
2930
Unread *bool `json:"unread,omitempty"`
3031
UpdatedAt *time.Time `json:"updated_at,omitempty" jsonschema:"Last update time (RFC3339)."`
3132
LastReadAt *time.Time `json:"last_read_at,omitempty" jsonschema:"Last acknowledgement time (RFC3339)."`
@@ -46,7 +47,7 @@ func notificationOutput(n *github.Notification) *NotificationOutput {
4647
}
4748
if n.Subject != nil {
4849
out.Subject = &NotificationSubjectOutput{
49-
Title: n.Subject.Title, Type: n.Subject.Type,
50+
Title: n.Subject.Title, Type: n.Subject.Type, URL: n.Subject.URL,
5051
}
5152
}
5253
if n.Repository != nil {
@@ -128,16 +129,6 @@ func discussionNotificationOutputSchema[Out any]() *jsonschema.Schema {
128129

129130
func notificationOutputSchema(list bool) *jsonschema.Schema {
130131
schema := discussionNotificationOutputSchema[NotificationOutput]()
131-
schema.Properties["subject"].Properties["type"].Enum = []any{
132-
"CheckSuite", "Commit", "Discussion", "Issue", "PullRequest", "Release",
133-
"RepositoryInvitation", "SecurityAdvisory",
134-
}
135-
schema.Properties["reason"].Enum = []any{
136-
"approval_requested", "assign", "author", "ci_activity", "comment",
137-
"invitation", "manual", "member_feature_requested", "mention",
138-
"review_requested", "security_alert", "security_advisory_credit",
139-
"state_change", "subscribed", "team_mention",
140-
}
141132
if list {
142133
schema.Type = ""
143134
schema.Types = []string{"object", "null"}
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
package github
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"net/http"
7+
"testing"
8+
9+
"github.com/github/github-mcp-server/pkg/inventory"
10+
"github.com/github/github-mcp-server/pkg/translations"
11+
"github.com/google/go-github/v92/github"
12+
"github.com/modelcontextprotocol/go-sdk/mcp"
13+
"github.com/stretchr/testify/assert"
14+
"github.com/stretchr/testify/require"
15+
)
16+
17+
func TestNotificationOutputFidelity(t *testing.T) {
18+
for _, subjectType := range []string{"Issue", "PullRequest", "Discussion", "FutureSubject"} {
19+
t.Run(subjectType, func(t *testing.T) {
20+
notification := &github.Notification{
21+
ID: new("123"), Reason: new("agent_session_finished"),
22+
Subject: &github.NotificationSubject{
23+
Type: new(subjectType),
24+
URL: new("https://github.example/api/v3/repos/owner/repo/issues/42"),
25+
LatestCommentURL: new("legacy-only-comment-url"),
26+
},
27+
URL: new("legacy-only-thread-url"),
28+
}
29+
deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
30+
GetNotifications: mockResponse(t, http.StatusOK, []*github.Notification{notification}),
31+
GetReposNotificationsByOwnerByRepo: mockResponse(t, http.StatusOK, []*github.Notification{notification}),
32+
GetNotificationsThreadsByThreadID: mockResponse(t, http.StatusOK, notification),
33+
}))}
34+
for _, protocol := range []string{"2025-11-25", inventory.ProtocolVersionMultiRoundTrip} {
35+
server := mcp.NewServer(&mcp.Implementation{Name: "notification-fidelity", Version: "v1"}, nil)
36+
server.AddReceivingMiddleware(InjectDepsMiddleware(deps))
37+
for _, tool := range []inventory.ServerTool{
38+
ListNotifications(translations.NullTranslationHelper),
39+
GetNotificationDetails(translations.NullTranslationHelper),
40+
} {
41+
tool.RegisterFunc(server, deps)
42+
}
43+
client := connectCommentVisibilityClient(t, server, protocol)
44+
for _, call := range []mcp.CallToolParams{
45+
{Name: "list_notifications", Arguments: map[string]any{}},
46+
{Name: "list_notifications", Arguments: map[string]any{"owner": "owner", "repo": "repo"}},
47+
{Name: "get_notification_details", Arguments: map[string]any{"notificationID": "123"}},
48+
} {
49+
result, err := client.CallTool(context.Background(), &call)
50+
require.NoError(t, err)
51+
require.False(t, result.IsError, "%s", getTextResult(t, result).Text)
52+
if protocol == "2025-11-25" {
53+
var expected any = notification
54+
if call.Name == "list_notifications" {
55+
expected = []*github.Notification{notification}
56+
}
57+
assert.Equal(t, mustMarshalJSON(t, expected), getTextResult(t, result).Text)
58+
assert.Nil(t, result.StructuredContent)
59+
continue
60+
}
61+
require.NotNil(t, result.StructuredContent)
62+
text := mustMarshalJSON(t, result.StructuredContent)
63+
assert.JSONEq(t, text, getTextResult(t, result).Text)
64+
assert.Contains(t, text, `"reason":"agent_session_finished"`)
65+
assert.Contains(t, text, `"type":"`+subjectType+`"`)
66+
assert.Contains(t, text, `"url":"`+notification.Subject.GetURL()+`"`)
67+
assert.NotContains(t, text, "legacy-only")
68+
resolved, err := notificationOutputSchema(call.Name == "list_notifications").Resolve(nil)
69+
require.NoError(t, err)
70+
var output any
71+
require.NoError(t, json.Unmarshal([]byte(text), &output))
72+
require.NoError(t, resolved.Validate(output))
73+
}
74+
}
75+
})
76+
}
77+
}

0 commit comments

Comments
 (0)