refactor(device): extract the text-exchange engine into a collaborator (part of #344)

[tylerkron]

The next slice of #344. ExecuteTextCommandCoreAsync plus the raw-capture consumer swap were the largest remaining block in DaqifiDevice — and the primitive every non-streaming operation is built on: the SD card operations, the diagnostics, the LAN chip info and the confirming administration commands all reach it through IDeviceOperationHost.ExecuteTextCommandAsync.

They now live in TextExchangeEngine, reached through a new internal ITextExchangeHost that DaqifiDevice implements explicitly — the same arrangement IDeviceOperationHost already uses.

DaqifiDevice.cs 3,961 → 3,615 lines.

No API change

The ExecuteTextCommandAsync overloads and ExecuteRawCaptureAsync keep their signatures and stay virtual, so the subclass and test-double overrides that intercept device I/O remain in the path.

Verified rather than asserted: I dumped the public + protected surface of both assemblies by reflection (MetadataLoadContext) and diffed against the merge base — byte-identical, 2,035 members.

The subtle part, and why it needed new tests

Three host members write AsyncLocal slots: the operation-lock claim, its release, and the re-entrancy flag. An AsyncLocal write reaches the writing frame's callees and never its caller, so those members have to stay synchronous and be called from the engine's own frame — the engine is what then invokes the caller's prepare/setup/finalize callbacks, which are exactly who must observe them. This is the same constraint RunExclusiveAsync observes by assigning its slot inline instead of in a helper. Both the interface and the implementations say so, at length, because making one of them async Task would compile and read fine.

I checked whether that was actually covered by mutating each write to happen behind an await. Both mutations left the entire suite green. So two tests are added:

• Send_FromInsideATextExchange_GoesStraightOut — a send from the exchange's own setup action must reach the wire during the exchange, not be parked by the deferral the exchange just switched on. The existing suite had the RunExclusiveAsync half of this (Send_FromTheOwningFlow_GoesStraightOut) but not the exchange's own. Broken, the command reaches the device only after the exchange closes, so the exchange collects nothing and returns an empty result — indistinguishable from a silent device.
• TextExchange_ReEnteredFromItsOwnSetupAction_IsRejected — drives the real guard instead of planting the flag by reflection, which only exercised the reading half. Worth pinning because getting it wrong does not deadlock: the nested call finds the lock already held by its own flow, declines to wait for it, and runs a second consumer swap on a stream mid-swap — the framing corruption the guard exists to prevent, failing silently as mangled replies.

Each new test fails against its corresponding mutation and passes against the real thing.

One intentional deviation from a pure move

SuspendInboundConsumer snapshots _messageConsumer once instead of reading it four times. The field is mutable and teardown proceeds once its bounded courtesy wait expires, so the original could detach one instance and stop another, or dereference null. This mirrors what RestartMessageConsumerAfterSwap next to it already did deliberately, and for the reason its remarks already gave. Flagging it because it is the one line that is not a mechanical redirection — happy to revert it to the literal original if you would rather keep this strictly pure.

Everything else is mechanical: _transport → a local captured once after validation (the field is readonly), SafeLog(() => _logger…) → the engine's own Log, and each piece of device state → its host member. I diffed the moved bodies against the originals comment-stripped to confirm nothing else changed.

Testing

2,893 passed / 0 failed / 2 skipped, on net9.0 and net10.0.

Bench-validated on Nq1 (fw 3.7.2, USB, /dev/cu.usbmodem1101), non-destructive throughout — no flash, no format, no delete, no config write. The example CLI was built against this worktree's core and against a build of origin/main, and the two were compared on the same hardware:

Path	Result
--lan-chip-info	ChipId 1377184, FW 19.7.7, build Mar 30 2022 — byte-identical to origin/main
--sd-list	45 files with parsed dates — byte-identical (exercises the prepare/finalize SPI bus switch)
--sd-storage	free 7,799,816,192 / total 7,800,356,864 — byte-identical, and matches the values recorded on this card in #344 earlier
--show-status + stream	identical status line and identical sample count (46 @ 20 Hz/3 s); sample values differ only as live data does

The LAN chip info run is the load-bearing one: it drives a complete real exchange through the extracted engine — lock acquisition, outbound drain, protobuf consumer stop, text consumer swap, stale-line boundary, setup send, two-phase collection, consumer restart, lock release — and parses a correct non-trivial answer.

One anomaly, reported rather than explained. The very first SD query of the session returned SdCardNotPresentException on a card that was demonstrably present. It did not reproduce in 9 further attempts on this branch or 4 on origin/main, including a deliberate stream-then-SD sequence (the known #703 buffer-collapse shape) run alternately on both builds 3× each — 6/6 identical. I could not attribute it to this change, and I could not explain it either; recording it so it is not lost.

Notes

• Sequenced after chore(device): add BootloaderSessionDevice so Core owns the bootloader stand-in #478, which is already in.
• The collaborators extracted by this ticket still have no direct unit tests (test: the extracted collaborators have no direct unit tests — coverage rides the facades they were split from #464); the two tests here are on DaqifiDevice's behaviour, not on TextExchangeEngine in isolation.

Part of #344.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Refactor: extract device text-exchange into TextExchangeEngine collaborator

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Extract SCPI text-exchange and raw-capture consumer swap into an internal TextExchangeEngine.
• Preserve public/protected device APIs via explicit ITextExchangeHost delegation.
• Add tests pinning AsyncLocal-based lock ownership and non-reentrancy behavior.

Diagram

graph TD
  A["DaqifiDevice"] --> H["ITextExchangeHost"] --> B["TextExchangeEngine"] --> E["Operation lock"]
  B --> C[("Transport stream")] --> F["Temp text consumer"]
  B --> D["Protobuf consumer"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Split DaqifiDevice via partial class or regions

• ➕ Keeps the logic physically separated without new abstractions
• ➕ Avoids introducing a new host interface
• ➖ Does not improve testability/encapsulation of the exchange protocol
• ➖ Still keeps the most order-sensitive logic tightly coupled to the device class

2. Private nested helper class inside DaqifiDevice

• ➕ Encapsulates logic without widening namespace surface area
• ➕ Can directly access private fields without a host interface
• ➖ Harder to reuse across other device implementations
• ➖ Still couples the engine to the concrete device shape; less explicit contract than an interface

3. Base-class extraction (e.g., ExchangeCapableDeviceBase)

• ➕ Shares exchange behavior across implementations via inheritance
• ➕ May reduce duplication if multiple device types exist
• ➖ Inheritance hard-couples lifecycle and locking semantics
• ➖ More invasive architectural change; higher risk than a collaborator + explicit host seam

Recommendation: The chosen collaborator + explicit internal ITextExchangeHost is the best trade-off: it isolates highly order-sensitive logic behind a small contract, keeps the public/protected API byte-stable, and makes the AsyncLocal/lock-ownership invariants explicit and testable. The added tests are a key safeguard given the subtle AsyncLocal propagation constraints.

Files changed (4) +847 / -427

Refactor (3) +775 / -427

DaqifiDevice.csDelegate text exchange and raw capture swapping to TextExchangeEngine +81/-427

Delegate text exchange and raw capture swapping to TextExchangeEngine

• Implements internal ITextExchangeHost explicitly and wires a new TextExchangeEngine collaborator. Replaces the in-class ExecuteTextCommandCoreAsync and raw-capture consumer swap logic with delegation to the engine while preserving virtual/public/protected method signatures and behavior contracts.

src/Daqifi.Core/Device/DaqifiDevice.cs

ITextExchangeHost.csIntroduce internal host contract for TextExchangeEngine +142/-0

Introduce internal host contract for TextExchangeEngine

• Adds an internal interface describing the subset of DaqifiDevice state and operations needed to validate, lock, swap consumers, and manage AsyncLocal-based ownership/reentrancy flags. Includes detailed remarks documenting why specific members must remain synchronous for AsyncLocal propagation correctness.

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

TextExchangeEngine.csExtract SCPI exchange and consumer swap engine from DaqifiDevice +552/-0

Extract SCPI exchange and consumer swap engine from DaqifiDevice

• Introduces TextExchangeEngine to own the text-exchange workflow: operation-lock acquisition/ownership, outbound drain, protobuf-consumer suspension, temporary line-consumer collection, stale-line filtering, and restart/error forwarding. Centralizes swap helpers shared by text exchange and raw capture, preserving prior ordering and failure-handling semantics.

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

Tests (1) +72 / -0

DaqifiDeviceOperationSerializationTests.csAdd tests for in-exchange send visibility and reentrancy rejection +72/-0

Add tests for in-exchange send visibility and reentrancy rejection

• Adds two new serialization tests to ensure sends issued from inside the exchange setup action go to the wire immediately, and that re-entering a text exchange from its own setup action is rejected. Introduces an async setupAction overload on the test TextExchangeDevice helper to allow asserting behavior from within the exchange frame.

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

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous review results

Review updated until commit 330d4a3 ⚖️ Balanced

Results up to commit 330d4a3 ⚖️ Balanced

No changes from previous review

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 330d4a3