Avoid priority annexing unlisted tools - #6127
Conversation
Signed-off-by: Emre K <110906681+kocaemre@users.noreply.github.com>
ChrisJBurns
left a comment
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: correctness-security, test-coverage, code-quality (pr-reviewer specialists)
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | Missing test coverage for 3+-way conflicts mixing listed and unlisted backends | 8/10 | MEDIUM | Suggest |
| 2 | selectWinner's nil-return doc comment is stale relative to its only call site |
8/10 | MEDIUM | Suggest |
| 3 | Fallback rename of a listed backend's tool is only logged at Debug | 7/10 | MEDIUM | Discuss |
Overall
This PR closes #6097 correctly: it inverts the conflict-resolution control flow so that any tool-name collision involving a backend absent from priorityOrder is treated as not safely rank-comparable, prefixing every candidate instead of letting a listed backend silently annex the bare name. Tracing the fix against the issue's exact repro steps confirms the annexation hole is closed, and the "forbid fails closed" invariant is preserved since the fallback renames but never drops candidates.
The remaining findings are refinements, not correctness gaps: the fix already generalizes to N-way conflicts by checking "any candidate unlisted" rather than "exactly one," but no test proves it; a stale doc comment on selectWinner no longer matches the guarantees of its only call site; and the fallback path — now reached more often since it broadened from "all unlisted" to "any unlisted" — renames a listed backend's previously-bare, Cedar-policy-bound tool name while only logging at Debug.
Documentation
No documentation files are affected by this diff; consider a one-line note in the PR description that the rename can also affect a listed backend's tool when it collides with an unlisted one, not just the unlisted backend's tool.
Generated with Claude Code
| "prod_deploy": vmcp.ConflictStrategyPrefix, | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
[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
| // Returns nil if none of the candidates are in the priority list. | ||
| func (r *PriorityConflictResolver) selectWinner(candidates []toolWithBackend) *toolWithBackend { |
There was a problem hiding this comment.
[MEDIUM] selectWinner's doc comment is stale relative to its only call site (Consensus: 8/10)
selectWinner is now only invoked after hasUnlistedCandidate confirms every candidate is listed, so it can never return nil at this call site — yet the doc comment doesn't say so, and the caller dereferences winner.Tool... without a nil check. Not a live bug today, but a latent nil-pointer-dereference hazard for any future caller that skips the hasUnlistedCandidate guard.
| // Returns nil if none of the candidates are in the priority list. | |
| func (r *PriorityConflictResolver) selectWinner(candidates []toolWithBackend) *toolWithBackend { | |
| // selectWinner chooses the tool from the highest-priority backend. | |
| // Returns nil if none of the candidates are in the priority list. Callers must | |
| // ensure at least one candidate is present in priorityMap (see hasUnlistedCandidate) | |
| // before dereferencing the result unconditionally. |
Raised by: correctness-security, code-quality
| } | ||
| slog.Debug("tool exists in backends not in priority order, using prefix fallback", | ||
| slog.Debug("tool conflict includes backend not in priority order, using prefix fallback", | ||
| "tool", toolName, "backends", backendIDs) |
There was a problem hiding this comment.
[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.
| "tool", toolName, "backends", backendIDs) | |
| slog.Warn("tool conflict includes backend not in priority order, using prefix fallback", |
Raised by: correctness-security
2 participants
Summary
priorityOrder.Tool::"name"permit to a different backend's tool when a listed backend later introduced the same name.deployconflict so both tools remain reachable asgithub_deployandprod_deploy.Fixes #6097
Type of change
Test plan
task test)task test-e2e)task lint-fix)Manual testing:
PATH=/usr/local/go/bin:/root/go/bin:$PATH go test ./pkg/vmcp/aggregator -run 'TestPriorityConflictResolver/mixed_listed_and_unlisted_conflict_uses_prefix_fallback'failed with only one resolved tool and the unlisted backend dropped.PATH=/usr/local/go/bin:/root/go/bin:$PATH go test ./pkg/vmcp/aggregator -run TestPriorityConflictResolverPATH=/usr/local/go/bin:/root/go/bin:$PATH go test ./pkg/vmcp/aggregatorPATH=/usr/local/go/bin:/root/go/bin:$PATH go test -race ./pkg/vmcp/aggregator -run TestPriorityConflictResolverPATH=/usr/local/go/bin:/root/go/bin:$PATH task lintgit diff --checkAPI Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Does this introduce a user-facing change?
Yes. Under the priority conflict strategy, a tool-name collision that includes any backend absent from
priorityOrdernow advertises the conflicting candidates with workload prefixes instead of selecting a bare-name winner.Special notes for reviewers
Conflicts where all candidates are listed in
priorityOrderkeep the existing priority-winner behavior. The prefix fallback is only for conflicts that include at least one unlisted backend, where no complete rank comparison exists.