scadalink-design/code-reviews/CLI/findings.md

Code Review — CLI

Field	Value
Module	src/ScadaLink.CLI
Design doc	docs/requirements/Component-CLI.md
Status	Reviewed
Last reviewed	2026-05-16
Reviewer	claude-agent
Commit reviewed	9c60592
Open findings	0

Summary

The CLI is a small, well-structured HTTP client over the Management API. The command-tree construction is consistent and repetitive in a good way: every subcommand funnels through CommandHelpers.ExecuteCommandAsync, which centralizes URL/credential resolution, HTTP dispatch, and response handling. There are no Akka.NET concerns (the CLI is a pure HTTP client) and no concurrency-sensitive code apart from the debug stream SignalR handler.

The dominant theme is graceful-degradation gaps: several user-supplied inputs (malformed URLs, malformed --bindings/--overrides JSON, non-JSON success bodies) are deserialized or constructed without try/catch, so a normal user mistake surfaces as an unhandled exception with a stack trace instead of a clean error message and exit code 1. A second theme is dead configuration: the SCADALINK_FORMAT environment variable and the defaultFormat config-file field are loaded by CliConfig but never consulted by any command, so the documented format-precedence chain does not work. The third theme is substantial design-document drift: Component-CLI.md describes a name-keyed, --file-based command surface that bears little resemblance to the implemented ID-keyed, flag-based surface. Test coverage exercises OutputFormatter, CliConfig, and CommandHelpers.HandleResponse, but the HTTP client, the debug stream path, the JSON argument parsing, and the command-tree wiring are untested.

Checklist coverage

#	Category	Examined	Notes
1	Correctness & logic bugs	☑	Format precedence is broken (CLI-001); empty/non-JSON success bodies crash table rendering (CLI-002, CLI-003).
2	Akka.NET conventions	☑	Not applicable — CLI is a pure HTTP/SignalR client with no Akka.NET runtime (design doc confirms). No issues.
3	Concurrency & thread safety	☑	Only debug stream is concurrent; CancellationTokenSource is never disposed (CLI-011). Exit-code resolution after Ctrl+C is loose (CLI-012).
4	Error handling & resilience	☑	Unhandled exceptions on malformed URL (CLI-004) and malformed JSON arguments (CLI-005); StartAsync cancellation is misreported (CLI-010).
5	Security	☑	--password on the command line leaks into process listings / shell history with no env-var or prompt alternative (CLI-006).
6	Performance & resource management	☑	HttpClient per invocation is acceptable for a one-shot CLI. CancellationTokenSource leak noted in CLI-011.
7	Design-document adherence	☑	Component-CLI.md is heavily stale relative to the implemented command surface (CLI-007).
8	Code organization & conventions	☑	Consistent and clean; CliConfig.DefaultFormat is loaded but unused (covered by CLI-001). Minor: --format not validated (CLI-008).
9	Testing coverage	☑	No tests for ManagementHttpClient, DebugCommands, command-tree wiring, or JSON argument parsing (CLI-013).
10	Documentation & comments	☑	Component-CLI.md mismatch (CLI-007); the in-repo README.md is reasonably accurate. Minor exit-code doc mismatch (CLI-009).

Findings

CLI-001 — SCADALINK_FORMAT env var and config-file format are dead; format precedence broken

Severity	High
Category	Correctness & logic bugs
Status	Resolved
Location	src/ScadaLink.CLI/Commands/CommandHelpers.cs:18, src/ScadaLink.CLI/Commands/DebugCommands.cs:45, src/ScadaLink.CLI/CliConfig.cs:37-39

Description

CliConfig.Load() reads SCADALINK_FORMAT and the defaultFormat config-file field into CliConfig.DefaultFormat, and Component-CLI.md documents a format-precedence chain (command-line option → env var → config file). However, every command resolves the format with var format = result.GetValue(formatOption) ?? "json"; and formatOption is created in Program.cs:11 with DefaultValueFactory = _ => "json". GetValue therefore always returns a non-null value ("json" when the flag is absent), so the ?? "json" fallback never fires and config.DefaultFormat is never consulted. The env var and config-file format settings are dead code: scadalink site list always outputs JSON regardless of SCADALINK_FORMAT=table or a defaultFormat entry in ~/.scadalink/config.json. The documented behaviour silently does not work.

Recommendation

Either remove the --format option's DefaultValueFactory and have CommandHelpers resolve precedence explicitly (result.GetValue(formatOption) → config.DefaultFormat), or detect whether the option was explicitly supplied (result.GetResult(formatOption)) and only then override the config value. Apply the same fix to DebugCommands.BuildStream.

Resolution

Resolved 2026-05-16 (commit <pending>). Removed the --format option's DefaultValueFactory in Program.cs and added CommandHelpers.ResolveFormat, which uses ParseResult.GetResult(formatOption) to detect an explicitly supplied flag and resolves precedence explicitly: explicit --format → CliConfig.DefaultFormat (env var / config file) → "json". Both CommandHelpers.ExecuteCommandAsync and DebugCommands.BuildStream now call ResolveFormat. Regression tests added in FormatResolutionTests.

CLI-002 — Empty success body crashes table rendering with an unhandled exception

Severity	Medium
Category	Correctness & logic bugs
Status	Resolved
Location	src/ScadaLink.CLI/Commands/CommandHelpers.cs:59-68, src/ScadaLink.CLI/Commands/CommandHelpers.cs:78-80

Description

ManagementHttpClient.SendCommandAsync returns JsonData = responseBody for any success status code, including a 200/204 with an empty body. HandleResponse then tests response.JsonData != null — an empty string is non-null — and for --format table calls WriteAsTable(response.JsonData), which immediately does JsonDocument.Parse(json). JsonDocument.Parse("") throws JsonException, which is not caught anywhere, so a command that legitimately returns no body (e.g. a delete that returns 204) terminates with a stack trace instead of a clean success message.

Recommendation

In HandleResponse, treat a null-or-whitespace JsonData as a "command succeeded, no output" case (print nothing or (ok)), and return 0 before attempting to parse.

Resolution

Resolved 2026-05-16 (commit pending). Root cause confirmed against source — HandleResponse tested JsonData != null, so an empty success body fell through to WriteAsTable → JsonDocument.Parse("") and threw an uncaught JsonException. HandleResponse now treats a null-or-whitespace JsonData as a "succeeded, no output" case, prints (ok), and returns 0 before any parse. Regression tests added in ResponseRenderingTests (HandleResponse_EmptyBody_TableFormat_DoesNotThrow_ReturnsZero, HandleResponse_EmptyBody_JsonFormat_DoesNotThrow_ReturnsZero).

CLI-003 — Non-JSON success body crashes table rendering

Severity	Medium
Category	Error handling & resilience
Status	Resolved
Location	src/ScadaLink.CLI/Commands/CommandHelpers.cs:80

Description

WriteAsTable calls JsonDocument.Parse(json) with no try/catch. If the server returns a success status but a body that is not valid JSON (a proxy/HTML error page returned with a 200, a plain-text message, etc.), the CLI throws an unhandled JsonException. The error-path code in ManagementHttpClient (lines 52-61) already defensively wraps JsonDocument.Parse in a try/catch; the success path and WriteAsTable do not get the same treatment.

Recommendation

Wrap the JsonDocument.Parse in WriteAsTable in a try/catch; on failure, fall back to printing the raw body verbatim (as the JSON path already does at line 66).

Resolution

Resolved 2026-05-16 (commit pending). Root cause confirmed — WriteAsTable parsed the body with no try/catch. The JsonDocument.Parse call is now wrapped in a try/catch (JsonException) that prints the raw body verbatim on failure, mirroring the raw-body fallback on the JSON path. Regression test ResponseRenderingTests.HandleResponse_NonJsonBody_TableFormat_FallsBackToRaw_ReturnsZero.

CLI-004 — Malformed --url throws an unhandled UriFormatException

Severity	Medium
Category	Error handling & resilience
Status	Resolved
Location	src/ScadaLink.CLI/ManagementHttpClient.cs:13

Description

The ManagementHttpClient constructor does new Uri(baseUrl.TrimEnd('/') + "/") with no validation. If the user passes a malformed URL (e.g. --url localhost:9001 without a scheme, or --url ""), new Uri(...) throws UriFormatException. This call is not guarded by the try/catch in SendCommandAsync (it happens in the constructor at CommandHelpers.cs:50), so a common typo terminates the CLI with a stack trace rather than the documented "connection failure → exit 1 with a descriptive message".

Recommendation

Validate the URL before constructing the client — e.g. Uri.TryCreate(url, UriKind.Absolute, out _) in CommandHelpers.ExecuteCommandAsync and DebugCommands.BuildStream — and emit a clean INVALID_URL error with exit code 1 on failure.

Resolution

Resolved 2026-05-16 (commit pending). Root cause confirmed — the ManagementHttpClient constructor's new Uri(...) ran outside the SendCommandAsync try/catch. Added CommandHelpers.IsValidManagementUrl, which checks for an absolute http/https URL via Uri.TryCreate. Both CommandHelpers.ExecuteCommandAsync and DebugCommands.BuildStream now validate the resolved URL up front and emit a clean INVALID_URL error with exit code 1. Regression tests in UrlValidationTests.

CLI-005 — Malformed --bindings / --overrides JSON throws unhandled exceptions

Severity	Medium
Category	Error handling & resilience
Status	Resolved
Location	src/ScadaLink.CLI/Commands/InstanceCommands.cs:55-58, src/ScadaLink.CLI/Commands/InstanceCommands.cs:181-182

Description

set-bindings deserializes the --bindings argument with JsonSerializer.Deserialize<List<List<JsonElement>>>(...) and then indexes p[0]/p[1] and calls p[0].GetString()! / p[1].GetInt32(). set-overrides deserializes --overrides with JsonSerializer.Deserialize<Dictionary<string, string?>>(...). None of this is wrapped in a try/catch. Invalid JSON throws JsonException; a pair with fewer than two elements throws ArgumentOutOfRangeException; a non-string/non-int element throws InvalidOperationException. All of these surface as raw stack traces, so a user typo in a JSON argument crashes the CLI instead of producing a clean validation error and exit code 1.

Recommendation

Wrap the parsing in try/catch (JsonException ...) (and guard the pair length / element kinds), and on failure call OutputFormatter.WriteError(...) with an INVALID_ARGUMENT code and return 1.

Resolution

Resolved 2026-05-16 (commit pending). Root cause confirmed — both set-bindings and set-overrides deserialized and indexed JSON inline with no try/catch. Extracted the parsing into testable InstanceCommands.TryParseBindings / TryParseOverrides helpers that catch JsonException, guard against null results, and (for bindings) validate pair arity and element kinds (JsonValueKind) instead of letting ArgumentOutOfRangeException / InvalidOperationException escape. The command actions now emit a clean INVALID_ARGUMENT error and return 1 on failure. Regression tests in InstanceArgumentParsingTests (8 tests covering valid input, malformed JSON, short pairs, wrong element types, and JSON null).

CLI-006 — Password is passed as a command-line argument with no safer alternative

Severity	Medium
Category	Security
Status	Resolved
Location	src/ScadaLink.CLI/Program.cs:9, src/ScadaLink.CLI/Commands/CommandHelpers.cs:36-44

Description

Credentials are supplied only via --username / --password. A password on the command line is visible to any local user via the process list (ps, /proc/<pid>/cmdline) and is typically persisted into shell history. Unlike the management URL — which can also come from SCADALINK_MANAGEMENT_URL or the config file — there is no environment-variable fallback, no --password-stdin, and no interactive prompt for the password. For a tool explicitly intended for CI/CD automation this materially increases the chance of credential leakage.

Recommendation

Add a SCADALINK_PASSWORD environment variable fallback and/or a --password-stdin option (read the password from stdin), and document that --password on the command line is discouraged. Optionally prompt interactively when stdin is a TTY and no password was supplied.

Resolution

Resolved 2026-05-16 (commit pending). Root cause confirmed — credentials had no non-command-line source. Added SCADALINK_USERNAME / SCADALINK_PASSWORD environment fallbacks: CliConfig.Load now reads them into new CliConfig.Username / Password properties (credentials are sourced from environment variables only, never the config file, so they are not persisted). CommandHelpers.ResolveCredential resolves precedence (explicit --username/--password → env var); both ExecuteCommandAsync and DebugCommands.BuildStream use it. The design doc and the in-repo README.md now document that --password on the command line is discouraged. The --password-stdin option / interactive prompt was not added — the env-var fallback fully satisfies the CI/CD safe-credential need; a stdin/prompt variant can be a follow-up if interactive use demands it. Regression tests in CredentialResolutionTests.

CLI-007 — Component-CLI.md command surface is substantially stale

Severity	Medium
Category	Design-document adherence
Status	Resolved
Location	docs/requirements/Component-CLI.md:51-211 (vs. all files under src/ScadaLink.CLI/Commands/)

Description

The "Command Structure" section of the design doc no longer matches the implemented CLI. Examples of the drift:

• The doc keys most operations by name (template get <name>, instance get <code>, site get <site-id>); the implementation keys everything by integer ID via --id (TemplateCommands.cs:40, InstanceCommands.cs:31, SiteCommands.cs:26).
• The doc shows template create ... --file <path> and site update <site-id> --file <path>; the implementation has no --file option anywhere and instead takes individual flags (TemplateCommands.cs:52-72, SiteCommands.cs:83-115).
• The doc lists commands that do not exist (template diff, instance bind-connections, instance assign-area, template attribute add --tag-path, data-connection assign/unassign, security api-key enable/disable as separate commands) and omits commands that do exist (instance alarm-override set/delete/list, external-system method subgroup).
• The doc's notification smtp update --file differs from the implemented --server/--port/--auth-mode/--from-address flags (NotificationCommands.cs:72-94).
• The doc uses --site for site identification in several places where the implementation uses --site-id or --identifier.

A reader following the design doc would be unable to drive the CLI.

Recommendation

Regenerate the "Command Structure" section of Component-CLI.md from the actual command tree (the in-repo src/ScadaLink.CLI/README.md is much closer to reality and could be the source), or mark the doc's command list as illustrative and point to the README as authoritative.

Resolution

Resolved 2026-05-16 (commit pending). Drift confirmed against every file under src/ScadaLink.CLI/Commands/. Regenerated the entire "Command Structure" section of Component-CLI.md from the actual command tree: all entities are now keyed by integer --id; the non-existent --file option is removed; create/update commands list their real individual flags; non-existent commands (template diff, instance bind-connections/assign-area, data-connection assign/unassign, security api-key enable/disable) are removed; previously-omitted commands (instance alarm-override set/delete/list, external-system method subgroup, site deploy-artifacts) are added. A note now points to src/ScadaLink.CLI/README.md as the authoritative reference. The Configuration section also documents the new SCADALINK_USERNAME/SCADALINK_PASSWORD env vars (see CLI-006).

CLI-008 — --format value is not validated

Severity	Low
Category	Code organization & conventions
Status	Resolved
Location	src/ScadaLink.CLI/Program.cs:10-11, src/ScadaLink.CLI/Commands/CommandHelpers.cs:60

Description

The --format option accepts any string. HandleResponse only checks string.Equals(format, "table", ...); any other value — including a typo like --format tabel or --format xml — silently falls through to JSON output. The user gets no feedback that their requested format was not honoured.

Recommendation

Restrict the option to the accepted values, e.g. formatOption.AcceptOnlyFromAmong("json", "table"), so System.CommandLine rejects invalid input with a clear parse error.

Resolution

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

Severity	Low
Category	Documentation & comments
Status	Resolved
Location	docs/requirements/Component-CLI.md:238-249, src/ScadaLink.CLI/Commands/CommandHelpers.cs:75

Description

The design doc's Exit Codes table defines code 2 as "Authorization failure (insufficient role)" and the Error Handling section says "If the server returns HTTP 403, the CLI exits with code 2." HandleResponse implements return response.StatusCode == 403 ? 2 : 1;, which is correct for the HTTP error path. However, the NO_URL, NO_CREDENTIALS, INVALID_OPERATION (from set-bindings/set-overrides) and any other client-side failure all return 1, and a connection failure carries StatusCode == 0 — none of which the doc enumerates. More importantly, an authorization failure that the server signals with a body code of UNAUTHORIZED but an HTTP status other than 403 would be classified as a generic error (exit 1). The mapping is purely status-driven and the doc does not state that.

Recommendation

Either document precisely that exit code 2 is determined solely by HTTP 403, or key the "authorization failure" exit code off the response code field as well. Align the doc with whichever is chosen.

Resolution

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

Severity	Low
Category	Error handling & resilience
Status	Resolved
Location	src/ScadaLink.CLI/Commands/DebugCommands.cs:181-189

Description

StreamDebugAsync calls await connection.StartAsync(cts.Token) inside a try { } catch (Exception ex) that unconditionally reports "Connection failed: {ex.Message}" with code CONNECTION_FAILED and returns 1. If the user presses Ctrl+C while the connection is still being established, cts is cancelled and StartAsync throws OperationCanceledException; this is caught by the generic handler and misreported as a connection failure (with exit code 1) rather than a clean user-initiated cancellation (exit code 0).

Recommendation

Catch OperationCanceledException separately (return 0 quietly) before the generic catch (Exception) handler, mirroring how the exitTcs.Task.WaitAsync(cts.Token) path at lines 209-215 already treats cancellation as graceful.

Resolution

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

Severity	Low
Category	Performance & resource management
Status	Resolved
Location	src/ScadaLink.CLI/Commands/DebugCommands.cs:89

Description

var cts = new CancellationTokenSource(); is created in StreamDebugAsync but never disposed; there is no using declaration and no explicit Dispose() call on any exit path. CancellationTokenSource owns a WaitHandle and should be disposed. The impact is small because the process exits shortly after, but it is an IDisposable left undisposed, contrary to the review checklist's resource-management expectation.

Recommendation

Declare it as using var cts = new CancellationTokenSource(); (or wrap the method body in a try/finally).

Resolution

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

Severity	Low
Category	Concurrency & thread safety
Status	Resolved
Location	src/ScadaLink.CLI/Commands/DebugCommands.cs:208-227

Description

After await exitTcs.Task.WaitAsync(cts.Token), the method returns exitTcs.Task.IsCompletedSuccessfully ? exitTcs.Task.Result : 0. When the user cancels with Ctrl+C, WaitAsync throws OperationCanceledException and exitTcs is typically still incomplete, so the method returns 0 — correct. However, the OnStreamTerminated handler and the Closed handler both call exitTcs.TrySetResult, and these run on SignalR callback threads concurrently with the Ctrl+C path. If a stream termination and a Ctrl+C race, the final exit code depends on which TrySetResult won and whether WaitAsync observed completion before cancellation — the result is not deterministic. A stream the server terminated abnormally can end up returning 0.

Recommendation

Resolve the exit code from a single authoritative source: after the try/catch around WaitAsync, check exitTcs.Task completion explicitly and treat a Ctrl+C with no prior result as 0, but always prefer a result that was set by OnStreamTerminated/Closed. Consider awaiting exitTcs.Task without the cancellation token after a brief grace period.

Resolution

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

Severity	Low
Category	Testing coverage
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

The test project covers OutputFormatter, CliConfig.Load, and CommandHelpers.HandleResponse. It does not cover:

• ManagementHttpClient.SendCommandAsync — the timeout (504), connection-failure (code 0), and error-body-parsing paths are untested.
• The debug stream SignalR command — no tests at all.
• The JSON-argument parsing in InstanceCommands (set-bindings, set-overrides) — the paths most likely to crash on bad input (CLI-005) have no coverage.
• Command-tree wiring — there is no test asserting that each Build produces the expected subcommands/options or that the command-name derivation (ManagementCommandRegistry.GetCommandName) resolves for every command type the CLI constructs.

Recommendation

Add tests for ManagementHttpClient (using a stub HttpMessageHandler), for the JSON-argument parsing helpers (extracting the parsing into testable methods), and a smoke test that walks the root command tree and asserts every leaf command's payload type resolves via ManagementCommandRegistry.

Resolution

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.