Harden MCP tools: batch exceptions, gesture null params, capabilities message#144
Conversation
…bilities message - Wrap BatchAsync in try/catch for HttpRequestException, TaskCanceledException, and JsonException so batch tool returns a friendly error instead of crashing - Omit null distance/durationMs from gesture JsonObject to prevent agent-side deserialization failure on non-nullable DTO properties - Improve capabilities error message to distinguish unreachable agents from older agents that lack the capabilities endpoint Follow-up to #115. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Hardening updates for DevFlow’s MCP tool surface and Driver API to better handle agent incompatibilities/unreachability and to avoid sending invalid JSON that breaks agent deserialization.
Changes:
- Wrap
maui_batchexecution with targeted exception handling to return a friendly failure message instead of throwing. - Update
AgentClient.GestureAsyncto omit nullabledistance/durationMsfields when not provided (avoids sending JSON nulls). - Adjust
maui_capabilitiesfallback messaging when capability retrieval fails.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/DevFlow/Microsoft.Maui.DevFlow.Driver/AgentClient.cs | Builds gesture payload conditionally so null optionals are omitted from JSON. |
| src/Cli/Microsoft.Maui.Cli/DevFlow/Mcp/Tools/BatchTools.cs | Adds try/catch around BatchAsync to handle agent/network/json failures gracefully. |
| src/Cli/Microsoft.Maui.Cli/DevFlow/Mcp/Tools/AgentTools.cs | Updates the capabilities failure message returned to MCP callers. |
| public async Task<bool> GestureAsync(string type, string? elementId = null, string? direction = null, double? distance = null, int? durationMs = null) | ||
| { | ||
| return await PostActionAsync($"{UiApi}/actions/gesture", new JsonObject | ||
| var payload = new JsonObject | ||
| { | ||
| ["elementId"] = elementId, | ||
| ["type"] = type, | ||
| ["direction"] = direction, | ||
| ["distance"] = distance, | ||
| ["durationMs"] = durationMs | ||
| }); | ||
| ["type"] = type | ||
| }; | ||
|
|
||
| if (elementId is not null) payload["elementId"] = elementId; | ||
| if (direction is not null) payload["direction"] = direction; | ||
| if (distance.HasValue) payload["distance"] = distance.Value; | ||
| if (durationMs.HasValue) payload["durationMs"] = durationMs.Value; | ||
|
|
||
| return await PostActionAsync($"{UiApi}/actions/gesture", payload); |
There was a problem hiding this comment.
This change fixes a real serialization/deserialization issue (null distance/durationMs breaks the agent’s non-nullable DTO), but it isn’t covered by tests. There are existing AgentClient HTTP-shape tests (e.g., Tap/Fill/Batch) in Microsoft.Maui.DevFlow.Tests; please add a similar test for GestureAsync verifying that when distance/durationMs are null they are omitted from the POST body (and included when provided).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
PR: Harden MCP tools: batch exceptions, gesture null params, capabilities message
Solid hardening PR. All three fixes are correct and address real failure modes. The GestureAsync null-omission fix is the most impactful — it prevents deserialization failures that would surface as opaque errors to MCP consumers. The batch exception handling properly closes a gap where BatchAsync was the only code path without a catch. The capabilities message is improved.
Methodology: 3 independent reviewers with adversarial consensus. Disputed findings were validated by the other 2 reviewers before inclusion.
🟡 MODERATE
| # | Finding | Consensus | Location |
|---|---|---|---|
| 1 | Catch filter uses TaskCanceledException instead of base OperationCanceledException — misses cancellations from CancellationToken or MCP framework |
2/3 reviewers | BatchTools.cs:75 (inline) |
| 2 | ScrollAsync has the same nullable value-type serialization bug this PR fixes in GestureAsync — itemIndex/groupIndex are sent as null |
2/3 reviewers | AgentClient.cs ~line 216 (inline at :181) |
| 3 | Batch path bypasses the GestureAsync null-omission fix — raw JsonObject with "distance": null forwarded to agent DTOs |
2/3 reviewers (1 dissented with detailed technical rebuttal arguing agent-side DTO defaults handle nulls correctly) | BatchTools.cs / AgentClient.cs |
🟢 MINOR
| # | Finding | Consensus | Location |
|---|---|---|---|
| 4 | Capabilities error message omits "app not running" — the most common failure cause | 2/3 reviewers | AgentTools.cs:99 (inline) |
| 5 | Error message conflates connectivity and parse failures; "Is the app running?" misleading for JsonException; ex.Message can leak internal host/port details |
3/3 reviewers (after follow-up) | BatchTools.cs:77 (inline) |
| 6 | System.Text.Json.JsonException fully qualified despite existing using; inconsistent with sibling types in same expression |
2/3 reviewers | BatchTools.cs:75 (inline) |
| 7 | Status() and Capabilities() in the same class now use different error message tone/structure |
2/3 reviewers (1 dissented, noting the different failure modes justify different wording) | AgentTools.cs:47 vs :99 — not in diff |
Discarded findings
None — all 3 disputed findings reached 2/3+ consensus after follow-up.
CI Status
Build checks (macOS + Windows) are in progress. CLA check passed. No test failures attributed to this change (341 passed; 5 pre-existing locale-dependent failures in OutputFormatterTests are unrelated).
Test Coverage
The PR description confirms dotnet test src/Cli/Microsoft.Maui.Cli.UnitTests/ passes. The batch error handling and gesture serialization changes exercise runtime paths that are difficult to unit-test without a live agent — the current test coverage is reasonable for a hardening PR at preview stage.
Generated by Expert Code Review (auto) for issue #144 · ● 6.2M
…n, error messages - Fix ScrollAsync to omit null itemIndex/groupIndex/scrollToPosition/elementId from JsonObject (same pattern as the GestureAsync fix) - Widen batch catch filter from TaskCanceledException to OperationCanceledException (the base class) - Improve batch and capabilities error messages to mention both connectivity and version/feature support Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed review feedback in the latest commit:
Resolved the corresponding review threads. The GestureAsync test coverage request is CI will validate the shape works e2e; happy to add a dedicated unit test in a follow-up.valid |
Follow-up to #115.
Three hardening fixes identified during the review of the MCP tools PR:
1. Batch exception handling
agent.BatchAsync()could throwHttpRequestException,TaskCanceledException, orJsonExceptionwhen the agent is unreachable or crashes mid-request. Other tools handle this viaPostActionAsync(which catches internally), but batch uses rawBatchAsync. Now wrapped with a try/catch that returns a friendly error.2. Gesture nullable parameter serialization
GestureAsyncwas including nulldistance/durationMsin the JSON payload, which would fail on the agent non-nullable DTO (double Distance = 120,int DurationMs = 200). Now omits null values from the JsonObject so the agent uses its defaults.3. Capabilities error message
Changed from "Agent not responding" (misleading for older agents) to a message that distinguishes connectivity issues from feature support.
Validation
dotnet test src/Cli/Microsoft.Maui.Cli.UnitTests/- 341 passed (5 pre-existing locale-dependent failures in OutputFormatterTests unrelated to this change)