Skip to content

fix: bound agent metadata previews and preserve removal outcomes - #15

Merged
harder merged 1 commit into
mainfrom
fix/search-metadata-hardening
Aug 28, 2026
Merged

harder merged 1 commit into
mainfrom
fix/search-metadata-hardening

Conversation

@harder

@harder harder commented Aug 28, 2026

Copy link
Copy Markdown
Owner

Summary

This is the next batch from the concurrency, resource, cancellation, and cross-platform hardening audit. It completes the remaining Discover agent-metadata scheduling work, addresses the final post-merge Copilot finding from PR #14, and closes the audit's weak LRU-test gap.

Why

Agent-filtered Discover searches may inspect up to 200 search results with gh skill preview. The prior implementation ran those previews sequentially with no per-item deadline. One hung process could consume the entire two-minute search lifetime, while large searches caused excessive latency and process churn.

The final review on PR #14 also identified a compact-removal race: the worker task could be complete while its IApplication.Invoke completion was still queued. During that interval, Esc/n could overwrite a successful removal with Cancelled, or a could launch the advanced wizard against an already-deleted skill.

Changes

Bounded and cancellable metadata loading

  • Adds SearchAgentMetadataLoader as the single owner of Discover agent-metadata preview scheduling.
  • Limits each request to four preview workers and enforces the same four-slot ceiling across overlapping or superseded searches.
  • Applies a 15-second linked deadline to every metadata preview under the existing two-minute whole-search deadline.
  • Propagates request/workspace cancellation through active previews and slot waits.
  • Keeps timeout, subprocess failure, and unexpected preview failure retryable instead of caching them as a false empty-agent result.
  • Rechecks the metadata cache after waiting for a slot so overlapping searches can reuse work completed while queued.
  • Captures the hidden-directory option at search submission, avoiding a worker-thread read from a live Terminal.Gui control.
  • Preserves the existing thread-safe, 512-entry metadata LRU.

Compact-removal completion ownership

  • Treats every non-null compact-removal task as owned until its queued UI completion callback runs.
  • Esc cancels only a worker that is still running.
  • Esc/n and a remain blocked while a completed worker is waiting for UI commit, preserving the real Removed/Failed outcome and preventing a duplicate wizard flow.
  • Shows a short finishing removal… state during that completion window.

Contract and regression coverage

  • Exercises the full 200-result search maximum while holding all four preview slots and proving a fifth process cannot start.
  • Proves the global limit also holds across overlapping searches.
  • Covers per-item timeout isolation, transient-failure retry, and parent cancellation reaching every active preview.
  • Strengthens the cache eviction test so it proves LRU rather than FIFO behavior.
  • Adds coverage that updating an existing cache value refreshes recency.
  • Adds a compact-modal regression for the worker-complete/UI-callback-pending state.
  • Updates the durable agent guidance and the full audit report with the new ownership and scheduling rules.

User-facing release notes

  • Agent-filtered Discover searches now use a small bounded preview pool, preventing large result sets from spawning excessive gh processes.
  • A single stalled metadata preview times out after 15 seconds without blocking healthy results indefinitely.
  • Temporary preview failures are retried by later searches instead of being remembered as permanent non-matches.
  • Compact removal no longer reports a completed deletion as canceled or opens the advanced wizard during a queued completion callback.

Validation

  • dotnet build --no-restore — passed; the only warning was the known sandbox denial writing NuGet vulnerability-cache data.
  • dotnet test --no-build — 650 passed, 0 failed, 0 skipped, including the ANSI-driver integration suite.
  • dotnet format SkillView.sln --no-restore --verify-no-changes — passed; workspace loading emitted its existing non-failing warning.
  • macOS ARM64 Native AOT publish — passed for both the standalone app and the gh extension.
  • AOT --version smoke — passed for skillview and gh-skillview, both reporting Terminal.Gui 2.4.17.0.
  • git diff --check — passed.

Review focus

  • Cancellation precedence between the parent search token and each per-preview timeout.
  • The shared semaphore and per-request worker bound under overlapping searches.
  • The choice to cache only successful preview metadata (plus definitive missing-repository cases).
  • Compact-modal keyboard behavior while worker completion is queued to the UI thread.

Follow-up audit items intentionally remain separate: install-modal task ownership, root CLI cancellation and post-kill waiting, cross-platform path identity, Esc focus behavior, and the lower-risk stress/streaming remainder.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The bounded scheduling and modal ownership changes are consistent, cancellation-safe, and adequately covered by regression tests.

Pull request overview

Hardens Discover metadata loading and compact removal lifecycle handling.

Changes:

  • Bounds metadata previews globally and adds per-item timeouts.
  • Preserves removal outcomes while UI completion is queued.
  • Adds concurrency, cancellation, timeout, LRU, and ownership tests.
File summaries
File Description
src/SkillView.Core/Ui/SearchAgentMetadataLoader.cs Implements bounded, timed metadata loading.
src/SkillView.Core/Ui/SkillViewApp.cs Integrates the loader and captures hidden-dir state.
src/SkillView.Core/Ui/RemoveConfirmModal.cs Blocks shortcuts until UI completion commits.
tests/SkillView.Tests/Ui/SearchAgentMetadataLoaderTests.cs Covers bounds, overlap, timeout, retry, and cancellation.
tests/SkillView.Tests/Ui/SearchAgentMetadataCacheTests.cs Strengthens LRU behavior coverage.
tests/SkillView.Tests/Ui/RemoveConfirmModalTests.cs Covers operation ownership states.
AGENTS.md Records durable scheduling and modal rules.
agent_docs/ui-lifecycle-and-resource-bounds.md Documents lifecycle and resource bounds.
docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md Updates audit remediation status.
Review details
  • Files reviewed: 9/9 changed files
  • Comments generated: 0
  • Review effort level: Balanced

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

@harder
harder merged commit 94a4a95 into main Aug 28, 2026
9 checks passed
@harder
harder deleted the fix/search-metadata-hardening branch August 28, 2026 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants