From 0128883e9dcc7a6626f10bd007b5b4d3f86ae957 Mon Sep 17 00:00:00 2001 From: Scott Hanselman Date: Tue, 31 Mar 2026 14:18:20 -0700 Subject: [PATCH 1/2] refactor: extract TryParseArgv helper, fix operator precedence (closes #91) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract TryParseArgv(JsonElement) to consolidate duplicated argv parsing in HandleRunPrepare and HandleRunAsync (~20 lines each → one 20-line helper) - Fix operator precedence in HandleExecApprovalsSet: add parentheses to make (True || False) grouping explicit (was safe by coincidence but misleading) - Add 5 new tests for argv parsing: array, string, empty, missing, multi-arg Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Capabilities/SystemCapability.cs | 108 ++++++++---------- tests/OpenClaw.Shared.Tests/SystemRunTests.cs | 84 ++++++++++++++ 2 files changed, 130 insertions(+), 62 deletions(-) diff --git a/src/OpenClaw.Shared/Capabilities/SystemCapability.cs b/src/OpenClaw.Shared/Capabilities/SystemCapability.cs index 986f3fb..bbe7a53 100644 --- a/src/OpenClaw.Shared/Capabilities/SystemCapability.cs +++ b/src/OpenClaw.Shared/Capabilities/SystemCapability.cs @@ -163,44 +163,50 @@ private NodeInvokeResponse HandleWhich(NodeInvokeRequest request) private static string FormatExecCommand(string[] argv) => ShellQuoting.FormatExecCommand(argv); /// - /// Pre-flight for system.run: echoes back the execution plan without running anything. - /// The gateway uses this to build its approval context before the actual run. + /// Parses a JSON "command" property as either a string array or a plain string. + /// Returns the argv array (command as first element) or null if missing/invalid. /// - private NodeInvokeResponse HandleRunPrepare(NodeInvokeRequest request) + private static string[]? TryParseArgv(System.Text.Json.JsonElement requestArgs) { - string? command = null; - string[]? argv = null; - string? rawCommand = null; - string? cwd = null; - - if (request.Args.ValueKind != System.Text.Json.JsonValueKind.Undefined && - request.Args.TryGetProperty("command", out var cmdEl)) + if (requestArgs.ValueKind == System.Text.Json.JsonValueKind.Undefined || + !requestArgs.TryGetProperty("command", out var cmdEl)) + return null; + + if (cmdEl.ValueKind == System.Text.Json.JsonValueKind.Array) { - if (cmdEl.ValueKind == System.Text.Json.JsonValueKind.Array) + var list = new List(); + foreach (var item in cmdEl.EnumerateArray()) { - var list = new List(); - foreach (var item in cmdEl.EnumerateArray()) - { - if (item.ValueKind == System.Text.Json.JsonValueKind.String) - list.Add(item.GetString() ?? ""); - } - argv = list.ToArray(); - command = argv.Length > 0 ? argv[0] : null; - } - else if (cmdEl.ValueKind == System.Text.Json.JsonValueKind.String) - { - command = cmdEl.GetString(); - argv = command != null ? new[] { command } : null; + if (item.ValueKind == System.Text.Json.JsonValueKind.String) + list.Add(item.GetString() ?? ""); } + return list.Count > 0 ? list.ToArray() : null; } - if (string.IsNullOrWhiteSpace(command) || argv == null || argv.Length == 0) + if (cmdEl.ValueKind == System.Text.Json.JsonValueKind.String) + { + var command = cmdEl.GetString(); + return command != null ? new[] { command } : null; + } + + return null; + } + + /// + /// Pre-flight for system.run: echoes back the execution plan without running anything. + /// The gateway uses this to build its approval context before the actual run. + /// + private NodeInvokeResponse HandleRunPrepare(NodeInvokeRequest request) + { + var argv = TryParseArgv(request.Args); + if (argv == null || argv.Length == 0) { return Error("Missing command parameter"); } - rawCommand = GetStringArg(request.Args, "rawCommand"); - cwd = GetStringArg(request.Args, "cwd"); + var command = argv[0]; + var rawCommand = GetStringArg(request.Args, "rawCommand"); + var cwd = GetStringArg(request.Args, "cwd"); var agentId = GetStringArg(request.Args, "agentId"); var sessionKey = GetStringArg(request.Args, "sessionKey"); @@ -229,44 +235,22 @@ private async Task HandleRunAsync(NodeInvokeRequest request) // Per OpenClaw spec, "command" is an argv array (e.g. ["echo","Hello"]). // Also accept a plain string for backward compatibility. - string? command = null; - string[]? args = null; + var argv = TryParseArgv(request.Args); + string? command = argv?[0]; + string[]? args = argv?.Length > 1 ? argv.Skip(1).ToArray() : null; - if (request.Args.ValueKind != System.Text.Json.JsonValueKind.Undefined && - request.Args.TryGetProperty("command", out var cmdEl)) + // When command is a string, also check for separate "args" array + if (argv?.Length == 1 && request.Args.TryGetProperty("args", out var argsEl) && + argsEl.ValueKind == System.Text.Json.JsonValueKind.Array) { - if (cmdEl.ValueKind == System.Text.Json.JsonValueKind.Array) - { - var argv = new List(); - foreach (var item in cmdEl.EnumerateArray()) - { - if (item.ValueKind == System.Text.Json.JsonValueKind.String) - argv.Add(item.GetString() ?? ""); - } - if (argv.Count > 0) - { - command = argv[0]; - args = argv.Count > 1 ? argv.Skip(1).ToArray() : null; - } - } - else if (cmdEl.ValueKind == System.Text.Json.JsonValueKind.String) + var list = new List(); + foreach (var item in argsEl.EnumerateArray()) { - command = cmdEl.GetString(); - - // When command is a string, also check for separate "args" array - if (request.Args.TryGetProperty("args", out var argsEl) && - argsEl.ValueKind == System.Text.Json.JsonValueKind.Array) - { - var list = new List(); - foreach (var item in argsEl.EnumerateArray()) - { - if (item.ValueKind == System.Text.Json.JsonValueKind.String) - list.Add(item.GetString() ?? ""); - } - if (list.Count > 0) - args = list.ToArray(); - } + if (item.ValueKind == System.Text.Json.JsonValueKind.String) + list.Add(item.GetString() ?? ""); } + if (list.Count > 0) + args = list.ToArray(); } if (string.IsNullOrWhiteSpace(command)) @@ -402,7 +386,7 @@ private NodeInvokeResponse HandleExecApprovalsSet(NodeInvokeRequest request) if (ruleEl.TryGetProperty("description", out var descEl) && descEl.ValueKind == System.Text.Json.JsonValueKind.String) rule.Description = descEl.GetString(); - if (ruleEl.TryGetProperty("enabled", out var enEl) && enEl.ValueKind == System.Text.Json.JsonValueKind.True || enEl.ValueKind == System.Text.Json.JsonValueKind.False) + if (ruleEl.TryGetProperty("enabled", out var enEl) && (enEl.ValueKind == System.Text.Json.JsonValueKind.True || enEl.ValueKind == System.Text.Json.JsonValueKind.False)) rule.Enabled = enEl.GetBoolean(); if (ruleEl.TryGetProperty("shells", out var shellsEl) && shellsEl.ValueKind == System.Text.Json.JsonValueKind.Array) diff --git a/tests/OpenClaw.Shared.Tests/SystemRunTests.cs b/tests/OpenClaw.Shared.Tests/SystemRunTests.cs index 8be2b53..54c4ea9 100644 --- a/tests/OpenClaw.Shared.Tests/SystemRunTests.cs +++ b/tests/OpenClaw.Shared.Tests/SystemRunTests.cs @@ -194,6 +194,90 @@ public async Task SystemRun_HandlesRunnerException() Assert.Contains("Execution failed", res.Error); } + [Fact] + public async Task SystemRunPrepare_ParsesArgvArray() + { + var cap = new SystemCapability(NullLogger.Instance); + var req = new NodeInvokeRequest + { + Id = "rp1", + Command = "system.run.prepare", + Args = Parse("""{"command":["git","status","--short"]}""") + }; + + var res = await cap.ExecuteAsync(req); + Assert.True(res.Ok, res.Error); + } + + [Fact] + public async Task SystemRunPrepare_ParsesStringCommand() + { + var cap = new SystemCapability(NullLogger.Instance); + var req = new NodeInvokeRequest + { + Id = "rp2", + Command = "system.run.prepare", + Args = Parse("""{"command":"echo hello"}""") + }; + + var res = await cap.ExecuteAsync(req); + Assert.True(res.Ok, res.Error); + } + + [Fact] + public async Task SystemRunPrepare_ReturnsError_WhenMissingCommand() + { + var cap = new SystemCapability(NullLogger.Instance); + var req = new NodeInvokeRequest + { + Id = "rp3", + Command = "system.run.prepare", + Args = Parse("""{}""") + }; + + var res = await cap.ExecuteAsync(req); + Assert.False(res.Ok); + Assert.Contains("Missing command", res.Error); + } + + [Fact] + public async Task SystemRunPrepare_ReturnsError_WhenEmptyArray() + { + var cap = new SystemCapability(NullLogger.Instance); + var req = new NodeInvokeRequest + { + Id = "rp4", + Command = "system.run.prepare", + Args = Parse("""{"command":[]}""") + }; + + var res = await cap.ExecuteAsync(req); + Assert.False(res.Ok); + Assert.Contains("Missing command", res.Error); + } + + [Fact] + public async Task SystemRun_ParsesArgvArrayForRun() + { + var runner = new FakeCommandRunner(); + var cap = new SystemCapability(NullLogger.Instance); + cap.SetCommandRunner(runner); + + var req = new NodeInvokeRequest + { + Id = "ra1", + Command = "system.run", + Args = Parse("""{"command":["git","push","origin","main"]}""") + }; + + var res = await cap.ExecuteAsync(req); + Assert.True(res.Ok); + Assert.Equal("git", runner.LastRequest!.Command); + Assert.NotNull(runner.LastRequest.Args); + Assert.Equal(3, runner.LastRequest.Args!.Length); + Assert.Equal("push", runner.LastRequest.Args[0]); + } + /// /// Fake runner for unit testing — no actual process execution. /// From 6656ef7bcb5877313bc86d8c6fa4ab959f2a42ac Mon Sep 17 00:00:00 2001 From: Scott Hanselman Date: Tue, 31 Mar 2026 14:20:24 -0700 Subject: [PATCH 2/2] Apply NotificationSound setting to all toast notifications (#92) Add ShowToast() helper that checks _settings.NotificationSound and configures audio accordingly (None=silent, Subtle=IM sound, Default= system default). Replace all 14 direct .Show() calls with ShowToast(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Capabilities/SystemCapability.cs | 2 +- src/OpenClaw.Tray.WinUI/App.xaml.cs | 81 ++++++++++--------- 2 files changed, 42 insertions(+), 41 deletions(-) diff --git a/src/OpenClaw.Shared/Capabilities/SystemCapability.cs b/src/OpenClaw.Shared/Capabilities/SystemCapability.cs index bbe7a53..7db3597 100644 --- a/src/OpenClaw.Shared/Capabilities/SystemCapability.cs +++ b/src/OpenClaw.Shared/Capabilities/SystemCapability.cs @@ -199,7 +199,7 @@ private NodeInvokeResponse HandleWhich(NodeInvokeRequest request) private NodeInvokeResponse HandleRunPrepare(NodeInvokeRequest request) { var argv = TryParseArgv(request.Args); - if (argv == null || argv.Length == 0) + if (argv == null || argv.Length == 0 || string.IsNullOrWhiteSpace(argv[0])) { return Error("Missing command parameter"); } diff --git a/src/OpenClaw.Tray.WinUI/App.xaml.cs b/src/OpenClaw.Tray.WinUI/App.xaml.cs index de0780f..06997f0 100644 --- a/src/OpenClaw.Tray.WinUI/App.xaml.cs +++ b/src/OpenClaw.Tray.WinUI/App.xaml.cs @@ -568,10 +568,9 @@ private void CopyDeviceIdToClipboard() global::Windows.ApplicationModel.DataTransfer.Clipboard.SetContent(dataPackage); // Show toast confirming copy - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(LocalizationHelper.GetString("Toast_DeviceIdCopied")) - .AddText(string.Format(LocalizationHelper.GetString("Toast_DeviceIdCopiedDetail"), _nodeService.ShortDeviceId)) - .Show(); + .AddText(string.Format(LocalizationHelper.GetString("Toast_DeviceIdCopiedDetail"), _nodeService.ShortDeviceId))); } catch (Exception ex) { @@ -597,10 +596,9 @@ private void CopyNodeSummaryToClipboard() dataPackage.SetText(summary); global::Windows.ApplicationModel.DataTransfer.Clipboard.SetContent(dataPackage); - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(LocalizationHelper.GetString("Toast_NodeSummaryCopied")) - .AddText(string.Format(LocalizationHelper.GetString("Toast_NodeSummaryCopiedDetail"), _lastNodes.Length)) - .Show(); + .AddText(string.Format(LocalizationHelper.GetString("Toast_NodeSummaryCopiedDetail"), _lastNodes.Length))); } catch (Exception ex) { @@ -654,10 +652,9 @@ private async Task ExecuteSessionActionAsync(string action, string sessionKey, s if (!sent) { - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(LocalizationHelper.GetString("Toast_SessionActionFailed")) - .AddText(LocalizationHelper.GetString("Toast_SessionActionFailedDetail")) - .Show(); + .AddText(LocalizationHelper.GetString("Toast_SessionActionFailedDetail"))); return; } @@ -671,10 +668,9 @@ private async Task ExecuteSessionActionAsync(string action, string sessionKey, s Logger.Warn($"Session action error ({action}): {ex.Message}"); try { - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(LocalizationHelper.GetString("Toast_SessionActionFailed")) - .AddText(ex.Message) - .Show(); + .AddText(ex.Message)); } catch { } } @@ -1157,10 +1153,9 @@ private void OnNodeStatusChanged(object? sender, ConnectionStatus status) { try { - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(LocalizationHelper.GetString("Toast_NodeModeActive")) - .AddText(LocalizationHelper.GetString("Toast_NodeModeActiveDetail")) - .Show(); + .AddText(LocalizationHelper.GetString("Toast_NodeModeActiveDetail"))); } catch { /* ignore */ } } @@ -1176,18 +1171,16 @@ private void OnPairingStatusChanged(object? sender, OpenClaw.Shared.PairingStatu { AddRecentActivity("Node pairing pending", category: "node", dashboardPath: "nodes", nodeId: args.DeviceId); // Show toast with approval instructions - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(LocalizationHelper.GetString("Toast_PairingPending")) - .AddText(string.Format(LocalizationHelper.GetString("Toast_PairingPendingDetail"), args.DeviceId.Substring(0, 16))) - .Show(); + .AddText(string.Format(LocalizationHelper.GetString("Toast_PairingPendingDetail"), args.DeviceId.Substring(0, 16)))); } else if (args.Status == OpenClaw.Shared.PairingStatus.Paired) { AddRecentActivity("Node paired", category: "node", dashboardPath: "nodes", nodeId: args.DeviceId); - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(LocalizationHelper.GetString("Toast_NodePaired")) - .AddText(LocalizationHelper.GetString("Toast_NodePairedDetail")) - .Show(); + .AddText(LocalizationHelper.GetString("Toast_NodePairedDetail"))); } } catch { /* ignore */ } @@ -1200,10 +1193,9 @@ private void OnNodeNotificationRequested(object? sender, OpenClaw.Shared.Capabil // Agent requested a notification via node.invoke system.notify try { - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(args.Title) - .AddText(args.Body) - .Show(); + .AddText(args.Body)); } catch (Exception ex) { @@ -1376,10 +1368,9 @@ private void OnSessionCommandCompleted(object? sender, SessionCommandResult resu dashboardPath: !string.IsNullOrWhiteSpace(result.Key) ? $"sessions/{result.Key}" : "sessions", sessionKey: result.Key); - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(title) - .AddText(message) - .Show(); + .AddText(message)); } catch (Exception ex) { @@ -1434,7 +1425,7 @@ private void OnNotificationReceived(object? sender, OpenClawNotification notific .AddArgument("action", "open_chat")); } - builder.Show(); + ShowToast(builder); } catch (Exception ex) { @@ -1498,10 +1489,9 @@ private async Task RunHealthCheckAsync(bool userInitiated = false) { if (userInitiated) { - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(LocalizationHelper.GetString("Toast_HealthCheck")) - .AddText(LocalizationHelper.GetString("Toast_HealthCheckNotConnected")) - .Show(); + .AddText(LocalizationHelper.GetString("Toast_HealthCheckNotConnected"))); } return; } @@ -1512,10 +1502,9 @@ private async Task RunHealthCheckAsync(bool userInitiated = false) await _gatewayClient.CheckHealthAsync(); if (userInitiated) { - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(LocalizationHelper.GetString("Toast_HealthCheck")) - .AddText(LocalizationHelper.GetString("Toast_HealthCheckSent")) - .Show(); + .AddText(LocalizationHelper.GetString("Toast_HealthCheckSent"))); } } catch (Exception ex) @@ -1523,10 +1512,9 @@ private async Task RunHealthCheckAsync(bool userInitiated = false) Logger.Warn($"Health check failed: {ex.Message}"); if (userInitiated) { - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(LocalizationHelper.GetString("Toast_HealthCheckFailed")) - .AddText(ex.Message) - .Show(); + .AddText(ex.Message)); } } } @@ -1749,13 +1737,12 @@ private void ShowSurfaceImprovementsTipIfNeeded() try { - new ToastContentBuilder() + ShowToast(new ToastContentBuilder() .AddText(LocalizationHelper.GetString("Toast_ActivityStreamTip")) .AddText(LocalizationHelper.GetString("Toast_ActivityStreamTipDetail")) .AddButton(new ToastButton() .SetContent(LocalizationHelper.GetString("Toast_ActivityStreamTipButton")) - .AddArgument("action", "open_activity")) - .Show(); + .AddArgument("action", "open_activity"))); } catch (Exception ex) { @@ -1765,6 +1752,20 @@ private void ShowSurfaceImprovementsTipIfNeeded() #endregion + private void ShowToast(ToastContentBuilder builder) + { + var sound = _settings?.NotificationSound; + if (string.Equals(sound, "None", StringComparison.OrdinalIgnoreCase)) + { + builder.AddAudio(new ToastAudio { Silent = true }); + } + else if (string.Equals(sound, "Subtle", StringComparison.OrdinalIgnoreCase)) + { + builder.AddAudio(new Uri("ms-winsoundevent:Notification.IM"), silent: false); + } + builder.Show(); + } + #region Actions private void OpenDashboard(string? path = null)