From 761b744abbc621abad3fb177cd50dc1d5666bb68 Mon Sep 17 00:00:00 2001 From: Ted Malone Date: Thu, 4 Jun 2026 10:41:05 -0700 Subject: [PATCH] fix(auth): preserve independent Codex pool entries on re-auth (#39236) The #33538 fix refreshed every credential_pool entry with source "manual:device_code" on every Codex OAuth re-auth, on the assumption that such entries were always legacy aliases of the singleton from the #33000 workaround era. That assumption is no longer true: `hermes auth add openai-codex` also produces "manual:device_code" entries for independent ChatGPT accounts, and the broad sync silently clobbered them with the latest-authenticated token pair (labels preserved, token material overwritten, status / quota readings then lie). Narrow the sync: refresh a "manual:device_code" entry only when its existing access_token matches the previous singleton access_token (true legacy alias). Entries with distinct token material represent independent accounts and are now left alone. Error markers are cleared only on entries actually rewritten, so an independent account's own 429 / 401 state survives a re-auth that targeted a different account. Tests: * New: independent acctB/acctC are not overwritten when acctA re-auths. * New: legacy singleton-alias still refreshed (preserves #33538). * New: missing previous singleton state handled (no crash, no false alias match). * New: access_token-only alias match (legacy schema without refresh_token still recognized). * New: error markers cleared only on entries actually refreshed. * Updated: existing manual-device-code sync test now covers both the legacy-alias path AND the independent-account path in one fixture. Behaviour change is zero for users with a single Codex account and zero for users whose only "manual:device_code" entry is the legacy alias of the singleton. Users with multiple independent Codex accounts added via `hermes auth add` now keep their distinct token material across re-auths. Local: 29 passed in tests/hermes_cli/test_auth_codex_provider.py, no new failures in tests/hermes_cli/ vs upstream/main baseline. Fixes #39236. --- hermes_cli/auth.py | 75 +++- tests/hermes_cli/test_auth_codex_provider.py | 396 +++++++++++++++++-- 2 files changed, 431 insertions(+), 40 deletions(-) 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)