[tylerkron]

What was wrong

Nothing a library user can see. This is internal tidying, and it is deliberately behaviour-free.

Twelve types in Core each carried their own private copy of the same small helper: run a logging or Trace call, and swallow anything a consumer-supplied logger or TraceListener throws, so a misbehaving one can't take down a connect, a reconnect, a drop path or the frame pipeline. Most copies came with a paragraph explaining that they mirrored the copy in the class next door. One guarantee, twelve implementations, and twelve places to forget when the guarantee changes.

How it was fixed

One internal DiagnosticGuard in Daqifi.Core.Internal:

• SafeLog(Action): swallows a throwing logger (or a throwing best-effort event subscriber)
• SafeTrace(string): swallows a throwing TraceListener
• SafeTrace(Func<string>): composes the line inside the guard, so an exception whose ToString() throws can't escape the drop path (bug(transport): a throwing StatusChanged subscriber on the drop path leaks the port handle and silently cancels auto-reconnect #494)

The twelve private copies are deleted. Call sites keep the familiar SafeLog / SafeTrace spelling via using static. Call sites changed in DaqifiDevice, DaqifiStreamingDevice, LifecycleGate, StatusChannelPopulator, OperationSerializer, ReconnectSupervisor, TextExchangeEngine, StreamingSessionController, MessageProducer, AllTransportsDeviceFinder, TcpStreamTransport, SerialStreamTransport.

Why this is behaviour-free

Each of the twelve deleted helpers was checked against the guard:

• Catch filter. Every one had a bare catch { } (all exceptions, not just non-fatal ones), and so does the guard. Nothing that used to propagate is now swallowed, and nothing that was swallowed now escapes. None had extra behaviour (a fallback trace, a rethrow, a counter) that the shared helper drops.
• Where the message is composed. The six SafeLog(Action) copies ran the whole lambda inside the try, and still do. The three SafeTrace(string) copies took a message the caller had already built, and still do. The two transports' SafeTrace(Exception) copies built the line inside the try, which the Func<string> overload preserves with the same text. TextExchangeEngine.Log(Action<ILogger>) read _host.Logger inside the try, and the one-line delegation still does.

Silent catches are the settled convention here (#180), and the guard adds no logging inside a catch (#98). The type-level remarks state the contract the empty catch relies on: best-effort notifications out to consumer code only (logger calls, trace writes, best-effort event raises such as SendFailed / ErrorOccurred), never anything the device's correctness depends on.

DropPathSubscriberIsolationTests is the real regression suite for this change. It installs a real listener, asserts the traced text, covers the unrenderable-exception case from #494, and passes unchanged.

Notes for reviewers

• The lambda in SafeTrace(() => $"...") at the two transports is load-bearing, not decoration. Collapsing it to SafeTrace($"...") would reopen bug(transport): a throwing StatusChanged subscriber on the drop path leaks the port handle and silently cancels auto-reconnect #494. Both call sites now say so in a comment, because a future reader would otherwise be right to think it was noise.
• Deliberately unchanged here: DaqifiStreamingDevice (the StreamFrameDiscarded / GapDetected subscriber catches) and AllTransportsDeviceFinder (RaiseIsolated and both SafeDiscoverAsync catches) interpolate {ex} before calling SafeTrace(string), exactly as they did on main. The bug(transport): a throwing StatusChanged subscriber on the drop path leaks the port handle and silently cancels auto-reconnect #494 hazard at those five sites is pre-existing. Moving them to the Func<string> overload would change behaviour, so it belongs in a follow-up, not in this refactor.
• DeviceFinderBase.RaiseIsolated, ContinuousDeviceFinder.SafeRaise / SafeScanError and StreamMessageConsumer's Safe* helpers are left alone. They are event or health-sink isolators with different shapes (catch Exception then trace, reroute to ScanError, no closure on a hot path), not copies of this helper.

Tests

DiagnosticGuardTests pins the swallow, the factory-throw isolation, and that the line actually reaches a listener, since "it didn't throw" is equally true of a guard that quietly writes nothing. It uses the parallel-safe synchronized-listener pattern DropPathSubscriberIsolationTests established. Mutation-checked: making SafeTrace(string) a no-op, or composing the Func<string> line outside the guard, turns DiagnosticGuardTests and the two #494 DropPathSubscriberIsolationTests cases red.

Full suite green locally on net9.0 and net10.0 (Core: 4347 passed, 3 skipped, 0 failed each; Mcp: 269 passed each). Build: 0 warnings, 0 errors. DiagnosticGuard is internal, so there is no public API change (PublicAPI.*.txt untouched). No hardware board attached, so no bench validation was run.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Guard contract excludes event isolation ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The type-level contract for DiagnosticGuard says guarded work must be logging or tracing whose
loss costs only a log line. Existing SafeLog callers also deliver SendFailed and ErrorOccurred
events, so maintainers following this contract could incorrectly remove or reject the
event-isolation behavior.

Code

src/Daqifi.Core/Internal/DiagnosticGuard.cs[R16-19]

+/// The empty <c>catch</c> is deliberate, and it is the reason this type is narrow. Swallowing an
+/// exception is only defensible when the work being swallowed is <em>purely</em> diagnostic —
+/// when dropping it costs a log line and nothing else. That is the whole contract here: pass a
+/// logger call or a <see cref="Trace"/> write, nothing that the device's own correctness depends

Relevance

●●● Strong

Recent history accepts corrections to XML contracts that misstate actual behavior; this guard also
isolates event notifications.

PR-#706
PR-#572
PR-#435

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new remarks restrict the helper to logger calls and trace writes, stating that dropping guarded
work costs only a log line. MessageProducer invokes SendFailed through SafeLog, while
DaqifiDevice invokes both SendFailed and ErrorOccurred through it, proving that the shared
abstraction's stated contract is already false.

src/Daqifi.Core/Internal/DiagnosticGuard.cs[16-32]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[306-312]
src/Daqifi.Core/Device/DaqifiDevice.cs[2854-2861]
src/Daqifi.Core/Device/DaqifiDevice.cs[2934-2949]

Agent prompt

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

## Issue description
`DiagnosticGuard` documents a diagnostic-only contract, but existing callers also use it to isolate best-effort event subscriber notifications.

## Fix Focus Areas
- src/Daqifi.Core/Internal/DiagnosticGuard.cs[16-32]
- src/Daqifi.Core/Communication/Producers/MessageProducer.cs[306-312]
- src/Daqifi.Core/Device/DaqifiDevice.cs[2854-2861]
- src/Daqifi.Core/Device/DaqifiDevice.cs[2934-2949]

## Recommended Fix
Broaden the type and method documentation to explicitly permit best-effort observer notifications whose subscriber failures must be isolated, or introduce a separately named shared guard for event invocation and migrate those callers to it.

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

---

Context sources

✅ Compliance rules (platform): 13 rules

✅ Cross-repo context — repo relationships

Review mode: 🧠 Deep: This push contains substantial, independent behavioral changes across concurrency-sensitive message draining, firmware-cache synchronization, protocol routing, CI/release publishing, and MCP safety metadata, creating multiple plausible defects that benefit from redundant review passes.

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 79429cc

[qodo-code-review]

PR Summary by Qodo

Centralize SafeLog and SafeTrace diagnostic guards

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Centralizes duplicated logging and tracing isolation in one internal diagnostic guard.
• Preserves lazy trace formatting so unrenderable subscriber exceptions cannot escape drop paths.
• Adds focused tests for execution, exception swallowing, trace output, and lazy factories.

Diagram

graph TD
  DEV["Device Core"] -->|safe diagnostics| GUARD["Diagnostic Guard"] -->|log actions| LOG["ILogger"]
  LIFE["Lifecycle Helpers"] -->|safe logging| GUARD
  MSG["Messaging Pipeline"] -->|safe logging| GUARD
  DISC["Device Discovery"] -->|safe tracing| GUARD -->|trace writes| TRACE["Trace Listeners"]
  TRANS["Stream Transports"] -->|lazy trace factory| GUARD

Loading

High-Level Assessment

The shared internal static guard is the best fit because the callers are unrelated types and both ILogger and Trace must receive identical containment semantics. A common base class would create artificial inheritance, while logger decorators would not cover TraceListener failures or lazy exception rendering.

Files changed (14) +174 / -235

Refactor (13) +96 / -235

MessageProducer.csRoute producer logging through DiagnosticGuard +1/-17

Route producer logging through DiagnosticGuard

• Imports the shared guard and removes the producer’s duplicate SafeLog implementation while preserving background-loop logger isolation.

src/Daqifi.Core/Communication/Producers/MessageProducer.cs

SerialStreamTransport.csUse lazy guarded tracing for serial drop failures +9/-35

Use lazy guarded tracing for serial drop failures

• Replaces the transport-specific exception tracing helper with DiagnosticGuard.SafeTrace. Message interpolation remains inside a lambda so a failing exception renderer cannot escape the dropped-connection path.

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

TcpStreamTransport.csUse lazy guarded tracing for TCP drop failures +5/-23

Use lazy guarded tracing for TCP drop failures

• Removes the local SafeTrace implementation and delegates subscriber-failure tracing to the shared guard. Lazy interpolation preserves isolation when consumer exceptions cannot render themselves.

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

DaqifiDevice.csCentralize device logger exception isolation +1/-18

Centralize device logger exception isolation

• Imports DiagnosticGuard.SafeLog and removes the device’s private duplicate helper, retaining protection for frame processing and classified event paths.

src/Daqifi.Core/Device/DaqifiDevice.cs

DaqifiStreamingDevice.csCentralize streaming-device trace isolation +1/-26

Centralize streaming-device trace isolation

• Uses the shared SafeTrace helper and removes the local TraceListener exception guard and now-unneeded diagnostics import.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

AllTransportsDeviceFinder.csCentralize discovery trace isolation +1/-13

Centralize discovery trace isolation

• Routes best-effort discovery tracing through DiagnosticGuard and deletes the finder’s duplicate SafeTrace implementation.

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

LifecycleGate.csShare lifecycle logging guard +1/-18

Share lifecycle logging guard

• Reuses DiagnosticGuard.SafeLog for lifecycle error reporting and removes the class-local exception-swallowing helper.

src/Daqifi.Core/Device/Internal/LifecycleGate.cs

OperationSerializer.csShare operation serializer logging guard +1/-16

Share operation serializer logging guard

• Replaces the serializer’s private SafeLog implementation with the common diagnostic guard without changing queue or operation behavior.

src/Daqifi.Core/Device/Internal/OperationSerializer.cs

ReconnectSupervisor.csShare reconnect logging guard +1/-16

Share reconnect logging guard

• Delegates reconnect diagnostic logging to DiagnosticGuard and removes the supervisor’s duplicate logger containment method.

src/Daqifi.Core/Device/Internal/ReconnectSupervisor.cs

StatusChannelPopulator.csShare status population logging guard +1/-21

Share status population logging guard

• Uses the centralized SafeLog helper for recoverable status warnings and removes the local implementation.

src/Daqifi.Core/Device/Internal/StatusChannelPopulator.cs

StreamingSessionController.csShare streaming-session trace guard +1/-17

Share streaming-session trace guard

• Routes session diagnostics through DiagnosticGuard.SafeTrace and removes the duplicate trace wrapper and unused diagnostics import.

src/Daqifi.Core/Device/Internal/StreamingSessionController.cs

TextExchangeEngine.csDelegate text exchange logging to DiagnosticGuard +2/-15

Delegate text exchange logging to DiagnosticGuard

• Simplifies the engine’s ILogger adapter to invoke its host logger through SafeLog, preserving the existing exception isolation boundary.

src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs

DiagnosticGuard.csIntroduce shared best-effort diagnostic guard +71/-0

Introduce shared best-effort diagnostic guard

• Adds internal SafeLog and SafeTrace helpers that swallow failures from consumer diagnostic code. The factory-based trace overload performs message rendering inside the guard, and documentation defines the narrow contract for silently contained work.

src/Daqifi.Core/Internal/DiagnosticGuard.cs

Tests (1) +78 / -0

DiagnosticGuardTests.csAdd direct coverage for shared diagnostic isolation +78/-0

Add direct coverage for shared diagnostic isolation

• Tests that SafeLog executes actions and swallows failures. Verifies both SafeTrace overloads emit their lines and that lazy message-factory failures remain contained, using a synchronized trace listener for parallel safety.

src/Daqifi.Core.Tests/Internal/DiagnosticGuardTests.cs

[qodo-code-review]

Code review by qodo was updated up to the latest commit 79429cc

[tylerkron]

/agentic_review

[tylerkron]

Reviewed (Claude): verified genuinely behaviour-free — twelve identical bare-catch helpers collapse to one, every catch body was already empty, and the two transports' message composition still happens inside the guard. Qodo-clean on 3bc7ef4, CI green — ready for review.

What I checked, since "pure refactor" is the claim doing the work here:

• Catch semantics. All twelve deleted helpers had an identical empty catch. Nothing that used to propagate is now swallowed, and nothing swallowed now escapes.
• Trace output shape. The two transports' SafeTrace(Exception) twins are replaced by SafeTrace(Func<string>) producing the same line, composed in the same place. DropPathSubscriberIsolationTests — which installs a real listener, asserts the traced text, and covers the unrenderable-exception case from bug(transport): a throwing StatusChanged subscriber on the drop path leaks the port handle and silently cancels auto-reconnect #494 — passes unchanged, and is the real regression suite for this change.
• Settled conventions. Silent catches stay silent (fix: bind UDP discovery per-NIC and skip virtual adapters (#179) #180); no logging was added inside a catch (feat: SerialDeviceFinder probes ports to identify DAQiFi devices #98).
• No deliberately-different call site was collapsed. The only two non-identical cases, the transports' exception-composing variant and TextExchangeEngine.Log's ILogger parameter, are preserved via the Func<string> overload and a one-line delegation.

Changes made under review:

• Restored the call-site rationale the extraction dropped. The lambda in SafeTrace(() => $"...") is load-bearing: collapsing it to SafeTrace($"...") reopens bug(transport): a throwing StatusChanged subscriber on the drop path leaks the port handle and silently cancels auto-reconnect #494. With the explaining doc deleted it read as idle indirection, so both transports now say why it is there.
• Corrected the guard's documented contract. My first pass wrote it as "purely diagnostic", which contradicts three real call sites that use SafeLog to isolate best-effort SendFailed / ErrorOccurred raises — a maintainer following it could have removed that isolation. Restated as: a best-effort notification out to consumer code, where the only thing lost is that one notification; never anything the device's correctness depends on.
• Made a vacuous test earn its place. SafeTrace_WritesWithoutThrowing only asserted that nothing was thrown, which is equally true of a guard that quietly stopped writing. Replaced with tests that capture the trace and assert the line lands, via the parallel-safe synchronized-listener pattern already established here.
• Merged main. The branch had drifted far enough to conflict, which is why GitHub had stopped producing CI runs for it. One conflict, in MessageProducer, where main added SignalIdleIfQuiet right where this branch deletes the SafeLog twin; kept both sides' intent. Re-checked that main introduced no new twins in the meantime — none remain in Daqifi.Core.

No public API change (PublicAPI.*.txt untouched), 0 warnings under TreatWarningsAsErrors. Full suite green locally on net9.0 and net10.0. No hardware board attached, so no bench validation was run.

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Qodo-clean, CI green — ready for review