Remove legacy optimizer decorator path - #6119
Conversation
Signed-off-by: Emre K <110906681+kocaemre@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6119 +/- ##
==========================================
+ Coverage 72.69% 72.77% +0.08%
==========================================
Files 741 741
Lines 77212 77147 -65
==========================================
+ Hits 56127 56142 +15
+ Misses 17127 17050 -77
+ Partials 3958 3955 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ChrisJBurns
left a comment
There was a problem hiding this comment.
Two stale doc comments elsewhere in this file describe the removed decorator path — not required for this PR, but worth a quick follow-up.
| ) | ||
|
|
||
| sm.factory = buildDecoratingFactory(cfg, optimizerFactory, sm.Terminate) | ||
| sm.factory = vmcpsession.NewDecoratingFactory(cfg.Base) |
There was a problem hiding this comment.
New()'s doc comment at lines 95-96 still says: "It builds the decorating session factory from cfg, wiring the optimizer and composite tool layers internally." That's no longer accurate — the factory is now just cfg.Base with no decorators, and the optimizer is surfaced solely via sm.optimizerFactory/OptimizerFactory() for the Serve layer to consume. Worth rewording.
| ) | ||
|
|
||
| sm.factory = buildDecoratingFactory(cfg, optimizerFactory, sm.Terminate) | ||
| sm.factory = vmcpsession.NewDecoratingFactory(cfg.Base) |
There was a problem hiding this comment.
The comment at lines 132-133 above sm := &Manager{...} says "Build the Manager first so we can reference sm.Terminate and sm.sessions directly in closures..." — with buildDecoratingFactory (and its sm.Terminate param) gone, nothing built in New() references sm.Terminate anymore, only sm.loadSession/sm.checkSession/sm.sessions. The comment should drop the sm.Terminate mention.
Summary
AdvertiseFromCorepath is now rejected bysessionmanager.New, so the old factory-level optimizer decorator branch can no longer run.Manager.OptimizerFactory().Fixes #6103
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/server/sessionmanagerPATH=/usr/local/go/bin:/root/go/bin:$PATH go test ./pkg/vmcp/server/sessionmanager ./pkg/vmcp/serverPATH=/usr/local/go/bin:/root/go/bin:$PATH go test -race ./pkg/vmcp/server/sessionmanagerPATH=/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?
No.
Special notes for reviewers
This is intentionally a deletion-only follow-up to the constructor guard merged in #6033. The optimizer is still resolved, cleaned up by the manager, and consumed by the Serve layer when enabled; the removed path was unreachable because
Newrejects an optimizer unlessAdvertiseFromCoreis set.