refactor(device): consolidate SCPI error-code extraction into ScpiResponseClassifier (part of #345)

[tylerkron]

Summary

DaqifiStreamingDevice.TryParseScpiErrorCode independently re-derived the **ERROR/ERROR prefix + delimiter-trim logic that ScpiResponseClassifier already owns for line detection — the exact drift risk #345 item 3 calls out. Extraction now lives next to the matchers as ScpiResponseClassifier.TryExtractErrorCode, so detection and extraction share one file (and one TokenDelimiters set); the device's two call sites delegate to it and the private duplicate is deleted. Behavior-preserving.

Changes

• ScpiResponseClassifier.TryExtractErrorCode(line, out code) (new, internal) — the moved extraction logic, plus a single-sourced private static readonly char[] TokenDelimiters = { ':', ' ', '\t' } documenting the accepted delimiters in one place.
• DaqifiStreamingDevice — both call sites (the SCPI-error classification and the -200 reboot-detection path) now call ScpiResponseClassifier.TryExtractErrorCode; the private TryParseScpiErrorCode is removed.

Testing

• dotnet test — 1647 passed / 0 failed / 2 skipped (net9.0 + net10.0); existing error-parse behavior preserved through the delegating call sites.
• 12 new TryExtractErrorCode unit tests: all :/space/tab delimiter variants across **ERROR/ERROR, no-trailing-comma, positive code, leading/trailing whitespace + CRLF; and non-numeric / non-error / filename lines → false with code == 0.
• No bench validation: this is pure SCPI-text parsing relocated with identical behavior — it has no device-facing surface to exercise (fully covered by unit tests).

Scope note

Closes acceptance-criterion 3 of #345 — the last of the three batch items (item 1 = PWM skip #356, item 2 = SetFriendlyNameAsync #355). #345 is fully addressed once all three merge; not adding a closes keyword here so the issue isn't closed before #355/#356 land.

Not merging — for review.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Consolidate SCPI error-code extraction into ScpiResponseClassifier

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Centralize SCPI error-code extraction to avoid delimiter/prefix drift between detection and
 parsing.
• Update DaqifiStreamingDevice call sites to delegate error-code parsing to ScpiResponseClassifier.
• Add unit tests covering delimiter variants, whitespace/CRLF, and non-error/non-numeric cases.

Diagram

graph TD
  device["DaqifiStreamingDevice"] --> classifier["ScpiResponseClassifier"] --> delims["TokenDelimiters"]
  tests["ScpiResponseClassifierTests"] --> classifier

Loading

High-Level Assessment

Keeping extraction alongside the existing SCPI error matchers is the most maintainable option: it single-sources prefix + delimiter rules and eliminates drift risk. Alternatives like retaining a device-local helper with a shared constant or using a regex add indirection/complexity without improving correctness over the current consolidated utility method.

Files changed (3) +72 / -37

Refactor (2) +46 / -37

DaqifiStreamingDevice.csDelegate SCPI error-code parsing to ScpiResponseClassifier +2/-37

Delegate SCPI error-code parsing to ScpiResponseClassifier

• Replaces two call sites that parsed SCPI error codes with calls to ScpiResponseClassifier.TryExtractErrorCode. Removes the duplicated private TryParseScpiErrorCode implementation to prevent parsing-rule drift.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

ScpiResponseClassifier.csAdd TryExtractErrorCode and single-source delimiter rules +44/-0

Add TryExtractErrorCode and single-source delimiter rules

• Adds internal TryExtractErrorCode to parse numeric SCPI error codes for both ERROR/**ERROR prefixes. Introduces a private TokenDelimiters array and uses invariant-culture integer parsing to keep detection and extraction logic in lockstep.

src/Daqifi.Core/Device/ScpiResponseClassifier.cs

Tests (1) +26 / -0

ScpiResponseClassifierTests.csAdd unit tests for TryExtractErrorCode delimiter and edge cases +26/-0

Add unit tests for TryExtractErrorCode delimiter and edge cases

• Introduces theory-based coverage for SCPI error-code extraction across ':', space, and tab delimiters, including whitespace/CRLF and positive codes. Adds negative cases (non-numeric, non-error, and filename-like lines) asserting false with code == 0.

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

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 chore: small API polish batch (PWM frequency idempotence, SetFriendlyNameAsync, SCPI error-code parsing consolidation)

1. Delimiter source not unified ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

ScpiResponseClassifier documents TokenDelimiters as a single source of truth for both detection and
extraction, but MatchesErrorPrefix/MatchesStrictScpiErrorPrefix still hard-code ':'/' '/'\t' checks.
A future change could update TokenDelimiters (affecting extraction) without updating the matchers
(affecting detection), reintroducing detection/extraction drift.

Code

src/Daqifi.Core/Device/ScpiResponseClassifier.cs[R91-99]

+        /// <summary>
+        /// The delimiters accepted between the <c>ERROR</c>/<c>**ERROR</c> token and the error code.
+        /// Single-sourced here so <see cref="TryExtractErrorCode"/> and the line matchers stay in lockstep.
+        /// </summary>
+        private static readonly char[] TokenDelimiters = { ':', ' ', '\t' };
+
        private static bool MatchesErrorPrefix(string trimmed, string prefix)
        {
            if (trimmed.Length < prefix.Length

Relevance

⭐⭐⭐ High

Team has accepted “single source of truth”/drift-prevention refactors (e.g., unify SCPI predicates,
extension lists).

PR-#182
PR-#318
PR-#195

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The file declares TokenDelimiters and claims it keeps accepted delimiters from drifting, but the
matcher methods still encode the delimiter set inline, meaning the delimiter set is duplicated and
can diverge in future edits.

src/Daqifi.Core/Device/ScpiResponseClassifier.cs[54-135]

Agent prompt

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

### Issue description
`TokenDelimiters` is intended to be the single authoritative delimiter set, but it is only referenced by `TryExtractErrorCode`. The prefix matchers still directly compare `next` against `':'`, `' '`, and `'\t'`, so detection and extraction can drift again if delimiters are updated in one place.

### Issue Context
This PR explicitly aims to prevent delimiter drift between SCPI error-line detection and error-code extraction.

### Fix Focus Areas
- src/Daqifi.Core/Device/ScpiResponseClassifier.cs[91-135]

### Suggested fix
- Add a small helper like `private static bool IsTokenDelimiter(char c)` that checks against `TokenDelimiters` (e.g., `Array.IndexOf(TokenDelimiters, c) >= 0`) and use it in both `MatchesErrorPrefix` and `MatchesStrictScpiErrorPrefix`.
- Alternatively, replace the hard-coded delimiter checks in the matchers with `IsTokenDelimiter(next)`.
- Update the XML doc comment to match the actual implementation (or make the implementation match the doc).

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

---

2. Incorrect XML doc ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New

Description

The TokenDelimiters XML comment claims TryExtractErrorCode consults IsTokenDelimiter, but extraction
actually trims using TokenDelimiters directly. This is misleading documentation that could cause
future delimiter changes to be applied in the wrong place (helper vs. shared array).

Code

src/Daqifi.Core/Device/ScpiResponseClassifier.cs[R93-94]

+        /// Single-sourced here and consulted via <see cref="IsTokenDelimiter"/> by both
+        /// <see cref="TryExtractErrorCode"/> and the line matchers, so detection and extraction can't drift.

Relevance

⭐⭐⭐ High

Team frequently accepts fixes for misleading XML docs mismatching behavior (e.g., doc drift fixes in
PRs #321, #348).

PR-#321
PR-#348
PR-#98

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The comment explicitly claims TryExtractErrorCode consults IsTokenDelimiter, but the extraction
code path trims with TokenDelimiters directly, while only the matchers call IsTokenDelimiter.

src/Daqifi.Core/Device/ScpiResponseClassifier.cs[91-99]
src/Daqifi.Core/Device/ScpiResponseClassifier.cs[65-89]

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 XML doc for `TokenDelimiters` states it is "consulted via `IsTokenDelimiter`" by both `TryExtractErrorCode` and the matchers, but `TryExtractErrorCode` uses `TrimStart(TokenDelimiters)` directly.

## Issue Context
This is a documentation correctness issue (not a runtime bug). The delimiter set is still single-sourced, but the comment is factually inaccurate and may mislead future maintenance.

## Fix Focus Areas
- src/Daqifi.Core/Device/ScpiResponseClassifier.cs[91-99]

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

---

Previous review results

Review updated until commit 55f468e

Results up to commit ff1176f

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

1. Delimiter source not unified ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

ScpiResponseClassifier documents TokenDelimiters as a single source of truth for both detection and
extraction, but MatchesErrorPrefix/MatchesStrictScpiErrorPrefix still hard-code ':'/' '/'\t' checks.
A future change could update TokenDelimiters (affecting extraction) without updating the matchers
(affecting detection), reintroducing detection/extraction drift.

Code

src/Daqifi.Core/Device/ScpiResponseClassifier.cs[R91-99]

+        /// <summary>
+        /// The delimiters accepted between the <c>ERROR</c>/<c>**ERROR</c> token and the error code.
+        /// Single-sourced here so <see cref="TryExtractErrorCode"/> and the line matchers stay in lockstep.
+        /// </summary>
+        private static readonly char[] TokenDelimiters = { ':', ' ', '\t' };
+
        private static bool MatchesErrorPrefix(string trimmed, string prefix)
        {
            if (trimmed.Length < prefix.Length

Relevance

⭐⭐⭐ High

Team has accepted “single source of truth”/drift-prevention refactors (e.g., unify SCPI predicates,
extension lists).

PR-#182
PR-#318
PR-#195

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The file declares TokenDelimiters and claims it keeps accepted delimiters from drifting, but the
matcher methods still encode the delimiter set inline, meaning the delimiter set is duplicated and
can diverge in future edits.

src/Daqifi.Core/Device/ScpiResponseClassifier.cs[54-135]

Agent prompt

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

### Issue description
`TokenDelimiters` is intended to be the single authoritative delimiter set, but it is only referenced by `TryExtractErrorCode`. The prefix matchers still directly compare `next` against `':'`, `' '`, and `'\t'`, so detection and extraction can drift again if delimiters are updated in one place.

### Issue Context
This PR explicitly aims to prevent delimiter drift between SCPI error-line detection and error-code extraction.

### Fix Focus Areas
- src/Daqifi.Core/Device/ScpiResponseClassifier.cs[91-135]

### Suggested fix
- Add a small helper like `private static bool IsTokenDelimiter(char c)` that checks against `TokenDelimiters` (e.g., `Array.IndexOf(TokenDelimiters, c) >= 0`) and use it in both `MatchesErrorPrefix` and `MatchesStrictScpiErrorPrefix`.
- Alternatively, replace the hard-coded delimiter checks in the matchers with `IsTokenDelimiter(next)`.
- Update the XML doc comment to match the actual implementation (or make the implementation match the doc).

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

---

[tylerkron]

Fixed in the latest commit. Added private static bool IsTokenDelimiter(char c) => Array.IndexOf(TokenDelimiters, c) >= 0; and routed both MatchesErrorPrefix and MatchesStrictScpiErrorPrefix through it, so TokenDelimiters is now genuinely the single source for detection and extraction. Behavior-preserving — in the strict matcher ':' is handled by the return true above, so !IsTokenDelimiter(next) reduces exactly to the prior next != ' ' && next != '\t'. Full suite 1647 pass; all delimiter-variant classifier tests green.

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Fixed — reworded the TokenDelimiters doc to be accurate: the line matchers test membership via IsTokenDelimiter, while TryExtractErrorCode trims with the array directly; both draw from the one set. Doc-only, no behavior change.

Note: the earlier CI "build" failure was the unrelated flaky WifiBridgeActivatorTests.RunWithHardTimeoutAsync_HardTimeoutElapses_StopsWorkerBeforeLateStep timing test (2nd recurrence across my PRs) — the fresh push re-runs CI.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 55f468e