--- phase: 15-wifi-ddp-fallback-and-transport-selection fixed_at: 2026-04-19T00:00:00Z review_path: .planning/phases/15-wifi-ddp-fallback-and-transport-selection/15-REVIEW.md iteration: 2 findings_in_scope: 10 fixed: 10 skipped: 0 status: all_fixed --- # Phase 15: Code Review Fix Report **Fixed at:** 2026-04-19 **Source review:** `.planning/phases/15-wifi-ddp-fallback-and-transport-selection/15-REVIEW.md` **Iteration:** 2 (combined record of iteration 1 + iteration 2) **Summary:** - Findings in scope: 10 (1 Critical + 4 Warning + 5 Info) - Fixed: 10 (iteration 1: CR-01, WR-01..WR-04; iteration 2: IN-01..IN-05) - Skipped: 0 Iteration 1 addressed the Critical + Warning findings under `fix_scope=critical_warning`. Iteration 2 expanded scope to `all` and applied the five Info-level fixes. Each fix was verified in three tiers: (1) re-read of the modified region to confirm the patch landed and surrounding code is intact, (2) full `cmake --build build --config Release` with zero new `error C####` / `fatal` diagnostics (the only warnings emitted are pre-existing Windows SDK `C4005 SERIAL_IOC_*` macro-redefinitions from `Ntddser.h` vs `winioctl.h`, unchanged by this phase), and (3) n/a — MSVC is the authoritative syntax checker for C++ on Windows. For `deploy-backglow-dev.ps1` the PowerShell parser accepts the file (`-File` invocation fails only at the downstream execution-policy signature check, which is an environment policy, not a parse error). Logic-bug caveat (per fixer contract): none of the ten findings introduced new runtime logic that requires human semantic confirmation. CR-01 adds pure synchronisation with no behavioural change on the happy path. WR-01 is a comment-only safety note. WR-02 constrains an existing string scan to the first REG_MULTI_SZ entry (matches documented intent). WR-03 initialises a variable to the pessimistic branch that was already the intended behaviour. WR-04 promotes a silent failure into a user-visible warning without changing the success path. IN-01 corrects a 4-bit DDP sequence wrap so value 15 is included (benign spec-compliance tweak — WLED duplicate-detection window grows from 14 to 15 values). IN-02 adds a size guard matching the lambda's documented invariant. IN-03 parameterises developer paths without altering the copy-to-Steam behaviour. IN-04 tightens WLED response parsing by skipping past HTTP headers before the substring scan (WLED never emits a header that would collide, so behaviour is unchanged in practice). IN-05 is a cosmetic `%-9s` format-string tweak that preserves current "port"/"host" alignment while future-proofing longer labels. --- ## Fixed Issues ### CR-01: Data race — hotplug thread writes shared state without synchronization **Files modified:** `src/driver/device_provider.h`, `src/driver/device_provider.cpp` **Commit:** `f6a222c` **Iteration:** 1 **Applied fix:** - Added `#include ` and a new `std::mutex m_backglowMtx` member to `DeviceProvider` (documented as guarding `m_backglowDisabled`, `m_backglowDisabledReason`, `m_backglowPort`, `m_pLedController`). - Acquired `std::lock_guard lk(m_backglowMtx)` at the top of `HandleBackglowCommand` (before the `status`-verb shortcut that reads all four guarded fields) so every command path executes under the lock. - Acquired the same lock in `OnHotplugArrival()` immediately after the 500 ms debounce `Sleep` and before any read/write of the guarded fields — kept the debounce outside the critical section so the main thread is not blocked for half a second on every hotplug event. ### WR-01: SetupAPI HDEVINFO leak on scan failure path **Files modified:** `src/led/com_port_scan.cpp` **Commit:** `8accb55` **Iteration:** 1 **Applied fix:** Added a prominent block comment directly above the enumeration `for`-loop documenting the invariant that `SetupDiDestroyDeviceInfoList(hDevInfo)` must be reached on every exit path, and instructing future maintainers to use `continue` (never `return`) inside the loop or else add explicit Destroy calls / an RAII wrapper. The existing code already routes all failure paths through `continue`, so this is a comment-only guard against regression (matches the reviewer's recommendation that the current code is correct but fragile). ### WR-02: `SetupDiGetDeviceRegistryPropertyA` multi-string semantics **Files modified:** `src/led/com_port_scan.cpp` **Commit:** `a72fa5f` **Iteration:** 1 **Applied fix:** Replaced the `std::string upper(hwBuf, hwSz ? hwSz : ...)` constructor (which copies the entire REG_MULTI_SZ buffer including embedded NULs and could spill a substring match into trailing hardware-ID strings) with `std::string upper(hwBuf)`, which stops at the first `\0` and so matches only the most-specific (first) hardware ID. Eliminates the false-positive risk called out by the reviewer and aligns with the intent of the VID/PID check. ### WR-03: `RunLighthouseCommand` uninitialized `exitCode` read **Files modified:** `src/driver/device_provider.cpp` **Commit:** `657c581` **Iteration:** 1 **Applied fix:** Changed `DWORD exitCode;` to `DWORD exitCode = STILL_ACTIVE;` inside the IPD-persist poll loop. If `GetExitCodeProcess` fails on the first call, the post-loop compare now falls through to the pessimistic `TerminateProcess` path (a no-op on an already-exited process) instead of reading an indeterminate value. ### WR-04: `SetNamedPipeHandleState` return value ignored in CLI client **Files modified:** `src/ctl/main.cpp` **Commit:** `8fd5a9c` **Iteration:** 1 **Applied fix:** Wrapped the existing `SetNamedPipeHandleState(hPipe, &mode, ...)` call in an `if (!...)` guard that prints a `Warning: Could not set pipe to message mode (error %lu); multi-line responses may be truncated` diagnostic to `stderr` via `GetLastError()`. Kept the call non-fatal (execution continues in byte-stream mode) so short single-line responses still work while multi-line `backglow status` truncation is at least visible to the operator. ### IN-01: DDP sequence counter wraps incorrectly for values 1–14 **Files modified:** `src/led/wled_ddp.cpp` **Commit:** `f0d216d` **Iteration:** 2 **Applied fix:** Replaced `(m_seq % 15) + 1` (which produced the cycle 1 → 2 → … → 14 → 1, skipping 15) with `(m_seq >= 15) ? 1 : m_seq + 1`, producing the spec-compliant cycle 1 → 2 → … → 15 → 1. Added an explanatory comment citing the review. Behaviour change is benign — WLED uses the 4-bit sequence only for duplicate-packet drop, and the detection window grows from 14 values to 15. ### IN-02: `com_port_scan.cpp` sort comparator assumes "COM" prefix ≥ 3 chars **Files modified:** `src/led/com_port_scan.cpp` **Commit:** `5794412` **Iteration:** 2 **Applied fix:** Guarded both `std::atoi(a.c_str() + 3)` calls in the sort lambda with `(a.size() > 3) ? … : 0` / `(b.size() > 3) ? … : 0`. Windows always emits "COMn" (4+ chars) so this is a defence-in-depth guard, not a behaviour change. Added a comment pointing at the review. ### IN-03: `deploy-backglow-dev.ps1` hardcodes developer-specific paths **Files modified:** `scripts/deploy-backglow-dev.ps1` **Commit:** `453152c` **Iteration:** 2 **Applied fix:** Added a `param(...)` block exposing `$BuildDir` (default derived from `$PSScriptRoot` so the script works from any checkout) and `$InstallDir` (default is the standard Steam library path, overridable for alternate Steam libraries). Replaced the hardcoded `$src`/`$dst` literals with `Resolve-Path $BuildDir` / `$InstallDir`. Removed the now-redundant post-`Test-Path $src` guard and kept the existing destination-path check. ### IN-04: `ProbeJsonInfo` scans full HTTP response for `"count":` **Files modified:** `src/led/wled_ddp.cpp` **Commit:** `eec214c` **Iteration:** 2 **Applied fix:** Before scanning for `"count":`, locate the HTTP header/body separator (`\r\n\r\n`) and advance 4 bytes past it; if the separator is absent, treat the response as malformed (`ERROR_INVALID_DATA`). A contrived server echoing `"count":` in a header can no longer confuse the parser. WLED itself never does this, so no observable behaviour change against real hardware. ### IN-05: `backglow status` label padding fragile **Files modified:** `src/driver/device_provider.cpp` **Commit:** `0a65380` **Iteration:** 2 **Applied fix:** Replaced the literal-padded `"%s: %s\n"` format specifier with `"%-9s: %s\n"`, which self-documents the 9-char label width and preserves the existing alignment for both current dynamic labels ("port", "host", 4 chars each). Future labels up to 9 chars will auto-align instead of silently breaking. No change to output for the present labels. --- _Fixed: 2026-04-19_ _Fixer: Claude (gsd-code-fixer)_ _Iteration: 2 (combined record with iteration 1)_