Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Assets/Tests/Editor/CliInstallationDetectorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,8 @@ public void IsShellDetectionUsableForPathSetup_WhenLegacyCliVersionIsHigh_Return
bool result = CliInstallationDetector.IsShellDetectionUsableForPathSetup(
detection,
UnityEngine.RuntimePlatform.OSXEditor,
(path, platform) => false);
(path, platform) => false,
"3.0.1-beta.6");

Assert.That(result, Is.False);
}
Expand Down
29 changes: 20 additions & 9 deletions Assets/Tests/Editor/CliSetupApplicationServiceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using UnityEngine;

using io.github.hatayama.UnityCliLoop.Application;
using io.github.hatayama.UnityCliLoop.Domain;

namespace io.github.hatayama.UnityCliLoop.Tests.Editor
{
Expand All @@ -17,7 +16,7 @@ public class CliSetupApplicationServiceTests
[Test]
public async Task InstallGlobalCliAsync_UsesMinimumRequiredDispatcherReleaseTag()
{
// Verifies that manual installs target the independent dispatcher release stream.
// Verifies that manual installs target the dispatcher release derived from the package pin.
FakeNativeCliInstaller nativeCliInstaller = new();
CliSetupApplicationService service = new(
new FakeCliInstallationDetector(new string[] { null }),
Expand All @@ -27,35 +26,35 @@ public async Task InstallGlobalCliAsync_UsesMinimumRequiredDispatcherReleaseTag(

Assert.That(
nativeCliInstaller.InstalledVersion,
Is.EqualTo(CliConstants.MINIMUM_REQUIRED_DISPATCHER_RELEASE_TAG));
Is.EqualTo(ExpectedDispatcherReleaseTag()));
}

[Test]
public void GetMinimumRequiredCliVersion_UsesDispatcherVersion()
{
// Verifies setup reports the minimum dispatcher required by the package.
// Verifies setup reads the minimum dispatcher version from the package pin JSON.
CliSetupApplicationService service = new(
new FakeCliInstallationDetector(new string[] { null }),
new FakeNativeCliInstaller());

Assert.That(service.GetMinimumRequiredCliVersion(), Is.EqualTo(CliConstants.MINIMUM_REQUIRED_DISPATCHER_VERSION));
Assert.That(service.GetMinimumRequiredCliVersion(), Is.EqualTo(ExpectedMinimumDispatcherVersion()));
}

[Test]
public void GetMinimumRequiredCliReleaseTag_UsesDispatcherReleaseTag()
{
// Verifies setup reports the prefixed release tag for the required dispatcher.
// Verifies setup derives the prefixed release tag from the package pin instead of a duplicated constant.
CliSetupApplicationService service = new(
new FakeCliInstallationDetector(new string[] { null }),
new FakeNativeCliInstaller());

Assert.That(service.GetMinimumRequiredCliReleaseTag(), Is.EqualTo(CliConstants.MINIMUM_REQUIRED_DISPATCHER_RELEASE_TAG));
Assert.That(service.GetMinimumRequiredCliReleaseTag(), Is.EqualTo(ExpectedDispatcherReleaseTag()));
}

[Test]
public void GetGlobalCliInstallCommand_UsesMinimumRequiredDispatcherReleaseTag()
{
// Verifies that fallback manual commands point at the independent dispatcher release stream.
// Verifies that fallback manual commands point at the pin-derived dispatcher release tag.
FakeNativeCliInstaller nativeCliInstaller = new();
CliSetupApplicationService service = new(
new FakeCliInstallationDetector(new string[] { null }),
Expand All @@ -67,7 +66,19 @@ public void GetGlobalCliInstallCommand_UsesMinimumRequiredDispatcherReleaseTag()

Assert.That(
command.ManualCommand,
Is.EqualTo("install " + CliConstants.MINIMUM_REQUIRED_DISPATCHER_RELEASE_TAG));
Is.EqualTo("install " + ExpectedDispatcherReleaseTag()));
}

private static string ExpectedMinimumDispatcherVersion()
{
CliPinLoadResult result = CliPinReader.LoadPackagePin();
Assert.That(result.Success, Is.True, result.ErrorMessage);
return result.Pin.MinimumDispatcherVersion;
}

private static string ExpectedDispatcherReleaseTag()
{
return CliPinReader.BuildDispatcherReleaseTag(ExpectedMinimumDispatcherVersion());
}

private sealed class FakeCliInstallationDetector : ICliInstallationDetector
Expand Down
8 changes: 5 additions & 3 deletions Assets/Tests/Editor/CliSetupCompatibilityTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
using NUnit.Framework;

using io.github.hatayama.UnityCliLoop.Domain;
using io.github.hatayama.UnityCliLoop.Presentation;

namespace io.github.hatayama.UnityCliLoop.Tests.Editor
Expand All @@ -10,6 +9,9 @@ namespace io.github.hatayama.UnityCliLoop.Tests.Editor
/// </summary>
public class CliSetupCompatibilityTests
{
// Why: fixed test value keeps compat logic assertions independent of the shipping pin version.
private const string TEST_MINIMUM_DISPATCHER_VERSION = "3.0.0";

[TestCase(null, false, false, false)]
[TestCase("2.1.10", false, true, false)]
[TestCase("3.0.0", false, true, false)]
Expand All @@ -26,7 +28,7 @@ public void Evaluate_ReturnsExpectedDispatcherSetupState(
CliSetupCompatibilityState state = CliSetupCompatibility.Evaluate(
cliVersion,
isDispatcher,
CliConstants.MINIMUM_REQUIRED_DISPATCHER_VERSION);
TEST_MINIMUM_DISPATCHER_VERSION);

Assert.That(state.NeedsUpdate, Is.EqualTo(expectedNeedsUpdate));
Assert.That(state.IsCompatible, Is.EqualTo(expectedCompatible));
Expand All @@ -40,7 +42,7 @@ public void Evaluate_WhenDispatcherVersionIsInvalid_RequestsUpdate()
CliSetupCompatibilityState state = CliSetupCompatibility.Evaluate(
"not-a-version",
true,
CliConstants.MINIMUM_REQUIRED_DISPATCHER_VERSION);
TEST_MINIMUM_DISPATCHER_VERSION);

Assert.That(state.NeedsUpdate, Is.True);
Assert.That(state.IsCompatible, Is.False);
Expand Down
130 changes: 130 additions & 0 deletions Packages/src/Editor/Application/CliPinReader.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
using System;
using System.IO;

using Newtonsoft.Json.Linq;

using io.github.hatayama.UnityCliLoop.Domain;
using io.github.hatayama.UnityCliLoop.ToolContracts;

namespace io.github.hatayama.UnityCliLoop.Application
{
/// <summary>
/// Immutable snapshot of the CLI pin JSON shipped with the package.
/// </summary>
public readonly struct CliPin
{
public CliPin(string projectRunnerVersion, string minimumDispatcherVersion)
{
ProjectRunnerVersion = projectRunnerVersion;
MinimumDispatcherVersion = minimumDispatcherVersion;
}

public string ProjectRunnerVersion { get; }
public string MinimumDispatcherVersion { get; }
}

/// <summary>
/// Result of loading the CLI pin JSON.
/// </summary>
public readonly struct CliPinLoadResult
{
private CliPinLoadResult(bool success, CliPin pin, string errorMessage)
{
Success = success;
Pin = pin;
ErrorMessage = errorMessage;
}

public bool Success { get; }
public CliPin Pin { get; }
public string ErrorMessage { get; }

public static CliPinLoadResult FromSuccess(CliPin pin)
{
return new CliPinLoadResult(true, pin, string.Empty);
}

public static CliPinLoadResult FromFailure(string errorMessage)
{
return new CliPinLoadResult(false, default, errorMessage);
}
}

/// <summary>
/// Reads the CLI pin JSON that ships with the Unity package so callers can
/// consult a single source of truth for minimum-version requirements.
/// </summary>
public static class CliPinReader
{
// Why: keep pin JSON keys named in one place so callers do not spread string literals.
private const string PIN_JSON_PROJECT_RUNNER_VERSION_KEY = "projectRunnerVersion";
private const string PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY = "minimumDispatcherVersion";

public static CliPinLoadResult LoadPackagePin()
{
string pinPath = Path.Combine(
UnityCliLoopConstants.PackageResolvedPath,
UnityCliLoopConstants.ULOOP_PROJECT_RUNNER_PIN_FILE_NAME);
return LoadPinFromPath(pinPath);
}

// Why: exposed so tests and edge-case callers can point at an alternate pin file layout.
public static CliPinLoadResult LoadPinFromPath(string pinPath)
{
if (string.IsNullOrWhiteSpace(pinPath))
{
return CliPinLoadResult.FromFailure("Unity CLI Loop pin file path is empty.");
}

if (!File.Exists(pinPath))
{
return CliPinLoadResult.FromFailure(
$"Unity CLI Loop pin file not found at {pinPath}.");
}

string content = File.ReadAllText(pinPath);
if (string.IsNullOrWhiteSpace(content))
{
return CliPinLoadResult.FromFailure(
$"Unity CLI Loop pin file at {pinPath} is empty.");
}

JObject parsed = JObject.Parse(content);
string projectRunnerVersion = parsed[PIN_JSON_PROJECT_RUNNER_VERSION_KEY]?.ToString();
string minimumDispatcherVersion = parsed[PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY]?.ToString();
if (string.IsNullOrWhiteSpace(projectRunnerVersion))
{
return CliPinLoadResult.FromFailure(
$"Unity CLI Loop pin file at {pinPath} is missing {PIN_JSON_PROJECT_RUNNER_VERSION_KEY}.");
}
if (string.IsNullOrWhiteSpace(minimumDispatcherVersion))
{
return CliPinLoadResult.FromFailure(
$"Unity CLI Loop pin file at {pinPath} is missing {PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY}.");
}

return CliPinLoadResult.FromSuccess(
new CliPin(projectRunnerVersion, minimumDispatcherVersion));
}
Comment on lines +85 to +108

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Unwrap JObject.Parse failures into CliPinLoadResult.FromFailure.

Every other failure mode here (missing file, empty content, missing keys) is surfaced via CliPinLoadResult.FromFailure with a descriptive message. A malformed pin JSON, however, will let JObject.Parse throw a raw JsonException straight through LoadPackagePin() / LoadMinimumDispatcherVersionOrThrow(), bypassing the friendly error wrapping and surfacing an opaque stack trace during setup instead of "Unity CLI Loop cannot resolve minimum dispatcher version from the package pin: ...". CliInstallationDetector.ParseCliContractOutput already catches JsonException for similarly untrusted content, so this would keep the pattern consistent.

🛡️ Proposed fix
-            JObject parsed = JObject.Parse(content);
-            string projectRunnerVersion = parsed[PIN_JSON_PROJECT_RUNNER_VERSION_KEY]?.ToString();
-            string minimumDispatcherVersion = parsed[PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY]?.ToString();
+            JObject parsed;
+            try
+            {
+                parsed = JObject.Parse(content);
+            }
+            catch (Newtonsoft.Json.JsonException ex)
+            {
+                return CliPinLoadResult.FromFailure(
+                    $"Unity CLI Loop pin file at {pinPath} contains invalid JSON: {ex.Message}");
+            }
+            string projectRunnerVersion = parsed[PIN_JSON_PROJECT_RUNNER_VERSION_KEY]?.ToString();
+            string minimumDispatcherVersion = parsed[PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY]?.ToString();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
string content = File.ReadAllText(pinPath);
if (string.IsNullOrWhiteSpace(content))
{
return CliPinLoadResult.FromFailure(
$"Unity CLI Loop pin file at {pinPath} is empty.");
}
JObject parsed = JObject.Parse(content);
string projectRunnerVersion = parsed[PIN_JSON_PROJECT_RUNNER_VERSION_KEY]?.ToString();
string minimumDispatcherVersion = parsed[PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY]?.ToString();
if (string.IsNullOrWhiteSpace(projectRunnerVersion))
{
return CliPinLoadResult.FromFailure(
$"Unity CLI Loop pin file at {pinPath} is missing {PIN_JSON_PROJECT_RUNNER_VERSION_KEY}.");
}
if (string.IsNullOrWhiteSpace(minimumDispatcherVersion))
{
return CliPinLoadResult.FromFailure(
$"Unity CLI Loop pin file at {pinPath} is missing {PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY}.");
}
return CliPinLoadResult.FromSuccess(
new CliPin(projectRunnerVersion, minimumDispatcherVersion));
}
string content = File.ReadAllText(pinPath);
if (string.IsNullOrWhiteSpace(content))
{
return CliPinLoadResult.FromFailure(
$"Unity CLI Loop pin file at {pinPath} is empty.");
}
JObject parsed;
try
{
parsed = JObject.Parse(content);
}
catch (Newtonsoft.Json.JsonException ex)
{
return CliPinLoadResult.FromFailure(
$"Unity CLI Loop pin file at {pinPath} contains invalid JSON: {ex.Message}");
}
string projectRunnerVersion = parsed[PIN_JSON_PROJECT_RUNNER_VERSION_KEY]?.ToString();
string minimumDispatcherVersion = parsed[PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY]?.ToString();
if (string.IsNullOrWhiteSpace(projectRunnerVersion))
{
return CliPinLoadResult.FromFailure(
$"Unity CLI Loop pin file at {pinPath} is missing {PIN_JSON_PROJECT_RUNNER_VERSION_KEY}.");
}
if (string.IsNullOrWhiteSpace(minimumDispatcherVersion))
{
return CliPinLoadResult.FromFailure(
$"Unity CLI Loop pin file at {pinPath} is missing {PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY}.");
}
return CliPinLoadResult.FromSuccess(
new CliPin(projectRunnerVersion, minimumDispatcherVersion));
}
🤖 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 `@Packages/src/Editor/Application/CliPinReader.cs` around lines 85 - 108, Wrap
the JObject.Parse call in CliPinReader.LoadCliPin (or the surrounding parsing
path used by LoadPackagePin and LoadMinimumDispatcherVersionOrThrow) so
malformed pin JSON returns CliPinLoadResult.FromFailure instead of throwing a
raw JsonException. Keep the existing failure pattern consistent with the
missing-file, empty-content, and missing-key checks by catching parse exceptions
and including the pinPath plus a descriptive parse error in the failure message.

Source: Learnings


// Why: composes the dispatcher release tag from the pin so callers do not have to know the prefix.
public static string BuildDispatcherReleaseTag(string minimumDispatcherVersion)
{
return CliConstants.DISPATCHER_RELEASE_TAG_PREFIX + minimumDispatcherVersion;
}

// Why: setup/detection paths must fail closed when the pin is unreadable rather than defaulting
// to "compatible", so both call sites share one Fail-Fast helper instead of duplicating it.
public static string LoadMinimumDispatcherVersionOrThrow()
{
CliPinLoadResult pinResult = LoadPackagePin();
if (!pinResult.Success)
{
throw new InvalidOperationException(
"Unity CLI Loop cannot resolve minimum dispatcher version from the package pin: "
+ pinResult.ErrorMessage);
}
return pinResult.Pin.MinimumDispatcherVersion;
}
}
}
11 changes: 11 additions & 0 deletions Packages/src/Editor/Application/CliPinReader.cs.meta

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 6 additions & 4 deletions Packages/src/Editor/Application/CliSetupApplicationService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,16 @@ public void InvalidateCliCache()

