fix(gateway): honor nested gateway.* form for 9 more top-level keys - #59779
fix(gateway): honor nested gateway.* form for 9 more top-level keys#59779pierrenode wants to merge 1 commit into
Conversation
load_gateway_config() already accepted both the top-level key and the nested gateway.<key> form (written by `hermes config set gateway.<key> ...`) for multiplex_profiles, max_concurrent_sessions, streaming, and write_sessions_json — each fixed one at a time as users hit it (most recently NousResearch#59320 for multiplex_profiles). Nine sibling top-level keys never got the same nested fallback: session_reset, quick_commands, stt, stt_echo_transcripts, group_sessions_per_user, thread_sessions_per_user, reset_triggers, always_log_local, and unauthorized_dm_behavior. `hermes config set gateway.<any-of-these> ...` builds exactly this nested shape (hermes_cli/config.py's _set_nested has no schema, so it accepts any dotted path), so a user following the same pattern that legitimately works for gateway.multiplex_profiles/gateway.streaming gets a silent no-op for these nine keys instead. Fix: read `gateway: {...}` into a single `gateway_section` variable once (consolidating three separate `yaml_cfg.get("gateway")` calls already in the function) and add the same top-level-wins/nested-fallback check for each of the nine keys, mirroring the existing write_sessions_json precedent exactly. Note: because every fallback here is guarded by `isinstance(gateway_section, dict)`, this also makes the streaming fallback tolerate a scalar `gateway:` block (e.g. `gateway: disabled`) without crashing — the same crash NousResearch#40837 (open) targets specifically for streaming. This change doesn't set out to fix that PR's issue, but the consolidated guard covers it as a side effect; flagging it for the reviewer rather than leaving it to be found in review.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the inconsistent nested gateway read path. The premise remains valid on current main: gateway/config.py:1021-1102 still bridges these settings only from top-level YAML while selected sibling settings already accept nested forms.
Problems
gateway/config.py:964does not fully implement the documented top-level-wins rule: a presentsession_reset: {}is treated as absent and is replaced bygateway.session_reset. Similarly,gateway/config.py:983replaces a present non-mapping top-levelsttvalue withgateway.stt. The other additions use key-presence precedence, so these two cases are inconsistent.
Suggested changes
- Fall back to nested
session_reset/sttonly when their top-level keys are absent, then add precedence tests for those cases. - Current main's
a7f65e3balready added the scalargateway:guard in this same region; retain that guard when resolving the salvage overlap.
Automated hermes-sweeper review.
| # Map config.yaml keys → GatewayConfig.from_dict() schema. | ||
| # Each key overwrites whatever gateway.json may have set. | ||
| sr = yaml_cfg.get("session_reset") | ||
| if not (sr and isinstance(sr, dict)) and isinstance(gateway_section, dict): |
There was a problem hiding this comment.
This falls back when session_reset is present but {}, contrary to the stated top-level-wins contract. Consider using top-level key presence to decide fallback, and add a precedence regression for an empty top-level mapping plus a populated nested value.
| ) | ||
|
|
||
| stt_cfg = yaml_cfg.get("stt") | ||
| if not isinstance(stt_cfg, dict) and isinstance(gateway_section, dict): |
There was a problem hiding this comment.
A present non-mapping top-level stt is treated as absent and replaced by nested gateway.stt. Gate the nested fallback on key absence if top-level precedence must be unconditional.
…llback Follow-up for salvaged PR #59779: the session_reset and stt fallbacks used truthiness/type checks, so a present-but-empty top-level value was silently replaced by the nested gateway.* form — inconsistent with the key-presence precedence every other key in the block uses. Switch both to 'key not in yaml_cfg' gating and add precedence regression tests.
|
Merged via PR #67982 — your commit was cherry-picked onto current main with your authorship preserved in git log (rebase-merge). We added one follow-up commit on top switching the session_reset/stt fallbacks to key-presence precedence (the two inconsistencies flagged in the sweeper review) plus precedence regression tests. Thanks for the thorough fix and the test coverage! |
…llback Follow-up for salvaged PR NousResearch#59779: the session_reset and stt fallbacks used truthiness/type checks, so a present-but-empty top-level value was silently replaced by the nested gateway.* form — inconsistent with the key-presence precedence every other key in the block uses. Switch both to 'key not in yaml_cfg' gating and add precedence regression tests.
What does this PR do?
load_gateway_config()already accepted both the top-level key and the nestedgateway.<key>form (written byhermes config set gateway.<key> ...) formultiplex_profiles,max_concurrent_sessions,streaming, andwrite_sessions_json— each fixed one at a time as users hit it (most recently #59320 formultiplex_profiles). Nine sibling top-level keys never got the same nested fallback:session_reset,quick_commands,stt,stt_echo_transcripts,group_sessions_per_user,thread_sessions_per_user,reset_triggers,always_log_local, andunauthorized_dm_behavior.hermes config set gateway.<any-of-these> ...builds exactly this nested shape (hermes_cli/config.py's_set_nestedhas no schema, so it accepts any dotted path), so a user following the same pattern that legitimately works forgateway.multiplex_profiles/gateway.streaminggets a silent no-op for these nine keys instead — the setting is written to config.yaml,hermes config showmay even look right, but the gateway never reads it.Related Issue
No filed issue — found via pattern analysis of
load_gateway_config(): 4 of 13 top-level keys it bridges already had the nested-fallback treatment, applied incrementally as separate bugs over time; the other 9 didn't. General class acknowledged in #34067 (hermes config setaccepts unknown paths silently) but that issue is about validating writes, not about closing this specific read-side gap.Type of Change
Changes Made
gateway/config.py: readgateway: {...}into a singlegateway_sectionvariable once (consolidating three separateyaml_cfg.get("gateway")calls already in the function), add the same top-level-wins/nested-fallback check for each of the nine keys, mirroring the existingwrite_sessions_jsonprecedent exactlytests/gateway/test_config.py: 10 new regression tests (one per key) + atest_top_level_still_wins_over_nested_gateway_sectionprecedence check (verified: fail without the fix, pass with it — thestttest specifically assertsFalse, not the class defaultTrue, so it can't false-pass)Note for the reviewer
Because every fallback here is guarded by
isinstance(gateway_section, dict), this also makes thestreamingfallback tolerate a scalargateway:block (e.g.gateway: disabled) without crashing — the same crash #40837 (open) targets specifically forstreaming. This PR doesn't set out to fix that PR's issue, but the consolidated guard covers it as a side effect; flagging it here rather than leaving it to be found in review.How to Test
Also re-ran the full file and the multiplex-profile suites to confirm no regressions:
tests/gateway/test_config.py(96 passed),tests/gateway/test_multiplex_*.py(63 passed).Checklist