dohertj2/lmxopcua
1
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases Wiki Activity

Residual TagConfig gate-coverage gaps found by the #549 survey #551

New Issue
Closed
opened 2026-08-09 22:20:16 -04:00 by dohertj2 · 1 comment
dohertj2 commented 2026-08-09 22:20:16 -04:00
Owner
Copy Link
Copy Source

Four residuals from the #549 fleet survey, none fixed there (each is a different rule than value-validity):

  1. RawCsvImportModal never calls TagConfigValidator — its typed columns are strictly parsed, but the free-form TagConfigJson column is unvalidated at authoring and meets only the deploy gate. Wiring it needs care: it could reject previously-importable files, and a rejected row must be an ERROR, not a skip.
  2. S7 hard-requires dataType to be PRESENT (S7TagDefinitionFactory rejects an absent key — unique in the fleet), but neither its Inspect nor the AdminUI model warns about absence — a hand-authored S7 blob without one passes both gates and dies at the driver.
  3. AbCip member-level dataType and Modbus stringByteOrder are strict at runtime but checked by neither Inspect nor any editor.
  4. Stale doc-comments: all seven pre-#549 Inspect XML summaries (and EquipmentTagConfigInspector class remarks) still say invalid enums are "silently defaulted by the lenient runtime" — false since R2-11 Phase C made the runtime reads strict (TryReadEnumStrict rejects the tag). The warnings are now describing rejections, not silent defaults.
Four residuals from the #549 fleet survey, none fixed there (each is a different rule than value-validity): 1. **`RawCsvImportModal` never calls `TagConfigValidator`** — its typed columns are strictly parsed, but the free-form `TagConfigJson` column is unvalidated at authoring and meets only the deploy gate. Wiring it needs care: it could reject previously-importable files, and a rejected row must be an ERROR, not a skip. 2. **S7 hard-requires `dataType` to be PRESENT** (`S7TagDefinitionFactory` rejects an absent key — unique in the fleet), but neither its `Inspect` nor the AdminUI model warns about absence — a hand-authored S7 blob without one passes both gates and dies at the driver. 3. **AbCip member-level `dataType`** and **Modbus `stringByteOrder`** are strict at runtime but checked by neither `Inspect` nor any editor. 4. **Stale doc-comments**: all seven pre-#549 `Inspect` XML summaries (and `EquipmentTagConfigInspector` class remarks) still say invalid enums are "silently defaulted by the lenient runtime" — false since R2-11 Phase C made the runtime reads strict (`TryReadEnumStrict` rejects the tag). The warnings are now describing rejections, not silent defaults.
dohertj2 commented 2026-08-10 00:26:21 -04:00
Author
Owner
Copy Link
Copy Source

FIXED — merged to master as 333ea488 (branch commits 05b42478 + 1ce6e8b1). All four items, plus the structural upgrade:

  1. Authoring validation is now enforced in the SERVICE — RawTreeService runs TagConfigValidator in CreateTagAsync, UpdateTagAsync AND ImportTagsAsync (the issue's premise that manual entry rides the import was wrong — it commits row-by-row via CreateTagAsync, which is why the gate landed on all three). The CSV review grid additionally surfaces per-row errors at the parse stage (RawTagCsvMapper). A rejected row is a named ERROR in the all-or-nothing result, never a skip. Falsified: removing the service check reddens exactly the service test while every mapper test stays green — the service is the guarantee, not the form.
  2. S7 absent-dataType now rejects at authoring (_dataTypeMissingInSource capture — judged on the KEY like the runtime's DeclaresDataType; a browse-commit-built model and any load→save round-trip stay clean) and warns at the deploy gate.
  3. AbCip members[] (non-object entry / missing name / typo'd member dataType, indexed) and Modbus stringByteOrder now checked at authoring and deploy.
  4. Doc sweep done — all "silently defaulted by the lenient runtime" comments rewritten to the strict reality (rejection → BadNodeIdUnknown), incl. EquipmentTagConfigInspector remarks and AdminOperationsActor.

Structural: all seven pre-#549 Inspects (Modbus/S7/AbCip/AbLegacy/TwinCAT/FOCAS/Mqtt — an unnamed sibling with the same gap) converted to accept-gate-first — parse accepts ⇒ silent; parse rejects ⇒ a warning must name why. That closed the three named gaps AND every unnamed one (absent/blank address keys fleet-wide, TwinCAT's valid-name-but-refused Structure, Mqtt's union-of-both-mappers gate), with a structural invariant test per factory: a blob the parse rejects can never draw zero warnings. Never-stricter preserved (Modbus's narrowing casts mirrored; AbLegacy's clamp deliberately not warned; FOCAS's accepted-blob advisories kept as a distinct class).

Verified on the merged tree: 12 suites / 5,098 tests green. Two findings filed as their own issues: Sql browse-commit emits a config the gate rejects (#552), and the CSV typed-column merge silently rewrites typo'd enums before any gate sees them (#553).

**FIXED** — merged to master as `333ea488` (branch commits `05b42478` + `1ce6e8b1`). All four items, plus the structural upgrade: 1. **Authoring validation is now enforced in the SERVICE** — `RawTreeService` runs `TagConfigValidator` in `CreateTagAsync`, `UpdateTagAsync` AND `ImportTagsAsync` (the issue's premise that manual entry rides the import was wrong — it commits row-by-row via `CreateTagAsync`, which is why the gate landed on all three). The CSV review grid additionally surfaces per-row errors at the parse stage (`RawTagCsvMapper`). A rejected row is a named ERROR in the all-or-nothing result, never a skip. Falsified: removing the service check reddens exactly the service test while every mapper test stays green — the service is the guarantee, not the form. 2. **S7 absent-`dataType`** now rejects at authoring (`_dataTypeMissingInSource` capture — judged on the KEY like the runtime's `DeclaresDataType`; a browse-commit-built model and any load→save round-trip stay clean) and warns at the deploy gate. 3. **AbCip `members[]`** (non-object entry / missing name / typo'd member `dataType`, indexed) and **Modbus `stringByteOrder`** now checked at authoring and deploy. 4. **Doc sweep** done — all "silently defaulted by the lenient runtime" comments rewritten to the strict reality (rejection → BadNodeIdUnknown), incl. `EquipmentTagConfigInspector` remarks and `AdminOperationsActor`. **Structural**: all seven pre-#549 `Inspect`s (Modbus/S7/AbCip/AbLegacy/TwinCAT/FOCAS/**Mqtt** — an unnamed sibling with the same gap) converted to **accept-gate-first** — parse accepts ⇒ silent; parse rejects ⇒ a warning must name why. That closed the three named gaps AND every unnamed one (absent/blank address keys fleet-wide, TwinCAT's valid-name-but-refused `Structure`, Mqtt's union-of-both-mappers gate), with a structural invariant test per factory: a blob the parse rejects can never draw zero warnings. Never-stricter preserved (Modbus's narrowing casts mirrored; AbLegacy's clamp deliberately not warned; FOCAS's accepted-blob advisories kept as a distinct class). Verified on the merged tree: 12 suites / 5,098 tests green. Two findings filed as their own issues: Sql browse-commit emits a config the gate rejects (#552), and the CSV typed-column merge silently rewrites typo'd enums before any gate sees them (#553).
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
auto-managed
queue management
 cross-driver
queue management
 driver/abcip
queue management
 driver/ablegacy
queue management
 driver/focas
queue management
 driver/mqtt
MQTT / Sparkplug B broker-subscribe driver
 driver/opcuaclient
queue management
 driver/s7
queue management
 driver/twincat
queue management
 phase/1
queue management
 phase/2
queue management
 phase/3
queue management
 phase/4
queue management
 phase/5
queue management
 phase/6
queue management
 queue/blocked
queue management
 queue/done
queue management
 queue/failed
queue management
 queue/in-progress
queue management
 queue/queued
queue management
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
dohertj2 (dohertj2)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: dohertj2/lmxopcua#551
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?