From 63e4a6baab325c40cd7649b4c89e4be6e9c079f4 Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Fri, 22 May 2026 09:55:10 -0400
Subject: [PATCH] fix(driver-modbus-cli): resolve Medium code-review finding
 (Driver.Modbus.Cli-001)

Add --bit-index, --string-length, and --string-byte-order options to
SubscribeCommand, mirroring ReadCommand, and pass them into ModbusTagDefinition
so that BitInRegister and String type subscriptions use the correct bit index and
string length rather than silently defaulting to bit-0 / zero-length.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 code-reviews/Driver.Modbus.Cli/findings.md    | 10 +++++-----
 .../Commands/SubscribeCommand.cs              | 20 ++++++++++++++++++-
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/code-reviews/Driver.Modbus.Cli/findings.md b/code-reviews/Driver.Modbus.Cli/findings.md
index 3da7507..3eb3792 100644
--- a/code-reviews/Driver.Modbus.Cli/findings.md
+++ b/code-reviews/Driver.Modbus.Cli/findings.md
@@ -7,7 +7,7 @@
 | Review date | 2026-05-22 |
 | Commit reviewed | `76d35d1` |
 | Status | Reviewed |
-| Open findings | 8 |
+| Open findings | 6 |
 
 ## Checklist coverage
 
@@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank.
 | Severity | Medium |
 | Category | Correctness & logic bugs |
 | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs:43-51` |
-| Status | Open |
+| Status | Resolved |
 
 **Description:** `SubscribeCommand` synthesises its `ModbusTagDefinition` with only
 `Name`, `Region`, `Address`, `DataType`, `Writable`, and `ByteOrder` — it never
@@ -54,7 +54,7 @@ options to `SubscribeCommand` (mirroring `ReadCommand`) and pass them into the
 actually supports and reject `BitInRegister` / `String` at command entry with a
 clear message.
 
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-22 — added `--bit-index`, `--string-length`, and `--string-byte-order` options to `SubscribeCommand`, mirroring `ReadCommand`, and passed them through to `ModbusTagDefinition` so `BitInRegister` and `String` types subscribe correctly.
 
 ### Driver.Modbus.Cli-002
 
@@ -63,7 +63,7 @@ clear message.
 | Severity | Medium |
 | Category | Correctness & logic bugs |
 | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs:54-89` |
-| Status | Open |
+| Status | Resolved |
 
 **Description:** `WriteCommand` rejects read-only regions (`DiscreteInputs` /
 `InputRegisters`) but does not validate that `--type` is meaningful for the
@@ -78,7 +78,7 @@ region only supports `Bool`-style boolean values.
 combined with any non-boolean `--type` (anything other than `Bool`), with a
 message explaining coils carry a single bit.
 
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-22 — added a `Region == Coils && DataType != Bool` check immediately after the read-only-region guard, throwing `CommandException` with a message explaining that coils carry a single bit and only `--type Bool` is valid.
 
 ### Driver.Modbus.Cli-003
 
diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs
index b536351..5f9168a 100644
--- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs
+++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs
@@ -35,6 +35,21 @@ public sealed class SubscribeCommand : ModbusCommandBase
         "BigEndian (default) or WordSwap.")]
     public ModbusByteOrder ByteOrder { get; init; } = ModbusByteOrder.BigEndian;
 
+    // Driver.Modbus.Cli-001: subscribe previously lacked these three options that read and
+    // write both expose. Without them, BitInRegister always watches bit 0 and String runs with
+    // StringLength=0, silently producing wrong results for any subscriber using those types.
+    [CommandOption("bit-index", Description =
+        "For type=BitInRegister: which bit of the holding register (0-15, LSB-first).")]
+    public byte BitIndex { get; init; }
+
+    [CommandOption("string-length", Description =
+        "For type=String: character count (2 per register, rounded up).")]
+    public ushort StringLength { get; init; }
+
+    [CommandOption("string-byte-order", Description =
+        "For type=String: HighByteFirst (standard) or LowByteFirst (DirectLOGIC).")]
+    public ModbusStringByteOrder StringByteOrder { get; init; } = ModbusStringByteOrder.HighByteFirst;
+
     public override async ValueTask ExecuteAsync(IConsole console)
     {
         ConfigureLogging();
@@ -47,7 +62,10 @@ public sealed class SubscribeCommand : ModbusCommandBase
             Address: Address,
             DataType: DataType,
             Writable: false,
-            ByteOrder: ByteOrder);
+            ByteOrder: ByteOrder,
+            BitIndex: BitIndex,
+            StringLength: StringLength,
+            StringByteOrder: StringByteOrder);
         var options = BuildOptions([tag]);
 
         await using var driver = new ModbusDriver(options, DriverInstanceId);