Skip to content

fix: scope stale cleanup to files this sync installed (data loss in ~/.claude/skills) - #902

Open
S-Keilani wants to merge 1 commit into
notque:mainfrom
S-Keilani:fix/scope-stale-cleanup-to-installed-files
Open

fix: scope stale cleanup to files this sync installed (data loss in ~/.claude/skills)#902
S-Keilani wants to merge 1 commit into
notque:mainfrom
S-Keilani:fix/scope-stale-cleanup-to-installed-files

Conversation

@S-Keilani

Copy link
Copy Markdown

Problem

The deferred stale cleanup in hooks/sync-to-user-claude.py walks the whole destination and unlinks every file absent from the current source set:

for item in dst.rglob("*"):
    if item.is_file():
        rel = item.relative_to(dst)
        if rel not in all_paths:
            ...
            item.unlink()

Destinations are shared. ~/.claude/skills also holds skills installed by plugins, marketplaces and other repos. Those are absent from this repo's source set, so the first sync deletes them.

The existing _resolves_inside guard doesn't help — foreign files don't resolve inside the repo, so they fall straight through to unlink().

Reproduction

repo/        skills/tk-skill/SKILL.md
~/.claude/   skills/cloudflare/SKILL.md            <- installed elsewhere
             skills/cloudflare/references/api.md

One sync, and both cloudflare files are gone.

On the machine where I hit this, a first sync would have removed 412 files across 11 skill directories: cloudflare (321 files), turnstile-spin, agents-sdk, use-railway, durable-objects, wrangler, workers-best-practices, cloudflare-email-service, sandbox-sdk, geo-audit, web-perf. All were live, in-use skills. I caught it before running the sync, not after.

Fix

Scope pruning by an ownership manifest at ~/.claude/.sync-manifest.json. A file is stale only if a previous run of this sync installed it and the source no longer provides it. Files this sync never installed are never touched.

With no manifest present, nothing is pruned and the manifest is seeded — so genuine stale cleanup (the phantom-hook protection this feature exists for) resumes from the second run. That one-run delay is the tradeoff for not being able to distinguish ours-from-theirs on a first pass. _resolves_inside is preserved.

Verification

Against the fixture above, on this branch:

foreign skill stale toolkit skill
unpatched, run 1 deleted
patched, run 1 (no manifest) preserved
patched, run 2 (skill renamed) preserved pruned

Run 2 logs the prune, so the feature still visibly works.

Notes

Found while auditing this toolkit on a Windows install. Two other issues I ran into turned out to be already fixed or obsolete upstream, so this is the only one I'm sending:

  • lib/stdin_timeout.py SIGALRM — you've since added a platform.system() == "Windows" branch. Worth noting that branch drops the timeout guard entirely on Windows; a daemon-thread reader would keep it on both platforms. Happy to send that separately if you want it.
  • install.sh hook-block shape — that script has been rewritten since; not applicable.

The deferred stale cleanup walks the whole destination and unlinks every file
absent from the current source set. Destinations are shared: ~/.claude/skills
also holds skills installed by plugins, marketplaces and other repos. Those
are absent from this repo's source set, so the first sync deletes them.

Reproduction (before this change):

    repo/           skills/tk-skill/SKILL.md
    ~/.claude/      skills/cloudflare/SKILL.md          <- installed elsewhere
                    skills/cloudflare/references/api.md

  After one sync both cloudflare files are gone.

On the machine where this was found it removed 412 files across 11 skill
directories — cloudflare (321), turnstile-spin, agents-sdk, use-railway,
durable-objects, wrangler, workers-best-practices, cloudflare-email-service,
sandbox-sdk, geo-audit, web-perf. All were live, in-use skills. The existing
_resolves_inside guard does not help: foreign files do not resolve inside the
repo, so they fall straight through to unlink().

Scope pruning by an ownership manifest at ~/.claude/.sync-manifest.json. A
file is stale only if a previous run of this sync installed it and the source
no longer provides it. Files this sync never installed are never touched.
With no manifest present nothing is pruned and the manifest is seeded, so
genuine stale cleanup resumes from the second run. The _resolves_inside guard
is preserved.

Verified against the fixture above: unpatched deletes both foreign files on
run 1; patched preserves them on runs 1 and 2 while still pruning a renamed
toolkit skill on run 2.
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.

1 participant