public string GetMinimumRequiredCliVersion()
{
// Why: v3 setup installs the global dispatcher; project-local CLI versions are pinned separately.
return CliConstants.MINIMUM_REQUIRED_DISPATCHER_VERSION;
// Why: v3 setup installs the global dispatcher and reads the minimum from the package pin JSON
// so that the single source of truth stays consistent with the dispatcher installation flow.
return CliPinReader.LoadMinimumDispatcherVersionOrThrow();
}

public string GetMinimumRequiredCliReleaseTag()
{
// Why: v3 setup installs the global dispatcher; project-local CLI versions are pinned separately.
return CliConstants.MINIMUM_REQUIRED_DISPATCHER_RELEASE_TAG;
// Why: v3 setup installs the global dispatcher, and the release tag is derived from the pin so it
// cannot drift from the version reported by GetMinimumRequiredCliVersion.
return CliPinReader.BuildDispatcherReleaseTag(CliPinReader.LoadMinimumDispatcherVersionOrThrow());
}

public bool IsPackageOwnedCurrentUserInstallPath(
Expand Down
7 changes: 0 additions & 7 deletions Packages/src/Editor/Domain/CliConstants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,6 @@ public static class CliConstants
// Bump it together with cli/contract.json protocolVersion only when this package can
// no longer interoperate with a different CLI protocol generation.
public const int REQUIRED_CLI_PROTOCOL_VERSION = 3;
// Why: setup installs this pinned project runner release; protocol bump PRs can advance it only after
// the matching project runner tag is published.
public const string MINIMUM_REQUIRED_PROJECT_RUNNER_VERSION = "3.0.0-beta.43";
public const string MINIMUM_REQUIRED_PROJECT_RUNNER_RELEASE_TAG = PROJECT_RUNNER_RELEASE_TAG_PREFIX + MINIMUM_REQUIRED_PROJECT_RUNNER_VERSION;
// Why: global uloop is a dispatcher; project-local CLI versions are downloaded separately.
public const string MINIMUM_REQUIRED_DISPATCHER_VERSION = "3.0.1-beta.6";
public const string MINIMUM_REQUIRED_DISPATCHER_RELEASE_TAG = DISPATCHER_RELEASE_TAG_PREFIX + MINIMUM_REQUIRED_DISPATCHER_VERSION;
public const string VERSION_FLAG = "--version";
public const string SHORT_VERSION_FLAG = "-v";
public const string JSON_FLAG = "--json";
Expand Down
13 changes: 10 additions & 3 deletions Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs
Original file line number Diff line number Diff line change
Expand Up @@ -118,14 +118,19 @@ public Task<bool> IsCliVisibleFromShellAsync(RuntimePlatform platform, Cancellat
return Task.FromResult(true);
}

// Why: resolve the minimum dispatcher version once on the caller thread so the background
// detection task does not perform Unity package IO from a worker thread.
string minimumDispatcherVersion = CliPinReader.LoadMinimumDispatcherVersionOrThrow();

return Task.Run(
() =>
{
CliInstallationDetection detection = DetectShellCliInstallationBlocking(platform, ct);
return IsShellDetectionUsableForPathSetup(
detection,
platform,
NativeCliInstaller.IsPackageOwnedCurrentUserInstallPath);
NativeCliInstaller.IsPackageOwnedCurrentUserInstallPath,
minimumDispatcherVersion);
},
ct);
}
Expand Down Expand Up @@ -241,9 +246,11 @@ internal static ProcessStartInfo BuildShellCliDetectionStartInfo(
internal static bool IsShellDetectionUsableForPathSetup(
CliInstallationDetection detection,
RuntimePlatform platform,
Func<string, RuntimePlatform, bool> isPackageOwnedCurrentUserInstallPath)
Func<string, RuntimePlatform, bool> isPackageOwnedCurrentUserInstallPath,
string minimumDispatcherVersion)
{
UnityEngine.Debug.Assert(isPackageOwnedCurrentUserInstallPath != null, "isPackageOwnedCurrentUserInstallPath must not be null");
UnityEngine.Debug.Assert(!string.IsNullOrEmpty(minimumDispatcherVersion), "minimumDispatcherVersion must not be null or empty");

if (isPackageOwnedCurrentUserInstallPath(detection.ExecutablePath, platform))
{
Expand All @@ -257,7 +264,7 @@ internal static bool IsShellDetectionUsableForPathSetup(

return CliVersionComparer.IsVersionGreaterThanOrEqual(
detection.Version,
CliConstants.MINIMUM_REQUIRED_DISPATCHER_VERSION);
minimumDispatcherVersion);
}

internal static string BuildShellCliDetectionCommandForShell(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -985,10 +985,12 @@ private static CliSetupCompatibilityState EvaluateCliSetupCompatibility(
string cliVersion,
bool cliIsDispatcher)
{
// Why: route through the setup facade so the pin JSON stays the single source for the minimum
// dispatcher version instead of duplicating the constant in the presentation layer.
return CliSetupCompatibility.Evaluate(
cliVersion,
cliIsDispatcher,
CliConstants.MINIMUM_REQUIRED_DISPATCHER_VERSION);
CliSetupApplicationFacade.GetMinimumRequiredCliVersion());
}

private async Task HandleUninstallCli()
Expand Down
Loading
Loading