Skip to content

test(credential_pool): align with .env-first seed precedence - #19021

Closed
Sanjays2402 wants to merge 1 commit into
NousResearch:mainfrom
Sanjays2402:fix/main-ci-credential-pool-dotenv-precedence-test
Closed

Sanjays2402 wants to merge 1 commit into
NousResearch:mainfrom
Sanjays2402:fix/main-ci-credential-pool-dotenv-precedence-test

Conversation

@Sanjays2402

Copy link
Copy Markdown
Contributor

Summary

Fixes one Tests failure observed on main (and therefore propagating to every open PR):

FAILED tests/tools/test_credential_pool_env_fallback.py::TestCredentialPoolSeedsFromDotEnv::test_os_environ_still_wins_over_dotenv
  AssertionError: assert 'sk-dotenv-stale' == 'sk-env-fresh-xyz'

Reference run: 25250051126 on 5d3be898a.

Root cause

The test docstring says:

get_env_value checks os.environ first — verify seeding picks that up.

…which was true for _seed_from_env until commit 2ef1ad280 ("fix: prefer ~/.hermes/.env over os.environ when seeding credential pool", fixes #18254). That commit introduced a private _get_env_prefer_dotenv helper and deliberately flipped the precedence:

# Prefer ~/.hermes/.env over os.environ — the user's config file is the
# authoritative source for Hermes credentials. Stale env vars from parent
# processes (Codex CLI, test scripts, etc.) should not override deliberate
# changes to the .env file.
def _get_env_prefer_dotenv(key: str) -> str:
    env_file = load_env()
    val = env_file.get(key) or os.environ.get(key) or ""
    return val.strip()

The fix was correct (#18254 reported real users hitting silent 401s with stale auth.json caches), but the unit test was never updated to match.

Fix

Rename + rewrite the test to assert the current, deliberate behaviour: .env wins over os.environ, with a docstring explaining why (stale shell env vars from parent processes shadowing deliberate .env edits, leading to cached 401s in auth.json).

The cousin Auth* tests below already exercise os.environ-first semantics for _resolve_api_key_provider_secret (which still uses get_env_value), so the two precedence policies are both pinned now.

Validation

$ pytest tests/tools/test_credential_pool_env_fallback.py -q
9 passed in 1.59s

Refs

Scope

  • ✅ No production code change (test-only)
  • ✅ All 9 tests in the file pass
  • ✅ Test name + docstring now accurately describe the contract

Out of scope

The other ~10 main-CI failures — separate focused PRs (#18972, #18974, #18977, #18979 already up).

`_seed_from_env` was intentionally flipped to prefer `~/.hermes/.env`
over `os.environ` in commit 2ef1ad2 ("fix: prefer ~/.hermes/.env over
os.environ when seeding credential pool", fixes NousResearch#18254). That change
introduced a private `_get_env_prefer_dotenv` helper and stopped using
`get_env_value` (which is os.environ-first).

The unit test
`test_os_environ_still_wins_over_dotenv` was written for the **old**
`get_env_value`-based behaviour (predates 2ef1ad2) and was never
updated, so it failed on `main`:

    AssertionError: assert 'sk-dotenv-stale' == 'sk-env-fresh-xyz'

Rename + rewrite to assert the **current**, deliberate behaviour:
`.env` wins over `os.environ`, with a docstring explaining *why*
(stale shell env vars from parent processes like Codex CLI shadowing
deliberate user edits to .env, leading to cached 401s in auth.json).
The cousin `Auth*` tests below already exercise os.environ-first
semantics for `_resolve_api_key_provider_secret` (which still uses
`get_env_value`), so the two precedence policies are now both pinned.

Validation:

    $ pytest tests/tools/test_credential_pool_env_fallback.py -q
    9 passed in 1.59s

No production code change. Fixes the failure observed on `main`
(run 25250051126):

`tests/tools/test_credential_pool_env_fallback.py::TestCredentialPoolSeedsFromDotEnv::test_os_environ_still_wins_over_dotenv`

Refs: NousResearch#18254 (precedence flip), NousResearch#18757 (cousin os.getenv \u2192 get_env_value
fix on `auth.py` base_url path).
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have area/auth Authentication, OAuth, credential pools labels May 2, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

This looks implemented on current main by later test cleanup and replacement coverage.

Automated hermes-sweeper review evidence:

  • The stale failing test from this PR, test_os_environ_still_wins_over_dotenv, is no longer present in tests/tools/test_credential_pool_env_fallback.py; it was removed by 66320de52e9d77c5afc9767a350447011c8577f1.
  • The production contract still matches the PR's intended behavior: agent/credential_pool.py:1949 documents .env as authoritative over stale parent-process environment variables, and _get_env_prefer_dotenv uses load_env().get(key) or os.environ.get(key).
  • Equivalent regression coverage now exists in tests/agent/test_credential_pool.py:1125: test_load_pool_prefers_dotenv_over_stale_os_environ writes a fresh ~/.hermes/.env key while os.environ contains a stale key, then asserts the .env credential wins.
  • That replacement coverage was added by 0a6865b328ee6057eb59ee4a150c4886aa72d48c; the stale-test removal is contained in release tag v2026.5.16.

Thanks for identifying the stale assertion; the same contract is now covered on main, so this PR is safe to close as already implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have sweeper:implemented-on-main Sweeper: behavior already present on current main type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: auth.json credential cache ignores .env changes — stale key persists

3 participants