ScadaBridge/code-reviews/CLI/findings.md

Code Review — CLI

Field	Value
Module	src/ZB.MOM.WW.ScadaBridge.CLI
Design doc	docs/requirements/Component-CLI.md
Status	Reviewed
Last reviewed	2026-05-28
Reviewer	claude-agent
Commit reviewed	1eb6e97
Open findings	1

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

Re-review 2026-05-28 (commit 1eb6e97)

The CLI has grown two substantial new command groups since the last re-review — scadabridge audit (Audit Log #23 M8) and scadabridge bundle (Transport #24) — together adding ~1,500 lines of new production code. The new audit surface is well-tested and well-factored (pure helpers + a clear IAuditFormatter seam), but the new bundle surface is untested, duplicates the URL/credential resolution that already exists in CommandHelpers, and inherits a partial authorization-exit-code regression that also appears in the audit path. Two longstanding fragility gaps that the prior reviews missed also surface in this pass: CliConfig.Load parses the config file with no try/catch, and CommandTreeTests still pins the old 14-group count so the two new groups are excluded from the leaf-action and registry-resolution coverage that protected the rest of the tree. Module health is broadly good but the consolidated count is now seven Open findings (none Critical, three Medium).

• CLI-017 — BundleCommands duplicates ExecuteCommandAsync and skips the FORBIDDEN/UNAUTHORIZED exit-code mapping (auth exit 2 contract regression).
• CLI-018 — AuditQueryHelpers.RunQueryAsync / AuditExportHelpers.RunExportAsync return exit 1 on every error, never the documented exit 2 for authorization failure.
• CLI-019 — BundleCommands.bundle export decodes the entire base64 bundle in memory and writes synchronously — 100 MB bundles double-buffer.
• CLI-020 — BundleCommands.bundle export parses the success body with bare JsonDocument.Parse + GetProperty and throws on a malformed/abbreviated envelope.
• CLI-021 — CliConfig.Load crashes the whole CLI when ~/.scadabridge/config.json is malformed or unreadable, even if --url was supplied on the command line.
• CLI-022 — AuditCommands and BundleCommands are absent from CommandTreeTests; the test still pins Equal(14, groups.Count) and silently excludes the new groups.
• CLI-023 — Component-CLI.md says the audit commands ride POST /management, but the implementation calls a new GET /api/audit/* REST endpoint pair.

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.

Re-review (2026-05-28, 1eb6e97):

#	Category	Examined	Notes
1	Correctness & logic bugs	☑	BundleCommands.BuildExport unguarded JsonDocument.Parse + GetProperty (CLI-020); CliConfig.Load unguarded JSON parse (CLI-021).
2	Akka.NET conventions	☑	Not applicable — pure HTTP/SignalR/REST client. No issues.
3	Concurrency & thread safety	☑	No new concurrency surface; debug stream unchanged since CLI-011/012. No issues.
4	Error handling & resilience	☑	Bundle and audit paths skip the auth exit-code contract (CLI-017, CLI-018); bundle JSON-envelope parse is brittle (CLI-020); config-file parse aborts the process (CLI-021).
5	Security	☑	No new credential or trust-boundary issues. No issues.
6	Performance & resource management	☑	bundle export double-buffers the whole bundle in memory (CLI-019).
7	Design-document adherence	☑	Component-CLI.md claims audit commands ride POST /management; implementation uses new REST endpoints (CLI-023).
8	Code organization & conventions	☑	BundleCommands.RunBundleCommandAsync re-implements credential/URL resolution that CommandHelpers.ExecuteCommandAsync already provides — drift waiting to happen (CLI-017).
9	Testing coverage	☑	BundleCommands has no tests; CommandTreeTests pins Equal(14, …) and excludes the new AuditCommands + BundleCommands groups (CLI-022).
10	Documentation & comments	☑	XML docs accurate; doc-vs-code transport drift folded into CLI-023. No other issues.

Findings

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

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

Description

CliConfig.Load() reads SCADABRIDGE_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: scadabridge site list always outputs JSON regardless of SCADABRIDGE_FORMAT=table or a defaultFormat entry in ~/.scadabridge/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/ZB.MOM.WW.ScadaBridge.CLI/Commands/CommandHelpers.cs:59-68, src/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.CLI/Commands/InstanceCommands.cs:55-58, src/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.CLI/Program.cs:9, src/ZB.MOM.WW.ScadaBridge.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 SCADABRIDGE_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 SCADABRIDGE_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 SCADABRIDGE_USERNAME / SCADABRIDGE_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/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.CLI/README.md as the authoritative reference. The Configuration section also documents the new SCADABRIDGE_USERNAME/SCADABRIDGE_PASSWORD env vars (see CLI-006).

CLI-008 — --format value is not validated

Severity	Low
Category	Code organization & conventions
Status	Resolved
Location	src/ZB.MOM.WW.ScadaBridge.CLI/Program.cs:10-11, src/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.CLI.Tests/ (vs. src/ZB.MOM.WW.ScadaBridge.CLI/ManagementHttpClient.cs, src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DebugCommands.cs, src/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs:77, src/ZB.MOM.WW.ScadaBridge.CLI/Commands/SiteCommands.cs:86, src/ZB.MOM.WW.ScadaBridge.CLI/Commands/ExternalSystemCommands.cs:40-42, src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DataConnectionCommands.cs:39-40, src/ZB.MOM.WW.ScadaBridge.CLI/Commands/NotificationCommands.cs:40-41, src/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.CLI/README.md (vs. src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs:404-413, src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DataConnectionCommands.cs:41, :86)

Re-triage 2026-05-17: verification found the same two drifts also present in the in-repo src/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.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/ZB.MOM.WW.ScadaBridge.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).

CLI-017 — BundleCommands.RunBundleCommandAsync duplicates ExecuteCommandAsync and breaks the auth exit-code contract

Severity	Medium
Category	Error handling & resilience
Status	Resolved
Location	src/ZB.MOM.WW.ScadaBridge.CLI/Commands/BundleCommands.cs:244-289 (vs. src/ZB.MOM.WW.ScadaBridge.CLI/Commands/CommandHelpers.cs:20-73, :159-174)

Resolution (2026-05-28): Extended CommandHelpers.ExecuteCommandAsync with optional timeout and onSuccess parameters so a caller can supply a longer per-command timeout (BundleCommandTimeout) and capture the success body for file I/O. The duplicated RunBundleCommandAsync was deleted; all three bundle sub-commands now delegate through ExecuteCommandAsync, which routes the error path through IsAuthorizationFailure — exit 2 fires on HTTP 403 OR a FORBIDDEN/UNAUTHORIZED error code regardless of status, unifying the contract with every other command group.

Description

BundleCommands.RunBundleCommandAsync re-implements the URL/credential resolution, validation, and HTTP plumbing that CommandHelpers.ExecuteCommandAsync already provides for every other command group — to attach a 5-minute timeout (BundleCommandTimeout) and a caller-supplied success handler. In duplicating it, two contracts that CommandHelpers carefully establishes were dropped:

1. Authorization exit code. CommandHelpers.HandleResponse routes through IsAuthorizationFailure, which returns exit 2 for either HTTP 403 or an UNAUTHORIZED/FORBIDDEN error code on any status (resolution of CLI-009). The bundle path at line 287 uses a bare if (response.StatusCode == 403) return 2; — a server that signals authorization failure via the code field on a non-403 status (the same channel the rest of the CLI honours) will exit 1 instead of 2 from bundle export/preview/import. Component-Transport.md:289 explicitly states "Exit codes follow the project convention: 0 = success, 1 = command failure, 2 = authorization failure," so this is a contract regression.
2. Error-message phrasing drift. The two duplicated error paths (bundle:258-260, :264-266) emit shorter messages that omit the SCADABRIDGE_MANAGEMENT_URL / SCADABRIDGE_USERNAME env-var hints the canonical paths give — confusing if the user is trying to debug what's missing.

Recommendation

Refactor CommandHelpers.ExecuteCommandAsync to accept an optional TimeSpan timeout and an optional success handler, and have BundleCommands call it. Failing that, extract CommandHelpers.IsAuthorizationFailure to internal and call it from RunBundleCommandAsync in place of the bare 403 check, and copy the canonical error messages verbatim.

CLI-018 — audit query and audit export never return exit 2 for an authorization failure

Severity	Medium
Category	Error handling & resilience
Status	Resolved
Location	src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditQueryHelpers.cs:186-193, src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditExportHelpers.cs:147-153

Description

The two audit-log subcommands (audit query, audit export) ride a new REST surface (GET /api/audit/query and GET /api/audit/export) — not the POST /management envelope that goes through CommandHelpers.HandleResponse. Both helpers map any non-success response to a generic OutputFormatter.WriteError(...) + return 1:

• AuditQueryHelpers.RunQueryAsync:186-193 returns 1 unconditionally when JsonData is null (i.e. any error). It never inspects StatusCode or ErrorCode.
• AuditExportHelpers.RunExportAsync:147-153 returns 1 for every non-success status, again with no 403 / FORBIDDEN carve-out.

Component-CLI.md:295-296 documents exit code 2 for "Authorization failure (insufficient role)". Component-AuditLog.md (Security & Tamper-Evidence) and Component-CLI.md:184-187 both call out that the audit endpoints are gated by OperationalAudit and AuditExport permissions enforced server-side — i.e. these are exactly the commands most likely to return 403 in routine use. The exit-code regression silently downgrades a 403 to a generic command failure, breaking the CI/CD scripting contract.

Recommendation

Promote CommandHelpers.IsAuthorizationFailure to internal (or move it to a small shared helper) and have RunQueryAsync / RunExportAsync return 2 when it matches. The check needs to use the ManagementResponse.StatusCode / ErrorCode pair the audit SendGetAsync already populates.

Resolution

Resolved 2026-05-28 (commit pending). Promoted CommandHelpers.IsAuthorizationFailure from private to internal so both helpers can reuse the same auth-failure rule (HTTP 403 OR FORBIDDEN/UNAUTHORIZED error code, case-insensitive). AuditQueryHelpers.RunQueryAsync now returns CommandHelpers.IsAuthorizationFailure(response) ? 2 : 1 on the error path instead of an unconditional 1. AuditExportHelpers.RunExportAsync doesn't ride ManagementResponse (it streams directly via SendGetStreamAsync), so a new AuditExportHelpers.TryExtractErrorCode helper parses the server's JSON error envelope to extract code, and the !IsSuccessStatusCode branch returns exit 2 on either HTTP 403 or a FORBIDDEN/UNAUTHORIZED envelope code. Regression tests: AuditQueryCommandTests.RunQuery_Http403_ReturnsExitCode2, ..._UnauthorizedCodeOnNon403_ReturnsExitCode2, ..._GenericServerError_ReturnsExitCode1 (negative guard); AuditExportCommandTests.RunExport_Http403_ReturnsExitCode2, ..._UnauthorizedCodeOnNon403_ReturnsExitCode2. All five fail on the pre-fix code and pass after.

CLI-019 — bundle export decodes the entire base64 bundle into memory before writing

Severity	Medium
Category	Performance & resource management
Status	Resolved
Location	src/ZB.MOM.WW.ScadaBridge.CLI/Commands/BundleCommands.cs:117-124, src/ZB.MOM.WW.ScadaBridge.CLI/ManagementHttpClient.cs:47-92

Resolution (2026-05-28): Replaced the Convert.FromBase64String(...) + File.WriteAllBytes(...) pair with a new StreamBase64ToFile(base64, output) helper that slices the base64 string into 4-char-aligned chunks (1 MB by default) and decodes each chunk straight into a FileStream via Convert.TryFromBase64Chars. The intermediate byte[] and the synchronous full-bundle write are gone — peak working set drops from ~base64-string + ~byte[] + ~envelope-string to ~base64-string + small-chunk-buffer + ~envelope-string. The response body is still buffered into the management envelope string (the POST /management wire format does not currently support streaming responses — this finding is bounded by that limit per the recommendation's stop-gap), so a streaming POST /api/bundle/export REST endpoint remains a follow-up. Regression tests BundleCommandsStreamingTests (6 tests) cover small payload round-trip, multi-chunk boundaries, empty input, invalid base64 → FormatException, and argument validation.

Description

Component-Transport.md:271 ceilings the raw bundle at 100 MB and notes the per-request body cap is raised to 200 MB once base64-inflated. The CLI's export path goes through ManagementHttpClient.SendCommandAsync, which reads the entire response body into a string (responseBody = await httpResponse.Content.ReadAsStringAsync(...)) and returns it as ManagementResponse.JsonData. BundleCommands.BuildExport then:

1. JsonDocument.Parse(jsonOk) re-allocates the JSON DOM (~200 MB string + DOM).
2. doc.RootElement.GetProperty("base64Bundle").GetString() materializes the base64 payload as another ~200 MB string.
3. Convert.FromBase64String(base64) allocates a fresh ~100 MB byte[].
4. File.WriteAllBytes(output, bytes) writes synchronously.

Peak working-set for a 100 MB bundle is therefore ~600 MB, all on the LOH, plus the file-I/O is fully synchronous. The streaming SendGetStreamAsync path the audit export uses (line 155-156) shows the right pattern is already available for plain GETs, but bundles ride a POST /management envelope so they currently can't reuse it.

Recommendation

For the export path specifically, add a streaming variant — either a new POST /api/bundle/export REST endpoint mirroring the audit pattern, or a chunk-fetch follow-up GET /api/bundle/<exportId> so the CLI can stream bytes through Stream.CopyToAsync without buffering the whole envelope. If a v1 stop-gap is needed, at minimum switch to File.WriteAllBytesAsync and use Convert.TryFromBase64Chars with a rented buffer to avoid the double-LOH allocation.

Resolution

Unresolved.

CLI-020 — bundle export success-envelope parse is unguarded

Severity	Low
Category	Correctness & logic bugs
Status	Resolved
Location	src/ZB.MOM.WW.ScadaBridge.CLI/Commands/BundleCommands.cs:117-126

Resolution (2026-05-28): Wrapped the JsonDocument.Parse + GetProperty extraction in a try/catch (JsonException or KeyNotFoundException or InvalidOperationException) block and the StreamBase64ToFile call in a separate try/catch (FormatException). Either failure now emits a clean OutputFormatter.WriteError(..., "INVALID_RESPONSE") and returns exit 1, matching the graceful-degradation pattern established by CLI-002 / CLI-003 / CLI-005. A malformed/abbreviated envelope no longer terminates the CLI with a raw stack trace.

Description

The export success handler does:

using var doc = JsonDocument.Parse(jsonOk);
var base64 = doc.RootElement.GetProperty("base64Bundle").GetString()!;
var byteCount = doc.RootElement.GetProperty("byteCount").GetInt32();
var bytes = Convert.FromBase64String(base64);

None of these calls are wrapped in a try/catch. A server-side bug that omits one of the two properties, returns a null base64Bundle, sends invalid base64, or sends a malformed JSON envelope will surface as one of KeyNotFoundException / InvalidOperationException / FormatException — an unhandled stack trace, not a clean INVALID_RESPONSE / exit 1, contradicting the "graceful-degradation" theme that the prior reviews (CLI-002, CLI-003, CLI-005) repeatedly hardened.

Recommendation

Wrap the parse + base64-decode in a try block that catches JsonException, KeyNotFoundException, InvalidOperationException, and FormatException and emits a clean OutputFormatter.WriteError(..., "INVALID_RESPONSE") + return 1. Add a regression test against a malformed-envelope stub HttpMessageHandler.

CLI-021 — CliConfig.Load crashes the CLI on a malformed config file

Severity	Low
Category	Error handling & resilience
Status	Resolved
Location	src/ZB.MOM.WW.ScadaBridge.CLI/CliConfig.cs:41-53

Resolution (2026-05-28): Wrapped the File.ReadAllText + JsonSerializer.Deserialize calls in a try/catch for JsonException/IOException/UnauthorizedAccessException that prints one warning to Console.Error and falls through with default values, so command-line and env-var precedence still works against a malformed ~/.scadabridge/config.json. Regression test CliConfigTests.Load_MalformedConfigFile_DoesNotThrow_WarnsAndReturnsDefault redirects HOME/USERPROFILE to a temp dir containing invalid JSON, asserts no throw, defaulted values, and the stderr warning.

Description

CliConfig.Load is the first thing every command runs (via ExecuteCommandAsync, AuditCommandHelpers.ResolveConnection, and BundleCommands.RunBundleCommandAsync). Its config-file branch is:

if (File.Exists(configPath))
{
    var json = File.ReadAllText(configPath);
    var fileConfig = JsonSerializer.Deserialize<CliConfigFile>(json, ...);
    ...
}

Neither call is guarded. If ~/.scadabridge/config.json exists but is malformed (stale, partial, or someone's vim swap), JsonSerializer.Deserialize throws JsonException. If the file exists but isn't readable (mode 0000), File.ReadAllText throws UnauthorizedAccessException. Either fault aborts every CLI invocation with an unhandled stack trace — even invocations that supply every input on the command line and don't need the config file at all (--url, --username, --password, --format all on the CLI).

Recommendation

Wrap the file-read and the JsonSerializer.Deserialize in a single try/catch (Exception) (or specifically JsonException + UnauthorizedAccessException + IOException). On failure, write a single one-line warning to Console.Error ("ignoring malformed ~/.scadabridge/config.json: {message}") and return the default CliConfig, so the rest of the precedence chain (env vars + command-line flags) still works.

Resolution

Unresolved.

CLI-022 — CommandTreeTests excludes the two new command groups

Severity	Low
Category	Testing coverage
Status	Resolved
Location	tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/CommandTreeTests.cs:21-37, :55-58 (vs. src/ZB.MOM.WW.ScadaBridge.CLI/Program.cs:21-36)

Resolution (2026-05-28): Added AuditCommands.Build and BundleCommands.Build to AllCommandGroups(), bumped the count assertion to Equal(16, …) with a maintenance comment, and added three new sub-command-surface tests (AllCommandGroups_Contains_AuditAndBundle, AuditCommandGroup_HasQueryExportAndVerifyChain, BundleCommandGroup_HasExportPreviewAndImport). CommandPayloadTypes_ResolveViaRegistry now also pins ExportBundleCommand / PreviewBundleCommand / ImportBundleCommand through ManagementCommandRegistry.

Description

CommandTreeTests.AllCommandGroups() builds 14 command groups; Program.cs now registers 16 (AuditCommands and BundleCommands were added since the last re-review). Worse, the smoke test pins Assert.Equal(14, groups.Count), so the test list intentionally matches the harness's array and stays green even though the real production tree is two groups larger. The downstream assertions (EveryLeafCommand_HasAnAction, CommandPayloadTypes_ResolveViaRegistry) therefore also do NOT cover the new audit / bundle leaves — and BundleCommands has zero test coverage of any kind (no parsing tests, no success-handler tests, no registry-resolution tests).

Recommendation

Add AuditCommands.Build(...) and BundleCommands.Build(...) to the AllCommandGroups() array, bump the assertion to Equal(16, groups.Count), and add representative payload types to CommandPayloadTypes_ResolveViaRegistry (ExportBundleCommand, PreviewBundleCommand, ImportBundleCommand). Optionally, add a BundleCommandsTests file covering the success-envelope parse and the NameListOption comma-split parser.

CLI-023 — Component-CLI.md claims audit commands ride POST /management; implementation uses REST endpoints

Severity	Low
Category	Design-document adherence
Status	Resolved
Location	docs/requirements/Component-CLI.md:310-311 (vs. src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditQueryHelpers.cs:186, src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditExportHelpers.cs:126, src/ZB.MOM.WW.ScadaBridge.CLI/ManagementHttpClient.cs:94-156)

Resolution (2026-05-28): Updated Component-CLI.md Dependencies bullets — the Management Service (#18) bullet now says the scadabridge audit group rides a parallel REST surface (GET /api/audit/query / GET /api/audit/export) sharing HTTP Basic Auth with /management but bypassing the actor; the Audit Log (#23) bullet names the specific endpoints and the server-side AuditEndpoints permission enforcement (OperationalAudit / AuditExport).

Description

Component-CLI.md:310 states: "The scadabridge audit command group rides this same transport — there is no separate audit endpoint." But the implementation calls a new REST surface — GET /api/audit/query and GET /api/audit/export — via two new methods on ManagementHttpClient (SendGetAsync, SendGetStreamAsync), distinct from the POST /management envelope. The plan document (docs/plans/2026-05-20-audit-log-code-roadmap.md:1583) corroborates the implementation: "REST endpoints GET /api/audit/query (paged) and GET /api/audit/export (streaming)" — i.e. the design doc is the stale one.

A reader following Component-CLI.md would expect the audit endpoints to share the management envelope's authentication + dispatch path and route through ManagementActor, neither of which is true. The auth-exit-code regression (CLI-018) is itself a direct consequence of this divergence: the audit helpers duplicate the management envelope's response handling instead of riding it, and forgot to copy the auth carve-out.

Recommendation

Update Component-CLI.md:310-311 (and the Dependencies bullet at :311) to describe the actual REST surface: GET /api/audit/query (paged) and GET /api/audit/export (streaming), with HTTP Basic Auth shared with the management envelope and permission checks enforced by the server-side AuditController. Optionally cross-link to docs/plans/2026-05-20-audit-log-code-roadmap.md (M8 task list) as the authoritative source.

Resolution

Unresolved.