fix(sandbox): delegate Windows Kiro spawns internally (#5620)

[bolichen97]

Brings #5620 (source squash c7a68282f) to release/0.4.0 so the insider line stops requiring sandbox_allow_unsandboxed_exec before a Windows user's first chat.

This is NOT a byte-identical cherry-pick, and it is not meant to read as one. A plain git cherry-pick -x produced 14 conflicts, 5 of them files that do not exist on this branch at all. Every conflict was resolved by hand and by judgment; every place the result differs from the source commit is listed below. Reviewers should read this as a hand-resolved port, not a replay.

The product fix itself IS a faithful replay. The nine src/ files carrying the behaviour change — sandbox.py, acp/client.py, acp/runtime.py, pod/runtime.py, dashboard/handlers/{sessions,agents}.py, apps/backend.py, weixin/attachments.py, testing/fixtures.py — have diffs byte-identical to the source commit (every difference is a @@ hunk offset). All the hand work is in the test / docs / CI periphery.

What it does

Windows has no Kiro Crew OS sandbox backend, so wrap_argv previously fail-closed and refused agent subprocesses unless the operator set agent.sandbox_allow_unsandboxed_exec. A spawn positively classified as the first-party Kiro backend now delegates isolation to kiro-cli's own internal sandbox, with the parent applying the full env scrub (scrub_agent_subprocess_env) because Windows has no POSIX env -u launcher. It stays audit-or-deny: an unwritable SEL event returns None and the caller falls through to normal policy, which still fail-closes on Windows. Scripts, hooks and third-party ACP backends keep the old opt-in requirement.

Why the conflicts, and how each was decided

Three CI/test mechanisms #5620 touches are wholly absent from this branch — not just the file, but the gate script, the ci.yml wiring and the tests. Their edits are bookkeeping for gates that cannot run here, so they were dropped:

Dropped	Introduced on main by	What #5620 changed in it
.github/subprocess-encoding-baseline.txt	#5378	removes 4 baseline entries, decrements one count
scripts/run_scoped_tests.py	#5262	"python3"/"python" literals → sys.executable
scripts/check_lockdown_before_publish.py	#5348	a comment-only edit
test/test_issue_radar_crew_fabric.py	—	file absent here
test/test_loop_lock.py	—	file absent here

The code-side encoding pins that baseline records are kept — the ratchet ledger is meaningless without its ratchet, but the pins are real behaviour.

The other conflicts are the 3-way merge presenting whole foreign test bodies as "incoming", because #5620's edits land inside content this branch lacks. Each was dropped after verifying the production symbols it exercises are absent here, so importing it would have shipped guaranteed-failing tests:

• test/test_agent.py — fix(agent): make a stale spec model pin diagnosable and resettable #4911's agent_spec_path traversal/symlink/ambiguity suite (~390 lines). agent_spec_path, _conflicting_spec_for and reset_agent_model are all absent from this branch's src/kiro_crew/agent.py. fix(sandbox): delegate Windows Kiro spawns internally #5620's own 10 @requires_symlinks decorators, its conftest import and its Windows entry["args"][-len(args):] assertion all landed and are kept.
• test/test_cli_doctor.py — TestEffectiveModelSection + TestWhatsAppSection; _doctor_effective_model, _doctor_whatsapp and project_agent_files are all absent from this branch's cli_doctor.py. fix(sandbox): delegate Windows Kiro spawns internally #5620's only other change to this file was the requires_symlinks import, whose sole consumer was inside the dropped block, so keeping it would have been an unused import (flake8 F401). Net: no change to this file.
• test/test_script_hooks.py — TestLastError is absent here. fix(sandbox): delegate Windows Kiro spawns internally #5620's other edit in this file (test_timeout → _script_command, removing the dependency on a sleep binary) landed and is kept; _script_command already exists on the branch.
• test/test_mcp_gateway_rewriter.py — test_overlay_lockdown_precedes_content is absent here. _spec_with_env, which it shares, already exists and is untouched.
• website/electron/test/auto-update.test.js — the whole externally-managed-marker suite and the install/check-phase suite are absent (readExternallyManaged, onInstallFailed, check-in-flight: zero hits on this branch, in the module as well as the test). fix(sandbox): delegate Windows Kiro spawns internally #5620's substantive edit here (a try/catch around a symlink assertion for Windows hosts without SeCreateSymbolicLinkPrivilege) sits inside the dropped block, and its other edit — adding a trailing newline at EOF — is already satisfied on this branch. Net: no change to this file.

One conflict was a genuine semantic fork rather than absent content:

• test/test_mcp_gateway_rewriter.py asserts which path the rewriter locks down. Main asserts p.suffix == ".tmp"; this branch asserts .json, and .json is the correct answer here: at rewriter.py:1486-1494 this branch does atomic_write(target, ...) then restrict_to_owner(target), locking the FINAL name. The temp-first ordering that makes .tmp true is not on this branch, so taking main's line would have made assert overlay_specs fail. HEAD's assertion is kept, reflowed to one line because the source commit's own baseline hunk prunes this file from .github/black-baseline.txt (it graduates, so it must be fully black-clean).

Two docs paragraphs needed trimming rather than wholesale acceptance, because the incoming text asserts things that are false on this branch:

• docs/system-specs/modules/harness-parity.md — the H7 row is taken verbatim. It documents exactly the behaviour being shipped, both tests it cites (test_harness_parity.py::test_is_kiro_cli_is_positive, test_sandbox_argv.py::TestKiroInternalSandboxExclusion) exist here, and the KAS backend it references exists in acp/{runtime,client,types}.py.
• docs/system-specs/modules/security.md — the parent-scrub description is taken, three claims are not. _PYTHON_ENV_PREFIXES here is PYTHONPATH + PYTHONHOME, so PYTHONPYCACHEPREFIX is dropped from the enumeration; pycache_gc.prune_pycache does not exist on this branch, so that sentence is dropped; and gatewayd.py here pops a hand-listed PYTHONPATH/PYTHONHOME pair (gatewayd.py:1305-1306) rather than the shared list, so HEAD's weaker and true sentence is kept over main's stronger and false one.
• docs/guides/windows-install.md — the updated "Core gateway / chat / dashboard" row is taken. The adjacent "Project skills" row in the same hunk is unchanged context on main (no +), not part of fix(sandbox): delegate Windows Kiro spawns internally #5620, and does not exist here, so it is not introduced.

Deviations that ADD content not in the source commit

Four, each a dependency the source commit did not need because main already had it. Without these the port is broken, and two of them were caught by the gates rather than by reading:

1. import atexit in conftest.py — the incoming _drain_windows_proactor_finalizers calls atexit.register. Main's conftest imported atexit before fix(sandbox): delegate Windows Kiro spawns internally #5620; this branch does not, so the function would NameError.
2. The drain call site in conftest.py — only the definition was inside the conflict; the call did not land. It is placed before the hasattr(session.config, "workerinput") early return, matching main, because that return fires on exactly the xdist workers whose exit code the drain protects — placing it after would make it a no-op for the case it exists to fix. This is load-bearing for this commit: fix(sandbox): delegate Windows Kiro spawns internally #5620 adds an asyncio.run to test_session_usage.py, which is the pattern that leaves ProactorEventLoop finalizers behind on Windows. Main's call-site comment claims the fix "ensure[s] a FRESH open event loop is set as current"; the implementation does no such thing (it installs an unraisablehook and forces GC), so that clause is not copied.
3. src/kiro_crew/subprocess_utf8.py — flake8 F821 and mypy both flagged UTF8_TEXT undefined at cli_server.py:1677. The module does not exist here, and apps/backend.py:45 had also landed an import of it, which would have been an ImportError on app-backend startup. Brought over from main; the docstring's claim that the mapping is "enforced by scripts/check_subprocess_encoding.py in CI" is corrected, since that gate is not on this branch.
4. The UTF8_TEXT import in cli_server.py — the cherry-pick brought the use without the import, because main's import predates fix(sandbox): delegate Windows Kiro spawns internally #5620.

Deviation: two tests re-pinned to this branch's contract

PYTHONPYCACHEPREFIX is main's third _PYTHON_ENV_PREFIXES key and is not on this branch, so two of #5620's tests asserted a scrub that does not happen here:

• test/test_session_usage.py::TestFetchWhoami::test_spawn_uses_full_agent_environment_scrub
• test/test_acp_runtime.py::test_runtime_spawn_scrubs_sensitive_env_on_default_auto

Both now use PYTHONPATH as the _PYTHON_ENV_PREFIXES representative. That keeps each test's discriminating power intact — it covers one key per scrub source (_SENSITIVE_ENV_PREFIXES, _PYTHON_ENV_PREFIXES, _AGENT_DENIED_ENV_KEYS), which is precisely what scrub_agent_subprocess_env unifies — and PYTHONPATH is a real discriminator because it is deliberately EXCLUDED from _SENSITIVE_ENV_PREFIXES, so only the extra-prefixes path removes it. Mutation-verified: with sessions.py:616 reverted to a plain scrub_env() the assertion fails on the PYTHONPATH line, and passes again once restored. Adding the third key was rejected deliberately — on main it arrives paired with the pycache_gc TTL/size bounding this branch lacks, so it is a behaviour decision for a maintainer, not something to smuggle into a port. See the finding below.

Tests

• Every backend test file in the diff (52 files): 4123 passed, 6 skipped. The only 2 failures are test_connections_mint.py::test_the_handlers_package_does_not_import_the_mint_engine and ::test_row_tokens_do_not_repeat_across_a_gateway_restart, both reproduced identically on a clean origin/release/0.4.0 worktree with this commit absent — pre-existing on the branch.
• Directly affected: test_sandbox_argv.py + test_harness_parity.py + test_spawn_audit.py → 182 passed, and after the final reflow test_mcp_gateway_rewriter.py + test_acp_runtime.py + test_session_usage.py alongside them → 536 passed.
• Frontend: tsc -b clean; the 4 affected test files → 108 passed.
• Local CI gates, all green: check_black_formatting.py, isort --check-only, flake8, mypy src/kiro_crew/ (1014 files), check_brand_name.py, check_harness_parity.py.
• Audited for dangling state: no conflict markers anywhere, and no reference to any of the 5 dropped files remains in the tree.

Finding worth a follow-up (not fixed here)

This branch's Electron app exports PYTHONPYCACHEPREFIX into the gateway environment at website/electron/main.js:992, and _PYTHON_ENV_PREFIXES does not include it. So on the packaged desktop app, an agent child on release/0.4.0 inherits it and mirrors its whole stdlib/site-packages bytecode under the crew home, with no pycache_gc bound to prune it. That is a real pre-existing gap on this branch — exactly what main's third key closes — and it is the reason the two tests above had to be re-pinned rather than satisfied. Worth deciding on separately for the insider line.

[github-actions]

UX Review (Fable 5) — ✅ PASS

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

UX-Verdict: PASS

Windows users get first chat with zero config; the only user-facing surfaces are docs and log copy, and both read clearly.

Suggestions

• The consolidated delegation warning in sandbox.py ("SECURITY: delegating this %s kiro-cli spawn…") dropped the old message's remedy — the settings path (_KIRO_INTERNAL_SETTINGS_PATH) and the {"sandbox": false} opt-out that told a macOS operator how to keep Kiro Crew's seatbelt instead. Restore the "to use Kiro Crew's sandbox instead, set…" clause on the macOS branch so the once-per-process warning still names the action.

[UX-REVIEWED] 20fab83

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 20fab83f71a9ab558705897b9cbba38d94cc4413 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 20fab83

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A faithful, well-audited port: the nine-file product diff replays merged #5620 byte-identically, and every hand-resolved deviation is disclosed, justified, and test-verified.

Suggestions

• The disclosed PYTHONPYCACHEPREFIX gap (Electron exports it at main.js:992; this branch's scrub and pycache_gc don't exist to bound it) should get a tracked issue before 0.4.0 stable, so the deliberate test re-pin doesn't silently become the permanent contract.

[DESIGN-REVIEWED] 20fab83

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 20fab83f71a9ab558705897b9cbba38d94cc4413 — 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.

Both data files and the surrounding repo are read; the mechanical counts I ran are cited inline. Final review:

First-Principles-Verdict: PASS

A hand-resolved port of a merged, defect-driven fix, with every conflict decision named, every deviation a counted dependency, and every kept boundary intact.

What this change ships

Intent: let a fresh Windows install chat without first granting sandbox_allow_unsandboxed_exec — a FIX (port of merged #5620 to release/0.4.0).

1. Fresh Windows install chats with no config edit — Kiro spawns delegate to kiro-cli's own sandbox — justified (reported defect; platform has no Crew backend)
2. Windows model picker, account identity and usage readouts work by the same delegation — justified (same cause, three one-shot sites)
3. Every ACP child env parent-scrubbed (sensitive + Python vars) on all platforms — justified (Windows has no env -u; 5 counted consumers of scrub_agent_subprocess_env)
4. Scripts, hooks, Papyrus, third-party backends keep the fail-closed opt-in — justified (H7 boundary preserved, documented invariant)
5. macOS SEL-failure fallback moved to caller fallthrough, same seatbelt outcome — justified (Windows needs the None branch)
6. Pod ports derived in-process, no cksum spawn — justified (binary absent on Windows; vectors pinned)
7. Symlink-requiring tests skip on unprivileged Windows hosts (probe + 106-entry list + decorators) — justified (SeCreateSymbolicLinkPrivilege is a real OS rule; the dual mechanism is main's shape, not relitigable)
8. WeCom CSV classified text/csv on Windows — justified (registry-backed mimetypes divergence)
9. ✓→OK in gate output; mypy pinned to linux typeshed — justified (cp1252 console, host-independent gate)
10. journalctl decode pinned UTF-8 in cli_server.py — inert on this branch (Linux-only spawn, UTF-8 locale); not among the nine declared byte-identical src files

List capped at 10; the proactor-loop test hygiene, the _vendor eol=lf pin, and the jscpd dist-dev ignore are further items, each with a named Windows harm.

Watch

• The description says nine src/ files carry the behaviour change, but cli_server.py is an eleventh src file touched (with subprocess_utf8.py); the intent file truncates inside deviation 3, exactly where it would be declared. Confirm the hunk exists in source c7a68282f; if not, delete it — its zero option costs nobody (journalctl never runs where the locale isn't UTF-8).
• subprocess_utf8.py ships as convention only — its enforcing gate is deliberately absent here (2 consumers: apps/backend.py:318, cli_server.py:1678; grepped UTF8_TEXT under src/). Acceptable as a port dependency; do not grow call sites on this branch expecting CI to check them.

[FIRST-PRINCIPLES-REVIEWED] 20fab83

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 20fab83f71a9ab558705897b9cbba38d94cc4413 — this comment is updated in place on each push.

Review details

I've now traced both candidates and the substantive production changes (the wrap_argv delegation control flow, the scrub_agent_denied_env → scrub_agent_subprocess_env switch, _delegate_to_kiro_internal_sandbox's new None return, and the launcher's env-scrub behavior).

Candidate 1 (mode="off" Windows spawn skips audited delegation): The mode == "off" block (sandbox.py:3299–3349) is not touched by this diff and its Windows behavior (loud-warn unconfined, deferring to kiro's internal sandbox) is unchanged from before. The child's isolation is identical whether it takes the delegated audit path or the "off" warned path — the candidate itself concedes "not an isolation hole." There is no observable wrong outcome at execution time; it is a doc-wording nuance about an untouched code path. Fails (c). Dropped.

Candidate 2 (_resolve_ssh_auth_sock then stripped): The launcher strips SSH_AUTH_SOCK (in _SENSITIVE_ENV_PREFIXES) at every tier anyway, so the sequence is redundant work, not a behavioral defect. The candidate itself classifies it as "efficiency/dead-work" — an explicitly rejected category. Dropped.

Step 2: I checked whether the env-scrub widening introduces a regression. On standard/strict tiers the namespace/seatbelt launchers already stripped _SENSITIVE_ENV_PREFIXES + _PYTHON_ENV_PREFIXES in the child, and the macOS delegated path applies the same env -u scrub — so the sandboxed child's environment is unchanged. The only newly-stripped paths are Linux mode="off" and Windows delegation, where the change removes credential exposure (a tightening the docs frame as intended), never adds a hole, crash, or data loss. The wrap_argv delegate control flow fail-closes correctly on Windows when _delegate_to_kiro_internal_sandbox returns None (audit failure) and preserves macOS's seatbelt fallback. KIRO_API_KEY survives the scrub. No grounded 80+ defect found.

No findings.

[OPUS-REVIEWED] 20fab83

Verdict parsed from the review's SHA-scoped output markers for commit 20fab83f71a9ab558705897b9cbba38d94cc4413.

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