Commit Graph

158 Commits

Author SHA1 Message Date
Joseph Doherty b827b0c0a2 fix(driver-s7): resolve Medium code-review finding (Driver.S7-012)
Remove the dead ProbeAddress config surface from S7ProbeOptions and the factory
DTO. ProbeLoopAsync uses Plc.ReadStatusAsync (CPU-status PDU), not a tag-address
read — ProbeAddress was never consumed. The XML doc on Probe is corrected to
describe the ReadStatusAsync-based probe. Existing configs that set probeAddress
are silently ignored by the JSON deserializer.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 10:16:54 -04:00
Joseph Doherty 19a2a81321 fix(driver-modbus-addressing): resolve Medium code-review finding (Driver.Modbus.Addressing-003)
Complete the incomplete Addressing-003 fix: TryParseByteOrder now produces a
diagnostic mentioning "field 2" when a known type-code token (e.g. BOOL) is
supplied in the byte-order slot, so the user is guided to the correct field.
The previous fix only wired the message in the else-branch, which was unreachable
because LooksLikeByteOrderToken(BOOL) returned true first.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 10:16:30 -04:00
Joseph Doherty dd1742e319 fix(driver-modbus-cli): resolve Medium code-review finding (Driver.Modbus.Cli-002)
Reject --region Coils combined with any non-boolean --type with a CommandException
that names the constraint: coils carry a single bit, so only --type Bool is valid.
Without this check a write like "--region Coils --type UInt16 --value 42" would
silently coerce to a coil ON with no diagnostic.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:55:16 -04:00
Joseph Doherty 63e4a6baab 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>
2026-05-22 09:55:10 -04:00
Joseph Doherty ba52c179fd fix(driver-modbus-addressing): resolve Medium code-review finding (Driver.Modbus.Addressing-002)
Reject an empty 3rd field in the address parser by checking parts[2].Length > 0
before the All(char.IsDigit) guard, so a trailing-colon typo like "40001:F:"
produces a diagnostic instead of silently parsing as a scalar.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:52:52 -04:00
Joseph Doherty ebfd5d7871 fix(driver-galaxy): fix XML doc comment cref in StatusCodeMap.ToQualityCategoryByte
StatusCode is not a .NET type reference in this assembly — replace the unresolvable
<see cref="StatusCode"/> with prose text so TreatWarningsAsErrors does not fail the
build on the CS1574 unresolved-cref warning.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:51:17 -04:00
Joseph Doherty ecc91b0e48 fix(driver-galaxy): resolve Medium code-review finding (Driver.Galaxy-011)
GetMemoryFootprint() returned a constant 0 with a stale "PR 4.4 sets this" comment
even though PR 4.4 shipped the SubscriptionRegistry. Replace with a live estimate:
64 bytes × TrackedItemHandleCount + 256 bytes × TrackedSubscriptionCount. A 50k-tag
set now registers ~3 MB with the server's cache-flush heuristic instead of being
invisible. Returns 0 when no subscriptions are active.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:48:37 -04:00
Joseph Doherty c5f2d91bcb fix(driver-modbus): resolve Medium code-review finding (Driver.Modbus-002)
Clear _tagsByName, _lastPublishedByRef, and _lastWrittenByRef in ShutdownAsync
(via the new shared TeardownAsync helper) so a ReinitializeAsync cycle starts
from a clean state, consistent with the existing _autoProhibited.Clear().

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:48:09 -04:00
Joseph Doherty 0f3de4d510 fix(driver-galaxy): resolve Medium code-review finding (Driver.Galaxy-009)
Fix two resource-management bugs in StartDeployWatcher / BuildDefaultHierarchySource:
(a) Replace the discarded `_ = StartAsync(...)` with an explicit task variable that
    surfaces any synchronous InvalidOperationException (called-twice guard) rather than
    silently swallowing it.
(b) Change both StartDeployWatcher and BuildDefaultHierarchySource to use ??= on
    _ownedRepositoryClient so the first client created (by whichever path runs first)
    is reused by the second path, preventing a second GalaxyRepositoryClient from being
    created and the first from leaking past the driver's lifetime.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:47:52 -04:00
