Skip to content

test(model-switch): assert fetch_api_models kwargs match current signature (#15243) - #15246

Closed
briandevans wants to merge 1 commit into
NousResearch:mainfrom
briandevans:fix/test-custom-provider-api-mode-drift
Closed

test(model-switch): assert fetch_api_models kwargs match current signature (#15243)#15246
briandevans wants to merge 1 commit into
NousResearch:mainfrom
briandevans:fix/test-custom-provider-api-mode-drift

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes `#15243`.

`_model_flow_named_custom` was extended in `647900e8` ("fix(cli): support model validation for anthropic_messages and cloudflare-protected endpoints") to pass `api_mode` through to `fetch_api_models` so Anthropic-messages and Cloudflare-protected endpoints validate correctly. The test that pins "we must still probe the endpoint when a model is already saved" was never updated for that new kwarg and has been failing on every PR's `test` CI lane since — it's one of the four standing baseline failures I've been calling out in CI notes on every open PR (#15158, #15162, #15173, #15185 at minimum, plus any PR a reporter opens in this window).

Fix

One line, plus a comment pointing future readers at the history so no one strips the kwarg again.

Before:
```python
mock_fetch.assert_called_once_with("sk-test", "https://vllm.example.com/v1\", timeout=8.0)
```

After:
```python

fetch_api_models MUST be called even though model was saved.

api_mode was added to the production signature by 647900e

("fix(cli): support model validation for anthropic_messages and

cloudflare-protected endpoints") — the test fixture has no

api_mode entry so api_mode or None resolves to None.

mock_fetch.assert_called_once_with(
"sk-test", "https://vllm.example.com/v1\", timeout=8.0, api_mode=None,
)
```

The production call at `hermes_cli/main.py:2920-2923` is:

```python
models = fetch_api_models(
api_key, base_url, timeout=8.0,
api_mode=api_mode or None,
)
```

where `api_mode` comes from `provider_info.get("api_mode", "")`. The test fixture has no `api_mode` entry so `api_mode or None` resolves to `None` — the expected assertion kwarg.

Related Issue

Fixes #15243

Type of Change

  • 🔧 Test fix (no production behavior change)

Test plan

  • All 6 tests in `TestCustomProviderModelSwitch` pass locally
  • This failure has appeared on every open PR's `test` CI lane since 647900e landed — the fix clears it from the baseline failure set
  • No production code touched

…ature (NousResearch#15243)

``_model_flow_named_custom`` was extended in ``647900e8`` to pass
``api_mode`` through to ``fetch_api_models`` so anthropic-messages and
Cloudflare-protected endpoints validate correctly.  The test that
pins "we must still probe the endpoint when a model is already saved"
kept asserting the pre-647900e8 kwarg shape and has been failing on
every PR's CI since — one of the four standing baseline failures I've
been flagging on open PRs (NousResearch#15158, NousResearch#15162, NousResearch#15173, NousResearch#15185, NousResearch#15244).

The production call is:

    fetch_api_models(api_key, base_url, timeout=8.0, api_mode=api_mode or None)

where ``api_mode`` comes from ``provider_info.get("api_mode", "")``.
The test fixture has no ``api_mode`` entry so ``api_mode or None``
resolves to ``None``.

One-line fix — add ``api_mode=None`` to the ``assert_called_once_with``.
Added an inline comment pointing at the 647900e history so future
contributors immediately see why the kwarg exists and don't
accidentally strip it.

All 6 tests in the class still pass locally.  This should clear the
corresponding entry from the baseline failure set on every open PR's
``test`` lane.

Closes NousResearch#15243

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 24, 2026 16:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates a CLI regression test to match the current _model_flow_named_custom()fetch_api_models() call signature, fixing a long-standing CI failure after api_mode was added to the production call path.

Changes:

  • Update the mocked fetch_api_models assertion to include the api_mode=None kwarg (matching production behavior when provider_info has no api_mode).
  • Add an inline comment documenting why api_mode is expected to be None in this fixture.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard labels Apr 24, 2026
@briandevans

Copy link
Copy Markdown
Contributor Author

Closing — this commit already landed on `main` via 5fdd9f6d3 (cherry-picked with authorship preserved). Verified locally: `tests/hermes_cli/test_custom_provider_model_switch.py` passes 11/11 against current `origin/main` (`810d98e89`) — the `api_mode=None` assertion is in place at line 59 and the standing baseline failure on the `test` CI lane is resolved.

No follow-up needed. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: test_custom_provider_model_switch asserts outdated fetch_api_models signature

3 participants