fix(sdcard): distinguish a timed-out SD LIST from an empty card (closes #396)

[tylerkron]

Problem

GetSdCardFilesAsync returned an empty list both when the SD card was genuinely empty and when the device never answered, so an unreachable device holding data rendered downstream as "SD card OK · 0 files" (daqifi-avalonia#2).

Which option, and why none of the three worked as written

The issue offered three options. Checking them against the firmware source (daqifi-nyquist-firmware, sd_card_manager.c / SCPIStorageSD.c) changed the answer:

Options 1 and 2 as stated are wrong — they would make a genuinely empty card throw. ListFilesInDirectoryChunked only calls sendChunk when strBuffIndex > 0, so an empty directory produces zero bytes, and SCPI_StorageSDListDir writes no SCPI result of its own. A phase-1 timeout with zero lines collected is therefore exactly what a healthy empty card looks like. "The device never answered" and "the card is empty" are byte-for-byte identical on the wire, and no amount of timeout bookkeeping inside ExecuteTextCommandCoreAsync can separate them.

Option 3 as stated is also out — the firmware emits no end-of-listing marker. Verified on the bench (raw SCPI probe below): the listing simply stops.

What shipped: option 3, with Core supplying the terminator the firmware lacks. GetSdCardFilesAsync appends a SYSTem:ERRor? query to the same text exchange, right after SYSTem:STORage:SD:LIST?, and treats its reply as the end-of-listing marker. The transport is ordered and SCPI_StorageSDListDir blocks in sd_card_manager_WaitForCompletion until every chunk has been handed to the output, so the reply arriving proves both that the device is answering and that the listing ahead of it arrived in full. Its absence raises the new SdCardListIncompleteException instead of a plausible-looking empty list.

That also fixes the truncation case the issue calls out as unreachable by any downstream mitigation: corroborating with a later GetSdCardStorageAsync proves the device answered a subsequent request, never that this listing was complete. A missing terminator does.

The terminator's error code is used only as a liveness marker, never for classification — the queue it pops can hold entries from earlier commands. Documented side effect: each listing now consumes one SCPI error-queue entry.

Caller audit of the text path

The blast radius is deliberately narrow. In particular the empty-reply contract of ExecuteTextCommandCoreAsync is untouched, which matters because that is a genuinely unsafe thing to change globally — several callers depend on an empty reply being the success case:

Caller	Zero lines means	Impact
DaqifiDevice.InitializeAsync (echo off / stop / power on / protobuf format)	success — these are commands, not queries	a blanket throw would break every device init
DaqifiStreamingDevice.OnDeviceInitializingAsync (SetStreamInterface)	success — command, not query	same
DaqifiDevice.DrainErrorQueueAsync	already handled explicitly: "Empty reply means timeout or unresponsive device… terminate rather than spin"	would become a throw
GetSdCardStorageAsync, GetLanChipInfoAsync, and the other query helpers	parser failure → already typed exceptions	unaffected
DeleteSdCardFileAsync (delete + LIST)	refreshes _sdCardFiles	unchanged; see below
GetSdCardFilesAsync	the bug	fixed

src/Daqifi.Mcp does not call GetSdCardFilesAsync, and no doc depends on empty-on-timeout.

Review round 1 did add one shared change to ExecuteTextCommandCoreAsync — dropping lines that predate the exchange's own commands (see below). That is additive for every caller in the table: none of them can want a reply to a command they had not yet sent, and all of the above were re-benched afterwards.

Deliberate scope boundary: DeleteSdCardFileAsync re-lists to refresh the cache and has the same latent ambiguity, but resolving it means deciding what a lost reply says about whether the delete succeeded — a different question — and it cannot be bench-verified non-destructively on the shared board. Left exactly as it is on main; no regression either way.

Bench evidence — DAQiFi Nyquist 1, firmware 3.7.2, /dev/cu.usbmodem1101

Example CLI built against this worktree's Daqifi.Core.

Real listing still returns in full and does not throw — 31 files, and the terminator is not parsed as a phantom entry:

$ ... --sd-list
Listing SD card files...
  log_20260623_143217.bin  2026-06-23 14:32:17  [Protobuf]
  ... (29 more)
  log_20260728_190448.bin  2026-07-28 19:04:48  [Protobuf]
Total: 31 file(s)

Raw SCPI probe (read-only; SD enable → list → LAN restore) confirming both halves of the design:

=== Probe 1: LIST alone ===
last 3 lines:
  'DAQiFi/log_20260728_104156.bin 13791'
  'DAQiFi/log_20260728_190448.bin 0'
  'DAQIFI> '
--> no end-of-listing marker of any kind

=== Probe 2: LIST followed by SYSTem:ERRor? ===
last 3 lines:
  'DAQIFI> SYSTem:ERRor?'
  '0,"No error"'
  'DAQIFI> '
--> terminator arrives last, after every listing line

The reply is the bare <code>,"<message>" form the matcher expects, and it lands after every listing line. Also re-ran --sd-storage (unchanged) and a 10 Hz / 2-channel stream (5 messages, clean stop), then --sd-list again — still 31 files. Nothing destructive: no format, delete, flash, reboot or WiFi change.

The silent-device and truncated-listing paths are covered by unit tests rather than the bench — provoking them on real hardware needs an unplug mid-transfer.

Tests

src/Daqifi.Core.Tests, full suite green on net9.0 and net10.0 (1958 passed, 0 failed), build clean with 0 warnings.

• genuinely empty card → empty list, no throw
• device never answers → SdCardListIncompleteException, and the previously-cached listing is not overwritten
• truncated listing → throws rather than returning a short list, partial response preserved in RawDeviceResponse
• terminator sent after the LIST, and never parsed as a file
• a stale terminator ahead of the listing (late reply from a previous timed-out exchange) still yields the full listing — the split scans from the end for exactly this reason
• missing terminator on the first attempt only → retried, succeeds
• the throw still restores the LAN interface
• ScpiResponseClassifier.IsSystemErrorReplyLine match/non-match table, including <path> <size> listing entries and **ERROR:-prefixed lines
• a line that arrived before the exchange sent anything is dropped, and one that arrives after is kept (DaqifiDeviceStaleTextLineTests)

The SD test doubles now model the device rather than replaying canned lines verbatim: they answer SYSTem:ERRor? when the exchange asked for it, with an UnterminatedAttempts knob to simulate a device that went silent.

Siblings

Companion to #394 / #395 / #398, worked in parallel. No semantic overlap: #395 owns the typed connectivity guards at the top of ExecuteTextCommandCoreAsync, #398 owns the GET-side empty-transfer ambiguity in SdCardFileReceiver. This PR touches neither: the only change inside ExecuteTextCommandCoreAsync is to the timeout/return semantics of its body (the stale-line boundary from review round 1), not to the connectivity guards at its top. Textual conflicts in DaqifiStreamingDevice.cs are possible.

Review round 1 (Qodo)

Two bugs found, both valid, both fixed in 918c32c.

Stale terminator could false-complete. Content alone cannot separate a stale terminator-shaped line from a fresh one — that is the same ambiguity this PR is about, one level up. What separates them is position relative to our own commands, so ExecuteTextCommandCoreAsync now marks the boundary: it snapshots the collected-line count immediately before the setup action runs and drops those lines from the result. A line captured before the exchange sent anything cannot be a response to a command that had not yet gone out, so this is right for every caller, not just the listing — and it needs no signature change. Covered by DaqifiDeviceStaleTextLineTests, which releases the stale line at the exact moment the exchange binds its text consumer (keyed off the Stream property access, so no timing dependence); verified to fail without the fix.

This is the one place the change reaches outside GetSdCardFilesAsync, and it is the timeout/return semantics of the exchange body rather than the connectivity guards at its top — still disjoint from #395.

Terminator could be missed by the completion window. Requiring a reply while keeping the 250ms inactivity default was the wrong trade — the firmware walks the directory tree between chunks. The listing exchange now uses a dedicated SD_LIST_COMPLETION_TIMEOUT_MS = 1000; other text queries keep the default, having no terminator to wait for.

Re-benched after both fixes: listing still returns all 31 files, and --sd-storage, --lan-chip-info and a 10 Hz two-channel stream all still pass — the last three specifically to prove the shared-exchange change did not regress the non-SD text paths. Full suite green on net9.0 and net10.0 (1958 passed), 0 warnings.

Review round 2 (Qodo)

One bug, valid, fixed in a82c3af.

Stale lines could still leak through a gap inside the setup action. The round-1 boundary only rules out lines that predate the setup action, not ones arriving during a gap within it — for the listing, the 100ms SPI settle delay. PrepareSdInterface() and that delay now run before the exchange opens, so the exchange has no internal gap and its first act is the LIST query; the boundary collapses from a timed wait to thread scheduling. From the device's side the delay is unchanged, if anything longer, since the consumer swap now follows it.

I did not take the suggested Send()-based gate: it flips on the first outbound write, and the listing setup action starts by sending the prep commands, so it would already be true before the settle delay — leaving the exact window it was meant to close. Details on the thread.

Re-benched: 31 files on three consecutive listings, --sd-storage and a 10 Hz two-channel stream unaffected.

Review rounds 3-4 (Qodo)

Round 3 — the settle delay lost its ConfigureAwait(false) when round 2 moved it out of the setup action lambda. Restored in 2d7952f, and applied to the retry-gap delay in the same loop.

Round 4 — Qodo re-raised the stale-line finding, but its quoted evidence ("the setup action ... awaits a settle delay before sending the LIST + terminator") no longer matches the code: that has not been true since a82c3af. Verified and explained on the thread.

The general point was still live elsewhere, though. Grepping every ExecuteTextCommandAsync call site turned up the identical pattern in DeleteSdCardFileAsync, at both its first-attempt and retry sites — the last two async ct => setup actions in the file. Fixed the same way in 0c2f9c0; every call site is now a synchronous lambda with no internal gap. The consequence there was milder than the listing's: delete keys off ContainsScpiError rather than a terminator, so a stale error line would have caused a spurious delete-and-relist retry rather than an incomplete listing being accepted.

This supersedes the scope note above: the delete path's gap is now fixed. What is still deliberately left alone there is the terminator itself, for the reason given — a lost reply's meaning for whether the delete succeeded is a separate question.

The delete path is deliberately not bench-verified: confirming it on hardware means actually deleting a file from the shared bench card. The reorder is byte-identical to the one validated on the listing path against real firmware.

closes #396

Not merging — opened for review.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Fix SD LIST timeout vs empty-card by adding an in-band terminator probe

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Append SYSTem:ERRor? after SYSTem:STORage:SD:LIST? to act as an in-band terminator.
• Throw SdCardListIncompleteException when the terminator is missing (timeout/truncation).
• Add classifier + tests to ensure terminator lines never parse as SD files.

Diagram

sequenceDiagram
participant Caller
participant Core as "DaqifiStreamingDevice"
participant FW as "Device/Firmware"
participant Classifier as "ScpiResponseClassifier"
participant Parser as "SdCardFileListParser"

Caller->>Core: GetSdCardFilesAsync()
Core->>FW: "SYSTem:STORage:SD:LIST?"
Core->>FW: "SYSTem:ERRor?" (terminator)
FW-->>Core: listing lines (0..N)
FW-->>Core: "<code>,\"<msg>\"" (error-queue reply)
Core->>Classifier: IsSystemErrorReplyLine(last line)
alt Terminator present
Core->>Parser: ParseFileList(listing-only lines)
Core-->>Caller: IReadOnlyList<SdCardFileInfo>
else Terminator missing
Core-->>Caller: throw SdCardListIncompleteException(raw lines)
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Firmware emits explicit end-of-list marker

• ➕ Unambiguous and purpose-built terminator; no SCPI error-queue side effect
• ➕ Reduces host-side heuristics and parsing complexity
• ➖ Requires firmware release and upgrade coordination
• ➖ Doesn't help users on older firmware in the field

2. Separate “liveness” query after LIST (new exchange)

• ➕ No need to consume an SCPI error-queue entry
• ➕ Simpler host implementation
• ➖ Does not prove the LIST response was complete (can still truncate)
• ➖ Does not disambiguate empty vs no-response when LIST yields zero bytes

3. Length-prefixed or chunk-counted listing protocol

• ➕ Detects truncation deterministically
• ➕ Avoids relying on error-queue formatting
• ➖ Protocol change on firmware and client; higher coordination cost
• ➖ More invasive than needed for api: a timed-out SD LIST is indistinguishable from an empty SD card #396

Recommendation: The chosen approach (in-band SYSTem:ERRor? terminator appended to the same text exchange) is the best practical fix without firmware changes: it leverages ordering guarantees to detect both total silence and mid-stream truncation, while keeping blast radius limited to GetSdCardFilesAsync. The documented trade-off (consuming one SCPI error-queue entry per listing) is acceptable given the correctness win and targeted scope.

Files changed (6) +488 / -37

Bug fix (3) +223 / -29

DaqifiStreamingDevice.csUse in-band 'SYSTem:ERRor?' reply as SD LIST terminator and throw on incomplete listings +119/-29

Use in-band 'SYSTem:ERRor?' reply as SD LIST terminator and throw on incomplete listings

• Appends 'SYSTem:ERRor?' immediately after 'SD:LIST?' within the same text exchange, then splits the response at the terminator (scanning from the end to avoid stale leading replies). Retries the listing when either a transient SCPI error is present or the terminator is missing, and throws 'SdCardListIncompleteException' if completion is never proven; only complete listings update the cached '_sdCardFiles'.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

ScpiResponseClassifier.csAdd 'IsSystemErrorReplyLine' for IEEE 488.2 error-queue replies +61/-0

Add 'IsSystemErrorReplyLine' for IEEE 488.2 error-queue replies

• Implements a narrow detector for 'SYSTem:ERRor?' response lines ('<code>,"<message>"') including optional sign and whitespace, plus a small whitespace-skip helper. This enables reliably identifying the SD LIST terminator without matching listing content.

src/Daqifi.Core/Device/ScpiResponseClassifier.cs

SdCardListIncompleteException.csIntroduce 'SdCardListIncompleteException' to represent missing/truncated SD LIST replies +43/-0

Introduce 'SdCardListIncompleteException' to represent missing/truncated SD LIST replies

• Adds a dedicated exception type (derived from 'SdCardOperationException') carrying the raw device response and explaining the semantic difference between “empty directory” and “unknown listing due to incomplete response”.

src/Daqifi.Core/Device/SdCard/SdCardListIncompleteException.cs

Tests (2) +260 / -8

ScpiResponseClassifierTests.csAdd tests for 'SYSTem:ERRor?' reply-line detection +29/-0

Add tests for 'SYSTem:ERRor?' reply-line detection

• Introduces positive/negative theory cases for 'IsSystemErrorReplyLine', ensuring it matches IEEE 488.2 error-queue replies while rejecting SD listing entries and other non-replies.

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

SdCardOperationsTests.csCover terminator-based SD LIST completion and extend SD device fakes +231/-8

Cover terminator-based SD LIST completion and extend SD device fakes

• Adds a focused test region for issue #396: verifies command ordering, ensures terminator replies aren’t parsed as files, throws on missing/truncated terminator, handles stale terminator lines, retries on first-miss, and always restores LAN interface on failure. Updates test device fakes to append a synthetic 'SYSTem:ERRor?' reply only when requested and not in simulated “unterminated” attempts.

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs

Documentation (1) +5 / -0

ISdCardOperations.csDocument new incomplete-listing exception for SD file listings +5/-0

Document new incomplete-listing exception for SD file listings

• Updates XML documentation for 'GetSdCardFilesAsync' to specify 'SdCardListIncompleteException' when the device doesn’t answer or truncates the listing, clarifying that an empty result now always means an empty card.

src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs

[tylerkron]

/agentic_review

[qodo-code-review]

Code Review by Qodo

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

1. Stale lines still leak 🐞 Bug ≡ Correctness

Description

ExecuteTextCommandCoreAsync snapshots staleLineCount before setupActionAsync executes, so any late
replies that arrive after that snapshot but before the exchange has actually sent its relevant
command(s) are treated as part of the new response. This can still let a stale
SYSTem:ERRor?-shaped line satisfy SD LIST terminator logic and incorrectly mark an incomplete
listing as complete.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R922-932]

+                    // Mark the boundary between "already in flight" and "answers to this exchange".
+                    // Anything captured before the setup action has sent anything is a late reply to
+                    // an EARLIER command, or line noise — never a response to a command this exchange
+                    // has yet to send. Those lines are dropped from the result below.
+                    //
+                    // Position matters as much as content: a caller that keys off response content —
+                    // e.g. the SD listing's end-of-listing terminator (#396) — would otherwise accept
+                    // a stale line as proof that the device answered a query it never even received,
+                    // and report a complete listing for a device that has gone silent.
+                    staleLineCount = collectedLines.Count;
+                    if (staleLineCount > 0)

Relevance

●●● Strong

Team accepts ExecuteTextCommandAsync race-window fixes; similar async timing issues fixed in #226
and locking/race fixes in #196.

PR-#226
PR-#196

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The boundary is captured before running the setup action, so lines arriving during the async setup
sequence are not considered stale. SD LIST’s setup action includes an awaited delay before sending
the list/terminator commands, creating a window where stale lines can arrive and later be
interpreted as part of the SD LIST exchange (including as the terminator).

src/Daqifi.Core/Device/DaqifiDevice.cs[922-945]
src/Daqifi.Core/Device/DaqifiDevice.cs[1012-1016]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1657-1670]
src/Daqifi.Core/Device/DaqifiDevice.cs[741-745]

Agent prompt

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

### Issue description
`ExecuteTextCommandCoreAsync` currently defines “stale” as whatever was collected *before* `setupActionAsync` is awaited, by snapshotting `staleLineCount = collectedLines.Count`. That misses lines that arrive after the snapshot but still before the exchange has meaningfully sent the command(s) whose replies the caller will interpret, so stale replies can still be misattributed.

### Issue Context
This is particularly risky for the SD LIST path, where the setup action sends preparatory commands and then `await`s a settle delay before sending the LIST + `SYSTem:ERRor?` terminator query; a stale `SYSTem:ERRor?` reply arriving during that gap can be treated as the exchange terminator.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[922-938]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1012-1016]

### Suggested fix
Replace the “snapshot count then Skip()” approach with a per-exchange ‘has sent anything yet’ gate:
1. In `ExecuteTextCommandCoreAsync`, initialize a shared (thread-safe) flag/counter for the current exchange, e.g. `_textExchangeHasSentAnything = false`.
2. In `Send<T>()`, when running inside a text exchange (you already set `_isInsideTextExchange.Value = true` for the exchange thread), set the shared flag/counter to true when the first outbound write is actually performed.
3. In the text consumer’s `MessageReceived` handler, **do not append** lines while the shared flag/counter indicates “no sends yet” (optionally increment a stale counter for logging).
4. Remove (or reduce) reliance on `staleLineCount` + `Skip()`, since the stale data never enters `collectedLines`.

This makes the boundary independent of timing (thread scheduling, the 50ms startup delay, and async gaps inside `setupActionAsync`) and matches the method’s documentation contract more closely.

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

---

2. Stale terminator false-completes ✓ Resolved 🐞 Bug ≡ Correctness

Description

TrySplitAtSdListTerminator treats any SYSTem:ERRor?-shaped line as the completion marker, so a
stale buffered error-queue reply can make GetSdCardFilesAsync mark the response complete even if
the device never answered the new LIST+terminator exchange. This can reintroduce the original
failure mode by returning an empty list (or otherwise trusting an incomplete response) instead of
throwing SdCardListIncompleteException.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R1706-1724]

+            // Scan from the end. A terminator reply from a PREVIOUS, timed-out exchange can still
+            // be sitting in the transport buffer and lead this response; splitting at the first
+            // match would then discard the listing that follows it and report an empty card —
+            // exactly the failure this terminator exists to prevent.
+            var terminatorIndex = -1;
+            for (var i = lines.Count - 1; i >= 0; i--)
+            {
+                if (ScpiResponseClassifier.IsSystemErrorReplyLine(lines[i]))
+                {
+                    terminatorIndex = i;
+                    break;
+                }
+            }
+
+            if (terminatorIndex < 0)
+            {
+                listingLines = lines;
+                return false;
+            }

Relevance

●● Moderate

Some precedent for terminator/queue-shape hardening (PR #185), but no clear history for
flushing/anti-stale completion semantics here.

PR-#185
PR-#149

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

ExecuteTextCommandCoreAsync starts the line-based consumer and waits before running
setupActionAsync, so already-buffered replies can be added to collectedLines.
TrySplitAtSdListTerminator then scans for any IsSystemErrorReplyLine and returns true once it
finds one, even if it's the only line, which can cause GetSdCardFilesAsync to treat a stale
buffered reply as the terminator and proceed without throwing.

src/Daqifi.Core/Device/DaqifiDevice.cs[897-918]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1627-1666]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1702-1742]

Agent prompt

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

### Issue description
`GetSdCardFilesAsync` relies on detecting a `SYSTem:ERRor?` reply line as an end-of-listing terminator. However, `ExecuteTextCommandCoreAsync` can collect already-buffered text lines before it sends the new LIST/ERR? commands, and `TrySplitAtSdListTerminator` will accept such a buffered line as the terminator even if no fresh terminator ever arrived.

### Issue Context
- The text consumer starts and can collect buffered lines before the setup action sends commands.
- The splitter considers the presence of *any* system-error-reply-shaped line (even as the only line) as proof of completion.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1627-1684]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1702-1742]
- src/Daqifi.Core/Device/DaqifiDevice.cs[897-918]

