Skip to content

fix(providers): honor target_model on the custom-provider resolve path - #93094

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-93092
Closed

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-93092

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Makes resolve_runtime_provider() honor a caller's explicit target_model on the named-custom-provider path, closing a precedence gap against every other branch in the same function.

Today the custom branch (_resolve_named_custom_runtime) fills the returned runtime's model key from the provider-level custom_providers.<name>.model default — on both the credential-pool and non-pool paths — and resolve_runtime_provider() returns it as-is, never consulting target_model. Every other branch already applies target_model or model_cfg.get("default") precedence (generic explicit path, Nous, opencode, Azure Foundry), so the custom branch is the one place where a per-call model silently loses to the provider default.

Concrete impact (#93092): an auxiliary.background_review task configured with provider: custom:<name> + model: <X> resolves to the provider's default model instead of X. _resolve_review_runtime() then marks the fork routed=True and replays a compact digest instead of full history — so the review runs on the parent's model with digest quality loss, paying the routed-path cost for zero benefit, with no warning anywhere.

The fix applies the same precedence at the custom branch return: when target_model is set, it overrides the runtime's model. A target_model of None keeps today's behavior exactly.

Related Issue

Fixes #93092

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/runtime_provider.py — in resolve_runtime_provider()'s named-custom branch, set custom_runtime["model"] = target_model before returning when target_model is provided, mirroring the target_model or default precedence used by the other branches. Single point covers all of _resolve_named_custom_runtime()'s internal return paths (bare-custom direct alias, credential pool, plain custom entry).
  • tests/hermes_cli/test_runtime_provider_resolution.py — three regression tests: target_model outranks the provider default; the provider default stands when no target_model is passed; target_model lands in the runtime even when the provider declares no top-level model.

How to Test

pytest tests/hermes_cli/test_runtime_provider_resolution.py -q — should pass (64 passed), including the three new tests.

  • test_target_model_outranks_custom_provider_default_model: provider entry with model: hermes, resolved with target_model="Curador"rt["model"] == "Curador". Observed result: on unpatched main this fails (rt["model"] == "hermes"); with this PR it passes.
  • test_custom_provider_default_model_stands_without_target_model: same provider entry, no target_model → rt["model"] == "hermes" (today's behavior preserved; passes on main too by design).
  • test_target_model_sets_model_when_provider_has_no_default: provider entry without a top-level model, resolved with target_model="Curador"rt["model"] == "Curador". Observed result: on unpatched main the key is absent; with this PR it carries the caller's intent.

Checklist

  • Code follows the project's style guidelines
  • Self-review completed
  • Comments added for complex logic (precedence rationale in the branch comment)
  • Tests added that prove the fix works
  • New and existing unit tests pass locally (64 passed in the resolution suite)
  • Platform: macOS (logic is platform-independent provider resolution)

resolve_runtime_provider()'s named-custom branch never consulted
target_model: _resolve_named_custom_runtime() fills the runtime's
"model" from the provider-level custom_providers.<name>.model default
(both the pool and non-pool paths), and the branch returned it as-is.
Every other branch already applies "target_model or model_cfg default"
precedence, so a caller's explicit per-task model — e.g.
auxiliary.background_review's model: — silently lost to the provider
default, and downstream (_resolve_review_runtime) marked the fork
routed=True and replayed a digest while actually running the parent's
model.

Apply the same precedence at the custom branch return: when target_model
is set it overrides the runtime's model. No target_model keeps today's
behavior exactly.

Fixes NousResearch#93092
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades duplicate This issue or pull request already exists labels Aug 23, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Duplicate of #78573: both repair named custom-provider target_model precedence over the provider default, including pooled and direct resolution. #78573 is the earlier open implementation with regression coverage.

@liuhao1024

Copy link
Copy Markdown
Contributor Author

The Python tests / e2e failure here is a known main flake, not caused by this change:

@teknium1

Copy link
Copy Markdown
Collaborator

Closing as duplicate with credit — @web-wyf's #78573 implemented the same fix three weeks earlier and was salvaged via #93356. Your test coverage confirmed the same root cause independently; the landed version covers both pool and direct paths. Thanks.

@teknium1 teknium1 closed this Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

3 participants