Skip to content

Commit ac54ad6

Browse files
refactor(features): simplify lazy resolution
Preserve string checker APIs, evaluate functional rules lazily through one request memo, remove metadata normalization and eager feature caches, and add remote-sized benchmarks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13
1 parent f479917 commit ac54ad6

22 files changed

Lines changed: 427 additions & 660 deletions

cmd/github-mcp-server/feature_flag_docs.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,8 @@ func flaggedToolDiff(t translations.TranslationHelperFunc, flag string, defaultT
100100
// the given flags as enabled and every other flag as disabled. Passing nil
101101
// produces the default-flagged inventory.
102102
func buildInventoryWithFlags(t translations.TranslationHelperFunc, enabled map[string]bool) *inventory.Inventory {
103-
checker := func(_ context.Context, flag inventory.FeatureFlag) (bool, error) {
104-
return enabled[string(flag)], nil
103+
checker := func(_ context.Context, flag string) (bool, error) {
104+
return enabled[flag], nil
105105
}
106106
inv, _ := github.NewInventory(t).
107107
WithToolsets([]string{"all"}).

cmd/github-mcp-server/generate_docs.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ func init() {
3131

3232
// noFeatureFlagsChecker reports every feature flag as disabled. It models the
3333
// default user experience used by the generated documentation.
34-
func noFeatureFlagsChecker(_ context.Context, _ inventory.FeatureFlag) (bool, error) {
34+
func noFeatureFlagsChecker(_ context.Context, _ string) (bool, error) {
3535
return false, nil
3636
}
3737

docs/feature-flags.md

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -51,22 +51,25 @@ tool.FeatureRule = inventory.NewFeatureRule(
5151
)
5252
```
5353

54-
The service deduplicates the declared flags, resolves each one at most once for
55-
the request, and shares those values with tool dependencies. Feature checks
56-
inside handlers continue to use `deps.IsFeatureEnabled`.
54+
Library consumers migrating existing inventory declarations should replace
55+
`FeatureFlagEnable`, `FeatureFlagEnableAll`, and `FeatureFlagDisable` on
56+
`ServerTool`, `ServerResourceTemplate`, and `ServerPrompt` with `FeatureRule`.
57+
`FeatureFlagChecker` and `ToolDependencies.IsFeatureEnabled` continue to accept
58+
string flag names.
59+
60+
Rules are evaluated lazily after request narrowing. Normal Go short-circuiting
61+
avoids checks that cannot affect the result, while one request-owned memo ensures
62+
each flag actually reached is resolved at most once across tools, resources,
63+
prompts, and `deps.IsFeatureEnabled`.
5764

5865
Feature predicates are pure and may depend only on their resolver. Construction
5966
validates every combination of up to 16 declared flags, so an undeclared lookup
6067
fails immediately even when ordinary evaluation would short-circuit that
6168
branch.
6269

63-
The inventory's checker owns request feature state. Once installed, that state
64-
is authoritative; a checker stored on tool dependencies is used only as a
65-
fallback when handlers are invoked directly without request state. HTTP
66-
availability is resolved after outer HTTP middleware and the inventory factory
67-
run, but before MCP receiving middleware, because the tool set must be known
68-
before constructing the MCP server. Handler-only lazy checks use the live
69-
tool-call context.
70+
The inventory's string-based checker owns request feature state. Once installed,
71+
that state is authoritative; a checker stored on tool dependencies is used only
72+
as a fallback when handlers are invoked directly without request state.
7073

7174
---
7275

docs/insiders-features.md

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -207,12 +207,10 @@ Insiders is a **meta feature flag** — the same shape as `default` or `all` for
207207
3. **Insiders expansion.** If insiders mode is on (`--insiders`, `/insiders` route, or `X-MCP-Insiders: true`), every flag in [`InsidersFeatureFlags`](../pkg/github/feature_flags.go) is unioned in. The insiders expansion is **not** re-validated against the allowlist — insiders is a server-controlled switch that can reach internal-only flags.
208208
4. **Server-side fallback (remote server only).** Any flag not yet decided falls back to the remote server's feature manager, which can roll a feature out independently of user input or insiders membership.
209209

210-
For tool availability, each functional feature rule statically declares the
211-
flags it reads. The service deduplicates those declarations, resolves every
212-
relevant flag once into request-owned state, and then evaluates all rules as
213-
in-memory boolean expressions. The same state backs
214-
`deps.IsFeatureEnabled`, so checks made inside a tool call reuse resolved values
215-
and lazily cache any handler-only flag using the live tool-call context.
210+
For tool availability, functional rules declare the flags they may read and are
211+
evaluated lazily after request narrowing. Short-circuiting skips unnecessary
212+
checks, and request-owned state memoizes each flag that is reached. The same
213+
state backs `deps.IsFeatureEnabled`.
216214

217215
`AllowedFeatureFlags` and `InsidersFeatureFlags` are deliberately independent sets:
218216

internal/ghmcp/server.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -436,8 +436,8 @@ func RunStdioServer(cfg StdioServerConfig) error {
436436
// features are resolved once at startup from --features CLI flag and insiders mode.
437437
func createFeatureChecker(enabledFeatures []string, insidersMode bool) inventory.FeatureFlagChecker {
438438
featureSet := github.ResolveFeatureFlags(enabledFeatures, insidersMode)
439-
return func(_ context.Context, flagName inventory.FeatureFlag) (bool, error) {
440-
return featureSet[string(flagName)], nil
439+
return func(_ context.Context, flagName string) (bool, error) {
440+
return featureSet[flagName], nil
441441
}
442442
}
443443

pkg/github/context_tools_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99

1010
"github.com/github/github-mcp-server/internal/githubv4mock"
1111
"github.com/github/github-mcp-server/internal/toolsnaps"
12-
"github.com/github/github-mcp-server/pkg/inventory"
1312
"github.com/github/github-mcp-server/pkg/translations"
1413
"github.com/google/go-github/v89/github"
1514
"github.com/modelcontextprotocol/go-sdk/mcp"
@@ -190,7 +189,7 @@ func Test_GetMe_IFC_FeatureFlag(t *testing.T) {
190189
translations.NullTranslationHelper,
191190
FeatureFlags{},
192191
0,
193-
func(_ context.Context, flagName inventory.FeatureFlag) (bool, error) {
192+
func(_ context.Context, flagName string) (bool, error) {
194193
return flagName == FeatureFlagIFCLabels && enabled, nil
195194
},
196195
stubExporters(),

pkg/github/dependencies_test.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import (
1414
ghcontext "github.com/github/github-mcp-server/pkg/context"
1515
"github.com/github/github-mcp-server/pkg/github"
1616
"github.com/github/github-mcp-server/pkg/http/headers"
17-
"github.com/github/github-mcp-server/pkg/inventory"
1817
"github.com/github/github-mcp-server/pkg/observability"
1918
"github.com/github/github-mcp-server/pkg/observability/metrics"
2019
"github.com/github/github-mcp-server/pkg/translations"
@@ -203,7 +202,7 @@ func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) {
203202
t.Parallel()
204203

205204
// Create a feature checker that returns true for "test_flag"
206-
checker := func(_ context.Context, flagName inventory.FeatureFlag) (bool, error) {
205+
checker := func(_ context.Context, flagName string) (bool, error) {
207206
return flagName == "test_flag", nil
208207
}
209208

@@ -254,7 +253,7 @@ func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) {
254253
t.Parallel()
255254

256255
// Create a feature checker
257-
checker := func(_ context.Context, _ inventory.FeatureFlag) (bool, error) {
256+
checker := func(_ context.Context, _ string) (bool, error) {
258257
return true, nil
259258
}
260259

@@ -389,7 +388,7 @@ func TestIsFeatureEnabled_CheckerError(t *testing.T) {
389388
t.Parallel()
390389

391390
// Create a feature checker that returns an error
392-
checker := func(_ context.Context, _ inventory.FeatureFlag) (bool, error) {
391+
checker := func(_ context.Context, _ string) (bool, error) {
393392
return false, errors.New("checker error")
394393
}
395394

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
package github
2+
3+
import (
4+
"context"
5+
"fmt"
6+
"sync/atomic"
7+
"testing"
8+
9+
"github.com/github/github-mcp-server/pkg/inventory"
10+
"github.com/github/github-mcp-server/pkg/translations"
11+
"github.com/modelcontextprotocol/go-sdk/mcp"
12+
)
13+
14+
func BenchmarkFeatureInventory(b *testing.B) {
15+
for _, distribution := range featureBenchmarkDistributions() {
16+
b.Run(distribution.name, func(b *testing.B) {
17+
b.Run("build", func(b *testing.B) {
18+
var calls atomic.Int64
19+
b.ReportAllocs()
20+
for b.Loop() {
21+
_, err := featureBenchmarkBuilder(distribution, &calls).Build()
22+
if err != nil {
23+
b.Fatal(err)
24+
}
25+
}
26+
b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op")
27+
})
28+
29+
builder := featureBenchmarkBuilder(distribution, nil)
30+
b.Run("preconstructed-builder", func(b *testing.B) {
31+
b.ReportAllocs()
32+
for b.Loop() {
33+
if _, err := builder.Build(); err != nil {
34+
b.Fatal(err)
35+
}
36+
}
37+
})
38+
39+
b.Run("tools-list", func(b *testing.B) {
40+
inv, calls := featureBenchmarkInventory(b, distribution)
41+
b.ReportAllocs()
42+
b.ResetTimer()
43+
for b.Loop() {
44+
_ = inv.ForMCPRequest(inventory.MCPMethodToolsList, "").ToolsForRegistration(context.Background())
45+
}
46+
b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op")
47+
})
48+
49+
b.Run("unflagged-tool-call", func(b *testing.B) {
50+
inv, calls := featureBenchmarkInventory(b, distribution)
51+
b.ReportAllocs()
52+
b.ResetTimer()
53+
for b.Loop() {
54+
_ = inv.ForMCPRequest(inventory.MCPMethodToolsCall, "get_commit").ToolsForRegistration(context.Background())
55+
}
56+
b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op")
57+
})
58+
59+
b.Run("gated-tool-call", func(b *testing.B) {
60+
inv, calls := featureBenchmarkInventory(b, distribution)
61+
b.ReportAllocs()
62+
b.ResetTimer()
63+
for b.Loop() {
64+
_ = inv.ForMCPRequest(inventory.MCPMethodToolsCall, "get_file_blame").ToolsForRegistration(context.Background())
65+
}
66+
b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op")
67+
})
68+
69+
b.Run("ui-tool-call", func(b *testing.B) {
70+
inv, calls := featureBenchmarkInventory(b, distribution)
71+
b.ReportAllocs()
72+
b.ResetTimer()
73+
for b.Loop() {
74+
_ = inv.ForMCPRequest(inventory.MCPMethodToolsCall, "ui_get").ToolsForRegistration(context.Background())
75+
}
76+
b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op")
77+
})
78+
79+
b.Run("direct-handler-checks", func(b *testing.B) {
80+
var calls atomic.Int64
81+
checker := func(_ context.Context, flag string) (bool, error) {
82+
calls.Add(1)
83+
return distribution.enabled["*"] || distribution.enabled[flag], nil
84+
}
85+
b.ReportAllocs()
86+
for b.Loop() {
87+
ctx := inventory.WithFeatureState(context.Background(), checker)
88+
_ = inventory.ResolveFeature(ctx, checker, inventory.FeatureFlag(FeatureFlagCSVOutput))
89+
_ = inventory.ResolveFeature(ctx, checker, inventory.FeatureFlag(FeatureFlagCSVOutput))
90+
}
91+
b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op")
92+
})
93+
94+
b.Run("build-list-register", func(b *testing.B) {
95+
var calls atomic.Int64
96+
b.ReportAllocs()
97+
for b.Loop() {
98+
inv, err := featureBenchmarkBuilder(distribution, &calls).Build()
99+
if err != nil {
100+
b.Fatal(err)
101+
}
102+
inv = inv.ForMCPRequest(inventory.MCPMethodToolsList, "")
103+
server := mcp.NewServer(&mcp.Implementation{Name: "benchmark", Version: "v0"}, nil)
104+
inv.RegisterAll(context.Background(), server, nil)
105+
}
106+
b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op")
107+
})
108+
})
109+
}
110+
}
111+
112+
type featureBenchmarkDistribution struct {
113+
name string
114+
enabled map[string]bool
115+
}
116+
117+
func featureBenchmarkDistributions() []featureBenchmarkDistribution {
118+
return []featureBenchmarkDistribution{
119+
{name: "all-false"},
120+
{
121+
name: "mixed",
122+
enabled: map[string]bool{
123+
MCPAppsFeatureFlag: true,
124+
FeatureFlagFileBlame: true,
125+
FeatureFlagIssuesGranular: true,
126+
FeatureFlagIssueDependencies: true,
127+
},
128+
},
129+
{name: "all-true", enabled: map[string]bool{"*": true}},
130+
}
131+
}
132+
133+
func featureBenchmarkBuilder(distribution featureBenchmarkDistribution, calls *atomic.Int64) *inventory.Builder {
134+
checker := func(_ context.Context, flag string) (bool, error) {
135+
if calls != nil {
136+
calls.Add(1)
137+
}
138+
return distribution.enabled["*"] || distribution.enabled[flag], nil
139+
}
140+
tools := AllTools(translations.NullTranslationHelper)
141+
for i, baseCount := 0, len(tools); len(tools) < 139; i++ {
142+
tool := tools[i%baseCount]
143+
tool.Tool.Name = fmt.Sprintf("%s_remote_%d", tool.Tool.Name, i)
144+
tools = append(tools, tool)
145+
}
146+
return inventory.NewBuilder().
147+
SetTools(tools).
148+
SetResources(AllResources(translations.NullTranslationHelper)).
149+
SetPrompts(AllPrompts(translations.NullTranslationHelper)).
150+
WithToolsets([]string{"all"}).
151+
WithFeatureChecker(checker)
152+
}
153+
154+
func featureBenchmarkInventory(b *testing.B, distribution featureBenchmarkDistribution) (*inventory.Inventory, *atomic.Int64) {
155+
b.Helper()
156+
var calls atomic.Int64
157+
inv, err := featureBenchmarkBuilder(distribution, &calls).Build()
158+
if err != nil {
159+
b.Fatal(err)
160+
}
161+
return inv, &calls
162+
}

pkg/github/feature_flags_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,11 @@ import (
2020
const RemoteMCPEnthusiasticGreeting inventory.FeatureFlag = "remote_mcp_enthusiastic_greeting"
2121

2222
func featureCheckerFor(enabledFlags ...inventory.FeatureFlag) inventory.FeatureFlagChecker {
23-
enabled := make(map[inventory.FeatureFlag]bool, len(enabledFlags))
23+
enabled := make(map[string]bool, len(enabledFlags))
2424
for _, flag := range enabledFlags {
25-
enabled[flag] = true
25+
enabled[string(flag)] = true
2626
}
27-
return func(_ context.Context, flagName inventory.FeatureFlag) (bool, error) {
27+
return func(_ context.Context, flagName string) (bool, error) {
2828
return enabled[flagName], nil
2929
}
3030
}

pkg/github/server_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) {
196196

197197
func TestFeatureStateMiddlewareCachesHandlerChecks(t *testing.T) {
198198
var calls int
199-
checker := func(_ context.Context, flag inventory.FeatureFlag) (bool, error) {
199+
checker := func(_ context.Context, flag string) (bool, error) {
200200
calls++
201201
return flag == "enabled", nil
202202
}

0 commit comments

Comments
 (0)