From bc3dcc0001e2779fccc3fb293e39213e21267006 Mon Sep 17 00:00:00 2001 From: zejiangg Date: Fri, 4 Sep 2026 04:53:19 +0000 Subject: [PATCH] feat(acp): wire Codex in behind an enforced tool-permission route Codex could be spelled by the core but not chosen. The vocabulary, the spawn path and the adapter resolution landed in #7813; what was missing was everything that decides whether offering the switch is honest. Nothing established how a harness is made to ask before it runs a tool, and backend_install.py had no probe that could explain a failed session. acp_tool_gate resolves one routing verdict per harness, so the refusal message, a doctor row and any later surface cannot disagree, and it REFUSES a session whose tool calls would bypass HookManager.on_tool_call. For Codex the client verifies that session/new advertised mode=read-only and applies it after session/new|load, before any prompt can run. Not advertised gives INDETERMINATE, an observed write rejection gives BYPASSED, and both refuse identically: a guarantee that lapses when evidence is missing is not a guarantee. There is no local opt-out, because a config bool could undercut a governance ceiling set above the operator. It also puts both ACP adapter OAuth token stores -- ~/.codex/auth.json and ~/.claude/.credentials.json -- on the sensitive-home read-gate floor. Neither was classified before, so the agent's own fs_read could lift either one. Kiro Crew never reads them and only ever checks that one exists so it can name the right sign-in command, so nothing legitimate loses access, and only the token leaf is classified -- the adapter's readable siblings stay readable. The credential mask handed to an enforced adapter's child is DERIVED from security.sensitive_home_dirs() minus that harness's own leaf, re-anchored under every override root and under both home spellings, so a leaf added to the floor is covered with no edit in the ACP layer. The macOS profile emits both a subpath and a literal deny for each entry, because most of that list is plain files and a subpath rule over a non-directory was asserted in three comments here and contradicted in a fourth, with no test exercising sandbox-exec either way. Selecting Codex with agent.sandbox="off" now refuses to start. wrap_argv returns from its off branch before it applies extra_hidden_dirs, so the mask -- Codex's only compensating control, since ACP v1 cannot prompt on a passive read -- silently evaporated. The refusal keys on the EFFECTIVE tier so a governed min_level floor does not false-refuse, and a host with no sandbox backend fails closed without consulting any mutable policy value, since both the opt-in and the floor can move between the preflight and the spawn. A gate refusal is non-retryable end to end. AcpToolGateUnroutable documents itself so but subclasses AcpError, so ensure_ready's transport ladder used to respawn the adapter once before reaching the identical refusal; it now has its own handler ahead of that ladder. And _sandbox_preflight translates the leaf module's plain ToolGateUnroutable into it at the boundary, the way the session-routing path already did -- raised raw it was neither an AcpError nor the type that handler names, so a sandbox-floor refusal escaped ensure_ready uncaught and skipped the cleanup every other refusal path runs. Known gap, documented rather than closed: ACP v1 offers no way to require a prompt for a passive read, so an adapter can still read its own token. That is inherent to running these harnesses at all -- kiro-cli is handed its own model credential in an environment nothing gates, and Claude Code already ships on this baseline with no credential mask at all -- and closing it needs upstream support rather than a change here. --- .github/black-baseline.txt | 1 - .../modules/harness-onboarding.md | 6 +- docs/system-specs/modules/harness-parity.md | 12 +- src/kiro_crew/acp/client.py | 155 ++++- src/kiro_crew/acp_backends.py | 111 +++- src/kiro_crew/acp_tool_gate.py | 383 +++++++++++ src/kiro_crew/agent_sdk/backend_install.py | 36 ++ src/kiro_crew/agent_sdk/drivers/acp.py | 47 ++ src/kiro_crew/sandbox.py | 87 ++- src/kiro_crew/security.py | 209 +++++- test/test_acp_backend_credentials.py | 170 +++++ test/test_acp_client_more_coverage.py | 65 ++ test/test_acp_tool_gate.py | 598 ++++++++++++++++++ test/test_agent_backend_editable.py | 6 +- test/test_agent_sdk_backend_install.py | 110 +++- test/test_harness_parity.py | 49 +- test/test_sandbox_cc_mode.py | 34 +- 17 files changed, 2011 insertions(+), 68 deletions(-) create mode 100644 src/kiro_crew/acp_tool_gate.py create mode 100644 test/test_acp_backend_credentials.py create mode 100644 test/test_acp_tool_gate.py diff --git a/.github/black-baseline.txt b/.github/black-baseline.txt index 3ae21ba6598..6740d36d0ca 100644 --- a/.github/black-baseline.txt +++ b/.github/black-baseline.txt @@ -990,7 +990,6 @@ test/test_safety_override.py test/test_sage_backend_routes_coverage.py test/test_sandbox_argv.py test/test_sandbox_backend_cache.py -test/test_sandbox_cc_mode.py test/test_sandbox_default_fix.py test/test_sandbox_first_party_exec.py test/test_sandbox_hardlink_scan.py diff --git a/docs/system-specs/modules/harness-onboarding.md b/docs/system-specs/modules/harness-onboarding.md index 62e6909123d..8bd9088e1b5 100644 --- a/docs/system-specs/modules/harness-onboarding.md +++ b/docs/system-specs/modules/harness-onboarding.md @@ -197,8 +197,10 @@ The Codex onboarding is a clean instance of stopping at Stage 6: | 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. | +| 5 install probe | Done — `_probe_codex` names `codex-acp` and the command that installs it. One component, not two: the adapter ships its own Codex binary. | +| 6 selectability | Selectable. `NOT_SHIPPED_SELECTABLE` is empty again, which is the healthy state. | +| routing | Done — `SESSION_CONFIG`, verified and applied as `mode=read-only` after session/new and before the first prompt, refusing otherwise. | +| residual | ACP v1 cannot require a prompt for a passive READ, so the sensitive-path block does not see this harness's reads. Mitigated at the OS boundary instead: its child cannot read the credential homes the standard tier leaves open. | | 7 live spill | Not reached. | The lesson worth carrying: the seam is dormant for exactly one reason, that diff --git a/docs/system-specs/modules/harness-parity.md b/docs/system-specs/modules/harness-parity.md index d485915c8cc..af6745fae5c 100644 --- a/docs/system-specs/modules/harness-parity.md +++ b/docs/system-specs/modules/harness-parity.md @@ -19,11 +19,11 @@ 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. +There is no exception today: `NOT_SHIPPED_SELECTABLE` is empty, which is the +healthy state. `ACP_BACKEND_CODEX` was the last member and left it once both +halves landed — `backend_install.py` gained its probe, so the install row names +the missing component and its command instead of reading `unknown`, and +`acp_tool_gate` established that its tool calls reach the PreToolUse gate. 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 @@ -90,7 +90,7 @@ already reads TRUE for KAS on a plain public build. | 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. 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. `ACP_BACKENDS_MCP_CONFIG_HOT_RELOAD` follows the same rule for a *skip*: membership is what lets the dashboard's MCP sync leave running sessions alone after a config write, because the harness reconciles the agent file itself (kiro-cli, from the release its reconcile was verified on; `mcp_hot_reload.py` holds every live process to that floor, read from its own `initialize` handshake). A harness that never demonstrated the reconcile would otherwise have its users' freshly installed servers stay unmounted with nothing red to say why. | `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`, `::test_mcp_config_hot_reload_is_opt_in` | `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`, `mcp_hot_reload.py` (`mcp_hot_reload_supported`) | | 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`, `::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`) | +| 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_selectable_and_answerable` | `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 diff --git a/src/kiro_crew/acp/client.py b/src/kiro_crew/acp/client.py index 506b321e42c..7069936f918 100644 --- a/src/kiro_crew/acp/client.py +++ b/src/kiro_crew/acp/client.py @@ -42,7 +42,7 @@ TypeVar, ) -from kiro_crew import agent_scratch, model_registry, platform_compat +from kiro_crew import acp_tool_gate, agent_scratch, model_registry, platform_compat from kiro_crew.acp._dispatch import ( _kiro_mcp_server_name, _kiro_tool_name, @@ -1345,6 +1345,20 @@ class AcpAuthRequired(AcpError): # noqa: N818 """ +class AcpToolGateUnroutable(AcpError): # noqa: N818 + """The harness's tool calls would not reach Kiro Crew's PreToolUse gate. + + Non-retryable, and a DISTINCT type from the transport errors around it: the + condition is a configuration fact, so a respawn re-reads the same answer and + refuses again while consuming a reconnect budget meant for transport faults. + + Wraps :class:`kiro_crew.acp_tool_gate.ToolGateUnroutable`, which cannot + subclass ``AcpError`` itself -- it lives in a LEAF module that must not import + this one (import cycle, and a forbidden-root edge for the SDK boundary gate). + Branchless callers keep degrading through their generic ``AcpError`` handling. + """ + + class AcpModelUnavailable(AcpError): # noqa: N818 """An explicitly requested model is not available to this account. @@ -2519,6 +2533,34 @@ def _select_tool_title( return None +def _sandbox_preflight(backend: str, mode: str) -> tuple[str, ...]: + """Refuse an unmasked enforced adapter, then resolve its credential mask. + + One function so the caller pays ONE ``asyncio.to_thread`` hop for both steps: + ``enforce_sandbox_floor`` probes for a sandbox backend and + ``adapter_hidden_credential_dirs`` resolves the home and every env-override root, + and both are blocking filesystem work that must not run on the event loop. + + Raises :class:`AcpToolGateUnroutable` when this session would spawn the adapter + with its mask dropped; returns the mask otherwise (empty for a harness this core + does not enforce, so their spawn arguments stay byte-identical). + """ + try: + acp_tool_gate.enforce_sandbox_floor(backend, mode) + return acp_tool_gate.adapter_hidden_credential_dirs(backend) + except acp_tool_gate.ToolGateUnroutable as exc: + # Translate at the boundary, exactly as the session-routing path does. + # ``acp_tool_gate`` is a LEAF that cannot import this module, so its + # ToolGateUnroutable is a plain ``Exception``: it is neither an + # ``AcpError`` (so the transport ladder in ``ensure_ready`` cannot see + # it) nor the ``AcpToolGateUnroutable`` the dedicated non-retrying + # handler names (an unrelated class). Raised raw, a sandbox-floor + # refusal therefore escaped ``ensure_ready`` uncaught and skipped the + # cleanup every other refusal path runs. ``from None`` because the + # wrapper carries the whole actionable message already. + raise AcpToolGateUnroutable(str(exc)) from None + + class AcpClient: """JSON-RPC 2.0 client over stdio with kiro-cli acp.""" @@ -3532,6 +3574,63 @@ async def set_config_option(self, config_id: str, value: str) -> None: # ── Dynamic Config from ACP ── + async def _apply_session_permission_routing(self) -> None: + """Make a SESSION_CONFIG harness actually ask, or refuse to run it. + + Called ONLY for a ``SESSION_CONFIG`` harness -- the caller tests that, so + the Kiro path never reaches this method (harness-parity H13). + + Two outcomes, and each is a different verdict on purpose: + + * the option was not advertised -> INDETERMINATE, because Kiro Crew cannot + tell what the adapter will do, and "cannot tell" must not read as armed; + * the write was rejected -> BYPASSED, an observed failure rather than an + unknown. + + Only the enforced mechanisms refuse; ``enforce_runtime_routing`` owns that + decision, so the scope lives in one place instead of being re-derived here. + """ + backend = self.backend + option_id, value = acp_tool_gate.permission_config_for(backend) + issue = acp_tool_gate.session_config_issue(backend, self._acp_config_options) + if issue: + # Not advertised: INDETERMINATE, never BYPASSED. The adapter may well + # ask anyway; Kiro Crew simply has no evidence, and the enforcement + # treats the two identically while the message stays honest. + try: + acp_tool_gate.enforce_runtime_routing( + backend, + issue, + verdict=acp_tool_gate.Verdict.INDETERMINATE, + remedy=acp_tool_gate.remediation_for(backend), + ) + except acp_tool_gate.ToolGateUnroutable as exc: + raise AcpToolGateUnroutable(str(exc)) from None + return + + try: + await self.set_config_option(option_id, value) + except AcpError as exc: + # The option was advertised and the write still failed, so this is an + # observed bypass rather than missing evidence. + try: + acp_tool_gate.enforce_runtime_routing( + backend, + "the adapter rejected its required session permission configuration", + verdict=acp_tool_gate.Verdict.BYPASSED, + remedy=acp_tool_gate.remediation_for(backend), + ) + except acp_tool_gate.ToolGateUnroutable as gate_exc: + raise AcpToolGateUnroutable(str(gate_exc)) from exc + return + + logger.info( + "ACP permission route armed: %s=%s (%s)", + option_id, + value, + acp_tool_gate.label_for(backend), + ) + def _store_session_config(self, resp: dict) -> None: """Extract effort configOptions from a session/new or session/load response. @@ -3721,6 +3820,13 @@ async def _spawn(self) -> None: if self.backend in ACP_BACKENDS_INTERNAL_SANDBOX: await asyncio.to_thread(assert_voice_runtime_outside_agent_workspace, self._work_dir) + # Credential mask for an enforced adapter, resolved inside that adapter's + # own branch below. Declared here only because wrap_argv_async takes it as + # one argument for every harness; the kiro branch never assigns it, so the + # kiro construction path gains no conditional, no awaited step and no new + # failure point in service of an adapter (harness-parity H13). + adapter_hidden_dirs: tuple[str, ...] = () + if self._is_claude: # Fold the requested model onto the exact spelling claude-agent-acp # advertised (from the persisted provider-model cache warmed by a @@ -3801,6 +3907,26 @@ async def _spawn(self) -> None: f"The 'codex' CLI alone does not serve ACP." ) argv = codex_argv + # Fail closed BEFORE the spawn when the mask below would be dropped: + # several wrap_argv paths return without applying extra_hidden_dirs, + # which would start an enforced adapter with no compensating control + # at all. Placed inside this pre-existing codex arm rather than in a + # gate of its own on the shared path: harness-parity H13 asks whether + # the kiro path CHANGED, and a conditional or an awaited step added + # there in service of an adapter is the change it names -- so the + # adapter's work lives entirely behind the adapter's own seam. + # Keyed on the ROUTING, not on codex's identity: _sandbox_preflight + # re-checks acp_tool_gate.is_enforced(self.backend) itself, so this + # site cannot mask a harness this core does not enforce. A future + # SESSION_CONFIG harness gets its own arm here and must make the same + # call; test_acp_tool_gate ratchets that so it cannot be forgotten. + # OFF-LOOP: both halves touch the filesystem -- the refusal probes for + # a sandbox backend (a cold probe shells out via subprocess.run) and + # the mask resolves the home plus every env-override root -- so they + # run in ONE worker thread rather than blocking the gateway loop. + adapter_hidden_dirs = await asyncio.to_thread( + _sandbox_preflight, self.backend, self._sandbox_mode + ) else: # Pin ONE reading of the environment for both the search and the # message that reports it. The previous code resolved against the live @@ -3851,6 +3977,10 @@ async def _spawn(self) -> None: argv, mode=self._sandbox_mode, strip_python_env=True, + # Credential homes the standard tier exposes for kiro-cli's sake and + # that an enforced adapter has no claim on. Empty for every harness + # this core does not enforce, so their spawn arguments are unchanged. + extra_hidden_dirs=adapter_hidden_dirs, is_kiro_cli=self.backend in ACP_BACKENDS_INTERNAL_SANDBOX, _prepare=wrap_argv, ) @@ -4697,6 +4827,17 @@ async def _initialize_session(self) -> None: # 5. Set model — override if KiroCrew config specifies non-default. await self._apply_startup_model() + # 6. Arm permission routing for harnesses whose asking is a session + # config option. AFTER the model apply (both write config options, and + # the permission one must land last) and before any prompt can run: + # _initialize_session is entirely pre-prompt, which is what makes this + # placement the guarantee rather than a best effort. + # Gated HERE rather than inside the method, so the first-class Kiro path + # gains no call, no await and no failure point in service of an adapter + # (harness-parity H13). A positive membership test, never "not claude". + if acp_tool_gate.routing_for(self.backend) is acp_tool_gate.Routing.SESSION_CONFIG: + await self._apply_session_permission_routing() + # Drain MCP server init notifications await self._drain_notifications() @@ -4748,6 +4889,18 @@ async def ensure_ready(self) -> None: _startup_outcome = "ready" return + except AcpToolGateUnroutable: + # Non-retryable BY CONSTRUCTION (see the class docstring): the + # refusal is a configuration fact, so a respawn re-reads the same + # answer and refuses again -- a wasted spawn plus teardown that + # also spends the reconnect budget this DISTINCT type exists to + # protect. Must sit BEFORE the generic transport handler, because + # it subclasses AcpError and would otherwise be retried by it, + # which is the shape that made the distinct type decorative. + _startup_outcome = "tool_gate_unroutable" + await self._cleanup_failed_live_spawn() + self._reset_state() + raise except (AcpTimeoutError, AcpError) as exc: if attempt == 0: logger.warning("ACP init failed (%s), retrying with fresh process...", exc) diff --git a/src/kiro_crew/acp_backends.py b/src/kiro_crew/acp_backends.py index 295efdff6d5..089f01e6449 100644 --- a/src/kiro_crew/acp_backends.py +++ b/src/kiro_crew/acp_backends.py @@ -29,6 +29,7 @@ from __future__ import annotations import logging +from enum import Enum from typing import FrozenSet, Set logger = logging.getLogger(__name__) @@ -94,14 +95,26 @@ #: :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. +#: ``ACP_BACKEND_CODEX`` is included, and the two things that were missing when it +#: was not are both worth naming, because each was a separate reason: +#: +#: * ``backend_install`` now has a probe, so the install row reads ``missing`` with +#: the component and the command rather than ``unknown``. A switch that cannot say +#: what is absent when a session fails is a switch offered ahead of the code that +#: answers for it. +#: * its tool calls are ROUTED. ``acp_tool_gate`` verifies ``session/new`` +#: advertised ``mode=read-only`` and applies it before the first prompt, refusing +#: the session otherwise, so the PreToolUse gate is armed for the calls it makes. +#: +#: One gap REMAINS and is survivable rather than closed: ACP v1 offers no way to +#: make an adapter ask for a passive READ, so the sensitive-path block cannot see +#: reads this harness performs. What made that dangerous was the credential homes +#: the standard sandbox tier leaves open, and those are denied to its child at the +#: OS boundary by ``acp_tool_gate.adapter_hidden_credential_dirs`` -- derived from +#: the read-gate floor itself, so the compensating control covers exactly what the +#: control it compensates for covers, minus the harness's own token store. BASELINE_SELECTABLE_BACKENDS: FrozenSet[str] = frozenset( - {ACP_BACKEND_KIRO, ACP_BACKEND_CLAUDE, ACP_BACKEND_KAS} + {ACP_BACKEND_KIRO, ACP_BACKEND_CLAUDE, ACP_BACKEND_KAS, ACP_BACKEND_CODEX} ) # ── Policy-facing spelling ── @@ -481,3 +494,87 @@ def model_registry_namespace(backend: str) -> str: # reads no agent file at all (``ACP_BACKENDS_SESSION_MCP_ARRAY``), and codex-acp # has not demonstrated the capability — neither inherits it. ACP_BACKENDS_MCP_CONFIG_HOT_RELOAD = frozenset({ACP_BACKEND_KIRO}) + + +# ── How a harness is made to ask ── +# Kiro Crew's PreToolUse gate -- the bundled denied-command rules, the +# sensitive-path block, the governance ceiling -- runs from exactly ONE place, +# ``HookManager.on_tool_call``, reached only from the permission-request branch of +# the dispatch parser. A harness that does not send ``session/request_permission`` +# per tool call is a harness where none of those controls execute. So "how is this +# one made to ask?" is a security property, not a compatibility note, and it is +# named here rather than assumed at each call site. + + +class Routing(str, Enum): + """The mechanism that makes a harness ask before it runs a tool. + + ``AGENT_SPEC`` -- the spawn names an agent, so the harness asks by + construction and there is nothing to probe or apply. + + ``SESSION_CONFIG`` -- the ACP v1 session advertises a config option whose + enforced value makes privileged tools ask. Kiro Crew verifies the option is + advertised and applies it before the first prompt. + + ``SEEDED_SETTINGS`` -- the harness is made to ask by a settings file Kiro + Crew writes, so the precondition would be confirmable by reading back what was + written. **Declared but not enforced by this core**, because that read-back + does not exist. ``AcpClient._write_claude_local_settings`` does seed + ``permissions.defaultMode``, but it is a CONDITIONAL write: it touches only the + file Crew owns (created this session, bytes still Crew's) and otherwise leaves + the path alone, and nothing confirms the adapter honoured the mode afterwards. + So a ``bypassPermissions`` already present in a user's own + ``settings.local.json`` or ``~/.claude`` is neither detected nor stripped, and + the guarantee cannot be asserted. Recorded as a known gap rather than papered + over with a ``ROUTED`` this core cannot earn -- see + ``docs/system-specs/modules/harness-onboarding.md``. + + ``UNVERIFIED`` -- Kiro Crew has NOT established how, or whether, this harness + can be made to ask. This member exists so "we do not know" is a state a + caller must handle rather than an absent case that falls through to a + permissive branch. It always resolves INDETERMINATE, which refuses. + """ + + AGENT_SPEC = "agent_spec" + SESSION_CONFIG = "session_config" + SEEDED_SETTINGS = "seeded_settings" + UNVERIFIED = "unverified" + + +#: Harness id -> its routing mechanism. +#: +#: A ``.get(backend, Routing.UNVERIFIED)`` read is deliberate: an id this table +#: does not name fails closed rather than inheriting a neighbour's mechanism. +ACP_BACKEND_ROUTING: dict = { + ACP_BACKEND_KIRO: Routing.AGENT_SPEC, + ACP_BACKEND_KAS: Routing.AGENT_SPEC, + ACP_BACKEND_CLAUDE: Routing.SEEDED_SETTINGS, + ACP_BACKEND_CODEX: Routing.SESSION_CONFIG, +} + + +#: Harness id -> the ``(option_id, required_value)`` its SESSION_CONFIG routing +#: needs, as advertised by ``session/new`` and applied through +#: ``session/set_config_option``. +#: +#: codex-acp's default ``agent`` mode permits writes inside the workspace without +#: asking. Its ACP v1 ``mode`` selector is the enforceable boundary: ``read-only`` +#: still permits passive READS -- ACP v1 has no way to require a prompt for those +#: -- but commands and changes request approval. That residual read gap does not +#: close and this option cannot close it; what makes it survivable is the +#: OS-boundary mask in ``acp_tool_gate.adapter_hidden_credential_dirs``, which +#: denies the child everything on the read-gate floor except the harness's own +#: token store. +ACP_BACKEND_PERMISSION_CONFIG: dict = { + ACP_BACKEND_CODEX: ("mode", "read-only"), +} + + +def routing_for(backend: str) -> "Routing": + """The routing mechanism for *backend*, failing closed on an unknown id.""" + return ACP_BACKEND_ROUTING.get(backend, Routing.UNVERIFIED) + + +def permission_config_for(backend: str) -> tuple: + """The ``(option_id, value)`` *backend* needs, or ``("", "")`` when it needs none.""" + return ACP_BACKEND_PERMISSION_CONFIG.get(backend, ("", "")) diff --git a/src/kiro_crew/acp_tool_gate.py b/src/kiro_crew/acp_tool_gate.py new file mode 100644 index 00000000000..1e18d7a0ca7 --- /dev/null +++ b/src/kiro_crew/acp_tool_gate.py @@ -0,0 +1,383 @@ +"""Whether a harness's tool decisions reach Kiro Crew's PreToolUse gate. + +One place resolves the verdict, so the refusal message, the doctor row and any +future dashboard surface cannot disagree about why a session was allowed. + +The gate itself -- the bundled denied-command rules, the sensitive-path block, +the governance ceiling -- runs only from ``HookManager.on_tool_call``, reached +only from the permission-request branch of the dispatch parser. A harness that +does not send ``session/request_permission`` per tool call is a harness where +none of those controls execute, so "does it ask?" is a security question rather +than a compatibility one. + +**A LEAF module, deliberately.** It imports the vocabulary from +:mod:`kiro_crew.acp_backends` and nothing from ``kiro_crew.acp``, which the SDK +boundary gate treats as a forbidden root. The callers that need a verdict -- +``acp/client.py`` on the session path, and later ``kirocrew doctor`` and the +dashboard -- can therefore all reach it, and a consumer naming a verdict does not +buy a forbidden edge to do it. + +**Enforcement scope.** Only :data:`~kiro_crew.acp_backends.Routing.SESSION_CONFIG` +is ENFORCED here today, because it is the only mechanism this core implements end +to end. ``AGENT_SPEC`` needs no enforcement (it holds by construction), and +``SEEDED_SETTINGS`` is declared-but-unenforced, and the reason is a read-back +gap rather than a missing writer. ``AcpClient._write_claude_local_settings`` does +seed ``permissions.defaultMode`` into ``/.claude/settings.local.json``, +but it writes only the file it OWNS -- created this session and still carrying the +bytes Crew wrote -- and declines otherwise, and nothing reads back whether the +adapter honoured the mode. A ``bypassPermissions`` already sitting in a user's own +``settings.local.json`` or ``~/.claude`` is therefore neither detected nor +stripped, so the precondition this mechanism would need is not established. +``routing_verdict`` reports that honestly as INDETERMINATE -- what is scoped is +whether a non-ROUTED verdict REFUSES, not whether it is told truthfully. Widening the scope +means implementing a mechanism, not editing an allowlist. +""" + +from __future__ import annotations + +import logging +from enum import Enum + +from kiro_crew.acp_backends import ( + ACP_BACKEND_CODEX, + Routing, + permission_config_for, + routing_for, +) + +logger = logging.getLogger(__name__) + +#: Routing mechanisms whose non-ROUTED verdict actually refuses a session. +#: +#: Scoped to what this core implements rather than to a list of harness ids: a +#: harness declaring an implemented mechanism is enforced automatically, and +#: adding a mechanism here without implementing it would assert a guarantee +#: nothing performs. +ENFORCED_ROUTINGS: frozenset = frozenset({Routing.SESSION_CONFIG}) + +#: What is NOT consulted when a harness's tool calls bypass the gate. Named in +#: full in the refusal, because "bypasses the security gate" does not tell an +#: operator what they are giving up. +UNENFORCED_CONTROLS = ( + "the bundled denied-command rules, the sensitive-path block and the governance ceiling" +) + +#: Operator-facing harness labels. Local rather than imported: the refusal text is +#: the only consumer, and a Codex host must never be told to run ``kiro-cli +#: login``-style advice aimed at a different harness. +_LABELS: dict = { + ACP_BACKEND_CODEX: "OpenAI Codex", +} + +#: The credential store each enforced harness must still be able to read. +#: +#: An adapter authenticates itself, so its OWN token is the one thing the mask +#: below must not take away. Everything else on the read-gate floor is denied. +#: +#: Home-relative, matching the floor's own spelling. An operator override +#: (``CODEX_HOME``) moves the real file outside the home anyway, so it is not on +#: the floor and the mask never had it to exclude. +ADAPTER_OWN_CREDENTIAL_LEAVES: dict = { + ACP_BACKEND_CODEX: (".codex/auth.json",), +} + + +class Verdict(str, Enum): + """Whether a harness's tool decisions reach Kiro Crew's gate. + + ``INDETERMINATE`` is deliberately NOT a synonym for "probably fine". A + guarantee that lapses whenever a file is unreadable is not a guarantee, so + :func:`enforce_runtime_routing` treats it exactly like ``BYPASSED``. It is a + distinct value only so the operator-facing message can say "could not + determine" instead of asserting a policy nothing established. + """ + + ROUTED = "routed" + BYPASSED = "bypassed" + INDETERMINATE = "indeterminate" + + +class ToolGateUnroutable(Exception): + """Raised when a harness's tool calls would not reach the PreToolUse gate. + + Deliberately NOT retryable: the condition is a configuration fact, so a retry + re-reads the same answer and refuses again while consuming a reconnect budget + that exists for transport faults. + + A plain ``Exception`` rather than an ``AcpError`` subclass because ``AcpError`` + lives in ``acp.client``, which imports THIS module -- subclassing would be an + import cycle, and it would also drag a forbidden-root import into a leaf. The + session path translates it into its own ACP-error type so branchless callers + keep degrading through their generic handling. + """ + + +def adapter_hidden_credential_dirs(backend: str) -> tuple: + """Absolute paths on the read-gate floor to hide from *backend*'s child. + + DERIVED from ``security.sensitive_home_dirs()`` rather than enumerated, and + that is the whole point. This mask is the compensating control for a harness + whose passive reads never reach ``HookManager.on_tool_call``: the floor states + what an agent may never read, so anything on it the child can still open is a + hole in the compensation. An enumerated short list left exactly that hole -- + ``.claude/.credentials.json``, ``.netrc``, ``.git-credentials``, ``.pypirc`` + and ``.npmrc`` were all on the floor and still readable by the child, the first + of them a leaf this very change had just classified. Deriving keeps the two in + step, and a floor entry added later is covered with no edit here. + + The harness's own credential store is excluded, because the adapter must read + it to authenticate. That asymmetry is intentional and safe: the floor still + blocks the AGENT's file tools from that leaf, so the two controls cover + different readers rather than cancelling each other. + + ``.ssh`` arrives through the floor and it has a cost: git-over-SSH inside such + a session stops working, because the private key is no longer readable. That + is accepted rather than worked around -- leaving private keys readable would + not close what this exists to close, and a harness landing new has no + established workflow to break. + + Empty for a harness this core does not enforce, so the first-class path and + every unenforced harness keep byte-identical sandbox arguments. Returns + ABSOLUTE paths: ``sandbox.wrap_argv`` runs ``os.path.abspath`` over what it is + handed, which would resolve a bare ``.aws`` against the CWD and silently deny + nothing. File leaves are fine to pass -- the Linux launcher classifies each + entry with its own ``isfile``/``isdir``, and the macOS profile emits both a + ``subpath`` and a ``literal`` deny for each one, so a plain file is covered + without depending on how Seatbelt treats a subpath over a non-directory. + """ + if not is_enforced(backend): + return () + # Imported here rather than at module scope: this is a LEAF that + # ``acp/client.py`` imports at import time, and security.py is a large module + # whose cost belongs on the one call that needs it. + from kiro_crew.security import sandbox_credential_targets + + # Delegated rather than projected under ``Path.home()`` here: a credential the + # operator relocated with ``KIROCREW_HOME`` / ``CLAUDE_CONFIG_DIR`` / + # ``CLAUDE_HOME`` does NOT live under the real home, so a home-only projection + # would hand the sandbox a path that denies nothing while the live secret stayed + # readable. ``sandbox_credential_targets`` owns the same anchor rules as the read + # gate, so this mask cannot drift from the floor it compensates for. + return sandbox_credential_targets(tuple(ADAPTER_OWN_CREDENTIAL_LEAVES.get(backend, ()))) + + +def enforce_sandbox_floor(backend: str, mode: str) -> None: + """Refuse an enforced adapter whose credential mask would never be applied. + + :func:`adapter_hidden_credential_dirs` is the COMPENSATING control for a + harness that self-approves its own tool calls: ACP v1 cannot force a prompt for + a passive read, so that mask is the only thing between the child and the + credential homes the standard tier deliberately leaves open for kiro-cli's + sake. ``wrap_argv`` returns from its ``mode == "off"`` branch BEFORE it applies + ``extra_hidden_dirs``, so in that configuration the mask silently evaporates and + this adapter becomes strictly WEAKER than the gate-routed harnesses beside it -- + whose reads still reach the PreToolUse gate under the very same setting. The + route's security argument assumes an OS boundary; this makes the assumption + explicit instead of letting it fail open. + + Keyed on the EFFECTIVE tier, not the configured one: a governed host whose + ``sandbox.min_level`` floor raises ``"off"`` still gets the mask, and must not + be refused over a config value the ceiling already overrode. + + Returns for a harness this core does not enforce, so the first-class path and + every unenforced harness reach the spawn unchanged. + """ + if not is_enforced(backend): + return + # Local import for the same reason as the mask builder: this is a leaf that + # ``acp/client.py`` imports at import time. + from kiro_crew.sandbox import credential_mask_applies + + # Ask whether the mask WILL BE APPLIED, never whether one particular tier is + # selected. An earlier revision tested ``effective_sandbox_mode(mode) != "off"`` + # and so covered only one of the two paths that hand back an unwrapped child: a + # host with no sandbox backend and ``sandbox_allow_unsandboxed_exec`` opted in + # resolves to a non-``off`` tier, passed the guard, and still spawned the adapter + # with its credential mask dropped. + if credential_mask_applies(mode): + return + raise ToolGateUnroutable( + "{} routes tool calls through an enforced permission route whose " + "compensating control is an OS-level credential mask, but this session would " + "spawn it unsandboxed -- either agent.sandbox is 'off', or no sandbox backend " + "is available and agent.sandbox_allow_unsandboxed_exec is set -- so the mask " + "is never applied and the adapter's credential reads are unfenced. Set " + "agent.sandbox to 'standard' or 'strict' ON A HOST WITH A WORKING BACKEND to " + "select this harness, or select a harness whose tool calls reach the gate " + "directly.".format(label_for(backend)) + ) + + +def label_for(backend: str) -> str: + """An operator-facing name for *backend*, falling back to the raw id.""" + return _LABELS.get(backend) or (backend or "Kiro CLI") + + +def routing_verdict(backend: str) -> tuple: + """Report how *backend* routes tool calls, and why. + + Dispatches on the routing MECHANISM rather than the harness id, so a harness + declaring an already-implemented mechanism needs no change here. + + Read-only and side-effect free: this is what a doctor row or a dashboard GET + calls, and a probe that wrote a settings file would create one on every + Settings page load. + """ + routing = routing_for(backend) + + if routing is Routing.AGENT_SPEC: + # kiro-cli and KAS are made to ask because the spawn names an agent, so + # the precondition holds by construction and there is nothing to probe. + return (Verdict.ROUTED, "the spawn names an agent") + + if routing is Routing.SESSION_CONFIG: + option_id, value = permission_config_for(backend) + if not option_id or not value: + # A harness declaring the mechanism without naming its option is a + # registration bug, and it must not read as routed. + return ( + Verdict.INDETERMINATE, + "the harness declares session-config routing but names no config option", + ) + # ROUTED on a PROMISE, not a probe: the option lives on a session that + # does not exist yet, so there is nothing on disk to read. The other half + # of the guarantee is ``session_config_issue`` + the apply, which MUST run + # after session/new and before the first prompt. Port this verdict without + # that caller and the harness reports routed while running its own default + # mode -- the one silent-bypass hole in this design. + return ( + Verdict.ROUTED, + f"the client enforces {option_id}={value} before the first prompt", + ) + + if routing is Routing.SEEDED_SETTINGS: + # Declared, not enforced here -- and the gap is the READ-BACK, not a missing + # writer. ``_write_claude_local_settings`` does seed the mode, but only into + # the file Crew owns (created this session, bytes still Crew's) and declines + # otherwise, and nothing confirms the adapter honoured it, so a + # ``bypassPermissions`` already in the user's own settings is neither + # detected nor stripped. Told truthfully rather than upgraded to ROUTED; + # whether it REFUSES is a separate decision, and SEEDED_SETTINGS is outside + # ENFORCED_ROUTINGS. + return ( + Verdict.INDETERMINATE, + "this core seeds the harness's permission settings only into a file it " + "owns, and nothing reads back whether they took effect", + ) + + return ( + Verdict.INDETERMINATE, + "Kiro Crew has not established how this harness routes tool calls", + ) + + +def is_enforced(backend: str) -> bool: + """Whether a non-ROUTED verdict for *backend* refuses the session.""" + return routing_for(backend) in ENFORCED_ROUTINGS + + +def remediation_for(backend: str) -> str: + """The concrete change an operator can make, or ``""`` when there is none.""" + routing = routing_for(backend) + if routing is Routing.SESSION_CONFIG: + option_id, value = permission_config_for(backend) + if option_id and value: + return ( + f"Install a {label_for(backend)} adapter that advertises ACP session " + f"config option {option_id!r} with value {value!r}." + ) + return "" + + +def session_config_issue(backend: str, config_options: object) -> str: + """Why *backend*'s required permission config cannot be applied. + + ``""`` means the exact option AND value were advertised by ``session/new``. + + Permission routing is stricter than optional model or effort configuration: a + missing option cannot be shrugged off as lazy advertising, because the first + prompt would then run ungated. + """ + if routing_for(backend) is not Routing.SESSION_CONFIG: + return "" + option_id, required = permission_config_for(backend) + if not option_id or not required: + return "the harness declares session-config routing but names no config option" + if not isinstance(config_options, list): + return "session/new did not advertise configOptions" + for option in config_options: + if not isinstance(option, dict) or option.get("id") != option_id: + continue + raw_values = option.get("options") + if not isinstance(raw_values, list): + return f"config option {option_id!r} has no values" + values = { + entry.get("value") + for entry in raw_values + if isinstance(entry, dict) and isinstance(entry.get("value"), str) + } + if required in values: + return "" + return f"config option {option_id!r} does not advertise required value {required!r}" + return f"session/new did not advertise config option {option_id!r}" + + +def enforce_runtime_routing( + backend: str, + reason: str, + *, + verdict: Verdict = Verdict.BYPASSED, + remedy: str = "", +) -> None: + """Act on a routing fact learned after the harness process started. + + Raises :class:`ToolGateUnroutable` before the first prompt can run, or returns + for a harness this core does not enforce. + + **There is deliberately no opt-out.** An earlier revision carried + ``agent.acp_backend_allow_ungated_tools``, a LOCAL config bool that started the + session anyway with a warning and an audit event. That is the precise shape the + central governance ceiling exists to forbid: a managed fleet could allow this + harness while a standard user's own config switched the compensating control + off, so POLICY-intersect-PROFILE would no longer hold for the calls the harness + self-approves. A security control with a local off-switch is not a control, and + the escape hatch it offered was never needed -- the refusal names the concrete + remedy (:func:`remediation_for`), and lowering the sandbox tier remains an + operator decision that IS clamped by the ceiling. + + A harness outside :data:`ENFORCED_ROUTINGS` returns unchanged: the verdict is + still reported truthfully by :func:`routing_verdict`, but this core does not + implement its mechanism and must not refuse a session over a guarantee it + never attempted. + """ + if not is_enforced(backend): + logger.debug( + "tool-gate routing not enforced for %s (%s): %s", + label_for(backend), + routing_for(backend).value, + reason, + ) + return + + suffix = f" {remedy}" if remedy else "" + raise ToolGateUnroutable( + f"{label_for(backend)} tool calls would not reach Kiro Crew's security gate " + f"({reason}), so {UNENFORCED_CONTROLS} would not be consulted for them.{suffix}" + ) + + +__all__ = [ + "ADAPTER_OWN_CREDENTIAL_LEAVES", + "ENFORCED_ROUTINGS", + "UNENFORCED_CONTROLS", + "ToolGateUnroutable", + "Verdict", + "adapter_hidden_credential_dirs", + "enforce_runtime_routing", + "enforce_sandbox_floor", + "is_enforced", + "label_for", + "remediation_for", + "routing_verdict", + "session_config_issue", +] diff --git a/src/kiro_crew/agent_sdk/backend_install.py b/src/kiro_crew/agent_sdk/backend_install.py index aa5bed90b4a..bd4ad20a7a6 100644 --- a/src/kiro_crew/agent_sdk/backend_install.py +++ b/src/kiro_crew/agent_sdk/backend_install.py @@ -33,6 +33,7 @@ from kiro_crew.acp_backends import ( ACP_BACKEND_CLAUDE, + ACP_BACKEND_CODEX, ACP_BACKEND_KAS, ACP_BACKEND_KIRO, ACP_BACKENDS_KNOWN, @@ -63,6 +64,10 @@ #: having one without the other is a real, distinguishable half-install. COMPONENT_CLAUDE_CODE_CLI = "claude" +#: The codex-acp adapter. ONE component, not two: the adapter ships its own +#: compatible Codex binary, so there is no second executable Crew resolves. +COMPONENT_CODEX_ACP_ADAPTER = "codex-acp" + #: How long a verdict is reused. The Claude driver shells out to mise and globs #: the filesystem, and the dashboard polls this endpoint, so an uncached probe #: would spawn a subprocess per poll. Module-level and read at call time (not @@ -193,10 +198,41 @@ def _probe_claude() -> BackendInstallState: #: Backend id → its probe. A registry rather than an ``if`` chain so an id with #: no probe is a lookup miss that degrades to ``UNKNOWN``, instead of falling #: through to whichever branch happened to be last. +def _probe_codex() -> BackendInstallState: + """The Codex backend needs one component, and names it when it is absent. + + Without this probe the switch could render with nothing to say about a session + that failed to start -- which was the stated reason the backend stayed out of + ``BASELINE_SELECTABLE_BACKENDS``. The install command comes from the same + constant the resolution ladder searches for, so the advice cannot drift from + what would actually satisfy it. + + ``restart_required`` mirrors the claude probe: when the adapter resolves now + but the running gateway cached a negative, the honest answer is "installed, + restart to use it" rather than a promise the next spawn breaks. + """ + policy_id = _policy_id(ACP_BACKEND_CODEX) + if acp_driver.codex_adapter_resolves(): + return BackendInstallState( + ACP_BACKEND_CODEX, + policy_id, + INSTALLED, + restart_required=acp_driver.codex_adapter_cached_negative(), + ) + return BackendInstallState( + ACP_BACKEND_CODEX, + policy_id, + MISSING, + (COMPONENT_CODEX_ACP_ADAPTER,), + acp_driver.codex_adapter_install_command(), + ) + + _PROBES: Dict[str, Callable[[], BackendInstallState]] = { ACP_BACKEND_KIRO: _probe_kiro, ACP_BACKEND_KAS: _probe_kas, ACP_BACKEND_CLAUDE: _probe_claude, + ACP_BACKEND_CODEX: _probe_codex, } diff --git a/src/kiro_crew/agent_sdk/drivers/acp.py b/src/kiro_crew/agent_sdk/drivers/acp.py index dd0ebd0ad28..46d4ef1ddd8 100644 --- a/src/kiro_crew/agent_sdk/drivers/acp.py +++ b/src/kiro_crew/agent_sdk/drivers/acp.py @@ -138,6 +138,53 @@ def claude_adapter_cached_negative() -> bool: return not argv +def codex_adapter_resolves() -> bool: + """Whether the codex-acp adapter resolves to a runnable argv. + + ONE component, unlike claude's two: codex-acp ships a compatible Codex binary + as an npm dependency and reads ``CODEX_PATH`` itself only to run a DIFFERENT + one, so there is no second executable Crew hands it and no half-install to + distinguish. + """ + from kiro_crew.acp.client import _resolve_codex_acp_bin + + adapter_argv, _searched_path = _resolve_codex_acp_bin() + return bool(adapter_argv) + + +def codex_adapter_cached_negative() -> bool: + """Has the RUNNING gateway already resolved the codex adapter as absent? + + Same hazard and same resolution as :func:`claude_adapter_cached_negative`: the + argv is resolved once per process behind an ``_UNRESOLVED`` sentinel and never + invalidated, so a fresh probe reporting "installed" after an install would + disagree with every spawn until a restart. Consulted, never invalidated -- a + dashboard GET must not mutate a global on the spawn path. + """ + from kiro_crew.acp import client as _client + + cached = getattr(_client, "_codex_acp_argv_cache", None) + if cached is None or cached is getattr(_client, "_UNRESOLVED", object()): + return False + try: + argv, _searched = cached # type: ignore[misc] + except Exception: + return False + return not argv + + +def codex_adapter_install_command() -> str: + """``npm i -g ``, with the package name read from the repo. + + A global install of the SCOPED package puts the UNSCOPED ``codex-acp`` binary + on PATH, which is what the resolution ladder looks for -- so this command and + that ladder agree by construction rather than by coincidence. + """ + from kiro_crew.acp.client import CODEX_ACP_NPM_PKG + + return f"npm i -g {CODEX_ACP_NPM_PKG}" + + def claude_adapter_install_command() -> str: """``npm i -g `` -- the adapter's remedy, from the repo. diff --git a/src/kiro_crew/sandbox.py b/src/kiro_crew/sandbox.py index 5754e1b93be..df6093dddf4 100644 --- a/src/kiro_crew/sandbox.py +++ b/src/kiro_crew/sandbox.py @@ -2805,8 +2805,9 @@ def _build_launcher_script( # the child decide keeps the syscalls in the child, where they are already happening # and where blocking costs nothing but that one spawn. # - # macOS is unaffected either way: its rule is `(deny file-read* (subpath …))`, and a - # subpath rule covers a plain file. + # macOS is unaffected either way: for these entries the profile emits BOTH a + # `(subpath …)` and a `(literal …)` deny, so a plain-file leaf is covered + # without relying on how subpath treats a non-directory. dirs_json = json.dumps(list(dict.fromkeys(hidden_dirs))) readonly_json = json.dumps(list(dict.fromkeys(readonly_dirs))) files_json = json.dumps( @@ -3726,6 +3727,22 @@ def _build_seatbelt_profile( rules.append(f'(deny file-read* (subpath "{escaped}"))') rules.append(f'(deny file-write* (subpath "{escaped}"))') rules.append(f'(deny file-link (subpath "{escaped}"))') + # BOTH shapes, because most of this list is plain FILES, not directories: + # sandbox_credential_targets() yields .codex/auth.json, + # .claude/.credentials.json, .netrc, .git-credentials, .npmrc, .pypirc, + # .docker/config.json, .kube/config, sel_hmac.key, token_signing.key. + # Whether a subpath rule alone covers a plain file is asserted in three + # comments in this tree and CONTRADICTED by the crew_hidden branch above + # ("A leaf may be a plain file, which no subpath rule addresses"), and + # nothing tests it -- no test in this repo executes sandbox-exec, so the + # claim has never been checked against the kernel. This mask is the ONLY + # compensating control for a harness whose passive reads never reach the + # gate, so it must not rest on an unverified reading of Seatbelt: the + # literal is redundant if subpath does cover files, and load-bearing if it + # does not. + rules.append(f'(deny file-read* (literal "{escaped}"))') + rules.append(f'(deny file-write* (literal "{escaped}"))') + rules.append(f'(deny file-link (literal "{escaped}"))') # .ssh: deny all access except reading known_hosts (strict only) if sandbox_level == "strict": @@ -5229,6 +5246,72 @@ def reset_backend() -> None: _SANDBOX_MODE_ALIASES = {"auto": "standard"} +def credential_mask_applies(mode: str) -> bool: + """Whether :func:`wrap_argv` would actually APPLY ``extra_hidden_dirs`` for *mode*. + + Exactly two outcomes hand back an UNWRAPPED child, dropping the mask: the ``off`` + tier, and a host with no backend where unsandboxed exec is opted in and no + governance floor mandates a sandbox (the ``return argv, None`` after + ``_warn_no_isolation``). Every other path either wraps the argv -- ``namespace`` + and ``sandbox-exec`` both thread ``extra_hidden_dirs`` through -- or REFUSES the + spawn outright with :class:`SandboxUnavailableError`, and a refusal needs no guard + because nothing starts. + + Note this predicate is STRICTER than that inventory on one path: with no + backend it never consults the unsandboxed-exec opt-in, so the opted-in host + answers False rather than tracking that mutable value. See the branch below. + + This lives here, beside those branches, so a caller whose security argument + depends on the mask cannot drift from them: a future branch that skips the mask + is a change to this function, not a silent hole in some other module's copy of + the reasoning. + """ + floor = _governance_sandbox_floor() + effective = _clamp_sandbox_mode_to_floor(mode, floor) + if effective == "off": + return False + # Already inside a Kiro Crew sandbox: a nested re-wrap is impossible by design, + # wrap_argv passes the argv through (at most an env scrub) and never reaches a + # backend that could apply the mask. The OUTER sandbox confines the child, but it + # was built for the tier's own hidden dirs -- which deliberately leave ~/.aws, + # ~/.ssh and ~/.kube readable for kiro-cli's sake -- so it is NOT a substitute for + # an adapter-specific credential mask. + if _inside_kirocrew_sandbox() and _macos_sandbox_state() is not False: + return False + if detect_backend(config_mode=effective) != "none": + return True + # backend == "none": FAIL CLOSED, reading NO policy value to decide it. + # + # Nothing on a host without a backend can carry ``extra_hidden_dirs``, so the + # only question was whether the spawn would be refused instead -- and every + # answer to THAT is mutable config read here at preflight and acted on at the + # spawn. The opt-in was the first such value (an operator opting in between the + # two let a session that had been told "the mask applies" hand back an unwrapped + # child); the governance floor is the second, because a ceiling LOOSENED in the + # same window drops the very refusal that made True safe to report. Both windows + # close only by refusing to derive this from policy at all: no backend, no mask, + # so no enforced adapter starts here regardless of how policy moves. + # + # The cost is that a no-backend host cannot run an enforced adapter even under a + # governance floor that forbids unsandboxed execution. That host could not run + # one anyway -- ``wrap_argv`` cannot satisfy the floor without a backend and + # raises -- so this changes which layer reports it, not whether it works. Only an + # ENFORCED adapter reaches here at all: ``enforce_sandbox_floor`` returns early + # for every harness this core does not enforce, so no first-class path changes. + return False + + +def effective_sandbox_mode(mode: str) -> str: + """The tier :func:`wrap_argv` would ACTUALLY apply for *mode* on this host. + + Applies the governed ``sandbox.min_level`` clamp, so a caller whose security + argument depends on the sandbox being on can ask whether it will be instead of + trusting the raw config value -- which a governance floor may silently raise. + Read-only: same clamp, no spawn, no side effects. + """ + return _clamp_sandbox_mode_to_floor(mode, _governance_sandbox_floor()) + + def _governance_sandbox_floor() -> str | None: """Read the governed ``sandbox.min_level`` floor, or ``None`` when ungoverned. diff --git a/src/kiro_crew/security.py b/src/kiro_crew/security.py index a94ad34631d..b9d2a4e3c64 100644 --- a/src/kiro_crew/security.py +++ b/src/kiro_crew/security.py @@ -6228,6 +6228,22 @@ def _emit_push_allow_event(command: str) -> None: ".pypirc", ".netrc", ".git-credentials", + # ACP adapter OAuth token stores. Each adapter owns its own sign-in flow and + # persists its own tokens; Kiro Crew never reads them, and only ever checks + # that the file EXISTS so it can name the right sign-in command. An agent + # that could ``fs_read`` one could impersonate the operator against that + # vendor, so both are on the floor "precisely so nothing else does". + # + # Only the token leaf is classified. The sibling config files — codex's + # ``config.toml``, claude's ``settings*.json`` — deliberately stay readable: + # routing diagnosis needs them and they carry no credential. + # + # These are ``$HOME``-rooted defaults. Both adapters honour a home override + # (``CODEX_HOME``; ``CLAUDE_CONFIG_DIR`` / ``CLAUDE_HOME``), re-anchored in + # ``_home_dir_targets_uncached`` so an override cannot move the token out + # from under the gate. + ".codex/auth.json", + ".claude/.credentials.json", # (The Notes builtin's GitHub PAT lives under the crew data-home at # ``/workspace/md-notebook/pat``; it is added below via # ``_CREW_SECRET_LEAVES`` so BOTH ``.kiro/crew`` and the legacy ``.kirocrew`` @@ -7922,12 +7938,12 @@ def _candidate_forms(path_str: str, base_dir: str | None = None) -> set[str]: def _home_dir_targets_uncached( home_dirs: list[str], - roots: tuple[str, str | None, str | None, str] | None = None, + roots: _ResolvedRoots | None = None, ) -> set[str]: """Anchor the ``$HOME``-relative *home_dirs* entries into absolute, casefolded on-disk targets. - *roots* optionally supplies the ``(home, crew_home, kiro_home)`` anchors + *roots* optionally supplies the already-resolved :class:`_ResolvedRoots` already resolved by the caller. The TTL cache in :func:`_home_dir_targets` MUST pass it: resolving the roots here as well would read the filesystem a second time, and a root symlink repointed between the two reads would file this @@ -7949,10 +7965,11 @@ def _home_dir_targets_uncached( secrets. On POSIX a single-segment entry splits to a 1-element list, so this is a no-op there. """ - if roots is not None: - home, crew_home, kiro_home_override, logical_home = roots - else: - home, crew_home, kiro_home_override, logical_home = _resolved_root_key() + resolved = roots if roots is not None else _resolved_root_key() + home = resolved.home + crew_home = resolved.crew_home + kiro_home_override = resolved.kiro_home + logical_home = resolved.logical_home def _anchor(root: str, d: str) -> str: return os.path.join(root, *d.split("/")).casefold() @@ -8017,6 +8034,26 @@ def _anchor(root: str, d: str) -> str: sensitive_targets.add(os.path.realpath(agents_full).casefold()) except (OSError, ValueError): pass + # An ACP adapter's OAuth token follows that adapter's own home override, so + # the ``$HOME``-rooted entry anchored above covers only the documented + # default. Re-anchor the token leaf under each override the adapter honours + # (the default form stays, so every location is always covered). Guarded on + # membership in *home_dirs* for the same reason as the agents dir above: a + # write-tier build must not gain a read-tier target. + for _leaf, _root_fields in _OVERRIDE_ANCHORED_LEAVES: + if _leaf not in home_dirs: + continue + _basename = _leaf.split("/")[-1] + for _field in _root_fields: + _root = getattr(resolved, _field, None) + if not _root: + continue + _full = os.path.join(_root, _basename) + sensitive_targets.add(_full.casefold()) + try: + sensitive_targets.add(os.path.realpath(_full).casefold()) + except (OSError, ValueError): + pass return sensitive_targets @@ -8065,8 +8102,70 @@ def _anchor(root: str, d: str) -> str: _home_targets_cache: dict[tuple[object, ...], tuple[float, set[str]]] = {} -def _resolved_root_key() -> tuple[str, str | None, str | None, str]: - """Return the (home, crew_home, kiro_home, logical_home) roots the target set is anchored on. +class _ResolvedRoots(NamedTuple): + """The roots the sensitive-target set is anchored on, AND its cache key. + + Those two jobs are the same object on purpose: every field is part of the + key, so an override that would move a target invalidates the cached set + instead of serving targets anchored on the previous value. Keying on fewer + fields than the builder anchors on is the fail-OPEN shape the resolved-home + key already exists to prevent. + + A new adapter with its own credential home adds a field here and an entry in + ``_OVERRIDE_ANCHORED_LEAVES``; nothing else changes, because the tuple is + unpacked by FIELD rather than by position. + """ + + home: str + crew_home: str | None + kiro_home: str | None + codex_home: str | None + claude_config_dir: str | None + claude_home: str | None + logical_home: str + + +#: Sensitive leaf → the override roots its parent directory can be moved to. +#: +#: The leaf's own ``$HOME``-rooted form is anchored by the ordinary path in +#: ``_home_dir_targets_uncached``; this table only covers the overrides. A leaf +#: absent from the *home_dirs* list being built is skipped, so a write-tier +#: build never leaks a read-tier target. +_OVERRIDE_ANCHORED_LEAVES: tuple[tuple[str, tuple[str, ...]], ...] = ( + (".codex/auth.json", ("codex_home",)), + (".claude/.credentials.json", ("claude_config_dir", "claude_home")), +) + + +def _resolved_env_root(name: str) -> str | None: + """Resolve an environment home override, or ``None`` when it is unset. + + Falls back to the unresolved absolute form on OSError/ValueError the same way + the builder does. No validity check: an unsafe override falls back to its + default inside the owning helper, and that default is already covered by the + ``$HOME``-rooted entry, so an extra target under a bogus value is harmless + and fail-safe. + + The value is read VERBATIM -- deliberately not stripped. Whitespace is a legal + POSIX path character, and the owning resolvers take the variable raw + (``_valid_override_home`` does ``Path(os.environ.get("KIROCREW_HOME"))``, and + ``config_dir`` then ``mkdir``s whatever that names). Stripping here would + anchor the target set on ```` while the process actually runs out of + ``" "``, leaving the real ``.env``, signing keys and governance files + outside the floor this set defines. Emptiness is the only test, so an unset + or empty override still resolves to ``None``. + """ + raw = os.environ.get(name, "") + if not raw: + return None + try: + return str(Path(raw).expanduser().resolve()) + except (OSError, ValueError): + return os.path.abspath(os.path.expanduser(raw)) + + +def _resolved_root_key() -> _ResolvedRoots: + """Return the roots the target set is anchored on. Mirrors how :func:`_home_dir_targets_uncached` derives its anchors, so the cache key changes exactly when the anchors would. Falls back to the @@ -8079,6 +8178,9 @@ def _resolved_root_key() -> tuple[str, str | None, str | None, str]: to ``~/.kiro`` in ``kiro_home()``, already covered by the default form); it is resolved only so a symlinked override keys and anchors identically. + The three adapter roots do the same for the OAuth token leaves in + ``_OVERRIDE_ANCHORED_LEAVES``. + ``logical_home`` is ``Path.home()`` UNRESOLVED. It is a separate anchor, not a duplicate: on a host where ``$HOME`` is itself a symlink (``/home/x`` -> ``/local/home/x`` on cloud desktops) the resolved home spells every target @@ -8094,23 +8196,15 @@ def _resolved_root_key() -> tuple[str, str | None, str | None, str]: home = str(Path.home().resolve()) except (OSError, ValueError): home = logical_home - crew_env = os.environ.get("KIROCREW_HOME") - if crew_env: - try: - crew: str | None = str(Path(crew_env).expanduser().resolve()) - except (OSError, ValueError): - crew = os.path.abspath(os.path.expanduser(crew_env)) - else: - crew = None - kiro_env = os.environ.get("KIRO_HOME") - if kiro_env: - try: - kiro: str | None = str(Path(kiro_env).expanduser().resolve()) - except (OSError, ValueError): - kiro = os.path.abspath(os.path.expanduser(kiro_env)) - else: - kiro = None - return home, crew, kiro, logical_home + return _ResolvedRoots( + home=home, + crew_home=_resolved_env_root("KIROCREW_HOME"), + kiro_home=_resolved_env_root("KIRO_HOME"), + codex_home=_resolved_env_root("CODEX_HOME"), + claude_config_dir=_resolved_env_root("CLAUDE_CONFIG_DIR"), + claude_home=_resolved_env_root("CLAUDE_HOME"), + logical_home=logical_home, + ) def _home_dir_targets(home_dirs: list[str]) -> set[str]: @@ -8367,6 +8461,71 @@ def crew_home_prefixes() -> tuple[str, ...]: return tuple(_CREW_HOME_PREFIXES) +def sandbox_credential_targets(exclude_leaves: tuple[str, ...] = ()) -> tuple[str, ...]: + """Absolute, on-disk-case credential targets for an OS sandbox deny list. + + Applies the SAME anchoring as :func:`_home_dir_targets_uncached` -- the + ``$HOME`` projection of every :data:`_SENSITIVE_HOME_DIRS` leaf, plus each + env-override re-anchor -- so a caller building a sandbox mask inherits the + read gate's anchor rules instead of re-deriving them. That is the whole point + of this living here: a mask that projected leaves under ``Path.home()`` only + would silently miss a credential the operator relocated with + ``KIROCREW_HOME``, ``CLAUDE_CONFIG_DIR`` or ``CLAUDE_HOME``, which is exactly + the drift a hand-maintained list already produced once. + + Unlike the read gate's target set the paths are NOT casefolded: that set + exists to COMPARE against candidate paths, while these are handed to a + sandbox backend to deny on disk, and a casefolded path denies nothing on a + case-sensitive filesystem. + + *exclude_leaves* drops a ``$HOME``-relative leaf (and its override + re-anchors) from the result -- for an adapter whose own OAuth token it must + still be able to read in order to authenticate. Excluding a leaf here only + removes it from THIS mask; the read gate still fences it for the agent's own + file tools, so the two controls keep covering different readers. + + Returns logical paths. The launcher ``os.path.abspath``es what it is handed, + and the macOS profile emits both a ``subpath`` and a ``literal`` deny for each + entry, so both a directory and a plain file leaf are valid entries. + """ + excluded = set(exclude_leaves) + leaves = [d for d in _SENSITIVE_HOME_DIRS if d not in excluded] + resolved = _resolved_root_key() + # BOTH home spellings, reusing the two anchors the read gate already keys on. + # On a host whose home is itself a symlink (``/home/u`` -> ``/local/home/u``) the + # resolved and logical spellings differ, and the read gate can absorb that because + # it realpaths a candidate BEFORE comparing. A sandbox deny list gets no such + # normalisation -- it denies the paths it is handed -- so denying only the resolved + # form would leave every credential reachable through the symlinked one. + home_anchors = {resolved.home, resolved.logical_home} + targets: set[str] = { + os.path.join(anchor, *d.split("/")) for anchor in home_anchors for d in leaves + } + # KIROCREW_HOME: the crew secrets (signing keys, governance ceiling, .env) + # live directly under the override, not under either default crew prefix. + if resolved.crew_home: + for d in leaves: + for prefix in _CREW_HOME_PREFIXES: + if d == prefix or d.startswith(prefix + "/"): + leaf = d[len(prefix) :].lstrip("/") + targets.add( + os.path.join(resolved.crew_home, *leaf.split("/")) + if leaf + else resolved.crew_home + ) + break + # An adapter's OAuth token follows that adapter's own home override. + for leaf, root_fields in _OVERRIDE_ANCHORED_LEAVES: + if leaf in excluded or leaf not in _SENSITIVE_HOME_DIRS: + continue + basename = leaf.split("/")[-1] + for field in root_fields: + root = getattr(resolved, field, None) + if root: + targets.add(os.path.join(root, basename)) + return tuple(sorted(targets)) + + def exfil_query_min_len() -> int: """Public view of the long-query exfiltration threshold (chars).""" return _EXFIL_QUERY_MIN_LEN diff --git a/test/test_acp_backend_credentials.py b/test/test_acp_backend_credentials.py new file mode 100644 index 00000000000..8faec637331 --- /dev/null +++ b/test/test_acp_backend_credentials.py @@ -0,0 +1,170 @@ +"""An ACP adapter's OAuth token store is on the sensitive-path floor. + +Each adapter owns its own sign-in flow and persists its own tokens. Kiro Crew +never reads them — it only ever checks that the file EXISTS so it can name the +right sign-in command — so an agent ``fs_read`` of one buys nothing legitimate +and would let it impersonate the operator against that vendor. + +Claude Code is already selectable, so its leaf being off the floor was a live +read, not a hypothetical. Codex's leaf has the same shape. + +Two properties are pinned separately because they fail apart: + +* the ``$HOME``-rooted default is classified, and the adapter's sibling CONFIG + files are NOT (routing diagnosis reads them and they hold no credential); and +* the leaf is re-anchored under every home override the adapter honours, since + an override moves the token out from under a ``$HOME``-rooted entry. + +Each test is revert-verified: with the corresponding half of the change removed +they fail. +""" + +from __future__ import annotations + +import os + +import pytest + +from kiro_crew import security +from kiro_crew.security import _SENSITIVE_HOME_DIRS, is_sensitive_path + +#: leaf, the env var that moves its parent, and the basename under that override. +ADAPTER_TOKEN_LEAVES: tuple[tuple[str, tuple[str, ...], str], ...] = ( + (".codex/auth.json", ("CODEX_HOME",), "auth.json"), + ( + ".claude/.credentials.json", + ("CLAUDE_CONFIG_DIR", "CLAUDE_HOME"), + ".credentials.json", + ), +) + +#: Sibling files that must STAY readable. Losing these would break routing +#: diagnosis, and unlike the token they carry no credential. +READABLE_SIBLINGS: tuple[str, ...] = ( + ".codex/config.toml", + ".claude/settings.json", + ".claude/settings.local.json", +) + + +def _clear_cache() -> None: + """Drop the TTL target cache so an env change is observed immediately.""" + security._home_targets_cache.clear() + + +@pytest.fixture(autouse=True) +def _isolated_home(monkeypatch, tmp_path): + """Anchor every case on a scratch home with no adapter overrides set. + + The overrides are cleared rather than merely unset-if-absent: a developer + machine that genuinely exports ``CODEX_HOME`` would otherwise make the + default-location assertions pass for the wrong reason. + """ + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setenv("USERPROFILE", str(tmp_path)) + for var in ("CODEX_HOME", "CLAUDE_CONFIG_DIR", "CLAUDE_HOME"): + monkeypatch.delenv(var, raising=False) + _clear_cache() + yield tmp_path + _clear_cache() + + +@pytest.mark.parametrize("leaf,_env_vars,_basename", ADAPTER_TOKEN_LEAVES) +def test_token_leaf_is_on_the_floor(leaf, _env_vars, _basename) -> None: + """The leaf is listed, so the registry and the gate cannot drift apart.""" + assert leaf in _SENSITIVE_HOME_DIRS, ( + f"{leaf} must be in _SENSITIVE_HOME_DIRS; without it an agent fs_read " + "can lift that adapter's OAuth token" + ) + + +@pytest.mark.parametrize("leaf,_env_vars,_basename", ADAPTER_TOKEN_LEAVES) +def test_default_location_is_blocked(_isolated_home, leaf, _env_vars, _basename) -> None: + """The documented ``$HOME``-rooted location is refused.""" + target = os.path.join(str(_isolated_home), *leaf.split("/")) + assert is_sensitive_path(target) is True + + +@pytest.mark.parametrize("sibling", READABLE_SIBLINGS) +def test_sibling_config_stays_readable(_isolated_home, sibling) -> None: + """Only the token leaf is classified, never the whole adapter directory. + + Classifying the directory would be the easy over-broad fix and would break + routing diagnosis, which reads these files. + """ + target = os.path.join(str(_isolated_home), *sibling.split("/")) + assert is_sensitive_path(target) is False, ( + f"{sibling} carries no credential and routing diagnosis reads it; " + "classify the token leaf, not the directory" + ) + + +@pytest.mark.parametrize("leaf,env_vars,basename", ADAPTER_TOKEN_LEAVES) +def test_home_override_is_anchored(monkeypatch, tmp_path, leaf, env_vars, basename) -> None: + """An override moves the token, and the gate follows it. + + One variable at a time: an adapter honouring two roots must cover EACH of + them, and asserting them together would pass while one was missed. + """ + for var in env_vars: + override = tmp_path / f"override-{var.lower()}" + override.mkdir(parents=True, exist_ok=True) + monkeypatch.setenv(var, str(override)) + _clear_cache() + try: + moved = override / basename + assert is_sensitive_path(str(moved)) is True, ( + f"{leaf} moved by {var} is no longer gated; a literal " + "$HOME-rooted entry only covers the default location" + ) + finally: + monkeypatch.delenv(var, raising=False) + _clear_cache() + + +@pytest.mark.parametrize("leaf,env_vars,basename", ADAPTER_TOKEN_LEAVES) +def test_default_location_survives_an_override( + monkeypatch, tmp_path, _isolated_home, leaf, env_vars, basename +) -> None: + """Setting an override ADDS a target; it never drops the default one. + + The override anchoring is additive precisely so a host where the adapter + still uses its default path keeps its protection. + """ + override = tmp_path / "elsewhere" + override.mkdir(parents=True, exist_ok=True) + monkeypatch.setenv(env_vars[0], str(override)) + _clear_cache() + default_target = os.path.join(str(_isolated_home), *leaf.split("/")) + assert is_sensitive_path(default_target) is True + + +def test_override_roots_are_part_of_the_cache_key() -> None: + """A changed override must invalidate the cached target set. + + The TTL cache is keyed on the resolved roots, so a root the BUILDER anchors + on but the KEY omits would serve targets computed for the previous value — + the fail-open shape the resolved-home key already exists to prevent. Asserts + on the key's own fields rather than on cache behaviour, so the reason a + failure happened is visible. + """ + fields = set(security._ResolvedRoots._fields) + for _leaf, root_fields in security._OVERRIDE_ANCHORED_LEAVES: + for field in root_fields: + assert field in fields, ( + f"_OVERRIDE_ANCHORED_LEAVES anchors on {field!r}, which is not a " + "_ResolvedRoots field, so it cannot be part of the cache key" + ) + + +def test_every_anchored_leaf_is_actually_on_the_floor() -> None: + """The override table cannot name a leaf the read tier does not classify. + + Guards the opposite drift from the tests above: an entry removed from + ``_SENSITIVE_HOME_DIRS`` while its override anchor stayed would leave the + table describing protection that no longer exists. + """ + for leaf, _root_fields in security._OVERRIDE_ANCHORED_LEAVES: + assert ( + leaf in _SENSITIVE_HOME_DIRS + ), f"{leaf} is override-anchored but absent from _SENSITIVE_HOME_DIRS" diff --git a/test/test_acp_client_more_coverage.py b/test/test_acp_client_more_coverage.py index 937f80c7690..edf72b5edc8 100644 --- a/test/test_acp_client_more_coverage.py +++ b/test/test_acp_client_more_coverage.py @@ -28,6 +28,7 @@ AcpError, AcpProcessDied, AcpTimeoutError, + AcpToolGateUnroutable, OversizeLineUnrecoverable, _direct_children, _drain_oversize_line, @@ -677,6 +678,70 @@ async def _init(): assert client._kill_process.await_count == 2 # once per attempt + @pytest.mark.asyncio + async def test_tool_gate_refusal_does_not_retry_the_spawn(self, tmp_path): + """A gate refusal is a configuration fact, so a respawn re-reads it. + + ``AcpToolGateUnroutable`` documents itself Non-retryable, but it subclasses + ``AcpError``, so the generic transport ladder used to retry it: attempt 0 + tore the child down, respawned, hit the identical refusal, and only then + raised. That is one wasted spawn plus teardown, and it spends the reconnect + budget the distinct type exists to protect. + + Revert-verified: dropping the dedicated handler makes both counters 2. + """ + client = _client(tmp_path) + spawns = {"n": 0} + + async def _spawn(): + spawns["n"] += 1 + client._process = _live_process() + + async def _init(): + raise AcpToolGateUnroutable("codex routes tool calls around the gate") + + def _reset(): + # Faithful to production: the real _reset_state drops the process + # handle, which is what makes the retry actually RESPAWN. A bare + # MagicMock leaves it set, so _spawn runs once either way and the + # spawn assertion below could never fail. + client._process = None + + client._spawn = _spawn + client._initialize_session = _init + client._snapshot_process_tree = AsyncMock() + client._kill_process = AsyncMock() + client._reset_state = _reset + + with pytest.raises(AcpToolGateUnroutable): + await client.ensure_ready() + + assert spawns["n"] == 1, "the refusal was retried with a fresh process" + assert client._kill_process.await_count == 1 + + def test_sandbox_preflight_translates_the_gate_refusal(self, monkeypatch): + """The RAW gate exception must not escape the preflight. + + ``acp_tool_gate`` is a leaf module that cannot import this one, so its + ``ToolGateUnroutable`` is a plain ``Exception``. That makes it invisible to + BOTH handlers around the spawn: it is not an ``AcpError``, so the transport + ladder cannot see it, and it is not ``AcpToolGateUnroutable``, so the + dedicated non-retrying handler cannot either. Raised raw, a sandbox-floor + refusal escaped ``ensure_ready`` entirely and skipped the cleanup every + other refusal path runs. + + Revert-verified: dropping the translation raises the raw type and fails here. + """ + from kiro_crew import acp_tool_gate + + def _refuse(backend, mode): + raise acp_tool_gate.ToolGateUnroutable("no sandbox backend on this host") + + monkeypatch.setattr(acp_tool_gate, "enforce_sandbox_floor", _refuse) + + with pytest.raises(AcpToolGateUnroutable, match="no sandbox backend"): + acp_client._sandbox_preflight("codex", "standard") + @pytest.mark.asyncio async def test_shutdown_kills_and_resets(self, tmp_path): client = _client(tmp_path) diff --git a/test/test_acp_tool_gate.py b/test/test_acp_tool_gate.py new file mode 100644 index 00000000000..7a8410a911b --- /dev/null +++ b/test/test_acp_tool_gate.py @@ -0,0 +1,598 @@ +"""The tool gate reports routing honestly and refuses what it enforces. + +Two axes that must not be conflated, and each test names which one it is on: + +* **Truth** -- what :func:`routing_verdict` SAYS about a harness. Every harness + gets an honest verdict, including the ones this core does not enforce. +* **Enforcement** -- whether a non-ROUTED verdict REFUSES. Scoped to the + mechanisms this core implements end to end, which today is ``SESSION_CONFIG`` + only. + +Collapsing them is the failure this file exists to prevent: upgrading an +unenforced harness to ``ROUTED`` so it stops refusing would make the picker and +the doctor row assert a guarantee nothing performs. + +Every test is revert-verified: with the corresponding guard removed they fail. +""" + +from __future__ import annotations + +import os +import pathlib + +import pytest + +from kiro_crew import acp_tool_gate as gate +from kiro_crew.acp_backends import ( + ACP_BACKEND_CLAUDE, + ACP_BACKEND_CODEX, + ACP_BACKEND_KAS, + ACP_BACKEND_KIRO, + Routing, + permission_config_for, + routing_for, +) +from kiro_crew.security import sensitive_home_dirs + +AGENT_SPEC_BACKENDS = (ACP_BACKEND_KIRO, ACP_BACKEND_KAS) + + +# ── Truth: what the verdict says ───────────────────────────────────────────── + + +@pytest.mark.parametrize("backend", AGENT_SPEC_BACKENDS) +def test_agent_spec_is_routed_by_construction(backend) -> None: + """kiro and KAS ask because the spawn names an agent; there is nothing to probe.""" + verdict, reason = gate.routing_verdict(backend) + assert verdict is gate.Verdict.ROUTED + assert "names an agent" in reason + + +def test_codex_verdict_names_the_option_it_promises() -> None: + """The SESSION_CONFIG verdict is a promise, so it must say what was promised. + + A bare ROUTED here would be unfalsifiable: the reason string is what lets a + reader check the promise against the apply. + """ + verdict, reason = gate.routing_verdict(ACP_BACKEND_CODEX) + assert verdict is gate.Verdict.ROUTED + assert "mode=read-only" in reason + + +def test_claude_is_indeterminate_not_routed() -> None: + """The unenforced harness is told truthfully, never upgraded to ROUTED. + + This core does not write claude's permission settings, so no read-back can + establish the guarantee. Reporting ROUTED to avoid a refusal would put a claim + in the doctor row that nothing performs. + """ + verdict, _reason = gate.routing_verdict(ACP_BACKEND_CLAUDE) + assert verdict is gate.Verdict.INDETERMINATE + + +def test_unknown_backend_fails_closed() -> None: + """An id the routing table does not name never inherits a neighbour's mechanism.""" + assert routing_for("no-such-harness") is Routing.UNVERIFIED + verdict, _reason = gate.routing_verdict("no-such-harness") + assert verdict is gate.Verdict.INDETERMINATE + + +# ── Enforcement scope ──────────────────────────────────────────────────────── + + +def test_only_session_config_is_enforced() -> None: + """The enforced set is a mechanism list, not a harness allowlist. + + Scoping by mechanism is what makes widening it require IMPLEMENTING one; an + id-based allowlist could be widened by editing a literal. + """ + assert gate.ENFORCED_ROUTINGS == frozenset({Routing.SESSION_CONFIG}) + assert gate.is_enforced(ACP_BACKEND_CODEX) is True + assert gate.is_enforced(ACP_BACKEND_CLAUDE) is False + for backend in AGENT_SPEC_BACKENDS: + assert gate.is_enforced(backend) is False + + +def test_unenforced_harness_does_not_refuse() -> None: + """An INDETERMINATE verdict on an unenforced mechanism starts the session. + + The point of the scoping: this core must not refuse a shipped harness over a + guarantee it never attempted to establish. + """ + gate.enforce_runtime_routing( + ACP_BACKEND_CLAUDE, + "this core does not seed its settings", + verdict=gate.Verdict.INDETERMINATE, + ) + + +# ── Refusal: there is no opt-out ───────────────────────────────────────────── + + +def test_enforced_harness_always_refuses() -> None: + """A routing failure on an enforced mechanism raises before the first prompt.""" + with pytest.raises(gate.ToolGateUnroutable) as excinfo: + gate.enforce_runtime_routing( + ACP_BACKEND_CODEX, + "session/new did not advertise config option 'mode'", + ) + message = str(excinfo.value) + assert "denied-command rules" in message + assert "sensitive-path block" in message + assert "governance ceiling" in message + + +def test_no_local_opt_out_exists() -> None: + """A security control with a local off-switch is not a control. + + An earlier revision carried ``agent.acp_backend_allow_ungated_tools``: a local + config bool that started the session with the compensating control off. That is + the shape the central governance ceiling exists to forbid, since a managed + fleet could allow the harness while a user's own config disabled the gate. + Pinned as an ABSENCE so it cannot be reintroduced as a convenience. + """ + import inspect + + assert not hasattr(gate, "OPT_OUT_KEY") + params = inspect.signature(gate.enforce_runtime_routing).parameters + assert "allow_ungated" not in params + source = inspect.getsource(gate) + assert "acp_backend_allow_ungated_tools" not in source.split('"""')[0] + + +def test_refusal_carries_the_remedy() -> None: + """The message names the concrete change, not just the problem.""" + with pytest.raises(gate.ToolGateUnroutable) as excinfo: + gate.enforce_runtime_routing( + ACP_BACKEND_CODEX, + "not advertised", + remedy=gate.remediation_for(ACP_BACKEND_CODEX), + ) + assert "adapter that advertises" in str(excinfo.value) + + +def test_indeterminate_refuses_alongside_bypassed() -> None: + """ "Cannot tell" must not be treated as "probably fine". + + A guarantee that lapses whenever evidence is missing is not a guarantee, so + both verdicts refuse identically. Only the wording differs. + """ + for verdict in (gate.Verdict.BYPASSED, gate.Verdict.INDETERMINATE): + with pytest.raises(gate.ToolGateUnroutable): + gate.enforce_runtime_routing(ACP_BACKEND_CODEX, "reason", verdict=verdict) + + +# ── The OS-boundary mask compensating for unrouted reads ───────────────────── + + +def test_mask_covers_the_whole_read_gate_floor() -> None: + """The compensation must cover what the control it compensates for covers. + + Codex's passive reads never reach ``HookManager.on_tool_call``, so the floor + cannot see them and this mask is the only thing standing in. An enumerated + subset left ``.claude/.credentials.json``, ``.netrc``, ``.git-credentials``, + ``.pypirc`` and ``.npmrc`` readable by the child while the floor called them + never-readable. Derived, so the two cannot drift. + """ + masked = set(gate.adapter_hidden_credential_dirs(ACP_BACKEND_CODEX)) + home = os.path.expanduser("~") + own = set(gate.ADAPTER_OWN_CREDENTIAL_LEAVES[ACP_BACKEND_CODEX]) + for leaf in sensitive_home_dirs(): + if leaf in own: + continue + assert os.path.join(home, *leaf.split("/")) in masked, ( + f"{leaf} is on the read-gate floor but readable by the codex child; " + "the mask must cover the floor it compensates for" + ) + + +@pytest.mark.parametrize( + "leaf", + ( + ".claude/.credentials.json", + ".netrc", + ".git-credentials", + ".pypirc", + ".npmrc", + ".aws", + ".ssh", + ), +) +def test_named_credential_leaves_are_masked(leaf) -> None: + """The specific leaves an enumerated mask missed, pinned by name. + + Named individually as well as by derivation: the derivation test would keep + passing if a leaf were dropped from the FLOOR too, and these are the ones a + codex agent driven by untrusted content would go after. + """ + masked = set(gate.adapter_hidden_credential_dirs(ACP_BACKEND_CODEX)) + home = os.path.expanduser("~") + assert os.path.join(home, *leaf.split("/")) in masked + + +def test_the_harness_keeps_its_own_token_readable() -> None: + """The adapter must read its own credential to authenticate. + + Excluding it is safe because the two controls cover different readers: the + floor still blocks the AGENT's file tools from this leaf, while the mask only + governs what the adapter's child process can open. + """ + masked = set(gate.adapter_hidden_credential_dirs(ACP_BACKEND_CODEX)) + own = os.path.join(os.path.expanduser("~"), ".codex", "auth.json") + assert own not in masked + assert ".codex/auth.json" in sensitive_home_dirs(), ( + "the agent's own file tools must still be fenced from the token even " + "though the adapter child may read it" + ) + + +@pytest.mark.parametrize("backend", (*AGENT_SPEC_BACKENDS, ACP_BACKEND_CLAUDE)) +def test_unenforced_harness_gets_no_mask(backend) -> None: + """The first-class path keeps byte-identical sandbox arguments. + + An adapter-driven change must not alter what the Kiro spawn is handed. + """ + assert gate.adapter_hidden_credential_dirs(backend) == () + + +def test_mask_paths_are_absolute() -> None: + """``wrap_argv`` abspaths what it is handed, so a bare leaf would deny nothing. + + A relative ``.aws`` would resolve against the CWD and silently mask an + unrelated path (or nothing), which fails OPEN. + """ + for path in gate.adapter_hidden_credential_dirs(ACP_BACKEND_CODEX): + assert os.path.isabs(path) + + +# ── session_config_issue: the other half of the promise ─────────────────────── + + +def test_advertised_option_and_value_is_no_issue() -> None: + option_id, value = permission_config_for(ACP_BACKEND_CODEX) + advertised = [{"id": option_id, "options": [{"value": value}, {"value": "agent"}]}] + assert gate.session_config_issue(ACP_BACKEND_CODEX, advertised) == "" + + +@pytest.mark.parametrize( + "config_options,expected_fragment", + [ + (None, "did not advertise configOptions"), + ([], "did not advertise config option"), + ([{"id": "unrelated", "options": [{"value": "x"}]}], "did not advertise config option"), + ([{"id": "mode", "options": "not-a-list"}], "has no values"), + ([{"id": "mode", "options": [{"value": "agent"}]}], "does not advertise required value"), + ], +) +def test_missing_or_wrong_advertisement_is_an_issue(config_options, expected_fragment) -> None: + """Each shape fails closed with its own reason. + + Permission routing is stricter than optional model/effort config: a missing + option cannot be shrugged off as lazy advertising, because the first prompt + would then run ungated. + """ + issue = gate.session_config_issue(ACP_BACKEND_CODEX, config_options) + assert issue, "a missing or wrong advertisement must not read as satisfied" + assert expected_fragment in issue + + +@pytest.mark.parametrize("backend", (*AGENT_SPEC_BACKENDS, ACP_BACKEND_CLAUDE)) +def test_non_session_config_harness_has_no_config_issue(backend) -> None: + """The check is scoped to the mechanism it belongs to.""" + assert gate.session_config_issue(backend, None) == "" + + +# ── the promise and the apply cannot drift ─────────────────────────────────── + + +def test_every_session_config_harness_names_its_option() -> None: + """A harness declaring the mechanism without an option would report a false ROUTED. + + ``routing_verdict`` builds its ROUTED reason from the option; a harness that + declared SESSION_CONFIG and named none would promise something the apply could + never perform, so the verdict degrades to INDETERMINATE instead. This pins that + no shipped harness is in that state. + """ + from kiro_crew.acp_backends import ACP_BACKEND_ROUTING + + for backend, routing in ACP_BACKEND_ROUTING.items(): + if routing is not Routing.SESSION_CONFIG: + continue + option_id, value = permission_config_for(backend) + assert option_id and value, ( + f"{backend!r} declares SESSION_CONFIG routing but names no config " + "option, so routing_verdict would promise an apply that cannot run" + ) + + +def test_every_enforced_harness_declares_its_own_credential() -> None: + """An enforced harness with no named token store would be masked out of its own auth. + + ``adapter_hidden_credential_dirs`` denies the whole floor minus the harness's + own leaf, so a harness absent from that table gets its token masked and cannot + authenticate. Fails here rather than as an opaque auth error on first use. + """ + from kiro_crew.acp_backends import ACP_BACKEND_ROUTING + + for backend, routing in ACP_BACKEND_ROUTING.items(): + if routing not in gate.ENFORCED_ROUTINGS: + continue + assert backend in gate.ADAPTER_OWN_CREDENTIAL_LEAVES, ( + f"{backend!r} is enforced, so the mask denies it the whole floor; it " + "must name its own credential leaf or it cannot authenticate" + ) + + +def test_every_enforced_harness_reaches_the_spawn_preflight() -> None: + """An enforced harness whose spawn arm skips the preflight would start unmasked. + + The preflight (refuse-then-mask) is invoked from inside each adapter's OWN arm + of ``AcpClient._spawn`` rather than from a gate on the shared path, so the kiro + construction path gains no conditional and no awaited step in service of an + adapter (harness-parity H13). The cost of that placement is that a new enforced + harness needs its own call: forgetting one would spawn it with no mask and no + refusal. This pins one preflight call site per enforced harness, so the + omission fails here instead of silently shipping an unmasked adapter. + """ + import ast + import inspect + import textwrap + + from kiro_crew.acp.client import AcpClient + from kiro_crew.acp_backends import ACP_BACKEND_ROUTING + + enforced = { + backend + for backend, routing in ACP_BACKEND_ROUTING.items() + if routing in gate.ENFORCED_ROUTINGS + } + assert enforced, "the gate enforces no mechanism; this ratchet would be vacuous" + + spawn_tree = ast.parse(textwrap.dedent(inspect.getsource(AcpClient._spawn))) + call_sites = sum( + 1 + for node in ast.walk(spawn_tree) + if isinstance(node, ast.Name) and node.id == "_sandbox_preflight" + ) + assert call_sites == len(enforced), ( + f"{len(enforced)} enforced harness(es) {sorted(enforced)!r} but " + f"{call_sites} _sandbox_preflight call site(s) in AcpClient._spawn: every " + "enforced harness must invoke the preflight inside its own spawn arm" + ) + + +def test_mask_reanchors_a_relocated_credential( + monkeypatch: pytest.MonkeyPatch, tmp_path: pathlib.Path +) -> None: + """A credential moved by an env override is still denied to the child. + + Regression: the mask projected every leaf under ``Path.home()``, so an operator + who relocated a store with ``CLAUDE_CONFIG_DIR`` (or ``KIROCREW_HOME``) kept the + LIVE secret readable while the sandbox was handed a home-rooted path that denied + nothing. The mask now shares the read gate's anchor rules. With the delegation + reverted to a home-only projection this fails. + """ + # tmp_path, not a hardcoded POSIX path: the assertion is about the OVERRIDE + # being honoured, and "/tmp/..." is not a path Windows resolves to itself. + monkeypatch.setenv("CLAUDE_CONFIG_DIR", str(tmp_path / "relocated-claude")) + # No cache reset needed: ``_resolved_root_key`` re-reads the environment on + # every call, which is what makes the mask honour a late override at all. + masked = gate.adapter_hidden_credential_dirs(ACP_BACKEND_CODEX) + + assert any( + "relocated-claude" in entry for entry in masked + ), "the relocated claude credential store is not denied to the enforced child" + + +def test_mask_still_exposes_the_adapters_own_token() -> None: + """The harness must keep reading its OWN token or it cannot authenticate. + + The deliberate asymmetry: this mask fences the CHILD, while the read gate still + fences the same leaf for the agent's own file tools, so the two controls cover + different readers. + """ + masked = gate.adapter_hidden_credential_dirs(ACP_BACKEND_CODEX) + own = gate.ADAPTER_OWN_CREDENTIAL_LEAVES[ACP_BACKEND_CODEX][0] + basename = own.split("/")[-1] + assert not any( + entry.endswith(basename) for entry in masked + ), "the adapter's own OAuth token was masked, which would break its auth" + + +def test_sandbox_off_refuses_an_enforced_adapter(monkeypatch: pytest.MonkeyPatch) -> None: + """With the sandbox off the mask is never applied, so the session must refuse. + + ``wrap_argv`` returns from its ``mode == "off"`` branch before it applies + ``extra_hidden_dirs``. Because ACP v1 cannot force a prompt for a passive read, + an enforced adapter started that way has NO compensating control and is strictly + weaker than the gate-routed harnesses beside it. Revert-verified: without the + guard this raises nothing. + """ + from kiro_crew import sandbox + + monkeypatch.setattr(sandbox, "_governance_sandbox_floor", lambda: None) + with pytest.raises(gate.ToolGateUnroutable): + gate.enforce_sandbox_floor(ACP_BACKEND_CODEX, "off") + + +def test_sandbox_off_is_allowed_for_an_unenforced_harness( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The first-class path keeps byte-identical spawn behaviour. + + The guard must not refuse a harness this core does not enforce -- it never + attempted the guarantee, so refusing would break the Kiro path over a promise + it does not make. + """ + from kiro_crew import sandbox + + monkeypatch.setattr(sandbox, "_governance_sandbox_floor", lambda: None) + monkeypatch.setattr(sandbox, "_inside_kirocrew_sandbox", lambda: False) + gate.enforce_sandbox_floor(ACP_BACKEND_KIRO, "off") + + +def test_governed_floor_keeps_an_enforced_adapter_startable( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A ceiling that raises ``off`` must not trigger the refusal. + + The guard is keyed on the EFFECTIVE tier, so a governed host whose + ``sandbox.min_level`` floor overrides the config value still gets the mask and + must start. Keying it on the raw config value instead fails this. + + ``detect_backend`` is pinned to a PRESENT backend deliberately. A governed host + that can actually satisfy its own floor has one, and without the pin this test + silently depended on the CI host having none -- which made it read as "a + no-backend host must stay startable", a claim it never meant and which + ``test_no_backend_refuses_under_a_governance_floor_too`` now contradicts. + """ + from kiro_crew import sandbox + + monkeypatch.setattr(sandbox, "_governance_sandbox_floor", lambda: "standard") + monkeypatch.setattr(sandbox, "_inside_kirocrew_sandbox", lambda: False) + monkeypatch.setattr(sandbox, "detect_backend", lambda **_: "namespace") + gate.enforce_sandbox_floor(ACP_BACKEND_CODEX, "off") + + +def test_no_backend_refuses_under_a_governance_floor_too( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """No backend refuses even when a governance floor forbids unsandboxed execution. + + An earlier revision answered from the floor here, reasoning that a floor + mandating isolation makes ``wrap_argv``'s unwrapped return unreachable, so + nothing could start unmasked. That held only for as long as the floor did: a + ceiling LOOSENED between this preflight and the spawn drops the very refusal + that made the answer safe, and the floor is mutable config read here just like + the opt-in was. With no backend nothing can carry the mask at all, so the + verdict must not be derived from policy in either direction. + + Revert-verified: returning ``_floor_mandates_sandbox(floor)`` passes this + through. + """ + from kiro_crew import sandbox + + monkeypatch.setattr(sandbox, "_governance_sandbox_floor", lambda: "strict") + monkeypatch.setattr(sandbox, "_inside_kirocrew_sandbox", lambda: False) + monkeypatch.setattr(sandbox, "detect_backend", lambda **_: "none") + monkeypatch.setattr(sandbox, "_allow_unsandboxed_exec", lambda: True) + with pytest.raises(gate.ToolGateUnroutable): + gate.enforce_sandbox_floor(ACP_BACKEND_CODEX, "standard") + + +def test_env_root_override_is_read_verbatim( + monkeypatch: pytest.MonkeyPatch, tmp_path: pathlib.Path +) -> None: + """A trailing space is a legal path character and must not be stripped. + + ``_valid_override_home`` takes ``KIROCREW_HOME`` raw and ``config_dir`` mkdirs + whatever it names, so stripping here anchored the sensitive-target set on + ```` while the process actually ran out of ``" "`` -- leaving the real + ``.env``, signing keys and governance files outside the floor. Revert-verified: + restoring ``.strip()`` fails this. + """ + from kiro_crew import security + + # Compare against the SAME resolution the helper performs on the raw value, + # rather than a POSIX literal: the contract under test is "the value is not + # stripped", and asserting an absolute spelling instead tests the platform's + # path semantics (Windows resolves a bare "/tmp/..." onto the current drive). + raw = str(tmp_path / "crew-home") + " " + monkeypatch.setenv("KIROCREW_HOME", raw) + expected = str(pathlib.Path(raw).expanduser().resolve()) + assert security._resolved_env_root("KIROCREW_HOME") == expected + monkeypatch.setenv("KIROCREW_HOME", "") + assert security._resolved_env_root("KIROCREW_HOME") is None + + +def test_no_backend_with_opt_in_refuses_an_enforced_adapter( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A non-``off`` tier is NOT proof the mask will be applied. + + On a host with no sandbox backend (Docker/CI where ``unshare(CLONE_NEWUSER)`` is + blocked) and ``sandbox_allow_unsandboxed_exec`` opted in, ``wrap_argv`` returns the + argv unwrapped, so ``extra_hidden_dirs`` never lands. The guard must refuse that + too. Revert-verified: keying it on ``effective_sandbox_mode(mode) != "off"`` + passes this configuration straight through. + """ + from kiro_crew import sandbox + + monkeypatch.setattr(sandbox, "_governance_sandbox_floor", lambda: None) + monkeypatch.setattr(sandbox, "detect_backend", lambda **_: "none") + monkeypatch.setattr(sandbox, "_allow_unsandboxed_exec", lambda: True) + with pytest.raises(gate.ToolGateUnroutable): + gate.enforce_sandbox_floor(ACP_BACKEND_CODEX, "standard") + + +def test_no_backend_refuses_even_without_the_opt_in( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """No sandbox backend refuses whether or not unsandboxed exec is opted in. + + An earlier revision answered "the mask applies" here, on the grounds that + ``wrap_argv`` would raise ``SandboxUnavailableError`` anyway with better + host-specific remedy text, so guarding twice only blurred the diagnostic. That + made the verdict depend on ``_allow_unsandboxed_exec()`` -- MUTABLE config, read + at preflight and acted on at the spawn. Flipping it on in that window left a + session that had already passed the guard taking wrap_argv's unwrapped path with + the credential paths readable, so the verdict must not consult it at all. + + Revert-verified: restoring the opt-in read makes this pass through instead. + """ + from kiro_crew import sandbox + + monkeypatch.setattr(sandbox, "_governance_sandbox_floor", lambda: None) + monkeypatch.setattr(sandbox, "detect_backend", lambda **_: "none") + monkeypatch.setattr(sandbox, "_allow_unsandboxed_exec", lambda: False) + monkeypatch.setattr(sandbox, "_inside_kirocrew_sandbox", lambda: False) + with pytest.raises(gate.ToolGateUnroutable): + gate.enforce_sandbox_floor(ACP_BACKEND_CODEX, "standard") + + +def test_no_backend_verdict_ignores_the_mutable_opt_in() -> None: + """The no-backend verdict is identical for both opt-in states. + + Pins the absence of the read itself rather than one configuration's outcome: a + future edit that reintroduces ``_allow_unsandboxed_exec()`` on this branch makes + the two calls disagree and fails here. + """ + import unittest.mock as _mock + + from kiro_crew import sandbox + + verdicts = set() + for opted_in in (True, False): + with ( + _mock.patch.object(sandbox, "_governance_sandbox_floor", lambda: None), + _mock.patch.object(sandbox, "detect_backend", lambda **_: "none"), + _mock.patch.object(sandbox, "_inside_kirocrew_sandbox", lambda: False), + _mock.patch.object(sandbox, "_allow_unsandboxed_exec", lambda: opted_in), + ): + verdicts.add(sandbox.credential_mask_applies("standard")) + assert verdicts == {False}, ( + "credential_mask_applies must answer False for a host with no sandbox " + f"backend regardless of the unsandboxed-exec opt-in; got {verdicts!r}" + ) + + +def test_nested_sandbox_passthrough_refuses_an_enforced_adapter( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Inside an existing Kiro Crew sandbox the mask is never applied either. + + A nested re-wrap is impossible by design (Linux seccomp denies the unshare, macOS + Seatbelt refuses sandbox_apply), so wrap_argv passes the argv through and + ``extra_hidden_dirs`` never lands. The outer sandbox is not a substitute: the + standard tier deliberately leaves ``~/.aws`` / ``~/.ssh`` / ``~/.kube`` readable + for kiro-cli's sake, which is exactly what this mask exists to close for an + enforced adapter. Revert-verified: without the nested branch the backend probe + returns a real backend and the guard passes this configuration through. + """ + from kiro_crew import sandbox + + monkeypatch.setattr(sandbox, "_governance_sandbox_floor", lambda: None) + monkeypatch.setattr(sandbox, "_inside_kirocrew_sandbox", lambda: True) + monkeypatch.setattr(sandbox, "_macos_sandbox_state", lambda: None) + with pytest.raises(gate.ToolGateUnroutable): + gate.enforce_sandbox_floor(ACP_BACKEND_CODEX, "standard") diff --git a/test/test_agent_backend_editable.py b/test/test_agent_backend_editable.py index dc5d63e04c5..eac87101317 100644 --- a/test/test_agent_backend_editable.py +++ b/test/test_agent_backend_editable.py @@ -31,7 +31,7 @@ #: 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}) +NOT_SHIPPED_SELECTABLE: frozenset = frozenset() @pytest.fixture @@ -153,5 +153,7 @@ def test_baseline_ships_every_known_backend(): 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_BACKEND_KIRO, ACP_BACKEND_CLAUDE, ACP_BACKEND_KAS, ACP_BACKEND_CODEX] + ) 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 967cc116a15..46875c679a2 100644 --- a/test/test_agent_sdk_backend_install.py +++ b/test/test_agent_sdk_backend_install.py @@ -73,6 +73,103 @@ def _stub_resolvers( monkeypatch.setattr(client, "_resolve_claude_code_executable", lambda: claude_cli) +# ── The codex driver seams ── + + +class TestCodexDriverSeams: + """The three functions ``_probe_codex`` reads its verdict from. + + Tested at the driver rather than only through the probe because the + cached-negative logic is the part with a real hazard: an operator installs the + adapter a MISSING row told them to install, and every spawn in the running + gateway still reuses the cached ``None`` until a restart. Reporting + ``restart_required`` instead of a bare ``installed`` is what keeps the panel + from promising something the next session breaks. + """ + + def test_resolves_reports_a_runnable_argv(self, monkeypatch): + """One component, unlike claude's two: the adapter ships its own Codex binary.""" + from kiro_crew.acp import client + from kiro_crew.agent_sdk.drivers import acp as driver + + monkeypatch.setattr(client, "_resolve_codex_acp_bin", lambda: (["node", "/n/c.js"], "/p")) + assert driver.codex_adapter_resolves() is True + + def test_resolves_reports_absence(self, monkeypatch): + """``(None, searched_path)`` is the resolver's own "not found", not an error.""" + from kiro_crew.acp import client + from kiro_crew.agent_sdk.drivers import acp as driver + + monkeypatch.setattr(client, "_resolve_codex_acp_bin", lambda: (None, "/searched")) + assert driver.codex_adapter_resolves() is False + + def test_unresolved_cache_is_not_a_negative(self, monkeypatch): + """No session has needed the adapter yet, so the fresh answer is the true one. + + Reading the sentinel as a negative would report ``restart_required`` on a + gateway that has simply never spawned codex. + """ + from kiro_crew.acp import client + from kiro_crew.agent_sdk.drivers import acp as driver + + monkeypatch.setattr(client, "_codex_acp_argv_cache", client._UNRESOLVED) + assert driver.codex_adapter_cached_negative() is False + + def test_absent_cache_attribute_is_not_a_negative(self, monkeypatch): + """A build without the global must not read as a cached failure.""" + from kiro_crew.acp import client + from kiro_crew.agent_sdk.drivers import acp as driver + + monkeypatch.delattr(client, "_codex_acp_argv_cache", raising=False) + assert driver.codex_adapter_cached_negative() is False + + def test_cached_negative_is_reported(self, monkeypatch): + """A cached ``None`` is the case the whole function exists for. + + The adapter may be on disk NOW while this process still refuses to spawn + it, so the row has to say "restart" rather than "installed". + """ + from kiro_crew.acp import client + from kiro_crew.agent_sdk.drivers import acp as driver + + monkeypatch.setattr(client, "_codex_acp_argv_cache", (None, "/searched")) + assert driver.codex_adapter_cached_negative() is True + + def test_cached_positive_is_not_a_negative(self, monkeypatch): + """A cached runnable argv means spawns work; nothing to disclose.""" + from kiro_crew.acp import client + from kiro_crew.agent_sdk.drivers import acp as driver + + monkeypatch.setattr(client, "_codex_acp_argv_cache", (["node", "/n/c.js"], "/p")) + assert driver.codex_adapter_cached_negative() is False + + def test_unreadable_cache_shape_fails_safe(self, monkeypatch): + """A cache that will not unpack must not crash a dashboard GET. + + Fails toward "no disclosure" rather than toward an exception: the row is + built on a read-only path that must degrade, not 500. + """ + from kiro_crew.acp import client + from kiro_crew.agent_sdk.drivers import acp as driver + + monkeypatch.setattr(client, "_codex_acp_argv_cache", object()) + assert driver.codex_adapter_cached_negative() is False + + def test_install_command_names_the_package_the_resolver_searches_for(self): + """The advice and the resolution ladder must agree by construction. + + A global install of the SCOPED package puts the UNSCOPED binary on PATH, + which is what the ladder looks for -- so the command is built from the same + constant rather than restated, and cannot drift from what satisfies it. + """ + from kiro_crew.acp.client import CODEX_ACP_NPM_PKG + from kiro_crew.agent_sdk.drivers import acp as driver + + command = driver.codex_adapter_install_command() + assert command == f"npm i -g {CODEX_ACP_NPM_PKG}" + assert CODEX_ACP_NPM_PKG in command + + # ── Per-backend verdicts ── @@ -510,11 +607,14 @@ def test_owner_gets_one_row_per_backend_in_the_pinned_shape(self, monkeypatch): # 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"] == "" + # codex now has a probe, so its row carries a real verdict rather than + # ``unknown``. That is the whole reason it could be offered: the operator + # gets the component name and the command that installs it. + assert by_policy["codex"]["installed"] == "missing" + assert by_policy["codex"]["missing_components"] == ["codex-acp"] + assert by_policy["codex"]["install_command"].startswith("npm i -g ") + # ``selectable`` stays False here because this test PINS the live enum to + # ``["", "kas"]`` above; it asserts the payload shape, not the registry. assert by_policy["codex"]["selectable"] is False def test_an_unknown_row_names_no_components(self, monkeypatch): diff --git a/test/test_harness_parity.py b/test/test_harness_parity.py index c1cdc9d6f20..cffc032991c 100644 --- a/test/test_harness_parity.py +++ b/test/test_harness_parity.py @@ -414,19 +414,46 @@ 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. +def test_codex_is_selectable_and_answerable() -> None: + """H1/H8: a harness may only be offered once the build can answer for it. + + Codex was withheld for one stated reason -- ``backend_install.py`` had no probe, + so its install row could only read ``unknown`` and a failed session arrived with + nothing to act on. The probe closes that, which is what makes the switch + honest rather than merely present. + + Asserted TOGETHER on purpose: selectability without a probe is the exact state + the withholding existed to prevent, so a future change that removed the probe + while leaving the baseline entry would fail here rather than silently ship a + switch with nothing behind it. """ + from kiro_crew.agent_sdk.backend_install import _PROBES + 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() + assert ACP_BACKEND_CODEX in BASELINE_SELECTABLE_BACKENDS + assert ACP_BACKEND_CODEX in selectable_backends() + assert ACP_BACKEND_CODEX in _PROBES, ( + "codex is offered in the switch, so backend_install must be able to say " + "what is missing when a session fails to start" + ) + + +def test_codex_tool_calls_are_gated_before_it_is_offered() -> None: + """A selectable harness must route its tool calls, or the switch is a trap. + + This is the invariant that makes admission mean something: the picker offering + an id and the gate being armed for it are separate facts, and selectability + without routing would put the operator's narrowing silently out of circuit. + """ + from kiro_crew import acp_tool_gate + + verdict, _reason = acp_tool_gate.routing_verdict(ACP_BACKEND_CODEX) + assert verdict is acp_tool_gate.Verdict.ROUTED + assert acp_tool_gate.is_enforced(ACP_BACKEND_CODEX) is True + assert acp_tool_gate.adapter_hidden_credential_dirs(ACP_BACKEND_CODEX), ( + "ACP v1 cannot require a prompt for a passive read, so the credential " + "homes must be denied at the OS boundary instead" + ) def test_codex_carries_its_own_provider_label() -> None: diff --git a/test/test_sandbox_cc_mode.py b/test/test_sandbox_cc_mode.py index 1a91b5fb128..2252b5a3b58 100644 --- a/test/test_sandbox_cc_mode.py +++ b/test/test_sandbox_cc_mode.py @@ -32,7 +32,8 @@ def _neutralize_sandbox_env(monkeypatch): """Prevent the 'already inside sandbox' passthrough on sandboxed hosts.""" monkeypatch.delenv("KIROCREW_SANDBOX_ACTIVE", raising=False) monkeypatch.setattr( - _sb_mod, "_KIRO_INTERNAL_SETTINGS_PATH", + _sb_mod, + "_KIRO_INTERNAL_SETTINGS_PATH", "/nonexistent/kirocrew-test/amazon-internal.json", ) @@ -143,6 +144,28 @@ def test_extra_hidden_directory_denies_reads_and_writes(self): assert '(deny file-write* (subpath "/private/kiro/crew"))' in profile assert '(deny file-link (subpath "/private/kiro/crew"))' in profile + def test_extra_hidden_file_leaf_also_gets_a_literal_deny(self): + """A file-shaped entry needs a ``literal`` rule, not only a ``subpath`` one. + + Most of what the adapter credential mask passes here is a plain FILE -- + ``.codex/auth.json``, ``.claude/.credentials.json``, ``.netrc``, + ``.git-credentials``, ``sel_hmac.key`` -- and whether a ``subpath`` rule + alone denies a non-directory was asserted in three comments in this tree + while the ``crew_hidden`` branch of the same function said the opposite + ("A leaf may be a plain file, which no subpath rule addresses"). Nothing + executes ``sandbox-exec`` here, so that could not be settled by test; the + profile emits BOTH shapes instead, and this pins the literal so the mask + never depends on the unverified reading again. + """ + leaf = "/Users/someone/.netrc" + profile = _build_seatbelt_profile("standard", extra_hidden_dirs=(leaf,)) + + assert f'(deny file-read* (literal "{leaf}"))' in profile + assert f'(deny file-write* (literal "{leaf}"))' in profile + assert f'(deny file-link (literal "{leaf}"))' in profile + # the subpath rule stays -- a directory entry still needs it + assert f'(deny file-read* (subpath "{leaf}"))' in profile + def test_cc_does_not_deny_aws(self): """CC seatbelt does NOT deny .aws — macOS needs full .aws access for credential_process and SSO token caches. LLM deny patterns provide @@ -331,8 +354,9 @@ def test_standard_spawn_strips_channel_secrets(self, monkeypatch): for key, value in _FAKE_CHANNEL_ENV.items(): monkeypatch.setenv(key, value) monkeypatch.setenv("KIROCREW_UNRELATED_KEEPME", "keep-this-value") - with patch("kiro_crew.sandbox.detect_backend", return_value="none"), patch( - "kiro_crew.sandbox._allow_unsandboxed_exec", return_value=True + with ( + patch("kiro_crew.sandbox.detect_backend", return_value="none"), + patch("kiro_crew.sandbox._allow_unsandboxed_exec", return_value=True), ): _argv, env, cleanup = sandboxed_spawn_argv(["echo", "hi"], mode="standard") try: @@ -711,9 +735,7 @@ def test_an_unreadable_known_hosts_aborts_setup(self, tmp_path: Path) -> None: stderr: list[str] = [] with pytest.raises(OSError): - _run_known_hosts_pre_read( - known_hosts=str(kh), tmp_path=tmp_path, stderr_sink=stderr - ) + _run_known_hosts_pre_read(known_hosts=str(kh), tmp_path=tmp_path, stderr_sink=stderr) # The refusal and its diagnostic are ONE behaviour observed from ONE setup, # so they are asserted together. Refusing silently would strand the operator