[AI-1292] Unattended review-flow reviewer auto-approves its kcap MCP tools

[realtonyyoung]

Problem

A daemon-hosted review-flow reviewer (Codex) is launched unattended (--ask-for-approval never), but Codex still fires a PermissionRequest hook for MCP tool calls even under that flag. LocalPermissionBridge auto-approved only submit_review_result; every other MCP tool call routed to server.RequestPermissionAsync — an interactive UI prompt with no human present. Now that the code-review flow whitelists kcap-review for the reviewer, its first get_pr_summary call blocked the flow until someone manually clicked Allow, defeating the "unattended reviewer" promise. (Requester-independent: the code-review reviewer is always Codex; Claude reviewers avoid it via bypassPermissions.)

Surfaced during the AI-1224 MCP-autoconfig E2E.

Fix

An unattended review-flow launch mints a per-reviewer LocalPermissionBridge token (CSPRNG, its own listener prefix) bound to the launch's read-only kcap allowlist, and gives the reviewer that token's URL as KCAP_DAEMON_URL. Requests on a reviewer token auto-approve the reviewer's kcap tools; the shared (interactive) token is unchanged.

Design highlights (spec went 5 rounds with the Codex spec-review flow — docs/superpowers/specs/2026-07-09-reviewer-auto-approve-design.md):

• Daemon-originated trust. The "unattended" signal is a secret token only the reviewer process holds — an interactive agent (which knows only the shared token) can't claim it. No body flag / fixed path segment.
• Server-granularity, read-only contract. Auto-approval is bounded to ReviewFlowAutoApprovableServers (kcap-review, kcap-sessions) — the reviewer's Codex MCP config already confines its callable tools to the launch allowlist. kcap-memory (writes) / kcap-flows (flow-starting) are not auto-approvable.
• Request classification order: validate the token (unknown/revoked → 404) → validate the body (missing session_id/tool_name → 400) → submit_review_result (any live token, unchanged) → reviewer token (bare Codex name → allow; server-qualified → must be in the bound allowlist, else deny outright — never defer to a prompt that would hang).
• Fail fast, not hang. An invalid reviewer allowlist fails the launch (LaunchFailedAsync); it never falls back to the shared token.
• Lifecycle. Token revoked on every teardown path (early-return, catch, and CleanupAgentAsync after the process exits). Concurrent reviewers get independent tokens.
• Secrecy. Token is CSPRNG, never logged, and rides in KCAP_DAEMON_URL which PtyEnvScrub already scrubs.

Tests

29 new tests, all green in isolation (repeatedly):

• KcapMcpRegistryReviewFlowTests (8) — resolver accept/reject + static classification guard.
• LocalPermissionBridgeTests (+10, 33 total) — reviewer-token auto-approve (bare + qualified), out-of-allowlist deny, malformed/missing body → 400, shared-token unchanged (no escalation), revoked → 404, concurrency, token-never-logged.
• AgentOrchestratorVendorTests (+4, 45 total) — mint on ReviewFlow + revoke on cleanup, Default → no token, invalid allowlist → fail-fast, KCAP_DAEMON_URL scrubbed.

Notes for review

• Contract guard scope: implemented as the static classification check (adding an auto-approvable server without a tool classification trips CI). The live tools/list cross-check (catching a new mutating tool added to an already-classified server) is left as a follow-up rather than spawning MCP servers in a unit test — happy to add it if you'd prefer it in-scope.
• Local full-suite instability: the full daemon unit suite is flaky on my machine under parallel load (a SIGSEGV at startup on one run, a pre-existing TimeoutException in an unrelated Fake_records_… test on another, and loopback-port contention). This is pre-existing native/PTY suite instability, not from this pure-managed-code change; the new tests are green in isolation. CI is the authoritative full-suite check.
• Touches the daemon flow/permission internals (Alexey's area) — his review comes after the Codex code-review cycle.

Closes AI-1292.

[linear-code]

AI-1292

[qodo-code-review]

PR Summary by Qodo

Unattended ReviewFlow: mint per-reviewer bridge token to auto-approve kcap MCP tools

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Mint a per-reviewer LocalPermissionBridge token for ReviewFlow to auto-approve read-only kcap
 tools.
• Fail fast on non-auto-approvable allowlists; never fall back to interactive prompts that would
 hang.
• Add registry contract/allowlist resolution plus unit tests for bridge behavior, wiring, and
 secrecy.

Diagram

graph TD
AO["AgentOrchestrator"] --> MR["KcapMcpRegistry"] --> AO
AO --> LPB["LocalPermissionBridge"] --> DEC{"Token class?"}
AO --> REV(["ReviewFlow reviewer"])
AO --> INT(["Interactive agent"])
REV --> LPB
INT --> LPB
DEC -->|"reviewer token"| AA["Auto-approve / deny"]
DEC -->|"shared token"| SRV["Server permission prompt"]
subgraph Legend
  direction LR
  _svc["Service"] ~~~ _agent(["Agent"]) ~~~ _dec{"Decision"}
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Session-id keyed auto-approval registry

• ➕ No extra URL/token to mint and manage
• ➖ Race: first tool call can arrive before daemon learns session_id
• ➖ session_id is not secret; interactive agents could spoof unattended status

2. Agent-supplied unattended flag (body field or fixed path)

• ➕ Simpler request classification logic
• ➖ Violates daemon-originated trust: any agent with shared token could claim unattended status
• ➖ Expands blast radius if misused

3. Per-tool allowlist enforcement in the bridge

• ➕ Tighter granularity than server-level allowlist
• ➖ Codex sends bare tool names (no server qualifier), so enforcement is unreliable/spoofable
• ➖ High risk of future hangs when new tools are added but not curated

Recommendation: Keep the PR’s approach (per-reviewer CSPRNG token bound to a read-only server allowlist). It cleanly satisfies daemon-originated trust, avoids session_id timing races, preserves interactive behavior via the shared token, and prevents unattended hangs by denying out-of-allowlist qualified calls and failing fast on invalid allowlists.

Files changed (8) +1163 / -29

Enhancement (1) +70 / -0

KcapMcpRegistry.csDefine ReviewFlow auto-approvable servers and allowlist resolver +70/-0

Define ReviewFlow auto-approvable servers and allowlist resolver

• Adds a read-only server set for unattended ReviewFlow auto-approval, an explicit unattended-safe tool classification map, and a resolver that canonicalizes/dedupes allowlists while rejecting unknown, flow-starting, or non-auto-approvable servers.

src/Capacitor.Cli.Core/KcapMcpRegistry.cs

Bug fix (2) +217 / -27

AgentOrchestrator.csMint/revoke reviewer bridge token for ReviewFlow launches +60/-6

Mint/revoke reviewer bridge token for ReviewFlow launches

• For LaunchKind.ReviewFlow, validates the MCP allowlist via KcapMcpRegistry, registers a dedicated LocalPermissionBridge token URL as the runtime’s DaemonBridgeUrl, and stores it on AgentInstance for teardown. Ensures fail-fast behavior on invalid allowlists and revokes tokens on all cleanup paths.

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

LocalPermissionBridge.csAdd per-reviewer token registry and unattended request classification +157/-21

Add per-reviewer token registry and unattended request classification

• Replaces the single-token model with a shared token plus a live reviewer-token registry, minted via CSPRNG. Classifies requests by token before approval logic, auto-approves reviewer-token tool calls (and submit_review_result), denies out-of-allowlist server-qualified calls to avoid unattended hangs, and keeps shared-token behavior prompting via server round-trip.

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

Tests (3) +388 / -2

AgentOrchestratorReviewerTokenTests.csTest ReviewFlow token minting, fail-fast validation, and revocation wiring +119/-0

Test ReviewFlow token minting, fail-fast validation, and revocation wiring

• Adds orchestrator-level tests asserting ReviewFlow launches mint a dedicated bridge token (distinct from BaseUrl), revoke it on cleanup, do not mint tokens for Default launches, fail fast on non-auto-approvable allowlists, and confirm KCAP_DAEMON_URL is scrubbed.

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

KcapMcpRegistryReviewFlowTests.csTest ReviewFlow allowlist resolution and classification guardrails +80/-0

Test ReviewFlow allowlist resolution and classification guardrails

• Adds unit tests covering case-insensitive resolution, deduping, rejection of write/flow-starting/unknown servers, null/empty allowlist handling, and static guard assertions that auto-approvable servers are classified.

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

LocalPermissionBridgeTests.csAdd reviewer-token auto-approve/deny behavior and secrecy tests +189/-2

Add reviewer-token auto-approve/deny behavior and secrecy tests

• Extends bridge tests to cover reviewer-token auto-approval for bare and qualified tool names, deny behavior for out-of-allowlist qualified calls, 400s for malformed/missing fields, no escalation for shared token, 404 after revocation, concurrent token independence, and token-not-logged assertions.

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

Documentation (2) +488 / -0

2026-07-09-reviewer-auto-approve-plan.mdAdd implementation plan for per-reviewer auto-approve tokens +231/-0

Add implementation plan for per-reviewer auto-approve tokens

• Introduces a step-by-step execution plan covering registry changes, bridge token registry/classification, orchestrator wiring, secrecy checks, and test strategy for AI-1292.

docs/superpowers/plans/2026-07-09-reviewer-auto-approve-plan.md

2026-07-09-reviewer-auto-approve-design.mdAdd design spec for unattended reviewer MCP auto-approval +257/-0

Add design spec for unattended reviewer MCP auto-approval

• Documents the problem, security constraints, token-based design, request classification order, lifecycle/concurrency handling, and alternatives considered for AI-1292.

docs/superpowers/specs/2026-07-09-reviewer-auto-approve-design.md

[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. Bare tools auto-approved ✓ Resolved 🐞 Bug ⛨ Security

Description

IsReviewerToolAllowed returns true for any non-mcp__... tool name on a reviewer token,
regardless of vendor, so a ReviewFlow launch (or any request) using a reviewer token with a vendor
that sends bare tool names (e.g. Claude’s built-ins) would have those tools auto-approved without
allowlist enforcement. This makes the reviewer-token security boundary depend on the Codex MCP
config-lock assumption, but the orchestrator mints reviewer tokens for ReviewFlow launches without
checking vendor.

Code

src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[R403-407]

+    static bool IsReviewerToolAllowed(string toolName, string[] boundAllowlist) {
+        const string prefix = "mcp__";
+
+        if (!toolName.StartsWith(prefix, StringComparison.Ordinal)) return true;   // bare name → config-lock bounded
+

Evidence

The reviewer-token allow check currently returns true for any tool name that is not mcp__..., and
the orchestrator mints reviewer tokens for ReviewFlow launches without vendor gating; existing tests
also show Claude requests can use bare tool names like Bash, which would be auto-approved if sent
on a reviewer token.

src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[393-420]
src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[396-422]
test/Capacitor.Cli.Tests.Unit/LocalPermissionBridgeTests.cs[136-162]

Agent prompt

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

## Issue description
Reviewer-token auto-approval currently treats *any* non-`mcp__...` tool name as allowed. That is only safe under the specific assumption that the requester is Codex and its MCP config-lock constrains callable servers; for other vendors (or future behavior changes), bare tool names can correspond to non-kcap / mutating built-in tools.

## Issue Context
- Reviewer tokens are minted for `LaunchKind.ReviewFlow` without checking the vendor.
- `IsReviewerToolAllowed` unconditionally allows bare tool names (`!toolName.StartsWith("mcp__")`).

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[396-422]
- src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[306-325]
- src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[393-420]

## Suggested fix
1. Make bare-name auto-approval conditional on the requesting vendor being `codex`:
  - Pass `vendor` into `IsReviewerToolAllowed(...)` (or handle inline) and:
    - If `vendor == "codex"`: allow bare names (existing behavior).
    - If `vendor == "claude"`: require a server-qualified `mcp__<server>__<tool>` name and enforce `<server>` ∈ bound allowlist; otherwise deny.
2. Defense-in-depth in the orchestrator: only mint/register reviewer tokens for ReviewFlow launches when `cmd.Vendor` is `codex` (or when the vendor is known to be constrained by an equivalent config-lock).
3. Add/adjust unit tests to prove that on a reviewer token a Claude bare tool name like `"Bash"` is **denied** (and does not round-trip to `server.RequestPermissionAsync`).

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

---

2. AI-1292 in comments ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

New/modified code comments include Linear issue identifiers (e.g., AI-1292, AI-1139), which
violates the policy against embedding Linear IDs in source comments. This can leak internal tracker
references and makes public-friendly references inconsistent.

Code

src/Capacitor.Cli.Core/KcapMcpRegistry.cs[R28-36]

+    // ── AI-1292: unattended review-flow reviewer auto-approval ──────────────────────────
+    //
+    // A hosted review-flow reviewer runs unattended, so any MCP tool it calls must be
+    // auto-approved (there's no human to answer a prompt). Authorization is a per-reviewer
+    // bridge token (see LocalPermissionBridge); this registry defines WHICH servers/tools that
+    // auto-approval may cover. The unit is the SERVER (bare Codex tool names carry no server,
+    // and an exact tool-name gate would HANG the reviewer on an un-curated tool). The set is
+    // therefore restricted to READ-ONLY kcap servers whose every tool is safe to run unattended
+    // — deliberately excluding the write server `kcap-memory` and the flow-starting `kcap-flows`.

Evidence

PR Compliance ID 6 forbids Linear identifiers (AI-###) in code comments. The cited added/modified
comment blocks contain AI-1292/AI-1139, directly matching the failure criteria.

CLAUDE.md: Do Not Use Linear Issue Numbers in Code Comments
src/Capacitor.Cli.Core/KcapMcpRegistry.cs[28-36]
src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[38-41]
src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[39-44]
src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[300-308]
test/Capacitor.Cli.Tests.Unit/LocalPermissionBridgeTests.cs[571-572]

Agent prompt

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

## Issue description
Code comments added/modified in this PR contain Linear issue identifiers (e.g., `AI-1292`, `AI-1139`), which is disallowed.

## Issue Context
Compliance requires that comments do not include Linear IDs. If a reference is needed, prefer a GitHub issue reference (e.g., `#255`) or remove the identifier entirely.

## Fix Focus Areas
- src/Capacitor.Cli.Core/KcapMcpRegistry.cs[28-36]
- src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[38-41]
- src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[39-44]
- src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[300-308]
- test/Capacitor.Cli.Tests.Unit/LocalPermissionBridgeTests.cs[571-572]

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

---

3. Reviewer body fields under-validated ✓ Resolved 🐞 Bug ≡ Correctness

Description

Reviewer-token requests only reject session_id when null and tool_name when null/empty, so
whitespace (and some empty-after-normalization) values can still reach the reviewer-token
auto-approve/deny logic. This violates the design requirement that reviewer-token requests must have
non-empty session_id and tool_name before any auto-approval decision.

Code

src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[R306-324]

+            } else if (isReviewer) {
+                // AI-1292: an unattended review-flow reviewer. Its Codex MCP config confines it to
+                // the bound (read-only) kcap servers, so a tool call arriving on its token is an
+                // auto-approvable kcap tool. A reviewer request MUST carry a tool to classify; an
+                // out-of-allowlist server-qualified call is an authorization failure we DENY
+                // outright — never deferred to a prompt no human can answer (that would hang).
+                if (string.IsNullOrEmpty(toolName)) {
+                    context.Response.StatusCode = 400;
+                    context.Response.Close();
+
+                    return;
+                }
+
+                if (IsReviewerToolAllowed(toolName, reviewerAllowlist!)) {
+                    decision = new PermissionDecision("allow", null, null);
+                } else {
+                    LogReviewerToolDenied(logger, sessionId, toolName);
+                    decision = new PermissionDecision("deny", null, null);
+                }

Evidence

The design states reviewer-token requests must have non-empty session_id and tool_name before
any auto-approval, but the implementation only checks sessionId is null and uses
IsNullOrEmpty(toolName); whitespace values can pass and reach the auto-approval path.

docs/superpowers/specs/2026-07-09-reviewer-auto-approve-design.md[117-123]
src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[274-325]

Agent prompt

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

## Issue description
Reviewer-token requests can currently be processed (and even auto-approved) with malformed-but-non-null fields:
- `session_id` is only checked for null, so `""`, whitespace, or values that become empty after dash stripping can pass.
- `tool_name` uses `IsNullOrEmpty`, so whitespace-only values pass and are then treated as a bare name (auto-allowed).

This contradicts the reviewer-token request classification contract that requires a non-empty `session_id` and `tool_name` before any auto-approval.

## Issue Context
Design/spec explicitly requires non-empty `session_id` and `tool_name` before steps 3–4 (auto-approval) are reachable.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[274-333]
- docs/superpowers/specs/2026-07-09-reviewer-auto-approve-design.md[117-123]

## Suggested fix
1. Tighten validation:
  - After computing `sessionId = ...Replace("-", "")`, reject if `string.IsNullOrWhiteSpace(sessionId)` (400).
  - In the `isReviewer` branch, reject if `string.IsNullOrWhiteSpace(toolName)` (400) instead of `IsNullOrEmpty`.
2. Add a unit test for reviewer token:
  - `session_id: "----"` (or `" "`) returns 400.
  - `tool_name: " "` returns 400.
3. Ensure these validations happen before any reviewer-token allow/deny decision is made (so malformed requests never get auto-approved).

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

---

4. Verbose reviewer-token explanation comment ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

New comments are lengthy and repeat design details inline, reducing readability and increasing
maintenance overhead. This violates the guideline to keep comments minimal and let code
structure/naming convey intent.

Code

src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[R396-400]

+            // AI-1292: an unattended review-flow reviewer must auto-approve its kcap tool calls (no
+            // human is present). Mint a dedicated bridge token bound to the launch's read-only kcap
+            // allowlist and give the reviewer that token's URL as KCAP_DAEMON_URL. An invalid
+            // allowlist fails the launch FAST — never a shared-token launch that would hang on a
+            // prompt no one can answer. Non-review-flow launches keep the shared (interactive) token.

Evidence

PR Compliance ID 5 requires concise, self-explanatory comments. The cited blocks are multi-line
explanatory narratives that could be reduced to a short summary (with a link to the design doc if
needed).

CLAUDE.md: Keep Comments Minimal and Self-Explanatory (Prefer Clear Code Over Verbose Comments)
src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[396-400]
src/Capacitor.Cli.Core/KcapMcpRegistry.cs[28-36]

Agent prompt

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

## Issue description
Several newly-added comments are verbose and restate design/spec rationale inline rather than staying concise.

## Issue Context
Compliance asks for minimal, self-explanatory comments. Consider shortening to a brief summary and (if needed) linking to the design/spec doc instead of embedding multi-line rationale.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[396-400]
- src/Capacitor.Cli.Core/KcapMcpRegistry.cs[28-36]

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

---