Skip to content

Commit c46bd2e

Browse files
JoannaaKLJoannaaKL
committed
.
1 parent d9e0e0c commit c46bd2e

File tree

4 files changed

+127
-80
lines changed

4 files changed

+127
-80
lines changed

‎pkg/github/issues_test.go‎

Lines changed: 90 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
"github.com/github/github-mcp-server/internal/githubv4mock"
1313
"github.com/github/github-mcp-server/internal/toolsnaps"
14+
"github.com/github/github-mcp-server/pkg/lockdown"
1415
"github.com/github/github-mcp-server/pkg/translations"
1516
"github.com/google/go-github/v79/github"
1617
"github.com/migueleliasweb/go-github-mock/src/mock"
@@ -19,66 +20,13 @@ import (
1920
"github.com/stretchr/testify/require"
2021
)
2122

23+
var defaultGQLClient *githubv4.Client = githubv4.NewClient(githubv4mock.NewMockedHTTPClient())
24+
var repoAccessCache *lockdown.RepoAccessCache = stubRepoAccessCache(defaultGQLClient, 15*time.Minute)
25+
2226
func Test_GetIssue(t *testing.T) {
2327
// Verify tool definition once
2428
mockClient := github.NewClient(nil)
25-
type repoAccessQuery struct {
26-
Repository struct {
27-
IsPrivate githubv4.Boolean
28-
Collaborators struct {
29-
Edges []struct {
30-
Permission githubv4.String
31-
Node struct {
32-
Login githubv4.String
33-
}
34-
}
35-
} `graphql:"collaborators(query: $username, first: 1)"`
36-
} `graphql:"repository(owner: $owner, name: $name)"`
37-
}
38-
39-
lockdownHTTPClient := githubv4mock.NewMockedHTTPClient(
40-
githubv4mock.NewQueryMatcher(
41-
repoAccessQuery{},
42-
map[string]any{
43-
"owner": githubv4.String("github"),
44-
"name": githubv4.String("github-mcp-server"),
45-
"username": githubv4.String("testuser"),
46-
},
47-
githubv4mock.DataResponse(map[string]any{
48-
"repository": map[string]any{
49-
"isPrivate": true,
50-
"collaborators": map[string]any{
51-
"edges": []any{},
52-
},
53-
},
54-
}),
55-
),
56-
githubv4mock.NewQueryMatcher(
57-
repoAccessQuery{},
58-
map[string]any{
59-
"owner": githubv4.String("owner"),
60-
"name": githubv4.String("repo"),
61-
"username": githubv4.String("testuser"),
62-
},
63-
githubv4mock.DataResponse(map[string]any{
64-
"repository": map[string]any{
65-
"isPrivate": false,
66-
"collaborators": map[string]any{
67-
"edges": []any{
68-
map[string]any{
69-
"permission": "READ",
70-
"node": map[string]any{
71-
"login": "testuser",
72-
},
73-
},
74-
},
75-
},
76-
},
77-
}),
78-
),
79-
)
80-
defaultGQLClient := githubv4.NewClient(lockdownHTTPClient)
81-
repoAccessCache := stubRepoAccessCache(defaultGQLClient, 15*time.Minute)
29+
defaultGQLClient := githubv4.NewClient(nil)
8230
tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(defaultGQLClient), repoAccessCache, translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
8331
require.NoError(t, toolsnaps.Test(tool.Name, tool))
8432

@@ -111,6 +59,7 @@ func Test_GetIssue(t *testing.T) {
11159
tests := []struct {
11260
name string
11361
mockedClient *http.Client
62+
gqlHTTPClient *http.Client
11463
requestArgs map[string]interface{}
11564
expectHandlerError bool
11665
expectResultError bool
@@ -159,10 +108,40 @@ func Test_GetIssue(t *testing.T) {
159108
mockIssue,
160109
),
161110
),
111+
gqlHTTPClient: githubv4mock.NewMockedHTTPClient(
112+
githubv4mock.NewQueryMatcher(
113+
struct {
114+
Repository struct {
115+
IsPrivate githubv4.Boolean
116+
Collaborators struct {
117+
Edges []struct {
118+
Permission githubv4.String
119+
Node struct {
120+
Login githubv4.String
121+
}
122+
}
123+
} `graphql:"collaborators(query: $username, first: 1)"`
124+
} `graphql:"repository(owner: $owner, name: $name)"`
125+
}{},
126+
map[string]any{
127+
"owner": githubv4.String("owner"),
128+
"name": githubv4.String("repo"),
129+
"username": githubv4.String("testuser"),
130+
},
131+
githubv4mock.DataResponse(map[string]any{
132+
"repository": map[string]any{
133+
"isPrivate": true,
134+
"collaborators": map[string]any{
135+
"edges": []any{},
136+
},
137+
},
138+
}),
139+
),
140+
),
162141
requestArgs: map[string]interface{}{
163142
"method": "get",
164-
"owner": "github",
165-
"repo": "github-mcp-server",
143+
"owner": "owner",
144+
"repo": "repo",
166145
"issue_number": float64(42),
167146
},
168147
expectedIssue: mockIssue,
@@ -176,6 +155,43 @@ func Test_GetIssue(t *testing.T) {
176155
mockIssue,
177156
),
178157
),
158+
gqlHTTPClient: githubv4mock.NewMockedHTTPClient(
159+
githubv4mock.NewQueryMatcher(
160+
struct {
161+
Repository struct {
162+
IsPrivate githubv4.Boolean
163+
Collaborators struct {
164+
Edges []struct {
165+
Permission githubv4.String
166+
Node struct {
167+
Login githubv4.String
168+
}
169+
}
170+
} `graphql:"collaborators(query: $username, first: 1)"`
171+
} `graphql:"repository(owner: $owner, name: $name)"`
172+
}{},
173+
map[string]any{
174+
"owner": githubv4.String("owner"),
175+
"name": githubv4.String("repo"),
176+
"username": githubv4.String("testuser"),
177+
},
178+
githubv4mock.DataResponse(map[string]any{
179+
"repository": map[string]any{
180+
"isPrivate": false,
181+
"collaborators": map[string]any{
182+
"edges": []any{
183+
map[string]any{
184+
"permission": "READ",
185+
"node": map[string]any{
186+
"login": "testuser",
187+
},
188+
},
189+
},
190+
},
191+
},
192+
}),
193+
),
194+
),
179195
requestArgs: map[string]interface{}{
180196
"method": "get",
181197
"owner": "owner",
@@ -192,10 +208,17 @@ func Test_GetIssue(t *testing.T) {
192208
t.Run(tc.name, func(t *testing.T) {
193209
client := github.NewClient(tc.mockedClient)
194210

195-
gqlClient := defaultGQLClient
211+
var gqlClient *githubv4.Client
212+
cache := repoAccessCache
213+
if tc.gqlHTTPClient != nil {
214+
gqlClient = githubv4.NewClient(tc.gqlHTTPClient)
215+
cache = stubRepoAccessCache(gqlClient, 15*time.Minute)
216+
} else {
217+
gqlClient = defaultGQLClient
218+
}
196219

197220
flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled})
198-
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), repoAccessCache, translations.NullTranslationHelper, flags)
221+
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), cache, translations.NullTranslationHelper, flags)
199222

