From f3dbcdbe00b6936238fcb16f19d9891a17dd3530 Mon Sep 17 00:00:00 2001 From: Vamil Gandhi <13998000+vamgan@users.noreply.github.com> Date: Sun, 30 Aug 2026 03:28:41 +0000 Subject: [PATCH 1/2] feat(acp): add a dormant Codex ACP backend seam Adds the Codex adapter as an adapted harness: known and driveable, but absent from BASELINE_SELECTABLE_BACKENDS, so no build offers it until an edition registers it with register_selectable_backend. Vocabulary and identifier - acp_backends.py: ACP_BACKEND_CODEX in ACP_BACKENDS_KNOWN and in POLICY_ID_BY_BACKEND, so a governance rule can name the id before an edition registers it. Absent from BASELINE_SELECTABLE_BACKENDS: the spawn path is complete, but backend_install.py has no probe for the adapter, so the switch would render with nothing to say about a session that failed to start. test_agent_backend_editable.py names the exception in NOT_SHIPPED_SELECTABLE rather than relaxing the baseline == known assertion. - acp/types.py: PROVIDER_LABEL_CODEX, plus an explicit non-membership decision with its reason for every Group B capability set (H6/H8): one process per session, no _session/steer, a Node harness so Crew's sandbox is the only OS confinement it gets, and its own credentials store so a kiro logout says nothing about it. Spawn path - acp/client.py: _is_codex; PROTOCOL_VERSION_CODEX as its own literal (H10, so a future divergence is a one-line edit rather than a silent downgrade of whichever harness moved first); _resolve_codex_acp_bin mirroring the claude resolver's order and node-resolution rules; a _spawn branch; the _codex_session_mcp_servers override seam; per-harness log labels; and a session/load arm -- the adapter keeps its own session records, so gating file_ok on a kiro transcript would make an activated codex session start fresh every time instead of resuming. - acp/client.py: each per-harness mcpServers hook is now spliced only for its own backend. Both defaults return [], so the previous shared splice was inert, but an edition overriding both hooks would hand a claude session codex's entries and vice versa -- and an entry whose transport the adapter does not advertise fails the whole session/new, not just that server. - acp/client.py: _vendored_claude_acp_roots -> _vendored_acp_roots. The roots are plain node_modules directories and each resolver joins its own package path onto them, so both adapters share it; the claude name would have made the next reader think codex had its own copy. Tuning channels stated as membership (H6) Six sites in providers/acp.py read `if is_claude_backend: else: `. That is correct with two harnesses and wrong with three: codex took the kiro arm at every one of them. ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION and ACP_BACKENDS_KIRO_SLASH_COMMANDS replace the negation. - stream_command and change_effort routed codex into _kiro.dev/commands/execute, a kiro extension the adapter does not implement, so every slash command and every effort push would have returned -32601 and reset the session. - _apply_effort_overlay and _apply_tool_search_overlay wrote the kiro workspace cli.json overlay for codex, which never reads it. The companion clear (_clear_cli_overlay_effort) was ALREADY membership-gated, so those writes left a stale effort level in the user's workspace that no later clear could reach. - clear_effort took the kiro arm: overlay write, then a /effort push. - The effort telemetry label was literally "claude" or "kiro", so a codex session logged backend=kiro. It now resolves through POLICY_ID_BY_BACKEND. - change_effort now reports unsupported for a harness in NEITHER set instead of defaulting to a channel. _set_claude_effort -> _set_effort_config_option: the method is the channel, not the harness, and codex now arrives there too. Tests - test_harness_parity.py: codex is known but not shipped-selectable; codex carries its own provider label; the model-switch channel is opt-in; the effort channel is opt-in and the two channels are disjoint; the overlay is written only for its readers; each mcpServers seam is spliced only for its own harness; codex keeps its own spawn branch and protocol literal. - test_acp_backend_kas.py: is_codex_backend in the mutual-exclusion pin, plus an ACP_BACKENDS_ACP_RUNTIME assertion. The is_acp_runtime_backend == not is_claude_backend equivalence is scoped to the three backends conversion covered rather than ACP_BACKENDS_KNOWN: that identity holds only while claude is the SOLE non-runtime harness, and codex is the first additional one, so widening it would require every future harness to be claude-shaped -- the negation reasoning H5/H6 exist to retire. - test_agent_sdk_backend_install.py: the codex install row reads unknown and names nothing to install, which is the gap that keeps it out of the baseline. Deliberately not changed: a failed adapter resolution stays cached for the process lifetime. That is pre-existing, documented behaviour for the claude resolver, and codex now matches it. Fixing it for codex alone would reintroduce the asymmetry the rest of this change removes -- it should be both or neither, in its own PR. .github/black-baseline.txt loses one entry: test_acp_backend_kas.py became black-clean, and the gate requires a graduated file to be pruned. --- .github/black-baseline.txt | 1 - docs/system-specs/modules/harness-parity.md | 36 ++-- src/kiro_crew/acp/client.py | 194 ++++++++++++++++-- src/kiro_crew/acp/types.py | 8 +- src/kiro_crew/acp_backends.py | 65 +++++- src/kiro_crew/providers/acp.py | 139 +++++++++---- test/test_acp_backend_kas.py | 32 ++- test/test_acp_client.py | 4 +- test/test_acp_provider.py | 2 +- test/test_agent_backend_editable.py | 20 +- test/test_agent_sdk_backend_install.py | 11 +- test/test_harness_parity.py | 156 ++++++++++++++ .../src/pages/developer/AgentBackendTab.tsx | 108 +++++++++- website/src/test/AgentBackendTab.test.tsx | 43 ++++ 14 files changed, 720 insertions(+), 99 deletions(-) diff --git a/.github/black-baseline.txt b/.github/black-baseline.txt index 44199e661d3..1a573b1358c 100644 --- a/.github/black-baseline.txt +++ b/.github/black-baseline.txt @@ -395,7 +395,6 @@ test/metrics/test_schema.py test/metrics/test_startup_channel_attrs.py test/metrics/test_telemetry_titles.py test/metrics/test_watchdog_metrics.py -test/test_acp_backend_kas.py test/test_acp_dynamic_config.py test/test_acp_error_surface.py test/test_acp_liveness.py diff --git a/docs/system-specs/modules/harness-parity.md b/docs/system-specs/modules/harness-parity.md index c9fc604e8d4..dfdaf2ccf63 100644 --- a/docs/system-specs/modules/harness-parity.md +++ b/docs/system-specs/modules/harness-parity.md @@ -3,21 +3,31 @@ A *harness* is the agent process Kiro Crew drives over ACP. Kiro Crew has one first-class harness — `kiro-cli` (`ACP_BACKEND_KIRO`, spelled `""`) — and a growing set of adapted ones: Claude Code (`ACP_BACKEND_CLAUDE`), `KAS` -(`ACP_BACKEND_KAS`), and whatever a bring-your-own (BYO) adapter registers next. +(`ACP_BACKEND_KAS`), the dormant `ACP_BACKEND_CODEX` seam, and whatever a +bring-your-own (BYO) adapter registers next. -All three are selectable on a plain public build. -`BASELINE_SELECTABLE_BACKENDS` equals `ACP_BACKENDS_KNOWN`, so every id this core -can spell is one an operator can choose — pinned by -`test_agent_backend_editable.py::test_baseline_ships_every_known_backend`, which -guards against a future NARROWING rather than a widening. Claude Code in +Kiro, Claude Code and KAS are selectable on a plain public build; Claude Code in particular is a shipped harness and not a dormant seam: `acp/client.py` owns the whole Claude spawn path and the adapter is a public npm package, so an earlier revision that left it out of the baseline removed only the switch, never a capability. Whether the binaries are INSTALLED on a given machine is a different question, answered by `agent_sdk/backend_install.py`'s probe rather than by -selectability. Read the invariants below against that tree: three harnesses can -serve a real session today, so a site that spells "kiro" by exclusion is already -wrong on two of them. +selectability. + +`BASELINE_SELECTABLE_BACKENDS` is otherwise `ACP_BACKENDS_KNOWN`, so an id this +core can spell is an id an operator can choose unless something states the +exception — pinned by +`test_agent_backend_editable.py::test_baseline_ships_every_known_backend`, which +guards against an undocumented NARROWING rather than a widening. +`ACP_BACKEND_CODEX` is the one exception and it is named in that test: the spawn +path is complete, but `backend_install.py` has no probe for the adapter, so a +build offering the switch could not tell an operator what was missing when the +session failed to start. It becomes selectable through +`register_selectable_backend`, or through the baseline once that probe lands. + +Read the invariants below against that tree: three harnesses can serve a real +session today, so a site that spells "kiro" by exclusion is already wrong on two +of them. *Parity* here does not mean equal treatment. It means the opposite, stated precisely: **an added harness may only adapt itself to the seams the Kiro @@ -78,9 +88,9 @@ already reads TRUE for KAS on a plain public build. | Id | Guarantees | Pinned by | Constrains | |---|---|---|---| | H5 | Harness identity is a positive comparison against a named constant, or membership in a named set. `not is_claude_backend`, `!= ACP_BACKEND_KAS`, and `== "kas"` (bare literal) are all forbidden; `is_kiro_backend` and `backend in ACP_BACKENDS_` are the forms. Enforced on the lines a change ADDS, not whole-tree — see the gate doc for why. | `scripts/check_harness_parity.py` (six rules, self-tested), `test_harness_parity.py::test_added_line_gate_self_test_passes`, `::test_added_line_gate_flags_a_planted_negative_test` | every module reading `AcpClient.backend` / `AcpProvider.is_*_backend` | -| H6 | A capability is granted by opt-in membership, never by negation. `is_session_sharing_eligible` reads `ACP_BACKENDS_SESSION_SHARING` and `supports_steer` reads `ACP_BACKENDS_STEER`, so a harness that has not demonstrated the capability does not inherit it from a set it was never added to. | `test_harness_parity.py::test_session_sharing_is_opt_in`, `::test_steer_is_opt_in` | `providers/acp.py` (`AcpProvider.is_session_sharing_eligible`), `acp/client.py` (`AcpClient.supports_steer`), `acp/types.py` | +| H6 | A capability is granted by opt-in membership, never by negation. `is_session_sharing_eligible` reads `ACP_BACKENDS_SESSION_SHARING` and `supports_steer` reads `ACP_BACKENDS_STEER`, so a harness that has not demonstrated the capability does not inherit it from a set it was never added to. Every *tuning channel* follows the same rule, one set per channel because a harness can implement one and not another: `ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION` (model switch), `ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION` (effort push), and `ACP_BACKENDS_KIRO_SLASH_COMMANDS` — membership in the last also decides who is sent `_kiro.dev/commands/execute` and who gets the workspace `cli.json` overlay written for them. A harness in none of these must not inherit a channel that answers `-32601`, nor collect an overlay it never reads and the membership-gated clear can never remove. | `test_harness_parity.py::test_session_sharing_is_opt_in`, `::test_steer_is_opt_in`, `::test_model_switch_channel_is_opt_in`, `::test_effort_channel_is_opt_in`, `::test_only_overlay_readers_are_written_to` | `providers/acp.py` (`AcpProvider.is_session_sharing_eligible`, `change_effort`, `clear_effort`, `_apply_effort_overlay`, `_apply_tool_search_overlay`, `stream_command`), `acp/client.py` (`AcpClient.supports_steer`), `acp_backends.py` | | H7 | `is_kiro_cli` is a positive Kiro test at every call site. It drives internal-sandbox delegation: macOS skips Kiro Crew's seatbelt because Kiro's sandbox cannot nest inside it, and Windows permits the official Kiro backend to run despite having no Kiro Crew OS wrapper. Passed for a harness with no internal sandbox, it hands isolation to a layer that never starts; this is the only Group B row that is also a security invariant. **Windows requires `is_kiro_cli is True` exactly** — `None` and `_spawns_kiro_cli` basename inference can never grant the backend-less-host exception. On macOS a site may grant membership explicitly or pass `None` to defer to the positive basename test. | `test_harness_parity.py::test_is_kiro_cli_is_positive`, `test_sandbox_argv.py::TestKiroInternalSandboxExclusion` | `acp/runtime.py` (`AcpRuntime.spawn`), `acp/client.py` (`AcpClient.ensure_ready`), `sandbox.py` (`wrap_argv`, `_spawns_kiro_cli`) | -| H8 | New harness identifiers live in `acp_backends.py` — a LEAF module, so every consumer can name the constants rather than copy them — and are added to `ACP_BACKENDS_KNOWN`; every capability set is a subset of it; and `AcpProvider.__init__` rejects anything outside it. `ACP_BACKEND_KIRO` is the empty string, so a value that falls through every identity check spawns `kiro-cli` under a foreign label. `acp/types.py` re-exports the vocabulary and remains the import site for existing callers. | `test_harness_parity.py::test_capability_sets_are_subsets_of_known_backends`, `::test_unknown_backend_rejected_at_construction` | `acp_backends.py` (`ACP_BACKENDS_KNOWN`), `providers/acp.py` (`AcpProvider.__init__`), `scripts/check_harness_parity.py` (`VOCABULARY_PATH`) | +| H8 | New harness identifiers live in `acp_backends.py` — a LEAF module, so every consumer can name the constants rather than copy them — and are added to `ACP_BACKENDS_KNOWN`; every capability set is a subset of it; and `AcpProvider.__init__` rejects anything outside it. `ACP_BACKEND_KIRO` is the empty string, so a value that falls through every identity check spawns `kiro-cli` under a foreign label. `acp/types.py` re-exports the vocabulary and remains the import site for existing callers. | `test_harness_parity.py::test_capability_sets_are_subsets_of_known_backends`, `::test_unknown_backend_rejected_at_construction`, `::test_codex_is_known_but_not_shipped_selectable` | `acp_backends.py` (`ACP_BACKENDS_KNOWN`), `providers/acp.py` (`AcpProvider.__init__`), `scripts/check_harness_parity.py` (`VOCABULARY_PATH`) | ## Group C: the Kiro path keeps its own machinery @@ -90,9 +100,9 @@ test still passes. | Id | Guarantees | Pinned by | Constrains | |---|---|---|---| -| H9 | `kiro-cli` remains the default branch of spawn-argv resolution, keeping its pre-spawn agent materialization (`kiro-cli` discovers selectable modes from `~/.kiro/agents/*.json` at startup, so a later `set_mode` fails with "Mode not found" without it) and its `--model` pin (the only way to run a model outside the agent's provider). A dict-of-builders refactor that treats Kiro as one entry among N drops both. | `test_harness_parity.py::test_kiro_spawn_argv_keeps_its_own_branch` | `acp/runtime.py` (`AcpRuntime._resolve_spawn_argv`) | +| H9 | `kiro-cli` remains the default branch of spawn-argv resolution, keeping its pre-spawn agent materialization (`kiro-cli` discovers selectable modes from `~/.kiro/agents/*.json` at startup, so a later `set_mode` fails with "Mode not found" without it) and its `--model` pin (the only way to run a model outside the agent's provider). A dict-of-builders refactor that treats Kiro as one entry among N drops both. | `test_harness_parity.py::test_kiro_spawn_argv_keeps_its_own_branch`, `::test_codex_spawn_keeps_its_own_branch` | `acp/runtime.py` (`AcpRuntime._resolve_spawn_argv`) | | H10 | Protocol version and client capabilities stay per-harness literals. Collapsing them to one handshake that every harness accepts silently downgrades the Kiro session's declared capabilities. | `test_harness_parity.py::test_handshake_is_per_backend` | `acp/runtime.py` (`AcpRuntime.spawn`), `acp/types.py` (`ACP_CLIENT_CAPABILITIES`, `KAS_CLIENT_CAPABILITIES`) | -| H11 | The provider label is a closed mapping and an absent label means Kiro. It indexes resume compatibility, session-map persistence, and session-file cleanup routing, so a harness with no `PROVIDER_LABEL_*` of its own persists as a Kiro session and its transcript is pruned for want of a Kiro session file. | `test_harness_parity.py::test_every_known_backend_has_a_label` | `acp/types.py` (`PROVIDER_LABEL_*`), `providers/acp.py` (`provider_label`, `cleanup_session`), `session.py` (`detect_provider_switch`) | +| H11 | The provider label is a closed mapping and an absent label means Kiro. It indexes resume compatibility, session-map persistence, and session-file cleanup routing, so a harness with no `PROVIDER_LABEL_*` of its own persists as a Kiro session and its transcript is pruned for want of a Kiro session file. | `test_harness_parity.py::test_every_known_backend_has_a_label`, `::test_codex_carries_its_own_provider_label` | `acp/types.py` (`PROVIDER_LABEL_*`), `providers/acp.py` (`provider_label`, `cleanup_session`), `session.py` (`detect_provider_switch`) | | H12 | Model pre-flight keeps "empty or unknown advertised set means allow", and never compares ids across harness namespaces. Harnesses advertise ids in their own spelling; one shared membership test across two namespaces calls every legitimate model unusable and withholds the model. | `test_harness_parity.py::test_model_preflight_allows_unknown_advertised_set` | `acp/client.py` (`model_is_unusable`, `advertised_model_ids`) | ## Group D: review-only invariants diff --git a/src/kiro_crew/acp/client.py b/src/kiro_crew/acp/client.py index 0a9de73c777..4562d7eeb14 100644 --- a/src/kiro_crew/acp/client.py +++ b/src/kiro_crew/acp/client.py @@ -56,8 +56,10 @@ from kiro_crew.acp.prompt_blocks import build_prompt_blocks from kiro_crew.acp.types import ( ACP_BACKEND_CLAUDE, + ACP_BACKEND_CODEX, ACP_BACKEND_KIRO, ACP_BACKENDS_INTERNAL_SANDBOX, + ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION, ACP_BACKENDS_STEER, ACP_CLIENT_CAPABILITIES, EVENT_AGENT_SWITCHED, @@ -163,6 +165,11 @@ # upstream ACP SDK (numeric integer, currently 1). See acp.types. PROTOCOL_VERSION = "2025-08-22" PROTOCOL_VERSION_CLAUDE = 1 +# codex-acp speaks the same numeric ACP version as the claude adapter today. +# Kept as its OWN literal rather than folded into the claude one: harness-parity +# H10 wants the handshake stated per harness, so a divergence is a one-line edit +# here instead of a silent downgrade of whichever harness moved first. +PROTOCOL_VERSION_CODEX = 1 DEFAULT_MODEL = "auto" KIRO_CLI_BIN = "kiro-cli" @@ -199,6 +206,26 @@ # incomplete root and fall through to the next candidate. _CLAUDE_ACP_DEP_MARKER = Path("@agentclientprotocol") / "sdk" +# ── codex-acp (ACP_BACKEND_CODEX) ── +# A Node stdio server that boots the Codex app server and translates ACP onto its +# operations. The ``codex`` CLI does not serve ACP itself -- it reads ``acp`` as a +# prompt -- so the adapter is the transport, not an optimization. It takes no argv +# beyond its own path: any invocation enters stdio-server mode and blocks on stdin. +CODEX_ACP_BIN = "codex-acp" +CODEX_ACP_NPM_PKG = "@agentclientprotocol/codex-acp" +_CODEX_ACP_PKG_ENTRY = Path(CODEX_ACP_NPM_PKG) / "dist" / "index.js" +# Same hoisted-dependency completeness check as the claude adapter, and the same +# dependency: codex-acp imports @agentclientprotocol/sdk, so a root carrying the +# entry script without it dies at ESM import time -- after the child is spawned. +_CODEX_ACP_DEP_MARKER = _CLAUDE_ACP_DEP_MARKER +# Explicit override, spelled the way the adapter's own documentation spells it. +_ENV_CODEX_ACP_BIN = "CODEX_ACP_BIN" +# No CODEX_PATH constant: the adapter ships a compatible Codex binary as an npm +# dependency and reads CODEX_PATH itself only to run a DIFFERENT one. An operator +# who sets it reaches the child through the ambient environment copy, so naming it +# here would imply a wiring that does not exist (its claude counterpart, +# CLAUDE_CODE_EXECUTABLE, IS explicitly forwarded — the asymmetry is deliberate). + # High-frequency, content-free adapter stderr diagnostics that _drain_stderr() # drops instead of forwarding as per-line WARNINGs. The driving case is the # claude-agent-acp "Unexpected case: {...thinking_tokens...}" line. Mechanism @@ -456,15 +483,19 @@ def _resolve_node_for_script(script_path: str) -> str | None: _claude_acp_argv_cache: tuple[list[str] | None, str] | object = _UNRESOLVED -def _vendored_claude_acp_roots(pkg_dir: Path | None = None) -> list[Path]: - """Directories that may contain a project-local ``node_modules`` copy of - the claude-agent-acp adapter. +def _vendored_acp_roots(pkg_dir: Path | None = None) -> list[Path]: + """Directories that may contain a project-local ``node_modules`` copy of a + Node ACP adapter. + + Harness-neutral: the roots are plain ``node_modules`` directories, and each + adapter resolver joins its own package path onto them, so this is shared by + the claude and codex resolvers rather than duplicated per harness. - A project-local install (``npm i @agentclientprotocol/claude-agent-acp`` in - the repo, or a copy bundled next to the installed package) lets the gateway - run without a global npm install — useful in non-login launchd/systemd - contexts with a minimal PATH. Resolution still falls back to global / PATH - installs in ``_resolve_claude_acp_bin``; these roots are just preferred. + A project-local install (``npm i @agentclientprotocol/`` in the + repo, or a copy bundled next to the installed package) lets the gateway run + without a global npm install — useful in non-login launchd/systemd contexts + with a minimal PATH. Resolution still falls back to global / PATH installs + in each ``_resolve_*_acp_bin``; these roots are just preferred. *pkg_dir* (the installed ``kiro_crew`` package directory) defaults to this module's location; it is a parameter so tests can inject a fake layout. @@ -497,7 +528,7 @@ def _resolve_vendored_claude_acp(pkg_dir: Path | None = None) -> str | None: copy (entry script but missing deps) is skipped in favour of a complete one rather than picked and crashed at ESM import time. """ - for root in _vendored_claude_acp_roots(pkg_dir): + for root in _vendored_acp_roots(pkg_dir): entry = root / _CLAUDE_ACP_PKG_ENTRY if entry.is_file() and (root / _CLAUDE_ACP_DEP_MARKER).is_dir(): return str(entry) @@ -579,6 +610,61 @@ def _resolve_claude_acp_bin() -> tuple[list[str] | None, str]: return None, search_path +_codex_acp_argv_cache: tuple[list[str] | None, str] | object = _UNRESOLVED + + +def _resolve_codex_acp_bin() -> tuple[list[str] | None, str]: + """Find the codex-acp Node entry script and the PATH searched for it. + + Same contract, order and node-resolution rules as + :func:`_resolve_claude_acp_bin` — deliberately, so an operator debugging one + adapter is debugging both: explicit override, then a project-local + ``node_modules`` copy (accepted only with the dependency marker beside it), + then mise, then the augmented PATH; and node is resolved explicitly rather + than left to a shebang that daemon contexts cannot follow. + """ + candidates: list[str] = [] + + override = os.environ.get(_ENV_CODEX_ACP_BIN) + if override and Path(override).is_file(): + candidates.append(override) + + for root in _vendored_acp_roots(): + entry = root / _CODEX_ACP_PKG_ENTRY + if entry.is_file() and (root / _CODEX_ACP_DEP_MARKER).is_dir(): + candidates.append(str(entry)) + break + + mise_resolved = _mise_which(CODEX_ACP_BIN) + if mise_resolved: + candidates.append(mise_resolved) + + mise_installs = _mise_node_installs_dir() + if mise_installs.is_dir(): + for bin_path in sorted(mise_installs.glob("*/bin/" + CODEX_ACP_BIN), reverse=True): + if bin_path.is_file(): + candidates.append(str(bin_path)) + break + + search_path = augmented_path(os.environ.get("PATH", "")) + on_path = shutil.which(CODEX_ACP_BIN, path=search_path) + if on_path: + candidates.append(on_path) + + for script in candidates: + resolved = str(Path(script).resolve()) + node = _resolve_node_for_script(resolved) + if node: + return [node, resolved], search_path + if platform_compat.is_executable_file(script): + return [_normalize_exe_casing(script) or script], search_path + node_on_path = shutil.which("node", path=search_path) + if node_on_path: + return [node_on_path, resolved], search_path + + return None, search_path + + def _resolve_claude_code_executable() -> str | None: """Find the Claude backend CLI binary for CLAUDE_CODE_EXECUTABLE. @@ -2466,12 +2552,16 @@ def backend(self) -> str: def _is_claude(self) -> bool: return self.backend == ACP_BACKEND_CLAUDE + @property + def _is_codex(self) -> bool: + return self.backend == ACP_BACKEND_CODEX + @property def _is_kiro(self) -> bool: """True when this client drives kiro-cli (the AcpClient default). - AcpClient serves exactly two backends — kiro-cli and claude-agent-acp — - so this is the positive spelling of the sites that used to read + AcpClient serves kiro-cli, claude-agent-acp and the dormant codex seam, so + this is the positive spelling of the sites that used to read ``not self._is_claude`` (harness-parity H5). KAS runs on AcpRuntime, not AcpClient, so it never reaches this property. """ @@ -2501,6 +2591,20 @@ def _claude_session_mcp_servers(self) -> list: """ return [] + def _codex_session_mcp_servers(self) -> list: + """MCP server array passed to a codex ``session/new`` / ``session/load``. + + The codex twin of :meth:`_claude_session_mcp_servers`, and ``[]`` for the + same reason: kiro-cli receives its servers through ``--agent``, so the + public core sends nothing here and stays byte-identical. + + An edition overriding this must drop any entry whose transport the adapter + does not advertise. codex-acp answers ``session/new`` with ``-32602`` for + an unsupported transport rather than skipping that one server, so a single + bad entry costs the whole session. + """ + return [] + @property def is_ready(self) -> bool: return self._process is not None and self._session_id is not None @@ -2594,7 +2698,7 @@ async def set_model(self, model_id: str) -> None: _rejected_log, _ = redact_exfiltration_urls(str(model_id)) _rejected_log, _ = redact_credentials(_rejected_log) raise AcpModelUnavailable(_rejected_log, self._advertised_model_ids()) - if self._is_claude: + if self.backend in ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION: await self.set_config_option("model", model_id) else: await self._send_request( @@ -2722,7 +2826,7 @@ async def _apply_startup_model(self) -> None: # unusable id here would re-offer it on every claim. self._model = DEFAULT_MODEL return - if self._is_claude: + if self.backend in ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION: await self.set_config_option("model", self._model) else: await self._send_request( @@ -2968,6 +3072,30 @@ async def _spawn(self) -> None: f"dependency), or set CLAUDE_AGENT_ACP_BIN to its entry script." ) argv: list[str] = claude_argv + elif self._is_codex: + # Dormant seam — see method docstring. codex-acp takes no argv of its + # own: the adapter is spawned bare and driven entirely over the pipe, + # so unlike the kiro branch there is nothing to append. CODEX_PATH is + # left exactly as the operator set it (the adapter ships its own Codex + # binary; overriding it is an explicit choice, never a default). + global _codex_acp_argv_cache # noqa: PLW0603 + if _codex_acp_argv_cache is _UNRESOLVED: + _codex_acp_argv_cache = await asyncio.to_thread(_resolve_codex_acp_bin) + cached_codex_resolution = _codex_acp_argv_cache + codex_argv, codex_search_path = ( + cached_codex_resolution + if isinstance(cached_codex_resolution, tuple) + else (None, "") + ) + if not isinstance(codex_argv, list) or not codex_argv: + raise AcpError( + f"{CODEX_ACP_BIN} not found " + f"({describe_search_path(codex_search_path)}). Install it with " + f"'npm i -g {CODEX_ACP_NPM_PKG}' (or add it as a project " + f"dependency), or set {_ENV_CODEX_ACP_BIN} to its entry script. " + f"The 'codex' CLI alone does not serve ACP." + ) + argv = codex_argv else: # Pin ONE reading of the environment for both the search and the # message that reports it. The previous code resolved against the live @@ -3163,7 +3291,9 @@ async def _spawn(self) -> None: raise self._pid = self._process.pid _spawn_label = ( - "claude-agent-acp" if self._is_claude else f"{KIRO_CLI_BIN} {KIRO_CLI_SUBCMD}" + CLAUDE_ACP_BIN + if self._is_claude + else CODEX_ACP_BIN if self._is_codex else f"{KIRO_CLI_BIN} {KIRO_CLI_SUBCMD}" ) # Everything from here to the end of _spawn runs with a LIVE subprocess # that nothing has recorded yet, so every step must be guarded. Without @@ -3292,7 +3422,11 @@ async def _drain_stderr(self, stderr: asyncio.StreamReader) -> None: self._stderr_lines.append(text) redacted, _ = redact_exfiltration_urls(text) redacted, _ = redact_credentials(redacted) - _bin_label = "claude-acp" if self._is_claude else KIRO_CLI_BIN + _bin_label = ( + "claude-acp" + if self._is_claude + else CODEX_ACP_BIN if self._is_codex else KIRO_CLI_BIN + ) logger.warning("%s stderr: %s", _bin_label, redacted) if suppressed: # Flush the residual count once the stream closes so the final burst @@ -3560,8 +3694,14 @@ async def _new_session_following_substitution(self) -> dict: # Pooled broker stubs are appended for kiro-cli: a session-injected # server outranks the same-named entry in the agent spec, which is # how pooling takes effect without writing a spec anywhere. + # Each per-harness hook is spliced only for ITS OWN backend. Both + # defaults return [] so an ungated splice is inert in this tree, but an + # edition overriding both would hand a claude session codex's entries + # and vice versa -- and one entry whose transport the adapter does not + # advertise fails the whole session/new, not just that server. "mcpServers": [ - *self._claude_session_mcp_servers(), + *(self._claude_session_mcp_servers() if self._is_claude else []), + *(self._codex_session_mcp_servers() if self._is_codex else []), *(await asyncio.to_thread(self._pooled_mcp_servers)), ], } @@ -3629,7 +3769,9 @@ async def _initialize_session(self) -> None: """Handshake: initialize → session/load or session/new → set_mode → set_model.""" # 1. Initialize protocol_version: int | str = ( - PROTOCOL_VERSION_CLAUDE if self._is_claude else PROTOCOL_VERSION + PROTOCOL_VERSION_CLAUDE + if self._is_claude + else PROTOCOL_VERSION_CODEX if self._is_codex else PROTOCOL_VERSION ) init_id = await self._send_request( METHOD_INITIALIZE, @@ -3666,6 +3808,15 @@ async def _initialize_session(self) -> None: # simply attempts the load. session_file = "" file_ok = True + elif self._is_codex: + # Same shape as claude, and for the same reason: the adapter keeps + # its own session records and resolves them from the sessionId, so + # there is no Crew-side file to name. Gating on the kiro transcript + # here would make file_ok always False — a codex session could + # never resume, it would silently start fresh every time — which is + # why the _meta block below gives codex no session_file either. + session_file = "" + file_ok = True else: session_file = str(kiro_sessions_dir() / f"{resume_sid}.json") file_ok = Path(session_file).exists() @@ -3680,13 +3831,20 @@ async def _initialize_session(self) -> None: # unchanged; a companion overrides the hook (see # session/new above). Pooled stubs are re-declared so a # resumed session keeps talking to the broker. + # Gated per backend for the same reason as session/new above. "mcpServers": [ - *self._claude_session_mcp_servers(), + *(self._claude_session_mcp_servers() if self._is_claude else []), + *(self._codex_session_mcp_servers() if self._is_codex else []), *(await asyncio.to_thread(self._pooled_mcp_servers)), ], } if self._is_claude: load_params["_meta"] = {"claudeCode": {"options": {}}} + elif self._is_codex: + # codex-acp carries no Crew-side session file and reads no + # _meta of ours, so it gets neither key rather than the + # kiro session_file it would not know what to do with. + pass else: load_params["_meta"] = {"_kiro.dev/session_file": session_file} load_id = await self._send_request(METHOD_SESSION_LOAD, load_params) diff --git a/src/kiro_crew/acp/types.py b/src/kiro_crew/acp/types.py index 5314367bf04..38afdb2fee3 100644 --- a/src/kiro_crew/acp/types.py +++ b/src/kiro_crew/acp/types.py @@ -15,12 +15,16 @@ # unchanged — see the "ACP Backend Identifiers" section below for why they moved. from kiro_crew.acp_backends import ( # noqa: F401 - re-exported for existing importers ACP_BACKEND_CLAUDE, + ACP_BACKEND_CODEX, ACP_BACKEND_KAS, ACP_BACKEND_KIRO, ACP_BACKENDS_ACP_RUNTIME, + ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION, ACP_BACKENDS_INTERNAL_SANDBOX, ACP_BACKENDS_KIRO_IDENTITY_STORE, + ACP_BACKENDS_KIRO_SLASH_COMMANDS, ACP_BACKENDS_KNOWN, + ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION, ACP_BACKENDS_SESSION_SHARING, ACP_BACKENDS_STEER, selectable_backends, @@ -139,14 +143,13 @@ # snapshot here would be read before boot registration and silently miss it. # ── Capability membership ── -# The five ``ACP_BACKENDS_*`` capability sets are DEFINED in the leaf module +# The ``ACP_BACKENDS_*`` capability sets are DEFINED in the leaf module # ``kiro_crew.acp_backends`` and re-exported by the import above, so # ``from kiro_crew.acp.types import ACP_BACKENDS_STEER`` still resolves. They moved # for the same reason the backend identifiers did: a consumer outside this package # must be able to ask a capability question without importing ``kiro_crew.acp``, # whose ``__init__`` pulls in the client and runtime. - # ── Provider labels ── # The backend identity key persisted in the session map. It indexes three # things, so every producer must agree on it: resume compatibility @@ -158,6 +161,7 @@ PROVIDER_LABEL_DEFAULT = "acp" PROVIDER_LABEL_CLAUDE = "claude_code" PROVIDER_LABEL_KAS = "kas" +PROVIDER_LABEL_CODEX = "codex" # KAS reads only fs.readTextFile / fs.writeTextFile / terminal from the top # level of clientCapabilities; every other capability it honours lives under diff --git a/src/kiro_crew/acp_backends.py b/src/kiro_crew/acp_backends.py index 0c4091869d6..8ca1cdc899c 100644 --- a/src/kiro_crew/acp_backends.py +++ b/src/kiro_crew/acp_backends.py @@ -39,6 +39,11 @@ ACP_BACKEND_CLAUDE = "claude" ACP_BACKEND_KAS = "kas" +# The Codex ACP adapter: a Node stdio server that boots the Codex app server and +# translates ACP onto its operations. Known so that an edition shipping a provider +# for it can register the id; absent from BASELINE_SELECTABLE_BACKENDS below, so no +# build offers it until something registers it. +ACP_BACKEND_CODEX = "codex" # The kiro-cli backend is spelled as the empty string throughout, so name it # rather than leaving every call site to infer it from "not claude". ACP_BACKEND_KIRO = "" @@ -51,6 +56,7 @@ ACP_BACKEND_KIRO, ACP_BACKEND_CLAUDE, ACP_BACKEND_KAS, + ACP_BACKEND_CODEX, } ) @@ -70,6 +76,13 @@ #: Whether it is USABLE on a given machine is a separate question with its own answer: #: :mod:`kiro_crew.agent_sdk.backend_install` probes for the two binaries and the #: dashboard reports what is absent plus the command that installs it. +#: +#: ``ACP_BACKEND_CODEX`` is deliberately absent, and for a reason that does NOT apply +#: to claude: the spawn path lands here, but no provider registers it and +#: ``backend_install`` has no probe for its adapter, so a build offering the option +#: could not tell an operator what is missing when the session failed to start. It +#: becomes selectable when something calls :func:`register_selectable_backend` — +#: adding it here instead would ship an option ahead of the code that answers for it. BASELINE_SELECTABLE_BACKENDS: FrozenSet[str] = frozenset( {ACP_BACKEND_KIRO, ACP_BACKEND_CLAUDE, ACP_BACKEND_KAS} ) @@ -88,6 +101,10 @@ ACP_BACKEND_KIRO: POLICY_ID_KIRO, ACP_BACKEND_KAS: ACP_BACKEND_KAS, ACP_BACKEND_CLAUDE: ACP_BACKEND_CLAUDE, + # Present even though no build ships codex as selectable: a policy author has to + # be able to deny an id BEFORE an edition registers it, and the mapping is what + # makes the id nameable in a rule at all. + ACP_BACKEND_CODEX: ACP_BACKEND_CODEX, } #: The backend a deployment policy may never deny. @@ -265,10 +282,13 @@ def resolve_selected_backend(value: object) -> str: # spawn_continue (conversation_gone). KAS therefore opts in only once a # keep-aware teardown lands (native subagent work); until then its subagents get # dedicated sessions. claude-agent-acp runs through AcpClient (one process per -# session) and is not a member. +# session) and is not a member. codex-acp is not either, for the same reason: one +# adapter process serves one session, so there is nothing to share. ACP_BACKENDS_SESSION_SHARING = frozenset({ACP_BACKEND_KIRO}) -# Backends implementing the ``_session/steer`` extension (mid-turn steer). +# Backends implementing the ``_session/steer`` extension (mid-turn steer). Neither +# claude-agent-acp nor codex-acp implements it, so a steer sent to either would be +# answered with method-not-found rather than reaching the turn. ACP_BACKENDS_STEER = frozenset({ACP_BACKEND_KIRO, ACP_BACKEND_KAS}) # Backends carrying their OWN internal OS sandbox, which on macOS cannot nest @@ -284,6 +304,11 @@ def resolve_selected_backend(value: object) -> str: # sandbox factory resolves an absent config to its no-op backend, so no OS # sandbox starts inside — adding KAS here would skip Crew's seatbelt in favour of # a layer that does not exist. See :mod:`kiro_crew.acp.kas_transport`. +# +# codex-acp is excluded on the same rule: it is a Node adapter, so Crew's own layer +# is the only OS confinement a codex session gets. The Codex sandbox modes the +# adapter can apply are in-process policy, not an OS sandbox that Crew's would +# nest inside. ACP_BACKENDS_INTERNAL_SANDBOX = frozenset({ACP_BACKEND_KIRO}) # Backends served by AcpRuntime + AcpSessionHandle — the kiro-agent family @@ -297,7 +322,9 @@ def resolve_selected_backend(value: object) -> str: # ``not is_claude_backend`` — an inference that silently captures every harness # added later. This is a SUPERSET of ACP_BACKENDS_SESSION_SHARING: running on # AcpRuntime is necessary for session sharing but not sufficient (KAS runs here -# yet is excluded from sharing until keep-aware teardown lands). +# yet is excluded from sharing until keep-aware teardown lands). codex-acp is not a +# member: it is spawned per session and reads none of the kiro-family cli.json +# overlay, so it takes the AcpClient path. ACP_BACKENDS_ACP_RUNTIME = frozenset({ACP_BACKEND_KIRO, ACP_BACKEND_KAS}) # Backends whose sign-in lives in kiro-cli's OWN identity store, so an external @@ -313,4 +340,36 @@ def resolve_selected_backend(value: object) -> str: # Excluding it would let a KAS session keep serving turns on the previous # account's credentials. Positive membership rather than "not claude" # (harness-parity H5). +# +# codex-acp is excluded: it signs in through its own credentials file, so a +# kiro-cli logout says nothing about whether a running codex session is still +# authenticated, and retiring its child on that signal would end a live turn for +# no reason. ACP_BACKENDS_KIRO_IDENTITY_STORE = frozenset({ACP_BACKEND_KIRO, ACP_BACKEND_KAS}) + +# Backends that switch models through ``session/set_config_option("model", ...)`` +# rather than the kiro-native ``session/set_model`` request. Opt-in for the same +# reason as every set above: a switch sent down a channel the adapter does not +# implement is answered with method-not-found, and the session keeps serving turns +# on the model the operator thought they had just left. +ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION = frozenset({ACP_BACKEND_CLAUDE, ACP_BACKEND_CODEX}) + +# Backends that take a reasoning-effort change through +# ``session/set_config_option("effort", ...)``. A SEPARATE set from the model +# channel above despite identical membership today: the two config options are +# advertised independently, and ``AcpClient.supports_config_option`` exists +# precisely because adapter builds ship one without the other. Collapsing them +# would make an adapter that gained model-switching inherit an effort channel it +# never advertised. +ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION = frozenset({ACP_BACKEND_CLAUDE, ACP_BACKEND_CODEX}) + +# Backends implementing ``_kiro.dev/commands/execute`` — the kiro extension that +# runs a slash command as an RPC. Non-members have no equivalent verb, so their +# slash commands go through ``session/prompt`` and are interpreted by the adapter +# (or degrade to prompt text) instead of returning -32601 for the whole call. +# +# The same membership decides who reads the workspace ``cli.json`` overlay: the +# kiro-family harnesses take effort and Tool Search from that file at spawn, and +# writing it for a harness that never reads it leaves a stale file in the user's +# workspace that no later clear can reach. +ACP_BACKENDS_KIRO_SLASH_COMMANDS = frozenset({ACP_BACKEND_KIRO, ACP_BACKEND_KAS}) diff --git a/src/kiro_crew/providers/acp.py b/src/kiro_crew/providers/acp.py index 4a6c3f819e8..9a69ab35233 100644 --- a/src/kiro_crew/providers/acp.py +++ b/src/kiro_crew/providers/acp.py @@ -23,19 +23,24 @@ from kiro_crew.acp.session_handle import AcpSessionHandle from kiro_crew.acp.session_provider import AcpSessionProvider from kiro_crew.acp.types import ( + ACP_BACKEND_CODEX, ACP_BACKEND_KAS, ACP_BACKEND_KIRO, ACP_BACKENDS_ACP_RUNTIME, + ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION, ACP_BACKENDS_KIRO_IDENTITY_STORE, + ACP_BACKENDS_KIRO_SLASH_COMMANDS, ACP_BACKENDS_KNOWN, ACP_BACKENDS_SESSION_SHARING, EVENT_COMPACTION_STATUS, PROVIDER_LABEL_CLAUDE, + PROVIDER_LABEL_CODEX, PROVIDER_LABEL_DEFAULT, PROVIDER_LABEL_KAS, STOP_REASON_CANCELLED, STOP_REASON_END_TURN, ) +from kiro_crew.acp_backends import POLICY_ID_BY_BACKEND from kiro_crew.agent_sdk.backend_identity import is_claude_backend_name from kiro_crew.atomic_write import atomic_write from kiro_crew.config.paths import kiro_sessions_dir @@ -454,6 +459,11 @@ def is_claude_backend(self) -> bool: """ return is_claude_backend_name(self._client.backend) + @property + def is_codex_backend(self) -> bool: + """True when this ACP provider talks to codex-acp (vs kiro-cli).""" + return self._client.backend == ACP_BACKEND_CODEX + @property def is_kas_backend(self) -> bool: """True when this ACP provider talks to KAS (kiro-agent).""" @@ -957,11 +967,19 @@ def _resolve_effort(self) -> str | None: def _apply_effort_overlay(self) -> None: """Write the kiro workspace cli.json overlay for (current model, effort). - No-op for the claude backend (it uses live set_config_option) and when - the model is not effort-capable or no level resolves. Called before - every (re)spawn so resume/restart keeps the same level. + Written only for the harnesses that READ it + (``ACP_BACKENDS_KIRO_SLASH_COMMANDS`` — the kiro family takes effort from + this file at spawn); adapter harnesses use a live set_config_option push + instead. Also a no-op when the model is not effort-capable or no level + resolves. Called before every (re)spawn so resume/restart keeps the same + level. + + Membership rather than "not claude": the companion clear + (``_clear_cli_overlay_effort`` in :meth:`change_effort`) is already + membership-gated, so a negation here writes an overlay for a harness the + clear will never reach — a stale file left in the user's workspace. """ - if self.is_claude_backend: + if self._client.backend not in ACP_BACKENDS_KIRO_SLASH_COMMANDS: return model = self._client._model level = self._resolve_effort() @@ -976,11 +994,14 @@ def _apply_effort_overlay(self) -> None: def _apply_tool_search_overlay(self) -> None: """Write the kiro Tool Search setting into the workspace cli.json overlay. - No-op for the claude backend (Tool Search is a kiro-cli feature) and - when no toggle value was supplied (``self._tool_search is None``). - Called before every (re)spawn so resume/restart keeps the same setting. + Tool Search is a kiro-cli feature read from this file at spawn, so the + write is scoped to the harnesses that read it; a no-op as well when no + toggle value was supplied (``self._tool_search is None``). Called before + every (re)spawn so resume/restart keeps the same setting. """ - if self.is_claude_backend or self._tool_search is None: + if self._client.backend not in ACP_BACKENDS_KIRO_SLASH_COMMANDS: + return + if self._tool_search is None: return try: _write_tool_search_overlay( @@ -1003,8 +1024,13 @@ def _apply_tool_search_overlay(self) -> None: except Exception: logger.warning("ACP tool-search overlay write failed", exc_info=True) - async def _set_claude_effort(self, level: str) -> None: - """Push an effort level to the claude backend, stepping down on reject. + async def _set_effort_config_option(self, level: str) -> None: + """Push an effort level over ``session/set_config_option``, stepping down + on reject. + + Named for the CHANNEL, not a harness: every member of + ``ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION`` arrives here, and the + adapter-behaviour notes below are what that channel does in practice. claude-agent-acp validates the value against the *current model's* ``supportedEffortLevels`` and throws ``Invalid value for config option @@ -1023,7 +1049,7 @@ async def _set_claude_effort(self, level: str) -> None: spam errors and trigger a session reset on every turn. """ if not self._client.supports_config_option("effort"): - logger.debug("claude-agent-acp exposes no 'effort' config option; skipping effort push") + logger.debug("adapter exposes no 'effort' config option; skipping effort push") return # Descend from the requested level through lower levels (e.g. # max → xhigh → high). Never escalate above what was asked. @@ -1063,21 +1089,34 @@ async def _set_claude_effort(self, level: str) -> None: async def change_effort(self, level: str) -> bool: """Change effort live for the current model. Returns True on success. - Persists the slot override and (kiro) rewrites the overlay so a later - respawn keeps the level, then pushes the change to the running session: - kiro via the ``/effort`` slash command, claude via - ``session/set_config_option``. Returns False when the current model - does not support effort. + Persists the slot override and (kiro family) rewrites the overlay so a + later respawn keeps the level, then pushes the change to the running + session: the ``/effort`` slash command for + ``ACP_BACKENDS_KIRO_SLASH_COMMANDS``, ``session/set_config_option`` for + ``ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION``. Returns False when the current + model does not support effort, or when this harness is in neither set and + so has no effort channel at all. """ model = self._client._model if not model_supports_effort(model): logger.info("change_effort skipped — model %s does not support effort", model) return False - # Older claude-agent-acp builds advertise no 'effort' config option; - # attempting to push would fail with 'Unknown config option' and reset - # the session. Report unsupported so the dashboard leaves the UI as-is. - if self.is_claude_backend and not self._client.supports_config_option("effort"): - logger.info("change_effort skipped — claude-agent-acp build exposes no 'effort' option") + via_config_option = self._client.backend in ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION + via_slash_command = self._client.backend in ACP_BACKENDS_KIRO_SLASH_COMMANDS + if not (via_config_option or via_slash_command): + # Both channels are opt-in, so a harness in neither has to be reported + # unsupported here. Guessing one would push into a verb the adapter + # does not implement and reset the session on a -32601. + logger.info( + "change_effort skipped — backend %r implements no effort channel", + self._client.backend, + ) + return False + # An adapter build may advertise no 'effort' config option; attempting to + # push would fail with 'Unknown config option' and reset the session. + # Report unsupported so the dashboard leaves the UI as-is. + if via_config_option and not self._client.supports_config_option("effort"): + logger.info("change_effort skipped — adapter build exposes no 'effort' option") return False # Accept any level the dynamic validation set knows about — ACP backends # can report levels beyond the canonical five (effort.py), and those are @@ -1097,8 +1136,8 @@ async def change_effort(self, level: str) -> bool: self._effort_per_model[model] = level self._apply_effort_overlay() try: - if self.is_claude_backend: - await self._set_claude_effort(level) + if via_config_option: + await self._set_effort_config_option(level) else: await self._client.send_command("/effort", args={"level": level}) except Exception: @@ -1118,7 +1157,11 @@ async def change_effort(self, level: str) -> bool: "ACP effort live-changed: model=%s effort=%s backend=%s", model, level, - "claude" if self.is_claude_backend else "kiro", + # Log the harness by its policy-facing name: ACP_BACKEND_KIRO is the + # empty string, so the raw value would read as a missing field, and a + # two-way "claude or kiro" label would name the wrong harness for + # every backend added after it. + POLICY_ID_BY_BACKEND.get(self._client.backend, self._client.backend), ) return True @@ -1129,10 +1172,11 @@ async def clear_effort(self) -> bool: falls back to its own default. Returns True ONLY when a concrete default was applied LIVE to the - running session (kiro with a resolvable workspace default). Returns - False — signalling the caller to reset the session so a cold start - re-resolves the true default — in the cases that cannot be reset live: - - claude-agent-acp has no "reset to default" config value (there is no + running session (kiro family with a resolvable workspace default). + Returns False — signalling the caller to reset the session so a cold + start re-resolves the true default — in the cases that cannot be reset + live: + - the config-option channel has no "reset to default" value (there is no level to push; only a respawn drops the override), and - kiro with no workspace default (the model's built-in default is only re-applied on respawn once the overlay is cleared). @@ -1143,11 +1187,17 @@ async def clear_effort(self) -> bool: if not model_supports_effort(model): return False self._effort_per_model.pop(model, None) - if self.is_claude_backend: - # No live "reset to default" — caller must reset the session. - logger.info("ACP effort cleared (claude); session reset needed for default") + if self._client.backend not in ACP_BACKENDS_KIRO_SLASH_COMMANDS: + # No live "reset to default" — caller must reset the session. Scoped + # by membership so a harness that reads neither the overlay nor the + # slash command is not sent down the kiro path below, which would + # write an overlay it ignores and then push a verb it lacks. + logger.info( + "ACP effort cleared (%s); session reset needed for default", + POLICY_ID_BY_BACKEND.get(self._client.backend, self._client.backend), + ) return False - # kiro: clear/rewrite the overlay so a respawn doesn't re-apply it. + # kiro family: clear/rewrite the overlay so a respawn doesn't re-apply it. level = self._resolve_effort() # workspace default, or None if level: self._apply_effort_overlay() @@ -1195,7 +1245,7 @@ async def _apply_initial_effort(self) -> None: if not level: return try: - await self._set_claude_effort(level) + await self._set_effort_config_option(level) logger.info("CC initial effort applied: model=%s effort=%s", self._client._model, level) except Exception: logger.warning( @@ -1259,15 +1309,18 @@ async def stream(self, message: str) -> AsyncIterator[LLMEvent]: yield self._to_llm_event(e) async def stream_command(self, command: str) -> AsyncIterator[LLMEvent]: - # claude-agent-acp does not implement the kiro-only - # _kiro.dev/commands/execute method — route slash commands through - # session/prompt, which the claude backend interprets natively for its - # SDK-supported commands (/compact, /help, /model, /context, …). - # Commands the SDK doesn't recognise (kiro-only ones like /agent, - # /experiment, /hooks) flow through as conversational prompt text; - # this is a softer failure mode than the previous -32601 - # "Method not found" hard error. - if self.is_claude_backend: + # _kiro.dev/commands/execute is a kiro extension, so only + # ACP_BACKENDS_KIRO_SLASH_COMMANDS members can be sent it. Everyone else + # routes through session/prompt, which an adapter interprets natively for + # the commands its SDK supports (/compact, /help, /model, /context, …). + # Commands it doesn't recognise (kiro-only ones like /agent, /experiment, + # /hooks) flow through as conversational prompt text — a softer failure + # mode than a -32601 "Method not found" on the whole call. + # + # Membership rather than "not claude": the RPC is the narrow capability + # here, so a harness added later must opt in to it, not inherit it and + # hard-error on every slash command a user types. + if self._client.backend not in ACP_BACKENDS_KIRO_SLASH_COMMANDS: async for e in self._client.stream_events(command): yield self._to_llm_event(e) return @@ -1533,4 +1586,6 @@ def provider_label(provider: Any) -> str: return PROVIDER_LABEL_CLAUDE if backend == ACP_BACKEND_KAS: return PROVIDER_LABEL_KAS + if backend == ACP_BACKEND_CODEX: + return PROVIDER_LABEL_CODEX return PROVIDER_LABEL_DEFAULT diff --git a/test/test_acp_backend_kas.py b/test/test_acp_backend_kas.py index 64658ff114c..8eda6268291 100644 --- a/test/test_acp_backend_kas.py +++ b/test/test_acp_backend_kas.py @@ -25,6 +25,7 @@ from kiro_crew.acp.session_provider import AcpSessionProvider from kiro_crew.acp.types import ( ACP_BACKEND_CLAUDE, + ACP_BACKEND_CODEX, ACP_BACKEND_KAS, ACP_BACKEND_KIRO, ACP_BACKENDS_KNOWN, @@ -78,17 +79,38 @@ def test_exactly_one_predicate_holds_for_every_known_backend(self, backend): provider.is_kiro_backend, provider.is_claude_backend, provider.is_kas_backend, + provider.is_codex_backend, ] assert sum(held) == 1 - @pytest.mark.parametrize("backend", sorted(ACP_BACKENDS_KNOWN)) + @pytest.mark.parametrize( + "backend", sorted({ACP_BACKEND_KIRO, ACP_BACKEND_CLAUDE, ACP_BACKEND_KAS}) + ) def test_acp_runtime_backend_is_the_positive_form_of_not_claude(self, backend): # The four provider sites that used to read ``not is_claude_backend`` - # now read ``is_acp_runtime_backend``; the two must stay equivalent for - # every known backend so the conversion is behavior-preserving. + # now read ``is_acp_runtime_backend``; the two must stay equivalent for the + # backends that conversion covered, so it is behavior-preserving. + # + # Scoped to those three deliberately, NOT to ACP_BACKENDS_KNOWN. The + # equivalence is an artifact of a world with one non-runtime harness: any + # further harness that is also off AcpRuntime (codex is the first) makes + # "not claude" and "not on the runtime" different questions. Widening the + # parametrisation would demand that every new harness be claude-shaped, + # which is the negation-based reasoning H5/H6 exist to retire; membership in + # ACP_BACKENDS_ACP_RUNTIME is the durable form and is pinned separately. provider = _build_provider(backend) assert provider.is_acp_runtime_backend is (not provider.is_claude_backend) + def test_codex_is_not_on_the_acp_runtime(self): + """The membership the narrowing above defers to, pinned so the claim holds. + + codex-acp is one process per session, so it belongs on AcpClient. Adding it + to ACP_BACKENDS_ACP_RUNTIME would route an activated codex session onto + AcpRuntime + AcpSessionHandle — the wrong transport for a per-session Node + adapter — and without this assertion that edit would land green. + """ + assert _build_provider(ACP_BACKEND_CODEX).is_acp_runtime_backend is False + class TestUnknownBackendRejected: """A typo must fail loudly rather than silently driving kiro-cli.""" @@ -214,9 +236,7 @@ def test_the_field_is_consumed_on_load(self, tmp_path): Asserted through a selectable value, since an unselectable one degrades and so cannot distinguish 'consumed' from 'dropped'. """ - cfg = _load_agent_config( - {"acp_backend": ACP_BACKEND_KIRO, "streaming": False}, tmp_path - ) + cfg = _load_agent_config({"acp_backend": ACP_BACKEND_KIRO, "streaming": False}, tmp_path) assert cfg.agent.acp_backend == ACP_BACKEND_KIRO assert cfg.agent.streaming is False diff --git a/test/test_acp_client.py b/test/test_acp_client.py index 4e65b1d202d..e4ca596260b 100644 --- a/test/test_acp_client.py +++ b/test/test_acp_client.py @@ -27,7 +27,7 @@ _make_unified_diff, _resolve_vendored_claude_acp, _substitute_model_from_advisory, - _vendored_claude_acp_roots, + _vendored_acp_roots, format_command_result, parse_slash_command, ) @@ -146,7 +146,7 @@ def test_skips_incomplete_copy_missing_deps(self, tmp_path, monkeypatch): def test_roots_include_pkg_vendor_dir(self): # The toolbox-bundle vendor location must always be the first candidate. - roots = _vendored_claude_acp_roots() + roots = _vendored_acp_roots() assert roots[0].name == "node_modules" and roots[0].parent.name == "_vendor" diff --git a/test/test_acp_provider.py b/test/test_acp_provider.py index 9151ee4ae11..b0980bc3721 100644 --- a/test/test_acp_provider.py +++ b/test/test_acp_provider.py @@ -584,7 +584,7 @@ async def test_claude_set_effort_swallows_unknown_config_option(self): ) ) # Must not raise. - await provider._set_claude_effort("max") + await provider._set_effort_config_option("max") @pytest.mark.asyncio async def test_kiro_clear_effort_no_default_returns_false_for_reset(self): diff --git a/test/test_agent_backend_editable.py b/test/test_agent_backend_editable.py index 24512413a83..dc5d63e04c5 100644 --- a/test/test_agent_backend_editable.py +++ b/test/test_agent_backend_editable.py @@ -17,13 +17,22 @@ import pytest from kiro_crew import acp_backends -from kiro_crew.acp_backends import ACP_BACKEND_CLAUDE, ACP_BACKEND_KAS, ACP_BACKEND_KIRO +from kiro_crew.acp_backends import ( + ACP_BACKEND_CLAUDE, + ACP_BACKEND_CODEX, + ACP_BACKEND_KAS, + ACP_BACKEND_KIRO, +) from kiro_crew.config.loader import KiroCrewConfig from kiro_crew.dashboard.handlers.agents import _supply_live_enum from kiro_crew.dashboard.handlers.core import _EDITABLE_CONFIG FIELD = "agent.acp_backend" +#: Known ids the public baseline deliberately does not offer, each entry carrying its +#: reason in ``test_baseline_ships_every_known_backend``. Empty is the healthy state. +NOT_SHIPPED_SELECTABLE = frozenset({ACP_BACKEND_CODEX}) + @pytest.fixture def restore_registry(): @@ -135,7 +144,14 @@ def test_baseline_ships_every_known_backend(): reason belongs next to that removal — a build that cannot run a harness is a different claim from a machine that has not installed it, and the install probe already answers the second one. + + ``NOT_SHIPPED_SELECTABLE`` is where that reason goes. It is an explicit list + rather than a relaxed assertion so a plain ``baseline != known`` still fails: + an id may sit outside the baseline only by being named there. Codex is the + only member — its spawn path is complete, but ``backend_install.py`` has no + probe for the adapter, so the switch would render with nothing to say about a + session that failed to start. """ baseline: List[str] = sorted(acp_backends.BASELINE_SELECTABLE_BACKENDS) assert baseline == sorted([ACP_BACKEND_KIRO, ACP_BACKEND_CLAUDE, ACP_BACKEND_KAS]) - assert baseline == sorted(acp_backends.ACP_BACKENDS_KNOWN) + assert baseline == sorted(acp_backends.ACP_BACKENDS_KNOWN - NOT_SHIPPED_SELECTABLE) diff --git a/test/test_agent_sdk_backend_install.py b/test/test_agent_sdk_backend_install.py index c704cb123fd..967cc116a15 100644 --- a/test/test_agent_sdk_backend_install.py +++ b/test/test_agent_sdk_backend_install.py @@ -477,7 +477,7 @@ def test_owner_gets_one_row_per_backend_in_the_pinned_shape(self, monkeypatch): assert response.status == 200 rows = json.loads(response.text or "{}")["backends"] - assert [r["policy_id"] for r in rows] == ["claude", "kas", "kiro"] + assert [r["policy_id"] for r in rows] == ["claude", "codex", "kas", "kiro"] for row in rows: assert set(row) == { "id", @@ -507,6 +507,15 @@ def test_owner_gets_one_row_per_backend_in_the_pinned_shape(self, monkeypatch): probe.COMPONENT_CLAUDE_ACP_ADAPTER, probe.COMPONENT_CLAUDE_CODE_CLI, ] + # codex is in ACP_BACKENDS_KNOWN with no entry in ``_PROBES``, so it gets a + # row -- the endpoint lists every id the switch can show -- but the row can + # only say ``unknown`` and must name nothing to install. That gap is why + # codex is absent from BASELINE_SELECTABLE_BACKENDS: offering the switch + # would offer a verdict this payload cannot supply. + assert by_policy["codex"]["installed"] == "unknown" + assert by_policy["codex"]["missing_components"] == [] + assert by_policy["codex"]["install_command"] == "" + assert by_policy["codex"]["selectable"] is False def test_an_unknown_row_names_no_components(self, monkeypatch): """The three-state rule, enforced at the payload boundary too. diff --git a/test/test_harness_parity.py b/test/test_harness_parity.py index 6dfc284638f..603b1a231d6 100644 --- a/test/test_harness_parity.py +++ b/test/test_harness_parity.py @@ -20,6 +20,7 @@ import subprocess import sys from dataclasses import fields +from unittest.mock import MagicMock import pytest @@ -28,6 +29,7 @@ from kiro_crew.acp import runtime as acp_runtime from kiro_crew.acp.types import ( ACP_BACKEND_CLAUDE, + ACP_BACKEND_CODEX, ACP_BACKEND_KAS, ACP_BACKEND_KIRO, ACP_BACKENDS_ACP_RUNTIME, @@ -38,10 +40,14 @@ ACP_CLIENT_CAPABILITIES, KAS_CLIENT_CAPABILITIES, PROVIDER_LABEL_CLAUDE, + PROVIDER_LABEL_CODEX, PROVIDER_LABEL_DEFAULT, PROVIDER_LABEL_KAS, ) from kiro_crew.acp_backends import ( + ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION, + ACP_BACKENDS_KIRO_SLASH_COMMANDS, + ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION, BASELINE_SELECTABLE_BACKENDS, selectable_backends, ) @@ -377,6 +383,7 @@ def test_every_known_backend_has_a_label() -> None: ACP_BACKEND_KIRO: PROVIDER_LABEL_DEFAULT, ACP_BACKEND_CLAUDE: PROVIDER_LABEL_CLAUDE, ACP_BACKEND_KAS: PROVIDER_LABEL_KAS, + ACP_BACKEND_CODEX: PROVIDER_LABEL_CODEX, } assert set(labels) == set(ACP_BACKENDS_KNOWN), ( "a known backend has no PROVIDER_LABEL_* of its own, so it would persist " @@ -386,6 +393,155 @@ def test_every_known_backend_has_a_label() -> None: assert len(set(labels.values())) == len(labels), "two backends share a label" +def test_codex_is_known_but_not_shipped_selectable() -> None: + """H1/H8: a switch a build cannot answer for must not be offered by default. + + This is not the stance ``claude`` has: claude is baseline-selectable because + ``client.py`` owns its spawn path and its adapter is a public npm package — + both true of codex now too. What codex still lacks is the other half, + ``backend_install.py``'s probe: without one its install row can only read + ``unknown``, so a failed session arrives with nothing to act on. + ``register_selectable_backend`` is the way in until that probe lands. + """ + assert ACP_BACKEND_CODEX in ACP_BACKENDS_KNOWN + assert ACP_BACKEND_CODEX not in BASELINE_SELECTABLE_BACKENDS + assert ACP_BACKEND_CODEX not in selectable_backends() + + +def test_codex_carries_its_own_provider_label() -> None: + """H11: the label is what keeps a codex session out of the kiro namespace. + + Resume compatibility, session-map persistence and session-file cleanup all index + this key, so a codex session labelled ``acp`` would be resumed as kiro and then + pruned for want of a kiro transcript. + """ + client = MagicMock() + client.backend = ACP_BACKEND_CODEX + provider = MagicMock(spec=providers_acp.AcpProvider) + provider.client = client + assert providers_acp.provider_label(provider) == PROVIDER_LABEL_CODEX + assert PROVIDER_LABEL_CODEX != PROVIDER_LABEL_DEFAULT + + +def test_model_switch_channel_is_opt_in() -> None: + """H6: the config-option model channel is granted by membership, not negation. + + kiro-cli switches models with ``session/set_model``; the claude and codex + adapters implement no such request and expose the model as a session config + option instead. Read as ``not is_kiro`` this would hand the config-option path + to every harness added later, and a harness that implements neither would + silently no-op its model switch. + """ + assert ACP_BACKEND_CLAUDE in ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION + assert ACP_BACKEND_CODEX in ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION + assert ACP_BACKEND_KIRO not in ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION + assert ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION <= ACP_BACKENDS_KNOWN + source = "\n".join( + ( + inspect.getsource(acp_client.AcpClient.set_model), + inspect.getsource(acp_client.AcpClient._apply_startup_model), + ) + ) + assert ( + "ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION" in source + ), "the model switch must read the membership set, not a per-backend literal" + + +def test_effort_channel_is_opt_in() -> None: + """H6: the effort channel is granted by membership, not by "not claude". + + The two channels are separate opt-ins because a harness can have neither. Read + as ``not is_claude_backend``, an adapter harness is handed kiro's ``/effort`` + slash command, which rides ``_kiro.dev/commands/execute`` — a verb it does not + implement — so the push fails -32601 and the dashboard resets the session. + """ + assert ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION <= ACP_BACKENDS_KNOWN + assert ACP_BACKENDS_KIRO_SLASH_COMMANDS <= ACP_BACKENDS_KNOWN + # Disjoint: a harness must not be told to push effort down both channels. + assert not (ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION & ACP_BACKENDS_KIRO_SLASH_COMMANDS) + assert ACP_BACKEND_KIRO in ACP_BACKENDS_KIRO_SLASH_COMMANDS + assert ACP_BACKEND_CODEX in ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION + assert ACP_BACKEND_CODEX not in ACP_BACKENDS_KIRO_SLASH_COMMANDS + source = "\n".join( + ( + inspect.getsource(providers_acp.AcpProvider.change_effort), + inspect.getsource(providers_acp.AcpProvider.clear_effort), + inspect.getsource(providers_acp.AcpProvider._apply_effort_overlay), + inspect.getsource(providers_acp.AcpProvider._apply_tool_search_overlay), + inspect.getsource(providers_acp.AcpProvider.stream_command), + ) + ) + assert "is_claude_backend" not in source, ( + "the effort, overlay and slash-command seams must read a membership set; " + "a claude test here decides the path for every harness added later" + ) + + +def test_only_overlay_readers_are_written_to() -> None: + """H6: the cli.json overlay is written only for the harnesses that read it. + + The clear side (``_clear_cli_overlay_effort``) is membership-gated, so a write + gated on anything wider leaves a stale overlay in the user's workspace that no + later clear can reach — and the overlay names an effort level, so a harness + that DOES read the file later inherits a level nobody set for it. + """ + for fn in ( + providers_acp.AcpProvider._apply_effort_overlay, + providers_acp.AcpProvider._apply_tool_search_overlay, + ): + source = inspect.getsource(fn) + assert ( + "ACP_BACKENDS_KIRO_SLASH_COMMANDS" in source + ), f"{fn.__name__}: overlay write is not scoped to the overlay's readers" + + +def test_codex_spawn_keeps_its_own_branch() -> None: + """H9/H10: codex resolves its own adapter and declares its own handshake. + + Falling through to the kiro branch would spawn kiro-cli under a codex label — + the exact failure ACP_BACKENDS_KNOWN's rejection exists to prevent one step + earlier — and folding its protocol version into the claude literal would make a + future divergence a silent downgrade for whichever harness moved first. + """ + spawn_source = inspect.getsource(acp_client.AcpClient._spawn) + assert "_is_codex" in spawn_source + assert "_resolve_codex_acp_bin" in spawn_source + assert acp_client.PROTOCOL_VERSION_CODEX is not None + assert "PROTOCOL_VERSION_CODEX" in inspect.getsource(acp_client.AcpClient._initialize_session) + + +def test_each_mcp_seam_is_spliced_only_for_its_own_harness() -> None: + """H6: a per-harness hook must not reach a session of a different harness. + + Both defaults return ``[]``, so an ungated splice is inert in this tree — but an + edition that overrides both hooks would hand a claude session codex's server + entries and vice versa, and an entry whose transport the adapter does not + advertise fails the whole ``session/new`` rather than being skipped. Pinned at + the source, in the file's existing idiom, because the splice sits inside an + async session-setup path with no unit-level seam. + """ + for fn in ( + acp_client.AcpClient._new_session_following_substitution, + acp_client.AcpClient._initialize_session, + ): + source = inspect.getsource(fn) + if "_codex_session_mcp_servers" not in source: + continue + assert "if self._is_codex" in source, f"{fn.__name__}: codex seam spliced ungated" + assert "if self._is_claude" in source, f"{fn.__name__}: claude seam spliced ungated" + + +def test_codex_mcp_seam_defaults_to_empty() -> None: + """The public core sends no mcpServers for codex, exactly as for claude. + + kiro-cli receives its servers through ``--agent``; an edition overrides the seam. + A non-empty default here would put servers on a public session that the adapter + was never configured for. + """ + client = acp_client.AcpClient.__new__(acp_client.AcpClient) + assert client._codex_session_mcp_servers() == [] + + def test_model_preflight_allows_unknown_advertised_set() -> None: """H12: an empty or unknown advertised set means allow. diff --git a/website/src/pages/developer/AgentBackendTab.tsx b/website/src/pages/developer/AgentBackendTab.tsx index c21d0110164..15217fae0a4 100644 --- a/website/src/pages/developer/AgentBackendTab.tsx +++ b/website/src/pages/developer/AgentBackendTab.tsx @@ -1,6 +1,6 @@ import { useState } from 'react' import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query' -import { Bot, Sparkles, Terminal } from 'lucide-react' +import { Bot, Boxes, Sparkles, Terminal } from 'lucide-react' import { api } from '../../api/client' import type { AcpBackendProbe } from '../../api/client' @@ -21,6 +21,17 @@ const KIRO = '' const CLAUDE = 'claude' const KAS = 'kas' +/** + * The agents this frontend has a translated name and an icon for. + * + * A FLOOR for what the panel renders, never a ceiling — see `candidates`. Every id + * here is a core agent the server always knows, so listing them costs nothing and + * keeps the control populated while the schema and probe queries are still in + * flight. An agent absent from this list still gets a row once a server answer + * names it, labelled with its `policy_id`. + */ +const NAMED = [KIRO, CLAUDE, KAS] + /** * DOM id of the row that states a backend's status. * @@ -63,9 +74,12 @@ const PROBE_REFRESH_MS = 30_000 * values the wire accepts cannot disagree, and a build that ships another agent * lights it up here with no frontend change. * - * Claude Code is the case that makes this worth doing: this build does not include - * it. Hiding it would imply it does not exist; enabling it would produce a 400 from - * a control that looked live. It is listed, disabled, and says which it is. + * That last clause is why `candidates` is a union of server answers rather than a + * list of ids written here. An earlier revision filtered a hard-coded + * `[KIRO, CLAUDE, KAS]` by the schema, which narrows correctly and can never widen — + * so an agent an edition registered through `register_selectable_backend` was + * selectable on the wire and invisible in the only control that sets it. Ids this + * frontend has no translated name for render under their `policy_id`. * * ## Why there is a SECOND gate, and why it is allowed to say nothing * @@ -222,6 +236,57 @@ export function AgentBackendTab() { const unavailable = (value: string) => (selectable ? !selectable.includes(value) : false) || probe(value)?.selectable === false + /** + * Every agent id this panel could render, from the SERVER rather than a literal. + * + * This used to be `[KIRO, CLAUDE, KAS]`, which quietly made the panel the last + * hard-coded copy of the selectable list — the very thing + * `register_selectable_backend` exists to retire. Filtering a literal by the live + * schema narrows correctly but can never WIDEN, so an agent an edition registered + * was selectable on the wire, valid to PATCH, present in the probe payload, and + * absent from this control. The module note above already promised the opposite + * ("a build that ships another agent lights it up here with no frontend change"); + * this is what makes that true. + * + * Union of the schema enum and the probe payload, because the two answer different + * questions and either can be in flight: the enum is what PATCH accepts, the probe + * is every id the core knows (including ones this build cannot select, which + * `unavailable` then drops). + * + * `NAMED` is unioned in as a FLOOR, not a ceiling, and the distinction is the whole + * fix. As a ceiling it capped the panel at three ids forever. As a floor it only + * guarantees the core agents still have rows when neither query has answered — + * which the loading behaviour requires, since hiding a row on absent information is + * the same mistake as disabling one. `current` joins for the same reason: the saved + * value must always have a chip. + * + * Sorted rather than left in arrival order: KIRO first because it is the default + * and the floor, then by `policy_id`, which is the order the probe endpoint already + * sorts by. Set iteration order would otherwise follow whichever query resolved + * first and reshuffle the control between renders. + */ + const candidates = Array.from( + new Set([ + ...NAMED, + current, + ...(selectable ?? []), + ...(probeQ.data?.backends ?? []).map(b => b.id), + ]), + ).sort((a, b) => { + if (a === KIRO) return -1 + if (b === KIRO) return 1 + // Byte order, not `localeCompare`/`compareText`: these are machine identifiers, + // and the point of the sort (see above) is to reproduce the order the probe + // endpoint already returned them in. A collator reads the READER's locale, so + // the same deployment would order the chips differently per browser -- the + // between-render reshuffle this sort exists to prevent, just keyed on locale + // instead of query timing. + const ka = probe(a)?.policy_id || a + const kb = probe(b)?.policy_id || b + if (ka === kb) return 0 + return ka < kb ? -1 : 1 + }) + /** * The agents this panel renders at all. * @@ -234,7 +299,7 @@ export function AgentBackendTab() { * persisted value to the floor on load, so this should not arise; if it ever does, * a control rendering no selected chip is a worse failure than one extra row. */ - const visible = [KIRO, CLAUDE, KAS].filter(value => value === current || !unavailable(value)) + const visible = candidates.filter(value => value === current || !unavailable(value)) /** * Installed === 'missing' is the only verdict that disables. `'unknown'` and an @@ -275,6 +340,12 @@ export function AgentBackendTab() { const caveat = (value: string): string => value === CLAUDE ? i18nT('pages.developer.agentBackendTab.claude_uses_its_own_permissions') : '' + /** + * Translated display names for the agents this frontend knows by name. + * + * Deliberately NOT the list of agents the panel renders — see `candidates`. An id + * absent here still gets a row; `nameOf` falls back to the server's `policy_id`. + */ const NAME: Record = { [KIRO]: i18nT('pages.developer.agentBackendTab.kiro_cli'), [CLAUDE]: i18nT('pages.developer.agentBackendTab.claude_code'), @@ -287,6 +358,27 @@ export function AgentBackendTab() { [KAS]: , } + /** + * A label for any selectable id, known to this frontend or not. + * + * The fallback is the server's `policy_id`, which exists precisely to be a + * human-readable wire name (`acp_backends.POLICY_ID_BY_BACKEND`) — it is what a + * governance rule spells, so it is already a word rather than an internal token. + * Untranslated, and that is the deliberate trade: a registered agent rendering + * under its policy name is legible, whereas `NAME[value]` returning `undefined` + * renders a chip with no text at all. A core agent that ships selectable gets a + * real translated entry above; this keeps a plugin-registered one usable until + * then. + * + * KIRO is the empty string, so the `||` chain must not treat it as absent — it is + * always in NAME, which is why the lookup comes first. + */ + const nameOf = (value: string): string => NAME[value] || probe(value)?.policy_id || value + + /** Generic mark for an agent this frontend has no icon for. */ + const iconOf = (value: string): React.ReactNode => ICON[value] ?? + + /** * The one status line a row carries, derived rather than authored per agent. * @@ -332,8 +424,8 @@ export function AgentBackendTab() { disabled={patchMut.isPending} options={visible.map(value => ({ value, - label: NAME[value], - icon: ICON[value], + label: nameOf(value), + icon: iconOf(value), disabled: disabledOption(value), describedById: statusId(value), }))} @@ -347,7 +439,7 @@ export function AgentBackendTab() { {visible.map(value => (
- {NAME[value]} + {nameOf(value)}
{ expect(patchConfigMock).not.toHaveBeenCalled() }) + it('offers an agent this frontend has no name for, under its policy id', async () => { + // The case a hard-coded candidate list could not express. An edition calls + // `register_selectable_backend`, so the id reaches the schema enum and the probe + // payload -- but nothing in this file knows it exists. Before, the row was + // filtered out of a literal `[KIRO, CLAUDE, KAS]` and the only control that sets + // `agent.acp_backend` could not offer a backend the wire already accepted. + // + // `policy_id` carries the label because it is the name a governance rule spells, + // so it is already a word rather than an internal token. Untranslated on purpose: + // legible beats a chip with no text, and a core agent that ships selectable earns + // a real translated entry instead. + schemaMock.mockReturnValue(schemaWith(['', 'claude', 'kas', 'codex'])) + acpBackendsMock.mockResolvedValue({ + backends: [probeRow(''), probeRow('claude'), probeRow('kas'), probeRow('codex')], + }) + wrap() + await waitFor(() => expect(button('codex')).toBeEnabled()) + + // Reachable, not merely rendered: the click has to write the id the wire accepts. + fireEvent.click(button('codex')) + await waitFor(() => expect(patchConfigMock).toHaveBeenCalledWith('agent.acp_backend', 'codex')) + }) + + it('hides a known-but-unselectable agent even when the probe lists it', async () => { + // `GET /api/acp-backends` returns a row per id the CORE knows, which is a wider + // set than the deployment may select -- codex ships known and not selectable. So + // widening `candidates` to the probe payload must not smuggle in a row the schema + // excludes, or the panel would offer an option PATCH answers 400 for. + schemaMock.mockReturnValue(schemaWith(['', 'claude', 'kas'])) + acpBackendsMock.mockResolvedValue({ + backends: [ + probeRow(''), + probeRow('claude'), + probeRow('kas'), + probeRow('codex', { selectable: false }), + ], + }) + wrap() + await waitFor(() => expect(button('Kiro CLI')).toBeEnabled()) + expect(screen.queryByRole('button', { name: 'codex' })).not.toBeInTheDocument() + expect(screen.queryByText('codex')).not.toBeInTheDocument() + }) + it('saves the Claude Code selection the shipped build offers', async () => { wrap() await waitFor(() => expect(button('Claude Code')).toBeEnabled()) From c21469892cf043bf784ddaa966e92f559fa0c7d8 Mon Sep 17 00:00:00 2001 From: zejiangg Date: Wed, 2 Sep 2026 01:27:17 +0000 Subject: [PATCH 2/2] docs: name the sequence a new ACP harness walks to land MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `harness-parity.md` says what an added harness may not do to the Kiro path. Nothing said what it must do to land at all, so each onboarding rediscovered the order — and rediscovered it from KAS and Claude Code in retrospect, which is the wrong direction: the two harnesses that predate the invariants cannot show which stage a third one gets stuck at. Derived from the Codex seam instead, which makes the gap visible as a gap. Two stages here did not exist as stages until a third harness needed them. Stage 2 had five capability sets and no tuning channels at all; the Codex work took it to eight, one set per channel because a harness can implement one and not another. Stage 5 (the install probe) was invisible while every known id happened to be selectable, and it is the reason Codex lands dormant — one named stage, not a re-litigation. Also fixes a contradiction the Codex seam left in `harness-parity.md`: the intro correctly describes Codex as a named exception carried in `NOT_SHIPPED_SELECTABLE`, while point 4 of "Adding or changing an invariant" still asserted `ACP_BACKENDS_KNOWN` and `BASELINE_SELECTABLE_BACKENDS` were equal and that any id outside the baseline fails the narrowing check. They are no longer equal, and the check now accepts a named exception. --- docs/system-specs/modules/README.md | 1 + .../modules/harness-onboarding.md | 207 ++++++++++++++++++ docs/system-specs/modules/harness-parity.md | 14 +- 3 files changed, 217 insertions(+), 5 deletions(-) create mode 100644 docs/system-specs/modules/harness-onboarding.md diff --git a/docs/system-specs/modules/README.md b/docs/system-specs/modules/README.md index 4f63d0db9aa..9a337e65b6b 100644 --- a/docs/system-specs/modules/README.md +++ b/docs/system-specs/modules/README.md @@ -15,6 +15,7 @@ agent loads only the one it needs. | [acp-client.md](acp-client.md) | The ACP JSON-RPC client that drives `kiro-cli`: transport, framing, timeouts, and the backend seam. | | [providers.md](providers.md) | The `LLMProvider` interface and the KiroACP-only provider surface. | | [harness-parity.md](harness-parity.md) | The invariants keeping the Kiro harness first-class while other harnesses are adapted, and the test pinning each. | +| [harness-onboarding.md](harness-onboarding.md) | The sequence a new ACP harness walks to land: vocabulary, capability decisions, spawn path, handshake, install probe, selectability, and what a live harness additionally touches. | | [kas-backend.md](kas-backend.md) | The second, adapted ACP backend: how Crew selects and adapts it (the `kas_wire` seam, harness-parity, ABC defaults) and the deferred hooks / transport / `/clear` items. Crew-side only. | | [session.md](session.md) | Sessions, slots, session keys, the warm pool, and PID tracking. | | [history.md](history.md) | Conversation persistence, JSONL rotation, and transcript search. | diff --git a/docs/system-specs/modules/harness-onboarding.md b/docs/system-specs/modules/harness-onboarding.md new file mode 100644 index 00000000000..62e6909123d --- /dev/null +++ b/docs/system-specs/modules/harness-onboarding.md @@ -0,0 +1,207 @@ +# Onboarding a new ACP harness + +[harness-parity.md](harness-parity.md) says what a new harness may **not** do to +the Kiro path. This file says what it **must** do to land at all, in the order +the work actually falls out. + +The sequence below is derived from onboarding Codex (`ACP_BACKEND_CODEX`), not +reconstructed from KAS and Claude Code after the fact. That matters, because two +of the stages here did not exist as stages until a third harness needed them: +Stage 2 had five capability sets and no tuning channels at all, and Stage 5 +was invisible while every known id happened to be selectable. A harness +that walks this list will find gaps the list does not predict; when it does, the +gap belongs here in the same change, as a stage — not in the harness's own +module as a special case. + +## The two landing states + +A harness lands in one of two states, and choosing between them is Stage 6, not +Stage 1: + +- **Dormant.** The core can *spell* the id — it is in `ACP_BACKENDS_KNOWN`, it + has a provider label, a policy name, and a decided membership in every + capability set — but no operator can *choose* it. A dormant harness is not a + stub: its spawn path can be complete. It is dormant because something a real + session depends on cannot yet answer for it. +- **Selectable.** The id is in `BASELINE_SELECTABLE_BACKENDS`, or an edition + called `register_selectable_backend`, so it renders in the dashboard switch + and survives a config load. + +Dormant is a legitimate destination, and shipping there deliberately is cheaper +than a long-lived branch. But it must be *named* as an exception (Stage 6), or +the narrowing check fails. + +## Stage 1 — the vocabulary, in the leaf + +Everything a consumer needs to *name* your harness goes in +`src/kiro_crew/acp_backends.py`, which imports no ACP and therefore may be +imported by anything: + +| Add | Why there | +|---|---| +| `ACP_BACKEND_` | The id. Never a bare literal at a call site (H5). | +| membership in `ACP_BACKENDS_KNOWN` | `AcpProvider.__init__` rejects anything outside it (H8), and every capability set is asserted a subset of it. | +| `PROVIDER_LABEL_` in `acp/types.py` | A closed mapping; an absent label means Kiro, so a harness without one persists as a Kiro session and has its transcript pruned for want of a Kiro session file (H11). | +| an entry in `POLICY_ID_BY_BACKEND` | A governance rule is written by a human as an identifier. The mapping is what makes the id nameable in a deny rule **before** anything registers it — so this is required even for a dormant harness. | + +`acp/types.py` re-exports the vocabulary, so existing callers keep their import +site. Do not define the constants there: it is a forbidden root for the SDK +boundary gate, and a definition there is a definition consumers cannot reach +without crossing it. + +## Stage 2 — an explicit decision for every capability set + +There are eight sets. **"Inherited the default" is not a decision** — a +capability is granted by opt-in membership, never by negation (H6), so a set you +do not think about is a set you have silently opted out of. That is usually +right, and it must still be deliberate, because the review lane and the tests +both read the membership as a claim. + +| Set | Grants | +|---|---| +| `ACP_BACKENDS_SESSION_SHARING` | One process may serve several sessions. Wrong membership hands a second session to a process that cannot hold it. | +| `ACP_BACKENDS_STEER` | The `_session/steer` extension. A steer sent to a non-implementer answers `-32601`. | +| `ACP_BACKENDS_INTERNAL_SANDBOX` | The harness sandboxes itself, so Kiro Crew's own wrapper stands down. Security-relevant: wrong membership hands isolation to a layer that never starts (H7). | +| `ACP_BACKENDS_ACP_RUNTIME` | Driven through `AcpRuntime` rather than its own spawn branch. | +| `ACP_BACKENDS_KIRO_IDENTITY_STORE` | Reads Kiro's identity/credential store. | +| `ACP_BACKENDS_MODEL_VIA_CONFIG_OPTION` | Model switching lands as a config option rather than a protocol call. | +| `ACP_BACKENDS_EFFORT_VIA_CONFIG_OPTION` | Reasoning-effort push, same channel shape. | +| `ACP_BACKENDS_KIRO_SLASH_COMMANDS` | Receives `_kiro.dev/commands/execute`, **and** gets the workspace `cli.json` overlay written for it. Membership decides both, so a non-member must not collect an overlay it never reads and the membership-gated clear can never remove. | + +The last three are one channel each rather than one "tuning" set, because a +harness can implement one and not another. If your harness needs a tuning +channel none of them describes, add a set — do not widen an existing one. + +## Stage 3 — the spawn path + +This is the irreducible new code, and on the harnesses measured so far it is the +largest single piece: `acp/client.py` grew between +194 and +806 lines per +harness. It is not reducible by refactoring, because it is the part that is +genuinely different. + +What a harness needs, using the Codex adapter as the shape: + +- **The adapter, and whether one is needed at all.** `codex-acp` exists because + the `codex` CLI does not serve ACP — it reads `acp` as a prompt. The adapter is + the transport, not an optimization. Establish this before anything else; a + harness that speaks ACP natively skips most of this stage. +- **Binary and package constants** (`CODEX_ACP_BIN`, `CODEX_ACP_NPM_PKG`) and + the package entry path. +- **A hoisted-dependency marker.** An adapter whose own dependencies are missing + dies at ESM import time — *after* the child is spawned, which is the worst + place to find out. +- **An explicit env override** (`CODEX_ACP_BIN`), spelled the way the adapter's + own documentation spells it. +- **Resolution order**: project-local `node_modules` first, then global/PATH. + Share the root discovery (`_vendored_acp_roots`) and join your own package + path onto it. Generalizing that helper is allowed; it is harness-neutral and + belongs to no harness. Adding a branch to the Kiro path is not (H13). + +Constants an adapter reads *itself* from the ambient environment do not get a +constant here. Naming one implies a forwarding that does not exist — the Codex +seam documents exactly this asymmetry against its Claude counterpart, which *is* +explicitly forwarded. + +## Stage 4 — the handshake, as your own literal + +Protocol version and client capabilities stay per-harness literals (H10). Give +your harness its own `PROTOCOL_VERSION_` **even when the number is +identical to an existing one.** That is not duplication: it makes a future +divergence a one-line edit here instead of a silent downgrade of whichever +harness happened to move first. + +## Stage 5 — the install probe + +`agent_sdk/backend_install.py` answers a question selectability does not: *is +this harness installed on this machine, and if not, what installs it?* It holds +one `_probe_` per harness in `_PROBES`, each returning a +`BackendInstallState` naming the missing component and the command that fixes +it. + +**This stage is the gate between dormant and selectable**, and it is the one +that is easy to skip because nothing fails without it. Nothing fails; the +operator does. A build that offers a switch with no probe behind it cannot tell +anyone what was missing when the session failed to start — the switch renders, +the session dies, and the dashboard has nothing to say. + +## Stage 6 — selectability, or a named exception + +With Stages 1–5 done, add the id to `BASELINE_SELECTABLE_BACKENDS`. + +If it is not done — most often Stage 5 — then the id is in +`ACP_BACKENDS_KNOWN` but not in the baseline, which is a NARROWING. Name it in +`NOT_SHIPPED_SELECTABLE` in +`test_agent_backend_editable.py::test_baseline_ships_every_known_backend`, with +the reason. An explicit allowlist rather than a relaxed assertion is the point: +a plain `baseline != known` still fails, so an id may sit outside the baseline +only by being named. + +Selectability has exactly one gate, `resolve_selected_backend`, and it logs +(H4). Do not add a static `enum` to `AgentConfig.acp_backend`: a literal frozen +at import cannot see a boot-time registration, and `validate_config_data` +*deletes* an out-of-enum value before the loader ever sees it — which strips a +registered harness from `config.json` with no degrade log at all. + +## Stage 7 — what a live harness additionally touches + +Stages 1–6 keep a harness inside `acp/`, `providers/`, and `acp_backends.py`. A +harness an operator can actually select spills further. Measured across the two +in-flight live-harness branches, roughly ten files outside those trees: + +`dashboard/handlers/agents.py` (the largest, +213 on one branch), +`mcp_gateway/session_servers.py` (+112), `dashboard/kiro_readiness.py`, +`dashboard/handlers/kiro_prerequisite.py`, `dashboard/handlers/sessions.py`, +`agent.py`, `config/loader.py`, `providers/base.py`, `session.py`, +`subagent.py`, `cli_doctor.py`. + +Two rules govern that spill. A capability the session layer reads off a provider +is declared on `LLMProvider` with a safe default, so an adapter never forces a +`hasattr` probe onto the Kiro path (H14) — the cost of obeying this is small, +around +11 lines in `providers/base.py` on the branch that needed it. And the +`ProviderRegistry` seam takes the addition without a `CONTRACT_VERSION` bump +(H13); if the Kiro construction path gains a conditional, a required argument, +or a new failure mode in service of your adapter, the design is wrong, not the +invariant. + +## Gates and tests + +Beyond the ordinary suite: + +- **`scripts/check_harness_parity.py`** enforces Group B on the lines your diff + *adds*, not the whole tree. Six rules, self-tested. +- **`scripts/check_agent_sdk_boundary.py`** is shrink-only. A new import of + `kiro_crew.acp` or `kiro_crew.providers` from a consumer fails even though the + baseline lists 106 existing ones. This is why Stage 1 puts the vocabulary in a + leaf: a consumer naming your constant must not have to cross the boundary to + do it. +- **`test_harness_parity.py`** pins the structural invariants (Groups A and C), + so they fail in the ordinary test job rather than a separate gate. +- **Group D is review-only.** `AUTOSDE.yaml`'s `harness-parity` rule carries + H13 and H14 to every AI review lane, because the absence of a mechanism is not + something a source scan can see. +- **`./scripts/docs-lint.sh`** requires every doc to be reachable from its + directory index, and checks that line citations still point at what they + claim. + +Never relax a check to make a red invariant green. If a harness genuinely cannot +be adapted within these invariants, the correct outcome is that it does not land +yet — say so in the PR instead of widening a seam. + +## Worked example: the Codex seam + +The Codex onboarding is a clean instance of stopping at Stage 6: + +| Stage | State | +|---|---| +| 1 vocabulary | Done — `ACP_BACKEND_CODEX`, in `ACP_BACKENDS_KNOWN`, `PROVIDER_LABEL_CODEX`, policy name mapped. | +| 2 capability sets | Decided for all eight: in the model and effort channels, out of the other six. All three channel sets were *created* by this work, which is why the count went from five to eight. | +| 3 spawn path | Done — adapter, npm package, dep marker, env override, project-local resolution. | +| 4 handshake | Done — `PROTOCOL_VERSION_CODEX`, its own literal at the same number as Claude's. | +| 5 install probe | **Absent.** No `_probe_codex` in `backend_install.py`. | +| 6 selectability | Dormant by consequence, named in `NOT_SHIPPED_SELECTABLE` with Stage 5 as the reason. | +| 7 live spill | Not reached. | + +The lesson worth carrying: the seam is dormant for exactly one reason, that +reason is written down where the narrowing check reads it, and closing it is a +single stage rather than a re-litigation. That is the shape to aim for — not +"complete or nothing", but "incomplete at a named stage". diff --git a/docs/system-specs/modules/harness-parity.md b/docs/system-specs/modules/harness-parity.md index dfdaf2ccf63..48676a95852 100644 --- a/docs/system-specs/modules/harness-parity.md +++ b/docs/system-specs/modules/harness-parity.md @@ -139,8 +139,12 @@ source of truth for what blocks. harness does not land yet — say so in the PR instead of widening a seam. 4. A new harness adds rows to `ACP_BACKENDS_KNOWN`, a `PROVIDER_LABEL_*`, and an explicit decision for every Group B membership set. "Inherited the - default" is not a decision. Note that `ACP_BACKENDS_KNOWN` and - `BASELINE_SELECTABLE_BACKENDS` are now equal, so adding an id to the first - without adding it to the second is the NARROWING that - `test_baseline_ships_every_known_backend` fails on — the id becomes spellable - but unreachable, and that state needs a stated reason rather than a default. + default" is not a decision. `BASELINE_SELECTABLE_BACKENDS` is otherwise + `ACP_BACKENDS_KNOWN`, so leaving a known id out of the baseline is a + NARROWING that `test_baseline_ships_every_known_backend` fails on **unless** + the id is named in that test's `NOT_SHIPPED_SELECTABLE` allowlist together + with the reason it cannot be offered yet: the id becomes spellable but + unreachable, and that state needs a stated reason rather than a default. + `ACP_BACKEND_CODEX` is the only member today. The full sequence a new + harness walks, and which stage decides whether it lands dormant or + selectable, is [harness-onboarding.md](harness-onboarding.md).