Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions cmd/github-mcp-server/generate_docs.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (

"github.com/github/github-mcp-server/pkg/github"
"github.com/github/github-mcp-server/pkg/lockdown"
"github.com/github/github-mcp-server/pkg/observability"
"github.com/github/github-mcp-server/pkg/observability/log"
"github.com/github/github-mcp-server/pkg/raw"
"github.com/github/github-mcp-server/pkg/toolsets"
"github.com/github/github-mcp-server/pkg/translations"
Expand Down Expand Up @@ -67,7 +69,9 @@ func generateReadmeDocs(readmePath string) error {

// Create toolset group with mock clients
repoAccessCache := lockdown.GetInstance(nil)
tsg := github.DefaultToolsetGroup(false, mockGetClient, mockGetGQLClient, mockGetRawClient, t, 5000, github.FeatureFlags{}, repoAccessCache)
nilLogger := log.NewNoopLogger()
nilObsv := observability.NewExporters(nilLogger)
tsg := github.DefaultToolsetGroup(false, mockGetClient, mockGetGQLClient, mockGetRawClient, t, 5000, github.FeatureFlags{}, repoAccessCache, nilObsv)

// Generate toolsets documentation
toolsetsDoc := generateToolsetsDoc(tsg)
Expand Down Expand Up @@ -323,7 +327,9 @@ func generateRemoteToolsetsDoc() string {

// Create toolset group with mock clients
repoAccessCache := lockdown.GetInstance(nil)
tsg := github.DefaultToolsetGroup(false, mockGetClient, mockGetGQLClient, mockGetRawClient, t, 5000, github.FeatureFlags{}, repoAccessCache)
nilLogger := log.NewNoopLogger()
nilObsv := observability.NewExporters(nilLogger)
tsg := github.DefaultToolsetGroup(false, mockGetClient, mockGetGQLClient, mockGetRawClient, t, 5000, github.FeatureFlags{}, repoAccessCache, nilObsv)

// Generate table header
buf.WriteString("| Name | Description | API URL | 1-Click Install (VS Code) | Read-only Link | 1-Click Read-only Install (VS Code) |\n")
Expand Down
6 changes: 6 additions & 0 deletions internal/ghmcp/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import (
"github.com/github/github-mcp-server/pkg/github"
"github.com/github/github-mcp-server/pkg/lockdown"
mcplog "github.com/github/github-mcp-server/pkg/log"
"github.com/github/github-mcp-server/pkg/observability"
"github.com/github/github-mcp-server/pkg/observability/log"
"github.com/github/github-mcp-server/pkg/raw"
"github.com/github/github-mcp-server/pkg/translations"
gogithub "github.com/google/go-github/v79/github"
Expand Down Expand Up @@ -151,6 +153,9 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
ghServer.AddReceivingMiddleware(addGitHubAPIErrorToContext)
ghServer.AddReceivingMiddleware(addUserAgentsMiddleware(cfg, restClient, gqlHTTPClient))

slogAdapter := log.NewSlogLogger(cfg.Logger, log.InfoLevel)
obsv := observability.NewExporters(slogAdapter)

// Create default toolsets
tsg := github.DefaultToolsetGroup(
cfg.ReadOnly,
Expand All @@ -161,6 +166,7 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
cfg.ContentWindowSize,
github.FeatureFlags{LockdownMode: cfg.LockdownMode},
repoAccessCache,
obsv,
)

// Enable and register toolsets if configured
Expand Down
6 changes: 5 additions & 1 deletion pkg/github/issues.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@ import (
"encoding/json"
"fmt"
"io"
"log/slog"
"net/http"
"strings"
"time"

ghErrors "github.com/github/github-mcp-server/pkg/errors"
"github.com/github/github-mcp-server/pkg/lockdown"
"github.com/github/github-mcp-server/pkg/observability"
"github.com/github/github-mcp-server/pkg/sanitize"
"github.com/github/github-mcp-server/pkg/translations"
"github.com/github/github-mcp-server/pkg/utils"
Expand Down Expand Up @@ -229,7 +231,7 @@ func fragmentToIssue(fragment IssueFragment) *github.Issue {
}

// IssueRead creates a tool to get details of a specific issue in a GitHub repository.
func IssueRead(getClient GetClientFn, getGQLClient GetGQLClientFn, cache *lockdown.RepoAccessCache, t translations.TranslationHelperFunc, flags FeatureFlags) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) {
func IssueRead(getClient GetClientFn, getGQLClient GetGQLClientFn, cache *lockdown.RepoAccessCache, t translations.TranslationHelperFunc, flags FeatureFlags, obsv *observability.Exporters) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) {
schema := &jsonschema.Schema{
Type: "object",
Properties: map[string]*jsonschema.Schema{
Expand Down Expand Up @@ -304,6 +306,8 @@ Options are:
return utils.NewToolResultErrorFromErr("failed to get GitHub graphql client", err), nil, nil
}

obsv.Logger.Info("deciding issue read method", slog.String("owner", owner), slog.String("method", method))

switch method {
case "get":
result, err := GetIssue(ctx, client, cache, owner, repo, issueNumber, flags)
Expand Down
43 changes: 35 additions & 8 deletions pkg/github/issues_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import (
"github.com/github/github-mcp-server/internal/githubv4mock"
"github.com/github/github-mcp-server/internal/toolsnaps"
"github.com/github/github-mcp-server/pkg/lockdown"
ghMcpObsv "github.com/github/github-mcp-server/pkg/observability"
ghMcpLogger "github.com/github/github-mcp-server/pkg/observability/log"
"github.com/github/github-mcp-server/pkg/translations"
"github.com/google/go-github/v79/github"
"github.com/google/jsonschema-go/jsonschema"
Expand Down Expand Up @@ -124,7 +126,9 @@ func Test_GetIssue(t *testing.T) {
// Verify tool definition once
mockClient := github.NewClient(nil)
defaultGQLClient := githubv4.NewClient(nil)
tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(defaultGQLClient), repoAccessCache, translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
nilLogger := ghMcpLogger.NewNoopLogger()
obsv := ghMcpObsv.NewExporters(nilLogger)
tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(defaultGQLClient), repoAccessCache, translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}), obsv)
require.NoError(t, toolsnaps.Test(tool.Name, tool))

assert.Equal(t, "issue_read", tool.Name)
Expand Down Expand Up @@ -331,7 +335,7 @@ func Test_GetIssue(t *testing.T) {
}

flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled})
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), cache, translations.NullTranslationHelper, flags)
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), cache, translations.NullTranslationHelper, flags, obsv)