Joseph Doherty d572a011ef fix(driver-galaxy): resolve Medium code-review finding (Driver.Galaxy-007)
Implement IAsyncDisposable on GalaxyDriver so async sub-component disposals
(EventPump, AlarmFeed, MxSession, MxClient, RepositoryClient) are awaited rather
than blocked on GetAwaiter().GetResult(). DisposeAsync is now the primary path;
Dispose() delegates to it for using-statement compatibility. Each async component's
shutdown is awaited individually with a best-effort catch so a single slow shutdown
cannot prevent the rest of the cleanup sequence from running.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:47:00 -04:00
Joseph Doherty d14564839e fix(driver-galaxy): resolve Medium code-review finding (Driver.Galaxy-006)
HashSet<T>.First() enumeration order is unspecified and unstable across mutations, so
the "owner" handle attached to alarm events was non-deterministic when multiple alarm
subscriptions were active. Change _alarmSubscriptions from HashSet to List (preserving
insertion order) and pick [0] — the earliest-registered handle — as the deterministic
owner. The server routes transitions by SourceNodeId, not by handle, so the choice of
handle does not affect delivery to active subscribers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:45:55 -04:00
Joseph Doherty 910a538b19 fix(driver-galaxy): resolve Medium code-review finding (Driver.Galaxy-004)
Add StatusCodeMap.ToQualityCategoryByte(uint) so the StatusCode → quality-byte
mapping lives in one place next to its inverse (FromQualityByte). GalaxyDriver
OnPumpDataChange now delegates to the helper instead of duplicating the shift+switch
inline; a future edit to the OPC UA bit layout cannot silently desync the probe-health
decode. Unit tests in StatusCodeMapTests pin all three category buckets and the
round-trip invariant.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:43:53 -04:00
Joseph Doherty 39a02f6794 fix(driver-galaxy): resolve Medium code-review finding (Driver.Galaxy-003)
StatusCodeMap.FromMxStatus checked `success != 0` to determine success, but the
mxaccessgw proto contract explicitly documents that `success` is not a boolean and
that clients must branch on `category` (MX_STATUS_CATEGORY_OK), not on `success`
alone. Replace the raw field check with `status.IsSuccess()` from
MxStatusProxyExtensions, which requires both `success != 0` AND `category == Ok`.
A worker reporting success=1 with a non-OK category was previously misreported as
Good. Updated StatusCodeMapTests with a regression case covering the inverted scenario.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:42:47 -04:00
Joseph Doherty f9dccaa732 Merge branch 'worktree-agent-ad34cad856c59bbc1' into feat/scripted-alarm-shelve-routing 2026-05-22 09:40:46 -04:00
Joseph Doherty f920de9878 Merge branch 'worktree-agent-af51f33c034e99fd4' into feat/scripted-alarm-shelve-routing 2026-05-22 09:40:46 -04:00
Joseph Doherty b21585767b Merge branch 'worktree-agent-aaf0e64363ca270b1' into feat/scripted-alarm-shelve-routing 2026-05-22 09:40:45 -04:00
Joseph Doherty ee5d7ad51e fix(driver-ablegacy): fix CS9124 build error and update stale status-mapper test
EffectiveCipPath now references ParsedAddress/Profile properties instead
of the captured primary-constructor parameters to avoid CS9124 (param
captured into enclosing type AND used to init a member).

NonZero_libplctag_status_maps_via_AbLegacyStatusMapper updated to pass
(int)Status.ErrorNotFound rather than the stale magic integer -14 that
the old mapper happened to handle but the new enum-based mapper does not.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:33:19 -04:00
Joseph Doherty 1a1b3df098 fix(driver-abcip): actually remove PlcTagHandle.cs from the git index
The file was physically deleted and unstaged in the Driver.AbCip-006
commit but the git rm was not included. Committed separately.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:31:56 -04:00
Joseph Doherty 17432bb1a4 fix(driver-abcip): correct Driver.AbCip-005 approach and fix 014 tests
Finding 005 revised approach: keep the parent Structure tag in
`_tagsByName` so the whole-UDT grouping planner can find it (required
for Driver.AbCip-003 opt-in path + alarm projection). Instead, detect a
direct read of a Structure-with-Members in `ReadSingleAsync` and return
`BadNotSupported` rather than Good/null — explicitly documenting the
contract that callers must address member paths. Duplicate-key checks
(scalar and member fan-out) remain.

Finding 014 test corrections: `Structure_parent_tag_read_returns_BadNotSupported`
now asserts the new contract. `Read_UDInt_tag_returns_uint_value_not_negative_wrapped_int`
assertion fixed to use `ShouldBeOfType<uint>()` instead of
`ShouldNotBe(-1)` (Shouldly overflows comparing uint.MaxValue with int).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:30:54 -04:00
Joseph Doherty e3648adcea fix(driver-ablegacy): resolve Medium code-review finding (Driver.AbLegacy-012)
Consume previously-dead AbLegacyPlcFamilyProfile fields:

- DeviceState.EffectiveCipPath applies DefaultCipPath when the parsed host
  address has an empty CIP path (SLC 500 / PLC-5 misconfigured without /1,0
  now gets the profile-supplied default route). All three tag/parent/probe
  Create() callers updated.
- InitializeAsync validates each tag's DataType against SupportsLongFile /
  SupportsStringFile and throws InvalidOperationException at init time so a
  MicroLogix Long tag or similar fails early rather than at runtime with an
  opaque comms error.
- MaxTagBytes tracked as a follow-up (string/array chunking requires broader
  design work).

Tests added for CipPath fallback and Long/String type validation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:30:42 -04:00
Joseph Doherty 228ad42ad7 fix(driver-ablegacy): resolve Medium code-review finding (Driver.AbLegacy-010)
MapLibplctagStatus now casts the int to libplctag.Status and switches on
named enum members (mirroring AbCipStatusMapper) instead of unverified
magic integers. A strongly-typed Status overload is the canonical path;
the int overload delegates to it. MapPcccStatus is retained with a comment
marking it as the reference mapping for future PCCC-STS inspection.
Tests updated to use Status enum members rather than raw integers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:28:27 -04:00
Joseph Doherty 54d51a1d20 fix(driver-ablegacy): resolve Medium code-review finding (Driver.AbLegacy-009)
InitializeAsync catch block now mirrors ShutdownAsync teardown: cancels
and disposes probe CancellationTokenSources, calls DisposeRuntimes, and
clears _devices/_tagsByName before rethrowing. A caller that catches and
abandons (rather than retrying via ReinitializeAsync) no longer leaves
orphaned probe tasks or libplctag handles alive.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:26:42 -04:00
Joseph Doherty 60ffcfe8bd fix(driver-ablegacy): resolve Medium code-review finding (Driver.AbLegacy-008)
Mark _health volatile. The record-reference assignment is atomic, but
without an acquire/release memory barrier GetHealth() on another thread
can observe a stale snapshot indefinitely. volatile enforces the barrier
at read and write sites without a lock.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:26:08 -04:00
Joseph Doherty 5b5e9ad83b fix(driver-focas): resolve Medium code-review finding (Driver.FOCAS-003)
Throw InvalidOperationException at InitializeAsync when a tag's
DeviceHostAddress does not match any entry in the Devices list, naming
both the tag and the unresolved host.  Previously the missing-device
check was guarded by a TryGetValue so a typo silently bypassed
capability-matrix validation and deferred the error to per-read
BadNodeIdUnknown — the opposite of the documented "fail at load" goal.

Also resolves findings 004, 005, and 006 in the same file:
- 004: DiscoverAsync now unconditionally emits ViewOnly for all user
  tags; the Writable config field no longer influences security class
  because the wire backend always returns BadNotWritable.
- 005: All _health reads use Volatile.Read and all writes use
  Volatile.Write so concurrent readers observe a consistent reference
  and read-modify-write sequences capture a stable snapshot.
- 006: EnsureConnectedAsync disposes and nulls any existing
  non-connected client before creating a fresh one, preventing
  ObjectDisposedException loops after a HandleRecycle race or teardown.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:25:57 -04:00
Joseph Doherty 7661d1b5dc fix(driver-ablegacy): resolve Medium code-review finding (Driver.AbLegacy-007)
Runtimes and ParentRuntimes changed from Dictionary to ConcurrentDictionary.
EnsureTagRuntimeAsync and EnsureParentRuntimeAsync now use a per-key
GetCreationLock semaphore with a double-checked pattern: fast-path read
requires no lock; slow-path create+initialize+store is serialised per key
so a concurrent caller waits rather than creating a duplicate runtime that
would be leaked when DisposeRuntimes runs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:25:35 -04:00
Joseph Doherty 72728a5d45 fix(driver-abcip): resolve Medium code-review finding (Driver.AbCip-010)
Add `EvictRuntime` helper that removes + disposes a stale
`ConcurrentDictionary` entry. Call it from `ReadSingleAsync`,
`ReadGroupAsync`, and `WriteAsync` on non-zero libplctag status and
transport exceptions so the next call for the same tag re-creates a
fresh handle — mirroring the probe loop's recreate-on-failure pattern.
Value-conversion exceptions (NotSupportedException, FormatException,
InvalidCastException, OverflowException) are not transport faults and
do not evict the handle.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:24:45 -04:00
Joseph Doherty 1723f5d5cd fix(driver-historian-wonderware): resolve Medium code-review finding (Driver.Historian.Wonderware-009)
Apply _config.MaxValuesPerRead as a bucket cap in ReadAggregateAsync,
mirroring the existing cap in ReadRawAsync. Without this guard a processed
read over a wide time range with a small IntervalMs could accumulate an
unbounded HistorianAggregateSample list; if the serialised reply exceeded
the 16 MiB FrameWriter frame cap WriteAsync would throw and the client
correlation-id wait would hang. Truncation now logs a Warning with a hint
to widen IntervalMs or reduce the time range.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:24:25 -04:00
Joseph Doherty 47eac2d84f fix(driver-ablegacy): resolve Medium code-review finding (Driver.AbLegacy-004)
DecodeValue for Bit with no bitIndex now reads the full 16-bit word via
GetInt16(0) and tests bit 0 instead of GetInt8(0), which only covered the
low byte and silently misread any bit in positions 8..15. The comment
explains the two decode paths (suffix-present vs suffix-absent).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:24:19 -04:00
Joseph Doherty 7474631992 fix(driver-historian-wonderware): resolve Medium code-review finding (Driver.Historian.Wonderware-006)
Add exponential backoff (250 ms → 500 ms → 1 s → 2 s → 4 s → 8 s cap) to
PipeServer.RunAsync after each connection-loop exception, replacing the spin
loop that previously pegged a CPU core and flooded the log on persistent errors
such as a duplicate pipe name or a failing PipeAcl.Create. After 20 consecutive
failures the method re-throws so the SCM / NSSM supervisor can restart the
sidecar cleanly. A clean connection (even a short-lived one) resets the counter.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:23:42 -04:00
Joseph Doherty 7d30009dc8 fix(driver-ablegacy): resolve Medium code-review finding (Driver.AbLegacy-003)
TryParse now rejects three classes of malformed PCCC address:
- Sub-element + bit-index together (e.g. T4:0.ACC/2) — never valid in PCCC
- File number on I/O/S system files (e.g. I3:0, S2:1) — single-letter only
- Sub-element on non-T/C/R files (e.g. B3:0.DN, N7:0.FOO) — only Timer,
  Counter, and Control files carry structured elements

New helper predicates IsNoFileNumberLetter / IsSubElementFileLetter
keep the parser's intent clear. Regression tests added in AbLegacyAddressTests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:23:35 -04:00
Joseph Doherty 02daacbfd0 fix(driver-historian-wonderware): resolve Medium code-review finding (Driver.Historian.Wonderware-003)
Extract the string-vs-numeric value selection from raw and at-time read
loops into a SelectValue helper method. aahClientManaged's HistoryQueryResult
has no data-type field in the bound SDK version, so the heuristic (prefer
StringValue when non-empty and Value==0) is unavoidable; the helper now
documents the limitation explicitly in its XML doc so the known edge case
(numeric tag at exactly zero with a formatted StringValue) is self-evident.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:23:00 -04:00
Joseph Doherty 37945deb0a fix(driver-abcip): resolve Medium code-review finding (Driver.AbCip-006)
`PlcTagHandle` and `DeviceState.TagHandles` were dead scaffolding: the
`ReleaseHandle` no-op never called `plc_tag_destroy` and the dict was
never populated. Removed the file, the dead dict, and its
`DisposeHandles` loop. Updated the `AbCipDriver` class doc to document
that native lifetime is owned by libplctag.NET `Tag.Dispose()` (invoked
from `DisposeHandles`) with the library's own finalizer covering any
GC-collected instances. Two test methods that only exercised the dead
`PlcTagHandle` class removed from `AbCipDriverTests`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:22:42 -04:00
Joseph Doherty 205b07f6aa fix(driver-historian-wonderware): resolve Medium code-review finding (Driver.Historian.Wonderware-002)
Normalise req.Events to Array.Empty<AlarmHistorianEventDto>() immediately
after MessagePack deserialization in HandleWriteAlarmEventsAsync. MessagePack
deserializes an absent or explicit-nil array field as null, not Array.Empty,
so a peer that sends a null Events array would trigger a NullReferenceException
on either .Length dereference (no-writer branch or catch block), leaving the
client correlation-id wait hanging with no reply frame.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:21:46 -04:00
Joseph Doherty 1679344ace fix(driver-historian-wonderware-client): resolve Medium code-review finding (Driver.Historian.Wonderware.Client-002)
Document explicitly that WriteBatchAsync never returns PermanentFail because
the WriteAlarmEventsReply wire contract carries only a bool-per-event (no
unrecoverable/transient distinction). Add a <remarks> XML block explaining
the structural limitation, why poison events retry rather than dead-letter,
and that a coordinated per-event status enum extension to the .NET 4.8
sidecar is a tracked follow-up. Add inline NOTE comments in both the
success and catch paths for discoverability.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:21:11 -04:00
Joseph Doherty 5bcbda1685 fix(driver-historian-wonderware-client): resolve Medium code-review finding (Driver.Historian.Wonderware.Client-007)
Introduce DeserializeSampleValue() helper that enforces a 64 KiB per-sample
ValueBytes size cap before calling MessagePackSerializer.Deserialize<object>,
and documents that the default StandardResolver (primitive-only, no typeless
or dynamic-type resolution) is in use. Both ToSnapshots and AlignAtTimeSnapshots
route through the new helper. Add inline XML comments to the two NuGetAuditSuppress
entries in the csproj recording the advisory title, why each does not apply to
this module's primitive-only deserialization, and when to revisit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:20:23 -04:00
Joseph Doherty d5b8c802ce fix(driver-abcip): resolve Medium code-review finding (Driver.AbCip-005)
Structure tags with declared Members no longer register the bare parent
name in `_tagsByName` — reading it would return Good/null, which is
misleading. Clients read individual member paths. Both the member
fan-out and the scalar-tag paths now perform a duplicate-key check that
throws `InvalidOperationException` naming both colliding entries (fail-
fast, consistent with the AbCipHostAddress validation pattern).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:20:22 -04:00
Joseph Doherty 1722c0328b fix(driver-abcip): resolve Medium code-review finding (Driver.AbCip-004)
`ToDriverDataType` mapped LInt/ULInt to Int32 (truncation) and UDInt
to Int32 (negative wrap for values > Int32.MaxValue). DriverDataType
already carries Int64/UInt64/UInt32, so map each Logix 64-bit and
unsigned-32-bit type to the correct member. `DecodeValueAt` in
`LibplctagTagRuntime` updated to return uint/ulong for UDInt/ULInt
so the runtime value type agrees with the declared OPC UA type.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:19:40 -04:00
Joseph Doherty 75580fb432 fix(driver-historian-wonderware-client): resolve Medium code-review finding (Driver.Historian.Wonderware.Client-005)
Replace the synchronous non-cancellable _stream.ReadByte() for the kind byte
in FrameReader.ReadFrameAsync with an async ReadExactAsync(new byte[1], ct)
call so the full frame read honours the EffectiveCallTimeout-linked token
and cannot wedge the call gate when the sidecar stalls mid-frame.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:19:14 -04:00
Joseph Doherty 6bb971c040 fix(driver-ablegacy-cli): resolve Medium code-review finding (Driver.AbLegacy.Cli-001)
WriteCommand.ParseValue wraps FormatException/OverflowException as
CliFx CommandException so a bad --value yields a clean one-line CLI error
naming the value and target type instead of a raw .NET stack trace.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:15:19 -04:00
Joseph Doherty 29e656912e fix(driver-abcip-cli): resolve Medium code-review findings (Driver.AbCip.Cli-001, -002)
Driver.AbCip.Cli-001: WriteCommand.ParseValue wraps FormatException/
OverflowException as CommandException so bad --value input yields a clean
CLI error instead of a raw stack trace.
Driver.AbCip.Cli-002: probe/read/subscribe commands reject Structure types
up front (RejectStructure helper), matching the write guard.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 09:14:41 -04:00
Joseph Doherty 7ff356bddc fix(driver-cli-common): resolve Medium code-review finding (Driver.Cli.Common-003)
ConfigureLogging is now idempotent via a _loggingConfigured guard field so
repeated calls from subclasses do not abandon and leak the previous logger.
The previous Log.Logger is disposed before overwriting to release its
console-sink resources cleanly.

A new protected static FlushLogging() helper calls Log.CloseAndFlush() so
commands can guarantee buffered output is flushed in their finally blocks
before the process exits — important for the long-running subscribe verb.

XML doc updated to reflect call-once semantics and document FlushLogging().

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 08:38:09 -04:00
Joseph Doherty 1433a1cf30 fix(driver-cli-common): resolve Medium code-review finding (Driver.Cli.Common-002)
FormatStatus now matches named codes against code & 0xFFFF0000 (high-word
mask) rather than exact equality, so status codes carrying sub-code or flag
bits in the low 16 bits (e.g. 0x80050001) still resolve to their named class.
For codes not in the named shortlist a severity-class fallback using the top
2 bits always emits Good / Uncertain / Bad rather than bare hex.

Updated the stale FormatStatus_unknown_codes_fall_back_to_hex_only test (its
expectation became invalid once the severity-class fallback was added) and
added new Theory cases exercising both the high-word matching and the
severity-class fallback paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 08:37:47 -04:00
Joseph Doherty 5499b817c8 fix(driver-historian-wonderware-client): resolve High code-review finding (Driver.Historian.Wonderware.Client-001)
WonderwareHistorianClient.ReadAtTimeAsync passed the sidecar's reply.Samples
straight through ToSnapshots, which violated the IHistorianDataSource
contract: the result MUST be the same length and order as the requested
timestampsUtc, with gaps returned as Bad-quality snapshots. If the sidecar
dropped or reordered samples, OPC UA HistoryReadAtTime would silently
misalign values with timestamps.

Add an AlignAtTimeSnapshots helper that indexes the returned samples by
timestamp ticks, builds the result array at timestampsUtc.Count in request
order, and emits a Bad-quality (0x80000000) snapshot for any requested
timestamp the sidecar did not return.

Add the ReadAtTimeAsync_PartialAndReorderedReply_AlignsByTimestamp_AndFillsGapsAsBad
regression test where the fake returns a partial, reordered sample set.

Update code-reviews/Driver.Historian.Wonderware.Client/findings.md: -001
Resolved, open-finding count 10 -> 9.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 06:59:40 -04:00
Joseph Doherty f982fa1f69 fix(driver-historian-wonderware): resolve High code-review finding (Driver.Historian.Wonderware-001)
WriteToReadOnlyFile was listed in MalformedErrors, so ClassifyOutcome/
MapOutcome routed it to PermanentFail and the store-and-forward sink
dead-lettered every alarm event in the batch. But WriteToReadOnlyFile is
a connection-configuration fault (the write session was opened without
ReadOnly = false), not an event-payload fault — treating it as permanent
silently and permanently discards alarm events on a misconfigured or
regressed connection, which is data loss.

Move WriteToReadOnlyFile from MalformedErrors into ConnectionErrors. The
batch loop now aborts the batch, resets the connection (so the reconnect
path re-opens a writable ReadOnly = false session), and defers the
events as RetryPlease for the next drain tick.

Updated the ClassifyOutcome theory data and added a dedicated regression
test pinning WriteToReadOnlyFile -> RetryPlease.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 06:59:40 -04:00
Joseph Doherty 1837b5a828 fix(driver-modbus-addressing): resolve High code-review finding (Driver.Modbus.Addressing-001)
The DL205 family-native branch routed every V-prefixed address through
DirectLogicAddress.UserVMemoryToPdu, a plain octal-to-decimal decode.
DL205/DL260 system V-memory (V40400 and up) is not a simple octal decode:
the CPU relocates the system bank to Modbus PDU 0x2100. Octal-decoding
V40400 produced 16640 (0x4100), the wrong register, so any tag addressing
a system register through the grammar string silently read/wrote the
wrong PLC memory.

- Add DirectLogicAddress.VMemoryToPdu, which decodes the octal V-address,
  detects the system bank (octal >= V40400 == SystemVMemoryOctalBase) and
  relocates it through SystemVMemoryToPdu to PDU 0x2100; user-bank
  addresses keep the plain octal decode.
- ModbusAddressParser's DL205 V branch now calls VMemoryToPdu instead of
  UserVMemoryToPdu. UserVMemoryToPdu is retained for user-bank-only callers.
- Correct the ModbusFamilyParserTests V40400 assertion (16640 -> 0x2100)
  and add system-bank regression cases plus direct helper coverage.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 06:59:39 -04:00
Joseph Doherty 532b961cf2 fix(driver-modbus): resolve High code-review finding (Driver.Modbus-001)
_lastPublishedByRef was a plain Dictionary<string, object> mutated inside
ShouldPublish, which runs on the PollGroupEngine onChange callback. The engine
runs one background Task per subscription, so a driver with two or more
subscriptions invokes ShouldPublish concurrently on separate threads. Concurrent
TryGetValue/indexer writes on a non-thread-safe Dictionary can corrupt internal
state, drop entries, or throw, crashing the poll loop.

Switch _lastPublishedByRef to ConcurrentDictionary<string, object>; its
TryGetValue and indexer-set operations are individually thread-safe, so the
deadband cache is now correct under concurrent multi-subscription publishing,
consistent with the lock-guarded sibling cache _lastWrittenByRef.

Add an xUnit + Shouldly regression test that runs 24 deadband-configured
single-tag subscriptions concurrently and asserts the poll loop survives without
faulting.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 06:59:39 -04:00
Joseph Doherty 7f2e144f8d fix(driver-galaxy): resolve High code-review findings (Driver.Galaxy-002, Driver.Galaxy-008)
Driver.Galaxy-002 — DataTypeMap.Map had no Int64 arm though MxValueDecoder/
MxValueEncoder both fully support Int64. Galaxy attributes with the Int64
mx_data_type code fell through to the String default, creating a String
address-space node while runtime reads decoded a boxed long. Added
`6 => DriverDataType.Int64`, extending the contiguous 0..5 scheme so the type
map agrees with the decoder/encoder on all seven Galaxy data types.

Driver.Galaxy-008 — after a stream fault the EventPump's StreamEvents consumer
loop exited and its channel completed; EventPump.Start() is a no-op on a
completed-but-non-null loop, so a replayed subscription had no consumer and
ReplayAsync never re-registered the post-reconnect item handles. ReplayAsync
now recreates the EventPump (RestartEventPumpForReplay) and rebinds the
SubscriptionRegistry per subscription with the fresh item handles returned by
the post-reconnect SubscribeBulkAsync, via new SubscriptionRegistry.SnapshotEntries
and Rebind APIs.

Regression tests: DataTypeMapTests (every code incl. Int64), SubscriptionRegistry
Tests (Rebind/SnapshotEntries), EventPumpStreamFaultTests (faulted pump dead,
fresh pump resumes dispatch).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 06:59:38 -04:00
Joseph Doherty c9e446387a fix(driver-focas): resolve High code-review findings (Driver.FOCAS-001, Driver.FOCAS-002)
Driver.FOCAS-001: FocasDriverConfigDto exposed no FixedTree / AlarmProjection /
HandleRecycle sections, so a deployment that opted into those features per
docs/drivers/FOCAS.md had the sections silently dropped by case-insensitive
JSON parsing and the features stayed at their disabled defaults. Added
FocasFixedTreeDto / FocasAlarmProjectionDto / FocasHandleRecycleDto and Build*
mappers in CreateInstance that populate the matching FocasDriverOptions
properties; a missing section or field keeps its existing default.

Driver.FOCAS-002: the fixed-tree bootstrap probe classified ProgramInfo as
"supported" whenever GetProgramInfoAsync returned non-null, but WireFocasClient
.GetProgramInfoAsync substituted defaults instead of throwing on a FOCAS error
return, so a CNC series answering EW_FUNC/EW_NOOPT for cnc_exeprgname2 /
cnc_rdopmode still got the Program/ and OperationMode/ subtrees. The method now
throws InvalidOperationException when neither the program-name nor the op-mode
read is IsOk, so SafeTryProbe correctly suppresses the capability.

Added FocasFactoryConfigTests covering the three opt-in config sections
round-tripping through CreateInstance and the fixed-tree bootstrap classifying
ProgramInfo as unsupported when the probe throws. Added an internal
FocasDriver.Options test seam.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 06:41:28 -04:00
Joseph Doherty ebc0511c72 fix(driver-opcuaclient): resolve High code-review findings (Driver.OpcUaClient-001..-005)
Driver.OpcUaClient-001 — ReadAsync/WriteAsync/DiscoverAsync captured the
session before acquiring _gate, so a reconnect that completed while the
operation was blocked on the gate left the wire call bound to a stale,
closed session. All three now re-read Session (and parse NodeIds) inside
the _gate critical section after WaitAsync returns.

