fix(proxy): resolve force-model provider against live availability (#874)

[NathanLively]

Summary

Fixes #874: /force-model and x-weave-force-model resolved a model's provider via catalog.Model.Providers[0] unconditionally, ignoring the deployment's actual enabled/excluded provider set. If the primary binding was excluded (e.g. makora via ROUTER_EXCLUDED_PROVIDERS), the pin silently pointed at a dead provider — the next turn's pin lookup treated it as equivalent to no pin (reason:"no_pin") and the cluster scorer silently overwrote it, with no error, no warning, and a force-model ack that falsely reported success.

Changes

• 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 — the same pattern cluster.Scorer's resolveProviderFor already uses for automatic routing — instead of hard-picking Providers[0]. A known model with no available binding now returns provider="" so callers can reject cleanly rather than pin a dead one.
• applyForceModelHeader / handleForceModelCommand thread the enabled-provider set through (computed earlier in the request path so it's available before force-model handling runs) and reject with a clear ack message + Warn log when no binding is available.
• turnloop's forced-pin fall-through now logs a Warn when a stored pin's provider became unavailable mid-session, so that drop is distinguishable in logs from ordinary no-pin automatic routing.

Testing

• TestResolveForceModel extended with availability-aware cases (primary binding excluded → falls to fallback binding; no binding available → empty provider).
• New TestRunTurnLoop_ForcedPin_FallsThroughWhenProviderUnavailable reproduces the issue's repro shape and asserts the pin is dropped rather than served, with the new Warn log confirmed in output.
• go build ./..., go vet ./..., go test ./... all pass.

Explicitly not done

Did not set ROUTER_HARD_PIN_MODEL/ROUTER_HARD_PIN_PROVIDER as "the fix" — that's a global override disabling the cluster scorer entirely for main-loop turns, discussed and rejected in the issue as a real solution.

🤖 Generated with Claude

[greptile-apps]

PR author is not in the allowed authors list.

[graphify-labs]

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 the deployment's live enabled-provider set into the /force-model resolution path. Previously resolveForceModel/resolveForceModelWithEffort unconditionally picked a model's primary binding (Providers[0]); now they accept an available map[string]struct{} and delegate to a new resolveForcedBinding helper that walks a model's bindings in catalog order to find the first available provider (with nil meaning unrestricted, preserving old behavior). The surface area covers the force-model helpers (resolveForceModel, resolveForceModelWithEffort, applyForceModelHeader, handleForceModelCommand) plus a new sortedProviderKeys logging helper, along with call sites like runAgentShadowEvaluationRoute that pass nil. It also adds a warning path when a recognized model has no available binding (routing falls back to automatic) and includes new internal tests exercising forced-model behavior against disabled/unavailable providers. Note: the changed-symbols list is much broader than the shown diff (touching pin stores, telemetry, routers, etc.), so there may be additional edits beyond the truncated portion I reviewed.

No blocking issues surfaced. 6 lower-confidence candidates did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 890 functions depend on the 292 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 — 890 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: 854 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).

[graphify-labs]

⚠️ Health regression — TestRunTurnLoop_ForcedPin_FallsThroughWhenProviderUnavailable()

fans out to 6 callees (efferent coupling).

Grounded coupling-delta finding (deterministic), not an LLM guess.

[devin-ai-integration]

Thank you so much for this, @NathanLively — this is a genuinely excellent contribution. You found a real and subtle bug (a force-model pin landing on an excluded binding, then reading back as no_pin with the ack falsely reporting success), diagnosed it precisely, and fixed it at exactly the right layer by mirroring the ordered binding walk that cluster.Scorer's resolveProviderFor already uses for automatic routing. The provider == "" vs known == false distinction so callers can reject accurately is a really nice touch, the nativeMismatch handling for explicit openai/<id> prefixes is correct, and the regression test reproduces the issue's exact shape. I verified the hoist of enabledProviders in ProxyMessages / ProxyOpenAIChatCompletion too — nothing between the old and new positions mutates the credential/header/subscription state those calls read, so the resulting set is identical. make precommit passes on your branch as-is.

To keep everything consistent with some of our internal conventions, I've opened a rewrite at #876 that keeps your production logic byte-for-byte and only adjusts three small things. Please don't read this as anything you did wrong — these are internal, still-evolving conventions that live in our AGENTS.md/CLAUDE.md files and are honestly not discoverable from the outside. The substance of the fix is entirely yours, and you're credited as co-author on the commit.

What changed and why:

1. Reused an existing helper instead of adding a new one. internal/proxy/turnloop.go already has sortedEnabledKeys(map[string]struct{}) []string, which is what the new sortedProviderKeys did — same package, same signature. Root AGENTS.md → "Adding a new helper": "Don't, unless same logic needed in 3+ places and no plausible existing home", plus the DRY engineering principle.

2. Corrected one godoc. resolveForceModelWithEffort's doc said the provider is resolved "via catalog.ResolveBinding" — that function does exist and does the same walk, but the code actually calls the new local resolveForcedBinding (which it has to, because of the nativeMismatch distinction that ResolveBinding can't express). Just made the doc match the code.

3. Shortened the added comments. Root AGENTS.md → Engineering principles: "Concise comments, sparingly — default to none. Only when why is non-obvious… Never rehash code, never reference current task/PR/caller, no multi-paragraph." The new godocs on resolveForcedBinding, resolveForceModelWithEffort, and applyForceModelHeader ran 10–12 lines each; for calibration, unexported-function godocs in internal/proxy/turnloop.go top out at 7 lines and are mostly 0–4. I cut each to ≤5 lines, single paragraph, keeping the two genuinely non-obvious invariants (nil available = unrestricted; nativeMismatch = "no binding under that provider at all" vs "exists but unavailable") and dropping the before/after narrative. There's also an advisory CI job (comment_length_review.yml) that flags added comment blocks of 3+ lines that could be shortened, so this keeps it quiet. Note that bare #874 issue references are fine here — that is our local convention; it's the surrounding prose that we keep short.

None of that touches your logic, your control flow, or your assertions. Really appreciate the care that went into this one — including the "Explicitly not done" note about ROUTER_HARD_PIN_MODEL, which is exactly the right call. Hope you'll send more our way.