diff --git a/code-reviews/CLI/findings.md b/code-reviews/CLI/findings.md index c0570e8..f11efce 100644 --- a/code-reviews/CLI/findings.md +++ b/code-reviews/CLI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 6 | +| Open findings | 0 | ## Summary @@ -318,7 +318,7 @@ env vars (see CLI-006). |--|--| | Severity | Low | | Category | Code organization & conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Program.cs:10-11`, `src/ScadaLink.CLI/Commands/CommandHelpers.cs:60` | **Description** @@ -334,7 +334,12 @@ Restrict the option to the accepted values, e.g. `formatOption.AcceptOnlyFromAmo **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed — the `--format` option was +constructed inline in `Program.cs` with no value constraint. Extracted option +construction into a new `CliOptions.CreateFormatOption()` factory which applies +`AcceptOnlyFromAmong("json", "table")`; `Program.cs` now uses it. Invalid values are +rejected by `System.CommandLine` with a clear parse error. Regression tests in +`FormatOptionValidationTests`. ### CLI-009 — Exit-code documentation does not match `HandleResponse` behaviour @@ -342,7 +347,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Documentation & comments | -| Status | Open | +| Status | Resolved | | Location | `docs/requirements/Component-CLI.md:238-249`, `src/ScadaLink.CLI/Commands/CommandHelpers.cs:75` | **Description** @@ -365,7 +370,19 @@ with whichever is chosen. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Took the recommendation's second option — keying +the authorization exit code off the response `code` field — since it makes the CLI honour +the documented "authorization failure → exit 2" contract regardless of which channel the +server uses, and it is the in-scope (code) fix. Replaced +`return response.StatusCode == 403 ? 2 : 1;` with a `HandleResponse` → +`IsAuthorizationFailure` helper that returns exit 2 for HTTP 403 **or** an error code of +`UNAUTHORIZED`/`FORBIDDEN` (case-insensitive), and exit 1 for everything else. +Authentication failure (HTTP 401 / bad credentials) deliberately remains exit 1. +Regression tests in `ExitCodeTests`. Note: the doc-side clarification of the full +client-side exit-code list (`NO_URL`, `NO_CREDENTIALS`, etc. → exit 1) was not made here — +`docs/requirements/Component-CLI.md` is outside this module's editable surface; the +remaining work is a non-blocking documentation-completeness edit owned by the docs +surface, and the CLI's exit-code behaviour itself is now correct and pinned by tests. ### CLI-010 — `debug stream` reports Ctrl+C during connect as a connection failure @@ -373,7 +390,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Commands/DebugCommands.cs:181-189` | **Description** @@ -394,7 +411,13 @@ lines 209-215 already treats cancellation as graceful. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed — the `StartAsync` `catch` +block reported every exception as `CONNECTION_FAILED`/exit 1. Extracted a pure +`DebugStreamHelpers.ClassifyConnectFailure(ex, cancellationRequested)` classifier: an +`OperationCanceledException` that coincides with a user-requested cancellation is treated +as a graceful shutdown (exit 0, no error printed); anything else is a genuine connection +failure (exit 1). The `StartAsync` catch block now uses it and disposes the connection on +both paths. Regression tests in `DebugStreamTests` (`ClassifyConnectFailure_*`). ### CLI-011 — `CancellationTokenSource` in `debug stream` is never disposed @@ -402,7 +425,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Commands/DebugCommands.cs:89` | **Description** @@ -420,7 +443,12 @@ a `try/finally`). **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed — `cts` was a plain local with +no disposal on any exit path. Changed to `using var cts = new CancellationTokenSource();` +in `StreamDebugAsync`, so it is disposed on every return path including the early +connect/subscribe-failure returns. No dedicated regression test — undisposed-`IDisposable` +is not observable from a unit test; the `using` declaration is verified by inspection and +covered indirectly by the `DebugStreamTests` exit-path tests. ### CLI-012 — `debug stream` exit code is unreliable after stream termination @@ -428,7 +456,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Concurrency & thread safety | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Commands/DebugCommands.cs:208-227` | **Description** @@ -452,7 +480,15 @@ Consider awaiting `exitTcs.Task` without the cancellation token after a brief gr **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed — the final +`exitTcs.Task.IsCompletedSuccessfully ? ... : 0` could miss an in-flight `TrySetResult` +that races with the cancelled `WaitAsync`. Extracted +`DebugStreamHelpers.ResolveStreamExitCodeAsync(exitTask)`: it returns an already-set +result immediately, otherwise waits up to a 250 ms grace period (`Task.WhenAny` against +`Task.Delay`) for a termination/closed result to land, and falls back to exit 0 only when +no result is ever produced (pure Ctrl+C). `StreamDebugAsync` now resolves its exit code +solely through this helper. Regression tests in `DebugStreamTests` +(`ResolveStreamExitCodeAsync_*`, including the termination-racing-with-cancellation case). ### CLI-013 — HTTP client, `debug stream`, and JSON-argument parsing are untested @@ -460,7 +496,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Testing coverage | -| Status | Open | +| Status | Resolved | | Location | `tests/ScadaLink.CLI.Tests/` (vs. `src/ScadaLink.CLI/ManagementHttpClient.cs`, `src/ScadaLink.CLI/Commands/DebugCommands.cs`, `src/ScadaLink.CLI/Commands/InstanceCommands.cs:55-58`) | **Description** @@ -487,4 +523,17 @@ resolves via `ManagementCommandRegistry`. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Coverage gaps confirmed and closed: +- `ManagementHttpClientTests` — added an `internal` test-only `ManagementHttpClient` + constructor accepting a pre-built `HttpClient`, and a stub `HttpMessageHandler`; tests + cover the success, JSON error-body, non-JSON error-body fallback, connection-failure + (status 0), and timeout (504) paths. +- `CommandTreeTests` — builds all 14 command groups, recursively collects every leaf + command and asserts each has an action (no dead wiring), and verifies representative + command payload records round-trip through `ManagementCommandRegistry`. +- `DebugStreamTests` — covers the `debug stream` decision logic via the new + `DebugStreamHelpers` (see CLI-010, CLI-012). +- JSON-argument parsing (`set-bindings`/`set-overrides`) was already extracted into + `InstanceCommands.TryParseBindings`/`TryParseOverrides` and covered by + `InstanceArgumentParsingTests` under CLI-005. +The CLI test suite went from 42 to 77 passing tests. diff --git a/src/ScadaLink.CLI/Commands/CliOptions.cs b/src/ScadaLink.CLI/Commands/CliOptions.cs new file mode 100644 index 0000000..dfc60b5 --- /dev/null +++ b/src/ScadaLink.CLI/Commands/CliOptions.cs @@ -0,0 +1,30 @@ +using System.CommandLine; + +namespace ScadaLink.CLI.Commands; + +/// +/// Factory methods for the global CLI options. Centralising option construction keeps +/// validation rules (e.g. the accepted --format values) in one place and makes +/// them testable without standing up the whole command tree. +/// +internal static class CliOptions +{ + /// + /// Creates the global --format option. The option deliberately has no + /// DefaultValueFactory — format precedence (explicit flag → config/env → + /// "json") is resolved by , which + /// needs to distinguish an absent flag. The accepted values are constrained so a + /// typo (e.g. --format tabel) is rejected with a clear parse error rather + /// than silently falling through to JSON. + /// + internal static Option CreateFormatOption() + { + var formatOption = new Option("--format") + { + Description = "Output format (json or table)", + Recursive = true, + }; + formatOption.AcceptOnlyFromAmong("json", "table"); + return formatOption; + } +} diff --git a/src/ScadaLink.CLI/Commands/CommandHelpers.cs b/src/ScadaLink.CLI/Commands/CommandHelpers.cs index eb393c7..6d6dc9b 100644 --- a/src/ScadaLink.CLI/Commands/CommandHelpers.cs +++ b/src/ScadaLink.CLI/Commands/CommandHelpers.cs @@ -132,7 +132,24 @@ internal static class CommandHelpers var error = response.Error ?? "Unknown error"; OutputFormatter.WriteError(error, errorCode); - return response.StatusCode == 403 ? 2 : 1; + return IsAuthorizationFailure(response) ? 2 : 1; + } + + /// + /// Determines whether an error response represents an authorization failure + /// (insufficient role), which the documented exit-code table maps to exit code 2. + /// An HTTP 403 status is the primary signal; the server may also signal it via an + /// UNAUTHORIZED / FORBIDDEN error code on a different HTTP status, so + /// both channels are honoured. (Authentication failure — HTTP 401 / bad credentials + /// — is deliberately not treated as authorization failure; it is exit 1.) + /// + private static bool IsAuthorizationFailure(ManagementResponse response) + { + if (response.StatusCode == 403) + return true; + + return string.Equals(response.ErrorCode, "FORBIDDEN", StringComparison.OrdinalIgnoreCase) + || string.Equals(response.ErrorCode, "UNAUTHORIZED", StringComparison.OrdinalIgnoreCase); } private static void WriteAsTable(string json) diff --git a/src/ScadaLink.CLI/Commands/DebugCommands.cs b/src/ScadaLink.CLI/Commands/DebugCommands.cs index b6bcc70..e080df7 100644 --- a/src/ScadaLink.CLI/Commands/DebugCommands.cs +++ b/src/ScadaLink.CLI/Commands/DebugCommands.cs @@ -94,7 +94,8 @@ public static class DebugCommands .WithAutomaticReconnect() .Build(); - var cts = new CancellationTokenSource(); + // CLI-011: CancellationTokenSource owns a WaitHandle and must be disposed. + using var cts = new CancellationTokenSource(); var exitTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); Console.CancelKeyPress += (_, e) => @@ -192,8 +193,18 @@ public static class DebugCommands } catch (Exception ex) { + // CLI-010: Ctrl+C during connect throws OperationCanceledException — that is + // a graceful user cancellation, not a connection failure. + var failure = DebugStreamHelpers.ClassifyConnectFailure(ex, cts.IsCancellationRequested); + if (failure.IsCancellation) + { + await connection.DisposeAsync(); + return failure.ExitCode; + } + OutputFormatter.WriteError($"Connection failed: {ex.Message}", "CONNECTION_FAILED"); - return 1; + await connection.DisposeAsync(); + return failure.ExitCode; } try @@ -232,7 +243,11 @@ public static class DebugCommands } await connection.DisposeAsync(); - return exitTcs.Task.IsCompletedSuccessfully ? exitTcs.Task.Result : 0; + + // CLI-012: resolve the exit code from a single authoritative source. A result + // set by OnStreamTerminated/Closed always wins; a brief grace period covers a + // termination racing with Ctrl+C. Pure Ctrl+C (no result) is a graceful exit 0. + return await DebugStreamHelpers.ResolveStreamExitCodeAsync(exitTcs.Task); } private static void PrintSnapshotTable(JsonElement snapshot) diff --git a/src/ScadaLink.CLI/Commands/DebugStreamHelpers.cs b/src/ScadaLink.CLI/Commands/DebugStreamHelpers.cs new file mode 100644 index 0000000..845e428 --- /dev/null +++ b/src/ScadaLink.CLI/Commands/DebugStreamHelpers.cs @@ -0,0 +1,54 @@ +namespace ScadaLink.CLI.Commands; + +/// +/// Pure, testable helpers for the debug stream command. The SignalR-driven +/// body itself cannot be unit-tested without a live hub, so +/// the decision logic — connect-failure classification (CLI-010) and exit-code +/// resolution after stream termination (CLI-012) — is extracted here. +/// +internal static class DebugStreamHelpers +{ + /// + /// The maximum time waits for an in-flight + /// TrySetResult (from OnStreamTerminated/Closed) to land after + /// the wait was cancelled by Ctrl+C, so a termination racing with cancellation is + /// observed deterministically rather than depending on scheduling. + /// + internal static readonly TimeSpan ExitGracePeriod = TimeSpan.FromMilliseconds(250); + + /// Outcome of classifying an exception thrown while connecting. + internal readonly record struct ConnectFailure(bool IsCancellation, int ExitCode); + + /// + /// Classifies an exception thrown by HubConnection.StartAsync. A + /// cancellation exception that coincides with a user-requested cancellation + /// (Ctrl+C during connect) is a graceful shutdown — exit 0, no error printed. + /// Anything else is a genuine connection failure — exit 1. + /// + internal static ConnectFailure ClassifyConnectFailure(Exception ex, bool cancellationRequested) + { + if (cancellationRequested && ex is OperationCanceledException) + return new ConnectFailure(IsCancellation: true, ExitCode: 0); + + return new ConnectFailure(IsCancellation: false, ExitCode: 1); + } + + /// + /// Resolves the debug stream exit code from a single authoritative source — + /// the exitTcs task. If a result was set by OnStreamTerminated or the + /// Closed handler it is always preferred (even when Ctrl+C also fired); + /// a brief grace period covers a termination that races with cancellation. If no + /// result is ever produced (pure Ctrl+C), the stream ended gracefully — exit 0. + /// + internal static async Task ResolveStreamExitCodeAsync(Task exitTask) + { + if (exitTask.IsCompletedSuccessfully) + return exitTask.Result; + + var completed = await Task.WhenAny(exitTask, Task.Delay(ExitGracePeriod)); + if (ReferenceEquals(completed, exitTask) && exitTask.IsCompletedSuccessfully) + return exitTask.Result; + + return 0; + } +} diff --git a/src/ScadaLink.CLI/ManagementHttpClient.cs b/src/ScadaLink.CLI/ManagementHttpClient.cs index 3026024..515bf42 100644 --- a/src/ScadaLink.CLI/ManagementHttpClient.cs +++ b/src/ScadaLink.CLI/ManagementHttpClient.cs @@ -9,8 +9,19 @@ public class ManagementHttpClient : IDisposable private readonly HttpClient _httpClient; public ManagementHttpClient(string baseUrl, string username, string password) + : this(new HttpClient(), baseUrl, username, password) { - _httpClient = new HttpClient { BaseAddress = new Uri(baseUrl.TrimEnd('/') + "/") }; + } + + /// + /// Test-only constructor that accepts a pre-built (typically + /// over a stub ) so the request/response handling can + /// be exercised without a live server. + /// + internal ManagementHttpClient(HttpClient httpClient, string baseUrl, string username, string password) + { + _httpClient = httpClient; + _httpClient.BaseAddress = new Uri(baseUrl.TrimEnd('/') + "/"); var credentials = Convert.ToBase64String(Encoding.UTF8.GetBytes($"{username}:{password}")); _httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", credentials); diff --git a/src/ScadaLink.CLI/Program.cs b/src/ScadaLink.CLI/Program.cs index 3cb5e2a..08da7d7 100644 --- a/src/ScadaLink.CLI/Program.cs +++ b/src/ScadaLink.CLI/Program.cs @@ -9,7 +9,8 @@ var usernameOption = new Option("--username") { Description = "LDAP user var passwordOption = new Option("--password") { Description = "LDAP password", Recursive = true }; // No DefaultValueFactory: format precedence (explicit --format -> config/env -> "json") // is resolved by CommandHelpers.ResolveFormat, which needs to distinguish an absent flag. -var formatOption = new Option("--format") { Description = "Output format (json or table)", Recursive = true }; +// CliOptions.CreateFormatOption also constrains the accepted values (json/table). +var formatOption = CliOptions.CreateFormatOption(); rootCommand.Add(urlOption); rootCommand.Add(usernameOption); diff --git a/tests/ScadaLink.CLI.Tests/CommandTreeTests.cs b/tests/ScadaLink.CLI.Tests/CommandTreeTests.cs new file mode 100644 index 0000000..387ddc9 --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/CommandTreeTests.cs @@ -0,0 +1,88 @@ +using System.CommandLine; +using ScadaLink.CLI.Commands; +using ScadaLink.Commons.Messages.Management; + +namespace ScadaLink.CLI.Tests; + +/// +/// Regression tests for CLI-013 — the command-tree wiring was untested. These tests +/// build every command group and assert the tree is well-formed (every leaf has an +/// action, no group is empty), and that every management command record the CLI sends +/// resolves via (so command-name derivation +/// never throws at runtime). +/// +public class CommandTreeTests +{ + private static readonly Option Url = new("--url") { Recursive = true }; + private static readonly Option Username = new("--username") { Recursive = true }; + private static readonly Option Password = new("--password") { Recursive = true }; + private static readonly Option Format = CliOptions.CreateFormatOption(); + + private static IEnumerable AllCommandGroups() => new[] + { + TemplateCommands.Build(Url, Format, Username, Password), + InstanceCommands.Build(Url, Format, Username, Password), + SiteCommands.Build(Url, Format, Username, Password), + DeployCommands.Build(Url, Format, Username, Password), + DataConnectionCommands.Build(Url, Format, Username, Password), + ExternalSystemCommands.Build(Url, Format, Username, Password), + NotificationCommands.Build(Url, Format, Username, Password), + SecurityCommands.Build(Url, Format, Username, Password), + AuditLogCommands.Build(Url, Format, Username, Password), + HealthCommands.Build(Url, Format, Username, Password), + DebugCommands.Build(Url, Format, Username, Password), + SharedScriptCommands.Build(Url, Format, Username, Password), + DbConnectionCommands.Build(Url, Format, Username, Password), + ApiMethodCommands.Build(Url, Format, Username, Password), + }; + + private static IEnumerable LeafCommands(Command command) + { + if (command.Subcommands.Count == 0) + { + yield return command; + yield break; + } + + foreach (var sub in command.Subcommands) + foreach (var leaf in LeafCommands(sub)) + yield return leaf; + } + + [Fact] + public void AllCommandGroups_Build_WithoutThrowing() + { + var groups = AllCommandGroups().ToList(); + Assert.Equal(14, groups.Count); + Assert.All(groups, g => Assert.False(string.IsNullOrWhiteSpace(g.Name))); + } + + [Fact] + public void EveryLeafCommand_HasAnAction() + { + // A leaf command with no action is dead wiring — invoking it would do nothing. + var leaves = AllCommandGroups().SelectMany(LeafCommands).ToList(); + + Assert.NotEmpty(leaves); + Assert.All(leaves, leaf => + Assert.True(leaf.Action != null, $"Leaf command '{leaf.Name}' has no action.")); + } + + [Theory] + [InlineData(typeof(GetInstanceCommand))] + [InlineData(typeof(ListSitesCommand))] + [InlineData(typeof(CreateTemplateCommand))] + [InlineData(typeof(SetConnectionBindingsCommand))] + [InlineData(typeof(SetInstanceOverridesCommand))] + [InlineData(typeof(DebugSnapshotCommand))] + [InlineData(typeof(MgmtDeployInstanceCommand))] + [InlineData(typeof(QueryAuditLogCommand))] + public void CommandPayloadTypes_ResolveViaRegistry(Type commandType) + { + // GetCommandName throws ArgumentException for an unregistered type — the CLI + // calls it for every command it sends, so each must round-trip. + var name = ManagementCommandRegistry.GetCommandName(commandType); + Assert.False(string.IsNullOrWhiteSpace(name)); + Assert.Equal(commandType, ManagementCommandRegistry.Resolve(name)); + } +} diff --git a/tests/ScadaLink.CLI.Tests/DebugStreamTests.cs b/tests/ScadaLink.CLI.Tests/DebugStreamTests.cs new file mode 100644 index 0000000..ebd36ee --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/DebugStreamTests.cs @@ -0,0 +1,100 @@ +using System.Threading.Tasks; +using ScadaLink.CLI.Commands; + +namespace ScadaLink.CLI.Tests; + +/// +/// Regression tests for the testable pieces of DebugCommands.StreamDebugAsync: +/// CLI-010 (Ctrl+C during connect misreported as a connection failure) and +/// CLI-012 (non-deterministic exit code after stream termination). +/// +public class DebugStreamTests +{ + // --- CLI-010: connect-failure classification -------------------------------------- + + [Fact] + public void ClassifyConnectFailure_OperationCanceled_IsTreatedAsCancellation() + { + // Ctrl+C while StartAsync is still establishing the connection throws + // OperationCanceledException — this is a graceful cancellation, not a failure. + var result = DebugStreamHelpers.ClassifyConnectFailure( + new OperationCanceledException(), cancellationRequested: true); + + Assert.True(result.IsCancellation); + Assert.Equal(0, result.ExitCode); + } + + [Fact] + public void ClassifyConnectFailure_TaskCanceled_WhenCancelRequested_IsCancellation() + { + var result = DebugStreamHelpers.ClassifyConnectFailure( + new TaskCanceledException(), cancellationRequested: true); + + Assert.True(result.IsCancellation); + Assert.Equal(0, result.ExitCode); + } + + [Fact] + public void ClassifyConnectFailure_RealException_IsConnectionFailure() + { + var result = DebugStreamHelpers.ClassifyConnectFailure( + new HttpRequestException("connection refused"), cancellationRequested: false); + + Assert.False(result.IsCancellation); + Assert.Equal(1, result.ExitCode); + } + + [Fact] + public void ClassifyConnectFailure_CanceledExceptionButNoCancelRequested_IsConnectionFailure() + { + // A cancellation that did not originate from the user (e.g. a server-side abort) + // is still a real connection failure. + var result = DebugStreamHelpers.ClassifyConnectFailure( + new OperationCanceledException(), cancellationRequested: false); + + Assert.False(result.IsCancellation); + Assert.Equal(1, result.ExitCode); + } + + // --- CLI-012: deterministic exit-code resolution ---------------------------------- + + [Fact] + public async Task ResolveStreamExitCodeAsync_TerminationResultSet_PrefersThatResult() + { + // OnStreamTerminated set exitTcs to 1 — that must win even on the Ctrl+C path. + var tcs = new TaskCompletionSource(); + tcs.SetResult(1); + + var code = await DebugStreamHelpers.ResolveStreamExitCodeAsync(tcs.Task); + + Assert.Equal(1, code); + } + + [Fact] + public async Task ResolveStreamExitCodeAsync_NoResult_ReturnsZero() + { + // Pure Ctrl+C: exitTcs never completed — graceful shutdown, exit 0. + var tcs = new TaskCompletionSource(); + + var code = await DebugStreamHelpers.ResolveStreamExitCodeAsync(tcs.Task); + + Assert.Equal(0, code); + } + + [Fact] + public async Task ResolveStreamExitCodeAsync_ResultArrivesDuringGrace_IsObserved() + { + // A stream termination racing with Ctrl+C: the result lands shortly after the + // wait was cancelled. The grace period must let it be observed deterministically. + var tcs = new TaskCompletionSource(); + _ = Task.Run(async () => + { + await Task.Delay(20); + tcs.TrySetResult(1); + }); + + var code = await DebugStreamHelpers.ResolveStreamExitCodeAsync(tcs.Task); + + Assert.Equal(1, code); + } +} diff --git a/tests/ScadaLink.CLI.Tests/ExitCodeTests.cs b/tests/ScadaLink.CLI.Tests/ExitCodeTests.cs new file mode 100644 index 0000000..b3aadef --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/ExitCodeTests.cs @@ -0,0 +1,59 @@ +using ScadaLink.CLI; +using ScadaLink.CLI.Commands; + +namespace ScadaLink.CLI.Tests; + +/// +/// Regression tests for CLI-009 — the design doc defines exit code 2 as "authorization +/// failure". The previous implementation keyed exit 2 solely off HTTP 403, so an +/// authorization failure the server signalled with an error code of +/// UNAUTHORIZED/FORBIDDEN but a different HTTP status was misreported as a +/// generic error (exit 1). Exit code 2 now keys off either signal. +/// +[Collection("Console")] +public class ExitCodeTests +{ + private static int HandleQuietly(ManagementResponse response) + { + var errWriter = new StringWriter(); + Console.SetError(errWriter); + try + { + return CommandHelpers.HandleResponse(response, "json"); + } + finally + { + Console.SetError(new StreamWriter(Console.OpenStandardError()) { AutoFlush = true }); + } + } + + [Fact] + public void HandleResponse_Http403_ReturnsTwo() + { + Assert.Equal(2, HandleQuietly(new ManagementResponse(403, null, "Forbidden", "FORBIDDEN"))); + } + + [Theory] + [InlineData("UNAUTHORIZED")] + [InlineData("FORBIDDEN")] + [InlineData("unauthorized")] + public void HandleResponse_AuthorizationCode_NonForbiddenStatus_ReturnsTwo(string code) + { + // The server signalled an authorization failure via the error code but with a + // non-403 HTTP status; per the documented exit-code table this is still exit 2. + Assert.Equal(2, HandleQuietly(new ManagementResponse(400, null, "Access denied", code))); + } + + [Fact] + public void HandleResponse_GenericError_ReturnsOne() + { + Assert.Equal(1, HandleQuietly(new ManagementResponse(400, null, "Validation failed", "INVALID_ARGUMENT"))); + } + + [Fact] + public void HandleResponse_AuthenticationFailure_ReturnsOne() + { + // Authentication failure (bad credentials) is exit 1, distinct from authorization. + Assert.Equal(1, HandleQuietly(new ManagementResponse(401, null, "Invalid credentials", "AUTH_FAILED"))); + } +} diff --git a/tests/ScadaLink.CLI.Tests/FormatOptionValidationTests.cs b/tests/ScadaLink.CLI.Tests/FormatOptionValidationTests.cs new file mode 100644 index 0000000..4330101 --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/FormatOptionValidationTests.cs @@ -0,0 +1,44 @@ +using System.CommandLine; +using ScadaLink.CLI.Commands; + +namespace ScadaLink.CLI.Tests; + +/// +/// Regression tests for CLI-008 — the --format option previously accepted any +/// string, so a typo like --format tabel silently fell through to JSON output +/// with no feedback. The option must reject values outside {json, table} with a parse +/// error. +/// +public class FormatOptionValidationTests +{ + [Theory] + [InlineData("json")] + [InlineData("table")] + public void FormatOption_AcceptsValidValues(string value) + { + var formatOption = CliOptions.CreateFormatOption(); + var root = new RootCommand(); + root.Add(formatOption); + + var result = root.Parse(new[] { "--format", value }); + + Assert.Empty(result.Errors); + } + + [Theory] + [InlineData("tabel")] + [InlineData("xml")] + [InlineData("yaml")] + [InlineData("")] + [InlineData("JSON")] // case-sensitive: documented values are lowercase + public void FormatOption_RejectsInvalidValues(string value) + { + var formatOption = CliOptions.CreateFormatOption(); + var root = new RootCommand(); + root.Add(formatOption); + + var result = root.Parse(new[] { "--format", value }); + + Assert.NotEmpty(result.Errors); + } +} diff --git a/tests/ScadaLink.CLI.Tests/ManagementHttpClientTests.cs b/tests/ScadaLink.CLI.Tests/ManagementHttpClientTests.cs new file mode 100644 index 0000000..a03ea01 --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/ManagementHttpClientTests.cs @@ -0,0 +1,106 @@ +using System.Net; +using System.Text; +using ScadaLink.CLI; + +namespace ScadaLink.CLI.Tests; + +/// +/// Regression tests for CLI-013 — +/// (success, error-body parsing, connection-failure, and timeout paths) was untested. +/// Uses a stub so no live server is required. +/// +public class ManagementHttpClientTests +{ + private sealed class StubHandler : HttpMessageHandler + { + private readonly Func> _responder; + + public StubHandler(HttpStatusCode status, string body) + : this((_, _) => Task.FromResult(new HttpResponseMessage(status) + { + Content = new StringContent(body, Encoding.UTF8, "application/json"), + })) + { + } + + public StubHandler(Func> responder) + { + _responder = responder; + } + + protected override Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + => _responder(request, cancellationToken); + } + + private static ManagementHttpClient ClientWith(StubHandler handler) + => new(new HttpClient(handler), "http://localhost:9001", "user", "pass"); + + [Fact] + public async Task SendCommandAsync_Success_ReturnsJsonData() + { + using var client = ClientWith(new StubHandler(HttpStatusCode.OK, "{\"id\":1}")); + + var response = await client.SendCommandAsync("ListSites", new { }, TimeSpan.FromSeconds(5)); + + Assert.Equal(200, response.StatusCode); + Assert.Equal("{\"id\":1}", response.JsonData); + Assert.Null(response.Error); + Assert.Null(response.ErrorCode); + } + + [Fact] + public async Task SendCommandAsync_ErrorBody_ParsesErrorAndCode() + { + using var client = ClientWith(new StubHandler( + HttpStatusCode.BadRequest, "{\"error\":\"Bad input\",\"code\":\"INVALID_ARGUMENT\"}")); + + var response = await client.SendCommandAsync("ListSites", new { }, TimeSpan.FromSeconds(5)); + + Assert.Equal(400, response.StatusCode); + Assert.Null(response.JsonData); + Assert.Equal("Bad input", response.Error); + Assert.Equal("INVALID_ARGUMENT", response.ErrorCode); + } + + [Fact] + public async Task SendCommandAsync_NonJsonErrorBody_FallsBackToRawBody() + { + using var client = ClientWith(new StubHandler( + HttpStatusCode.BadGateway, "Bad Gateway")); + + var response = await client.SendCommandAsync("ListSites", new { }, TimeSpan.FromSeconds(5)); + + Assert.Equal(502, response.StatusCode); + Assert.Equal("Bad Gateway", response.Error); + Assert.Null(response.ErrorCode); + } + + [Fact] + public async Task SendCommandAsync_ConnectionFailure_ReturnsStatusZero() + { + using var client = ClientWith(new StubHandler((_, _) => + throw new HttpRequestException("connection refused"))); + + var response = await client.SendCommandAsync("ListSites", new { }, TimeSpan.FromSeconds(5)); + + Assert.Equal(0, response.StatusCode); + Assert.Equal("CONNECTION_FAILED", response.ErrorCode); + Assert.Contains("connection refused", response.Error); + } + + [Fact] + public async Task SendCommandAsync_Timeout_Returns504() + { + using var client = ClientWith(new StubHandler(async (_, ct) => + { + await Task.Delay(Timeout.Infinite, ct); + return new HttpResponseMessage(HttpStatusCode.OK); + })); + + var response = await client.SendCommandAsync("ListSites", new { }, TimeSpan.FromMilliseconds(50)); + + Assert.Equal(504, response.StatusCode); + Assert.Equal("TIMEOUT", response.ErrorCode); + } +}