refactor: extract gh client generation into gh package - #997
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the workspace-engine GitHub job agent to centralize GitHub App client creation logic into a dedicated pkg/github package (fixing #996), and updates the workflow dispatcher to use the new client factory.
Changes:
- Replace per-agent GitHub client generation in the workflow dispatcher with
pkg/github.CreateClientForInstallation. - Remove the legacy
pkg/githubclientimplementation. - Add
pkg/github/client.goproviding GitHub App JWT creation plus installation/repo-scoped client helpers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/workspace-engine/pkg/jobagents/github/workflow_dispatcher.go | Switches workflow dispatch to use the new centralized GitHub client factory. |
| apps/workspace-engine/pkg/githubclient/githubclient.go | Removes the old GitHub client/JWT/token exchange implementation. |
| apps/workspace-engine/pkg/github/client.go | Introduces new shared GitHub App client generation and installation/repo client helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func appClient() (*github.Client, error) { | ||
| jwtStr, err := generateJWT() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return github.NewClient(nil).WithAuthToken(jwtStr), nil | ||
| } |
There was a problem hiding this comment.
github.NewClient(nil) uses the default HTTP client (no timeout). Since callers often pass a context without a deadline (e.g. async dispatch), GitHub API calls can hang indefinitely. Consider constructing a shared *http.Client with a sane Timeout and passing it into github.NewClient, and/or wrapping calls in context.WithTimeout inside this package.
| token, _, err := app.Apps.CreateInstallationToken(ctx, installationID, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("create installation token: %w", err) | ||
| } | ||
|
|
||
| return github.NewClient(nil).WithAuthToken(token.GetToken()), nil | ||
| } |
There was a problem hiding this comment.
The installation-authenticated client is created with github.NewClient(nil) as well, inheriting http.DefaultClient without timeouts. To avoid indefinitely hung dispatches/token refreshes, pass a configured *http.Client (Timeout/Transport) into github.NewClient here too (and ideally reuse the same client as appClient).
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA refactoring that consolidates GitHub App authentication logic from scattered locations ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/github/client.go (1)
76-83: Consider checking config upfront for clarity.The current pattern calls
appClient(), catches the error, then re-checks the config to determine if it was a "not configured" scenario. This duplicates the config validation logic. Checking upfront would be more explicit about the early return path.♻️ Suggested refactor
func CreateClientForRepo(ctx context.Context, owner, repo string) (*github.Client, error) { + if config.Global.GithubBotAppID == "" || config.Global.GithubBotPrivateKey == "" { + return nil, nil + } + app, err := appClient() if err != nil { - if config.Global.GithubBotAppID == "" || config.Global.GithubBotPrivateKey == "" { - return nil, nil - } return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/github/client.go` around lines 76 - 83, The code in CreateClientForRepo calls appClient() and then inspects config.Global.GithubBotAppID and config.Global.GithubBotPrivateKey only on error, duplicating config validation; instead, check the config values up-front in CreateClientForRepo (inspect config.Global.GithubBotAppID and config.Global.GithubBotPrivateKey) and return nil,nil immediately if not configured, then call appClient() and return its error normally—this removes the duplicate config check and makes the early-return path explicit while keeping appClient() usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/workspace-engine/pkg/github/client.go`:
- Around line 76-83: The code in CreateClientForRepo calls appClient() and then
inspects config.Global.GithubBotAppID and config.Global.GithubBotPrivateKey only
on error, duplicating config validation; instead, check the config values
up-front in CreateClientForRepo (inspect config.Global.GithubBotAppID and
config.Global.GithubBotPrivateKey) and return nil,nil immediately if not
configured, then call appClient() and return its error normally—this removes the
duplicate config check and makes the early-return path explicit while keeping
appClient() usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03e00e2a-dbe9-4092-9a31-66ca1cdafe3f
📒 Files selected for processing (3)
apps/workspace-engine/pkg/github/client.goapps/workspace-engine/pkg/githubclient/githubclient.goapps/workspace-engine/pkg/jobagents/github/workflow_dispatcher.go
💤 Files with no reviewable changes (1)
- apps/workspace-engine/pkg/githubclient/githubclient.go
fixes #996
Summary by CodeRabbit