fix(skills): bound SSH discovery lifecycle

This commit is contained in:
Brennan Benson
2026-08-05 18:50:12 -07:00
parent 38b32a95e7
commit 17e2ea2d6a
8 changed files with 285 additions and 168 deletions
+33 -142
View File
@@ -1,8 +1,7 @@
import type { Dirent } from 'node:fs'
import { open, readdir, realpath, stat } from 'node:fs/promises'
import { readdir, realpath, stat } from 'node:fs/promises'
import { homedir } from 'node:os'
import { basename, dirname, isAbsolute, join, relative, sep } from 'node:path'
import { summarizeSkillMarkdown } from '../../shared/skill-metadata'
import { isAbsolute, join, relative, sep } from 'node:path'
import type { Repo } from '../../shared/types'
import {
SKILL_DISCOVERY_LIMITS,
@@ -10,22 +9,14 @@ import {
type SkillDiscoveryResult,
type SkillDiscoverySource
} from '../../shared/skills'
import {
buildSkillDiscoverySources,
compareSkills,
sourceKindForSkill,
sourceLabelForSkill,
stablePathId,
type SkillScanRoot
} from './skill-discovery-sources'
import { buildSkillDiscoverySources, compareSkills } from './skill-discovery-sources'
import { discoverClaudePluginSkillSources } from './claude-plugin-skill-sources'
import { runSkillCandidateTasks } from './skill-candidate-concurrency'
import { scanSkillFile, type ScannedSkill } from './skill-file-scanner'
export { buildSkillDiscoverySources } from './skill-discovery-sources'
const SKILL_FILE_NAME = 'SKILL.md'
const MAX_MARKDOWN_BYTES = 256 * 1024
const MAX_SKILL_FILES = 200
/** Node-version-safe alternative to signal.throwIfAborted(); an aborted scan
* must reject rather than resolve with partial results. */
@@ -61,13 +52,14 @@ function isWithinDepth(rootPath: string, childPath: string, maxDepth: number): b
async function findSkillFiles(
rootPath: string,
maxDepth: number,
budget: { remaining: number },
signal?: AbortSignal
): Promise<string[]> {
const out: string[] = []
const visitedDirectoryPaths = new Set<string>()
async function visit(dirPath: string): Promise<void> {
throwIfDiscoveryAborted(signal)
if (out.length >= SKILL_DISCOVERY_LIMITS.skills) {
if (budget.remaining <= 0) {
return
}
if (!isWithinDepth(rootPath, dirPath, maxDepth)) {
@@ -90,20 +82,26 @@ async function findSkillFiles(
} catch {
return
}
entries.sort((left, right) => (left.name < right.name ? -1 : left.name > right.name ? 1 : 0))
for (const entry of entries) {
throwIfDiscoveryAborted(signal)
if (out.length >= SKILL_DISCOVERY_LIMITS.skills) {
if (budget.remaining <= 0) {
return
}
const entryPath = join(dirPath, entry.name)
if (entry.name === SKILL_FILE_NAME) {
if (entry.isFile()) {
budget.remaining -= 1
out.push(entryPath)
continue
}
if (entry.isSymbolicLink()) {
try {
if ((await stat(entryPath)).isFile()) {
if (budget.remaining <= 0) {
return
}
budget.remaining -= 1
out.push(entryPath)
}
} catch {
@@ -133,120 +131,6 @@ async function findSkillFiles(
return out
}
async function countFiles(dirPath: string, signal?: AbortSignal): Promise<number> {
let count = 0
const visitedDirectoryPaths = new Set<string>()
async function visit(currentPath: string): Promise<void> {
throwIfDiscoveryAborted(signal)
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
updatedAt: number | null
} | null> {
try {
const fileStat = await stat(skillFilePath)
const file = await open(skillFilePath, 'r')
let content = ''
try {
const buffer = Buffer.alloc(Math.min(fileStat.size, MAX_MARKDOWN_BYTES))
const { bytesRead } = await file.read(buffer, 0, buffer.length, 0)
content = buffer.toString('utf8', 0, bytesRead)
} finally {
await file.close()
}
return {
...summarizeSkillMarkdown(content),
updatedAt: fileStat.mtimeMs
}
} catch {
return null
}
}
type ScannedSkill = DiscoveredSkill & { canonicalSkillFilePath: string }
async function scanRoot(root: SkillScanRoot, signal?: AbortSignal): Promise<ScannedSkill[]> {
const maxDepth = root.sourceKind === 'plugin' ? 9 : 4
const skillFiles = await findSkillFiles(root.path, maxDepth, signal)
const skills = await runSkillCandidateTasks(
skillFiles.map((skillFilePath) => async (): Promise<ScannedSkill | null> => {
throwIfDiscoveryAborted(signal)
// 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)
const directoryPath = dirname(skillFilePath)
const summary = await readSkillSummary(skillFilePath)
if (!summary) {
return null
}
const sourceKind = sourceKindForSkill(root, skillFilePath, { relative, sep })
return {
id: stablePathId(canonicalSkillFilePath),
name: (summary.name ?? basename(directoryPath)).slice(0, SKILL_DISCOVERY_LIMITS.nameLength),
description:
summary.description?.slice(0, SKILL_DISCOVERY_LIMITS.descriptionLength) ?? null,
// Copy: `root.providers` is shared across every skill/source from this
// root, so the dedup merge below must not mutate the aliased array.
providers: [...root.providers],
sourceKind,
sourceLabel: sourceLabelForSkill(root, sourceKind),
rootPath: root.path,
directoryPath,
skillFilePath,
installed: true,
fileCount: await countFiles(directoryPath, signal),
updatedAt: summary.updatedAt,
canonicalSkillFilePath
} satisfies ScannedSkill
})
)
return skills.filter((skill): skill is ScannedSkill => skill !== null)
}
export async function discoverSkills(args: {
repos?: Repo[]
homeDir?: string
@@ -267,26 +151,33 @@ export async function discoverSkills(args: {
]
.filter((root) => root.path.length <= SKILL_DISCOVERY_LIMITS.pathLength)
.slice(0, SKILL_DISCOVERY_LIMITS.sources)
const sources: SkillDiscoverySource[] = []
const skillGroups = await runSkillCandidateTasks(
const budget = { remaining: SKILL_DISCOVERY_LIMITS.skills }
const rootScans = await runSkillCandidateTasks(
roots.map((root) => async () => {
throwIfDiscoveryAborted(signal)
const exists = await pathExists(root.path)
sources.push({
...root,
providers: [...root.providers],
exists,
skippedReason: exists ? undefined : 'missing'
})
if (!exists) {
return []
}
return scanRoot(root, signal)
const maxDepth = root.sourceKind === 'plugin' ? 9 : 4
const skillFiles = exists ? await findSkillFiles(root.path, maxDepth, budget, signal) : []
return { root, exists, skillFiles }
})
)
throwIfDiscoveryAborted(signal)
const sources: SkillDiscoverySource[] = rootScans.map(({ root, exists }) => ({
...root,
providers: [...root.providers],
exists,
skippedReason: exists ? undefined : 'missing'
}))
const scannedSkills = (
await runSkillCandidateTasks(
rootScans.flatMap(({ root, skillFiles }) =>
skillFiles.map((skillFilePath) => () => scanSkillFile(root, skillFilePath, signal))
)
)
).filter((skill): skill is ScannedSkill => skill !== null)
throwIfDiscoveryAborted(signal)
const seen = new Map<string, DiscoveredSkill>()
for (const skill of skillGroups.flat()) {
for (const skill of scannedSkills) {
// 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.
@@ -147,4 +147,30 @@ describe('resolvePaneSkillDiscoveryWorkspace', () => {
})
).toEqual({ connectionId: 'target-1', cwd: '/remote/folder' })
})
it('isolates duplicate folder project groups using persisted workspace ownership', () => {
const workspace = {
id: 'folder-1',
projectGroupId: 'group-1',
folderPath: '/remote/folder',
connectionId: 'target-2'
} as FolderWorkspace
const group = (connectionId: string): ProjectGroup =>
({
id: 'group-1',
connectionId,
executionHostId: `ssh:${connectionId}`,
parentPath: '/remote/folder'
}) as ProjectGroup
expect(
resolvePaneSkillDiscoveryWorkspace({
worktreeId: 'folder:folder-1',
repos: [],
projectGroups: [group('target-1'), group('target-2')],
folderWorkspaces: [workspace],
sessions: []
})
).toEqual({ connectionId: 'target-2', cwd: '/remote/folder' })
})
})
@@ -48,14 +48,16 @@ export function resolvePaneSkillDiscoveryWorkspace(
if (!workspace) {
throw new Error('pane_skill_discovery_workspace_not_found')
}
const group = args.projectGroups.find((candidate) => candidate.id === workspace.projectGroupId)
const executionHostId = parseOptionalHostId(workspace.executionHostId ?? group?.executionHostId)
const connectionId = parseOptionalConnectionId(workspace.connectionId ?? group?.connectionId)
const connectionHostId = connectionId ? toSshExecutionHostId(connectionId) : null
assertMatchingHost(executionHostId, connectionHostId)
const workspaceHostId = declaredHostId(workspace)
const group = selectFolderProjectGroup(
args.projectGroups.filter((candidate) => candidate.id === workspace.projectGroupId),
sessionOwner?.hostId ?? workspaceHostId
)
const groupHostId = group ? declaredHostId(group) : null
assertMatchingHost(workspaceHostId, groupHostId)
const hostId =
executionHostId ??
connectionHostId ??
workspaceHostId ??
groupHostId ??
inferLegacyFolderHostId(workspace, args.projectGroups, args.repos)
assertMatchingHost(sessionOwner?.hostId, hostId)
return sshWorkspace(hostId, sessionOwner?.tab.startupCwd?.trim() || workspace.folderPath)
@@ -135,6 +137,37 @@ function selectFolderWorkspace(
return exact[0]
}
function selectFolderProjectGroup(
candidates: readonly ProjectGroup[],
hostId: ExecutionHostId | null | undefined
): ProjectGroup | null {
if (candidates.length === 0) {
return null
}
if (candidates.length === 1) {
return candidates[0]
}
if (!hostId) {
throw new Error('pane_skill_discovery_owner_ambiguous')
}
const exact = candidates.filter((group) => declaredHostId(group) === hostId)
if (exact.length !== 1) {
throw new Error('pane_skill_discovery_owner_ambiguous')
}
return exact[0]
}
function declaredHostId(owner: {
executionHostId?: string | null
connectionId?: string | null
}): ExecutionHostId | null {
const executionHostId = parseOptionalHostId(owner.executionHostId)
const connectionId = parseOptionalConnectionId(owner.connectionId)
const connectionHostId = connectionId ? toSshExecutionHostId(connectionId) : null
assertMatchingHost(executionHostId, connectionHostId)
return executionHostId ?? connectionHostId
}
function findPaneSession(
sessions: readonly HostSession[],
worktreeId: string,
+131
View File
@@ -0,0 +1,131 @@
import type { Dirent } from 'node:fs'
import { open, readdir, realpath, stat } from 'node:fs/promises'
import { basename, dirname, join, relative, sep } from 'node:path'
import { summarizeSkillMarkdown } from '../../shared/skill-metadata'
import { SKILL_DISCOVERY_LIMITS, type DiscoveredSkill } from '../../shared/skills'
import {
sourceKindForSkill,
sourceLabelForSkill,
stablePathId,
type SkillScanRoot
} from './skill-discovery-sources'
const MAX_MARKDOWN_BYTES = 256 * 1024
const MAX_SKILL_FILES = 200
export type ScannedSkill = DiscoveredSkill & { canonicalSkillFilePath: string }
function throwIfAborted(signal: AbortSignal | undefined): void {
if (signal?.aborted) {
const error = new Error('Skill discovery aborted')
error.name = 'AbortError'
throw error
}
}
async function countFiles(dirPath: string, signal?: AbortSignal): Promise<number> {
let count = 0
const visitedDirectoryPaths = new Set<string>()
async function visit(currentPath: string): Promise<void> {
throwIfAborted(signal)
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
updatedAt: number | null
} | null> {
try {
const fileStat = await stat(skillFilePath)
const file = await open(skillFilePath, 'r')
let content = ''
try {
const buffer = Buffer.alloc(Math.min(fileStat.size, MAX_MARKDOWN_BYTES))
const { bytesRead } = await file.read(buffer, 0, buffer.length, 0)
content = buffer.toString('utf8', 0, bytesRead)
} finally {
await file.close()
}
return { ...summarizeSkillMarkdown(content), updatedAt: fileStat.mtimeMs }
} catch {
return null
}
}
export async function scanSkillFile(
root: SkillScanRoot,
skillFilePath: string,
signal?: AbortSignal
): Promise<ScannedSkill | null> {
throwIfAborted(signal)
const canonicalSkillFilePath = await realpath(skillFilePath).catch(() => skillFilePath)
const directoryPath = dirname(skillFilePath)
if (
directoryPath.length > SKILL_DISCOVERY_LIMITS.pathLength ||
skillFilePath.length > SKILL_DISCOVERY_LIMITS.pathLength
) {
return null
}
const summary = await readSkillSummary(skillFilePath)
if (!summary) {
return null
}
const sourceKind = sourceKindForSkill(root, skillFilePath, { relative, sep })
return {
id: stablePathId(canonicalSkillFilePath),
name: (summary.name ?? basename(directoryPath)).slice(0, SKILL_DISCOVERY_LIMITS.nameLength),
description: summary.description?.slice(0, SKILL_DISCOVERY_LIMITS.descriptionLength) ?? null,
providers: [...root.providers],
sourceKind,
sourceLabel: sourceLabelForSkill(root, sourceKind),
rootPath: root.path,
directoryPath,
skillFilePath,
installed: true,
fileCount: await countFiles(directoryPath, signal),
updatedAt: summary.updatedAt,
canonicalSkillFilePath
}
}
@@ -42,7 +42,6 @@ export type NativeChatSkillDiscoveryContext =
}
| {
key: string
cwd: string
executionHostKind: 'ssh'
runtimeTarget: RuntimeClientTarget
/** Identity only; the owning runtime derives the scanned directory. */
@@ -99,18 +98,6 @@ export function resolveNativeChatSkillDiscoveryContext(
if (!worktreeId) {
return null
}
const workspaceScope = parseWorkspaceKey(worktreeId)
const cwd =
resolveNativeChatSkillDiscoveryCwd(state, terminalTabId) ??
(workspaceScope?.type === 'folder'
? state.folderWorkspaces.find(
(workspace) => workspace.id === workspaceScope.folderWorkspaceId
)?.folderPath
: null)
if (!cwd) {
return null
}
const hostId = getExecutionHostIdForWorktree(state, worktreeId)
const parsedHost = parseExecutionHostId(hostId)
if (parsedHost?.kind === 'ssh') {
@@ -134,11 +121,9 @@ export function resolveNativeChatSkillDiscoveryContext(
runtimeEnvironmentId ?? null,
hostId,
connectionState?.connectionGeneration ?? 0,
cwd,
worktreeId,
terminalTabId
]),
cwd,
executionHostKind: 'ssh',
runtimeTarget: runtimeEnvironmentId
? { kind: 'environment', environmentId: runtimeEnvironmentId }
@@ -148,6 +133,18 @@ export function resolveNativeChatSkillDiscoveryContext(
}
}
const workspaceScope = parseWorkspaceKey(worktreeId)
const cwd =
resolveNativeChatSkillDiscoveryCwd(state, terminalTabId) ??
(workspaceScope?.type === 'folder'
? state.folderWorkspaces.find(
(workspace) => workspace.id === workspaceScope.folderWorkspaceId
)?.folderPath
: null)
if (!cwd) {
return null
}
const runtimeEnvironmentId = getExplicitRuntimeEnvironmentIdForWorktree(state, worktreeId)
// Why: a selected global runtime is not proof that it owns this pane. Modern
// panes carry an owner stamp; ambiguous legacy panes stay not-ready.
@@ -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'
@@ -224,4 +224,28 @@ describe('useNativeChatSkills', () => {
expect(mocks.snapshots.at(-1)?.status).toBe('ready')
})
})
it('does not cache a canceled reconnect generation', async () => {
const pending: ((value: { status: 'ok'; result: typeof DISCOVERY_RESULT }) => void)[] = []
mocks.state = connectedSshState(1)
mocks.callRuntimeRpc.mockImplementation(
() =>
new Promise((resolve) => {
pending.push(resolve)
})
)
const view = render(<Probe enabled />)
await waitFor(() => expect(mocks.callRuntimeRpc).toHaveBeenCalledTimes(1))
mocks.state = connectedSshState(2)
view.rerender(<Probe enabled />)
await waitFor(() => expect(mocks.callRuntimeRpc).toHaveBeenCalledTimes(2))
await act(async () => pending[1]({ status: 'ok', result: DISCOVERY_RESULT }))
await waitFor(() => expect(mocks.snapshots.at(-1)?.status).toBe('ready'))
await act(async () => pending[0]({ status: 'ok', result: DISCOVERY_RESULT }))
mocks.state = connectedSshState(1)
view.rerender(<Probe enabled />)
await waitFor(() => expect(mocks.callRuntimeRpc).toHaveBeenCalledTimes(3))
})
})
@@ -325,6 +325,23 @@ describe('resolveNativeChatSkillDiscoveryContext for SSH panes', () => {
expect(context?.executionHostKind === 'ssh' && context.sshDisconnected).toBe(false)
})
it('uses pane identity when the SSH worktree catalog has not hydrated a cwd', () => {
const worktreeId = 'repo-1::/repo/worktree'
const context = resolveNativeChatSkillDiscoveryContext(
sshInputs({
tabsByWorktree: { [worktreeId]: [{ id: 'tab-1' }] },
worktreesByRepo: {}
}),
'tab-1'
)
expect(context?.executionHostKind).toBe('ssh')
if (context?.executionHostKind !== 'ssh') {
throw new Error('expected ssh context')
}
expect(context.paneTarget).toEqual({ worktreeId, terminalTabId: 'tab-1' })
})
it('keeps the same remote path on two SSH hosts cache-isolated', () => {
const onTargetTwo = resolveNativeChatSkillDiscoveryContext(
sshInputs({
@@ -341,7 +358,6 @@ describe('resolveNativeChatSkillDiscoveryContext for SSH panes', () => {
'tab-1'
)
const onTargetOne = resolveNativeChatSkillDiscoveryContext(sshInputs(), 'tab-1')
expect(onTargetOne?.cwd).toBe(onTargetTwo?.cwd)
expect(onTargetOne?.key).not.toBe(onTargetTwo?.key)
})
@@ -358,7 +374,6 @@ describe('resolveNativeChatSkillDiscoveryContext for SSH panes', () => {
}),
'tab-2'
)
expect(firstPane?.cwd).toBe(secondPane?.cwd)
expect(firstPane?.key).not.toBe(secondPane?.key)
})
@@ -146,10 +146,10 @@ export function useNativeChatSkills(
const request = getOrStartDiscovery(context)
void request.then(
(result) => {
writePaneDiscoveryCache(paneDiscoveryCache.current, paneCacheKey, result)
if (cancelled) {
return
}
writePaneDiscoveryCache(paneDiscoveryCache.current, paneCacheKey, result)
emitNativeChatSkillDiscovery({
agent,
outcome: 'ready',