[AI-1126] Phase D-c (CLI): materialize definition MCP allowlists — kcap-owned registry, capability strip, additive wire field

[alexeyzimarev]

Summary

CLI/daemon half of the final Phase-D slice (spec: kcap-server docs/superpowers/specs/2026-07-02-ai1126-flows-phase-d-design.md; server half: kurrent-io/kcap-server#932 — either merge order is safe):

• Wire: the daemon's LaunchAgentCommand gains string[]? McpAllowlist = null (appended last, snake_case mcp_allowlist). Compat pinned in BOTH directions through the production source-gen options: new reader/old payload → null, and an old-reader test deserializing a new payload into a frozen copy of the pre-change record shape (a real frozen JsonSerializerContext, not reflection defaults).
• KcapMcpRegistry (Cli.Core): the kcap-owned server registry — kcap-review/kcap-sessions/kcap-memory (StartsFlows=false) + kcap-flows (StartsFlows=true); case-insensitive, trimmed, canonical lower-case ids; args pinned against CodexConfigToml + kcap/.mcp.json; kcap-flow-result deliberately absent (injection-only, never allowlistable).
• Launcher materialization: allowlist names resolve through the registry only (never user config). Flow-starting entries stripped by capability (any casing); unknown names skip + Warning with the agent id, never fail the launch. Claude: allowed servers merge into the SAME --mcp-config JSON as the AI-1139 kcap-flow-result injection, under unchanged --strict-mcp-config + --disallowedTools Agent; Codex: dotted -c mcp_servers.<id>.… overrides after the whole-table mcp_servers={} clear. Allowlist servers get KCAP_URL only — KCAP_FLOW_AGENT_ID stays flow-result-exclusive. Same ServerUrl/CapacitorPath gating as flow-result.
• Empty/absent allowlist ⇒ byte-identical launch args to today, pinned by order-sensitive exact-equality regression tests (CollectionOrdering.Matching — a reordering probe fails them, verified).

Test plan

• Wire-format: round-trip + legacy-absent + old-reader-ignores (frozen shape, production options) — 10/10
• KcapMcpRegistryTests 22/22 (args independently verified against both authoritative sources in review)
• Launcher tests: merge-alongside-flow-result, capability strip any-case, unknown-skip, byte-identical baselines — Claude 12/12, Codex 35/35
• Full Unit suite 2166/2166; Integration 74/76 (both failures are known parallel-load environmental flakes — pass in isolation; ServerUrlProbe previously verified reproducing on clean baseline)

Notes for the merger

• A definition's allowlist takes effect only when BOTH halves are deployed; until then it degrades gracefully (old daemon ignores the field → today's flow-result-only config).
• If a second flow-starting server ever ships, add it to the registry with StartsFlows: true AND to the server's FlowMcpServers denylist (kcap-server#932) — the registry is the authoritative layer.

🤖 Generated with Claude Code

[linear-code]

AI-1126

[qodo-code-review]

PR Summary by Qodo

Materialize MCP allowlists for review flows via kcap-owned registry (CLI/daemon)

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add additive LaunchAgentCommand mcp_allowlist field with backward/forward wire compatibility.
• Introduce kcap-owned MCP registry and strip flow-starting servers from allowlists.
• Materialize allowlisted servers into Claude/Codex MCP configs with regression-locked argument
 ordering.

Diagram

