Skip to content

feat(core): remove a directory on Unix and finish the leftovers of a crash (#445) - #1074

Merged
qnbs merged 9 commits into
mainfrom
feat/445-gate4e-h1a2-unix-dir-and-leftovers-v2
Oct 16, 2026
Merged

qnbs merged 9 commits into
mainfrom
feat/445-gate4e-h1a2-unix-dir-and-leftovers-v2

Conversation

@qnbs

@qnbs qnbs commented Oct 15, 2026 •

Copy link
Copy Markdown
Owner

What this is

A fresh re-cut of #1070 on the hardened main (1f37cd83), not a retarget. #1070 was stacked on the history of the squash-merged #1069 and its code predates what main has now (quarantine-directory sync, identity checks, fail-closed look at a retained entry). Its one own change (24683a83) is re-applied on that code with its tests; one signed commit on main. #1070 stays frozen until this PR is integrated and is then closed as superseded, not merged. No history was rewritten.

test-support only: no caller, no new unsafe, no new package. Windows still answers Unsupported (its own slice, #1071), and so does the removal of a directory on every Unix system but Linux (see below). Slice H1a-2 part 3, part of #359 (Linear QNB-168).

What it adds

  • remove_empty_dir (Linux only): opens the child directory (no link followed), refuses one with entries (NotEmpty), and moves it through the same quarantine directory; identified by the open directory, then unlinkat(AT_REMOVEDIR). A directory that fills after the check goes back, through the step that syncs the quarantine directory like any put-back.
  • remove_leftover_file: takes a .h1q-* directory by its generated name only (prefix plus exactly 32 lowercase hex digits, otherwise InvalidName before anything is opened), requires a directory of the effective user without group/other access, authenticates the file in it again and unlinks it only if accepted. A file that does not authenticate stays (Refused), and so does anything that is neither a regular file nor an empty directory (a link, a FIFO; a directory with entries is NotEmpty). An empty directory left by a crashed directory removal is removed without authentication (Linux), and an emptied generated quarantine directory is removed; the parent is synced whenever that removal changed it, whatever the result.

What changed in the re-application (the point of the re-cut)

  • 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. (feat(core): remove a directory on Unix and finish the leftovers of a crash (#445) #1070's code had neither.)
  • Quarantine::open_existing records the directory's identity, so its cleanup uses the same identity-checked removal as every other quarantine directory.
  • The old stranded-by-lost-permission test is not carried over: on main that case is Disturbed (a failed look proves nothing). The stranded recovery is tested with a blocked put-back, where the entry is proved to be there, and runs for root too.
  • The module documentation no longer says directories and leftovers answer Unsupported on Unix.

Tests and proof (own file remove_unix_dir.rs)

A directory swapped or filled after the check; an empty and a non-empty directory that win the last window (Disturbed, original intact; the non-empty one is not reported as NotEmpty); a simulated crash and its recovery with a refused and an accepted authentication; the name grammar (too short, too long, upper case, not hexadecimal); a foreign object and a link left in the quarantine directory; a stranded entry finished by the recovery; the failed sync of the held directory (NotConfirmed, through the cfg(test) seam that models the error; no crash is simulated). Mutation-checked, each caught: no sync in the leftover removal, no refusal of a foreign entry, a loosened name grammar (this one survived the first version of the tests and drove the extra name cases), an ignored authentication result, and reporting NotEmpty for any directory.

The 7 findings that were open on #1070, and where they stand here

Finding on #1070 Here
Sourcery: rollback of a directory is not synced Closed by the hardening on main: a rolled-back or stranded failure syncs the parent (best effort, the cause is kept).
Sourcery: leftover deletion overstates durability Closed: the held quarantine directory is synced for what the unlink changed, else Removed(NotConfirmed); tested with the failing-sync seam.
Sourcery: root-run test asserts non-root Closed: the stranded recovery is tested with a blocked put-back, which runs for root.
Codex P2: refused leftover discarded by its stale name Narrowed: discard removes the name only if it still leads to the directory that was held (remove_if_named); rmdir takes a name, so the race is narrowed, not closed.
Codex P2: NotEmpty reported without rechecking the directory Fixed here: if what is inside is no longer the authenticated directory, it goes back under the name and the outcome is Disturbed. Test a_non_empty_directory_swapped_in_the_last_window_is_not_reported_as_not_empty; mutation-checked.
Sourcery: replacement empty directory is removed in the last window Stated, not closed: no Unix primitive unlinks "this inode". An empty foreign directory that wins that window is removed instead and the removal says Disturbed; it holds no data. Part of the owner-gated same-user residual.
Sourcery: a directory left by a crash cannot be recovered (raised again here) Fixed here (Linux): an empty directory left as x by a crashed directory removal holds nothing, so it needs no authentication and goes with the same unlink and check; a directory with entries stays (NotEmpty); a link or FIFO stays (Refused); other systems refuse it. Tests for each; mutation-checked.

Findings raised on this PR itself (3 waves)

  • NotEmpty reported for a directory an actor swapped in (Codex P2 on feat(core): remove a directory on Unix and finish the leftovers of a crash (#445) #1070, fixed, test); the same check moved to after the rename (Codex P2 here, fixed, test).
  • The parent was not synced when the emptied quarantine directory of a refused or failed leftover removal went away (Sourcery, Codex P2 and a CodeAnt nitpick, fixed, test).
  • Hosted macOS failed the directory removal: see the next section.
  • The temporary macOS probe thread (Codex P1) is obsolete, the probe is removed.

Why directory removal is Linux only (measured, not guessed)

The first hosted macOS run failed the removal of an empty directory (Disturbed instead of Removed): the check after the unlink asks the open directory whether its link count dropped, which holds on Linux only. A second attempt via fcntl(F_GETPATH) failed the same way, so I stopped guessing and ran a temporary probe on the hosted macOS runner (commits b59fe385, 8e294a2c, removed again in 2989ba80) that compares an open directory that was removed, one that was only moved and one that was untouched: the link count stays 2 for all three, F_GETPATH keeps the old path for the removed one, openat("..") and statat(".") succeed for all three, and only creating an entry in the removed one fails (ENOENT). That last signal is a write into a directory that, in the attack case, an actor moved away, and it fails for lack of permission on a read-only directory, so it 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 with entries) and before anything is moved; the directory cases run on Linux and the other systems pin Unsupported. Whether and how macOS gets directory removal is left to the decision about the production use (H1b). The leftover recovery (files) is unaffected and runs on macOS.

Limits, stated

The same-user residual is unchanged and not accepted for production: the unlinkat(name) window, the link-count heuristic, and a foreign entry's identity without a held handle (see #359). A removed empty directory that wins the last window is the one case the removal reports as Disturbed after the fact. Owner-gated before any production use (H1b).

Local evidence, hosted CI and review pending

cargo fmt --check; cargo clippy --all-targets -D warnings on the host and compile-only for aarch64-apple-darwin and x86_64-pc-windows-msvc; the pinned:: unit tests on Linux (the macOS and Windows runs are the hosted evidence); canonical pnpm run ci:prepush; pr:budget against main: CAUTION, OK, 8 commits (two of them the temporary macOS probe and its correction), 9 files, 775 meaningful lines. Hosted Windows/macOS/Ubuntu evidence, CI, CodeQL and independent review are pending; nothing from #1070 carries over.

Part of #359.

Summary by CodeRabbit

  • New Features

    • Linux now supports safely removing empty directories and recovering quarantined leftovers. Leftover files are removed only after authentication; empty leftover directories can also be removed.
    • On other Unix platforms, attempts to remove an empty directory return an unsupported-operation error without moving it.
    • Directory changes during removal are detected, and affected directories are synchronized.
  • Documentation

    • Updated platform support and recovery behavior documentation.

…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.
@sourcery-ai

sourcery-ai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Reviewer's Guide

This PR finishes the Unix directory-removal slice and crash-leftover recovery by routing directories through descriptor- and identity-checked quarantine handling, and by safely authenticating and synchronizing leftover cleanup. Extensive test-support coverage exercises race windows, crash and stranded states, malformed names, foreign objects, links, and durability failures; Windows remains unsupported.

Sequence diagram for Unix directory removal

sequenceDiagram
    participant Caller
    participant PinnedDir
    participant Quarantine
    participant Filesystem

    Caller->>PinnedDir: remove_dir(name, observer)
    PinnedDir->>Filesystem: open_dir(name)
    Filesystem-->>PinnedDir: held directory identity
    PinnedDir->>Filesystem: entries(0)
    alt directory is not empty
        Filesystem-->>PinnedDir: TooManyEntries
        PinnedDir-->>Caller: NotEmpty
    else directory is empty
        PinnedDir->>Quarantine: create()
        PinnedDir->>Filesystem: move entry with identity check
        Quarantine->>Filesystem: unlinkat(AT_REMOVEDIR)
        Quarantine-->>PinnedDir: removal outcome
        PinnedDir-->>Caller: RemovalOutcome
    end
Loading

Sequence diagram for Unix crash-leftover recovery

sequenceDiagram
    participant Caller
    participant PinnedDir
    participant Quarantine
    participant Authenticator
    participant Filesystem

    Caller->>PinnedDir: remove_leftover(quarantine, authenticate)
    PinnedDir->>Quarantine: open_existing(quarantine)
    Quarantine->>Filesystem: validate generated name
    Quarantine->>Filesystem: open_owned directory
    Filesystem-->>Quarantine: directory handle and identity
    Quarantine->>Filesystem: inspect file entry
    alt foreign object, link, or authentication refused
        Quarantine->>Authenticator: authenticate(file)
        Authenticator-->>Quarantine: false or refused object
        Quarantine-->>PinnedDir: Refused
    else accepted regular file
        Quarantine->>Authenticator: authenticate(file)
        Authenticator-->>Quarantine: true
        Quarantine->>Filesystem: unlink verified file
        Quarantine->>Filesystem: sync quarantine directory
        Filesystem-->>Quarantine: sync result
        Quarantine-->>PinnedDir: Removed or NotConfirmed
    else empty quarantine directory
        Quarantine-->>PinnedDir: Vanished
    end
    PinnedDir-->>Caller: RemovalOutcome
Loading

File-Level Changes

Change Details Files
Implemented Unix empty-directory removal using descriptor-backed quarantine and identity checks.
  • Opens directories without following links and rejects non-empty directories.
  • Moves the held directory through quarantine, verifies the held identity, and removes it with directory-specific unlink semantics.
  • Reports swaps, late fills, and last-window races without deleting the wrong object.
crates/worldscript-secure-storage/src/pinned/unix/removal.rs
crates/worldscript-secure-storage/src/pinned/tests/remove.rs
crates/worldscript-secure-storage/src/pinned/tests/remove_unix_dir.rs
Added authenticated recovery for crash and failed-put-back quarantine leftovers.
  • Validates the generated quarantine name before opening anything.
  • Requires an owner-only quarantine directory and re-authenticates a regular file before unlinking it.
  • Leaves unauthenticated, foreign, linked, or otherwise non-regular entries in place; removes empty quarantine directories.
  • Syncs the quarantine directory after unlink and returns NotConfirmed when that sync fails.
crates/worldscript-secure-storage/src/pinned/unix/removal.rs
crates/worldscript-secure-storage/src/pinned/tests/remove_unix_dir.rs
Completed the re-cut’s identity and test-support integration across documentation and test infrastructure.
  • Records existing quarantine-directory identity for cleanup through the same checked path as created quarantines.
  • Exposes shared Unix test helpers for race and inode fixtures.
  • Adds coverage for swaps, fills, crash recovery, stranded entries, name grammar, foreign objects, links, and sync failures.
  • Updates module documentation, changelog, and durable-evidence records to mark Unix directories and leftovers as implemented while retaining Windows Unsupported behavior.
crates/worldscript-secure-storage/src/pinned/unix/removal.rs
crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs
crates/worldscript-secure-storage/src/pinned/tests/mod.rs
crates/worldscript-secure-storage/src/pinned/tests/remove_unix_dir.rs
crates/worldscript-secure-storage/src/pinned/removal.rs
CHANGELOG.md
docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@vercel

vercel Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
worldscript-studio Ready Ready Preview Oct 16, 2026 1:43am UTC

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Warning

Review limit reached

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

Next included review available in 35 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 62 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: qnbs/WorldScript-Studio/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: d16ec19e-c69a-4df8-b011-291cc0e6c95f
📥 Commits

Reviewing files that changed from the base of the PR and between ec7ba93 and 3a08350.

📒 Files selected for processing: 2
  • CHANGELOG.md
  • docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md
📝 Walkthrough

Unix directory removal now uses quarantine on Linux and returns Unsupported on other Unix systems before moving an empty directory. Quarantine recovery validates generated names, reauthenticates files, and handles empty directory leftovers on Linux. Tests and documentation cover platform behavior, identity checks, recovery outcomes, and directory syncing.

Changes: Unix Removal and Recovery

Layer / File(s) Summary
Quarantine-based directory removal
crates/worldscript-secure-storage/src/pinned/unix/removal.rs, crates/worldscript-secure-storage/src/pinned/tests/remove.rs, crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs, crates/worldscript-secure-storage/src/pinned/tests/remove_unix_dir.rs, crates/worldscript-secure-storage/src/pinned/tests/remove_linux_dir.rs, crates/worldscript-secure-storage/src/pinned/tests/mod.rs, crates/worldscript-secure-storage/src/pinned/removal.rs, docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md
Directory removal checks for entries and uses quarantine on Linux. If a held directory becomes non-empty, the code restores it and checks its identity to distinguish NotEmpty from Disturbed. Tests cover platform behavior, links, missing names, swaps, and late additions.
Validated leftover-file recovery
crates/worldscript-secure-storage/src/pinned/unix/removal.rs, crates/worldscript-secure-storage/src/pinned/tests/remove_unix_dir.rs, crates/worldscript-secure-storage/src/pinned/tests/remove_linux_dir.rs, crates/worldscript-secure-storage/src/pinned/removal.rs, CHANGELOG.md, docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md
Recovery validates generated quarantine names and reauthenticates files before unlinking. Linux recovery removes empty directory leftovers without authentication. Tests cover crashes, refused entries, invalid names, symlinks, and sync failures.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to ec7ba

The PR description gives the wrong expectation for Linux crash recovery. Correct it before merging; no implementation failure was established.

- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@codeant-ai

codeant-ai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

🤖 CodeAnt AI — Review Status

Status Commit Started (UTC) Finished (UTC)
✅ Incremental review completed ec7ba93 Oct 16, 2026 · 01:18 01:18
✅ Reviewed your PR 01917c1 Oct 15, 2026 · 23:57 00:02

@codeant-ai

codeant-ai Bot commented Oct 15, 2026

Copy link
Copy Markdown

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@codeant-ai

codeant-ai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

🏁 CodeAnt Quality Gate Results

Commit: 3a083508
Scan Time: 2026-10-16 01:43:37 UTC

✅ Overall Status: PASSED

Quality Gate Details

Quality Gate Status Details
Secrets ✅ PASSED 0 secrets found
Duplicate Code ✅ PASSED 0.0% duplicated
SAST ✅ PASSED No security issues
Bugs ✅ PASSED Rating S: No bugs
IAC ✅ PASSED No IAC issues

View Full Results

@codeant-ai codeant-ai Bot added the size:L This PR changes 100-499 lines, ignoring generated files label Oct 15, 2026
codescene-access[bot]

This comment was marked as outdated.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

[check-pr-size] PR size is over the target tier (normal profile): 9 files, 776 meaningful lines, 9 commits — limit ≤8 files / ≤400 lines / ≤6 commits. Consider splitting into smaller, independently reviewable PRs.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 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="402" />
<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>
**Empty quarantine reappears after crashes**

When authentication refuses or errors, or inspection errors, while another actor removes the held entry before `held.discard`, `leftover_step` returns `Unchanged` or an error, but `held.discard` removes the empty quarantine directory. `settle` skips syncing the parent for `Unchanged`, and `step?` returns before `settle` on errors, so the caller gets `Refused` or an error while a crash can restore the empty quarantine directory.

Sync the parent whenever `held.discard` removes the quarantine directory, including when `leftover_step` is unchanged or returns an error, while preserving the operation result.

Also at `crates/worldscript-secure-storage/src/pinned/unix/removal.rs:572`.
</issue_to_address>

### Comment 2
<location path="crates/worldscript-secure-storage/src/pinned/unix/removal.rs" line_range="373-391" />
<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 {
</code_context>
<issue_to_address>
**Stranded directories cannot be recovered**

When a crash or failed put-back leaves a directory inside a quarantine directory, a crash or blocked put-back during `remove_dir` leaves the directory as `x` in quarantine, but `leftover_step` inspects only `EntryKind::File` and treats the directory as foreign, so `remove_leftover_file` returns `Refused` and leaves the stranded quarantine entry in place.

Provide recovery for an empty stranded directory, or otherwise ensure the stranded-directory outcome has a supported cleanup path.

Also at `crates/worldscript-secure-storage/src/pinned/unix/removal.rs:413-417`.
</issue_to_address>

Sourcery assessment

Needs a human reviewer. 2 findings to address first, and if the leftover cleanup identifies the wrong object or unlinks one that should have been retained, that file is permanently deleted; reverting prevents future removals but cannot restore files already unlinked. The implementation is test-support-only, which limits where it can run but does not make an executed deletion reversible.

Blocking findings: crates/worldscript-secure-storage/src/pinned/unix/removal.rs:402, crates/worldscript-secure-storage/src/pinned/unix/removal.rs:391


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

Comment thread crates/worldscript-secure-storage/src/pinned/unix/removal.rs Outdated
Comment thread crates/worldscript-secure-storage/src/pinned/unix/removal.rs
@codeant-ai

codeant-ai Bot commented Oct 16, 2026

Copy link
Copy Markdown

CodeAnt Nitpicks

1 code suggestion

1. discard can remove the quarantine directory after a refused file disappears concurrently, but Unchanged skips syncing the parent, leaving that removal unconfirmed.

Race condition · crates/worldscript-secure-storage/src/pinned/unix/removal.rs:402

@codeant-ai

codeant-ai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

CodeAnt PR Risk: Low Risk

  • The PR appears safe to merge; the new removal behavior is confined to test-support and does not enter production builds.
  • Linux directory removal and Unix leftover recovery include tests for swaps, crashes, authentication, and sync failures.

Assessed commit: ec7ba9317b67

…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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 01917c1c65

ℹ️ 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".

Comment thread crates/worldscript-secure-storage/src/pinned/unix/removal.rs Outdated
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-16T01:47:17.340542Z 3a08350 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

codescene-access[bot]

This comment was marked as outdated.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5d62328e5b

ℹ️ 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".

Comment thread crates/worldscript-secure-storage/src/pinned/unix/removal.rs Outdated
…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.
codescene-access[bot]

This comment was marked as outdated.

…ved 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.
codescene-access[bot]

This comment was marked as outdated.

…e the directory is removed (#445)

Still temporary and removed again in the next commit.
codescene-access[bot]

This comment was marked as outdated.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8e294a2cf4

ℹ️ 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".

Comment thread crates/worldscript-secure-storage/src/pinned/tests/macos_probe.rs Outdated
… 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.
codescene-access[bot]

This comment was marked as outdated.

@codecov

codecov Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

…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.
@codeant-ai codeant-ai Bot removed the size:L This PR changes 100-499 lines, ignoring generated files label Oct 16, 2026
codescene-access[bot]

This comment was marked as outdated.

@codeant-ai codeant-ai Bot added the size:XL This PR changes 500-999 lines, ignoring generated files label Oct 16, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository: qnbs/WorldScript-Studio/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: e2876f84-a6fe-490d-baec-746c2fe4bb44
📥 Commits

Reviewing files that changed from the base of the PR and between 5d62328 and ec7ba93.

📒 Files selected for processing: 8
  • CHANGELOG.md
  • crates/worldscript-secure-storage/src/pinned/removal.rs
  • crates/worldscript-secure-storage/src/pinned/tests/mod.rs
  • crates/worldscript-secure-storage/src/pinned/tests/remove.rs
  • crates/worldscript-secure-storage/src/pinned/tests/remove_linux_dir.rs
  • crates/worldscript-secure-storage/src/pinned/tests/remove_unix_dir.rs
  • crates/worldscript-secure-storage/src/pinned/unix/removal.rs
  • docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Comment thread CHANGELOG.md Outdated
…nly 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.

@codescene-access codescene-access Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gates Passed
3 Quality 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 branch was successfully deployed

1 active deployment
Preview — 3a083508 Deployed Oct 16, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XL This PR changes 500-999 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant