feat(auth): add xAI, GitHub Copilot, DigitalOcean, and Snowflake Cortex OAuth login providers - #215
Merged
Conversation
…ule header The refactor moved the best-effort fetch/timeout into the shared models.dev catalog module, so the provider test now asserts graceful degradation via the shared loader instead of the removed inline timeout constant.
- Use utf-8 (not ascii) for the base64url decode to satisfy the explicit-encoding static requirement (base64url output is ASCII, so equivalent). - Cast socket getsockname() before len/index so strict pyright has a known arg type.
Add a 'copilot' managed provider for individual github.com accounts. Login runs the GitHub device-code flow, exchanges the OAuth token for a short-lived Copilot bearer via copilot_internal/v2/token, and stores the GitHub token as the refresh credential so OAuthManager can re-exchange on expiry. Chat routes through openai_legacy to api.githubcopilot.com with the Copilot integration headers. Wired into shell /login /logout and CLI login/logout --copilot. Business/Enterprise routing is out of scope (individual host only).
Add an 'xai' managed provider with two OAuth methods: browser loopback-PKCE (pinned redirect 127.0.0.1:56121, plan=generic + OIDC nonce) and RFC 8628 device-code. Tokens exchange/refresh against auth.x.ai with rotating refresh tokens persisted by OAuthManager; chat routes through openai_legacy to api.x.ai/v1. Wired into shell /login /logout and CLI login --xai/--xai-device, logout --xai.
Add a reusable OAuth 2.0 implicit-flow loopback helper for providers whose token arrives in the URL fragment (response_type=token). Serves an HTML bootstrap page on GET <callback_path> whose inline JS posts the parsed fragment to a pinned-port POST <token_path>; validates state, requires a non-empty access_token, and coerces expires_in with a 30-day fallback. Binds on a caller-pinned host (default localhost) and port so the redirect URI exact-matches an upstream registration. Existing device-code and authorization-code/PKCE helpers are unchanged.
Add DigitalOcean as a login provider using the OAuth implicit flow. The returned access token is stored as a bare API-key provider on inference.do-ai.run/v1 (no refresh; re-login on ~30-day expiry), and the model catalog is seeded dynamically from the Gradient Inference Routers API as 'router:<name>' aliases. Login still succeeds when the router catalog is unavailable (info event, zero models). Wires platform registration, the managed-models refresh skip-guard, shell /login /logout selectors, and CLI --digitalocean flags, mirroring the xAI provider.
Add Snowflake Cortex as an account-scoped browser-OAuth login provider. Login prompts for a Snowflake account identifier (and optional role), runs loopback-PKCE against the account's https://<account>.snowflakecomputing.com OAuth endpoints (HTTP Basic client creds, role-scoped), and stores an openai_legacy provider on the account's Cortex OpenAI-compatible base_url with a curated model catalog. The account is encoded into the OAuth ref key (oauth/snowflake-cortex/<account>) and parsed back in the refresh dispatch to build the account-scoped token URL. Wires platform registration, the refresh skip-guard, shell /login /logout, and CLI --snowflake --account --role.
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:
📝 WalkthroughWalkthroughAdds OAuth login/logout for GitHub Copilot, DigitalOcean, Snowflake Cortex, and xAI, shared OAuth flow helpers, cached models.dev discovery, atomic persistence, CLI and shell integrations, platform registration, and comprehensive tests. ChangesOAuth providers and model discovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant OAuthFlow
participant Provider
participant Config
User->>CLI: select provider login
CLI->>OAuthFlow: start device-code or loopback flow
OAuthFlow->>Provider: authorize and exchange tokens
Provider->>OAuthFlow: return tokens and model metadata
OAuthFlow->>Config: persist credentials and models
Config-->>CLI: emit success event
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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: 20
🤖 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/copilot.py`:
- Around line 220-223: Make OAuth credential and configuration persistence
failure-atomic across all listed sites. In
src/pythinker_code/auth/copilot.py:220-223, roll back save_tokens and
_apply_copilot_config if save_config fails; at
src/pythinker_code/auth/copilot.py:237-246, persist configuration removal before
deleting credentials. In src/pythinker_code/auth/xai.py:184-186 and :231-233,
stage or roll back browser- and device-login persistence; at :244-253, make
logout cleanup transactional or explicitly preserve partial-state recovery. In
src/pythinker_code/auth/snowflake.py:255-258, stage or roll back login
persistence; at :269-280, preserve recoverability when either logout write
fails.
In `@src/pythinker_code/auth/digitalocean.py`:
- Around line 125-127: Update the login and logout flows around
_apply_digitalocean_config and save_config so configuration mutations are
applied to a copy or protected by a restorable snapshot before persistence.
Catch the expected configuration and I/O failures from save_config, restore or
discard the in-memory mutation, and emit a safe error event. Keep partial or
internal-error states explicit and do not report success unless persistence
completes.
- Around line 67-82: Update _fetch_router_names to preserve distinct
unauthorized, outage, malformed-response, valid-empty, and successful catalog
outcomes instead of returning () for every exception; use the project’s existing
result/error conventions. In tests/auth/test_digitalocean_auth.py lines 93-100,
exercise successful discovery through a fake HTTP response rather than mocking
internal logic. At lines 133-140, route a valid 200 empty catalog through the
HTTP boundary and add separate cases for 401, timeout, and malformed responses,
asserting each distinct outcome.
In `@src/pythinker_code/auth/models_dev.py`:
- Around line 141-146: Update parse_models_dev_catalog and the related
catalog-fetching flow to return a typed result containing the catalog, status,
and source instead of collapsing failures and valid empty data to {}. Preserve
distinct outcomes for invalid input, disabled or absent data, dependency/lock
failures, stale fallback, and valid empty catalogs, and propagate those
status/source values to callers and logs.
- Around line 253-275: Update the lock acquisition flow around _acquire_lock in
the catalog-loading function to use bounded, cancellable nonblocking polling
with a deadline instead of awaiting an uninterruptible asyncio.to_thread call.
Ensure cancellation, timeout, recovery, and debug observability are handled, and
that no lock or descriptor can be acquired orphaned after task cancellation;
preserve the existing cached fallback behavior and add coverage for cancellation
while another process holds the lock.
- Line 4: Update the imports in models_dev.py to avoid unconditionally importing
the Unix-only fcntl module: use a platform-portable locking implementation or
guard the fcntl import and provide an appropriate Windows-compatible path.
Ensure the module imports successfully on Windows while preserving existing
locking behavior on Unix.
In `@src/pythinker_code/auth/oauth_flows.py`:
- Around line 482-487: Update the expiry parsing in oauth_flows.py lines 482-487
to reject malformed, missing, or nonpositive expires_in values with a typed
failure, rather than substituting _DEFAULT_IMPLICIT_EXPIRES_IN. Update
tests/auth/test_oauth_flows.py lines 466-478 to assert the expected typed
failure instead of the 2,592,000-second fallback.
- Around line 214-216: Update the successful-response branch in the OAuth token
parsing flow to require a nonempty string access_token before returning payload.
For any 2xx response with a missing, empty, or non-string access_token, raise
OAuthError instead of returning success; preserve the existing error handling
for responses containing an OAuth error.
- Around line 25-41: Update the inline script in _IMPLICIT_BOOTSTRAP_HTML to
clear the URL fragment immediately after reading window.location.hash and before
the fetch call posts the parsed token. Use browser history replacement so the
access token and related OAuth parameters are removed from the address bar and
history while preserving the current page.
- Around line 612-655: Update the OAuth implicit flow function around
redirect_host and port validation to require a genuine loopback host and reject
port 0. Validate redirect_host as a loopback address or approved localhost name
before starting the server, and require a usable nonzero port so the generated
redirect URI is reachable; fail closed with ValueError for invalid values while
preserving existing callback-path and timeout checks.
- Around line 424-430: Update the OAuth callback handler around
reader.readexactly() to catch asyncio.IncompleteReadError alongside the existing
parsing failures. For a truncated POST body, return HTTP 400 and resolve result
with an OAuthError instead of allowing the callback task to fail; add a
regression test covering this truncated-body behavior.
In `@src/pythinker_code/auth/snowflake.py`:
- Around line 178-203: Prevent _apply_snowflake_config from registering
Snowflake as a usable provider/default model until the chat adapter is
implemented, or mark the registration explicitly unsupported. Update the
readiness path at src/pythinker_code/auth/snowflake.py lines 258-258 to avoid
reporting successful readiness while request transformation is unavailable.
Update tests/auth/test_snowflake_auth.py lines 136-161 to stop asserting
successful setup or default-model selection until adapter support exists.
- Around line 41-57: Update normalize_account() to validate the normalized value
as a plain Snowflake account locator, rejecting path, port, userinfo, query,
fragment, and other authority-delimiter payloads before _authorize_url(),
_token_url(), or _cortex_base_url() build URLs; preserve valid account
normalization. In tests/auth/test_snowflake_auth.py lines 14-26, add coverage
for path, port, userinfo, and delimiter inputs, verifying they are rejected.
In `@src/pythinker_code/auth/xai.py`:
- Around line 174-183: Require a non-empty refresh token before treating xAI
authentication as successful: validate the browser-flow payload before
save_tokens in src/pythinker_code/auth/xai.py lines 174-183, and apply the same
validation to the device-flow response in lines 218-230. Update
tests/auth/test_xai_auth.py lines 21-116 with missing and empty refresh-token
cases, asserting that no configuration is persisted.
- Around line 114-117: Treat OAuth payloads with error code invalid_grant as
OAuthUnauthorized before the generic non-200 fallback in the response-handling
helper in src/pythinker_code/auth/xai.py at lines 114-117 and the corresponding
helper in src/pythinker_code/auth/snowflake.py at lines 139-143, preserving the
existing 401/403 handling and fallback OAuthError behavior.
In `@src/pythinker_code/cli/__init__.py`:
- Around line 1533-1536: Update the login command around the account, role, and
snowflake options to reject any non-empty --account or --role value when
--snowflake is not selected. Perform this validation before loading
configuration or starting the default OpenAI browser flow, and return an
explicit CLI error instead of silently ignoring the options.
In `@tests/auth/test_copilot_auth.py`:
- Around line 118-203: The Copilot token refresh tests only cover success and
unauthorized responses; extend test_refresh_copilot_token_raises_unauthorized
coverage with cases for non-JSON responses, incomplete payloads, non-auth HTTP
failures, timeouts, and aiohttp.ClientError. Assert each produces the
appropriate typed error and verify failed refreshes do not persist tokens,
reusing the existing response/session fixtures and OAuth manager persistence
path where needed.
In `@tests/auth/test_snowflake_auth.py`:
- Around line 51-70: Update the test around refresh_snowflake_cortex_token to
mock the HTTP boundary via snowflake.new_client_session instead of replacing the
private _post_form helper. Make the fake session exercise form encoding,
propagated headers, JSON parsing, and status handling, and add coverage for
malformed JSON and HTTP 400 responses containing invalid_grant.
- Around line 14-26: Expand test_normalize_account with hostile inputs covering
authority/path delimiters, ports, userinfo, uppercase schemes, query and
fragment components, and noncanonical casing. Assert malformed or unsafe account
values are rejected or normalized to the documented canonical form, and verify
invalid inputs fail before any browser or HTTP action.
In `@tests/auth/test_xai_auth.py`:
- Around line 21-116: Extend the xAI authentication tests around
login_xai_browser and login_xai_headless to cover token responses with an absent
or empty refresh_token and HTTP 400 invalid_grant refresh failures. Assert each
flow emits the expected error event and leaves neither OAuth credentials nor the
managed:xai provider persisted in configuration or storage.
🪄 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: 5dae9d6e-cea1-430b-b0d3-e6e3350cd0b4
⛔ Files ignored due to path filters (3)
docs/en/release-notes/changelog.mdis excluded by!docs/**tasks/lessons.mdis excluded by!tasks/**tasks/todo.mdis excluded by!tasks/**
📒 Files selected for processing (23)
CHANGELOG.mdsrc/pythinker_code/auth/__init__.pysrc/pythinker_code/auth/copilot.pysrc/pythinker_code/auth/digitalocean.pysrc/pythinker_code/auth/models_dev.pysrc/pythinker_code/auth/oauth.pysrc/pythinker_code/auth/oauth_flows.pysrc/pythinker_code/auth/opencode_go.pysrc/pythinker_code/auth/platforms.pysrc/pythinker_code/auth/snowflake.pysrc/pythinker_code/auth/xai.pysrc/pythinker_code/cli/__init__.pysrc/pythinker_code/ui/shell/oauth.pytests/auth/test_copilot_auth.pytests/auth/test_digitalocean_auth.pytests/auth/test_models_dev.pytests/auth/test_oauth_flows.pytests/auth/test_opencode_go_auth.pytests/auth/test_platforms.pytests/auth/test_snowflake_auth.pytests/auth/test_xai_auth.pytests/cli/test_openai_login_cli.pytests/ui_and_conv/test_shell_slash_commands.py
Address review findings on the shared OAuth helpers and login command: - poll_device_token: reject a 2xx success payload that carries no usable access token instead of returning it as success. - Implicit loopback callback: catch truncated request bodies (IncompleteReadError) and fail closed with HTTP 400 rather than hanging until timeout; scrub the bearer token from the browser URL fragment via history.replaceState before it is posted. - Implicit expiry: represent missing/malformed/nonpositive expires_in as unknown (None) rather than fabricating a trusted 30-day lifetime. - run_loopback_implicit_flow: require a genuine loopback redirect_host and a nonzero port, failing closed on non-loopback binds. - login: reject --account/--role unless --snowflake is selected. Also make three implicit-callback assertions effectful to satisfy the static analyzer.
…al API Rework the shared models.dev catalog so any provider can consume it and so degraded data is never mistaken for authoritative: - get_models_dev_catalog now returns a typed CatalogResult (catalog + status + source), distinguishing fresh, cached, stale, disabled, and unavailable outcomes instead of collapsing every state into an empty dict. - Replace the unconditional Unix-only fcntl import and blocking, uninterruptible lock acquisition with a cross-platform (fcntl/msvcrt) non-blocking lock that is bounded by a deadline, cancellation-safe, and never orphans a descriptor. - Add a provider-neutral chat-model filter (text-output modality, excluding embedding/reranker/moderation ids) and build_catalog_models() so each provider can resolve its own catalog entries without opencode-specific coupling. opencode_go consumes the new CatalogResult shape.
…ned logins Rewire xAI, GitHub Copilot, Snowflake Cortex, and DigitalOcean onto the shared, provider-neutral models.dev catalog and make their login/logout robust: - Each provider now discovers its own models generically (xAI/Copilot/Snowflake from their models.dev provider id, filtered to chat models; DigitalOcean from its native Inference Routers API), falling back to a curated list when the catalog is unavailable or degraded — no opencode coupling. - Persist login/logout atomically via shared helpers: a login writes tokens, applies config, then saves; on failure it rolls back the token and restores the in-memory config. Logout persists the config removal before deleting credentials so an interruption leaves recoverable creds, not orphaned config. - xAI/Snowflake: map 400 invalid_grant to unauthorized so rejected refresh tokens are suppressed; require a refresh token before reporting login success. - Snowflake: validate the account locator (reject authority/path/port/userinfo/ query/fragment payloads) before building any URL; register models without making Snowflake the default, since its Cortex chat adapter is not yet wired. - DigitalOcean: report router-discovery outcomes (unauthorized, outage, malformed, empty) distinctly instead of collapsing them into an empty catalog. Expand tests to cover malformed/transport-failure responses, hostile account input, missing refresh tokens, invalid_grant, catalog-driven registration, and the distinct router-discovery outcomes. Also revert an unrelated prompt.py change that had been picked up inadvertently.
…anch An in-progress prompt_toolkit screen-mode refactor (prompt.py, config.py sticky_input docs, and the prompt/erase-when-done tests) was captured from the working tree by an earlier auth commit, leaving the tests ahead of the reverted source and failing four prompt tests. Restore the entire prompt subsystem and the config docstring to origin/main so the auth PR carries only auth changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pythinker_code/auth/snowflake.py (1)
278-305: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMalformed token-exchange response crashes instead of failing closed.
OAuthToken.from_response(payload)at line 298 runs outside thetry/except OAuthErrorblock (lines 278–296).from_responsedoespayload["access_token"]— if Snowflake ever returns a 200 response missingaccess_token(malformed/degraded upstream), this raises an uncaughtKeyErrorfrom the async generator instead of a gracefulOAuthEvent("error", ...)._post_tokendoesn't validate that a 200 payload containsaccess_tokeneither, so nothing upstream guards this. No test currently covers a missing-access_tokenexchange response (only missingrefresh_tokenis tested).🐛 Proposed fix: bring token construction inside the guarded block
try: auth = await run_loopback_pkce_flow( authorize_endpoint=_authorize_url(account), client_id=SNOWFLAKE_CLIENT_ID, scope=_scope(role), redirect_path=SNOWFLAKE_REDIRECT_PATH, port=0, extra_authorize_params=None, browser_open=browser_open, ) payload = await _exchange_code_for_tokens( account, auth.authorization_code, auth.code_verifier, auth.redirect_uri, ) + token = OAuthToken.from_response(payload) except OAuthError as exc: yield OAuthEvent("error", f"Snowflake Cortex browser login failed: {exc}") return + except (KeyError, TypeError, ValueError) as exc: + yield OAuthEvent("error", f"Snowflake Cortex returned an invalid token response: {exc}") + return - - token = OAuthToken.from_response(payload) if not token.refresh_token:As per coding guidelines, "handle invalid, malformed, unauthorized, expired, timed-out, duplicated, concurrent, partial, cancelled, and retry-exhausted cases explicitly; never silently ignore unexpected states" applies directly to this malformed-response path.
🤖 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/snowflake.py` around lines 278 - 305, Move OAuthToken.from_response(payload) into the existing try block that catches OAuthError, and ensure malformed token responses such as a missing access_token are converted into the existing Snowflake browser-login error OAuthEvent instead of escaping the async generator. Preserve the current refresh-token validation and error behavior.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/copilot.py`:
- Around line 254-260: Update the exception handlers in login_copilot and
logout_copilot around persist_login and persist_logout to emit an actionable
server-side log containing the exception and operation context, while preserving
the existing safe OAuthEvent error messages and returns.
- Around line 128-142: Update _discover_copilot_models to emit an info-level log
when result.is_authoritative is false before returning GITHUB_COPILOT_MODELS,
identifying that discovery is using the curated fallback because the catalog is
degraded or unavailable. Keep the existing authoritative catalog conversion and
fallback behavior unchanged.
In `@src/pythinker_code/auth/oauth.py`:
- Around line 497-519: Update persist_login to snapshot the existing token for
ref before save_tokens, then restore that token in the exception path when the
login/config persistence fails; only delete tokens when no prior credential
existed. Preserve restoring the in-memory config and re-raising the original
failure, and ensure token restoration errors are handled consistently with the
existing cleanup behavior.
In `@src/pythinker_code/auth/opencode_go.py`:
- Around line 274-275: Update the catalog-loading flow to pass result.catalog
directly to _metadata_from_catalog instead of _parse_models_dev_metadata,
preserving the normalized ModelsDevProvider objects and their metadata. Add a
test covering a successful get_models_dev_catalog result and verifying the
returned metadata is populated.
In `@src/pythinker_code/auth/xai.py`:
- Around line 107-120: Update _discover_xai_models to log when
result.is_authoritative is false before returning XAI_MODELS, including that the
fallback list is being used and identifying the catalog as unavailable or
non-authoritative. Follow the logging approach used by _discover_copilot_models
without changing the existing fallback behavior.
- Around line 222-236: The persist_login and persist_logout exception handlers
in the xAI OAuth flows catch failures without server-side logging. Update the
handlers around persist_login and persist_logout to log the caught exception
with actionable context before yielding the existing safe OAuthEvent error
messages, covering all affected login, logout, and configuration persistence
paths.
In `@tests/auth/test_models_dev.py`:
- Around line 145-154: Extend
test_build_catalog_models_applies_default_context_and_ordering with a
text-output model fixture that omits limit.context, then build the corresponding
catalog and assert its max_context_size resolves to the supplied default_context
value of 100_000. Preserve the existing chat-model ordering and missing-catalog
assertions.
---
Outside diff comments:
In `@src/pythinker_code/auth/snowflake.py`:
- Around line 278-305: Move OAuthToken.from_response(payload) into the existing
try block that catches OAuthError, and ensure malformed token responses such as
a missing access_token are converted into the existing Snowflake browser-login
error OAuthEvent instead of escaping the async generator. Preserve the current
refresh-token validation and error behavior.
🪄 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: 36c88334-51a0-456b-8f6a-b23554394612
⛔ Files ignored due to path filters (1)
docs/en/release-notes/changelog.mdis excluded by!docs/**
📒 Files selected for processing (17)
CHANGELOG.mdsrc/pythinker_code/auth/copilot.pysrc/pythinker_code/auth/digitalocean.pysrc/pythinker_code/auth/models_dev.pysrc/pythinker_code/auth/oauth.pysrc/pythinker_code/auth/oauth_flows.pysrc/pythinker_code/auth/opencode_go.pysrc/pythinker_code/auth/snowflake.pysrc/pythinker_code/auth/xai.pysrc/pythinker_code/cli/__init__.pytests/auth/test_copilot_auth.pytests/auth/test_digitalocean_auth.pytests/auth/test_models_dev.pytests/auth/test_oauth_flows.pytests/auth/test_opencode_go_auth.pytests/auth/test_snowflake_auth.pytests/auth/test_xai_auth.py
…istence errors - persist_login now snapshots the existing token via load_tokens and restores it (rather than blind-deleting) when the config save fails, so an unrelated save error during re-login no longer destroys a still-valid credential; a fresh login with no prior token still deletes on failure. Adds direct regression tests for the atomicity helper (fails before, passes after). - Log provider login/logout persistence failures across all four providers so unexpected errors leave a diagnosable trail alongside the safe user message. - Log the curated-model fallback in xai/copilot discovery when the models.dev catalog is not authoritative, matching DigitalOcean's degraded-discovery signal. - Add a success-path test proving _fetch_models_dev_metadata populates metadata from an authoritative catalog, and a default-context fallback test for build_catalog_models. - Drop the changelog entry for the reverted prompt-fullscreen feature and note the credential-preservation fix.
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/copilot.py`:
- Around line 135-140: The curated fallback paths in _catalog_models_to_copilot
and _catalog_models_to_xai need explicit debug logs when catalog conversion
returns no provider models. In src/pythinker_code/auth/copilot.py lines 135-140,
log the fallback when _catalog_models_to_copilot(built) is empty; apply the
equivalent log in src/pythinker_code/auth/xai.py lines 116-121 when
_catalog_models_to_xai(built) is empty, while preserving the existing fallback
behavior.
In `@src/pythinker_code/auth/oauth.py`:
- Around line 512-517: Update the helper containing the snapshot, token, and
config persistence flow to execute the complete serialized unit—including
load_tokens, save_tokens, apply_config, and save_config—off the async event loop
via the project’s existing executor/thread mechanism. Preserve the current
ordering and rollback behavior, and propagate any persistence failure unchanged
to the async provider login callers.
- Around line 512-524: Serialize the snapshot, token persistence, configuration
update, and rollback in the oauth transaction around the existing
apply_config/save_config flow using an inter-process lock scoped to the shared
config file, not only an in-process or per-ref lock. Update
tests/auth/test_oauth_persist.py lines 27-96 with an ordered concurrent-login
test that deterministically verifies a failed login cannot roll back a
successful login; both the source and test sites require changes.
In `@tests/auth/test_opencode_go_auth.py`:
- Around line 251-298: Extend
test_fetch_models_dev_metadata_uses_authoritative_catalog with a
CatalogStatus.UNAVAILABLE scenario by mocking get_models_dev_catalog to return
an unavailable CatalogResult, then assert _fetch_models_dev_metadata() returns
an empty dictionary. Preserve the existing authoritative catalog assertions.
🪄 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: 6f95e683-6bf0-44a8-81fb-34b7125188bf
⛔ Files ignored due to path filters (1)
docs/en/release-notes/changelog.mdis excluded by!docs/**
📒 Files selected for processing (9)
CHANGELOG.mdsrc/pythinker_code/auth/copilot.pysrc/pythinker_code/auth/digitalocean.pysrc/pythinker_code/auth/oauth.pysrc/pythinker_code/auth/snowflake.pysrc/pythinker_code/auth/xai.pytests/auth/test_models_dev.pytests/auth/test_oauth_persist.pytests/auth/test_opencode_go_auth.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pythinker_code/auth/copilot.py (1)
270-275: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDo not expose persistence exceptions in OAuth events.
Awaiting persistence makes both handlers reachable on failure; interpolating
exccan disclose filesystem details or other internal diagnostics. Keep the detailed warning log, but return fixed safe messages.
src/pythinker_code/auth/copilot.py#L270-L275: replace the login error event text with a generic save-failure message.src/pythinker_code/auth/copilot.py#L300-L303: replace the logout error event text with a generic logout-failure message.As per coding guidelines, “Use typed or categorized errors, actionable logging, safe user-facing messages, and never leak secrets, tokens, PII, or stack traces.”
🤖 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/copilot.py` around lines 270 - 275, Replace the exception-interpolated OAuth error event in the login handler around persist_login with a fixed generic save-failure message while retaining the detailed logger.warning call. Apply the same change to the logout handler at src/pythinker_code/auth/copilot.py lines 300-303, using a fixed generic logout-failure message and never exposing exc in either OAuthEvent.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.
Outside diff comments:
In `@src/pythinker_code/auth/copilot.py`:
- Around line 270-275: Replace the exception-interpolated OAuth error event in
the login handler around persist_login with a fixed generic save-failure message
while retaining the detailed logger.warning call. Apply the same change to the
logout handler at src/pythinker_code/auth/copilot.py lines 300-303, using a
fixed generic logout-failure message and never exposing exc in either
OAuthEvent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1dda6474-cc07-4148-9b24-124f308c53c5
⛔ Files ignored due to path filters (1)
docs/superpowers/specs/2026-07-18-pr-215-auth-review-fixes-design.mdis excluded by!docs/**
📒 Files selected for processing (16)
src/pythinker_code/auth/copilot.pysrc/pythinker_code/auth/digitalocean.pysrc/pythinker_code/auth/oauth.pysrc/pythinker_code/auth/oauth_flows.pysrc/pythinker_code/auth/snowflake.pysrc/pythinker_code/auth/xai.pysrc/pythinker_code/config.pysrc/pythinker_code/tools/lsp/tool.pysrc/pythinker_code/utils/io.pytests/auth/test_copilot_auth.pytests/auth/test_digitalocean_auth.pytests/auth/test_oauth_flows.pytests/auth/test_oauth_persist.pytests/auth/test_snowflake_auth.pytests/auth/test_xai_auth.pytests/core/test_config_atomic_save.py
Member
Author
|
Addressed CodeRabbit’s current-head outside-diff persistence-event finding in a5f0dbe. OAuth login/logout handlers now retain detailed internal logs while returning fixed safe failure messages across GitHub Copilot, DigitalOcean, Snowflake Cortex, and both xAI login modes. Added provider-wide regressions; 454 auth tests, |
elkaix
added a commit
that referenced
this pull request
Jul 19, 2026
…fix (#217) - Applies the PR #215 OAuth review remediation: scoped token refresh, lock-serialized atomic credential persistence, rollback on failure, fail-closed migration, a reentrant credential lock that serializes the keyring migration without deadlocking, and OAuth response validation, with comprehensive tests. - Fixes the intermittent running-prompt handoff ghost by awaiting the re-requested absolute-cursor CPR to settle before the suppressed live body re-expands. - Constructs OAuthManager off the event loop (cancellation-safe); gitignores /tasks/ working files.
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
Adds four new managed OAuth login providers plus the shared plumbing they need, extending Pythinker's multi-provider auth without changing the existing per-provider pattern:
pythinker login --snowflake), with per-account base URL and refresh keyed onoauth/snowflake-cortex/<account>.Supporting changes:
oauth_flows.py: newrun_loopback_implicit_flowhelper (OAuth implicit flow over a pinned loopback port) alongside the existing PKCE/device-code helpers.models_dev.py: cached, provider-agnostic models.dev catalog for dynamic model-metadata discovery.platforms.py/auth/__init__.py: platform records + managed-model skip guards for the new providers.--xai,--copilot,--digitalocean,--snowflake,--account,--role) and shell/login//logoutmodes wired to mirror the existing provider surface.Each provider derives its provider key from the active model (no hard-coded provider fan-out), fails closed on missing refresh tokens, and never leaks credentials in events or logs.
Verification
make check-pythinker-code— ruff + format + pyright + ty, 0 errors.make test-pythinker-code—tests+tests_e2e, 65 passed, 4 skipped (real-LLM only). No snapshot movement.New/updated focused tests:
tests/auth/test_{xai,copilot,digitalocean,snowflake,oauth_flows,platforms,models_dev}.py,tests/cli/test_openai_login_cli.py,tests/ui_and_conv/test_shell_slash_commands.py.Notes
max_tokens→max_completion_tokens, streaming role normalization) are not replicated by theopenai_legacyprovider type; this affects live chat, not login, and is tracked as follow-up.Changelog
## Unreleasedentries added for all five user-facing additions.Summary by CodeRabbit