diff --git a/src/oclif/commands/login.ts b/src/oclif/commands/login.ts index 888e51969..c50a42b3c 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,15 @@ export default class Login extends Command { }), } + /** 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 { return withDaemonRetry( async (client) => client.requestWithAck(AuthEvents.LOGIN_WITH_API_KEY, {apiKey}), @@ -32,40 +61,117 @@ 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?.() + 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) + timer = undefined + } + + 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) + timer = undefined + } + + 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' + + if (!apiKey && !this.canOpenBrowser()) { + this.emitError( + format, + '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 + } try { - if (format === 'text') { - this.log('Logging in...') + await (apiKey ? this.runApiKey(apiKey, format) : this.runOAuth(format)) + } catch (error) { + const message = formatConnectionError(error) + if (format === 'json') { + this.emitError(format, message) + } else { + this.log(message) } + } + } - const response = await this.loginWithApiKey(apiKey) + private emitError(format: OutputFormat, message: string): void { + if (format === 'json') { + writeJsonResponse({command: 'login', data: {error: message}, success: false}) + } else { + this.log(message) + } + } - 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) - } - } - } catch (error) { - const errorMessage = error instanceof Error ? error.message : 'Login failed' + private emitSuccess(format: OutputFormat, userEmail: string | undefined): void { + if (format === 'json') { + writeJsonResponse({command: 'login', data: {userEmail}, success: true}) + } else { + this.log(userEmail ? `Logged in as ${userEmail}` : 'Logged in successfully') + } + } - if (format === 'json') { - writeJsonResponse({command: 'login', data: {error: errorMessage}, success: false}) - } else { - this.log(formatConnectionError(error)) + 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) { + this.emitSuccess(format, result.user?.email) + } else { + this.emitError(format, result.error ?? 'Authentication failed') + } } } 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}
+
Promise constructor(argv: string[], mockConnector: () => Promise, config: Config) { @@ -20,6 +25,10 @@ class TestableLoginCommand extends Login { this.mockConnector = mockConnector } + protected override canOpenBrowser(): boolean { + return this.browserAvailable + } + protected override async loginWithApiKey(apiKey: string): Promise { return super.loginWithApiKey(apiKey, { maxRetries: 1, @@ -27,6 +36,26 @@ 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, + }) + } + + public setCanOpenBrowser(value: boolean): void { + this.browserAvailable = value + } +} + +class RealCheckLoginCommand extends Login { + public checkCanOpenBrowser(): boolean { + return this.canOpenBrowser() + } } // ==================== Tests ==================== @@ -100,6 +129,23 @@ 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 === AuthEvents.LOGIN_COMPLETED && completion) { + setImmediate(() => { + cb(completion) + }) + } + + return () => {} + }) + ;(mockClient.requestWithAck as sinon.SinonStub).callsFake((event: string) => { + if (event === AuthEvents.START_LOGIN) return Promise.resolve(startResponse) + return Promise.resolve({}) + }) + } + // ==================== Successful Login ==================== describe('successful login', () => { @@ -119,7 +165,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'}) }) }) @@ -169,7 +215,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() @@ -177,7 +223,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 () => { @@ -224,4 +270,207 @@ 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] === AuthEvents.START_LOGIN)).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 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(() => { + /* 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 + }) + }) + + // ==================== Environments without a browser ==================== + + 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.setCanOpenBrowser(false) + + await command.run() + + 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 browser is unavailable and no --api-key', async () => { + const command = createJsonCommand() + 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('browser') + }) + + 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.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 + }) + }) }) diff --git a/test/commands/providers/list.test.ts b/test/commands/providers/list.test.ts index 6ad7cd6ec..92306d19c 100644 --- a/test/commands/providers/list.test.ts +++ b/test/commands/providers/list.test.ts @@ -145,6 +145,37 @@ describe('Provider List Command', () => { 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 ====================