Widen the Gemini review MCP gate to exactly the injected server set

[realtonyyoung]

AI-1732 (Linear; the auto-imported GitHub twin #449 is closed — tracking moved to Linear at the maintainer's direction).

The defect

A Gemini review launch replaced the deny-all --allowed-mcp-server-names value with only identity.ResultChannelWireName, while AcpReviewFlowMcp.Build could put more into session/new.mcpServers: every server resolved from the definition's mcp: allowlist. Gemini's gate is measured exclusive exact-match (AI-1413 spec §2.2/§2.6 — it gates injected servers too), so those extra servers were injected and silently blocked: the reviewer starts and can report, but never sees the context servers the definition granted it. Unexercised by the built-in code-review/spec-review definitions (no mcp: list); reachable today by any catalog or dynamic definition with one targeting a Gemini reviewer.

The fix

• Gate == injected set, by construction. The review arm now builds the same AcpReviewFlowMcp.Build list the launch sends in session/new and joins its names into the ONE allowlist value (replace-never-append preserved — the option is comma-coerced, a second occurrence would widen the gate). The whole-vector argv assertion takes the computed gate as an input.
• Aliased, never canonical. Every non-result-channel injected server rides under a per-launch wire name — canonical id + a third independent LaunchIdentity GUID (AllowlistWireName). A canonical id is a fixed public literal the reviewed repository could declare its own .gemini/settings.json server under, and the gate matches names exactly: admitting it would spawn that repo-authored process as the daemon user — the impersonation shape the result channel's alias already closes (§2.3). The borrowed-snapshot kcap-review-context name goes through the same helper (identity for non-aliasing vendors, future-proof for aliasing ones).
• Non-aliasing vendors byte-identical. AllowlistWireName returns the input unchanged for Cursor/Copilot/Kiro identities — pinned by LaunchIdentityTests and the extended Cursor regression test, and CopilotAvailableToolIds/BuildCopilotAdditionalMcpConfig keep seeing canonical names.

Measured before coding (gemini 0.53.0, review argv)

Purpose-built logging stdio servers (the AI-1614 probe harness): with --allowed-mcp-server-names "A,B" as one comma-joined value, both admitted servers spawn and complete initialize → tools/list → tools/call with their nonces reaching the model (A=<nonceA> B=<nonceB>, end_turn), while a third injected server outside the gate never spawns — exclusivity survives multi-name mode — and --approval-mode yolo emits zero interaction frames.

Tests

• GeminiReviewerLaunchTests: one option occurrence with the joined value; gate.Split(',') == injected list names, same order (the parity pin); never-the-canonical-id (verified by mutation: an AllowlistWireName returning the canonical id goes red in 4 tests — this one is the oracle-independent kill); gate-missing assertion arm; Cursor keeps canonical ids.
• LaunchIdentityTests: third-GUID construction rules (exact suffix, independence, pairwise-reuse refusal, comma/whitespace freedom, non-aliasing passthrough).
• Empty-allowlist launches are byte-identical to before (all pre-existing vectors unchanged).
• Local: unit Acp namespace 408 ✓, GeminiReviewerLaunchTests 28 ✓, AcpHostedAgentRuntimeFactoryTests 75 ✓, integration 199 ✓, AOT publish clean.

Note for the merge queue

This touches the same Gemini descriptor comment region as #447 (which records a KNOWN LIMIT for exactly this gap). Whichever merges second will see a trivial comment conflict there — this PR's paragraph is the post-fix truth and supersedes #447's KNOWN LIMIT sentence.

🤖 Generated with Claude Code

[linear-code]

AI-1732

[qodo-code-review]

PR Summary by Qodo

Widen Gemini review MCP gate to exactly the injected server set

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Fixes a security/functional defect where Gemini review launches only allowlisted the result
 channel, silently blocking any extra MCP servers resolved from the definition's allowlist.
• The gate is now built from the same AcpReviewFlowMcp.Build server list injected into
 session/new, comma-joined into a single allowlist value (replace-never-append preserved).
• Every non-result-channel injected server (allowlist servers, kcap-review-context) now rides
 under a per-launch aliased wire name via a new third GUID (AllowlistWireName) to prevent the
 reviewed repo from impersonating a canonical server name.
• Non-aliasing vendors (Cursor, Copilot, Kiro) remain byte-identical since AllowlistWireName is an
 identity function for them.
• Adds/extends unit tests pinning gate == injected set parity, aliasing behavior, and GUID
 independence across the three generated names.

Diagram

graph TD
    A["AcpHostedAgentRuntimeFactory.BuildProcessStartInfo"] --> B["AcpReviewFlowMcp.Build"]
    B --> C["LaunchIdentity.AllowlistWireName"]
    C --> D["session/new mcpServers"]
    B --> E["reviewGate comma-joined names"]
    E --> F["--allowed-mcp-server-names argv"]
    F --> G["AssertGeminiArgvIsCanonical"]
    D -.parity.-> F

    subgraph Legend
      direction LR
      _svc([Process step]) ~~~ _data[/Computed value/]
    end

Loading

Files changed (8) +233 / -67

Enhancement (1) +28 / -8

LaunchIdentity.csAdd AllowlistWireName aliasing for injected review servers +28/-8

Add AllowlistWireName aliasing for injected review servers

• Introduces a third independent GUID (allowlist suffix) and a new AllowlistWireName method that aliases non-result-channel injected server names for aliasing vendors, while returning names unchanged for non-aliasing vendors; FromGuids/ForLaunch signatures updated to take a third GUID.

src/Capacitor.Cli.Daemon/Acp/LaunchIdentity.cs

Bug fix (2) +50 / -15

AcpHostedAgentRuntimeFactory.csBuild review gate from injected server list instead of only the result channel +39/-13

Build review gate from injected server list instead of only the result channel

• Computes the allowlist value from the AcpReviewFlowMcp.Build server list and comma-joins their names into a single --allowed-mcp-server-names value; threads reviewGate into the whole-vector argv assertion (ExpectedGeminiArgv/AssertGeminiArgvIsCanonical) which now requires an explicit gate value for review launches.

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

AcpReviewFlowMcp.csAlias injected allowlist and review-context server names +11/-2

Alias injected allowlist and review-context server names

• Allowlist servers and the borrowed-snapshot kcap-review-context server now go through a WireName helper that applies LaunchIdentity.AllowlistWireName so their wire names match the aliased gate.

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

Tests (3) +141 / -35

LaunchIdentityTests.csExtend LaunchIdentity tests for third allowlist GUID +51/-20

Extend LaunchIdentity tests for third allowlist GUID

• Updates all FromGuids calls to include a third GUID and adds tests for AllowlistWireName aliasing behavior, GUID independence across all three names, and refusal of empty/reused GUIDs.

test/Capacitor.Cli.Tests.Unit/Acp/LaunchIdentityTests.cs

AcpHostedAgentRuntimeFactoryTests.csUpdate attacker-identity test for new GUID parameter +1/-0

Update attacker-identity test for new GUID parameter

• Adds a third GUID argument to a FromGuids call used in a caller-supplied identity isolation test.

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

GeminiReviewerLaunchTests.csAdd tests pinning gate-equals-injected-set parity and aliasing +89/-15

Add tests pinning gate-equals-injected-set parity and aliasing

• Adds AllowlistGuid and mcpAllowlist support to test helpers; new tests assert the review gate matches the comma-joined injected server names, that canonical allowlist ids never appear in the gate, and that a missing gate value fails the whole-vector assertion; updates existing tests for the new signatures.

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

Documentation (2) +14 / -9

AcpVendorDescriptor.csUpdate Gemini vendor descriptor docs for review-flow gate widening +11/-6

Update Gemini vendor descriptor docs for review-flow gate widening

• Documentation update explaining that a review launch now opens the MCP gate to exactly the servers it injects, replacing the prior deny-all-only note; references the coupling tests.

src/Capacitor.Cli.Daemon/Acp/AcpVendorDescriptor.cs

IHostedAgentRuntimeFactory.csUpdate McpAllowlist field documentation +3/-3

Update McpAllowlist field documentation

• Clarifies that the ACP factory now resolves the McpAllowlist into extra session/new servers admitted under per-launch wire names, rather than leaving it unused.

src/Capacitor.Cli.Daemon/Services/IHostedAgentRuntimeFactory.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. Identity not threaded ✓ Resolved 🐞 Bug ⛨ Security

Description

BuildProcessStartInfo may generate a fresh LaunchIdentity when ctx.LaunchIdentity is null, but it
computes Gemini’s reviewGate via ValidateAndBuildReviewFlowMcp(ctx, ...) which derives MCP server
wire names from ctx.LaunchIdentity. In the direct-builder/null-identity review-flow path, this can
produce a gate that admits canonical (repository-matchable) server ids instead of per-launch aliased
wire names, weakening the exact-name containment this PR intends.

Code

src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntimeFactory.cs[R438-441]

+                var reviewMcp = ValidateAndBuildReviewFlowMcp(ctx, descriptor, resolved)!;
+                reviewGate = string.Join(",", reviewMcp.Select(s => s.Name));
                for (var i = 0; i < argv.Count; i++)
                    if (argv[i] == identity.UnmatchableMcpName)
-                        argv[i] = identity.ResultChannelWireName;

Evidence

BuildProcessStartInfo creates a local identity but does not write it back into ctx before
calling ValidateAndBuildReviewFlowMcp(ctx, ...) to compute reviewGate.
ValidateAndBuildReviewFlowMcp returns AcpReviewFlowMcp.Build(ctx, ...), and
AcpReviewFlowMcp.Build derives both the result-channel name and allowlist server names from
ctx.LaunchIdentity (falling back to canonical ids when null). Therefore, when ctx.LaunchIdentity
is null, the computed gate can be based on canonical names rather than the intended per-launch
aliases.

src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntimeFactory.cs[404-443]
src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntimeFactory.cs[270-305]
src/Capacitor.Cli.Daemon/Services/AcpReviewFlowMcp.cs[17-26]
src/Capacitor.Cli.Daemon/Services/AcpReviewFlowMcp.cs[55-58]

Agent prompt

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

### Issue description
`BuildProcessStartInfo()` synthesizes `identity` when `ctx.LaunchIdentity` is null, but then computes `reviewGate` by calling `ValidateAndBuildReviewFlowMcp(ctx, ...)`. The MCP builder (`AcpReviewFlowMcp.Build`) uses `ctx.LaunchIdentity` to alias server names; when it’s null it falls back to canonical ids. This makes the direct-builder/null-identity review-flow path potentially emit a Gemini allowlist gate containing canonical (matchable) server names.

### Issue Context
This mismatch is introduced by the new “gate == injected set” logic which derives the gate from the built MCP list.

### Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntimeFactory.cs[404-443]

### Suggested fix
In `BuildProcessStartInfo`, after computing `identity`, create a `ctxWithIdentity` (or overwrite `ctx`) when `ctx.LaunchIdentity` is null:

- `var identity = ctx.LaunchIdentity ?? LaunchIdentity.ForLaunch(...);`
- `if (ctx.LaunchIdentity is null) ctx = ctx with { LaunchIdentity = identity };`

Then compute `reviewMcp` / `reviewGate` using that updated context, ensuring the gate uses the same per-launch wire names as the synthesized identity.

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[realtonyyoung]

CI note: attempt 1's Build and test (ubuntu-latest) failed on Snapshot_launch_failure_revokes_context_grant_and_removes_sidecar — an ENOENT spawning git inside CreateGitRepo(), i.e. in test setup before any code this PR touches is reached (this PR is Gemini MCP naming; the failure is causally excluded). windows-latest passed the same test on attempt 1, and the ubuntu job passed on a rerun of the same SHA (new job id 92116730868 vs 92113638760). All six checks are green on attempt 2.

Tracked separately as #452 with the evidence, the mechanisms I ruled out, and a diagnostic fix — .NET reports 'working directory missing' and 'executable not found' as the same ENOENT string, which is why the log can't name a cause.