### Implementation direction
Pick one (or combine):
1) **Flush buffered inbound text before sending LIST/ERR?** for this exchange (or add an opt-in flag to `ExecuteTextCommandAsync`), so stale replies cannot be mistaken for the terminator.
2) **Make completion require a fresh marker**, e.g. send **two** `SYSTem:ERRor?` queries in the same exchange and require seeing two system-error-reply lines (use the last as the terminator). This forces at least one post-LIST reply to arrive even if one stale reply was buffered.
3) Add a regression test that simulates: `lines == ["-200,\"Execution error\""]` (stale) and no further replies; assert `SdCardListIncompleteException` is thrown and cache is not overwritten.

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

---

3. Timeout can miss terminator ✓ Resolved 🐞 Bug ☼ Reliability

Description

GetSdCardFilesAsync appends SYSTem:ERRor? but does not increase completionTimeoutMs, so after
the first listing line arrives the collection phase can end after 250ms of inactivity and miss a
slightly-delayed terminator reply. This can throw SdCardListIncompleteException even when the
device would have completed the exchange shortly thereafter.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R1644-1658]

+                    lines = await ExecuteTextCommandAsync(async ct =>
+                    {
+                        PrepareSdInterface();

-                        if (!ContainsScpiError(lines))
-                        {
-                            break;
-                        }
+                        // Allow the device firmware to complete the SPI bus switch
+                        // before querying the SD card. Without this delay, the device
+                        // can return SCPI error -200 (Execution error).
+                        await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, ct).ConfigureAwait(false);
+
+                        Send(ScpiMessageProducer.GetSdFileList);
+
+                        // End-of-listing terminator — see this method's remarks. Sent inside the
+                        // same text exchange so the ordering guarantee holds.
+                        Send(ScpiMessageProducer.GetSystemError);
+                    }, responseTimeoutMs: 3000, cancellationToken: cancellationToken);

Relevance

●●● Strong

Team accepts timeout-window robustness for ExecuteTextCommandAsync paths (phase split/timeout tuning
in PR #126; SD async timing fixes in PR #226).

PR-#126
PR-#226

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The SD list exchange sends both GetSdFileList and the terminator query in a single
ExecuteTextCommandAsync call without overriding completionTimeoutMs. In the core text-collection
loop, once any line is received, the method stops collecting after completionTimeoutMs (default
250ms) of inactivity, which can exclude a delayed terminator reply from lines and make terminator
detection fail.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1644-1664]
src/Daqifi.Core/Device/DaqifiDevice.cs[920-959]

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 SD LIST exchange now includes an extra query (`SYSTem:ERRor?`) whose reply must be received to consider the listing complete, but the call keeps the default `completionTimeoutMs=250`. Once *any* line has been received, the text exchange terminates after 250ms of inactivity, which can drop a delayed terminator reply.

### Issue Context
`ExecuteTextCommandCoreAsync` switches to the short completion timeout immediately after the first received line; this is fine for single-burst responses, but the added terminator introduces an additional post-listing reply that may arrive after a small gap.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1644-1658]
- src/Daqifi.Core/Device/DaqifiDevice.cs[920-959]

### Implementation direction
- In `GetSdCardFilesAsync`, pass an explicit larger `completionTimeoutMs` for the LIST+terminator exchange (e.g. 1000–3000ms, or a dedicated constant).
- Optionally add a unit test using a fake transport/consumer that delivers the terminator reply after >250ms following at least one listing line, asserting the method succeeds rather than throwing `SdCardListIncompleteException`.

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

