fix(system): bind the two sibling trash removals to descriptors

[chenmingwei23]

What is the problem?

#7011 bound the trash DELETE path to descriptors and named two sibling paths it left open.
Both were still removing a batch by re-resolved path:

• _discard_restored_batch, after a restore has moved every listed file back out
• move_to_trash's cleanup, when a staging attempt ends with nothing recorded

Both spelled it shutil.rmtree(batch, ignore_errors=True). A path is re-resolved component by
component by the kernel, so a directory above the trash swapped to a symbolic link after the
caller's own checks is followed, and the removal lands wherever the link points. Neither caller
holds the mutation lock across that window, every check above them is by path, and
ignore_errors=True means the deletion that lands outside the trash is also silent.

Why this issue matters to the user

The trash holds the only copy of a session's replay log, transcript and archive segments
between staging and an explicit empty. These two paths run on ordinary operations - every
restore, and every move that rolls back - so the exposure is not a rare corner.

Everything here runs as one uid, which in this product includes an agent acting on the user's
behalf. That is the actor the pinning defends against: not a remote attacker, but a process
that can write in the directories above the trash while the user's own restore is in flight.

How our fix solves it

Both siblings now remove by descriptor. They call
pinned_fs.remove_tree_pinned, which pins the parent chain one openat per component, opens
the batch through it with O_NOFOLLOW, scans it once, unlinks links and files against the
inodes that scan recorded, removes directories deepest-first through remove_dir_verified,
re-scans to decide whether the tree is actually empty, and only then removes the batch itself,
verified against the descriptor the whole operation was pinned to.

Pinning is the weaker half, so an identity carries the fix. Path.resolve() follows an
ancestor that is ALREADY a link, so a swap landing before the resolve produces a faithful
pinned walk to the wrong tree. Each caller therefore records the batch's (st_dev, st_ino)
before its own by-path read - at target.mkdir(...) under the mutation lock on the staging
path, and before the leftover scan on the restore path - and the approval hook compares the
pinned root's fstat against it as its first question. An unreadable identity refuses.

The content checks come second and cannot carry it. From ONE inode-verified read of ONE
file, the approval re-asks that the manifest's header claims this batch's own directory name,
and that NOTHING but the manifest remains - not "nothing unlisted", nothing at all. Both
callers arrive with the batch empty of files, so a file at a listed path is not the listed
file; it arrived afterwards and may be the only copy of whatever it is. An actor who can write
into the tree a swapped link points at can plant a matching header; an inode cannot be forged
by writing files.

The manifest goes last. list_trash() omits a batch with no readable manifest, so removing
it first and then failing on the directory would leave data on disk that nothing lists.
keep_until_empty names it: skipped by the file pass, required to be the only survivor of the
closing scan and verified there by inode, then moved aside under an unguessable debris name and
put back through put_back_no_clobber if the tree still will not go - link first, then an
O_CREAT | O_EXCL copy, because os.link in os.supports_dir_fd tests what the OS accepts and
not what the mount supports. The debris copy is RETAINED whenever the removal failed: no check
available before an unlink can prove the name it went back to still holds a good copy, so
removing the redundant one is a chance to turn a stuck batch into an unlistable one. The
retained name is reported through staged_name.

On a platform with no openat, the removal is bound to a verified name instead.
_stage_batch_by_name renames the batch aside inside the trash root, verifies (st_dev, st_ino) on the RENAMED directory against the caller's identity, and removes only the staged
name; a mismatch, an unreadable identity, or a tree that will not go puts the directory back
under the name the user saw. It is one owner for three callers - the explicit empty, which
already worked this way, and the two cleanups. Refusing outright instead is not available:
that branch is the whole of Windows, so refusing leaves a batch after every restore and every
rolled-back move, still listing sessions it no longer holds, which five pre-existing tests
read as a failure. The residual is stated rather than implied and tracked in #7566.

A refused cleanup writes nothing. The batch stays exactly as found, including a manifest
whose entries describe files the restore already moved out. That stale listing is a decided
residual: clearing it needs a write to the batch path, and there is nowhere safe to put one.
After the cleanup, every refusal is itself evidence the path may be redirected, and
atomic_write REPLACES its destination. Before it, the full-restore branch has no write at
all today, so adding one hands that primitive a path a batch swapped to a link points wherever
an actor chose, including another batch's manifest, whose sessions would become unlistable. A
stale listing is visible and reversible; that is not, and empty_trash clears the batch on the
user's own say-so.

One owner per mechanism. rename-verify-remove was spelled inline three times - the
interior directories, the batch directory, and the coarse path - and is now
pinned_fs.remove_dir_verified, with scan_tree_pinned, open_verified_chain,
drain_verified_chain, remove_tree_pinned and put_back_no_clobber beside it.
session_storage keeps the policy: which map authorises a removal, what a refusal means to
the user, and how it is worded. Per-call-site respelling of this mechanism is what pinned_fs
was created to end after #2446 and #2447.

Riders, same root cause. The delete path's manifest recovery trusted a mutable name: it
opened the debris without O_NOFOLLOW, linked with the default follow_symlinks=True, and
unlinked unconditionally, on a name in a directory an actor may be able to write to, reached
only after something already failed. Its source is now opened O_NOFOLLOW and inode-checked
before anything is read, the copy reads that descriptor rather than re-opening the name, the
link passes follow_symlinks=False and checks what landed, and both debris unlinks are
identity-checked. Leaving this out would have shipped a shared put-back owner whose delete-path
caller kept the unguarded spelling of the same hole.

_read_manifest gains expect_ino, checked with fstat on the open handle, so a caller that
needs the header and the listing to describe ONE file makes a single call - two calls are two
opens, and a manifest rewritten between them lets the header check pass on one file while the
listing comes from another. The approval also refuses a manifest that is not valid UTF-8:
UnicodeDecodeError is a ValueError, so the except OSError that turns an unreadable
manifest into a refusal does not cover it, and an escape would abort a restore that had already
moved the user's files back.

What tests we did

test/test_trash_pinned_sibling_removal.py is new and covers both siblings end to end. The
ancestor swap is set up ON DISK rather than simulated, and the victim carries a file, so a
passing test says the file survived rather than that a branch was taken:

• a swapped ancestor is refused on both paths, and the victim's file is intact
• a victim holding a manifest FORGED to name the selected batch is still refused, because the
  identity was captured before the swap
• an identity that could never be established refuses rather than proceeding
• a clean batch is still removed, and a batch holding an unlisted file is kept
• a file at a listed path is kept, not deleted
• the manifest survives a tree that will not go, and comes back when the batch directory will
  not go, with the redundant copy retained and reported
• the put-back falls back to a copy where the filesystem has no hard links, refuses a swapped
  source name, and removes a partial copy only while it is still the file this call created
• a manifest that is not valid UTF-8 refuses instead of raising
• on the descriptor-less platform the batch is removed through a verified staged name, a
  directory that is not the selected one is put back with its contents intact, and a swapped
  ancestor reaches no victim

Every security assertion is mutation-verified: reverting the guard reddens the test, each on a
victim file that no longer exists or a credential that was copied.

The descriptor-less branch is unreachable on Linux, so it was run locally against a forced
coarse platform in both directions - the five pre-existing Windows tests pass with the staged
removal and fail with a refusal - and then confirmed on the Windows shards in CI.

Gates: black, flake8, isort, mypy clean on touched files; docs_lint passes; the spec
in docs/system-specs/modules/session-storage.md moves with the code in the same commit.

Any other suggestions on the work?

Accepted residuals, each named rather than implied. The descriptor-less platform still
resolves the staging path, so an actor who can observe that name inside the window can redirect
the removal through an ancestor swapped afterwards, and _identity_of is itself a by-path
stat that a swap already in place can fool. Both are tracked in #7566, which describes the
handle-based deletion that would close them. Neither is a regression: on mainline these paths
are a bare shutil.rmtree(batch, ignore_errors=True) with no identity check at any point, and
_identity_of does not exist there at all.

Deferred, with a reason. _delete_listed_files still drives its own move-aside-remove
choreography rather than calling the primitive. Converging it means carrying freed-byte
accounting, the cleared flag behind SKIP_INCOMPLETE, and a progress callback into a module
that holds no policy by design, on a path that only just converged in #7011.

remove_tree_pinned has one consumer today, serving both fixed sites. The alternative is to
inline the whole pinned walk back into session_storage, which is the respelling pinned_fs
exists to end.

Pattern harvest

Rule candidate: extend the AST ratchet in test_pinned_staging.py to session_storage.py,
banning shutil.rmtree and by-name os.rmdir/os.unlink inside any function that holds a
directory descriptor.

The defect class is "a filesystem mutation that addresses a PATH inside a function that already
holds, or could hold, a descriptor for the object". test_pinned_staging.py already ratchets
exactly this for pinned_fs.py by walking the AST - and it is the reason this PR's own moved
code was caught rather than shipped. session_storage.py is not covered, which is why these
two rmtree(path) calls survived #7011: nothing mechanical would have named them.

It cannot be done here as a clean gate: session_storage.py has legitimate by-name sites - the
coarse branch and the by-path pre-screens - and separating them needs the per-line marker
convention that the existing ratchet's own docstring identifies as the blocker for widening to
snapshot.py. That is one change serving three modules rather than three narrow ones.

Not a one-off: this is the third time this exact class has been closed in this module (#2446,
#2447, #7011), and each round found it at a site the previous round's review had not looked at.

Closes #7113

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Real TOCTOU hole on a named boundary (same-uid agent vs. trash), closed with the mechanism the module already owns, residuals disclosed and tracked.

Suggestions

• The ratchet extension to session_storage.py is the only recurrence-prevention here and lives solely in the PR description as a "rule candidate" — unlike the coarse-platform residual (Windows session-trash removal and manifest recovery have no descriptor-bound path #7566), nothing tracks it; file the issue, since this class has now recurred three times past human review and only the AST gate caught the moved code this round.

[DESIGN-REVIEWED] 87348df

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 87348df

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

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

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 87348df3fa130a33bdf743bf5883c079e68149b5 — 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 mechanical checks are done. Consumer counts, sibling counts, and the existing-mechanism check all verified against the repo at HEAD. Final review:

First-Principles-Verdict: PASS

Both siblings #7011 named are fixed at cause level — removal bound to a pre-captured inode instead of a re-resolved name — with every residual declared and test-pinned.

What this change ships

Intent: stop a restore or rolled-back move from deleting outside the trash through a swapped ancestor — a FIX.

1. Restore's batch cleanup refuses a swapped ancestor instead of following it — justified (named boundary: same-uid agent).
2. Rolled-back move's cleanup gets the same containment — justified, the second named sibling.
3. A refused cleanup leaves the batch untouched; its listing goes stale (manifest rewrite deleted) — declared, subtractive, pinned by test.
4. A batch holding any file or link is now kept, not partially removed — declared.
5. Windows: both cleanups reuse the explicit empty's rename-verify-remove — declared; residual tracked (Windows session-trash removal and manifest recovery have no descriptor-bound path #7566).
6. Three inline spellings of rename-verify-remove collapse into pinned_fs.remove_dir_verified — subtractive; 3 real consumers counted.
7. Delete-path debris recovery hardened (O_NOFOLLOW + inode checks) — declared rider, same root cause.
8. _read_manifest(expect_ino=) + UTF-8 refusal — declared rider; 1 consumer, optional keyword, not generalized.
9. New pinned_fs exports — every symbol has ≥1 non-test consumer (grepped remove_tree_pinned|remove_dir_verified|put_back_no_clobber|scan_tree_pinned|open_verified_chain in src/).
10. By-name ratchet exempts entry-named receivers — rides along, backstopped by a behavior test.

Watch

• keep_until_empty has exactly 1 consumer, always passing MANIFEST_NAME (session_storage.py:1293 in the diff). Kept because hardcoding the manifest name in pinned_fs breaks the documented mechanism/policy split — but it is a one-consumer generalization until a second index-shaped tree arrives.
• Remaining shutil.rmtree in session_storage.py (lines 1596, 2955) are the coarse-platform residual the description declares and tracks in Windows session-trash removal and manifest recovery have no descriptor-bound path #7566, not undeclared siblings.

Subtractions

• Shrink the ratchet exemption in test/test_pinned_staging.py: any receiver named entry/_entry now bypasses the by-name check for every method; limit it to the three os.DirEntry methods the scan uses (is_symlink, is_dir, inode) so an unrelated entry.unlink() still trips it.

[FIRST-PRINCIPLES-REVIEWED] 87348df

[github-actions]

GPT 5.6 Review — ✅ human override accepted

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

[chenmingwei23]

Every item actioned on head 32d63863e - no rebuttals, all four were real.

Blocker, fixed. The duplicated logger.warning in _remove_emptied_batch was residue from repairing an earlier bad splice in this same function: the first draft had spliced the coarse-platform branch onto the refusal branch, so the removal warned and then fell through to shutil.rmtree anyway. My own reproduction test caught that half and I removed the dead tail, but not the duplicated warning above it. Second call deleted.

Worth recording why that matters more than a stray log line: the function it landed in is the one whose whole job is to not remove by path, and the splice had it doing exactly that. Reading it as "adjacent identical calls, delete one" was the right read of the diff.

Watch item, fixed rather than noted. pinned_fs.SKIP_UNREADABLE = "unreadable" next to the pre-existing session_storage.SKIP_UNREADABLE = "unreadable_batch" is one name for two values, and both reach the same outcome.reason in the same log line. The pinned module's four codes are now named for what they describe - REMOVAL_IDENTITY_CHANGED, REMOVAL_UNVERIFIABLE, REMOVAL_FAILED, REMOVAL_STAGE_FAILED - so neither the names nor the values can be confused with the consumer's skip codes. The docstring states why the SKIP_ prefix was not reused.

All three subtractions taken.

• StagedRemoval.restored - dropped. It carried the same fact as staged_name is None, which makes it a second place for one fact to be wrong. The test that asserted it now asserts the staging name, which is what a caller actually reads.
• PinnedTree.empty - dropped, along with the one test assertion that used it.
• scan_tree_pinned(prefix=) - dropped. Two of the moved test doubles did reference prefix, using prefix == () to fire their swap on the first scan; they now use an explicit one-shot latch, which is what they meant and what the three sibling doubles in the same file already did. Same window under test, same assertions.

Item 7 in your ship list was the duplicate; items 8-10 are the subtractions. Nothing on that list is left undeclared.

Targeted suites green on the new head: test_trash_pinned_sibling_removal.py + test_session_storage.py + test_pinned_staging.py, 293 passing. black / flake8 / isort / mypy clean on touched files.

The PR Hygiene red was unrelated to the code: the gate wants a literal Rule candidate: line and my Pattern harvest section opened with a bolded variant. Fixed in the body, which now also records these subtractions.

[chenmingwei23]

Both blocking findings were real and both were introduced by this diff. Fixed on head 75afed2ce; no override requested.

1. The manifest was removed before the tree, so a survivor left the batch unlistable.

Correct, and it is a regression against a property the delete path already had. remove_tree_pinned unlinked every scanned file - the manifest with them - and only then removed the directories. If anything then survived (a file arriving after the scan, a directory that would not go), the closing scan reported removed=False and the batch stayed on disk with its manifest already gone. list_trash() omits a batch with no readable manifest, so that is data on disk that is neither visible nor restorable, reported as a partial success.

_delete_listed_files avoids exactly this by moving the manifest aside instead of unlinking it, and putting it back if the rmdir fails. I did not carry that across when writing the new primitive.

The fix generalises the property rather than special-casing the manifest: remove_tree_pinned takes keep_until_empty, a top-level file to remove LAST. The name is skipped by the file pass, the closing scan must find nothing but it - verified by INODE against the first scan, so a file substituted at that name is not accepted and destroyed - and only then is it moved to the parent under an unguessable debris name, the tree removed, and the debris unlinked. If the tree still will not go, the entry goes back with os.link rather than rename, because rename replaces its destination silently and this direction must not clobber a file that arrived at that name in the interval; the debris is unlinked only after the link succeeds, so a failed put-back leaves the entry reachable under the debris name rather than nowhere. keep_until_empty also refuses up front on a platform with no descriptor-relative os.link, since without it there is no safe way back.

The concept is not "manifest" - it is "the entry that makes this tree discoverable" - which is why it is a parameter of the primitive and not a branch in the caller.

2. The approval read the manifest twice, so a replacement between reads could forge the listing.

Also correct, and the worse of the two: the header check came from _summarize_manifest and the rels from _manifest_rels, two separate opens. A manifest replaced in between passes the header check on the first file while the listing that decides which staged files may be deleted comes from the second - so a forged listing naming an unlisted staged file would have had the removal delete its only copy. That is the exact protection the unlisted-file rule exists to provide, defeated by the read pattern.

Now one call. _read_manifest takes expect_ino and checks it with fstat on the OPEN handle - not a stat of the name - so there is no interval between the check and the read at all, and the inode compared against is the one the pinned scan recorded. The header and the listing both come from that single parse. A batch with no manifest in the scan is refused outright, since nothing then vouches for it being the selected batch.

Tests, mutation-verified. Four new cases, and reverting each fix turns them red:

• a staged file that will not unlink leaves the manifest in place (batch stays listable)
• a batch directory that will not go gets its manifest linked back, with no debris left in the trash root
• the primitive keeps its deferred entry when the tree survives, and removes it with the tree when the tree goes
• a manifest whose inode is not the scanned one is refused, while the real one still passes - so the check binds rather than always refusing

Removing keep_until_empty reddens the first two; removing expect_ino reddens the fourth.

Targeted suites on the new head: test_trash_pinned_sibling_removal.py + test_session_storage.py + test_pinned_staging.py, 299 passing. black / flake8 / isort / mypy clean on touched files; docs_lint passes.

One follow-up I am deliberately not doing here: _delete_listed_files still spells its own manifest-aside-and-restore inline, and it is now the same shape as keep_until_empty. Folding it onto the primitive is a real simplification but it would mean moving byte-progress reporting and the O_CREAT | O_EXCL copy fallback into pinned_fs in the same change, on a path that is already converged. Named in the PR body instead.

[chenmingwei23]

CONCERNS dispositioned on head b4349d773. The duplication finding was right, and chasing it found a real hole in my own code rather than just an aesthetic one.

Taken, in the half that mattered. The two spellings were not equivalent. The delete path's put-back is os.link with an O_CREAT | O_EXCL copy fallback, and its own comment says why the fallback exists: os.link in os.supports_dir_fd tests whether the OS accepts dir_fd, not what the MOUNT supports, so a filesystem without hard links passes the probe and then refuses the call. My _remove_with_deferred_entry had link only, and I had "guarded" it with exactly that probe - so on a link-less filesystem the put-back would fail at runtime and leave the manifest as debris in the trash root, making the batch unlistable. That is precisely the loss keep_until_empty exists to prevent, and the duplication was hiding it.

So the recovery half now has one owner: pinned_fs.put_back_no_clobber (link first, O_CREAT | O_EXCL create-and-copy second, PUT_BACK_NAME_TAKEN when something else holds the name). _copy_back_exclusive is deleted from session_storage and the delete path's inline link/copy branching collapses onto the shared call, keeping its own two log lines. The bogus platform guard on keep_until_empty is gone, since there is now a path that needs no hard-link support.

Two tests pin it: the copy fallback with os.link raising EOPNOTSUPP (manifest back, no debris left), and a no-clobber refusal where a file arrived at the name while the entry was out. Mutation-verified - making the put-back link-only reddens the first.

Deferred, with the reason stated rather than implied. The remaining duplication is the orchestration around that recovery, and replacing _delete_listed_files's block with _remove_with_deferred_entry outright is not the pure subtraction it looks like. That block also counts the manifest's bytes into freed, drives the cleared flag that decides SKIP_INCOMPLETE, and reports byte progress through a callback. Folding it in means moving progress accounting and skip-code policy into pinned_fs, which holds no policy by design - so it is a redesign of the primitive's contract, on the path that just converged in #7011, and it belongs in its own change. What is shared now is the part that had a correctness difference; what is left is bookkeeping that differs for real reasons.

On keep_until_empty having one consumer: agreed, and I would rather say that plainly than argue it is general. It is one consumer today. The alternative was a manifest-shaped parameter in a module that must not know what a manifest is, or the ordering spelled at the call site - a fourth respelling of the thing this PR exists to stop. It becomes real generality when the delete path is consumer #2, which is the follow-up above.

Targeted suites on the new head: 301 passing across test_trash_pinned_sibling_removal.py, test_session_storage.py, test_pinned_staging.py. black / flake8 / isort / mypy clean on touched files; docs_lint passes.

[chenmingwei23]

Round on ca5d9de6f -- fixed on 5b707cc7a

GPT 5.6 BLOCKING -- session_storage.py:2383, full restores rewrite through a replaceable path: FIXED, remedy taken as prescribed

Correct, and it lands on a change I made LAST round in response to the previous blocking
finding, so it is worth stating the whole arc plainly. The manifest tidy-up has now been tried
in both positions and neither is safe:

position	why it was tried	why it fails
AFTER the cleanup, gated on path_verified	keeps the batch honest once it is kept	the descriptor is closed by then, so a swap redirects atomic_write (blocked last round)
BEFORE the cleanup, unconditional	moves the write to where the partial branch already writes	this branch had NO write before, so it ADDS one; a batch swapped to a link sends it to another batch's manifest, whose sessions become unlistable

So the tidy-up is not done at all, which is the remedy this round asked for. The conditional is
restored: an empty remaining goes straight to the descriptor-bound cleanup, and neither the
cleanup nor the full-restore branch writes to the batch. The partial branch keeps its
pre-existing rewrite -- it has to record what is still staged, and it is not reached through a
refusal.

The cost is a declared residual, now stated in the code and the spec rather than left implied:
a batch the cleanup declines to remove still lists the sessions the restore moved out. That
is untidy and it is visible and reversible, which the write is not. empty_trash still takes
such a batch -- I did not assert that from reading, the test performs the empty and checks the
batch is gone.

test_a_batch_the_cleanup_keeps_lists_no_phantom_sessions is replaced by
test_a_full_restore_never_writes_to_the_batch_it_is_leaving, which patches atomic_write to
fail the test outright, pins the stale listing as the accepted outcome (so clearing it later is
a deliberate change), then empties the trash. Mutation-verified: restoring the unconditional
rewrite reddens it on the write assertion.

First Principles CONCERNS (advisory) -- all three subtractions taken, one with a correction

TreeRemoval.error -- DROPPED. Zero readers, confirmed. I considered the other direction
(log the errno in the caller's warning) and rejected it: inventing a consumer to justify a
field is backwards, and the module's convention is that the exception rides along only where
the caller RE-RAISES it, which is why StagedRemoval keeps its own. That reasoning is now in
the class docstring so the asymmetry does not read as an oversight.

approve non-Optional -- DONE, and the is not None branch is gone. Last round I argued a
caller with nothing to check was a real case; no such caller exists, and leaving the parameter
nullable only makes the ask-nothing mode reachable by omission. Tests that meant "approve
everything" now pass a named _approve_anything hook, so each call states which mode it
exercises.

_open_chain -- the wrapper is gone, the NAME stays as an alias. The subtraction is right
about the body: it added no check, no default, no error translation, and it is now
_open_chain = pinned_fs.open_verified_chain. Deleting the name outright would have cost
something the grep did not surface, though: test_session_storage.py patches
session_storage._open_chain to swap a directory inside the removal window
(test_a_directory_swapped_after_the_scan_keeps_its_files), and that seam is deliberately
narrow -- patching pinned_fs.open_verified_chain instead would also hook the pinned removal's
own internals, widening the double past what the test is about. The four call sites and two
main-owned test doubles now pass the upstream keyword-only arguments.

Watch item -- single-consumer knobs on remove_tree_pinned

Accepted as stated. keep_until_empty is forced by the mechanism/policy split and refusal
follows the module's standing convention; approve's speculative half is exactly what the
subtraction above removed, so the remaining single-consumer surface is the two that earn it.

Verification: 308 passing across test_trash_pinned_sibling_removal.py,
test_session_storage.py, test_pinned_staging.py. black/flake8/isort clean on the four
touched files, mypy clean on both source files (its 2 errors remain pre-existing in
transcribe.py), docs_lint passes.

[chenmingwei23]

Windows regression fixed on aa7095615 -- and it reverses a remedy I took two rounds ago

Auditing the completed runs on the superseded head found five FAILURES on
Backend Tests (Windows) (3) that I had not read, all in pre-existing tests, all mine:

test_session_storage.py::TestRestoreIsAllOrNothing::test_restores_every_half
test_session_storage.py::TestRestoreIsAllOrNothing::test_a_failed_restore_stays_retryable
test_session_storage.py::TestRestoreIsAllOrNothing::test_an_unstattable_file_aborts_the_whole_session
test_session_storage.py::TestManifestPersistenceFailure::test_a_session_that_cannot_be_recorded_is_put_back
test_session_storage.py::TestPartialMoveRollsBack::test_a_session_that_cannot_move_wholly_is_left_alone

All five read assert [TrashBatch(...)] == []: the batch was still in the trash after a
restore or a rolled-back move. That is the coarse-platform refusal I added at GPT's request
in an earlier round (REFUSE when there is no descriptor to bind a removal to). I accepted
that remedy on the reasoning that these two cleanups are not user-requested, so the safer
half of the trade came free.

It does not come free, and this is the evidence: that branch is the WHOLE OF WINDOWS. Refusing
there leaves a batch behind after every restore and every rolled-back move -- still listing the
sessions it no longer holds, so the next restore attempt fails its own staged-file check. Five
tests read that as a failure and so would a user. My local runs are Linux, where
_FD_SAFE_DELETE is True, so nothing I ran could see it.

The fix is the mechanism the neighbouring path already uses on that platform, not a revert
to rmtree(path). _stage_batch_by_name renames the batch aside inside the trash root,
verifies (st_dev, st_ino) on the RENAMED directory against the identity the caller captured,
and only the staged name is removed; a mismatch, an unreadable identity, or a tree that will
not go puts the directory back under the name the user saw. The delete path's inline copy is
now a caller of it, so the descriptor-less platform has ONE owner for three call sites instead
of a second spelling.

The residual is stated rather than implied, and it is the one this platform already accepts for
an explicit empty: the removal still resolves the staging path, so an actor who can OBSERVE
that name inside the window can redirect it through an ancestor swapped afterwards. It is
bounded by an identity check a same-named impostor cannot pass -- test_no_victim_is_reachable_without_descriptors
sets the swap up on disk and confirms the victim's file survives, and a new test confirms a
non-selected directory is put back with its contents intact.

Verified rather than inferred, since CI cannot reach this branch on Linux: I ran the five
Windows-failing tests locally against a forced coarse platform. They pass with the fix, and
with the refusal restored all five fail with the byte-identical CI assertion
(assert [TrashBatch(...)] == []).

Two coarse-platform tests that asserted retention are inverted accordingly, and two of my own
tests that used the coarse platform as a cheap way to force a refusal now use a real one (the
batch directory refusing to go), because the coarse platform no longer refuses.

Backend Tests (3.10, 3) / Windows census failure is MAIN-OWNED

Same red on both shards, and it is not this PR:

New gate-side log/audit line(s) reading the BASELINE redactor
dashboard/handlers/memory.py: 2 sites, census says 0

Evidence: my commit touches neither file (git show --name-only HEAD), my copy of
test_security_posture.py is byte-identical to origin/main, and on fresh origin/main the
_BASELINE_LOG_SITE_CENSUS dict has NO entry for dashboard/handlers/memory.py while main's
own memory.py carries gate-side log sites. So main's tree fails its own test and every open
PR inherits it through the merge ref -- the same shape as the files.py census that #7492
fixed earlier today. Not folding a fix in; a re-run cannot help either, since a replay re-tests
the same base snapshot. I will rebase once the main-side fix lands.

309 passing across the three targeted suites; black/flake8/isort/mypy clean on touched
files, docs_lint passes, spec updated (it described a coarse path that removes nothing).

[chenmingwei23]

Round on aa7095615 -- one finding fixed, one escalated to the maintainer

FINDING (non-blocking) -- stale docstring on _discard_restored_batch: FIXED on ca3520fc0

Correct and mine. The docstring still claimed "the caller has already cleared the manifest's
entries", which was true for exactly one round -- the hoisted rewrite that the previous round
asked me to revert. It now states the actual behaviour: nothing writes, the listing goes stale,
and that is a decided residual with empty_trash as the way out.

BLOCKING -- "descriptor-less cleanup remains path-traversable, refuse when _FD_SAFE_DELETE is false": ESCALATING, not fixed

The finding is technically accurate. I am not implementing the prescribed remedy, because I
implemented it two rounds ago at this lane's request and it is disproven by the repository's own
tests. Both halves matter, so here is the full picture.

The remedy breaks five pre-existing tests. _FD_SAFE_DELETE false is the whole of Windows.
Refusing there leaves a batch in the trash after EVERY restore and every rolled-back move,
still listing sessions it no longer holds, so the next restore fails its own staged-file check:

test_session_storage.py::TestRestoreIsAllOrNothing::test_restores_every_half
test_session_storage.py::TestRestoreIsAllOrNothing::test_a_failed_restore_stays_retryable
test_session_storage.py::TestRestoreIsAllOrNothing::test_an_unstattable_file_aborts_the_whole_session
test_session_storage.py::TestManifestPersistenceFailure::test_a_session_that_cannot_be_recorded_is_put_back
test_session_storage.py::TestPartialMoveRollsBack::test_a_session_that_cannot_move_wholly_is_left_alone

All five failed on Backend Tests (Windows) (3) for head ca5d9de6f with
assert [TrashBatch(...)] == []. Verified locally in both directions against a forced coarse
platform: they pass with the staged removal, and restoring the refusal reddens all five with
the byte-identical assertion.

The residual is pre-existing and this PR narrows it. Measured against origin/main:

	origin/main	this PR
sibling A (_discard_restored_batch)	shutil.rmtree(batch, ignore_errors=True), line 1380	rename aside in the trash root, verify (st_dev, st_ino) there, rmtree the staged name, put back on mismatch
sibling B (move_to_trash cleanup)	shutil.rmtree(target, ignore_errors=True), line 1936	same
empty_trash coarse path	rename-verify-remove, residual documented and accepted	unchanged, now sharing one owner

On main the attack needs no window and no name: the bare rmtree follows a swapped ancestor
by default, and ignore_errors=True makes it silent. On this branch it needs the random
staging name AND a swap landing after the identity check. The exposure GPT names is therefore
strictly narrower than base, not introduced here -- and it is the same residual main already
documents in _delete_listed_files for this platform, in a comment that says so explicitly.

The mechanism space on this platform is exhausted. Three positions have now been through
this lane:

round	position	outcome
1	rmtree(path) (main's behaviour)	blocked -- the original defect
2	refuse when no descriptor	taken, then disproven by five Windows tests
3	rename-verify-remove, as empty_trash does	blocked now, as path-traversable

There is no fourth option in Python: Windows has no openat, and a handle-based delete would
need ctypes against NtSetInformationFile/FILE_FLAG_BACKUP_SEMANTICS with reparse-point
handling -- a new platform mechanism, the right long-term fix, and plainly its own change
rather than a fourth patch inside a PR whose subject is the descriptor-capable path.

What I need from the maintainer. I will not post /ai-review override on my own
authority. The disposition I am asking to record is "pre-existing platform limitation,
measured delta strictly narrower than base, not applicable to THIS PR", with the Windows
mechanism tracked as a separate issue. The alternative that needs no override is to descope
Windows from this PR entirely -- keep the descriptor path, leave both siblings on main's
rmtree(ignore_errors=True) where _FD_SAFE_DELETE is false -- which satisfies the letter of
this lane's requirement (this PR adds no by-path removal) while leaving Windows exactly as
insecure as it is today. I think that is the worse product outcome and am not doing it
unasked.

309 passing across the three targeted suites on ca3520fc0; gates and docs_lint clean. The
other red on this head, the dashboard/handlers/memory.py log-site census, is main-owned with
evidence in my previous comment.

[chenmingwei23]

Round on ca3520fc0 -- two new blockers fixed on 2f7dfc5c7, the third still escalated

BLOCKING -- pinned_fs.py:1723, recovery can delete its last valid manifest: FIXED

Real, and the sharpest kind of finding: the unlink was identity-checked, but on the WRONG side.
_unlink_verified(parent_fd, debris, ...) proves the debris is still the file that was staged;
it says nothing about whether the name the manifest was just put back to still holds a good
copy. A file replaced at the entry's name after the put-back leaves the debris as the last
readable manifest, and unlinking it turns a batch that is merely stuck into one list_trash
does not offer and nothing can restore -- precisely the loss the deferral exists to prevent.

The debris is now RETAINED whenever the directory removal failed, and reported: staged_name
carries the directory's staging name when there is one (the whole tree sits under it, so it is
the more urgent thing to name) and otherwise the debris, so no retained copy is silent. The
success path keeps its unlink, which is not optional there -- the tree is gone, so the copy
must go with it.

The cost, stated rather than implied: one small named file in the trash root per stuck cleanup.
It is a dotfile and not a directory, so list_trash ignores it, and the log line names it.

I considered the tighter alternative -- have put_back_no_clobber report the inode it landed,
so the debris could be unlinked whenever the destination is provably the same file -- and
rejected it. It adds a second meaning to that function's return value to buy tidiness on an
already-degraded path, in a module the First Principles lane has just asked me to shrink.

Two of my own tests asserted "no debris left behind" and now assert the retention plus the
content of the retained copy. Mutation-verified: restoring the unlink reddens both.

BLOCKING -- session_storage.py:1471, invalid UTF-8 escapes the cleanup refusal: FIXED

Also real, and specific to the read this PR adds. _read_manifest reports an unreadable
manifest as None but decodes strictly, so non-UTF-8 bytes raise UnicodeDecodeError -- a
ValueError, which its own except OSError does not cover and nothing between the approval
hook and restore() catches. The pinned re-read exists exactly to meet content the by-path
pre-screen never saw, so it is the one read that can hit this, and on the restore path an
escape aborts AFTER the user's files have already been moved back.

Caught at the call site as prescribed, returning SKIP_UNREADABLE: a manifest that cannot be
decoded cannot identify the batch, so it withholds the removal like any other unreadable one.
New test writes real invalid bytes and asserts a refusal with the batch intact;
mutation-verified -- without the guard the test fails with the raw UnicodeDecodeError.

Scope note: the same decode hole exists on origin/main in _summarize_manifest and in
_read_manifest's pre-existing callers. I am not widening the diff to cover them; that is its
own change.

BLOCKING -- descriptor-less cleanup path-traversable: UNCHANGED, awaiting the maintainer

Third raise of this one, and my position is in the previous comment with the evidence: the
prescribed remedy (refuse when _FD_SAFE_DELETE is false) was implemented two rounds ago and
five pre-existing Windows tests disprove it, while the residual named here is strictly narrower
than origin/main, where both sibling paths are a bare shutil.rmtree(batch, ignore_errors=True) on every platform. I am not re-litigating it here and I will not post an
override on my own authority. It needs a maintainer decision between recording the disposition
and descoping Windows from this PR.

310 passing across the three targeted suites; black/flake8/isort/mypy clean on touched
files, docs_lint passes. The dashboard/handlers/memory.py census red remains main-owned --
still no entry for it in _BASELINE_LOG_SITE_CENSUS on fresh origin/main.

[chenmingwei23]

Round on 2f7dfc5c7 -- last round's two blockers are gone; the remaining one adds one sub-claim

Both blockers from the previous head are fixed and no longer raised. What is left is the fourth
raise of the descriptor-less finding, and it now includes one mechanism I had NOT addressed, so
here is that half specifically rather than a repeat of my earlier reply.

New sub-claim: "ancestor swap BEFORE identity capture", at identity = _identity_of(batch).
Accurate as stated. _identity_of is os.stat(..., follow_symlinks=False) on a path whose
ancestors the kernel re-resolves, so a swap already in place when it runs captures the VICTIM's
inode, and every later check then agrees with itself about the wrong directory.

Two things follow, and neither changes the disposition:

1. It cannot be closed from inside this function. Pinning the capture does not help -- pin_parent
   resolves the parent path too, so an ancestor that is ALREADY a link is followed identically.
   Distinguishing "this ancestor was always a link" from "this is the real path" needs an anchor
   established before the operation began, and the restore has none: it is by-path throughout on
   main, so the only real fix is pinning the whole restore, which is a different change from the
   one this PR is.
2. Reaching it requires the victim tree to have already served as the restore's SOURCE. The
   restore read that tree's manifest and moved every listed file out of it into the session
   stores. An attacker who can arrange that has already achieved content injection into the
   user's live sessions; the subsequent removal of the now-empty directory is not the damage.

And the delta, which is the load-bearing part: _identity_of does not exist on origin/main --
zero occurrences. On main this same path is shutil.rmtree(batch, ignore_errors=True) with no
identity check at any point and errors suppressed. So the sub-claim names a check that main does
not have, in a path this PR is the first to guard at all.

Status unchanged, and I am not re-litigating the rest. The prescribed remedy ("keep the batch
when no descriptor-bound removal is available") is the one five pre-existing Windows tests
disprove -- evidence in my earlier comment, verified in both directions locally. I will not post
an override on my own authority.

I have recorded the residual locally as f-20260901-10 rather than filing an issue for it now,
since which issue is the right one depends on the decision: if the disposition is recorded, that
entry gets promoted to a tracked issue for handle-based deletion on Windows
(NtSetInformationFile / FILE_FLAG_BACKUP_SEMANTICS with reparse-point handling); if Windows is
descoped from this PR instead, it becomes the issue describing what was descoped.

310 passing across the three targeted suites on this head; gates and docs_lint clean. The
dashboard/handlers/memory.py census red is still main-owned and still unfixed on fresh
origin/main.

[chenmingwei23]

Residual now tracked: #7566

The descriptor-less Windows removal path is filed as
#7566 -- handle-based deletion
(FILE_FLAG_BACKUP_SEMANTICS + FILE_FLAG_OPEN_REPARSE_POINT, identity verified on the
handle via GetFileInformationByHandle, delete through the handle rather than the name).
It records both residuals this PR leaves on that platform: the removal still resolving the
staging path, and _identity_of being a by-path stat that a swap already in place can
fool.

It also records, because it is the part that decides scope, that this is not a regression
introduced here: on mainline both sibling cleanups are
shutil.rmtree(batch, ignore_errors=True) with errors suppressed and no identity check at
any point -- _identity_of does not exist there at all. This PR is strictly better than
base on Windows, and #7566 is what finishing the job looks like.

Next on this PR: #7554 (the dashboard/handlers/memory.py log-site census, the one red
here that is main-owned) is still open. Once it merges I will rebase onto it so the census
red clears on a fresh merge ref, and let all lanes re-roll on the rebased head. No push
before then, and no override on the current head -- a rebase would void a SHA-pinned
marker anyway.

[chenmingwei23]

Windows regression confirmed fixed by CI, and First Principles' three Watch items dispositioned

The Windows fix is verified in the environment, not just locally

Backend Tests (Windows) (3) on 2f7dfc5c7 now fails on ONE test, and it is the main-owned
census (test_security_posture.py::TestGateSideLogRedactorSpelling). The five
test_session_storage.py failures that the earlier coarse-platform refusal caused are gone:

before (ca5d9de6f): 5x test_session_storage + 1x census   -> refusal left a batch after every restore
now    (2f7dfc5c7): census only                           -> staged removal completes

Windows shards 1, 2 and 4 are green, as is the Windows installer build. That is the branch
Linux CI cannot reach, so it is the reading that matters for that fix.

Opus 4.8 reports no blocking findings on this head, having validated and dropped its one
candidate on the merits.

First Principles -- CONCERNS (advisory), three Watch items

1. remove_tree_pinned has one consumer, and ships as public surface ahead of a second:
ACCEPTED, declared. The observation is right and I am not adding a second consumer to
justify it. Inlining it back into _remove_emptied_batch is the alternative, and it would put
the whole pinned walk -- parent chain, O_NOFOLLOW open, scan, verified chain-opens,
deepest-first directory removal -- inside session_storage, which is the respelling pinned_fs
exists to end. The one consumer serves BOTH fixed sites.

Worth noting that I apply this rule symmetrically rather than only when it favours keeping
code: when GPT's remedy dropped _stage_for_coarse_removal to a single consumer earlier in
this review I reverted that extraction back inline, and when the Windows fix brought its
consumer count back to three I re-extracted it as _stage_batch_by_name. The difference for
remove_tree_pinned is that it is the mechanism/policy split itself, not a convenience wrapper.

2. The move-aside-remove-put-back choreography is spelled twice: ACCEPTED-AND-DEFERRED,
recorded. Agreed, including that it is in tension with this PR's own rationale. The concrete
blocker is named in the description: _delete_listed_files's block also counts the manifest's
bytes into freed, drives the cleared flag behind SKIP_INCOMPLETE, and reports byte
progress through a caller-supplied callback. Converging means moving progress accounting and
skip-code policy into a module that holds no policy by design, on a path that only just
converged in #7011. Recorded locally as f-20260901-11 so it is re-judged on a clock rather
than living in a review thread.

3. Item 7 (delete-path debris unlink and put-back now inode-verified) is not in the
description: CORRECT, and I will fix the description. The rider is real and the reason it
rides along is that leaving it out would have shipped a shared put-back owner whose delete-path
caller kept the unguarded spelling of the same name-trust hole. The description covers the
extraction but not the inode-verification, nor two later rounds' work (the retained debris copy,
the UTF-8 refusal, and the three subtractions this lane asked for). I am folding all of that
into the body with the rebase below rather than editing it now -- an edit re-triggers the GPT
lane on a head that is about to be replaced, and the lanes read the body from the push event,
so editing before the push is what makes the reviewer see the current text.

Next step is blocked on #7554, not on me

#7554 (the memory.py log-site census) is still open. It is the sole remaining red on this
head apart from the adjudicated GPT finding, it is main-owned, and a re-run cannot absorb it
because a replay re-tests the same base snapshot. When it merges I will rebase, update the body
in the same push, and let all lanes re-roll on the rebased head.

[chenmingwei23]

Post-rebase round on acd7e84bd -- census red cleared, one new blocker fixed on 125cf6d4e

The rebase onto main (>= ebc0936f2) cleared the census failure: Backend Tests (3.10, 3)
and Backend Tests (Windows) (3) are green on acd7e84bd, which were the two shards carrying
test_security_posture.py::TestGateSideLogRedactorSpelling. 49 checks green before this push.

BLOCKING -- pinned_fs.py:1457, copy-error cleanup can unlink a replacement: FIXED, with one deviation

The finding is right about the mechanism. _unlink_verified stats the name and then unlinks
it, so a replacement arriving between those two syscalls is destroyed by a cleanup that meant
to remove only its own partial file. POSIX has no "unlink if this inode", so the check narrows
that window and cannot close it.

I did NOT take the prescribed remedy as written ("never unlink dst_name by name"), because
it reddens a test that is on mainline and states the opposite intent:

test_session_storage.py::TestEmptyTrash::test_a_failed_copy_back_leaves_no_half_written_manifest
  "A truncated manifest is worse than none at all. ... So a failed copy removes what it
   wrote and reports the failure, which at least leaves the debris named in the log."
  assert not manifest.exists(), "a partial manifest must not be left behind"

Truncate-only leaves a zero-length file at the manifest name. list_trash omits such a batch
exactly as it omits one with no manifest, so no listing harm - but a later read cannot tell it
from a corrupted manifest, a later retry's O_CREAT | O_EXCL cannot overwrite it, and it
contradicts a decision main already made explicitly.

So the truncate is taken as an ADDITION rather than a replacement: os.ftruncate(dst, 0)
through the descriptor this call is still holding, BEFORE any name is addressed, then the
existing identity-checked unlink. The ordering is what matters. The truncate cannot touch
anything else - dst is the file this call created, so even if the name now leads elsewhere it
lands on our own inode - which means the partial content is gone whether or not the unlink
happens. The unlink then keeps main's "nothing left behind" property in the ordinary case, and
its residual is bounded to a file that arrived at a private name this call created moments
earlier, inside a recovery path reached only after a removal already failed.

New test for the property the truncate actually buys, since the ordinary case looks identical
either way: test_a_partial_copy_that_cannot_be_unlinked_is_left_empty_not_partial refuses the
unlink outright and asserts the file at the name is empty rather than carrying half a manifest.
Mutation-verified - removing the ftruncate leaves b'entry-one\nentry-' at that name and the
test fails on exactly that.

BLOCKING -- descriptor-less cleanup: UNCHANGED, adjudicated

Sixth raise, and the Command Center has ruled on it: the disposition is approved on the merits,
with the residual tracked in #7566. The remedy suggested this round ("rename the batch back and
return False") is the same refusal that reddens five pre-existing Windows tests by leaving a
batch in the trash after every restore and every rolled-back move.

I am not posting the override on this head: an override suppresses ALL findings on the commit it
names, so pressing it while the copy-cleanup blocker stood would have buried a real one. That is
why this round is a push rather than an override. If the finding returns as the sole red on
125cf6d4e, the override goes up then.

311 passing across the three targeted suites; black/flake8/isort/mypy clean on touched
files, docs_lint passes.

[chenmingwei23]

Round on 125cf6d4e -- the remedy changed, so I implemented it instead of overriding: 57bd82c21

This round's finding is the pre-capture-swap half of the adjudicated one, but the prescribed
remedy is NEW and it is right, so this is a fix rather than an override.

Earlier rounds asked me to refuse when no descriptor-bound removal exists, which five
pre-existing Windows tests disprove, and I said the capture could not be hardened "from inside
this function" - pin_parent resolves the parent path too, so an ancestor that is already a link
cannot be told from a real path. That was true of _discard_restored_batch and it was the wrong
frame. This round names the actual fix: capture the identity BEFORE _restore_locked reads the
manifest and pass it in.

That closes a real window rather than shrinking one. The cleanup is reached only after the
restore has read the manifest and moved every listed file out, so an identity sampled there is
already the substitute's if an ancestor was swapped during any of that - and then every later
check agrees with itself about the wrong directory, including the pinned fstat comparison that
is supposed to be the backstop. Anchored at the top of _restore_locked, immediately after
_batch_dir and before the first read, the same comparison now refuses a swap that lands at ANY
point inside the operation.

_discard_restored_batch takes expect and no longer samples one; None refuses. The staging
path already had the strongest anchor available to it (target.mkdir(...) under the mutation
lock), so only the restore path changed.

New test, driven through the public restore() so the capture point under test is the real one:
the victim is set up on disk as a directory that would satisfy every CONTENT question - a
manifest whose header names the selected batch - and the swap lands after the restore's own
reads. It asserts the victim's file and manifest both survive and that the refusal is logged.
Mutation-verified: restoring the sample-in-place capture (expect=_identity_of(batch)) reddens
it on the victim's file.

What this does NOT close, unchanged and still tracked in #7566: a swap already in place before
the restore begins. Reaching it requires the victim tree to have served as the restore's own
source, which means content was already injected into the live session stores; the removal of the
emptied directory afterwards is not the damage.

The spec paragraph on the identity anchor is rewritten in the same commit - it described the old
capture point, which is exactly the kind of claim that survives as prose after the code moves.

312 passing across the three targeted suites; black/flake8/mypy clean on touched files,
docs_lint passes. No override posted: the finding this round was actionable, so pressing one
would have suppressed a real fix.

[chenmingwei23]

Round on 57bd82c21 -- new blocker fixed on 87348df3f; the adjudicated one still stands alone

BLOCKING -- session_storage.py:1498, the approval admits mutable links: FIXED

Real, and it catches an exemption I had argued for in a docstring. The approval refused any FILE
but the manifest and said links were deliberately out of scope, on the reasoning that a symbolic
link holds no data and the delete path removes scanned ones.

That misses what makes a link different in the other direction: it is the ONLY entry the removal
can take by NAME. _unlink_verified compares the scanned inode and then unlinks, and POSIX has
no atomic "unlink if this inode", so those are two syscalls. Admitting a link admits that window

• plant one, let the approval pass it, put a file at that name in between, and the removal
  deletes the file rather than the link. That is the same argument I accepted one round earlier for
  the copy-error cleanup, so accepting it here is consistency rather than a new concession.

Nonempty tree.links now withholds the removal.

I also checked the claim my docstring used to justify the exemption -- that refusing "would
strand a batch nothing can empty" -- and it is false, which is why the fix is free.
empty_trash reaches such a batch through its own approval, which does remove scanned links, so
a batch kept for this reason is still cleared on the user's own say-so. A dangling link is not
counted by _unlisted_files either (is_file() is False), so it does not block that path.
Docstring and spec both corrected; the spec's "nothing but the manifest" paragraph now says
explicitly that links are included and why these two paths differ from the delete path.

New test plants a dangling link and asserts the removal withholds with the link intact.
Mutation-verified: disabling the check reddens it.

BLOCKING -- descriptor-less coarse removal: UNCHANGED, adjudicated, now sole

Seventh raise, same prescribed refusal that reddens five pre-existing Windows tests. The
Command Center has approved the disposition on the merits with the residual tracked in #7566.

I did not post the override on 57bd82c21 because the link finding stood beside it, and an
override suppresses ALL findings on the commit it names -- pressing it would have buried a real
one. That is now the only outstanding item, so if it returns as the sole red on 87348df3f the
override goes up against that SHA.

313 passing across the three targeted suites; black/flake8/mypy clean on touched files,
docs_lint passes.

[chenmingwei23]

Round on 87348df3f -- escalating the recovery-residual cluster; measured against mainline

Three blockers this round. One is the adjudicated coarse-platform finding (eighth raise). The
other two are in the delete-path manifest RECOVERY, and both describe residuals that exist on
origin/main in strictly worse form, with remedies that contradict earlier rounds of this same
review and a test that is on mainline. I am not implementing them unasked and I am not
overriding on my own authority.

The two recovery findings, measured against origin/main

pinned_fs.py:1400 -- "recovery can install a substituted manifest". The mechanism is real:
os.link re-resolves src_name, so a swap after fstat(src) links whatever is there now.

	origin/main (session_storage.py:3232)	this PR
source	never opened or verified at all	opened O_RDONLY | O_NOFOLLOW, inode checked against expect_ino before anything is read
link	os.link(debris, MANIFEST_NAME, src_dir_fd=..., dst_dir_fd=...), default follow_symlinks	follow_symlinks=False
after	nothing checks what landed	landed inode verified; on mismatch the substitute is left untouched and the failure is reported

The prescribed remedy is to delete the hard-link branch and copy exclusively from the verified
descriptor. That removes the only non-clobbering ATOMIC put-back and makes every recovery a
byte copy on every filesystem, including the ones where links work - the branch's own docstring
records why link is first choice. It also cannot be tested as an improvement: the copy path is
already exercised, so the change is pure subtraction of a working mechanism.

There IS a real fix that GPT did not propose: link from the DESCRIPTOR rather than the name, via
os.link(f"/proc/self/fd/{src}", ...), which cannot be redirected because no name is resolved.
It is Linux-only, so it needs the name-based link as a fallback everywhere else, which leaves the
window on exactly the platforms that keep it. I flag it as an option rather than shipping it at
round sixteen of a review whose subject is two other functions.

pinned_fs.py:1471 (plus :1749, session_storage.py:2811) -- "verified cleanup can unlink a
replacement". Also real, and irreducible: POSIX has no atomic "unlink if this inode", so a
stat-then-unlink is two syscalls.

	origin/main (session_storage.py:3276-3280)	this PR
debris cleanup	with suppress(OSError): os.unlink(debris, dir_fd=parent_fd) -- unconditional, by name	identity-checked, and on the tree-survived path the debris is RETAINED rather than unlinked
partial copy	removed by name	emptied through the descriptor FIRST (os.ftruncate(dst, 0)), then the identity-checked unlink

This is the third distinct remedy this lane has asked for at this one site, and they are mutually
exclusive:

round	prescribed	outcome
1	unlink only while the name still holds our file	implemented
2	never unlink by name; truncate through the descriptor	truncate implemented; the unlink kept, because dropping it reddens a mainline test
3 (now)	retain the empty partial or debris instead of unlinking	reddens that same mainline test

The test is not mine:

test_session_storage.py::TestEmptyTrash::test_a_failed_copy_back_leaves_no_half_written_manifest
  "A truncated manifest is worse than none at all. ... So a failed copy removes what it wrote
   and reports the failure, which at least leaves the debris named in the log."
  assert not manifest.exists(), "a partial manifest must not be left behind"

Applying the same change at :1749 is worse still: that is the path where the tree IS gone, so
retaining there leaves a debris file behind after every successful trash cleanup.

What I am asking for

The Command Center has already adjudicated the coarse-platform finding on exactly this basis
(pre-existing, measurably narrowed here, prescribed remedy disproven by mainline tests) with the
residual tracked in #7566. These two are the same shape at two more sites in a RIDER - the delete
path's recovery, which this PR hardened because it shared the root cause, not because it was the
subject.

Options, in the order I would rank them:

1. Extend the recorded disposition to cover all three, and add the two recovery residuals to
   Windows session-trash removal and manifest recovery have no descriptor-bound path #7566 (or a sibling issue) as named, measured, strictly-narrowed-versus-base items.
2. Descope the rider: revert the delete-path recovery hardening to mainline's spelling, keeping
   only what the two sibling cleanups need. That structurally removes the finding cluster's root
   cause, at the cost of leaving mainline's weaker recovery in place.
3. Take the /proc/self/fd link on Linux with a name-based fallback elsewhere, as its own change.

313 passing across the three targeted suites on this head; black/flake8/mypy clean on
touched files, docs_lint passes. No code change this round: none of the three remedies is
takeable without breaking a mainline test or subtracting a working mechanism.

[chenmingwei23]

/ai-review override gpt 87348df: All three residuals exist on main in strictly worse form (unconditional by-name unlink; link with no source verification; bare rmtree with no identity check), so this PR narrows every site it touches. The prescribed remedies are mutually exclusive across rounds and two redden pre-existing tests (five Windows cases; a mainline copy-back case whose docstring states the opposite intent). Tracked in #7566.

[github-actions]

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 87348df3fa130a33bdf743bf5883c079e68149b5.

All three residuals exist on main in strictly worse form (unconditional by-name unlink; link with no source verification; bare rmtree with no identity check), so this PR narrows every site it touches. The prescribed remedies are mutually exclusive across rounds and two redden pre-existing tests (five Windows cases; a mainline copy-back case whose docstring states the opposite intent). Tracked in #7566.

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

[chenmingwei23]

Disposition of all three findings on 87348df3f (override posted above)

Recorded explicitly so each finding has its own answer rather than a cross-reference.

1. session_storage.py:1596 -- coarse cleanup follows a swapped ancestor.
ADJUDICATED RESIDUAL, not fixed. Eighth raise. The prescribed refusal was implemented earlier in
this review and reverted: _FD_SAFE_DELETE false is the whole of Windows, so refusing leaves a
batch in the trash after every restore and every rolled-back move, still listing sessions it no
longer holds. Five pre-existing tests read that as a failure -- verified in both directions
locally against a forced coarse platform, since Linux CI cannot reach the branch. On mainline
this path is shutil.rmtree(batch, ignore_errors=True) with errors suppressed and no identity
check at any point, and _identity_of does not exist there at all. Tracked in
#7566.

2. pinned_fs.py:1400 -- the put-back's hard link resolves its source by name.
ADJUDICATED RESIDUAL, not fixed. Mainline links the debris with no source verification at all
and default symlink following; this PR opens it O_RDONLY | O_NOFOLLOW, checks its inode against
the value recorded before the move, passes follow_symlinks=False, and verifies the landed inode
afterwards, leaving a substitute untouched and reporting the failure. The prescribed remedy
(delete the link branch, always copy) subtracts the only atomic non-clobbering put-back on every
filesystem including those where links work. The real fix -- linking through the descriptor via
/proc/self/fd -- is Linux-only and needs the name-based link as a fallback elsewhere, so it is a
new platform mechanism; named in #7566.

3. pinned_fs.py:1471 (and :1749, session_storage.py:2811) -- verified cleanup can unlink a
replacement. ADJUDICATED RESIDUAL, partially fixed. Irreducible by name: POSIX has no atomic
"unlink if this inode", so a stat-then-unlink is two syscalls. What this PR does close is the
consequence -- the partial file is emptied through the open descriptor BEFORE any name is
addressed, so no partial content survives whether or not the unlink happens, and on the path
where the tree survives the debris copy is retained rather than unlinked. Mainline's spelling is
with suppress(OSError): os.unlink(debris, dir_fd=parent_fd), unconditional. Three consecutive
rounds asked for three mutually exclusive remedies here (unlink only while it is still ours;
never unlink, truncate instead; retain rather than unlink), and the last reddens
test_a_failed_copy_back_leaves_no_half_written_manifest on mainline, whose docstring states
that a failed copy must remove what it wrote. Tracked in #7566.

State of this head

59 checks green, zero pending, the GPT lane the only red -- so the override clears the last
gate rather than masking anything. All four Windows shards, all four 3.10 and all four 3.12
shards, E2E, both lint/type-check jobs, CodeQL and the coverage lanes are green. 313 tests pass
across the three targeted suites; black/flake8/isort/mypy clean on touched files;
docs_lint passes; the spec moved with the code in the same commit.

The branch is PUSH-FROZEN from here: the override marker is SHA-pinned, so any further push
would void it and re-arm the gate.

[bolichen97]

Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.