fix(skill-sync): address review findings

src/cli/install.test.ts

@@ -1,10 +1,143 @@
-import { afterEach, describe, expect, test } from 'bun:test';
+import { afterEach, beforeEach, describe, expect, mock, test } from 'bun:test';
 import { shouldInstallCompanion } from './install';
 import type { InstallConfig } from './types';
 
 const ORIGINAL_ENV = { ...process.env };
 const ORIGINAL_STDIN_IS_TTY = process.stdin.isTTY;
 
+const actualSkillSync = require('../hooks/auto-update-checker/skill-sync');
+const actualConfigManager = require('./config-manager');
+const actualBackgroundSubagents = require('./background-subagents');
+const actualPaths = require('./paths');
+
+const originalSyncBundledSkillsFromPackage =
+  actualSkillSync.syncBundledSkillsFromPackage;
+
+const originalIsOpenCodeInstalled = actualConfigManager.isOpenCodeInstalled;
+const originalGetOpenCodeVersion = actualConfigManager.getOpenCodeVersion;
+const originalGetOpenCodePath = actualConfigManager.getOpenCodePath;
+const originalAddPluginToOpenCodeConfig =
+  actualConfigManager.addPluginToOpenCodeConfig;
+const originalAddPluginToOpenCodeTuiConfig =
+  actualConfigManager.addPluginToOpenCodeTuiConfig;
+const originalWarmOpenCodePluginCache =
+  actualConfigManager.warmOpenCodePluginCache;
+const originalDisableDefaultAgents = actualConfigManager.disableDefaultAgents;
+const originalEnableLspByDefault = actualConfigManager.enableLspByDefault;
+const originalDetectCurrentConfig = actualConfigManager.detectCurrentConfig;
+const originalGenerateLiteConfig = actualConfigManager.generateLiteConfig;
+const originalWriteLiteConfig = actualConfigManager.writeLiteConfig;
+
+const originalIsBackgroundSubagentsEnabled =
+  actualBackgroundSubagents.isBackgroundSubagentsEnabled;
+const originalDetectBackgroundSubagentsTarget =
+  actualBackgroundSubagents.detectBackgroundSubagentsTarget;
+const originalExpandHomePath = actualBackgroundSubagents.expandHomePath;
+const originalGetBackgroundSubagentsBlock =
+  actualBackgroundSubagents.getBackgroundSubagentsBlock;
+const originalWriteBackgroundSubagentsBlock =
+  actualBackgroundSubagents.writeBackgroundSubagentsBlock;
+const originalManualBackgroundSubagentsInstructions =
+  actualBackgroundSubagents.manualBackgroundSubagentsInstructions;
+
+const originalGetExistingLiteConfigPath = actualPaths.getExistingLiteConfigPath;
+
+let importCounter = 0;
+let mockFailedResult: string[] = [];
+let enableInstallMocks = false;
+
+mock.module('../hooks/auto-update-checker/skill-sync', () => {
+  return {
+    ...actualSkillSync,
+    syncBundledSkillsFromPackage: (packageRoot: string, options?: any) =>
+      enableInstallMocks
+        ? {
+            installed: [],
+            skippedExisting: [],
+            failed: mockFailedResult,
+            updated: [],
+            staged: [],
+            adopted: [],
+            customized: [],
+          }
+        : originalSyncBundledSkillsFromPackage(packageRoot, options),
+  };
+});
+
+mock.module('./config-manager', () => {
+  return {
+    ...actualConfigManager,
+    isOpenCodeInstalled: async () =>
+      enableInstallMocks ? true : originalIsOpenCodeInstalled(),
+    getOpenCodeVersion: async () =>
+      enableInstallMocks ? '1.0.0' : originalGetOpenCodeVersion(),
+    getOpenCodePath: () =>
+      enableInstallMocks
+        ? '/usr/local/bin/opencode'
+        : originalGetOpenCodePath(),
+    addPluginToOpenCodeConfig: async () =>
+      enableInstallMocks
+        ? { success: true, configPath: '/path' }
+        : originalAddPluginToOpenCodeConfig(),
+    addPluginToOpenCodeTuiConfig: async () =>
+      enableInstallMocks
+        ? { success: true, configPath: '/path' }
+        : originalAddPluginToOpenCodeTuiConfig(),
+    warmOpenCodePluginCache: async () =>
+      enableInstallMocks
+        ? { success: true, configPath: '/path' }
+        : originalWarmOpenCodePluginCache(),
+    disableDefaultAgents: () =>
+      enableInstallMocks
+        ? { success: true, configPath: '/path' }
+        : originalDisableDefaultAgents(),
+    enableLspByDefault: () =>
+      enableInstallMocks
+        ? { success: true, configPath: '/path' }
+        : originalEnableLspByDefault(),
+    detectCurrentConfig: () =>
+      enableInstallMocks
+        ? { isInstalled: true }
+        : originalDetectCurrentConfig(),
+    generateLiteConfig: (cfg: any) =>
+      enableInstallMocks ? {} : originalGenerateLiteConfig(cfg),
+    writeLiteConfig: (cfg: any, path?: string) =>
+      enableInstallMocks
+        ? { success: true, configPath: '/path' }
+        : originalWriteLiteConfig(cfg, path),
+  };
+});
+
+mock.module('./background-subagents', () => {
+  return {
+    ...actualBackgroundSubagents,
+    isBackgroundSubagentsEnabled: (env?: string) =>
+      enableInstallMocks ? true : originalIsBackgroundSubagentsEnabled(env),
+    detectBackgroundSubagentsTarget: () =>
+      enableInstallMocks ? '/path' : originalDetectBackgroundSubagentsTarget(),
+    expandHomePath: (p: string) =>
+      enableInstallMocks ? p : originalExpandHomePath(p),
+    getBackgroundSubagentsBlock: (target: string) =>
+      enableInstallMocks ? '' : originalGetBackgroundSubagentsBlock(target),
+    writeBackgroundSubagentsBlock: (target: string) =>
+      enableInstallMocks ? {} : originalWriteBackgroundSubagentsBlock(target),
+    manualBackgroundSubagentsInstructions: (opts?: any) =>
+      enableInstallMocks
+        ? ''
+        : originalManualBackgroundSubagentsInstructions(opts),
+  };
+});
+
+mock.module('./paths', () => {
+  return {
+    ...actualPaths,
+    getExistingLiteConfigPath: () =>
+      enableInstallMocks
+        ? '/path/lite-config.json'
+        : originalGetExistingLiteConfigPath(),
+  };
+});
+
 function baseConfig(): InstallConfig {
   return {
     hasTmux: false,
@@ -49,3 +182,82 @@ describe('shouldInstallCompanion', () => {
     expect(config.companion).toBe('no');
   });
 });
+
+describe('install skill synchronization error mapping', () => {
+  let logSpy: ReturnType<typeof mock>;
+  let originalConsoleLog: typeof console.log;
+
+  beforeEach(() => {
+    enableInstallMocks = true;
+    mockFailedResult = [];
+    originalConsoleLog = console.log;
+    logSpy = mock(() => {});
+    console.log = logSpy;
+  });
+
+  afterEach(() => {
+    enableInstallMocks = false;
+    console.log = originalConsoleLog;
+  });
+
+  test('maps __lock__ to lock acquisition failure', async () => {
+    mockFailedResult = ['__lock__'];
+    const { install } = await import(`./install?test=${importCounter++}`);
+
+    await install({
+      skills: 'yes',
+      tui: false,
+      companion: 'no',
+    });
+
+    const calls = logSpy.mock.calls.map((call: any[]) => call[0] as string);
+    const hasLockErr = calls.some((msg: string) =>
+      msg?.includes('Lock acquisition failed'),
+    );
+    expect(hasLockErr).toBe(true);
+
+    const hasRawSentinel = calls.some((msg: string) =>
+      msg?.includes('__lock__'),
+    );
+    expect(hasRawSentinel).toBe(false);
+  });
+
+  test('maps __manifest__ to manifest write failure', async () => {
+    mockFailedResult = ['__manifest__'];
+    const { install } = await import(`./install?test=${importCounter++}`);
+
+    await install({
+      skills: 'yes',
+      tui: false,
+      companion: 'no',
+    });
+
+    const calls = logSpy.mock.calls.map((call: any[]) => call[0] as string);
+    const hasManifestErr = calls.some((msg: string) =>
+      msg?.includes('Manifest write failed'),
+    );
+    expect(hasManifestErr).toBe(true);
+
+    const hasRawSentinel = calls.some((msg: string) =>
+      msg?.includes('__manifest__'),
+    );
+    expect(hasRawSentinel).toBe(false);
+  });
+
+  test('keeps normal skill names prefix as Failed: <name>', async () => {
+    mockFailedResult = ['some-custom-skill'];
+    const { install } = await import(`./install?test=${importCounter++}`);
+
+    await install({
+      skills: 'yes',
+      tui: false,
+      companion: 'no',
+    });
+
+    const calls = logSpy.mock.calls.map((call: any[]) => call[0] as string);
+    const hasSkillErr = calls.some((msg: string) =>
+      msg?.includes('Failed: some-custom-skill'),
+    );
+    expect(hasSkillErr).toBe(true);
+  });
+});

src/cli/install.ts

@@ -435,7 +435,13 @@ async function runInstall(config: InstallConfig): Promise<number> {
         }
         if (result.failed.length > 0) {
           for (const skill of result.failed) {
-            printError(`Failed: ${skill}`);
+            if (skill === '__lock__') {
+              printError('Lock acquisition failed');
+            } else if (skill === '__manifest__') {
+              printError('Manifest write failed');
+            } else {
+              printError(`Failed: ${skill}`);
+            }
           }
         }
 

src/hooks/auto-update-checker/skill-sync.test.ts

@@ -1,8 +1,23 @@
-import { afterEach, beforeEach, describe, expect, test } from 'bun:test';
+import { afterEach, beforeEach, describe, expect, mock, test } from 'bun:test';
 import * as fs from 'node:fs';
 import * as os from 'node:os';
 import * as path from 'node:path';
 
+let shouldFailRename = false;
+
+mock.module('node:fs', () => {
+  const actualFs = require('node:fs');
+  return {
+    ...actualFs,
+    renameSync: (src: string, dest: string) => {
+      if (shouldFailRename && src.includes('recovery-failure-test-skill')) {
+        throw new Error('Mocked rename failure');
+      }
+      return actualFs.renameSync(src, dest);
+    },
+  };
+});
+
 let importCounter = 0;
 
 async function syncBundledSkillsFromPackage(packageRoot: string) {
@@ -31,6 +46,7 @@ describe('syncBundledSkillsFromPackage', () => {
   let origEnvConfigDir: string | undefined;
 
   beforeEach(() => {
+    shouldFailRename = false;
     origEnvConfigDir = process.env.OPENCODE_CONFIG_DIR;
     // Create a unique temporary directory for this test run
     const randomId = Math.random().toString(36).substring(2, 10);
@@ -837,4 +853,51 @@ describe('syncBundledSkillsFromPackage', () => {
     expect((fs.statSync(destSkillFile).mode & 0o777).toString(8)).toBe('600');
     expect(fs.readFileSync(destSkillFile, 'utf-8')).toBe('# Original');
   });
+
+  test('crash safe recovery: preserves most recent backup when renameSync fails', async () => {
+    const skillName = 'recovery-failure-test-skill';
+    const skillSrcDir = path.join(fakePackageRoot, 'src', 'skills', skillName);
+    fs.mkdirSync(skillSrcDir, { recursive: true });
+    fs.writeFileSync(path.join(skillSrcDir, 'SKILL.md'), '# Bundled Content');
+
+    const destSkillsDir = path.join(fakeDestConfigDir, 'skills');
+    fs.mkdirSync(destSkillsDir, { recursive: true });
+
+    const backupDir = path.join(destSkillsDir, `.backup-${skillName}-12345`);
+    fs.mkdirSync(backupDir, { recursive: true });
+    fs.writeFileSync(path.join(backupDir, 'SKILL.md'), '# Backup Content');
+
+    const manifestDir = path.join(fakeDestConfigDir, '.oh-my-opencode-slim');
+    fs.mkdirSync(manifestDir, { recursive: true });
+    const manifestPath = path.join(manifestDir, 'skills-manifest.json');
+    const initialManifest = {
+      schemaVersion: 1,
+      updatedAt: new Date().toISOString(),
+      skills: {
+        [skillName]: {
+          status: 'managed',
+          packageVersion: '1.0.0',
+          sourceHash: 'some-hash',
+          lastManagedHash: 'some-hash',
+          lastSeenHash: 'some-hash',
+          updatedAt: new Date().toISOString(),
+        },
+      },
+    };
+    fs.writeFileSync(manifestPath, JSON.stringify(initialManifest, null, 2));
+
+    shouldFailRename = true;
+
+    await syncBundledSkillsFromPackage(fakePackageRoot);
+
+    // The destination directory should not exist (since rename failed)
+    const destSkillDir = path.join(destSkillsDir, skillName);
+    expect(fs.existsSync(destSkillDir)).toBe(false);
+
+    // The most recent backup directory should still exist and not be cleaned up
+    expect(fs.existsSync(backupDir)).toBe(true);
+    expect(fs.readFileSync(path.join(backupDir, 'SKILL.md'), 'utf-8')).toBe(
+      '# Backup Content',
+    );
+  });
 });

src/hooks/auto-update-checker/skill-sync.ts

@@ -158,7 +158,11 @@ export function computeDirectoryHash(dirPath: string): string {
 
   traverse(dirPath);
 
-  entriesToHash.sort((a, b) => a.relativePath.localeCompare(b.relativePath));
+  entriesToHash.sort((a, b) => {
+    if (a.relativePath < b.relativePath) return -1;
+    if (a.relativePath > b.relativePath) return 1;
+    return 0;
+  });
 
   for (const entry of entriesToHash) {
     hash.update(entry.kind);
@@ -385,12 +389,12 @@ function recoverOrphanArtifacts(
     const mostRecentBackup = backups[backups.length - 1];
 
     if (!existsSync(destPath)) {
+      backups.pop();
       try {
         renameSync(mostRecentBackup, destPath);
         log(
           `[skill-sync] Recovered backup for ${skillName} back to destination.`,
         );
-        backups.pop();
       } catch (err) {
         log(`[skill-sync] Failed to restore backup for ${skillName}:`, err);
       }