Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 62 additions & 3 deletions src/server/core/domain/entities/provider-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,25 @@
* Non-sensitive data (API keys stored separately in keychain).
*/

/**
* Durable record of why/when a provider was disconnected.
*
* Persisted on the provider entry (tombstone) when the provider was disconnected
* with a recorded reason — e.g. a permanent OAuth refresh failure — so the user
* can later see what happened and how to reconnect, instead of silently finding
* "no provider connected" with no explanation.
*/
export interface ProviderDisconnectInfo {
/** ISO timestamp of when the disconnect happened */
readonly at: string
/** OAuth error code when available (e.g. invalid_grant, invalid_client) */
readonly errorCode?: string
/** Human-readable reason (e.g. "OAuth token refresh failed") */
readonly reason: string
/** HTTP status code when available (e.g. 400, 401) */
readonly statusCode?: number
}

/**
* Configuration for a single connected provider.
*/
Expand All @@ -21,6 +40,12 @@ export interface ConnectedProviderConfig {
readonly connectedAt: string
/** User's favorite models (for quick access) */
readonly favoriteModels: readonly string[]
/**
* Set when the provider was disconnected with a recorded reason. Its presence
* marks this entry as a disconnected tombstone — `isProviderConnected` returns
* false while it is set, and reconnecting clears it.
*/
readonly lastDisconnect?: ProviderDisconnectInfo
/** OAuth account ID (e.g. ChatGPT-Account-Id for OpenAI) */
readonly oauthAccountId?: string
/** Recently used models (last 10) */
Expand Down Expand Up @@ -131,9 +156,13 @@ export class ProviderConfig {

/**
* Check if a provider is connected.
*
* A disconnected tombstone (an entry retained only to record `lastDisconnect`)
* is NOT connected — it exists purely to surface why the provider dropped.
*/
public isProviderConnected(providerId: string): boolean {
return providerId in this.providers
const entry = this.providers[providerId]
return entry !== undefined && entry.lastDisconnect === undefined
}

/**
Expand Down Expand Up @@ -249,11 +278,41 @@ export class ProviderConfig {

/**
* Create a new config with a provider disconnected.
*
* Without `details`, the provider entry is removed entirely (a clean manual
* disconnect). With `details`, the entry is retained as a tombstone carrying a
* `lastDisconnect` record so the reason survives to the providers view — used
* when the disconnect was involuntary (e.g. a permanent OAuth refresh failure).
* The active provider is cleared either way when it was the disconnected one.
*/
public withProviderDisconnected(providerId: string): ProviderConfig {
public withProviderDisconnected(
providerId: string,
details?: {errorCode?: string; reason: string; statusCode?: number},
): ProviderConfig {
const newActiveProvider = this.activeProvider === providerId ? '' : this.activeProvider
const existingConfig = this.providers[providerId]

// Record a tombstone (keep the entry) when a reason is supplied and the
// provider actually existed — so the disconnect is visible after the fact.
if (details && existingConfig) {
const lastDisconnect: ProviderDisconnectInfo = {
at: new Date().toISOString(),
errorCode: details.errorCode,
reason: details.reason,
statusCode: details.statusCode,
}

return new ProviderConfig({
activeProvider: newActiveProvider,
providers: {
...this.providers,
[providerId]: {...existingConfig, lastDisconnect},
},
})
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
const {[providerId]: _removed, ...remainingProviders} = this.providers
const newActiveProvider = this.activeProvider === providerId ? '' : this.activeProvider

return new ProviderConfig({
activeProvider: newActiveProvider,
Expand Down
9 changes: 8 additions & 1 deletion src/server/core/interfaces/i-provider-config-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,15 @@ export interface IProviderConfigStore {
* Removes a provider connection.
*
* @param providerId The provider ID to disconnect
* @param details Optional disconnect reason. When provided, the provider entry
* is retained as a tombstone carrying `lastDisconnect` (so the reason is
* visible in the providers view) instead of being removed entirely. Omit for
* a clean manual disconnect that fully removes the entry.
*/
disconnectProvider: (providerId: string) => Promise<void>
disconnectProvider: (
providerId: string,
details?: {errorCode?: string; reason: string; statusCode?: number},
) => Promise<void>

/**
* Gets the active model for a provider.
Expand Down
21 changes: 18 additions & 3 deletions src/server/infra/provider-oauth/token-refresh-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type {ProviderTokenResponse, RefreshTokenExchangeParams, TokenRequestCont
import {getProviderById} from '../../core/domain/entities/provider-registry.js'
import {TransportDaemonEventNames} from '../../core/domain/transport/schemas.js'
import {processLog} from '../../utils/process-logger.js'
import {isPermanentOAuthError} from './errors.js'
import {isPermanentOAuthError, ProviderTokenExchangeError} from './errors.js'
import {exchangeRefreshToken as defaultExchangeRefreshToken} from './refresh-token-exchange.js'
import {computeExpiresAt} from './types.js'

Expand Down Expand Up @@ -111,9 +111,24 @@ export class TokenRefreshManager implements ITokenRefreshManager {
this.deps.transport.broadcast(TransportDaemonEventNames.PROVIDER_UPDATED, {})
return true
} catch (error) {
// 7. Permanent failure (token revoked, client invalid): disconnect provider, clean up
// 7. Permanent failure (token revoked, client invalid): disconnect provider, clean up.
// Record the reason durably (lastDisconnect tombstone) and log symmetrically
// with the transient branch so the disconnect leaves a visible trace instead
// of silently dropping the provider.
if (isPermanentOAuthError(error)) {
await this.deps.providerConfigStore.disconnectProvider(providerId).catch(() => {})
const statusCode = error instanceof ProviderTokenExchangeError ? error.statusCode : undefined
const errorCode = error instanceof ProviderTokenExchangeError ? error.errorCode : undefined
const detail = [statusCode ? `status ${statusCode}` : undefined, errorCode].filter(Boolean).join(', ')

processLog(
`[TokenRefreshManager] Permanent refresh failure for ${providerId}${
detail ? ` (${detail})` : ''
}: disconnecting provider`,
)

await this.deps.providerConfigStore
.disconnectProvider(providerId, {errorCode, reason: 'OAuth token refresh failed', statusCode})
.catch(() => {})
await this.deps.providerOAuthTokenStore.delete(providerId).catch(() => {})
await this.deps.providerKeychainStore.deleteApiKey(providerId).catch(() => {})
this.deps.transport.broadcast(TransportDaemonEventNames.PROVIDER_UPDATED, {})
Expand Down
7 changes: 6 additions & 1 deletion src/server/infra/provider/provider-config-resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,12 @@ export async function clearStaleProviderConfig(
})),
)

const staleProviderIds = results.filter(({accessible}) => !accessible).map(({providerId}) => providerId)
// Skip entries already tombstoned with a lastDisconnect reason: their
// credentials are intentionally gone, and re-disconnecting would drop the
// tombstone (deleting the entry) and erase the recorded disconnect reason.
const staleProviderIds = results
.filter(({accessible, providerId}) => !accessible && config.providers[providerId]?.lastDisconnect === undefined)
.map(({providerId}) => providerId)

if (staleProviderIds.length === 0) return

Expand Down
10 changes: 7 additions & 3 deletions src/server/infra/storage/file-provider-config-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,15 @@ export class FileProviderConfigStore implements IProviderConfigStore {
}

/**
* Removes a provider connection.
* Removes a provider connection. When `details` is supplied, the entry is
* retained as a `lastDisconnect` tombstone (see IProviderConfigStore).
*/
public async disconnectProvider(providerId: string): Promise<void> {
public async disconnectProvider(
providerId: string,
details?: {errorCode?: string; reason: string; statusCode?: number},
): Promise<void> {
const config = await this.read()
const newConfig = config.withProviderDisconnected(providerId)
const newConfig = config.withProviderDisconnected(providerId, details)
await this.write(newConfig)
}

Expand Down
5 changes: 4 additions & 1 deletion src/server/infra/transport/handlers/model-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,10 @@ export class ModelHandler {
const config = await this.providerConfigStore.read()
const providerConfig = config.providers[data.providerId]

if (!providerConfig) {
// A tombstoned entry (disconnected with a recorded reason, e.g. a permanent
// OAuth refresh failure) still exists in config.providers, so presence alone
// isn't enough — isProviderConnected() also rejects tombstones.
if (!providerConfig || !config.isProviderConnected(data.providerId)) {
return {
error: `Provider "${data.providerId}" is not connected`,
success: false,
Expand Down
15 changes: 15 additions & 0 deletions src/server/infra/transport/handlers/provider-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ export class ProviderHandler {
return false
}),
isCurrent: def.id === activeProviderId,
lastDisconnect: providerConfig?.lastDisconnect,
name: def.name,
oauthCallbackMode: def.oauth?.callbackMode,
oauthLabel: def.oauth?.modes[0]?.label,
Expand All @@ -409,6 +410,20 @@ export class ProviderHandler {
return {error: BYTEROVER_AUTH_REQUIRED_MESSAGE, success: false}
}

// byterover's gate is isByteRoverAuthSatisfied() above — it is authorized
// independently of config.providers (see resolveProviderConfig's early
// return for it), so it is exempt from the isProviderConnected check below.
if (data.providerId !== 'byterover') {
const config = await this.providerConfigStore.read()
// A tombstoned entry (disconnected with a recorded reason, e.g. a
// permanent OAuth refresh failure) still exists in config.providers —
// isProviderConnected() rejects it so SET_ACTIVE can't silently
// reactivate a provider whose credentials were deliberately dropped.
if (!config.isProviderConnected(data.providerId)) {
return {error: `Provider "${data.providerId}" is not connected`, success: false}
}
}

await this.providerConfigStore.setActiveProvider(data.providerId)
this.transport.broadcast(TransportDaemonEventNames.PROVIDER_UPDATED, {})
return {success: true}
Expand Down
7 changes: 7 additions & 0 deletions src/shared/transport/types/dto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,13 @@ export interface ProviderDTO {
id: string
isConnected: boolean
isCurrent: boolean
/**
* Present when the provider was disconnected with a recorded reason (e.g. a
* permanent OAuth refresh failure). Set alongside `isConnected: false` so the
* providers view can surface when/why it dropped and how to reconnect
* (`brv providers connect <id> --oauth` for oauth authMethod, else without `--oauth`).
*/
lastDisconnect?: {at: string; errorCode?: string; reason: string; statusCode?: number}
name: string
oauthCallbackMode?: 'auto' | 'code-paste'
oauthLabel?: string
Expand Down
90 changes: 90 additions & 0 deletions test/unit/core/domain/entities/provider-config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,5 +184,95 @@ describe('ProviderConfig', () => {

expect(disconnected.activeProvider).to.equal('anthropic')
})

it('should retain a tombstone with lastDisconnect when details are provided', () => {
const config = ProviderConfig.createDefault()
.withProviderConnected('openai', {authMethod: 'oauth', oauthAccountId: 'acct_123'})
.withActiveProvider('openai')

const disconnected = config.withProviderDisconnected('openai', {
errorCode: 'invalid_grant',
reason: 'OAuth token refresh failed',
statusCode: 400,
})

// Entry is kept (exists in config) but reports as not connected
expect(disconnected.providers.openai).to.not.be.undefined
expect(disconnected.isProviderConnected('openai')).to.be.false
expect(disconnected.activeProvider).to.equal('')

const {lastDisconnect} = disconnected.providers.openai
expect(lastDisconnect?.reason).to.equal('OAuth token refresh failed')
expect(lastDisconnect?.errorCode).to.equal('invalid_grant')
expect(lastDisconnect?.statusCode).to.equal(400)
expect(lastDisconnect?.at).to.be.a('string')

if (!lastDisconnect) {
throw new Error('Expected lastDisconnect to be set')
}

expect(Number.isNaN(Date.parse(lastDisconnect.at))).to.be.false

// Last-known authMethod is preserved so the view can build the reconnect hint
expect(disconnected.providers.openai.authMethod).to.equal('oauth')
})

it('should record lastDisconnect without optional error/status fields', () => {
const config = ProviderConfig.createDefault().withProviderConnected('openrouter', {authMethod: 'api-key'})

const disconnected = config.withProviderDisconnected('openrouter', {reason: 'Manual disconnect'})

const {lastDisconnect} = disconnected.providers.openrouter
expect(lastDisconnect?.reason).to.equal('Manual disconnect')
expect(lastDisconnect?.errorCode).to.be.undefined
expect(lastDisconnect?.statusCode).to.be.undefined
})

it('should remove the entry entirely when details are omitted even if it existed', () => {
const config = ProviderConfig.createDefault().withProviderConnected('openai', {authMethod: 'oauth'})

const disconnected = config.withProviderDisconnected('openai')

expect(disconnected.providers.openai).to.be.undefined
expect(disconnected.isProviderConnected('openai')).to.be.false
})

it('should not create a tombstone for a provider that was never connected', () => {
const config = ProviderConfig.createDefault()

const disconnected = config.withProviderDisconnected('openai', {reason: 'OAuth token refresh failed'})

expect(disconnected.providers.openai).to.be.undefined
})

it('should clear lastDisconnect when the provider is reconnected', () => {
const disconnected = ProviderConfig.createDefault()
.withProviderConnected('openai', {authMethod: 'oauth'})
.withProviderDisconnected('openai', {errorCode: 'invalid_grant', reason: 'OAuth token refresh failed'})

expect(disconnected.isProviderConnected('openai')).to.be.false

const reconnected = disconnected.withProviderConnected('openai', {authMethod: 'oauth'})

expect(reconnected.isProviderConnected('openai')).to.be.true
expect(reconnected.providers.openai.lastDisconnect).to.be.undefined
})

it('should round-trip lastDisconnect through toJson/fromJson', () => {
const disconnected = ProviderConfig.createDefault()
.withProviderConnected('openai', {authMethod: 'oauth'})
.withProviderDisconnected('openai', {
errorCode: 'invalid_grant',
reason: 'OAuth token refresh failed',
statusCode: 400,
})

const restored = ProviderConfig.fromJson(disconnected.toJson())

expect(restored.isProviderConnected('openai')).to.be.false
expect(restored.providers.openai.lastDisconnect?.reason).to.equal('OAuth token refresh failed')
expect(restored.providers.openai.lastDisconnect?.errorCode).to.equal('invalid_grant')
expect(restored.providers.openai.lastDisconnect?.statusCode).to.equal(400)
})
})
})
12 changes: 12 additions & 0 deletions test/unit/infra/provider-oauth/token-refresh-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,13 @@ describe('TokenRefreshManager', () => {
expect(providerOAuthTokenStore.delete.calledWith('openai')).to.be.true
expect(providerKeychainStore.deleteApiKey.calledWith('openai')).to.be.true
expect(transport.broadcast.calledWith(TransportDaemonEventNames.PROVIDER_UPDATED, {})).to.be.true

// Verify the disconnect reason/details were plumbed through so the drop is
// recorded durably (lastDisconnect) instead of vanishing silently.
const [disconnectedId, disconnectDetails] = providerConfigStore.disconnectProvider.firstCall.args
expect(disconnectedId).to.equal('openai')
expect(disconnectDetails).to.include({errorCode: 'invalid_grant', statusCode: 400})
expect(disconnectDetails?.reason).to.equal('OAuth token refresh failed')
})

it('should return true and keep credentials intact on transient refresh failure', async () => {
Expand Down Expand Up @@ -319,6 +326,11 @@ describe('TokenRefreshManager', () => {
expect(providerOAuthTokenStore.delete.calledWith('openai')).to.be.true
expect(providerKeychainStore.deleteApiKey.calledWith('openai')).to.be.true
expect(transport.broadcast.calledWith(TransportDaemonEventNames.PROVIDER_UPDATED, {})).to.be.true

// Reason/details still plumbed through even when a later cleanup step fails.
const disconnectDetails = providerConfigStore.disconnectProvider.firstCall.args[1]
expect(disconnectDetails).to.include({errorCode: 'invalid_grant', statusCode: 401})
expect(disconnectDetails?.reason).to.equal('OAuth token refresh failed')
})
})

Expand Down
28 changes: 28 additions & 0 deletions test/unit/infra/provider/provider-config-resolver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -531,5 +531,33 @@ describe('provider-config-resolver', () => {

expect(configStore.write.calledOnce).to.be.true
})

it('should preserve an already-tombstoned provider instead of re-disconnecting it', async () => {
const {configStore, keychainStore} = createStubStores(sandbox)
// A provider that was disconnected with a recorded reason: its keychain key
// is intentionally gone, so it would otherwise look "stale" and be removed.
const tombstoned = ProviderConfig.createDefault()
.withProviderConnected('openai', {authMethod: 'oauth'})
.withProviderDisconnected('openai', {
errorCode: 'invalid_grant',
reason: 'OAuth token refresh failed',
statusCode: 400,
})
configStore.read.resolves(tombstoned)
keychainStore.getApiKey.resolves()

const oauthTokenStore: SinonStubbedInstance<IProviderOAuthTokenStore> = {
delete: sandbox.stub().resolves(),
get: sandbox.stub().resolves(),
has: sandbox.stub().resolves(false),
set: sandbox.stub().resolves(),
} as unknown as SinonStubbedInstance<IProviderOAuthTokenStore>

await clearStaleProviderConfig(configStore, keychainStore, oauthTokenStore)

// Tombstone left untouched: no re-write, no re-delete — the reason survives
expect(configStore.write.notCalled).to.be.true
expect(oauthTokenStore.delete.notCalled).to.be.true
})
})
})
Loading