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

feat(acp): add gateway-backed ACP server#7415
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

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)

  • Added a strict ACP v1 JSON-RPC stdio server exposed as kirocrew acp, backed by the existing gateway by default with --standalone as an explicit isolation mode.
  • Implemented session creation, loading, listing, resumption, prompting, cancellation, backend-gated mode/config selectors, provider-aware slash commands, title synchronization, plan/message follow-along, and structured tool locations.
  • Added editor-supplied stdio MCP validation, sandboxed child supervision, capability-authenticated proxying, owner/internal-only slot registration, and session-scoped MCP recreation when editor configuration changes.
  • Added explicit remote-gateway credential requirements, loopback-only local-secret use, recoverable frame limits, stable gateway tool-call identities, and Windows-safe thread-backed stdio with bounded reader backpressure.
  • Vendored the pinned ACP v1 schema with LF-normalized checkout bytes and added independent closed-box subprocess validation plus an official SDK interoperability smoke test.
  • Rebased the complete change onto current main at efb9fc4ba0f2 and squashed it to one commit.
  • Addressed the requested review changes explicitly:
    1. Classified 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.
    2. Pruned the stale Black entries and regenerated the affected policy/docs baselines.
    3. Corrected the ACP server specification to document backend-gated selector behavior, busy/rollback semantics, session/set_model as unsupported, and the vendored test/conformance/vendor/acp-v1 schema as the wire oracle.
  • Addressed local GPT/Opus review findings by bounding Windows stdin buffering, preserving retryable question cards on busy streaming turns, restricting cancellation to locally owned sessions, binding each elicitation completion to its own registry key, and retaining title-subscription refresh tasks through completion.
  • Made Unix-connector failure-path mocks portable to Windows and ensured the stdout-only pipe test closes its unused stdin writer before threaded output, preventing executor starvation on the Windows shard.

Tests

Post-rebase validation on commit fd125e6bbf13665c8c79910d88c8ab39640be139:

  • Expanded ACP and hook-contract set: 2,756 passed, 5 expected skips.
  • Focused ACP CLI/MCP proxy Windows remediation set: 25 passed.
  • Focused review-remediation set: 159 passed; focused coverage set: 169 passed.
  • Per-file coverage for the files that failed the earlier coverage gate: mcp_proxy.py 98%, cli_acp.py 83%, ask_question.py 87%.
  • Independent GPT and Opus fallback reviews completed against pre-fix candidate 234e70e362ad; focused Opus verification confirmed all five findings closed between 234e70e362ad and 32a4e92572fb. Range-diff review confirms the latest rebase changed only the mechanical comment-history total plus the required diff_path hook-provenance forwarding and its integration assertion.
  • Mypy: 1,392 source files clean.
  • Black baseline, subprocess encoding, agent-SDK boundary, comment-history ratchet, sync-I/O, lockdown-before-publish, isort, flake8, docs lint, harness parity, loop-bound locks, builtin-skill scope, testpaths coverage, changelog history, focus cue, vendor integrity, and brand gates: passed on the exact rebased candidate.
  • Push guard: one commit directly on current origin/main.

Earlier validation retained by the rebased single commit:

  • Final CI-fix regression sets: 538 passed plus 203 passed, 2 expected skips, covering Windows stdio, tool-location identity, MCP supervision, gateway selectors, vendored schema integrity, and context-aware log redaction.
  • Earlier complete affected ACP server/session-MCP/dashboard/websocket/boundary/metrics aggregate: 2,142 passed, 2 expected skips.
  • Frontend: all 4 canonical Vitest shards passed; Electron shell tests, TypeScript (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_path hook-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 clean origin/main: the mandated scratch directory is below the protected real ~/.kiro tree, and /local/home ownership 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 acp stdio 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

  • 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
@twpedersen

Copy link
Copy Markdown
Author

Updated at 17b4c52775856cfe674d8ad9525e7e96e35aad72, rebased onto main 6c24f116e8fb and squashed to one commit.

Reviewer disposition:

  1. Agent-SDK boundary — fixed. src/kiro_crew/acp_server/ is now an explicit, narrowly pinned protocol-boundary exemption. The gate rationale and RFC are updated, and test_agent_sdk_boundary.py pins the exact four-tree exemption set so it cannot widen silently.
  2. Black baseline — fixed. The stale entries were pruned and the Black gate now passes on the complete diff. The affected docs and boundary baselines were also regenerated/pruned.
  3. ACP specification — fixed. The method table and selector section now describe backend-gated session/set_mode / session/set_config_option, prompt-vs-selector busy rejection, rollback-on-failure behavior, and session/set_model always returning -32601. The conformance section now names the vendored test/conformance/vendor/acp-v1 schema as the independent wire oracle and describes the official SDK smoke test separately.

The maintainer rebase follow-up from c1789d641c19 was compared before the force-push. Its remote relay ownership, timeout-dispatch shape, current member_session_key test compatibility, and credential-mask behavior are preserved; the final branch adds the reviewer and self-review fixes above.

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.

@twpedersen

Copy link
Copy Markdown
Author

CI follow-up published at f236d6ecbbad7a902556cdde3d3d1d2b23992a8a on current main c190fbe5a6e7.

  • Cross-platform portability: replaced direct process-group and directory permission primitives with platform_compat seams and added focused assertions.
  • Inclusive language: renamed the new conformance surface from black-box to closed-box, including the test file and xdist group, and removed the flagged restating comments.
  • Semgrep/security posture: removed the credential-looking log phrase, routed owner-only directories through the fail-loud helper, and changed child-stderr logging to the companion-aware redactor.
  • Comment history: rewrote new historical narration in present tense and ratcheted the baseline down.
  • Windows backend: added thread-backed stdio for synchronous Windows standard handles, preserved POSIX protocol path identity on Windows, marked the Unix-socket MCP proxy test POSIX-only, and pinned vendored schema bytes to LF.

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.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 9, 2026
@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 9, 2026
@twpedersen

Copy link
Copy Markdown
Author

Review-remediation follow-up published at 84c15ba76832677ad129f27a0bcc8779935cb300, one commit on main ab3a5ac8d191a23d3f1e4c2cf005ce649be66e2d.

Validation on the exact rebased head:

  • expanded ACP regression set: 341 passed
  • focused coverage: mcp_proxy.py 98%, cli_acp.py 83%, ask_question.py 87%
  • Black, subprocess encoding, agent-SDK boundary, comment-history, isort, flake8, mypy (1,354 files), docs lint, harness parity, and brand gates: passed
  • independent GPT + Opus fallback reviews completed; focused Opus verifier found all five remediations closed with no residual Critical/High finding
  • push guard: safe, single commit on current base

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.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 9, 2026
@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 readiness: checking Automated validation is still running labels Sep 9, 2026
@twpedersen

Copy link
Copy Markdown
Author

Windows CI remediation and fresh rebase published at 5d747810b83f82976bab50bef250784e0cb952f8, one commit on main 8113a5ada032e4b1294071a55aded69e1aef34a4.

Exact-head validation:

  • ACP regression set: 341 passed
  • focused CLI/MCP proxy set: 25 passed
  • mypy: 1,392 source files clean
  • formatting, lint, agent-SDK boundary, comment-history, sync-I/O, lockdown, docs, harness parity, loop-lock, testpaths, changelog, focus, vendor, and brand gates passed
  • range-diff against the previously reviewed head contains only the mechanical baseline total and two Windows test fixes
  • push guard: safe, one commit on current base

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.

@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Sep 9, 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.
@twpedersen

Copy link
Copy Markdown
Author

Republished after rebasing onto current main (efb9fc4ba0f2584fd9d4cdcfa307edf596bcc23a).

Exact head: fd125e6bbf13665c8c79910d88c8ab39640be139

Validation on this head:

  • ACP + hook-contract suite: 2,756 passed, 5 expected skips
  • Focused post-rebase gateway/protocol/integration set: 121 passed
  • Mypy: 1,392 source files clean
  • Black, isort, flake8, subprocess encoding, agent-SDK boundary, sync-I/O, lockdown, comment-history, vendor, docs, CloudFormation, and strict no-CRalph gates passed
  • Frontend: all four canonical Vitest shards, Electron shell tests, TypeScript/build, lint/i18n, duplication, and bundle-size gates passed
  • Push guard: one commit directly on current base

The rebase-owned upstream contract change is handled by forwarding the ACP event diff_path into the security hook and pinning that provenance in the full-stack integration test. Range-diff otherwise shows only the mechanical comment-history total update.

@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants