Add in memory cache for lockdown mode #1416
Conversation
JoannaaKL
force-pushed
the
lockdown-mode-more-tools
branch
from
a6fb6ea to
5562335
Compare
| 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. | ||
| type RepoAccessCache struct { |
There was a problem hiding this comment.
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.
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 implementation Co-authored-by: JoannaaKL <[email protected]> * Final verification complete Co-authored-by: JoannaaKL <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: JoannaaKL <[email protected]>
…cp-server into lockdown-mode-more-tools
* Initial plan * Implement RepoAccessCache as a singleton pattern Co-authored-by: JoannaaKL <[email protected]> * Complete singleton implementation and verification Co-authored-by: JoannaaKL <[email protected]> * Remove cacheIDCounter as requested Co-authored-by: JoannaaKL <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: JoannaaKL <[email protected]>
JoannaaKL
changed the title
There was a problem hiding this comment.
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 on github.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 |
SamMorrowDrums
left a comment
SamMorrowDrums
left a comment
There was a problem hiding this comment.
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.
I addressed Tony's comment and this pr uses muesli/cache2go as a cache. |
JoannaaKL
force-pushed
the
lockdown-mode-more-tools
branch
from
206cecc to
c8d5b6c
Compare
SamMorrowDrums
left a comment
SamMorrowDrums
left a comment
There was a problem hiding this comment.
Thanks for changes @JoannaaKL hope the merge conflict with Go SDK work Isn't too painful for Adam after this. 😅
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