feat(acp): add gateway-backed ACP server - #7415
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
|
Updated at Reviewer disposition:
The maintainer rebase follow-up from Post-rebase validation: 2,142 affected backend tests passed with 2 expected skips; TypeScript passed; Vitest passed 30,599 tests with 1 expected failure and 2 skips; Black, subprocess encoding, agent-SDK boundary, isort, flake8, mypy (1,351 files), docs, harness parity, and brand gates passed. |
17b4c52 to
f236d6e
Compare
|
CI follow-up published at
Final local validation: 538 affected tests passed; an additional Windows/security/conformance set passed 203 tests with 2 expected skips; Black, subprocess encoding, boundary, comment-history, isort, flake8, mypy (1,354 files), docs, harness parity, and brand gates passed. Push guard confirmed one commit directly on current main. |
f236d6e to
84c15ba
Compare
|
Review-remediation follow-up published at Validation on the exact rebased head:
The fork GPT/Opus workflows remain a deterministic infrastructure blocker: the authentic diff is 1,366,663 bytes, above their 1 MB pre-model cap. They fail before model invocation and require maintainer workflow action or override; required vendored protocol data was not removed to evade the cap. |
84c15ba to
5d74781
Compare
|
Windows CI remediation and fresh rebase published at Exact-head validation:
Windows changes: failure-path mocks may install the Unix connector on platforms where asyncio does not define it, and the stdout-only pipe test closes unused stdin before starting threaded output so a blocked reader cannot starve the write. |
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.
5d74781 to
fd125e6
Compare
|
Republished after rebasing onto current Exact head: Validation on this head:
The rebase-owned upstream contract change is handled by forwarding the ACP event |
Problem / Motivation
Kiro Crew can consume ACP agents but cannot currently 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, and MCP policy boundary.
Why it matters
An ACP server lets editor conversations share the same durable sessions and security controls as the dashboard while preserving an explicit standalone option for isolated operation. Without it, editor integration requires a separate runtime and loses Kiro Crew session continuity.
What changed (motivation → approach → change)
kirocrew acp, backed by the existing gateway by default with--standaloneas an explicit isolation mode.mainatefb9fc4ba0f2and squashed it to one commit.acp_server/as a tightly pinned protocol-boundary implementation in the agent-SDK import gate, documented the rationale in the RFC, and pinned the exact exemption set in tests.session/set_modelas unsupported, and the vendoredtest/conformance/vendor/acp-v1schema as the wire oracle.Tests
Post-rebase validation on commit
fd125e6bbf13665c8c79910d88c8ab39640be139:mcp_proxy.py98%,cli_acp.py83%,ask_question.py87%.234e70e362ad; focused Opus verification confirmed all five findings closed between234e70e362adand32a4e92572fb. Range-diff review confirms the latest rebase changed only the mechanical comment-history total plus the requireddiff_pathhook-provenance forwarding and its integration assertion.origin/main.Earlier validation retained by the rebased single commit:
tsc -b), production build, ESLint, i18n, phantom-class, duplication, and bundle-size gates passed. The final fixes touch no frontend source.A full backend suite attempt completed with 93,317 passed, 336 skipped, 9 xfailed, and 67 failed. Its one branch-owned failure was the new upstream
diff_pathhook-provenance contract; that call site and integration assertion were fixed and the exact focused set now passes. The remaining clusters are host-environment failures already reproduced on cleanorigin/main: the mandated scratch directory is below the protected real~/.kirotree, and/local/homeownership breaks provider-executable hierarchy tests. The PR does not alter those current-main host failures.The fork GPT/Opus workflows fail closed before model invocation because the authentic diff is 1,366,663 bytes, above their 1 MB workflow cap. Local model-pinned review was used for code remediation, but the workflow result requires maintainer override or workflow action.
Manual verification
The closed-box tests launch the real
kirocrew acpstdio adapter and exercise framing, initialization, sessions, selectors, prompts, cancellation, tool updates, and failure handling through an independent editor harness. No additional dashboard UI is introduced; live editor smoke testing remains appropriate after CI installs the branch in an editor environment.Related Issues
no linked issue: this ports the reviewed ACP server capability without an existing public issue.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)