40 KiB
Code Review — CLI
| Field | Value |
|---|---|
| Module | src/ScadaLink.CLI |
| Design doc | docs/requirements/Component-CLI.md |
| Status | Reviewed |
| Last reviewed | 2026-05-17 |
| Reviewer | claude-agent |
| Commit reviewed | 39d737e |
| 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.
Re-review 2026-05-17 (commit 39d737e)
All 13 prior findings are confirmed resolved — the resilience gaps, the dead format
configuration, the credential-handling weakness, and the test-coverage holes have all
been closed, and the test suite has grown substantially (CommandTreeTests,
ManagementHttpClientTests, DebugStreamTests, etc.). The CLI's runtime behaviour is now
solid. This re-review walked all 14 command groups against the full checklist and found
three new issues, all rooted in update-command and design-document drift rather than
runtime defects: every update command requires the entity's "core" fields (--name,
--script) even though the design doc presents them as optional, so a partial update is
impossible (CLI-014); the design doc's command surface has drifted again in two specific
places — template composition delete and the data-connection config flags (CLI-015);
and WriteAsTable derives table columns from only the first array element, silently
dropping columns for any later element with a different shape (CLI-016). No
Critical/High issues; the module remains healthy.
Checklist coverage
Original review (2026-05-16, 9c60592):
| # | 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). |
Re-review (2026-05-17, 39d737e):
| # | Category | Examined | Notes |
|---|---|---|---|
| 1 | Correctness & logic bugs | ☑ | Update commands require "core" fields, blocking partial updates (CLI-014); WriteAsTable headers derived from first array element only (CLI-016). |
| 2 | Akka.NET conventions | ☑ | Not applicable — pure HTTP/SignalR client. No issues. |
| 3 | Concurrency & thread safety | ☑ | debug stream concurrency now resolved via DebugStreamHelpers (CLI-011/012). No new issues. |
| 4 | Error handling & resilience | ☑ | Malformed-URL / malformed-JSON / connect-cancellation paths all hardened (CLI-004/005/010). No new issues. |
| 5 | Security | ☑ | Env-var credential fallback in place (CLI-006). Basic Auth over HTTP is by design. No new issues. |
| 6 | Performance & resource management | ☑ | CancellationTokenSource now using-scoped (CLI-011). No new issues. |
| 7 | Design-document adherence | ☑ | Two residual command-surface drifts: template composition delete and data-connection --primary-config (CLI-015). |
| 8 | Code organization & conventions | ☑ | Consistent and clean; option construction centralised in CliOptions. No new issues. |
| 9 | Testing coverage | ☑ | Substantially expanded (CommandTreeTests, ManagementHttpClientTests, DebugStreamTests). No new gaps. |
| 10 | Documentation & comments | ☑ | XML docs accurate. Component-CLI.md drift folded into CLI-015. |
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>andsite update <site-id> --file <path>; the implementation has no--fileoption 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/disableas separate commands) and omits commands that do exist (instance alarm-override set/delete/list,external-system methodsubgroup). - The doc's
notification smtp update --filediffers from the implemented--server/--port/--auth-mode/--from-addressflags (NotificationCommands.cs:72-94). - The doc uses
--sitefor site identification in several places where the implementation uses--site-idor--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 streamSignalR 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
Buildproduces 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 aninternaltest-onlyManagementHttpClientconstructor accepting a pre-builtHttpClient, and a stubHttpMessageHandler; 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 throughManagementCommandRegistry.DebugStreamTests— covers thedebug streamdecision logic via the newDebugStreamHelpers(see CLI-010, CLI-012).- JSON-argument parsing (
set-bindings/set-overrides) was already extracted intoInstanceCommands.TryParseBindings/TryParseOverridesand covered byInstanceArgumentParsingTestsunder CLI-005. The CLI test suite went from 42 to 77 passing tests.
CLI-014 — update commands require "core" fields, making partial updates impossible
| Severity | Low |
| Category | Design-document adherence |
| Status | Resolved |
| Location | src/ScadaLink.CLI/Commands/TemplateCommands.cs:77, src/ScadaLink.CLI/Commands/SiteCommands.cs:86, src/ScadaLink.CLI/Commands/ExternalSystemCommands.cs:40-42, src/ScadaLink.CLI/Commands/DataConnectionCommands.cs:39-40, src/ScadaLink.CLI/Commands/NotificationCommands.cs:40-41, src/ScadaLink.CLI/Commands/ApiMethodCommands.cs:79 |
Re-triage 2026-05-17: the finding was originally filed as a Medium "Correctness &
logic bugs" issue, but verification against the Commons message contracts shows the
code is correct and the defect is purely documentation drift. Every Update*Command
record (UpdateTemplateCommand, UpdateSiteCommand, UpdateDataConnectionCommand,
UpdateExternalSystemCommand, UpdateNotificationListCommand, UpdateApiMethodCommand,
UpdateTemplateAttribute/Alarm/ScriptCommand, UpdateRoleMappingCommand,
UpdateSharedScriptCommand, UpdateDatabaseConnectionDefCommand, UpdateAreaCommand)
carries non-nullable "core" fields — they are whole-entity replace records, not
sparse patches. Marking the corresponding --name / --protocol / --script flags
Required = true is therefore the correct CLI behaviour: making them optional would
let an omitted flag send null/empty and silently blank the field server-side. Adopting
the sparse-patch model (recommendation option a) would require changing the Commons
records and the server-side Management handlers — both outside this module's editable
surface. Re-triaged to Low / Design-document adherence; resolved per recommendation
option (b).
Description
The design doc presents update commands with all non---id fields as optional, e.g.
template update --id <id> [--name <name>] [--description <desc>] [--parent-id <id>]
(Component-CLI.md:62) and api-method update --id <id> [--script <code>] ...
(Component-CLI.md:224). The implementation contradicts this: the update commands mark
the entity's "core" fields as Required = true, so the user must always re-supply them:
template update—--nameisRequired = true(TemplateCommands.cs:77).site update—--nameisRequired = true(SiteCommands.cs:86).external-system update—--name,--endpoint-url,--auth-typeare allRequired = true(ExternalSystemCommands.cs:40-42).data-connection update—--name,--protocolareRequired = true(DataConnectionCommands.cs:39-40).notification update—--name,--emailsareRequired = true(NotificationCommands.cs:40-41).api-method update—--scriptisRequired = true(ApiMethodCommands.cs:79).- The same pattern applies to
template attribute/alarm/script update(TemplateCommands.cs:164-165,246-248,332-334) androle-mapping update(SecurityCommands.cs:110-111).
Because the corresponding Update*Command records are whole-replace (they carry the full
field set, not a sparse patch), a user who wants to change only one field — e.g. flip an
API method's timeout, or rename a template — must look up and re-pass every other field's
current value. Omitting any required flag is a hard parse error. This makes scripted,
single-field updates (a core CLI/CI use case) awkward and error-prone, and it does not
match the documented optional-flag surface.
Recommendation
Decide on one model and align doc + code. Either (a) make the update flags genuinely
optional and have the server/Update*Command treat a null field as "leave unchanged"
(sparse patch), or (b) if whole-replace is intentional, update Component-CLI.md to show
these flags as required (no [...]) and document that an update replaces the whole
entity. Option (a) matches the documented surface and the typical CLI expectation.
Resolution
Resolved 2026-05-17 (commit pending) per recommendation option (b). Verification of the
Commons Update*Command records confirmed whole-replace is the intentional contract, so
the CLI's Required = true flags are correct and were left unchanged. The in-repo
src/ScadaLink.CLI/README.md — which is authoritative and previously listed every
update core field as optional [--name] — was corrected: the core flags
(--name/--protocol/--script/--code/--emails/--endpoint-url/--auth-type/
--data-type/--trigger-type/--priority/--connection-string/--ldap-group/--role)
are now marked required: yes, the command synopses drop the [...], and each update
section states that an update replaces the whole entity. Added
UpdateCommandContractTests (10 tests) pinning the whole-replace contract — every
listed update command's core flags must be Required, and the genuinely-sparse
external-system method update must remain optional. Note: docs/requirements/Component-CLI.md
also still shows these flags as optional, but it is outside this module's editable
surface — that doc-side correction is owned by the docs surface.
CLI-015 — Component-CLI.md command surface has drifted again in two places
| Severity | Low |
| Category | Design-document adherence |
| Status | Resolved |
| Location | docs/requirements/Component-CLI.md:75, docs/requirements/Component-CLI.md:125-126, src/ScadaLink.CLI/README.md (vs. src/ScadaLink.CLI/Commands/TemplateCommands.cs:404-413, src/ScadaLink.CLI/Commands/DataConnectionCommands.cs:41, :86) |
Re-triage 2026-05-17: verification found the same two drifts also present in the
in-repo src/ScadaLink.CLI/README.md (the authoritative reference): its
template composition delete section used the non-existent --template-id /
--instance-name form, and data-connection create/update documented only
--configuration without the canonical --primary-config flag (--configuration is in
fact an alias of --primary-config). The README is in this module's editable surface;
docs/requirements/Component-CLI.md is not.
Description
CLI-007 regenerated the doc's "Command Structure" section, but two specific drifts remain or were introduced:
Component-CLI.md:75documentstemplate composition delete --template-id <id> --instance-name <name>, but the implementation (TemplateCommands.cs:404-413) deletes a composition by its own integer ID via a single--idoption (DeleteTemplateCompositionCommand(id)). The doc's two-flag form does not exist.data-connection createandupdateaccept a--primary-configoption (aliased--configuration) for the primary configuration JSON (DataConnectionCommands.cs:86,:41), butComponent-CLI.md:125-126lists only--backup-configand--failover-retry-count— the primary-config flag is absent from the doc.
A reader following the doc would use a non-existent template composition delete form and
would not discover the --primary-config flag.
Recommendation
Correct Component-CLI.md:75 to template composition delete --id <id>, and add
[--primary-config <json>] to the documented data-connection create/update signatures
(Component-CLI.md:125-126). Also note the --configuration alias if aliases are
documented elsewhere.
Resolution
Resolved 2026-05-17 (commit pending). Both drifts were present in the in-repo
src/ScadaLink.CLI/README.md and were corrected there (the README is this module's
authoritative reference): template composition delete now documents the real single
--id <int> form, and data-connection create/update now document --primary-config
(with the --configuration alias noted) alongside --backup-config and
--failover-retry-count. Added CommandTreeTests.TemplateCompositionDelete_IsKeyedByIdOnly
pinning the composition-delete surface (--id present; --template-id/--instance-name
absent). The corresponding edit to docs/requirements/Component-CLI.md:75,125-126 is
outside this module's editable surface and remains for the docs surface to apply.
CLI-016 — WriteAsTable derives columns from the first array element only
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | src/ScadaLink.CLI/Commands/CommandHelpers.cs:184-200 |
Description
When rendering a JSON array as a table, WriteAsTable builds the header set from
items[0].EnumerateObject() only (CommandHelpers.cs:184-186) and then projects every
row against that fixed header list (:188-198). If a later element of the array has a
different shape — additional properties, or properties the first element lacks — those
extra columns are silently dropped from the table and a row missing a header property
renders an empty cell. The user sees a table that appears complete but has omitted data,
with no indication that columns were discarded. (The JSON output path is unaffected; this
only affects --format table.) Management API list responses are generally homogeneous,
so the practical impact is low, but a heterogeneous array — e.g. a diff or a mixed-status
list — would be rendered incorrectly with no warning.
Recommendation
Compute the header set as the union of property names across all array elements (iterate all items, collect distinct property names preserving first-seen order) before projecting rows, so no element's data is silently dropped.
Resolution
Resolved 2026-05-17 (commit pending). Root cause confirmed — WriteAsTable derived the
header set from items[0].EnumerateObject() only, dropping any property unique to a
later element. The header set is now computed as the union of property names across
every object element of the array, in first-seen order (a HashSet guards duplicates
while an ordered list preserves column order). Rows already project against the header
list and OutputFormatter.WriteTable pads missing cells, so heterogeneous arrays now
render every column. Regression tests added in TableHeaderUnionTests (3 tests:
later-element-only column included, first-seen column order preserved,
first-element-extra column still rendered).