feat(acp): select an ACP interface per crew

[benwart-consensus]

Problem / Motivation

Every crew in a gateway is locked to the same ACP backend. create_provider_factory
reads agent.acp_backend once and closes over it, so there is no way to run one
crew on kiro-cli and another on a different harness — a local model behind an ACP
adapter, or a second vendor's agent binary. Adding a harness at all meant editing
ACP_BACKENDS_SELECTABLE, a closed enum in the product's own source, which is the
wrong shape for something an operator supplies rather than something Kiro Crew ships.

Why it matters

An operator who wants part of their fleet on a local model today has exactly one
option: point KIROCREW_KIRO_BIN at a substitute binary, which replaces the
backend for every session in the gateway. That is all-or-nothing — it takes
the operator's own interactive chat down onto the substitute along with the
background work they actually wanted to move.

The mixed case is the useful one and is currently unreachable: an interactive
crew on kiro-cli during the day, and unattended crews on a cheaper or local
harness, in one gateway at the same time. It also means anyone evaluating a new
ACP harness has to patch the product to try it.

What changed (motivation → approach → change)

Goal: let a crew choose its harness, without making kiro-cli users configure
anything and without turning the backend list into operator-editable code.

Approach: separate the two ideas acp_backend was carrying at once.

• An interface is what an operator selects — a name, a command, an env.
  Open-ended, because the set of harnesses someone might run is not knowable.
• A backend stays what the code speaks — a closed set of protocol dialects
  the product must actually understand.

The alternative considered was extending ACP_BACKENDS_SELECTABLE with one id
per harness. Rejected: every new harness would then need a product change, and
the id would have to carry launch details the enum has nowhere to put. A single
ACP_BACKEND_EXTERNAL dialect plus named interfaces in config keeps the dialect
set closed while leaving the harness list open.

What was built:

• ACP_BACKEND_EXTERNAL for an operator-declared harness, plus the interface
  vocabulary and the built-in name map (acp/types.py).
• AcpInterfaceConfig and a new acp_interfaces config section, and
  resolve_acp_interface() implementing the resolution order: the crew's
  acp_interface → the global agent.acp_interface → the pre-existing
  agent.acp_backend → kiro-cli.
• The interface is resolved per factory call rather than captured at
  factory-build time. That is the actual fix: a value closed over once binds
  every crew together, which is the bug.
• The launch argv threads through AcpProvider into AcpClient._spawn, and is
  inherited alongside the backend id for subagents.
• acp_interface accepted on the crew create/update API.

{
  "acp_interfaces": {
    "lmstudio": {
      "command": "/Users/you/bin/acp-lmstudio",
      "args": ["acp", "--agent", "{agent}"],
      "env": { "LMSTUDIO_MODEL": "qwen3-coder" }
    }
  },
  "agents": {
    "day":   { "kiro_agent": "kirocrew" },
    "night": { "kiro_agent": "kirocrew", "acp_interface": "lmstudio" }
  }
}

kiro-cli and kas are built in, need no configuration, and cannot be
redefined, so an install that touches none of this behaves exactly as before.

Security posture is opt-out by construction. The capability sets in
acp/types.py are opt-in, and an external harness has demonstrated nothing:

• ACP_BACKENDS_INTERNAL_SANDBOX excludes it deliberately. That membership
  is what makes wrap_argv skip Kiro Crew's seatbelt in deference to a
  harness's own internal sandbox. An operator-supplied command has no such
  sandbox, so it is precisely the case that must keep Crew's. The comment says
  so explicitly, because the tempting "fix" for a harness that dislikes the
  sandbox is to add it to that set.
• Kiro's API key is stripped — already true for non-kiro backends via
  _resolve_spawn_env(kiro_api_key=self._is_kiro).
• No session sharing, no mid-turn steer, no kiro identity store.

One latent bug found and fixed on the way. Three sites gated on
not is_claude_backend, so any backend that is not claude inherited kiro-cli's
own machinery by default: _apply_effort_overlay and
_apply_tool_search_overlay (both write kiro-cli's cli.json) and
change_effort (sends the /effort slash command). An external harness would
have been handed a settings file it never reads and a slash command it would be
right to reject. Re-stated as positive kiro-family membership per the repo's own
harness-parity H5 convention — behaviour-identical for kiro, kas and claude.

Failure modes are deliberately early, because each alternative surfaces as a
dead session on a crew's first message instead:

Mistake	Where it surfaces
interface with no command	dropped at config load, with a warning
entry named kiro-cli	refused at load — a redefined built-in would silently move every unconfigured crew onto an operator's command
external backend with no argv	ValueError at provider construction
command is not an executable file	AcpError at spawn — checked, never executed, so operator-supplied code does not run before the sandbox wraps it
unknown interface name on a crew	degrades to kiro-cli with a warning — a typo in one binding must not take the install down

Tests

tests/test_acp_interfaces.py, 24 tests:

• Parse guards — an entry with no command is dropped; a built-in name
  cannot be shadowed; non-string args / env collapse rather than crashing the
  load (config.json is hand-editable and agent-writable).
• Resolution — all four tiers in order, including that the pre-existing
  agent.acp_backend = "kas" still resolves to KAS so an existing install is not
  silently re-pointed; two crews in one config resolving to different backends;
  {agent} / {model} substitution; an unknown placeholder passing through
  instead of failing the launch; an unknown interface name degrading to kiro-cli;
  config round-trip preserving the binding.
• Capability membership — parametrized over all five sets, asserting
  ACP_BACKEND_EXTERNAL is in none of them. INTERNAL_SANDBOX is the one that
  matters: this test is what fails if someone later adds external to it and
  unconfines every external harness.
• Guards — the provider refusing an external backend with no command, and the
  launchability check rejecting a non-executable, a missing path, an empty file
  and a directory.
• Subagent inheritance — the backend id and its argv travel together, so a
  subagent of an external-backed parent does not inherit a backend with nothing
  to launch.

Manual verification

Verified end to end against a local ACP adapter over LM Studio, driving the
real AcpClient rather than a stub — config declares the interface, the
resolver produces the argv, the product's own client speaks ACP to it:

crew 'day'   -> interface='kiro-cli' backend=''
crew 'night' -> interface='lmstudio' backend='external'
night argv   -> ['/…/acp-lmstudio', 'acp', '--agent', 'kirocrew']
handshake OK  session='lmstudio-1' backend='external'
assistant text: 'interface seam works.'

Repeated with sandbox_mode="auto": the session still works and the
running without OS-level confinement warning is absent, confirming Crew's
sandbox wraps the external harness rather than being skipped.

Caveat on the automated suite: I could not run pytest in this checkout —
.venv is unprovisioned and make backend was out of scope for this change — so
the same assertions were executed directly against the source tree (21 checks,
all passing, re-run after the rebase onto f99b11e04 to confirm upstream's new
effort-drop warning survived the merge resolution). Reviewers with a provisioned
venv should run pytest tests/test_acp_interfaces.py; CI will be the first to
execute the committed module.

Known limitation

Readiness is still computed from kiro-cli's own --version / whoami probes and
is not per-interface, so an install with no working kiro-cli reports not-ready
even if every crew runs elsewhere. KiroPrerequisiteService is a process-level
singleton that probes one binary and gates all session readiness; making it
interface-aware is a larger change, so it is documented in the guide and called
out here rather than half-done.

Documentation

docs/guides/acp-interfaces.md — the two nouns, the config schema, the
resolution order, the ACP surface an external harness must implement, and a
security section including the thing operators are most likely to get wrong:
tool gating runs on the session/request_permission path, so a harness that
executes tools without asking bypasses the deny engine — not because the floor
failed, but because it was never consulted.

Why no screenshot: backend and config change only; the new acp_interface
field renders through the existing reflection-derived settings form with no
layout or component change.

Related Issues

no linked issue: this implements a capability that had no filed request.

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

[dwu96]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[benwart-consensus]

Thanks — both template checks were correct, and one of them caught a second problem.

• ## Problem / Motivation, ## Why it matters, ## What changed, ## Tests missing — fixed in 9ab5c3d4d. The original body was composed from my own section names (## Problem, ## Approach, ## Security) rather than from .github/PULL_REQUEST_TEMPLATE.md. Rewritten on the template as the literal scaffold, so the headings now match exactly what the auto-approval check greps for. Also added the ## Manual verification, ## Related Issues and ## Checklist sections the original omitted, a <!-- no-visual-delta --> marker with justification (backend + config only; the new field renders through the existing reflection-derived settings form), and a no linked issue: line since this implements a capability with no filed request.

• Also fixed while in here: the PR had gone CONFLICTING against main. Rebased onto f99b11e04 and resolved one conflict in config/loader.py, where feat(loader): warn at the effort gate when a level is dropped (#6186) #6239's new effort-drop warning and this PR's interface resolution both land in the same part of create_provider_factory. Both are kept — the elif warning branch is upstream's and unchanged, with the interface resolution after it. Re-ran the feature's assertions after the rebase, including an explicit check that the effort-drop warning survived the resolution, so the conflict fix is not taken on trust. Still one commit.

No code change was needed for the template finding itself — it was a description defect, not a diff defect.

[benwart-consensus]

CI round on 9d8dd8d27, now pushed as 80256b73c.

• Harness Parity Gate — fixed (the interesting one). The gate was right: two lines I added spelled harness identity as the absence of another harness — not self.is_acp_runtime_backend and not self.is_claude_backend in change_effort, and not self.is_claude_backend in _apply_initial_effort. Correct with three backends, and then it hands the fourth whatever it guards. Added an is_external_backend predicate alongside the existing is_kiro_backend (same rationale, stated in its docstring) and restructured _apply_initial_effort so the claude push is scoped if self.is_claude_backend: rather than early-returning on a negation. scripts/check_harness_parity.py now passes against the base, self-test included.

  Also added test_harness_identity_is_tested_positively, because the gate only inspects lines a change adds — without a test, a later refactor could reintroduce the negation on a line the gate no longer treats as new.

• Backend Lint & Type Check (3.10, 3.12) — fixed. config/loader.py was not black-formatted (one call I had wrapped that black joins). Reformatted with the pinned black==26.3.1. I deliberately reverted black's changes to dashboard/handlers/agents.py and providers/acp.py: those hits are all pre-existing lines, and since this repo's black gate is baselined to a change's own diff, reformatting them would widen the PR for no reason. My own added lines are clean.

• Backend Tests (3.10, 3) and Backend Tests (Windows) (3) — pre-existing on the base, not from this PR. The CI run for f99b11e04 on main (this PR's merge base, run 33113629543) fails those same two shards plus Coverage Gate. The annotations are asyncio teardown Event loop is closed errors; this PR adds no async tests and no event-loop handling. Backend Tests (Windows) (1) reported the same error and is not in main's failing set, so I am watching whether it reproduces on this push rather than assuming either way.

• Merge conflict — fixed in the prior push: rebased onto f99b11e04, one conflict in create_provider_factory where feat(loader): warn at the effort gate when a level is dropped (#6186) #6239's new effort-drop warning and this PR's interface resolution land adjacently. Both kept, upstream's elif branch unchanged, and re-verified afterwards that the warning survived the resolution rather than taking the merge on trust.

Apologies for the mangled earlier comment — a shell quoting error on my end, superseded by this one.

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of c08364a052b7a8d1d56ca9c06458101479ef2272 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have enough to assess. Confirmed: the external harness spawn goes through wrap_argv(..., is_kiro_cli=False), so Crew's OS sandbox does wrap it; and config.json is agent-writable (security.py:5053-5054 explicitly notes echo x > config.json is not blocked, which is why other execution/authorization controls are deliberately kept in keystone files outside it).

Design assessment: real problem, right shape (interface-vs-backend split), additive/reversible, opt-in with kiro-cli unchanged, and unusually thorough security reasoning already in the diff (INTERNAL_SANDBOX exclusion, API-key strip, realpath, pool-mismatch bypass). One design-level interaction isn't reconciled anywhere in the PR: this places an exec'd command into agent-writable config.json, whereas the established pattern moves execution/authorization controls to keystone files precisely because that file is agent-writable.

Design-Verdict: CONCERNS

Sound, well-reasoned per-crew harness feature; one unaddressed boundary — an exec'd command now lives in agent-writable config.json.

Watch

• acp_interfaces.<name>.command + agents.<crew>.acp_interface both live in config.json, which security.py:5053-5054 documents as writable by any auto-approved agent shell (the reason computer_use.json, autonomy policy, etc. were moved to keystone files). A prompt-injected agent could declare an interface and bind a crew to it; on the next top-level spawn the gateway execs that command, and a harness that never calls session/request_permission (documented as "the harness's responsibility to invoke") runs tool calls that skip Crew's deny engine and approval gate — bounded only by the OS sandbox that wrap_argv still applies. The sandbox floor and the "declaring an interface is a deliberate operator trust grant" framing meaningfully limit this (it's not the unsandboxed ~/.kiro/agents mcpServers vector), so it's CONCERNS not BLOCK — but the PR reasons about the external harness as the trust boundary and never about config.json's own writability being the self-grant path. A human security reviewer should confirm the sandbox floor is sufficient given the request_permission/deny-engine bypass, or move the interface command out of agent-writable config.

[DESIGN-REVIEWED] c08364a

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of c08364a052b7a8d1d56ca9c06458101479ef2272 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification checks against the base tree are done. Here is the review.

First-Principles-Verdict: CONCERNS

The feature earns its place, but three shipped surfaces document a global agent.acp_interface tier and a description field this diff deliberately does not implement.

What this change ships

Intent: let an operator run some crews on a different ACP harness while the rest stay on kiro-cli — an ADDITION.

1. Crews bindable to a named harness via agents.*.acp_interface — justified
2. New acp_interfaces config section (command/args/env per harness) — justified
3. Bad entries refused at load (no/relative command, built-in shadowing) — justified
4. Unknown interface name degrades to kiro-cli with a warning — justified
5. Crew create/update API accepts acp_interface — justified
6. External harness keeps Crew's sandbox, no kiro capabilities, key stripped — derived (H5/H7)
7. Effort/tool-search sites converted from claude-negation to positive membership — derived (H5)
8. Warm pool bypassed on interface mismatch (bypass_interface) — justified
9. Subagents inherit the parent's launch argv with the backend id — justified
10. Help text and guide describe a global agent.acp_interface tier — undeclared, phantom

Watch

• The description promises a four-tier resolution ("the crew's acp_interface → the global agent.acp_interface → …") while resolve_acp_interface implements three and test_there_is_no_global_interface_tier pins the global tier's absence. The subtraction was right; the description and shipped docs were not updated to match it.
• Grep for agent.acp_interface in the diff: 3 operator-facing surfaces advertise it — the per-crew field help in config/loader.py ("Empty inherits the global agent.acp_interface"), the generated config-baseline.json copy of the same text, and step 2 of the guide's resolution list. An operator who sets it is silently ignored — the exact half-wired-tier failure the pinning test names.

Subtractions

• Delete the phantom agent.acp_interface sentence from the acp_interface field metadata in config/loader.py (regenerating config-baseline.json) and drop step 2 from docs/guides/acp-interfaces.md's resolution list.
• Delete the description row from the guide's field table: AcpInterfaceConfig has no such field, _parse_acp_interfaces drops it, and a save loses it (0 consumers; grepped description in the dataclass and parser).
• Delete the three "(First Principles review on this PR)" / "(GPT review on this PR)" comment markers in acp/client.py, config/loader.py, session.py — AGENTS.md forbids review-round markers in comments.

[FIRST-PRINCIPLES-REVIEWED] c08364a

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

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

BLOCKING -- src/kiro_crew/acp/client.py:2813 -- external harnesses can bypass mandatory tool governance
argv = list(self._acp_command)
External harness executes a tool without session/request_permission -> no hook or SEL gate runs -> governed commands execute unaudited.
Anchor: backend-security-controls
Fix: Revert the external spawn path until every tool invocation is mediated.

BLOCKING -- src/kiro_crew/config/loader.py:4790 -- interface routing silently selects the wrong harness
crew_cfg = config.agents.get(crew_agent or ""); name=ACP_INTERFACE_DEFAULT,
Unknown binding or external-parent dedicated subagent -> resolver loses the requested interface -> task content is sent to kiro-cli.
Anchor: residual/security
Fix: Revert per-crew routing until explicit invalid bindings fail and dedicated subagents inherit the parent interface.

BLOCKING -- src/kiro_crew/config/loader.py:8971 -- adapter selection changes the Kiro construction path
iface = resolve_acp_interface(self, crew_agent, kiro_agent=agent, model=m)
Every Kiro provider construction -> adapter resolver and branch execute -> the first-class Kiro path is no longer unchanged.
Anchor: harness-parity H13
Fix: Revert this factory-path integration and register the adapter additively at ProviderRegistry.

BLOCKING -- src/kiro_crew/config/loader.py:3531 -- interface is omitted from dispatch-binding identity (origin: validation)
acp_interface: str = field(
Two crews differing only by interface -> chat mismatch guard treats them as the same binding -> dispatch uses the slot’s crew and sends the prompt to the wrong harness.
Anchor: residual/security
Fix: Revert the field until dispatch-binding identity includes the interface.

BLOCKING -- src/kiro_crew/config/loader.py:4838 -- executable canonicalization blocks the event loop (origin: validation)
command=[os.path.realpath(os.path.expanduser(iface.command)), *rendered],
External command on a stalled network mount -> chat cold-start factory -> synchronous realpath -> gateway loop freezes and the watchdog terminates it.
Anchor: no-blocking-call-on-event-loop
Fix: Canonicalize the command during config loading instead of inside the session factory.

FINDING -- src/kiro_crew/config/loader.py:3537 -- "global agent.acp_interface" is documented as an inheritance tier, but the resolver explicitly omits it -> Fix: remove that claim from the changed metadata and guide.

[GPT-REVIEWED] c08364a
[BLOCK-MERGE] c08364a

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No blocking findings — two advisory items on the effort-control paths.

FINDING — src/kiro_crew/providers/acp.py:1147 — clear_effort's non-claude branch runs await self._client.send_command("/effort", …) (the kiro-only _kiro.dev/commands/execute method) for ACP_BACKEND_EXTERNAL too; its sibling change_effort got the new if self.is_external_backend: return False guard but this path did not, and the caller gate provider.supports_effort() keys only on the model family, so clearing the slider on an effort-capable external-bound crew (workspace default resolves) emits a kiro-only command to a foreign harness → a 60s timeout or a spurious session reset, the exact fault the change_effort guard prevents → Fix: add if self.is_external_backend: return False before the kiro branch in clear_effort, mirroring change_effort.

FINDING — src/kiro_crew/config/loader.py:840 — except (KeyError, IndexError, ValueError) around rendered.append(arg.format(**subs)) omits AttributeError/TypeError, so an acp_interfaces.<name>.args entry containing a brace-dot/brace-subscript literal (e.g. "{model.json}" → str has no .json, or "{model[k]}" → "string indices must be integers") raises out of the unguarded provider factory _acp and the crew's session fails to construct — the opposite of the comment's "pass the argument through verbatim" intent → Fix: add AttributeError, TypeError to the caught tuple.

[OPUS-REVIEWED] c08364a

[benwart-consensus]

Dispositions — GPT 5.6 Review

All fixes are in 9b6f69d43. Three of the four blockers were real; one is rebutted.

• client.py:447 — validation and execution resolve different commands — FIXED

Relative command validates from gateway CWD -> spawn resolves it from crew CWD -> a workspace-controlled executable runs.

Correct, and the sharpest finding in the set. _is_launchable_command resolved a realpath but _spawn exec'd argv[0] verbatim, and the child's cwd is the session work dir, which is workspace-controlled — so the file checked and the file executed could differ. Closed on both halves of the invariant: _parse_acp_interfaces now refuses a non-absolute command at load (a relative command is ambiguous by construction, not something to resolve twice), and resolve_acp_interface stores os.path.realpath(...) in command[0], so the argv already carries the canonical path and nothing re-resolves it later. Pinned by test_external_command_must_be_absolute and test_resolved_command_is_absolute_and_canonical (the latter builds the command through a symlinked directory and asserts the argv holds the realpath).

• loader.py:1625 — global interface configuration is discarded — FIXED by deleting the tier

Configured agent.acp_interface -> loader leaves the field at "" -> sessions use kiro-cli and the next save overwrites the configured value.

Correct: I declared the field and never parsed it, so a configured value was ignored and silently overwritten on the next cfg.save(). Rather than wire the missing parse, the whole tier is removed — the First Principles lane independently found it had no named consumer, since the mixed-fleet case is served by the per-crew key and the all-crews case by agent.acp_backend. Resolution drops from four tiers to three. test_there_is_no_global_interface_tier asserts the field is absent from AgentConfig and that setting it anyway is not half-obeyed, so it is not reintroduced later as symmetry.

• loader.py:8962 — session reuse bypasses per-crew interface selection — FIXED

External-bound crew claims a matching warm provider or spawns a subagent without its crew identity -> factory resolution is skipped or defaults -> prompts go to kiro-cli instead of the selected harness.

Correct on the warm-pool half, and it is the worst failure mode in the diff because nothing fails: _fill_warm_pool builds providers with no crew_agent, and _claim_from_pool compares only the kiro agent name — so two crews sharing one kiro_agent while binding different interfaces can be handed each other's provider, and the prompts go to a harness the operator did not choose. Added a bypass_interface disqualifier beside the existing bypass_effort / bypass_env, so a request whose crew resolves to a different interface than the pool's rebuilds instead of claiming. It fails closed: any error comparing the two bypasses the pool, because a needless cold start costs latency while a wrong claim costs correctness.

On the subagent half: the backend id and its argv already travel together through _parent_runtime_kwargs (acp_backend + acp_command), which is why a subagent of an external-bound parent inherits the parent's harness rather than re-resolving without crew identity.

• client.py:2801 — external harnesses can bypass governance — REBUTTED

Harness executes a tool without requesting permission -> HookManager and SEL never run -> policy-denied commands execute.

The mechanism is real but it is not introduced by this branch, and the proposed fix ("revert the external-backend execution branch") deletes the feature. Tool gating lives on the session/request_permission path, so every ACP backend — kiro-cli and the claude seam included — must invoke it for the deny engine to see anything; there is no way for Crew to force a foreign process to ask. What declaring an interface actually is, is a deliberate trust grant, on the same footing as enabling a third-party app, which docs/architecture/app-platform-trust-model.md already calls equivalent to running that code with Crew's own privileges.

What is enforced regardless, and is the reason this is a bounded grant rather than an open door: ACP_BACKEND_EXTERNAL is excluded from ACP_BACKENDS_INTERNAL_SANDBOX, so wrap_argv does not skip Crew's seatbelt for it (that membership is the one that fails open, and the comment there says so); the kiro API key is stripped by _resolve_spawn_env(kiro_api_key=self._is_kiro); and it is in no capability set, so it gets no session sharing, no steer, no cli.json overlay and no kiro identity store. The requirement that a harness request permission is stated in the Security section of docs/guides/acp-interfaces.md, naming exactly this consequence.

• loader.py:4702 — function-local imports violate top-level-imports — REBUTTED

Fix: move both imports to module scope.

Cannot be done: it is a genuine import cycle, and the file already documents it. _normalize_acp_backend in this same module uses the identical deferred import with the reason inline — reaching kiro_crew.acp.types executes the kiro_crew.acp package init, which imports the ACP client and runtime, which import config.loader, and the gateway and desktop entrypoints import this module first. The new deferred kiro_prerequisite import in acp/client.py follows that file's own precedent at _resolve_kiro_bin, which carries the same comment for the same reason.

[benwart-consensus]

Dispositions — First Principles Review

The BLOCK was correct and it caught the most consequential defect in the change. Fixes are in 9b6f69d43.

• BLOCKER: the feature's own tests are dead on arrival — FIXED

setup.cfg:289 reads testpaths = test src/kiro_crew/apps/builtins — root tests/ is not collected [...] The description's "CI will be the first to execute the committed module" is false: CI will never execute it.

Correct, and worth stating plainly: I checked for testpaths in pyproject.toml, found none, and concluded rootdir collection would pick up both directories. The setting is in setup.cfg, so my claim that the tests run was wrong, and the suite that the description sells as the security pin would have shipped without ever executing. git mv tests/test_acp_interfaces.py test/test_acp_interfaces.py; 27 tests now collect and pass, and the file is under the test/conftest.py isolation floor as you note.

This is the second instance of the same class in this PR — earlier, a hand-written verification script had diverged from the committed tests, so "all checks pass" was verifying code CI never ran. Both had the same shape: evidence that looked like test evidence but was not connected to the collector. I now run the committed suite under pytest against the repo's own config rather than a parallel path.

• Subtraction: drop AcpInterfaceConfig.description — ACCEPTED, removed

parsed and stored, read by nothing [...] it exists by symmetry with MemoryStoreConfig.

Right on both counts, including the motive. Removed from the dataclass and the parse.

• Subtraction: drop _is_launchable_command — ACCEPTED, removed

it re-spells apps/interpreter.py:41 _runnable [...] and kiro_prerequisite._acp_executable_is_runnable, which its own docstring cites; that makes three spellings of one rule.

The docstring citing the predicate it duplicates is the tell. Deleted; the external spawn branch now calls kiro_prerequisite._acp_executable_is_runnable(exe, platform_name=sys.platform). test_spawn_asks_the_prerequisite_predicate_not_a_third_spelling asserts the local helper is gone and the shared one is called, because the half that drifts silently is Windows — there is no execute bit there, so a hand-rolled os.access(X_OK) answers True for a text file and the guard stops guarding without failing anything.

• Subtraction: drop the agent.acp_interface global key — ACCEPTED, removed

the mixed-fleet harm the PR names is served entirely by the crew-level key, and the all-crews case by tier 3; no one who needs a global external default is named.

Agreed, and it converged with a GPT blocker from the other direction: the tier was also never parsed, so a configured value was discarded and overwritten on save. Removing it fixes the bug and the generalization in one move; resolution is now three tiers.

• Item 4/9/10 framing — noted, no change

Items 4 and 9 are the two subtractions above, both taken. On item 10 (docs/guides/acp-interfaces.md — "AGENTS.md forbids new markdown unless instructed"): I could not find that rule in AGENTS.md, so I have kept the guide rather than delete it on an unverified reading — an operator-facing config surface with no operator-facing documentation seemed the worse outcome. Happy to remove it if you can point me at the line; the related mandate I did find (same-commit spec updates, AGENTS.md:30 and :205) is addressed in the Design Review disposition and is still outstanding.

• Watch: 12 pre-existing test files in root tests/ are equally uncollected — ACCEPTED-AND-DEFERRED

same cause, out of this PR's scope but worth a follow-up.

Confirmed, and it is a live hole rather than a tidiness issue: test_select_crew.py, test_config_roundtrip.py, test_context.py and nine others have presumably never run in CI. Out of scope here as you say. I will file an issue whose body names the task (move them under test/, or add tests to testpaths, and reconcile whatever then fails) so it is actionable by anyone who picks it up, and will link it here.

[benwart-consensus]

Dispositions — Design Review

CONCERNS is advisory and this check passed, so nothing in the pipeline would have forced these to be answered — both are legitimate and one is still open.

• tests/test_acp_interfaces.py is dead code — FIXED in 9b6f69d43

setup.cfg:289 pins testpaths = test src/kiro_crew/apps/builtins; a top-level tests/ directory is never collected [...] The description's "CI will be the first to execute the committed module" is false.

Correct, and independently found by the First Principles lane. Moved to test/, which both puts it under the collector and under the test/conftest.py isolation floor as you note. 27 tests collect and pass. The specific pin you name — test_external_claims_no_capability, the one that fails if ACP_BACKEND_EXTERNAL is ever added to INTERNAL_SANDBOX and unconfines every external harness — now actually runs.

• Module specs not updated in the same commit — ACCEPTED, still outstanding

docs/system-specs/modules/harness-parity.md enumerates the backend set and its own checklist ("a new harness adds rows to ACP_BACKENDS_KNOWN, a PROVIDER_LABEL_*…") now describes shipped work it doesn't record [...] AGENTS.md mandates same-commit spec updates; only a new guide was added.

Legitimate, and I verified the mandate rather than taking it on trust: AGENTS.md:30 and :205 both require the spec to change in the same commit as what it documents, and harness-parity.md:114 carries the literal checklist this change satisfies without recording — a new identifier in ACP_BACKENDS_KNOWN (H8), a new PROVIDER_LABEL_* (H11), and the capability-set memberships an external harness is excluded from.

Not yet done, so I am not claiming it as fixed. The three files and what each needs:

• harness-parity.md — add ACP_BACKEND_EXTERNAL to the enumerated backend set, and record that it claims no capability set, with INTERNAL_SANDBOX called out as the exclusion that must stay (it is the membership that fails open).

• acp-client.md — document that acp_backend can now select an operator-declared command rather than only a known subprocess, and that the argv comes from acp_interfaces already realpath-resolved.

• providers.md — the three sites it describes as not is_claude_backend are now positive kiro-family membership tests, which is what makes them correct with a fourth backend present.

• Suggestion: verify config-baseline.json regenerates rather than trusting hand-edited hunks — CONFIRMED, no change needed

a drifted baseline fails whichever gate compares them.

Sound caution and it is how the baseline was produced: python scripts/generate_config_baseline.py, never hand-edited, re-run after removing the agent.acp_interface and description fields (474 entries then, 473 now). test/test_config_baseline.py::TestCommittedBaselineParity::test_committed_snapshot_matches_generator passes locally, which is the gate that compares them.

[benwart-consensus]

Follow-up for the deferred item in the First Principles disposition: the 12 uncollected files in root tests/ are now tracked as #6577 — with the file list, the two candidate fixes (move under test/, or add tests to testpaths), and a fourth step for a gate so a test file outside testpaths fails rather than silently not running. That last step is what stops it recurring; this PR only moved its own file.

[bolichen97]

ACP coordination note from the actual full diffs of #5349, #6307, and this PR:

This PR's per-crew AcpInterfaceConfig / arbitrary absolute command+args+env interface is real functionality that #6307 does not provide, so it is not a duplicate. However, both patches rewrite the backend set, provider factory, child spawn, session inheritance, and model/effort routes. #5349's Codex-specialized path is already subsumed by #6307's broader adapter registry and should not become a third architecture.

Please rebase/integrate the external interface as a descriptor/admission implementation under #6307's registry and tool gate, rather than retaining a parallel closed backend-selection model. Preserve per-crew selection, absolute-command validation, env handling, and session inheritance through one shared spawn/validation path.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR has been stale with failing CI. I reviewed the blockers but they require your input:

• The GPT 5.6 blocking round on head c08364a05 demands architecture-level reverts: mediate every external-harness tool invocation through session/request_permission (governance/SEL gate), revert per-crew interface routing until invalid bindings fail explicitly, and register the adapter additively at ProviderRegistry instead of the factory path (harness-parity H13). These are design-direction rulings, not mechanical fixes an automated drive can make.
• The branch conflicts with current main across the same provider/spawn surface the findings target, so conflict resolution depends on the design outcome.
• The maintainer overlap audit (2026-08-29) notes this PR and feat: add staged acp adapter admission #6307 rewrite the same backend set, provider factory, child spawn, and session inheritance. Coordinating which PR hosts that shared surface is needed before fixes are useful.

When you've addressed these, the pipeline will re-assess on its next cycle.

[benwart-consensus]

Other pull requests are a better fit for the capability this was trying to add.