fix(auth+shell): PR #215 review remediation and running-prompt ghost fix - #217
Merged
Conversation
Hardens the xAI / GitHub Copilot / DigitalOcean / Snowflake Cortex OAuth login providers merged in #215: scope OAuth refresh to the active/selected provider, serialize credential persistence under a lock with atomic config replacement, roll back replaced credentials on failure, fail closed on credential migration, validate OAuth token and implicit-state responses, bound implicit callback bodies, and preserve router-discovery and catalog degradation truthfulness. Adds comprehensive OAuth persistence, refresh, platform, and provider test coverage.
…mpt ghost On long streaming turns the running-prompt block (agent tree, spinner, tip) could render a doubled "ghost" copy. run_in_terminal re-requests an absolute cursor position (async CPR round-trip) on teardown; the suppressed live body re-expanded before that settled, diffing against a provisional cursor model and stranding the old rows. Await the CPR settle while the handoff is still suppressed so the body re-expands against a correct cursor model. Unlike a renderer reset this touches no static scrollback, so it does not re-fossilize committed content (the queued-input ghost fix stays intact).
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughOAuth handling now validates provider responses, uses scoped refreshes, bounded locks, atomic persistence, rollback, and guarded managed-model commits. DigitalOcean discovery and terminal scrollback handoffs gain dedicated edge-case handling and regression coverage. ChangesOAuth and provider flows
Terminal rendering
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant refresh_managed_models
participant OAuthManager
participant persist_config_change
refresh_managed_models->>OAuthManager: ensure_fresh(oauth_ref)
OAuthManager-->>refresh_managed_models: refreshed provider credentials
refresh_managed_models->>persist_config_change: apply snapshot-guarded model updates
persist_config_change-->>refresh_managed_models: authoritative committed configuration
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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: 4
🤖 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.
Inline comments:
In `@src/pythinker_code/auth/oauth.py`:
- Around line 642-645: Update load_tokens and the keyring migration flow around
_migrate_keyring_token so its read-copy-delete operation is serialized with
persist_login, persist_logout, and other credential writes via
_credential_transaction_locks. Ensure callers such as resolve_api_key,
get_chatgpt_account_id, and _refresh_tokens cannot perform keyring migration
outside the same lock, while preserving normal file-token loading.
In `@src/pythinker_code/auth/platforms.py`:
- Around line 353-356: Update the OAuthManager construction in the
provider.oauth branch to run off the event loop, using asyncio.to_thread around
OAuthManager(working_config) so its synchronous initialization and
_migrate_oauth_storage file lock cannot block async refresh handling.
In `@src/pythinker_code/ui/shell/visualize/_interactive.py`:
- Around line 249-250: Replace the suppress(Exception) block in
_settle_cursor_after_handoff with explicit exception handling that records the
exception through _handoff_trace and logger.debug, matching
_reset_prompt_renderer and _safe_prompt_invalidate. Preserve the existing
wait_for_cpr_responses behavior while ensuring CPR-settling failures are logged.
In `@tests/ui_and_conv/test_visualize_running_prompt.py`:
- Around line 217-273: Add a failure-path regression test alongside
test_scrollback_handoff_settles_cursor_before_reexpanding, making
run_in_terminal raise during _emit_incremental_content_commits. Assert the
RuntimeError propagates, _settle_cursor_after_handoff runs while
_scrollback_handoff_depth is 1, _reset_prompt_renderer is invoked, and the
handoff depth returns to zero.
🪄 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: f9a4760d-1e92-4f0e-b002-9a6b55e679fc
⛔ Files ignored due to path filters (2)
docs/en/release-notes/changelog.mdis excluded by!docs/**docs/superpowers/specs/2026-07-18-pr-215-auth-review-fixes-design.mdis excluded by!docs/**
📒 Files selected for processing (21)
.gitignoreCHANGELOG.mdsrc/pythinker_code/auth/digitalocean.pysrc/pythinker_code/auth/oauth.pysrc/pythinker_code/auth/oauth_flows.pysrc/pythinker_code/auth/platforms.pysrc/pythinker_code/auth/snowflake.pysrc/pythinker_code/auth/xai.pysrc/pythinker_code/ui/shell/visualize/_interactive.pysrc/pythinker_code/utils/io.pysrc/pythinker_code/web/api/sessions.pytests/auth/test_digitalocean_auth.pytests/auth/test_oauth_cross_process.pytests/auth/test_oauth_flows.pytests/auth/test_oauth_persist.pytests/auth/test_oauth_refresh.pytests/auth/test_platforms.pytests/auth/test_snowflake_auth.pytests/auth/test_xai_auth.pytests/ui_and_conv/test_visualize_running_prompt.pytests/web/test_sessions_api.py
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
- Log CPR-settle failures in `_settle_cursor_after_handoff` via `_handoff_trace` + `logger.debug` instead of silently suppressing, matching the sibling `_reset_prompt_renderer` / `_safe_prompt_invalidate` handlers (no silent swallow). - Construct `OAuthManager` off the event loop in `refresh_managed_models` via `asyncio.to_thread`, so its synchronous `_migrate_oauth_storage()` file lock cannot stall async refresh. - Add a failure-path test asserting the scrollback-handoff `finally` still settles the cursor (and the fail path resets the renderer) when emit raises.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pythinker_code/ui/shell/visualize/_interactive.py (1)
311-313: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake handoff cleanup cancellation-safe. Cancellation at the CPR wait can leave
_scrollback_handoff_depthstuck> 0and keep the prompt in finalizing mode; move the decrement and invalidate into an innerfinally.🤖 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 `@src/pythinker_code/ui/shell/visualize/_interactive.py` around lines 311 - 313, Update the handoff cleanup around `_settle_cursor_after_handoff()` so cancellation during its wait still executes cleanup by placing `_scrollback_handoff_depth` decrement and `_safe_prompt_invalidate()` in an inner finally block. Preserve the existing ordering and ensure both cleanup actions run exactly once after the handoff attempt.Source: Coding guidelines
src/pythinker_code/auth/platforms.py (1)
548-568: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn the guarded commit result, not the local working-copy diff.
changedis computed while mutatingworking_config, butapply_updatesmay skip every update whenproviders_matchfails—or may apply a change to the latest committed config even whenchangedisFalse. The function can therefore report success or no change incorrectly on concurrent refreshes. Track whether guarded applications actually changedcommittedand return that status.As per coding guidelines, observable results must distinguish success, failure, and partial success.
🤖 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 `@src/pythinker_code/auth/platforms.py` around lines 548 - 568, Update the surrounding function that defines apply_updates and returns changed so it tracks whether guarded applications actually modify committed. Have each successful _apply_models, apply_opencode_go_models, apply_minimax_models, apply_z_ai_models, and apply_kimi_models operation contribute to a committed-change flag, while skipped provider_match guards do not; return the persist_config_change result or equivalent guarded commit status instead of the local working_config diff, preserving distinct success, failure, and partial-success outcomes.Source: Coding guidelines
🤖 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.
Inline comments:
In `@src/pythinker_code/auth/platforms.py`:
- Around line 357-360: Update the OAuthManager construction in the
refresh_managed_models flow to retain the asyncio.to_thread task, and on
cancellation await that task to completion before re-raising CancelledError.
Preserve the existing OAuthManager construction and ensure
_migrate_oauth_storage cannot continue writing shared state after the caller has
unwound.
In `@tests/ui_and_conv/test_visualize_running_prompt.py`:
- Around line 311-314: Update the test around the view handoff to exercise the
renderer boundary instead of monkeypatching private helpers such as
_settle_cursor_after_handoff and _reset_prompt_renderer. Fake get_app_or_none()
with a renderer test double, have run_in_terminal invoke func() while making
emission fail, and assert the renderer’s CPR wait/reset calls and their
ordering.
---
Outside diff comments:
In `@src/pythinker_code/auth/platforms.py`:
- Around line 548-568: Update the surrounding function that defines
apply_updates and returns changed so it tracks whether guarded applications
actually modify committed. Have each successful _apply_models,
apply_opencode_go_models, apply_minimax_models, apply_z_ai_models, and
apply_kimi_models operation contribute to a committed-change flag, while skipped
provider_match guards do not; return the persist_config_change result or
equivalent guarded commit status instead of the local working_config diff,
preserving distinct success, failure, and partial-success outcomes.
In `@src/pythinker_code/ui/shell/visualize/_interactive.py`:
- Around line 311-313: Update the handoff cleanup around
`_settle_cursor_after_handoff()` so cancellation during its wait still executes
cleanup by placing `_scrollback_handoff_depth` decrement and
`_safe_prompt_invalidate()` in an inner finally block. Preserve the existing
ordering and ensure both cleanup actions run exactly once after the handoff
attempt.
🪄 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: 7b91e679-9e4e-4158-bf9a-a6edd362db82
📒 Files selected for processing (3)
src/pythinker_code/auth/platforms.pysrc/pythinker_code/ui/shell/visualize/_interactive.pytests/ui_and_conv/test_visualize_running_prompt.py
load_tokens()'s keyring read-copy-delete migration ran outside _credential_transaction_locks, so it could race persist_login/persist_logout and overwrite a newer file token with the older keyring value. Serialize it under the credential lock. The lock is now reentrant per thread (fcntl.flock / msvcrt.locking deny a second same-file acquisition within one process), so callers already inside a credential transaction — e.g. persist_login loading the previous token at the same key (oauth.py:804) — re-enter instead of self-deadlocking. Adds reentrancy and migration-under-held-lock regression tests.
- Make the off-loop OAuthManager construction cancellation-safe: drain the asyncio.to_thread worker on cancellation so _migrate_oauth_storage cannot keep writing shared OAuth state (or hold its file lock) after the caller unwinds. - Add a behavior-level test exercising the real _settle_cursor_after_handoff against a renderer double (awaits CPR when supported, no-op otherwise, failing wait swallowed) rather than only mocking the helper.
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
Consolidates two independent fixes onto one branch off
main:PR feat(auth): add xAI, GitHub Copilot, DigitalOcean, and Snowflake Cortex OAuth login providers #215 auth review remediation — hardens the xAI / GitHub Copilot / DigitalOcean / Snowflake Cortex OAuth login providers merged in feat(auth): add xAI, GitHub Copilot, DigitalOcean, and Snowflake Cortex OAuth login providers #215: scope token refresh to the active/selected provider, serialize credential persistence under a lock with atomic config replacement, roll back replaced credentials on a save failure, fail closed on credential migration, validate OAuth token and implicit-state responses, bound implicit callback bodies, and preserve router-discovery / catalog-degradation truthfulness. Adds comprehensive OAuth persistence, refresh, platform, and provider test coverage.
Running-prompt "ghost" fix — on long streaming turns the running-prompt block (agent tree, spinner, tip) could render a doubled/ghost copy.
run_in_terminalre-requests an absolute cursor position (an async CPR round-trip) on teardown; the suppressed live body re-expanded before that settled, diffing against a provisional cursor model and stranding the old rows. It now awaits the CPR settle while the handoff is still suppressed, so the body re-expands against a correct cursor model. Unlike a renderer reset, this touches no static scrollback, so the fix(shell): remove queued-input bordered ghost in running-turn TUI #216 queued-input ghost fix stays intact (verified by its e2e test).Verification
make check-pythinker-code— ruff + format + pyright + ty: all pass.make test-pythinker-code— fulltests+tests_e2esuites green (7511 passed, 7 skipped), including the fix(shell): remove queued-input bordered ghost in running-turn TUI #216 queued-input regression e2e test and a new running-prompt handoff regression unit test.Notes
renderer.clear()mechanism)./tasks/working files.Summary by CodeRabbit
Content-Lengthand rejects blank access tokens.