Skip to content

fix: make TokenManager.is_valid a property, matching the async mirror - #8

Merged
danieyal merged 1 commit into
masterfrom
fix/is-valid-property
Jul 22, 2026
Merged

fix: make TokenManager.is_valid a property, matching the async mirror#8
danieyal merged 1 commit into
masterfrom
fix/is-valid-property

Conversation

@danieyal

@danieyal danieyal commented Jul 22, 2026

Copy link
Copy Markdown
Owner

One-line source change plus a regression guard.

The bug

TokenManager.is_valid was a method while AsyncTokenManager.is_valid was a property, so porting sync code to async raised TypeError: 'bool' object is not callable at runtime. Neither suite caught it because each only exercised its own side.

Why the property form, not the method form

  • access_token and token are already properties on both managers, so is_valid was the odd one out within its own class.
  • A zero-arg, side-effect-free boolean accessor reads better as a property.
  • Nothing is published to PyPI yet, so there are no external callers to break — and that cost only grows after the first release.

is_valid is called nowhere in src/ (neither client uses it), so the blast radius is four assertions in tests/unit/test_auth.py.

Regression guard

A one-line fix doesn't stop the drift recurring, so this adds test_managers_expose_the_same_public_surface. It compares both classes' public members and their kinds (property vs function) via inspect.getattr_static, with aclose as the single sanctioned async-only member. Adding a member to one manager and not the other now fails the build.

Verified to catch the original bug — reverting auth.py makes it fail with:

assert not {'is_valid': ('function', 'property')}

Found while verifying, not fixed here

TokenManager creates its own httpx.Client when none is injected but exposes no close(), whereas AsyncTokenManager tracks _owns_client and has aclose().

I checked before calling it a leak — it isn't one. Both clients inject their own httpx client, so the manager never builds one on the normal path. It only affects a bare TokenManager(), which isn't exported. Recorded in AGENTS.md as low priority.

Testing

303 tests. ruff check, ruff format --check, mypy src/myinvois all clean.

Next

A follow-up PR will fix test_auth.py::test_token_manager_refresh_margin, which is vacuous — it never calls get_token(), so is_valid is False only because no token was ever acquired. It passes with expires_in=3600 (far outside the 60s margin) and 0 calls to the token endpoint, i.e. it would pass with the refresh-margin logic deleted. Kept separate to kept this PR single-concern; it touches the same file so it goes after this merges.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation

    • Updated authentication documentation to clarify synchronous and asynchronous API parity.
  • Bug Fixes

    • Changed TokenManager.is_valid to a boolean property for consistent access across authentication managers.
    • Preserved existing token expiry and refresh-margin behavior.
  • Tests

    • Added coverage ensuring synchronous and asynchronous managers expose matching public APIs, with async cleanup as the only exception.

TokenManager.is_valid was a method while AsyncTokenManager.is_valid was a
property, so porting sync code to async raised
`TypeError: 'bool' object is not callable` at runtime. Neither suite caught
it because each only exercised its own side.

Unified on the property form rather than the method form because:
- access_token and token are already properties on BOTH managers, so
  is_valid was the odd one out within its own class.
- A zero-arg, side-effect-free boolean accessor reads better as a property.
- Nothing is published to PyPI yet, so there are no external callers to
  break. That cost only grows after the first release.

is_valid is called nowhere in src/ -- neither client uses it -- so the blast
radius is four assertions in tests/unit/test_auth.py.

Adds test_managers_expose_the_same_public_surface, which compares both
classes' public members AND their kinds (property vs function) via
inspect.getattr_static, so future drift fails the build instead of being
found by a user. `aclose` is the single sanctioned async-only member.
Verified to catch the original bug: reverting auth.py makes it fail with
{'is_valid': ('function', 'property')}.

Also recorded in AGENTS.md while verifying the surfaces, not fixed here:
TokenManager creates its own httpx.Client when none is injected but exposes
no close(), whereas AsyncTokenManager tracks _owns_client and has aclose().
Not a live leak -- both clients inject their own httpx client, so the manager
never builds one on the normal path; it only affects a bare TokenManager(),
which is not exported.

303 tests. ruff, ruff format, mypy src/myinvois all clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

The authentication API now exposes TokenManager.is_valid as a boolean property. Sync tests use property access, and async tests enforce matching public member names and kinds between the sync and async managers, allowing only aclose as async-only.

Authentication API parity

Layer / File(s) Summary
Token validity property
src/myinvois/auth.py, tests/unit/test_auth.py
TokenManager.is_valid is converted to a property, and validity assertions are updated accordingly.
Sync/async surface contract
tests/unit/test_async_auth.py, AGENTS.md
A parity test compares public members and their kinds, while documentation records the contract and client ownership note.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main change: converting TokenManager.is_valid to a property to match the async mirror.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/is-valid-property

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/unit/test_auth.py (1)

111-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make the refresh-margin test acquire a token.

This assertion runs before mgr.get_token(), so is_valid is false simply because no token exists. The test would still pass if the refresh-margin logic were deleted. Mock two responses, call get_token() twice, and assert that the route is called twice.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_auth.py` around lines 111 - 118, Update
test_token_manager_refresh_margin to mock two token endpoint responses, call
mgr.get_token() twice, and assert the mocked route was called twice. Ensure the
test validates refresh-margin behavior after an initial token is acquired rather
than checking is_valid before any token exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@tests/unit/test_auth.py`:
- Around line 111-118: Update test_token_manager_refresh_margin to mock two
token endpoint responses, call mgr.get_token() twice, and assert the mocked
route was called twice. Ensure the test validates refresh-margin behavior after
an initial token is acquired rather than checking is_valid before any token
exists.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3167de9d-bae8-435a-8eda-9e4b19ce6b3b

📥 Commits

Reviewing files that changed from the base of the PR and between a3a04d1 and 61ec935.

📒 Files selected for processing (4)
  • AGENTS.md
  • src/myinvois/auth.py
  • tests/unit/test_async_auth.py
  • tests/unit/test_auth.py

@danieyal
danieyal merged commit d62fa7a into master Jul 22, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant