dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(cli): resolve CLI-008..013 — format validation, exit-code semantics, debug-stream cancellation/disposal, test coverage

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-16 22:04:21 -04:00
parent bc88a36435
commit 404216b4ee
12 changed files with 593 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

75
code-reviews/CLI/findings.md
View File

@@ -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.