[tylerkron]

Summary

WindowsUsbLocationProvider (added in #290) queries DEVPKEY_Device_LocationInfo directly on the PnP node HidDevicePathParser resolves a HID device interface path to — but that node (the HID collection's own PnP node) never carries the property. Only the physical USB device node one level up in the device tree does.

Confirmed on real bootloader hardware (a DAQiFi device forced into PIC32 HID bootloader mode): the bootloader's HID node returns no LocationInfo at all via WMI (not an exception — the property is simply absent), while querying its DEVPKEY_Device_Parent node returns the exact same value (Port_#0001.Hub_#0001) the same physical port reports in serial/CDC mode.

This silently broke location-key resolution for every real HID device — always null, no exception, no visible failure — which defeats the entire point of the cross-mode correlation daqifi-desktop#655 depends on (PR 692 there is the consumer). #290's own test plan explicitly flagged this exact case as unverified on real Windows hardware; this PR closes that gap.

Changes

• New LocationParentWalker: pure, WMI-independent walk-up logic (bounded to 4 hops to guard against a pathological/cyclic parent chain), unit tested with a fake query function — following the same "extract the pure logic" pattern HidDevicePathParser already uses.
• WindowsUsbLocationProvider now walks to DEVPKEY_Device_Parent and retries when the direct lookup on a node comes back empty.
• Two more bugs found and fixed while building/testing this against real hardware:
  • WMI's GetDeviceProperties throws ManagementException("Not found") when reading a property key it couldn't resolve on a given node — only surfaced once two keys (LocationInfo + Parent) were requested in the same call. Property reads are now defensive per-entry (TryGetStringProperty).
  • DEVPKEY_Device_Parent values come back from WMI with lowercase hex segments (e.g. 5&2c705bfe&0&1), which fails InstanceIdRegex (uppercase-only, by design, to safely interpolate into WQL) unless normalized before the next hop's query — without this the walk silently stopped one hop too early even with the parent-walk fix in place.

Test plan

• dotnet build Daqifi.Core.sln -c Release — 0 warnings/errors
• dotnet test Daqifi.Core.sln -c Release — 1391/1392 passed (1 pre-existing unrelated flake in ContinuousDeviceFinderTests, reproduces identically on main without this change)
• New LocationParentWalker unit tests (7 cases: immediate resolution, one-hop walk, lowercase-parent normalization, empty-string-treated-as-not-found, no-parent termination, malformed-parent termination, cyclic-chain bounded termination)
• Bench-validated on real hardware: built this branch's Daqifi.Core.dll, loaded it via reflection against the real WindowsUsbLocationProvider type, and confirmed the same physical DAQiFi device's location key resolves to the identical Port_#0001.Hub_#0001 in both serial mode (GetLocationKey("COM3")) and HID bootloader mode (GetLocationKey(rawHidDevicePath)) — before this fix, the HID-mode call always returned null.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Fix Windows HID USB location resolution by walking parent PnP nodes

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Resolve HID USB location by retrying on the parent PnP node when LocationInfo is absent.
• Harden WMI property reads and normalize parent instance IDs to keep parent traversal working.
• Add unit tests for parent-walk depth, termination, and normalization edge cases.

Diagram

graph TD
  A([Caller]) --> B["WindowsUsbLocationProvider"] --> C{"Input is COM?"}
  C -->|"Yes"| D{{"WMI caption lookup"}} --> G["LocationParentWalker"]
  C -->|"No"| E["HidDevicePathParser"] --> G
  G --> F{{"WMI device properties"}} --> H["Location key / null"]

  subgraph Legend
    direction LR
    _usr([Caller]) ~~~ _svc["Module/Service"] ~~~ _dec{"Decision"} ~~~ _ext{{"External/WMI"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use SetupAPI/ConfigManager APIs instead of WMI

• ➕ More direct access to devnode properties and parent relationships
• ➕ Avoids WMI-specific failure modes (e.g., per-entry ManagementException behavior)
• ➕ Potentially faster and more reliable on locked-down systems
• ➖ Requires P/Invoke and substantially more Windows-specific code
• ➖ Harder to unit test without abstraction layers
• ➖ Higher maintenance burden across Windows versions

2. Model the USB topology via WMI associations (USB controller/hub relationships)

• ➕ Could produce richer topology data than LocationInfo alone
• ➕ May avoid relying on DEVPKEY_Device_Parent formatting quirks
• ➖ More complex WMI queries and join logic
• ➖ Often less deterministic across device classes/drivers
• ➖ Still dependent on WMI availability/behavior

Recommendation: Keep the current approach: parent-walking is the smallest fix that matches real device-tree behavior for HID collections, and the new LocationParentWalker cleanly isolates traversal decisions for unit testing. If WMI continues to show fragility in the field, consider a follow-up migration to SetupAPI/ConfigManager with a similar testable abstraction boundary.

Files changed (3) +228 / -29

Bug fix (2) +123 / -29

LocationParentWalker.csIntroduce bounded parent-chain traversal for USB location resolution +66/-0

Introduce bounded parent-chain traversal for USB location resolution

• Adds pure logic to walk parent PnP instance IDs until a non-empty LocationInfo is found. Validates instance IDs to prevent WQL injection and normalizes parent IDs to uppercase to match expected conventions.

src/Daqifi.Core/Device/Discovery/LocationParentWalker.cs

WindowsUsbLocationProvider.csResolve LocationInfo via parent node and harden WMI property reads +57/-29

Resolve LocationInfo via parent node and harden WMI property reads

• Switches location resolution to use LocationParentWalker and query both LocationInfo and Parent in WMI. Adds defensive per-entry property access to handle WMI 'Not found' exceptions and ensures parent IDs can be walked reliably.

src/Daqifi.Core/Device/Discovery/WindowsUsbLocationProvider.cs

Tests (1) +105 / -0

UsbLocationProviderTests.csAdd unit tests for parent-walk location resolution and termination +105/-0

Add unit tests for parent-walk location resolution and termination

• Adds a focused test suite for LocationParentWalker using a fake query function. Covers immediate vs parent resolution, lowercase parent normalization, empty-string handling, malformed IDs, and hop-bounded cyclic chains.

src/Daqifi.Core.Tests/Device/Discovery/UsbLocationProviderTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Unvalidated WQL instance ID ✓ Resolved 🐞 Bug ⛨ Security

Description

WindowsUsbLocationProvider.QueryLocationByCaption takes the first WMI-returned DeviceID and passes
it into LocationParentWalker.Resolve without validating it against InstanceIdRegex, even though
QueryLocationAndParent interpolates that value into a quoted WQL string literal with no single-quote
escaping. If WMI returns an unexpected DeviceID shape, the WQL query can fail/throw and
GetLocationKey will swallow it and silently return null, breaking location correlation on the
COM-port path.

Code

src/Daqifi.Core/Device/Discovery/WindowsUsbLocationProvider.cs[R63-64]

+        var deviceId = FindDeviceId(searcher);
+        return deviceId != null ? LocationParentWalker.Resolve(deviceId, QueryLocationAndParent) : null;

Relevance

⭐⭐⭐ High

Team previously accepted WMI/WQL input validation+escaping hardening; similar safety fix likely
accepted here too.

PR-#201
PR-#290

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The COM-port caption path calls LocationParentWalker.Resolve(deviceId, ...) with a deviceId
obtained from WMI without any InstanceIdRegex check, while the HID path does validate before
walking. The walker documents that the initial ID must already match InstanceIdRegex, and the
follow-up query interpolates instanceId into a single-quoted WQL literal while only escaping
backslashes. Any resulting WMI/query exception is swallowed by GetLocationKey’s broad catch and
converted to a silent null result.

src/Daqifi.Core/Device/Discovery/WindowsUsbLocationProvider.cs[26-54]
src/Daqifi.Core/Device/Discovery/WindowsUsbLocationProvider.cs[57-65]
src/Daqifi.Core/Device/Discovery/WindowsUsbLocationProvider.cs[67-72]
src/Daqifi.Core/Device/Discovery/WindowsUsbLocationProvider.cs[74-80]
src/Daqifi.Core/Device/Discovery/LocationParentWalker.cs[35-41]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`QueryLocationByCaption` now returns the first `DeviceID` from WMI and immediately calls `LocationParentWalker.Resolve(deviceId, ...)`. Unlike the HID-device-path flow, this bypasses `InstanceIdRegex` validation. That same ID is later interpolated into a WQL string literal in `QueryLocationAndParent`, which escapes backslashes but not single quotes.

This weakens the intended “validate-before-interpolate” safety contract and can turn unexpected WMI data into a malformed WQL query that gets swallowed by `GetLocationKey`’s broad catch, yielding a silent `null` location.

### Issue Context
- `LocationParentWalker.Resolve(...)` explicitly documents that the starting `instanceId` must already match `InstanceIdRegex`.
- The HID path enforces this; the COM caption path currently doesn’t.

### Fix Focus Areas
- src/Daqifi.Core/Device/Discovery/WindowsUsbLocationProvider.cs[57-80]
- src/Daqifi.Core/Device/Discovery/LocationParentWalker.cs[35-41]

### Suggested change
1) In `QueryLocationByCaption`, normalize + validate the returned `deviceId` before calling `Resolve` (mirror `QueryLocationByDeviceId`):
  - `deviceId = deviceId.ToUpperInvariant();`
  - `if (!LocationParentWalker.InstanceIdRegex.IsMatch(deviceId)) return null;`

2) Defense-in-depth: in `QueryLocationAndParent`, also escape single quotes for WQL literals (e.g. `escaped = escaped.Replace("'", "''", StringComparison.Ordinal);`) before interpolation.

(Optionally, also enforce the contract inside `LocationParentWalker.Resolve` by validating the initial `instanceId` up front.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---