Skip to content

feat(acp): author any ACP provider as config data and serve it generically - #7984

Closed
billygerhard wants to merge 1 commit into
kirodotdev:mainfrom
billygerhard:feat/acp-providers
Closed

feat(acp): author any ACP provider as config data and serve it generically#7984
billygerhard wants to merge 1 commit into
kirodotdev:mainfrom
billygerhard:feat/acp-providers

Conversation

@billygerhard

@billygerhard billygerhard commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

KiroCrew can drive multiple AI providers today, but every provider is a hand-written code path: Kiro CLI, KAS, and Claude Code each have bespoke spawn logic, capability handling, and UI wiring, and Codex is being hand-built the same way. An operator who wants to run any other ACP-speaking provider — pi, goose, opencode, or an in-house ACP adapter — cannot: there is no way to describe a provider to KiroCrew without writing and shipping a new backend.

Why it matters

Every new provider currently costs a full per-backend PR (the open Codex backend PR is 61 files). The ACP conversation is identical across providers — only the launch command line differs — so that cost is accidental, not essential. Config-authored providers make "run provider X" an operator decision instead of a maintainer roadmap item, which is exactly the shape needed for the stated pi/goose/opencode direction.

What changed (motivation → approach → change)

Goal: author ANY ACP provider as configuration data and serve it through one generic path, without weakening the per-harness security posture.

Approach: make the harness descriptor the single source of truth. A descriptor carries the launch recipe (executable, argv template with {executable}/{agent}/{model}/{workdir} placeholders only), an opt-in CapabilitySet (everything defaults off, fail-closed), a model_source (ACP-advertised or static), and an mcp_delivery mode (wire_fed/file_fed). Bundled providers (Kiro CLI, KAS, Claude Code, the dormant Codex seam) are expressed as bundled descriptors; operators add more under agent.harnesses in config.json — a map keyed by harness id, validated per entry with a diagnosable reason (one bad entry never costs the others), no restart required, and no per-provider code.

What was built on that:

  • Descriptor + registry (acp/harness_descriptor.py, acp/harness_registry.py): parsing/validation with per-field reasons; alias resolution for the legacy agent.acp_backend key; availability probing. Four capabilities (internal_sandbox, acp_runtime_pool, session_sharing, kiro_identity_store) are code-only — honoured by machinery written for specific bundled harnesses — and are refused from configuration with a written reason (internal_sandbox would otherwise let a config entry waive KiroCrew's own OS sandbox).
  • Generic serving path (acp/harness_adapters.py, protocol profiles): a GenericAdapter speaks the standard ACP wire to any descriptor-launched binary; ProtocolProfile captures per-harness wire dialects (kiro-cli's date-string protocolVersion vs the public integer, session/set_config_option vs /effort), replacing scattered is_claude-style branches.
  • Session binding (acp/harness_selection.py, session_allocation.py, subagent_manager/admission.py): a HarnessBinding resolves once per session and threads through chat, spawn (per-subagent), and cron paths; the warm pool bypasses on harness mismatch; MCP delivery follows the bound descriptor's declared mode, gated on the transports the harness advertised at initialize.
  • Selection surfaces: harness picker on the new-chat surface, a Settings panel listing every registered harness with Available/Unavailable/Invalid state and the per-entry reason, a read-only harness chip in the composer shelf and session header, and harness attribution on subagent listings. Invalid or unavailable entries render visible-and-unselectable with their reason rather than disappearing.
  • Reconciliation with parallel upstream work: merged the recent capability-set leaf placement and the dormant Codex seam; the leaf sets and the descriptors' grants are pinned in agreement by test, and the bundled Codex descriptor now launches the codex-acp adapter (the codex CLI alone does not serve ACP, per the seam's own spawn path).
  • The gateway now pins its bound port into managed MCP children's env (KIROCREW_PORT beside the existing KIROCREW_HOME), fixing spawn callbacks on gateways bound to non-default ports whose hosts fail the run-marker ownership proof closed.

Deletions: website/src/pages/developer/AgentBackendTab.tsx and its test are replaced by the Settings HarnessPanel (server-derived rows, per-row reasons). config-baseline.json is regenerated for the new agent.harnesses/agent.default_harness fields. website/package-lock.json churn is transitive-only (no package.json change).

Tests

  • Descriptor validation: per-field refusal reasons, strict-bool capability values, code-only capabilities refused from config (parameterized over all four, including refusal of explicit false), operator-grantable flags still parse, argv placeholder allowlist.
  • Registry: alias resolution totality, invalid entries served as diagnosable rows, Codex descriptor pinned to the codex-acp adapter shape, capability grants pinned against the leaf ACP_BACKENDS_* sets and shipped literals (three-way agreement test).
  • Generic serving end-to-end: a stub ACP harness driven through real ensure_ready()/_spawn — full chat turn, permission round-trips, model advertisement, sandbox wrap decision, spawn/cron binding resolution, and the bound descriptor winning MCP delivery over the backend roster (regression for a review finding where operator sessions silently got zero MCP servers).
  • Harness parity gate (scripts/check_harness_parity.py + tests): added lines may not test harness identity by negation or bare literal; golden kiro argv byte-identity pinned against the pre-migration formula.
  • Spawn/session binding, chip rendering (frameless + framed composer), settings surface, and the port-pin env tests.

Manual verification

Exercised hands-on in a pod: all three bundled providers serve real sessions; a config-authored stub provider (agent.harnesses entry) appears in the picker, serves an echo session, and advertises its own models; an invalid descriptor renders in the picker with its per-field reason; the composer chip shows the session's harness. The doc example was validated end-to-end (a wrong mcp_delivery value in the shipped example was caught this way and is now pinned by a test that validates every doc example against the real parser).

Screenshots / video

Harness picker with bundled, unavailable, and invalid rows

Composer shelf harness chip

Settings — registered harnesses panel

Settings harness panel

Related Issues

no linked issue: this implements the generic provider-authoring path discussed alongside the per-provider backends (e.g. the open Codex backend PR #5349 becomes an optional refinement rather than a prerequisite for running Codex).

Pattern harvest

Rule candidate: review-prompt
Pattern: "a capability flag parseable from operator config must be provably honoured by code that works for arbitrary processes — a flag honoured only by machinery built for one bundled binary is code-only and the parser must refuse it."

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

@billygerhard
billygerhard requested a review from a team September 2, 2026 20:26
@billygerhard
billygerhard requested a review from a team as a code owner September 2, 2026 20:26
@billygerhard
billygerhard requested a review from cixuuz September 2, 2026 20:26
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026
@billygerhard
billygerhard force-pushed the feat/acp-providers branch 2 times, most recently from 913ecf6 to cf484d5 Compare September 2, 2026 23:39
@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 3, 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 3, 2026
@billygerhard
billygerhard force-pushed the feat/acp-providers branch 2 times, most recently from be309c0 to ce50f70 Compare September 3, 2026 02:22
@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 3, 2026
@github-actions

github-actions Bot commented Sep 3, 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 35aeea6edb3d5511c80cc5e2632bfd9f0c7b2bcb (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 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ⚠️ review incomplete

Reviewed 35aeea6edb3d5511c80cc5e2632bfd9f0c7b2bcb 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 3, 2026

Copy link
Copy Markdown
Contributor

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

The design review did not produce a verdict for 35aeea6edb3d5511c80cc5e2632bfd9f0c7b2bcb (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 3, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ⚠️ review incomplete

Reviewed 35aeea6edb3d5511c80cc5e2632bfd9f0c7b2bcb 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 added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 3, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 3, 2026
…cally

Operator-authored harnesses: agent.harnesses config descriptors (executable, argv template, capabilities, model_source, mcp_delivery) served through a generic ACP adapter with protocol profiles, harness-bound sessions across chat/spawn/cron, per-harness MCP delivery (file-fed/wire-fed), selection surfaces with per-harness unavailability reasons, and a composer/session harness chip.
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed 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 labels Sep 3, 2026
@billygerhard

Copy link
Copy Markdown
Contributor Author

Closing in favor of a 3-PR stack: this PR's 3.3 MB diff exceeds the fork AI-review lanes' 1 MB fail-closed cap, so PR Readiness can never pass here regardless of CI state (all deterministic checks were green as of the last push). The same work, unchanged in substance, now ships as three sequential PRs each under the cap — part 1 (core descriptors/registry/selection) is open at #8255; parts 2 (consumer wiring) and 3 (UI + docs) follow as each predecessor merges. Thanks to the maintainers for the earlier CI iterations here — those fixes all carry over into the stack.

@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 3, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant