From ede69ffc7f9963cc0350c3a282df26622c64ef04 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 13 Aug 2026 01:21:06 -0700 Subject: [PATCH] perf(skills): bound and share skill discovery scans (#14204) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/main/ipc/skills.test.ts | 11 +- src/main/ipc/skills.ts | 11 +- src/main/runtime/rpc/methods/skills.test.ts | 21 +- src/main/runtime/rpc/methods/skills.ts | 4 +- src/main/skills/discovery.test.ts | 10 +- src/main/skills/discovery.ts | 290 +++++++----------- .../skill-discovery-concurrency.test.ts | 195 ++++++++++++ .../skills/skill-discovery-target.test.ts | 158 ++++++++++ src/main/skills/skill-discovery-target.ts | 73 ++++- src/main/skills/skill-root-file-walk.test.ts | 74 +++++ src/main/skills/skill-root-file-walk.ts | 145 +++++++++ src/main/skills/skill-scan-coalescer.test.ts | 222 ++++++++++++++ src/main/skills/skill-scan-coalescer.ts | 121 ++++++++ .../use-native-chat-skills.react.test.tsx | 27 +- .../native-chat/use-native-chat-skills.ts | 21 +- .../src/components/skills/SkillsPage.tsx | 66 ++-- ...talled-agent-skill-discovery-cache.test.ts | 28 +- .../installed-agent-skill-discovery-cache.ts | 40 ++- .../hooks/installed-agent-skill-discovery.ts | 5 +- .../useInstalledAgentSkills.react.test.tsx | 73 +++-- .../src/hooks/useInstalledAgentSkills.ts | 25 +- .../src/runtime/runtime-skills-client.test.ts | 20 ++ .../src/runtime/runtime-skills-client.ts | 8 +- src/shared/skills.ts | 6 +- 24 files changed, 1386 insertions(+), 268 deletions(-) create mode 100644 src/main/skills/skill-discovery-concurrency.test.ts create mode 100644 src/main/skills/skill-discovery-target.test.ts create mode 100644 src/main/skills/skill-root-file-walk.test.ts create mode 100644 src/main/skills/skill-root-file-walk.ts create mode 100644 src/main/skills/skill-scan-coalescer.test.ts create mode 100644 src/main/skills/skill-scan-coalescer.ts diff --git a/src/main/ipc/skills.test.ts b/src/main/ipc/skills.test.ts index e6f88345ff2..b85abfe276d 100644 --- a/src/main/ipc/skills.test.ts +++ b/src/main/ipc/skills.test.ts @@ -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 () => { diff --git a/src/main/ipc/skills.ts b/src/main/ipc/skills.ts index e2d12ca57ba..746f02416d1 100644 --- a/src/main/ipc/skills.ts +++ b/src/main/ipc/skills.ts @@ -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 => { 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 } + ) } ) diff --git a/src/main/runtime/rpc/methods/skills.test.ts b/src/main/runtime/rpc/methods/skills.test.ts index a4335ec2e62..cd82be0ca3a 100644 --- a/src/main/runtime/rpc/methods/skills.test.ts +++ b/src/main/runtime/rpc/methods/skills.test.ts @@ -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' }) + }) }) diff --git a/src/main/runtime/rpc/methods/skills.ts b/src/main/runtime/rpc/methods/skills.ts index 346198aeffc..f6e5acbe6d9 100644 --- a/src/main/runtime/rpc/methods/skills.ts +++ b/src/main/runtime/rpc/methods/skills.ts @@ -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 + }) } }) ] diff --git a/src/main/skills/discovery.test.ts b/src/main/skills/discovery.test.ts index 9f94e9f8347..55dcb55ed36 100644 --- a/src/main/skills/discovery.test.ts +++ b/src/main/skills/discovery.test.ts @@ -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-')) diff --git a/src/main/skills/discovery.ts b/src/main/skills/discovery.ts index 00edfc8e705..07bebe52b29 100644 --- a/src/main/skills/discovery.ts +++ b/src/main/skills/discovery.ts @@ -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(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 { try { @@ -34,129 +62,6 @@ async function pathExists(pathValue: string): Promise { } } -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 { - const out: string[] = [] - const visitedDirectoryPaths = new Set() - async function visit(dirPath: string): Promise { - 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 { - let count = 0 - const visitedDirectoryPaths = new Set() - async function visit(currentPath: string): Promise { - 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 { const maxDepth = root.sourceKind === 'plugin' ? 9 : 4 const skillFiles = await findSkillFiles(root.path, maxDepth) - const skills = await Promise.all( - skillFiles.map(async (skillFilePath): Promise => { + // 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 => { // 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 { 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 { 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> { + 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, 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 { + 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() - 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' }) ), diff --git a/src/main/skills/skill-discovery-concurrency.test.ts b/src/main/skills/skill-discovery-concurrency.test.ts new file mode 100644 index 00000000000..162ea10cf89 --- /dev/null +++ b/src/main/skills/skill-discovery-concurrency.test.ts @@ -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('node:fs/promises') + return { + ...actual, + readdir: (path: Parameters[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 { + 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() + }) +}) diff --git a/src/main/skills/skill-discovery-target.test.ts b/src/main/skills/skill-discovery-target.test.ts new file mode 100644 index 00000000000..f136536b189 --- /dev/null +++ b/src/main/skills/skill-discovery-target.test.ts @@ -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() + }) +}) diff --git a/src/main/skills/skill-discovery-target.ts b/src/main/skills/skill-discovery-target.ts index 2be10232c8c..3670aef905e 100644 --- a/src/main/skills/skill-discovery-target.ts +++ b/src/main/skills/skill-discovery-target.ts @@ -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(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 { - 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 } diff --git a/src/main/skills/skill-root-file-walk.test.ts b/src/main/skills/skill-root-file-walk.test.ts new file mode 100644 index 00000000000..654312d7f5d --- /dev/null +++ b/src/main/skills/skill-root-file-walk.test.ts @@ -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 { + return mkdtemp(join(tmpdir(), 'orca-skill-walk-')) +} + +async function writeFileAt(path: string, content = 'x'): Promise { + 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([]) + }) +}) diff --git a/src/main/skills/skill-root-file-walk.ts b/src/main/skills/skill-root-file-walk.ts new file mode 100644 index 00000000000..ab7aedc4249 --- /dev/null +++ b/src/main/skills/skill-root-file-walk.ts @@ -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 { + try { + return await readdir(dirPath, { withFileTypes: true }) + } catch { + return null + } +} + +export async function findSkillFiles(rootPath: string, maxDepth: number): Promise { + const out: string[] = [] + const visitedDirectoryPaths = new Set() + async function visit(dirPath: string): Promise { + 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 { + let count = 0 + const visitedDirectoryPaths = new Set() + async function visit(currentPath: string, depth: number): Promise { + 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 +} diff --git a/src/main/skills/skill-scan-coalescer.test.ts b/src/main/skills/skill-scan-coalescer.test.ts new file mode 100644 index 00000000000..7a8d1bb1539 --- /dev/null +++ b/src/main/skills/skill-scan-coalescer.test.ts @@ -0,0 +1,222 @@ +import { describe, expect, it } from 'vitest' +import { SkillScanCoalescer } from './skill-scan-coalescer' + +function deferred(): { promise: Promise; resolve: (value: T) => void } { + let resolve!: (value: T) => void + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise + }) + return { promise, resolve } +} + +describe('SkillScanCoalescer', () => { + it('collapses concurrent callers on one key into a single scan', async () => { + const coalescer = new SkillScanCoalescer(8) + const gate = deferred() + let runs = 0 + const task = (): Promise => { + 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(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(8, () => now) + let runs = 0 + const task = async (): Promise => { + 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(8) + let runs = 0 + const task = async (): Promise => { + 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(8, () => now) + let runs = 0 + const task = async (): Promise => { + 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(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(8) + const slowScan = deferred() + + // 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(8) + const slowScan = deferred() + + 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(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(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(8, () => now) + const wedged = deferred() + let runs = 0 + const task = (): Promise => { + 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 }) + }) +}) diff --git a/src/main/skills/skill-scan-coalescer.ts b/src/main/skills/skill-scan-coalescer.ts new file mode 100644 index 00000000000..422a3045783 --- /dev/null +++ b/src/main/skills/skill-scan-coalescer.ts @@ -0,0 +1,121 @@ +/** `cached` is false when this call did the filesystem work, for diagnostics. */ +export type SkillScanOutcome = { 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 = { value: T; expiresAt: number } +type PendingEntry = { promise: Promise; 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 { + private readonly pending = new Map>() + private readonly cache = new Map>() + + constructor( + private readonly maximumEntries: number, + private readonly now: () => number = Date.now + ) {} + + async run( + key: string, + options: SkillScanRunOptions, + task: () => Promise + ): Promise> { + 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): Promise { + 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 | 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) + } + } +} diff --git a/src/renderer/src/components/native-chat/use-native-chat-skills.react.test.tsx b/src/renderer/src/components/native-chat/use-native-chat-skills.react.test.tsx index 647a07b2f4f..8004071d679 100644 --- a/src/renderer/src/components/native-chat/use-native-chat-skills.react.test.tsx +++ b/src/renderer/src/components/native-chat/use-native-chat-skills.react.test.tsx @@ -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() + 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() diff --git a/src/renderer/src/components/native-chat/use-native-chat-skills.ts b/src/renderer/src/components/native-chat/use-native-chat-skills.ts index 42c7702d017..f8a7e943935 100644 --- a/src/renderer/src/components/native-chat/use-native-chat-skills.ts +++ b/src/renderer/src/components/native-chat/use-native-chat-skills.ts @@ -83,11 +83,17 @@ export function useNativeChatSkills( const [state, setState] = useState(IDLE_STATE) const [retryGeneration, setRetryGeneration] = useState(0) const paneDiscoveryCache = useRef(new Map()) + // 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 { 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( context.runtimeTarget, 'skills.discover', - context.discoveryTarget, + refresh ? { ...context.discoveryTarget, refresh: true } : context.discoveryTarget, { timeoutMs: DISCOVERY_TIMEOUT_MS } diff --git a/src/renderer/src/components/skills/SkillsPage.tsx b/src/renderer/src/components/skills/SkillsPage.tsx index 70b7795912b..8bb3c6fef9d 100644 --- a/src/renderer/src/components/skills/SkillsPage.tsx +++ b/src/renderer/src/components/skills/SkillsPage.tsx @@ -85,36 +85,44 @@ export default function SkillsPage(): React.JSX.Element { const mountedRef = useMountedRef() const scanGenerationRef = useRef(0) - const loadSkills = useCallback(async (): Promise => { - 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 => { + 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) }} > @@ -302,7 +310,7 @@ export default function SkillsPage(): React.JSX.Element { 0} - onRefresh={() => void loadSkills()} + onRefresh={() => void loadSkills(true)} /> )} diff --git a/src/renderer/src/hooks/installed-agent-skill-discovery-cache.test.ts b/src/renderer/src/hooks/installed-agent-skill-discovery-cache.test.ts index ed23b3c5d76..0a99ddd26df 100644 --- a/src/renderer/src/hooks/installed-agent-skill-discovery-cache.test.ts +++ b/src/renderer/src/hooks/installed-agent-skill-discovery-cache.test.ts @@ -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)) + }) }) diff --git a/src/renderer/src/hooks/installed-agent-skill-discovery-cache.ts b/src/renderer/src/hooks/installed-agent-skill-discovery-cache.ts index a93abe2c3f9..29d30eb4c98 100644 --- a/src/renderer/src/hooks/installed-agent-skill-discovery-cache.ts +++ b/src/renderer/src/hooks/installed-agent-skill-discovery-cache.ts @@ -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() +// 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() + +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) { diff --git a/src/renderer/src/hooks/installed-agent-skill-discovery.ts b/src/renderer/src/hooks/installed-agent-skill-discovery.ts index a501611889c..2530bdeb11a 100644 --- a/src/renderer/src/hooks/installed-agent-skill-discovery.ts +++ b/src/renderer/src/hooks/installed-agent-skill-discovery.ts @@ -104,7 +104,10 @@ function startInstalledAgentSkillDiscovery( ): Promise { 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) diff --git a/src/renderer/src/hooks/useInstalledAgentSkills.react.test.tsx b/src/renderer/src/hooks/useInstalledAgentSkills.react.test.tsx index f6b446964ac..19494e46aac 100644 --- a/src/renderer/src/hooks/useInstalledAgentSkills.react.test.tsx +++ b/src/renderer/src/hooks/useInstalledAgentSkills.react.test.tsx @@ -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() - const focusScan = deferred() const discover = vi .fn<(target?: SkillDiscoveryTarget) => Promise>() .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>() + .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() - const focusScan = deferred() + const forcedScan = deferred() const discover = vi .fn<(target?: SkillDiscoveryTarget) => Promise>() .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) }) diff --git a/src/renderer/src/hooks/useInstalledAgentSkills.ts b/src/renderer/src/hooks/useInstalledAgentSkills.ts index 482ea67386d..a7d96fe9451 100644 --- a/src/renderer/src/hooks/useInstalledAgentSkills.ts +++ b/src/renderer/src/hooks/useInstalledAgentSkills.ts @@ -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]) diff --git a/src/renderer/src/runtime/runtime-skills-client.test.ts b/src/renderer/src/runtime/runtime-skills-client.test.ts index 197db33cddf..e4535e3eff6 100644 --- a/src/renderer/src/runtime/runtime-skills-client.test.ts +++ b/src/renderer/src/runtime/runtime-skills-client.test.ts @@ -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 } }) + ) + }) }) diff --git a/src/renderer/src/runtime/runtime-skills-client.ts b/src/renderer/src/runtime/runtime-skills-client.ts index 700e90a66fd..40101379a81 100644 --- a/src/renderer/src/runtime/runtime-skills-client.ts +++ b/src/renderer/src/runtime/runtime-skills-client.ts @@ -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( runtimeTarget, 'skills.discover', - {}, + target?.refresh ? { refresh: true } : {}, { timeoutMs: SKILL_DISCOVERY_TIMEOUT_MS } ) } diff --git a/src/shared/skills.ts b/src/shared/skills.ts index c908954d3a9..381ebf6eb3a 100644 --- a/src/shared/skills.ts +++ b/src/shared/skills.ts @@ -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 = z.obj worktreeId: z.string().nullable().optional(), projectRuntime: z .discriminatedUnion('status', [ResolvedProjectRuntimeSchema, RepairProjectRuntimeSchema]) - .optional() + .optional(), + refresh: z.boolean().optional() }) export type SkillFrontmatterSummary = {