Skip to content
Merged
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
9 changes: 8 additions & 1 deletion src/server/infra/transport/handlers/provider-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,14 @@ export class ProviderHandler {
// "needs setup" and unmounts any in-flight setup flow on the home
// page. The model:setActive handler activates the provider when the
// user picks a model, which is the right moment.
const willHaveActiveModel = Boolean(provider?.defaultModel)
//
// byterover bypasses this gate: it has no model fetcher and no
// `brv model switch` recovery path, so deferring would strand it as
// connected-but-never-active. Its model is resolved at runtime via
// DEFAULT_LLM_MODEL in agent-process.ts rather than persisted here,
// so future default changes roll out without a per-user migration.
const willHaveActiveModel = providerId === 'byterover'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): 'byterover' now appears four times in this file (lines 259, 309, 344, 397). The pattern pre-dates this PR, but the PR description already flags that a second "no-model-fetcher" provider would need an authType field — at that point you'll be doing a search-replace across all four sites. Worth extracting a local constant, or a ProviderDefinition flag (noModelFetcher: true) to avoid the drift. The fix is correct and consistent with existing style; this is just a note for the follow-up refactor.

|| Boolean(provider?.defaultModel)
|| Boolean(await this.providerConfigStore.getActiveModel(providerId))
await this.providerConfigStore.connectProvider(providerId, {
activeModel: provider?.defaultModel,
Expand Down
9 changes: 6 additions & 3 deletions src/tui/features/auth/api/get-auth-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@ export const getAuthState = (): Promise<AuthGetStateResponse> => {
const {apiClient} = useTransportStore.getState()
if (!apiClient) return Promise.reject(new Error('Not connected'))

// Use 500ms timeout to fail fast if handler not ready yet
// React Query will retry automatically with exponential backoff
return apiClient.request<AuthGetStateResponse>(AuthEvents.GET_STATE, undefined, {timeout: 500})
// The daemon-side handler does a network round-trip to /user/me. Measured
// p99 across multiple networks ranges 1.2-3.1s with occasional outliers,
// so 4000ms gives ~1.3x headroom over the worst observed sample with
// margin left for slower connections (mobile, international, VPN).
// React Query retries once on failure for transient blips.
return apiClient.request<AuthGetStateResponse>(AuthEvents.GET_STATE, undefined, {timeout: 4000})
}

export const getAuthStateQueryOptions = () =>
Expand Down
6 changes: 4 additions & 2 deletions src/tui/features/auth/components/auth-initializer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ export function AuthInitializer({children}: {children: React.ReactNode}): React.
} = useGetAuthState({
queryConfig: {
enabled: apiClient !== null,
retry: 5,
retryDelay: (attemptIndex) => Math.min(500 * 2 ** attemptIndex, 2000),
// One retry covers transient blips; the per-attempt timeout is now generous
// (3s) so we don't need 5+ retries that would block startup for ~17s when offline.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (non-blocking): Comment says (3s) but the actual timeout in get-auth-state.ts:17 is 4000ms = 4s. Flagged in the previous two reviews; still present after the merge.

Suggested change
// (3s) so we don't need 5+ retries that would block startup for ~17s when offline.
// One retry covers transient blips; the per-attempt timeout is now generous
// (4s) so we don't need 5+ retries that would block startup for ~17s when offline.

retry: 1,
retryDelay: 500,
staleTime: 2 * 60 * 1000,
},
})
Expand Down
6 changes: 6 additions & 0 deletions test/unit/core/domain/entities/provider-registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ describe('Provider Registry', () => {
expect(provider?.oauth).to.be.undefined
})

it('should not have a defaultModel for byterover (model is resolved at runtime via DEFAULT_LLM_MODEL)', () => {
// intentional: model is runtime-resolved so default changes auto-roll without per-user migration.
const provider = getProviderById('byterover')
expect(provider?.defaultModel).to.be.undefined
})

it('should not have oauth config for anthropic yet', () => {
const provider = getProviderById('anthropic')
expect(provider?.oauth).to.be.undefined
Expand Down
12 changes: 12 additions & 0 deletions test/unit/infra/transport/handlers/provider-handler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,18 @@ describe('ProviderHandler', () => {
const connectArgs = providerConfigStore.connectProvider.firstCall.args[1] as Record<string, unknown>
expect(connectArgs.setAsActive).to.equal(true)
})

it('should activate byterover on connect without persisting an activeModel', async () => {
// byterover bypasses the gate (no model fetcher, no model-switch recovery path); runtime resolves via DEFAULT_LLM_MODEL.
createHandler()

const handler = transport._handlers.get(ProviderEvents.CONNECT)
await handler!({providerId: 'byterover'}, 'client-1')

const connectArgs = providerConfigStore.connectProvider.firstCall.args[1] as Record<string, unknown>
expect(connectArgs.setAsActive).to.equal(true)
expect(connectArgs.activeModel).to.be.undefined
})
})

describe('provider:disconnect', () => {
Expand Down
Loading