Skip to content

Commit 7689727

Browse files
Refactor feature flags to use functional availability rules (#3166)
* refactor(features)!: use functional availability rules Resolve declared inventory features once per request and share the request-owned cache with in-handler feature checks. BREAKING CHANGE: Inventory items now use FeatureRule instead of FeatureFlagEnable, FeatureFlagEnableAll, and FeatureFlagDisable; FeatureFlagChecker now accepts FeatureFlag. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 * fix(features): address resolver review feedback Keep legacy string APIs compatible, seed feature state from each inventory's checker, persist caching for stdio calls, and fail closed for empty undeclared flags. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 * 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 * test(features): preserve query flag coverage after rebase Resolve the feature-query test against the final string-compatible flag API. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 * 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 * fix(features): share registration resolution Seed feature state once for direct registration so tools, resources, and prompts use one consistent snapshot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 * fix(features): isolate metadata and validate variants Deep-clone JSON metadata at inventory boundaries and exhaustively reject duplicate tool variants that can be enabled together. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 * fix(inventory): isolate tool lookup results Return owned tool metadata from unfiltered lookup APIs so callers cannot mutate cached inventory state. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 * fix(inventory): normalize owned metadata Canonicalize JSON metadata at builder boundaries, report invalid values from Build, and preserve wire semantics for typed mutable values. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 * fix(inventory): preserve metadata number precision Decode normalized metadata with UseNumber so large integers and other JSON number forms retain their exact wire representation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 * 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 * fix(features): fail nested checker resolution closed Disallow recursive ResolveFeature calls from feature checkers so direct, negating, multi-node, and concurrent cycles cannot cache enabled results or wait on one another. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 * docs(features): describe lazy rule resolution Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 * perf(features): filter static exclusions first Skip feature checks for read-only, filtered, unselected, protocol-ineligible, and capability-ineligible tools, and reject declared rules without predicates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 * fix(features): defer unknown client availability Apply pre-feature protocol and elicitation filtering only when stateless request metadata is known, leaving existing middleware to enforce unknown contexts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 * fix(features): preserve unavailable call errors Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13
1 parent bd47e63 commit 7689727

45 files changed

Lines changed: 1836 additions & 327 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

cmd/github-mcp-server/generate_docs.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,8 @@ func generateReadmeDocs(readmePath string) error {
6161

6262
// The README documents the default user experience: tools that are
6363
// enabled with no special flags set. Installing a checker that reports
64-
// every flag as disabled excludes tools gated by FeatureFlagEnable and
65-
// keeps the legacy variants of tools gated by FeatureFlagDisable, so
66-
// flag-gated duplicates don't appear twice.
64+
// every flag as disabled keeps the default variants selected by functional
65+
// feature rules, so flag-gated duplicates don't appear twice.
6766
// Build() can only fail if WithTools specifies invalid tools - not used here
6867
r, _ := github.NewInventory(t).
6968
WithToolsets([]string{"all"}).

docs/feature-flags.md

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,15 +35,54 @@ Only flags listed in
3535
[`AllowedFeatureFlags`](../pkg/github/feature_flags.go) can be enabled by
3636
end users. Insiders-only flags are not user-toggleable.
3737

38+
## Declaring tool availability
39+
40+
Tools, resources, and prompts use `inventory.NewFeatureRule` when feature flags
41+
change whether they are available. Each rule declares the flags it references
42+
and evaluates them with a fail-closed `FeatureResolver`, so normal Go boolean
43+
expressions can represent AND, OR, NOT, and mixed conditions:
44+
45+
```go
46+
tool.FeatureRule = inventory.NewFeatureRule(
47+
[]inventory.FeatureFlag{x, y},
48+
func(featureAsBool inventory.FeatureResolver) bool {
49+
return !(featureAsBool(x) && featureAsBool(y))
50+
},
51+
)
52+
```
53+
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 and static availability
61+
filters. Normal Go short-circuiting avoids checks that cannot affect the result,
62+
while one request-owned memo ensures each flag actually reached is resolved at
63+
most once across tools, resources, prompts, and `deps.IsFeatureEnabled`.
64+
Unavailable named tool calls skip rule evaluation but remain registered so the
65+
handler can return the specific client-availability error.
66+
67+
Feature predicates are pure and may depend only on their resolver. Construction
68+
validates every combination of up to 16 declared flags, so an undeclared lookup
69+
fails immediately even when ordinary evaluation would short-circuit that
70+
branch.
71+
72+
The inventory's string-based checker owns request feature state. Once installed,
73+
that state is authoritative; a checker stored on tool dependencies is used only
74+
as a fallback when handlers are invoked directly without request state.
75+
Feature checkers must not call `ResolveFeature`; nested resolution fails the
76+
owning check closed.
77+
3878
---
3979

4080
## Tools affected by each flag
4181

42-
The list below is regenerated from the Go source. For each user-controllable
43-
feature flag, it lists every tool whose **inventory or input schema** differs
44-
from the default — either because the flag introduces a new tool, or because
45-
it selects a flag-aware variant of an existing tool. Flags that only affect
46-
runtime behavior (such as output formatting) won't appear here.
82+
The list below is regenerated by comparing the default tool surface with each
83+
user-controllable flag enabled individually. Complex multi-flag rules may
84+
require separate documentation. Flags that only affect runtime behavior (such
85+
as output formatting) won't appear here.
4786

4887
<!-- START AUTOMATED FEATURE FLAG TOOLS -->
4988

docs/insiders-features.md

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,11 @@ 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, 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`.
214+
210215
`AllowedFeatureFlags` and `InsidersFeatureFlags` are deliberately independent sets:
211216

212217
- A flag in **`AllowedFeatureFlags` only** is a regular opt-in: users can turn it on, but insiders does not auto-enable it. Granular issues/PRs flags work this way.
@@ -219,5 +224,6 @@ Insiders is a **meta feature flag** — the same shape as `default` or `all` for
219224
2. Add it to `AllowedFeatureFlags` if end users should be able to opt in via
220225
`--features`, `X-MCP-Features`, or the `features` URL query parameter.
221226
3. Add it to `InsidersFeatureFlags` if insiders mode should turn it on automatically.
222-
4. Gate the behavior on the concrete flag (`deps.IsFeatureEnabled(ctx, FeatureFlagX)`), never on `cfg.InsidersMode`. There is a `TestGitHubPackageDoesNotReadInsidersMode` guard test that fails if `pkg/github` reads `InsidersMode` directly.
223-
5. The MCP-diff CI workflow picks up new entries in `AllowedFeatureFlags` automatically — see `.github/workflows/mcp-diff.yml`.
227+
4. For tool availability, attach an `inventory.NewFeatureRule` that declares every flag used by its predicate. For behavior inside a handler, use `deps.IsFeatureEnabled(ctx, FeatureFlagX)`.
228+
5. Gate on concrete flags, never on `cfg.InsidersMode`. There is a `TestGitHubPackageDoesNotReadInsidersMode` guard test that fails if `pkg/github` reads `InsidersMode` directly.
229+
6. The MCP-diff CI workflow picks up new entries in `AllowedFeatureFlags` automatically — see `.github/workflows/mcp-diff.yml`.

internal/ghmcp/server.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,7 @@ type StdioServerConfig struct {
256256
EnabledTools []string
257257

258258
// EnabledFeatures is a list of feature flags that are enabled
259-
// Items with FeatureFlagEnable matching an entry in this list will be available
259+
// Tool feature rules evaluate entries in this list.
260260
EnabledFeatures []string
261261

262262
// ReadOnly indicates if we should only register read-only tools

pkg/context/mcp_info.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package context
33
import (
44
"context"
55
"encoding/json"
6+
7+
"github.com/modelcontextprotocol/go-sdk/mcp"
68
)
79

810
type mcpMethodInfoCtx string
@@ -22,6 +24,10 @@ type MCPMethodInfo struct {
2224
ItemName string
2325
// RawArguments contains the unmaterialized tool arguments for tools/call requests.
2426
RawArguments json.RawMessage
27+
// ProtocolVersion and ClientCapabilities describe the requesting MCP client
28+
// when stateless HTTP parsing makes them available before registration.
29+
ProtocolVersion string
30+
ClientCapabilities *mcp.ClientCapabilities
2531
}
2632

2733
// DecodeArguments materializes tool arguments when request middleware needs

pkg/github/actions_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -574,7 +574,7 @@ func Test_ActionsGetJobLogs(t *testing.T) {
574574
// Note: consolidated ActionsGetJobLogs has same tool name "get_job_logs" as the individual tool
575575
// but with different descriptions. We skip toolsnap validation here since the individual
576576
// tool's toolsnap already exists and is tested in Test_GetJobLogs.
577-
// The consolidated tool has FeatureFlagEnable set, so only one will be active at a time.
577+
// The functional feature rules ensure only one variant is active at a time.
578578
assert.Equal(t, "get_job_logs", toolDef.Tool.Name)
579579
assert.NotEmpty(t, toolDef.Tool.Description)
580580
inputSchema := toolDef.Tool.InputSchema.(*jsonschema.Schema)

pkg/github/csv_output_test.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,20 +29,18 @@ func TestCSVOutputAppliedToDefaultListTools(t *testing.T) {
2929
require.Len(t, available, 2)
3030

3131
listing := requireToolByName(t, available, "list_things")
32-
assert.Empty(t, listing.FeatureFlagEnable)
33-
assert.Empty(t, listing.FeatureFlagDisable)
32+
assert.True(t, listing.FeatureRule.IsZero())
3433

3534
getting := requireToolByName(t, available, "get_thing")
36-
assert.Empty(t, getting.FeatureFlagEnable)
37-
assert.Empty(t, getting.FeatureFlagDisable)
35+
assert.True(t, getting.FeatureRule.IsZero())
3836
}
3937
}
4038

4139
func TestCSVOutputAppliesToFlagGatedListTools(t *testing.T) {
4240
enabledOnly := testCSVOutputTool("list_things", `[{"number":1}]`)
43-
enabledOnly.FeatureFlagEnable = FeatureFlagFileBlame
41+
enabledOnly.FeatureRule = featureEnabledRule(FeatureFlagFileBlame)
4442
disabledOnly := testCSVOutputTool("list_legacy_things", `[{"number":2}]`)
45-
disabledOnly.FeatureFlagDisable = []string{FeatureFlagFileBlame}
43+
disabledOnly.FeatureRule = featureDisabledRule(FeatureFlagFileBlame)
4644

4745
tools := withCSVOutput([]inventory.ServerTool{enabledOnly, disabledOnly})
4846
require.Len(t, tools, 2)

pkg/github/dependencies.go

Lines changed: 11 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"fmt"
77
"log/slog"
88
"net/http"
9-
"os"
109

1110
ghcontext "github.com/github/github-mcp-server/pkg/context"
1211
"github.com/github/github-mcp-server/pkg/http/transport"
@@ -95,7 +94,7 @@ type ToolDependencies interface {
9594
GetContentWindowSize() int
9695

9796
// IsFeatureEnabled checks if a feature flag is enabled.
98-
IsFeatureEnabled(ctx context.Context, flagName string) bool
97+
IsFeatureEnabled(ctx context.Context, flag string) bool
9998

10099
// Logger returns the structured logger, optionally enriched with
101100
// request-scoped data from ctx. Integrators provide their own slog.Handler
@@ -204,22 +203,11 @@ func (d BaseDeps) Metrics(ctx context.Context) metrics.Metrics {
204203
// GetRequestStateSealer implements RequestStateSealerProvider.
205204
func (d BaseDeps) GetRequestStateSealer() RequestStateSealer { return d.StateSealer }
206205

207-
// IsFeatureEnabled checks if a feature flag is enabled.
208-
// Returns false if the feature checker is nil, flag name is empty, or an error occurs.
209-
// This allows tools to conditionally change behavior based on feature flags.
210-
func (d BaseDeps) IsFeatureEnabled(ctx context.Context, flagName string) bool {
211-
if d.featureChecker == nil || flagName == "" {
212-
return false
213-
}
214-
215-
enabled, err := d.featureChecker(ctx, flagName)
216-
if err != nil {
217-
// Log error but don't fail the tool - treat as disabled
218-
fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", flagName, err)
219-
return false
220-
}
221-
222-
return enabled
206+
// IsFeatureEnabled checks if a feature flag is enabled. Request feature state
207+
// is authoritative when present; the dependency checker is a fallback for
208+
// direct handler invocation. Empty names and checker errors resolve false.
209+
func (d BaseDeps) IsFeatureEnabled(ctx context.Context, flag string) bool {
210+
return inventory.ResolveFeature(ctx, d.featureChecker, inventory.FeatureFlag(flag))
223211
}
224212

225213
// NewTool creates a ServerTool that retrieves ToolDependencies from context at call time.
@@ -495,18 +483,9 @@ func (d *RequestDeps) Metrics(ctx context.Context) metrics.Metrics {
495483
return d.obsv.Metrics(ctx)
496484
}
497485

498-
// IsFeatureEnabled checks if a feature flag is enabled.
499-
func (d *RequestDeps) IsFeatureEnabled(ctx context.Context, flagName string) bool {
500-
if d.featureChecker == nil || flagName == "" {
501-
return false
502-
}
503-
504-
enabled, err := d.featureChecker(ctx, flagName)
505-
if err != nil {
506-
// Log error but don't fail the tool - treat as disabled
507-
fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", flagName, err)
508-
return false
509-
}
510-
511-
return enabled
486+
// IsFeatureEnabled checks if a feature flag is enabled. Request feature state
487+
// is authoritative when present; the dependency checker is a fallback for
488+
// direct handler invocation.
489+
func (d *RequestDeps) IsFeatureEnabled(ctx context.Context, flag string) bool {
490+
return inventory.ResolveFeature(ctx, d.featureChecker, inventory.FeatureFlag(flag))
512491
}

pkg/github/feature_flags.go

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
package github
22

3-
import "slices"
3+
import (
4+
"slices"
5+
6+
"github.com/github/github-mcp-server/pkg/inventory"
7+
)
48

59
// MCPAppsFeatureFlag is the feature flag name for MCP Apps (interactive UI forms).
610
const MCPAppsFeatureFlag = "remote_mcp_ui_apps"
@@ -71,6 +75,33 @@ type FeatureFlags struct {
7175
LockdownMode bool
7276
}
7377

78+
func featureEnabledRule(feature string) inventory.FeatureRule {
79+
flag := inventory.FeatureFlag(feature)
80+
return inventory.NewFeatureRule(
81+
[]inventory.FeatureFlag{flag},
82+
func(featureAsBool inventory.FeatureResolver) bool {
83+
return featureAsBool(flag)
84+
},
85+
)
86+
}
87+
88+
func featureDisabledRule(feature string) inventory.FeatureRule {
89+
flag := inventory.FeatureFlag(feature)
90+
return inventory.NewFeatureRule(
91+
[]inventory.FeatureFlag{flag},
92+
func(featureAsBool inventory.FeatureResolver) bool {
93+
return !featureAsBool(flag)
94+
},
95+
)
96+
}
97+
98+
var (
99+
issuesGranularFeatureRule = featureEnabledRule(FeatureFlagIssuesGranular)
100+
issuesConsolidatedFeatureRule = featureDisabledRule(FeatureFlagIssuesGranular)
101+
pullRequestsGranularFeatureRule = featureEnabledRule(FeatureFlagPullRequestsGranular)
102+
pullRequestsConsolidatedRule = featureDisabledRule(FeatureFlagPullRequestsGranular)
103+
)
104+
74105
// ResolveFeatureFlags computes the effective set of enabled feature flags by:
75106
// 1. Taking the user-supplied flags (from --features or HTTP request
76107
// configuration) and
@@ -89,9 +120,9 @@ type FeatureFlags struct {
89120
// Returns a set (map) for O(1) lookup by the feature checker.
90121
func ResolveFeatureFlags(enabledFeatures []string, insidersMode bool) map[string]bool {
91122
effective := make(map[string]bool)
92-
for _, f := range enabledFeatures {
93-
if slices.Contains(AllowedFeatureFlags, f) {
94-
effective[f] = true
123+
for _, feature := range enabledFeatures {
124+
if slices.Contains(AllowedFeatureFlags, feature) {
125+
effective[feature] = true
95126
}
96127
}
97128
if insidersMode {

0 commit comments

Comments
 (0)