Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
165 changes: 165 additions & 0 deletions pkg/github/inventory_cache.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
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 CachedInventoryBuilder is called.
//
// 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
initialized bool // set to true after init completes
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...)
}

c.initialized = true
})
}

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 should typically be initialized via InitInventoryCache at startup.
// If not already initialized when this function is called, it will automatically
// initialize with NullTranslationHelper as a fallback.
//
// 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 {
return globalInventoryCache.initialized
}

// ResetInventoryCache resets the global inventory cache, allowing it to be
// reinitialized with a different translator.
//
// WARNING: This function is for testing only. It is NOT thread-safe and must only
// be called when no other goroutines are accessing the cache. Tests using this
// function must not use t.Parallel().
func ResetInventoryCache() {
globalInventoryCache = &CachedInventory{}
}
Loading