From a976e3b1b529564eb972fbe1d673053756b01d86 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 7 Jul 2026 10:32:31 +0900 Subject: [PATCH 1/2] Move tool execution session state into Domain Extract the single-flight execution slot state into a Domain session so the Application service only orchestrates tool execution and preserves busy exception shaping. --- ...nRpcRequestProcessorCliVersionGateTests.cs | 97 +++++++++++++++++++ .../Tests/Editor/ToolExecutionSessionTests.cs | 85 ++++++++++++++++ .../Editor/ToolExecutionSessionTests.cs.meta | 11 +++ .../UnityCliLoopToolExecutionService.cs | 64 +----------- .../src/Editor/Domain/ToolExecutionSession.cs | 96 ++++++++++++++++++ .../Domain/ToolExecutionSession.cs.meta | 11 +++ 6 files changed, 305 insertions(+), 59 deletions(-) create mode 100644 Assets/Tests/Editor/ToolExecutionSessionTests.cs create mode 100644 Assets/Tests/Editor/ToolExecutionSessionTests.cs.meta create mode 100644 Packages/src/Editor/Domain/ToolExecutionSession.cs create mode 100644 Packages/src/Editor/Domain/ToolExecutionSession.cs.meta diff --git a/Assets/Tests/Editor/JsonRpcRequestProcessorCliVersionGateTests.cs b/Assets/Tests/Editor/JsonRpcRequestProcessorCliVersionGateTests.cs index b9d313a89b..78522b05fb 100644 --- a/Assets/Tests/Editor/JsonRpcRequestProcessorCliVersionGateTests.cs +++ b/Assets/Tests/Editor/JsonRpcRequestProcessorCliVersionGateTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Text.RegularExpressions; using System.Threading; using System.Threading.Tasks; @@ -277,6 +278,46 @@ public async Task ProcessRequest_WhenExecuteDynamicCodeWaitsForMainThread_Allows } } + [Test] + public async Task ProcessRequest_WhenToolIsDisabled_ReturnsInternalErrorShape() + { + // Verifies disabled tools keep the JSON-RPC internal_error data shape before PR3 folds gates together. + InMemoryToolSettingsPort toolSettingsPort = new InMemoryToolSettingsPort(); + toolSettingsPort.SetToolEnabled(SingleFlightTestTool.Name, false); + UnityCliLoopToolRegistrarService service = CreateRegistrarServiceWithToolSettings(toolSettingsPort); + service.RegisterCustomTool(new SingleFlightTestTool()); + JsonRpcRequestProcessor processor = CreateProcessor(service); + + LogAssert.Expect(LogType.Error, new Regex(@"\[JsonRpcRequestProcessor\] Error: Tool 'single-flight-test' is disabled")); + string response = await processor.ProcessRequest( + BuildToolRequest(SingleFlightTestTool.Name, 1), + CancellationToken.None); + JObject error = ParseError(response); + JObject data = ParseErrorData(response); + + Assert.That(error["message"]?.ToString(), Does.Contain(SingleFlightTestTool.Name)); + Assert.That(data["type"]?.ToString(), Is.EqualTo("internal_error")); + } + + [Test] + public async Task ProcessRequest_WhenToolRequiresBlockedSecuritySetting_ReturnsSecurityBlockedShape() + { + // Verifies security-blocked tools keep their machine-readable JSON-RPC error data. + UnityCliLoopToolRegistrarService service = CreateRegistrarService(); + service.RegisterCustomTool(new SecurityBlockedTestTool()); + JsonRpcRequestProcessor processor = CreateProcessor(service); + + LogAssert.Expect(LogType.Error, new Regex(@"\[JsonRpcRequestProcessor\] Error: Tool 'security-blocked-test' is blocked by security settings")); + string response = await processor.ProcessRequest( + BuildToolRequest(SecurityBlockedTestTool.Name, 1), + CancellationToken.None); + JObject data = ParseErrorData(response); + + Assert.That(data["type"]?.ToString(), Is.EqualTo("security_blocked")); + Assert.That(data["command"]?.ToString(), Is.EqualTo(SecurityBlockedTestTool.Name)); + Assert.That(data["reason"]?.ToString(), Does.Contain("security settings")); + } + [Test] public async Task ProcessRequest_WhenCompileWaitsForDomainReload_KeepsAcceptedRequestAliveAfterDisconnect() { @@ -553,6 +594,15 @@ private static UnityCliLoopToolRegistrarService CreateRegistrarService() return UnityCliLoopToolRegistrarTestFactory.Create(UnityCliLoopToolDiscovery.DiscoverTools); } + private static UnityCliLoopToolRegistrarService CreateRegistrarServiceWithToolSettings(IToolSettingsPort toolSettingsPort) + { + return new UnityCliLoopToolRegistrarService( + new EmptyInternalToolNameProvider(), + toolSettingsPort, + new UnityCliLoopToolExecutionService(new NoOpEditorRuntimeStatePort()), + UnityCliLoopToolDiscovery.DiscoverTools); + } + private static JsonRpcRequestProcessor CreateProcessor(UnityCliLoopToolRegistrarService service) { UnityCliLoopExecutionRouter executionRouter = new(service); @@ -756,6 +806,53 @@ public Task ExecuteAsync(JToken paramsToken, Cancellat } } + [UnityCliLoopTool(RequiredSecuritySetting = (UnityCliLoopSecuritySetting)999)] + private sealed class SecurityBlockedTestTool : IUnityCliLoopTool + { + public const string Name = "security-blocked-test"; + + public string ToolName => Name; + + public ToolParameterSchema ParameterSchema => new(); + + public Task ExecuteAsync(JToken paramsToken, CancellationToken ct) + { + return Task.FromResult(new SingleFlightTestResponse()); + } + } + + private sealed class InMemoryToolSettingsPort : IToolSettingsPort + { + private readonly HashSet _disabledTools = new(); + + public bool IsToolEnabled(string toolName) + { + return !_disabledTools.Contains(toolName); + } + + public void SetToolEnabled(string toolName, bool enabled) + { + if (enabled) + { + _disabledTools.Remove(toolName); + return; + } + + _disabledTools.Add(toolName); + } + + public string[] GetDisabledTools() + { + string[] disabledTools = new string[_disabledTools.Count]; + _disabledTools.CopyTo(disabledTools); + return disabledTools; + } + + public void InvalidateCache() + { + } + } + private sealed class SingleFlightTestResponse : UnityCliLoopToolResponse { public bool Success { get; set; } = true; diff --git a/Assets/Tests/Editor/ToolExecutionSessionTests.cs b/Assets/Tests/Editor/ToolExecutionSessionTests.cs new file mode 100644 index 0000000000..c0245fae08 --- /dev/null +++ b/Assets/Tests/Editor/ToolExecutionSessionTests.cs @@ -0,0 +1,85 @@ +using NUnit.Framework; + +using io.github.hatayama.UnityCliLoop.Domain; +using io.github.hatayama.UnityCliLoop.ToolContracts; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor +{ + public sealed class ToolExecutionSessionTests + { + [Test] + public void TryEnter_WhenNoToolIsRunning_ShouldEnterAndAllowExitThenNextTool() + { + // Tests that an empty session admits a tool and releases the slot after exit. + ToolExecutionSession session = new ToolExecutionSession(); + + ToolExecutionSessionEnterResult firstResult = session.TryEnter("first-tool"); + session.Exit(); + ToolExecutionSessionEnterResult secondResult = session.TryEnter("second-tool"); + + Assert.That(firstResult.IsEntered, Is.True); + Assert.That(firstResult.RunningToolName, Is.Empty); + Assert.That(secondResult.IsEntered, Is.True); + + session.Exit(); + } + + [Test] + public void TryEnter_WhenDifferentToolIsRunning_ShouldReturnBusyWithRunningToolName() + { + // Tests that the single-flight gate reports the already running tool for rejected requests. + ToolExecutionSession session = new ToolExecutionSession(); + + ToolExecutionSessionEnterResult firstResult = session.TryEnter("running-tool"); + ToolExecutionSessionEnterResult busyResult = session.TryEnter("requested-tool"); + + Assert.That(firstResult.IsEntered, Is.True); + Assert.That(busyResult.IsEntered, Is.False); + Assert.That(busyResult.RunningToolName, Is.EqualTo("running-tool")); + + session.Exit(); + } + + [Test] + public void TryEnter_WhenExecuteDynamicCodeIsAlreadyRunning_ShouldAllowSecondExecuteDynamicCode() + { + // Tests that execute-dynamic-code keeps its existing shared execution slot behavior. + ToolExecutionSession session = new ToolExecutionSession(); + + ToolExecutionSessionEnterResult firstResult = + session.TryEnter(UnityCliLoopConstants.TOOL_NAME_EXECUTE_DYNAMIC_CODE); + ToolExecutionSessionEnterResult secondResult = + session.TryEnter(UnityCliLoopConstants.TOOL_NAME_EXECUTE_DYNAMIC_CODE); + + Assert.That(firstResult.IsEntered, Is.True); + Assert.That(secondResult.IsEntered, Is.True); + + session.Exit(); + session.Exit(); + } + + [Test] + public void Exit_WhenTwoSharedDynamicCodeExecutionsEntered_ShouldKeepSlotBusyUntilBothExit() + { + // Tests that shared execute-dynamic-code entries keep the session busy until every entry exits. + ToolExecutionSession session = new ToolExecutionSession(); + + session.TryEnter(UnityCliLoopConstants.TOOL_NAME_EXECUTE_DYNAMIC_CODE); + session.TryEnter(UnityCliLoopConstants.TOOL_NAME_EXECUTE_DYNAMIC_CODE); + + ToolExecutionSessionEnterResult busyBeforeExit = session.TryEnter("other-tool"); + session.Exit(); + ToolExecutionSessionEnterResult busyAfterOneExit = session.TryEnter("other-tool"); + session.Exit(); + ToolExecutionSessionEnterResult enteredAfterBothExit = session.TryEnter("other-tool"); + + Assert.That(busyBeforeExit.IsEntered, Is.False); + Assert.That(busyBeforeExit.RunningToolName, Is.EqualTo(UnityCliLoopConstants.TOOL_NAME_EXECUTE_DYNAMIC_CODE)); + Assert.That(busyAfterOneExit.IsEntered, Is.False); + Assert.That(busyAfterOneExit.RunningToolName, Is.EqualTo(UnityCliLoopConstants.TOOL_NAME_EXECUTE_DYNAMIC_CODE)); + Assert.That(enteredAfterBothExit.IsEntered, Is.True); + + session.Exit(); + } + } +} diff --git a/Assets/Tests/Editor/ToolExecutionSessionTests.cs.meta b/Assets/Tests/Editor/ToolExecutionSessionTests.cs.meta new file mode 100644 index 0000000000..c46c999d4e --- /dev/null +++ b/Assets/Tests/Editor/ToolExecutionSessionTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 36c7d5bad1e247e5ba1e036f5cced913 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/Application/UnityCliLoopToolExecutionService.cs b/Packages/src/Editor/Application/UnityCliLoopToolExecutionService.cs index 15c8abc16f..171416e44c 100644 --- a/Packages/src/Editor/Application/UnityCliLoopToolExecutionService.cs +++ b/Packages/src/Editor/Application/UnityCliLoopToolExecutionService.cs @@ -14,12 +14,8 @@ namespace io.github.hatayama.UnityCliLoop.Application /// internal sealed class UnityCliLoopToolExecutionService { - private const string UnknownToolName = "unknown"; - private readonly IEditorRuntimeStatePort _editorRuntimeStatePort; - private readonly object _executionStateLock = new(); - private string _runningToolName; - private int _runningExecutionCount; + private readonly ToolExecutionSession _executionSession = new(); internal UnityCliLoopToolExecutionService(IEditorRuntimeStatePort editorRuntimeStatePort) { @@ -55,9 +51,10 @@ internal async Task ExecuteToolAsync( throw new UnityCliLoopSecurityException(toolName, "Tool is blocked by security settings"); } - if (!TryEnterExecution(toolName, out string runningToolName)) + ToolExecutionSessionEnterResult enterResult = _executionSession.TryEnter(toolName); + if (!enterResult.IsEntered) { - throw CreateBusyException(runningToolName, toolName, _editorRuntimeStatePort); + throw CreateBusyException(enterResult.RunningToolName, toolName, _editorRuntimeStatePort); } try @@ -76,55 +73,10 @@ internal async Task ExecuteToolAsync( } finally { - ExitExecution(); + _executionSession.Exit(); } } - private bool TryEnterExecution(string toolName, out string runningToolName) - { - lock (_executionStateLock) - { - if (_runningExecutionCount == 0) - { - _runningToolName = toolName; - _runningExecutionCount = 1; - runningToolName = toolName; - return true; - } - - if (CanShareExecutionSlot(_runningToolName, toolName)) - { - _runningExecutionCount++; - runningToolName = _runningToolName; - return true; - } - - runningToolName = GetRunningToolNameInsideLock(); - return false; - } - } - - private void ExitExecution() - { - lock (_executionStateLock) - { - Debug.Assert(_runningExecutionCount > 0, "running execution count must be positive before exit"); - _runningExecutionCount--; - if (_runningExecutionCount > 0) - { - return; - } - - _runningToolName = null; - } - } - - private static bool CanShareExecutionSlot(string runningToolName, string requestedToolName) - { - return runningToolName == UnityCliLoopConstants.TOOL_NAME_EXECUTE_DYNAMIC_CODE - && requestedToolName == UnityCliLoopConstants.TOOL_NAME_EXECUTE_DYNAMIC_CODE; - } - internal static UnityCliLoopToolBusyException CreateBusyException( string runningToolName, string requestedToolName, @@ -157,11 +109,5 @@ internal static UnityCliLoopToolBusyException CreateBusyException( return new UnityCliLoopToolBusyException(runningToolName, requestedToolName); } - private string GetRunningToolNameInsideLock() - { - return string.IsNullOrWhiteSpace(_runningToolName) - ? UnknownToolName - : _runningToolName; - } } } diff --git a/Packages/src/Editor/Domain/ToolExecutionSession.cs b/Packages/src/Editor/Domain/ToolExecutionSession.cs new file mode 100644 index 0000000000..b0d7cc5cbc --- /dev/null +++ b/Packages/src/Editor/Domain/ToolExecutionSession.cs @@ -0,0 +1,96 @@ +using System.Diagnostics; + +using io.github.hatayama.UnityCliLoop.ToolContracts; + +namespace io.github.hatayama.UnityCliLoop.Domain +{ + /// + /// Owns the thread-safe single-flight state for tool execution sessions. + /// + internal sealed class ToolExecutionSession + { + private const string UnknownToolName = "unknown"; + + private readonly object _executionStateLock = new(); + private string _runningToolName; + private int _runningExecutionCount; + + internal ToolExecutionSessionEnterResult TryEnter(string requestedToolName) + { + Debug.Assert(!string.IsNullOrWhiteSpace(requestedToolName), "requestedToolName must not be null or whitespace"); + + lock (_executionStateLock) + { + if (_runningExecutionCount == 0) + { + _runningToolName = requestedToolName; + _runningExecutionCount = 1; + return ToolExecutionSessionEnterResult.Entered(); + } + + if (CanShareExecutionSlot(_runningToolName, requestedToolName)) + { + _runningExecutionCount++; + return ToolExecutionSessionEnterResult.Entered(); + } + + return ToolExecutionSessionEnterResult.Busy(GetRunningToolNameInsideLock()); + } + } + + internal void Exit() + { + lock (_executionStateLock) + { + Debug.Assert(_runningExecutionCount > 0, "running execution count must be positive before exit"); + _runningExecutionCount--; + if (_runningExecutionCount > 0) + { + return; + } + + _runningToolName = null; + } + } + + private static bool CanShareExecutionSlot(string runningToolName, string requestedToolName) + { + return runningToolName == UnityCliLoopConstants.TOOL_NAME_EXECUTE_DYNAMIC_CODE + && requestedToolName == UnityCliLoopConstants.TOOL_NAME_EXECUTE_DYNAMIC_CODE; + } + + private string GetRunningToolNameInsideLock() + { + return string.IsNullOrWhiteSpace(_runningToolName) + ? UnknownToolName + : _runningToolName; + } + } + + /// + /// Reports whether a tool execution request entered the session or was rejected by the single-flight gate. + /// + internal readonly struct ToolExecutionSessionEnterResult + { + public readonly bool IsEntered; + public readonly string RunningToolName; + + private ToolExecutionSessionEnterResult(bool isEntered, string runningToolName) + { + Debug.Assert(isEntered || !string.IsNullOrWhiteSpace(runningToolName), "runningToolName must not be null or whitespace for busy decisions"); + + IsEntered = isEntered; + RunningToolName = runningToolName; + } + + public static ToolExecutionSessionEnterResult Entered() + { + return new ToolExecutionSessionEnterResult(true, string.Empty); + } + + public static ToolExecutionSessionEnterResult Busy(string runningToolName) + { + return new ToolExecutionSessionEnterResult(false, runningToolName); + } + } +} diff --git a/Packages/src/Editor/Domain/ToolExecutionSession.cs.meta b/Packages/src/Editor/Domain/ToolExecutionSession.cs.meta new file mode 100644 index 0000000000..50fb8ff92b --- /dev/null +++ b/Packages/src/Editor/Domain/ToolExecutionSession.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 72cc50aac65c4b4b8a4d4185cf9960be +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: From 70ad1cda838d8cb63c04e15dc9c3fd766621edb2 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 7 Jul 2026 10:37:28 +0900 Subject: [PATCH 2/2] Clarify security-blocked test enum value Document why the JSON-RPC security shape test uses an undefined security enum value while the production enum currently only exposes None. --- .../Tests/Editor/JsonRpcRequestProcessorCliVersionGateTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Assets/Tests/Editor/JsonRpcRequestProcessorCliVersionGateTests.cs b/Assets/Tests/Editor/JsonRpcRequestProcessorCliVersionGateTests.cs index 78522b05fb..911c459855 100644 --- a/Assets/Tests/Editor/JsonRpcRequestProcessorCliVersionGateTests.cs +++ b/Assets/Tests/Editor/JsonRpcRequestProcessorCliVersionGateTests.cs @@ -806,6 +806,7 @@ public Task ExecuteAsync(JToken paramsToken, Cancellat } } + // The enum currently has only None, so an undefined value is the only way to exercise security-blocked handling. [UnityCliLoopTool(RequiredSecuritySetting = (UnityCliLoopSecuritySetting)999)] private sealed class SecurityBlockedTestTool : IUnityCliLoopTool {