Refactor feature flags to use functional availability rules

[SamMorrowDrums]

Summary

• replace fixed enable/disable gates with typed functional FeatureRule predicates
• evaluate rules lazily after request narrowing with normal Go short-circuiting
• memoize each reached flag once per request across tools, resources, prompts, and handlers
• keep AllowedFeatureFlags as the separate user-controllable allowlist
• validate rule declarations and duplicate-name exclusivity
• add real-inventory performance benchmarks

Breaking change and migration

The following exported fields are removed:

• inventory.ServerTool.FeatureFlagEnable
• inventory.ServerTool.FeatureFlagEnableAll
• inventory.ServerTool.FeatureFlagDisable
• inventory.ServerResourceTemplate.FeatureFlagEnable
• inventory.ServerResourceTemplate.FeatureFlagDisable
• inventory.ServerPrompt.FeatureFlagEnable
• inventory.ServerPrompt.FeatureFlagDisable

Replace an enable gate with:

flag := inventory.FeatureFlag("my_feature")
tool.FeatureRule = inventory.NewFeatureRule(
    []inventory.FeatureFlag{flag},
    func(featureAsBool inventory.FeatureResolver) bool {
        return featureAsBool(flag)
    },
)

Replace a disable gate with the same declaration and return !featureAsBool(flag). Multi-flag AND, OR, and UNLESS behavior belongs directly in the predicate; list every referenced flag in the declaration.

inventory.FeatureFlagChecker remains func(context.Context, string) (bool, error), and github.ToolDependencies.IsFeatureEnabled continues to accept string flag names.

Performance

A shared 139-tool/5-resource/2-prompt benchmark compares current main with this branch under all-false, mixed, and all-true distributions:

• build performs zero flag checks
• unflagged tool calls perform zero checks
• gated and UI tool calls perform one check
• tools/list checks each reached unique OSS availability flag once: 41/43 repeated checks become 7
• end-to-end build+narrow+register: -9.0% to -3.9% median CPU in the final run, +0.4% bytes, +0.6–0.8% allocations
• a longer all-false rerun measured -0.2% CPU, +0.34% bytes, and +0.77% allocations

Validation

• targeted feature/query/registration tests under -race -count=20
• representative benchmark under three flag distributions
• script/lint
• script/test
• script/generate-docs

[Copilot]

Pull request overview

Refactors feature gating around typed functional rules and shared request-scoped resolution.

Changes:

• Introduces FeatureFlag, FeatureRule, and cached resolution state.
• Migrates inventory items and GitHub tools from legacy flag fields.
• Updates HTTP/stdio integration, tests, generators, and documentation.

Show a summary per file

File	Description
script/print-mcp-diff-configs/main.go	Uses the header-compatible flag accessor.
pkg/inventory/server_tool.go	Replaces legacy tool gates with FeatureRule.
pkg/inventory/resources.go	Adds resource feature rules.
pkg/inventory/registry.go	Collects and pre-resolves required features.
pkg/inventory/registry_test.go	Migrates inventory feature tests.
pkg/inventory/prompts.go	Adds prompt feature rules.
pkg/inventory/filters.go	Evaluates functional rules during filtering.
pkg/inventory/features.go	Implements typed rules and resolution state.
pkg/inventory/features_test.go	Tests predicates and caching.
pkg/inventory/builder.go	Removes the legacy feature filter.
pkg/http/server.go	Adapts HTTP feature resolution to typed flags.
pkg/http/server_test.go	Updates HTTP checker tests.
pkg/http/handler.go	Seeds request-owned feature state.
pkg/http/handler_test.go	Migrates handler feature tests.
pkg/github/ui_tools.go	Migrates the UI tool gate.
pkg/github/ui_tools_test.go	Verifies the UI feature rule.
pkg/github/ui_capability_test.go	Uses typed UI flags.
pkg/github/tools.go	Types granular flags and converts header flags.
pkg/github/tools_validation_test.go	Updates gated-duplicate validation.
pkg/github/server.go	Updates feature configuration documentation.
pkg/github/server_test.go	Updates dependency stubs.
pkg/github/repositories.go	Migrates file-blame gating.
pkg/github/repositories_test.go	Verifies file-blame rules.
pkg/github/pullrequests.go	Migrates consolidated PR rules.
pkg/github/pullrequests_granular.go	Migrates granular PR rules.
pkg/github/issues.go	Migrates consolidated issue rules.
pkg/github/issues_test.go	Updates issue-rule assertions.
pkg/github/issues_granular.go	Migrates granular issue rules.
pkg/github/issue_dependencies.go	Migrates dependency-tool gates.
pkg/github/issue_dependencies_test.go	Updates dependency gate tests.
pkg/github/granular_tools_test.go	Tests granular functional rules.
pkg/github/find_duplicate.go	Migrates duplicate-detection gating.
pkg/github/find_duplicate_test.go	Updates duplicate gate tests.
pkg/github/feature_flags.go	Types flags and defines reusable rules.
pkg/github/feature_flags_test.go	Migrates feature-resolution tests.
pkg/github/dependencies.go	Shares resolution through dependencies.
pkg/github/dependencies_test.go	Updates dependency checker tests.
pkg/github/csv_output_test.go	Migrates CSV rule fixtures.
pkg/github/context_tools_test.go	Uses typed IFC flags.
pkg/github/actions_test.go	Updates functional-rule terminology.
internal/ghmcp/server.go	Adapts stdio feature checking.
docs/insiders-features.md	Documents shared functional resolution.
docs/feature-flags.md	Documents availability rules.
cmd/github-mcp-server/generate_docs.go	Updates default documentation checker.
cmd/github-mcp-server/feature_flag_docs.go	Types feature documentation generation.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

