fix(auth): update browser command arguments for Windows and clean URL… - #55
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesCross-platform process launching
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/auth/auth-manager.ts`:
- Line 22: Update the Windows launch flow around execFile to resolve
%SystemRoot%\System32\rundll32.exe and pass that absolute path instead of the
bare executable name. Handle a missing SystemRoot by logging the launch failure
and returning before invoking execFile, and update both affected Windows test
assertions to expect the resolved path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a73a00bc-0bc4-4967-a657-fdd9be6a402a
📒 Files selected for processing (3)
packages/core/src/auth/auth-manager.tspackages/core/test/integration/auth/auth-manager.test.tspackages/core/test/unit/auth/open-browser.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mcp-wrapper.js`:
- Around line 118-119: Update the npx launch logic in scripts/mcp-wrapper.js at
lines 118-119 to invoke cmd.exe on Windows with argument-safe quoting,
preserving boundaries for the URL and header values; keep direct npx execution
on non-Windows platforms. Apply the same generated-launcher change in
scripts/plugin-generators.mjs at lines 262-263, and add a Windows test covering
a URL with query parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 806e3e29-cb15-4131-b0ce-14a0a11517cf
📒 Files selected for processing (2)
scripts/mcp-wrapper.jsscripts/plugin-generators.mjs
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| const child = spawn(npxBin, ['-y', 'mcp-remote@0.1.38', url, ...headerArgs, '--transport', 'http-first', '--silent'], { | ||
| stdio: 'inherit', | ||
| env: process.env, | ||
| shell: isWin, |
There was a problem hiding this comment.
Setting shell:true on Windows runs this through cmd.exe /c, so the url (and each --header value) is pasted back into a shell command line instead of staying a separate argv element — which is the exact class of bug the rundll32 change below fixes. Node quotes the args so & is fine, but cmd still expands %VAR% even inside quotes, so a percent-encoded URL (%20, %2F) or an env-looking token in a header value will get mangled. Can we avoid passing the url/headers through the shell?
| if (process.platform === 'win32') { | ||
| // Use ShellExecute directly instead of cmd /c start to avoid & being | ||
| // interpreted as a command separator on Windows. | ||
| execFile('rundll32', ['url.dll,FileProtocolHandler', url], (err) => { |
There was a problem hiding this comment.
rundll32 returns 0 and exits almost immediately even when the FileProtocolHandler can't open anything (no default browser, etc.), so this err branch will essentially never fire and launch failures go silently unlogged on Windows — unlike open/xdg-open which do surface real child-process errors. Is silent failure acceptable here, or should we at least try to detect it?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/test/unit/mcp/mcp-wrapper.test.ts`:
- Around line 77-88: Update the “wrapper preserves argv boundaries outside
Windows” test to skip execution on Windows, since its fixture only creates the
Unix npx executable while Windows resolves npx.cmd. Preserve the existing
assertions and behavior for non-Windows platforms.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2a7095c-22fd-4b86-987c-69f77d309604
📒 Files selected for processing (6)
packages/core/src/auth/auth-manager.tspackages/core/test/integration/auth/auth-manager.test.tspackages/core/test/unit/auth/open-browser.test.tspackages/core/test/unit/mcp/mcp-wrapper.test.tsscripts/mcp-wrapper.jsscripts/plugin-generators.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/test/integration/auth/auth-manager.test.ts
Skip the POSIX npx fixture on Windows and inject the invalid SystemRoot only after Node startup so the wrapper, rather than Node initialization, is under test.
4 participants
… in tests
Summary by CodeRabbit