Add native Player2 provider with assisted login (local app detection + device code flow)#165
Add native Player2 provider with assisted login (local app detection + device code flow)#165CarlosNahuelcoy wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used🪛 Ruff (0.15.21)src/rotator_library/providers/utilities/player2_auth.py[warning] 157-160: Avoid specifying long messages outside the exception class (TRY003) [warning] 162-164: Avoid specifying long messages outside the exception class (TRY003) [warning] 166-168: Avoid specifying long messages outside the exception class (TRY003) 🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Player2 authentication utilities, an OpenAI-compatible provider, an assisted credential wizard, and related environment and README guidance. The integration supports local app detection, device-code login, streaming completions, error mapping, and numbered credential storage. ChangesPlayer2 Integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CredentialTool
participant Player2Auth
participant Player2API
participant EnvFile
User->>CredentialTool: select Player2 assisted login
CredentialTool->>Player2Auth: request p2Key
Player2Auth->>Player2API: perform device-code flow when local app is unavailable
Player2API-->>CredentialTool: return p2Key
CredentialTool->>EnvFile: write next numbered Player2 API key
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
| Filename | Overview |
|---|---|
| src/rotator_library/providers/utilities/player2_auth.py | Adds Player2 local-app and device-code authentication helpers. |
| src/rotator_library/credential_tool.py | Adds the Player2 assisted-login path to the credential wizard. |
| src/rotator_library/providers/player2_provider.py | Adds Player2 completion handling, streaming support, and Player2-specific error mapping. |
Reviews (3): Last reviewed commit: "Wrap device flow init request in Player2..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Starting my review of the new Player2 provider and assisted-login wizard. I'll dig into the auth flow, provider logic, and credential-tool wiring, then post a bundled review shortly. |
![mirrobot-agent[bot]](https://avatars.githubusercontent.com/in/2140342?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
A really strong first contribution. The Player2 provider closely follows the established patterns (it mirrors the DeepSeek provider's structure for custom completion logic, and the auto-registration via player2_provider.py → "player2" works without any manual plugin wiring). The assisted-login wizard is a thoughtful UX touch — auto-detecting the local desktop app before falling back to the device flow is exactly right for the game-mod audience, and the headless-friendly device flow keeps it usable in Docker/SSH. Documentation throughout (module docstrings, the README row, .env.example) is thorough and clearly explains why things behave the way they do.
Merge Prerequisite
The PR body and code comments call this out clearly, but flagging it here too so it's not lost: DEFAULT_CLIENT_ID is tied to a personal/contributor Player2 account. It should be replaced with a project-owned Game Client ID (registered via the Player2 Developer Dashboard) before this merges. It's a one-line change, and the PLAYER2_CLIENT_ID env override is already in place for self-hosters.
Key Suggestions
- 402 → cooldown churn: Insufficient credits is persistent, so surfacing it as a plain
RateLimitErrorlikely yields a short cooldown and rapid retries against an out-of-credits key. Implementingparse_quota_error/force_exhaustedwould align it with how the project handles real quota exhaustion (see inline). - Device-flow robustness: A defensive
verificationUrilookup and RFC 8628slow_downhandling would make the under-documented token endpoint behave gracefully (see inline).
Minor Points
_api_basedoesn't honor aPLAYER2_API_BASEenv var, breaking consistency with the*_API_BASEconvention other providers follow.- The deliberate
extra_bodyfiltering is worth a one-line "why" comment.
Questions for the Author
- For 402/insufficient-credits, is there a known Player2 behavior (e.g. a reset timestamp or Retry-After) we could parse to set a more accurate cooldown?
- Has the device-flow token endpoint been observed returning anything other than 200/500 in practice (e.g. an explicit "pending" status)? Confirming that would resolve the RFC-assumption noted in the docstring.
Nice work overall — once the Client ID is swapped and the 402 handling is tightened up, this is in great shape.
This review was generated by an AI assistant.
| if response.status_code == 402: | ||
| # Insufficient credits - treat like a quota/rate-limit condition | ||
| # so the credential is rotated instead of the request just | ||
| # failing outright. | ||
| raise RateLimitError( | ||
| f"Player2 insufficient credits: {error_text}", | ||
| llm_provider="player2", | ||
| model=model, | ||
| response=response, | ||
| ) |
There was a problem hiding this comment.
Mapping 402 (insufficient credits) to RateLimitError makes the credential eligible for rotation, but insufficient credits is a persistent condition that won't clear after a short rate-limit cooldown. Without a Retry-After header, the proxy will likely apply the default short cooldown and retry the out-of-credits key again soon, causing churn. Consider implementing parse_quota_error() (and/or on_request_complete with force_exhausted=True) so an exhausted-credits credential gets a long cooldown until the next billing cycle rather than being retried like a transient 429.
|
|
||
| device_code = data["deviceCode"] | ||
| user_code = data["userCode"] | ||
| verification_uri = data.get("verificationUriComplete") or data["verificationUri"] |
There was a problem hiding this comment.
If /login/device/new omits both verificationUriComplete and verificationUri, this raises an opaque KeyError instead of a helpful Player2AuthError. Since you note the API surface isn't fully documented, a defensive .get() with an explicit error would give users a clear message:
| verification_uri = data.get("verificationUriComplete") or data["verificationUri"] | |
| verification_uri = data.get("verificationUriComplete") or data.get("verificationUri") | |
| if not verification_uri: | |
| raise Player2AuthError("Player2 did not return a verification URI.") |
| # Anything else (400/403/404/428, etc.) is treated as "authorization | ||
| # pending" per RFC 8628 convention. See the docstring note above. | ||
| lib_logger.debug( | ||
| f"Player2 device login still pending (status {token_resp.status_code})." | ||
| ) |
There was a problem hiding this comment.
You're aligning to RFC 8628 for the pending status, but the RFC also defines a slow_down error (HTTP 400 with "error": "slow_down") that signals the client must increase its polling interval by ~5s. This loop treats every non-200/500 response as pending and keeps the same interval, so repeated polls could get rejected if Player2 ever enforces slow_down. Worth handling that case (or confirming Player2 never sends it) since you're already following the RFC convention.
| def _api_base(self, override: Optional[str] = None) -> str: | ||
| return (override or DEFAULT_API_BASE).rstrip("/") |
There was a problem hiding this comment.
Unlike the sibling DeepSeek provider, _api_base doesn't consult a PLAYER2_API_BASE env var, so a self-hoster pointing at a mirror/preview endpoint can't override the base (the override arg is only populated if the framework injects api_base into kwargs). Falling back to the env var would keep this consistent with the project's *_API_BASE convention:
| def _api_base(self, override: Optional[str] = None) -> str: | |
| return (override or DEFAULT_API_BASE).rstrip("/") | |
| def _api_base(self, override: Optional[str] = None) -> str: | |
| return (override or os.getenv("PLAYER2_API_BASE") or DEFAULT_API_BASE).rstrip("/") |
| extra_body = kwargs.get("extra_body") | ||
| if isinstance(extra_body, dict): | ||
| payload.update({k: v for k, v in extra_body.items() if k in SUPPORTED_PARAMS}) |
There was a problem hiding this comment.
Filtering extra_body to SUPPORTED_PARAMS differs from the comparable DeepSeek provider, which passes extra_body through wholesale. That's likely intentional given Player2's strict schema, but it does mean any field routed through extra_body is silently dropped unless it's also in the set — easy to misdiagnose. A short comment here noting the deliberate restriction (and that new Player2 params must be added to SUPPORTED_PARAMS) would save future debuggers some head-scratching.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 @.env.example:
- Around line 55-60: Document the optional PLAYER2_CLIENT_ID override alongside
PLAYER2_API_KEY_1 in the Player2 configuration section, explaining that it can
be set for project- or fork-specific Player2 registrations and showing the
expected placeholder format.
In `@src/rotator_library/credential_tool.py`:
- Around line 1188-1190: Update the client ID resolution in the relevant
credential initialization logic to check the process environment variable
PLAYER2_CLIENT_ID before falling back to the .env file value and
DEFAULT_CLIENT_ID. Preserve existing behavior when the environment variable is
unset, and use the existing env_file/get_key flow as the subsequent fallback.
- Around line 1200-1201: Update the manual p2Key prompt in the choice-2 branch
to pass password=True to Prompt.ask, while preserving the existing formatting,
stripping, and None fallback.
In `@src/rotator_library/providers/player2_provider.py`:
- Around line 153-156: Filter and map the Player2 provider payload to only
supported LiteLLM fields before constructing responses in the response handling
logic and streaming chunk path. Update the code around
litellm.ModelResponse(**response_data) and litellm.ModelResponseStream(**chunk)
to discard provider-specific fields while preserving valid schema data, then
construct the LiteLLM objects from the sanitized payloads.
In `@src/rotator_library/providers/utilities/player2_auth.py`:
- Line 57: Replace the hard-coded personal client ID in DEFAULT_CLIENT_ID with a
newly registered, project-owned Player2 client ID, ensuring all authentication
flows use the project credentials.
- Around line 133-142: The device-token polling logic should not treat all
non-500 responses as pending. In the device authorization polling
implementation, whitelist only Player2’s confirmed pending response, fail
immediately on denial, expiry, invalid-client, and other terminal errors, and
handle slow_down by increasing the poll interval before retrying. Also validate
successful responses and raise an error when a 200 response lacks p2Key.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1757097c-e368-4b96-bc10-27848a206d4b
📒 Files selected for processing (5)
.env.exampleREADME.mdsrc/rotator_library/credential_tool.pysrc/rotator_library/providers/player2_provider.pysrc/rotator_library/providers/utilities/player2_auth.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: CodeRabbit / Review
- GitHub Check: Greptile Review
- GitHub Check: review-pr
🧰 Additional context used
🪛 ast-grep (0.44.1)
src/rotator_library/credential_tool.py
[warning] 1245-1245: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(env_file, "r")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 Ruff (0.15.20)
src/rotator_library/providers/utilities/player2_auth.py
[warning] 35-35: Import from collections.abc instead: Callable
Import from collections.abc
(UP035)
[warning] 99-100: Logging statement uses f-string
(G004)
[warning] 103-103: Logging statement uses f-string
(G004)
[warning] 104-104: Do not catch blind exception: Exception
(BLE001)
[warning] 105-105: Logging statement uses f-string
(G004)
[warning] 181-183: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 188-188: Logging statement uses f-string
(G004)
[warning] 191-193: Avoid specifying long messages outside the exception class
(TRY003)
src/rotator_library/providers/player2_provider.py
[warning] 33-33: Import from collections.abc instead: AsyncGenerator
Import from collections.abc
(UP035)
[warning] 33-33: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 33-33: typing.List is deprecated, use list instead
(UP035)
[warning] 74-74: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
[warning] 80-80: Unused method argument: api_key
(ARG002)
[warning] 80-80: Unused method argument: client
(ARG002)
[warning] 113-113: Missing type annotation for **kwargs
(ANN003)
[warning] 114-117: Use X | Y for type annotations
Convert to X | Y
(UP007)
[warning] 188-188: Logging statement uses f-string
(G004)
[warning] 202-207: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 213-218: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 220-224: Avoid specifying long messages outside the exception class
(TRY003)
src/rotator_library/credential_tool.py
[warning] 1154-1154: Missing return type annotation for private function _setup_player2_credential
Add return type annotation: None
(ANN202)
[error] 1221-1222: try-except-pass detected, consider logging the exception
(S110)
[warning] 1221-1221: Do not catch blind exception: Exception
(BLE001)
[warning] 1230-1230: Do not catch blind exception: Exception
(BLE001)
[warning] 1246-1246: Async functions should not open files with blocking methods like open
(ASYNC230)
[warning] 1246-1246: Unnecessary mode argument
Remove mode argument
(UP015)
[warning] 1551-1551: Blocking call to input() in async context
(ASYNC250)
🔇 Additional comments (6)
src/rotator_library/providers/player2_provider.py (2)
99-110: LGTM!
194-224: LGTM!src/rotator_library/providers/utilities/player2_auth.py (2)
1-36: LGTM!Also applies to: 38-56, 59-132, 143-170, 190-224
37-37: 🩺 Stability & Availability
httpxis already declared inrequirements.txt> Likely an incorrect or invalid review comment.src/rotator_library/credential_tool.py (1)
12-12: LGTM!Also applies to: 1154-1187, 1192-1199, 1202-1265, 1545-1553
README.md (1)
266-266: LGTM!
| response_data = response.json() | ||
| response_data["model"] = model | ||
| file_logger.log_final_response(response_data) | ||
| return litellm.ModelResponse(**response_data) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check litellm ModelResponse definition for extra field handling
rg -n "class ModelResponse" --type=py -g '!**/test/**' -A 20 | head -80Repository: Mirrowel/LLM-API-Key-Proxy
Length of output: 164
🌐 Web query:
Does litellm ModelResponse ignore unexpected kwargs when constructed with **kwargs?
💡 Result:
No, LiteLLM's ModelResponse class does not ignore unexpected keyword arguments when constructed with kwargs [1][2]. The ModelResponse class typically inherits from a base class (such as OpenAIObject) which is designed to accept and store additional parameters [1][2]. In the class implementation, the init method accepts params as a catch-all for keyword arguments and passes them to the parent class constructor via super.init(params) [1][2]. This means any extra keyword arguments provided during instantiation are generally captured and stored as attributes on the object rather than being ignored or causing an error [1][2]. However, in the broader context of LiteLLM, passing unsupported parameters to completion functions often leads to errors unless specifically handled [3]. LiteLLM provides a mechanism to drop unsupported parameters (via drop_params=True) to prevent these exceptions [3]. While the ModelResponse class itself captures extra arguments, issues in the library's handling of completion parameters often arise when these arguments are passed through to the underlying LLM provider APIs [4][5][6]. In summary, while the ModelResponse object itself will collect these extra keyword arguments [1][2], using them in the context of API completion calls may result in errors if the provider does not support them, unless you explicitly configure the request to drop unsupported parameters [3].
Citations:
- 1: https://github.com/BerriAI/litellm/blob/d7e58d13bf9ba9edbab2ab2f096f3de7547f35fa/litellm/utils.py
- 2: https://github.com/BerriAI/litellm/blob/1a6ea20a0bb66491968907c2bfaabb7fe45fc064/litellm/utils.py
- 3: https://docs.litellm.ai/docs/completion/drop_params
- 4: [Bug]: completion kwargs interfere with response parameters BerriAI/litellm#16809
- 5: Custom_openaiException - AsyncCompletions.create() got an unexpected keyword argument 'client_metadata' BerriAI/litellm#28539
- 6: [Bug]: Litellm proxy incorrectly handles
extra_bodyparameter for OpenAI client BerriAI/litellm#18039
🌐 Web query:
litellm ModelResponse ModelResponseStream source extra kwargs __init__ unexpected keyword arguments
💡 Result:
The errors you are encountering related to "unexpected keyword arguments" in LiteLLM's ModelResponse or ModelResponseStream typically stem from attempts to initialize these Pydantic-based classes with fields that are not defined in their rigid schema [1][2][3]. LiteLLM has undergone significant refactoring to enforce stricter schema validation for these response objects to ensure consistency across providers and observability tools [1][2]. Key reasons and solutions include: 1. Rigid Schema Enforcement: LiteLLM's ModelResponse and ModelResponseStream are Pydantic models. They do not support arbitrary kwargs during initialization [1][2]. If you pass a keyword argument that is not explicitly defined in the class (e.g., passing "image" to an ImageResponse or unexpected fields to ModelResponse), Python will raise a TypeError: init() got an unexpected keyword argument [4]. 2. Schema Mismatches: In older versions, certain fields (like stream=True) were sometimes used in ModelResponse.init as workarounds, but recent updates have removed these or migrated them to dedicated classes like ModelResponseStream [2]. Always ensure you are using the correct class for the response type you are handling [2][5][6]. 3. Handling Provider-Specific Data: If you need to store extra information from a provider that does not fit into the standard schema, do not attempt to pass it to the init method. Instead, use the provider_specific_fields attribute (if available on the object) or handle it separately after instantiation [3]. LiteLLM provides internal helpers like _coerce_provider_specific_fields to manage this data without triggering Pydantic validation errors [3]. 4. Troubleshooting Tips: - Verify Versions: Ensure you are using the latest version of LiteLLM, as many of these "unexpected argument" issues were addressed in recent PRs that cleaned up type definitions and class initialization [2][3]. - Use Proper Factories: Rather than manual instantiation, use LiteLLM's internal conversion utilities (e.g., convert_to_model_response_object or convert_to_streaming_response_async) when processing dictionaries from API responses [4][5]. These utilities are designed to map raw provider data to the correct LiteLLM response types safely [5]. - Check for "Extra" Data: If you are encountering this error when using libraries that wrap LiteLLM, check if they are passing excess arguments in their own ModelResponse calls. They may need to be updated to filter these arguments before initialization [7].
Citations:
- 1: feat(core): enforce strict schema validation for Usage via TypeAdapter to improve SDK quality BerriAI/litellm#19546
- 2: fix(types): remove StreamingChoices from ModelResponse, use ModelResponseStream BerriAI/litellm#21629
- 3: fix: : PydanticSerializationUnexpectedValue warnings during streaming BerriAI/litellm#23346
- 4: [Feature]: Support for AzureAI Image Generation Models BerriAI/litellm#10057
- 5: https://github.com/BerriAI/litellm/blob/e15b37a1/litellm/litellm_core_utils/llm_response_utils/convert_dict_to_response.py
- 6: https://github.com/BerriAI/litellm/blob/e15b37a1/litellm/llms/base_llm/base_model_iterator.py
- 7: https://github.com/openai/openai-agents-python/blob/3a11cf52/src/agents/extensions/models/litellm_model.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/rotator_library/providers/player2_provider.py"
echo "== outline =="
ast-grep outline "$file" --view expanded
echo
echo "== relevant lines =="
sed -n '110,210p' "$file" | cat -nRepository: Mirrowel/LLM-API-Key-Proxy
Length of output: 5236
Filter the Player2 payload before constructing LiteLLM responses. litellm.ModelResponse(**response_data) and litellm.ModelResponseStream(**chunk) reject unexpected kwargs, so any provider-specific field will raise here. Map the payload to the LiteLLM schema first.
🤖 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/rotator_library/providers/player2_provider.py` around lines 153 - 156,
Filter and map the Player2 provider payload to only supported LiteLLM fields
before constructing responses in the response handling logic and streaming chunk
path. Update the code around litellm.ModelResponse(**response_data) and
litellm.ModelResponseStream(**chunk) to discard provider-specific fields while
preserving valid schema data, then construct the LiteLLM objects from the
sanitized payloads.
| # project (not a contributor's personal account) owns and controls it going | ||
| # forward. Forks/self-hosters can also override it via the PLAYER2_CLIENT_ID | ||
| # environment variable without touching this file. | ||
| DEFAULT_CLIENT_ID = "019f426e-760b-7ba6-9009-fe85034c9057" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace the contributor-owned client ID before merge.
Line 57 leaves production authentication controlled by a personal Player2 account. Register and use a project-owned client ID as required by the PR objective.
🤖 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/rotator_library/providers/utilities/player2_auth.py` at line 57, Replace
the hard-coded personal client ID in DEFAULT_CLIENT_ID with a newly registered,
project-owned Player2 client ID, ensuring all authentication flows use the
project credentials.
…2_API_BASE/env client ID, hide manual key input, add 402 quota cooldown
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/rotator_library/providers/utilities/player2_auth.py`:
- Around line 149-160: In the device-flow initialization logic, normalize HTTP
status, JSON parsing, and required-field lookup failures to the function’s
declared Player2AuthError contract. Within the async client block that posts to
"/login/device/new", catch httpx, JSON, and KeyError-related failures, then
raise Player2AuthError with descriptive context while chaining the original
exception; preserve the existing explicit error for a missing verification URI.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 262b9486-741f-4ff2-a61b-14e6bceda1c7
📒 Files selected for processing (4)
.env.examplesrc/rotator_library/credential_tool.pysrc/rotator_library/providers/player2_provider.pysrc/rotator_library/providers/utilities/player2_auth.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: CodeRabbit / Review
- GitHub Check: Greptile Review
🧰 Additional context used
🪛 Ruff (0.15.20)
src/rotator_library/providers/utilities/player2_auth.py
[warning] 158-160: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 187-189: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 192-194: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 210-210: Logging statement uses f-string
(G004)
src/rotator_library/providers/player2_provider.py
[warning] 206-206: Unused static method argument: error_body
(ARG004)
🔇 Additional comments (4)
src/rotator_library/credential_tool.py (1)
1188-1192: LGTM!Both past review comments have been addressed:
PLAYER2_CLIENT_IDenv var is now checked first (lines 1189), and the manual p2Key prompt usespassword=True(line 1203). Theon_verificationcallback signature matches the upstreamdevice_code_logincontract, error handling with early returns is appropriate for the CLI wizard, and the numbered-key storage logic correctly prevents partial-match collisions via the=suffix check.Also applies to: 1203-1203, 1205-1236, 1238-1265, 1547-1555
.env.example (1)
55-64: LGTM!The past review comment requesting
PLAYER2_CLIENT_IDdocumentation has been addressed (lines 60-63). The section clearly directs users to the assisted login wizard and documents the advanced override with appropriate guidance.src/rotator_library/providers/utilities/player2_auth.py (1)
33-33: LGTM!Also applies to: 131-148, 185-219
src/rotator_library/providers/player2_provider.py (1)
33-33: LGTM!Also applies to: 68-71, 99-99, 111-115, 204-227, 245-260
|
Thanks for the thorough review, all, pushed a commit addressing everything raised: Device flow error handling (Greptile + CodeRabbit): 402 → long cooldown, not a short rate-limit retry (mirrobot-agent): implemented
Manual p2Key prompt not hidden (CodeRabbit): fixed - now uses
Not addressed (needs the maintainer, not code): Also re-verified end-to-end after all these changes (real curl request against the live proxy) - still working correctly. |
There was a problem hiding this comment.
Follow-up Review
All previously raised items have been addressed cleanly. The changes are well-structured, follow existing project conventions, and are thoroughly documented.
Previous Feedback — Resolved:
- 402 → long cooldown:
parse_quota_error()with a customplayer2_error_reasonattribute tag correctly distinguishes insufficient credits from a transient 429, applying a 24h cooldown. Verified end-to-end through the error_handler classification flow. (inline confirmation below) PLAYER2_API_BASEenv var:_api_base()now checks the env var before falling back to the default, consistent with the*_API_BASEconvention.extra_bodyfiltering: The added comment clearly explains why fields are filtered (Player2's strict schema) and that new params must be added toSUPPORTED_PARAMS.- Device flow RFC 8628: Token polling now parses standard error bodies —
authorization_pendingkeeps polling,slow_downincreases the interval, and terminal errors (access_denied,expired_token, etc.) fail fast instead of looping until timeout. The defensiveverification_uricheck and "200 with no key → error" are good additions. password=TrueandPLAYER2_CLIENT_IDresolution order (CodeRabbit items): Both addressed.
Still Open (non-blocking):
- The device-flow init at
player2_auth.py:149-155(client.post+raise_for_status+.json()+ key access) still isn't wrapped to normalizehttpx/JSON/KeyErrorexceptions intoPlayer2AuthError— CodeRabbit flagged this and it remains valid. It's non-blocking since_setup_player2_credentialcatchesExceptionas a fallback, so the user still sees an error panel, just with a less polished message. DEFAULT_CLIENT_IDis still the personal/contributor ID — acknowledged in the PR body as a maintainer task before merge.
Overall Status:
Code is in great shape. Once the Client ID is swapped by the maintainer, this is ready to merge. The CodeRabbit init-normalization item is a nice-to-have polish but shouldn't block.
This review was generated by an AI assistant.
…raw httpx/JSON/KeyError
|
@CarlosNahuelcoy PR seems good. only problem is that i am in the middle of experimental rewrite that adds... a lot. Most likely i will merge this PR and any other PRs into that or rewrite them to work on that instead. Main is a bit behind dev, and dev is a lot behind experimental #162 . |
|
@mirrobot-agent Check this PR against experimental branch(in it's current state) and how big of the changes are needed. Basically, analyze porting this from main branch to experimental. Keep in mind experimental is in a fluid alpha state and might change a lot based on review - but core idea is there. |
|
@Mirrowel, on it. I'll analyze how PR #165 (Player2 provider) ports over to the Starting with the branch structure and the PR diffs now. |
|
@Mirrowel, I've analyzed PR #165 against TL;DR
Why it ports cleanlyThe PR's
File-by-file:
Things to be aware of (non-blocking)
RecommendationThis is the easier of the two (#166 is more involved — posting there next). When you do the This analysis was generated by an AI assistant. |
2 participants
What this adds
Native support for Player2, an AI platform for game NPC integrations, as a first-party provider.
player2_provider.py— custom completion logic (streaming + non-streaming) against Player2's OpenAI-compatible/chat/completionsendpoint. Player2's API doesn't accept amodelfield or expose model selection, so it's stripped from the outgoing payload;402 Insufficient creditsand429are mapped toRateLimitErrorso they're handled the same way quota exhaustion is for every other provider.player2_auth.py— the login flow. Tries, in order: (1) detecting a Player2 desktop app already running locally (instant, no browser), (2) the OAuth 2.0 Device Authorization Flow (a URL + short code, works headless too).credential_tool.py— a small assisted-login wizard wired into the existing "Add API Key" flow, so setting up Player2 is "run the wizard, approve once" instead of manually hunting for a key.README.md, plus a commented example block in.env.example.About the Game Client ID
Both Player2 login flows need a "Game Client ID" registered via the Player2 Developer Dashboard. Right now
player2_auth.DEFAULT_CLIENT_IDis set to an ID from my own personal Player2 account, just so this could be built and tested end-to-end.Before merging, please register your own game in the Player2 Developer Dashboard (https://player2.game) and swap in your own Client ID in
player2_auth.py, so the project owns and controls it going forward rather than depending on my account. It's a one-line change (DEFAULT_CLIENT_ID = "..."), and there's also aPLAYER2_CLIENT_IDenv var override for anyone self-hosting with their own registered game.Testing
player2:1recognized, and both streaming and non-streaming completions return correct responses viacurl.player2/defaultas the model, with good results across multiple dialogue turns.Notes / things worth a second look
/login/device/tokenendpoint only documents200/500responses - the "still pending approval" status isn't spelled out in their public API reference. I implemented the standard RFC 8628 convention (treat other 4xx as "still pending"), documented inplayer2_auth.py, but it'd be good to have this double-checked against Player2's actual behavior if anyone has more visibility into it.