From 7580e378078b6ebed026d4456717e75eb644eeae Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 12:08:45 -0400 Subject: [PATCH] review(Driver.TwinCAT.Cli): clean parse errors + FlushLogging() in finally Re-review at 7286d320. -008 (Low): ParseValue maps FormatException/OverflowException to a clean CommandException (was raw stack trace) + tests. -009: FlushLogging() in all 5 commands' finally blocks (parity with AbCip.Cli). --- code-reviews/Driver.TwinCAT.Cli/findings.md | 86 ++++++++++++++++++- .../Commands/BrowseCommand.cs | 1 + .../Commands/ProbeCommand.cs | 1 + .../Commands/ReadCommand.cs | 1 + .../Commands/SubscribeCommand.cs | 1 + .../Commands/WriteCommand.cs | 63 +++++++++----- .../WriteCommandParseValueTests.cs | 27 +++++- 7 files changed, 154 insertions(+), 26 deletions(-) diff --git a/code-reviews/Driver.TwinCAT.Cli/findings.md b/code-reviews/Driver.TwinCAT.Cli/findings.md index 182db7ac..e936ea3b 100644 --- a/code-reviews/Driver.TwinCAT.Cli/findings.md +++ b/code-reviews/Driver.TwinCAT.Cli/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-06-19 | +| Commit reviewed | `111d6983` | | Status | Reviewed | | Open findings | 0 | @@ -251,3 +251,85 @@ accessor required by the abstract base property is intentionally a no-op, and th backing field would cause the two to drift on every refactor. The inner-block comment was tightened to point at the XML summary so the design intent survives whichever doc surface a future maintainer reads first. + +--- + +## Re-review 2026-06-19 (commit 111d6983) + +All seven prior findings remain Resolved. The re-review covers the same 8 source files and 4 +test files at the current HEAD. No new findings were added to categories 2, 3, 5, 6, 7, 8; +two new Low findings were identified in categories 4 and 1/4, both fixed in-session. + +#### Checklist coverage (re-review) + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | No new issues found | +| 2 | OtOpcUa conventions | No new issues found | +| 3 | Concurrency & thread safety | No new issues found | +| 4 | Error handling & resilience | Driver.TwinCAT.Cli-008, Driver.TwinCAT.Cli-009 | +| 5 | Security | No new issues found | +| 6 | Performance & resource management | No new issues found | +| 7 | Design-document adherence | No new issues found | +| 8 | Code organization & conventions | No new issues found | +| 9 | Testing coverage | No new issues found | +| 10 | Documentation & comments | No new issues found | + +### Driver.TwinCAT.Cli-008 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Error handling & resilience | +| Location | `Commands/WriteCommand.cs:73-93` | +| Status | Resolved | + +**Description:** `ParseValue` delegates to the BCL numeric parse methods (`int.Parse`, +`sbyte.Parse`, etc.) which throw raw `FormatException` or `OverflowException` on bad +input. Those exceptions propagate all the way through `ExecuteAsync` and are caught by +CliFx's top-level handler, but CliFx prints a full stack trace for unrecognised exception +types. An operator passing `--value xyz --type DInt` sees several lines of .NET internals +instead of the single-line "Cannot parse 'xyz' as DInt" message that `CommandException` would +produce. The existing test `ParseValue_non_numeric_for_numeric_types_throws` confirmed and +preserved the broken behaviour by asserting `FormatException`, so the gap survived review. + +**Recommendation:** Wrap the switch body in a `try/catch` that re-throws `CommandException` +(which is already thrown by `ParseBool`) and maps `FormatException`/`OverflowException` to a +`CommandException` carrying the raw value and type name. + +**Resolution:** Wrapped the `ParseValue` switch in `try/catch`; `FormatException` and +`OverflowException` are caught (via `when` pattern) and re-thrown as `CommandException` +with `"Cannot parse '{raw}' as {type}: {message}"`. Pre-existing `CommandException` from +`ParseBool` / the unsupported-type arm / the `Structure` guard are re-thrown unchanged. +Updated the test: renamed `ParseValue_non_numeric_for_numeric_types_throws` → +`ParseValue_non_numeric_for_numeric_types_throws_CommandException` and asserted the message +contains both the value and type; added a second test +`ParseValue_overflow_for_numeric_types_throws_CommandException` (300 as SInt). 70 tests +pass (was 69). Date: 2026-06-19. SHA: blank (not committed). + +### Driver.TwinCAT.Cli-009 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Error handling & resilience | +| Location | `Commands/BrowseCommand.cs:62-65`, `Commands/ProbeCommand.cs:59-62`, `Commands/ReadCommand.cs:50-53`, `Commands/WriteCommand.cs:63-67`, `Commands/SubscribeCommand.cs:103-111` | +| Status | Resolved | + +**Description:** `DriverCommandBase` documents that `FlushLogging()` must be called in a +`finally` block to flush any buffered Serilog output before the process exits (see its XML +`` on `FlushLogging`). None of the five TwinCAT CLI commands called it. The sibling +`AbCip.Cli` correctly calls `FlushLogging()` in each command's `finally`; TwinCAT, Modbus, +S7, AbLegacy, and FOCAS CLIs all omit it. The impact is that any `--verbose` debug lines +buffered by Serilog's default asynchronous sink may be lost on process exit — a diagnostic +CLI that drops diagnostic output on crash is self-defeating. + +**Recommendation:** Add `FlushLogging()` as the last statement in every command's outer +`finally` block, after `ShutdownAsync` (mirroring the AbCip pattern). + +**Resolution:** Added `FlushLogging()` as the last call in the `finally` block of every +`ExecuteAsync` in `BrowseCommand`, `ProbeCommand`, `ReadCommand`, `WriteCommand`, and +`SubscribeCommand`. No test change needed (the call is not unit-testable without a real ADS +router and a real Serilog sink; the fix is verified structurally at build time). The systemic +omission in the Modbus/S7/AbLegacy/FOCAS sibling CLIs is out of this module's scope and left +for their respective re-reviews. Date: 2026-06-19. SHA: blank (not committed). diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/BrowseCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/BrowseCommand.cs index fc8bec30..6e2a9223 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/BrowseCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/BrowseCommand.cs @@ -62,6 +62,7 @@ public sealed class BrowseCommand : TwinCATCommandBase finally { await driver.ShutdownAsync(CancellationToken.None); + FlushLogging(); } var matched = FilterByPrefix(builder.Variables, Prefix); diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ProbeCommand.cs index 364241ff..0aeadbfd 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ProbeCommand.cs @@ -59,6 +59,7 @@ public sealed class ProbeCommand : TwinCATTagCommandBase finally { await driver.ShutdownAsync(CancellationToken.None); + FlushLogging(); } } } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ReadCommand.cs index b597cbad..22e23894 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ReadCommand.cs @@ -50,6 +50,7 @@ public sealed class ReadCommand : TwinCATTagCommandBase finally { await driver.ShutdownAsync(CancellationToken.None); + FlushLogging(); } } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/SubscribeCommand.cs index c1750aa5..12ade782 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/SubscribeCommand.cs @@ -108,6 +108,7 @@ public sealed class SubscribeCommand : TwinCATTagCommandBase catch { /* teardown best-effort */ } } await driver.ShutdownAsync(CancellationToken.None); + FlushLogging(); } } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/WriteCommand.cs index 8aa3a0cc..d19549e7 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/WriteCommand.cs @@ -63,33 +63,54 @@ public sealed class WriteCommand : TwinCATTagCommandBase finally { await driver.ShutdownAsync(CancellationToken.None); + FlushLogging(); } } - /// Parse --value per , invariant culture. + /// + /// Parse --value per , invariant culture. Wraps + /// and in a + /// so the operator sees a clean one-line + /// error instead of a raw stack trace (Driver.TwinCAT.Cli-008). + /// /// The raw string value to parse. /// The target TwinCAT data type. - internal static object ParseValue(string raw, TwinCATDataType type) => type switch + internal static object ParseValue(string raw, TwinCATDataType type) { - TwinCATDataType.Bool => ParseBool(raw), - TwinCATDataType.SInt => sbyte.Parse(raw, CultureInfo.InvariantCulture), - TwinCATDataType.USInt => byte.Parse(raw, CultureInfo.InvariantCulture), - TwinCATDataType.Int => short.Parse(raw, CultureInfo.InvariantCulture), - TwinCATDataType.UInt => ushort.Parse(raw, CultureInfo.InvariantCulture), - TwinCATDataType.DInt => int.Parse(raw, CultureInfo.InvariantCulture), - TwinCATDataType.UDInt => uint.Parse(raw, CultureInfo.InvariantCulture), - TwinCATDataType.LInt => long.Parse(raw, CultureInfo.InvariantCulture), - TwinCATDataType.ULInt => ulong.Parse(raw, CultureInfo.InvariantCulture), - TwinCATDataType.Real => float.Parse(raw, CultureInfo.InvariantCulture), - TwinCATDataType.LReal => double.Parse(raw, CultureInfo.InvariantCulture), - TwinCATDataType.String or TwinCATDataType.WString => raw, - // IEC 61131-3 time/date types are stored as UDINT on the wire — accept a numeric raw - // value + let the caller handle the encoding semantics. - TwinCATDataType.Time or TwinCATDataType.Date - or TwinCATDataType.DateTime or TwinCATDataType.TimeOfDay - => uint.Parse(raw, CultureInfo.InvariantCulture), - _ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."), - }; + try + { + return type switch + { + TwinCATDataType.Bool => ParseBool(raw), + TwinCATDataType.SInt => sbyte.Parse(raw, CultureInfo.InvariantCulture), + TwinCATDataType.USInt => byte.Parse(raw, CultureInfo.InvariantCulture), + TwinCATDataType.Int => short.Parse(raw, CultureInfo.InvariantCulture), + TwinCATDataType.UInt => ushort.Parse(raw, CultureInfo.InvariantCulture), + TwinCATDataType.DInt => int.Parse(raw, CultureInfo.InvariantCulture), + TwinCATDataType.UDInt => uint.Parse(raw, CultureInfo.InvariantCulture), + TwinCATDataType.LInt => long.Parse(raw, CultureInfo.InvariantCulture), + TwinCATDataType.ULInt => ulong.Parse(raw, CultureInfo.InvariantCulture), + TwinCATDataType.Real => float.Parse(raw, CultureInfo.InvariantCulture), + TwinCATDataType.LReal => double.Parse(raw, CultureInfo.InvariantCulture), + TwinCATDataType.String or TwinCATDataType.WString => raw, + // IEC 61131-3 time/date types are stored as UDINT on the wire — accept a numeric raw + // value + let the caller handle the encoding semantics. + TwinCATDataType.Time or TwinCATDataType.Date + or TwinCATDataType.DateTime or TwinCATDataType.TimeOfDay + => uint.Parse(raw, CultureInfo.InvariantCulture), + _ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."), + }; + } + catch (CliFx.Exceptions.CommandException) + { + throw; // already a clean user-facing error (Bool parse, Structure, unsupported type) + } + catch (Exception ex) when (ex is FormatException or OverflowException) + { + throw new CliFx.Exceptions.CommandException( + $"Cannot parse '{raw}' as {type}: {ex.Message}"); + } + } private static bool ParseBool(string raw) => raw.Trim().ToLowerInvariant() switch { diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs index 95b62eb1..89ed5c22 100644 --- a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs @@ -141,12 +141,33 @@ public sealed class WriteCommandParseValueTests () => WriteCommand.ParseValue("42", TwinCATDataType.Structure)); } - /// Verifies that ParseValue non-numeric for numeric types throws. + /// + /// Verifies that ParseValue wraps raw parse errors in a CommandException so the operator + /// sees a clean one-line message (Driver.TwinCAT.Cli-008). + /// [Fact] - public void ParseValue_non_numeric_for_numeric_types_throws() + public void ParseValue_non_numeric_for_numeric_types_throws_CommandException() { - Should.Throw( + // Previously threw raw FormatException. After the fix, FormatException + OverflowException + // are caught and re-thrown as CommandException so CliFx prints a single error line. + var ex = Should.Throw( () => WriteCommand.ParseValue("xyz", TwinCATDataType.DInt)); + ex.Message.ShouldContain("xyz"); + ex.Message.ShouldContain("DInt"); + } + + /// + /// Verifies that ParseValue wraps OverflowException (value out of type range) in a + /// CommandException (Driver.TwinCAT.Cli-008). + /// + [Fact] + public void ParseValue_overflow_for_numeric_types_throws_CommandException() + { + // 300 is out of range for SInt (sbyte: -128..127). + var ex = Should.Throw( + () => WriteCommand.ParseValue("300", TwinCATDataType.SInt)); + ex.Message.ShouldContain("300"); + ex.Message.ShouldContain("SInt"); } /// Verifies that SynthesiseTagName preserves symbolic path verbatim.