---

4. Delay captures sync context ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

The new settle-delay await after PrepareSdInterface() does not use ConfigureAwait(false), so its
continuation may attempt to resume on a captured SynchronizationContext. In UI/legacy environments,
callers that block synchronously on GetSdCardFilesAsync (sync-over-async) are more likely to hang
than if the continuation were context-free.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1675]

+                    await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken);

Relevance

●●● Strong

Team has accepted adding ConfigureAwait(false) in library async paths to avoid sync-context
deadlocks (e.g. #94).

PR-#94
PR-#226

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The focused hunk shows the new delay without ConfigureAwait(false). In the text exchange core,
delays explicitly use ConfigureAwait(false) as part of the library’s pattern to avoid
context-related deadlocks/hangs.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1674-1676]
src/Daqifi.Core/Device/DaqifiDevice.cs[915-919]

Agent prompt

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

### Issue description
A newly added `await Task.Delay(..., cancellationToken)` in `GetSdCardFilesAsync` omits `.ConfigureAwait(false)`, causing unnecessary sync-context capture.

### Issue Context
Elsewhere in the device command path, waits use `ConfigureAwait(false)` to avoid resuming on a UI thread / captured context.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1674-1676]

### What to change
Update the settle delay to:
```csharp
await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false);
```

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

---

5. Unsynchronized SD interface switch 🐞 Bug ☼ Reliability ⭐ New

Description

GetSdCardFilesAsync now calls PrepareSdInterface() and awaits the settle delay before entering
ExecuteTextCommandAsync, so another concurrent text exchange can run in between and change the
SD/LAN SPI-bus state before the LIST+terminator commands are sent. This can cause intermittent SD
listings to fail, time out, or run against the wrong interface state depending on what the
interleaving operation does.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R1674-1678]

+                    PrepareSdInterface();
+                    await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken);
+
+                    lines = await ExecuteTextCommandAsync(
+                        () =>

Relevance

● Weak

PR’s design rationale explicitly moves SD switch+delay outside text exchange to avoid stale
terminator window.

PR-#196
PR-#226

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The focused hunk shows the SD/LAN switch and delay occur before acquiring the exchange lock (they
happen before ExecuteTextCommandAsync(...)). ExecuteTextCommandCoreAsync acquires
_textExchangeLock and runs the setup action inside that lock, indicating the intended
serialization boundary for multi-command exchanges; moving interface switching outside that boundary
creates an interleaving window.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1664-1688]
src/Daqifi.Core/Device/DaqifiDevice.cs[816-941]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1818-1828]

Agent prompt

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

### Issue description
`GetSdCardFilesAsync` prepares the SD interface (and waits the settle delay) *outside* the `ExecuteTextCommandAsync`-protected text exchange. Because `ExecuteTextCommandAsync` is the only place that acquires the device’s `_textExchangeLock`, another text exchange can run during that pre-exchange delay and toggle interfaces (e.g., `PrepareLanInterface()` in a `finally`), leaving this SD LIST exchange to run with SD disabled / LAN re-enabled.

### Issue Context
- This PR moved `PrepareSdInterface()` + settle delay out of the exchange to tighten the stale-line boundary.
- However, it unintentionally introduces a race window where interface state is modified without holding the text-exchange lock.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1664-1688]

### What to change
- Ensure the SD interface switch + settle delay are serialized with the subsequent LIST+terminator exchange.
- One robust approach is to extend the text-exchange API so callers can run a **pre-send** step under `_textExchangeLock` but **before** the text consumer starts / stale boundary is captured (so you can keep the “no internal gap before first send” guarantee).
- Then move `PrepareSdInterface()` + settle delay into that locked pre-send step, and only after that start the exchange and immediately send `GetSdFileList` and `GetSystemError`.

ⓘ 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 0c2f9c0

Results up to commit c15bdce ⚖️ Balanced

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

1. Stale terminator false-completes ✓ Resolved 🐞 Bug ≡ Correctness

Description

TrySplitAtSdListTerminator treats any SYSTem:ERRor?-shaped line as the completion marker, so a
stale buffered error-queue reply can make GetSdCardFilesAsync mark the response complete even if
the device never answered the new LIST+terminator exchange. This can reintroduce the original
failure mode by returning an empty list (or otherwise trusting an incomplete response) instead of
throwing SdCardListIncompleteException.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R1706-1724]

+            // Scan from the end. A terminator reply from a PREVIOUS, timed-out exchange can still
+            // be sitting in the transport buffer and lead this response; splitting at the first
+            // match would then discard the listing that follows it and report an empty card —
+            // exactly the failure this terminator exists to prevent.
+            var terminatorIndex = -1;
+            for (var i = lines.Count - 1; i >= 0; i--)
+            {
+                if (ScpiResponseClassifier.IsSystemErrorReplyLine(lines[i]))
+                {
+                    terminatorIndex = i;
+                    break;
+                }
+            }
+
+            if (terminatorIndex < 0)
+            {
+                listingLines = lines;
+                return false;
+            }

Relevance

●● Moderate

Some precedent for terminator/queue-shape hardening (PR #185), but no clear history for
flushing/anti-stale completion semantics here.

PR-#185
PR-#149

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

ExecuteTextCommandCoreAsync starts the line-based consumer and waits before running
setupActionAsync, so already-buffered replies can be added to collectedLines.
TrySplitAtSdListTerminator then scans for any IsSystemErrorReplyLine and returns true once it
finds one, even if it's the only line, which can cause GetSdCardFilesAsync to treat a stale
buffered reply as the terminator and proceed without throwing.

src/Daqifi.Core/Device/DaqifiDevice.cs[897-918]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1627-1666]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1702-1742]

Agent prompt

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

### Issue description
`GetSdCardFilesAsync` relies on detecting a `SYSTem:ERRor?` reply line as an end-of-listing terminator. However, `ExecuteTextCommandCoreAsync` can collect already-buffered text lines before it sends the new LIST/ERR? commands, and `TrySplitAtSdListTerminator` will accept such a buffered line as the terminator even if no fresh terminator ever arrived.

### Issue Context
- The text consumer starts and can collect buffered lines before the setup action sends commands.
- The splitter considers the presence of *any* system-error-reply-shaped line (even as the only line) as proof of completion.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1627-1684]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1702-1742]
- src/Daqifi.Core/Device/DaqifiDevice.cs[897-918]

### Implementation direction
Pick one (or combine):
1) **Flush buffered inbound text before sending LIST/ERR?** for this exchange (or add an opt-in flag to `ExecuteTextCommandAsync`), so stale replies cannot be mistaken for the terminator.
2) **Make completion require a fresh marker**, e.g. send **two** `SYSTem:ERRor?` queries in the same exchange and require seeing two system-error-reply lines (use the last as the terminator). This forces at least one post-LIST reply to arrive even if one stale reply was buffered.
3) Add a regression test that simulates: `lines == ["-200,\"Execution error\""]` (stale) and no further replies; assert `SdCardListIncompleteException` is thrown and cache is not overwritten.

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

---

2. Timeout can miss terminator ✓ Resolved 🐞 Bug ☼ Reliability

Description

GetSdCardFilesAsync appends SYSTem:ERRor? but does not increase completionTimeoutMs, so after
the first listing line arrives the collection phase can end after 250ms of inactivity and miss a
slightly-delayed terminator reply. This can throw SdCardListIncompleteException even when the
device would have completed the exchange shortly thereafter.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R1644-1658]

+                    lines = await ExecuteTextCommandAsync(async ct =>
+                    {
+                        PrepareSdInterface();

-                        if (!ContainsScpiError(lines))
-                        {
-                            break;
-                        }
+                        // Allow the device firmware to complete the SPI bus switch
+                        // before querying the SD card. Without this delay, the device
+                        // can return SCPI error -200 (Execution error).
+                        await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, ct).ConfigureAwait(false);
+
+                        Send(ScpiMessageProducer.GetSdFileList);
+
+                        // End-of-listing terminator — see this method's remarks. Sent inside the
+                        // same text exchange so the ordering guarantee holds.
+                        Send(ScpiMessageProducer.GetSystemError);
+                    }, responseTimeoutMs: 3000, cancellationToken: cancellationToken);

Relevance

●●● Strong

Team accepts timeout-window robustness for ExecuteTextCommandAsync paths (phase split/timeout tuning
in PR #126; SD async timing fixes in PR #226).

PR-#126
PR-#226

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The SD list exchange sends both GetSdFileList and the terminator query in a single
ExecuteTextCommandAsync call without overriding completionTimeoutMs. In the core text-collection
loop, once any line is received, the method stops collecting after completionTimeoutMs (default
250ms) of inactivity, which can exclude a delayed terminator reply from lines and make terminator
detection fail.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1644-1664]
src/Daqifi.Core/Device/DaqifiDevice.cs[920-959]

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 SD LIST exchange now includes an extra query (`SYSTem:ERRor?`) whose reply must be received to consider the listing complete, but the call keeps the default `completionTimeoutMs=250`. Once *any* line has been received, the text exchange terminates after 250ms of inactivity, which can drop a delayed terminator reply.

### Issue Context
`ExecuteTextCommandCoreAsync` switches to the short completion timeout immediately after the first received line; this is fine for single-burst responses, but the added terminator introduces an additional post-listing reply that may arrive after a small gap.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1644-1658]
- src/Daqifi.Core/Device/DaqifiDevice.cs[920-959]

### Implementation direction
- In `GetSdCardFilesAsync`, pass an explicit larger `completionTimeoutMs` for the LIST+terminator exchange (e.g. 1000–3000ms, or a dedicated constant).
- Optionally add a unit test using a fake transport/consumer that delivers the terminator reply after >250ms following at least one listing line, asserting the method succeeds rather than throwing `SdCardListIncompleteException`.

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

---

Results up to commit 918c32c ⚖️ Balanced

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

1. Stale lines still leak 🐞 Bug ≡ Correctness

Description

