Skip to content

fix: cleanStaleSkills preserves git-tracked custom skills - #510

Open
PulkitCozar wants to merge 1 commit into
sahil87:mainfrom
PulkitCozar:fix/clean-stale-skills-preserve-git-tracked
Open

fix: cleanStaleSkills preserves git-tracked custom skills#510
PulkitCozar wants to merge 1 commit into
sahil87:mainfrom
PulkitCozar:fix/clean-stale-skills-preserve-git-tracked

Conversation

@PulkitCozar

Copy link
Copy Markdown

Why

fab sync's stale-skill cleanup (cleanStaleSkills in internal/skills.go) removes any entry under an agent's skill directory (.claude/skills/, .opencode/commands/, .agents/skills/, .gemini/skills/) whose name isn't in fab-kit's own canonical skill list. It doesn't distinguish "not fab-kit's" from "stale" — so a custom skill a project authors and commits to its own git history gets deleted on every sync, including the sync wt create runs automatically as its init script.

Reproduced end-to-end against a real consuming repo (a Terraform infra monorepo) that commits one custom Claude Code skill alongside fab-kit's deployed ones: every wt create silently deleted the custom skill's SKILL.md, and git status showed it as a working-tree deletion of a tracked file.

What

cleanStaleSkills now checks whether a candidate-for-removal entry is tracked by git in the consuming repo before deleting it, and preserves it when it is. Entries that are genuinely stale (not git-tracked — e.g. leftovers from a retired fab-kit skill) are still cleaned up exactly as before.

How

New isGitTracked(repoRoot, relPath string) bool helper shells out to git ls-files --error-unmatch (same exec.Command("git", ...) + fail-open pattern already used elsewhere in this package, e.g. warnIfFabVersionIgnored). Fails open (returns false) when git is unavailable or repoRoot isn't a git work tree, so the historical cleanup behavior for non-git contexts is unchanged — the loosening only ever applies to entries git can positively confirm as tracked.

Where

  • src/go/fab-kit/internal/skills.goisGitTracked + cleanStaleSkills (both directory- and flat-format branches)
  • src/go/fab-kit/internal/skills_test.go — two new tests mirroring the existing TestCleanStaleSkills_Directory/_Flat style, each asserting a git-tracked custom entry survives while an untracked stale entry is still removed

Verification

  • gofmt -l . — clean
  • go vet ./... — clean
  • go test ./... -count=1 -race — all packages pass (cmd/fab, cmd/fab-kit, internal)
  • Red→green: reverted the skills.go fix with the new tests in place — both new tests failed for the expected reason (custom skill deleted); restored the fix — both pass
  • End-to-end with the actual compiled binary (not just go test): built fab-kit from this branch, ran fab-kit sync twice against a temp repo shaped like the real reproduction case (git-tracked custom skill + one canonical stock skill) — first run preserved the custom skill and deployed the canonical one; second run (with an added untracked leftover skill) removed the untracked stale entry while still preserving the git-tracked custom skill

Follow-ups / out of scope

None — this is scoped to cleanStaleSkills's deletion check only.

@PulkitCozar
PulkitCozar marked this pull request as ready for review July 20, 2026 05:56
@sahil-noon

Copy link
Copy Markdown
Collaborator

Run this through the fab-kit pipeline
Also - in other repos we already do this via the untouched .claude/commands/ folder

@sahil-noon

Copy link
Copy Markdown
Collaborator

@PulkitCozar

Copy link
Copy Markdown
Author

Checked the shll example (.claude/commands/) — confirmed it's genuinely untouched by fab-kit's sync (none of the four agent configs in cleanStaleSkills reference it), so it's a real escape hatch for some cases. But it doesn't fully cover skills, for two reasons:

  1. Encapsulation.claude/commands/ is flat, one file per command. .claude/skills/<name>/ is a directory, so a skill can ship supporting resources alongside SKILL.md (references, scripts, templates) rather than being squeezed into a single file.
  2. Auto-invocation — Claude Code's model-driven skill discovery only scans .claude/skills/*/SKILL.md. Commands under .claude/commands/ are explicit-invocation only; there's no equivalent of auto-matching there. archive-dormant-repos happens to be explicit-only today (disable-model-invocation), but the underlying cleanStaleSkills bug isn't specific to that — any git-tracked skill relying on auto-invocation would still get deleted the same way.

So keeping this fix, rather than routing everyone to .claude/commands/, for the general case.

Also — re-running this through the fab-kit pipeline (/fab-new etc.) now.

@PulkitCozar
PulkitCozar force-pushed the fix/clean-stale-skills-preserve-git-tracked branch from a676b59 to 0e44ddd Compare July 20, 2026 11:51
@PulkitCozar

Copy link
Copy Markdown
Author

Ran this through fab-kit's own pipeline as requested — change 260720-4t2a-preserve-git-tracked-skills (fab/changes/260720-4t2a-preserve-git-tracked-skills/), intake → apply → review → hydrate → ship:

  • Review (fresh sub-agent pass, re-running gofmt/vet/go test ./... -race): passed, two non-blocking findings — the directory/flat format branches' git-tracked check was duplicated, and the "well under 50 lines" acceptance claim was inaccurate (function was 53 lines). Both fixed: extracted a shared isPreservedByGit helper, function is now 51 lines total across two cohesive format branches.
  • Hydrate: updated docs/memory/distribution/kit-architecture.md's sync-pipeline description to document the new preservation behavior.
  • Squashed to a single commit on this branch (force-pushed) per the pipeline's ship stage.

Same fix as before, now with the duplication cleaned up and docs updated.

fab sync's stale-skill cleanup treats any .claude/skills (or
.opencode/commands, .agents/skills, .gemini/skills) entry not in fab-kit's
own canonical skill list as deployment cruft and deletes it — including a
project-authored, git-tracked custom skill that was never fab-kit's to
manage. This is destructive on every `fab sync` (including the one `wt
create` runs automatically as its init script) whenever a repo commits its
own skill alongside the fab-kit-deployed ones.

cleanStaleSkills now checks whether the candidate entry is tracked by git
in the consuming repo (`git ls-files --error-unmatch`) before removing it,
via a shared isPreservedByGit helper used by both the directory- and
flat-format branches, and skips deletion when tracked. Fails open (git
absent/not a repo) to the prior behavior, so existing non-git cleanup
semantics are unchanged.

Run through fab-kit's own intake -> apply -> review -> hydrate -> ship
pipeline (change 260720-4t2a-preserve-git-tracked-skills): a fresh
sub-agent review pass flagged the two format branches' duplicated
tracking check, folded into isPreservedByGit; hydrate updated
docs/memory/distribution/kit-architecture.md to describe the new
preservation behavior in the sync pipeline writeup.
@PulkitCozar
PulkitCozar force-pushed the fix/clean-stale-skills-preserve-git-tracked branch from 0e44ddd to c5826a9 Compare July 20, 2026 11:52
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