test(auth): stub macOS Keychain and skip botocore soft-dep tests by briandevans · Pull Request #15958 · NousResearch/hermes-agent

briandevans · 2026-04-26T08:27:08Z

Summary

Fixes 10 test_anthropic_adapter.py tests that broke on developer machines with real Claude Code credentials in the macOS Keychain
Fixes 6 test_bedrock_adapter.py tests that fail with ModuleNotFoundError: No module named 'botocore' when botocore isn't installed

The bug

Anthropic adapter (10 tests): Commit e1106772 added _read_claude_code_credentials_from_keychain() as the first credential source, checked before the JSON file path. The existing tests only mock Path.home() to a tmp_path, leaving the Keychain path unsuppressed. On any machine with Claude Code installed and an active Keychain entry, the Keychain wins and every file-based assertion fails (wrong token, or None instead of the expected value, or TypeError: the JSON object must be str, bytes or bytearray when subprocess.run is globally patched but its .stdout.strip() returns a MagicMock).

Bedrock adapter (6 tests): The production code in is_stale_connection_error already guards from botocore.exceptions import ... with try/except ImportError (comment: # pragma: no cover — botocore always present with boto3). The test functions import from botocore.exceptions directly at function scope without a guard, so they fail with ModuleNotFoundError in environments (CI, contributor machines) without botocore installed.

The fix

Add monkeypatch.setattr("agent.anthropic_adapter._read_claude_code_credentials_from_keychain", lambda: None) to each of the 10 tests that exercise file-based credential paths — this isolates the test to the file path only, which is what the test intends.
Add pytest.importorskip("botocore") at the top of each of the 6 bedrock tests that require botocore exceptions — they skip cleanly when botocore isn't installed (matching the production code's defensive posture), and still run and pass in boto3-equipped environments.

Test plan

Before: 16 additional failures beyond the 7-dingtalk/1-matrix baseline on clean main
After: 0 additional failures; 6 botocore tests skip cleanly instead of erroring
Regression guard: stashed fix → observed 7 failures; restored → 0 failures, 6 skips
All other tests in both files unchanged: 247 passed, 6 skipped

Two independent test isolation failures on clean main: 1. test_anthropic_adapter.py — read_claude_code_credentials checks the macOS Keychain before falling back to the JSON file (added in e110677). Tests that mock Path.home() to a tmp_path were not suppressing the keychain path, so on a developer machine with real Claude Code credentials the keychain wins and every file-based assertion fails. Add monkeypatch.setattr("..._read_claude_code_credentials_from_keychain", lambda: None) to each affected test so only the file path is exercised (10 tests fixed). 2. test_bedrock_adapter.py — TestIsStaleConnectionError and TestCallConverseInvalidatesOnStaleError import botocore.exceptions directly at test-function scope. botocore is an optional runtime dep (boto3 brings it in production; the test venv doesn't include it). The production code already handles ImportError defensively; the tests should follow suit. Add pytest.importorskip("botocore") to the 6 affected tests so they skip cleanly when botocore is absent rather than failing with ModuleNotFoundError. Before: 16 additional failures beyond the 7-dingtalk/1-matrix baseline. After: 0 additional failures (6 botocore tests skip instead of error). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR fixes test-isolation and optional-dependency issues in the agent adapter test suite so tests behave consistently across developer machines and minimal CI environments.

Changes:

Stubs the macOS Keychain credential source in file-based Anthropic credential tests to prevent real Keychain entries from affecting assertions.
Adds pytest.importorskip("botocore") to Bedrock adapter tests that import botocore.exceptions, so they skip cleanly when botocore isn’t installed.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`tests/agent/test_anthropic_adapter.py`	Disables Keychain credential lookup within tests that are intended to exercise file/env credential paths.
`tests/agent/test_bedrock_adapter.py`	Skips botocore-specific stale-connection tests when `botocore` is not available.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-26T08:30:34Z

+        # Keychain is checked before the file path on macOS; suppress it so
+        # the test exercises the file-based credential path exclusively.
+        monkeypatch.setattr(
+            "agent.anthropic_adapter._read_claude_code_credentials_from_keychain",
+            lambda: None,
+        )


These tests all repeat the same monkeypatch to disable _read_claude_code_credentials_from_keychain. To reduce duplication and the risk of missing it in future file-based credential tests, consider extracting this into a small fixture/helper (e.g., an autouse fixture in TestReadClaudeCodeCredentials / TestResolveAnthropicToken) that forces the keychain reader to return None unless a test explicitly opts into keychain behavior.

Copilot · 2026-04-26T08:30:35Z

    def test_detects_botocore_connection_closed_error(self):
+        pytest.importorskip("botocore")
        from agent.bedrock_adapter import is_stale_connection_error
        from botocore.exceptions import ConnectionClosedError
        exc = ConnectionClosedError(endpoint_url="https://bedrock.example")
        assert is_stale_connection_error(exc) is True

    def test_detects_botocore_endpoint_connection_error(self):
+        pytest.importorskip("botocore")
        from agent.bedrock_adapter import is_stale_connection_error
        from botocore.exceptions import EndpointConnectionError
        exc = EndpointConnectionError(endpoint_url="https://bedrock.example")
        assert is_stale_connection_error(exc) is True

    def test_detects_botocore_read_timeout(self):
+        pytest.importorskip("botocore")
        from agent.bedrock_adapter import is_stale_connection_error
        from botocore.exceptions import ReadTimeoutError
        exc = ReadTimeoutError(endpoint_url="https://bedrock.example")


pytest.importorskip("botocore") is duplicated across multiple tests in this section. For maintainability, consider factoring this into a fixture (e.g. requires_botocore) or doing a single botocore = pytest.importorskip("botocore", reason=...) at the class level and referencing botocore.exceptions in the tests, so the skip behavior is defined once.

Suggested change

def test_detects_botocore_connection_closed_error(self):

pytest.importorskip("botocore")

from agent.bedrock_adapter import is_stale_connection_error

from botocore.exceptions import ConnectionClosedError

exc = ConnectionClosedError(endpoint_url="https://bedrock.example")

assert is_stale_connection_error(exc) is True

def test_detects_botocore_endpoint_connection_error(self):

pytest.importorskip("botocore")

from agent.bedrock_adapter import is_stale_connection_error

from botocore.exceptions import EndpointConnectionError

exc = EndpointConnectionError(endpoint_url="https://bedrock.example")

assert is_stale_connection_error(exc) is True

def test_detects_botocore_read_timeout(self):

pytest.importorskip("botocore")

from agent.bedrock_adapter import is_stale_connection_error

from botocore.exceptions import ReadTimeoutError

exc = ReadTimeoutError(endpoint_url="https://bedrock.example")

botocore = pytest.importorskip(

"botocore",

reason="botocore is required for botocore-specific stale connection tests",

)

def test_detects_botocore_connection_closed_error(self):

from agent.bedrock_adapter import is_stale_connection_error

exc = TestIsStaleConnectionError.botocore.exceptions.ConnectionClosedError(

endpoint_url="https://bedrock.example"

)

assert is_stale_connection_error(exc) is True

def test_detects_botocore_endpoint_connection_error(self):

from agent.bedrock_adapter import is_stale_connection_error

exc = TestIsStaleConnectionError.botocore.exceptions.EndpointConnectionError(

endpoint_url="https://bedrock.example"

)

assert is_stale_connection_error(exc) is True

def test_detects_botocore_read_timeout(self):

from agent.bedrock_adapter import is_stale_connection_error

exc = TestIsStaleConnectionError.botocore.exceptions.ReadTimeoutError(

endpoint_url="https://bedrock.example"

)

… into fixtures Copilot review (PR NousResearch#15958) flagged two maintenance concerns: - All file-based credential tests in TestReadClaudeCodeCredentials, TestResolveAnthropicToken, and TestRunOauthSetupToken individually repeated the same monkeypatch to suppress keychain lookup; extract into per-class autouse _stub_keychain fixtures. - pytest.importorskip("botocore") appeared once per method in TestIsStaleConnectionError; replace with a single requires_botocore fixture so the skip guard is defined once without losing per-method granularity (urllib3/AssertionError tests still run without botocore). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

briandevans · 2026-04-26T09:12:19Z

Thanks @copilot — both findings are real. Addressed in `2cd9b730`.

1. Repeated keychain monkeypatch (test_anthropic_adapter.py)
Added per-class autouse `_stub_keychain` fixtures to `TestReadClaudeCodeCredentials`, `TestResolveAnthropicToken`, and `TestRunOauthSetupToken`. The five / two / three individual `monkeypatch.setattr` calls for `_read_claude_code_credentials_from_keychain` are removed; the fixture covers the whole class automatically.

2. Repeated `pytest.importorskip("botocore")` (test_bedrock_adapter.py)
Added a `requires_botocore` fixture inside `TestIsStaleConnectionError` and threaded it into only the three botocore-specific tests as a parameter. The urllib3 and AssertionError tests (which don't need botocore) remain unaffected and still run without botocore installed — using class-level `importorskip` would have over-skipped them.

All 1951 agent tests green (7 botocore tests skip as expected on this machine where botocore isn't installed).

briandevans · 2026-04-26T16:37:53Z

@copilot Both are style suggestions rather than correctness issues. The monkeypatch duplications are intentional test isolation — each test class is self-contained. The pytest.importorskip repetition in the botocore tests is explicit per-test, which makes them individually runnable without the class-level import. Happy to refactor into fixtures if the maintainers prefer a unified style.

briandevans · 2026-05-13T14:08:15Z

Closing to keep the queue clean — happy to reopen if this is still useful.

Copilot AI review requested due to automatic review settings April 26, 2026 08:27

Copilot started reviewing on behalf of briandevans April 26, 2026 08:27 View session

Copilot AI reviewed Apr 26, 2026

View reviewed changes

alt-glitch mentioned this pull request Apr 26, 2026

fix(test): skip bedrock adapter tests when botocore is not installed #16064

Closed

alt-glitch mentioned this pull request Apr 27, 2026

fix(test): stub macOS Keychain in TestReadClaudeCodeCredentials #16498

Closed

3 tasks

alt-glitch mentioned this pull request May 9, 2026

fix(tests): isolate credential tests from macOS Keychain #22692

Closed

19 tasks

briandevans closed this May 13, 2026

alt-glitch mentioned this pull request May 15, 2026

fix(tests): mock keychain in TestReadClaudeCodeCredentials to prevent credential leakage #26215

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(auth): stub macOS Keychain and skip botocore soft-dep tests#15958

test(auth): stub macOS Keychain and skip botocore soft-dep tests#15958
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/test-stubs-anthropic-keychain-bedrock-botocore

briandevans commented Apr 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

briandevans commented Apr 26, 2026

Uh oh!

briandevans commented Apr 26, 2026

Uh oh!

briandevans commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

briandevans commented Apr 26, 2026

Summary

The bug

The fix

Test plan

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

briandevans commented Apr 26, 2026

Uh oh!

briandevans commented Apr 26, 2026

Uh oh!

briandevans commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants