P1: GitHub API hardening (token support, retries, rate-limit messaging)#37
Merged
Kinin-Code-Offical merged 1 commit intomainfrom Dec 22, 2025
Merged
P1: GitHub API hardening (token support, retries, rate-limit messaging)#37Kinin-Code-Offical merged 1 commit intomainfrom
Kinin-Code-Offical merged 1 commit intomainfrom
Conversation
Reviewer's GuideAdds a reusable GitHub API helper that injects optional token-based auth, centralizes retry/backoff logic with rate-limit-aware logging, and refactors existing release and checksum fetchers to use it. Class-style diagram for selfUpdate GitHub helper and API functionsclassDiagram
class SelfUpdateModule {
<<module>>
+getLatestRelease() Promise~ReleaseInfo~
+getReleaseByTag(tag string) Promise~ReleaseInfo~
+getLatestPrerelease() Promise~ReleaseInfo~
+fetchSha256Sums(release ReleaseInfo) Promise~Map~string,string~
}
class GitHubHelpers {
<<utility>>
+getGithubHeaders() Record~string,string~
+getRateLimitMessage(error any) string
+shouldRetry(error any) boolean
+githubGet~T~(url string) Promise~AxiosResponse~T~~
}
class ReleaseInfo {
+tag_name string
+name string
+body string
+prerelease boolean
+draft boolean
+assets any[]
}
class UpdateStatus {
+updateAvailable boolean
+currentVersion string
+latestVersion string
+releaseInfo ReleaseInfo
}
SelfUpdateModule ..> GitHubHelpers : uses
SelfUpdateModule ..> ReleaseInfo : returns
SelfUpdateModule ..> UpdateStatus : returns
GitHubHelpers ..> ReleaseInfo : typed responses
Flow diagram for githubGet retry and backoff logicflowchart TD
A["start githubGet(url)"] --> B["set attempt = 0"]
B --> C["attempt <= MAX_RETRIES?"]
C -->|no| Z["throw Error GitHub API request failed after retries"]
C -->|yes| D["try axios.get(url, timeout, headers from getGithubHeaders)"]
D --> E{"request succeeded?"}
E -->|yes| F["return response"]
E -->|no| G["attempt++"]
G --> H["getRateLimitMessage(error)"]
H --> I{"rate limit message?"}
I -->|yes| J["logger.warn(rate limit reset time)"]
I -->|no| K["skip logging"]
J --> L["shouldRetry(error)?"]
K --> L["shouldRetry(error)?"]
L -->|no or attempt > MAX_RETRIES| M["throw error"]
L -->|yes and attempt <= MAX_RETRIES| N["delay 1000ms * attempt"]
N --> C
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- For consistency and to benefit from the retry logic, consider routing the checksum fetch in
fetchSha256SumsthroughgithubGetinstead of callingaxios.getdirectly. - In
getRateLimitMessage, it may be safer to validate thatx-ratelimit-resetis a parseable numeric timestamp before usingNumber(reset)to avoid logging an invalid date if the header is malformed or missing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For consistency and to benefit from the retry logic, consider routing the checksum fetch in `fetchSha256Sums` through `githubGet` instead of calling `axios.get` directly.
- In `getRateLimitMessage`, it may be safer to validate that `x-ratelimit-reset` is a parseable numeric timestamp before using `Number(reset)` to avoid logging an invalid date if the header is malformed or missing.
## Individual Comments
### Comment 1
<location> `src/core/selfUpdate.ts:46-51` </location>
<code_context>
+ if (axios.isAxiosError(error) && error.response) {
+ const remaining = error.response.headers['x-ratelimit-remaining'];
+ const reset = error.response.headers['x-ratelimit-reset'];
+ if (remaining === '0' && reset) {
+ const resetTime = new Date(Number(reset) * 1000).toISOString();
+ return `GitHub API rate limit exceeded. Resets at ${resetTime}.`;
</code_context>
<issue_to_address>
**suggestion:** Handle non-numeric or unexpected `x-ratelimit-reset` values more defensively.
If `x-ratelimit-reset` is present but not a valid epoch, `Number(reset)` becomes `NaN` and `new Date(NaN).toISOString()` yields `Invalid Date`, producing a misleading message. You could guard with:
```ts
const resetEpoch = Number(reset);
if (!Number.isFinite(resetEpoch)) return null;
const resetTime = new Date(resetEpoch * 1000).toISOString();
```
This avoids logging invalid timestamps.
```suggestion
const remaining = error.response.headers['x-ratelimit-remaining'];
const reset = error.response.headers['x-ratelimit-reset'];
if (remaining === '0' && reset) {
const resetEpoch = Number(reset);
if (!Number.isFinite(resetEpoch)) return null;
const resetTime = new Date(resetEpoch * 1000).toISOString();
return `GitHub API rate limit exceeded. Resets at ${resetTime}.`;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+46
to
+51
| const remaining = error.response.headers['x-ratelimit-remaining']; | ||
| const reset = error.response.headers['x-ratelimit-reset']; | ||
| if (remaining === '0' && reset) { | ||
| const resetTime = new Date(Number(reset) * 1000).toISOString(); | ||
| return `GitHub API rate limit exceeded. Resets at ${resetTime}.`; | ||
| } |
There was a problem hiding this comment.
suggestion: Handle non-numeric or unexpected x-ratelimit-reset values more defensively.
If x-ratelimit-reset is present but not a valid epoch, Number(reset) becomes NaN and new Date(NaN).toISOString() yields Invalid Date, producing a misleading message. You could guard with:
const resetEpoch = Number(reset);
if (!Number.isFinite(resetEpoch)) return null;
const resetTime = new Date(resetEpoch * 1000).toISOString();This avoids logging invalid timestamps.
Suggested change
| const remaining = error.response.headers['x-ratelimit-remaining']; | |
| const reset = error.response.headers['x-ratelimit-reset']; | |
| if (remaining === '0' && reset) { | |
| const resetTime = new Date(Number(reset) * 1000).toISOString(); | |
| return `GitHub API rate limit exceeded. Resets at ${resetTime}.`; | |
| } | |
| const remaining = error.response.headers['x-ratelimit-remaining']; | |
| const reset = error.response.headers['x-ratelimit-reset']; | |
| if (remaining === '0' && reset) { | |
| const resetEpoch = Number(reset); | |
| if (!Number.isFinite(resetEpoch)) return null; | |
| const resetTime = new Date(resetEpoch * 1000).toISOString(); | |
| return `GitHub API rate limit exceeded. Resets at ${resetTime}.`; | |
| } |
There was a problem hiding this comment.
Pull request overview
This PR enhances the GitHub API integration by adding authentication token support, implementing retry logic for transient failures, and providing better rate-limit messaging. The changes centralize GitHub API requests through a new githubGet helper function.
Key Changes:
- Introduced
githubGethelper with retry logic (up to 2 retries) for handling transient HTTP errors - Added support for GitHub authentication tokens via
CLOUDSQLCTL_GITHUB_TOKENorGITHUB_TOKENenvironment variables - Implemented rate-limit detection that surfaces the reset timestamp when quota is exhausted
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+74
to
+78
| const rateLimit = getRateLimitMessage(error); | ||
| if (rateLimit) { | ||
| logger.warn(rateLimit); | ||
| } | ||
| if (attempt > MAX_RETRIES || !shouldRetry(error)) { |
There was a problem hiding this comment.
The rate limit warning is logged on every retry attempt when rate limited, which could result in duplicate warnings. Consider only logging the rate limit message once, or restructuring the logic so the warning is shown before the final throw rather than on each retry attempt.
Suggested change
| const rateLimit = getRateLimitMessage(error); | |
| if (rateLimit) { | |
| logger.warn(rateLimit); | |
| } | |
| if (attempt > MAX_RETRIES || !shouldRetry(error)) { | |
| if (attempt > MAX_RETRIES || !shouldRetry(error)) { | |
| const rateLimit = getRateLimitMessage(error); | |
| if (rateLimit) { | |
| logger.warn(rateLimit); | |
| } |
| function shouldRetry(error: unknown): boolean { | ||
| if (axios.isAxiosError(error)) { | ||
| if (!error.response) return true; | ||
| return RETRYABLE_STATUS.has(error.response.status); |
There was a problem hiding this comment.
When GitHub API returns a 429 (rate limit exceeded), the code will retry with delays of 1 second and 2 seconds. However, rate limit resets typically occur at fixed times (shown in the reset header), not after a few seconds. Retrying immediately with short delays is unlikely to succeed and will waste time. Consider checking the reset time and either failing immediately with the reset message, or implementing a longer delay based on the actual reset time.
Comment on lines
+64
to
+86
| async function githubGet<T>(url: string) { | ||
| let attempt = 0; | ||
| while (attempt <= MAX_RETRIES) { | ||
| try { | ||
| return await axios.get<T>(url, { | ||
| timeout: TIMEOUT_MS, | ||
| headers: getGithubHeaders() | ||
| }); | ||
| } catch (error) { | ||
| attempt++; | ||
| const rateLimit = getRateLimitMessage(error); | ||
| if (rateLimit) { | ||
| logger.warn(rateLimit); | ||
| } | ||
| if (attempt > MAX_RETRIES || !shouldRetry(error)) { | ||
| throw error; | ||
| } | ||
| const delayMs = 1000 * attempt; | ||
| await new Promise(resolve => setTimeout(resolve, delayMs)); | ||
| } | ||
| } | ||
| throw new Error('GitHub API request failed after retries'); | ||
| } |
There was a problem hiding this comment.
The new githubGet helper function introduces significant new behavior including retry logic, token authentication, and rate limit handling, but no tests have been added to verify this functionality. Consider adding tests to cover scenarios such as: successful retry after transient failure, rate limit detection and messaging, token header inclusion when environment variable is set, and failure after max retries.
| await new Promise(resolve => setTimeout(resolve, delayMs)); | ||
| } | ||
| } | ||
| throw new Error('GitHub API request failed after retries'); |
There was a problem hiding this comment.
This line is unreachable because the while loop condition ensures that when attempt > MAX_RETRIES, the error is thrown at line 79 within the catch block. The function will never reach this point. Consider removing this line or restructuring the retry logic.
Suggested change
| throw new Error('GitHub API request failed after retries'); |
Comment on lines
169
to
173
| const response = await axios.get(checksumAsset.browser_download_url, { | ||
| responseType: 'text', | ||
| timeout: TIMEOUT_MS | ||
| timeout: TIMEOUT_MS, | ||
| headers: getGithubHeaders() | ||
| }); |
There was a problem hiding this comment.
The fetchSha256Sums function uses axios.get directly instead of using the new githubGet helper. This means it lacks retry logic and won't benefit from the token authentication when downloading checksums. Consider using githubGet here as well, or if the responseType: 'text' option is needed, consider extending githubGet to accept additional axios options.
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.
Closes #20
Summary:
Tests:
Rollback:
Summary by Sourcery
Harden GitHub API interactions used for self-update by introducing a shared request helper with retries, token-based authentication, and improved rate-limit handling.
Enhancements: