Merge branch 'main' into prcomments

Author: juruen

.github/CODEOWNERS

@@ -1 +1 @@
-@juruen @sammorrowdrums @williammartin @toby
+* @juruen @sammorrowdrums @williammartin @toby

README.md

@@ -15,7 +15,8 @@ automation and interaction capabilities for developers and tools.
 ## Prerequisites
 
 1. To run the server in a container, you will need to have [Docker](https://www.docker.com/) installed.
-2. [Create a GitHub Personal Access Token](https://github.com/settings/personal-access-tokens/new).
+2. Once Docker is installed, you will also need to ensure Docker is running.
+3. Lastly you will need to [Create a GitHub Personal Access Token](https://github.com/settings/personal-access-tokens/new).
 The MCP server can use many of the GitHub APIs, so enable the permissions that you feel comfortable granting your AI tools (to learn more about access tokens, please check out the [documentation](https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens)).
 
 
@@ -310,6 +311,17 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
   - `comment_id`: The unique identifier of the comment to reply to (number, required)
   - `body`: The text of the reply comment (string, required)
 
+- **update_pull_request** - Update an existing pull request in a GitHub repository
+
+  - `owner`: Repository owner (string, required)
+  - `repo`: Repository name (string, required)
+  - `pullNumber`: Pull request number to update (number, required)
+  - `title`: New title (string, optional)
+  - `body`: New description (string, optional)
+  - `state`: New state ('open' or 'closed') (string, optional)
+  - `base`: New base branch name (string, optional)
+  - `maintainer_can_modify`: Allow maintainer edits (boolean, optional)
+
 ### Repositories
 
 - **create_or_update_file** - Create or update a single file in a repository
@@ -365,14 +377,21 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
   - `branch`: New branch name (string, required)
   - `sha`: SHA to create branch from (string, required)
 
-- **list_commits** - Gets commits of a branch in a repository
+- **list_commits** - Get a list of commits of a branch in a repository
   - `owner`: Repository owner (string, required)
   - `repo`: Repository name (string, required)
   - `sha`: Branch name, tag, or commit SHA (string, optional)
   - `path`: Only commits containing this file path (string, optional)
   - `page`: Page number (number, optional)
   - `perPage`: Results per page (number, optional)
 
+- **get_commit** - Get details for a commit from a repository
+  - `owner`: Repository owner (string, required)
+  - `repo`: Repository name (string, required)
+  - `sha`: Commit SHA, branch name, or tag name (string, required)
+  - `page`: Page number, for files in the commit (number, optional)
+  - `perPage`: Results per page, for files in the commit (number, optional)
+
 ### Search
 
 - **search_code** - Search for code across GitHub repositories

cmd/github-mcp-server/main.go

@@ -1,9 +1,7 @@
 package main
 
 import (
-	"bytes"
 	"context"
-	"encoding/json"
 	"fmt"
 	"io"
 	stdlog "log"
@@ -41,7 +39,6 @@ var (
 			logFile := viper.GetString("log-file")
 			readOnly := viper.GetBool("read-only")
 			exportTranslations := viper.GetBool("export-translations")
-			prettyPrintJSON := viper.GetBool("pretty-print-json")
 			logger, err := initLogger(logFile)
 			if err != nil {
 				stdlog.Fatal("Failed to initialize logger:", err)
@@ -52,7 +49,6 @@ var (
 				logger:             logger,
 				logCommands:        logCommands,
 				exportTranslations: exportTranslations,
-				prettyPrintJSON:    prettyPrintJSON,
 			}
 			if err := runStdioServer(cfg); err != nil {
 				stdlog.Fatal("failed to run stdio server:", err)
@@ -70,15 +66,13 @@ func init() {
 	rootCmd.PersistentFlags().Bool("enable-command-logging", false, "When enabled, the server will log all command requests and responses to the log file")
 	rootCmd.PersistentFlags().Bool("export-translations", false, "Save translations to a JSON file")
 	rootCmd.PersistentFlags().String("gh-host", "", "Specify the GitHub hostname (for GitHub Enterprise etc.)")
-	rootCmd.PersistentFlags().Bool("pretty-print-json", false, "Pretty print JSON output")
 
 	// Bind flag to viper
 	_ = viper.BindPFlag("read-only", rootCmd.PersistentFlags().Lookup("read-only"))
 	_ = viper.BindPFlag("log-file", rootCmd.PersistentFlags().Lookup("log-file"))
 	_ = viper.BindPFlag("enable-command-logging", rootCmd.PersistentFlags().Lookup("enable-command-logging"))
 	_ = viper.BindPFlag("export-translations", rootCmd.PersistentFlags().Lookup("export-translations"))
 	_ = viper.BindPFlag("gh-host", rootCmd.PersistentFlags().Lookup("gh-host"))
-	_ = viper.BindPFlag("pretty-print-json", rootCmd.PersistentFlags().Lookup("pretty-print-json"))
 
 	// Add subcommands
 	rootCmd.AddCommand(stdioCmd)
@@ -112,20 +106,6 @@ type runConfig struct {
 	logger             *log.Logger
 	logCommands        bool
 	exportTranslations bool
-	prettyPrintJSON    bool
-}
-
-// JSONPrettyPrintWriter is a Writer that pretty prints input to indented JSON
-type JSONPrettyPrintWriter struct {
-	writer io.Writer
-}
-
-func (j JSONPrettyPrintWriter) Write(p []byte) (n int, err error) {
-	var prettyJSON bytes.Buffer
-	if err := json.Indent(&prettyJSON, p, "", "\t"); err != nil {
-		return 0, err
-	}
-	return j.writer.Write(prettyJSON.Bytes())
 }
 
 func runStdioServer(cfg runConfig) error {
@@ -157,8 +137,11 @@ func runStdioServer(cfg runConfig) error {
 
 	t, dumpTranslations := translations.TranslationHelper()
 
+	getClient := func(_ context.Context) (*gogithub.Client, error) {
+		return ghClient, nil // closing over client
+	}
 	// Create
-	ghServer := github.NewServer(ghClient, version, cfg.readOnly, t)
+	ghServer := github.NewServer(getClient, version, cfg.readOnly, t)
 	stdioServer := server.NewStdioServer(ghServer)
 
 	stdLogger := stdlog.New(cfg.logger.Writer(), "stdioserver", 0)
@@ -179,9 +162,6 @@ func runStdioServer(cfg runConfig) error {
 			in, out = loggedIO, loggedIO
 		}
 
-		if cfg.prettyPrintJSON {
-			out = JSONPrettyPrintWriter{writer: out}
-		}
 		errC <- stdioServer.Listen(ctx, in, out)
 	}()
 

pkg/github/code_scanning.go

@@ -13,7 +13,7 @@ import (
 	"github.com/mark3labs/mcp-go/server"
 )
 
-func GetCodeScanningAlert(client *github.Client, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {
+func GetCodeScanningAlert(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {
 	return mcp.NewTool("get_code_scanning_alert",
 			mcp.WithDescription(t("TOOL_GET_CODE_SCANNING_ALERT_DESCRIPTION", "Get details of a specific code scanning alert in a GitHub repository.")),
 			mcp.WithString("owner",
@@ -43,6 +43,11 @@ func GetCodeScanningAlert(client *github.Client, t translations.TranslationHelpe
 				return mcp.NewToolResultError(err.Error()), nil
 			}
 
+			client, err := getClient(ctx)
+			if err != nil {
+				return nil, fmt.Errorf("failed to get GitHub client: %w", err)
+			}
+
 			alert, resp, err := client.CodeScanning.GetAlert(ctx, owner, repo, int64(alertNumber))
 			if err != nil {
 				return nil, fmt.Errorf("failed to get alert: %w", err)
@@ -66,7 +71,7 @@ func GetCodeScanningAlert(client *github.Client, t translations.TranslationHelpe
 		}
 }
 
-func ListCodeScanningAlerts(client *github.Client, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {
+func ListCodeScanningAlerts(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {
 	return mcp.NewTool("list_code_scanning_alerts",
 			mcp.WithDescription(t("TOOL_LIST_CODE_SCANNING_ALERTS_DESCRIPTION", "List code scanning alerts in a GitHub repository.")),
 			mcp.WithString("owner",
@@ -110,6 +115,10 @@ func ListCodeScanningAlerts(client *github.Client, t translations.TranslationHel
 				return mcp.NewToolResultError(err.Error()), nil
 			}
 
+			client, err := getClient(ctx)
+			if err != nil {
+				return nil, fmt.Errorf("failed to get GitHub client: %w", err)
+			}
 			alerts, resp, err := client.CodeScanning.ListAlertsForRepo(ctx, owner, repo, &github.AlertListOptions{Ref: ref, State: state, Severity: severity})
 			if err != nil {
 				return nil, fmt.Errorf("failed to list alerts: %w", err)

pkg/github/code_scanning_test.go

@@ -16,7 +16,7 @@ import (
 func Test_GetCodeScanningAlert(t *testing.T) {
 	// Verify tool definition once
 	mockClient := github.NewClient(nil)
-	tool, _ := GetCodeScanningAlert(mockClient, translations.NullTranslationHelper)
+	tool, _ := GetCodeScanningAlert(stubGetClientFn(mockClient), translations.NullTranslationHelper)
 
 	assert.Equal(t, "get_code_scanning_alert", tool.Name)
 	assert.NotEmpty(t, tool.Description)
@@ -82,7 +82,7 @@ func Test_GetCodeScanningAlert(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			// Setup client with mock
 			client := github.NewClient(tc.mockedClient)
-			_, handler := GetCodeScanningAlert(client, translations.NullTranslationHelper)
+			_, handler := GetCodeScanningAlert(stubGetClientFn(client), translations.NullTranslationHelper)
 
 			// Create call request
 			request := createMCPRequest(tc.requestArgs)
@@ -118,7 +118,7 @@ func Test_GetCodeScanningAlert(t *testing.T) {
 func Test_ListCodeScanningAlerts(t *testing.T) {
 	// Verify tool definition once
 	mockClient := github.NewClient(nil)
-	tool, _ := ListCodeScanningAlerts(mockClient, translations.NullTranslationHelper)
+	tool, _ := ListCodeScanningAlerts(stubGetClientFn(mockClient), translations.NullTranslationHelper)
 
 	assert.Equal(t, "list_code_scanning_alerts", tool.Name)
 	assert.NotEmpty(t, tool.Description)
@@ -201,7 +201,7 @@ func Test_ListCodeScanningAlerts(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			// Setup client with mock
 			client := github.NewClient(tc.mockedClient)
-			_, handler := ListCodeScanningAlerts(client, translations.NullTranslationHelper)
+			_, handler := ListCodeScanningAlerts(stubGetClientFn(client), translations.NullTranslationHelper)
 
 			// Create call request
 			request := createMCPRequest(tc.requestArgs)

pkg/github/helper_test.go

@@ -93,3 +93,115 @@ func getTextResult(t *testing.T, result *mcp.CallToolResult) mcp.TextContent {
 	assert.Equal(t, "text", textContent.Type)
 	return textContent
 }
+
+func TestOptionalParamOK(t *testing.T) {
+	tests := []struct {
+		name        string
+		args        map[string]interface{}
+		paramName   string
+		expectedVal interface{}
+		expectedOk  bool
+		expectError bool
+		errorMsg    string
+	}{
+		{
+			name:        "present and correct type (string)",
+			args:        map[string]interface{}{"myParam": "hello"},
+			paramName:   "myParam",
+			expectedVal: "hello",
+			expectedOk:  true,
+			expectError: false,
+		},
+		{
+			name:        "present and correct type (bool)",
+			args:        map[string]interface{}{"myParam": true},
+			paramName:   "myParam",
+			expectedVal: true,
+			expectedOk:  true,
+			expectError: false,
+		},
+		{
+			name:        "present and correct type (number)",
+			args:        map[string]interface{}{"myParam": float64(123)},
+			paramName:   "myParam",
+			expectedVal: float64(123),
+			expectedOk:  true,
+			expectError: false,
+		},
+		{
+			name:        "present but wrong type (string expected, got bool)",
+			args:        map[string]interface{}{"myParam": true},
+			paramName:   "myParam",
+			expectedVal: "",   // Zero value for string
+			expectedOk:  true, // ok is true because param exists
+			expectError: true,
+			errorMsg:    "parameter myParam is not of type string, is bool",
+		},
+		{
+			name:        "present but wrong type (bool expected, got string)",
+			args:        map[string]interface{}{"myParam": "true"},
+			paramName:   "myParam",
+			expectedVal: false, // Zero value for bool
+			expectedOk:  true,  // ok is true because param exists
+			expectError: true,
+			errorMsg:    "parameter myParam is not of type bool, is string",
+		},
+		{
+			name:        "parameter not present",
+			args:        map[string]interface{}{"anotherParam": "value"},
+			paramName:   "myParam",
+			expectedVal: "", // Zero value for string
+			expectedOk:  false,
+			expectError: false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			request := createMCPRequest(tc.args)
+
+			// Test with string type assertion
+			if _, isString := tc.expectedVal.(string); isString || tc.errorMsg == "parameter myParam is not of type string, is bool" {
+				val, ok, err := OptionalParamOK[string](request, tc.paramName)
+				if tc.expectError {
+					require.Error(t, err)
+					assert.Contains(t, err.Error(), tc.errorMsg)
+					assert.Equal(t, tc.expectedOk, ok)   // Check ok even on error
+					assert.Equal(t, tc.expectedVal, val) // Check zero value on error
+				} else {
+					require.NoError(t, err)
+					assert.Equal(t, tc.expectedOk, ok)
+					assert.Equal(t, tc.expectedVal, val)
+				}
+			}
+
+			// Test with bool type assertion
+			if _, isBool := tc.expectedVal.(bool); isBool || tc.errorMsg == "parameter myParam is not of type bool, is string" {
+				val, ok, err := OptionalParamOK[bool](request, tc.paramName)
+				if tc.expectError {
+					require.Error(t, err)
+					assert.Contains(t, err.Error(), tc.errorMsg)
+					assert.Equal(t, tc.expectedOk, ok)   // Check ok even on error
+					assert.Equal(t, tc.expectedVal, val) // Check zero value on error
+				} else {
+					require.NoError(t, err)
+					assert.Equal(t, tc.expectedOk, ok)
+					assert.Equal(t, tc.expectedVal, val)
+				}
+			}
+
+			// Test with float64 type assertion (for number case)
+			if _, isFloat := tc.expectedVal.(float64); isFloat {
+				val, ok, err := OptionalParamOK[float64](request, tc.paramName)
+				if tc.expectError {
+					// This case shouldn't happen for float64 in the defined tests
+					require.Fail(t, "Unexpected error case for float64")
+				} else {
+					require.NoError(t, err)
+					assert.Equal(t, tc.expectedOk, ok)
+					assert.Equal(t, tc.expectedVal, val)
+				}
+			}
+		})
+	}
+}