refactor(lockdown): trim comments to non-obvious invariants

The cache changes carried explanatory comments that restated the code or
narrated what each step did. Drop them and keep only what the code cannot
express: that cache2go never reclaims a named table, that its own expiry
slides on every read, that createdAt survives entry updates, and that
RepoAccessOpts is shared across requests. Exported options keep a short
doc comment.

Comment-only; no behavior change.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Author: SamMorrowDrums

pkg/github/dependencies.go

@@ -463,12 +463,8 @@ func (d *RequestDeps) GetRepoAccessCache(ctx context.Context) (*lockdown.RepoAcc
 		return nil, err
 	}
 
-	// Scope cache entries to the requesting identity so a trust decision
-	// computed under one caller's credentials is never served to another.
-	// RepoAccessOpts is built once at server startup and shared by every
-	// request, so identity scoping has to be applied per request here. Copy
-	// the slice before appending so concurrent requests never mutate the
-	// shared backing array.
+	// RepoAccessOpts is shared across requests, so copy before appending the
+	// per-request identity scope.
 	opts := d.RepoAccessOpts
 	if tokenInfo, ok := ghcontext.GetTokenInfo(ctx); ok && tokenInfo.Token != "" {
 		opts = append(append([]lockdown.RepoAccessOption{}, d.RepoAccessOpts...), lockdown.WithIdentity(tokenInfo.Token))

pkg/github/dependencies_test.go

@@ -124,15 +124,8 @@ func TestRequestDepsScopesTokensToConfiguredHosts(t *testing.T) {
 	assert.Empty(t, foreignAuth, "GraphQL redirect must not authenticate to a foreign host")
 }
 
-// TestGetRepoAccessCacheIsolatesTrustDecisionsPerIdentity is a regression test
-// for issue #3107. It mirrors exactly how the HTTP server builds RequestDeps:
-// a single RepoAccessOpts slice is constructed once at startup (with no
-// per-identity WithCacheName) and reused across every request, and
-// GetRepoAccessCache is called fresh per request. Two different token
-// identities querying the same owner/repo/author must each perform their own
-// upstream lookups instead of one being served from the other's cached
-// decision, while repeated requests from the same identity must reuse a warm
-// cache.
+// Regression test for #3107: RequestDeps is built once at startup and shared,
+// so identity scoping has to happen per request in GetRepoAccessCache.
 func TestGetRepoAccessCacheIsolatesTrustDecisionsPerIdentity(t *testing.T) {
 	t.Parallel()
 
@@ -159,7 +152,7 @@ func TestGetRepoAccessCacheIsolatesTrustDecisionsPerIdentity(t *testing.T) {
 		return gqlCalls, restCalls
 	}
 
-	// Built once, exactly as pkg/http/server.go does today: no WithCacheName.
+	// Built as pkg/http/server.go does: no per-identity options.
 	deps := github.NewRequestDeps(
 		newRequestDepsAPIHostResolver(t, server.URL),
 		"test",
@@ -195,8 +188,6 @@ func TestGetRepoAccessCacheIsolatesTrustDecisionsPerIdentity(t *testing.T) {
 	require.Equal(t, 2, gqlN, "a different identity's request must not be served from another identity's cached trust decision")
 	require.Equal(t, 2, restN, "a different identity's request must not be served from another identity's cached trust decision")
 
-	// Repeating the same identity's token must reuse the warm per-identity
-	// cache without any additional upstream calls.
 	cacheAliceAgain, err := deps.GetRepoAccessCache(ctxAlice)
 	require.NoError(t, err)
 	_, err = cacheAliceAgain.IsSafeContent(ctxAlice, "mallory", "owner", "repo")

pkg/lockdown/lockdown.go

@@ -27,14 +27,8 @@ type RepoAccessCache struct {
 	ttl              time.Duration
 	logger           *slog.Logger
 	trustedBotLogins map[string]struct{}
-
-	// identityDigest scopes this instance's entry keys to a single request
-	// identity. Empty means entries are unscoped. See WithIdentity.
-	identityDigest string
-
-	// now returns the current time and defaults to time.Now. Tests override it
-	// to exercise bounded expiry deterministically without sleeping.
-	now func() time.Time
+	identityDigest   string
+	now              func() time.Time
 
 	viewerMu    sync.Mutex
 	viewerLogin string
@@ -44,10 +38,7 @@ type repoAccessCacheEntry struct {
 	isPrivate  bool
 	knownUsers map[string]bool // normalized login -> has push access
 
-	// createdAt is the wall-clock time this repository's trust decision was
-	// first fetched. It is preserved across every subsequent update to the
-	// entry (e.g. learning about a newly-seen author), so an entry's maximum
-	// age is bounded from its original creation rather than reset by access.
+	// Preserved across entry updates, so age is bounded from the first fetch.
 	createdAt time.Time
 }
 
@@ -66,12 +57,8 @@ const (
 type RepoAccessOption func(*RepoAccessCache)
 
 // WithTTL overrides the default maximum age applied to cache entries. A
-// non-positive duration disables expiration.
-//
-// The TTL is a bounded, absolute age measured from when an entry's trust
-// decision was first fetched: repeated reads never extend it. This ensures
-// an actively-read entry is still refreshed once it reaches the maximum age,
-// rather than sliding its expiration forward indefinitely.
+// non-positive duration disables expiration. The age is absolute, measured
+// from an entry's first fetch: repeated reads never extend it.
 func WithTTL(ttl time.Duration) RepoAccessOption {
 	return func(c *RepoAccessCache) {
 		c.ttl = ttl
@@ -88,13 +75,9 @@ func WithLogger(logger *slog.Logger) RepoAccessOption {
 // WithCacheName overrides the cache table name used for storing entries.
 // Use this to isolate cache entries between tenants or in tests.
 //
-// cache2go.Cache(name) returns a process-wide singleton table that is created
-// on first use and never reclaimed, so the set of names a process passes here
-// must be bounded and known ahead of time. Never derive a name from
-// request-supplied data such as an auth token: the table registry would grow
-// without bound, retaining every distinct value seen for the lifetime of the
-// process. To isolate cached decisions per request identity, use WithIdentity,
-// which keeps a single table and scopes individual entries instead.
+// cache2go never reclaims a named table, so names must come from a bounded,
+// known set; never derive one from request data. Use WithIdentity instead to
+// isolate per request identity.
 func WithCacheName(name string) RepoAccessOption {
 	return func(c *RepoAccessCache) {
 		if name != "" {
@@ -103,21 +86,14 @@ func WithCacheName(name string) RepoAccessOption {
 	}
 }
 
-// WithIdentity scopes this cache's entries to a single request identity
-// (typically an auth token), so a trust decision computed under one caller's
-// credentials is never served to another. Two instances configured with the
-// same identity share a warm cache; instances with different identities
-// cannot observe each other's entries.
-//
-// Isolation is applied to the entry key rather than the cache table: entries
-// are stored in the shared table under a key prefixed with a digest of the
-// identity. This keeps storage bounded, because per-identity entries are
-// reclaimed by the same TTL cleanup as any other entry. Allocating a table
-// per identity instead would leak, since cache2go never evicts tables.
+// WithIdentity scopes cache entries to a single request identity, typically an
+// auth token, so a decision computed under one caller's credentials is never
+// served to another. Equal identities share a warm cache; an empty one is a
+// no-op.
 //
-// The identity is hashed so it never appears verbatim in cache keys, logs, or
-// metrics. An empty identity is a no-op, leaving this instance's entries
-// unscoped; callers that need isolation must supply a non-empty identity.
+// Scoping lives in the entry key rather than the table so per-identity state
+// stays bounded and is reclaimed by ordinary TTL cleanup. The identity is
+// hashed so it never appears verbatim in a key.
 func WithIdentity(identity string) RepoAccessOption {
 	return func(c *RepoAccessCache) {
 		if identity == "" {
@@ -261,8 +237,7 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner
 			users := make(map[string]bool, len(entry.knownUsers)+1)
 			maps.Copy(users, entry.knownUsers)
 			users[userKey] = hasPush
-			// Preserve the entry's original createdAt: learning about a newly
-			// seen author must not reset the entry's bounded maximum age.
+			// Preserve createdAt: a new author must not reset the entry's age.
 			c.cache.Add(key, c.ttl, &repoAccessCacheEntry{
 				isPrivate:  entry.isPrivate,
 				knownUsers: users,
@@ -303,21 +278,16 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner
 	}, nil
 }
 
-// entryExpired reports whether entry has reached the cache's bounded maximum
-// age, measured from its original creation time rather than its last access
-// time. Unlike the underlying cache2go table's own sliding expiry (which
-// resets on every read), this check ensures a frequently-accessed entry is
-// still forced to refresh once it is old enough, so stale trust decisions
-// cannot be kept alive indefinitely by repeated reads.
+// entryExpired reports whether entry has reached the cache's maximum age,
+// measured from creation. cache2go's own expiry instead slides on every read,
+// which would let repeated reads keep a stale decision alive indefinitely.
 func (c *RepoAccessCache) entryExpired(entry *repoAccessCacheEntry) bool {
 	if c.ttl <= 0 {
 		return false
 	}
 	return c.clock().Sub(entry.createdAt) >= c.ttl
 }
 
-// clock returns the current time, using the injected now function if set
-// (tests use this to exercise bounded expiry deterministically).
 func (c *RepoAccessCache) clock() time.Time {
 	if c.now != nil {
 		return c.now()
@@ -390,10 +360,8 @@ func (c *RepoAccessCache) isTrustedBot(username string) bool {
 	return ok
 }
 
-// cacheKey returns the entry key for owner/repo, prefixed with this cache's
-// identity digest when one is configured. Instances sharing a cache table are
-// kept isolated by this prefix rather than by separate tables, so every
-// identity's entries remain subject to the table's ordinary TTL cleanup.
+// cacheKey scopes the owner/repo key to this cache's identity, so identities
+// sharing a table cannot observe each other's entries.
 func (c *RepoAccessCache) cacheKey(owner, repo string) string {
 	key := fmt.Sprintf("%s/%s", strings.ToLower(owner), strings.ToLower(repo))
 	if c.identityDigest == "" {

pkg/lockdown/lockdown_test.go

@@ -134,13 +134,8 @@ func TestRepoAccessCacheEvictsAfterTTL(t *testing.T) {
 	require.EqualValues(t, 2, transport.CallCount())
 }
 
-// TestRepoAccessCacheBoundedExpiryIgnoresRepeatedAccess is a regression test for
-// issue #3107: a sliding-expiry cache extends an entry's life on every read, so
-// a frequently-accessed entry never refreshes even once revoked access should
-// have invalidated it. With bounded expiry, an entry's maximum age is measured
-// from its original creation, not its last access, so repeated reads within
-// the TTL are served from cache but the entry is still forced to refresh once
-// its absolute age exceeds the TTL.
+// Regression test for #3107: sliding expiry would let a frequently-read entry
+// outlive revoked access, so age must be bounded from creation.
 func TestRepoAccessCacheBoundedExpiryIgnoresRepeatedAccess(t *testing.T) {
 	ctx := t.Context()
 
@@ -154,31 +149,23 @@ func TestRepoAccessCacheBoundedExpiryIgnoresRepeatedAccess(t *testing.T) {
 	require.True(t, info.HasPushAccess)
 	require.EqualValues(t, 1, transport.CallCount())
 
-	// Repeatedly access the entry well within the window. A sliding-expiry
-	// cache would extend the entry's life on every one of these reads and
-	// never refresh it; bounded expiry must keep serving it from cache
-	// without making new upstream calls, since the absolute age is still
-	// under the TTL.
+	// Each read lands well inside the TTL; only their sum exceeds it.
 	for range 4 {
 		current = current.Add(20 * time.Second)
 		_, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
 		require.NoError(t, err)
 	}
 	require.EqualValues(t, 1, transport.CallCount(), "repeated access within the bounded window must still be served from cache")
 
-	// Cross the bound: total elapsed time since creation now exceeds the TTL,
-	// even though every individual access happened well inside it.
 	current = current.Add(30 * time.Second)
 	info, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
 	require.NoError(t, err)
 	require.True(t, info.HasPushAccess)
 	require.EqualValues(t, 2, transport.CallCount(), "entry must refresh once its absolute age exceeds the TTL, regardless of access frequency")
 }
 
-// TestRepoAccessCacheNewUserDoesNotResetEntryAge ensures that learning about a
-// newly-seen author on an existing repo entry does not reset the entry's
-// bounded creation time, which would otherwise re-introduce sliding behavior
-// through a different code path.
+// A "known users" miss updates an existing entry, a second path that must not
+// reset its age.
 func TestRepoAccessCacheNewUserDoesNotResetEntryAge(t *testing.T) {
 	ctx := t.Context()
 
@@ -194,16 +181,11 @@ func TestRepoAccessCacheNewUserDoesNotResetEntryAge(t *testing.T) {
 	require.NoError(t, err)
 	require.EqualValues(t, 1, transport.CallCount())
 
-	// A different, previously-unseen user triggers a "known users" miss but
-	// not a full entry miss, exercising the path that preserves createdAt.
 	cache.now = func() time.Time { return start.Add(50 * time.Second) }
 	_, err = cache.getRepoAccessInfo(ctx, "someone-else", testOwner, testRepo)
 	require.NoError(t, err)
 	require.EqualValues(t, 1, transport.CallCount(), "checking a new user against a cached repo entry must not re-query repo metadata")
 
-	// Total elapsed time since the entry's original creation now exceeds the
-	// TTL. If the new-user update above had reset createdAt, this would still
-	// be considered fresh (50s < 100s from the reset point); it must not be.
 	cache.now = func() time.Time { return start.Add(120 * time.Second) }
 	_, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
 	require.NoError(t, err)
@@ -233,8 +215,6 @@ func TestRepoAccessCacheIsolatesViewerPerInstance(t *testing.T) {
 	require.True(t, safe)
 }
 
-// TestRepoAccessCacheIdentityScopedKeys covers the key derivation that keeps
-// identities isolated inside a single shared cache table.
 func TestRepoAccessCacheIdentityScopedKeys(t *testing.T) {
 	restClient := newMockRESTServer(t, "write")
 	gqlClient, _ := newMockGQLClient(testUser, false)
@@ -259,13 +239,8 @@ func TestRepoAccessCacheIdentityScopedKeys(t *testing.T) {
 		"identity scoping must preserve owner/repo case-insensitivity")
 }
 
-// TestRepoAccessCacheIdentityScopingIsolatesWithinOneTable is a regression
-// test for issue #3107. Isolating identities by allocating a cache2go table
-// per token grows a process-wide registry that is never reclaimed, so
-// isolation must instead come from the entry key inside a single table. This
-// asserts both halves: different identities cannot see each other's trust
-// decisions, and their entries share one table so ordinary TTL cleanup can
-// reclaim them.
+// Regression test for #3107: a table per identity leaks, so isolation must come
+// from the entry key inside one table.
 func TestRepoAccessCacheIdentityScopingIsolatesWithinOneTable(t *testing.T) {
 	ctx := t.Context()
 
@@ -291,19 +266,14 @@ func TestRepoAccessCacheIdentityScopingIsolatesWithinOneTable(t *testing.T) {
 	require.EqualValues(t, 2, table.Count(),
 		"per-identity entries must be stored in one shared table rather than a table per identity")
 
-	// Repeating the same identity must hit the warm cache and must not
-	// allocate additional storage.
 	_, err = newCache(aliceGQL, "token-alice").getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
 	require.NoError(t, err)
 	require.EqualValues(t, 1, aliceTransport.CallCount(), "repeated requests from the same identity should reuse the warm cache")
 	require.EqualValues(t, 2, table.Count(), "a repeated request from a known identity must not add another entry")
 }
 
-// TestRepoAccessCacheIdentityScopedEntriesAreReclaimed proves the storage held
-// for distinct identities is bounded: because identity scoping lives in the
-// entry key, per-identity state is removed by the cache table's ordinary TTL
-// cleanup. A table-per-identity design could not shrink this way, since
-// cache2go retains every named table for the life of the process.
+// Key-scoped entries stay bounded because ordinary TTL cleanup reclaims them;
+// a table per identity could not shrink this way.
 func TestRepoAccessCacheIdentityScopedEntriesAreReclaimed(t *testing.T) {
 	ctx := t.Context()