From f5216bcbaf0bf3f3b3146434f746746937c87656 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:49:18 -0700 Subject: [PATCH] fix(skills): stop the skill review dialog contradicting the badge that opens it (#11128) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .../skills/skill-freshness-grouping.test.ts | 52 ++++++++++++++++++- .../skills/skill-freshness-grouping.ts | 23 +++++--- .../skill-freshness-display-status.test.ts | 8 +++ .../src/lib/skill-freshness-display-status.ts | 13 +---- src/shared/skill-freshness.ts | 20 +++++++ 5 files changed, 97 insertions(+), 19 deletions(-) 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'