• Files reviewed: 45/45 changed files
• Comments generated: 4
• Review effort level: Balanced

[IrynaKulakova]

A few notes on the new feature-rule machinery. Overall direction looks right — replacing the three annotation fields with one declared predicate is a clear improvement, and the fail-closed semantics of the old featureFlagAllowed are preserved. Comments below are about the resolution state, not the rule model itself.

[FuzzysTodd]

tijr

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot review overview

🟡 Changes recommended

Cached metadata lacks enforced ownership, and an additional public API break is undocumented.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 2

New issues introduced by this change (2)

Severity	Finding
	pkg/​github/​dependencies.go — This introduces another public source-breaking change that is not listed in the PR's Breaking…
	pkg/​inventory/​registry.go — These caches assume immutable inventory contents, but Builder.SetTools, SetResources, and…

[Copilot]

Copilot review overview

🟡 Changes recommended

Feature resolution can deadlock or incorrectly enable cyclic flags, and cached metadata is not protected from caller mutation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 4 · 1

New issues introduced by this change (3)

Severity	Finding
	pkg/​inventory/​features.go — Cycle detection does not necessarily fail the resolved flag closed. For a checker that computes…
	pkg/​inventory/​features.go — This wait can deadlock on a concurrent cross-feature cycle. If one goroutine owns A and its…
	pkg/​inventory/​builder.go — The metadata cache assumes immutable inventory contents, but Build retains the slices supplied to…

Pre-existing issues (2)

Severity	Finding
	pkg/​inventory/​registry.go — These caches assume immutable inventory contents, but Builder.SetTools, SetResources, and… View comment
	pkg/​github/​dependencies.go — This introduces another public source-breaking change that is not listed in the PR's Breaking… View comment

Suppressed comments (1)

pkg/github/dependencies.go:97

• This changes the public ToolDependencies interface, so external implementations and callers using string variables no longer compile. The PR's breaking-change section lists the inventory fields and FeatureFlagChecker, but not this interface change. Either preserve the string boundary and convert internally, or explicitly document ToolDependencies.IsFeatureEnabled as another breaking change.

	IsFeatureEnabled(ctx context.Context, flag inventory.FeatureFlag) bool

[Copilot]

Copilot review overview

🟡 Changes recommended

RegisterAll does not propagate its resolved feature state across tool, resource, and prompt registration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1 · 1

New issues introduced by this change (1)

Severity	Finding
	pkg/​inventory/​registry.go — The feature state created here is confined to this local ctx. RegisterAll then calls resource…

Pre-existing issues (1)

Severity	Finding
	pkg/​inventory/​registry.go — These caches assume immutable inventory contents, but Builder.SetTools, SetResources, and… View comment

Issues resolved since last review (4)

Severity	Finding
	pkg/​inventory/​builder.go — The metadata cache assumes immutable inventory contents, but Build retains the slices supplied to… View resolved comment
	pkg/​inventory/​features.go — This wait can deadlock on a concurrent cross-feature cycle. If one goroutine owns A and its… View resolved comment
	pkg/​inventory/​features.go — Cycle detection does not necessarily fail the resolved flag closed. For a checker that computes… View resolved comment
	pkg/​github/​dependencies.go — This introduces another public source-breaking change that is not listed in the PR's Breaking… View resolved comment

[Copilot]

Copilot review overview

🔵 Needs a closer look

Nested metadata remains shared despite the ownership contract, and duplicate-rule validation does not verify mutual exclusivity.

Review tier: Balanced
Findings: 1

Pre-existing issues (1)

Severity	Finding
	pkg/​inventory/​registry.go — These caches assume immutable inventory contents, but Builder.SetTools, SetResources, and… View comment

Issues resolved since last review (1)

Severity	Finding
	pkg/​inventory/​registry.go — The feature state created here is confined to this local ctx. RegisterAll then calls resource… View resolved comment

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/github/tools_validation_test.go:144

• A nonzero functional rule does not imply that duplicate variants are mutually exclusive. Two overlapping rules—or one ungated and one gated tool, since this map is keyed only by name—now bypass this test, and when both are enabled mcp.Server.AddTool silently replaces one definition (pkg/inventory/server_tool.go:159). For each duplicate-name group, evaluate the rules across the union of declared flags and fail whenever more than one variant is enabled for an assignment; retain only the explicit get_label exception.

	// First pass: identify tools that have feature flags (mutually exclusive at runtime)
	for _, tool := range tools {
		if !tool.FeatureRule.IsZero() {
			featureFlagged[tool.Tool.Name] = true

pkg/inventory/builder.go:96

• maps.Clone only copies the outer metadata map. Production tool metadata contains nested mutable maps/slices (for example pkg/github/ui_tools.go:45-48 and pkg/github/context_tools.go:58-62), so mutating the source after SetTools, or mutating nested metadata returned by AvailableTools, still changes the inventory and future registrations despite the new ownership contract. Deep-copy the JSON-like metadata values recursively (and apply the same protection to resource/prompt metadata), then cover a nested mutation in the ownership test.

[Copilot]

Copilot review overview

🟡 Changes recommended

Metadata cloning still aliases valid mutable metadata representations, violating the new inventory ownership contract.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1 · 1

New issues introduced by this change (1)

Severity	Finding
	pkg/​inventory/​metadata.go — Metadata isolation is incomplete for valid mutable values whose concrete type is not…

Pre-existing issues (1)

Severity	Finding
	pkg/​inventory/​registry.go — These caches assume immutable inventory contents, but Builder.SetTools, SetResources, and… View comment

[Copilot]

Copilot review overview

🟡 Changes recommended

Metadata normalization can lose precision for valid large integer values.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 2 · 1

New issues introduced by this change (1)

Severity	Finding
	pkg/​inventory/​metadata.go — This JSON round-trip can silently corrupt valid metadata numbers: unmarshalling into mcp.Meta…

Pre-existing issues (2)

Severity	Finding
	pkg/​inventory/​registry.go — These caches assume immutable inventory contents, but Builder.SetTools, SetResources, and… View comment
	pkg/​inventory/​metadata.go — Metadata isolation is incomplete for valid mutable values whose concrete type is not… View comment

[Copilot]

Copilot review overview

🟡 Changes recommended

The public metadata normalization behavior is an undocumented additional breaking change.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1 · 1

New issues introduced by this change (1)

Severity	Finding
	pkg/​inventory/​builder.go — This introduces another public behavioral break that is not covered by the PR's Breaking change…

Pre-existing issues (1)

Severity	Finding
	pkg/​inventory/​registry.go — These caches assume immutable inventory contents, but Builder.SetTools, SetResources, and… View comment

Issues resolved since last review (2)

Severity	Finding
	pkg/​inventory/​metadata.go — This JSON round-trip can silently corrupt valid metadata numbers: unmarshalling into mcp.Meta… View resolved comment
	pkg/​inventory/​metadata.go — Metadata isolation is incomplete for valid mutable values whose concrete type is not… View resolved comment

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/github/actions_test.go:577

• This sentence documents behavior that does not exist: ActionsGetJobLogs has a zero FeatureRule, and it is the repository's only get_job_logs definition. Remove the stale variant explanation so the test does not imply that feature gating is being verified here.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Longer feature-dependency cycles can cache an enabled value instead of failing closed.

Review tier: Balanced
Findings: None

Issues resolved since last review (2)

Severity	Finding
	pkg/​inventory/​builder.go — This introduces another public behavioral break that is not covered by the PR's Breaking change… View resolved comment
	pkg/​inventory/​registry.go — These caches assume immutable inventory contents, but Builder.SetTools, SetResources, and… View resolved comment

Suppressed comments (1)

pkg/inventory/features.go:172

• This only marks the requested feature and its immediate owner as failed, so longer dependency cycles do not fail closed. For example, with a = !b, b = !c, and c = !a, resolving a marks only a and c; b then caches true even though it participates in the cycle. Track the full active dependency chain/graph and fail every cycle participant (while distinguishing an unrelated in-flight resolution).

[Copilot]

Copilot review overview

🔵 Needs a closer look

The breaking API migration and request-scoped concurrent caching affect core registration and handler behavior and warrant final human validation.

Review tier: Balanced
Findings: None

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/inventory/features.go:30

• This says checks resolve before the predicate runs, but resolution is intentionally lazy: the predicate calls the resolver, which resolves only reached branches. Please describe the actual timing so consumers do not expect eager resolution.