Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 160 additions & 0 deletions pkg/github/inventory_cache.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
package github

import (
"sync"

"github.com/github/github-mcp-server/pkg/inventory"
"github.com/github/github-mcp-server/pkg/translations"
)

// CachedInventory provides a cached inventory builder that builds tool definitions
// only once, regardless of how many times NewInventoryBuilder is called.
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment uses inconsistent terminology. The comment says "NewInventoryBuilder" but the actual function is called "CachedInventoryBuilder". This creates confusion about which function is being referred to.

Suggested change
// only once, regardless of how many times NewInventoryBuilder is called.
// only once, regardless of how many times CachedInventoryBuilder is called.

Copilot uses AI. Check for mistakes.
//
// This is particularly useful for stateless server patterns (like the remote server)
// where a new server instance is created per request. Without caching, every request
// would rebuild all ~130 tool definitions including JSON schema generation, causing
// significant performance overhead.
//
// Usage:
//
// // Option 1: Initialize once at startup with your translator
// github.InitInventoryCache(myTranslator)
//
// // Then get pre-built inventory on each request
// inv := github.CachedInventoryBuilder().
// WithReadOnly(cfg.ReadOnly).
// WithToolsets(cfg.Toolsets).
// Build()
//
// // Option 2: Use NewInventory which doesn't use the cache (legacy behavior)
// inv := github.NewInventory(myTranslator).Build()
//
// The cache stores the built []ServerTool, []ServerResourceTemplate, and []ServerPrompt.
// Per-request configuration (read-only, toolsets, feature flags, filters) is still
// applied when building the Inventory from the cached data.
type CachedInventory struct {
once sync.Once
tools []inventory.ServerTool
resources []inventory.ServerResourceTemplate
prompts []inventory.ServerPrompt
}

// global singleton for caching
var globalInventoryCache = &CachedInventory{}

// InitInventoryCache initializes the global inventory cache with the given translator.
// This should be called once at startup before any requests are processed.
// It's safe to call multiple times - only the first call has any effect.
//
// For the local server, this is typically called with the configured translator.
// For the remote server, use translations.NullTranslationHelper since translations
// aren't needed per-request.
//
// Example:
//
// func main() {
// t, _ := translations.TranslationHelper()
// github.InitInventoryCache(t)
// // ... start server
// }
func InitInventoryCache(t translations.TranslationHelperFunc) {
globalInventoryCache.init(t, nil, nil, nil)
}

// InitInventoryCacheWithExtras initializes the global inventory cache with the given
// translator plus additional tools, resources, and prompts.
//
// This is useful for the remote server which has additional tools (e.g., Copilot tools)
// that aren't part of the base github-mcp-server package.
//
// The extra items are appended to the base items from AllTools/AllResources/AllPrompts.
// It's safe to call multiple times - only the first call has any effect.
//
// Example:
//
// func init() {
// github.InitInventoryCacheWithExtras(
// translations.NullTranslationHelper,
Comment on lines +77 to +78
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation is contradictory. Line 76 states "The cache must be initialized via InitInventoryCache before calling this function" but line 77 immediately says "If the cache is not initialized, this will initialize it with NullTranslationHelper."

This creates confusion about whether initialization is required or optional. Consider rephrasing to clarify the behavior, such as: "The cache should typically be initialized via InitInventoryCache before calling this function. If not already initialized, it will automatically initialize with NullTranslationHelper as a fallback."

Copilot uses AI. Check for mistakes.
// remoteOnlyTools, // []inventory.ServerTool
// remoteOnlyResources, // []inventory.ServerResourceTemplate
// remoteOnlyPrompts, // []inventory.ServerPrompt
// )
// }
func InitInventoryCacheWithExtras(
t translations.TranslationHelperFunc,
extraTools []inventory.ServerTool,
extraResources []inventory.ServerResourceTemplate,
extraPrompts []inventory.ServerPrompt,
) {
globalInventoryCache.init(t, extraTools, extraResources, extraPrompts)
}

