Codex review: needs maintainer review before merge. Reviewed July 18, 2026, 5:44 PM ET / 21:44 UTC.

Summary
The PR separates ChatGPT Responses HTTP retry handling from transport exceptions and adds regression coverage that non-retryable 400/401/403 responses do not back off while retryable 503 responses still do.

PR surface: Source +4, Tests +118. Total +122 across 2 files.

Reproducibility: yes. at source level: a non-retryable HTTP response previously reached the transport retry catch, and the supplied local-server trace demonstrates the four-request baseline versus one-request fixed behavior. The review did not independently execute the harness.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: serialized state: packages/ai/src/providers/openai-chatgpt-responses.retry.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

• [P2] Let the assigned reviewer confirm the final merge result and wait for the current required checks to complete.

Risk before merge

• [P1] The PR is behind main and its broader required-check fan-out is still running; merge should use the final clean three-way result and completed checks for the reviewed head.

Maintainer options:

1. Decide the mitigation before merge
   Land the focused HTTP-versus-transport retry separation once the assigned reviewer confirms the clean merge result and current-head required checks remain green.
2. Pause or close
   Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

• [P2] No mechanical repair is identified; the active, assigned PR needs its normal final owner review and current-head merge/check confirmation.

Maintainer decision needed

• Question: Should the assigned reviewer merge this focused retry-boundary fix after the final merge result and current-head checks are confirmed?
• Rationale: The remaining action is ordinary owner judgment on an active, assigned provider-runtime change rather than a mechanical contributor repair.
• Likely owner: steipete — The timeline assigns this PR to steipete and the final retry-boundary refactor is authored by steipete.
• Options:
  • Review and merge after checks (recommended): Confirm the clean merge result preserves the tested non-retryable and retryable paths, then merge the focused fix.
  • Request a narrower revision: Request changes only if final-base review reveals a concrete regression in the HTTP or transport retry contract.

Security
Cleared: The reviewed changes are confined to provider retry control flow and tests; no dependency, workflow, permission, secret, or supply-chain change is indicated.

Real correctness fix, surgical implementation. The NonRetryableCodexResponseError class is the right design — clearly distinguishes retry-classified errors from network errors without changing the catch's general retry logic.

Two test-coverage concerns worth addressing:

1. What if parseErrorResponse(fakeResponse) itself throws? The new if (error instanceof NonRetryableCodexResponseError) { throw error; } re-throws only when the thrown error is the new class. If parseErrorResponse throws a regular Error (e.g., a malformed JSON body that fails parsing), the catch treats it as a network error and retries — but the underlying status was already classified as non-retryable. The 4-second wait could happen for a status that should have surfaced immediately.

   The fix would be to wrap the throw more defensively — e.g., check response.status after parseErrorResponse throws and decide retryability from the status code, not just from the error type. Or move the throw new NonRetryableCodexResponseError(...) outside the inner try/catch entirely so a parse failure still surfaces with the right retry classification.

2. Test for the 5xx-retry-still-works path. Tests cover 401, 403, 400 (non-retryable) and assert toHaveBeenCalledTimes(1). The retryable path (500, 502, 503, 504) is presumably tested elsewhere in openai-chatgpt-responses.test.ts, but after this change the catch has a new re-throw branch. Worth a regression test asserting that a 500 still triggers fetchMock.toHaveBeenCalledTimes(2) (or maxRetries + 1) with the new code path. If the new re-throw branch ever widens (e.g., to also re-throw on network errors), the 5xx test catches the regression.

The fix itself is correct for the documented case. These are hardening asks, not blockers.

@knoal Checked both. Neither is reachable as described, so no change here — details below in case I'm missing something.

1. parseErrorResponse cannot throw on a malformed body.

The JSON.parse is entirely inside a swallowing catch:

async function parseErrorResponse(
  response: Response,
): Promise<{ message: string; friendlyMessage?: string }> {
  const raw = await readChatGptResponsesErrorTextLimited(response);
  let message = raw || response.statusText || "Request failed";
  let friendlyMessage: string | undefined;

  try {
    const parsed = JSON.parse(raw) as { ... };
    ...
  } catch {}

  return { message, friendlyMessage };
}

A malformed JSON body returns { message: raw }. The only statement outside that try is readChatGptResponsesErrorTextLimited, and it's reading new Response(errorText, ...) — an in-memory string body, whose stream doesn't reject.

The control-flow shape you describe is right (the throw is inside the try whose catch is the retry catch), there's just no way to trigger it from this path.

2. The 5xx retry test is already in this PR.

You looked at openai-chatgpt-responses.test.ts, which only has 200/400/429. The test you're asking for is in the file this PR adds, openai-chatgpt-responses.retry.test.ts:75:

  it("still retries retryable ChatGPT responses", async () => {
    const fetchMock = vi
      .fn<typeof fetch>()
      .mockResolvedValueOnce(new Response("overloaded", { status: 503 }))
      .mockResolvedValueOnce(
        new Response(JSON.stringify({ error: { message: "Invalid credentials" } }), {
          status: 401,
        }),
      );
    ...
    expect(fetchMock).toHaveBeenCalledTimes(2);
  });

That's exactly the "new re-throw branch didn't widen" regression guard — a 503 still retries, then the following 401 stops it at 2 calls.

One real instance of the shape you're pointing at, but it predates this PR.

Two lines above parseErrorResponse:

          const fakeResponse = new Response(errorText, {
            status: response.status,
            ...

new Response(body, { status }) throws TypeError for null-body statuses (204/205/304). A 304 is !response.ok and non-retryable, so it reaches this line, throws before NonRetryableCodexResponseError is constructed, and gets retried as a network error — the exact mis-classification you describe, just at a different line. It's on main today and independent of this change, so I'd rather not pull it into this diff. Happy to file it separately if a maintainer wants it.

Merged via squash.

• Prepared head SHA: 3c2254402406fe5c1bccc2ea0d67f184cfa0155e
• Landed commit: 19d887169f78df2ea8d3caeee67dc33ab6b9466c