diff --git a/src/main/skills/discovery.test.ts b/src/main/skills/discovery.test.ts index 720d97f1f66..3d905a01d84 100644 --- a/src/main/skills/discovery.test.ts +++ b/src/main/skills/discovery.test.ts @@ -1,8 +1,13 @@ -import { mkdtemp, mkdir, symlink, writeFile } from 'node:fs/promises' +import { mkdtemp, mkdir, rm, symlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { buildSkillDiscoverySources, clearSkillRootScanCache, discoverSkills } from './discovery' +import { + buildSkillDiscoverySources, + clearSkillRootScanCache, + discoverSkills, + LAST_KNOWN_ROOT_SCAN_RETENTION_MS +} from './discovery' import { TUI_AGENT_CONFIG } from '../../shared/tui-agent-config' import type * as SkillRootFileWalk from './skill-root-file-walk' import type { Repo } from '../../shared/repo-types' @@ -48,6 +53,7 @@ beforeEach(() => { afterEach(() => { unavailableRootPath = null + vi.restoreAllMocks() }) describe('skill discovery', () => { @@ -94,6 +100,76 @@ describe('skill discovery', () => { }) }) + // The impact this exists for: every consumer derives "installed" from the skill + // list, so an empty list for an unanswered root made an installed skill offer + // Install again the moment a mount stalled. + it('serves the last answered skills for a root whose rescan did not answer', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-skills-')) + const home = join(root, 'home') + const stalledRoot = join(home, '.omp', 'agent', 'skills') + await mkdir(join(stalledRoot, 'planning'), { recursive: true }) + await writeFile(join(stalledRoot, 'planning', 'SKILL.md'), '# Planning\n\nPlan work.') + + const first = await discoverSkills({ homeDir: home, cwd: join(root, 'missing-cwd') }) + expect(first.skills.map((skill) => skill.name)).toEqual(['Planning']) + + unavailableRootPath = stalledRoot + const second = await discoverSkills({ + homeDir: home, + cwd: join(root, 'missing-cwd'), + refresh: true + }) + + expect(second.skills.map((skill) => skill.name)).toEqual(['Planning']) + expect(second.sources.find((source) => source.path === stalledRoot)).toMatchObject({ + skippedReason: 'unavailable' + }) + }) + + it('drops the retained copy once a root answers as absent', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-skills-')) + const home = join(root, 'home') + const stalledRoot = join(home, '.omp', 'agent', 'skills') + await mkdir(join(stalledRoot, 'planning'), { recursive: true }) + await writeFile(join(stalledRoot, 'planning', 'SKILL.md'), '# Planning\n\nPlan work.') + await discoverSkills({ homeDir: home, cwd: join(root, 'missing-cwd') }) + await rm(stalledRoot, { recursive: true }) + + await discoverSkills({ homeDir: home, cwd: join(root, 'missing-cwd'), refresh: true }) + unavailableRootPath = stalledRoot + const result = await discoverSkills({ + homeDir: home, + cwd: join(root, 'missing-cwd'), + refresh: true + }) + + // A removed skill must not come back the next time its root stalls. + expect(result.skills.map((skill) => skill.name)).toEqual([]) + }) + + it('stops serving a retained copy once it is older than the retention window', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-skills-')) + const home = join(root, 'home') + const stalledRoot = join(home, '.omp', 'agent', 'skills') + await mkdir(join(stalledRoot, 'planning'), { recursive: true }) + await writeFile(join(stalledRoot, 'planning', 'SKILL.md'), '# Planning\n\nPlan work.') + await discoverSkills({ homeDir: home, cwd: join(root, 'missing-cwd') }) + + const recordedAt = Date.now() + vi.spyOn(Date, 'now').mockReturnValue(recordedAt + LAST_KNOWN_ROOT_SCAN_RETENTION_MS + 1) + unavailableRootPath = stalledRoot + const result = await discoverSkills({ + homeDir: home, + cwd: join(root, 'missing-cwd'), + refresh: true + }) + + expect(result.skills).toEqual([]) + expect(result.sources.find((source) => source.path === stalledRoot)).toMatchObject({ + skippedReason: 'unavailable' + }) + }) + it('discovers home and repo SKILL.md packages with provider metadata', async () => { const root = await mkdtemp(join(tmpdir(), 'orca-skills-')) const home = join(root, 'home') diff --git a/src/main/skills/discovery.ts b/src/main/skills/discovery.ts index 7fa5cd077c5..c5b8515c319 100644 --- a/src/main/skills/discovery.ts +++ b/src/main/skills/discovery.ts @@ -48,14 +48,55 @@ const MAX_CACHED_SKILL_ROOTS = 1_024 // line grow with the install. Root *ids* are safe to log where labels and paths // are not — a repo/plugin id is already a hash. export const MAX_LOGGED_ROOT_IDS = 12 +// Why: a root that did not answer holds unknown skills, not zero, so serving what +// it last held is what stops an installed skill flipping to "Install" while a +// mount is wedged. Bounded rather than forever: it covers a stall +// (MAX_JOINABLE_SCAN_AGE_MS) many times over without pinning a permanently dead +// root to rows that can no longer be checked. +export const LAST_KNOWN_ROOT_SCAN_RETENTION_MS = 5 * 60_000 type RootScan = { exists: boolean; skills: ScannedSkill[]; unavailable?: boolean } const rootScans = new SkillScanCoalescer(MAX_CACHED_SKILL_ROOTS) +/** Last answered scan per root key, read only when a later scan goes unavailable. */ +const lastKnownRootScans = new Map() /** Drop every shared root scan, e.g. after a skill install/update mutates disk. */ export function clearSkillRootScanCache(): void { rootScans.clear() + // A mutation invalidates the retained copy too — it is a pre-mutation answer. + lastKnownRootScans.clear() +} + +function recordLastKnownRootScan(key: string, scan: RootScan): void { + if (!scan.exists) { + // Why: a root that scanned as absent is known-empty, so keeping an older copy + // would resurrect skills the user actually removed the next time it stalls. + lastKnownRootScans.delete(key) + return + } + // Delete first so re-insert refreshes recency under the LRU bound below. + lastKnownRootScans.delete(key) + lastKnownRootScans.set(key, { skills: scan.skills, recordedAt: Date.now() }) + while (lastKnownRootScans.size > MAX_CACHED_SKILL_ROOTS) { + const oldestKey = lastKnownRootScans.keys().next().value + if (oldestKey === undefined) { + break + } + lastKnownRootScans.delete(oldestKey) + } +} + +function readLastKnownRootScan(key: string): ScannedSkill[] { + const retained = lastKnownRootScans.get(key) + if (!retained) { + return [] + } + if (Date.now() - retained.recordedAt > LAST_KNOWN_ROOT_SCAN_RETENTION_MS) { + lastKnownRootScans.delete(key) + return [] + } + return retained.skills } async function pathExists(pathValue: string): Promise { @@ -146,15 +187,18 @@ async function scanRootShared( root: SkillScanRoot, refresh: boolean ): Promise> { + const key = rootScanKey(root) try { - return await rootScans.run( - rootScanKey(root), + const outcome = await rootScans.run( + key, { ttlMs: SKILL_ROOT_SCAN_TTL_MS, refresh }, async (signal) => { const exists = await pathExists(root.path) return { exists, skills: exists ? await scanRoot(root, signal) : [] } } ) + recordLastKnownRootScan(key, outcome.value) + return outcome } catch (error) { if (!isSkillRootUnavailableError(error)) { throw error @@ -166,7 +210,15 @@ async function scanRootShared( // The abort case matters as much as the shed one: callers already waiting on // a scan that is later abandoned for age see it reject, and re-throwing here // would turn one slow root into a failed discovery for every root. - return { value: { exists: true, skills: [], unavailable: true }, cached: false } + // + // Empty skills here is not the same claim as `exists: false`: every consumer + // that derives "installed" does it from the skill list, so shipping an empty + // one turned an unanswered root into proof the skill is gone. Serve what the + // root last held and let `unavailable` carry the uncertainty. + return { + value: { exists: true, skills: readLastKnownRootScan(key), unavailable: true }, + cached: false + } } } diff --git a/src/renderer/src/components/skills/skill-source-inventory.test.ts b/src/renderer/src/components/skills/skill-source-inventory.test.ts index 4aff2e0f8a6..67fd41965e8 100644 --- a/src/renderer/src/components/skills/skill-source-inventory.test.ts +++ b/src/renderer/src/components/skills/skill-source-inventory.test.ts @@ -66,6 +66,20 @@ describe('summarizeSkillSources', () => { expect(scannedSkillSourceCount(entries)).toBe(0) }) + it('reports an unanswered root as unavailable even though it claims to exist', () => { + // Why: the host cannot prove a stalled root is gone, so it reports exists:true + // and carries the uncertainty in skippedReason. Reading exists first counted a + // root nobody walked as scanned, and its retained skills as its full contents. + const entries = summarizeSkillSources( + result({ + sources: [source({ id: 'stalled', exists: true, skippedReason: 'unavailable' })], + skills: [skill()] + }) + ) + expect(entries.map((entry) => entry.status)).toEqual(['unavailable']) + expect(scannedSkillSourceCount(entries)).toBe(0) + }) + it('counts only roots that were actually scanned', () => { const entries = summarizeSkillSources( result({ sources: [source(), source({ id: 'gone', exists: false })] }) diff --git a/src/renderer/src/components/skills/skill-source-inventory.ts b/src/renderer/src/components/skills/skill-source-inventory.ts index df702333c38..1bedd5fa02b 100644 --- a/src/renderer/src/components/skills/skill-source-inventory.ts +++ b/src/renderer/src/components/skills/skill-source-inventory.ts @@ -19,6 +19,12 @@ function ownsSkill(source: SkillDiscoverySource, skill: DiscoveredSkill): boolea } function sourceStatus(source: SkillDiscoverySource): SkillSourceStatus { + // Why before `exists`: an unanswered root reports `exists: true` because the + // host could not prove otherwise. Reading that as `scanned` presented a root + // nobody walked as a successful scan, and its retained skills as its full count. + if (source.skippedReason === 'unavailable') { + return 'unavailable' + } if (source.exists) { return 'scanned' } diff --git a/src/renderer/src/hooks/useInstalledAgentSkills.react.test.tsx b/src/renderer/src/hooks/useInstalledAgentSkills.react.test.tsx index 3c379e1c1f2..cfa9028b130 100644 --- a/src/renderer/src/hooks/useInstalledAgentSkills.react.test.tsx +++ b/src/renderer/src/hooks/useInstalledAgentSkills.react.test.tsx @@ -6,6 +6,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { DiscoveredSkill, SkillDiscoveryResult, + SkillDiscoverySource, SkillDiscoveryTarget } from '../../../shared/skills' import type { ProjectExecutionRuntimeResolution } from '../../../shared/project-execution-runtime' @@ -45,14 +46,32 @@ function skill(overrides: Partial): DiscoveredSkill { } } -function discoveryResult(skills: DiscoveredSkill[] = []): SkillDiscoveryResult { +function discoveryResult( + skills: DiscoveredSkill[] = [], + sources: SkillDiscoverySource[] = [] +): SkillDiscoveryResult { return { skills, - sources: [], + sources, scannedAt: Date.now() } } +/** A root the host could not read: it reports `exists` because it cannot prove otherwise. */ +function unavailableSource(overrides: Partial = {}): SkillDiscoverySource { + return { + id: 'home', + label: 'Agent skills home', + path: '/Users/test/.agents/skills', + sourceKind: 'home', + providers: ['agent-skills'], + owner: null, + exists: true, + skippedReason: 'unavailable', + ...overrides + } +} + function deferred(): { promise: Promise resolve: (value: T) => void @@ -152,6 +171,71 @@ beforeEach(() => { }) describe('useInstalledAgentSkill', () => { + // A root that did not answer holds unknown skills, not zero. Reporting a bare + // "not installed" there is what offered Install for an already-installed skill. + it('says the scan was incomplete when an in-scope root did not answer', async () => { + const scan = deferred() + const discover = vi + .fn<(target?: SkillDiscoveryTarget) => Promise>() + .mockReturnValue(scan.promise) + Object.defineProperty(window, 'api', { + configurable: true, + value: { skills: { discover } } + }) + + await renderProbe() + scan.resolve(discoveryResult([], [unavailableSource()])) + await act(async () => { + await scan.promise + }) + + expect(latestState?.installed).toBe(false) + expect(latestState?.error).toContain('did not respond') + }) + + it('keeps a negative authoritative when the unread root is out of scope', async () => { + const scan = deferred() + const discover = vi + .fn<(target?: SkillDiscoveryTarget) => Promise>() + .mockReturnValue(scan.promise) + Object.defineProperty(window, 'api', { + configurable: true, + value: { skills: { discover } } + }) + + await renderProbe() + // The probe asks only about home roots, so a stalled repo root proves nothing. + scan.resolve( + discoveryResult([], [unavailableSource({ id: 'repo', sourceKind: 'repo', path: '/repo' })]) + ) + await act(async () => { + await scan.promise + }) + + expect(latestState?.installed).toBe(false) + expect(latestState?.error).toBeNull() + }) + + it('does not flag an incomplete scan once the skill is found anyway', async () => { + const scan = deferred() + const discover = vi + .fn<(target?: SkillDiscoveryTarget) => Promise>() + .mockReturnValue(scan.promise) + Object.defineProperty(window, 'api', { + configurable: true, + value: { skills: { discover } } + }) + + await renderProbe() + scan.resolve(discoveryResult([skill({ name: 'orca-linear' })], [unavailableSource()])) + await act(async () => { + await scan.promise + }) + + expect(latestState?.installed).toBe(true) + expect(latestState?.error).toBeNull() + }) + it('ignores stale discovery results after the discovery target changes', async () => { const hostScan = deferred() const wslScan = deferred() diff --git a/src/renderer/src/hooks/useInstalledAgentSkills.test.ts b/src/renderer/src/hooks/useInstalledAgentSkills.test.ts index 7be3dd952c6..d3989662d56 100644 --- a/src/renderer/src/hooks/useInstalledAgentSkills.test.ts +++ b/src/renderer/src/hooks/useInstalledAgentSkills.test.ts @@ -1,11 +1,16 @@ import { afterEach, describe, expect, it, vi } from 'vitest' -import type { DiscoveredSkill, SkillDiscoveryResult } from '../../../shared/skills' +import type { + DiscoveredSkill, + SkillDiscoveryResult, + SkillDiscoverySource +} from '../../../shared/skills' import type { ProjectExecutionRuntimeResolution } from '../../../shared/project-execution-runtime' import { GLOBAL_AGENT_SKILL_SOURCE_KINDS, _installedAgentSkillDiscoveryInternalsForTests, hasInstalledAgentSkill, hasInstalledAgentSkillNamed, + hasUnreadableAgentSkillSource, notifyInstalledAgentSkillsRefreshed } from './useInstalledAgentSkills' @@ -157,6 +162,44 @@ describe('hasInstalledAgentSkill', () => { }) }) +describe('hasUnreadableAgentSkillSource', () => { + function source(overrides: Partial): SkillDiscoverySource { + return { + id: 'home', + label: 'Agent skills home', + path: '/Users/test/.agents/skills', + sourceKind: 'home', + providers: ['agent-skills'], + owner: null, + // An unread root reports `exists`: the host could not prove otherwise. + exists: true, + ...overrides + } + } + + it('flags a root that did not answer even though it reports as present', () => { + expect(hasUnreadableAgentSkillSource([source({ skippedReason: 'unavailable' })])).toBe(true) + }) + + it('ignores roots that were scanned or are genuinely absent', () => { + expect( + hasUnreadableAgentSkillSource([ + source({}), + source({ id: 'gone', exists: false, skippedReason: 'missing' }) + ]) + ).toBe(false) + }) + + it('ignores an unread root outside the scopes the caller asked about', () => { + expect( + hasUnreadableAgentSkillSource( + [source({ id: 'repo', sourceKind: 'repo', skippedReason: 'unavailable' })], + GLOBAL_AGENT_SKILL_SOURCE_KINDS + ) + ).toBe(false) + }) +}) + describe('isOrchestrationSkillName', () => { it('matches only the orchestration skill name', () => { expect( diff --git a/src/renderer/src/hooks/useInstalledAgentSkills.ts b/src/renderer/src/hooks/useInstalledAgentSkills.ts index a7d96fe9451..7ce56b4a369 100644 --- a/src/renderer/src/hooks/useInstalledAgentSkills.ts +++ b/src/renderer/src/hooks/useInstalledAgentSkills.ts @@ -8,6 +8,7 @@ import type { } from '../../../shared/skills' import { ORCHESTRATION_SKILL_NAME } from '@/lib/agent-feature-install-commands' import { markOrchestrationSetupComplete } from '@/lib/orchestration-setup-state' +import { translate } from '@/i18n/i18n' import { discoverInstalledAgentSkills, getCachedSkillDiscovery, @@ -93,6 +94,23 @@ export function hasInstalledAgentSkillNamed( }) } +/** + * True when a root this query cares about did not answer, so its skills are + * unknown rather than absent. The host serves such a root's last answer, but a + * root that has never answered has none to serve, and a bare "Not installed" + * there offers Install for a skill that may already be present. + */ +export function hasUnreadableAgentSkillSource( + sources: readonly SkillDiscoverySource[], + sourceKinds?: readonly SkillSourceKind[] +): boolean { + return sources.some( + (source) => + source.skippedReason === 'unavailable' && + (!sourceKinds || sourceKinds.includes(source.sourceKind)) + ) +} + export function notifyInstalledAgentSkillsRefreshed(): void { if (typeof window !== 'undefined') { window.dispatchEvent(new CustomEvent(INSTALLED_AGENT_SKILLS_REFRESHED_EVENT)) @@ -285,6 +303,11 @@ export function useInstalledAgentSkillNames( [candidateSkillNames, enabled, skills, sourceKinds] ) + const incompleteScan = useMemo( + () => enabled && !installed && hasUnreadableAgentSkillSource(sources, sourceKinds), + [enabled, installed, sources, sourceKinds] + ) + useEffect(() => { if (installed && candidateSkillNames.some(isOrchestrationSkillName)) { // Why: older floating-workspace education still keys off this marker; any @@ -299,7 +322,14 @@ export function useInstalledAgentSkillNames( installed, loading: loadingForRender, settled: enabled && resultForRender !== null, - error: errorForRender, + error: + errorForRender ?? + (incompleteScan + ? translate( + 'auto.hooks.useInstalledAgentSkills.unreadableSkillSource', + 'A skill folder did not respond, so this status may be incomplete.' + ) + : null), skills, sources, refresh: forceRefresh diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index bacd04d4ba1..0ffa9424aeb 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -955,6 +955,9 @@ "useLargeTextControlPaste": { "pasteTooLarge": "Paste is too large." }, + "useInstalledAgentSkills": { + "unreadableSkillSource": "A skill folder did not respond, so this status may be incomplete." + }, "useMacosTccPromptNotice": { "title": "Seeing “Orca would like to access…” prompts?", "description": "Permission messages from macOS may appear when an agent or terminal tool running in Orca attempts to access protected files. Grant Full Disk Access in Settings to reduce these prompts.",