feat: honour UNITY_MCP_SERVER_PATH — launch a given server binary, skip download + version match - #975
Merged
Merged
Conversation
…ip download + version match Adds a dev/CI-only override so a workspace-built `gamedev-mcp-server` can be driven end-to-end without cutting a GameDev-MCP-Server release. When `UNITY_MCP_SERVER_PATH` resolves to a file that EXISTS, that file is what `StartServer` launches, `ExecutableFolderPath` (its WorkingDirectory) is the override's own directory, and `IsVersionMatches()` reports true — so `IsBinaryReadyToStart()`, the `DownloadServerBinaryIfNeeded` gate and the post-publish check all short-circuit and `DownloadAndUnpackBinary` is never entered from the editor-startup or package-update paths. Set-but-missing falls through to the pinned release, matching Unreal-MCP's `UNREAL_MCP_SERVER_PATH`. Resolved through the existing `DevControlEnv.Resolve` layer (process env > `<projectRoot>/.env`) rather than a bare `Environment.GetEnvironmentVariable`, so an Editor launched from the GUI or an IDE — which inherits no shell exports — can still pick the override up from a `.env` file. `DownloadAndUnpackBinary` now publishes to a new `CachedExecutableFolderPath` instead of `ExecutableFolderPath`, so the manual `Tools/AI Game Developer/Server/Download Binaries` menu item always lands in `Library/mcp-server/<rid>/` and can never delete and replace the directory the override points at. `ServerVersion` is unchanged and no `.github/**` file is touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bac1LKpVobv1i1FNGCRNvM
Three report-only reviewers plus an in-context pass over the UNITY_MCP_SERVER_PATH override. Everything below was verified from source before it was applied. Product (McpServerManager.cs) - the download path must not read the launch target. Once the override makes ExecutableFullPath / IsBinaryExists() / IsVersionMatches() / GetBinaryVersion() describe the developer's own binary, every existing READER of them has to be re-classified. PublishStagedBinary was moved to CachedExecutableFolderPath already; the verification and the reporting beside it were not, and the manual Tools/AI Game Developer/Server/Download Binaries item still reaches them: - the post-publish verification was VACUOUS under an active override. Both checks were true by construction (the resolver returns a path only after File.Exists passed; IsVersionMatches() short-circuits to true), so a publish that landed nothing in Library/mcp-server/<rid>/ reported success. It now verifies CachedExecutableFullPath and GetCachedBinaryVersion() - the cache it actually wrote to. - the success log named the override as the download destination, contradicting the documented "downloads into Library/ regardless of the override" sentence. - ShowUpdateResultPopup rendered Version: from GetBinaryVersion(), which follows the override's directory - where the tests assert no version marker normally sits, so the popup would read blank after a successful download. - StartServer derives WorkingDirectory from the executablePath local instead of re-resolving ExecutableFolderPath, so FileName and WorkingDirectory cannot end up describing different binaries. Adds a non-public CachedExecutableFullPath (which also removes the duplicated cache composition inside ExecutableFullPath) and GetCachedBinaryVersion(), public so the launch-vs-download contrast can be asserted. IsVersionMatches()'s comment no longer claims the post-publish check as a beneficiary of its short-circuit. Tests (McpServerPathOverrideTests.cs): - [SetUp] MOVES the real <projectRoot>/.env aside instead of deleting it. That file is gitignored, user-owned config this feature's own docs tell developers to create, so git holds no copy and a run killed before [TearDown] destroyed it with the only copy in a managed field. [SetUp] also recovers a file left parked by an earlier killed run, and a failed restore is now a Debug.LogError rather than a silent best-effort catch. - removed two assertions that could not fail: GetBinaryVersion() re-derived from its own body, and IsBinaryReadyToStart() restated as IsBinaryExists() && IsVersionMatches() (whose operands are equal in both environments the suite runs in, so even the && -> || mutation leaves it green). The entailed but DoD-mandated IsBinaryExists() assertions are kept and labelled as documentation, not evidence. - new test for the public projectRootPath overload, which was documented as the unit-test seam and used by no test. - new assertions pinning that an active override does NOT move the download cache, and that the cache version read does not follow the override. Docs: the override notice is logged once per domain load, so it is absent when the .env is written afterwards - the Starting MCP server line is the reliable confirmation and the docs now say so. Adds the relative-path and Open Server Logs notes. Verification: 6 plants, Edit tool, each confirmed live in git diff and reverted after, verdicts read per test from each run's own JSON (unity-mcp-cli exits 0 even when tests fail); every one RED and attributed, final round 7/7 green with Logs: []. Full chunked EditMode suite green. PR body updated with the corrected environment-conditional disclosure (four assertions, not one). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bac1LKpVobv1i1FNGCRNvM
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.
Summary
UNITY_MCP_SERVER_PATH. When it resolves to a file that exists, that file is thebinary the Editor launches, and both the GitHub-release download and the pinned-version match are
skipped. When it is unset — or set to a path that does not exist — nothing changes: the plugin
keeps using
Library/mcp-server/<rid>/and the release pinned byServerVersion(9.2.5, whichthis PR does not touch). Same rule as Unreal-MCP's
UNREAL_MCP_SERVER_PATH, fall-throughincluded.
DevControlEnv.Resolvelayer (process env→<projectRoot>/.env→ unset) rather than a bareEnvironment.GetEnvironmentVariable, so anEditor launched from the GUI or an IDE — which inherits no shell exports — can still pick the
override up from a
.envat the Unity project root.download cache never does. Everything that describes or verifies a download reads the cache
tier explicitly, so a manual
Download Binariesrun still verifies and reports the file itactually wrote.
git diff --stat origin/main -- Unity-MCP-Plugin/Packages/com.ivanmurzak.unity.mcp/Editorlistsonly
Scripts/McpServerManager.cs;git diff --stat origin/main -- .githubis empty.What the override changes, member by member
ResolveServerPathOverride()nullExecutableFullPathLibrary/mcp-server/<rid>/gamedev-mcp-server(.exe)ExecutableFolderPath(StartServer'sWorkingDirectory)Library/mcp-server/<rid>/VersionFullPathLibrary/mcp-server/<rid>/version<override dir>/versionIsVersionMatches()versionmarker and compares toServerVersiontrue(there is no marker beside a workspace-built binary)CachedExecutableFolderPathLibrary/mcp-server/<rid>/Library/mcp-server/<rid>/— never redirectedCachedExecutableFullPathLibrary/mcp-server/<rid>/gamedev-mcp-server(.exe)GetCachedBinaryVersion()Library/mcp-server/<rid>/versionIsBinaryReadyToStart(), theDownloadServerBinaryIfNeededgate and the editor-startup /package-update paths all short-circuit off
IsBinaryExists() && IsVersionMatches(), so under anoverride
DownloadAndUnpackBinaryis never entered from either automatic path.The manual menu item is the one download path that stays live under an override, and the bottom
three rows of that table are what keep it honest.
MenuItems.DownloadServercallsDownloadAndUnpackBinarydirectly, bypassing that gate, so with an override set it still runs —and every member that describes the launch target is, by construction, true about the developer's
own binary. So the download path publishes to
CachedExecutableFolderPath, verifiesCachedExecutableFullPath+GetCachedBinaryVersion(), and reports the same, rather thanExecutableFullPath/IsBinaryExists()/IsVersionMatches()/GetBinaryVersion(). Had it keptreading the launch tier,
Download Binarieswould have reported success however the publish went(the override file exists because the resolver only returns it after
File.Existspassed, andIsVersionMatches()short-circuits totrue), logged the override path as the downloaddestination, and shown a blank
Version:in the result popup. This is the same reasoning that movedPublishStagedBinaryoffExecutableFolderPath: without it, a manual download would have deletedand replaced the developer's own override directory, and the documented sentence "downloads into
Library/regardless of the override; the override still wins at launch" would have been false.Evidence
1. Behavioural proof against the real launch path
Unity 2022.3.62f3, this repo's own
Unity-MCP-Pluginproject in a fresh worktree whoseLibrary/did not exist. Override target: a locally published server —dotnet publish shared/GameDev-MCP-Server/com.IvanMurzak.GameDev.MCP.Server.csproj -c Release -r win-x64 --self-contained true -p:PublishSingleFile=true→ProductVersion 9.2.6+62a6b7d1,i.e. deliberately not the pinned
9.2.5, at a path outsideLibrary/.Half 1 —
UNITY_MCP_SERVER_PATHset (Editor.log, verbatim, colour tags stripped):grep -c 'Downloaded and unpacked' Editor.log→ 0grep -c 'Deleted existing MCP server folder' Editor.log→ 0Unity-MCP-Plugin/Library/mcp-server→ does not existgamedev-mcp-server.exeExecutablePath = ...\.agent-scratch\chain-server\gamedev-mcp-server.exewait-for-readyexit 0, SignalRnegotiatereturned 200 with aconnectionId).Half 2 — the plant the brief asks for: same Editor, variable unset.
Library/mcp-server/win-x64/now exists with aversionmarker reading9.2.5, and theoverride activeline is absent from that run. So the launch target moved with the variable inboth directions, and the download only happened when the override was gone.
2. Plant rounds — six plants, each RED attributed from its own run
Applied with the
Edittool, each confirmed live ingit diffbefore the run and reverted after;assets-refreshbetween every plant and its run. Every round rantests-run --input {"testMode":"EditMode","testClass":"McpServerPathOverrideTests",...}and theverdict is read per test from the run's own JSON —
unity-mcp-cliexits0even when testsfail, so an exit status could not have told these apart.
IsVersionMatches()override short-circuitOverride_ExistingFile_IsWhatGetsLaunched("the override must skip the pinned-release version match"),Override_SkipsVersionMatch_EvenWithAMismatchedVersionMarkerBesideIt,Override_ResolvesFromProjectDotEnv_WhenTheProcessEnvIsUnsetFile.Exists(raw)check inResolveServerPathOverrideOverride_SetButMissingFile_FallsThroughToThePinnedRelease("a set-but-missing override must not resolve (the Unreal rule)")ExecutableFolderRootPathliteral"mcp-server"→"mcp-server-planted"NoOverride_KeepsThePinnedLibraryCache_AndTheVersionMarkerCheck, the pinned-cache assertion inOverride_SetButMissingFile_FallsThroughToThePinnedRelease, and the cache assertion inOverride_ExistingFile_IsWhatGetsLaunchedDevControlEnv.Resolve(...)→Environment.GetEnvironmentVariable(...)Override_ResolvesFromProjectDotEnv_WhenTheProcessEnvIsUnsetandResolveServerPathOverride_ReadsTheDotEnvOfTheGivenProjectRoot(bothBut was: null)CachedExecutableFolderPathfollow the overrideGetCachedBinaryVersion()readsVersionFullPathinstead of<cache>/versionBut was: "0.0.0-not-the-pinned-version")Logs: []P1 and P2 are the two the brief mandates. P3 and P4 exist because two other claims would otherwise
have been unfalsifiable: P3 proves the no-override test really pins
Library/mcp-server/<rid>/rather than comparing a value with itself, and P4 proves the
.envlayer is load-bearing ratherthan incidental. P5 and P6 cover the download/launch tier split — they are the plants for behaviour
this PR changed in production code, derived from the change rather than from an existing test.
P5 and P6 attack one claim at two sites and share a red with identical text. Both redden
"the download-cache version read must NOT follow the override" verbatim. They are told apart by
their failure SET, not by that line: P5 additionally reddens the cache-folder assertion in
Override_ExistingFile_IsWhatGetsLaunched; P6 reddens the version assertion alone. Stated ratherthan papered over, because a future red on that marker alone means P6's site, not P5's.
What makes each assertion able to fail (stated because a check that cannot fail scores green
with the feature deleted):
versionmarker, and the test asserts that(
Assert.IsFalse(File.Exists(VersionFullPath)),Assert.IsNull(GetBinaryVersion())) beforeasserting
IsVersionMatches()istrue— so thattruecan only come from the short-circuit.A sibling test goes further and puts a marker reading
0.0.0-not-the-pinned-versionbeside theoverride, asserts the marker is readable and disagrees with
ServerVersion, and still requiresIsVersionMatches()to betrue. That same fixture is what makes P6 discriminate: with themismatched marker beside the override, a cache read that followed the override returns it.
[SetUp]neutralises bothsources — the process env var and
<projectRoot>/.env— and[TearDown]restores them), thenasserts equality with it and equality with an independently re-derived
Library/mcp-server/<rid>/gamedev-mcp-server.exe, so "unchanged" cannot be satisfied by nothinghaving been read at all.
.envtests assert the process env var is empty and that nothing resolves before the fileis written, so neither can pass on a process-env read.
Environment-conditional assertions — four of them, not one. Corrected here after the review
pass measured it; the earlier revision of this section understated the count.
Library/cache)NoOverride…:IsVersionMatches() == (marker == ServerVersion)false == false, and an unconditionaltruereddens)true)SetButMissing…:IsBinaryExists()unchangedSetButMissing…:IsVersionMatches()unchangedSetButMissing…:IsBinaryReadyToStart()unchangedThe suite is sound regardless, because the path equalities in those same tests — and the
cache-folder assertions P3 and P5 redden — discriminate in every environment. No assertion is
relied on where it cannot fail, and the two assertions that could not discriminate anywhere
(
GetBinaryVersion()re-derived from its own body, andIsBinaryReadyToStart()restated asIsBinaryExists() && IsVersionMatches()) were removed rather than left reading as coverage.3. Local suites
Unity-MCP-Plugin(2022.3.62f3): see the"Suites" line below.
Editor/(Editor-only assembly)and
docs/; nothing is compiled into a player.cli/changed..github/**change), butpython .github/scripts/check_nuget_gate.pywas run anyway and exits 0:NuGet gate OK: 15 pins, generation UNITY_MCP_DEPS_3, propagation consistent.Review pass (
/code-review+/simplify)Three report-only reviewers over the full diff, plus an in-context pass. Everything below was
verified from source before it was applied; findings whose prescription was wrong are recorded as
such rather than followed.
Applied — three real defects, all on the manual-download path, all invisible unless the override
is active (detailed in the member table above): the post-publish verification was vacuous, the
success log named the override instead of the download destination, and the result popup read the
version marker from the override's directory. Plus one consistency fix:
StartServernow derivesWorkingDirectoryfrom theexecutablePathlocal instead of re-resolvingExecutableFolderPath,so
FileNameandWorkingDirectorycannot describe different binaries.Applied — test hardening.
[SetUp]used toFile.Deletethe real<projectRoot>/.envwith theonly copy in a managed field. That file is gitignored, user-owned config that this feature's own
docs tell developers to create, so git holds no copy and a run killed before
[TearDown]— adomain reload, a cancelled run, an editor crash — would have destroyed it. It is now moved aside
to a sibling path and moved back,
[SetUp]recovers a file left parked by an earlier killed run,and a failed restore is a
Debug.LogError(which fails the test) instead of a silentcatch { /* best effort */ }.Applied — one new test.
ResolveServerPathOverride(string? projectRootPath)is public anddocumented as the unit-test seam, and no test used it. It has one now, reading an arbitrary root's
.envfrom a temp directory — which also pins that the root is an argument rather than the liveproject.
Not applied, recorded instead:
DevControlEnv, where the two sibling dev-only vars andtheir resolvers live. Genuinely the better home; out of bounds, since the brief fixes
McpServerManager.csas the only product file..envtests to use the seam against a temp root instead of the real project root.That would drop the end-to-end coverage of
ExecutableFullPathfollowing<projectRoot>/.env,which is exactly what the brief asks the
.envcase to prove. The seam got its own test instead.UNITY_MCP_SERVER_PATHis set but the file is missing. The silentfall-through is the ruled Unreal semantics; a new unverified log line late in review is not worth
it. The docs now point at the
Starting MCP server: <path>line as the reliable confirmationinstead — which is also why the
override activenotice is no longer described as authoritative:it is emitted once per domain load, so it is absent if you write the
.envafterwards even thoughthe override still applies.
is on a per-frame path (checked: no
EditorApplication.updatesubscriber reads these memberswithout self-unsubscribing, no
OnGUI, noschedule.Executeloop).Not done, on purpose
ServerVersion(McpServerManager.cs) is unchanged.MenuItems.DownloadServerstill downloads intoLibrary/, which is documented.Editor/DependencyResolver/**,NuGetConfig.cs, theAssets/Plugins/NuGetdrops,cli/**,Runtime/**,UpdateCheckerand every UI file are untouched.package.jsonwas bumped.DownloadAndUnpackBinary's own body has no EditMode coverage — it downloads — so thepost-publish fix is verified through the members it now reads (
CachedExecutableFolderPath,GetCachedBinaryVersion(), planted P5/P6) rather than by exercising the download itself.Test plan
McpServerPathOverrideTests— 7 tests, 0 failures,Logs: [].check_nuget_gate.pyexit 0.test.md).CI
test-pull-request) on05be00d2f137907414582d362e9836f8322e5dcc: id33815830447—https://github.com/IvanMurzak/Unity-MCP/actions/runs/33815830447 — 17/17 checks green:
nuget gate,test-cli (20),test-cli (22), the twelve licensedtest-unity-*legs(2022.3.62f3 / 2023.2.22f1 / 6000.3.1f1 x editmode/standalone x base/windows-mono),
Test Resultsandsave-event-file. SHA-tied: that run'sheadShaequals this PR'scurrent
headRefOid, so it covers the refine commit as well as the implement commit.80143639: id33807352185—https://github.com/IvanMurzak/Unity-MCP/actions/runs/33807352185 — all 17 checks green
(SHA-tied:
801436398b953ad345e454b3f719fdc5b96a65c9). Retained for history only; it doesNOT cover the refine commit. The authoritative run above is the one this PR is gated on.
This is a same-repository branch, so the twelve licensed
test-unity-*legsreceive the repository secrets (the late-August red runs on this repo were fork PRs, per docs(ci): the red test-unity legs are fork PRs with no secrets, not a regression #974).