refactor(vmcp): pull optimizer and composite tool decoration into session factory#4231
refactor(vmcp): pull optimizer and composite tool decoration into session factory#4231
Conversation
yrobla
requested review from
JAORMX,
amirejaz,
jerm-dro and
jhrozek
as code owners
There was a problem hiding this comment.
Pull request overview
Refactors vMCP session creation so composite-tool and optimizer decoration happens inside the MultiSessionFactory at construction time, instead of being applied as post-session steps in the server registration hook.
Changes:
- Added a
session.Decoratorconcept and aNewDecoratingFactorywrapper to apply session decorators duringMakeSessionWithID. - Moved composite-tool workflow conversion/filter/validation helpers into
pkg/vmcp/session/compositetoolsand updated tests accordingly. - Introduced
adaptToolsForFactoryto build optimizer indexing tools without session-manager-specific termination behavior; removed legacy optimizer adapter re-exports.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/session/decorating_factory.go | Adds a decorating factory wrapper to apply decorators at session creation time. |
| pkg/vmcp/session/compositetools/workflow_converter.go | Moves and exports workflow→tool conversion/filtering/validation into the compositetools package. |
| pkg/vmcp/session/compositetools/workflow_converter_test.go | Updates tests to the new package and exported function names. |
| pkg/vmcp/session/compositetools/decorator.go | Refactors CallTool to use an early-return guard clause. |
| pkg/vmcp/server/server.go | Builds a decorated session factory up-front and removes server-level post-creation decoration steps. |
| pkg/vmcp/server/adapt_tools.go | Adds a factory-time tool adapter for optimizer indexing and tool invocation. |
| pkg/vmcp/server/workflow_converter_test.go | Removes tests that were moved alongside the converter into compositetools. |
| pkg/vmcp/server/adapter/optimizer_adapter.go | Deletes legacy constant re-exports (now provided by optimizerdec). |
| pkg/vmcp/server/adapter/optimizer_adapter_test.go | Deletes the trivial test for the removed constants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f018c4aa9
ℹ️ 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4231 +/- ##
========================================
Coverage 69.29% 69.30%
========================================
Files 470 471 +1
Lines 47197 47323 +126
========================================
+ Hits 32706 32797 +91
- Misses 11957 11996 +39
+ Partials 2534 2530 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
size/L
There was a problem hiding this comment.
Pull request overview
This PR refactors vMCP session construction so that composite-tools and optimizer decoration happens inside the MultiSessionFactory (via a new decorating factory wrapper), instead of being applied as server-level post-creation steps during session registration. It also relocates workflow→tool conversion helpers into the compositetools package and removes a redundant optimizer adapter re-export.
Changes:
- Add
session.Decorator+NewDecoratingFactoryto apply session decorators at creation time. - Move workflow conversion/filter/validation helpers and tests into
pkg/vmcp/session/compositetools. - Introduce
adaptToolsForFactoryfor optimizer indexing and remove the old optimizer adapter constants + tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/session/decorating_factory.go | Adds a factory wrapper that chains decorators when creating MultiSessions. |
| pkg/vmcp/session/compositetools/workflow_converter.go | Moves and exports workflow conversion/filter/validation helpers into compositetools. |
| pkg/vmcp/session/compositetools/workflow_converter_test.go | Updates tests to the new compositetools package and exported function names. |
| pkg/vmcp/session/compositetools/decorator.go | Refactors CallTool with an early-return guard for non-composite tools. |
| pkg/vmcp/server/server.go | Moves optimizer factory wiring into New(), builds a decorating session factory, and removes post-registration decoration. |
| pkg/vmcp/server/adapt_tools.go | Adds a helper to adapt session tools for optimizer indexing with explicit termination callback wiring. |
| pkg/vmcp/server/workflow_converter_test.go | Removes workflow→tool conversion/conflict tests from server (now covered in compositetools). |
| pkg/vmcp/server/adapter/optimizer_adapter.go | Deleted redundant optimizer tool-name constants re-export. |
| pkg/vmcp/server/adapter/optimizer_adapter_test.go | Deleted trivial test for the removed constants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
bot
added
size/L
github-actions
bot
added
size/L
There was a problem hiding this comment.
Pull request overview
This refactor shifts vMCP session “decoration” (composite tools + optimizer) from server-side post-registration steps into the session factory, so sessions are fully constructed (and decorated) at creation time.
Changes:
- Introduce
session.DecoratorandNewDecoratingFactoryto apply session wrappers duringMakeSessionWithID. - Move workflow→tool conversion/filtering/validation utilities into
session/compositetoolsand export them. - Add
adaptToolsForFactoryto build optimizer-indexableServerTools without depending on sessionmanager internals; remove old optimizer adapter re-export files.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/session/decorating_factory.go | Adds a factory wrapper that applies decorators at session creation time with error/cleanup semantics. |
| pkg/vmcp/session/decorating_factory_test.go | Unit tests for decorator ordering and failure/cleanup behavior. |
| pkg/vmcp/session/compositetools/workflow_converter.go | Moves & exports workflow filtering/conversion/conflict validation into compositetools. |
| pkg/vmcp/session/compositetools/workflow_converter_test.go | Updates tests to compositetools package and exported function names. |
| pkg/vmcp/session/compositetools/decorator.go | Simplifies CallTool control flow via early-return guard. |
| pkg/vmcp/server/server.go | Wires composite tools + optimizer decoration into a decorating session factory; removes post-registration decoration steps. |
| pkg/vmcp/server/adapt_tools.go | New helper to adapt session tools for optimizer indexing with explicit session-termination callback. |
| pkg/vmcp/server/workflow_converter_test.go | Removes tests that moved to compositetools package. |
| pkg/vmcp/server/adapter/optimizer_adapter.go | Deleted (was only re-exporting constants). |
| pkg/vmcp/server/adapter/optimizer_adapter_test.go | Deleted along with adapter file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
This refactor moves session decoration (optimizer + composite tools) from server-side, post-creation mutation into the session-manager’s factory construction so sessions are fully decorated at creation time. This aligns the vMCP session lifecycle around a single “create once, ready-to-use” factory flow and reduces server hook complexity.
Changes:
- Added a generic
session.Decorator+NewDecoratingFactorywrapper to apply decorators duringMakeSessionWithID. - Moved composite tool conversion/filtering/validation utilities into
pkg/vmcp/internal/compositetoolsand updated call sites. - Moved optimizer + workflow executor telemetry wiring from
server/telemetry.gointosessionmanagerfactory construction, and removed old adapter re-export constants.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/session/decorating_factory.go | Adds a decorator-based factory wrapper that applies session decorators at creation time. |
| pkg/vmcp/session/decorating_factory_test.go | Unit tests for decorator ordering, error handling, and session closure behavior. |
| pkg/vmcp/server/workflow_executor_adapter.go | Removes server-level workflow executor adapter (logic moved into sessionmanager factory). |
| pkg/vmcp/server/workflow_converter_test.go | Removes tests for converter functions that were moved into internal/compositetools. |
| pkg/vmcp/server/telemetry.go | Removes optimizer/workflow telemetry concerns from server package (leaving backend telemetry). |
| pkg/vmcp/server/telemetry_test.go | Removes optimizer telemetry tests from server package. |
| pkg/vmcp/server/sessionmanager/factory.go | New: builds decorating factory (composite tools + optimizer) and contains optimizer/workflow telemetry wiring + tool adaptation helper. |
| pkg/vmcp/server/sessionmanager/session_manager.go | Changes New signature to accept FactoryConfig, returns cleanup function + error, and wires decorating factory internally. |
| pkg/vmcp/server/sessionmanager/session_manager_test.go | Updates tests to new sessionmanager.New signature. |
| pkg/vmcp/server/sessionmanager/telemetry_test.go | New: optimizer telemetry tests relocated under sessionmanager. |
| pkg/vmcp/server/server.go | Simplifies registration path: server now relies on session manager to return fully decorated sessions/tools. |
| pkg/vmcp/server/adapter/optimizer_adapter.go | Deletes adapter-level constant re-exports. |
| pkg/vmcp/server/adapter/optimizer_adapter_test.go | Deletes trivial test for removed constants. |
| pkg/vmcp/server/adapter/handler_factory.go | Updates composite tools import path to internal/compositetools. |
| pkg/vmcp/internal/compositetools/workflow_converter.go | Exposes workflow conversion/filter/validation helpers as exported functions under correct package. |
| pkg/vmcp/internal/compositetools/workflow_converter_test.go | Updates package name and references to exported converter functions. |
| pkg/vmcp/internal/compositetools/decorator.go | Refactors CallTool with an early return guard clause (reduced nesting). |
| pkg/vmcp/internal/compositetools/decorator_test.go | Updates imports to new internal compositetools location. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…sion factory Move optimizer and composite tool construction from server-level post-session decoration into the MultiSessionFactory via a new DecoratingFactory wrapper. Sessions are now fully formed at creation time — the server unconditionally calls the factory and always gets a decorated MultiSession. - Add session.Decorator type and NewDecoratingFactory to apply decorators at session construction time - Add adaptToolsForFactory helper for optimizer tool indexing without session-manager-specific terminate callbacks - Move composite tool converter functions (FilterWorkflowDefsForSession, ConvertWorkflowDefsToTools, ValidateNoToolConflicts) out of server/ into the compositetools package where they belong - Remove DecorateSession and GetMultiSession from SessionManager interface and implementation — no longer needed - Delete adapter/optimizer_adapter.go (re-export of constants already in optimizerdec); delete its trivial test - Apply early-return guard clause in compositetools.CallTool to reduce nesting Closes: #3872
github-actions
bot
added
size/XL
github-actions
bot
dismissed
their stale review
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
This PR refactors vMCP session construction so sessions are fully decorated at creation time (via a decorating factory) rather than being post-processed by the server after registration. This centralizes optimizer + composite tool wiring inside the session manager’s factory construction flow and moves composite-tool conversion utilities into the composite tools package.
Changes:
- Add
session.Decorator+session.NewDecoratingFactoryto apply session decorators duringMakeSessionWithID. - Introduce
sessionmanager.FactoryConfig+factory.goto build the decorated session factory (composite tools, optimizer, telemetry) inside the session manager. - Move/rename composite tool conversion helpers into
pkg/vmcp/internal/compositetoolsand relocate optimizer telemetry tests accordingly.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/session/decorating_factory.go | New decorating factory wrapper applying session decorators during construction |
| pkg/vmcp/session/decorating_factory_test.go | Unit tests for decoration ordering and error/cleanup behavior |
| pkg/vmcp/server/sessionmanager/session_manager.go | New now builds a decorating factory from config and returns a cleanup function |
| pkg/vmcp/server/sessionmanager/factory.go | New: constructs composite-tools + optimizer decorators and optimizer/workflow telemetry wrapping |
| pkg/vmcp/server/sessionmanager/telemetry_test.go | Moved optimizer telemetry tests from server package into sessionmanager |
| pkg/vmcp/server/sessionmanager/session_manager_test.go | Updated for new New(...) (*Manager, cleanup, error) signature |
| pkg/vmcp/server/server.go | Server now delegates optimizer/composite decoration setup to sessionmanager factory config |
| pkg/vmcp/server/telemetry.go | Removes optimizer/workflow executor telemetry code moved into sessionmanager |
| pkg/vmcp/server/telemetry_test.go | Removes optimizer telemetry tests moved into sessionmanager |
| pkg/vmcp/server/workflow_executor_adapter.go | Deleted; functionality re-homed into sessionmanager factory wiring |
| pkg/vmcp/server/workflow_converter_test.go | Removes tests for functions moved out of server package |
| pkg/vmcp/server/adapter/handler_factory.go | Updates import path to internal compositetools package |
| pkg/vmcp/server/adapter/optimizer_adapter.go | Deleted (constants already live in optimizer decorator) |
| pkg/vmcp/server/adapter/optimizer_adapter_test.go | Deleted trivial constant test |
| pkg/vmcp/internal/compositetools/workflow_converter.go | Renames/moves workflow conversion & conflict validation helpers into compositetools |
| pkg/vmcp/internal/compositetools/workflow_converter_test.go | Updates tests to new package name + exported function names |
| pkg/vmcp/internal/compositetools/decorator.go | Refactor CallTool with early-return guard to reduce nesting |
| pkg/vmcp/internal/compositetools/decorator_test.go | Updates import path after compositetools move |
Comments suppressed due to low confidence (1)
pkg/vmcp/internal/compositetools/workflow_converter.go:163
- ValidateNoToolConflicts no longer appears to have direct unit test coverage after being moved/renamed (the previous server/workflow_converter_test cases were removed, and there are no remaining references to ValidateNoToolConflicts() in tests). Please add tests covering both the no-conflict and conflict cases (including verifying the error wraps vmcp.ErrToolNameConflict and lists the conflicting names).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Move optimizer and composite tool construction from server-level post-session decoration into the MultiSessionFactory via a new DecoratingFactory wrapper. Sessions are now fully formed at creation time — the server unconditionally calls the factory and always gets a decorated MultiSession.
Fixes #3872
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
Does this introduce a user-facing change?
Special notes for reviewers
Large PR Justification
This is a complete pr with isolated functionality. It includes moving relevant pieces of code and important refactors. Cannot be split.