Harden asynchronous removal lifecycles

[harder]

Summary

Completes the next recommended audit batch by moving every TUI removal path off Terminal.Gui's event loop and giving removal work explicit cancellation, progress, bounded error retention, and disposal-safe ownership.

This covers compact skill removal, the advanced removal wizard, cleanup batches, cleanup validation, and agent-link unlinking. It builds on the existing O(depth), reparse-point-aware traversal from PR #12.

What changed

Asynchronous, cancellable removal

• Added RemoveAsync, RemoveManyAsync, and RemoveLinkAsync thread-pool boundaries around synchronous filesystem APIs.
• Preserved cancellation checks between lazy enumeration entries and destructive operations.
• Kept traversal memory at O(depth); no complete file or directory tree is materialized.
• Added cancellation-terminal progress for both mid-target and between-target cancellation.

Bounded progress and memory

• Added a shared removal progress model with aggregate target, file, directory, and error counts.
• Throttled progress delivery to at most 10 updates per second, with forced completion/cancellation updates.
• Added an inline callback progress adapter so the TUI explicitly owns UI dispatch instead of creating an implicit Progress<T> queue.
• Bounded retained runtime error details to 128 entries plus an omission summary while preserving the exact ErrorCount.
• Added runtimeErrorCount to remove JSON output so automation can distinguish the exact total from the bounded detail array.

TUI lifecycle safety

• Compact remove, advanced remove, and cleanup now show an inline spinner beside live progress text.
• Esc cancels active removal and waits for traversal quiescence before the owning dialog/window is disposed.
• Each removal UI captures a stable token, retains its active task, rechecks lifetime inside queued UI callbacks, and contains UI-dispatch failures.
• Operation controls cannot start overlapping removals; retry progress state is reset so cancellation cannot reuse stale counts.
• Cleanup validation now also runs off the UI thread.

Partial-cancellation correctness

• Partial file/directory deletion is retained in the cancellation report even when a whole target did not finish.
• The workflow invalidates and rescans inventory after any filesystem change, including canceled operations with zero fully completed targets.
• Cleanup tracks target, file, and directory changes separately so partial work still refreshes the underlying tabs.

Agent unlink hardening

• Agent-link unlinking now uses the same asynchronous removal service.
• The path is rechecked as a symlink immediately before deletion and the target is never followed.

Safety boundary

The portable implementation still cannot make path validation and path-based deletion atomic against a hostile process running as the same user. .NET 10's supported Windows and Unix deletion APIs remain path-based. An audited native handle-relative/no-follow implementation remains a separate Finding 1 security design, rather than being implied by this PR.

Tests

• Added coverage for asynchronous cancellation terminal progress.
• Added aggregate monotonic batch progress coverage.
• Added a regression for cancellation exactly between batch targets.
• Added progress-throttling and final-count coverage over a 2,000-file tree.
• Added agent-link unlink coverage proving the external target survives.
• Added bounded-error retention coverage with exact totals.
• Converted cleanup removal tests to exercise the asynchronous production path and verify target/file/directory accounting.
• Updated the remove JSON contract test for runtimeErrorCount.

Validation

• dotnet build --no-restore — passed. The only local warning was the existing sandbox-denied NuGet vulnerability-cache refresh; compilation completed successfully.
• dotnet test --no-build — 631 passed, 0 failed.
• dotnet format SkillView.sln --no-restore --verify-no-changes — passed.
• git diff --check — passed.
• macOS ARM64 native-AOT publish — passed for both standalone and gh-extension hosts.
• Native standalone and gh-extension --version smoke tests — passed.

Release notes

• Removal and cleanup no longer freeze the terminal UI while scanning or deleting large skill trees.
• Active removals show live inline progress and can be canceled with Esc.
• Canceled or partially failed removals now refresh inventory whenever any files changed, preventing stale Installed/Updates views.
• Cleanup batch progress and cancellation remain bounded even with large candidate sets.
• Extremely large failure sets no longer retain an unbounded list in memory; output reports the exact count and a bounded sample.
• Agent unlink operations now verify they are still deleting a link and never follow its target.

Remaining ordered audit work

1. Finish metadata-preview per-item deadlines and bounded scheduling.
2. Apply the disposal-safe owned-task pattern to the three install flows.
3. Wire root CLI cancellation and bounded post-kill waiting.
4. Centralize cross-platform path identity semantics.
5. Correct Esc focus behavior and strengthen the LRU contract test.
6. Complete the lower-risk hardening and stress coverage.

[Copilot]

🟡 Changes recommended

