Conversation
There was a problem hiding this comment.
Code Review
This pull request modularizes the monolithic auth.ts file into several sub-modules under src/sse/services/auth/ to improve code organization. The review feedback identifies a critical regression where the disableCooling logic (transient connection cooldown opt-out) was omitted during the refactoring. Additionally, the feedback suggests adding optional chaining to prevent potential runtime TypeErrors when accessing settings, and recommends removing a large number of unused imports that were copied over to the new sub-modules.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Read passthroughModels from connection config (user-configured per-model quota) | ||
| const connProviderSpecificData = (conn?.providerSpecificData as Record<string, unknown>) || {}; | ||
| const connectionPassthroughModels = connProviderSpecificData.passthroughModels as | ||
| | boolean | ||
| | undefined; |
There was a problem hiding this comment.
During modularization, the disableCooling logic (originally from the transient connection cooldown opt-out feature #2997) was completely omitted. This is a critical regression that breaks the ability to opt-out of connection-level cooldowns. Please restore the disableCooling flag definition.
| // Read passthroughModels from connection config (user-configured per-model quota) | |
| const connProviderSpecificData = (conn?.providerSpecificData as Record<string, unknown>) || {}; | |
| const connectionPassthroughModels = connProviderSpecificData.passthroughModels as | |
| | boolean | |
| | undefined; | |
| // Read passthroughModels from connection config (user-configured per-model quota) | |
| const connProviderSpecificData = (conn?.providerSpecificData as Record<string, unknown>) || {}; | |
| const connectionPassthroughModels = connProviderSpecificData.passthroughModels as | |
| | boolean | |
| | undefined; | |
| const disableCooling = connProviderSpecificData.disableCooling === true; |
| } else if (cooldownMs > 0) { | ||
| await updateProviderConnection(connectionId, { | ||
| ...baseUpdate, | ||
| rateLimitedUntil: getUnavailableUntil(cooldownMs), | ||
| testStatus: "unavailable", | ||
| }); |
There was a problem hiding this comment.
Use the restored disableCooling flag to prevent setting the connection to unavailable and applying rateLimitedUntil when disableCooling is enabled.
| } else if (cooldownMs > 0) { | |
| await updateProviderConnection(connectionId, { | |
| ...baseUpdate, | |
| rateLimitedUntil: getUnavailableUntil(cooldownMs), | |
| testStatus: "unavailable", | |
| }); | |
| } else if (cooldownMs > 0 && !disableCooling) { | |
| await updateProviderConnection(connectionId, { | |
| ...baseUpdate, | |
| rateLimitedUntil: getUnavailableUntil(cooldownMs), | |
| testStatus: "unavailable", | |
| }); |
| const settings = await getCachedSettings(); | ||
| const autoDisableEnabled = settings.autoDisableBannedAccounts ?? false; |
There was a problem hiding this comment.
To prevent potential runtime TypeError if getCachedSettings() returns null or undefined, use optional chaining when accessing autoDisableBannedAccounts.
| const settings = await getCachedSettings(); | |
| const autoDisableEnabled = settings.autoDisableBannedAccounts ?? false; | |
| const settings = await getCachedSettings(); | |
| const autoDisableEnabled = settings?.autoDisableBannedAccounts ?? false; |
| const settings = await getSettings(); | ||
| const strategy = settings.fallbackStrategy || "fill-first"; | ||
| const sessionAffinityTtlMs = | ||
| provider === "codex" | ||
| ? Number.isFinite(Number(options.sessionAffinityTtlMs)) && | ||
| Number(options.sessionAffinityTtlMs) > 0 | ||
| ? Number(options.sessionAffinityTtlMs) | ||
| : Number.isFinite(Number(settings.codexSessionAffinityTtlMs)) && | ||
| Number(settings.codexSessionAffinityTtlMs) > 0 | ||
| ? Number(settings.codexSessionAffinityTtlMs) | ||
| : 0 | ||
| : 0; |
There was a problem hiding this comment.
To prevent potential runtime TypeError if getSettings() returns null or undefined, use optional chaining when accessing fallbackStrategy and codexSessionAffinityTtlMs.
| const settings = await getSettings(); | |
| const strategy = settings.fallbackStrategy || "fill-first"; | |
| const sessionAffinityTtlMs = | |
| provider === "codex" | |
| ? Number.isFinite(Number(options.sessionAffinityTtlMs)) && | |
| Number(options.sessionAffinityTtlMs) > 0 | |
| ? Number(options.sessionAffinityTtlMs) | |
| : Number.isFinite(Number(settings.codexSessionAffinityTtlMs)) && | |
| Number(settings.codexSessionAffinityTtlMs) > 0 | |
| ? Number(settings.codexSessionAffinityTtlMs) | |
| : 0 | |
| : 0; | |
| const settings = await getSettings(); | |
| const strategy = settings?.fallbackStrategy || "fill-first"; | |
| const sessionAffinityTtlMs = | |
| provider === "codex" | |
| ? Number.isFinite(Number(options.sessionAffinityTtlMs)) && | |
| Number(options.sessionAffinityTtlMs) > 0 | |
| ? Number(options.sessionAffinityTtlMs) | |
| : Number.isFinite(Number(settings?.codexSessionAffinityTtlMs)) && | |
| Number(settings?.codexSessionAffinityTtlMs) > 0 | |
| ? Number(settings?.codexSessionAffinityTtlMs) | |
| : 0 | |
| : 0; |
| import { randomUUID, createHash } from "crypto"; | ||
| import { | ||
| getProviderConnections, | ||
| getProviderNodes, | ||
| validateApiKey, | ||
| updateProviderConnection, | ||
| getSettings, | ||
| getCachedSettings, | ||
| getSessionAccountAffinity, | ||
| upsertSessionAccountAffinity, | ||
| touchSessionAccountAffinity, | ||
| deleteSessionAccountAffinity, | ||
| } from "@/lib/localDb"; | ||
| import { | ||
| DEFAULT_QUOTA_THRESHOLD_PERCENT, | ||
| getQuotaCache, | ||
| getQuotaWindowStatus, | ||
| isAccountQuotaExhausted, | ||
| } from "@/domain/quotaCache"; | ||
| import { | ||
| isAccountUnavailable, | ||
| getUnavailableUntil, | ||
| getEarliestRateLimitedUntil, | ||
| formatRetryAfter, | ||
| checkFallbackError, | ||
| isModelLocked, | ||
| getModelLockoutInfo, | ||
| lockModel, | ||
| hasPerModelQuota, | ||
| getRuntimeProviderProfile, | ||
| recordModelLockoutFailure, | ||
| } from "@omniroute/open-sse/services/accountFallback.ts"; | ||
| import { isLocalProvider } from "@omniroute/open-sse/config/providerRegistry.ts"; | ||
| import { COOLDOWN_MS } from "@omniroute/open-sse/config/constants.ts"; | ||
| import { | ||
| preflightQuota, | ||
| isQuotaPreflightEnabled, | ||
| } from "@omniroute/open-sse/services/quotaPreflight.ts"; | ||
| import { resolveResilienceSettings } from "@/lib/resilience/settings"; | ||
| import { syncHealthFromDB, type KeyHealth } from "@omniroute/open-sse/services/apiKeyRotator.ts"; | ||
| import { | ||
| classifyProviderError, | ||
| PROVIDER_ERROR_TYPES, | ||
| } from "@omniroute/open-sse/services/errorClassifier.ts"; | ||
| import { looksLikeQuotaExhausted } from "@/shared/utils/classify429"; | ||
| import { getCodexModelScope } from "@omniroute/open-sse/executors/codex.ts"; | ||
| import { | ||
| getProviderById, | ||
| getProviderAlias, | ||
| resolveProviderId, | ||
| NOAUTH_PROVIDERS, | ||
| WEB_COOKIE_PROVIDERS, | ||
| } from "@/shared/constants/providers"; | ||
| import { isModelExcludedByConnection } from "@/domain/connectionModelRules"; | ||
| import * as log from "../utils/logger"; | ||
| import { fisherYatesShuffle, getNextFromDeckSync } from "@/shared/utils/shuffleDeck"; | ||
| import crypto from "node:crypto"; |
There was a problem hiding this comment.
| import { randomUUID, createHash } from "crypto"; | ||
| import { | ||
| getProviderConnections, | ||
| getProviderNodes, | ||
| validateApiKey, | ||
| updateProviderConnection, | ||
| getSettings, | ||
| getCachedSettings, | ||
| getSessionAccountAffinity, | ||
| upsertSessionAccountAffinity, | ||
| touchSessionAccountAffinity, | ||
| deleteSessionAccountAffinity, | ||
| } from "@/lib/localDb"; | ||
| import { | ||
| DEFAULT_QUOTA_THRESHOLD_PERCENT, | ||
| getQuotaCache, | ||
| getQuotaWindowStatus, | ||
| isAccountQuotaExhausted, | ||
| } from "@/domain/quotaCache"; | ||
| import { | ||
| isAccountUnavailable, | ||
| getUnavailableUntil, | ||
| getEarliestRateLimitedUntil, | ||
| formatRetryAfter, | ||
| checkFallbackError, | ||
| isModelLocked, | ||
| getModelLockoutInfo, | ||
| lockModel, | ||
| hasPerModelQuota, | ||
| getRuntimeProviderProfile, | ||
| recordModelLockoutFailure, | ||
| } from "@omniroute/open-sse/services/accountFallback.ts"; | ||
| import { isLocalProvider } from "@omniroute/open-sse/config/providerRegistry.ts"; | ||
| import { COOLDOWN_MS } from "@omniroute/open-sse/config/constants.ts"; | ||
| import { | ||
| preflightQuota, | ||
| isQuotaPreflightEnabled, | ||
| } from "@omniroute/open-sse/services/quotaPreflight.ts"; | ||
| import { resolveResilienceSettings } from "@/lib/resilience/settings"; | ||
| import { syncHealthFromDB, type KeyHealth } from "@omniroute/open-sse/services/apiKeyRotator.ts"; | ||
| import { | ||
| classifyProviderError, | ||
| PROVIDER_ERROR_TYPES, | ||
| } from "@omniroute/open-sse/services/errorClassifier.ts"; | ||
| import { looksLikeQuotaExhausted } from "@/shared/utils/classify429"; | ||
| import { getCodexModelScope } from "@omniroute/open-sse/executors/codex.ts"; | ||
| import { | ||
| getProviderById, | ||
| getProviderAlias, | ||
| resolveProviderId, | ||
| NOAUTH_PROVIDERS, | ||
| WEB_COOKIE_PROVIDERS, | ||
| } from "@/shared/constants/providers"; | ||
| import { isModelExcludedByConnection } from "@/domain/connectionModelRules"; | ||
| import * as log from "../utils/logger"; | ||
| import { fisherYatesShuffle, getNextFromDeckSync } from "@/shared/utils/shuffleDeck"; | ||
| import crypto from "node:crypto"; |
There was a problem hiding this comment.
This file contains 57 lines of unused imports. It only needs resolveProviderId from @/shared/constants/providers and CredentialSelectionOptions from ./types.ts. Cleaning this up improves readability and maintainability.
import { resolveProviderId } from "@/shared/constants/providers";
import { CredentialSelectionOptions } from "./types.ts";| import { randomUUID, createHash } from "crypto"; | ||
| import { | ||
| getProviderConnections, | ||
| getProviderNodes, | ||
| validateApiKey, | ||
| updateProviderConnection, | ||
| getSettings, | ||
| getCachedSettings, | ||
| getSessionAccountAffinity, | ||
| upsertSessionAccountAffinity, | ||
| touchSessionAccountAffinity, | ||
| deleteSessionAccountAffinity, | ||
| } from "@/lib/localDb"; | ||
| import { | ||
| DEFAULT_QUOTA_THRESHOLD_PERCENT, | ||
| getQuotaCache, | ||
| getQuotaWindowStatus, | ||
| isAccountQuotaExhausted, | ||
| } from "@/domain/quotaCache"; | ||
| import { | ||
| isAccountUnavailable, | ||
| getUnavailableUntil, | ||
| getEarliestRateLimitedUntil, | ||
| formatRetryAfter, | ||
| checkFallbackError, | ||
| isModelLocked, | ||
| getModelLockoutInfo, | ||
| lockModel, | ||
| hasPerModelQuota, | ||
| getRuntimeProviderProfile, | ||
| recordModelLockoutFailure, | ||
| } from "@omniroute/open-sse/services/accountFallback.ts"; | ||
| import { isLocalProvider } from "@omniroute/open-sse/config/providerRegistry.ts"; | ||
| import { COOLDOWN_MS } from "@omniroute/open-sse/config/constants.ts"; | ||
| import { | ||
| preflightQuota, | ||
| isQuotaPreflightEnabled, | ||
| } from "@omniroute/open-sse/services/quotaPreflight.ts"; | ||
| import { resolveResilienceSettings } from "@/lib/resilience/settings"; | ||
| import { syncHealthFromDB, type KeyHealth } from "@omniroute/open-sse/services/apiKeyRotator.ts"; | ||
| import { | ||
| classifyProviderError, | ||
| PROVIDER_ERROR_TYPES, | ||
| } from "@omniroute/open-sse/services/errorClassifier.ts"; | ||
| import { looksLikeQuotaExhausted } from "@/shared/utils/classify429"; | ||
| import { getCodexModelScope } from "@omniroute/open-sse/executors/codex.ts"; | ||
| import { | ||
| getProviderById, | ||
| getProviderAlias, | ||
| resolveProviderId, | ||
| NOAUTH_PROVIDERS, | ||
| WEB_COOKIE_PROVIDERS, | ||
| } from "@/shared/constants/providers"; | ||
| import { isModelExcludedByConnection } from "@/domain/connectionModelRules"; | ||
| import * as log from "../utils/logger"; | ||
| import { fisherYatesShuffle, getNextFromDeckSync } from "@/shared/utils/shuffleDeck"; | ||
| import crypto from "node:crypto"; |
There was a problem hiding this comment.
This file contains 57 lines of unused imports. It only needs getProviderById, NOAUTH_PROVIDERS, and WEB_COOKIE_PROVIDERS from @/shared/constants/providers. Cleaning this up improves readability and maintainability.
import {
getProviderById,
NOAUTH_PROVIDERS,
WEB_COOKIE_PROVIDERS,
} from "@/shared/constants/providers";| import { randomUUID, createHash } from "crypto"; | ||
| import { | ||
| getProviderConnections, | ||
| getProviderNodes, | ||
| validateApiKey, | ||
| updateProviderConnection, | ||
| getSettings, | ||
| getCachedSettings, | ||
| getSessionAccountAffinity, | ||
| upsertSessionAccountAffinity, | ||
| touchSessionAccountAffinity, | ||
| deleteSessionAccountAffinity, | ||
| } from "@/lib/localDb"; | ||
| import { | ||
| DEFAULT_QUOTA_THRESHOLD_PERCENT, | ||
| getQuotaCache, | ||
| getQuotaWindowStatus, | ||
| isAccountQuotaExhausted, | ||
| } from "@/domain/quotaCache"; | ||
| import { | ||
| isAccountUnavailable, | ||
| getUnavailableUntil, | ||
| getEarliestRateLimitedUntil, | ||
| formatRetryAfter, | ||
| checkFallbackError, | ||
| isModelLocked, | ||
| getModelLockoutInfo, | ||
| lockModel, | ||
| hasPerModelQuota, | ||
| getRuntimeProviderProfile, | ||
| recordModelLockoutFailure, | ||
| } from "@omniroute/open-sse/services/accountFallback.ts"; | ||
| import { isLocalProvider } from "@omniroute/open-sse/config/providerRegistry.ts"; | ||
| import { COOLDOWN_MS } from "@omniroute/open-sse/config/constants.ts"; | ||
| import { | ||
| preflightQuota, | ||
| isQuotaPreflightEnabled, | ||
| } from "@omniroute/open-sse/services/quotaPreflight.ts"; | ||
| import { resolveResilienceSettings } from "@/lib/resilience/settings"; | ||
| import { syncHealthFromDB, type KeyHealth } from "@omniroute/open-sse/services/apiKeyRotator.ts"; | ||
| import { | ||
| classifyProviderError, | ||
| PROVIDER_ERROR_TYPES, | ||
| } from "@omniroute/open-sse/services/errorClassifier.ts"; | ||
| import { looksLikeQuotaExhausted } from "@/shared/utils/classify429"; | ||
| import { getCodexModelScope } from "@omniroute/open-sse/executors/codex.ts"; | ||
| import { | ||
| getProviderById, | ||
| getProviderAlias, | ||
| resolveProviderId, | ||
| NOAUTH_PROVIDERS, | ||
| WEB_COOKIE_PROVIDERS, | ||
| } from "@/shared/constants/providers"; | ||
| import { isModelExcludedByConnection } from "@/domain/connectionModelRules"; | ||
| import * as log from "../utils/logger"; | ||
| import { fisherYatesShuffle, getNextFromDeckSync } from "@/shared/utils/shuffleDeck"; | ||
| import crypto from "node:crypto"; | ||
|
|
||
| import { getProviderCredentialsWithQuotaPreflight } from "./credentials.ts"; |
|
Thanks @oyi77 — two blockers before this auth modularization can land:
|
3a2a046 to
432ec31
Compare
|
Lossless verification completed. All 10 monolith exports found in modularized version:
The modularized version also exports additional internal helper functions (60+ total) that were previously module-private. Additionally fixed:
Requesting re-review. |
- extracted types, utils, sessionAffinity, quotaLimits, connectionStatus, scoring, syntheticFallback, selectionLocks, credentials, accountManagement, and authHeaders to domain files - auth.ts is now a 1-line facade re-exporting modules
…estore lockModel in accountManagement
New no-auth providers: - llm7: Free OpenAI-compatible proxy via api.llm7.io (no key required) New web-cookie providers: - mistral-web: chat.mistral.ai session cookie auth with CSRF token - you-web: you.com session cookie with NDJSON streaming translation - pi-web: pi.ai session cookie auth for Inflection Pi - character-web: character.ai char_token auth via neo API All providers registered in executors/index.ts and providers/index.ts
Critical regression from diegosouzapw#2997: the disableCooling flag (per-connection cooldown opt-out) was omitted during the auth.ts → accountManagement.ts modularization. This broke the ability to opt out of connection-level cooldowns. - Restore disableCooling declaration from connProviderSpecificData - Guard the cooldown branch with !disableCooling so connections with disableCooling=true are not marked unavailable/rateLimited
…iegosouzapw#3953 Adds the no-auth provider blocking check that was merged in diegosouzapw#3953. The guard prevents synthetic fallback when provider is blocked by settings. Also makes maybeSyntheticNoAuthFallback async to support the check. Addresses maintainer review: rebase includes merged fixes.
|
Thanks for all the modularization work here, @oyi77 🙏. We've decided to hold the per-module "non-stacked" refactors and run the decomposition as one coordinated pass after the in-flight quality-gate work lands, instead of merging them piecemeal. Reason: on the two we did merge (#3993, #3988) we caught logic being dropped during the move — and the gates (provider-consistency / typecheck) don't detect internal-logic loss — so each of these needs a full lossless audit, which isn't tractable across many overlapping PRs against a moving release branch right now. The coordinated modularization is tracked in #3501 / #3594; we'd genuinely value your input on that plan once it's up. Closing for now — purely sequencing, not a reflection on the effort. |
…egosouzapw#3987) Split src/sse/services/auth.ts into 13 focused modules: - accountManagement.ts, authHeaders.ts, connectionStatus.ts, constants.ts - credentials.ts, quotaLimits.ts, scoring.ts, selectionLocks.ts - sessionAffinity.ts, syntheticFallback.ts, types.ts, utils.ts - index.ts (barrel) Original file replaced with thin re-export.
Replaces #3791 as an independent, non-stacked PR branched from release/v3.8.27.
Extracts auth services into modular structure.
Supersedes: #3791