request := createMCPRequest(tc.requestArgs)
result, _, err := handler(context.Background(), &request, tc.requestArgs)
Expand Down Expand Up @@ -1846,7 +1850,11 @@ func Test_GetIssueComments(t *testing.T) {
// Verify tool definition once
mockClient := github.NewClient(nil)
gqlClient := githubv4.NewClient(nil)
tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))

nilLogger := ghMcpLogger.NewNoopLogger()
obsv := ghMcpObsv.NewExporters(nilLogger)

tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}), obsv)
require.NoError(t, toolsnaps.Test(tool.Name, tool))

assert.Equal(t, "issue_read", tool.Name)
Expand Down Expand Up @@ -1997,7 +2005,10 @@ func Test_GetIssueComments(t *testing.T) {
}
cache := stubRepoAccessCache(gqlClient, 15*time.Minute)
flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled})
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), cache, translations.NullTranslationHelper, flags)
nilLogger := ghMcpLogger.NewNoopLogger()
obsv := ghMcpObsv.NewExporters(nilLogger)

_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), cache, translations.NullTranslationHelper, flags, obsv)

// Create call request
request := createMCPRequest(tc.requestArgs)
Expand Down Expand Up @@ -2037,7 +2048,11 @@ func Test_GetIssueLabels(t *testing.T) {
// Verify tool definition
mockGQClient := githubv4.NewClient(nil)
mockClient := github.NewClient(nil)
tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(mockGQClient), stubRepoAccessCache(mockGQClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))

nilLogger := ghMcpLogger.NewNoopLogger()
obsv := ghMcpObsv.NewExporters(nilLogger)

tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(mockGQClient), stubRepoAccessCache(mockGQClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}), obsv)
require.NoError(t, toolsnaps.Test(tool.Name, tool))

assert.Equal(t, "issue_read", tool.Name)
Expand Down Expand Up @@ -2112,7 +2127,11 @@ func Test_GetIssueLabels(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
gqlClient := githubv4.NewClient(tc.mockedClient)
client := github.NewClient(nil)
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))

nilLogger := ghMcpLogger.NewNoopLogger()
obsv := ghMcpObsv.NewExporters(nilLogger)

_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}), obsv)

request := createMCPRequest(tc.requestArgs)
result, _, err := handler(context.Background(), &request, tc.requestArgs)
Expand Down Expand Up @@ -2803,7 +2822,11 @@ func Test_GetSubIssues(t *testing.T) {
// Verify tool definition once
mockClient := github.NewClient(nil)
gqlClient := githubv4.NewClient(nil)
tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))

nilLogger := ghMcpLogger.NewNoopLogger()
obsv := ghMcpObsv.NewExporters(nilLogger)

tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}), obsv)
require.NoError(t, toolsnaps.Test(tool.Name, tool))

assert.Equal(t, "issue_read", tool.Name)
Expand Down Expand Up @@ -3000,7 +3023,11 @@ func Test_GetSubIssues(t *testing.T) {
// Setup client with mock
client := github.NewClient(tc.mockedClient)
gqlClient := githubv4.NewClient(nil)
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))

nilLogger := ghMcpLogger.NewNoopLogger()
obsv := ghMcpObsv.NewExporters(nilLogger)

_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}), obsv)

// Create call request
request := createMCPRequest(tc.requestArgs)
Expand Down
14 changes: 12 additions & 2 deletions pkg/github/tools.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"strings"

"github.com/github/github-mcp-server/pkg/lockdown"
"github.com/github/github-mcp-server/pkg/observability"
"github.com/github/github-mcp-server/pkg/raw"
"github.com/github/github-mcp-server/pkg/toolsets"
"github.com/github/github-mcp-server/pkg/translations"
Expand Down Expand Up @@ -160,7 +161,16 @@ func GetDefaultToolsetIDs() []string {
}
}

