fix(llm): fail fast when routed providers lack their own API key - #499

No actionable comments were generated in the recent review. 🎉

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Linked Issues check		Issue #498 requires every routed provider to fail before client construction when its own key is empty and no non-empty explicit api_key is supplied. The registry-driven routed providers meet this r…	Remove the placeholder exception for custom-openai and custom-anthropic. Raise ValueError when CUSTOM_OPENAI_API_KEY or CUSTOM_ANTHROPIC_API_KEY is empty and no non-empty explicit api_key is supplied. Update the custom-provider …
Docstring Coverage		Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: routed providers now fail fast when their provider-specific API key is missing.
Out of Scope Changes check		The routed-key tests cover provider-key failures, explicit keys, vendor-key isolation, and native-provider regression behavior. The fallback regression coverage supports the objective because routed-k…

Full details: Out of Scope Changes check

Explanation

The routed-key tests cover provider-key failures, explicit keys, vendor-key isolation, and native-provider regression behavior. The fallback regression coverage supports the objective because routed-key failures occur during fallback model construction and must remain visible as construction errors. Test environment updates isolate the new behavior. No unrelated change is demonstrated.

Full details: Linked Issues check

Explanation

Issue #498 requires every routed provider to fail before client construction when its own key is empty and no non-empty explicit api_key is supplied. The registry-driven routed providers meet this requirement, and native providers remain outside the check. However, custom-openai and custom-anthropic pass allow_placeholder=True, receive "not-needed", and only emit a warning. This leaves the routed fallback risk unresolved for those providers.

Resolution

Remove the placeholder exception for custom-openai and custom-anthropic. Raise ValueError when CUSTOM_OPENAI_API_KEY or CUSTOM_ANTHROPIC_API_KEY is empty and no non-empty explicit api_key is supplied. Update the custom-provider tests to verify the error. Preserve native-provider behavior.

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.

_{Comment @coderabbitai help to get the list of available commands.}

已处理 CodeRabbit review 的全部意见：

1. _require_routed_api_key 拒绝 None / 空字符串显式 key

• if "api_key" in kwargs: → if kwargs.get("api_key"):
• 新增 4 个回归测试：minimax + novita 分别覆盖 api_key=None 和 api_key=""

2. 测试环境变量改用 monkeypatch

• 所有 os.environ[...] = ... → monkeypatch.setenv(...)
• 每个测试函数接受 monkeypatch fixture，测试后环境自动恢复
• 移除未使用的 import os

3. Docstring coverage

• 为 _require_routed_api_key、_env_flag_enabled、_env_flag_disabled 补全 Args/Raises docstring
• 所有 18 个测试函数均有 docstring

测试从 14 个增加到 18 个，全部 mock init_chat_model 无网络调用。

Addressing review feedback (commit f1fa9d8)

Thank you for the detailed review! Here is what I changed:

1. Removed unused routed_via parameter (models.py)

• Dropped routed_via from _require_routed_api_key() signature, docstring, and both call sites.

2. Env var read once (models.py)

• _require_routed_api_key() now reads os.environ.get(api_key_env, "") once and returns the resolved key. Both OpenAI and Anthropic call sites use the return value instead of re-reading the env var.

3. Docstring synced with code (models.py)

• Updated the custom-* docstring to match actual behavior: the "not-needed" placeholder covers keyless local servers (vLLM / LM Studio); gateways requiring authentication should set CUSTOM_*_API_KEY explicitly.
• Moved the developer-facing "security bug" note out of the user-facing docstring into a code comment.

4. Test coverage gaps (tests/test_models_routed_key.py)

• "With own key" tests now also set the vendor key (ANTHROPIC_API_KEY / OPENAI_API_KEY) and assert it is not used — pinning the actual fix.
• Added @pytest.mark.parametrize over all routed providers (minimax, kimi-coding, novita, siliconflow, moonshot) for the missing-key-raises case.
• api_key=None / api_key="" cases were already covered in the previous round.

5. CodeRabbit: fallback construction error attribution (model_fallback.py)

• Both sync and async fallback loops now track last_was_construction_error. When get_chat_model() raises during construction (fb_request stays equal to the primary request), the original ValueError is preserved on exhaustion instead of being wrapped as the primary model's ProviderStreamError.
• Added regression test test_fallback_construction_error_not_attributed_to_primary.

6. CodeRabbit: custom gateway migration path (models.py)

• The docstring now explicitly directs gateway users to configure CUSTOM_*_API_KEY with their intended key. The placeholder "not-needed" remains for keyless local servers only.

All three files pass py_compile.

Round-2 fixes + rebased onto latest main (commit d9b617e)

Rebased the PR onto the current main so it is now a single clean commit. Round-2 feedback addressed:

1. Parametrize over all 16 routed providers (test_models_routed_key.py)

• Cases are now built from the registry per your suggestion:

  _ROUTED_CASES = [
      (provider, api_key_env, vendor_key_env)
      for table, vendor_key_env in (
          (_OPENAI_ROUTED_PROVIDERS, "OPENAI_API_KEY"),
          (_ANTHROPIC_ROUTED_PROVIDERS, "ANTHROPIC_API_KEY"),
      )
      for provider, (_, api_key_env) in table.items()
      if not provider.startswith("custom-")
  ]

  This covers all 16 (including zhipu-code, volcengine-code, dashscope-code, zhipu, volcengine, dashscope, atlascloud, requesty, opper, xiaomi, xiaomi-token-plan) and any providers added later. The per-provider copies were removed.

2. Fallback regression test now actually fails on main (test_models_routed_key.py)

• Replaced the MagicMock primary model with a real ChatAnthropic(model="claude-sonnet-4-6", api_key="sk-test"), so _normalize recognizes it. This test fails with main's model_fallback and passes with the fix.
• Parametrized over ["sync", "async"], so both loops are covered.
• Invoke stubs raise AssertionError since invoke must never run.

3. Log the gateway migration, not just docstring (models.py)

• When a custom-* provider uses the placeholder, a UserWarning is emitted once per provider (via a _PLACEHOLDER_WARNED set), naming the config key:

  No custom_openai_api_key is set; sending a placeholder key. Set it if your endpoint requires authentication.

4. nit: simplify the construction-error flag (model_fallback.py)

• Dropped the reset after request.override(...); it is now set once in the except as last_was_construction_error = fb_request is request in both loops.

5. nit: simplify call sites (models.py)

• Call sites are now kwargs["api_key"] = _require_routed_api_key(...); the helper no longer writes "not-needed" into kwargs itself.

6. nit: stale comment — removed/rewritten.

CI fixes

• ruff format: fixed the multi-line calls in test_model_fallback.py (now part of the rebased file) and the new test file.
• pytest: test_blank_base_url_env_falls_back_to_default now sets the routed provider's own key (looked up from the registry), isolating it from the missing-key check; both previously failing cases pass.

Round-3 test-file fixes (commit 3c27750, single commit)

Thanks for the careful review. All four points are addressed:

1. Restored the flaky-test fix (tests/test_llm.py)

• test_kimi_coding_declares_thinking now takes monkeypatch and sets KIMI_API_KEY explicitly:

  def test_kimi_coding_declares_thinking(self, mock_init, monkeypatch):
      mock_init.return_value = "mock_model"
      monkeypatch.setenv("KIMI_API_KEY", "test-key")
      get_chat_model("kimi-for-coding", provider="kimi-coding")

  It no longer relies on keys leaked by test_cli_tui_dispatch.py / test_config.py, so it passes under -k, pytest tests/test_llm.py, or any reordering.

2. Restored the env-clearing fixture (tests/test_models_routed_key.py)

• Added back the autouse fixture so every test starts with no provider keys or base URLs:

  @pytest.fixture(autouse=True)
  def _clear_provider_env(monkeypatch):
      for _, api_key_env in (*_OPENAI_ROUTED_PROVIDERS.values(),
                            *_ANTHROPIC_ROUTED_PROVIDERS.values()):
          monkeypatch.delenv(api_key_env, raising=False)
      for name in ("OPENAI_API_KEY", "OPENAI_BASE_URL",
                   "ANTHROPIC_API_KEY", "ANTHROPIC_BASE_URL"):
          monkeypatch.delenv(name, raising=False)

3. Placeholder warning tested for a second, non-warning build (tests/test_models_routed_key.py)

• The test now builds twice: the first get_chat_model is wrapped in pytest.warns(...); the second is wrapped in catch_warnings() + simplefilter("error", UserWarning) and must not raise, confirming the warning fires once only.

4. Moved the fallback test to the correct file (tests/test_model_fallback.py)

• Removed test_fallback_construction_error_keeps_original_exception (and its helpers) from test_models_routed_key.py and added it next to TestFallbackChainConstructionError, keeping the real ChatAnthropic primary model and sync/async parametrization.

ruff

• Both CI failures (PT031 / PT012: warns/raises block should contain a single simple statement) are fixed. The moved fallback test now extracts the sync/async branch into a _run() helper so the pytest.raises block contains only _run().

Note: I couldn't run ruff locally (package index unreachable from this machine), so the ruff fixes are based on the CI error output; the rest of the changes are verified with py_compile.

Fixed (commit fa951b5): the get_chat_model("minimax-m3", provider="minimax", api_key=explicit) call is now on a single line. Also moved import warnings to the top of the file. Thanks for the review!

Good catch. Replaced the cache-clearing with monkeypatch.setattr(models, "_PLACEHOLDER_WARNED", set()) (commit 03f616d), so the test starts from an empty cache and monkeypatch restores the original cache afterward, preserving the once-per-provider state for other providers.

Thanks @din0s and @X-iZhang for the thorough reviews! I pushed a small follow-up (d625a20) covering the last two open notes:

• Added test_explicit_key_used_without_env, which asserts a routed provider configured with an explicit API key works without any vendor env var set.
• Expanded custom placeholder coverage to both custom providers: test_custom_placeholder_warns_once is now parametrized over custom-openai and custom-anthropic.
• Also fixed the trailing-newline failures (ruff W292) that the Lint workflow flagged on five files.

Local checks: ruff check . and ruff format --check pass, and the routed-key (24), model-fallback (75), and llm (275 passed, 2 skipped) test suites are green.

Thanks for these last revisions, the fixes look right.

One small problem is that both commits converted the files to Windows line endings (CRLF), so GitHub shows every line as changed (1300 lines in test_model_fallback.py, 201 in test_models_routed_key.py) for a 14-line edit. Once squashed into main, that hides the real change and resets git blame on all of test_model_fallback.py. Every other file in the repo uses LF.

Could you convert both back to LF and push? For example:

git config core.autocrlf input
sed -i 's/\r$//' tests/test_model_fallback.py tests/test_models_routed_key.py
git commit -am "test(llm): restore LF line endings"

git diff --ignore-cr-at-eol main should then show only your comment and model-id edits.

Good catch — that was my mistake. The Windows checkout had core.autocrlf enabled, so when I pushed the revised blobs the working-tree bytes were CRLF.

I converted both files back to LF and pushed a single commit (test(llm): restore LF line endings), and set core.autocrlf=input locally.

Verified:

• git diff --ignore-cr-at-eol main now shows only the real changes — the updated docstrings in test_model_fallback.py and the vendor/some-model id in test_models_routed_key.py; both files contain no CR bytes and the PR net diff is clean (+120/-0 for test_model_fallback.py, +201/-0 for the new test_models_routed_key.py), with no whole-file rewrites.
• The affected tests still pass (99 passed across the two files).