Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 29 additions & 10 deletions src/main/core/skills/SkillsService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -287,15 +287,25 @@ export class SkillsService {
async uninstallSkill(skillId: string): Promise<void> {
const skillDir = path.join(SKILLS_ROOT, skillId);

// Remove agent symlinks first
// Remove agent symlinks first. Never delete real directories from agent config paths —
// those may be user-managed skills that Emdash only discovered.
await this.unsyncFromAgents(skillId);

// Remove skill directory
try {
await fs.promises.rm(skillDir, { recursive: true, force: true });
const stat = await fs.promises.lstat(skillDir);
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`);
}
Comment on lines +296 to +302
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 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.

} catch (error) {
log.error(`Failed to remove skill directory ${skillDir}:`, error);
throw error;
const code = (error as NodeJS.ErrnoException).code;
if (code !== 'ENOENT') {
log.error(`Failed to remove skill directory ${skillDir}:`, error);
throw error;
}
}

// Invalidate cache
Expand Down Expand Up @@ -376,19 +386,23 @@ export class SkillsService {
}

async unsyncFromAgents(skillId: string): Promise<void> {
for (const target of agentTargets) {
const syncPaths = [
...agentTargets.map((target) => target.getSkillDir(skillId)),
...skillScanPaths.map((scanPath) => path.join(scanPath, skillId)),
];

for (const targetDir of new Set(syncPaths)) {
try {
const targetDir = target.getSkillDir(skillId);
const stat = await fs.promises.lstat(targetDir);
if (stat.isSymbolicLink()) {
// Only remove symlinks that point into our central skills root
// Only remove symlinks that point into our central skills root.
const linkTarget = await fs.promises.readlink(targetDir);
const resolved = path.resolve(path.dirname(targetDir), linkTarget);
if (resolved.startsWith(SKILLS_ROOT)) {
if (this.isPathInsideSkillsRoot(resolved)) {
await fs.promises.unlink(targetDir);
}
}
// Never rm -rf real directories in agent config — they may be user-managed
// Never rm -rf real directories in agent config — they may be user-managed.
} catch {
// Doesn't exist or can't remove — skip
}
Expand Down Expand Up @@ -417,6 +431,11 @@ export class SkillsService {

// --- Private helpers ---

private isPathInsideSkillsRoot(candidatePath: string): boolean {
const relativePath = path.relative(SKILLS_ROOT, candidatePath);
return !relativePath.startsWith('..') && !path.isAbsolute(relativePath);
}

private loadBundledCatalog(): CatalogIndex {
return bundledCatalog as CatalogIndex;
}
Expand Down
4 changes: 2 additions & 2 deletions src/renderer/features/skills/components/SkillDetailModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ const SkillDetailModal: React.FC<SkillDetailModalProps> = ({
if (!skill) return;
setIsProcessing(true);
try {
await onUninstall(skill.id);
onClose();
const success = await onUninstall(skill.id);
if (success) onClose();
} finally {
setIsProcessing(false);
}
Expand Down
Loading