fix(skill-sync): harden lock and manifest recovery

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

@@ -886,6 +886,33 @@ describe('syncBundledSkillsFromPackage', () => {
     fs.mkdirSync(skillSrcDir, { recursive: true });
     fs.writeFileSync(path.join(skillSrcDir, 'SKILL.md'), '# Bundled Content');
 
+    const customizedSkillName = 'reconciliation-customized-skill';
+    const customizedSkillSrcDir = path.join(
+      fakePackageRoot,
+      'src',
+      'skills',
+      customizedSkillName,
+    );
+    fs.mkdirSync(customizedSkillSrcDir, { recursive: true });
+    fs.writeFileSync(
+      path.join(customizedSkillSrcDir, 'SKILL.md'),
+      '# Clean Source',
+    );
+
+    const destSkillsDir = path.join(fakeDestConfigDir, 'skills');
+    fs.mkdirSync(destSkillsDir, { recursive: true });
+
+    // Customized skill exists at destination but with customized content
+    const destCustomizedSkillDir = path.join(
+      destSkillsDir,
+      customizedSkillName,
+    );
+    fs.mkdirSync(destCustomizedSkillDir, { recursive: true });
+    fs.writeFileSync(
+      path.join(destCustomizedSkillDir, 'SKILL.md'),
+      '# Customized Content',
+    );
+
     const manifestDir = path.join(fakeDestConfigDir, '.oh-my-opencode-slim');
     fs.mkdirSync(manifestDir, { recursive: true });
     const manifestPath = path.join(manifestDir, 'skills-manifest.json');
@@ -903,6 +930,8 @@ describe('syncBundledSkillsFromPackage', () => {
 
     // Since destination didn't exist, it should install it
     expect(result.installed).toContain(skillName);
+    expect(result.skippedExisting).toContain(customizedSkillName);
+    expect(result.customized).toContain(customizedSkillName);
 
     // The manifest should be successfully reconciled and written as valid JSON
     expect(fs.existsSync(manifestPath)).toBe(true);
@@ -912,6 +941,41 @@ describe('syncBundledSkillsFromPackage', () => {
     expect(parsed.schemaVersion).toBe(1);
     expect(parsed.skills[skillName].status).toBe('managed');
     expect(parsed.skills[skillName].packageVersion).toBe('1.2.3');
+
+    // Customized skill should be marked customized with correct stagedPath
+    const custEntry = parsed.skills[customizedSkillName];
+    expect(custEntry.status).toBe('customized');
+    expect(custEntry.stagedPath).toBeDefined();
+    expect(fs.existsSync(custEntry.stagedPath)).toBe(true);
+    expect(
+      fs.readFileSync(path.join(custEntry.stagedPath, 'SKILL.md'), 'utf-8'),
+    ).toBe('# Clean Source');
+  });
+
+  test('lock owner-safety: releaseLock cleans up its own lock even if owner.json is missing', async () => {
+    const { acquireLock, releaseLock } = await import(
+      `./skill-sync?test=${importCounter++}`
+    );
+    const lockDir = path.join(
+      fakeDestConfigDir,
+      'test-missing-owner-json.lock',
+    );
+
+    // Acquire the lock first
+    const acquired = acquireLock(lockDir);
+    expect(acquired).toBe(true);
+
+    // Delete owner.json to simulate missing metadata
+    const metadataPath = path.join(lockDir, 'owner.json');
+    if (fs.existsSync(metadataPath)) {
+      fs.unlinkSync(metadataPath);
+    }
+
+    // Now call releaseLock
+    releaseLock(lockDir);
+
+    // The lock directory should be deleted successfully!
+    expect(fs.existsSync(lockDir)).toBe(false);
   });
 
   test('staged path safety: does not delete staging directories outside managed root', async () => {
@@ -1024,6 +1088,39 @@ describe('syncBundledSkillsFromPackage', () => {
     expect(fs.existsSync(lockDir)).toBe(false);
   });
 
+  test('lock owner-safety: releaseLock does not delete lock if metadata is overwritten by a foreign owner', async () => {
+    const { acquireLock, releaseLock } = await import(
+      `./skill-sync?test=${importCounter++}`
+    );
+    const lockDir = path.join(
+      fakeDestConfigDir,
+      'test-overwritten-metadata.lock',
+    );
+
+    // Acquire and track the path in memory
+    const acquired = acquireLock(lockDir);
+    expect(acquired).toBe(true);
+
+    // Overwrite owner.json with a foreign owner
+    const foreignOwner = {
+      pid: 99999,
+      host: 'foreign-host',
+      time: Date.now(),
+      token: 'foreign-token',
+    };
+    fs.writeFileSync(
+      path.join(lockDir, 'owner.json'),
+      JSON.stringify(foreignOwner),
+      'utf-8',
+    );
+
+    // releaseLock should NOT delete the lock directory because metadata is authoritative and foreign
+    releaseLock(lockDir);
+
+    expect(fs.existsSync(lockDir)).toBe(true);
+    expect(fs.existsSync(path.join(lockDir, 'owner.json'))).toBe(true);
+  });
+
   test('crash safe recovery: recovers backup directory when destination directory is missing', async () => {
     const skillName = 'recovery-test-skill';
     const skillSrcDir = path.join(fakePackageRoot, 'src', 'skills', skillName);

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

@@ -25,6 +25,8 @@ if (!localProcessToken) {
 }
 const PROCESS_TOKEN = localProcessToken;
 
+const ACQUIRED_LOCKS = new Set<string>();
+
 export interface SkillSyncResult {
   installed: string[];
   skippedExisting: string[];
@@ -212,7 +214,7 @@ const CROSS_HOST_LOCK_EXPIRY_MS = 5 * 60 * 1000; // 5 minutes
  * Avoids stealing active locks purely by time; writes owner metadata
  * and only steals dead same-host pid if detectable.
  */
-function acquireLock(lockDir: string): boolean {
+export function acquireLock(lockDir: string): boolean {
   const metadataPath = path.join(lockDir, 'owner.json');
   const currentHost = os.hostname();
   const currentPid = process.pid;
@@ -234,6 +236,7 @@ function acquireLock(lockDir: string): boolean {
   try {
     mkdirSync(lockDir);
     writeMetadata();
+    ACQUIRED_LOCKS.add(path.resolve(lockDir));
     return true;
   } catch (err) {
     if ((err as { code?: string }).code !== 'EEXIST') {
@@ -287,6 +290,7 @@ function acquireLock(lockDir: string): boolean {
     fs.rmSync(lockDir, { recursive: true, force: true });
     mkdirSync(lockDir);
     writeMetadata();
+    ACQUIRED_LOCKS.add(path.resolve(lockDir));
     return true;
   } catch (err) {
     log(`[skill-sync] Failed to check/recover lock at ${lockDir}:`, err);
@@ -298,29 +302,45 @@ function acquireLock(lockDir: string): boolean {
  * Releases the lock.
  */
 export function releaseLock(lockDir: string): void {
+  const resolvedPath = path.resolve(lockDir);
   try {
+    let isOurLock = false;
     const metadataPath = path.join(lockDir, 'owner.json');
+
     if (existsSync(metadataPath)) {
-      const content = fs.readFileSync(metadataPath, 'utf-8');
-      const metadata = JSON.parse(content);
-      if (
-        metadata.host === os.hostname() &&
-        metadata.pid === process.pid &&
-        metadata.token === PROCESS_TOKEN
-      ) {
+      try {
+        const content = fs.readFileSync(metadataPath, 'utf-8');
+        const metadata = JSON.parse(content);
+        if (
+          metadata.host === os.hostname() &&
+          metadata.pid === process.pid &&
+          metadata.token === PROCESS_TOKEN
+        ) {
+          isOurLock = true;
+        } else {
+          isOurLock = false;
+        }
+      } catch (err) {
+        log(`[skill-sync] Lock owner.json is unreadable/corrupt:`, err);
+        isOurLock = false;
+      }
+    } else if (ACQUIRED_LOCKS.has(resolvedPath)) {
+      isOurLock = true;
+    }
+
+    if (isOurLock) {
+      if (existsSync(lockDir)) {
         fs.rmSync(lockDir, { recursive: true, force: true });
-      } else {
-        log(
-          `[skill-sync] Skipping lock directory removal: lock is now owned by host=${metadata.host}, pid=${metadata.pid}, token=${metadata.token}`,
-        );
       }
     } else if (existsSync(lockDir)) {
       log(
-        `[skill-sync] Skipping lock directory removal: lock directory exists but owner.json was missing.`,
+        `[skill-sync] Skipping lock directory removal: lock is not owned by this process/token or owner.json check failed.`,
       );
     }
   } catch (err) {
     log(`[skill-sync] Failed to release lock at ${lockDir}:`, err);
+  } finally {
+    ACQUIRED_LOCKS.delete(resolvedPath);
   }
 }
 
@@ -740,14 +760,44 @@ export function syncBundledSkillsFromPackage(
                 updatedAt: new Date().toISOString(),
               };
             } else {
-              manifest.skills[skill.name] = {
-                status: 'customized',
-                packageVersion,
-                sourceHash,
-                lastManagedHash: '',
-                lastSeenHash: destHash,
-                updatedAt: new Date().toISOString(),
-              };
+              try {
+                const stagedSkillDir = path.join(
+                  manifestDir,
+                  'skill-updates',
+                  packageVersion,
+                  skill.name,
+                );
+                if (existsSync(stagedSkillDir)) {
+                  fs.rmSync(stagedSkillDir, { recursive: true, force: true });
+                }
+                mkdirSync(stagedSkillDir, { recursive: true });
+                copyDirRecursive(sourcePath, stagedSkillDir);
+
+                manifest.skills[skill.name] = {
+                  status: 'customized',
+                  packageVersion,
+                  sourceHash,
+                  lastManagedHash: '',
+                  lastSeenHash: destHash,
+                  stagedPath: stagedSkillDir,
+                  updatedAt: new Date().toISOString(),
+                };
+                staged.push(skill.name);
+                customized.push(skill.name);
+              } catch (err) {
+                log(
+                  `[skill-sync] Failed to stage update for customized skill ${skill.name} during recovery:`,
+                  err,
+                );
+                manifest.skills[skill.name] = {
+                  status: 'customized',
+                  packageVersion: 'unknown',
+                  sourceHash: '',
+                  lastManagedHash: '',
+                  lastSeenHash: destHash,
+                  updatedAt: new Date().toISOString(),
+                };
+              }
             }
           }
           continue;