Harden removal, async lifecycles, cache, and logging

[harder]

Summary

This is the first implementation checkpoint from the repository-wide adversarial concurrency, cancellation, resource-usage, and cross-platform audit. It completes the first six recommended remediations and includes all five rounds of Copilot review fixes with deterministic regression coverage.

The changes focus on preventing destructive filesystem escapes, duplicate subprocess work, late UI mutation after teardown, stale workspace callbacks and previews, premature request-lease disposal, cross-operation spinner clearing, lost or duplicated log entries, malformed inventory caching, and unbounded log memory/disk growth.

What changed

1. Safe removal traversal

• Replaced recursive filesystem enumeration with an explicit, depth-bounded traversal using one lazy enumerator frame per active depth.
• Treats every child reparse point as a leaf, including Unix symbolic links, Windows junctions, broken links, cycles, and other link-like entries.
• Revalidates lexical containment and ancestor reparse-point state immediately before destructive operations.
• Added cancellation checks throughout single and batch removal.
• Checks cancellation between individual directory entries and retains O(depth) traversal state rather than materialized child arrays or pending sibling paths.

This closes the static packaged-link path where a nested link could allow removal to traverse outside the selected skill directory. It does not claim atomic protection against a hostile same-user process replacing a path component between validation and a path-based delete; supported .NET 10 APIs do not expose portable handle-relative/no-follow deletion. That native cross-platform design remains in the removal follow-up and is documented in the audit.

2. Thread-safe, single-flight gh skill list cache

• Protects cache entries, expiry, generation, and in-flight loads with one synchronization boundary.
• Concurrent misses for the same key now share one gh skill list process.
• Individual callers can cancel independently without canceling work still needed by another waiter.
• The final canceled waiter waits for underlying subprocess cleanup.
• Invalidation cancels in-flight loads, prevents stale completions from repopulating the cache, and does not release waiters until loader/process cleanup has returned.
• Failed process responses and malformed, truncated, or schema-incompatible successful output are not cached as empty inventory.
• The canonical or legacy skill-name identity must be a nonblank JSON string; numeric, null, Boolean, and canonical-invalid payloads remain retryable.
• A genuinely valid empty JSON array remains cacheable.

3. Owned background work and shutdown quiescence

• Added BackgroundTaskTracker to reserve and track application-owned background work before it starts.
• Routed app, Installed, and Updates fire-and-forget operations through the tracker.
• Shutdown now stops task admission, cancels app/workspace lifetimes, drains admitted tasks, and only then disposes Terminal.Gui and logging resources.
• Late UI dispatch is permanently ignored after the real application lifecycle ends.
• Background fault reporting is guarded so a reporting failure cannot create another unobserved task.
• Cancellation of a queued Installed width-stabilization callback during its own app/view teardown is treated as expected shutdown rather than logged as a false CRASH; unrelated cancellation and faults still surface.

4. Discover and Doctor workspace lifetimes

• Added explicit activation lifetimes and generation checks for Discover and Doctor.
• Leaving Discover cancels search and selected-preview work.
• Search and preview capture the Discover token before awaiting; navigation can dispose the source without late continuations dereferencing it or producing a false crash.
• Leaving Doctor cancels its environment probe and rejects queued stale callbacks.
• New searches supersede previous searches and have a whole-request two-minute deadline.
• Search cancels selected-preview work both when it starts and immediately before committing a new result set, covering previews launched from the old table while search was in flight.
• Search and preview await queued UI callbacks before disposing their latest-request leases, so valid delayed callbacks are not discarded merely because the worker method returned first.
• Awaitable dispatch is tied to application and workspace cancellation; callbacks still queued after ownership ends are rejected before touching UI.
• Shared busy state is tracked by operation ID. Canceling or completing a preview restores a still-running search rather than clearing its spinner or status text.
• Opening logs also cancels a preview that could otherwise close the log pane later.
• Startup, Doctor, and workflow consumers share a single in-flight environment probe.

5. Exact-once log replay and subscription

• Added sequence numbers and Logger.SubscribeWithReplay to establish an atomic retained-history/live-entry boundary.
• Concurrent log calls are delivered to each observer exactly once and in commit order, including entries emitted during replay.
• Out-of-order producers now wait for the missing sequence instead of accumulating entries in an unbounded gap dictionary.
• Observer callbacks cannot write to any logger. A scalar thread-local depth guard rejects those writes before ring mutation, preventing direct recursion and concurrent cross-logger lock cycles without per-entry collection allocation.
• The TUI log pane and FileLogSink now use the shared replay primitive instead of separate snapshot-then-subscribe protocols.
• File-sink attachment is explicitly one-shot.

This resolves both the visible-log replacement race highlighted in PR #11 and the analogous disk-sink gap discovered during reassessment.

6. Bounded logging memory and disk usage

• Caps individual retained log messages at 16 KiB and total retained message characters at 2 MiB, in addition to the existing entry-count limit.
• Caps categories and uses compact, single-line stderr excerpts in gh adapters.
• Bounds the TUI-visible log queue by both line count and total characters, even while the pane is hidden.
• Adds same-day size rotation with numbered log parts and safe restart behavior when the latest part is already full.
• Excludes the active file from trimming and opens files with Windows delete sharing.
• Preserves date retention and the total disk budget across rotated parts.

Cross-platform behavior

• Added coverage for Unix directory/file links, broken links, ancestor cycles, and post-validation link retargeting.
• Added a Windows-only junction test using mklink /J; it executes on the repository's Windows CI runner.
• File sharing and active-file retention are designed to behave consistently on Windows, macOS, and Linux.
• The audit records the remaining path-identity work for case-insensitive and per-directory case-sensitive filesystems; that work is intentionally not folded into this checkpoint.

Validation

• dotnet build --no-restore — passed with zero warnings and zero errors.
• dotnet test --no-build — 613 passed, 0 failed, including the ANSI-driver integration suite.
• git diff --check — passed.
• New tests include:
  • removal link escapes, cycles, retargeting, per-entry cancellation in a 2,000-file directory, and Windows junctions;
  • 100,000 mixed cache operations and simultaneous single-flight misses/expiry;
  • final-waiter cancellation cleanup and environment-probe sharing;
  • valid-empty versus malformed/schema-incompatible inventory caching, including non-string identity fields;
  • application shutdown waiting for pending cancellation cleanup;
  • Discover/Doctor workspace transitions, search-commit preview invalidation, awaitable queued dispatch, delayed-callback cancellation, and overlapping search/preview busy ownership;
  • exact-once replay/live log interleavings, sequence-gap backpressure, and synchronized two-producer cross-logger cycles;
  • stable workspace-token capture across source disposal;
  • expected cancellation of queued Installed layout work during shutdown;
  • message and visible-pane memory budgets;
  • same-day log rotation, oversized restart, and active-file retention.

The optional, non-configured AnalysisLevel=latest-all ruleset currently reports 190 warnings-as-errors across changed and unchanged repository code. It requires a separate baseline and triage before it can serve as a meaningful regression gate; the configured repository build remains clean.

Remaining audit work

This PR intentionally stops at a cohesive, independently reviewable checkpoint. The next remediation items remain documented and will be handled separately:

1. Move inventory and removal I/O off the UI thread, carry cancellation through the complete filesystem pipeline, and evaluate native handle-relative deletion for hostile same-user mutation.
2. Add per-metadata-preview deadlines and tightly bounded scheduling.
3. Make modal operation lifetimes awaitable and disposal-safe.
4. Wire root CLI cancellation and bounded post-kill waiting.
5. Centralize cross-platform path identity semantics.
6. Correct Esc focus behavior and strengthen the LRU contract test.
7. Finish lower-risk hardening and larger stress coverage.

Documentation

• Adds docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md with the complete findings, evidence, all five Copilot-review reassessments, remediation status, remaining order, and Terminal.Gui upstream opportunities.
• Updates AGENTS.md and agent_docs/ui-lifecycle-and-resource-bounds.md with durable rules for task ownership, workspace generations, safe removal traversal, exact-once bounded log delivery, parse-aware caching, and bounded logging.

Release-note summary

• Prevents skill removal from traversing nested links already present inside the selected target.
• Eliminates duplicate concurrent gh skill list subprocesses and cache races.
• Keeps malformed successful inventory output retryable instead of caching it as empty.
• Ensures background TUI work is canceled and drained before shutdown.
• Prevents stale Discover, Doctor, and old-table preview callbacks from changing current UI.
• Keeps search and preview request leases alive through queued UI delivery and preserves the correct spinner owner when their work overlaps.
• Makes log history/live delivery exact-once without an unbounded sequence-gap buffer.
• Prevents same-thread and cross-thread logger callback cycles from deadlocking, avoids disposed-workspace-token crashes, and avoids false shutdown-crash reports from deferred Installed layout work.
• Bounds in-memory and on-disk logs and adds same-day size rotation.

[Copilot]

🟡 Changes recommended

Removal retains a path-based escape race, and cache invalidation can outlive shutdown cleanup.

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