// init initializes the cache with the given translator and optional extras (sync.Once protected).
func (c *CachedInventory) init(
t translations.TranslationHelperFunc,
extraTools []inventory.ServerTool,
extraResources []inventory.ServerResourceTemplate,
extraPrompts []inventory.ServerPrompt,
) {
c.once.Do(func() {
c.tools = AllTools(t)
c.resources = AllResources(t)
c.prompts = AllPrompts(t)

// Append extra items if provided
if len(extraTools) > 0 {
c.tools = append(c.tools, extraTools...)
}
if len(extraResources) > 0 {
Comment on lines +106 to +109
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IsCacheInitialized check is unreliable. It checks if len(globalInventoryCache.tools) > 0, but this has two problems:

  1. If AllTools(t) returns an empty slice (which shouldn't happen in practice but is theoretically possible), IsCacheInitialized would return false even after successful initialization
  2. After ResetInventoryCache() is called, the check looks at the new instance which will have empty tools, making it work correctly. However, the comment at line 106 says "We can't directly check sync.Once state" which suggests the intent is to check if the Once has fired, not just if tools are populated.

A more reliable approach would be to add an explicit boolean flag that's set to true inside the once.Do callback, which would accurately reflect whether initialization has occurred regardless of whether AllTools returns an empty slice.

Copilot uses AI. Check for mistakes.
c.resources = append(c.resources, extraResources...)
}
if len(extraPrompts) > 0 {
c.prompts = append(c.prompts, extraPrompts...)
}
})
}

Comment on lines +117 to +119
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ResetInventoryCache function creates a race condition. After resetting globalInventoryCache to a new instance, the old sync.Once in the previous instance is lost. If another goroutine was in the middle of calling init() on the old instance, it would complete initialization on the old instance while new callers would get the new uninitialized instance. Additionally, resetting doesn't prevent the old sync.Once from keeping the "already called" state.

While the comment correctly warns this is test-only and not thread-safe, a safer approach would be to reset the fields inside the existing CachedInventory struct rather than replacing the entire global variable. However, sync.Once cannot be reset by design.

Consider adding a mutex-based solution for tests or document more clearly that tests must ensure serial execution when calling ResetInventoryCache.

Copilot uses AI. Check for mistakes.
// CachedInventoryBuilder returns an inventory.Builder pre-populated with cached
// tool/resource/prompt definitions.
//
// The cache must be initialized via InitInventoryCache before calling this function.
// If the cache is not initialized, this will initialize it with NullTranslationHelper.
//
// Per-request configuration can still be applied via the builder methods:
// - WithReadOnly(bool) - filter to read-only tools
// - WithToolsets([]string) - enable specific toolsets
// - WithTools([]string) - enable specific tools
// - WithFeatureChecker(func) - per-request feature flag evaluation
// - WithFilter(func) - custom filtering
//
// Example:
//
// inv := github.CachedInventoryBuilder().
// WithReadOnly(cfg.ReadOnly).
// WithToolsets(cfg.EnabledToolsets).
// WithFeatureChecker(createFeatureChecker(cfg.EnabledFeatures)).
// Build()
func CachedInventoryBuilder() *inventory.Builder {
// Ensure cache is initialized (with NullTranslationHelper as fallback)
globalInventoryCache.init(translations.NullTranslationHelper, nil, nil, nil)

return inventory.NewBuilder().
SetTools(globalInventoryCache.tools).
SetResources(globalInventoryCache.resources).
SetPrompts(globalInventoryCache.prompts)
}

// IsCacheInitialized returns true if the inventory cache has been initialized.
// This is primarily useful for testing.
func IsCacheInitialized() bool {
// We can't directly check sync.Once state, but we can check if tools are populated
return len(globalInventoryCache.tools) > 0
}

// ResetInventoryCache resets the global inventory cache, allowing it to be
// reinitialized with a different translator. This should only be used in tests.
//
// WARNING: This is not thread-safe and should never be called in production code.
func ResetInventoryCache() {
globalInventoryCache = &CachedInventory{}
}
Loading