Skip to content

[AI-1139] kcap mcp flow-result + review-flow launcher injection - #240

Merged
alexeyzimarev merged 5 commits into
mainfrom
alexeyzimarev/ai-1139-flow-result-mcp
Jul 3, 2026
Merged

alexeyzimarev merged 5 commits into
mainfrom
alexeyzimarev/ai-1139-flow-result-mcp

Conversation

@alexeyzimarev

Copy link
Copy Markdown
Member

CLI half of AI-1139 — pairs with kurrent-io/kcap-server#920 (merged). Hosted review-flow reviewers get a dedicated result-submission MCP tool instead of relying solely on the transcript marker.

  • kcap mcp flow-result — new stdio MCP server (deliberately separate from kcap mcp flows: a hard security boundary, no flow-starting tools can leak to an unattended reviewer). One tool: submit_review_result(round_token, kind, findings) → POST /api/flows/reviewer/result. Reads KCAP_FLOW_AGENT_ID from daemon-injected env (exit 2 if run manually). Retry policy for the launch race (no_active_flow/no_open_round: 5 attempts, 3 s apart); stale_round_token surfaces immediately with the server's discard guidance and deliberately NO marker-fallback hint (routing a stale result through the marker would bypass the round-token guard); all other errors carry the marker-fallback reminder.
  • CodexLauncher — review-flow launches now inject exactly this server via clear-then-whitelist: -c mcp_servers={} FIRST (replaces the whole config.toml table), then the dotted kcap-flow-result overrides into the empty table. Order is the security invariant (dotted overrides alone MERGE and would re-expose user-registered servers); tests pin it.
  • ClaudeLauncher — the strict --mcp-config now whitelists exactly kcap-flow-result (AOT-safe JsonNode construction); --strict-mcp-config and --disallowedTools Agent unchanged. Both launchers fall back to zero servers when the daemon lacks a server URL / kcap path.

Testing

  • Unit 2079/2079; new coverage: raw-wire snake_case body, retry-then-succeed + exhaust-with-hint, stale-token no-retry/no-hint, validation-without-network, missing-env exit, clear-before-dotted order, exactly-one-server configs, no-injection defaults.
  • Integration 74/76 — both failures verified pre-existing/environmental (one fails on base too; the other passes in isolation and its polluting output comes from the Codex hook path this branch never touches).
  • NativeAOT publish smoke: no IL2xxx/IL3xxx warnings on the new/changed files.

Mixed versions are safe both ways: old daemon + new server → marker path; new daemon + old server → tool error pointing at the marker fallback.

🤖 Generated with Claude Code

alexeyzimarev and others added 4 commits July 2, 2026 18:58
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…server (AI-1139)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ar-then-whitelist) (AI-1139)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…via strict MCP config (AI-1139)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Jul 2, 2026

Copy link
Copy Markdown

AI-1139

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add flow-result MCP server and strictly whitelist it for review-flow launches

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Add kcap mcp flow-result MCP server to submit hosted review-flow results via HTTP.
• Inject/whitelist only kcap-flow-result for review-flow (Codex clear-then-whitelist; Claude
 strict config).
• Add unit tests pinning retry/error semantics and the launcher security invariants.
Diagram

graph TD
  D["kcap daemon"] --> CL["CodexLauncher"] --> A1["Codex reviewer"] --> MCP["kcap mcp flow-result"] --> API[("kcap-server API")]
  D --> LL["ClaudeLauncher"] --> A2["Claude reviewer"] --> MCP --> API
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Constrain existing `kcap mcp flows` via a "reviewer mode" flag
  • ➕ No new MCP server/command to maintain
  • ➕ Reuses existing auth + JSON-RPC plumbing
  • ➖ Higher regression risk: tool filtering bugs could re-expose flow-starting tools
  • ➖ Harder to make the security boundary obvious and testable
2. Have the daemon submit results directly (no reviewer-side MCP tool)
  • ➕ Eliminates the need to run an extra stdio MCP server process
  • ➕ Centralizes retry logic and server interactions in the daemon
  • ➖ Requires new daemon↔reviewer signaling for result payloads
  • ➖ More protocol/state surface area than the current “tool call → POST” design

Recommendation: Keep the PR’s approach: a separate kcap mcp flow-result command is the clearest security boundary, and the launcher-side clear/whitelist + strict-config mechanisms make “exactly one server” enforceable and testable. Alternatives either increase regression risk (filtering tools in-place) or require larger daemon/protocol changes.

Files changed (9) +1179 / -13

Enhancement (5) +342 / -6
ClaudeLauncher.csWhitelist kcap-flow-result via strict MCP config for review-flow +28/-1

Whitelist kcap-flow-result via strict MCP config for review-flow

• Replaces the prior empty strict MCP config with a generated JsonNode-based config that contains exactly one server entry (kcap-flow-result) when ServerUrl/CapacitorPath are available; otherwise keeps the empty map fallback. Preserves the existing '--strict-mcp-config' and '--disallowedTools Agent' guardrails.

src/Capacitor.Cli.Daemon/Services/ClaudeLauncher.cs

CodexLauncher.csClear then whitelist kcap-flow-result for review-flow Codex launches +24/-4

Clear then whitelist kcap-flow-result for review-flow Codex launches

• Keeps the 'mcp_servers={}' table reset for review-flow launches and adds dotted overrides that register only the kcap-flow-result server, with ordering that prevents merging user-configured MCP servers. Skips injection entirely (zero servers) when ServerUrl/CapacitorPath are missing.

src/Capacitor.Cli.Daemon/Services/CodexLauncher.cs

McpFlowResultServer.csAdd flow-result stdio MCP server with submit_review_result tool +285/-0

Add flow-result stdio MCP server with submit_review_result tool

• Implements a dedicated stdio JSON-RPC MCP server exposing only 'submit_review_result', guarded by 'KCAP_FLOW_AGENT_ID' and designed as a security boundary from 'kcap mcp flows'. Adds validation, differentiated error messaging (including stale-token no-marker-hint), retry-on-launch-race behavior, and 401 refresh retry using TokenStore.

src/Capacitor.Cli/Commands/McpFlowResultServer.cs

McpReviewServer.csRegister SubmitReviewerResultDto for source-generated JSON serialization +1/-0

Register SubmitReviewerResultDto for source-generated JSON serialization

• Extends 'McpJsonContext' with 'SubmitReviewerResultDto' to ensure NativeAOT-safe serialization for the new flow-result POST body.

src/Capacitor.Cli/Commands/McpReviewServer.cs

Program.csExpose 'kcap mcp flow-result' subcommand in CLI routing and usage +4/-1

Expose 'kcap mcp flow-result' subcommand in CLI routing and usage

• Updates help text to include 'flow-result' and adds a new switch case to route 'kcap mcp flow-result' to 'McpFlowResultServer.RunAsync'.

src/Capacitor.Cli/Program.cs

Tests (3) +227 / -7
ClaudeLauncherReviewFlowTests.csAssert Claude review-flow MCP config contains exactly kcap-flow-result +20/-7

Assert Claude review-flow MCP config contains exactly kcap-flow-result

• Updates launcher construction to accept ServerUrl/CapacitorPath and adds coverage for both the empty-server fallback and the single whitelisted server configuration.

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

CodexLauncherTests.csPin clear-before-whitelist ordering and zero-server fallback for Codex +69/-0

Pin clear-before-whitelist ordering and zero-server fallback for Codex

• Adds tests asserting the 'mcp_servers={}' reset precedes all dotted overrides and that only kcap-flow-result is whitelisted. Includes a fallback test ensuring no dotted overrides are emitted when ServerUrl is missing.

test/Capacitor.Cli.Tests.Unit/Codex/CodexLauncherTests.cs

McpFlowResultServerTests.csAdd unit tests for flow-result submission semantics and error policy +138/-0

Add unit tests for flow-result submission semantics and error policy

• Covers snake_case wire body, clean-kind null omission, retry-then-succeed, retry exhaustion messaging, stale-round-token no-fallback-hint behavior, validation without network calls, and missing-agent-env exit code.

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

Documentation (1) +610 / -0
2026-07-02-ai1139-flow-result-cli.mdAdd AI-1139 CLI implementation plan for flow-result + launcher injection +610/-0

Add AI-1139 CLI implementation plan for flow-result + launcher injection

• Introduces a detailed step-by-step plan covering the new MCP server, launcher injection strategy, AOT constraints, and required unit test coverage.

docs/superpowers/plans/2026-07-02-ai1139-flow-result-cli.md

@qodo-code-review

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

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Action required

1. flow-result not documented README ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The PR adds a new user-facing CLI subcommand kcap mcp flow-result, but README.md is not updated to
document it. This can leave users unaware of the new command and its intended daemon-launched usage.
Code

src/Capacitor.Cli/Program.cs[R333-334]

+            case "flow-result":
+                return await McpFlowResultServer.RunAsync(baseUrl!);
Evidence
Rule 3 requires README.md updates for user-facing CLI changes. The PR adds the flow-result
subcommand routing in Program.cs, while README.md sections describing MCP servers mention `kcap
mcp sessions and kcap mcp flows but not kcap mcp flow-result`.

CLAUDE.md: README.md Must Be Updated in the Same PR for Any User-Facing CLI Surface Change
src/Capacitor.Cli/Program.cs[292-334]
README.md[120-125]
README.md[277-300]

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

## Issue description
This PR introduces a new user-facing CLI surface (`kcap mcp flow-result`) but README.md does not document it.

## Issue Context
Compliance requires README.md updates in the same PR for any user-facing CLI surface change. The CLI now routes a `flow-result` MCP subcommand, and the README currently documents `kcap mcp sessions` and `kcap mcp flows` but not `flow-result`.

## Fix Focus Areas
- README.md[120-125]
- README.md[277-320]
- src/Capacitor.Cli/Program.cs[292-335]

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


2. Unguarded params parsing ✓ Resolved 🐞 Bug ☼ Reliability
Description
McpFlowResultServer.DispatchToolCallAsync parses params/name/arguments via
AsObject()/GetValue<string>() before entering its try/catch, so a malformed tools/call request can
throw and terminate the stdio server loop. This breaks the “guarded tool dispatch” invariant and can
kill the reviewer’s only result-submission tool for the session.
Code

src/Capacitor.Cli/Commands/McpFlowResultServer.cs[R63-65]

+            var paramsNode = callRequest["params"]?.AsObject();
+            var toolName   = paramsNode?["name"]?.GetValue<string>();
+            var arguments  = paramsNode?["arguments"]?.AsObject();
Evidence
In McpFlowResultServer, callRequest["params"]?.AsObject() and GetValue<string>() are executed
before the try/catch, so type mismatches can throw outside the handler and escape the loop.
McpReviewServer instead wraps the dispatch in a try/catch and delegates parsing to a method invoked
inside that guarded section, preventing malformed requests from crashing the server.

src/Capacitor.Cli/Commands/McpFlowResultServer.cs[59-90]
src/Capacitor.Cli/Commands/McpReviewServer.cs[53-65]
src/Capacitor.Cli/Commands/McpReviewServer.cs[139-149]

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` extracts `params`, `name`, and `arguments` using `AsObject()` / `GetValue<string>()` *before* the `try/catch`. If the MCP host sends malformed JSON-RPC (e.g., `params` is not an object, or `name` is not a string), these calls can throw and crash the entire stdio MCP server process.

## Issue Context
Other MCP servers in this repo (e.g., `McpReviewServer`) ensure request parsing happens inside the guarded `try/catch` so malformed requests return a JSON-RPC error/tool error instead of terminating the loop.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/McpFlowResultServer.cs[59-90]

## Implementation notes
- Move `paramsNode/toolName/arguments` extraction inside the existing `try` block, or wrap just that extraction in its own `try/catch`.
- On parse/type errors, return a JSON-RPC invalid params error (e.g., `BuildErrorResponse(callId, -32602, "Invalid params")`) or a tool-result error, but do not throw.
- Keep behavior consistent with `McpReviewServer`/`McpFlowsServer` so the loop stays alive on bad input.

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


Grey Divider

Qodo Logo

Comment thread src/Capacitor.Cli/Program.cs
Comment thread src/Capacitor.Cli/Commands/McpFlowResultServer.cs Outdated
…EADME (Qodo review on #240)

Malformed tools/call params (non-object params, non-string name) now yield
a JSON-RPC -32602 error instead of throwing past the stdio loop and killing
the reviewer's only result-submission tool. README documents the new
daemon-launched server per the user-facing-CLI-surface rule.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@alexeyzimarev
alexeyzimarev merged commit 9ed0b6c into main Jul 3, 2026
5 checks passed
@alexeyzimarev
alexeyzimarev deleted the alexeyzimarev/ai-1139-flow-result-mcp branch July 3, 2026 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant