[tylerkron]

What was wrong

Several "isolation" catches in Core exist so a misbehaving consumer can't break streaming or discovery: they catch an exception thrown by an event subscriber (or a consumer-supplied IDeviceFinder) and write it to a trace line. But the trace line was composed before it reached the guard — SafeTrace($"... {ex}") renders ex.ToString() eagerly, outside the try. An exception whose ToString (or Message) throws therefore escaped the very catch meant to contain it:

• DaqifiStreamingDevice.RaiseGapDetected — the frame that triggered the gap was dropped by the per-frame catch instead of being decoded.
• DaqifiStreamingDevice.RaiseStreamFrameDiscarded — the exception escaped the discard report.
• AllTransportsDeviceFinder.RaiseIsolated and both SafeDiscoverAsync overloads — one bad subscriber or one bad finder faulted the whole discovery pass.

Separately, DeviceState.Streaming was documented as "device is actively streaming data", but nothing ever assigns it: after initialization DaqifiDevice.State stays Ready, and live acquisition is IStreamingDevice.IsStreaming.

Fix

• The five sites now use SafeTrace(Func<string>), which composes the line inside the guard — the same overload SerialStreamTransport and TcpStreamTransport already use for the same reason.
• DeviceState.Streaming's doc now says it is never assigned and points to IsStreaming. The member stays (shipped; removing it is CP0002). No public API change.

Tests

Five new facts throw an exception whose ToString throws (same UnrenderableException shape as DropPathSubscriberIsolationTests):

• DaqifiStreamingDeviceDecodeTests: GapDetected_SubscriberExceptionThatCannotRenderItself_DoesNotSkipFrameDecode, Decode_DiscardSubscriberExceptionThatCannotRenderItself_DoesNotEscape
• AllTransportsDeviceFinderTests: subscriber, finder (token overload), finder (timeout overload)

All five fail against main's source and pass with this change. Full solution: dotnet build -c Release 0 warnings; dotnet test -c Release green on net9.0 and net10.0.

Related: #806 moves DeviceFinderBase.RaiseIsolated onto the same SafeTrace(Func<string>) overload; the two PRs touch different files and agree on the pattern.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Contain trace formatting failures during streaming and discovery

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Compose exception trace messages inside SafeTrace so formatting failures cannot interrupt
 streaming or discovery.
• Add five regression tests for subscriber and finder exceptions whose ToString throws.
• Clarify that DeviceState.Streaming is never assigned; IsStreaming reports live acquisition.

Diagram

graph TD
  A["Subscriber or finder"] --> B["Isolation catches"] --> C["SafeTrace factory"] --> D["Diagnostic guard"] --> E["Trace listener"]
  D --> F["Frame decoding"]
  D --> G["Discovery results"]

Loading

High-Level Assessment

Use the existing SafeTrace(Func) overload at each affected catch. It already guards both message formatting and trace writing; adding local formatting catches would duplicate that behavior without improving isolation.

Files changed (5) +135 / -7

Bug fix (2) +16 / -6

DaqifiStreamingDevice.csGuard exception formatting in streaming event handlers +6/-2

Guard exception formatting in streaming event handlers

• Changes gap and frame-discard subscriber diagnostics to use message factories. Exceptions thrown while rendering a subscriber exception now remain inside SafeTrace rather than interrupting the stream path.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

AllTransportsDeviceFinder.csGuard exception formatting across discovery isolation catches +10/-4

Guard exception formatting across discovery isolation catches

• Uses SafeTrace message factories for subscriber failures and finder failures in both cancellation-token and timeout discovery paths. An exception that fails during formatting can no longer abort publication or the overall discovery pass.

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

Tests (2) +116 / -0

DaqifiStreamingDeviceDecodeTests.csTest streaming isolation when subscriber exceptions cannot be formatted +63/-0

Test streaming isolation when subscriber exceptions cannot be formatted

• Adds gap and discard-event regressions using an exception whose ToString throws. They verify that the gap frame is decoded and that a discard notification does not escape or prevent later decoding.

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceDecodeTests.cs

AllTransportsDeviceFinderTests.csTest discovery isolation for unrenderable exceptions +53/-0

Test discovery isolation for unrenderable exceptions

• Adds regressions for a throwing discovery subscriber and failing finders in both discovery overloads. They verify that healthy finders still return results and subscriber failure does not suppress completion.

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

Documentation (1) +3 / -1

DeviceState.csClarify the unused Streaming enum member +3/-1

Clarify the unused Streaming enum member

• Documents that DeviceState.Streaming is never assigned and directs callers to IStreamingDevice.IsStreaming for live acquisition state. The shipped enum member remains unchanged.

src/Daqifi.Core/Device/DeviceState.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Disconnected devices are documented as ready ✗ Dismissed

Description

The new Streaming comment says DaqifiDevice.State stays Ready after initialization without
limiting that statement to an active connection. When an initialized device is disconnected,
MarkDisconnectedWithoutTeardown sets its state to Disconnected, so the documented behavior no
longer holds.

Code

src/Daqifi.Core/Device/DeviceState.cs[35]

+    /// After initialization, <see cref="DaqifiDevice.State"/> stays <see cref="Ready"/>;

Relevance

●●● Strong

Recent history accepts corrections to overbroad XML claims when runtime behavior has important
exceptions.

PR-#757
PR-#572
PR-#508

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The added XML comment makes an unqualified claim that State stays Ready after initialization. The
device teardown path assigns Disconnected, while successful initialization assigns Ready.

src/Daqifi.Core/Device/DeviceState.cs[33-37]
src/Daqifi.Core/Device/DaqifiDevice.cs[1502-1505]
src/Daqifi.Core/Device/DaqifiDevice.cs[3379-3383]

Agent prompt

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

## Issue description
The public documentation says the device state stays Ready after initialization, but disconnecting changes it to Disconnected.
## Fix Focus Areas
- src/Daqifi.Core/Device/DeviceState.cs[34-36]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1502-1505]
## Recommended Fix
Say that State remains Ready during streaming while the initialized device is connected, rather than claiming it stays Ready after initialization generally.

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

---

Context sources

✅ Compliance rules (platform): 13 rules

Review mode: Auto: ⏭️ Skipped: Pure documentation change with no behavioral or semantic effect.

Tip of the day

💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 2790f0b

Results up to commit ae1cd1d ⚖️ Balanced

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

1. Disconnected devices are documented as ready ✗ Dismissed

Description

The new Streaming comment says DaqifiDevice.State stays Ready after initialization without
limiting that statement to an active connection. When an initialized device is disconnected,
MarkDisconnectedWithoutTeardown sets its state to Disconnected, so the documented behavior no
longer holds.

Code

src/Daqifi.Core/Device/DeviceState.cs[35]

+    /// After initialization, <see cref="DaqifiDevice.State"/> stays <see cref="Ready"/>;

Relevance

●●● Strong

Recent history accepts corrections to overbroad XML claims when runtime behavior has important
exceptions.

PR-#757
PR-#572
PR-#508

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The added XML comment makes an unqualified claim that State stays Ready after initialization. The
device teardown path assigns Disconnected, while successful initialization assigns Ready.

src/Daqifi.Core/Device/DeviceState.cs[33-37]
src/Daqifi.Core/Device/DaqifiDevice.cs[1502-1505]
src/Daqifi.Core/Device/DaqifiDevice.cs[3379-3383]

Agent prompt

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

## Issue description
The public documentation says the device state stays Ready after initialization, but disconnecting changes it to Disconnected.
## Fix Focus Areas
- src/Daqifi.Core/Device/DeviceState.cs[34-36]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1502-1505]
## Recommended Fix
Say that State remains Ready during streaming while the initialized device is connected, rather than claiming it stays Ready after initialization generally.

ⓘ 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 2790f0b