Improve python bridge log destination#1072
Conversation
…lows the OS overall structure
📝 WalkthroughWalkthroughThe changes introduce platform-aware log directory resolution with environment variable override support. A new utility module provides Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/README.md`:
- Line 150: Update the README entry for UNITY_MCP_LOG_DIR to use the project's
shortened macOS path convention by replacing the macOS default path
`~/Library/Application Support/UnityMCP/Logs` with
`~/Library/AppSupport/UnityMCP/Logs`; ensure this matches how ServerInstaller.cs
creates the `~/Library/AppSupport` symlink so user-facing docs and the installer
use the same macOS path form.
In `@Server/src/utils/log_paths.py`:
- Around line 31-44: The function that returns the log directory (the code
handling UNITY_MCP_LOG_DIR override and the XDG_STATE_HOME fallback in
log_paths.py) must guarantee an absolute path; fix it by expanding user
variables then converting to an absolute path before returning (i.e., apply
os.path.expanduser followed by os.path.abspath to the override value and to the
computed XDG_STATE_HOME/base path), and do the same for the Windows/macOS paths
to be consistent so get_log_dir always returns an absolute path.
In `@Server/tests/test_log_paths.py`:
- Around line 12-40: The tests in TestEnvOverride use hardcoded "/tmp" and "~"
paths which trigger Ruff S108; update the four affected tests to accept the
pytest tmp_path fixture and use tmp_path (e.g., str(tmp_path / "custom-logs"))
instead of "/tmp/custom-logs", and for the expand-user case use tmp_path /
"my-logs" and compare against os.path.expanduser(str(tmp_path / "my-logs"))
(ensure os is imported); keep the same env keys passed to resolve_log_dir
(UNITY_MCP_LOG_DIR and the extras like LOCALAPPDATA/XDG_STATE_HOME) but set
their values to the tmp_path-derived strings so tests remain platform-neutral
and lint-clean.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d621081c-a3d9-4438-9e34-54496211ce89
📒 Files selected for processing (4)
Server/README.mdServer/src/main.pyServer/src/utils/log_paths.pyServer/tests/test_log_paths.py
| - `UNITY_MCP_HTTP_REMOTE_HOSTED` - Enable remote-hosted mode (`true`, `1`, or `yes`) | ||
| - `UNITY_MCP_DEFAULT_INSTANCE` - Default Unity instance to target (project name, hash, or `Name@hash`) | ||
| - `UNITY_MCP_SKIP_STARTUP_CONNECT=1` - Skip initial Unity connection attempt on startup | ||
| - `UNITY_MCP_LOG_DIR` - Override the rotating server log directory. Default: `%LOCALAPPDATA%\UnityMCP\Logs` (Windows), `~/Library/Application Support/UnityMCP/Logs` (macOS), `$XDG_STATE_HOME/UnityMCP/Logs` (Linux/BSD, defaults to `~/.local/state/UnityMCP/Logs`). |
There was a problem hiding this comment.
Use the project’s shortened macOS path convention.
The macOS default should be documented with ~/Library/AppSupport/... to avoid reintroducing a path with spaces in user-facing setup guidance.
Proposed docs update
-- `UNITY_MCP_LOG_DIR` - Override the rotating server log directory. Default: `%LOCALAPPDATA%\UnityMCP\Logs` (Windows), `~/Library/Application Support/UnityMCP/Logs` (macOS), `$XDG_STATE_HOME/UnityMCP/Logs` (Linux/BSD, defaults to `~/.local/state/UnityMCP/Logs`).
+- `UNITY_MCP_LOG_DIR` - Override the rotating server log directory. Default: `%LOCALAPPDATA%\UnityMCP\Logs` (Windows), `~/Library/AppSupport/UnityMCP/Logs` (macOS), `$XDG_STATE_HOME/UnityMCP/Logs` (Linux/BSD, defaults to `~/.local/state/UnityMCP/Logs`).Based on learnings, ServerInstaller.cs creates ~/Library/AppSupport as a symlink to avoid spaces in macOS paths, and README documentation should use ~/Library/AppSupport rather than ~/Library/Application Support.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/README.md` at line 150, Update the README entry for UNITY_MCP_LOG_DIR
to use the project's shortened macOS path convention by replacing the macOS
default path `~/Library/Application Support/UnityMCP/Logs` with
`~/Library/AppSupport/UnityMCP/Logs`; ensure this matches how ServerInstaller.cs
creates the `~/Library/AppSupport` symlink so user-facing docs and the installer
use the same macOS path form.
| override = env.get("UNITY_MCP_LOG_DIR") | ||
| if override: | ||
| return os.path.expanduser(override) | ||
|
|
||
| if platform == "darwin": | ||
| return os.path.expanduser("~/Library/Application Support/UnityMCP/Logs") | ||
|
|
||
| if platform == "win32": | ||
| base = env.get("LOCALAPPDATA") or os.path.expanduser("~/AppData/Local") | ||
| return os.path.join(base, "UnityMCP", "Logs") | ||
|
|
||
| # Linux/BSD and anything else: XDG_STATE_HOME per freedesktop.org basedir spec. | ||
| base = env.get("XDG_STATE_HOME") or os.path.expanduser("~/.local/state") | ||
| return os.path.join(base, "UnityMCP", "Logs") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import os
print(os.path.expanduser("logs"))
print(os.path.isabs(os.path.expanduser("logs")))
PYRepository: CoplayDev/unity-mcp
Length of output: 73
🏁 Script executed:
# First, let's examine the complete log_paths.py file and understand the function signature
cat -n Server/src/utils/log_paths.pyRepository: CoplayDev/unity-mcp
Length of output: 1683
🏁 Script executed:
# Check how resolve_log_dir is used in the codebase
rg "resolve_log_dir" -A 3 -B 3Repository: CoplayDev/unity-mcp
Length of output: 6351
🏁 Script executed:
# Verify if there are any tests for log_paths.py
fd -e py -e test | xargs grep -l "log_paths\|resolve_log_dir" 2>/dev/nullRepository: CoplayDev/unity-mcp
Length of output: 142
Ensure all log paths are absolute per function contract.
The docstring (line 24) promises an absolute path, but os.path.expanduser() does not make relative paths absolute. If UNITY_MCP_LOG_DIR or XDG_STATE_HOME contain relative values (e.g., logs or state), the function returns a relative path, causing logs to be written relative to the current working directory. In stdio/MCP contexts where cwd varies, this breaks log discoverability. The hardcoded macOS and Windows fallback paths are safe because they either start with ~ or use environment variables expected to be absolute, but the override and XDG_STATE_HOME paths require fixing.
Proposed fix
+def _expand_abs(path: str) -> str:
+ return os.path.abspath(os.path.expanduser(path))
+
+
def resolve_log_dir(
@@
override = env.get("UNITY_MCP_LOG_DIR")
if override:
- return os.path.expanduser(override)
+ return _expand_abs(override)
@@
- base = env.get("XDG_STATE_HOME") or os.path.expanduser("~/.local/state")
+ base = env.get("XDG_STATE_HOME")
+ if not base or not os.path.isabs(os.path.expanduser(base)):
+ base = "~/.local/state"
+ base = _expand_abs(base)
return os.path.join(base, "UnityMCP", "Logs")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/src/utils/log_paths.py` around lines 31 - 44, The function that
returns the log directory (the code handling UNITY_MCP_LOG_DIR override and the
XDG_STATE_HOME fallback in log_paths.py) must guarantee an absolute path; fix it
by expanding user variables then converting to an absolute path before returning
(i.e., apply os.path.expanduser followed by os.path.abspath to the override
value and to the computed XDG_STATE_HOME/base path), and do the same for the
Windows/macOS paths to be consistent so get_log_dir always returns an absolute
path.
| class TestEnvOverride: | ||
| def test_override_wins_on_macos(self): | ||
| path = resolve_log_dir( | ||
| platform="darwin", | ||
| env={"UNITY_MCP_LOG_DIR": "/tmp/custom-logs"}, | ||
| ) | ||
| assert path == "/tmp/custom-logs" | ||
|
|
||
| def test_override_wins_on_windows(self): | ||
| path = resolve_log_dir( | ||
| platform="win32", | ||
| env={"UNITY_MCP_LOG_DIR": "/tmp/custom-logs", "LOCALAPPDATA": r"C:\ignored"}, | ||
| ) | ||
| assert path == "/tmp/custom-logs" | ||
|
|
||
| def test_override_wins_on_linux(self): | ||
| path = resolve_log_dir( | ||
| platform="linux", | ||
| env={"UNITY_MCP_LOG_DIR": "/tmp/custom-logs", "XDG_STATE_HOME": "/ignored"}, | ||
| ) | ||
| assert path == "/tmp/custom-logs" | ||
|
|
||
| def test_override_expands_user(self): | ||
| path = resolve_log_dir( | ||
| platform="linux", | ||
| env={"UNITY_MCP_LOG_DIR": "~/my-logs"}, | ||
| ) | ||
| assert path == os.path.expanduser("~/my-logs") | ||
| assert "~" not in path |
There was a problem hiding this comment.
Avoid hardcoded /tmp paths flagged by Ruff.
Ruff reports S108 on these test literals. Use tmp_path so lint stays green and the tests remain platform-neutral.
Proposed test update
- def test_override_wins_on_macos(self):
+ def test_override_wins_on_macos(self, tmp_path):
+ custom_logs = str(tmp_path / "custom-logs")
path = resolve_log_dir(
platform="darwin",
- env={"UNITY_MCP_LOG_DIR": "/tmp/custom-logs"},
+ env={"UNITY_MCP_LOG_DIR": custom_logs},
)
- assert path == "/tmp/custom-logs"
+ assert path == custom_logs
- def test_override_wins_on_windows(self):
+ def test_override_wins_on_windows(self, tmp_path):
+ custom_logs = str(tmp_path / "custom-logs")
path = resolve_log_dir(
platform="win32",
- env={"UNITY_MCP_LOG_DIR": "/tmp/custom-logs", "LOCALAPPDATA": r"C:\ignored"},
+ env={"UNITY_MCP_LOG_DIR": custom_logs, "LOCALAPPDATA": r"C:\ignored"},
)
- assert path == "/tmp/custom-logs"
+ assert path == custom_logs
- def test_override_wins_on_linux(self):
+ def test_override_wins_on_linux(self, tmp_path):
+ custom_logs = str(tmp_path / "custom-logs")
path = resolve_log_dir(
platform="linux",
- env={"UNITY_MCP_LOG_DIR": "/tmp/custom-logs", "XDG_STATE_HOME": "/ignored"},
+ env={"UNITY_MCP_LOG_DIR": custom_logs, "XDG_STATE_HOME": "/ignored"},
)
- assert path == "/tmp/custom-logs"
+ assert path == custom_logs
@@
- def test_no_args_reads_live_environment(self, monkeypatch):
+ def test_no_args_reads_live_environment(self, monkeypatch, tmp_path):
"""With no args, should read sys.platform and os.environ directly."""
- monkeypatch.setenv("UNITY_MCP_LOG_DIR", "/tmp/live-env-test")
- assert resolve_log_dir() == "/tmp/live-env-test"
+ live_env_path = str(tmp_path / "live-env-test")
+ monkeypatch.setenv("UNITY_MCP_LOG_DIR", live_env_path)
+ assert resolve_log_dir() == live_env_pathAlso applies to: 93-97
🧰 Tools
🪛 Ruff (0.15.10)
[error] 16-16: Probable insecure usage of temporary file or directory: "/tmp/custom-logs"
(S108)
[error] 18-18: Probable insecure usage of temporary file or directory: "/tmp/custom-logs"
(S108)
[error] 23-23: Probable insecure usage of temporary file or directory: "/tmp/custom-logs"
(S108)
[error] 25-25: Probable insecure usage of temporary file or directory: "/tmp/custom-logs"
(S108)
[error] 30-30: Probable insecure usage of temporary file or directory: "/tmp/custom-logs"
(S108)
[error] 32-32: Probable insecure usage of temporary file or directory: "/tmp/custom-logs"
(S108)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/tests/test_log_paths.py` around lines 12 - 40, The tests in
TestEnvOverride use hardcoded "/tmp" and "~" paths which trigger Ruff S108;
update the four affected tests to accept the pytest tmp_path fixture and use
tmp_path (e.g., str(tmp_path / "custom-logs")) instead of "/tmp/custom-logs",
and for the expand-user case use tmp_path / "my-logs" and compare against
os.path.expanduser(str(tmp_path / "my-logs")) (ensure os is imported); keep the
same env keys passed to resolve_log_dir (UNITY_MCP_LOG_DIR and the extras like
LOCALAPPDATA/XDG_STATE_HOME) but set their values to the tmp_path-derived
strings so tests remain platform-neutral and lint-clean.
Description
There was a hardcoded path to store the logs from the python bridge used by stdio, that resolved to a MacOS-like path. This change allows for this path to be customized by the User and also defaults it to different paths depending on the OS
Type of Change
Bug fix (non-breaking change)
Changes Made
Changed logging setup in Server/src/main.py
Testing/Screenshots/Recordings
Not applicable
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdI have updated README.md to include the new env var
Related Issues
None
Additional Notes
None
Summary by CodeRabbit
New Features
UNITY_MCP_LOG_DIRenvironment variable.Documentation