diff --git a/src/main/skills/skill-delete/enumeration.ts b/src/main/skills/skill-delete/enumeration.ts index 2175267739c..6c914a1451b 100644 --- a/src/main/skills/skill-delete/enumeration.ts +++ b/src/main/skills/skill-delete/enumeration.ts @@ -1,9 +1,9 @@ import { posix as pathPosix, win32 as pathWin32 } from 'node:path' import { skillDirectoryMaxDepth } from '../../../shared/skill-discovery-depth' import type { SkillPathSemantics } from '../../../shared/skill-path-containment' -import type { SkillScanRoot } from '..//' -import type { SkillInstallFilesystem, SkillPathInspection } from '..//' -import { SKILL_FILE_NAME } from '..//' +import type { SkillScanRoot } from '../skill-discovery-sources' +import type { SkillInstallFilesystem, SkillPathInspection } from '../skill-install-filesystem' +import { SKILL_FILE_NAME } from '../skill-root-file-walk' import { isSkillStagingEntryName } from './staging-names' /** One directory in a discovery root that holds a `SKILL.md`, with everything @@ -32,7 +32,11 @@ export function requireEnumerableFilesystem(filesystem: SkillInstallFilesystem): if (!filesystem.listEntries || !filesystem.inspectPaths) { throw new SkillDeleteEnumerationUnsupportedError() } - return { listEntries: filesystem.listEntries, inspectPaths: filesystem.inspectPaths } + // Bound, not detached: the WSL filesystem's `listEntries` reaches `this.runOutput`. + return { + listEntries: filesystem.listEntries.bind(filesystem), + inspectPaths: filesystem.inspectPaths.bind(filesystem) + } } /** diff --git a/src/main/skills/skill-delete/guards.ts b/src/main/skills/skill-delete/guards.ts index a788e1d7251..840e0c74f83 100644 --- a/src/main/skills/skill-delete/guards.ts +++ b/src/main/skills/skill-delete/guards.ts @@ -9,7 +9,7 @@ import { skillPathsEqual, type SkillPathSemantics } from '../../../shared/skill-path-containment' -import type { SkillScanRoot } from '..//' +import type { SkillScanRoot } from '../skill-discovery-sources' import type { SkillPlacementCandidate } from './enumeration' /** diff --git a/src/main/skills/skill-delete/plan.test.ts b/src/main/skills/skill-delete/plan.test.ts index d7790b2bc00..1262435a1eb 100644 --- a/src/main/skills/skill-delete/plan.test.ts +++ b/src/main/skills/skill-delete/plan.test.ts @@ -4,7 +4,10 @@ import { join } from 'node:path' import { afterEach, describe, expect, it } from 'vitest' import type { SkillDeleteRequest } from '../../../shared/skill-delete-contract' import { buildSkillDeletePlan } from './plan' -import { nativeSkillInstallFilesystem } from '..//' +import { nativeSkillInstallFilesystem } from '../skill-install-filesystem' + +/** Windows rejects `symlink` with EPERM without elevation or Developer Mode. */ +const WINDOWS = process.platform === 'win32' const roots: string[] = [] @@ -56,86 +59,104 @@ function plan(home: string, deleteRequest: SkillDeleteRequest) { } describe('buildSkillDeletePlan placement enumeration', () => { - it('finds the canonical directory and every alias that resolves to it', async () => { - const { home } = await fixture() - const canonicalDirectory = join(home, '.agents', 'skills', 'demo') - const file = await writeSkill(canonicalDirectory, 'demo') - // An alias-dir: a symlinked directory in another agent's root. - await mkdir(join(home, '.claude', 'skills'), { recursive: true }) - await symlink(canonicalDirectory, join(home, '.claude', 'skills', 'demo'), 'dir') - // An alias-file: a real directory whose SKILL.md is a symlink. - const aliasFileDirectory = join(home, '.codex', 'skills', 'demo') - await mkdir(aliasFileDirectory, { recursive: true }) - await symlink(file, join(aliasFileDirectory, 'SKILL.md')) + it.skipIf(WINDOWS)( + 'finds the canonical directory and every alias that resolves to it', + async () => { + const { home } = await fixture() + const canonicalDirectory = join(home, '.agents', 'skills', 'demo') + const file = await writeSkill(canonicalDirectory, 'demo') + // An alias-dir: a symlinked directory in another agent's root. + await mkdir(join(home, '.claude', 'skills'), { recursive: true }) + await symlink(canonicalDirectory, join(home, '.claude', 'skills', 'demo'), 'dir') + // An alias-file: a real directory whose SKILL.md is a symlink. + const aliasFileDirectory = join(home, '.codex', 'skills', 'demo') + await mkdir(aliasFileDirectory, { recursive: true }) + await symlink(file, join(aliasFileDirectory, 'SKILL.md')) - const resolved = await plan(home, await request(file)) - const entry = resolved.plan.skills[0] - expect(entry.blocked).toBeUndefined() - expect(new Set(entry.placements.map((placement) => placement.kind))).toEqual( - new Set(['canonical', 'alias-dir', 'alias-file']) - ) - expect(entry.placements.map((placement) => placement.path)).toContain(canonicalDirectory) - }) + const resolved = await plan(home, await request(file)) + const entry = resolved.plan.skills[0] + expect(entry.blocked).toBeUndefined() + expect(new Set(entry.placements.map((placement) => placement.kind))).toEqual( + new Set(['canonical', 'alias-dir', 'alias-file']) + ) + expect(entry.placements.map((placement) => placement.path)).toContain(canonicalDirectory) + } + ) - it('classifies a directory whose SKILL.md is a symlink as alias-file, not canonical', async () => { - const { home } = await fixture() - const canonicalDirectory = join(home, '.agents', 'skills', 'demo') - const file = await writeSkill(canonicalDirectory, 'demo') - const aliasDirectory = join(home, '.claude', 'skills', 'demo') - await mkdir(aliasDirectory, { recursive: true }) - await symlink(file, join(aliasDirectory, 'SKILL.md')) + it.skipIf(WINDOWS)( + 'classifies a directory whose SKILL.md is a symlink as alias-file, not canonical', + async () => { + const { home } = await fixture() + const canonicalDirectory = join(home, '.agents', 'skills', 'demo') + const file = await writeSkill(canonicalDirectory, 'demo') + const aliasDirectory = join(home, '.claude', 'skills', 'demo') + await mkdir(aliasDirectory, { recursive: true }) + await symlink(file, join(aliasDirectory, 'SKILL.md')) - const resolved = await plan(home, await request(file)) - const alias = resolved.plan.skills[0].placements.find( - (placement) => placement.path === aliasDirectory - ) - expect(alias?.kind).toBe('alias-file') - }) + const resolved = await plan(home, await request(file)) + const alias = resolved.plan.skills[0].placements.find( + (placement) => placement.path === aliasDirectory + ) + expect(alias?.kind).toBe('alias-file') + } + ) - it('removes a home-root link to content outside every root, and only the link', async () => { - const { home } = await fixture() - const outside = join(home, 'elsewhere', 'demo') - await writeSkill(outside, 'demo') - await mkdir(join(home, '.claude', 'skills'), { recursive: true }) - await symlink(outside, join(home, '.claude', 'skills', 'demo'), 'dir') + it.skipIf(WINDOWS)( + 'removes a home-root link to content outside every root, and only the link', + async () => { + const { home } = await fixture() + const outside = join(home, 'elsewhere', 'demo') + await writeSkill(outside, 'demo') + await mkdir(join(home, '.claude', 'skills'), { recursive: true }) + await symlink(outside, join(home, '.claude', 'skills', 'demo'), 'dir') - const resolved = await plan( - home, - await request(join(home, '.claude', 'skills', 'demo', 'SKILL.md')) - ) - expect(resolved.plan.skills[0].blocked).toBeUndefined() - expect(resolved.plan.skills[0].placements).toEqual([ - expect.objectContaining({ path: join(home, '.claude', 'skills', 'demo'), kind: 'alias-dir' }) - ]) - }) + const resolved = await plan( + home, + await request(join(home, '.claude', 'skills', 'demo', 'SKILL.md')) + ) + expect(resolved.plan.skills[0].blocked).toBeUndefined() + expect(resolved.plan.skills[0].placements).toEqual([ + expect.objectContaining({ + path: join(home, '.claude', 'skills', 'demo'), + kind: 'alias-dir' + }) + ]) + } + ) - it('deletes the links of a repo skill whose content lives outside every root', async () => { - // The shape that bit a real user: `/.agents/skills/` is a link - // to a tool-managed directory elsewhere on disk. The links are Orca's to - // remove; the content behind them is not. - const { home } = await fixture() - const repo = join(home, 'projects', 'app') - const managedElsewhere = join(home, '.local', 'share', 'devex', 'skills', 'demo') - await writeSkill(managedElsewhere, 'demo') - await mkdir(join(repo, '.agents', 'skills'), { recursive: true }) - await symlink(managedElsewhere, join(repo, '.agents', 'skills', 'demo'), 'dir') + it.skipIf(WINDOWS)( + 'deletes the links of a repo skill whose content lives outside every root', + async () => { + // The shape that bit a real user: `/.agents/skills/` is a link + // to a tool-managed directory elsewhere on disk. The links are Orca's to + // remove; the content behind them is not. + const { home } = await fixture() + const repo = join(home, 'projects', 'app') + const managedElsewhere = join(home, '.local', 'share', 'devex', 'skills', 'demo') + await writeSkill(managedElsewhere, 'demo') + await mkdir(join(repo, '.agents', 'skills'), { recursive: true }) + await symlink(managedElsewhere, join(repo, '.agents', 'skills', 'demo'), 'dir') - const linked = join(repo, '.agents', 'skills', 'demo', 'SKILL.md') - const resolved = await buildSkillDeletePlan({ - request: await request(linked), - target: { kind: 'native-host', cwd: undefined }, - repos: [{ id: 'repo-1', name: 'app', path: repo } as never], - filesystem: nativeSkillInstallFilesystem, - homeDir: home - }) - const entry = resolved.plan.skills[0] - expect(entry.blocked).toBeUndefined() - expect(entry.placements).toEqual([ - expect.objectContaining({ path: join(repo, '.agents', 'skills', 'demo'), kind: 'alias-dir' }) - ]) - // Nothing outside the roots is ever offered for removal. - expect(entry.placements.some((placement) => placement.path.includes('devex'))).toBe(false) - }) + const linked = join(repo, '.agents', 'skills', 'demo', 'SKILL.md') + const resolved = await buildSkillDeletePlan({ + request: await request(linked), + target: { kind: 'native-host', cwd: undefined }, + repos: [{ id: 'repo-1', name: 'app', path: repo } as never], + filesystem: nativeSkillInstallFilesystem, + homeDir: home + }) + const entry = resolved.plan.skills[0] + expect(entry.blocked).toBeUndefined() + expect(entry.placements).toEqual([ + expect.objectContaining({ + path: join(repo, '.agents', 'skills', 'demo'), + kind: 'alias-dir' + }) + ]) + // Nothing outside the roots is ever offered for removal. + expect(entry.placements.some((placement) => placement.path.includes('devex'))).toBe(false) + } + ) it('still refuses when the content is outside and no link sits in a root', async () => { const { home } = await fixture() @@ -146,19 +167,22 @@ describe('buildSkillDeletePlan placement enumeration', () => { expect(resolved.plan.skills[0].placements).toEqual([]) }) - it('refuses a bundled skill even when the row claimed a home source', async () => { - const { home } = await fixture() - const bundled = join(home, '.claude', 'skills', '.system', 'demo') - const file = await writeSkill(bundled, 'demo') - // Symlinked into a plain home root, which is what makes the row read `home`. - await mkdir(join(home, '.agents', 'skills'), { recursive: true }) - await symlink(bundled, join(home, '.agents', 'skills', 'demo'), 'dir') + it.skipIf(WINDOWS)( + 'refuses a bundled skill even when the row claimed a home source', + async () => { + const { home } = await fixture() + const bundled = join(home, '.claude', 'skills', '.system', 'demo') + const file = await writeSkill(bundled, 'demo') + // Symlinked into a plain home root, which is what makes the row read `home`. + await mkdir(join(home, '.agents', 'skills'), { recursive: true }) + await symlink(bundled, join(home, '.agents', 'skills', 'demo'), 'dir') - const resolved = await plan(home, await request(file)) - expect(resolved.plan.skills[0].blocked).toBe('bundled') - }) + const resolved = await plan(home, await request(file)) + expect(resolved.plan.skills[0].blocked).toBe('bundled') + } + ) - it('refuses a plugin-cache skill symlinked into a plain home root', async () => { + it.skipIf(WINDOWS)('refuses a plugin-cache skill symlinked into a plain home root', async () => { const { home } = await fixture() const cached = join(home, '.codex', 'plugins', 'cache', 'pack', 'demo') const file = await writeSkill(cached, 'demo') diff --git a/src/main/skills/skill-delete/plan.ts b/src/main/skills/skill-delete/plan.ts index c0a5de19e16..a17266cad21 100644 --- a/src/main/skills/skill-delete/plan.ts +++ b/src/main/skills/skill-delete/plan.ts @@ -21,10 +21,10 @@ import { type SkillDeleteGuardContext } from './guards' import { buildSkillDeleteRootSet } from './roots' -import type { ResolvedSkillDiscoveryTarget } from '..//' -import type { SkillInstallFilesystem } from '..//' -import type { SkillProviderRootOverrides } from '..//' -import type { SkillScanRoot } from '..//' +import type { ResolvedSkillDiscoveryTarget } from '../skill-discovery-target' +import type { SkillInstallFilesystem } from '../skill-install-filesystem' +import type { SkillProviderRootOverrides } from '../skill-provider-destinations' +import type { SkillScanRoot } from '../skill-discovery-sources' /** Overlapping roots can reach one directory twice. Staging the same path twice * would fail the second rename and roll the whole skill back. */ diff --git a/src/main/skills/skill-delete/recovery.test.ts b/src/main/skills/skill-delete/recovery.test.ts index 30271f71193..70d15bf5b76 100644 --- a/src/main/skills/skill-delete/recovery.test.ts +++ b/src/main/skills/skill-delete/recovery.test.ts @@ -9,7 +9,11 @@ import { writeSkillDeleteJournal, type SkillDeleteJournalV1 } from './recovery' -import { writeSkillStateFile } from '..//' +import { + nativeSkillInstallFilesystem, + type SkillInstallFilesystem +} from '../skill-install-filesystem' +import { writeSkillStateFile } from '../skill-install-provenance' import { skillDeleteStagedName } from './staging-names' const roots: string[] = [] @@ -147,6 +151,30 @@ describe('recoverSkillDeleteTransaction', () => { expect(await exists(skillDeleteJournalPath(stateDirectory, canonicalPath))).toBe(false) }) + it('keeps the journal when a rollback rename fails, so startup can retry it', async () => { + const { stateDirectory, root } = await fixture() + const canonicalPath = join(root, 'demo') + const entry = journal(root, canonicalPath, { phase: 'staging' }) + await mkdir(entry.moves[0].stagedPath, { recursive: true }) + await writeSkillDeleteJournal(stateDirectory, entry) + + const filesystem: SkillInstallFilesystem = { + ...nativeSkillInstallFilesystem, + rename: () => Promise.reject(new Error('EPERM')) + } + await recoverSkillDeleteTransaction(stateDirectory, canonicalPath, filesystem) + + // Journal retained: the staged path is still parked, and dropping the record + // would strand it with nothing left to retry from. + expect(await exists(entry.moves[0].stagedPath)).toBe(true) + expect(await exists(skillDeleteJournalPath(stateDirectory, canonicalPath))).toBe(true) + + // Retry on a working filesystem completes and only then clears the journal. + await recoverSkillDeleteTransaction(stateDirectory, canonicalPath) + expect(await exists(canonicalPath)).toBe(true) + expect(await exists(skillDeleteJournalPath(stateDirectory, canonicalPath))).toBe(false) + }) + it('restores canonical before alias-file, so no alias dangles mid-recovery', async () => { const { home, stateDirectory, root } = await fixture() const canonicalPath = join(root, 'demo') diff --git a/src/main/skills/skill-delete/recovery.ts b/src/main/skills/skill-delete/recovery.ts index 8097a37179c..960b7f8ed41 100644 --- a/src/main/skills/skill-delete/recovery.ts +++ b/src/main/skills/skill-delete/recovery.ts @@ -6,8 +6,15 @@ import { type SkillDeletePlacementKind } from '../../../shared/skill-delete-contract' import { nativeSkillPathSemantics, skillPathInside } from '../../../shared/skill-path-containment' -import { nativeSkillInstallFilesystem, type SkillInstallFilesystem } from '..//' -import { removeSkillInstallReceipt, skillInstallStateKey, writeSkillStateFile } from '..//' +import { + nativeSkillInstallFilesystem, + type SkillInstallFilesystem +} from '../skill-install-filesystem' +import { + removeSkillInstallReceipt, + skillInstallStateKey, + writeSkillStateFile +} from '../skill-install-provenance' import { isSkillDeleteStagedName } from './staging-names' const JOURNAL_MAX_BYTES = 4 * 1024 * 1024 @@ -158,9 +165,18 @@ export async function recoverSkillDeleteTransaction( // Roll back in reverse of staging order, so a restored alias never points at // a canonical directory that does not exist yet. Indexed rather than // `toReversed()`: this module reaches the Node 18 relay bundle. + let restoredEvery = true for (let index = staged.length - 1; index >= 0; index -= 1) { const move = staged[index] - await filesystem.rename(move.stagedPath, move.sourcePath).catch(() => undefined) + // Keep going past a failure so the other moves still come back, but keep + // the journal: dropping it here strands the staged path with no record + // that startup could retry from. Restores are idempotent. + await filesystem.rename(move.stagedPath, move.sourcePath).catch(() => { + restoredEvery = false + }) + } + if (!restoredEvery) { + return } } await rm(skillDeleteJournalPath(stateDirectory, canonicalPath), { force: true }) diff --git a/src/main/skills/skill-delete/request-service.ts b/src/main/skills/skill-delete/request-service.ts index 3c5b21c7177..5e1a0f2a6ce 100644 --- a/src/main/skills/skill-delete/request-service.ts +++ b/src/main/skills/skill-delete/request-service.ts @@ -6,10 +6,10 @@ import type { SkillDeleteResult } from '../../../shared/skill-delete-contract' import { deleteSkills, previewSkillDeletion, type SkillDeleteServiceInput } from './service' -import type { ResolvedSkillDiscoveryTarget } from '..//' -import { nativeSkillInstallFilesystem } from '..//' -import type { SkillProviderRootOverrides } from '..//' -import { WslSkillInstallFilesystem } from '..//' +import type { ResolvedSkillDiscoveryTarget } from '../skill-discovery-target' +import { nativeSkillInstallFilesystem } from '../skill-install-filesystem' +import type { SkillProviderRootOverrides } from '../skill-provider-destinations' +import { WslSkillInstallFilesystem } from '../skill-wsl-install-filesystem' /** Assembly shared by the local IPC channel and the runtime RPC method, so both * transports resolve the same host, roots, and filesystem. */ diff --git a/src/main/skills/skill-delete/roots.ts b/src/main/skills/skill-delete/roots.ts index 97afcfd49d5..1958ed91942 100644 --- a/src/main/skills/skill-delete/roots.ts +++ b/src/main/skills/skill-delete/roots.ts @@ -7,11 +7,11 @@ import { type SkillPathSemantics } from '../../../shared/skill-path-containment' import { toWindowsWslPath } from '../../../shared/wsl-paths' -import { discoverClaudePluginSkillSources } from './claude-plugin-skill-sources' -import { discoverClaudePluginSkillSourcesInWsl } from './claude-plugin-skill-sources-wsl' -import { buildSkillDiscoverySources, type SkillScanRoot } from '..//' -import type { ResolvedSkillDiscoveryTarget } from '..//' -import type { SkillProviderRootOverrides } from '..//' +import { discoverClaudePluginSkillSources } from '../claude-plugin-skill-sources' +import { discoverClaudePluginSkillSourcesInWsl } from '../claude-plugin-skill-sources-wsl' +import { buildSkillDiscoverySources, type SkillScanRoot } from '../skill-discovery-sources' +import type { ResolvedSkillDiscoveryTarget } from '../skill-discovery-target' +import type { SkillProviderRootOverrides } from '../skill-provider-destinations' /** * The delete host owns everything about where skills may live, so it rebuilds diff --git a/src/main/skills/skill-delete/service.test.ts b/src/main/skills/skill-delete/service.test.ts index 2413d1c010e..97fcd07fc07 100644 --- a/src/main/skills/skill-delete/service.test.ts +++ b/src/main/skills/skill-delete/service.test.ts @@ -4,9 +4,12 @@ import { join } from 'node:path' import { afterEach, describe, expect, it, vi } from 'vitest' import type { SkillDeleteRequest } from '../../../shared/skill-delete-contract' import { deleteSkills } from './service' -import { acquireSkillInstallLock, skillInstallLockPath } from '..//' -import { nativeSkillInstallFilesystem, type SkillInstallFilesystem } from '..//' -import { readSkillInstallReceipt, writeSkillInstallReceipt } from '..//' +import { acquireSkillInstallLock, skillInstallLockPath } from '../skill-install-lock' +import { + nativeSkillInstallFilesystem, + type SkillInstallFilesystem +} from '../skill-install-filesystem' +import { readSkillInstallReceipt, writeSkillInstallReceipt } from '../skill-install-provenance' import { skillDeleteJournalPath } from './recovery' const roots: string[] = [] diff --git a/src/main/skills/skill-delete/service.ts b/src/main/skills/skill-delete/service.ts index 10e1643d6c7..b02f0e4baf3 100644 --- a/src/main/skills/skill-delete/service.ts +++ b/src/main/skills/skill-delete/service.ts @@ -22,12 +22,15 @@ import { removeStagedSkillDeleteMoves, stageSkillDeleteMoves } from './staging' -import { clearSkillDiscoveryCaches, type ResolvedSkillDiscoveryTarget } from '..//' -import { acquireSkillInstallLock, skillInstallLockPath } from '..//' -import { removeSkillInstallReceipt } from '..//' -import type { SkillInstallFilesystem } from '..//' -import type { SkillProviderRootOverrides } from '..//' -import { recordSkillDeleteOperation } from '..//' +import { + clearSkillDiscoveryCaches, + type ResolvedSkillDiscoveryTarget +} from '../skill-discovery-target' +import { acquireSkillInstallLock, skillInstallLockPath } from '../skill-install-lock' +import { removeSkillInstallReceipt } from '../skill-install-provenance' +import type { SkillInstallFilesystem } from '../skill-install-filesystem' +import type { SkillProviderRootOverrides } from '../skill-provider-destinations' +import { recordSkillDeleteOperation } from '../skill-operation-observability' export type SkillDeleteServiceInput = { request: SkillDeleteRequest diff --git a/src/main/skills/skill-delete/staging-visibility.test.ts b/src/main/skills/skill-delete/staging-visibility.test.ts index 1be7ea670fe..ffb43ce11c0 100644 --- a/src/main/skills/skill-delete/staging-visibility.test.ts +++ b/src/main/skills/skill-delete/staging-visibility.test.ts @@ -2,8 +2,8 @@ import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it } from 'vitest' -import { buildWslSkillDiscoveryCommand } from '..//' -import { findSkillFiles } from '..//' +import { buildWslSkillDiscoveryCommand } from '../skill-discovery-wsl' +import { findSkillFiles } from '../skill-root-file-walk' import { isSkillStagingEntryName, skillDeleteStagedName, SKILL_STAGING_GLOB } from './staging-names' const roots: string[] = [] diff --git a/src/main/skills/skill-delete/staging.ts b/src/main/skills/skill-delete/staging.ts index f561ee2adc9..dcf8c6f7107 100644 --- a/src/main/skills/skill-delete/staging.ts +++ b/src/main/skills/skill-delete/staging.ts @@ -3,8 +3,8 @@ import { posix as pathPosix, win32 as pathWin32 } from 'node:path' import type { SkillDeletePlacement } from '../../../shared/skill-delete-contract' import { nativeSkillPathSemantics } from '../../../shared/skill-path-containment' import type { SkillDeleteJournalMove } from './recovery' -import type { SkillInstallFilesystem } from '..//' -import { SKILL_FILE_NAME } from '..//' +import type { SkillInstallFilesystem } from '../skill-install-filesystem' +import { SKILL_FILE_NAME } from '../skill-root-file-walk' import { skillDeleteStagedName } from './staging-names' function api(): typeof pathPosix { diff --git a/src/main/skills/skill-delete/wsl-enumeration-protocol.ts b/src/main/skills/skill-delete/wsl-enumeration-protocol.ts index 8c5cacd1e4b..3776ee58ee3 100644 --- a/src/main/skills/skill-delete/wsl-enumeration-protocol.ts +++ b/src/main/skills/skill-delete/wsl-enumeration-protocol.ts @@ -1,4 +1,8 @@ -import type { SkillDirectoryEntry, SkillFilesystemEntryKind, SkillPathInspection } from '..//' +import type { + SkillDirectoryEntry, + SkillFilesystemEntryKind, + SkillPathInspection +} from '../skill-install-filesystem' /** * Guest-side enumeration for path-based skill deletion. @@ -98,10 +102,12 @@ export function parseWslListEntriesOutput( } const name = fields[index++] const kind = fields[index++] - if (name === undefined || !isEntryKind(kind)) { + // An `E` before any `D` would silently vanish, and a listing short of its + // real contents reads to the planner as an empty directory it may remove. + if (name === undefined || !isEntryKind(kind) || current === undefined) { throw new WslEnumerationProtocolError() } - current?.push({ name, kind }) + current.push({ name, kind }) } return listings } diff --git a/src/main/skills/skill-install-filesystem.ts b/src/main/skills/skill-install-filesystem.ts index d58ec51ea92..d51929d79f6 100644 --- a/src/main/skills/skill-install-filesystem.ts +++ b/src/main/skills/skill-install-filesystem.ts @@ -71,10 +71,20 @@ export const nativeSkillInstallFilesystem: SkillInstallFilesystem = { new Map( await runSkillCandidateTasks( directories.map((directory) => async (): Promise<[string, SkillDirectoryEntry[]]> => { - const entries = await readdir(directory, { withFileTypes: true }).catch(() => null) + // Only a confirmed absence may read as an empty directory: the delete + // planner treats an empty listing as "nothing left here" and removes + // the parent, so a swallowed EACCES/EIO would delete unenumerated files. + const entries = await readdir(directory, { withFileTypes: true }).catch( + (error: NodeJS.ErrnoException) => { + if (error?.code === 'ENOENT' || error?.code === 'ENOTDIR') { + return [] + } + throw error + } + ) return [ directory, - (entries ?? []).map((entry) => ({ name: entry.name, kind: direntKind(entry) })) + entries.map((entry) => ({ name: entry.name, kind: direntKind(entry) })) ] }) ) diff --git a/src/main/skills/skill-wsl-delete.integration.test.ts b/src/main/skills/skill-wsl-delete.integration.test.ts index 5fa4f4fb966..143216a46b5 100644 --- a/src/main/skills/skill-wsl-delete.integration.test.ts +++ b/src/main/skills/skill-wsl-delete.integration.test.ts @@ -1,19 +1,22 @@ -import { execFile } from 'node:child_process' -import { promisify } from 'node:util' import { afterAll, beforeAll, describe, expect, it } from 'vitest' +import { runProcess } from '../../shared/child-process/run-process' +import { resolveWslExecutablePath } from '../wsl/wsl-executable-path' import { WslSkillInstallFilesystem } from './skill-wsl-install-filesystem' -const execFileAsync = promisify(execFile) const DISTRO = process.env.ORCA_REAL_WSL_SKILL_DISTRO ?? 'Ubuntu-24.04' const RUN_REAL_WSL = process.platform === 'win32' && process.env.ORCA_REAL_WSL_SKILL_TEST === '1' async function runWsl(...args: string[]): Promise { - const { stdout } = await execFileAsync('wsl.exe', ['-d', DISTRO, '--exec', ...args], { - encoding: 'utf8', - timeout: 30_000, - windowsHide: true + const result = await runProcess({ + program: resolveWslExecutablePath(), + args: ['-d', DISTRO, '--exec', ...args], + timeoutMs: 30_000 }) - return stdout.trim() + // `runProcess` reports a non-zero exit as data; this harness wants it fatal. + if (result.code !== 0) { + throw new Error(`wsl ${args.join(' ')} exited ${result.code}: ${result.stderr.trim()}`) + } + return result.stdout.trim() } function uncPath(guestPath: string): string { diff --git a/src/renderer/src/components/skills/skill-delete-copy.ts b/src/renderer/src/components/skills/skill-delete-copy.ts index afd27269f11..cf65f0b60be 100644 --- a/src/renderer/src/components/skills/skill-delete-copy.ts +++ b/src/renderer/src/components/skills/skill-delete-copy.ts @@ -86,9 +86,18 @@ export function skillDeletePlacementSummary(plan: SkillDeletePlan): string | nul ) } +/** + * Not `node:path`: the renderer may be showing a Windows host's paths from macOS, + * so `dirname` would pick the wrong separator. Both separators are honoured, and + * a root keeps its trailing one — `/SKILL.md` is `/`, `C:\SKILL.md` is `C:\`. + */ function parentDirectory(path: string): string { const index = Math.max(path.lastIndexOf('/'), path.lastIndexOf('\\')) - return index > 0 ? path.slice(0, index) : path + if (index < 0) { + return path + } + const isDriveRoot = index === 2 && path[1] === ':' + return index === 0 || isDriveRoot ? path.slice(0, index + 1) : path.slice(0, index) } /** diff --git a/src/renderer/src/components/skills/use-skill-delete-flow.ts b/src/renderer/src/components/skills/use-skill-delete-flow.ts index eaa0e8f697f..69f979c7988 100644 --- a/src/renderer/src/components/skills/use-skill-delete-flow.ts +++ b/src/renderer/src/components/skills/use-skill-delete-flow.ts @@ -8,7 +8,6 @@ import type { SkillDeleteRequest, SkillDeleteResult } from '../../../../shared/skill-delete-contract' -import { SKILL_DELETE_UPDATE_REQUIRED_MESSAGE } from '../../../../shared/skill-install-capability' import type { DiscoveredSkill } from '../../../../shared/skills' import { deleteSkillsOnRuntimeTarget, @@ -173,8 +172,13 @@ function blockedOnlyResult(plan: SkillDeletePlan): SkillDeleteResult { } function unsupportedReason(runtimeTarget: RuntimeClientTarget | null): string { + // The shared constant is the wire-side error text; what the user reads here + // goes through the catalog like every other string in this panel. return runtimeTarget - ? SKILL_DELETE_UPDATE_REQUIRED_MESSAGE + ? translate( + 'auto.components.skills.SkillDelete.hostUpdateRequired', + 'Update Orca on the selected machine to delete skills.' + ) : translate( 'auto.components.skills.SkillDelete.hostUnresolved', 'Still finding the machine that owns these skills.' diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index bfa7afa23ab..45f318b0707 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -4670,6 +4670,7 @@ "confirmPermanent": "This cannot be undone.", "failed": "Could not delete skills", "hostUnresolved": "Still finding the machine that owns these skills.", + "hostUpdateRequired": "Update Orca on the selected machine to delete skills.", "nothingOne": "That skill cannot be deleted from here.", "nothingOther": "None of the {{count}} selected skills can be deleted from here.", "placementSummaryParts": "Removes {{parts}} across {{roots}}.",