Skip to content

fix(tests): monkeypatch copilot token resolution to survive borrowed-credential pruning - #31975

Open
talwayh1 wants to merge 2 commits into
NousResearch:mainfrom
talwayh1:ci-fix/test-auth-remove-copilot-monkeypatch
Open

fix(tests): monkeypatch copilot token resolution to survive borrowed-credential pruning#31975
talwayh1 wants to merge 2 commits into
NousResearch:mainfrom
talwayh1:ci-fix/test-auth-remove-copilot-monkeypatch

Conversation

@talwayh1

Copy link
Copy Markdown

What

Fix test_auth_remove_copilot_suppresses_all_variantsload_pool() prunes the gh_cli borrowed credential entry because resolve_copilot_token() returns empty string in CI/test environment.

Root Cause

From PR #31416, _prune_stale_seeded_entries() removes all borrowed credential entries (gh_cli, env:* for copilot, etc.) when the token resolver returns empty — which is always the case in test environments without real tokens. The test writes a gh_cli entry, but load_pool() immediately prunes it, so resolve_target("1") fails with "No credential #1".

Fix

Monkeypatch hermes_cli.copilot_auth.resolve_copilot_token and get_copilot_api_token before calling auth_remove_command so the entry survives pruning (pitfall #13).

CI

Tranquil-Flow and others added 2 commits May 25, 2026 09:59
…copilot_suppresses_all_variants

CI: test_auth_remove_copilot_suppresses_all_variants fails with
SystemExit: No credential NousResearch#1. Provider: copilot. because load_pool()
prunes the gh_cli borrowed credential entry when resolve_copilot_token()
returns empty in CI/test environment (pitfall NousResearch#13).

Monkeypatch hermes_cli.copilot_auth.resolve_copilot_token and
get_copilot_api_token so the gh_cli entry survives pruning.
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard labels May 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

The copilot token monkeypatch test fix in test_auth_commands.py duplicates merged #31946 (same fix). This PR also bundles an unrelated agent_init.py change (stale localhost base_url guard for direct providers) which is the same fix as open PR #30034. Consider splitting — the test fix is already merged, and the agent_init.py change should coordinate with #30034.

@hclsys

hclsys commented May 25, 2026

Copy link
Copy Markdown

The copilot test fix itself is correct — monkeypatching resolve_copilot_token/get_copilot_api_token so the gh_cli borrowed entry survives _prune_stale_seeded_entries() is the right call for the CI/no-real-token case, and matches the #31416 pruning behavior described.

But the diff is broader than the fix(tests): title suggests — it also lands a production change in agent/agent_init.py: a ~30-line guard that clears agent.base_url/api_key for a direct registry provider when the base_url host is localhost (the Nous→DeepSeek 'Unknown Model' migration fix), plus a new test_direct_provider_ignores_stale_local_base_url in test_run_agent.py. That behavior change is reasonable and it's tested — but under a fix(tests): heading a maintainer skimming the title could merge a real runtime change without reviewing it as one. Worth either splitting the agent_init guard into its own fix(agent): PR, or retitling this to reflect that it carries a provider-routing fix + its test alongside the copilot test repair.

One correctness check on the guard while it's here: it sets agent.api_key = "" along with base_url = "". That's only safe if resolve_provider_client() re-derives the key for the direct provider from PROVIDER_REGISTRY/its key_env when handed an empty key — if it doesn't, clearing api_key would break auth for a provider that did have a valid key set. Could you confirm the empty-key path re-resolves from the registry? The localhost-override intent is right regardless; just want to be sure clearing the key doesn't strand a correctly-configured direct provider.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying the borrowed-Copilot fixture interaction.

Problems

  • The auth-test portion is already on main: tests/hermes_cli/test_auth_commands.py:1832-1864 hydrates the credential through the same Copilot resolver path. It merged in 920b350e57544285002c813b35747ebcc6659f9a (#31946).
  • The added provider guard would clear every direct provider's loopback endpoint. That conflicts with the current resolver, which deliberately honors model.base_url when its configured provider matches (hermes_cli/runtime_provider.py:481-491). Existing switch persistence clears stale URLs only when the new provider has no endpoint (tui_gateway/server.py:2813-2823).

Suggested changes

  • Drop the already-merged fixture change from salvage.
  • Keep the routing change separate and target a verified stale-persistence path while preserving explicit local proxy endpoints.

Automated hermes-sweeper review.

Comment thread agent/agent_init.py
_base_host = urlparse(agent.base_url or "").hostname or ""
if (
agent.provider in PROVIDER_REGISTRY
and agent.provider not in {"custom", "openrouter"}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition cannot distinguish a stale loopback URL from an intentional local proxy for a direct provider. Current runtime resolution intentionally honors a matching provider's configured model.base_url (hermes_cli/runtime_provider.py:481-491); please preserve that supported override and address the specific stale persistence path instead.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants