Skip to content

Avoid priority annexing unlisted tools #6127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kocaemre wants to merge 1 commit into
base: main
Choose a base branch
from
+90 −25
Open

Avoid priority annexing unlisted tools #6127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions pkg/vmcp/aggregator/conflict_resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,53 @@ func TestPriorityConflictResolver(t *testing.T) {
"teams_send_message": vmcp.ConflictStrategyPrefix, // Prefix fallback used
},
},
{
name: "mixed listed and unlisted conflict uses prefix fallback",
priorityOrder: []string{"github"},
toolsByBackend: map[string][]vmcp.Tool{
"github": {
{Name: "deploy", Description: "GitHub deploy"},
},
"prod": {
{Name: "deploy", Description: "Production deploy"},
},
},
wantCount: 2,
wantWinners: map[string]string{
"github_deploy": "github",
"prod_deploy": "prod",
},
wantStrategies: map[string]vmcp.ConflictResolutionStrategy{
"github_deploy": vmcp.ConflictStrategyPrefix,
"prod_deploy": vmcp.ConflictStrategyPrefix,
},
},
{
name: "three-way mixed listed and unlisted conflict uses prefix fallback",
priorityOrder: []string{"github", "staging"},
toolsByBackend: map[string][]vmcp.Tool{
"github": {
{Name: "deploy", Description: "GitHub deploy"},
},
"staging": {
{Name: "deploy", Description: "Staging deploy"},
},
"prod": {
{Name: "deploy", Description: "Production deploy"},
},
},
wantCount: 3,
wantWinners: map[string]string{
"github_deploy": "github",
"staging_deploy": "staging",
"prod_deploy": "prod",
},
wantStrategies: map[string]vmcp.ConflictResolutionStrategy{
"github_deploy": vmcp.ConflictStrategyPrefix,
"staging_deploy": vmcp.ConflictStrategyPrefix,
"prod_deploy": vmcp.ConflictStrategyPrefix,
},
},
{

@ChrisJBurns ChrisJBurns Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Missing test coverage for 3+-way conflicts mixing listed and unlisted backends (Consensus: 8/10)

This case covers exactly 1 listed + 1 unlisted backend. No case proves that a conflict with 2+ listed backends plus 1 unlisted backend still prefixes all candidates, rather than letting the listed backends fall back to rank-comparison among themselves — the scenario this PR's own reviewer notes call out. hasUnlistedCandidate/addPrefixedCandidates already handle this correctly by inspection, but nothing pins it down.

Consider adding a sibling case, e.g.:

{
	name:          "three-way conflict with unlisted backend forces prefix for all",
	priorityOrder: []string{"a", "b"},
	toolsByBackend: map[string][]vmcp.Tool{
		"a":        {{Name: "deploy"}},
		"b":        {{Name: "deploy"}},
		"unlisted": {{Name: "deploy"}},
	},
	wantCount: 3,
	wantWinners: map[string]string{
		"a_deploy":        "a",
		"b_deploy":        "b",
		"unlisted_deploy": "unlisted",
	},
	wantStrategies: map[string]vmcp.ConflictResolutionStrategy{
		"a_deploy":        vmcp.ConflictStrategyPrefix,
		"b_deploy":        vmcp.ConflictStrategyPrefix,
		"unlisted_deploy": vmcp.ConflictStrategyPrefix,
	},
},

Raised by: test-coverage

name: "empty priority order",
priorityOrder: []string{},
Expand Down
68 changes: 43 additions & 25 deletions pkg/vmcp/aggregator/priority_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@ import (
)

// PriorityConflictResolver implements priority-based conflict resolution.
// The first backend in the priority order wins; conflicting tools from
// lower-priority backends are dropped.
// When every conflicting backend is listed in the priority order, the first
// backend in that order wins and lower-priority tools are dropped.
//
// For backends not in the priority list, conflicts are resolved using
// prefix strategy as a fallback (prevents data loss).
// When any conflicting backend is absent from the priority list, all candidates
// in that conflict use the prefix strategy as a fallback to prevent a listed
// backend from annexing the bare tool name.
type PriorityConflictResolver struct {
// PriorityOrder defines the priority of backends (first has highest priority).
PriorityOrder []string
Expand Down Expand Up @@ -82,35 +83,23 @@ func (r *PriorityConflictResolver) ResolveToolConflicts(
continue
}

// Conflict detected - choose the highest priority backend
winner := r.selectWinner(candidates)
if winner == nil {
// All candidates are from backends not in priority list
// Use prefix strategy as fallback to avoid data loss
if r.hasUnlistedCandidate(candidates) {
// A collision involving a backend outside priorityOrder cannot be safely
// rank-compared. Prefix every candidate instead of awarding the bare name
// to a listed backend, which could silently redirect name-only policies.
backendIDs := make([]string, len(candidates))
for i, c := range candidates {
backendIDs[i] = c.BackendID
}
slog.Debug("tool exists in backends not in priority order, using prefix fallback",
slog.Warn("tool conflict includes backend not in priority order, using prefix fallback",
"tool", toolName, "backends", backendIDs)

@ChrisJBurns ChrisJBurns Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Fallback rename of a listed backend's tool only logged at Debug (Consensus: 7/10)

Per the project's logging convention, WARN is for fallback behavior. This path now fires whenever any candidate is unlisted (broadened from "all unlisted"), and can rename a previously bare-named, Cedar-policy-bound tool belonging to a listed backend — with no signal above Debug that an operator's existing policy just stopped matching.

Suggested change
"tool", toolName, "backends", backendIDs)
slog.Warn("tool conflict includes backend not in priority order, using prefix fallback",

Raised by: correctness-security


// Apply prefix strategy to these unmapped backends
for _, candidate := range candidates {
prefixedName := r.prefixResolver.applyPrefix(candidate.BackendID, toolName)
resolved[prefixedName] = &ResolvedTool{
ResolvedName: prefixedName,
OriginalName: toolName,
Description: candidate.Tool.Description,
InputSchema: candidate.Tool.InputSchema,
OutputSchema: candidate.Tool.OutputSchema,
Annotations: candidate.Tool.Annotations,
BackendID: candidate.BackendID,
ConflictResolutionApplied: vmcp.ConflictStrategyPrefix, // Fallback used prefix
}
}
r.addPrefixedCandidates(resolved, toolName, candidates)
continue
}

// Conflict detected among only listed backends; choose the highest priority backend.
winner := r.selectWinner(candidates)
resolved[toolName] = &ResolvedTool{
ResolvedName: toolName,
OriginalName: toolName,
Expand Down Expand Up @@ -142,8 +131,37 @@ func (r *PriorityConflictResolver) ResolveToolConflicts(
return resolved, nil
}

func (r *PriorityConflictResolver) hasUnlistedCandidate(candidates []toolWithBackend) bool {
for _, candidate := range candidates {
if _, exists := r.priorityMap[candidate.BackendID]; !exists {
return true
}
}
return false
}

func (r *PriorityConflictResolver) addPrefixedCandidates(
resolved map[string]*ResolvedTool,
toolName string,
candidates []toolWithBackend,
) {
for _, candidate := range candidates {
prefixedName := r.prefixResolver.applyPrefix(candidate.BackendID, toolName)
resolved[prefixedName] = &ResolvedTool{
ResolvedName: prefixedName,
OriginalName: toolName,
Description: candidate.Tool.Description,
InputSchema: candidate.Tool.InputSchema,
OutputSchema: candidate.Tool.OutputSchema,
Annotations: candidate.Tool.Annotations,
BackendID: candidate.BackendID,
ConflictResolutionApplied: vmcp.ConflictStrategyPrefix, // Fallback used prefix
}
}
}

// selectWinner chooses the tool from the highest-priority backend.
// Returns nil if none of the candidates are in the priority list.
// Callers should only pass candidates from backends that are in the priority list.
func (r *PriorityConflictResolver) selectWinner(candidates []toolWithBackend) *toolWithBackend {
var winner *toolWithBackend
winnerPriority := -1
Expand Down
Loading