Repository navigation
feat(mcpx): sampling.answerLocally — answer upstream sampling from the configured provider layer (#1925 slice 3) - #4947
Conversation
…e configured provider layer (#1925 slice 3)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 53 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. CodeRabbit Verify
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🤖 Agent note · done
Codex findings disposition (merge gate): the Codex code review on head |
What
Slice 3 of #1925:
sampling.answerLocally— when enabled, mcpx answers an upstream server'ssampling/createMessagerequest from its own configured provider layer instead of relaying it through the broker to a caller that (per #1925) never declares the sampling capability.Queue task 8019. Slices 1–2 are landed: slice 1 PR #2592 (internal/model provider layer +
model.*settings), slice 2 PR #4836 (provider-keyedmodel.providers, named orderedmodel.profiles, cross-provider fallback ininternal/modelResolve).Changes
internal/settings: new daemon settingsampling.answerLocally(bool, defaultfalse) in the one settings registry;docs/configuration.mdregenerated; effect-coverage exemption added with the same model-layer rationale asmodel.*, andEffectExemptionPindeliberately raised 201 → 202 with the reason recorded here and in the pin comment.internal/daemon:ConsumerPolicy.SamplingAnswerLocallypopulated from the set;Registry.answerServerdivertssampling/createMessageto the newsampleLocallywhen the setting is on (before the broker check — a local answer needs no broker). Default off: the relay path is byte-for-byte unchanged.internal/daemon/sampling_local.go: the local answer path.elicit.ValidateSampleRequest,-32602on shape errors), and validates its own answer withelicit.ValidateSampleResult.internal/model.Resolveover themodel.*policy withSamplerdeliberately nil — resolving the sampling backend here would relay to the caller, the path this setting replaces; a sampling-only configuration therefore resolves to unavailable, honestly.maxTokenscapped bymodel.budget(modelMaxTokens),model.timeoutas the Generate deadline.modelPreferenceshints name the model when given; otherwisemodel.defaultapplies through Resolve/Generate.Tests
New
internal/daemon/sampling_local_test.go:model.provider=samplingonly → clean failure, never a silent relay.model.budgetcapsmaxTokens, tools / non-text blocks are refused.Verified on the laptop worktree:
go test ./internal/daemon/ -run 'TestSample|TestSampling|TestConsumer|TestHook|TestElicit'passes;go vet ./internal/daemon/ ./internal/settings/passes;internal/modelpasses.Pre-existing failures, verified on clean main (not this PR):
internal/settingsfails on main inTestEveryDocumentHasAChapter,TestNoUndeclaredEnvRead(MCPX_ADAPTER_DECLARATION_SHA256),TestEverySettingHasAnEffectProbeOrAnExemption(proto.* / script.* entries only, after this PR's exemption),TestTheScriptEnvironmentIsWhatTheCodeSets, andTestTheEnvironmentDocIsCurrent(the generated table's padding differs). This PR deliberately does not regeneratedocs/environment.md— that churn is unrelated to this change.Out of scope
model.provider=plugin) — depends on the opencode v2 plugin work; remains the final remainder recorded on mcpx: where the model comes from — a provider layer mcpx owns, model profiles, and life after MCP Sampling's deprecation #1925.Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.