feat(device): read the live capability document and merge it into DeviceCapabilities (closes #390)

[tylerkron]

Part 2 of #256. Firmware ≥ v3.5.0 can describe itself; Core now asks, and merges the answer onto
the board-derived capability table.

closes #390

Why now

The trigger recorded on the issue: daqifi-desktop #118
wants a streaming ceiling that depends on the enabled channel set, the channel types, the active
interface, and the encoding. DeviceCapabilities.FromDeviceType() cannot express that, and no
number of rows in DeviceFeatureRequirements can either. Worse, Core was enforcing a wrong
number: MaxSamplingRate is hardcoded to 1000, and since #336 StreamingFrequency throws above
it — so a caller asking for 3 kHz got an ArgumentOutOfRangeException from Core for a rate the
device accepts. That literal is stale on its own terms; firmware v3.5.0 removed the 1 kHz Type-2
muxed scan cap (firmware #528).

Design

SCPI surface. ScpiMessageProducer.GetCapabilitiesApiVersion / .GetCapabilitiesJson.

Parser. Daqifi.Core.Device.Capabilities — CapabilityDocumentParser.TryParse /
TryParseLines / TryParseApiVersion producing a CapabilityDocument (identity, flat
channels[], streaming block, storage/power/transport flags, raw JSON). Written against the
firmware emitter in SCPIInterface.c and cross-checked against the real board, not guessed.
Tolerant throughout: every optional field is nullable and every parse returns false rather than
throwing, because the firmware's schema rules make additive change routine and require clients to
ignore what they do not know.

CapabilityRateModel.TryComputeMaxRateHz(simultaneousCount, totalCount) evaluates the device's own
published formula, which is the client-side preview desktop #118 needs.

Two gates. The read is skipped entirely unless Supports(DeviceFeature.CapabilityDocument)
(already in the table at v3.5.0 from #389), and the document is only trusted when the reported
schema version is within MinimumCapabilityDocumentApiVersion..MaximumCapabilityDocumentApiVersion
(1..2). The upper bound is deliberate — firmware bumps that byte only on a breaking change
(field renamed/removed/retyped, semantics changed, layout reshaped), so a higher version is a
document whose fields may no longer mean what this parser assumes. Falling back to the board table
beats handing a consumer a plausible-but-wrong number. This is the concrete answer to the issue's
"bounds were realigned in firmware #548, don't treat the document as stable across versions"
warning.

Merge, never replace. CapabilityDocument.MergeInto overlays only what the document states;
everything else keeps its FromDeviceType value. Because every parsed field is nullable, an
omitted field can never read as "hardware absent" — which is exactly what preserves the
"Unknown board means not-yet-known" rule Supports() depends on. Two fields are deliberately
never overlaid: HasWincWifiModule (the schema carries no chipset facts by design — it publishes
what a client can do) and SupportsStreaming (the firmware emits the streaming block
unconditionally, so its presence is not evidence either way).

UpdateFromProtobuf had to stop replacing — scope item 6 from the issue comment, and the
load-bearing change. It ran Capabilities = FromDeviceType(DeviceType) on every status message
carrying a part number, which would clobber the overlay mid-session. It was already a latent bug
independent of this issue: a status frame with a part number but no port fields reset the channel
counts to zero. It now rebuilds only when the board the capabilities were derived from actually
changes, and re-applies the document overlay last so the device's own answer outranks both the
board table and the status message's port counts.

That "board the capabilities were derived from" is tracked in its own field rather than compared
against the public DeviceType property — otherwise a consumer that assigns DeviceType itself
and then receives a matching status message would never get board-derived capabilities built at
all.

The refresh point, and why

InitializeAsync, after WaitForChannelsPopulatedAsync and before OnDeviceInitializingAsync.

• Why not status processing. The capability fields are not in the protobuf status message —
  DaqifiOutMessage stops at tag 69, and Capabilities.h:18's claim of "tags 70..75" is
  aspirational, not shipped. This is a genuine SCPI round-trip, and a text exchange pauses the
  protobuf consumer, so it needs a quiescent point rather than a hot path.
• Why after channel population. The board and firmware version arrive in the status message.
  Supports() fails closed on an unknown firmware version, so any earlier read would skip on every
  device.
• Why before OnDeviceInitializingAsync. Derived-class initialization then already sees the
  device's own capabilities.
• Failure is absorbed. A device that cannot answer keeps its board-derived capabilities and
  initializes exactly as before. Cancellation still propagates — that is the caller's request, not
  a device fault.
• Re-reading is the caller's call. current_max_rate_hz is computed from the channel set
  enabled at the moment of the read, so it goes stale as soon as that set changes;
  ReadCapabilityDocumentAsync() is public for exactly that. (Issue scope item 7 — reconciling
  Core's view of the enabled set with protobuf field 22 — is deliberately not in this PR; it is a
  separate change to channel-state tracking.)

Connect latency — measured, then fixed

The naive shape cost real time, so I measured it against origin/main on the bench rather than
guessing:

	connect+initialize
origin/main baseline	3.16 s
naive (2 exchanges, 1500 ms completion window)	6.80 s
completion window 250 ms	5.30 s
both queries in one exchange (this PR)	4.29 s

The 1500 ms window was sized for a worst case I had not measured. On the wire the document is
8014 bytes delivered in ~25 ms end to end with a longest inter-chunk gap of 8 ms, so 250 ms
already carries an order of magnitude of headroom. And swapping the protobuf consumer out and back
dominates the cost — the reader thread must time out of its blocking read first — so a second
exchange bought nothing but latency, to avoid transferring a document that takes 25 ms. The schema
version still gates whether the document is trusted, which is what the gate is for.

Bench validation

DAQiFi Nyquist 1, firmware 3.7.2, /dev/cu.usbmodem1101, over USB CDC. Non-destructive throughout;
the only device state touched was the ADC enable mask, restored to its original 0.

CONFigure:CAPabilities:APIVersion? → 2. The captured document is checked in verbatim as the
parser fixture (CapabilityDocumentSamples.Nyquist1Firmware372, 8010 chars) so the tests run
against what the hardware actually emits.

schema_version : 2
identity       : DAQiFi Nyquist NQ1 sn=7E2815916200E898 fw=3.7.2 hw=2.0.0
channels       : 32 total (16 ain, 0 aout, 16 dio)
simultaneous   : [4,8,10,12,14]
pwm-capable dio: [0,3,4,5,6,7]
streaming      : min=1 max=22000 conservative=500 current=0 validation=error
rate model     : absolute=22000 type1Agg=55000 tickBudget=110000 tickOverhead=6
flags          : sd=True usb=True wifi=True eth=False battery=True

The PWM-capable set {0,3,4,5,6,7} independently matches what we already knew about this board
from firmware source — a good sign the parser is reading the schema correctly rather than
plausibly.

Does the document agree with FromDeviceType?

Yes on everything it states, with one deliberate exception:

  SupportsStreaming      board=True     merged=True     agree
  HasSdCard              board=True     merged=True     agree
  HasWiFi                board=True     merged=True     agree
  HasWincWifiModule      board=True     merged=True     agree
  HasUsb                 board=True     merged=True     agree
  AnalogInputChannels    board=0        merged=16       DIFFERS
  AnalogOutputChannels   board=0        merged=0        agree
  DigitalChannels        board=0        merged=16       DIFFERS
  MaxSamplingRate        board=1000     merged=22000    DIFFERS

• Every hardware flag agrees. No contradiction on anything the static table asserts.
• The channel counts are fills, not contradictions — FromDeviceType leaves them 0 and never
  set them; the protobuf status message supplies them today, and the document now supplies them
  too (and agrees with the protobuf: 16/0/16).
• MaxSamplingRate is the one real disagreement, and it is the point of the exercise. 1000 is
  the stale literal; 22000 is the device's absolute ISR ceiling. This is checked in as a regression
  test (MergeInto_RealNyquist1Document_AgreesWithTheBoardTable) so a future document that starts
  contradicting the table on a hardware flag fails CI.

End to end

• StreamingFrequency = 3000 — accepted (ceiling is now 22000 Hz). On origin/main this
  throws ArgumentOutOfRangeException. That is desktop SD ops should not rely solely on internal IsStreaming state #118 unblocked.
• Actually streamed at 3 kHz for 2 s on channels 0+1: 4737 samples ≈ 2369 samples/s, matching
  the expected 3000 × 0.794 = 2382 for this unit's known PLL clock mismatch (firmware #716). The
  device really does accept and deliver the rate Core used to refuse.
• Refresh works: current_max_rate_hz moved 0 (nothing enabled) → 5713 (channels 0–3 enabled)
  → back after restoring the mask. The rate model previewed 11000 Hz for that selection, correctly
  higher than the device's 5713 since the model excludes the transport cap.
• Streaming regression check via the example CLI (10 Hz, 3 s, channels 0+1): connect, populated
  init, samples flowing, clean stop, exit code 0.

Tests

Full suite green on net9.0 and net10.0: 2006 passed, 0 failed. No new warnings
(TreatWarningsAsErrors is on). New coverage:

• Parser over the real captured document — identity, per-kind channel counts, analog detail,
  dedicated-converter set, PWM-capable set, streaming block, rate-model constants, flags.
• Malformed and partial input: truncated JSON, wrong shape, missing schema_version, retyped
  fields, unknown channel kinds, channel entries missing id/kind.
• API-version parsing including the -113 reply a below-floor device gives.
• Merge semantics: agrees-with-board-table, partial document overlays only what it states,
  document-states-nothing leaves every board value intact, Unknown board keeps its
  not-yet-known flags, non-positive ceiling does not lower the bound.
• Device level: below-floor device sends nothing and changes nothing; no reported firmware version
  is skipped; version out of range is not trusted; unparseable document leaves capabilities alone;
  the overlay survives a later status message; and Supports() answers identically before and
  after the overlay for every DeviceFeature (the "no new refusals" requirement).
• Rate model against the device's own constants, including the muxed-only case where dividing by a
  zero simultaneous count would be a division by zero.
• From review: the retained document is dropped on a transition between two known boards but
  survives one involving Unknown; and a document whose body schema_version disagrees with the
  APIVersion? reply is rejected.

Review rounds

Three rounds; converged with no findings in the last one. Three valid bugs found and fixed, all in
the same family — trusting a document nothing vouched for:

1. The retained document survived a board change, overlaying the previous device's flags and rate
   ceiling onto a different unit reconnected through the same instance.
2. The document body's schema_version was never compared against the APIVersion? reply. This
   matters more given both queries share one exchange, so an interleaved or stale line would mean
   the version vetted was not the version in hand.
3. My fix for (1) then over-corrected: it also cleared the document on transitions involving
   Unknown. Since Supports() skips the board axis while the board is Unknown, the read does
   run on such a device, so an unrecognized part number followed by a recognized one discarded a
   perfectly good document and reverted MaxSamplingRate to the stale 1 kHz ceiling. Unknown
   means "not yet known", not "a different board" (ADR 0001), so it is now treated as such in both
   directions.

One suggestion declined with reasoning on the thread: normalizing part numbers with Trim() in
DeviceTypeDetector. Reasonable on its own, but it changes a shared component this PR does not
touch, with blast radius on discovery and registry matching, and it is not load-bearing for the bug
it was suggested against.

Docs

ADR 0001 Decision 2 item 4 moved from growth path to built, with an implementation note
covering the two gates, the merge rule, the UpdateFromProtobuf fix, the refresh-point reasoning,
and the bench comparison. Also dropped the stale 1-1000 range from StartStreaming's doc comment
— it is now actively misleading with MaxSamplingRate populated from the device.

Scope notes

Deliberately not in this PR: issue scope item 7 (reconciling Core's enabled-channel view with
protobuf field 22) and the MCP-layer half of item 8 — both are changes to channel-state tracking
and the agent layer rather than to the reader, and sibling work is in flight nearby.

---

Not merging — opened for review.

[qodo-code-review]

PR Summary by Qodo

Read SCPI capability document and overlay DeviceCapabilities

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add SCPI queries to fetch capability schema version and JSON self-description.
• Parse capability JSON tolerantly and merge stated fields into board-derived capabilities.
• Persist overlays across status updates; add regression tests and ADR implementation notes.

Diagram

graph TD
  A["DaqifiDevice.InitializeAsync"] --> B["ExecuteTextCommandAsync"] --> C["ScpiMessageProducer"] --> D{{"Device firmware (SCPI)"}} --> E["CapabilityDocumentParser"] --> F[("CapabilityDocument")] --> G["DeviceMetadata"] --> H["DeviceCapabilities"]
  G -. "UpdateFromProtobuf re-applies overlay" .-> H

  subgraph Legend
    direction LR
    _svc["Service/Module"] ~~~ _ext{{"External"}} ~~~ _data[("Data")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Two-step SCPI exchange (version first, JSON only if compatible)

• ➕ Avoids transferring a multi-KB JSON document when schema is unsupported
• ➕ Keeps behavior strictly minimal on unknown future firmware
• ➖ Adds a second text exchange, which is expensive here due to protobuf consumer swap/timeout costs
• ➖ Increases connect latency and complexity; current design already fails closed by not trusting the doc

2. JsonSerializer-based DTOs with extension data

• ➕ Less manual parsing code; easier to map large schema surface
• ➕ Can capture unknown fields via JsonExtensionData for forward compatibility
• ➖ Harder to achieve the PR’s specific tolerance behavior (unexpected types => 'unstated' instead of exceptions/defaults)
• ➖ More brittle when fields are retyped; manual parsing makes 'ignore unknown/mistyped' explicit

3. Lazy capability-document read (only when a caller requests it)

• ➕ No added initialization latency
• ➕ Avoids pausing protobuf consumer during connect unless needed
• ➖ Leaves MaxSamplingRate and channel counts stale/wrong unless every consumer opts in
• ➖ Higher risk of inconsistent runtime behavior across callers; initialization-time enrichment is safer as a default

Recommendation: Keep the PR’s current approach: best-effort read during initialization with two gates (feature support + schema range) and merge-only semantics. It corrects known stale capability values early (notably MaxSamplingRate), remains safe for older devices (no queries sent when unsupported), and avoids future-schema footguns by failing closed on versions above the maximum.

Files changed (18) +2195 / -18

Enhancement (9) +1098 / -2

ScpiMessageProducer.csAdd capability-document SCPI queries and clarify streaming frequency docs +39/-2

Add capability-document SCPI queries and clarify streaming frequency docs

• Adds GetCapabilitiesApiVersion and GetCapabilitiesJson SCPI producers with versioning guidance. Updates StartStreaming documentation to remove the stale 1–1000 Hz claim and point to capability-derived limits (MaxSamplingRate and CurrentMaximumRateHz).

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

CapabilityChannel.csIntroduce CapabilityChannel model for channels[] entries +76/-0

Introduce CapabilityChannel model for channels[] entries

• Adds an immutable model representing a single channel entry with nullable/tolerant properties for per-kind fields (ranges, calibration, PWM capabilities, simultaneous sampling). Designed to preserve diagnosability for unknown kinds via RawKind.

src/Daqifi.Core/Device/Capabilities/CapabilityChannel.cs

CapabilityChannelKind.csAdd channel-kind enum with forward-compatible Unknown value +25/-0

Add channel-kind enum with forward-compatible Unknown value

• Defines known channel kinds and an Unknown fallback to support additive firmware kinds without breaking parsing or miscounting channels.

src/Daqifi.Core/Device/Capabilities/CapabilityChannelKind.cs

CapabilityDocument.csAdd CapabilityDocument model plus merge overlay into DeviceCapabilities +169/-0

Add CapabilityDocument model plus merge overlay into DeviceCapabilities

• Introduces the parsed capability document representation (schema, identity, channels, streaming, transport/storage/power flags, raw JSON). Implements MergeInto(DeviceCapabilities) to overlay only stated fields while preserving board-derived defaults and explicitly excluding fields the schema does not carry.

src/Daqifi.Core/Device/Capabilities/CapabilityDocument.cs

CapabilityDocumentParser.csAdd tolerant parser for capability JSON and schema version replies +400/-0

Add tolerant parser for capability JSON and schema version replies

• Implements TryParse/TryParseLines for JSON capability documents and TryParseApiVersion for integer schema-version responses, all designed to return false instead of throwing. Uses helper readers that treat absent or mistyped fields as 'unstated' to support schema evolution and safe fallback behavior.

src/Daqifi.Core/Device/Capabilities/CapabilityDocumentParser.cs

CapabilityIdentity.csAdd identity block model for diagnostics/cross-checking +32/-0

Add identity block model for diagnostics/cross-checking

• Adds a small model for the document’s identity fields (vendor/model/variant/serial/firmware/hardware), explicitly not used to derive core metadata to avoid conflicting sources.

src/Daqifi.Core/Device/Capabilities/CapabilityIdentity.cs

CapabilityRateModel.csAdd streaming-rate formula evaluation for UI preview use cases +116/-0

Add streaming-rate formula evaluation for UI preview use cases

• Implements client-side evaluation of the firmware-published streaming-rate formula with validated inputs and partial-term fallback when some constants are missing. Intended as an optimistic preview distinct from the authoritative CurrentMaximumRateHz.

src/Daqifi.Core/Device/Capabilities/CapabilityRateModel.cs

CapabilityStreaming.csAdd streaming block model with distinct rate contracts +68/-0

Add streaming block model with distinct rate contracts

• Models streaming fields including min/max sample rate, conservative envelope, current max rate, validation behavior, supported encodings/transports, and optional rate model. Documents the different semantics of maximum vs conservative vs current caps.

src/Daqifi.Core/Device/Capabilities/CapabilityStreaming.cs

DaqifiDevice.csRead and apply capability document during initialization (schema-gated) +173/-0

Read and apply capability document during initialization (schema-gated)

• Adds ReadCapabilityDocumentAsync with two gates: Supports(DeviceFeature.CapabilityDocument) and a schema version range (1..2). Performs a single text exchange to query schema version and JSON, parses tolerantly, and applies via Metadata.ApplyCapabilityDocument; failures are absorbed during InitializeAsync via TryReadCapabilityDocumentAsync so older/unresponsive devices remain unaffected.

src/Daqifi.Core/Device/DaqifiDevice.cs

Bug fix (1) +62 / -2

DeviceMetadata.csPersist capability overlay across status updates and track derivation board +62/-2

Persist capability overlay across status updates and track derivation board

• Introduces CapabilityDocument storage and ApplyCapabilityDocument to merge overlays into Capabilities. Refactors UpdateFromProtobuf to rebuild board-derived capabilities only when the detected board changes (or capabilities were never derived), preserving channel counts and overlays; re-applies the overlay last so it remains authoritative for stated fields.

src/Daqifi.Core/Device/DeviceMetadata.cs

Tests (7) +968 / -0

ScpiMessageProducerTests.csAdd tests for new capability SCPI queries +22/-0

Add tests for new capability SCPI queries

• Introduces assertions for the exact SCPI command strings for GetCapabilitiesApiVersion and GetCapabilitiesJson, ensuring message formatting remains consistent.

src/Daqifi.Core.Tests/Communication/Producers/ScpiMessageProducerTests.cs

CapabilityDocumentMergeTests.csVerify CapabilityDocument.MergeInto overlays without replacement +157/-0

Verify CapabilityDocument.MergeInto overlays without replacement

• Adds merge-behavior tests covering partial documents, channel set overlay semantics, non-positive max-rate handling, Unknown-board flag behavior, and null argument validation. Includes regression checks against a real captured Nyquist1 document.

src/Daqifi.Core.Tests/Device/Capabilities/CapabilityDocumentMergeTests.cs

CapabilityDocumentParserTests.csAdd tolerant parser tests driven by real hardware fixture +309/-0

Add tolerant parser tests driven by real hardware fixture

• Validates parsing of schema/identity, channels and per-kind counts, PWM and simultaneous-channel detection, streaming block and rate-model constants, and transport/power/storage flags. Adds robustness tests for malformed input, unexpected field types, unknown channel kinds, and line-based parsing behavior.

src/Daqifi.Core.Tests/Device/Capabilities/CapabilityDocumentParserTests.cs

CapabilityDocumentSamples.csAdd verbatim Nyquist1 firmware 3.7.2 capability JSON fixture +24/-0

Add verbatim Nyquist1 firmware 3.7.2 capability JSON fixture

• Stores a real device capability JSON response as a regression fixture to ensure the parser continues to ignore unknown fields and tolerate schema growth without narrowing the test data.

src/Daqifi.Core.Tests/Device/Capabilities/CapabilityDocumentSamples.cs

CapabilityRateModelTests.csTest client-side evaluation of device streaming rate formula +117/-0

Test client-side evaluation of device streaming rate formula

• Adds unit tests for CapabilityRateModel.TryComputeMaxRateHz across representative channel selections, including muxed-only behavior, bounds checking, and partial-constant fallbacks. Cross-checks results against counts derived from the same captured capability document.

src/Daqifi.Core.Tests/Device/Capabilities/CapabilityRateModelTests.cs

DaqifiDeviceCapabilityDocumentTests.csTest ReadCapabilityDocumentAsync gating and non-destructive behavior +240/-0

Test ReadCapabilityDocumentAsync gating and non-destructive behavior

• Introduces a scripted device harness to validate that capability-document reads are skipped when unsupported, schema-gated (min/max), tolerant to parse failures, and absorbed without changing existing board-derived capabilities. Also verifies overlays survive later status frames and do not alter Supports(...) outcomes.

src/Daqifi.Core.Tests/Device/Capabilities/DaqifiDeviceCapabilityDocumentTests.cs

DeviceMetadataTests.csAdd regression tests for capability overlay persistence across status updates +99/-0

Add regression tests for capability overlay persistence across status updates

• Adds tests ensuring repeated part numbers don’t reset channel counts, changed part numbers rebuild board-derived capabilities, and applying/copying capability documents persists the overlay through subsequent UpdateFromProtobuf calls. Validates null handling for ApplyCapabilityDocument.

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

Documentation (1) +67 / -14

0001-firmware-feature-gating.mdRecord capability-document reader as implemented (Decision 2 item 4) +67/-14

Record capability-document reader as implemented (Decision 2 item 4)

• Updates ADR follow-ups and Decision 2 notes to mark the capability-document reader as built in #390. Adds an implementation note documenting the two-gate strategy, merge semantics, UpdateFromProtobuf lifecycle changes, and measured connect-time impact.

docs/adr/0001-firmware-feature-gating.md

[qodo-code-review]

Code Review by Qodo

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

1. Stale doc survives board change ✓ Resolved 🐞 Bug ≡ Correctness

Description

DeviceMetadata.UpdateFromProtobuf() re-applies CapabilityDocument on every status message, but never
clears CapabilityDocument when the detected board changes and Capabilities are rebuilt. Reusing the
same device/metadata instance across a reconnect can therefore overlay the previous device’s
flags/channel counts/max rate onto the new board, breaking Supports() gating and streaming frequency
validation.

Code

src/Daqifi.Core/Device/DeviceMetadata.cs[R267-270]

+        // Re-apply the device's own capability document last, so it stays the authority over both
+        // the board table and the status message's port counts for the fields it states. This is
+        // what makes the merge durable: without it a status frame would win by arriving later.
+        CapabilityDocument?.MergeInto(Capabilities);

Relevance

●●● Strong

Repo history favors clearing/refreshing cached state on reconnect to avoid stale overlays; similar
reconnect-cache fixes accepted (PR #356/#348).

PR-#356
PR-#348

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code rebuilds Capabilities on board change but does not clear CapabilityDocument, and then
merges CapabilityDocument last on every update; the test suite explicitly calls out reconnecting to
a different unit on the same object, and streaming validation depends on MaxSamplingRate, which the
stale document can overwrite.

src/Daqifi.Core/Device/DeviceMetadata.cs[177-192]
src/Daqifi.Core/Device/DeviceMetadata.cs[267-271]
src/Daqifi.Core.Tests/Device/DeviceMetadataTests.cs[75-87]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[137-152]

Agent prompt

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

### Issue description
`DeviceMetadata.UpdateFromProtobuf()` rebuilds `Capabilities` when the detected board changes, but it keeps the previously-read `CapabilityDocument` and then unconditionally re-merges it at the end of the method. This can apply a document from the *previous* device/board onto the newly-detected device.

### Issue Context
This PR intentionally makes the capability-document overlay “durable” across later status messages. That durability must not persist across a device/board change (reconnect to a different unit on the same object).

### Fix Focus Areas
- src/Daqifi.Core/Device/DeviceMetadata.cs[177-192]
- src/Daqifi.Core/Device/DeviceMetadata.cs[267-271]
- src/Daqifi.Core.Tests/Device/DeviceMetadataTests.cs[75-87]

### Suggested fix
1. When `_capabilitiesBoard != detectedDeviceType` (i.e., you rebuild board-derived capabilities), also invalidate the retained capability document (e.g., `CapabilityDocument = null;`).
2. Add a regression test:
  - Start with `DevicePn = Nq1`, apply a capability document that changes `MaxSamplingRate`.
  - Then send `DevicePn = Nq3` and assert `CapabilityDocument` is null (or at least not re-applied) and that `Capabilities` reflect only the new board table.

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

---

2. Doc cleared on Unknown->known ✓ Resolved 🐞 Bug ☼ Reliability

Description

DeviceMetadata.UpdateFromProtobuf clears CapabilityDocument on any detected board change,
including when the previously-derived board was DeviceType.Unknown, which can discard a valid
overlay and revert capabilities (e.g., MaxSamplingRate) to board defaults. This contradicts the
comment’s intended behavior (“document read before the board was known was still read from this
device”) and can reintroduce the wrong 1 kHz ceiling until the document is re-read.

Code

src/Daqifi.Core/Device/DeviceMetadata.cs[R196-199]

+                if (_capabilitiesBoard.HasValue)
+                {
+                    CapabilityDocument = null;
+                }

Relevance

●●● Strong

Team often accepts sentinel/transition reliability fixes (Unknown handling, preserve derived state).
Similar accepted fixes in PRs 288/348.

PR-#288
PR-#348
PR-#250

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new invalidation logic clears CapabilityDocument whenever _capabilitiesBoard changes (based
only on .HasValue), which includes the case where the previous derived board was Unknown. Since
DetectFromPartNumber does not trim/normalize input beyond lowercasing and exact matching,
transient/unrecognized part numbers map to Unknown, making an Unknown -> Known transition
plausible and currently destructive to the retained overlay.

src/Daqifi.Core/Device/DeviceMetadata.cs[177-203]
src/Daqifi.Core/Device/DeviceTypeDetector.cs[13-26]

Agent prompt

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

### Issue description
`DeviceMetadata.UpdateFromProtobuf` currently clears `CapabilityDocument` whenever `_capabilitiesBoard` changes, even if the previous board value was `DeviceType.Unknown`. This makes `Unknown -> Known` transitions destructive: the already-read document overlay can be dropped and the device’s capabilities can regress to board-table defaults.

### Issue Context
- `_capabilitiesBoard` is used to decide whether to rebuild capabilities and whether to discard a retained capability document.
- `DeviceTypeDetector.DetectFromPartNumber` only lowercases and exact-matches known part numbers; it does not normalize (e.g., trim), so transient/unrecognized `DevicePn` values map to `DeviceType.Unknown`.

### Fix Focus Areas
- src/Daqifi.Core/Device/DeviceMetadata.cs[189-203]
- src/Daqifi.Core/Device/DeviceTypeDetector.cs[13-26]

### Suggested fix
1. Change the invalidation condition so the document is cleared only when the previous board was a *known* board (not `Unknown`) and the newly detected board is a *different known* board.
  - Example logic: clear only if `_capabilitiesBoard is { } old && old != DeviceType.Unknown && detectedDeviceType != DeviceType.Unknown && old != detectedDeviceType`.
2. Normalize part numbers before detection (e.g., `Trim()` in `DetectFromPartNumber` or before calling it) to reduce spurious `Unknown` detection.
3. Add/extend a unit test covering `Unknown -> Nyquist1` transition preserving an existing `CapabilityDocument` (or at least preserving the overlaid `MaxSamplingRate`).

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

---

3. Schema mismatch not rejected ✓ Resolved 🐞 Bug ☼ Reliability

Description

DaqifiDevice.ReadCapabilityDocumentAsync() validates the separately-reported APIVersion? value, but
then applies whatever parsed JSON document it finds without verifying document.SchemaVersion matches
that API version. This can bypass the intended fail-closed schema gate if the response lines are
inconsistent (interleaved/stale/mis-emitted).

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R1215-1223]

+            if (!CapabilityDocumentParser.TryParseLines(lines, out var document))
+            {
+                SafeLog(() => _logger.LogDebug(
+                    "[ReadCapabilityDocumentAsync] The capability document did not parse; keeping board-derived capabilities."));
+                return null;
+            }
+
+            Metadata.ApplyCapabilityDocument(document);
+            return document;

Relevance

●●● Strong

Team frequently accepts fail-closed validation hardening in device gates; similar defensive fixes
merged in PRs #389/#349.

PR-#389
PR-#349

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code gates trust only on the parsed API version, then applies the parsed document without
comparing versions; the document type itself states that the body schema_version is the same value
returned by APIVersion?, so mismatch is a meaningful inconsistency that should fail closed.

src/Daqifi.Core/Device/DaqifiDevice.cs[1165-1224]
src/Daqifi.Core/Device/Capabilities/CapabilityDocument.cs[27-32]

Agent prompt

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

### Issue description
`ReadCapabilityDocumentAsync()` checks `apiVersion` is within the supported range, but does not assert that the parsed document body’s `schema_version` matches the `apiVersion` reply before applying it.

### Issue Context
`CapabilityDocument.SchemaVersion` is documented as “the same value `CONFigure:CAPabilities:APIVersion?` returns”. If those disagree, the response is inconsistent and should be treated as untrusted to preserve the PR’s stated “fail closed” behavior.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[1165-1224]
- src/Daqifi.Core/Device/Capabilities/CapabilityDocument.cs[27-32]
- src/Daqifi.Core.Tests/Device/Capabilities/DaqifiDeviceCapabilityDocumentTests.cs[43-132]

### Suggested fix
1. After `TryParseLines(..., out document)` and before `ApplyCapabilityDocument`, add:
  - `if (document.SchemaVersion != apiVersion) { log + return null; }`
2. Add a unit test where APIVersion returns `2` but the JSON contains `"schema_version":1` (and vice versa), and assert the document is not applied.

ⓘ 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 5b3fce0 ⚖️ Balanced

Results up to commit 6d70c61 ⚖️ Balanced

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

1. Stale doc survives board change ✓ Resolved 🐞 Bug ≡ Correctness

Description

DeviceMetadata.UpdateFromProtobuf() re-applies CapabilityDocument on every status message, but never
clears CapabilityDocument when the detected board changes and Capabilities are rebuilt. Reusing the
same device/metadata instance across a reconnect can therefore overlay the previous device’s
flags/channel counts/max rate onto the new board, breaking Supports() gating and streaming frequency
validation.

Code

src/Daqifi.Core/Device/DeviceMetadata.cs[R267-270]

+        // Re-apply the device's own capability document last, so it stays the authority over both
+        // the board table and the status message's port counts for the fields it states. This is
+        // what makes the merge durable: without it a status frame would win by arriving later.
+        CapabilityDocument?.MergeInto(Capabilities);

Relevance

●●● Strong

Repo history favors clearing/refreshing cached state on reconnect to avoid stale overlays; similar
reconnect-cache fixes accepted (PR #356/#348).

PR-#356
PR-#348

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code rebuilds Capabilities on board change but does not clear CapabilityDocument, and then
merges CapabilityDocument last on every update; the test suite explicitly calls out reconnecting to
a different unit on the same object, and streaming validation depends on MaxSamplingRate, which the
stale document can overwrite.

src/Daqifi.Core/Device/DeviceMetadata.cs[177-192]
src/Daqifi.Core/Device/DeviceMetadata.cs[267-271]
src/Daqifi.Core.Tests/Device/DeviceMetadataTests.cs[75-87]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[137-152]

Agent prompt

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

### Issue description
`DeviceMetadata.UpdateFromProtobuf()` rebuilds `Capabilities` when the detected board changes, but it keeps the previously-read `CapabilityDocument` and then unconditionally re-merges it at the end of the method. This can apply a document from the *previous* device/board onto the newly-detected device.

### Issue Context
This PR intentionally makes the capability-document overlay “durable” across later status messages. That durability must not persist across a device/board change (reconnect to a different unit on the same object).

### Fix Focus Areas
- src/Daqifi.Core/Device/DeviceMetadata.cs[177-192]
- src/Daqifi.Core/Device/DeviceMetadata.cs[267-271]
- src/Daqifi.Core.Tests/Device/DeviceMetadataTests.cs[75-87]

### Suggested fix
1. When `_capabilitiesBoard != detectedDeviceType` (i.e., you rebuild board-derived capabilities), also invalidate the retained capability document (e.g., `CapabilityDocument = null;`).
2. Add a regression test:
  - Start with `DevicePn = Nq1`, apply a capability document that changes `MaxSamplingRate`.
  - Then send `DevicePn = Nq3` and assert `CapabilityDocument` is null (or at least not re-applied) and that `Capabilities` reflect only the new board table.

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

---

2. Schema mismatch not rejected ✓ Resolved 🐞 Bug ☼ Reliability

Description

DaqifiDevice.ReadCapabilityDocumentAsync() validates the separately-reported APIVersion? value, but
then applies whatever parsed JSON document it finds without verifying document.SchemaVersion matches
that API version. This can bypass the intended fail-closed schema gate if the response lines are
inconsistent (interleaved/stale/mis-emitted).

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R1215-1223]

+            if (!CapabilityDocumentParser.TryParseLines(lines, out var document))
+            {
+                SafeLog(() => _logger.LogDebug(
+                    "[ReadCapabilityDocumentAsync] The capability document did not parse; keeping board-derived capabilities."));
+                return null;
+            }
+
+            Metadata.ApplyCapabilityDocument(document);
+            return document;

Relevance

●●● Strong

Team frequently accepts fail-closed validation hardening in device gates; similar defensive fixes
merged in PRs #389/#349.

PR-#389
PR-#349

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code gates trust only on the parsed API version, then applies the parsed document without
comparing versions; the document type itself states that the body schema_version is the same value
returned by APIVersion?, so mismatch is a meaningful inconsistency that should fail closed.

src/Daqifi.Core/Device/DaqifiDevice.cs[1165-1224]
src/Daqifi.Core/Device/Capabilities/CapabilityDocument.cs[27-32]

Agent prompt

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

### Issue description
`ReadCapabilityDocumentAsync()` checks `apiVersion` is within the supported range, but does not assert that the parsed document body’s `schema_version` matches the `apiVersion` reply before applying it.

### Issue Context
`CapabilityDocument.SchemaVersion` is documented as “the same value `CONFigure:CAPabilities:APIVersion?` returns”. If those disagree, the response is inconsistent and should be treated as untrusted to preserve the PR’s stated “fail closed” behavior.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[1165-1224]
- src/Daqifi.Core/Device/Capabilities/CapabilityDocument.cs[27-32]
- src/Daqifi.Core.Tests/Device/Capabilities/DaqifiDeviceCapabilityDocumentTests.cs[43-132]

### Suggested fix
1. After `TryParseLines(..., out document)` and before `ApplyCapabilityDocument`, add:
  - `if (document.SchemaVersion != apiVersion) { log + return null; }`
2. Add a unit test where APIVersion returns `2` but the JSON contains `"schema_version":1` (and vice versa), and assert the document is not applied.

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

---

Results up to commit ff5c3be ⚖️ Balanced

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

1. Doc cleared on Unknown->known ✓ Resolved 🐞 Bug ☼ Reliability

Description

DeviceMetadata.UpdateFromProtobuf clears CapabilityDocument on any detected board change,
including when the previously-derived board was DeviceType.Unknown, which can discard a valid
overlay and revert capabilities (e.g., MaxSamplingRate) to board defaults. This contradicts the
comment’s intended behavior (“document read before the board was known was still read from this
device”) and can reintroduce the wrong 1 kHz ceiling until the document is re-read.

Code

src/Daqifi.Core/Device/DeviceMetadata.cs[R196-199]

+                if (_capabilitiesBoard.HasValue)
+                {
+                    CapabilityDocument = null;
+                }

Relevance

●●● Strong

Team often accepts sentinel/transition reliability fixes (Unknown handling, preserve derived state).
Similar accepted fixes in PRs 288/348.

PR-#288
PR-#348
PR-#250

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new invalidation logic clears CapabilityDocument whenever _capabilitiesBoard changes (based
only on .HasValue), which includes the case where the previous derived board was Unknown. Since
DetectFromPartNumber does not trim/normalize input beyond lowercasing and exact matching,
transient/unrecognized part numbers map to Unknown, making an Unknown -> Known transition
plausible and currently destructive to the retained overlay.

src/Daqifi.Core/Device/DeviceMetadata.cs[177-203]
src/Daqifi.Core/Device/DeviceTypeDetector.cs[13-26]

Agent prompt

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

### Issue description
`DeviceMetadata.UpdateFromProtobuf` currently clears `CapabilityDocument` whenever `_capabilitiesBoard` changes, even if the previous board value was `DeviceType.Unknown`. This makes `Unknown -> Known` transitions destructive: the already-read document overlay can be dropped and the device’s capabilities can regress to board-table defaults.

### Issue Context
- `_capabilitiesBoard` is used to decide whether to rebuild capabilities and whether to discard a retained capability document.
- `DeviceTypeDetector.DetectFromPartNumber` only lowercases and exact-matches known part numbers; it does not normalize (e.g., trim), so transient/unrecognized `DevicePn` values map to `DeviceType.Unknown`.

### Fix Focus Areas
- src/Daqifi.Core/Device/DeviceMetadata.cs[189-203]
- src/Daqifi.Core/Device/DeviceTypeDetector.cs[13-26]

### Suggested fix
1. Change the invalidation condition so the document is cleared only when the previous board was a *known* board (not `Unknown`) and the newly detected board is a *different known* board.
  - Example logic: clear only if `_capabilitiesBoard is { } old && old != DeviceType.Unknown && detectedDeviceType != DeviceType.Unknown && old != detectedDeviceType`.
2. Normalize part numbers before detection (e.g., `Trim()` in `DetectFromPartNumber` or before calling it) to reduce spurious `Unknown` detection.
3. Add/extend a unit test covering `Unknown -> Nyquist1` transition preserving an existing `CapabilityDocument` (or at least preserving the overlaid `MaxSamplingRate`).

ⓘ 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 ff5c3be

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 5b3fce0