Surface ACP permission requests to the desktop app, first answer wins

[realtonyyoung]

AI-2197

What & why

An ACP agent's permission request (Copilot, Cursor, Gemini, Kiro) went only to the server's web card; the desktop app's local permission broker was fed only by the Claude Code / Codex HTTP hooks, so a hosted ACP agent's permission never appeared in the desktop app. This wraps the server-facing interaction delegate the ACP bridge already calls: for a permission it registers a card on the local PermissionPromptBroker (which the app subscribes to over local control IPC) alongside the server and races the two — first answer wins, the loser is dismissed. The desktop card answers allow/deny, which maps back to one of the agent's own offered options by kind, so MapPermissionDecision resolves the option id exactly as a server-side pick would. Elicitations stay server-only.

Where to look

AcpPermissionSurface is the whole race + mapping. It is wired at the five ACP factory registrations in DaemonRunner and used by AcpHostedAgentRuntimeFactory only when a broker is present — null keeps a launch server-only, so every existing test and the review-flow path are unchanged. The broker is the same DI singleton the desktop app's PermissionIpc reads and the Claude/Codex LocalPermissionBridge already writes.

Safety properties the surface holds, mirroring LocalPermissionBridge:

• A generic allow never becomes a standing grant. The desktop shows one "Allow"; if the agent offers only an allow_always option, honouring the click would grant more than was shown, so it fails closed to a deny rather than escalating.
• The local card reuses the bounded pending builder. An arbitrarily large tool name or input from an ACP child would otherwise ride a control frame the codec rejects and replays on every subscription forever; an over-cap payload skips the local card and stays server-only.
• The server id is paired onto the local card via PermissionPromptBroker.TryCorrelate, so a client that sees both the server and the local lane coalesces them into one card instead of showing two.
• Every settlement is written to permission-decisions.jsonl, so a locally-answered ACP decision is in the audit log like a Claude/Codex one.

When the desktop answers first, the same mapped decision is submitted back to the still-open server interaction, so server history records that allow/deny and the server's first-writer-wins tracker keeps a web answer that already landed. A visible web card may linger in a browser until refreshed, but the interaction is resolved and the agent is answered exactly once.

Verification

AcpPermissionSurfaceTests (8): a desktop allow maps to the allow option's id; a deny to a deny outcome; a generic allow with only a standing grant offered fails closed; the server id is correlated onto the local card; a settlement is written to the audit log; an oversized tool name skips the local card and stays server-only; a server answer is returned and dismisses the desktop card; an elicitation never registers a card. AcpHostedAgentRuntimeFactoryTests and AcpHostedAgentRuntimePermissionTests stay green; daemon AOT publish is IL-clean.

[qodo-code-review]

PR Summary by Qodo

Surface ACP permission prompts to desktop with first-answer wins

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Surface ACP permissions in desktop and server interfaces concurrently.
• Resolve the first response and map desktop decisions to agent-offered options.
• Keep elicitations server-only and verify both race outcomes.

Diagram

sequenceDiagram
    participant Agent as ACP Agent
    participant Bridge as Interaction Bridge
    participant Surface as Permission Surface
    participant Server as Server Web UI
    participant Broker as Prompt Broker
    actor Desktop as Desktop App
    Agent->>Bridge: Permission request
    Bridge->>Surface: Request interaction
    par Server card
        Surface->>Server: Request decision
    and Desktop card
        Surface->>Broker: Register prompt
        Broker-->>Desktop: Publish prompt
    end
    alt Server answers first
        Server-->>Surface: ACP decision
        Surface->>Broker: Dismiss prompt
    else Desktop answers first
        Desktop-->>Broker: Allow or deny
        Broker-->>Surface: Settlement
        Surface-->>Server: Cancel wait
    end
    Surface-->>Bridge: Winning decision
    Bridge-->>Agent: Selected option

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Server-mediated prompt fan-out

• ➕ Creates one authoritative request shared by web and desktop clients.
• ➕ Could withdraw or synchronize the web card after a desktop response.
• ➖ Requires new server APIs and lifecycle coordination.
• ➖ Broadens the change across backend and desktop connectivity layers.

2. Embed routing in AcpInteractionBridge

• ➕ Keeps all ACP interaction routing in one component.
• ➕ Avoids introducing a separate delegate wrapper.
• ➖ Couples protocol translation directly to local IPC infrastructure.
• ➖ Risks affecting unattended, review-flow, and server-only interaction paths.

Recommendation: Keep the delegate-wrapper approach used by this PR. It isolates dual-surface coordination from ACP protocol handling, preserves server-only behavior when no broker is supplied, and reuses the established desktop broker. Server-mediated fan-out is the stronger long-term option if web-card withdrawal becomes necessary, but it is disproportionate for this fix.

Files changed (4) +236 / -7

Enhancement (1) +14 / -2

AcpHostedAgentRuntimeFactory.csRoute ACP interactions through the optional desktop surface +14/-2

Route ACP interactions through the optional desktop surface

• Adds an optional permission broker dependency and wraps the server interaction delegate when it is present. Null broker configurations retain the existing server-only behavior for tests and review flows.

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

Bug fix (1) +118 / -0

AcpPermissionSurface.csRace ACP server and desktop permission responses +118/-0

Race ACP server and desktop permission responses

• Introduces the dual-surface permission coordinator, where the first server or desktop response wins. It maps desktop allow decisions to offered ACP options, handles deny outcomes safely, dismisses local prompts after server responses, and leaves elicitations server-only.

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

Tests (1) +94 / -0

AcpPermissionSurfaceTests.csVerify ACP permission race and decision mapping +94/-0

Verify ACP permission race and decision mapping

• Covers desktop allow and deny mapping, server-first settlement with local prompt dismissal, and elicitation bypass of the desktop broker.

test/Capacitor.Cli.Daemon.Tests.Unit/Services/AcpPermissionSurfaceTests.cs

Other (1) +10 / -5

DaemonRunner.csInject the desktop permission broker into ACP factories +10/-5

Inject the desktop permission broker into ACP factories

• Passes the shared PermissionPromptBroker singleton to all five production ACP runtime factory registrations, enabling desktop permission prompts for every supported ACP vendor.

src/Capacitor.Cli.Daemon/DaemonRunner.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Desktop answers are recorded cancelled ✓ Resolved 🔗 Cross-repo conflict ≡ Correctness

Description

RequestAsync cancels serverTask after a desktop settlement, which makes
AwaitInteractionDecisionAsync submit the fixed cancel outcome instead of the mapped allow or
deny. Whenever the desktop wins, kcap-server accepts that cancellation as the first decision and
persists it as the sole ACP resolution, so server history disagrees with the decision returned to
the agent.

Code

src/Capacitor.Cli.Daemon/Services/AcpPermissionSurface.cs[R65-68]

+        // Desktop app answered first — stop waiting on the server, map allow/deny to an offered option.
+        var settlement = await localTask.ConfigureAwait(false);
+        linked.Cancel();
+        Observe(serverTask);

Relevance

●●● Strong

Recent accepted precedents prioritize preserving cancellation and server outcome semantics across
asynchronous races.

PR-#734
PR-#149

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed desktop-winner branch cancels the server task and independently returns its mapped
result. The daemon's cancellation handler converts that cancellation into a cancel response, while
kcap-server treats the first response as terminal and persists that exact decision as the single ACP
resolution.

src/Capacitor.Cli.Daemon/Services/AcpPermissionSurface.cs[65-70]
src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[1123-1145]
External repo: kurrent-io/kcap-server, src/Capacitor.Server.Services/Sessions/AcpInteractionTracker.cs [67-79]
External repo: kurrent-io/kcap-server, src/Capacitor.Server/Hooks/SessionHookHandlers.cs [1997-2042]

Agent prompt

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

## Issue description
When the desktop permission surface wins, cancelling the server task invokes the existing abandonment path, which resolves the server interaction as `cancel`. The daemon must instead submit the desktop's mapped allow or deny decision, including its selected ACP option, so the server and agent record the same result.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/AcpPermissionSurface.cs[65-70]
- src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[1101-1145]

## Recommended Fix
Refactor the server interaction API so the local winner can resolve the server request with the mapped `AcpInteractionDecision` and its selected option ID. Let kcap-server's existing first-writer-wins tracker determine whether the desktop or web response won, then return that accepted decision instead of cancelling through the abandonment path.

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

---

2. Allow can grant permanent access ✓ Resolved 🐞 Bug ⛨ Security

Description

PickAllow falls back to allow_always when no once-use option exists, even though the ACP card
hides “Allow always” and displays only a generic “Allow” button. When an agent offers only a
standing grant, clicking that button returns its option id and lets the vendor persist access
without telling the user that scope.

Code

src/Capacitor.Cli.Daemon/Services/AcpPermissionSurface.cs[109]

+        return preferAlways ? always ?? once ?? other : once ?? other ?? always;

Relevance

●●● Strong

Recent security precedents accept tightening permission matching to prevent unintended grants and
capability bypasses.

PR-#304
PR-#255

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The desktop view exposes only a button labeled “Allow” for non-Claude vendors, because “Allow
always” is visible only when entry.Vendor == "claude"; all newly wired ACP vendors use their own
vendor names. Nevertheless, PickAllow returns always when no once-use or fallback option exists,
while the existing ACP bridge explicitly documents that allow_always is a standing vendor-side
grant after which the vendor may stop asking.

src/Capacitor.App/ViewModels/PermissionCardViewModel.cs[32-44]
src/Capacitor.App/Views/PendingCardTemplates.axaml[24-30]
src/Capacitor.Cli.Daemon/Services/AcpPermissionSurface.cs[98-109]
src/Capacitor.Cli.Daemon/Acp/AcpInteractionBridge.cs[747-755]
src/Capacitor.Cli.Daemon/DaemonRunner.cs[462-502]

Agent prompt

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

## Issue description
The desktop ACP card labels its action “Allow,” but `PickAllow` falls back to an `allow_always` option when no `allow_once` option exists. This can turn an ordinary approval into an undisclosed persistent vendor grant.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/AcpPermissionSurface.cs[98-109]
- test/Capacitor.Cli.Daemon.Tests.Unit/Services/AcpPermissionSurfaceTests.cs[39-55]

## Recommended Fix
For the generic desktop Allow action, select only one unambiguous `allow_once` option with a nonblank, unique option id. Fail closed when no such option exists, and add tests proving that a sole `allow_always` option and ambiguous once-use options are not selected.

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

---

3. Large tool calls hide desktop prompts ✓ Resolved 🐞 Bug ☼ Reliability

Description

BuildPending copies request.ToolInput into the local DTO and marks it present without applying
the permission wire's element-size bound. ACP accepts unbounded JSON lines, so a tool call above the
local codec's 8 MiB payload cap is broadcast to the desktop, rejected by FrameCodec.ReadAsync, and
never appears as a usable prompt.

Code

src/Capacitor.Cli.Daemon/Services/AcpPermissionSurface.cs[79]

+            ToolInput:  request.ToolInput,

Relevance

●●● Strong

Accepted precedents consistently enforce bounds and reject unbounded or malformed server-provided
inputs.

PR-#188
PR-#253

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The established local bridge drops tool input above PermissionWire.MaxElementBytes and records
that omission, specifically to keep permission frames bounded. The new builder instead forwards the
complete ACP ToolCall; ACP's read loop uses ReadLineAsync without a length limit, while local
IPC rejects received payloads above 8 MiB.

src/Capacitor.Cli.Daemon/Services/AcpPermissionSurface.cs[73-86]
src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[743-756]
src/Capacitor.Cli.Core/LocalIpc/PermissionIpc.cs[37-43]
src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs[9-29]
src/Capacitor.Cli.Daemon/Acp/AcpConnection.cs[186-204]
src/Capacitor.Cli.Daemon/Acp/AcpInteractionBridge.cs[304-320]

Agent prompt

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

## Issue description
The new ACP desktop DTO bypasses the size handling used by the existing local permission bridge. A sufficiently large agent-supplied tool call can exceed the local IPC frame limit and break delivery of the permission prompt.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/AcpPermissionSurface.cs[73-86]
- test/Capacitor.Cli.Daemon.Tests.Unit/Services/AcpPermissionSurfaceTests.cs[39-55]

## Recommended Fix
Apply `PermissionWire` bounds while constructing the ACP pending DTO, omitting oversized tool input and setting `ToolInputOmitted` to true as the existing local bridge does. Also bound the other caller-controlled fields and add an oversized-input test that verifies the emitted DTO remains within IPC limits.

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

---

4. New boolean check bypasses helpers ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The added IsTrue method directly pattern-matches JsonElement.ValueKind rather than calling a
JsonElementExtensions helper. Permission settlement mapping now introduces a separate JSON
shape-check idiom that later handling changes can miss.

Code

src/Capacitor.Cli.Daemon/Services/AcpPermissionSurface.cs[115]

+    static bool IsTrue(JsonElement? value) => value is { ValueKind: JsonValueKind.True };

Relevance

●●● Strong

A recent accepted precedent explicitly requires repository JSON helpers instead of direct ValueKind
comparisons.

PR-#497

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 2270023 prohibits new direct JsonElement.ValueKind comparisons when performing
JSON type or shape checks. The added IsTrue implementation directly pattern-matches ValueKind
against JsonValueKind.True, while the repository extension class establishes the required
helper-based pattern.

Rule 2270023: Use JsonElementExtensions helpers instead of direct JsonValueKind comparisons
src/Capacitor.Cli.Daemon/Services/AcpPermissionSurface.cs[115-115]
src/Capacitor.Models.Transcripts/JsonElementExtensions.cs[5-13]

Agent prompt

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

## Issue description
`IsTrue` directly checks `JsonElement.ValueKind`, bypassing the repository's required helper abstraction.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/AcpPermissionSurface.cs[115-115]
- src/Capacitor.Models.Transcripts/JsonElementExtensions.cs[5-13]

## Recommended Fix
Add or expose an appropriate `JsonElementExtensions` helper for checking an explicit true boolean value, then replace the direct `ValueKind` pattern in `IsTrue` with that helper.

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

---

Context sources

✅ Compliance rules (platform): 64 rules

✅ Cross-repo context — repo relationships

  Explored: repo: kurrent-io/kcap-server (sha: f582e72a)

Review mode: ⚖️ Balanced: This introduces concurrency-sensitive permission arbitration across desktop IPC, server interaction handling, DI wiring, cancellation, and option mapping, creating genuine behavioral and integration risk but not enough independent logic density to warrant extended review.

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[linear-code]

AI-2197