fix(mcp): re-validate the live sample rate when a channel-configure call shrinks the cap

[tylerkron]

Summary

set_sample_rate's device-cap guard (CapabilityStreaming.CurrentMaximumRateHz, refreshed after every channel-configuration call) only validates the rate at the moment it is set. configure_analog_channels / configure_digital_channels already refresh the cap, but never re-check the rate that is already live against it — so widening the channel set can leave StreamingFrequency above the new, lower cap for that channel set.

Fixes #447.

Bench evidence from the issue (not re-run here — no device on hand)

configure_analog_channels([0])        -> cap 7746
set_sample_rate(7746)                 -> ACCEPT          (legal for this channel set)
configure_analog_channels([0..15])    -> cap is now 3518

After that third call: ConfigureResult echoed back {"enabledAnalogChannels":[0..15], "sampleRateHz": 7746} — a rate the guard itself would reject if re-requested (set_sample_rate(7746) → "exceeds the maximum 3518 Hz"). The firmware's response to an over-cap rate is silent: -222,"Data out of range" and zero samples, no exception, no ErrorOccurred.

Fix

Following the issue's suggested approach:

• Extracted the cap arithmetic into a pure, directly-testable SampleRateCapCalculator (ComputeCapHz / EnforceCap), used by both SetSampleRateAsync and the two configure calls.
• ConfigureAnalogChannelsAsync / ConfigureDigitalChannelsAsync now re-validate the live rate against the refreshed cap after every channel change. When it no longer fits, it's lowered to the cap and the adjustment is reported via a new SampleRateAdjustedFromHz field on ConfigureResult / ConfigureDigitalResult (null when no adjustment was needed) — so the agent is told, not silently overridden.
• A cap of 0 (nothing enabled) leaves the rate alone rather than driving it to 0, per the issue's guidance.
• Secondary fix from the issue: set_sample_rate with a 0 cap now says to enable a channel first, instead of the generic "exceeds the maximum 0 Hz" message that reads as "you asked for too much."
• StartLoggingAsync re-checks the live rate against the cap as a use-time backstop — the point an out-of-range rate would actually reach the firmware — throwing instead of letting a logging session come back with silently zero samples.
• Tool descriptions for configure_analog_channels / configure_digital_channels now mention the auto-adjustment and sampleRateAdjustedFromHz.

Testing

No device on hand for this pass, so I leaned on making the fix's logic directly unit-testable rather than requiring a fake connected-device harness (that infrastructure is #465's job):

• SampleRateCapCalculatorTests (new, 11 cases) — reproduces the exact 7746 Hz → 3518 Hz reorder trap from the issue, the 0-cap non-flooring behavior, hardware-max flooring/bounding, and the --max-sample-rate-hz interaction.
• dotnet test Daqifi.Core.sln — full suite green: 2853 Core tests (2 skipped real-hardware, unchanged) + 34 Mcp tests (23 existing + 11 new).
• dotnet build Daqifi.Core.sln — 0 warnings, 0 errors.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Revalidate live sample rate after channel configuration lowers device cap

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Recompute the effective sample-rate cap after every channel configuration change.
• Auto-lower an over-cap live rate and report the adjustment in configure results.
• Add unit tests plus clearer tool/docs messaging and a StartLogging safety backstop.

Diagram

graph TD
  T["MCP tools"] --> A["DaqifiAgent"] --> Calc["SampleRateCapCalculator"]
  A --> R["Refresh capability"] --> CDoc[("Capability doc")]
  CDoc --> Calc --> Dev[("Streaming device")]
  A --> DTO["Configure*Result DTOs"]
  Dev --> FW{{"Firmware"}}

  subgraph Legend
    direction LR
    _svc["Service"] ~~~ _mod["Module"] ~~~ _db[("Device/State")] ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Fail configure_* when the live sample rate exceeds the new cap

• ➕ Avoids implicit behavior; forces clients/agents to explicitly resolve the mismatch
• ➕ Simpler mental model: configure never mutates sample-rate state
• ➖ Creates a more brittle workflow: a channel change can unexpectedly hard-fail
• ➖ Requires extra round-trips and error-handling in agents for a recoverable condition

2. Defer validation to StartLogging/stream start only

• ➕ Minimizes state mutation during configuration calls
• ➕ One enforcement point closest to firmware interaction
• ➖ Leaves the system reporting an invalid live rate for longer
• ➖ Agents can proceed under false assumptions until logging starts

3. Always re-issue set_sample_rate internally after configure_*

