fix(sandbox): let an unreadable cc expose source degrade, not abort

[rnoack1]

Problem / Motivation

The cc-mode sandbox launcher pre-reads EXPOSE_FILES before bind-mounting empty
directories over the credential paths, so ~/.aws/config survives the hiding and
credential_process still resolves inside an otherwise-hidden ~/.aws. The read was
unguarded (src/kiro_crew/sandbox.py, in the generated launcher):

# Pre-read files that must survive dir hiding
expose_data = {}
for src_path, filename in EXPOSE_FILES:
    if os.path.isfile(src_path):
        with open(src_path, "rb") as fh:      # <-- unguarded
            expose_data[src_path] = fh.read()

if os.path.isfile(src_path) already handles the file being absent. It does not
handle the file being unreadable, and those are different conditions: stat can
succeed on a path whose open is then denied.

This also rules out the weaker fix. Tightening the guard to a pre-flight
os.access(src_path, os.R_OK) looks equivalent from the source alone, but measured on the
affected host os.stat() succeeded and os.access() reported both X_OK and R_OK
as True while the operation was denied anyway. Catching the error is the only guard that
holds, and a test now pins that (see below) so the substitution cannot be made silently.

Because this read happens during sandbox setup, the OSError aborts the child
before the command runs at all. _CC_EXPOSE_FILES is non-empty only when
sandbox_level == "cc", so the failure is scoped to cc mode.

Why it matters (measured impact)

On one host this took out an entire cron kind. cron_script.py splits:

entry point	wrap mode	outcome
run_script_sandboxed	mode="standard"	unaffected
run_command_sandboxed	mode="cc"	every spawn died in setup

Seven command crons failed. Jobs latched into auto-pause off the back of it, and an
auto-paused job cannot self-heal: enabled is derived as
not user_paused and not auto_paused, so the scheduler never fires it and it can never
record the success that would clear the latch.

One figure is deliberately not stated more precisely than the evidence supports. An
independent scan of that host's cron history corroborates the seven jobs (7 jobs, 38
occurrences, one path, a 77-minute window closing at the restart), but it could not
attribute each auto-pause latch to this defect: 6 of the 7 jobs show a >= 5 consecutive
non-ok streak, yet those streaks (37, 15, 16, 14, 6, 7) far exceed their
PermissionError counts (12, 7, 7, 6, 3, 2), so other failures from the same window are
mixed in. Treat the per-job latch attribution as unestablished.

Each failure surfaced only as a raw traceback in the job's last_result. Nothing was
logged at ERROR level, so the surface an operator would check showed nothing.

On the cause: not established

I could not determine what produced the EACCES, and this PR does not claim to know.
The following were ruled out by measurement:

• DAC permissions — the file was -rw-------, owned by the same user the process ran
  as; widening the parent directory changed nothing.
• uid mismatch — the process ran as the file's owner (confirmed via ps).
• SELinux — getenforce reported Permissive.

The restriction was inherited from the parent process and disappeared when the process was
restarted from a clean parent (verified: 7/7 jobs back to last_status=ok,
consecutive_failures=0). A kernel LSM is the obvious suspect and landlock is present in
/sys/kernel/security/lsm on that host, but KiroCrew has zero landlock references, so
whatever applied it came from the environment rather than from this codebase. I am
deliberately not asserting it.

The argument does not depend on the cause. Selective exposure is an optimisation, not
a security control, so a failure to read one should degrade to "not exposed" rather than
abort the whole spawn.

What changed

Wrap the pre-read in try/except OSError, skip that entry, and warn on stderr:

except OSError as exc:
    print(
        "sandbox: WARNING — cannot read %s (%s); it will be "
        "ABSENT inside the sandbox. Anything depending on it "
        "(e.g. credential_process in ~/.aws/config) will fail."
        % (src_path, exc),
        file=sys.stderr,
    )

This follows the precedent already in the launcher: the Step 7 pre-exec hardlink scan
deliberately "degrades OPEN with a stderr warning rather than failing closed" when its
budget is exhausted.

The diagnostic is not optional. A bare except OSError: pass would trade a loud
setup failure for a silent one — the child would have no ~/.aws/config and no
explanation, and auth would fail later with an error pointing nowhere near this line.

Scope note: the restore loop below is already keyed on src_path in expose_data, so a
skipped entry is skipped there too with no second change needed.

