From cf152841201aba265457c191fdbe8f200ec9713d Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 28 Oct 2026 21:57:25 -0700 Subject: [PATCH] fix(wsl): Share only starts the stopped distros that hold the chosen skills (#35659) #34632 let Share read every stopped WSL distro's skill folders. The Share dialog now sends the chosen skills' folders, and discovery reads only the stopped roots (and workspace plugin folder) that hold them; every other stopped root stays unread and unavailable. The skills.share RPC names skills, not folders, so it scans gated first and reads stopped roots only when a selector is missing and a folder went unread. --- src/main/ipc/skill-cloud-ipc-handlers.ts | 15 ++++--- .../ipc/skill-share-publishing-ipc-schemas.ts | 2 + src/main/ipc/skills.test.ts | 16 ++++--- src/main/ipc/skills.ts | 10 +++-- src/main/runtime/rpc/methods/skills.test.ts | 42 ++++++++++++++++++- src/main/runtime/rpc/methods/skills.ts | 30 +++++++++---- .../discovery-wsl-stopped-distro.test.ts | 30 ++++++++++++- src/main/skills/discovery.ts | 14 +++++-- .../skill-discovery-stopped-wsl-roots.ts | 27 +++++++++++- src/main/skills/skill-discovery-target.ts | 13 ++++-- src/preload/api/agent-skill-api.ts | 1 + src/preload/api/skills-bridge.ts | 1 + .../skills/SkillShareDialog.test.tsx | 3 ++ .../components/skills/SkillShareDialog.tsx | 2 + 14 files changed, 169 insertions(+), 37 deletions(-) diff --git a/src/main/ipc/skill-cloud-ipc-handlers.ts b/src/main/ipc/skill-cloud-ipc-handlers.ts index 06924e71aa6..fc2b46115ec 100644 --- a/src/main/ipc/skill-cloud-ipc-handlers.ts +++ b/src/main/ipc/skill-cloud-ipc-handlers.ts @@ -37,10 +37,13 @@ import { } from './skill-share-publishing-ipc-schemas' import { getRuntimeEnvironmentRegistryRoot } from './runtime-environments/runtime-environment-registry-root' -function registerSharingHandlers( - runtime: OrcaRuntimeService, - discover: (target?: z.infer) => Promise -): void { +/** Discovery for Share; `skillDirectories` limits which stopped WSL roots it may read. */ +export type SkillShareDiscover = ( + target?: z.infer, + skillDirectories?: readonly string[] +) => Promise + +function registerSharingHandlers(runtime: OrcaRuntimeService, discover: SkillShareDiscover): void { const preparations = new SkillSharePreparationService( join(app.getPath('userData'), 'skill-share-preparations'), { @@ -52,7 +55,7 @@ function registerSharingHandlers( ) handleMainWindowSkillIpc('skills:prepareShare', async (_event, value: unknown) => { const input = skillSharePrepareIpcSchema.parse(value) - const result = await discover(input.target) + const result = await discover(input.target, input.skillDirectories) const requested = new Set(input.skillIds) const skills = result.skills.filter((candidate) => requested.has(candidate.id)) if (skills.length !== requested.size) { @@ -289,7 +292,7 @@ function registerCloudInstallHandlers(runtime: OrcaRuntimeService): void { export function registerSkillCloudIpcHandlers( runtime: OrcaRuntimeService, - discover: (target?: z.infer) => Promise + discover: SkillShareDiscover ): void { registerSharingHandlers(runtime, discover) registerCloudInstallHandlers(runtime) diff --git a/src/main/ipc/skill-share-publishing-ipc-schemas.ts b/src/main/ipc/skill-share-publishing-ipc-schemas.ts index 7caf1f8b4ac..7f2a8b72e23 100644 --- a/src/main/ipc/skill-share-publishing-ipc-schemas.ts +++ b/src/main/ipc/skill-share-publishing-ipc-schemas.ts @@ -4,6 +4,8 @@ import { SkillDiscoveryTargetSchema } from '../../shared/skills' export const skillSharePrepareIpcSchema = z .object({ skillIds: z.array(z.string().min(1).max(4096)).min(1).max(512), + /** The chosen skills' folders: only their roots may start a stopped WSL distro. */ + skillDirectories: z.array(z.string().min(1).max(4096)).max(512).optional(), bundleName: z.string().regex(/^(?!.*(?:--|\.\.))[a-z0-9](?:[a-z0-9.-]{0,62}[a-z0-9])?$/), target: SkillDiscoveryTargetSchema.optional(), packageId: z.string().min(1).max(128).optional() diff --git a/src/main/ipc/skills.test.ts b/src/main/ipc/skills.test.ts index aeed666f93b..a818336f250 100644 --- a/src/main/ipc/skills.test.ts +++ b/src/main/ipc/skills.test.ts @@ -230,14 +230,20 @@ describe('registerSkillsHandlers', () => { expect(discoverSkillObservationInWslMock).toHaveBeenCalled() }) - it('lets the Share dialog, which sends no target, read a stopped distro’s repo skills', async () => { + it('lets the Share dialog, which sends no target, read only its chosen skills’ stopped roots', async () => { registerSkillsHandlers(store as never, bundledRuntime() as never) // oxlint-disable-line typescript/consistent-type-assertions -- SAFETY: registration reads only getRepos and the runtime's provider roots. const shareDiscover = registerSkillCloudIpcHandlersMock.mock.lastCall?.[1] + const chosen = ['\\\\wsl.localhost\\Ubuntu\\home\\me\\repo\\.agents\\skills\\a'] - await shareDiscover(undefined) - + await shareDiscover(undefined, chosen) expect(discoverSkillsMock).toHaveBeenLastCalledWith( - expect.objectContaining({ repos, gateStoppedWslDistros: false }) + expect.objectContaining({ repos, readStoppedWslRootsFor: chosen }) + ) + + // A caller that cannot name folders keeps the earlier behavior. + await shareDiscover(undefined) + expect(discoverSkillsMock).toHaveBeenLastCalledWith( + expect.objectContaining({ repos, readStoppedWslRootsFor: 'all' }) ) }) @@ -245,7 +251,7 @@ describe('registerSkillsHandlers', () => { const handler = getDiscoverHandler() await handler(null, undefined) expect(discoverSkillsMock).toHaveBeenLastCalledWith( - expect.not.objectContaining({ gateStoppedWslDistros: false }) + expect.not.objectContaining({ readStoppedWslRootsFor: expect.anything() }) ) }) diff --git a/src/main/ipc/skills.ts b/src/main/ipc/skills.ts index 06fd894fa49..31bf4d87614 100644 --- a/src/main/ipc/skills.ts +++ b/src/main/ipc/skills.ts @@ -24,12 +24,12 @@ export function registerSkillsHandlers(store: Store, runtime?: OrcaRuntimeServic const discoverResolved = async ( resolvedTarget: Awaited>, refresh: boolean, - userAct = false + readStoppedWslRootsFor?: 'all' | readonly string[] ): Promise => discoverSkillsOnTarget(resolvedTarget, store.getRepos(), { providerRootOverrides: await runtime?.resolveSkillDiscoveryProviderRoots(resolvedTarget), refresh, - ...(userAct ? { gateStoppedWslDistros: false } : {}) + ...(readStoppedWslRootsFor ? { readStoppedWslRootsFor } : {}) }) // Passive UI mounts discovery, so it must not start a stopped WSL distro. const discover = async (target?: SkillDiscoveryTarget): Promise => { @@ -45,7 +45,8 @@ export function registerSkillsHandlers(store: Store, runtime?: OrcaRuntimeServic } // Share is a click on named skills: it may start their distro, as any user act may. const discoverForUserAct = async ( - target?: SkillDiscoveryTarget + target?: SkillDiscoveryTarget, + skillDirectories?: readonly string[] ): Promise => { const parsedTarget = target ? SkillDiscoveryTargetSchema.parse(target) : undefined const resolvedTarget = await resolveSkillDiscoveryTarget(parsedTarget) @@ -56,7 +57,8 @@ export function registerSkillsHandlers(store: Store, runtime?: OrcaRuntimeServic ? wslDistroForPath(resolvedTarget.cwd) : null return runAsWslUserAct(distro, () => - discoverResolved(resolvedTarget, parsedTarget?.refresh === true, true) + // Only the chosen skills' roots may start a stopped distro; without folders, any may. + discoverResolved(resolvedTarget, parsedTarget?.refresh === true, skillDirectories ?? 'all') ) } diff --git a/src/main/runtime/rpc/methods/skills.test.ts b/src/main/runtime/rpc/methods/skills.test.ts index 507ee86a2d2..43546d2bb9a 100644 --- a/src/main/runtime/rpc/methods/skills.test.ts +++ b/src/main/runtime/rpc/methods/skills.test.ts @@ -338,11 +338,51 @@ describe('skills.share RPC', () => { expect(discoverSkillsOnTarget).toHaveBeenCalledWith( expect.objectContaining({ kind: 'native-host', cwd: '/repo' }), [], - { providerRootOverrides: {}, refresh: true, gateStoppedWslDistros: false } + { providerRootOverrides: {}, refresh: true } ) + // Every selector was found without starting any stopped distro. + expect(discoverSkillsOnTarget).toHaveBeenCalledTimes(1) 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' } + vi.mocked(discoverSkillsOnTarget) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the handler reads only skill ids and source skippedReason. + .mockResolvedValueOnce({ + skills: [{ id: 'alpha' }], + sources: [stoppedSource], + scannedAt: 1 + } as never) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the handler reads only skill ids and source skippedReason. + .mockResolvedValueOnce({ + skills: [{ id: 'alpha' }, { id: 'beta' }], + sources: [], + scannedAt: 2 + } as never) + const publishDiscoveredSkillsFromAgent = vi.fn(async () => ({ status: 'ok' })) + const runtime = { + assertAgentSkillSharingAllowed: vi.fn(), + resolveProjectRuntimeForWorktree: vi.fn(), + resolveSkillDiscoveryProviderRoots: vi.fn(async () => ({})), + listRepos: vi.fn(() => []), + publishDiscoveredSkillsFromAgent + } + + // 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(vi.mocked(discoverSkillsOnTarget).mock.calls.map((call) => call[2])).toEqual([ + { providerRootOverrides: {}, refresh: true }, + { providerRootOverrides: {}, refresh: true, readStoppedWslRootsFor: 'all' } + ]) + expect(publishDiscoveredSkillsFromAgent).toHaveBeenCalledWith( + request, + [{ id: 'alpha' }, { id: 'beta' }], + undefined + ) + }) + it('rejects WSL before attempting to read Linux paths with native fs', async () => { vi.mocked(resolveSkillDiscoveryTarget).mockResolvedValueOnce({ kind: 'wsl', diff --git a/src/main/runtime/rpc/methods/skills.ts b/src/main/runtime/rpc/methods/skills.ts index 3dbafd5a51a..a5bf89b2c67 100644 --- a/src/main/runtime/rpc/methods/skills.ts +++ b/src/main/runtime/rpc/methods/skills.ts @@ -160,15 +160,27 @@ export const SKILL_METHODS = [ ) } // Sharing named skills is a user act: it may start the workspace's distro. - const discovered = await runAsWslUserAct( - resolvedTarget.cwd ? wslDistroForPath(resolvedTarget.cwd) : null, - async () => - discoverSkillsOnTarget(resolvedTarget, runtime.listRepos(), { - providerRootOverrides: await runtime.resolveSkillDiscoveryProviderRoots(resolvedTarget), - refresh: true, - gateStoppedWslDistros: false - }) - ) + const discover = async (readStoppedWslRootsFor?: 'all') => + runAsWslUserAct( + resolvedTarget.cwd ? wslDistroForPath(resolvedTarget.cwd) : null, + async () => + discoverSkillsOnTarget(resolvedTarget, runtime.listRepos(), { + providerRootOverrides: + await runtime.resolveSkillDiscoveryProviderRoots(resolvedTarget), + refresh: true, + ...(readStoppedWslRootsFor ? { readStoppedWslRootsFor } : {}) + }) + ) + // Selectors name skills, not folders: start other stopped distros only when a selector is missing. + let discovered = await discover() + if ( + !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') + } return runtime.publishDiscoveredSkillsFromAgent(params, discovered.skills, signal) } }), diff --git a/src/main/skills/discovery-wsl-stopped-distro.test.ts b/src/main/skills/discovery-wsl-stopped-distro.test.ts index fbf6e01e46f..d6ed889c12e 100644 --- a/src/main/skills/discovery-wsl-stopped-distro.test.ts +++ b/src/main/skills/discovery-wsl-stopped-distro.test.ts @@ -132,13 +132,39 @@ describe('skill discovery for a stopped WSL distro (E9)', () => { ) }) - it('lets a user act (Share) read a stopped distro’s repo folders and find its skill', async () => { + it('lets Share read only the stopped roots holding the chosen skills, leaving the rest down', async () => { + const chosen = join(mocks.wslRepoRoot, '.agents', 'skills', 'repo-skill') const result = await discoverSkills({ homeDir: join(root, 'home'), repos: [repo('wsl', mocks.wslRepoRoot), repo('host', hostRepo)], includeCwd: false, sourceKinds: ['repo'], - gateStoppedWslDistros: false + readStoppedWslRootsFor: [chosen] + }) + expect(result.skills.map((skill) => skill.name).sort()).toEqual([ + 'Skill in host', + 'Skill in wsl' + ]) + const chosenRoot = join(mocks.wslRepoRoot, '.agents', 'skills') + // The chosen skill's root is read; the distro's other roots stay unread and unavailable. + const otherWslTouches = mocks.touched.filter( + (path) => path.startsWith(mocks.wslRepoRoot) && !path.startsWith(chosenRoot) + ) + expect(otherWslTouches).toEqual([]) + const otherWslSources = result.sources.filter( + (source) => source.path.startsWith(mocks.wslRepoRoot) && source.path !== chosenRoot + ) + expect(otherWslSources.length).toBeGreaterThan(0) + expect(otherWslSources.every((source) => source.skippedReason === 'unavailable')).toBe(true) + }) + + it("lets Share read every stopped root when it can't name the skills' folders", async () => { + const result = await discoverSkills({ + homeDir: join(root, 'home'), + repos: [repo('wsl', mocks.wslRepoRoot), repo('host', hostRepo)], + includeCwd: false, + sourceKinds: ['repo'], + readStoppedWslRootsFor: 'all' }) expect(mocks.blockedWslDistroForPath).not.toHaveBeenCalled() expect(result.skills.map((skill) => skill.name).sort()).toEqual([ diff --git a/src/main/skills/discovery.ts b/src/main/skills/discovery.ts index a97f66c28c8..ad9e05be752 100644 --- a/src/main/skills/discovery.ts +++ b/src/main/skills/discovery.ts @@ -24,6 +24,7 @@ import { isOnStoppedWslDistro, splitStoppedWslDistroRoots, stoppedPluginRoot, + stoppedRootReader, unavailableSkillSource } from './skill-discovery-stopped-wsl-roots' import { wslDistroForPath } from '../wsl/wsl-unc-access-gate' @@ -234,8 +235,11 @@ export async function discoverSkills(args: { refresh?: boolean names?: string[] sourceKinds?: SkillSourceKind[] - /** False for a user act (Share): read stopped WSL distros' folders instead of reporting them unavailable. */ - gateStoppedWslDistros?: boolean + /** + * 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. + */ + readStoppedWslRootsFor?: 'all' | readonly string[] }): Promise { args.signal?.throwIfAborted() const startedAt = Date.now() @@ -248,10 +252,12 @@ export async function discoverSkills(args: { (!args.sourceKinds?.length || args.sourceKinds.includes('plugin')) ? args.cwd : undefined - const gateStopped = args.gateStoppedWslDistros !== false + const gateStopped = args.readStoppedWslRootsFor !== 'all' + const mayReadStopped = stoppedRootReader(args.readStoppedWslRootsFor) const pluginCwdStopped = gateStopped && pluginCwd !== undefined && + !mayReadStopped(pluginCwd) && wslDistroForPath(pluginCwd) !== null && (await isOnStoppedWslDistro(pluginCwd)) const candidateRoots = [ @@ -266,7 +272,7 @@ export async function discoverSkills(args: { // Host-only root sets never await the WSL gate. const { readable, unavailable } = gateStopped && candidateRoots.some((root) => wslDistroForPath(root.path)) - ? await splitStoppedWslDistroRoots(candidateRoots) + ? await splitStoppedWslDistroRoots(candidateRoots, mayReadStopped) : { readable: candidateRoots, unavailable: [] } const roots = readable.filter((root) => rootMayContainSourceKind(root, args.sourceKinds)) args.signal?.throwIfAborted() diff --git a/src/main/skills/skill-discovery-stopped-wsl-roots.ts b/src/main/skills/skill-discovery-stopped-wsl-roots.ts index 0e210fd9a8e..aa38b296a95 100644 --- a/src/main/skills/skill-discovery-stopped-wsl-roots.ts +++ b/src/main/skills/skill-discovery-stopped-wsl-roots.ts @@ -23,13 +23,15 @@ export function unavailableSkillSource(root: SkillScanRoot): SkillDiscoverySourc * it, and passive UI runs this scan. The caller reports them unavailable, so the next scan reads them. */ export async function splitStoppedWslDistroRoots( - roots: readonly SkillScanRoot[] + roots: readonly SkillScanRoot[], + /** Roots a user act may read even on a stopped distro; they never ask the gate. */ + mayReadStopped: (path: string) => boolean = () => false ): Promise<{ readable: SkillScanRoot[]; unavailable: SkillScanRoot[] }> { const blockedByDistro = new Map>() const blocked = await Promise.all( roots.map(async (root) => { const distro = wslDistroForPath(root.path) - if (!distro) { + if (!distro || mayReadStopped(root.path)) { return false } const key = distro.toLowerCase() @@ -47,6 +49,27 @@ export async function splitStoppedWslDistroRoots( } } +function comparablePath(path: string): string { + return path + .replace(/[\\/]+/g, '/') + .replace(/\/$/, '') + .toLowerCase() +} + +/** Whether a stopped-distro root or plugin cwd at `path` holds one of the chosen skill directories. */ +export function stoppedRootReader( + readFor: 'all' | readonly string[] | undefined +): (path: string) => boolean { + if (readFor === 'all') { + return () => true + } + const directories = (readFor ?? []).map(comparablePath) + return (path) => { + const root = comparablePath(path) + return directories.some((directory) => directory === root || directory.startsWith(`${root}/`)) + } +} + /** Whether reading `path` would start a stopped (or unverifiable) WSL distro. */ export async function isOnStoppedWslDistro(path: string): Promise { return wslDistroForPath(path) !== null && (await blockedWslDistroForPath(path)) !== null diff --git a/src/main/skills/skill-discovery-target.ts b/src/main/skills/skill-discovery-target.ts index 4fd28d78c3f..9b1f6b6a630 100644 --- a/src/main/skills/skill-discovery-target.ts +++ b/src/main/skills/skill-discovery-target.ts @@ -156,16 +156,21 @@ export async function discoverSkillsOnTarget( options: { refresh?: boolean providerRootOverrides?: SkillProviderRootOverrides - /** False for a user act (Share), which may read a stopped distro's folders. */ - gateStoppedWslDistros?: boolean + /** A user act (Share) may read stopped distros' folders: 'all', or only those holding these skill directories. */ + readStoppedWslRootsFor?: 'all' | readonly string[] } = {} ): Promise { const refresh = options.refresh === true - const gate = options.gateStoppedWslDistros === false ? { gateStoppedWslDistros: false } : {} + const readFor = options.readStoppedWslRootsFor + const gate = readFor ? { readStoppedWslRootsFor: readFor } : {} + const userActKey = + readFor === undefined + ? '' + : `:user-act:${readFor === 'all' ? 'all' : stablePathId([...readFor].sort().join('\0'))}` try { const outcome = await targetScans.run( // A gated and an ungated scan of one target must not answer for each other. - `${scanKey(target, repos, options.providerRootOverrides)}${options.gateStoppedWslDistros === false ? ':user-act' : ''}`, + `${scanKey(target, repos, options.providerRootOverrides)}${userActKey}`, { ttlMs: target.kind === 'wsl' ? WSL_RESULT_TTL_MS : 0, refresh }, async (): Promise => { if (target.kind === 'wsl') { diff --git a/src/preload/api/agent-skill-api.ts b/src/preload/api/agent-skill-api.ts index 66611000238..36a5ba17099 100644 --- a/src/preload/api/agent-skill-api.ts +++ b/src/preload/api/agent-skill-api.ts @@ -46,6 +46,7 @@ export type SkillsApi = { bundledExecute: (request: BundledSkillExecuteRequest) => Promise prepareShare: (input: { skillIds: string[] + skillDirectories?: string[] bundleName: string target?: SkillDiscoveryTarget packageId?: string diff --git a/src/preload/api/skills-bridge.ts b/src/preload/api/skills-bridge.ts index d9d6d92dd16..d7cc98cd232 100644 --- a/src/preload/api/skills-bridge.ts +++ b/src/preload/api/skills-bridge.ts @@ -49,6 +49,7 @@ export const skillsApi = { ipcRenderer.invoke('skills:bundledExecute', request), prepareShare: (input: { skillIds: string[] + skillDirectories?: string[] bundleName: string target?: SkillDiscoveryTarget packageId?: string diff --git a/src/renderer/src/components/skills/SkillShareDialog.test.tsx b/src/renderer/src/components/skills/SkillShareDialog.test.tsx index 78e4684e91a..a6a0b23ccaf 100644 --- a/src/renderer/src/components/skills/SkillShareDialog.test.tsx +++ b/src/renderer/src/components/skills/SkillShareDialog.test.tsx @@ -245,6 +245,7 @@ describe('SkillShareDialog', () => { expect(screen.getByRole('button', { name: 'Publish new version' })).toBeTruthy() expect(skills.prepareShare).toHaveBeenCalledWith({ skillIds: [skill.id], + skillDirectories: [skill.directoryPath], bundleName: skill.name, packageId: 'pkg_1' }) @@ -260,6 +261,7 @@ describe('SkillShareDialog', () => { await waitFor(() => expect(skills.prepareShare).toHaveBeenCalledWith({ skillIds: [skill.id], + skillDirectories: [skill.directoryPath], bundleName: skill.name }) ) @@ -280,6 +282,7 @@ describe('SkillShareDialog', () => { expect(screen.getByRole('button', { name: 'Publish new version' })).toBeTruthy() expect(skills.prepareShare).toHaveBeenCalledWith({ skillIds: [skill.id, secondSkill.id], + skillDirectories: [skill.directoryPath, secondSkill.directoryPath], bundleName: 'private-skill-and-1-more', packageId: 'pkg_1' }) diff --git a/src/renderer/src/components/skills/SkillShareDialog.tsx b/src/renderer/src/components/skills/SkillShareDialog.tsx index 4e2f739f37b..6686f43f319 100644 --- a/src/renderer/src/components/skills/SkillShareDialog.tsx +++ b/src/renderer/src/components/skills/SkillShareDialog.tsx @@ -102,6 +102,8 @@ export function SkillShareDialog({ } const nextPreview = await window.api.skills.prepareShare({ skillIds: selectedSkills.map((skill) => skill.id), + // Only these folders' roots may start a stopped WSL distro. + skillDirectories: selectedSkills.map((skill) => skill.directoryPath), bundleName, ...(managedInstall ? { packageId: managedInstall.packageId } : {}) })