Pull request overview

Hardens destructive removal, asynchronous TUI lifecycles, shared caches, and bounded logging.

Changes:

• Adds safer removal traversal and single-flight cache/probe operations.
• Tracks background tasks and introduces workspace-scoped cancellation.
• Adds exact-once log replay, memory limits, file rotation, and regression tests.

File summaries

File	Description
AGENTS.md	Documents new lifecycle, removal, and logging rules.
docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md	Records audit findings and remediation status.
src/SkillView.Core/Gh/GhSkillInstallService.cs	Bounds logged installation errors.
src/SkillView.Core/Gh/GhSkillListAdapter.cs	Uses single-flight cache loading.
src/SkillView.Core/Gh/GhSkillListCache.cs	Synchronizes caching, loading, and invalidation.
src/SkillView.Core/Gh/GhSkillSearchService.cs	Bounds logged search errors.
src/SkillView.Core/Gh/GhSkillUpdateService.cs	Bounds logged update errors.
src/SkillView.Core/Inventory/RemoveService.cs	Adds bounded, link-aware removal traversal.
src/SkillView.Core/Logging/FileLogSink.cs	Adds replay attachment and size rotation.
src/SkillView.Core/Logging/Logger.cs	Adds retention budgets and ordered replay.
src/SkillView.Core/Logging/LogPaths.cs	Supports numbered log parts.
src/SkillView.Core/Ui/BackgroundTaskTracker.cs	Tracks and drains owned background work.
src/SkillView.Core/Ui/SharedAsyncOperation.cs	Shares cancellable concurrent operations.
src/SkillView.Core/Ui/SkillViewApp.cs	Integrates task ownership and workspace lifetimes.
src/SkillView.Core/Ui/SkillViewWorkflowCoordinator.cs	Shares environment probes across workflows.
src/SkillView.Core/Ui/Tabs/InstalledTabView.cs	Routes deferred work through task tracking.
src/SkillView.Core/Ui/Tabs/UpdatesTabView.cs	Tracks update and loading operations.
tests/SkillView.Tests/Gh/GhSkillListCacheTests.cs	Covers cache concurrency and cancellation.
tests/SkillView.Tests/Inventory/RemoveServiceTests.cs	Covers links, junctions, cycles, and cancellation.
tests/SkillView.Tests/Logging/FileLogSinkTests.cs	Covers replay, rotation, and retention.
tests/SkillView.Tests/Logging/LoggerTests.cs	Covers bounded retention and ordered replay.
tests/SkillView.Tests/Ui/BackgroundTaskTrackerTests.cs	Covers task admission, draining, and faults.
tests/SkillView.Tests/Ui/InstalledTabViewTests.cs	Updates task-runner setup.
tests/SkillView.Tests/Ui/SharedAsyncOperationTests.cs	Covers shared execution and cleanup.
tests/SkillView.Tests/Ui/SkillViewAppTests.cs	Covers workspace and shutdown lifecycles.
tests/SkillView.Tests/Ui/UpdatesTabViewTests.cs	Updates tracked-task test setup.

Review details

• Files reviewed: 26/26 changed files
• Comments generated: 4
• 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

Stale previews can overwrite new results, log ordering can retain unbounded pending entries, and parsing failures are cached as empty inventory.

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

Review details

Suppressed comments (2)

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

src/SkillView.Core/Ui/SkillViewApp.cs:1162

• A preview can still become stale if it is started from the old table while this search is in flight: the search only cancels previews before awaiting, so that later preview remains current and can overwrite the pane after these new results are installed. Cancel the preview gate again when committing successful search results so any preview launched against the superseded result set cannot paint afterward.

                if (!request.IsCurrent
                    || !IsDiscoverWorkspaceActive(discoverGeneration)
                    || System.Threading.Interlocked.Read(ref _searchGeneration) != generation)

src/SkillView.Core/Gh/GhSkillListAdapter.cs:93

• Malformed or unexpected successful output is still cached as a valid empty result because Parse reports failure by returning an empty array. A truncated JSON payload or schema mismatch therefore suppresses retries for the TTL and can hide gh inventory records. Preserve a parse-success signal and set ShouldCache: false when parsing fails; only a genuinely valid empty payload should be cached.

        var parsed = Parse(result.StdOut, _logger);
        return new GhSkillListCache.LoadResult(parsed);

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

[harder]

Addressed the two suppressed findings from the latest Copilot review in a982a50 as first-class review issues:

1. Stale old-table preview: successful search result commit now cancels the preview gate again before replacing the table. This catches a preview launched after search submission but against the superseded rows. SearchResultCommit_CancelsPreviewStartedAgainstSupersededTable covers the commit boundary deterministically.

2. Malformed inventory caching: gh skill list parsing now returns an explicit success signal. Malformed, blank, non-array, non-object-record, and missing-name payloads produce ShouldCache=false, while a valid empty JSON array remains cacheable. The adapter tests cover both retryable failure shapes and the genuine empty result.

The full suite passes: 602 tests, 0 failures; build passes with 0 warnings and 0 errors. The audit, AGENTS.md, lifecycle guidance, and PR release-note description have been updated.

[Copilot]

🟡 Changes recommended

Queued search and preview callbacks can outlive their request leases, causing valid UI updates to be discarded.

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

Review details

Suppressed comments (2)

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

src/SkillView.Core/Ui/SkillViewApp.cs:1994

• Canceling the preview with clearBusy: true can clear a still-running search's shared spinner. A search permits a preview from the old table while it is in flight, so opening logs in that state cancels the preview and leaves the ongoing search with no busy indication. Track busy ownership (or restore the search busy state) instead of unconditionally clearing the global indicator for the canceled preview.

        // Logs are an explicit user choice. A preview already in flight must
        // not be allowed to close the log pane when it completes.
        CancelCurrentPreview(clearBusy: true);

src/SkillView.Core/Ui/SkillViewApp.cs:1343

• This callback has the same lease-lifetime race: after ConfigureAwait(false), Invoke queues the action and returns, then PreviewSelectedAsync disposes request. A callback delayed until the next UI-loop tick observes request.IsCurrent == false and silently discards a valid preview. Keep the lease alive until an awaitable UI dispatch completes; update the timeout/error/finally dispatches consistently.

            Invoke(() =>
            {
                if (!request.IsCurrent || !IsDiscoverWorkspaceActive(discoverGeneration)) return;

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

[harder]

Also addressed the latest review summary’s suppressed busy-state finding in 78efa90. The status spinner is no longer a single ownerless boolean for overlapping search and preview work. Each operation registers a monotonic busy owner; ending or canceling one removes only that owner and restores the newest remaining operation. Opening logs while an old-table preview and search overlap therefore cancels the preview but retains the search spinner and exact search status text. Workspace deactivation retains an explicit clear-all operation. CancelingPreview_RestoresStillRunningSearchBusyState exercises the real log-pane path deterministically.

Validation: warning-free build; 605 tests passed; audit, AGENTS.md, lifecycle guidance, PR description, and release notes updated.

[Copilot]

🟡 Changes recommended

Cache schema validation, logger recursion handling, and shutdown cancellation reporting have unresolved correctness issues.

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

Review details

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

[Copilot]

🟡 Changes recommended

Disposed-token races and a cross-thread logger cycle can still cause teardown failures or deadlocks.

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

Review details

Suppressed comments (2)

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

src/SkillView.Core/Ui/SkillViewApp.cs:1196

• Workspace deactivation cancels and disposes _discoverLifetime, but this continuation dereferences discoverLifetime.Token after awaiting the search. If tab navigation wins the race after the subprocess returns, this throws ObjectDisposedException before the ownership check can reject the stale callback, and the tracked task is reported as a crash. Capture the CancellationToken value before the first await and use that value for every dispatch in this method.

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

            }, discoverLifetime.Token).ConfigureAwait(false);

src/SkillView.Core/Ui/SkillViewApp.cs:1372

• This has the same disposed-source race as search: leaving Discover disposes the captured CancellationTokenSource, so a preview that returns concurrently can throw while evaluating discoverLifetime.Token; the generic catch then dereferences it again and lets the exception escape to BackgroundTaskTracker. Capture the token struct before awaiting and use it for all preview dispatches.

            }, discoverLifetime.Token).ConfigureAwait(false);

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

[Copilot]

🔵 Needs a closer look

Aggregate log usage can exceed the configured disk budget until the next file rotation.

Review details

Suppressed comments (1)

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

src/SkillView.Core/Logging/FileLogSink.cs:125

• The total disk budget is only checked on the first append after opening or rotating a writer. After that _trimPending is false while the active part can grow by almost 5 MiB, so previously retained parts can remain even when active growth pushes aggregate usage above _totalSizeBudgetBytes. Re-run budget enforcement as the active file grows (ideally with incremental accounting or a threshold to avoid enumerating files for every line), and add a test where old parts initially fit but later appends force their removal.

                if (_trimPending)
                {
                    _trimPending = false;
                    TrimLocked();

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