- Notifications
You must be signed in to change notification settings - Fork3.1k
Add in memory cache for lockdown mode#1416
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
Conversation
a6fb6ea to5562335Compare| isPrivate,hasPushAccess,err:=repoAccessInfo(ctx,client,username,owner,repo) | ||
| // RepoAccessCache caches repository metadata related to lockdown checks so that | ||
| // multiple tools can reuse the same access information safely across goroutines. | ||
| typeRepoAccessCachestruct { |
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.
is there not a well tested library we can use?
The impl looks fine, but Id prefer a general cache impl from a tested public lib.
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.
I can try this one go get github.com/muesli/cache2go
* Initial plan* Replace custom cache with cache2go library- Added github.com/muesli/cache2go dependency- Replaced custom map-based cache with cache2go.CacheTable- Removed manual timer management (scheduleExpiry, ensureEntry methods)- Removed timer field from repoAccessCacheEntry struct- Updated GetRepoAccessInfo to use cache2go's Value() and Add() methods- Updated SetTTL to flush and re-add entries with new TTL- Used unique cache names per instance to avoid test interference- All existing tests pass with the new implementationCo-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>* Final verification completeCo-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>---------Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>
…cp-server into lockdown-mode-more-tools
* Initial plan* Implement RepoAccessCache as a singleton patternCo-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>* Complete singleton implementation and verificationCo-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>* Remove cacheIDCounter as requestedCo-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>---------Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>
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.
Pull Request Overview
This PR adds an in-memory caching layer for lockdown mode to reduce redundant GraphQL API calls when checking repository access permissions. It introduces a dependency ongithub.com/muesli/cache2go for TTL-based caching and refactors the lockdown implementation from a simple function to a cache-backed service with configurable TTL.
Key changes:
- Replaces
ShouldRemoveContentfunction with aRepoAccessCachethat caches repository privacy status and user push access per-repository - Integrates the cache into five lockdown-enabled tools:
GetIssue,GetIssueComments,GetSubIssues,GetPullRequest,GetPullRequestReviewComments, andGetPullRequestReviews - Adds CLI flag
--repo-access-cache-ttl(default 5m) to configure cache expiration
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| third-party/github.com/muesli/cache2go/LICENSE.txt | BSD-3-Clause license for new cache2go dependency |
| third-party-licenses.*.md | Updated license documentation for all platforms (darwin, linux, windows) |
| go.mod, go.sum | Added muesli/cache2go dependency at commit 518229cd8021 |
| pkg/lockdown/lockdown.go | Complete rewrite: introduces RepoAccessCache with singleton pattern, per-repo caching with per-user permission tracking, and configurable TTL |
| pkg/lockdown/lockdown_test.go | New test file with TTL eviction test |
| pkg/github/issues.go | Updated GetIssue, GetIssueComments, GetSubIssues, GetIssueLabels to accept and use cache parameter |
| pkg/github/pullrequests.go | Updated GetPullRequest, GetPullRequestReviewComments, GetPullRequestReviews to accept and use cache parameter |
| pkg/github/tools.go | Added cache parameter to DefaultToolsetGroup and passed to IssueRead and PullRequestRead tool constructors |
| pkg/github/issues_test.go, pullrequests_test.go, server_test.go | Updated test signatures to create and pass cache instances using stubRepoAccessCache helper |
| internal/ghmcp/server.go | Initializes cache singleton when lockdown mode is enabled, with optional TTL configuration |
| cmd/github-mcp-server/main.go | Added --repo-access-cache-ttl flag and plumbing to StdioServerConfig |
| cmd/github-mcp-server/generate_docs.go | Updated to pass cache instance (with nil client) to DefaultToolsetGroup |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
SamMorrowDrums left a comment
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.
I think Tony's comment makes sense and broadly this is very clear. I'd encourage stacked PRs for the separate tools files perhaps just to make review burden lower, but I'm really happy with the direction and@Chuxel will be very happy to see this arrive.
JoannaaKL commentedNov 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I addressed Tony's comment and this pr uses muesli/cache2go as a cache. |
206cecc toc8d5b6cCompareSamMorrowDrums left a comment
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.
Thanks for changes@JoannaaKL hope the merge conflict with Go SDK work Isn't too painful for Adam after this. 😅
28b868d intomainUh oh!
There was an error while loading.Please reload this page.
t3xash0rnyt0ad777-commits commentedNov 21, 2025 via email
| … On Fri, Nov 21, 2025, 3:35 AM JoannaaKL ***@***.***> wrote: Merged#1416 <#1416> into main. — Reply to this email directly, view it on GitHub <#1416 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BXWTFNRT3RXMBZSURBPSHZL353MHDAVCNFSM6AAAAACMLHGHTCVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMRRGA4TENJQGA4DINA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.*** com> |
Uh oh!
There was an error while loading.Please reload this page.
Improving lockdown mode:
GetIssueComments,GetSubIssues,GetPullRequest,GetPullRequestReviewComments,GetPullRequestReviewsThe bigger difference is that lockdown mode uses a cache with configureable TTL. That's because to fetch repo permissions like collaborators we need to do a graphql call which is costly, so we want to minimise number of calls we make.
Tools that are expected to return one result, like
GetIssuewill return an error in lockdown mode. Tools returning a list of items, likeGetIssueCommentswill filter out comments that were added by the user without push access.One caveat - Copilot is treated as non-collaborator and content created by it is filtered too. :D (I have a second pr to address that.)
Screenshots