Prune removed skills from server feeds (#1403)#1408
Merged
Aaronontheweb merged 3 commits intoJun 15, 2026
Conversation
ServerFeedSkillSyncService only iterated forward through the server's RFC index, so skills deleted or renamed on a private skill server were never cleaned up: the on-disk directory persisted, the sync-state entry remained stale, and a rename left both old and new versions registered. Add a reverse pass after the forward sync loop. SkillSyncHelpers.PruneRemovedSkills removes sync-state entries and on-disk skill directories absent from the server index, skipping dot-prefixed bookkeeping (.staging, .sync-state.json) and tolerating per-directory delete failures. The registry self-heals via the existing RescanAndUpdateIndex disk rebuild. Pruning runs per-feed and only after a confirmed, non-empty index fetch, so a transient outage or empty response never wipes locally synced skills. No skill-server changes are needed: GetRfcIndexAsync returns a complete snapshot, so absence from the index reliably means removed.
Apply the surviving findings from the high-effort review of the prune pass: - Guard against an empty index in PruneRemovedSkills itself, not just the caller, so the destructive op is safe by construction. - Only delete directories that own a SKILL.md (SkillScanner's own skill-dir rule), so a stray non-skill directory under the feed root is never recursively deleted. This also neutralizes the scan-vs-prune race: prune no longer deletes the dot/non-SKILL.md dirs that SkillScanner's unguarded Pass-2 enumeration would choke on. - Wrap the post-sync RescanAndUpdateIndex in the same defensive try/catch the per-feed loop uses, so a directory vanishing mid-scan cannot tear down the background service. Transient or partial-index conditions are intentionally left to self-heal: prune clears the sync-state entry, so the next full index re-downloads the skill. That makes a deletion grace period / circuit breaker unnecessary, and the delete-then-forget ordering is dropped since the registry is rebuilt from disk and the forward loop never reads a removed skill's stale state. Tests: non-skill directories survive prune, stale state is still cleared, and an empty index is a no-op.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1403.
Problem
ServerFeedSkillSyncServiceonly iterated forward through a server feed's RFC index — it downloaded/updated skills that exist, but never pruned skills that were deleted or renamed on the server. The stale skill directory persisted on disk, theSkillSyncStateentry stayed, a rename left both old and new versions registered, andskill_loadkept working on deleted skills.Fix
Add a reverse pass after the forward sync loop (
SkillSyncHelpers.PruneRemovedSkills) that removes sync-state entries and on-disk skill directories no longer present in the index. The registry self-heals via the existing disk-rebuild (RescanAndUpdateIndex).Pruning runs per-feed and only after a confirmed, non-empty index fetch, so a transient outage or empty response never wipes locally synced skills.
Safety hardening (from an adversarial self-review)
SKILL.mdare deleted (the same ruleSkillScanneruses), so a stray non-skill directory under the feed root is never recursively deleted. This also neutralizes a scan-vs-prune race: prune no longer deletes the kind of directorySkillScanner's unguarded enumeration would choke on.RescanAndUpdateIndexis wrapped in the same defensive try/catch the per-feed loop uses, so a directory vanishing mid-scan can't tear down the background service.Deliberate non-goals
skill-serverchanges.GetRfcIndexAsyncreturns a complete snapshot (no pagination), so "absent from the index" reliably means removed.Netclaw.SkillClientstays a pure HTTP client.Tests
SkillSyncHelpersTestscovers: stale dirs + state pruned while present skills and.staging/.sync-state.jsonbookkeeping are preserved; non-skill directories survive prune (state still cleared); and an empty index is a no-op.Validation
dotnet test— 31/31 skill tests pass (incl. 4 new prune tests)dotnet slopwatch analyze— 0 issuesAdd-FileHeaders.ps1 -Verify— all headers present