fix(skills): stop the skill review dialog contradicting the badge that opens it (#11128)

* fix(skills): stop the skill review dialog contradicting the badge that opens it

A skill whose only fault was an edited copy or one Orca could not read turned the
setup-rail badge amber and offered Details — and Details opened a dialog headlined
"All installed Orca skills are up to date." over an empty list. The badge says
something is wrong, the dialog it points at says nothing is.

The grouping only returned skills with an out-of-date copy, so those two states
produced no row and the summary fell through to the all-clear headline. Include a
skill when a copy needs attention as well, using one shared predicate so the badge
and the dialog cannot disagree again. A plugin's own copy of a same-named skill
stays out: that is the vendor's, not the user's drift.

* test(skills): pin that a routine outdated copy raises no attention marker

src/renderer/src/components/skills/skill-freshness-grouping.test.ts

@@ -39,16 +39,64 @@ describe('groupSkillFreshness', () => {
     ])
   })
 
-  it('hides skills with nothing out of date (current, unrecognized-only, unreadable-only)', () => {
+  it('hides skills whose every copy is current', () => {
     const groups = groupSkillFreshness(
       [
         placement('orca-cli', { status: 'current' }),
+        placement('orca-cli', { status: 'current', topology: 'provider-alias' })
+      ],
+      []
+    )
+    expect(groups).toEqual([])
+  })
+
+  it('keeps a plugin-managed copy of a same-named skill out of the list', () => {
+    // Why: the vendor owns that copy, so it is not drift the user can act on —
+    // reporting it put permanent amber on any ordinary plugin install.
+    const groups = groupSkillFreshness(
+      [placement('dataviz', { status: 'unrecognized', topology: 'plugin-cache' })],
+      []
+    )
+    expect(groups).toEqual([])
+  })
+
+  it('lists a skill whose only fault cannot be fixed by the update, so Details explains it', () => {
+    // Why: these turn the setup-rail badge amber and its Details opens this list. Omitting
+    // them left that list headlined "all up to date" over nothing, contradicting the badge.
+    const groups = groupSkillFreshness(
+      [
         placement('dataviz', { status: 'unrecognized', topology: 'independent-copy' }),
         placement('linear-tickets', { status: 'inaccessible' })
       ],
       []
     )
-    expect(groups).toEqual([])
+    expect(groups).toEqual([
+      {
+        name: 'dataviz',
+        status: 'cannot-update',
+        locations: [
+          { id: expect.any(String), path: '/home/.agents/skills/dataviz', chip: 'unrecognized' }
+        ]
+      },
+      {
+        name: 'linear-tickets',
+        status: 'cannot-update',
+        locations: [
+          {
+            id: expect.any(String),
+            path: '/home/.agents/skills/linear-tickets',
+            chip: 'inaccessible'
+          }
+        ]
+      }
+    ])
+  })
+
+  it('lists an edited canonical copy, which is what an update would overwrite', () => {
+    const groups = groupSkillFreshness([placement('orchestration', { status: 'unrecognized' })], [])
+    expect(groups).toHaveLength(1)
+    expect(groups[0]?.status).toBe('cannot-update')
+    expect(groups[0]?.locations[0]?.chip).toBe('unrecognized')
   })
 
   it('groups a blocked skill and flags the culprit location, not the main copy', () => {

src/renderer/src/components/skills/skill-freshness-grouping.ts

@@ -1,4 +1,7 @@
-import type { SkillFreshnessInstallation } from '../../../../shared/skill-freshness'
+import {
+  isSkillCopyNeedingAttention,
+  type SkillFreshnessInstallation
+} from '../../../../shared/skill-freshness'
 
 export type SkillGroupStatus = 'update-available' | 'cannot-update'
 
@@ -57,10 +60,15 @@ export function locationChip(installation: SkillFreshnessInstallation): SkillLoc
 }
 
 /**
- * Groups installations by skill for the update modal and derives each skill's
- * update disposition. Only skills with an out-of-date official copy are returned —
- * up-to-date, unrecognized-only, and unreadable-only skills have nothing to change
- * here, so they are omitted entirely.
+ * Groups installations by skill for the review modal and derives each skill's
+ * update disposition. A skill appears when a copy is out of date, or when a copy is
+ * wrong in a way the update cannot fix — an edited copy, one Orca could not read.
+ *
+ * That second half is what the badge already reports: it turns amber and offers
+ * Details, and Details opens this modal. Returning only out-of-date skills left that
+ * modal saying every skill was up to date over an empty list, contradicting the badge
+ * that sent the user there. Skills where every copy is current are still omitted, as
+ * is a plugin's own copy of a same-named skill, which is not the user's to fix.
  *
  * `alwaysIncludeNames` overrides that filter. A successful update makes every
  * targeted skill current, which would otherwise drop its row the instant the
@@ -82,7 +90,10 @@ export function groupSkillFreshness(
   }
   const groups: SkillFreshnessGroupModel[] = []
   for (const [name, entries] of byName) {
-    if (!pinned.has(name) && !entries.some((entry) => entry.status === 'outdated')) {
+    if (
+      !pinned.has(name) &&
+      !entries.some((entry) => entry.status === 'outdated' || isSkillCopyNeedingAttention(entry))
+    ) {
       continue
     }
     const locations = entries

src/renderer/src/lib/skill-freshness-display-status.test.ts

@@ -167,6 +167,14 @@ describe('getSkillFreshnessDisplayStatus', () => {
 })
 
 describe('hasSkillCopyNeedingAttention', () => {
+  it('raises no attention marker over a routine outdated copy the update converges', () => {
+    // Why: an out-of-date convergent copy is ordinary work for the update command.
+    // Amber on the review affordance there would cry wolf on every routine update.
+    expect(
+      hasSkillCopyNeedingAttention(inventory([placement('outdated')], [SKILL_NAME]), SKILL_NAME)
+    ).toBe(false)
+  })
+
   it('ignores a traversal bound but keeps a real scan fault', () => {
     expect(
       hasSkillCopyNeedingAttention(

src/renderer/src/lib/skill-freshness-display-status.ts

@@ -1,6 +1,6 @@
 import {
+  isSkillCopyNeedingAttention,
   isSkillScanIssueNeedingAttention,
-  SUPPORTED_GLOBAL_SKILL_TOPOLOGIES,
   type SkillFreshnessInventory
 } from '../../../shared/skill-freshness'
 
@@ -63,15 +63,6 @@ export function hasSkillCopyNeedingAttention(
   return (
     (placements.length > 0 &&
       Boolean(inventory?.scanIssues.some(isSkillScanIssueNeedingAttention))) ||
-    placements.some(
-      (installation) =>
-        installation.status !== 'current' &&
-        !(installation.status === 'unrecognized' && installation.topology === 'plugin-cache') &&
-        // Why: an out-of-date copy the command converges is ordinary work, not a problem.
-        !(
-          SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(installation.topology) &&
-          installation.status === 'outdated'
-        )
-    )
+    placements.some(isSkillCopyNeedingAttention)
   )
 }

src/shared/skill-freshness.ts

@@ -86,6 +86,26 @@ export type SkillFreshnessInstallation = {
   errorCategory: string | null
 }
 
+/**
+ * Whether a copy is wrong in a way running the update would not resolve.
+ *
+ * Shared so the badge and the review dialog cannot disagree about what counts: the
+ * badge points at the dialog for the explanation, so a copy that turns the badge
+ * amber must also produce a row there. An out-of-date copy the command converges is
+ * ordinary work, not a problem, and a plugin's own copy of a same-named skill is the
+ * vendor's business rather than the user's drift.
+ */
+export function isSkillCopyNeedingAttention(installation: SkillFreshnessInstallation): boolean {
+  return (
+    installation.status !== 'current' &&
+    !(installation.status === 'unrecognized' && installation.topology === 'plugin-cache') &&
+    !(
+      SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(installation.topology) &&
+      installation.status === 'outdated'
+    )
+  )
+}
+
 export type SkillFreshnessScanIssueReason =
   | 'depth-limit'
   | 'entry-limit'