• ➕ Keeps a single codepath for updating the device rate
• ➕ Could centralize error handling in SetSampleRateAsync
• ➖ More device chatter and potential timing issues
• ➖ Harder to distinguish 'adjusted due to cap shrink' from a requested change

Recommendation: The PR’s approach (centralized cap arithmetic + enforce-on-configure with explicit reporting, plus a StartLogging backstop) is the best tradeoff. It preserves forward progress in common workflows, prevents the silent zero-samples failure mode, and makes the implicit adjustment observable via SampleRateAdjustedFromHz.

Files changed (5) +266 / -25

Enhancement (1) +23 / -4

Dtos.csExtend configure result DTOs to report sample-rate adjustments +23/-4

Extend configure result DTOs to report sample-rate adjustments

• ConfigureResult and ConfigureDigitalResult now include SampleRateHz and a nullable SampleRateAdjustedFromHz to indicate when a channel-set change forced an automatic rate reduction. Adds documentation explaining when the field is populated and why.

src/Daqifi.Mcp/Dtos.cs

Bug fix (1) +90 / -19

DaqifiAgent.csEnforce refreshed sample-rate caps on configure and logging start +90/-19

Enforce refreshed sample-rate caps on configure and logging start

• ConfigureAnalogChannelsAsync/ConfigureDigitalChannelsAsync now revalidate the live StreamingFrequency against the refreshed cap and return any adjustment. SetSampleRateAsync uses shared cap computation and emits a clearer error when cap is 0 (no channels enabled). StartLoggingAsync adds a use-time guard that throws if the live rate exceeds the current cap to avoid silent firmware failure.

src/Daqifi.Mcp/DaqifiAgent.cs

Refactor (1) +57 / -0

SampleRateCapCalculator.csExtract pure cap arithmetic into SampleRateCapCalculator +57/-0

Extract pure cap arithmetic into SampleRateCapCalculator

• Adds a static helper encapsulating effective cap computation (hardware max, device current max, optional server clamp) and enforcement semantics (lower rate when above cap; treat cap 0 as 'leave unchanged'). Designed to be testable without device access.

src/Daqifi.Mcp/SampleRateCapCalculator.cs

Tests (1) +94 / -0

SampleRateCapCalculatorTests.csAdd unit coverage for cap computation and enforcement logic +94/-0

Add unit coverage for cap computation and enforcement logic

• Introduces 11 test cases covering cap fallback, bounding, option clamping, and enforcement behavior. Includes explicit reproducer coverage for the #447 sequence and the zero-cap (no channels enabled) semantics.

src/Daqifi.Mcp.Tests/SampleRateCapCalculatorTests.cs

Documentation (1) +2 / -2

DaqifiTools.csDocument auto-adjust behavior in configure_* tool descriptions +2/-2

Document auto-adjust behavior in configure_* tool descriptions

• Updates configure_analog_channels and configure_digital_channels descriptions to mention that widening channels can lower the cap and that an over-cap live rate will be auto-lowered and reported via sampleRateAdjustedFromHz.

src/Daqifi.Mcp/Tools/DaqifiTools.cs

[qodo-code-review]

Code Review by Qodo

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

1. Negative cap bypasses validation ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

SampleRateCapCalculator.ComputeCapHz can return a negative cap if the capability document contains a
negative current_max_rate_hz (the parser accepts any int32), which then makes StartLoggingAsync skip
its over-cap guard (cap > 0) and makes SetSampleRateAsync misreport the condition as "No channels
are enabled" (cap <= 0). This can allow an out-of-range sample rate to reach firmware without the
intended MCP-side fail-fast and/or block valid usage with a misleading error message.

Code

src/Daqifi.Mcp/SampleRateCapCalculator.cs[R31-33]

+        var hardwareMax = Math.Max(1, hardwareMaxSamplingRateHz);
+        var deviceCap = currentMaxRateHz.HasValue ? Math.Min(currentMaxRateHz.Value, hardwareMax) : hardwareMax;
+        return maxSampleRateHzOption.HasValue ? Math.Min(maxSampleRateHzOption.Value, deviceCap) : deviceCap;

Relevance

●●● Strong

Team has recently accepted defensive cap-sanitization/bounding to prevent invalid rate validation
behavior.

PR-#412
PR-#277

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cap calculator uses Math.Min on currentMaxRateHz without sanitizing negatives, while the
capability document parser will parse and return negative integers as-is. The callers then treat
non-positive caps differently, which means a negative cap can both disable StartLoggingAsync’s
safety check and trigger an incorrect “no channels enabled” error path.

src/Daqifi.Mcp/SampleRateCapCalculator.cs[29-34]
src/Daqifi.Core/Device/Capabilities/CapabilityDocumentParser.cs[354-359]
src/Daqifi.Core/Device/Capabilities/CapabilityStreaming.cs[45-51]
src/Daqifi.Mcp/DaqifiAgent.cs[386-399]
src/Daqifi.Mcp/DaqifiAgent.cs[486-493]

Agent prompt

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

### Issue description
`SampleRateCapCalculator.ComputeCapHz(...)` currently preserves negative `currentMaxRateHz` values. Because `CapabilityDocumentParser.ReadInt(...)` accepts any `int32` (including negatives), a malformed/buggy capability document can yield a negative effective cap. That negative cap is then handled inconsistently:
- `StartLoggingAsync` only enforces when `cap > 0`, so a negative cap disables the backstop entirely.
- `SetSampleRateAsync` treats `cap <= 0` as “no channels enabled”, which is only valid for `cap == 0`.

### Issue Context
The codebase explicitly treats `0` as a meaningful value (“no channels enabled”), but does not define negative caps as meaningful. Negative caps should be treated as invalid input and normalized (e.g., treated as absent -> fall back to hardware max), while preserving `0`.

### Fix Focus Areas
- src/Daqifi.Mcp/SampleRateCapCalculator.cs[29-34]
- src/Daqifi.Mcp.Tests/SampleRateCapCalculatorTests.cs[9-58]
- src/Daqifi.Mcp/DaqifiAgent.cs[386-399]

### Suggested implementation outline
1. In `ComputeCapHz`, normalize inputs:
  - If `currentMaxRateHz < 0`, treat it as `null` (or otherwise fall back to `hardwareMax`).
  - Optionally, if `maxSampleRateHzOption <= 0`, ignore it (treat as `null`) to keep the function robust even if called outside `ServerOptions.Parse`.
2. Keep `currentMaxRateHz == 0` behavior unchanged.
3. Add unit tests:
  - `ComputeCapHz_NegativeCurrentMax_TreatedAsAbsent()` expecting fallback to `hardwareMax` (or to the server clamp if present).
4. (Optional) In `SetSampleRateAsync`, consider changing `if (cap <= 0)` to `if (cap == 0)` to keep the “no channels enabled” message strictly tied to the documented `0` meaning, with negative caps treated as invalid/buggy metadata.

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

---

2. DTO constructor ABI break ✗ Dismissed 🐞 Bug ⚙ Maintainability

Description

ConfigureResult and ConfigureDigitalResult were changed to add new positional parameters, which
changes the generated public constructors and Deconstruct overloads and will break downstream
consumers compiled against the prior record signatures. This is especially risky because Daqifi.Mcp
is packaged for distribution, so existing integrations may load the updated assembly but still
expect the old ctor/deconstruct signatures.

Code

src/Daqifi.Mcp/Dtos.cs[R112-114]

+    IReadOnlyList<int> EnabledAnalogChannels,
+    int SampleRateHz,
+    int? SampleRateAdjustedFromHz);

Relevance

●●● Strong

Team previously accepted avoiding breaking record positional-parameter ABI changes for downstream
consumers.

PR-#321
PR-#349

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Dtos.cs shows both records were changed from their prior (shorter) positional forms to new longer
primary constructors. Because records generate public constructors and Deconstruct overloads from
the primary constructor parameter list, this is a breaking API/ABI change for any downstream .NET
code consuming these DTOs. The project is also packaged for distribution, increasing the likelihood
of downstream consumers existing.

src/Daqifi.Mcp/Dtos.cs[103-126]
src/Daqifi.Mcp/Daqifi.Mcp.csproj[3-19]
PR-#321

Agent prompt

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

### Issue description
`ConfigureResult` and `ConfigureDigitalResult` are public positional records. Adding positional parameters changes their primary constructor signature and the compiler-generated `Deconstruct(...)` overloads, which is a source + binary compatibility break for downstream .NET consumers.

### Issue Context
These DTOs are part of the `Daqifi.Mcp` assembly, which is packaged/distributed as a .NET tool/package. If any external code references these DTO types directly (constructs them, pattern-matches, or deconstructs them), it will fail to compile (source break) or fail at runtime when loading a newer assembly (binary break).

### Fix Focus Areas
- src/Daqifi.Mcp/Dtos.cs[103-126]
- src/Daqifi.Mcp/DaqifiAgent.cs[217-264]

### Suggested fix approach
Option A (preferred for compatibility):
1. Revert the *primary constructor* signatures back to the original arity:
  - `ConfigureResult(string DeviceId, IReadOnlyList<int> EnabledAnalogChannels, int SampleRateHz)`
  - `ConfigureDigitalResult(string DeviceId, IReadOnlyList<int> EnabledDigitalChannels)`
2. Add the new fields as additional `init` properties on the records (e.g., `public int? SampleRateAdjustedFromHz { get; init; }` and for digital also `public int SampleRateHz { get; init; }`).
3. Update call sites to set those new properties via object initializer:
  - `new ConfigureResult(...){ SampleRateAdjustedFromHz = adjustedFromHz }`
  - `new ConfigureDigitalResult(...){ SampleRateHz = streaming.StreamingFrequency, SampleRateAdjustedFromHz = adjustedFromHz }`

Option B (if you want to keep 4-arg primary ctor):
- Add explicit compatibility constructors matching the old signatures *and* add explicit `Deconstruct` overloads matching old arity (construction compatibility alone won’t preserve old deconstruction calls).

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

Results up to commit 9b5a0c8 ⚖️ Balanced

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

1. DTO constructor ABI break ✗ Dismissed 🐞 Bug ⚙ Maintainability

Description

ConfigureResult and ConfigureDigitalResult were changed to add new positional parameters, which
changes the generated public constructors and Deconstruct overloads and will break downstream
consumers compiled against the prior record signatures. This is especially risky because Daqifi.Mcp
is packaged for distribution, so existing integrations may load the updated assembly but still
expect the old ctor/deconstruct signatures.

Code

src/Daqifi.Mcp/Dtos.cs[R112-114]

+    IReadOnlyList<int> EnabledAnalogChannels,
+    int SampleRateHz,
+    int? SampleRateAdjustedFromHz);

Relevance

●●● Strong

Team previously accepted avoiding breaking record positional-parameter ABI changes for downstream
consumers.

PR-#321
PR-#349

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Dtos.cs shows both records were changed from their prior (shorter) positional forms to new longer
primary constructors. Because records generate public constructors and Deconstruct overloads from
the primary constructor parameter list, this is a breaking API/ABI change for any downstream .NET
code consuming these DTOs. The project is also packaged for distribution, increasing the likelihood
of downstream consumers existing.

src/Daqifi.Mcp/Dtos.cs[103-126]
src/Daqifi.Mcp/Daqifi.Mcp.csproj[3-19]
PR-#321

Agent prompt

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

### Issue description
`ConfigureResult` and `ConfigureDigitalResult` are public positional records. Adding positional parameters changes their primary constructor signature and the compiler-generated `Deconstruct(...)` overloads, which is a source + binary compatibility break for downstream .NET consumers.

### Issue Context
These DTOs are part of the `Daqifi.Mcp` assembly, which is packaged/distributed as a .NET tool/package. If any external code references these DTO types directly (constructs them, pattern-matches, or deconstructs them), it will fail to compile (source break) or fail at runtime when loading a newer assembly (binary break).

### Fix Focus Areas
- src/Daqifi.Mcp/Dtos.cs[103-126]
- src/Daqifi.Mcp/DaqifiAgent.cs[217-264]

### Suggested fix approach
Option A (preferred for compatibility):
1. Revert the *primary constructor* signatures back to the original arity:
  - `ConfigureResult(string DeviceId, IReadOnlyList<int> EnabledAnalogChannels, int SampleRateHz)`
  - `ConfigureDigitalResult(string DeviceId, IReadOnlyList<int> EnabledDigitalChannels)`
2. Add the new fields as additional `init` properties on the records (e.g., `public int? SampleRateAdjustedFromHz { get; init; }` and for digital also `public int SampleRateHz { get; init; }`).
3. Update call sites to set those new properties via object initializer:
  - `new ConfigureResult(...){ SampleRateAdjustedFromHz = adjustedFromHz }`
  - `new ConfigureDigitalResult(...){ SampleRateHz = streaming.StreamingFrequency, SampleRateAdjustedFromHz = adjustedFromHz }`

Option B (if you want to keep 4-arg primary ctor):
- Add explicit compatibility constructors matching the old signatures *and* add explicit `Deconstruct` overloads matching old arity (construction compatibility alone won’t preserve old deconstruction calls).

ⓘ 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 9b5a0c8

[tylerkron]

/agentic_review

[qodo-code-review]

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