fix(transport): name serial connect failures instead of calling them all access denials (closes #424)

[tylerkron]

Why

Connecting to a serial port that doesn't exist reported Access to the port '/dev/cu.doesnotexist' is denied. — sending users off to check permissions and dialout group membership when the real problem was a wrong or stale port name. That's the common case, because USB serial device nodes get renumbered every time you replug. The TCP transport already avoids exactly this trap by translating a misleading TaskCanceledException into a real TimeoutException; serial had no equivalent.

What

A failed serial connect now says what actually went wrong: "Serial port 'X' was not found.", "Serial port 'X' is in use.", or the original access-denied wording when it really is a permission problem. Callers who want to branch on it get a typed SerialPortConnectException with a Reason of NotFound, InUse, AccessDenied, or Unknown, instead of string-matching. The original platform exception is always kept as InnerException, so nothing is lost.

How

The interesting part is that the reason cannot be recovered from the platform exception at all, which the issue's suggested fix assumed it could. On macOS a missing port and a port held by another process produce byte-identical exceptions — same type, same message, same inner IOException. Worse, that inner message wasn't even stable: on this machine, for the same port, one process reported Unknown error: 203 and another reported No such file or directory. Its HResult is a stale errno, not a signal.

So the reason is derived from evidence gathered at the moment of the failure instead: whether the port is still present (the same probe the transport already trusts to detect an unplug), and whether a per-user permission gate could apply at all (a macOS /dev/cu.* node is crw-rw-rw-, so nobody can be denied on permission grounds — a denial there means someone else holds it; a Linux dialout port at crw-rw---- keeps the access-denied reading). Anything that can't be determined degrades to today's wording rather than asserting something false. Retry behaviour is untouched — a translated failure is still just one failed attempt.

Bench test

Real Nq1 (FW 3.7.2), macOS, example CLI built against this branch.

USB/serial — the issue's own repro, before and after:

BEFORE (raw SerialPort.Open — what Core used to forward)
  UnauthorizedAccessException: Access to the port '/dev/cu.doesnotexist' is denied.
   ---> IOException: Unknown error: 203 (HResult=203)

AFTER  $ Daqifi.Core.Cli.dll --serial /dev/cu.doesnotexist --duration 2
  Daqifi.Core.Communication.Transport.SerialPortConnectException:
      Serial port '/dev/cu.doesnotexist' was not found.
   ---> System.UnauthorizedAccessException: Access to the port '...' is denied.  (preserved)

All three classifications against the real device:

A. missing port      Reason = NotFound   "Serial port '/dev/cu.doesnotexist' was not found."
B. real port, held   Reason = InUse      "Serial port '/dev/cu.usbmodem1101' is in use."
C. real port, free   connected OK: Serial: /dev/cu.usbmodem1101 @ 9600 baud

USB happy path not broken — connect + SCPI init + clean disconnect still work, and discovery still identifies the unit:

Found: Nq1 (/dev/cu.usbmodem1101) SN:9090539562006014104 FW:3.7.2

WiFi/TCP — no-regression check only (this change is serial-only and touches zero TCP code). One consolidated connect to 192.168.1.30:9760: the TCP transport connected fine and got through to DaqifiDevice.InitializeAsync, which then hit the known 8 s channel-config timeout this unit is currently in (SD low-heap / degraded state, power cycle being arranged). That is pre-existing and unrelated.

Streaming, end to end on the real device (after the unit was power cycled out of the channel-config window it had been stuck in):

$ Daqifi.Core.Cli.dll --serial /dev/cu.usbmodem1101 --channels 7 --rate 50 --duration 6
Connected to /dev/cu.usbmodem1101 @ 9600 baud
Streaming at 50 Hz...
ts=3192967391 analog=[3, 8, 0]
ts=3193807390 analog=[4, 8, 0]
...
ts=3389527390 analog=[4, 7, 0]
Streaming stopped.
Status: Disconnected                      exit 0

237 sample messages across the 6 s window on three analog channels, timestamps monotonic, clean stop. That is ~79% of the requested 50 Hz, which is the known rate behaviour of this bench unit (fw#716) and not a shortfall introduced here.

This closes the one gap left open earlier in review: the happy path is now proven to connect and stream, not just connect.

Full suite green: 2496 passed on both net9.0 and net10.0, Release build clean with 0 warnings.

Note for daqifi-desktop

SerialStreamTransport.ConnectAsync now throws SerialPortConnectException (an IOException) where it previously threw the raw UnauthorizedAccessException. Desktop's SerialStreamingDevice.LogConnectFailure switches on UnauthorizedAccessException / FileNotFoundException to classify these as warnings rather than Sentry errors, so it will need one added case or those connect failures will start reporting as errors. Nothing inside Core regressed — serial discovery uses a raw SerialPort and has a catch-all fallback.

Closes #424

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix serial connect failures by translating to typed SerialPortConnectException

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Translate misleading SerialPort.Open failures into a typed SerialPortConnectException.
• Classify failures as NotFound/InUse/AccessDenied/Unknown using runtime evidence, not messages.
• Add unit/integration tests to pin behavior and preserve InnerException + retry semantics.

Diagram

graph TD
  A["Caller / CLI"] --> B["SerialStreamTransport.ConnectAsync"] --> C["SerialPort.Open"] --> D{"Open failed?"}
  D -->|"No"| E["Connected"]
  D -->|"Yes"| F["Evidence probes"] --> G["SerialPortConnectException.FromOpenFailure"] --> H["Throw SerialPortConnectException"]
  F --> F1["IsPortPresent()"]
  F --> F2["File.GetUnixFileMode"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Classify by exception type/message/HResult (string/errno matching)

• ➕ No extra probing calls after failure
• ➕ Simpler implementation surface
• ➖ Not reliable across platforms (macOS missing vs busy can be byte-identical)
• ➖ Brittle to framework changes/localization; risks misdiagnosis

2. Only use port presence (present => AccessDenied/InUse, absent => NotFound)

• ➕ Very portable and minimal OS-specific logic
• ➕ Directly fixes the most common misleading case (missing port)
• ➖ Cannot distinguish InUse vs AccessDenied where that distinction matters (e.g., Linux permissions)
• ➖ Would reduce the value of the new typed Reason for callers

3. Expose a diagnostic callback / result object instead of throwing a new exception type

• ➕ Avoids introducing a new public exception type to catch
• ➕ Could carry richer structured diagnostics without overloading exception taxonomy
• ➖ Requires API shape changes for callers; larger compatibility surface
• ➖ Harder to adopt incrementally vs wrapping the existing failure path

Recommendation: Keep the PR’s approach: translate at the failure point and classify using immediate, observable evidence (presence + permission-gate feasibility), while preserving the original exception as InnerException. This is the only option that avoids brittle exception-text inference on macOS while still providing actionable, typed reasons for callers on platforms where the distinction is meaningful.

Files changed (5) +591 / -1

Enhancement (1) +42 / -0

SerialPortConnectFailure.csAdd SerialPortConnectFailure enum for stable connect-failure reasons +42/-0

Add SerialPortConnectFailure enum for stable connect-failure reasons

• Defines the stable, typed failure reasons (Unknown, NotFound, InUse, AccessDenied) used by SerialPortConnectException. Documents cross-platform motivation and retry implications for each reason.

src/Daqifi.Core/Communication/Transport/SerialPortConnectFailure.cs

Bug fix (2) +273 / -1

SerialPortConnectException.csIntroduce SerialPortConnectException with typed Reason and stable message +180/-0

Introduce SerialPortConnectException with typed Reason and stable message

• Adds a new IOException-derived exception carrying PortName and a SerialPortConnectFailure Reason. Implements evidence-based classification (including FileNotFoundException and presence/permission signals) and generates user-facing messages that avoid the misleading blanket 'access denied' wording.

src/Daqifi.Core/Communication/Transport/SerialPortConnectException.cs

SerialStreamTransport.csTranslate SerialPort.Open failures using evidence probes +93/-1

Translate SerialPort.Open failures using evidence probes

• Wraps SerialPort.Open to catch UnauthorizedAccessException/IOException and rethrow a translated SerialPortConnectException that preserves the original as InnerException. Adds probes to observe port presence and (on Unix) infer whether a permission gate could apply via File.GetUnixFileMode; unknown probes degrade to current access-denied behavior.

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs

Tests (2) +276 / -0

SerialPortConnectExceptionTests.csAdd unit tests for connect-failure classification + messaging +191/-0

Add unit tests for connect-failure classification + messaging

• Introduces focused unit tests for SerialPortConnectException.Classify/DescribeFailure/FromOpenFailure, including nested exception cases and null-safety. Pins the intended platform-agnostic behavior (NotFound vs InUse vs AccessDenied vs Unknown) and verifies InnerException preservation.

src/Daqifi.Core.Tests/Communication/Transport/SerialPortConnectExceptionTests.cs

SerialStreamTransportTests.csAdd integration-style tests for missing-port connect translation +85/-0

Add integration-style tests for missing-port connect translation

• Adds ConnectAsync tests asserting missing-port failures surface as SerialPortConnectException with Reason=NotFound and a non-misleading message. Verifies the platform exception is preserved, the exception remains catchable as IOException, StatusChanged reports the typed error, and retry behavior remains unchanged.

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.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. Windows enumeration hard-fails tests ✗ Dismissed 🐞 Bug ☼ Reliability ⭐ New

Description

CreateVerifiedAbsentPort() throws InvalidOperationException on Windows when
SerialPort.GetPortNames() fails, which will fail every new “missing port” test before ConnectAsync
is exercised. This makes the suite brittle on restricted Windows environments (where enumeration can
legitimately throw) and contradicts the helper’s intent to not take the suite down on enumeration
failures.

Code

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[R183-186]

+            if (!enumerationAnswered)
+            {
+                throw new InvalidOperationException(
+                    "Cannot verify an absent COM name: SerialPort.GetPortNames() failed and Windows " +

Relevance

●●● Strong

PR #403 accepted treating SerialPort.GetPortNames() failures as inconclusive; test helpers shouldn’t
hard-fail on probe exceptions.

PR-#403

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The helper explicitly records enumeration failure, then immediately converts that into a thrown
InvalidOperationException on Windows. Because the new missing-port tests call
CreateVerifiedAbsentPort(), the exception will fail those tests before transport behavior is
exercised. Past PR #403 documents that SerialPort.GetPortNames() can throw and should be treated as
‘no observation’, supporting that this is a real environment mode to handle gracefully in tests.

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[155-189]
src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[222-229]
PR-#403

Agent prompt

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

### Issue description
On Windows, `CreateVerifiedAbsentPort()` currently throws `InvalidOperationException` when `SerialPort.GetPortNames()` throws. All new missing-port tests depend on this helper, so those tests will **fail** (not skip) on Windows hosts where enumeration is unavailable (locked-down runners/containers), even though that’s an environmental limitation rather than a product regression.

### Issue Context
Unlike Unix, Windows has no independent `File.Exists`-style check for COM names, so we *should not* guess an “absent” COM port after a failed enumeration. The correct behavior for the test suite is to **skip** the missing-port tests (or otherwise mark them as inconclusive) when absence cannot be verified.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[155-190]

### Suggested change
- In the `if (!enumerationAnswered)` branch on Windows, throw an xUnit skip mechanism (e.g., `Xunit.Sdk.SkipException`) instead of `InvalidOperationException`, with the existing explanatory message.
- Keep the current safety property: do **not** proceed to pick an unverified COM name when enumeration failed.

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

---

2. Unverified COM port chosen ✓ Resolved 🐞 Bug ☼ Reliability

Description

CreateVerifiedAbsentPort() swallows SerialPort.GetPortNames() failures and proceeds with an empty
set, which on Windows can cause it to return a COM name that actually exists. This can make the
missing-port tests behave nondeterministically (unexpected success, different exception, or
potential blocking in Open()).

Code

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[R166-169]

+        catch (Exception)
+        {
+            enumerated = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
+        }

Relevance

●●● Strong

Team often hardens tests against nondeterministic probes; unverified COM selection likely fixed for
determinism.

PR-#403
PR-#237

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The focus-area change catches all exceptions from SerialPort.GetPortNames() and replaces the
enumeration with an empty set. The Windows branch later selects the first COM number not present in
that set; when the set is empty due to an enumeration failure, the selected COM name is no longer
verified absent, yet this helper is used by the missing-port tests.

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[155-186]
src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[201-216]

Agent prompt

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

## Issue description
`CreateVerifiedAbsentPort()` treats a `SerialPort.GetPortNames()` failure as “no ports exist” by substituting an empty set. On Windows, that makes the helper return a COM name without any evidence it’s absent, which can cause flaky/hanging tests.

## Issue Context
The missing-port tests depend on `CreateVerifiedAbsentPort()` returning a port name that is truly absent on the running host.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[155-186]

## Suggested fix
- Track whether enumeration succeeded (e.g., `bool enumerationSucceeded`).
- If enumeration failed on Windows, **do not** pick from the relatively plausible range `COM200..COM255`; instead return a much less collision-prone name (e.g., `COM99999` or a random very high number like `COM{Random.Shared.Next(10000, 60000)}`) to minimize the chance of hitting a real port when enumeration is unavailable.
- Keep the current “highest unused in enumeration” logic when enumeration succeeds.

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

---

3. Unhandled GetPortNames exception ✓ Resolved 🐞 Bug ☼ Reliability

Description

CreateVerifiedAbsentPort() calls SerialPort.GetPortNames() without handling exceptions, so any
transient/OS-specific enumeration failure will fail all new missing-port tests before they even
reach the ConnectAsync assertions. The production transport already treats enumeration failures as
“no observation” rather than hard failure, so the tests should similarly retry or skip when
enumeration can’t be read.

Code

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[R155-156]

+        var enumerated = new HashSet<string>(SerialPort.GetPortNames(), StringComparer.OrdinalIgnoreCase);
+

Relevance

●●● Strong

PR #403 accepted treating GetPortNames probe exceptions as “no observation”; tests should mirror
that to avoid flakiness.

PR-#403

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new tests depend on CreateVerifiedAbsentPort() but it does not handle enumeration failures. In
contrast, SerialStreamTransport’s presence probe explicitly documents and guards against
SerialPort.GetPortNames() throwing, demonstrating this failure mode is expected and should be
handled to keep tests deterministic.

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[153-172]
src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[580-600]
PR-#403

Agent prompt

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

### Issue description
`CreateVerifiedAbsentPort()` builds a `HashSet` from `SerialPort.GetPortNames()` directly. If `GetPortNames()` throws (which can happen transiently or due to platform/host restrictions), the helper throws and all tests that depend on it fail for environmental reasons.

### Issue Context
The production code already anticipates that `SerialPort.GetPortNames()` can throw and treats that as an inconclusive observation (not evidence the port is absent). The tests should not be more brittle than the production behavior they are validating.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[153-172]

### Suggested change
- Wrap `SerialPort.GetPortNames()` in `try/catch`.
- Prefer one of:
 - **Bounded retries** (e.g., 2–3 attempts) before giving up, OR
 - **Skip the missing-port tests** when enumeration is unavailable (e.g., throw `Xunit.Sdk.SkipException` with a clear message), to avoid false CI failures and accidental interaction with real ports.
- Avoid treating a failed enumeration as “empty set” on Windows unless you also add an additional safety check, since you otherwise lose the “verified absent” guarantee that prevents accidental hardware access.

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

---

View more (1)

4. Hardcoded missing port ✓ Resolved 🐞 Bug ☼ Reliability

Description

SerialStreamTransportTests uses a fixed "nonexistent" port name (COM254 or a fixed /dev path); if
that port exists on a given machine/CI runner, the test can connect to real hardware or fail with a
different error shape. This makes the new missing-port tests environment-dependent and potentially
flaky.

Code

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[R148-149]

+    private static string NonexistentPort =>
+        OperatingSystem.IsWindows() ? "COM254" : "/dev/tty.daqifi-core-nonexistent-424";

Relevance

●●● Strong

Team has accepted making transport tests deterministic and avoiding environment-dependent endpoints;
likely will change port selection/seam.

PR-#237
PR-#403

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The tests assume NonexistentPort cannot resolve on the host, but it is a fixed string; if it
resolves, the assertions about a missing-port SerialPortConnectException become invalid and may
even hit real devices.

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[144-167]

Agent prompt

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

## Issue description
`NonexistentPort` is hard-coded and not guaranteed to be absent on all hosts, which can make the test suite flaky or interact with real devices.

## Issue Context
The tests require a port name that is definitively absent on the running platform so they can assert the `NotFound` translation path.

## Fix
Generate a unique, verified-absent port name at runtime:
- **Windows**: enumerate `SerialPort.GetPortNames()` and pick a COM name not present (loop and/or use a very high number with a guard that it’s not in the returned set).
- **Unix**: use a GUID-suffixed path like `/dev/tty.daqifi-core-nonexistent-424-{guid}` and assert `File.Exists(...) == false`.
- If the generated name unexpectedly exists, regenerate until it’s absent.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[144-167]

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

---

5. UnauthorizedAccessException hidden ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

SerialStreamTransport.ConnectAsync now catches UnauthorizedAccessException/IOException from
SerialPort.Open and rethrows SerialPortConnectException (an IOException-derived type), so callers
that previously relied on catching UnauthorizedAccessException from ConnectAsync will no longer
catch it directly. The original platform exception is only available via InnerException, which
changes downstream exception-type branching behavior.

Code

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[R319-320]

+                    throw SerialPortConnectException.FromOpenFailure(
+                        _portName, ex, TryObservePortPresence(), TryRuleOutPermissionGate());

Relevance

●● Moderate

Wrapping/translation of connect failures into typed exceptions is an established pattern, but
exception-type breaking change may prompt docs/compat tweaks.

PR-#237

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

ConnectAsync wraps Open failures with SerialPortConnectException.FromOpenFailure(...), and the new
exception type derives from IOException (not UnauthorizedAccessException), so only InnerException
retains the original type.

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[302-321]
src/Daqifi.Core/Communication/Transport/SerialPortConnectException.cs[42-74]

Agent prompt

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

## Issue description
`ConnectAsync` now normalizes platform exceptions into `SerialPortConnectException`. This is intentional, but it changes the public exception type surfaced to callers, especially for code doing `catch (UnauthorizedAccessException)`.

## Issue Context
The PR preserves the original exception as `InnerException`, but direct exception-type catches won’t match anymore.

## Fix
Explicitly document the behavioral change for downstream consumers:
- In `SerialStreamTransport.ConnectAsync` XML docs, mention that `UnauthorizedAccessException`/`IOException` from `SerialPort.Open()` are wrapped into `SerialPortConnectException` and preserved as `InnerException`.
- If the project maintains a CHANGELOG/release notes, add an entry calling out the new exception type and recommending `catch (SerialPortConnectException)` (or `catch (IOException)` if appropriate) + inspecting `Reason`.

## Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[257-321]
- src/Daqifi.Core/Communication/Transport/SerialPortConnectException.cs[3-41]

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Previous review results

Review updated until commit 27a3a44

Results up to commit 5e5824f ⚖️ Balanced

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

1. Hardcoded missing port ✓ Resolved 🐞 Bug ☼ Reliability

Description

SerialStreamTransportTests uses a fixed "nonexistent" port name (COM254 or a fixed /dev path); if
that port exists on a given machine/CI runner, the test can connect to real hardware or fail with a
different error shape. This makes the new missing-port tests environment-dependent and potentially
flaky.

Code

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[R148-149]

+    private static string NonexistentPort =>
+        OperatingSystem.IsWindows() ? "COM254" : "/dev/tty.daqifi-core-nonexistent-424";

Relevance

●●● Strong

Team has accepted making transport tests deterministic and avoiding environment-dependent endpoints;
likely will change port selection/seam.

PR-#237
PR-#403

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The tests assume NonexistentPort cannot resolve on the host, but it is a fixed string; if it
resolves, the assertions about a missing-port SerialPortConnectException become invalid and may
even hit real devices.

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[144-167]

Agent prompt

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

## Issue description
`NonexistentPort` is hard-coded and not guaranteed to be absent on all hosts, which can make the test suite flaky or interact with real devices.

## Issue Context
The tests require a port name that is definitively absent on the running platform so they can assert the `NotFound` translation path.

## Fix
Generate a unique, verified-absent port name at runtime:
- **Windows**: enumerate `SerialPort.GetPortNames()` and pick a COM name not present (loop and/or use a very high number with a guard that it’s not in the returned set).
- **Unix**: use a GUID-suffixed path like `/dev/tty.daqifi-core-nonexistent-424-{guid}` and assert `File.Exists(...) == false`.
- If the generated name unexpectedly exists, regenerate until it’s absent.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[144-167]

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

---

2. UnauthorizedAccessException hidden ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

SerialStreamTransport.ConnectAsync now catches UnauthorizedAccessException/IOException from
SerialPort.Open and rethrows SerialPortConnectException (an IOException-derived type), so callers
that previously relied on catching UnauthorizedAccessException from ConnectAsync will no longer
catch it directly. The original platform exception is only available via InnerException, which
changes downstream exception-type branching behavior.

Code

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[R319-320]

+                    throw SerialPortConnectException.FromOpenFailure(
+                        _portName, ex, TryObservePortPresence(), TryRuleOutPermissionGate());

Relevance

●● Moderate

Wrapping/translation of connect failures into typed exceptions is an established pattern, but
exception-type breaking change may prompt docs/compat tweaks.

PR-#237

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

ConnectAsync wraps Open failures with SerialPortConnectException.FromOpenFailure(...), and the new
exception type derives from IOException (not UnauthorizedAccessException), so only InnerException
retains the original type.

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[302-321]
src/Daqifi.Core/Communication/Transport/SerialPortConnectException.cs[42-74]

Agent prompt

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

## Issue description
`ConnectAsync` now normalizes platform exceptions into `SerialPortConnectException`. This is intentional, but it changes the public exception type surfaced to callers, especially for code doing `catch (UnauthorizedAccessException)`.

## Issue Context
The PR preserves the original exception as `InnerException`, but direct exception-type catches won’t match anymore.

## Fix
Explicitly document the behavioral change for downstream consumers:
- In `SerialStreamTransport.ConnectAsync` XML docs, mention that `UnauthorizedAccessException`/`IOException` from `SerialPort.Open()` are wrapped into `SerialPortConnectException` and preserved as `InnerException`.
- If the project maintains a CHANGELOG/release notes, add an entry calling out the new exception type and recommending `catch (SerialPortConnectException)` (or `catch (IOException)` if appropriate) + inspecting `Reason`.

## Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[257-321]
- src/Daqifi.Core/Communication/Transport/SerialPortConnectException.cs[3-41]

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

---

Results up to commit 0d96dec ⚖️ Balanced

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

1. Unhandled GetPortNames exception ✓ Resolved 🐞 Bug ☼ Reliability

Description

CreateVerifiedAbsentPort() calls SerialPort.GetPortNames() without handling exceptions, so any
transient/OS-specific enumeration failure will fail all new missing-port tests before they even
reach the ConnectAsync assertions. The production transport already treats enumeration failures as
“no observation” rather than hard failure, so the tests should similarly retry or skip when
enumeration can’t be read.

Code

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[R155-156]

+        var enumerated = new HashSet<string>(SerialPort.GetPortNames(), StringComparer.OrdinalIgnoreCase);
+

Relevance

●●● Strong

PR #403 accepted treating GetPortNames probe exceptions as “no observation”; tests should mirror
that to avoid flakiness.

PR-#403

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new tests depend on CreateVerifiedAbsentPort() but it does not handle enumeration failures. In
contrast, SerialStreamTransport’s presence probe explicitly documents and guards against
SerialPort.GetPortNames() throwing, demonstrating this failure mode is expected and should be
handled to keep tests deterministic.

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[153-172]
src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[580-600]
PR-#403

Agent prompt

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

### Issue description
`CreateVerifiedAbsentPort()` builds a `HashSet` from `SerialPort.GetPortNames()` directly. If `GetPortNames()` throws (which can happen transiently or due to platform/host restrictions), the helper throws and all tests that depend on it fail for environmental reasons.

### Issue Context
The production code already anticipates that `SerialPort.GetPortNames()` can throw and treats that as an inconclusive observation (not evidence the port is absent). The tests should not be more brittle than the production behavior they are validating.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[153-172]

### Suggested change
- Wrap `SerialPort.GetPortNames()` in `try/catch`.
- Prefer one of:
 - **Bounded retries** (e.g., 2–3 attempts) before giving up, OR
 - **Skip the missing-port tests** when enumeration is unavailable (e.g., throw `Xunit.Sdk.SkipException` with a clear message), to avoid false CI failures and accidental interaction with real ports.
- Avoid treating a failed enumeration as “empty set” on Windows unless you also add an additional safety check, since you otherwise lose the “verified absent” guarantee that prevents accidental hardware access.

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

---

Results up to commit ff877f8 ⚖️ Balanced

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

1. Unverified COM port chosen ✓ Resolved 🐞 Bug ☼ Reliability

Description

CreateVerifiedAbsentPort() swallows SerialPort.GetPortNames() failures and proceeds with an empty
set, which on Windows can cause it to return a COM name that actually exists. This can make the
missing-port tests behave nondeterministically (unexpected success, different exception, or
potential blocking in Open()).

Code

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[R166-169]

+        catch (Exception)
+        {
+            enumerated = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
+        }

Relevance

●●● Strong

Team often hardens tests against nondeterministic probes; unverified COM selection likely fixed for
determinism.

PR-#403
PR-#237

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The focus-area change catches all exceptions from SerialPort.GetPortNames() and replaces the
enumeration with an empty set. The Windows branch later selects the first COM number not present in
that set; when the set is empty due to an enumeration failure, the selected COM name is no longer
verified absent, yet this helper is used by the missing-port tests.

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[155-186]
src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[201-216]

Agent prompt

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

## Issue description
`CreateVerifiedAbsentPort()` treats a `SerialPort.GetPortNames()` failure as “no ports exist” by substituting an empty set. On Windows, that makes the helper return a COM name without any evidence it’s absent, which can cause flaky/hanging tests.

## Issue Context
The missing-port tests depend on `CreateVerifiedAbsentPort()` returning a port name that is truly absent on the running host.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs[155-186]

## Suggested fix
- Track whether enumeration succeeded (e.g., `bool enumerationSucceeded`).
- If enumeration failed on Windows, **do not** pick from the relatively plausible range `COM200..COM255`; instead return a much less collision-prone name (e.g., `COM99999` or a random very high number like `COM{Random.Shared.Next(10000, 60000)}`) to minimize the chance of hitting a real port when enumeration is unavailable.
- Keep the current “highest unused in enumeration” logic when enumeration succeeds.

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

---

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0d96dec

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit ff877f8

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit c4e328e

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit c4e328e