From 86e3e0dcedfd1d18c2bbdcbd2b4d135d7675498e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Thu=E1=BA=ADn=20Ph=C3=A1t?= Date: Sun, 3 May 2026 16:48:22 +0700 Subject: [PATCH 1/3] fix: [ENG-2548] activate byterover provider on connect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #549 added a deferred-activation gate in provider:connect that skips withActiveProvider when the provider has no defaultModel and no existing activeModel. byterover hits both conditions on a fresh connect — the registry has no defaultModel, and the user has no `brv model switch` recovery path because byterover has no model fetcher and the model-switch command is hard-blocked. The result was "connected but never active": brv providers showed an empty active provider, brv curate failed with "no provider connected", and the TUI bounced users back to the setup view. Bypasses the gate by id for byterover. The registry stays free of defaultModel intentionally so the runtime fallback in agent-process.ts (activeModel ?? DEFAULT_LLM_MODEL) keeps owning model resolution — that way changing the constant rolls out a new default to every existing user without needing a per-user migration step. openai-compatible — the original target of the PR #549 gate — is unchanged: still defers, still recovers via model:setActive when the user picks a model. --- .../infra/transport/handlers/provider-handler.ts | 9 ++++++++- .../domain/entities/provider-registry.test.ts | 9 +++++++++ .../transport/handlers/provider-handler.test.ts | 16 ++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/server/infra/transport/handlers/provider-handler.ts b/src/server/infra/transport/handlers/provider-handler.ts index 38834f1ef..a751ae19a 100644 --- a/src/server/infra/transport/handlers/provider-handler.ts +++ b/src/server/infra/transport/handlers/provider-handler.ts @@ -300,7 +300,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' + || Boolean(provider?.defaultModel) || Boolean(await this.providerConfigStore.getActiveModel(providerId)) await this.providerConfigStore.connectProvider(providerId, { activeModel: provider?.defaultModel, diff --git a/test/unit/core/domain/entities/provider-registry.test.ts b/test/unit/core/domain/entities/provider-registry.test.ts index e3da74413..e21b64afd 100644 --- a/test/unit/core/domain/entities/provider-registry.test.ts +++ b/test/unit/core/domain/entities/provider-registry.test.ts @@ -107,6 +107,15 @@ 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)', () => { + // byterover's model is intentionally NOT persisted in providers.json. + // The runtime resolver in agent-process.ts falls back to DEFAULT_LLM_MODEL + // on every invocation, so changing the constant rolls out to all users + // without a per-user migration step. + 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 diff --git a/test/unit/infra/transport/handlers/provider-handler.test.ts b/test/unit/infra/transport/handlers/provider-handler.test.ts index 360b80196..b195a250a 100644 --- a/test/unit/infra/transport/handlers/provider-handler.test.ts +++ b/test/unit/infra/transport/handlers/provider-handler.test.ts @@ -367,6 +367,22 @@ describe('ProviderHandler', () => { const connectArgs = providerConfigStore.connectProvider.firstCall.args[1] as Record expect(connectArgs.setAsActive).to.equal(true) }) + + it('should activate byterover on connect without persisting an activeModel', async () => { + // byterover has no model fetcher and `brv model switch --provider byterover` + // is hard-blocked, so the deferred-activation gate would strand it forever. + // It bypasses the gate by id and intentionally leaves activeModel unset — + // the runtime resolver in agent-process.ts falls back to DEFAULT_LLM_MODEL, + // which lets future default changes roll out without a per-user migration. + createHandler() + + const handler = transport._handlers.get(ProviderEvents.CONNECT) + await handler!({providerId: 'byterover'}, 'client-1') + + const connectArgs = providerConfigStore.connectProvider.firstCall.args[1] as Record + expect(connectArgs.setAsActive).to.equal(true) + expect(connectArgs.activeModel).to.be.undefined + }) }) describe('provider:disconnect', () => { From 4357580062c3a9176124ab03e279979cbbf3878d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Thu=E1=BA=ADn=20Ph=C3=A1t?= Date: Sun, 3 May 2026 17:33:49 +0700 Subject: [PATCH 2/3] refactor: [ENG-2548] address review-agent feedback on test comments Trim multi-line block comments on the two new tests to one short line each, per CLAUDE.md "one short line max" guidance. The PR description already carries the full rationale; the test names are self-documenting; the inline comments now flag only the non-obvious why ("byterover bypasses the gate", "intentional runtime resolution"). The third review note (extract 'byterover' string literal in provider-handler.ts to a constant or ProviderDefinition flag) is non-blocking per the reviewer and aligns with the follow-up refactor already documented in the PR's risks section. Deferring to that ticket to keep this PR scoped to the regression fix. --- test/unit/core/domain/entities/provider-registry.test.ts | 5 +---- test/unit/infra/transport/handlers/provider-handler.test.ts | 6 +----- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/test/unit/core/domain/entities/provider-registry.test.ts b/test/unit/core/domain/entities/provider-registry.test.ts index e21b64afd..564442fca 100644 --- a/test/unit/core/domain/entities/provider-registry.test.ts +++ b/test/unit/core/domain/entities/provider-registry.test.ts @@ -108,10 +108,7 @@ describe('Provider Registry', () => { }) it('should not have a defaultModel for byterover (model is resolved at runtime via DEFAULT_LLM_MODEL)', () => { - // byterover's model is intentionally NOT persisted in providers.json. - // The runtime resolver in agent-process.ts falls back to DEFAULT_LLM_MODEL - // on every invocation, so changing the constant rolls out to all users - // without a per-user migration step. + // intentional: model is runtime-resolved so default changes auto-roll without per-user migration. const provider = getProviderById('byterover') expect(provider?.defaultModel).to.be.undefined }) diff --git a/test/unit/infra/transport/handlers/provider-handler.test.ts b/test/unit/infra/transport/handlers/provider-handler.test.ts index b195a250a..65e0084a4 100644 --- a/test/unit/infra/transport/handlers/provider-handler.test.ts +++ b/test/unit/infra/transport/handlers/provider-handler.test.ts @@ -369,11 +369,7 @@ describe('ProviderHandler', () => { }) it('should activate byterover on connect without persisting an activeModel', async () => { - // byterover has no model fetcher and `brv model switch --provider byterover` - // is hard-blocked, so the deferred-activation gate would strand it forever. - // It bypasses the gate by id and intentionally leaves activeModel unset — - // the runtime resolver in agent-process.ts falls back to DEFAULT_LLM_MODEL, - // which lets future default changes roll out without a per-user migration. + // 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) From b576b7a42d36f53c4a6f7f34369294fc23544d33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Thu=E1=BA=ADn=20Ph=C3=A1t?= Date: Mon, 4 May 2026 11:38:26 +0700 Subject: [PATCH 3/3] fix: [ENG-2548] raise auth:getState client timeout to fit /user/me round-trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TUI's AuthInitializer fires auth:getState on every startup with a 500ms transport timeout. The daemon-side handler does an HTTPS round-trip to /user/me that takes 400-3100ms in practice (measured p99 across three home networks), so every TUI startup running on a normal-or-slow link timed out and the auth store fell back to its default isAuthorized: false. The bouncer was always firing — but useAppViewMode only routes to the provider picker on isAuthorized: false for byterover-active users. For non-byterover providers, view-mode routes to home regardless of isAuthorized, so users never saw the consequence. ENG-2548 making byterover reachable from CLI surfaced this latent bug. Bumps the per-attempt timeout to 4000ms — covers the worst observed sample (3085ms) with ~1.3x headroom plus margin for slower mobile, international, and VPN connections. Also drops React Query retries from 5 to 1: with the larger per-attempt budget, retrying 5x on a genuinely- offline machine would block startup for ~22s. One retry covers transient blips while keeping worst-case offline-wait under 9s. Backend revocation is still caught at startup (the network call still fires synchronously in setupGetState, just with enough time to complete). --- src/tui/features/auth/api/get-auth-state.ts | 9 ++++++--- src/tui/features/auth/components/auth-initializer.tsx | 6 ++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/tui/features/auth/api/get-auth-state.ts b/src/tui/features/auth/api/get-auth-state.ts index 5a021fc7c..a127cc0a1 100644 --- a/src/tui/features/auth/api/get-auth-state.ts +++ b/src/tui/features/auth/api/get-auth-state.ts @@ -9,9 +9,12 @@ export const getAuthState = (): Promise => { 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(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(AuthEvents.GET_STATE, undefined, {timeout: 4000}) } export const getAuthStateQueryOptions = () => diff --git a/src/tui/features/auth/components/auth-initializer.tsx b/src/tui/features/auth/components/auth-initializer.tsx index 5c397bf58..ca4b81f83 100644 --- a/src/tui/features/auth/components/auth-initializer.tsx +++ b/src/tui/features/auth/components/auth-initializer.tsx @@ -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. + retry: 1, + retryDelay: 500, staleTime: 2 * 60 * 1000, }, })