fix: allow uninstalling managed skills

[janburzinski]

summary

• fix uninstalling skills

[greptile-apps]

Greptile Summary

This PR fixes uninstalling catalog-managed skills by making uninstallSkill symlink-aware and extending unsyncFromAgents to cover skillScanPaths (not just agentTargets), so symlinks in additional agent scan directories are also cleaned up on removal. The modal now correctly stays open when uninstall fails, matching the existing install pattern.

• uninstallSkill now calls lstat to distinguish symlinks (unlink) from real directories (rm -rf) and silently ignores ENOENT, enabling clean removal for both managed and externally-discovered skills.
• unsyncFromAgents is extended to include skillScanPaths alongside agentTargets, with Set-based deduplication; symlinks are still only removed when they resolve into SKILLS_ROOT via the new isPathInsideSkillsRoot helper, which correctly replaces a fragile startsWith check.
• SkillDetailModal.handleUninstall now gates onClose() on a truthy return value from onUninstall, fixing premature modal dismissal on failure.

Confidence Score: 4/5

The core uninstall flow is safe: symlink-vs-directory detection is correct, the SKILLS_ROOT containment check is a strict improvement, and the modal correctly stays open on failure.

The logic changes are targeted and well-reasoned. Two small gaps exist — the isPathInsideSkillsRoot edge case and silent no-op for unexpected file types — but neither is likely to trigger in normal usage.

src/main/core/skills/SkillsService.ts — specifically the isPathInsideSkillsRoot helper and the uninstallSkill lstat branch.

Important Files Changed

Filename	Overview
src/main/core/skills/SkillsService.ts	uninstallSkill now differentiates symlinks from real dirs and handles ENOENT; unsyncFromAgents extended to cover skillScanPaths; new isPathInsideSkillsRoot replaces fragile startsWith check
src/renderer/features/skills/components/SkillDetailModal.tsx	handleUninstall now reads the boolean return value from onUninstall and only closes the modal on success, matching the existing handleInstall pattern

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
src/main/core/skills/SkillsService.ts:434-436
The `relativePath === ''` branch matches when `candidatePath` is exactly `SKILLS_ROOT` itself (not a child of it). A symlink pointing at `~/.agentskills` directly would satisfy this condition and be unlinked. In practice this shouldn't happen, but the condition can be dropped — a valid skill symlink will always resolve to a path *inside* SKILLS_ROOT (e.g. `~/.agentskills/my-skill`), never to the root directory itself.

```suggestion
    return !relativePath.startsWith('..') && !path.isAbsolute(relativePath);
```

### Issue 2 of 2
src/main/core/skills/SkillsService.ts:296-300
If `skillDir` exists but is neither a symlink nor a directory (e.g. a regular file left by a failed install), the `try` block exits silently without deleting it and without logging. The cache is still invalidated so the stale entry disappears from the UI on the next scan, but the file remains on disk indefinitely. A final `else` branch with a `log.warn` would make this visible.

```suggestion
      if (stat.isSymbolicLink()) {
        await fs.promises.unlink(skillDir);
      } else if (stat.isDirectory()) {
        await fs.promises.rm(skillDir, { recursive: true, force: true });
      } else {
        log.warn(`Unexpected entry type at ${skillDir} during uninstall — skipping`);
      }
```

_{Reviews (1): Last reviewed commit: "fix: allow uninstalling managed skills" | Re-trigger Greptile}

[greptile-apps]

The relativePath === '' branch matches when candidatePath is exactly SKILLS_ROOT itself (not a child of it). A symlink pointing at ~/.agentskills directly would satisfy this condition and be unlinked. In practice this shouldn't happen, but the condition can be dropped — a valid skill symlink will always resolve to a path inside SKILLS_ROOT (e.g. ~/.agentskills/my-skill), never to the root directory itself.

Suggested change

	return (
	relativePath === '' || (!relativePath.startsWith('..') && !path.isAbsolute(relativePath))
	);
	return !relativePath.startsWith('..') && !path.isAbsolute(relativePath);

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/main/core/skills/SkillsService.ts
Line: 434-436

Comment:
The `relativePath === ''` branch matches when `candidatePath` is exactly `SKILLS_ROOT` itself (not a child of it). A symlink pointing at `~/.agentskills` directly would satisfy this condition and be unlinked. In practice this shouldn't happen, but the condition can be dropped — a valid skill symlink will always resolve to a path *inside* SKILLS_ROOT (e.g. `~/.agentskills/my-skill`), never to the root directory itself.

```suggestion
    return !relativePath.startsWith('..') && !path.isAbsolute(relativePath);
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

If skillDir exists but is neither a symlink nor a directory (e.g. a regular file left by a failed install), the try block exits silently without deleting it and without logging. The cache is still invalidated so the stale entry disappears from the UI on the next scan, but the file remains on disk indefinitely. A final else branch with a log.warn would make this visible.

Suggested change

	if (stat.isSymbolicLink()) {
	await fs.promises.unlink(skillDir);
	} else if (stat.isDirectory()) {
	await fs.promises.rm(skillDir, { recursive: true, force: true });
	}
	if (stat.isSymbolicLink()) {
	await fs.promises.unlink(skillDir);
	} else if (stat.isDirectory()) {
	await fs.promises.rm(skillDir, { recursive: true, force: true });
	} else {
	log.warn(`Unexpected entry type at ${skillDir} during uninstall — skipping`);
	}

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/main/core/skills/SkillsService.ts
Line: 296-300

Comment:
If `skillDir` exists but is neither a symlink nor a directory (e.g. a regular file left by a failed install), the `try` block exits silently without deleting it and without logging. The cache is still invalidated so the stale entry disappears from the UI on the next scan, but the file remains on disk indefinitely. A final `else` branch with a `log.warn` would make this visible.

```suggestion
      if (stat.isSymbolicLink()) {
        await fs.promises.unlink(skillDir);
      } else if (stat.isDirectory()) {
        await fs.promises.rm(skillDir, { recursive: true, force: true });
      } else {
        log.warn(`Unexpected entry type at ${skillDir} during uninstall — skipping`);
      }
```

How can I resolve this? If you propose a fix, please make it concise.

[arnestrickmann]

thanks for the fix!