Driver.OpcUaClient-002 — OnReconnectComplete ignored the give-up (null
session) case, permanently wedging the driver with no Faulted signal and
no reconnect loop. The give-up branch now transitions HostState to
Faulted, sets a Faulted DriverHealth with an explanatory message, and
re-arms a fresh SessionReconnectHandler (TryRearmReconnect) against the
last-known session so an always-on gateway self-heals.

Driver.OpcUaClient-003 — BrowseRecursiveAsync discarded browse
continuation points, silently truncating large remote folders.
It now loops on BrowseResult.ContinuationPoint calling BrowseNextAsync
and appending each page until the continuation point is empty.

Driver.OpcUaClient-004 — driver-specs.md §8 namespace handling was
absent. Added NamespaceMap (built from session.NamespaceUris at connect,
rebuilt on reconnect) which persists discovered NodeIds in the
server-stable nsu=<uri>;... form; reads/writes re-resolve that form
against the current session so a remote namespace-table reorder no
longer misaddresses nodes. Added the TargetNamespaceKind option +
UnsMappingTable and ValidateNamespaceKind startup enforcement.

Driver.OpcUaClient-005 — OnKeepAlive read/wrote _reconnectHandler
without a lock, racing the SDK keep-alive timer thread and leaking
handlers. The check-and-set in OnKeepAlive, the take-and-clear in
ShutdownAsync, and the dispose/re-arm in OnReconnectComplete now all
run inside the _probeLock critical section.

Adds OpcUaClientNamespaceTests (11 xUnit + Shouldly regression tests)
covering ValidateNamespaceKind and the NamespaceMap stable encoding.
Reconnect/browse wire paths remain fixture-gated per finding -015.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 06:41:28 -04:00
Joseph Doherty 090d2a4b44 fix(driver-s7): resolve High code-review findings (Driver.S7-001, -006, -007, -011)
Driver.S7-001: Timer (T{n}) / Counter (C{n}) addresses parsed cleanly but
the read path had no S7DataType or decode case for them, so a Timer/Counter
tag passed fail-fast init and then threw a misleading type-mismatch on every
read. InitializeAsync now runs RejectUnsupportedTagAddresses, throwing a clear
NotSupportedException ("not yet supported", echoing tag name + address) so the
config error fails fast at init.

Driver.S7-006: ShutdownAsync cancelled the probe/poll CTSs but did not await
the fire-and-forget loop tasks before DisposeAsync disposed _gate, letting a
loop iteration mid-semaphore race a disposed object. The probe task is now
tracked in _probeTask and each poll task in SubscriptionState.PollTask;
ShutdownAsync cancels every CTS, awaits Task.WhenAll of those handles with a
bounded 5 s DrainTimeout, then disposes the CTSs and gate. Task.Run is passed
CancellationToken.None so the handle is always awaitable.

Driver.S7-007: a PUT/GET-disabled fault (permanent misconfiguration) was
mapped identically to a transient PlcException — both BadDeviceFailure +
Degraded. ReadAsync/WriteAsync now split the catch via an IsAccessDenied
filter (S7.Net exposes no typed code for AccessingObjectNotAllowed, so the
inner-exception chain is inspected for the "not allowed" marker). Access-denied
now maps to BadNotSupported and Faulted with a config-alert message pointing
at the TIA Portal PUT/GET toggle; genuine device faults stay BadDeviceFailure.

Driver.S7-011: S7Driver ignored driverConfigJson on Initialize/Reinitialize,
so a config change delivered through ReinitializeAsync (the only Core-initiated
in-process recovery path) was silently discarded. Config parsing was factored
into S7DriverFactoryExtensions.ParseOptions; InitializeAsync now re-parses
driverConfigJson and rebuilds _options whenever the document has a real body.
An empty / placeholder document keeps the constructor options.

Adds S7DriverCodeReviewFixTests covering Timer/Counter rejection, config-json
application on Initialize/Reinitialize, and shutdown-drain with active
subscriptions. All 68 S7 driver tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 06:41:26 -04:00