fix: Improve Unity reload recovery and skill setup reliability#1150
Merged
Conversation
The v3 branch still treated grouped skill directories as the command default, which kept freshly installed skills outside the standard agent discovery path. Route list and install through the flat layout by default, and make public uninstall remove both managed layouts so stale grouped copies do not survive the migration.
Adapt the main-branch Unity fixes to the v3 server split so compiler discovery, Unity object identifiers, and runtime assembly targets match newer Unity layouts. Route domain-reload cleanup through the v3 lifecycle hook so dynamic-code shutdown is signaled without blocking reload teardown.
Mark the demo test-support assembly as Editor-only because it depends on package assemblies that are intentionally excluded from player builds. Add an assembly-definition guard so future non-test assemblies cannot reference Editor-only package code without also targeting the Editor platform.
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces a domain-reload lifecycle interface for server cleanup, migrates hierarchy node identifiers from integers to strings, switches object reference serialization from instanceId to entityId with culture-invariant formatting, refactors external compiler path resolution with multi-SDK support, and adds default flat-layout behavior for skills management along with assembly dependency constraint tests. ChangesDomain Reload Lifecycle and Server Shutdown
Hierarchy Node Identifier Migration
Object Reference Serialization to EntityId
External Compiler Path Resolution
Skills Layout Management
Assembly Dependency Constraints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Assets/Tests/Editor/DynamicCodeToolTests/ExternalCompilerPathResolverTests.cs (1)
151-163: ⚡ Quick winAdd a regression test for mixed-layout fallback (stale legacy + valid DotNetSdk).
This will protect the resolver behavior when
DotNetSdkRoslynexists as a directory but the usable compiler is only underDotNetSdk/sdk/.../Roslyn/bincore.Suggested test
+ [Test] + public void ResolveCompilerDirectoryPath_WhenLegacyDirectoryExistsButCscIsMissing_ShouldFallbackToDotNetSdkLayout() + { + string scriptingRootPath = CreateDirectory("Scripting"); + CreateDirectory(Path.Combine("Scripting", "DotNetSdkRoslyn")); // intentionally empty + string expectedCompilerDirectoryPath = CreateDirectory(Path.Combine("Scripting", "DotNetSdk", "sdk", "8.0.318", "Roslyn", "bincore")); + + string resolvedCompilerDirectoryPath = ExternalCompilerPathResolver.ResolveCompilerDirectoryPath(scriptingRootPath); + + Assert.That(resolvedCompilerDirectoryPath, Is.EqualTo(expectedCompilerDirectoryPath)); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/Tests/Editor/DynamicCodeToolTests/ExternalCompilerPathResolverTests.cs` around lines 151 - 163, Add a regression test in ExternalCompilerPathResolverTests that ensures ExternalCompilerPathResolver.ResolveCompilerDirectoryPath falls back to the highest-versioned sdk Roslyn bincore when a stale legacy DotNetSdkRoslyn directory exists; create a test that (1) creates a "DotNetSdkRoslyn" legacy directory alongside a "DotNetSdk/sdk/{olderVersion}/Roslyn/bincore" and a newer "DotNetSdk/sdk/{newerVersion}/Roslyn/bincore" (and optionally a "current" symlink/dir), (2) calls ResolveCompilerDirectoryPath(scriptingRootPath), and (3) asserts the resolved path equals the newer sdk Roslyn bincore path so the resolver prefers the highest sdk version over the stale DotNetSdkRoslyn directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/ExternalCompilerPathResolver.cs`:
- Around line 201-207: The current fast path returns legacyCompilerDirectoryPath
when Directory.Exists(DotNetSdkRoslynDirectoryName) is true, but that can be a
stale/partial folder; change the check to validate the legacy folder is usable
(e.g., contains expected compiler artifacts such as csc.dll or the "bincore"
subdirectory) before returning it; if those expected files/subdirs are missing
or invalid, call ResolveDotNetSdkCompilerDirectoryPath(scriptingRootPath)
instead (use the symbols legacyCompilerDirectoryPath,
DotNetSdkRoslynDirectoryName, ResolveDotNetSdkCompilerDirectoryPath, and
Directory.Exists to locate and implement the check).
In
`@Packages/src/Editor/FirstPartyTools/GetHierarchy/HierarchyAnalyzer/HierarchyService.cs`:
- Around line 382-393: Replace the unsafe conversion using EntityId.ToULong in
GetObjectId: instead of calling UnityEngine.EntityId.ToULong(obj.GetEntityId()),
call ToString() on the EntityId returned by obj.GetEntityId() (e.g., use
obj.GetEntityId().ToString()) so you rely on the EntityId public API rather than
converting to integer types; keep the Assert and CultureInfo usage where
applicable but remove the ToULong conversion.
---
Nitpick comments:
In
`@Assets/Tests/Editor/DynamicCodeToolTests/ExternalCompilerPathResolverTests.cs`:
- Around line 151-163: Add a regression test in
ExternalCompilerPathResolverTests that ensures
ExternalCompilerPathResolver.ResolveCompilerDirectoryPath falls back to the
highest-versioned sdk Roslyn bincore when a stale legacy DotNetSdkRoslyn
directory exists; create a test that (1) creates a "DotNetSdkRoslyn" legacy
directory alongside a "DotNetSdk/sdk/{olderVersion}/Roslyn/bincore" and a newer
"DotNetSdk/sdk/{newerVersion}/Roslyn/bincore" (and optionally a "current"
symlink/dir), (2) calls ResolveCompilerDirectoryPath(scriptingRootPath), and (3)
asserts the resolved path equals the newer sdk Roslyn bincore path so the
resolver prefers the highest sdk version over the stale DotNetSdkRoslyn
directory.
🪄 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: bf04cc57-f6ab-4b52-b4e3-39421593b880
⛔ Files ignored due to path filters (5)
Assets/Tests/Demo/uLoopMCP.Tests.Demo.asmdefis excluded by none and included by nonePackages/src/Cli~/dist/darwin-amd64/uloopis excluded by!**/dist/**and included by nonePackages/src/Cli~/dist/darwin-arm64/uloopis excluded by!**/dist/**and included by nonePackages/src/Cli~/dist/windows-amd64/uloop.exeis excluded by!**/dist/**,!**/*.exeand included by nonePackages/src/Runtime/uLoopMCP.Runtime.asmdefis excluded by none and included by none
📒 Files selected for processing (24)
Assets/Tests/Editor/DomainReloadRecoveryUseCaseTests.csAssets/Tests/Editor/DynamicCodeToolTests/DynamicCodeExecutionFacadeTests.csAssets/Tests/Editor/DynamicCodeToolTests/ExternalCompilerPathResolverTests.csAssets/Tests/Editor/FindGameObjectsToolTests.csAssets/Tests/Editor/HierarchySerializerTests.csAssets/Tests/Editor/OnionAssemblyDependencyTests.csAssets/Tests/Editor/UnityCliLoopServerControllerStartupLockTests.csAssets/Tests/Editor/UnityCliLoopServerStartupProtectionTests.csPackages/src/Cli~/internal/cli/skills.goPackages/src/Cli~/internal/cli/skills_targets.goPackages/src/Cli~/internal/cli/skills_test.goPackages/src/Editor/Application/UnityCliLoopServerApplicationService.csPackages/src/Editor/Application/UseCases/DomainReloadRecoveryUseCase.csPackages/src/Editor/CompositionRoot/UnityCliLoopApplicationRegistration.csPackages/src/Editor/CompositionRoot/UnityCliLoopFirstPartyServerLifecycleBinding.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCodeServices.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/ExternalCompilerPathResolver.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/ExecuteDynamicCodeEditorStartup.csPackages/src/Editor/FirstPartyTools/FindGameObjects/GameObjectFinder/ComponentPropertySerializer.csPackages/src/Editor/FirstPartyTools/FirstPartyToolsEditorStartup.csPackages/src/Editor/FirstPartyTools/GetHierarchy/HierarchyAnalyzer/HierarchyNode.csPackages/src/Editor/FirstPartyTools/GetHierarchy/HierarchyAnalyzer/HierarchySerializer.csPackages/src/Editor/FirstPartyTools/GetHierarchy/HierarchyAnalyzer/HierarchyService.csPackages/src/Editor/Infrastructure/Server/UnityCliLoopServerController.cs
![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 29 files
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~/internal/cli/skills.go">
<violation number="1" location="Packages/src/Cli~/internal/cli/skills.go:302">
P1: Uninstall in flat mode is not layout-scoped; it removes skill directories from both flat and grouped layouts.
(Based on your team's feedback about scoping uninstall operations to the selected layout.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
Address PR review findings by validating legacy compiler folders before selecting them, using the Unity EntityId string API instead of integer conversion, and keeping skills uninstall scoped to the selected layout.
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
User Impact
Changes
Verification