From 34d1ba043f01b23e231fe093ac9eaabae5aefa09 Mon Sep 17 00:00:00 2001 From: Allan Thraen Date: Wed, 11 Nov 2026 08:31:08 +0100 Subject: [PATCH] fix(run-commands): open a dev server's URL once it is listening A run command's URL opened only when the command exited 0, and the commands it is used with are dev servers (npm run dev, npm start, node serve.js) that never exit, so the URL never opened. It now opens once, on whichever comes first: - a loopback URL starts accepting TCP connections while the run is alive (polled every 500ms for up to 2 minutes, every address of the host in parallel, since localhost is both ::1 and 127.0.0.1); - the run exits 0, which is the old rule, kept for one-shot commands. Remote URLs and SSH parents keep the exit-0 rule only. A remote URL is reachable before a deploy finishes, and an SSH run's server listens on the remote machine's loopback. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- src/CodeShellManager/Services/RunInstance.cs | 155 +++++++++++-- .../Views/SessionRunCommandsDialog.xaml | 2 +- .../PostRunUrlReadinessTests.cs | 203 ++++++++++++++++++ 4 files changed, 344 insertions(+), 18 deletions(-) create mode 100644 tests/CodeShellManager.Tests/PostRunUrlReadinessTests.cs diff --git a/CLAUDE.md b/CLAUDE.md index 022f113..c6984e1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1017,7 +1017,7 @@ Each session can have a list of "run commands" — labelled command lines invoke **Data:** `ShellSession.RunCommands: List { Id, Label, CommandLine, IsDefault, Mode, PostRunUrl }`. Exactly one item has `IsDefault=true`; see `RunCommandItem.EnsureSingleDefault`. Persisted to `state.json`. - **`Mode`** (`RunMode.Process` default / `RunMode.PowerShell`) — `Process` runs through `cmd /c` as before; `PowerShell` wraps the command line in `pwsh.exe -NonInteractive -NoLogo -ExecutionPolicy Bypass -EncodedCommand ` (falls back to `powershell.exe` if `pwsh` isn't on PATH). SSH and WSL parents ignore `Mode` — those runs always go through a POSIX shell (`ssh … bash -c` / `wsl.exe … -e -lc`, where the WSL shell is probed per-distro and falls back to `sh` where bash is absent). Use PowerShell when the command relies on pipes (`|`), redirection (`>`), `$env:` variables, or cmdlets. -- **`PostRunUrl`** (`string?`, default `null`) — when set and the run exits with code 0, `Process.Start` opens the URL via `UseShellExecute=true` (default browser). No health-check polling. The value is gated by `RunInstance.IsLaunchableUrl` first: **only absolute `http`/`https` URLs are launched.** ShellExecute would otherwise run a local exe, a `.ps1`, a UNC path or any registered protocol handler, and this fires automatically with no confirmation — and `ImportExportService` deserializes a whole `AppState` (run commands included) from any JSON file the user points at, so the stored value is not trusted. Rejections and launch failures both append to `crash.log`; neither pops UI, since this runs on the PTY-exit callback thread. Scheme-less input (`localhost:5173`) is rejected rather than guessed at. +- **`PostRunUrl`** (`string?`, default `null`) — opened once in the default browser (`UseShellExecute=true`), by whichever comes first: **a loopback URL starts accepting TCP connections** while the run is alive, or the run **exits 0**. The readiness half exists because a URL is almost always a dev server's (`npm run dev`), and a server never exits — with only the exit-0 rule the URL never opened. The watcher (`RunInstance.StartReadyWatcher`) polls every 500ms for up to 2 minutes, connects to every address of the host in parallel (`localhost` is `::1` *and* `127.0.0.1`, servers bind one, and a refused loopback connect costs ~2s on Windows), and stops on exit, Stop or Dispose. It is **loopback-only and skipped for SSH parents**: a remote URL (a deploy target) is reachable before the command has done anything, and an SSH run's server listens on the remote machine's loopback. Those keep the exit-0 rule. Tested in `PostRunUrlReadinessTests`, against real listeners for the probe. The value is gated by `RunInstance.IsLaunchableUrl` first: **only absolute `http`/`https` URLs are launched.** ShellExecute would otherwise run a local exe, a `.ps1`, a UNC path or any registered protocol handler, and this fires automatically with no confirmation — and `ImportExportService` deserializes a whole `AppState` (run commands included) from any JSON file the user points at, so the stored value is not trusted. Rejections and launch failures both append to `crash.log`; neither pops UI, since this runs on the PTY-exit callback thread. Scheme-less input (`localhost:5173`) is rejected rather than guessed at. **Templates:** `RunCommandTemplatesService.SeedFor(folder)` detects project type (top-level scan, first-match: dotnet → cargo → node → python → make) and returns a seed list with fresh Ids. Templates are *copied* onto new sessions at creation time; subsequent edits don't propagate back. SSH sessions skip detection (empty list). diff --git a/src/CodeShellManager/Services/RunInstance.cs b/src/CodeShellManager/Services/RunInstance.cs index 5971571..5bc1c15 100644 --- a/src/CodeShellManager/Services/RunInstance.cs +++ b/src/CodeShellManager/Services/RunInstance.cs @@ -1,6 +1,10 @@ using System; using System.Diagnostics; using System.IO; +using System.Net; +using System.Net.Sockets; +using System.Threading; +using System.Threading.Tasks; using System.Text; using System.Text.RegularExpressions; using CodeShellManager.Models; @@ -50,6 +54,22 @@ public partial class RunInstance : ObservableObject, IDisposable private long _totalAppended; private bool _disposed; + // PostRunUrl is opened at most once per run: by the readiness watcher (a server that has + // started listening) or by a successful exit (a one-shot command), whichever comes first. + private int _urlOpened; + private CancellationTokenSource? _readyCts; + + /// How long the readiness watcher keeps probing a loopback URL before giving up. + internal TimeSpan ReadyTimeout { get; set; } = TimeSpan.FromMinutes(2); + internal TimeSpan ReadyPollInterval { get; set; } = TimeSpan.FromMilliseconds(500); + + /// Test seam: "is something listening behind this URL?". Default: a TCP connect. + internal Func> ReadyProbe { get; set; } = IsListeningAsync; + + /// Test seam: opens the validated URL. Default: ShellExecute → default browser. + internal Action UrlLauncher { get; set; } = + static url => Process.Start(new ProcessStartInfo(url) { UseShellExecute = true }); + public event Action? OutputChanged; public event Action? StateChanged; @@ -136,6 +156,7 @@ public void Start(ShellSession parent) } _pty.Start(command, args, workDir, cols: 200, rows: 50, useJobObject: true); + StartReadyWatcher(parent); } catch (Exception ex) { @@ -211,26 +232,127 @@ private void OnPtyExited() State = ExitCode == 0 ? RunState.ExitedOk : RunState.ExitedFailed; StateChanged?.Invoke(); - // Open post-run URL on success only. ShellExecute hands the URL to the - // OS default browser. We can't pop UI from the PTY-exit callback thread, - // so failures are logged to crash.log for diagnosability rather than silenced. - if (State == RunState.ExitedOk && !string.IsNullOrWhiteSpace(PostRunUrl)) + _readyCts?.Cancel(); + + // A one-shot command (build, deploy) opens its URL when it succeeds. A server never + // exits, so for that case the readiness watcher started in Start() opens it instead. + if (State == RunState.ExitedOk) OpenPostRunUrlOnce(); + } + + /// + /// A run command with a PostRunUrl is usually a dev server (`npm run dev`, `node serve.js`), + /// and a server never exits — so "open the URL when the command exits 0" meant the URL never + /// opened at all. While the run is alive, probe the URL and open it once something is + /// listening there. + /// + /// Only for loopback URLs, and not for SSH parents. A remote URL (a deploy target) is + /// reachable before the command has done anything, so probing it would open the page before + /// the deploy finished — those keep the exit-0 rule. For an SSH parent the server listens on + /// the REMOTE machine's loopback, which a local probe cannot see. + /// + private void StartReadyWatcher(ShellSession parent) + { + if (parent.Kind == SessionKind.Ssh) return; + if (!TryGetLaunchableUrl(PostRunUrl, out string? safeUrl)) return; + var uri = new Uri(safeUrl!); + if (!IsLoopbackHost(uri)) return; + + var cts = new CancellationTokenSource(); + _readyCts = cts; + var probe = ReadyProbe; + var timeout = ReadyTimeout; + var interval = ReadyPollInterval; + _ = Task.Run(async () => { - if (!TryGetLaunchableUrl(PostRunUrl, out string? safeUrl)) + var deadline = DateTime.UtcNow + timeout; + try { - LogPostRunUrl(PostRunUrl, "rejected — only http and https URLs are opened"); - return; + while (!cts.IsCancellationRequested && DateTime.UtcNow < deadline) + { + bool ready; + try { ready = await probe(uri, cts.Token).ConfigureAwait(false); } + catch (OperationCanceledException) { return; } + catch { ready = false; } + if (cts.IsCancellationRequested) return; + if (ready) { OpenPostRunUrlOnce(); return; } + await Task.Delay(interval, cts.Token).ConfigureAwait(false); + } } - // safeUrl, not PostRunUrl: launch the string the validator actually inspected. - // Uri.TryCreate accepts and internally escapes characters that the raw string - // still contains — a quote or space survives into ShellExecute, which expands it - // into the handler's registered `shell\open\command` template. Modern browsers - // pass --single-argument and are unaffected; other registered handlers may not - // be. Validating one string and launching a different one is the bug class, - // regardless of who is currently immune to it. - try { Process.Start(new ProcessStartInfo(safeUrl!) { UseShellExecute = true }); } - catch (Exception ex) { LogPostRunUrl(PostRunUrl, ex.Message); } + catch (OperationCanceledException) { } + }); + } + + /// localhost, 127.0.0.0/8, [::1] — and 0.0.0.0 / [::], which dev servers print. + internal static bool IsLoopbackHost(Uri uri) + { + if (uri.IsLoopback) return true; + return IPAddress.TryParse(uri.Host.Trim('[', ']'), out var ip) + && (ip.Equals(IPAddress.Any) || ip.Equals(IPAddress.IPv6Any)); + } + + private static async Task IsListeningAsync(Uri uri, CancellationToken ct) + { + string host = uri.Host.Trim('[', ']'); + if (host == "0.0.0.0") host = "127.0.0.1"; + else if (host == "::") host = "::1"; + + // "localhost" is ::1 AND 127.0.0.1, and a server usually binds only one of them (Vite on + // Node 17+ binds ::1, many others 127.0.0.1). Probe every address in parallel: on + // Windows a refused loopback connect takes ~2s (SYN retries), so trying them in turn + // inside one timeout never reached the second address. + IPAddress[] addresses; + try + { + addresses = IPAddress.TryParse(host, out var literal) + ? [literal] + : await Dns.GetHostAddressesAsync(host, ct).ConfigureAwait(false); + } + catch (SocketException) { return false; } + + var attempts = new Task[addresses.Length]; + for (int i = 0; i < addresses.Length; i++) + attempts[i] = ConnectsAsync(new IPEndPoint(addresses[i], uri.Port), ct); + foreach (bool ok in await Task.WhenAll(attempts).ConfigureAwait(false)) + if (ok) return true; + return false; + } + + private static async Task ConnectsAsync(IPEndPoint endpoint, CancellationToken ct) + { + using var socket = new Socket(endpoint.AddressFamily, SocketType.Stream, ProtocolType.Tcp); + using var attempt = CancellationTokenSource.CreateLinkedTokenSource(ct); + attempt.CancelAfter(TimeSpan.FromSeconds(3)); + try + { + await socket.ConnectAsync(endpoint, attempt.Token).ConfigureAwait(false); + return true; + } + catch (OperationCanceledException) { return false; } + catch (SocketException) { return false; } + } + + private void OpenPostRunUrlOnce() + { + if (string.IsNullOrWhiteSpace(PostRunUrl)) return; + if (Interlocked.Exchange(ref _urlOpened, 1) != 0) return; + + // ShellExecute hands the URL to the OS default browser. This runs on the PTY-exit + // callback thread or the watcher's pool thread, so failures go to crash.log for + // diagnosability rather than popping UI. + if (!TryGetLaunchableUrl(PostRunUrl, out string? safeUrl)) + { + LogPostRunUrl(PostRunUrl, "rejected — only http and https URLs are opened"); + return; } + // safeUrl, not PostRunUrl: launch the string the validator actually inspected. + // Uri.TryCreate accepts and internally escapes characters that the raw string + // still contains — a quote or space survives into ShellExecute, which expands it + // into the handler's registered `shell\open\command` template. Modern browsers + // pass --single-argument and are unaffected; other registered handlers may not + // be. Validating one string and launching a different one is the bug class, + // regardless of who is currently immune to it. + try { UrlLauncher(safeUrl!); } + catch (Exception ex) { LogPostRunUrl(PostRunUrl, ex.Message); } } /// @@ -390,6 +512,7 @@ public void Dispose() { if (_disposed) return; _disposed = true; + _readyCts?.Cancel(); if (_pty != null) { _pty.DataReceived -= OnPtyData; diff --git a/src/CodeShellManager/Views/SessionRunCommandsDialog.xaml b/src/CodeShellManager/Views/SessionRunCommandsDialog.xaml index 496b253..e3445c1 100644 --- a/src/CodeShellManager/Views/SessionRunCommandsDialog.xaml +++ b/src/CodeShellManager/Views/SessionRunCommandsDialog.xaml @@ -106,7 +106,7 @@ Background="#313244" Foreground="#cdd6f4" BorderBrush="#45475a" CaretBrush="#cdd6f4" Padding="6,4" Margin="0,0,8,0" VerticalContentAlignment="Center" - ToolTip="Optional. Opens in the default browser when the command exits 0. Must be a full http:// or https:// URL — e.g. http://localhost:5173, not localhost:5173."/> + ToolTip="Optional. Opens in the default browser once — for a localhost URL as soon as the server starts listening there, otherwise when the command exits 0. Must be a full http:// or https:// URL — e.g. http://localhost:5173, not localhost:5173."/> +public class PostRunUrlReadinessTests +{ + private sealed class FakePty : IPseudoTerminal + { + public int? ExitCode { get; private set; } +#pragma warning disable CS0067 // never raised: these tests don't need output + public event Action? DataReceived; +#pragma warning restore CS0067 + public event Action? Exited; + public void Start(string command, string args, string workingDirectory, + int cols = 220, int rows = 50, bool useJobObject = false) { } + public void Exit(int code) { ExitCode = code; Exited?.Invoke(); } + public void Dispose() { } + } + + private static readonly TimeSpan Wait = TimeSpan.FromSeconds(5); + private static readonly TimeSpan Short = TimeSpan.FromMilliseconds(250); + + private static (RunInstance run, FakePty pty, BlockingCollection opened) Make( + string? url, Func>? probe = null) + { + var pty = new FakePty(); + var run = new RunInstance( + new RunCommandItem { Id = "r", Label = "Run", CommandLine = "npm run dev", PostRunUrl = url }, + () => pty); + var opened = new BlockingCollection(); + run.UrlLauncher = opened.Add; + run.ReadyPollInterval = TimeSpan.FromMilliseconds(10); + if (probe != null) run.ReadyProbe = probe; + return (run, pty, opened); + } + + private static ShellSession Local() => new() { WorkingFolder = Environment.CurrentDirectory }; + + [Fact] + public void Server_OpensWhenListening_WithoutExiting() + { + int calls = 0; + var (run, _, opened) = Make("http://localhost:5173", + (_, _) => Task.FromResult(Interlocked.Increment(ref calls) >= 3)); + run.Start(Local()); + + Assert.True(opened.TryTake(out var url, Wait)); + Assert.Equal("http://localhost:5173/", url); + Assert.Equal(RunState.Running, run.State); + run.Dispose(); + } + + [Fact] + public void Server_ThenExitZero_OpensOnlyOnce() + { + var (run, pty, opened) = Make("http://127.0.0.1:4377", (_, _) => Task.FromResult(true)); + run.Start(Local()); + Assert.True(opened.TryTake(out _, Wait)); + pty.Exit(0); + Assert.False(opened.TryTake(out _, Short)); + } + + [Fact] + public void OneShot_ExitZero_Opens() + { + var (run, pty, opened) = Make("http://localhost:8000", (_, _) => Task.FromResult(false)); + run.Start(Local()); + pty.Exit(0); + Assert.True(opened.TryTake(out _, Wait)); + } + + [Fact] + public void ExitNonZero_BeforeReady_DoesNotOpen() + { + var (run, pty, opened) = Make("http://localhost:8000", (_, _) => Task.FromResult(false)); + run.Start(Local()); + pty.Exit(1); + Assert.False(opened.TryTake(out _, Short)); + } + + [Fact] + public void Stop_BeforeReady_DoesNotOpenLater() + { + int ready = 0; + var (run, _, opened) = Make("http://localhost:8000", + (_, _) => Task.FromResult(Volatile.Read(ref ready) == 1)); + run.Start(Local()); + run.Stop(); + Volatile.Write(ref ready, 1); + Assert.False(opened.TryTake(out _, Short)); + } + + [Fact] + public void RemoteUrl_IsNotProbed_OpensOnlyOnExitZero() + { + int calls = 0; + var (run, pty, opened) = Make("https://example.com/deployed", + (_, _) => { Interlocked.Increment(ref calls); return Task.FromResult(true); }); + run.Start(Local()); + Assert.False(opened.TryTake(out _, Short)); + Assert.Equal(0, Volatile.Read(ref calls)); + pty.Exit(0); + Assert.True(opened.TryTake(out _, Wait)); + } + + [Fact] + public void SshParent_IsNotProbed() + { + int calls = 0; + var (run, _, opened) = Make("http://localhost:5173", + (_, _) => { Interlocked.Increment(ref calls); return Task.FromResult(true); }); + run.Start(new ShellSession { Kind = SessionKind.Ssh, SshHost = "box", SshUser = "u" }); + Assert.False(opened.TryTake(out _, Short)); + Assert.Equal(0, Volatile.Read(ref calls)); + run.Dispose(); + } + + [Fact] + public void NonHttpUrl_NeverProbedOrOpened() + { + int calls = 0; + var (run, pty, opened) = Make(@"C:\Windows\System32\calc.exe", + (_, _) => { Interlocked.Increment(ref calls); return Task.FromResult(true); }); + run.Start(Local()); + pty.Exit(0); + Assert.False(opened.TryTake(out _, Short)); + Assert.Equal(0, Volatile.Read(ref calls)); + } + + [Theory] + [InlineData("http://localhost:5173", true)] + [InlineData("http://127.0.0.1:4377", true)] + [InlineData("http://127.5.0.1/", true)] + [InlineData("http://[::1]:8080/", true)] + [InlineData("http://0.0.0.0:8000", true)] + [InlineData("http://192.168.1.10:8000", false)] + [InlineData("https://example.com", false)] + [InlineData("http://localhost.example.com", false)] + public void IsLoopbackHost(string url, bool expected) + => Assert.Equal(expected, RunInstance.IsLoopbackHost(new Uri(url))); + + [Theory] + [InlineData("127.0.0.1", "http://localhost:{0}")] // IPv4-only server, reached via "localhost" + [InlineData("::1", "http://localhost:{0}")] // Vite on Node 17+ binds ::1 only + [InlineData("127.0.0.1", "http://0.0.0.0:{0}")] + public void DefaultProbe_DetectsRealListener(string bindAddress, string urlFormat) + { + var listener = new TcpListener(IPAddress.Parse(bindAddress), 0); + listener.Start(); + try + { + int port = ((IPEndPoint)listener.LocalEndpoint).Port; + var pty = new FakePty(); + var run = new RunInstance(new RunCommandItem + { + Id = "r", Label = "Run", CommandLine = "x", + PostRunUrl = string.Format(urlFormat, port), + }, () => pty); + var opened = new BlockingCollection(); + run.UrlLauncher = opened.Add; + run.Start(Local()); + Assert.True(opened.TryTake(out _, Wait)); + run.Dispose(); + } + finally { listener.Stop(); } + } + + [Fact] + public void DefaultProbe_NothingListening_DoesNotOpen() + { + // Grab a free port, then release it so nothing listens there. + var l = new TcpListener(IPAddress.Loopback, 0); + l.Start(); + int port = ((IPEndPoint)l.LocalEndpoint).Port; + l.Stop(); + + var pty = new FakePty(); + var run = new RunInstance(new RunCommandItem + { + Id = "r", Label = "Run", CommandLine = "x", PostRunUrl = $"http://127.0.0.1:{port}", + }, () => pty); + var opened = new BlockingCollection(); + run.UrlLauncher = opened.Add; + run.ReadyPollInterval = TimeSpan.FromMilliseconds(20); + run.Start(Local()); + Assert.False(opened.TryTake(out _, TimeSpan.FromMilliseconds(600))); + run.Dispose(); + } +}