fix(mcp): raise discover_devices timeout floor from 250ms to 1000ms

[tylerkron]

Summary

DaqifiAgent.DiscoverAsync clamped the caller's timeoutMs to a 250 ms floor, but the serial identify handshake takes ~830 ms on real hardware. Every value in the lower two-thirds of the advertised 250..30000 range — including the floor itself — returned an empty list, silently indistinguishable from "no device is plugged in".

Confirmed on bench hardware (Nq1, fw 3.7.2, USB CDC) in #448: 16/16 successful runs returned in 0.80–0.86 s regardless of a 900 ms or 2000 ms budget (the device answers as soon as it's ready, not at the timeout), while every run below ~830 ms failed at exactly its timeout.

Fix

• Raise DaqifiAgent.MinDiscoveryTimeoutMs from 250 to 1000 (measured ~830 ms + ~20% margin).
• Update the discover_devices tool description to match and explain why.
• Extract the clamp into DaqifiAgent.ClampDiscoveryTimeout — there was no test for the clamp before; this makes it directly unit-testable without spinning up real device finders.

The secondary "distinguish timeout from no-device" and "call out the ~1ms claim-skip window" suggestions from the issue are left for a follow-up if it comes up again — this fix addresses the actual bug (a floor that could never succeed).

Testing

• New tests: ClampDiscoveryTimeout_BelowFloor_ClampsToFloor, ClampDiscoveryTimeout_WithinRange_IsUnchanged, ClampDiscoveryTimeout_AboveCeiling_ClampsTo30Seconds
• Full Daqifi.Mcp.Tests suite: 43 passed
• Full Daqifi.Core.Tests suite: 2853 passed

Fixes #448

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Fix device discovery timeout clamp: raise floor to 1000ms and add unit tests

🐞 Bug fix 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Raise discovery timeout floor to 1000ms to match measured serial handshake latency.
• Extract timeout clamping into a dedicated helper for direct unit testing.
• Update discover_devices tool help text to explain the new clamp range and rationale.

Diagram

graph TD
  caller(("Caller")) --> tool["discover_devices tool"] --> agent["DaqifiAgent.DiscoverAsync"] --> clamp["ClampDiscoveryTimeout"] --> disc["Device discovery (WiFi/Serial)"] --> dev["DAQiFi device"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make discovery timeout floor configurable (options/env)

• ➕ Allows different hardware/OS combinations to tune discovery without code changes
• ➕ Avoids hardcoding an empirical constant if variability is expected
• ➖ Adds configuration surface area and support burden
• ➖ Still needs a safe default floor; doesn’t fix current incorrect default by itself

2. Return explicit timeout vs empty list (distinguish ‘no device’ from ‘timed out’)

• ➕ Prevents silent false negatives and improves diagnosability
• ➕ Avoids needing to encode hardware timing assumptions in a floor
• ➖ Behavior change for callers that currently treat empty list as ‘no device’
• ➖ Requires API/contract work and likely wider test updates

Recommendation: The PR’s approach is the best low-risk fix: raise the floor to a measured value and document it, while extracting the clamp for unit test coverage. The alternatives (configurable floor, explicit timeout signaling) are valuable but broader contract/design changes and are reasonable follow-ups rather than blocking this correctness fix.

Files changed (4) +54 / -2

Bug fix (1) +20 / -1

DaqifiAgent.csRaise discovery timeout floor and extract ClampDiscoveryTimeout +20/-1

Raise discovery timeout floor and extract ClampDiscoveryTimeout

• Introduces MinDiscoveryTimeoutMs=1000 based on measured ~830ms serial identify handshake plus margin. Extracts timeout clamping into ClampDiscoveryTimeout and updates DiscoverAsync to use it instead of an inline Math.Clamp.

src/Daqifi.Mcp/DaqifiAgent.cs

Tests (1) +29 / -0

DaqifiMcpTests.csAdd unit tests for discovery timeout clamping +29/-0

Add unit tests for discovery timeout clamping

• Adds theory/fact tests that pin clamping behavior below the floor, within range, and above the ceiling. Tests reference the new clamp helper directly to avoid requiring real device discovery runs.

src/Daqifi.Mcp.Tests/DaqifiMcpTests.cs

Documentation (1) +1 / -1

DaqifiTools.csUpdate discover_devices timeout documentation to 1000..30000ms +1/-1

Update discover_devices timeout documentation to 1000..30000ms

• Updates the tool parameter description to reflect the new clamp floor and explain why sub-1000ms budgets can never succeed on real serial hardware. Clarifies that successful discovery returns as soon as the device responds, not at the full timeout.

src/Daqifi.Mcp/Tools/DaqifiTools.cs

Other (1) +4 / -0

Daqifi.Mcp.csprojExpose internals to test assembly +4/-0

Expose internals to test assembly

• Adds InternalsVisibleTo for Daqifi.Mcp.Tests so tests can access internal clamp logic without widening the public API surface.

src/Daqifi.Mcp/Daqifi.Mcp.csproj

[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. Misleading discovery timing docs ✓ Resolved 🐞 Bug ≡ Correctness

Description

The discover_devices timeout description claims successful calls return as soon as a device
responds, but WiFi discovery runs receive loops until the timeout cancellation and typically lasts
the full timeout. This can mislead MCP clients/agents about expected latency and cause incorrect
timeout budgeting/retries.

Code

src/Daqifi.Mcp/Tools/DaqifiTools.cs[R21-22]

+        [Description("Discovery timeout in milliseconds (default 2000; clamped to 1000..30000). The floor is bench-measured: the serial identify handshake takes ~830 ms, so a lower budget returns an empty list before a device could ever answer — indistinguishable from none being attached. Successful calls return as soon as a device responds, not at the full timeout.")] int timeoutMs = 2000,
        [Description("Include WiFi/network discovery (default true).")] bool wifi = true,

Relevance

●●● Strong

Team often updates MCP/tool docs when behavior is misleading or mismatched with implementation.

PR-#470
PR-#321

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The tool description explicitly promises early return, but the WiFi discovery implementation
schedules cancellation via CancelAfter(timeout) and then awaits receive-loop tasks that run until
cancellation; therefore WiFi-inclusive discovery generally takes ~the full timeout regardless of how
quickly devices respond.

src/Daqifi.Mcp/Tools/DaqifiTools.cs[17-25]
src/Daqifi.Mcp/DaqifiAgent.cs[99-115]
src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[232-236]
src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[295-300]
src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[323-330]

Agent prompt

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

### Issue description
`discover_devices`'s `timeoutMs` parameter description states that successful calls return as soon as a device responds (i.e., before the timeout). That is not accurate when WiFi discovery is enabled: WiFi discovery starts receive loops and waits for timeout-driven cancellation, so the operation can take approximately the full timeout even after devices respond.

### Issue Context
This is a contract/documentation issue in the MCP tool schema. The core behavior is fine; the problem is the newly-added wording implying early completion for all successful calls.

### Fix Focus Areas
- src/Daqifi.Mcp/Tools/DaqifiTools.cs[17-25]
- (optional) src/Daqifi.Mcp/DaqifiAgent.cs[99-115]

### What to change
- Update the `timeoutMs` description to remove/qualify the "returns as soon as a device responds" claim.
 - Suggested direction: clarify that **WiFi discovery listens up to the timeout window** to collect replies, while serial probing may complete earlier once all probes finish.
 - If you want to keep an "early return" guarantee, that would require behavioral changes (not just docs) to stop WiFi receive loops after the first response / after a short settle window.

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

---

Tip of the day

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

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

/agentic_review

[qodo-code-review]

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