fix(gateway): expand config env templates before auth bridging and model resolution - #35444
Conversation
…nfiguration loading
|
Same class of bug as #21275 and merged #30842 — gateway paths reading raw YAML without |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Overall
Solid fix for a subtle but impactful config expansion bug. The gateway was reading raw ${VAR} templates in some code paths while other paths had expansion applied — causing runtime mismatches that are hard to debug.
What's Good
- Root cause identification: The gap between
config.yaml→ expanded values in some paths vs. raw templates in others is well-documented - Comprehensive fix: Covers both the
load_gateway_config()path and the_resolve_gateway_model()direct-dict path - Regression tests: Both test cases directly verify the broken scenarios
- Clean blast radius: LOW — only adds env var expansion where it was missing
- Good test isolation: Uses
tmp_path/monkeypatchfixtures, no global state contamination
Suggestion (Non-blocking)
Consider adding a docstring to _expand_env_vars in hermes_cli/config.py (if not already present) noting it's also called from gateway/config.py, so future refactors don't accidentally break the gateway path.
Reviewed by Hermes Agent
|
Thanks for the focused regression coverage. The premise remains valid on current Automated hermes-sweeper review. |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address or reference this gateway configuration complex. #35444 directly fixes the reported raw-YAML environment-template gap in model resolution and Telegram auth bridging, while #46181 uses expanded runtime configuration as part of a much broader prompt-refresh change that does not fix those two missing paths.
Related pull requests
- #35444
related— (+37/-1) — merge: The focused diff expands config.yaml before GatewayConfig mapping/auth bridging and routes model resolution through expanded runtime config, with direct regression tests for${GW_MODEL}and${TG_ALLOWED_USER}; the contributor keep_open review confirms both raw-YAML paths remain unfixed on current main. - #46181 [closed]
related— (+210/-11) — keep closed, not a duplicate: This remains relevant as a contrasting broader prompt-policy implementation built around runtime config loading, but it neither adds #35444's load_gateway_config expansion nor its auth-bridging fix. Despite the keep_open review on #46181, its own blocking contributor analysis identifies per-turn prompt replacement/cache rebuilding as violating the byte-stable per-conversation prompt invariant, activates a CLI-only display.personality setting in the gateway, and notes incomplete case-insensitive lookup; the later closure therefore matches the documented design constraints.
Suggested consolidation
Merge #35444 as the focused fix for the two verified raw-config paths. Keep #46181 closed because its broader live prompt-refresh design conflicts with documented gateway invariants and does not substitute for #35444; no PR should be closed as a duplicate in this complex.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 20 kB of PR diffs, 4 kB of issue/PR text, 5 kB of discussion (6 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Summary
Fix gateway-side
${VAR}config expansion drift so gateway runtime and auth/env bridging behave the same way as the main config loader.Before this change, some gateway paths read raw
config.yamlvalues without expanding${ENV_VAR}templates first. That caused real mismatches such as:model: ${GW_MODEL}working in some config paths but not in gateway model resolutiontelegram.allow_from: ${TG_ALLOWED_USER}being bridged intoTELEGRAM_ALLOWED_USERSas the literal string${TG_ALLOWED_USER}instead of the resolved user IDThis PR makes gateway config handling consistent with the documented config expansion behavior.
Fix
${VAR}templates ingateway.config.load_gateway_config()immediately after loadingconfig.yaml, before YAML values are mapped into gateway config fields or bridged into env vars.gateway.run._resolve_gateway_model()to read from expanded gateway runtime config when loading from disk._resolve_gateway_model()is given a config dict directly, so raw config callers do not bypass expansion.Testing
Added regression coverage for both broken paths:
tests/gateway/test_runtime_config_env_expansion.py_resolve_gateway_model()expands${GW_MODEL}fromconfig.yamltests/gateway/test_config.pytelegram.allow_from: ${TG_ALLOWED_USER}is expanded before bridging intoTELEGRAM_ALLOWED_USERSTest Results
Passed:
tests/gateway/test_runtime_config_env_expansion.pytests/gateway/test_config.pytests/test_empty_model_fallback.pytests/gateway/test_api_server_toolset.pytests/gateway/test_api_server.pytests/gateway/test_telegram_group_gating.pyObserved results: