Browse Source

fix(auto-update): surface staged skill sync outcomes

Alvin Unreal 1 month ago
parent
commit
a816f5c66f

+ 40 - 2
src/cli/install.test.ts

@@ -44,6 +44,8 @@ const originalGetExistingLiteConfigPath = actualPaths.getExistingLiteConfigPath;
 
 let importCounter = 0;
 let mockFailedResult: string[] = [];
+let mockStagedResult: string[] = [];
+let mockAdoptedResult: string[] = [];
 let enableInstallMocks = false;
 
 mock.module('../hooks/auto-update-checker/skill-sync', () => {
@@ -56,8 +58,8 @@ mock.module('../hooks/auto-update-checker/skill-sync', () => {
             skippedExisting: [],
             failed: mockFailedResult,
             updated: [],
-            staged: [],
-            adopted: [],
+            staged: mockStagedResult,
+            adopted: mockAdoptedResult,
             customized: [],
           }
         : originalSyncBundledSkillsFromPackage(packageRoot, options),
@@ -190,6 +192,8 @@ describe('install skill synchronization error mapping', () => {
   beforeEach(() => {
     enableInstallMocks = true;
     mockFailedResult = [];
+    mockStagedResult = [];
+    mockAdoptedResult = [];
     originalConsoleLog = console.log;
     logSpy = mock(() => {});
     console.log = logSpy;
@@ -281,4 +285,38 @@ describe('install skill synchronization error mapping', () => {
     expect(summaryMsg).toBeDefined();
     expect(summaryMsg).toContain('1 failed.');
   });
+
+  test('prints staged skills during sync', async () => {
+    mockStagedResult = ['staged-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);
+    expect(
+      calls.some((msg: string) =>
+        msg?.includes('Staged for review: staged-skill'),
+      ),
+    ).toBe(true);
+  });
+
+  test('prints adopted skills during sync', async () => {
+    mockAdoptedResult = ['adopted-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);
+    expect(
+      calls.some((msg: string) => msg?.includes('Adopted: adopted-skill')),
+    ).toBe(true);
+  });
 });

+ 10 - 0
src/cli/install.ts

@@ -444,6 +444,16 @@ async function runInstall(config: InstallConfig): Promise<number> {
             }
           }
         }
+        if ((result.staged ?? []).length > 0) {
+          for (const skill of result.staged ?? []) {
+            printInfo(`Staged for review: ${skill}`);
+          }
+        }
+        if ((result.adopted ?? []).length > 0) {
+          for (const skill of result.adopted ?? []) {
+            printInfo(`Adopted: ${skill}`);
+          }
+        }
 
         const realFailed = result.failed.filter(
           (skill) => skill !== '__lock__' && skill !== '__manifest__',

+ 37 - 0
src/hooks/auto-update-checker/index.test.ts

@@ -702,4 +702,41 @@ describe('auto-update-checker/index', () => {
       '/tmp/opencode',
     );
   });
+
+  test('logs staged and customized skills during startup reconciliation', async () => {
+    checkerMocks.getCurrentRuntimePackageJsonPath.mockImplementation(
+      () => '/tmp/opencode/package.json',
+    );
+    checkerMocks.findPluginEntry.mockImplementation(() => ({
+      pinnedVersion: '0.9.11',
+      isPinned: false,
+    }));
+    checkerMocks.getCachedVersion.mockImplementation(() => null);
+    skillSyncMocks.syncBundledSkillsFromPackage.mockImplementation(() => ({
+      installed: [],
+      skippedExisting: [],
+      failed: [],
+      staged: ['reflect'],
+      customized: ['my-custom-skill'],
+    }));
+
+    const { createAutoUpdateCheckerHook } = await import(
+      `./index?test=${importCounter++}`
+    );
+    const { ctx } = createCtx();
+
+    const hook = createAutoUpdateCheckerHook(ctx as never);
+    hook.event({ event: { type: 'session.created', properties: {} } });
+
+    await waitForCalls(logMock, 3);
+
+    const logs = logMock.mock.calls.map((entry: [string]) => entry[0]);
+
+    expect(logs).toContain(
+      '[auto-update-checker] Startup skill sync staged: reflect',
+    );
+    expect(logs).toContain(
+      '[auto-update-checker] Startup skill sync customized: my-custom-skill',
+    );
+  });
 });

+ 10 - 0
src/hooks/auto-update-checker/index.ts

@@ -92,6 +92,16 @@ async function runBackgroundUpdateCheck(
             `[auto-update-checker] Startup skill sync failures: ${syncResult.failed.join(', ')}`,
           );
         }
+        if ((syncResult.staged ?? []).length > 0) {
+          log(
+            `[auto-update-checker] Startup skill sync staged: ${syncResult.staged?.join(', ')}`,
+          );
+        }
+        if ((syncResult.customized ?? []).length > 0) {
+          log(
+            `[auto-update-checker] Startup skill sync customized: ${syncResult.customized?.join(', ')}`,
+          );
+        }
       } else {
         log(
           '[auto-update-checker] Could not resolve runtime package path for startup skill reconciliation',

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

@@ -498,6 +498,53 @@ describe('syncBundledSkillsFromPackage', () => {
     ).toBe(false);
   });
 
+  test('recovers conflict status when destination is now a directory', async () => {
+    const skillName = 'conflict-skill';
+    const skillSrcDir = path.join(fakePackageRoot, 'src', 'skills', skillName);
+    fs.mkdirSync(skillSrcDir, { recursive: true });
+    fs.writeFileSync(path.join(skillSrcDir, 'SKILL.md'), '# Bundled 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: 'conflict',
+          packageVersion: '1.0.0',
+          sourceHash: 'old-source-hash',
+          lastManagedHash: 'old-managed-hash',
+          lastSeenHash: 'old-seen-hash',
+          updatedAt: new Date().toISOString(),
+        },
+      },
+    };
+    fs.writeFileSync(manifestPath, JSON.stringify(initialManifest, null, 2));
+
+    const destSkillsDir = path.join(fakeDestConfigDir, 'skills');
+    fs.mkdirSync(destSkillsDir, { recursive: true });
+    const destSkillDir = path.join(destSkillsDir, skillName);
+    fs.mkdirSync(destSkillDir, { recursive: true });
+    fs.writeFileSync(
+      path.join(destSkillDir, 'SKILL.md'),
+      '# Customized Content',
+    );
+
+    const result = await syncBundledSkillsFromPackage(fakePackageRoot);
+
+    expect(result.customized).toContain(skillName);
+    const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf-8'));
+    expect(manifest.skills[skillName].status).toBe('customized');
+    const stagedPath = manifest.skills[skillName].stagedPath as string;
+    expect(stagedPath).toBeDefined();
+    expect(fs.readFileSync(path.join(stagedPath, 'SKILL.md'), 'utf-8')).toBe(
+      '# Bundled Content',
+    );
+  });
+
   test('fails closed (only installs missing) when manifest validation fails (schemaVersion mismatch)', async () => {
     const missingSkill = 'missing-skill';
     const existingSkill = 'existing-skill';
@@ -791,6 +838,37 @@ describe('syncBundledSkillsFromPackage', () => {
     expect(manifest.skills[skillName].updatedAt).not.toBe(originalTime);
   });
 
+  test('directory hashing symlink consistency: ignores symlinks when hashing, matching copy semantics', async () => {
+    const skillName = 'symlink-hash-skill';
+    const skillSrcDir = path.join(fakePackageRoot, 'src', 'skills', skillName);
+    fs.mkdirSync(skillSrcDir, { recursive: true });
+    fs.writeFileSync(path.join(skillSrcDir, 'SKILL.md'), '# Bundle Content');
+
+    // Create a symlink in the source directory
+    const linkTarget = path.join(tempDir, 'outside-target.txt');
+    fs.writeFileSync(linkTarget, 'target content');
+    fs.symlinkSync(linkTarget, path.join(skillSrcDir, 'a-link.txt'));
+
+    const { computeDirectoryHash } = await import(
+      `./skill-sync?test=${importCounter++}`
+    );
+
+    const sourceHashWithSymlink = computeDirectoryHash(skillSrcDir);
+
+    const result = await syncBundledSkillsFromPackage(fakePackageRoot);
+    expect(result.installed).toContain(skillName);
+
+    const destSkillDir = path.join(fakeDestConfigDir, 'skills', skillName);
+    expect(fs.existsSync(destSkillDir)).toBe(true);
+
+    // The destination directory should have the symlink omitted due to copyDirRecursive
+    expect(fs.existsSync(path.join(destSkillDir, 'a-link.txt'))).toBe(false);
+
+    // The computed hash of destination should match the source hash
+    const destHash = computeDirectoryHash(destSkillDir);
+    expect(destHash).toBe(sourceHashWithSymlink);
+  });
+
   test('deleted to customized: stages and marks customized', async () => {
     const skillName = 'recreated-custom-skill';
     const skillSrcDir = path.join(fakePackageRoot, 'src', 'skills', skillName);
@@ -1264,12 +1342,11 @@ describe('syncBundledSkillsFromPackage', () => {
 
     const result = await syncBundledSkillsFromPackage(fakePackageRoot);
 
-    expect(result.customized).toContain(skillName);
-    expect(
-      fs.lstatSync(path.join(destSkillDir, 'user-link')).isSymbolicLink(),
-    ).toBe(true);
+    expect(result.installed).toContain(skillName);
+    expect(result.updated).toContain(skillName);
+    expect(fs.existsSync(path.join(destSkillDir, 'user-link'))).toBe(false);
     expect(fs.readFileSync(path.join(destSkillDir, 'SKILL.md'), 'utf-8')).toBe(
-      '# Original',
+      '# Updated',
     );
   });
 

+ 55 - 11
src/hooks/auto-update-checker/skill-sync.ts

@@ -6,7 +6,6 @@ import {
   mkdirSync,
   readdirSync,
   readFileSync,
-  readlinkSync,
   renameSync,
   rmSync,
   statSync,
@@ -138,7 +137,7 @@ export function computeDirectoryHash(dirPath: string): string {
   const entriesToHash: {
     relativePath: string;
     absolutePath: string;
-    kind: 'directory' | 'file' | 'symlink';
+    kind: 'directory' | 'file';
     mode: number;
   }[] = [];
 
@@ -149,13 +148,9 @@ export function computeDirectoryHash(dirPath: string): string {
       const stat = lstatSync(absolutePath);
       const relativePath = path.relative(dirPath, absolutePath);
       if (stat.isSymbolicLink()) {
-        entriesToHash.push({
-          relativePath,
-          absolutePath,
-          kind: 'symlink',
-          mode: stat.mode,
-        });
-      } else if (stat.isDirectory()) {
+        continue;
+      }
+      if (stat.isDirectory()) {
         entriesToHash.push({
           relativePath,
           absolutePath,
@@ -192,8 +187,6 @@ export function computeDirectoryHash(dirPath: string): string {
     if (entry.kind === 'file') {
       const content = readFileSync(entry.absolutePath);
       hash.update(content);
-    } else if (entry.kind === 'symlink') {
-      hash.update(readlinkSync(entry.absolutePath));
     }
   }
 
@@ -1092,6 +1085,57 @@ export function syncBundledSkillsFromPackage(
               }
             }
           } else if (entry.status === 'conflict') {
+            if (destHash === sourceHash) {
+              entry.status = 'managed';
+              entry.packageVersion = packageVersion;
+              entry.sourceHash = sourceHash;
+              entry.lastManagedHash = sourceHash;
+              entry.lastSeenHash = sourceHash;
+              delete entry.stagedPath;
+              entry.updatedAt = new Date().toISOString();
+              adopted.push(skill.name);
+            } else {
+              try {
+                const stagedSkillDir = path.join(
+                  manifestDir,
+                  'skill-updates',
+                  packageVersion,
+                  skill.name,
+                );
+                if (entry.stagedPath && entry.stagedPath !== stagedSkillDir) {
+                  removeManagedStagedPath(
+                    entry.stagedPath,
+                    manifestDir,
+                    skill.name,
+                  );
+                }
+                if (existsSync(stagedSkillDir)) {
+                  rmSync(stagedSkillDir, { recursive: true, force: true });
+                }
+                mkdirSync(stagedSkillDir, { recursive: true });
+                copyDirRecursive(sourcePath, stagedSkillDir);
+
+                entry.status = 'customized';
+                entry.packageVersion = packageVersion;
+                entry.sourceHash = '';
+                entry.lastManagedHash = sourceHash;
+                entry.lastSeenHash = destHash;
+                entry.stagedPath = stagedSkillDir;
+                entry.updatedAt = new Date().toISOString();
+
+                staged.push(skill.name);
+                customized.push(skill.name);
+                log(
+                  `[skill-sync] Conflicted skill ${skill.name} recovered as customized and staged at ${stagedSkillDir}`,
+                );
+              } catch (err) {
+                log(
+                  `[skill-sync] Failed to stage update for conflicted skill ${skill.name}:`,
+                  err,
+                );
+                failed.push(skill.name);
+              }
+            }
             skippedExisting.push(skill.name);
           }
         } else {