[realtonyyoung]

Follow-up to #413. Linear: AI-1586

Tests only — no production change, 106 lines.

#413 landed with three guards whose tests could not fail if the guard were deleted. The code review named all three in its first round; I fixed the other findings and disclosed these instead, then let each subsequent round's new P1 push them down. Stating a gap in a commit message is not closing it.

Why each was vacuous

Guard	Why the old assertion proved nothing
Cursor	asserted exit 0 — but its outer catch-all returns 0 too
Claude client	asserted the degraded-path effects — but deleting the guard lets CreateClientWithinBudgetAsync swallow the exception and take the same branch, so every effect is identical
McpJudgeServer	had no test at all

What replaces them

Cursor and Claude are proved by non-entry, through the injectable client factories they already expose: the factory records invocation and fails the test if called. That is the only assertion form that separates "the guard ran" from "something downstream absorbed it".

McpJudgeServer gets a child-process JSON-RPC exchange. A child is the only honest venue, because the failure being fixed is the process dying before the handshake — an in-process test cannot observe it. The test completes initialize, asserts a tools/call returns a protocol-level error rather than crashing, asserts the server still answers a subsequent tools/list, and asserts it does not exit 2.

Mutation-checked

• delete the Cursor guard → 1 failure
• delete the Claude client guard → 1 failure
• restore eager client construction in the judge → 5 failures

Unit suite diffed against origin/main: one extra failure, ImportChainsAsync_dispatches_independent_chains_in_parallel — a parallel-dispatch timing test unrelated to this change, green 3/3 in isolation. Integration 191/191. AOT publish clean.

[linear-code]

AI-1586

[qodo-code-review]

PR Summary by Qodo

Add mutation-resistant tests for remaining unusable-URL guards

🧪 Tests 🕐 20-40 Minutes

AI Description

• Add non-entry unit tests proving Cursor/Claude unusable-URL guards skip client construction.
• Add child-process integration test for kcap mcp judge JSON-RPC handshake on unusable URLs.
• Assert judge returns protocol-level tool errors and keeps serving after the fault.

Diagram

graph TD
  UT["Guard unit tests"] --> Cursor["Cursor hook guard"] --> Factory["Injected clientFactory (spy)"]
  UT --> Claude["Claude hook guard"] --> Factory
  IT["Judge integration test"] --> Proc["kcap judge process"] --> Rpc["JSON-RPC stdin/stdout"] --> Tool["Judge tool dispatch"] --> Rpc

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. In-process judge test with injectable streams

• ➕ Faster and less flaky than spawning a real process
• ➕ Easier to debug failures (no stdio timing issues)
• ➖ Does not faithfully validate the “process dies before handshake” failure mode in the real CLI entrypoint
• ➖ Requires refactoring to inject Console/stdin/stdout or to expose a reusable transport harness

Recommendation: The chosen approach is appropriate: Cursor/Claude are best proved via non-entry at existing injectable seams, and the judge scenario is most honestly validated with a child process because the historical failure mode was pre-handshake process death. If subprocess flakiness becomes an issue, consider extracting an injectable stdio transport for McpJudgeServer and keeping a single end-to-end subprocess smoke test.

Files changed (2) +106 / -0

Tests (2) +106 / -0

UnusableUrlHookMatrixTests.csAdd subprocess JSON-RPC test for 'kcap mcp judge' unusable-URL behavior +60/-0

Add subprocess JSON-RPC test for 'kcap mcp judge' unusable-URL behavior

• Adds an integration test that launches 'kcap mcp judge' as a child process, completes 'initialize', and validates 'tools/call' returns a protocol-level error message for unusable URLs. Also asserts the server continues to answer 'tools/list' afterwards and does not exit with code 2, using a line-read timeout helper to avoid hangs.

test/Capacitor.Cli.Tests.Integration/UnusableUrlHookMatrixTests.cs

