Uh oh!
There was an error while loading. Please reload this page.
feat(providers): add OpenAI OAuth multi-account routing - #2904
Conversation
4c3ae16 to
9326db9Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Codex automated review
I reviewed exact head 9326db987dbe9221fdd8c55a702777c506de42e3 with independent adversarial passes across storage/catalog/Vault, routing and health, OAuth execution/usage, Desktop UI, config transfer, and the end-to-end fixtures. I found four concrete production issues; see the inline findings. This head currently reports no CI checks.
The 19,383-line / 79-file change is not one reviewable owner. It combines at least five independently testable and mergeable slices, and the cross-slice failures below show why that matters. Please split and sequence it as:
- Core + Storage profile/catalog/Vault contracts, schema, and migration;
- routing store/router and API-key runtime dispatch;
- OAuth profile enrollment, refresh, usage, and dispatch;
- Desktop profile UI and explicit activation controls;
- config transfer plus final cross-slice E2E/docs.
The line count is not itself a P-level finding, and I found no generated/vendor bulk to exclude. The goal is to make each authority independently green and reviewable, then add the final integration tests after its dependencies land.
Disclosure: This is an automated review performed by Codex using delegated adversarial review passes and a final evidence check. It has not been independently verified by Astro-Han or another human reviewer, does not constitute human approval, and does not represent the final judgment of a human reviewer.
| @@ -413,17 +418,23 @@ class OAuthCredentialSupersededError extends Error { | |||
| } | |||
| function requireOAuthLocator(locator: CredentialLocator): OAuthCredentialLocator { | |||
There was a problem hiding this comment.
[P1] Persist refreshes for secondary OAuth profiles
The new OAuth authority accepts a connection_profile locator and later passes it into compareAndSetOAuthCredential, but the storage coordinator still rejects every locator whose scope is not connection. A secondary OpenAI account therefore cannot persist a rotated token after expiry or 401 replay; refresh ends as invalid_credential_input/persistence failure and the otherwise healthy account becomes unusable. Please extend the CAS validation/commit path to connection_profile with its profile revision, and add expired-token and forced-401 refresh regressions for a secondary profile.
There was a problem hiding this comment.
Addressed in ee706e6. compareAndSetOAuthCredential now accepts a live connection_profile OAuth locator in the same serialized coordinator lane; the locator is revalidated against the current Catalog and the credential-generation CAS prevents a stale refresh from writing. Added regressions for an expired secondary profile and for a secondary Codex profile that receives 401, refreshes, persists, and replays successfully.
| ) { | ||
| continue; | ||
| } | ||
| // A configured account is eligible without a synthetic model probe. |
There was a problem hiding this comment.
[P1] Do not dispatch configured-but-unverified accounts
This explicitly makes every configured account eligible without reading the verification store. A newly configured account/model can therefore receive a real request before any successful test/discovery evidence, despite the routing contract that unknown access is never optimistically scheduled (and despite the activation path now writing verification records). Gate both balanced activation and every per-model lease on current credential/basis verification, and add no-evidence activation and dispatch tests.
There was a problem hiding this comment.
Addressed in ee706e6. Balanced activation now requires current supported verification for every enabled profile/model/basis, and per-attempt eligibility independently checks credential id/revision, execution-basis digest, model, and supported status. Configured-but-unverified profiles are therefore excluded from dispatch. Updated the E2E journey to prove no-evidence activation is rejected before verification.
| }); | ||
| const router = new ProviderCredentialRouter( | ||
| { | ||
| getRouting: async () => input.routing, |
There was a problem hiding this comment.
[P2] Re-read routing state before every lease
The resolver closure captures input.routing from backend creation. Profile disable/removal, priority changes, or switching back to legacy_primary only invalidate idle backends, so an active Turn can keep leasing a now-disabled profile or continue balanced routing after the user used the kill switch. Read the current catalog/routing revision for each acquire attempt (or terminate/invalidate the active routing scope on mutation), and cover a mid-Turn disable/mode-switch retry.
There was a problem hiding this comment.
Addressed in ee706e6. The resolver no longer captures routing metadata for lease decisions: it re-reads the current Catalog before routing, eligibility/probing, claim, and material resolution. A disabled or removed profile cannot be resolved after a state change, and an active turn observes a switch back to legacy_primary. Added a mid-turn disable and mode-switch retry regression.
| ); | ||
| } | ||
| const binding = this.#selectBinding(context, routing, candidates, probeReason); | ||
| const material = await this.#provider.resolveCredential( |
There was a problem hiding this comment.
[P1] Recover from credential-resolution failure before creating a lease
The selected OAuth profile is resolved before a lease exists, and an expired/revoked refresh can throw here. That exception bypasses settle/failure accounting and candidate reselection, so one bad account aborts the Turn even when another balanced profile is healthy; if this was a claimed half-open probe, its circuit can remain stuck half_open because settleProbeAborted is never called. Catch resolution failure as a structured profile failure, release/abort any probe claim, exclude that candidate, and continue failover; add refresh-failure → healthy-secondary and half-open cleanup regressions.
There was a problem hiding this comment.
Addressed in ee706e6. OAuth resolution errors are normalized as credential-scoped router failures before a model lease is exposed. The router settles health (including the exact claimed half-open circuit), discards the stale binding, and reselects a healthy verified profile. Added fallback and half-open cleanup regressions.
jischeng
commented
Aug 13, 2026
Follow-up pushed in ee706e6. All four inline correctness findings are addressed with focused regression coverage; the affected Storage and Runtime Host test suites, builds, and typechecks are green.\n\nOn scope: the current branch already preserves the proposed dependency order in separate commits. I agree the next iteration should be split into stacked review units; doing that after these correctness repairs would make the intermediate branches reviewable without carrying an unsafe variant of the end-to-end flow. |
…og, Vault and Host protocol (PR 1) Implement the first slice of the provider credential profiles plan (docs/provider-credential-profiles-load-balancing.zh-CN.md): - Core: ConnectionCredentialProfileEntry / ConnectionCredentialRouting types on ConnectionCatalogEntry (not on drafts or generic updates), primary profile identity (profileId === connectionId), dedicated profile mutation inputs with profile-level version basis, and a connection_profile CredentialLocator scope (api_key | oauth_token). - Codecs: connection_profile locator decode (fail closed on kind / profileId === connectionId), credentialRouting decode with bounded arrays and case-insensitive label uniqueness, strict label (1-64) and weight (1-100) validation, and normalizers for all five profile mutations. - Catalog document: v1 -> v2 lazy migration (v1 read as implicit primary, persisted as v2 only on the first profile mutation), profile CRUD with CAS on connection + profile revisions, primary materialization (revision 1, enabled, weight 1), capacity limit (32), label conflict, and balanced activation structural gates. - Vault document: v1 -> v2 lazy migration, connection_profile locator support, and connection removal / orphan cleanup covering both connection and connection_profile scopes. - Coordinator: fail-closed profile lifecycle (create disabled -> configure -> test -> enable -> balanced), balanced activation checks combining Catalog structure with Vault configuration (every enabled profile configured, at least two enabled), and removal that disables, deletes the vault secret, then removes metadata. - Host protocol: five new operations (credential.profile.create/update/ set-enabled/remove/set-routing-mode) with wire codecs and projections that never leak secrets or full snapshots. Tests: codec round-trips, v1->v2 lazy migration, fail-closed lifecycle (create disabled, capacity, label conflict, primary not removable, balanced activation gating), profile removal vault cleanup, connection removal cleanup across both locator scopes, and protocol round-trips. No runtime routing or failover is wired in this slice.
… authority (PR 2) Second slice of the provider credential profiles plan. No real Provider dispatch is wired yet; this is the pure selection + health authority. Core (packages/core/src/provider-credential-routing.ts): - Cross-boundary routing contracts: route context, turn binding, per-attempt credential lease, outcome, resolver, selection reasons, ProviderFailureKind (mirroring Runtime ModelFailureKind values), ProviderFailureRoutingHint (kind/scope/retryAt/evidence), circuit states, readiness projection, verification record and pool-exhausted diagnostic. Storage: - sqlite-provider-routing-schema.ts: provider_credential_verification and provider_credential_health tables registered as a new provider_routing scope in runtime.sqlite (version 1), wired into operational-state-store migration and operational-state-backup validation. - provider-credential-routing-store.ts: versioned execution-basis digest helper (provider-execution-basis-v1 + canonical JSON + SHA-256, never a secret hash), verification upsert / authoritative basis replacement, health settle with no-op clean success, circuit transitions (auth -> invalid, rate/usage/billing -> open with bounded cadence, network/timeout/connection/unknown -> no profile health change), single half-open probe admission, per-connection/profile deletion, and cleanup of stale profiles and over-aged closed rows. Runtime Host (packages/runtime-host/src/server/provider-credential-router.ts): - ProviderCredentialRouter implementing the core resolver contract: classic SWRR selection (per-round accumulation, first-max deterministic tie-break, re-normalized when eligible set/weights change), turn-granular sticky bindings with LRU backstop, legacy single / legacy_primary fast path (never fabricates failover), per-attempt lease with fresh credential resolution (stale bindings never reuse old secrets), account_failover exclusion, half-open probe targeting, binding_reselect without failover credit, and fail-closed pool exhausted behavior. Tests: SWRR distribution (2:1, 5:2:1, no long runs), deterministic tie break, turn stickiness and release, failover/reselect/probe semantics, credential replacement invalidation, pool exhaustion, aborts, and store verification/health/circuit/cleanup/delete semantics. 14 router + 13 store tests.
…tch path (PR 3) Third slice of the provider credential profiles plan. Delivers the parts of the API-key Runtime integration that do not depend on the PR 0 error classification blocker (Gate 0): per-send dynamic credential materialization, ModelCallAttempt v2 attribution, and turn binding lifecycle. Account failover inside the retry loop stays gated on the routing-hint contract and remains inert (fail closed) until PR 0 lands. Core — ModelCallAttempt v2 (Gate 2): - MODEL_CALL_ATTEMPT_SCHEMA_VERSION_V1/V2 with v2 as the canonical version; decode accepts both, rejects malformed v2 attribution (empty id / unknown reason), and never fabricates a profile id for v1. - New optional credentialProfileId + credentialSelectionReason fields; never a secret and never a mutable label. Runtime: - ModelAdapter.resolveModel(apiKeyOverride?): per-attempt materialization with the leased Profile's key; absent override keeps the construct-time key byte-for-byte identical (legacy fast path). - AiSdkBackend: optional credentialRouting input (resolver + connection identity). When present, send() acquires a per-turn attempt lease, materializes the model with the leased key, passes the attribution into the accounting tracker, and settles/releases the lease in cleanup. When absent, no code path changes. - ProviderRequestTracker: ModelCallAccountingInput carries the Profile attribution and emits it into every canonical ModelCallAttempt. Runtime Host: - ResolvedExecutionTarget now carries connectionId + credentialRouting. - createHostCredentialResolver composes the PR 2 Router with Catalog (profile metadata), Vault (exportCredentialMaterial per profile locator), and the routing Health/Verification store: eligibility = enabled + configured + health-closed + model-support verification (unknown is never optimistically scheduled, RFC 11.1). - createHostAiSdkBackend wires the resolver only for balanced API-key connections; OAuth and legacy_primary/missing routing stay on the legacy fixed-key path. Tests: ModelCallAttempt v1/v2 codec round-trips and malformed-attribution rejection; AiSdkBackend seam test proves acquire -> lease-key materialization -> v2 attribution -> settle/release, plus the absent-routing fast path. 2 new runtime tests; full suites green (core 812, runtime-host 829; runtime/storage failures are the pre-existing --ignore-scripts environment ones, verified unchanged by stash).
Addresses all eight review findings. One independent correctness-repair
commit before any Desktop (PR 4) work.
P1-1 — verification gate + production writer:
- New coordinator operation recordCredentialProfileVerification: CAS-bound
against connection/profile revision, keyed to the current credential
identity/revision + execution basis digest; this is the production
verification writer (Profile test/discovery completion will call it).
- setCredentialRoutingMode('balanced') now requires current supported
verification evidence for every enabled model and at least one model
with two or more verified profiles. Activation with credentials but no
verification is rejected, so 'balanced' can never be configured into a
guaranteed pool-exhausted first dispatch.
- Host e2e test: configure two profiles -> verify -> activate balanced ->
real resolver acquires a lease with a real profile secret and settles.
- The digest the coordinator computes now matches the resolver's (request
headers credential identity/revision included on both sides).
P1-2 — connection_profile credential authority:
- validateConnectionCredentialLocator now covers the connection_profile
scope: connection exists, profile belongs to the connection, the primary
identity cannot be smuggled through the profile scope, and the kind must
match the provider auth contract. Applied to set/delete/status/export.
- clearCredentialDependentLastTests also invalidates on profile credential
changes. Tests for ghost profile, ghost connection, primary-as-secondary,
and wrong-kind writes.
P1-3 — honest settlement:
- Each physical attempt settles with its actual outcome: success only on a
completed stream, aborted on cancellation, failure otherwise. Failures
carry a conservative unknown-scope routing hint (Gate 0), so health is
never changed by unclassified failures and never records a false success.
- Test: a failed attempt settles 'failure', the recovered retry settles
'success'.
P1-4 — per-physical-attempt lease lifecycle:
- acquire/materialize/settle moved into the physical provider request
boundary: every retry and every fresh step re-acquires a lease, re-resolves
the newest credential, re-materializes the model, and updates per-attempt
ModelCallAttempt attribution. A credential replaced or Profile disabled
between attempts is never reused. Test asserts two leases on a retry.
P1-5 — model-scoped health isolation:
- ProviderCredentialLease carries modelId; eligibility only considers the
credential-global row and the current model row; settlement keys the
model-scoped row. e2e test proves a credential_model deny on gpt-5 pool-
exhausts gpt-5 while gpt-4o still dispatches on the same profiles.
P1-6 — half-open probe admission:
- Router acquires half-open probes: when normal candidates are exhausted it
atomically claims one probe per circuit (claimHalfOpenProbe) and
dispatches it with selectionReason half_open_probe. A failed probe re-opens
the circuit with a fresh bounded cadence (store fix) so it can never be
stuck half_open. Router + store tests cover single-flight admission and
re-open.
P2-7 — binding identity:
- Every lease reuses the exact turn binding's bindingId instead of minting a
fresh one. Test asserts same-turn attempts share the id and a new turn gets
a distinct one.
P2-8 — routing store lifecycle:
- ProviderCredentialRoutingStore holds its operational database lease and
exposes dispose(); the composed resolver returns dispose(); the backend's
credentialRouting input threads it into AiSdkBackend.dispose(), so each
per-backend store releases its lease. The coordinator's store is one per
lease (bounded), consistent with other operational authorities.
Tests: +7 targeted tests across storage/runtime/runtime-host; full suites
green (core 812, runtime-host 834; storage/runtime failures are the
pre-existing --ignore-scripts environment ones, unchanged).…t, auxiliary routing, dispose gaps Addresses the five findings from the second review round. P1 — credential-scoped health no longer leaks into the model row: - settleRoutingOutcome now derives the health row from the failure scope: credential failures write the credential-global row (model_id=''), and credential_model failures write only the current model row. A normal model success never closes a global billing/usage circuit. - Half-open probe leases carry healthCircuitModelId (the exact circuit row they claimed — '' global or a model id), so probe success/failure/abort settle back onto that same row. probeEligibleProfiles returns profileId -> circuitModelId and uses the injected clock, matching the claim's now. P1 — cancelled probes can no longer stay stuck half_open: - New store settleProbeAborted: half_open -> open with a conservative probe cadence. The resolver's aborted branch re-opens a claimed probe circuit instead of leaving it blocked forever (user stop / backend dispose / cancellation). Tests cover re-open and re-probe. P1 — auxiliary model calls now route through the Profile Router: - buildLlmHistorySummarizer accepts an acquireCredential lifecycle (resolveModel(apiKey?), settle with the real outcome, always release). The Host history summarizer wires the composed resolver with a synthetic aux:history turn binding. - AiSdkCompactionCapabilities carries credentialRouting; the semantic-compact standalone summarizer model acquires a per-call lease, materializes with the leased key, and settles/releases in a finally — no more fixed construct-time key. Mid-turn e2e proves acquire -> settle success -> release. P2 — dispose gaps closed: - RuntimePolicyCoordinator.dispose() releases the lazily-created routing store lease; the writer facade exposes dispose() and the host composition calls it during shutdown. - createHostAiSdkBackend disposes the composed resolver in both failure catches (model-composition build and backend construction), so an early failure cannot leak the operational database lease. Tests: +9 across storage (probe abort), runtime-host (credential-scope global row, probe claims global circuit row and recovers), runtime (history summarizer lease success/failure/abort, mid-turn auxiliary lease e2e). Full suites green: core 812, runtime-host 836; storage/runtime failures are the pre-existing --ignore-scripts environment ones (verified unchanged). Note on verification: balanced activation is now safely closed (no evidence = no activation), and recordCredentialProfileVerification is the production writer, but no Profile test/discovery effect calls it yet — that producer wiring lands with PR 4 (Profile test effects), so this repair closes the safety hole without claiming a full end-to-end product loop.
…e, honest summarizer settlement, early dispose Addresses the five findings from the third review round. P1 — semantic-compact auxiliary lease now matches the real dispatch: - The acquire/materialize/settle lifecycle moved INSIDE the summarizer callback, so projection early-returns (below-min-step, no compressible span, head-anchor mismatch, validation failure) never acquire and never settle a lease — a half-open probe can no longer be closed by a request that never happened. - acquireAuxiliaryLease now takes the model that will actually dispatch (summarizerModelId), not the main call's model, so eligibility/ verification/health are checked for the summarizer's model when a separate summarizer model is configured. - Even without a separate summarizerModel, the summarizer model is re-materialized with the leased key instead of reusing the main call's already-materialized model/key (RFC 9.1). - Abort settles as 'aborted' (not 'failure'); provider errors as 'failure'; success only after a completed dispatch. P1 — auxiliary ModelCallAttempt carries the full lease attribution: - The summarizer callback calls tracker.setCredentialAttribution(lease) before dispatch, so canonical semantic_compact records carry credentialProfileId + credentialSelectionReason (v2 only surfaced apiKey/settle/release and skipped attribution). P1/P2 — history summarizer settles exactly once: - The lease outcome is decided first (output-length = failure, provider error = failure, abort = aborted, else success), then settled once in the finally, with release guaranteed after settle. The previous code settled success before checking finishReason and then settled failure again on output_length (success -> failure double-settle). P2 — pre-ownership resolver dispose: - createHostAiSdkBackend now wraps the pricing/policy snapshot reads in a try/catch that disposes the resolver, and the OAuth-resolve catch also disposes it. Combined with the already-correct model-composition and backend-constructor catches, no failure between resolver creation and backend ownership can leak the operational DB lease. Tests: +4 — semantic compaction end-to-end lease (acquire -> re-materialize with leased key -> attributed ModelCallAttempt -> settle success -> release), no-dispatch never settles, history output-length single-settle, plus fixture wiring so modelFactory captures keys and records attempts. Full suites green: core 812, runtime-host 836; runtime/storage failures are the pre-existing --ignore-scripts environment ones (verified unchanged).
…tory attribution, unified ownership guard Addresses the three findings from the fourth review round. P1 — semantic compact materialization failure no longer leaks the lease: - Tracker attribution, model materialization and the actual dispatch now all live INSIDE the lease's protection frame. A synchronous modelFactory throw (bad provider/model config) settles a claimed half-open probe conservatively (as failure, never as success) and always releases the synthetic turn binding, so the probe can never stay claimed and the binding never waits for the LRU/TTL backstop. - Regression test: acquire succeeds, modelFactory throws with the leased key; the lease settles as failure and releases. P1 — history compact now carries full Profile attribution (Gate 2): - acquireCredential's contract exposes profileId + selectionReason from the real lease; the Host history summarizer passes them through. - buildLlmHistorySummarizer calls providerRequestTracker.setCredentialAttribution before dispatch, so the canonical history_compact ModelCallAttempt carries credentialProfileId + credentialSelectionReason — matching the semantic summarizer's closed loop. - Regression test asserts both fields on the canonical record. P2 — unified ownership guard instead of scattered catches: - createHostAiSdkBackend now wraps everything between resolver creation and HostAiSdkBackend ownership in ONE guard. Any failure — including the previously unprotected createFetchTransport sync throw, provider-options building and snapshotForSession — releases the transport (if created), the client-capability lease and the resolver. Inner step catches are reduced to plain rethrows so nothing is released twice. On the success path HostAiSdkBackend.dispose() owns those releases. - Regression tests: a late snapshotForSession throw closes the already-created transport; a synchronous transport-factory throw rejects without hanging. Tests: +4 (semantic materialization failure, history attribution, two ownership-guard cases). Full suites green: core 812, runtime-host 838; runtime/storage failures are the pre-existing --ignore-scripts environment ones (verified unchanged). Four packages typecheck with 0 errors.
…ecise ownership-guard tests Addresses the two P2 findings from the fifth review round. No routing logic changes. P2 — history attribution contract is now fail-closed and atomic: - acquireCredential's contract replaced the two independently-optional profileId/selectionReason fields with a single atomic structure. selectionReason is narrowed to the formal union (no bare string), and the summarizer only calls setCredentialAttribution when the whole structure is present — a half-set attribution is now a compile-time impossibility instead of an OR-guarded runtime risk. - The Host history summarizer and the mid-turn fixture helper emit the same atomic shape from the real lease. P2 — ownership-guard tests no longer leak and now verify resolver dispose: - Removed the leaked, un-awaited from the first test (it created a backend whose transport/capability/resolver were abandoned). - Test A: a late capability-snapshot throw closes the already-created transport exactly once (no capability was acquired). - Test B: snapshotForSession succeeds (a capability lease IS held) then a later step throws — capability released exactly once and transport closed exactly once. - Test C: a resolver factory (new HostAiSdkBackendInput test seam, defaulting to the production buildCredentialResolver) succeeds, then createFetchTransport throws synchronously — resolver.dispose called exactly once, proving the guard releases the resolver's owned lease on the transport-factory failure path. Full suites green: core 812, runtime-host 839; runtime/storage failures are the pre-existing --ignore-scripts environment ones (verified unchanged). Four packages typecheck with 0 errors.
…ss query Add the Profile-scoped connection effects and their protocol contracts: - begin/completeConnectionProfileTest and ProfileModelFetch tickets pin the connection revision, profile enabled/config revision, credential identity/revision, endpoint and model basis; stale completions supersede. - Profile test success writes verification through the production authority (recordCredentialProfileVerification, positive-only, tested evidence); failed tests never write evidence. - Profile discovery records the enabled-model intersection (positive-only upsert or authoritative basis replacement) before CAS-merging catalog metadata; a single Profile's missing models never drive inventory deletion. - credential.profile.query projects Catalog+Vault+routing readiness (labels, credentials, last test, supported models, circuits, ready candidates) without exposing secrets. - Host coordinators dispatch connection.profile.test.run / models.fetch and the readiness query; protocol codecs stay fail-closed.
…ness Connection detail sheet gains an Accounts / API Keys section: - Host client and connections IPC expose profile create/update/enable/remove, routing mode, readiness query, profile test and profile model fetch. - Readiness projection (label, enabled, weight, configured/unverified/ready/ cooldown/invalid/needs_reauth, last test, supported models, ready candidate count, routing mode) never carries secret material. - API keys are written only through the per-profile replace form at save time; removal confirms against the label only; the primary profile cannot be removed; new profiles start disabled; adding a secondary profile never changes routing mode; balanced activation stays an explicit toggle.
- Config bundles move to schema v2: connection payloads carry non-secret Profile metadata with export-local profileRefs, credentials reference connectionSlug + profileRef + kind, and request headers ride the primary ref. v1 bundles keep the legacy single-credential import path. - v2 import follows the RFC order: connections, profiles (disabled, legacy_primary), credentials, validation, re-test of previously-enabled profiles, enable only the verified, restore balanced only when gates hold. Overwrite updates a profile in place only on an exact profile-ID match; label conflicts are reported and never receive secrets. - Desktop E2E journey: create secondary -> set credential -> enable -> profile test writes verification -> explicit balanced activation -> UI shows ready accounts and balanced routing, plus primary-immutability and label-only removal confirmation. - Fix catalog wire contract: credentialRouting must not ride connection header items (the readiness query is its only surface) — the protocol decoder now fails closed on a smuggled declaration.
…sis and evidence revocation Addresses the PR4 review findings: - P1-1: a configured-but-disabled Profile can now be tested/discovered (RFC 11.1 verify-first lifecycle). The ticket pins the ACTUAL enabled state, and completion supersedes when it changed mid-flight. Config v2 import re-tests every bundle-enabled profile including the primary, so a clean target can satisfy the balanced gate. - P1-2: parseConfigBundle preserves the validated SOURCE schemaVersion, so real v1 files stay on the legacy import path instead of being rewritten into the v2 importer. - P1-3: v2 overwrite import quiesces first — drops to legacy_primary and disables enabled secondaries before any credential is replaced, so a partial failure never leaves "balanced declared + credentials replaced". - P1-4: readiness matches verification evidence against the current execution basis digest per enabled model, surfaces the credential-global circuit row, and never counts half-open circuits as ready candidates. - P1-5: authoritative discovery replaces every current enabled-model digest group (not just the newly supported ones), so dropped models lose evidence across apiProtocol digest groups and an empty intersection clears the set. - P2-1: profile test completion is bound to the ticket model — explicit model ids must match exactly and default-model completions must stay in the frozen canonical basis. - P2-2: the desktop profile editor commits weight-only edits instead of bailing out when the label is unchanged. New coverage: forged/mismatched completions, cross-digest and empty authoritative revocation, digest-aware readiness, v1 parser round-trip, overwrite quiesce ordering, the clean-target import journey against the real coordinator, and the E2E lifecycle order (test disabled -> enable -> balanced).
… readiness and import preservation - P1-1: ordinary eligibility now treats half_open as blocked, so a claimed probe is single-flight: a second acquire can never dispatch the same profile as a normal candidate while its probe is unsettled. Regression test claims one circuit's probe and asserts the next acquire takes the other circuit's probe instead of an ordinary dispatch. - P1-2: readiness computes ready candidates PER MODEL (credential-global row + that model's row); a model-scoped deny no longer removes a profile from other models' ready sets. The aggregate circuit stays for display. Regression test denies both profiles on gpt-5 only and asserts gpt-4o keeps two ready candidates with readyCandidateCount 1. - P2-1: lastTest is projected only from records matching the current credential, enabled model and execution basis digest, so a stale endpoint/headers/overlay result (including needs_reauth) cannot leak into readiness. - P2-2: the overwrite quiesce records every secondary it disabled and the restore pass re-enables the ones the bundle did not list (RFC 13.4: absent profiles are kept) instead of silently leaving them disabled.
…sabled and failed profiles The overwrite quiesce restore re-enabled every profile the bundle had not successfully restored, including ones the bundle explicitly disabled and ones that failed re-verification. The restore now tracks every target profile the bundle CLAIMED (mapped, regardless of enabled state or test outcome) and only re-enables quiesced profiles outside that set: - bundle-disabled profiles stay disabled after import; - re-verification failures stay disabled and are reported, never silently re-enabled around the fail-closed flow; - profiles genuinely absent from the bundle keep their previous enabled state (RFC 13.4). Adds regressions for both branches plus the existing absent-profile restore.
…cy dispatch fail-closed - P1-1: the v2 overwrite import now quiesces the PRIMARY profile too (every enabled profile is disabled before any credential is replaced), and the restore pass enforces the bundle's enabled state for every claimed profile: a bundle-disabled primary stays disabled, a primary that fails re-verification stays disabled and is reported, and a primary is only re-enabled after a verified test. Created connections now apply the bundle's explicit primary-disabled state instead of inheriting the default. - P1-2: a legacy_primary connection with an explicitly disabled primary fails closed at dispatch: resolveExecutionConnection returns the new profile_disabled readiness kind (session catalog maps it to operation_unavailable) and the Router's legacy fast path resolves the primary by identity and rejects a disabled primary instead of dispatching it or silently using a secondary. Implicit legacy connections (no routing declaration) are unchanged. - P2: listed target profile ids are scoped per connection slug, so one connection's bundle listing can never block another connection's absent-profile restore when profile ids collide across connections. Regressions: primary bundle-disable, primary re-verification failure, primary disabled before credential replacement, primary re-enabled only after a verified test, per-slug scoping with colliding profile ids, the Router legacy fast path and the storage execution resolve path.
…target quiesce - P1-1: the overwrite quiesce now covers CREATED connections too. A new connection's implicit primary routing is materialized through the new authority operation and the primary is disabled before any credential write or test; a failed re-verification leaves it disabled and reported, and it is only re-enabled after a verified test. - P1-2: new formal authority operation `credential.profile.materialize- primary` (storage coordinator + catalog, protocol spec + decoders, host handler, client method, transfer dep) materializes the implicit primary into an explicit routing declaration idempotently, so a primary-only v2 bundle imports on a clean target without relying on the create-secondary side effect. Auth-less providers are rejected. - P2: the v2 export projects the REAL primary metadata (label/enabled/ weight) from the routing declaration instead of hardcoding primary/enabled/weight-1, so an explicitly disabled primary round-trips and its metadata is preserved. Regressions: storage materialize idempotency + auth-awareness, primary-only clean-target import call order (materialize -> quiesce -> credential -> test -> enable), clean-target primary re-verification failure, and the v2 export projection (disabled primary, stable profileRef order, implicit legacy omission).
…aterialize not-found - P1: the import quiesce now runs only for PROFILE-AWARE sources (an explicit credentialProfiles declaration or routingMode), so an implicit auth-less connection (e.g. Ollama) never hits materialize (which the authority correctly rejects as auth_not_supported), and materialize runs for both created and overwritten connections (idempotent) so an explicit bundle overwriting an implicit legacy target materializes the primary before quiescing instead of failing on profile_not_found. - P2: materialize-primary returns the domain-level connection_not_found for a missing connection instead of a fabricated stale basis, and the host handler projects it through without an invariant failure. Regressions: explicit primary-only bundle overwriting an implicit legacy target, implicit auth-less clean-target import (asserts the profile lifecycle is skipped entirely), a mixed profile-aware + auth-less bundle, and host-handler integration for the connection_not_found projection and materialize idempotency.
ee706e6 to
07e8386Comparejischeng
commented
Aug 13, 2026
Rebased the repaired series onto the current The four inline correctness findings are covered by the current head: secondary-account OAuth persistence, verified-only eligibility before balanced routing, fresh policy reads during routing, and structured OAuth resolution failures with failover / half-open cleanup. I also corrected the renderer type import to use Core's public Validated on this head:
The Desktop-wide typecheck still stops on unrelated upstream UI contract errors around scheduled tasks and live-content exports; the OAuth renderer import error is no longer present. |
Astro-Han
commented
Aug 13, 2026
Thanks for the follow-up, but it is still too large to review within a single PR size. It combines at least five independently testable and mergeable slices, and the cross-slice failures below show why that matters. Could we split it by slices and I would be happy to review after the split. |
Astro-Han
left a comment
There was a problem hiding this comment.
The profile-routing direction is useful, but this head is not yet safe to merge. It has broad conflicts with current main, and two authority boundaries remain split: routing eligibility is read from a live execution basis while credential resolution/health settlement use the captured basis, and import-overwrite does not reliably clear an omitted endpoint.
The first-principles simplification is one immutable execution-basis lease per dispatch: endpoint, protocol, headers/body overlay, credential ID/revision, routing decision, and health settlement must all use the same version. A basis change invalidates/rebuilds the backend rather than mixing generations. For imports, use an explicit tri-state (undefined preserve, null clear, string replace) and map snapshot omission to clear. Rebase and split/reconcile along the existing Core/Storage, Host routing, OAuth, and Desktop transfer seams before retesting.
Reviewed with Codex using two independent review passes; I verified the cited races and merge conflicts against this head and current main.
中文
Profile routing 方向有价值，但当前 head 还不能安全合并。它与最新 main 有大量冲突，而且 routing eligibility 使用实时 execution basis，credential resolve/health settle 却仍使用旧 basis；导入覆盖时也无法可靠清除被省略的 endpoint。
最小且一致的方案是每次 dispatch 只绑定一个不可变 execution-basis lease：endpoint、protocol、headers/body overlay、credential ID/revision、routing 决策和 health settle 全部使用同一版本；basis 变化就失效并重建 backend。导入使用明确三态（undefined 保留、null 清除、string 替换），snapshot 省略应映射为清除。建议先按现有职责边界 rebase/整合后再测试。
本次由 Codex 进行两轮独立审查，并核对了当前 head 与最新 main。
| { | ||
| getRouting: (connectionId) => readCurrentRouting(input, connectionId), | ||
| getEligibleProfileIds: async (connectionId, profileIds, modelId) => { | ||
| const current = await readCurrentExecutionBasis(input, connectionId, modelId); |
There was a problem hiding this comment.
P1 — Do not mix live eligibility with a captured credential/settlement basis. This reads the current digest and routing, but resolveCredential still uses captured input.connection and settleHealth writes the captured digest. During an active-backend config change, a retry can select a credential authorized for the new basis and dispatch it to the old endpoint, then record health under the wrong digest. Bind eligibility, materialization, dispatch, and settlement to one immutable basis lease; fail closed/rebuild when its revision changes. Add an active-backend endpoint/header/body-overlay change regression.
| connections: [ | ||
| { | ||
| ...conn('deepseek-main'), | ||
| baseUrl: 'https://new-endpoint.example/v1', |
There was a problem hiding this comment.
P1 — Add the missing custom-to-default overwrite case. This covers replacing one explicit baseUrl with another, but the production update omits baseUrl when the imported snapshot uses the provider default. That omission preserves the old custom endpoint and may send the imported credential there. Make overwrite tri-state (undefined preserve, null clear, string replace), map snapshot omission to clear, and assert that a custom URL is removed.
Astro-Han
commented
Aug 18, 2026
/agentic_review |
Code Review by Qodo
1. Routing policy not round-tripped |
| ...projected, | ||
| ...(profiles === undefined ? {} : { credentialProfiles: profiles }), | ||
| ...(routing === undefined ? {} : { routingMode: routing.mode }), |
There was a problem hiding this comment.
1. Routing policy not round-tripped 🐞 Bug≡ Correctness
The v2 connection payload serializes only routing.mode, and import restores only that mode without the routing strategy or ordered profile IDs. An exported load-balanced connection consequently imports with the target/default strategy, while overwrite imports retain the target's prior account order instead of the exported manual order.
Agent Prompt
## Issue description
Configuration v2 drops the credential routing strategy and fails to apply exported profile order, changing load-balancing and manual failover behavior after import.
## Issue Context
Reuse the existing `SetCredentialRoutingModeInput` seam, which already accepts `strategy` and `orderedProfileIds`, rather than creating another routing authority. The bundle needs optional serialized strategy/order fields because no current field carries those values; this adds v2 transfer-surface and compatibility-test burden, but deletion or consolidation cannot preserve the source routing policy.
## Fix Focus Areas
- apps/desktop/src/main/runtime-host-config-ipc-main.ts[159-204]
- apps/desktop/src/main/config-transfer-service.ts[47-49]
- apps/desktop/src/main/config-transfer-service.ts[627-635]
- apps/desktop/src/main/runtime-host-config-ipc-main.ts[251-334]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // PR4 scope is API-key profiles; OAuth profile material is a later PR. | ||
| if (entry.kind !== 'api_key') { | ||
| skipped += 1; | ||
| continue; |
There was a problem hiding this comment.
3. Oauth profile imports lose secrets 🐞 Bug≡ Correctness
V2 export emits secondary OpenAI OAuth credentials as oauth_token, but V2 import skips every secondary credential not marked api_key, recreating multi-account profile metadata without login tokens. Consequently, all imported secondary OAuth accounts are unusable.
Agent Prompt
## Issue description
V2 config export includes secondary OpenAI OAuth credentials as `oauth_token`, but V2 import unconditionally skips secondary credentials whose kind is not `api_key`. As a result, OpenAI OAuth multi-account exports recreate profile metadata without tokens, leaving the secondary accounts unusable after import.
## Issue Context
Reuse the existing profile-scoped credential locator/vault write seam and preserve the exported credential kind rather than hard-coding `api_key`. Derive the profile locator kind from the target connection/provider credential kind; `ExportedCredential.kind` already carries `oauth_token`, so no new transfer format, state, or credential authority is needed. Deletion is insufficient because the V2 export contract promises that secondary secrets survive a round trip, and tests should cover OAuth-token restoration.
## Fix Focus Areas
- apps/desktop/src/main/config-transfer-service.ts[537-548]
- apps/desktop/src/main/runtime-host-config-ipc-main.ts[335-354]
- apps/desktop/src/main/runtime-host-config-ipc-main.ts[526-548]
- apps/desktop/src/main/__tests__/config-transfer-service.test.ts[1-1808]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (!Array.isArray(item.profiles) || item.profiles.length > CONNECTION_CREDENTIAL_PROFILE_MAX) { | ||
| throw domainError('credential routing profiles must be a bounded array'); | ||
| } | ||
| const profiles = item.profiles.map((profile) => decodeConnectionCredentialProfileEntry(profile)); |
There was a problem hiding this comment.
4. Invalid routing reaches dispatch 🐞 Bug≡ Correctness
The canonical routing decoder accepts an empty profile array or a legacy_primary declaration with no profile whose ID is the connection ID. Such catalog data is accepted during decoding but legacy dispatch later throws primary profile is missing, so malformed persisted/imported configuration fails at request time instead of being rejected at the catalog boundary.
Agent Prompt
## Issue description
Canonical credential-routing decoding does not enforce the declared primary-profile invariant. A decoded `legacy_primary` routing declaration can contain zero profiles or omit the primary profile, causing all later legacy requests to fail closed in the router.
## Issue Context
Consolidate this invariant at the canonical catalog decoding seam, where malformed persisted/imported entries are admitted. The decoder needs the enclosing connection ID (or the enclosing canonical-entry decoder must perform the check immediately after decoding) to require exactly one profile with that ID and require a non-empty list. This is a local validation change; no new runtime behavior or state is necessary.
## Fix Focus Areas
- packages/core/src/runtime-policy/connection-catalog-codec.ts[257-277]
- packages/core/src/runtime-policy/connection-catalog-codec.ts[501-581]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| : { | ||
| kind: 'failure', | ||
| failure: { kind: 'unknown', retryable: false }, | ||
| routingHint: { kind: 'unknown', scope: 'unknown', evidence: 'provider_adapter' }, | ||
| }; |
There was a problem hiding this comment.
5. Compaction failures bypass failover 🐞 Bug☼ Reliability
The new semantic-compaction lease settlement maps every non-abort provider exception to unknown, including auth, billing, and rate-limit failures. The selected account is consequently not marked unhealthy and cannot participate in the PR's credential-scoped recovery/failover behavior for compaction requests.
Agent Prompt
## Issue description
Semantic compaction settles all non-abort exceptions as an unknown routing failure. Credential-scoped provider failures must retain their normalized failure kind so account health is updated and later auxiliary requests can route away from an exhausted or invalid account.
## Issue Context
Reuse the model adapter's existing failure normalization seam already used by the main model-call path. Map normalized `auth`, `provider_billing`, and `rate_limit` failures to credential scope; retain unknown scope for other failures. This adds no state or public surface.
## Fix Focus Areas
- packages/runtime/src/ai-sdk-compaction.ts[1158-1172]
- packages/runtime/src/ai-sdk-backend.ts[3303-3322]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Astro-Han
commented
Aug 19, 2026
This PR adds a substantial multi-account management UX. Could you please add screenshots of the provider connection detail showing multiple OAuth accounts and the key controls such as ordering, enable or disable, usage, and load balancing? One annotated composite is fine, with all account details sanitized. Thanks! Posted by Codex on behalf of Astro-Han. |
jischeng
commented
Aug 20, 2026
Thanks for the detailed review and for the screenshot request. Sorry for the slow response, I have been busy recently. I will address the review comments over the next couple of days and add sanitized screenshots of the provider connection detail showing multiple OAuth accounts, account ordering, enable/disable, usage, and load balancing. Thanks! |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — this is a serious piece of work and the core of it is right. Reviewed exact head 84d15bf01b9e862de9217419fa5bba09610bc395; three independent passes, on security and credential handling, on correctness and architecture boundaries, and on Occam and minimal final state.
The headline number is misleading and I want to say so before criticising: of the ~20.8k added lines, 9.2k are tests and 1.5k is a design doc, and only 14 files are genuinely new. The storage layer extends the existing Credential Vault, Connection Catalog and operational-state lease rather than building a parallel credential store, and the OAuth modules are extended in place rather than forked. We found no parallel implementation of an existing seam. Several hard things are done well: every retry and every new step re-acquires the lease and matchingExecutionBasis fails closed when the live basis digest has drifted, so a credential replaced mid-turn is never reused; a profile disabled mid-turn is discarded and reselected without a request going out on it; a refresh failure is converted into a routing error and settled against health, including reopening a claimed half-open circuit; claimHalfOpenProbe is genuinely atomic; and the main turn dispatch path forwards both the leased key and the leased transport correctly. On the security side, the diff adds no logging at all, no secret reaches the renderer, and the readiness projection exposes only a masked account hint.
The defect that matters most is that the auxiliary paths do not forward the leased transport the way the main path does. On an OAuth connection the leased account's identity lives entirely in its fetch, not in its apiKey, so dropping it silently substitutes the primary account's token while the outcome is recorded against the leased one. Both compaction paths do this. Details inline; both were traced hop by hop.
Six P1s and six P2s inline. Four of the P1s are things that are not in the PR body at all — the global retry budget going 10 to 3 for every provider, the config-bundle export version going 1 to 2 unconditionally, and the packaging. Not approving while P1/P2 findings are open. No CI checks are reported at this head, which for a change of this size is the other thing I would want before merge.
A cluster of smaller items I am not filing individually, all confirmed by reading code at this head: RouterPoolExhaustedError's structured diagnostics have no consumer outside tests, so an exhausted pool reaches the user as a raw message; acquireCredentialLease at turn start sits two lines above the try that exists to convert model-resolution failures into a structured error event, so a pool-exhausted turn takes the flow's generic error path instead; CredentialProfileReadiness and CredentialPoolExhausted are exported on the newly published @maka/core/provider-credential-routing subpath with zero references, while the renderer restates an overlapping seven-member union of its own; CreateProviderCredentialRouterOptions is never supplied by any caller; weight means "priority index" on the reorder path and "traffic ratio" on the SWRR path with no converter, currently inert only because both gates happen to be closed; the weight bounds and the 32-profile cap are hardcoded a third time in the Desktop IPC layer; a const result = update.run(...) is never read and Biome will not catch it; and 44 RFC section N citations across 23 production files point at a design doc added in this same PR that nothing else in the repo references by path.
On packaging, see the inline note — the split is not a stylistic preference, it is the reason a reviewer cannot give this the independent human approval the project requires.
Review disclosure: this review was prepared with Claude Code, which ran three parallel adversarial passes over the diff at this head and I checked each surviving finding against the source before keeping it — including re-verifying the two undisclosed constant changes against the merge base myself. Evidence grade is stated per finding; everything below is code reading and reference search, not execution. The human contributor reviewed this before posting.
| signal: new AbortController().signal, | ||
| }); | ||
| return { | ||
| apiKey: lease?.apiKey, |
There was a problem hiding this comment.
[P1] Return the lease's fetch, not just its apiKey — on OAuth the transport is the account. resolveProfileCredential returns OAuth material as { apiKey: access_token, fetch: createHostOAuthModelFetch(...) }, and that fetch is the only thing carrying the account identity: it unconditionally rewrites Authorization: Bearer and re-derives ChatGPT-Account-Id from its own binding. This helper's return object declares only apiKey, attribution, settle and release, so leasedFetch is always undefined, modelFactory falls through to modelInput.fetch ?? modelFetch, and modelFetch is the primary binding built at backend creation. Concretely: a Codex connection with account A primary and exhausted, account B healthy; the session hits the compaction threshold; the router correctly leases B; the request goes out on A's bearer token and A's account header, so A is throttled again and B's quota is untouched — while the history_compactModelCallAttempt records credentialProfileId = B and settle() writes health against B. Attribution, metering and circuit state all point at an account that never served the request. Note the plumbing above this is already correct: buildLlmHistorySummarizer forwards credential?.fetch and the host's resolveModel spreads it when present — only this object withholds it. Fix: ...(lease?.fetch ? { fetch: lease.fetch } : {}), and add the field to the declared return type so the omission becomes a type error. The existing unit test passes precisely because its stub supplies the fetch this composition does not, so the regression test has to be at the composition layer: build a real host backend for a balanced OAuth connection and assert the compaction request carries the leased profile's bearer token.
| auxiliaryLease || policy.summarizerModel | ||
| ? this.input.modelFactory({ | ||
| connection: this.input.connection, | ||
| apiKey: auxiliaryLease?.apiKey ?? this.input.apiKey, |
There was a problem hiding this comment.
[P1] Same substitution on the semantic-compact path, with a worse consequence — it can permanently retire a healthy account. modelFactory({ connection, apiKey: auxiliaryLease?.apiKey ?? this.input.apiKey, modelId }) passes no fetch, so the connection-scoped primary OAuth fetch wins and overwrites the header. Unlike the history path, this one settles failures with scope: 'credential': primary A's token is revoked or rate-limited, the router leases healthy B, the request goes out as A and returns 401, normalizeFailure classifies it kind: 'auth' / scope: 'credential', computeFailureState returns circuitState: 'invalid', and blockedForModel then excludes B for every model until a credential-revision change or a manual retest. A healthy account is removed from rotation because a different account's token was used. On the success path the same substitution silently bills every semantic compaction to the primary. ModelFactory already accepts fetch; pass ...(auxiliaryLease?.fetch ? { fetch: auxiliaryLease.fetch } : {}). Regression test: two profiles with distinguishable tokens, force a semantic compact, assert the outbound Authorization and ChatGPT-Account-Id match the leased profile, and assert an auth failure raised by the primary's token does not mark the leased profile invalid.
| // normal candidates while this one safe half-open request is in flight. | ||
| // Existing turn bindings stay sticky and account-failover retries never | ||
| // detour through an unrelated recovery probe. | ||
| if (newTurn && context.reason === 'initial') { |
There was a problem hiding this comment.
[P1] Do not let a recovery probe take over the user's real request while healthy accounts are available. This runs before the if (normal.length > 0) branch and, on success, returns the probe profile as the sole candidate — so a known-bad account serves the first request of the turn, and failover has nowhere to go. That contradicts the PR body's "while healthy accounts continue serving requests": there is no side channel here, the probe is the user's turn. The failure is concrete and not recoverable by the retry loop: account A is cooled down, B is healthy, a new turn is routed to A, A returns a few tokens of text and then errors — attemptHasNoObservableOutput() is now false, so credentialAccountRecovery is false, the retry gate refuses, and the user gets a truncated response and an error while B was healthy the whole time. Probe only when normal.length === 0, or keep the probe but append the healthy candidates so the balanced binding has a fallback. Regression test: one expired-but-open circuit plus one healthy profile, assert the turn's first lease is the healthy profile. The ordering and the single-candidate return are direct from the code; the interaction with attemptHasNoObservableOutput is inference about stream timing.
| } | ||
| const MAX_PROVIDER_ATTEMPTS_PER_STEP = 10; | ||
| const MAX_PROVIDER_ATTEMPTS_PER_STEP = 3; |
There was a problem hiding this comment.
[P1] Restore the retry budget or give account failover its own — this cuts resilience for every user and every provider, and the PR body does not mention it. MAX_PROVIDER_ATTEMPTS_PER_STEP was 10 at the merge base and is 3 here, and it gates the whole retry loop, not just the account-failover branch — so it applies to a single-key Anthropic or OpenAI connection that never touches this feature. A connection hitting three consecutive provider_unavailable 529s during a provider deploy window used to recover on attempt four through ten and now fails the turn outright. Worse for routed connections: account failover consumes attempts from this same budget, so the transient-error headroom left for a balanced connection is smaller than three. Whatever the reasoning is — and there may be a good one, since failover changes what a retry costs — it is a behaviour change to every existing user that arrives inside a PR titled for multi-account routing. Either restore 10 and give failover a separate counter, or split it out with its own rationale; at minimum it belongs in the PR body and the release notes. I verified 10 against the merge base and searched the body for any mention of retries or attempts; there is none.
| @@ -22,7 +22,14 @@ import type { LlmConnection } from '@maka/core/llm-connections'; | |||
| * Reads fail closed on unknown schema versions (mirrors credential-store). | |||
| */ | |||
| export const CONFIG_TRANSFER_SCHEMA_VERSION = 1; | |||
| export const CONFIG_TRANSFER_SCHEMA_VERSION = 2; | |||
There was a problem hiding this comment.
[P1] Emit v1 when the bundle contains no profile data, so exports stay readable by the release people are actually running. buildConfigBundle writes CONFIG_TRANSFER_SCHEMA_VERSION unconditionally, so every bundle produced by this build is stamped schemaVersion: 2 — and older builds check version !== 1 and return unsupported_version. The reader side here was correctly widened to accept both; the writer side was not conditionalised, which makes this a one-way door for a user who has never created a credential profile. Concretely: a user on this build exports settings and connections with no profiles anywhere, sends the file to a teammate on the previous release, and gets Unsupported config schemaVersion 2 for a bundle whose contents are byte-for-byte expressible in v1. Emit v1 unless some connection actually carries credentialProfiles or a non-default routingMode, and escalate to v2 only then. Regression test: export a profile-free bundle and assert schemaVersion === 1. Like the retry budget above, this is not mentioned in the PR body.
| sawUnusableStepUsage = true; | ||
| const delayMs = providerRetryDelayMs(providerAttempt, failure.retryAfterMs); | ||
| const delayMs = | ||
| scope.credentialRouteReason === 'account_failover' |
There was a problem hiding this comment.
[P2] Reset credentialRouteReason after a successful failover — as written, one account failure disables backoff and turn stickiness for the rest of the turn. Once 'account_failover' is set nothing clears it, so this expression forces delayMs to 0 for every subsequent retry, and the router's reason === 'account_failover' branch always re-runs candidate selection instead of consulting the existing turn binding. The sequence is ordinary: attempt 1 on A returns 429, A is excluded and the reason flips; attempt 2 on B hits a transient network error that has nothing to do with accounts; attempt 3 retries immediately with zero backoff — hammering the provider at exactly the moment backoff exists for — and re-runs SWRR, so a single turn can end up split across accounts mid-conversation. Today the blast radius is bounded only by the reduced attempt cap, which is not a property to rely on. Reset the reason to 'initial' after a successful failover acquire, and gate the zero-delay path on this attempt's failure being an account failure rather than on turn-sticky state.
| if (this.activeTurns.size > 0) await this.stop('user_stop'); | ||
| else this.compaction.abortHistoryCompact(); | ||
| this.modelAdapter.dispose(); | ||
| this.input.credentialRouting?.dispose?.(); |
There was a problem hiding this comment.
[P2] Release the routing lease in a finally. dispose() runs stop(), then modelAdapter.dispose(), then this — with no try/finally — so a throw in either earlier step skips the release entirely. The composition code already reasons about exactly this hazard on the creation side ("any failure before that point must dispose it, or the lease leaks"); the teardown side has no equivalent guard, and the host subclass's own try/finally covers only the transport and client capabilities. Concretely: the backend is disposed while a turn is active and stop('user_stop') rejects because a tool runtime rejects during abort — acquireOperationalStateDatabase's refcount never reaches zero, runtime.sqlite stays open for the life of the process, and the operational-state backup path that requires a closed handle is blocked with no way to recover short of a restart. Wrap the body in try { ... } finally { this.input.credentialRouting?.dispose?.(); } and make the release idempotent.
| snapshot: catalogSnapshot(catalogAfterDisable), | ||
| }); | ||
| } | ||
| return this.catalog.removeCredentialProfile(root, { |
There was a problem hiding this comment.
[P2] Delete a removed profile's routing rows — the garbage collection this PR ships has no callers. ProviderCredentialRoutingStore.cleanup, deleteProfile and deleteConnection are implemented and unit-tested, and a repo-wide search over the PR head finds zero production callers: the only references are their own declarations, implementations and tests. removeCredentialProfile commits the catalog and vault changes — disable, vault delete, catalog remove — and never touches the routing tables, and nothing schedules cleanup(liveProfileKeys, now). Two consequences. provider_credential_verification and provider_credential_health in runtime.sqlite grow monotonically with orphan rows for profiles that no longer exist, for any user who adds and removes accounts. And "remove this account" leaves that account's verification records on disk indefinitely, including the stored test_summary_json — a data-retention behaviour the UI does not set an expectation for and that a user removing an account would reasonably assume does not happen. Call deleteProfile here and deleteConnection on connection removal, and wire cleanup into whatever already schedules operational-state maintenance. Regression test: create, verify and remove a profile, then assert both routing tables are empty for it.
| createRefreshTransport: () => createFetchTransport(refreshProxy), | ||
| }), | ||
| networkProxy: resolved.networkProxy, | ||
| connectionId: resolved.connection.connectionId, |
There was a problem hiding this comment.
[P2] Stop requiring the primary account's credential on the balanced path. Attaching credentialRouting to the OAuth execution target is what enables balanced routing, but the OAuth branch still throws Runtime Host OAuth credential is not configured when the connection-scoped material is missing — before routing is considered at all — and createHostAiSdkBackend then calls oauthBinding.resolve() unconditionally, which performs a real refresh and persist. So a user who disables the primary account and keeps two healthy secondaries still exercises the disabled account's refresh token on every session start, and if that credential is gone or its refresh fails, backend creation throws and the whole connection is unusable despite two eligible profiles. The router has its own deliberate fail-closed rule for a disabled primary; this pre-routing binding sits in front of it and bypasses it. Make the binding lazy, or skip it when credentialRouting.mode === 'balanced' — once the leased transport is forwarded on the auxiliary paths, nothing on the balanced path needs the primary binding at all. The throw and resolve path are confirmed by reading code at this head; that a user can reach the missing-credential state is inference from the existing deleteCredential IPC.
| context.connectionId, | ||
| routing, | ||
| candidates, | ||
| context.providerId === 'openai-codex', |
There was a problem hiding this comment.
[P2] Move the provider name out of the router. equalWeights = context.providerId === 'openai-codex' rewrites every profile's weight to 1 inside the one layer whose stated contract is provider-agnostic purity, and it does so for exactly the provider this PR exists for — so the entire weighted apparatus that ships alongside it (the min/max constants, the codec validation, normalizeSwrr/sameWeightShape, the weight input in the UI, weight in the protocol, IPC and preload contracts) is inert for the only provider that surfaces profiles as accounts. Note the asymmetry it creates: #prioritySelect still honours profile.weight, so the same stored number means "priority" under one strategy and "ignored" under the other. It is not user-visible today, because the UI only selects SWRR when the user turns on load balancing and equal distribution is the plausible intent there — the cost is that the abstraction's own justification is contradicted at its selection core, and the escape hatch is the evidence the generality is not needed yet. Express it as a strategy value or a per-provider capability flag on PROVIDER_DEFAULTS, or ship priority_failover only and defer SWRR to the PR that has a provider needing it.
jischeng
commented
Aug 21, 2026
     |
What changed
Adds multi-account credential profiles for model connections, with an OpenAI OAuth (ChatGPT/Codex) flow that keeps multiple logged-in accounts under one provider connection.
Why
Users need multiple OpenAI OAuth accounts without duplicating the whole provider connection, while retaining predictable ordered failover and clear per-account state.
Issue context
Related to #2677. Manual ordering is the predictable default. Load balancing is explicit opt-in and does not treat an ambiguous HTTP 403 as quota exhaustion. The larger question of automatic routing versus a relay/gateway boundary remains open in #2677.
Validation