diff --git a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts b/src/renderer/src/components/skills/skill-freshness-grouping.test.ts index d305353d950..c4913f678da 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts +++ b/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', () => { diff --git a/src/renderer/src/components/skills/skill-freshness-grouping.ts b/src/renderer/src/components/skills/skill-freshness-grouping.ts index ee2ebbbf3e0..2a306b44c72 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.ts +++ b/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 diff --git a/src/renderer/src/lib/skill-freshness-display-status.test.ts b/src/renderer/src/lib/skill-freshness-display-status.test.ts index b619e59e854..f8bdf54d1b1 100644 --- a/src/renderer/src/lib/skill-freshness-display-status.test.ts +++ b/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( diff --git a/src/renderer/src/lib/skill-freshness-display-status.ts b/src/renderer/src/lib/skill-freshness-display-status.ts index 0da1e08cb52..029725106a8 100644 --- a/src/renderer/src/lib/skill-freshness-display-status.ts +++ b/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) ) } diff --git a/src/shared/skill-freshness.ts b/src/shared/skill-freshness.ts index 0daddd4ed58..5009d3b2c87 100644 --- a/src/shared/skill-freshness.ts +++ b/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'