Dont filter content from Copilot by JoannaaKL · Pull Request #1464 · github/github-mcp-server

JoannaaKL · 2025-11-21T10:56:55Z

Lockdown mode improvement: if the author is trusted bot, don't filter out content. For now the list of trusted bots is hardcoded but it can easily be configurable if needed.

Copilot

Pull Request Overview

This pull request enhances lockdown mode to allow content from trusted bots (dependabot, github-actions, github-copilot) to bypass content filtering. The changes add a ViewerType field to track whether the GitHub API viewer is a Bot, and implement a isTrustedBot() check with a hardcoded list of trusted bot logins.

Key changes:

Added ViewerType field to track the __typename from GitHub's GraphQL API
Added trustedBotLogins map with hardcoded trusted bot identifiers
Modified IsSafeContent() to check for trusted bots before filtering content

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
pkg/lockdown/lockdown.go	Added ViewerType field, trustedBotLogins map, isTrustedBot() method, and modified IsSafeContent() to check trusted bots; updated GraphQL query to fetch __typename
pkg/lockdown/lockdown_test.go	Updated test mocks and assertions to include ViewerType field with value "User"

pkg/lockdown/lockdown.go

almaleksia · 2025-11-24T08:55:29Z

pkg/lockdown/lockdown.go

 		}, nil
 	}

-	c.logDebug("repo access cache miss", "owner", owner, "repo", repo, "user", username)


why you decided to remove attributes?

I didn't want to be too verbose but this is good point, I re-added them. It will make debugging easier

SamMorrowDrums

That seems ok. I think there's no easy way to do this other than string matching.

Could always allow a trusted user/bot arg so you can allow exceptions with intention. Like comma separated list of names or something.

SamMorrowDrums · 2025-11-24T08:57:13Z

pkg/lockdown/lockdown.go

-	logger *slog.Logger
+	client           *githubv4.Client
+	mu               sync.Mutex
+	cache            *cache2go.CacheTable


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?

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

lockdown mode becomes popular and

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 · 2025-11-24T08:59:17Z

pkg/lockdown/lockdown.go

 	}
-	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",


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.

It's also sending logs to stderr, I am basically passing down the logger that is created in main:

Or is that not what you meant?

That should be sufficient.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Copilot · 2025-11-26T07:55:50Z

pkg/lockdown/lockdown.go

 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


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 }

This sounds reasonable

Copilot · 2025-11-26T07:55:50Z

pkg/lockdown/lockdown.go

+// - the author currently has push access to the repository;
+// - the repository is private;
+// - the content was created by the viewer.


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

trusted bot

push access

private repo

viewer matches username

But the code checks:

trusted bot

private repo

viewer matches username

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

// - the author currently has push access to the repository;

// - the repository is private;

// - the content was created by the viewer.

// - the repository is private;

// - the content was created by the viewer;

// - the author currently has push access to the repository.

Copilot · 2025-11-26T07:55:51Z

pkg/lockdown/lockdown.go

+func (c *RepoAccessCache) isTrustedBot(username string) bool {
+	_, ok := c.trustedBotLogins[strings.ToLower(username)]
+	return ok
+}


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:

Verifying that "copilot" (case-insensitive) is recognized as a trusted bot

Ensuring IsSafeContent returns true for trusted bots without querying the API

Testing that non-trusted usernames still trigger the normal access checks

LuluBeatson

Looks good!

Only minor things are: checking isTrustedBot before getRepoAccessInfo, test coverage

JoannaaKL · 2025-11-26T10:24:07Z

Looks good!

Only minor things are: checking isTrustedBot before getRepoAccessInfo, test coverage

Thanks - I will address your feedback in a follow up pr ❤️

JoannaaKL requested a review from a team as a code owner November 21, 2025 10:56

Copilot AI review requested due to automatic review settings November 21, 2025 10:56

Copilot started reviewing on behalf of JoannaaKL November 21, 2025 10:57 View session

Copilot finished reviewing on behalf of JoannaaKL November 21, 2025 11:00

Copilot AI reviewed Nov 21, 2025

View reviewed changes

pkg/lockdown/lockdown.go Outdated Show resolved Hide resolved

pkg/lockdown/lockdown.go Outdated Show resolved Hide resolved

JoannaaKL force-pushed the allow-copilot-in-lockdown branch from 7f851b4 to 869c024 Compare November 21, 2025 11:04

Dont filter content from trusted bots

8400964

JoannaaKL force-pushed the allow-copilot-in-lockdown branch from 869c024 to 8400964 Compare November 21, 2025 11:06

Final changes

8ea409b

JoannaaKL changed the title ~~Dont filter content from trusted bots~~ Dont filter content from Copilot Nov 21, 2025

almaleksia reviewed Nov 24, 2025

View reviewed changes

SamMorrowDrums previously approved these changes Nov 24, 2025

View reviewed changes

SamMorrowDrums reviewed Nov 24, 2025

View reviewed changes

JoannaaKL added 2 commits November 24, 2025 10:35

Use only debug level

881dc82

Add logs and comments

c504141

JoannaaKL dismissed SamMorrowDrums’s stale review via c504141 November 24, 2025 10:15

Merge branch 'main' into allow-copilot-in-lockdown

b3ddb56

JoannaaKL requested review from SamMorrowDrums and almaleksia November 24, 2025 10:16

Merge branch 'main' into allow-copilot-in-lockdown

ef48964

JoannaaKL requested a review from Copilot November 26, 2025 07:52

Copilot started reviewing on behalf of JoannaaKL November 26, 2025 07:53 View session

Copilot finished reviewing on behalf of JoannaaKL November 26, 2025 07:55

Copilot AI reviewed Nov 26, 2025

View reviewed changes

LuluBeatson approved these changes Nov 26, 2025

View reviewed changes

tonytrg approved these changes Nov 26, 2025

View reviewed changes

JoannaaKL merged commit 7cfb354 into main Nov 26, 2025
23 checks passed

JoannaaKL deleted the allow-copilot-in-lockdown branch November 26, 2025 10:24

BrewTestBot mentioned this pull request Nov 26, 2025

github-mcp-server 0.23.0 Homebrew/homebrew-core#256096

Merged

Conversation

JoannaaKL commented Nov 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamMorrowDrums left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

LuluBeatson left a comment

Choose a reason for hiding this comment

Uh oh!

JoannaaKL commented Nov 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants