Add deny-by-default visibility to vMCP aggregation

[jerm-dro]

Summary

• aggregation.tools is a sparse list that fails open: a backend in the group with no entry has every one of its tools advertised. An operator scoping a vMCP to a curated subset has to remember to add excludeAll: true for each new group member, and forgetting silently widens what clients see. Reported in Default deny filter for group exposed by vMCP #6073, where the group grows over time and each addition is a chance to accidentally expose a server.
• Adds aggregation.defaultVisibility (allow | deny). Under deny, a backend absent from tools contributes no tools, so only listed backends are advertised. A backend that is listed is opted in by its entry — its own excludeAll/filter then decide which of its tools show, so an overrides-only entry keeps working.
• Defaults to allow, which is exactly today's behavior, so existing YAML configs and already-deployed CRs are unaffected and there is no migration or backfill.
• Like the sibling visibility settings, this controls advertising only. Every backend tool stays in the routing table, so composite tools can still call hidden tools.

Closes #6073

Type of change

• New feature

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)

task test passes with zero failures. New coverage:

• TestDefaultAggregator_AdvertisingFilterPreservesBackendID — added cases for unset / allow / deny, plus deny combined with a listed backend's filter, with no filter, and with excludeAll. The existing table already asserts the routing-table invariant, so each new case also proves tools stay routable.
• TestDefaultAggregator_DefaultVisibilityDenyMixedBackends — the motivating scenario: a group with one listed and one unlisted backend. Asserts only the listed backend is advertised and the unlisted backend's tools remain in the routing table.
• TestConvert_DefaultVisibilityPreserved — CRD → rendered-config carry-through for deny / allow / unset.
• validateAggregation — accepts unset/allow/deny, rejects an unknown value.

Two pre-existing failures in this workspace, confirmed identical on a clean tree via git stash and unrelated to this change: 4 lint findings (3 gosec, 1 staticcheck) in files this PR does not touch, and the cmd/thv-operator/test-integration/virtualmcp envtest suite failing on a missing local /usr/local/kubebuilder/bin/etcd (that suite is excluded from task test).

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Purely additive: one new optional string field with +kubebuilder:default=allow and enum: [allow, deny]. Omitting it yields the current behavior. No zz_generated deepcopy drift, since the field is a value type.

Changes

File	Change
pkg/vmcp/config/config.go	DefaultVisibility field on AggregationConfig + DefaultVisibility type and allow/deny constants
pkg/vmcp/aggregator/default_aggregator.go	denyUnlisted derived at construction; applied in shouldAdvertiseTool's no-config branch
pkg/vmcp/config/validator.go	Enum validation for the CLI path (no admission webhook there)
cmd/thv-operator/pkg/vmcpconfig/converter.go	Carry the field through CRD → config conversion
deploy/charts/operator-crds/** (2 files)	Regenerated CRDs (both chart copies)
docs/operator/crd-api.md	Regenerated
docs/operator/virtualmcpserver-api.md	Field docs + deny-by-default example + scope note
docs/arch/10-virtual-mcp-architecture.md	Expanded the Tool Filtering section
3 *_test.go	Coverage described above

Does this introduce a user-facing change?

Yes. vMCP operators can set aggregation.defaultVisibility: deny so that only workloads explicitly listed in aggregation.tools have their tools advertised. Adding a workload to the group then no longer exposes it by default. The default remains allow, so nothing changes for existing deployments unless the field is set.

aggregation:
  conflictResolution: prefix
  defaultVisibility: deny
  tools:
    - workload: github
      filter: ["get_issue", "list_prs"]
    - workload: jira
      filter: ["get_issue"]

Special notes for reviewers

Deliberate semantics — "listed means allowed". Under deny, an entry with neither filter nor excludeAll advertises all of that workload's tools; deny governs only backends with no entry at all. This matches the request in #6073 ("only showing servers that are specifically listed") and avoids silently blanking out existing curated setups (e.g. overrides-only entries) the moment someone enables the flag. The stricter alternative — deny unless filter enumerates — is not what this implements. Locked in by test.

The converter is the sharp edge. convertAggregation hand-copies fields rather than deep-copying, so omitting DefaultVisibility there would let the CRD accept defaultVisibility: deny while the rendered config falls back to advertise-everything — a security-relevant setting that looks applied and isn't. TestConvert_DefaultVisibilityPreserved guards this; I verified it genuinely catches the regression by deleting the copy line and confirming the failure (expected "deny", got "") before restoring it. Worth keeping in mind for any future field added here.

Upgrade ordering matters. The aggregation subtree has no x-kubernetes-preserve-unknown-fields, so on a cluster where the operator is upgraded but the CRDs are not, defaultVisibility: deny is pruned at admission and the setting silently fails open. Standard ToolHive CRD-before-operator ordering, but the failure mode here is "tools you expected hidden are advertised", so it may deserve a release note.

Scope — what this does not do. Documented in both docs files:

1. defaultVisibility is advertising-only, so it does not affect resources or prompts, which have no filtering config at all. A vMCP scoped this way still exposes every backend resource and prompt.
2. On the Modern (2026-07-28) dispatch path, tools/call resolves straight against the routing table, so a hidden tool is still callable by name when no Cedar policy is configured. That is a pre-existing bug independent of this PR and is tracked as a separate follow-up; the Legacy path is unaffected because only advertised tools are registered per session. Reviewers evaluating this as a hardening measure should know that fix is a prerequisite for calling it a boundary.

Related but intentionally out of scope: #3493 (tool-level deny-list with default-allow) is a different question — per-tool convenience rather than per-server scoping — and points the opposite way on fail-open. Kept separate so the two defaults don't land in one confusing change.

Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.56%. Comparing base (a48448d) to head (9bf1406).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6163      +/-   ##
==========================================
- Coverage   72.59%   72.56%   -0.04%     
==========================================
  Files         736      736              
  Lines       76359    76367       +8     
==========================================
- Hits        55435    55415      -20     
- Misses      16981    17026      +45     
+ Partials     3943     3926      -17     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

This PR adds a new aggregation.defaultVisibility setting to vMCP tool aggregation so operators can choose between the current “fail open” behavior (allow, default) and a “deny-by-default” advertising mode (deny) where only backends explicitly listed under aggregation.tools contribute advertised tools. The change is explicitly scoped to advertising only: hidden tools remain routable for composite tools, and prompts/resources are unaffected.

Changes:

• Introduce DefaultVisibility (allow/deny) on AggregationConfig, validate it on the CLI config path, and carry it through CRD → rendered config conversion.
• Teach the default aggregator to withhold tools from unlisted backends when defaultVisibility: deny.
• Add focused unit tests covering compatibility (unset/allow) and deny-by-default behavior, plus conversion preservation.

Key Concerns

• suggestion: convertAggregation comments contradict themselves about “deep copy” vs “hand-copy”. This is minor but worth fixing to avoid misleading future maintainers about aliasing/mutation expectations (see stored comment in cmd/thv-operator/pkg/vmcpconfig/converter.go).

Testing Assessment

The added tests cover the important input matrix (unset vs allow vs deny, deny + listed backend with filter/no filter/excludeAll, mixed listed+unlisted backends) and also reassert the routing-table invariant, which is the key safety property for “advertising-only” changes.

vMCP Anti-Pattern Check

No new vMCP anti-patterns stood out in the touched pkg/vmcp/** files (notably: no new interface churn/abstractions (anti-pattern #8), no new shared mutable state or concurrency hazards, and no “god object” expansion concerns from this change).

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/vmcp/config/config.go	Add DefaultVisibility type/field and document allow vs deny advertising semantics.
pkg/vmcp/config/validator.go	Validate defaultVisibility values for the CLI config path.
pkg/vmcp/config/validator_test.go	Add coverage for allow/deny/unset validity and invalid value rejection.
pkg/vmcp/aggregator/default_aggregator.go	Implement deny-by-default advertising for unlisted backends via a derived denyUnlisted flag.
pkg/vmcp/aggregator/advertised_backendid_test.go	Extend advertising tests for allow/deny/unset and add mixed-backend deny scenario.
cmd/thv-operator/pkg/vmcpconfig/converter.go	Preserve DefaultVisibility through CRD → rendered config conversion.
cmd/thv-operator/pkg/vmcpconfig/converter_test.go	Guard against converter regression dropping defaultVisibility.
deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml	Regenerated CRD schema/docs including defaultVisibility (chart template copy).
deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml	Regenerated CRD schema/docs including defaultVisibility (packaged CRD copy).
docs/operator/crd-api.md	Regenerated CRD API docs reflecting the new field and semantics.
docs/operator/virtualmcpserver-api.md	Add user-facing docs and deny-by-default example + advertising-only scope note.
docs/arch/10-virtual-mcp-architecture.md	Expand architecture docs around tool filtering/advertising semantics.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jerm-dro]

Good catch — and the contradiction ran deeper than the two comments disagreeing: the pre-existing "Start with a deep copy" claim was simply wrong, and had been before this PR.

This is a field-by-field copy that aliases the source's slices (ConflictResolutionConfig.PriorityOrder, WorkloadToolConfig.Filter); only Overrides is genuinely deep-copied (via DeepCopy() in resolveToolConfigRefs). So "deep copy" was misleading in exactly the way you'd worry about — a maintainer trusting it might mutate a slice here and silently mutate the CR's in-memory spec.

Replaced both comments with one accurate statement covering the two things that actually matter: treat the source's slices as read-only, and every new AggregationConfig field must be added to this literal explicitly or it gets silently dropped (which for a visibility setting means failing open). Kept the fail-open rationale as you asked.

Fixed in 9bf1406.