Skip to content

Conversation

@DPTPhatUS
Copy link

@DPTPhatUS DPTPhatUS commented Nov 30, 2025

Description

Fixes a bug where the "Start Server" button fails to execute the command correctly on Linux systems.

Problem

In ServerManagementService.cs, the Linux terminal command construction wraps the script in redundant single quotes when building the bash -c argument:

// Before (broken)
string script = $"'{escapedCommandLinux}; exec bash'";
string bashCmdArgs = $"bash -c \"{escapedScriptForArg}\"";
// Results in: bash -c "'command; exec bash'"

The extra single quotes cause bash to interpret the entire command as a literal string rather than executing it.

Solution

Remove the redundant single quotes since the double quotes already properly delimit the argument for bash -c:

// After (fixed)
string script = $"{escapedCommandLinux}; exec bash";
string bashCmdArgs = $"bash -c \"{escapedScriptForArg}\"";
// Results in: bash -c "command; exec bash"

Testing

  • Tested on Linux (Ubuntu) with various terminal emulators (gnome-terminal, xterm)
  • Verified the server starts correctly after the fix

Checklist

  • Code follows project style guidelines
  • Changes are minimal and focused on the specific bug fix
  • Tested on Linux platform

Summary by CodeRabbit

  • Bug Fixes
    • Fixed terminal process execution on Linux by correcting how shell commands are formatted in the server management system, improving command interpretation and reliability.

✏️ Tip: You can customize this high-level summary in your review settings.

The bash -c command was wrapped with extra single quotes around the
script argument, causing the Start Server button to fail on Linux.

Before: bash -c "'command; exec bash'"
After:  bash -c "command; exec bash"

The double quotes are sufficient for bash -c to receive the command
as a single argument.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 30, 2025

Walkthrough

A single file modification to ServerManagementService.cs removes outer single-quote enclosure from a Linux terminal process command script construction, changing how the command string is passed to bash -c. Internal escaping remains intact; control flow is unchanged.

Changes

Cohort / File(s) Summary
Terminal process command quoting
MCPForUnity/Editor/Services/ServerManagementService.cs
Modified Linux branch in CreateTerminalProcessStartInfo: removed outer single-quote wrapping from script construction. Changed from '{escapedCommandLinux}; exec bash' to {escapedCommandLinux}; exec bash, altering shell command interpretation while preserving internal escaping.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Specific area for attention: Verify that removing the outer single quotes does not introduce shell injection vulnerabilities or unintended command parsing in the Linux terminal initialization path, especially given that internal escaping mechanisms remain in place.

Poem

🐰 A quote stripped clean from the command line,
No longer wrapped in quotes so fine,
The bash shall parse it, unadorned and true,
Escaped within, yet simpler too!
CodeRabbit hops through terminal affairs,
One line refined, beyond all cares. 🚀

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: Remove redundant single quotes in Linux terminal command' directly and clearly describes the main change in the pull request—removing unnecessary single quotes from Linux terminal command construction.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)

437-437: Consider updating single-quote escaping for consistency.

The '\\'' escape pattern on this line is specifically designed for escaping single quotes within single-quoted bash strings. Now that the script no longer wraps the command in single quotes (after the fix on line 439), this escaping idiom may not work as intended if the command ever contains single quotes.

For the current context (command passed to bash -c without surrounding quotes), the simpler \' escape would be more appropriate:

-string escapedCommandLinux = command.Replace("'", "'\\''");
+string escapedCommandLinux = command.Replace("'", "\\'");

However, since the command is constructed from controlled sources (uvx path, package name, HTTP URL) and is unlikely to contain single quotes in practice, this is not a pressing issue.

If you'd like to verify the escaping behavior with commands containing single quotes, I can generate a test script.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2126745 and 51262f8.

📒 Files selected for processing (1)
  • MCPForUnity/Editor/Services/ServerManagementService.cs (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.

Applied to files:

  • MCPForUnity/Editor/Services/ServerManagementService.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.

Applied to files:

  • MCPForUnity/Editor/Services/ServerManagementService.cs
🔇 Additional comments (1)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)

438-439: LGTM! Fix correctly removes redundant single quotes.

The removal of outer single quotes from the script construction correctly fixes the issue where bash was treating the entire command as a literal string instead of executing it. The change transforms bash -c "'command; exec bash'" into bash -c "command; exec bash", which allows bash to properly parse and execute the command.

The double quotes in the final construction (line 442) are consumed by the C# argument parser to group the script as a single argument to bash -c, while bash itself receives the unquoted command string to execute.

@dsarno
Copy link
Collaborator

dsarno commented Dec 2, 2025

Not clear what the value add is, and Coderabbit says it could be a security vulnerability.

@dsarno dsarno closed this Dec 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants