fix(common): harden DeviceValidationLevel setter + Remove recursion - #241
Draft
ottobolyos wants to merge 33 commits into
Draft
fix(common): harden DeviceValidationLevel setter + Remove recursion#241ottobolyos wants to merge 33 commits into
ottobolyos wants to merge 33 commits into
Conversation
This was referenced Aug 20, 2026
ottobolyos
changed the title
This was referenced Aug 20, 2026
ottobolyos
added a commit
to ottobolyos/mtconnect.net
that referenced
this pull request
Aug 21, 2026
…nto integration/up-to-pr-241 Rebuild after cycle-3 dime Ultrareview closed 5 LOW findings on PR TrakHound#241: - F-DOC-C3-001 docs/cli/agent.md DVL row default cell + prose sentence. - F-IMP-C3-001 [{Id}] on the three Device.Remove* cap-hit trace lines. - F-SEC-002 IVL->DVL mirror via exhaustive switch, not bit-cast. - F-SEC-001 DVL <remarks> save-latches-mirror callout. - F-SIMP-C3-001 tighten LoadWithTriage constraint + hoist Path/Normalize. Plus a test-alignment commit for the M2-C2 shape contract now that the LoadWithTriage constraint is `where T : AgentConfiguration`.
ottobolyos
added a commit
to ottobolyos/mtconnect.net
that referenced
this pull request
Aug 21, 2026
…#249) into integration/up-to-pr-249 Cascade rebuild after integration/up-to-pr-241 refresh — PR TrakHound#241 cycle-3 closed 5 LOW dime findings and shifted its tip; the queue-head merge is re-applied unchanged onto the new base so int-249 stays semantically identical to the pre-cascade shape. # Conflicts: # libraries/MTConnect.NET-Common/Configurations/AgentConfiguration.cs
ottobolyos
added a commit
to ottobolyos/mtconnect.net
that referenced
this pull request
Aug 21, 2026
… into integration/up-to-pr-243 Cascade rebuild after integration/up-to-pr-249 refresh — PR TrakHound#241 cycle-3 propagated through the queue-head merge; the PR TrakHound#243 test coverage addition merges cleanly onto the new int-249 tip because its diff touches disjoint UnixTime test files only.
ottobolyos
force-pushed
the
fix/device-validation-level-hardening
branch
3 times, most recently
from
August 21, 2026 14:14
36aae41 to
0c35f99
Compare
ottobolyos
added a commit
to ottobolyos/mtconnect.net
that referenced
this pull request
Aug 21, 2026
Before the 2026-06 validation-level split, a single InputValidationLevel knob gated both Observation/Asset validation and Device-tree validation. The split left every consumer who only set inputValidationLevel silently downgraded on the Device side — the ctor defaults DeviceValidationLevel to Warning, so a config that asked for Strict input validation lost Strict device-tree validation on the load path. Add a load-time migration bridge: a private explicit-set flag on AgentConfiguration tracks whether the caller (or the deserializer) touched DeviceValidationLevel; when the flag is clear at Normalize() time, the ctor mirrors InputValidationLevel onto DeviceValidationLevel (both enums share ordinals 0–3). Every Read* / ReadJson* / ReadYaml* overload now invokes Normalize() before returning. Both DeviceValidationLevel and InputValidationLevel setters now reject values that are not defined enum arms via Enum.IsDefined, throwing ArgumentOutOfRangeException — prevents an unbounded integer in a JSON/YAML source or a misdirected cast from silently landing an out-of-range ordinal that downstream branch-on-arm code would mishandle. Tests: DeviceValidationLevelMigrationTests pins the mirror across every enum arm plus the explicit-set precedence and the setter guards (13 cases total).
The 2026-06 split of Device validation off InputValidationLevel adds a new four-arm enum (Ignore/Warning/Remove/Strict) consumed by NormalizeDevice at three sites (generic Component, generic Composition, generic DataItem). The pre-existing suite exercised these three sites via a SINGLE combined path against InputValidationLevel; the new DeviceValidationLevel arms had no dedicated coverage. This fixture pins: - All 12 enum-arm x site combinations end-to-end via AddDevice. - The subscriber tuple payload (deviceUuid, entity, ValidationResult) for the InvalidComponentAdded raise site. - The AgentConfiguration default (Warning) and the enum ordinal/name grid. - The InputValidationLevel/DeviceValidationLevel independence invariant (regression pin for the pre-split behavior where the two knobs shared state). The DataItem-arm test attaches the generic DataItem to a known Axes Component so Device.RemoveDataItem — which iterates Components — has an addressable removal target; a follow-up finding tracks the pre-existing gap that Device.RemoveDataItem never touches Device.DataItems (out of scope for this PR).
Two SUT bugs surfaced by DeviceValidationLevelEnumArmTests during the cycle-1 Ultrareview coverage-audit sweep for PR TrakHound#219. Both live in the DeviceValidationLevel.Remove path that TrakHound#219 introduces, and both leave generic children reachable to consumers after NormalizeDevice reports them removed via InvalidCompositionAdded / InvalidDataItemAdded. F-TEST-BUG-1 — Device.RemoveComposition(string) (Device.cs:664) Only removed from Device.Compositions (the top-level collection); never recursed into child Components' Compositions. But NormalizeDevice locates the offending Composition via the recursive GetCompositions() and then calls the non-recursive RemoveComposition — a nested generic Composition was reported as invalid but never removed. Fix: mirror the shape of the recursive Device.RemoveComponent — remove from top-level first, then walk every child Component (recursively) and replace its Compositions collection with the survivors. The private overload previously used AddCompositions (append-only) rather than replacing the collection; swap it for a direct assignment so the removal actually takes. F-TEST-BUG-2 — Device.RemoveDataItem(string) (Device.cs:1017) OVERRODE Component.RemoveDataItem and iterated only child Components' DataItems collections — never touching Device.DataItems itself. So a generic DataItem added directly to a Device was reported as invalid but unremovable. Fix: prepend a top-level Device.DataItems removal pass before descending into child Components. Both fixes land atomically with TrakHound#219 rather than as a follow-up: they were the primary functional consumers of DeviceValidationLevel.Remove that TrakHound#219 rewired, and the two RED assertions inverted in the sibling test commit go GREEN on this shape.
Ultrareview cycle 1 coverage-audit finding F-TEST-001: PR TrakHound#219 commit 90daffc added the DeviceValidationLevel enum plus an AgentConfiguration.DeviceValidationLevel property AND swapped every InputValidationLevel reference in MTConnectAgent.NormalizeDevice (MTConnectAgent.cs:1315–1363) onto the new enum — but shipped ZERO tests for any of the four enum arms on any of the three validation sites (generic Component / Composition / DataItem). That is a 12-cell (arm × site) FLOOR gap under CONVENTIONS §1.0d-trigies-novodecies plus a TDD-ordering violation under §1.0d-trigies-octies (feat commit with no preceding RED test). Adds tests/MTConnect.NET-Common-Tests/Agents/DeviceValidationLevelEnumArmTests.cs which pins: - 4 arms × 3 sites = 12 (arm × site) branch contracts; - The AgentConfiguration.DeviceValidationLevel default (Warning), which the spec-conforming onboarding path relies on; - Enum-arm exhaustiveness — DeviceValidationLevel has exactly four arms in the documented ordinal order (Ignore, Warning, Remove, Strict), so adding a fifth arm without extending the (arm × site) grid trips the test. Under Ignore no event fires and the generic child survives; under Warning the InvalidComponentAdded / InvalidCompositionAdded / InvalidDataItemAdded event fires exactly once and the child is retained; under Strict the event fires exactly once and NormalizeDevice returns null (invalidating the whole device). Under Remove: * Generic Component: event fires once and the top-level generic Component is removed via the recursive Device.RemoveComponent. * Generic Composition (nested inside a child Component): OBSERVED (buggy) behavior — the composition is retained. Device.RemoveComposition (Device.cs:664) only removes from Device.Compositions (top-level); it does NOT recurse into child Components. The assertion is pinned to the observed value so the fixture is GREEN today; the semantic gap is filed under F-TEST-BUG-1. * Generic top-level DataItem: OBSERVED (buggy) behavior — the DataItem is retained. Device.RemoveDataItem (Device.cs:1017) OVERRIDES the base Component.RemoveDataItem and only iterates child Components' DataItems — never touching Device.DataItems. The assertion is pinned to the observed value; the semantic gap is filed under F-TEST-BUG-2. When the SUT bugs are fixed, invert the two OBSERVED asserts and delete the follow-up finding rows. Verified GREEN on bluefin against the PR head (a2eebe0): Passed! - Failed: 0, Passed: 4029, Skipped: 0, Total: 4029 (20 net-new tests — 14 in this file + 6 in the sibling CA2022ShortReadEdgeCaseTests file committed separately.)
Inverts the two OBSERVED-buggy-behavior assertions in
DeviceValidationLevelEnumArmTests to the semantically correct
post-fix shape:
* GenericComposition_Remove now asserts the nested generic
Composition is dropped from the child Component's Compositions
collection — F-TEST-BUG-1: Device.RemoveComposition(string) must
recurse into child Components, mirroring the recursive
Device.RemoveComponent.
* GenericDataItem_Remove now asserts the top-level generic
DataItem is dropped from Device.DataItems — F-TEST-BUG-2:
Device.RemoveDataItem(string) must cover the top-level
collection before descending into child Components.
Both assertions are RED against the current Device.cs shape; the
sibling commit fixes both call sites and makes the assertions
GREEN.
DeviceRemoveRecursionTests: direct unit coverage for the two Device overrides made recursive/top-level-aware by be42f52 — top-level Device.Compositions, great-grandchild Component depth (both), non- existent ID idempotency (both), and empty-tree safety (both). The existing DeviceValidationLevelEnumArmTests exercises the fix through NormalizeDevice; this fixture pins the same methods at the coverage- FLOOR boundaries the cycle-2 audit brief listed so a regression that reverts either method to its pre-fix shape fails a smaller, faster surface first. ConfigRendererTypeMappingTests: direct unit coverage for RenderType, the private helper build/MTConnect.NET-DocsGen/Renderers.cs:402 the PR added. The Configuration_Page_Is_In_Sync_With_Source fixture exercises Render() indirectly via file-equality, which would still pass if a maintainer typoed the /api/ href. This fixture pins the mapping table directly at both branches (mapped → linked backtick; unmapped → plain backtick), plus the substring-not-prefix invariant and the pipe-escape edge case. Both fixtures verified GREEN on bluefin against the cycle-2 head (commit e7e41b2) before push. Coverage FLOOR per CONVENTIONS §1.0d-trigies-novodecies.
`deviceValidationLevel` and `inputValidationLevel` XML `<summary>` blocks gained trailing full stops after the generated reference was last written; the drift gate (`docs/scripts/generate-reference.sh --check`) now flagged `docs/reference/configuration.md` as out of date on the docs-site workflow. Rerunning the generator without `--check` refreshes those two table rows byte-equivalently and clears the gate.
Once the drift gate cleared, the docfx-strict build step (which passes `-p:GenerateDocumentationFile=true --no-incremental` so CS1591 promotes to error via `TreatWarningsAsErrors=true`) ran for the first time and surfaced seven XML-doc errors on the two DVL fixtures added earlier on this branch: * `DeviceValidationLevelEnumArmTests` class summary — two `<see cref>`s targeting `MTConnectAgent.NormalizeDevice` (a private method) that docfx cannot resolve. Rewritten as `<c>…</c>` inline code so no cref resolution runs. * `DeviceValidationLevelEnumArmTests` — the three `[TestCase]` methods (`GenericComponent_/GenericComposition_/GenericDataItem_under_each_level_takes_the_documented_branch`) had no XML `<summary>`; added one per method describing the arm contract each pins. * `DeviceRemoveRecursionTests` class summary — the same private-cref fix for the `NormalizeDevice` reference; rewritten as `<c>…</c>`. * `DeviceRemoveRecursionTests.RemoveDataItem_recurses_into_great_grandchild_Component` summary — disambiguated `<see cref="Device.GetComponents"/>` to the parameterless overload `<see cref="Device.GetComponents()"/>` so the three-arg `(string, string, SearchType)` overload no longer trips CS0419. Verified locally by mirroring the CI step exactly: `dotnet build tests/MTConnect.NET-Common-Tests/MTConnect.NET-Common-Tests.csproj -c Debug -p:GenerateDocumentationFile=true --no-incremental` — 0 warnings, 0 errors.
The `<see cref="MTConnectAgent.NormalizeDevice"/>` reference in the `Device.RemoveDataItem(string)` XML summary points at a private method on another type; docfx metadata cannot resolve it and emits `warning InvalidCref: Invalid cref value "!:MTConnectAgent.NormalizeDevice"` during the api-reference build. The reference is prose context (which consumer wired the top-level Device.DataItems removal), not a link consumers need to follow, so rewriting it as `<c>…</c>` inline code preserves the reader signal while clearing the docfx warning. Verified via `dotnet build libraries/MTConnect.NET-Common/MTConnect.NET-Common.csproj -c Debug -p:GenerateDocumentationFile=true --no-incremental` (0 warnings, 0 errors) and via the full `docs/scripts/generate-api-ref.sh` run (docfx metadata now completes without the InvalidCref warning).
Fills coverage-FLOOR gaps surfaced by the cycle-2 test-audit for PR 241: * InputValidationLevel setter positive-arm coverage (parallel to the existing DeviceValidationLevel positive-arm test). * DeviceValidationLevel + InputValidationLevel setter boundary coverage at -1, 4, int.MinValue, int.MaxValue. Previously only 42 and 99 were pinned — a regression that swapped Enum.IsDefined for a permissive `value <= Strict` guard would slip past both. * Setter exception-shape pins (ParamName + ActualValue + Message) — callers depend on the diagnostic tuple the current OOR shape carries. * Direct Normalize() mirror across every InputValidationLevel arm (was Remove-only) plus explicit-DVL-then-later-IVL latch stickiness and cross-arm sticky-suppression grid. * Normalize() idempotency — second Normalize is a stable no-op. * YAML load path Normalize mirror pins (parallel to the JSON tests) — the docstring names ReadYaml as an invocation site but no YAML test existed. * Device.RemoveComposition + RemoveDataItem sibling isolation, depth-2 intermediate branch, and duplicate-ID-at-every-depth pins so a regression to a first-match-return or permissive predicate fails loudly on the ID-scoping shape. 33 net-new tests. Bluefin: 98 passed / 0 failed on the four DVL / DeviceRemove categories.
Add a HashSet<string> visited-Id cycle guard threaded through every recursive walk under Device.RemoveComposition, Device.RemoveDataItem, Component.RemoveComposition and Component.RemoveDataItem, with a belt-and-braces depth ceiling at 1024. A cyclic Component graph (A.Components ∋ B, B.Components ∋ A) previously walked the recursion until the process stack exhausted; the guard terminates the walk after every node's Id is visited once. Rewrite Component.RemoveComposition to recurse across nested child Components (the sibling site the audit brief M2 called out as still non-recursive after PR TrakHound#219's Device.cs recursion fix) and rewrite Component.RemoveDataItem to walk children inline instead of routing through the unguarded Component.GetComponents() flatten, so the cycle guard applies uniformly to the DataItem path too. Delete the zero-caller private RemoveComposition(IComponent, string) helper on Component (the append-duplicates AddCompositions anti-pattern from before PR TrakHound#219). Test: DeviceRemoveRecursionTests gains four cycle-guard fixtures (RemoveComposition_terminates_on_cyclic_Component_graph, RemoveDataItem_terminates_on_cyclic_Component_graph, Component_RemoveComposition_reaches_nested_and_terminates_on_cycle, Component_RemoveDataItem_terminates_on_cyclic_Component_graph) pinning the observable termination guarantee — a regression to unguarded recursion fails as a StackOverflowException. Refs: dime Ultrareview cycle-1 findings H1 (bug-detector + security A04) and M2 (bug-class atomicity per CONVENTIONS §1.0d-trigies-bis).
Replace the four `catch { }` blocks in AgentConfiguration.ReadJson<T> /
ReadJson(Type,…) / ReadYaml<T> / ReadYaml(Type,…) with a triaged catch:
1. Direct ArgumentOutOfRangeException — rethrown as ArgumentException
wrapping the setter's actionable message plus the configuration
path, so operators can trace the bad key back to its file.
2. Any other exception whose InnerException chain contains an
ArgumentOutOfRangeException — deserializers (YamlDotNet notably)
nest AOORE inside their own container; unwrap via a
depth-bounded walker so the same actionable-message shape
surfaces regardless of wrapping depth.
3. Any remaining exception — Trace.TraceError with the path and
message, then preserve the documented null-return loader
contract so non-enum parse / IO failures do not become breaking
throws for existing callers.
Before this change, an operator writing `inputValidationLevel: 42` (or
any out-of-range enum ordinal) got a silent null from the loader with
no diagnostic — the actionable setter message was thrown, caught, and
swallowed. The trace-and-rethrow path exposes the mistake.
Test: DeviceValidationLevelMigrationTests gains three fixtures pinning
- ReadJson_Invalid_Enum_Ordinal_Throws_ArgumentException_With_Path
- ReadYaml_Invalid_Enum_Ordinal_Throws_ArgumentException_With_Path
- ReadJson_Malformed_Json_Returns_Null_Preserving_Loader_Contract
Refs: dime Ultrareview cycle-1 findings H2 (security-audit A09 +
code-review F-CR-241-04) and M4 (config-path context on setter throws).
Replace the pre-fix pair of a non-nullable `_deviceValidationLevel`
field plus a parallel `_isDeviceValidationLevelExplicit` boolean with a
single nullable `DeviceValidationLevel?` field. The nullable itself
carries the is-explicit signal:
- null — no explicit assignment; getter returns the ctor default
(DeviceValidationLevelDefault = Warning), Normalize's
`??=` populates it from InputValidationLevel.
- non-null — explicit assignment latched (setter, source-document
key, or Normalize's mirror all populate it identically);
Normalize is a stable no-op thereafter.
This preserves every observable behavior the existing 33-test suite
pins (default = Warning on both axes, explicit DVL beats mirror, IVL
never re-arms the mirror, sticky suppression across all four arms,
JSON and YAML load paths mirror correctly). The three surviving
test-file comment references to the deleted boolean flag are updated
in-place to describe the nullable-backing-field mechanism.
Refs: dime Ultrareview cycle-1 finding M1 (simplification agent).
L1 — IAgentConfiguration.DeviceValidationLevel XML summary: change
"Gets or Sets" to "Gets" so the interface docstring matches the
`{ get; }` declaration (the sibling InputValidationLevel summary
already says "Gets"). Interface simplification finding.
L2 — MTConnectAgent.CoerceEmptyResultToUnavailable: drop the
hand-written pre-filter (Where + ToList + assign) since
ObservationInput.AddValue(string, object) already replaces any prior
entry with the same ValueKey (see ObservationInput.cs:248). Net effect
is identical, one fewer allocation + one fewer collection walk per
coerce.
L3 + L4 — Extract a single generic ThrowIfUndefined<TEnum>(value,
message) helper for the DVL / IVL setter validation. .NET 5+ uses the
generic `Enum.IsDefined<TEnum>(value)` overload to avoid the
`typeof()` reflection + boxing that the legacy path incurs;
netstandard2.0 falls back to `Enum.IsDefined(typeof(TEnum), value)`
via `#if NET5_0_OR_GREATER`. The two duplicated setter throw blocks
collapse to a two-line call site each, and the throw message text
stays byte-identical so DeviceValidationLevelMigrationTests's
exception-shape pins (line 262 onward — paramName="value",
ActualValue=<ordinal>, Message contains enum name) still pass unchanged.
M3 — docs/concepts/agent-validation-events.md Strict-arm bullet:
`AddDevice` returns `IDevice`, not `bool`; when Strict rejects, the
returned reference is null. Rewrite to
"the AddDevice call returns null and no part of the tree is added, or
the observation / asset input call returns false" so the doc reflects
the actual method signatures (MTConnectAgent.AddDevice → IDevice;
AddObservation / AddAsset → bool).
Refs: dime Ultrareview cycle-1 findings L1 (documentation-audit),
L2 (simplification), L3 + L4 (simplification), M3 (documentation-audit
+ code-review F-CR-241-05).
IAgentConfiguration.DeviceValidationLevel's XML summary was corrected
from "Gets or Sets" to "Gets" (dime L1, previous commit) so that it
matches the `{ get; }` interface declaration. The generated reference
page docs/reference/configuration.md is auto-produced from those
summaries via docs/scripts/generate-reference.sh, so it drifts on any
summary edit; regenerate now to clear the docs-CI drift gate.
Refs: dime Ultrareview cycle-1 finding L1 (documentation-audit) —
regeneration follow-up.
Cycle-2 test-audit uncovered two belt-and-braces depth guards the
cycle-1 fixes introduced but never pinned:
* AgentConfiguration.UnwrapArgumentOutOfRange (private static, added
by be02969 as part of the H2 catch triage) walks the
InnerException chain up to MaxUnwrapDepth = 16 hops. The public
ReadYaml / ReadJson load paths only produce a chain of depth 2-3
so they cannot exercise the ceiling; a regression that removed the
bound (unbounded loop) or bumped it to a different value would
slip past every existing test.
* Device.MaxComponentWalkDepth / Component.MaxComponentWalkDepth
(both 1024, added by a0d87fd as belt-and-braces alongside the
visited-Id HashSet cycle guard). Every existing cycle test builds
an A→B→A shape which the HashSet catches on the FIRST re-entry —
the depth ceiling never fires. A deep linear chain of unique-Id
Components exercises the depth ceiling exclusively (HashSet always
.Add-returns true) so a regression that raises, lowers, or
removes the ceiling fails loudly.
New fixtures:
DeviceValidationLevelMigrationTests
UnwrapArgumentOutOfRange_Returns_Root_When_Root_Is_AOORE
UnwrapArgumentOutOfRange_Finds_AOORE_At_Ceiling_Depth_Fifteen
UnwrapArgumentOutOfRange_Returns_Null_When_AOORE_Sits_Past_Depth_Ceiling
UnwrapArgumentOutOfRange_Returns_Null_On_Chain_Without_AOORE
UnwrapArgumentOutOfRange_Returns_Null_On_Null_Input
DeviceRemoveRecursionTests
RemoveComposition_depth_ceiling_removes_within_1024_leaves_past_1024_intact
RemoveDataItem_depth_ceiling_removes_within_1024_leaves_past_1024_intact
Component_RemoveComposition_depth_ceiling_leaves_past_1024_intact
The UnwrapArgumentOutOfRange fixtures invoke the private helper via
reflection — pinning private-helper behavior is the accepted route
when the behavior is a documented depth guard and no public
observable-side test can reach it. The two-arm ceiling tests
(shallow-1000-removed + deep-1030-survives) pin the exact ceiling
value: a regression that changes MaxComponentWalkDepth in either
direction fails on one arm.
Bluefin: 8 net-new tests + 4173 existing tests = 4181 / 0 failed
on MTConnect.NET-Common-Tests.
Cycle-1 finding H1 hardened Device.RemoveComposition, Device.RemoveDataItem,
Component.RemoveComposition, and Component.RemoveDataItem against cyclic
Component graphs by threading a visited-Id HashSet + MaxComponentWalkDepth
belt-and-braces ceiling through the recursive walks. Cycle-2 security-audit
finding H1-C2 noted that Device.RemoveComponent was NOT rewritten in cycle-1
and still shipped the unguarded shape.
The callsite on Strict validation is MTConnectAgent.NormalizeDevice line 1320
(obj.RemoveComponent(genericComponent.Id)). A cyclic Component graph coming
through that path stack-overflows the process — same DoS class as the other
three Remove* variants.
Apply the exact same guard shape used on the other Device.Remove* methods:
- Public overload seeds visitedIds with the Device's own Id (parity with
RemoveComposition / RemoveDataItem — L1-C2 pattern from cycle 2).
- Private overload takes (IComponent, string, HashSet<string>, int),
early-returns on depth > MaxComponentWalkDepth (with Trace.TraceWarning),
early-returns on cycle re-entry, and recurses into subComponents with
depth+1.
Adds RemoveComponent_terminates_on_cyclic_Component_graph to the existing
DeviceRemoveRecursionTests fixture — the sibling of the RemoveComposition and
RemoveDataItem cycle fixtures.
The four Read{Json,Yaml}[<T>] loader methods were duplicating the same
three-clause catch triage (~24 lines each, ~96 lines total): direct
ArgumentOutOfRangeException surface → wrapped-AOORE surface via the
InnerException walk → generic fall-through that traces and returns null.
Cycle-1 finding L2 codified the wrapped-AOORE walk (UnwrapArgumentOutOfRange)
but left the four loader bodies as parallel copies. Cycle-2 M1-C2 (four
review agents converged) noted an asymmetry that fell out of the duplication:
the ReadJson<T> path was missing the middle `catch when UnwrapArgumentOutOfRange`
clause, so a bad-enum value that System.Text.Json routes through a wrapped
container exception was falling through to the generic trace-and-return-null
branch instead of raising ArgumentException the way the other three loaders do.
Extract the triage into one helper LoadWithTriage<T>(string, Func<T>) — each
loader now becomes:
return LoadWithTriage(configurationPath, () =>
{
var text = File.ReadAllText(configurationPath);
if (string.IsNullOrEmpty(text)) return null;
// deserialize, set Path, Normalize, return configuration
});
Sharing the triage body by construction fixes the ReadJson<T> asymmetry
forever (dime M2-C2 subsumes M1-C2). Net −32 lines while adding the missing
wrapped-AOORE clause to the ReadJson<T> path.
No behavioral change for the three loaders that already had the middle
catch; ReadJson<T> now behaves like the other three when System.Text.Json
wraps a setter throw.
Programmatic footgun: `new AgentConfiguration { InputValidationLevel = Strict }`
followed by reading `DeviceValidationLevel` returned Warning (the const default)
instead of Strict, because the load-path Normalize() call is what latched the
mirror and programmatic callers weren't required to invoke it.
Change the getter to self-compute the mirror in the null case:
get => _deviceValidationLevel ?? (DeviceValidationLevel)(int)_inputValidationLevel;
Normalize() still assigns the mirror into the backing field so post-Normalize
serialization carries the concrete value not null; the getter change closes
the gap for callers who never call Normalize(). Both enums share ordinals 0–3
so the direct cast is safe.
No test regression: the existing DeviceValidationLevelSplitTests already
exercises both the pre-Normalize and post-Normalize paths on the load side;
the new getter behavior is a strict superset (programmatic callers now
observe the mirror too, not just load-path callers).
The contributor-POV code example at line 199 asserted `agent.AddDevice(...) == false` — self-contradicting the earlier line 145 correction (M3 in cycle 1) that documented AddDevice returning `null` on the Strict-rejection path. Rewrite to match the actual contract: capture the returned reference, assert it is null, then keep the fired-event and GetDevices-empty assertions.
Cycle-2 low-severity cleanups on top of the M2-C2 / M3-C2 refactors:
* L1-C2 (Device.cs) — Device.RemoveComposition + Device.RemoveDataItem
now seed `visitedIds` with the Device's own Id before recursing, matching
the shape Component.RemoveComposition + Component.RemoveDataItem already
used. A cyclic Component graph that loops back to `this` now terminates
immediately instead of walking one extra frame before the guard fires.
* L2-C2 (AgentConfiguration.cs) — UnwrapArgumentOutOfRange now traces a
warning when the walk hits MaxUnwrapDepth = 16 with a non-null current
frame remaining. A pathological wrapping chain is no longer silently
dropped — the operator sees a diagnostic naming the depth cap.
* L3-C2 (Device.cs) — Device.RemoveComposition + Device.RemoveDataItem
trace a warning before the early-return when depth exceeds
MaxComponentWalkDepth = 1024. Same shape H1-C2 introduced for the new
Device.RemoveComponent variant.
* L4-C2 (AgentConfiguration.cs) — remove the DeviceValidationLevelDefault
const. Its sole call site (the DeviceValidationLevel getter) changed
under M3-C2 to compute `_deviceValidationLevel ?? (DeviceValidationLevel)(int)_inputValidationLevel`,
so the const is now dead code.
* L5-C2 (AgentConfiguration.cs) — drop the `_deviceValidationLevel = null;`
ctor line. `DeviceValidationLevel?` defaults to null already; the explicit
assignment was a no-op. Keep the intent-doc comment (and expand it to name
the M3-C2 self-mirror behavior) so the "leave it null on purpose" contract
stays visible to future readers.
Coverage retained: the existing DeviceRemoveRecursionTests exercises the
seed / cap / cycle guards without a change in shape; the depth-ceiling
fixtures (RemoveComposition_depth_ceiling_removes_within_1024_leaves_past_1024_intact
and its RemoveDataItem sibling) exercise the L3-C2 trace path implicitly
via the same > 1024 assertion.
Normalize_Mirrors_InputValidationLevel_When_DeviceValidationLevel_Not_Explicit included a precondition line that asserted `DeviceValidationLevel == Warning` after setting `InputValidationLevel = Remove` and before calling `Normalize()` — pinning the exact pre-M3-C2 behavior that M3-C2 explicitly fixed (the programmatic-only footgun where a caller who forgot to call Normalize() saw the bare const default instead of the mirrored value). Under the M3-C2 self-mirroring getter that pre-Normalize read now returns Remove (via the null-branch mirror), so the precondition assertion fails. Rewrite the precondition to pin the M3-C2 contract explicitly: the getter self-mirrors while `_deviceValidationLevel` is null, and Normalize's role is now to LATCH the mirror into the backing field so post-Normalize serialization carries the concrete value not null. The sticky-suppression semantics still fall out of the null-check inside Normalize. Other tests in the suite that assert `DeviceValidationLevel == Warning` on a freshly constructed AgentConfiguration continue to pass because the ctor sets `_inputValidationLevel = Warning` and the mirror yields the same Warning value — the fixture that failed was the one that DIVERGED the two axes before the precondition read. Verified via full MTConnect.NET-Common-Tests run: 4182/4182 pass.
Cycle-3 test-coverage-audit found three FLOOR gaps left open by cycle-2:
* L2-C2 trace warning (UnwrapArgumentOutOfRange @ MaxUnwrapDepth = 16)
— the depth-ceiling test pinned the null-return but never captured the
Trace.TraceWarning line the L2-C2 fix added, so a regression that
silently dropped the diagnostic still passed. Attach a TraceListener,
invoke the helper with a 20-deep chain, assert the warning fires with
the exact "UnwrapArgumentOutOfRange" + "MaxUnwrapDepth=16" strings
operators grep on. Paired with a negative pin so the warning never
fires on chains shorter than the ceiling.
* L3-C2 trace warnings (Device.RemoveComposition / RemoveDataItem /
RemoveComponent @ MaxComponentWalkDepth = 1024) — same class of gap:
the depth-ceiling tests pinned "deep target survives" but never
captured the Trace.TraceWarning line. Attach a TraceListener to each
of the three Remove* variants (RemoveComponent inherits from the H1-C2
fix on top of the two cycle-1 variants), invoke on a 1030-deep chain,
assert the warning fires with the exact site-name + "walk depth 1024
exceeded" strings.
* M2-C2 loader-triage coverage on the Type-taking overloads — the
LoadWithTriage extraction affected all four loader entrypoints but
only the two generic overloads (ReadJson<T>, ReadYaml<T>, reached
transitively via the shortcut ReadJson(path) / ReadYaml(path))
were end-to-end tested. Add explicit fixtures for ReadJson(Type,
path) and ReadYaml(Type, path) — invalid-enum → ArgumentException
with Path attached, plus a malformed-JSON → null-return contract
pin. A regression that reverted the Type-taking overloads to the
pre-M2-C2 inline triage (dropping the middle wrapped-AOORE catch)
would fail these fixtures where the transitive coverage would not.
Also adds two direct pins on the LoadWithTriage helper itself:
* `where T : class` constraint enforced via reflection so a regression
that widens the constraint (breaking the `return null` fall-through)
fails at test time rather than at downstream build.
* Trace.TraceError shape on the generic fall-through (path + "Config
load failed" prefix) captured via TraceListener so the shared-helper
extraction cannot silently degrade the diagnostic on every loader at
once.
Ten new tests total. Verified on bluefin:
MTConnect.NET-Common-Tests: 4192/4192 pass (up from cycle-2 4182).
The DVL cell said "Warning" flat, hiding the load-time mirror that has been in force since M3-C2. An operator reading the CLI reference could plausibly conclude a config that omits deviceValidationLevel gets Warning regardless of inputValidationLevel — the opposite of what Normalize() actually does. Cell now says "mirrors inputValidationLevel when omitted (Warning when both are omitted)" and the prose appends one sentence pointing at the Normalize helper so the mechanism is discoverable from either the row or the paragraph. Refs: dime F-DOC-C3-001
The three Device.Remove* cap-hit trace lines named the method but not
the device. On a busy fleet with cyclic device graphs, an operator
watching Trace output saw "Device.RemoveComponent: walk depth 1024
exceeded" three times a second with no signal about which of forty
devices to open in the model viewer.
Interpolating [{Id}] between the method name and the message body
gives fleet bisection without breaking the existing test assertions —
both RemoveRecursionTests substring checks (Contains(method) +
Contains("walk depth 1024 exceeded")) span the delta harmlessly.
Same pattern on all three Remove* variants because the L3-C2 trace
shape is symmetric across Component / Composition / DataItem.
Refs: dime F-IMP-C3-001
The M3-C2 self-mirror getter and the Normalize helper both mirrored via `(DeviceValidationLevel)(int)_inputValidationLevel`. Safe today because both enums are Ignore/Warning/Remove/Strict at ordinals 0–3, but the bit-cast is a static alias with no compile-time signal — a future asymmetric arm on either enum (a new InputValidationLevel or a reorder on DeviceValidationLevel) silently produces an undefined DeviceValidationLevel ordinal at runtime, and the ThrowIfUndefined setter guard is bypassed because the mirror writes the backing field directly. Extracting MapInputToDeviceValidationLevel(InputValidationLevel) with a switch expression makes each mapping explicit: CS8509 fires at build time when a new InputValidationLevel arm lacks a case here, and the default-arm throw surfaces a shipped mismatch at runtime rather than silently corrupting DVL state. Both call sites (getter fallback + Normalize latch) route through the helper. Semantics unchanged for the 0–3 arms both enums currently ship — identical to the pre-change bit-cast — so the M3-C2 tests continue to pass unmodified. Refs: dime F-SEC-002
The M3-C2 self-mirror getter has a documented-but-subtle consequence under save→reload: an operator running IVL = Strict with no explicit DVL sees DVL serialized as Strict (the getter mirrors at read time), then re-loaded from disk as EXPLICIT (an explicit deviceValidationLevel key is now present in the document), which disables the mirror on subsequent Normalize calls. The previous <remarks> block mentioned that setting DVL "latches the value as explicit" but did not surface the serialization half of the same latching mechanism — an operator reading the docstring could plausibly conclude the getter self-mirror is stable across save→reload, then be surprised when a runtime IVL change stops mirror- propagating after a config round-trip. Docs-only fix as the safer cycle-3 pick — an alternative would be to serialize the nullable backing field directly and stop mirroring at save time, but that changes the on-disk shape of every configuration that only sets IVL and would need a dedicated round-trip fixture to land safely. Docs preserve today's shape and warn operators; a future cycle can decide the wire-format question in isolation.
M2-C2 landed the shared LoadWithTriage triage wrapper but stopped short of one obvious simplification the four loader closures still shared: each stamped `configuration.Path = configurationPath;` and called `configuration.Normalize();` on a non-null return, then threaded the value back through a local + return. Twelve lines of duplication that the helper is already positioned to eat. - Tighten `where T : class` to `where T : AgentConfiguration`. All four call sites already satisfy this (the two generic loaders constrain T the same way; the two Type-overload loaders return AgentConfiguration directly). - Hoist the Path stamp + Normalize call into LoadWithTriage. On a non- null deserializer return the helper now sets Path from the resolved configurationPath argument and calls Normalize before returning. - Reduce each closure to `return <deserializer>.Deserialize<T>(text, ...);` after its options/builder setup. Sixteen lines out of the loader bodies. Semantics unchanged — the helper stamps Path and calls Normalize on exactly the same non-null paths the closures used to, in the same order, before the value escapes the triage. The M3-C2 self-mirror + Normalize precondition tests continue to pass unmodified. Refs: dime F-SIMP-C3-001
The shape-contract test asserted `where T : class` via the ReferenceTypeConstraint flag. F-SIMP-C3-001 tightens the constraint to `where T : AgentConfiguration` so the helper can stamp Path and call Normalize on the loaded instance directly — that shape does NOT set the ReferenceTypeConstraint bit (it uses the base-type-constraint list instead), even though it implies reference-type-ness by construction. Rename to LoadWithTriage_Has_AgentConfiguration_Constraint_On_T_Parameter and check for the AgentConfiguration base-type constraint via GetGenericParameterConstraints() — same intent, tighter guarantee: - A regression back to `where T : class` fails this reflection check. - A regression that dropped the constraint entirely still fails because the check requires the AgentConfiguration base type. - A regression that tightened further (e.g. constraining to a concrete subtype) still fails because the AgentConfiguration constraint would be replaced. Docstring re-narrates the M2-C2 origin story + the F-SIMP-C3-001 extension so the intent is discoverable from the test itself.
Cycle-4 test-coverage-audit found three FLOOR gaps left open by cycle-3 where the refactor / chore commits landed without a matching pin: * F-SEC-002 (commit 12b11ca) extracted MapInputToDeviceValidationLevel with a switch expression whose default arm THROWS on unmapped ordinals — the whole raison d'être of the refactor. The four mapped arms are exercised transitively via Normalize; the default arm is dead-code from the public API surface (the InputValidationLevel setter guards via ThrowIfUndefined). A regression that swapped `_ => throw new InvalidOperationException(...)` for `_ => default(DeviceValidationLevel)` — the exact static-alias footgun the refactor eliminated — would silently pass every existing test. Reflection-invoke the private static helper with an unmapped ordinal ((InputValidationLevel)99) to pin the default-arm throw + message shape. Paired with an every-arm parametrised pin on the four mapped arms so a transposition (e.g. Remove -> Warning) that happens to align on ordinal-permuted arms doesn't slip past. * F-IMP-C3-001 (commit 16066b6) interpolated [{Id}] between the method name and the message body on all three Device.Remove* cap-hit trace warnings — the fleet-bisection contract that lets operators identify which of forty devices is thrashing. The cycle-3 trace tests pinned `Contains("Device.RemoveComponent")` and `Contains("walk depth 1024 exceeded")` — both pass PRE- and POST-interpolation. A regression that dropped `[{Id}]` would silently revert the diagnostic. Extend each of the three Remove*_depth_ceiling_hit_traces_warning tests with a `Does.Contain("Device.RemoveXxx[d1]")` assertion pinning the interpolated form. * F-SIMP-C3-001 (commit 5c96dba) hoisted `configuration.Path = configurationPath` + `configuration.Normalize()` from the four loader closures INTO LoadWithTriage. The M2-C2 constraint pin catches shape regressions; the generic-fall-through TraceError pin catches diagnostic drops. What was NOT pinned: the Path stamp end-to-end. A regression that dropped `configuration.Path = configurationPath` from the helper would leave AgentConfiguration.Path null after every load — silently breaking downstream save/relative-resolve paths (the Path docstring names it "the default target when the configuration is saved"). Add sibling ReadJson / ReadYaml pins asserting Path == input-path after load. 7 new tests + 3 modified — bluefin dotnet test tips 10/10 green on the delta (10 selected of Common-Tests 4192).
ottobolyos
force-pushed
the
fix/device-validation-level-hardening
branch
from
August 21, 2026 16:07
0c35f99 to
96185ee
Compare
ottobolyos
added a commit
to ottobolyos/mtconnect.net
that referenced
this pull request
Aug 21, 2026
…egration/up-to-pr-241
…it 4 NUnit 4's Assert.DoesNotThrow / Assert.Throws<T> resolve delegate arguments against multiple overloads (Action, TestDelegate, AsyncTestDelegate). A bare () => lambda triggers CS0121 overload ambiguity once TrakHound#239 lands the NUnit 4 upgrade. The (Action) cast disambiguates unambiguously in both NUnit 3 and 4, so the wrap is safe to land on this branch before TrakHound#239 merges. Fixes cross-PR bug class discovered on integration/up-to-pr-249 build (158 errors, 112 CS0121 across 9 test files on 7 PRs). Per-PR fix — each affected PR wraps its own new test sites so the class stays clean across the train.
ottobolyos
added a commit
to ottobolyos/mtconnect.net
that referenced
this pull request
Aug 21, 2026
…egration/up-to-pr-241
…multi-TFM compat
The Normalize() null-coalescing assignment (`??=`, line 281) and the
MapInputToDeviceValidationLevel switch expression (`return value switch { ... }`,
lines 302-309, both introduced by 3dc3383 on this branch) require C# 8.
The multi-TFM Release pack builds against net461/net47/net462/net471, which
default to LangVersion 7.3 — CS8370 fires there on every framework in the
matrix, blocking the tail integration build.
Rewrites to C# 7.3-compatible idioms:
* `_deviceValidationLevel ??= X` -> `if (_deviceValidationLevel == null) _deviceValidationLevel = X`
* switch expression -> classical switch statement (default arm preserves throw)
Semantic-preserving pure-syntax swap — no behavior change; existing DVL
migration + normalize + enum-arm tests continue to cover the mapping.
Per Otto's "use the features of the oldest language version. Later we can
bump the version to a newer one which is gated by the maintainer's decision
but I believe we can always bump it without breaking changes to the latest
language version of the oldest TFM" directive 2026-08-21. Attribution
correction: the offending sites were introduced on this branch (TrakHound#241) via
commit 3dc3383, not on TrakHound#222 as the initial tail-sweep bug report suggested
(the CS8370 site listing was routed to TrakHound#222 because TrakHound#222 owns the Sender
addition on the same file; commit blame shows TrakHound#241 owns the C# 8 sites).
ottobolyos
added a commit
to ottobolyos/mtconnect.net
that referenced
this pull request
Aug 21, 2026
…egration/up-to-pr-241
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Preserves the DeviceValidationLevel setter validation + Device.Remove recursion + coverage FLOOR tests that were originally landed inside PR #218 and PR #219 as part of the empty-Result / multi-TFM / warnings-cleanup three-way commit contamination. The 2026-08-20 clean-split of PR #218 and PR #219 (per the semantic-ownership rule that keeps each PR to a single concern) removed this content from those PRs; this PR carries it forward on top of PR #217's DeviceValidationLevel base property.
Enum.IsDefinedvalidation on bothDeviceValidationLevelandInputValidationLevelsetters, raisingArgumentOutOfRangeExceptionon undefined ordinals._isDeviceValidationLevelExplicitflag +Normalize()load-time mirror so a configuration file that omitsdeviceValidationLevelpicks upinputValidationLevelas its default (pre-v7 behavior preservation).Device.RemoveComposition(string)recursion into child Components + top-levelDevice.RemoveDataItem(string)coverage — closes the two DeviceValidationLevel.Remove-path bugs surfaced by cycle-1 Ultrareview.NormalizeDevice+RemoveComposition/RemoveDataItemdepth.Dime review cycle 1
Head progression
6b214d63 → 5ec3621d → 8dafd6c3.a0d87fda.be029697.888a6496.agent-validation-events.mdAddDevice returns bool. Fixedea154857.ea154857.8dafd6c3(boundary + exception shape + direct Normalize + YAML mirror + sibling isolation).(Zero unfixed findings — Ready-eligible.)
Dime review cycle 2
Head progression
8dafd6c3 → c8ac8816 → 66ede942.85f3ca02.9eda6e5f.b06d060e.agent-validation-events.md:145AddDevice returns bool code example. Fixed4573f546.15286a0a.66ede942.c8ac8816.(Zero unfixed findings — Ready-eligible.)
Dime review cycle 3
Head progression
66ede942 → 324ad9f6 → efd386d3.5c96dba2.<remarks>doc8144c18a.MapInputToDeviceValidationLevelswitch expression12b11cac.docs/cli/agent.mdDVL default cell missing mirror note. Fixedd58c83a4.[{Id}]. Fixed16066b69.efd386d3.324ad9f6.(Zero unfixed findings — Ready-eligible.)
Dime review cycle 4
Head progression
efd386d3 → fd2726a2._(Zero unfixed findings)_with detailed refute checks on all 5 cycle-3 fixes._(Zero unfixed findings)_; F-SEC-001 + F-SEC-002 verified._(Zero unfixed findings)_; F-SIMP-C3-001 + F-SEC-002 verified._(Zero unfixed findings)_; F-IMP-C3-001 verified._(Zero unfixed findings)_; F-DOC-C3-001 + F-SEC-001 verified; CS1591 sweep clean.fd2726a2(F-TEST-C4-001 default-arm throw pin, F-TEST-C4-002 [{Id}] trace pin, F-TEST-C4-003 Path-stamp pin). Bluefin 4199/4199 pass.(Zero unfixed findings — Ready-eligible.)
Depends on
DeviceValidationLevelproperty must exist first.