Skip to content

fix(cli): keep exhausted-pool providers visible in the main /model picker - #103843

Open
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-103829
Open

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-103829

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

/model with no args in an interactive CLI session opens the main model picker via _show_model_picker, which feeds build_models_payload without for_picker=True. A provider whose credential-pool entries are all marked exhausted (HTTP 429, last_error_reset_at set) therefore fails the has_creds gate in _overlay_has_creds and its row disappears from the main picker as if the provider were not authenticated — the exact symptom in #103829.

The visibility branch already exists and is gated on the flag: with for_picker=True, load_pool(slug).has_credentials() keeps the row visible so the user can pick a different model under the same provider (limits are per-model for many providers). The gateway interactive picker forwards it (#66584) and the aux-task/vision pickers forward it (#66624); the CLI main picker surface was simply never wired to the same contract. This PR forwards for_picker=True from _show_model_picker — no filter logic is duplicated or moved.

Related Issue

Fixes #103829

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/cli_model_switch_mixin.py: _show_model_picker now passes for_picker=True to build_models_payload (+4 lines incl. rationale comment).
  • tests/hermes_cli/test_show_model_picker_exhausted_pool.py: new regression tests asserting the flag is forwarded and the picker open/usage-fallback paths are unchanged.

How to Test

  1. pytest tests/hermes_cli/test_show_model_picker_exhausted_pool.py -v — 2 passed (should pass).
  2. pytest tests/hermes_cli/test_authenticated_providers_exhausted_pool.py tests/hermes_cli/test_aux_picker_inventory.py tests/hermes_cli/test_25106_global_switch_persists_base_url_api_mode.py tests/hermes_cli/test_apply_model_switch_result_context.py tests/hermes_cli/test_model_switch_context_offload.py tests/hermes_cli/test_user_providers_model_switch.py -q — 27 passed, no regressions (Observed result: 27 passed on Python 3.11.15).
  3. Manual repro of the issue: exhaust a pool-based provider (e.g. openai-codex device-code auth hitting HTTP 429), run /model in an interactive session — the provider row should now stay visible instead of vanishing until last_error_reset_at elapses or hermes auth reset <provider> is run.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (arm64), Python 3.11.15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

For New Skills

N/A — not a skill.

Screenshots / Logs

Not applicable — behavior verified via the regression tests above; the underlying visibility branch is already covered end-to-end by tests/hermes_cli/test_authenticated_providers_exhausted_pool.py::test_picker_shows_exhausted_pool_provider.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/auth Authentication, OAuth, credential pools P2 Medium — degraded but workaround exists labels Sep 5, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor Author

CI: Python tests / e2e failure is unrelated to this PR — transient timing flake

The only failing test is tests/e2e/test_relay_native_openai_stream.py::test_openai_stream_final_tool_call_delta_reaches_relay_parent_event (AssertionError: Relay's finalizer did not start; 62 passed, 7 skipped).

Evidence that this is a flake rather than a regression from this change:

  • Unrelated domain: this PR only touches hermes_cli/cli_model_switch_mixin.py (for_picker=True wiring in _show_model_picker) plus a new test file. The failing test exercises the gateway relay streaming path and never imports anything from hermes_cli.
  • New test, first exposure: the test was added to main in 74de0fd (09-05 10:25 UTC), ~8h before this PR was opened. This is the first time this PR's merge ref runs it.
  • Merge base is green locally: 9dd6634c56 (the exact merge base this CI ran against) passes the test 6/6 consecutive runs locally (1.6–2.4s each), and main's own ci.yaml runs since the test landed are green.
  • Timing-sensitive shape: the assertion is a threading.Event.wait(5) synchronization between the relay finalizer thread and the consumer — exactly the kind of deadline that gets missed on a loaded CI runner. A nearby unrelated PR (#"Too big for this machine" model tagging, run 33984526328, 18:35 UTC) failed only deadline/nonblocking/ticker-style tests in the same window, which points to runner load at that time.

No code change needed; happy to re-run if a maintainer deems it necessary.

@seppegadeyne

Copy link
Copy Markdown
Contributor

Revalidated this against current main at 8863b36fd663 on Linux / Python 3.11.15. The CLI main picker still does not forward for_picker=True.

Using this PR's two regression tests on unmodified main reproduces both failures (captured.get("for_picker") is None). Adding only for_picker=True to the current _show_model_picker call fixes them while preserving the newer capabilities=True argument. The regression file plus the five still-present adjacent provider/picker/model-switch test files pass through scripts/run_tests.sh -j 2: 31 passed, 0 failed, 6 files. (test_25106_global_switch_persists_base_url_api_mode.py from the original test list is no longer present on this main snapshot, so it is not included in that count.)

I previously closed my duplicate #109257 in favor of this PR and still carry the equivalent fix locally. Maintainers: could this get another look? It is still needed for #103829. The original CI run remains red on the relay e2e test reported above; I have not rerun that e2e case, so this is a focused regression check rather than a claim that the full suite is green.

…cker

Rebased onto current main (3917c7d): the picker call gained
capabilities=True for the reasoning-effort step, so the for_picker=True
forwarding now rides alongside it instead of conflicting.

The regression tests additionally assert capabilities=True so a future
rebase cannot silently drop either flag.
@liuhao1024
liuhao1024 force-pushed the liuhao/cron-bugfix-103829 branch from 1ce2213 to 0c4c092 Compare September 21, 2026 09:28
@liuhao1024

Copy link
Copy Markdown
Contributor Author

Thanks @seppegadeyne for the thorough revalidation against current main — exactly the signal this needed.

I've rebased the branch onto current main (3917c7dd6a): the for_picker=True forwarding now sits alongside the newer capabilities=True (kept for the reasoning-effort step) instead of conflicting with it. The regression tests additionally assert capabilities=True now, so a future rebase can't silently drop either flag.

Same shape as before — a one-call-site flag change plus the two regression tests, verified green locally (26 passed across the new tests and the adjacent picker/model-switch suites). The rebase push also restarts CI, which should clear the stale red run from 09-05 (that one was the unrelated relay e2e timing flake documented above).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Main model picker hides provider row when credential pool is exhausted (aux pickers already fixed in #66624)

3 participants