func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetGQLClientFn, getRawClient raw.GetRawClientFn, t translations.TranslationHelperFunc, contentWindowSize int, flags FeatureFlags, cache *lockdown.RepoAccessCache) *toolsets.ToolsetGroup {
func DefaultToolsetGroup(readOnly bool,
getClient GetClientFn,
getGQLClient GetGQLClientFn,
getRawClient raw.GetRawClientFn,
t translations.TranslationHelperFunc,
contentWindowSize int,
flags FeatureFlags,
cache *lockdown.RepoAccessCache,
obsv *observability.Exporters,
) *toolsets.ToolsetGroup {
tsg := toolsets.NewToolsetGroup(readOnly)

// Define all available features with their default state (disabled)
Expand Down Expand Up @@ -200,7 +210,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG
)
issues := toolsets.NewToolset(ToolsetMetadataIssues.ID, ToolsetMetadataIssues.Description).
AddReadTools(
toolsets.NewServerTool(IssueRead(getClient, getGQLClient, cache, t, flags)),
toolsets.NewServerTool(IssueRead(getClient, getGQLClient, cache, t, flags, obsv)),
toolsets.NewServerTool(SearchIssues(getClient, t)),
toolsets.NewServerTool(ListIssues(getGQLClient, t)),
toolsets.NewServerTool(ListIssueTypes(getClient, t)),
Expand Down
29 changes: 29 additions & 0 deletions pkg/observability/log/level.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package log

// Level represents the log level, from debug to fatal
type Level struct {
level string
}

var (
// DebugLevel causes all logs to be logged
DebugLevel = Level{"debug"}
// InfoLevel causes all logs of level info or more severe to be logged
InfoLevel = Level{"info"}
// WarnLevel causes all logs of level warn or more severe to be logged
WarnLevel = Level{"warn"}
// ErrorLevel causes all logs of level error or more severe to be logged
ErrorLevel = Level{"error"}
// FatalLevel causes only logs of level fatal to be logged
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment for FatalLevel states "causes only logs of level fatal to be logged" which is inconsistent with other level comments. This suggests filtering that would suppress all other logs, but FatalLevel should be described as "causes all logs of level fatal to be logged" or "causes only fatal severity logs to be logged" to maintain consistency with the pattern of the other levels.

Suggested change
// FatalLevel causes only logs of level fatal to be logged
// FatalLevel causes all logs of level fatal to be logged

Copilot uses AI. Check for mistakes.
FatalLevel = Level{"fatal"}
)

// String returns the string representation for Level
//
// This is useful when trying to get the string values for Level and mapping level in other external libraries. For example:
// ```
// trace.SetLogLevel(kvp.String("loglevel", log.DebugLevel.String())
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example code in the comment contains a syntax error. It shows kvp.String("loglevel", log.DebugLevel.String()) but should likely be demonstrating a valid function call.

Suggested change
// trace.SetLogLevel(kvp.String("loglevel", log.DebugLevel.String())
// trace.SetLogLevel(kvp.String("loglevel", log.DebugLevel.String()))

Copilot uses AI. Check for mistakes.
// ```
func (l Level) String() string {
return l.level
}
21 changes: 21 additions & 0 deletions pkg/observability/log/log.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package log

import (
"context"
"log/slog"
)

type Logger interface {
Log(ctx context.Context, level Level, msg string, fields ...slog.Attr)
Debug(msg string, fields ...slog.Attr)
Info(msg string, fields ...slog.Attr)
Warn(msg string, fields ...slog.Attr)
Error(msg string, fields ...slog.Attr)
Fatal(msg string, fields ...slog.Attr)
WithFields(fields ...slog.Attr) Logger
WithError(err error) Logger
Named(name string) Logger
WithLevel(level Level) Logger
Sync() error
Level() Level
}
62 changes: 62 additions & 0 deletions pkg/observability/log/noop_adapter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package log

import (
"context"
"log/slog"
)

type NoopLogger struct{}

var _ Logger = (*NoopLogger)(nil)

func NewNoopLogger() *NoopLogger {
return &NoopLogger{}
}

func (l *NoopLogger) Level() Level {
return DebugLevel
}

func (l *NoopLogger) Log(_ context.Context, _ Level, _ string, _ ...slog.Attr) {
// No-op
}

func (l *NoopLogger) Debug(_ string, _ ...slog.Attr) {
// No-op
}

func (l *NoopLogger) Info(_ string, _ ...slog.Attr) {
// No-op
}

func (l *NoopLogger) Warn(_ string, _ ...slog.Attr) {
// No-op
}

func (l *NoopLogger) Error(_ string, _ ...slog.Attr) {
// No-op
}

func (l *NoopLogger) Fatal(_ string, _ ...slog.Attr) {
// No-op
Comment on lines +40 to +41
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NoopLogger's Fatal method should maintain consistent behavior with the SlogLogger implementation. Since SlogLogger panics after logging a fatal message, NoopLogger should also panic to ensure consistent Fatal semantics across implementations. Otherwise, code that relies on Fatal terminating execution could continue running unexpectedly when using NoopLogger.

Suggested change
func (l *NoopLogger) Fatal(_ string, _ ...slog.Attr) {
// No-op
func (l *NoopLogger) Fatal(msg string, _ ...slog.Attr) {
panic("NoopLogger.Fatal called: " + msg)

Copilot uses AI. Check for mistakes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your a badass ai

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your a badass ai

}

func (l *NoopLogger) WithFields(_ ...slog.Attr) Logger {
return l
}

func (l *NoopLogger) WithError(_ error) Logger {
return l
}

func (l *NoopLogger) Named(_ string) Logger {
return l
}

func (l *NoopLogger) WithLevel(_ Level) Logger {
return l
}

func (l *NoopLogger) Sync() error {
return nil
}
Comment on lines +12 to +62
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NoopLogger implementation lacks test coverage. While it's a simple no-op implementation, tests should verify that it correctly implements the Logger interface and that all methods are safe to call without side effects. Similar packages in this repository have corresponding test files.

Copilot uses AI. Check for mistakes.
Loading