fix(oauth): persist and reuse rotated Codex OAuth refresh token (#6352) (#6602)

CHANGELOG.md

@@ -10,6 +10,7 @@ _Living section — bullets land here as PRs merge into `release/v3.8.47` (paral
 
 ### 🐛 Bug Fixes
 
+- **fix(oauth):** Codex/ChatGPT (and every other OAuth provider) connection stays stuck showing "Auth Failed" even after a genuinely successful token refresh ([#6352](https://github.com/diegosouzapw/OmniRoute/issues/6352)) — `updateProviderCredentials()` (the shared `onPersist` callback for the manual "Refresh token" route, the reactive per-request refresh in `chat.ts`, and the Codex/Claude auth-file importers) correctly reused the stored `refresh_token`, persisted the new `access_token`, and replaced a rotated `refresh_token`, but never cleared the stale `testStatus`/`lastError*`/`errorCode` fields left over from a prior expired/invalid refresh or upstream 401/403 — only the separate background health-check sweep did that clearing. A successful refresh now resets `testStatus` to `"active"` and clears `lastError`, `lastErrorAt`, `lastErrorType`, `lastErrorSource`, and `errorCode` (an explicit `testStatus` from the caller still wins). Regression guard: `tests/unit/codex-oauth-refresh-persist-6352.test.ts`.
 - **fix(resilience):** `headroom` combo routing did not always select the Codex account with the most free quota ([#6379](https://github.com/diegosouzapw/OmniRoute/issues/6379)) — `orderTargetsByHeadroom` (`open-sse/services/combo/quotaStrategies.ts`) already loaded the per-connection DB snapshot (with decrypted credentials) via `expandTargetsByQuotaAwareConnections`, but discarded it before calling `getSaturation`; for Codex, `fetchCodexSaturation` forwards straight to `fetchCodexQuota(connectionId, connection)`, which needs `connection` (or a prior `registerCodexConnection()` call, which never happens before headroom ranking runs) to read `accessToken` — so it returned `null` for every candidate, saturation failed open to `0` across the board, and ranking fell back to the original combo order regardless of actual free quota. `getSaturation()` and the headroom `SaturationFetcher` seam now accept and thread the loaded connection snapshot through to `fetchCodexQuota`. Kilo's dup flag vs #5903 was a false positive — that issue is about session-sticky reset-aware/least-used selection, not headroom's Codex saturation lookup. Regression guard: `tests/unit/headroom-codex-quota-snapshot-6379.test.ts`. (thanks @eidoog)
 - **fix(providers):** custom models a provider actually has are no longer dropped from the Free Provider Rankings when both **"Configured only"** and **"Available only"** filters are applied ([#6368](https://github.com/diegosouzapw/OmniRoute/issues/6368)), follow-up to #6150 — `freeProviderRankings.ts::getProviderModels()` only ever walked the static `open-sse/config/providerRegistry.ts` catalog, so a user-added custom model (e.g. a Puter `claude-fable-5` model saved as "Claude Fable 5") never entered the candidate model list the ranking scores against, and could never survive the #6150 configured/available filters even when actually configured and available. It now additively merges the provider's custom models (`db/models.ts::getCustomModels`) into that candidate list via a new pure, de-duping `mergeProviderModels()` helper, before scoring/filtering runs — catalog free/paid filtering elsewhere is untouched. Regression guard: `tests/unit/free-provider-rankings-custom-models-6368.test.ts`. (thanks @shabeer)
 - **fix(providers):** `cloudflare-ai` no longer silently drops image/non-text content parts ([#6390](https://github.com/diegosouzapw/OmniRoute/issues/6390)) — `transformRequest()`'s `flattenContent()` (added for #2539 to satisfy the Workers AI `/ai/v1/chat/completions` plain-string `content` requirement) mapped any non-text OpenAI content part (e.g. `image_url`) to `""` and joined the rest, so a request carrying an image quietly went out as text-only with the attachment gone and no error surfaced. It now throws a clear error on the first non-text part instead of dropping it silently, which the existing top-level `chatCore.ts` catch already routes through `buildErrorBody()`/`sanitizeErrorMessage()` (same pattern as `buildUrl()`'s missing-Account-ID error). Regression guard: `tests/unit/cloudflare-ai-image-parts-6390.test.ts`.

src/sse/services/tokenRefresh.ts

@@ -119,6 +119,24 @@ export async function updateProviderCredentials(connectionId: string, newCredent
 
     if (newCredentials.accessToken) {
       updates.accessToken = newCredentials.accessToken;
+      // #6352: a successful refresh proves the connection is reachable and its
+      // refresh_token is valid again — clear any stale auth-failure state
+      // (testStatus/lastError*) left over from a prior expired/invalid refresh
+      // token or an upstream 401/403. Without this, a genuinely successful
+      // rotating-refresh (e.g. Codex/OpenAI) persisted the new access/refresh
+      // token while leaving the dashboard showing "Auth Failed" forever,
+      // because the error metadata was never reset here — only the health-check
+      // sweep (tokenHealthCheck.ts::checkConnection) did this clearing, so any
+      // OTHER caller of updateProviderCredentials (the manual refresh route,
+      // the reactive per-request refresh in chat.ts) looked like it "didn't
+      // pick up" the refreshed token. Explicit `newCredentials.testStatus`
+      // below still wins for callers that need a specific terminal state.
+      updates.testStatus = "active";
+      updates.lastError = null;
+      updates.lastErrorAt = null;
+      updates.lastErrorType = null;
+      updates.lastErrorSource = null;
+      updates.errorCode = null;
     }
     if (newCredentials.refreshToken) {
       updates.refreshToken = newCredentials.refreshToken;

tests/unit/codex-oauth-refresh-persist-6352.test.ts

@@ -0,0 +1,204 @@
+/**
+ * #6352 — Codex/ChatGPT OAuth refresh: reuse + persist + rotate + clear stale
+ * "auth failed" state.
+ *
+ * Reported symptom: a ChatGPT Plus account added via Codex OAuth stops working
+ * after ~2 days and the dashboard's manual "Refresh token" button is reported
+ * as "not enough" — after clicking it the connection still shows `auth failed`.
+ *
+ * Root cause traced in this PR: `updateProviderCredentials()`
+ * (src/sse/services/tokenRefresh.ts) is the shared onPersist callback for every
+ * refresh entry point (the manual refresh route, the reactive per-request
+ * refresh in chat.ts's `checkAndRefreshToken`, the Codex auth-file importer).
+ * It correctly persists the new accessToken/refreshToken/expiresAt — but it
+ * NEVER cleared the stale auth-failure metadata (`testStatus`, `lastError`,
+ * `lastErrorType`, `lastErrorSource`, `errorCode`) left over from a prior
+ * expired/invalid refresh or upstream 401/403. Only the separate background
+ * health-check sweep (tokenHealthCheck.ts::checkConnection) did this clearing
+ * inline in its own onPersist callback. So a refresh that ACTUALLY succeeded
+ * — reusing the stored refresh_token, obtaining a fresh access_token, and even
+ * rotating in a new refresh_token — still left the connection displaying
+ * "Auth Failed" forever, because nothing ever reset the error columns.
+ *
+ * This test drives `checkAndRefreshToken("codex", ...)` — the exact function
+ * the real per-request refresh path (src/sse/handlers/chat.ts) calls — against
+ * a connection pre-seeded in a stale "auth failed" state, with a mocked Codex
+ * token endpoint. It asserts:
+ *   (a) the refresh REUSES the stored refresh_token (request body assertion),
+ *   (b) the refreshed access_token is PERSISTED back to the connection row,
+ *   (c) a ROTATED refresh_token REPLACES the previously stored one,
+ *   (d) the stale testStatus/lastError* auth-failure fields are CLEARED so the
+ *       dashboard stops showing "Auth Failed" after a refresh that worked.
+ *
+ * (d) is the part that reproduces the reported bug: before the fix in
+ * src/sse/services/tokenRefresh.ts, this assertion fails (RED) because
+ * updateProviderCredentials left testStatus/lastError untouched.
+ */
+import test from "node:test";
+import assert from "node:assert/strict";
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+
+const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-codex-refresh-6352-"));
+process.env.DATA_DIR = TEST_DATA_DIR;
+process.env.API_KEY_SECRET = "test-api-key-secret-6352";
+
+const core = await import("../../src/lib/db/core.ts");
+const providersDb = await import("../../src/lib/db/providers.ts");
+const tokenRefresh = await import("../../src/sse/services/tokenRefresh.ts");
+const { OAUTH_ENDPOINTS } = await import("../../open-sse/config/constants.ts");
+
+async function resetStorage() {
+  core.resetDbInstance();
+  fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true });
+  fs.mkdirSync(TEST_DATA_DIR, { recursive: true });
+}
+
+function jsonResponse(body: unknown, status = 200) {
+  return new Response(JSON.stringify(body), {
+    status,
+    headers: { "content-type": "application/json" },
+  });
+}
+
+type ConnectionRecord = {
+  id: string;
+  accessToken?: string | null;
+  refreshToken?: string | null;
+  testStatus?: string | null;
+  lastError?: string | null;
+  lastErrorType?: string | null;
+  lastErrorSource?: string | null;
+  errorCode?: string | null;
+};
+
+type FetchOptions = { body?: string };
+
+async function withMockedFetch<T>(
+  fetchImpl: (url: unknown, options?: FetchOptions) => Promise<Response>,
+  fn: () => Promise<T>
+): Promise<T> {
+  const originalFetch = globalThis.fetch;
+  globalThis.fetch = fetchImpl as typeof fetch;
+  try {
+    return await fn();
+  } finally {
+    globalThis.fetch = originalFetch;
+  }
+}
+
+test.beforeEach(async () => {
+  await resetStorage();
+});
+
+test.after(async () => {
+  await resetStorage();
+  fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true });
+});
+
+test("checkAndRefreshToken reuses the stored Codex refresh_token, persists the new access_token, rotates the refresh_token, and clears stale auth-failure state (#6352)", async () => {
+  const now = Date.now();
+
+  // Seed a connection in the exact "auth failed" state the issue describes:
+  // a prior refresh/request failure left testStatus/lastError* populated.
+  const connection = await providersDb.createProviderConnection({
+    provider: "codex",
+    authType: "oauth",
+    name: "ChatGPT Plus (Codex OAuth)",
+    accessToken: "codex-stale-access",
+    refreshToken: "codex-stored-refresh-token",
+    // Already past the 5-minute Codex refresh lead → checkAndRefreshToken refreshes.
+    expiresAt: new Date(now - 60_000).toISOString(),
+    testStatus: "invalid",
+    lastError: "Refresh token expired. Please re-authenticate this account.",
+    lastErrorAt: new Date(now - 120_000).toISOString(),
+    lastErrorType: "upstream_auth_error",
+    lastErrorSource: "oauth",
+    errorCode: "401",
+  } as unknown as Record<string, unknown>);
+  const connectionId = (connection as ConnectionRecord).id;
+
+  const capturedRequests: Array<{ url: string; body: string }> = [];
+
+  await withMockedFetch(
+    async (url, options: FetchOptions = {}) => {
+      const body = String(options?.body ?? "");
+      capturedRequests.push({ url: String(url), body });
+      assert.equal(String(url), OAUTH_ENDPOINTS.openai.token);
+
+      // (a) REUSE assertion: the refresh request must present the refresh_token
+      // that was actually stored on the connection — not a re-run of the full
+      // authorization_code flow, and not a stale/blank token.
+      const params = new URLSearchParams(body);
+      assert.equal(params.get("grant_type"), "refresh_token");
+      assert.equal(
+        params.get("refresh_token"),
+        "codex-stored-refresh-token",
+        "must reuse the connection's stored refresh_token"
+      );
+
+      // Simulate OpenAI rotating in a brand-new refresh_token on this refresh.
+      return jsonResponse({
+        access_token: "codex-fresh-access-token",
+        refresh_token: "codex-rotated-refresh-token",
+        expires_in: 3600,
+      });
+    },
+    async () => {
+      const connSnapshot = connection as ConnectionRecord;
+      const refreshed = await tokenRefresh.checkAndRefreshToken("codex", {
+        connectionId,
+        accessToken: connSnapshot.accessToken,
+        refreshToken: connSnapshot.refreshToken,
+        expiresAt: (connection as Record<string, unknown>).expiresAt,
+      });
+
+      assert.equal(capturedRequests.length, 1, "the Codex token endpoint must be hit exactly once");
+
+      // The in-memory result returned to the caller carries the fresh tokens.
+      assert.equal(refreshed.accessToken, "codex-fresh-access-token");
+      assert.equal(refreshed.refreshToken, "codex-rotated-refresh-token");
+
+      const stored = (await providersDb.getProviderConnectionById(
+        connectionId
+      )) as ConnectionRecord;
+
+      // (b) PERSIST assertion: the refreshed access_token must be saved to the row.
+      assert.equal(
+        stored.accessToken,
+        "codex-fresh-access-token",
+        "the refreshed access_token must be persisted back to the connection"
+      );
+
+      // (c) ROTATION assertion: the new refresh_token must REPLACE the old one.
+      assert.equal(
+        stored.refreshToken,
+        "codex-rotated-refresh-token",
+        "a rotated refresh_token must replace the previously stored one"
+      );
+      assert.notEqual(
+        stored.refreshToken,
+        "codex-stored-refresh-token",
+        "the stale, already-consumed refresh_token must not remain stored"
+      );
+
+      // (d) CLEAR-STALE-STATE assertion: a successful refresh must clear the
+      // auth-failure fields that drive the dashboard's "Auth Failed" badge.
+      // Before the fix these remained "invalid" / "upstream_auth_error" / "401"
+      // even though the token had genuinely refreshed.
+      assert.equal(
+        stored.testStatus,
+        "active",
+        "testStatus must clear to active after a successful refresh"
+      );
+      // The read path (getProviderConnectionById) strips null-valued columns via
+      // cleanNulls(), so a cleared column surfaces as `undefined`, not `null` —
+      // both mean "cleared" here.
+      assert.equal(stored.lastError ?? null, null, "lastError must be cleared");
+      assert.equal(stored.lastErrorType ?? null, null, "lastErrorType must be cleared");
+      assert.equal(stored.lastErrorSource ?? null, null, "lastErrorSource must be cleared");
+      assert.equal(stored.errorCode ?? null, null, "errorCode must be cleared");
+    }
+  );
+});