Browse Source

Merge pull request #805 from alvinunreal/fix/quiet-skill-update-toast

fix: quiet repeated skill update notices
Alvin 3 weeks ago
parent
commit
0618070aca

+ 268 - 8
src/hooks/auto-update-checker/index.test.ts

@@ -29,6 +29,7 @@ const skillSyncMocks = {
     staged: [],
     adopted: [],
     customized: [],
+    stagedThisSync: [],
   })),
 };
 
@@ -121,6 +122,10 @@ describe('auto-update-checker/index', () => {
     checkerMocks.getLatestVersion.mockImplementation(async () => null);
     checkerMocks.getLocalDevVersion.mockReset();
     checkerMocks.getLocalDevVersion.mockImplementation(() => null);
+    checkerMocks.getCurrentRuntimePackageJsonPath.mockReset();
+    checkerMocks.getCurrentRuntimePackageJsonPath.mockImplementation(
+      () => null,
+    );
 
     cacheMocks.preparePackageUpdate.mockReset();
     cacheMocks.preparePackageUpdate.mockImplementation(() => '/tmp/opencode');
@@ -147,6 +152,7 @@ describe('auto-update-checker/index', () => {
       staged: [],
       adopted: [],
       customized: [],
+      stagedThisSync: [],
     }));
 
     companionUpdaterMocks.ensureCompanionVersion.mockReset();
@@ -236,13 +242,102 @@ describe('auto-update-checker/index', () => {
     expect(showToast).toHaveBeenCalledWith({
       body: {
         title: 'OMO-Slim Updated!',
-        message: 'v0.9.1 → v0.9.11\nRestart OpenCode to apply.',
+        message:
+          'v0.9.1 → v0.9.11\nRestart OpenCode to apply the plugin update.',
         variant: 'success',
         duration: 8000,
       },
     });
   });
 
+  test('shows a manual-review toast for newly staged startup skills when up to date', async () => {
+    checkerMocks.getCurrentRuntimePackageJsonPath.mockImplementation(
+      () => '/tmp/opencode/package.json',
+    );
+    checkerMocks.findPluginEntry.mockImplementation(() => ({
+      pinnedVersion: null,
+      isPinned: false,
+    }));
+    checkerMocks.getCachedVersion.mockImplementation(() => '0.9.11');
+    checkerMocks.getLatestCompatibleVersion.mockImplementation(async () => ({
+      latestVersion: '0.9.11',
+      latestMajorVersion: null,
+      blockedByMajor: false,
+    }));
+    skillSyncMocks.syncBundledSkillsFromPackage.mockImplementation(() => ({
+      installed: [],
+      skippedExisting: [],
+      failed: [],
+      staged: ['reflect'],
+      adopted: [],
+      customized: ['reflect'],
+      stagedThisSync: ['reflect'],
+    }));
+
+    const { createAutoUpdateCheckerHook } = await import(
+      `./index?test=${importCounter++}`
+    );
+    const { ctx, showToast } = createCtx();
+
+    createAutoUpdateCheckerHook(ctx as never).event({
+      event: { type: 'session.created', properties: {} },
+    });
+    await waitForCalls(showToast);
+
+    expect(showToast).toHaveBeenCalledTimes(1);
+    expect(showToast).toHaveBeenCalledWith({
+      body: {
+        title: 'Skill updates need review',
+        message: 'Manual review required: reflect',
+        variant: 'info',
+        duration: 8000,
+      },
+    });
+  });
+
+  test('shows a manual-review toast when a version-pinned update is staged at startup', async () => {
+    checkerMocks.getCurrentRuntimePackageJsonPath.mockImplementation(
+      () => '/tmp/opencode/package.json',
+    );
+    checkerMocks.findPluginEntry.mockImplementation(() => ({
+      pinnedVersion: '0.9.1',
+      isPinned: true,
+    }));
+    checkerMocks.getLatestCompatibleVersion.mockImplementation(async () => ({
+      latestVersion: '0.9.11',
+      latestMajorVersion: null,
+      blockedByMajor: false,
+    }));
+    skillSyncMocks.syncBundledSkillsFromPackage.mockImplementation(() => ({
+      installed: [],
+      skippedExisting: [],
+      failed: [],
+      staged: ['reflect'],
+      adopted: [],
+      customized: ['reflect'],
+      stagedThisSync: ['reflect'],
+    }));
+
+    const { createAutoUpdateCheckerHook } = await import(
+      `./index?test=${importCounter++}`
+    );
+    const { ctx, showToast } = createCtx();
+
+    createAutoUpdateCheckerHook(ctx as never).event({
+      event: { type: 'session.created', properties: {} },
+    });
+    await waitForCalls(showToast, 2);
+
+    expect(showToast).toHaveBeenCalledWith({
+      body: {
+        title: 'Skill updates need review',
+        message: 'Manual review required: reflect',
+        variant: 'info',
+        duration: 8000,
+      },
+    });
+  });
+
   test('includes newly installed bundled skills in success toast', async () => {
     checkerMocks.findPluginEntry.mockImplementation(() => ({
       pinnedVersion: null,
@@ -261,6 +356,7 @@ describe('auto-update-checker/index', () => {
       staged: [],
       adopted: [],
       customized: [],
+      stagedThisSync: [],
     }));
 
     const { createAutoUpdateCheckerHook } = await import(
@@ -276,14 +372,14 @@ describe('auto-update-checker/index', () => {
       body: {
         title: 'OMO-Slim Updated!',
         message:
-          'v0.9.1 → v0.9.11\nAdded bundled skills: reflect, worktrees\nRestart OpenCode to apply.',
+          'v0.9.1 → v0.9.11\nAdded bundled skills: reflect, worktrees\nRestart OpenCode to apply the plugin update.',
         variant: 'success',
         duration: 8000,
       },
     });
   });
 
-  test('includes staged and customized skills in success toast', async () => {
+  test('reports only new skill transitions in success toast without duplication', async () => {
     checkerMocks.findPluginEntry.mockImplementation(() => ({
       pinnedVersion: null,
       isPinned: false,
@@ -300,7 +396,8 @@ describe('auto-update-checker/index', () => {
       failed: [],
       staged: ['worktrees'],
       adopted: [],
-      customized: ['my-custom-skill'],
+      customized: ['worktrees', 'my-custom-skill'],
+      stagedThisSync: ['worktrees'],
     }));
 
     const { createAutoUpdateCheckerHook } = await import(
@@ -316,7 +413,115 @@ describe('auto-update-checker/index', () => {
       body: {
         title: 'OMO-Slim Updated!',
         message:
-          'v0.9.1 → v0.9.11\nAdded bundled skills: reflect\nStaged skill updates: worktrees\nCustomized skills: my-custom-skill\nRestart OpenCode to apply.',
+          'v0.9.1 → v0.9.11\nAdded bundled skills: reflect\nStaged skill updates require manual review: worktrees\nRestart OpenCode to apply the plugin update.',
+        variant: 'success',
+        duration: 8000,
+      },
+    });
+  });
+
+  test('retains staged transitions from startup reconciliation for the update toast', async () => {
+    checkerMocks.getCurrentRuntimePackageJsonPath.mockImplementation(
+      () => '/tmp/opencode/package.json',
+    );
+    checkerMocks.findPluginEntry.mockImplementation(() => ({
+      pinnedVersion: null,
+      isPinned: false,
+    }));
+    checkerMocks.getCachedVersion.mockImplementation(() => '0.9.1');
+    checkerMocks.getLatestCompatibleVersion.mockImplementation(async () => ({
+      latestVersion: '0.9.11',
+      latestMajorVersion: null,
+      blockedByMajor: false,
+    }));
+    skillSyncMocks.syncBundledSkillsFromPackage.mockImplementationOnce(() => ({
+      installed: [],
+      skippedExisting: [],
+      failed: [],
+      staged: ['reflect'],
+      adopted: [],
+      customized: ['reflect'],
+      stagedThisSync: ['reflect'],
+    }));
+    skillSyncMocks.syncBundledSkillsFromPackage.mockImplementationOnce(() => ({
+      installed: [],
+      skippedExisting: [],
+      failed: [],
+      staged: [],
+      adopted: [],
+      customized: ['reflect'],
+      stagedThisSync: [],
+    }));
+
+    const { createAutoUpdateCheckerHook } = await import(
+      `./index?test=${importCounter++}`
+    );
+    const { ctx, showToast } = createCtx();
+
+    createAutoUpdateCheckerHook(ctx as never).event({
+      event: { type: 'session.created', properties: {} },
+    });
+    await waitForCalls(showToast);
+
+    expect(showToast).toHaveBeenCalledWith({
+      body: {
+        title: 'OMO-Slim Updated!',
+        message:
+          'v0.9.1 → v0.9.11\nStaged skill updates require manual review: reflect\nRestart OpenCode to apply the plugin update.',
+        variant: 'success',
+        duration: 8000,
+      },
+    });
+  });
+
+  test('removes startup staged transitions adopted by post-install sync', async () => {
+    checkerMocks.getCurrentRuntimePackageJsonPath.mockImplementation(
+      () => '/tmp/opencode/package.json',
+    );
+    checkerMocks.findPluginEntry.mockImplementation(() => ({
+      pinnedVersion: null,
+      isPinned: false,
+    }));
+    checkerMocks.getCachedVersion.mockImplementation(() => '0.9.1');
+    checkerMocks.getLatestCompatibleVersion.mockImplementation(async () => ({
+      latestVersion: '0.9.11',
+      latestMajorVersion: null,
+      blockedByMajor: false,
+    }));
+    skillSyncMocks.syncBundledSkillsFromPackage.mockImplementationOnce(() => ({
+      installed: [],
+      skippedExisting: [],
+      failed: [],
+      staged: ['reflect'],
+      adopted: [],
+      customized: ['reflect'],
+      stagedThisSync: ['reflect'],
+    }));
+    skillSyncMocks.syncBundledSkillsFromPackage.mockImplementationOnce(() => ({
+      installed: [],
+      skippedExisting: [],
+      failed: [],
+      staged: [],
+      adopted: ['reflect'],
+      customized: [],
+      stagedThisSync: ['reflect'],
+    }));
+
+    const { createAutoUpdateCheckerHook } = await import(
+      `./index?test=${importCounter++}`
+    );
+    const { ctx, showToast } = createCtx();
+
+    createAutoUpdateCheckerHook(ctx as never).event({
+      event: { type: 'session.created', properties: {} },
+    });
+    await waitForCalls(showToast);
+
+    expect(showToast).toHaveBeenCalledWith({
+      body: {
+        title: 'OMO-Slim Updated!',
+        message:
+          'v0.9.1 → v0.9.11\nRestart OpenCode to apply the plugin update.',
         variant: 'success',
         duration: 8000,
       },
@@ -375,7 +580,7 @@ describe('auto-update-checker/index', () => {
       body: {
         title: 'OMO-Slim Updated!',
         message:
-          'v0.9.1 → v0.9.11\nCompanion updated.\nRestart OpenCode to apply.',
+          'v0.9.1 → v0.9.11\nCompanion updated.\nRestart OpenCode to apply the plugin update.',
         variant: 'success',
         duration: 8000,
       },
@@ -416,7 +621,7 @@ describe('auto-update-checker/index', () => {
       body: {
         title: 'OMO-Slim Updated!',
         message:
-          'v0.9.1 → v0.9.11\nCompanion update will retry on restart.\nRestart OpenCode to apply.',
+          'v0.9.1 → v0.9.11\nCompanion update will retry on restart.\nRestart OpenCode to apply the plugin update.',
         variant: 'success',
         duration: 8000,
       },
@@ -441,6 +646,7 @@ describe('auto-update-checker/index', () => {
       staged: [],
       adopted: [],
       customized: [],
+      stagedThisSync: [],
     }));
 
     const { createAutoUpdateCheckerHook } = await import(
@@ -455,7 +661,8 @@ describe('auto-update-checker/index', () => {
     expect(showToast).toHaveBeenCalledWith({
       body: {
         title: 'OMO-Slim Updated!',
-        message: 'v0.9.1 → v0.9.11\nRestart OpenCode to apply.',
+        message:
+          'v0.9.1 → v0.9.11\nRestart OpenCode to apply the plugin update.',
         variant: 'success',
         duration: 8000,
       },
@@ -582,6 +789,58 @@ describe('auto-update-checker/index', () => {
     });
   });
 
+  test('shows a manual-review toast when installation fails after startup staging', async () => {
+    checkerMocks.getCurrentRuntimePackageJsonPath.mockImplementation(
+      () => '/tmp/opencode/package.json',
+    );
+    checkerMocks.findPluginEntry.mockImplementation(() => ({
+      pinnedVersion: null,
+      isPinned: false,
+    }));
+    checkerMocks.getCachedVersion.mockImplementation(() => '0.9.1');
+    checkerMocks.getLatestCompatibleVersion.mockImplementation(async () => ({
+      latestVersion: '0.9.11',
+      latestMajorVersion: null,
+      blockedByMajor: false,
+    }));
+    skillSyncMocks.syncBundledSkillsFromPackage.mockImplementation(() => ({
+      installed: [],
+      skippedExisting: [],
+      failed: [],
+      staged: ['reflect'],
+      adopted: [],
+      customized: ['reflect'],
+      stagedThisSync: ['reflect'],
+    }));
+    crossSpawnMock.mockImplementation(() => ({
+      exited: Promise.resolve(1),
+      exitCode: 1,
+      kill: mock(() => true),
+      stdout: () => Promise.resolve(''),
+      stderr: () => Promise.resolve(''),
+      proc: {} as never,
+    }));
+
+    const { createAutoUpdateCheckerHook } = await import(
+      `./index?test=${importCounter++}`
+    );
+    const { ctx, showToast } = createCtx();
+
+    createAutoUpdateCheckerHook(ctx as never).event({
+      event: { type: 'session.created', properties: {} },
+    });
+    await waitForCalls(showToast, 2);
+
+    expect(showToast).toHaveBeenCalledWith({
+      body: {
+        title: 'Skill updates need review',
+        message: 'Manual review required: reflect',
+        variant: 'info',
+        duration: 8000,
+      },
+    });
+  });
+
   test('does not auto-update across major versions', async () => {
     checkerMocks.findPluginEntry.mockImplementation(() => ({
       pinnedVersion: null,
@@ -732,6 +991,7 @@ describe('auto-update-checker/index', () => {
       staged: ['reflect'],
       adopted: [],
       customized: ['my-custom-skill'],
+      stagedThisSync: [],
     }));
 
     const { createAutoUpdateCheckerHook } = await import(

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

@@ -73,6 +73,8 @@ async function runBackgroundUpdateCheck(
   autoUpdate: boolean,
   companion: AutoUpdateCheckerOptions['companion'],
 ): Promise<void> {
+  const stagedSkillsThisUpdate = new Set<string>();
+
   // Startup reconciliation (run once per top-level startup)
   if (!hasReconciledAtStartup) {
     try {
@@ -82,6 +84,9 @@ async function runBackgroundUpdateCheck(
         const packageRoot = path.dirname(runtimePackageJsonPath);
         log('[auto-update-checker] Running startup skill reconciliation');
         const syncResult = syncBundledSkillsFromPackage(packageRoot);
+        for (const skill of syncResult.stagedThisSync) {
+          stagedSkillsThisUpdate.add(skill);
+        }
         if (syncResult.installed.length > 0) {
           log(
             `[auto-update-checker] Startup skill sync installed: ${syncResult.installed.join(', ')}`,
@@ -115,6 +120,7 @@ async function runBackgroundUpdateCheck(
   const pluginInfo = findPluginEntry(ctx.directory);
   if (!pluginInfo) {
     log('[auto-update-checker] Plugin not found in config');
+    showStagedSkillsReviewToast(ctx, stagedSkillsThisUpdate);
     return;
   }
 
@@ -122,6 +128,7 @@ async function runBackgroundUpdateCheck(
   const currentVersion = cachedVersion ?? pluginInfo.pinnedVersion;
   if (!currentVersion) {
     log('[auto-update-checker] No version found (cached or pinned)');
+    showStagedSkillsReviewToast(ctx, stagedSkillsThisUpdate);
     return;
   }
 
@@ -140,6 +147,7 @@ async function runBackgroundUpdateCheck(
         8000,
       );
     }
+    showStagedSkillsReviewToast(ctx, stagedSkillsThisUpdate);
     return;
   }
 
@@ -148,6 +156,7 @@ async function runBackgroundUpdateCheck(
     log(
       `[auto-update-checker] Major update available; skipping auto-update: ${latestInfo.latestMajorVersion}`,
     );
+    showStagedSkillsReviewToast(ctx, stagedSkillsThisUpdate);
     return;
   }
 
@@ -157,6 +166,7 @@ async function runBackgroundUpdateCheck(
       '[auto-update-checker] Failed to fetch latest version for channel:',
       channel,
     );
+    showStagedSkillsReviewToast(ctx, stagedSkillsThisUpdate);
     return;
   }
 
@@ -165,6 +175,7 @@ async function runBackgroundUpdateCheck(
       '[auto-update-checker] Already on latest version for channel:',
       channel,
     );
+    showStagedSkillsReviewToast(ctx, stagedSkillsThisUpdate);
     return;
   }
 
@@ -181,6 +192,7 @@ async function runBackgroundUpdateCheck(
       8000,
     );
     log(`[auto-update-checker] Version is pinned; skipping auto-update.`);
+    showStagedSkillsReviewToast(ctx, stagedSkillsThisUpdate);
     return;
   }
 
@@ -193,6 +205,7 @@ async function runBackgroundUpdateCheck(
       8000,
     );
     log('[auto-update-checker] Auto-update disabled, notification only');
+    showStagedSkillsReviewToast(ctx, stagedSkillsThisUpdate);
     return;
   }
 
@@ -206,6 +219,7 @@ async function runBackgroundUpdateCheck(
       8000,
     );
     log('[auto-update-checker] Failed to prepare install root for auto-update');
+    showStagedSkillsReviewToast(ctx, stagedSkillsThisUpdate);
     return;
   }
 
@@ -213,16 +227,18 @@ async function runBackgroundUpdateCheck(
 
   if (installSuccess) {
     let installedSkills: string[] = [];
-    let stagedSkills: string[] = [];
-    let customizedSkills: string[] = [];
     let companionUpdated = false;
     let companionWillRetry = false;
     const packageRoot = path.join(installDir, 'node_modules', PACKAGE_NAME);
     try {
       const syncResult = syncBundledSkillsFromPackage(packageRoot);
       installedSkills = syncResult.installed;
-      stagedSkills = syncResult.staged;
-      customizedSkills = syncResult.customized;
+      for (const skill of syncResult.stagedThisSync) {
+        stagedSkillsThisUpdate.add(skill);
+      }
+      for (const skill of [...syncResult.installed, ...syncResult.adopted]) {
+        stagedSkillsThisUpdate.delete(skill);
+      }
       if (syncResult.failed.length > 0) {
         log(
           `[auto-update-checker] Skill sync warnings/failures: ${syncResult.failed.join(', ')}`,
@@ -271,18 +287,17 @@ async function runBackgroundUpdateCheck(
     if (installedSkills.length > 0) {
       messageLines.push(`Added bundled skills: ${installedSkills.join(', ')}`);
     }
-    if (stagedSkills.length > 0) {
-      messageLines.push(`Staged skill updates: ${stagedSkills.join(', ')}`);
-    }
-    if (customizedSkills.length > 0) {
-      messageLines.push(`Customized skills: ${customizedSkills.join(', ')}`);
+    if (stagedSkillsThisUpdate.size > 0) {
+      messageLines.push(
+        `Staged skill updates require manual review: ${[...stagedSkillsThisUpdate].join(', ')}`,
+      );
     }
     if (companionUpdated) {
       messageLines.push('Companion updated.');
     } else if (companionWillRetry) {
       messageLines.push('Companion update will retry on restart.');
     }
-    messageLines.push('Restart OpenCode to apply.');
+    messageLines.push('Restart OpenCode to apply the plugin update.');
 
     showToast(
       ctx,
@@ -303,6 +318,7 @@ async function runBackgroundUpdateCheck(
       8000,
     );
     log('[auto-update-checker] bun install failed; update not installed');
+    showStagedSkillsReviewToast(ctx, stagedSkillsThisUpdate);
   }
 }
 
@@ -316,6 +332,21 @@ function showMajorUpgradeToast(ctx: PluginInput, version: string): void {
   );
 }
 
+function showStagedSkillsReviewToast(
+  ctx: PluginInput,
+  stagedSkills: ReadonlySet<string>,
+): void {
+  if (stagedSkills.size === 0) return;
+
+  showToast(
+    ctx,
+    'Skill updates need review',
+    `Manual review required: ${[...stagedSkills].join(', ')}`,
+    'info',
+    8000,
+  );
+}
+
 export function getAutoUpdateInstallDir(): string {
   return resolveInstallContext()?.installDir ?? CACHE_DIR;
 }

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

@@ -406,6 +406,11 @@ describe('syncBundledSkillsFromPackage', () => {
     );
 
     expect(manifest.skills[skillName].status).toBe('customized');
+    expect(result.stagedThisSync).toEqual([skillName]);
+
+    const unchangedResult = await syncBundledSkillsFromPackage(fakePackageRoot);
+    expect(unchangedResult.customized).toEqual([skillName]);
+    expect(unchangedResult.stagedThisSync).toEqual([]);
   });
 
   test('fails closed (only installs missing) when manifest is corrupt', async () => {

+ 12 - 0
src/hooks/auto-update-checker/skill-sync.ts

@@ -37,6 +37,7 @@ export interface SkillSyncResult {
   staged: string[];
   adopted: string[];
   customized: string[];
+  stagedThisSync: string[];
 }
 
 export interface SkillManifestEntry {
@@ -558,6 +559,7 @@ export function syncBundledSkillsFromPackage(
   const staged: string[] = [];
   const adopted: string[] = [];
   const customized: string[] = [];
+  const stagedThisSync: string[] = [];
 
   const sourceSkillsDir = path.join(packageRoot, 'src', 'skills');
 
@@ -574,6 +576,7 @@ export function syncBundledSkillsFromPackage(
         staged,
         adopted,
         customized,
+        stagedThisSync,
       };
     }
   } catch {
@@ -587,6 +590,7 @@ export function syncBundledSkillsFromPackage(
       staged,
       adopted,
       customized,
+      stagedThisSync,
     };
   }
 
@@ -630,6 +634,7 @@ export function syncBundledSkillsFromPackage(
       staged,
       adopted,
       customized,
+      stagedThisSync,
     };
   }
 
@@ -793,6 +798,7 @@ export function syncBundledSkillsFromPackage(
                 };
                 staged.push(skill.name);
                 customized.push(skill.name);
+                stagedThisSync.push(skill.name);
               } catch (err) {
                 log(
                   `[skill-sync] Failed to stage update for customized skill ${skill.name} during recovery:`,
@@ -963,6 +969,7 @@ export function syncBundledSkillsFromPackage(
 
                   staged.push(skill.name);
                   customized.push(skill.name);
+                  stagedThisSync.push(skill.name);
                   skippedExisting.push(skill.name);
                   log(
                     `[skill-sync] Skill ${skill.name} is customized. Staged update at ${stagedSkillDir}`,
@@ -1028,6 +1035,7 @@ export function syncBundledSkillsFromPackage(
 
                   staged.push(skill.name);
                   customized.push(skill.name);
+                  stagedThisSync.push(skill.name);
                   skippedExisting.push(skill.name);
                   log(
                     `[skill-sync] Staged new update for customized skill ${skill.name} at ${stagedSkillDir}`,
@@ -1088,6 +1096,7 @@ export function syncBundledSkillsFromPackage(
 
                 staged.push(skill.name);
                 customized.push(skill.name);
+                stagedThisSync.push(skill.name);
                 skippedExisting.push(skill.name);
                 log(
                   `[skill-sync] Skill ${skill.name} re-created by user (custom). Marked customized and staged.`,
@@ -1148,6 +1157,7 @@ export function syncBundledSkillsFromPackage(
 
                 staged.push(skill.name);
                 customized.push(skill.name);
+                stagedThisSync.push(skill.name);
                 skippedExisting.push(skill.name);
                 log(
                   `[skill-sync] Conflicted skill ${skill.name} recovered as customized and staged at ${stagedSkillDir}`,
@@ -1223,6 +1233,7 @@ export function syncBundledSkillsFromPackage(
               };
               staged.push(skill.name);
               customized.push(skill.name);
+              stagedThisSync.push(skill.name);
               skippedExisting.push(skill.name);
               log(
                 `[skill-sync] Skill ${skill.name} is customized (no manifest entry). Staged update at ${stagedSkillDir}`,
@@ -1276,5 +1287,6 @@ export function syncBundledSkillsFromPackage(
     staged,
     adopted,
     customized,
+    stagedThisSync,
   };
 }