Make ICommandExecutor's synchronous path a real primitive

CLAUDE.md

@@ -46,7 +46,7 @@ This is a .NET library (`ktsu.Essentials`) providing high-performance interfaces
 - `Essentials/IValidationProvider.cs` - Validation interface with structured results
 - `Essentials/ILoggingProvider.cs` - Logging interface with six severity levels
 - `Essentials/INavigationProvider.cs` - Browser-like back/forward navigation interface
-- `Essentials/ICommandExecutor.cs` - Shell command execution interface
+- `Essentials/ICommandExecutor.cs` - Shell command execution interface; `Execute(command, environmentVariables, workingDirectory, cancellationToken)` is the synchronous primitive that every other synchronous member composes over
 - `Essentials/IFileSystemProvider.cs` - Filesystem abstraction extending Testably.Abstractions
 - `Essentials/ProviderHelpers.cs` - Internal utilities for async wrapping, stream bridging, UTF8 transforms
 - `Essentials/PersistenceProviderUtilities.cs` - Shared utilities for persistence providers (safe filenames, key conversion)
@@ -90,6 +90,15 @@ All provider interfaces follow a consistent three-tier pattern:
 2. **Convenience methods**: Self-allocating methods that call Try\* methods and manage buffers automatically. Provided via default interface implementations.
 3. **Async variants**: Task-based async versions with `CancellationToken` support. The stream paths of the compression providers and of `AesEncryptionProvider`, along with `IHashProvider.TryHashAsync(Stream, ...)` and `IKeyedHashProvider.TryHashAsync(ReadOnlyMemory<byte>, Stream, ...)`, are genuinely asynchronous — real `ReadAsync`/`WriteAsync`, no thread held. The rest are still `Task.Run` wrappers over synchronous work via `ProviderHelpers.RunAsync()`; see issue #8. A provider makes its stream paths genuine by declaring the two `Try…Async(Stream, Stream, ...)` primitives itself, which replaces the default implementation; the four derived stream defaults compose over those primitives, so overriding two members converts all six. Span-destination async overloads do not exist — an `out` parameter cannot cross an async boundary.
 
+`ICommandExecutor` is the one interface with the mirror-image concern: synchronous methods layered over an
+asynchronous one. It declares a synchronous primitive, `Execute(string, IReadOnlyDictionary<string, string>?,
+string?, CancellationToken)`, that the other two synchronous members compose over. Its default body bridges to
+`ExecuteAsync` with `GetAwaiter().GetResult()` — not `.Result`, which wraps the failure in an
+`AggregateException` — and still blocks a thread for the child process's lifetime. `NativeCommandExecutor`
+declares the primitive itself and drives `System.Diagnostics.Process` synchronously (`BeginOutputReadLine` plus
+a `WaitForExit(timeout)` poll that honours the cancellation token), so no pool thread is held; declaring that
+one member converts all three synchronous overloads. See issue #17.
+
 Common patterns are centralized in `ProviderHelpers.cs`:
 
 - `RunAsync()` - Wraps sync methods in `Task.Run` with cancellation. Used by the in-memory async variants and by any stream path whose provider has not declared its own asynchronous primitives.
