Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Dont filter content from Copilot#1464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
JoannaaKL merged 6 commits intomainfromallow-copilot-in-lockdown
Nov 26, 2025
Merged
Show file tree
Hide file tree
Changes fromall 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 deletionsinternal/ghmcp/server.go
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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
65 changes: 49 additions & 16 deletionspkg/lockdown/lockdown.go
View file
Open in desktop
Original file line numberDiff line numberDiff 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
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
ContributorAuthor

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)

SamMorrowDrums reacted with thumbs up emoji
ttl time.Duration
logger *slog.Logger
trustedBotLogins map[string]struct{}
}

type repoAccessCacheEntry struct {
Expand DownExpand 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 All@@ -109,13 +113,22 @@ type CacheStats struct {
Evictions int64
}

// IsSafeContent determines if the specified user can safely access the requested repository content.
// Safe access applies when any of the following is true:
// - the content was created by a trusted bot;
// - the author currently has push access to the repository;
// - the repository is private;
// - the content was created by the viewer.
Comment on lines +119 to +121

Choose a reason for hiding this comment

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

The documentation lists the conditions in a different order than they are checked in the code. The comment states:

  1. trusted bot
  2. push access
  3. private repo
  4. viewer matches username

But the code checks:

  1. trusted bot
  2. private repo
  3. viewer matches username
  4. push access

For clarity and maintainability, the documentation should match the code order. Consider updating the comment to:

// IsSafeContent determines if the specified user can safely access the requested repository content.// Safe access applies when any of the following is true:// - the content was created by a trusted bot;// - the repository is private;// - the content was created by the viewer;// - the author currently has push access to the repository.
Suggested change
// - theauthor currently has push access to the repository;
// - therepository is private;
// - thecontent was created bytheviewer.
// - therepository is private;
// - thecontent was created by the viewer;
// - theauthor currently has push access totherepository.

Copilot uses AI. Check for mistakes.
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.logDebug(ctx, fmt.Sprintf("evaluated repo access for user %s to %s/%s for content filtering, result: hasPushAccess=%t, isPrivate=%t",
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

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 callinggetRepoAccessInfo 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,repostring) (bool,error) {ifc.isTrustedBot(username) {returntrue,nil}repoInfo,err:=c.getRepoAccessInfo(ctx,username,owner,repo)iferr!=nil {returnfalse,err}// ... rest of the logic}

Copilot uses AI. Check for mistakes.
Copy link
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 +149,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, fmt.Sprintf("repo access cache hit for user %s to %s/%s", username, owner, repo))
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, fetching from graphql API")

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
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
ContributorAuthor

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, fmt.Sprintf("repo access cache miss for user %s to %s/%s", username, owner, repo))

info, queryErr := c.queryRepoAccessInfo(ctx, username, owner, repo)
if queryErr != nil {
Expand DownExpand Up@@ -223,19 +240,35 @@ func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, username, own
}
}

c.logDebug(ctx, fmt.Sprintf("queried repo access info for user %s to %s/%s: isPrivate=%t, hasPushAccess=%t, viewerLogin=%s",
username, owner, repo, bool(query.Repository.IsPrivate), hasPush, query.Viewer.Login))

return RepoAccessInfo{
IsPrivate: bool(query.Repository.IsPrivate),
HasPushAccess: hasPush,
ViewerLogin: string(query.Viewer.Login),
}, 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) isTrustedBot(username string) bool {
_, ok := c.trustedBotLogins[strings.ToLower(username)]
return ok
}
Comment on lines +267 to +270

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. EnsuringIsSafeContent returnstrue 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
[8]ページ先頭
©2009-2025 Movatter.jp