-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8400964
8ea409b
881dc82
c504141
b3ddb56
ef48964
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||||||||||
| ttl time.Duration | ||||||||||||||
| logger *slog.Logger | ||||||||||||||
| trustedBotLogins map[string]struct{} | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| type repoAccessCacheEntry struct { | ||||||||||||||
|
|
@@ -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 { | ||||||||||||||
|
|
@@ -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
|
||||||||||||||
| // - 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
AI
Nov 26, 2025
There was a problem hiding this comment.
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
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds reasonable
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
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:
- Verifying that "copilot" (case-insensitive) is recognized as a trusted bot
- Ensuring
IsSafeContentreturnstruefor trusted bots without querying the API - Testing that non-trusted usernames still trigger the normal access checks

There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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