fix: restore removed comments in provider-handler, token-refresh-manager, and connect

Author: maros7

src/oclif/commands/providers/connect.ts

@@ -159,6 +159,8 @@ export default class ProviderConnect extends Command {
         throw new Error(`Provider "${providerId}" does not support OAuth. Use --api-key instead.`)
       }
 
+      // --code is only valid for code-paste providers (e.g., Anthropic).
+      // Browser-callback providers like OpenAI handle the code exchange automatically.
       if (code && provider.oauthCallbackMode !== 'code-paste') {
         throw new Error(
           `Provider "${providerId}" uses browser-based OAuth and does not accept --code.\n` +
@@ -185,6 +187,7 @@ export default class ProviderConnect extends Command {
         throw new Error(startResponse.error ?? 'Failed to start OAuth flow')
       }
 
+      // Always print auth URL (user's machine may not support browser launch)
       if (startResponse.callbackMode === 'device') {
         onProgress?.(`\nOpen this URL and enter the code below:`)
         onProgress?.(`  ${startResponse.verificationUri ?? startResponse.authUrl}`)
@@ -194,6 +197,7 @@ export default class ProviderConnect extends Command {
         onProgress?.(`\nOpen this URL to authenticate:\n  ${startResponse.authUrl}\n`)
       }
 
+      // 3. Handle based on callback mode
       if (startResponse.callbackMode === 'auto' || startResponse.callbackMode === 'device') {
         if (startResponse.callbackMode === 'auto') {
           onProgress?.('Waiting for authentication in browser...')
@@ -211,6 +215,7 @@ export default class ProviderConnect extends Command {
         return {providerName: provider.name, showInstructions: false}
       }
 
+      // code-paste mode: print instructions and exit
       onProgress?.('Copy the authorization code from the browser and run:')
       onProgress?.(`  brv providers connect ${providerId} --oauth --code <code>`)
       return {providerName: provider.name, showInstructions: true}

src/server/infra/http/provider-model-fetchers.ts

@@ -545,16 +545,14 @@ export class ChatBasedModelFetcher implements IProviderModelFetcher {
   }
 }
 
-// ============================================================================
-// OpenRouter Model Fetcher (wraps existing client)
-// ============================================================================
-
-import {getOpenRouterApiClient, type NormalizedModel} from './openrouter-api-client.js'
-
 // ============================================================================
 // Copilot Model Fetcher
 // ============================================================================
 
+/**
+ * Model fetcher for GitHub Copilot.
+ * Queries the Copilot API for available models using the session token.
+ */
 export class CopilotModelFetcher implements IProviderModelFetcher {
   private cache: ModelCache | undefined
   private readonly cacheTtlMs: number
@@ -636,6 +634,13 @@ export class CopilotModelFetcher implements IProviderModelFetcher {
 // ============================================================================
 // OpenRouter Model Fetcher (wraps existing client)
 // ============================================================================
+
+import {getOpenRouterApiClient, type NormalizedModel} from './openrouter-api-client.js'
+
+/**
+ * Model fetcher that wraps the existing OpenRouterApiClient.
+ * Adapts NormalizedModel to ProviderModelInfo.
+ */
 export class OpenRouterModelFetcher implements IProviderModelFetcher {
   async fetchModels(apiKey: string, options?: FetchModelsOptions): Promise<ProviderModelInfo[]> {
     const client = getOpenRouterApiClient()

src/server/infra/provider-oauth/token-refresh-manager.ts

@@ -117,6 +117,7 @@ export class TokenRefreshManager implements ITokenRefreshManager {
     const contentType: TokenRequestContentType =
       oauthConfig.tokenContentType === 'form' ? 'application/x-www-form-urlencoded' : 'application/json'
 
+    // 5. Attempt refresh
     try {
       const tokens = await this.exchangeRefreshToken({
         clientId: oauthConfig.clientId,
@@ -125,6 +126,7 @@ export class TokenRefreshManager implements ITokenRefreshManager {
         tokenUrl: oauthConfig.tokenUrl,
       })
 
+      // 6. Success: update keychain + encrypted store
       await this.deps.providerKeychainStore.setApiKey(providerId, tokens.access_token)
 
       const newExpiresAt = tokens.expires_in ? computeExpiresAt(tokens.expires_in) : tokenRecord.expiresAt
@@ -142,6 +144,7 @@ export class TokenRefreshManager implements ITokenRefreshManager {
   }
 
   private async handleRefreshError(providerId: string, error: unknown): Promise<boolean> {
+    // 7. Permanent failure (token revoked, client invalid): disconnect provider, clean up
     if (isPermanentOAuthError(error)) {
       await this.deps.providerConfigStore.disconnectProvider(providerId).catch(() => {})
       await this.deps.providerOAuthTokenStore.delete(providerId).catch(() => {})
@@ -150,6 +153,9 @@ export class TokenRefreshManager implements ITokenRefreshManager {
       return false
     }
 
+    // Transient errors (network timeout, 5xx): keep credentials intact.
+    // Return true so the caller uses the existing access token from the keychain,
+    // which may still be valid until it actually expires.
     processLog(
       `[TokenRefreshManager] Transient refresh error for ${providerId}: ${error instanceof Error ? error.message : String(error)}`,
     )

src/server/infra/transport/handlers/provider-handler.ts

@@ -77,14 +77,20 @@ type OAuthFlowState = {
 export interface ProviderHandlerDeps {
   authStateStore: IAuthStateStore
   browserLauncher: IBrowserLauncher
+  /** Factory for creating callback servers (injectable for testing) */
   createCallbackServer?: (options: {callbackPath?: string; port: number}) => ProviderCallbackServer
+  /** Token exchange function (injectable for testing) */
   exchangeCodeForTokens?: (params: TokenExchangeParams) => Promise<ProviderTokenResponse>
+  /** Copilot token exchange function (injectable for testing) */
   exchangeForCopilotToken?: (githubToken: string) => Promise<CopilotTokenResponse>
+  /** PKCE generator function (injectable for testing) */
   generatePkce?: () => PkceParameters
+  /** Device flow polling function (injectable for testing) */
   pollForAccessToken?: (params: PollForAccessTokenParams) => Promise<string>
   providerConfigStore: IProviderConfigStore
   providerKeychainStore: IProviderKeychainStore
   providerOAuthTokenStore: IProviderOAuthTokenStore
+  /** Device code request function (injectable for testing) */
   requestDeviceCode?: (params: RequestDeviceCodeParams) => Promise<DeviceCodeResponse>
   transport: ITransportServer
 }
@@ -165,18 +171,23 @@ export class ProviderHandler {
       tokenUrl: oauthConfig.tokenUrl,
     })
 
+    // Parse JWT id_token for account ID
     const oauthAccountId = tokens.id_token ? parseAccountIdFromIdToken(tokens.id_token) : undefined
 
+    // Store access token as the "API key" in keychain
     await this.providerKeychainStore.setApiKey(providerId, tokens.access_token)
 
+    // Store refresh token + expiry in encrypted OAuth token store
     if (tokens.refresh_token) {
-      const expiresAt = tokens.expires_in ? computeExpiresAt(tokens.expires_in) : computeExpiresAt(3600)
+      const expiresAt = tokens.expires_in ? computeExpiresAt(tokens.expires_in) : computeExpiresAt(3600) // 1-hour default when provider omits expires_in
       await this.providerOAuthTokenStore.set(providerId, {
         expiresAt,
         refreshToken: tokens.refresh_token,
       })
     }
 
+    // Connect provider — secrets stored in keychain + encrypted token store, not config
+    // OAuth providers may define their own default model (e.g., Codex for OpenAI OAuth)
     const defaultModel = oauthConfig.defaultModel ?? providerDef.defaultModel
     await this.providerConfigStore.connectProvider(providerId, {
       activeModel: defaultModel,
@@ -266,6 +277,7 @@ export class ProviderHandler {
 
           return {error: getErrorMessage(error), success: false}
         } finally {
+          // Only clean up if this is still the same flow (guard against concurrent START_OAUTH)
           if (this.oauthFlows.get(data.providerId) === flow) {
             await flow.callbackServer?.stop().catch(() => {})
             this.oauthFlows.delete(data.providerId)
@@ -429,8 +441,10 @@ export class ProviderHandler {
             return await this.startDeviceFlow(data.providerId, oauthConfig, clientId)
           }
 
+          // Generate PKCE parameters
           const pkce = this.generatePkce()
 
+          // Build auth URL
           const mode = oauthConfig.modes.find((m) => m.id === (data.mode ?? 'default')) ?? oauthConfig.modes[0]
           const params = new URLSearchParams({
             client_id: oauthConfig.clientId,
@@ -442,6 +456,7 @@ export class ProviderHandler {
             state: pkce.state,
           })
 
+          // Provider-specific extra params (e.g. OpenAI's codex_cli_simplified_flow)
           if (oauthConfig.extraParams) {
             for (const [key, value] of Object.entries(oauthConfig.extraParams)) {
               params.set(key, value)
@@ -450,19 +465,22 @@ export class ProviderHandler {
 
           const authUrl = `${mode.authUrl}?${params.toString()}`
 
+          // Start callback server for auto mode
           let callbackServer: ProviderCallbackServer | undefined
           if (oauthConfig.callbackMode === 'auto' && oauthConfig.callbackPort) {
             callbackServer = this.createCallbackServer({port: oauthConfig.callbackPort})
             await callbackServer.start()
           }
 
+          // Store flow state
           this.oauthFlows.set(data.providerId, {
             callbackServer,
             clientId,
             codeVerifier: pkce.codeVerifier,
             state: pkce.state,
           })
 
+          // Open browser (non-fatal on failure)
           try {
             await this.browserLauncher.open(authUrl)
           } catch {
@@ -471,6 +489,7 @@ export class ProviderHandler {
 
           return {authUrl, callbackMode: oauthConfig.callbackMode, success: true}
         } catch (error) {
+          // Clean up callback server if it was started but flow setup failed
           const partialFlow = this.oauthFlows.get(data.providerId)
           if (partialFlow?.callbackServer) {
             await partialFlow.callbackServer.stop().catch(() => {})

test/unit/infra/http/copilot-model-fetcher.test.ts

@@ -1,4 +1,5 @@
 /* eslint-disable camelcase */
+import axios from 'axios'
 import {expect} from 'chai'
 import nock from 'nock'
 import {restore, stub} from 'sinon'
@@ -158,5 +159,15 @@ describe('CopilotModelFetcher', () => {
 
       expect(result.isValid).to.be.false
     })
+
+    it('should return isValid false with error message for non-Axios errors', async () => {
+      stub(axios, 'get').rejects(new TypeError('Cannot read properties of undefined'))
+
+      const fetcher = new CopilotModelFetcher()
+      const result = await fetcher.validateApiKey('token')
+
+      expect(result.isValid).to.be.false
+      expect(result.error).to.equal('Cannot read properties of undefined')
+    })
   })
 })

test/unit/infra/provider-oauth/token-refresh-manager.test.ts

@@ -22,6 +22,7 @@ import {
   createMockTransportServer,
 } from '../../../helpers/mock-factories.js'
 
+// Helper to create a provider config with OAuth
 function oauthConfig(providerId: string): ProviderConfig {
   return ProviderConfig.createDefault().withProviderConnected(providerId, {
     authMethod: 'oauth',