close
Skip to content

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

Merged
iamwhatever merged 1 commit into
mainfrom
codex/fix-windows-kiro-sandbox-default
Aug 24, 2026
Merged

fix(sandbox): delegate Windows Kiro spawns internally#5620
iamwhatever merged 1 commit into
mainfrom
codex/fix-windows-kiro-sandbox-default

Conversation

@bolichen97

@bolichen97 bolichen97 commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

A fresh KiroCrew Nightly install on Windows cannot start its first official Kiro CLI agent. The backend requires an outer OS sandbox before spawning the CLI, but KiroCrew has no supported Windows sandbox backend, so every launch fails with SandboxUnavailableError before Kiro CLI can activate its own sandbox.

The same clean Windows validation run also exposed host-dependent test and gate failures that prevented the repository's own blocking checks from being run reliably on a supported desktop platform.

Why it matters

This is a first-run outage for new Windows users: chat, auto-title, and every ACP session fail at process launch. Asking each new user to opt into globally unsandboxed execution is both poor onboarding and broader than necessary.

A Windows fix also needs trustworthy Windows verification. Leaving unrelated local gate failures in place would make regressions indistinguishable from harness noise and keep contributors from reproducing CI locally.

What changed (motivation → approach → change)

Kiro CLI already owns and enables the agent sandbox, so the backend now delegates isolation to that inner sandbox on Windows only when the spawn path has explicitly identified the executable as the official Kiro CLI. The identity is propagated through ACP, pod, dashboard, and CLI spawn paths rather than inferred from a basename.

Before delegation, KiroCrew still scrubs the child environment and completes the synchronous SEL spawn audit. Audit failures deny the spawn. Unknown identity, non-Kiro executables, and requests for additional visible/hidden path policy continue to fail closed when no outer OS sandbox exists. There is intentionally no version floor or best-effort capability probe that could recreate the fresh-install outage.

The clean-host failures found while proving the Windows path are fixed in the same commit: real-symlink capability inventory, event-loop/port and subprocess portability, Git Bash/path separator handling, vendor-manifest and checksum portability, deterministic frontend probes, Linux-targeted mypy configuration, paid-down lockdown debt, and bounded timeouts for genuinely disk-heavy Windows tests. Two production-facing portability fixes are also explicit: ACP secret/Python environment scrubbing is now enforced parent-side on every platform (including sandbox="off"; that mode disables the OS wrapper, not credential hygiene), and CSV attachments are normalized to text/csv because Windows registry-backed MIME lookup otherwise reports an Excel MIME.

Tests

  • Full backend scoped suite on Windows: 61,032 passed, 2,520 skipped, 6 xfailed.
  • Fresh-base Windows delta after rebase, including the newly enabled Code Review Sage suite: 913 passed, 70 skipped.
  • Sandbox regression coverage verifies official-Kiro delegation, raw argv preservation, environment scrubbing and audit ordering, plus fail-close behavior for unknown/non-Kiro identity, policy paths, and audit errors.
  • Website focused regression: hljsCoreOnly.test.ts — 4 passed.
  • Latest-base frontend approval rollback regression: 2 passed.
  • Electron suite: 1,281 passed, 2 skipped.
  • Production build, TypeScript, ESLint, i18n source/render checks, jscpd, bundle budget, Black, isort, flake8, mypy, lockdown, vendor manifest, docs, brand, harness parity, loop lock, changelog, focus-cue, scrub, CloudFormation, and subprocess-encoding gates passed.
  • GPT and Opus-compatible local review mirrors reported no findings for exact SHA c71e9b953db438cbe58ed201df49ca05c0863ac0.

Manual verification

N/A — a release-packaged Nightly was not rebuilt locally. The Windows-native spawn boundary and every retained fail-close branch are covered directly, and the full backend plus platform-specific gate matrix passed on the affected host.

Screenshots / video

N/A — frontend changes are test/gate portability only; no rendered UI, interaction, or layout changed.

Related Issues

no linked issue: reported from a fresh Windows Nightly installation; no public issue was provided.

Checklist

  • Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@bolichen97
bolichen97 requested a review from a team August 24, 2026 14:30
@bolichen97
bolichen97 requested a review from a team as a code owner August 24, 2026 14:30
@bolichen97
bolichen97 requested a review from cixuuz August 24, 2026 14:30
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 24, 2026
@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @bolichen97 overrides the GPT 5.6 finding for c71e9b953db438cbe58ed201df49ca05c0863ac0; 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 c71e9b953db438cbe58ed201df49ca05c0863ac0: <one-sentence reason>

