You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Simplify feature-flag handling: collapse CSV dual-variant + skip filtering when no checker (#2516)
* refactor: generic toolset+name sort, clarify feature flag intent
Address review feedback on #2450:
- Collapse the three near-identical sort helpers in pkg/inventory/filters.go
into a generic sortByToolsetThenName so adding new inventory item types
doesn't require copying the comparator.
- Expand the doc comments on the three *WithoutFeatureFiltering helpers to
spell out why they exist: HTTP mode builds a static (process-wide)
inventory as an upper bound, but per-request feature flags from headers
(X-MCP-Features, X-MCP-Insiders) are evaluated later, so feature-flagged
variants must be preserved here.
- Strengthen the doc comment on ResolveFeatureFlags to make the contract
explicit: user-supplied flags are validated against AllowedFeatureFlags,
but insiders expansion deliberately is not — InsidersFeatureFlags may
include server-controlled flags that are not user-toggleable.
CORS comments are intentionally left for the PR author.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* docs(feature-flags): clarify allowed and insiders sets are independent
Also add tests covering:
- a user-toggleable flag (FeatureFlagIssuesGranular) that insiders does
not turn on automatically
- insiders mode not turning on user-only allowed flags
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* refactor(inventory): collapse three *WithoutFeatureFiltering helpers into StaticUpperBound
The three parallel methods (AvailableToolsWithoutFeatureFiltering,
AvailableResourceTemplatesWithoutFeatureFiltering,
AvailablePromptsWithoutFeatureFiltering) were always called as a triple
in exactly two places: HTTP buildStaticInventory and its test mirror.
They exist because the dual-variant pattern (sibling tools with mirrored
FeatureFlagEnable / FeatureFlagDisable on the same name, e.g. CSV output)
makes feature filtering at static-build time impossible — both variants
must be kept and resolved per-request.
Replace the three with one method, Inventory.StaticUpperBound(ctx), that
returns (tools, resources, prompts) and carries the rationale in its
doc comment. Reduces API surface, eliminates the triplication, and makes
the single "skip feature filtering" concept obvious to readers.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* refactor: simplify feature-flag handling
Two related simplifications, both about treating insiders as a meta flag
that expands once at startup and then stops mattering:
- Collapse CSV's dual-variant pattern into a single tool whose handler
performs a runtime feature-flag check via deps.IsFeatureEnabled. CSV
is a pure response-format toggle, not a schema change, so it does not
need the dual-name pattern that genuine schema variants (granular
issues/PRs) still use.
- When no feature checker is installed, skip feature-flag filtering and
return the full upper bound. The static HTTP inventory now uses plain
AvailableTools/Resources/Prompts; the per-request inventory always
installs a checker, so MCP registration (which serves a tool name once)
always sees a deduplicated set. The bespoke StaticUpperBound helper and
the isToolEnabledWithFeatureFlags split go away.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* ci(mcp-diff): add insiders + per-feature configs
The mcp-diff matrix now includes:
- --insiders (and --insiders --read-only)
- one config per github.AllowedFeatureFlags entry, generated by
script/print-mcp-diff-configs so new user-controllable flags get
diffed automatically without editing the workflow
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* docs(insiders): explain feature-flag resolution for contributors
Adds a 'How feature flags are resolved' section covering:
- Insiders is a meta flag, like 'all'/'default' for toolsets
- User input -> allowlist filter -> insiders expansion ->
server-side fallback (remote only)
- AllowedFeatureFlags vs InsidersFeatureFlags are independent
- How to add a new feature flag, including the
TestGitHubPackageDoesNotReadInsidersMode guard
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* refactor(inventory): make feature-flag gating a regular ToolFilter
Move tool feature-flag evaluation out of isToolEnabled and into a
ToolFilter installed at the head of the pipeline by Build() when
WithFeatureChecker received a non-nil checker. The 'no checker = no
filtering' contract is now expressed structurally (the filter isn't
installed) instead of by a runtime nil check inside the helper.
Resources and prompts have no filter pipeline, so they call the now-pure
featureFlagAllowed helper behind an explicit r.featureChecker != nil
guard at the iteration site.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* perf(inventory): cache extracted toolset IDs in sort comparator
Avoid evaluating the extractor closures up to three times per comparison.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Because this changes list tool response shape, clients that require JSON list responses should avoid enabling this feature.
70
+
71
+
---
72
+
73
+
## How feature flags are resolved
74
+
75
+
> [!NOTE]
76
+
> This section is for contributors. End users only need the table at the top of this page.
77
+
78
+
Insiders is a **meta feature flag** — the same shape as `default` or `all` for toolsets. It expands once at startup into a curated set of individual feature flags, and from that point on every code path keys off concrete flags, never `InsidersMode` directly. New experimental work should always get its own flag and then be added to the insiders expansion list, never folded into `insiders` as a catch-all.
79
+
80
+
### Resolution order
81
+
82
+
1.**User input.** Users may opt into specific features:
83
+
- Local server: `--features=<flag>,<flag>` CLI flag (or `GITHUB_FEATURES` env var).
2.**Allowlist filter.** User-supplied flags are filtered against [`AllowedFeatureFlags`](../pkg/github/feature_flags.go). Anything not on the allowlist is silently dropped — flags missing from the allowlist can only be turned on by remote-server feature management, not by end users.
86
+
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.
87
+
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.
88
+
89
+
`AllowedFeatureFlags` and `InsidersFeatureFlags` are deliberately independent sets:
90
+
91
+
- 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.
92
+
- A flag in **`InsidersFeatureFlags` only** is reachable through insiders (and remote-server rollouts), but cannot be enabled by user input. Internal-only experiments work this way.
93
+
- A flag in **both** is opt-in for end users *and* automatically on under insiders.
94
+
95
+
### Adding a new feature flag
96
+
97
+
1. Add a constant in `pkg/github/feature_flags.go`.
98
+
2. Add it to `AllowedFeatureFlags` if end users should be able to opt in via `--features` / `X-MCP-Features`.
99
+
3. Add it to `InsidersFeatureFlags` if insiders mode should turn it on automatically.
100
+
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.
101
+
5. The MCP-diff CI workflow picks up new entries in `AllowedFeatureFlags` automatically — see `.github/workflows/mcp-diff.yml`.
0 commit comments