Skip to content

Commit 32de15f

Browse files
tommaso-morotommaso-moro
committed
Merge branch 'main' into tommy/bug-fix-invalid-tool-should-error-out
2 parents 0c00ca4 + 87a1110 commit 32de15f

File tree

105 files changed

+1386
-1243
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

105 files changed

+1386
-1243
lines changed

‎.gitignore‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ bin/
1818

1919
# binary
2020
github-mcp-server
21+
mcpcurl
22+
e2e.test
2123

2224
.history
2325
conformance-report/

‎cmd/mcpcurl/mcpcurl‎

-6.41 MB
Binary file not shown.

‎e2e.test‎

-15.2 MB
Binary file not shown.

‎internal/toolsnaps/toolsnaps.go‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,15 +67,35 @@ func Test(toolName string, tool any) error {
6767
}
6868

6969
func writeSnap(snapPath string, contents []byte) error {
70+
// Sort the JSON keys recursively to ensure consistent output.
71+
// We do this by unmarshaling and remarshaling, which ensures Go's JSON encoder
72+
// sorts all map keys alphabetically at every level.
73+
sortedJSON, err := sortJSONKeys(contents)
74+
if err != nil {
75+
return fmt.Errorf("failed to sort JSON keys: %w", err)
76+
}
77+
7078
// Ensure the directory exists
7179
if err := os.MkdirAll(filepath.Dir(snapPath), 0700); err != nil {
7280
return fmt.Errorf("failed to create snapshot directory: %w", err)
7381
}
7482

7583
// Write the snapshot file
76-
if err := os.WriteFile(snapPath, contents, 0600); err != nil {
84+
if err := os.WriteFile(snapPath, sortedJSON, 0600); err != nil {
7785
return fmt.Errorf("failed to write snapshot file: %w", err)
7886
}
7987

8088
return nil
8189
}
90+
91+
// sortJSONKeys recursively sorts all object keys in a JSON byte array by
92+
// unmarshaling to map[string]any and remarshaling. Go's JSON encoder
93+
// automatically sorts map keys alphabetically.
94+
func sortJSONKeys(jsonData []byte) ([]byte, error) {
95+
var data any
96+
if err := json.Unmarshal(jsonData, &data); err != nil {
97+
return nil, err
98+
}
99+
100+
return json.MarshalIndent(data, "", " ")
101+
}

‎internal/toolsnaps/toolsnaps_test.go‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,3 +131,124 @@ func TestMalformedSnapshotJSON(t *testing.T) {
131131
require.Error(t, err)
132132
assert.Contains(t, err.Error(), "failed to parse snapshot JSON for dummy", "expected error about malformed snapshot JSON")
133133
}
134+
135+
func TestSortJSONKeys(t *testing.T) {
136+
tests := []struct {
137+
name string
138+
input string
139+
expected string
140+
}{
141+
{
142+
name: "simple object",
143+
input: `{"z": 1, "a": 2, "m": 3}`,
144+
expected: "{\n \"a\": 2,\n \"m\": 3,\n \"z\": 1\n}",
145+
},
146+
{
147+
name: "nested object",
148+
input: `{"z": {"y": 1, "x": 2}, "a": 3}`,
149+
expected: "{\n \"a\": 3,\n \"z\": {\n \"x\": 2,\n \"y\": 1\n }\n}",
150+
},
151+
{
152+
name: "array with objects",
153+
input: `{"items": [{"z": 1, "a": 2}, {"y": 3, "b": 4}]}`,
154+
expected: "{\n \"items\": [\n {\n \"a\": 2,\n \"z\": 1\n },\n {\n \"b\": 4,\n \"y\": 3\n }\n ]\n}",
155+
},
156+
{
157+
name: "deeply nested",
158+
input: `{"z": {"y": {"x": 1, "a": 2}, "b": 3}, "m": 4}`,
159+
expected: "{\n \"m\": 4,\n \"z\": {\n \"b\": 3,\n \"y\": {\n \"a\": 2,\n \"x\": 1\n }\n }\n}",
160+
},
161+
{
162+
name: "properties field like in toolsnaps",
163+
input: `{"name": "test", "properties": {"repo": {"type": "string"}, "owner": {"type": "string"}, "page": {"type": "number"}}}`,
164+
expected: "{\n \"name\": \"test\",\n \"properties\": {\n \"owner\": {\n \"type\": \"string\"\n },\n \"page\": {\n \"type\": \"number\"\n },\n \"repo\": {\n \"type\": \"string\"\n }\n }\n}",
165+
},
166+
}
167+
168+
for _, tt := range tests {
169+
t.Run(tt.name, func(t *testing.T) {
170+
result, err := sortJSONKeys([]byte(tt.input))
171+
require.NoError(t, err)
172+
assert.Equal(t, tt.expected, string(result))
173+
})
174+
}
175+
}
176+
177+
func TestSortJSONKeysIdempotent(t *testing.T) {
178+
// Given a JSON string that's already sorted
179+
input := `{"a": 1, "b": {"x": 2, "y": 3}, "c": [{"m": 4, "n": 5}]}`
180+
181+
// When we sort it once
182+
sorted1, err := sortJSONKeys([]byte(input))
183+
require.NoError(t, err)
184+
185+
// And sort it again
186+
sorted2, err := sortJSONKeys(sorted1)
187+
require.NoError(t, err)
188+
189+
// Then the results should be identical
190+
assert.Equal(t, string(sorted1), string(sorted2))
191+
}
192+
193+
func TestToolSnapKeysSorted(t *testing.T) {
194+
withIsolatedWorkingDir(t)
195+
196+
// Given a tool with fields that could be in any order
197+
type complexTool struct {
198+
Name string `json:"name"`
199+
Description string `json:"description"`
200+
Properties map[string]interface{} `json:"properties"`
201+
Annotations map[string]interface{} `json:"annotations"`
202+
}
203+
204+
tool := complexTool{
205+
Name: "test_tool",
206+
Description: "A test tool",
207+
Properties: map[string]interface{}{
208+
"zzz": "last",
209+
"aaa": "first",
210+
"mmm": "middle",
211+
"owner": map[string]interface{}{"type": "string", "description": "Owner"},
212+
"repo": map[string]interface{}{"type": "string", "description": "Repo"},
213+
},
214+
Annotations: map[string]interface{}{
215+
"readOnly": true,
216+
"title": "Test",
217+
},
218+
}
219+
220+
// When we write the snapshot
221+
t.Setenv("UPDATE_TOOLSNAPS", "true")
222+
err := Test("complex", tool)
223+
require.NoError(t, err)
224+
225+
// Then the snapshot file should have sorted keys
226+
snapJSON, err := os.ReadFile("__toolsnaps__/complex.snap")
227+
require.NoError(t, err)
228+
229+
// Verify that the JSON is properly sorted by checking key order
230+
var parsed map[string]interface{}
231+
err = json.Unmarshal(snapJSON, &parsed)
232+
require.NoError(t, err)
233+
234+
// Check that properties are sorted
235+
propsJSON, _ := json.MarshalIndent(parsed["properties"], "", " ")
236+
propsStr := string(propsJSON)
237+
// The properties should have "aaa" before "mmm" before "zzz"
238+
aaaIndex := -1
239+
mmmIndex := -1
240+
zzzIndex := -1
241+
for i, line := range propsStr {
242+
if line == 'a' && i+2 < len(propsStr) && propsStr[i:i+3] == "aaa" {
243+
aaaIndex = i
244+
}
245+
if line == 'm' && i+2 < len(propsStr) && propsStr[i:i+3] == "mmm" {
246+
mmmIndex = i
247+
}
248+
if line == 'z' && i+2 < len(propsStr) && propsStr[i:i+3] == "zzz" {
249+
zzzIndex = i
250+
}
251+
}
252+
assert.Greater(t, mmmIndex, aaaIndex, "mmm should come after aaa")
253+
assert.Greater(t, zzzIndex, mmmIndex, "zzz should come after mmm")
254+
}

‎pkg/github/__toolsnaps__/actions_get.snap‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,8 @@
55
},
66
"description": "Get details about specific GitHub Actions resources.\nUse this tool to get details about individual workflows, workflow runs, jobs, and artifacts by their unique IDs.\n",
77
"inputSchema": {
8-
"type": "object",
98
"properties": {
109
"method": {
11-
"type": "string",
1210
"description": "The method to execute",
1311
"enum": [
1412
"get_workflow",
@@ -17,27 +15,29 @@
1715
"download_workflow_run_artifact",
1816
"get_workflow_run_usage",
1917
"get_workflow_run_logs_url"
20-
]
18+
],
19+
"type": "string"
2120
},
2221
"owner": {
23-
"type": "string",
24-
"description": "Repository owner"
22+
"description": "Repository owner",
23+
"type": "string"
2524
},
2625
"repo": {
27-
"type": "string",
28-
"description": "Repository name"
26+
"description": "Repository name",
27+
"type": "string"
2928
},
3029
"resource_id": {
31-
"type": "string",
32-
"description": "The unique identifier of the resource. This will vary based on the \"method\" provided, so ensure you provide the correct ID:\n- Provide a workflow ID or workflow file name (e.g. ci.yaml) for 'get_workflow' method.\n- Provide a workflow run ID for 'get_workflow_run', 'get_workflow_run_usage', and 'get_workflow_run_logs_url' methods.\n- Provide an artifact ID for 'download_workflow_run_artifact' method.\n- Provide a job ID for 'get_workflow_job' method.\n"
30+
"description": "The unique identifier of the resource. This will vary based on the \"method\" provided, so ensure you provide the correct ID:\n- Provide a workflow ID or workflow file name (e.g. ci.yaml) for 'get_workflow' method.\n- Provide a workflow run ID for 'get_workflow_run', 'get_workflow_run_usage', and 'get_workflow_run_logs_url' methods.\n- Provide an artifact ID for 'download_workflow_run_artifact' method.\n- Provide a job ID for 'get_workflow_job' method.\n",
31+
"type": "string"
3332
}
3433
},
3534
"required": [
3635
"method",
3736
"owner",
3837
"repo",
3938
"resource_id"
40-
]
39+
],
40+
"type": "object"
4141
},
4242
"name": "actions_get"
4343
}

‎pkg/github/__toolsnaps__/actions_list.snap‎

Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -5,68 +5,66 @@
55
},
66
"description": "Tools for listing GitHub Actions resources.\nUse this tool to list workflows in a repository, or list workflow runs, jobs, and artifacts for a specific workflow or workflow run.\n",
77
"inputSchema": {
8-
"type": "object",
98
"properties": {
109
"method": {
11-
"type": "string",
1210
"description": "The action to perform",
1311
"enum": [
1412
"list_workflows",
1513
"list_workflow_runs",
1614
"list_workflow_jobs",
1715
"list_workflow_run_artifacts"
18-
]
16+
],
17+
"type": "string"
1918
},
2019
"owner": {
21-
"type": "string",
22-
"description": "Repository owner"
20+
"description": "Repository owner",
21+
"type": "string"
2322
},
2423
"page": {
25-
"type": "number",
2624
"description": "Page number for pagination (default: 1)",
27-
"minimum": 1
25+
"minimum": 1,
26+
"type": "number"
2827
},
2928
"per_page": {
30-
"type": "number",
3129
"description": "Results per page for pagination (default: 30, max: 100)",
30+
"maximum": 100,
3231
"minimum": 1,
33-
"maximum": 100
32+
"type": "number"
3433
},
3534
"repo": {
36-
"type": "string",
37-
"description": "Repository name"
35+
"description": "Repository name",
36+
"type": "string"
3837
},
3938
"resource_id": {
40-
"type": "string",
41-
"description": "The unique identifier of the resource. This will vary based on the \"method\" provided, so ensure you provide the correct ID:\n- Do not provide any resource ID for 'list_workflows' method.\n- Provide a workflow ID or workflow file name (e.g. ci.yaml) for 'list_workflow_runs' method, or omit to list all workflow runs in the repository.\n- Provide a workflow run ID for 'list_workflow_jobs' and 'list_workflow_run_artifacts' methods.\n"
39+
"description": "The unique identifier of the resource. This will vary based on the \"method\" provided, so ensure you provide the correct ID:\n- Do not provide any resource ID for 'list_workflows' method.\n- Provide a workflow ID or workflow file name (e.g. ci.yaml) for 'list_workflow_runs' method, or omit to list all workflow runs in the repository.\n- Provide a workflow run ID for 'list_workflow_jobs' and 'list_workflow_run_artifacts' methods.\n",
40+
"type": "string"
4241
},
4342
"workflow_jobs_filter": {
44-
"type": "object",
43+
"description": "Filters for workflow jobs. **ONLY** used when method is 'list_workflow_jobs'",
4544
"properties": {
4645
"filter": {
47-
"type": "string",
4846
"description": "Filters jobs by their completed_at timestamp",
4947
"enum": [
5048
"latest",
5149
"all"
52-
]
50+
],
51+
"type": "string"
5352
}
5453
},
55-
"description": "Filters for workflow jobs. **ONLY** used when method is 'list_workflow_jobs'"
54+
"type": "object"
5655
},
5756
"workflow_runs_filter": {
58-
"type": "object",
57+
"description": "Filters for workflow runs. **ONLY** used when method is 'list_workflow_runs'",
5958
"properties": {
6059
"actor": {
61-
"type": "string",
62-
"description": "Filter to a specific GitHub user's workflow runs."
60+
"description": "Filter to a specific GitHub user's workflow runs.",
61+
"type": "string"
6362
},
6463
"branch": {
65-
"type": "string",
66-
"description": "Filter workflow runs to a specific Git branch. Use the name of the branch."
64+
"description": "Filter workflow runs to a specific Git branch. Use the name of the branch.",
65+
"type": "string"
6766
},
6867
"event": {
69-
"type": "string",
7068
"description": "Filter workflow runs to a specific event type",
7169
"enum": [
7270
"branch_protection_rule",
@@ -101,28 +99,30 @@
10199
"workflow_call",
102100
"workflow_dispatch",
103101
"workflow_run"
104-
]
102+
],
103+
"type": "string"
105104
},
106105
"status": {
107-
"type": "string",
108106
"description": "Filter workflow runs to only runs with a specific status",
109107
"enum": [
110108
"queued",
111109
"in_progress",
112110
"completed",
113111
"requested",
114112
"waiting"
115-
]
113+
],
114+
"type": "string"
116115
}
117116
},
118-
"description": "Filters for workflow runs. **ONLY** used when method is 'list_workflow_runs'"
117+
"type": "object"
119118
}
120119
},
121120
"required": [
122121
"method",
123122
"owner",
124123
"repo"
125-
]
124+
],
125+
"type": "object"
126126
},
127127
"name": "actions_list"
128128
}

0 commit comments

Comments
 (0)