From 3d54f2174f3d2b6a65444838c8e2eac2071eadb0 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:57:51 -0700 Subject: [PATCH] fix(skills): tell the user how to fix a skill the updater cannot converge (#11248) Re-lands #11129, which was merged into #11128's branch rather than main and so never reached main. Content is identical to the reviewed and live-QA'd head ac5ec5b0b0 (1775d83cf6 + ac5ec5b0b0, minus the intermediate merge). --- .../SkillFreshnessUpdateDialog.test.tsx | 19 ++++++++++ .../src/components/skills/SkillUpdateRow.tsx | 2 +- .../skills/skill-freshness-grouping.test.ts | 3 ++ .../skills/skill-freshness-grouping.ts | 12 +++++-- .../skill-freshness-skipped-reason.test.ts | 26 +++++++++++++- .../skills/skill-freshness-skipped-reason.ts | 35 ++++++++++++++++--- .../skills/skill-location-chip-copy.ts | 7 ++++ src/renderer/src/i18n/locales/en.json | 6 +++- src/renderer/src/i18n/locales/es.json | 6 +++- src/renderer/src/i18n/locales/ja.json | 6 +++- src/renderer/src/i18n/locales/ko.json | 6 +++- src/renderer/src/i18n/locales/zh.json | 6 +++- 12 files changed, 119 insertions(+), 15 deletions(-) diff --git a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx index 0733df60470..7c00fb26d53 100644 --- a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx @@ -423,6 +423,25 @@ describe('SkillFreshnessUpdateDialog', () => { ) }) + it('names the skill in the stale-record remedy so the command is runnable as-is', async () => { + // Why: the reinstall advice only fires when the row hands its group name through + // to skippedReason — dropping that argument silently degrades every stale-record + // row to the generic sentence with no command to run. + mocks.inventory = { + schemaVersion: 1, + installations: [placement('orchestration')], + eligibleUpdateNames: [], + scanIssues: [], + scannedAt: 3 + } + await renderDialog() + await openViaRequest() + + expect(container?.textContent).toContain( + 'npx skills add https://github.com/stablyai/orca --skill orchestration --global' + ) + }) + it('stays coherent while an idle re-scan is in flight', async () => { await renderDialog() await openViaRequest() diff --git a/src/renderer/src/components/skills/SkillUpdateRow.tsx b/src/renderer/src/components/skills/SkillUpdateRow.tsx index 3f069be5b54..d6d95f34783 100644 --- a/src/renderer/src/components/skills/SkillUpdateRow.tsx +++ b/src/renderer/src/components/skills/SkillUpdateRow.tsx @@ -118,7 +118,7 @@ export function SkillUpdateRow({ nor on a mount-time `defaultOpen` that a re-scan can't re-fire. */} {state === 'blocked' ? (

- {skippedReason(group.locations)} + {skippedReason(group.locations, group.name)}

) : null} 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 c4913f678da..39bb58cc4cb 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.test.ts @@ -164,6 +164,9 @@ describe('groupSkillFreshness', () => { expect(chipFor(at('g', { topology: 'repo-scope' }))).toBe('in-a-repo') expect(chipFor(at('h', { topology: 'plugin-cache' }))).toBe('plugin-cache') expect(chipFor(at('i', { status: 'current', topology: 'provider-alias' }))).toBe('current') + // Why: a bare chip is what the reason copy reads as "behind, and a reinstall fixes + // it". A copy that is ahead must be told apart, or that advice rolls it back. + expect(chipFor(at('k', { status: 'newer-known', topology: 'canonical-copy' }))).toBe('newer') expect(chipFor(at('j', { status: 'unrecognized', topology: 'plugin-cache' }))).toBe( 'plugin-cache' ) diff --git a/src/renderer/src/components/skills/skill-freshness-grouping.ts b/src/renderer/src/components/skills/skill-freshness-grouping.ts index 2a306b44c72..79d39b31aff 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.ts @@ -7,6 +7,7 @@ export type SkillGroupStatus = 'update-available' | 'cannot-update' export type SkillLocationChip = | 'current' + | 'newer' | 'unrecognized' | 'inaccessible' | 'duplicate' @@ -53,9 +54,14 @@ export function locationChip(installation: SkillFreshnessInstallation): SkillLoc return 'plugin-cache' case 'canonical-copy': case 'provider-alias': - // Why: a supported location only needs a chip when it's already up to date, - // to explain why the update won't touch it; the out-of-date main copy is bare. - return installation.status === 'current' ? 'current' : null + // Why: a supported location only needs a chip when the update won't touch it — + // already current, or ahead of what this build knows. The out-of-date main copy + // is bare, and 'newer' is what keeps a bare chip meaning "behind, and fixable": + // reinstalling a copy that is ahead would quietly roll the user back. + if (installation.status === 'current') { + return 'current' + } + return installation.status === 'newer-known' ? 'newer' : null } } 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 index 16ef8cb0876..664a4883f84 100644 --- a/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts +++ b/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts @@ -25,7 +25,31 @@ describe('skippedReason', () => { expect(skippedReason([row('duplicate'), row('read-only')])).toContain('read-only location') }) - it('falls back to the generic sentence when nothing is blocking', () => { + it('hands over the reinstall command when no location is at fault', () => { + // Why: the only way to reach this with a bare out-of-date copy is the updater's own + // record, which `skills update` can never converge — so the sentence has to give the + // one command that does, not report a skip the user cannot act on. + const reason = skippedReason([row(null)], 'orchestration') + expect(reason).toContain('reports the skill as already up to date') + expect(reason).toContain( + 'npx skills add https://github.com/stablyai/orca --skill orchestration --global' + ) + }) + + it('never offers the reinstall for a copy that is ahead of this build', () => { + // Why: reinstalling a newer copy rolls the user back to what this build ships. + const reason = skippedReason([row('newer')], 'orchestration') + expect(reason).toContain('later version') + expect(reason).not.toContain('skills add') + }) + + it('keeps a placement blocker ahead of the record advice', () => { + expect(skippedReason([row(null), row('unrecognized')], 'orchestration')).toContain( + 'doesn’t match the official version' + ) + }) + + it('falls back to the generic sentence when no skill name is available', () => { 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 index 05af44e1a63..0e9c01c01f1 100644 --- a/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts +++ b/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts @@ -1,3 +1,4 @@ +import { buildAgentFeatureSkillInstallCommand } from '../../../../shared/agent-feature-install-commands' import type { SkillLocationChip, SkillLocationRow } from './skill-freshness-grouping' import { translate } from '@/i18n/i18n' @@ -8,6 +9,9 @@ const SKIPPED_REASON_PRIORITY: SkillLocationChip[] = [ 'unrecognized', 'read-only', 'inaccessible', + // Why: above the placement chips — a copy that is ahead of this build must never + // fall through to the reinstall advice below, which would roll it back. + 'newer', 'in-a-repo', 'plugin-cache', 'external-link', @@ -29,10 +33,18 @@ function blockingChip(locations: readonly SkillLocationRow[]): SkillLocationChip * 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. + * + * `skillName` is only used for the no-chip case, where the fault is not a placement at + * all but the updater's own record of this skill, and the remedy has to name it. */ -export function skippedReason(locations: readonly SkillLocationRow[]): string { +export function skippedReason(locations: readonly SkillLocationRow[], skillName?: string): string { const chip = blockingChip(locations) switch (chip) { + case 'newer': + return translate( + 'auto.components.skills.SkillFreshnessRow.skippedReasonNewer', + 'This copy is a later version than the one this build of Orca ships, so Orca left it alone rather than roll it back. Updating Orca will bring the two back in line.' + ) case 'unrecognized': return translate( 'auto.components.skills.SkillFreshnessRow.skippedReasonUnrecognized', @@ -75,9 +87,22 @@ export function skippedReason(locations: readonly SkillLocationRow[]): string { ) case 'current': case undefined: - return translate( - 'auto.components.skills.SkillFreshnessRow.cantUpdateReason', - 'Orca left this skill out of the update command.' - ) + // Why: no location is at fault here — the copy is an ordinary out-of-date one the + // update would happily write. What is wrong is the updater's own record of it: + // `skills update` decides what to do by comparing that record against the source + // and never reads disk, so when the record is missing or already names the version + // it was meant to fetch, the command reports "up to date" and writes nothing. No + // retry converges it; only a reinstall rewrites the record, which is why the + // sentence has to hand over the command rather than say the update was skipped. + return skillName + ? translate( + 'auto.components.skills.SkillFreshnessRow.skippedReasonStaleRecord', + 'The skills updater has no usable record of this copy, so it reports the skill as already up to date and changes nothing. Reinstall it to bring the record back in line: {{value0}}', + { value0: buildAgentFeatureSkillInstallCommand([skillName]) } + ) + : translate( + 'auto.components.skills.SkillFreshnessRow.cantUpdateReason', + 'Orca left this skill out of the update command.' + ) } } diff --git a/src/renderer/src/components/skills/skill-location-chip-copy.ts b/src/renderer/src/components/skills/skill-location-chip-copy.ts index 82e709665cc..fdee4dce2a8 100644 --- a/src/renderer/src/components/skills/skill-location-chip-copy.ts +++ b/src/renderer/src/components/skills/skill-location-chip-copy.ts @@ -5,6 +5,8 @@ export function chipLabel(chip: SkillLocationChip): string { switch (chip) { case 'current': return translate('auto.components.skills.SkillFreshnessRow.chipCurrent', 'Current') + case 'newer': + return translate('auto.components.skills.SkillFreshnessRow.chipNewer', 'Newer') case 'unrecognized': return translate('auto.components.skills.SkillFreshnessRow.chipUnrecognized', 'Unrecognized') case 'inaccessible': @@ -33,6 +35,11 @@ export function chipTooltip(chip: SkillLocationChip): string { 'auto.components.skills.SkillFreshnessRow.tipCurrent', 'This copy matches the current official version.' ) + case 'newer': + return translate( + 'auto.components.skills.SkillFreshnessRow.tipNewer', + 'This copy is a later version than the one this build of Orca ships.' + ) case 'unrecognized': return translate( 'auto.components.skills.SkillFreshnessRow.tipUnrecognized', diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 8af57bc0104..f79e7b4ca8a 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -3820,7 +3820,11 @@ "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.", - "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." + "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.", + "skippedReasonNewer": "This copy is a later version than the one this build of Orca ships, so Orca left it alone rather than roll it back. Updating Orca will bring the two back in line.", + "skippedReasonStaleRecord": "The skills updater has no usable record of this copy, so it reports the skill as already up to date and changes nothing. Reinstall it to bring the record back in line: {{value0}}", + "chipNewer": "Newer", + "tipNewer": "This copy is a later version than the one this build of Orca ships." }, "SkillFreshnessUpdateDialog": { "title": "Update skills", diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index d60f8704ecf..eb0f09ab65f 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -3797,7 +3797,11 @@ "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.", - "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." + "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.", + "skippedReasonNewer": "This copy is a later version than the one this build of Orca ships, so Orca left it alone rather than roll it back. Updating Orca will bring the two back in line.", + "skippedReasonStaleRecord": "The skills updater has no usable record of this copy, so it reports the skill as already up to date and changes nothing. Reinstall it to bring the record back in line: {{value0}}", + "chipNewer": "Newer", + "tipNewer": "This copy is a later version than the one this build of Orca ships." }, "SkillFreshnessUpdateDialog": { "title": "Actualizar skills", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index e625872dbf1..97933a61fde 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -3797,7 +3797,11 @@ "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.", - "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." + "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.", + "skippedReasonNewer": "This copy is a later version than the one this build of Orca ships, so Orca left it alone rather than roll it back. Updating Orca will bring the two back in line.", + "skippedReasonStaleRecord": "The skills updater has no usable record of this copy, so it reports the skill as already up to date and changes nothing. Reinstall it to bring the record back in line: {{value0}}", + "chipNewer": "Newer", + "tipNewer": "This copy is a later version than the one this build of Orca ships." }, "SkillFreshnessUpdateDialog": { "title": "スキルを更新", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 184d1acebef..ff1a4ec3171 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -3797,7 +3797,11 @@ "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.", - "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." + "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.", + "skippedReasonNewer": "This copy is a later version than the one this build of Orca ships, so Orca left it alone rather than roll it back. Updating Orca will bring the two back in line.", + "skippedReasonStaleRecord": "The skills updater has no usable record of this copy, so it reports the skill as already up to date and changes nothing. Reinstall it to bring the record back in line: {{value0}}", + "chipNewer": "Newer", + "tipNewer": "This copy is a later version than the one this build of Orca ships." }, "SkillFreshnessUpdateDialog": { "title": "스킬 업데이트", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 7f9a1ddfe43..4fc16a95476 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -3797,7 +3797,11 @@ "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.", - "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." + "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.", + "skippedReasonNewer": "This copy is a later version than the one this build of Orca ships, so Orca left it alone rather than roll it back. Updating Orca will bring the two back in line.", + "skippedReasonStaleRecord": "The skills updater has no usable record of this copy, so it reports the skill as already up to date and changes nothing. Reinstall it to bring the record back in line: {{value0}}", + "chipNewer": "Newer", + "tipNewer": "This copy is a later version than the one this build of Orca ships." }, "SkillFreshnessUpdateDialog": { "title": "更新技能",