refactor(inventory): extract shared ConfigContext + build_models_payload#23666
refactor(inventory): extract shared ConfigContext + build_models_payload#23666kshitijk4poor wants to merge 1 commit into
Conversation
Three call-sites in the codebase each duplicated the same config-slice
+ list_authenticated_providers + post-processing pattern:
- hermes_cli/web_server.py /api/model/options
- tui_gateway/server.py model.options JSON-RPC
- tui_gateway/server.py model.save_key JSON-RPC
This consolidates them onto hermes_cli/inventory.py:
load_picker_context() -> ConfigContext
Replaces the 17-LOC config-slice (model.{default,name,provider,
base_url}, providers:, custom_providers:) every consumer did
inline.
ConfigContext.with_overrides(*, current_provider=, current_model=,
current_base_url=) -> ConfigContext
Truthy-only overlay for TUI agent-session state on top of disk
config. Empty getattr(agent, ...) attrs MUST NOT clobber disk.
build_models_payload(ctx, *, include_unconfigured, picker_hints,
canonical_order, max_models) -> dict
Single payload builder. Delegates curation to
list_authenticated_providers (does not call provider_model_ids
per row \u2014 that pulls non-agentic models). picker_hints +
canonical_order produce the TUI ModelPickerDialog shape;
defaults match the dashboard's existing /api/model/options
contract.
Two latent bugs fixed by consolidation:
1. The dashboard read cfg.get('custom_providers') directly, missing
the v12+ keyed providers: form. Now both surfaces go through
get_compatible_custom_providers().
2. The TUI's canonical-merge keyed on is_user_defined to decide order.
Section 3 of list_authenticated_providers sets is_user_defined=True
on rows from the providers: config dict even when the slug is
canonical \u2014 that silently demoted them to the picker tail.
_reorder_canonical now keys on slug membership instead.
Stats: +666 / -145 (net +521). Module 240 LOC; 18 behavior tests.
This PR replaces the rejected NousResearch#23369 (which bundled the consolidation
with new scriptable CLI surfaces \u2014 hermes models list/status, hermes
providers list \u2014 and a JSON contract that have no external user
demand). Just the refactor; the CLI surface is deferred to a separate
PR gated on actual demand.
Refs NousResearch#23359.
|
@daimon-nous review |
Review: refactor(inventory) — extract shared ConfigContext + build_models_payloadVerdict: ✅ LGTM — clean, well-scoped refactor with a real bug fix included. What it doesExtracts the duplicated "config-slice → Bug fix (real, confirmed on main)All three pre-existing call-sites used Code quality observations
Minor observations (not blocking)
CI status3 checks fail ( SummaryWell-scoped refactor that reduces duplication across 3 inventory call-sites, fixes a real v12+ provider visibility bug, and adds a regression guard for canonical-slug ordering. The consolidation ratio (145 deleted / 240 added = 0.60) is honest. Replaces the rejected #23369 which bundled unnecessary CLI surfaces. Ready to merge. |
The interactive CLI /model picker was the third call-site duplicating the inline config-slice + list_authenticated_providers pattern that PR #23666 consolidated for the dashboard and TUI. Route it through load_picker_context() + build_models_payload() too so all surfaces that show authenticated providers share one substrate. Side effect: cli.py now also benefits from the latent v12+ keyed providers fix (custom_providers populated via get_compatible_custom_providers, not cfg.get raw). The aux-task switcher (hermes_cli/main.py) and gateway model switcher (gateway/run.py) deliberately stay on the legacy path — they use different config sections (auxiliary.<task>.*) and a different config loader (_load_gateway_config) respectively, so forcing them through ConfigContext would either overload its semantics or grow the module past the clean refactor scope.
|
Salvaged onto current main via #25450. Your inventory module and tests cherry-picked with authorship preserved; we also extended the consolidation to the CLI |
The interactive CLI /model picker was the third call-site duplicating the inline config-slice + list_authenticated_providers pattern that PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through load_picker_context() + build_models_payload() too so all surfaces that show authenticated providers share one substrate. Side effect: cli.py now also benefits from the latent v12+ keyed providers fix (custom_providers populated via get_compatible_custom_providers, not cfg.get raw). The aux-task switcher (hermes_cli/main.py) and gateway model switcher (gateway/run.py) deliberately stay on the legacy path — they use different config sections (auxiliary.<task>.*) and a different config loader (_load_gateway_config) respectively, so forcing them through ConfigContext would either overload its semantics or grow the module past the clean refactor scope.
The interactive CLI /model picker was the third call-site duplicating the inline config-slice + list_authenticated_providers pattern that PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through load_picker_context() + build_models_payload() too so all surfaces that show authenticated providers share one substrate. Side effect: cli.py now also benefits from the latent v12+ keyed providers fix (custom_providers populated via get_compatible_custom_providers, not cfg.get raw). The aux-task switcher (hermes_cli/main.py) and gateway model switcher (gateway/run.py) deliberately stay on the legacy path — they use different config sections (auxiliary.<task>.*) and a different config loader (_load_gateway_config) respectively, so forcing them through ConfigContext would either overload its semantics or grow the module past the clean refactor scope.
The interactive CLI /model picker was the third call-site duplicating the inline config-slice + list_authenticated_providers pattern that PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through load_picker_context() + build_models_payload() too so all surfaces that show authenticated providers share one substrate. Side effect: cli.py now also benefits from the latent v12+ keyed providers fix (custom_providers populated via get_compatible_custom_providers, not cfg.get raw). The aux-task switcher (hermes_cli/main.py) and gateway model switcher (gateway/run.py) deliberately stay on the legacy path — they use different config sections (auxiliary.<task>.*) and a different config loader (_load_gateway_config) respectively, so forcing them through ConfigContext would either overload its semantics or grow the module past the clean refactor scope.
The interactive CLI /model picker was the third call-site duplicating the inline config-slice + list_authenticated_providers pattern that PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through load_picker_context() + build_models_payload() too so all surfaces that show authenticated providers share one substrate. Side effect: cli.py now also benefits from the latent v12+ keyed providers fix (custom_providers populated via get_compatible_custom_providers, not cfg.get raw). The aux-task switcher (hermes_cli/main.py) and gateway model switcher (gateway/run.py) deliberately stay on the legacy path — they use different config sections (auxiliary.<task>.*) and a different config loader (_load_gateway_config) respectively, so forcing them through ConfigContext would either overload its semantics or grow the module past the clean refactor scope.
The interactive CLI /model picker was the third call-site duplicating the inline config-slice + list_authenticated_providers pattern that PR #23666 consolidated for the dashboard and TUI. Route it through load_picker_context() + build_models_payload() too so all surfaces that show authenticated providers share one substrate. Side effect: cli.py now also benefits from the latent v12+ keyed providers fix (custom_providers populated via get_compatible_custom_providers, not cfg.get raw). The aux-task switcher (hermes_cli/main.py) and gateway model switcher (gateway/run.py) deliberately stay on the legacy path — they use different config sections (auxiliary.<task>.*) and a different config loader (_load_gateway_config) respectively, so forcing them through ConfigContext would either overload its semantics or grow the module past the clean refactor scope.
The interactive CLI /model picker was the third call-site duplicating the inline config-slice + list_authenticated_providers pattern that PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through load_picker_context() + build_models_payload() too so all surfaces that show authenticated providers share one substrate. Side effect: cli.py now also benefits from the latent v12+ keyed providers fix (custom_providers populated via get_compatible_custom_providers, not cfg.get raw). The aux-task switcher (hermes_cli/main.py) and gateway model switcher (gateway/run.py) deliberately stay on the legacy path — they use different config sections (auxiliary.<task>.*) and a different config loader (_load_gateway_config) respectively, so forcing them through ConfigContext would either overload its semantics or grow the module past the clean refactor scope.
The interactive CLI /model picker was the third call-site duplicating the inline config-slice + list_authenticated_providers pattern that PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through load_picker_context() + build_models_payload() too so all surfaces that show authenticated providers share one substrate. Side effect: cli.py now also benefits from the latent v12+ keyed providers fix (custom_providers populated via get_compatible_custom_providers, not cfg.get raw). The aux-task switcher (hermes_cli/main.py) and gateway model switcher (gateway/run.py) deliberately stay on the legacy path — they use different config sections (auxiliary.<task>.*) and a different config loader (_load_gateway_config) respectively, so forcing them through ConfigContext would either overload its semantics or grow the module past the clean refactor scope.
The interactive CLI /model picker was the third call-site duplicating the inline config-slice + list_authenticated_providers pattern that PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through load_picker_context() + build_models_payload() too so all surfaces that show authenticated providers share one substrate. Side effect: cli.py now also benefits from the latent v12+ keyed providers fix (custom_providers populated via get_compatible_custom_providers, not cfg.get raw). The aux-task switcher (hermes_cli/main.py) and gateway model switcher (gateway/run.py) deliberately stay on the legacy path — they use different config sections (auxiliary.<task>.*) and a different config loader (_load_gateway_config) respectively, so forcing them through ConfigContext would either overload its semantics or grow the module past the clean refactor scope.
The interactive CLI /model picker was the third call-site duplicating the inline config-slice + list_authenticated_providers pattern that PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through load_picker_context() + build_models_payload() too so all surfaces that show authenticated providers share one substrate. Side effect: cli.py now also benefits from the latent v12+ keyed providers fix (custom_providers populated via get_compatible_custom_providers, not cfg.get raw). The aux-task switcher (hermes_cli/main.py) and gateway model switcher (gateway/run.py) deliberately stay on the legacy path — they use different config sections (auxiliary.<task>.*) and a different config loader (_load_gateway_config) respectively, so forcing them through ConfigContext would either overload its semantics or grow the module past the clean refactor scope.
The interactive CLI /model picker was the third call-site duplicating the inline config-slice + list_authenticated_providers pattern that PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through load_picker_context() + build_models_payload() too so all surfaces that show authenticated providers share one substrate. Side effect: cli.py now also benefits from the latent v12+ keyed providers fix (custom_providers populated via get_compatible_custom_providers, not cfg.get raw). The aux-task switcher (hermes_cli/main.py) and gateway model switcher (gateway/run.py) deliberately stay on the legacy path — they use different config sections (auxiliary.<task>.*) and a different config loader (_load_gateway_config) respectively, so forcing them through ConfigContext would either overload its semantics or grow the module past the clean refactor scope.
The interactive CLI /model picker was the third call-site duplicating the inline config-slice + list_authenticated_providers pattern that PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through load_picker_context() + build_models_payload() too so all surfaces that show authenticated providers share one substrate. Side effect: cli.py now also benefits from the latent v12+ keyed providers fix (custom_providers populated via get_compatible_custom_providers, not cfg.get raw). The aux-task switcher (hermes_cli/main.py) and gateway model switcher (gateway/run.py) deliberately stay on the legacy path — they use different config sections (auxiliary.<task>.*) and a different config loader (_load_gateway_config) respectively, so forcing them through ConfigContext would either overload its semantics or grow the module past the clean refactor scope.
What this PR does
Three call-sites in the codebase each duplicated the same config-slice +
list_authenticated_providers+ post-processing pattern:hermes_cli/web_server.py/api/model/options(dashboard)tui_gateway/server.pymodel.optionsJSON-RPCtui_gateway/server.pymodel.save_keyJSON-RPCThis consolidates them onto a new
hermes_cli/inventory.py:/api/model/optionscfg.get("custom_providers")(which misses v12+ keyed providers — see Bug A below) +list_authenticated_providersbuild_models_payload(load_picker_context(), max_models=50)model.optionslist_authenticated_providers+ 45-line canonical-merge / picker-hint post-processingbuild_models_payload(ctx, include_unconfigured=True, picker_hints=True, canonical_order=True, max_models=50)withwith_overrides()for agent-session statemodel.save_keylist_authenticated_providers+ slug matchbuild_models_payload(ctx, picker_hints=True)then slug matchScope: 3 of 5 inventory call-sites
Issue #23359 named four call-sites (plus one this PR found in
gateway/run.py). This PR migrates 3 of 5:hermes_cli/web_server.py/api/model/optionstui_gateway/server.pymodel.optionstui_gateway/server.pymodel.save_keycli.py:6294interactive pickerhermes_cli/main.py:2269aux-task switchergateway/run.py:8999gateway model switcherThe remaining 3 call-sites would migrate cleanly to
build_models_payload()in a follow-up PR; they're stable enough that there's no urgency, and bundling everything would expand this PR's scope past what reviewers can reasonably check in one pass.One latent bug fixed; one regression guard added
Bug A (real, fixed in 3 sites):
web_server.py:998,tui_gateway/server.py:5178(model.options), andtui_gateway/server.py:5312(model.save_key) all readcfg.get("custom_providers")directly onmain, missing the v12+ keyedproviders:form. Routing all 3 sites throughload_picker_context()(which callsget_compatible_custom_providers()) fixes the dashboard + both TUI handlers in one go.Regression guard against canonical-slug demotion (preventive, not curative): Section 3 of
list_authenticated_providerssetsis_user_defined=Trueon rows from theproviders:config dict even when the slug is canonical. If a future refactor naively keys row ordering onis_user_defined, canonical providers would silently sink to the picker tail. Currenttui_gateway/server.py:5194already keys on slug membership correctly (landed in commitc23c7c994"fix(tui): address remaining review feedback — ordering and digit shortcuts")._reorder_canonicalpreserves that invariant in the new module, andtest_canonical_order_uses_slug_not_is_user_defined_flaglocks it as a regression guard. The bug doesn't currently exist; this prevents it from being reintroduced.Why this PR (vs #23369)
This replaces the rejected #23369, which bundled this consolidation with three new scriptable CLI surfaces (
hermes models list,hermes models status,hermes providers list), an offline-mode synthesis path, a versioned JSON contract, and text renderers — none of which have external user demand. That PR was+1638/-140; only ~230 LOC of it served the consolidation goal.This PR is just the refactor. The scriptable CLI is deferred until a real user asks. Issue #23359 stays open after this lands — the consolidation half is addressed; the scriptable-CLI half is deferred pending external demand.
Stats
+666 / -145(net+521)145 / 240 = 0.60(above the 0.5 threshold for honest consolidation accounting)inventory.py +240/-0,web_server.py +2/-32,tui_gateway/server.py +46/-113,test_inventory.py +378/-0What this PR is NOT doing
--offline) —--no-livealready covers cron/CI use cases via curated lists.list_authenticated_providersitself — the substrate is unchanged; this only consolidates how three callers invoke it.cli.py), the aux-task switcher (hermes_cli/main.py), or the gateway model switcher (gateway/run.py).Test plan
tests/hermes_cli/test_inventory.pylock the public API contract:load_picker_context()reproduces the inline 17-LOC config-slice exactly (full dict,namefallback, legacy bare-stringmodel, empty config).with_overrides()is truthy-only (empty agent attrs preserved).build_models_payload()returns the expected{providers, model, provider}shape and does NOT callprovider_model_idsper row.include_unconfigured=TrueappendsCANONICAL_PROVIDERSskeleton rows without duplicating already-present slugs.picker_hints=Trueaddsauthenticated/auth_type/key_env/warningand the api_key warning format references the actual env var (e.g.ANTHROPIC_API_KEY).canonical_orderkeys on slug membership, NOTis_user_defined— canonical slugs configured via the keyedproviders:schema must NOT be demoted to the tail.OPENROUTER_API_KEY/ANTHROPIC_API_KEYvalues never appear in the JSON-serialized payload, even withpicker_hints=True.web/src/components/ModelPickerDialog.tsxreads (name,slug,models,total_models,is_current,authenticated).web_server.get_model_options()→ 9 authenticated rows, clean shape, current_provider/current_model populated correctly.model.optionsJSON-RPC → 36 rows (full universe with hints + canonical order), warning hint correctly formatted.tests/hermes_cli/+tests/test_tui_gateway_server.py: 4379 passed, 17 failed. The 17 failures reproduce identically onorigin/main(gateway systemd / WSL detection / custom-provider grouping / plugin discovery / web-server auth / hangup protection — all pre-existing, none introduced by this PR).hermes_cli/inventory.py.Known test gaps (acknowledged, not blockers)
build_models_payloadoutput equals the OLD inline pipeline output for shared inputs. Behavioral coverage is solid via individual feature tests; if the inline slice had a quirk (e.g.or ""precedence on an unusual model dict), parity tests would catch it. Worth adding in a follow-up if reviewers want belt-and-braces.model.save_keypost-save populated-models contract. The credential-leak canary exercises the real curation path but doesn't assert the freshly-keyed provider has non-emptymodels. Low risk in practice (env var is set intoos.environbefore the rebuild call) but uncovered in tests.provider_model_idsper row" assertion runs with default flags, not the production TUI shape (include_unconfigured=True, picker_hints=True, canonical_order=True). Risk is bounded because_apply_picker_hintsand_reorder_canonicalare pure transformations, but a one-line additional assertion would harden it.Refs