Skip to content

feat(acp): add gateway-backed ACP server - #7415

Open
twpedersen wants to merge 1 commit into
kirodotdev:mainfrom
twpedersen:feat/acp-server-port
Open

twpedersen wants to merge 1 commit into
kirodotdev:mainfrom
twpedersen:feat/acp-server-port

Conversation

@twpedersen

@twpedersen twpedersen commented Sep 1, 2026

Copy link
Copy Markdown

Implemented ACP features

  • Editor-native Kiro Crew sessions with shared history, memory, tools, titles, and dashboard synchronization.
  • Session create/list/load/resume/cancel plus standard model, reasoning, and slash-command selectors.
  • Streaming text, thinking, tool calls, structured diffs, file follow-along, and task plans.
  • Tool approvals, question forms, and single- or multi-select reply options.
  • Sandboxed per-session stdio MCP servers with isolation, ownership fencing, cleanup, and restart recovery.
  • Strict ACP v1 conformance, secure local/remote authentication, companion-aware redaction, and closed-box testing.

Problem / Motivation

Kiro Crew can consume ACP agents but cannot act as a standard ACP v1 agent for editor clients. Users therefore cannot connect an ACP-aware editor to their existing Kiro Crew sessions, controls, approvals, memory, and MCP policy boundary.

Why it matters

An ACP server gives editors the same durable sessions and gateway-managed capabilities as the dashboard instead of creating a separate agent runtime. Conversations retain Kiro Crew context, tools, approvals, security policy, title/history continuity, and channel integration while the editor provides its native chat, diff, and file-navigation experience.

What changed (motivation → approach → change)

  • kirocrew acp exposes a strict newline-framed ACP v1 JSON-RPC stdio server. It uses the running gateway by default and offers --standalone as an explicit diagnostic fallback.
  • ACP sessions map to dashboard slots, keeping history, titles, finalized messages, task plans, tools, permissions, question cards, memory, lessons, skills, and channel mirroring gateway-owned. Durable message origins prevent editor/dashboard echo loops.
  • The session surface includes initialization, create/list/load/resume, prompt streaming, cancellation, history replay, project-scoped discovery, workspace relocation, model and reasoning selectors, and slash-command advertisement.
  • Editor updates include streamed text, thinking, tool activity, structured diffs, redacted file locations, and full plan snapshots. Internal task bookkeeping does not appear as a duplicate editor tool card.
  • Standard session/set_config_option handles model selection and session/set_mode handles reasoning selection. The non-standard session/set_model extension is intentionally unsupported.
  • Standard form elicitation handles question cards and single- or multi-select reply options, including streamed follow-up turns.
  • Client-supplied stdio MCP servers run under strict sandboxing, credential scrubbing, process-tree supervision, capability-guarded local proxies, session-scoped registration, generation-owned replacement, restart restoration, and durable crash cleanup. Unsupported HTTP/SSE MCP transports fail explicitly.
  • Transport and lifecycle guarantees include a recoverable 10 MiB frame ceiling, non-blocking request dispatch, authoritative cancellation, busy-slot refusal, atomic activation rollback, idempotent mutation recovery, cross-platform pipe support, and complete child cleanup.
  • The gateway's internal credential is loopback-only. Remote gateways require HTTPS and an explicit presigned credential exchanged for a bounded session cookie; redirects and unsafe URL forms are rejected.
  • The offline conformance oracle contains the authoritative stable ACP-v1 schema, metadata, license, checksums, and provenance.

Backwards compatibility

Compatible: the ACP server is a new opt-in CLI entrypoint. Existing dashboard, messaging, provider, and ACP-client behavior remains available; optional server methods and selectors are advertised only when the selected backend implements them.

Tests

  • Focused ACP runtime/server, dashboard, session, MCP, conformance, platform, and cloud-login aggregate: 2,238 passed, 26 capability skips.
  • Stable-schema provenance and closed-box conformance are included in that aggregate; the checksum manifest is pinned to exactly the stable schema, metadata, and license.
  • Black changed-file baseline, subprocess encoding, agent-SDK boundary, isort, flake8, mypy (1,640 source files), documentation lint, and git diff --check: passed.
  • Structural push guard: clean worktree and exactly one commit directly on current origin/main.

The test surface covers strict framing and error mapping, session lifecycle and rollback, concurrent clients, selectors and commands, cancellation, history replay, dashboard/editor message and title synchronization, question-card elicitation, tool locations and plans, stdio MCP spawn/proxy/ownership/teardown, credential rotation, remote authentication, cross-platform pipes, and gateway failure recovery.

Manual verification

The closed-box gate launches the real kirocrew acp subprocess and drives it as an external editor over stdio while validating every emitted frame against the independent stable ACP-v1 oracle. A live Zed/VS Code smoke remains recommended after installing the branch; this PR adds no dashboard UI surface.

Related Issues

no linked issue: this upstreams the ACP server capability without an existing public issue.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@twpedersen
twpedersen requested a review from a team as a code owner September 1, 2026 01:11
@twpedersen
twpedersen requested a review from buluoray September 1, 2026 01:11
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 1, 2026
@twpedersen
twpedersen force-pushed the feat/acp-server-port branch 2 times, most recently from 2a4ad14 to 97b82c7 Compare September 1, 2026 18:58
@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 1, 2026
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 1, 2026
@twpedersen
twpedersen marked this pull request as draft September 2, 2026 06:20

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description / code mismatch

Three claims in the Description do not match the tree at 5ea762689: the gate results it vouches for cannot hold on this head, the new server crosses an import boundary the Description never raises, and the specification this PR adds contradicts the surface the same PR implements.

1. The new acp_server package crosses the agent-SDK import boundary, and the Description does not raise it

The Description says

Added a strict ACP v1 JSON-RPC stdio server exposed as kirocrew acp, with dashboard-backed operation by default and --standalone as an explicit isolation mode.

The code does — four of the new modules under src/kiro_crew/acp_server/ import kiro_crew.acp directly, for example src/kiro_crew/acp_server/server.py:24. scripts/check_agent_sdk_boundary.py is a shrink-only ratchet over that dependency, so these are new offenders inside the PR's own changed-file scope and the gate fails deterministically on this head; Backend Lint & Type Check (3.12) is red. (The fifth path the gate reports, src/kiro_crew/subagent_manager/terminal.py, is a baseline-drift artifact — the branch still carries the merge-base copy of the baseline, current main already lists that file, and the PR does not touch it, so the gate's changed-file scope excludes it.)

Risk — the gate documents no inline opt-out ("'this consumer legitimately needs the ACP layer' is precisely the claim the boundary exists to refuse") and its baseline as append-forbidden ("Do NOT add or raise a line to make a red gate green — route the dependency through kiro_crew.agent_sdk instead"). There is therefore no author-side fix that keeps the current import shape: landing this either reds a required check or requires a maintainer policy decision to add src/kiro_crew/acp_server/ to EXEMPT_PREFIXES — that is, to declare the agent-role server part of the boundary. The Description asks for neither.

Required change — either route the protocol constants through kiro_crew.agent_sdk, or make the exemption explicit and reviewed: add src/kiro_crew/acp_server/ to EXEMPT_PREFIXES in scripts/check_agent_sdk_boundary.py, update docs/request-for-change/rfc-crew-agent-sdk-boundary.md, and state it in the Description so the boundary change is reviewed rather than absorbed.

2. The black gate fails on this head: chat_utils.py is reformatted clean but its baseline entry is not pruned

The Description says

Black, isort, flake8, docs lint, scrub lint, vendor, brand, harness-parity, frontend build, TypeScript, ESLint, and i18n gates passed locally.

The code does — the diff reformats src/kiro_crew/dashboard/chat_utils.py whole-file (reformat-only hunks across old-side lines 164 through 2235, in a file the change otherwise only appends to), so the file is now black-clean. .github/black-baseline.txt:204 still carries its entry, so python3 scripts/check_black_formatting.py rejects the stale line. Verified with black==26.3.1, the version pinned in pyproject.toml's dev group: the head tree is clean for that file, the merge-base and main are not.

Riskcheck_black_formatting.py is the first step of Backend Lint & Type Check, so the job dies there and the isort, flake8, and mypy steps the Description also vouches for never run, which is what the red (3.12) / cancelled (3.10) pair on this head shows. A gate that fails deterministically on the head tree cannot be reconciled with "passed locally", and it leaves the mypy, flake8, and isort claims unverified by CI as well.

Required change — either run python3 scripts/check_black_formatting.py --update-baseline and commit the pruned .github/black-baseline.txt, or revert the reformat-only hunks in chat_utils.py, which is what AGENTS.md's "format only what you touched" calls for and keeps the baseline untouched. Then re-state the gate results from a run that actually reaches flake8 and mypy.

3. The spec this PR adds documents session/set_mode and session/set_config_option as not implemented, while the code implements both

The Description says

Implemented session creation, loading, listing, resumption, prompting, cancellation, model selection, and reasoning controls against current Kiro Crew provider/session APIs.

The code doesdocs/system-specs/modules/acp-server.md:74, added by this PR, lists session/set_mode and session/set_config_option as not implemented. Both are dispatched at src/kiro_crew/acp_server/server.py:551-554 and served by _handle_set_mode (from ~940) and _handle_set_config_option (~1008-1096), with selectors resolved through _selector_backend() (~599) and _selector_fields() (~643). The gateway backend implements get_session_selectors, set_session_mode, and set_session_config_option at src/kiro_crew/acp_server/http_backend.py:498, :524, and :545. The real contract is backend-gated, not absent: served when the backend exposes those three methods, -32601 otherwise, and session/set_model always -32601.

The same file's "Dependency decision" paragraph (docs/system-specs/modules/acp-server.md:443) is also stale in part: it says acp_bb_schema is built from kiro_crew.acp.types, while test/acp_bb_schema.py states it imports nothing from kiro_crew and derives every vocabulary from the vendored schema-v1.21.0 files via import acp_v1_vendor as acp. Its "no offline Draft 2020-12 validator" caveat remains accurate.

Risk — AGENTS.md makes docs/system-specs/modules/ the read-before-you-touch authority for acp_server/ and requires the spec to move in the same commit as the behaviour it documents. A method table that understates the served surface is worse than no table: the next change reads "not implemented, never no-oped", assumes a client calling session/set_config_option gets -32601, and does not know it is mutating a live dashboard slot's model. It also contradicts the Description's own claim in the same PR, so a reviewer cannot tell which is authoritative.

Required change — replace line 74 with the real contract (backend-gated selectors: served when the backend exposes get_session_selectors / set_session_mode / set_session_config_option, -32601 otherwise; session/set_model always -32601), document the selector advertisement on session/new, session/load, and session/resume plus the busy and rollback error semantics, and correct the "Dependency decision" paragraph's oracle to the vendored acp-v1 schema while keeping the no-offline-validator caveat.

@bolichen97

Copy link
Copy Markdown
Collaborator

Rebased onto main 6c24f116e by a maintainer as part of the 2026-09-08 open-PR audit. Was 854 commits behind with a merge-conflict label.

Conflicts resolved (7 files):

  • AGENTS.md, docs/system-specs/modules/README.md: kept main's rows, added this PR's acp-server.md entries.
  • acp/runtime.py: load_session now takes both main's member_session_key and this PR's mcp_servers; the editor-supplied set wins, main's member mount still applies.
  • config/loader.py: kept main's noqa, added Any. The _acp factory hunk reapplied cleanly at its new location (refactor(config): split loader responsibilities #7304 moved it).
  • dashboard/state.py: both new slot fields kept.
  • providers/acp.py: both kwargs forwarded.
  • session_allocation.py: both bypass arms kept, bypass_mcp before bypass_effort so the config-reading arm stays last.
  • dashboard/chat_handlers.py: main's remote-relay arm kept; the ACP-origin release became a wrapper taking the turn coroutine, so _run_chat( still sits inside the spawn_guarded_turn(...) body that test_chat_turn_timeout_consistency scans.

One follow-up commit adapts to main: stream_slot_response takes relay_mode/relay_owned (main added mirror detach into the loop this PR extracted), _FakeRuntime accepts member_session_key, and the credential-masking guard now pins only the gateway secret because main's launcher masks the home .env by default. Please review those.

Ran locally on changed files: black, isort, flake8, and pytest over the acp_server suites, dashboard chat, runtime, token auth and spawn audit (all pass). test_acp_conformance_blackbox.py still fails collection without agent-client-protocol installed, unchanged from before the rebase.

A maintainer push makes the maintainer the last pusher, so a second approver is needed under the repo's last-push rule. Reply if anything looks wrong.

@dwu96

dwu96 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

@twpedersen
twpedersen marked this pull request as ready for review September 8, 2026 22:38
@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ⚠️ could not complete

The design review did not produce a verdict for c1789d641c199288e3e46932a4e2a4833d08787a (the review step never ran, because an earlier step in this job failed — no model call was made). See the Fork Design Review job logs. Advisory — does not block merge.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ⚠️ review incomplete

Reviewed c1789d641c199288e3e46932a4e2a4833d08787a via the fork AI-review pipeline; updated in place on each push.

No completed Opus verdict for this commit; see the Fork Opus 4.8 Review job logs.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ⚠️ review incomplete

Reviewed c1789d641c199288e3e46932a4e2a4833d08787a via the fork AI-review pipeline; updated in place on each push.

No completed GPT verdict for this commit; see the Fork GPT 5.6 Review job logs.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — ⚠️ could not complete

The first-principles review did not produce a verdict for c1789d641c199288e3e46932a4e2a4833d08787a (the review step never ran, because an earlier step in this job failed — no model call was made). See the Fork First Principles Review job logs. Advisory — does not block merge.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running labels Sep 10, 2026
@NicholasRBowers NicholasRBowers added the needs-pr-triage PR scanner: awaiting automated triage label Sep 14, 2026
@chenmingwei23 chenmingwei23 added needs-human-security-review Security scan flagged — needs manual review and removed needs-pr-triage PR scanner: awaiting automated triage labels Sep 14, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor

Kiro Crew [operator: chenmingwei23#de330d0c]: Security scan flagged this PR for manual review.

Findings (content-based scan, layer=code):

  • test/test_acp_server_gateway.py:200 - matched destructive-command pattern: title="rm -rf /",
  • test/test_acp_server_gateway.py:203 - matched destructive-command pattern: raw_tool_params={"command": "rm -rf /"},

These occur inside an EVENT_PERMISSION_REQUEST test fixture (is_shell=True) that exercises how the ACP permission gate surfaces a destructive shell command; they are test literals, not an executed operation. Flagging for a maintainer to confirm before any automated action is taken.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Security Scope Review (fork) — 🔴 BLOCK (blocking)

Which legitimate operations does b1974796228bea8c3fde251e5c86d0e29b25de70 newly refuse? Model proposes, scripts/deny_diff.py decides — updated in place on each push. A script-confirmed regression blocks PR readiness.

no leg reported, and this was not the nothing-to-adjudicate short circuit

No [SCOPE-REVIEWED] marker for this head was produced, so the model's text is withheld: it cannot be shown to describe this revision. See the job logs.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 17, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 18, 2026
@twpedersen

twpedersen commented Sep 18, 2026

Copy link
Copy Markdown
Author

Rebased the sole ACP commit onto current main at 0da53b3aaab1; new exact head: 2a2a6fc14b00.

The three items from the outstanding review remain addressed on this head:

  1. Agent-SDK boundary: acp_server remains an explicit reviewed boundary; check_agent_sdk_boundary.py passes.
  2. Black baseline: the scoped Black gate passes with no stale changed-file entry.
  3. ACP specification: the spec documents the backend-gated selector contract, busy/rollback behavior, and stable vendored ACP-v1 schema oracle.

The PR was also reduced from 37,274 to 28,336 additions by removing the unused informational schema.unstable.json and meta.unstable.json artifacts. The authoritative stable schema, metadata, license, checksums, provenance, and closed-box oracle remain intact.

Post-rebase evidence: 2,238 focused ACP/dashboard/session/platform/cloud tests passed with 26 capability skips; Black, subprocess encoding, agent-SDK boundary, isort, flake8, mypy (1,640 files), docs lint, and diff checks passed. No local GPT/Opus review was run. Please re-review this head.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 18, 2026
Expose Kiro Crew as an ACP agent endpoint backed by the existing gateway, including editor session synchronization, selector handling, tool locations, title updates, and session-scoped MCP servers.

Keep direct ACP protocol imports inside the explicit protocol-boundary packages, document the exception and wire behavior, and cover the endpoint with vendored-schema, black-box, integration, and dashboard regressions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

allow-fork-workflow-change fork Pull request from a fork (external contributor) merge conflict Branch has merge conflicts with its base — author must resolve before merge needs-human-security-review Security scan flagged — needs manual review readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants