Conversation
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
|
@kocaemre you able to address the above? |
b9b3789 to
0fa55cc
Compare
|
Addressed, thanks. I added the 3-way mixed listed/unlisted regression, updated the stale resolver comments, and changed the prefix-fallback log from Debug to Warn. Also updated the PR description with the reviewer follow-up and latest verification commands. |
github-actions
Bot
added
size/S
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6127 +/- ##
==========================================
- Coverage 78.98% 78.91% -0.08%
==========================================
Files 782 782
Lines 78065 78058 -7
==========================================
- Hits 61658 61597 -61
- Misses 16402 16456 +54
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
One remaining legacy I attempted to rerun the failed check from the CLI; if GitHub permissions allow it, this comment can be ignored. Otherwise, could a maintainer rerun that failed legacy matrix job? |
0fa55cc to
47e17ca
Compare
|
Refreshed this PR branch onto current No code changes beyond replaying the existing PR commit on top of current upstream. Local verification in this cron environment: PATH=/usr/local/go/bin:/root/go/bin:$PATH go test -ldflags=-extldflags=-Wl,-w -race ./pkg/vmcp/aggregator -run TestPriorityConflictResolver
# ok github.com/stacklok/toolhive/pkg/vmcp/aggregator 1.215s
PATH=/usr/local/go/bin:/root/go/bin:$PATH golangci-lint run --allow-parallel-runners ./pkg/vmcp/aggregator/...
# 0 issues.
PATH=/usr/local/go/bin:/root/go/bin:$PATH go vet ./pkg/vmcp/aggregator/...
# passed
git diff --check origin/main..HEAD
# passed
git log --format='%h %s%n%b' origin/main..HEAD
# 47e17ca6 Avoid priority annexing unlisted tools
# Signed-off-by: Emre K <110906681+kocaemre@users.noreply.github.com>I also attempted the full repo GitHub had not populated the refreshed check rollup yet immediately after the push ( |
|
Follow-up on the refreshed CI run: the only red check I see is Failure evidence from the job log: This looks like an E2E runner port collision in I attempted to rerun the failed job from the CLI, but GitHub refused it for my account on this repo: gh api -X POST repos/stacklok/toolhive/actions/runs/34322045823/rerun-failed-jobs
# HTTP 403: Must have admin rights to Repository.Could a maintainer rerun that failed E2E core job when convenient? |
47e17ca to
0944c34
Compare
|
Refreshed this PR branch onto current No code changes beyond replaying the existing priority-conflict fix on top of current upstream. The previous failed GitHub check looked unrelated to this PR: Local verification after the refresh: git diff --check origin/main..HEAD
# passed
PATH=/usr/local/go/bin:/root/go/bin:$PATH go test -run 'TestPriorityConflictResolver' ./pkg/vmcp/aggregator
# ok github.com/stacklok/toolhive/pkg/vmcp/aggregator 0.086s
PATH=/usr/local/go/bin:/root/go/bin:$PATH go test ./pkg/vmcp/aggregator
# ok github.com/stacklok/toolhive/pkg/vmcp/aggregator 0.091s
PATH=/usr/local/go/bin:/root/go/bin:$PATH golangci-lint run --allow-parallel-runners ./pkg/vmcp/aggregator
# 0 issues.I attempted the repo-level |
|
Investigated the new red Failure evidence from the job log: That failing test is in Local checks from the refreshed branch: git diff --check origin/main..HEAD
# passed
/usr/local/go/bin/go test -race ./pkg/vmcp/aggregator -run TestPriorityConflictResolver
# ok github.com/stacklok/toolhive/pkg/vmcp/aggregator 1.193s
/usr/local/go/bin/go test -ldflags=-extldflags=-Wl,-w -race ./pkg/transport/proxy/transparent -run TestRoundTripReinitializesPreservesNonUUIDBackendSessionID -count=1 -v
# PASS
# ok github.com/stacklok/toolhive/pkg/transport/proxy/transparent 1.114sI also tried to rerun the failed job, but GitHub rejected it for my account: gh api -X POST repos/stacklok/toolhive/actions/runs/34422700480/rerun-failed-jobs
# HTTP 403: Must have admin rights to Repository.This looks like another transient/unrelated test-runner failure rather than a regression from this PR. Could a maintainer rerun the failed |
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Panel review of 0944c34c0f441363d5368a5e157e58bae89f368b against main cdb04c94f2462553fc27572bc8db9bb88a0c3c78 and #6097.
Blocking — prefix fallback is an authorization fail-open
The mixed listed/unlisted path prefixes every candidate (pkg/vmcp/aggregator/priority_resolver.go:86-98). That re-advertises a tool under a name outside existing policies, so a name-scoped forbid(... Tool::"deploy") that previously denied the tool no longer applies to github_deploy / prod_deploy. This is the exact reason the architecture requires priority losers to be dropped rather than aliased (docs/arch/10-virtual-mcp-architecture.md:177-180). With name-only Cedar authorization, retaining availability cannot override the fail-closed policy invariant. Drop all candidates in an unrankable mixed collision and log it at error level; add coverage combining a broad permit with a bare-name forbid.
Blocking — generated prefix names can collide and redirect a permitted call
addPrefixedCandidates writes directly into resolved (pkg/vmcp/aggregator/priority_resolver.go:143-160) without globally checking advertised-name collisions. For example, a listed github/deploy, unlisted prod/deploy, and an evil/github_deploy tool can all claim github_deploy; random map iteration determines the surviving route. A Cedar permit for Tool::"github_deploy" can therefore execute another backend's tool. Resolve/validate all proposed names before inserting routes, and drop every claimant to an ambiguous output name (the established prompts behavior at docs/arch/10-virtual-mcp-architecture.md:182-193); test the three-backend routing and authorization case.
The fallback also changes published tool names but does not update the operator-facing priority documentation or give an actionable warning; removing the unsafe fallback resolves that compatibility/documentation concern. Standards, reuse, and duplication checks found no other merge blockers.
|
Addressed the two blocking review findings in What changed:
Local verification: export PATH=/usr/local/go/bin:/root/go/bin:$PATH
git diff --check origin/main..HEAD
# passed
go test -run 'TestPriorityConflictResolver' ./pkg/vmcp/aggregator
# ok github.com/stacklok/toolhive/pkg/vmcp/aggregator 0.060s
go test -race ./pkg/vmcp/aggregator -run TestPriorityConflictResolver -count=1
# ok github.com/stacklok/toolhive/pkg/vmcp/aggregator 1.186s
go test -race ./pkg/vmcp/aggregator -count=1
# ok github.com/stacklok/toolhive/pkg/vmcp/aggregator 1.250s
golangci-lint run --allow-parallel-runners ./pkg/vmcp/aggregator
# 0 issues.Repo-level checks attempted: task lint
# failed in pre-existing/out-of-scope gci formatting issues:
# ../../work/toolhive/pkg/authserver/server/provider.go:391:1: File is not properly formatted (gci)
# pkg/authserver/server_impl.go:194:1: File is not properly formatted (gci)
task test
# failed before package results due gotestfmt panic:
# panic: BUG: Empty package name encountered.Those repo-level failures are outside |
|
CI/status follow-up for the review-fix head
No new code changes in this follow-up; this is just the evidence snapshot after CI completed on the pushed blocker fix. |
Signed-off-by: Emre K <110906681+kocaemre@users.noreply.github.com>
Signed-off-by: Emre K <110906681+kocaemre@users.noreply.github.com>
4389cc3 to
834d834
Compare
|
Refreshed this PR branch onto current No code changes beyond replaying the existing two PR commits on top of current upstream; DCO sign-offs are preserved. Local verification after the refresh: git diff --check upstream/main..HEAD
# passed
git log --format='%h %s%n%b' upstream/main..HEAD
# 834d8346 Drop unrankable priority conflicts
# Signed-off-by: Emre K <110906681+kocaemre@users.noreply.github.com>
# ee4954a2 Avoid priority annexing unlisted tools
# Signed-off-by: Emre K <110906681+kocaemre@users.noreply.github.com>
PATH=/usr/local/go/bin:/root/go/bin:$PATH go test -run 'TestPriorityConflictResolver' ./pkg/vmcp/aggregator
# ok github.com/stacklok/toolhive/pkg/vmcp/aggregator 0.055s
PATH=/usr/local/go/bin:/root/go/bin:$PATH go test -race ./pkg/vmcp/aggregator -run TestPriorityConflictResolver -count=1
# ok github.com/stacklok/toolhive/pkg/vmcp/aggregator 1.272sI also attempted the repo-level Taskfile lint after the rebase, but this cron runner's installed PATH=/usr/local/go/bin:/root/go/bin:$PATH task lint
# Error: can't load config: the Go language version (go1.26) used to build golangci-lint is lower than the targeted Go version (1.27.0)
# task: Failed to run task "lint": exit status 3GitHub CI has been retriggered on the refreshed head and was queued at the time of this comment. |
|
Current refreshed head Status snapshot from this run:
I also tried to re-run So the remaining blocker is only the stale |
4 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. That fallback prefixes every candidate in the collision, so a listed backend's previously bare tool name can also be renamed when it collides with an unlisted backend.Reviewer follow-up addressed: