Skip to content

Make branch-authored MCP config reviewable without executing it - #443

Merged
realtonyyoung merged 7 commits into
mainfrom
codex/ai-1706-reviewable-mcp-config
Aug 4, 2026
Merged

Make branch-authored MCP config reviewable without executing it#443
realtonyyoung merged 7 commits into
mainfrom
codex/ai-1706-reviewable-mcp-config

Conversation

@realtonyyoung

Copy link
Copy Markdown
Collaborator

Summary

  • extract recognized workspace MCP config from Git index stage 0 as exact, bounded evidence
  • keep executable config absent from borrowed reviewer snapshots and serve the evidence through an exact loopback capability
  • inject a daemon-only read MCP server into independent-snapshot runtimes and atomically refresh/revoke its unified grant
  • harden snapshot path identity, case handling, symlink boundaries, sidecar permissions, and cleanup

Why

Borrowed reviewers previously had to omit branch-authored MCP configuration to avoid executing untrusted commands, which also made those changes invisible to review. This exposes the staged Git blobs as explicitly untrusted review evidence without placing them in the reviewer worktree.

Validation

  • unit test project build (single worker): passed
  • integration test project build (single worker): passed
  • BorrowedReviewContextTests: 16 passed
  • LocalPermissionBridgeTests: 49 passed
  • McpReviewContextServerTests: 6 passed
  • AcpReviewFlowMcpContextTests: 3 passed
  • focused snapshot launch/refresh/failure lifecycle tests: passed
  • focused process integration test: passed
  • focused Copilot exact allowlist test: passed
  • git diff --check: clean
  • no AI issue identifiers in production/test C#

Full unit/integration suites and NativeAOT publishing are intentionally delegated to CI to avoid overloading the development machine.

Design

  • docs/superpowers/specs/2026-08-02-ai1706-reviewable-mcp-config-design.md
  • docs/superpowers/plans/2026-08-03-ai1706-reviewable-mcp-config-implementation.md

@linear-code

linear-code Bot commented Aug 3, 2026

Copy link
Copy Markdown

AI-1706

…ble-mcp-config

# Conflicts:
#	src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs
#	test/Capacitor.Cli.Tests.Unit/Services/AcpHostedAgentRuntimeFactoryTests.cs
@realtonyyoung
realtonyyoung marked this pull request as ready for review August 3, 2026 19:05
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Expose branch-authored MCP config for borrowed reviews via sidecar + loopback MCP tool

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Capture workspace MCP config from Git index blobs as bounded, review-only evidence
• Serve evidence via an exact 127.0.0.1 capability and daemon-only MCP server
• Harden borrowed snapshot path/symlink/case handling and cleanly revoke/refresh context
Diagram

graph TD
  A["Source repo (Git index)"] --> B["WorktreeManager"] --> C[("Review-context sidecar")] --> D["LocalPermissionBridge"] --> E["Loopback capability URL"] --> F["kcap-review-context MCP"] --> G["Borrowed reviewer runtime"]
  B -. "refresh publishes new generation" .-> D
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Embed MCP config into server-authored review prompt
  • ➕ Guaranteed visibility to the model without adding a new local MCP server
  • ➕ Avoids loopback HTTP capability surface
  • ➖ Requires kcap-server prompt contract/version changes and coordinated rollout
  • ➖ Raises questions about persistence/retention of branch-authored content server-side
  • ➖ Couples a local containment fix to remote infrastructure
2. In-tree quarantine (rename/suffix) inside snapshot
  • ➕ Simpler reviewer UX (files exist in worktree, easy to inspect)
  • ➕ No additional MCP server/tooling
  • ➖ Cannot prove bytes came from the branch (skip-worktree/assume-unchanged/private overrides)
  • ➖ Expands the snapshot copy surface (path mapping, encoding, collisions, symlinks) and increases risk
  • ➖ Reintroduces executable/config discovery confusion
3. Expose sidecar via filesystem path to reviewer
  • ➕ No loopback HTTP server routing changes
  • ➕ Direct file reads
  • ➖ Requires widening sandbox/FS permissions and risks path traversal or writes
  • ➖ Harder to make access 'one exact operation' and to audit

Recommendation: Keep the PR’s approach: extract exact stage-0 blobs into an owner-only sidecar and expose them only via a narrowly scoped loopback capability + daemon-only MCP tool. It preserves non-execution guarantees while making branch-authored config reviewable, and it fails closed if extraction/bounding cannot be done safely.

Files changed (21) +2803 / -62

Enhancement (9) +775 / -60
WorktreeManager.ReviewContext.csGenerate immutable review-context manifest from stage-0 Git index +345/-0

Generate immutable review-context manifest from stage-0 Git index

• Introduces types and logic to extract recognized workspace MCP config paths from raw 'git ls-files --stage -z' output before decoding unrelated paths. Writes an owner-only sidecar generation containing a validated manifest with bounded total bytes (256KiB), base64 content, and SHA-256, and provides helpers for case probing, path classification, and strict validation.

src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs

WorktreeManager.csCreate/refresh borrowed snapshots with review-context sidecar + hardened path handling +176/-42

Create/refresh borrowed snapshots with review-context sidecar + hardened path handling

• Extends borrowed snapshot creation and refresh to produce and publish a review-context generation adjacent to the snapshot root and to clean it up on removal/orphan cleanup. Hardens UTF-8 path normalization (rejects rewriting), adds case-sensitivity probing, avoids following reparse points/symlinks in both source and destination components, and makes exclusions/collisions respect filesystem case policy.

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

LocalPermissionBridge.csServe review-context via exact loopback GET and support atomic generation swaps +53/-5

Serve review-context via exact loopback GET and support atomic generation swaps

• Changes reviewer token storage to a structured grant containing both tool allowlist and an optional bound review-context generation. Adds an exact, queryless GET route '/{token}/review-context/workspace-mcp-configs' that returns immutable manifest bytes only for live reviewer tokens, plus an atomic publish method to swap generations and return the retired one for deletion.

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

AgentOrchestrator.csBind review-context capability to borrowed snapshots and refresh it safely +43/-13

Bind review-context capability to borrowed snapshots and refresh it safely

• Mints a single reviewer-token record representing the union of unattended permission allowlist (Codex) and borrowed snapshot review context. Provides a per-launch capability URL for borrowed snapshots and, on refresh, publishes a new generation to the bridge and deletes the retired on-disk generation to avoid drift/leaks.

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

IHostedAgentRuntimeFactory.csPlumb review-context loopback capability into runtime start context +3/-0

Plumb review-context loopback capability into runtime start context

• Adds 'ReviewContextCapabilityUrl' to 'RuntimeStartContext' so hosted runtime factories can inject the daemon-only context MCP server during borrowed snapshot review flows.

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

AcpReviewFlowMcp.csInject kcap-review-context server for borrowed snapshot review flows +10/-0

Inject kcap-review-context server for borrowed snapshot review flows

• When 'IsBorrowedSnapshot' is true, injects an MCP server named 'kcap-review-context' that runs 'kcap mcp review' with only local env vars ('KCAP_REVIEW_CONTEXT_*'). Fails before launch if the capability URL is missing to avoid blind reviews.

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

AcpHostedAgentRuntimeFactory.csExpose review-context tool id as available in hosted runtimes +5/-0

Expose review-context tool id as available in hosted runtimes

• Adds the 'kcap-review-context-get_branch_authored_mcp_configs' tool id to the set of available tools when the reserved context server is present.

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

McpReviewContextServer.csDaemon-only MCP server exposing one review-context tool +133/-0

Daemon-only MCP server exposing one review-context tool

• Implements a minimal stdio MCP server mode with exactly one argumentless tool ('get_branch_authored_mcp_configs') that performs one HTTP GET to a validated 127.0.0.1 capability URL. Intentionally avoids backend URL/auth/config state and returns the manifest body as text, labeling it as untrusted evidence.

src/Capacitor.Cli/Commands/McpReviewContextServer.cs

Program.csEarly-dispatch context-mode entrypoint for 'kcap mcp review' +7/-0

Early-dispatch context-mode entrypoint for 'kcap mcp review'

• Adds an early 'mcp review' dispatch gate keyed by 'KCAP_REVIEW_CONTEXT_MODE=1' so the context server runs before backend URL resolution or update/config checks, ensuring it has no non-loopback authority.

src/Capacitor.Cli/Program.cs

Tests (9) +1116 / -2
McpReviewContextServerIntegrationTests.csIntegration test for context-mode MCP server performing a single exact GET +116/-0

Integration test for context-mode MCP server performing a single exact GET

• Spawns 'kcap mcp review' in daemon-only mode, verifies tools/list exposes only the context tool, and asserts 'tools/call' triggers exactly one GET to the exact route and leaves config dir untouched (no backend/config side effects).

test/Capacitor.Cli.Tests.Integration/McpReviewContextServerIntegrationTests.cs

McpReviewContextServerTests.csUnit tests for capability URL validation and tool shape +27/-0

Unit tests for capability URL validation and tool shape

• Verifies capability URLs must be exact (http, 127.0.0.1, fixed path suffix, no query/fragment) and that the server exposes exactly one argumentless tool with untrusted-evidence warnings.

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

AcpReviewFlowMcpContextTests.csUnit tests for context server injection rules +45/-0

Unit tests for context server injection rules

• Asserts borrowed snapshots inject 'kcap-review-context' with only local env vars, direct reviews do not inject it, and missing capability URL fails fast before launch.

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

BorrowedReviewContextTests.csComprehensive tests for review-context extraction and fail-closed safety +464/-0

Comprehensive tests for review-context extraction and fail-closed safety

• Adds extensive coverage ensuring staged/committed index blobs are returned (not private working-tree bytes), and that unsafe conditions fail closed (reserved descendants, symlinks/modes, unmerged stages, invalid UTF-8 under reserved paths, capacity bounds). Verifies POSIX permissions on sidecar storage and cleanup semantics.

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

WorktreeManagerTests.csTests for symlink boundaries, path identity rules, and sidecar orphan cleanup +220/-0

Tests for symlink boundaries, path identity rules, and sidecar orphan cleanup

• Adds tests asserting destination parent/leaf symlinks are rejected without touching external targets, source linked components are rejected, invalid paths requiring rewriting are rejected, case-sensitive alias cleanup works, and orphan cleanup preserves active sidecars while removing orphaned ones and avoids following a linked borrowed-snapshots root.

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

LocalPermissionBridgeTests.csTests for exact context GET routing and atomic publish semantics +69/-0

Tests for exact context GET routing and atomic publish semantics

• Verifies the reviewer-only exact GET route returns the bound generation bytes, shared/revoked/non-exact routes cannot access context, and publishing context atomically replaces the generation and returns the retired value.

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

AgentOrchestratorBorrowLaunchTests.csBorrowed snapshot launch tests for context capability, refresh, and cleanup +169/-1

Borrowed snapshot launch tests for context capability, refresh, and cleanup

• Extends borrowed review flow tests to assert '.mcp.json' is absent from snapshot worktree, the capability URL is present, refresh swaps context generations and deletes retired storage, and stop/launch-failure/refresh-failure revokes the token and removes snapshot + sidecar.

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

AcpHostedAgentRuntimeFactoryTests.csExpect reserved review-context tool in available-tools list +4/-1

Expect reserved review-context tool in available-tools list

• Updates expected available tools to include 'kcap-review-context-get_branch_authored_mcp_configs' and supplies a review-context capability URL in the test runtime context.

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

AgentOrchestratorVendorTests.csAdd test seam for runtime start failures +2/-0

Add test seam for runtime start failures

• Extends the spy hosted runtime factory to optionally throw on StartAsync, enabling launch-failure cleanup assertions in borrowed snapshot tests.

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

Documentation (3) +912 / -0
README.mdDocument borrowed-review MCP config visibility via review-context tool +5/-0

Document borrowed-review MCP config visibility via review-context tool

• Clarifies that borrowed snapshots keep MCP config non-executable while still reviewable via a private, read-only review-context tool backed by Git index bytes. Documents omission of unstaged/untracked config and fail-closed behavior.

README.md

2026-08-02-ai1706-reviewable-mcp-config-design.mdDesign spec for reviewable, non-executable MCP config evidence +578/-0

Design spec for reviewable, non-executable MCP config evidence

• Adds a detailed design covering sidecar extraction from stage-0 Git blobs, loopback capability exposure, MCP injection, and security/fail-closed constraints. Documents rejected alternatives (in-tree quarantine, prompt embedding, sidecar paths, injecting kcap-review).

docs/superpowers/specs/2026-08-02-ai1706-reviewable-mcp-config-design.md

2026-08-03-ai1706-reviewable-mcp-config-implementation.mdImplementation plan and constraints for AI-1706 +329/-0

Implementation plan and constraints for AI-1706

• Adds a task-by-task plan describing how to implement extraction, sidecar lifecycle, capability serving, MCP injection, orchestrator refresh/revoke flows, and the focused test matrix.

docs/superpowers/plans/2026-08-03-ai1706-reviewable-mcp-config-implementation.md

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

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

Qodo Logo

@realtonyyoung

Copy link
Copy Markdown
Collaborator Author

NO FINDINGS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant