Merge branch 'main' into copilot/add-custom-instructions-parameter

Author: SamMorrowDrums

cmd/github-mcp-server/generate_docs.go

@@ -51,7 +51,8 @@ func generateReadmeDocs(readmePath string) error {
 	t, _ := translations.TranslationHelper()
 
 	// (not available to regular users) while including tools with FeatureFlagDisable.
-	r := github.NewInventory(t).WithToolsets([]string{"all"}).Build()
+	// Build() can only fail if WithTools specifies invalid tools - not used here
+	r, _ := github.NewInventory(t).WithToolsets([]string{"all"}).Build()
 
 	// Generate toolsets documentation
 	toolsetsDoc := generateToolsetsDoc(r)
@@ -341,7 +342,8 @@ func generateRemoteToolsetsDoc() string {
 	t, _ := translations.TranslationHelper()
 
 	// Build inventory - stateless
-	r := github.NewInventory(t).Build()
+	// Build() can only fail if WithTools specifies invalid tools - not used here
+	r, _ := github.NewInventory(t).Build()
 
 	// Generate table header (icon is combined with Name column)
 	buf.WriteString("| Name | Description | API URL | 1-Click Install (VS Code) | Read-only Link | 1-Click Read-only Install (VS Code) |\n")

cmd/github-mcp-server/list_scopes.go

@@ -121,7 +121,10 @@ func runListScopes() error {
 		inventoryBuilder = inventoryBuilder.WithTools(enabledTools)
 	}
 
-	inv := inventoryBuilder.Build()
+	inv, err := inventoryBuilder.Build()
+	if err != nil {
+		return fmt.Errorf("failed to build inventory: %w", err)
+	}
 
 	// Collect all tools and their scopes
 	output := collectToolScopes(inv, readOnly)

internal/ghmcp/server.go

@@ -221,15 +221,18 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
 		WithDeprecatedAliases(github.DeprecatedToolAliases).
 		WithReadOnly(cfg.ReadOnly).
 		WithToolsets(enabledToolsets).
-		WithTools(github.CleanTools(cfg.EnabledTools)).
+		WithTools(cfg.EnabledTools).
 		WithFeatureChecker(createFeatureChecker(cfg.EnabledFeatures))
 
 	// Apply token scope filtering if scopes are known (for PAT filtering)
 	if cfg.TokenScopes != nil {
 		inventoryBuilder = inventoryBuilder.WithFilter(github.CreateToolScopeFilter(cfg.TokenScopes))
 	}
 
-	inventory := inventoryBuilder.Build()
+	inventory, err := inventoryBuilder.Build()
+	if err != nil {
+		return nil, fmt.Errorf("failed to build inventory: %w", err)
+	}
 
 	if unrecognized := inventory.UnrecognizedToolsets(); len(unrecognized) > 0 {
 		fmt.Fprintf(os.Stderr, "Warning: unrecognized toolsets ignored: %s\n", strings.Join(unrecognized, ", "))

internal/toolsnaps/toolsnaps.go

@@ -67,15 +67,35 @@ func Test(toolName string, tool any) error {
 }
 
 func writeSnap(snapPath string, contents []byte) error {
+	// Sort the JSON keys recursively to ensure consistent output.
+	// We do this by unmarshaling and remarshaling, which ensures Go's JSON encoder
+	// sorts all map keys alphabetically at every level.
+	sortedJSON, err := sortJSONKeys(contents)
+	if err != nil {
+		return fmt.Errorf("failed to sort JSON keys: %w", err)
+	}
+
 	// Ensure the directory exists
 	if err := os.MkdirAll(filepath.Dir(snapPath), 0700); err != nil {
 		return fmt.Errorf("failed to create snapshot directory: %w", err)
 	}
 
 	// Write the snapshot file
-	if err := os.WriteFile(snapPath, contents, 0600); err != nil {
+	if err := os.WriteFile(snapPath, sortedJSON, 0600); err != nil {
 		return fmt.Errorf("failed to write snapshot file: %w", err)
 	}
 
 	return nil
 }
+
+// sortJSONKeys recursively sorts all object keys in a JSON byte array by
+// unmarshaling to map[string]any and remarshaling. Go's JSON encoder
+// automatically sorts map keys alphabetically.
+func sortJSONKeys(jsonData []byte) ([]byte, error) {
+	var data any
+	if err := json.Unmarshal(jsonData, &data); err != nil {
+		return nil, err
+	}
+
+	return json.MarshalIndent(data, "", "  ")
+}

internal/toolsnaps/toolsnaps_test.go

@@ -131,3 +131,184 @@ func TestMalformedSnapshotJSON(t *testing.T) {
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "failed to parse snapshot JSON for dummy", "expected error about malformed snapshot JSON")
 }
+
+func TestSortJSONKeys(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "simple object",
+			input:    `{"z": 1, "a": 2, "m": 3}`,
+			expected: "{\n  \"a\": 2,\n  \"m\": 3,\n  \"z\": 1\n}",
+		},
+		{
+			name:     "nested object",
+			input:    `{"z": {"y": 1, "x": 2}, "a": 3}`,
+			expected: "{\n  \"a\": 3,\n  \"z\": {\n    \"x\": 2,\n    \"y\": 1\n  }\n}",
+		},
+		{
+			name:     "array with objects",
+			input:    `{"items": [{"z": 1, "a": 2}, {"y": 3, "b": 4}]}`,
+			expected: "{\n  \"items\": [\n    {\n      \"a\": 2,\n      \"z\": 1\n    },\n    {\n      \"b\": 4,\n      \"y\": 3\n    }\n  ]\n}",
+		},
+		{
+			name:     "deeply nested",
+			input:    `{"z": {"y": {"x": 1, "a": 2}, "b": 3}, "m": 4}`,
+			expected: "{\n  \"m\": 4,\n  \"z\": {\n    \"b\": 3,\n    \"y\": {\n      \"a\": 2,\n      \"x\": 1\n    }\n  }\n}",
+		},
+		{
+			name:     "properties field like in toolsnaps",
+			input:    `{"name": "test", "properties": {"repo": {"type": "string"}, "owner": {"type": "string"}, "page": {"type": "number"}}}`,
+			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}",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := sortJSONKeys([]byte(tt.input))
+			require.NoError(t, err)
+			assert.Equal(t, tt.expected, string(result))
+		})
+	}
+}
+
+func TestSortJSONKeysIdempotent(t *testing.T) {
+	// Given a JSON string that's already sorted
+	input := `{"a": 1, "b": {"x": 2, "y": 3}, "c": [{"m": 4, "n": 5}]}`
+
+	// When we sort it once
+	sorted1, err := sortJSONKeys([]byte(input))
+	require.NoError(t, err)
+
+	// And sort it again
+	sorted2, err := sortJSONKeys(sorted1)
+	require.NoError(t, err)
+
+	// Then the results should be identical
+	assert.Equal(t, string(sorted1), string(sorted2))
+}
+
+func TestToolSnapKeysSorted(t *testing.T) {
+	withIsolatedWorkingDir(t)
+
+	// Given a tool with fields that could be in any order
+	type complexTool struct {
+		Name        string                 `json:"name"`
+		Description string                 `json:"description"`
+		Properties  map[string]interface{} `json:"properties"`
+		Annotations map[string]interface{} `json:"annotations"`
+	}
+
+	tool := complexTool{
+		Name:        "test_tool",
+		Description: "A test tool",
+		Properties: map[string]interface{}{
+			"zzz":   "last",
+			"aaa":   "first",
+			"mmm":   "middle",
+			"owner": map[string]interface{}{"type": "string", "description": "Owner"},
+			"repo":  map[string]interface{}{"type": "string", "description": "Repo"},
+		},
+		Annotations: map[string]interface{}{
+			"readOnly": true,
+			"title":    "Test",
+		},
+	}
+
+	// When we write the snapshot
+	t.Setenv("UPDATE_TOOLSNAPS", "true")
+	err := Test("complex", tool)
+	require.NoError(t, err)
+
+	// Then the snapshot file should have sorted keys
+	snapJSON, err := os.ReadFile("__toolsnaps__/complex.snap")
+	require.NoError(t, err)
+
+	// Verify that the JSON is properly sorted by checking key order
+	var parsed map[string]interface{}
+	err = json.Unmarshal(snapJSON, &parsed)
+	require.NoError(t, err)
+
+	// Check that properties are sorted
+	propsJSON, _ := json.MarshalIndent(parsed["properties"], "", "  ")
+	propsStr := string(propsJSON)
+	// The properties should have "aaa" before "mmm" before "zzz"
+	aaaIndex := -1
+	mmmIndex := -1
+	zzzIndex := -1
+	for i, line := range propsStr {
+		if line == 'a' && i+2 < len(propsStr) && propsStr[i:i+3] == "aaa" {
+			aaaIndex = i
+		}
+		if line == 'm' && i+2 < len(propsStr) && propsStr[i:i+3] == "mmm" {
+			mmmIndex = i
+		}
+		if line == 'z' && i+2 < len(propsStr) && propsStr[i:i+3] == "zzz" {
+			zzzIndex = i
+		}
+	}
+	assert.Greater(t, mmmIndex, aaaIndex, "mmm should come after aaa")
+	assert.Greater(t, zzzIndex, mmmIndex, "zzz should come after mmm")
+}
+
+func TestStructFieldOrderingSortedAlphabetically(t *testing.T) {
+	withIsolatedWorkingDir(t)
+
+	// Given a struct with fields defined in non-alphabetical order
+	// This test ensures that struct field order doesn't affect the JSON output
+	type toolWithNonAlphabeticalFields struct {
+		ZField string `json:"zField"` // Should appear last in JSON
+		AField string `json:"aField"` // Should appear first in JSON
+		MField string `json:"mField"` // Should appear in the middle
+	}
+
+	tool := toolWithNonAlphabeticalFields{
+		ZField: "z value",
+		AField: "a value",
+		MField: "m value",
+	}
+
+	// When we write the snapshot
+	t.Setenv("UPDATE_TOOLSNAPS", "true")
+	err := Test("struct_field_order", tool)
+	require.NoError(t, err)
+
+	// Then the snapshot file should have alphabetically sorted keys despite struct field order
+	snapJSON, err := os.ReadFile("__toolsnaps__/struct_field_order.snap")
+	require.NoError(t, err)
+
+	snapStr := string(snapJSON)
+
+	// Find the positions of each field in the JSON string
+	aFieldIndex := -1
+	mFieldIndex := -1
+	zFieldIndex := -1
+	for i := 0; i < len(snapStr)-7; i++ {
+		switch snapStr[i : i+6] {
+		case "aField":
+			aFieldIndex = i
+		case "mField":
+			mFieldIndex = i
+		case "zField":
+			zFieldIndex = i
+		}
+	}
+
+	// Verify alphabetical ordering in the JSON output
+	require.NotEqual(t, -1, aFieldIndex, "aField should be present")
+	require.NotEqual(t, -1, mFieldIndex, "mField should be present")
+	require.NotEqual(t, -1, zFieldIndex, "zField should be present")
+	assert.Less(t, aFieldIndex, mFieldIndex, "aField should appear before mField")
+	assert.Less(t, mFieldIndex, zFieldIndex, "mField should appear before zField")
+
+	// Also verify idempotency - running the test again should produce identical output
+	err = Test("struct_field_order", tool)
+	require.NoError(t, err)
+
+	snapJSON2, err := os.ReadFile("__toolsnaps__/struct_field_order.snap")
+	require.NoError(t, err)
+
+	assert.Equal(t, string(snapJSON), string(snapJSON2), "Multiple runs should produce identical output")
+}

pkg/github/__toolsnaps__/actions_get.snap

@@ -5,16 +5,8 @@
   },
   "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",
   "inputSchema": {
-    "type": "object",
-    "required": [
-      "method",
-      "owner",
-      "repo",
-      "resource_id"
-    ],
     "properties": {
       "method": {
-        "type": "string",
         "description": "The method to execute",
         "enum": [
           "get_workflow",
@@ -23,21 +15,29 @@
           "download_workflow_run_artifact",
           "get_workflow_run_usage",
           "get_workflow_run_logs_url"
-        ]
+        ],
+        "type": "string"
       },
       "owner": {
-        "type": "string",
-        "description": "Repository owner"
+        "description": "Repository owner",
+        "type": "string"
       },
       "repo": {
-        "type": "string",
-        "description": "Repository name"
+        "description": "Repository name",
+        "type": "string"
       },
       "resource_id": {
-        "type": "string",
-        "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"
+        "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",
+        "type": "string"
       }
-    }
+    },
+    "required": [
+      "method",
+      "owner",
+      "repo",
+      "resource_id"
+    ],
+    "type": "object"
   },
   "name": "actions_get"
 }