fix(features): close cycle and ownership gaps

Preserve the public string dependency API, detect concurrent resolution cycles without blocking, and isolate cached feature metadata from caller mutation.

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

Author: SamMorrowDrums

pkg/github/csv_output_test.go

@@ -366,7 +366,7 @@ type csvOutputTestDeps struct {
 	csvOn bool
 }
 
-func (d csvOutputTestDeps) IsFeatureEnabled(_ context.Context, flag inventory.FeatureFlag) bool {
+func (d csvOutputTestDeps) IsFeatureEnabled(_ context.Context, flag string) bool {
 	return flag == FeatureFlagCSVOutput && d.csvOn
 }
 

pkg/github/dependencies.go

@@ -94,7 +94,7 @@ type ToolDependencies interface {
 	GetContentWindowSize() int
 
 	// IsFeatureEnabled checks if a feature flag is enabled.
-	IsFeatureEnabled(ctx context.Context, flag inventory.FeatureFlag) bool
+	IsFeatureEnabled(ctx context.Context, flag string) bool
 
 	// Logger returns the structured logger, optionally enriched with
 	// request-scoped data from ctx. Integrators provide their own slog.Handler
@@ -206,8 +206,8 @@ func (d BaseDeps) GetRequestStateSealer() RequestStateSealer { return d.StateSea
 // 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)
+func (d BaseDeps) IsFeatureEnabled(ctx context.Context, flag string) bool {
+	return inventory.ResolveFeature(ctx, d.featureChecker, inventory.FeatureFlag(flag))
 }
 
 // NewTool creates a ServerTool that retrieves ToolDependencies from context at call time.
@@ -486,6 +486,6 @@ func (d *RequestDeps) Metrics(ctx context.Context) metrics.Metrics {
 // 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)
+func (d *RequestDeps) IsFeatureEnabled(ctx context.Context, flag string) bool {
+	return inventory.ResolveFeature(ctx, d.featureChecker, inventory.FeatureFlag(flag))
 }

pkg/github/feature_flags_test.go

@@ -47,7 +47,7 @@ func HelloWorldTool(t translations.TranslationHelperFunc) inventory.ServerTool {
 
 			// Check feature flag to determine greeting style
 			greeting := "Hello, world!"
-			if deps.IsFeatureEnabled(ctx, RemoteMCPEnthusiasticGreeting) {
+			if deps.IsFeatureEnabled(ctx, string(RemoteMCPEnthusiasticGreeting)) {
 				greeting += " Welcome to the future of MCP! 🎉"
 			}
 

pkg/github/server_test.go

@@ -65,7 +65,7 @@ func (s stubDeps) GetRepoAccessCache(_ context.Context) (*lockdown.RepoAccessCac
 func (s stubDeps) GetT() translations.TranslationHelperFunc { return s.t }
 func (s stubDeps) GetFlags(_ context.Context) FeatureFlags  { return s.flags }
 func (s stubDeps) GetContentWindowSize() int                { return s.contentWindowSize }
-func (s stubDeps) IsFeatureEnabled(_ context.Context, _ inventory.FeatureFlag) bool {
+func (s stubDeps) IsFeatureEnabled(_ context.Context, _ string) bool {
 	return false
 }
 func (s stubDeps) Logger(_ context.Context) *slog.Logger {

pkg/inventory/builder.go

@@ -23,7 +23,9 @@ const mcpAppsFeatureFlag FeatureFlag = "remote_mcp_ui_apps"
 // Returns true if the tool should be included, false to exclude it.
 type ToolFilter func(ctx context.Context, tool *ServerTool) (bool, error)
 
-// Builder builds a Registry with the specified configuration.
+// Builder builds a Registry with the specified configuration. SetTools,
+// SetResources, and SetPrompts copy the feature and metadata state retained by
+// the inventory.
 // Use NewBuilder to create a builder, chain configuration methods,
 // then call Build() to create the final inventory.
 //
@@ -65,22 +67,49 @@ func NewBuilder() *Builder {
 
 // SetTools sets the tools for the inventory. Returns self for chaining.
 func (b *Builder) SetTools(tools []ServerTool) *Builder {
-	b.tools = tools
+	b.tools = slices.Clone(tools)
+	for i := range b.tools {
+		b.tools[i] = cloneServerTool(b.tools[i])
+	}
 	return b
 }
 
 // SetResources sets the resource templates for the inventory. Returns self for chaining.
 func (b *Builder) SetResources(resources []ServerResourceTemplate) *Builder {
-	b.resourceTemplates = resources
+	b.resourceTemplates = slices.Clone(resources)
+	for i := range b.resourceTemplates {
+		b.resourceTemplates[i] = cloneResourceTemplate(b.resourceTemplates[i])
+	}
 	return b
 }
 
 // SetPrompts sets the prompts for the inventory. Returns self for chaining.
 func (b *Builder) SetPrompts(prompts []ServerPrompt) *Builder {
-	b.prompts = prompts
+	b.prompts = slices.Clone(prompts)
+	for i := range b.prompts {
+		b.prompts[i] = clonePrompt(b.prompts[i])
+	}
 	return b
 }
 
+func cloneServerTool(tool ServerTool) ServerTool {
+	tool.Tool.Meta = maps.Clone(tool.Tool.Meta)
+	tool.FeatureRule = tool.FeatureRule.clone()
+	return tool
+}
+
+func cloneResourceTemplate(resource ServerResourceTemplate) ServerResourceTemplate {
+	resource.Template.Meta = maps.Clone(resource.Template.Meta)
+	resource.FeatureRule = resource.FeatureRule.clone()
+	return resource
+}
+
+func clonePrompt(prompt ServerPrompt) ServerPrompt {
+	prompt.Prompt.Meta = maps.Clone(prompt.Prompt.Meta)
+	prompt.FeatureRule = prompt.FeatureRule.clone()
+	return prompt
+}
+
 // WithDeprecatedAliases adds deprecated tool name aliases that map to canonical names.
 // Returns self for chaining.
 func (b *Builder) WithDeprecatedAliases(aliases map[string]string) *Builder {

pkg/inventory/features.go

@@ -3,6 +3,7 @@ package inventory
 import (
 	"context"
 	"fmt"
+	"maps"
 	"os"
 	"slices"
 	"sync"
@@ -35,6 +36,14 @@ type FeatureRule struct {
 	predicate  FeaturePredicate
 }
 
+func (r FeatureRule) clone() FeatureRule {
+	return FeatureRule{
+		features:   slices.Clone(r.features),
+		featureSet: maps.Clone(r.featureSet),
+		predicate:  r.predicate,
+	}
+}
+
 // NewFeatureRule creates an availability rule over the supplied feature flags.
 func NewFeatureRule(features []FeatureFlag, predicate FeaturePredicate) FeatureRule {
 	declared := make([]FeatureFlag, 0, len(features))
@@ -124,22 +133,25 @@ type featureState struct {
 
 	mu      sync.Mutex
 	results map[FeatureFlag]*featureResult
+	waiting map[FeatureFlag]map[FeatureFlag]int
 }
 
 type featureResult struct {
 	ready   chan struct{}
 	enabled bool
+	failed  bool
+	done    bool
 }
 
 type resolvingFeature struct {
-	flag   FeatureFlag
-	parent *resolvingFeature
+	flag FeatureFlag
 }
 
 func newFeatureState(checker FeatureFlagChecker) *featureState {
 	return &featureState{
 		checker: checker,
 		results: make(map[FeatureFlag]*featureResult),
+		waiting: make(map[FeatureFlag]map[FeatureFlag]int),
 	}
 }
 
@@ -148,21 +160,34 @@ func (s *featureState) enabled(ctx context.Context, feature FeatureFlag) bool {
 		return false
 	}
 
-	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
-		}
-	}
-
+	owner := resolvingFeatureFromContext(ctx)
 	s.mu.Lock()
 	result, found := s.results[feature]
+	if found && result.done {
+		enabled := result.enabled
+		s.mu.Unlock()
+		return enabled
+	}
 	if !found {
 		result = &featureResult{ready: make(chan struct{})}
 		s.results[feature] = result
 	}
+	if owner != nil {
+		if path := s.pathLocked(feature, owner.flag, nil); path != nil {
+			s.failCycleLocked(owner.flag, path)
+			s.mu.Unlock()
+			return false
+		}
+		if s.waiting[owner.flag] == nil {
+			s.waiting[owner.flag] = make(map[FeatureFlag]int)
+		}
+		s.waiting[owner.flag][feature]++
+	}
 	s.mu.Unlock()
 
+	if owner != nil {
+		defer s.clearWait(owner.flag, feature)
+	}
 	if found {
 		select {
 		case <-result.ready:
@@ -175,25 +200,77 @@ func (s *featureState) enabled(ctx context.Context, feature FeatureFlag) bool {
 	completed := false
 	defer func() {
 		if !completed {
+			s.mu.Lock()
+			result.failed = true
+			result.done = true
 			close(result.ready)
+			s.mu.Unlock()
 		}
 	}()
 
-	resolutionCtx := context.WithValue(ctx, resolvingFeatureContextKey{}, &resolvingFeature{
-		flag:   feature,
-		parent: resolvingFeatureFromContext(ctx),
-	})
+	resolutionCtx := context.WithValue(ctx, resolvingFeatureContextKey{}, &resolvingFeature{flag: feature})
 	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.mu.Lock()
+	if result.failed {
+		enabled = false
+	}
 	result.enabled = enabled
+	result.done = true
 	completed = true
 	close(result.ready)
+	s.mu.Unlock()
 	return enabled
 }
 
+func (s *featureState) pathLocked(current, target FeatureFlag, seen map[FeatureFlag]bool) []FeatureFlag {
+	if current == target {
+		return []FeatureFlag{current}
+	}
+	if seen == nil {
+		seen = make(map[FeatureFlag]bool)
+	}
+	if seen[current] {
+		return nil
+	}
+	seen[current] = true
+	for next := range s.waiting[current] {
+		if path := s.pathLocked(next, target, seen); path != nil {
+			return append([]FeatureFlag{current}, path...)
+		}
+	}
+	return nil
+}
+
+func (s *featureState) failCycleLocked(owner FeatureFlag, path []FeatureFlag) {
+	if result := s.results[owner]; result != nil {
+		result.failed = true
+	}
+	for _, feature := range path {
+		if result := s.results[feature]; result != nil {
+			result.failed = true
+		}
+	}
+	fmt.Fprintf(os.Stderr, "Feature flag resolution cycle detected for %q\n", path[0])
+}
+
+func (s *featureState) clearWait(owner, target FeatureFlag) {
+	s.mu.Lock()
+	if targets := s.waiting[owner]; targets != nil {
+		targets[target]--
+		if targets[target] == 0 {
+			delete(targets, target)
+		}
+	}
+	if len(s.waiting[owner]) == 0 {
+		delete(s.waiting, owner)
+	}
+	s.mu.Unlock()
+}
+
 func resolvingFeatureFromContext(ctx context.Context) *resolvingFeature {
 	feature, _ := ctx.Value(resolvingFeatureContextKey{}).(*resolvingFeature)
 	return feature

pkg/inventory/features_test.go

@@ -131,7 +131,7 @@ func TestFeatureResolutionIsReentrantAcrossFlags(t *testing.T) {
 	assert.True(t, ResolveFeature(ctx, nil, "meta"))
 }
 
-func TestFeatureResolutionCycleFailsClosed(t *testing.T) {
+func TestDirectFeatureResolutionCycleFailsClosed(t *testing.T) {
 	var checker FeatureFlagChecker
 	checker = func(ctx context.Context, flag FeatureFlag) (bool, error) {
 		return ResolveFeature(ctx, checker, flag), nil
@@ -141,6 +141,34 @@ func TestFeatureResolutionCycleFailsClosed(t *testing.T) {
 	assert.False(t, ResolveFeature(ctx, nil, "cycle"))
 }
 
+func TestSelfNegatingFeatureResolutionCycleFailsClosed(t *testing.T) {
+	var checker FeatureFlagChecker
+	checker = func(ctx context.Context, flag FeatureFlag) (bool, error) {
+		return !ResolveFeature(ctx, checker, flag), nil
+	}
+
+	ctx := WithResolvedFeatures(context.Background(), checker, nil)
+	assert.False(t, ResolveFeature(ctx, nil, "cycle"))
+}
+
+func TestMutualFeatureResolutionCycleFailsClosed(t *testing.T) {
+	var checker FeatureFlagChecker
+	checker = func(ctx context.Context, flag FeatureFlag) (bool, error) {
+		switch flag {
+		case "a":
+			return !ResolveFeature(ctx, checker, "b"), nil
+		case "b":
+			return !ResolveFeature(ctx, checker, "a"), nil
+		default:
+			return false, nil
+		}
+	}
+
+	ctx := WithResolvedFeatures(context.Background(), checker, nil)
+	assert.False(t, ResolveFeature(ctx, nil, "a"))
+	assert.False(t, ResolveFeature(ctx, nil, "b"))
+}
+
 func TestConcurrentFeatureResolutionIsDeduplicated(t *testing.T) {
 	var (
 		calls   int
@@ -177,6 +205,40 @@ func TestConcurrentFeatureResolutionIsDeduplicated(t *testing.T) {
 	callsMu.Unlock()
 }
 
+func TestConcurrentCrossFeatureCycleFailsClosed(t *testing.T) {
+	startedA := make(chan struct{})
+	startedB := make(chan struct{})
+	var checker FeatureFlagChecker
+	checker = func(ctx context.Context, flag FeatureFlag) (bool, error) {
+		switch flag {
+		case "a":
+			close(startedA)
+			<-startedB
+			return !ResolveFeature(ctx, checker, "b"), nil
+		case "b":
+			close(startedB)
+			<-startedA
+			return !ResolveFeature(ctx, checker, "a"), nil
+		default:
+			return false, nil
+		}
+	}
+
+	ctx := WithResolvedFeatures(context.Background(), checker, nil)
+	results := make(chan bool, 2)
+	go func() { results <- ResolveFeature(ctx, nil, "a") }()
+	go func() { results <- ResolveFeature(ctx, nil, "b") }()
+
+	for range 2 {
+		select {
+		case result := <-results:
+			assert.False(t, result)
+		case <-time.After(time.Second):
+			t.Fatal("concurrent feature cycle deadlocked")
+		}
+	}
+}
+
 func TestContextFeatureStateTakesPrecedence(t *testing.T) {
 	var fallbackCalls int
 	stateChecker := func(context.Context, FeatureFlag) (bool, error) {