diff --git a/src/main/skills/skill-freshness-eligibility.test.ts b/src/main/skills/skill-freshness-eligibility.test.ts index e3754197559..cd826298628 100644 --- a/src/main/skills/skill-freshness-eligibility.test.ts +++ b/src/main/skills/skill-freshness-eligibility.test.ts @@ -55,14 +55,39 @@ describe('skill freshness name-scoped update eligibility', () => { ['current', 'read-only'], ['current', 'repo-scope'], ['current', 'plugin-cache'] - ] as const)('poisons a name for a %s placement in %s topology', (status, topology) => { - expect( - eligibleSkillUpdateNames([ - placement('orca-cli'), - placement('orca-cli', { id: `poison-${status}-${topology}`, status, topology }) - ]) - ).toEqual([]) - }) + ] as const)( + 'still updates the canonical copy despite a %s placement in %s topology', + (status, topology) => { + // Why: `--global` provably never writes these placements, so withholding the + // update over one refuses work the command could do to a copy that is never at + // stake. The canonical copy converges and the outlier is reported separately. + expect( + eligibleSkillUpdateNames([ + placement('orca-cli'), + placement('orca-cli', { id: `outlier-${status}-${topology}`, status, topology }) + ]) + ).toEqual(['orca-cli']) + } + ) + + it.each(['unrecognized', 'inaccessible', 'newer-known'] as const)( + 'withholds the update when the convergent copy itself is %s', + (status) => { + // Why: this is the placement the command writes to, so overwriting it is the + // real data-loss case the rail exists to avoid. + expect( + eligibleSkillUpdateNames([ + placement('orca-cli', { id: 'blocked-canonical', status }), + placement('orca-cli', { + id: 'orca-cli-claude', + rootId: 'home-claude', + topology: 'provider-alias', + status: 'outdated' + }) + ]) + ).toEqual([]) + } + ) it('still updates the canonical copy when a clean standalone duplicate exists', () => { // Why: a duplicate no longer omits the whole name — the canonical copy converges @@ -82,6 +107,26 @@ describe('skill freshness name-scoped update eligibility', () => { ).toEqual(['orca-cli']) }) + it('does not promise an update when only an unreachable duplicate is outdated', () => { + // Why: `--global` converges the canonical copy and its aliases only. Offering the + // name here advertises an update the command reports as already up to date, so the + // badge could never clear; the dialog explains the duplicate as skipped instead. + expect( + eligibleSkillUpdateNames([ + placement('orchestration', { status: 'current' }), + placement('orchestration', { + id: 'orchestration-factory', + rootId: 'home-factory', + unresolvedPath: '/home/.factory/skills/orchestration', + resolvedPath: '/home/.factory/skills/orchestration', + physicalIdentity: 'physical-orchestration-factory', + topology: 'independent-copy', + status: 'outdated' + }) + ]) + ).toEqual([]) + }) + it('does not offer a skill that exists only as a standalone copy', () => { // Why: with no canonical or alias to anchor `--global`, the command has no // reliable target, so a duplicate-only skill stays unoffered. @@ -98,7 +143,9 @@ describe('skill freshness name-scoped update eligibility', () => { ).toEqual([]) }) - it('does not offer an all-current name or let another safe name hide a poisoned one', () => { + it('scopes each name independently and leaves an all-current name alone', () => { + // Why: a project copy is never written by `--global`, so it does not speak for + // the global one — while a name whose convergent copy is current stays unoffered. expect( eligibleSkillUpdateNames([ placement('computer-use', { status: 'current' }), @@ -109,7 +156,7 @@ describe('skill freshness name-scoped update eligibility', () => { topology: 'repo-scope' }) ]) - ).toEqual([]) + ).toEqual(['orchestration']) }) it('builds only an explicit, deterministic global command', () => { diff --git a/src/main/skills/skill-freshness-eligibility.ts b/src/main/skills/skill-freshness-eligibility.ts index f4eb27a9f5a..76e21611693 100644 --- a/src/main/skills/skill-freshness-eligibility.ts +++ b/src/main/skills/skill-freshness-eligibility.ts @@ -3,6 +3,17 @@ import { type SkillFreshnessInstallation } from '../../shared/skill-freshness' +/** + * Names the global update command can actually converge. + * + * Eligibility is decided purely over the placements that command touches — the + * canonical copy and its symlink aliases. Copies it provably leaves alone (standalone + * duplicates, project skills, plugin caches, links out of tree) neither authorize an + * update nor withhold one: the badge would otherwise promise work the command cannot + * do, or refuse work it could, over a copy that is never at stake either way. A + * blocked *convergent* copy still withholds it, because that is the placement the + * command would write to and overwriting it is the real data-loss case. + */ export function eligibleSkillUpdateNames( installations: readonly SkillFreshnessInstallation[] ): string[] { @@ -14,27 +25,23 @@ export function eligibleSkillUpdateNames( } const eligible: string[] = [] - for (const [name, entries] of byName) { - const hasOutdated = entries.some((entry) => entry.status === 'outdated') - const everyPlacementIsOfficialAndUpdatable = entries.every( - (entry) => - (entry.status === 'current' || entry.status === 'outdated') && - // Why: the rail reliably converges the canonical copy and its symlink aliases. - // A standalone duplicate no longer blocks the whole name — the canonical copy - // still updates and the duplicate row is flagged as maybe-not-reached — while - // data-loss topologies (unrecognized/read-only/etc.) still poison via these checks. - (SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(entry.topology) || - entry.topology === 'independent-copy') && - Boolean(entry.resolvedPath && entry.physicalIdentity) - ) - // Why: only offer the global command when a reliably-convergent placement anchors it, - // so a skill that exists solely as a standalone copy never draws a command that could - // no-op or error against a canonical install that isn't there. - const hasReliableTarget = entries.some((entry) => + for (const [, entries] of byName) { + const convergent = entries.filter((entry) => SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(entry.topology) ) - if (hasOutdated && everyPlacementIsOfficialAndUpdatable && hasReliableTarget) { - eligible.push(name) + // Why: without a convergent placement the command has no anchor, so it would + // no-op or error against a canonical install that isn't there. + if (convergent.length === 0) { + continue + } + const hasOutdated = convergent.some((entry) => entry.status === 'outdated') + const everyConvergentCopyIsSafeToWrite = convergent.every( + (entry) => + (entry.status === 'current' || entry.status === 'outdated') && + Boolean(entry.resolvedPath && entry.physicalIdentity) + ) + if (hasOutdated && everyConvergentCopyIsSafeToWrite) { + eligible.push(entries[0].name) } } return eligible.sort((left, right) => left.localeCompare(right, 'en')) diff --git a/src/main/skills/skill-freshness-inventory.test.ts b/src/main/skills/skill-freshness-inventory.test.ts index 1570430adc3..2b010639e62 100644 --- a/src/main/skills/skill-freshness-inventory.test.ts +++ b/src/main/skills/skill-freshness-inventory.test.ts @@ -192,7 +192,7 @@ describe('read-only skill freshness inventory', () => { ) it.runIf(process.platform !== 'win32')( - 'deduplicates aliases within an unsupported topology without hiding its poison', + 'deduplicates aliases within an unsupported topology while still updating the canonical copy', async () => { const test = await fixture() await test.writeSkill(join(test.homeDir, '.agents', 'skills'), test.oldMarkdown) @@ -217,11 +217,11 @@ describe('read-only skill freshness inventory', () => { expect( inventory.installations.filter((entry) => entry.topology === 'repo-scope') ).toHaveLength(1) - expect(inventory.eligibleUpdateNames).toEqual([]) + expect(inventory.eligibleUpdateNames).toEqual(['orca-cli']) } ) - it('keeps inaccessible placements visible and lets them poison the name', async () => { + it('keeps an unreadable foreign-home placement visible without withholding the update', async () => { const test = await fixture() await test.writeSkill(join(test.homeDir, '.agents', 'skills'), test.oldMarkdown) const inaccessiblePath = join(test.homeDir, '.codex', 'skills', 'orca-cli') @@ -243,7 +243,9 @@ describe('read-only skill freshness inventory', () => { 'outdated', 'inaccessible' ]) - expect(inventory.eligibleUpdateNames).toEqual([]) + // Why: `--global` never writes another agent's home, so an unreadable copy there + // cannot be harmed by the update and must not withhold it from the canonical copy. + expect(inventory.eligibleUpdateNames).toEqual(['orca-cli']) }) it('does not lose an inaccessible known repository placement', async () => { @@ -274,14 +276,14 @@ describe('read-only skill freshness inventory', () => { }) ]) ) - expect(inventory.eligibleUpdateNames).toEqual([]) + expect(inventory.eligibleUpdateNames).toEqual(['orca-cli']) }) it.each([ ['repo', 'repo-scope'], ['plugin', 'plugin-cache'] ] as const)( - 'keeps an official %s placement informational and name-poisoning', + 'keeps an official %s placement informational without withholding the update', async (kind, topology) => { const test = await fixture() await test.writeSkill(join(test.homeDir, '.agents', 'skills'), test.oldMarkdown) @@ -305,7 +307,7 @@ describe('read-only skill freshness inventory', () => { }) expect(inventory.installations.some((entry) => entry.topology === topology)).toBe(true) - expect(inventory.eligibleUpdateNames).toEqual([]) + expect(inventory.eligibleUpdateNames).toEqual(['orca-cli']) } ) @@ -351,7 +353,7 @@ describe('read-only skill freshness inventory', () => { }) }) - it('withholds updates when stored repositories exceed the probe budget', async () => { + it('reports the repository scan limit without withholding the global update', async () => { const test = await fixture() await test.writeSkill(join(test.homeDir, '.agents', 'skills'), test.oldMarkdown) const repos = Array.from( @@ -371,6 +373,8 @@ describe('read-only skill freshness inventory', () => { expect.objectContaining({ errorCategory: 'repository-scan-limit', status: 'inaccessible' }) ]) ) - expect(inventory.eligibleUpdateNames).toEqual([]) + // Why: unscanned repositories only ever hold project skills, which the global + // command does not touch, so the limit is reported without blocking the update. + expect(inventory.eligibleUpdateNames).toEqual(['orca-cli']) }) }) diff --git a/src/renderer/src/components/feature-wall/BrowserUseSkillSetupCard.tsx b/src/renderer/src/components/feature-wall/BrowserUseSkillSetupCard.tsx index b8b1cd58306..87e733f76d0 100644 --- a/src/renderer/src/components/feature-wall/BrowserUseSkillSetupCard.tsx +++ b/src/renderer/src/components/feature-wall/BrowserUseSkillSetupCard.tsx @@ -1,6 +1,7 @@ import type { JSX } from 'react' import { ORCA_CLI_SKILL_INSTALL_COMMAND, + ORCA_CLI_SKILL_NAME, ORCA_CLI_SKILL_UPDATE_COMMAND } from '@/lib/agent-feature-install-commands' import { @@ -74,6 +75,11 @@ export function BrowserUseSkillSetupCard(props: { onBeforeOpenTerminal={handleBeforeOpenTerminal} showRecheckWhenInstalled={false} onRecheck={skill.refresh} + // Why: the local-host-only freshness scan cannot vouch for a WSL runtime, + // so fall back to the presence-only pill there (mirrors the settings cards). + freshnessSkillName={ + activeSkillRuntime.agentRuntime?.runtime === 'wsl' ? undefined : ORCA_CLI_SKILL_NAME + } /> ) diff --git a/src/renderer/src/components/floating-terminal/FloatingTerminalOrchestrationDialog.tsx b/src/renderer/src/components/floating-terminal/FloatingTerminalOrchestrationDialog.tsx index abfc314d685..6f9aed3f2e8 100644 --- a/src/renderer/src/components/floating-terminal/FloatingTerminalOrchestrationDialog.tsx +++ b/src/renderer/src/components/floating-terminal/FloatingTerminalOrchestrationDialog.tsx @@ -8,6 +8,7 @@ import { } from '@/components/ui/dialog' import { AgentSkillSetupPanel } from '@/components/settings/AgentSkillSetupPanel' import { IntegrationStatusPill } from '@/components/integration-status-pill' +import { SkillFreshnessStatusPill } from '@/components/skills/SkillFreshnessStatusPill' import { ORCHESTRATION_SKILL_NAME } from '@/lib/agent-feature-install-commands' import { AGENT_SKILL_CLI_PREREQUISITE_NOTICE, @@ -94,12 +95,19 @@ export function FloatingTerminalOrchestrationDialog({ )} ) : orchestrationSkillDetected ? ( - - {translate( - 'auto.components.floating.terminal.FloatingTerminalOrchestrationDialog.630c0ac8c8', - 'Installed' - )} - + // Why: the modal owns the status pill, so it must carry the same + // freshness signal — and route to the same review dialog — as the + // settings card for this skill; WSL falls back to presence-only. + activeSkillRuntime.agentRuntime?.runtime === 'wsl' ? ( + + {translate( + 'auto.components.floating.terminal.FloatingTerminalOrchestrationDialog.630c0ac8c8', + 'Installed' + )} + + ) : ( + + ) ) : ( {translate( diff --git a/src/renderer/src/components/settings/MobileEmulatorAgentControlRow.tsx b/src/renderer/src/components/settings/MobileEmulatorAgentControlRow.tsx index 17946134bc1..ec1403f6ac7 100644 --- a/src/renderer/src/components/settings/MobileEmulatorAgentControlRow.tsx +++ b/src/renderer/src/components/settings/MobileEmulatorAgentControlRow.tsx @@ -1,6 +1,7 @@ import { Import, Loader2 } from 'lucide-react' import { ORCA_CLI_SKILL_INSTALL_COMMAND, + ORCA_CLI_SKILL_NAME, ORCA_CLI_SKILL_UPDATE_COMMAND } from '@/lib/agent-feature-install-commands' import { @@ -171,6 +172,9 @@ export function MobileEmulatorAgentControlRow(): React.JSX.Element { await ensureOrcaCliAvailableForAgentSkillTerminal() }} onRecheck={setup.refreshCliSkill} + // Why: this row builds its commands for the local host only, so the + // local-host freshness scan can vouch for the copy it points at. + freshnessSkillName={ORCA_CLI_SKILL_NAME} /> diff --git a/src/renderer/src/components/settings/SettingsSidebar.tsx b/src/renderer/src/components/settings/SettingsSidebar.tsx index 54908713bc6..d9a0093f0b8 100644 --- a/src/renderer/src/components/settings/SettingsSidebar.tsx +++ b/src/renderer/src/components/settings/SettingsSidebar.tsx @@ -162,6 +162,11 @@ export function SettingsSidebar({ 'auto.components.skills.SkillFreshnessStatusPill.updateAvailable', 'Update available' ) + case 'needs-attention': + return translate( + 'auto.components.skills.SkillFreshnessStatusPill.needsAttention', + 'Needs attention' + ) case 'checking': return translate('auto.components.settings.AgentSkillSetupPanel.68a468752e', 'Checking...') } @@ -171,7 +176,7 @@ export function SettingsSidebar({ 'ml-auto shrink-0 rounded-full border px-1.5 py-0.5 text-[10px] font-medium leading-none', status === 'installed' || status === 'up-to-date' ? 'border-status-success-border bg-status-success-background text-status-success' - : status === 'update-available' + : status === 'update-available' || status === 'needs-attention' ? 'border-amber-500/40 bg-amber-500/10 text-amber-700 dark:text-amber-300' : status === 'install' ? 'border-foreground/15 bg-foreground/10 text-foreground' diff --git a/src/renderer/src/components/skills/SkillFreshnessStatusPill.test.tsx b/src/renderer/src/components/skills/SkillFreshnessStatusPill.test.tsx index 785b67aaf07..e3b1d1e018b 100644 --- a/src/renderer/src/components/skills/SkillFreshnessStatusPill.test.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessStatusPill.test.tsx @@ -5,6 +5,7 @@ import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { SkillFreshnessInventory } from '../../../../shared/skill-freshness' import { SkillFreshnessStatusPill } from './SkillFreshnessStatusPill' +import { consumeSkillFreshnessUpdateDialogRequest } from './skill-freshness-update-dialog' const mocks = vi.hoisted(() => ({ inventory: null as SkillFreshnessInventory | null @@ -19,6 +20,14 @@ vi.mock('@/hooks/useSkillFreshness', () => ({ }) })) +function detailsButton(container: HTMLDivElement): HTMLButtonElement | null { + return container.querySelector('[data-slot="button"]') +} + +function pillText(container: HTMLDivElement): string { + return (container.textContent ?? '').replace(detailsButton(container)?.textContent ?? '', '') +} + function inventory( entries: { name: string; status: 'current' | 'outdated' | 'unrecognized' }[], eligibleUpdateNames: string[] @@ -66,6 +75,8 @@ async function renderPill(skillName: string): Promise { describe('SkillFreshnessStatusPill', () => { beforeEach(() => { mocks.inventory = null + // Why: the dialog request is module-level state shared across tests. + consumeSkillFreshnessUpdateDialogRequest() }) afterEach(async () => { @@ -80,16 +91,21 @@ describe('SkillFreshnessStatusPill', () => { it('shows Update available for an eligible outdated skill', async () => { mocks.inventory = inventory([{ name: 'orca-cli', status: 'outdated' }], ['orca-cli']) - expect((await renderPill('orca-cli')).textContent).toBe('Update available') + const rendered = await renderPill('orca-cli') + expect(pillText(rendered)).toBe('Update available') + expect(detailsButton(rendered)?.textContent).toBe('Details') }) it('shows Up to date when every placement is current', async () => { mocks.inventory = inventory([{ name: 'orca-cli', status: 'current' }], []) - expect((await renderPill('orca-cli')).textContent).toBe('Up to date') + const rendered = await renderPill('orca-cli') + expect(pillText(rendered)).toBe('Up to date') + // Why: nothing is out of date, so the review dialog would have no row to show. + expect(detailsButton(rendered)).toBeNull() }) - it('falls back to Installed for a blocked outdated placement', async () => { + it('flags a blocked outdated placement instead of reading as all-clear', async () => { mocks.inventory = inventory( [ { name: 'orca-cli', status: 'outdated' }, @@ -98,10 +114,26 @@ describe('SkillFreshnessStatusPill', () => { [] ) - expect((await renderPill('orca-cli')).textContent).toBe('Installed') + const rendered = await renderPill('orca-cli') + // Why: a green pill over a copy the update cannot reach hides real drift. + expect(pillText(rendered)).toBe('Needs attention') + expect(detailsButton(rendered)?.textContent).toBe('Details') }) it('falls back to Installed before the inventory loads', async () => { - expect((await renderPill('orca-cli')).textContent).toBe('Installed') + const rendered = await renderPill('orca-cli') + expect(pillText(rendered)).toBe('Installed') + expect(detailsButton(rendered)).toBeNull() + }) + + it('opens the freshness review dialog from Details', async () => { + mocks.inventory = inventory([{ name: 'orca-cli', status: 'outdated' }], ['orca-cli']) + const rendered = await renderPill('orca-cli') + + await act(async () => { + detailsButton(rendered)?.click() + }) + + expect(consumeSkillFreshnessUpdateDialogRequest()).toBe(true) }) }) diff --git a/src/renderer/src/components/skills/SkillFreshnessStatusPill.tsx b/src/renderer/src/components/skills/SkillFreshnessStatusPill.tsx index d71bf0d8ba7..6329622a1e6 100644 --- a/src/renderer/src/components/skills/SkillFreshnessStatusPill.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessStatusPill.tsx @@ -1,15 +1,17 @@ import { useSkillFreshness } from '@/hooks/useSkillFreshness' import { translate } from '@/i18n/i18n' +import { Button } from '@/components/ui/button' import { IntegrationStatusPill } from '@/components/integration-status-pill' -import { getSkillFreshnessDisplayStatus } from '@/lib/skill-freshness-display-status' +import { cn } from '@/lib/utils' +import { AlertTriangle, ChevronRight } from 'lucide-react' +import { + getSkillFreshnessDisplayStatus, + hasSkillCopyNeedingAttention, + type SkillFreshnessDisplayStatus +} from '@/lib/skill-freshness-display-status' +import { requestSkillFreshnessUpdateDialog } from './skill-freshness-update-dialog' -// Why: the setup rails' Installed pill is presence-only; when freshness knows a -// safe update exists (or that every copy is current) the pill should say so. -// Falls back to plain Installed for blocked/unrecognized copies so an unsafe -// placement is never advertised as updatable here. -export function SkillFreshnessStatusPill({ skillName }: { skillName: string }): React.JSX.Element { - const { inventory } = useSkillFreshness() - const status = getSkillFreshnessDisplayStatus(inventory, skillName) +function statusPill(status: SkillFreshnessDisplayStatus): React.JSX.Element { if (status === 'update-available') { return ( @@ -20,6 +22,16 @@ export function SkillFreshnessStatusPill({ skillName }: { skillName: string }): ) } + if (status === 'needs-attention') { + return ( + + {translate( + 'auto.components.skills.SkillFreshnessStatusPill.needsAttention', + 'Needs attention' + )} + + ) + } if (status === 'up-to-date') { return ( @@ -33,3 +45,46 @@ export function SkillFreshnessStatusPill({ skillName }: { skillName: string }): ) } + +// Why: the setup rails' Installed pill is presence-only. Freshness knows more — that +// a safe update exists, that every copy is current, or that a copy is out of date +// somewhere the update cannot reach — and green must never stand in for that last +// case, which is real drift the user would otherwise have no way to see. +export function SkillFreshnessStatusPill({ skillName }: { skillName: string }): React.JSX.Element { + const { inventory } = useSkillFreshness() + const status = getSkillFreshnessDisplayStatus(inventory, skillName) + // Why: the dialog lists every placement, so Details is offered whenever a placement + // is what drove the status — an available update, or a copy that blocked one. + const hasDetails = status === 'update-available' || status === 'needs-attention' + // Why: the badge alone can't say which copies are wrong, and the reasons only read + // correctly beside the locations they describe. Marking the way in is enough here — + // the dialog does the explaining, with every location and cause it knows about. + const needsAttention = hasSkillCopyNeedingAttention(inventory, skillName) + return ( + + {statusPill(status)} + {hasDetails ? ( + + ) : null} + + ) +} diff --git a/src/renderer/src/components/skills/skill-freshness-group.tsx b/src/renderer/src/components/skills/skill-freshness-group.tsx index 514acad743c..977af348292 100644 --- a/src/renderer/src/components/skills/skill-freshness-group.tsx +++ b/src/renderer/src/components/skills/skill-freshness-group.tsx @@ -1,11 +1,8 @@ -import type { - SkillFreshnessGroupModel, - SkillLocationChip, - SkillLocationRow -} from './skill-freshness-grouping' +import type { SkillFreshnessGroupModel, SkillLocationChip } from './skill-freshness-grouping' import { translate } from '@/i18n/i18n' import { Badge } from '@/components/ui/badge' import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' +import { skippedReason } from './skill-freshness-skipped-reason' function chipLabel(chip: SkillLocationChip): string { switch (chip) { @@ -82,70 +79,6 @@ function chipTooltip(chip: SkillLocationChip): string { } } -// Why: a skill is skipped for one concrete reason; lead with the highest-priority -// blocking placement so the sentence explains the real cause (an edited copy is -// more useful to surface than a downstream symptom). -const SKIPPED_REASON_PRIORITY: SkillLocationChip[] = [ - 'unrecognized', - 'read-only', - 'inaccessible', - 'in-a-repo', - 'plugin-cache', - 'external-link', - 'broken-link' -] - -function skippedReason(locations: readonly SkillLocationRow[]): string { - const present = new Set(locations.map((location) => location.chip)) - const chip = SKIPPED_REASON_PRIORITY.find((candidate) => present.has(candidate)) - switch (chip) { - case 'unrecognized': - return translate( - 'auto.components.skills.SkillFreshnessRow.skippedReasonUnrecognized', - 'The copy here doesn’t match the official version — it may be modified, or a different skill with the same name. Orca left it out of the update so it won’t overwrite it. Remove it if you want Orca to update this skill.' - ) - case 'read-only': - return translate( - 'auto.components.skills.SkillFreshnessRow.skippedReasonReadOnly', - 'This copy is in a read-only location, so Orca left it out of the update. Change its permissions to let Orca update it.' - ) - case 'inaccessible': - return translate( - 'auto.components.skills.SkillFreshnessRow.skippedReasonInaccessible', - 'Orca couldn’t read this copy, so it left the skill out of the update.' - ) - case 'in-a-repo': - return translate( - 'auto.components.skills.SkillFreshnessRow.skippedReasonInRepo', - 'This is a project skill, not a global one — Orca only updates your global skills, so it left this out of the update.' - ) - case 'plugin-cache': - return translate( - 'auto.components.skills.SkillFreshnessRow.skippedReasonPluginCache', - 'A plugin manages this skill, so Orca left it out of the update — update the plugin instead.' - ) - case 'external-link': - return translate( - 'auto.components.skills.SkillFreshnessRow.skippedReasonExternalLink', - 'This copy is a shortcut pointing outside Orca’s skill folders, so Orca left it out of the update.' - ) - case 'broken-link': - return translate( - 'auto.components.skills.SkillFreshnessRow.skippedReasonBrokenLink', - 'This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it.' - ) - // Why: 'current'/'duplicate' are non-blocking chips, and an empty priority - // list is possible; all fall through to the generic skipped message. - case 'current': - case 'duplicate': - case undefined: - return translate( - 'auto.components.skills.SkillFreshnessRow.cantUpdateReason', - 'Orca left this skill out of the update command.' - ) - } -} - export function SkillFreshnessGroup({ group }: { 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 03368ec07bf..3a89ae1c220 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.test.ts @@ -73,6 +73,28 @@ describe('groupSkillFreshness', () => { ]) }) + it('blocks a skill whose only outdated copy is an unreachable duplicate', () => { + // Why: the global command reports "already up to date" here, so the group must + // read as skipped with the duplicate flagged rather than promising an update. + const groups = groupSkillFreshness( + [ + placement('orchestration', { status: 'current' }), + placement('orchestration', { + rootId: 'home-factory', + unresolvedPath: '/home/.factory/skills/orchestration', + topology: 'independent-copy' + }) + ], + [] + ) + expect(groups).toHaveLength(1) + expect(groups[0]?.status).toBe('cannot-update') + expect(groups[0]?.locations).toEqual([ + { id: expect.any(String), path: '/home/.agents/skills/orchestration', chip: 'current' }, + { id: expect.any(String), path: '/home/.factory/skills/orchestration', chip: 'duplicate' } + ]) + }) + it('prefers a location status over its topology and maps every topology to a chip', () => { const chipFor = (overrides: Partial): string | null => groupSkillFreshness( diff --git a/src/renderer/src/components/skills/skill-freshness-grouping.ts b/src/renderer/src/components/skills/skill-freshness-grouping.ts index db76800fdfb..b33dd1f49fe 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.ts @@ -25,7 +25,7 @@ export type SkillFreshnessGroupModel = { locations: SkillLocationRow[] } -function locationChip(installation: SkillFreshnessInstallation): SkillLocationChip | null { +export function locationChip(installation: SkillFreshnessInstallation): SkillLocationChip | null { // Why: a location's own status wins over its topology — "the contents don't // match" is more useful to the user than "it's a duplicate". if (installation.status === 'unrecognized') { diff --git a/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts b/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts new file mode 100644 index 00000000000..16ef8cb0876 --- /dev/null +++ b/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, it } from 'vitest' +import type { SkillLocationRow } from './skill-freshness-grouping' +import { skippedReason } from './skill-freshness-skipped-reason' + +function row( + chip: SkillLocationRow['chip'], + path = `/home/.agents/skills/${chip}` +): SkillLocationRow { + return { id: `row-${chip}-${path}`, path, chip } +} + +describe('skippedReason', () => { + it('names the stale duplicate the global command cannot reach', () => { + const reason = skippedReason([row('current'), row('duplicate')]) + expect(reason).toContain('separate copy') + expect(reason).toContain('only refreshes the main copy') + }) + + it('leads with the harder blocker when several placements are off', () => { + // Why: an edited copy is the real cause; the duplicate is the lesser symptom, and + // telling the user to remove the duplicate would not unblock the update. + expect(skippedReason([row('duplicate'), row('unrecognized')])).toContain( + 'doesn’t match the official version' + ) + expect(skippedReason([row('duplicate'), row('read-only')])).toContain('read-only location') + }) + + it('falls back to the generic sentence when nothing is blocking', () => { + expect(skippedReason([row('current')])).toContain('left this skill out of the update') + expect(skippedReason([])).toContain('left this skill out of the update') + }) +}) diff --git a/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts b/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts new file mode 100644 index 00000000000..2d693b6916b --- /dev/null +++ b/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts @@ -0,0 +1,85 @@ +import type { SkillLocationChip, SkillLocationRow } from './skill-freshness-grouping' +import { translate } from '@/i18n/i18n' + +// Why: a skill is skipped for one concrete reason; lead with the highest-priority +// blocking placement so the sentence explains the real cause (an edited copy is +// more useful to surface than a downstream symptom). +const SKIPPED_REASON_PRIORITY: SkillLocationChip[] = [ + 'unrecognized', + 'read-only', + 'inaccessible', + 'in-a-repo', + 'plugin-cache', + 'external-link', + 'broken-link', + // Why: lowest priority — a stale duplicate only explains the skip once no + // harder blocker is present, since the others describe a more specific cause. + 'duplicate' +] + +function blockingChip(locations: readonly SkillLocationRow[]): SkillLocationChip | undefined { + const present = new Set(locations.map((location) => location.chip)) + return SKIPPED_REASON_PRIORITY.find((candidate) => present.has(candidate)) +} + +/** + * The one sentence that explains why an update won't reach a skill. Shared by the + * review dialog and the setup rails so the badge and the dialog can never disagree. + * + * The wording is deictic ("this copy") on purpose: it is only ever rendered beside the + * location rows it describes, which is why the setup rails link into the dialog rather + * than repeating a sentence that would have nothing to point at. + */ +export function skippedReason(locations: readonly SkillLocationRow[]): string { + const chip = blockingChip(locations) + switch (chip) { + case 'unrecognized': + return translate( + 'auto.components.skills.SkillFreshnessRow.skippedReasonUnrecognized', + 'The copy here doesn’t match the official version — it may be modified, or a different skill with the same name. Orca left it out of the update so it won’t overwrite it. Remove it if you want Orca to update this skill.' + ) + case 'read-only': + return translate( + 'auto.components.skills.SkillFreshnessRow.skippedReasonReadOnly', + 'This copy is in a read-only location, so Orca left it out of the update. Change its permissions to let Orca update it.' + ) + case 'inaccessible': + return translate( + 'auto.components.skills.SkillFreshnessRow.skippedReasonInaccessible', + 'Orca couldn’t read this copy, so it left the skill out of the update.' + ) + case 'in-a-repo': + return translate( + 'auto.components.skills.SkillFreshnessRow.skippedReasonInRepo', + 'This is a project skill, not a global one — Orca only updates your global skills, so it left this out of the update.' + ) + case 'plugin-cache': + return translate( + 'auto.components.skills.SkillFreshnessRow.skippedReasonPluginCache', + 'A plugin manages this skill, so Orca left it out of the update — update the plugin instead.' + ) + case 'external-link': + return translate( + 'auto.components.skills.SkillFreshnessRow.skippedReasonExternalLink', + 'This copy is a shortcut pointing outside Orca’s skill folders, so Orca left it out of the update.' + ) + case 'broken-link': + return translate( + 'auto.components.skills.SkillFreshnessRow.skippedReasonBrokenLink', + 'This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it.' + ) + case 'duplicate': + return translate( + 'auto.components.skills.SkillFreshnessRow.skippedReasonDuplicate', + 'This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one.' + ) + // Why: 'current' is non-blocking and an empty priority list is possible; + // both fall through to the generic skipped message. + case 'current': + case undefined: + return translate( + 'auto.components.skills.SkillFreshnessRow.cantUpdateReason', + 'Orca left this skill out of the update command.' + ) + } +} diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index e32011ff669..0e13ddeab69 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -3748,7 +3748,8 @@ "tipBrokenLink": "A shortcut to something that no longer exists.", "tipReadOnly": "This copy is in a read-only location.", "tipInRepo": "This copy lives inside a project, not your global skills.", - "tipPluginCache": "This copy is managed by a plugin." + "tipPluginCache": "This copy is managed by a plugin.", + "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one." }, "SkillFreshnessUpdateDialog": { "title": "Update skills", @@ -3769,7 +3770,9 @@ "SkillFreshnessStatusPill": { "updateAvailable": "Update available", "upToDate": "Up to date", - "installed": "Installed" + "installed": "Installed", + "details": "Details", + "needsAttention": "Needs attention" } }, "sidebar": { diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index f698b2ab172..a6f65f6df48 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -3725,7 +3725,8 @@ "skippedReasonInRepo": "This is a project skill, not a global one — Orca only updates your global skills, so it left this out of the update.", "skippedReasonPluginCache": "A plugin manages this skill, so Orca left it out of the update — update the plugin instead.", "skippedReasonExternalLink": "This copy is a shortcut pointing outside Orca’s skill folders, so Orca left it out of the update.", - "skippedReasonBrokenLink": "This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it." + "skippedReasonBrokenLink": "This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it.", + "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one." }, "SkillFreshnessUpdateDialog": { "title": "Actualizar skills", @@ -3746,7 +3747,9 @@ "SkillFreshnessStatusPill": { "updateAvailable": "Actualización disponible", "upToDate": "Actualizado", - "installed": "Instalado" + "installed": "Instalado", + "details": "Details", + "needsAttention": "Needs attention" } }, "sidebar": { diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 5ffc4a375f4..89e694716c5 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -3725,7 +3725,8 @@ "skippedReasonInRepo": "This is a project skill, not a global one — Orca only updates your global skills, so it left this out of the update.", "skippedReasonPluginCache": "A plugin manages this skill, so Orca left it out of the update — update the plugin instead.", "skippedReasonExternalLink": "This copy is a shortcut pointing outside Orca’s skill folders, so Orca left it out of the update.", - "skippedReasonBrokenLink": "This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it." + "skippedReasonBrokenLink": "This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it.", + "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one." }, "SkillFreshnessUpdateDialog": { "title": "スキルを更新", @@ -3746,7 +3747,9 @@ "SkillFreshnessStatusPill": { "updateAvailable": "更新があります", "upToDate": "最新です", - "installed": "インストール済み" + "installed": "インストール済み", + "details": "Details", + "needsAttention": "Needs attention" } }, "sidebar": { diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 14d8164070d..16a465d66de 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -3725,7 +3725,8 @@ "skippedReasonInRepo": "This is a project skill, not a global one — Orca only updates your global skills, so it left this out of the update.", "skippedReasonPluginCache": "A plugin manages this skill, so Orca left it out of the update — update the plugin instead.", "skippedReasonExternalLink": "This copy is a shortcut pointing outside Orca’s skill folders, so Orca left it out of the update.", - "skippedReasonBrokenLink": "This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it." + "skippedReasonBrokenLink": "This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it.", + "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one." }, "SkillFreshnessUpdateDialog": { "title": "스킬 업데이트", @@ -3746,7 +3747,9 @@ "SkillFreshnessStatusPill": { "updateAvailable": "업데이트 가능", "upToDate": "최신 상태", - "installed": "설치됨" + "installed": "설치됨", + "details": "Details", + "needsAttention": "Needs attention" } }, "sidebar": { diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index bca48c084d9..7b33271bca3 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -3725,7 +3725,8 @@ "skippedReasonInRepo": "This is a project skill, not a global one — Orca only updates your global skills, so it left this out of the update.", "skippedReasonPluginCache": "A plugin manages this skill, so Orca left it out of the update — update the plugin instead.", "skippedReasonExternalLink": "This copy is a shortcut pointing outside Orca’s skill folders, so Orca left it out of the update.", - "skippedReasonBrokenLink": "This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it." + "skippedReasonBrokenLink": "This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it.", + "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one." }, "SkillFreshnessUpdateDialog": { "title": "更新技能", @@ -3746,7 +3747,9 @@ "SkillFreshnessStatusPill": { "updateAvailable": "有可用更新", "upToDate": "已是最新", - "installed": "已安装" + "installed": "已安装", + "details": "Details", + "needsAttention": "Needs attention" } }, "sidebar": { diff --git a/src/renderer/src/lib/settings-navigation-types.ts b/src/renderer/src/lib/settings-navigation-types.ts index f3409e69000..7ce835e6938 100644 --- a/src/renderer/src/lib/settings-navigation-types.ts +++ b/src/renderer/src/lib/settings-navigation-types.ts @@ -8,6 +8,7 @@ export type SettingsNavInstallStatus = | 'installed' | 'up-to-date' | 'update-available' + | 'needs-attention' | 'checking' export type SettingsNavTarget = 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 9d244007d0e..15b25aa4a92 100644 --- a/src/renderer/src/lib/skill-freshness-display-status.test.ts +++ b/src/renderer/src/lib/skill-freshness-display-status.test.ts @@ -56,14 +56,23 @@ describe('getSkillFreshnessDisplayStatus', () => { it.each([ ['before the inventory loads', null], - ['when the inventory has no matching placement', inventory([])], + ['when the inventory has no matching placement', inventory([])] + ])('reports presence only %s', (_scenario, value) => { + // Why: with nothing scanned there is no drift to claim, and flashing attention + // on every launch before the first scan would train the user to ignore it. + expect(getSkillFreshnessDisplayStatus(value, SKILL_NAME)).toBe('installed') + }) + + it.each([ [ 'when any placement is unrecognized', inventory([placement('current'), placement('unrecognized', 1)]) ], ['when a placement is inaccessible', inventory([placement('inaccessible')])], ['when an outdated placement is not eligible', inventory([placement('outdated')])] - ])('falls back to installed %s', (_scenario, value) => { - expect(getSkillFreshnessDisplayStatus(value, SKILL_NAME)).toBe('installed') + ])('reports needs attention %s', (_scenario, value) => { + // Why: no eligible update is not proof a copy is fine. Green here would read as + // all-clear over drift the update command cannot reach and the user cannot see. + expect(getSkillFreshnessDisplayStatus(value, SKILL_NAME)).toBe('needs-attention') }) }) diff --git a/src/renderer/src/lib/skill-freshness-display-status.ts b/src/renderer/src/lib/skill-freshness-display-status.ts index 25264022659..581c281a8e8 100644 --- a/src/renderer/src/lib/skill-freshness-display-status.ts +++ b/src/renderer/src/lib/skill-freshness-display-status.ts @@ -1,6 +1,13 @@ -import type { SkillFreshnessInventory } from '../../../shared/skill-freshness' +import { + SUPPORTED_GLOBAL_SKILL_TOPOLOGIES, + type SkillFreshnessInventory +} from '../../../shared/skill-freshness' -export type SkillFreshnessDisplayStatus = 'installed' | 'up-to-date' | 'update-available' +export type SkillFreshnessDisplayStatus = + | 'installed' + | 'up-to-date' + | 'update-available' + | 'needs-attention' export function getSkillFreshnessDisplayStatus( inventory: SkillFreshnessInventory | null, @@ -11,15 +18,44 @@ export function getSkillFreshnessDisplayStatus( } let hasPlacement = false + let hasBlockedCopy = false for (const installation of inventory?.installations ?? []) { if (installation.name !== skillName) { continue } hasPlacement = true - // No eligible update is not proof that a blocked or unrecognized copy is current. if (installation.status !== 'current') { - return 'installed' + hasBlockedCopy = true } } - return hasPlacement ? 'up-to-date' : 'installed' + // Why: with no scan yet (or nothing found) the only honest answer is presence. + // Reporting attention here would flash amber on every launch before the first scan. + if (!hasPlacement) { + return 'installed' + } + // Why: no eligible update is not proof a copy is fine — it can equally mean a copy + // is out of date somewhere the update command cannot reach. Saying "Installed" there + // reads as all-clear and hides real drift, so that case gets its own attention state. + return hasBlockedCopy ? 'needs-attention' : 'up-to-date' +} + +/** + * Whether a copy needs the user's own hands — it is not current, and running the update + * would not resolve it. This is what marks the review affordance as carrying a problem + * rather than a routine update, so the badge can stay a badge and the dialog explains. + */ +export function hasSkillCopyNeedingAttention( + inventory: SkillFreshnessInventory | null, + skillName: string +): boolean { + return (inventory?.installations ?? []).some( + (installation) => + installation.name === skillName && + installation.status !== 'current' && + // 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' + ) + ) }