Harden kcap mcp sessions dispatch: guard invalid URL + catch per-request by alexeyzimarev · Pull Request #226 · kurrent-io/kcap-cli

alexeyzimarev · 2026-07-01T17:06:15Z

Problem

The three auto-registered MCP servers now create the authenticated HttpClient lazily on first tools/call. Two dispatch-path gaps (raised by Qodo on #224):

Invalid server_url hard-exits mid-request — reaches EnsureAbsolute → Environment.Exit(2) inside the lazy auth-client factory (uncatchable), terminating the process on first tools/call instead of returning an error.
Unexpected exceptions kill the stdio loop — the tools/call arm has no surrounding catch, so an unexpected failure bubbles out and the server closes stdout without a JSON-RPC response.

Change (sessions — the third server)

Validate server_url once at startup via the pure, local IsAcceptableUrl (no network/token/stderr).
Route tools/call through a guarded dispatcher returning a JSON-RPC tool error for an unusable URL or any unexpected exception; the server keeps serving. initialize / tools/list stay local-only.
Test: Tool_call_with_invalid_server_url_returns_error_and_server_survives.

This is the same hardening applied to flows (#217) and review (#224) — landing per-server since each one's lazy-client change lives in a different place; sessions' lazy client is already on main, so this is standalone.

Verification

Sessions integration suite: 8 passed (incl. the new graceful-failure test).
dotnet build clean; dotnet publish -c Release — no IL3050/IL2026 AOT warnings.

Closes #225.

🤖 Generated with Claude Code

Follow-up to the sessions lazy-auth change (already on main). With the client created lazily, a scheme-less server_url would reach EnsureAbsolute inside the auth-client factory and hard-exit the process (Environment.Exit(2)) mid-request; and any unexpected client-creation/token failure would bubble out of the stdio loop with no JSON-RPC response. Validate the server_url shape once at startup with IsAcceptableUrl (pure local check), and route tools/call through a guarded dispatcher that returns a JSON-RPC tool error for an unusable URL or any unexpected exception. Server keeps serving subsequent requests. Completes the uniform hardening across all three auto-registered MCP servers (flows in #217, review in #224, sessions here). Test: Tool_call_with_invalid_server_url_returns_error_and_server_survives. Sessions integration suite (8) green; no IL3050/IL2026 AOT warnings. Closes #225. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

qodo-code-review · 2026-07-01T17:07:50Z

PR Summary by Qodo

Harden MCP sessions server: validate server_url and guard tools/call

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Validate configured server_url at startup to avoid hard process exits mid-request.
• Wrap tools/call handling to always return a JSON-RPC tool error on failures.
• Add integration test ensuring invalid URLs don’t kill the stdio server loop.

Diagram

graph TD
  A["MCP stdio JSON-RPC loop"] --> B["IsAcceptableUrl(startup)"] --> C{"URL OK?"}
  C -->|"no"| D["JSON-RPC tool error"]
  C -->|"yes"| E["Lazy auth HttpClient"] --> F["HandleToolCallAsync"] --> G[("Capacitor API")]
  E -->|"exception"| D
  F -->|"unexpected exception"| D
  subgraph Legend
    direction LR
    _step["Step"] ~~~ _dec{"Decision"} ~~~ _ext[("External")]
  end

High-Level Assessment

The following are alternative approaches to this PR:

1. Remove Environment.Exit from EnsureAbsolute (throw/Result instead)

➕ Eliminates uncatchable termination across the entire codebase, not just MCP servers
➕ Centralizes URL validation/error handling in one place (HttpClientExtensions)
➖ Behavior change may affect existing CLI commands that rely on fast-fail exit codes
➖ Requires auditing all call sites and updating error handling/messaging consistency

2. Validate server_url earlier (on config set / process startup)

➕ Fails fast before any MCP request is processed
➕ Keeps per-request dispatch logic simpler
➖ MCP servers may be spawned with env overrides; still needs runtime validation
➖ Doesn’t by itself address unexpected per-request exceptions killing the loop

Recommendation: The PR’s approach is appropriate for MCP sessions: validate server_url once (pure check) and guard tools/call so the stdio loop always returns a JSON-RPC response. Longer term, consider replacing EnsureAbsolute’s Environment.Exit with a throwable/Result-based mechanism to prevent similar hard-exit hazards anywhere HttpClientExtensions is used in non-interactive/long-lived processes.

Files changed (2) +47 / -3

Bug fix (1) +21 / -1

McpSessionsServer.csGuard tools/call with startup URL validation and per-request exception handling +21/-1

Guard tools/call with startup URL validation and per-request exception handling

• Adds a one-time server_url shape validation using HttpClientExtensions.IsAcceptableUrl. Routes tools/call through a guarded dispatcher that returns a JSON-RPC tool error when the URL is unusable or when unexpected exceptions occur during lazy HttpClient creation or tool execution, preventing process termination and keeping the stdio loop alive.

src/Capacitor.Cli/Commands/McpSessionsServer.cs

Tests (1) +26 / -2

McpSessionsServerTests.csAdd integration test for invalid server_url and server survival +26/-2

Add integration test for invalid server_url and server survival
• Extends the MCP server spawner to allow overriding KCAP_URL. Adds an end-to-end stdio JSON-RPC test verifying that a scheme-less/invalid server_url produces an isError tool response and that the server continues responding to subsequent requests.
test/Capacitor.Cli.Tests.Integration/McpSessionsServerTests.cs

qodo-code-review · 2026-07-01T17:12:37Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 Harden MCP server dispatch: invalid server_url hard-exits mid tools/call; unexpected errors kill the loop

1. ~~Faulted lazy client sticks~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

If the first lazy authenticated-client creation throws, the cached Lazy<Task<HttpClient>> remains
faulted and every later tools/call will continue failing for the lifetime of the process. This
undermines the goal of “server keeps serving” after transient failures (e.g., temporary IO issues
reading tokens).

Code

src/Capacitor.Cli/Commands/McpSessionsServer.cs[R26-45]

        var clientLazy = new Lazy<Task<HttpClient>>(() => HttpClientExtensions.CreateAuthenticatedClientAsync(baseUrl));

+        // Validate the server_url shape once, locally (pure string check — no network, token,
+        // or stderr). Used to fail gracefully instead of hard-exiting mid-request (below).
+        var urlOk = HttpClientExtensions.IsAcceptableUrl(baseUrl);
+
+        // Guarded tool dispatch: never let the stdio JSON-RPC loop die on one bad request. An
+        // unusable server_url would otherwise reach EnsureAbsolute inside the lazy auth-client
+        // factory, which hard-exits the process (Environment.Exit(2)) mid-request; and an
+        // unexpected client-creation/token failure would bubble out of the loop. Return a
+        // JSON-RPC tool error in both cases so the server keeps serving.
+        async Task<string> DispatchToolCallAsync(JsonNode callId, JsonObject callRequest) {
+            if (!urlOk)
+                return BuildToolResult(callId, HttpClientExtensions.SchemeMissingHint, isError: true);
+
+            try {
+                return await HandleToolCallAsync(callId, callRequest, await clientLazy.Value, baseUrl, cwdRepoHash);
+            } catch (Exception ex) {
+                return BuildToolResult(callId, $"Error: {ex.Message}", isError: true);
+            }

Evidence
The server caches client creation in a process-lifetime Lazy<Task<HttpClient>> and awaits it
inside tools/call dispatch. Token loading deliberately propagates real IO/permission exceptions
(only missing-file cases are handled), so client creation can throw; once it does, the cached lazy
value will keep failing on subsequent tools/calls unless reset.
src/Capacitor.Cli/Commands/McpSessionsServer.cs[19-46]
src/Capacitor.Cli.Core/Auth/TokenStore.cs[55-76]
src/Capacitor.Cli.Core/Auth/TokenStore.cs[225-252]

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 server uses `Lazy<Task<HttpClient>>` for on-demand client creation. If the factory throws once, the faulted task is reused on every subsequent access, causing persistent per-request failures even if the underlying problem was transient.

### Issue Context
Client creation can throw due to real IO/permission faults in token/profile loading (these are intentionally not swallowed). After this PR, those exceptions are caught and converted into tool errors, but the server cannot recover because the lazy remains faulted.

### Fix
Replace the `Lazy<Task<HttpClient>>` with a retryable pattern:
- Keep a nullable `HttpClient? client` (or `Task<HttpClient>? clientTask`) and create it on-demand inside `DispatchToolCallAsync`.
- If creation fails, do **not** cache the failure; return an error and allow the next tools/call to attempt creation again.
- Preserve existing disposal behavior: dispose the client at shutdown if it was successfully created.

(Concurrency note: the stdio loop processes requests serially, so a simple nullable field is sufficient; no heavy locking required.)

### Fix Focus Areas
- src/Capacitor.Cli/Commands/McpSessionsServer.cs[19-46]
- src/Capacitor.Cli.Core/Auth/TokenStore.cs[55-76]
- src/Capacitor.Cli.Core/Auth/TokenStore.cs[225-252]

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

2. ~~Exception text disclosure~~ ✓ Resolved 🐞 Bug ⛨ Security

Description

DispatchToolCallAsync returns raw exception messages in the JSON-RPC tool result, which can expose
local runtime details (e.g., filesystem paths/permission errors) to the MCP client. This turns
otherwise-internal failures into client-visible information disclosure.

Code

src/Capacitor.Cli/Commands/McpSessionsServer.cs[R41-45]

+            try {
+                return await HandleToolCallAsync(callId, callRequest, await clientLazy.Value, baseUrl, cwdRepoHash);
+            } catch (Exception ex) {
+                return BuildToolResult(callId, $"Error: {ex.Message}", isError: true);
+            }

Evidence

The new guarded dispatcher returns ex.Message directly to the tool caller. Separately, token
loading intentionally lets real IO/permission exceptions propagate (only missing-file cases are
swallowed), so those exception messages can include local paths and environment details that would
now be sent back over JSON-RPC.

src/Capacitor.Cli/Commands/McpSessionsServer.cs[28-46]
src/Capacitor.Cli.Core/Auth/TokenStore.cs[55-76]

Agent prompt

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

### Issue description
`DispatchToolCallAsync` catches `Exception` and returns `ex.Message` in the JSON-RPC tool result. Exception messages can contain sensitive local details (e.g., token/config file paths from IO exceptions) and should not be forwarded verbatim to the MCP client.

### Issue Context
This catch-all path is intended as a safety net to keep the stdio loop alive. It should preserve reliability without disclosing internal details.

### Fix
- Return a generic tool error message to the client (e.g., `"Error: unexpected failure (see stderr)"`).
- Log the full exception (preferably `ex.ToString()`) to `Console.Error` for debugging.
- Optionally include a short correlation id in both stderr and the tool error so failures can be traced without leaking details.

### Fix Focus Areas
- src/Capacitor.Cli/Commands/McpSessionsServer.cs[28-46]

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

qodo-code-review · 2026-07-01T17:12:38Z

        var clientLazy = new Lazy<Task<HttpClient>>(() => HttpClientExtensions.CreateAuthenticatedClientAsync(baseUrl));

+        // Validate the server_url shape once, locally (pure string check — no network, token,
+        // or stderr). Used to fail gracefully instead of hard-exiting mid-request (below).
+        var urlOk = HttpClientExtensions.IsAcceptableUrl(baseUrl);
+
+        // Guarded tool dispatch: never let the stdio JSON-RPC loop die on one bad request. An
+        // unusable server_url would otherwise reach EnsureAbsolute inside the lazy auth-client
+        // factory, which hard-exits the process (Environment.Exit(2)) mid-request; and an
+        // unexpected client-creation/token failure would bubble out of the loop. Return a
+        // JSON-RPC tool error in both cases so the server keeps serving.
+        async Task<string> DispatchToolCallAsync(JsonNode callId, JsonObject callRequest) {
+            if (!urlOk)
+                return BuildToolResult(callId, HttpClientExtensions.SchemeMissingHint, isError: true);
+
+            try {
+                return await HandleToolCallAsync(callId, callRequest, await clientLazy.Value, baseUrl, cwdRepoHash);
+            } catch (Exception ex) {
+                return BuildToolResult(callId, $"Error: {ex.Message}", isError: true);
+            }


2. Faulted lazy client sticks 🐞 Bug ☼ Reliability

If the first lazy authenticated-client creation throws, the cached Lazy<Task<HttpClient>> remains faulted and every later tools/call will continue failing for the lifetime of the process. This undermines the goal of “server keeps serving” after transient failures (e.g., temporary IO issues reading tokens).

Agent Prompt

### Issue description The MCP server uses `Lazy<Task<HttpClient>>` for on-demand client creation. If the factory throws once, the faulted task is reused on every subsequent access, causing persistent per-request failures even if the underlying problem was transient. ### Issue Context Client creation can throw due to real IO/permission faults in token/profile loading (these are intentionally not swallowed). After this PR, those exceptions are caught and converted into tool errors, but the server cannot recover because the lazy remains faulted. ### Fix Replace the `Lazy<Task<HttpClient>>` with a retryable pattern: - Keep a nullable `HttpClient? client` (or `Task<HttpClient>? clientTask`) and create it on-demand inside `DispatchToolCallAsync`. - If creation fails, do **not** cache the failure; return an error and allow the next tools/call to attempt creation again. - Preserve existing disposal behavior: dispose the client at shutdown if it was successfully created. (Concurrency note: the stdio loop processes requests serially, so a simple nullable field is sufficient; no heavy locking required.) ### Fix Focus Areas - src/Capacitor.Cli/Commands/McpSessionsServer.cs[19-46] - src/Capacitor.Cli.Core/Auth/TokenStore.cs[55-76] - src/Capacitor.Cli.Core/Auth/TokenStore.cs[225-252]

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

…x.Message leak Two findings on the guarded dispatcher: 1. Reliability: a `Lazy<Task<HttpClient>>` caches a faulted task, so a single transient client-creation failure would make every later tools/call re-throw for the rest of the session — the guard turned a would-be crash (which the host restarts and retries) into a permanently-erroring server. Create the client on demand into a nullable field instead: a failed attempt leaves it null and the next call retries. Safe without locking — the stdio loop handles one request at a time. 2. Security: the catch-all returned `ex.Message` to the MCP client, which could forward local details (e.g. file paths from IO exceptions). It's a safety net for *unexpected* exceptions (expected ones are already handled with useful messages inside HandleToolCallAsync), so log the full exception to stderr and return a generic tool error instead. Sessions integration suite (8) green; no IL3050/IL2026 AOT warnings.

…Message leak Mirror the sessions fixes (#226) in the flows dispatcher: 1. Reliability: replace Lazy<Task<HttpClient>> with an on-demand nullable client so a transient creation failure leaves it null and the next tools/call retries, instead of a faulted task sticking for the session. The AI-1061 InfiniteTimeSpan timeout is applied on creation. 2. Security: the catch-all logs the full exception to stderr and returns a generic tool error instead of forwarding ex.Message to the client. Flows integration suite (18) green; no IL3050/IL2026 AOT warnings.

alexeyzimarev · 2026-07-01T17:29:25Z

Thanks @qodo-code-review — both fixed in c806604.

1. Faulted lazy client sticks (Reliability) — fixed. Good catch: a Lazy<Task<HttpClient>> caches a faulted task, so a single transient client-creation failure would make every later tools/call re-throw for the rest of the session — my guard had turned a would-be crash (which the host restarts → retries) into a permanently-erroring server. Replaced the Lazy<Task> with an on-demand nullable HttpClient? created inside the dispatcher (client ??= await …); a failed attempt leaves it null so the next call retries. No lock needed — the stdio loop handles one request at a time. Startup stays local-only (initialize/tools-list never create it), so the deferral property is preserved.

2. Exception text disclosure (Security) — fixed. The catch-all now logs the full exception to stderr and returns a generic "internal error handling the request." to the client instead of ex.Message. It's a safety net for unexpected exceptions only — the expected ones (ArgumentException, HttpRequestException, 401/HTTP errors) are still handled inside HandleToolCallAsync with their specific, safe messages.

Applied the same two fixes to the sibling servers for consistency: flows in #217, review in #224.

Sessions integration suite (8) green; no IL3050/IL2026 AOT warnings.

qodo-code-review · 2026-07-01T17:29:37Z

Thanks @qodo-code-review — both fixed in c806604.

1. Faulted lazy client sticks (Reliability) — fixed. Good catch: a Lazy<Task<HttpClient>> caches a faulted task, so a single transient client-creation failure would make every later tools/call re-throw for the rest of the session — my guard had turned a would-be crash (which the host restarts → retries) into a permanently-erroring server. Replaced the Lazy<Task> with an on-demand nullable HttpClient? created inside the dispatcher (client ??= await …); a failed attempt leaves it null so the next call retries. No lock needed — the stdio loop handles one request at a time. Startup stays local-only (initialize/tools-list never create it), so the deferral property is preserved.

2. Exception text disclosure (Security) — fixed. The catch-all now logs the full exception to stderr and returns a generic "internal error handling the request." to the client instead of ex.Message. It's a safety net for unexpected exceptions only — the expected ones (ArgumentException, HttpRequestException, 401/HTTP errors) are still handled inside HandleToolCallAsync with their specific, safe messages.

Applied the same two fixes to the sibling servers for consistency: flows in #217, review in #224.

Sessions integration suite (8) green; no IL3050/IL2026 AOT warnings.

Thanks — this looks good.

finding 1 is resolved: switching away from Lazy<Task<HttpClient>> avoids the faulted-task stickiness, and the on-demand nullable client preserves the local-only startup behavior.
finding 2 is resolved: the client now gets a generic error while the full exception stays on stderr, which avoids leaking implementation details.

Nice touch applying the same pattern to the sibling servers too. The green integration run and AOT-warning check are reassuring as well.

…uash-merge (#227) #217 (flows) and #224 (review) were squash-merged at tips that predated their final commits, so main ended up missing work that CI had passed on those PRs: - flows: has only the bare lazy client — missing the startup server_url guard, the per-request try/catch, the on-demand-client-with-retry (Qodo #226), the generic error message, and the invalid-URL integration test. - review: has the guard but still uses Lazy<Task> + returns ex.Message — missing the on-demand-client-with-retry and generic error message. sessions (#226) landed complete. This brings flows and review to the same final state (byte-identical to the merged sessions dispatch pattern): on-demand nullable client so a transient creation failure retries instead of a faulted task sticking; catch-all logs to stderr and returns a generic tool error rather than ex.Message; startup server_url validation so a bad URL yields a JSON-RPC error instead of Environment.Exit mid-request. Restored the final McpFlowsServer.cs / McpReviewServer.cs and the flows invalid-URL integration test from the (still-present) branch commits cd69ef3 and d6b5a9e. Flows integration (18), review integration (4), full unit (2028) green; no IL3050/IL2026 AOT warnings.

alexeyzimarev mentioned this pull request Jul 1, 2026

Defer kcap mcp review auth client until first tool call #224

Merged

qodo-code-review Bot reviewed Jul 1, 2026

View reviewed changes

alexeyzimarev merged commit d78134c into main Jul 1, 2026
5 checks passed

alexeyzimarev deleted the harden-sessions-mcp branch July 1, 2026 17:31

alexeyzimarev mentioned this pull request Jul 1, 2026

Restore flows + review MCP dispatch hardening + Qodo fixes lost in squash-merge #227

Merged

qodo-code-review Bot mentioned this pull request Sep 23, 2026

Find a sessionless driver's flow from a local run ledger #1131

Merged

Navigation Menu

Repository navigation

Harden kcap mcp sessions dispatch: guard invalid URL + catch per-request - #226

Harden kcap mcp sessions dispatch: guard invalid URL + catch per-request#226
alexeyzimarev merged 2 commits into
mainfrom
harden-sessions-mcp

alexeyzimarev commented Jul 1, 2026

Uh oh!

qodo-code-review Bot commented Jul 1, 2026

Uh oh!

qodo-code-review Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

qodo-code-review Bot Jul 1, 2026

Uh oh!

alexeyzimarev commented Jul 1, 2026

Uh oh!

qodo-code-review Bot commented Jul 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

alexeyzimarev commented Jul 1, 2026

Problem

Change (sessions — the third server)

Verification

Uh oh!

qodo-code-review Bot commented Jul 1, 2026

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

qodo-code-review Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

alexeyzimarev commented Jul 1, 2026

Uh oh!

qodo-code-review Bot commented Jul 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

qodo-code-review Bot commented Jul 1, 2026 •

edited

Loading