fix(auth): surface pool base_url and log errors in _resolve_api_key_p… - #42386
fix(auth): surface pool base_url and log errors in _resolve_api_key_p…#42386D7y1 wants to merge 1 commit into
Conversation
…rovider_secret When a provider credential is stored via `hermes auth add` (e.g. DeepSeek), the runtime resolver found the key in the credential pool but silently discarded the pool entry's base_url and swallowed all exceptions with a bare `except Exception: pass`. Two bugs fixed in hermes_cli/auth.py: 1. `_resolve_api_key_provider_secret`: replace `except Exception: pass` with `logger.debug(..., exc_info=True)` so load_pool() failures are visible in debug logs instead of silently looking like no credentials. 2. `resolve_api_key_provider_credentials`: when the key came from the credential pool, do a focused lookup for the pool entry's runtime_base_url and prefer it over pconfig.inference_base_url. Fixes providers added against a non-default endpoint. The `_resolve_api_key_provider_secret` return type is unchanged -- the base_url concern stays in the one function that assembles the full credential dict, not scattered across all callers. Integration tests in test_deepseek_auth_pool_resolution.py write a real auth.json pool entry (exactly as `hermes auth add deepseek` does) and exercise the full resolution chain without mocking load_pool. Closes NousResearch#42269
cb69df3 to
54c7d25
Compare
|
Positive review — credential pool base_url propagation looks solid. The fix addresses two real issues: (1) silent exception swallowing in Observations:
No issues found. |
|
This PR surfaces What this PR does: makes How #62467 extends this:
If this PR merges first, #62467 will rebase cleanly (the unified resolver delegates through the same path). The logging improvements here would complement the resolver's source tracking nicely. |
|
Thanks for the focused auth investigation. The reported runtime path has moved since this PR was opened: current main selects the pool entry in Problems
Suggested changes
Automated hermes-sweeper review. |
What does this PR do?
hermes auth add deepseek --type api-keycorrectly stores the credential to~/.hermes/auth.json→credential_pool.deepseek, but at runtimeresolve_api_key_provider_credentials()silently returns an emptyapi_key,causing every inference request to fail with:
Two root causes fixed:
1. Silent exception swallowing —
_resolve_api_key_provider_secret()wrappedthe entire credential pool lookup in
except Exception: pass. Any failure inload_pool()(import error, I/O error, lock contention) was invisible: thefunction fell through to
return "", ""as if no credentials existed, with nolog entry to debug from.
2. pool
base_urlsilently dropped — when the key was successfully readfrom the pool, the pool entry's
base_url(written byhermes auth add) wasdiscarded.
resolve_api_key_provider_credentials()always fell back topconfig.inference_base_url, ignoring the URL stored alongside the key. Thismatters for providers added against a non-default endpoint (e.g. a
DeepSeek-compatible server at a custom URL).
Related Issue
Fixes #42269
Type of Change
Changes Made
hermes_cli/auth.py— two targeted edits, no interface changes (+33 / -1):_resolve_api_key_provider_secret():Replace
except Exception: passwithlogger.debug(..., exc_info=True)soload_pool()failures are visible inhermes --debugoutput instead ofsilently appearing as "no credentials found".
resolve_api_key_provider_credentials():When
key_sourceindicates the key came from the credential pool, do afocused lookup for the pool entry's
runtime_base_urland prefer it overpconfig.inference_base_url. The_resolve_api_key_provider_secretreturntype is unchanged — the
base_urlconcern stays in the one functionthat assembles the full credential dict, not propagated to all callers.
tests/hermes_cli/test_deepseek_auth_pool_resolution.py(new):5 integration tests that write a real
auth.jsonpool entry (exactly ashermes auth add deepseekdoes) and callresolve_api_key_provider_credentials()end-to-end without mocking
load_pool.How to Test
Automated (recommended):
All 5 tests should pass.
test_resolve_api_key_credentials_finds_pool_keydirectly reproduces the bug: it seeds a pool entry exactly as
hermes auth addwould, then calls
resolve_api_key_provider_credentials("deepseek")and assertsthe key is returned — this test fails on main and passes with this fix.
Manual:
DEEPSEEK_API_KEYis not set in your environmenthermes auth add deepseek --type api-key→ paste a real or fake keyhermes model→ select DeepSeekhermes chat -q "hello"→ should send the request (previously raised:Provider 'deepseek' is set in config.yaml but no API key was found)hermes --debug chat -q "hello"→ if the key is invalid you will see a 401 from DeepSeek instead of the "no API key" error, confirming the pool is being readChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
Before fix — pool credential ignored, error raised:
After fix — integration test output confirming end-to-end resolution works: