Repository navigation
fix: restore TerminalLauncher tests, Windows prerelease source match, read_console log types - #1477
Merged
Merged
Conversation
…s paths Prerelease packages expect "mcpforunityserver>=0.0.0a0". PathsEqual passed it to Path.GetFullPath, which throws on '>' in Unity's Mono on Windows, so the comparison returned false. JSON clients, Codex and OpenClaw then reported VersionMismatch for a correct stdio config, and the startup auto-rewrite rewrote it every session.
InferTypeFromMessage searched the whole message, including the appended stack trace. With Full stack traces the frames contain "System.Exception&", so a Debug.Log was typed Exception and returned by types=["error"] (the second bug in #1433). Body text such as "TimeoutException" in a plain log did the same. The mode bit table was fixed in #1348, so read the type from it alone.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 5 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarizes the three main changes: restoring TerminalLauncher tests, fixing Windows prerelease source matching, and correcting read_console log types. Description check The description is detailed, relevant, and covers the changes, affected behavior, test results, supported Unity versions, known unrelated failures, and verification limits. It does not follow every te… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is detailed, relevant, and covers the changes, affected behavior, test results, supported Unity versions, known unrelated failures, and verification limits. It does not follow every template heading or checklist, but it provides the required review context.
-
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused fixes have regression coverage, and no blocking issues were identified.
No open issues 🎉
What changed in this PR
This PR fixes package-source matching and console log classification in the Unity Editor bridge, and restores previously removed terminal-launcher tests.
Changes:
- Match identical package sources before filesystem normalization.
- Classify console entries using Unity’s mode bits rather than message text.
- Restore terminal-launcher coverage and add regression tests for both fixes.
| File | Description |
|---|---|
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs | Tests log filtering with Full stack traces. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Server/TerminalLauncherTests.cs | Restores launcher tests, including Windows stdin redirection. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Clients/CheckStatusUrlPropertyTests.cs | Tests prerelease source recognition. |
| MCPForUnity/Editor/Tools/ReadConsole.cs | Removes text-based severity inference. |
| MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs | Compares identical source strings before normalizing paths. |
Review effort: Balanced
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
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.
Three maintainer fixes, one commit each.
1. Restore the TerminalLauncher tests removed with #1451
The cleanup commit in #1451 (c1f9344) deleted the whole
TerminalLauncherTests.cs(375 lines), not just the Unicode-path case the PR added. That removed the 24 tests that existed before, including the #1279 guardCreateHeadlessProcessStartInfo_RedirectsStdinFromNul, and left an orphan.metaon beta. This restores the file exactly as it was before #1451 (from 31429ff). #1451's own test is not brought back.2. Match identical package sources before comparing them as paths
Prerelease packages expect
--from mcpforunityserver>=0.0.0a0.McpConfigurationHelper.PathsEqualpassed both values toPath.GetFullPath, which throws on>in Unity's Mono on Windows, and the catch returned false. As a result:McpClientConfiguratorBase.cs:254), Codex (:498) and OpenClaw (OpenClawConfigurator.cs:347) reported VersionMismatch: "Server version doesn't match the plugin" for a correct stdio config.macOS/Linux and stable pins (
==x.y.z) were not affected. The fix compares the trimmed text first, so every caller is fixed in one place. Local paths still go throughGetFullPath.3. read_console: take the log type from Unity's mode bits
InferTypeFromMessagesearched the whole message, including the appended stack trace, and its result won over the mode bits for anyDebug.Log. With Full stack traces the frames containSystem.Exception&, so aDebug.Logcame back as Exception and was returned bytypes: ["error"]. This is the second bug in #1433, which closed when #1443 fixed the first. Body text did the same in every stack-trace mode: aDebug.Logcontaining "TimeoutException", "LogError" or "Assertion" was typed Exception/Error/Assert. The mode-bit table was corrected in #1348, so the type now comes from it alone, and the text scan is deleted.Part of #1433.
Verification (local, Windows)
Each new test was run on the code before the fix and on the fixed code:
CheckStatus_Configured_WhenStdioUsesPrereleasePackageRange, old codeHandleCommand_Get_KeepsDebugLogOutOfErrors_WithFullStackTrace, old codeA temporary probe (not committed) logged Debug.Log, LogWarning, LogError, LogException and LogAssertion, plus Log/Warning/Error bodies containing those keywords, under Full, ScriptOnly and None stack traces. That is 10 cases × 3 modes on each version.
Full EditMode suite on 2021.3 with the fix (probe excluded): 1564 tests, 1484 passed, 79 skipped, 1 failed. The failure is
ExecPathBatchShimTests.FindAllInPath_ReturnsEveryMatchInPathOrder, a local artifact: this machine's TEMP is an 8.3 short path andwhereprints long paths. It fails on beta too.Unity 6000.6.4f1: the package compiles. The test project does not compile there: existing tests call
GetInstanceID(), which 6000.6 marks obsolete as an error, andcom.unity.ide.windsurfalso fails. Both are unrelated to this PR, and 6000.5+ is outside the CI matrix.Not verified at runtime: a real C# compile error read through read_console. The mode table maps its bit (ScriptCompileError, 1<<11), and that table is unit-tested.
Summary by CodeRabbit