Repository navigation
Complete coderouter private-beta routing reliability - #9692
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds team-scoped CodeRouter account deletion, provider-read retries, credential-refresh observability, proxy account failover, usage timing metadata, updated credential-storage documentation, landing-page status text, and expanded tests. ChangesCodeRouter reliability and account management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DeleteAccountRoute
participant AccountRemover
participant Repository
participant LegacyVault
Client->>DeleteAccountRoute: DELETE account
DeleteAccountRoute->>AccountRemover: removeAccount(teamId, accountId)
AccountRemover->>Repository: deleteAccount(teamId, accountId)
Repository-->>AccountRemover: removal metadata
AccountRemover->>LegacyVault: delete legacy credentials under lease
AccountRemover-->>DeleteAccountRoute: cleanup status
DeleteAccountRoute-->>Client: deletion response
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (22 passed)
✨ Finishing Touches 💡 1📝 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 |
4c179d2 to
4bc5712
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@web/app/coderouter/page.tsx`:
- Around line 38-45: Move the landing-page strings in the coderouter page,
including “private beta” and the availability paragraph, to the established
next-intl locale source. Add keys and matching translations for every locale
listed in web/i18n/routing.ts under web/messages/, then read them through the
page’s existing localization mechanism instead of hardcoded English text.
In `@web/services/coderouter/accounts.ts`:
- Around line 76-88: Update removeAccount() so a failed deleteLegacy cleanup
remains recoverable when deleteRuntime reports the account is already absent:
persist an idempotent legacy-cleanup job or invoke cleanup during the retry path
before returning 404. Preserve idempotency, ensure successful retries clear
legacyCleanupPending, and add a recovery test covering failure followed by a
retry DELETE.
In `@web/services/coderouter/codexProxy.ts`:
- Around line 129-138: Extract the routing-skip tag predicate currently used by
proxyCodexRequest into a shared CodeRouter helper, then use it in
proxyCodexRequest and both account-selection loops. In codexProxy.ts at lines
129-138 and opencodeProxy.ts at lines 90-92, skip only CodeRouterRefreshBusy and
CodeRouterCredentialBroken errors; report or rethrow all other credential
failures so infrastructure errors reach the existing failure handling.
- Around line 142-150: Update the upstream fetch inside the providerRead
callback in the codex proxy to create a fresh AbortSignal.timeout(5_000) for
each retry attempt and pass it as the fetch signal, matching the bounded reads
used by sibling providers.
In `@web/services/coderouter/providerFetch.ts`:
- Around line 7-12: Update the retryable-status branch in the request flow
around `request()` and `RETRYABLE_STATUS` to cancel the discarded first
`Response` body before issuing the second request. Preserve returning the first
response for non-retryable statuses and the existing retry behavior for thrown
requests.
In `@web/services/coderouter/refresh.ts`:
- Around line 107-118: Separate the `dependencies.refresh` error handling from
the surrounding lease operations in the refresh flow. Report `provider_refresh`
with the provider tag only when `dependencies.refresh` fails; report failures
from `dependencies.read`, `dependencies.release`, `dependencies.encrypt`, or
`dependencies.complete` under the existing `rds` category without a provider
tag. Keep the existing `dependencies.fail` lease-failure recording unchanged.
In `@web/services/coderouter/repository.ts`:
- Around line 100-112: The final-account check in the account deletion flow of
web/services/coderouter/repository.ts (lines 100-112) must be serialized per
team: add a transaction-scoped per-team lock shared by both account creation and
deletion, then perform the remaining-account check and active route-token
revocation under that boundary. Extend
web/tests/coderouter-account-removal.test.ts (lines 83-124) with a database
regression test that concurrently deletes two accounts from the same team and
verifies every active route token is revoked.
In `@web/tests/coderouter-models-proxy.test.ts`:
- Around line 22-27: Update the fake select implementation in the proxy test to
inspect its third argument and record or assert the supplied excludedAccountIds,
then verify that createCodexModelsProxy passes previously attempted account IDs
on subsequent selections. Preserve the existing queue behavior while adding an
assertion matching the OpenCode proxy test’s coverage.
In `@web/tests/coderouter-refresh.test.ts`:
- Around line 113-125: Add a test alongside the existing
createCredentialRefresher cases that makes read return an expired credential on
the initial call and a valid credential after lease acquisition. Configure
release to record invocation and refresh to fail if called, then assert the
refresher returns the existing valid token, release was invoked, and the
provider refresh path was not contacted.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8b11d05a-2616-4550-be7f-166353cf6de0
📒 Files selected for processing (18)
web/app/api/coderouter/accounts/[accountId]/route.tsweb/app/api/coderouter/accounts/route.tsweb/app/coderouter/page.tsxweb/db/schema.tsweb/services/coderouter/accounts.tsweb/services/coderouter/codexProxy.tsweb/services/coderouter/observability.tsweb/services/coderouter/opencodeProxy.tsweb/services/coderouter/providerFetch.tsweb/services/coderouter/refresh.tsweb/services/coderouter/repository.tsweb/services/coderouter/usage.tsweb/tests/coderouter-account-removal.test.tsweb/tests/coderouter-encryption.test.tsweb/tests/coderouter-models-proxy.test.tsweb/tests/coderouter-opencode-proxy.test.tsweb/tests/coderouter-provider-fetch.test.tsweb/tests/coderouter-refresh.test.ts
| private beta | ||
| </span> | ||
| </div> | ||
|
|
||
| <p className="mt-5 max-w-xl font-mono text-[11px] leading-5 text-black/40"> | ||
| Codex and OpenCode Go are available now. Pi support is experimental. | ||
| Claude Max and API-key routing are coming soon. | ||
| </p> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Move the new landing-page copy into the locale catalogs.
Lines 38, 43, and 44 add raw English user-facing text. Read these strings from next-intl or the established locale source. Add matching translations for every supported locale.
As per coding guidelines, “User-facing web UI text ... must use next-intl or another locale-specific runtime source and be represented for every supported locale.” As per path instructions, update every supported locale in web/i18n/routing.ts and web/messages/.
🤖 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 `@web/app/coderouter/page.tsx` around lines 38 - 45, Move the landing-page
strings in the coderouter page, including “private beta” and the availability
paragraph, to the established next-intl locale source. Add keys and matching
translations for every locale listed in web/i18n/routing.ts under web/messages/,
then read them through the page’s existing localization mechanism instead of
hardcoded English text.
Sources: Coding guidelines, Path instructions
| const result = await dependencies.deleteRuntime({ teamId, accountId }); | ||
| if (!result.removed) return { ...result, legacyCleanupPending: false }; | ||
| try { | ||
| // Temporary rollback copy only. This call disappears after the migration | ||
| // cleanup window; failure cannot restore runtime access to the credential. | ||
| await dependencies.withLease( | ||
| teamId, | ||
| async () => await dependencies.deleteLegacy(teamId, accountId), | ||
| ); | ||
| return { ...result, legacyCleanupPending: false }; | ||
| } catch (error) { | ||
| dependencies.report("legacy_cleanup", error); | ||
| return { ...result, legacyCleanupPending: true }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file sizes =="
wc -l web/services/coderouter/accounts.ts web/app/api/coderouter/accounts/[accountId]/route.ts web/tests/coderouter-account-removal.test.ts web/services/coderouter/vault.ts web/services/coderouter/repository.ts 2>/dev/null || true
echo "== accounts.ts relevant section =="
sed -n '1,140p' web/services/coderouter/accounts.ts
echo "== route.ts =="
sed -n '1,90p' 'web/app/api/coderouter/accounts/[accountId]/route.ts'
echo "== tests relevant section =="
sed -n '1,180p' web/tests/coderouter-account-removal.test.ts
echo "== search legacy cleanup pending / reconciler / deleteVaultCredential =="
rg -n "legacyCleanupPending|clean.*legacy|deleteLegacy|deleteVault|legacy cleanup|legacy_cleanup|Delete.*Account|removeAccount" web -S || trueRepository: manaflow-ai/cmux
Length of output: 13763
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file sizes =="
wc -l web/services/coderouter/accounts.ts 'web/app/api/coderouter/accounts/[accountId]/route.ts' web/tests/coderouter-account-removal.test.ts web/services/coderouter/vault.ts web/services/coderouter/repository.ts 2>/dev/null || true
echo "== accounts.ts relevant section =="
sed -n '1,140p' web/services/coderouter/accounts.ts
echo "== route.ts =="
sed -n '1,90p' 'web/app/api/coderouter/accounts/[accountId]/route.ts'
echo "== tests relevant section =="
sed -n '1,180p' web/tests/coderouter-account-removal.test.ts
echo "== search legacy cleanup pending / reconciler / deleteVaultCredential =="
rg -n "legacyCleanupPending|clean.*legacy|deleteLegacy|deleteVault|legacy cleanup|legacy_cleanup|Delete.*Account|removeAccount" web -S || trueRepository: manaflow-ai/cmux
Length of output: 13763
Sensitive Data Exposure (CWE-459)
Reachability: Internal
Reachability path
● Entry
web/app/api/coderouter/accounts/route.ts:8
runtime
│
▼
● Hop
web/app/api/coderouter/accounts/[accountId]/route.ts:9
createDeleteAccountHandler
│
▼
● Sink
web/services/coderouter/accounts.ts
Make failed legacy cleanup recoverable.
removeAccount() deletes the runtime account before the leased legacy vault cleanup, and cleanup failures return legacyCleanupPending: true. A retry DELETE reuses removeAccount(); when the runtime account is already absent, 404 is returned, so this path cannot retry the vault cleanup. Persist an idempotent cleanup job or retry legacy cleanup even after the runtime account is gone, and add a recovery test for failed legacy cleanup.
🤖 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 `@web/services/coderouter/accounts.ts` around lines 76 - 88, Update
removeAccount() so a failed deleteLegacy cleanup remains recoverable when
deleteRuntime reports the account is already absent: persist an idempotent
legacy-cleanup job or invoke cleanup during the retry path before returning 404.
Preserve idempotency, ensure successful retries clear legacyCleanupPending, and
add a recovery test covering failure followed by a retry DELETE.
| let credential; | ||
| try { | ||
| credential = await dependencies.credential({ | ||
| teamId: identity.teamId, | ||
| accountId: account.id, | ||
| expectedRevision: account.vaultRevision, | ||
| }); | ||
| } catch { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unclassified credential errors turn infrastructure outages into silent 503s. Both new account-selection loops catch every error from the credential dependency and continue to the next account. Neither one distinguishes a routing-skip condition (CodeRouterRefreshBusy, CodeRouterCredentialBroken) from an infrastructure fault such as a KMS outage, an RDS failure, or a decryption error. The caller receives no_usable_account (503) after the attempt bound, and reportCoderouterFailure never runs, so the outage produces no alert. proxyCodexRequest at web/services/coderouter/codexProxy.ts lines 49-56 already classifies the error correctly, so the two paths now disagree.
web/services/coderouter/codexProxy.ts#L129-L138: replace the barecatchwith the tag check used byproxyCodexRequest, and rethrow anything that is not a routing-skip tag.web/services/coderouter/opencodeProxy.ts#L90-L92: replace the barecatchinopenCodeAccountwith the same tag check, and report or rethrow anything else instead of skipping silently.
Extract the skip predicate into one shared helper in the CodeRouter service directory, then call it from all three loops. That keeps the entrypoints from diverging again.
As per coding guidelines: "When behavior has multiple entrypoints, implement one shared action path and verify every entrypoint rather than duplicating logic."
📍 Affects 2 files
web/services/coderouter/codexProxy.ts#L129-L138(this comment)web/services/coderouter/opencodeProxy.ts#L90-L92
🤖 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 `@web/services/coderouter/codexProxy.ts` around lines 129 - 138, Extract the
routing-skip tag predicate currently used by proxyCodexRequest into a shared
CodeRouter helper, then use it in proxyCodexRequest and both account-selection
loops. In codexProxy.ts at lines 129-138 and opencodeProxy.ts at lines 90-92,
skip only CodeRouterRefreshBusy and CodeRouterCredentialBroken errors; report or
rethrow all other credential failures so infrastructure errors reach the
existing failure handling.
Source: Coding guidelines
| try { | ||
| const first = await request(); | ||
| return RETRYABLE_STATUS.has(first.status) ? await request() : first; | ||
| } catch { | ||
| return await request(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Release the discarded response body before you retry.
Line 9 drops the first Response when the status is retryable. The body stream is never read or cancelled. The underlying connection and buffered bytes stay allocated until garbage collection. This runs on the Codex models, OpenCode config, and usage hot paths, so 5xx bursts from a provider accumulate unreleased streams.
Cancel the body before you issue the retry.
🔧 Proposed fix to release the abandoned body
export async function fetchProviderRead(
request: () => Promise<Response>,
): Promise<Response> {
try {
const first = await request();
- return RETRYABLE_STATUS.has(first.status) ? await request() : first;
+ if (!RETRYABLE_STATUS.has(first.status)) return first;
+ await first.body?.cancel().catch(() => undefined);
+ return await request();
} catch {
return await request();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const first = await request(); | |
| return RETRYABLE_STATUS.has(first.status) ? await request() : first; | |
| } catch { | |
| return await request(); | |
| } | |
| try { | |
| const first = await request(); | |
| if (!RETRYABLE_STATUS.has(first.status)) return first; | |
| await first.body?.cancel().catch(() => undefined); | |
| return await request(); | |
| } catch { | |
| return await request(); | |
| } |
🤖 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 `@web/services/coderouter/providerFetch.ts` around lines 7 - 12, Update the
retryable-status branch in the request flow around `request()` and
`RETRYABLE_STATUS` to cancel the discarded first `Response` body before issuing
the second request. Preserve returning the first response for non-retryable
statuses and the existing retry behavior for thrown requests.
| } catch (error) { | ||
| const terminal = isTerminalRefreshError(error); | ||
| await failRefreshLease( | ||
| const terminal = dependencies.isTerminal(error); | ||
| reportCoderouterFailure("provider_refresh", error, { | ||
| provider: currentProvider(before.credential), | ||
| terminal, | ||
| }); | ||
| await dependencies.fail( | ||
| input.accountId, | ||
| leaseId, | ||
| terminal, | ||
| refreshFailureCode(error), | ||
| dependencies.failureCode(error), | ||
| ).catch(() => undefined); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not report persistence failures as provider failures.
The catch at line 107 covers the whole lease block, not only dependencies.refresh. dependencies.read (line 82), dependencies.release (line 87), dependencies.encrypt (line 92), and dependencies.complete (line 99) all land here. A lost CAS race in completeRefreshLease or an RDS outage therefore emits coderouter.provider_refresh with a provider tag. On-call then investigates the upstream provider for a database fault.
observability.ts already declares an rds failure category, and this path never uses it. The PR objective is scrubbed, accurate alert signals, so the category must match the failing subsystem.
Narrow the reported category to the operation that failed. Keep the lease-failure recording unchanged.
🔧 Proposed fix to separate the provider failure from the persistence failure
const refreshed = await dependencies.refresh(current.credential);
+ let persisted = false;
const encrypted = await dependencies.encrypt({
teamId: input.teamId,
accountId: input.accountId,
provider: refreshed.provider,
credentialRevision: current.envelope.credentialRevision + 1,
credential: refreshed,
});
+ persisted = true;
await dependencies.complete({
accountId: input.accountId,
leaseId,
expectedRevision: current.envelope.credentialRevision,
credential: refreshed,
encrypted,
});
return refreshed;
} catch (error) {
const terminal = dependencies.isTerminal(error);
- reportCoderouterFailure("provider_refresh", error, {
- provider: currentProvider(before.credential),
- terminal,
- });
+ reportCoderouterFailure(persisted ? "rds" : "provider_refresh", error, {
+ provider: currentProvider(before.credential),
+ terminal,
+ });A clearer alternative is to wrap only dependencies.refresh in its own try/catch and report rds from an outer catch.
🤖 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 `@web/services/coderouter/refresh.ts` around lines 107 - 118, Separate the
`dependencies.refresh` error handling from the surrounding lease operations in
the refresh flow. Report `provider_refresh` with the provider tag only when
`dependencies.refresh` fails; report failures from `dependencies.read`,
`dependencies.release`, `dependencies.encrypt`, or `dependencies.complete` under
the existing `rds` category without a provider tag. Keep the existing
`dependencies.fail` lease-failure recording unchanged.
| test("does not return a refreshed token when the RDS CAS fails", async () => { | ||
| let failedLease = false; | ||
| const refresh = createCredentialRefresher(fakeDependencies({ | ||
| complete: async () => { | ||
| throw new Error("database unavailable"); | ||
| }, | ||
| fail: async () => { | ||
| failedLease = true; | ||
| }, | ||
| })); | ||
| await expect(refresh(input())).rejects.toThrow("database unavailable"); | ||
| expect(failedLease).toBe(true); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the post-claim re-read that releases the lease.
createCredentialRefresher re-reads the credential after it claims the lease. If that second read shows a still-valid token, it calls dependencies.release and returns the existing credential without contacting the provider. That branch is the core fix for the double-refresh race, and no test exercises it.
Add a test where read returns an expired credential first and a valid credential second. Assert that release runs, that refresh never runs, and that the returned token is the existing one.
🧪 Proposed test for the release branch
+ test("releases the lease when another refresh already rotated the token", async () => {
+ let released = false;
+ let refreshCalls = 0;
+ const reads = [
+ { envelope, credential: expired },
+ { envelope, credential: { ...expired, expiresAt: Date.now() + 3_600_000 } },
+ ];
+ const refresh = createCredentialRefresher(fakeDependencies({
+ read: async () => reads.shift() ?? { envelope, credential: expired },
+ release: async () => {
+ released = true;
+ },
+ refresh: async () => {
+ refreshCalls++;
+ return refreshed();
+ },
+ }));
+ const result = await refresh(input());
+ expect(result.accessToken).toBe("old-access");
+ expect(released).toBe(true);
+ expect(refreshCalls).toBe(0);
+ });🤖 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 `@web/tests/coderouter-refresh.test.ts` around lines 113 - 125, Add a test
alongside the existing createCredentialRefresher cases that makes read return an
expired credential on the initial call and a valid credential after lease
acquisition. Configure release to record invocation and refresh to fail if
called, then assert the refresher returns the existing valid token, release was
invoked, and the provider refresh path was not contacted.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
♻️ Duplicate comments (6)
web/tests/coderouter-refresh.test.ts (1)
113-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the post-claim re-read that releases the lease.
createCredentialRefresherre-reads the credential after it claims the lease. If the second read returns a still-valid token, it callsdependencies.releaseand returns the existing credential without contacting the provider. That branch is the fix for the double-refresh race. No test in this file exercises it, andfakeDependencies.releaseis never asserted.🧪 Proposed test for the release branch
+ test("releases the lease when another refresh already rotated the token", async () => { + let released = false; + let refreshCalls = 0; + const reads = [ + { envelope, credential: expired }, + { envelope, credential: { ...expired, expiresAt: Date.now() + 3_600_000 } }, + ]; + const refresh = createCredentialRefresher(fakeDependencies({ + read: async () => reads.shift() ?? { envelope, credential: expired }, + release: async () => { + released = true; + }, + refresh: async () => { + refreshCalls++; + return refreshed(); + }, + })); + const result = await refresh(input()); + expect(result.accessToken).toBe("old-access"); + expect(released).toBe(true); + expect(refreshCalls).toBe(0); + });🤖 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 `@web/tests/coderouter-refresh.test.ts` around lines 113 - 125, Add a test in coderouter-refresh.test.ts covering createCredentialRefresher’s post-claim re-read: configure the second credential read to return a still-valid token, assert dependencies.release is called, verify the existing credential is returned, and confirm the provider is not contacted.web/services/coderouter/codexProxy.ts (2)
142-150: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the models fetch with a timeout.
This fetch sets no
AbortSignal. The sibling provider reads both bound themselves:remoteConfiginweb/services/coderouter/opencodeProxy.tsat line 101, and the usage read inweb/services/coderouter/usage.ts, useAbortSignal.timeout(5_000).
dependencies.providerReadnow replays the call once on a transient failure. An unresponsive provider connection therefore holds the request thread for two unbounded attempts instead of one. The retry wrapper increases the existing exposure.Create the signal inside the callback so the retry attempt receives a fresh deadline.
🔧 Proposed fix to bound the upstream read
upstream = await dependencies.providerRead(() => fetch(upstreamUrl, { headers: { authorization: `Bearer ${credential.accessToken}`, "chatgpt-account-id": credential.accountId, originator: "codex_cli_rs", "user-agent": request.headers.get("user-agent") ?? "coderouter", }, cache: "no-store", + signal: AbortSignal.timeout(5_000), }));🤖 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 `@web/services/coderouter/codexProxy.ts` around lines 142 - 150, Update the upstream fetch callback in the models-read flow to pass a fresh AbortSignal.timeout(5_000) on every attempt. Create the signal inside the callback passed to dependencies.providerRead, and preserve the existing request headers and no-store cache behavior.
129-138: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnclassified credential errors turn infrastructure outages into silent 503s. Both account-selection loops catch every error from the credential dependency and continue to the next account. Neither distinguishes a routing-skip condition (
CodeRouterRefreshBusy,CodeRouterCredentialBroken) from an infrastructure fault such as a KMS outage, an RDS failure, or a decryption error. After the 8-attempt bound the caller receivesno_usable_account(503), andreportCoderouterFailurenever runs, so the outage produces no alert.proxyCodexRequestatweb/services/coderouter/codexProxy.tslines 49-56 classifies the error by_tag, so the entrypoints now disagree.
web/services/coderouter/codexProxy.ts#L129-L138: replace the barecatchwith the_tagcheck used byproxyCodexRequest, and rethrow anything that is not a routing-skip tag.web/services/coderouter/opencodeProxy.ts#L90-L92: replace the barecatchinopenCodeAccountwith the same check, and report or rethrow anything else instead of skipping silently.Extract the skip predicate into one shared helper in the CodeRouter service directory, then call it from all three loops. If you narrow the catch in
openCodeAccount, updateweb/tests/coderouter-opencode-proxy.test.tsline 46 to throw a tagged error instead of a plainError.As per coding guidelines: "When behavior has multiple entrypoints, implement one shared action path and verify every entrypoint rather than duplicating logic."
🤖 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 `@web/services/coderouter/codexProxy.ts` around lines 129 - 138, Unify credential-error handling across proxy account-selection loops by extracting a shared CodeRouter helper that identifies only CodeRouterRefreshBusy and CodeRouterCredentialBroken as skippable. In web/services/coderouter/codexProxy.ts lines 129-138, web/services/coderouter/codexProxy.ts lines 49-56, and web/services/coderouter/opencodeProxy.ts lines 90-92, use the helper in each catch block and rethrow non-skippable infrastructure errors so reporting can occur; update web/tests/coderouter-opencode-proxy.test.ts line 46 to throw a tagged routing-skip error if required.Source: Coding guidelines
web/tests/coderouter-models-proxy.test.ts (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the proxy excludes accounts it already attempted.
The fake
selectignores its third argument. ProductionselectAccountForRequestusesexcludedAccountIdsto skip accounts already tried in the same request. Nothing here verifies thatcreateCodexModelsProxypopulatesattempted.If the proxy stopped passing
attempted, this test would still pass, because the fake advances its queue on every call. In production the same unusable account would be reselected until the 8-attempt bound, and the request would returnno_usable_account.The OpenCode test asserts this contract at
web/tests/coderouter-opencode-proxy.test.tsline 58. Apply the same check here.🧪 Proposed change to record and assert the excluded account IDs
let selectedAccounts = ["account-1"]; let unusableAccounts = new Set<string>(); +let excludedAccounts: string[] = [];- select: async () => { + select: async (_teamId, _provider, excluded) => { + excludedAccounts = [...(excluded ?? [])]; const id = selectedAccounts.shift(); return id ? { id, vaultRevision: 1, credentialExpiresAt: new Date() } : null; },beforeEach(() => { selectedAccounts = ["account-1"]; unusableAccounts = new Set(); + excludedAccounts = []; });expect(response.status).toBe(200); + expect(excludedAccounts).toContain("refreshing-account"); expect(await response.json()).toEqual({ models: [{ slug: "gpt-test" }], });🤖 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 `@web/tests/coderouter-models-proxy.test.ts` around lines 22 - 27, Update the fake select implementation in the createCodexModelsProxy test to inspect its excludedAccountIds argument and avoid returning accounts whose IDs were already attempted. Add an assertion that the proxy passes the accumulated attempted IDs to selectAccountForRequest, matching the contract covered by the OpenCode proxy test.web/services/coderouter/providerFetch.ts (1)
7-12: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancel the discarded response body before the retry.
Line 9 drops the first
Responsewhen the status is retryable. The body stream is never read or cancelled, so the connection and buffered bytes stay allocated until garbage collection. This helper now runs on the Codex models path, the OpenCode config path, and the usage path, so a provider 5xx burst accumulates unreleased streams.🔧 Proposed fix to release the abandoned body
export async function fetchProviderRead( request: () => Promise<Response>, ): Promise<Response> { try { const first = await request(); - return RETRYABLE_STATUS.has(first.status) ? await request() : first; + if (!RETRYABLE_STATUS.has(first.status)) return first; + await first.body?.cancel().catch(() => undefined); + return await request(); } catch { return await request(); } }🤖 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 `@web/services/coderouter/providerFetch.ts` around lines 7 - 12, Update the retryable-response branch in the request helper to cancel or otherwise release the discarded first Response body before issuing the second request. Preserve the existing retry behavior for retryable statuses and the catch-path retry, ensuring only the abandoned response stream is cleaned up.web/services/coderouter/refresh.ts (1)
107-118: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReport persistence failures under the
rdscategory, notprovider_refresh.The
catchat line 107 covers the whole lease block.dependencies.read(line 82),dependencies.release(line 87),dependencies.encrypt(line 92), anddependencies.complete(line 99) all land here. A lost CAS race incompleteRefreshLeaseor an RDS outage therefore emitscoderouter.provider_refreshwith aprovidertag, and on-call investigates the upstream provider for a database fault.
observability.tsdeclares anrdsfailure category. No path in this file uses it.The same mismatch exists at line 70.
dependencies.readresolves toreadCredential, which throwsCodeRouterCredentialBroken("encrypted credential not found")for a missing row. That storage condition is reported ascredential_decrypt.Wrap only the provider call in its own
try/catch, and report the surrounding storage operations underrds.🔧 Proposed fix to separate the provider failure from the persistence failure
const refreshed = await dependencies.refresh(current.credential); + let persisted = false; const encrypted = await dependencies.encrypt({ teamId: input.teamId, accountId: input.accountId, provider: refreshed.provider, credentialRevision: current.envelope.credentialRevision + 1, credential: refreshed, }); + persisted = true; await dependencies.complete({ accountId: input.accountId, leaseId, expectedRevision: current.envelope.credentialRevision, credential: refreshed, encrypted, }); return refreshed; } catch (error) { const terminal = dependencies.isTerminal(error); - reportCoderouterFailure("provider_refresh", error, { + reportCoderouterFailure(persisted ? "rds" : "provider_refresh", error, { provider: currentProvider(before.credential), terminal, });🤖 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 `@web/services/coderouter/refresh.ts` around lines 107 - 118, Restructure the lease flow so only the provider call is handled by the existing provider-refresh error reporting, while storage operations including dependencies.read, release, encrypt, complete, and fail use the rds failure category without a provider tag. Move the provider try/catch around the provider invocation and update the outer persistence catch accordingly, preserving the existing terminal handling and failure-code propagation.
🤖 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.
Duplicate comments:
In `@web/services/coderouter/codexProxy.ts`:
- Around line 142-150: Update the upstream fetch callback in the models-read
flow to pass a fresh AbortSignal.timeout(5_000) on every attempt. Create the
signal inside the callback passed to dependencies.providerRead, and preserve the
existing request headers and no-store cache behavior.
- Around line 129-138: Unify credential-error handling across proxy
account-selection loops by extracting a shared CodeRouter helper that identifies
only CodeRouterRefreshBusy and CodeRouterCredentialBroken as skippable. In
web/services/coderouter/codexProxy.ts lines 129-138,
web/services/coderouter/codexProxy.ts lines 49-56, and
web/services/coderouter/opencodeProxy.ts lines 90-92, use the helper in each
catch block and rethrow non-skippable infrastructure errors so reporting can
occur; update web/tests/coderouter-opencode-proxy.test.ts line 46 to throw a
tagged routing-skip error if required.
In `@web/services/coderouter/providerFetch.ts`:
- Around line 7-12: Update the retryable-response branch in the request helper
to cancel or otherwise release the discarded first Response body before issuing
the second request. Preserve the existing retry behavior for retryable statuses
and the catch-path retry, ensuring only the abandoned response stream is cleaned
up.
In `@web/services/coderouter/refresh.ts`:
- Around line 107-118: Restructure the lease flow so only the provider call is
handled by the existing provider-refresh error reporting, while storage
operations including dependencies.read, release, encrypt, complete, and fail use
the rds failure category without a provider tag. Move the provider try/catch
around the provider invocation and update the outer persistence catch
accordingly, preserving the existing terminal handling and failure-code
propagation.
In `@web/tests/coderouter-models-proxy.test.ts`:
- Around line 22-27: Update the fake select implementation in the
createCodexModelsProxy test to inspect its excludedAccountIds argument and avoid
returning accounts whose IDs were already attempted. Add an assertion that the
proxy passes the accumulated attempted IDs to selectAccountForRequest, matching
the contract covered by the OpenCode proxy test.
In `@web/tests/coderouter-refresh.test.ts`:
- Around line 113-125: Add a test in coderouter-refresh.test.ts covering
createCredentialRefresher’s post-claim re-read: configure the second credential
read to return a still-valid token, assert dependencies.release is called,
verify the existing credential is returned, and confirm the provider is not
contacted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8aa14d1a-fe32-4be0-9f82-627216916f8b
📒 Files selected for processing (18)
web/app/api/coderouter/accounts/[accountId]/route.tsweb/app/api/coderouter/accounts/route.tsweb/app/coderouter/page.tsxweb/db/schema.tsweb/services/coderouter/accounts.tsweb/services/coderouter/codexProxy.tsweb/services/coderouter/observability.tsweb/services/coderouter/opencodeProxy.tsweb/services/coderouter/providerFetch.tsweb/services/coderouter/refresh.tsweb/services/coderouter/repository.tsweb/services/coderouter/usage.tsweb/tests/coderouter-account-removal.test.tsweb/tests/coderouter-encryption.test.tsweb/tests/coderouter-models-proxy.test.tsweb/tests/coderouter-opencode-proxy.test.tsweb/tests/coderouter-provider-fetch.test.tsweb/tests/coderouter-refresh.test.ts
4bc5712 to
138dc42
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
138dc42 to
e61c2fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
web/tests/coderouter-models-proxy.test.ts (1)
22-27: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert that the proxy excludes accounts it already attempted.
The fake
selectat line 22 takes no parameters, so it discardsexcludedAccountIds. ProductionselectAccountForRequestuses that argument to skip accounts already tried in the same request. Nothing here verifies thatcreateCodexModelsProxypopulatesattempted.If the proxy stopped passing
attempted, this test would still pass, because the fake advances its queue on every call. In production the same unusable account would be reselected until the 8-attempt bound, and the request would returnno_usable_account.The OpenCode test asserts this contract at
web/tests/coderouter-opencode-proxy.test.tsline 58. A previous review thread marked this as fixed, but the current code does not read the argument.🧪 Proposed change to record and assert the excluded account IDs
let selectedAccounts = ["account-1"]; let unusableAccounts = new Set<string>(); +let excludedAccounts: string[] = [];- select: async () => { + select: async (_teamId, _provider, excluded) => { + excludedAccounts = [...(excluded ?? [])]; const id = selectedAccounts.shift(); return id ? { id, vaultRevision: 1, credentialExpiresAt: new Date() } : null; },beforeEach(() => { selectedAccounts = ["account-1"]; unusableAccounts = new Set(); + excludedAccounts = []; });expect(response.status).toBe(200); + expect(excludedAccounts).toContain("refreshing-account"); expect(await response.json()).toEqual({ models: [{ slug: "gpt-test" }], });🤖 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 `@web/tests/coderouter-models-proxy.test.ts` around lines 22 - 27, Update the fake select implementation in the proxy test to accept and record the excludedAccountIds argument, then assert that createCodexModelsProxy passes previously attempted account IDs on subsequent selections. Mirror the existing contract assertion from the OpenCode proxy test, while preserving the selectedAccounts queue behavior.
🤖 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 `@web/app/api/coderouter/accounts/route.ts`:
- Around line 32-37: Update the Server-Timing construction in the accounts route
to distinguish timings from the current request versus a cached refresh: have
accountsWithUsage return cache-hit/refresh metadata separately, emit rds and
provider timings only when this request performed the refresh, and emit a cache
metric for cache hits while preserving auth, serialize, and total timings.
In `@web/services/coderouter/codexProxy.ts`:
- Around line 142-150: Update the fetch call in fetchProviderRead to pass a
fresh AbortSignal.timeout(5_000) inside the providerRead callback, ensuring
every retry gets its own deadline. Wrap this provider read in the account
iteration’s existing failure handling so rejected transport errors are treated
like rate-limited accounts and processing continues to the next account.
In `@web/services/coderouter/opencodeProxy.ts`:
- Around line 198-200: Move the shared `isRecord` type guard into a common
CodeRouter helper module, then remove the local definitions from
`opencodeProxy.ts`, `refresh.ts`, `accounts.ts`, `usage.ts`, `vault.ts`, and
`encryption.ts` and import the shared implementation consistently. Use one
uniform `Record<string, unknown>` signature, including replacing the differing
`vault.ts` variant.
---
Duplicate comments:
In `@web/tests/coderouter-models-proxy.test.ts`:
- Around line 22-27: Update the fake select implementation in the proxy test to
accept and record the excludedAccountIds argument, then assert that
createCodexModelsProxy passes previously attempted account IDs on subsequent
selections. Mirror the existing contract assertion from the OpenCode proxy test,
while preserving the selectedAccounts queue behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fd9008f5-e763-47bf-8de7-258ffd75964f
📒 Files selected for processing (18)
web/app/api/coderouter/accounts/[accountId]/route.tsweb/app/api/coderouter/accounts/route.tsweb/app/coderouter/page.tsxweb/db/schema.tsweb/services/coderouter/accounts.tsweb/services/coderouter/codexProxy.tsweb/services/coderouter/observability.tsweb/services/coderouter/opencodeProxy.tsweb/services/coderouter/providerFetch.tsweb/services/coderouter/refresh.tsweb/services/coderouter/repository.tsweb/services/coderouter/usage.tsweb/tests/coderouter-account-removal.test.tsweb/tests/coderouter-encryption.test.tsweb/tests/coderouter-models-proxy.test.tsweb/tests/coderouter-opencode-proxy.test.tsweb/tests/coderouter-provider-fetch.test.tsweb/tests/coderouter-refresh.test.ts
| const serverTiming = [ | ||
| timing("auth", authMs), | ||
| timing("rds", result.timing.rdsMs), | ||
| timing("provider", result.timing.providerMs), | ||
| timing("serialize", serializeMs), | ||
| timing("total", performance.now() - startedAt), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not report cached refresh time as current request time.
accountsWithUsage can return a cached result whose rdsMs and providerMs were captured during an earlier refresh. These lines emit those values in the current Server-Timing response, while total measures only the current request. A cache hit can therefore report RDS or provider time greater than total time.
Return cache-hit metadata separately. Emit refresh timings only when this request performed the refresh. Emit a cache metric for cache hits.
🤖 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 `@web/app/api/coderouter/accounts/route.ts` around lines 32 - 37, Update the
Server-Timing construction in the accounts route to distinguish timings from the
current request versus a cached refresh: have accountsWithUsage return
cache-hit/refresh metadata separately, emit rds and provider timings only when
this request performed the refresh, and emit a cache metric for cache hits while
preserving auth, serialize, and total timings.
| function isRecord(value: unknown): value is Record<string, unknown> { | ||
| return typeof value === "object" && value !== null && !Array.isArray(value); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare every isRecord definition in the coderouter service directory.
set -euo pipefail
fd . --type f --extension ts | xargs rg -nP -A2 'function\s+isRecord\s*\('Repository: manaflow-ai/cmux
Length of output: 4130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== coderouter isRecord definitions =="
for f in \
web/services/coderouter/opencodeProxy.ts \
web/services/coderouter/refresh.ts \
web/services/coderouter/accounts.ts \
web/services/coderouter/usage.ts \
web/services/coderouter/vault.ts \
web/services/coderouter/encryption.ts
do
if [ -f "$f" ]; then
echo "--- $f"
rg -n -A2 'function\s+isRecord\s*\(' "$f"
else
echo "--- missing $f"
fi
done
echo "== imports of helper locations / nearby isRecord usage in coderouter =="
for f in \
web/services/coderouter/opencodeProxy.ts \
web/services/coderouter/refresh.ts \
web/services/coderouter/accounts.ts \
web/services/coderouter/usage.ts \
web/services/coderouter/vault.ts \
web/services/coderouter/encryption.ts
do
if [ -f "$f" ]; then
echo "--- $f"
python3 - <<'PY' "$f"
import sys, re
p=sys.argv[1]
txt=open(p).read()
lines=txt.splitlines()
for i,l in enumerate(lines,1):
if re.search(r'\bisRecord\s*\(', l):
print(f"{i}: {l.strip()}")
PY
fi
doneRepository: manaflow-ai/cmux
Length of output: 3192
Share one isRecord definition across CodeRouter files.
isRecord is defined locally in web/services/coderouter/opencodeProxy.ts, web/services/coderouter/refresh.ts, web/services/coderouter/accounts.ts, web/services/coderouter/usage.ts, web/services/coderouter/vault.ts, and web/services/coderouter/encryption.ts. Several definitions differ in signature, including the Record<string, any> return type in vault.ts. Move one definition into a shared CodeRouter helper module and import it consistently.
🤖 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 `@web/services/coderouter/opencodeProxy.ts` around lines 198 - 200, Move the
shared `isRecord` type guard into a common CodeRouter helper module, then remove
the local definitions from `opencodeProxy.ts`, `refresh.ts`, `accounts.ts`,
`usage.ts`, `vault.ts`, and `encryption.ts` and import the shared implementation
consistently. Use one uniform `Record<string, unknown>` signature, including
replacing the differing `vault.ts` variant.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
web/tests/coderouter-refresh.test.ts (1)
113-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the post-claim re-read that releases the lease.
createCredentialRefresherre-reads the credential after it claims the lease at line 82 ofweb/services/coderouter/refresh.ts. If the second read shows a still-valid token, it callsdependencies.releaseand returns the existing credential without contacting the provider. That branch is the core fix for the double-refresh race and no test exercises it.Add a test where
readreturns an expired credential first and a valid credential second. Assertreleaseruns,refreshnever runs, and the returned token is the existing one.🤖 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 `@web/tests/coderouter-refresh.test.ts` around lines 113 - 125, Add a test alongside the existing createCredentialRefresher tests where read first returns an expired credential and then a valid credential after the lease claim. Assert the post-claim re-read path invokes release, does not invoke the provider refresh operation, and returns the existing valid token.web/tests/coderouter-models-proxy.test.ts (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the proxy excludes accounts it already attempted.
The fake
selectstill ignores its third argument. ProductionselectAccountForRequestusesexcludedAccountIdsto skip accounts already tried in this request. IfcreateCodexModelsProxystopped passingattempted, the test at lines 68-80 would still pass, because the fake advances its queue on every call. In production the same unusable account would be reselected until the 8-attempt bound and the request would returnno_usable_account.The OpenCode suite asserts this contract at
web/tests/coderouter-opencode-proxy.test.tsline 58. Apply the same check here.🧪 Proposed change
- select: async () => { + select: async (_teamId, _provider, excluded) => { + excludedAccounts = [...(excluded ?? [])]; const id = selectedAccounts.shift(); return id ? { id, vaultRevision: 1, credentialExpiresAt: new Date() } : null; },expect(response.status).toBe(200); + expect(excludedAccounts).toContain("refreshing-account");🤖 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 `@web/tests/coderouter-models-proxy.test.ts` around lines 22 - 27, Update the fake select implementation in the proxy test to honor its excludedAccountIds argument by skipping already-attempted account IDs before returning an account. Ensure the test verifies createCodexModelsProxy passes attempted IDs through select, matching the contract covered by the OpenCode proxy test.web/services/coderouter/refresh.ts (1)
107-118: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not report persistence failures as provider failures.
The
catchat line 107 covers the whole lease block, not onlydependencies.refresh.dependencies.read(line 82),dependencies.release(line 87),dependencies.encrypt(line 92), anddependencies.complete(line 99) all land here. A lost CAS race incompleteRefreshLeaseor an RDS outage therefore emitscoderouter.provider_refreshwith aprovidertag.observability.tsalready declares anrdscategory and this path never uses it.Wrap only
dependencies.refreshin its owntry/catchand reportrdsfrom the outercatch. Keep the lease-failure recording unchanged.🤖 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 `@web/services/coderouter/refresh.ts` around lines 107 - 118, The catch surrounding the lease workflow currently reports every failure as a provider refresh failure. In the refresh flow, isolate dependencies.refresh in its own try/catch so only provider refresh errors call reportCoderouterFailure with the provider context; change the outer catch to report the rds category for persistence failures while keeping the existing dependencies.fail lease-failure recording unchanged.web/services/coderouter/opencodeProxy.ts (1)
198-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one
isRecorddefinition across CodeRouter files.
isRecordis defined locally inopencodeProxy.ts,refresh.ts,accounts.ts,usage.ts,vault.ts, andencryption.ts. Move one definition into a shared CodeRouter helper module and import it. This change already narrows the local copy toRecord<string, unknown>, so it is the right moment to consolidate.🤖 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 `@web/services/coderouter/opencodeProxy.ts` around lines 198 - 200, Move the local isRecord definition from opencodeProxy.ts into a shared CodeRouter helper module, then remove the duplicate definitions from opencodeProxy.ts, refresh.ts, accounts.ts, usage.ts, vault.ts, and encryption.ts and import the shared type guard wherever used. Preserve its current Record<string, unknown> narrowing and runtime checks.web/services/coderouter/providerFetch.ts (1)
7-12: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancel the discarded response body before the retry.
Line 9 drops the first
Responsewhen the status is retryable. The body stream is never read or cancelled, so buffered bytes and the connection stay allocated until garbage collection. This path runs for Codex models, OpenCode config, and usage reads.🔧 Proposed fix
try { const first = await request(); - return RETRYABLE_STATUS.has(first.status) ? await request() : first; + if (!RETRYABLE_STATUS.has(first.status)) return first; + await first.body?.cancel().catch(() => undefined); + return await request(); } catch { return await request(); }🤖 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 `@web/services/coderouter/providerFetch.ts` around lines 7 - 12, Update the retry branch in the request flow around the initial response to cancel or otherwise dispose of first.body before issuing the second request when the status is retryable. Preserve returning the retry response, and leave the catch-path retry behavior unchanged.web/services/coderouter/codexProxy.ts (2)
142-150: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the models read and handle a rejected provider read.
Two defects share this call.
First, this
fetchsets noAbortSignal. The sibling provider reads bound themselves withAbortSignal.timeout(5_000):remoteConfiginweb/services/coderouter/opencodeProxy.tsline 101, and the usage read inweb/services/coderouter/usage.tsline 87.providerReadreplays the call once, so an unresponsive provider holds the request for two unbounded attempts. Create the signal inside the callback so each attempt gets a fresh deadline.Second,
fetchProviderReadrethrows when both attempts throw. Line 142 is not inside atry, so a transport fault on one account rejects the whole handler instead of failing over to the next account.🔧 Proposed fix
- upstream = await dependencies.providerRead(() => fetch(upstreamUrl, { - headers: { - authorization: `Bearer ${credential.accessToken}`, - "chatgpt-account-id": credential.accountId, - originator: "codex_cli_rs", - "user-agent": request.headers.get("user-agent") ?? "coderouter", - }, - cache: "no-store", - })); + try { + upstream = await dependencies.providerRead(() => fetch(upstreamUrl, { + headers: { + authorization: `Bearer ${credential.accessToken}`, + "chatgpt-account-id": credential.accountId, + originator: "codex_cli_rs", + "user-agent": request.headers.get("user-agent") ?? "coderouter", + }, + cache: "no-store", + signal: AbortSignal.timeout(5_000), + })); + } catch (error) { + reportCoderouterFailure("provider_usage", error, { provider: "codex" }); + upstream = null; + continue; + }🤖 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 `@web/services/coderouter/codexProxy.ts` around lines 142 - 150, Update the provider-read flow around fetchProviderRead so the fetch callback creates a fresh AbortSignal.timeout(5_000) for each providerRead attempt and passes it to fetch. Also catch a rejected fetchProviderRead call at the surrounding account-processing boundary, treating transport failures as account-level failures so the handler continues failing over to the next account instead of rejecting.
129-138: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBoth account-selection loops swallow infrastructure errors as routing skips. Neither loop separates a routing-skip condition (
CodeRouterRefreshBusy,CodeRouterCredentialBroken) from an infrastructure fault such as a KMS outage, an RDS failure, or a decryption error. Each loop advances to the next account, the caller returnsno_usable_account(503), and no alert is emitted.proxyCodexRequestatweb/services/coderouter/codexProxy.tslines 49-56 already classifies the same error correctly, so the entrypoints disagree. The shared root cause is the absence of one skip predicate.
web/services/coderouter/codexProxy.ts#L129-L138: replace the barecatchwith the tag check used byproxyCodexRequest, and rethrow anything that is not a routing-skip tag.web/services/coderouter/opencodeProxy.ts#L90-L92: replace the barecatchinopenCodeAccountwith the same tag check, and report or rethrow anything else.Extract the predicate into one helper in the CodeRouter service directory and call it from all three loops.
As per coding guidelines: "When behavior has multiple entrypoints, implement one shared action path and verify every entrypoint rather than duplicating logic."
🤖 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 `@web/services/coderouter/codexProxy.ts` around lines 129 - 138, The account-selection loops incorrectly swallow infrastructure failures as routing skips. Add one shared CodeRouter helper that identifies only CodeRouterRefreshBusy and CodeRouterCredentialBroken as skippable, then use it in proxyCodexRequest, the codex account-selection loop at web/services/coderouter/codexProxy.ts lines 129-138, and openCodeAccount at web/services/coderouter/opencodeProxy.ts lines 90-92; continue only for those tags and report or rethrow all other errors.Source: Coding guidelines
🤖 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 `@web/tests/coderouter-encryption.test.ts`:
- Around line 98-120: Add a rejecting-provider test alongside the existing
coalescingCredentialsProvider coverage: make the first exchange reject, assert
the initial provider call rejects, then invoke it again and assert it succeeds
with a second exchange and the call counter equals two, verifying pending is
cleared for retries.
In `@web/tests/coderouter-opencode-proxy.test.ts`:
- Around line 45-46: Update the unavailable-account fixture’s credential
callback in the test to throw the tagged CodeRouterRefreshBusy error instead of
a plain Error, preserving the existing “refreshing” scenario and matching the
production skip predicate.
In `@web/tests/coderouter-provider-fetch.test.ts`:
- Around line 26-36: Extend the fetchProviderRead test coverage with one case
where every response is 503, asserting the returned status is 503 and the
provider callback is called exactly twice. Add another case where every provider
attempt throws, asserting fetchProviderRead rejects while preserving the retry
limit behavior.
---
Duplicate comments:
In `@web/services/coderouter/codexProxy.ts`:
- Around line 142-150: Update the provider-read flow around fetchProviderRead so
the fetch callback creates a fresh AbortSignal.timeout(5_000) for each
providerRead attempt and passes it to fetch. Also catch a rejected
fetchProviderRead call at the surrounding account-processing boundary, treating
transport failures as account-level failures so the handler continues failing
over to the next account instead of rejecting.
- Around line 129-138: The account-selection loops incorrectly swallow
infrastructure failures as routing skips. Add one shared CodeRouter helper that
identifies only CodeRouterRefreshBusy and CodeRouterCredentialBroken as
skippable, then use it in proxyCodexRequest, the codex account-selection loop at
web/services/coderouter/codexProxy.ts lines 129-138, and openCodeAccount at
web/services/coderouter/opencodeProxy.ts lines 90-92; continue only for those
tags and report or rethrow all other errors.
In `@web/services/coderouter/opencodeProxy.ts`:
- Around line 198-200: Move the local isRecord definition from opencodeProxy.ts
into a shared CodeRouter helper module, then remove the duplicate definitions
from opencodeProxy.ts, refresh.ts, accounts.ts, usage.ts, vault.ts, and
encryption.ts and import the shared type guard wherever used. Preserve its
current Record<string, unknown> narrowing and runtime checks.
In `@web/services/coderouter/providerFetch.ts`:
- Around line 7-12: Update the retry branch in the request flow around the
initial response to cancel or otherwise dispose of first.body before issuing the
second request when the status is retryable. Preserve returning the retry
response, and leave the catch-path retry behavior unchanged.
In `@web/services/coderouter/refresh.ts`:
- Around line 107-118: The catch surrounding the lease workflow currently
reports every failure as a provider refresh failure. In the refresh flow,
isolate dependencies.refresh in its own try/catch so only provider refresh
errors call reportCoderouterFailure with the provider context; change the outer
catch to report the rds category for persistence failures while keeping the
existing dependencies.fail lease-failure recording unchanged.
In `@web/tests/coderouter-models-proxy.test.ts`:
- Around line 22-27: Update the fake select implementation in the proxy test to
honor its excludedAccountIds argument by skipping already-attempted account IDs
before returning an account. Ensure the test verifies createCodexModelsProxy
passes attempted IDs through select, matching the contract covered by the
OpenCode proxy test.
In `@web/tests/coderouter-refresh.test.ts`:
- Around line 113-125: Add a test alongside the existing
createCredentialRefresher tests where read first returns an expired credential
and then a valid credential after the lease claim. Assert the post-claim re-read
path invokes release, does not invoke the provider refresh operation, and
returns the existing valid token.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f826a6f8-12ec-42e7-85b7-87cdeb2350d7
📒 Files selected for processing (19)
web/app/api/coderouter/accounts/[accountId]/route.tsweb/app/api/coderouter/accounts/route.tsweb/app/coderouter/page.tsxweb/db/schema.tsweb/services/coderouter/accounts.tsweb/services/coderouter/codexProxy.tsweb/services/coderouter/encryption.tsweb/services/coderouter/observability.tsweb/services/coderouter/opencodeProxy.tsweb/services/coderouter/providerFetch.tsweb/services/coderouter/refresh.tsweb/services/coderouter/repository.tsweb/services/coderouter/usage.tsweb/tests/coderouter-account-removal.test.tsweb/tests/coderouter-encryption.test.tsweb/tests/coderouter-models-proxy.test.tsweb/tests/coderouter-opencode-proxy.test.tsweb/tests/coderouter-provider-fetch.test.tsweb/tests/coderouter-refresh.test.ts
| describe("coderouter Vercel OIDC credentials", () => { | ||
| test("coalesces parallel AWS role exchanges and refreshes before expiry", async () => { | ||
| let calls = 0; | ||
| let now = 1_000; | ||
| const provider = coalescingCredentialsProvider(async () => { | ||
| calls++; | ||
| await Promise.resolve(); | ||
| return { | ||
| accessKeyId: `access-${calls}`, | ||
| secretAccessKey: "secret", | ||
| expiration: new Date(now + 10 * 60 * 1_000), | ||
| }; | ||
| }, () => now); | ||
| const first = await Promise.all([provider(), provider(), provider()]); | ||
| expect(calls).toBe(1); | ||
| expect(first.map((value) => value.accessKeyId)).toEqual([ | ||
| "access-1", | ||
| "access-1", | ||
| "access-1", | ||
| ]); | ||
| now += 6 * 60 * 1_000; | ||
| expect((await provider()).accessKeyId).toBe("access-2"); | ||
| expect(calls).toBe(2); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover the rejecting-provider branch of coalescingCredentialsProvider.
The suite covers the success path only. If provider() rejects, the finally block must clear pending so the next call retries. A regression there would wedge every KMS request for the process lifetime. Add a case where the first exchange rejects, assert the call rejects, then assert the next call succeeds and performs a second exchange.
🧪 Proposed test
+ test("clears the pending exchange after a failure", async () => {
+ let calls = 0;
+ const now = 1_000;
+ const provider = coalescingCredentialsProvider(async () => {
+ calls++;
+ if (calls === 1) throw new Error("role exchange rejected");
+ return {
+ accessKeyId: "access-2",
+ secretAccessKey: "secret",
+ expiration: new Date(now + 10 * 60 * 1_000),
+ };
+ }, () => now);
+ await expect(provider()).rejects.toThrow("role exchange rejected");
+ expect((await provider()).accessKeyId).toBe("access-2");
+ expect(calls).toBe(2);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("coderouter Vercel OIDC credentials", () => { | |
| test("coalesces parallel AWS role exchanges and refreshes before expiry", async () => { | |
| let calls = 0; | |
| let now = 1_000; | |
| const provider = coalescingCredentialsProvider(async () => { | |
| calls++; | |
| await Promise.resolve(); | |
| return { | |
| accessKeyId: `access-${calls}`, | |
| secretAccessKey: "secret", | |
| expiration: new Date(now + 10 * 60 * 1_000), | |
| }; | |
| }, () => now); | |
| const first = await Promise.all([provider(), provider(), provider()]); | |
| expect(calls).toBe(1); | |
| expect(first.map((value) => value.accessKeyId)).toEqual([ | |
| "access-1", | |
| "access-1", | |
| "access-1", | |
| ]); | |
| now += 6 * 60 * 1_000; | |
| expect((await provider()).accessKeyId).toBe("access-2"); | |
| expect(calls).toBe(2); | |
| describe("coderouter Vercel OIDC credentials", () => { | |
| test("coalesces parallel AWS role exchanges and refreshes before expiry", async () => { | |
| let calls = 0; | |
| let now = 1_000; | |
| const provider = coalescingCredentialsProvider(async () => { | |
| calls++; | |
| await Promise.resolve(); | |
| return { | |
| accessKeyId: `access-${calls}`, | |
| secretAccessKey: "secret", | |
| expiration: new Date(now + 10 * 60 * 1_000), | |
| }; | |
| }, () => now); | |
| const first = await Promise.all([provider(), provider(), provider()]); | |
| expect(calls).toBe(1); | |
| expect(first.map((value) => value.accessKeyId)).toEqual([ | |
| "access-1", | |
| "access-1", | |
| "access-1", | |
| ]); | |
| now += 6 * 60 * 1_000; | |
| expect((await provider()).accessKeyId).toBe("access-2"); | |
| expect(calls).toBe(2); | |
| }); | |
| test("clears the pending exchange after a failure", async () => { | |
| let calls = 0; | |
| const now = 1_000; | |
| const provider = coalescingCredentialsProvider(async () => { | |
| calls++; | |
| if (calls === 1) throw new Error("role exchange rejected"); | |
| return { | |
| accessKeyId: "access-2", | |
| secretAccessKey: "secret", | |
| expiration: new Date(now + 10 * 60 * 1_000), | |
| }; | |
| }, () => now); | |
| await expect(provider()).rejects.toThrow("role exchange rejected"); | |
| expect((await provider()).accessKeyId).toBe("access-2"); | |
| expect(calls).toBe(2); | |
| }); | |
| }); |
🤖 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 `@web/tests/coderouter-encryption.test.ts` around lines 98 - 120, Add a
rejecting-provider test alongside the existing coalescingCredentialsProvider
coverage: make the first exchange reject, assert the initial provider call
rejects, then invoke it again and assert it succeeds with a second exchange and
the call counter equals two, verifying pending is cleared for retries.
| credential: async ({ accountId }) => { | ||
| if (accountId === "busy") throw new Error("refreshing"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Throw a tagged error from the unavailable-account fixture.
Line 46 throws a plain Error. The production catch in openCodeAccount currently swallows every error, so this passes today. If the skip predicate proposed for web/services/coderouter/opencodeProxy.ts lines 90-92 lands, an untagged error must no longer be skipped, and this test will fail for the wrong reason. Throw a CodeRouterRefreshBusy error here so the fixture states the intended routing-skip condition.
🔧 Proposed change
- if (accountId === "busy") throw new Error("refreshing");
+ if (accountId === "busy") {
+ throw new CodeRouterRefreshBusy("credential refresh already in progress");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| credential: async ({ accountId }) => { | |
| if (accountId === "busy") throw new Error("refreshing"); | |
| credential: async ({ accountId }) => { | |
| if (accountId === "busy") { | |
| throw new CodeRouterRefreshBusy("credential refresh already in progress"); | |
| } |
🤖 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 `@web/tests/coderouter-opencode-proxy.test.ts` around lines 45 - 46, Update the
unavailable-account fixture’s credential callback in the test to throw the
tagged CodeRouterRefreshBusy error instead of a plain Error, preserving the
existing “refreshing” scenario and matching the production skip predicate.
| test("does not retry terminal responses or rate limits", async () => { | ||
| for (const status of [400, 401, 403, 404, 429]) { | ||
| let calls = 0; | ||
| const response = await fetchProviderRead(async () => { | ||
| calls++; | ||
| return new Response(null, { status }); | ||
| }); | ||
| expect(response.status).toBe(status); | ||
| expect(calls).toBe(1); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the bound on retries.
The suite proves one retry happens. It does not prove the retry stops. Add a case where every attempt returns 503 and assert calls is 2 and the returned status is 503. Add a case where every attempt throws and assert the call rejects. Both behaviors are contracts the proxy loops depend on.
🧪 Proposed tests
+ test("stops after one retry when the read stays transient", async () => {
+ let calls = 0;
+ const response = await fetchProviderRead(async () => {
+ calls++;
+ return new Response(null, { status: 503 });
+ });
+ expect(response.status).toBe(503);
+ expect(calls).toBe(2);
+ });
+
+ test("rejects when both attempts fail", async () => {
+ let calls = 0;
+ await expect(fetchProviderRead(async () => {
+ calls++;
+ throw new Error("network unavailable");
+ })).rejects.toThrow("network unavailable");
+ expect(calls).toBe(2);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("does not retry terminal responses or rate limits", async () => { | |
| for (const status of [400, 401, 403, 404, 429]) { | |
| let calls = 0; | |
| const response = await fetchProviderRead(async () => { | |
| calls++; | |
| return new Response(null, { status }); | |
| }); | |
| expect(response.status).toBe(status); | |
| expect(calls).toBe(1); | |
| } | |
| }); | |
| test("does not retry terminal responses or rate limits", async () => { | |
| for (const status of [400, 401, 403, 404, 429]) { | |
| let calls = 0; | |
| const response = await fetchProviderRead(async () => { | |
| calls++; | |
| return new Response(null, { status }); | |
| }); | |
| expect(response.status).toBe(status); | |
| expect(calls).toBe(1); | |
| } | |
| }); | |
| test("stops after one retry when the read stays transient", async () => { | |
| let calls = 0; | |
| const response = await fetchProviderRead(async () => { | |
| calls++; | |
| return new Response(null, { status: 503 }); | |
| }); | |
| expect(response.status).toBe(503); | |
| expect(calls).toBe(2); | |
| }); | |
| test("rejects when both attempts fail", async () => { | |
| let calls = 0; | |
| await expect(fetchProviderRead(async () => { | |
| calls++; | |
| throw new Error("network unavailable"); | |
| })).rejects.toThrow("network unavailable"); | |
| expect(calls).toBe(2); | |
| }); |
🤖 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 `@web/tests/coderouter-provider-fetch.test.ts` around lines 26 - 36, Extend the
fetchProviderRead test coverage with one case where every response is 503,
asserting the returned status is 503 and the provider callback is called exactly
twice. Add another case where every provider attempt throws, asserting
fetchProviderRead rejects while preserving the retry limit behavior.
e61c2fe to
db6cc10
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
♻️ Duplicate comments (9)
web/tests/coderouter-refresh.test.ts (1)
113-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the post-claim re-read that releases the lease.
createCredentialRefresherre-reads the credential after it claims the lease. If that second read shows a still-valid token, it callsdependencies.releaseand returns the existing credential without contacting the provider. That branch is the core fix for the double-refresh race, and no test exercises it.Add a test where
readreturns an expired credential first and a valid credential second. Assert thatreleaseruns, thatrefreshnever runs, and that the returned token is the existing one.🧪 Proposed test for the release branch
+ test("releases the lease when another refresh already rotated the token", async () => { + let released = false; + let refreshCalls = 0; + const reads = [ + { envelope, credential: expired }, + { envelope, credential: { ...expired, expiresAt: Date.now() + 3_600_000 } }, + ]; + const refresh = createCredentialRefresher(fakeDependencies({ + read: async () => reads.shift() ?? { envelope, credential: expired }, + release: async () => { + released = true; + }, + refresh: async () => { + refreshCalls++; + return refreshed(); + }, + })); + const result = await refresh(input()); + expect(result.accessToken).toBe("old-access"); + expect(released).toBe(true); + expect(refreshCalls).toBe(0); + });🤖 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 `@web/tests/coderouter-refresh.test.ts` around lines 113 - 125, Add coverage in the createCredentialRefresher tests for the post-claim re-read branch: configure read to return an expired credential first and a valid credential second, then assert release is called, the provider refresh is not called, and the existing valid token is returned.web/tests/coderouter-provider-fetch.test.ts (1)
26-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the bound on retries.
The suite proves one retry happens. It does not prove the retry stops. Add a case where every attempt returns 503 and assert
callsis 2 and the returned status is 503. Add a case where every attempt throws and assert the call rejects. The proxy loops depend on both behaviors.🧪 Proposed tests
+ test("stops after one retry when the read stays transient", async () => { + let calls = 0; + const response = await fetchProviderRead(async () => { + calls++; + return new Response(null, { status: 503 }); + }); + expect(response.status).toBe(503); + expect(calls).toBe(2); + }); + + test("rejects when both attempts fail", async () => { + let calls = 0; + await expect(fetchProviderRead(async () => { + calls++; + throw new Error("network unavailable"); + })).rejects.toThrow("network unavailable"); + expect(calls).toBe(2); + });🤖 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 `@web/tests/coderouter-provider-fetch.test.ts` around lines 26 - 36, Add tests alongside the existing fetchProviderRead retry tests to verify retry termination: use an always-503 response callback and assert it is called twice with a final 503 response, then use an always-throwing callback and assert fetchProviderRead rejects after the bounded attempts.web/services/coderouter/opencodeProxy.ts (1)
198-200: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winShare one
isRecorddefinition across CodeRouter files.
isRecordis defined locally inweb/services/coderouter/opencodeProxy.ts,web/services/coderouter/refresh.ts,web/services/coderouter/accounts.ts,web/services/coderouter/usage.ts,web/services/coderouter/vault.ts, andweb/services/coderouter/encryption.ts. This change narrows one copy toRecord<string, unknown>whilevault.tsstill returnsRecord<string, any>. Move one definition into a shared CodeRouter helper module and import it in all six files.🤖 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 `@web/services/coderouter/opencodeProxy.ts` around lines 198 - 200, Move the local isRecord definition from opencodeProxy.ts into a shared CodeRouter helper module, using the consistent Record<string, unknown> type guard. Remove the duplicate definitions from opencodeProxy.ts, refresh.ts, accounts.ts, usage.ts, vault.ts, and encryption.ts, and import the shared helper in each file; update vault.ts to use the shared typing.web/services/coderouter/providerFetch.ts (1)
7-12: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease the discarded response body before you retry.
Line 9 drops the first
Responsewhen the status is retryable. The body stream is never read or cancelled. The connection and buffered bytes stay allocated until garbage collection. This helper runs on the Codex models, OpenCode config, and usage paths, so a 5xx burst from a provider accumulates unreleased streams.🔧 Proposed fix to release the abandoned body
try { const first = await request(); - return RETRYABLE_STATUS.has(first.status) ? await request() : first; + if (!RETRYABLE_STATUS.has(first.status)) return first; + await first.body?.cancel().catch(() => undefined); + return await request(); } catch { return await request(); }🤖 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 `@web/services/coderouter/providerFetch.ts` around lines 7 - 12, Update the retry logic around the first response in the provider fetch helper: when first.status is retryable, consume or cancel first.body before issuing the second request, while preserving the existing return behavior for non-retryable responses and the catch-path retry.web/tests/coderouter-models-proxy.test.ts (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the proxy excludes accounts it already attempted.
A previous thread marked this as addressed, but the fake
selectat line 22 still ignores its third argument. ProductionselectAccountForRequestusesexcludedAccountIdsto skip accounts already tried in the same request.If
createCodexModelsProxystopped passingattempted, this test would still pass, because the fake advances its own queue on every call. In production the same unusable account would be reselected until the 8-attempt bound and the request would returnno_usable_account. The OpenCode test asserts this contract atweb/tests/coderouter-opencode-proxy.test.tsline 58.🧪 Proposed change to record and assert the excluded account IDs
let selectedAccounts = ["account-1"]; let unusableAccounts = new Set<string>(); +let excludedAccounts: string[] = [];- select: async () => { + select: async (_teamId, _provider, excluded) => { + excludedAccounts = [...(excluded ?? [])]; const id = selectedAccounts.shift(); return id ? { id, vaultRevision: 1, credentialExpiresAt: new Date() } : null; },beforeEach(() => { selectedAccounts = ["account-1"]; unusableAccounts = new Set(); + excludedAccounts = []; });expect(response.status).toBe(200); + expect(excludedAccounts).toContain("refreshing-account"); expect(await response.json()).toEqual({ models: [{ slug: "gpt-test" }], });🤖 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 `@web/tests/coderouter-models-proxy.test.ts` around lines 22 - 27, Update the fake select implementation in the proxy test to accept and record its third argument, excludedAccountIds, and assert that attempted account IDs are passed to createCodexModelsProxy as exclusions on subsequent selections. Ensure the test would fail if the proxy stopped forwarding attempted IDs, while preserving the existing account-selection behavior.web/services/coderouter/codexProxy.ts (2)
129-138: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnclassified credential errors turn infrastructure outages into silent 503s. Both account-selection loops catch every error from the credential dependency and continue to the next account. Neither distinguishes a routing-skip condition (
CodeRouterRefreshBusy,CodeRouterCredentialBroken) from an infrastructure fault such as a KMS outage, an RDS failure, or a decryption error. The caller receivesno_usable_account(503) after the attempt bound, andreportCoderouterFailurenever runs, so the outage produces no alert.proxyCodexRequestatweb/services/coderouter/codexProxy.tslines 49-56 already classifies the error correctly, so the paths disagree. The shared root cause is one missing skip predicate.
web/services/coderouter/codexProxy.ts#L129-L138: replace the barecatchwith the tag check used byproxyCodexRequest, and rethrow anything that is not a routing-skip tag.web/services/coderouter/opencodeProxy.ts#L90-L92: replace the barecatchinopenCodeAccountwith the same tag check, and report or rethrow anything else instead of skipping silently.web/tests/coderouter-opencode-proxy.test.ts#L45-L46: throwCodeRouterRefreshBusyinstead of a plainError, so the fixture states the intended routing-skip condition once the predicate lands.Extract the skip predicate into one shared helper in the CodeRouter service directory, then call it from all three loops.
As per coding guidelines: "When behavior has multiple entrypoints, implement one shared action path and verify every entrypoint rather than duplicating logic."
🤖 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 `@web/services/coderouter/codexProxy.ts` around lines 129 - 138, Unclassified credential errors are being silently skipped instead of propagating infrastructure failures. Add one shared skip-predicate helper in the CodeRouter service directory for CodeRouterRefreshBusy and CodeRouterCredentialBroken, use it in the credential catch blocks in web/services/coderouter/codexProxy.ts#L129-L138 and web/services/coderouter/opencodeProxy.ts#L90-L92, and rethrow non-skippable errors so proxyCodexRequest/reportCoderouterFailure can handle them; update web/tests/coderouter-opencode-proxy.test.ts#L45-L46 to throw CodeRouterRefreshBusy rather than a plain Error, with no other direct change required there.Source: Coding guidelines
142-150: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the models read and handle a rejected provider read.
A previous thread marked this as fixed, but the current code still shows both defects.
First, this
fetchsets noAbortSignal. Both sibling provider reads bound themselves:remoteConfiginweb/services/coderouter/opencodeProxy.tsat line 101 and the usage read inweb/services/coderouter/usage.tsuseAbortSignal.timeout(5_000).dependencies.providerReadreplays the call once, so an unresponsive provider holds the request thread for two unbounded attempts. Create the signal inside the callback so each attempt receives a fresh deadline.Second,
fetchProviderReadrethrows when both attempts throw. Line 142 is not inside atry, so a transport fault on one account rejects the whole handler instead of failing over to the next account. The loop already tolerates a rate-limited account.🔧 Proposed fix to bound the read and continue on transport failure
- upstream = await dependencies.providerRead(() => fetch(upstreamUrl, { - headers: { - authorization: `Bearer ${credential.accessToken}`, - "chatgpt-account-id": credential.accountId, - originator: "codex_cli_rs", - "user-agent": request.headers.get("user-agent") ?? "coderouter", - }, - cache: "no-store", - })); + try { + upstream = await dependencies.providerRead(() => fetch(upstreamUrl, { + headers: { + authorization: `Bearer ${credential.accessToken}`, + "chatgpt-account-id": credential.accountId, + originator: "codex_cli_rs", + "user-agent": request.headers.get("user-agent") ?? "coderouter", + }, + cache: "no-store", + signal: AbortSignal.timeout(5_000), + })); + } catch (error) { + reportCoderouterFailure("provider_usage", error, { provider: "codex" }); + upstream = null; + continue; + }🤖 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 `@web/services/coderouter/codexProxy.ts` around lines 142 - 150, Update the provider read around fetchProviderRead so the fetch callback creates and passes a fresh AbortSignal.timeout(5_000) for each retry attempt. Also catch a rejected providerRead call at the surrounding account-processing flow and continue to the next account on transport failure, preserving the existing rate-limit failover behavior.web/services/coderouter/refresh.ts (1)
107-118: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReport persistence failures under the
rdscategory.The
catchat line 107 still covers the whole lease block.dependencies.read,dependencies.release,dependencies.encrypt, anddependencies.completeall reportprovider_refreshwith aprovidertag. A lost CAS race incompleteRefreshLeaseor an RDS outage therefore pages on-call for the upstream provider.One additional case shares the same root cause: line 118 discards a rejection from
dependencies.fail. If the lease release fails, the account stays inrefreshinguntil the lease expires and no alert fires. Report that rejection underrdstoo.🔧 Proposed fix to separate the provider failure from the persistence failure
- const refreshed = await dependencies.refresh(current.credential); + let refreshed; + try { + refreshed = await dependencies.refresh(current.credential); + } catch (error) { + reportCoderouterFailure("provider_refresh", error, { + provider: currentProvider(before.credential), + terminal: dependencies.isTerminal(error), + }); + throw error; + } const encrypted = await dependencies.encrypt({} catch (error) { const terminal = dependencies.isTerminal(error); - reportCoderouterFailure("provider_refresh", error, { - provider: currentProvider(before.credential), - terminal, - }); await dependencies.fail( input.accountId, leaseId, terminal, dependencies.failureCode(error), - ).catch(() => undefined); + ).catch((leaseError: unknown) => { + reportCoderouterFailure("rds", leaseError, { operation: "fail_lease" }); + });🤖 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 `@web/services/coderouter/refresh.ts` around lines 107 - 118, Update the refresh error handling around the lease block so provider-operation failures remain reported as provider_refresh, while failures from dependencies.read, dependencies.release, dependencies.encrypt, and dependencies.complete are caught and reported under rds without a provider tag. In the dependencies.fail rejection handler, stop silently discarding the error and report the rejection under rds as well, preserving the existing lease-failure flow.web/tests/coderouter-encryption.test.ts (1)
98-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the rejecting-provider branch of
coalescingCredentialsProvider.The suite covers the success path only. If
provider()rejects, thefinallyblock must clearpendingso the next call retries. A regression there wedges every KMS request for the process lifetime. Add a case where the first exchange rejects, then assert the next call succeeds and performs a second exchange.🧪 Proposed test
+ test("clears the pending exchange after a failure", async () => { + let calls = 0; + const now = 1_000; + const provider = coalescingCredentialsProvider(async () => { + calls++; + if (calls === 1) throw new Error("role exchange rejected"); + return { + accessKeyId: "access-2", + secretAccessKey: "secret", + expiration: new Date(now + 10 * 60 * 1_000), + }; + }, () => now); + await expect(provider()).rejects.toThrow("role exchange rejected"); + expect((await provider()).accessKeyId).toBe("access-2"); + expect(calls).toBe(2); + });🤖 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 `@web/tests/coderouter-encryption.test.ts` around lines 98 - 120, Add a test case in the “coderouter Vercel OIDC credentials” suite covering a rejected exchange from coalescingCredentialsProvider: make the first provider invocation reject, assert that rejection, then make the next invocation succeed and verify it performs a second exchange, confirming pending is cleared for retries.
🤖 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.
Duplicate comments:
In `@web/services/coderouter/codexProxy.ts`:
- Around line 129-138: Unclassified credential errors are being silently skipped
instead of propagating infrastructure failures. Add one shared skip-predicate
helper in the CodeRouter service directory for CodeRouterRefreshBusy and
CodeRouterCredentialBroken, use it in the credential catch blocks in
web/services/coderouter/codexProxy.ts#L129-L138 and
web/services/coderouter/opencodeProxy.ts#L90-L92, and rethrow non-skippable
errors so proxyCodexRequest/reportCoderouterFailure can handle them; update
web/tests/coderouter-opencode-proxy.test.ts#L45-L46 to throw
CodeRouterRefreshBusy rather than a plain Error, with no other direct change
required there.
- Around line 142-150: Update the provider read around fetchProviderRead so the
fetch callback creates and passes a fresh AbortSignal.timeout(5_000) for each
retry attempt. Also catch a rejected providerRead call at the surrounding
account-processing flow and continue to the next account on transport failure,
preserving the existing rate-limit failover behavior.
In `@web/services/coderouter/opencodeProxy.ts`:
- Around line 198-200: Move the local isRecord definition from opencodeProxy.ts
into a shared CodeRouter helper module, using the consistent Record<string,
unknown> type guard. Remove the duplicate definitions from opencodeProxy.ts,
refresh.ts, accounts.ts, usage.ts, vault.ts, and encryption.ts, and import the
shared helper in each file; update vault.ts to use the shared typing.
In `@web/services/coderouter/providerFetch.ts`:
- Around line 7-12: Update the retry logic around the first response in the
provider fetch helper: when first.status is retryable, consume or cancel
first.body before issuing the second request, while preserving the existing
return behavior for non-retryable responses and the catch-path retry.
In `@web/services/coderouter/refresh.ts`:
- Around line 107-118: Update the refresh error handling around the lease block
so provider-operation failures remain reported as provider_refresh, while
failures from dependencies.read, dependencies.release, dependencies.encrypt, and
dependencies.complete are caught and reported under rds without a provider tag.
In the dependencies.fail rejection handler, stop silently discarding the error
and report the rejection under rds as well, preserving the existing
lease-failure flow.
In `@web/tests/coderouter-encryption.test.ts`:
- Around line 98-120: Add a test case in the “coderouter Vercel OIDC
credentials” suite covering a rejected exchange from
coalescingCredentialsProvider: make the first provider invocation reject, assert
that rejection, then make the next invocation succeed and verify it performs a
second exchange, confirming pending is cleared for retries.
In `@web/tests/coderouter-models-proxy.test.ts`:
- Around line 22-27: Update the fake select implementation in the proxy test to
accept and record its third argument, excludedAccountIds, and assert that
attempted account IDs are passed to createCodexModelsProxy as exclusions on
subsequent selections. Ensure the test would fail if the proxy stopped
forwarding attempted IDs, while preserving the existing account-selection
behavior.
In `@web/tests/coderouter-provider-fetch.test.ts`:
- Around line 26-36: Add tests alongside the existing fetchProviderRead retry
tests to verify retry termination: use an always-503 response callback and
assert it is called twice with a final 503 response, then use an always-throwing
callback and assert fetchProviderRead rejects after the bounded attempts.
In `@web/tests/coderouter-refresh.test.ts`:
- Around line 113-125: Add coverage in the createCredentialRefresher tests for
the post-claim re-read branch: configure read to return an expired credential
first and a valid credential second, then assert release is called, the provider
refresh is not called, and the existing valid token is returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 566e9f19-9505-4b19-8751-a1bae663a583
📒 Files selected for processing (19)
web/app/api/coderouter/accounts/[accountId]/route.tsweb/app/api/coderouter/accounts/route.tsweb/app/coderouter/page.tsxweb/db/schema.tsweb/services/coderouter/accounts.tsweb/services/coderouter/codexProxy.tsweb/services/coderouter/encryption.tsweb/services/coderouter/observability.tsweb/services/coderouter/opencodeProxy.tsweb/services/coderouter/providerFetch.tsweb/services/coderouter/refresh.tsweb/services/coderouter/repository.tsweb/services/coderouter/usage.tsweb/tests/coderouter-account-removal.test.tsweb/tests/coderouter-encryption.test.tsweb/tests/coderouter-models-proxy.test.tsweb/tests/coderouter-opencode-proxy.test.tsweb/tests/coderouter-provider-fetch.test.tsweb/tests/coderouter-refresh.test.ts
1 participant
Adds manager-scoped account removal with encrypted-credential cascade and final-account route-token revocation, transient-safe provider reads, multi-account failure routing, refresh/KMS/RDS race tests, scrubbed alert signals, usage freshness metadata, Server-Timing, and accurate landing-page support states. Also includes the production backend needed by experimental
cr pi.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Improves CodeRouter private beta reliability with safe, retried provider reads, multi-account failover, and secure manager-scoped deletion that cascades encrypted credentials and revokes route tokens when a team loses its last account. Adds usage freshness fields with
Server-Timingmirrored in headers, rate-limit telemetry, and updates the landing page to reflect private beta and experimentalcr pi.New Features
api/coderouter/accounts/[accountId](manage scope, team-scoped) with encrypted-credential cascade and final route-token revocation; returnslegacyCleanupPendingwhen rollback cleanup fails.usageAsOf,usageAgeSeconds,cacheMaxAgeSeconds, andServer-Timing; mirrors timing inx-coderouter-server-timing. Landing page clarifies: Codex and OpenCode Go available, Pi experimental, Claude Max and API-key routing coming soon.Bug Fixes
fetchProviderReadfor one safe retry of read-only provider calls; used in Codex models, usage reads, and OpenCode config; reports rate limits and applies cooldowns; routes around refreshing/broken accounts.reportCoderouterFailure; coalesces Vercel AWS role exchanges for KMS.Written for commit db6cc10. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation