Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates vMCP session-scoped routing so that when an individual backend fails mid-session, the returned error clearly identifies which backend failed (per issue #3875), while keeping the original cause unwrappable.
Changes:
- Extended
lookupBackendto return the resolved*vmcp.BackendTargetalongside the backend session connection. - Wrapped backend errors in
CallTool,ReadResource, andGetPromptwith a backend-identifying"backend <id> unavailable: ..."message. - Added/updated unit tests to validate backend ID presence and session resilience for tool calls across multiple backends.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/vmcp/session/default_session.go | Returns backend target from lookup and wraps backend call errors with backend-identifying messages. |
| pkg/vmcp/session/default_session_test.go | Updates CallTool error assertions and adds multi-backend crash resilience tests for tool calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 178fbeaf56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4053 +/- ##
==========================================
+ Coverage 68.48% 68.56% +0.07%
==========================================
Files 446 447 +1
Lines 45573 45672 +99
==========================================
+ Hits 31211 31315 +104
+ Misses 11948 11942 -6
- Partials 2414 2415 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jerm-dro
left a comment
There was a problem hiding this comment.
Some nitpicks that you can address now or in another PR
Why: When a backend crashed mid-session, errors propagated with no indication of which backend failed, making it impossible for clients to distinguish a single-backend outage from a total session failure. This also violated the issue requirement that error messages identify the backend. Closes: #3875
Summary
Why: When a backend crashed mid-session, errors propagated with no indication of which backend failed, making it impossible for clients to distinguish a single-backend outage from a total session failure. This also violated the issue requirement that error messages identify the backend.
Fixes #3875
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
What changed:
lookupBackend now returns *vmcp.BackendTarget alongside the connection
CallTool, ReadResource, and GetPrompt wrap backend errors as backend "" unavailable: , keeping the original error unwrappable via errors.Is
Session resilience was already correct (one backend crash never terminates the session); this change makes the error surface match the spec
Affected components: pkg/vmcp/session/default_session.go, default_session_test.go
Does this introduce a user-facing change?
No
Special notes for reviewers