Repository navigation
Conversation
…crash (#445) A directory is moved through the same quarantine directory, identified by the open directory and removed with AT_REMOVEDIR; one that fills after the check goes back. The link count of the held directory is checked afterwards, as for a file. A quarantine directory left behind by a crash or by an entry that could not be put back is taken by its generated name only, the file in it is authenticated again, and only an accepted file is removed. Names outside the generated grammar are refused and left alone.
Reviewer's GuideThis PR completes the Unix test-support implementations for empty-directory removal and leftover quarantine recovery, using descriptor-based quarantine, identity checks, strict generated-name and ownership validation, and repeat authentication; extensive tests document crash and race behavior without enabling production reclamation. Sequence diagram for authenticated Unix directory removalsequenceDiagram
participant Caller
participant PinnedDir
participant Quarantine
participant Unix
Caller->>PinnedDir: remove_dir
PinnedDir->>PinnedDir: open_dir
PinnedDir->>PinnedDir: entries
alt directory is not empty
PinnedDir-->>Caller: NotEmpty
else directory is empty
PinnedDir->>Quarantine: renameat2(RENAME_NOREPLACE)
Quarantine->>Quarantine: inspect open directory identity
Quarantine->>Unix: unlinkat(AT_REMOVEDIR)
Unix-->>Quarantine: removal result
Quarantine->>Quarantine: verify link count
alt identity changed
Quarantine-->>Caller: Disturbed
else removal succeeds
Quarantine-->>Caller: Removed
end
end
Sequence diagram for Unix leftover quarantine recoverysequenceDiagram
participant Caller
participant PinnedDir
participant Quarantine
participant File
Caller->>PinnedDir: remove_leftover
PinnedDir->>Quarantine: open_existing
Quarantine->>Quarantine: is_generated_name
alt invalid name or unsafe directory
Quarantine-->>Caller: InvalidName or Unsupported
else accepted quarantine directory
Quarantine->>Quarantine: inspect
alt empty quarantine
Quarantine-->>Caller: Vanished
else foreign object
Quarantine-->>Caller: Refused
else file found
Quarantine->>File: authenticate
alt authentication fails
File-->>Caller: Refused
else authentication succeeds
Quarantine->>File: unlink_verified
Quarantine->>Quarantine: discard
Quarantine-->>Caller: Removed
end
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
codeant-ai
Bot
added
the
size:L
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/worldscript-secure-storage/src/pinned/unix/removal.rs" line_range="271-287" />
<code_context>
remove_moved(dir, target, observer)
}
+pub(in crate::pinned) fn remove_dir(
+ dir: &PinnedDir,
+ name: &OsStr,
+ observer: Observer<'_>,
+) -> Result<RemovalOutcome, PinnedError> {
+ let child = dir.open_dir(name)?;
+ match child.entries(0) {
+ Ok(_) => {}
+ Err(PinnedError::TooManyEntries) => return Ok(RemovalOutcome::NotEmpty),
+ Err(error) => return Err(error),
+ }
+ observer(RemovalStage::Checked)?;
+ let target = Target {
+ name,
+ expected: child.identity(),
+ kind: EntryKind::Directory,
+ };
+ remove_moved(dir, target, observer)
+}
</code_context>
<issue_to_address>
**Replacement directory is removed**
When a same-user actor replaces the quarantined directory with an empty directory between identification and `unlinkat`, `unlink_verified` unlinks the current quarantine binding and checks its identity only afterwards, so it removes the replacement and returns `Disturbed` instead of removing exactly the opened directory.
Make `unlink_verified` ensure it cannot unlink a replacement of the opened directory.
</issue_to_address>
### Comment 2
<location path="crates/worldscript-secure-storage/src/pinned/unix/removal.rs" line_range="300-301" />
<code_context>
+) -> Result<RemovalOutcome, PinnedError> {
+ let held = Quarantine::open_existing(&dir.handle, quarantine)?;
+ let step = leftover_step(&held, dir, authenticate, observer);
+ held.discard(&dir.handle);
+ settle(dir, step?, observer)
+}
+
</code_context>
<issue_to_address>
**Leftover deletion overstates durability**
When a same-user process adds an entry to the quarantine directory between unlinking `x` and discarding that directory, a same-user process can add an entry to the quarantine directory after `unlink_verified` removes `x`, making `held.discard` fail. `settle` then syncs only the pinned parent and returns `Removed(Confirmed)`, although the change inside the still-existing quarantine directory was not synced and `x` can reappear after a crash.
Ensure the held quarantine directory is synced when it remains after the unlink, and report `Confirmed` only after the relevant directory change is durably synced.
</issue_to_address>
### Comment 3
<location path="crates/worldscript-secure-storage/src/pinned/unix/removal.rs" line_range="288" />
<code_context>
remove_moved(dir, target, observer)
}
</code_context>
<issue_to_address>
**Rolled-back directory changes are unsynced**
When an observer returns an error after the directory has moved and the restore succeeds, `remove_moved` propagates an observer error after restoring the directory without calling `settle`, so the parent directory is not synced and a crash can leave the rename rollback unrecorded despite the caller receiving an error.
Sync the parent after a successful rollback before propagating the observer error, and preserve/report a sync failure.
</issue_to_address>
### Comment 4
<location path="crates/worldscript-secure-storage/src/pinned/unix/removal.rs" line_range="310" />
<code_context>
+ authenticate: impl FnOnce(&mut PinnedFile) -> Result<bool, PinnedError>,
+ observer: Observer<'_>,
+) -> Result<Step, PinnedError> {
+ let (file, identity) = match held.inspect(EntryKind::File)? {
+ // An empty quarantine directory is junk and goes with `discard`.
+ Inside::Absent => return Ok(Step::Changed(RemovalOutcome::Vanished)),
</code_context>
<issue_to_address>
**Stranded directories cannot be recovered**
When the process crashes after an empty directory has been moved into quarantine, `leftover_step` inspects the held entry only as `EntryKind::File` and returns `Refused` for the directory, so `discard` cannot remove the nonempty quarantine and no recovery API reclaims it.
Teach leftover recovery to recognize and remove quarantined directories.
Also at `crates/worldscript-secure-storage/src/pinned/unix/removal.rs:271`.
</issue_to_address>
### Comment 5
<location path="crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs" line_range="720" />
<code_context>
+#[test]
+fn a_stranded_entry_is_finished_by_the_leftover_removal_after_authenticating_it() {
+ assert!(
+ !rustix::process::geteuid().is_root(),
+ "the permission checks of this case do not apply to root; run the tests as a normal user"
+ );
</code_context>
<issue_to_address>
**Root-run tests fail**
When the Unix test suite runs with effective UID 0, the non-root assertion in `a_stranded_entry_is_finished_by_the_leftover_removal_after_authenticating_it` panics before exercising recovery, so the Unix test suite exits unsuccessfully in root-run CI jobs and containers.
Skip the permission-dependent test when the effective UID is root instead of asserting that it is not.
Also at `crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs:719`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and if the empty-directory check or leftover-file authentication is wrong, the quarantine flow can unlink an object that should have been kept; reverting cannot restore it. The new removal APIs are limited to test-support builds, which bounds where this deletion behavior is available.
Blocking findings: crates/worldscript-secure-storage/src/pinned/unix/removal.rs:287, crates/worldscript-secure-storage/src/pinned/unix/removal.rs:301, crates/worldscript-secure-storage/src/pinned/unix/removal.rs:288, crates/worldscript-secure-storage/src/pinned/unix/removal.rs:310, crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs:720
Comment on lines
+271
to
+287
| pub(in crate::pinned) fn remove_dir( | ||
| dir: &PinnedDir, | ||
| name: &OsStr, | ||
| observer: Observer<'_>, | ||
| ) -> Result<RemovalOutcome, PinnedError> { | ||
| let child = dir.open_dir(name)?; | ||
| match child.entries(0) { | ||
| Ok(_) => {} | ||
| Err(PinnedError::TooManyEntries) => return Ok(RemovalOutcome::NotEmpty), | ||
| Err(error) => return Err(error), | ||
| } | ||
| observer(RemovalStage::Checked)?; | ||
| let target = Target { | ||
| name, | ||
| expected: child.identity(), | ||
| kind: EntryKind::Directory, | ||
| }; |
There was a problem hiding this comment.
🟡 Medium · Replacement directory is removed
When a same-user actor replaces the quarantined directory with an empty directory between identification and unlinkat, unlink_verified unlinks the current quarantine binding and checks its identity only afterwards, so it removes the replacement and returns Disturbed instead of removing exactly the opened directory.
Make unlink_verified ensure it cannot unlink a replacement of the opened directory.
Prompt for AI agents
In `crates/worldscript-secure-storage/src/pinned/unix/removal.rs` at lines 271-287:
**Replacement directory is removed**
When a same-user actor replaces the quarantined directory with an empty directory between identification and `unlinkat`, `unlink_verified` unlinks the current quarantine binding and checks its identity only afterwards, so it removes the replacement and returns `Disturbed` instead of removing exactly the opened directory.
Make `unlink_verified` ensure it cannot unlink a replacement of the opened directory.
Comment on lines
+300
to
+301
| held.discard(&dir.handle); | ||
| settle(dir, step?, observer) |
There was a problem hiding this comment.
🟡 Medium · Leftover deletion overstates durability
When a same-user process adds an entry to the quarantine directory between unlinking x and discarding that directory, a same-user process can add an entry to the quarantine directory after unlink_verified removes x, making held.discard fail. settle then syncs only the pinned parent and returns Removed(Confirmed), although the change inside the still-existing quarantine directory was not synced and x can reappear after a crash.
Ensure the held quarantine directory is synced when it remains after the unlink, and report Confirmed only after the relevant directory change is durably synced.
Prompt for AI agents
In `crates/worldscript-secure-storage/src/pinned/unix/removal.rs` at lines 300-301:
**Leftover deletion overstates durability**
When a same-user process adds an entry to the quarantine directory between unlinking `x` and discarding that directory, a same-user process can add an entry to the quarantine directory after `unlink_verified` removes `x`, making `held.discard` fail. `settle` then syncs only the pinned parent and returns `Removed(Confirmed)`, although the change inside the still-existing quarantine directory was not synced and `x` can reappear after a crash.
Ensure the held quarantine directory is synced when it remains after the unlink, and report `Confirmed` only after the relevant directory change is durably synced.| expected: child.identity(), | ||
| kind: EntryKind::Directory, | ||
| }; | ||
| remove_moved(dir, target, observer) |
There was a problem hiding this comment.
🟡 Medium · Rolled-back directory changes are unsynced
When an observer returns an error after the directory has moved and the restore succeeds, remove_moved propagates an observer error after restoring the directory without calling settle, so the parent directory is not synced and a crash can leave the rename rollback unrecorded despite the caller receiving an error.
Sync the parent after a successful rollback before propagating the observer error, and preserve/report a sync failure.
Prompt for AI agents
In `crates/worldscript-secure-storage/src/pinned/unix/removal.rs` at line 288:
**Rolled-back directory changes are unsynced**
When an observer returns an error after the directory has moved and the restore succeeds, `remove_moved` propagates an observer error after restoring the directory without calling `settle`, so the parent directory is not synced and a crash can leave the rename rollback unrecorded despite the caller receiving an error.
Sync the parent after a successful rollback before propagating the observer error, and preserve/report a sync failure.| authenticate: impl FnOnce(&mut PinnedFile) -> Result<bool, PinnedError>, | ||
| observer: Observer<'_>, | ||
| ) -> Result<Step, PinnedError> { | ||
| let (file, identity) = match held.inspect(EntryKind::File)? { |
There was a problem hiding this comment.
🟡 Medium · Stranded directories cannot be recovered
When the process crashes after an empty directory has been moved into quarantine, leftover_step inspects the held entry only as EntryKind::File and returns Refused for the directory, so discard cannot remove the nonempty quarantine and no recovery API reclaims it.
Teach leftover recovery to recognize and remove quarantined directories.
Also at crates/worldscript-secure-storage/src/pinned/unix/removal.rs:271.
Prompt for AI agents
In `crates/worldscript-secure-storage/src/pinned/unix/removal.rs` at line 310:
**Stranded directories cannot be recovered**
When the process crashes after an empty directory has been moved into quarantine, `leftover_step` inspects the held entry only as `EntryKind::File` and returns `Refused` for the directory, so `discard` cannot remove the nonempty quarantine and no recovery API reclaims it.
Teach leftover recovery to recognize and remove quarantined directories.
Also at `crates/worldscript-secure-storage/src/pinned/unix/removal.rs:271`.| #[test] | ||
| fn a_stranded_entry_is_finished_by_the_leftover_removal_after_authenticating_it() { | ||
| assert!( | ||
| !rustix::process::geteuid().is_root(), |
There was a problem hiding this comment.
🟡 Medium · Root-run tests fail
When the Unix test suite runs with effective UID 0, the non-root assertion in a_stranded_entry_is_finished_by_the_leftover_removal_after_authenticating_it panics before exercising recovery, so the Unix test suite exits unsuccessfully in root-run CI jobs and containers.
Skip the permission-dependent test when the effective UID is root instead of asserting that it is not.
Also at crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs:719.
Prompt for AI agents
In `crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs` at line 720:
**Root-run tests fail**
When the Unix test suite runs with effective UID 0, the non-root assertion in `a_stranded_entry_is_finished_by_the_leftover_removal_after_authenticating_it` panics before exercising recovery, so the Unix test suite exits unsuccessfully in root-run CI jobs and containers.
Skip the permission-dependent test when the effective UID is root instead of asserting that it is not.
Also at `crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs:719`.
CodeAnt Nitpicks1 code suggestion1. If rollback fails after moving a directory, it can remain as
|
CodeAnt PR Risk: Medium Risk
Assessed commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24683a83a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) -> Result<RemovalOutcome, PinnedError> { | ||
| let held = Quarantine::open_existing(&dir.handle, quarantine)?; | ||
| let step = leftover_step(&held, dir, authenticate, observer); | ||
| held.discard(&dir.handle); |
There was a problem hiding this comment.
When authentication returns false (or inspection finds a foreign entry) while another same-user process renames the opened quarantine directory and places an empty directory under the same generated name, leftover_step returns Unchanged(Refused) but this unconditional name-based discard deletes the replacement. The call consequently reports Refused and skips synchronization even though it removed an unverified directory; cleanup should occur only for a step that proved it appropriate and must verify or detect the directory identity race.
AGENTS.md reference: AGENTS.md:L29-L30
Useful? React with 👍 / 👎.
| expected: child.identity(), | ||
| kind: EntryKind::Directory, | ||
| }; | ||
| remove_moved(dir, target, observer) |
There was a problem hiding this comment.
When a same-user actor wins the Identified window by renaming the authenticated empty directory out of x and replacing it with a non-empty directory, unlinkat(AT_REMOVEDIR) returns NOTEMPTY; this new directory-removal path then restores that foreign directory to the original name and reports NotEmpty without comparing its identity. The caller therefore cannot distinguish an ordinary directory that filled after the check from the exact swap that RemovalOutcome::Swapped is intended to report, which can hide tampering from a future reclamation caller.
Useful? React with 👍 / 👎.
…ate4e-h1a2-unix-dir-and-leftovers
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
This was referenced Oct 15, 2026
qnbs
added a commit
that referenced
this pull request
Oct 16, 2026
…e the limits of the leftover recovery (#445) Carries the findings that were open on the superseded #1070 into this PR, checked against the code of main (most were already closed by the hardening it has: the rollback sync, the sync of the held directory, the identity-checked cleanup, the root-safe test). A directory that cannot be removed because it has entries goes back where it was and was reported as NotEmpty. If an actor won the last window by moving the authenticated empty directory away and putting a non-empty one in its place, the refusal was about that directory and said nothing about ours (Codex P2 on #1070). The unlink refusal now checks that what is inside is still the authenticated directory (held open, so its identity is unique); if not, it goes back under the name all the same and the outcome is Disturbed. A put-back that fails stays a failure that ends in Stranded or Disturbed as before. Stated, not changed: an empty foreign directory that wins the same window is still removed instead of the authenticated one (no Unix primitive unlinks "this inode"; it holds no data, and the removal reports Disturbed), and the leftover recovery reclaims files, so an empty directory left by a crashed directory removal stays in its .h1q-* directory (Sourcery on #1070). Both are in the changelog, the evidence document and the API documentation. Test: a non-empty directory swapped in at the last window gives Disturbed with the original directory intact and the swapped-in one back under the name. Mutation-checked: reporting NotEmpty for any directory fails it. test-support only, no caller, no new unsafe, no new package. Issue #359.
Owner
Author
|
Superseded by #1074, not merged. This PR was stacked on the history of the squash-merged #1069, and its code predates the hardening that is now on The 7 findings open here are accounted for in the body of #1074 (table): four are closed by the hardening on No history was rewritten and the branch is kept; #1071 is still based on it and is re-cut next. Checks and reviews on this head did not carry over to #1074. |
qnbs
added a commit
that referenced
this pull request
Oct 16, 2026
…path, not its link count (#445) The hosted macOS evidence of this PR failed the removal of an empty directory: the outcome was Disturbed instead of Removed(Confirmed). The check after the unlink asked the open directory whether its link count had dropped, which holds on Linux, where a removed directory that is still open reports zero. macOS keeps reporting the old count (or cannot be asked), so the check could never confirm a removal there. The superseded #1070 never ran on a hosted OS (a stacked PR gets no hosted evidence), so this was the first time that path met macOS; the other directory cases passed there only because they expect Disturbed. For a directory on Apple targets the signal is now that the open descriptor no longer has a path: fcntl(F_GETPATH), rustix::fs::getpath, fails with "no such file" for a removed directory and still answers for one that an actor moved away and that is therefore not the one that was removed. Anything else, such as a path that is still there or another error, stays Disturbed (a removal that cannot be checked afterwards says nothing). Linux and files are unchanged. No new package and no new feature: getpath needs rustix's alloc, which std already enables. Whether macOS answers as described is not known from here; the hosted macOS run of this commit is the evidence, and the changelog and the evidence document state the signal as the one that run showed. test-support only, no caller, no new unsafe. Issue #359.
qnbs
added a commit
that referenced
this pull request
Oct 16, 2026
…s, look at a restored directory again, recover an empty directory leftover (#445) Three findings on 2989ba8, all valid. A leftover removal removes the emptied quarantine directory with discard, which changes the parent. When the removal itself changed nothing (a refusal) or failed, that change was never synced: Unchanged skipped the sync, and an error returned before the settle. A crash could then bring the empty directory back although the caller had been told the result (Sourcery, Codex P2 and a CodeAnt nitpick, independently). discard now reports whether it removed the directory; the parent is synced in that case for a refusal and for an error, and the result of the removal is kept. A directory that cannot be removed because it has entries goes back under its name, and was looked at before the rename. An actor that swaps another directory in between the look and the rename had it restored and reported as NotEmpty (Codex P2). It is now looked at under its name after it went back, against the identity of the open authenticated directory, and anything else is Disturbed. The earlier look is gone: it was the same check at a worse place. A crashed directory removal leaves an empty directory as x in its quarantine directory, and the leftover recovery only took files (Sourcery, on #1070 and again here). An empty directory holds nothing, so it needs no authentication and goes with the same unlink and the same check as the removal of a directory, on Linux; elsewhere it is refused, a directory with entries stays (NotEmpty), and a link or a FIFO stays (Refused). The temporary macOS probe thread (Codex P1) is obsolete: the probe was removed in the previous commit and the hosted macOS run is green. Tests: the parent sync for a refusal and an error with the entry taken, and not when the entry stays; the recovery of an empty directory after a crash during a directory removal, with the callback never asked; a directory with entries stays; Refused for an empty directory where the removal cannot be checked. Each of the three changes is mutation-checked. test-support only, no caller, no new unsafe, no new package. Issue #359.
qnbs
added a commit
that referenced
this pull request
Oct 16, 2026
…crash (#445) (#1074) * feat(core): remove a directory on Unix and finish the leftovers of a crash (#445) Fresh re-cut of #1070 on the hardened main (1f37cd8), not a retarget: that PR was built on the history of the squash-merged #1069 and its code predates the quarantine sync, the identity checks and the fail-closed look that main has now. Its one own change is re-applied on that code, with its tests. test-support only, no caller, no new unsafe, no new package. Windows still answers Unsupported. - remove_empty_dir opens the child directory (no link followed), refuses one that has entries and moves it through the same quarantine directory; it is identified by the open directory and unlinked with AT_REMOVEDIR. A directory that fills after the check goes back (NotEmpty), through the step that syncs the quarantine directory like any put-back. - remove_leftover_file takes a .h1q-* directory by its generated name only (the prefix and exactly 32 lowercase hexadecimal digits, otherwise InvalidName before anything is opened), requires a directory of the effective user without access for group or others, authenticates the file in it again and unlinks it only if accepted. A file that does not authenticate and anything that is not a regular file (a link, a directory) stay and give Refused; an empty generated directory is removed. - On the hardened code the leftover removal syncs the quarantine directory it unlinks in and says Removed(NotConfirmed) if that fails; an entry that vanished before the unlink is reported like one that vanished before. Tests (own file): a directory swapped or filled after the check, an empty directory that wins the last window (Disturbed), a simulated crash and its recovery, the name grammar (short, long, upper case, not hexadecimal), a foreign object and a link left in the quarantine directory, a stranded entry (put-back blocked, proved to be there) finished by the recovery, and the failed sync of the held directory. The earlier permission-based stranded case is now Disturbed on main (a failed look proves nothing) and is not asserted here. Mutation-checked: no sync in the leftover removal, no refusal of a foreign entry, a loosened name grammar and an ignored authentication each fail a case. The PR number in the changelog entry is to be set to the number of this PR when it is opened. Issue #359. * docs(changelog): set the PR number of the Unix directory and leftover entry (#445) * fix(core): report a swapped-in non-empty directory as disturbed, state the limits of the leftover recovery (#445) Carries the findings that were open on the superseded #1070 into this PR, checked against the code of main (most were already closed by the hardening it has: the rollback sync, the sync of the held directory, the identity-checked cleanup, the root-safe test). A directory that cannot be removed because it has entries goes back where it was and was reported as NotEmpty. If an actor won the last window by moving the authenticated empty directory away and putting a non-empty one in its place, the refusal was about that directory and said nothing about ours (Codex P2 on #1070). The unlink refusal now checks that what is inside is still the authenticated directory (held open, so its identity is unique); if not, it goes back under the name all the same and the outcome is Disturbed. A put-back that fails stays a failure that ends in Stranded or Disturbed as before. Stated, not changed: an empty foreign directory that wins the same window is still removed instead of the authenticated one (no Unix primitive unlinks "this inode"; it holds no data, and the removal reports Disturbed), and the leftover recovery reclaims files, so an empty directory left by a crashed directory removal stays in its .h1q-* directory (Sourcery on #1070). Both are in the changelog, the evidence document and the API documentation. Test: a non-empty directory swapped in at the last window gives Disturbed with the original directory intact and the swapped-in one back under the name. Mutation-checked: reporting NotEmpty for any directory fails it. test-support only, no caller, no new unsafe, no new package. Issue #359. * fix(core): tell a removed directory from a moved one on macOS by its path, not its link count (#445) The hosted macOS evidence of this PR failed the removal of an empty directory: the outcome was Disturbed instead of Removed(Confirmed). The check after the unlink asked the open directory whether its link count had dropped, which holds on Linux, where a removed directory that is still open reports zero. macOS keeps reporting the old count (or cannot be asked), so the check could never confirm a removal there. The superseded #1070 never ran on a hosted OS (a stacked PR gets no hosted evidence), so this was the first time that path met macOS; the other directory cases passed there only because they expect Disturbed. For a directory on Apple targets the signal is now that the open descriptor no longer has a path: fcntl(F_GETPATH), rustix::fs::getpath, fails with "no such file" for a removed directory and still answers for one that an actor moved away and that is therefore not the one that was removed. Anything else, such as a path that is still there or another error, stays Disturbed (a removal that cannot be checked afterwards says nothing). Linux and files are unchanged. No new package and no new feature: getpath needs rustix's alloc, which std already enables. Whether macOS answers as described is not known from here; the hosted macOS run of this commit is the evidence, and the changelog and the evidence document state the signal as the one that run showed. test-support only, no caller, no new unsafe. Issue #359. * test(core): TEMPORARY probe of what macOS reports for a removed, a moved and a live open directory (#445) Always fails on purpose so the hosted macOS log prints the answer; removed again in the next commit. Not part of the change. * test(core): TEMPORARY macOS probe, do not create the probe file before the directory is removed (#445) Still temporary and removed again in the next commit. * fix(core): remove a directory on Linux only, answer Unsupported where the system cannot tell afterwards (#445) The hosted macOS evidence of this PR failed the removal of an empty directory (Disturbed instead of Removed). A first attempt to read the removal from fcntl(F_GETPATH) was also refused by macOS, so the answer was measured instead of guessed: a temporary probe on the hosted runner (commits b59fe38 and 8e294a2, removed here) compared an open directory that was removed, one that was only moved and one that was untouched. link count 2 / 2 / 2 (unchanged for the removed one) F_GETPATH the old path for the removed one (stale) openat(".."), statat(".") succeed for all three create an entry in it fails with ENOENT only for the removed one The removal of a directory counts only if the open directory can be asked afterwards whether it was the one that lost its name. Linux answers (a removed directory that is still open reports a link count of zero). On macOS the only signal is a write into a directory that, in the attack case, an actor moved away and that belongs to another path, and it fails for lack of permission on a read-only directory. That is not a check to build a success on. remove_empty_dir therefore answers Unsupported on any Unix system but Linux, after the checks that need no such signal (a link, a missing name, a directory that has entries) and before anything is moved. The directory cases move to a Linux-only test file; on the other systems the same call is pinned to Unsupported with nothing moved, and the generic directory test expects it. The leftover recovery (files) is unchanged and runs on both. Whether and how macOS gets directory removal is left to the decision about the production use. The changelog, the evidence document and the API documentation state the limit and the measured behaviour. test-support only, no caller, no new unsafe, no new package. Issue #359. * fix(core): sync the parent when a leftover's quarantine directory goes, look at a restored directory again, recover an empty directory leftover (#445) Three findings on 2989ba8, all valid. A leftover removal removes the emptied quarantine directory with discard, which changes the parent. When the removal itself changed nothing (a refusal) or failed, that change was never synced: Unchanged skipped the sync, and an error returned before the settle. A crash could then bring the empty directory back although the caller had been told the result (Sourcery, Codex P2 and a CodeAnt nitpick, independently). discard now reports whether it removed the directory; the parent is synced in that case for a refusal and for an error, and the result of the removal is kept. A directory that cannot be removed because it has entries goes back under its name, and was looked at before the rename. An actor that swaps another directory in between the look and the rename had it restored and reported as NotEmpty (Codex P2). It is now looked at under its name after it went back, against the identity of the open authenticated directory, and anything else is Disturbed. The earlier look is gone: it was the same check at a worse place. A crashed directory removal leaves an empty directory as x in its quarantine directory, and the leftover recovery only took files (Sourcery, on #1070 and again here). An empty directory holds nothing, so it needs no authentication and goes with the same unlink and the same check as the removal of a directory, on Linux; elsewhere it is refused, a directory with entries stays (NotEmpty), and a link or a FIFO stays (Refused). The temporary macOS probe thread (Codex P1) is obsolete: the probe was removed in the previous commit and the hosted macOS run is green. Tests: the parent sync for a refusal and an error with the entry taken, and not when the entry stays; the recovery of an empty directory after a crash during a directory removal, with the callback never asked; a directory with entries stays; Refused for an empty directory where the removal cannot be checked. Each of the three changes is mutation-checked. test-support only, no caller, no new unsafe, no new package. Issue #359. * docs(core): say that the recovery refuses a link or a FIFO and that only an explicit call takes a leftover (#445) A leftover quarantine directory is not removed by a sweep; the explicit recovery takes a file or an empty directory and refuses anything else, which stays. Wording only (CodeRabbit on ec7ba93), plus one spelling nit in the changelog. No code change.
qnbs
added a commit
that referenced
this pull request
Oct 16, 2026
…#1075) * feat(core): remove on Windows through the authenticated handle (#445) Fresh re-cut of #1071 on the main that carries the Unix hardening and the directory removal, not a retarget: that PR was stacked on the history of the squash-merged #1069 and #1070. Its one own change (bdf2011) is re-applied here with its tests, and the two findings that were open on it are dealt with. test-support only, no caller. One new unsafe call (the disposition, one fixed structure, its layout pinned by a test), no new package. - The object is opened through the handle-relative lookup of every child, with delete access and a share mode of read only, authenticated on that handle and then given the POSIX delete disposition on the same handle: no FILE_FLAG_DELETE_ON_CLOSE, no fallback to the classic disposition, a read-only file is refused. A directory is removed the same way and a directory with entries refuses (NotEmpty). The removal is reported as Removed(NotConfirmed): Windows offers no directory sync. A leftover removal is Unsupported, because Windows creates no quarantine directory. - The cases of remove.rs now run on Windows too. The directory case there expects the removal on Linux and on Windows and Unsupported on any other Unix system, which the Unix re-cut introduced and which would otherwise have failed the hosted Windows run. Findings carried from #1071: - Codex P2: ERROR_INVALID_FUNCTION, which an older Windows or a driver gives when it does not implement the information class, was not mapped and became a plain I/O error. Every code that means "no POSIX delete" now maps to Unsupported through one function, known_refusal, that a test pins (including that an unrelated code stays an error). - Sourcery: a writable memory-mapped view that another process created earlier and keeps after closing its handle is not stopped by the read-only sharing rule, so it can still change the authenticated bytes before the disposition. No sharing mode closes this; it is stated in the module documentation, the evidence document and the changelog as a same-user residual, not claimed closed, and not tested because a hosted run cannot be made to hold such a mapping reliably. Checked here only by compiling for x86_64-pc-windows-msvc (fmt, clippy -D warnings); the hosted Windows run is the evidence. The PR number in the changelog entry is set when this PR is opened. Issue #359. * docs(changelog): set the PR number of the Windows removal entry (#445) * docs(core): say that the removal binds the object, not its bytes, on Unix and on Windows (#445) CodeAnt asked whether the mapped-writer limit on Windows is acceptable for this slice. Looking at it showed that the Unix removal has the same limit in a wider form and never said so: there is no sharing mode, so a same-user process with a writable descriptor or a writable shared mapping can change the bytes between the authentication and the unlink. Both are now stated in the module documentation, the evidence document and the changelog as parts of the same-user residual, not closed. Documentation only. * fix(core): look at a directory again right before its disposition on Windows (#445) Codex P2 on 7c43e60: the share mode of a directory handle cannot stop a writer from converting the empty directory into a reparse point in place (the module already says so for the pinned directory), and the observer, a caller's code, runs between the check and the disposition. The disposition would then have removed the converted entry and reported it as Removed although the API refuses links. The held handle is now checked a second time right before the disposition: a junction made in that window is left, its target is untouched and the removal answers Redirected. This narrows the window and does not close it; a file cannot be converted because its handle shares no write access. Test: the directory is converted into a junction at the Checked stage with the existing convert_in_place helper. It runs on Windows only, so the hosted Windows run is the evidence (checked here by compiling for the Windows target); it is not mutation-checked locally. Documentation, changelog and evidence document say the same. test-support only, no new unsafe, no new package. Issue #359. * docs(core): limit the Windows write protection claim to handles (#445) CodeRabbit on 140e981: the changelog said no writer can change the authenticated bytes while its own entry notes that a writable mapped view that existed before can. The claim is now limited to handles that open the file, and the test header says the same. Wording only.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Part of #359 (R-15 Gate 4, slice H1a-2), part 3 of 4, stacked on #1069.
test-supportonly: no production caller, no production deletion release.What this adds
remove_empty_diron Unix: the child directory is opened without following a link, refused if it has entries (NotEmpty), moved through the same quarantine directory, identified by the open directory and removed withunlinkat(AT_REMOVEDIR); one that fills after the check goes back. The link count of the held directory is checked afterwards (Disturbedotherwise): an empty foreign directory that wins the last window is removed instead and the removal says so (it holds no data).remove_leftover_file: a crash, a refused put-back (Swapped { restored: false }) orStrandedleaves a.h1q-*directory holding the entry asx; a sweep never removes it by itself. It is taken by its generated name only (.h1q-+ exactly 32 lowercase hex digits; anything else isInvalidNameand untouched), must be a directory of the effective user without group/other access, the file in it is authenticated again, and only an accepted file is removed. An empty generated quarantine directory is removed.No new
unsafe, no new package.Proof
Directory swapped in the first window / filled after the check (put back, not removed); an empty directory in the last window (
Disturbed, original intact); a simulated crash (observer panics once the entry is in the quarantine directory): the entry is left there, refused when the authentication fails, finished when it succeeds; names outside the generated grammar refused (.h1q-empty, non-hex, other directories); a foreign object left by a refused put-back is never removed; a stranded entry (quarantine lost its search permission) is finished after the permission is back. Local: fmt, clippy-D warningson Linux / Windows / macOS targets, lib and isolation tests.Findings of the first draft answered here: Codex / CodeAnt on the leftover-name grammar, on the directory in the last window (
unlinkat(AT_REMOVEDIR)can remove an empty replacement: detected, documented), and on the honesty of the claims.Status:
H1A_2_UNIX_DIRECTORY_AND_LEFTOVERS=IMPLEMENTED_TEST_SUPPORT_ONLY,H1_PROVED=NO,R1_PRODUCTION_RECLAMATION=NOT_ADMITTED,GATE4_TERMINAL=NO.Summary by Sourcery
Complete the Unix test-support implementation for empty-directory removal and safe recovery of interrupted quarantine cleanup.
New Features:
Enhancements:
Documentation:
Tests:
Summary by cubic
Implements Unix directory removal and crash-leftover recovery for test-support, replacing the
Unsupportedstubs from part 1. No production caller and no production deletion release.remove_empty_diropens the child without following links, refuses a non-empty directory, and removes it through the quarantine directory with the same link-count check as files.remove_leftover_filefinishes.h1q-*directories left after a crash or a refused put-back: it accepts only generated names, re-authenticates the held file, and removes it only when accepted.No new
unsafe, no new package.Written for commit 5c535b6. Summary will update on new commits.
CodeAnt-AI Description
Add safe Unix directory removal and recovery for interrupted cleanup
What Changed
Impact
✅ Empty directories removable in Unix test-support✅ Interrupted removals recoverable after authentication✅ Unrecognized or unauthenticated leftovers stay intact💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.