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

[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

[Copilot]

🔵 Needs a closer look

Destructive native filesystem interop and platform-specific ABI behavior require final human review on every supported operating system.

Review details

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

[Copilot]

🟡 Changes recommended

TUI revalidation can authorize a replaced object, and lazy cleanup validation misclassifies duplicate selections.

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

Review details

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

[Copilot]

🟡 Changes recommended

Modal teardown can race active work, and CLI cleanup can falsely report duplicate candidates as environment failures.

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

Review details

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

[Copilot]

🟡 Changes recommended

Unix validation can cross mounted directories outside the scan-root authority, and canceled cleanup can misreport duplicate skips as failures.

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

Review details

Suppressed comments (1)

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

• The duplicate skip count is only held in this local iterator. If RemoveManyAsync throws for cancellation after deduplication, no report is returned and the outer cancellation handler only sees RemoveProgress, which has no skipped count; it consequently reports every duplicate as failed (selectedCount - removed). Preserve the pre-validation skip count across the cancellation path or include it in cancellation progress/accounting so deduplicated selections remain skips.

        var validationSkipped = 0;
        var report = await _remove.RemoveManyAsync(
            ValidateImmediatelyBeforeRemoval(
                selected,
                cancellationToken,
                () => Interlocked.Increment(ref validationSkipped)),

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

[harder]

Also addressed the suppressed CleanupScreen cancellation-accounting finding from review 5071393867 in 44bdfa8. Deduplication now publishes its pre-validation skip count into per-attempt state before removal begins; the cancellation handler subtracts those known skips from failures and reports removed, skipped, and failed counts separately even when RemoveManyAsync throws before returning a batch report. A regression cancels at the first native deletion boundary with two classifications for one path and verifies the duplicate remains a skip. The full local and PR verification matrix is green.

[Copilot]

🔵 Needs a closer look

Compact-path validation still performs native filesystem inspection synchronously on the Terminal.Gui thread.

Review details

Suppressed comments (1)

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

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

• This native capture is still reached synchronously from SkillViewWorkflowCoordinator.OpenRemoveDialogInternal (RemoveTargetResolver.Evaluate at line 338), which is invoked directly by the Installed tab's onRemove UI callback. Therefore opening the remove flow performs handle opens, directory enumeration, and policy inspection on the Terminal.Gui thread before the new background evaluation in RemoveScreen can run; slow or disconnected filesystem roots can freeze the TUI. Move the compact-eligibility evaluation behind an owned background operation (or enter RemoveScreen without synchronously evaluating first).

                && SecureRemovalBackend.TryCaptureDirectoryValidationWithinRoot(
                    matchedRootByRawPath.Path,
                    target.ResolvedPath,
                    out var rootedSnapshot,
                    out identityError))

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