fix(cli): preserve existing skill on reinstall rename failure

packages/cli/src/acp-integration/skill-management.test.ts

@@ -15,11 +15,31 @@ import * as path from 'node:path';
 import { afterEach, describe, expect, it, vi } from 'vitest';
 
 const downloadSkillMock = vi.hoisted(() => vi.fn());
+type RenameFn = typeof import('node:fs/promises').rename;
+const renameOverride = vi.hoisted(() => ({
+  fn: null as RenameFn | null,
+  realRename: null as RenameFn | null,
+}));
 
 vi.mock('./skill-source-download.js', () => ({
   downloadSkill: downloadSkillMock,
 }));
 
+vi.mock('node:fs/promises', async () => {
+  const actual =
+    await vi.importActual<typeof import('node:fs/promises')>(
+      'node:fs/promises',
+    );
+  renameOverride.realRename = actual.rename;
+  return {
+    ...actual,
+    rename: ((...args: Parameters<RenameFn>) => {
+      if (renameOverride.fn) return renameOverride.fn(...args);
+      return actual.rename(...args);
+    }) as RenameFn,
+  };
+});
+
 import {
   deleteManagedSkill,
   installManagedSkill,
@@ -166,6 +186,37 @@ describe('managed Skill mutations', () => {
     }
   });
 
+  it('can disable and delete a legacy artifact-shaped global Skill', async () => {
+    const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-skill-'));
+    vi.spyOn(Storage, 'getGlobalQwenDir').mockReturnValue(tempHome);
+    const slug = 'foo.backup-1-2';
+    const { skillDir, skillFile } = await writeSkill(tempHome, 'skills', slug);
+    const manager = managerFor(slug);
+    const config = configWith(manager);
+
+    try {
+      await expect(
+        setManagedSkillEnabled(config, {
+          skill: { slug, enabled: false },
+        }),
+      ).resolves.toMatchObject({
+        slug,
+        enabled: false,
+        installedPath: skillFile,
+      });
+      await expect(fs.readFile(skillFile, 'utf8')).resolves.toContain(
+        'disable-model-invocation: true',
+      );
+
+      await expect(
+        deleteManagedSkill(config, { skill: { slug } }),
+      ).resolves.toEqual({ slug, deleted: true });
+      await expect(fs.stat(skillDir)).rejects.toThrow();
+    } finally {
+      await fs.rm(tempHome, { recursive: true, force: true });
+    }
+  });
+
   it('preserves comments and nested hooks when toggling frontmatter', async () => {
     const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-skill-'));
     vi.spyOn(Storage, 'getGlobalQwenDir').mockReturnValue(tempHome);
@@ -308,6 +359,126 @@ describe('managed Skill mutations', () => {
     }
   });
 
+  it('cleans up the backup directory on a successful reinstall', async () => {
+    const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-skill-'));
+    vi.spyOn(Storage, 'getGlobalQwenDir').mockReturnValue(tempHome);
+    await writeSkill(tempHome, 'skills', 'pptx');
+    const manager = managerFor('pptx');
+    downloadSkillMock.mockResolvedValue({
+      skillContent:
+        '---\nname: pptx\ndescription: Create slide decks\n---\nNew body\n',
+      files: [
+        {
+          relativePath: 'SKILL.md',
+          content: Buffer.from('---\nname: pptx\n---\nNew body\n'),
+        },
+      ],
+    });
+
+    try {
+      await installManagedSkill(configWith(manager), {
+        skill: {
+          id: 'pptx-id',
+          slug: 'pptx',
+          name: 'PPTX',
+          sourceUrl:
+            'https://github.com/anthropics/skills/blob/main/skills/pptx/SKILL.md',
+        },
+      });
+
+      const skillsDir = path.join(tempHome, 'skills');
+      const entries = await fs.readdir(skillsDir);
+      // Only the real skill dir should remain; no leftover .backup-* siblings.
+      expect(entries).toEqual(['pptx']);
+      const installedPath = path.join(skillsDir, 'pptx', 'SKILL.md');
+      await expect(fs.readFile(installedPath, 'utf8')).resolves.toContain(
+        'name: pptx',
+      );
+      // Verify the reinstall actually replaced the skill content, not just
+      // that the pre-existing SKILL.md is still present.
+      await expect(fs.readFile(installedPath, 'utf8')).resolves.toContain(
+        'New body',
+      );
+    } finally {
+      await fs.rm(tempHome, { recursive: true, force: true });
+    }
+  });
+
+  it('restores the original skill when the swap rename fails', async () => {
+    const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-skill-'));
+    vi.spyOn(Storage, 'getGlobalQwenDir').mockReturnValue(tempHome);
+    const originalContent =
+      '---\nname: pptx\ndescription: Original skill\n---\nOriginal body\n';
+    const { skillFile } = await writeSkill(tempHome, 'skills', 'pptx');
+    // Overwrite with known content so we can assert restoration exactly.
+    await fs.writeFile(skillFile, originalContent, 'utf8');
+
+    const manager = managerFor('pptx');
+    downloadSkillMock.mockResolvedValue({
+      skillContent:
+        '---\nname: pptx\ndescription: New version\n---\nNew body\n',
+      files: [
+        {
+          relativePath: 'SKILL.md',
+          content: Buffer.from('---\nname: pptx\n---\nNew body\n'),
+        },
+      ],
+    });
+
+    // Make the staging → final rename fail with EPERM, but let the
+    // original → backup rename succeed so the rollback path is exercised.
+    const realRename = renameOverride.realRename!;
+    let renameCalled = false;
+    renameOverride.fn = (async (
+      oldPath: Parameters<RenameFn>[0],
+      newPath: Parameters<RenameFn>[1],
+    ) => {
+      const dest = String(newPath);
+      const skillsRoot = path.join(tempHome, 'skills');
+      // The swap rename: staging dir → final skill dir.
+      if (
+        dest === path.join(skillsRoot, 'pptx') &&
+        String(oldPath).includes('.installing-')
+      ) {
+        renameCalled = true;
+        const err = new Error('EPERM') as NodeJS.ErrnoException;
+        err.code = 'EPERM';
+        throw err;
+      }
+      return realRename(oldPath, newPath);
+    }) as RenameFn;
+
+    try {
+      await expect(
+        installManagedSkill(configWith(manager), {
+          skill: {
+            id: 'pptx-id',
+            slug: 'pptx',
+            name: 'PPTX',
+            sourceUrl:
+              'https://github.com/anthropics/skills/blob/main/skills/pptx/SKILL.md',
+          },
+        }),
+      ).rejects.toThrow('EPERM');
+
+      // Sanity: the failing rename was actually hit.
+      expect(renameCalled).toBe(true);
+
+      // Original content must be intact after the rollback.
+      await expect(fs.readFile(skillFile, 'utf8')).resolves.toBe(
+        originalContent,
+      );
+
+      // No leftover .backup-* or .installing-* siblings.
+      const skillsDir = path.join(tempHome, 'skills');
+      const entries = await fs.readdir(skillsDir);
+      expect(entries).toEqual(['pptx']);
+    } finally {
+      renameOverride.fn = null;
+      await fs.rm(tempHome, { recursive: true, force: true });
+    }
+  });
+
   it('rejects traversal slugs before downloading or touching disk', async () => {
     const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-skill-'));
     vi.spyOn(Storage, 'getGlobalQwenDir').mockReturnValue(tempHome);
@@ -341,4 +512,54 @@ describe('managed Skill mutations', () => {
       await fs.rm(tempHome, { recursive: true, force: true });
     }
   });
+
+  it('rejects artifact-shaped slugs reserved by the reinstall swap', async () => {
+    const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-skill-'));
+    vi.spyOn(Storage, 'getGlobalQwenDir').mockReturnValue(tempHome);
+    const config = configWith(managerFor('unused'));
+
+    try {
+      // Names shaped exactly like the swap artifacts
+      // (`<slug>.backup-<pid>-<timestamp>` / `.installing-...`) are
+      // skipped by the skill loaders, so installing them must fail loudly
+      // instead of reporting success for a skill that never loads.
+      for (const slug of ['foo.backup-1-2', 'foo.installing-12345-67890']) {
+        await expect(
+          installManagedSkill(config, {
+            skill: {
+              slug,
+              sourceUrl:
+                'https://github.com/anthropics/skills/blob/main/SKILL.md',
+            },
+          }),
+        ).rejects.toThrow('Invalid skill.slug');
+      }
+      expect(downloadSkillMock).not.toHaveBeenCalled();
+      await expect(fs.readdir(path.join(tempHome, 'skills'))).rejects.toThrow();
+
+      for (const slug of ['foo.backup-1-2-extra', 'foo-backup-1-2']) {
+        downloadSkillMock.mockResolvedValueOnce({
+          skillContent: `---\nname: ${slug}\n---\nBody\n`,
+          files: [
+            {
+              relativePath: 'SKILL.md',
+              content: Buffer.from(`---\nname: ${slug}\n---\nBody\n`),
+            },
+          ],
+        });
+
+        await expect(
+          installManagedSkill(configWith(managerFor(slug)), {
+            skill: {
+              slug,
+              sourceUrl:
+                'https://github.com/anthropics/skills/blob/main/SKILL.md',
+            },
+          }),
+        ).resolves.toMatchObject({ slug, installed: true });
+      }
+    } finally {
+      await fs.rm(tempHome, { recursive: true, force: true });
+    }
+  });
 });

