fix(trash): preserve dangling symlinks across filesystems - #117
Conversation
clawsweeper
Bot
added
proof: sufficient
|
Codex review: needs maintainer review before merge. Reviewed August 3, 2026, 2:48 AM ET / 06:48 UTC. ClawSweeper reviewWhat this changesThe PR recreates symbolic links rather than dereferencing them in the cross-filesystem trash fallback, adds forced-EXDEV dangling-link coverage, and removes ambient-umask assumptions from three tests. Merge readinessThis PR addresses a real current-release defect in the cross-filesystem trash fallback: current Priority: P1 Review scores
Verification
How this fits togetherThe trash helper moves a caller-supplied path from an allowed filesystem root into a private trash directory under the user home directory. It first performs an identity-guarded rename and, only for cross-filesystem moves, uses a copy-and-guarded-delete fallback. flowchart LR
A[Caller path] --> B[Allowed-root and identity guard]
B --> C[Private trash destination]
C --> D{Guarded rename}
D -->|Succeeds| E[Moved entry]
D -->|EXDEV| F[Copy fallback]
F --> G[Identity recheck and guarded removal]
G --> E
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the explicit readlink-and-recreate fallback after the Windows validation result is resolved, keeping the identity rechecks and forced-EXDEV regression coverage intact. Do we have a high-confidence way to reproduce the issue? Yes. Forcing the guarded rename to fail with EXDEV for a dangling symlink reaches current main's cpSync fallback, whose source-level behavior attempts to follow the missing target; the branch's regression test captures that exact path. Is this the best way to solve the issue? Yes. Recreating only the already-identified symlink from readlink, while retaining the existing identity checks and ordinary cpSync path, is the narrowest maintainable fix. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b8738eb59b29. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
cdbb504 to
33e25d0
Compare
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(trash): preserve dangling symlinks across filesystems This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
1 participant
Summary
EXDEVcopy pathLinux reproduction
Tested unmodified
origin/mainin an unprivileged Debian 12 arm64 container with Node 22.23.2, pnpm 10.34.5, and a locally built native binding.TMPDIR=/dev/shm pnpm test test/edge-coverage.test.ts -t "moves broken symlinks to trash"failed withENOENTatsrc/trash.ts:166because/dev/shmand the home trash are different filesystems.umask 0027; pnpm testexposed three ambient-mode assertions: the ZIP staging assertion and two symlink-parent fixture-mode assertions.0022passed on the unmodified baseline, confirming why these cases stay hidden in common environments.After the fix, the full suite passed with both conditions active:
TMPDIR=/dev/shm, umask0027, 1,061 tests passed and 26 platform tests skipped.Verification
pnpm checkpnpm test:security(70 passed)git diff --checkThe Linux proof used OrbStack's local arm64 container backend. It did not cover x86_64 Linux or a remote crabbox host.