kcap mcp analytics: governed SQL analytics tools for coding agents

[stktung]

Closes #343. Linear: AI-1477 (parent AI-1468). Server side: kurrent-io/kcap-server#1157 + kurrent-io/kcap-server#1158 (merge those first — against an older server the tools return a clear "upgrade kcap-server" message).

What

New kcap mcp analytics stdio server with two read-only tools wrapping the bearer-authed /api/analytics endpoints:

• get_analytics_schema — fetches the governed schema document (views/columns, glossary, SQL rules, worked examples) and unwraps the text envelope. Description steers agents to call it once before writing SQL.
• query_analytics(sql, scope? "repo"|"global", max_rows?) — one governed Postgres SELECT. Defaults to the cwd-derived repo hash; fail-closed when the repo is unresolvable (error suggests scope: "global", mirroring McpMemoryServer.BuildSaveBody). Success bodies pass through as raw JSON with an explicit truncation trailer; 400 → REJECTED: {validator reason} for agent self-repair; 408 → narrow-the-query hint; 401 → not-logged-in; 404 → upgrade hint.

Structure cloned from McpMemoryServer (deferred authenticated client, 401 refresh retry, guarded dispatch, AOT-safe JsonNode bodies).

Registration (v1: Claude Code + Codex only)

• KcapMcpServers.All gains kcap-analytics (ReadOnly: true → Codex per-server trust; NeedsProjectCwd: true) — the contract tests derive both bundled manifests from this, and both are updated.
• ForCursor name-filters it out (the kcap-workitems pattern), so Cursor/Copilot/OpenCode/Kiro/Gemini/Antigravity don't get it yet — widening is AI-1475.
• KcapMcpRegistry: StartsFlows: false, deliberately not added to the review-flow auto-approvable sets.
• Help text (help-mcp.txt, help-usage.txt) + README (overview + new "Analytics MCP server" section) in this PR.

Tests

• Unit (10 new): query-body scope mapping (repo default / global / fail-closed cwd / unknown scope / missing sql / max_rows passthrough), response mapping (schema unwrap, truncation trailer, REJECTED: detail, status→message table), tools-list schema.
• Updated exact-set suites: KcapMcpServersTests (6 canonical servers, ForCodex/ForCursor sets, ReadOnly pin), McpCanonicalContractTests (Codex includes / Cursor excludes analytics), PluginCommandCursorTests (cursor mcp.json excludes analytics), AcpHostedAgentRuntimeFactoryTests (+kcap-analytics as non-auto-approvable).
• Integration (2 new): spawned-binary stdio handshake — serverInfo/instructions, tools/list with the schema-first routing cue.
• AOT: trim analysis clean (no IL2xxx/IL3xxx); the local native-link step failed on a machine toolchain issue (vswhere.exe missing), unrelated to this change — CI runs the real publish.

🤖 Generated with Claude Code

[stktung]

/review

[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. Linear IDs in comments ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

New/modified comments include Linear issue identifiers (e.g., AI-1475, AI-1468, AI-1470). This
violates the requirement to avoid Linear IDs in code comments, which reduces long-term
maintainability and portability of the source.

Code

src/Capacitor.Cli.Core/Mcp/KcapMcpServers.cs[R36-40]

    /// <summary>The shared set for every non-Claude JSON harness (Cursor, Copilot, OpenCode,
    /// Kiro, Gemini, Antigravity) — omits `kcap-workitems` (Claude Code plugin only;
-    /// its session-id default rides the Claude hook env). Unlike Codex, these still get
-    /// `kcap-flows`.</summary>
+    /// its session-id default rides the Claude hook env) and, for now, `kcap-analytics`
+    /// (v1 rollout is Claude Code + Codex only — widening to these harnesses is AI-1475).
+    /// Unlike Codex, these still get `kcap-flows`.</summary>

Evidence

PR Compliance ID 5 forbids Linear issue IDs in comments. The cited locations include new comments
containing AI-1475, AI-1470, and AI-1468.

CLAUDE.md: Do not include Linear issue numbers in code comments
src/Capacitor.Cli.Core/Mcp/KcapMcpServers.cs[36-40]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[167-169]
test/Capacitor.Cli.Tests.Unit/Mcp/McpCanonicalContractTests.cs[63-66]

Agent prompt

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

## Issue description
Code comments added/modified in this PR reference Linear issue IDs (e.g., `AI-1475`). Compliance requires that code comments do not include Linear IDs; use a GitHub issue number if a durable reference is necessary, or remove the issue reference entirely.

## Issue Context
Linear IDs are external and unstable for repository readers/auditors; comments should remain repo-relevant.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Mcp/KcapMcpServers.cs[36-40]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[167-169]
- test/Capacitor.Cli.Tests.Unit/Mcp/McpCanonicalContractTests.cs[63-66]

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

---

2. Schema timeout hint wrong ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

McpAnalyticsServer returns the query-specific timeout hint ("narrow the date range or aggregate")
for any OperationCanceledException or HTTP 408, even when the timed-out tool is
get_analytics_schema. This produces incorrect recovery guidance during schema fetches and can
misroute agent self-repair behavior.

Code

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[R157-161]

+        } catch (OperationCanceledException) {
+            // Client-side HttpClient timeout (TaskCanceledException : OperationCanceledException):
+            // the request never got a server 408, so give the same actionable hint the 408 path
+            // does rather than letting it fall through to the generic outer "internal error".
+            return BuildToolResult(id, TimedOutMessage, isError: true);

Evidence

The timeout message is explicitly query-oriented, but both the client-side timeout catch and HTTP
408 mapping return it without checking which tool was invoked; the unit test also validates the
query-oriented text, reinforcing that this message is tailored for query execution rather than
schema retrieval.

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[23-24]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[157-161]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[183-184]
test/Capacitor.Cli.Tests.Unit/McpAnalyticsServerTests.cs[84-99]

Agent prompt

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

### Issue description
`TimedOutMessage` is worded for `query_analytics` (date range / aggregation), but it is returned for timeouts from **both** tools (`get_analytics_schema` and `query_analytics`) via the `OperationCanceledException` catch and the `HttpStatusCode.RequestTimeout` mapping. This yields misleading guidance when the schema call times out.

### Issue Context
- Client-side timeouts surface as `OperationCanceledException`/`TaskCanceledException`.
- Server-side timeouts surface as HTTP 408.
- Both paths currently return the same `TimedOutMessage`, which is query-specific.

### Fix Focus Areas
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[23-24]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[157-161]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[183-184]
- test/Capacitor.Cli.Tests.Unit/McpAnalyticsServerTests.cs[84-101]

### Suggested fix
- Introduce a separate, generic timeout message for schema fetches (e.g., `SchemaTimedOutMessage = "Request timed out — retry."`).
- In the `OperationCanceledException` catch, return:
 - `TimedOutMessage` when `toolName == "query_analytics"`
 - `SchemaTimedOutMessage` when `toolName == "get_analytics_schema"`
- In `MapResponse` for `HttpStatusCode.RequestTimeout`, also branch on `toolName` similarly.
- (Optional but recommended) add/extend a unit test to cover timeout behavior for `get_analytics_schema` so this doesn’t regress.

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

---

3. Timeouts become internal error ✓ Resolved 🐞 Bug ☼ Reliability

Description

McpAnalyticsServer.HandleToolCallAsync does not catch
OperationCanceledException/TaskCanceledException from HttpClient, so slow/hung requests can fall
into the outer DispatchToolCallAsync catch and return a generic "internal error" instead of a
timeout hint (and won’t hit the 408 mapping unless the server responds). This makes analytics
queries harder for agents to self-repair (e.g., narrowing date range / aggregating) when a request
times out locally.

Code

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[R141-159]

+        try {
+            using var httpResponse = toolName switch {
+                "get_analytics_schema" => await SendWithRefreshRetryAsync(client, c => c.GetAsync($"{baseUrl}/api/analytics/schema")),
+                "query_analytics"      => await SendWithRefreshRetryAsync(client, c => c.PostAsync($"{baseUrl}/api/analytics/query", ToJsonContent(BuildQueryBody(arguments, cwdRepoHash)))),
+                _                      => throw new ArgumentException($"Unknown tool: {toolName}")
+            };
+
+            var body = await httpResponse.Content.ReadAsStringAsync();
+
+            return BuildToolResult(id, MapResponse(toolName, httpResponse.StatusCode, body, out var isError), isError);
+        } catch (ArgumentException ex) {
+            return BuildToolResult(id, $"Error: {ex.Message}", isError: true);
+        } catch (HttpRequestException ex) {
+            return BuildToolResult(id, $"Error: {ex.Message}", isError: true);
+        } catch (Exception ex) when (ex is InvalidOperationException or FormatException or JsonException) {
+            // A wrong-typed argument that slipped past the tolerant readers still becomes a tool
+            // error the agent can react to — never the generic outer "internal error".
+            return BuildToolResult(id, $"Error: malformed arguments — {ex.Message}", isError: true);
+        }

Evidence

The tool-call handler catches ArgumentException/HttpRequestException and some JSON/format
exceptions, but not cancellation/timeout exceptions; those would be caught by the outer dispatch
catch and turned into the generic internal error. The codebase explicitly documents that HTTP
timeouts commonly surface via cancellation exceptions unless wrapped with a linked
CancellationTokenSource, which reinforces the risk here.

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[34-44]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[141-159]
src/Capacitor.Cli.Core/HttpClientExtensions.cs[142-150]

Agent prompt

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

### Issue description
`McpAnalyticsServer.HandleToolCallAsync` does not handle client-side HTTP timeouts/cancellations (`TaskCanceledException`/`OperationCanceledException`). When they occur, they bubble to `DispatchToolCallAsync`'s broad catch and get reported as a generic internal error, rather than an actionable timeout message consistent with the existing HTTP 408 mapping.

### Issue Context
`MapResponse` already maps server-returned `408 RequestTimeout` to a helpful “Query timed out — narrow the date range or aggregate.” message, but this is bypassed if the client times out before receiving a response.

### Fix Focus Areas
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[34-44]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[141-159]

### Proposed fix
- Add a `catch (OperationCanceledException ex)` (or `catch (TaskCanceledException ex)`) in `HandleToolCallAsync` to return `BuildToolResult(id, "Query timed out — narrow the date range or aggregate.", isError: true)` (or similar), so client-side timeouts produce actionable guidance.
- Optionally, distinguish explicit cancellation vs timeout if you later introduce cancellation tokens; currently no CT is passed to `GetAsync`/`PostAsync`, so these exceptions will predominantly represent timeouts.

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

---

4. Opaque arg-type failures ✓ Resolved 🐞 Bug ☼ Reliability

Description

McpAnalyticsServer reads JSON-RPC/tool args with JsonNode.GetValue<T>() but only catches
ArgumentException/HttpRequestException, so wrong-typed values can throw (e.g.,
InvalidOperationException) and get turned into the generic "internal error" tool response. This
prevents agents from self-repairing on simple schema/type mistakes (they don’t get a field-specific
validation message).

Code

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[R130-152]

+        var paramsNode = request["params"]?.AsObject();
+        var toolName   = paramsNode?["name"]?.GetValue<string>();
+        var arguments  = paramsNode?["arguments"]?.AsObject();
+
+        if (toolName is null) {
+            return BuildErrorResponse(id, -32602, "Missing params.name");
+        }
+
+        try {
+            using var httpResponse = toolName switch {
+                "get_analytics_schema" => await SendWithRefreshRetryAsync(client, c => c.GetAsync($"{baseUrl}/api/analytics/schema")),
+                "query_analytics"      => await SendWithRefreshRetryAsync(client, c => c.PostAsync($"{baseUrl}/api/analytics/query", ToJsonContent(BuildQueryBody(arguments, cwdRepoHash)))),
+                _                      => throw new ArgumentException($"Unknown tool: {toolName}")
+            };
+
+            var body = await httpResponse.Content.ReadAsStringAsync();
+
+            return BuildToolResult(id, MapResponse(toolName, httpResponse.StatusCode, body, out var isError), isError);
+        } catch (ArgumentException ex) {
+            return BuildToolResult(id, $"Error: {ex.Message}", isError: true);
+        } catch (HttpRequestException ex) {
+            return BuildToolResult(id, $"Error: {ex.Message}", isError: true);
+        }

Evidence

The analytics server currently uses GetValue<string>() for request fields but only catches
ArgumentException locally; other parse/type exceptions fall to the outer catch-all which returns a
generic internal error. The codebase already demonstrates a safer parsing pattern
(TryGetValue<string>) used to avoid these throws and provide controlled validation behavior.

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[35-45]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[130-152]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[233-247]
src/Capacitor.Cli/Commands/McpReviewServer.cs[369-377]

Agent prompt

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

### Issue description
`McpAnalyticsServer` parses `params.name`, `params.arguments`, and tool arguments (`sql`, `scope`) using `JsonNode.GetValue<T>()`. When an MCP client sends a wrong-typed value (common with LLM clients), `GetValue<T>()` can throw non-`ArgumentException` exceptions (e.g. `InvalidOperationException`). Those exceptions bypass the local `catch (ArgumentException)` and are handled by the outer catch-all, which returns a generic `"Error: internal error handling the request."`.

### Issue Context
There is already an in-repo precedent for defensive argument reading that avoids throwing and enables field-specific feedback (`TryGetValue<T>` pattern in `McpReviewServer`).

### Fix Focus Areas
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[35-45]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[130-152]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[233-247]
- src/Capacitor.Cli/Commands/McpReviewServer.cs[369-377]

### Suggested implementation approach
- Replace `GetValue<string>()` reads for user-provided fields with a defensive helper (e.g., `TryGetStringArg`) similar to `McpReviewServer`.
- For numeric args, keep using the tolerant int parsing, but ensure type mismatches and non-integer numbers return an `ArgumentException` with a precise message.
- Expand `HandleToolCallAsync` error handling to translate `InvalidOperationException`/`JsonException`/`FormatException` into a tool error like `Error: <field> must be a string` (or return JSON-RPC `-32602` if preferred), rather than falling through to the outer generic internal error.

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

---

View more (1)

5. Verbose comments in McpAnalyticsServer ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

Several newly added comments are overly verbose and explain behavior that is already clear from the
code structure. This conflicts with the requirement to keep comments concise to improve
maintainability.

Code

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[R12-17]

+/// <summary>
+/// `kcap mcp analytics` — governed SQL analytics over the server's read-model views.
+/// Two read-only tools wrapping the bearer-authed /api/analytics endpoints: the agent
+/// fetches the governed schema document, writes SQL itself, and self-repairs from the
+/// server's machine-consumable rejection reasons. Structure cloned from McpMemoryServer.
+/// </summary>

Evidence

PR Compliance ID 6 requires concise comments. The cited comment blocks are newly added and contain
multi-line explanatory prose that could be shortened or moved to README/help text.

CLAUDE.md: Keep code comments concise; prefer self-explanatory code over verbose comments
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[12-17]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[30-33]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[155-157]

Agent prompt

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

## Issue description
New comments are longer than necessary and provide extensive narrative guidance that can be moved to documentation or reduced to short intent-focused notes.

## Issue Context
Compliance prefers concise, high-signal comments; verbose prose in code becomes stale and increases review/maintenance overhead.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[12-17]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[30-33]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[155-157]

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

---

6. Cross-server helper coupling ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

McpAnalyticsServer.BuildQueryBody calls McpMemoryServer.TryReadInt, introducing a compile-time
dependency between two otherwise independent MCP servers. This increases refactor risk
(moving/splitting one server can break the other) for a small utility function.

Code

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[R254-255]

+        if (McpMemoryServer.TryReadInt(args, "max_rows", out var maxRows)) body["max_rows"] = maxRows;
+

Evidence

The analytics server directly references an internal parsing helper implemented inside the memory
server, creating an avoidable dependency edge between two otherwise separate command servers.

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[249-256]
src/Capacitor.Cli/Commands/McpMemoryServer.cs[279-324]

Agent prompt

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

### Issue description
`McpAnalyticsServer` depends on `McpMemoryServer.TryReadInt` for parsing `max_rows`. This couples two separate MCP servers at compile time for a generic JSON-argument utility.

### Issue Context
The helper is broadly applicable to multiple MCP servers and doesn’t logically belong to the memory server.

### Fix Focus Areas
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[249-256]
- src/Capacitor.Cli/Commands/McpMemoryServer.cs[279-324]

### Suggested implementation approach
- Move `TryReadInt` into a small shared static utility (e.g., `McpArgParsing.TryReadInt`) in `src/Capacitor.Cli/Commands/`.
- Update both servers to reference the shared helper.
- Keep existing unit tests; optionally add a tiny unit test class for the shared helper if desired.

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

---

Previous review results

Review updated until commit 056174f

Results up to commit 323d1f9 ⚖️ Balanced

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

1. Linear IDs in comments ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

New/modified comments include Linear issue identifiers (e.g., AI-1475, AI-1468, AI-1470). This
violates the requirement to avoid Linear IDs in code comments, which reduces long-term
maintainability and portability of the source.

Code

src/Capacitor.Cli.Core/Mcp/KcapMcpServers.cs[R36-40]

    /// <summary>The shared set for every non-Claude JSON harness (Cursor, Copilot, OpenCode,
    /// Kiro, Gemini, Antigravity) — omits `kcap-workitems` (Claude Code plugin only;
-    /// its session-id default rides the Claude hook env). Unlike Codex, these still get
-    /// `kcap-flows`.</summary>
+    /// its session-id default rides the Claude hook env) and, for now, `kcap-analytics`
+    /// (v1 rollout is Claude Code + Codex only — widening to these harnesses is AI-1475).
+    /// Unlike Codex, these still get `kcap-flows`.</summary>

Evidence

PR Compliance ID 5 forbids Linear issue IDs in comments. The cited locations include new comments
containing AI-1475, AI-1470, and AI-1468.

CLAUDE.md: Do not include Linear issue numbers in code comments
src/Capacitor.Cli.Core/Mcp/KcapMcpServers.cs[36-40]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[167-169]
test/Capacitor.Cli.Tests.Unit/Mcp/McpCanonicalContractTests.cs[63-66]

Agent prompt

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

## Issue description
Code comments added/modified in this PR reference Linear issue IDs (e.g., `AI-1475`). Compliance requires that code comments do not include Linear IDs; use a GitHub issue number if a durable reference is necessary, or remove the issue reference entirely.

## Issue Context
Linear IDs are external and unstable for repository readers/auditors; comments should remain repo-relevant.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Mcp/KcapMcpServers.cs[36-40]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[167-169]
- test/Capacitor.Cli.Tests.Unit/Mcp/McpCanonicalContractTests.cs[63-66]

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

---

2. Opaque arg-type failures ✓ Resolved 🐞 Bug ☼ Reliability

Description

McpAnalyticsServer reads JSON-RPC/tool args with JsonNode.GetValue<T>() but only catches
ArgumentException/HttpRequestException, so wrong-typed values can throw (e.g.,
InvalidOperationException) and get turned into the generic "internal error" tool response. This
prevents agents from self-repairing on simple schema/type mistakes (they don’t get a field-specific
validation message).

Code

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[R130-152]

+        var paramsNode = request["params"]?.AsObject();
+        var toolName   = paramsNode?["name"]?.GetValue<string>();
+        var arguments  = paramsNode?["arguments"]?.AsObject();
+
+        if (toolName is null) {
+            return BuildErrorResponse(id, -32602, "Missing params.name");
+        }
+
+        try {
+            using var httpResponse = toolName switch {
+                "get_analytics_schema" => await SendWithRefreshRetryAsync(client, c => c.GetAsync($"{baseUrl}/api/analytics/schema")),
+                "query_analytics"      => await SendWithRefreshRetryAsync(client, c => c.PostAsync($"{baseUrl}/api/analytics/query", ToJsonContent(BuildQueryBody(arguments, cwdRepoHash)))),
+                _                      => throw new ArgumentException($"Unknown tool: {toolName}")
+            };
+
+            var body = await httpResponse.Content.ReadAsStringAsync();
+
+            return BuildToolResult(id, MapResponse(toolName, httpResponse.StatusCode, body, out var isError), isError);
+        } catch (ArgumentException ex) {
+            return BuildToolResult(id, $"Error: {ex.Message}", isError: true);
+        } catch (HttpRequestException ex) {
+            return BuildToolResult(id, $"Error: {ex.Message}", isError: true);
+        }

Evidence

The analytics server currently uses GetValue<string>() for request fields but only catches
ArgumentException locally; other parse/type exceptions fall to the outer catch-all which returns a
generic internal error. The codebase already demonstrates a safer parsing pattern
(TryGetValue<string>) used to avoid these throws and provide controlled validation behavior.

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[35-45]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[130-152]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[233-247]
src/Capacitor.Cli/Commands/McpReviewServer.cs[369-377]

Agent prompt

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

### Issue description
`McpAnalyticsServer` parses `params.name`, `params.arguments`, and tool arguments (`sql`, `scope`) using `JsonNode.GetValue<T>()`. When an MCP client sends a wrong-typed value (common with LLM clients), `GetValue<T>()` can throw non-`ArgumentException` exceptions (e.g. `InvalidOperationException`). Those exceptions bypass the local `catch (ArgumentException)` and are handled by the outer catch-all, which returns a generic `"Error: internal error handling the request."`.

### Issue Context
There is already an in-repo precedent for defensive argument reading that avoids throwing and enables field-specific feedback (`TryGetValue<T>` pattern in `McpReviewServer`).

### Fix Focus Areas
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[35-45]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[130-152]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[233-247]
- src/Capacitor.Cli/Commands/McpReviewServer.cs[369-377]

### Suggested implementation approach
- Replace `GetValue<string>()` reads for user-provided fields with a defensive helper (e.g., `TryGetStringArg`) similar to `McpReviewServer`.
- For numeric args, keep using the tolerant int parsing, but ensure type mismatches and non-integer numbers return an `ArgumentException` with a precise message.
- Expand `HandleToolCallAsync` error handling to translate `InvalidOperationException`/`JsonException`/`FormatException` into a tool error like `Error: <field> must be a string` (or return JSON-RPC `-32602` if preferred), rather than falling through to the outer generic internal error.

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

---

3. Verbose comments in McpAnalyticsServer ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

Several newly added comments are overly verbose and explain behavior that is already clear from the
code structure. This conflicts with the requirement to keep comments concise to improve
maintainability.

Code

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[R12-17]

+/// <summary>
+/// `kcap mcp analytics` — governed SQL analytics over the server's read-model views.
+/// Two read-only tools wrapping the bearer-authed /api/analytics endpoints: the agent
+/// fetches the governed schema document, writes SQL itself, and self-repairs from the
+/// server's machine-consumable rejection reasons. Structure cloned from McpMemoryServer.
+/// </summary>

Evidence

PR Compliance ID 6 requires concise comments. The cited comment blocks are newly added and contain
multi-line explanatory prose that could be shortened or moved to README/help text.

CLAUDE.md: Keep code comments concise; prefer self-explanatory code over verbose comments
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[12-17]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[30-33]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[155-157]

Agent prompt

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

## Issue description
New comments are longer than necessary and provide extensive narrative guidance that can be moved to documentation or reduced to short intent-focused notes.

## Issue Context
Compliance prefers concise, high-signal comments; verbose prose in code becomes stale and increases review/maintenance overhead.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[12-17]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[30-33]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[155-157]

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

---

4. Cross-server helper coupling ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

McpAnalyticsServer.BuildQueryBody calls McpMemoryServer.TryReadInt, introducing a compile-time
dependency between two otherwise independent MCP servers. This increases refactor risk
(moving/splitting one server can break the other) for a small utility function.

Code

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[R254-255]

+        if (McpMemoryServer.TryReadInt(args, "max_rows", out var maxRows)) body["max_rows"] = maxRows;
+

Evidence

The analytics server directly references an internal parsing helper implemented inside the memory
server, creating an avoidable dependency edge between two otherwise separate command servers.

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[249-256]
src/Capacitor.Cli/Commands/McpMemoryServer.cs[279-324]

Agent prompt

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

### Issue description
`McpAnalyticsServer` depends on `McpMemoryServer.TryReadInt` for parsing `max_rows`. This couples two separate MCP servers at compile time for a generic JSON-argument utility.

### Issue Context
The helper is broadly applicable to multiple MCP servers and doesn’t logically belong to the memory server.

### Fix Focus Areas
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[249-256]
- src/Capacitor.Cli/Commands/McpMemoryServer.cs[279-324]

### Suggested implementation approach
- Move `TryReadInt` into a small shared static utility (e.g., `McpArgParsing.TryReadInt`) in `src/Capacitor.Cli/Commands/`.
- Update both servers to reference the shared helper.
- Keep existing unit tests; optionally add a tiny unit test class for the shared helper if desired.

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

---

Results up to commit 93b65b4 ⚖️ Balanced

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

1. Timeouts become internal error ✓ Resolved 🐞 Bug ☼ Reliability

Description

McpAnalyticsServer.HandleToolCallAsync does not catch
OperationCanceledException/TaskCanceledException from HttpClient, so slow/hung requests can fall
into the outer DispatchToolCallAsync catch and return a generic "internal error" instead of a
timeout hint (and won’t hit the 408 mapping unless the server responds). This makes analytics
queries harder for agents to self-repair (e.g., narrowing date range / aggregating) when a request
times out locally.

Code

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[R141-159]

+        try {
+            using var httpResponse = toolName switch {
+                "get_analytics_schema" => await SendWithRefreshRetryAsync(client, c => c.GetAsync($"{baseUrl}/api/analytics/schema")),
+                "query_analytics"      => await SendWithRefreshRetryAsync(client, c => c.PostAsync($"{baseUrl}/api/analytics/query", ToJsonContent(BuildQueryBody(arguments, cwdRepoHash)))),
+                _                      => throw new ArgumentException($"Unknown tool: {toolName}")
+            };
+
+            var body = await httpResponse.Content.ReadAsStringAsync();
+
+            return BuildToolResult(id, MapResponse(toolName, httpResponse.StatusCode, body, out var isError), isError);
+        } catch (ArgumentException ex) {
+            return BuildToolResult(id, $"Error: {ex.Message}", isError: true);
+        } catch (HttpRequestException ex) {
+            return BuildToolResult(id, $"Error: {ex.Message}", isError: true);
+        } catch (Exception ex) when (ex is InvalidOperationException or FormatException or JsonException) {
+            // A wrong-typed argument that slipped past the tolerant readers still becomes a tool
+            // error the agent can react to — never the generic outer "internal error".
+            return BuildToolResult(id, $"Error: malformed arguments — {ex.Message}", isError: true);
+        }

Evidence

The tool-call handler catches ArgumentException/HttpRequestException and some JSON/format
exceptions, but not cancellation/timeout exceptions; those would be caught by the outer dispatch
catch and turned into the generic internal error. The codebase explicitly documents that HTTP
timeouts commonly surface via cancellation exceptions unless wrapped with a linked
CancellationTokenSource, which reinforces the risk here.

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[34-44]
src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[141-159]
src/Capacitor.Cli.Core/HttpClientExtensions.cs[142-150]

Agent prompt

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

### Issue description
`McpAnalyticsServer.HandleToolCallAsync` does not handle client-side HTTP timeouts/cancellations (`TaskCanceledException`/`OperationCanceledException`). When they occur, they bubble to `DispatchToolCallAsync`'s broad catch and get reported as a generic internal error, rather than an actionable timeout message consistent with the existing HTTP 408 mapping.

### Issue Context
`MapResponse` already maps server-returned `408 RequestTimeout` to a helpful “Query timed out — narrow the date range or aggregate.” message, but this is bypassed if the client times out before receiving a response.

### Fix Focus Areas
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[34-44]
- src/Capacitor.Cli/Commands/McpAnalyticsServer.cs[141-159]

### Proposed fix
- Add a `catch (OperationCanceledException ex)` (or `catch (TaskCanceledException ex)`) in `HandleToolCallAsync` to return `BuildToolResult(id, "Query timed out — narrow the date range or aggregate.", isError: true)` (or similar), so client-side timeouts produce actionable guidance.
- Optionally, distinguish explicit cancellation vs timeout if you later introduce cancellation tokens; currently no CT is passed to `GetAsync`/`PostAsync`, so these exceptions will predominantly represent timeouts.

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

---

[stktung]

/review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 93b65b4

[stktung]

/review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 13b6695

[stktung]

/review

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 056174f

[qodo-code-review]

PR Summary by Qodo

Add kcap-analytics MCP server for governed SQL analytics queries

✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Add kcap mcp analytics MCP stdio server with schema + governed SELECT query tools.
• Register kcap-analytics across harness manifests, marking it read-only for auto-trust.
• Add unit/integration tests and update help/README docs for new analytics tooling.

Diagram

graph TD
A["Agent harness (MCP client)"] --> B["kcap mcp analytics (stdio)"] --> C["McpAnalyticsServer"] --> D{{"kcap-server /api/analytics"}}
C --> E["RepoDetection (cwd→repo hash)"]
C --> F["TokenStore (401 refresh)"]

subgraph Legend
  direction LR
  _ext{{"External"}} ~~~ _svc(["Service"]) ~~~ _mod["Module"]
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Fold analytics tools into an existing MCP server (e.g., kcap-sessions)

• ➕ Fewer servers to register/manage
• ➕ Reuse existing server entry points
• ➖ Harder to apply per-server trust/read-only policy cleanly
• ➖ Mixes unrelated domains (transcripts vs analytics SQL)
• ➖ Increases blast radius of changes and tool surface

2. Introduce a shared MCP server base/helper (dispatch/auth/error mapping)

• ➕ Reduces duplicated JSON-RPC loop and tolerant-arg parsing across servers
• ➕ Centralizes auth retry and error envelope patterns
• ➖ Broader refactor/churn across stable MCP servers
• ➖ Risk of subtle behavior changes and AOT constraints
• ➖ Higher upfront review and rollout cost

3. Provide higher-level analytics endpoints instead of SQL

• ➕ Less SQL malformation risk; simpler agent interaction
• ➕ More controlled semantics per question type
• ➖ Less flexible for ad-hoc analysis
• ➖ Requires ongoing endpoint expansion/versioning
• ➖ Doesn’t mirror the governed SQL surface of the UI

Recommendation: A dedicated read-only kcap-analytics server is the best fit: it keeps domain separation, enables per-server trust/auto-approval where supported, and preserves the governed SQL workflow. A shared MCP base could be considered later if more servers adopt the same patterns, but deferring avoids cross-cutting refactor risk.

Files changed (25) +865 / -22

Enhancement (3) +393 / -4

KcapMcpServers.csAdd kcap-analytics to canonical MCP server list (read-only) +3/-3

Add kcap-analytics to canonical MCP server list (read-only)

• Adds 'kcap-analytics' with 'NeedsProjectCwd: true' and 'ReadOnly: true' so writers can register it and harnesses can auto-trust it where supported.

src/Capacitor.Cli.Core/Mcp/KcapMcpServers.cs

McpAnalyticsServer.csImplement analytics MCP stdio server with governed schema/query tools +386/-0

Implement analytics MCP stdio server with governed schema/query tools

• Adds 'kcap mcp analytics' JSON-RPC stdio server exposing 'get_analytics_schema' (unwraps 'text' envelope) and 'query_analytics' (POSTs governed SELECT). Defaults to repo scope via cwd-derived repo hash and fails closed when repo is unresolvable; supports 'scope: global' and 'max_rows'. Maps statuses to actionable messages, surfaces validator 'detail' as 'REJECTED: ...', appends an explicit truncation warning trailer, and retries once after token refresh on 401.

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs

Program.csWire 'kcap mcp analytics' into CLI dispatch and usage +4/-1

Wire 'kcap mcp analytics' into CLI dispatch and usage

• Adds 'analytics' to MCP usage output and dispatches the subcommand to 'McpAnalyticsServer.RunAsync'.

src/Capacitor.Cli/Program.cs

Refactor (3) +6 / -8

CodexConfigToml.csReference canonical Codex MCP server set in XML docs +2/-4

Reference canonical Codex MCP server set in XML docs

• Updates documentation to refer to 'KcapMcpServers.ForCodex' as the authoritative set of registered servers.

src/Capacitor.Cli.Core/CodexConfigToml.cs

CodingAgentsStep.csDerive Codex MCP registration success message from canonical list +2/-2

Derive Codex MCP registration success message from canonical list

• Replaces hard-coded Codex server name list with 'KcapMcpServers.ForCodex' when printing registration output.

src/Capacitor.Cli/Commands/CodingAgentsStep.cs

PluginCommand.csDerive Codex MCP registration output from canonical list +2/-2

Derive Codex MCP registration output from canonical list

• Updates plugin install messaging to print the server names from 'KcapMcpServers.ForCodex' rather than a fixed string list.

src/Capacitor.Cli/Commands/PluginCommand.cs

Tests (4) +188 / -7

McpAnalyticsServerTests.csIntegration tests for analytics MCP stdio handshake and tools/list +160/-0

Integration tests for analytics MCP stdio handshake and tools/list

• Spawns the built 'kcap' binary and validates initialize response serverInfo/instructions and tools/list contents, pinning the schema-first routing cue in tool descriptions.

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

JsonMcpConfigWriterTests.csUpdate JSON MCP writer test to follow canonical server count +1/-1

Update JSON MCP writer test to follow canonical server count

• Changes expectations from a fixed server count to 'KcapMcpServers.All.Count' for stability as servers are added.

test/Capacitor.Cli.Tests.Unit/Mcp/JsonMcpConfigWriterTests.cs

KcapMcpServersTests.csPin six canonical servers and analytics read-only flag +13/-6

Pin six canonical servers and analytics read-only flag

• Updates canonical-set assertions to include 'kcap-analytics' and adds a test verifying analytics is marked 'ReadOnly' (drives per-server trust where supported).

test/Capacitor.Cli.Tests.Unit/Mcp/KcapMcpServersTests.cs

McpCanonicalContractTests.csContract tests ensure analytics included in Codex/Cursor subsets +14/-0

Contract tests ensure analytics included in Codex/Cursor subsets

• Adds tests asserting 'kcap-analytics' is present in 'ForCodex' and 'ForCursor', alongside existing workitems exclusions.

test/Capacitor.Cli.Tests.Unit/Mcp/McpCanonicalContractTests.cs

Documentation (4) +57 / -3

README.mdDocument analytics MCP server and read-only trust behavior +18/-1

Document analytics MCP server and read-only trust behavior

• Adds an overview of 'kcap mcp analytics', its governed SQL purpose, and how scope is derived from CWD. Updates per-harness trust/auto-approval guidance to include 'kcap-analytics' as a read-only server.

README.md

README.mdDescribe kcap-analytics server tools and requirements +12/-1

Describe kcap-analytics server tools and requirements

• Documents the new analytics MCP server, lists the two tools, and clarifies repo-aware scope plus login and server-version requirements.

kcap/README.md

help-mcp.txtAdd 'mcp analytics' help + include in install list +26/-1

Add 'mcp analytics' help + include in install list

• Documents the two tools, schema-first workflow, scope behavior, and login/server requirements; updates install instructions to include 'kcap-analytics'.

src/Capacitor.Cli.Core/Resources/help-mcp.txt

help-usage.txtExpose 'mcp analytics' in CLI usage output +1/-0

Expose 'mcp analytics' in CLI usage output

• Adds the analytics server to the MCP subcommand list with a short description.

src/Capacitor.Cli.Core/Resources/help-usage.txt

Other (11) +221 / -0

.codex-mcp.jsonAdd kcap-analytics to bundled Codex MCP manifest +4/-0

Add kcap-analytics to bundled Codex MCP manifest

• Adds a 'kcap-analytics' entry that runs 'kcap mcp analytics' for Codex descriptor-based registration.

kcap/.codex-mcp.json

.mcp.jsonAdd kcap-analytics to bundled Claude MCP manifest with schema-first cue +6/-0

Add kcap-analytics to bundled Claude MCP manifest with schema-first cue

• Adds 'kcap-analytics' with a description instructing agents to call 'get_analytics_schema' before 'query_analytics', and documents repo/default + global scope usage.

kcap/.mcp.json

KcapMcpRegistry.csAdd kcap-analytics to kcap-owned MCP registry +1/-0

Add kcap-analytics to kcap-owned MCP registry

• Registers 'kcap-analytics' as a known allowlist entry with 'StartsFlows: false', keeping it out of unattended review-flow auto-approvable sets.

src/Capacitor.Cli.Core/KcapMcpRegistry.cs

McpAnalyticsServerTests.csUnit tests for analytics request/response mapping and argument validation +199/-0

Unit tests for analytics request/response mapping and argument validation

• Adds coverage for query body construction (repo/global, fail-closed, unknown scope, missing/wrong-typed args, max_rows), tool-call protocol errors on wrong-shaped params, client-side timeout hints, schema unwrap, truncation trailer, RFC-7807 detail extraction, and status-to-message mapping.

test/Capacitor.Cli.Tests.Unit/McpAnalyticsServerTests.cs

PluginCommandAntigravityTests.csInstaller test: Antigravity config includes analytics (no trust knob) +2/-0

Installer test: Antigravity config includes analytics (no trust knob)

• Asserts 'kcap-analytics' is registered for Antigravity and that no 'trust' field is written for harnesses without that capability.

test/Capacitor.Cli.Tests.Unit/PluginCommandAntigravityTests.cs

PluginCommandCopilotTests.csInstaller test: Copilot config includes analytics +1/-0

Installer test: Copilot config includes analytics

• Extends Copilot installer tests to assert the analytics server entry is written while preserving user servers.

test/Capacitor.Cli.Tests.Unit/PluginCommandCopilotTests.cs

PluginCommandCursorTests.csInstaller test: Cursor config includes analytics; still excludes workitems +2/-0

Installer test: Cursor config includes analytics; still excludes workitems

• Ensures Cursor registration includes 'kcap-analytics' (repo-aware) while keeping the Claude-only workitems server excluded.

test/Capacitor.Cli.Tests.Unit/PluginCommandCursorTests.cs

PluginCommandGeminiTests.csInstaller test: Gemini config includes analytics with trust enabled +2/-0

Installer test: Gemini config includes analytics with trust enabled

• Asserts 'kcap-analytics' is registered for Gemini and is marked trusted alongside other read-only servers.

test/Capacitor.Cli.Tests.Unit/PluginCommandGeminiTests.cs

PluginCommandKiroTests.csInstaller test: Kiro config includes analytics (no trust knob) +2/-0

Installer test: Kiro config includes analytics (no trust knob)

• Asserts 'kcap-analytics' is registered for Kiro and that no trust field is written, while preserving user auto-approve config.

test/Capacitor.Cli.Tests.Unit/PluginCommandKiroTests.cs

PluginCommandOpenCodeTests.csInstaller test: OpenCode config includes analytics +1/-0

Installer test: OpenCode config includes analytics

• Extends OpenCode installer tests to assert 'kcap-analytics' is included in the MCP server block.

test/Capacitor.Cli.Tests.Unit/PluginCommandOpenCodeTests.cs

AcpHostedAgentRuntimeFactoryTests.csDisallow analytics as auto-approvable in unattended review flows +1/-0

Disallow analytics as auto-approvable in unattended review flows

• Adds 'kcap-analytics' to the non-auto-approvable allowlist-entry test inputs so review-flow spawning fails fast if it’s requested for unattended use.

test/Capacitor.Cli.Tests.Unit/Services/AcpHostedAgentRuntimeFactoryTests.cs