diff --git a/hermes_cli/auth.py b/hermes_cli/auth.py index cfd4ad5f8a6..954a948f0e3 100644 --- a/hermes_cli/auth.py +++ b/hermes_cli/auth.py @@ -3355,6 +3355,7 @@ def _sync_codex_pool_entries( auth_store: Dict[str, Any], tokens: Dict[str, str], last_refresh: Optional[str], + previous_singleton_tokens: Optional[Dict[str, str]] = None, ) -> None: """Mirror a fresh Codex re-auth into the credential_pool OAuth entries. @@ -3370,24 +3371,34 @@ def _sync_codex_pool_entries( OAuth flow when the user logged in via ``hermes setup`` / the model picker. Always synced with the fresh tokens. * ``manual:device_code`` — entries created by ``hermes auth add openai-codex`` - that use the same device-code OAuth mechanism. An interactive re-auth - proves the user owns the ChatGPT account, so it is safe (and expected) - to refresh these entries too. Without this, a user who once ran the - ``hermes auth add`` workaround for #33000 would silently leave that - manual entry stale on every subsequent re-auth, recreating the issue - reported in #33538. + that use the same device-code OAuth mechanism. ONLY synced if the + entry's existing access_token matches the *previous* singleton + access_token (i.e. the entry is a legacy singleton-alias from the + #33000 workaround era). Manual entries whose tokens never matched the + singleton represent INDEPENDENT accounts added via + ``hermes auth add openai-codex`` and must not be overwritten by a + re-auth that targeted a different account (regression for #39236). + + The original #33538 fix refreshed every ``manual:device_code`` entry + unconditionally. That worked when ``manual:device_code`` only meant + "legacy alias of the singleton", but the same source string is now + also produced by independent-account additions, and the broad sync + silently clobbered distinct accounts with the latest-authenticated + token pair. The access_token-match check distinguishes the two cases + without changing the source-string contract. What does NOT get refreshed: * ``manual:api_key`` and any other non-device-code manual sources — those are independent credentials (an explicit API key, a different ChatGPT account, etc.) and must not be overwritten by a single re-auth. + * ``manual:device_code`` entries whose access_token does NOT match the + previous singleton — see above; these are independent accounts. - Error markers (``last_status``, ``last_error_*``) are also cleared on - every device-code-backed entry — even those whose tokens we did not - rewrite — so that an interactive re-auth gives every relevant pool entry - a fresh selection chance instead of leaving them marked unhealthy from a - pre-re-auth 401. + Error markers (``last_status``, ``last_error_*``) are cleared ONLY on + entries that actually had their tokens rewritten by this re-auth. + Independent entries keep their own error state (their 401/429 markers + belong to that account's own auth flow, not this re-auth). """ access_token = tokens.get("access_token") if not access_token: @@ -3399,15 +3410,34 @@ def _sync_codex_pool_entries( entries = pool.get("openai-codex") if not isinstance(entries, list): return - # Sources whose tokens should be rewritten by a fresh Codex device-code - # OAuth re-auth. ``manual:api_key`` and unknown sources are intentionally - # excluded — they represent independent credentials. - REFRESHABLE_SOURCES = {"device_code", "manual:device_code"} + # Previous singleton access_token (before this re-auth overwrote it) — + # used to distinguish legacy singleton-aliases from independent accounts. + # When None or empty, no manual entry can be treated as an alias (which + # is the right default for first-ever-save or a freshly initialized + # auth.json). + prev_at = None + if isinstance(previous_singleton_tokens, dict): + prev_at = previous_singleton_tokens.get("access_token") or None for entry in entries: if not isinstance(entry, dict): continue source = entry.get("source") - if source not in REFRESHABLE_SOURCES: + if source == "device_code": + # Singleton-seeded mirror — always refresh. + refresh_this_entry = True + elif source == "manual:device_code": + # Refresh only if this entry's existing access_token matches the + # previous singleton access_token (i.e. it is a true alias of the + # singleton from the #33000 workaround era). An entry with its + # own distinct token material is an independent account and must + # be left alone (#39236). + refresh_this_entry = bool( + prev_at and entry.get("access_token") == prev_at + ) + else: + # ``manual:api_key`` and any future non-device-code sources. + refresh_this_entry = False + if not refresh_this_entry: continue entry["access_token"] = access_token if refresh_token: @@ -3429,13 +3459,24 @@ def _save_codex_tokens(tokens: Dict[str, str], last_refresh: str = None, label: with _auth_store_lock(): auth_store = _load_auth_store() state = _load_provider_state(auth_store, "openai-codex") or {} + # Capture the previous singleton tokens BEFORE overwriting them. The + # pool-sync step uses this to distinguish legacy singleton-aliases + # (which should be refreshed) from independent accounts that + # ``hermes auth add openai-codex`` created (which must not be + # overwritten — see #39236). + previous_singleton_tokens = state.get("tokens") if isinstance(state.get("tokens"), dict) else None state["tokens"] = tokens state["last_refresh"] = last_refresh state["auth_mode"] = "chatgpt" if label and str(label).strip(): state["label"] = str(label).strip() _save_provider_state(auth_store, "openai-codex", state) - _sync_codex_pool_entries(auth_store, tokens, last_refresh) + _sync_codex_pool_entries( + auth_store, + tokens, + last_refresh, + previous_singleton_tokens=previous_singleton_tokens, + ) _save_auth_store(auth_store) diff --git a/tests/hermes_cli/test_auth_codex_provider.py b/tests/hermes_cli/test_auth_codex_provider.py index 52a8a4a2c45..cb85cf6818e 100644 --- a/tests/hermes_cli/test_auth_codex_provider.py +++ b/tests/hermes_cli/test_auth_codex_provider.py @@ -301,19 +301,23 @@ def test_save_codex_tokens_syncs_credential_pool(tmp_path, monkeypatch): def test_save_codex_tokens_syncs_manual_device_code_entries(tmp_path, monkeypatch): - """Re-auth must also refresh ``manual:device_code`` pool entries. + """Re-auth must refresh ``manual:device_code`` entries that are true + aliases of the singleton, while leaving INDEPENDENT entries alone. - Regression for #33538: a user who hit #33000 before the #33164 fix landed - would have run ``hermes auth add openai-codex`` as a workaround, leaving - a pool entry with ``source="manual:device_code"``. On every subsequent - re-auth via setup/model picker, the singleton-seeded ``device_code`` entry - got refreshed but the ``manual:device_code`` entry stayed stale, recreating - the same 401 token_invalidated symptom that #33164 was supposed to fix. + Original regression for #33538: a user who hit #33000 before the #33164 + fix landed would have run ``hermes auth add openai-codex`` as a + workaround, leaving a pool entry with ``source="manual:device_code"``. + On every subsequent re-auth via setup/model picker, the singleton-seeded + ``device_code`` entry got refreshed but the ``manual:device_code`` entry + stayed stale, recreating the same 401 token_invalidated symptom that + #33164 was supposed to fix. - An interactive Codex device-code re-auth proves the user owns the ChatGPT - account, so it is safe to refresh every device-code-backed entry in the - pool — but NOT independent ``manual:api_key`` entries (separate accounts / - explicit API keys). + Narrowed for #39236: the original fix treated every ``manual:device_code`` + entry as a singleton-alias and refreshed them all, which silently + clobbered independent accounts added via ``hermes auth add openai-codex``. + The current behavior refreshes only entries whose access_token matches + the *previous* singleton access_token (true legacy aliases), and leaves + distinct-token entries alone (independent accounts). """ hermes_home = tmp_path / "hermes" hermes_home.mkdir(parents=True, exist_ok=True) @@ -335,16 +339,30 @@ def test_save_codex_tokens_syncs_manual_device_code_entries(tmp_path, monkeypatc "access_token": "old-at", "refresh_token": "old-rt", }, + # Legacy alias from the #33000 workaround era — its tokens + # match the singleton, so it is a true alias and SHOULD be + # refreshed (preserves #33538 behavior). { - "id": "auth-add", + "id": "legacy-alias", "source": "manual:device_code", "auth_type": "oauth", - "access_token": "stale-manual-at", - "refresh_token": "stale-manual-rt", + "access_token": "old-at", + "refresh_token": "old-rt", "last_status": "exhausted", "last_error_code": 401, "last_error_reason": "token_invalidated", }, + # Independent account from `hermes auth add openai-codex` — + # its tokens are distinct from the singleton. Must NOT be + # overwritten by a re-auth that targeted a different account + # (#39236). + { + "id": "independent", + "source": "manual:device_code", + "auth_type": "oauth", + "access_token": "independent-at", + "refresh_token": "independent-rt", + }, { "id": "api-key", "source": "manual:api_key", @@ -363,18 +381,23 @@ def test_save_codex_tokens_syncs_manual_device_code_entries(tmp_path, monkeypatc pool = auth["credential_pool"]["openai-codex"] # Singleton-seeded device_code entry: refreshed and error markers cleared. - seeded = next(e for e in pool if e["source"] == "device_code") + seeded = next(e for e in pool if e["id"] == "seeded") assert seeded["access_token"] == "fresh-at" assert seeded["refresh_token"] == "fresh-rt" - # manual:device_code entry: ALSO refreshed (the new behavior). - manual_dc = next(e for e in pool if e["source"] == "manual:device_code") - assert manual_dc["access_token"] == "fresh-at" - assert manual_dc["refresh_token"] == "fresh-rt" - assert manual_dc["last_refresh"] == "2026-05-28T00:00:00Z" - assert manual_dc["last_status"] is None - assert manual_dc["last_error_code"] is None - assert manual_dc["last_error_reason"] is None + # Legacy alias (tokens matched previous singleton): ALSO refreshed. + legacy = next(e for e in pool if e["id"] == "legacy-alias") + assert legacy["access_token"] == "fresh-at" + assert legacy["refresh_token"] == "fresh-rt" + assert legacy["last_refresh"] == "2026-05-28T00:00:00Z" + assert legacy["last_status"] is None + assert legacy["last_error_code"] is None + assert legacy["last_error_reason"] is None + + # Independent manual:device_code entry: NOT overwritten (#39236). + independent = next(e for e in pool if e["id"] == "independent") + assert independent["access_token"] == "independent-at" + assert independent["refresh_token"] == "independent-rt" # manual:api_key entry: untouched — independent credential. api_key = next(e for e in pool if e["source"] == "manual:api_key") @@ -382,6 +405,333 @@ def test_save_codex_tokens_syncs_manual_device_code_entries(tmp_path, monkeypatc assert "refresh_token" not in api_key or api_key.get("refresh_token") is None +def test_save_codex_tokens_does_not_overwrite_independent_manual_entries(tmp_path, monkeypatch): + """Re-auth must NOT overwrite ``manual:device_code`` entries that hold + independent token material (different OpenAI/ChatGPT accounts). + + Regression for #39236: ``hermes auth add openai-codex`` for accounts B and C + routes through ``_save_codex_tokens`` because the singleton path is the + only Codex OAuth save flow. The #33538 fix refreshed every + ``manual:device_code`` entry on every re-auth, which works fine for the + one-account/legacy-workaround case but silently overwrote distinct + independent accounts with the latest-authenticated tokens (labels + preserved, token material clobbered, status/quota readings then lie). + + The safe invariant: an entry is a singleton-alias only when its current + access_token matches the *previous* singleton access_token. Manual + entries whose tokens never matched the singleton are independent accounts + and must be left alone. + """ + hermes_home = tmp_path / "hermes" + hermes_home.mkdir(parents=True, exist_ok=True) + (hermes_home / "auth.json").write_text(json.dumps({ + "version": 1, + "providers": { + "openai-codex": { + # Old singleton tokens — represent "account A" which the user + # logged in with via setup originally. + "tokens": {"access_token": "acctA-at", "refresh_token": "acctA-rt"}, + "last_refresh": "2026-01-01T00:00:00Z", + "auth_mode": "chatgpt", + "label": "account-A", + }, + }, + "credential_pool": { + "openai-codex": [ + # The seeded singleton mirror of account A. + { + "id": "seeded", + "label": "account-A", + "source": "device_code", + "auth_type": "oauth", + "access_token": "acctA-at", + "refresh_token": "acctA-rt", + }, + # Two INDEPENDENT manual entries added later via + # ``hermes auth add openai-codex`` (account B and account C). + # Each has its OWN distinct token material, unrelated to the + # singleton. + { + "id": "acctB", + "label": "account-B", + "source": "manual:device_code", + "auth_type": "oauth", + "access_token": "acctB-at", + "refresh_token": "acctB-rt", + }, + { + "id": "acctC", + "label": "account-C", + "source": "manual:device_code", + "auth_type": "oauth", + "access_token": "acctC-at", + "refresh_token": "acctC-rt", + }, + ], + }, + })) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + + # User re-authenticates account A — fresh device-code login produces new + # tokens. The legitimate update is the seeded singleton mirror; the + # independent acctB/acctC entries must be untouched. + _save_codex_tokens( + {"access_token": "acctA-new-at", "refresh_token": "acctA-new-rt"}, + last_refresh="2026-06-05T00:00:00Z", + ) + + auth = json.loads((hermes_home / "auth.json").read_text()) + pool = auth["credential_pool"]["openai-codex"] + + # Singleton-seeded entry: refreshed (legitimate sync). + seeded = next(e for e in pool if e["source"] == "device_code") + assert seeded["access_token"] == "acctA-new-at" + assert seeded["refresh_token"] == "acctA-new-rt" + assert seeded["last_refresh"] == "2026-06-05T00:00:00Z" + + # acctB: INDEPENDENT entry — must NOT be overwritten. + acctB = next(e for e in pool if e["id"] == "acctB") + assert acctB["access_token"] == "acctB-at", ( + "acctB was clobbered by acctA re-auth (#39236 regression)" + ) + assert acctB["refresh_token"] == "acctB-rt" + + # acctC: INDEPENDENT entry — must NOT be overwritten. + acctC = next(e for e in pool if e["id"] == "acctC") + assert acctC["access_token"] == "acctC-at", ( + "acctC was clobbered by acctA re-auth (#39236 regression)" + ) + assert acctC["refresh_token"] == "acctC-rt" + + +def test_save_codex_tokens_still_refreshes_legacy_manual_alias(tmp_path, monkeypatch): + """The #33538 legacy use case must keep working. + + A user who hit #33000 before the #33164 fix landed might have run + ``hermes auth add openai-codex`` as a workaround when there was no + singleton entry — that created a ``manual:device_code`` pool entry that + holds the SAME token material as the (later) singleton. This entry is a + true alias of the singleton and SHOULD still be refreshed on subsequent + re-auths, otherwise it goes stale and recreates the #33538 symptom. + + The distinguishing signal: a legacy alias has access_token == previous + singleton access_token; an independent account does not. + """ + hermes_home = tmp_path / "hermes" + hermes_home.mkdir(parents=True, exist_ok=True) + (hermes_home / "auth.json").write_text(json.dumps({ + "version": 1, + "providers": { + "openai-codex": { + "tokens": {"access_token": "shared-at", "refresh_token": "shared-rt"}, + "last_refresh": "2026-01-01T00:00:00Z", + "auth_mode": "chatgpt", + }, + }, + "credential_pool": { + "openai-codex": [ + { + "id": "seeded", + "source": "device_code", + "auth_type": "oauth", + "access_token": "shared-at", + "refresh_token": "shared-rt", + }, + { + "id": "legacy", + "label": "legacy-alias", + "source": "manual:device_code", + "auth_type": "oauth", + # Token material matches the singleton — this is a true + # alias from the #33000 workaround era. + "access_token": "shared-at", + "refresh_token": "shared-rt", + "last_status": "exhausted", + "last_error_code": 401, + "last_error_reason": "token_invalidated", + }, + ], + }, + })) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + + _save_codex_tokens( + {"access_token": "fresh-at", "refresh_token": "fresh-rt"}, + last_refresh="2026-06-05T00:00:00Z", + ) + + auth = json.loads((hermes_home / "auth.json").read_text()) + pool = auth["credential_pool"]["openai-codex"] + + # Singleton: refreshed. + seeded = next(e for e in pool if e["source"] == "device_code") + assert seeded["access_token"] == "fresh-at" + + # Legacy alias: still refreshed (preserves #33538 fix). + legacy = next(e for e in pool if e["id"] == "legacy") + assert legacy["access_token"] == "fresh-at" + assert legacy["refresh_token"] == "fresh-rt" + assert legacy["last_refresh"] == "2026-06-05T00:00:00Z" + # Error markers cleared on the refreshed entry. + assert legacy["last_status"] is None + assert legacy["last_error_code"] is None + assert legacy["last_error_reason"] is None + + +def test_save_codex_tokens_handles_missing_previous_singleton_tokens(tmp_path, monkeypatch): + """First-ever Codex save (no prior singleton tokens) must not crash. + + Edge case: a user has only pool entries (e.g. via direct auth.json edit + or a partial state from a corrupted upgrade), no `providers.openai-codex.tokens` + block at all. The previous-singleton-tokens guard must handle missing + state gracefully — fall back to "no previous tokens", which means no + pool entry can be a true alias and only the singleton-seeded entry gets + written. + """ + hermes_home = tmp_path / "hermes" + hermes_home.mkdir(parents=True, exist_ok=True) + (hermes_home / "auth.json").write_text(json.dumps({ + "version": 1, + "providers": {}, + "credential_pool": { + "openai-codex": [ + { + "id": "preexisting", + "label": "pre-existing-manual", + "source": "manual:device_code", + "auth_type": "oauth", + "access_token": "preexisting-at", + "refresh_token": "preexisting-rt", + }, + ], + }, + })) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + + _save_codex_tokens( + {"access_token": "first-at", "refresh_token": "first-rt"}, + last_refresh="2026-06-05T00:00:00Z", + ) + + auth = json.loads((hermes_home / "auth.json").read_text()) + pool = auth["credential_pool"]["openai-codex"] + # Pre-existing independent entry with no relationship to a (now-new) + # singleton MUST be preserved. + pre = next(e for e in pool if e["id"] == "preexisting") + assert pre["access_token"] == "preexisting-at" + assert pre["refresh_token"] == "preexisting-rt" + + +def test_save_codex_tokens_alias_match_uses_access_token_only(tmp_path, monkeypatch): + """A manual entry counts as an alias if its access_token matches the + previous singleton access_token, regardless of refresh_token presence. + + Some legacy entries (older auth.json schemas, pre-refresh-token versions) + have access_token but no refresh_token. These should still be treated as + aliases when the access_token matches. + """ + hermes_home = tmp_path / "hermes" + hermes_home.mkdir(parents=True, exist_ok=True) + (hermes_home / "auth.json").write_text(json.dumps({ + "version": 1, + "providers": { + "openai-codex": { + "tokens": {"access_token": "shared-at", "refresh_token": "shared-rt"}, + "auth_mode": "chatgpt", + }, + }, + "credential_pool": { + "openai-codex": [ + { + "id": "alias-no-refresh", + "source": "manual:device_code", + "auth_type": "oauth", + "access_token": "shared-at", + # No refresh_token at all — legacy schema. + }, + ], + }, + })) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + + _save_codex_tokens( + {"access_token": "new-at", "refresh_token": "new-rt"}, + last_refresh="2026-06-05T00:00:00Z", + ) + + auth = json.loads((hermes_home / "auth.json").read_text()) + pool = auth["credential_pool"]["openai-codex"] + alias = next(e for e in pool if e["id"] == "alias-no-refresh") + # Treated as alias → refreshed with new tokens. + assert alias["access_token"] == "new-at" + assert alias["refresh_token"] == "new-rt" + + +def test_save_codex_tokens_clears_error_markers_only_on_refreshed_entries(tmp_path, monkeypatch): + """Error markers must be cleared only on entries that were actually + refreshed by this re-auth. Independent ``manual:device_code`` entries + with their own stale-error markers must be left alone (their stale state + is not the current re-auth's business). + """ + hermes_home = tmp_path / "hermes" + hermes_home.mkdir(parents=True, exist_ok=True) + (hermes_home / "auth.json").write_text(json.dumps({ + "version": 1, + "providers": { + "openai-codex": { + "tokens": {"access_token": "acctA-at", "refresh_token": "acctA-rt"}, + "auth_mode": "chatgpt", + }, + }, + "credential_pool": { + "openai-codex": [ + { + "id": "seeded", + "source": "device_code", + "auth_type": "oauth", + "access_token": "acctA-at", + "refresh_token": "acctA-rt", + "last_status": "exhausted", + "last_error_code": 401, + }, + { + "id": "acctB", + "source": "manual:device_code", + "auth_type": "oauth", + "access_token": "acctB-at", + "refresh_token": "acctB-rt", + "last_status": "exhausted", + "last_error_code": 429, + "last_error_reason": "quota_exhausted", + }, + ], + }, + })) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + + _save_codex_tokens( + {"access_token": "fresh-at", "refresh_token": "fresh-rt"}, + last_refresh="2026-06-05T00:00:00Z", + ) + + auth = json.loads((hermes_home / "auth.json").read_text()) + pool = auth["credential_pool"]["openai-codex"] + + # Singleton: refreshed AND error markers cleared. + seeded = next(e for e in pool if e["id"] == "seeded") + assert seeded["access_token"] == "fresh-at" + assert seeded["last_status"] is None + assert seeded["last_error_code"] is None + + # Independent acctB: NOT refreshed AND error markers NOT cleared. + # (Its 429 quota state belongs to acctB's own account, not acctA's re-auth.) + acctB = next(e for e in pool if e["id"] == "acctB") + assert acctB["access_token"] == "acctB-at" # not overwritten + assert acctB["last_status"] == "exhausted" # not cleared + assert acctB["last_error_code"] == 429 + assert acctB["last_error_reason"] == "quota_exhausted" + + def test_import_codex_cli_tokens(tmp_path, monkeypatch): codex_home = tmp_path / "codex-cli" codex_home.mkdir(parents=True, exist_ok=True)