Conversation
Relay Server の add_signal_handler NotImplementedError を修正し、 Unity Editor のプロセス管理・PATH 解決を Windows 対応に改善。 - relay/server.py: シグナルハンドラを win32 ガード、stop() 冪等化、 ログディレクトリを LOCALAPPDATA フォールバック対応、 get_event_loop() → get_running_loop() - RelayServerLauncher.cs: KillProcess を Process.Kill() に置換、 GetProcessIdForPort を netstat/lsof プラットフォーム分岐、 GetPlatformSearchPaths() で Windows Python パス動的走査 - completion.py: PowerShell 補完スクリプト追加、シェル自動検出改善 - test_relay_logging.py: ハードコード /tmp → tmp_path、OS 非依存化 - hub/paths.py: ProgramFiles(x86) の Hub パス候補追加 Closes #89, Closes #88
|
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:
📝 WalkthroughWalkthroughUnityBridge 側でプラットフォーム別の実行ファイル/PATH検出、ポート→PID 検出、プロセス終了処理を導入。Relay サーバーは Windows 非対応箇所を回避するシグナル処理と冪等な停止処理、ログディレクトリ解決をプラットフォーム対応に変更した。 Changes
Sequence Diagram(s)sequenceDiagram
participant UnityEditor as Unity Editor (C#)
participant OS as OS / Shell
participant Relay as Relay Server (Python)
UnityEditor->>OS: GetPlatformSearchPaths()
OS-->>UnityEditor: platform-specific dirs
UnityEditor->>OS: BuildAugmentedPath(original PATH, dirs)
UnityEditor->>OS: FindExecutable(name)
OS-->>UnityEditor: executable path or not found
UnityEditor->>OS: GetProcessIdForPort(port)
OS-->>UnityEditor: PID (netstat or lsof) or -1
alt PID found
UnityEditor->>OS: KillProcess(pid)
OS-->>UnityEditor: termination result
end
UnityEditor->>Relay: Launch relay process
Relay->>Relay: run_server()
alt Non-Windows
Relay->>OS: loop.add_signal_handler(SIGINT/SIGTERM) -> server.stop()
else Windows
OS->>Relay: KeyboardInterrupt -> server.stop()
end
Relay->>Relay: stop() (acquire _stop_lock, cleanup guarded)
Relay-->>UnityEditor: shutdown complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![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
🧹 Nitpick comments (4)
UnityBridge/Editor/RelayServerLauncher.cs (2)
250-278:lsofのパスがハードコードされている点について。
/usr/sbin/lsofはmacOS では正しいですが、一部の Linux ディストリビューションでは/usr/bin/lsofに配置されています。現在 Unity Editor の主要ターゲットが macOS であれば問題ありませんが、Linux Editor サポートを強化する場合はwhich lsofや PATH 検索のフォールバックが必要になる可能性があります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UnityBridge/Editor/RelayServerLauncher.cs` around lines 250 - 278, GetProcessIdForPortUnix currently hardcodes FileName = "/usr/sbin/lsof", which can fail on Linux where lsof may live in /usr/bin; update GetProcessIdForPortUnix to resolve the lsof executable from PATH (e.g., try Environment.GetEnvironmentVariable("PATH") lookup or call "which lsof" / use System.IO.Path search) before creating the ProcessStartInfo (the psi variable), and fall back to common locations (/usr/sbin/lsof, /usr/bin/lsof) or return -1 with a logged warning if none found; ensure you only change how psi.FileName is set and keep Arguments and other ProcessStartInfo settings the same.
302-351: Windows.exe拡張子のフォールバック検索は適切ですが、.cmd拡張子のケースも検討してください。一部のツール（例: npm 経由でインストールされたもの）は
.cmdラッパーとして提供されることがあります。uvとpythonの検索には.exeで十分ですが、将来的に他のツールの検索を追加する場合は考慮が必要です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UnityBridge/Editor/RelayServerLauncher.cs` around lines 302 - 351, The FindExecutable method currently falls back to checking ".exe" on Windows but misses ".cmd" wrappers; update FindExecutable to also check for a ".cmd" variant when Application.platform == RuntimePlatform.WindowsEditor in both the platform-specific searchPaths loop and the PATH loop (i.e., after checking fullPath, check fullPath + ".exe" and fullPath + ".cmd"), ensuring the method returns the first existing match found for either extension.unity_cli/cli/commands/completion.py (1)
67-67:import sysの重複。Line 67 で
import sysが実行済みのため、line 85 のimport sysは冗長です。♻️ 重複 import の削除
shell = shell.lower() if shell not in _COMPLETION_SCRIPTS: - import sys - if is_no_color():Also applies to: 85-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unity_cli/cli/commands/completion.py` at line 67, Remove the redundant second "import sys" in completion.py (the duplicate import at the later occurrence) so only the first import sys remains; locate the duplicate import statement (the second plain "import sys") and delete it to avoid the redundant import.tests/test_relay_logging.py (1)
61-66: スキップ方法の不一致（軽微）。
test_default_path_unixではランタイムのpytest.skip()を使用していますが、Windows 用テスト（line 68, 75）では@pytest.mark.skipifデコレータを使用しています。一貫性のために@pytest.mark.skipifに統一すると、テスト収集時にスキップ理由が明確になります。♻️ デコレータベースのスキップへの統一案
+ `@pytest.mark.skipif`(sys.platform == "win32", reason="Unix-specific test") def test_default_path_unix(self, monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.delenv("XDG_STATE_HOME", raising=False) - if sys.platform == "win32": - pytest.skip("Unix-specific test") log_dir = _resolve_log_dir() assert str(log_dir).endswith(".local/state/unity-cli/logs")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_relay_logging.py` around lines 61 - 66, The test uses a runtime pytest.skip() inside test_default_path_unix while other Windows-only tests use the `@pytest.mark.skipif` decorator; change test_default_path_unix to use `@pytest.mark.skipif`(sys.platform == "win32", reason="Unix-specific test") so skipping is applied at collection time and consistent with the other tests (locate the test_default_path_unix function in tests/test_relay_logging.py and keep the existing monkeypatch and _resolve_log_dir assertion intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@unity_cli/cli/commands/completion.py`:
- Around line 33-45: The PowerShell completion script sets the wrong environment
variable pattern; update the assignment of $env:_U_COMPLETE in the "powershell"
completion block to use the Typer/Click convention (e.g. "powershell_complete"
or "powershell_source") instead of "complete_powershell" so the completion hook
matches Typer 0.15.0 expectations; change the value in the powershell string
literal where $env:_U_COMPLETE is set (the "powershell" completion block) to
"powershell_complete" (or "powershell_source") accordingly.
---
Nitpick comments:
In `@tests/test_relay_logging.py`:
- Around line 61-66: The test uses a runtime pytest.skip() inside
test_default_path_unix while other Windows-only tests use the
`@pytest.mark.skipif` decorator; change test_default_path_unix to use
`@pytest.mark.skipif`(sys.platform == "win32", reason="Unix-specific test") so
skipping is applied at collection time and consistent with the other tests
(locate the test_default_path_unix function in tests/test_relay_logging.py and
keep the existing monkeypatch and _resolve_log_dir assertion intact).
In `@unity_cli/cli/commands/completion.py`:
- Line 67: Remove the redundant second "import sys" in completion.py (the
duplicate import at the later occurrence) so only the first import sys remains;
locate the duplicate import statement (the second plain "import sys") and delete
it to avoid the redundant import.
In `@UnityBridge/Editor/RelayServerLauncher.cs`:
- Around line 250-278: GetProcessIdForPortUnix currently hardcodes FileName =
"/usr/sbin/lsof", which can fail on Linux where lsof may live in /usr/bin;
update GetProcessIdForPortUnix to resolve the lsof executable from PATH (e.g.,
try Environment.GetEnvironmentVariable("PATH") lookup or call "which lsof" / use
System.IO.Path search) before creating the ProcessStartInfo (the psi variable),
and fall back to common locations (/usr/sbin/lsof, /usr/bin/lsof) or return -1
with a logged warning if none found; ensure you only change how psi.FileName is
set and keep Arguments and other ProcessStartInfo settings the same.
- Around line 302-351: The FindExecutable method currently falls back to
checking ".exe" on Windows but misses ".cmd" wrappers; update FindExecutable to
also check for a ".cmd" variant when Application.platform ==
RuntimePlatform.WindowsEditor in both the platform-specific searchPaths loop and
the PATH loop (i.e., after checking fullPath, check fullPath + ".exe" and
fullPath + ".cmd"), ensuring the method returns the first existing match found
for either extension.
Comment on lines
+33
to
+45
| "powershell": """Register-ArgumentCompleter -Native -CommandName u,unity,'unity-cli' -ScriptBlock { | ||
| param($wordToComplete, $commandAst, $cursorPosition) | ||
| $env:_TYPER_COMPLETE_ARGS = $commandAst.ToString() | ||
| $env:_U_COMPLETE = "complete_powershell" | ||
| try { | ||
| u | ForEach-Object { | ||
| [System.Management.Automation.CompletionResult]::new($_, $_, 'ParameterValue', $_) | ||
| } | ||
| } finally { | ||
| Remove-Item Env:_TYPER_COMPLETE_ARGS -ErrorAction SilentlyContinue | ||
| Remove-Item Env:_U_COMPLETE -ErrorAction SilentlyContinue | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Typer 0.15.0 PowerShell completion support complete_powershell
💡 Result:
Typer 0.15.0 does support PowerShell tab-completion (and pwsh). PowerShell completion support was added much earlier (Typer 0.0.11) and includes help strings in completion results. [1]
How to use it (Typer 0.15.0):
- Install completion for your Typer app:
python .\app.py --install-completion(auto-detects shell ifshellinghamis installed) [2]- or explicitly:
python .\app.py --install-completion powershell/... pwsh[2]
- Or just print the script:
python .\app.py --show-completion powershell/... pwsh(then add it to your PowerShell profile). [2]
About complete_powershell:
- Typer/Click completion is driven by an environment variable like
_{PROG_NAME}_COMPLETEwhose value selects the mode (e.g. “source” to output a script, “complete” to return completions). In Click’s docs, the pattern is{shell}_source/{shell}_complete(for example,bash_source). [3] - So for PowerShell, you generally want the
{shell}_source/{shell}_completepattern (e.g.powershell_source/powershell_complete), notcomplete_powershell. [3]
If you paste the exact command/snippet where you saw complete_powershell, I can map it to the correct Typer/Click invocation for PowerShell on your setup.
Sources: Typer release notes (PowerShell support) [1]; Typer docs/examples showing --install-completion and PowerShell (pwsh) usage [2]; Click shell-completion env-var mechanism (how the “complete/source” modes work) [3].
PowerShell 補完スクリプトの環境変数パターンを修正してください。
Typer 0.15.0 は PowerShell 補完をサポートしていますが、環境変数のパターンが不正です。現在の _U_COMPLETE=complete_powershell ではなく、_U_COMPLETE=powershell_complete または _U_COMPLETE=powershell_source を使用する必要があります。Click/Typer の標準パターンは {shell}_complete / {shell}_source で、complete_{shell} ではありません。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@unity_cli/cli/commands/completion.py` around lines 33 - 45, The PowerShell
completion script sets the wrong environment variable pattern; update the
assignment of $env:_U_COMPLETE in the "powershell" completion block to use the
Typer/Click convention (e.g. "powershell_complete" or "powershell_source")
instead of "complete_powershell" so the completion hook matches Typer 0.15.0
expectations; change the value in the powershell string literal where
$env:_U_COMPLETE is set (the "powershell" completion block) to
"powershell_complete" (or "powershell_source") accordingly.
- stop() の _stopped フラグをクリーンアップ完了後に設定し、 途中例外時の再試行を可能に。各ステップを個別例外隔離 - PowerShell 補完: typer の ::: 区切り出力を分解して CompletionResult に text/description を正しく渡す - PowerShell 補完: コマンド名を $commandAst から動的取得 - GetPlatformSearchPaths: 結果キャッシュ、ディレクトリ列挙の 例外保護、Python バージョンのソート（降順） - sys の重複 import 除去、skip 記法を decorator に統一
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
relay/server.py (1)
139-179:⚠️ Potential issue | 🟠 Major
_stoppedフラグの設定タイミングに非同期の再入リスクがあります
_stoppedがすべてのクリーンアップ完了後（Line 179）に設定されていますが、チェックは Line 146 で行われています。Unix 環境では、シグナルハンドラ（Line 745）がasyncio.create_task(server.stop())でタスクを生成し、finallyブロック（Line 752）でもawait server.stop()が呼ばれます。
await self.registry.close_all()などの yield ポイントで、もう一方のstop()コルーチンに制御が移ると、_stoppedがまだFalseのため両方がクリーンアップを実行してしまいます。registry.close_all()やrequest_cache.stop()の二重呼び出しはエラーを引き起こす可能性があります。
_stoppedをクリーンアップ開始前に設定してください。🔒 修正案
async def stop(self) -> None: """Stop the relay server (idempotent).""" if self._stopped: return + self._stopped = True logger.info("Stopping Relay Server...") self._running = False # Cancel all heartbeat tasks ... - self._stopped = True logger.info("Relay Server stopped")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/server.py` around lines 139 - 179, The stop() coroutine has a race where multiple concurrent stop() calls can run cleanup because _stopped is only set after awaits; change stop() so that after the early return check and logger.info("Stopping Relay Server...") you immediately set self._stopped = True (and keep self._running = False) before performing any cancellation or awaiting (heartbeat/task cancellation, pending command cancellation, await self.registry.close_all(), await self.request_cache.stop(), and server close/wait_closed()); this makes stop() idempotent and prevents reentrant async cleanup while preserving the existing cleanup steps (refer to stop, _stopped, _running, registry.close_all, request_cache.stop, _heartbeat_tasks, _pending_commands, _server).
🧹 Nitpick comments (3)
tests/test_relay_logging.py (2)
67-79:test_default_path_windows_without_localappdataでLOCALAPPDATA削除後のPath.home()依存に注意Line 79 で
Path("AppData", "Local", "unity-cli", "logs")の末尾一致を検証していますが、_resolve_log_dirのフォールバックはPath.home() / "AppData" / "Local"を使います。Windows CI でPath.home()が想定外のパスを返す場合、テストが脆弱になる可能性があります。monkeypatchでPath.homeもモックすれば、より堅牢なテストになります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_relay_logging.py` around lines 67 - 79, In test_default_path_windows_without_localappdata, make the test deterministic by monkeypatching Path.home to return a controlled directory (e.g. the tmp_path used in the other test) after removing LOCALAPPDATA and XDG_STATE_HOME; update the assertion to compare against Path.home() / "AppData" / "Local" / "unity-cli" / "logs" (or to use tmp_path / "AppData" / "Local" / "unity-cli" / "logs") so the _resolve_log_dir fallback behaviour is validated reliably; reference the test function test_default_path_windows_without_localappdata and the helper _resolve_log_dir when applying this change.
60-85:@pytest.mark.skipifと monkeypatch を組み合わせても Windows テストはスキップされるWindows テストが
sys.platform != "win32"でスキップされるのは確かな問題です。ただし、提案のmonkeypatch.setattr(sys, "platform", "win32")では解決しません。
@pytest.mark.skipifはテスト収集時（pytest の起動時）に評価され、テスト実行時には評価されません。monkeypatch はテスト実行時に機能するため、skipif が既にスキップを決定した後では効きません。正しい対策:
- Windows テストから
@pytest.mark.skipif(sys.platform != "win32", ...)を削除- テスト内で
monkeypatch.setattr(sys, "platform", "win32")を追加- こうすれば Linux/macOS CI でも Windows コードパスが実行され、テストカバレッジが完全になります
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_relay_logging.py` around lines 60 - 85, The Windows-specific tests in class TestResolveLogDir are being skipped at collection because they use `@pytest.mark.skipif`(sys.platform != "win32", ...); remove those skipif decorators from test_default_path_windows_with_localappdata and test_default_path_windows_without_localappdata and instead set the platform inside each test using monkeypatch.setattr(sys, "platform", "win32") (also ensure you still monkeypatch LOCALAPPDATA appropriately in test_default_path_windows_with_localappdata and clear it in test_default_path_windows_without_localappdata) so the Windows code paths in _resolve_log_dir are exercised during test execution on non-Windows CI.UnityBridge/Editor/RelayServerLauncher.cs (1)
108-171:GetPlatformSearchPaths()— 動的 Python パス検出とキャッシュは良い設計ですWindows での Python インストールパスを動的にスキャンし、降順ソートで最新バージョンを優先する実装は堅実です。
IOException/UnauthorizedAccessExceptionのガードも適切です。1 点だけ：Line 135-136 で
Array.Sort+Array.Reverseを使用していますが、Array.Sortに逆順コンパレータを渡すことで 1 パスにまとめられます。♻️ ソート最適化
- System.Array.Sort(dirs, StringComparer.OrdinalIgnoreCase); - System.Array.Reverse(dirs); // Newest version first + System.Array.Sort(dirs, (a, b) => StringComparer.OrdinalIgnoreCase.Compare(b, a));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UnityBridge/Editor/RelayServerLauncher.cs` around lines 108 - 171, Replace the two-step sort (Array.Sort(dirs, StringComparer.OrdinalIgnoreCase) followed by Array.Reverse(dirs)) in GetPlatformSearchPaths with a single sort that uses a reverse comparer so the array is ordered descending in one call; locate the block that populates the local variable dirs inside GetPlatformSearchPaths and change the Array.Sort usage to pass a comparer that reverses StringComparer.OrdinalIgnoreCase (i.e., compare b vs a) so you no longer need Array.Reverse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@UnityBridge/Editor/RelayServerLauncher.cs`:
- Around line 224-299: GetProcessIdForPortUnix currently hardcodes
"/usr/sbin/lsof", which fails on systems where lsof is in /usr/bin or only on
PATH; change GetProcessIdForPortUnix to resolve the lsof executable first (try
Environment.GetEnvironmentVariable("PATH") search, check common locations like
/usr/bin and /usr/sbin, or use an existing FindExecutable("lsof") helper), then
use the resolved path in the ProcessStartInfo Arguments/FileName and fall back
to invoking "lsof" (relying on PATH) if no absolute path found; also add
/usr/sbin to your non-Windows search paths (e.g., GetPlatformSearchPaths) so
PATH-based resolution can find it.
- Around line 243-268: The netstat parsing can falsely match ports by substring;
update the prefilter and final check to compare the actual port token instead of
using Contains/EndsWith on portSuffix. In the loop that inspects
output.Split('\n') (variables portSuffix, trimmed, parts and parts[1]), extract
the port from parts[1] (handle IPv6 forms like [::]:port and IPv4 host:port by
taking the substring after the last ':' or by splitting on ':' and using the
last segment) and compare that token exactly to port (or int-parse it) and only
then check LISTENING + PID; remove the loose Contains(portSuffix) prefilter or
make it port-token-aware so only exact port matches are returned.
---
Outside diff comments:
In `@relay/server.py`:
- Around line 139-179: The stop() coroutine has a race where multiple concurrent
stop() calls can run cleanup because _stopped is only set after awaits; change
stop() so that after the early return check and logger.info("Stopping Relay
Server...") you immediately set self._stopped = True (and keep self._running =
False) before performing any cancellation or awaiting (heartbeat/task
cancellation, pending command cancellation, await self.registry.close_all(),
await self.request_cache.stop(), and server close/wait_closed()); this makes
stop() idempotent and prevents reentrant async cleanup while preserving the
existing cleanup steps (refer to stop, _stopped, _running, registry.close_all,
request_cache.stop, _heartbeat_tasks, _pending_commands, _server).
---
Nitpick comments:
In `@tests/test_relay_logging.py`:
- Around line 67-79: In test_default_path_windows_without_localappdata, make the
test deterministic by monkeypatching Path.home to return a controlled directory
(e.g. the tmp_path used in the other test) after removing LOCALAPPDATA and
XDG_STATE_HOME; update the assertion to compare against Path.home() / "AppData"
/ "Local" / "unity-cli" / "logs" (or to use tmp_path / "AppData" / "Local" /
"unity-cli" / "logs") so the _resolve_log_dir fallback behaviour is validated
reliably; reference the test function
test_default_path_windows_without_localappdata and the helper _resolve_log_dir
when applying this change.
- Around line 60-85: The Windows-specific tests in class TestResolveLogDir are
being skipped at collection because they use `@pytest.mark.skipif`(sys.platform !=
"win32", ...); remove those skipif decorators from
test_default_path_windows_with_localappdata and
test_default_path_windows_without_localappdata and instead set the platform
inside each test using monkeypatch.setattr(sys, "platform", "win32") (also
ensure you still monkeypatch LOCALAPPDATA appropriately in
test_default_path_windows_with_localappdata and clear it in
test_default_path_windows_without_localappdata) so the Windows code paths in
_resolve_log_dir are exercised during test execution on non-Windows CI.
In `@UnityBridge/Editor/RelayServerLauncher.cs`:
- Around line 108-171: Replace the two-step sort (Array.Sort(dirs,
StringComparer.OrdinalIgnoreCase) followed by Array.Reverse(dirs)) in
GetPlatformSearchPaths with a single sort that uses a reverse comparer so the
array is ordered descending in one call; locate the block that populates the
local variable dirs inside GetPlatformSearchPaths and change the Array.Sort
usage to pass a comparer that reverses StringComparer.OrdinalIgnoreCase (i.e.,
compare b vs a) so you no longer need Array.Reverse.
Comment on lines
+224
to
+299
| #if UNITY_EDITOR_WIN | ||
| private static int GetProcessIdForPortWindows(int port) | ||
| { | ||
| var psi = new ProcessStartInfo | ||
| { | ||
| FileName = "netstat", | ||
| Arguments = "-ano", | ||
| UseShellExecute = false, | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| CreateNoWindow = true | ||
| }; | ||
|
|
||
| using var process = Process.Start(psi); | ||
| if (process == null) return -1; | ||
| using var process = Process.Start(psi); | ||
| if (process == null) return -1; | ||
|
|
||
| var output = process.StandardOutput.ReadToEnd().Trim(); | ||
| process.WaitForExit(5000); | ||
| var output = process.StandardOutput.ReadToEnd(); | ||
| process.WaitForExit(5000); | ||
|
|
||
| if (process.ExitCode == 0 && !string.IsNullOrEmpty(output)) | ||
| var portSuffix = $":{port}"; | ||
| foreach (var line in output.Split('\n')) | ||
| { | ||
| var trimmed = line.Trim(); | ||
| if (!trimmed.StartsWith("TCP", StringComparison.OrdinalIgnoreCase)) | ||
| continue; | ||
| if (!trimmed.Contains(portSuffix)) | ||
| continue; | ||
|
|
||
| var parts = trimmed.Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries); | ||
| // Format: TCP <local> <foreign> <state> <PID> | ||
| if (parts.Length < 5) | ||
| continue; | ||
|
|
||
| if (!parts[1].EndsWith(portSuffix, StringComparison.Ordinal)) | ||
| continue; | ||
|
|
||
| if (parts[^2].Equals("LISTENING", StringComparison.OrdinalIgnoreCase) | ||
| && int.TryParse(parts[^1], out var pid)) | ||
| { | ||
| // lsof -t returns PID(s), take the first one | ||
| var firstLine = output.Split('\n')[0].Trim(); | ||
| if (int.TryParse(firstLine, out var pid)) | ||
| { | ||
| return pid; | ||
| } | ||
| return pid; | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
|
|
||
| return -1; | ||
| } | ||
| #else | ||
| private static int GetProcessIdForPortUnix(int port) | ||
| { | ||
| var psi = new ProcessStartInfo | ||
| { | ||
| BridgeLog.Warn($"Error checking port {port}: {ex.Message}"); | ||
| FileName = "/usr/sbin/lsof", | ||
| Arguments = $"-i :{port} -t", | ||
| UseShellExecute = false, | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| CreateNoWindow = true | ||
| }; | ||
|
|
||
| using var process = Process.Start(psi); | ||
| if (process == null) return -1; | ||
|
|
||
| var output = process.StandardOutput.ReadToEnd().Trim(); | ||
| process.WaitForExit(5000); | ||
|
|
||
| if (process.ExitCode == 0 && !string.IsNullOrEmpty(output)) | ||
| { | ||
| var firstLine = output.Split('\n')[0].Trim(); | ||
| if (int.TryParse(firstLine, out var pid)) | ||
| { | ||
| return pid; | ||
| } | ||
| } | ||
|
|
||
| return -1; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
GetProcessIdForPortUnix: lsof のパスがハードコードされている
Line 274 で /usr/sbin/lsof にハードコードされていますが、多くの Linux ディストリビューションでは /usr/bin/lsof に配置されています。macOS では /usr/sbin/lsof が正しいですが、Linux 対応を考慮する場合、FindExecutable("lsof") や PATH 経由の解決を検討してください。
GetPlatformSearchPaths() の non-Windows パスにも /usr/sbin が含まれていないため、PATH 経由でも解決できない可能性があります。
🔧 修正案: PATH 解決へのフォールバック
var psi = new ProcessStartInfo
{
- FileName = "/usr/sbin/lsof",
+ FileName = FindExecutable("lsof") ?? "/usr/sbin/lsof",
Arguments = $"-i :{port} -t",non-Windows の検索パスに /usr/sbin を追加することも検討：
_cachedSearchPaths = new[]
{
"/opt/homebrew/bin",
"/usr/local/bin",
"/usr/bin",
"/bin",
+ "/usr/sbin",
Path.Combine(homeDir, ".local", "bin"),
Path.Combine(homeDir, ".cargo", "bin"),
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@UnityBridge/Editor/RelayServerLauncher.cs` around lines 224 - 299,
GetProcessIdForPortUnix currently hardcodes "/usr/sbin/lsof", which fails on
systems where lsof is in /usr/bin or only on PATH; change
GetProcessIdForPortUnix to resolve the lsof executable first (try
Environment.GetEnvironmentVariable("PATH") search, check common locations like
/usr/bin and /usr/sbin, or use an existing FindExecutable("lsof") helper), then
use the resolved path in the ProcessStartInfo Arguments/FileName and fall back
to invoking "lsof" (relying on PATH) if no absolute path found; also add
/usr/sbin to your non-Windows search paths (e.g., GetPlatformSearchPaths) so
PATH-based resolution can find it.
- stop() に asyncio.Lock を導入して並行呼び出しレースを防止 - Python バージョンソートを数値ベースに変更 (Python39 > Python312 の誤順序を修正) - netstat/lsof の WaitForExit タイムアウト時にプロセスを Kill
EndsWith(":6500") は ":16500" にもマッチする偽陽性があった。
LastIndexOf(':') でポート部分を抽出し、完全一致比較に修正。
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
UnityBridge/Editor/RelayServerLauncher.cs (1)
551-610:⚠️ Potential issue | 🟠 Major
ServerStoppedイベントが二重に発火する可能性
_serverProcess.Exitedハンドラ (Line 443-447) とStop()の finally ブロック (Line 603) の両方がServerStoppedを発火します。Kill()後にプロセスが終了するとExitedイベントが ThreadPool 経由で発火し、Stop()の finally でも手動発火するため、リスナーが二重に呼ばれます。また、Line 565 の
Kill()はWaitForExit()を呼んでいないため、Stop()が返った時点でプロセスがまだ存在し、ポートが解放されていない可能性があります（KillProcessメソッド (Line 335-336) ではWaitForExit(3000)を呼んでいるのと不整合です）。🐛 修正案
// Try to stop via direct process reference first if (_serverProcess != null && !_serverProcess.HasExited) { _serverProcess.Kill(); + _serverProcess.WaitForExit(3000); }二重発火を防ぐため、
Stop()に入った時点でExitedハンドラを解除するか、フラグで制御してください:+ private bool _stopping; + public void Stop() { if (!IsRunning) { return; } + _stopping = true; try {_serverProcess.Exited += (_, _) => { + if (_stopping) return; BridgeLog.Info("Relay Server stopped"); ServerStopped?.Invoke(this, EventArgs.Empty); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UnityBridge/Editor/RelayServerLauncher.cs` around lines 551 - 610, Stop() can currently trigger ServerStopped twice because the Process.Exited handler and the manual ServerStopped?.Invoke in Stop() both fire; also Kill() in Stop() doesn't wait for exit unlike KillProcess which uses WaitForExit(3000). Fix by unregistering or disabling the Exited handler early in Stop() (remove or set a boolean guard) before calling _serverProcess.Kill() or KillProcess, and then use the same wait behavior as KillProcess (call WaitForExit with a timeout or call KillProcess(savedPid) which already waits) so Stop() only raises ServerStopped once after the process has actually exited; update references to _serverProcess, Process.Exited handler, Stop(), Kill(), KillProcess(...) and ServerStopped accordingly.
🧹 Nitpick comments (3)
UnityBridge/Editor/RelayServerLauncher.cs (2)
244-292: stderr 未読による理論的デッドロックリスク
RedirectStandardError = trueに設定しつつ stderr を読み取っていません。netstatが stderr に大量出力した場合（OS パイプバッファが溢れると）、プロセスが stderr 書き込みでブロック → stdout が閉じられない →ReadToEnd()が永久ブロック、というデッドロックの可能性があります。
netstatでは実質的にほぼ発生しませんが、.NET のベストプラクティスとしてRedirectStandardError = falseにするか、非同期で stderr も読み取ることを推奨します。Unix 側のGetProcessIdForPortUnix(Line 296-313) も同様です。♻️ 修正案: stderr リダイレクトを無効化
var psi = new ProcessStartInfo { FileName = "netstat", Arguments = "-ano", UseShellExecute = false, RedirectStandardOutput = true, - RedirectStandardError = true, + RedirectStandardError = false, CreateNoWindow = true };同様に
GetProcessIdForPortUnixにも適用してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UnityBridge/Editor/RelayServerLauncher.cs` around lines 244 - 292, The ProcessStartInfo in GetProcessIdForPortWindows is redirecting stderr but the code never reads it, risking a deadlock; change RedirectStandardError = false in GetProcessIdForPortWindows (and similarly in GetProcessIdForPortUnix) so stderr is not redirected, or alternatively read StandardError asynchronously—update the ProcessStartInfo initialization in both GetProcessIdForPortWindows and GetProcessIdForPortUnix to set RedirectStandardError = false (or wire up BeginErrorReadLine/ReadToEndAsync and handlers if you prefer to keep redirection).
349-398: Windows での.cmd/.bat拡張子が未考慮
FindExecutableは Windows で.exeのみをフォールバックチェックしていますが、pip や npm でインストールされたツールは.cmdや.batラッパーを生成することがあります。uvは.exeなので主要なユースケースには影響しませんが、将来的なツール対応を考慮するとPATHEXT環境変数の拡張子も検索すると堅牢になります。♻️ 修正案: PATHEXT 対応
if (Application.platform == RuntimePlatform.WindowsEditor) { - var exePath = fullPath + ".exe"; - if (File.Exists(exePath)) + var pathExt = Environment.GetEnvironmentVariable("PATHEXT") ?? ".COM;.EXE;.BAT;.CMD"; + foreach (var ext in pathExt.Split(';')) { - return exePath; + var extPath = fullPath + ext; + if (File.Exists(extPath)) + return extPath; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UnityBridge/Editor/RelayServerLauncher.cs` around lines 349 - 398, FindExecutable currently only falls back to ".exe" on Windows and misses executables with .cmd/.bat (or other PATHEXT extensions); update FindExecutable to, when Application.platform == RuntimePlatform.WindowsEditor, read Environment.GetEnvironmentVariable("PATHEXT") (or default to ".COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC"), split it, and for each candidate basePath + name + ext check File.Exists before continuing; apply this logic both for the platform-specific searchPaths loop and the PATH loop, and keep existing .exe handling by including it in the PATHEXT list so FindExecutable (and related calls) will resolve .cmd/.bat wrappers as well as .exe.relay/server.py (1)
154-157: ハートビートタスクをキャンセルのみで待機しない（任意改善）
task.cancel()後にawaitしないため、タスクがasyncio.CancelledErrorを処理するまでの間に「Task was destroyed but it is pending!」警告が出る場合があります。_heartbeat_loop自体はCancelledErrorをpassで処理しているため動作上の問題はありませんが、シャットダウンログがノイジーになる可能性があります。♻️ 修正案（gather でキャンセル完了を待機）
# Cancel all heartbeat tasks for task in self._heartbeat_tasks.values(): task.cancel() + if self._heartbeat_tasks: + await asyncio.gather(*self._heartbeat_tasks.values(), return_exceptions=True) self._heartbeat_tasks.clear()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/server.py` around lines 154 - 157, When shutting down you only call task.cancel() on entries in self._heartbeat_tasks without awaiting their completion, which can emit "Task was destroyed but it is pending!" warnings; change the shutdown to collect the tasks (e.g., tasks = list(self._heartbeat_tasks.values()), call cancel() on each), then await their completion with asyncio.gather(*tasks, return_exceptions=True) to let _heartbeat_loop handle asyncio.CancelledError and suppress warnings, and finally clear self._heartbeat_tasks; reference the _heartbeat_tasks collection and the _heartbeat_loop task handler when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@relay/server.py`:
- Around line 177-182: The shutdown path can raise in await
self._server.wait_closed() and prevent self._stopped from being set; wrap the
server close/wait logic in try/except/finally inside stop() so any exception
from self._server.wait_closed() is caught (log it) and ensure self._stopped =
True is executed in the finally block; keep calling registry.close_all() and
request_cache.stop() as before (they are idempotent) but move/guard them so they
always run or are skipped appropriately when _stopped is set to avoid repeated
cleanup on subsequent stop() calls.
---
Outside diff comments:
In `@UnityBridge/Editor/RelayServerLauncher.cs`:
- Around line 551-610: Stop() can currently trigger ServerStopped twice because
the Process.Exited handler and the manual ServerStopped?.Invoke in Stop() both
fire; also Kill() in Stop() doesn't wait for exit unlike KillProcess which uses
WaitForExit(3000). Fix by unregistering or disabling the Exited handler early in
Stop() (remove or set a boolean guard) before calling _serverProcess.Kill() or
KillProcess, and then use the same wait behavior as KillProcess (call
WaitForExit with a timeout or call KillProcess(savedPid) which already waits) so
Stop() only raises ServerStopped once after the process has actually exited;
update references to _serverProcess, Process.Exited handler, Stop(), Kill(),
KillProcess(...) and ServerStopped accordingly.
---
Duplicate comments:
In `@UnityBridge/Editor/RelayServerLauncher.cs`:
- Around line 296-304: The ProcessStartInfo uses a hardcoded path for lsof
(FileName = "/usr/sbin/lsof") which fails on systems where lsof lives in
/usr/bin; update the launcher to resolve the executable via PATH instead of
hardcoding: set ProcessStartInfo.FileName to "lsof" (rely on the environment
PATH) or implement a small resolver that checks common locations (e.g.,
/usr/sbin/lsof and /usr/bin/lsof) and falls back to "lsof" if found, updating
the code that creates the psi variable in RelayServerLauncher (the
ProcessStartInfo named psi) so it reliably locates the lsof binary across Linux
installs.
---
Nitpick comments:
In `@relay/server.py`:
- Around line 154-157: When shutting down you only call task.cancel() on entries
in self._heartbeat_tasks without awaiting their completion, which can emit "Task
was destroyed but it is pending!" warnings; change the shutdown to collect the
tasks (e.g., tasks = list(self._heartbeat_tasks.values()), call cancel() on
each), then await their completion with asyncio.gather(*tasks,
return_exceptions=True) to let _heartbeat_loop handle asyncio.CancelledError and
suppress warnings, and finally clear self._heartbeat_tasks; reference the
_heartbeat_tasks collection and the _heartbeat_loop task handler when making the
change.
In `@UnityBridge/Editor/RelayServerLauncher.cs`:
- Around line 244-292: The ProcessStartInfo in GetProcessIdForPortWindows is
redirecting stderr but the code never reads it, risking a deadlock; change
RedirectStandardError = false in GetProcessIdForPortWindows (and similarly in
GetProcessIdForPortUnix) so stderr is not redirected, or alternatively read
StandardError asynchronously—update the ProcessStartInfo initialization in both
GetProcessIdForPortWindows and GetProcessIdForPortUnix to set
RedirectStandardError = false (or wire up BeginErrorReadLine/ReadToEndAsync and
handlers if you prefer to keep redirection).
- Around line 349-398: FindExecutable currently only falls back to ".exe" on
Windows and misses executables with .cmd/.bat (or other PATHEXT extensions);
update FindExecutable to, when Application.platform ==
RuntimePlatform.WindowsEditor, read
Environment.GetEnvironmentVariable("PATHEXT") (or default to
".COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC"), split it, and for each
candidate basePath + name + ext check File.Exists before continuing; apply this
logic both for the platform-specific searchPaths loop and the PATH loop, and
keep existing .exe handling by including it in the PATHEXT list so
FindExecutable (and related calls) will resolve .cmd/.bat wrappers as well as
.exe.
Comment on lines
+177
to
+182
| # Close server | ||
| if self._server: | ||
| self._server.close() | ||
| await self._server.wait_closed() | ||
|
|
||
| logger.info("Relay Server stopped") | ||
| self._stopped = True |
There was a problem hiding this comment.
wait_closed() が例外を送出した場合、_stopped が True にならない
self._server.close() と await self._server.wait_closed() は try/except で保護されていません。wait_closed() が例外を送出した場合（イベントループの早期終了時など）、_stopped = True に到達できず、次回の stop() 呼び出しが再びクリーンアップパスに入ります。registry.close_all() と request_cache.stop() は冪等ですが、wait_closed() が繰り返し失敗するシナリオでは不要な再実行が発生します。
🛡️ 修正案
- # Close server
- if self._server:
- self._server.close()
- await self._server.wait_closed()
+ # Close server
+ if self._server:
+ try:
+ self._server.close()
+ await self._server.wait_closed()
+ except Exception:
+ logger.exception("Error closing server")
self._stopped = True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Close server | |
| if self._server: | |
| self._server.close() | |
| await self._server.wait_closed() | |
| logger.info("Relay Server stopped") | |
| self._stopped = True | |
| # Close server | |
| if self._server: | |
| try: | |
| self._server.close() | |
| await self._server.wait_closed() | |
| except Exception: | |
| logger.exception("Error closing server") | |
| self._stopped = True |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@relay/server.py` around lines 177 - 182, The shutdown path can raise in await
self._server.wait_closed() and prevent self._stopped from being set; wrap the
server close/wait logic in try/except/finally inside stop() so any exception
from self._server.wait_closed() is caught (log it) and ensure self._stopped =
True is executed in the finally block; keep calling registry.close_all() and
request_cache.stop() as before (they are idempotent) but move/guard them so they
always run or are skipped appropriately when _stopped is set to avoid repeated
cleanup on subsequent stop() calls.
絶対パス (macOS の /Volumes/...) を相対パス (../../UnityBridge) に変更し どの環境でも解決できるようにする。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- BridgeStatusFile: ++_seq → Interlocked.Increment でスレッドセーフに
- Scene: path なし (無名シーン) での save が Unity ダイアログを表示し
タイムアウトになる問題を修正。ProtocolException を即座に返す
- Tests: ResultState を Split(':')[0] でパースし Failed:Error / Skipped:Explicit
などのラベル付きを正しく分類
- BridgeManagerAsyncTests: LogAssert.Expect でエラーログを予期宣言
- SceneTest: 無名シーンへの save が INVALID_PARAMS を返すことを検証するテスト追加
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
UnityBridge/Editor/Tools/Scene.cs (1)
206-216:⚠️ Potential issue | 🟠 MajorUnityBridge/Editor 内のファイル変更時は EditMode テストの実行が必須
UnityBridge/Editor/Tools/Scene.csは UnityBridge/Editor 以下のファイルであるため、コミット前に EditMode テストの実行が必要です（'u -i TestProject tests run edit' コマンド）。PR の説明では Python テスト（pytest）の実行のみ記載されており、Unity EditMode テストの実行確認ができません。変更を確定する前に、この要件を満たしていることを確認してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UnityBridge/Editor/Tools/Scene.cs` around lines 206 - 216, This change touches UnityBridge/Editor (specifically UnityBridge/Editor/Tools/Scene.cs and the SaveScene method), so before finalizing the commit run the Unity EditMode tests using the provided command (`u -i TestProject tests run edit`), fix any failing EditMode tests caused by your change, and then update the PR description or CI configuration to confirm EditMode tests were executed; ensure the PR notes mention the successful run of the EditMode tests and include any test-fix details if you modified SaveScene or related editor code.
🧹 Nitpick comments (1)
UnityBridge/Editor/Tools/Tests.cs (1)
514-515:result.ResultStateがヌルの場合にNullReferenceExceptionが発生する可能性があります変更前の
switch (result.ResultState)はヌルをdefaultケースに流しましたが、result.ResultState.Split(':')はヌルの場合にNullReferenceExceptionをスローします。Unity テストフレームワークが実際にヌルを返すことは稀ですが、元のコードより堅牢性が低下しています。🛡️ ヌル安全な修正案
- var resultStatus = result.ResultState.Split(':')[0]; + var resultStatus = result.ResultState?.Split(':')[0] ?? string.Empty;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UnityBridge/Editor/Tools/Tests.cs` around lines 514 - 515, result.ResultState can be null which makes result.ResultState.Split(':')[0] throw; make the extraction of resultStatus null-safe by first guarding result.ResultState (e.g. use a null-coalescing fallback or check string.IsNullOrEmpty) before calling Split and assign a safe default (e.g. empty string or "Unknown") to resultStatus so the subsequent switch on resultStatus does not throw; reference result.ResultState, the Split call, and the resultStatus variable when applying the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TestProject/Assets/Tests/Editor/BridgeManagerAsyncTests.cs`:
- Around line 49-50: This test depends on BridgeLog.Enabled being true so
BridgeLog.Error will call Debug.LogError; at the start of the test method set
BridgeLog.Enabled = true to ensure LogAssert.Expect(LogType.Error, ...) will see
the error, i.e. before calling manager.ExecuteCommandOnMainThreadAsync(args)
explicitly assign BridgeLog.Enabled = true (and optionally restore the previous
value at the end of the test if needed).
---
Outside diff comments:
In `@UnityBridge/Editor/Tools/Scene.cs`:
- Around line 206-216: This change touches UnityBridge/Editor (specifically
UnityBridge/Editor/Tools/Scene.cs and the SaveScene method), so before
finalizing the commit run the Unity EditMode tests using the provided command
(`u -i TestProject tests run edit`), fix any failing EditMode tests caused by
your change, and then update the PR description or CI configuration to confirm
EditMode tests were executed; ensure the PR notes mention the successful run of
the EditMode tests and include any test-fix details if you modified SaveScene or
related editor code.
---
Nitpick comments:
In `@UnityBridge/Editor/Tools/Tests.cs`:
- Around line 514-515: result.ResultState can be null which makes
result.ResultState.Split(':')[0] throw; make the extraction of resultStatus
null-safe by first guarding result.ResultState (e.g. use a null-coalescing
fallback or check string.IsNullOrEmpty) before calling Split and assign a safe
default (e.g. empty string or "Unknown") to resultStatus so the subsequent
switch on resultStatus does not throw; reference result.ResultState, the Split
call, and the resultStatus variable when applying the fix.
Comment on lines
+49
to
50
| LogAssert.Expect(LogType.Error, new Regex("Command execution failed: InvalidOperationException - test error")); | ||
| await manager.ExecuteCommandOnMainThreadAsync(args); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check BridgeLog's Enabled property definition and default value
echo "=== BridgeLog.cs Enabled property ==="
rg -n "Enabled" --type=cs -B2 -A5 UnityBridge/Editor/Helpers/BridgeLog.cs
echo ""
echo "=== BridgeLog initialization/usage in BridgeManagerAsyncTests ==="
rg -n "BridgeLog\|SetUp\|setUp" --type=cs -B2 -A3 TestProject/Assets/Tests/Editor/BridgeManagerAsyncTests.cs
echo ""
echo "=== Test file structure and full context around lines 49-50 ==="
sed -n '1,60p' TestProject/Assets/Tests/Editor/BridgeManagerAsyncTests.cs | cat -nRepository: bigdra50/unity-cli
Length of output: 5232
🏁 Script executed:
#!/bin/bash
# Check if there's a test fixture or setup class that configures BridgeLog
echo "=== Looking for test fixture or setup files ==="
fd -e cs TestProject/Assets/Tests/Editor/ | xargs rg -l "SetUp\|TearDown\|OneTimeSetUp" --type=cs
echo ""
echo "=== Check for BridgeLog.Enabled assignments in test files ==="
rg -n "BridgeLog\\.Enabled" --type=cs TestProject/Assets/Tests/Repository: bigdra50/unity-cli
Length of output: 623
🏁 Script executed:
#!/bin/bash
# Verify the file matches the coding guidelines
echo "=== Verify file location and asmdef ==="
ls -la TestProject/Assets/Tests/Editor/BridgeManagerAsyncTests.cs
if [ -f "TestProject/Assets/Tests/Editor/Game.Tests.Editor.asmdef" ]; then
echo "asmdef file found:"
cat TestProject/Assets/Tests/Editor/Game.Tests.Editor.asmdef
else
echo "Looking for asmdef files in the directory:"
find TestProject/Assets/Tests/Editor/ -name "*.asmdef"
fiRepository: bigdra50/unity-cli
Length of output: 551
🏁 Script executed:
#!/bin/bash
# Search for all assignments to BridgeLog.Enabled in the entire codebase
echo "=== All BridgeLog.Enabled assignments ==="
rg -n "BridgeLog\.Enabled\s*=" --type=cs
echo ""
echo "=== Search for EditorPrefs modifications related to EnabledKey ==="
rg -n "UnityBridge\.LogEnabled\|EnabledKey" --type=csRepository: bigdra50/unity-cli
Length of output: 275
🏁 Script executed:
#!/bin/bash
# Check if there are any other tests in the same file that might disable logging
echo "=== All test methods in BridgeManagerAsyncTests.cs ==="
rg -n "\[Test\]\|\[OneTimeSetUp\]\|\[SetUp\]\|\[TearDown\]\|public.*Task.*\(" --type=cs TestProject/Assets/Tests/Editor/BridgeManagerAsyncTests.cs | head -20Repository: bigdra50/unity-cli
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Check ExecuteCommandOnMainThreadAsync implementation to understand error logging
echo "=== ExecuteCommandOnMainThreadAsync implementation ==="
rg -n "ExecuteCommandOnMainThreadAsync" --type=cs -A 30 UnityBridge/Editor/ | head -50Repository: bigdra50/unity-cli
Length of output: 3172
テスト実行時の BridgeLog.Enabled 状態に依存しています
BridgeLog.Error は if (!Enabled) return; のガード節を持つため、Debug.LogError が呼び出されるには BridgeLog.Enabled が true である必要があります。デフォルト値は true ですが、このテストは明示的に BridgeLog.Enabled を設定していないため、EditorPrefs から previous session の値が読み込まれる可能性があります。LogAssert.Expect が確実に機能するよう、テストメソッドの開始時に BridgeLog.Enabled = true; を明示的に設定してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TestProject/Assets/Tests/Editor/BridgeManagerAsyncTests.cs` around lines 49 -
50, This test depends on BridgeLog.Enabled being true so BridgeLog.Error will
call Debug.LogError; at the start of the test method set BridgeLog.Enabled =
true to ensure LogAssert.Expect(LogType.Error, ...) will see the error, i.e.
before calling manager.ExecuteCommandOnMainThreadAsync(args) explicitly assign
BridgeLog.Enabled = true (and optionally restore the previous value at the end
of the test if needed).
- プラットフォームバッジ追加 (macOS | Windows | Linux) - Shell Completion に PowerShell を追加 - トラブルシューティングに Windows 向けコマンドを追加
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.jp.md`:
- Line 7: The Platform badge markdown currently has an empty link
'()[]' which triggers MD042; update the badge link in
README.jp.md by replacing the empty parentheses with a valid URL (for example
the repository root or the CI workflow URL) so the badge points to a real target
— locate the badge line containing
'[]()' and set the URL
inside the trailing parentheses to the chosen valid link.
In `@README.md`:
- Line 7: The Platform badge markdown
"[]()"
has an empty link target causing MD042; update the trailing parentheses to the
same URL used in README.jp.md (or a valid destination) so the badge links
correctly, ensuring you edit the exact badge string to replace the empty "()"
with the intended URL.
README.jp.md
Outdated
| [](https://github.com/bigdra50/unity-cli/actions/workflows/ci.yml) | ||
| [](https://github.com/bigdra50/unity-cli/actions/workflows/ci.yml) | ||
| [](https://www.python.org/) | ||
| []() |
There was a problem hiding this comment.
Platformバッジのリンク先が空です (MD042)
[]() のリンク先 () が空であり、markdownlint の MD042 警告が発生しています。他のバッジはすべて有効なURLを持っています。CI ワークフローへのリンク、またはリポジトリ root へのリンクを追加してください。
🔧 修正案
-[]()
+[](https://github.com/bigdra50/unity-cli)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| []() | |
| [](https://github.com/bigdra50/unity-cli) |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 7-7: No empty links
(MD042, no-empty-links)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.jp.md` at line 7, The Platform badge markdown currently has an empty
link '()[]' which triggers MD042; update the badge link in
README.jp.md by replacing the empty parentheses with a valid URL (for example
the repository root or the CI workflow URL) so the badge points to a real target
— locate the badge line containing
'[]()' and set the URL
inside the trailing parentheses to the chosen valid link.
README.md
Outdated
| [](https://github.com/bigdra50/unity-cli/actions/workflows/ci.yml) | ||
| [](https://github.com/bigdra50/unity-cli/actions/workflows/ci.yml) | ||
| [](https://www.python.org/) | ||
| []() |
There was a problem hiding this comment.
Platformバッジのリンク先が空です (MD042)
README.jp.md と同様に、() が空でmarkdownlint MD042 警告が発生しています。同一の修正を適用してください。
🔧 修正案
-[]()
+[](https://github.com/bigdra50/unity-cli)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| []() | |
| [](https://github.com/bigdra50/unity-cli) |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 7-7: No empty links
(MD042, no-empty-links)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 7, The Platform badge markdown
"[]()"
has an empty link target causing MD042; update the trailing parentheses to the
same URL used in README.jp.md (or a valid destination) so the badge links
correctly, ensuring you edit the exact badge string to replace the empty "()"
with the intended URL.
- stop() の wait_closed() を try/except で保護し _stopped 到達を保証 - BridgeManagerAsyncTests で BridgeLog.Enabled を明示的に設定 - Platform バッジのリンク先を設定 (MD042 解消)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
add_signal_handlerのNotImplementedErrorで Windows 起動不可だった問題を修正 (Relay ServerがWindows環境でadd_signal_handlerがサポートされていないために起動できない #88)Changes
relay/server.pystop()冪等化、ログディレクトリ%LOCALAPPDATA%フォールバック、get_running_loop()RelayServerLauncher.csKillProcess→Process.Kill()、GetProcessIdForPortnetstat/lsof 分岐、GetPlatformSearchPaths()で Windows Python パス動的走査completion.pytest_relay_logging.py/tmpハードコード →tmp_pathfixture、/proc依存除去、Windows 分岐テスト追加hub/paths.pyProgramFiles(x86)の Hub パス候補追加Test plan
uv run python -m pytest tests/全テスト通過 (532 passed, 2 skipped)Closes #89, Closes #88
Summary by CodeRabbit
新機能
改善
バグ修正
テスト
その他