[tylerkron]

What was wrong

The tool descriptions are the only documentation an AI client ever sees, and they had grown into essays. list_analog_outputs ran to a thousand characters. capture_samples, read_channel_values, disable_pwm, get_server_info and the timeoutMs/durationMs parameters retold bench measurements and firmware history. Every client pays for all of it on every tools/list, in context-window space that would otherwise hold the user's actual data.

Two smaller problems came with it:

• get_device_status said it returned "enabled analog channels", but it has always returned digital ones too. So an agent had no reason to call it to check a DIO pin.
• The session rules that matter most (discover before connect, retrieve SD data before you stream, analog output is Nyquist 3 only) lived only in the README, which no agent reads. There was nowhere to put a rule that isn't about one specific tool.

How it was fixed

• Every tool and parameter description is now one job, its failure mode, and how to read the result. The firmware history and bench notes stay in the MCP README, where a human will find them. About 6,000 characters come out of every tools/list (roughly 1,500 tokens).
• get_device_status now names analog and digital channels.
• The session rules are sent once, in the initialization handshake, as MCP ServerInstructions. Clients pass these to the model as context, so a rule can be stated without paying for it on every request.
• The "retrieve before you stream" rule is lifted only for devices that report firmware 3.7.3 or later. Firmware 3.7.3 fixed the SD-buffer collapse (firmware chore(style): enforce file-scoped namespaces and PascalCase public members at build time #703), but v3.7.2 is still the latest stable release, so most devices still need the advice. An empty or unknown version counts as older.
• Contract tests pin the digital-channel wording, the clamp ranges on timeout parameters, the firmware scoping, and a length budget, so the long descriptions can't creep back. A handshake test starts the real server and checks that the rules actually arrive in initialize.

Review notes (Claude)

• Verified end to end over stdio that the SDK in use (ModelContextProtocol 2.2.0) really does put instructions in the initialize result. The field isn't set and then dropped. This is now a test: ServerInstructionsHandshakeTests runs daqifi-mcp.dll from the test output with the SDK's own McpClient, with and without --read-only. Commenting out o.ServerInstructions = ... in Program.cs turns both tests red on net9.0 and net10.0.
• The --read-only rule is conditional. Sent to every session, including ones where writes are allowed, it was noise, plus a hint that writes might be refused on a server where they won't be. It now appears only when the server really is running --read-only, phrased as a statement about this server.
• The SD-collapse rule is scoped to affected firmware (Qodo). Firmware v3.7.3 fixed chore(style): enforce file-scoped namespaces and PascalCase public members at build time #703, but v3.7.3 and v3.8.0 are pre-releases and the in-app updater still offers v3.7.2. So the rule stays, and is lifted only when the firmwareVersion that discover_devices already returns is 3.7.3 or later. Discovery can report an empty or "Unknown" version, so those count as older (second Qodo round). The same wording is in the handshake, in download_sd_file, and in the MCP README. The recovery step now says "power-cycled" rather than "reconnected", which matches the firmware issue and SdCardEmptyTransferException.
• Restored the capture-rate guidance to capture_samples. It was the only text telling an agent how to read measuredRateHz / deviceClockRateHz against sampleRateHz. Nothing else the agent can see carries it: the output schema is generated from [Description] attributes, so the DTO's XML doc comments never reach it. Also put back "configure channels and the sample rate first".
• Reworded set_analog_output. "(latch is device-wide)" was too cryptic to act on. It now says what that costs you: latch=true also applies values staged earlier on other channels.
• Reviewed every other cut against the DTOs and the README. Everything else is either recoverable from the structured result or belongs in human documentation.
• No hardware attached, so this wasn't bench-validated. Nothing here touches the wire.

Tests

Full dotnet test green on net9.0 and net10.0 (4342 Core + 278 MCP).

Merge order

This branch has merged main, with #744's tool hints already in. Each conflict took main's [McpServerTool(...)] line and this PR's [Description] line. It still rewrites the [Description] line next to the [McpServerTool] attribute that #750 (UseStructuredContent) rewrites on all 26 tools, and it touches Program.cs's AddMcpServer block, which #765 also edits. Whichever of these lands second will need a rebase.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Older devices can return empty downloads ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

DownloadSdFile limits the retrieve-before-stream instruction to devices identified as below 3.7.3,
but DiscoveredDevice.From exposes raw discovery firmware values that can be empty or "Unknown".
When such a device is running affected firmware, the client cannot classify it from
discover_devices and can stream first, reaching the SD-buffer collapse and empty later download
described by this tool.

Code

src/Daqifi.Mcp/Tools/DaqifiTools.cs[194]

+    [Description("Download an SD-card log file and by default parse it to CSV. On firmware below 3.7.3, retrieve before any live stream on the same connection — streaming collapses the SD buffer (firmware #703) and later downloads come back empty.")]

Relevance

●●● Strong

Unknown firmware metadata leaves affected devices unprotected; conservative handling or explicit
uncertainty is a clear correctness fix.

PR-#546
PR-#693

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed description directs clients to act only on a version threshold, while the discovery DTO
passes through missing firmware values that cannot be compared to that threshold. Therefore valid
devices with unavailable version metadata receive no safe decision rule despite being potentially
affected.

src/Daqifi.Mcp/Tools/DaqifiTools.cs[193-200]
src/Daqifi.Mcp/Dtos.cs[9-24]
src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[1108-1121]
src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[406-419]
src/Daqifi.Core/Device/Discovery/HidDeviceFinder.cs[111-125]

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 firmware-scoped SD retrieval guidance has no safe fallback when `discover_devices` returns an empty or `Unknown` `firmwareVersion`. Treat unknown firmware as potentially affected, or tell clients explicitly to retrieve before streaming unless they can confirm a version of 3.7.3 or later.

Fix Focus Areas
- src/Daqifi.Mcp/Tools/DaqifiTools.cs[194-194]
- src/Daqifi.Mcp/ServerOptions.cs[77-77]
- src/Daqifi.Mcp/README.md[59-62]

Recommended Fix
Update the session rule, tool description, and README to state that the retrieve-first workflow applies to firmware below 3.7.3 **or an unknown firmware version**, and that clients may skip it only after `discover_devices` reports a known version at or above 3.7.3. Add a contract assertion covering this fallback wording.

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

---

2. Clients receive obsolete SD guidance ✓ Resolved 🔗 Cross-repo conflict ≡ Correctness

Description

ServerOptions.SessionRules says every live stream collapses the SD buffer and leaves downloads
empty until reconnection or SD recording. With the pinned Nyquist firmware, streaming retains the
required 4096-byte SD buffer and any undersized buffer produces an explicit error, so initialized
clients receive incorrect workflow and recovery instructions.

Code

src/Daqifi.Mcp/ServerOptions.cs[77]

+        - Retrieve before you stream. A live streaming session collapses the device's SD buffer (firmware #703), after which downloads come back empty until the device is reconnected or another SD recording re-arms it. Do the SD work first on a fresh connection.

Relevance

●●● Strong

Recent history accepts correcting client-facing descriptions when they contradict actual firmware or
runtime behavior.

PR-#693
PR-#546

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR newly tells every MCP client that live streaming collapses the SD buffer. The pinned firmware
instead enforces a 4096-byte inactive-SD floor, documents the old 512-byte state as an unreachable
historical fault signature, and returns an execution error if the read-buffer invariant is ever
violated.

src/Daqifi.Mcp/ServerOptions.cs[74-79]
src/Daqifi.Mcp/Tools/DaqifiTools.cs[193-195]
External repo: daqifi/daqifi-nyquist-firmware, firmware/src/Util/StreamingBufferPool.h [41-47]
External repo: daqifi/daqifi-nyquist-firmware, docs/MEMORY_ARCHITECTURE.md [150-158]
External repo: daqifi/daqifi-nyquist-firmware, firmware/src/services/sd_card_services/sd_card_manager.c [4200-4213]
External repo: daqifi/daqifi-nyquist-firmware, firmware/src/services/SCPI/SCPIStorageSD.c [811-822]

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 MCP handshake and download-tool description present firmware issue #703 as current behavior for every Nyquist device, but current firmware prevents the buffer collapse and fails explicitly if the invariant is violated.

## Fix Focus Areas
- src/Daqifi.Mcp/ServerOptions.cs[74-79]
- src/Daqifi.Mcp/Tools/DaqifiTools.cs[193-195]
- src/Daqifi.Mcp.Tests/ToolDescriptionContractTests.cs[32-40]

## Recommended Fix
Remove the unconditional claim that streaming makes later downloads empty, or explicitly identify it as compatibility guidance for affected legacy firmware. Apply the same wording to both the handshake and `download_sd_file` description, then update the contract test so it does not require presenting issue #703 as current universal behavior.

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

---

3. Handshake regressions escape the tests ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

ServerInstructions_AreTheReadmeSessionRules reads ServerOptions.Instructions directly instead of
exercising the MCP server registration and initialization response. If the separate
ServerInstructions assignment is removed or broken, these tests remain green while clients
initialize without any session rules.

Code

src/Daqifi.Mcp.Tests/ToolDescriptionContractTests.cs[R35-39]

+        var text = new ServerOptions().Instructions;
+
+        Assert.Contains("discover_devices", text);
+        Assert.Contains("firmware #703", text);
+        Assert.Contains("Nyquist 3", text);

Relevance

●●● Strong

Accepted history favors tests that exercise production behavior and prevent regressions escaping
coverage.

PR-#718
PR-#705

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The tests only instantiate ServerOptions and read its property, while production delivery occurs
independently in Program.cs; therefore they cannot detect a regression in the registration path.

src/Daqifi.Mcp.Tests/ToolDescriptionContractTests.cs[33-53]
src/Daqifi.Mcp/Program.cs[33-47]

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 instruction contract tests inspect `ServerOptions.Instructions` directly, so they do not verify that those instructions reach clients in the MCP initialization response.

## Fix Focus Areas
- src/Daqifi.Mcp.Tests/ToolDescriptionContractTests.cs[33-53]
- src/Daqifi.Mcp/Program.cs[33-47]

## Recommended Fix
Add an integration-style test that constructs or starts the configured MCP server, performs an initialization handshake, and asserts the returned `instructions` for both normal and read-only options. Keep the direct text assertions only if they provide additional focused coverage.

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

---

Context sources

✅ Compliance rules (platform): 13 rules

✅ Cross-repo context — repo relationships

  Explored: repo: daqifi/daqifi-nyquist-firmware (sha: 7d8b27a0) — View relationship

Review mode: ⚖️ Balanced: This small push changes runtime MCP instructions and tool behavior-facing descriptions plus contract and handshake tests, so it has contained but genuine behavioral and compatibility risk.

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit a59dab9

Results up to commit 9ae0ee6 ⚖️ Balanced

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

1. Clients receive obsolete SD guidance ✓ Resolved 🔗 Cross-repo conflict ≡ Correctness

Description

ServerOptions.SessionRules says every live stream collapses the SD buffer and leaves downloads
empty until reconnection or SD recording. With the pinned Nyquist firmware, streaming retains the
required 4096-byte SD buffer and any undersized buffer produces an explicit error, so initialized
clients receive incorrect workflow and recovery instructions.

Code

src/Daqifi.Mcp/ServerOptions.cs[77]

+        - Retrieve before you stream. A live streaming session collapses the device's SD buffer (firmware #703), after which downloads come back empty until the device is reconnected or another SD recording re-arms it. Do the SD work first on a fresh connection.

Relevance

●●● Strong

Recent history accepts correcting client-facing descriptions when they contradict actual firmware or
runtime behavior.

PR-#693
PR-#546

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR newly tells every MCP client that live streaming collapses the SD buffer. The pinned firmware
instead enforces a 4096-byte inactive-SD floor, documents the old 512-byte state as an unreachable
historical fault signature, and returns an execution error if the read-buffer invariant is ever
violated.

src/Daqifi.Mcp/ServerOptions.cs[74-79]
src/Daqifi.Mcp/Tools/DaqifiTools.cs[193-195]
External repo: daqifi/daqifi-nyquist-firmware, firmware/src/Util/StreamingBufferPool.h [41-47]
External repo: daqifi/daqifi-nyquist-firmware, docs/MEMORY_ARCHITECTURE.md [150-158]
External repo: daqifi/daqifi-nyquist-firmware, firmware/src/services/sd_card_services/sd_card_manager.c [4200-4213]
External repo: daqifi/daqifi-nyquist-firmware, firmware/src/services/SCPI/SCPIStorageSD.c [811-822]

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 MCP handshake and download-tool description present firmware issue #703 as current behavior for every Nyquist device, but current firmware prevents the buffer collapse and fails explicitly if the invariant is violated.

## Fix Focus Areas
- src/Daqifi.Mcp/ServerOptions.cs[74-79]
- src/Daqifi.Mcp/Tools/DaqifiTools.cs[193-195]
- src/Daqifi.Mcp.Tests/ToolDescriptionContractTests.cs[32-40]

## Recommended Fix
Remove the unconditional claim that streaming makes later downloads empty, or explicitly identify it as compatibility guidance for affected legacy firmware. Apply the same wording to both the handshake and `download_sd_file` description, then update the contract test so it does not require presenting issue #703 as current universal behavior.

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

---

2. Handshake regressions escape the tests ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

ServerInstructions_AreTheReadmeSessionRules reads ServerOptions.Instructions directly instead of
exercising the MCP server registration and initialization response. If the separate
ServerInstructions assignment is removed or broken, these tests remain green while clients
initialize without any session rules.

Code

src/Daqifi.Mcp.Tests/ToolDescriptionContractTests.cs[R35-39]

+        var text = new ServerOptions().Instructions;
+
+        Assert.Contains("discover_devices", text);
+        Assert.Contains("firmware #703", text);
+        Assert.Contains("Nyquist 3", text);

Relevance

●●● Strong

Accepted history favors tests that exercise production behavior and prevent regressions escaping
coverage.

PR-#718
PR-#705

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The tests only instantiate ServerOptions and read its property, while production delivery occurs
independently in Program.cs; therefore they cannot detect a regression in the registration path.

src/Daqifi.Mcp.Tests/ToolDescriptionContractTests.cs[33-53]
src/Daqifi.Mcp/Program.cs[33-47]

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 instruction contract tests inspect `ServerOptions.Instructions` directly, so they do not verify that those instructions reach clients in the MCP initialization response.

## Fix Focus Areas
- src/Daqifi.Mcp.Tests/ToolDescriptionContractTests.cs[33-53]
- src/Daqifi.Mcp/Program.cs[33-47]

## Recommended Fix
Add an integration-style test that constructs or starts the configured MCP server, performs an initialization handshake, and asserts the returned `instructions` for both normal and read-only options. Keep the direct text assertions only if they provide additional focused coverage.

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

---

[tylerkron]

Reviewed (Claude): APPROVE — the trim is real (about 6,000 characters, roughly 1,500 tokens, off every tools/list) and I checked each cut against the DTOs and the README rather than taking the description on trust. Verified over stdio that ModelContextProtocol 2.2.0 really does put instructions in the initialize result. Three changes: the --read-only rule is now sent only when the server actually runs read-only, capture_samples gets back the measuredRateHz/deviceClockRateHz guidance (nothing else the agent can see carries it — DTO XML docs do not reach the output schema), and set_analog_output says what "device-wide latch" costs you. Not bench-validated — no hardware attached; nothing here touches the wire. Note for merge: this rewrites the [Description] line adjacent to the [McpServerTool] attribute that #744 and #750 both rewrite on all 26 tools, so it will conflict with both; suggested order #744 → #750 → #761 → #765. Qodo-clean on 9ae0ee6, CI green — ready for review.

[qodo-code-review]

PR Summary by Qodo

Streamline MCP descriptions and add session instructions

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Sends session-wide operating rules once during MCP initialization, including conditional read-only
 guidance.
• Trims tool metadata while preserving failure modes, limits, and result interpretation.
• Adds contract tests for wording, timeout clamps, and description length budgets.

Diagram

graph TD
  Options["Server Options"] -->|"session rules"| Program["MCP Setup"] -->|"initialize"| Client["MCP Client"]
  Tools["Tool Metadata"] -->|"tools/list"| Client
  Tests["Contract Tests"] -->|"checks rules"| Options
  Tests -->|"checks budgets"| Tools

Loading

High-Level Assessment

The current split is appropriate: cross-tool operating rules belong in MCP ServerInstructions, while tool-specific jobs, failures, and result semantics remain in descriptions. Keeping all guidance in the README would hide it from agents, and repeating session rules across tools would restore the context overhead this PR removes. Conditional generation also avoids misleading writable sessions with read-only guidance.

Files changed (4) +177 / -33

Enhancement (2) +32 / -4

Program.csPublish session instructions during MCP initialization +11/-4

Publish session instructions during MCP initialization

• Expands MCP server configuration to attach the parsed server options' instructions alongside server identity. Clients now receive shared operating rules once in the initialization handshake.

src/Daqifi.Mcp/Program.cs

ServerOptions.csGenerate mode-aware MCP server instructions +21/-0

Generate mode-aware MCP server instructions

• Defines shared discovery, SD retrieval, and Nyquist 3 rules for ServerInstructions. Appends read-only restrictions only when the server is actually running with --read-only.

src/Daqifi.Mcp/ServerOptions.cs

Bug fix (1) +29 / -29

DaqifiTools.csCondense tool descriptions and correct status wording +29/-29

Condense tool descriptions and correct status wording

• Shortens agent-visible tool and parameter descriptions while retaining actionable failures, limits, and result interpretation. Corrects get_device_status to state that both analog and digital channels are reported.

src/Daqifi.Mcp/Tools/DaqifiTools.cs

Tests (1) +116 / -0

ToolDescriptionContractTests.csAdd contracts for agent-visible MCP guidance +116/-0

Add contracts for agent-visible MCP guidance

• Adds reflection-based tests that enforce tool and parameter description budgets. Verifies analog and digital status wording, timeout clamp documentation, core session rules, and conditional read-only instructions.

src/Daqifi.Mcp.Tests/ToolDescriptionContractTests.cs

[qodo-code-review]

Code review by qodo was updated up to the latest commit 9ae0ee6

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8bf8d1c

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Qodo-clean, CI green — ready for review