From f481ab1a213107ad01db333c087c985841d5db07 Mon Sep 17 00:00:00 2001 From: wzlng Date: Wed, 22 Apr 2026 22:35:50 +0700 Subject: [PATCH 1/5] feat: [ENG-2354] default brv login to OAuth; --api-key for CI only Running `brv login` with no flags now opens a browser for OAuth instead of rejecting with "Missing required flag api-key". This matches the TUI `/login` slash command and removes the copy/paste-from-settings-page step that users kept getting stuck on. `--api-key` is still accepted (and required in non-interactive shells, with a clear fail-fast message pointing at the docs) so CI and scripts continue to work unchanged. OAuth errors now route through formatConnectionError on the same path as api-key errors, so users get "Daemon failed to start automatically..." instead of raw transport messages. --- src/oclif/commands/login.ts | 148 +++++++++++++++++++++++------ test/commands/login.test.ts | 181 +++++++++++++++++++++++++++++++++++- 2 files changed, 299 insertions(+), 30 deletions(-) diff --git a/src/oclif/commands/login.ts b/src/oclif/commands/login.ts index 888e51969..9a8be7b1e 100644 --- a/src/oclif/commands/login.ts +++ b/src/oclif/commands/login.ts @@ -1,22 +1,42 @@ import {Command, Flags} from '@oclif/core' -import {AuthEvents, type AuthLoginWithApiKeyResponse} from '../../shared/transport/events/auth-events.js' +import { + AuthEvents, + type AuthLoginCompletedEvent, + type AuthLoginWithApiKeyResponse, + type AuthStartLoginResponse, +} from '../../shared/transport/events/auth-events.js' import {type DaemonClientOptions, formatConnectionError, withDaemonRetry} from '../lib/daemon-client.js' import {writeJsonResponse} from '../lib/json-response.js' +const DEFAULT_OAUTH_TIMEOUT_MS = 5 * 60 * 1000 + +type OutputFormat = 'json' | 'text' + +export interface LoginOAuthOptions extends DaemonClientOptions { + /** Max time to wait for LOGIN_COMPLETED after the browser opens. */ + oauthTimeoutMs?: number + /** Invoked with the auth URL once the daemon has started the flow. */ + onAuthUrl?: (authUrl: string) => void +} + export default class Login extends Command { public static description = 'Authenticate with ByteRover for cloud sync features (optional for local usage)' public static examples = [ + '# Browser OAuth (default)', + '<%= config.bin %> <%= command.id %>', + '', + '# API key (for CI / headless environments)', '<%= config.bin %> <%= command.id %> --api-key ', '', '# JSON output (for automation)', - '<%= config.bin %> <%= command.id %> --api-key --format json', + '<%= config.bin %> <%= command.id %> --format json', ] public static flags = { 'api-key': Flags.string({ char: 'k', - description: 'API key for authentication (get yours at https://app.byterover.dev/settings/keys)', - required: true, + description: + 'API key for headless/CI login (get yours at https://app.byterover.dev/settings/keys). Omit to use the browser OAuth flow.', }), format: Flags.string({ default: 'text', @@ -25,6 +45,10 @@ export default class Login extends Command { }), } + protected isInteractive(): boolean { + return process.stdout.isTTY === true && process.stdin.isTTY === true + } + protected async loginWithApiKey(apiKey: string, options?: DaemonClientOptions): Promise { return withDaemonRetry( async (client) => client.requestWithAck(AuthEvents.LOGIN_WITH_API_KEY, {apiKey}), @@ -32,40 +56,108 @@ export default class Login extends Command { ) } + protected async loginWithOAuth(options?: LoginOAuthOptions): Promise { + const timeoutMs = options?.oauthTimeoutMs ?? DEFAULT_OAUTH_TIMEOUT_MS + + return withDaemonRetry(async (client) => { + // Subscribe *before* initiating, so a fast callback cannot race past us. + let unsubscribe: (() => void) | undefined + let timer: NodeJS.Timeout | undefined + const completion = new Promise((resolve, reject) => { + timer = setTimeout(() => { + unsubscribe?.() + reject(new Error(`Login timed out after ${Math.round(timeoutMs / 1000)}s`)) + }, timeoutMs) + + unsubscribe = client.on(AuthEvents.LOGIN_COMPLETED, (data) => { + if (timer) clearTimeout(timer) + unsubscribe?.() + resolve(data) + }) + }) + + try { + const startResponse = await client.requestWithAck(AuthEvents.START_LOGIN) + options?.onAuthUrl?.(startResponse.authUrl) + + return await completion + } catch (error) { + if (timer) clearTimeout(timer) + unsubscribe?.() + throw error + } + }, options) + } + public async run(): Promise { const {flags} = await this.parse(Login) const apiKey = flags['api-key'] - const format = (flags.format ?? 'text') as 'json' | 'text' + const format: OutputFormat = flags.format === 'json' ? 'json' : 'text' - try { - if (format === 'text') { - this.log('Logging in...') - } - - const response = await this.loginWithApiKey(apiKey) + if (!apiKey && !this.isInteractive()) { + this.emitError( + format, + 'Non-interactive shell detected. Use --api-key for headless login (get yours at https://app.byterover.dev/settings/keys).', + ) + return + } - if (response.success) { - if (format === 'json') { - writeJsonResponse({command: 'login', data: {userEmail: response.userEmail}, success: true}) - } else { - this.log(`Logged in as ${response.userEmail}`) - } - } else { - const errorMessage = response.error ?? 'Authentication failed' - if (format === 'json') { - writeJsonResponse({command: 'login', data: {error: errorMessage}, success: false}) - } else { - this.log(errorMessage) - } - } + try { + await (apiKey ? this.runApiKey(apiKey, format) : this.runOAuth(format)) } catch (error) { - const errorMessage = error instanceof Error ? error.message : 'Login failed' - if (format === 'json') { - writeJsonResponse({command: 'login', data: {error: errorMessage}, success: false}) + const message = error instanceof Error ? error.message : 'Login failed' + this.emitError(format, message) } else { this.log(formatConnectionError(error)) } } } + + private emitError(format: OutputFormat, message: string): void { + if (format === 'json') { + writeJsonResponse({command: 'login', data: {error: message}, success: false}) + } else { + this.log(message) + } + } + + private emitSuccess(format: OutputFormat, userEmail: string | undefined): void { + if (format === 'json') { + writeJsonResponse({command: 'login', data: {userEmail}, success: true}) + } else { + this.log(`Logged in as ${userEmail}`) + } + } + + private async runApiKey(apiKey: string, format: OutputFormat): Promise { + if (format === 'text') { + this.log('Logging in...') + } + + const response = await this.loginWithApiKey(apiKey) + + if (response.success) { + this.emitSuccess(format, response.userEmail) + } else { + this.emitError(format, response.error ?? 'Authentication failed') + } + } + + private async runOAuth(format: OutputFormat): Promise { + const onAuthUrl = (authUrl: string): void => { + if (format === 'text') { + this.log('Opening browser for authentication...') + this.log(`If the browser did not open, visit: ${authUrl}`) + } + } + + const result = await this.loginWithOAuth({onAuthUrl}) + + if (result.success && result.user) { + this.emitSuccess(format, result.user.email) + } else { + this.emitError(format, result.error ?? 'Authentication failed') + } + } } diff --git a/test/commands/login.test.ts b/test/commands/login.test.ts index abdb3af24..4bf2356ce 100644 --- a/test/commands/login.test.ts +++ b/test/commands/login.test.ts @@ -6,13 +6,18 @@ import {Config as OclifConfig} from '@oclif/core' import {expect} from 'chai' import sinon, {restore, stub} from 'sinon' -import type {AuthLoginWithApiKeyResponse} from '../../src/shared/transport/events/auth-events.js' +import type { + AuthLoginCompletedEvent, + AuthLoginWithApiKeyResponse, + AuthStartLoginResponse, +} from '../../src/shared/transport/events/auth-events.js' -import Login from '../../src/oclif/commands/login.js' +import Login, {type LoginOAuthOptions} from '../../src/oclif/commands/login.js' // ==================== TestableLoginCommand ==================== class TestableLoginCommand extends Login { + public interactive = true private readonly mockConnector: () => Promise constructor(argv: string[], mockConnector: () => Promise, config: Config) { @@ -20,6 +25,10 @@ class TestableLoginCommand extends Login { this.mockConnector = mockConnector } + protected override isInteractive(): boolean { + return this.interactive + } + protected override async loginWithApiKey(apiKey: string): Promise { return super.loginWithApiKey(apiKey, { maxRetries: 1, @@ -27,6 +36,16 @@ class TestableLoginCommand extends Login { transportConnector: this.mockConnector, }) } + + protected override async loginWithOAuth(options?: LoginOAuthOptions): Promise { + return super.loginWithOAuth({ + ...options, + maxRetries: 1, + oauthTimeoutMs: 100, + retryDelayMs: 0, + transportConnector: this.mockConnector, + }) + } } // ==================== Tests ==================== @@ -100,6 +119,24 @@ describe('Login Command', () => { ;(mockClient.requestWithAck as sinon.SinonStub).resolves(response) } + function mockOAuthFlow(startResponse: AuthStartLoginResponse, completion?: AuthLoginCompletedEvent): void { + const onStub = mockClient.on as sinon.SinonStub + onStub.callsFake((event: string, cb: (data: AuthLoginCompletedEvent) => void) => { + if (event === 'auth:loginCompleted' && completion) { + setImmediate(() => { + cb(completion) + }) + } + + return () => {} + }) + + ;(mockClient.requestWithAck as sinon.SinonStub).callsFake((event: string) => { + if (event === 'auth:startLogin') return Promise.resolve(startResponse) + return Promise.resolve({}) + }) + } + // ==================== Successful Login ==================== describe('successful login', () => { @@ -224,4 +261,144 @@ describe('Login Command', () => { expect(loggedMessages.some((m) => m.includes('Something went wrong'))).to.be.true }) }) + + // ==================== OAuth Flow (no --api-key) ==================== + + describe('oauth flow', () => { + it('should start OAuth flow and print success on completion', async () => { + mockOAuthFlow( + {authUrl: 'https://auth.byterover.dev/oauth?state=abc'}, + {success: true, user: {email: 'oauth@example.com', hasOnboardedCli: true, id: 'u1', name: 'Oauth User'}}, + ) + + await createCommand().run() + + const requestWithAckCalls = (mockClient.requestWithAck as sinon.SinonStub).getCalls() + expect(requestWithAckCalls.some((c) => c.args[0] === 'auth:startLogin')).to.be.true + expect(loggedMessages.some((m) => m.includes('Logged in as oauth@example.com'))).to.be.true + }) + + it('should print the auth URL as a browser fallback', async () => { + mockOAuthFlow( + {authUrl: 'https://auth.byterover.dev/oauth?state=abc'}, + {success: true, user: {email: 'oauth@example.com', hasOnboardedCli: true, id: 'u1'}}, + ) + + await createCommand().run() + + expect(loggedMessages.some((m) => m.includes('https://auth.byterover.dev/oauth?state=abc'))).to.be.true + }) + + it('should print error message when LOGIN_COMPLETED reports failure', async () => { + mockOAuthFlow( + {authUrl: 'https://auth.byterover.dev/oauth'}, + {error: 'User denied access', success: false}, + ) + + await createCommand().run() + + expect(loggedMessages.some((m) => m.includes('User denied access'))).to.be.true + }) + + it('should time out if LOGIN_COMPLETED never arrives', async () => { + mockOAuthFlow({authUrl: 'https://auth.byterover.dev/oauth'}) + + await createCommand().run() + + expect(loggedMessages.some((m) => m.toLowerCase().includes('timed out'))).to.be.true + }) + + it('should emit JSON on successful OAuth login', async () => { + mockOAuthFlow( + {authUrl: 'https://auth.byterover.dev/oauth'}, + {success: true, user: {email: 'oauth@example.com', hasOnboardedCli: true, id: 'u1'}}, + ) + + await createJsonCommand().run() + + const json = parseJsonOutput() + expect(json.command).to.equal('login') + expect(json.success).to.be.true + expect(json.data).to.deep.include({userEmail: 'oauth@example.com'}) + }) + + it('should emit JSON on OAuth failure', async () => { + mockOAuthFlow({authUrl: 'https://auth.byterover.dev/oauth'}, {error: 'User denied access', success: false}) + + await createJsonCommand().run() + + const json = parseJsonOutput() + expect(json.command).to.equal('login') + expect(json.success).to.be.false + expect(json.data).to.deep.include({error: 'User denied access'}) + }) + + it('should clear the timeout and surface the error when START_LOGIN rejects', async () => { + let timerFired = false + ;(mockClient.on as sinon.SinonStub).returns(() => { + /* unsubscribe */ + }) + ;(mockClient.requestWithAck as sinon.SinonStub).rejects(new Error('start failed')) + + await createCommand().run() + // Wait past the 100 ms test timeout. If the timer was not cleared, it would + // reject an already-discarded promise and surface as an unhandled rejection. + await new Promise((resolve) => { + setTimeout(() => { + timerFired = true + resolve() + }, 150) + }) + + expect(timerFired).to.be.true + expect(loggedMessages.some((m) => m.includes('start failed'))).to.be.true + expect(loggedMessages.some((m) => m.toLowerCase().includes('timed out'))).to.be.false + }) + + it('should handle connection errors during OAuth flow via formatConnectionError', async () => { + mockConnector.rejects(new NoInstanceRunningError()) + + await createCommand().run() + + expect(loggedMessages.some((m) => m.includes('Daemon failed to start automatically'))).to.be.true + }) + }) + + // ==================== Non-interactive shells ==================== + + describe('non-interactive shell', () => { + it('should error with a pointer to --api-key when no flag and not a TTY', async () => { + const command = createCommand() + command.interactive = false + + await command.run() + + expect(loggedMessages.some((m) => m.toLowerCase().includes('non-interactive'))).to.be.true + expect(loggedMessages.some((m) => m.includes('--api-key'))).to.be.true + expect((mockClient.requestWithAck as sinon.SinonStub).called).to.be.false + }) + + it('should emit JSON error when non-interactive and no --api-key', async () => { + const command = createJsonCommand() + command.interactive = false + + await command.run() + + const json = parseJsonOutput() + expect(json.command).to.equal('login') + expect(json.success).to.be.false + expect(String(json.data.error ?? '').toLowerCase()).to.include('non-interactive') + }) + + it('should still perform api-key login when non-interactive and --api-key provided', async () => { + mockLoginResponse({success: true, userEmail: 'ci@example.com'}) + + const command = createCommand('--api-key', 'ci-key') + command.interactive = false + + await command.run() + + expect(loggedMessages.some((m) => m.includes('Logged in as ci@example.com'))).to.be.true + }) + }) }) From 79c133f1b5ad0337166b0a9eef2593a7eeef293d Mon Sep 17 00:00:00 2001 From: wzlng Date: Wed, 22 Apr 2026 23:37:38 +0700 Subject: [PATCH 2/5] feat: [ENG-2354] show provider descriptions in CLI and web picker Providers already ship a one-line description via ProviderDTO; surface it alongside the name in ByteRover [byterover] (current) OpenRouter [openrouter] Anthropic [anthropic] OpenAI [openai] Google Gemini [google] xAI (Grok) [xai] Groq [groq] Mistral [mistral] DeepInfra [deepinfra] Cohere [cohere] Together AI [togetherai] Perplexity [perplexity] Cerebras [cerebras] Vercel [vercel] MiniMax [minimax] GLM (Z.AI) [glm] Moonshot AI (Kimi) [moonshot] OpenAI Compatible [openai-compatible] and the web provider-picker dialog so users can pick by capability, not just by brand recognition. - oclif: second dim-indented line under each provider, skipped when empty - web picker: single-line truncated tagline under the name with title hover for the full string; uniform 2-line rows match the pattern used by Vercel/Raycast/GitHub Marketplace for dense lists - registry: trim two verbose descriptions (byterover, openai-compatible) that overflowed the picker's 1-line budget --- src/oclif/commands/providers/list.ts | 3 ++ .../core/domain/entities/provider-registry.ts | 4 +-- .../provider-flow/provider-select-step.tsx | 6 +++- test/commands/providers/list.test.ts | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/oclif/commands/providers/list.ts b/src/oclif/commands/providers/list.ts index 90a665207..2230ed0dc 100644 --- a/src/oclif/commands/providers/list.ts +++ b/src/oclif/commands/providers/list.ts @@ -40,6 +40,9 @@ export default class ProviderList extends Command { const authBadge = p.authMethod === 'oauth' ? chalk.cyan('[OAuth]') : p.authMethod === 'api-key' ? chalk.dim('[API Key]') : '' this.log(` ${p.name} [${p.id}] ${status} ${authBadge}`.trimEnd()) + if (p.description) { + this.log(` ${chalk.dim(p.description)}`) + } } } catch (error) { if (format === 'json') { diff --git a/src/server/core/domain/entities/provider-registry.ts b/src/server/core/domain/entities/provider-registry.ts index 614459f89..b5f9908d0 100644 --- a/src/server/core/domain/entities/provider-registry.ts +++ b/src/server/core/domain/entities/provider-registry.ts @@ -98,7 +98,7 @@ export const PROVIDER_REGISTRY: Readonly> = { byterover: { baseUrl: '', category: 'popular', - description: 'Built-in LLM, logged-in ByteRover account required. Limited free usage.', + description: 'Built-in LLM, ByteRover account required. Limited free usage.', headers: {}, id: 'byterover', modelsEndpoint: '', @@ -259,7 +259,7 @@ export const PROVIDER_REGISTRY: Readonly> = { baseUrl: '', category: 'other', defaultModel: 'llama3', - description: 'Connect any OpenAI-compatible endpoint (Ollama, LM Studio, etc.)', + description: 'OpenAI-compatible endpoint (Ollama, LM Studio, etc.)', envVars: ['OPENAI_COMPATIBLE_API_KEY'], headers: {}, id: 'openai-compatible', diff --git a/src/webui/features/provider/components/provider-flow/provider-select-step.tsx b/src/webui/features/provider/components/provider-flow/provider-select-step.tsx index 161a54f34..2e883f9e6 100644 --- a/src/webui/features/provider/components/provider-flow/provider-select-step.tsx +++ b/src/webui/features/provider/components/provider-flow/provider-select-step.tsx @@ -50,12 +50,16 @@ export function ProviderSelectStep({onSelect, providers}: ProviderSelectStepProp )} key={provider.id} onClick={() => onSelect(provider)} + title={provider.description} type="button" >
{icon && }
- {provider.name} +
+
{provider.name}
+
{provider.description}
+
{ expect(loggedMessages.some((m) => m.includes('(connected)'))).to.be.false expect(loggedMessages.some((m) => m.includes('Groq') && m.includes('[groq]'))).to.be.true }) + + it('should print description on a separate indented line', async () => { + mockListResponse([ + { + description: 'Claude models by Anthropic', + id: 'anthropic', + isConnected: true, + isCurrent: true, + name: 'Anthropic', + }, + ]) + + await createCommand().run() + + const headerIndex = loggedMessages.findIndex((m) => m.includes('Anthropic') && m.includes('[anthropic]')) + expect(headerIndex).to.be.greaterThan(-1) + const descriptionLine = loggedMessages[headerIndex + 1] + expect(descriptionLine).to.include('Claude models by Anthropic') + expect(descriptionLine?.startsWith(' ')).to.be.true + }) + + it('should skip the description line when description is empty', async () => { + mockListResponse([{description: '', id: 'groq', isConnected: false, isCurrent: false, name: 'Groq'}]) + + await createCommand().run() + + const headerIndex = loggedMessages.findIndex((m) => m.includes('Groq')) + const next = loggedMessages[headerIndex + 1] + // Next entry must not be an indented empty line + expect(next === undefined || !next.startsWith(' ')).to.be.true + }) }) // ==================== JSON Output ==================== From 3f7bb63a0d268170d9b3c16d337a3c15f7f63ea6 Mon Sep 17 00:00:00 2001 From: wzlng Date: Wed, 22 Apr 2026 23:58:25 +0700 Subject: [PATCH 3/5] refactor: [ENG-2354] post-review polish on brv login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Encapsulate test `interactive` flag behind `setInteractive()` now that the field is protected on the test subclass - Null out the OAuth timer after clearTimeout so the catch block can skip its redundant clear (intent, not behavior) - Route JSON errors through formatConnectionError too, so automation consumers get the same "Daemon failed to start automatically…" hints that text mode already showed --- src/oclif/commands/login.ts | 21 +++++++++++++------ test/commands/login.test.ts | 42 ++++++++++++++++++------------------- 2 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/oclif/commands/login.ts b/src/oclif/commands/login.ts index 9a8be7b1e..d11a01abc 100644 --- a/src/oclif/commands/login.ts +++ b/src/oclif/commands/login.ts @@ -66,11 +66,16 @@ export default class Login extends Command { const completion = new Promise((resolve, reject) => { timer = setTimeout(() => { unsubscribe?.() + timer = undefined reject(new Error(`Login timed out after ${Math.round(timeoutMs / 1000)}s`)) }, timeoutMs) unsubscribe = client.on(AuthEvents.LOGIN_COMPLETED, (data) => { - if (timer) clearTimeout(timer) + if (timer) { + clearTimeout(timer) + timer = undefined + } + unsubscribe?.() resolve(data) }) @@ -82,7 +87,11 @@ export default class Login extends Command { return await completion } catch (error) { - if (timer) clearTimeout(timer) + if (timer) { + clearTimeout(timer) + timer = undefined + } + unsubscribe?.() throw error } @@ -105,11 +114,11 @@ export default class Login extends Command { try { await (apiKey ? this.runApiKey(apiKey, format) : this.runOAuth(format)) } catch (error) { + const message = formatConnectionError(error) if (format === 'json') { - const message = error instanceof Error ? error.message : 'Login failed' this.emitError(format, message) } else { - this.log(formatConnectionError(error)) + this.log(message) } } } @@ -154,8 +163,8 @@ export default class Login extends Command { const result = await this.loginWithOAuth({onAuthUrl}) - if (result.success && result.user) { - this.emitSuccess(format, result.user.email) + if (result.success) { + this.emitSuccess(format, result.user?.email) } else { this.emitError(format, result.error ?? 'Authentication failed') } diff --git a/test/commands/login.test.ts b/test/commands/login.test.ts index 4bf2356ce..a066371a8 100644 --- a/test/commands/login.test.ts +++ b/test/commands/login.test.ts @@ -6,18 +6,18 @@ import {Config as OclifConfig} from '@oclif/core' import {expect} from 'chai' import sinon, {restore, stub} from 'sinon' -import type { - AuthLoginCompletedEvent, - AuthLoginWithApiKeyResponse, - AuthStartLoginResponse, -} from '../../src/shared/transport/events/auth-events.js' - import Login, {type LoginOAuthOptions} from '../../src/oclif/commands/login.js' +import { + AuthEvents, + type AuthLoginCompletedEvent, + type AuthLoginWithApiKeyResponse, + type AuthStartLoginResponse, +} from '../../src/shared/transport/events/auth-events.js' // ==================== TestableLoginCommand ==================== class TestableLoginCommand extends Login { - public interactive = true + protected interactive = true private readonly mockConnector: () => Promise constructor(argv: string[], mockConnector: () => Promise, config: Config) { @@ -46,6 +46,10 @@ class TestableLoginCommand extends Login { transportConnector: this.mockConnector, }) } + + public setInteractive(value: boolean): void { + this.interactive = value + } } // ==================== Tests ==================== @@ -122,7 +126,7 @@ describe('Login Command', () => { function mockOAuthFlow(startResponse: AuthStartLoginResponse, completion?: AuthLoginCompletedEvent): void { const onStub = mockClient.on as sinon.SinonStub onStub.callsFake((event: string, cb: (data: AuthLoginCompletedEvent) => void) => { - if (event === 'auth:loginCompleted' && completion) { + if (event === AuthEvents.LOGIN_COMPLETED && completion) { setImmediate(() => { cb(completion) }) @@ -130,9 +134,8 @@ describe('Login Command', () => { return () => {} }) - ;(mockClient.requestWithAck as sinon.SinonStub).callsFake((event: string) => { - if (event === 'auth:startLogin') return Promise.resolve(startResponse) + if (event === AuthEvents.START_LOGIN) return Promise.resolve(startResponse) return Promise.resolve({}) }) } @@ -156,7 +159,7 @@ describe('Login Command', () => { expect((mockClient.requestWithAck as sinon.SinonStub).calledOnce).to.be.true const [event, data] = (mockClient.requestWithAck as sinon.SinonStub).firstCall.args - expect(event).to.equal('auth:loginWithApiKey') + expect(event).to.equal(AuthEvents.LOGIN_WITH_API_KEY) expect(data).to.deep.equal({apiKey: 'my-secret-key'}) }) }) @@ -206,7 +209,7 @@ describe('Login Command', () => { expect(json.data).to.deep.include({error: 'Invalid API key'}) }) - it('should output JSON on connection error', async () => { + it('should output JSON with a user-friendly message on connection error', async () => { mockConnector.rejects(new NoInstanceRunningError()) await createJsonCommand('--api-key', 'test-key').run() @@ -214,7 +217,7 @@ describe('Login Command', () => { const json = parseJsonOutput() expect(json.command).to.equal('login') expect(json.success).to.be.false - expect(json.data).to.have.property('error') + expect(String(json.data.error ?? '')).to.include('Daemon failed to start automatically') }) it('should not log "Logging in..." in json mode', async () => { @@ -274,7 +277,7 @@ describe('Login Command', () => { await createCommand().run() const requestWithAckCalls = (mockClient.requestWithAck as sinon.SinonStub).getCalls() - expect(requestWithAckCalls.some((c) => c.args[0] === 'auth:startLogin')).to.be.true + expect(requestWithAckCalls.some((c) => c.args[0] === AuthEvents.START_LOGIN)).to.be.true expect(loggedMessages.some((m) => m.includes('Logged in as oauth@example.com'))).to.be.true }) @@ -290,10 +293,7 @@ describe('Login Command', () => { }) it('should print error message when LOGIN_COMPLETED reports failure', async () => { - mockOAuthFlow( - {authUrl: 'https://auth.byterover.dev/oauth'}, - {error: 'User denied access', success: false}, - ) + mockOAuthFlow({authUrl: 'https://auth.byterover.dev/oauth'}, {error: 'User denied access', success: false}) await createCommand().run() @@ -369,7 +369,7 @@ describe('Login Command', () => { describe('non-interactive shell', () => { it('should error with a pointer to --api-key when no flag and not a TTY', async () => { const command = createCommand() - command.interactive = false + command.setInteractive(false) await command.run() @@ -380,7 +380,7 @@ describe('Login Command', () => { it('should emit JSON error when non-interactive and no --api-key', async () => { const command = createJsonCommand() - command.interactive = false + command.setInteractive(false) await command.run() @@ -394,7 +394,7 @@ describe('Login Command', () => { mockLoginResponse({success: true, userEmail: 'ci@example.com'}) const command = createCommand('--api-key', 'ci-key') - command.interactive = false + command.setInteractive(false) await command.run() From 62ed9ac76a91ae25232b93371c7ea1a84963e525 Mon Sep 17 00:00:00 2001 From: wzlng Date: Thu, 23 Apr 2026 00:10:23 +0700 Subject: [PATCH 4/5] feat: [ENG-2354] avoid "Logged in as undefined" on OAuth success without user AuthLoginCompletedEvent.user is optional, so a {success: true} event without a user object used to print "Logged in as undefined" in text mode (and drop userEmail silently from the JSON envelope). emitSuccess now falls back to "Logged in successfully" when userEmail is absent. Added a regression test covering {success: true, user: undefined}. --- src/oclif/commands/login.ts | 2 +- test/commands/login.test.ts | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/oclif/commands/login.ts b/src/oclif/commands/login.ts index d11a01abc..c42f1a7e7 100644 --- a/src/oclif/commands/login.ts +++ b/src/oclif/commands/login.ts @@ -135,7 +135,7 @@ export default class Login extends Command { if (format === 'json') { writeJsonResponse({command: 'login', data: {userEmail}, success: true}) } else { - this.log(`Logged in as ${userEmail}`) + this.log(userEmail ? `Logged in as ${userEmail}` : 'Logged in successfully') } } diff --git a/test/commands/login.test.ts b/test/commands/login.test.ts index a066371a8..b059c9df5 100644 --- a/test/commands/login.test.ts +++ b/test/commands/login.test.ts @@ -333,6 +333,15 @@ describe('Login Command', () => { expect(json.data).to.deep.include({error: 'User denied access'}) }) + it('should display a fallback message when OAuth succeeds but user is absent', async () => { + mockOAuthFlow({authUrl: 'https://auth.byterover.dev/oauth'}, {success: true, user: undefined}) + + await createCommand().run() + + expect(loggedMessages.some((m) => m.includes('Logged in'))).to.be.true + expect(loggedMessages.some((m) => m.includes('undefined'))).to.be.false + }) + it('should clear the timeout and surface the error when START_LOGIN rejects', async () => { let timerFired = false ;(mockClient.on as sinon.SinonStub).returns(() => { From 4f71edf6db38ecb7f776dbf39739228619e8ba9f Mon Sep 17 00:00:00 2001 From: wzlng Date: Thu, 23 Apr 2026 10:30:01 +0700 Subject: [PATCH 5/5] fix: [ENG-2354] treat SSH sessions as non-browser for brv login Rename 'isInteractive()' to 'canOpenBrowser()' and reject sessions where SSH_CONNECTION, SSH_CLIENT, or SSH_TTY is set. TTY presence alone let 'brv login' ask the daemon to open a browser on a remote VPS that the user could never see; matches the detection used by sindresorhus/is-in-ssh. Updates the guard's error message and test overrides accordingly. --- src/oclif/commands/login.ts | 13 ++++-- test/commands/login.test.ts | 93 +++++++++++++++++++++++++++++++------ 2 files changed, 87 insertions(+), 19 deletions(-) diff --git a/src/oclif/commands/login.ts b/src/oclif/commands/login.ts index c42f1a7e7..c50a42b3c 100644 --- a/src/oclif/commands/login.ts +++ b/src/oclif/commands/login.ts @@ -45,8 +45,13 @@ export default class Login extends Command { }), } - protected isInteractive(): boolean { - return process.stdout.isTTY === true && process.stdin.isTTY === true + /** Gates the OAuth flow. DISPLAY/WAYLAND_DISPLAY deliberately not checked — unset on macOS/Windows, would false-positive. */ + protected canOpenBrowser(): boolean { + // Either stream not a TTY means piped/scripted/CI — no interactive user to complete OAuth. + if (process.stdout.isTTY !== true || process.stdin.isTTY !== true) return false + // SSH has a TTY but can't reach the user's local browser. + if (process.env.SSH_CONNECTION || process.env.SSH_CLIENT || process.env.SSH_TTY) return false + return true } protected async loginWithApiKey(apiKey: string, options?: DaemonClientOptions): Promise { @@ -103,10 +108,10 @@ export default class Login extends Command { const apiKey = flags['api-key'] const format: OutputFormat = flags.format === 'json' ? 'json' : 'text' - if (!apiKey && !this.isInteractive()) { + if (!apiKey && !this.canOpenBrowser()) { this.emitError( format, - 'Non-interactive shell detected. Use --api-key for headless login (get yours at https://app.byterover.dev/settings/keys).', + 'Cannot open a local browser here (non-interactive shell or SSH session). Use --api-key for headless login (get yours at https://app.byterover.dev/settings/keys).', ) return } diff --git a/test/commands/login.test.ts b/test/commands/login.test.ts index b059c9df5..c4f2fd73c 100644 --- a/test/commands/login.test.ts +++ b/test/commands/login.test.ts @@ -17,7 +17,7 @@ import { // ==================== TestableLoginCommand ==================== class TestableLoginCommand extends Login { - protected interactive = true + protected browserAvailable = true private readonly mockConnector: () => Promise constructor(argv: string[], mockConnector: () => Promise, config: Config) { @@ -25,8 +25,8 @@ class TestableLoginCommand extends Login { this.mockConnector = mockConnector } - protected override isInteractive(): boolean { - return this.interactive + protected override canOpenBrowser(): boolean { + return this.browserAvailable } protected override async loginWithApiKey(apiKey: string): Promise { @@ -47,8 +47,14 @@ class TestableLoginCommand extends Login { }) } - public setInteractive(value: boolean): void { - this.interactive = value + public setCanOpenBrowser(value: boolean): void { + this.browserAvailable = value + } +} + +class RealCheckLoginCommand extends Login { + public checkCanOpenBrowser(): boolean { + return this.canOpenBrowser() } } @@ -373,41 +379,98 @@ describe('Login Command', () => { }) }) - // ==================== Non-interactive shells ==================== + // ==================== Environments without a browser ==================== - describe('non-interactive shell', () => { - it('should error with a pointer to --api-key when no flag and not a TTY', async () => { + describe('environments without a browser', () => { + it('should error with a pointer to --api-key when no flag and browser is unavailable', async () => { const command = createCommand() - command.setInteractive(false) + command.setCanOpenBrowser(false) await command.run() - expect(loggedMessages.some((m) => m.toLowerCase().includes('non-interactive'))).to.be.true + expect(loggedMessages.some((m) => m.toLowerCase().includes('browser'))).to.be.true expect(loggedMessages.some((m) => m.includes('--api-key'))).to.be.true expect((mockClient.requestWithAck as sinon.SinonStub).called).to.be.false }) - it('should emit JSON error when non-interactive and no --api-key', async () => { + it('should emit JSON error when browser is unavailable and no --api-key', async () => { const command = createJsonCommand() - command.setInteractive(false) + command.setCanOpenBrowser(false) await command.run() const json = parseJsonOutput() expect(json.command).to.equal('login') expect(json.success).to.be.false - expect(String(json.data.error ?? '').toLowerCase()).to.include('non-interactive') + expect(String(json.data.error ?? '').toLowerCase()).to.include('browser') }) - it('should still perform api-key login when non-interactive and --api-key provided', async () => { + it('should still perform api-key login when browser is unavailable and --api-key provided', async () => { mockLoginResponse({success: true, userEmail: 'ci@example.com'}) const command = createCommand('--api-key', 'ci-key') - command.setInteractive(false) + command.setCanOpenBrowser(false) await command.run() expect(loggedMessages.some((m) => m.includes('Logged in as ci@example.com'))).to.be.true }) }) + + // ==================== canOpenBrowser() default implementation ==================== + + describe('canOpenBrowser default implementation', () => { + const sshVars = ['SSH_CONNECTION', 'SSH_CLIENT', 'SSH_TTY'] as const + const savedEnv: Partial> = {} + let stdinTtyDesc: PropertyDescriptor | undefined + let stdoutTtyDesc: PropertyDescriptor | undefined + + beforeEach(() => { + for (const v of sshVars) { + savedEnv[v] = process.env[v] + delete process.env[v] + } + + stdinTtyDesc = Object.getOwnPropertyDescriptor(process.stdin, 'isTTY') + stdoutTtyDesc = Object.getOwnPropertyDescriptor(process.stdout, 'isTTY') + Object.defineProperty(process.stdin, 'isTTY', {configurable: true, value: true, writable: true}) + Object.defineProperty(process.stdout, 'isTTY', {configurable: true, value: true, writable: true}) + }) + + afterEach(() => { + for (const v of sshVars) { + if (savedEnv[v] === undefined) delete process.env[v] + else process.env[v] = savedEnv[v] + } + + if (stdinTtyDesc) Object.defineProperty(process.stdin, 'isTTY', stdinTtyDesc) + else delete (process.stdin as unknown as Record).isTTY + if (stdoutTtyDesc) Object.defineProperty(process.stdout, 'isTTY', stdoutTtyDesc) + else delete (process.stdout as unknown as Record).isTTY + }) + + it('returns true with TTY and no SSH env', () => { + expect(new RealCheckLoginCommand([], config).checkCanOpenBrowser()).to.be.true + }) + + it('returns false without a TTY', () => { + Object.defineProperty(process.stdout, 'isTTY', {configurable: true, value: false, writable: true}) + expect(new RealCheckLoginCommand([], config).checkCanOpenBrowser()).to.be.false + }) + + it('returns false when SSH_CONNECTION is set', () => { + process.env.SSH_CONNECTION = '1.2.3.4 55555 5.6.7.8 22' + expect(new RealCheckLoginCommand([], config).checkCanOpenBrowser()).to.be.false + }) + + it('returns false when SSH_CLIENT is set', () => { + process.env.SSH_CLIENT = '1.2.3.4 55555 22' + expect(new RealCheckLoginCommand([], config).checkCanOpenBrowser()).to.be.false + }) + + it('returns false when SSH_TTY is set', () => { + process.env.SSH_TTY = '/dev/pts/0' + expect(new RealCheckLoginCommand([], config).checkCanOpenBrowser()).to.be.false + }) + }) })