fix: config priority — flags > env vars > on-disk config > defaults#689
Merged
Conversation
The plugin's startup config loader treated UserSettings/AI-Game-Developer-Config.json as authoritative even when overrides were supplied via UNITY_MCP_* environment variables or short flags such as --url / --token / --auth. In a worktree with a fresh local MCP server (token from .worktree.env), the editor would still boot in connectionMode=Cloud (from stale on-disk config), route the env-var token to CloudToken instead of LocalToken, and surface a "Cloud mode requires authentication" dialog while attempting to authenticate against the cloud. This commit implements the standard layered priority flags > env vars > on-disk config > built-in defaults with the following concrete changes: * EnvironmentUtils.ApplyEnvironmentOverrides is refactored to take optional args / env readers (testable) and to return an OverrideRecord that captures the disk-baseline value for every overridden field. * New env vars: UNITY_MCP_CONNECTION_MODE; UNITY_MCP_CLOUD_URL is now also read as the host override (with trailing-slash defence). * New short flag aliases: --url, --token, --auth (highest priority). * Loopback host URLs (localhost, 127.0.0.0/8, ::1) infer ConnectionMode=Custom unless an explicit UNITY_MCP_CONNECTION_MODE override says otherwise — fixes the worktree case where a local-dev URL was used with stale Cloud mode. * Token override routes to LocalToken or CloudToken based on the post-mode- override ConnectionMode and tracks the specific backing field, so the baseline restore on Save targets the same field that was clobbered. * UnityMcpPluginEditor.Save now temporarily restores baseline values before serialising and re-applies overrides afterwards, ensuring runtime-only overrides NEVER leak into the persisted JSON. Existing Save callers (UI flows, configurators) are unaffected — the in-memory config keeps its active runtime values once Save returns. * Adds 20 EditMode tests in Packages/com.ivanmurzak.unity.mcp/Tests/Editor/Environment/EnvironmentUtilsTests.cs covering disk-only, env-only, flag-vs-env precedence, per-field layering, trailing-slash robustness, loopback inference, baseline persistence round-trip, and quoted-value sanitisation. Lets the .claude/pipeline/workflows/implement-task/targets/unity-mcp/setup.md pipeline drop its on-disk JSON pinning workaround for the worktree case.
Contributor
There was a problem hiding this comment.
Pull request overview
Implements a layered startup configuration loader for the Unity MCP plugin so that runtime overrides (CLI flags and UNITY_MCP_* env vars) take precedence over on-disk config, while ensuring those runtime overrides do not get persisted back into UserSettings/AI-Game-Developer-Config.json.
Changes:
- Added a layered override system in
EnvironmentUtils(args > env > disk) that returns anOverrideRecordcapturing baseline vs override values. - Updated
UnityMcpPluginEditorto store runtime overrides and modifiedSave()to serialize disk-baseline values while keeping runtime overrides active in memory. - Added EditMode tests covering override precedence, loopback inference, sanitization, and persistence behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Unity-MCP-Plugin/Packages/com.ivanmurzak.unity.mcp/Runtime/Utils/EnvironmentUtils.cs | Implements layered override resolution, loopback inference, and baseline/override re-application helpers. |
| Unity-MCP-Plugin/Packages/com.ivanmurzak.unity.mcp/Editor/Scripts/UnityMcpPluginEditor.cs | Captures the runtime override record during editor plugin construction. |
| Unity-MCP-Plugin/Packages/com.ivanmurzak.unity.mcp/Editor/Scripts/UnityMcpPluginEditor.Config.cs | Adjusts Save() to avoid persisting runtime overrides to disk via baseline/override swapping. |
| Unity-MCP-Plugin/Packages/com.ivanmurzak.unity.mcp/Tests/Editor/Environment/EnvironmentUtilsTests.cs | Adds test coverage for layered override semantics and persistence guarantees. |
| Unity-MCP-Plugin/Packages/com.ivanmurzak.unity.mcp/Tests/Editor/Environment/EnvironmentUtilsTests.cs.meta | Unity meta for the new test file. |
| record.Track(FieldHost, config.LocalHost, host); | ||
| config.LocalHost = host; | ||
| _logger.LogInformation("[MCP] Override: {Key}={Value}", FieldHost, host); | ||
| } |
There was a problem hiding this comment.
--url / UNITY_MCP_HOST overrides are currently only applied to config.LocalHost. When ConnectionMode remains Cloud, UnityConnectionConfig.Host ignores LocalHost (uses CloudServerUrl), and UnityConnectionConfig.CloudServerBaseUrl currently reads only UNITY_MCP_CLOUD_URL (no --url alias, no UNITY_MCP_HOST alias). Result: --url https://… will not actually change the Cloud endpoint (and DeviceAuthFlow.StartAsync(CloudServerBaseUrl, ...) will still target the default cloud URL). Consider updating the cloud URL resolution to honor the same layered sources (e.g., check the url flag alias and UNITY_MCP_HOST alongside UNITY_MCP_CLOUD_URL), so the documented precedence applies in Cloud mode too.
Suggested change
| } | |
| } | |
| // Mirror the resolved host into the canonical cloud URL key so cloud-mode | |
| // consumers that read UNITY_MCP_CLOUD_URL observe the same effective value | |
| // as the layered override contract (--url / UNITY_MCP_HOST / UNITY_MCP_CLOUD_URL). | |
| if (!string.Equals(Environment.GetEnvironmentVariable(EnvCloudUrl), host, StringComparison.Ordinal)) | |
| { | |
| Environment.SetEnvironmentVariable(EnvCloudUrl, host, EnvironmentVariableTarget.Process); | |
| } | |
| // Keep the legacy host key aligned within the current process as well. | |
| if (!string.Equals(Environment.GetEnvironmentVariable(EnvHost), host, StringComparison.Ordinal)) | |
| { | |
| Environment.SetEnvironmentVariable(EnvHost, host, EnvironmentVariableTarget.Process); | |
| } |
Comment on lines
+165
to
+188
| // Runtime-only overrides (env vars / CLI flags) MUST NOT be persisted to disk. | ||
| // Temporarily restore the disk-baseline values for any overridden field, serialize, | ||
| // then re-apply the overrides so the in-memory config keeps its runtime values. | ||
| // The "in-memory keeps overrides" property is critical: callers (UI, configurators, | ||
| // SignalR client) hold references to unityConnectionConfig and continue reading | ||
| // from it after Save returns. Restoring overrides ensures behaviour is unchanged. | ||
| var hasRuntimeOverrides = RuntimeOverrides != null && RuntimeOverrides.HasAny; | ||
| if (hasRuntimeOverrides) | ||
| EnvironmentUtils.ApplyBaseline(unityConnectionConfig, RuntimeOverrides!); | ||
| try | ||
| { | ||
| var json = JsonSerializer.Serialize(unityConnectionConfig, new JsonSerializerOptions | ||
| { | ||
| WriteIndented = true, | ||
| PropertyNamingPolicy = JsonNamingPolicy.CamelCase, | ||
| Converters = { new JsonStringEnumConverter() } | ||
| }); | ||
| File.WriteAllText(AssetsFileAbsolutePath, json); | ||
| } | ||
| finally | ||
| { | ||
| WriteIndented = true, | ||
| PropertyNamingPolicy = JsonNamingPolicy.CamelCase, | ||
| Converters = { new JsonStringEnumConverter() } | ||
| }); | ||
| File.WriteAllText(AssetsFileAbsolutePath, json); | ||
| if (hasRuntimeOverrides) | ||
| EnvironmentUtils.ApplyOverrides(unityConnectionConfig, RuntimeOverrides!); | ||
| } |
There was a problem hiding this comment.
The baseline-restore approach in Save() can prevent users from persisting edits to fields that were overridden at startup. Example: if LocalHost was overridden via env/flags, the UI lets users change LocalHost and calls Save(), but ApplyBaseline() will revert the value back to the original disk baseline before serialization and ApplyOverrides() will then re-apply the runtime override—so the user’s new value is neither saved to disk nor retained in memory. Consider updating the RuntimeOverrides record per-field before serialization (e.g., if the current config value no longer matches OverrideValues[field], treat it as a user edit and update BaselineValues[field] to the current value so it can be persisted), or serialize from a copy that uses the updated disk-layer values without mutating/reverting the live config.
1 task
Contributor
Test Results 12 files 480 suites 41m 14s ⏱️ Results for commit 81779d0. ♻️ This comment has been updated with latest results. |
- Use nameof() for all OverrideRecord field keys (rename-safe) - Expose OverrideRecord dictionaries as IReadOnlyDictionary - Throw on unknown override field in ApplyValues (fail-loud on missed Track call) - Restore LocalHost null baseline to DefaultHost instead of empty string - Remove WHAT-only section comments simplify-pass: 1
IvanMurzak
added a commit
that referenced
this pull request
Apr 29, 2026
* feat(cli): add close subcommand for Unity Editor Adds `unity-mcp-cli close <path>` to gracefully terminate the Unity Editor instance running for a given project. Symmetric counterpart of `open` — lets scripted workflows (CI agents, pipeline executors, integration test fixtures) reach a clean tear-down state without unsafe OS-level kills. Resolution strategy: read PID from `<project>/Temp/UnityLockfile` (little-endian uint32, first 4 bytes), cross-check via process enumeration to handle stale lock files. On match, send the polite-quit signal — `SIGTERM` on Linux/macOS, `taskkill` (no `/F`) on Windows — then poll every 250ms until the process exits or `--timeout` elapses (default 30s, configurable). On timeout with `--force`, fall back to `SIGKILL` / `taskkill /F`. Refuses to act on any path that is not a Unity project root (`ProjectSettings/ProjectVersion.txt` must exist) — protects against accidental kill-all-Unity-on-host invocations. Idempotent: closing an already-closed Editor exits 0 with a clear no-op message. The graceful-quit path uses an OS signal rather than a plugin-side `editor-quit` MCP tool. Defining that tool would require touching plugin code currently being refactored under PR #689 (config-loader priority). The OS path is sufficient for the issue's acceptance criteria — `SIGTERM` triggers Unity's normal exit hooks (autosave, asset-import finalisation, plugin disconnect). A plugin-hub graceful path can be added in a follow-up once #689 lands. Test coverage (Vitest): 38 tests, 37 active + 1 gated. Unit tests cover path normalisation, refusal on non-Unity paths, idempotent no-op, flag parsing, lockfile PID parsing (including stale-byte and zero-PID edge cases), cross-platform shutdown signal selection, and the `waitForExit` poll loop. The live-editor integration test is gated behind `UMCP_LIVE=1` so CI does not require Unity bring-up. Closes #688 * refactor: simplify close subcommand per review pass 1 Hoist findUnityProcess in resolveEditorPid so the stale-lockfile path no longer pays for two enumerations. Add a TOCTOU note near signal delivery. Clarify parseTimeoutSeconds(undefined) is test-only. Switch isProcessAlive to try process.kill(pid, 0) first on Windows (works via OpenProcess) before falling back to tasklist; parse the tasklist CSV field-wise (column 1 = PID) so digit runs in other columns can't yield false positives. Drop the dead Number.isFinite guard after readUInt32LE. Document the Windows session 0 / WM_CLOSE caveat on sendGracefulShutdown. Live-editor smoke test now asserts "exited cleanly" and forbids "no running Editor", so a missing precondition fails loudly. Sentence-case the "Not a Unity project root" refusal message for consistency with the sibling errors. Note the headless / session 0 caveat in the README. simplify-pass: 1 * refactor: simplify resolveEditorPid docstring per review pass 2 The "cheapest first / fall back to enumerating" framing no longer matched the post-hoist control flow (enumeration is now unconditional and the lockfile picks between lockPid and proc.pid). Reword the JSDoc so it describes what the code actually does. simplify-pass: 2 * refactor(cli): handle ESRCH races and symlink paths in close sendGracefulShutdown / sendForceKill now treat "process gone" errors as success by re-checking isProcessAlive in the catch — preserves the PR's idempotency guarantee when Unity exits between PID resolution and signal delivery. resolveEditorPid falls back to a path.resolve match when realpath canonicalisation finds no enumerated process, fixing the silent no-op when Unity is launched with a symlinked project path. Live-editor smoke test now passes timeoutMs=80000 to runCli so the 15s exec timeout no longer kills the runner before the 60s polite-quit window elapses. Comment typo and PID-0 test comment fixes. simplify-pass: 1 * refactor(cli): clarify tasklist parser and waitForExit comments Replace the misleading "scan some later column" comment in the tasklist CSV parser with an honest note that the simple cols[1] check accepts a known false-negative for image names containing commas (Unity exec names never do; the cost is one extra poll tick at most). Add a one-liner explaining why waitForExit's post-loop isProcessAlive check is not redundant — it catches an exit during the trailing poll-interval sleep. simplify-pass: 2
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
The plugin's startup config loader treated
UserSettings/AI-Game-Developer-Config.jsonas authoritative even when overrides were supplied viaUNITY_MCP_*environment variables or short flags (--url,--token,--auth). This commit implements the standard layered priority:The user-reported failure mode
In a worktree with
.worktree.envproviding a freshUNITY_MCP_TOKEN/UNITY_MCP_CLOUD_URL(pointing at a localhost MCP server in that worktree), the editor was opened byunity-mcp-cli open --url <U> --token <T>. The plugin still booted withconnectionMode: Cloudfrom a stale on-disk config and tried to authenticate against the cloud server with the worktree's local-dev token — surfacing a "Cloud mode requires authentication" dialog. The downstreamunity-mcppipeline (inai-game-developer-infra/.claude/pipeline/workflows/implement-task/targets/unity-mcp/setup.md) was forced to write the JSON config directly before opening the editor as a brittle workaround.What changed
Layered loader
EnvironmentUtils.ApplyEnvironmentOverridesnow takes optionalargReader/envReaderparameters (testable) and returns anOverrideRecordthat captures the disk-baseline value for every overridden field.UnityMcpPluginEditorstashes theOverrideRecordsoSave()can restore baseline values before serialising and re-apply runtime overrides afterwards. This guarantees runtime-only overrides NEVER leak into the persisted JSON.Env-var contract
UNITY_MCP_CLOUD_URL/stripped defensively.UNITY_MCP_HOSTUNITY_MCP_TOKENLocalTokenorCloudTokenbased on the post-mode-overrideConnectionMode.UNITY_MCP_CONNECTION_MODECloud/Customoverride. If absent and the URL is a loopback host (localhost/127.0.0.0/8/::1),Customis inferred.UNITY_MCP_AUTH_OPTIONnone/required.UNITY_MCP_KEEP_CONNECTED,UNITY_MCP_TRANSPORT,UNITY_MCP_START_SERVER,UNITY_MCP_TOOLSOverrideRecordso Save doesn't persist them.Flag contract
The plugin parses
Environment.GetCommandLineArgs()via the existingArgsUtilsand accepts both--UNITY_MCP_*style flags and these short aliases (highest precedence):--url <U>--token <T>--auth <required|none>Flags beat env vars; env vars beat disk; disk beats defaults.
Loopback inference
When a URL override is supplied without an explicit
UNITY_MCP_CONNECTION_MODE, the plugin infersCustommode if the URL targets a loopback host. This fixes the worktree scenario:UNITY_MCP_CLOUD_URL=http://localhost:5220/+UNITY_MCP_TOKEN=foonow resolves toLocalHost=http://localhost:5220,ConnectionMode=Custom,LocalToken=foo, with no leakage into the on-diskcloudTokenfield.Persistence
Save round-trips disk values to disk regardless of overrides. The in-memory config keeps its active overrides, so callers (UI, configurators, SignalR client) see no behaviour change.
Test plan
20 new EditMode test cases in
Packages/com.ivanmurzak.unity.mcp/Tests/Editor/Environment/EnvironmentUtilsTests.cs:CloudTokenLocalToken--token bar) beats env var (UNITY_MCP_TOKEN=foo)--UNITY_MCP_TOKEN=foostyle flag beats env varUNITY_MCP_CLOUD_URL=http://localhost:5220/→host=http://localhost:5220IsLoopbackUrlrecogniseslocalhost,127.0.0.0/8,::1; rejects remote hosts and malformed URLsCustommode and routes token toLocalTokenUNITY_MCP_CONNECTION_MODE=Cloudbeats loopback inferenceOverrideRecordis a no-op forApplyBaseline/ApplyOverridesUNITY_MCP_CONNECTION_MODE=Bogus) are silently ignored"VAL") are trimmedAll 810 existing EditMode tests still pass:
Notes
.claude/pipeline/workflows/implement-task/targets/unity-mcp/setup.md— it can drop the JSON-pinning workaround once this lands.