Skip to content

test(agent): isolate anthropic token resolution tests from macOS Keyc… - #49796

Closed
NarahariRaghava wants to merge 1 commit into
NousResearch:mainfrom
NarahariRaghava:fix/anthropic-adapter-test-keychain-isolation
Closed

test(agent): isolate anthropic token resolution tests from macOS Keyc…#49796
NarahariRaghava wants to merge 1 commit into
NousResearch:mainfrom
NarahariRaghava:fix/anthropic-adapter-test-keychain-isolation

Conversation

@NarahariRaghava

Copy link
Copy Markdown

What does this PR do?

Fixes a test isolation bug in tests/agent/test_anthropic_adapter.py. Three test classes (TestResolveAnthropicToken, TestResolveWithRefresh, TestRunOauthSetupToken) call resolve_anthropic_token() / run_oauth_setup_token(), which internally check the macOS Keychain via _read_claude_code_credentials_from_keychain() before falling back to the mocked credential sources the tests set up.

On a Mac with Claude Code installed (real OAuth credentials present in Keychain), this causes two failure modes:

  1. Tests asserting on mocked credential values fail because the real Keychain token is returned instead (e.g. assert None == 'cc-auto-token').
  2. Tests in TestRunOauthSetupToken that mock subprocess.run globally (to fake the claude setup-token subprocess call) also intercept the Keychain's own subprocess.run(["security", ...]) call, returning a generic MagicMock that crashes json.loads() with TypeError: the JSON object must be str, bytes or bytearray, not MagicMock.

This makes the test suite non-deterministic. It passes or fails depending on what's in the developer's real Keychain, not on the code logic being tested.

The fix adds an autouse no_keychain pytest fixture to all three affected classes, mirroring the fixture already correctly used in TestReadClaudeCodeCredentials in the same file. This blocks the Keychain lookup so these tests only exercise the credential file and env var paths they're actually meant to test.

Related Issue

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tests/agent/test_anthropic_adapter.py: added an autouse no_keychain pytest fixture to TestResolveAnthropicToken, TestResolveWithRefresh, and TestRunOauthSetupToken, each mocking agent.anthropic_adapter._read_claude_code_credentials_from_keychain to return None. This matches the existing pattern already used in TestReadClaudeCodeCredentials in the same file.

How to Test

  1. On macOS with Claude Code installed and logged in (real OAuth credentials present in Keychain), run scripts/run_tests.sh tests/agent/test_anthropic_adapter.py.
  2. Before this fix, 5 to 6 of 160 tests fail non deterministically (varies by run) with either an AssertionError (real Keychain token returned instead of mocked value) or TypeError: the JSON object must be str, bytes or bytearray, not MagicMock (Keychain subprocess call intercepted by an unrelated subprocess.run mock).
  3. After this fix, all 160 tests pass consistently. scripts/run_tests.sh tests/agent/test_anthropic_adapter.py returns 160 tests passed, 0 failed.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Sequoia), Python 3.12.12

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — N/A, this fix only affects macOS-specific Keychain test isolation
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

Before fix (failing)

before1 before2 before3 before4

After fix (passing)

after1 Screenshot 2026-06-20 at 3 38 11 PM Screenshot 2026-06-20 at 3 38 19 PM Screenshot 2026-06-20 at 3 39 11 PM

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/auth Authentication, OAuth, credential pools provider/anthropic Anthropic native Messages API P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jun 20, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #43344 — same fix: both PRs add an autouse no_keychain fixture monkeypatching agent.anthropic_adapter._read_claude_code_credentials_from_keychain to None on the same three test classes (TestResolveAnthropicToken, TestResolveWithRefresh, TestRunOauthSetupToken) in tests/agent/test_anthropic_adapter.py. #43344 (open) is the earlier submission. The bug is still live on main (only TestReadClaudeCodeCredentials carries the fixture), so the fix is valid — but the two PRs are identical in mechanism.

@NarahariRaghava

Copy link
Copy Markdown
Author

Closing in favor of #43344 (same fix, opened first). Left a comment there confirming I independently hit and verified the same bug + fix, hopefully that helps move it along since it looks like it's been waiting on review for a couple weeks.

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 comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have provider/anthropic Anthropic native Messages API type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants