fix(features): harden request resolution

Make feature checks reentrant and single-flight, validate rule declarations across short-circuit paths, cache inventory feature metadata, and codify the HTTP context boundary used by the remote server.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13

Author: SamMorrowDrums

docs/feature-flags.md

@@ -40,6 +40,19 @@ The service deduplicates the declared flags, resolves each one at most once for
 the request, and shares those values with tool dependencies. Feature checks
 inside handlers continue to use `deps.IsFeatureEnabled`.
 
+Feature predicates are pure and may depend only on their resolver. Construction
+validates every combination of up to 16 declared flags, so an undeclared lookup
+fails immediately even when ordinary evaluation would short-circuit that
+branch.
+
+The inventory's checker owns request feature state. Once installed, that state
+is authoritative; a checker stored on tool dependencies is used only as a
+fallback when handlers are invoked directly without request state. HTTP
+availability is resolved after outer HTTP middleware and the inventory factory
+run, but before MCP receiving middleware, because the tool set must be known
+before constructing the MCP server. Handler-only lazy checks use the live
+tool-call context.
+
 ---
 
 ## Tools affected by each flag

pkg/github/dependencies.go

@@ -203,9 +203,9 @@ func (d BaseDeps) Metrics(ctx context.Context) metrics.Metrics {
 // GetRequestStateSealer implements RequestStateSealerProvider.
 func (d BaseDeps) GetRequestStateSealer() RequestStateSealer { return d.StateSealer }
 
-// IsFeatureEnabled checks if a feature flag is enabled.
-// Returns false if the feature checker is nil, flag name is empty, or an error occurs.
-// This allows tools to conditionally change behavior based on feature flags.
+// IsFeatureEnabled checks if a feature flag is enabled. Request feature state
+// is authoritative when present; the dependency checker is a fallback for
+// direct handler invocation. Empty names and checker errors resolve false.
 func (d BaseDeps) IsFeatureEnabled(ctx context.Context, flag inventory.FeatureFlag) bool {
 	return inventory.ResolveFeature(ctx, d.featureChecker, flag)
 }
@@ -483,7 +483,9 @@ func (d *RequestDeps) Metrics(ctx context.Context) metrics.Metrics {
 	return d.obsv.Metrics(ctx)
 }
 
-// IsFeatureEnabled checks if a feature flag is enabled.
+// IsFeatureEnabled checks if a feature flag is enabled. Request feature state
+// is authoritative when present; the dependency checker is a fallback for
+// direct handler invocation.
 func (d *RequestDeps) IsFeatureEnabled(ctx context.Context, flag inventory.FeatureFlag) bool {
 	return inventory.ResolveFeature(ctx, d.featureChecker, flag)
 }

pkg/http/handler.go

@@ -23,6 +23,11 @@ import (
 
 const subscriptionsListenMethod = "subscriptions/listen"
 
+// InventoryFactoryFunc builds the inventory for one HTTP request. All context
+// values required by its feature checker must be installed before this runs:
+// feature availability is resolved immediately afterward, before MCP receiving
+// middleware can run. Handler-only lazy checks still see receiving-middleware
+// context.
 type InventoryFactoryFunc func(r *http.Request) (*inventory.Inventory, error)
 
 // GitHubMCPServerFactoryFunc is a function type for creating a new MCP Server instance.
@@ -214,6 +219,9 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	if methodInfo, ok := ghcontext.MCPMethod(r.Context()); ok && methodInfo != nil {
 		invToUse = inv.ForMCPRequest(methodInfo.Method, methodInfo.ItemName)
 	}
+	// Tool registration must know availability before the MCP server exists.
+	// Remote consumers install user identity in HTTP middleware before the
+	// inventory factory, so their per-user checker has its full context here.
 	r = r.WithContext(invToUse.WithResolvedFeatures(r.Context()))
 
 	ghServer, err := h.githubMcpServerFactory(r, h.deps, invToUse, &github.MCPServerConfig{

pkg/http/handler_test.go

@@ -1020,6 +1020,74 @@ func TestCrossOriginProtection(t *testing.T) {
 	}
 }
 
+func TestFeatureResolutionUsesOuterHTTPContext(t *testing.T) {
+	type userContextKey struct{}
+	const (
+		userValue   = "remote-user"
+		featureFlag = inventory.FeatureFlag("remote-feature")
+	)
+
+	var checkerCalls int
+	tool := mockTool("feature_tool", "test", true)
+	tool.FeatureRule = inventory.NewFeatureRule(
+		[]inventory.FeatureFlag{featureFlag},
+		func(featureAsBool inventory.FeatureResolver) bool {
+			return featureAsBool(featureFlag)
+		},
+	)
+	inventoryFactory := func(_ *http.Request) (*inventory.Inventory, error) {
+		checker := func(ctx context.Context, flag inventory.FeatureFlag) (bool, error) {
+			checkerCalls++
+			return flag == featureFlag && ctx.Value(userContextKey{}) == userValue, nil
+		}
+		return inventory.NewBuilder().
+			SetTools([]inventory.ServerTool{tool}).
+			WithToolsets([]string{"all"}).
+			WithFeatureChecker(checker).
+			Build()
+	}
+
+	apiHost, err := utils.NewAPIHost("https://api.github.com")
+	require.NoError(t, err)
+	handler := NewHTTPMcpHandler(
+		context.Background(),
+		&ServerConfig{Version: "test"},
+		nil,
+		translations.NullTranslationHelper,
+		slog.Default(),
+		apiHost,
+		WithInventoryFactory(inventoryFactory),
+		WithGitHubMCPServerFactory(func(r *http.Request, _ github.ToolDependencies, inv *inventory.Inventory, _ *github.MCPServerConfig) (*mcp.Server, error) {
+			assert.True(t, inventory.ResolveFeature(r.Context(), nil, featureFlag))
+			require.Len(t, inv.AvailableTools(r.Context()), 1)
+			return mcp.NewServer(&mcp.Implementation{Name: "test", Version: "0.0.1"}, nil), nil
+		}),
+		WithScopeFetcher(allScopesFetcher{}),
+	)
+
+	router := chi.NewRouter()
+	router.Use(func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			next.ServeHTTP(w, r.WithContext(context.WithValue(r.Context(), userContextKey{}, userValue)))
+		})
+	})
+	handler.RegisterMiddleware(router)
+	handler.RegisterRoutes(router)
+
+	body := `{"jsonrpc":"2.0","id":1,"method":"tools/list","params":{"_meta":{"io.modelcontextprotocol/protocolVersion":"2026-07-28","io.modelcontextprotocol/clientInfo":{"name":"test","version":"1.0.0"},"io.modelcontextprotocol/clientCapabilities":{}}}}`
+	req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body))
+	req.Header.Set(headers.ContentTypeHeader, headers.ContentTypeJSON)
+	req.Header.Set(headers.AcceptHeader, strings.Join([]string{headers.ContentTypeJSON, headers.ContentTypeEventStream}, ", "))
+	req.Header.Set("Mcp-Protocol-Version", "2026-07-28")
+	req.Header.Set("Mcp-Method", "tools/list")
+	req.Header.Set(headers.AuthorizationHeader, "ghs_test-token")
+
+	recorder := httptest.NewRecorder()
+	router.ServeHTTP(recorder, req)
+	require.Equal(t, http.StatusOK, recorder.Code, "response body: %s", recorder.Body.String())
+	assert.Equal(t, 1, checkerCalls)
+}
+
 func TestHTTPToolMinimumProtocolVersion(t *testing.T) {
 	apiHost, err := utils.NewAPIHost("https://api.github.com")
 	require.NoError(t, err)

pkg/inventory/builder.go

@@ -255,6 +255,8 @@ func (b *Builder) Build() (*Inventory, error) {
 		}
 	}
 
