Tokens not saved to disk after fresh credential login#342
Conversation
WalkthroughDuring credentials-based login the code records a normalized Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@garminconnect/__init__.py`:
- Around line 473-481: The code references tokenstore_path unconditionally which
can be undefined; ensure tokenstore_path is always set (e.g., initialize
tokenstore_path = None before the tokenstore handling and only assign
self.client._tokenstore_path when tokenstore_path is not None) and update the
login/credential path to use that guarded value (look for
self.client._tokenstore_path, tokenstore_path, tokenstore, and the call
self.client.login). Also wrap the call to self.client.dump(tokenstore_path) in a
try/except (or use contextlib.suppress(Exception)) and log a warning on failure
instead of letting I/O exceptions propagate (consistent with _refresh_session
behavior). Ensure these changes cover the inline-token (>512 chars) load-failure
case so tokenstore_path is never referenced unless defined.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e7b1b60d-4c26-4bdd-9148-2402b8763cd4
📒 Files selected for processing (1)
garminconnect/__init__.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
garminconnect/__init__.py (1)
422-440: 🧹 Nitpick | 🔵 TrivialAdd type hint for
tokenstore_pathfor consistency.The initialization fixes the
NameErrorissue from the previous review. However, per coding guidelines, type hints should be used consistently. Consider adding an explicit type annotation:tokens_loaded = False - tokenstore_path = None + tokenstore_path: str | None = NoneAs per coding guidelines,
garminconnect/**: "use type hints consistently".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/__init__.py` around lines 422 - 440, tokenstore_path is initialized without a type hint; update its declaration to include a type annotation (e.g., change "tokenstore_path = None" to "tokenstore_path: Optional[str] = None") and ensure Optional is imported from typing at the top of the module so static type checks and code style are consistent with the rest of the codebase; locate the variable in the __init__ (where token loading happens) and add the import if missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@garminconnect/__init__.py`:
- Around line 482-486: Replace the silent contextlib.suppress(Exception) around
self.client.dump(tokenstore_path) with a try/except that preserves the non-fatal
behavior but logs the failure; e.g., wrap the call in try:
self.client.dump(tokenstore_path) except Exception as e:
self.logger.warning("Failed to persist tokenstore to %s: %s", tokenstore_path,
e) (keep swallowing the exception so login continues). This references the
self.client.dump(...) call and uses the instance logger to surface
permission/disk errors without changing control flow.
---
Outside diff comments:
In `@garminconnect/__init__.py`:
- Around line 422-440: tokenstore_path is initialized without a type hint;
update its declaration to include a type annotation (e.g., change
"tokenstore_path = None" to "tokenstore_path: Optional[str] = None") and ensure
Optional is imported from typing at the top of the module so static type checks
and code style are consistent with the rest of the codebase; locate the variable
in the __init__ (where token loading happens) and add the import if missing.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2e2190c2-f25a-492e-86b5-0419eff28e76
📒 Files selected for processing (1)
garminconnect/__init__.py
| # Persist tokens so next run restores without re-login/MFA | ||
| if tokenstore_path is not None: | ||
| with contextlib.suppress(Exception): | ||
| self.client.dump(tokenstore_path) | ||
| # Continue to load profile/settings below |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging a warning when token dump fails.
The exception suppression using contextlib.suppress(Exception) correctly prevents I/O failures from causing login to appear failed. However, silent suppression means the user won't know if token persistence failed (e.g., permission denied, disk full).
Consider adding a debug/warning log on failure for observability:
# Persist tokens so next run restores without re-login/MFA
if tokenstore_path is not None:
- with contextlib.suppress(Exception):
- self.client.dump(tokenstore_path)
+ try:
+ self.client.dump(tokenstore_path)
+ except Exception as e:
+ logger.warning("Failed to persist tokens to %s: %s", tokenstore_path, e)That said, the current implementation matches the existing pattern in client.py (context snippet 1), so this is optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garminconnect/__init__.py` around lines 482 - 486, Replace the silent
contextlib.suppress(Exception) around self.client.dump(tokenstore_path) with a
try/except that preserves the non-fatal behavior but logs the failure; e.g.,
wrap the call in try: self.client.dump(tokenstore_path) except Exception as e:
self.logger.warning("Failed to persist tokenstore to %s: %s", tokenstore_path,
e) (keep swallowing the exception so login continues). This references the
self.client.dump(...) call and uses the instance logger to surface
permission/disk errors without changing control flow.
On a fresh login, _tokenstore_path was never set on the client before calling client.login(), so auto-refresh dump() calls never fired and the token file was never written. Every run forced a full re-login with MFA regardless of tokenstore path provided. Fix: normalize tokenstore_path to str upfront, set client._tokenstore_path before credential login, and explicitly dump tokens after a successful fresh login. Reported and fixed by @davidbsandor — thanks! Co-Authored-By: davidbsandor <[email protected]> Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
2 participants
After the garth replacement (21aea2d),
Garmin.login()callsclient.load()when restoring existing tokens, which sets_tokenstore_pathas a side effect. On a fresh credential login however, it calledclient.login()directly without ever setting_tokenstore_path. Thedump()calls inside_refresh_session()therefore never fired, and the initial tokens were never written to disk at all which forced a full login + MFA prompt on every single run regardless of the tokenstore path provided.Fix: set
client._tokenstore_pathbefore callingclient.login()so that in-session auto-refreshes also persist correctly, then explicitly callclient.dump()after a successful fresh login when a tokenstore path was provided.To reproduce: set
GARMINTOKENSto any directory, delete any existinggarmin_tokens.json, runexample.pythrough a full MFA login, and the token file was never created. With this fix the file is written immediately after login completes and subsequent runs restore the session without prompting.Summary by CodeRabbit