fix(sandbox): screen pinned-spawn PATH by directory identity, not lexically

[dwu96]

Fixes #7095.

What

create_subprocess_limited(..., chdir_fd=fd) sanitizes PATH while the child's working directory is pinned by descriptor — for its own argv[0] resolution AND for the child's environment. The screen was lexical: os.path.isabs(entry) only. That closes every relative spelling ('', ., .., tools), but an absolute entry that resolves inside the bound workspace (e.g. PATH=/Users/me/.kiro/crew/workspace/bin:/usr/bin) passed unchanged — and resolving argv[0] is not the last lookup that happens: the wrapper this spawns looks its own target up on PATH from inside the child, after the shim has already entered the bound directory. A binary planted behind such an entry would be exec'd ahead of the sandbox meant to contain it.

This PR makes the screen an identity check: after the cheap lexical filter, each surviving absolute entry is opened and its (st_dev, st_ino) ancestry (via the existing _directory_ancestor_identities, which walks by descriptor) is tested against the bound descriptor's identity. An entry that IS the pinned directory or lives anywhere beneath it — by any spelling, symlink aliases included — is dropped from the resolve search and from the child's environment alike. _absolutely_rooted_path is renamed _pinned_spawn_path, since the old name described only the lexical half.

Severity, stated honestly

This is not agent-reachable on a default install: it requires the gateway's own PATH to already contain a directory inside the agent workspace, and an agent cannot edit the gateway's environment. It is a screen that did not enforce its stated invariant, in security-critical code, on a configuration an operator could plausibly create. Not an exploitable escape; not cosmetic either — the chdir_fd docstring and the (a)/(b)/(c) invariant comment both already claimed the pinned directory is untrusted for name resolution, and they over-claimed. Closing the gap and correcting the prose are the same change.

Design decisions

• Cost / event-loop: the identity walk opens one descriptor per ancestor level per entry — filesystem work, with the same stalled-NFS/autofs hazard that motivated the existing asyncio.to_thread hop for the resolve. The screen therefore shares one worker-thread hop with the resolve (_screened_spawn_plan), which also means a pinned explicit-path spawn now takes the hop — its child env still needs screening (clause (c)). Returning the screened env alongside the resolved target keeps clauses (b) and (c) fed from the same value by construction.
• Degrade, deliberate: when the bound descriptor's own identity cannot be read (os.fstat(chdir_fd) raises), there is nothing to compare against, so the lexical absolute-only screen stands alone for that spawn. In production chdir_fd always originates from bind_voice_safe_agent_workspace's real opened descriptor; one that cannot be fstated is one the shim's own fchdir rejects before any command runs (pinned by an existing test). This is why every pre-existing TestCreateSubprocessLimited test passing a placeholder chdir_fd=9 under a mocked spawn keeps passing untouched — a zero-regression property asserted deliberately by test_an_unreadable_bound_descriptor_degrades_to_the_lexical_screen, not discovered. The degrade is pinned so it cannot silently become fail-open in a later refactor.
• Fail-closed per entry: a PATH entry that cannot be opened or walked is dropped. An unopenable entry cannot contribute a resolvable binary today, and dropping is the direction that cannot be gamed by making a directory un-stat-able.
• Fail-closed on empty: the screen emptying PATH still raises FileNotFoundError from the resolve, exactly as before. No fallback added.
• Scope unchanged: unpinned spawns keep full execvpe parity, relative entries included — pinned behavior only.

Tests (all in test/test_spawn_exec_shim.py::TestCreateSubprocessLimited, real directory descriptors)

• absolute entry inside the workspace dropped — from the search AND the child env; planted binary loses to an outside entry whose pathname deliberately extends the workspace's own (pinned-outside startswith pinned), so the same test guards against a string-prefix reimplementation
• the bound directory itself as an entry dropped (the E is bound case)
• a symlink alias of the workspace dropped, with os.path.realpath deliberately broken in the test — proving the screen is descriptor identity, not pathname/realpath string comparison
• screen emptying PATH raises FileNotFoundError
• unopenable entries (ENOENT and ENOTDIR) dropped, resolution through the surviving entry still works
• the degrade path pinned (deterministically unreadable descriptor at the NOFILE soft limit)
• unpinned spawn env byte-identical
• screen + resolve run off the event loop

Red-before-fix: 6 of the new tests fail on unfixed source; the two behavior-preserving pins pass. Mutation checks, each killed: (1) revert to lexical isabs, (2) equality-only (drop the descendant half), (3) realpath string comparison instead of descriptor identity, (4) screen the search but not kwargs["env"], (5) invert per-entry OSError to keep.

Full local verification: test_spawn_exec_shim.py + test_sandbox_argv.py + test_spawn_audit all green (255 passed); full backend suite vs an origin/main worktree with failure sets compared both directions — branch failures (107) are a strict subset of main's (108, the one main-only failure is an unrelated flake that passed on branch), identical 2 pre-existing setup errors. black/isort/flake8/mypy clean.

Out of scope (audited, not fixed here)

• _resolve_spawn_target's if cwd: relative-entry join — correct for the unpinned path, unreachable on the pinned one (the pinned branch passes cwd=None); claim re-verified on this tip.
• Broadening the screen to unpinned spawns — an execvpe-parity break, a product decision.

No screenshot evidence: backend-only change with no visual surface; the evidence is the focus of the test assertions above.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A named boundary (agent-writable workspace vs. the sandbox spawn path) enforced at the one lever that controls the child's later lookup — proportionate, fail-closed, degrade pinned.

The screen is aimed at the right mechanism: the wrapper's own in-child PATH lookup after fchdir, which no parent-side argv resolution can cover, so screening the env itself is the correct shape rather than a symptom patch. Descriptor-identity comparison plus re-spelling kept entries from the verified descriptor closes the alias and retarget windows the first commit alone left open; per-entry and empty-PATH failures both fall toward refusal; the fstat-degrade is deliberate and test-pinned rather than silent. Scope stays pinned-spawn-only, preserving execvpe parity elsewhere, and everything is private helpers — fully reversible. The obvious simpler alternative (replace PATH with a fixed trusted set) would break operator-customized wrappers; this wins.

[DESIGN-REVIEWED] c8f8fdf

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c8f8fdf

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

The single candidate (sandbox.py:6793, emptied screened PATH falling back to os.defpath) does not survive falsification. Its (c) never resolves to an observable boundary-harming outcome:

• The or os.defpath fallback lives at line 6570 in _resolve_spawn_target, which the diff does not touch; it is pre-existing behavior shared with the correct non-pinned path (test_an_unpinned_spawn_still_resolves_a_relative_path_entry relies on it).
• The fallback's leading empty component and /bin:/usr/bin are resolved by a parent-side shutil.which against the gateway's cwd — never the child's pinned cwd, never the pinned workspace or anything inside it. The actual invariant boundary (the untrusted pinned directory) is not breached: the child still receives PATH="", and any name that resolves resolves to a system binary outside the workspace, not a planted workspace binary.
• The candidate itself concedes "practical escalation is limited… no workspace binary is reached," and the workspace-only-PATH case demonstrably fails closed (test_chdir_fd_with_only_workspace_path_entries_resolves_nothing raises FileNotFoundError). What remains is a docstring-precision gap on unchanged code, not a reachable defect on the changed lines.

No new grounded findings emerged; the identity-screen logic closes fds via finally on every branch (including the continue under bound_identity in ancestors), resolved_entry is always bound where read, and the added tests cover the descendant/self/alias/unopenable/re-spell/degrade cases.

[OPUS-REVIEWED] c8f8fdf

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

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of c8f8fdfda78fa426fa1b156d746df8486d59fdec — 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 counts I need are confirmed. Writing the review now.

First-Principles-Verdict: CONCERNS

The sandbox fix earns every part of itself; roughly two-thirds of this diff is other jobs the description never mentions, and it matches commits already sitting on the base branch.

What this change ships

Intent: stop a binary planted inside the pinned agent workspace from winning the child's PATH lookup during a sandboxed spawn (#7095) — a FIX.

1. Absolute PATH entries inside the pinned workspace now dropped, by (st_dev,st_ino) identity — justified (reported defect Pinned-spawn PATH screen keeps absolute entries inside the bound workspace #7095; the (a)/(b)/(c) invariant already claimed it)
2. Kept PATH entries re-spelled to the verified descriptor's canonical path — justified (closes the symlink retarget window the screen otherwise reopens)
3. Pinned explicit-path spawns now take a worker-thread hop — declared, justified (same stalled-NFS hazard as the resolve)
4. Unreadable bound descriptor degrades to the lexical screen — declared, pinned by test, justified
5. New CI gate + script banning repo paths in shipped skills — undeclared, rides along
6. Per-test SEL root fixture + trust-race tests — undeclared; matches base tip cda8750 ("…own SEL root… (test: give each test its own SEL root so chain-lock races cannot refuse trust (#7029) #7109)")
7. MCP app marker moved to lead the tool text, re-injected after cuts — undeclared; matches base commit 9a444de (fix: prepend mcp app marker so it survives acp result cuts #6637)
8. Website app-art failures rebound to URL generations — undeclared; matches base commit 4bd1629 (fix(website): bind app art failures to url generations #6964)
9. Personal-shopper embed ops moved to the mc-embed pool; Windows stale-PID gateway-lock reclaim; config_dir memo key fix; source-provider marker cap + ConfirmationRequired pass-through — four more undeclared riders (grouped)
10. Session-test provider mocks gain runtime_info/stream_command/close_all — undeclared, rides along
    More than 10 differences exist; these are the most visible.

Watch

• The description says "backend-only change with no visual surface", yet the patch edits website/src/pages/AppDetailPage.tsx — but that hunk's subject is the base tip's neighbour 4bd1629, and the checkout's merge is "Merge c8f8fdf into cda8750", whose base-side subjects match inventory items 6–8 verbatim. The patch was almost certainly computed against a stale base and sweeps in merged main work. A human should confirm fix(sandbox): screen pinned-spawn PATH by directory identity, not lexically #7114's real delta against cda8750 is the sandbox change alone; if any of items 5–10 are actually on the branch, they are riders in a fix and belong in their own PRs.
• The sandbox change itself is cause-level, reuses the existing _directory_ancestor_identities walk (3 prior consumers, sandbox.py:482,489), and declares its one unfixed sibling (unpinned spawns) with a derived reason — no depth finding.

Subtractions

• Drop the chdir_fd: int | None = None default on _pinned_spawn_path (src/kiro_crew/sandbox.py:6583) — grepped _pinned_spawn_path(: one production caller (sandbox.py:6791) and it always passes the fd; the None arm is dead surface, make the keyword required.

[FIRST-PRINCIPLES-REVIEWED] c8f8fdf

[github-actions]

UX Review (Fable 5) — ✅ PASS

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

UX-Verdict: PASS

Backend sandbox fix plus an image-latch timing repair that only improves the experience: failed hero/screenshot art now retries on theme flip-back instead of staying hidden.

[UX-REVIEWED] c8f8fdf

[dwu96]

Disposition of First Principles concerns (head c8f8fdf)

Riders (inventory items 5-10) - declined as not-on-branch, with evidence. The lane's own Watch paragraph self-diagnosed this correctly: its patch was computed on a merge checkout against a stale base, sweeping in work already merged to main (the cited subjects match base commits cda8750, 9a444de, 4bd1629). The authoritative GitHub compare for this PR lists exactly two files:

$ gh pr view 7114 --json files --jq '[.files[].path]'
["src/kiro_crew/sandbox.py","test/test_spawn_exec_shim.py"]

The branch carries two commits on top of main touching only those files - the identity screen and its review-round hardening. None of items 5-10 (skill-path CI gate, SEL root fixture, MCP marker move, app-art rebind, embed-pool/gateway-lock/config-memo/marker-cap changes, session-test provider mocks) exist on this branch, so there is nothing to split out. The website/src/pages/AppDetailPage.tsx hunk the Watch item flags is likewise base-side merge content, not PR delta - consistent with the PR body's "backend-only" claim.

Subtraction (make chdir_fd required on _pinned_spawn_path) - declined on design grounds. The observation is accurate: the single production caller (sandbox.py:6791, inside the if chdir_fd is not None: arm) always passes the descriptor, so the None default is never exercised by production code today. It is kept deliberately, not accidentally:

• The optional keyword is what makes the lexical-only screen a REAL, independently callable behaviour rather than an unreachable branch: _pinned_spawn_path(env) IS the documented degrade semantics - the bound-descriptor-unreadable path (os.fstat raising) lands on exactly the same lexical-only outcome, and the function docstring describes the two screens as cheapest-first layers with lexical as the base.
• Making the keyword required would not delete the None arm - the degrade path needs the identity half to be skippable regardless - so the subtraction removes a call-shape without removing code, while costing the ability to invoke the documented lexical-only behaviour directly.

If a maintainer prefers the required-keyword shape, it is a two-line follow-up with no behaviour change; it is not taken here to keep this security fix at its reviewed surface.