fix: Avoid false "Unity not running" errors when the editor is still open#919
Merged
Merged
Conversation
The CLI previously trusted isServerRunning before attempting a connection, which made stale settings after crashes or forced termination look like a clean Unity-not-running state. That hid the difference between an editor that is closed and an editor that is still open while the Unity CLI Loop server is unavailable. This change keeps port resolution focused on reading customPort from project settings and moves the final diagnosis to the point where a real connection attempt fails. When a retryable connection error happens for a known project, the CLI now checks the OS process list for that project and reports either UnityNotRunningError or UnityServerNotRunningError with dedicated user-facing guidance. It also adds a standalone unity-process module with macOS, Linux, and Windows process queries plus parser-focused unit tests, so the OS-level detection can be verified without launching Unity. The affected surface is limited to CLI connection diagnostics, project error messaging, and the related unit tests.
The repository already expected code comments, commit messages, and pull request text to be written in English, but the previous sentence bundled those rules together in a way that was easy to skim past. This updates AGENTS.md to say the same policy more explicitly by calling out commit messages, PR titles, and PR descriptions individually. The goal is to reduce future conflicts between repository-specific conventions and local automation prompts.
Contributor
📝 WalkthroughWalkthroughIntroduces UnityServerNotRunningError and diagnostics that inspect running Unity editor processes; adds a new unity-process module; integrates diagnosis into execute-tool and CLI error handling; adjusts port-resolver to not fail when settings report the server as not running; updates tests and docs accordingly. Changes
Sequence DiagramsequenceDiagram
actor CLI as CLI Client
participant Exec as executeToolCommand
participant Diag as diagnoseRetryableProjectConnectionError
participant ProcFinder as findRunningUnityProcessForProject
participant Reporter as getProjectResolutionErrorLines
participant Exit as CLI Exit
CLI->>Exec: run tool for project
Exec->>Exec: attempt/connect/retry
Exec->>Diag: diagnoseRetryableProjectConnectionError(error, projectRoot, shouldDiagnose)
Diag->>ProcFinder: findRunningUnityProcessForProject(projectRoot)
alt process found
ProcFinder-->>Diag: { pid }
Diag-->>Exec: UnityServerNotRunningError
else no process
ProcFinder-->>Diag: null
Diag-->>Exec: UnityNotRunningError
end
Exec->>Reporter: getProjectResolutionErrorLines(error)
Reporter-->>Exec: lines[]
Exec->>CLI: print lines
CLI->>Exit: exit(1)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Packages/src/Cli~/src/cli-project-error.ts (1)
7-14: Reuse the existing product/menu constants here.This introduces another hard-coded copy of the server guidance, while
Packages/src/Cli~/src/cli.tsalready prints the same concept via shared constants. PullingPRODUCT_DISPLAY_NAME/MENU_PATH_SERVERinto this branch would keep the wording consistent if the menu path or branding changes again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Packages/src/Cli`~/src/cli-project-error.ts around lines 7 - 14, The branch handling UnityServerNotRunningError in the function that returns the message array currently hard-codes the product/menu text; update this branch to use the shared constants PRODUCT_DISPLAY_NAME and MENU_PATH_SERVER (same symbols used in cli.ts) instead of literal strings, add the necessary import for those constants, and interpolate them into the returned lines (e.g., replace "Unity Editor" and the "Window > Unity CLI Loop > Server" line with PRODUCT_DISPLAY_NAME and MENU_PATH_SERVER) so wording stays consistent across the codebase.Packages/src/Cli~/src/__tests__/unity-process.test.ts (1)
19-29: Add test coverage for Windows single-process JSON output; consider simplifying with array-wrapping.The production parser already handles both JSON array and single-object output (via the conditional on line 199 of unity-process.ts). However, the test suite should verify this single-object case, as PowerShell
ConvertTo-Jsonoutputs a plain object{...}rather than an array[...]when exactly oneWin32_Processmatches. Optionally, wrapping the pipeline in@(...)would force consistent array output and eliminate the conditional check:Suggested improvement
- 'Get-CimInstance Win32_Process -Filter "name = \'Unity.exe\'" | Select-Object ProcessId, CommandLine | ConvertTo-Json -Compress', + '@(Get-CimInstance Win32_Process -Filter "name = \'Unity.exe\'" | Select-Object ProcessId, CommandLine) | ConvertTo-Json -Compress',Add a test case for single-object JSON output (e.g.,
{"ProcessId":101,"CommandLine":"..."}) to verify it parses correctly alongside the existing array test.Also applies to: 85–103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Packages/src/Cli`~/src/__tests__/unity-process.test.ts around lines 19 - 29, Add a unit test in unity-process.test.ts that exercises the parser in unity-process.ts with a single-object PowerShell JSON string (e.g. '{"ProcessId":101,"CommandLine":"..."}') to ensure parseUnityProcessOutput (or the exported parser function in unity-process.ts) correctly returns the same normalized result as the array case; construct the single-object JSON string, call the parser, and assert the parsed output matches the expected process entry (you can mirror the existing array test assertions), or alternatively update the PowerShell command to force array output via @(...) if you prefer consistent command output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Packages/src/Cli`~/src/unity-process.ts:
- Around line 9-11: The exported interface RunningUnityProcess is flagged as
unused; remove the export to make it internal (change "export interface
RunningUnityProcess" to "interface RunningUnityProcess") or eliminate the
interface and inline its shape ({ pid: number }) where it’s used/returned so no
exported type is unused; update any function or return signatures that
referenced RunningUnityProcess to use the inline type if you remove the
interface.
- Around line 201-207: Remove the redundant case-sensitive CommandLine check
that filters for UNITY_WINDOWS_PROCESS_NAME and rely on the existing
isWindowsUnityProcessWithCommandLine predicate returned from the PowerShell
query; specifically delete the .filter((processInfo) =>
processInfo.CommandLine.includes(UNITY_WINDOWS_PROCESS_NAME)) call so the
pipeline becomes
processArray.filter(isWindowsUnityProcessWithCommandLine).map(...) — this
prevents false negatives caused by path casing and avoids incorrectly producing
UnityNotRunningError while leaving the mapping to pid/commandLine intact.
---
Nitpick comments:
In `@Packages/src/Cli`~/src/__tests__/unity-process.test.ts:
- Around line 19-29: Add a unit test in unity-process.test.ts that exercises the
parser in unity-process.ts with a single-object PowerShell JSON string (e.g.
'{"ProcessId":101,"CommandLine":"..."}') to ensure parseUnityProcessOutput (or
the exported parser function in unity-process.ts) correctly returns the same
normalized result as the array case; construct the single-object JSON string,
call the parser, and assert the parsed output matches the expected process entry
(you can mirror the existing array test assertions), or alternatively update the
PowerShell command to force array output via @(...) if you prefer consistent
command output.
In `@Packages/src/Cli`~/src/cli-project-error.ts:
- Around line 7-14: The branch handling UnityServerNotRunningError in the
function that returns the message array currently hard-codes the product/menu
text; update this branch to use the shared constants PRODUCT_DISPLAY_NAME and
MENU_PATH_SERVER (same symbols used in cli.ts) instead of literal strings, add
the necessary import for those constants, and interpolate them into the returned
lines (e.g., replace "Unity Editor" and the "Window > Unity CLI Loop > Server"
line with PRODUCT_DISPLAY_NAME and MENU_PATH_SERVER) so wording stays consistent
across the codebase.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04cb44d6-912f-4b6a-beb5-7611d5242846
📒 Files selected for processing (10)
AGENTS.mdPackages/src/Cli~/src/__tests__/cli-project-error.test.tsPackages/src/Cli~/src/__tests__/execute-tool.test.tsPackages/src/Cli~/src/__tests__/port-resolver.test.tsPackages/src/Cli~/src/__tests__/unity-process.test.tsPackages/src/Cli~/src/cli-project-error.tsPackages/src/Cli~/src/cli.tsPackages/src/Cli~/src/execute-tool.tsPackages/src/Cli~/src/port-resolver.tsPackages/src/Cli~/src/unity-process.ts
The build-typescript-server GitHub Actions job failed in the CLI knip step because RunningUnityProcess was exported from src/unity-process.ts but never imported anywhere else. This change keeps the type internal to the module while preserving the public helper functions used by the CLI and the unit tests. The goal is to satisfy the unused-export check without changing the runtime behavior of the Unity process detection code. Verification: - npx knip - npx tsc --noEmit - npm run build - npx jest src/__tests__/unity-process.test.ts --runInBand --testTimeout=60000
hatayama
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Packages/src/Cli~/src/unity-process.ts (1)
201-203:⚠️ Potential issue | 🟠 MajorRemove the extra case-sensitive
Unity.exefilter.The PowerShell query already limits rows to
name = 'Unity.exe'; this second filter drops real processes when the executable path casing differs and can incorrectly downgrade the diagnosis to “Unity not running”.Suggested fix
return processArray .filter(isWindowsUnityProcessWithCommandLine) - .filter((processInfo) => processInfo.CommandLine.includes(UNITY_WINDOWS_PROCESS_NAME)) .map((processInfo) => ({ pid: processInfo.ProcessId, commandLine: processInfo.CommandLine, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Packages/src/Cli`~/src/unity-process.ts around lines 201 - 203, The second, case-sensitive filter that checks processInfo.CommandLine.includes(UNITY_WINDOWS_PROCESS_NAME) is redundant and can drop matching Unity processes due to path casing; remove that filter and return only processArray.filter(isWindowsUnityProcessWithCommandLine) so the PowerShell name restriction is relied upon instead—update any downstream assumptions if they depended on the extra include check and keep references to isWindowsUnityProcessWithCommandLine and UNITY_WINDOWS_PROCESS_NAME for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Packages/src/Cli`~/src/unity-process.ts:
- Around line 151-154: parseUnityProcesses returns every ps row so the current
lookup can false-positive match any process with the same -projectPath value;
update the search to first assert the row is actually a Unity editor process
before checking project path. Add or reuse a predicate (e.g.,
isUnityEditorProcess or isUnityBinary) that inspects
processInfo.commandLine/executable for Unity editor signatures (case-insensitive
"unity", "Unity.app/Contents/MacOS/Unity", or platform-specific Unity binary
names) and on macOS/Linux check for an .app or expected binary path; then change
the find call to require this predicate AND
isUnityProcessForProject(processInfo.commandLine, projectRoot,
dependencies.platform) so only true Unity editor processes are matched.
---
Duplicate comments:
In `@Packages/src/Cli`~/src/unity-process.ts:
- Around line 201-203: The second, case-sensitive filter that checks
processInfo.CommandLine.includes(UNITY_WINDOWS_PROCESS_NAME) is redundant and
can drop matching Unity processes due to path casing; remove that filter and
return only processArray.filter(isWindowsUnityProcessWithCommandLine) so the
PowerShell name restriction is relied upon instead—update any downstream
assumptions if they depended on the extra include check and keep references to
isWindowsUnityProcessWithCommandLine and UNITY_WINDOWS_PROCESS_NAME for clarity.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d2c0898-400e-4718-bbb6-468b808dc306
📒 Files selected for processing (1)
Packages/src/Cli~/src/unity-process.ts
hatayama
changed the title
Address the PR review feedback around OS-level Unity detection so the CLI can distinguish real Unity editor processes from unrelated commands more reliably. The previous implementation could reject valid Windows processes when the executable casing differed from 'Unity.exe', and it could also accept non-Unity macOS/Linux processes as long as they reused the same -projectPath argument. Key changes: - removed the redundant Windows command-line filter that re-checked for 'Unity.exe' after the PowerShell query had already limited results to Unity.exe processes - added an isUnityEditorProcess predicate and required findRunningUnityProcessForProject to validate both the executable identity and the project path before returning a match - covered the new behavior with unit tests for uppercase Windows executable names and for non-Unity processes that happen to share the same projectPath Impact: - reduces false negatives on Windows when CommandLine casing differs from the expected literal - reduces false positives on macOS/Linux by avoiding matches against arbitrary tools that expose a -projectPath flag - keeps the detection logic small and local to unity-process.ts instead of spreading platform-specific checks across callers
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Packages/src/Cli`~/src/unity-process.ts:
- Around line 65-109: tokenizeCommandLine and extractUnityProjectPath fail on
POSIX ps output because quoting is lost; update extractUnityProjectPath (and
stop relying on tokenizeCommandLine for POSIX) to match the known projectRoot
directly against the raw commandLine string instead of tokenizing it: detect a
substring equal to projectRoot (with path separators or boundaries to avoid
false matches) and return that when found; adjust
findRunningUnityProcessForProject to pass the expected projectRoot into
extractUnityProjectPath (or call the new raw-match helper) and remove/limit
usage of tokenizeCommandLine for POSIX-only code paths so we reliably detect
running Unity instances even when ps has flattened quotes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e312e0e5-e4e0-4a51-9c22-5b7195daacd6
📒 Files selected for processing (2)
Packages/src/Cli~/src/__tests__/unity-process.test.tsPackages/src/Cli~/src/unity-process.ts
Fix the remaining PR review issue around OS-level Unity detection on macOS and Linux. POSIX `ps` output does not reliably preserve shell quoting, so a Unity command like `-projectPath /Users/me/My Project` could be flattened into a plain string that the existing token-based parser split at the first space. When that happened, `findRunningUnityProcessForProject` could return `null` even though the editor was running, which reintroduced the false `Unity not running` diagnosis this branch is meant to remove. Key changes: - changed `isUnityProcessForProject` to use a POSIX-specific raw command-line matcher instead of relying on `extractUnityProjectPath` tokenization - added `commandLineContainsProjectRoot` and a terminator check so the matcher accepts flattened paths with spaces but rejects prefix-only matches such as `/Users/me/My Project Backup` - kept the Windows path extraction flow unchanged so the existing `-projectPath` parsing behavior still applies there - added regression tests for flattened macOS `ps` output and for the prefix false-positive case, including the end-to-end `findRunningUnityProcessForProject` path Impact: - reduces false `Unity not running` errors when the Unity project path contains spaces on macOS or Linux - avoids expanding the detection to unrelated neighboring paths that merely share the same prefix - keeps the platform-specific behavior localized to `unity-process.ts`, which makes later changes to process detection easier to reason about
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/src/Cli~/src/unity-process.ts">
<violation number="1" location="Packages/src/Cli~/src/unity-process.ts:126">
P2: Non-Windows project matching can fail when `-projectPath` has a trailing slash, causing running Unity editors to be misclassified as not matching the target project.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Fix the latest PR review issue around the POSIX project matcher in `unity-process.ts`. After the previous raw command-line matching change, macOS and Linux could still misclassify a running Unity editor when the `-projectPath` value ended with one or more trailing `/` characters. That left a gap where normalized project roots such as `/Users/me/My Project` no longer matched raw process output like `/Users/me/My Project/`, which could surface as another false `Unity not running` diagnosis. Key changes: - added `skipTrailingProjectPathSeparators` so the POSIX matcher ignores trailing `/` characters before evaluating the end of the project path - kept the existing prefix guard intact so neighboring paths such as `/Users/me/My Project Backup` still do not match the target project - added regression tests for single and repeated trailing slash variants in both `isUnityProcessForProject` and `findRunningUnityProcessForProject` Impact: - reduces false negative Unity process detection when POSIX process output preserves trailing slashes on `-projectPath` - preserves the earlier fix for flattened paths with spaces without reopening the prefix-only false positive case - keeps the path-normalization behavior localized to the POSIX-specific matcher, so Windows detection remains unchanged
This was referenced May 10, 2026
Merged
Closed
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
isServerRunningflag up frontAGENTS.mdthat commit messages, PR titles, and PR descriptions must all be written in EnglishWhy
The previous CLI flow could report
Unity not runningtoo early becauseUserSettings/UnityMcpSettings.jsonmay still sayisServerRunning: trueorfalseafter crashes, force-kills, or other unclean shutdowns. That made it harder to distinguish between a closed Unity Editor and a still-open editor whose C# server was unavailable.What changed
customPortfrom settingsUnityNotRunningErrororUnityServerNotRunningErrorAGENTS.mdso the English-writing rule is harder to misreadTesting
npx jest src/__tests__/cli-project-error.test.ts src/__tests__/execute-tool.test.ts src/__tests__/port-resolver.test.ts src/__tests__/unity-process.test.ts --runInBand --testTimeout=60000npm run buildnpm run lint(existing warnings only)node Packages/src/Cli~/dist/cli.bundle.cjs get-logs --project-path .while Unity is not running to confirm the CLI reports the not-running guidanceSummary by cubic
Prevents false “Unity not running” errors by diagnosing after real connection failures and matching the running Unity Editor per project across macOS/Linux/Windows. Detection now tolerates flattened POSIX paths with spaces and trailing slashes, and shows clear guidance when the Editor is closed vs. when the Unity CLI Loop server is stopped.
Bug Fixes
UnityNotRunningErrororUnityServerNotRunningErrorwith dedicated CLI guidance.psoutput and ignores trailing “/” in-projectPathwhile guarding against prefix-only matches.Unity.execase‑insensitively on Windows; ignore non‑Unity tools that use-projectPath.throwFinalToolError; keep port resolution oncustomPortand ignore staleisServerRunning.New Features
ps/PowerShell) plusisUnityEditorProcess;diagnoseRetryableProjectConnectionErrorchecks OS processes per project.AGENTS.mdto require English commit/PR text.Written for commit 46cee42. Summary will update on new commits.
This PR improves Unity connection diagnostics by performing OS-level process inspection after retryable connection failures and moving the final diagnosis to after an actual connection attempt instead of trusting the potentially stale isServerRunning flag.
Key changes
New module: Packages/src/Cli~/src/unity-process.ts
Error classification & port resolution
Connection failure diagnosis & control flow
CLI UX & messages
Tests & verification
Documentation & minor policy change
API / surface changes
Architecture Impact
classDiagram
class UnityNotRunningError {
+projectRoot: string
}
class UnityServerNotRunningError {
+projectRoot: string
}
class ConnectionFailureDiagnosisDependencies {
+findRunningUnityProcessForProjectFn
}
class UnityProcessModule {
+buildUnityProcessCommand()
+parseUnityProcesses()
+tokenizeCommandLine()
+extractUnityProjectPath()
+normalizeUnityProjectPath()
+isUnityProcessForProject()
+isUnityEditorProcess()
+findRunningUnityProcessForProject()
}
class ExecuteToolModule {
+diagnoseRetryableProjectConnectionError()
+throwFinalToolError()
+executeToolCommand()
+listAvailableTools()
+syncTools()
}
ExecuteToolModule --> UnityProcessModule: uses
ExecuteToolModule --> UnityNotRunningError: throws
ExecuteToolModule --> UnityServerNotRunningError: throws
ExecuteToolModule --> ConnectionFailureDiagnosisDependencies: depends on
UnityProcessModule --> RunningUnityProcess: returns
Behavior changes
Notes for reviewers