Conversation
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
There was a problem hiding this comment.
Pull request overview
Refactors vMCP session token-binding / hijack-prevention code by moving the security implementation into an internal package and centralizing the “compute + persist metadata + wrap session” logic, while also simplifying/reshaping the associated tests and removing prior integration coverage.
Changes:
- Move token-binding security utilities + hijack-prevention decorator into
pkg/vmcp/session/internal/securityand introducePreventSessionHijacking(...)as the single entry point. - Update
MultiSessionfactory to apply hijack-prevention via the new internal security helper and remove the old decorator/standalone security package. - Reshuffle tests (unit tests moved/updated) and remove the previous server token-binding integration test; simplify v2 server integration fake factory setup.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/session/token_binding_test.go | Updates tests to reference internal security metadata keys and changes validation strategy for defensive-copy behavior. |
| pkg/vmcp/session/security/security.go | Removes the previous (non-internal) security utilities package. |
| pkg/vmcp/session/internal/security/security.go | Adds internalized token-binding + hijack-prevention implementation (hashing, salt, decorator, wrapper function). |
| pkg/vmcp/session/internal/security/security_test.go | Updates security unit tests to use the new internal package location and adds tests for ShouldAllowAnonymous. |
| pkg/vmcp/session/internal/security/hijack_prevention_test.go | Adds focused tests for validateCaller edge cases, concurrency, and basic PreventSessionHijacking metadata behavior. |
| pkg/vmcp/session/hijack_prevention_decorator.go | Removes the previous session-level decorator implementation. |
| pkg/vmcp/session/factory.go | Switches factory to internal/security.PreventSessionHijacking(...) and removes in-factory token binding computation/metadata writes. |
| pkg/vmcp/server/token_binding_integration_test.go | Removes end-to-end HTTP integration tests that exercised token binding with Authorization headers. |
| pkg/vmcp/server/sessionmanager/session_manager.go | Replaces use of the removed helper with inline allowAnonymous logic. |
| pkg/vmcp/server/session_management_v2_integration_test.go | Removes fake token-hash metadata population logic from the v2 integration fakes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4028 +/- ##
==========================================
- Coverage 68.62% 68.62% -0.01%
==========================================
Files 445 445
Lines 45374 45372 -2
==========================================
- Hits 31140 31135 -5
- Misses 11827 11830 +3
Partials 2407 2407 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Simplify code, remove tests and move related packages Related-to: #3867
jerm-dro
left a comment
There was a problem hiding this comment.
Approving since I know other issues are stacked on this. I've called at a few things that would be good to change in subsequent PRs.
Simplify code, remove tests and move related packages
Related-to: #3867
Large PR Justification
This is a code refactor, that means moving and deleting files, so that's the reason for the big diff