Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
7 changes: 5 additions & 2 deletions internal/ghmcp/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ type MCPServerConfig struct {

const stdioServerLogPrefix = "stdioserver"

func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) {
func NewMCPServer(cfg MCPServerConfig, logger *slog.Logger) (*server.MCPServer, error) {
apiHost, err := parseAPIHost(cfg.Host)
if err != nil {
return nil, fmt.Errorf("failed to parse API host: %w", err)
Expand All @@ -88,6 +88,9 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) {
if cfg.RepoAccessTTL != nil {
repoAccessOpts = append(repoAccessOpts, lockdown.WithTTL(*cfg.RepoAccessTTL))
}

repoAccessLogger := logger.With("component", "lockdown")
repoAccessOpts = append(repoAccessOpts, lockdown.WithLogger(repoAccessLogger))
var repoAccessCache *lockdown.RepoAccessCache
if cfg.LockdownMode {
repoAccessCache = lockdown.GetInstance(gqlClient, repoAccessOpts...)
Expand Down Expand Up @@ -273,7 +276,7 @@ func RunStdioServer(cfg StdioServerConfig) error {
ContentWindowSize: cfg.ContentWindowSize,
LockdownMode: cfg.LockdownMode,
RepoAccessTTL: cfg.RepoAccessCacheTTL,
})
}, logger)
if err != nil {
return fmt.Errorf("failed to create MCP server: %w", err)
}
Expand Down
60 changes: 44 additions & 16 deletions pkg/lockdown/lockdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@ import (
// RepoAccessCache caches repository metadata related to lockdown checks so that
// multiple tools can reuse the same access information safely across goroutines.
type RepoAccessCache struct {
client *githubv4.Client
mu sync.Mutex
cache *cache2go.CacheTable
ttl time.Duration
logger *slog.Logger
client *githubv4.Client
mu sync.Mutex
cache *cache2go.CacheTable
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's possible if it's expensive enough we might want to use redis or memcached on remote server, do you have a solution in mind for that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was thinking of that, or even creating an adapter for the local server. Generally using Redis in the remote server would be the next logical step.
Two main aspects that will guide enhancing this mode are

  1. lockdown mode becomes popular and
  2. if the memory burden on maintaining this cache with TTL is too high (but I need to analyse usage patterns to be 100% sure, my gut feeling is that typical user won't be scanning thousands of repos)

ttl time.Duration
logger *slog.Logger
trustedBotLogins map[string]struct{}
}

type repoAccessCacheEntry struct {
Expand Down Expand Up @@ -85,6 +86,9 @@ func GetInstance(client *githubv4.Client, opts ...RepoAccessOption) *RepoAccessC
client: client,
cache: cache2go.Cache(defaultRepoAccessCacheKey),
ttl: defaultRepoAccessTTL,
trustedBotLogins: map[string]struct{}{
"copilot": {},
},
}
for _, opt := range opts {
if opt != nil {
Expand Down Expand Up @@ -112,10 +116,13 @@ type CacheStats struct {
func (c *RepoAccessCache) IsSafeContent(ctx context.Context, username, owner, repo string) (bool, error) {
repoInfo, err := c.getRepoAccessInfo(ctx, username, owner, repo)
if err != nil {
c.logDebug("error checking repo access info for content filtering", "owner", owner, "repo", repo, "user", username, "error", err)
return false, err
}
if repoInfo.IsPrivate || repoInfo.ViewerLogin == username {

c.logInfo(ctx, fmt.Sprintf("evaluated repo access fur user %s to %s/%s for content filtering, result: hasPushAccess=%t, isPrivate=%t",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does this logger hook into MCP logging because log output shows up like an error with MCP servers using stdio, and I think you can instead send server log messages explicitly.

Also you can log to stderr (at least the docs from MCP debugging in VS Code say that's better.

It looks like this is logging to stdio unless I missed something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's also sending logs to stderr, I am basically passing down the logger that is created in main:
CleanShot 2025-11-24 at 10 31 52@2x
Or is that not what you meant?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That should be sufficient.

username, owner, repo, repoInfo.HasPushAccess, repoInfo.IsPrivate))

if c.isTrustedBot(username) || repoInfo.IsPrivate || repoInfo.ViewerLogin == strings.ToLower(username) {
return true, nil
}
return repoInfo.HasPushAccess, nil
Comment on lines 122 to 134
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

The trusted bot check should be performed before calling getRepoAccessInfo to avoid an unnecessary API query for trusted bots. Currently, the function fetches repo access information first (which may involve a GraphQL API call), then checks if the user is a trusted bot. This wastes API quota and adds latency for bot accounts.

Consider moving the trusted bot check to the beginning:

func (c *RepoAccessCache) IsSafeContent(ctx context.Context, username, owner, repo string) (bool, error) {
	if c.isTrustedBot(username) {
		return true, nil
	}
	
	repoInfo, err := c.getRepoAccessInfo(ctx, username, owner, repo)
	if err != nil {
		return false, err
	}
	// ... rest of the logic
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This sounds reasonable

Expand All @@ -136,30 +143,34 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner
if err == nil {
entry := cacheItem.Data().(*repoAccessCacheEntry)
if cachedHasPush, known := entry.knownUsers[userKey]; known {
c.logDebug("repo access cache hit", "owner", owner, "repo", repo, "user", username)
c.logDebug(ctx, "repo access cache hit")
return RepoAccessInfo{
IsPrivate: entry.isPrivate,
HasPushAccess: cachedHasPush,
ViewerLogin: entry.viewerLogin,
}, nil
}
c.logDebug("known users cache miss", "owner", owner, "repo", repo, "user", username)

c.logDebug(ctx, "known users cache miss")

info, queryErr := c.queryRepoAccessInfo(ctx, username, owner, repo)
if queryErr != nil {
return RepoAccessInfo{}, queryErr
}

entry.knownUsers[userKey] = info.HasPushAccess
entry.viewerLogin = info.ViewerLogin
entry.isPrivate = info.IsPrivate
c.cache.Add(key, c.ttl, entry)

return RepoAccessInfo{
IsPrivate: entry.isPrivate,
HasPushAccess: entry.knownUsers[userKey],
ViewerLogin: entry.viewerLogin,
}, nil
}

c.logDebug("repo access cache miss", "owner", owner, "repo", repo, "user", username)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why you decided to remove attributes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to be too verbose but this is good point, I re-added them. It will make debugging easier
CleanShot 2025-11-24 at 11 16 06@2x

c.logDebug(ctx, "repo access cache miss")

info, queryErr := c.queryRepoAccessInfo(ctx, username, owner, repo)
if queryErr != nil {
Expand Down Expand Up @@ -230,12 +241,29 @@ func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, username, own
}, nil
}

func cacheKey(owner, repo string) string {
return fmt.Sprintf("%s/%s", strings.ToLower(owner), strings.ToLower(repo))
func (c *RepoAccessCache) log(ctx context.Context, level slog.Level, msg string, attrs ...slog.Attr) {
if c == nil || c.logger == nil {
return
}
if !c.logger.Enabled(ctx, level) {
return
}
c.logger.LogAttrs(ctx, level, msg, attrs...)
}

func (c *RepoAccessCache) logDebug(msg string, args ...any) {
if c != nil && c.logger != nil {
c.logger.Debug(msg, args...)
}
func (c *RepoAccessCache) logDebug(ctx context.Context, msg string, attrs ...slog.Attr) {
c.log(ctx, slog.LevelDebug, msg, attrs...)
}

func (c *RepoAccessCache) logInfo(ctx context.Context, msg string, attrs ...slog.Attr) {
c.log(ctx, slog.LevelInfo, msg, attrs...)
}

func (c *RepoAccessCache) isTrustedBot(username string) bool {
_, ok := c.trustedBotLogins[strings.ToLower(username)]
return ok
}
Comment on lines +267 to +270
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

The new trusted bot functionality lacks test coverage. Since other functions in this package have tests (e.g., TestRepoAccessCacheEvictsAfterTTL), the new behavior should also be tested.

Consider adding test cases for:

  1. Verifying that "copilot" (case-insensitive) is recognized as a trusted bot
  2. Ensuring IsSafeContent returns true for trusted bots without querying the API
  3. Testing that non-trusted usernames still trigger the normal access checks

Copilot uses AI. Check for mistakes.

func cacheKey(owner, repo string) string {
return fmt.Sprintf("%s/%s", strings.ToLower(owner), strings.ToLower(repo))
}
Loading