Closed
Conversation
The preauthorized endpoint intermittently returns 401 or 429. Retry up to 3 times with linear backoff (1s, 2s) before failing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds retry-with-backoff to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #214 +/- ##
==========================================
- Coverage 99.91% 99.86% -0.06%
==========================================
Files 68 68
Lines 3566 3581 +15
==========================================
+ Hits 3563 3576 +13
- Misses 3 5 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/garth/sso.py`:
- Around line 148-150: The get_oauth1_token function can leave resp undefined
when retries <= 0; add an upfront guard in get_oauth1_token that validates the
retries parameter (e.g., ensure retries is an int >= 1 or raise a ValueError)
before entering the retry loop so resp is always assigned; alternatively clamp
retries to a minimum of 1 at function start and proceed—this prevents the
UnboundLocalError when later referencing resp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f47eee5-c735-43e9-acf9-522229d4ed6b
📒 Files selected for processing (2)
src/garth/sso.pysrc/garth/version.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_sso.py`:
- Around line 202-224: Update the test_get_oauth1_token_raises_after_retries
test to verify that retries actually occurred: instrument the mocked
GarminOAuth1Session.get (or wrap mock_resp) with a call counter (e.g., use a
MagicMock for get or attach side_effect that increments) and patch time.sleep so
you can assert it was called the expected number of times; after invoking
sso.get_oauth1_token("ticket", client) inside the pytest.raises block, assert
the mocked get call count and the patched time.sleep call count reflect the
retry attempts (reference GarminOAuth1Session.get, sso.get_oauth1_token, and
time.sleep) so the test fails if the function raises on the first 401.
- Around line 170-200: Extend the test for get_oauth1_token to also simulate a
429 response and assert the linear backoff sleeps: create a mock_resp_429
(ok=False, status_code=429) and adjust mock_get to return 429 on the first call,
401 on the second, and the successful 200 on the third; patch
sso.GarminOAuth1Session.get to use this sequence, patch time.sleep (e.g., with a
MagicMock) and after calling sso.get_oauth1_token assert that time.sleep was
called with the expected linear delays (for two retries, e.g., 1 then 2) and
that call_count reached 3 and the returned token is correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e64b483-bfaa-4e51-8310-a18bed63691a
📒 Files selected for processing (1)
tests/test_sso.py
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fetch public IP once at session start via ipify.org and include it in every span. Helps diagnose IP-based issues with Garmin's SSO (per cyberjunky/python-garminconnect#332). Skipped when telemetry is disabled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/garth/telemetry.py (1)
149-184: Split_response_hookinto smaller helpers.This method is doing extraction, sanitization, payload assembly, and dispatch/error isolation in one block; please split into helper methods to keep it within the file’s readability rule.
As per coding guidelines,
**/*.py: “No function should require scrolling to read”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/garth/telemetry.py` around lines 149 - 184, The _response_hook is too large; split its responsibilities into small helpers: implement a helper like _extract_request_response(response) that returns a raw dict with keys method, url, status_code, request, response, headers, body and uses response.request, and another helper _sanitize_payload(raw_dict) that calls sanitize_headers, sanitize and decodes bytes to produce the final payload including session_id, __version__, and _public_ip, and a _dispatch_telemetry(payload) that invokes self.callback or self._default_callback; then reduce _response_hook to calling these three helpers inside a minimal try/except so telemetry errors don’t bubble up. Ensure you reference existing symbols _default_callback, sanitize, sanitize_headers, callback, session_id, __version__, and _public_ip when building helpers so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/garth/telemetry.py`:
- Around line 126-128: Telemetry currently performs a blocking network call
during construction by setting self._public_ip = self._fetch_public_ip() in
Telemetry.__init__, which adds startup latency; remove that eager call and
instead initialize self._public_ip to a sentinel (e.g., None or "disabled") and
lazily call and cache _fetch_public_ip() the first time a telemetry event is
prepared/sent (provide a helper like _get_public_ip() or call _fetch_public_ip()
inside the existing event creation path), ensuring you catch exceptions and
apply a short timeout so failures don’t block the caller; update references to
self._public_ip accordingly so initialization is non-blocking and the actual
fetch happens only on first event send.
- Around line 130-139: Change the broad except in _fetch_public_ip to only catch
network-related errors: import requests (or keep the in-try import but add a
separate except ImportError) and replace "except Exception" with "except
_requests.RequestException" so only request/network failures are swallowed and
other bugs surface; ensure the name _requests is available in the except by
importing requests before the try or by handling ImportError separately, and
keep returning "unknown" on request failures.
- Line 161: The telemetry payload currently includes raw public IP via
self._public_ip; change the default behavior to avoid emitting raw IP by either
removing self._public_ip from the default payload and replacing it with an
anonymized value (e.g., salted hash or truncated form) or require explicit
opt-in before emitting the raw IP; update the code that builds/sends the payload
(look for the Telemetry class and the method that constructs the payload
containing "public_ip") to emit only the anonymized token by default and ensure
config defaults (enabled=True/send_to_logfire=True) do not cause raw IP export.
---
Nitpick comments:
In `@src/garth/telemetry.py`:
- Around line 149-184: The _response_hook is too large; split its
responsibilities into small helpers: implement a helper like
_extract_request_response(response) that returns a raw dict with keys method,
url, status_code, request, response, headers, body and uses response.request,
and another helper _sanitize_payload(raw_dict) that calls sanitize_headers,
sanitize and decodes bytes to produce the final payload including session_id,
__version__, and _public_ip, and a _dispatch_telemetry(payload) that invokes
self.callback or self._default_callback; then reduce _response_hook to calling
these three helpers inside a minimal try/except so telemetry errors don’t bubble
up. Ensure you reference existing symbols _default_callback, sanitize,
sanitize_headers, callback, session_id, __version__, and _public_ip when
building helpers so behavior remains identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0695b246-0cb8-4a31-a7bc-4f9d502bc5df
📒 Files selected for processing (1)
src/garth/telemetry.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/garth/telemetry.py (1)
141-142:⚠️ Potential issue | 🟠 MajorFollow-up: hashing the full IP is still reversible for IPv4.
This is still a stable pseudonymous identifier, not anonymous telemetry; truncating SHA-256 to 16 hex chars does not materially change that. If this must stay anonymous by default, hash a coarser network bucket or make the field opt-in. As per coding guidelines,
src/garth/telemetry.pyrequiresDEFAULT_TOKENtelemetry to be "sanitized, anonymous trace data".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/garth/telemetry.py` around lines 141 - 142, The current code fetches the full public IP via _req.get("https://api.ipify.org") and hashes it (then truncates) which still yields a stable pseudonymous identifier; instead, in src/garth/telemetry.py change the logic around the IP collection in the same function/section to derive a coarse network bucket (e.g., mask IPv4 to /24 or IPv6 to /64) before hashing, or remove automatic collection and make the telemetry field opt-in by default (tied to DEFAULT_TOKEN behavior); ensure you update the code that reads ip, the hashing path, and any DEFAULT_TOKEN default so the stored value is sanitized and non-identifying.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/garth/telemetry.py`:
- Around line 163-166: The telemetry code eagerly evaluates self.ip_hash while
building the data dict, causing an unnecessary network call; change it so
ip_hash is only resolved when needed by the chosen send path: either compute
ip_hash lazily (turn self.ip_hash into a property that caches on first access)
or move the lookup into the branches that actually send to Logfire (check
send_to_logfire flag and the selected callback) and only add "ip_hash" to the
data dict in those branches; reference the existing self.ip_hash access and the
send logic (send_to_logfire / logfire callback / the telemetry send function)
and ensure any custom callback path can opt out without triggering the lookup.
---
Duplicate comments:
In `@src/garth/telemetry.py`:
- Around line 141-142: The current code fetches the full public IP via
_req.get("https://api.ipify.org") and hashes it (then truncates) which still
yields a stable pseudonymous identifier; instead, in src/garth/telemetry.py
change the logic around the IP collection in the same function/section to derive
a coarse network bucket (e.g., mask IPv4 to /24 or IPv6 to /64) before hashing,
or remove automatic collection and make the telemetry field opt-in by default
(tied to DEFAULT_TOKEN behavior); ensure you update the code that reads ip, the
hashing path, and any DEFAULT_TOKEN default so the stored value is sanitized and
non-identifying.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6553981b-68e3-445d-b8fd-c7535481648f
📒 Files selected for processing (2)
src/garth/telemetry.pytests/test_telemetry.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_telemetry.py
- SHA256 hash (first 16 chars) instead of raw IP for privacy - Lazy fetch on first telemetry event (not at construction) - Catch RequestException only, not all exceptions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
matin
force-pushed
the
retry-preauthorized
branch
from
281ba8a to
270cbfb
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Data shows retries never succeed on preauthorized — 0/12 for 401, 0/0 for 429. Remove the manual retry loop and revert status_forcelist. Keep: public IP and OS in telemetry spans for diagnosing IP-based Garmin SSO issues. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The preauthorized endpoint intermittently returns 401 or 429 even with correct Android SSO constants. Retry up to 3 times with linear backoff (1s, 2s) before failing.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores