fix: deduplicate CodeDom assembly references

[kpkhxlgy0]

Description

Fix CodeDom dynamic compilation failures when Unity has multiple loaded assemblies with the same simple name. The observed failure involved System.Security 2.0.0.0 and 4.0.0.0: excluding System.Security entirely avoided the duplicate-reference error but made types defined only in that assembly unavailable.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Refactoring (no functional changes)
• Test update

Changes Made

• Preserve the existing netstandard facade exclusions used by the CodeDom backend.
• Deduplicate remaining CodeDom references by managed assembly simple name.
• Prefer the exact assembly identity referenced by the most loaded assemblies, then the higher version, then a stable path tie-break.
• Preserve System.Security instead of removing it from CodeDom references.
• Add portable EditMode regression coverage using temporary generated assemblies, without project-specific DLL paths.

Compatibility / Package Source

• Unity version(s) tested: Unity 2022.3.23f1 on Windows
• Package source used (#beta, #main, tag, branch, or file:): local file checkout based on CoplayDev/unity-mcp#beta at fc70dda7
• Resolved commit hash from Packages/packages-lock.json (if using a Git package URL): not applicable (file: checkout)

Testing/Screenshots/Recordings

• Python tests (cd Server && uv run pytest tests/ -v)
• Unity EditMode tests
• Unity PlayMode tests
• Package import/compile check
• Not applicable (explain why in Additional Notes)

Validation in the active Unity 2022.3 project:

• Package scripts compiled with no ExecuteCode or error CS Console entries.
• CodeDom selected System.Security, Version=4.0.0.0 from simultaneous 2.0.0.0 and 4.0.0.0 inputs.
• CodeDom compiled and resolved System.Security.Cryptography.ProtectedData from System.Security, Version=4.0.0.0.
• Equivalent temporary-assembly checks preserved a file named System.Security.dll with netstandard present and selected referenced version 1.0.0.0 over unreferenced version 2.0.0.0.

Documentation Updates

• I have added/removed/modified tools or resources
• If yes, I have updated all documentation files using:
  • The LLM prompt at tools/UPDATE_DOCS_PROMPT.md (recommended)
  • Manual review of the generated changes

No tool or resource surface changed.

Related Issues

None.

Additional Notes

The two Unity EditMode regression tests were added to ExecuteCodeTests.cs, but the standalone UnityMCPTests project was not opened or run locally. The active host project was used for package compilation and equivalent runtime checks.

Summary by CodeRabbit

• Bug Fixes

  • Improved CodeDom compilation reliability when projects include multiple versions of the same assembly.
  • Preserved required security assemblies during compilation with netstandard references.
  • Added smarter assembly selection to favor versions referenced by the running Unity environment.
  • Improved repeated compilation performance by reusing filtered assembly paths until the Unity domain reloads.

• Tests

  • Added regression coverage for assembly filtering, duplicate assembly resolution, security assemblies, and cache reuse.

[coderabbitai]

📝 Walkthrough

Walkthrough

CodeDom assembly filtering now caches filtered paths, deduplicates assembly identities, selects referenced or newer versions, and preserves unresolved paths. EditMode tests cover security assembly preservation, duplicate selection, and cache invalidation.

Changes

CodeDom assembly filtering

Layer / File(s)	Summary
Assembly identity filtering MCPForUnity/Editor/Tools/ExecuteCode.cs	CodeDom filtering now caches results, deduplicates resolved assemblies by identity, selects candidates using loaded references and versions, and preserves unresolved paths.
Filtering regression coverage TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs	Added compilation-based tests and helpers for System.Security.dll preservation, duplicate assembly selection, cache reuse, and domain-reload invalidation.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• CoplayDev/unity-mcp#1001: Extends the same ExecuteCode.cs CodeDom assembly-path filtering and test coverage.

Suggested reviewers: scriptwonder

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the primary change: deduplication of CodeDom assembly references.
Description check	✅ Passed	The description covers the required sections, explains the fix, identifies compatibility details, and documents validation and test limitations.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Scriptwonder]

Hi, thanks for the PR and identify the issue. One thing before merge: is it possible to also cache the CodeDomPaths instead of calling getAseemblyName on every path on every execute_code call? Just thinking on ways to optimize this, lmk what you think.

[Scriptwonder]

Nice catch on the root cause, and the version-preference tests are the right shape.

One thing before merge: DeduplicateAssemblyPathsForCodeDom calls AssemblyName.GetAssemblyName() on every path — 200–500 in a typical Editor, each one opening the DLL to read its manifest — on every CodeDom execute_code call. Since assemblies only change across a domain reload, this can just be memoized next to _cachedAssemblyPaths and cleared in the existing OnDomainReload() (ExecuteCode.cs:32-39), which makes it a once-per-reload cost instead of once-per-call.

Guard the memo with ReferenceEquals(allPaths, _cachedAssemblyPaths) so the two new tests keep hitting the uncached path with their own arrays.

Also, the red CI isn't yours: actions/checkout now refuses fork checkouts under pull_request_target, and we haven't set allow-unsafe-pr-checkout yet. Tracking that separately — nothing for you to chase.

[coderabbitai]

🧹 Nitpick comments (1)

TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs (1)

436-444: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use cachedCodeDomAssemblyPathsField to assert cache state directly.

cachedCodeDomAssemblyPathsField is retrieved and null-checked at Line 443 but never read afterward. Adding direct assertions on this field strengthens the regression coverage that the PR's caching fix was meant to exercise: confirm it is null right after OnDomainReload (Line 448 and Line 463), and confirm it holds first after the initial call (Line 455) and after the cache-hit call (Line 460), instead of only inferring cache state indirectly through ReferenceEquals on the return value.

♻️ Example of strengthening the assertions

                 var first = ExecuteCode.FilterAssemblyPathsForCodeDom(cachedAssemblyPaths);
                 Assert.AreEqual(1, first.Length);
+                Assert.AreSame(first, cachedCodeDomAssemblyPathsField.GetValue(null));

                 File.WriteAllText(olderPath, "invalidated");
                 File.WriteAllText(newerPath, "invalidated");
                 var second = ExecuteCode.FilterAssemblyPathsForCodeDom(cachedAssemblyPaths);
                 Assert.AreSame(first, second);

                 onDomainReload.Invoke(null, null);
+                Assert.IsNull(cachedCodeDomAssemblyPathsField.GetValue(null));
                 cachedAssemblyPathsField.SetValue(null, cachedAssemblyPaths);

🤖 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 `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs`
around lines 436 - 444, Update the OnDomainReload test flow in ExecuteCodeTests
to assert cachedCodeDomAssemblyPathsField.GetValue(null) is null immediately
after each domain reload, equals first after the initial cache population, and
still equals first after the cache-hit call. Keep the existing return-value
assertions while adding these direct cache-state checks.

🤖 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.

Nitpick comments:
In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs`:
- Around line 436-444: Update the OnDomainReload test flow in ExecuteCodeTests
to assert cachedCodeDomAssemblyPathsField.GetValue(null) is null immediately
after each domain reload, equals first after the initial cache population, and
still equals first after the cache-hit call. Keep the existing return-value
assertions while adding these direct cache-state checks.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8c5b53d0-97ce-42b7-a552-317a01f2e874

📥 Commits

Reviewing files that changed from the base of the PR and between 6f4c812 and d2247e9.

📒 Files selected for processing (2)

• MCPForUnity/Editor/Tools/ExecuteCode.cs
• TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs

🚧 Files skipped from review as they are similar to previous changes (1)

• MCPForUnity/Editor/Tools/ExecuteCode.cs