fix(auxiliary): resolve key_env in _resolve_task_provider_model (#66641) - #66655
fix(auxiliary): resolve key_env in _resolve_task_provider_model (#66641)#66655webtecnica wants to merge 1 commit into
Conversation
…Research#66641) _resolve_task_provider_model() read api_key from the auxiliary task config but never consulted key_env (or api_key_env). When a user configured an auxiliary task with key_env instead of a plaintext api_key, the resolved API key was None, causing 401 on every call. Add the same key_env → os.getenv() resolution pattern already used in _fallback_entry_api_key() and named custom provider resolution. Closes NousResearch#66641
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Fixes key_env/api_key_env resolution in _resolve_task_provider_model() to match the existing pattern in _fallback_entry_api_key() and named custom provider resolution. Clean 7-line fix with no behavioral changes to paths that already use direct api_key.
LGTM.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused auxiliary credential-resolution fix. Current main still reads only auxiliary.<task>.api_key in _resolve_task_provider_model() (agent/auxiliary_client.py:6251-6256) and returns that value to provider resolution (agent/auxiliary_client.py:6331-6344), so the premise is verified.
Problems
- This production change has no regression coverage.
tests/agent/test_auxiliary_client.py:113-276exercises configured endpoint propagation but does not cover task-levelkey_envorapi_key_envresolution.
Suggested changes
- Add focused resolver tests for both aliases, direct
api_keyprecedence, and an unset referenced variable. The member-linked related PR #36080 already contains test coverage for these exact cases and can be used when salvaging this narrow production fix.
Automated hermes-sweeper review.
| cfg_model = str(task_config.get("model", "")).strip() or None | ||
| cfg_base_url = str(task_config.get("base_url", "")).strip() or None | ||
| cfg_api_key = str(task_config.get("api_key", "")).strip() or None | ||
| # Resolve key_env → env var when api_key is not set directly |
There was a problem hiding this comment.
Please add a focused regression test for this branch: task-level key_env and api_key_env resolution, direct api_key precedence, and an unset referenced variable. The existing resolver tests do not cover these aliases.
|
Salvaged into #67192 with authorship preserved, rebased on current |
Summary
_resolve_task_provider_model()readapi_keyfrom the auxiliary task config but never consultedkey_env(orapi_key_env). When a user configured an auxiliary task withkey_envinstead of a plaintextapi_key, the resolved API key wasNone, causing 401 on every call.Root cause:
cfg_api_keywas only read fromtask_config.get("api_key", ""). Thekey_env→os.getenv()resolution pattern already existed in two other places in the same file (_fallback_entry_api_key()and named custom provider resolution) but was missing from the top-level_resolve_task_provider_model().Change
key_env/api_key_env→os.getenv()fallback in_resolve_task_provider_model()right after readingcfg_api_key, matching the existing pattern used elsewhere in the file.Verification
python3 -m py_compile agent/auxiliary_client.pypasses cleanlykey_env/api_key_envfallback pattern already proven in_fallback_entry_api_key()(L4291) and named custom provider resolution (L5047)Closes #66641
/cc @hanjunwp