The launcher's other pre-read of this shape is guarded too — in the OPPOSITE
direction. The known_hosts exposure (src/kiro_crew/sandbox.py, the .ssh block in the
generated launcher) had the identical unguarded os.path.isfile(...) -> open(...)
construction. Same root cause; not the same remedy. That read now FAILS CLOSED: it
reports sandbox: FATAL naming the path, then re-raises, so sandbox setup is refused.

The asymmetry is the whole point, so it is worth stating plainly:

• An unreadable ~/.aws/config costs reachability. Skipping it trades a convenience for
  a working sandbox, and nothing about host trust changes.
• An unreadable known_hosts costs verification. The launcher puts
  StrictHostKeyChecking=accept-new into GIT_SSH_COMMAND, gated only on that variable
  being unset — never on whether this read succeeded. So continuing with an empty kh_data
  would point UserKnownHostsFile at an absent file while auto-accept is still on: every
  host then reads as NEW and an interceptor's key is accepted. With known_hosts present,
  accept-new refuses a changed key.

Degrading there would therefore convert "refuse a changed key" into "accept anything" — a
fail-open on a security control. Refusing is the safe direction: no sandbox at all beats one
that has quietly stopped verifying hosts.

This site also has wider reach than the expose read: it is gated on HIDE_SSH, which is
set at the default strict level, whereas the expose read is reachable only when
sandbox_level == "cc".

A census of the generated launcher's child-setup path finds exactly two isfile -> open
pre-reads, and both are now guarded. (A third isfile in that path guards a bind-mount,
not a read.)

Tests

Ten tests in the existing test/test_sandbox_cc_mode.py — six for the expose pre-read and
four for the known_hosts pre-read. They are deliberately not mirrors: the expose tests
pin a degrade, the known_hosts tests pin a refusal. They extract each read from the
shipped launcher and run it via runpy.run_path, following the pattern
test/test_sandbox_hardlink_scan.py already established, so they cannot drift from what
the child actually executes. _EXPOSE_SLICE_LANDMARKS and _KH_SLICE_LANDMARKS fail
loudly if an edit shrinks either extracted slice, rather than leaving the assertions
vacuously green. Only the known_hosts pre-read itself is sliced, not the whole .ssh
block, because the lines around it call _libc.mount().

test	asserts
..._does_not_abort_setup	an unreadable source does not raise, and is not exposed
..._is_reported_on_stderr	the skip is reported, naming the path
..._is_still_read	positive control: the readable path still works, silently
..._stays_silent	an absent source stays silent (isfile shorts out first)
..._does_not_block_the_others	the skip is per entry, not per loop
..._not_a_pre_flight_access_check	the read is attempted and the error caught, rather than gated on os.access
..._known_hosts_aborts_setup	an unreadable known_hosts RAISES, and the refusal is reported on stderr naming the path and marked FATAL
..._known_hosts_is_still_read	positive control: the readable path still works, silently
..._absent_known_hosts_stays_silent	an absent known_hosts stays silent (isfile shorts out first)
..._known_hosts_guard_is_the_exception...	the read is attempted and the failure REFUSES, rather than being gated on os.access

Verified against three break-arms:

• guard removed — 3 tests fail with PermissionError: [Errno 13], reproducing the
  defect.
• guard present but silent (except OSError: pass) — 2 tests fail on the stderr
  assertions (skipping an exposure silently must not be an option).
• guard replaced by and os.access(src_path, os.R_OK) — 3 tests fail, including the
  one that exists for this case, so the weaker fix cannot be substituted silently.

The known_hosts tests were verified against their own break-arm. Reverting that guard to
the unguarded read makes 2 of the 4 fail — one on the missing refusal, one on the missing
diagnostic. The other 2 are the positive controls (readable, absent), which pass with or
without the guard by design: they are what would catch a "fix" that refused unconditionally.

The refusal and its diagnostic are asserted together in ..._known_hosts_aborts_setup,
because they are one behaviour observed from one setup. Splitting them across two tests
required a module-level global to carry stderr past the raise; the harness now takes a
caller-owned stderr_sink instead, so no test state lives at module scope.

So for the expose site the non-fatal requirement and the diagnostic requirement are each
enforced independently, rather than one riding on the other.

test/test_worktree_create.py — the severity ratchet. The new sandbox: FATAL line adds
a severity to the launcher's vocabulary, and that file pins the SET of severities the
launcher may emit so a new one is a deliberate edit rather than a silent reclassification.
FATAL is added to the pinned set, and the fatal-vs-advisory decision is encoded rather than
just asserted: the classifier
(src/kiro_crew/dashboard/handlers/worktree.py) treats a line as a refusal when it starts
with sandbox: and NOT with sandbox: WARNING, so two assertions pin that FATAL carries
the former and not the latter, and a new test
(test_the_fail_closed_pre_read_line_still_refuses) proves the classifier actually raises
SandboxUnavailable for the new spelling. The existing advisory — the hardlink-scan
WARNING, which must never be read as a refusal — is untouched. Verified with a negative
control: injecting a throwaway sandbox: NOPE line still fails the ratchet, so it has not
been widened into uselessness.

The two tests that need an unreadable file skip themselves via os.access if the host can
read a 0000 file (root, or a filesystem ignoring the mode).

Manual verification

• test/test_sandbox_cc_mode.py, test_sandbox_hardlink_scan.py, test_sandbox_argv.py
  — 205 passed, 1 skipped.
• scripts/check_black_formatting.py, scripts/check_subprocess_encoding.py,
  scripts/scrub-lint.sh --no-history, scripts/check_lockdown_before_publish.py,
  scripts/docs-lint.sh — all pass at the merge scope CI uses.
• black --target-version py310 is clean on sandbox.py; in the test file it is clean on
  every line this change adds (the file's two pre-existing offences at lines 30 and 329 are
  in the baseline and left untouched).

Repo checks before opening

• No existing issue matches (searched "EXPOSE_FILES expose_data landlock" and
  "sandbox aws config PermissionError"; a bare "sandbox" search confirms the query does
  reach open issues, so the two empty results are not an artefact of the search).
• Three open PRs touch src/kiro_crew/sandbox.py — fix(sandbox): give the shielded spawn wrapper one owner, migrate 6 sites #5863, fix: refuse to let the agent relocate the data home #5411, feat: monitor pull requests across source providers #5305 — and none of them
  modifies the expose pre-read. fix: refuse to let the agent relocate the data home #5411 carries those lines as diff context only (no
  +/- on them), so a textual conflict in that function is possible but there is no
  semantic overlap with this change.

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[rnoack1]

Two measurements from the same host that landed independently of this PR, plus a survey of the sibling reads in the same launcher. Nothing here contradicts the diagnosis — one item narrows the choice of fix, and one is a caveat on a figure.

1. The EACCES is invisible to stat() and os.access() — which rules out the obvious alternative guard

Reproduced live, in-process, inside a cc-mode sandbox on the affected host, probing the parent directory:

dir mode: 0o700  uid: 12900012  searchable: True  readable: True
PermissionError: [Errno 13] Permission denied: '<home>/.aws'

os.stat() succeeds, and os.access() reports both X_OK and R_OK as True, while the actual directory operation is denied anyway.

Two consequences:

• It is consistent with the PR's "restriction was inherited from the parent process" reading, and with declining to blame DAC. A DAC-only restriction cannot produce a stat-permitted / operation-denied split for the owning uid.
• It disqualifies os.access() as a pre-flight guard. A reader looking at if os.path.isfile(src_path) might reasonably propose tightening it to os.access(src_path, os.R_OK) instead of catching the error. On this host that check returns True and the following open() still raises — so try/except OSError is not just the tidier option, it is the only one that actually holds. Might be worth a line in the comment block, since the weaker fix looks equivalent from the source alone.

Related retraction of my own: I had separately argued the parent must be genuinely searchable because os.path.isfile() swallows OSError and would otherwise have skipped the file entirely. That inference fails for the same reason the access() check fails, so I withdraw it. The parent's real searchability is not established — and the fix does not depend on it.

2. The failure window correlates with gateway loop-stalls, and closes exactly at the restart

An independent scan of that host's cron history agrees with "seven command crons" precisely: 7 jobs, 38 occurrences, all one path, window 02:07:38Z → 03:24:44Z (77 min).

Loop-stall dumps from the gateway land inside that window, twice within two seconds of a failure:

loop-stall dump	nearest failure(s)
02:06:02Z	02:07:38Z, 02:10:04Z
02:23:12Z	02:23:10Z (−2s)
02:34:46Z	02:34:44Z (−2s)
03:25:15Z	03:24:14Z, 03:24:44Z

The window closes at the ~03:22Z gateway restart, after which every run is ok. That is the same event the PR describes as "restarted from a clean parent" — it is independently visible in the dump timeline, so whatever applied the restriction arrived with, or alongside, a wedged parent. I am not claiming a mechanism connecting the wedge to the restriction; the correlation is all I have.

Caveat on one figure. I could not confirm or dispute "three reached _AUTO_PAUSE_THRESHOLD". My metric conflates error classes: 6 of the 7 jobs show a ≥5 consecutive non-ok streak, but those streaks (37, 15, 16, 14, 6, 7) far exceed their PermissionError counts (12, 7, 7, 6, 3, 2), so other failures from the same window are mixed in and I cannot attribute each latch to this defect. Flagging it only so the number is not treated as independently corroborated.

3. Sibling reads with the same shape (not a request to widen this PR)

I went looking for other EXPOSE_FILES entries that share the hazard. There are none — the list has exactly one entry, and the macOS site is structurally immune, since it never reads the file at all: it emits (require-not (literal …)) exceptions inside the deny file-read* rule, and cc mode there excludes .aws from hiding entirely.

But the shape — isfile() as the guard, unguarded open(), during setup — recurs three more times in the same launcher:

1. The .ssh/known_hosts pre-read. Identical construction (if os.path.isfile(SSH_KNOWN_HOSTS) then an unguarded open), and it exposes optional content for exactly the same reason. It is gated on HIDE_SSH, i.e. sandbox_level == "strict" — the default level, so its reach is wider than the cc-only path this PR fixes, not narrower.
2. The expose restore write. The scope note is right that a skipped entry propagates via if src_path in expose_data, but that covers a failed pre-read only. If the pre-read succeeds and the restore open(dest, "wb") / os.chmod(dest, 0o444) fails, an OSError still aborts setup — one loop below the fix, same severity, same optional-convenience justification.
3. The known_hosts restore write, same as (2).

Separately, and in the opposite direction: all four _libc.mount() calls discard the return value, although restype is c_int and the CDLL is built with use_errno=True. unshare is checked a few lines above, with a specific errno exit — so the checking pattern exists in the same function and the mounts are the exception. A failed bind-mount means a credential directory is not hidden and the spawn proceeds silently: fail-open on the actual security control, where the bug this PR fixes fails closed on something the PR correctly identifies as not a control. I found no post-mount verification (positive control: verify does occur elsewhere in the file, so the search can find it; within the launcher body the only return-value checks are the two unshare calls).

All of (3) is follow-up material — this PR is scoped to one loop and reads well as-is. Raising it here only so the neighbours are on record rather than rediscovered from the next incident.

Environment for everything above: single Linux host, cc mode via run_command_sandboxed.

[rnoack1]

Thanks — all three actioned in code at 97acc47b5, not argued in prose. Dispositions:

1. os.access() is disqualified as a pre-flight guard — FIXED IN CODE.
You were right that the weaker fix looks equivalent from the source alone, so a comment
line alone would not hold. Two changes:

• The comment block now states the measurement and forbids the substitution explicitly
  ("Catching the error is the only guard that HOLDS. Do not 'tighten' this into a
  pre-flight os.access(src_path, os.R_OK)").
• A test now enforces it:
  test_the_guard_is_the_exception_not_a_pre_flight_access_check runs the shipped block
  with an os whose access() lies exactly the way the real one did, and asserts the read
  is attempted and the failure reported. Break-arm confirmed: rewriting the guard as
  and os.access(src_path, os.R_OK) fails 3 tests including that one.

Your retraction is applied too. I had carried the same unsupported inference — the
comment and the description both said the EACCES "still passes isfile whenever the path
is traversable and stat-able". Both now say only that stat can succeed on a path whose
open is then denied. No traversability claim remains on either surface.

2. The _AUTO_PAUSE_THRESHOLD figure — CORRECTED IN THE DESCRIPTION.
The defect here was the claim itself, so amending it is the fix rather than a rebuttal. The
description no longer asserts "three reached _AUTO_PAUSE_THRESHOLD = 5". It now records
that jobs latched, states your corroborated figures (7 jobs / 38 occurrences / one path /
77-minute window closing at the restart), and says plainly that per-job latch attribution
is unestablished, with your streak-vs-PermissionError counts as the reason.

3. Sibling reads and the unchecked _libc.mount() returns — NOT actioned, needs your
call. You scoped this as follow-up and said it is not a request to widen the PR, so I
have deliberately not touched it, and I am flagging rather than closing it since it is not
mine to refuse. Four items on record: the .ssh/known_hosts pre-read (same construction,
gated on HIDE_SSH, i.e. the default strict level, so wider reach than this cc-only
path), the expose restore write, the known_hosts restore write, and the four
_libc.mount() calls discarding their return value despite use_errno=True — that last
one failing open on an actual security control.

Tell me which you want: fold the known_hosts pre-read into this PR since it is the same
one-loop change with wider reach, or keep this PR at one loop and take all four as
follow-ups.

Full suite locally: 206 passed / 1 skipped across test_sandbox_cc_mode.py,
test_sandbox_hardlink_scan.py, test_sandbox_argv.py. Black, subprocess-encoding,
scrub-lint, lockdown and docs-lint all pass at CI merge scope against current main.

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of c8b9ee5370e8eb87adf1260851fe17fb8fbae1f7 via the fork AI-review pipeline — 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 against the base. I have everything needed for the review.

First-Principles-Verdict: PASS

One mechanism-level fix, its counted sibling handled in the opposite (derived) direction, and every new surface has a named consumer.

What this change ships

Intent: stop one unreadable optional credential file from killing every cc-mode sandbox spawn (and with it the whole command cron kind). This is a FIX.

1. Unreadable expose source now degrades to "not exposed" instead of aborting the spawn — justified; mechanism-level.
2. New stderr WARNING naming the skipped path — justified; classifier-safe (matches _SANDBOX_LAUNCHER_WARNING_PREFIX, worktree.py:154).
3. Unreadable known_hosts abort now prints sandbox: FATAL before re-raising — rides along (same root cause, counted sibling); justified: 1 consumer, worktree.py:272 otherwise misreads the bare traceback as a git error.
4. Ten tests sliced from the shipped launcher — justified; follows test_sandbox_hardlink_scan.py precedent.
5. Worktree severity census extended with the FATAL spelling — rides along with item 3; pins its one consumer.

Verified counts: exactly two isfile → open pre-reads in the launcher (sandbox.py:1685, 1727; the third isfile at 1718 guards a bind-mount) — both addressed, zero unfixed siblings. accept-new is injected gated only on GIT_SSH_COMMAND being unset (sandbox.py:1758-1764), so fail-closed at known_hosts is derived, not symmetry.

Watch

• The base already failed closed at known_hosts (an unguarded raise aborts setup), so item 3's real delta is the diagnostic, not the remedy the description's "That read now FAILS CLOSED" implies.
• The docstring sentence "This test previously pinned the opposite, and that was a defect" is false at base — no known_hosts test existed (grepped known_hosts under test/: only an unrelated fixture in test_sandbox_mount_checked.py). Delete that sentence.

[FIRST-PRINCIPLES-REVIEWED] c8b9ee5

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of c8b9ee5370e8eb87adf1260851fe17fb8fbae1f7 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A measured setup-abort fixed at the fragile read itself, with the fail-open/fail-closed split correctly derived from what each file protects.

I verified the claims against the base tree: both isfile → open pre-reads are unguarded in the shipped launcher (sandbox.py:1682-1687, 1727-1729), accept-new is injected gated only on GIT_SSH_COMMAND being unset (sandbox.py:1758), and the worktree classifier's prefix-pair logic (worktree.py:272-274) classifies the new FATAL spelling as a refusal exactly as described. The asymmetry — degrade the reachability-only expose, refuse on the host-trust anchor — is the right design, and the known_hosts site's behavior is unchanged (it already aborted via the bare OSError; it now aborts diagnosably and lands on the SandboxUnavailable path instead of masquerading as a git error). No new public surface, no one-way doors, and the severity-ratchet test makes the new stderr vocabulary a deliberate contract change rather than a drive-by.

[DESIGN-REVIEWED] c8b9ee5

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed c8b9ee5370e8eb87adf1260851fe17fb8fbae1f7 via the fork AI-review pipeline; updated in place on each push.

Review details

The candidate file contains the discovery pass's own conclusion ("No candidates."), not actual candidates — so Step 1 has nothing to falsify. Verifying the diff independently:

• The launcher body is a brace-doubled format template ({{}}, {strict_host_key_opt}); the added print(...) lines contain no single braces, so template expansion is unaffected, and the %s/% are literal within the template.
• EXPOSE_FILES pre-read degrades open (warns, continues); the restore loop at sandbox.py:1699-1700 guards on src_path in expose_data, so an unread source is correctly skipped.
• known_hosts pre-read re-raises (fail-closed) with a distinct sandbox: FATAL line; the worktree classifier keys on the launcher prefix and excludes only the WARNING prefix, so FATAL classifies as a refusal.

No grounded (a)/(b)/(c) chain to an observable wrong outcome exists in the changed lines, and nothing in the base rules is weakened.

No findings.

[OPUS-REVIEWED] c8b9ee5

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed c8b9ee5370e8eb87adf1260851fe17fb8fbae1f7 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c8b9ee5