@@ -109,7 +118,7 @@ Tests use **MSTest.Sdk** targeting net10.0 only. The test project (`Essentials.T
 - `IncrementalHashTests.cs` - Tests `CreateIncremental()` and async stream hashing across all 15 hash providers, asserting incremental output equals one-shot output
 - `KeyedHashProviderTests.cs` - Tests all 3 HMAC keyed hash providers, `Verify`, and `FixedTimeComparison`
 - `CacheProviderTests.cs` - Tests cache operations including expiration
-- `CommandExecutorTests.cs` - Tests command execution
+- `CommandExecutorTests.cs` - Tests command execution, including the synchronous path, cancellation before and during a run, a working directory that does not exist, and that `ExecuteAndGetOutput` throws unwrapped. `ICommandExecutor`'s own synchronous defaults are reached through a test double that declares only the asynchronous members, since `NativeCommandExecutor` replaces them
 - `EncodingProviderTests.cs` - Tests Base64 and Hex encoding
 - `ObfuscationProviderTests.cs` - Tests all obfuscation providers via round-trip (obfuscate → deobfuscate)
 - `FileSystemProviderTests.cs` - Tests filesystem operations
@@ -122,6 +131,52 @@ Tests use **MSTest.Sdk** targeting net10.0 only. The test project (`Essentials.T
 
 ## CI/CD
 
+### Running the ktsu analyzers locally
+
+`ktsu.Sdk.Analyzers` requires a newer Roslyn than some installed SDKs carry. When it does not match,
+every build fails with `CSC : error CS9057: Analyzer assembly ... references version '5.9.0.0' of the
+compiler, which is newer than the currently running version` — and the analyzers never run, so
+`KTSU****` findings are invisible until CI reports them. CI uses SDK 10.0.400, which carries Roslyn
+5.9.
+
+Rather than chase the SDK, override the compiler from NuGet. Put this in a file outside the
+repository and point MSBuild at it:
+
+```xml
+<Project>
+  <ItemGroup>
+    <PackageReference Include="Microsoft.Net.Compilers.Toolset" VersionOverride="5.9.0" PrivateAssets="all" />
+  </ItemGroup>
+</Project>
+```
+
+```bash
+dotnet build Essentials.slnx -p:CustomAfterMicrosoftCommonProps=/path/to/roslyn59.props
+```
+
+Note **`After`**, not `Before`: projects here declare their SDK with `<Sdk Name="..." />` elements
+rather than the `<Project Sdk="...">` attribute, which `CustomBeforeMicrosoftCommonProps` does not
+reach. Keep the file out of the repository so normal builds, CI and packaging are unaffected.
+
+### Two analyzer rules that contradict each other
+
+`KTSU0001` requires a `System.Memory` reference from every project using `Span<T>`/`Memory<T>`, and
+the 47 projects targeting netstandard2.1 do. That reference **cannot be added**: NuGet rejects it
+during solution restore with `NU1510` — *"This package is automatically available and does not need
+to be referenced explicitly. Remove the PackageReference item."* `NoWarn` metadata on the item does
+not reach that check, so the two rules cannot both be satisfied. NuGet is the one describing
+reality — the framework supplies the package — so `KTSU0001` is suppressed instead, in
+`Directory.Build.targets`, scoped to netstandard2.1.
+
+It has to be `Directory.Build.targets`, not `.props`: ktsu.Sdk assigns `NoWarn` outright, and props
+is imported before the SDK, so an addition there is silently overwritten. Check with
+`dotnet msbuild <proj> -p:TargetFramework=netstandard2.1 -getProperty:NoWarn` if it ever stops
+working.
+
+`CA1859` (use concrete types) is likewise wrong for `Essentials.Tests` and is in its `NoWarn`: the
+providers are built on default interface implementations, which are only callable through the
+interface, so binding a test to the concrete type would change or break what it dispatches to.
+
 Uses `scripts/PSBuild.psm1` PowerShell module for CI pipeline. Version increments are controlled by commit message tags: `[major]`, `[minor]`, `[patch]`, `[pre]`.
 
 ## Code Quality

Directory.Build.targets

@@ -0,0 +1,21 @@
+<Project>
+
+  <!--
+    KTSU0001 wants every project using Span<T>/Memory<T> to reference System.Memory, and the 47
+    projects here that target netstandard2.1 do. That reference cannot be added: on the SDK CI
+    builds with, NuGet rejects it outright during solution restore — "This package is automatically
+    available and does not need to be referenced explicitly. Remove the PackageReference item."
+    (NU1510, where NoWarn metadata on the item does not reach it).
+
+    The two rules cannot both be satisfied, and NuGet is the one describing reality: the framework
+    supplies the package, so KTSU0001's premise no longer holds on this SDK. Scoped to
+    netstandard2.1, the only target it fires on.
+
+    This lives in Directory.Build.targets rather than Directory.Build.props because ktsu.Sdk assigns
+    NoWarn outright, and props is imported before the SDK - an addition there is overwritten.
+  -->
+  <PropertyGroup Condition="'$(TargetFramework)' == 'netstandard2.1'">
+    <NoWarn>$(NoWarn);KTSU0001</NoWarn>
+  </PropertyGroup>
+
+</Project>

Essentials.All/Essentials.All.csproj

@@ -7,6 +7,10 @@
     <Description>Batteries-included package that references every ktsu.Essentials provider implementation and registers them all with one AddEssentials() call.</Description>
   </PropertyGroup>
 
+  <ItemGroup>
+    <InternalsVisibleTo Include="ktsu.Essentials.Tests" />
+  </ItemGroup>
+
   <ItemGroup>
     <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" />
     <PackageReference Include="Polyfill" PrivateAssets="All" />

Essentials.CommandExecutors.Native/NativeCommandExecutor.cs

@@ -7,6 +7,7 @@ namespace ktsu.Essentials.CommandExecutors.Native;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Runtime.InteropServices;
+using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
 
@@ -15,6 +16,11 @@ namespace ktsu.Essentials.CommandExecutors.Native;
 /// </summary>
 public class NativeCommandExecutor : ICommandExecutor
 {
+	/// <summary>
+	/// How long the synchronous wait blocks before checking the cancellation token again.
+	/// </summary>
+	private const int CancellationPollIntervalMilliseconds = 50;
+
 	/// <summary>
 	/// Executes a command asynchronously and returns the result.
 	/// </summary>
@@ -39,32 +45,13 @@ public async Task<CommandResult> ExecuteAsync(string command, IReadOnlyDictionar
 
 		if (cancellationToken.IsCancellationRequested)
 		{
-			return new CommandResult(-1, string.Empty, "Operation was cancelled.");
+			return Cancelled();
 		}
 
 		try
 		{
-			bool isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
-
 			using Process process = new();
-			process.StartInfo = new ProcessStartInfo
-			{
-				FileName = isWindows ? "cmd.exe" : "/bin/sh",
-				Arguments = isWindows ? $"/c {command}" : $"-c \"{command.Replace("\"", "\\\"")}\"",
-				WorkingDirectory = workingDirectory ?? string.Empty,
-				RedirectStandardOutput = true,
-				RedirectStandardError = true,
-				UseShellExecute = false,
-				CreateNoWindow = true,
-			};
-
-			if (environmentVariables is not null)
-			{
-				foreach (KeyValuePair<string, string> kvp in environmentVariables)
-				{
-					process.StartInfo.Environment[kvp.Key] = kvp.Value;
-				}
-			}
+			process.StartInfo = CreateStartInfo(command, environmentVariables, workingDirectory);
 
 			process.Start();
 
@@ -89,7 +76,7 @@ public async Task<CommandResult> ExecuteAsync(string command, IReadOnlyDictionar
 		}
 		catch (OperationCanceledException)
 		{
-			return new CommandResult(-1, string.Empty, "Operation was cancelled.");
+			return Cancelled();
 		}
 		catch (InvalidOperationException ex)
 		{
@@ -100,4 +87,141 @@ public async Task<CommandResult> ExecuteAsync(string command, IReadOnlyDictionar
 			return new CommandResult(-1, string.Empty, ex.Message);
 		}
 	}
+
+	/// <summary>
+	/// Executes a command synchronously with custom environment variables and returns the result.
+	/// </summary>
+	/// <param name="command">The command to execute.</param>
+	/// <param name="environmentVariables">Optional environment variables to set for the command.</param>
+	/// <param name="workingDirectory">The optional working directory for the command.</param>
+	/// <param name="cancellationToken">A cancellation token to cancel the operation.</param>
+	/// <returns>A <see cref="CommandResult"/> containing the exit code, standard output, and standard error.</returns>
+	/// <remarks>
+	/// Declaring the synchronous primitive replaces <see cref="ICommandExecutor"/>'s default, which bridges
+	/// to the asynchronous path and blocks a thread-pool thread for the lifetime of the child process. This
+	/// implementation drives <see cref="Process"/> synchronously instead, so no thread is borrowed from the
+	/// pool and no <see cref="Task"/> is awaited.
+	/// <para>
+	/// Output is captured through the asynchronous read handlers rather than
+	/// <see cref="System.IO.StreamReader.ReadToEnd"/>, because reading one redirected stream to the end while
+	/// the other fills its buffer deadlocks.
+	/// </para>
+	/// </remarks>
+	public CommandResult Execute(string command, IReadOnlyDictionary<string, string>? environmentVariables, string? workingDirectory = null, CancellationToken cancellationToken = default)
+	{
+		Ensure.NotNull(command);
+
+		if (cancellationToken.IsCancellationRequested)
+		{
+			return Cancelled();
+		}
+
+		try
+		{
+			using Process process = new();
+			process.StartInfo = CreateStartInfo(command, environmentVariables, workingDirectory);
+
+			StringBuilder stdout = new();
+			StringBuilder stderr = new();
+			process.OutputDataReceived += (_, e) => AppendLine(stdout, e.Data);
+			process.ErrorDataReceived += (_, e) => AppendLine(stderr, e.Data);
+
+			process.Start();
+			process.BeginOutputReadLine();
+			process.BeginErrorReadLine();
+
+			while (!process.WaitForExit(CancellationPollIntervalMilliseconds))
+			{
+				if (cancellationToken.IsCancellationRequested)
+				{
+					TryKill(process);
+					return Cancelled();
+				}
+			}
+
+			// WaitForExit(int) can return before the asynchronous read handlers have drained the streams.
+			// The parameterless overload waits for them, and is documented as the way to flush them.
+			process.WaitForExit();
+
+			return new CommandResult(process.ExitCode, stdout.ToString(), stderr.ToString());
+		}
+		catch (InvalidOperationException ex)
+		{
+			return new CommandResult(-1, string.Empty, ex.Message);
+		}
+		catch (System.ComponentModel.Win32Exception ex)
+		{
+			return new CommandResult(-1, string.Empty, ex.Message);
+		}
+	}
+
+	/// <summary>
+	/// Builds the <see cref="ProcessStartInfo"/> that runs <paramref name="command"/> through the platform shell.
+	/// </summary>
+	/// <param name="command">The command to execute.</param>
+	/// <param name="environmentVariables">Optional environment variables to set for the command.</param>
+	/// <param name="workingDirectory">The optional working directory for the command.</param>
+	/// <returns>The configured start info.</returns>
+	private static ProcessStartInfo CreateStartInfo(string command, IReadOnlyDictionary<string, string>? environmentVariables, string? workingDirectory)
+	{
+		bool isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
+
+		ProcessStartInfo startInfo = new()
+		{
+			FileName = isWindows ? "cmd.exe" : "/bin/sh",
+			Arguments = isWindows ? $"/c {command}" : $"-c \"{command.Replace("\"", "\\\"")}\"",
+			WorkingDirectory = workingDirectory ?? string.Empty,
+			RedirectStandardOutput = true,
+			RedirectStandardError = true,
+			UseShellExecute = false,
+			CreateNoWindow = true,
+		};
+
+		if (environmentVariables is not null)
+		{
+			foreach (KeyValuePair<string, string> kvp in environmentVariables)
+			{
+				startInfo.Environment[kvp.Key] = kvp.Value;
+			}
+		}
+
+		return startInfo;
+	}
+
+	/// <summary>
+	/// Appends one line of redirected output, ignoring the null that signals end of stream.
+	/// </summary>
+	/// <param name="buffer">The buffer to append to. Only the one handler thread for that stream writes to it, and the parameterless <see cref="Process.WaitForExit()"/> publishes those writes to the caller.</param>
+	/// <param name="line">The line received, or null at end of stream.</param>
+	private static void AppendLine(StringBuilder buffer, string? line)
+	{
+		if (line is not null)
+		{
+			buffer.AppendLine(line);
+		}
+	}
+
+	/// <summary>
+	/// Kills a process that is being abandoned because the operation was cancelled, ignoring the races
+	/// where it has already exited or was never started.
+	/// </summary>
+	/// <param name="process">The process to kill.</param>
+	private static void TryKill(Process process)
+	{
+		try
+		{
+			process.Kill();
+		}
+		catch (Exception ex) when (ex is InvalidOperationException or System.ComponentModel.Win32Exception)
+		{
+			// The process exited between the wait timing out and this call, or could not be terminated.
+		}
+	}
+
+	/// <summary>
+	/// Builds the result returned when an operation is cancelled.
+	/// </summary>
+	/// <returns>A failed <see cref="CommandResult"/> describing the cancellation.</returns>
+	private static CommandResult Cancelled() =>
+		new(-1, string.Empty, "Operation was cancelled.");
 }