fix: bound agent metadata previews and preserve removal outcomes

AGENTS.md

@@ -176,6 +176,15 @@ the terminal, with both a full-screen TUI and scriptable CLI commands.
   `CancellationTokenSource`, capture its `CancellationToken` before the first
   await and use only that value afterward. Never dereference the source from a
   continuation after navigation can cancel and dispose it.
+- Discover agent filtering previews at most four skills concurrently, applies
+  a 15-second deadline to each metadata preview, and retains the existing
+  two-minute whole-search deadline. Keep scheduling bounded across overlapping
+  searches through `SearchAgentMetadataLoader`; do not cache timeout, process,
+  or other transient preview failures as an empty agent list.
+- A modal operation remains owned until its queued UI completion callback has
+  run and explicitly releases or closes the modal. `Task.IsCompleted` only
+  describes the worker; it is not permission for keyboard shortcuts to cancel,
+  close, retry, or escalate against controls whose completion is still queued.
 - `SkillViewApp` now keeps the search shell and pane state, while
   `SkillViewWorkflowCoordinator` owns install/update/installed/remove/cleanup/
   doctor orchestration plus the shared inventory capture/rescan flow. Put new

agent_docs/ui-lifecycle-and-resource-bounds.md

@@ -52,6 +52,12 @@ logging, or subprocess adapters.
   capture limit only with evidence that a supported command needs more output.
 - `SearchAgentMetadataCache` is a thread-safe 512-entry LRU. Do not replace it
   with an unbounded dictionary.
+- Discover agent metadata loading has a global four-preview concurrency bound,
+  a 15-second deadline per preview, and the search request's two-minute outer
+  deadline. A timeout or failed preview stays retryable; only a successful
+  preview or a result with definitively absent repository metadata is cached.
+  Keep both the per-request worker bound and shared slot bound so superseded
+  searches cannot temporarily multiply process concurrency.
 - Local inventory capture schedules synchronous filesystem work on the thread
   pool and runs it concurrently with `gh skill list`. Resolver, scanner,
   package-lock reader, and cleanup classification check cancellation between
@@ -97,12 +103,18 @@ logging, or subprocess adapters.
 - Fire-and-forget UI work must consume `OperationCanceledException` only when
   its own app/view lifetime is canceled. Normal shutdown must not become a
   false `CRASH`, while unrelated cancellation and real faults remain visible.
+- For synchronous modals that own an async task, non-null task identity is the
+  ownership boundary. A completed worker may still have an `IApplication.Invoke`
+  completion queued; shortcuts must remain blocked until that callback clears
+  the task or closes the modal. Cancel only when the owned task is still
+  running, then drain it before controls are disposed.
 
 ## Focused verification
 
 Run `dotnet build`, then `dotnet test --no-build`. Resource-focused coverage
 includes large subprocess output, LRU eviction, subscription disposal,
 callback-safe cancellation gates, bounded inventory files, iteration failures,
 overlapping preview cancellation, superseded inventory scans, asynchronous
-removal cancellation/progress/error bounds, inline busy state, and layout
+removal cancellation/progress/error bounds, bounded and timed metadata
+previews, inline busy state, and layout
 guards at 80×24, 100×30, and 140×42.

docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md

@@ -8,12 +8,15 @@ merged by PR #11 as `40bbf0b`
 Hardening branch: `fix/adversarial-hardening`
 Second hardening branch: `fix/resource-lifecycle-hardening-2`
 Third hardening branch: `fix/removal-lifecycle-hardening`
-Status: Active remediation. PRs #12 and #13 are merged. Aggregate log retention,
+Fourth hardening branch: `fix/search-metadata-hardening`
+Status: Active remediation. PRs #12, #13, and #14 are merged. Aggregate log retention,
 callback-safe cancellation ownership, bounded/cancellable inventory I/O, and
-portable asynchronous removal are implemented. Native handle-relative deletion
-for hostile same-user mutation remains a separate security design; metadata
-scheduling, install-modal ownership, root CLI cancellation, path identity, Esc/
-LRU coverage, and the lower-risk remainder stay scheduled.
+portable asynchronous removal are implemented. The fourth batch adds bounded,
+timed metadata-preview scheduling and closes the compact-removal queued-
+completion race from PR #14's final review. Native handle-relative deletion for
+hostile same-user mutation remains a separate security design; install-modal
+ownership, root CLI cancellation, path identity, Esc coverage, and the lower-
+risk remainder stay scheduled.
 
 ## Executive summary
 
@@ -750,9 +753,10 @@ I/O, and a simulated iteration-time disconnect.
 
 Severity: **Medium**
 
-Implementation status: **Partially completed.** New searches now supersede the
-previous request and the whole search has a two-minute deadline. Per-metadata
-preview deadlines and bounded scheduling remain in remediation item 8.
+Implementation status: **Completed on `fix/search-metadata-hardening`.** New
+searches supersede the previous request, the whole search has a two-minute
+deadline, metadata previews have a 15-second per-item deadline, and no more
+than four previews run concurrently across overlapping searches.
 
 Locations:
 
@@ -787,6 +791,23 @@ rejected rather than canceling and superseding the stale request.
 - Preserve the bounded LRU metadata cache.
 - Add timeout, supersession, partial-failure, and 200-result tests.
 
+### Remediation
+
+`SearchAgentMetadataLoader` now runs at most four preview workers per request
+and enforces the same four-slot ceiling across overlapping/superseded searches.
+Each metadata subprocess receives a linked 15-second deadline under the outer
+two-minute search request. Parent cancellation is propagated; per-item timeout
+and transient preview failure affect only that result and remain retryable
+rather than being cached as a false empty-agent result. Missing repository
+identity is the only non-preview case cached as definitively empty.
+
+Deterministic coverage holds all four slots while submitting the full 200-result
+maximum, proves no fifth preview starts, then releases the batch and verifies
+all results. Separate tests cover per-item timeout with a healthy concurrent
+result, transient failure retry, and request cancellation reaching every active
+preview. The LRU contract test now touches an older entry before eviction and
+also verifies that updating an existing value refreshes recency.
+
 ## Finding 8: install modal async handlers can outlive disposed dialogs
 
 Severity: **Medium**
@@ -933,6 +954,21 @@ returned after its initial progress event. Refusals now publish a forced
 terminal completed snapshot with one processed target, zero deleted targets,
 and the refusal error count.
 
+PR #14's final post-merge review contained one suppressed finding from
+unchanged lines. It was independently reassessed and accepted:
+
+1. **Compact-modal shortcuts can race a queued completion callback — correct
+   and fixed on `fix/search-metadata-hardening`.** `Task.IsCompleted` can become
+   true on a worker thread before the callback queued through
+   `IApplication.Invoke` commits `Removed`/`Failed` and clears or closes the
+   modal. During that interval, Esc/`n` could overwrite a successful outcome
+   with `Cancelled`, while `a` could open the advanced wizard against an
+   already-deleted skill. Any non-null task is now treated as owned until its
+   UI completion runs. Esc cancels only a still-running worker; completed-but-
+   queued workers display a finishing status and all close/escalate shortcuts
+   remain blocked. A state-classification regression test protects the worker-
+   complete/UI-pending boundary.
+
 ### Impact
 
 - Large trees allocate proportional path lists before deletion begins.
@@ -1146,14 +1182,14 @@ but the UI message and expected two-step Esc then q behavior are wrong.
 - Keep the application-level Ctrl+Q test because focused editors are precisely
   where global quit routing tends to regress.
 
-## Test gap: the LRU test currently proves only FIFO eviction
+## Test gap: the LRU test previously proved only FIFO eviction
+
+Status: **Closed on `fix/search-metadata-hardening`.**
 
-`Store_EvictsLeastRecentlyUsedMetadataAtCapacity` inserts first, second, and
-third without touching an older entry before eviction. A FIFO cache would pass
-the same test. The implementation's `Has` method does call `Touch`, so this is
-not evidence of a production defect, but the contract is unprotected. Touch
-`first` before storing `third` and assert that `second` was evicted. Also test
-that updating an existing value refreshes recency.
+`Store_EvictsLeastRecentlyUsedMetadataAtCapacity` now touches `first` before
+storing `third` and asserts that `second` was evicted. A separate test verifies
+that updating an existing value refreshes recency and retains the replacement
+metadata.
 
 ## Lower-risk hardening opportunities
 
@@ -1214,14 +1250,17 @@ coordinated within their current scope:
    confirmed that supported .NET 10 APIs cannot make path validation and
    deletion atomic; an audited Unix/Windows native implementation remains a
    separate Finding 1 security follow-up rather than an implicit portable fix.
-9. [ ] Finish metadata-preview deadlines and bounded scheduling (search
-   supersession and a whole-request deadline are complete).
+9. [x] Finish metadata-preview deadlines and bounded scheduling. Search
+   supersession, the whole-request deadline, a per-preview deadline, and a
+   global four-process ceiling are complete.
 10. [~] Make modal operation lifetimes awaitable and disposal-safe, including
-    stable token capture before the first await. Removal modals are complete;
-    the three install flows remain.
+    stable token capture before the first await. Removal modals are complete,
+    including the compact worker-complete/UI-pending shortcut boundary; the
+    three install flows remain.
 11. [ ] Wire root CLI cancellation and bounded post-kill waiting.
 12. [ ] Centralize cross-platform path identity semantics.
-13. [ ] Correct Esc focus behavior and strengthen the LRU contract test.
+13. [~] Correct Esc focus behavior and strengthen the LRU contract test. The
+    LRU contract is complete; Esc focus behavior remains.
 14. [ ] Finish the lower-risk hardening and stress coverage.
 
 `FileLogSink` lock ordering, Doctor-to-tab cancellation, and Updates operation

src/SkillView.Core/Ui/RemoveConfirmModal.cs

@@ -28,6 +28,13 @@ internal enum Outcome
 
     internal sealed record Result(Outcome Outcome, RemoveService.RemoveReport? Report);
 
+    internal enum OperationOwnership
+    {
+        None,
+        Running,
+        AwaitingUiCompletion,
+    }
+
     private readonly IApplication _app;
     private readonly RemoveService _remove;
     private readonly Logger _logger;
@@ -61,6 +68,13 @@ internal static bool CanRunCompact(RemoveTargetEvaluation evaluation)
         return true;
     }
 
+    internal static OperationOwnership ClassifyOperation(Task? operation) => operation switch
+    {
+        null => OperationOwnership.None,
+        { IsCompleted: false } => OperationOwnership.Running,
+        _ => OperationOwnership.AwaitingUiCompletion,
+    };
+
     internal Result Show()
     {
         using var lifetime = new CancellationTokenSource();
@@ -287,10 +301,18 @@ async Task RunRemovalAsync(CancellationToken cancellationToken)
             else if (ch == 'n' || ch == 'N' || key.KeyCode == KeyCode.Esc)
             {
                 key.Handled = true;
-                if (activeOperation is { IsCompleted: false })
+                var operationOwnership = ClassifyOperation(activeOperation);
+                if (operationOwnership != OperationOwnership.None)
                 {
-                    lifetime.Cancel();
-                    status.Text = " canceling removal…";
+                    if (operationOwnership == OperationOwnership.Running)
+                    {
+                        lifetime.Cancel();
+                        status.Text = " canceling removal…";
+                    }
+                    else
+                    {
+                        status.Text = " finishing removal…";
+                    }
                     return;
                 }
                 lifetime.Cancel();
@@ -300,9 +322,11 @@ async Task RunRemovalAsync(CancellationToken cancellationToken)
             else if (ch == 'a' || ch == 'A')
             {
                 key.Handled = true;
-                if (activeOperation is { IsCompleted: false })
+                if (ClassifyOperation(activeOperation) != OperationOwnership.None)
                 {
-                    status.Text = " removal in progress — Esc cancels";
+                    status.Text = activeOperation!.IsCompleted
+                        ? " finishing removal…"
+                        : " removal in progress — Esc cancels";
                     return;
                 }
                 outcome = Outcome.EscalateToWizard;

src/SkillView.Core/Ui/SearchAgentMetadataLoader.cs

@@ -0,0 +1,141 @@
+using System.Collections.Immutable;
+using SkillView.Gh.Models;
+using SkillView.Logging;
+
+namespace SkillView.Ui;
+
+/// <summary>
+/// Loads the preview front matter needed by Discover's agent filter without
+/// allowing one search to fan out into an unbounded number of subprocesses.
+/// </summary>
+internal sealed class SearchAgentMetadataLoader
+{
+    internal const int DefaultMaxConcurrency = 4;
+    internal static readonly TimeSpan DefaultPreviewTimeout = TimeSpan.FromSeconds(15);
+
+    private readonly SearchAgentMetadataCache _cache;
+    private readonly Logger _logger;
+    private readonly int _maxConcurrency;
+    private readonly TimeSpan _previewTimeout;
+    private readonly SemaphoreSlim _previewSlots;
+
+    internal SearchAgentMetadataLoader(
+        SearchAgentMetadataCache cache,
+        Logger logger,
+        int maxConcurrency = DefaultMaxConcurrency,
+        TimeSpan? previewTimeout = null)
+    {
+        ArgumentNullException.ThrowIfNull(cache);
+        ArgumentNullException.ThrowIfNull(logger);
+        ArgumentOutOfRangeException.ThrowIfLessThan(maxConcurrency, 1);
+
+        var effectiveTimeout = previewTimeout ?? DefaultPreviewTimeout;
+        if (effectiveTimeout <= TimeSpan.Zero)
+        {
+            throw new ArgumentOutOfRangeException(nameof(previewTimeout));
+        }
+
+        _cache = cache;
+        _logger = logger;
+        _maxConcurrency = maxConcurrency;
+        _previewTimeout = effectiveTimeout;
+        _previewSlots = new SemaphoreSlim(maxConcurrency, maxConcurrency);
+    }
+
+    internal async Task<IReadOnlyList<SearchResultSkill>> FilterAsync(
+        IReadOnlyList<SearchResultSkill> results,
+        string? requestedAgent,
+        Func<SearchResultSkill, CancellationToken, Task<PreviewResult>> loadPreviewAsync,
+        CancellationToken cancellationToken)
+    {
+        ArgumentNullException.ThrowIfNull(results);
+        ArgumentNullException.ThrowIfNull(loadPreviewAsync);
+
+        var normalizedAgent = SearchAgentMetadataCache.NormalizeAgent(requestedAgent);
+        if (normalizedAgent is null)
+        {
+            return results;
+        }
+
+        await Parallel.ForEachAsync(
+            results,
+            new ParallelOptions
+            {
+                CancellationToken = cancellationToken,
+                MaxDegreeOfParallelism = _maxConcurrency,
+            },
+            async (result, token) =>
+            {
+                await EnsureLoadedAsync(result, loadPreviewAsync, token).ConfigureAwait(false);
+            }).ConfigureAwait(false);
+
+        cancellationToken.ThrowIfCancellationRequested();
+        return _cache.Filter(results, normalizedAgent);
+    }
+
+    private async Task EnsureLoadedAsync(
+        SearchResultSkill result,
+        Func<SearchResultSkill, CancellationToken, Task<PreviewResult>> loadPreviewAsync,
+        CancellationToken cancellationToken)
+    {
+        cancellationToken.ThrowIfCancellationRequested();
+        if (_cache.Has(result))
+        {
+            return;
+        }
+
+        if (string.IsNullOrWhiteSpace(result.Repo))
+        {
+            _cache.Store(result, ImmutableArray<string>.Empty);
+            return;
+        }
+
+        await _previewSlots.WaitAsync(cancellationToken).ConfigureAwait(false);
+        try
+        {
+            // Another overlapping search may have populated this result while
+            // this worker waited for a globally bounded preview slot.
+            if (_cache.Has(result))
+            {
+                return;
+            }
+
+            using var timeout = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
+            timeout.CancelAfter(_previewTimeout);
+            try
+            {
+                var preview = await loadPreviewAsync(result, timeout.Token).ConfigureAwait(false);
+                if (!preview.Succeeded)
+                {
+                    // A subprocess failure is transient. Leave it uncached so
+                    // a later search can retry instead of persisting a false
+                    // "agent not present" result for the cache lifetime.
+                    return;
+                }
+
+                cancellationToken.ThrowIfCancellationRequested();
+                var agents = SearchAgentMetadataCache.ExtractAgentsFromMarkdown(
+                    preview.MarkdownBody ?? preview.Body ?? string.Empty);
+                _cache.Store(result, agents);
+            }
+            catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
+            {
+                throw;
+            }
+            catch (OperationCanceledException) when (timeout.IsCancellationRequested)
+            {
+                _logger.Warn(
+                    "search.agent",
+                    $"{result.Repo}/{result.SkillName}: metadata preview timed out after {_previewTimeout.TotalSeconds:F0}s");
+            }
+            catch (Exception ex)
+            {
+                _logger.Warn("search.agent", $"{result.Repo}/{result.SkillName}: {ex.Message}");
+            }
+        }
+        finally
+        {
+            _previewSlots.Release();
+        }
+    }
+}