fix(auth): cache login failures and bound device-approval poll - #18
Merged
Conversation
A missing/expired session pickle plus no ROBINHOOD_TOTP_SECRET sent the server into the verification_workflow push-approval path, which polled synchronously for 120s while waiting for mobile-app approval. The MCP server is single-threaded, so every tool call froze for ~2 minutes and the failure was not cached — the next call repeated the freeze. From the user's perspective Claude Desktop looked unable to connect. Three changes: - server.py: _ensure_logged_in() now records transient AuthenticationError failures with a 5-minute cooldown. Subsequent calls inside the window raise immediately with the cached message instead of re-running the full login flow. EnvironmentVariablesError is still permanent. - auth.py: both poll loops in _patched_validate_sherrif_id now read ROBINHOOD_APPROVAL_TIMEOUT (default 60s, floor 5s) instead of a hard-coded 120s. Garbage values fall back to the default with a stderr warning. Error messages point users at ROBINHOOD_TOTP_SECRET. - README.md / CLAUDE.md: document the new env vars and the failure cache. Reframe TOTP and push-approval as two equally valid paths since Robinhood no longer exposes TOTP enrollment on every account. Worst-case server freeze drops from 2 min × every call to ~60s once, then fast failures for 5 min. Tests: 4 new server cooldown tests, 4 new approval-timeout tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the authentication path to prevent the single-threaded MCP server from freezing on repeated tool calls when Robinhood login falls back to device-approval polling, by bounding the polling duration and caching transient auth failures for a short cooldown.
Changes:
- Cache transient
AuthenticationErrorlogin failures in_ensure_logged_in()for ~5 minutes so subsequent tool calls fail fast instead of re-running the blocking login flow. - Add
ROBINHOOD_APPROVAL_TIMEOUT(default 60s, min 5s) to bound device-approval/TOTP-finalization polling in the patchedrobin_stocksworkflow. - Add targeted unit tests and update docs to explain push-approval vs TOTP and the new cooldown behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/robinhood_mcp/server.py |
Adds transient-auth failure cooldown caching to avoid repeated blocking logins. |
src/robinhood_mcp/auth.py |
Introduces configurable approval timeout and applies it to both polling loops. |
tests/test_server.py |
New tests validating cooldown caching, expiry retry, and permanent env-var errors. |
tests/test_auth.py |
New tests for ROBINHOOD_APPROVAL_TIMEOUT parsing/clamping and stderr warning. |
README.md |
Documents push approval vs TOTP paths, new env var, and failure caching behavior. |
CLAUDE.md |
Documents env vars and new auth failure caching behavior (one default value mismatch noted). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- server.py: use math.ceil for the cooldown "retry in Ns" message so sub-second remainders don't render as "0s" while still inside the cooldown window. - CLAUDE.md: sync ROBINHOOD_APPROVAL_TIMEOUT default to 60 (matches auth.py and README). Co-Authored-By: Claude Opus 4.7 <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
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.
Why
A user report: after restarting Claude Desktop, the robinhood-mcp server appeared "unable to connect" with every tool call hanging ~2 minutes and failing with
RobinhoodError: Not logged in: Login returned empty result. Diagnosis traced it to the auth path, not the MCP transport — the protocol handshake (initialize,tools/list) succeeds; only tool calls hang.Root cause: with no cached session pickle and no
ROBINHOOD_TOTP_SECRET, every login goes through Robinhood'sverification_workflowdevice-approval flow. The patched_validate_sherrif_idpolled synchronously for 120 seconds waiting for mobile-app approval. The MCP server is single-threaded, so this froze the whole server. The failure was also not cached, so subsequent tool calls repeated the full 2-minute block. With Robinhood increasingly hiding TOTP enrollment for passkey-primary accounts, this is now a realistic state.What changed
server.py—_ensure_logged_in()now caches transientAuthenticationErrorfailures with a 5-minute cooldown (_AUTH_FAILURE_COOLDOWN_SECONDS). During the cooldown, subsequent tool calls raise immediately with(cached failure; will retry in Xs)instead of re-running the login flow.EnvironmentVariablesError(missing creds) stays permanent. A successful login clears the cache.auth.py— both poll loops in_patched_validate_sherrif_id(push-approval and TOTP finalization) now readROBINHOOD_APPROVAL_TIMEOUTvia a new_approval_timeout_seconds()helper. Default 60s (down from 120s), floor 5s, garbage values fall back to the default with a stderr warning (not stdout — protocol safety). Error messages now point users atROBINHOOD_TOTP_SECRETfor non-interactive 2FA.README.md+CLAUDE.md— document the new env vars and the failure cache. Reframe TOTP and push-approval as two equally valid paths since Robinhood no longer surfaces TOTP enrollment on every account.Worst-case server freeze drops from 2 min × every call to ~60s once, then fast failures for 5 min.
Tests
tests/test_server.py(4 tests): cooldown cachesAuthenticationError, cooldown expiry allows one retry, success clears the cache,EnvironmentVariablesErrorstays permanent.TestApprovalTimeoutintests/test_auth.py(4 tests): default value, valid override, sub-minimum clamping, garbage fallback (asserts warning lands on stderr, not stdout).ruff check+ruff format --checkclean.Risk
Behavioral changes are confined to the auth-failure path:
TestPatchedValidationWorkflowtests still pass; the mockedtime.timeprogression has plenty of headroom under the new shorter loop.Read-only tool surface unchanged; no new tools or trading paths.