diff --git a/README.md b/README.md index c36661620..d4af5faad 100644 --- a/README.md +++ b/README.md @@ -326,7 +326,7 @@ Requires `kcap login` **and a running daemon with this repo checked out** — th kcap mcp flow-result # launched by the daemon — not meant to be run manually ``` -Stdio MCP server the **daemon injects into hosted review-flow reviewer sessions** (both Claude and Codex reviewers) as their only MCP server. It exposes a single tool, **`submit_review_result`** (`round_token`, `kind: "findings" | "clean"`, `findings`), which posts the reviewer's result to the Capacitor server — the reliable alternative to the transcript-marker fallback. It is deliberately separate from `kcap mcp flows` so an unattended reviewer can never start a nested flow, and it reads its identity from daemon-provided environment (`KCAP_FLOW_AGENT_ID`); run manually it just exits with an explanation. There is nothing to register or configure. +Stdio MCP server the **daemon injects into hosted review-flow reviewer sessions** (both Claude and Codex reviewers). It exposes a single tool, **`submit_review_result`** (`round_token`, `kind: "findings" | "clean"`, `findings`), which posts the reviewer's result to the Capacitor server — the reliable alternative to the transcript-marker fallback. It is deliberately separate from `kcap mcp flows` so an unattended reviewer can never start a nested flow, and it reads its identity from daemon-provided environment (`KCAP_FLOW_AGENT_ID`); run manually it just exits with an explanation. It's not necessarily the only server a reviewer gets, though: the flow definition's `mcp:` allowlist can additionally grant kcap-owned context servers (e.g. `kcap-sessions`), resolved against the same built-in registry — unknown names are skipped and any flow-starting server is always stripped regardless of listing, so a reviewer still can't start a nested flow. There is nothing to register or configure. ### Memory MCP server (for agents) diff --git a/src/Capacitor.Cli.Core/KcapMcpRegistry.cs b/src/Capacitor.Cli.Core/KcapMcpRegistry.cs new file mode 100644 index 000000000..08cffb235 --- /dev/null +++ b/src/Capacitor.Cli.Core/KcapMcpRegistry.cs @@ -0,0 +1,27 @@ +namespace Capacitor.Cli.Core; + +/// AI-1126 D-c: the kcap-owned MCP server registry — the ONLY source a flow +/// definition's mcp: allowlist resolves against (never user config). StartsFlows marks +/// servers that can start flows; they are stripped from every allowlist regardless of +/// listing (the recursion guard — the server strips too, this is the authoritative layer). +/// Ids are canonical lower-case; Resolve is case-insensitive so casing can't dodge the strip. +public sealed record KcapMcpServerDescriptor(string Id, string[] Args, bool StartsFlows); + +public static class KcapMcpRegistry { + static readonly Dictionary Entries = new(StringComparer.OrdinalIgnoreCase) { + ["kcap-review"] = new("kcap-review", ["mcp", "review"], false), + ["kcap-sessions"] = new("kcap-sessions", ["mcp", "sessions"], false), + ["kcap-memory"] = new("kcap-memory", ["mcp", "memory"], false), + ["kcap-flows"] = new("kcap-flows", ["mcp", "flows"], true), + }; + + /// Resolves an allowlist entry to its descriptor. Case-insensitive, trims + /// surrounding whitespace. A null or blank name — e.g. a wire-deserialized allowlist + /// element — returns null rather than throwing, so callers can route it through the + /// same unknown-name skip path as any other unresolvable name. + public static KcapMcpServerDescriptor? Resolve(string? name) { + if (string.IsNullOrWhiteSpace(name)) return null; + + return Entries.TryGetValue(name.Trim(), out var d) ? d : null; + } +} diff --git a/src/Capacitor.Cli.Core/Models.cs b/src/Capacitor.Cli.Core/Models.cs index 1a7312575..ea3c1bef8 100644 --- a/src/Capacitor.Cli.Core/Models.cs +++ b/src/Capacitor.Cli.Core/Models.cs @@ -814,7 +814,12 @@ public readonly record struct LaunchAgentCommand( // a mismatch (e.g. a different machine, where the path doesn't resolve) skips the sync. // Appended last as an optional field so the SignalR positional binding stays wire-compatible // with older daemons/servers. - string? SyncFromRepoRoot = null + string? SyncFromRepoRoot = null, + // AI-1126 D-c: for a review-flow launch, the flow definition's MCP allowlist — server-owned + // names the daemon resolves against the kcap-owned KcapMcpRegistry and materializes into the + // launcher's MCP config (flow-starting servers are stripped regardless of listing). Appended + // last, same wire-compat rule as SyncFromRepoRoot above. + string[]? McpAllowlist = null ); /// diff --git a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs index 7f2ee6a36..bd8ba1d88 100644 --- a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs +++ b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs @@ -359,7 +359,9 @@ async Task HandleLaunchAgent(LaunchAgentCommand cmd) { ReviewLaunch: isReview && cmd.Review is { } reviewArgs ? await ReviewLaunchBuilder.BuildAsync(cmd.Vendor, _config.CapacitorPath, _config.ServerUrl ?? "", reviewArgs.Owner, reviewArgs.Repo, reviewArgs.PrNumber) : null - ); + ) { + McpAllowlist = cmd.McpAllowlist + }; try { launcher.Prepare(launcherCtx); diff --git a/src/Capacitor.Cli.Daemon/Services/ClaudeLauncher.cs b/src/Capacitor.Cli.Daemon/Services/ClaudeLauncher.cs index 08f57a0d3..220a54294 100644 --- a/src/Capacitor.Cli.Daemon/Services/ClaudeLauncher.cs +++ b/src/Capacitor.Cli.Daemon/Services/ClaudeLauncher.cs @@ -130,7 +130,13 @@ public LaunchArgs BuildArgs(LauncherContext ctx) { /// kcap-flow-result submission server, or the empty map when the daemon has no server /// URL / kcap path (zero servers is the recursion-safe default). Built via JsonNode /// string casts — JsonValue.Create / collection expressions lower to generic Add<T> - /// and trip NativeAOT (IL3050). + /// and trip NativeAOT (IL3050). + /// AI-1126 D-c: on the non-empty path, also materializes the flow definition's + /// into the same mcpServers object — each name + /// resolved against the kcap-owned (never ambient user + /// config), unknown names skipped, flow-starting servers stripped regardless of listing. + /// Allowlist servers get KCAP_URL only — never KCAP_FLOW_AGENT_ID, which is exclusive to + /// the flow-result submission channel. string BuildReviewFlowMcpConfig(LauncherContext ctx) { if (string.IsNullOrWhiteSpace(config.ServerUrl) || string.IsNullOrWhiteSpace(config.CapacitorPath)) return EmptyMcpConfig; @@ -147,7 +153,36 @@ string BuildReviewFlowMcpConfig(LauncherContext ctx) { } }; - return new JsonObject { ["mcpServers"] = new JsonObject { ["kcap-flow-result"] = server } }.ToJsonString(); + var mcpServers = new JsonObject { ["kcap-flow-result"] = server }; + + foreach (var name in ctx.McpAllowlist ?? []) { + var descriptor = KcapMcpRegistry.Resolve(name); + + if (descriptor is null) { + // name may be null here — a wire-deserialized allowlist element — so log it + // defensively rather than let a null flow into the formatter unlabeled. + LogAllowlistEntryUnknown(name ?? "(null)", 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 + } + }; + } + + return new JsonObject { ["mcpServers"] = mcpServers }.ToJsonString(); } /// Claude needs no mandatory daemon-level flags (cwd is set via forkpty chdir), so a @@ -450,4 +485,10 @@ static void WriteMcpConfig(string sourceRepoPath, string worktreePath) { [LoggerMessage(Level = LogLevel.Warning, Message = "Failed to remove mcp config for agent {AgentId}")] partial void LogCleanupMcpConfigFailed(Exception ex, string agentId); + + [LoggerMessage(Level = LogLevel.Warning, Message = "MCP allowlist entry '{Name}' is not a kcap-owned server — skipping (agent {AgentId})")] + partial void LogAllowlistEntryUnknown(string name, string agentId); + + [LoggerMessage(Level = LogLevel.Warning, Message = "MCP allowlist entry '{Name}' can start flows — stripped (agent {AgentId})")] + partial void LogAllowlistEntryStripped(string name, string agentId); } diff --git a/src/Capacitor.Cli.Daemon/Services/CodexLauncher.cs b/src/Capacitor.Cli.Daemon/Services/CodexLauncher.cs index e05d2209d..7df53eb3e 100644 --- a/src/Capacitor.Cli.Daemon/Services/CodexLauncher.cs +++ b/src/Capacitor.Cli.Daemon/Services/CodexLauncher.cs @@ -93,6 +93,7 @@ public LaunchArgs BuildArgs(LauncherContext ctx) { args.Add("-c"); args.Add("mcp_servers={}"); AddFlowResultServer(args, ctx); + AddAllowlistServers(args, ctx); } AddModelArg(args, ctx.Model); @@ -131,6 +132,44 @@ void AddFlowResultServer(List args, LauncherContext ctx) { args.Add($"mcp_servers.{name}.env={{KCAP_URL={TomlString(config.ServerUrl)},KCAP_FLOW_AGENT_ID={TomlString(ctx.AgentId)}}}"); } + /// AI-1126 D-c: materializes the flow definition's + /// as additional dotted overrides, in the same clear-then-whitelist style as + /// . Each name resolves against the kcap-owned + /// — never ambient user config — unknown names are skipped, + /// and any flow-starting server is stripped regardless of listing (the recursion guard). + /// Allowlist servers get KCAP_URL only — never KCAP_FLOW_AGENT_ID, which is exclusive to + /// the flow-result submission channel. Skipped (same as the flow-result server) when the + /// daemon has no server URL or kcap path configured. + void AddAllowlistServers(List args, LauncherContext ctx) { + if (string.IsNullOrWhiteSpace(config.ServerUrl) || string.IsNullOrWhiteSpace(config.CapacitorPath)) return; + + foreach (var name in ctx.McpAllowlist ?? []) { + var descriptor = KcapMcpRegistry.Resolve(name); + + if (descriptor is null) { + // name may be null here — a wire-deserialized allowlist element — so log it + // defensively rather than let a null flow into the formatter unlabeled. + LogAllowlistEntryUnknown(name ?? "(null)", ctx.AgentId); + continue; + } + + if (descriptor.StartsFlows) { + LogAllowlistEntryStripped(name, ctx.AgentId); + continue; + } + + var id = descriptor.Id; + var argsList = string.Join(",", descriptor.Args.Select(TomlString)); + + args.Add("-c"); + args.Add($"mcp_servers.{id}.command={TomlString(config.CapacitorPath)}"); + args.Add("-c"); + args.Add($"mcp_servers.{id}.args=[{argsList}]"); + args.Add("-c"); + args.Add($"mcp_servers.{id}.env={{KCAP_URL={TomlString(config.ServerUrl)}}}"); + } + } + /// Append `-m ` unless the model is empty or the "default" no-override sentinel. /// "default" is the sentinel from the flow/agent dispatch; passing it as `-m default` is /// rejected by Codex on a ChatGPT account ("The 'default' model is not supported when using @@ -263,4 +302,10 @@ static bool HooksInstalledIn(string hooksPath) { [LoggerMessage(Level = LogLevel.Debug, Message = "Tools array of length {Count} ignored for vendor=codex (no allowlist concept) — agent {AgentId}")] partial void LogToolsIgnoredForCodex(string agentId, int count); + + [LoggerMessage(Level = LogLevel.Warning, Message = "MCP allowlist entry '{Name}' is not a kcap-owned server — skipping (agent {AgentId})")] + partial void LogAllowlistEntryUnknown(string name, string agentId); + + [LoggerMessage(Level = LogLevel.Warning, Message = "MCP allowlist entry '{Name}' can start flows — stripped (agent {AgentId})")] + partial void LogAllowlistEntryStripped(string name, string agentId); } diff --git a/src/Capacitor.Cli.Daemon/Services/IHostedAgentLauncher.cs b/src/Capacitor.Cli.Daemon/Services/IHostedAgentLauncher.cs index 61720b421..2b422ea4b 100644 --- a/src/Capacitor.Cli.Daemon/Services/IHostedAgentLauncher.cs +++ b/src/Capacitor.Cli.Daemon/Services/IHostedAgentLauncher.cs @@ -81,6 +81,13 @@ internal sealed record LauncherContext( /// Owned worktree (daemon-created) vs borrowed cwd (the user's own checkout). /// Borrowed-cwd launches skip repo-mutating Prepare() steps. public WorkLocation Work { get; init; } = WorkLocation.OwnedWorktree; + + /// AI-1126 D-c: the review-flow definition's MCP allowlist, carried verbatim from + /// . Launchers resolve each + /// name against the kcap-owned MCP registry and materialize matching servers into the vendor's + /// MCP config, stripping any flow-starting server regardless of listing. Null/local-spawn + /// launches (e.g. kcap run-agent) never set this. + public string[]? McpAllowlist { get; init; } } internal readonly record struct LaunchArgs(string[] Args, string? McpConfigPath); diff --git a/test/Capacitor.Cli.Tests.Unit/ClaudeLauncherReviewFlowTests.cs b/test/Capacitor.Cli.Tests.Unit/ClaudeLauncherReviewFlowTests.cs index 1afc1b456..cd75f59a1 100644 --- a/test/Capacitor.Cli.Tests.Unit/ClaudeLauncherReviewFlowTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/ClaudeLauncherReviewFlowTests.cs @@ -3,6 +3,7 @@ using Capacitor.Cli.Daemon; using Capacitor.Cli.Daemon.Services; using Microsoft.Extensions.Logging.Abstractions; +using TUnit.Assertions.Enums; namespace Capacitor.Cli.Tests.Unit; @@ -96,4 +97,108 @@ public async Task Non_review_flow_launch_is_unchanged_no_bypass_or_strict_mcp() public async Task Claude_launcher_supports_unattended() { await Assert.That(NewLauncher().SupportsUnattended).IsTrue(); } + + // === AI-1126 D-c: definition MCP allowlist materialization === + + [Test] + public async Task ReviewFlow_with_allowlist_merges_servers_into_mcp_config() { + var ctx = NewCtx(isReviewFlow: true) with { McpAllowlist = ["kcap-sessions"] }; + var args = NewLauncher(serverUrl: "https://t.example", capacitorPath: "/opt/kcap").BuildArgs(ctx).Args; + + await Assert.That(args).Contains("--strict-mcp-config"); + await Assert.That(args).Contains("--disallowedTools"); + var dIdx = Array.IndexOf(args, "--disallowedTools"); + await Assert.That(args[dIdx + 1]).IsEqualTo("Agent"); + + var cfgIndex = Array.IndexOf(args, "--mcp-config"); + var servers = JsonNode.Parse(args[cfgIndex + 1])!.AsObject()["mcpServers"]!.AsObject(); + + await Assert.That(servers.Count).IsEqualTo(2); + + var flowResult = servers["kcap-flow-result"]!.AsObject(); + await Assert.That(flowResult["env"]!["KCAP_FLOW_AGENT_ID"]!.GetValue()).IsEqualTo("a-1"); + + var sessions = servers["kcap-sessions"]!.AsObject(); + await Assert.That(sessions["command"]!.GetValue()).IsEqualTo("/opt/kcap"); + await Assert.That(sessions["args"]![0]!.GetValue()).IsEqualTo("mcp"); + await Assert.That(sessions["args"]![1]!.GetValue()).IsEqualTo("sessions"); + await Assert.That(sessions["env"]!["KCAP_URL"]!.GetValue()).IsEqualTo("https://t.example"); + await Assert.That(sessions["env"]!.AsObject().ContainsKey("KCAP_FLOW_AGENT_ID")).IsFalse(); + } + + [Test] + public async Task ReviewFlow_allowlist_strips_flow_starting_server_any_case() { + var ctx = NewCtx(isReviewFlow: true) with { McpAllowlist = ["KCAP-Flows", "kcap-sessions"] }; + var args = NewLauncher(serverUrl: "https://t.example", capacitorPath: "/opt/kcap").BuildArgs(ctx).Args; + + var cfgIndex = Array.IndexOf(args, "--mcp-config"); + var servers = JsonNode.Parse(args[cfgIndex + 1])!.AsObject()["mcpServers"]!.AsObject(); + + await Assert.That(servers.Count).IsEqualTo(2); + await Assert.That(servers.ContainsKey("kcap-flow-result")).IsTrue(); + await Assert.That(servers.ContainsKey("kcap-sessions")).IsTrue(); + await Assert.That(servers.ContainsKey("kcap-flows")).IsFalse(); + } + + [Test] + public async Task ReviewFlow_allowlist_skips_unknown_names() { + var ctxWithAllowlist = NewCtx(isReviewFlow: true) with { McpAllowlist = ["not-a-server"] }; + var ctxWithout = NewCtx(isReviewFlow: true); + var launcher = NewLauncher(serverUrl: "https://t.example", capacitorPath: "/opt/kcap"); + + var argsWithAllowlist = launcher.BuildArgs(ctxWithAllowlist).Args; + var argsWithout = launcher.BuildArgs(ctxWithout).Args; + + var cfgIndexWith = Array.IndexOf(argsWithAllowlist, "--mcp-config"); + var cfgIndexWithout = Array.IndexOf(argsWithout, "--mcp-config"); + + await Assert.That(argsWithAllowlist[cfgIndexWith + 1]).IsEqualTo(argsWithout[cfgIndexWithout + 1]); + } + + [Test] + public async Task ReviewFlow_without_allowlist_args_byte_identical_to_today() { + var args = NewLauncher(serverUrl: "https://t.example", capacitorPath: "/opt/kcap") + .BuildArgs(NewCtx(isReviewFlow: true)).Args; + + const string expectedMcpConfig = + """{"mcpServers":{"kcap-flow-result":{"command":"/opt/kcap","args":["mcp","flow-result"],"env":{"KCAP_URL":"https://t.example","KCAP_FLOW_AGENT_ID":"a-1"}}}}"""; + + string[] expected = [ + "--permission-mode", "bypassPermissions", + "--strict-mcp-config", + "--mcp-config", expectedMcpConfig, + "--disallowedTools", "Agent", + "--model", "sonnet", + "--", "review this" + ]; + + await Assert.That(args).IsEquivalentTo(expected, CollectionOrdering.Matching); + } + + [Test] + public async Task ReviewFlow_allowlist_with_null_element_skips_it_without_throwing() { + // A wire-deserialized allowlist can contain a null element (e.g. a malformed JSON + // array entry). It must flow through the existing unknown-name skip path rather + // than throw a NullReferenceException out of the launch path. + var ctx = NewCtx(isReviewFlow: true) with { McpAllowlist = [null!, "kcap-sessions"] }; + + var args = NewLauncher(serverUrl: "https://t.example", capacitorPath: "/opt/kcap").BuildArgs(ctx).Args; + + var cfgIndex = Array.IndexOf(args, "--mcp-config"); + var servers = JsonNode.Parse(args[cfgIndex + 1])!.AsObject()["mcpServers"]!.AsObject(); + + await Assert.That(servers.Count).IsEqualTo(2); + await Assert.That(servers.ContainsKey("kcap-flow-result")).IsTrue(); + await Assert.That(servers.ContainsKey("kcap-sessions")).IsTrue(); + } + + [Test] + public async Task ReviewFlow_empty_allowlist_args_byte_identical_to_null_allowlist() { + var launcher = NewLauncher(serverUrl: "https://t.example", capacitorPath: "/opt/kcap"); + + var argsNull = launcher.BuildArgs(NewCtx(isReviewFlow: true) with { McpAllowlist = null }).Args; + var argsEmpty = launcher.BuildArgs(NewCtx(isReviewFlow: true) with { McpAllowlist = [] }).Args; + + await Assert.That(argsEmpty).IsEquivalentTo(argsNull, CollectionOrdering.Matching); + } } diff --git a/test/Capacitor.Cli.Tests.Unit/Codex/CodexLauncherTests.cs b/test/Capacitor.Cli.Tests.Unit/Codex/CodexLauncherTests.cs index d6c0ad2b8..e579af021 100644 --- a/test/Capacitor.Cli.Tests.Unit/Codex/CodexLauncherTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/Codex/CodexLauncherTests.cs @@ -3,6 +3,7 @@ using Capacitor.Cli.Daemon; using Capacitor.Cli.Daemon.Services; using Microsoft.Extensions.Logging.Abstractions; +using TUnit.Assertions.Enums; namespace Capacitor.Cli.Tests.Unit.Codex; @@ -454,4 +455,99 @@ public async Task BuildArgs_review_toml_escapes_control_chars_in_command_args_an public async Task Codex_launcher_supports_unattended() { await Assert.That(NewLauncher().SupportsUnattended).IsTrue(); } + + // === AI-1126 D-c: definition MCP allowlist materialization === + + static CodexLauncher NewFlowResultLauncher() => + new(new DaemonConfig { CodexPath = "codex", CapacitorPath = "/opt/kcap", ServerUrl = "https://t.example" }, NullLogger.Instance); + + static LauncherContext NewFlowCtx(string[]? allowlist) => new LauncherContext( + AgentId: "agent-xyz", + SourceRepoPath: "/tmp/repo", + Worktree: new WorktreeInfo(Path: "/tmp/wt", Branch: "wt-branch", SourceRepo: "/tmp/repo"), + Prompt: null, + Model: "gpt-5.3-codex", + Effort: null, + Tools: null, + IsReview: false, + IsReviewFlow: true, + Review: null, + ReviewLaunch: null + ) { McpAllowlist = allowlist }; + + [Test] + public async Task ReviewFlow_with_allowlist_merges_servers_into_dotted_overrides() { + var args = NewFlowResultLauncher().BuildArgs(NewFlowCtx(["kcap-sessions"])).Args; + + var clearIdx = Array.IndexOf(args, "mcp_servers={}"); + await Assert.That(clearIdx).IsGreaterThanOrEqualTo(0); + + var dotted = args.Where(a => a.StartsWith("mcp_servers.", StringComparison.Ordinal)).ToArray(); + await Assert.That(dotted.Any(a => a.StartsWith("mcp_servers.kcap-flow-result.", StringComparison.Ordinal))).IsTrue(); + + var sessionsDotted = dotted.Where(a => a.StartsWith("mcp_servers.kcap-sessions.", StringComparison.Ordinal)).ToArray(); + await Assert.That(sessionsDotted.Length).IsEqualTo(3); + + foreach (var d in sessionsDotted) { + await Assert.That(Array.IndexOf(args, d)).IsGreaterThan(clearIdx); + } + + var command = sessionsDotted.Single(a => a.Contains(".command=")); + await Assert.That(command).Contains("/opt/kcap"); + var argsOverride = sessionsDotted.Single(a => a.Contains(".args=")); + await Assert.That(argsOverride).Contains("mcp"); + await Assert.That(argsOverride).Contains("sessions"); + var env = sessionsDotted.Single(a => a.Contains(".env=")); + await Assert.That(env).Contains("KCAP_URL"); + await Assert.That(env).Contains("https://t.example"); + await Assert.That(env).DoesNotContain("KCAP_FLOW_AGENT_ID"); + } + + [Test] + public async Task ReviewFlow_allowlist_strips_flow_starting_server_any_case() { + var args = NewFlowResultLauncher().BuildArgs(NewFlowCtx(["KCAP-Flows", "kcap-sessions"])).Args; + + var dotted = args.Where(a => a.StartsWith("mcp_servers.", StringComparison.Ordinal)).ToArray(); + await Assert.That(dotted.Any(a => a.StartsWith("mcp_servers.kcap-flows.", StringComparison.Ordinal))).IsFalse(); + await Assert.That(dotted.Any(a => a.StartsWith("mcp_servers.kcap-sessions.", StringComparison.Ordinal))).IsTrue(); + } + + [Test] + public async Task ReviewFlow_allowlist_skips_unknown_names() { + var launcher = NewFlowResultLauncher(); + + var argsWith = launcher.BuildArgs(NewFlowCtx(["not-a-server"])).Args; + var argsWithout = launcher.BuildArgs(NewFlowCtx(null)).Args; + + await Assert.That(argsWith).IsEquivalentTo(argsWithout, CollectionOrdering.Matching); + } + + [Test] + public async Task ReviewFlow_without_allowlist_args_byte_identical_to_today() { + var args = NewFlowResultLauncher().BuildArgs(NewFlowCtx(null)).Args; + + string[] expected = [ + "--cd", "/tmp/wt", + "--sandbox", "workspace-write", + "--ask-for-approval", "never", + "-c", "mcp_servers={}", + "-c", "mcp_servers.kcap-flow-result.command=\"/opt/kcap\"", + "-c", "mcp_servers.kcap-flow-result.args=[\"mcp\",\"flow-result\"]", + "-c", "mcp_servers.kcap-flow-result.env={KCAP_URL=\"https://t.example\",KCAP_FLOW_AGENT_ID=\"agent-xyz\"}", + "-m", "gpt-5.3-codex", + "--no-alt-screen" + ]; + + await Assert.That(args).IsEquivalentTo(expected, CollectionOrdering.Matching); + } + + [Test] + public async Task ReviewFlow_empty_allowlist_args_byte_identical_to_null_allowlist() { + var launcher = NewFlowResultLauncher(); + + var argsNull = launcher.BuildArgs(NewFlowCtx(null)).Args; + var argsEmpty = launcher.BuildArgs(NewFlowCtx([])).Args; + + await Assert.That(argsEmpty).IsEquivalentTo(argsNull, CollectionOrdering.Matching); + } } diff --git a/test/Capacitor.Cli.Tests.Unit/KcapMcpRegistryTests.cs b/test/Capacitor.Cli.Tests.Unit/KcapMcpRegistryTests.cs new file mode 100644 index 000000000..db0869b51 --- /dev/null +++ b/test/Capacitor.Cli.Tests.Unit/KcapMcpRegistryTests.cs @@ -0,0 +1,177 @@ +using Capacitor.Cli.Core; + +namespace Capacitor.Cli.Tests.Unit; + +public class KcapMcpRegistryTests { + [Test] + public async Task Resolve_KnownId_ReturnsDescriptor() { + var result = KcapMcpRegistry.Resolve("kcap-review"); + + await Assert.That(result).IsNotNull(); + } + + [Test] + public async Task Resolve_KnownId_CanonicalLowercaseId() { + var result = KcapMcpRegistry.Resolve("kcap-review"); + + await Assert.That(result!.Id).IsEqualTo("kcap-review"); + } + + [Test] + public async Task Resolve_KnownId_CorrectArgs() { + var result = KcapMcpRegistry.Resolve("kcap-review"); + + await Assert.That(result!.Args).IsEquivalentTo(new[] { "mcp", "review" }); + } + + [Test] + public async Task Resolve_KcapReview_StartsFlowsFalse() { + var result = KcapMcpRegistry.Resolve("kcap-review"); + + await Assert.That(result!.StartsFlows).IsFalse(); + } + + [Test] + public async Task Resolve_KcapSessions_ReturnsDescriptor() { + var result = KcapMcpRegistry.Resolve("kcap-sessions"); + + await Assert.That(result).IsNotNull(); + } + + [Test] + public async Task Resolve_KcapSessions_CorrectArgs() { + var result = KcapMcpRegistry.Resolve("kcap-sessions"); + + await Assert.That(result!.Args).IsEquivalentTo(new[] { "mcp", "sessions" }); + } + + [Test] + public async Task Resolve_KcapSessions_StartsFlowsFalse() { + var result = KcapMcpRegistry.Resolve("kcap-sessions"); + + await Assert.That(result!.StartsFlows).IsFalse(); + } + + [Test] + public async Task Resolve_KcapMemory_ReturnsDescriptor() { + var result = KcapMcpRegistry.Resolve("kcap-memory"); + + await Assert.That(result).IsNotNull(); + } + + [Test] + public async Task Resolve_KcapMemory_CorrectArgs() { + var result = KcapMcpRegistry.Resolve("kcap-memory"); + + await Assert.That(result!.Args).IsEquivalentTo(new[] { "mcp", "memory" }); + } + + [Test] + public async Task Resolve_KcapMemory_StartsFlowsFalse() { + var result = KcapMcpRegistry.Resolve("kcap-memory"); + + await Assert.That(result!.StartsFlows).IsFalse(); + } + + [Test] + public async Task Resolve_KcapFlows_ReturnsDescriptor() { + var result = KcapMcpRegistry.Resolve("kcap-flows"); + + await Assert.That(result).IsNotNull(); + } + + [Test] + public async Task Resolve_KcapFlows_CorrectArgs() { + var result = KcapMcpRegistry.Resolve("kcap-flows"); + + await Assert.That(result!.Args).IsEquivalentTo(new[] { "mcp", "flows" }); + } + + [Test] + public async Task Resolve_KcapFlows_StartsFlowsTrue() { + var result = KcapMcpRegistry.Resolve("kcap-flows"); + + await Assert.That(result!.StartsFlows).IsTrue(); + } + + [Test] + public async Task Resolve_CaseInsensitive_UpperCase() { + var result = KcapMcpRegistry.Resolve("KCAP-REVIEW"); + + await Assert.That(result).IsNotNull(); + await Assert.That(result!.Id).IsEqualTo("kcap-review"); + } + + [Test] + public async Task Resolve_CaseInsensitive_MixedCase() { + var result = KcapMcpRegistry.Resolve("KcAp-ReViEw"); + + await Assert.That(result).IsNotNull(); + await Assert.That(result!.Id).IsEqualTo("kcap-review"); + } + + [Test] + public async Task Resolve_Trimmed_WithLeadingWhitespace() { + var result = KcapMcpRegistry.Resolve(" kcap-review"); + + await Assert.That(result).IsNotNull(); + } + + [Test] + public async Task Resolve_Trimmed_WithTrailingWhitespace() { + var result = KcapMcpRegistry.Resolve("kcap-review "); + + await Assert.That(result).IsNotNull(); + } + + [Test] + public async Task Resolve_Unknown_ReturnsNull() { + var result = KcapMcpRegistry.Resolve("unknown-server"); + + await Assert.That(result).IsNull(); + } + + [Test] + public async Task Resolve_KcapFlowResult_ReturnsNull() { + var result = KcapMcpRegistry.Resolve("kcap-flow-result"); + + await Assert.That(result).IsNull(); + } + + [Test] + public async Task Resolve_KcapFlowResult_InjectionOnly() { + // kcap-flow-result is deliberately NOT in the registry + // It is injection-only, never allowlistable + var result = KcapMcpRegistry.Resolve("kcap-flow-result"); + + await Assert.That(result).IsNull(); + } + + [Test] + public async Task Resolve_EmptyString_ReturnsNull() { + var result = KcapMcpRegistry.Resolve(""); + + await Assert.That(result).IsNull(); + } + + [Test] + public async Task Resolve_Whitespace_ReturnsNull() { + var result = KcapMcpRegistry.Resolve(" "); + + await Assert.That(result).IsNull(); + } + + [Test] + public async Task Resolve_Null_ReturnsNull() { + var result = KcapMcpRegistry.Resolve(null); + + await Assert.That(result).IsNull(); + } + + [Test] + public async Task Resolve_TwoSpaceWhitespace_ReturnsNull() { + var result = KcapMcpRegistry.Resolve(" "); + + await Assert.That(result).IsNull(); + } +} diff --git a/test/Capacitor.Cli.Tests.Unit/LaunchAgentCommandWireFormatTests.cs b/test/Capacitor.Cli.Tests.Unit/LaunchAgentCommandWireFormatTests.cs index b657bb851..47956f363 100644 --- a/test/Capacitor.Cli.Tests.Unit/LaunchAgentCommandWireFormatTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/LaunchAgentCommandWireFormatTests.cs @@ -130,6 +130,73 @@ public async Task DaemonDeserialises_SyncFromRepoRoot_DefaultsToNull_WhenAbsent( await Assert.That(parsed.RepoPath).IsEqualTo("/tmp/repo"); } + [Test] + public async Task Mcp_allowlist_round_trips_snake_case() { + // AI-1126 D-c: the server sends the flow definition's MCP allowlist so the daemon can + // thread it to the launcher (Task 6 materializes it). Appended last after + // SyncFromRepoRoot so older daemons/servers stay wire-compatible. + var cmd = new LaunchAgentCommand( + AgentId: "mcp00001", + Prompt: null, + Model: "opus", + Effort: null, + RepoPath: "/tmp/repo", + Tools: null, + AttachmentIds: null, + Vendor: "codex", + Kind: LaunchKind.ReviewFlow, + McpAllowlist: ["kcap-sessions", "kcap-review"] + ); + + var wire = JsonSerializer.Serialize(cmd, ServerWireOptions); + var parsed = JsonSerializer.Deserialize(wire, CapacitorJsonContext.Default.LaunchAgentCommand); + + await Assert.That(wire).Contains("\"mcp_allowlist\":[\"kcap-sessions\",\"kcap-review\"]"); + await Assert.That(parsed.McpAllowlist).IsEquivalentTo(["kcap-sessions", "kcap-review"]); + } + + [Test] + public async Task Legacy_payload_without_mcp_allowlist_deserializes_null() { + // Version skew: a server predating AI-1126 D-c never sends mcp_allowlist. The daemon + // must still bind the command (positional SignalR binding) and default the field to + // null — i.e. no allowlist materialization — rather than failing to invoke LaunchAgent. + const string legacyWire = + """ + {"agent_id":"legacy02","prompt":null,"model":"opus","effort":null,"repo_path":"/tmp/repo","tools":null,"attachment_ids":null,"vendor":"claude","kind":"reviewFlow"} + """; + + var parsed = JsonSerializer.Deserialize(legacyWire, CapacitorJsonContext.Default.LaunchAgentCommand); + + await Assert.That(parsed.McpAllowlist).IsNull(); + await Assert.That(parsed.Kind).IsEqualTo(LaunchKind.ReviewFlow); + await Assert.That(parsed.RepoPath).IsEqualTo("/tmp/repo"); + } + + [Test] + public async Task Old_reader_ignores_mcp_allowlist() { + // AI-1126 D-c: a new server sends mcp_allowlist to an old daemon that predates this + // task. The old reader must ignore the unknown field and still bind everything else — + // launches must not break just because the server got the new field first. + var cmd = new LaunchAgentCommand( + AgentId: "old0001", + Prompt: null, + Model: "opus", + Effort: null, + RepoPath: "/tmp/repo", + Tools: null, + AttachmentIds: null, + Vendor: "claude", + McpAllowlist: ["kcap-sessions"] + ); + + var wire = JsonSerializer.Serialize(cmd, ServerWireOptions); + var parsed = JsonSerializer.Deserialize(wire, OldWireJsonContext.Default.OldLaunchAgentCommand); + + await Assert.That(wire).Contains("\"mcp_allowlist\":[\"kcap-sessions\"]"); + await Assert.That(parsed.AgentId).IsEqualTo("old0001"); + await Assert.That(parsed.Vendor).IsEqualTo("claude"); + } + [Test] public async Task Vendor_field_round_trips_through_json_serializer() { var cmd = new LaunchAgentCommand( @@ -165,3 +232,39 @@ public async Task AgentRunStarted_vendor_serialises_into_json_body() { await Assert.That(json.ToLowerInvariant()).Contains("\"vendor\""); } } + +/// +/// Frozen snapshot of 's shape from BEFORE AI-1126 D-c added +/// McpAllowlist — used by +/// to prove an old daemon build tolerates the new wire field rather than failing to bind. +/// +readonly record struct OldLaunchAgentCommand( + string AgentId, + string? Prompt, + string Model, + string? Effort, + string RepoPath, + string[]? Tools, + string[]? AttachmentIds, + string Vendor, + LaunchKind Kind = LaunchKind.Default, + ReviewLaunchInfo? Review = null, + string? BaseRef = null, + string? SyncFromRepoRoot = null + ); + +/// +/// Mirrors 's +/// exactly (snake_case properties, camelCase string enums) but over the frozen +/// shape — a real source-generated reader, not a bare +/// reflection-based default, so the test proves what an actual old +/// AOT-compiled daemon binary would do. +/// +[JsonSerializable(typeof(OldLaunchAgentCommand))] +[JsonSerializable(typeof(LaunchKind))] +[JsonSerializable(typeof(ReviewLaunchInfo))] +[JsonSourceGenerationOptions( + PropertyNamingPolicy = JsonKnownNamingPolicy.SnakeCaseLower, + UseStringEnumConverter = true +)] +sealed partial class OldWireJsonContext : JsonSerializerContext;