diff --git a/AGENTS.md b/AGENTS.md index 7c34d71..98ecd76 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -8,7 +8,7 @@ fail-fast error handling. Keep changes focused and defer to the linked docs for | Project | Role | Target frameworks | |---------|------|-------------------| -| `Invex.Process` | The library: `IProcessRunner` / `ProcessRunner`, `ProcessRunOptions`, `ProcessRunResult`, `ProcessHostExtensions` | `net10.0` | +| `Invex.Process` | The library: `IProcessRunner` / `ProcessRunner`, `ProcessRunOptions`, `ProcessRunResult`, `ProcessException`, `ProcessHostExtensions` | `net10.0` | | `Invex.Process.Tests` | NUnit test suite, including a public API surface snapshot test | `net10.0` | | `_atom` | Atom build definition (`IBuild.cs`) that generates the GitHub Actions workflows | `net10.0` | @@ -40,24 +40,30 @@ docfx docfx.json # add --serve to preview locally ## Architecture overview -The entire public surface is four types in the `Invex.Process` namespace: +The entire public surface is five types in the `Invex.Process` namespace: - **`IProcessRunner`** — the injectable service: synchronous `Run(options)` and asynchronous `RunAsync(options, ct)`. Registered as a singleton; stateless and safe for concurrent use. -- **`ProcessRunner`** — the `internal sealed` default implementation wrapping - `System.Diagnostics.Process`. stdout/stderr are redirected and streamed line-by-line to - `ILogger` in real time via `BeginOutputReadLine`/`BeginErrorReadLine`, while also being fully - captured into the result. +- **`ProcessHostExtensions`** — the `AddProcessRunner()` extension on `IServiceCollection`, which + registers `IProcessRunner` as a singleton backed by the default implementation. - **`ProcessRunOptions`** — immutable record describing one invocation: name, args, working directory, environment variables, per-stream log levels, per-line transforms (`TransformOutput`/`TransformError`), and `AllowFailedResult`. - **`ProcessRunResult`** — immutable record with `ExitCode`, captured `Output`/`Error`, and the originating `RunOptions`. +- **`ProcessException`** — thrown on a non-zero exit (unless `AllowFailedResult`); derives from + `Exception` and carries `ExitCode` and the full `Result`. + +The default implementation, `ProcessRunner`, is `internal sealed` and not part of the public +surface. It wraps `System.Diagnostics.Process`; stdout/stderr are redirected and streamed +line-by-line to `ILogger` in real time via `BeginOutputReadLine`/`BeginErrorReadLine`, while also +being fully captured into the result. ### Behavioral contracts (do not break these) -- **Fail-fast by default**: a non-zero exit code throws a descriptive exception (command line, - exit code, working directory, stderr) unless `AllowFailedResult` is `true`. +- **Fail-fast by default**: a non-zero exit code throws a descriptive `ProcessException` (command + line, exit code, working directory, stderr) unless `AllowFailedResult` is `true`. +- **Environment variable values are never logged** — the invocation line names the variables only. - **Output is always fully captured** into the result regardless of log levels. - **Failure promotion**: on any non-zero exit, stdout/stderr configured below `Information` are re-logged in full at `Information`/`Warning` respectively — regardless of `AllowFailedResult`. diff --git a/README.md b/README.md index 61a121b..836a4ac 100644 --- a/README.md +++ b/README.md @@ -17,8 +17,8 @@ full output capture, and fail-fast error handling. It wraps `System.Diagnostics. instead of being buffered until exit. - **Full output capture.** Complete stdout/stderr are always captured into the result, regardless of log level, so you can inspect them even when logging is suppressed. -- **Fail-fast by default.** A non-zero exit code throws a descriptive exception (including command, exit code, working - directory, and stderr), or you opt in to inspect failures yourself. +- **Fail-fast by default.** A non-zero exit code throws a descriptive `ProcessException` (including command, exit code, + working directory, and stderr, plus the full result), or you opt in to inspect failures yourself. - **Configurable log levels.** Control how the invocation, stdout, and stderr are logged. - **Per-line transforms.** Rewrite or suppress individual output/error lines, for example to redact secrets or filter noisy progress output. diff --git a/api/index.md b/api/index.md index 79c7540..08361fc 100644 --- a/api/index.md +++ b/api/index.md @@ -10,6 +10,7 @@ match the shipped package; the table below is a map of where to start. | [`IProcessRunner`](xref:Invex.Process.IProcessRunner) | Interface | Injectable service for executing external processes. Exposes synchronous `Run` and asynchronous `RunAsync`. Registered as a singleton. | | [`ProcessRunOptions`](xref:Invex.Process.ProcessRunOptions) | Record | Immutable configuration for a single execution: executable name, arguments, working directory, environment variables, log levels, per-line transforms, and failure tolerance. | | [`ProcessRunResult`](xref:Invex.Process.ProcessRunResult) | Record | The result of a completed execution: exit code, full captured stdout/stderr, and the originating options. | +| [`ProcessException`](xref:Invex.Process.ProcessException) | Exception | Thrown on a non-zero exit (unless `AllowFailedResult` is set); carries the exit code and the full `ProcessRunResult`. | | [`ProcessHostExtensions`](xref:Invex.Process.ProcessHostExtensions) | Static class | `IServiceCollection` extensions; call `AddProcessRunner()` once during host setup. | ## Typical flow diff --git a/docs/handling-failures.md b/docs/handling-failures.md index b907080..d06af03 100644 --- a/docs/handling-failures.md +++ b/docs/handling-failures.md @@ -5,8 +5,8 @@ to opt out of the default exception when a failure is an expected outcome. ## Fail-fast by default -When a process exits with a **non-zero exit code**, `Run` and `RunAsync` throw an exception rather -than returning a result. This means build steps and tools fail immediately without callers having +When a process exits with a **non-zero exit code**, `Run` and `RunAsync` throw a `ProcessException` +rather than returning a result. This means build steps and tools fail immediately without callers having to remember to check exit codes: ```csharp @@ -31,6 +31,21 @@ It includes: - the working directory (when one was set), - the captured stderr (when the process wrote any). +`ProcessException` derives from `Exception` and also exposes the failure programmatically: +`ExitCode` and `Result` (the full `ProcessRunResult`, with captured stdout/stderr and the original +`ProcessRunOptions`): + +```csharp +try +{ + processRunner.Run(new ProcessRunOptions("git", "rev-parse HEAD")); +} +catch (ProcessException ex) when (ex.ExitCode == 128) +{ + Console.WriteLine($"Not a git repository: {ex.Result.Error}"); +} +``` + ## Tolerating failures with `AllowFailedResult` Sometimes a non-zero exit code is an *expected* outcome you want to inspect — linters, format @@ -87,4 +102,5 @@ catch (Win32Exception ex) - [Logging](logging.md): failure promotion in detail. - [Testing Your Code](testing.md): simulating failures in unit tests. -- , and +- , , and + diff --git a/docs/introduction.md b/docs/introduction.md index d499500..b5edba6 100644 --- a/docs/introduction.md +++ b/docs/introduction.md @@ -54,7 +54,7 @@ It is probably not the right tool when: ## Design principles -- **Fail fast by default.** A non-zero exit code throws a descriptive exception that includes the +- **Fail fast by default.** A non-zero exit code throws a descriptive `ProcessException` that includes the command line, exit code, working directory, and captured stderr. Opt out per-invocation with `AllowFailedResult` when you want to handle failures yourself. - **Never lose output.** Full stdout and stderr are always captured into the result, regardless of diff --git a/docs/logging.md b/docs/logging.md index da15cc9..901aa4b 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -11,10 +11,11 @@ Each invocation produces three kinds of log entries: 1. **The invocation line** — logged once when the process starts: ```text - Run: dotnet build --configuration Release in /repo/src with env CI=true + Run: dotnet build --configuration Release in /repo/src with env CI, NUGET_API_KEY ``` - The working directory and environment variables are included only when set. + The working directory and environment variables are included only when set. Only the + environment variable *names* are logged, never their values. 2. **Output lines** — each stdout line, streamed in real time as the process runs. @@ -63,8 +64,10 @@ failure). The common pattern is therefore: Two things to watch: -- The **invocation line** includes the command arguments and any environment variables you set. - Avoid passing secrets as plain arguments/variables, or lower `InvocationLogLevel`. +- The **invocation line** includes the full command arguments and the *names* (not values) of any + environment variables you set. Prefer passing secrets through `EnvironmentVariables`; if a + secret must go on the command line, lower `InvocationLogLevel` (or set it to `LogLevel.None`). + The exception thrown on a non-zero exit also includes the full command line. - **Output lines** may echo secrets printed by the tool. Use `TransformOutput` / `TransformError` to redact or suppress them — see [Transforming Output](transforming-output.md). diff --git a/docs/testing.md b/docs/testing.md index 5894771..67ddcc8 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -64,7 +64,10 @@ A.CallTo(() => processRunner.RunAsync(A._, A processRunner.RunAsync(A._, A._)) - .Throws(new Exception("Process 'git' failed with exit code 128")); + .Throws((ProcessRunOptions options, CancellationToken _) => + new ProcessException( + "Process 'git' failed with exit code 128", + new ProcessRunResult(options, 128, "", "fatal: not a git repository"))); ``` ## Tips diff --git a/docs/transforming-output.md b/docs/transforming-output.md index 8de6df7..e2627d3 100644 --- a/docs/transforming-output.md +++ b/docs/transforming-output.md @@ -26,8 +26,8 @@ The redaction applies to both the live log stream *and* the captured `result.Out > [!WARNING] > Transforms only see the process's output lines. The invocation line (`Run: deploy --token …`) is logged before the -> process starts and includes the full arguments and any environment variables you set, so a secret passed on the -> command line still reaches the log at `InvocationLogLevel`. See [Logging](logging.md#secrets-in-logs). +> process starts and includes the full arguments (environment variables are logged by name only), so a secret passed +> on the command line still reaches the log at `InvocationLogLevel`. See [Logging](logging.md#secrets-in-logs). ## Filtering noise diff --git a/src/Invex.Process/ProcessException.cs b/src/Invex.Process/ProcessException.cs new file mode 100644 index 0000000..4eedd01 --- /dev/null +++ b/src/Invex.Process/ProcessException.cs @@ -0,0 +1,61 @@ +namespace Invex.Process; + +/// +/// The exception thrown by when a process exits with a non-zero +/// exit code and is false. +/// +/// +/// +/// The describes the failure (command line, exit code, +/// working directory when set, and any captured stderr), and exposes +/// the full result of the failed run — including the captured stdout and stderr and the +/// originating — for programmatic inspection. +/// +/// +/// Derives directly from , so existing catch (Exception) +/// blocks continue to handle it. +/// +/// +/// +/// Inspecting a failed run: +/// +/// try +/// { +/// processRunner.Run(new ProcessRunOptions("git", "rev-parse HEAD")); +/// } +/// catch (ProcessException ex) when (ex.ExitCode == 128) +/// { +/// logger.LogWarning("Not a git repository: {Error}", ex.Result.Error); +/// } +/// +/// +[PublicAPI] +[SuppressMessage("Roslynator", + "RCS1194:Implement exception constructors", + Justification = "A ProcessException always describes a failed run, so every constructor requires its result.")] +public sealed class ProcessException : Exception +{ + /// + /// Initializes a new instance of for a failed process run. + /// + /// The message that describes the failure. + /// The result of the failed process run. + public ProcessException(string message, ProcessRunResult result) : base(message) + { + Result = result; + } + + /// + /// Gets the result of the failed process run, including the captured stdout and stderr and + /// the options the process was run with. + /// + public ProcessRunResult Result { get; } + + /// + /// Gets the non-zero exit code the process returned. + /// + /// + /// Shorthand for Result.ExitCode. + /// + public int ExitCode => Result.ExitCode; +} diff --git a/src/Invex.Process/ProcessRunOptions.cs b/src/Invex.Process/ProcessRunOptions.cs index 2300741..309241f 100644 --- a/src/Invex.Process/ProcessRunOptions.cs +++ b/src/Invex.Process/ProcessRunOptions.cs @@ -8,8 +8,9 @@ /// /// All properties have defaults suited to typical build-automation use: invocation is /// logged at , stdout at , -/// and stderr at . A non-zero exit code throws by default -/// so build steps fail fast without requiring callers to check exit codes manually. +/// and stderr at . A non-zero exit code throws a +/// by default so build steps fail fast without requiring +/// callers to check exit codes manually. /// /// /// Both stdout and stderr are fully captured into regardless @@ -60,8 +61,18 @@ public ProcessRunOptions(string Name, string[] Args) : this(Name, /// (e.g. Run: dotnet build --configuration Release). /// /// - /// Defaults to . Lower this to - /// for noisy or frequently-called sub-processes. + /// + /// Defaults to . Lower this to + /// for noisy or frequently-called sub-processes. + /// + /// + /// The invocation line includes the full , the + /// when set, and the names of any + /// . Environment variable values are never logged, + /// so they are a safer way to pass secrets than command-line arguments. When a secret + /// must be passed as an argument, lower this level (or set it to + /// ) to keep it out of the log. + /// /// public LogLevel InvocationLogLevel { get; init; } = LogLevel.Information; @@ -122,9 +133,15 @@ public ProcessRunOptions(string Name, string[] Args) : this(Name, /// Gets the environment variables to inject into the process's environment. /// /// - /// Keys are variable names and values are their string values. A null value removes - /// the inherited variable from the child process's environment. Variables not listed here - /// are inherited from the current process unchanged. + /// + /// Keys are variable names and values are their string values. A null value + /// removes the inherited variable from the child process's environment. Variables not + /// listed here are inherited from the current process unchanged. + /// + /// + /// Only the variable names appear in the invocation log line (see + /// ); their values are never logged. + /// /// public Dictionary EnvironmentVariables { get; init; } = []; diff --git a/src/Invex.Process/ProcessRunner.cs b/src/Invex.Process/ProcessRunner.cs index 8c1d5cf..5e647a7 100644 --- a/src/Invex.Process/ProcessRunner.cs +++ b/src/Invex.Process/ProcessRunner.cs @@ -42,11 +42,11 @@ public interface IProcessRunner /// Only returned when the exit code is zero, or when /// is true. /// - /// + /// /// Thrown when the process exits with a non-zero code and /// is false. The message /// includes the command line, exit code, working directory (when set), and any captured - /// stderr. + /// stderr; carries the full captured result. /// /// /// Thrown when the executable named by cannot be @@ -68,11 +68,11 @@ public interface IProcessRunner /// Only completes successfully when the exit code is zero, or when /// is true. /// - /// + /// /// Thrown (on the returned task) when the process exits with a non-zero code and /// is false. The message /// includes the command line, exit code, working directory (when set), and any captured - /// stderr. + /// stderr; carries the full captured result. /// /// /// Thrown when the executable named by cannot be @@ -108,8 +108,8 @@ public interface IProcessRunner /// ( / ) /// are below . This happens regardless of /// , ensuring failure diagnostics are -/// visible even when per-line output logging was suppressed. A descriptive exception is -/// then thrown unless is true. +/// visible even when per-line output logging was suppressed. A descriptive +/// is then thrown unless is true. /// /// /// The logger for capturing process execution diagnostics. @@ -127,7 +127,7 @@ public ProcessRunResult Run(ProcessRunOptions options) options.Name, options.Args, options.WorkingDirectory, - string.Join(", ", options.EnvironmentVariables.Select(kv => $"{kv.Key}={kv.Value}"))); + FormatEnvironmentVariableNames(options)); break; case { WorkingDirectory.Length: > 0 }: @@ -143,7 +143,7 @@ public ProcessRunResult Run(ProcessRunOptions options) "Run: {Name} {Args} with env {EnvironmentVariables}", options.Name, options.Args, - string.Join(", ", options.EnvironmentVariables.Select(kv => $"{kv.Key}={kv.Value}"))); + FormatEnvironmentVariableNames(options)); break; default: @@ -206,7 +206,7 @@ public ProcessRunResult Run(ProcessRunOptions options) return result; } - throw new(BuildProcessErrorMessage(options, result.ExitCode, error)); + throw new ProcessException(BuildProcessErrorMessage(options, result.ExitCode, error), result); void OnProcessOnErrorDataReceived(object _, DataReceivedEventArgs e) { @@ -254,7 +254,7 @@ public async Task RunAsync( options.Name, options.Args, options.WorkingDirectory, - string.Join(", ", options.EnvironmentVariables.Select(kv => $"{kv.Key}={kv.Value}"))); + FormatEnvironmentVariableNames(options)); break; case { WorkingDirectory.Length: > 0 }: @@ -270,7 +270,7 @@ public async Task RunAsync( "Run: {Name} {Args} with env {EnvironmentVariables}", options.Name, options.Args, - string.Join(", ", options.EnvironmentVariables.Select(kv => $"{kv.Key}={kv.Value}"))); + FormatEnvironmentVariableNames(options)); break; default: @@ -333,7 +333,7 @@ public async Task RunAsync( return result; } - throw new(BuildProcessErrorMessage(options, result.ExitCode, error)); + throw new ProcessException(BuildProcessErrorMessage(options, result.ExitCode, error), result); void OnProcessOnErrorDataReceived(object _, DataReceivedEventArgs e) { @@ -368,6 +368,18 @@ void OnProcessOnOutputDataReceived(object _, DataReceivedEventArgs e) } } + /// + /// Formats the names of the configured environment variables for the invocation log line. + /// + /// + /// Only the variable names are logged, never their values, because environment variables + /// are a common way to pass secrets (tokens, passwords) to child processes. + /// + /// The options whose environment variables are formatted. + /// A comma-separated list of the variable names. + private static string FormatEnvironmentVariableNames(ProcessRunOptions options) => + string.Join(", ", options.EnvironmentVariables.Keys); + private static string BuildProcessErrorMessage(ProcessRunOptions options, int exitCode, string error) { var errorMessage = new StringBuilder(); diff --git a/tests/Invex.Process.Tests/ProcessRunnerTests.cs b/tests/Invex.Process.Tests/ProcessRunnerTests.cs index 600fcfb..41cbf9e 100644 --- a/tests/Invex.Process.Tests/ProcessRunnerTests.cs +++ b/tests/Invex.Process.Tests/ProcessRunnerTests.cs @@ -1,4 +1,4 @@ -namespace Invex.Process.Tests; +namespace Invex.Process.Tests; /// /// Integration tests for that spawn real OS processes. @@ -206,6 +206,57 @@ public void Run_WithEnvironmentVariable_ProcessReceivesIt() .ShouldContain(value); } + [Test] + public void Run_WithEnvironmentVariable_LogsNameButNotValue() + { + // Arrange + var logger = new RecordingLogger(); + var runner = new ProcessRunner(logger); + const string key = "ATOM_TEST_SECRET_RUNNER"; + const string value = "super-secret-value"; + + var options = EchoEnvVarOptions(key) with + { + EnvironmentVariables = new() + { + [key] = value, + }, + TransformOutput = _ => null, + }; + + // Act + runner.Run(options); + + // Assert + logger + .Messages + .Where(m => m.StartsWith("Run: ", StringComparison.Ordinal)) + .ShouldHaveSingleItem() + .ShouldContain(key); + logger.Messages.ShouldAllBe(m => !m.Contains(value)); + } + + [Test] + public void Run_WhenExitCodeIsNonZero_ThrowsProcessExceptionWithResult() + { + // Arrange + var runner = CreateRunner(); + + var options = FailOptions() with + { + AllowFailedResult = false, + }; + + // Act + var exception = Should.Throw(() => runner.Run(options)); + + // Assert + exception.ExitCode.ShouldBe(1); + exception.Result.ExitCode.ShouldBe(1); + exception.Result.RunOptions.ShouldBeSameAs(options); + exception.Message.ShouldContain("exit code 1"); + } + [Test] public void Run_WithTransformOutput_TransformIsAppliedToEachLine() { @@ -363,6 +414,57 @@ public async Task RunAsync_WithEnvironmentVariable_ProcessReceivesIt() .ShouldContain(value); } + [Test] + public async Task RunAsync_WithEnvironmentVariable_LogsNameButNotValue() + { + // Arrange + var logger = new RecordingLogger(); + var runner = new ProcessRunner(logger); + const string key = "ATOM_TEST_SECRET_RUNNER_ASYNC"; + const string value = "async-super-secret-value"; + + var options = EchoEnvVarOptions(key) with + { + EnvironmentVariables = new() + { + [key] = value, + }, + TransformOutput = _ => null, + }; + + // Act + await runner.RunAsync(options); + + // Assert + logger + .Messages + .Where(m => m.StartsWith("Run: ", StringComparison.Ordinal)) + .ShouldHaveSingleItem() + .ShouldContain(key); + logger.Messages.ShouldAllBe(m => !m.Contains(value)); + } + + [Test] + public async Task RunAsync_WhenExitCodeIsNonZero_ThrowsProcessExceptionWithResult() + { + // Arrange + var runner = CreateRunner(); + + var options = FailOptions() with + { + AllowFailedResult = false, + }; + + // Act + var exception = await Should.ThrowAsync(() => runner.RunAsync(options)); + + // Assert + exception.ExitCode.ShouldBe(1); + exception.Result.ExitCode.ShouldBe(1); + exception.Result.RunOptions.ShouldBeSameAs(options); + exception.Message.ShouldContain("exit code 1"); + } + [Test] public async Task RunAsync_WithTransformOutput_TransformIsAppliedToEachLine() { @@ -428,4 +530,39 @@ public async Task RunAsync_WhenCancelledMidRun_ThrowsOperationCanceledException( // Act / Assert await Should.ThrowAsync(() => runner.RunAsync(LongRunningOptions(), cts.Token)); } + + /// + /// A minimal that records every formatted message. + /// + private sealed class RecordingLogger : ILogger + { + private readonly List _messages = []; + + public IReadOnlyList Messages + { + get + { + lock (_messages) + return [.. _messages]; + } + } + + public IDisposable? BeginScope(TState state) + where TState : notnull => + null; + + public bool IsEnabled(LogLevel logLevel) => + true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + lock (_messages) + _messages.Add(formatter(state, exception)); + } + } } diff --git a/tests/Invex.Process.Tests/PublicApiTests.VerifyPublicApiSurface.verified.txt b/tests/Invex.Process.Tests/PublicApiTests.VerifyPublicApiSurface.verified.txt index 0a3ae0c..e7eaaed 100644 --- a/tests/Invex.Process.Tests/PublicApiTests.VerifyPublicApiSurface.verified.txt +++ b/tests/Invex.Process.Tests/PublicApiTests.VerifyPublicApiSurface.verified.txt @@ -10,6 +10,17 @@ } ] }, + { + Name: Invex.Process.ProcessException, + Members: [ + { + Name: ExitCode + }, + { + Name: Result + } + ] + }, { Name: Invex.Process.ProcessHostExtensions, Members: [