-
Notifications
You must be signed in to change notification settings - Fork 771
Fix/windows bundle paths tested #529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughUpdates deploy command path construction to use pathlib join, refactors Wrangler wrapper to centralize env updates, introduce Windows-specific npm prefix handling and shell execution, and minor test assertion reformatting without logic changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant D as Deploy CLI
participant W as WranglerWrapper
participant P as subprocess.run
participant WR as Wrangler (npx)
U->>D: invoke deploy
D->>W: prepare and run wrangler deploy
Note over W: Build env_updates (API keys, etc.)
alt Windows
W->>W: Set npm_config_prefix to cache\\npm-global
W->>P: run(cmd, env, shell=True)
else Non-Windows
W->>P: run(cmd, env, shell=False)
end
P->>WR: execute "npx wrangler deploy ..."
WR-->>P: exit code / output
alt error
P-->>W: non-zero return
W->>W: _handle_wrangler_error(...)
else success
P-->>W: success
end
W-->>D: result
D-->>U: deployment status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
148-156
: Consider adding error handling for npm prefix directory creation.The npm-global directory creation uses
mkdir(parents=True, exist_ok=True)
which should handle most cases, but filesystem permission issues could cause silent failures.Consider wrapping the directory creation in a try-except to provide clearer error messages:
if os.name == "nt": # On Windows, configure npm to use a safe prefix within our cache directory # to avoid issues with missing global npm directories npm_prefix = ( Path(os.path.expanduser(settings.DEPLOYMENT_CACHE_DIR)) / "npm-global" ) - npm_prefix.mkdir(parents=True, exist_ok=True) + try: + npm_prefix.mkdir(parents=True, exist_ok=True) + except OSError as e: + raise CLIError(f"Failed to create npm prefix directory on Windows: {e}") from e env_updates["npm_config_prefix"] = str(npm_prefix)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mcp_agent/cli/cloud/commands/deploy/main.py
(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py
(2 hunks)tests/cli/commands/test_deploy_command.py
(1 hunks)tests/cli/commands/test_wrangler_wrapper.py
(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/cli/commands/test_wrangler_wrapper.py (1)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
_needs_requirements_modification
(35-41)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
src/mcp_agent/cli/cloud/commands/deploy/settings.py (3)
wrangler_auth_domain
(24-26)wrangler_auth_url
(29-31)cloudflare_api_base_url
(34-36)
🔇 Additional comments (5)
tests/cli/commands/test_deploy_command.py (1)
455-457
: LGTM! Formatting update only.The assertion has been reformatted to a single line with the message wrapped in parentheses. This is purely a stylistic change with no functional impact.
tests/cli/commands/test_wrangler_wrapper.py (1)
473-475
: LGTM! Consistent formatting improvements.All assertion statements have been reformatted to single-line format with messages in parentheses. These are purely stylistic changes with no impact on test behavior or coverage.
Also applies to: 547-549, 572-577, 653-663, 677-685, 728-730, 846-848, 876-878, 884-886, 953-955, 961-975, 977-983, 994-996, 1000-1002
src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
278-278
: Excellent path construction improvement.Switching from string formatting to pathlib's
/
operator is more idiomatic and ensures correct path separator handling across Windows and Unix systems. This change aligns with the PR's Windows compatibility goals.src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (2)
136-157
: Good refactor for Windows compatibility.The env_updates structure centralizes environment variable configuration and defers application, making the Windows-specific npm prefix handling cleaner. The Windows npm prefix configuration addresses a common issue where global npm directories may not exist.
315-337
: Confirm shell injection mitigation for app_id parameter.The
shell=True
flag on Windows (line 334) is necessary fornpx
to work, but introduces shell injection risk. Whileapp_id
comes from the API backend (mcp_app_client.create_app()
at line 210-214 in main.py) rather than direct user input, defense-in-depth is recommended.Two mitigation options:
- Use
shlex.quote()
on POSIX (already imported elsewhere in the codebase):import shlex # Quote app_id on POSIX systems quoted_app_id = shlex.quote(app_id) if os.name != "nt" else app_id
- Avoid
shell=True
entirely by usingnpx.cmd
explicitly on Windows:npx_cmd = "npx.cmd" if os.name == "nt" else "npx" cmd = [npx_cmd, "--yes", "[email protected]", ...] # Then use shell=FalseOption 2 is preferred as it eliminates the shell entirely. Consider also documenting that
app_id
format is controlled by the backend API.
Description
Spun up a windows VM to test mcp-agent deploy and found a couple more issues:
shell
for thesubprocess.run
and point to our cache dir instead of expecting some global npm pathTesting
Spun up windows VM and deployed successfully:
Also deployed successfully from mac (
uv run mcp-agent deploy MCPAgentServerTemporal -c examples/mcp_agent_server/temporal
)Summary by CodeRabbit
Bug Fixes
Tests
Refactor