Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the auth login flow to better support “fire-and-forget” browser launching and faster shutdown of the localhost OAuth callback server, and bumps the CLI beta version with small docs tweaks.
Changes:
- Replace
execFilewith detachedspawn(...).unref()for opening the system browser. - Force-close HTTP connections after OAuth callback responses (via
Connection: close+closeAllConnections?.()). - Bump package version to
1.5.0-beta.5and update installation notes in docs/README.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/services/auth/loginFlow.ts | Updates browser launch strategy and callback server connection handling. |
| package.json | Version bump for the beta release. |
| docs/mcp.md | Clarifies that @beta install is required for MCP support. |
| README.md | Same @beta install clarification in quick setup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/services/auth/loginFlow.ts
Outdated
| const spawnOpts = { detached: true, stdio: 'ignore' as const }; | ||
| let child; | ||
| switch (process.platform) { |
There was a problem hiding this comment.
let child; creates an implicit any under the repo’s strict TS config, which will fail compilation. Please give this a concrete type (e.g., ChildProcess) or restructure to assign a typed const child = ... from a helper so the switch still works without any.
| execFile('xdg-open', [url]); | ||
| child = spawn('xdg-open', [url], spawnOpts); | ||
| break; | ||
| } |
There was a problem hiding this comment.
The spawned child process can emit an error event (e.g., xdg-open/open/powershell.exe not found). With no error listener, Node will treat it as an unhandled EventEmitter error and crash the CLI. Add a no-op/error-logging handler (or handle ENOENT explicitly) before calling unref().
| } | |
| } | |
| child.on('error', (error) => { | |
| console.error(`Failed to open browser automatically: ${error.message}`); | |
| }); |
src/services/auth/loginFlow.ts
Outdated
| export function openBrowser(url: string): void { | ||
| // detached:true + stdio:'ignore' + unref() is the standard Node.js pattern for | ||
| // fire-and-forget child processes — the event loop will not wait for them to exit. | ||
| const spawnOpts = { detached: true, stdio: 'ignore' as const }; | ||
| let child; | ||
| switch (process.platform) { | ||
| case 'darwin': | ||
| execFile('open', [url]); | ||
| child = spawn('open', [url], spawnOpts); | ||
| break; | ||
| case 'win32': | ||
| // Pass the URL via $args[0] so it is never interpolated into the -Command | ||
| // string — avoids quote-breaking and injection risk from special characters. | ||
| execFile('powershell.exe', ['-NoProfile', '-Command', 'Start-Process $args[0]', '-args', url]); | ||
| child = spawn('powershell.exe', ['-NoProfile', '-Command', 'Start-Process $args[0]', '-args', url], spawnOpts); | ||
| break; | ||
| default: | ||
| execFile('xdg-open', [url]); | ||
| child = spawn('xdg-open', [url], spawnOpts); | ||
| break; | ||
| } | ||
| child.unref(); | ||
| } |
There was a problem hiding this comment.
openBrowser behavior changed (detached + stdio ignore + unref + platform-specific spawn args), but there are currently unit tests for other loginFlow functions and none covering this. Consider adding a unit test that asserts the correct command/args/options are selected per platform (which may require routing process spawning through a stub-friendly indirection similar to loginFlowClient).
- Add explicit ChildProcess type to suppress implicit-any lint warning - Add no-op error handler to prevent unhandled EventEmitter crash on ENOENT - Extract getBrowserCommand() pure function for testability - Add unit tests covering all platforms and URL passthrough Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
No description provided.