graph TD
  S{{"kcap-server"}} -->|"SignalR JSON"| C(("LaunchAgentCommand")) --> D["AgentOrchestrator"] --> L(("LauncherContext"))
  L --> R["KcapMcpRegistry"]
  R --> CL["ClaudeLauncher"]
  R --> CX["CodexLauncher"]

  subgraph Legend
    direction LR
    _ext{{"External"}} ~~~ _model(("Wire model")) ~~~ _svc["Component"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Server sends fully-resolved MCP descriptors (id/args/capabilities)

• ➕ Daemon becomes a pure pass-through with less duplicated logic
• ➕ Registry updates can ship server-first without CLI release pressure
• ➖ Expands wire surface area and increases coupling to descriptor schema
• ➖ Harder to guarantee recursion-guard stripping in multiple places
• ➖ More risk of accidental user-config injection via payload construction

2. Resolve allowlists from user/ambient MCP config instead of a kcap-owned registry

• ➕ No need to maintain a central registry in the CLI
• ➕ Lets users extend allowlists without server/CLI changes
• ➖ Violates the security/ownership boundary (definitions should not depend on ambient config)
• ➖ Makes behavior non-deterministic across machines
• ➖ Harder to enforce flow-start recursion guards reliably

3. Materialize allowlist servers in a shared helper used by both launchers

• ➕ Reduces duplication between Claude/Codex materialization paths
• ➕ Single place to enforce strip/unknown handling and env policy
• ➖ May complicate vendor-specific output formats (JSON vs dotted TOML)
• ➖ Refactor cost may exceed benefit for a small set of servers

Recommendation: Keep the PR’s approach: transmitting allowlist names and resolving them locally via a kcap-owned registry preserves determinism, maintains a tight security boundary (no ambient config), and enables an explicit recursion-guard via StartsFlows stripping. The chosen additive wire field plus explicit forward/backward compatibility tests is the right tradeoff for phased deployment.

Files changed (10) +570 / -4

Enhancement (6) +120 / -4

KcapMcpRegistry.csAdd kcap-owned MCP registry with StartsFlows metadata +20/-0

Add kcap-owned MCP registry with StartsFlows metadata

• Introduces an authoritative, case-insensitive registry mapping allowlist names to canonical ids, command args, and a StartsFlows capability flag. Provides the single source of truth for which kcap MCP servers are allowlistable and which must be stripped.

src/Capacitor.Cli.Core/KcapMcpRegistry.cs

Models.csExtend LaunchAgentCommand wire model with optional McpAllowlist +6/-1

Extend LaunchAgentCommand wire model with optional McpAllowlist

• Adds a nullable string array McpAllowlist to LaunchAgentCommand as the last field to preserve positional SignalR binding compatibility. Documents that allowlist names are server-owned and resolved against KcapMcpRegistry.

src/Capacitor.Cli.Core/Models.cs

AgentOrchestrator.csThread McpAllowlist from LaunchAgentCommand into LauncherContext +3/-1

Thread McpAllowlist from LaunchAgentCommand into LauncherContext

• Populates LauncherContext.McpAllowlist when building the launcher context during agent launch handling. Ensures launchers can materialize review-flow allowlists without reading user config.

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

ClaudeLauncher.csMerge allowlisted MCP servers into Claude review-flow --mcp-config JSON +41/-2

Merge allowlisted MCP servers into Claude review-flow --mcp-config JSON

• Extends review-flow MCP config generation to resolve allowlist entries via KcapMcpRegistry, skip unknown names with warnings, and strip StartsFlows servers regardless of casing. Adds log messages and ensures allowlist servers receive only KCAP_URL (no KCAP_FLOW_AGENT_ID).

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

CodexLauncher.csMaterialize allowlisted MCP servers as Codex dotted TOML overrides +43/-0

Materialize allowlisted MCP servers as Codex dotted TOML overrides

• Adds allowlist materialization after the mcp_servers={} clear using dotted overrides per server (command/args/env). Uses KcapMcpRegistry for resolution, warns on unknown entries, strips StartsFlows servers, and sets only KCAP_URL for allowlist servers.

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

IHostedAgentLauncher.csAdd McpAllowlist to LauncherContext +7/-0

Add McpAllowlist to LauncherContext

• Introduces LauncherContext.McpAllowlist to carry the review-flow definition’s allowlist from the wire command to vendor launchers. Documents resolution and stripping behavior and clarifies it is unset for local-spawn launches.

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

Tests (4) +450 / -0

ClaudeLauncherReviewFlowTests.csAdd Claude review-flow allowlist materialization regression tests +88/-0

Add Claude review-flow allowlist materialization regression tests

• Adds tests verifying allowlist servers merge into the same mcpServers JSON as kcap-flow-result, flow-starting entries are stripped case-insensitively, and unknown names are ignored. Pins byte-identical args when allowlist is null/empty and adds order-sensitive assertions.

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

CodexLauncherTests.csAdd Codex review-flow allowlist dotted-override regression tests +96/-0

Add Codex review-flow allowlist dotted-override regression tests

• Adds tests ensuring allowlist servers are expressed as dotted overrides after the mcp_servers={} clear, flow-starting entries are stripped case-insensitively, and unknown names do not change args. Pins byte-identical args for null vs empty allowlists and the baseline review-flow case.

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

KcapMcpRegistryTests.csAdd unit tests for registry resolution and capability flags +163/-0

Add unit tests for registry resolution and capability flags

• Covers known ids, canonical lower-case ids, correct args, and StartsFlows flags. Verifies case-insensitive + trimmed resolution and confirms kcap-flow-result is intentionally absent from the registry.

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

LaunchAgentCommandWireFormatTests.csLock LaunchAgentCommand McpAllowlist snake_case and skew tolerance +103/-0

Lock LaunchAgentCommand McpAllowlist snake_case and skew tolerance

• Adds tests verifying mcp_allowlist round-trips as snake_case, legacy payloads without the field deserialize to null, and an old reader (frozen pre-change record with source-generated context) ignores the unknown field. This pins compatibility for phased server/daemon rollouts.

test/Capacitor.Cli.Tests.Unit/LaunchAgentCommandWireFormatTests.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. README says flow-result only ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

README.md states hosted review-flow reviewers have kcap-flow-result as their only MCP server,
but the updated launchers now also materialize additional MCP servers from
LauncherContext.McpAllowlist. This makes the public CLI documentation inaccurate for hosted
reviewers when an allowlist is present.

Code

src/Capacitor.Cli.Daemon/Services/ClaudeLauncher.cs[R156-181]

+        var mcpServers = new JsonObject { ["kcap-flow-result"] = server };
+
+        foreach (var name in ctx.McpAllowlist ?? []) {
+            var descriptor = KcapMcpRegistry.Resolve(name);
+
+            if (descriptor is null) {
+                LogAllowlistEntryUnknown(name, ctx.AgentId);
+                continue;
+            }
+
+            if (descriptor.StartsFlows) {
+                LogAllowlistEntryStripped(name, ctx.AgentId);
+                continue;
+            }
+
+            var entryArgs = new JsonArray();
+            foreach (var a in descriptor.Args) entryArgs.Add((JsonNode?)a);
+
+            mcpServers[descriptor.Id] = new JsonObject {
+                ["command"] = (JsonNode?)config.CapacitorPath,
+                ["args"]    = entryArgs,
+                ["env"]     = new JsonObject {
+                    ["KCAP_URL"] = (JsonNode?)config.ServerUrl
+                }
+            };
+        }

Evidence

The README claims hosted reviewers have kcap-flow-result as their only MCP server, but the updated
launcher code iterates ctx.McpAllowlist and adds additional entries to mcpServers, changing the
documented behavior and requiring a README update per the checklist.

CLAUDE.md: Update README.md in the same PR for any user-facing CLI surface change
README.md[323-330]
src/Capacitor.Cli.Daemon/Services/ClaudeLauncher.cs[156-183]

Agent prompt

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

## Issue description
`README.md` documents that hosted review-flow reviewers get `kcap-flow-result` as their only MCP server, but the launcher now adds extra kcap-owned MCP servers when a flow definition provides an allowlist.

## Issue Context
- The README section "Flow-result MCP server (hosted reviewers)" currently claims exclusivity (only MCP server).
- The PR adds allowlist materialization in the daemon launchers (Claude/Codex), so hosted reviewers may have additional MCP servers (e.g., `kcap-sessions`, `kcap-review`, `kcap-memory`) depending on the server-provided flow definition.

## Fix Focus Areas
- README.md[323-330]
- src/Capacitor.Cli.Daemon/Services/ClaudeLauncher.cs[134-183]

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

---

2. Null allowlist NRE ✓ Resolved 🐞 Bug ☼ Reliability

Description

KcapMcpRegistry.Resolve dereferences the input via name.Trim() without guarding null, while both
launchers pass each McpAllowlist element (copied directly from the wire) into Resolve without
element validation. A null entry in McpAllowlist will throw NullReferenceException during launch arg
construction and break the agent launch.

Code

src/Capacitor.Cli.Core/KcapMcpRegistry.cs[R18-19]

+    public static KcapMcpServerDescriptor? Resolve(string name) =>
+        Entries.TryGetValue(name.Trim(), out var d) ? d : null;

Evidence

Resolve() unconditionally calls Trim() on the provided name, and both launchers call Resolve(name)
for each allowlist entry coming from the wire via AgentOrchestrator; therefore a null element in the
allowlist causes a runtime NullReferenceException on the daemon launch path.

src/Capacitor.Cli.Core/KcapMcpRegistry.cs[18-19]
src/Capacitor.Cli.Daemon/Services/ClaudeLauncher.cs[156-163]
src/Capacitor.Cli.Daemon/Services/CodexLauncher.cs[143-152]
src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[348-364]
src/Capacitor.Cli.Core/Models.cs[797-823]

Agent prompt

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

### Issue description
`KcapMcpRegistry.Resolve` calls `name.Trim()` and will throw if `name` is null. Both `ClaudeLauncher` and `CodexLauncher` iterate `LauncherContext.McpAllowlist` (populated directly from `LaunchAgentCommand.McpAllowlist`) and call `Resolve(name)` without validating each element, so a malformed/buggy payload containing a `null` array element can crash the daemon during launch argument building.

### Issue Context
- `LaunchAgentCommand.McpAllowlist` is deserialized from SignalR JSON and is not validated per-element before being used.
- `ctx.McpAllowlist ?? []` only protects against the array itself being null, not null elements within it.

### Fix Focus Areas
- src/Capacitor.Cli.Core/KcapMcpRegistry.cs[18-19]
- src/Capacitor.Cli.Daemon/Services/ClaudeLauncher.cs[156-163]
- src/Capacitor.Cli.Daemon/Services/CodexLauncher.cs[143-152]
- src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[348-364]

### Suggested fix
1. Make `Resolve` accept nullable input and return null for null/blank:
  - `public static KcapMcpServerDescriptor? Resolve(string? name)`
  - `if (string.IsNullOrWhiteSpace(name)) return null;`
  - then `var key = name.Trim(); ...`
2. Optionally (defense-in-depth), also skip null/blank entries in both launchers before calling `Resolve` (and log once if desired).

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

---