diff --git a/src/kiro_crew/kiro_cli.py b/src/kiro_crew/kiro_cli.py index ecb784fa725..ea99a511bd8 100644 --- a/src/kiro_crew/kiro_cli.py +++ b/src/kiro_crew/kiro_cli.py @@ -258,8 +258,20 @@ def resolve_kiro_cli( platform_name: str | None = None, home: Path | None = None, environ: Mapping[str, str] | None = None, + include_inherited_path: bool = True, ) -> str | None: - """Return the first executable Kiro CLI candidate, if one exists.""" + """Return the first executable Kiro CLI candidate, if one exists. + + ``include_inherited_path=False`` forwards to + :func:`find_kiro_cli_candidates` and drops the inherited ``PATH`` from the + candidate set. What remains is the fixed known install directories plus the + explicit ``KIROCREW_KIRO_BIN`` override, which is deliberately still + honoured: it is set by the operator who starts the gateway, not named by a + directory an agent can plant a file in. Unattended callers pass the keyword + so a ``PATH`` leading with an agent-writable directory cannot choose what + they execute; interactive ones keep the default, where a nonstandard install + on ``PATH`` is a convenience rather than an exposure. + """ resolved_platform = platform_name or sys.platform resolved_home = home or Path.home() @@ -268,6 +280,6 @@ def resolve_kiro_cli( resolved_platform, resolved_home, resolved_environ, - include_inherited_path=True, + include_inherited_path=include_inherited_path, ) return candidates[0] if candidates else None diff --git a/src/kiro_crew/slack/gateway.py b/src/kiro_crew/slack/gateway.py index 85ab3f7a8bd..2e088073349 100644 --- a/src/kiro_crew/slack/gateway.py +++ b/src/kiro_crew/slack/gateway.py @@ -172,6 +172,7 @@ ) from kiro_crew.history import ConversationLog, HistoryConsolidator from kiro_crew.hooks import HookManager, HooksConfig, hooks_config_from_config_dict +from kiro_crew.kiro_cli import resolve_kiro_cli from kiro_crew.learn import LessonStore from kiro_crew.llm_helpers import ( PromptBusyExhaustedError, @@ -1462,6 +1463,73 @@ def _channel_transport_permitted(member: str) -> bool: return False +#: Budget for pinning kiro-cli's path before an unattended spawn. The lookup is +#: a handful of `stat` calls, but they are under the home directory and +#: `_warn_if_kiro_cli_outdated` awaits them BEFORE `_init_dashboard` binds its +#: socket — so on an unresponsive network-mounted home an unbounded lookup would +#: keep the gateway from ever coming up. Overrunning the budget refuses the +#: spawn, exactly as an absent binary does. +_KIRO_CLI_RESOLVE_TIMEOUT_SECS = 5.0 + + +def _kiro_cli_pin_probe() -> tuple[str | None, bool]: + """``(pinned path, an unpinned install exists)`` — the sync half of the pin. + + The second element separates the two ways the pin can come back empty, which + a caller must report differently: kiro-cli is not installed at all (nothing + to say — the backend is optional), or it IS installed somewhere the pin does + not accept, which is a state an operator needs told about. + """ + + pinned = resolve_kiro_cli(include_inherited_path=False) + if pinned is not None: + return pinned, False + return None, resolve_kiro_cli() is not None + + +async def _pinned_kiro_cli(purpose: str) -> str | None: + """kiro-cli's absolute path for an unattended spawn, or ``None`` to refuse. + + Neither unattended spawn may exec a bare argv0: the gateway's inherited + ``PATH`` can lead with an agent-writable directory (a worktree venv's + ``bin``), and whatever that names would decide the payload. So the candidate + set is the fixed known install directories plus the operator's own + ``KIROCREW_KIRO_BIN``, with the inherited ``PATH`` excluded. + + That set does not cover every install: a system-wide one outside + ``known_kiro_cli_dirs`` — a root-owned ``/usr/local/bin`` on Linux — is + refused here while sessions keep launching it off ``PATH``. Refusing is the + right default for a spawn with no operator present, but being SILENT about + it is not: the resulting host never auto-updates and never warns it is + outdated, with nothing in the log to say why. Hence the warning naming the + override, and hence its condition — an install the pin declined is worth a + line, a backend that simply is not installed is not. + + Off the loop and bounded: see :data:`_KIRO_CLI_RESOLVE_TIMEOUT_SECS`. + """ + + try: + pinned, unpinned_exists = await asyncio.wait_for( + asyncio.to_thread(_kiro_cli_pin_probe), + timeout=_KIRO_CLI_RESOLVE_TIMEOUT_SECS, + ) + except (TimeoutError, asyncio.TimeoutError): + logger.warning( + "kiro-cli: path lookup exceeded %.0fs (unresponsive home?), skipping %s", + _KIRO_CLI_RESOLVE_TIMEOUT_SECS, + purpose, + ) + return None + if pinned is None and unpinned_exists: + logger.warning( + "kiro-cli resolves only through PATH, which an unattended spawn does " + "not trust, so %s is skipped. Point KIROCREW_KIRO_BIN at the binary " + "to have it used here.", + purpose, + ) + return pinned + + class GatewayOrchestrator: """Manages the lifecycle of all gateway services. @@ -2408,10 +2476,18 @@ async def _warn_if_kiro_cli_outdated(self) -> None: stall every other callback for the 5s budget, and a timeout is logged (not silently swallowed) so a wedged kiro-cli that costs 5s on every boot is diagnosable from gateway.log. + + Pinned the same way the auto-update pins it, via `_pinned_kiro_cli`: + this probe runs unattended at boot, so a shim planted on `PATH` would + execute here regardless of the `--version` argument. A binary the pin + refuses has no version worth warning about — and the pin logs why. """ + kiro_cli_bin = await _pinned_kiro_cli("the kiro-cli version check") + if kiro_cli_bin is None: + return try: proc = await asyncio.create_subprocess_exec( - "kiro-cli", + kiro_cli_bin, "--version", stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, @@ -10035,12 +10111,17 @@ def _obstructions(name: str) -> bool: return logger.info("Auto-update: reset to origin/%s, rebuilding", branch) - # Update the optional kiro-cli backend if present. - if shutil.which("kiro-cli"): + # Update the optional kiro-cli backend, by the pinned absolute path + # `_pinned_kiro_cli` returns — never a bare argv0 this unattended + # path would let `PATH` answer. `None` means do not spawn it, + # skipped like any absent backend, which this step already treats as + # non-fatal. + kiro_cli_bin = await _pinned_kiro_cli("the optional kiro-cli backend update") + if kiro_cli_bin is not None: kiro_update: asyncio.subprocess.Process | None = None try: kiro_update = await asyncio.create_subprocess_exec( - "kiro-cli", + kiro_cli_bin, "update", stdout=asyncio.subprocess.DEVNULL, stderr=asyncio.subprocess.DEVNULL, diff --git a/test/test_slack_gateway.py b/test/test_slack_gateway.py index 56d76e4c086..2ea05c7250d 100644 --- a/test/test_slack_gateway.py +++ b/test/test_slack_gateway.py @@ -3528,7 +3528,8 @@ async def test_kiro_cli_update_timeout_kills_child_and_stays_nonfatal(self): async def _fake_exec(*args, **kwargs): argv = [a for a in args if isinstance(a, str)] - if argv and argv[0] == "kiro-cli": + # The resolved absolute path is argv0 now, not the bare name. + if argv and Path(argv[0]).name == "kiro-cli": proc = AsyncMock() proc.kill = MagicMock() proc.returncode = None @@ -3561,9 +3562,10 @@ async def _fake_kill_and_reap(proc): new_callable=AsyncMock, ) as mock_build: with patch("os.execv", side_effect=OSError("test")): - # Truthy: the optional kiro-cli step runs. + # Resolves: the optional kiro-cli step runs. with patch( - "shutil.which", return_value="/usr/bin/kiro-cli" + "kiro_crew.slack.gateway.resolve_kiro_cli", + return_value="/usr/bin/kiro-cli", ): # The gateway resolves _kill_and_reap # function-locally on every call, so @@ -3582,6 +3584,152 @@ async def _fake_kill_and_reap(proc): mock_build.assert_awaited_once() assert mock_install.call_count == 1 + @pytest.mark.asyncio + async def test_kiro_cli_update_execs_resolved_absolute_path(self): + """The kiro-cli update spawns the RESOLVED path, never a bare argv0. + + A bare `"kiro-cli"` argv0 is re-resolved off the gateway's inherited + `PATH` inside `exec`, and that `PATH` can lead with an agent-writable + directory — so a planted shim would run unattended as the gateway user. + Asserting the absolute path reached `create_subprocess_exec` is what + pins the lookup to the resolver: with the bare name restored, argv0 is + `"kiro-cli"` and this fails. + """ + orch = _make_orchestrator() + ds = _mock_dashboard_state() + orch.dashboard_state = ds + orch.sessions = _mock_sessions() + + _git_fake = _git_exec_fake() + kiro_argvs: list[list[str]] = [] + + async def _fake_exec(*args, **kwargs): + argv = [a for a in args if isinstance(a, str)] + if argv and Path(argv[0]).name == "kiro-cli": + kiro_argvs.append(argv) + proc = AsyncMock() + proc.returncode = 0 + proc.wait = AsyncMock(return_value=0) + proc.communicate = AsyncMock(return_value=(b"", b"")) + return proc + return await _git_fake(*args, **kwargs) + + with patch("kiro_crew.env.is_toolbox_install", return_value=False): + with patch.dict("os.environ", {"KIROCREW_PROJECT_DIR": "/tmp/proj"}): + with patch("asyncio.create_subprocess_exec", side_effect=_fake_exec): + with patch("kiro_crew.dep_sync.sync_or_reinstall", return_value=0): + with patch.object( + GatewayOrchestrator, "_is_brazil_install", return_value=False + ): + with patch( + "kiro_crew.slack.gateway.build_frontend_async", + new_callable=AsyncMock, + ): + with patch("os.execv", side_effect=OSError("test")): + with patch( + "kiro_crew.slack.gateway.resolve_kiro_cli", + return_value="/opt/pinned/bin/kiro-cli", + ): + await orch._auto_apply_update() + + assert kiro_argvs, "the kiro-cli update spawn never happened" + assert kiro_argvs[0][0] == "/opt/pinned/bin/kiro-cli" + + @pytest.mark.asyncio + async def test_kiro_cli_update_resolves_without_inherited_path(self): + """The candidate set excludes the inherited `PATH`. + + Pinning argv0 is not enough on its own: with the inherited `PATH` in + the candidate set, a leading agent-writable directory still gets to + name the binary this unattended path resolves to. Asserting the + keyword is what pins that — the default is `True`, so a call that + forgets it fails here. + """ + orch = _make_orchestrator() + ds = _mock_dashboard_state() + orch.dashboard_state = ds + orch.sessions = _mock_sessions() + + _git_fake = _git_exec_fake() + + async def _fake_exec(*args, **kwargs): + argv = [a for a in args if isinstance(a, str)] + if argv and Path(argv[0]).name == "kiro-cli": + proc = AsyncMock() + proc.returncode = 0 + proc.wait = AsyncMock(return_value=0) + proc.communicate = AsyncMock(return_value=(b"", b"")) + return proc + return await _git_fake(*args, **kwargs) + + with patch("kiro_crew.env.is_toolbox_install", return_value=False): + with patch.dict("os.environ", {"KIROCREW_PROJECT_DIR": "/tmp/proj"}): + with patch("asyncio.create_subprocess_exec", side_effect=_fake_exec): + with patch("kiro_crew.dep_sync.sync_or_reinstall", return_value=0): + with patch.object( + GatewayOrchestrator, "_is_brazil_install", return_value=False + ): + with patch( + "kiro_crew.slack.gateway.build_frontend_async", + new_callable=AsyncMock, + ): + with patch("os.execv", side_effect=OSError("test")): + with patch( + "kiro_crew.slack.gateway.resolve_kiro_cli", + return_value="/opt/pinned/bin/kiro-cli", + ) as mock_resolve: + await orch._auto_apply_update() + + # The pin excludes the inherited PATH; the second call is the probe that + # decides whether a PATH-only install is worth a log line. + assert mock_resolve.call_args_list[0].kwargs == {"include_inherited_path": False} + + @pytest.mark.asyncio + async def test_kiro_cli_update_skipped_when_unresolvable(self): + """An unresolvable kiro-cli is SKIPPED, not exec'd by bare name. + + Fail-closed, matching what the git path does when `trusted_git_bin` + returns `None`. The rest of the update is unaffected: this backend is + optional, so the frontend build and dependency install still run. + """ + orch = _make_orchestrator() + ds = _mock_dashboard_state() + orch.dashboard_state = ds + orch.sessions = _mock_sessions() + + _git_fake = _git_exec_fake() + kiro_argvs: list[list[str]] = [] + + async def _fake_exec(*args, **kwargs): + argv = [a for a in args if isinstance(a, str)] + if argv and Path(argv[0]).name == "kiro-cli": + kiro_argvs.append(argv) + return await _git_fake(*args, **kwargs) + + with patch("kiro_crew.env.is_toolbox_install", return_value=False): + with patch.dict("os.environ", {"KIROCREW_PROJECT_DIR": "/tmp/proj"}): + with patch("asyncio.create_subprocess_exec", side_effect=_fake_exec): + with patch( + "kiro_crew.dep_sync.sync_or_reinstall", return_value=0 + ) as mock_install: + with patch.object( + GatewayOrchestrator, "_is_brazil_install", return_value=False + ): + with patch( + "kiro_crew.slack.gateway.build_frontend_async", + new_callable=AsyncMock, + ) as mock_build: + with patch("os.execv", side_effect=OSError("test")): + with patch( + "kiro_crew.slack.gateway.resolve_kiro_cli", + return_value=None, + ): + await orch._auto_apply_update() + + assert kiro_argvs == [] + mock_build.assert_awaited_once() + assert mock_install.call_count == 1 + # ═══════════════════════════════════════════════════════════════════════════ # Tests: Subagent Slack injection timeout @@ -6027,8 +6175,11 @@ async def _communicate(): proc.kill = MagicMock() proc.communicate = MagicMock(side_effect=_communicate) orch = _make_orchestrator() - with patch("asyncio.create_subprocess_exec", AsyncMock(return_value=proc)): - await orch._warn_if_kiro_cli_outdated() # must not raise + with patch( + "kiro_crew.slack.gateway.resolve_kiro_cli", return_value="/opt/pinned/bin/kiro-cli" + ): + with patch("asyncio.create_subprocess_exec", AsyncMock(return_value=proc)): + await orch._warn_if_kiro_cli_outdated() # must not raise proc.kill.assert_called_once() @pytest.mark.asyncio diff --git a/test/test_slack_gateway_more_coverage.py b/test/test_slack_gateway_more_coverage.py index 1cf1533ded0..f2f0470594b 100644 --- a/test/test_slack_gateway_more_coverage.py +++ b/test/test_slack_gateway_more_coverage.py @@ -136,6 +136,108 @@ def _probe_proc(communicate: Any, *, returncode: int = 0) -> MagicMock: class TestWarnIfKiroCliOutdated: """The boot-time kiro-cli version probe never raises and never hangs.""" + @pytest.fixture(autouse=True) + def _resolvable_kiro_cli(self): + """Every arm below exercises the spawn, which now needs a resolved path. + + The probe resolves kiro-cli from the fixed install directories before + spawning, so without this the arms would take the "not installed" early + return on a host that has no kiro-cli and assert against a spawn that + never happened. The refusal path itself is covered separately by + :meth:`test_unresolvable_binary_never_spawns`. + """ + with patch( + "kiro_crew.slack.gateway.resolve_kiro_cli", return_value="/opt/pinned/bin/kiro-cli" + ): + yield + + @pytest.mark.asyncio + async def test_unresolvable_binary_never_spawns(self, capsys): + """An unresolvable kiro-cli is not spawned by bare name. + + This probe runs unattended at gateway boot, so falling back to a bare + argv0 would let a `PATH`-planted shim execute here — the `--version` + argument is no protection. Nothing to warn about, so nothing runs. + """ + orch = _make_orchestrator() + with patch("kiro_crew.slack.gateway.resolve_kiro_cli", return_value=None): + with patch("asyncio.create_subprocess_exec") as spawn: + await orch._warn_if_kiro_cli_outdated() + spawn.assert_not_called() + assert "outdated" not in capsys.readouterr().out + + @pytest.mark.asyncio + async def test_probe_execs_resolved_absolute_path(self): + """The resolved absolute path is argv0, and `PATH` is out of the lookup.""" + + async def _communicate() -> tuple[bytes, bytes]: + return (b"kiro-cli 9.9.9", b"") + + proc = _probe_proc(_communicate) + orch = _make_orchestrator() + with patch( + "kiro_crew.slack.gateway.resolve_kiro_cli", return_value="/opt/pinned/bin/kiro-cli" + ) as mock_resolve: + with patch("asyncio.create_subprocess_exec", AsyncMock(return_value=proc)) as spawn: + await orch._warn_if_kiro_cli_outdated() + assert spawn.await_args.args[0] == "/opt/pinned/bin/kiro-cli" + mock_resolve.assert_called_once_with(include_inherited_path=False) + + @pytest.mark.asyncio + async def test_path_only_install_is_refused_but_reported(self, caplog): + """A PATH-only install is refused for the spawn AND named in the log. + + Refusing is the point of the pin, but silence about it would leave a + host that never auto-updates and never warns it is outdated with nothing + in `gateway.log` to say why. The line has to name the override, which is + the operator's way out. + """ + orch = _make_orchestrator() + + def _resolve(**kwargs: Any) -> str | None: + # Pinned lookup misses; the unpinned one (PATH included) hits. + return None if kwargs.get("include_inherited_path") is False else "/w/venv/bin/kiro-cli" + + with caplog.at_level("WARNING"): + with patch("kiro_crew.slack.gateway.resolve_kiro_cli", side_effect=_resolve): + with patch("asyncio.create_subprocess_exec") as spawn: + await orch._warn_if_kiro_cli_outdated() + spawn.assert_not_called() + assert "KIROCREW_KIRO_BIN" in caplog.text + + @pytest.mark.asyncio + async def test_absent_backend_is_refused_quietly(self, caplog): + """No kiro-cli anywhere is not a problem to report — the backend is optional.""" + orch = _make_orchestrator() + with caplog.at_level("WARNING"): + with patch("kiro_crew.slack.gateway.resolve_kiro_cli", return_value=None): + with patch("asyncio.create_subprocess_exec") as spawn: + await orch._warn_if_kiro_cli_outdated() + spawn.assert_not_called() + assert "KIROCREW_KIRO_BIN" not in caplog.text + + @pytest.mark.asyncio + async def test_slow_home_directory_cannot_stall_boot(self, caplog): + """A wedged path lookup is bounded, and the refusal keeps boot moving. + + `_init_services` awaits this probe BEFORE `_init_dashboard` binds its + socket, so an unbounded lookup on an unresponsive network-mounted home + would mean the dashboard never comes up at all. + """ + orch = _make_orchestrator() + + def _hang(**kwargs: Any) -> str: + time.sleep(5) # outlives the budget pinned below + return "/opt/pinned/bin/kiro-cli" + + with caplog.at_level("WARNING"): + with patch.object(gw, "_KIRO_CLI_RESOLVE_TIMEOUT_SECS", 0.01): + with patch("kiro_crew.slack.gateway.resolve_kiro_cli", side_effect=_hang): + with patch("asyncio.create_subprocess_exec") as spawn: + await orch._warn_if_kiro_cli_outdated() + spawn.assert_not_called() + assert "exceeded" in caplog.text + @pytest.mark.asyncio async def test_unspawnable_binary_is_silent(self, capsys): orch = _make_orchestrator()