@github-actions

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

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

All evidence confirms this PR is backend sandbox delegation plus test/gate portability: the website/ changes are test files and a lint-config ignore only, no rendered UI, no new dashboard strings, no screenshots. The one new user-facing string (PodError("pods require \systemctl --user`...")`) states what's missing and where in plain terms. The change itself removes a first-run failure for Windows users, and the notable log/error copy edits stay operator-facing and accurate to the state the code holds.

UX-Verdict: PASS

No rendered UI changes; the diff removes a Windows first-run outage and its new failure text names the missing dependency plainly.

[UX-REVIEWED] c71e9b9

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 24, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

/ai-review override gpt c71e9b9: Official Kiro CLI is the explicitly trusted inner-sandbox boundary on Windows across supported releases; adding a version or capability gate would recreate the fresh-install outage this fix addresses, while unknown or non-Kiro spawns and audit or path-policy failures still fail closed.

@github-actions

Copy link
Copy Markdown
Contributor

Human judgment recorded

@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for c71e9b953db438cbe58ed201df49ca05c0863ac0.

Official Kiro CLI is the explicitly trusted inner-sandbox boundary on Windows across supported releases; adding a version or capability gate would recreate the fresh-install outage this fix addresses, while unknown or non-Kiro spawns and audit or path-policy failures still fail closed.

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

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

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

First-Principles-Verdict: CONCERNS

The Windows fix is cause-level and fail-closed, but a cross-platform env-scrub tightening and a WeChat MIME change ride along undeclared, and one helper ships twice.

What this change ships

Intent: let a fresh Windows install run its first official Kiro CLI chat without opting into globally unsandboxed execution — a FIX.

  1. Fresh Windows install chats/lists models/reads identity+usage with no config edit — justified (reported first-run outage; positive is_kiro_cli is True gate, H7 updated same commit)
  2. Unknown/non-Kiro/extra-path/audit-failed Windows spawns still refuse — justified
  3. ACP children on every platform now lose AWS-secret/SSH/Python env vars in the parent, even with agent.sandbox="off" — rides along, framed as Windows-only
  4. Pod ports derived in-process; cksum spawn deleted — justified (deletes a subprocess + a BENIGN_SPAWNS entry)
  5. WeChat CSV attachments now always classified text/csv — undeclared production change
  6. Symlink-requiring tests skip via exact capability inventory — justified (probe, not OS guess)
  7. Windows test runs no longer exhaust the ephemeral-port range — justified (named OS limit)
  8. Tests locate Git Bash/OpenSSL instead of the WSL stub — duplicate of conftest._find_posix_test_shell
  9. mypy pinned platform="linux"; gate scripts print ASCII — justified (AGENTS.md names this as the parity invocation)
  10. Security/acp-client/harness-parity/windows-install specs updated — mandated (same-commit spec rule)

Watch

  • Item 3: the description says "Before delegation, KiroCrew still scrubs the child environment" (Windows framing), but the diff deletes security.md's documented exception — "mode "off" means genuinely unconfined, environment included" — and applies scrub_agent_subprocess_env() unconditionally in both ACP spawn paths on all platforms. A tightening, but it changes a documented POSIX posture under a Windows fix: title; a human should confirm it is intended for "off" on Linux.
  • Item 5: _PORTABLE_FILE_MIMES in weixin/attachments.py changes channel behavior on POSIX too (any host whose mimetypes table disagrees) and appears nowhere in the description.

Subtractions

  • Delete _bash() in test/test_ai_review_workflows.py — it is a second spelling of test/conftest.py::_find_posix_test_shell (grep Git.*bin.*bash: 2 implementations; test_pod_e2e_video_guard.py already imports the conftest one, proving the import path works).

[FIRST-PRINCIPLES-REVIEWED] c71e9b9

@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Windows delegation trusts an inner sandbox it never verifies — the macOS path is config-confirmed, the Windows path is asserted by classification alone.

Watch

  • On macOS, delegation fires only when kiro_internal_sandbox_enabled() confirms the inner sandbox is on; the Windows branch (sys.platform == "win32" and is_kiro_cli is True → return list(argv), None) never consults that toggle. An operator who sets "sandbox": false in kiro-cli's settings gets a fully unconfined agent with a SEL record claiming "kiro internal sandbox owns this spawn" — an audit trail that asserts an isolation nobody is providing. Skipping the capability probe to avoid re-bricking fresh installs is defensible; delegating against an explicit disable is a different case and deserves a decision, not inheritance.
  • scrub_agent_subprocess_env replaces scrub_agent_denied_env on every platform, so agent.sandbox="off" — previously documented as "genuinely unconfined, environment included" — now strips AWS/SSH/GPG vars on POSIX too. The spec was updated, but this cross-platform behavior change for existing "off" operators is invisible in a PR framed as Windows-only.

Suggestions

  • Read the kiro settings file on Windows and fail closed (standard opt-in path) only when it explicitly disables the inner sandbox; absent/unreadable file keeps the current delegate-by-default, preserving the fresh-install fix.

[DESIGN-REVIEWED] c71e9b9

@bolichen97

Copy link
Copy Markdown
Collaborator Author

Review dispositions for c71e9b953db438cbe58ed201df49ca05c0863ac0:

First Principles:

  • accepted-and-documented — cross-platform ACP env scrub: this is intentional. sandbox="off" disables the outer OS wrapper; it does not opt agent subprocesses back into inheriting gateway credentials, AWS/SSH/GPG pointers, or host Python injection. The same-commit security spec and the PR body now state that invariant explicitly.
  • accepted-and-documented — WeChat CSV MIME normalization: this was a real clean-Windows failure: the registry-backed MIME table returns an Excel MIME for .csv, routing a text attachment differently by host. The PR body now declares the production-facing normalization.
  • accepted-and-deferred — duplicate _bash() test helper: consolidating two private test helpers has no behavior, security, or reliability consequence for this fix. Importing a private conftest helper would also create a new test-internal coupling, so this cleanup is intentionally left out of the final SHA.

Design:

  • rebutted — make the parent read Kiro settings and fail closed on an explicit inner-sandbox disable: official Kiro CLI is the positively identified owner of the Windows isolation boundary and of its own settings semantics. Having the parent reinterpret child configuration creates a second, divergent policy engine and contradicts the confirmed product requirement not to reintroduce Windows fail-close. Unknown/non-Kiro identity, failed audit, and extra path-policy requests still fail closed; a user deliberately changing Kiro CLI's own sandbox setting remains Kiro CLI's explicit choice.
  • accepted-and-documented — sandbox="off" env semantics: as above, the scrub is deliberate credential hygiene rather than an OS-isolation claim, and is now explicit in the PR body.

No code change or new SHA is required for these advisory concerns.

@github-actions

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I have enough to complete the validation. Let me verify the one remaining factual pivot — whether the default-tier behavior actually changed, by confirming the launcher already strips _SENSITIVE_ENV_PREFIXES at the standard tier (line 1463 shows it does, unconditionally, before the cc/strict conditional).

My analysis of CANDIDATE 1:

  • The behavioral change is real but confined to mode="off". On the default auto/standard tier, the OS launcher already strips _SENSITIVE_ENV_PREFIXES (sandbox.py:1463, applied at every tier) and _PYTHON_ENV_PREFIXES (when strip_python_env=True, line 1470), so the child's final environment is byte-identical before and after. No default-path regression.
  • Only on an explicit agent.sandbox="off" non-delegating spawn does wrap_argv return raw argv with no env -u, so previously AWS/SSH/GPG/PYTHON vars reached the child and now the parent scrub_agent_subprocess_env strips them first.
  • That change is a security tightening (removing more secrets from the agent), not a security hole, crash, data loss, corruption, or removed guard. It is deliberately and explicitly documented in the same commit (security.md rewrite: "making wrapped and delegated Kiro spawns inherit one policy"; the scrub_agent_subprocess_env docstring). The candidate itself scores it "low" and concedes it is plausibly intended hardening.
  • KIRO_API_KEY (not in any scrub prefix) survives; PATH is preserved. No functional break of the agent's own operation.

It does not meet the 80+ real-defect bar, and adding-a-scrub is the secure direction, so it neither blocks nor rises to an advisory finding. Step 2 review of the reworked wrap_argv delegation branch and the parent-scrub ordering found the control flow sound (Windows extra-dirs and SEL-audit-failure paths both fall through to the no-backend fail-closed detect_backend; macOS audit-failure fallback preserved).

No findings.

[OPUS-REVIEWED] c71e9b9

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

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

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 24, 2026
@iamwhatever
iamwhatever merged commit c7a6828 into main Aug 24, 2026
82 of 85 checks passed
@iamwhatever
iamwhatever deleted the codex/fix-windows-kiro-sandbox-default branch August 24, 2026 16:27
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 24, 2026
bolichen97 added a commit that referenced this pull request Aug 24, 2026
(cherry picked from commit c7a6828)

Co-authored-by: Bolin_Chen <17506219+bobbyfine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants