Skip to content

test(hermes_cli): isolate qwen fallthrough test from the ambient credential pool - #75235

Open
griffinwork40 wants to merge 1 commit into
NousResearch:mainfrom
griffinwork40:test/qwen-fallthrough-pool-isolation
Open

test(hermes_cli): isolate qwen fallthrough test from the ambient credential pool#75235
griffinwork40 wants to merge 1 commit into
NousResearch:mainfrom
griffinwork40:test/qwen-fallthrough-pool-isolation

Conversation

@griffinwork40

Copy link
Copy Markdown

Problem

test_qwen_oauth_auto_fallthrough_on_auth_failure depends on the contributor's ambient credentials, so it passes or fails depending on the machine it runs on.

The test stubs resolve_provider, resolve_qwen_runtime_credentials and _get_model_config — but not load_pool.

resolve_runtime_provider consults the credential pool first. When a pool entry matches the provider it returns at hermes_cli/runtime_provider.py:1874-1887, before ever reaching the qwen try/except at :1958 that this test exists to exercise:

if (
    entry is not None
    and pool_api_key
    and credential_pool_matches_provider(pool, provider, base_url=...)
):
    return _resolve_runtime_from_pool_entry(...)   # :1887

So on a machine with a live Qwen credential the stubbed AuthError is never raised, the resolver returns provider="qwen-oauth" straight from the pool, and the assertion at line 105 fails:

E       AssertionError: assert 'qwen-oauth' != 'qwen-oauth'

Traced with sys.settrace to confirm the mechanism rather than infer it — resolve_qwen_runtime_credentials is never called, and the return comes from line 1887 with source='qwen-cli'.

Not a resolver bug

The pool-first branch is correct and deliberate; I am not proposing any change to it. Following this repo's guidance to check whether an apparent gap is load-bearing before touching it, the fix belongs entirely in the test.

Fix

Adds the load_pool stub that the other tests in this same file already use — there are 23 such call sites, so this is the established convention here, and this test is the outlier:

monkeypatch.setattr(rp, "load_pool", lambda _provider: SimpleNamespace(has_credentials=lambda: False))

Verification

  • scripts/run_tests.sh tests/hermes_cli/test_runtime_provider_resolution.py52 passed
  • Full tests/hermes_cli/ slice → 3637 passed, with the same 2 pre-existing macOS-environment failures (test_service_manager.py::test_seed_supervise_skeleton_creates_expected_layout, test_gateway_service.py::…test_systemd_restart_gracefully_restarts_running_service_and_waits) present identically on main@cc4cab2f5, so no collateral change.
  • ruff check . clean · python scripts/check-windows-footguns.py --all clean (893 files)
  • The test is not made vacuous: with the stub in place the AuthError now genuinely fires and resolution falls through to openrouter (source='env/config'), which is what the test claims to verify.

Honest scope note

This is flakiness-hardening, not a hard red on main. The test passes in a clean environment, which is presumably why CI is green. It fails for contributors who have Qwen credentials configured — I hit it while working on an unrelated change, and it cost real time to distinguish from my own edits.

…ential pool

test_qwen_oauth_auto_fallthrough_on_auth_failure stubs resolve_provider,
resolve_qwen_runtime_credentials and _get_model_config, but not load_pool.

resolve_runtime_provider consults the credential pool first: when a pool entry
matches the provider it returns at hermes_cli/runtime_provider.py:1887, before
the qwen try/except at :1958 that the test exists to exercise. So on a machine
with a live Qwen credential the stubbed AuthError is never raised, the resolver
returns provider="qwen-oauth" from the pool, and the assertion fails — while the
same test passes wherever no Qwen credential happens to match.

Observed failing locally against a real Qwen credential; passes again once the
pool stops matching. Nothing about the resolver is wrong — the pool branch is
correct and deliberate — the test simply was not isolated from it.

Adds the load_pool stub the other tests in this file already use (23 call sites)
so the outcome no longer depends on the contributor's ambient credentials. The
test still exercises what it claims: the stubbed AuthError now fires and
resolution falls through to OpenRouter.
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating this test from local credential state. The premise is verified on current main: resolve_runtime_provider() checks a non-OpenRouter provider's pool before reaching the Qwen resolver (hermes_cli/runtime_provider.py:1808-1887), while the target test currently stubs only resolve_qwen_runtime_credentials (tests/hermes_cli/test_runtime_provider_resolution.py:89-105). A matching Qwen pool entry can therefore bypass the intended AuthError fallthrough path.

The added false has_credentials() stub is scoped to this test and prevents that early return without changing production behavior. Current-main commits since the PR branch do not touch the target test or hermes_cli/runtime_provider.py, so this should salvage cleanly.

Automated hermes-sweeper review.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure comp/cli CLI entry point, hermes_cli/, setup wizard provider/qwen Qwen / Alibaba Cloud (OAuth) area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jul 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #67465: same empty credential-pool test isolation mechanism.

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 duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have provider/qwen Qwen / Alibaba Cloud (OAuth) sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants