Harden removal with native filesystem identities

[harder]

Summary

Implements the remaining native removal-security batch from the adversarial concurrency, resource, cancellation, and cross-platform audit.

Skill removal no longer relies on repeated path validation followed by path-based recursive deletion on the three supported release platforms. Validation pins the selected filesystem object, and execution traverses opened directory handles/descriptors while verifying native identities.

Why

The portable removal hardening in PRs #12 through #16 prevented static nested-link escapes, bounded traversal state, added cancellation, and moved filesystem work off the UI thread. One deliberate security follow-up remained: another process running as the same user could replace a path component between a managed validation check and a path-based delete.

Supported .NET 10 deletion APIs cannot close that check/use interval. This change introduces audited platform backends instead of adding another path check.

What changed

Validation-time identity pinning

• Every allowed removal captures the selected target native identity before confirmation completes.
• Windows records the volume serial number and file ID.
• macOS and Linux record the device and inode after canonicalizing the full path.
• If the identity cannot be captured, validation refuses removal instead of silently downgrading to the old path-based behavior.
• Execution opens the target again and refuses it if the identity, type, or reparse-point state changed.

Windows opened-handle removal

• Opens directories and reparse points with CreateFileW using backup and open-reparse-point semantics.
• Enumerates each opened directory handle with GetFileInformationByHandleEx(FileIdBothDirectoryInfo).
• Opens every observed child and compares its volume, file ID, directory bit, and reparse-point bit before descent or deletion.
• Treats junctions and symbolic links as leaves.
• Deletes the opened object with SetFileInformationByHandle(FileDispositionInfoEx), with the legacy disposition class used only when the extended class is unavailable.
• A renamed or replaced ancestor pathname cannot redirect enumeration or deletion because the operation remains bound to the opened object.

macOS and Linux descriptor-relative removal

• Canonicalizes the validated target once, then opens every path component with no-follow directory semantics.
• Enumerates duplicated opened directory descriptors through fdopendir/readdir.
• Compares st_dev, st_ino, and file type before descent and again before directory removal.
• Uses openat and unlinkat relative to held parent directory descriptors.
• Linux child-directory opens use openat2 with RESOLVE_BENEATH, RESOLVE_NO_SYMLINKS, and RESOLVE_NO_XDEV, rejecting both bind mounts and filesystem mount crossings.
• macOS rejects a directory whose device differs from the selected target.
• Symlinks, broken links, and other non-directory entries remain leaves and are never followed.

Cancellation, memory, and resource bounds

• Preserves the existing cancellation checks between entries and immediately before descent/deletion.
• Preserves throttled progress, exact partial-mutation counts, batch accounting, and the 128-detail error cap.
• Retains O(depth) traversal state with the existing maximum depth of 256.
• Windows retains one 1 KiB enumeration buffer per active depth.
• Unix retains one descriptor and directory stream per active depth plus one reusable 512-byte stat buffer for the entire operation.
• Every opened native handle, descriptor, directory stream, and native buffer has deterministic disposal on success, failure, and cancellation.

Adversarial coverage

New deterministic tests prove that:

• Replacing the selected target after validation causes a hard identity refusal; neither the original object nor the replacement is deleted.
• Replacing an already-observed child directory with an external directory link cannot redirect traversal; both the original child data and external data survive.

The existing suite continues to cover external directory links, external file links, broken links, ancestor cycles, retargeted links, Windows junctions, cancellation during traversal, partial progress, 2,000-file stress trees, and bounded error reporting.

Precise Unix security boundary

This PR intentionally does not claim a guarantee POSIX does not expose. unlinkat removes a final name relative to a held parent descriptor; Unix has no general operation that unlinks an already-open inode directly. A process with the same UID can theoretically replace a non-directory leaf between the final fstatat identity comparison and unlinkat.

That narrow interval cannot redirect recursive traversal through an ancestor or replacement directory. A replacement symlink is unlinked rather than followed, and a replacement directory is not recursively traversed or removed as a file. Windows deletion is bound directly to the opened object handle.

The full audit and durable agent guidance now record this distinction rather than describing Unix deletion as completely atomic.

User-visible release notes

• Skill removal now verifies that the selected installation is the same filesystem object the user confirmed.
• Concurrent path replacement can no longer redirect recursive removal into a different directory tree.
• Nested mount points are refused instead of traversed on Linux, and cross-device nested directories are refused on macOS.
• Existing cancellation, progress, and partial-failure reporting behavior is preserved.

Validation

• dotnet build --no-restore — passed; only the known sandbox-local NuGet vulnerability-cache warning was emitted.
• dotnet test --no-build — 685 passed, 0 failed, including ANSI-driver integration tests.
• dotnet format SkillView.sln --no-restore --verify-no-changes — passed; workspace loading emitted its existing non-failing warning.
• git diff --check — passed.
• macOS ARM64 Native AOT publish — passed for the standalone app.
• macOS ARM64 Native AOT publish — passed for the gh extension.

Review focus

• Windows FILE_ID_BOTH_DIR_INFO parsing and opened-handle disposition behavior on NTFS/ReFS.
• Linux openat2 availability and RESOLVE_NO_XDEV behavior in the CI environment.
• macOS stat/dirent layouts and descriptor ownership.
• Cancellation and cleanup while multiple traversal frames are open.
• The documented Unix final-leaf limitation versus the stronger recursive-traversal guarantee.

[Copilot]

🟡 Changes recommended

Critical containment, ReFS identity, replacement handling, and cancellation issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds native, identity-pinned removal backends to harden deletion against concurrent path replacement.

Changes:

• Adds Windows handle-based and Unix descriptor-relative traversal.
• Pins filesystem identities during validation.
• Adds adversarial tests and updates security guidance.

File summaries

File	Description
AGENTS.md	Records native-removal requirements.
agent_docs/ui-lifecycle-and-resource-bounds.md	Documents traversal guarantees and bounds.
docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md	Updates audit status and security boundary.
SecureRemovalBackend.cs	Defines shared identity and backend dispatch.
UnixSecureRemovalBackend.cs	Implements descriptor-relative Unix removal.
WindowsSecureRemovalBackend.cs	Implements opened-handle Windows removal.
RemoveValidator.cs	Captures target identity during validation.
RemoveService.cs	Routes deletion through secure backends.
RemoveServiceTests.cs	Adds replacement-race tests.

Review details

Suppressed comments (3)

src/SkillView.Core/Inventory/WindowsSecureRemovalBackend.cs:172

• A cancellation that arrives while the final enumeration call is completing still allows this directory handle to be marked for deletion. Recheck the token immediately before the destructive call so cancellation does not perform another directory mutation.

                    if (!TryDeleteHandle(frame.Directory, out var deleteError))

src/SkillView.Core/Inventory/UnixSecureRemovalBackend.cs:212

• This directory deletion follows enumeration and fstatat without a final cancellation check. Cancellation during either native operation can therefore still mutate the filesystem. Recheck the token after the identity comparison and immediately before unlinkat.

                    else if (UnixNative.unlinkat(
                            frame.Parent.FileDescriptor,
                            frame.Name,
                            UnixConstants.RemoveDirectoryFlag) != 0)

src/SkillView.Core/Inventory/UnixSecureRemovalBackend.cs:276

• The token is checked before the second stat, but cancellation during that call still permits the link to be unlinked. Add the required final check at the destructive boundary.

            if (UnixNative.unlinkat(parent.FileDescriptor, name, flags: 0) != 0)

• Files reviewed: 9/9 changed files
• Comments generated: 9
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Identity-less cleanup deletion remains vulnerable to replacement, and Linux ARM64 stat parsing uses an incompatible layout.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/SkillView.Core/Inventory/UnixSecureRemovalBackend.cs:488

• This Linux layout assumes x86-64 struct stat, where st_mode is at offset 24. On Linux ARM64, st_mode is at offset 16 and offset 24 is st_uid; because the backend is enabled for every Linux architecture, directories are misclassified and secure removal is refused. Use architecture-specific native layouts (or explicitly gate unsupported architectures) instead of a single hard-coded Linux layout.

        return new NativeStat(
            unchecked((ulong)Marshal.ReadInt64(buffer, 0)),
            unchecked((ulong)Marshal.ReadInt64(buffer, 8)),
            unchecked((uint)Marshal.ReadInt32(buffer, 24)));

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Linux ARM64 stat parsing is incorrect, and Windows opens request unnecessary permissions that can reject valid removals.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/SkillView.Core/Inventory/WindowsSecureRemovalBackend.cs:280

• Every open requests FILE_WRITE_ATTRIBUTES, although identity reads need read attributes and FileDispositionInfo(Ex) requires DELETE, not attribute-write access. An ACL can therefore allow the removal while denying attribute writes, causing validation or traversal to fail at CreateFileW unnecessarily. Drop this access bit (and its now-unused constant) so the backend does not impose permissions unrelated to the operation.

        var desiredAccess = DeleteAccess
            | FileReadAttributes
            | FileWriteAttributes
            | SynchronizeAccess;

• Files reviewed: 12/12 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Windows canonicalization and identityless link cleanup leave path-redirection gaps, while unavailable openat2 can cause partial deletion.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 13/13 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Several remaining race windows can authorize or redirect deletion of replacement filesystem entries.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/SkillView.Core/Inventory/RemoveValidator.cs:344

• The brokenness check is not bound to the identity captured afterward. A broken cleanup link can be replaced with a valid symlink after Resolve returns null but before TryCaptureLinkIdentity; validation then pins and authorizes deletion of the new valid link. Check brokenness through the captured parent/link boundary, or recapture and compare identities around the target-resolution check, so ValidateBrokenSymlink cannot approve a different entry.

        else if (requireBroken && PathResolver.Resolve(fullPath) is not null)
        {
            errors.Add(new Error(ErrorKind.NotASkillDirectory,
                $"'{fullPath}' is no longer broken"));
        }

• Files reviewed: 16/16 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Object-policy and broken-link checks remain vulnerable to concurrent pathname replacement.

Review details

Suppressed comments (2)

src/SkillView.Core/Inventory/RemoveValidator.cs:347

• The brokenness check and identity capture apply to different observations. If the original broken link is replaced with a valid link after Resolve returns null but before TryCaptureLinkIdentity, validation pins the valid replacement and cleanup later deletes it even though it is no longer a broken-link candidate. Capture first and verify brokenness through the same held parent/link identity (for example, descriptor-relative readlinkat on Unix and the opened reparse handle on Windows), failing if that object changes.

        else if (requireBroken && PathResolver.Resolve(fullPath) is not null)
        {
            errors.Add(new Error(ErrorKind.NotASkillDirectory,
                $"'{fullPath}' is no longer broken"));
        }
        else if (matchedRootByRawPath is not null)
        {
            if (SecureRemovalBackend.TryCaptureLinkIdentity(

src/SkillView.Core/Inventory/RemoveValidator.cs:92

• The captured identity does not bind the subsequent skill/.git policy checks to this object. A same-user process can rename an ancestor after capture, put a clean skill at the same canonical pathname while LooksLikeSkill and the .git check run, then restore the original ancestor; execution will match the stored target identity (an ancestor rename does not change the target's Unix ctime) and can delete an object whose policy was never evaluated, such as an in-place clone. Keep the opened target handle/descriptor through the object-local policy checks and inspect SKILL.md and .git relative to it; pathname rechecks cannot close this ABA swap.

                executionIdentity = capturedIdentity;
                resolved = capturedIdentity.CanonicalPath;

• Files reviewed: 17/17 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[harder]

Addressed the two suppressed “needs a closer look” findings in commit 279a974, with Windows regression-test corrections in d5eefbf and 24abfcf.

• Directory-policy ABA: validation now captures native identity, canonical path, SKILL.md presence, .git directory presence, and emptiness through the same held directory handle or descriptor, with a generation recheck after inspection. Policy validation no longer reopens the canonical pathname.
• Broken-link ABA: target existence is observed relative to the held parent and final name, bracketed by parent and link identity checks. Replacing a broken link with a valid link during validation now fails closed.
• Analogous permissions issue: removed the unrelated FILE_WRITE_ATTRIBUTES access request from Windows opens; deletion retains only the access required for identity inspection, synchronization, and disposition.
• Regression coverage: added deterministic Unix and Windows tests for directory and path replacement and broken-to-valid link replacement, plus validator-level coverage.
• Durable guidance: updated AGENTS.md, the lifecycle and resource guide, and the adversarial audit with the opened-object policy boundary.

Verification is green: 727 local Release tests, formatting and build checks, sequential local Native AOT publishes, and all GitHub test, CodeQL, and AOT jobs on macOS, Linux, and Windows.

[Copilot]

🔵 Needs a closer look

Native destructive filesystem code warrants human platform review, and Linux canonicalization mishandles valid names ending in (deleted).

Review details

• Files reviewed: 17/17 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Root-binding races, batch link invalidation, and platform path edge cases remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/SkillView.Core/Ui/CleanupScreen.cs:384

• These identities are captured for the whole cleanup batch before RemoveManyAsync starts. Removing the first selected entry changes its parent directory's st_ctim/ChangeTime, so a second broken symlink in the same parent will fail with "parent identity changed" even without any external race. Validate each link candidate immediately before its removal, or refresh remaining validations after mutations owned by this batch.

                CleanupClassifier.CandidateKind.BrokenSymlink =>
                    RemoveValidator.ValidateBrokenSymlink(candidate.Path, _scanRoots),
                CleanupClassifier.CandidateKind.EmptyDirectory =>
                    RemoveValidator.ValidateEmptyDirectory(candidate.Path, _scanRoots),

src/SkillView.Core/Inventory/WindowsSecureRemovalBackend.cs:763

• Only DOS drive paths can safely lose the \\?\ prefix. GetFinalPathNameByHandleW can return \\?\Volume{GUID}\... for a volume without a drive letter; stripping its prefix produces a relative Volume{GUID}\... path, so containment and execution resolve against the process working directory and valid removals fail. Preserve extended paths that are not drive-letter paths.

        return path.StartsWith(extendedPrefix, StringComparison.OrdinalIgnoreCase)
            ? path[extendedPrefix.Length..]
            : path;

src/SkillView.Core/Inventory/UnixSecureRemovalBackend.cs:268

• A filesystem-root scan root cannot be canonicalized here. For /, RealPath returns /, then OpenAbsoluteDirectory reaches OpenAbsoluteParent, whose empty component list throws "refusing to remove a filesystem root"; consequently every removal under a valid --scan-root / is rejected by the canonical containment check. This non-destructive operation can open the resolved directory directly and still derive the final path from its handle.

            var resolvedPath = RealPath(path);
            using var handle = OpenAbsoluteDirectory(resolvedPath, out var parent, out _);
            using (parent)
            {
                canonicalPath = ReadFinalPath(handle);
            }

• Files reviewed: 17/17 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Windows legacy disposition uses an incorrect native field width and lacks a cancellation check immediately before fallback deletion.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 18/18 changed files
• Comments generated: 2
• Review effort level: Balanced