fix(auth): unify credential source removal — every source sticks (#13427)
Every credential source Hermes reads from now behaves identically on `hermes auth remove`: the pool entry stays gone across fresh load_pool() calls, even when the underlying external state (env var, OAuth file, auth.json block, config entry) is still present. Before this, auth_remove_command was a 110-line if/elif with five special cases, and three more sources (qwen-cli, copilot, custom config) had no removal handler at all — their pool entries silently resurrected on the next invocation. Even the handled cases diverged: codex suppressed, anthropic deleted-without-suppressing, nous cleared without suppressing. Each new provider added a new gap. What's new: agent/credential_sources.py — RemovalStep registry, one entry per source (env, claude_code, hermes_pkce, nous device_code, codex device_code, qwen-cli, copilot gh_cli + env vars, custom config). auth_remove_command dispatches uniformly via find_removal_step(). Changes elsewhere: agent/credential_pool.py — every upsert in _seed_from_env, _seed_from_singletons, and _seed_custom_pool now gates on is_source_suppressed(provider, source) via a shared helper. hermes_cli/auth_commands.py — auth_remove_command reduced to 25 lines of dispatch; auth_add_command now clears ALL suppressions for the provider on re-add (was env:* only). Copilot is special: the same token is seeded twice (gh_cli via _seed_from_singletons + env:<VAR> via _seed_from_env), so removing one entry without suppressing the other variants lets the duplicate resurrect. The copilot RemovalStep suppresses gh_cli + all three env variants (COPILOT_GITHUB_TOKEN, GH_TOKEN, GITHUB_TOKEN) at once. Tests: 11 new unit tests + 4059 existing pass. 12 E2E scenarios cover every source in isolated HERMES_HOME with simulated fresh processes.
This commit is contained in:
Teknium
@@ -1185,3 +1185,292 @@ def test_seed_from_env_respects_openrouter_suppression(tmp_path, monkeypatch):
|
||||
assert changed is False
|
||||
assert entries == []
|
||||
assert active == set()
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Unified credential-source stickiness — every source Hermes reads from has a
|
||||
# registered RemovalStep in agent.credential_sources, and every seeding path
|
||||
# gates on is_source_suppressed. Below: one test per source proving remove
|
||||
# sticks across a fresh load_pool() call.
|
||||
# =============================================================================
|
||||
|
||||
|
||||
def test_seed_from_singletons_respects_nous_suppression(tmp_path, monkeypatch):
|
||||
"""nous device_code must not re-seed from auth.json when suppressed."""
|
||||
hermes_home = tmp_path / "hermes"
|
||||
hermes_home.mkdir(parents=True, exist_ok=True)
|
||||
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
|
||||
|
||||
(hermes_home / "auth.json").write_text(json.dumps({
|
||||
"version": 1,
|
||||
"providers": {"nous": {"access_token": "tok", "refresh_token": "r", "expires_at": 9999999999}},
|
||||
"suppressed_sources": {"nous": ["device_code"]},
|
||||
}))
|
||||
|
||||
from agent.credential_pool import _seed_from_singletons
|
||||
entries = []
|
||||
changed, active = _seed_from_singletons("nous", entries)
|
||||
assert changed is False
|
||||
assert entries == []
|
||||
assert active == set()
|
||||
|
||||
|
||||
def test_seed_from_singletons_respects_copilot_suppression(tmp_path, monkeypatch):
|
||||
"""copilot gh_cli must not re-seed when suppressed."""
|
||||
hermes_home = tmp_path / "hermes"
|
||||
hermes_home.mkdir(parents=True, exist_ok=True)
|
||||
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
|
||||
|
||||
(hermes_home / "auth.json").write_text(json.dumps({
|
||||
"version": 1,
|
||||
"providers": {},
|
||||
"suppressed_sources": {"copilot": ["gh_cli"]},
|
||||
}))
|
||||
|
||||
# Stub resolve_copilot_token to return a live token
|
||||
import hermes_cli.copilot_auth as ca
|
||||
monkeypatch.setattr(ca, "resolve_copilot_token", lambda: ("ghp_fake", "gh auth token"))
|
||||
|
||||
from agent.credential_pool import _seed_from_singletons
|
||||
entries = []
|
||||
changed, active = _seed_from_singletons("copilot", entries)
|
||||
assert changed is False
|
||||
assert entries == []
|
||||
assert active == set()
|
||||
|
||||
|
||||
def test_seed_from_singletons_respects_qwen_suppression(tmp_path, monkeypatch):
|
||||
"""qwen-oauth qwen-cli must not re-seed from ~/.qwen/oauth_creds.json when suppressed."""
|
||||
hermes_home = tmp_path / "hermes"
|
||||
hermes_home.mkdir(parents=True, exist_ok=True)
|
||||
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
|
||||
|
||||
(hermes_home / "auth.json").write_text(json.dumps({
|
||||
"version": 1,
|
||||
"providers": {},
|
||||
"suppressed_sources": {"qwen-oauth": ["qwen-cli"]},
|
||||
}))
|
||||
|
||||
import hermes_cli.auth as ha
|
||||
monkeypatch.setattr(ha, "resolve_qwen_runtime_credentials", lambda **kw: {
|
||||
"api_key": "tok", "source": "qwen-cli", "base_url": "https://q",
|
||||
})
|
||||
|
||||
from agent.credential_pool import _seed_from_singletons
|
||||
entries = []
|
||||
changed, active = _seed_from_singletons("qwen-oauth", entries)
|
||||
assert changed is False
|
||||
assert entries == []
|
||||
assert active == set()
|
||||
|
||||
|
||||
def test_seed_from_singletons_respects_hermes_pkce_suppression(tmp_path, monkeypatch):
|
||||
"""anthropic hermes_pkce must not re-seed from ~/.hermes/.anthropic_oauth.json when suppressed."""
|
||||
hermes_home = tmp_path / "hermes"
|
||||
hermes_home.mkdir(parents=True, exist_ok=True)
|
||||
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
|
||||
|
||||
import yaml
|
||||
(hermes_home / "config.yaml").write_text(yaml.dump({"model": {"provider": "anthropic", "model": "claude"}}))
|
||||
(hermes_home / "auth.json").write_text(json.dumps({
|
||||
"version": 1,
|
||||
"providers": {},
|
||||
"suppressed_sources": {"anthropic": ["hermes_pkce"]},
|
||||
}))
|
||||
|
||||
# Stub the readers so only hermes_pkce is "available"; claude_code returns None
|
||||
import agent.anthropic_adapter as aa
|
||||
monkeypatch.setattr(aa, "read_hermes_oauth_credentials", lambda: {
|
||||
"accessToken": "tok", "refreshToken": "r", "expiresAt": 9999999999000,
|
||||
})
|
||||
monkeypatch.setattr(aa, "read_claude_code_credentials", lambda: None)
|
||||
|
||||
from agent.credential_pool import _seed_from_singletons
|
||||
entries = []
|
||||
changed, active = _seed_from_singletons("anthropic", entries)
|
||||
# hermes_pkce suppressed, claude_code returns None → nothing should be seeded
|
||||
assert entries == []
|
||||
assert "hermes_pkce" not in active
|
||||
|
||||
|
||||
def test_seed_custom_pool_respects_config_suppression(tmp_path, monkeypatch):
|
||||
"""Custom provider config:<name> source must not re-seed when suppressed."""
|
||||
hermes_home = tmp_path / "hermes"
|
||||
hermes_home.mkdir(parents=True, exist_ok=True)
|
||||
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
|
||||
|
||||
import yaml
|
||||
(hermes_home / "config.yaml").write_text(yaml.dump({
|
||||
"model": {},
|
||||
"custom_providers": [
|
||||
{"name": "my", "base_url": "https://c.example.com", "api_key": "sk-custom"},
|
||||
],
|
||||
}))
|
||||
|
||||
from agent.credential_pool import _seed_custom_pool, get_custom_provider_pool_key
|
||||
pool_key = get_custom_provider_pool_key("https://c.example.com")
|
||||
|
||||
(hermes_home / "auth.json").write_text(json.dumps({
|
||||
"version": 1,
|
||||
"providers": {},
|
||||
"suppressed_sources": {pool_key: ["config:my"]},
|
||||
}))
|
||||
|
||||
entries = []
|
||||
changed, active = _seed_custom_pool(pool_key, entries)
|
||||
assert changed is False
|
||||
assert entries == []
|
||||
assert "config:my" not in active
|
||||
|
||||
|
||||
def test_credential_sources_registry_has_expected_steps():
|
||||
"""Sanity check — the registry contains the expected RemovalSteps.
|
||||
|
||||
Guards against accidentally dropping a step during future refactors.
|
||||
If you add a new credential source, add it to the expected set below.
|
||||
"""
|
||||
from agent.credential_sources import _REGISTRY
|
||||
|
||||
descriptions = {step.description for step in _REGISTRY}
|
||||
expected = {
|
||||
"gh auth token / COPILOT_GITHUB_TOKEN / GH_TOKEN",
|
||||
"Any env-seeded credential (XAI_API_KEY, DEEPSEEK_API_KEY, etc.)",
|
||||
"~/.claude/.credentials.json",
|
||||
"~/.hermes/.anthropic_oauth.json",
|
||||
"auth.json providers.nous",
|
||||
"auth.json providers.openai-codex + ~/.codex/auth.json",
|
||||
"~/.qwen/oauth_creds.json",
|
||||
"Custom provider config.yaml api_key field",
|
||||
}
|
||||
assert descriptions == expected, f"Registry mismatch. Got: {descriptions}"
|
||||
|
||||
|
||||
def test_credential_sources_find_step_returns_none_for_manual():
|
||||
"""Manual entries have nothing external to clean up — no step registered."""
|
||||
from agent.credential_sources import find_removal_step
|
||||
assert find_removal_step("openrouter", "manual") is None
|
||||
assert find_removal_step("xai", "manual") is None
|
||||
|
||||
|
||||
def test_credential_sources_find_step_copilot_before_generic_env(tmp_path, monkeypatch):
|
||||
"""copilot env:GH_TOKEN must dispatch to the copilot step, not the
|
||||
generic env-var step. The copilot step handles the duplicate-source
|
||||
problem (same token seeded as both gh_cli and env:<VAR>); the generic
|
||||
env step would only suppress one of the variants.
|
||||
"""
|
||||
from agent.credential_sources import find_removal_step
|
||||
|
||||
step = find_removal_step("copilot", "env:GH_TOKEN")
|
||||
assert step is not None
|
||||
assert "copilot" in step.description.lower() or "gh" in step.description.lower()
|
||||
|
||||
# Generic step still matches any other provider's env var
|
||||
step = find_removal_step("xai", "env:XAI_API_KEY")
|
||||
assert step is not None
|
||||
assert "env-seeded" in step.description.lower()
|
||||
|
||||
|
||||
def test_auth_remove_copilot_suppresses_all_variants(tmp_path, monkeypatch):
|
||||
"""Removing any copilot source must suppress gh_cli + all env:* variants
|
||||
so the duplicate-seed paths don't resurrect the credential.
|
||||
"""
|
||||
hermes_home = tmp_path / "hermes"
|
||||
hermes_home.mkdir(parents=True, exist_ok=True)
|
||||
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
|
||||
|
||||
_write_auth_store(
|
||||
tmp_path,
|
||||
{
|
||||
"version": 1,
|
||||
"credential_pool": {
|
||||
"copilot": [{
|
||||
"id": "c1",
|
||||
"label": "gh auth token",
|
||||
"auth_type": "api_key",
|
||||
"priority": 0,
|
||||
"source": "gh_cli",
|
||||
"access_token": "ghp_fake",
|
||||
}]
|
||||
},
|
||||
},
|
||||
)
|
||||
|
||||
from types import SimpleNamespace
|
||||
from hermes_cli.auth import is_source_suppressed
|
||||
from hermes_cli.auth_commands import auth_remove_command
|
||||
|
||||
auth_remove_command(SimpleNamespace(provider="copilot", target="1"))
|
||||
|
||||
assert is_source_suppressed("copilot", "gh_cli")
|
||||
assert is_source_suppressed("copilot", "env:COPILOT_GITHUB_TOKEN")
|
||||
assert is_source_suppressed("copilot", "env:GH_TOKEN")
|
||||
assert is_source_suppressed("copilot", "env:GITHUB_TOKEN")
|
||||
|
||||
|
||||
def test_auth_add_clears_all_suppressions_including_non_env(tmp_path, monkeypatch):
|
||||
"""Re-adding a credential via `hermes auth add <provider>` clears ALL
|
||||
suppression markers for the provider, not just env:*. This matches
|
||||
the single "re-engage" semantic — the user wants auth back, period.
|
||||
"""
|
||||
hermes_home = tmp_path / "hermes"
|
||||
hermes_home.mkdir(parents=True, exist_ok=True)
|
||||
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
|
||||
|
||||
_write_auth_store(
|
||||
tmp_path,
|
||||
{
|
||||
"version": 1,
|
||||
"providers": {},
|
||||
"suppressed_sources": {
|
||||
"copilot": ["gh_cli", "env:GH_TOKEN", "env:COPILOT_GITHUB_TOKEN"],
|
||||
},
|
||||
},
|
||||
)
|
||||
|
||||
from types import SimpleNamespace
|
||||
from hermes_cli.auth import is_source_suppressed
|
||||
from hermes_cli.auth_commands import auth_add_command
|
||||
|
||||
auth_add_command(SimpleNamespace(
|
||||
provider="copilot", auth_type="api_key",
|
||||
api_key="ghp-manual", label="m",
|
||||
))
|
||||
|
||||
assert not is_source_suppressed("copilot", "gh_cli")
|
||||
assert not is_source_suppressed("copilot", "env:GH_TOKEN")
|
||||
assert not is_source_suppressed("copilot", "env:COPILOT_GITHUB_TOKEN")
|
||||
|
||||
|
||||
def test_auth_remove_codex_manual_device_code_suppresses_canonical(tmp_path, monkeypatch):
|
||||
"""Removing a manual:device_code entry (from `hermes auth add openai-codex`)
|
||||
must suppress the canonical ``device_code`` key, not ``manual:device_code``.
|
||||
The re-seed gate in _seed_from_singletons checks ``device_code``.
|
||||
"""
|
||||
hermes_home = tmp_path / "hermes"
|
||||
hermes_home.mkdir(parents=True, exist_ok=True)
|
||||
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
|
||||
|
||||
_write_auth_store(
|
||||
tmp_path,
|
||||
{
|
||||
"version": 1,
|
||||
"providers": {"openai-codex": {"tokens": {"access_token": "t", "refresh_token": "r"}}},
|
||||
"credential_pool": {
|
||||
"openai-codex": [{
|
||||
"id": "cdx",
|
||||
"label": "manual-codex",
|
||||
"auth_type": "oauth",
|
||||
"priority": 0,
|
||||
"source": "manual:device_code",
|
||||
"access_token": "t",
|
||||
}]
|
||||
},
|
||||
},
|
||||
)
|
||||
|
||||
from types import SimpleNamespace
|
||||
from hermes_cli.auth import is_source_suppressed
|
||||
from hermes_cli.auth_commands import auth_remove_command
|
||||
|
||||
auth_remove_command(SimpleNamespace(provider="openai-codex", target="1"))
|
||||
assert is_source_suppressed("openai-codex", "device_code")
|
||||
|
||||
Reference in New Issue
Block a user