refactor(auth): unify Codex credential resolution - #30911
blazing-mj wants to merge 8 commits into
Conversation
Pin default gateway argv with --profile, warn on sticky active_profile redirects, log gateway boot profile drift, add profile gateway status visibility, and harden related fallback/config paths. Verified: targeted pytest slices passed; py_compile passed; static scan clean; Claude Code Max review PASS; dashboard HTTP 200.
Introduces ``agent.auth.codex.resolve_codex_credentials`` — a single function that resolves Codex OAuth credentials via the documented chain: 1. ``HERMES_CODEX_ACCESS_TOKEN`` env override (developer escape hatch). 2. ``~/.hermes/auth.json`` under ``_auth_store_lock`` with optional refresh-when-expiring. 3. ``~/.codex/auth.json`` read-only borrow when the Hermes store has no tokens and ``allow_codex_cli_fallback=True``. The module docstring lays out the ownership contract verbatim — both stores must stay separate, Hermes never writes back to the Codex CLI file, and the borrow path returns only the live access token (the refresh token is intentionally not exposed past the borrow point). The resolver also adds a small double-checked-locking window so concurrent ``force_refresh`` callers serialize into a single HTTP refresh instead of each spending the single-use refresh token. Pure addition: no callers updated in this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…esolver The legacy ``resolve_codex_runtime_credentials`` becomes a thin adapter over ``agent.auth.codex.resolve_codex_credentials`` so historical callers (gateway, cron, runtime_provider, run_agent, account_usage) compile unchanged while transparently inheriting the unified chain — including the Codex CLI borrow fallback that fixes the original symptom. Existing test pins ``CODEX_HOME`` to a tmp path: previously the ``codex_auth_missing_access_token`` test relied on the resolver raising; with the borrow fallback in place the test must explicitly deny the resolver any Codex CLI store to land at to keep its intent intact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…olver ``_read_codex_access_token`` keeps its pool-first short-circuit (pool entries are runtime-aware) and otherwise delegates to the unified resolver — same chain as the main agent path, converted to a silent ``None`` on AuthError so the auxiliary fallback ladder still gets a turn. Behaviour change: when ``~/.hermes/auth.json`` has no Codex tokens but ``~/.codex/auth.json`` does, compression now borrows the Codex CLI access token instead of failing through to OpenRouter/Anthropic. This is the original bug surface — compression was silently paying for the fallback while main chat worked fine. The inline JWT expiry check stays because the resolver is called with ``refresh_if_expiring=False`` (compression should not block on a refresh round-trip), and an already-expired token would 401 anyway. Test pins for ``CODEX_HOME`` on the auxiliary tests prevent the resolver's borrow fallback from escaping to a developer's real ``~/.codex/auth.json`` and masking the assertions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
``_fetch_codex_account_usage`` no longer makes a second ``_read_codex_tokens()`` call to fish out ``account_id``. The unified resolver exposes it directly on ``CodexCredentials``, so the function collapses to a single resolve and reads ``creds.account_id`` off the dataclass. The borrow path also surfaces ``account_id`` from ``~/.codex/auth.json`` opportunistically, so usage reporting keeps working when Hermes is running on a borrowed Codex CLI token. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
``hermes doctor`` now reports which store the unified resolver would pick (``env`` / ``hermes-auth-store`` / ``codex-cli-borrow``). When the source is ``codex-cli-borrow``, doctor emits a WARN line prompting the user to run ``hermes auth login codex`` so they can import the borrowed token into the Hermes store instead of staying dependent on ``~/.codex/auth.json``. Silent borrowing is what this refactor exists to fix — making the diagnostic visible is part of that. Also documents the new ``HERMES_CODEX_ACCESS_TOKEN`` env var in the environment-variables reference: developer escape hatch only, no refresh, no expiry check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
New test module ``tests/agent/test_auth_codex.py`` exercises the matrix the unified resolver promises: A. env override beats Hermes store beats Codex CLI borrow B. only ~/.codex/auth.json populated → main + compression both work C. only ~/.hermes/auth.json populated → both paths succeed D. borrow path never writes to disk (no _save_codex_tokens, no ~/.codex/auth.json mtime change, no ~/.hermes/auth.json mtime change) E. allow_codex_cli_fallback=False rejects the borrow F. two threads with force_refresh serialize into one HTTP refresh under _auth_store_lock + DCL window H. expired Codex CLI borrow is rejected I. env override skips expiry check entirely J. resolve_codex_runtime_credentials shim returns the legacy dict shape under each source branch Also covers account_id propagation from each source and the refresh-failure-doesn't-silently-borrow invariant. Plus a parity case in ``tests/plugins/image_gen/test_openai_codex_provider.py``: when only ``~/.codex/auth.json`` is populated, ``is_available()`` returns True after unification (gap G). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d540e92 to
0e49a48
Compare
|
This PR established the Codex unification pattern that our PR #62467 extends to all 19+ providers. The resolve_provider_credentials() function follows the same architecture: single entry point, shared by both auxiliary_client.py and runtime_provider.py. Credit for the original pattern. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for documenting the ownership model and covering the auxiliary path. The underlying divergence still exists on current main: agent/auxiliary_client.py:1791-1822 reads the Hermes store after its pool path, while the main resolver can recover from Codex CLI credentials at hermes_cli/auth.py:3655-3669.
Problems
- The shim at PR
hermes_cli/auth.py:3365-3369replaces current main's resolver with a chain that has no credential-pool branch. Current main explicitly preserves pool-only Codex operation athermes_cli/auth.py:3673-3687(commit69dfcdcc15f71ba5ee243bc192365629b9b9b85c). A salvage must retain that behavior. - The PR also includes unrelated gateway/profile and Gemini changes, which should not be coupled to this auth fix.
Suggested changes
- Salvage the Codex portion against current main with explicit precedence and regression coverage for pool-only, Hermes-store, and Codex-CLI cases.
- Split the unrelated changes for independent review.
Automated hermes-sweeper review.
| "last_refresh": data.get("last_refresh"), | ||
| "auth_mode": "chatgpt", | ||
| } | ||
| return resolve_codex_credentials( |
There was a problem hiding this comment.
Current main's resolver now falls back to credential_pool.openai-codex when the singleton is empty (hermes_cli/auth.py:3673-3687, commit 69dfcdcc15f71ba5ee243bc192365629b9b9b85c). The unified resolver needs to preserve that branch before this shim replaces the current implementation.
refactor(auth): unify Codex credential resolution
Summary
Two different Codex credential resolvers lived in the same process: the
main agent path raised loudly when the Hermes auth store was empty, while
the auxiliary/compression path silently returned
Noneand let thefallback ladder route to Anthropic (paying metered API costs without a
warning). This PR introduces a single resolver —
agent.auth.codex.resolve_codex_credentials— that both paths now go through, with an explicit chain: env override →
Hermes auth store → read-only borrow from
~/.codex/auth.json.Problem
Before this PR there were two divergent runtime resolvers for the same
provider (
openai-codex) inside the same process. FromSCOPING.md §1:hermes_cli/auth.py:3132—resolve_codex_runtime_credentials()reads
~/.hermes/auth.jsononly and raisesAuthErroron miss. Loud.agent/auxiliary_client.py:1333—_read_codex_access_token()reads the Hermes auth store and returnsNoneon miss. Silent.Neither path borrowed from
~/.codex/auth.jsonat runtime. The onlyCodex-CLI import was the interactive prompt inside
_login_openai_codex.User-visible symptom: a user whose Codex credentials live only in
~/.codex/auth.json(because they came in via Codex CLI / VS Codeextension and never ran
hermes auth login codex) would see the mainagent fail loudly with an actionable re-auth message — but compression
would silently fall through the auxiliary chain to OpenRouter, then Nous,
then Anthropic, and start paying metered Anthropic API costs without any
warning. They had no signal that compression was no longer running on
their ChatGPT subscription.
Solution
A single unified resolver,
agent.auth.codex.resolve_codex_credentials,walks the same chain regardless of caller. Both the main agent and the
compression/auxiliary path go through it. The legacy
resolve_codex_runtime_credentialsbecomes a thin shim that returnsthe same dict shape so existing callers compile unchanged.
The auxiliary path keeps its pool-first short-circuit (pool entries are
runtime-aware) and converts the resolver's
AuthErrorinto a silentNoneitself, so the existing auxiliary fallback ladder still gets aturn when truly nothing is configured. But when
~/.codex/auth.jsonispopulated, compression now borrows from it instead of falling through to
Anthropic.
Ownership contract
Restated verbatim from the top of
agent/auth/codex.py:This is the load-bearing design decision; the PR exists to protect it.
Changes
Six commits, intentionally split so each is reviewable on its own:
feat(auth): add unified Codex credential resolver(f578883) —introduces
agent/auth/codex.pywith theresolve_codex_credentialsfunction,
CodexCredentialsdataclass, double-checked-locking windowfor concurrent
force_refreshcallers, and the ownership-contractdocstring. Pure addition: no callers updated in this commit.
refactor(auth): shim resolve_codex_runtime_credentials onto unified resolver(
b554fc3) — replaces the legacy resolver body with a one-lineadapter over the unified resolver. Gateway, cron, runtime_provider,
run_agent, account_usage compile unchanged but transparently inherit
the borrow fallback.
refactor(auxiliary): route compression Codex auth through unified resolver(
f7a46b6) —_read_codex_access_tokenkeeps its pool-firstshort-circuit and otherwise delegates to the unified resolver. This
is where the original bug surface is fixed.
refactor(account-usage): pull account_id from unified resolver(
72916e1) —_fetch_codex_account_usagedrops its second_read_codex_tokens()call;account_idis now exposed onCodexCredentials.account_id(also for the borrow path).feat(doctor): surface Codex credential source(ce6f6a4) —hermes doctornow reportsSource: <env|hermes-auth-store|codex-cli-borrow>and emits a WARN line when on the borrow path, prompting the user to
run
hermes auth login codex. Also documentsHERMES_CODEX_ACCESS_TOKENin
website/docs/reference/environment-variables.md.test(auth): cover unified Codex resolver chain(d540e92) —17 new tests in
tests/agent/test_auth_codex.pycovering scenariosA–J from the Phase 2 spec; parity case in
tests/plugins/image_gen/test_openai_codex_provider.py.Tests
tests/agent/test_auth_codex.pycovering theresolver matrix:
~/.codex/auth.jsonpopulated → main + compression both work~/.hermes/auth.jsonpopulated → both paths succeed_save_codex_tokens,no mtime change on either auth file)
allow_codex_cli_fallback=Falserejects the borrowforce_refreshserialize into one HTTPrefresh under
_auth_store_lock+ DCL windowresolve_codex_runtime_credentialsshim returns the legacydict shape under each source branch
account_idpropagation from each source and therefresh-failure-doesn't-silently-borrow invariant.
tests/agent/test_auxiliary_client.py(+21 lines) —CODEX_HOMEpin so the resolver's borrow fallback doesn't escape to a
developer's real
~/.codex/auth.jsonand mask assertions.tests/test_account_usage.py(+20 lines) — account_id surfaces fromthe unified resolver.
tests/plugins/image_gen/test_openai_codex_provider.py(+38 lines)— scenario G: only
~/.codex/auth.jsonpopulated →is_available()returns True after unification.
tests/hermes_cli/test_auth_codex_provider.py(+4 lines) — pinCODEX_HOMEoncodex_auth_missing_access_tokenso the borrowfallback can't satisfy the test's intent.
documented under "Pre-existing flakes" below.
scripts/run_tests.sh):23,815 pass / 75 skipped / 44 failed / 0 new regressions caused by this
diff. Same suite on
mainproduces 23,799 pass / 75 skipped / 42failed — the same 42 failures appear on both branches (pre-existing,
documented below). The 16-test delta in passes corresponds to the 17
new tests in this PR minus the two order-dependent flakes; both extra
failures pass when run in isolation and neither test references
Codex code.
Breaking changes
None. The legacy
resolve_codex_runtime_credentialsfunction ispreserved as a thin shim with the identical signature and dict return
shape; all existing callers continue to work without modification.
New env var
HERMES_CODEX_ACCESS_TOKEN— developer escape hatch. When set, theresolver returns it verbatim and skips both auth stores, refresh, and
the expiry check. Strictly additive: leaving it unset preserves current
behaviour exactly. Documented in
website/docs/reference/environment-variables.md.Doctor surfacing
hermes doctorgains one new diagnostic line in the OpenAI Codex authsection:
Source: <hermes-auth-store|codex-cli-borrow|env>. When theresolver picks
codex-cli-borrow, doctor additionally emits a WARN lineexplaining that Hermes is running on a borrowed Codex CLI access token
and recommending the user run
hermes auth login codexto import thetoken into the Hermes store. Silent borrowing is what this refactor
exists to fix — making the diagnostic visible is part of that.
Pre-existing flakes documented
Reviewers can ignore the following — every test in this section fails on
main(1c78b7a21) underscripts/run_tests.shfrom the same machine,i.e. they have nothing to do with this PR. Most are platform-coupled
tests (systemd / WSL on macOS) or order-dependent xdist flakes.
42 failures that occur on both
mainand this branch:tests/agent/test_anthropic_adapter.py— 14 tests inTestResolveAnthropicToken,TestResolveWithRefresh,TestRunOauthSetupToken. Unrelated to Codex; touches Anthropic OAuthresolution which this PR does not modify.
tests/hermes_cli/test_gateway_service.py— 6 tests inTestSystemdServiceRefresh/TestGatewaySystemServiceRouting.systemd-coupled; fails on macOS hosts.
tests/hermes_cli/test_gateway_wsl.py— 2 tests inTestSupportsSystemdServicesWSL. WSL-coupled.tests/gateway/test_shutdown_forensics.py— 1 testtest_spawns_subprocess_and_writes_output(async subprocessdiagnostic timing).
tests/hermes_cli/test_aux_config.py— 2 tests(
test_session_search_defaults_include_extra_body_and_concurrency,test_aux_tasks_keys_all_exist_in_default_config). Config defaultsdrift, unrelated.
tests/test_tui_gateway_server.py—test_browser_manage_connect_default_local_reports_launch_hint.tests/test_live_system_guard_self_test.py— 4test_systemctl_*tests. systemd-coupled.
tests/tools/test_file_read_guards.py— 6 tests inTestFileDedup,TestWriteInvalidatesDedup.tests/tools/test_file_staleness.py— 3 tests inTestStalenessCheck/
TestPatchStaleness.tests/tools/test_file_state_registry.py— 2 tests inFileToolsIntegrationTests.tests/tools/test_delegate.py—TestDelegateHeartbeat::test_heartbeat_does_not_trip_idle_stale_while_inside_tool.Order-dependent xdist flakes (fail on this branch's full-suite run,
pass on this branch in isolation, did not fail on the main full-suite
run but only because the worker assignment shifted):
tests/hermes_cli/test_image_gen_picker.py::TestConfigPrompt::test_image_gen_still_prompts_when_nothing_availabletests/hermes_cli/test_web_server.py::TestPtyWebSocket::test_pub_broadcasts_to_events_subscribersBoth files contain zero references to Codex resolution paths; the diff
in this PR does not touch any code those tests exercise. They pass when
re-run alone with
scripts/run_tests.sh <path>. Adding 17 new tests tothe suite shifted xdist's per-worker assignments enough that they ended
up sharing a worker with whatever existing tests they leak state with.
Known pre-existing flake from the task brief — did NOT fire this run:
tests/agent/test_auxiliary_client.py::TestCodexAuxiliaryAdapterTimeout::test_enforces_total_timeout_while_stream_keeps_emitting_eventsis documented as a flaky timing assertion (
elapsed < 0.14s) buthappened to pass on both runs above. Listed for completeness.
Reviewer notes
in this process. Future per-provider unifiers (Anthropic, xAI) can
follow the same pattern in
agent/auth/<provider>.py. The newagent/auth/package exists for exactly this reason._auth_store_lock(cross-processfcntl.flockon POSIX,msvcrt.lockingon Windows, withthread-reentrancy via a
threading.localdepth counter) is reusedunchanged. No new locking added; see
SCOPING.md §8for the fullaudit. The borrow path holds no lock because it only reads Codex
CLI's file — Codex CLI's atomic-write semantics are out of Hermes'
scope.
SCOPING.mdwere answeredyes / yes / yes / keep-shim by the project maintainer and are
reflected in the implementation:
account_idis exposed onCodexCredentials.account_id.HERMES_CODEX_ACCESS_TOKENis wired in as resolution step 1.sourceand emits a WARN on borrow.resolve_codex_runtime_credentialskept as a permanentbackwards-compatible shim — no follow-up rename planned.