+	r.cacheFeatureMetadata()
+
 	if b.generateInstructions {
 		r.instructions = generateInstructions(r)
 	}

pkg/inventory/features.go

@@ -8,17 +8,22 @@ import (
 	"sync"
 )
 
+const maxFeatureRuleFlags = 16
+
 // FeatureFlag identifies a feature consistently across inventory consumers.
 type FeatureFlag string
 
-// FeatureFlagChecker resolves one feature flag for the current request.
+// FeatureFlagChecker resolves one feature flag for the current request. Every
+// context value needed for availability checks must be installed before the
+// inventory is resolved. Handler-only checks receive the live tool-call context.
 type FeatureFlagChecker func(ctx context.Context, flag FeatureFlag) (bool, error)
 
 // FeatureResolver returns the resolved value of a feature flag.
 // Implementations absorb resolution errors and fail closed.
 type FeatureResolver func(flag FeatureFlag) bool
 
-// FeaturePredicate determines whether an inventory item is available.
+// FeaturePredicate determines whether an inventory item is available. Predicates
+// must be pure: their result may depend only on calls to the supplied resolver.
 type FeaturePredicate func(featureAsBool FeatureResolver) bool
 
 // FeatureRule declares the feature flags used by an availability predicate.
@@ -44,11 +49,33 @@ func NewFeatureRule(features []FeatureFlag, predicate FeaturePredicate) FeatureR
 		featureSet[feature] = struct{}{}
 		declared = append(declared, feature)
 	}
