fix(proxy): resolve force-model provider against live availability (#874) - #876
fix(proxy): resolve force-model provider against live availability (#874)#876devin-ai-integration[bot] wants to merge 4 commits into
Conversation
) resolveForceModelWithEffort picked a model's provider from Providers[0] unconditionally, ignoring excluded/unregistered providers. /force-model and x-weave-force-model could silently pin to a dead provider (e.g. makora when ROUTER_EXCLUDED_PROVIDERS excludes it), which the next turn's pin-lookup then dropped as if no pin existed (reason:"no_pin") — no error, no warning, and the ack message falsely reported success the whole session. - resolveForceModelWithEffort/resolveForceModel now take the deployment's live enabled-provider set and walk a model's bindings in catalog order for the first available one (same pattern as cluster.Scorer's resolveProviderFor), instead of hard-picking the primary binding. A known model with no available binding now returns provider="" so callers can reject cleanly. - applyForceModelHeader / handleForceModelCommand thread the enabled-provider set through and reject with a clear ack/log message when no binding is available, instead of writing a pin that will silently die on the next turn. - turnloop's forced-pin fall-through now logs a Warn when a stored pin's provider became unavailable, so the drop is distinguishable from ordinary no-pin automatic routing. Deliberately did not touch ROUTER_HARD_PIN_MODEL/PROVIDER as a fix — that disables the cluster scorer entirely for main-loop turns and was ruled out as a real solution.
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
PR author is not in the allowed authors list. |
|
Claude finished @devin-ai-integration[bot]'s task —— View job Comment-length lint
Posted 2 comment-length nits on |
workweave-bot
left a comment
workweave-bot
left a comment
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
| // resolveForceModelWithEffort strips a `:level` suffix and resolves catalog | ||
| // bindings in order against available. known remains true when no binding is | ||
| // available; nil available means unrestricted and selects the primary binding. |
There was a problem hiding this comment.
| // resolveForceModelWithEffort strips a `:level` suffix and resolves catalog | |
| // bindings in order against available. known remains true when no binding is | |
| // available; nil available means unrestricted and selects the primary binding. | |
| // resolveForceModelWithEffort strips a `:level` suffix and resolves catalog | |
| // bindings in order against available; known stays true when no binding is available. |
Was 3 lines; last clause ("selects the primary binding") restates the nil-unrestricted invariant already stated on resolveForceModel.
| // resolveForcedBinding narrows explicit openai/<id> inputs before walking | ||
| // bindings in catalog order. nativeMismatch distinguishes no required-provider | ||
| // binding from one that exists but is unavailable; nil available is unrestricted. |
There was a problem hiding this comment.
| // resolveForcedBinding narrows explicit openai/<id> inputs before walking | |
| // bindings in catalog order. nativeMismatch distinguishes no required-provider | |
| // binding from one that exists but is unavailable; nil available is unrestricted. | |
| // resolveForcedBinding walks bindings in catalog order against available. | |
| // nativeMismatch is true when requiredProvider has no binding; nil available is unrestricted. |
Was 3 lines; first clause ("narrows explicit openai/ inputs") restates what the requiredProvider filter does rather than explaining why.
|
Claude finished @devin-ai-integration[bot]'s task —— View job
No added comment blocks ≥ 3 consecutive lines found; nothing to flag. |
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR threads a provider-availability map through the force-model resolution path. resolveForceModel and resolveForceModelWithEffort now take an available map[string]struct{} argument, and a new helper resolveForcedBinding walks a model's provider bindings in catalog order to pick an available one (with nil meaning unrestricted). Callers were updated accordingly: applyForceModelHeader and handleForceModelCommand now accept an enabledProviders argument and add branches that decline to pin a model when no available provider binding is found, logging and returning synthetic messages instead. The agent-shadow eval call site passes nil since it applies its own gate. The command handler was also restructured from if/else chains into switch statements, and doc comments were revised throughout.
No blocking issues surfaced. 6 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 889 functions depend on the 291 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
NewService()— 156 callers, 3 callees - new:
TestRunTurnLoop_ForcedPin_FallsThroughWhenProviderUnavailable()— 0 callers, 6 callees
Verification — 889 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 853 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 1 more finding(s) on lines outside this diff (see the check run).
| ) | ||
|
|
||
| // A pin with an unavailable provider falls through to automatic routing (#874). | ||
| func TestRunTurnLoop_ForcedPin_FallsThroughWhenProviderUnavailable(t *testing.T) { |
There was a problem hiding this comment.
TestRunTurnLoop_ForcedPin_FallsThroughWhenProviderUnavailable()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR threads live provider availability into force-model resolution. It changes resolveForceModel/resolveForceModelWithEffort to accept an available provider map and adds a resolveForcedBinding helper that walks a model's provider bindings in catalog order to pick an available one (with nil meaning unrestricted). Callers (applyForceModelHeader, handleForceModelCommand, and the agent shadow eval route) are updated to pass provider availability and to handle the new "recognized model but no available provider" case by declining to pin and returning warning/acknowledgment messages instead.
No blocking issues surfaced. 6 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 889 functions depend on the 291 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
NewService()— 156 callers, 3 callees - new:
TestRunTurnLoop_ForcedPin_FallsThroughWhenProviderUnavailable()— 0 callers, 6 callees
Verification — 889 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 853 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 1 more finding(s) on lines outside this diff (see the check run).
| ) | ||
|
|
||
| // A pin with an unavailable provider falls through to automatic routing (#874). | ||
| func TestRunTurnLoop_ForcedPin_FallsThroughWhenProviderUnavailable(t *testing.T) { |
There was a problem hiding this comment.
TestRunTurnLoop_ForcedPin_FallsThroughWhenProviderUnavailable()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
3 participants
Summary
Convention-conforming rewrite of #875 by @NathanLively (fixes #874). Production behavior is unchanged from that PR; this one drops a duplicated helper, corrects an inaccurate godoc, and brings the added comments in line with the root AGENTS.md comment rule.
/force-modelandx-weave-force-modelpicked a provider viacatalog.Model.Providers[0]unconditionally, so a model whose primary binding is excluded (ROUTER_EXCLUDED_PROVIDERS) got pinned to a dead provider — the next turn's pin lookup read asno_pinand the scorer silently overwrote it, while the force-model ack reported success.resolveForceModel(WithEffort)take the enabled-provider set; a known model with no available binding returnsprovider == ""(distinct fromknown == false) soapplyForceModelHeader/handleForceModelCommandreject with an accurate ack +Warninstead of pinning.enabledProvidersis computed earlier inProxyMessages/ProxyOpenAIChatCompletionso it exists before force-model handling. Nothing between the old and new positions mutates the credential/header/subscription state those calls read, so the set is identical.turnlooplogs aWarnwhen a stored pin's provider goes unavailable mid-session, distinguishing that drop from ordinary no-pin routing. The pin row is left in place.nilavailable means unrestricted (used by the agent-shadow path, which applies its own gate).Delta vs #875: reuses the existing
sortedEnabledKeysinstead of addingsortedProviderKeys;resolveForceModelWithEffort's godoc no longer claims resolution happens viacatalog.ResolveBinding(it doesn't call it); added godocs/comments trimmed from 10–12-line multi-paragraph blocks to ≤5 lines, why-only.make precommitclean.Co-authored-by: NathanLively
Link to Devin session: https://app.devin.ai/sessions/9ed14440a4034ff188dbec857983c652
Requested by: @steventohme