fix(skills): start only the unread distros when Share retries discovery (#35725)

The share RPC's fallback now reads stopped roots only on the distros its first pass left unread, the directory matcher refuses an empty root, and the lowercasing is documented as widen-only.
This commit is contained in:
Neil
2026-10-28 23:14:52 -07:00
committed by GitHub
parent 571eb885ac
commit 682ef86dfe
6 changed files with 144 additions and 23 deletions
+44 -5
View File
@@ -9,6 +9,11 @@ vi.mock('../../../skills/skill-discovery-target', () => ({
resolveSkillDiscoveryTarget: vi.fn((target) => ({ kind: 'native-host', cwd: target?.cwd })),
discoverSkillsOnTarget: vi.fn(async () => ({ skills: [], sources: [], scannedAt: 1 }))
}))
// Off Windows the real gate never sees a distro; stand in for its UNC parse.
vi.mock('../../../wsl/wsl-unc-access-gate', async (importOriginal) => ({
...(await importOriginal<Record<string, unknown>>()),
wslDistroForPath: (path: string) => /^\\\\wsl\.localhost\\([^\\]+)/.exec(path)?.[1] ?? null
}))
import { SKILL_METHODS } from './skills'
import {
discoverSkillsOnTarget,
@@ -345,13 +350,23 @@ describe('skills.share RPC', () => {
expect(publishDiscoveredSkillsFromAgent).toHaveBeenCalledWith(request, discoveredSkills, signal)
})
it('reads stopped distros only when a selector is missing and a folder went unread', async () => {
const stoppedSource = { id: 'wsl-root', sourceKind: 'repo', skippedReason: 'unavailable' }
it('reads only the unread distros when a selector is missing', async () => {
const unavailable = (path: string) => ({
id: path,
path,
sourceKind: 'repo',
skippedReason: 'unavailable'
})
vi.mocked(discoverSkillsOnTarget)
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the handler reads only skill ids and source skippedReason.
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the handler reads only skill ids and source skippedReason and path.
.mockResolvedValueOnce({
skills: [{ id: 'alpha' }],
sources: [stoppedSource],
sources: [
unavailable('\\\\wsl.localhost\\Ubuntu\\home\\me\\.claude\\skills'),
unavailable('\\\\wsl.localhost\\Ubuntu\\repo\\.claude\\skills'),
// A host folder that failed to read starts no distro.
unavailable('C:\\Users\\me\\.claude\\skills')
],
scannedAt: 1
} as never)
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the handler reads only skill ids and source skippedReason.
@@ -374,7 +389,7 @@ describe('skills.share RPC', () => {
expect(vi.mocked(discoverSkillsOnTarget).mock.calls.map((call) => call[2])).toEqual([
{ providerRootOverrides: {}, refresh: true },
{ providerRootOverrides: {}, refresh: true, readStoppedWslRootsFor: 'all' }
{ providerRootOverrides: {}, refresh: true, readStoppedWslRootsFor: { distros: ['Ubuntu'] } }
])
expect(publishDiscoveredSkillsFromAgent).toHaveBeenCalledWith(
request,
@@ -383,6 +398,30 @@ describe('skills.share RPC', () => {
)
})
it('starts no distro when the only unread folders are on the host', async () => {
vi.mocked(discoverSkillsOnTarget)
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the handler reads only skill ids and source skippedReason and path.
.mockResolvedValueOnce({
skills: [{ id: 'alpha' }],
sources: [
{ id: 'host', path: 'C:\\skills', sourceKind: 'home', skippedReason: 'unavailable' }
],
scannedAt: 1
} as never)
const runtime = {
assertAgentSkillSharingAllowed: vi.fn(),
resolveProjectRuntimeForWorktree: vi.fn(),
resolveSkillDiscoveryProviderRoots: vi.fn(async () => ({})),
listRepos: vi.fn(() => []),
publishDiscoveredSkillsFromAgent: vi.fn(async () => ({ status: 'ok' }))
}
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: skills.share reads only these runtime members and the signal.
await method('skills.share').handler(request, { runtime } as unknown as RpcContext)
expect(discoverSkillsOnTarget).toHaveBeenCalledTimes(1)
})
it('rejects WSL before attempting to read Linux paths with native fs', async () => {
vi.mocked(resolveSkillDiscoveryTarget).mockResolvedValueOnce({
kind: 'wsl',
+12 -5
View File
@@ -160,7 +160,7 @@ export const SKILL_METHODS = [
)
}
// Sharing named skills is a user act: it may start the workspace's distro.
const discover = async (readStoppedWslRootsFor?: 'all') =>
const discover = async (readStoppedWslRootsFor?: { distros: readonly string[] }) =>
runAsWslUserAct(
resolvedTarget.cwd ? wslDistroForPath(resolvedTarget.cwd) : null,
async () =>
@@ -171,15 +171,22 @@ export const SKILL_METHODS = [
...(readStoppedWslRootsFor ? { readStoppedWslRootsFor } : {})
})
)
// Selectors name skills, not folders: start other stopped distros only when a selector is missing.
// Selectors name skills, not folders: when one is missing, start only the distros left unread.
let discovered = await discover()
const unreadDistros = [
...new Set(
discovered.sources.flatMap((source) =>
source.skippedReason === 'unavailable' ? (wslDistroForPath(source.path) ?? []) : []
)
)
]
if (
unreadDistros.length > 0 &&
!params.skillSelectors.every((selector) =>
discovered.skills.some((skill) => skill.id === selector || skill.name === selector)
) &&
discovered.sources.some((source) => source.skippedReason === 'unavailable')
)
) {
discovered = await discover('all')
discovered = await discover({ distros: unreadDistros })
}
return runtime.publishDiscoveredSkillsFromAgent(params, discovered.skills, signal)
}
+6 -4
View File
@@ -25,7 +25,8 @@ import {
splitStoppedWslDistroRoots,
stoppedPluginRoot,
stoppedRootReader,
unavailableSkillSource
unavailableSkillSource,
type StoppedWslRootsReadScope
} from './skill-discovery-stopped-wsl-roots'
import { wslDistroForPath } from '../wsl/wsl-unc-access-gate'
import { buildRemoteRepoSkillSources } from './skill-remote-repo-sources'
@@ -236,10 +237,11 @@ export async function discoverSkills(args: {
names?: string[]
sourceKinds?: SkillSourceKind[]
/**
* A user act (Share) may read stopped WSL distros' folders: 'all', or only the roots holding these
* skill directories. Every other stopped root is reported unavailable, so its distro stays down.
* A user act (Share) may read stopped WSL distros' folders: 'all', those on named distros, or only
* the roots holding these skill directories. Every other stopped root is reported unavailable, so
* its distro stays down.
*/
readStoppedWslRootsFor?: 'all' | readonly string[]
readStoppedWslRootsFor?: StoppedWslRootsReadScope
}): Promise<SkillDiscoveryResult> {
args.signal?.throwIfAborted()
const startedAt = Date.now()
@@ -1,6 +1,53 @@
import { describe, expect, it } from 'vitest'
import { describe, expect, it, vi } from 'vitest'
import { WslDistroStoppedError, WslDistroUnverifiableError } from '../wsl/wsl-unc-access-gate'
import { unavailableWhileWslDistroStopped } from './skill-discovery-stopped-wsl-roots'
import {
stoppedRootReader,
stoppedWslRootsReadScopeKey,
unavailableWhileWslDistroStopped
} from './skill-discovery-stopped-wsl-roots'
// Off Windows the real gate never sees a distro; stand in for its UNC parse.
vi.mock('../wsl/wsl-unc-access-gate', async (importOriginal) => ({
...(await importOriginal<Record<string, unknown>>()),
wslDistroForPath: (path: string) => /^\\\\wsl\.localhost\\([^\\]+)/.exec(path)?.[1] ?? null
}))
const ubuntu = (rest: string): string => `\\\\wsl.localhost\\Ubuntu${rest}`
describe('stoppedRootReader', () => {
it('reads a root that holds a chosen skill directory, and no other', () => {
const read = stoppedRootReader([ubuntu('\\home\\me\\.claude\\skills\\alpha')])
expect(read(ubuntu('\\home\\me\\.claude\\skills'))).toBe(true)
expect(read(ubuntu('\\HOME\\me\\.claude\\skills\\'))).toBe(true)
expect(read(ubuntu('\\repo\\.claude\\skills'))).toBe(false)
})
it('never lets an empty, `/` or bare share root match every directory', () => {
const read = stoppedRootReader([ubuntu('\\home\\me\\.claude\\skills\\alpha')])
expect(read('')).toBe(false)
expect(read('/')).toBe(false)
expect(read('\\\\')).toBe(false)
})
it('reads every root on the named distros, matching names case-insensitively', () => {
const read = stoppedRootReader({ distros: ['ubuntu'] })
expect(read(ubuntu('\\home\\me\\.claude\\plugins\\x\\skills'))).toBe(true)
expect(read('\\\\wsl.localhost\\Debian\\home\\me\\.claude\\skills')).toBe(false)
expect(read('C:\\Users\\me\\.claude\\skills')).toBe(false)
})
it('keys each scope apart for the scan coalescer', () => {
const keys = [
stoppedWslRootsReadScopeKey('all'),
stoppedWslRootsReadScopeKey({ distros: ['Ubuntu'] }),
stoppedWslRootsReadScopeKey(['Ubuntu'])
]
expect(new Set(keys).size).toBe(3)
expect(stoppedWslRootsReadScopeKey({ distros: ['Ubuntu', 'Debian'] })).toBe(
stoppedWslRootsReadScopeKey({ distros: ['debian', 'ubuntu'] })
)
})
})
describe('unavailableWhileWslDistroStopped', () => {
it.each([new WslDistroStoppedError('Ubuntu'), new WslDistroUnverifiableError('Ubuntu')])(
@@ -49,6 +49,7 @@ export async function splitStoppedWslDistroRoots(
}
}
// Lowercasing can only widen the user-act-readable roots: Windows treats these UNC paths case-insensitively.
function comparablePath(path: string): string {
return path
.replace(/[\\/]+/g, '/')
@@ -56,16 +57,39 @@ function comparablePath(path: string): string {
.toLowerCase()
}
/** Whether a stopped-distro root or plugin cwd at `path` holds one of the chosen skill directories. */
/** Stopped WSL roots a user act may read: every one, those on named distros, or those holding chosen skill directories. */
export type StoppedWslRootsReadScope = 'all' | { distros: readonly string[] } | readonly string[]
/** Distinguishes scans with different read scopes in the scan coalescer. */
export function stoppedWslRootsReadScopeKey(scope: StoppedWslRootsReadScope): string {
if (scope === 'all') {
return 'all'
}
if ('distros' in scope) {
const distros = scope.distros.map((distro) => distro.toLowerCase())
return `distros:${stablePathId([...distros].sort().join('\0'))}`
}
return stablePathId([...scope].sort().join('\0'))
}
/** Whether a stopped-distro root or plugin cwd at `path` falls in the user act's read scope. */
export function stoppedRootReader(
readFor: 'all' | readonly string[] | undefined
readFor: StoppedWslRootsReadScope | undefined
): (path: string) => boolean {
if (readFor === 'all') {
return () => true
}
if (readFor !== undefined && 'distros' in readFor) {
const distros = new Set(readFor.distros.map((distro) => distro.toLowerCase()))
return (path) => distros.has(wslDistroForPath(path)?.toLowerCase() ?? '')
}
const directories = (readFor ?? []).map(comparablePath)
return (path) => {
const root = comparablePath(path)
// `/` or `\\` normalizes to '', which would prefix every directory.
if (root === '') {
return false
}
return directories.some((directory) => directory === root || directory.startsWith(`${root}/`))
}
}
+7 -5
View File
@@ -13,6 +13,10 @@ import { skillScanSourceKinds } from './skill-discovery-source-filter'
import { getRepoExecutionHostId } from '../../shared/execution-host'
import { isSkillRootUnavailableError, SkillScanCoalescer } from './skill-scan-coalescer'
import { assertWslUncPathRunning } from '../wsl/wsl-unc-access-gate'
import {
stoppedWslRootsReadScopeKey,
type StoppedWslRootsReadScope
} from './skill-discovery-stopped-wsl-roots'
// Cache whole WSL results to amortize process boots; native roots already cache, so only coalesce concurrent targets.
const WSL_RESULT_TTL_MS = 10_000
@@ -156,17 +160,15 @@ export async function discoverSkillsOnTarget(
options: {
refresh?: boolean
providerRootOverrides?: SkillProviderRootOverrides
/** A user act (Share) may read stopped distros' folders: 'all', or only those holding these skill directories. */
readStoppedWslRootsFor?: 'all' | readonly string[]
/** A user act (Share) may read stopped distros' folders; see `StoppedWslRootsReadScope`. */
readStoppedWslRootsFor?: StoppedWslRootsReadScope
} = {}
): Promise<SkillDiscoveryResult> {
const refresh = options.refresh === true
const readFor = options.readStoppedWslRootsFor
const gate = readFor ? { readStoppedWslRootsFor: readFor } : {}
const userActKey =
readFor === undefined
? ''
: `:user-act:${readFor === 'all' ? 'all' : stablePathId([...readFor].sort().join('\0'))}`
readFor === undefined ? '' : `:user-act:${stoppedWslRootsReadScopeKey(readFor)}`
try {
const outcome = await targetScans.run(
// A gated and an ungated scan of one target must not answer for each other.