RemoveMany can publish a final canceled progress update that under-reports partial deletions when cancellation happens mid-target, which can cause callers to miss filesystem changes and skip a needed inventory rescan.

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

Pull request overview

This PR hardens SkillView’s destructive filesystem “remove/cleanup” flows by moving them off Terminal.Gui’s event loop and introducing explicit cancellation, throttled progress reporting, bounded error retention, and disposal-safe task ownership—improving UI responsiveness and lifecycle correctness during large removals.

Changes:

• Added async removal APIs (RemoveAsync, RemoveManyAsync, RemoveLinkAsync) with throttled progress + bounded runtime-error retention and surfaced exact runtimeErrorCount in JSON output.
• Updated TUI removal surfaces (compact modal, wizard, cleanup) to run removals on background tasks, show inline spinners/progress, support Esc cancellation, and drain tasks before disposing UI.
• Expanded/updated tests to cover cancellation terminal progress, monotonic aggregate progress, throttling, bounded errors, and agent-link unlink behavior.

File summaries

File	Description
tests/SkillView.Tests/Ui/CleanupScreenTests.cs	Migrates tests from reflective DoRemove invocation to the new async cleanup removal path and validates new counters/summary.
tests/SkillView.Tests/Inventory/RemoveServiceTests.cs	Adds coverage for async cancellation terminal progress, throttling, batch monotonic progress, bounded error retention, and link unlinking.
tests/SkillView.Tests/Cli/CliDispatcherJsonSnapshotTests.cs	Updates remove JSON contract expectations for runtimeErrorCount.
src/SkillView.Core/Ui/SkillViewWorkflowCoordinator.cs	Rescans inventory after any partial filesystem change (including cancellation) and uses exact error counts in status.
src/SkillView.Core/Ui/RemoveScreen.cs	Moves wizard removals off the UI thread with spinner/progress, Esc cancellation, and disposal-safe task draining.
src/SkillView.Core/Ui/RemoveConfirmModal.cs	Makes compact remove modal async/cancellable with spinner/progress and drains the owned removal task before disposal.
src/SkillView.Core/Ui/CleanupScreen.cs	Runs cleanup validation/removal asynchronously with throttled progress, Esc cancellation behavior, and new removed file/dir counters.
src/SkillView.Core/Ui/CallbackProgress.cs	Introduces an inline progress adapter to avoid Progress<T> queueing/sync-context ambiguity for TUI callers.
src/SkillView.Core/Inventory/RemoveService.cs	Adds progress model + throttling, async wrappers, link removal, bounded error retention w/ exact count, and batch progress aggregation.
src/SkillView.Core/Cli/CliDispatcher.cs	Prints exact runtime error counts and emits runtimeErrorCount in remove JSON.
docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md	Updates audit status to reflect completed portable async removal/lifecycle remediation.
AGENTS.md	Documents new repo-wide removal lifecycle requirements (async APIs, throttling, bounded errors, cancel+drain).
agent_docs/ui-lifecycle-and-resource-bounds.md	Documents removal lifecycle/resource bounds expectations alongside other UI lifecycle rules.

Review details

• Files reviewed: 13/13 changed files
• Comments generated: 2
• Review effort level: Lite

---

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

[Copilot]

🔵 Needs a closer look

Cancellation accounting and retry handling can misreport removals and skip required inventory rescans.

Review details

Suppressed comments (6)

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

src/SkillView.Core/Inventory/RemoveService.cs:35

• TargetsProcessed cannot represent how many targets were actually deleted: CompleteTarget increments it even when report.Succeeded is false, but the cancellation paths consume it as RemovedCount/TargetsDeleted. If one target fails and cancellation occurs before the next, the UI reports that failed target as removed and can trigger a no-change rescan. Track a separate aggregate TargetsDeleted value populated from report.Succeeded and use that in cancellation reports.

    public sealed record RemoveProgress(
        int TargetsProcessed,
        int FilesProcessed,
        int DirectoriesProcessed,
        int Errors,

src/SkillView.Core/Ui/RemoveScreen.cs:352

• This replaces LastReport with only the latest attempt's counts. A failed attempt can delete some files while leaving the target incomplete; if the user retries and the second attempt fails without deleting anything, closing the wizard leaves the coordinator seeing zero changes and skips the required inventory rescan. Preserve cumulative filesystem-change counts across retries while resetting only the displayed per-attempt progress.

                var report = await ExecuteAsync(evaluation, cancellationToken, progress)
                    .ConfigureAwait(false);
                LastReport = report;

src/SkillView.Core/Ui/RemoveConfirmModal.cs:195

• Assigning the latest result discards filesystem changes from earlier failed attempts. For example, an initial partial failure can delete files, a retry can fail with zero deletions, and then Cancel/Advanced returns only the zero-change report, so the coordinator does not invalidate or rescan inventory. Keep cumulative deletion counts (or a separate any-filesystem-change result flag) across retries.

                var completed = await _remove.RemoveAsync(
                    validation,
                    new RemoveService.Options(DryRun: false),
                    cancellationToken,
                    progress).ConfigureAwait(false);
                report = completed;

src/SkillView.Core/Ui/RemoveScreen.cs:379

• Cancellation progress already carries the exact number of failures observed so far, but this synthetic report resets ErrorCount to 1 by retaining only "removal canceled". If failures occurred before cancellation, the coordinator reports an incorrect total, contrary to the exact-count contract. Carry the observed error count into the report and include an omission summary for the unavailable details.

                var progressAtCancellation = lastProgress;
                LastReport = new RemoveService.BatchRemoveReport(
                    Succeeded: false,
                    TargetsDeleted: progressAtCancellation?.TargetsProcessed ?? 0,
                    FilesDeleted: progressAtCancellation?.FilesProcessed ?? 0,
                    DirectoriesDeleted: progressAtCancellation?.DirectoriesProcessed ?? 0,
                    Errors: ImmutableArray.Create("removal canceled"),
                    DryRun: false);

src/SkillView.Core/Ui/RemoveConfirmModal.cs:224

• This cancellation report also drops any failures counted by lastProgress and defaults ErrorCount to 1. A removal can record filesystem errors before the user presses Esc, so the returned Result.Report no longer preserves the exact runtime-error total. Carry progressAtCancellation.Errors into ErrorCount and add an omission summary for those logged details.

                var progressAtCancellation = lastProgress;
                report = new RemoveService.RemoveReport(
                    Succeeded: false,
                    ResolvedPath: validation.ResolvedPath,
                    FilesDeleted: progressAtCancellation?.FilesProcessed ?? 0,
                    DirectoriesDeleted: progressAtCancellation?.DirectoriesProcessed ?? 0,
                    Errors: ImmutableArray.Create("removal canceled"),
                    DryRun: false);

src/SkillView.Core/Cli/CliDispatcher.cs:1222

• The actual validation-refusal paths populate r.Errors from validation errors/warnings, so both the new count and runtimeErrors report runtime failures even though removal never ran (the test masks this by constructing an empty report). Gate both runtime fields on validation having passed and any required confirmation having been accepted.

            w.WriteNumber("runtimeErrorCount", r.ErrorCount);
            w.WriteStartArray("errors");

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

[Copilot]

🟡 Changes recommended

Refused removals lack terminal progress, and exceptional cleanup paths can lose partial-mutation accounting.

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/RemoveService.cs:102

• A refused validation publishes only the initial progress snapshot and returns without a terminal IsCompleted update. Direct RemoveAsync observers therefore see an operation that never finishes, unlike every other normal return path and the advertised forced terminal-progress contract. Publish a completed snapshot with one processed target, zero deleted targets, and the refusal error before returning.

        var progressTracker = new ProgressTracker(progress, _logger);
        progressTracker.Publish(0, 0, 0, 0, target, force: true);

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

[Copilot]

🔵 Needs a closer look

Compact-modal shortcuts can race queued completion callbacks and misreport or re-run completed removals.

Review details

Suppressed comments (2)

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

src/SkillView.Core/Ui/RemoveConfirmModal.cs:295

• A completed removal can still have its UI completion callback queued because IApplication.Invoke is asynchronous. In that window, this IsCompleted: false check falls through, overwrites the worker's Removed/Failed outcome with Cancelled, and closes the modal; a successful deletion is then reported as canceled. Treat any non-null operation as owned until its queued callback clears it (cancel only while it is still running).

This issue also appears on line 303 of the same file.

                if (activeOperation is { IsCompleted: false })
                {
                    lifetime.Cancel();
                    status.Text = " canceling removal…";
                    return;

src/SkillView.Core/Ui/RemoveConfirmModal.cs:307

• The same queued-completion race lets the a shortcut escalate to the wizard after the removal task has completed but before its UI callback runs. That can open the wizard against an already-deleted skill and replace the completed outcome. Keep blocking this shortcut while activeOperation is non-null; the failure callback clears it when escalation is safe again.

                if (activeOperation is { IsCompleted: false })
                {
                    status.Text = " removal in progress — Esc cancels";
                    return;
                }

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