perf(skills): bound and share skill discovery scans (#14204)

Skill discovery re-walked every skill root on every window focus, pane mount,
and connected client. The root set was already bounded; what was not bounded
was how often and how redundantly it was walked.

- Focus called refresh(true), bypassing every cache down to a disk walk.
- The process that owns the disk had no cache and no in-flight dedup.
- Panes with different cwds each re-walked the same 12 home roots.
- Fan-out inside a scan was unbounded, and every package was walked twice
  (once to find SKILL.md, once to count its files, node_modules included).

Adds one coalescing primitive — in-flight dedup plus a short TTL behind a
bounded LRU — used for per-target dedup below both the IPC and RPC entry
points, per-root sharing on the native path, and whole-result reuse on the
WSL path. A scan may publish only while it still owns its pending slot, so a
scan begun before an invalidation can never re-cache a pre-mutation result.
Bounds per-skill fan-out to the existing candidate concurrency limit, and
bounds the package file walk by depth with a node_modules prune.

Focus now reads through a 15s freshness window; explicit signals (install
completed, Settings Refresh, native-chat Retry, terminal exit) set a new
optional `refresh` wire field that bypasses every cache, including on remote
runtimes.

Measured on a 32-concurrent-scan burst across 8 workspaces: 134,880 -> 2,956
filesystem calls and 1080ms -> 45ms, same 31 skills returned.
This commit is contained in:
Neil
2026-08-13 01:21:06 -07:00
committed by GitHub
parent 0f51d0b3bb
commit ede69ffc7f
24 changed files with 1386 additions and 268 deletions
+8 -3
View File
@@ -28,7 +28,8 @@ vi.mock('electron', () => ({
}))
vi.mock('../skills/discovery', () => ({
discoverSkills: discoverSkillsMock
discoverSkills: discoverSkillsMock,
clearSkillRootScanCache: vi.fn()
}))
vi.mock('../skills/skill-discovery-wsl', () => ({
@@ -131,7 +132,7 @@ describe('registerSkillsHandlers', () => {
}
})
expect(discoverSkillsMock).toHaveBeenCalledWith({ repos })
expect(discoverSkillsMock).toHaveBeenCalledWith({ repos, refresh: false })
expect(getWslHomeMock).not.toHaveBeenCalled()
})
@@ -140,7 +141,11 @@ describe('registerSkillsHandlers', () => {
await handler(null, { cwd: '/repo/worktree' })
expect(discoverSkillsMock).toHaveBeenCalledWith({ repos: [], cwd: '/repo/worktree' })
expect(discoverSkillsMock).toHaveBeenCalledWith({
repos: [],
cwd: '/repo/worktree',
refresh: false
})
})
it('uses the selected project WSL distro for skill discovery', async () => {
+10 -1
View File
@@ -15,6 +15,7 @@ import { SkillUpdateRunner } from '../skills/skill-update-run'
import { skillUpdateFailedNames } from '../skills/skill-update-outcome'
import { readGloballyUpdatableSkillLocks } from '../skills/skill-update-registration'
import {
clearSkillDiscoveryCaches,
discoverSkillsOnTarget,
resolveSkillDiscoveryTarget
} from '../skills/skill-discovery-target'
@@ -32,6 +33,9 @@ export function registerSkillsHandlers(store: Store): void {
// Why: per-skill outcomes come from re-hashing what is actually on disk, not
// from scraping stdout.
rescanOutdatedNames: async (names) => {
// Why: the run just rewrote skill packages on this host. Clients that never
// send `refresh` (older builds) would otherwise read a pre-run scan.
clearSkillDiscoveryCaches()
// The lock read is fresh on purpose: the run just rewrote it, and the
// verdict accepts unrecognized content only when disk matches that record.
const [inventory, globalSkillLocks] = await Promise.all([
@@ -53,7 +57,12 @@ export function registerSkillsHandlers(store: Store): void {
'skills:discover',
async (_event, target?: SkillDiscoveryTarget): Promise<SkillDiscoveryResult> => {
const parsedTarget = target ? SkillDiscoveryTargetSchema.parse(target) : undefined
return discoverSkillsOnTarget(resolveSkillDiscoveryTarget(parsedTarget), store.getRepos())
return discoverSkillsOnTarget(
resolveSkillDiscoveryTarget(parsedTarget),
store.getRepos(),
// Why: only a caller that knows disk changed may bypass the shared scans.
{ refresh: parsedTarget?.refresh === true }
)
}
)
+20 -1
View File
@@ -7,7 +7,10 @@ vi.mock('../../../skills/skill-discovery-target', () => ({
}))
import { SKILL_METHODS } from './skills'
import { resolveSkillDiscoveryTarget } from '../../../skills/skill-discovery-target'
import {
discoverSkillsOnTarget,
resolveSkillDiscoveryTarget
} from '../../../skills/skill-discovery-target'
const WSL_RUNTIME = {
status: 'resolved',
@@ -65,4 +68,20 @@ describe('skills.discover RPC', () => {
expect.objectContaining({ projectRuntime: WSL_RUNTIME })
)
})
it('only bypasses the host scan cache when the caller asks for a refresh', async () => {
await discoverMethod().handler({ cwd: '/repo' }, makeContext({}))
expect(vi.mocked(discoverSkillsOnTarget)).toHaveBeenLastCalledWith(expect.anything(), [], {
refresh: false
})
await discoverMethod().handler({ cwd: '/repo', refresh: true }, makeContext({}))
expect(vi.mocked(discoverSkillsOnTarget)).toHaveBeenLastCalledWith(expect.anything(), [], {
refresh: true
})
})
it('accepts a params payload from an older client that cannot send refresh', () => {
expect(discoverMethod().params?.parse({ cwd: '/repo' })).toEqual({ cwd: '/repo' })
})
})
+3 -1
View File
@@ -19,7 +19,9 @@ export const SKILL_METHODS: RpcMethod[] = [
...params,
projectRuntime: runtime.resolveProjectRuntimeForWorktree(params.worktreeId)
}
return discoverSkillsOnTarget(resolveSkillDiscoveryTarget(target), runtime.listRepos())
return discoverSkillsOnTarget(resolveSkillDiscoveryTarget(target), runtime.listRepos(), {
refresh: params.refresh === true
})
}
})
]
+8 -2
View File
@@ -1,8 +1,8 @@
import { mkdtemp, mkdir, symlink, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { describe, expect, it } from 'vitest'
import { buildSkillDiscoverySources, discoverSkills } from './discovery'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { buildSkillDiscoverySources, clearSkillRootScanCache, discoverSkills } from './discovery'
import { TUI_AGENT_CONFIG } from '../../shared/tui-agent-config'
import type { Repo } from '../../shared/types'
@@ -18,6 +18,12 @@ function makeRepo(path: string, connectionId: string | null = null): Repo {
}
}
beforeEach(() => {
// Roots are shared between scans for a few seconds; each case owns its own tree.
clearSkillRootScanCache()
vi.spyOn(console, 'info').mockImplementation(() => undefined)
})
describe('skill discovery', () => {
it('discovers home and repo SKILL.md packages with provider metadata', async () => {
const root = await mkdtemp(join(tmpdir(), 'orca-skills-'))
+118 -172
View File
@@ -1,7 +1,6 @@
import type { Dirent } from 'node:fs'
import { open, readdir, realpath, stat } from 'node:fs/promises'
import { open, realpath, stat } from 'node:fs/promises'
import { homedir } from 'node:os'
import { basename, dirname, isAbsolute, join, relative, sep } from 'node:path'
import { basename, dirname, relative, sep } from 'node:path'
import { summarizeSkillMarkdown } from '../../shared/skill-metadata'
import type { Repo } from '../../shared/types'
import type {
@@ -18,12 +17,41 @@ import {
type SkillScanRoot
} from './skill-discovery-sources'
import { discoverClaudePluginSkillSources } from './claude-plugin-skill-sources'
import { countPackageFiles, findSkillFiles } from './skill-root-file-walk'
import { runSkillCandidateTasks } from './skill-candidate-concurrency'
import { SkillScanCoalescer, type SkillScanOutcome } from './skill-scan-coalescer'
export { buildSkillDiscoverySources } from './skill-discovery-sources'
const SKILL_FILE_NAME = 'SKILL.md'
const MAX_MARKDOWN_BYTES = 256 * 1024
const MAX_SKILL_FILES = 200
// Why: the fixed home roots are identical for every target, so one worktree pane
// per open workspace used to re-walk the same directories once per pane. Sharing
// them for a few seconds is what bounds that fan-out.
export const SKILL_ROOT_SCAN_TTL_MS = 10_000
// Why: sized off the root formula, not a round number. One scan builds
// `12 fixed home roots + 2 per local repo (+ cwd) + plugin roots`, so a bound
// smaller than a single scan's root count makes that scan evict its own earlier
// entries and the cache degrades to a ~0% hit rate — exactly the walk this
// exists to prevent. The live key space is the union across targets — the fixed
// home roots plus two per repo plus two per distinct workspace cwd — so this holds
// a few hundred repos with panes open, not an unbounded install. Past that the LRU
// keeps the hot home roots and the repo roots thrash, which degrades rather than
// breaks. Most repo roots do not exist, and a missing root caches as
// `{exists: false, skills: []}`.
const MAX_CACHED_SKILL_ROOTS = 1_024
// Why: roots grow with the repo count, so an uncapped id list would make one log
// 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
type RootScan = { exists: boolean; skills: ScannedSkill[] }
const rootScans = new SkillScanCoalescer<RootScan>(MAX_CACHED_SKILL_ROOTS)
/** Drop every shared root scan, e.g. after a skill install/update mutates disk. */
export function clearSkillRootScanCache(): void {
rootScans.clear()
}
async function pathExists(pathValue: string): Promise<boolean> {
try {
@@ -34,129 +62,6 @@ async function pathExists(pathValue: string): Promise<boolean> {
}
}
function isWithinDepth(rootPath: string, childPath: string, maxDepth: number): boolean {
const rel = relative(rootPath, childPath)
if (!rel) {
return true
}
// Why: `..cache` is a valid child name; only a real parent traversal escapes.
if (rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)) {
return false
}
return rel.split(sep).length <= maxDepth
}
async function findSkillFiles(rootPath: string, maxDepth: number): Promise<string[]> {
const out: string[] = []
const visitedDirectoryPaths = new Set<string>()
async function visit(dirPath: string): Promise<void> {
if (!isWithinDepth(rootPath, dirPath, maxDepth)) {
return
}
let resolvedDirPath: string
try {
resolvedDirPath = await realpath(dirPath)
} catch {
return
}
if (visitedDirectoryPaths.has(resolvedDirPath)) {
return
}
visitedDirectoryPaths.add(resolvedDirPath)
let entries: Dirent[]
try {
entries = await readdir(dirPath, { withFileTypes: true })
} catch {
return
}
for (const entry of entries) {
const entryPath = join(dirPath, entry.name)
if (entry.name === SKILL_FILE_NAME) {
if (entry.isFile()) {
out.push(entryPath)
continue
}
if (entry.isSymbolicLink()) {
try {
if ((await stat(entryPath)).isFile()) {
out.push(entryPath)
}
} catch {
// Broken links are not valid skill files.
}
}
continue
}
if (entry.isDirectory()) {
await visit(entryPath)
continue
}
if (entry.isSymbolicLink()) {
// Why: users commonly symlink agent skill dirs across providers; follow
// directory links but guard by realpath so recursive links cannot loop.
try {
if ((await stat(entryPath)).isDirectory()) {
await visit(entryPath)
}
} catch {
// Broken links are not valid skill directories.
}
}
}
}
await visit(rootPath)
return out
}
async function countFiles(dirPath: string): Promise<number> {
let count = 0
const visitedDirectoryPaths = new Set<string>()
async function visit(currentPath: string): Promise<void> {
if (count >= MAX_SKILL_FILES) {
return
}
let resolvedPath: string
try {
resolvedPath = await realpath(currentPath)
} catch {
return
}
if (visitedDirectoryPaths.has(resolvedPath)) {
return
}
visitedDirectoryPaths.add(resolvedPath)
let entries: Dirent[]
try {
entries = await readdir(currentPath, { withFileTypes: true })
} catch {
return
}
for (const entry of entries) {
if (count >= MAX_SKILL_FILES) {
return
}
const entryPath = join(currentPath, entry.name)
if (entry.isFile()) {
count += 1
} else if (entry.isDirectory()) {
await visit(entryPath)
} else if (entry.isSymbolicLink()) {
try {
if ((await stat(entryPath)).isFile()) {
count += 1
}
} catch {
// Broken links do not contribute to the skill package file count.
}
}
}
}
await visit(dirPath)
return count
}
async function readSkillSummary(skillFilePath: string): Promise<{
name: string | null
description: string | null
@@ -187,8 +92,11 @@ type ScannedSkill = DiscoveredSkill & { canonicalSkillFilePath: string }
async function scanRoot(root: SkillScanRoot): Promise<ScannedSkill[]> {
const maxDepth = root.sourceKind === 'plugin' ? 9 : 4
const skillFiles = await findSkillFiles(root.path, maxDepth)
const skills = await Promise.all(
skillFiles.map(async (skillFilePath): Promise<ScannedSkill | null> => {
// Why: a root can hold many packages and each one costs a summary read plus a
// package walk. Unbounded fan-out here is what turned one scan into a burst of
// filesystem-metadata work across every core.
const skills = await runSkillCandidateTasks(
skillFiles.map((skillFilePath) => async (): Promise<ScannedSkill | null> => {
// Why: path identity belongs to the scanning host; canonicalizing before
// returning prevents symlinked roots from becoming duplicate picker rows.
const canonicalSkillFilePath = await realpath(skillFilePath).catch(() => skillFilePath)
@@ -211,7 +119,7 @@ async function scanRoot(root: SkillScanRoot): Promise<ScannedSkill[]> {
directoryPath,
skillFilePath,
installed: true,
fileCount: await countFiles(directoryPath),
fileCount: await countPackageFiles(directoryPath),
updatedAt: summary.updatedAt,
canonicalSkillFilePath
} satisfies ScannedSkill
@@ -220,13 +128,66 @@ async function scanRoot(root: SkillScanRoot): Promise<ScannedSkill[]> {
return skills.filter((skill): skill is ScannedSkill => skill !== null)
}
// Why: two roots can share a path (e.g. `~/.claude/skills` is both a home root
// and a repo root when the home dir is the workspace), and their scan differs
// only by depth, which `sourceKind` decides.
function rootScanKey(root: SkillScanRoot): string {
return `${root.sourceKind}\0${root.path}`
}
function scanRootShared(
root: SkillScanRoot,
refresh: boolean
): Promise<SkillScanOutcome<RootScan>> {
return rootScans.run(rootScanKey(root), { ttlMs: SKILL_ROOT_SCAN_TTL_MS, refresh }, async () => {
const exists = await pathExists(root.path)
return { exists, skills: exists ? await scanRoot(root) : [] }
})
}
function mergeScannedSkill(seen: Map<string, DiscoveredSkill>, skill: ScannedSkill): void {
// Why: overlapping repo/cwd roots and symlinked provider homes can reach
// the same file. Keep the first source's higher-level scope identity, but
// record every contributing root so per-agent visibility survives dedup.
const existing = seen.get(skill.canonicalSkillFilePath)
if (!existing) {
const { canonicalSkillFilePath, ...publicSkill } = skill
// Copy: a shared root scan hands the same skill object to every caller, so the
// result each one owns must not alias the cached arrays.
seen.set(canonicalSkillFilePath, {
...publicSkill,
providers: [...publicSkill.providers],
rootPaths: [skill.rootPath]
})
return
}
if (existing.rootPaths && !existing.rootPaths.includes(skill.rootPath)) {
existing.rootPaths.push(skill.rootPath)
}
// Why: providers is per-agent visibility just like rootPaths; keeping only
// the first root's tags makes a shared/symlinked skill under-report which
// agents can see it on the Settings provider badges/filter. Reassign a
// fresh array — `providers` aliases the scan root's array, so pushing in
// place would mutate the root and every sibling skill/source sharing it.
const mergedProviders = [...existing.providers]
for (const provider of skill.providers) {
if (!mergedProviders.includes(provider)) {
mergedProviders.push(provider)
}
}
existing.providers = mergedProviders
}
export async function discoverSkills(args: {
repos?: Repo[]
homeDir?: string
cwd?: string
includeCwd?: boolean
refresh?: boolean
}): Promise<SkillDiscoveryResult> {
const startedAt = Date.now()
const homeDir = args.homeDir ?? homedir()
const refresh = args.refresh === true
const roots = [
...buildSkillDiscoverySources({ ...args, homeDir }),
// Why: plugin discovery is native-chat data keyed to an explicit workspace.
@@ -235,51 +196,36 @@ export async function discoverSkills(args: {
? await discoverClaudePluginSkillSources({ homeDir, cwd: args.cwd })
: [])
]
const sources: SkillDiscoverySource[] = []
const skillGroups = await Promise.all(
roots.map(async (root) => {
const exists = await pathExists(root.path)
sources.push({
...root,
providers: [...root.providers],
exists,
skippedReason: exists ? undefined : 'missing'
})
if (!exists) {
return []
}
return scanRoot(root)
})
)
const scans = await Promise.all(roots.map((root) => scanRootShared(root, refresh)))
const sources: SkillDiscoverySource[] = roots.map((root, index) => ({
...root,
providers: [...root.providers],
exists: scans[index].value.exists,
skippedReason: scans[index].value.exists ? undefined : 'missing'
}))
const seen = new Map<string, DiscoveredSkill>()
for (const skill of skillGroups.flat()) {
// Why: overlapping repo/cwd roots and symlinked provider homes can reach
// the same file. Keep the first source's higher-level scope identity, but
// record every contributing root so per-agent visibility survives dedup.
const existing = seen.get(skill.canonicalSkillFilePath)
if (existing) {
if (existing.rootPaths && !existing.rootPaths.includes(skill.rootPath)) {
existing.rootPaths.push(skill.rootPath)
}
// Why: providers is per-agent visibility just like rootPaths; keeping only
// the first root's tags makes a shared/symlinked skill under-report which
// agents can see it on the Settings provider badges/filter. Reassign a
// fresh array — `providers` aliases the scan root's array, so pushing in
// place would mutate the root and every sibling skill/source sharing it.
const mergedProviders = [...existing.providers]
for (const provider of skill.providers) {
if (!mergedProviders.includes(provider)) {
mergedProviders.push(provider)
}
}
existing.providers = mergedProviders
continue
for (const { value } of scans) {
for (const skill of value.skills) {
mergeScannedSkill(seen, skill)
}
const { canonicalSkillFilePath, ...publicSkill } = skill
seen.set(canonicalSkillFilePath, { ...publicSkill, rootPaths: [skill.rootPath] })
}
const skills = Array.from(seen.values()).sort(compareSkills)
// Why: root *ids* — a repo/plugin id is already a hash, while its label carries
// the repo or plugin name and its path carries the user's directory names. A
// fully cached scan did no filesystem work, so it stays silent rather than
// burying the bursts this line exists to make visible.
const walked = roots.filter((_, index) => !scans[index].cached).map((root) => root.id)
if (walked.length > 0) {
// `present` is not derivable from the rest: "walked 500 roots, 3 existed" is
// the shape that says the root set, not the tree, is what costs. The id list
// is capped because roots grow with the repo count.
const present = sources.filter((source) => source.exists).length
console.info(
`[skills] scan roots=${roots.length} present=${present} walked=${walked.length} skills=${skills.length} ms=${Date.now() - startedAt} ids=${walked.slice(0, MAX_LOGGED_ROOT_IDS).join(',')}`
)
}
return {
skills: Array.from(seen.values()).sort(compareSkills),
skills,
sources: sources.sort((a, b) =>
a.label.localeCompare(b.label, undefined, { sensitivity: 'base' })
),
@@ -0,0 +1,195 @@
import { mkdir, mkdtemp, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join, sep } from 'node:path'
import type * as FsPromises from 'node:fs/promises'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
// Why: the regression is measured in filesystem syscalls, not in returned data, so
// the walk's own readdir is what the assertions below count.
const { readdirPaths } = vi.hoisted(() => ({ readdirPaths: [] as string[] }))
vi.mock('node:fs/promises', async () => {
const actual = await vi.importActual<typeof FsPromises>('node:fs/promises')
return {
...actual,
readdir: (path: Parameters<typeof actual.readdir>[0], ...rest: unknown[]) => {
readdirPaths.push(String(path))
return (actual.readdir as (...args: unknown[]) => unknown)(path, ...rest)
}
}
})
const { clearSkillRootScanCache, discoverSkills, MAX_LOGGED_ROOT_IDS } = await import('./discovery')
async function writeSkill(directory: string, name: string): Promise<void> {
await mkdir(directory, { recursive: true })
await writeFile(join(directory, 'SKILL.md'), `---\nname: ${name}\ndescription: d\n---\n`)
}
/** A home with three provider roots populated, plus `paneCount` workspace roots. */
async function buildFixture(
paneCount: number
): Promise<{ home: string; panes: string[]; noWorkspace: string }> {
const root = await mkdtemp(join(tmpdir(), 'orca-skill-concurrency-'))
const home = join(root, 'home')
await writeSkill(join(home, '.agents', 'skills', 'shared'), 'shared')
await writeSkill(join(home, '.claude', 'skills', 'review'), 'review')
await writeSkill(join(home, '.codex', 'skills', 'plan'), 'plan')
const panes: string[] = []
for (let index = 0; index < paneCount; index += 1) {
const pane = join(root, `pane-${index}`)
await writeSkill(join(pane, '.agents', 'skills', `pane-${index}`), `pane-${index}`)
panes.push(pane)
}
// Why: with no cwd the source builder falls back to process.cwd(), which would
// drag this repo's own skills into every count below.
return { home, panes, noWorkspace: join(root, 'no-workspace') }
}
// One populated root costs three readdirs per real walk: the root itself, the
// package directory while looking for SKILL.md, and the package directory again
// while counting its files. Anything above that is a root walked more than once.
const READDIR_CALLS_PER_POPULATED_ROOT = 3
function readdirCountUnder(path: string): number {
return readdirPaths.filter(
(candidate) => candidate === path || candidate.startsWith(`${path}${sep}`)
).length
}
beforeEach(() => {
clearSkillRootScanCache()
readdirPaths.length = 0
vi.spyOn(console, 'info').mockImplementation(() => undefined)
})
afterEach(() => {
clearSkillRootScanCache()
vi.restoreAllMocks()
})
describe('bounded concurrent skill discovery', () => {
it('collapses a burst of identical scans into one walk of each root', async () => {
const { home, noWorkspace } = await buildFixture(0)
const claudeRoot = join(home, '.claude', 'skills')
const results = await Promise.all(
Array.from({ length: 32 }, () =>
discoverSkills({ homeDir: home, repos: [], cwd: noWorkspace })
)
)
expect(results).toHaveLength(32)
for (const result of results) {
expect(result.skills.map((skill) => skill.name).sort()).toEqual(['plan', 'review', 'shared'])
}
expect(readdirCountUnder(claudeRoot)).toBe(READDIR_CALLS_PER_POPULATED_ROOT)
})
it('shares the fixed home roots across panes that only differ by workspace', async () => {
const { home, panes } = await buildFixture(8)
const results = await Promise.all(
panes.flatMap((pane) =>
Array.from({ length: 4 }, () => discoverSkills({ homeDir: home, repos: [], cwd: pane }))
)
)
expect(results).toHaveLength(32)
// Every pane still sees the shared home skills plus its own workspace skill.
for (const [index, result] of results.entries()) {
const paneIndex = Math.floor(index / 4)
expect(result.skills.map((skill) => skill.name).sort()).toEqual([
`pane-${paneIndex}`,
'plan',
'review',
'shared'
])
}
// The home roots are walked once for all 32 scans; only the per-pane roots repeat.
expect(readdirCountUnder(join(home, '.agents', 'skills'))).toBe(
READDIR_CALLS_PER_POPULATED_ROOT
)
expect(readdirCountUnder(join(home, '.claude', 'skills'))).toBe(
READDIR_CALLS_PER_POPULATED_ROOT
)
expect(readdirCountUnder(join(home, '.codex', 'skills'))).toBe(READDIR_CALLS_PER_POPULATED_ROOT)
for (const pane of panes) {
expect(readdirCountUnder(join(pane, '.agents', 'skills'))).toBe(
READDIR_CALLS_PER_POPULATED_ROOT
)
}
})
it('reports missing roots without walking them', async () => {
const { home, noWorkspace } = await buildFixture(0)
const result = await discoverSkills({ homeDir: home, repos: [], cwd: noWorkspace })
const missing = result.sources.find((source) => source.id === 'home-cursor')
expect(missing?.exists).toBe(false)
expect(missing?.skippedReason).toBe('missing')
expect(readdirCountUnder(join(home, '.cursor', 'skills'))).toBe(0)
})
it('re-reads disk when a caller refreshes, and serves the new result afterwards', async () => {
const { home, noWorkspace } = await buildFixture(0)
await discoverSkills({ homeDir: home, repos: [], cwd: noWorkspace })
await writeSkill(join(home, '.agents', 'skills', 'added-later'), 'added-later')
const stale = await discoverSkills({ homeDir: home, repos: [], cwd: noWorkspace })
expect(stale.skills.map((skill) => skill.name)).not.toContain('added-later')
const refreshed = await discoverSkills({
homeDir: home,
repos: [],
cwd: noWorkspace,
refresh: true
})
expect(refreshed.skills.map((skill) => skill.name)).toContain('added-later')
// Why: asserting only that the skill is present would pass with no caching at
// all — the roots would simply re-walk and find it. The refresh has to leave
// every root cached, or the next scan re-walks the whole set.
readdirPaths.length = 0
const afterRefresh = await discoverSkills({ homeDir: home, repos: [], cwd: noWorkspace })
expect(afterRefresh.skills.map((skill) => skill.name)).toContain('added-later')
expect(readdirPaths).toEqual([])
})
it('gives every caller its own arrays so a shared scan cannot be mutated across results', async () => {
const { home, noWorkspace } = await buildFixture(0)
const [first, second] = await Promise.all([
discoverSkills({ homeDir: home, repos: [], cwd: noWorkspace }),
discoverSkills({ homeDir: home, repos: [], cwd: noWorkspace })
])
const firstShared = first.skills.find((skill) => skill.name === 'shared')
const secondShared = second.skills.find((skill) => skill.name === 'shared')
expect(firstShared?.providers).not.toBe(secondShared?.providers)
expect(firstShared?.rootPaths).not.toBe(secondShared?.rootPaths)
})
it('logs the roots it walked, by id, and never a filesystem path', async () => {
const { home, noWorkspace } = await buildFixture(0)
const info = vi.spyOn(console, 'info').mockImplementation(() => undefined)
await discoverSkills({ homeDir: home, repos: [], cwd: noWorkspace })
const line = String(info.mock.calls.at(0)?.at(0))
// `present` is the signal that separates "big tree" from "big root set", and
// is not derivable from the other counts.
expect(line).toContain('[skills] scan roots=14 present=3 walked=14 skills=3')
expect(line).toContain('home-claude')
expect(line).not.toContain(home)
expect(line).not.toContain(tmpdir())
// The id list is capped so one line cannot grow with the repo count.
expect(line.slice(line.indexOf('ids=')).split(',')).toHaveLength(MAX_LOGGED_ROOT_IDS)
info.mockClear()
await discoverSkills({ homeDir: home, repos: [], cwd: noWorkspace })
// A fully cached scan did no filesystem work, so it must stay silent.
expect(info).not.toHaveBeenCalled()
})
})
@@ -0,0 +1,158 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import type { SkillDiscoveryResult } from '../../shared/skills'
import type { Repo } from '../../shared/types'
const { nativeScans, wslScans } = vi.hoisted(() => ({
nativeScans: [] as unknown[],
wslScans: [] as unknown[]
}))
const emptyResult = (): SkillDiscoveryResult => ({ skills: [], sources: [], scannedAt: 1 })
vi.mock('./discovery', () => ({
clearSkillRootScanCache: vi.fn(),
discoverSkills: vi.fn(async (args: unknown) => {
nativeScans.push(args)
return emptyResult()
})
}))
vi.mock('./skill-discovery-wsl', () => ({
discoverSkillsInWsl: vi.fn(async (args: unknown) => {
wslScans.push(args)
return emptyResult()
})
}))
const { clearSkillDiscoveryCaches, discoverSkillsOnTarget } =
await import('./skill-discovery-target')
const { clearSkillRootScanCache } = await import('./discovery')
function makeRepo(path: string): Repo {
return {
id: `repo-${path}`,
path,
displayName: 'Repo',
badgeColor: '#737373',
addedAt: 1,
kind: 'git',
connectionId: null
}
}
beforeEach(() => {
clearSkillDiscoveryCaches()
nativeScans.length = 0
wslScans.length = 0
vi.mocked(clearSkillRootScanCache).mockClear()
})
afterEach(() => {
clearSkillDiscoveryCaches()
})
describe('discoverSkillsOnTarget', () => {
it('collapses simultaneous identical requests from several clients into one scan', async () => {
await Promise.all(
Array.from({ length: 12 }, () =>
discoverSkillsOnTarget({ kind: 'native-host', cwd: '/workspace' }, [])
)
)
expect(nativeScans).toHaveLength(1)
})
it('does not let two workspaces share a scan', async () => {
await Promise.all([
discoverSkillsOnTarget({ kind: 'native-host', cwd: '/workspace-a' }, []),
discoverSkillsOnTarget({ kind: 'native-host', cwd: '/workspace-b' }, [])
])
expect(nativeScans).toEqual([
{ repos: [], cwd: '/workspace-a', refresh: false },
{ repos: [], cwd: '/workspace-b', refresh: false }
])
})
it('does not let two repo lists share a scan', async () => {
await Promise.all([
discoverSkillsOnTarget({ kind: 'native-host', cwd: undefined }, [makeRepo('/repo-a')]),
discoverSkillsOnTarget({ kind: 'native-host', cwd: undefined }, [makeRepo('/repo-b')])
])
expect(nativeScans).toHaveLength(2)
})
// Why: two clients can hold the same repo set in a different stored order. If the
// digest is order-sensitive they get different keys and each runs a full native
// scan — the fan-out this cache exists to bound. ttl is 0 here, so the pin is
// that two *concurrent* callers coalesce, which they only do on an equal key.
it('keeps repo-list identity stable regardless of stored order', async () => {
const repos = [makeRepo('/repo-a'), makeRepo('/repo-b')]
await discoverSkillsOnTarget({ kind: 'native-host', cwd: undefined }, repos)
await Promise.all([
discoverSkillsOnTarget({ kind: 'native-host', cwd: undefined }, repos.toReversed()),
discoverSkillsOnTarget({ kind: 'native-host', cwd: undefined }, repos)
])
expect(nativeScans).toHaveLength(2)
})
it('forwards refresh to the native scan so it re-reads disk', async () => {
await discoverSkillsOnTarget({ kind: 'native-host', cwd: '/workspace' }, [], { refresh: true })
expect(nativeScans).toEqual([{ repos: [], cwd: '/workspace', refresh: true }])
})
it('reuses a WSL result rather than booting wsl.exe again', async () => {
const target = {
kind: 'wsl',
distro: 'Ubuntu',
homeDir: '/home/dev',
cwd: '/home/dev/repo'
} as const
await discoverSkillsOnTarget(target, [])
await discoverSkillsOnTarget(target, [])
expect(wslScans).toHaveLength(1)
})
it('never shares a WSL result across distros or workspaces', async () => {
await discoverSkillsOnTarget(
{ kind: 'wsl', distro: 'Ubuntu', homeDir: '/home/dev', cwd: '/home/dev/a' },
[]
)
await discoverSkillsOnTarget(
{ kind: 'wsl', distro: 'Fedora', homeDir: '/home/dev', cwd: '/home/dev/a' },
[]
)
await discoverSkillsOnTarget(
{ kind: 'wsl', distro: 'Ubuntu', homeDir: '/home/dev', cwd: '/home/dev/b' },
[]
)
expect(wslScans).toHaveLength(3)
})
it('re-reads a WSL target when the caller refreshes', async () => {
const target = {
kind: 'wsl',
distro: 'Ubuntu',
homeDir: '/home/dev',
cwd: '/home/dev/repo'
} as const
await discoverSkillsOnTarget(target, [])
await discoverSkillsOnTarget(target, [], { refresh: true })
await discoverSkillsOnTarget(target, [])
// The refreshed result replaces the cached one, so the third call is free.
expect(wslScans).toHaveLength(2)
})
it('drops the root cache too when the host clears its scans', () => {
clearSkillDiscoveryCaches()
expect(clearSkillRootScanCache).toHaveBeenCalled()
})
})
+61 -12
View File
@@ -1,8 +1,25 @@
import type { Repo } from '../../shared/types'
import type { SkillDiscoveryResult, SkillDiscoveryTarget } from '../../shared/skills'
import { getDefaultWslDistro, getWslHome, parseWslPath, toLinuxPath } from '../wsl'
import { discoverSkills } from './discovery'
import { clearSkillRootScanCache, discoverSkills } from './discovery'
import { discoverSkillsInWsl } from './skill-discovery-wsl'
import { stablePathId } from './skill-discovery-sources'
import { getRepoExecutionHostId } from '../../shared/execution-host'
import { SkillScanCoalescer } from './skill-scan-coalescer'
// Why: on WSL the unit of cost is the wsl.exe boot plus one `find` per skill, so
// the whole result is what must be shared. The native path shares at root level
// instead, and only needs concurrent callers collapsed into one walk.
const WSL_RESULT_TTL_MS = 10_000
const MAX_CACHED_SKILL_TARGETS = 32
const targetScans = new SkillScanCoalescer<SkillDiscoveryResult>(MAX_CACHED_SKILL_TARGETS)
/** Drop every shared scan; used when a skill update run has rewritten disk. */
export function clearSkillDiscoveryCaches(): void {
targetScans.clear()
clearSkillRootScanCache()
}
export type ResolvedSkillDiscoveryTarget =
| { kind: 'native-host'; cwd: string | undefined }
@@ -53,18 +70,50 @@ export function resolveSkillDiscoveryTarget(
return { kind: 'wsl', distro: wslDistro, homeDir: linuxHomeDir, cwd }
}
// Why: repos widen the native root set, so two targets that differ only by the
// stored repo list must not share a scan. Paths are digested rather than joined
// so the key cannot grow with a large repo list.
function repoDigest(repos: readonly Repo[]): string {
return stablePathId(
repos
// Why: the source builder keeps only locally-executed repos, so the same
// path reassigned to another execution host is a different root set.
.map((repo) => `${getRepoExecutionHostId(repo)}\0${repo.path}`)
.sort((left, right) => left.localeCompare(right))
// NUL is the one byte a path cannot contain, so no repo list can be spelled
// two ways that digest alike.
.join('\0')
)
}
// Keys use exact paths — lowercasing would alias two roots that are distinct on Linux.
function scanKey(target: ResolvedSkillDiscoveryTarget, repos: readonly Repo[]): string {
return target.kind === 'wsl'
? `wsl\0${target.distro}\0${target.homeDir}\0${target.cwd}`
: `native\0${target.cwd ?? ''}\0${target.cwd ? '' : repoDigest(repos)}`
}
export async function discoverSkillsOnTarget(
target: ResolvedSkillDiscoveryTarget,
repos: readonly Repo[]
repos: readonly Repo[],
options: { refresh?: boolean } = {}
): Promise<SkillDiscoveryResult> {
if (target.kind === 'wsl') {
return discoverSkillsInWsl({
distro: target.distro,
homeDir: target.homeDir,
cwd: target.cwd
})
}
return target.cwd
? discoverSkills({ repos: [], cwd: target.cwd })
: discoverSkills({ repos: [...repos] })
const refresh = options.refresh === true
const outcome = await targetScans.run(
scanKey(target, repos),
{ ttlMs: target.kind === 'wsl' ? WSL_RESULT_TTL_MS : 0, refresh },
async () => {
if (target.kind === 'wsl') {
return discoverSkillsInWsl({
distro: target.distro,
homeDir: target.homeDir,
cwd: target.cwd
})
}
return target.cwd
? discoverSkills({ repos: [], cwd: target.cwd, refresh })
: discoverSkills({ repos: [...repos], refresh })
}
)
return outcome.value
}
@@ -0,0 +1,74 @@
import { mkdir, mkdtemp, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { describe, expect, it } from 'vitest'
import {
countPackageFiles,
findSkillFiles,
MAX_SKILL_PACKAGE_DEPTH,
MAX_SKILL_PACKAGE_FILES
} from './skill-root-file-walk'
async function makeTree(): Promise<string> {
return mkdtemp(join(tmpdir(), 'orca-skill-walk-'))
}
async function writeFileAt(path: string, content = 'x'): Promise<void> {
await mkdir(join(path, '..'), { recursive: true })
await writeFile(path, content)
}
describe('countPackageFiles', () => {
it('prunes node_modules so a vendored dependency tree is not the file count', async () => {
const pkg = join(await makeTree(), 'skill')
await writeFileAt(join(pkg, 'SKILL.md'))
for (let index = 0; index < 50; index += 1) {
await writeFileAt(join(pkg, 'node_modules', `dep-${index}`, 'index.js'))
}
expect(await countPackageFiles(pkg)).toBe(1)
})
it('stops at the depth bound instead of walking an arbitrarily deep payload', async () => {
const pkg = join(await makeTree(), 'skill')
await writeFileAt(join(pkg, 'SKILL.md'))
const deep = join(pkg, ...Array.from({ length: MAX_SKILL_PACKAGE_DEPTH + 2 }, () => 'nested'))
await writeFileAt(join(deep, 'buried.txt'))
const reachable = join(pkg, ...Array.from({ length: MAX_SKILL_PACKAGE_DEPTH }, () => 'ok'))
await writeFileAt(join(reachable, 'shallow.txt'))
// The reachable file is counted; the one past the bound is not.
expect(await countPackageFiles(pkg)).toBe(2)
})
// Why: discovery counts `dirname(SKILL.md)`, and a package can vanish between
// being found and being counted — an uninstall mid-scan is exactly that race.
it('returns zero for a directory it cannot read', async () => {
expect(await countPackageFiles(join(await makeTree(), 'absent'))).toBe(0)
})
it('stops at the file bound', async () => {
const pkg = join(await makeTree(), 'skill')
for (let index = 0; index < MAX_SKILL_PACKAGE_FILES + 25; index += 1) {
await writeFileAt(join(pkg, `file-${index}.txt`))
}
expect(await countPackageFiles(pkg)).toBe(MAX_SKILL_PACKAGE_FILES)
})
})
describe('findSkillFiles', () => {
it('finds packages inside the depth bound and ignores deeper ones', async () => {
const root = join(await makeTree(), 'skills')
await writeFileAt(join(root, 'near', 'SKILL.md'))
await writeFileAt(join(root, 'a', 'b', 'c', 'd', 'far', 'SKILL.md'))
const found = await findSkillFiles(root, 4)
expect(found).toEqual([join(root, 'near', 'SKILL.md')])
})
it('returns nothing for a missing root rather than throwing', async () => {
expect(await findSkillFiles(join(await makeTree(), 'absent'), 4)).toEqual([])
})
})
+145
View File
@@ -0,0 +1,145 @@
import type { Dirent } from 'node:fs'
import { readdir, realpath, stat } from 'node:fs/promises'
import { isAbsolute, join, relative, sep } from 'node:path'
export const SKILL_FILE_NAME = 'SKILL.md'
export const MAX_SKILL_PACKAGE_FILES = 200
// Why: `fileCount` is a display number, so the walk that produces it must not be
// able to cost more than the walk that found the skill. A package deeper than this
// under-reports its size; it never hides the skill.
export const MAX_SKILL_PACKAGE_DEPTH = 6
function isWithinDepth(rootPath: string, childPath: string, maxDepth: number): boolean {
const rel = relative(rootPath, childPath)
if (!rel) {
return true
}
// Why: `..cache` is a valid child name; only a real parent traversal escapes.
if (rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)) {
return false
}
return rel.split(sep).length <= maxDepth
}
async function readEntries(dirPath: string): Promise<Dirent[] | null> {
try {
return await readdir(dirPath, { withFileTypes: true })
} catch {
return null
}
}
export async function findSkillFiles(rootPath: string, maxDepth: number): Promise<string[]> {
const out: string[] = []
const visitedDirectoryPaths = new Set<string>()
async function visit(dirPath: string): Promise<void> {
if (!isWithinDepth(rootPath, dirPath, maxDepth)) {
return
}
let resolvedDirPath: string
try {
resolvedDirPath = await realpath(dirPath)
} catch {
return
}
if (visitedDirectoryPaths.has(resolvedDirPath)) {
return
}
visitedDirectoryPaths.add(resolvedDirPath)
const entries = await readEntries(dirPath)
if (!entries) {
return
}
for (const entry of entries) {
const entryPath = join(dirPath, entry.name)
if (entry.name === SKILL_FILE_NAME) {
if (entry.isFile()) {
out.push(entryPath)
continue
}
if (entry.isSymbolicLink()) {
try {
if ((await stat(entryPath)).isFile()) {
out.push(entryPath)
}
} catch {
// Broken links are not valid skill files.
}
}
continue
}
if (entry.isDirectory()) {
await visit(entryPath)
continue
}
if (entry.isSymbolicLink()) {
// Why: users commonly symlink agent skill dirs across providers; follow
// directory links but guard by realpath so recursive links cannot loop.
try {
if ((await stat(entryPath)).isDirectory()) {
await visit(entryPath)
}
} catch {
// Broken links are not valid skill directories.
}
}
}
}
await visit(rootPath)
return out
}
/**
* Files in a skill package, for the picker's size column. Bounded by count and
* depth, and `node_modules` is pruned the way the plugin cache scan prunes it —
* vendored dependencies are payload, not part of the skill.
*/
export async function countPackageFiles(dirPath: string): Promise<number> {
let count = 0
const visitedDirectoryPaths = new Set<string>()
async function visit(currentPath: string, depth: number): Promise<void> {
if (count >= MAX_SKILL_PACKAGE_FILES || depth > MAX_SKILL_PACKAGE_DEPTH) {
return
}
let resolvedPath: string
try {
resolvedPath = await realpath(currentPath)
} catch {
return
}
if (visitedDirectoryPaths.has(resolvedPath)) {
return
}
visitedDirectoryPaths.add(resolvedPath)
const entries = await readEntries(currentPath)
if (!entries) {
return
}
for (const entry of entries) {
if (count >= MAX_SKILL_PACKAGE_FILES) {
return
}
if (entry.name === 'node_modules') {
continue
}
const entryPath = join(currentPath, entry.name)
if (entry.isFile()) {
count += 1
} else if (entry.isDirectory()) {
await visit(entryPath, depth + 1)
} else if (entry.isSymbolicLink()) {
try {
if ((await stat(entryPath)).isFile()) {
count += 1
}
} catch {
// Broken links do not contribute to the skill package file count.
}
}
}
}
await visit(dirPath, 0)
return count
}
@@ -0,0 +1,222 @@
import { describe, expect, it } from 'vitest'
import { SkillScanCoalescer } from './skill-scan-coalescer'
function deferred<T>(): { promise: Promise<T>; resolve: (value: T) => void } {
let resolve!: (value: T) => void
const promise = new Promise<T>((resolvePromise) => {
resolve = resolvePromise
})
return { promise, resolve }
}
describe('SkillScanCoalescer', () => {
it('collapses concurrent callers on one key into a single scan', async () => {
const coalescer = new SkillScanCoalescer<number>(8)
const gate = deferred<number>()
let runs = 0
const task = (): Promise<number> => {
runs += 1
return gate.promise
}
const outcomes = Promise.all([
coalescer.run('root', { ttlMs: 0 }, task),
coalescer.run('root', { ttlMs: 0 }, task),
coalescer.run('root', { ttlMs: 0 }, task)
])
gate.resolve(7)
expect((await outcomes).map((outcome) => outcome.value)).toEqual([7, 7, 7])
expect((await outcomes).map((outcome) => outcome.cached)).toEqual([false, true, true])
expect(runs).toBe(1)
})
it('keeps distinct keys isolated, including paths differing only by case', async () => {
const coalescer = new SkillScanCoalescer<string>(8)
const seen: string[] = []
const run = (key: string): Promise<{ value: string }> =>
coalescer.run(key, { ttlMs: 1_000 }, async () => {
seen.push(key)
return key
})
const [lower, upper] = await Promise.all([run('/home/a/Skills'), run('/home/a/skills')])
expect(lower.value).toBe('/home/a/Skills')
expect(upper.value).toBe('/home/a/skills')
expect(seen).toHaveLength(2)
})
it('reuses a result inside the ttl and rescans after it lapses', async () => {
let now = 1_000
const coalescer = new SkillScanCoalescer<number>(8, () => now)
let runs = 0
const task = async (): Promise<number> => {
runs += 1
return runs
}
expect((await coalescer.run('root', { ttlMs: 100 }, task)).cached).toBe(false)
now = 1_050
const cached = await coalescer.run('root', { ttlMs: 100 }, task)
expect(cached).toEqual({ value: 1, cached: true })
now = 1_101
expect(await coalescer.run('root', { ttlMs: 100 }, task)).toEqual({ value: 2, cached: false })
expect(runs).toBe(2)
})
it('retains nothing when the ttl is zero', async () => {
const coalescer = new SkillScanCoalescer<number>(8)
let runs = 0
const task = async (): Promise<number> => {
runs += 1
return runs
}
await coalescer.run('root', { ttlMs: 0 }, task)
await coalescer.run('root', { ttlMs: 0 }, task)
expect(runs).toBe(2)
})
it('bypasses cached and in-flight results when refreshing', async () => {
let now = 1_000
const coalescer = new SkillScanCoalescer<number>(8, () => now)
let runs = 0
const task = async (): Promise<number> => {
runs += 1
return runs
}
await coalescer.run('root', { ttlMs: 10_000 }, task)
const refreshed = await coalescer.run('root', { ttlMs: 10_000, refresh: true }, task)
expect(refreshed).toEqual({ value: 2, cached: false })
// The refreshed result is what later readers see, not the entry it replaced.
expect(await coalescer.run('root', { ttlMs: 10_000 }, task)).toEqual({ value: 2, cached: true })
expect(runs).toBe(2)
})
// Why: discovery issues one refreshing run() per root, synchronously. An
// invalidation counter shared across keys would let only the last-issued root
// publish and silently discard the rest, so every later scan re-walks them.
it('caches every key when a refresh fans out across roots', async () => {
const coalescer = new SkillScanCoalescer<string>(64)
const keys = ['root-a', 'root-b', 'root-c', 'root-d']
await Promise.all(
keys.map((key) => coalescer.run(key, { ttlMs: 10_000, refresh: true }, async () => key))
)
const readBack = await Promise.all(
keys.map((key) => coalescer.run(key, { ttlMs: 10_000 }, async () => 're-walked'))
)
expect(readBack.map((outcome) => outcome.cached)).toEqual([true, true, true, true])
expect(readBack.map((outcome) => outcome.value)).toEqual(keys)
})
it('does not let a scan that started before a refresh publish its stale result', async () => {
const coalescer = new SkillScanCoalescer<string>(8)
const slowScan = deferred<string>()
// A focus/mount scan is already in flight when the user installs a skill.
const inFlight = coalescer.run('root', { ttlMs: 10_000 }, () => slowScan.promise)
const refreshed = await coalescer.run(
'root',
{ ttlMs: 10_000, refresh: true },
async () => 'after-install'
)
expect(refreshed.value).toBe('after-install')
// The older scan now lands. It must not overwrite the post-install entry with
// a pre-install listing and a fresh lifetime.
slowScan.resolve('before-install')
await inFlight
expect(await coalescer.run('root', { ttlMs: 10_000 }, async () => 'rescanned')).toEqual({
value: 'after-install',
cached: true
})
})
it('does not let a scan that started before clear() repopulate the cache', async () => {
const coalescer = new SkillScanCoalescer<string>(8)
const slowScan = deferred<string>()
const inFlight = coalescer.run('root', { ttlMs: 10_000 }, () => slowScan.promise)
// A skill update run rewrote disk while that scan was walking it.
coalescer.clear()
slowScan.resolve('pre-update')
await inFlight
expect(await coalescer.run('root', { ttlMs: 10_000 }, async () => 'post-update')).toEqual({
value: 'post-update',
cached: false
})
})
it('does not cache a failed scan', async () => {
const coalescer = new SkillScanCoalescer<number>(8)
let runs = 0
await expect(
coalescer.run('root', { ttlMs: 10_000 }, async () => {
runs += 1
throw new Error('scan failed')
})
).rejects.toThrow('scan failed')
expect(await coalescer.run('root', { ttlMs: 10_000 }, async () => 5)).toEqual({
value: 5,
cached: false
})
expect(runs).toBe(1)
})
it('evicts the least recently used entry past the bound', async () => {
let now = 1_000
const coalescer = new SkillScanCoalescer<string>(2, () => now)
const scan = (key: string): Promise<{ cached: boolean }> =>
coalescer.run(key, { ttlMs: 10_000 }, async () => key)
await scan('a')
await scan('b')
// Reading 'a' promotes it, so 'b' is the eviction candidate when 'c' arrives.
expect((await scan('a')).cached).toBe(true)
await scan('c')
expect((await scan('a')).cached).toBe(true)
expect((await scan('b')).cached).toBe(false)
})
// ttl is non-zero on purpose: with ttl 0 nothing is ever written, which hides
// whether the abandoned scan can still publish over the replacement's result.
it('stops joining a scan that never settles, and never lets it publish later', async () => {
let now = 1_000
const coalescer = new SkillScanCoalescer<number>(8, () => now)
const wedged = deferred<number>()
let runs = 0
const task = (): Promise<number> => {
runs += 1
// The first scan models a root on a stalled mount: its readdir never settles.
return runs === 1 ? wedged.promise : Promise.resolve(runs)
}
void coalescer.run('root', { ttlMs: 10_000 }, task)
now = 1_100
// Still young enough to share.
const joined = coalescer.run('root', { ttlMs: 10_000 }, task)
now = 40_000
expect(await coalescer.run('root', { ttlMs: 10_000 }, task)).toEqual({
value: 2,
cached: false
})
expect(runs).toBe(2)
// The wedged callers still receive its eventual value rather than being orphaned...
wedged.resolve(99)
expect((await joined).value).toBe(99)
// ...but it must not overwrite the replacement's newer result with a fresh ttl.
expect(await coalescer.run('root', { ttlMs: 10_000 }, task)).toEqual({ value: 2, cached: true })
})
})
+121
View File
@@ -0,0 +1,121 @@
/** `cached` is false when this call did the filesystem work, for diagnostics. */
export type SkillScanOutcome<T> = { value: T; cached: boolean }
export type SkillScanRunOptions = {
/** 0 keeps nothing after the scan settles, so the entry only dedups concurrent callers. */
ttlMs: number
/** Skip every cached and in-flight result and re-read disk. */
refresh?: boolean
}
type CacheEntry<T> = { value: T; expiresAt: number }
type PendingEntry<T> = { promise: Promise<T>; startedAt: number }
// Why: a root on a stalled network mount can leave a readdir that never settles.
// Joining it forever would make one wedged mount permanently wedge discovery for
// every later caller — worse than before this cache existed, where each caller at
// least retried. Past this age a new caller starts its own scan instead; the old
// promise is dropped, so at most one pending entry per key survives.
const MAX_JOINABLE_SCAN_AGE_MS = 30_000
/**
* Shares one filesystem scan between concurrent callers and, optionally, reuses
* its result for a short window.
*
* Keys are used verbatim — callers must not normalize case, because two paths
* that differ only by case can be two different targets on Linux.
*/
export class SkillScanCoalescer<T> {
private readonly pending = new Map<string, PendingEntry<T>>()
private readonly cache = new Map<string, CacheEntry<T>>()
constructor(
private readonly maximumEntries: number,
private readonly now: () => number = Date.now
) {}
async run(
key: string,
options: SkillScanRunOptions,
task: () => Promise<T>
): Promise<SkillScanOutcome<T>> {
if (options.refresh) {
// Why: a forced caller is answering a mutation it just made, so it must not
// join a scan that may have started before that mutation. Concurrent forced
// callers therefore duplicate; they are rare (install / explicit recheck).
this.cache.delete(key)
return { value: await this.start(key, options.ttlMs, task), cached: false }
}
const fresh = this.readFresh(key)
if (fresh) {
return { value: fresh.value, cached: true }
}
const inFlight = this.pending.get(key)
if (inFlight && this.now() - inFlight.startedAt < MAX_JOINABLE_SCAN_AGE_MS) {
return { value: await inFlight.promise, cached: true }
}
return { value: await this.start(key, options.ttlMs, task), cached: false }
}
/** Drop every cached and in-flight entry (e.g. after a skill update run). */
clear(): void {
this.cache.clear()
this.pending.clear()
}
private start(key: string, ttlMs: number, task: () => Promise<T>): Promise<T> {
const promise = task()
.then((value) => {
// Why: owning the pending slot is what makes a scan publishable, and it is
// per key by construction. Deleting the cache entry is not enough to
// invalidate — a scan that began before the mutation resolves afterwards
// and would re-cache its pre-mutation result with a fresh lifetime. This
// one check covers all three ways it can be superseded: `clear()` empties
// `pending`, a refresh overwrites the slot, and so does the replacement
// for a scan abandoned past MAX_JOINABLE_SCAN_AGE_MS.
if (ttlMs > 0 && this.pending.get(key)?.promise === promise) {
this.write(key, value, ttlMs)
}
return value
})
.finally(() => {
// Why: a newer forced scan may already own this key; only the entry that
// registered itself may remove itself.
if (this.pending.get(key)?.promise === promise) {
this.pending.delete(key)
}
})
// Why: rejections must not surface as an unhandled rejection on the shared
// promise before the caller that started it awaits.
promise.catch(() => undefined)
this.pending.set(key, { promise, startedAt: this.now() })
return promise
}
private readFresh(key: string): CacheEntry<T> | null {
const entry = this.cache.get(key)
if (!entry) {
return null
}
if (entry.expiresAt <= this.now()) {
this.cache.delete(key)
return null
}
// Refresh recency so a hot root outlives a one-off target under the bound.
this.cache.delete(key)
this.cache.set(key, entry)
return entry
}
private write(key: string, value: T, ttlMs: number): void {
this.cache.delete(key)
this.cache.set(key, { value, expiresAt: this.now() + ttlMs })
while (this.cache.size > this.maximumEntries) {
const oldestKey = this.cache.keys().next().value
if (oldestKey === undefined) {
break
}
this.cache.delete(oldestKey)
}
}
}
@@ -1,6 +1,6 @@
// @vitest-environment happy-dom
import { cleanup, render, waitFor } from '@testing-library/react'
import { act, cleanup, render, waitFor } from '@testing-library/react'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import type { NativeChatSkillDiscovery } from './use-native-chat-skills'
@@ -130,6 +130,31 @@ describe('useNativeChatSkills', () => {
expect(mocks.callRuntimeRpc).not.toHaveBeenCalled()
})
it('makes Retry reach disk instead of the host shared scan', async () => {
render(<Probe enabled />)
await waitFor(() => expect(mocks.snapshots.at(-1)?.status).toBe('ready'))
expect(mocks.callRuntimeRpc).toHaveBeenLastCalledWith(
{ kind: 'local' },
'skills.discover',
{ cwd: '/repo/worktree', worktreeId: 'worktree-1' },
{ timeoutMs: 10_000 }
)
act(() => {
mocks.snapshots.at(-1)?.retry()
})
await waitFor(() => expect(mocks.callRuntimeRpc).toHaveBeenCalledTimes(2))
// Why: Retry is the user saying "I changed something" — without `refresh` it
// would be answered from the scan it is trying to get past.
expect(mocks.callRuntimeRpc).toHaveBeenLastCalledWith(
{ kind: 'local' },
'skills.discover',
{ cwd: '/repo/worktree', worktreeId: 'worktree-1', refresh: true },
{ timeoutMs: 10_000 }
)
})
it('routes runtime-owned panes through their saved environment', async () => {
mocks.state = stateForHost('runtime:env-1')
render(<Probe enabled />)
@@ -83,11 +83,17 @@ export function useNativeChatSkills(
const [state, setState] = useState<StoredDiscoveryState>(IDLE_STATE)
const [retryGeneration, setRetryGeneration] = useState(0)
const paneDiscoveryCache = useRef(new Map<string, SkillDiscoveryResult>())
// Why: retry intent belongs to the next request, not to every later render —
// keying off the generation counter would keep forcing after a pane switch.
const forceNextDiscovery = useRef(false)
const profile = getNativeChatAgentProfile(agent)
useEffect(() => {
let cancelled = false
if (!profile || !enabled || !context) {
// Why: there is no pane to retry into, so a pending retry intent must not
// survive to force an unrelated pane's first scan.
forceNextDiscovery.current = false
setState(IDLE_STATE)
return
}
@@ -119,7 +125,12 @@ export function useNativeChatSkills(
return
}
setState({ status: 'loading', skills: [], error: null, contextKey: context.key })
const request = getOrStartDiscovery(context)
// Why: Retry is an explicit "I changed something, look again", so it has to
// reach the host's disk rather than its shared scans. The first attempt for a
// pane rides those scans like every other passive reader.
const forced = forceNextDiscovery.current
forceNextDiscovery.current = false
const request = getOrStartDiscovery(context, forced)
void request.then(
(result) => {
paneDiscoveryCache.current.set(paneCacheKey, result)
@@ -182,6 +193,7 @@ export function useNativeChatSkills(
}, [agent, context, effectiveState, profile])
const retry = useCallback(() => {
forceNextDiscovery.current = true
if (context) {
paneDiscoveryCache.current.delete(context.key)
setState({ status: 'loading', skills: [], error: null, contextKey: context.key })
@@ -201,10 +213,11 @@ export function useNativeChatSkills(
}
function getOrStartDiscovery(
context: NativeChatSkillDiscoveryContext
context: NativeChatSkillDiscoveryContext,
refresh = false
): Promise<SkillDiscoveryResult> {
const existing = inFlightDiscovery.get(context.key)
if (existing) {
if (existing && !refresh) {
return existing
}
// Why: the local runtime.call branch ignores timeoutMs, so the renderer must
@@ -213,7 +226,7 @@ function getOrStartDiscovery(
callRuntimeRpc<SkillDiscoveryResult>(
context.runtimeTarget,
'skills.discover',
context.discoveryTarget,
refresh ? { ...context.discoveryTarget, refresh: true } : context.discoveryTarget,
{
timeoutMs: DISCOVERY_TIMEOUT_MS
}
@@ -85,36 +85,44 @@ export default function SkillsPage(): React.JSX.Element {
const mountedRef = useMountedRef()
const scanGenerationRef = useRef(0)
const loadSkills = useCallback(async (): Promise<void> => {
setLoading(true)
// Why: a cold local scan walks every skill root, so switching runtimes can
// land a stale result after a newer one. Only the newest scan may write.
const scanGeneration = ++scanGenerationRef.current
const isCurrentScan = (): boolean =>
mountedRef.current && scanGeneration === scanGenerationRef.current
if (!runtimeTarget) {
// Why: keep scanning until the owning runtime is known, rather than
// showing the client's skills to someone whose skills live remotely.
return
}
try {
const nextResult = await discoverSkillsForRuntimeTarget(runtimeTarget)
if (isCurrentScan()) {
setResult(nextResult)
// `refresh` bypasses the host's shared scans; only an explicit user re-scan does
// that, so opening the page rides the same cached roots every other surface uses.
const loadSkills = useCallback(
async (refresh = false): Promise<void> => {
setLoading(true)
// Why: a cold local scan walks every skill root, so switching runtimes can
// land a stale result after a newer one. Only the newest scan may write.
const scanGeneration = ++scanGenerationRef.current
const isCurrentScan = (): boolean =>
mountedRef.current && scanGeneration === scanGenerationRef.current
if (!runtimeTarget) {
// Why: keep scanning until the owning runtime is known, rather than
// showing the client's skills to someone whose skills live remotely.
return
}
} catch (error) {
console.error('Failed to discover skills:', error)
if (isCurrentScan()) {
toast.error(
translate('auto.components.skills.SkillsPage.ea72d6185b', 'Could not scan skills')
try {
const nextResult = await discoverSkillsForRuntimeTarget(
runtimeTarget,
refresh ? { refresh: true } : undefined
)
if (isCurrentScan()) {
setResult(nextResult)
}
} catch (error) {
console.error('Failed to discover skills:', error)
if (isCurrentScan()) {
toast.error(
translate('auto.components.skills.SkillsPage.ea72d6185b', 'Could not scan skills')
)
}
} finally {
if (isCurrentScan()) {
setLoading(false)
}
}
} finally {
if (isCurrentScan()) {
setLoading(false)
}
}
}, [mountedRef, runtimeTarget])
},
[mountedRef, runtimeTarget]
)
useEffect(() => {
void loadSkills()
@@ -274,7 +282,7 @@ export default function SkillsPage(): React.JSX.Element {
className="h-8"
disabled={loading}
onClick={() => {
void loadSkills()
void loadSkills(true)
}}
>
<RefreshCw className={cn('size-4', loading && 'animate-spin')} />
@@ -302,7 +310,7 @@ export default function SkillsPage(): React.JSX.Element {
<EmptyState
loading={loading}
hasSkills={skills.length > 0}
onRefresh={() => void loadSkills()}
onRefresh={() => void loadSkills(true)}
/>
)}
</section>
@@ -1,10 +1,11 @@
import { afterEach, describe, expect, it } from 'vitest'
import { afterEach, describe, expect, it, vi } from 'vitest'
import type { SkillDiscoveryResult } from '../../../shared/skills'
import {
clearInstalledAgentSkillDiscoveryCache,
getInstalledAgentSkillDiscoveryCacheSizeForTests,
hasInstalledAgentSkillDiscoveryCacheEntryForTests,
INSTALLED_AGENT_SKILL_DISCOVERY_CACHE_MAX,
INSTALLED_AGENT_SKILL_DISCOVERY_FRESH_MS,
peekInstalledAgentSkillDiscoveryCache,
readInstalledAgentSkillDiscoveryCache,
resetInstalledAgentSkillDiscoveryCacheForTests,
@@ -17,6 +18,7 @@ function result(scannedAt: number): SkillDiscoveryResult {
afterEach(() => {
resetInstalledAgentSkillDiscoveryCacheForTests()
vi.restoreAllMocks()
})
describe('installed agent skill discovery cache', () => {
@@ -89,4 +91,28 @@ describe('installed agent skill discovery cache', () => {
expect(getInstalledAgentSkillDiscoveryCacheSizeForTests()).toBe(0)
expect(peekInstalledAgentSkillDiscoveryCache('target')).toBeNull()
})
it('stops serving a read once the freshness window lapses', () => {
const startedAt = 1_700_000_000_000
const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(startedAt)
writeInstalledAgentSkillDiscoveryCache('target', result(1))
nowSpy.mockReturnValue(startedAt + INSTALLED_AGENT_SKILL_DISCOVERY_FRESH_MS - 1)
expect(readInstalledAgentSkillDiscoveryCache('target')).toEqual(result(1))
nowSpy.mockReturnValue(startedAt + INSTALLED_AGENT_SKILL_DISCOVERY_FRESH_MS)
expect(readInstalledAgentSkillDiscoveryCache('target')).toBeNull()
expect(getInstalledAgentSkillDiscoveryCacheSizeForTests()).toBe(0)
})
it('still peeks a lapsed result so a first render shows the last known state', () => {
const startedAt = 1_700_000_000_000
const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(startedAt)
writeInstalledAgentSkillDiscoveryCache('target', result(1))
nowSpy.mockReturnValue(startedAt + INSTALLED_AGENT_SKILL_DISCOVERY_FRESH_MS + 1)
// A lapsed entry triggers a rescan; showing it beats flashing empty meanwhile.
expect(peekInstalledAgentSkillDiscoveryCache('target')).toEqual(result(1))
})
})
@@ -5,21 +5,44 @@ import type { SkillDiscoveryResult } from '../../../shared/skills'
// the window has ever resolved.
export const INSTALLED_AGENT_SKILL_DISCOVERY_CACHE_MAX = 256
let cachedDiscoveryByTarget = new Map<string, SkillDiscoveryResult>()
// Why: focus-triggered refreshes read through this cache instead of forcing a
// disk walk, so it needs a lifetime — without one a non-forced read would never
// see a skill installed outside Orca. Matches the focus-rescan cooldown the
// freshness inventory already applies to its own scan (`useSkillFreshness`), so
// the two disk-reading surfaces answer a burst of alt-tabs the same way.
export const INSTALLED_AGENT_SKILL_DISCOVERY_FRESH_MS = 15_000
type CachedDiscovery = { result: SkillDiscoveryResult; expiresAt: number }
let cachedDiscoveryByTarget = new Map<string, CachedDiscovery>()
function readUnexpired(key: string): CachedDiscovery | null {
const cached = cachedDiscoveryByTarget.get(key)
if (!cached) {
return null
}
if (cached.expiresAt <= Date.now()) {
cachedDiscoveryByTarget.delete(key)
return null
}
return cached
}
// Why: render reads must not reorder recency — React can discard a render pass.
export function peekInstalledAgentSkillDiscoveryCache(key: string): SkillDiscoveryResult | null {
return cachedDiscoveryByTarget.get(key) ?? null
// Why: an expired entry is still the last thing this window knew, and showing it
// beats flashing empty while the refresh it triggers is in flight.
return cachedDiscoveryByTarget.get(key)?.result ?? null
}
export function readInstalledAgentSkillDiscoveryCache(key: string): SkillDiscoveryResult | null {
const result = cachedDiscoveryByTarget.get(key)
if (!result) {
const cached = readUnexpired(key)
if (!cached) {
return null
}
cachedDiscoveryByTarget.delete(key)
cachedDiscoveryByTarget.set(key, result)
return result
cachedDiscoveryByTarget.set(key, cached)
return cached.result
}
export function writeInstalledAgentSkillDiscoveryCache(
@@ -27,7 +50,10 @@ export function writeInstalledAgentSkillDiscoveryCache(
result: SkillDiscoveryResult
): void {
cachedDiscoveryByTarget.delete(key)
cachedDiscoveryByTarget.set(key, result)
cachedDiscoveryByTarget.set(key, {
result,
expiresAt: Date.now() + INSTALLED_AGENT_SKILL_DISCOVERY_FRESH_MS
})
while (cachedDiscoveryByTarget.size > INSTALLED_AGENT_SKILL_DISCOVERY_CACHE_MAX) {
const oldestKey = cachedDiscoveryByTarget.keys().next().value
if (oldestKey === undefined) {
@@ -104,7 +104,10 @@ function startInstalledAgentSkillDiscovery(
): Promise<SkillDiscoveryResult> {
const generation = discoveryGeneration
const normalizedTarget = normalizeSkillDiscoveryTarget(target)
const discovery = discoverSkillsForRuntimeTarget(runtimeTarget, normalizedTarget)
// Why: a forced caller knows disk changed (install finished, explicit recheck),
// so it must also bypass the host's shared scans — not just this window's cache.
const requestTarget = force ? { ...normalizedTarget, refresh: true } : normalizedTarget
const discovery = discoverSkillsForRuntimeTarget(runtimeTarget, requestTarget)
.then((result) => {
if (generation === discoveryGeneration) {
writeInstalledAgentSkillDiscoveryCache(key, result)
@@ -13,6 +13,7 @@ import type { GlobalSettings } from '../../../shared/types'
import { createCompatibleRuntimeStatusResponseIfNeeded } from '@/runtime/runtime-compatibility-test-fixture'
import { clearRuntimeCompatibilityCacheForTests } from '@/runtime/runtime-rpc-client'
import { useAppStore } from '@/store'
import { INSTALLED_AGENT_SKILL_DISCOVERY_FRESH_MS } from './installed-agent-skill-discovery-cache'
import {
GLOBAL_AGENT_SKILL_SOURCE_KINDS,
type InstalledAgentSkillState,
@@ -216,7 +217,8 @@ describe('useInstalledAgentSkill', () => {
expect(latestState?.installed).toBe(false)
expect(discover).toHaveBeenNthCalledWith(1, undefined)
expect(discover).toHaveBeenNthCalledWith(2, undefined)
// A forced refresh must also bypass the host's shared scans, not just this cache.
expect(discover).toHaveBeenNthCalledWith(2, { refresh: true })
})
it('returns installed from refresh when a legacy Linear skill is discovered', async () => {
@@ -289,13 +291,15 @@ describe('useInstalledAgentSkill', () => {
})
})
it('stays settled through a focus rescan so status surfaces do not flash', async () => {
it('serves a focus rescan from cache so window switching does not walk disk', async () => {
// Why: the freshness window is wall-clock, so pin the clock — a stalled runner
// could otherwise cross it mid-test and turn this into a flake.
vi.spyOn(Date, 'now').mockReturnValue(1_700_000_000_000)
const firstScan = deferred<SkillDiscoveryResult>()
const focusScan = deferred<SkillDiscoveryResult>()
const discover = vi
.fn<(target?: SkillDiscoveryTarget) => Promise<SkillDiscoveryResult>>()
.mockReturnValueOnce(firstScan.promise)
.mockReturnValueOnce(focusScan.promise)
.mockResolvedValue(discoveryResult([skill({ name: 'orca-linear' })]))
Object.defineProperty(window, 'api', {
configurable: true,
value: { skills: { discover } }
@@ -310,21 +314,48 @@ describe('useInstalledAgentSkill', () => {
})
expect(latestState?.settled).toBe(true)
expect(latestState?.installed).toBe(true)
expect(discover).toHaveBeenCalledTimes(1)
for (let focusEvent = 0; focusEvent < 5; focusEvent += 1) {
await act(async () => {
window.dispatchEvent(new Event('focus'))
})
await flushMicrotasks()
}
// Focus is a backstop, not a mutation signal: a burst of window switches
// costs no scans, and the surface never flashes unsettled.
expect(discover).toHaveBeenCalledTimes(1)
expect(latestState?.loading).toBe(false)
expect(latestState?.settled).toBe(true)
expect(latestState?.installed).toBe(true)
})
it('rescans on focus once the cached scan is no longer fresh', async () => {
const nowSpy = vi.spyOn(Date, 'now')
const startedAt = 1_700_000_000_000
nowSpy.mockReturnValue(startedAt)
const discover = vi
.fn<(target?: SkillDiscoveryTarget) => Promise<SkillDiscoveryResult>>()
.mockResolvedValue(discoveryResult([skill({ name: 'orca-linear' })]))
Object.defineProperty(window, 'api', {
configurable: true,
value: { skills: { discover } }
})
await renderProbe()
await flushMicrotasks()
expect(discover).toHaveBeenCalledTimes(1)
nowSpy.mockReturnValue(startedAt + INSTALLED_AGENT_SKILL_DISCOVERY_FRESH_MS + 1)
await act(async () => {
window.dispatchEvent(new Event('focus'))
})
await flushMicrotasks()
// The forced rescan is in flight, but the previous result is still current.
expect(latestState?.loading).toBe(true)
expect(latestState?.settled).toBe(true)
expect(latestState?.installed).toBe(true)
focusScan.resolve(discoveryResult([skill({ name: 'orca-linear' })]))
await act(async () => {
await focusScan.promise
})
expect(latestState?.settled).toBe(true)
// The freshness window is what bounds the storm; past it, focus still reads disk.
expect(discover).toHaveBeenCalledTimes(2)
expect(discover).toHaveBeenLastCalledWith(undefined)
})
it('reuses cached discovery when another surface finishes re-checking', async () => {
@@ -351,13 +382,13 @@ describe('useInstalledAgentSkill', () => {
expect(latestState?.installed).toBe(true)
})
it('clears loading when a silent refresh supersedes an in-flight focus rescan', async () => {
it('clears loading when a silent refresh supersedes an in-flight forced rescan', async () => {
const firstScan = deferred<SkillDiscoveryResult>()
const focusScan = deferred<SkillDiscoveryResult>()
const forcedScan = deferred<SkillDiscoveryResult>()
const discover = vi
.fn<(target?: SkillDiscoveryTarget) => Promise<SkillDiscoveryResult>>()
.mockReturnValueOnce(firstScan.promise)
.mockReturnValueOnce(focusScan.promise)
.mockReturnValueOnce(forcedScan.promise)
Object.defineProperty(window, 'api', {
configurable: true,
value: { skills: { discover } }
@@ -369,8 +400,10 @@ describe('useInstalledAgentSkill', () => {
await firstScan.promise
})
// The recheck button forces past the cache without clearing it, so the silent
// refresh below can still be served from it while this scan is in flight.
await act(async () => {
window.dispatchEvent(new Event('focus'))
void latestState?.refresh()
})
expect(latestState?.loading).toBe(true)
@@ -382,9 +415,9 @@ describe('useInstalledAgentSkill', () => {
await flushMicrotasks()
expect(latestState?.loading).toBe(false)
focusScan.resolve(discoveryResult([skill({ name: 'orca-linear' })]))
forcedScan.resolve(discoveryResult([skill({ name: 'orca-linear' })]))
await act(async () => {
await focusScan.promise
await forcedScan.promise
})
expect(latestState?.loading).toBe(false)
})
@@ -247,21 +247,26 @@ export function useInstalledAgentSkillNames(
if (!enabled) {
return
}
const refreshFromExternalChange = (): void => {
// Why: skill install commands run outside React state, often in a terminal, so
// an install event is authoritative and forces past every cache.
const refreshFromInstall = (): void => {
void refresh(true)
}
const refreshFromCompletedScan = (): void => {
// Why: focus fires on every app and window switch, and a forced refresh
// bypasses every cache down to the host's disk walk — that is what turned an
// alt-tab into a multi-root filesystem scan per window and per client. Focus,
// and another surface finishing its own scan, are both only hints that
// something may have changed, so they read through the freshness window.
const refreshQuietly = (): void => {
void refresh(false, false)
}
// Why: skill install commands run outside React state, often in a terminal.
// Refresh on focus and explicit install events so completion is detected.
window.addEventListener('focus', refreshFromExternalChange)
window.addEventListener(INSTALLED_AGENT_SKILLS_CHANGED_EVENT, refreshFromExternalChange)
window.addEventListener(INSTALLED_AGENT_SKILLS_REFRESHED_EVENT, refreshFromCompletedScan)
window.addEventListener('focus', refreshQuietly)
window.addEventListener(INSTALLED_AGENT_SKILLS_CHANGED_EVENT, refreshFromInstall)
window.addEventListener(INSTALLED_AGENT_SKILLS_REFRESHED_EVENT, refreshQuietly)
return () => {
window.removeEventListener('focus', refreshFromExternalChange)
window.removeEventListener(INSTALLED_AGENT_SKILLS_CHANGED_EVENT, refreshFromExternalChange)
window.removeEventListener(INSTALLED_AGENT_SKILLS_REFRESHED_EVENT, refreshFromCompletedScan)
window.removeEventListener('focus', refreshQuietly)
window.removeEventListener(INSTALLED_AGENT_SKILLS_CHANGED_EVENT, refreshFromInstall)
window.removeEventListener(INSTALLED_AGENT_SKILLS_REFRESHED_EVENT, refreshQuietly)
}
}, [enabled, refresh])
@@ -99,4 +99,24 @@ describe('discoverSkillsForRuntimeTarget', () => {
expect.objectContaining({ method: 'skills.discover', params: {} })
)
})
// Why: refresh describes the request, not the client's host. Dropping it would
// leave an explicit re-check reading the remote host's shared scan instead of
// its disk, which is exactly what an install-completed refresh must not do.
it('forwards an explicit refresh to a remote runtime', async () => {
runtimeEnvironmentCall.mockResolvedValueOnce({
id: 'skills',
ok: true,
result: discoveryResult('orchestration')
})
await discoverSkillsForRuntimeTarget(
{ kind: 'environment', environmentId: 'env-1' },
{ runtime: 'wsl', wslDistro: 'Ubuntu', refresh: true }
)
expect(runtimeEnvironmentCall).toHaveBeenCalledWith(
expect.objectContaining({ method: 'skills.discover', params: { refresh: true } })
)
})
})
@@ -9,13 +9,17 @@ const SKILL_DISCOVERY_TIMEOUT_MS = 15_000
* when one is active. This keeps install badges in sync with where the skill
* files land instead of always reading the client's disk (#6789).
*
* The target is dropped entirely for a remote call. Every target any caller can
* The target is otherwise dropped for a remote call. Every target any caller can
* currently produce describes the *client's* host — a WSL distro or a local
* project-runtime resolution — and forwarding those would ask a Linux server to
* resolve a WSL distro it does not have. The server does honour `cwd` and
* `worktreeId` (see `main/runtime/rpc/methods/skills.ts`), so if a caller ever
* supplies workspace identity, forward those two fields rather than widening
* this to the whole target.
*
* `refresh` is the exception and must be forwarded: it describes the *request*,
* not the client's host, and it is the only way an explicit re-check reaches
* past the remote host's shared scans to its disk.
*/
export async function discoverSkillsForRuntimeTarget(
runtimeTarget: RuntimeClientTarget,
@@ -27,7 +31,7 @@ export async function discoverSkillsForRuntimeTarget(
return callRuntimeRpc<SkillDiscoveryResult>(
runtimeTarget,
'skills.discover',
{},
target?.refresh ? { refresh: true } : {},
{ timeoutMs: SKILL_DISCOVERY_TIMEOUT_MS }
)
}
+5 -1
View File
@@ -51,6 +51,9 @@ export type SkillDiscoveryTarget = {
* when the caller (e.g. a remote client) cannot supply `projectRuntime`. */
worktreeId?: string | null
projectRuntime?: ProjectExecutionRuntimeResolution
/** Bypass the host's shared scans because the caller knows disk just changed.
* Optional so an older host simply ignores it and scans as it always did. */
refresh?: boolean
}
const ResolvedProjectRuntimeSchema = z.object({
@@ -100,7 +103,8 @@ export const SkillDiscoveryTargetSchema: z.ZodType<SkillDiscoveryTarget> = z.obj
worktreeId: z.string().nullable().optional(),
projectRuntime: z
.discriminatedUnion('status', [ResolvedProjectRuntimeSchema, RepairProjectRuntimeSchema])
.optional()
.optional(),
refresh: z.boolean().optional()
})
export type SkillFrontmatterSummary = {