No defects found across all 10 checklist categories. NamespaceMap's
namespace-URI-stable NodeId encoding and v3 RawPath resolution table match
docs/v2/driver-specs.md §8 and are exercised by
OpcUaClientNamespaceTests.cs. Zero findings recorded.
No functional defects found. One Low/Open record-only finding: Inspect()
double-parses its TagConfig blob (once directly, once via TryReadTagConfig)
by deliberate design of the module's single-authority split — not pursued
as a fix since it is deploy-time-only (not a hot path) and widening
TryReadTagConfig's signature to avoid it touches consumers outside this
module (Driver.Calculation, AdminUI).
Code review of Driver.Calculation found two real defects, both TDD-fixed:
- CalculationDriver's ctor tried to reuse its ILogger<CalculationDriver> as the
evaluator's ILogger<CalculationEvaluator> via an `is` check that can never
succeed (unrelated closed generics), so CalculationEvaluator always ran with
a NullLogger regardless of what the driver was constructed with. Fixed by
having CalculationEvaluator accept the non-generic ILogger instead.
- EvaluateTag called the evaluator BEFORE checking whether every declared
dependency had arrived, so a multi-dependency change-triggered calc tag
evaluated its script against a partial snapshot on every warm-up tick.
VirtualTagContext.GetTag returns a Bad snapshot (.Value == null) for a
missing dep, so the idiomatic `(double)ctx.GetTag("x").Value` cast throws
NullReferenceException — caught and logged as a spurious "script threw"
script-log Warning on ordinary deploy/restart warm-up, for the standard
multi-input calc-tag shape. Fixed by gating evaluation itself on
"all deps arrived", not just the publish.
See code-reviews/Driver.Calculation/findings.md for full detail (5 findings:
001-004 Resolved, 005 Open/cross-module).
Driver.MTConnect: 8 findings — 1 High (Driver.MTConnect-003, browse-committed tags
are typed String and defeat the probe inference; cause is in AdminUI's
RawBrowseCommitMapper, so recorded Open with a recommendation), 3 Medium
(001/002 fixed in 6c63bf8f, 004 the missing write.enabled authoring surface,
cross-module Open), 4 Low. 5 remain Open, all cross-module or record-only.
Driver.MTConnect.Contracts: 1 finding — a Low documentation defect whose false
premise concealed Driver.MTConnect-003; fixed in 6c63bf8f. No behavioural issues:
the accept-gate-first Inspect contract, the FirstNonBlank alias precedence and the
digits-only enum pre-check were all verified as deliberate and correct at HEAD.
README.md deliberately not regenerated (out of scope for this review).
001 the Postgres (63) and Oracle (128) identifier ceilings are BYTE limits but are
enforced as UTF-16 char counts, so a non-ASCII name passes the check and is still
truncated server-side — the exact silent truncation the check documents itself as
preventing;
002 the seam carries no max-bind-parameter member, which is why the planner cannot
chunk a key-value IN list (Driver.Sql-002);
003 Oracle's ListSchemasSql reads ALL_USERS, justified only from the catalog gate's
point of view — the browse tree shares the query and renders every database user as
an expandable schema.
Quoting, marker/bind-name split, BindByName and the type maps are all sound.
001 keyValue is read string-only while whereValue accepts any scalar, and Inspect
then reports a numerically-authored key as ABSENT — an authoring surface
disagreeing with the runtime about why (the #549 class);
002 cross-module: TagConfigJson's strict enum read rests on Enum.TryParse, which
accepts a numeric string, so "type":"99" is Valid and an undefined
DriverDataType reaches the address space. Fix belongs in Core.Abstractions.
The accept-gate-first Inspect and the strict enum reads are otherwise exactly the
#549-#559 arc's rules, implemented structurally.
001 the #552 topByTimestamp default admits a date-only column, so the committed
wide-row read orders by a column that ties for every row written that day;
002 the session's gate is disposed under a racing call and LastUsedUtc only
advances on completion, so a long expand is invisible to the TTL reaper;
003 the ad-hoc pasted connection string is an arbitrary outbound-connect primitive
— correctly behind the /raw ConfigEditor policy, but undocumented as such.
No code changes: the NodeId codec, the bound-parameter catalog path and the
connection-ownership handoff are all sound. 79/79 tests unchanged.
All 10 checklist categories completed for both modules. 12 findings total:
1 High + 2 Medium fixed under TDD (6eace4bb, 31182137), 1 Medium fixed in the
browser, 8 Low recorded Open with concrete recommendations.
README.md deliberately NOT regenerated — regen-readme.py is left for the caller.
Driver.Sql-001 (Medium): SqlCatalogLoader.ReadDefaultSchemaAsync documented a
fallback to the sole visible schema when the server reports no default schema and
did not implement it, so a login with no DEFAULT_SCHEMA (or a Postgres search_path
that resolves to nothing) rejected EVERY unqualified authored table at the catalog
gate — a whole-driver BadNodeIdUnknown outage against a healthy database. Fixed;
SqlCatalogDefaultSchemaTests written red first, with a control proving the
ambiguous multi-schema case still refuses to guess.
Driver.Sql-003 (Medium): docs/drivers/Sql.md spelled the WideRow row selector at
the top level, but the parser reads those keys only inside a nested rowSelector —
a tag authored from the doc is refused and reads BadNodeIdUnknown (the #552 class,
via the documentation). Also removed the documented-but-nonexistent pollIntervalMs
per-tag field.
Driver.Sql-007 (Low): dropped the unused Microsoft.Data.SqlClient PackageReference
left behind by the .Dialect extraction (#526).
Open: 002 (no IN-list chunking vs the 2100/1000 provider bind caps), 004, 005, 006,
008, 009, 010.
Per-module review at b990adae (this branch's base): all 10 checklist
categories completed, 3 findings recorded and resolved in the two prior
commits on this branch (55b12dc2, a86a0976). Not run through
regen-readme.py per the review task's instructions - the aggregated
README.md is unchanged.
NamespaceMap relocation deferred: the Browser references only Contracts (not the
runtime driver), so the recommended move would break its compile; the only clean
relocation needs a new shared-helper project — disproportionate to a Low finding.
All 51 modules now have zero Open/In-Progress findings.
A rename-only deploy produced an IsEmpty plan that short-circuited before MaterialiseHierarchy,
leaving the OPC UA folder DisplayName stale. AddressSpacePlanner now diffs UnsAreas/UnsLines by
stable id into a RenamedFolders set (counted in IsEmpty); the applier refreshes the folder in
place via a new UpdateFolderDisplayName on ISurgicalAddressSpaceSink (forwarded through
DeferredAddressSpaceSink so it is NOT inert on driver hosts; falls back to rebuild when the sink
is non-surgical). DeploymentArtifact byte-parity untouched (rename rides the existing Name
round-trip). No EF migration, no serialized wire/proto contract change. +13 OpcUaServer tests, Runtime rebuild test.
- OpcUaServer-002: HistoryRead-Events NumValuesPerNode==0 now maps to unbounded (int.MaxValue) instead of the backend default-cap sentinel; no Core.Abstractions contract change (+EventMaxEvents helper tests)
- OpcUaServer-004: EnsureAddressSpaceCreated guard on public mutators -> clear InvalidOperationException instead of bare NRE if called pre-start (+tests)
- OpcUaServer-003: Deferred (endUtc inclusive/exclusive needs live Wonderware boundary confirmation)
- Configuration-013: wire DraftValidator.ValidateClusterTopology into AdminOperationsActor deploy gate (read-only, no migration) (+2 tests)
Cross-module fix from the review sweep. -018 (Medium): AbCipTagDto/AbCipMemberDto dropped
elementCount/isArray, so driver-config-authored array tags read as a single scalar. Added the
two optional JSON fields (additive; missing -> scalar as before) threaded into the tag def +
TDD.
Cross-module fix from the review sweep. -007 (Medium): OnTimedUnshelve built its AlarmCommand
with User=string.Empty, so Part9StateMachine.ApplyUnshelve rejected it (ArgumentException,
swallowed) and a TimedShelve never auto-expired. Pass the canonical 'system' user; the
AlarmAck-gate bypass is preserved. Repurposed the test that had encoded the bug.
Cross-module fix from the review sweep. -003 (Medium): the browser's JsonOpts lacked
JsonStringEnumConverter (the factory+probe both carry it), so AdminUI string-enum configs
(AuthType/SecurityPolicy/SecurityMode/TargetNamespaceKind) threw on deserialize. Added the
converter (accepts string AND numeric) + TDD.
First review at 7286d320. -001 (High): record ToString() leaked SharedSecret into logs ->
override ToString() that omits it. -003 (Medium): [Range(1,65535)] on Port. -004 thumbprint
doc. -002 (SHA-1 pin -> SHA-256, lives in FrameChannel) deferred cross-module.
First review at 7286d320. -001 (Low): ReadArrayLength doc corrected (positive int32 via
TryGetInt32, not uint). -002 (Structure datatype doc) Open. Consistent with the sibling
Driver.TwinCAT-017 ArrayLength flow.
First review at 7286d320. -001 (Medium): TryResolve session param -> ISession? matching the
null guard + out-contract doc. -003: [Range] on MaxDiscoveredNodes/MaxBrowseDepth, drop dead
[Display]. -002 (NamespaceMap pulls full SDK into a DTO project) Open. Surfaced cross-module:
the OpcUaClient.Browser serializer lacks JsonStringEnumConverter (enum-as-int bug).
First review at 7286d320. -002 (Medium) fixed: EventPumpChannelCapacity now [Range(1,..)]
(a 0 passed AdminUI validation but faulted the driver at InitializeAsync). -001 stale finding-id
doc fixed. -003 ResolveApiKey dedup Open (3-module coordination).
First review at 7286d320. -001 (Low) fixed: ArrayLength=1 doc corrected (it is a 1-element
array node, not scalar). -002/-003/-004 (parser edge doc, dead MaxTagBytes, ForFamily
fallback) Open.
First review at 7286d320. 6 findings (doc fixes -005/-006 resolved; -001 writable-hardcode,
-002 Dt-units doc, -003 dead [Display] attrs, -004 parser test gap left Open). Surfaced
cross-module: AbCipTagDto/AbCipMemberDto in the factory drop ElementCount/IsArray.
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).
Re-review at 7286d320. -006 (Low): FlushLogging() in all command finally blocks + tests.
-007: rewrite the inaccurate handler-detach comment (cleanup is via await using disposal).
Re-review at 7286d320. -008 (Low): all four commands now FlushLogging() in finally (parity
with AbCip.Cli; subscribe could drop shutdown log lines) + IL-inspection test.
Re-review at 7286d320. -009: 'four'->'six' driver-CLI count in Program.cs. -010: ReadCommand
--type help no longer lists Structure (rejected at runtime) + pinning test.
Joseph Doherty