UnusableUrlGuardTests.csProve Cursor/Claude unusable-URL guards via non-entry clientFactory spies +46/-0

Prove Cursor/Claude unusable-URL guards via non-entry clientFactory spies

• Adds unit tests for CursorHookCommand and ClaudeHookCommand that inject a clientFactory which records invocation and fails if entered. Asserts the factory is never called for an unusable URL and that the hook handlers still return exit code 0 (fail-open contract).

test/Capacitor.Cli.Tests.Unit/Http/UnusableUrlGuardTests.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. MCP judge test desync ✓ Resolved 🐞 Bug ☼ Reliability

Description

The Mcp_judge_completes_the_handshake_and_returns_a_tool_error integration test writes to
proc.StandardInput without flushing and uses ReadLineWithTimeoutAsync that returns null without
cancelling the underlying ReadLineAsync. If the child is slow, the request may not be delivered
before the timeout and the abandoned read can later consume a response meant for a subsequent
assertion, making the test flaky.

Code

test/Capacitor.Cli.Tests.Integration/UnusableUrlHookMatrixTests.cs[R243-270]

+        await proc.StandardInput.WriteLineAsync("""{"jsonrpc":"2.0","id":1,"method":"initialize","params":{}}""");
+        var initialize = await ReadLineWithTimeoutAsync(proc);
+        await Assert.That(initialize).IsNotNull();
+        await Assert.That(initialize!).Contains("\"id\":1");
+
+        await proc.StandardInput.WriteLineAsync(
+            """{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"get_session_recap","arguments":{"session_id":"0123456789abcdef0123456789abcdef"}}}""");
+        var toolCall = await ReadLineWithTimeoutAsync(proc);
+
+        // The fault is reported IN the protocol, not by dying.
+        await Assert.That(toolCall).IsNotNull();
+        await Assert.That(toolCall!).Contains("server_url is missing a scheme");
+
+        // And it is still serving afterwards.
+        await proc.StandardInput.WriteLineAsync("""{"jsonrpc":"2.0","id":3,"method":"tools/list","params":{}}""");
+        var afterwards = await ReadLineWithTimeoutAsync(proc);
+        await Assert.That(afterwards).IsNotNull();
+        await Assert.That(afterwards!).Contains("\"id\":3");
+
+        proc.StandardInput.Close();
+        using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(15));
+        await proc.WaitForExitAsync(cts.Token);
+        await Assert.That(proc.ExitCode).IsNotEqualTo(2);
+    }
+
+    static async Task<string?> ReadLineWithTimeoutAsync(Process proc) {
+        var read = proc.StandardOutput.ReadLineAsync();
+        return await Task.WhenAny(read, Task.Delay(TimeSpan.FromSeconds(15))) == read ? await read : null;

Evidence

The new test writes three requests without flushing and uses a non-cancelling timeout wrapper around
ReadLineAsync, while existing MCP integration tests flush stdin and use cancellation-aware reads
with stderr surfaced for debugging, demonstrating the new harness is weaker and can become flaky
under timing variance.

test/Capacitor.Cli.Tests.Integration/UnusableUrlHookMatrixTests.cs[226-271]
test/Capacitor.Cli.Tests.Integration/McpSessionsServerTests.cs[130-145]
test/Capacitor.Cli.Tests.Integration/UnusableUrlHookMatrixTests.cs[148-155]

Agent prompt

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

### Issue description
`Mcp_judge_completes_the_handshake_and_returns_a_tool_error` communicates with a child process over stdio but:
- does not flush after `WriteLineAsync`, risking the child not receiving the request before the timeout.
- implements timeouts via `Task.WhenAny(read, Task.Delay(...))` without cancelling the losing `ReadLineAsync`, which can leave a pending read that later consumes a subsequent response line and desynchronizes the protocol assertions.

### Issue Context
Other MCP integration tests in this repo use a safer harness: flush after each request and use cancellation-aware `ReadLineAsync(ct)` with a timeout `CancellationTokenSource`, and they surface stderr when stdout closes.

### Fix Focus Areas
- test/Capacitor.Cli.Tests.Integration/UnusableUrlHookMatrixTests.cs[226-271]
- test/Capacitor.Cli.Tests.Integration/McpSessionsServerTests.cs[130-145]

### Implementation notes
- After each `WriteLineAsync(...)`, call `await proc.StandardInput.FlushAsync()` (or set `proc.StandardInput.AutoFlush = true` once).
- Replace `ReadLineWithTimeoutAsync` with a cancellation-token-based read pattern (like `McpSessionsServerTests.SendRequest`):
 - `using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(15));`
 - `var line = await proc.StandardOutput.ReadLineAsync(cts.Token);`
- On timeout/`null`, consider reading and including `proc.StandardError.ReadToEndAsync()` in the failure message for debugging.
- Optionally mirror this file’s existing `RunAsync` binary existence guard before `Process.Start` to produce a clearer failure when the CLI binary is missing.

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

---

2. Verbose Mcp_judge XML comment ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The newly added XML doc comments above the Cursor/Claude guard tests (including
Mcp_judge_completes_the_handshake_and_returns_a_tool_error) are overly long and narrative,
reducing readability and increasing maintenance burden. This conflicts with the guideline to keep
comments concise and prefer self-explanatory test names/structure.

Code

test/Capacitor.Cli.Tests.Integration/UnusableUrlHookMatrixTests.cs[R213-223]

+    /// <summary>
+    /// <c>kcap mcp judge</c> was the only one of the eight MCP servers building its authenticated
+    /// client BEFORE opening stdin/stdout, so an unusable URL killed it ahead of the JSON-RPC
+    /// handshake and the judge run failed opaquely.
+    ///
+    /// <para>A child process is the only honest venue: the failure mode being fixed is the process
+    /// dying, which an in-process test cannot observe. The assertions are that the handshake
+    /// completes, that a tool call comes back as a JSON-RPC tool ERROR rather than a crash, and that
+    /// the server is still answering afterwards — a server must keep serving and report the fault in
+    /// its own protocol.</para>
+    /// </summary>

Evidence

PR Compliance ID 5 requires comments to be concise and to prefer self-explanatory code. The cited
changes add multi-paragraph XML doc blocks (including long narrative/background such as historical
or vacuous-assertion explanations and a verbose <para>), which restate test intent and context
that are already conveyed by the descriptive test names and assertions, thereby introducing
unnecessary verbosity.

CLAUDE.md: Keep Comments Concise and Prefer Self-Explanatory Code
test/Capacitor.Cli.Tests.Integration/UnusableUrlHookMatrixTests.cs[213-223]
test/Capacitor.Cli.Tests.Unit/Http/UnusableUrlGuardTests.cs[212-215]
test/Capacitor.Cli.Tests.Unit/Http/UnusableUrlGuardTests.cs[234-238]

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 newly added XML documentation blocks above the relevant guard/integration tests are overly verbose and narrative, duplicating intent and background that should be conveyed by self-explanatory test names and minimal, targeted inline notes.

## Issue Context
PR Compliance ID 5 requires keeping comments concise and preferring self-explanatory code/test structure. The current multi-paragraph XML doc blocks (including long `<para>` narrative and historical/vacuous-assertion background) reduce readability and add maintenance overhead without adding proportional value beyond what the test names and assertions already communicate.

## Fix Focus Areas
- test/Capacitor.Cli.Tests.Integration/UnusableUrlHookMatrixTests.cs[213-223]
- test/Capacitor.Cli.Tests.Unit/Http/UnusableUrlGuardTests.cs[212-215]
- test/Capacitor.Cli.Tests.Unit/Http/UnusableUrlGuardTests.cs[234-238]

ⓘ 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.}