# 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>` 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.

### 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` — `--name` is `Required = true` (`TemplateCommands.cs:77`).
- `site update` — `--name` is `Required = true` (`SiteCommands.cs:86`).
- `external-system update` — `--name`, `--endpoint-url`, `--auth-type` are all
  `Required = true` (`ExternalSystemCommands.cs:40-42`).
- `data-connection update` — `--name`, `--protocol` are `Required = true`
  (`DataConnectionCommands.cs:39-40`).
- `notification update` — `--name`, `--emails` are `Required = true`
  (`NotificationCommands.cs:40-41`).
- `api-method update` — `--script` is `Required = true` (`ApiMethodCommands.cs:79`).
- The same pattern applies to `template attribute/alarm/script update`
  (`TemplateCommands.cs:164-165`, `246-248`, `332-334`) and `role-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:75` documents `template 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 `--id` option
  (`DeleteTemplateCompositionCommand(id)`). The doc's two-flag form does not exist.
- `data-connection create` and `update` accept a `--primary-config` option (aliased
  `--configuration`) for the primary configuration JSON (`DataConnectionCommands.cs:86`,
  `:41`), but `Component-CLI.md:125-126` lists only `--backup-config` and
  `--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).