packages/cli/src/acp-integration/skill-management.ts

@@ -88,13 +88,23 @@ function validateSkillSlug(slug: string): void {
   }
 }
 
+function rejectInstallArtifactSlug(slug: string): void {
+  if (/\.(backup|installing)-\d+-\d+$/.test(slug)) {
+    throw RequestError.invalidParams(
+      undefined,
+      'Invalid skill.slug: name ends with a reserved install-artifact suffix',
+    );
+  }
+}
+
 function readSkillInstallRequest(
   params: Record<string, unknown>,
 ): QwenSkillInstallRequest {
   const skillParams = toRecord(params['skill']);
   const input = Object.keys(skillParams).length > 0 ? skillParams : params;
   const slug = readRequiredString(input['slug'], 'skill.slug');
   validateSkillSlug(slug);
+  rejectInstallArtifactSlug(slug);
 
   const scope = readOptionalString(input['scope'], 'skill.scope') ?? 'global';
   if (scope !== 'global') {
@@ -294,11 +304,11 @@ async function installSkillFromUrl(
   }
 
   // Install atomically: stage all files in a sibling temp directory, then
-  // swap it in with a single rename. A mid-write failure (disk full,
-  // permission error) therefore leaves the previously installed skill
-  // intact instead of deleting it up front and ending up with a partial
-  // install. Removing the old dir before writing also dropped orphaned
-  // files from older versions; the rename preserves that property.
+  // swap it in with a rollback-capable rename sequence. A mid-write failure
+  // (disk full, permission error) therefore leaves the previously installed
+  // skill intact instead of deleting it up front and ending up with a partial
+  // install. The rename-based swap also drops orphaned files from older
+  // versions, preserving the behavior of the previous rm-based approach.
   const stagingDir = `${skillDir}.installing-${process.pid}-${Date.now()}`;
   try {
     await fs.rm(stagingDir, { recursive: true, force: true });
@@ -307,11 +317,29 @@ async function installSkillFromUrl(
       await fs.mkdir(path.dirname(targetPath), { recursive: true });
       await fs.writeFile(targetPath, file.content);
     }
-    // stagingDir is a sibling of skillDir (same filesystem), so the rename
-    // is atomic; the only gap is between the rm and rename, during which
-    // the fully-staged copy still exists for recovery.
-    await fs.rm(skillDir, { recursive: true, force: true });
-    await fs.rename(stagingDir, skillDir);
+    // Rollback-capable swap: backup the old directory, rename staging in,
+    // then remove the backup. On rename failure, restore the backup.
+    const backupDir = `${skillDir}.backup-${process.pid}-${Date.now()}`;
+    let backedUp = false;
+    try {
+      try {
+        await fs.rename(skillDir, backupDir);
+        backedUp = true;
+      } catch (err: unknown) {
+        if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err;
+        // Old directory doesn't exist, no backup needed.
+      }
+      await fs.rename(stagingDir, skillDir);
+    } catch (error) {
+      if (backedUp) {
+        await fs.rename(backupDir, skillDir).catch(() => {});
+      }
+      await fs.rm(stagingDir, { recursive: true, force: true }).catch(() => {});
+      throw error;
+    }
+    if (backedUp) {
+      await fs.rm(backupDir, { recursive: true, force: true }).catch(() => {});
+    }
   } catch (error) {
     await fs.rm(stagingDir, { recursive: true, force: true }).catch(() => {});
     throw error;

packages/cli/src/serve/workspace-skill-management.test.ts

@@ -329,6 +329,44 @@ describe('workspace Skill management', () => {
     ).rejects.toMatchObject({ code: 'invalid_skill_name' });
   });
 
+  it('rejects install-artifact-shaped Skill names before reading their source', async () => {
+    const workspace = await temporaryDirectory('qwen-skill-workspace-');
+    const source = path.join(workspace, 'missing-source');
+
+    // Names shaped exactly like reinstall artifacts are skipped by the
+    // skill loaders, so creating one must fail loudly instead of reporting
+    // success for a skill that would never load.
+    for (const name of ['foo.backup-1-2', 'foo.installing-12345-67890']) {
+      await expect(
+        installWorkspaceSkill(workspace, {
+          name,
+          scope: 'workspace',
+          source: { type: 'folder', path: source },
+        }),
+      ).rejects.toMatchObject({
+        code: 'invalid_skill_name',
+        message: expect.stringContaining('reserved install-artifact suffix'),
+      });
+    }
+  });
+
+  it('accepts non-artifact Skill names near the reserved install suffix', async () => {
+    const workspace = await temporaryDirectory('qwen-skill-workspace-');
+    const source = await temporaryDirectory('qwen-skill-source-');
+
+    for (const name of ['foo.backup-1-2-extra', 'foo-backup-1-2']) {
+      await fs.writeFile(path.join(source, 'SKILL.md'), skillMarkdown(name));
+
+      await expect(
+        installWorkspaceSkill(workspace, {
+          name,
+          scope: 'workspace',
+          source: { type: 'folder', path: source },
+        }),
+      ).resolves.toMatchObject({ skillName: name });
+    }
+  });
+
   it('rejects relative folder paths before reading files', async () => {
     const workspace = await temporaryDirectory('qwen-skill-workspace-');
 
@@ -529,6 +567,24 @@ describe('workspace Skill management', () => {
     ).resolves.toContain('name: stable-skill');
   });
 
+  it('deletes legacy install-artifact-shaped Skill directories', async () => {
+    const workspace = await temporaryDirectory('qwen-skill-workspace-');
+    const name = 'foo.backup-1-2';
+    const skillDir = path.join(workspace, '.qwen', 'skills', name);
+    const skillFile = path.join(skillDir, 'SKILL.md');
+    await fs.mkdir(skillDir, { recursive: true });
+    await fs.writeFile(skillFile, skillMarkdown(name));
+
+    await expect(
+      deleteWorkspaceSkill(workspace, 'workspace', name, skillFile),
+    ).resolves.toEqual({
+      skillName: name,
+      scope: 'workspace',
+      deleted: true,
+    });
+    await expect(fs.stat(skillDir)).rejects.toThrow();
+  });
+
   it('keeps a committed replacement when backup cleanup fails', async () => {
     const workspace = await temporaryDirectory('qwen-skill-workspace-');
     const source = await temporaryDirectory('qwen-skill-source-');