fix(gateway): pin both unattended kiro-cli spawns off PATH

[iamwhatever]

Problem / Motivation

Two spawns in slack/gateway.py exec'd the bare name "kiro-cli", so the lookup that decides which program runs happened inside exec, against the gateway's inherited PATH:

• _auto_apply_update — the unattended auto-update, also what an enterprise min_version floor calls.
• _warn_if_kiro_cli_outdated — the version probe at gateway boot.

The auto-update's guard was shutil.which("kiro-cli"), whose answer was discarded — it proved a binary existed somewhere on PATH and then let exec resolve again, so probe and spawn could land on different files. The boot probe had no guard at all.

Why it matters

HIGH. A gateway's PATH can legitimately lead with an agent-writable directory — platform_compat.trusted_system_bin's own docstring names a worktree venv's bin. Anything able to drop an executable called kiro-cli there gets code execution as the gateway user on a path with no operator present. The --version argument is no protection on the boot probe: a planted shim executes before it ever reads its argv.

The git spawns in _auto_apply_update were already hardened for exactly this reason — trusted_git_bin() for the binary, git_command_env() for the environment. These two kiro-cli spawns were what was left.

What changed (motivation → approach → change)

Bare argv0 re-resolved inside exec → resolution must happen in-process, from a candidate set no agent-writable directory contributes to, and the resolved path must be what runs → both sites call _pinned_kiro_cli, which returns an absolute path or refuses.

kiro_cli_bin = await _pinned_kiro_cli("the kiro-cli version check")
if kiro_cli_bin is None:
    return

The candidate set is the fixed known install directories plus the explicit KIROCREW_KIRO_BIN, with the inherited PATH excluded via a new include_inherited_path keyword on resolve_kiro_cli — forwarded to find_kiro_cli_candidates, which already had it, default True, so the three existing consumers (acp/client.py, cli_doctor.py, review_pool.py) are untouched. The override stays honoured on purpose: it is set by the operator who starts the gateway, not named by a directory an agent can write into.

resolve_kiro_cli rather than trusted_system_bin. The system-tool pin restricts to /usr/bin, /bin, /usr/sbin, /sbin, /run/current-system/sw/bin. kiro-cli is a user-installed backend living in ~/.local/bin or ~/.cargo/bin (kiro_cli.known_kiro_cli_dirs) and is never in those, so that pin would return None on every real install and retire both steps.

Bounded and off the loop. _init_services awaits _warn_if_kiro_cli_outdated strictly before _init_dashboard binds its socket, and the lookup stats directories under the home directory. Unbounded, an unresponsive network-mounted home would mean the dashboard never comes up; the budget refuses the spawn instead, like an absent binary.

Refusal is reported, not silent. The chosen set does not cover every install — a system-wide one outside known_kiro_cli_dirs, a root-owned /usr/local/bin on Linux, is declined here while sessions keep launching it off PATH. Refusing is right for a spawn with no operator present; being quiet about it is not, because that host never auto-updates and never warns it is outdated with nothing in the log to say why. So the pin distinguishes declined an install that exists — logged, naming KIROCREW_KIRO_BIN — from no backend installed, which is not worth a line.

No env= pin. git_command_env() neutralizes git-specific exec vectors (GIT_CONFIG_COUNT/KEY/VALUE) and strips GIT_DIR/GIT_WORK_TREE, which must be absent rather than overwritten. kiro-cli has no analogous fixed-key exec vector and there is no general non-git system-command env helper, so an environment pin here would be a new layer rather than a mirror of the existing one.

Two bare-name kiro-cli spawns remain outside this file: cli_server.py:1371-1375 (the kirocrew update CLI) and diagnostics.py:288 (_kiro_cli_version, reached from POST /api/diagnostics/collect). Both are the same shape and neither is fixed here; tracked in #8495 rather than declared harmless.

Tests

test/test_slack_gateway.py — auto-update path:

• test_kiro_cli_update_execs_resolved_absolute_path — the pinned absolute path is argv0.
• test_kiro_cli_update_resolves_without_inherited_path — the pin excludes the inherited PATH.
• test_kiro_cli_update_skipped_when_unresolvable — a refusal spawns nothing; the frontend build and dependency install still run.
• test_kiro_cli_update_timeout_kills_child_and_stays_nonfatal — updated to the new guard; tree-kill and non-fatal assertions unchanged.

test/test_slack_gateway_more_coverage.py — boot probe:

• test_probe_execs_resolved_absolute_path — pinned absolute path as argv0, PATH excluded.
• test_unresolvable_binary_never_spawns — nothing spawned, nothing warned.
• test_path_only_install_is_refused_but_reported — refused for the spawn, and the log names KIROCREW_KIRO_BIN.
• test_absent_backend_is_refused_quietly — no line for a backend that simply is not installed.
• test_slow_home_directory_cannot_stall_boot — a wedged lookup is bounded and boot moves on.
• The class's existing six arms gain an autouse fixture supplying a resolvable path, since each asserts against a spawn that now requires the pin first.

Revert-verified load-bearing: removing the timeout bound, removing the PATH-only warning, or widening that warning to fire when no backend exists each fails a test; dropping include_inherited_path=False and restoring the bare argv0 each fail two more. 1539 tests pass across the gateway, more-coverage, spawn-audit, governance-update, kiro-prerequisite, ACP-client, doctor and env suites.

Manual verification

N/A — every branch is argv0, guard shape, or a log line at a spawn site, all observable in unit coverage.

Related Issues

Security finding from merged PR #5387 (untrusted PATH resolution → arbitrary code execution).

Pattern harvest

Rule candidate: semgrep
Pattern: shutil.which(X) used as a boolean existence test while X is then passed as a bare argv0 — the resolved path is discarded, the lookup moves into exec, and probe and spawn can disagree, so an attacker-controlled PATH entry decides the payload.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — N/A, the rationale lives on _pinned_kiro_cli and the resolver's docstring
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 408641bc92ca01570d6cd02338fcc1b102a837ce — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound pin at the right seam, but the pinned set is itself same-uid-writable, so this narrows the vector the description claims to close.

Watch

• The description claims "a candidate set no agent-writable directory contributes to," but on POSIX the pinned set leads with ~/.local/bin and ~/.cargo/bin (known_kiro_cli_dirs) — same-uid-writable wherever the agent runs as the gateway user, and covered by no sensitive-path deny rule. An agent that plants ~/.local/bin/kiro-cli still gets executed unattended by both fixed spawns (and it shadows a legitimate ~/.cargo/bin install, since the override is the only earlier candidate). The residual is inherent to a user-installed backend and pre-exists on the session path, so the design is still the right one — but the "pinned = closed" framing will propagate to the Pin the two remaining bare-name kiro-cli spawns off PATH #8495 follow-up sites and the pattern-harvest rule; state the residual explicitly there so nobody treats include_inherited_path=False as a complete answer.

[DESIGN-REVIEWED] 408641b

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 408641bc92ca01570d6cd02338fcc1b102a837ce — this comment is updated in place on each push.

Review details

I've verified the code. The candidate's evidence matches the diff and the code exists as described.

Assessing CANDIDATE 1 against the survival bar:

• The claimed outcome (default ThreadPoolExecutor exhaustion stalling other asyncio.to_thread users) is not an observable failure that occurs in practice from the changed lines — it requires a home whose stat calls hang indefinitely (a hard-mounted, permanently-wedged network home), and many repeated auto-update cycles to consume all default-executor workers, and the absence of any other cap. Every link is "could accumulate over enough cycles." Its own (c) resolves to "might," which the instructions require me to drop.
• Boot is protected (the caller returns after the 5s budget), which the candidate concedes.
• The thread-abandonment behavior is inherent to asyncio.to_thread+wait_for — a codebase-wide pattern, not something these changed lines introduce as a reachable defect. Confidence well below 80.

It dies under falsification.

Step 2: the change is a focused, well-guarded security hardening. Only the PATH-excluded pinned value is ever exec'd, both spawns are non-fatal, the timeout is bounded off the loop, the known-install-dir + KIROCREW_KIRO_BIN candidate set is intact, and the refuse-but-report logic is sound and tested. I opened kiro_cli.py and the gateway hunks and found no grounded (a)/(b)/(c) defect in the changed lines.

No findings.

[OPUS-REVIEWED] 408641b

Verdict parsed from the review's SHA-scoped output markers for commit 408641bc92ca01570d6cd02338fcc1b102a837ce.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 408641bc92ca01570d6cd02338fcc1b102a837ce: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

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

All claims verified: siblings at cli_server.py:1371 and diagnostics.py:288 exist and are declared; acp/client.py spawns via resolve_kiro_cli (its KIRO_CLI_BIN literal is label-only); the boot ordering behind the timeout is real (gateway.py:2557 awaits the probe before _init_dashboard). Final review:

First-Principles-Verdict: PASS

Both unattended spawns now exec a path resolved in-process from operator-controlled directories; every rider is declared, counted, and traced to the PR #5387 finding.

What this change ships

Intent: stop the gateway's two unattended kiro-cli spawns from letting an agent-writable PATH entry choose what runs — a FIX.

1. Auto-update runs kiro-cli by pinned absolute path, never bare name — justified
2. Boot version probe runs by pinned absolute path — justified
3. A PATH-only install is now refused (skipped) at both steps — declared, changed behavior
4. New warning naming KIROCREW_KIRO_BIN when an install is declined; silence when none exists — declared
5. Path lookup bounded at 5s so a wedged home can't block dashboard boot — declared, derived (probe awaited at gateway.py:2557 before _init_dashboard)
6. include_inherited_path keyword on resolve_kiro_cli — 1 consumer (gateway.py:1484), but it forwards a keyword find_kiro_cli_candidates already had; smallest form
7. Two same-shape spawns left unfixed (cli_server.py:1371, diagnostics.py:288) — declared, deferred to Pin the two remaining bare-name kiro-cli spawns off PATH #8495

Counts run: resolve_kiro_cli consumers = 4 prior call sites, all untouched by the default-True keyword; bare-name kiro-cli spawns remaining = exactly the 2 the description names. trusted_system_bin was correctly rejected: known_kiro_cli_dirs is ~/.local/bin / ~/.cargo/bin, outside its system-dir pin.

Watch

Root-cause boundary is drawn at this file, not at "unattended": session spawns resolve in-process (acp/client.py:347) so probe and spawn agree, but their candidate set still includes inherited PATH, and cron-driven sessions also run with no operator present. The docstring acknowledges this ("sessions keep launching it off PATH") without counting it against #8495.

[FIRST-PRINCIPLES-REVIEWED] 408641b

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @iamwhatever overrides the GPT 5.6 finding for 408641bc92ca01570d6cd02338fcc1b102a837ce; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 408641bc92ca01570d6cd02338fcc1b102a837ce: <one-sentence reason>

[iamwhatever]

/ai-review override gpt 408641bc9edd06f1b0e0e8e2b78a12df8fca5c9d: kiro-cli's documented install directory is where install.sh also symlinks the gateway's own kirocrew launcher, so an agent that can plant a binary there can already replace the entry point the gateway runs from — refusing that directory buys nothing, and the requested process-start executable attestation has no mechanism in this codebase.

[github-actions]

AI-review override not recorded: 408641bc9edd06f1b0e0e8e2b78a12df8fca5c9d is not the current PR head. Re-run the command with 408641bc92ca01570d6cd02338fcc1b102a837ce.

[iamwhatever]

[operator: iamwhatever] Dispositions for the round on 24deff5f5, all addressed in 408641bc9.

GPT F2 — new awaited work before the dashboard binds — fixed. Legitimate and self-inflicted by the previous round's to_thread. _init_services awaits _warn_if_kiro_cli_outdated strictly before _init_dashboard binds its socket, so an unbounded home-directory walk meant the gateway could never come up. The pin is now bounded by _KIRO_CLI_RESOLVE_TIMEOUT_SECS and refuses the spawn on overrun. Pinned by test_slow_home_directory_cannot_stall_boot, revert-verified.

GPT F3 — docstring contradicted the retained KIROCREW_KIRO_BIN — fixed. Correct: "no environment-supplied directory" was false while the override is deliberately honoured. The docstring now says the inherited PATH is excluded and states why the override survives — the operator sets it, an agent cannot name it.

GPT F1 — unattended spawns still trust agent-writable install directories — rebutted, override posted. install.sh:528 symlinks the gateway's own kirocrew launcher into that same directory, so an agent able to plant kiro-cli there can already replace the entry point the gateway runs from; refusing the documented install directory buys nothing while that is true. The requested process-start executable attestation has no mechanism in this codebase, and building one is a subsystem, not this PR's fix.

Design Review — silent refusal leaves an undiagnosable drift state — fixed. The concern held exactly as described: a host whose only kiro-cli is PATH-findable keeps launching sessions while never auto-updating and never warning it is outdated. The pin now separates "declined an install that exists" — logged, naming the override — from "no backend installed", which stays quiet because the backend is optional. Both branches pinned and revert-verified.

Design Review + First Principles — diagnostics.py:288 and cli_server.py:1371 — accepted-and-deferred (#8495). Design Review is right that diagnostics.py runs from POST /api/diagnostics/collect, a dashboard click, so the "operator at a TTY" framing was wrong for it and the PR body no longer claims it. Deferred rather than folded in: they are a third and fourth file, cli_server.py is sync so it needs a different call shape, and this PR has already grown twice under review. #8495 carries the deferred-finding label, an owner and a due date.

First Principles — the chosen set retires a root-owned /usr/local/bin install — fixed as to the real harm. The overstatement is gone from the rationale, which now says plainly that the pin does not cover every install and names that case. Preferring trusted_system_bin first would not have closed it — /usr/local/bin is deliberately outside the trusted set — and would emit a misleading "treated as unavailable" warning on every standard install, where the fallback in fact succeeds. What the case actually needed was to stop being silent, which the new log line does.

[iamwhatever]

/ai-review override gpt 408641b: kiro-cli's documented install directory is where install.sh also symlinks the gateway's own launcher, so an agent that can plant a binary there can already replace the entry point the gateway runs from, and the requested agent-writability rejection would refuse every standard install while buying nothing.

[operator: iamwhatever] Re-posting this override: the earlier one named a SHA I got wrong (408641bc9edd… — the tail was not read from the ref), so it never matched this head and was never consumed. The correct head is 408641bc92ca01570d6cd02338fcc1b102a837ce. No code change accompanies it; the reasoning is unchanged from the disposition comment above.

[github-actions]

Human judgment recorded

@iamwhatever marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 408641bc92ca01570d6cd02338fcc1b102a837ce.

kiro-cli's documented install directory is where install.sh also symlinks the gateway's own launcher, so an agent that can plant a binary there can already replace the entry point the gateway runs from, and the requested agent-writability rejection would refuse every standard install while buying nothing.

This decision applies only to this commit. A new push requires a new judgment.

[bolichen97]

LGTM.