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

fix(cli): resolve CLI-002..007 — robust response rendering, URL/JSON arg validation, credential env-vars, doc refresh

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-16 20:58:03 -04:00
parent 658b659c0c
commit 738e67acc5
15 changed files with 685 additions and 150 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 12 |
| Open findings | 6 |
## Summary
@@ -92,7 +92,7 @@ now call `ResolveFormat`. Regression tests added in `FormatResolutionTests`.
|--|--|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.CLI/Commands/CommandHelpers.cs:59-68`, `src/ScadaLink.CLI/Commands/CommandHelpers.cs:78-80` |
**Description**
@@ -112,7 +112,13 @@ output" case (print nothing or `(ok)`), and return 0 before attempting to parse.
**Resolution**
_Unresolved._
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
@@ -120,7 +126,7 @@ _Unresolved._
|--|--|
| Severity | Medium |
| Category | Error handling & resilience |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.CLI/Commands/CommandHelpers.cs:80` |
**Description**
@@ -139,7 +145,11 @@ printing the raw body verbatim (as the JSON path already does at line 66).
**Resolution**
_Unresolved._
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`
@@ -147,7 +157,7 @@ _Unresolved._
|--|--|
| Severity | Medium |
| Category | Error handling & resilience |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.CLI/ManagementHttpClient.cs:13` |
**Description**
@@ -166,7 +176,12 @@ clean `INVALID_URL` error with exit code 1 on failure.
**Resolution**
_Unresolved._
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
@@ -174,7 +189,7 @@ _Unresolved._
|--|--|
| Severity | Medium |
| Category | Error handling & resilience |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.CLI/Commands/InstanceCommands.cs:55-58`, `src/ScadaLink.CLI/Commands/InstanceCommands.cs:181-182` |
**Description**
@@ -195,7 +210,15 @@ code and return 1.
**Resolution**
_Unresolved._
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
@@ -203,7 +226,7 @@ _Unresolved._
|--|--|
| Severity | Medium |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.CLI/Program.cs:9`, `src/ScadaLink.CLI/Commands/CommandHelpers.cs:36-44` |
**Description**
@@ -225,7 +248,17 @@ supplied.
**Resolution**
_Unresolved._
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
@@ -233,7 +266,7 @@ _Unresolved._
|--|--|
| Severity | Medium |
| Category | Design-document adherence |
| Status | Open |
| Status | Resolved |
| Location | `docs/requirements/Component-CLI.md:51-211` (vs. all files under `src/ScadaLink.CLI/Commands/`) |
**Description**
@@ -267,7 +300,17 @@ authoritative.
**Resolution**
_Unresolved._
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