feat(acp): add gateway-backed ACP server - #7415
twpedersen wants to merge 1 commit into
Conversation
1ee3dbd to
de0ec82
Compare
2a4ad14 to
97b82c7
Compare
0a1de29 to
5ea7626
Compare
bolichen97
left a comment
There was a problem hiding this comment.
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--standaloneas 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.
Risk — check_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 does — docs/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.
1c8afcf to
c1789d6
Compare
|
Rebased onto main Conflicts resolved (7 files):
One follow-up commit adapts to main: 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). 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. |
|
👋 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. |
Design Review (Fable 5, fork) —
|
Opus 4.8 Review (fork) —
|
GPT 5.6 Review (fork) —
|
First Principles Review (Fable 5, fork) —
|
c1789d6 to
17b4c52
Compare
|
Kiro Crew [operator: chenmingwei23#de330d0c]: Security scan flagged this PR for manual review. Findings (content-based scan, layer=code):
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. |
d365f5c to
b197479
Compare
Security Scope Review (fork) — 🔴 BLOCK (blocking)Which legitimate operations does no leg reported, and this was not the nothing-to-adjudicate short circuit No |
b197479 to
998d2da
Compare
998d2da to
9a50846
Compare
9a50846 to
2dc193f
Compare
|
Rebased the sole ACP commit onto current The three items from the outstanding review remain addressed on this head:
The PR was also reduced from 37,274 to 28,336 additions by removing the unused informational 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. |
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.
Implemented ACP features
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 acpexposes a strict newline-framed ACP v1 JSON-RPC stdio server. It uses the running gateway by default and offers--standaloneas an explicit diagnostic fallback.session/set_config_optionhandles model selection andsession/set_modehandles reasoning selection. The non-standardsession/set_modelextension is intentionally unsupported.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
git diff --check: passed.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 acpsubprocess 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
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)