mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 16:03:47 +00:00
fix(skills): keep an unanswered skill root's last known skills instead of reporting none
An aborted or shed root scan degraded to `{skills: [], unavailable: true}`. Every
consumer derives "installed" from the skill list, so an unreadable root read as
proof the skill was gone and an already-installed skill offered Install again.
- discovery: retain the last answered scan per root (5 min, LRU-bounded, dropped
on install/update invalidation and when a root answers as absent) and serve it
when a later scan goes unavailable.
- source inventory: an unanswered root reports `exists: true`, so check
`skippedReason` first rather than presenting it as a successful scan.
- useInstalledAgentSkills: when an in-scope root did not answer and the skill was
not found, say the status may be incomplete instead of a bare "Not installed".
This commit is contained in:
@@ -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')
|
||||
|
||||
@@ -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<RootScan>(MAX_CACHED_SKILL_ROOTS)
|
||||
/** Last answered scan per root key, read only when a later scan goes unavailable. */
|
||||
const lastKnownRootScans = new Map<string, { skills: ScannedSkill[]; recordedAt: number }>()
|
||||
|
||||
/** 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<boolean> {
|
||||
@@ -146,15 +187,18 @@ async function scanRootShared(
|
||||
root: SkillScanRoot,
|
||||
refresh: boolean
|
||||
): Promise<SkillScanOutcome<RootScan>> {
|
||||
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
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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 })] })
|
||||
|
||||
@@ -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'
|
||||
}
|
||||
|
||||
@@ -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>): 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> = {}): 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<T>(): {
|
||||
promise: Promise<T>
|
||||
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<SkillDiscoveryResult>()
|
||||
const discover = vi
|
||||
.fn<(target?: SkillDiscoveryTarget) => Promise<SkillDiscoveryResult>>()
|
||||
.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<SkillDiscoveryResult>()
|
||||
const discover = vi
|
||||
.fn<(target?: SkillDiscoveryTarget) => Promise<SkillDiscoveryResult>>()
|
||||
.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<SkillDiscoveryResult>()
|
||||
const discover = vi
|
||||
.fn<(target?: SkillDiscoveryTarget) => Promise<SkillDiscoveryResult>>()
|
||||
.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<SkillDiscoveryResult>()
|
||||
const wslScan = deferred<SkillDiscoveryResult>()
|
||||
|
||||
@@ -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>): 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(
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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.",
|
||||
|
||||
Reference in New Issue
Block a user