200223
request := createMCPRequest(tc.requestArgs)
201224
result, err := handler(context.Background(), request)
@@ -1693,7 +1716,7 @@ func Test_GetIssueComments(t *testing.T) {
16931716
// Verify tool definition once
16941717
mockClient := github.NewClient(nil)
16951718
gqlClient := githubv4.NewClient(nil)
1696-
tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
1719+
tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
16971720
require.NoError(t, toolsnaps.Test(tool.Name, tool))
16981721

16991722
assert.Equal(t, "issue_read", tool.Name)
@@ -1799,7 +1822,7 @@ func Test_GetIssueComments(t *testing.T) {
17991822
// Setup client with mock
18001823
client := github.NewClient(tc.mockedClient)
18011824
gqlClient := githubv4.NewClient(nil)
1802-
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
1825+
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
18031826

18041827
// Create call request
18051828
request := createMCPRequest(tc.requestArgs)
@@ -1836,7 +1859,7 @@ func Test_GetIssueLabels(t *testing.T) {
18361859
// Verify tool definition
18371860
mockGQClient := githubv4.NewClient(nil)
18381861
mockClient := github.NewClient(nil)
1839-
tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(mockGQClient), stubRepoAccessCache(mockGQClient, 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
1862+
tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(mockGQClient), stubRepoAccessCache(mockGQClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
18401863
require.NoError(t, toolsnaps.Test(tool.Name, tool))
18411864

18421865
assert.Equal(t, "issue_read", tool.Name)
@@ -1911,7 +1934,7 @@ func Test_GetIssueLabels(t *testing.T) {
19111934
t.Run(tc.name, func(t *testing.T) {
19121935
gqlClient := githubv4.NewClient(tc.mockedClient)
19131936
client := github.NewClient(nil)
1914-
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
1937+
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
19151938

19161939
request := createMCPRequest(tc.requestArgs)
19171940
result, err := handler(context.Background(), request)
@@ -2602,7 +2625,7 @@ func Test_GetSubIssues(t *testing.T) {
26022625
// Verify tool definition once
26032626
mockClient := github.NewClient(nil)
26042627
gqlClient := githubv4.NewClient(nil)
2605-
tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
2628+
tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
26062629
require.NoError(t, toolsnaps.Test(tool.Name, tool))
26072630

26082631
assert.Equal(t, "issue_read", tool.Name)
@@ -2799,7 +2822,7 @@ func Test_GetSubIssues(t *testing.T) {
27992822
// Setup client with mock
28002823
client := github.NewClient(tc.mockedClient)
28012824
gqlClient := githubv4.NewClient(nil)
2802-
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
2825+
_, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
28032826

28042827
// Create call request
28052828
request := createMCPRequest(tc.requestArgs)

‎pkg/github/pullrequests_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import (
2121
func Test_GetPullRequest(t *testing.T) {
2222
// Verify tool definition once
2323
mockClient := github.NewClient(nil)
24-
tool, _ := PullRequestRead(stubGetClientFn(mockClient), stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
24+
tool, _ := PullRequestRead(stubGetClientFn(mockClient), stubRepoAccessCache(githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
2525
require.NoError(t, toolsnaps.Test(tool.Name, tool))
2626

2727
assert.Equal(t, "pull_request_read", tool.Name)
@@ -102,7 +102,7 @@ func Test_GetPullRequest(t *testing.T) {
102102
t.Run(tc.name, func(t *testing.T) {
103103
// Setup client with mock
104104
client := github.NewClient(tc.mockedClient)
105-
_, handler := PullRequestRead(stubGetClientFn(client), stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
105+
_, handler := PullRequestRead(stubGetClientFn(client), stubRepoAccessCache(githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
106106

107107
// Create call request
108108
request := createMCPRequest(tc.requestArgs)
@@ -1133,7 +1133,7 @@ func Test_SearchPullRequests(t *testing.T) {
11331133
func Test_GetPullRequestFiles(t *testing.T) {
11341134
// Verify tool definition once
11351135
mockClient := github.NewClient(nil)
1136-
tool, _ := PullRequestRead(stubGetClientFn(mockClient), stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
1136+
tool, _ := PullRequestRead(stubGetClientFn(mockClient), stubRepoAccessCache(githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
11371137
require.NoError(t, toolsnaps.Test(tool.Name, tool))
11381138

11391139
assert.Equal(t, "pull_request_read", tool.Name)
@@ -1236,7 +1236,7 @@ func Test_GetPullRequestFiles(t *testing.T) {
12361236
t.Run(tc.name, func(t *testing.T) {
12371237
// Setup client with mock
12381238
client := github.NewClient(tc.mockedClient)
1239-
_, handler := PullRequestRead(stubGetClientFn(client), stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
1239+
_, handler := PullRequestRead(stubGetClientFn(client), stubRepoAccessCache(githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
12401240

12411241
// Create call request
12421242
request := createMCPRequest(tc.requestArgs)
@@ -1277,7 +1277,7 @@ func Test_GetPullRequestFiles(t *testing.T) {
12771277
func Test_GetPullRequestStatus(t *testing.T) {
12781278
// Verify tool definition once
12791279
mockClient := github.NewClient(nil)
1280-
tool, _ := PullRequestRead(stubGetClientFn(mockClient), stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
1280+
tool, _ := PullRequestRead(stubGetClientFn(mockClient), stubRepoAccessCache(githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false}))
12811281
require.NoError(t, toolsnaps.Test(tool.Name, tool))
12821282

12831283
assert.Equal(t, "pull_request_read", tool.Name)

‎pkg/github/server_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,8 @@ func stubGetGQLClientFn(client *githubv4.Client) GetGQLClientFn {
4141
}
4242

4343
func stubRepoAccessCache(client *githubv4.Client, ttl time.Duration) *lockdown.RepoAccessCache {
44-
return lockdown.GetInstance(client, lockdown.WithTTL(ttl))
44+
cacheName := fmt.Sprintf("repo-access-cache-test-%d", time.Now().UnixNano())
45+
return lockdown.NewRepoAccessCache(client, lockdown.WithTTL(ttl), lockdown.WithCacheName(cacheName))
4546
}
4647

4748
func stubFeatureFlags(enabledFlags map[string]bool) FeatureFlags {

‎pkg/lockdown/lockdown.go‎

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,14 @@ type repoAccessCacheEntry struct {
2727
knownUsers map[string]bool // normalized login -> has push access
2828
}
2929

30-
const defaultRepoAccessTTL = 5 * time.Minute
30+
const (
31+
defaultRepoAccessTTL = 5 * time.Minute
32+
defaultRepoAccessCacheKey = "repo-access-cache"
33+
)
3134

3235
var (
33-
instance *RepoAccessCache
34-
instanceOnce sync.Once
36+
instance *RepoAccessCache
37+
instanceMu sync.Mutex
3538
)
3639

3740
// RepoAccessOption configures RepoAccessCache at construction time.
@@ -52,31 +55,51 @@ func WithLogger(logger *slog.Logger) RepoAccessOption {
5255
}
5356
}
5457

58+
// WithCacheName overrides the cache table name used for storing entries. This option is intended for tests
59+
// that need isolated cache instances.
60+
func WithCacheName(name string) RepoAccessOption {
61+
return func(c *RepoAccessCache) {
62+
if name != "" {
63+
c.cache = cache2go.Cache(name)
64+
}
65+
}
66+
}
67+
5568
// GetInstance returns the singleton instance of RepoAccessCache.
5669
// It initializes the instance on first call with the provided client and options.
5770
// Subsequent calls ignore the client and options parameters and return the existing instance.
5871
// This is the preferred way to access the cache in production code.
5972
func GetInstance(client *githubv4.Client, opts ...RepoAccessOption) *RepoAccessCache {
60-
instanceOnce.Do(func() {
73+
instanceMu.Lock()
74+
defer instanceMu.Unlock()
75+
if instance == nil {
6176
instance = newRepoAccessCache(client, opts...)
62-
})
77+
}
6378
return instance
6479
}
6580

81+
// NewRepoAccessCache constructs a repo access cache without mutating the global singleton.
82+
// This helper is useful for tests that need isolated cache instances.
83+
func NewRepoAccessCache(client *githubv4.Client, opts ...RepoAccessOption) *RepoAccessCache {
84+
return newRepoAccessCache(client, opts...)
85+
}
86+
6687
// newRepoAccessCache creates a new cache instance. This is a private helper function
6788
// used by GetInstance.
6889
func newRepoAccessCache(client *githubv4.Client, opts ...RepoAccessOption) *RepoAccessCache {
69-
cacheName := "repo-access-cache"
7090
c := &RepoAccessCache{
7191
client: client,
72-
cache: cache2go.Cache(cacheName),
92+
cache: cache2go.Cache(defaultRepoAccessCacheKey),
7393
ttl: defaultRepoAccessTTL,
7494
}
7595
for _, opt := range opts {
7696
if opt != nil {
7797
opt(c)
7898
}
7999
}
100+
if c.cache == nil {
101+
c.cache = cache2go.Cache(defaultRepoAccessCacheKey)
102+
}
80103
c.logInfo("repo access cache initialized", "ttl", c.ttl)
81104
return c
82105
}

0 commit comments

Comments
 (0)