Feat/eng 2354 by wzlng · Pull Request #533 · campfirein/byterover-cli

wzlng · 2026-04-23T02:29:40Z

No description provided.

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.

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

- 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

…out 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}.

github-actions · 2026-04-23T02:29:57Z

Claude finished @wzlng's task in 2m 25s —— View job

PR Review: feat/ENG-2354

Gather context and read changed files
Analyze login.ts changes
Analyze provider registry and list changes
Analyze web UI changes
Analyze test coverage
Post review with inline comments and overall score

Summary

Overall score: 4/5

This is a well-executed feature. The OAuth-first login flow is cleanly decomposed, the race condition is handled correctly (subscribe-before-request), timer cleanup on error is thorough, and test coverage is comprehensive — including non-interactive shells, timeouts, and the "undefined user" edge case. The refactor to runApiKey/runOAuth + emitError/emitSuccess helpers makes the run() body readable.

Findings

Severity	Location	Issue
suggestion	`login.ts:114–116`	`catch` block re-implements `emitError` inline — simplify to one call (Fix this →)
issue	`login.ts:156–170`	`runOAuth` in JSON mode still triggers a browser open via `START_LOGIN`; `AuthStartLoginRequest.skipBrowserLaunch` exists on the daemon side and could make this headless-safe
nitpick	`login.ts:134–136`	`emitSuccess` in JSON mode emits `{userEmail: undefined}` — key is silently dropped by `JSON.stringify`; should omit or use `null`
nitpick	`login.test.ts:354–360`	150 ms sleep to detect unhandled rejections is timing-sensitive; prefer `process.on('unhandledRejection')`
nitpick	`login.test.ts:20`	`interactive` field on `TestableLoginCommand` is `protected` but it's purely test-scaffolding; `private` is sufficient

Strengths

Subscribe-before-request pattern prevents the LOGIN_COMPLETED callback race — correctly handled
Timer is cleared in both the success and error paths — no dangling timers
Non-interactive shell guard is clean and tested end-to-end
Provider description feature is consistent across CLI, web UI, and JSON output
TDD order appears to have been followed; tests are readable and targeted

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.

github-actions · 2026-04-23T03:31:09Z

Claude finished @wzlng's task in 2m 24s —— View job

PR Review: feat/ENG-2354

Summary

Overall score: 4/5

Well-structured PR. The OAuth-first login flow is cleanly decomposed (runApiKey/runOAuth + emitError/emitSuccess), the subscribe-before-request pattern correctly avoids the LOGIN_COMPLETED race, timer cleanup is thorough in all paths, and the SSH guard is both correct and tested end-to-end. The 5 commits show a methodical feature → polish → fix progression.

Findings

Severity	Location	Issue
suggestion	`login.ts:121–128`	`catch` block re-implements `emitError` inline — simplify to one call (Fix this →)
issue	`login.ts:169`	`runOAuth` in JSON mode still opens a browser — `skipBrowserLaunch` flag on daemon side could make this headless-safe (Fix this →)
nitpick	`login.ts:141`	`emitSuccess` in JSON mode emits `{userEmail: undefined}` — `JSON.stringify` silently drops the key; use `userEmail ? {userEmail} : {}`
nitpick	`login.test.ts:361–366`	150 ms sleep to detect timer cleanup is timing-sensitive; prefer `process.on('unhandledRejection')`
nitpick	`login.test.ts:20`	`browserAvailable` is `protected` but it's test-only scaffolding; `private` is sufficient
suggestion	`provider-select-step.tsx:22`	Search filter only matches `p.name` — now that descriptions are visible, also matching `p.description` would let users search by capability (e.g. "fast inference" → Groq)

Strengths

Subscribe-before-request pattern prevents LOGIN_COMPLETED race — correctly handled
Timer cleared in both success and error paths — no dangling timers
SSH guard (SSH_CONNECTION / SSH_CLIENT / SSH_TTY) is correct and well-motivated in the comment
Non-interactive shell guard is clean and tested end-to-end
emitSuccess text path gracefully handles missing user ('Logged in successfully' fallback)
Provider description is consistent across CLI output, web UI picker, and JSON responses
Test coverage is comprehensive: timeouts, SSH env vars, non-interactive shells, undefined user

cuongdo-byterover

lgtm

wzlng added 4 commits April 23, 2026 09:28

wzlng requested review from bao-byterover and leehpham as code owners April 23, 2026 02:29