ExecuteTextCommandCoreAsync snapshots staleLineCount before setupActionAsync executes, so any late
replies that arrive after that snapshot but before the exchange has actually sent its relevant
command(s) are treated as part of the new response. This can still let a stale
SYSTem:ERRor?-shaped line satisfy SD LIST terminator logic and incorrectly mark an incomplete
listing as complete.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R922-932]

+                    // Mark the boundary between "already in flight" and "answers to this exchange".
+                    // Anything captured before the setup action has sent anything is a late reply to
+                    // an EARLIER command, or line noise — never a response to a command this exchange
+                    // has yet to send. Those lines are dropped from the result below.
+                    //
+                    // Position matters as much as content: a caller that keys off response content —
+                    // e.g. the SD listing's end-of-listing terminator (#396) — would otherwise accept
+                    // a stale line as proof that the device answered a query it never even received,
+                    // and report a complete listing for a device that has gone silent.
+                    staleLineCount = collectedLines.Count;
+                    if (staleLineCount > 0)

Relevance

●●● Strong

Team accepts ExecuteTextCommandAsync race-window fixes; similar async timing issues fixed in #226
and locking/race fixes in #196.

PR-#226
PR-#196

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The boundary is captured before running the setup action, so lines arriving during the async setup
sequence are not considered stale. SD LIST’s setup action includes an awaited delay before sending
the list/terminator commands, creating a window where stale lines can arrive and later be
interpreted as part of the SD LIST exchange (including as the terminator).

src/Daqifi.Core/Device/DaqifiDevice.cs[922-945]
src/Daqifi.Core/Device/DaqifiDevice.cs[1012-1016]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1657-1670]
src/Daqifi.Core/Device/DaqifiDevice.cs[741-745]

Agent prompt

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

### Issue description
`ExecuteTextCommandCoreAsync` currently defines “stale” as whatever was collected *before* `setupActionAsync` is awaited, by snapshotting `staleLineCount = collectedLines.Count`. That misses lines that arrive after the snapshot but still before the exchange has meaningfully sent the command(s) whose replies the caller will interpret, so stale replies can still be misattributed.

### Issue Context
This is particularly risky for the SD LIST path, where the setup action sends preparatory commands and then `await`s a settle delay before sending the LIST + `SYSTem:ERRor?` terminator query; a stale `SYSTem:ERRor?` reply arriving during that gap can be treated as the exchange terminator.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[922-938]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1012-1016]

### Suggested fix
Replace the “snapshot count then Skip()” approach with a per-exchange ‘has sent anything yet’ gate:
1. In `ExecuteTextCommandCoreAsync`, initialize a shared (thread-safe) flag/counter for the current exchange, e.g. `_textExchangeHasSentAnything = false`.
2. In `Send<T>()`, when running inside a text exchange (you already set `_isInsideTextExchange.Value = true` for the exchange thread), set the shared flag/counter to true when the first outbound write is actually performed.
3. In the text consumer’s `MessageReceived` handler, **do not append** lines while the shared flag/counter indicates “no sends yet” (optionally increment a stale counter for logging).
4. Remove (or reduce) reliance on `staleLineCount` + `Skip()`, since the stale data never enters `collectedLines`.

This makes the boundary independent of timing (thread scheduling, the 50ms startup delay, and async gaps inside `setupActionAsync`) and matches the method’s documentation contract more closely.

ⓘ 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 918c32c

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 2d7952f

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Post-merge note, for anyone reading the two findings that Qodo left unmarked in the accumulated review body. Both have a verified position; neither is an open bug at c2d5da8, and one of them did produce a real follow-up.

"Stale lines still leak" — not a defect at the merged commit. The finding's quoted evidence is that the listing setup action "sends preparatory commands and then awaits a settle delay before sending the LIST + SYSTem:ERRor? terminator query". That was true at 918c32c and stopped being true at a82c3af. Re-verified against the merged tree: the setup action is two synchronous Send() calls with no await between them, so there is no gap inside the exchange for a stale reply to land in. The general point was still live elsewhere, in DeleteSdCardFileAsync — found by grepping every call site, and fixed in 0c2f9c0, which is in this merge.

"Unsynchronized SD interface switch" — real, and NOT fixed by this merge. Qodo rated it Weak, but the mechanism holds and I now think it was underrated. ExecuteTextCommandCoreAsync runs its setup action while holding _textExchangeLock; the bus switch used to be inside that lock because it was inside the setup action, and a82c3af hoisted it out:

thread A: PrepareSdInterface()                 <- outside the lock
thread B: acquires _textExchangeLock, runs another text exchange,
          whose finally calls PrepareLanInterface()
thread A: acquires the lock, sends LIST        <- against LAN interface state

Closing the stale-terminator window opened an interface-interleaving one. Reachable, since concurrent text operations are exactly what _textExchangeLock exists for (#186).

Fixed in #406, which splits the setup rather than choosing between the two: a prepare phase that runs inside the lock and ahead of the stale-line boundary, so the switch stays serialized and the setup action stays gap-free. Bench-verified on the Nyquist 1, full suite green on net9.0 and net10.0.

#406 also flags one thing neither PR addresses: the restore side (PrepareLanInterface() in each SD method's finally) is still outside the lock. That predates all of this work and is on main today.