fix(storage): walk the empty-trash guard by descriptor, not by name

[jeeshofone]

Problem / Motivation

Fixes #8724. On macOS, a Trash batch whose nested tree pushes component paths past PATH_MAX (1024) becomes permanently un-emptyable: _unlisted_files() — the guard that decides whether an Empty-Trash delete may proceed — still walked by name with os.walk after #7011 descriptor-hardened the approval scan, chain-open, and removal passes. The walk dies with ENAMETOOLONG, which the guard's fail-closed contract converts into the SessionStorageError the caller maps to SKIP_UNREADABLE — so the batch silently survives every Empty-Trash attempt, with only a warning log as a trace.

Why it matters

The failure is silent and permanent: the batch shows in Trash, Empty-Trash reports success, and the data is never reclaimed. It also inverts #7011's intent — the passes that act were descriptor-hardened, but the gate that authorizes them remained the one name-based walk, so the platform path limit wedges the whole batch. The #7011 deep-nesting regression test only passes on Linux CI because Linux's PATH_MAX is 4096; on macOS it fails on main today.

What changed

_unlisted_files now walks with os.fwalk: traversal and the per-entry stat run via directory descriptors (dir_fd), so only manifest-relative strings ever use the textual path — pure string operations with no length limit. Two semantics deliberately preserved:

• The per-entry check mirrors Path.is_file() exactly: os.stat(name, dir_fd=rootfd) follows symlinks, and the errno set Path.is_file() reports as False (ENOENT/ENOTDIR/EBADF/ELOOP) is skipped rather than treated as a scan failure; any other OSError still joins the failure list and blocks the delete.
• Unlike os.walk, fwalk RAISES when the top itself cannot be opened instead of routing that error through onerror — the new wrapper catches it into the same failure list, so an unopenable batch remains a reason'd refusal, never an escaping OSError.

Two test doubles that monkeypatched session_storage.os.walk are repointed at fwalk with matching signatures; they assert the guard's contract (hostile-name escaping, unreadable-scan refusal), not the walk mechanism.

Tests

• On macOS 15 (arm64), the fix(system): bind the session-Trash delete to descriptors, not names #7011 deep-nesting test (test_a_deeply_nested_batch_does_not_break_the_walk) fails on main with assert ['unreadable_batch'] == [] and passes with this change — the exact fail→pass across the fix for the reported mechanism. (It passes on Linux CI either way because Linux's PATH_MAX is 4096.)
• Full test/test_session_storage.py: 229 passed.
• black gate, isort, flake8, mypy: green on both changed files.

Pattern harvest

Rule candidate: guards that gate destructive operations must not walk by name — a platform path-length limit converts a traversal error into a permanently wedged state. Use os.fwalk where the platform defines it (CPython: only where {open, stat} <= os.supports_dir_fd, so never on native Windows) and keep the name-based walk as the fallback; note fwalk raises the top-level open error instead of calling onerror, so wrappers must catch it to keep fail-closed semantics.

[dwu96]

👋 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
• ## Tests

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.

[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
• ## Tests

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.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

FINDING -- src/kiro_crew/session_storage.py:1543 -- "CPython's fwalk is iterative" contradicts its recursive _fwalk, misleading the deep-tree failure contract -> Fix: describe the recursion and caught refusal accurately.
[GPT-REVIEWED] 651615d

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

The candidate list contains no candidates (the discovery pass found none). I independently verified the one behavioral change: replacing os.walk + Path.is_file() with os.fwalk + os.stat(name, dir_fd=rootfd).

Key checks:

• The dangerous direction here is under-reporting leftovers (_unlisted_files returning empty when files exist → _discard_restored_batch deletes the batch → data loss). Both walk and fwalk enumerate the same directory entries with identical is_dir()-based classification, so no regular file is dropped from the names set.
• The ignored-errno set {ENOENT, ENOTDIR, EBADF, ELOOP} matches CPython's pathlib._ignore_error exactly; both old and new treat those as "not a file" (not counted), and every other errno (e.g. EACCES) routes to failures → SessionStorageError, keeping the batch. No divergence in the deletion-safety direction.
• follow_symlinks=True on the stat matches is_file(); symlink-to-file/broken-link/FIFO/socket/symlink-to-dir all classify identically.
• os.stat(name, dir_fd=rootfd) uses only a single path component, so the macOS ENAMETOOLONG wedge (macOS: deep Trash batches are permanently un-emptyable (PATH_MAX vs the name-based unlisted-files guard) #8724) is genuinely avoided; the textual path is used only for pure relative_to(...).as_posix() string ops.
• fwalk availability implies stat supports dir_fd; the Windows else branch preserves the original name-based walk verbatim; the outer except OSError/RecursionError keeps any top-level or impossible traversal failure inside failures, never escaping as a crash.

No grounded (a)/(b)/(c) defect on the changed lines, and no base-rule pattern targets this file.

No findings.

[OPUS-REVIEWED] 651615d

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 651615df6bdef685d8243f917582d6a6719bdd70 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

Root-cause fix in the module's established descriptor discipline: the last name-based walk on the empty-trash path, with fail-closed semantics provably preserved.

The residuals I probed all check out against the base tree: the O(depth)-fd / extreme-depth failure mode of fwalk is already an accepted, documented trade in this module (the #7011 deep-nesting test's own docstring names EMFILE as the handled OSError), the top-level-raise divergence from os.walk is caught into the same failure list, the Path.is_file() errno mirror matches CPython's ignore set, and the Windows fallback keeps status quo consistent with _FD_SAFE_DELETE's coarse path. Description and diff account for each other fully.

[DESIGN-REVIEWED] 651615d

[github-actions]

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

Premise-level review of 651615df6bdef685d8243f917582d6a6719bdd70 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 tree. Composing the final review.

First-Principles-Verdict: PASS

One reported wedge (#8724), one mechanism-level fix: the sole name-based walk still gating Empty-Trash goes descriptor-based, and every rider preserves the guard's documented fail-closed contract.

What this change ships

Intent: make Empty-Trash reclaim macOS batches whose nested paths exceed PATH_MAX, instead of silently wedging forever. FIX.

1. Deeply nested Trash batches on macOS become emptyable again — justified (reported defect macOS: deep Trash batches are permanently un-emptyable (PATH_MAX vs the name-based unlisted-files guard) #8724, platform limit)
2. Windows keeps the exact prior name-walk behavior — justified (fwalk undefined there; unconditional switch would crash)
3. Symlink-to-file and vanished-entry handling unchanged from Path.is_file() — justified (preserves documented guard semantics)
4. An unopenable batch still refuses with a reason, never crashes — justified (fwalk raises where walk used onerror)
5. Recursion overflow during the scan becomes a refusal, not a crash — rides along, undeclared; derived (module's prior escaped-RecursionError defect, test/test_session_storage.py:5091)
6. Two test doubles now intercept both walk mechanisms — justified (required by the availability-based pick)

Counts backing the verdict: os.walk|os.fwalk in src/kiro_crew/session_storage.py matches exactly 1 call site (line 1521, the one fixed), so zero unfixed siblings of the root cause in scope. No new public surface, config key, or flag — the change is internal to a private function. The nearest existing descriptor walk, skills._walk_confined_skill_fd (skills.py:553), swallows every OSError and never follows links — the opposite of this guard's raise-on-incomplete-scan contract — so it is not a duplicate mechanism.

[FIRST-PRINCIPLES-REVIEWED] 651615d

[jeeshofone]

Round 1 disposition — the unanimous Windows finding taken in c3d47f117

os.fwalk is Unix-only and my call was unconditional (GPT + Opus + Design Review, all blocking): FIXED, exactly as all three lanes prescribed. CPython defines fwalk only where {open, stat} <= os.supports_dir_fd, so on native Windows the loop header raised AttributeError — which no caller catches — turning every Empty-Trash, rollback check, and _discard_restored_batch into a crash where main works today. This was a regression I introduced and the Windows CI shards caught it directly.

The guard now capability-probes at call time (getattr(os, "fwalk", None)), matching the module's own _FD_SAFE_DELETE coarse-path discipline:

• fwalk available (POSIX): the descriptor walk from the previous round, unchanged — this is what fixes the macOS PATH_MAX wedge (macOS: deep Trash batches are permanently un-emptyable (PATH_MAX vs the name-based unlisted-files guard) #8724), including the top-level-open catch that keeps an unopenable batch a reason'd refusal.
• fwalk absent (Windows): the ORIGINAL os.walk body, byte-for-byte the pre-PR behavior. Windows never had the macOS 1024-byte wedge (its limits fail differently and main's behavior there is the status quo being preserved), so no new machinery is invented for a platform without the bug.

The two repointed test doubles are now platform-agnostic per Design Review's suggestion: each patches both walk and fwalk (the latter with raising=False), and since the production code selects the mechanism at call time by availability, the double fires on whichever path the platform takes.

First Principles CONCERNS (advisory): acknowledged, no change — the premise note doesn't request an edit.

Verification: full test/test_session_storage.py 229/229 on macOS (the fwalk branch), including the #7011 deep-nesting test that fails on main here; black gate, isort, flake8, mypy green. The Windows branch is the restored original code, exercised by the Windows CI shards this round.

[jeeshofone]

CI status note — the remaining red shard is main's own breakage, not this diff

Backend Tests (Windows) (3) fails in test_push_branch_gate.py / test_security.py (publish-gate arity assertions) — the known main-side semantic conflict from #7808, tracked in #8706/#8695. This PR touches only session_storage.py and its test file; my base commit is main's current head (6d1b51704, unchanged since), so there is nothing to rebase onto yet. I'll rebase and re-run once the push-gate fix lands on main.

Everything this PR is responsible for is green on the current head: all four AI lanes cleared (GPT, Opus, Design, First Principles), Windows shards 1/2/4 pass — confirming the restored os.walk fallback works where fwalk is absent — and the PR Hygiene "Pattern harvest" section has been added to the description.

[jeeshofone]

Round 2 disposition — the RecursionError premise is false on every supported Python, but the belt is taken (651615df6)

GPT blocking (deep batches abort with RecursionError from recursive os.fwalk): the premise doesn't hold on this package's Python floor — refined and taken as a belt, and I'm saying so. CPython's os.fwalk has been iterative since 3.11, and this package's requires-python is >=3.12 (also the only version CI runs). Verified empirically before pushing: on CPython 3.12.13 I built a 5,000-level directory tree via dir_fd-relative mkdir (plain mkdir can't — it hits PATH_MAX at depth ~482 on macOS, which is this PR's original bug) and os.fwalk traversed all 5,001 directories under the default 1,000-frame recursion limit without error. The "~1,000 nested directories → recursive fwalk → RecursionError" mechanism describes pre-3.11 CPython.

Taken anyway, as stated hardening rather than a bug fix: the guard's contract is that NO traversal failure escapes as a crash — an escaping exception here turns a refusal-with-reason into an Empty-Trash abort. So the fwalk wrapper now also catches RecursionError and converts it into the same failure list (→ SessionStorageError → SKIP_UNREADABLE), exactly GPT's first suggested fix. Two lines, no behavior change on any supported interpreter, and it future-proofs against an interpreter where the premise does hold. The code comment records that this is belt-only and why.

Verification: full test/test_session_storage.py 229/229; black, flake8, mypy green. The rebase onto c791f0f1d (push-gate fix) is unchanged in this amend — shard 3 and Coverage Gate should now be exercising the repaired main.

[bolichen97]

Tech Lead review — APPROVE

Filesystem-safety-sensitive change reviewed for TOCTOU and symlink-following. The fix is correct, minimal, and moves the guard in the safe direction.

The bug mechanism, independently reproduced. Built a 3000-level dir_fd-relative tree (6077-byte component path vs Linux PATH_MAX 4096): os.walk routes exactly one ENAMETOOLONG (errno 36) to onerror, which the guard's fail-closed contract converts to SessionStorageError → SKIP_UNREADABLE → the batch survives every Empty-Trash forever. os.fwalk + os.stat(name, dir_fd=rootfd) over the same tree: 0 errors, leaf file correctly classified. The 1024 vs 4096 limit is why this reproduces on macOS and not on Linux CI, but the mechanism is the same and it is now provably gone.

Symlink following: no regression, marginally stronger. os.fwalk's default is follow_symlinks=False (verified on the 3.12 floor this package requires), and CPython's _fwalk does not merely skip links — it lstats the name, opens it, then samestats the opened fd against that lstat before descending, so a directory swapped for a symlink between the two is refused rather than followed. Probed directly: a symlink-to-directory planted in a batch yields no entries from the linked target. That is a TOCTOU check os.walk does not have. Per-entry os.stat(..., follow_symlinks=True) deliberately mirrors the Path.is_file() it replaces, and it is classification-only — a symlink-to-file counts as unlisted (refusal, safe), a symlink-to-dir and a broken link do not, exactly as before. Nothing here unlinks, so nothing follows a link into a destructive operation.

Fail-closed contract preserved end to end. The ignored errno set {ENOENT, ENOTDIR, EBADF, ELOOP} matches CPython's pathlib._ignore_error exactly, so there is zero semantic drift from Path.is_file(); every other errno (notably EACCES, and ENAMETOOLONG itself) joins failures and keeps the batch. fwalk raising the top-level open error instead of routing it through onerror is caught into the same list, so an unopenable batch stays a reason'd refusal rather than an escaping OSError. EBADF in that set is unreachable inside the loop body (fwalk owns rootfd until the generator resumes) — a wash that mirrors the code it replaces, not a new hole.

The dangerous direction is under-reporting, and it is closed twice. I checked all three call sites (_discard_restored_batch, the no-sessions-staged rollback, empty_trash): the return value is used only as if leftovers: / len(leftovers). The Path objects are never handed to a filesystem call, so a >PATH_MAX textual path is pure string work and a mid-walk rename can only produce a false-positive refusal, never a wrong unlink. And removal still re-establishes the same "is anything here unaccounted for" question from the pinned descriptor via _scan_tree(batch_fd, device=…) plus an st_dev/st_ino identity match against the approval, so a guard under-report cannot by itself destroy data.

Scope and process. Two files, one private function plus its two test doubles; no new public surface, config key, or flag. os.walk|os.fwalk now matches exactly one call site in the module, so no unfixed sibling of the root cause is left in scope. Windows keeps the original name-based body byte-for-byte, which is right: fwalk is undefined there and Windows never had this wedge. The repointed doubles patch both mechanisms (raising=False for fwalk), so they still assert the guard's contract on whichever path the platform takes — strengthened, not weakened.

All checks green on 651615df, all four AI lanes PASS, zero unresolved findings. The one residual — GPT's nit that the code comment's "iterative" wording overstates _fwalk's yield from structure — is documentation-accuracy only, and moot because the wrapper catches RecursionError regardless of which reading is right. Squash-merging.