feat: first-party fixed-argv carve-out in sandbox fail-close (#1563)

[bolichen97]

Summary

agent.sandbox_allow_unsandboxed_exec is one boolean that conflates two decisions on a host with no sandbox backend (detect_backend() == "none" — every Windows host, Linux without user namespaces):

1. allowing Kiro Crew's OWN managed MCP servers to spawn — first-party binaries whose argv is fully derived by agent._kirocrew_mcp_invocation(), with no agent/repo/user-config input;
2. unconfining the mode="strict" hostile-input paths (worktree handler's repo-controlled git include.path, Papyrus' crafted-.tex chokepoints).

A Windows user who just wants working first-party MCP tooling had to unconfine the hostile-input paths too. This adds a caller-class axis distinguishing WHO chose the argv, enforced structurally.

What changed

• wrap_argv / sandboxed_spawn_argv gain keyword-only first_party_fixed_argv: bool = False. In the backend == "none" fail-close branch, the spawn proceeds unconfined ONLY when ALL of:
  • the flag is set — a reviewed property, ratcheted by test_spawn_audit.py::FIRST_PARTY_SPAWNS (a new site passing the kwarg without an allowlist entry fails CI);
  • the unavailability class is no_backend — transient still raises (self-heals; must not buy a bypass), foreign_sandbox still raises (host sandbox is fine; remedy is config);
  • no governance sandbox.min_level floor is active — _governance_sandbox_floor_active() shares ONE read (_governance_sandbox_floor()) with _clamp_sandbox_mode, including the PlatformCompositionError-propagates / other-errors-treat-absent posture, so the two cannot disagree.
• Allowed path: env scrub via the trusted absolute-path env binary (_unset_env_argv, never PATH-resolved; on Windows where none exists, the chokepoint's scrub_env on the child env is the guarantee — a superset of the unset keys), one-shot loud SECURITY warning, and a SEL tool-invocation event with a distinct third outcome unconfined (critical=True; SEL failure is log-and-proceed, matching the mode="off" delegation precedent).
• One classified site: the managed-server MCP probe (mcp_discovery.probe_server). The flag is computed by _is_first_party_managed_argv, which requires the spec's command+args to equal the freshly re-resolved _kirocrew_mcp_invocation — stronger than name matching, so an mcp.json row squatting a managed name with custom command text stays fail-closed. The computer_use.py doctor probe was verified NOT to route through the chokepoint (it is in BENIGN_SPAWNS) and is untouched per the spec's verify-before-touching condition. All hostile-input callers are explicitly unchanged.
• Structural ratchet: FIRST_PARTY_SPAWNS allowlist + AST scan for the kwarg (any value expression, not just literal True) + a staleness test.
• No config change: sandbox_allow_unsandboxed_exec=true remains a strict superset — byte-identical behavior when set (pinned by test).
• Docs: security.md fail-closed section documents the carve-out, its three preconditions, and the unconfined SEL outcome; windows-install.md MCP tool-listing row updated. _warn_managed_in_process_once reworded (the "set the opt-in to probe for real" tail went stale — the fallback now fires only for transient/foreign/floor/customized-command cases).

Tested

• New test/test_sandbox_first_party_exec.py (13 tests): full behavior matrix — passthrough + env scrub + SEL unconfined + one-shot warning; no-flag still raises; transient/foreign/floor still raise despite the flag; opt-in byte-identical with/without flag; flag inert with a backend; chokepoint threading; Windows no-env-binary shape.
• test_mcp_discovery.py: 6 new tests on _is_first_party_managed_argv (equality gate, customized-command rejection, resolution-failure fail-safe) and probe flag pass-through.
• test_spawn_audit.py: 2 new ratchet tests.
• Local gates: isort / flake8 / mypy (856 files) / brand / scrub-lint (CI mode) / docs-lint all green. Full pytest: 0 branch-only failures vs a pristine-main baseline run in an identical environment (65 vs 66 failures, all pre-existing host-environmental: this box runs inside a KiroCrew session).
• Pre-push review-fleet subagents were governance-barred in this run (spawn refused); a contract-driven self-review against both reviewer charters was performed instead per the pipeline spec — 1 Low applied (documented the ratchet's **kwargs-indirection limitation). The server-side GPT + Opus review gates on this PR are the real gate.

Closes #1563

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

This is a carefully-scoped, structurally-ratcheted security hardening change (first-party fixed-argv carve-out) with thorough tests. Verifying the key invariants:

• The carve-out only fires when first_party_fixed_argv=True AND _classify_unavailable(transient) == "no_backend" AND not governance_floor — transient/foreign/governed hosts still raise (confirmed in code and tests).
• _is_first_party_managed_argv requires exact equality of command+args+env against the package-derived invocation, rejects the -m kiro_crew interpreter fallback, and fails toward "not first-party" on resolution errors.
• The env scrub, one-shot SECURITY warning, and distinct unconfined SEL event are all emitted; the spawn still routes through wrap_argv (the chokepoint the blocking backend-security-controls rule names).
• Spawn argv ([resolved, *args]) is consistent with the validated server.command since expected_command is the resolved absolute binary.

No AUTOSDE rule is weakened and no residual-class defect is present on the changed lines.

No findings.

[OPUS-REVIEWED] 09f98db

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

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

[github-actions]

GPT 5.6 Review — ✅ human override accepted

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 09f98db02fbac54db4e8d3f238dc66f0f4619f50 — updated in place on each push; does not block merge.

Design-Verdict: PASS

A root-cause split of a conflated opt-in, gated structurally (equality-checked argv, three preconditions, CI ratchet), reversible, and it removes the doc's nudge toward the dangerous global flag.

Suggestions

• The Windows env-scrub guarantee rests on a comment ("every ratchet-allowlisted caller routes through sandboxed_spawn_argv, whose scrub_env…") — make the FIRST_PARTY_SPAWNS ratchet also assert allowlisted sites call sandboxed_spawn_argv rather than bare wrap_argv, so a future wrap_argv-direct site can't silently skip the only scrub Windows gets.

[DESIGN-REVIEWED] 09f98db

[bolichen97]

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97] — response to GPT 5.6 review of f4e3d04 (fixes pushed as 1497a14)

Finding 1 (env bypass) — LEGITIMATE, FIXED. probe_server does merge server.env into the child environment, and _refresh_dynamic_fields preserves user env keys on managed specs, so a spec-carried LD_PRELOAD under a managed name would have executed unconfined with a matching argv. _is_first_party_managed_argv now also requires the spec's env to equal the package-derived agent._managed_mcp_env() ({} on a default install; exactly the KIROCREW_HOME pin under an override home). Tests added: LD_PRELOAD rejection, override-pin exact-match (extra key alongside the pin rejected, missing pin rejected).

Finding 3 (second governance walk) — LEGITIMATE, FIXED. wrap_argv now performs ONE _governance_sandbox_floor() read per call at entry and reuses the value for both the mode clamp (_clamp_sandbox_mode_to_floor) and the carve-out condition. No second profile walk, and the clamp and carve-out can no longer race to different answers. The read-then-clamp wrapper _clamp_sandbox_mode(mode) is preserved for its existing callers/tests (test_governance_chokepoints.py passes unchanged).

Finding 2 (SEL log-and-proceed → re-raise) — REBUTTED as designed behavior. The log-and-proceed posture on the unconfined event is an explicit, documented design decision, not an oversight:

• It matches the two sibling precedents in the SAME function: the mode="off" delegation audit (outcome="delegated") and the nested-passthrough audit (outcome="allowed") both log-and-proceed on SEL failure, for the same stated reason — failing the spawn would couple every built-in MCP probe to SEL filesystem health and re-brick the exact hosts this change fixes (the prior in-sandbox spawn outage documented in security.md § nested-sandbox passthrough).
• Audit-or-deny is reserved in this codebase for paths with a SAFE ALTERNATIVE (_delegate_to_kiro_internal_sandbox falls back to KiroCrew's own seatbelt). A backend-less host has none: deny = built-in tooling bricked on an audit hiccup, for a spawn whose argv+env are package-derived.
• The posture, and its rationale, are documented in docs/system-specs/modules/security.md (first-party carve-out paragraph) and in the issue's decided SEL story.

Design Review CONCERNS (advisory) — answered. The declaration fallback lists tools but cannot verify the server can START — _fix_stale_managed_command exists precisely because managed invocations go stale, and the probe is the one surface that catches it (its ok in the shared _cache_probe store changed meaning silently on backend-less hosts). The issue asks for the caller-class axis explicitly and for structural enforcement; the generic-vs-probe-local trade (ratcheted allowlist + distinct SEL outcome vs an unauditable probe-local bypass) was decided in the issue's fix strategy. The human-ratification ask is satisfied by this PR's review: the trade is stated in the description rather than inherited silently.

[bolichen97]

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97] — response to GPT 5.6 review of 1497a14

Findings 1 (env bypass) and 3 (double governance walk) from the previous round are confirmed cleared. The two remaining blockers are rebutted with evidence:

"Agent-writable package code runs unconfined" — REBUTTED: no new exposure class. The scenario (injected edit to an editable checkout → managed probe executes modified code) is not introduced by the carve-out; it is the pre-existing, explicitly documented residual of backend-less hosts, and GPT's proposed fix has the identical exposure:

• The recommended alternative — _managed_tools_in_process — imports kiro_crew.mcp_core and friends INTO THE GATEWAY PROCESS on every discovery cycle. A Python import executes the module's top-level code, so a tampered package file runs unconfined via the fallback exactly as it would via the carve-out spawn. The code's own long-standing comment in the SandboxUnavailableError handler states this trade: "Reaching here means the sandbox could not confine anything anyway, so the import adds no exposure the refused spawn had not already conceded." The carve-out inherits precisely that already-conceded exposure — while restoring the one property the import cannot provide (verifying the server can START, the reason _fix_stale_managed_command exists).
• On any host WITH a sandbox backend the carve-out is structurally inert (it lives inside the backend == "none" + opt-in-unset branch), so tampered-package spawns remain confined there — unchanged.
• Removing the flag as suggested would therefore keep the identical tampered-code exposure and lose the handshake verification the issue explicitly asks for ("upgrades the 'verify the server can start' probe").

"SEL audit-or-deny" (2nd repetition) — REBUTTED as designed behavior, unchanged from the previous round: the log-and-proceed posture matches the two sibling audits in the same function (mode="off" delegation and nested-passthrough, both log-and-proceed on SEL failure, both documented in security.md), because a backend-less host has no safe alternative — audit-or-deny is reserved for paths that have one (_delegate_to_kiro_internal_sandbox falls back to KiroCrew's own seatbelt). Denying here couples every built-in MCP probe to SEL filesystem health and re-bricks the exact hosts this change fixes. This is the issue's decided SEL story and is documented in the security spec in the same commit.

[bolichen97]

/ai-review override gpt 1497a14: Both findings are designed residuals documented in security.md — the in-process fallback GPT recommends imports (executes) the same agent-writable package code unconfined in the gateway, so the carve-out adds no new exposure; SEL log-and-proceed matches the mode=off delegation and nested-passthrough precedents because a backend-less host has no safe fallback layer.

[github-actions]

Human judgment recorded

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

Both findings are designed residuals documented in security.md — the in-process fallback GPT recommends imports (executes) the same agent-writable package code unconfined in the gateway, so the carve-out adds no new exposure; SEL log-and-proceed matches the mode=off delegation and nested-passthrough precedents because a backend-less host has no safe fallback layer.

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

[bolichen97]

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97] — response to GPT 5.6 review of b0187ed (fixes pushed as 09f98db)

Both round-3 findings are legitimate and fixed:

python -m kiro_crew CWD injection — FIXED as recommended. _is_first_party_managed_argv now refuses the interpreter fallback outright (expected_args[:2] == ["-m", "kiro_crew"] → not first-party): python -m prepends the child's CWD to sys.path (3.10 is supported, so -P/PYTHONSAFEPATH cannot be assumed), and the probe child inherits the gateway's cwd, where a planted kiro_crew/ tree would shadow the install. Only a resolved console-script binary qualifies — which covers the primary Windows target (Scripts\kirocrew.exe); interpreter-fallback installs keep the declaration fallback. Test + security.md updated.

Synchronous critical SEL flush on the event loop — FIXED as recommended. critical=True removed from the unconfined event: unlike the rare one-shot denied/nested-passthrough audits, this fires per managed probe per discovery cycle on a backend-less host, on the async probe_server path. Best-effort async write now; docstring, test, and security.md updated with the rationale.

[bolichen97]

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97] — response to GPT 5.6 review of 09f98db

"Mutable launcher is trusted for unconfined execution" — REBUTTED (2nd repetition of the round-2 finding, console-script variant; same rebuttal, previously overridden on 1497a14). A tampered install is a conceded, pre-existing residual on backend-less hosts, and the recommended fix provides zero protection against it: the declaration fallback _managed_tools_in_process imports kiro_crew.mcp_core/mcp_cron/mcp_computer into the gateway process — executing the same agent-writable package code unconfined — as the existing handler comment states verbatim: "the package dir is writable by the same uid the agent runs as … Reaching here means the sandbox could not confine anything anyway, so the import adds no exposure the refused spawn had not already conceded." The console script is part of that same install prefix. On hosts WITH a backend the carve-out is structurally unreachable, so nothing changes there. Removing the kwarg would keep the identical tampered-install exposure while losing the start-verification the issue explicitly requests.

[bolichen97]

/ai-review override gpt 09f98db: Repetition of the finding overridden on 1497a14 — the recommended declaration fallback imports the same agent-writable package code unconfined into the gateway (documented conceded residual), so the carve-out adds no new exposure on a backend-less host and is unreachable where a backend exists.

[github-actions]

Human judgment recorded

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

Repetition of the finding overridden on 1497a14 — the recommended declaration fallback imports the same agent-writable package code unconfined into the gateway (documented conceded residual), so the carve-out adds no new exposure on a backend-less host and is unreachable where a backend exists.

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