Added GH Auth for use with GH and Copilot models. Uses GH cli first, …#251
Open
judggernaut wants to merge 3 commits intompfaffenberger:mainfrom
Open
Added GH Auth for use with GH and Copilot models. Uses GH cli first, …#251judggernaut wants to merge 3 commits intompfaffenberger:mainfrom
judggernaut wants to merge 3 commits intompfaffenberger:mainfrom
Conversation
…then PAT, then app id as last resort.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a GitHub Models OAuth plugin: new config constants and helpers, RFC 8628 device-flow implementation, token discovery and secure persistence, GitHub/Copilot model catalog fetching and local config management, callback wiring for auth/status/logout and model registration, tests, and integration of GitHub model overlays into model loading. Changes
Sequence DiagramssequenceDiagram
participant User
participant Plugin as GitHub OAuth Plugin
participant GH as GitHub Device<br/>Endpoints
participant Browser
participant API as GitHub API
User->>Plugin: /github-auth
Plugin->>Plugin: Check gh CLI token
alt gh token found
Plugin->>API: Validate token
API-->>Plugin: Username
Plugin->>Plugin: Save token & import models
else No gh token
Plugin->>Plugin: Check GITHUB_TOKEN env
alt env token found
Plugin->>API: Validate token
API-->>Plugin: Username
Plugin->>Plugin: Save token & import models
else No env token
Plugin->>User: Prompt for PAT
User->>Plugin: Paste token
Plugin->>API: Validate token
API-->>Plugin: Username
Plugin->>Plugin: Save token & import models
else Invalid/empty PAT
Plugin->>GH: Start device flow
GH-->>Plugin: Device code + URI
Plugin->>User: Display code & URL
Plugin->>Browser: Open verification URI
Browser->>GH: User authorizes
Plugin->>GH: Poll for access token
GH-->>Plugin: Access token
Plugin->>API: Validate token
API-->>Plugin: Username
Plugin->>Plugin: Save token & import models
end
end
Plugin-->>User: Auth success/failure
sequenceDiagram
participant Poll as Polling Loop
participant GH as GitHub API
participant Plugin as Device Flow Handler
loop until timeout
Poll->>GH: POST access_token (device_code)
alt access_token returned
GH-->>Poll: access_token
Poll->>Plugin: Return token
else authorization_pending
GH-->>Poll: error authorization_pending
Poll->>Poll: Continue polling
else slow_down
GH-->>Poll: error slow_down
Poll->>Poll: Increase interval, retry
else expired_token
GH-->>Poll: error expired_token
Poll->>Plugin: Return None
else other error
GH-->>Poll: error
Poll->>Plugin: Return None (abort)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code_puppy/plugins/github_models_oauth/device_flow.py`:
- Around line 46-53: The POSTs in device_flow.py are sending JSON but GitHub
requires form-encoded bodies; update both requests.post calls (the one that
initiates the device flow and the one that polls/exchanges the device code) to
pass form data via data= instead of json= and remove the explicit "Content-Type:
application/json" header so requests will set application/x-www-form-urlencoded
automatically; locate the two calls by the requests.post invocations used in the
device flow initiation and token polling/exchange functions and change their
payload handling and headers accordingly.
In `@code_puppy/plugins/github_models_oauth/register_callbacks.py`:
- Around line 104-114: The code currently proceeds to call
set_model_and_reload_agent even when discovery produced zero models because
_handle_auth() returns True; change the logic so that after discovery/config
writes you only call set_model_and_reload_agent when total > 0 (i.e., inside the
same branch that emits emit_success), and pick a model from the actual
discovered list (or verify existence) rather than hard-coding
"github-openai-gpt-4.1"; apply the same guard to the other occurrence referenced
(the block around set_model_and_reload_agent at the second location).
- Around line 145-154: Wrap token_path.unlink() in a try/except that catches
OSError and logs the error via emit_info/emit_error instead of allowing it to
raise, track a boolean indicating whether token and model removals succeeded,
call remove_github_models() as before but after it ensure the running
session/cache is invalidated or switched away from any plugin-managed model
(e.g., if the active model id startswith "github-" or "copilot-", clear or set a
safe default), and only call emit_success("👋 GitHub Models logout complete")
when all cleanup steps succeeded; if any step failed emit an error/info message
and do not emit success.
In `@code_puppy/plugins/github_models_oauth/test_plugin.py`:
- Around line 561-596: The test calls _handle_auth which always invokes
fetch_copilot_models()/add_copilot_models_to_config in addition to
fetch_github_models/add_models_to_config, so the test must stub the Copilot
discovery path to avoid live network/filesystem side effects; update the test
patches to also patch
"code_puppy.plugins.github_models_oauth.register_callbacks.fetch_copilot_models"
(returning an empty list or expected stubbed models) and
"code_puppy.plugins.github_models_oauth.register_callbacks.add_copilot_models_to_config"
(return_value=True) in the same with(...) block so _handle_auth only exercises
the GitHub-models branch and does not perform real requests or writes.
In `@code_puppy/plugins/github_models_oauth/utils.py`:
- Around line 175-188: The current fetch logic treats HTTP 401/403 as a generic
catalog outage and falls back to DEFAULT_GITHUB_MODELS, which masks auth
failures; change the behavior in the function that calls response.status_code
(the block using _parse_model_list, emit_info, emit_warning, logger and
DEFAULT_GITHUB_MODELS) to detect 401 and 403 explicitly and raise or return a
clear auth error (or re-raise the response/Exception) instead of emitting the
generic warning and returning built-ins; apply the same explicit 401/403
handling to the other identical block referenced (lines around where
DEFAULT_GITHUB_MODELS is returned again) so _handle_auth() can surface
permission problems rather than registering inaccessible models.
- Around line 55-59: The message emitted by emit_info currently recommends
creating a classic PAT with 'read:user' scope which is misleading for the GitHub
Models API; update the emitted text in the emit_info call to recommend creating
a fine-grained Personal Access Token with the "models:read" permission (and note
that classic PATs may still work) and remove the emphasis on 'read:user'; also
add a brief note that this token is required for endpoints like /catalog/models
and /inference/chat/completions to make the guidance accurate and secure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1cda3ec-e6fd-4ac7-bcd4-9597a84e06ea
📒 Files selected for processing (8)
code_puppy/config.pycode_puppy/model_factory.pycode_puppy/plugins/github_models_oauth/__init__.pycode_puppy/plugins/github_models_oauth/config.pycode_puppy/plugins/github_models_oauth/device_flow.pycode_puppy/plugins/github_models_oauth/register_callbacks.pycode_puppy/plugins/github_models_oauth/test_plugin.pycode_puppy/plugins/github_models_oauth/utils.py
Comment on lines
+145
to
+154
| token_path = get_token_storage_path() | ||
| if token_path.exists(): | ||
| token_path.unlink() | ||
| emit_info("✓ Removed GitHub OAuth tokens") | ||
|
|
||
| removed = remove_github_models() | ||
| if removed: | ||
| emit_info(f"✓ Removed {removed} GitHub models from configuration") | ||
|
|
||
| emit_success("👋 GitHub Models logout complete") |
There was a problem hiding this comment.
Harden /github-logout against cleanup failures and stale model state.
token_path.unlink() can still raise out of this callback, and after remove_github_models() the session/cache can keep pointing at deleted github-* / copilot-* entries. This path should catch OSError, avoid emitting success on partial cleanup, and invalidate/switch away from plugin-managed models before returning.
As per coding guidelines, "Fail gracefully in plugin code — never crash the app, especially in callback handlers".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code_puppy/plugins/github_models_oauth/register_callbacks.py` around lines
145 - 154, Wrap token_path.unlink() in a try/except that catches OSError and
logs the error via emit_info/emit_error instead of allowing it to raise, track a
boolean indicating whether token and model removals succeeded, call
remove_github_models() as before but after it ensure the running session/cache
is invalidated or switched away from any plugin-managed model (e.g., if the
active model id startswith "github-" or "copilot-", clear or set a safe
default), and only call emit_success("👋 GitHub Models logout complete") when
all cleanup steps succeeded; if any step failed emit an error/info message and
do not emit success.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
code_puppy/plugins/github_models_oauth/register_callbacks.py (1)
146-159:⚠️ Potential issue | 🟠 Major
/github-logoutreports success even when cleanup fails.Line 158 emits success unconditionally, so token unlink errors (Lines 150-153) still end with a “complete” message.
🔧 Proposed fix
def _handle_logout() -> None: token_path = get_token_storage_path() + cleanup_ok = True try: if token_path.exists(): token_path.unlink() emit_info("✓ Removed GitHub OAuth tokens") except OSError as exc: logger.error("Failed to remove token file: %s", exc) emit_error(f"Failed to remove token file: {exc}") + cleanup_ok = False removed = remove_github_models() if removed: emit_info(f"✓ Removed {removed} GitHub models from configuration") + elif token_path.exists(): + cleanup_ok = False - emit_success("👋 GitHub Models logout complete") + if cleanup_ok: + emit_success("👋 GitHub Models logout complete") + else: + emit_error("GitHub logout completed with errors. Check logs and retry.")As per coding guidelines, "Fail gracefully in plugin code — never crash the app, especially in callback handlers".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code_puppy/plugins/github_models_oauth/register_callbacks.py` around lines 146 - 159, The logout flow currently always calls emit_success("👋 GitHub Models logout complete") even if token removal failed; change the logic in register_callbacks.py around token_path handling and remove_github_models so failures prevent the final success message: detect errors when unlinking token_path (token_path.exists() / token_path.unlink()) by setting a failure flag or returning early after logger.error/emit_error, and only call emit_success if no failure occurred (and still run remove_github_models but treat its failure similarly); reference token_path, remove_github_models, emit_error, logger, and emit_success to locate and update the code accordingly.code_puppy/plugins/github_models_oauth/utils.py (1)
188-191:⚠️ Potential issue | 🟠 MajorFallback guidance still points to
read:user, which is misleading for Models access.Line 190 suggests
gh auth login -s read:user, which can send users down the wrong path when troubleshooting model catalog/inference access.🔧 Proposed fix
- emit_warning( - f" Catalog returned HTTP {response.status_code}; using {len(DEFAULT_GITHUB_MODELS)} built-in models.\n" - " 💡 For the full list, use a PAT or re-run: gh auth login -s read:user" - ) + emit_warning( + f" Catalog returned HTTP {response.status_code}; using {len(DEFAULT_GITHUB_MODELS)} built-in models.\n" + " 💡 For full catalog access, use a token with GitHub Models access " + "(fine-grained PATs need models:read)." + )GitHub REST API models inference and catalog token requirements for fine-grained PATs; confirm whether models:read is required and whether read:user is sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code_puppy/plugins/github_models_oauth/utils.py` around lines 188 - 191, The fallback warning currently suggests using "gh auth login -s read:user", which is misleading for model/catalog access; update the message in the emit_warning call (the block that references response.status_code and DEFAULT_GITHUB_MODELS) to recommend the correct PAT scopes for Models access (e.g., "models:read" or the exact fine-grained PAT scope required) and/or instruct users to create a PAT with model/catalog access rather than read:user; verify the precise GitHub scope required and replace the "gh auth login -s read:user" substring with the accurate scope suggestion and a short action (e.g., "gh auth login -s models:read" or "create a PAT with the models:read scope").
🧹 Nitpick comments (1)
code_puppy/plugins/github_models_oauth/test_plugin.py (1)
476-485: Add a regression test for logout partial-failure behavior.Current command-routing tests assert return values, but there’s no assertion that
/github-logoutsuppresses success messaging when unlink fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code_puppy/plugins/github_models_oauth/test_plugin.py` around lines 476 - 485, Update the test for /github-logout to cover the partial-failure case: patch register_callbacks._handle_logout to simulate an unlink failure (raise an exception or return False) and assert that _handle_custom_command("/github-logout", "github-logout") still returns True but does not emit the success message by verifying the success-message sender is not called (mock the module's message-sending function, e.g., send_message/send_response used by register_callbacks). Ensure you reference and patch _handle_logout and _handle_custom_command in the test and assert the message-sender mock has zero calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code_puppy/plugins/github_models_oauth/device_flow.py`:
- Around line 84-90: The polling loop currently increments elapsed only by sleep
(using variables delay, elapsed, interval and while elapsed < timeout) which
ignores the time spent performing the HTTP request (the request with its own
timeout), so change the loop to use time.monotonic() to measure real elapsed
time: record a loop_start = time.monotonic() before entering the while, compute
elapsed = time.monotonic() - loop_start each iteration (instead of adding
delay), and when waiting between polls compute sleep time from the configured
interval minus the time already spent in that iteration (or cap sleep by
remaining timeout) so total elapsed accounts for both request duration and sleep
and the loop respects timeout correctly.
- Around line 166-169: Wrap the blocking call to
poll_for_access_token(device.device_code, device.interval) in a try/except that
catches KeyboardInterrupt so the Ctrl+C path is handled gracefully; after
emit_info("⏳ Waiting for authorization (press Ctrl+C to cancel)…") surrounding
that call, catch KeyboardInterrupt and perform a clean abort flow (e.g., emit an
informational cancellation message and return/exit the function without
raising), ensuring any cleanup or state rollback is executed before returning.
---
Duplicate comments:
In `@code_puppy/plugins/github_models_oauth/register_callbacks.py`:
- Around line 146-159: The logout flow currently always calls emit_success("👋
GitHub Models logout complete") even if token removal failed; change the logic
in register_callbacks.py around token_path handling and remove_github_models so
failures prevent the final success message: detect errors when unlinking
token_path (token_path.exists() / token_path.unlink()) by setting a failure flag
or returning early after logger.error/emit_error, and only call emit_success if
no failure occurred (and still run remove_github_models but treat its failure
similarly); reference token_path, remove_github_models, emit_error, logger, and
emit_success to locate and update the code accordingly.
In `@code_puppy/plugins/github_models_oauth/utils.py`:
- Around line 188-191: The fallback warning currently suggests using "gh auth
login -s read:user", which is misleading for model/catalog access; update the
message in the emit_warning call (the block that references response.status_code
and DEFAULT_GITHUB_MODELS) to recommend the correct PAT scopes for Models access
(e.g., "models:read" or the exact fine-grained PAT scope required) and/or
instruct users to create a PAT with model/catalog access rather than read:user;
verify the precise GitHub scope required and replace the "gh auth login -s
read:user" substring with the accurate scope suggestion and a short action
(e.g., "gh auth login -s models:read" or "create a PAT with the models:read
scope").
---
Nitpick comments:
In `@code_puppy/plugins/github_models_oauth/test_plugin.py`:
- Around line 476-485: Update the test for /github-logout to cover the
partial-failure case: patch register_callbacks._handle_logout to simulate an
unlink failure (raise an exception or return False) and assert that
_handle_custom_command("/github-logout", "github-logout") still returns True but
does not emit the success message by verifying the success-message sender is not
called (mock the module's message-sending function, e.g.,
send_message/send_response used by register_callbacks). Ensure you reference and
patch _handle_logout and _handle_custom_command in the test and assert the
message-sender mock has zero calls.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1743bda8-09c5-4e29-a947-58d3f37d50d4
📒 Files selected for processing (4)
code_puppy/plugins/github_models_oauth/device_flow.pycode_puppy/plugins/github_models_oauth/register_callbacks.pycode_puppy/plugins/github_models_oauth/test_plugin.pycode_puppy/plugins/github_models_oauth/utils.py
mpfaffenberger
force-pushed
the
main
branch
2 times, most recently
from
c1f396b to
daa401a
Compare
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.
Vibes are strong here. Adds ability for users to use GitHub and Copilot models as option if they have access to them via
github-authcommand. Github and Copilot models are not the same but we can share creds as needed to access the respective models.Attempts to login with:
Summary by CodeRabbit
New Features
Tests