[AI] Issue #111: [FEATURE] Bounded exponential retry with jitter for external calls

[koydas]

AI Generated Change

Added retryWithBackoff utility and integrated it into anthropic_client, groq_client, and ghFetch

Closes #111

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 27a38304d5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Escape slashes in diff regex literal

The new pattern rawDiff.matchAll(/^diff --git a/(.*?) b//gm) is not a valid JavaScript regex literal because the / characters in a/ and b/ are unescaped, so Node throws a SyntaxError while parsing the module. This prevents auto_fix_pr.mjs from starting at all, which blocks every auto-fix run.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Remove unsupported node:abort-controller import

retry.mjs imports AbortController from node:abort-controller, but this built-in module does not exist in the repository’s Node 20 runtime, causing ERR_UNKNOWN_BUILTIN_MODULE at import time. Because this helper is now used by API clients, the failure cascades and breaks multiple scripts before they execute any logic.

Useful? React with 👍 / 👎.

[koydas]

🔍 Automated Code Review

Okay, let's take a look at this pull request. The issue was about adding bounded exponential retry with jitter for external calls. The diff shows changes in three files and a new retry.mjs file.

First, in auto_fix_pr.mjs, they added the retryWithBackoff import and wrapped the fetch call. That seems correct for adding retries around the GitHub API calls.

Looking at anthropic_client.mjs, they added retryWithBackoff around the fetch to Anthropic. They also added a RETRYABLE_STATUS_CODES set. That makes sense to retry on specific HTTP errors. The error handling here sets retryable based on status codes, which aligns with the issue's goal.

In groq_client.mjs, the changes are more involved. They renamed maxRetries to maxAttempts, which should match the retry function's parameter. The original loop for retries was replaced with retryWithBackoff. However, in the Groq client, the maxAttempts is set as parsed + 1, while the default in retry.mjs is 4. Wait, in the Groq code, the default maxAttempts would be 4, but the original maxRetries was 3. So with the new code, maxAttempts is 3 + 1 = 4, which matches the default. That seems okay. But in the retry function, the maxAttempts is the total number of attempts. The original loop ran for attempt <= maxRetries, which is 0 to 3 (4 attempts). So the new code using maxAttempts as parsed + 1 would be correct. However, when the environment variable is set, say to 3, the maxAttempts becomes 4, which is the same as before. So that part looks okay.

Wait, in the Groq client's retryWithBackoff call, they pass { maxAttempts }, but the retry function's options include maxAttempts as the total attempts. The original code had maxRetries = 3, leading to 4 attempts (0 to 3). The new code uses maxAttempts = parsed + 1, which for default 3 becomes 4. So that's correct. So that's okay.

Another thing to check is the error handling. The Groq client sets error.retryable based on RETRYABLE_STATUS_CODES, which is correct. Also, the parseWaitMs is assigned to error.waitMs, which the retry function uses. That should work.

Now, looking at the retry.mjs implementation. The function uses maxAttempts, which is the total number of attempts. The loop runs while attempt < maxAttempts, which is correct. The calculation for waitMs uses error.waitMs if present, otherwise computes it with backoff and jitter. That's correct.

Potential issues: In the Groq client, the original code had a variable called maxRetries, which was 3 by default, leading to 4 attempts. The new code uses maxAttempts = parsed + 1. So if the environment variable is set to 3, maxAttempts becomes 4, which is the same as before. So the behavior should be consistent. But if the environment variable is set to 0, maxAttempts becomes 1, which is correct.

Another thing: in the Groq client, after retryWithBackoff, they parse the JSON. The original code did that inside the loop, but now it's outside. Since retryWithBackoff returns the rawText, parsing it after is correct.

Is there any place where the retry logic might retry when it shouldn't? For example, in the Anthropic client, the error.retryable is set based on the status codes. Similarly in Groq. The retry function checks if error.retryable is false, which would prevent retries. That's correct.

Wait, in the Groq client's retryWithBackoff call, they pass { maxAttempts }, and the default in retry.mjs is maxAttempts: 4. So if the user doesn't set GROQ_MAX_RETRIES, it defaults to 3 + 1 = 4. Which matches the default in retry.mjs. So that's okay.

Another thing: in the Groq client's retryWithBackoff, they pass the function that fetches and returns text. Then, after retries, they parse the JSON. That's correct because the retry is on the fetch level, and parsing happens once successfully fetched.

Is there any missing error handling? For example, in the Groq client, after retries, if it's successful, the content is parsed. If JSON parsing fails, it throws an error, which is correct.

Potential issue: in the Groq client's retryWithBackoff, the function passed to retryWithBackoff includes a try-catch around the fetch, setting fetchErr.retryable = false. That's correct because network errors (like fetch failures) should not be retried unless they are retryable. But since the error is not marked as retryable, the retry logic will not retry on those. Wait, but in the retry function, if the error has retryable: false, it throws immediately. So if a fetch error occurs (like network issues), unless the error is marked retryable, it won't retry. But in the Groq client's code, any fetch error is marked as non-retryable. That might be too strict. For example, transient network errors could be retryable. However, the issue says "bounded exponential retry with jitter for external calls", which typically includes retrying on certain HTTP status codes and maybe some network errors. But in the current code, network errors (like fetch errors) are not retried because they are marked as non-retryable. Is that correct?

The original Groq client code had a loop that would retry on non-TRANSIENT errors? Wait, in the original Groq code, the loop would retry if errorType is TRANSIENT (since it's not TRANSIENT, it would throw). Wait, the original code in Groq had:

if (errorType !== 'TRANSIENT' || attempt === maxRetries) {
throw lastError;
}

So, if errorType is 'TRANSIENT', it would retry. So the retryable errors in the original code were those with errorType 'TRANSIENT'. The new code uses RETRYABLE_STATUS_CODES which includes 429, 500, etc. But the classifyError function would determine the errorType. So if the error is retryable based on status codes, then it's retried. However, in the new code, network errors (like fetch errors) are marked as non-retryable. So if there's a network error (e.g., DNS failure, connection reset), the retry logic won't retry, which might be intentional. But maybe those should be considered retryable. However, the original code didn't handle those cases either. The original code's loop only handled HTTP errors with specific status codes. So the new code is consistent with the original behavior in that aspect. Therefore, it's correct.

Another thing to check: in the Groq client's retryWithBackoff call, the options passed are { maxAttempts }, which is correct. The default maxAttempts is 4, so if not set, it uses 4 attempts. Which matches the original maxRetries of 3 (4 attempts).

Another point: in the Groq client, the original code had a variable called maxRetries, which was 3 by default. The new code uses maxAttempts = parsed + 1. So if the environment variable is set to 0, maxAttempts is 1, which is correct. So the behavior is preserved.

In the retry.mjs file, the DEFAULT_OPTIONS has maxAttempts: 4. So if no options are passed, it uses 4 attempts. Which is the same as the original default in Groq (maxRetries 3 leading to 4 attempts). So that's correct.

Another thing: in the Groq client's retryWithBackoff, the error object has a waitMs property, which is used in the retry delay. The original code had parseWaitMs and used it in the wait time. The new code sets error.waitMs = parseWaitMs(...) and then uses it in retryWithBackoff. That's correct.

In the retry function, the waitMs is calculated as error.waitMs if present, otherwise the exponential backoff. So that's correct.

Is there any case where the retry logic would retry when it shouldn't? For example, if an error is not marked as retryable, the retry function throws immediately. So in the Anthropic client, errors not in RETRYABLE_STATUS_CODES are not retried. That's correct.

In the GitHub fetch wrapper, they wrap the fetch in retryWithBackoff without any additional error handling. So any error from fetch (network errors) would be retried? Wait, in the GitHub ghFetch function, they wrap the fetch in retryWithBackoff, but in the retry function, the default maxAttempts is 4. But in the GitHub code, the retryWithBackoff call doesn't specify any options, so it uses the default maxAttempts of 4. However, the GitHub API might have different retry policies. But according to the issue, the change is to add retry for external calls, including GitHub. The current code for ghFetch just wraps the fetch in retryWithBackoff, but doesn't set any custom options. So the retry logic for GitHub would use the default maxAttempts (4), baseDelayMs (200), etc. However, the original code for GitHub didn't have any retry logic. So this change adds retries for GitHub API calls, which is part of the issue's scope. That's correct.

Wait, but in the GitHub's ghFetch, they don't set any options for retryWithBackoff. So the default is 4 attempts. But maybe the GitHub client should have different parameters. However, the issue doesn't specify per-client configurations, so using the same