Skip to content

fix(cli): preserve current credentials on same-provider model switch - #44502

Open
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/model-switch-same-provider-empty-apikey
Open

liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/model-switch-same-provider-empty-apikey

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Preserves current working credentials when resolve_runtime_provider() returns empty api_key/base_url during a same-provider model switch. This fixes 401 errors when switching models on providers like opencode-go where the resolver does not re-resolve env-var credentials for the same provider.

Related Issue

Fixes #44490

Type of Change

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

Changes Made

  • hermes_cli/model_switch.py: In the same-provider (provider_changed=False) branch of switch_model(), fallback to current_api_key/current_base_url when resolve_runtime_provider() returns empty values. Previously the code unconditionally overwrote credentials with empty strings.
  • tests/hermes_cli/test_model_switch_same_provider_credential.py: New test file with 4 regression tests covering empty key fallback, empty base URL fallback, non-empty resolved values preferred, and both-empty fallback.

How to Test

  1. Configure opencode-go provider with a working API key
  2. Use /model kimi-k2.5 to switch from mimo-v2.5 (same provider)
  3. Verify the switch succeeds without 401 error
  4. Run tests: pytest tests/hermes_cli/test_model_switch_same_provider_credential.py -v

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

  • Analyzed: switch_model() same-provider branch (callers: 4, flows: CLI/gateway/TUI model switch)
  • Blast radius: LOW — only affects same-provider switch path; provider-changed path and exception path unchanged
  • Related patterns: matches existing exception-path behavior (lines 968-969 except: pass already preserves current credentials)

When resolve_runtime_provider() returns an empty api_key or base_url
during a same-provider switch (e.g. opencode-go mimo-v2.5 → kimi-k2.5),
the code unconditionally overwrites the current working credentials
with empty strings, causing 401 errors.

Fallback to current_api_key / current_base_url when the resolved
value is empty, matching the existing exception-path behavior.

Fixes NousResearch#44490
@AIalliAI

Copy link
Copy Markdown
Contributor

Verified this against main (2ee69d0):

The fix is correct and the tests are genuine.

  • Ran the new test file on the PR head: 4/4 pass. As a negative control I reverted hermes_cli/model_switch.py to main while keeping the tests: the 3 fallback tests fail on main (assert '' == '***'), confirming they exercise the same-provider branch with the mocked resolution (the function-level imports in switch_model make the hermes_cli.runtime_provider / hermes_cli.models patch targets correct).
  • Full -k model_switch selection: 113 passed, no regressions.
  • The change also brings the code in line with the comment directly above it ("…keep existing credentials"), which the code previously didn't do — only the except path preserved them — and mirrors the user-provider branch a few lines up, which already does runtime.get(...) or fallback.

Two notes on the surrounding analysis:

  1. The root-cause wording in [Bug]: Same-provider model switch on opencode-go produces empty api_key → 401 #44490 ("resolve_runtime_provider() does not re-resolve env-var credentials for the same provider") isn't quite right: resolve_api_key_provider_credentials()_resolve_api_key_provider_secret() does check OPENCODE_GO_API_KEY (os.environ and ~/.hermes/.env) and then the credential pool. The empty key arises when the running session's working key came from a source that chain doesn't consult (e.g. model.api_key in config.yaml, or explicit creds at session start). That doesn't change the fix — silently clobbering known-working credentials with "" is wrong regardless of why resolution came back empty.
  2. The reporter said a manual patch equivalent to this didn't resolve it for them — note their environment also had the gateway override fallback in play (gateway/run.py already falls through to env-based resolution when an override has no api_key, which only helps if env resolution finds a key) and their manual patches were overwritten by hermes update. With this fix the override stored in _session_model_overrides is never empty in the first place, which covers the non-gateway consumers too.

LGTM.

@AIalliAI

Copy link
Copy Markdown
Contributor

CI note: the red test (2) on this PR is not caused by this change — it's the known checkout-reset flake tracked in #43298.

Fingerprint from the failing run (slice 2/6):

  • 22:09:22tests/gateway/test_update_command.py passes (35✓). The TestUpdateCommandPlatformGate::test_allows_* tests in that file spawn a real detached hermes update, which on CI's shallow merge-ref checkout falls back to git reset --hard origin/main.
  • 22:09:32 — ten seconds later, this PR's newly added tests/hermes_cli/test_model_switch_same_provider_credential.py fails with ERROR: file or directory not found (the reset wiped PR-added files; exit-4 forensics block confirms the file no longer exists on disk).

All other slices are green, and the added tests passed 4/4 when I ran them on the PR head locally (see my review comment above). A re-run of the failed job should go green unless the update tests land on the same slice again; the real fix is #43298.

…o fix CI shard

The standalone test file test_model_switch_same_provider_credential.py was
not found by the CI sharded runner (exit code 4 on shard 2). Move the 4
tests into the existing test_model_switch_custom_providers.py which already
has the required imports and mock fixtures.
@AIalliAI

Copy link
Copy Markdown
Contributor

Also checked the follow-up commit 72f8ac8: it's a pure test relocation — the four #44490 regression tests move from the standalone file into tests/hermes_cli/test_model_switch_custom_providers.py with bodies unchanged, reusing that file's existing _MOCK_VALIDATION. Production code untouched. Ran the merged file locally: 27/27 pass (23 pre-existing + 4 relocated). Still LGTM.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have labels Jun 11, 2026
AIalliAI added a commit to AIalliAI/Hermes that referenced this pull request Jun 14, 2026
NousResearch#44502 follow-up)

Mirrors upstream PR NousResearch#44502 commit 72f8ac8: relocates the four issue
NousResearch#44490 regression tests from the standalone file into
test_model_switch_custom_providers.py. Test bodies unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused fix. The premise remains true on current main: hermes_cli/model_switch.py:1232-1243 enters the same-provider branch and unconditionally replaces initialized current credentials with empty values returned by resolve_runtime_provider().

Suggested changes

  • GitHub currently marks this PR DIRTY; when maintainers salvage it, retain the four regression cases alongside the production fallback. The current test file no longer contains those named same-provider empty-resolution cases.

The fallback itself is appropriately narrow: it preserves empty-resolution credentials while continuing to prefer non-empty resolver output. This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
fangliquanflq added a commit to fangliquanflq/hermes-agent that referenced this pull request Aug 1, 2026
Pass live session URL/key as explicit_* for bare custom/local same-provider
/model switches so resolve cannot fall through to OpenRouter. Drop the
overlapping empty-result current_* refill (owned by NousResearch#44502). When a direct
alias changes the host, clear the prior session key before validation so
endpoint A's credential is never sent to host B.
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 P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Same-provider model switch on opencode-go produces empty api_key → 401

4 participants