-	return FeatureRule{
+	rule := FeatureRule{
 		features:   declared,
 		featureSet: featureSet,
 		predicate:  predicate,
 	}
+	rule.validate()
+	return rule
+}
+
+func (r FeatureRule) validate() {
+	if r.predicate == nil {
+		return
+	}
+	if len(r.features) > maxFeatureRuleFlags {
+		panic(fmt.Sprintf("feature rule declares %d flags; maximum is %d", len(r.features), maxFeatureRuleFlags))
+	}
+
+	for assignment := range 1 << len(r.features) {
+		r.evaluate(func(feature FeatureFlag) bool {
+			for i, declared := range r.features {
+				if feature == declared {
+					return assignment&(1<<i) != 0
+				}
+			}
+			return false
+		})
+	}
 }
 
 // Features returns the feature flags referenced by the rule.
@@ -69,7 +96,10 @@ func (r FeatureRule) Enabled(featureAsBool FeatureResolver) bool {
 	if featureAsBool == nil {
 		return false
 	}
+	return r.evaluate(featureAsBool)
+}
 
+func (r FeatureRule) evaluate(featureAsBool FeatureResolver) bool {
 	var undeclared FeatureFlag
 	usedUndeclared := false
 	enabled := r.predicate(func(feature FeatureFlag) bool {
@@ -81,25 +111,35 @@ func (r FeatureRule) Enabled(featureAsBool FeatureResolver) bool {
 		return featureAsBool(feature)
 	})
 	if usedUndeclared {
-		fmt.Fprintf(os.Stderr, "Feature rule used undeclared feature %q\n", undeclared)
-		return false
+		panic(fmt.Sprintf("feature rule used undeclared feature %q", undeclared))
 	}
 	return enabled
 }
 
 type featureStateContextKey struct{}
+type resolvingFeatureContextKey struct{}
 
 type featureState struct {
 	checker FeatureFlagChecker
 
-	mu     sync.Mutex
-	values map[FeatureFlag]bool
+	mu      sync.Mutex
+	results map[FeatureFlag]*featureResult
+}
+
+type featureResult struct {
+	ready   chan struct{}
+	enabled bool
+}
+
+type resolvingFeature struct {
+	flag   FeatureFlag
+	parent *resolvingFeature
 }
 
 func newFeatureState(checker FeatureFlagChecker) *featureState {
 	return &featureState{
 		checker: checker,
-		values:  make(map[FeatureFlag]bool),
+		results: make(map[FeatureFlag]*featureResult),
 	}
 }
 
@@ -108,24 +148,60 @@ func (s *featureState) enabled(ctx context.Context, feature FeatureFlag) bool {
 		return false
 	}
 
-	s.mu.Lock()
-	defer s.mu.Unlock()
+	for current := resolvingFeatureFromContext(ctx); current != nil; current = current.parent {
+		if current.flag == feature {
+			fmt.Fprintf(os.Stderr, "Feature flag resolution cycle detected for %q\n", feature)
+			return false
+		}
+	}
 
-	if enabled, ok := s.values[feature]; ok {
-		return enabled
+	s.mu.Lock()
+	result, found := s.results[feature]
+	if !found {
+		result = &featureResult{ready: make(chan struct{})}
+		s.results[feature] = result
+	}
+	s.mu.Unlock()
+
+	if found {
+		select {
+		case <-result.ready:
+			return result.enabled
+		case <-ctx.Done():
+			return false
+		}
 	}
 
-	enabled, err := s.checker(ctx, feature)
+	completed := false
+	defer func() {
+		if !completed {
+			close(result.ready)
+		}
+	}()
+
+	resolutionCtx := context.WithValue(ctx, resolvingFeatureContextKey{}, &resolvingFeature{
+		flag:   feature,
+		parent: resolvingFeatureFromContext(ctx),
+	})
+	enabled, err := s.checker(resolutionCtx, feature)
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", feature, err)
 		enabled = false
 	}
-	s.values[feature] = enabled
+	result.enabled = enabled
+	completed = true
+	close(result.ready)
 	return enabled
 }
 
+func resolvingFeatureFromContext(ctx context.Context) *resolvingFeature {
+	feature, _ := ctx.Value(resolvingFeatureContextKey{}).(*resolvingFeature)
+	return feature
+}
+
 // WithResolvedFeatures resolves the deduplicated feature names into state owned
-// by the returned context. Repeated calls extend and reuse that state.
+// by the returned context. Repeated calls extend and reuse that state. When state
+// already exists, its checker is authoritative and checker is ignored.
 func WithResolvedFeatures(ctx context.Context, checker FeatureFlagChecker, features []FeatureFlag) context.Context {
 	state, _ := ctx.Value(featureStateContextKey{}).(*featureState)
 	if state == nil {
@@ -145,18 +221,20 @@ func WithResolvedFeatures(ctx context.Context, checker FeatureFlagChecker, featu
 }
 
 // ResolveFeature returns a feature value from request-owned resolution state.
-// Features not resolved up front are resolved lazily and cached.
-func ResolveFeature(ctx context.Context, checker FeatureFlagChecker, feature FeatureFlag) bool {
+// Context state and its checker are authoritative. fallbackChecker is used only
+// when the context has no state; that uncached compatibility path lets handlers
+// invoked directly outside a server continue to resolve features.
+func ResolveFeature(ctx context.Context, fallbackChecker FeatureFlagChecker, feature FeatureFlag) bool {
 	if feature == "" {
 		return false
 	}
 	if state, _ := ctx.Value(featureStateContextKey{}).(*featureState); state != nil {
 		return state.enabled(ctx, feature)
 	}
-	if checker == nil {
+	if fallbackChecker == nil {
 		return false
 	}
-	return newFeatureState(checker).enabled(ctx, feature)
+	return newFeatureState(fallbackChecker).enabled(ctx, feature)
 }
 
 func featureResolver(ctx context.Context, checker FeatureFlagChecker) FeatureResolver {
@@ -173,19 +251,3 @@ func featureResolver(ctx context.Context, checker FeatureFlagChecker) FeatureRes
 		return state.enabled(ctx, feature)
 	}
 }
-
-func collectFeatures(rules ...FeatureRule) []FeatureFlag {
-	seen := make(map[FeatureFlag]struct{})
-	for _, rule := range rules {
-		for _, feature := range rule.features {
-			seen[feature] = struct{}{}
-		}
-	}
-
-	features := make([]FeatureFlag, 0, len(seen))
-	for feature := range seen {
-		features = append(features, feature)
-	}
-	slices.Sort(features)
-	return features
-}