diff --git a/hermes_cli/auth.py b/hermes_cli/auth.py index 603a418f43..0843b5ce71 100644 --- a/hermes_cli/auth.py +++ b/hermes_cli/auth.py @@ -2159,52 +2159,45 @@ def refresh_nous_oauth_from_state( ) -def persist_nous_credentials( - creds: Dict[str, Any], - *, - label: str, - source: str, -): - """Persist minted Nous OAuth credentials to both auth-store sections. +NOUS_DEVICE_CODE_SOURCE = "device_code" + + +def persist_nous_credentials(creds: Dict[str, Any]): + """Persist minted Nous OAuth credentials as the singleton provider state + and ensure the credential pool is in sync. Nous credentials are read at runtime from two independent locations: - - ``credential_pool.nous``: used by the runtime ``pool.select()`` path that - services outbound inference requests. - - ``providers.nous``: used by ``resolve_nous_runtime_credentials()`` — the - singleton-state reader invoked during 401 recovery and dashboard status - checks. + - ``providers.nous``: singleton state read by + ``resolve_nous_runtime_credentials()`` during 401 recovery and by + ``_seed_from_singletons()`` during pool load. + - ``credential_pool.nous``: used by the runtime ``pool.select()`` path. - Historically ``hermes auth add nous`` wrote only to the pool while the web - dashboard device-code flow wrote to both, so CLI-provisioned profiles - failed silently when the recovery path was later consulted. This helper - is the single source of truth for CLI/web device-code persistence: both - stores are always written together. + Historically ``hermes auth add nous`` wrote a ``manual:device_code`` pool + entry only, skipping ``providers.nous``. When the 24h agent_key TTL + expired, the recovery path read the empty singleton state and raised + ``AuthError`` silently (``logger.debug`` at INFO level). - Returns the added :class:`PooledCredential` entry. + This helper writes ``providers.nous`` then calls ``load_pool("nous")`` so + ``_seed_from_singletons`` materialises the canonical ``device_code`` pool + entry from the singleton. Re-running login upserts the same entry in + place; the pool never accumulates duplicate device_code rows. + + Returns the upserted :class:`PooledCredential` entry (or ``None`` if + seeding somehow produced no match — shouldn't happen). """ - from agent.credential_pool import ( - PooledCredential, - load_pool, - AUTH_TYPE_OAUTH, - ) - - pool = load_pool("nous") - entry = PooledCredential.from_dict("nous", { - **creds, - "label": label, - "auth_type": AUTH_TYPE_OAUTH, - "source": source, - "base_url": creds.get("inference_base_url"), - }) - pool.add_entry(entry) + from agent.credential_pool import load_pool with _auth_store_lock(): auth_store = _load_auth_store() _save_provider_state(auth_store, "nous", creds) _save_auth_store(auth_store) - return entry + pool = load_pool("nous") + return next( + (e for e in pool.entries() if e.source == NOUS_DEVICE_CODE_SOURCE), + None, + ) def resolve_nous_runtime_credentials( diff --git a/hermes_cli/auth_commands.py b/hermes_cli/auth_commands.py index 04d9d15c20..71fca04912 100644 --- a/hermes_cli/auth_commands.py +++ b/hermes_cli/auth_commands.py @@ -217,17 +217,11 @@ def auth_add_command(args) -> None: ca_bundle=getattr(args, "ca_bundle", None), min_key_ttl_seconds=max(60, int(getattr(args, "min_key_ttl_seconds", 5 * 60))), ) - label = (getattr(args, "label", None) or "").strip() or label_from_token( - creds.get("access_token", ""), - _oauth_default_label(provider, len(pool.entries()) + 1), + entry = auth_mod.persist_nous_credentials(creds) + shown_label = entry.label if entry is not None else label_from_token( + creds.get("access_token", ""), _oauth_default_label(provider, 1), ) - auth_mod.persist_nous_credentials( - creds, - label=label, - source=f"{SOURCE_MANUAL}:device_code", - ) - count = len(load_pool(provider).entries()) - print(f'Added {provider} OAuth credential #{count}: "{label}"') + print(f'Saved {provider} OAuth device-code credentials: "{shown_label}"') return if provider == "openai-codex": diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 18dcfa1874..e5f2eb5376 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -1444,13 +1444,8 @@ def _nous_poller(session_id: str) -> None: auth_state, min_key_ttl_seconds=300, timeout_seconds=15.0, force_refresh=False, force_mint=True, ) - from agent.credential_pool import SOURCE_MANUAL from hermes_cli.auth import persist_nous_credentials - persist_nous_credentials( - full_state, - label="dashboard device_code", - source=f"{SOURCE_MANUAL}:dashboard_device_code", - ) + persist_nous_credentials(full_state) with _oauth_sessions_lock: sess["status"] = "approved" _log.info("oauth/device: nous login completed (session=%s)", session_id) diff --git a/tests/hermes_cli/test_auth_commands.py b/tests/hermes_cli/test_auth_commands.py index c67ebeb154..6294526218 100644 --- a/tests/hermes_cli/test_auth_commands.py +++ b/tests/hermes_cli/test_auth_commands.py @@ -141,10 +141,18 @@ def test_auth_add_nous_oauth_persists_pool_entry(tmp_path, monkeypatch): auth_add_command(_Args()) payload = json.loads((tmp_path / "hermes" / "auth.json").read_text()) + + # Pool has exactly one canonical `device_code` entry — not a duplicate + # pair of `manual:device_code` + `device_code` (the latter would be + # materialised by _seed_from_singletons on every load_pool). entries = payload["credential_pool"]["nous"] - entry = next(item for item in entries if item["source"] == "manual:device_code") - assert entry["label"] == "nous@example.com" - assert entry["source"] == "manual:device_code" + device_code_entries = [ + item for item in entries if item["source"] == "device_code" + ] + assert len(device_code_entries) == 1, entries + assert not any(item["source"] == "manual:device_code" for item in entries) + entry = device_code_entries[0] + assert entry["source"] == "device_code" assert entry["agent_key"] == "ak-test" assert entry["portal_base_url"] == "https://portal.example.com" diff --git a/tests/hermes_cli/test_auth_nous_provider.py b/tests/hermes_cli/test_auth_nous_provider.py index c716a903b9..8a54a879ec 100644 --- a/tests/hermes_cli/test_auth_nous_provider.py +++ b/tests/hermes_cli/test_auth_nous_provider.py @@ -495,7 +495,7 @@ def test_persist_nous_credentials_writes_both_pool_and_providers(tmp_path, monke agent failed with "Non-retryable client error". Both stores must stay in sync at write time. """ - from hermes_cli.auth import persist_nous_credentials + from hermes_cli.auth import persist_nous_credentials, NOUS_DEVICE_CODE_SOURCE hermes_home = tmp_path / "hermes" hermes_home.mkdir(parents=True, exist_ok=True) @@ -504,38 +504,35 @@ def test_persist_nous_credentials_writes_both_pool_and_providers(tmp_path, monke })) monkeypatch.setenv("HERMES_HOME", str(hermes_home)) - creds = _full_state_fixture() - entry = persist_nous_credentials( - creds, label="test-label", source="manual:device_code", - ) + entry = persist_nous_credentials(_full_state_fixture()) assert entry is not None assert entry.provider == "nous" + assert entry.source == NOUS_DEVICE_CODE_SOURCE payload = json.loads((hermes_home / "auth.json").read_text()) - # providers.nous populated with the full state (new behavior) + # providers.nous populated with the full state (new behaviour) singleton = payload["providers"]["nous"] assert singleton["access_token"] == "access-tok" assert singleton["refresh_token"] == "refresh-tok" assert singleton["agent_key"] == "agent-key-value" assert singleton["agent_key_expires_at"] == "2026-04-18T22:00:00+00:00" - # credential_pool.nous populated with the pool entry + # credential_pool.nous has exactly one canonical device_code entry pool_entries = payload["credential_pool"]["nous"] - pool_entry = next( - item for item in pool_entries if item["source"] == "manual:device_code" - ) - assert pool_entry["label"] == "test-label" + assert len(pool_entries) == 1, pool_entries + pool_entry = pool_entries[0] + assert pool_entry["source"] == NOUS_DEVICE_CODE_SOURCE assert pool_entry["agent_key"] == "agent-key-value" - assert pool_entry["base_url"] == "https://inference.example.com/v1" + assert pool_entry["inference_base_url"] == "https://inference.example.com/v1" def test_persist_nous_credentials_allows_recovery_from_401(tmp_path, monkeypatch): """End-to-end: after persisting via the helper, resolve_nous_runtime_credentials must succeed (not raise "Hermes is not logged into Nous Portal"). - This is the exact path that ran_agent.py's `_try_refresh_nous_client_credentials` + This is the exact path that run_agent.py's `_try_refresh_nous_client_credentials` calls after a Nous 401 — before the fix it would raise AuthError because providers.nous was empty. """ @@ -548,11 +545,7 @@ def test_persist_nous_credentials_allows_recovery_from_401(tmp_path, monkeypatch })) monkeypatch.setenv("HERMES_HOME", str(hermes_home)) - persist_nous_credentials( - _full_state_fixture(), - label="recovery-test", - source="manual:device_code", - ) + persist_nous_credentials(_full_state_fixture()) # Stub the network-touching steps so we don't actually contact the # portal — the point of this test is that state lookup succeeds and @@ -575,9 +568,17 @@ def test_persist_nous_credentials_allows_recovery_from_401(tmp_path, monkeypatch assert creds["api_key"] == "new-agent-key" -def test_persist_nous_credentials_preserves_existing_providers_entry(tmp_path, monkeypatch): - """Calling persist twice must upsert providers.nous (not duplicate or crash).""" - from hermes_cli.auth import persist_nous_credentials +def test_persist_nous_credentials_idempotent_no_duplicate_pool_entries(tmp_path, monkeypatch): + """Re-running persist must upsert — not accumulate duplicate device_code rows. + + Regression guard for the review comment on PR #11858: before normalisation, + the helper wrote `manual:device_code` while `_seed_from_singletons` wrote + `device_code`, so the pool grew a second duplicate entry on every + ``load_pool()``. The helper now writes providers.nous and lets seeding + materialise the pool entry under the canonical ``device_code`` source, so + two persists still leave the pool with exactly one row. + """ + from hermes_cli.auth import persist_nous_credentials, NOUS_DEVICE_CODE_SOURCE hermes_home = tmp_path / "hermes" hermes_home.mkdir(parents=True, exist_ok=True) @@ -587,12 +588,12 @@ def test_persist_nous_credentials_preserves_existing_providers_entry(tmp_path, m monkeypatch.setenv("HERMES_HOME", str(hermes_home)) first = _full_state_fixture() - persist_nous_credentials(first, label="first", source="manual:device_code") + persist_nous_credentials(first) second = _full_state_fixture() second["access_token"] = "access-second" second["agent_key"] = "agent-key-second" - persist_nous_credentials(second, label="second", source="manual:device_code") + persist_nous_credentials(second) payload = json.loads((hermes_home / "auth.json").read_text()) @@ -600,8 +601,35 @@ def test_persist_nous_credentials_preserves_existing_providers_entry(tmp_path, m assert payload["providers"]["nous"]["access_token"] == "access-second" assert payload["providers"]["nous"]["agent_key"] == "agent-key-second" - # credential_pool.nous has both entries (pool = multi-credential) + # credential_pool.nous has exactly one entry, carrying the latest agent_key pool_entries = payload["credential_pool"]["nous"] - labels = [e.get("label") for e in pool_entries] - assert "first" in labels - assert "second" in labels + assert len(pool_entries) == 1, pool_entries + assert pool_entries[0]["source"] == NOUS_DEVICE_CODE_SOURCE + assert pool_entries[0]["agent_key"] == "agent-key-second" + # And no stray `manual:device_code` / `manual:dashboard_device_code` rows + assert not any( + e["source"].startswith("manual:") for e in pool_entries + ) + + +def test_persist_nous_credentials_reloads_pool_after_singleton_write(tmp_path, monkeypatch): + """The entry returned by the helper must come from a fresh ``load_pool`` so + callers observe the canonical seeded state, including any legacy entries + that ``_seed_from_singletons`` pruned or upserted. + """ + from hermes_cli.auth import persist_nous_credentials, NOUS_DEVICE_CODE_SOURCE + + hermes_home = tmp_path / "hermes" + hermes_home.mkdir(parents=True, exist_ok=True) + (hermes_home / "auth.json").write_text(json.dumps({ + "version": 1, "providers": {}, + })) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + + entry = persist_nous_credentials(_full_state_fixture()) + assert entry is not None + assert entry.source == NOUS_DEVICE_CODE_SOURCE + # Label derived by _seed_from_singletons via label_from_token; we don't + # assert its exact value, just that the helper returned a real entry. + assert entry.access_token == "access-tok" + assert entry.agent_key == "agent-key-value"