diff --git a/src/main/ipc/register-core-handlers.test.ts b/src/main/ipc/register-core-handlers.test.ts index 650384452e4..f175a2d0865 100644 --- a/src/main/ipc/register-core-handlers.test.ts +++ b/src/main/ipc/register-core-handlers.test.ts @@ -59,6 +59,7 @@ const { registerTerminalPreviewHandlersMock, registerSpeechHandlersMock, registerSkillsHandlersMock, + registerSkillDeleteIpcHandlersMock, registerWorkspaceSpaceHandlersMock, registerWorkspacePortHandlersMock, registerLocalhostWorktreeLabelHandlersMock, @@ -124,6 +125,7 @@ const { registerTerminalPreviewHandlersMock: vi.fn(), registerSpeechHandlersMock: vi.fn(), registerSkillsHandlersMock: vi.fn(), + registerSkillDeleteIpcHandlersMock: vi.fn(), registerWorkspaceSpaceHandlersMock: vi.fn(), registerWorkspacePortHandlersMock: vi.fn(), registerLocalhostWorktreeLabelHandlersMock: vi.fn(), @@ -226,6 +228,10 @@ vi.mock('./skills', () => ({ registerSkillsHandlers: registerSkillsHandlersMock })) +vi.mock('./skill-delete-ipc-handlers', () => ({ + registerSkillDeleteIpcHandlers: registerSkillDeleteIpcHandlersMock +})) + vi.mock('./workspace-space', () => ({ registerWorkspaceSpaceHandlers: registerWorkspaceSpaceHandlersMock })) @@ -441,6 +447,7 @@ describe('registerCoreHandlers', () => { registerTerminalPreviewHandlersMock.mockReset() registerSpeechHandlersMock.mockReset() registerSkillsHandlersMock.mockReset() + registerSkillDeleteIpcHandlersMock.mockReset() registerWorkspaceSpaceHandlersMock.mockReset() registerWorkspacePortHandlersMock.mockReset() registerLocalhostWorktreeLabelHandlersMock.mockReset() @@ -528,6 +535,7 @@ describe('registerCoreHandlers', () => { expect(registerTerminalPreviewHandlersMock).toHaveBeenCalledWith(runtime) expect(registerSettingsHandlersMock).toHaveBeenCalledWith(store, agentAwakeService) expect(registerSkillsHandlersMock).toHaveBeenCalledWith(store, runtime) + expect(registerSkillDeleteIpcHandlersMock).toHaveBeenCalledWith(store, runtime) expect(registerWorkspaceSpaceHandlersMock).toHaveBeenCalledWith(store) expect(registerWorkspacePortHandlersMock).toHaveBeenCalledWith(store) expect(registerLocalhostWorktreeLabelHandlersMock).toHaveBeenCalledWith(store) diff --git a/src/main/ipc/register-core-handlers.ts b/src/main/ipc/register-core-handlers.ts index 137d49216ac..eff88534dd2 100644 --- a/src/main/ipc/register-core-handlers.ts +++ b/src/main/ipc/register-core-handlers.ts @@ -39,6 +39,7 @@ import { registerSessionHandlers } from './session' import { registerSettingsHandlers } from './settings' import { registerDiagnosticsHandlers } from './diagnostics' import { registerSkillsHandlers } from './skills' +import { registerSkillDeleteIpcHandlers } from './skill-delete-ipc-handlers' import { registerWorkspaceSpaceHandlers } from './workspace-space' import { registerWorkspacePortHandlers } from './workspace-ports' import { registerLocalhostWorktreeLabelHandlers } from './localhost-worktree-labels' @@ -176,6 +177,7 @@ export function registerCoreHandlers( registerComputerUsePermissionHandlers() registerSettingsHandlers(store, agentAwakeService) registerSkillsHandlers(store, runtime) + registerSkillDeleteIpcHandlers(store, runtime) if (automations) { registerAutomationHandlers(store, automations) } diff --git a/src/main/ipc/skill-delete-ipc-handlers.ts b/src/main/ipc/skill-delete-ipc-handlers.ts new file mode 100644 index 00000000000..9043b400dee --- /dev/null +++ b/src/main/ipc/skill-delete-ipc-handlers.ts @@ -0,0 +1,60 @@ +import { app } from 'electron' +import type { Store } from '../persistence' +import type { OrcaRuntimeService } from '../runtime/orca-runtime' +import { + SkillDeleteRequestSchema, + type SkillDeletePlan, + type SkillDeleteResult +} from '../../shared/skill-delete-contract' +import { SkillDiscoveryTargetSchema } from '../../shared/skills' +import { + previewSkillDeleteRequest, + runSkillDeleteRequest, + type SkillDeleteRequestDependencies +} from '../skills/skill-delete-request-service' +import { resolveSkillDiscoveryTarget } from '../skills/skill-discovery-target' +import { handleMainWindowSkillIpc } from './skill-ipc-main-window' + +/** + * Local only. A remote runtime is reached straight from the renderer through + * `callRuntimeRpc`, mirroring how the scan is routed — there is deliberately no + * main-process remote branch here, unlike install and remove, because a + * main-side capability check on one transport never runs on the other. + */ +export function registerSkillDeleteIpcHandlers(store: Store, runtime?: OrcaRuntimeService): void { + const dependencies: SkillDeleteRequestDependencies = { + repos: () => store.getRepos(), + ...(runtime + ? { + resolveProviderRootOverrides: (target) => + runtime.resolveSkillDiscoveryProviderRoots(target) + } + : {}), + userDataPath: app.getPath('userData') + } + const resolve = (value: unknown) => { + const request = SkillDeleteRequestSchema.parse(value) + // Send exactly what the scan sent: the nested target keeps discovery's own + // stripping schema, while the request itself is strict. + const target = resolveSkillDiscoveryTarget( + request.target ? SkillDiscoveryTargetSchema.parse(request.target) : undefined + ) + return { request, target } + } + + handleMainWindowSkillIpc( + 'skills:previewDelete', + async (_event, value: unknown): Promise => { + const { request, target } = resolve(value) + return previewSkillDeleteRequest(request, target, dependencies) + } + ) + + handleMainWindowSkillIpc( + 'skills:delete', + async (_event, value: unknown): Promise => { + const { request, target } = resolve(value) + return runSkillDeleteRequest(request, target, dependencies) + } + ) +} diff --git a/src/main/runtime/rpc/methods/skills.ts b/src/main/runtime/rpc/methods/skills.ts index bb18f43f804..ad5cd50a8f7 100644 --- a/src/main/runtime/rpc/methods/skills.ts +++ b/src/main/runtime/rpc/methods/skills.ts @@ -1,5 +1,12 @@ +import { app } from 'electron' import { defineMethod, type RpcMethod } from '../core' import { z } from 'zod' +import { SkillDeleteRequestSchema } from '../../../../shared/skill-delete-contract' +import { + previewSkillDeleteRequest, + runSkillDeleteRequest, + type SkillDeleteRequestDependencies +} from '../../../skills/skill-delete-request-service' import { SkillDiscoveryTargetSchema } from '../../../../shared/skills' import { SkillInstallPreviewRequestSchema, @@ -27,7 +34,9 @@ import { AgentSkillSharingError } from '../../../../shared/agent-skill-sharing-contract' -function resolveDiscoveryTarget( +/** Exported so the delete plan's root rebuild resolves its target exactly the + * way `skills.discover` resolved the scan's — including WSL. */ +export function resolveDiscoveryTarget( params: z.infer, runtime: Pick ) { @@ -40,6 +49,16 @@ function resolveDiscoveryTarget( return resolveSkillDiscoveryTarget(target) } +function skillDeleteDependencies( + runtime: Pick +): SkillDeleteRequestDependencies { + return { + repos: () => runtime.listRepos(), + resolveProviderRootOverrides: (target) => runtime.resolveSkillDiscoveryProviderRoots(target), + userDataPath: app.getPath('userData') + } +} + export const SKILL_METHODS: RpcMethod[] = [ defineMethod({ name: 'skills.discover', @@ -55,6 +74,26 @@ export const SKILL_METHODS: RpcMethod[] = [ }) } }), + defineMethod({ + name: 'skills.previewDelete', + params: SkillDeleteRequestSchema, + handler: async (params, { runtime }) => + previewSkillDeleteRequest( + params, + resolveDiscoveryTarget(params.target ?? {}, runtime), + skillDeleteDependencies(runtime) + ) + }), + defineMethod({ + name: 'skills.delete', + params: SkillDeleteRequestSchema, + handler: async (params, { runtime }) => + runSkillDeleteRequest( + params, + resolveDiscoveryTarget(params.target ?? {}, runtime), + skillDeleteDependencies(runtime) + ) + }), defineMethod({ name: 'skills.share', params: AgentSkillShareRequestSchema, diff --git a/src/main/skills/discovery.ts b/src/main/skills/discovery.ts index 77e1e477ffd..a93968177a9 100644 --- a/src/main/skills/discovery.ts +++ b/src/main/skills/discovery.ts @@ -25,6 +25,7 @@ import { type SkillScanOutcome } from './skill-scan-coalescer' import type { SkillProviderRootOverrides } from './skill-provider-destinations' +import { skillDirectoryMaxDepth } from '../../shared/skill-discovery-depth' export { buildSkillDiscoverySources } from './skill-discovery-sources' @@ -136,7 +137,7 @@ async function readSkillSummary(skillFilePath: string): Promise<{ type ScannedSkill = DiscoveredSkill & { canonicalSkillFilePath: string } async function scanRoot(root: SkillScanRoot, signal: AbortSignal): Promise { - const maxDepth = root.sourceKind === 'plugin' ? 9 : 4 + const maxDepth = skillDirectoryMaxDepth(root.sourceKind) const skillFiles = await findSkillFiles(root.path, maxDepth, signal) // 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 diff --git a/src/main/skills/skill-delete-enumeration.ts b/src/main/skills/skill-delete-enumeration.ts new file mode 100644 index 00000000000..d97498d6050 --- /dev/null +++ b/src/main/skills/skill-delete-enumeration.ts @@ -0,0 +1,158 @@ +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 './skill-discovery-sources' +import type { SkillInstallFilesystem, SkillPathInspection } from './skill-install-filesystem' +import { SKILL_FILE_NAME } from './skill-root-file-walk' +import { isSkillStagingEntryName } from './skill-staging-names' + +/** One directory in a discovery root that holds a `SKILL.md`, with everything + * the classifier needs to decide which of the three placement kinds it is. */ +export type SkillPlacementCandidate = { + path: string + root: SkillScanRoot + /** `lstat` kind of the directory entry itself — a symlink here is an alias. */ + entryKind: 'directory' | 'symlink' + /** `lstat` kind of the `SKILL.md` inside it. */ + skillFileKind: 'file' | 'symlink' + directoryRealpath: string | null + skillFileRealpath: string | null +} + +export class SkillDeleteEnumerationUnsupportedError extends Error { + constructor() { + super('skill-delete-filesystem-cannot-enumerate') + this.name = 'SkillDeleteEnumerationUnsupportedError' + } +} + +type Enumerable = Required> + +export function requireEnumerableFilesystem(filesystem: SkillInstallFilesystem): Enumerable { + if (!filesystem.listEntries || !filesystem.inspectPaths) { + throw new SkillDeleteEnumerationUnsupportedError() + } + return { listEntries: filesystem.listEntries, inspectPaths: filesystem.inspectPaths } +} + +/** + * Breadth-first so every level costs one batched round trip rather than one per + * directory — the bound that keeps a WSL plan to a handful of `wsl.exe` boots + * instead of hundreds. + * + * Plugin roots are deliberately not walked: they nest up to ten levels, and a + * plugin skill is blocked from the canonical path alone (see the guards), so + * paying for that walk would buy nothing. + */ +export async function enumerateSkillPlacementCandidates(input: { + roots: readonly SkillScanRoot[] + filesystem: SkillInstallFilesystem + semantics: SkillPathSemantics + toFilesystemPath: (path: string) => string +}): Promise { + const { listEntries, inspectPaths } = requireEnumerableFilesystem(input.filesystem) + const api = input.semantics.sep === '\\' ? pathWin32 : pathPosix + const walkable = input.roots.filter((root) => root.sourceKind !== 'plugin') + const candidates: SkillPlacementCandidate[] = [] + const inspectionTargets = new Set() + let frontier = dedupeByPath(walkable.map((root) => ({ root, path: root.path, depth: 0 }))) + + while (frontier.length > 0) { + const listings = await listEntries(frontier.map((entry) => input.toFilesystemPath(entry.path))) + const next: typeof frontier = [] + for (const current of frontier) { + const entries = listings.get(input.toFilesystemPath(current.path)) ?? [] + const skillFile = entries.find((entry) => entry.name === SKILL_FILE_NAME) + // The root itself is never a placement, only the directories below it. + if (current.depth > 0 && (skillFile?.kind === 'file' || skillFile?.kind === 'symlink')) { + candidates.push({ + path: current.path, + root: current.root, + entryKind: 'directory', + skillFileKind: skillFile.kind, + directoryRealpath: null, + skillFileRealpath: null + }) + inspectionTargets.add(current.path) + inspectionTargets.add(api.join(current.path, SKILL_FILE_NAME)) + } + for (const entry of entries) { + if (entry.kind !== 'directory' && entry.kind !== 'symlink') { + continue + } + // The SKILL.md of this directory is already accounted for above; as a + // child entry it can only ever be a file, never a placement. + if (entry.name === SKILL_FILE_NAME || isSkillStagingEntryName(entry.name)) { + continue + } + const childPath = api.join(current.path, entry.name) + if (entry.kind === 'symlink') { + // A symlinked directory is an alias candidate on its own, and is not + // descended into: its contents belong to whatever it points at. + candidates.push({ + path: childPath, + root: current.root, + entryKind: 'symlink', + skillFileKind: 'file', + directoryRealpath: null, + skillFileRealpath: null + }) + inspectionTargets.add(childPath) + inspectionTargets.add(api.join(childPath, SKILL_FILE_NAME)) + continue + } + if (current.depth + 1 <= skillDirectoryMaxDepth(current.root.sourceKind)) { + next.push({ root: current.root, path: childPath, depth: current.depth + 1 }) + } + } + } + // Two roots can share a path — `~/.claude/skills` is both a home root and a + // repo root when the home directory is the workspace — so without this the + // same directory is walked (and later staged) twice. + frontier = dedupeByPath(next) + } + + const inspections = await resolveInspections(inspectPaths, inspectionTargets, input) + return candidates + .map((candidate) => { + const directory = inspections.get(candidate.path) + const skillFile = inspections.get(api.join(candidate.path, SKILL_FILE_NAME)) + return { + ...candidate, + entryKind: directory?.kind === 'symlink' ? ('symlink' as const) : candidate.entryKind, + skillFileKind: + skillFile?.kind === 'symlink' ? ('symlink' as const) : candidate.skillFileKind, + directoryRealpath: directory?.realpath ?? null, + skillFileRealpath: skillFile?.realpath ?? null + } + }) + .filter( + (candidate) => candidate.skillFileRealpath !== null || candidate.entryKind === 'symlink' + ) +} + +function dedupeByPath(entries: readonly T[]): T[] { + const seen = new Set() + return entries.filter((entry) => { + if (seen.has(entry.path)) { + return false + } + seen.add(entry.path) + return true + }) +} + +async function resolveInspections( + inspectPaths: Enumerable['inspectPaths'], + targets: ReadonlySet, + input: { toFilesystemPath: (path: string) => string } +): Promise> { + const ordered = [...targets] + const raw = await inspectPaths(ordered.map((path) => input.toFilesystemPath(path))) + return new Map( + ordered.flatMap((path) => { + const inspection = raw.get(input.toFilesystemPath(path)) + return inspection ? [[path, inspection] as const] : [] + }) + ) +} diff --git a/src/main/skills/skill-delete-guards.ts b/src/main/skills/skill-delete-guards.ts new file mode 100644 index 00000000000..49e29c00d47 --- /dev/null +++ b/src/main/skills/skill-delete-guards.ts @@ -0,0 +1,137 @@ +import { posix as pathPosix, win32 as pathWin32 } from 'node:path' +import { skillFileMaxDepth } from '../../shared/skill-discovery-depth' +import type { + SkillDeleteBlockReason, + SkillDeletePlacement +} from '../../shared/skill-delete-contract' +import { + skillPathDepthBelow, + skillPathsEqual, + type SkillPathSemantics +} from '../../shared/skill-path-containment' +import type { SkillScanRoot } from './skill-discovery-sources' +import type { SkillPlacementCandidate } from './skill-delete-enumeration' + +/** + * Headroom against ordinary clock and read skew, not against WSL's + * second-granularity mtime: the displayed value and the host's re-read come + * from the same `stat` on the same host, so they already agree exactly. + */ +export const SKILL_DELETE_FRESHNESS_TOLERANCE_MS = 1_000 + +export type SkillDeleteGuardContext = { + semantics: SkillPathSemantics + roots: readonly SkillScanRoot[] +} + +function pathApi(semantics: SkillPathSemantics): typeof pathPosix { + return semantics.sep === '\\' ? pathWin32 : pathPosix +} + +/** + * A request carrying no `updatedAt` fails closed. It is reachable on WSL when + * the guest `stat` fails, and "unknown" is not "unchanged" for a delete. + */ +export function isSkillDeleteFresh( + displayedUpdatedAt: number | null, + observedMtimeMs: number | null +): boolean { + if (displayedUpdatedAt === null || observedMtimeMs === null) { + return false + } + return Math.abs(displayedUpdatedAt - observedMtimeMs) <= SKILL_DELETE_FRESHNESS_TOLERANCE_MS +} + +/** The root a path belongs to, honouring depth; null when no root owns it. */ +export function owningSkillRoot( + path: string, + context: SkillDeleteGuardContext +): SkillScanRoot | null { + const api = pathApi(context.semantics) + const skillFilePath = api.join(path, 'SKILL.md') + for (const root of context.roots) { + const depth = skillPathDepthBelow(root.path, skillFilePath, context.semantics) + if (depth !== null && depth <= skillFileMaxDepth(root.sourceKind)) { + return root + } + } + return null +} + +/** + * Host-side eligibility, re-derived from where the skill actually lives rather + * than read off the row: `sourceKind` is first-scanned-root-wins, so a + * plugin-cache or `.system/` skill symlinked into a plain home root reports + * `home` on the client. The host verdict may be stricter than the badge. + * + * A canonical directory *outside* every root is deliberately not blocked here. + * The rule is "remove every placement Orca owns, and never touch anything + * outside its roots" — so a tool-managed skill linked into a repo stays + * deletable by its links, while its content is left alone. When no placement + * sits inside a root either, the plan reports `unowned` on that basis instead. + */ +export function blockedCanonicalReason( + canonicalDirectoryPath: string, + context: SkillDeleteGuardContext +): SkillDeleteBlockReason | null { + const api = pathApi(context.semantics) + const canonicalSkillFile = api.join(canonicalDirectoryPath, 'SKILL.md') + for (const root of context.roots) { + const depth = skillPathDepthBelow(root.path, canonicalSkillFile, context.semantics) + if (depth === null || depth > skillFileMaxDepth(root.sourceKind)) { + continue + } + if (root.sourceKind === 'plugin') { + return 'plugin' + } + if ( + root.sourceKind === 'home' && + api.relative(root.path, canonicalSkillFile).split(context.semantics.sep)[0] === '.system' + ) { + return 'bundled' + } + } + return null +} + +export type ClassifiedPlacement = SkillDeletePlacement & { root: SkillScanRoot } + +/** + * Each kind compares against a different realpath. Comparing every entry's own + * realpath against the file identity alone finds every `canonical` placement + * and silently zero `alias-dir` ones — the exact shape this exists to remove. + */ +export function classifySkillPlacement( + candidate: SkillPlacementCandidate, + canonicalSkillFilePath: string, + context: SkillDeleteGuardContext +): ClassifiedPlacement | null { + const api = pathApi(context.semantics) + const canonicalDirectoryPath = api.dirname(canonicalSkillFilePath) + const owningRoot = owningSkillRoot(candidate.path, context) + if (!owningRoot || skillPathsEqual(owningRoot.path, candidate.path, context.semantics)) { + return null + } + const placement = (kind: SkillDeletePlacement['kind']): ClassifiedPlacement => ({ + path: candidate.path, + kind, + rootLabel: owningRoot.label, + root: owningRoot + }) + + if (candidate.entryKind === 'symlink') { + // A symlinked directory's realpath can never equal a file path, so this + // compares against the canonical *directory*. + return candidate.directoryRealpath && + skillPathsEqual(candidate.directoryRealpath, canonicalDirectoryPath, context.semantics) + ? placement('alias-dir') + : null + } + if ( + !candidate.skillFileRealpath || + !skillPathsEqual(candidate.skillFileRealpath, canonicalSkillFilePath, context.semantics) + ) { + return null + } + return placement(candidate.skillFileKind === 'symlink' ? 'alias-file' : 'canonical') +} diff --git a/src/main/skills/skill-delete-plan.test.ts b/src/main/skills/skill-delete-plan.test.ts new file mode 100644 index 00000000000..a9121780cf4 --- /dev/null +++ b/src/main/skills/skill-delete-plan.test.ts @@ -0,0 +1,240 @@ +import { mkdir, mkdtemp, rm, stat, symlink, utimes, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import type { SkillDeleteRequest } from '../../shared/skill-delete-contract' +import { buildSkillDeletePlan } from './skill-delete-plan' +import { nativeSkillInstallFilesystem } from './skill-install-filesystem' + +const roots: string[] = [] + +afterEach(async () => { + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +async function fixture(): Promise<{ home: string }> { + const home = await mkdtemp(join(tmpdir(), 'orca-skill-delete-plan-')) + roots.push(home) + return { home } +} + +async function writeSkill(directory: string, name: string): Promise { + await mkdir(directory, { recursive: true }) + const file = join(directory, 'SKILL.md') + await writeFile(file, `---\nname: ${name}\ndescription: ${name}\n---\n\n# ${name}\n`) + return file +} + +async function request( + skillFilePath: string, + overrides: Partial = {} +): Promise { + const mtime = (await stat(skillFilePath).catch(() => null))?.mtimeMs ?? null + return { + operationId: 'op', + skills: [ + { + id: 'skill-id', + directoryPath: join(skillFilePath, '..'), + skillFilePath, + name: 'demo', + updatedAt: mtime, + ...overrides + } + ] + } +} + +function plan(home: string, deleteRequest: SkillDeleteRequest) { + return buildSkillDeletePlan({ + request: deleteRequest, + target: { kind: 'native-host', cwd: undefined }, + repos: [], + filesystem: nativeSkillInstallFilesystem, + homeDir: home + }) +} + +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')) + + 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')) + + 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') + + 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') + + 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() + const outside = join(home, 'elsewhere', 'demo') + const file = await writeSkill(outside, 'demo') + const resolved = await plan(home, await request(file)) + expect(resolved.plan.skills[0].blocked).toBe('unowned') + 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') + + 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 () => { + const { home } = await fixture() + const cached = join(home, '.codex', 'plugins', 'cache', 'pack', 'demo') + const file = await writeSkill(cached, 'demo') + await mkdir(join(home, '.agents', 'skills'), { recursive: true }) + await symlink(cached, join(home, '.agents', 'skills', 'demo'), 'dir') + + const resolved = await plan(home, await request(file)) + expect(resolved.plan.skills[0].blocked).toBe('plugin') + }) + + it('lists an overlapping root\u2019s directory once, not twice', async () => { + const { home } = await fixture() + // `~/.claude/skills` is both a home root and a repo root when the home + // directory is also the workspace; staging that path twice would fail the + // second rename and roll the whole skill back. + const canonicalDirectory = join(home, '.claude', 'skills', 'demo') + const file = await writeSkill(canonicalDirectory, 'demo') + const resolved = await buildSkillDeletePlan({ + request: await request(file), + target: { kind: 'native-host', cwd: home }, + repos: [], + filesystem: nativeSkillInstallFilesystem, + homeDir: home + }) + expect( + resolved.plan.skills[0].placements.filter( + (placement) => placement.path === canonicalDirectory + ) + ).toHaveLength(1) + }) + + it('never treats a root itself as a placement', async () => { + const { home } = await fixture() + const root = join(home, '.agents', 'skills') + // A SKILL.md directly in the root would otherwise look like a placement. + const file = await writeSkill(root, 'demo') + const resolved = await plan(home, await request(file)) + expect(resolved.plan.skills[0].placements).toEqual([]) + expect(resolved.plan.skills[0].blocked).toBe('unowned') + }) +}) + +describe('buildSkillDeletePlan freshness guard', () => { + it('reports stale when the file changed beyond the tolerance', async () => { + const { home } = await fixture() + const file = await writeSkill(join(home, '.agents', 'skills', 'demo'), 'demo') + const displayed = (await stat(file)).mtimeMs + const shifted = new Date(displayed + 60_000) + await utimes(file, shifted, shifted) + + const resolved = await plan(home, await request(file, { updatedAt: displayed })) + expect(resolved.plan.skills[0].blocked).toBe('stale') + }) + + it('fails closed on a null updatedAt rather than skipping the check', async () => { + const { home } = await fixture() + const file = await writeSkill(join(home, '.agents', 'skills', 'demo'), 'demo') + const resolved = await plan(home, await request(file, { updatedAt: null })) + expect(resolved.plan.skills[0].blocked).toBe('stale') + }) + + it('accepts a second-granularity updatedAt, as WSL discovery reports it', async () => { + const { home } = await fixture() + const file = await writeSkill(join(home, '.agents', 'skills', 'demo'), 'demo') + const seconds = Math.floor((await stat(file)).mtimeMs / 1000) * 1000 + const resolved = await plan(home, await request(file, { updatedAt: seconds })) + expect(resolved.plan.skills[0].blocked).toBeUndefined() + }) + + it('reports missing when the skill file is gone', async () => { + const { home } = await fixture() + const directory = join(home, '.agents', 'skills', 'demo') + const file = await writeSkill(directory, 'demo') + const deleteRequest = await request(file) + await rm(file) + const resolved = await plan(home, deleteRequest) + expect(resolved.plan.skills[0].blocked).toBe('missing') + }) +}) diff --git a/src/main/skills/skill-delete-plan.ts b/src/main/skills/skill-delete-plan.ts new file mode 100644 index 00000000000..8d801fb5b94 --- /dev/null +++ b/src/main/skills/skill-delete-plan.ts @@ -0,0 +1,211 @@ +import { posix as pathPosix, win32 as pathWin32 } from 'node:path' +import type { Repo } from '../../shared/repo-types' +import type { + SkillDeleteBlockReason, + SkillDeletePlan, + SkillDeletePlanEntry, + SkillDeleteRequest, + SkillDeleteTargetSkill +} from '../../shared/skill-delete-contract' +import { normalizedSkillPath, type SkillPathSemantics } from '../../shared/skill-path-containment' +import { + enumerateSkillPlacementCandidates, + requireEnumerableFilesystem +} from './skill-delete-enumeration' +import { + blockedCanonicalReason, + classifySkillPlacement, + isSkillDeleteFresh, + owningSkillRoot, + type ClassifiedPlacement, + type SkillDeleteGuardContext +} from './skill-delete-guards' +import { buildSkillDeleteRootSet } from './skill-delete-roots' +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. */ +function dedupePlacements( + placements: readonly ClassifiedPlacement[], + semantics: SkillPathSemantics +): ClassifiedPlacement[] { + const seen = new Set() + return placements.filter((placement) => { + const key = normalizedSkillPath(placement.path, semantics) + if (seen.has(key)) { + return false + } + seen.add(key) + return true + }) +} + +function blockedEntry( + skill: SkillDeleteTargetSkill, + canonicalPath: string, + blocked: SkillDeleteBlockReason +): SkillDeletePlanEntry { + return { id: skill.id, name: skill.name, canonicalPath, placements: [], blocked } +} + +export type SkillDeletePlanInput = { + request: SkillDeleteRequest + target: ResolvedSkillDiscoveryTarget + repos: readonly Repo[] + providerRootOverrides?: SkillProviderRootOverrides + filesystem: SkillInstallFilesystem + homeDir?: string +} + +/** The plan plus what execution needs but the wire contract must not carry. */ +export type ResolvedSkillDeletePlan = { + plan: SkillDeletePlan + semantics: SkillPathSemantics + toFilesystemPath: (path: string) => string + /** Root paths the filesystem must be authorized for before any operation. */ + rootPaths: string[] + placementRoots: Map +} + +/** + * One enumeration serves the whole batch, and `previewDelete` and `delete` both + * come through here. There is deliberately no second enumeration path: a + * preview promising one set of removals and a delete performing another is the + * failure this design exists to avoid. + */ +export async function buildSkillDeletePlan( + input: SkillDeletePlanInput +): Promise { + const { roots, semantics, toFilesystemPath } = await buildSkillDeleteRootSet({ + target: input.target, + repos: input.repos, + ...(input.providerRootOverrides ? { providerRootOverrides: input.providerRootOverrides } : {}), + ...(input.homeDir ? { homeDir: input.homeDir } : {}) + }) + const rootPaths = roots.map((root) => root.path) + // Widen the guest allow-list before anything touches it: the WSL filesystem's + // constructor list covers installable providers only, a strict subset of the + // discovery roots, so nearly every placement would throw before a safety rule + // ever ran. This is a second, independent containment check with its own + // semantics; it does not replace the shared path guard. + input.filesystem.authorizeRoots?.(rootPaths.map(toFilesystemPath)) + + const api = semantics.sep === '\\' ? pathWin32 : pathPosix + // Placements are literal paths inside literal roots; the canonical path is a + // realpath. Comparing the two spellings makes every skill under a symlinked + // component (a symlinked home, macOS `/tmp` -> `/private/tmp`) read `unowned`, + // so canonical classification gets roots resolved the same way it is. + const context: SkillDeleteGuardContext = { semantics, roots } + const canonicalContext: SkillDeleteGuardContext = { + semantics, + roots: await resolvedRoots(roots, input.filesystem, toFilesystemPath) + } + const requested = input.request.skills + // Inspect only paths a root already owns. A path outside every root is + // `unowned` by definition, and asking the guest filesystem about it would + // throw its own containment error before any of this reported a reason. + const inspectable = requested.filter( + (skill) => owningSkillRoot(api.dirname(skill.skillFilePath), context) !== null + ) + const inspections = await inspectRequestedSkillFiles( + inspectable, + input.filesystem, + toFilesystemPath + ) + const candidates = await enumerateSkillPlacementCandidates({ + roots, + filesystem: input.filesystem, + semantics, + toFilesystemPath + }) + + const placementRoots = new Map() + const skills = requested.map((skill): SkillDeletePlanEntry => { + const inspection = inspections.get(skill.skillFilePath) + if (!inspection) { + return blockedEntry(skill, skill.skillFilePath, 'unowned') + } + const canonicalPath = inspection.realpath + if (!canonicalPath || inspection.kind === 'missing') { + return blockedEntry(skill, skill.skillFilePath, 'missing') + } + if (!isSkillDeleteFresh(skill.updatedAt, inspection.mtimeMs)) { + return blockedEntry(skill, canonicalPath, 'stale') + } + const blocked = blockedCanonicalReason(api.dirname(canonicalPath), canonicalContext) + if (blocked) { + return blockedEntry(skill, canonicalPath, blocked) + } + const placements = dedupePlacements( + candidates + .map((candidate) => classifySkillPlacement(candidate, canonicalPath, context)) + .filter((placement): placement is ClassifiedPlacement => placement !== null), + semantics + ) + if (placements.length === 0) { + return blockedEntry(skill, canonicalPath, 'unowned') + } + placementRoots.set(skill.id, placements) + return { + id: skill.id, + name: skill.name, + canonicalPath, + placements: placements.map(({ path, kind, rootLabel }) => ({ path, kind, rootLabel })) + } + }) + + return { + plan: { operationId: input.request.operationId, skills }, + semantics, + toFilesystemPath, + rootPaths, + placementRoots + } +} + +async function resolvedRoots( + roots: readonly SkillScanRoot[], + filesystem: SkillInstallFilesystem, + toFilesystemPath: (path: string) => string +): Promise { + const inspections = await filesystem + .inspectPaths?.(roots.map((root) => toFilesystemPath(root.path))) + .catch(() => null) + return roots.map((root) => { + const realpath = inspections?.get(toFilesystemPath(root.path))?.realpath + return realpath + ? { ...root, path: fromFilesystemPath(realpath, root.path, toFilesystemPath) } + : root + }) +} + +/** Guest realpaths already come back in host-owned spelling; native ones are + * the same string space. Falls back to the literal root if they ever diverge. */ +function fromFilesystemPath( + realpath: string, + literalRoot: string, + toFilesystemPath: (path: string) => string +): string { + return toFilesystemPath(realpath) === realpath || toFilesystemPath(literalRoot) !== literalRoot + ? realpath + : literalRoot +} + +async function inspectRequestedSkillFiles( + skills: readonly SkillDeleteTargetSkill[], + filesystem: SkillInstallFilesystem, + toFilesystemPath: (path: string) => string +): ReturnType> { + const { inspectPaths } = requireEnumerableFilesystem(filesystem) + const paths = [...new Set(skills.map((skill) => skill.skillFilePath))] + const raw = await inspectPaths(paths.map(toFilesystemPath)) + return new Map( + paths.flatMap((path) => { + const inspection = raw.get(toFilesystemPath(path)) + return inspection ? [[path, inspection] as const] : [] + }) + ) +} diff --git a/src/main/skills/skill-delete-recovery.test.ts b/src/main/skills/skill-delete-recovery.test.ts new file mode 100644 index 00000000000..26bc45616c2 --- /dev/null +++ b/src/main/skills/skill-delete-recovery.test.ts @@ -0,0 +1,191 @@ +import { lstat, 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 { + readSkillDeleteRecoveryJournal, + recoverSkillDeleteTransaction, + skillDeleteJournalPath, + writeSkillDeleteJournal, + type SkillDeleteJournalV1 +} from './skill-delete-recovery' +import { writeSkillStateFile } from './skill-install-provenance' +import { skillDeleteStagedName } from './skill-staging-names' + +const roots: string[] = [] + +afterEach(async () => { + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +async function fixture(): Promise<{ home: string; stateDirectory: string; root: string }> { + const home = await mkdtemp(join(tmpdir(), 'orca-skill-delete-recovery-')) + roots.push(home) + const root = join(home, '.agents', 'skills') + await mkdir(root, { recursive: true }) + return { home, stateDirectory: join(home, 'state'), root } +} + +function journal( + root: string, + canonicalPath: string, + overrides: Partial = {} +): SkillDeleteJournalV1 { + return { + schemaVersion: 1, + operation: 'delete', + phase: 'staged', + canonicalPath, + wslDistro: null, + allowedRoots: [root], + movedCount: 1, + moves: [ + { + sourcePath: canonicalPath, + stagedPath: join(root, skillDeleteStagedName('demo', 'id-1')), + kind: 'canonical' + } + ], + ...overrides + } +} + +async function exists(path: string): Promise { + return Boolean(await lstat(path).catch(() => null)) +} + +describe('skill delete journal validation', () => { + it('round-trips a valid journal', async () => { + const { stateDirectory, root } = await fixture() + const canonicalPath = join(root, 'demo') + await writeSkillDeleteJournal(stateDirectory, journal(root, canonicalPath)) + expect(await readSkillDeleteRecoveryJournal(stateDirectory, canonicalPath)).toMatchObject({ + operation: 'delete', + phase: 'staged' + }) + }) + + it('returns null when there is no journal', async () => { + const { stateDirectory, root } = await fixture() + expect(await readSkillDeleteRecoveryJournal(stateDirectory, join(root, 'demo'))).toBeNull() + }) + + it('refuses a move whose source escapes every allowed root', async () => { + const { home, stateDirectory, root } = await fixture() + const canonicalPath = join(root, 'demo') + const escaped = join(home, 'elsewhere', 'demo') + await writeSkillStateFile( + skillDeleteJournalPath(stateDirectory, canonicalPath), + journal(root, canonicalPath, { + moves: [ + { + sourcePath: escaped, + stagedPath: join(home, 'elsewhere', skillDeleteStagedName('demo', 'id-1')), + kind: 'canonical' + } + ] + }) + ) + await expect(readSkillDeleteRecoveryJournal(stateDirectory, canonicalPath)).rejects.toThrow( + 'skill-delete-journal-invalid' + ) + }) + + it('refuses a staged path that is not our sibling naming convention', async () => { + const { stateDirectory, root } = await fixture() + const canonicalPath = join(root, 'demo') + await writeSkillStateFile( + skillDeleteJournalPath(stateDirectory, canonicalPath), + journal(root, canonicalPath, { + moves: [ + { sourcePath: canonicalPath, stagedPath: join(root, 'demo.bak'), kind: 'canonical' } + ] + }) + ) + await expect(readSkillDeleteRecoveryJournal(stateDirectory, canonicalPath)).rejects.toThrow( + 'skill-delete-journal-invalid' + ) + }) + + it('refuses a movedCount larger than the move list', async () => { + const { stateDirectory, root } = await fixture() + const canonicalPath = join(root, 'demo') + await writeSkillStateFile( + skillDeleteJournalPath(stateDirectory, canonicalPath), + journal(root, canonicalPath, { movedCount: 4 }) + ) + await expect(readSkillDeleteRecoveryJournal(stateDirectory, canonicalPath)).rejects.toThrow( + 'skill-delete-journal-invalid' + ) + }) +}) + +describe('recoverSkillDeleteTransaction', () => { + it('rolls a staged phase forward by removing the staged path', async () => { + const { stateDirectory, root } = await fixture() + const canonicalPath = join(root, 'demo') + const entry = journal(root, canonicalPath) + await mkdir(entry.moves[0].stagedPath, { recursive: true }) + await writeSkillDeleteJournal(stateDirectory, entry) + + await recoverSkillDeleteTransaction(stateDirectory, canonicalPath) + expect(await exists(entry.moves[0].stagedPath)).toBe(false) + expect(await exists(skillDeleteJournalPath(stateDirectory, canonicalPath))).toBe(false) + }) + + it('rolls an interrupted staging back so the skill survives whole', 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 writeFile(join(entry.moves[0].stagedPath, 'SKILL.md'), 'content') + await writeSkillDeleteJournal(stateDirectory, entry) + + await recoverSkillDeleteTransaction(stateDirectory, canonicalPath) + expect(await exists(canonicalPath)).toBe(true) + expect(await exists(entry.moves[0].stagedPath)).toBe(false) + 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') + const aliasRoot = join(home, '.claude', 'skills', 'demo') + await mkdir(aliasRoot, { recursive: true }) + const entry = journal(root, canonicalPath, { + phase: 'staging', + movedCount: 2, + allowedRoots: [root, join(home, '.claude', 'skills')], + // Staging order is alias-file first, then canonical, so plain reversal + // gives canonical-first restore. + moves: [ + { + sourcePath: join(aliasRoot, 'SKILL.md'), + stagedPath: join(aliasRoot, skillDeleteStagedName('SKILL.md', 'id-2')), + kind: 'alias-file' + }, + { + sourcePath: canonicalPath, + stagedPath: join(root, skillDeleteStagedName('demo', 'id-1')), + kind: 'canonical' + } + ] + }) + await mkdir(entry.moves[1].stagedPath, { recursive: true }) + await writeFile(entry.moves[0].stagedPath, 'link-stand-in') + await writeSkillDeleteJournal(stateDirectory, entry) + + const order: string[] = [] + await recoverSkillDeleteTransaction(stateDirectory, canonicalPath, { + prepareExtractedSkill: async () => undefined, + observeSkill: async () => { + throw new Error('unused') + }, + rename: async (_source, target) => { + order.push(target) + }, + remove: async () => undefined + }) + expect(order).toEqual([canonicalPath, join(aliasRoot, 'SKILL.md')]) + }) +}) diff --git a/src/main/skills/skill-delete-recovery.ts b/src/main/skills/skill-delete-recovery.ts new file mode 100644 index 00000000000..7caacb6ea07 --- /dev/null +++ b/src/main/skills/skill-delete-recovery.ts @@ -0,0 +1,167 @@ +import { rm } from 'node:fs/promises' +import { join, posix as pathPosix, win32 as pathWin32 } from 'node:path' +import { readNodeFileWithinLimit } from '../../shared/node-bounded-file-reader' +import { + SKILL_DELETE_PLACEMENT_KINDS, + type SkillDeletePlacementKind +} from '../../shared/skill-delete-contract' +import { nativeSkillPathSemantics, skillPathInside } from '../../shared/skill-path-containment' +import { + nativeSkillInstallFilesystem, + type SkillInstallFilesystem +} from './skill-install-filesystem' +import { skillInstallStateKey, writeSkillStateFile } from './skill-install-provenance' +import { isSkillDeleteStagedName } from './skill-staging-names' + +const JOURNAL_MAX_BYTES = 4 * 1024 * 1024 + +export type SkillDeleteJournalMove = { + sourcePath: string + stagedPath: string + kind: SkillDeletePlacementKind +} + +/** + * A path-based delete has no install receipt — that absence is the reason this + * feature exists — so replay is validated by containment and placement shape + * instead of a receipt digest. + * + * `wslDistro` and `allowedRoots` are not extras: at startup there is no target, + * no workspace, and no repo list to resolve either from, so a journal that + * omits them cannot be replayed on WSL and cannot be validated at all. + */ +export type SkillDeleteJournalV1 = { + schemaVersion: 1 + operation: 'delete' + phase: 'prepared' | 'staging' | 'staged' + /** + * The `canonical` placement's own literal, pre-realpath directory path — the + * same value the lock key is derived from, and the name the shared journal + * scanner cross-checks against the file name. + */ + canonicalPath: string + /** Set when the files live in a WSL distro, so startup recovery can rebuild + * the guest filesystem with no target, workspace, or repo list to resolve + * from. Every path above is already in the spelling that filesystem takes. */ + wslDistro: string | null + allowedRoots: string[] + movedCount: number + moves: SkillDeleteJournalMove[] +} + +export function skillDeleteJournalPath(stateDirectory: string, canonicalPath: string): string { + return join(stateDirectory, 'delete-journals', `${skillInstallStateKey(canonicalPath)}.json`) +} + +function validMove(move: unknown, journal: SkillDeleteJournalV1): move is SkillDeleteJournalMove { + if (typeof move !== 'object' || move === null) { + return false + } + const candidate: Partial = move + if ( + typeof candidate.sourcePath !== 'string' || + typeof candidate.stagedPath !== 'string' || + !SKILL_DELETE_PLACEMENT_KINDS.includes(candidate.kind as SkillDeletePlacementKind) + ) { + return false + } + const semantics = nativeSkillPathSemantics() + const api = semantics.sep === '\\' ? pathWin32 : pathPosix + // A sibling rename by construction: same parent, hidden name, our marker. + return ( + api.dirname(candidate.sourcePath) === api.dirname(candidate.stagedPath) && + isSkillDeleteStagedName( + api.basename(candidate.sourcePath), + api.basename(candidate.stagedPath) + ) && + journal.allowedRoots.some((root) => skillPathInside(root, candidate.sourcePath!, semantics)) + ) +} + +function isDeleteJournal(value: unknown, canonicalPath: string): value is SkillDeleteJournalV1 { + if (typeof value !== 'object' || value === null) { + return false + } + const journal: Partial = value + if ( + journal.schemaVersion !== 1 || + journal.operation !== 'delete' || + journal.canonicalPath !== canonicalPath || + (journal.phase !== 'prepared' && journal.phase !== 'staging' && journal.phase !== 'staged') || + (typeof journal.wslDistro !== 'string' && journal.wslDistro !== null) || + !Array.isArray(journal.allowedRoots) || + !journal.allowedRoots.every((root) => typeof root === 'string') || + !Array.isArray(journal.moves) || + !Number.isInteger(journal.movedCount) + ) { + return false + } + const candidate = journal as SkillDeleteJournalV1 + if (candidate.movedCount < 0 || candidate.movedCount > candidate.moves.length) { + return false + } + const sources = new Set(candidate.moves.map((move: SkillDeleteJournalMove) => move?.sourcePath)) + return ( + sources.size === candidate.moves.length && + candidate.moves.every((move) => validMove(move, candidate)) + ) +} + +export async function writeSkillDeleteJournal( + stateDirectory: string, + journal: SkillDeleteJournalV1 +): Promise { + await writeSkillStateFile(skillDeleteJournalPath(stateDirectory, journal.canonicalPath), journal) +} + +export async function readSkillDeleteRecoveryJournal( + stateDirectory: string, + canonicalPath: string +): Promise { + try { + const parsed: unknown = JSON.parse( + ( + await readNodeFileWithinLimit( + skillDeleteJournalPath(stateDirectory, canonicalPath), + JOURNAL_MAX_BYTES + ) + ).buffer.toString('utf8') + ) + if (!isDeleteJournal(parsed, canonicalPath)) { + throw new Error('skill-delete-journal-invalid') + } + return parsed + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return null + } + throw error + } +} + +export async function recoverSkillDeleteTransaction( + stateDirectory: string, + canonicalPath: string, + filesystem: SkillInstallFilesystem = nativeSkillInstallFilesystem +): Promise { + const journal = await readSkillDeleteRecoveryJournal(stateDirectory, canonicalPath) + if (!journal) { + return + } + const staged = journal.moves.slice(0, journal.movedCount) + if (journal.phase === 'staged') { + // Roll forward: the skill is already gone from every discovered location. + for (const move of staged) { + await filesystem.remove(move.stagedPath) + } + } else { + // 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. + for (let index = staged.length - 1; index >= 0; index -= 1) { + const move = staged[index] + await filesystem.rename(move.stagedPath, move.sourcePath).catch(() => undefined) + } + } + 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 new file mode 100644 index 00000000000..bf87514df01 --- /dev/null +++ b/src/main/skills/skill-delete-request-service.ts @@ -0,0 +1,66 @@ +import { join } from 'node:path' +import type { Repo } from '../../shared/repo-types' +import type { + SkillDeletePlan, + SkillDeleteRequest, + SkillDeleteResult +} from '../../shared/skill-delete-contract' +import { + deleteSkills, + previewSkillDeletion, + type SkillDeleteServiceInput +} from './skill-delete-service' +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. */ +export type SkillDeleteRequestDependencies = { + repos: () => readonly Repo[] + resolveProviderRootOverrides?: ( + target: ResolvedSkillDiscoveryTarget + ) => Promise + /** `app.getPath('userData')`; the skill-install state root hangs off it. */ + userDataPath: string +} + +async function serviceInput( + request: SkillDeleteRequest, + target: ResolvedSkillDiscoveryTarget, + dependencies: SkillDeleteRequestDependencies +): Promise { + const providerRootOverrides = await dependencies.resolveProviderRootOverrides?.(target) + return { + request, + target, + repos: dependencies.repos(), + ...(providerRootOverrides ? { providerRootOverrides } : {}), + // The allow-list starts empty on purpose: the plan authorizes the rebuilt + // discovery root set, which is wider than the installable-provider roots the + // factory would seed. + filesystem: + target.kind === 'wsl' + ? new WslSkillInstallFilesystem(target.distro, []) + : nativeSkillInstallFilesystem, + stateDirectory: join(dependencies.userDataPath, 'skill-installs'), + wslDistro: target.kind === 'wsl' ? target.distro : null + } +} + +export async function previewSkillDeleteRequest( + request: SkillDeleteRequest, + target: ResolvedSkillDiscoveryTarget, + dependencies: SkillDeleteRequestDependencies +): Promise { + return previewSkillDeletion(await serviceInput(request, target, dependencies)) +} + +export async function runSkillDeleteRequest( + request: SkillDeleteRequest, + target: ResolvedSkillDiscoveryTarget, + dependencies: SkillDeleteRequestDependencies +): Promise { + return deleteSkills(await serviceInput(request, target, dependencies)) +} diff --git a/src/main/skills/skill-delete-roots.ts b/src/main/skills/skill-delete-roots.ts new file mode 100644 index 00000000000..2309bdc9769 --- /dev/null +++ b/src/main/skills/skill-delete-roots.ts @@ -0,0 +1,74 @@ +import { homedir } from 'node:os' +import { posix as pathPosix } from 'node:path' +import type { Repo } from '../../shared/repo-types' +import { + POSIX_SKILL_PATH_SEMANTICS, + nativeSkillPathSemantics, + 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 './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 + * the discovery root set from the same three inputs `discoverSkillsOnTarget` + * uses — target, repo list, provider-root overrides. Rebuilding beats trusting + * the client: the worst case is a refusal, never a delete outside a root. + */ +export type SkillDeleteRootSet = { + roots: SkillScanRoot[] + semantics: SkillPathSemantics + /** Translates a host-owned path into the spelling the filesystem accepts. */ + toFilesystemPath: (path: string) => string +} + +export async function buildSkillDeleteRootSet(input: { + target: ResolvedSkillDiscoveryTarget + repos: readonly Repo[] + providerRootOverrides?: SkillProviderRootOverrides + homeDir?: string +}): Promise { + if (input.target.kind === 'wsl') { + const { distro, homeDir, cwd } = input.target + return { + roots: [ + ...buildSkillDiscoverySources({ + homeDir, + cwd, + repos: [], + pathApi: pathPosix, + providerRootOverrides: input.providerRootOverrides + }), + // Why tolerated: plugin metadata is enrichment. A failed read must not + // turn every skill on the host into `unowned`. + ...(await discoverClaudePluginSkillSourcesInWsl({ distro, homeDir, cwd }).catch(() => [])) + ], + // A WSL runtime is POSIX and case-sensitive even though the process is win32. + semantics: POSIX_SKILL_PATH_SEMANTICS, + toFilesystemPath: (path) => toWindowsWslPath(path, distro) + } + } + + const home = input.homeDir ?? homedir() + const cwd = input.target.cwd + return { + roots: [ + // Mirrors `discoverSkillsOnTarget`: a cwd-scoped scan drops the repo list, + // and `buildSkillDiscoverySources` adds `cwd ?? process.cwd()` on top + // either way, so `repos: []` is not the whole exclusivity rule by itself. + ...buildSkillDiscoverySources({ + homeDir: home, + ...(cwd ? { cwd } : {}), + repos: cwd ? [] : [...input.repos], + providerRootOverrides: input.providerRootOverrides + }), + ...(cwd ? await discoverClaudePluginSkillSources({ homeDir: home, cwd }).catch(() => []) : []) + ], + semantics: nativeSkillPathSemantics(), + toFilesystemPath: (path) => path + } +} diff --git a/src/main/skills/skill-delete-service.test.ts b/src/main/skills/skill-delete-service.test.ts new file mode 100644 index 00000000000..f86baa33c91 --- /dev/null +++ b/src/main/skills/skill-delete-service.test.ts @@ -0,0 +1,261 @@ +import { lstat, mkdir, mkdtemp, readdir, rm, stat, symlink, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { SkillDeleteRequest } from '../../shared/skill-delete-contract' +import { deleteSkills } from './skill-delete-service' +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 './skill-delete-recovery' + +const roots: string[] = [] + +afterEach(async () => { + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) + vi.restoreAllMocks() +}) + +async function fixture(): Promise<{ + home: string + stateDirectory: string + canonicalDirectory: string + file: string +}> { + const home = await mkdtemp(join(tmpdir(), 'orca-skill-delete-service-')) + roots.push(home) + const canonicalDirectory = join(home, '.agents', 'skills', 'demo') + await mkdir(canonicalDirectory, { recursive: true }) + const file = join(canonicalDirectory, 'SKILL.md') + await writeFile(file, '---\nname: demo\ndescription: Demo\n---\n\n# Demo\n') + return { home, stateDirectory: join(home, 'state'), canonicalDirectory, file } +} + +async function request(file: string): Promise { + return { + operationId: 'op', + skills: [ + { + id: 'skill-id', + directoryPath: join(file, '..'), + skillFilePath: file, + name: 'demo', + updatedAt: (await stat(file)).mtimeMs + } + ] + } +} + +function run( + home: string, + stateDirectory: string, + deleteRequest: SkillDeleteRequest, + filesystem: SkillInstallFilesystem = nativeSkillInstallFilesystem +) { + return deleteSkills({ + request: deleteRequest, + target: { kind: 'native-host', cwd: undefined }, + repos: [], + filesystem, + stateDirectory, + homeDir: home, + lockTimeoutMs: 50 + }) +} + +async function exists(path: string): Promise { + return Boolean(await lstat(path).catch(() => null)) +} + +describe('deleteSkills', () => { + it('removes every placement and leaves no staged residue', async () => { + const { home, stateDirectory, canonicalDirectory, file } = await fixture() + const aliasDirectory = join(home, '.claude', 'skills', 'demo') + await mkdir(join(home, '.claude', 'skills'), { recursive: true }) + await symlink(canonicalDirectory, aliasDirectory, 'dir') + + const result = await run(home, stateDirectory, await request(file)) + expect(result.skills[0].status).toBe('deleted') + expect(await exists(canonicalDirectory)).toBe(false) + expect(await exists(aliasDirectory)).toBe(false) + expect(await readdir(join(home, '.agents', 'skills'))).toEqual([]) + expect(await exists(skillDeleteJournalPath(stateDirectory, canonicalDirectory))).toBe(false) + }) + + it('removes an alias-file symlink but keeps a directory that holds anything else', async () => { + const { home, stateDirectory, file } = await fixture() + const aliasDirectory = join(home, '.codex', 'skills', 'demo') + await mkdir(aliasDirectory, { recursive: true }) + await symlink(file, join(aliasDirectory, 'SKILL.md')) + await writeFile(join(aliasDirectory, 'notes.md'), 'kept') + + const result = await run(home, stateDirectory, await request(file)) + expect(result.skills[0].status).toBe('deleted') + expect(await exists(join(aliasDirectory, 'SKILL.md'))).toBe(false) + // "Not shared with another placement" is not "contains nothing else". + expect(await exists(join(aliasDirectory, 'notes.md'))).toBe(true) + }) + + it('removes an emptied alias-file directory', async () => { + const { home, stateDirectory, file } = await fixture() + const aliasDirectory = join(home, '.codex', 'skills', 'demo') + await mkdir(aliasDirectory, { recursive: true }) + await symlink(file, join(aliasDirectory, 'SKILL.md')) + + await run(home, stateDirectory, await request(file)) + expect(await exists(aliasDirectory)).toBe(false) + }) + + it('takes the lock an install of the same destination directory would take', async () => { + const { home, stateDirectory, canonicalDirectory, file } = await fixture() + // Keyed on the placement's literal directory path, which is what install + // hashes — not the realpath'd file identity's dirname. + const release = await acquireSkillInstallLock({ + path: skillInstallLockPath(stateDirectory, canonicalDirectory) + }) + try { + const result = await run(home, stateDirectory, await request(file)) + expect(result.skills[0].status).toBe('busy') + expect(await exists(canonicalDirectory)).toBe(true) + } finally { + await release() + } + }) + + it('continues the batch when one skill is locked by another transaction', async () => { + const { home, stateDirectory, canonicalDirectory, file } = await fixture() + const otherDirectory = join(home, '.agents', 'skills', 'other') + await mkdir(otherDirectory, { recursive: true }) + const otherFile = join(otherDirectory, 'SKILL.md') + await writeFile(otherFile, '---\nname: other\ndescription: Other\n---\n') + + const release = await acquireSkillInstallLock({ + path: skillInstallLockPath(stateDirectory, canonicalDirectory) + }) + try { + const [first, second] = await Promise.all([request(file), request(otherFile)]) + const result = await run(home, stateDirectory, { + operationId: 'batch', + skills: [first.skills[0], { ...second.skills[0], id: 'other-id', name: 'other' }] + }) + expect(result.skills.map((skill) => skill.status)).toEqual(['busy', 'deleted']) + expect(await exists(otherDirectory)).toBe(false) + } finally { + await release() + } + }) + + it('restores every staged placement when a later rename fails', async () => { + const { home, stateDirectory, canonicalDirectory, file } = await fixture() + const aliasDirectory = join(home, '.claude', 'skills', 'demo') + await mkdir(join(home, '.claude', 'skills'), { recursive: true }) + await symlink(canonicalDirectory, aliasDirectory, 'dir') + + let renames = 0 + const filesystem: SkillInstallFilesystem = { + ...nativeSkillInstallFilesystem, + rename: async (source, target) => { + renames += 1 + // Fail the canonical stage, which runs last. + if (renames === 2) { + throw new Error('EBUSY') + } + return nativeSkillInstallFilesystem.rename(source, target) + } + } + + const result = await run(home, stateDirectory, await request(file), filesystem) + expect(result.skills[0].status).toBe('failed') + expect(await exists(canonicalDirectory)).toBe(true) + expect(await exists(aliasDirectory)).toBe(true) + }) + + it('reports partial with the staged paths when the rollback itself fails', async () => { + const { home, stateDirectory, canonicalDirectory, file } = await fixture() + await mkdir(join(home, '.claude', 'skills'), { recursive: true }) + await symlink(canonicalDirectory, join(home, '.claude', 'skills', 'demo'), 'dir') + + let renames = 0 + const filesystem: SkillInstallFilesystem = { + ...nativeSkillInstallFilesystem, + rename: async (source, target) => { + renames += 1 + if (renames === 1) { + return nativeSkillInstallFilesystem.rename(source, target) + } + throw new Error('EACCES') + } + } + + const result = await run(home, stateDirectory, await request(file), filesystem) + // Neither restored nor removed — reported as `partial`, never folded into + // `failed`, which would wrongly imply nothing changed. + expect(result.skills[0].status).toBe('partial') + expect(result.skills[0].stagedPaths?.length).toBe(1) + }) + + it('reports partial rather than deleted when a staged removal fails', async () => { + const { home, stateDirectory, file } = await fixture() + const filesystem: SkillInstallFilesystem = { + ...nativeSkillInstallFilesystem, + remove: async () => { + throw new Error('EACCES') + } + } + const result = await run(home, stateDirectory, await request(file), filesystem) + expect(result.skills[0].status).toBe('partial') + expect(result.skills[0].stagedPaths?.length).toBe(1) + }) + + it('deletes the install receipt so no phantom managed install survives', async () => { + const { home, stateDirectory, canonicalDirectory, file } = await fixture() + await writeSkillInstallReceipt(stateDirectory, { + schemaVersion: 1, + packageId: 'package_1', + versionId: 'version_1', + packageDigest: 'a'.repeat(64), + archiveSha256: 'b'.repeat(64), + scope: 'global', + destinationIdentity: 'identity', + canonicalPath: canonicalDirectory, + placements: [], + installedAt: new Date(0).toISOString(), + hostIdentity: 'host' + }) + await run(home, stateDirectory, await request(file)) + expect(await readSkillInstallReceipt(stateDirectory, canonicalDirectory)).toBeNull() + }) + + it('removes a link to outside content and leaves the content itself intact', async () => { + const { home, stateDirectory } = await fixture() + const managedElsewhere = join(home, '.local', 'share', 'devex', 'skills', 'linked') + await mkdir(managedElsewhere, { recursive: true }) + await writeFile( + join(managedElsewhere, 'SKILL.md'), + '---\nname: linked\ndescription: Linked\n---\n' + ) + const link = join(home, '.claude', 'skills', 'linked') + await mkdir(join(home, '.claude', 'skills'), { recursive: true }) + await symlink(managedElsewhere, link, 'dir') + + const result = await run(home, stateDirectory, await request(join(link, 'SKILL.md'))) + expect(result.skills[0].status).toBe('deleted') + expect(await exists(link)).toBe(false) + // The whole point: Orca removed its own pointer, not the tool's content. + expect(await exists(join(managedElsewhere, 'SKILL.md'))).toBe(true) + }) + + it('skips a blocked skill without touching disk', async () => { + const { home, stateDirectory, file } = await fixture() + const deleteRequest = await request(file) + const result = await run(home, stateDirectory, { + ...deleteRequest, + skills: [{ ...deleteRequest.skills[0], updatedAt: 1 }] + }) + expect(result.skills[0]).toMatchObject({ status: 'skipped', blocked: 'stale' }) + expect(await exists(file)).toBe(true) + }) +}) diff --git a/src/main/skills/skill-delete-service.ts b/src/main/skills/skill-delete-service.ts new file mode 100644 index 00000000000..a4873d57655 --- /dev/null +++ b/src/main/skills/skill-delete-service.ts @@ -0,0 +1,202 @@ +import { rm } from 'node:fs/promises' +import { posix as pathPosix, win32 as pathWin32 } from 'node:path' +import type { Repo } from '../../shared/repo-types' +import type { + SkillDeletePlan, + SkillDeletePlanEntry, + SkillDeleteRequest, + SkillDeleteResult, + SkillDeleteResultEntry +} from '../../shared/skill-delete-contract' +import { nativeSkillPathSemantics } from '../../shared/skill-path-containment' +import { buildSkillDeletePlan, type ResolvedSkillDeletePlan } from './skill-delete-plan' +import { + recoverSkillDeleteTransaction, + skillDeleteJournalPath, + writeSkillDeleteJournal, + type SkillDeleteJournalMove, + type SkillDeleteJournalV1 +} from './skill-delete-recovery' +import { + planSkillDeleteMoves, + removeStagedSkillDeleteMoves, + stageSkillDeleteMoves +} from './skill-delete-staging' +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 + target: ResolvedSkillDiscoveryTarget + repos: readonly Repo[] + providerRootOverrides?: SkillProviderRootOverrides + filesystem: SkillInstallFilesystem + /** `/skill-installs` — the same state root install and remove use. */ + stateDirectory: string + wslDistro?: string | null + homeDir?: string + /** Mirrors install's own knob; the shared 5s default applies when unset. */ + lockTimeoutMs?: number +} + +export async function previewSkillDeletion( + input: SkillDeleteServiceInput +): Promise { + return (await buildSkillDeletePlan(input)).plan +} + +/** + * Recomputes the plan rather than trusting a client-supplied one, and executes + * it per skill: one lock, one journal, stage → remove → receipt cleanup. A + * failure is confined to its own skill; the rest of the batch continues. + */ +export async function deleteSkills(input: SkillDeleteServiceInput): Promise { + const resolved = await buildSkillDeletePlan(input) + const skills: SkillDeleteResultEntry[] = [] + for (const entry of resolved.plan.skills) { + skills.push( + entry.blocked + ? { + id: entry.id, + name: entry.name, + status: 'skipped', + blocked: entry.blocked, + removedPaths: [] + } + : await deleteOneSkill(entry, resolved, input) + ) + } + // Once per batch, on the executing host: the target coalescer, the per-root + // cache, and the last-known-good retention would otherwise serve a pre-delete + // answer and the row visibly comes back. + clearSkillDiscoveryCaches() + recordSkillDeleteOperation({ + operationId: resolved.plan.operationId, + hostKind: input.target.kind, + skills, + // Root *ids* only. A repo or plugin id is already a hash, where a label + // carries the repo name and a path carries the user's directory names. + rootIds: [ + ...new Set( + [...resolved.placementRoots.values()].flatMap((placements) => + placements.map((placement) => placement.root.id) + ) + ) + ] + }) + return { operationId: resolved.plan.operationId, skills } +} + +async function deleteOneSkill( + entry: SkillDeletePlanEntry, + resolved: ResolvedSkillDeletePlan, + input: SkillDeleteServiceInput +): Promise { + const api = nativeSkillPathSemantics().sep === '\\' ? pathWin32 : pathPosix + const canonical = entry.placements.find((placement) => placement.kind === 'canonical') + // The lock key is the canonical placement's own LITERAL directory path, in the + // filesystem's spelling — not `canonicalPath`'s dirname. Install hashes the + // literal destination without realpath'ing it, so a symlinked component (a + // symlinked home, macOS `/tmp`) would hash to a different, uncontended lock. + // + // With no canonical placement (a tool-managed skill linked in from outside the + // roots) no install could have created one, so any stable key excludes + // correctly; prefer a path Orca owns over the out-of-root canonical. + const lockKeyPath = resolved.toFilesystemPath( + canonical?.path ?? entry.placements[0]?.path ?? api.dirname(entry.canonicalPath) + ) + let releaseLock: (() => Promise) | undefined + try { + releaseLock = await acquireSkillInstallLock({ + path: skillInstallLockPath(input.stateDirectory, lockKeyPath), + ...(input.lockTimeoutMs === undefined ? {} : { timeoutMs: input.lockTimeoutMs }) + }) + } catch { + // Someone else is mid-transaction on this exact skill. The rest of the batch + // is unaffected. + return { id: entry.id, name: entry.name, status: 'busy', removedPaths: [] } + } + try { + return await runStagedDeletion(entry, resolved, input, lockKeyPath) + } catch { + await recoverSkillDeleteTransaction(input.stateDirectory, lockKeyPath, input.filesystem).catch( + () => undefined + ) + return { id: entry.id, name: entry.name, status: 'failed', removedPaths: [] } + } finally { + await releaseLock() + } +} + +async function runStagedDeletion( + entry: SkillDeletePlanEntry, + resolved: ResolvedSkillDeletePlan, + input: SkillDeleteServiceInput, + lockKeyPath: string +): Promise { + const moves = planSkillDeleteMoves(entry.placements, resolved.toFilesystemPath) + const journal = ( + phase: SkillDeleteJournalV1['phase'], + movedCount: number + ): SkillDeleteJournalV1 => ({ + schemaVersion: 1, + operation: 'delete', + phase, + canonicalPath: lockKeyPath, + wslDistro: input.wslDistro ?? null, + allowedRoots: resolved.rootPaths.map(resolved.toFilesystemPath), + movedCount, + moves + }) + // Written before anything is renamed: a crash between staging and cleanup must + // not orphan a directory nothing will ever revisit. + await writeSkillDeleteJournal(input.stateDirectory, journal('prepared', 0)) + + const staging = await stageSkillDeleteMoves({ + moves, + filesystem: input.filesystem, + onProgress: (movedCount) => + writeSkillDeleteJournal(input.stateDirectory, journal('staging', movedCount)) + }) + if (staging.status !== 'staged') { + await rm(skillDeleteJournalPath(input.stateDirectory, lockKeyPath), { force: true }) + return staging.status === 'partial' + ? { + id: entry.id, + name: entry.name, + status: 'partial', + removedPaths: [], + stagedPaths: staging.stagedPaths + } + : { id: entry.id, name: entry.name, status: 'failed', removedPaths: [] } + } + + await writeSkillDeleteJournal(input.stateDirectory, journal('staged', moves.length)) + const removal = await removeStagedSkillDeleteMoves(moves, input.filesystem) + await removeSkillInstallReceipt(input.stateDirectory, lockKeyPath).catch(() => undefined) + await rm(skillDeleteJournalPath(input.stateDirectory, lockKeyPath), { force: true }) + return { + id: entry.id, + name: entry.name, + // Staging fully succeeded, so the skill is gone from every discovered + // location — but an unremoved staged path still occupies disk. + status: removal.unremoved.length > 0 ? 'partial' : 'deleted', + removedPaths: sourcePathsFor(moves, removal.removedPaths), + ...(removal.unremoved.length > 0 ? { stagedPaths: removal.unremoved } : {}) + } +} + +function sourcePathsFor( + moves: readonly SkillDeleteJournalMove[], + removedSourcePaths: readonly string[] +): string[] { + const removed = new Set(removedSourcePaths) + return moves.filter((move) => removed.has(move.sourcePath)).map((move) => move.sourcePath) +} diff --git a/src/main/skills/skill-delete-staging.ts b/src/main/skills/skill-delete-staging.ts new file mode 100644 index 00000000000..d6bbbc2edde --- /dev/null +++ b/src/main/skills/skill-delete-staging.ts @@ -0,0 +1,145 @@ +import { randomUUID } from 'node:crypto' +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 './skill-delete-recovery' +import type { SkillInstallFilesystem } from './skill-install-filesystem' +import { SKILL_FILE_NAME } from './skill-root-file-walk' +import { skillDeleteStagedName } from './skill-staging-names' + +function api(): typeof pathPosix { + return nativeSkillPathSemantics().sep === '\\' ? pathWin32 : pathPosix +} + +/** + * Aliases stage before the canonical directory, so plain reversal of the move + * list is exactly the rollback order the design requires: canonical restored + * first, then `alias-dir`, then `alias-file` symlinks last — a restored alias + * never points at a canonical directory that does not exist yet. + */ +const STAGING_ORDER: Record = { + 'alias-file': 0, + 'alias-dir': 1, + canonical: 2 +} + +/** + * A staged sibling shares one parent directory with its source by construction, + * so the rename cannot fail with `EXDEV` — there is no volume to cross. The one + * failure that survives is staging a placement that is itself a mount point, + * which fails with `EBUSY` and is reported per placement. + */ +export function planSkillDeleteMoves( + placements: readonly SkillDeletePlacement[], + toFilesystemPath: (path: string) => string +): SkillDeleteJournalMove[] { + const path = api() + return [...placements] + .sort((left, right) => STAGING_ORDER[left.kind] - STAGING_ORDER[right.kind]) + .map((placement) => { + // An `alias-file` stages only the symlinked SKILL.md, in place. Its + // directory is never staged, because it has to survive when it holds + // anything besides that symlink. + const source = + placement.kind === 'alias-file' + ? path.join(toFilesystemPath(placement.path), SKILL_FILE_NAME) + : toFilesystemPath(placement.path) + return { + sourcePath: source, + stagedPath: path.join( + path.dirname(source), + skillDeleteStagedName(path.basename(source), randomUUID()) + ), + kind: placement.kind + } + }) +} + +export type SkillDeleteStagingOutcome = + | { status: 'staged' } + | { status: 'rolled-back'; error: unknown } + | { status: 'partial'; error: unknown; stagedPaths: string[] } + +export async function stageSkillDeleteMoves(input: { + moves: readonly SkillDeleteJournalMove[] + filesystem: SkillInstallFilesystem + onProgress: (movedCount: number) => Promise +}): Promise { + const staged: SkillDeleteJournalMove[] = [] + for (const move of input.moves) { + // Record the intent before the rename, so a crash in between leaves a move + // recovery treats as possibly-done rather than orphaning the directory. + await input.onProgress(staged.length + 1) + try { + await input.filesystem.rename(move.sourcePath, move.stagedPath) + staged.push(move) + } catch (error) { + const unrestored = await rollbackSkillDeleteMoves(staged, input.filesystem) + return unrestored.length > 0 + ? { status: 'partial', error, stagedPaths: unrestored } + : { status: 'rolled-back', error } + } + } + return { status: 'staged' } +} + +/** Returns the staged paths that could not be put back. */ +export async function rollbackSkillDeleteMoves( + staged: readonly SkillDeleteJournalMove[], + filesystem: SkillInstallFilesystem +): Promise { + const unrestored: string[] = [] + // Indexed rather than `toReversed()`: the delete modules reach the Node 18 + // relay bundle, where the ES2023 array-copy methods do not exist. + for (let index = staged.length - 1; index >= 0; index -= 1) { + const move = staged[index] + try { + await filesystem.rename(move.stagedPath, move.sourcePath) + } catch { + // Neither fully restored nor fully removed. Reported as `partial` with the + // staged path so the result band can say where the files actually are, + // rather than `failed`, which would wrongly imply nothing changed. + unrestored.push(move.stagedPath) + } + } + return unrestored +} + +export type SkillDeleteRemovalOutcome = { removedPaths: string[]; unremoved: string[] } + +export async function removeStagedSkillDeleteMoves( + moves: readonly SkillDeleteJournalMove[], + filesystem: SkillInstallFilesystem +): Promise { + const path = api() + const removedPaths: string[] = [] + const unremoved: string[] = [] + for (const move of moves) { + try { + await filesystem.remove(move.stagedPath) + removedPaths.push(move.sourcePath) + if (move.kind === 'alias-file') { + await removeDirectoryIfEmpty(path.dirname(move.sourcePath), filesystem) + } + } catch { + unremoved.push(move.stagedPath) + } + } + return { removedPaths, unremoved } +} + +/** Matches the model's guarantee: a directory holding anything besides the + * removed symlink is left in place, because "not shared with another + * placement" is not the same guarantee as "contains nothing else". */ +async function removeDirectoryIfEmpty( + directory: string, + filesystem: SkillInstallFilesystem +): Promise { + if (!filesystem.listEntries) { + return + } + const listing = await filesystem.listEntries([directory]).catch(() => null) + if (listing?.get(directory)?.length === 0) { + await filesystem.remove(directory).catch(() => undefined) + } +} diff --git a/src/main/skills/skill-discovery-wsl.ts b/src/main/skills/skill-discovery-wsl.ts index 6923066268e..28c7579ea20 100644 --- a/src/main/skills/skill-discovery-wsl.ts +++ b/src/main/skills/skill-discovery-wsl.ts @@ -17,6 +17,8 @@ import { } from './skill-discovery-sources' import { discoverClaudePluginSkillSourcesInWsl } from './claude-plugin-skill-sources-wsl' import type { SkillProviderRootOverrides } from './skill-provider-destinations' +import { SKILL_STAGING_GLOB } from './skill-staging-names' +import { skillFileMaxDepth } from '../../shared/skill-discovery-depth' const MAX_MARKDOWN_BYTES = 256 * 1024 const WSL_SCAN_TIMEOUT_MS = 10_000 @@ -42,11 +44,11 @@ export function buildWslSkillDiscoveryCommand(roots: readonly SkillScanRoot[]): ` printf '%s\\0%s\\0%s\\0%s\\0%s\\0' S "$root_index" "$skill_file" "$canonical_path" "$updated_at"`, ` printf '%s' "$encoded_markdown"`, ` printf '\\0'`, - ` done < <(find -L "$root_path" -mindepth 1 -maxdepth "$max_depth" -type f -name 'SKILL.md' -print0 2>/dev/null)`, + ` done < <(find -L "$root_path" -mindepth 1 -maxdepth "$max_depth" \\( -name '${SKILL_STAGING_GLOB}' -prune \\) -o \\( -type f -name 'SKILL.md' -print0 \\) 2>/dev/null)`, '}' ] roots.forEach((root, index) => { - const maxDepth = root.sourceKind === 'plugin' ? 10 : 5 + const maxDepth = skillFileMaxDepth(root.sourceKind) lines.push(`scan_root ${index} ${quoteBashString(root.path)} ${maxDepth}`) }) return lines.join('\n') diff --git a/src/main/skills/skill-install-filesystem.ts b/src/main/skills/skill-install-filesystem.ts index 67ab5d6d36f..d58ec51ea92 100644 --- a/src/main/skills/skill-install-filesystem.ts +++ b/src/main/skills/skill-install-filesystem.ts @@ -1,6 +1,8 @@ -import { rm } from 'node:fs/promises' +import type { Dirent, Stats } from 'node:fs' +import { lstat, readdir, realpath, rm, stat } from 'node:fs/promises' import type { SkillPackageManifestV1 } from '../../shared/skill-package-manifest' import { renameSkillPathWithWindowsRetry } from './skill-filesystem-retry' +import { runSkillCandidateTasks } from './skill-candidate-concurrency' import { SKILL_PACKAGE_OBSERVATION_LIMITS, observeSkillPackage, @@ -9,8 +11,36 @@ import { export type SkillInstalledFileMode = { path: string; executable: boolean } +/** `lstat` semantics: a symlink is `symlink` whatever it points at. */ +export type SkillFilesystemEntryKind = 'directory' | 'file' | 'symlink' | 'other' | 'missing' + +export type SkillDirectoryEntry = { name: string; kind: SkillFilesystemEntryKind } + +export type SkillPathInspection = { + kind: SkillFilesystemEntryKind + /** Fully resolved path; null when it cannot be resolved (broken link, gone). */ + realpath: string | null + /** + * Modification time in ms, read the same way the host's own discovery read it + * so a freshness comparison stays self-consistent: native `stat` (follows a + * symlink), WSL's `stat -c %Y` (does not). Null when it cannot be read. + */ + mtimeMs: number | null +} + export type SkillInstallFilesystem = { authorizeRoots?(paths: readonly string[]): void + /** + * Enumeration primitives for path-based deletion. Both take arrays because a + * guest or remote filesystem must answer a whole delete plan in one round + * trip — the WSL home alone contributes ~16 discovery roots, and one + * `wsl.exe` boot per root is the cost this shape exists to avoid. + * + * Optional: only implementations that can enumerate provide them, and the + * delete service refuses rather than guessing when they are absent. + */ + listEntries?(directories: readonly string[]): Promise> + inspectPaths?(paths: readonly string[]): Promise> prepareExtractedSkill(path: string, manifest: SkillPackageManifestV1): Promise observeSkill( path: string, @@ -33,5 +63,58 @@ export const nativeSkillInstallFilesystem: SkillInstallFilesystem = { : undefined ), rename: renameSkillPathWithWindowsRetry, - remove: (path) => rm(path, { recursive: true, force: true }) + remove: (path) => rm(path, { recursive: true, force: true }), + // Why the bounded pool rather than `Promise.all`: a delete plan enumerates + // every discovery root, and unbounded fan-out here is the same burst of + // filesystem-metadata work discovery already learned to cap. + listEntries: async (directories) => + new Map( + await runSkillCandidateTasks( + directories.map((directory) => async (): Promise<[string, SkillDirectoryEntry[]]> => { + const entries = await readdir(directory, { withFileTypes: true }).catch(() => null) + return [ + directory, + (entries ?? []).map((entry) => ({ name: entry.name, kind: direntKind(entry) })) + ] + }) + ) + ), + inspectPaths: async (paths) => + new Map( + await runSkillCandidateTasks( + paths.map( + (path) => async (): Promise<[string, SkillPathInspection]> => [ + path, + { + kind: statKind(await lstat(path).catch(() => null)), + realpath: await realpath(path).catch(() => null), + mtimeMs: (await stat(path).catch(() => null))?.mtimeMs ?? null + } + ] + ) + ) + ) +} + +function direntKind(entry: Dirent): SkillFilesystemEntryKind { + if (entry.isSymbolicLink()) { + return 'symlink' + } + if (entry.isDirectory()) { + return 'directory' + } + return entry.isFile() ? 'file' : 'other' +} + +function statKind(value: Stats | null): SkillFilesystemEntryKind { + if (!value) { + return 'missing' + } + if (value.isSymbolicLink()) { + return 'symlink' + } + if (value.isDirectory()) { + return 'directory' + } + return value.isFile() ? 'file' : 'other' } diff --git a/src/main/skills/skill-operation-observability.ts b/src/main/skills/skill-operation-observability.ts index 7bcc9718cca..bf4a3f49c6e 100644 --- a/src/main/skills/skill-operation-observability.ts +++ b/src/main/skills/skill-operation-observability.ts @@ -15,6 +15,9 @@ import { skillInstallFailureFromError } from './skill-install-operation-error' const SAFE_LABEL = /^[a-z0-9][a-z0-9._-]{0,63}$/i const MAX_OBSERVED_COUNT = 1_000_000 +// Roots grow with the repo count, so an uncapped id list makes one log line +// grow with the install. +const MAX_LOGGED_DELETE_ROOT_IDS = 12 const MAX_OBSERVED_BYTES = 64 * 1024 * 1024 type SkillOperationPhase = @@ -279,3 +282,36 @@ export function startSkillBundleInstallOperation( } } } + +/** + * One summary line per delete batch. Root *ids* are safe to log where labels + * and paths are not — a repo or plugin id is already a hash — matching + * discovery's own logging rule. + */ +export function recordSkillDeleteOperation(input: { + operationId: string + hostKind: 'native-host' | 'wsl' + skills: readonly { status: string }[] + rootIds: readonly string[] +}): void { + const counts = new Map() + for (const skill of input.skills) { + counts.set(skill.status, (counts.get(skill.status) ?? 0) + 1) + } + const span = startSpan('skill.delete', { + attributes: { + phase: 'delete', + platform: platformLabel(), + host: safeLabel(input.hostKind, 'unknown'), + skillCount: boundedNumber(input.skills.length), + ...Object.fromEntries( + [...counts].map(([status, count]) => [ + `${safeLabel(status, 'unknown')}Count`, + boundedNumber(count) + ]) + ), + rootIds: input.rootIds.slice(0, MAX_LOGGED_DELETE_ROOT_IDS).join(',') + } + }) + span.end() +} diff --git a/src/main/skills/skill-removable-placement.ts b/src/main/skills/skill-removable-placement.ts index a4b5b66590b..369864ea608 100644 --- a/src/main/skills/skill-removable-placement.ts +++ b/src/main/skills/skill-removable-placement.ts @@ -1,22 +1,21 @@ import { lstat, readlink } from 'node:fs/promises' -import { basename, dirname, isAbsolute, join, relative, resolve } from 'node:path' +import { basename, dirname, join, resolve } from 'node:path' +import { + nativeSkillPathSemantics, + normalizedSkillPath, + skillPathInside +} from '../../shared/skill-path-containment' import type { SkillPlacementResult } from '../../shared/skill-install-contract' import type { SkillInstallFilesystem } from './skill-install-filesystem' import type { SkillInstallReceiptV1 } from './skill-install-provenance' +// Native-only call site: these placements are always the process's own paths. function normalizedPath(path: string): string { - const normalized = resolve(path) - return process.platform === 'win32' ? normalized.toLocaleLowerCase('en-US') : normalized + return normalizedSkillPath(path, nativeSkillPathSemantics()) } function pathInside(root: string, path: string): boolean { - const child = relative(resolve(root), resolve(path)) - return ( - child !== '' && - child !== '..' && - !child.startsWith(`..${process.platform === 'win32' ? '\\' : '/'}`) && - !isAbsolute(child) - ) + return skillPathInside(root, path, nativeSkillPathSemantics()) } async function aliasTargetsCanonical(path: string, canonicalPath: string): Promise { diff --git a/src/main/skills/skill-remove-recovery.ts b/src/main/skills/skill-remove-recovery.ts index d3d5c0c2e74..4f4bcf401d0 100644 --- a/src/main/skills/skill-remove-recovery.ts +++ b/src/main/skills/skill-remove-recovery.ts @@ -1,5 +1,10 @@ import { lstat, readlink, rm } from 'node:fs/promises' -import { basename, dirname, isAbsolute, join, relative, resolve } from 'node:path' +import { basename, dirname, join, resolve } from 'node:path' +import { + nativeSkillPathSemantics, + normalizedSkillPath, + skillPathInside +} from '../../shared/skill-path-containment' import { readNodeFileWithinLimit } from '../../shared/node-bounded-file-reader' import type { SkillPlacementResult } from '../../shared/skill-install-contract' import type { SkillInstallFilesystem } from './skill-install-filesystem' @@ -47,19 +52,13 @@ function validBackup(move: RemovalMove): boolean { ) } +// Native-only call site: a journal's paths are always the process's own. function normalizedPath(path: string): string { - const normalized = resolve(path) - return process.platform === 'win32' ? normalized.toLocaleLowerCase('en-US') : normalized + return normalizedSkillPath(path, nativeSkillPathSemantics()) } function pathInside(root: string, path: string): boolean { - const child = relative(resolve(root), resolve(path)) - return ( - child !== '' && - child !== '..' && - !child.startsWith(`..${process.platform === 'win32' ? '\\' : '/'}`) && - !isAbsolute(child) - ) + return skillPathInside(root, path, nativeSkillPathSemantics()) } function moveOwnedByJournal(journal: Partial, move: RemovalMove): boolean { diff --git a/src/main/skills/skill-root-file-walk.ts b/src/main/skills/skill-root-file-walk.ts index 91c5ee66b51..cd87ebb7dc4 100644 --- a/src/main/skills/skill-root-file-walk.ts +++ b/src/main/skills/skill-root-file-walk.ts @@ -1,6 +1,7 @@ import type { Dirent } from 'node:fs' import { readdir, realpath, stat } from 'node:fs/promises' import { isAbsolute, join, relative, sep } from 'node:path' +import { isSkillStagingEntryName } from './skill-staging-names' export const SKILL_FILE_NAME = 'SKILL.md' @@ -62,6 +63,11 @@ export async function findSkillFiles( } for (const entry of entries) { signal?.throwIfAborted() + // Why: a staged sibling sits directly in a scanned root, so without this a + // skill mid-transaction surfaces as a second, separately actionable row. + if (isSkillStagingEntryName(entry.name)) { + continue + } const entryPath = join(dirPath, entry.name) if (entry.name === SKILL_FILE_NAME) { if (entry.isFile()) { diff --git a/src/main/skills/skill-staging-names.ts b/src/main/skills/skill-staging-names.ts new file mode 100644 index 00000000000..605ba466c32 --- /dev/null +++ b/src/main/skills/skill-staging-names.ts @@ -0,0 +1,33 @@ +/** + * Every skill transaction stages a path as a hidden sibling before touching it, + * and those siblings land directly in a scanned discovery root. Discovery must + * not surface them, or a skill mid-transaction shows up as a second row (and, + * for delete, as a row the user can delete again). + * + * The four conventions in the tree today: + * `..orca-skill-delete-` (skill-delete-service) + * `..orca-remove-backup-` (skill-remove-transaction) + * `..orca-placement-backup-` (skill-placement-transaction-controller) + * `..orca-placement-staging-` (skill-placement-transaction-controller) + * + * Matched by shape rather than by an enumerated list so the native walker and + * WSL's `find -prune` (which can only express a glob) cannot drift apart. + */ +export const SKILL_DELETE_STAGING_MARKER = '.orca-skill-delete-' + +/** The `find -name` glob the WSL guest script prunes on. */ +export const SKILL_STAGING_GLOB = '.*.orca-*' + +const SKILL_STAGING_NAME = /^\..+\.orca-/u + +export function isSkillStagingEntryName(name: string): boolean { + return SKILL_STAGING_NAME.test(name) +} + +export function skillDeleteStagedName(basename: string, id: string): string { + return `.${basename}${SKILL_DELETE_STAGING_MARKER}${id}` +} + +export function isSkillDeleteStagedName(basename: string, staged: string): boolean { + return staged.startsWith(`.${basename}${SKILL_DELETE_STAGING_MARKER}`) +} diff --git a/src/main/skills/skill-staging-visibility.test.ts b/src/main/skills/skill-staging-visibility.test.ts new file mode 100644 index 00000000000..5b9cfee48ef --- /dev/null +++ b/src/main/skills/skill-staging-visibility.test.ts @@ -0,0 +1,82 @@ +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 './skill-discovery-wsl' +import { findSkillFiles } from './skill-root-file-walk' +import { + isSkillStagingEntryName, + skillDeleteStagedName, + SKILL_STAGING_GLOB +} from './skill-staging-names' + +const roots: string[] = [] + +afterEach(async () => { + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +async function stagedRoot(stagedName: string): Promise { + const root = await mkdtemp(join(tmpdir(), 'orca-skill-staging-visibility-')) + roots.push(root) + const staged = join(root, stagedName) + await mkdir(staged, { recursive: true }) + await writeFile(join(staged, 'SKILL.md'), '---\nname: demo\n---\n') + return root +} + +describe('isSkillStagingEntryName', () => { + it.each([ + '.demo.orca-skill-delete-1234', + '.demo.orca-remove-backup-1234', + '.demo.orca-placement-backup-1234', + '.demo.orca-placement-staging-1234' + ])('matches the %s convention', (name) => { + expect(isSkillStagingEntryName(name)).toBe(true) + }) + + it.each(['demo', '.hidden-skill', '..cache', '.orca'])('leaves %s alone', (name) => { + expect(isSkillStagingEntryName(name)).toBe(false) + }) +}) + +describe('native walker', () => { + it.each([ + skillDeleteStagedName('demo', 'id'), + '.demo.orca-remove-backup-id', + '.demo.orca-placement-backup-id' + ])('does not surface a %s staged directly in a scanned root', async (stagedName) => { + const root = await stagedRoot(stagedName) + expect(await findSkillFiles(root, 4)).toEqual([]) + }) + + it('still finds an ordinary skill beside a staged one', async () => { + const root = await stagedRoot(skillDeleteStagedName('demo', 'id')) + const real = join(root, 'demo') + await mkdir(real) + await writeFile(join(real, 'SKILL.md'), '---\nname: demo\n---\n') + expect(await findSkillFiles(root, 4)).toEqual([join(real, 'SKILL.md')]) + }) +}) + +describe('WSL guest scan', () => { + it('prunes the same staging shapes the native walker skips', () => { + // The command is base64-wrapped, so assert on the decoded script. + const command = buildWslSkillDiscoveryCommand([ + { + id: 'home-agents', + label: 'Agent skills home', + path: '/home/u/.agents/skills', + sourceKind: 'home', + providers: ['agent-skills'], + owner: null + } + ]) + const encoded = command.match(/printf %s '([^']+)'/)?.[1] + expect(encoded).toBeTruthy() + const script = Buffer.from(encoded ?? '', 'base64').toString('utf8') + expect(script).toContain(`-name '${SKILL_STAGING_GLOB}' -prune`) + // The prune must gate the print, not sit beside it. + expect(script).toContain("-prune \\) -o \\( -type f -name 'SKILL.md' -print0 \\)") + }) +}) diff --git a/src/main/skills/skill-transaction-startup-recovery.ts b/src/main/skills/skill-transaction-startup-recovery.ts index ab8928f9fd9..eba17ca9a53 100644 --- a/src/main/skills/skill-transaction-startup-recovery.ts +++ b/src/main/skills/skill-transaction-startup-recovery.ts @@ -19,6 +19,10 @@ import { import { startSkillPhaseOperation } from './skill-operation-observability' import { WslSkillInstallFilesystem } from './skill-wsl-install-filesystem' import { readSkillPlacementRecoveryJournal } from './skill-placement-recovery-journal' +import { + readSkillDeleteRecoveryJournal, + recoverSkillDeleteTransaction +} from './skill-delete-recovery' import { recoverSkillPlacementTransaction } from './skill-placement-transaction' const MAX_PENDING_TRANSACTION_JOURNALS = 64 @@ -30,6 +34,7 @@ type PendingTransaction = { install: boolean removal: boolean placement: boolean + delete: boolean } export type SkillTransactionStartupRecoveryReport = { @@ -49,7 +54,7 @@ function failureCode(error: unknown): string { async function scanJournalDirectory( stateDirectory: string, - directoryName: 'journals' | 'removal-journals' | 'placement-journals' + directoryName: 'journals' | 'removal-journals' | 'placement-journals' | 'delete-journals' ): Promise<{ candidates: { canonicalPath: string; journalKey: string }[] failures: { journalKey: string; code: string }[] @@ -109,14 +114,15 @@ function pendingTransactions( const pending = new Map() const add = ( candidate: { canonicalPath: string; journalKey: string }, - kind: 'install' | 'removal' | 'placement' + kind: 'install' | 'removal' ): void => { const current = pending.get(candidate.canonicalPath) ?? { canonicalPath: candidate.canonicalPath, journalKey: candidate.journalKey, install: false, removal: false, - placement: false + placement: false, + delete: false } current[kind] = true pending.set(candidate.canonicalPath, current) @@ -126,13 +132,38 @@ function pendingTransactions( return [...pending.values()] } +/** Merged after the map-based dedupe so a kind that shares a canonical path with + * an install or removal joins that transaction rather than racing it. */ +function mergeJournalCandidates( + pending: PendingTransaction[], + candidates: readonly { canonicalPath: string; journalKey: string }[], + kind: 'placement' | 'delete' +): void { + for (const candidate of candidates) { + const current = pending.find((entry) => entry.canonicalPath === candidate.canonicalPath) + if (current) { + current[kind] = true + continue + } + pending.push({ + ...candidate, + install: false, + removal: false, + placement: false, + delete: false, + [kind]: true + }) + } +} + async function recoverPendingSkillTransactionsUnobserved( stateDirectory: string ): Promise { - const [installs, removals, placements, extractions, locks] = await Promise.all([ + const [installs, removals, placements, deletes, extractions, locks] = await Promise.all([ scanJournalDirectory(stateDirectory, 'journals'), scanJournalDirectory(stateDirectory, 'removal-journals'), scanJournalDirectory(stateDirectory, 'placement-journals'), + scanJournalDirectory(stateDirectory, 'delete-journals'), recoverPendingSkillExtractions(stateDirectory), reclaimDeadSkillInstallLocks(stateDirectory) ]) @@ -141,6 +172,7 @@ async function recoverPendingSkillTransactionsUnobserved( installs.candidates.length + removals.candidates.length + placements.candidates.length + + deletes.candidates.length + extractions.scanned, recovered: extractions.recovered, orphanedExtractionsRecovered: extractions.recovered, @@ -149,24 +181,20 @@ async function recoverPendingSkillTransactionsUnobserved( ...installs.failures, ...removals.failures, ...placements.failures, + ...deletes.failures, ...extractions.failures ], truncated: installs.truncated || removals.truncated || placements.truncated || + deletes.truncated || extractions.truncated || locks.truncated } const pending = pendingTransactions(installs.candidates, removals.candidates) - for (const placement of placements.candidates) { - const current = pending.find((candidate) => candidate.canonicalPath === placement.canonicalPath) - if (current) { - current.placement = true - } else { - pending.push({ ...placement, install: false, removal: false, placement: true }) - } - } + mergeJournalCandidates(pending, placements.candidates, 'placement') + mergeJournalCandidates(pending, deletes.candidates, 'delete') for (const pendingTransaction of pending) { let releaseLock: (() => Promise) | null = null try { @@ -182,11 +210,15 @@ async function recoverPendingSkillTransactionsUnobserved( const placementJournal = pendingTransaction.placement ? await readSkillPlacementRecoveryJournal(stateDirectory, pendingTransaction.canonicalPath) : null + const deleteJournal = pendingTransaction.delete + ? await readSkillDeleteRecoveryJournal(stateDirectory, pendingTransaction.canonicalPath) + : null const distros = new Set( [ installJournal?.receipt.wslDistro, removalJournal?.receipt.wslDistro, - placementJournal?.wslDistro + placementJournal?.wslDistro, + deleteJournal?.wslDistro ].filter((distro): distro is string => Boolean(distro)) ) if (distros.size > 1 || (distros.size && process.platform !== 'win32')) { @@ -200,6 +232,12 @@ async function recoverPendingSkillTransactionsUnobserved( ...(placementJournal?.actions.map((action) => action.rootPath) ?? []) ]) : undefined + // Why explicit rather than only widening the constructor list: the delete + // journal is the sole record of which roots its moves are allowed to + // touch, and replay must be authorized for them before it operates. + if (deleteJournal) { + filesystem?.authorizeRoots(deleteJournal.allowedRoots) + } if (removalJournal) { await recoverSkillRemovalTransaction( stateDirectory, @@ -224,6 +262,14 @@ async function recoverPendingSkillTransactionsUnobserved( ) report.recovered += 1 } + if (deleteJournal) { + await recoverSkillDeleteTransaction( + stateDirectory, + pendingTransaction.canonicalPath, + filesystem + ) + report.recovered += 1 + } } catch (error) { report.failures.push({ journalKey: pendingTransaction.journalKey, diff --git a/src/main/skills/skill-wsl-delete.integration.test.ts b/src/main/skills/skill-wsl-delete.integration.test.ts new file mode 100644 index 00000000000..5fa4f4fb966 --- /dev/null +++ b/src/main/skills/skill-wsl-delete.integration.test.ts @@ -0,0 +1,119 @@ +import { execFile } from 'node:child_process' +import { promisify } from 'node:util' +import { afterAll, beforeAll, describe, expect, it } from 'vitest' +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 + }) + return stdout.trim() +} + +function uncPath(guestPath: string): string { + return `\\\\wsl.localhost\\${DISTRO}${guestPath.replaceAll('/', '\\')}` +} + +/** + * Enumeration and deletion stay server-side in the distro for the same reason + * install does: node's `fs` over a `\\wsl$` path applies Windows semantics to + * POSIX symlinks and permissions. + */ +describe.runIf(RUN_REAL_WSL)('real WSL skill deletion primitives', () => { + let guestRoot = '' + let skillsRoot = '' + + beforeAll(async () => { + guestRoot = await runWsl('mktemp', '-d', '/tmp/orca-skill-delete.XXXXXX') + if (!guestRoot.startsWith('/tmp/orca-skill-delete.')) { + throw new Error('unexpected-wsl-integration-root') + } + skillsRoot = `${guestRoot}/home/.agents/skills` + await runWsl('mkdir', '-p', `${skillsRoot}/demo`, `${guestRoot}/home/.codex/skills/demo`) + await runWsl('sh', '-c', `printf '%s' 'x' > ${skillsRoot}/demo/SKILL.md`) + // An alias-dir in another provider root, and an alias-file inside a real one. + await runWsl('mkdir', '-p', `${guestRoot}/home/.claude/skills`) + await runWsl('ln', '-sT', `${skillsRoot}/demo`, `${guestRoot}/home/.claude/skills/demo`) + await runWsl( + 'ln', + '-sT', + `${skillsRoot}/demo/SKILL.md`, + `${guestRoot}/home/.codex/skills/demo/SKILL.md` + ) + // Deliberately differs only by case, to prove POSIX case-sensitivity. + await runWsl('mkdir', '-p', `${guestRoot}/home/.agents/Skills`) + }) + + afterAll(async () => { + if (guestRoot.startsWith('/tmp/orca-skill-delete.')) { + await runWsl('rm', '-rf', '--', guestRoot) + } + }) + + it('refuses an unauthorized root before authorizeRoots, and permits it after', async () => { + const filesystem = new WslSkillInstallFilesystem(DISTRO, []) + await expect(filesystem.listEntries([uncPath(skillsRoot)])).rejects.toThrow( + 'skill-install-wsl-path-outside-root' + ) + filesystem.authorizeRoots([uncPath(skillsRoot)]) + const listing = await filesystem.listEntries([uncPath(skillsRoot)]) + expect(listing.get(uncPath(skillsRoot))).toContainEqual({ name: 'demo', kind: 'directory' }) + }) + + it('keeps containment case-sensitive, as the guest filesystem is', async () => { + const filesystem = new WslSkillInstallFilesystem(DISTRO, [ + uncPath(`${guestRoot}/home/.agents/Skills`) + ]) + await expect(filesystem.remove(uncPath(`${skillsRoot}/demo`))).rejects.toThrow( + 'skill-install-wsl-path-outside-root' + ) + }) + + it('classifies alias-dir and alias-file from one batched inspection', async () => { + const filesystem = new WslSkillInstallFilesystem(DISTRO, [uncPath(`${guestRoot}/home`)]) + const aliasDir = uncPath(`${guestRoot}/home/.claude/skills/demo`) + const aliasFile = uncPath(`${guestRoot}/home/.codex/skills/demo/SKILL.md`) + const canonicalFile = uncPath(`${skillsRoot}/demo/SKILL.md`) + const inspections = await filesystem.inspectPaths([aliasDir, aliasFile, canonicalFile]) + + expect(inspections.get(aliasDir)).toMatchObject({ + kind: 'symlink', + realpath: `${skillsRoot}/demo` + }) + expect(inspections.get(aliasFile)).toMatchObject({ + kind: 'symlink', + realpath: `${skillsRoot}/demo/SKILL.md` + }) + expect(inspections.get(canonicalFile)).toMatchObject({ + kind: 'file', + realpath: `${skillsRoot}/demo/SKILL.md` + }) + }) + + it('reads mtime without dereferencing, so an alias-file records the link itself', async () => { + const filesystem = new WslSkillInstallFilesystem(DISTRO, [uncPath(`${guestRoot}/home`)]) + const aliasFile = uncPath(`${guestRoot}/home/.codex/skills/demo/SKILL.md`) + const inspection = (await filesystem.inspectPaths([aliasFile])).get(aliasFile) + const linkSeconds = Number.parseInt( + await runWsl('stat', '-c', '%Y', '--', `${guestRoot}/home/.codex/skills/demo/SKILL.md`), + 10 + ) + expect(inspection?.mtimeMs).toBe(linkSeconds * 1000) + }) + + it('renames a placement to a hidden sibling and removes it', async () => { + const filesystem = new WslSkillInstallFilesystem(DISTRO, [uncPath(`${guestRoot}/home`)]) + const source = `${skillsRoot}/demo` + const staged = `${skillsRoot}/.demo.orca-skill-delete-integration` + await filesystem.rename(uncPath(source), uncPath(staged)) + expect(await runWsl('sh', '-c', `[ -d '${staged}' ] && printf yes`)).toBe('yes') + await filesystem.remove(uncPath(staged)) + expect(await runWsl('sh', '-c', `[ -e '${staged}' ] || printf gone`)).toBe('gone') + }) +}) diff --git a/src/main/skills/skill-wsl-enumeration-protocol.test.ts b/src/main/skills/skill-wsl-enumeration-protocol.test.ts new file mode 100644 index 00000000000..045b391dfcf --- /dev/null +++ b/src/main/skills/skill-wsl-enumeration-protocol.test.ts @@ -0,0 +1,92 @@ +import { describe, expect, it } from 'vitest' +import { + parseWslInspectPathsOutput, + parseWslListEntriesOutput, + WslEnumerationProtocolError, + WSL_INSPECT_PATHS_SCRIPT, + WSL_LIST_ENTRIES_SCRIPT +} from './skill-wsl-enumeration-protocol' + +const NUL = '\0' + +function records(...fields: string[]): string { + return fields.map((field) => `${field}${NUL}`).join('') +} + +describe('parseWslListEntriesOutput', () => { + it('keys listings by the caller’s own strings, not the guest spelling', () => { + const directories = [ + '\\\\wsl.localhost\\Ubuntu\\home\\u\\.agents\\skills', + '\\\\wsl.localhost\\Ubuntu\\home\\u\\.claude\\skills' + ] + const output = records('D', '0', 'E', 'demo', 'directory', 'E', 'link', 'symlink', 'D', '1') + const listings = parseWslListEntriesOutput(output, directories) + expect(listings.get(directories[0])).toEqual([ + { name: 'demo', kind: 'directory' }, + { name: 'link', kind: 'symlink' } + ]) + expect(listings.get(directories[1])).toEqual([]) + }) + + it('reports an absent directory as an empty listing rather than a missing key', () => { + const listings = parseWslListEntriesOutput('', ['/a', '/b']) + expect([...listings.keys()]).toEqual(['/a', '/b']) + expect(listings.get('/a')).toEqual([]) + }) + + it('refuses an unknown record kind instead of guessing', () => { + expect(() => parseWslListEntriesOutput(records('X', '0'), ['/a'])).toThrow( + WslEnumerationProtocolError + ) + }) + + it('refuses an index no argument produced', () => { + expect(() => parseWslListEntriesOutput(records('D', '7'), ['/a'])).toThrow( + WslEnumerationProtocolError + ) + }) +}) + +describe('parseWslInspectPathsOutput', () => { + it('scales the guest’s whole-second mtime the way WSL discovery does', () => { + const output = records('P', '0', 'file', '/home/u/.agents/skills/demo/SKILL.md', '1700000000') + const inspections = parseWslInspectPathsOutput(output, ['/host/path']) + expect(inspections.get('/host/path')).toEqual({ + kind: 'file', + realpath: '/home/u/.agents/skills/demo/SKILL.md', + mtimeMs: 1_700_000_000_000 + }) + }) + + it('reports an unresolvable realpath and an unreadable mtime as null', () => { + const inspections = parseWslInspectPathsOutput(records('P', '0', 'symlink', '', ''), ['/p']) + expect(inspections.get('/p')).toEqual({ kind: 'symlink', realpath: null, mtimeMs: null }) + }) + + it('refuses an unknown entry kind', () => { + expect(() => + parseWslInspectPathsOutput(records('P', '0', 'socket', '/x', '1'), ['/p']) + ).toThrow(WslEnumerationProtocolError) + }) +}) + +describe('guest scripts', () => { + it('classify a symlink before following it, so a broken link is not "missing"', () => { + expect(WSL_INSPECT_PATHS_SCRIPT.indexOf('-L "$path"')).toBeLessThan( + WSL_INSPECT_PATHS_SCRIPT.indexOf('-d "$path"') + ) + expect(WSL_LIST_ENTRIES_SCRIPT.indexOf('-L "$entry"')).toBeLessThan( + WSL_LIST_ENTRIES_SCRIPT.indexOf('-d "$entry"') + ) + }) + + it('reads mtime without dereferencing, matching WSL discovery', () => { + expect(WSL_INSPECT_PATHS_SCRIPT).toContain(`stat -c '%Y' -- "$path"`) + expect(WSL_INSPECT_PATHS_SCRIPT).not.toContain('stat -L') + }) + + it('takes every path positionally rather than interpolating it into the script', () => { + expect(WSL_LIST_ENTRIES_SCRIPT).toContain('for dir in "$@"; do') + expect(WSL_INSPECT_PATHS_SCRIPT).toContain('for path in "$@"; do') + }) +}) diff --git a/src/main/skills/skill-wsl-enumeration-protocol.ts b/src/main/skills/skill-wsl-enumeration-protocol.ts new file mode 100644 index 00000000000..57e4c1294e6 --- /dev/null +++ b/src/main/skills/skill-wsl-enumeration-protocol.ts @@ -0,0 +1,139 @@ +import type { + SkillDirectoryEntry, + SkillFilesystemEntryKind, + SkillPathInspection +} from './skill-install-filesystem' + +/** + * Guest-side enumeration for path-based skill deletion. + * + * Both scripts take every path as a positional argument and emit one + * NUL-delimited stream, because the delete plan needs the whole discovery root + * set at once and one `wsl.exe` boot per root is the cost this shape avoids. + * Records are keyed by argument index rather than by path so the caller rebuilds + * its map under the exact strings it passed in, with no path round-tripping. + */ +export const WSL_LIST_ENTRIES_SCRIPT = [ + 'set -u', + 'index=0', + 'for dir in "$@"; do', + ` printf 'D\\0%s\\0' "$index"`, + ' index=$((index + 1))', + ' [ -d "$dir" ] || continue', + ' for entry in "$dir"/* "$dir"/.*; do', + ' name=${entry##*/}', + ' case "$name" in "*"|".*"|"."|"..") continue ;; esac', + ' if [ -L "$entry" ]; then kind=symlink', + ' elif [ -d "$entry" ]; then kind=directory', + ' elif [ -f "$entry" ]; then kind=file', + ' elif [ -e "$entry" ]; then kind=other', + ' else continue; fi', + ` printf 'E\\0%s\\0%s\\0' "$name" "$kind"`, + ' done', + 'done' +].join('\n') + +export const WSL_INSPECT_PATHS_SCRIPT = [ + 'set -u', + 'index=0', + 'for path in "$@"; do', + ' if [ -L "$path" ]; then kind=symlink', + ' elif [ -d "$path" ]; then kind=directory', + ' elif [ -f "$path" ]; then kind=file', + ' elif [ -e "$path" ]; then kind=other', + ' else kind=missing; fi', + ` resolved=$(realpath -- "$path" 2>/dev/null || printf '')`, + // Deliberately no `-L`: WSL discovery reads mtime the same way, so the + // freshness guard compares like with like on this host. + ` updated=$(stat -c '%Y' -- "$path" 2>/dev/null || printf '')`, + ` printf 'P\\0%s\\0%s\\0%s\\0%s\\0' "$index" "$kind" "$resolved" "$updated"`, + ' index=$((index + 1))', + 'done' +].join('\n') + +export class WslEnumerationProtocolError extends Error { + constructor() { + super('skill-install-wsl-guest-operation-failed') + this.name = 'WslEnumerationProtocolError' + } +} + +function splitRecords(output: string): string[] { + const fields = output.split('\0') + // A trailing delimiter always leaves one empty field; anything else is data. + if (fields.at(-1) === '') { + fields.pop() + } + return fields +} + +function isEntryKind(value: string | undefined): value is SkillFilesystemEntryKind { + return ( + value === 'directory' || + value === 'file' || + value === 'symlink' || + value === 'other' || + value === 'missing' + ) +} + +export function parseWslListEntriesOutput( + output: string, + directories: readonly string[] +): Map { + const listings = new Map( + directories.map((directory) => [directory, []]) + ) + const fields = splitRecords(output) + let current: SkillDirectoryEntry[] | undefined + let index = 0 + while (index < fields.length) { + const record = fields[index++] + if (record === 'D') { + const directory = directories[Number.parseInt(fields[index++] ?? '', 10)] + if (directory === undefined) { + throw new WslEnumerationProtocolError() + } + current = listings.get(directory) + continue + } + if (record !== 'E') { + throw new WslEnumerationProtocolError() + } + const name = fields[index++] + const kind = fields[index++] + if (name === undefined || !isEntryKind(kind)) { + throw new WslEnumerationProtocolError() + } + current?.push({ name, kind }) + } + return listings +} + +export function parseWslInspectPathsOutput( + output: string, + paths: readonly string[] +): Map { + const inspections = new Map() + const fields = splitRecords(output) + let index = 0 + while (index < fields.length) { + if (fields[index++] !== 'P') { + throw new WslEnumerationProtocolError() + } + const path = paths[Number.parseInt(fields[index++] ?? '', 10)] + const kind = fields[index++] + const resolved = fields[index++] + const updated = Number.parseInt(fields[index++] ?? '', 10) + if (path === undefined || !isEntryKind(kind) || resolved === undefined) { + throw new WslEnumerationProtocolError() + } + inspections.set(path, { + kind, + realpath: resolved === '' ? null : resolved, + // `stat -c %Y` is whole seconds, matching how WSL discovery scales it. + mtimeMs: Number.isFinite(updated) ? updated * 1000 : null + }) + } + return inspections +} diff --git a/src/main/skills/skill-wsl-install-filesystem.ts b/src/main/skills/skill-wsl-install-filesystem.ts index 447b94efdfa..1c2aef97680 100644 --- a/src/main/skills/skill-wsl-install-filesystem.ts +++ b/src/main/skills/skill-wsl-install-filesystem.ts @@ -6,7 +6,18 @@ import { observeSkillPackage, type ObservedSkillPackage } from './skill-package-identity' -import type { SkillInstallFilesystem, SkillInstalledFileMode } from './skill-install-filesystem' +import type { + SkillDirectoryEntry, + SkillInstallFilesystem, + SkillInstalledFileMode, + SkillPathInspection +} from './skill-install-filesystem' +import { + parseWslInspectPathsOutput, + parseWslListEntriesOutput, + WSL_INSPECT_PATHS_SCRIPT, + WSL_LIST_ENTRIES_SCRIPT +} from './skill-wsl-enumeration-protocol' import { SKILL_INSTALL_PROVIDERS } from '../../shared/skill-install-providers' import type { SkillProviderRootOverrides } from './skill-provider-destinations' import { runWslProcess } from '../wsl/wsl-runner' @@ -14,6 +25,10 @@ import { runWslProcess } from '../wsl/wsl-runner' const BATCH_FILE_COUNT = 8 const GUEST_COMMAND_TIMEOUT_MS = 30_000 const GUEST_COMMAND_MAX_OUTPUT_BYTES = 64 * 1024 +// Why far above the default: one batched `listEntries` covers every discovery +// root of a WSL home at once, which is orders of magnitude more output than any +// install call. Matched to the discovery scanner's own bound. +const ENUMERATION_MAX_BUFFER_BYTES = 128 * 1024 * 1024 function isWindowsBackedGuestPath(path: string): boolean { return /^\/mnt\/[a-z](?:\/|$)/iu.test(path) @@ -149,6 +164,34 @@ export class WslSkillInstallFilesystem implements SkillInstallFilesystem { ) } + async listEntries(directories: readonly string[]): Promise> { + if (directories.length === 0) { + return new Map() + } + return parseWslListEntriesOutput( + await this.runOutput( + WSL_LIST_ENTRIES_SCRIPT, + directories.map((directory) => this.requireReadable(directory)), + ENUMERATION_MAX_BUFFER_BYTES + ), + directories + ) + } + + async inspectPaths(paths: readonly string[]): Promise> { + if (paths.length === 0) { + return new Map() + } + return parseWslInspectPathsOutput( + await this.runOutput( + WSL_INSPECT_PATHS_SCRIPT, + paths.map((path) => this.requireReadable(path)), + ENUMERATION_MAX_BUFFER_BYTES + ), + paths + ) + } + private async applyModes(entries: { path: string; mode: string }[]): Promise { for (let offset = 0; offset < entries.length; offset += BATCH_FILE_COUNT) { await this.run( @@ -206,11 +249,30 @@ export class WslSkillInstallFilesystem implements SkillInstallFilesystem { return guestPath } + /** Enumeration also has to reach an authorized root *itself*, which + * `requireAllowed` refuses because it demands strict containment. Read-only, + * so admitting the root adds no mutation surface. */ + private requireReadable(path: string): string { + const guestPath = this.toGuestPath(path) + const allowed = this.guestAllowedRoots.some((root) => { + const child = posix.relative(root, guestPath) + return child !== '..' && !child.startsWith('../') && !posix.isAbsolute(child) + }) + if (!allowed) { + throw new Error('skill-install-wsl-path-outside-root') + } + return guestPath + } + private async run(script: string, args: string[]): Promise { await this.runOutput(script, args) } - private async runOutput(script: string, args: string[]): Promise { + private async runOutput( + script: string, + args: string[], + maxOutputBytes = GUEST_COMMAND_MAX_OUTPUT_BYTES + ): Promise { const result = await runWslProcess({ distro: this.distro, loginPath: 'none', @@ -219,7 +281,7 @@ export class WslSkillInstallFilesystem implements SkillInstallFilesystem { shell: 'sh', args, timeoutMs: GUEST_COMMAND_TIMEOUT_MS, - maxOutputBytes: GUEST_COMMAND_MAX_OUTPUT_BYTES + maxOutputBytes }) if (result.code !== 0 || result.timedOut) { throw Object.assign(new Error('skill-install-wsl-guest-operation-failed'), { diff --git a/src/preload/api/agent-skill-api.ts b/src/preload/api/agent-skill-api.ts index 04eb45b682b..c42cbd358b0 100644 --- a/src/preload/api/agent-skill-api.ts +++ b/src/preload/api/agent-skill-api.ts @@ -32,6 +32,12 @@ import type { SkillUpdateStartResult } from '../../shared/skill-freshness' +import type { + SkillDeletePlan, + SkillDeleteRequest, + SkillDeleteResult +} from '../../shared/skill-delete-contract' + export type SkillsApi = { discover: (target?: SkillDiscoveryTarget) => Promise freshnessInventory: () => Promise @@ -65,6 +71,8 @@ export type SkillsApi = { input: SkillBundleInstallPreviewInput ) => Promise removeInstall: (input: SkillRemoveInput) => Promise + previewDelete: (request: SkillDeleteRequest) => Promise + delete: (request: SkillDeleteRequest) => Promise listManagedInstalls: (environmentId?: string) => Promise getPackage: (packageId: string) => Promise> listOwnedShares: () => Promise> diff --git a/src/preload/index.ts b/src/preload/index.ts index 4c30124a6da..e57bfc586a9 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -3,6 +3,11 @@ import { contextBridge, ipcRenderer, webFrame, webUtils } from 'electron' import { electronAPI } from '@electron-toolkit/preload' import { preloadE2EConfig } from './e2e-config' import { glApi } from './gitlab' +import type { + SkillDeletePlan, + SkillDeleteRequest, + SkillDeleteResult +} from '../shared/skill-delete-contract' import type { AppIdentity } from '../shared/app-identity' import type { MacCapturedDigitRowChord } from '../shared/macos-symbolic-hotkeys' import type { ComputerAwakeStatus } from '../shared/computer-awake-mode' @@ -2628,6 +2633,10 @@ const api = { ipcRenderer.invoke('skills:previewBundleInstall', input), removeInstall: (input: SkillRemoveInput): Promise => ipcRenderer.invoke('skills:removeInstall', input), + previewDelete: (request: SkillDeleteRequest): Promise => + ipcRenderer.invoke('skills:previewDelete', request), + delete: (request: SkillDeleteRequest): Promise => + ipcRenderer.invoke('skills:delete', request), listManagedInstalls: (environmentId?: string): Promise => ipcRenderer.invoke('skills:listManagedInstalls', environmentId), getPackage: (packageId: string): Promise> => diff --git a/src/renderer/src/components/skills/SkillDeleteResultBand.tsx b/src/renderer/src/components/skills/SkillDeleteResultBand.tsx new file mode 100644 index 00000000000..c22acd0b69b --- /dev/null +++ b/src/renderer/src/components/skills/SkillDeleteResultBand.tsx @@ -0,0 +1,47 @@ +import { X } from 'lucide-react' +import { Button } from '@/components/ui/button' +import { cn } from '@/lib/utils' +import { translate } from '@/i18n/i18n' +import type { SkillDeleteResult } from '../../../../shared/skill-delete-contract' +import { skillDeleteResultLines } from './skill-delete-copy' +import { SKILLS_PAGE_COLUMN } from './skills-page-column' + +/** + * Persistent rather than a toast: a toast disappears before a user can act on + * "3 failed", and the lines are grouped per reason, which the flat four-string + * install status component cannot render. + */ +export function SkillDeleteResultBand({ + result, + onDismiss +}: { + result: SkillDeleteResult + onDismiss: () => void +}): React.JSX.Element | null { + const lines = skillDeleteResultLines(result.skills) + if (lines.length === 0) { + return null + } + return ( +
+
+
    + {lines.map((line) => ( +
  • + {line.label} +
    • + ))} +
+ +
+
+ ) +} diff --git a/src/renderer/src/components/skills/SkillDetailDialog.tsx b/src/renderer/src/components/skills/SkillDetailDialog.tsx index 703d2fad96e..06f143fdbe0 100644 --- a/src/renderer/src/components/skills/SkillDetailDialog.tsx +++ b/src/renderer/src/components/skills/SkillDetailDialog.tsx @@ -1,4 +1,4 @@ -import { ClipboardCopy, FolderOpen, Share2 } from 'lucide-react' +import { ClipboardCopy, FolderOpen, Share2, Trash2 } from 'lucide-react' import { toast } from 'sonner' import { Badge } from '@/components/ui/badge' import { Button } from '@/components/ui/button' @@ -10,6 +10,7 @@ import { DialogHeader, DialogTitle } from '@/components/ui/dialog' +import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' import { translate } from '@/i18n/i18n' import type { DiscoveredSkill } from '../../../../shared/skills' import { sourceKindLabel } from './skill-display-labels' @@ -54,14 +55,22 @@ export function SkillDetailDialog({ skill, agentByRootPath, shareable, + deletable, + deleteDisabledReason, onOpenChange, - onShare + onShare, + onDelete }: { skill: DiscoveredSkill | null agentByRootPath: ReadonlyMap shareable: boolean + deletable: boolean + /** Why a reason string rather than share's bare boolean: delete is valid on a + * remote host, so "can't" always has a specific cause worth showing. */ + deleteDisabledReason: string | null onOpenChange: (open: boolean) => void onShare: () => void + onDelete: () => void }): React.JSX.Element | null { if (!skill) { return null @@ -148,6 +157,28 @@ export function SkillDetailDialog({ {translate('auto.components.skills.SkillsPage.dc4c3328ee', 'Reveal file')} + + + + + + + {deleteDisabledReason && !deletable ? ( + + {deleteDisabledReason} + + ) : null} + - {actions.map((action) => ( - - {action.icon} - {action.label} - - ))} + - {actions.map((action) => ( - - {action.icon} - {action.label} - - ))} + ) diff --git a/src/renderer/src/components/skills/SkillRowActions.tsx b/src/renderer/src/components/skills/SkillRowActions.tsx new file mode 100644 index 00000000000..fb98401eb91 --- /dev/null +++ b/src/renderer/src/components/skills/SkillRowActions.tsx @@ -0,0 +1,88 @@ +import React from 'react' +import { ContextMenuItem } from '@/components/ui/context-menu' +import { DropdownMenuItem } from '@/components/ui/dropdown-menu' +import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' + +export type SkillRowAction = { + key: string + label: string + icon: React.JSX.Element + disabled?: boolean + /** Why an item-level reason: the existing row badge is shown only in selection + * mode, and a disabled `Delete…` has to explain itself outside it too. */ + disabledReason?: string + destructive?: boolean + onSelect: () => void +} + +/** + * Radix disables pointer events on a disabled item, so the tooltip has to fire + * on a wrapper that keeps them. + */ +function withReason(action: SkillRowAction, item: React.JSX.Element): React.JSX.Element { + if (!action.disabled || !action.disabledReason) { + return item + } + return ( + + + {item} + + + {action.disabledReason} + + + ) +} + +export function SkillRowDropdownActions({ + actions +}: { + actions: readonly SkillRowAction[] +}): React.JSX.Element { + return ( + <> + {actions.map((action) => ( + + {withReason( + action, + + {action.icon} + {action.label} + + )} + + ))} + + ) +} + +export function SkillRowContextActions({ + actions +}: { + actions: readonly SkillRowAction[] +}): React.JSX.Element { + return ( + <> + {actions.map((action) => ( + + {withReason( + action, + + {action.icon} + {action.label} + + )} + + ))} + + ) +} diff --git a/src/renderer/src/components/skills/SkillsList.tsx b/src/renderer/src/components/skills/SkillsList.tsx index 3efce6c56fd..50e83f7df3f 100644 --- a/src/renderer/src/components/skills/SkillsList.tsx +++ b/src/renderer/src/components/skills/SkillsList.tsx @@ -8,6 +8,7 @@ import { selectedShareSkillNameKeys, skillShareEligibilityReason } from './skill-share-selection' +import { isSkillDeleteEligible, skillDeleteEligibilityReason } from './skill-delete-selection' const OPTION_SELECTOR = '[role="option"]' @@ -28,19 +29,28 @@ export function SkillsList({ agentByRootPath, selectedIds, selectionMode, + deleteSupported, + deleteUnsupportedReason, onSelectedChange, onSelectResults, - onShare + onShare, + onDelete }: { skills: readonly DiscoveredSkill[] allSkills: readonly DiscoveredSkill[] local: boolean agentByRootPath: ReadonlyMap selectedIds: ReadonlySet - selectionMode: boolean + selectionMode: 'share' | 'delete' | null + /** False while the runtime target is unresolved or the host predates + * `skills.delete.v1`; the action disables with a reason rather than issuing + * an RPC nothing answers. */ + deleteSupported: boolean + deleteUnsupportedReason: string | null onSelectedChange: (skillId: string, selected: boolean) => void onSelectResults: (results: readonly DiscoveredSkill[]) => void onShare: (skill: DiscoveredSkill) => void + onDelete: (skill: DiscoveredSkill) => void }): React.JSX.Element { const listRef = useRef(null) // Why: an id survives filtering; an index would silently anchor a shift-click @@ -53,6 +63,9 @@ export function SkillsList({ const isMac = navigator.userAgent.includes('Mac') + const deleteReasonFor = (skill: DiscoveredSkill): string | null => + deleteSupported ? skillDeleteEligibilityReason(skill) : deleteUnsupportedReason + const handleSelection = (index: number, selected: boolean, range: boolean): void => { const skill = skills[index] const anchorIndex = skills.findIndex((candidate) => candidate.id === anchorIdRef.current) @@ -95,6 +108,8 @@ export function SkillsList({ skill={detailSkill} agentByRootPath={agentByRootPath} shareable={detailSkill ? isSkillShareEligible(detailSkill, local) : false} + deletable={detailSkill ? deleteSupported && isSkillDeleteEligible(detailSkill) : false} + deleteDisabledReason={detailSkill ? deleteReasonFor(detailSkill) : null} onOpenChange={(open) => { if (!open) { setDetailSkill(null) @@ -106,36 +121,57 @@ export function SkillsList({ setDetailSkill(null) } }} + onDelete={() => { + if (detailSkill) { + onDelete(detailSkill) + setDetailSkill(null) + } + }} />
{skills.map((skill, index) => { const duplicateNameSelected = !selectedIds.has(skill.id) && selectedNames.has(skill.name.toLocaleLowerCase('en-US')) - const eligible = isSkillShareEligible(skill, local) + const shareEligible = isSkillShareEligible(skill, local) + const deleteEligible = isSkillDeleteEligible(skill) + const deleting = selectionMode === 'delete' return ( setFocusedId(skill.id)} onOpenDetail={() => setDetailSkill(skill)} onSelectionChange={(selected, range) => handleSelection(index, selected, range)} onShare={() => onShare(skill)} + onDelete={() => onDelete(skill)} onKeyDown={onListKeyDown} /> ) diff --git a/src/renderer/src/components/skills/SkillsPage.test.tsx b/src/renderer/src/components/skills/SkillsPage.test.tsx index 75b5101032c..22ec5632407 100644 --- a/src/renderer/src/components/skills/SkillsPage.test.tsx +++ b/src/renderer/src/components/skills/SkillsPage.test.tsx @@ -9,6 +9,7 @@ import type { DiscoveredSkill, SkillDiscoveryResult } from '../../../../shared/s import { createCompatibleRuntimeStatusResponseIfNeeded } from '@/runtime/runtime-compatibility-test-fixture' import { clearRuntimeCompatibilityCacheForTests } from '@/runtime/runtime-rpc-client' import { TooltipProvider } from '@/components/ui/tooltip' +import { ConfirmationDialogProvider } from '@/components/confirmation-dialog' import { useAppStore } from '@/store' import SkillsPage from './SkillsPage' @@ -65,7 +66,9 @@ async function renderPage(): Promise { await act(async () => { root?.render( - + + + ) }) diff --git a/src/renderer/src/components/skills/SkillsPage.tsx b/src/renderer/src/components/skills/SkillsPage.tsx index 142c8ddcd6b..8c573c86087 100644 --- a/src/renderer/src/components/skills/SkillsPage.tsx +++ b/src/renderer/src/components/skills/SkillsPage.tsx @@ -1,4 +1,5 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react' +import { Share2, Trash2 } from 'lucide-react' import { cn } from '@/lib/utils' import { useAppStore } from '@/store' import { discoverSkillsForRuntimeTarget } from '@/runtime/runtime-skills-client' @@ -28,13 +29,25 @@ import { SkillSharedLinksView } from './SkillSharedLinksView' import { useOwnedSkillShares } from './use-owned-skill-shares' import type { SkillsPageView } from './skills-page-view' import { translate } from '@/i18n/i18n' -import { INSTALLED_AGENT_SKILLS_CHANGED_EVENT } from '@/hooks/installed-agent-skills-change-event' +import { + INSTALLED_AGENT_SKILLS_CHANGED_EVENT, + INSTALLED_AGENT_SKILLS_REFRESHED_EVENT +} from '@/hooks/installed-agent-skills-change-event' import { addShareableSkillResults, eligibleShareSkillCount, retainedShareableSkillSelection, updatedSkillSelection } from './skill-share-selection' +import { + addDeletableSkillResults, + eligibleDeleteSkillCount, + retainedDeletableSkillSelection +} from './skill-delete-selection' +import { skillDeleteActionLabel } from './skill-delete-copy' +import { shareSelectionActionLabel } from './skill-display-labels' +import { SkillDeleteResultBand } from './SkillDeleteResultBand' +import { useSkillDeleteFlow } from './use-skill-delete-flow' const EMPTY_SKILLS: DiscoveredSkill[] = [] const NO_FILTERS: SkillsFilterState = { @@ -55,7 +68,7 @@ export default function SkillsPage(): React.JSX.Element { const [loading, setLoading] = useState(true) const [scanError, setScanError] = useState(null) const [shareSkills, setShareSkills] = useState([]) - const [selectingShare, setSelectingShare] = useState(false) + const [selectionMode, setSelectionMode] = useState<'share' | 'delete' | null>(null) const [selectedSkillIds, setSelectedSkillIds] = useState>(() => new Set()) const [installOpen, setInstallOpen] = useState(false) const [installLink, setInstallLink] = useState('') @@ -65,44 +78,59 @@ export default function SkillsPage(): React.JSX.Element { const ownedShares = useOwnedSkillShares() const mountedRef = useMountedRef() const scanGenerationRef = useRef(0) + // Why a ref: `loadSkills` must not re-identify (and re-scan) when the user + // merely switches selection mode. Switching modes clears the selection anyway, + // so a one-render lag here cannot retain the wrong rows. + const selectionModeRef = useRef(selectionMode) + useEffect(() => { + selectionModeRef.current = selectionMode + }, [selectionMode]) - 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) - const local = runtimeTarget.kind === 'local' - if (isCurrentScan()) { - setResult(nextResult) - setScanError(null) - setSelectedSkillIds((current) => - retainedShareableSkillSelection(current, nextResult.skills, local) + 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 + } + try { + const nextResult = await discoverSkillsForRuntimeTarget( + runtimeTarget, + refresh ? { refresh: true } : undefined ) + const local = runtimeTarget.kind === 'local' + if (isCurrentScan()) { + setResult(nextResult) + setScanError(null) + setSelectedSkillIds((current) => + selectionModeRef.current === 'delete' + ? retainedDeletableSkillSelection(current, nextResult.skills) + : retainedShareableSkillSelection(current, nextResult.skills, local) + ) + } + } catch (error) { + console.error('Failed to discover skills:', error) + if (isCurrentScan()) { + // Why: a failed scan needs to stay on screen with a retry — a toast + // disappears before the user can act on it. + setScanError( + translate('auto.components.skills.SkillsPage.ea72d6185b', 'Could not scan skills') + ) + } + } finally { + if (isCurrentScan()) { + setLoading(false) + } } - } catch (error) { - console.error('Failed to discover skills:', error) - if (isCurrentScan()) { - // Why: a failed scan needs to stay on screen with a retry — a toast - // disappears before the user can act on it. - setScanError( - translate('auto.components.skills.SkillsPage.ea72d6185b', 'Could not scan skills') - ) - } - } finally { - if (isCurrentScan()) { - setLoading(false) - } - } - }, [mountedRef, runtimeTarget]) + }, + [mountedRef, runtimeTarget] + ) useEffect(() => { void loadSkills() @@ -132,10 +160,19 @@ export default function SkillsPage(): React.JSX.Element { }, [clearPendingSkillShare, pendingSkillShareId]) const exitSelection = useCallback((): void => { - setSelectingShare(false) + setSelectionMode(null) setSelectedSkillIds(new Set()) }, []) + const deleteFlow = useSkillDeleteFlow(runtimeTarget, hostLabel, () => { + exitSelection() + // Why an explicit refresh instead of firing the change event: this page + // would then run a second, non-refresh scan of the host it just refreshed. + // Other subscribers (settings badges, pickers) still hear the event. + void loadSkills(true) + window.dispatchEvent(new Event(INSTALLED_AGENT_SKILLS_REFRESHED_EVENT)) + }) + // Why: the search box is shared between the two lists, but a link query means // nothing against skill names and vice versa. const exitSharedLinks = useCallback((): void => { @@ -188,7 +225,7 @@ export default function SkillsPage(): React.JSX.Element { event.preventDefault() // Why: leaving the page would silently discard a selection that can hold // dozens of skills, so Escape backs out of the mode first. - if (selectingShare) { + if (selectionMode) { exitSelection() return } @@ -205,7 +242,7 @@ export default function SkillsPage(): React.JSX.Element { // keeps page-level back navigation reliable when no overlay is active. window.addEventListener('keydown', handleKeyDown, { capture: true }) return () => window.removeEventListener('keydown', handleKeyDown, { capture: true }) - }, [closeSkillsPage, exitSelection, exitSharedLinks, selectingShare, view]) + }, [closeSkillsPage, exitSelection, exitSharedLinks, selectionMode, view]) const skills = result?.skills ?? EMPTY_SKILLS const local = runtimeTarget?.kind === 'local' @@ -217,7 +254,18 @@ export default function SkillsPage(): React.JSX.Element { ) const sourceCounts = useMemo(() => countSkillsBySource(skills), [skills]) const sourceEntries = useMemo(() => summarizeSkillSources(result), [result]) - const eligibleCount = eligibleShareSkillCount(visibleSkills, local) + const eligibleCount = + selectionMode === 'delete' + ? eligibleDeleteSkillCount(visibleSkills) + : eligibleShareSkillCount(visibleSkills, local) + const deleting = selectionMode === 'delete' + const addSelected = ( + current: ReadonlySet, + results: readonly DiscoveredSkill[] + ): Set => + deleting + ? addDeletableSkillResults(current, skills, results) + : addShareableSkillResults(current, skills, results, local) const openInstallDialog = (): void => { setInstallLink('') setInstallOpen(true) @@ -225,18 +273,47 @@ export default function SkillsPage(): React.JSX.Element { return (
- {selectingShare ? ( + {selectionMode ? ( - setSelectedSkillIds((current) => - addShareableSkillResults(current, skills, visibleSkills, local) + title={ + deleting + ? translate( + 'auto.components.skills.SkillsSelectionHeader.deleteTitle', + 'Select skills to delete' + ) + : translate( + 'auto.components.skills.SkillsSelectionHeader.title', + 'Select skills to share' + ) + } + icon={ + deleting ? ( + + ) : ( + ) } + actionIcon={deleting ? : } + actionLabel={ + deleting + ? skillDeleteActionLabel(selectedSkillIds.size) + : shareSelectionActionLabel(selectedSkillIds.size) + } + destructive={deleting} + busy={deleting && deleteFlow.running} + selectedCount={selectedSkillIds.size} + eligibleCount={eligibleCount} + onSelectAll={() => setSelectedSkillIds((current) => addSelected(current, visibleSkills))} onClear={() => setSelectedSkillIds(new Set())} onCancel={exitSelection} - onShare={() => setShareSkills(skills.filter((skill) => selectedSkillIds.has(skill.id)))} + onSubmit={() => { + const selected = skills.filter((skill) => selectedSkillIds.has(skill.id)) + if (deleting) { + void deleteFlow.requestDelete(selected) + return + } + setShareSkills(selected) + }} /> ) : ( { - setSelectingShare(true) + setSelectionMode('share') + setSelectedSkillIds(new Set()) + }} + deleteSupported={deleteFlow.supported} + deleteUnsupportedReason={deleteFlow.unsupportedReason} + onStartDelete={() => { + setSelectionMode('delete') setSelectedSkillIds(new Set()) }} onInstallFromLink={openInstallDialog} @@ -274,6 +357,9 @@ export default function SkillsPage(): React.JSX.Element { onRetry={() => void loadSkills()} /> ) : null} + {deleteFlow.result ? ( + + ) : null}
@@ -291,18 +377,19 @@ export default function SkillsPage(): React.JSX.Element { local={local} agentByRootPath={agentByRootPath} selectedIds={selectedSkillIds} - selectionMode={selectingShare} + selectionMode={selectionMode} + deleteSupported={deleteFlow.supported} + deleteUnsupportedReason={deleteFlow.unsupportedReason} onSelectedChange={(skillId, selected) => setSelectedSkillIds((current) => updatedSkillSelection(current, skillId, selected) ) } onSelectResults={(results) => - setSelectedSkillIds((current) => - addShareableSkillResults(current, skills, results, local) - ) + setSelectedSkillIds((current) => addSelected(current, results)) } onShare={(skill) => setShareSkills([skill])} + onDelete={(skill) => void deleteFlow.requestDelete([skill])} /> ) : skills.length > 0 ? ( setFilters(NO_FILTERS)} /> diff --git a/src/renderer/src/components/skills/SkillsPageHeader.tsx b/src/renderer/src/components/skills/SkillsPageHeader.tsx index a8817c3ecf4..548a43114cf 100644 --- a/src/renderer/src/components/skills/SkillsPageHeader.tsx +++ b/src/renderer/src/components/skills/SkillsPageHeader.tsx @@ -1,4 +1,4 @@ -import { BookOpen, Download, History, Link2, MoreHorizontal, Share2, X } from 'lucide-react' +import { BookOpen, Download, History, Link2, MoreHorizontal, Share2, Trash2, X } from 'lucide-react' import { Button } from '@/components/ui/button' import { DropdownMenu, @@ -21,6 +21,9 @@ export function SkillsPageHeader({ hostLabel, onClose, onStartShare, + deleteSupported, + deleteUnsupportedReason, + onStartDelete, onInstallFromLink, onManageInstalls, onOpenSharedLinks @@ -31,6 +34,12 @@ export function SkillsPageHeader({ hostLabel: string | null onClose: () => void onStartShare: () => void + /** False while the target is unresolved or the host predates the delete + * capability, so the entry disables with a reason rather than routing a + * request nothing on that host answers. */ + deleteSupported: boolean + deleteUnsupportedReason: string | null + onStartDelete: () => void onInstallFromLink: () => void onManageInstalls: () => void onOpenSharedLinks: () => void @@ -113,6 +122,25 @@ export function SkillsPageHeader({ {translate('auto.components.skills.SkillsPage.sharedLinks', 'Shared links')} + + + + + + {translate('auto.components.skills.SkillsPage.deleteSkills', 'Delete skills…')} + + + + {deleteSupported || !deleteUnsupportedReason ? null : ( + + {deleteUnsupportedReason} + + )} +
diff --git a/src/renderer/src/components/skills/SkillsSelectionHeader.tsx b/src/renderer/src/components/skills/SkillsSelectionHeader.tsx index c6269348751..663fd703e92 100644 --- a/src/renderer/src/components/skills/SkillsSelectionHeader.tsx +++ b/src/renderer/src/components/skills/SkillsSelectionHeader.tsx @@ -1,9 +1,9 @@ -import { Share2, X } from 'lucide-react' +import { X } from 'lucide-react' import { Button } from '@/components/ui/button' import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' import { cn } from '@/lib/utils' import { translate } from '@/i18n/i18n' -import { selectedCountLabel, shareSelectionActionLabel } from './skill-display-labels' +import { selectedCountLabel } from './skill-display-labels' import { SKILLS_PAGE_COLUMN } from './skills-page-column' import { SKILLS_SUBTITLE_ACTION_CLASS } from './skills-subtitle-action' @@ -13,19 +13,34 @@ import { SKILLS_SUBTITLE_ACTION_CLASS } from './skills-subtitle-action' * Esc means — leave the mode, not the page. */ export function SkillsSelectionHeader({ + title, + icon, + actionIcon, + actionLabel, + destructive = false, + busy = false, selectedCount, eligibleCount, onSelectAll, onClear, onCancel, - onShare + onSubmit }: { + /** Copy is passed in already translated: this header's own key namespace + * cannot own strings for two unrelated modes. */ + title: string + icon: React.JSX.Element + actionIcon: React.JSX.Element + actionLabel: string + destructive?: boolean + /** Keeps a slow remote submit from being fired twice. */ + busy?: boolean selectedCount: number eligibleCount: number onSelectAll: () => void onClear: () => void onCancel: () => void - onShare: () => void + onSubmit: () => void }): React.JSX.Element { return (
@@ -53,14 +68,9 @@ export function SkillsSelectionHeader({
- + {icon}
-

- {translate( - 'auto.components.skills.SkillsSelectionHeader.title', - 'Select skills to share' - )} -

+

{title}

{selectedCountLabel(selectedCount)} @@ -91,9 +101,15 @@ export function SkillsSelectionHeader({
-
diff --git a/src/renderer/src/components/skills/skill-delete-copy.test.ts b/src/renderer/src/components/skills/skill-delete-copy.test.ts new file mode 100644 index 00000000000..113c10dce7a --- /dev/null +++ b/src/renderer/src/components/skills/skill-delete-copy.test.ts @@ -0,0 +1,146 @@ +import { describe, expect, it } from 'vitest' +import type { SkillDeletePlan } from '../../../../shared/skill-delete-contract' +import { + skillDeleteBlockedLines, + skillDeletePlacementSummary, + skillDeleteResultLines, + skillDeleteRetainedSourceLines +} from './skill-delete-copy' + +function plan(skills: SkillDeletePlan['skills']): SkillDeletePlan { + return { operationId: 'op', skills } +} + +describe('skillDeletePlacementSummary', () => { + it('summarizes folders, links, and the roots they sit in', () => { + const summary = skillDeletePlacementSummary( + plan([ + { + id: 'a', + name: 'demo', + canonicalPath: '/root/demo/SKILL.md', + placements: [ + { + path: '/home/.agents/skills/demo', + kind: 'canonical', + rootLabel: 'Agent skills home' + }, + { path: '/home/.claude/skills/demo', kind: 'alias-dir', rootLabel: 'Claude home' } + ] + } + ]) + ) + expect(summary).toContain('1') + expect(summary).toContain('Agent skills home') + expect(summary).toContain('Claude home') + }) + + it('drops the zero half so a link-only delete never reads "0 folders"', () => { + const summary = skillDeletePlacementSummary( + plan([ + { + id: 'a', + name: 'demo', + canonicalPath: '/home/.local/share/devex/skills/demo/SKILL.md', + placements: [ + { path: '/repo/.agents/skills/demo', kind: 'alias-dir', rootLabel: 'Repo app .agents' } + ] + } + ]) + ) + expect(summary).not.toContain('0') + expect(summary).toContain('link') + expect(summary).not.toContain('folder') + }) + + it('is null when the plan removes nothing', () => { + // Otherwise the dialog read "0 folders and 0 links across " with an empty + // tail, beside a red button that could not do anything. + expect( + skillDeletePlacementSummary( + plan([ + { + id: 'a', + name: 'demo', + canonicalPath: '/elsewhere/demo/SKILL.md', + placements: [], + blocked: 'unowned' + } + ]) + ) + ).toBeNull() + }) +}) + +describe('skillDeleteRetainedSourceLines', () => { + it('says where the content stays when only links are removed', () => { + const lines = skillDeleteRetainedSourceLines( + plan([ + { + id: 'a', + name: 'demo', + canonicalPath: '/home/.local/share/devex/skills/demo/SKILL.md', + placements: [ + { path: '/repo/.agents/skills/demo', kind: 'alias-dir', rootLabel: 'Repo app .agents' } + ] + } + ]) + ) + expect(lines).toHaveLength(1) + expect(lines[0]).toContain('/home/.local/share/devex/skills/demo') + }) + + it('stays silent when a real folder is being removed', () => { + expect( + skillDeleteRetainedSourceLines( + plan([ + { + id: 'a', + name: 'demo', + canonicalPath: '/home/.agents/skills/demo/SKILL.md', + placements: [ + { + path: '/home/.agents/skills/demo', + kind: 'canonical', + rootLabel: 'Agent skills home' + } + ] + } + ]) + ) + ).toEqual([]) + }) +}) + +describe('skillDeleteBlockedLines', () => { + it('groups by typed reason rather than one merged skipped line', () => { + const lines = skillDeleteBlockedLines( + plan([ + { id: 'a', name: 'a', canonicalPath: '/p', placements: [], blocked: 'bundled' }, + { id: 'b', name: 'b', canonicalPath: '/p', placements: [], blocked: 'bundled' }, + { id: 'c', name: 'c', canonicalPath: '/p', placements: [], blocked: 'plugin' } + ]) + ) + expect(lines).toHaveLength(2) + expect(lines.some((line) => line.includes('2') && line.includes('Bundled'))).toBe(true) + }) +}) + +describe('skillDeleteResultLines', () => { + it('omits deleted rows and keeps one line per distinct outcome', () => { + const lines = skillDeleteResultLines([ + { id: 'a', name: 'a', status: 'deleted', removedPaths: ['/x'] }, + { id: 'b', name: 'b', status: 'skipped', blocked: 'unowned', removedPaths: [] }, + { id: 'c', name: 'c', status: 'skipped', blocked: 'unowned', removedPaths: [] }, + { id: 'd', name: 'd', status: 'busy', removedPaths: [] } + ]) + expect(lines.map((line) => line.key)).toEqual(['skipped:unowned', 'busy']) + expect(lines[0].label).toContain('2') + }) + + it('renders nothing when every skill was deleted', () => { + expect( + skillDeleteResultLines([{ id: 'a', name: 'a', status: 'deleted', removedPaths: ['/x'] }]) + ).toEqual([]) + }) +}) diff --git a/src/renderer/src/components/skills/skill-delete-copy.ts b/src/renderer/src/components/skills/skill-delete-copy.ts new file mode 100644 index 00000000000..afd27269f11 --- /dev/null +++ b/src/renderer/src/components/skills/skill-delete-copy.ts @@ -0,0 +1,203 @@ +import { translate } from '@/i18n/i18n' +import type { + SkillDeleteBlockReason, + SkillDeletePlan, + SkillDeleteResultEntry +} from '../../../../shared/skill-delete-contract' + +export function skillDeleteBlockReasonLabel(reason: SkillDeleteBlockReason): string { + switch (reason) { + case 'bundled': { + return translate( + 'auto.components.skills.SkillDelete.reasonBundled', + 'Bundled with Orca — it would be restored' + ) + } + case 'plugin': { + return translate( + 'auto.components.skills.SkillDelete.reasonPlugin', + 'Installed by a plugin — remove the plugin instead' + ) + } + case 'unowned': { + return translate( + 'auto.components.skills.SkillDelete.reasonUnowned', + 'This skill lives outside Orca’s skill folders — delete it where it is stored' + ) + } + case 'missing': { + return translate( + 'auto.components.skills.SkillDelete.reasonMissing', + 'This skill is no longer on disk' + ) + } + case 'stale': { + return translate( + 'auto.components.skills.SkillDelete.reasonStale', + 'This skill changed since the list was loaded — refresh and try again' + ) + } + } +} + +export function skillDeleteActionLabel(count: number): string { + return count === 1 + ? translate('auto.components.skills.count.deleteOne', 'Delete {{count}} skill', { count }) + : translate('auto.components.skills.count.deleteOther', 'Delete {{count}} skills', { count }) +} + +function foldersLabel(count: number): string { + return count === 1 + ? translate('auto.components.skills.count.deleteFolderOne', '{{count}} folder', { count }) + : translate('auto.components.skills.count.deleteFolderOther', '{{count}} folders', { count }) +} + +function linksLabel(count: number): string { + return count === 1 + ? translate('auto.components.skills.count.deleteLinkOne', '{{count}} link', { count }) + : translate('auto.components.skills.count.deleteLinkOther', '{{count}} links', { count }) +} + +/** "Removes 12 folders and 5 links across Claude, Codex, Agent Skills." + * Null when the plan removes nothing, so the caller states that instead of + * rendering "0 folders and 0 links across " with an empty tail. Zero-valued + * halves are dropped: a link-only delete must not read "0 folders and 1 links". */ +export function skillDeletePlacementSummary(plan: SkillDeletePlan): string | null { + const placements = plan.skills.flatMap((skill) => skill.placements) + if (placements.length === 0) { + return null + } + const folders = placements.filter((placement) => placement.kind === 'canonical').length + const links = placements.length - folders + const parts = [ + folders > 0 ? foldersLabel(folders) : null, + links > 0 ? linksLabel(links) : null + ].filter((part): part is string => part !== null) + const roots = [...new Set(placements.map((placement) => placement.rootLabel))].sort((a, b) => + a.localeCompare(b, undefined, { sensitivity: 'base' }) + ) + return translate( + 'auto.components.skills.SkillDelete.placementSummaryParts', + 'Removes {{parts}} across {{roots}}.', + { + parts: parts.join(translate('auto.components.skills.SkillDelete.placementJoin', ' and ')), + roots: roots.join(', ') + } + ) +} + +function parentDirectory(path: string): string { + const index = Math.max(path.lastIndexOf('/'), path.lastIndexOf('\\')) + return index > 0 ? path.slice(0, index) : path +} + +/** + * A skill whose content lives outside every Orca root is removed by its links + * only. Saying so is the difference between "deleted" and "gone" — the content + * survives at its source, and the user has to be told which one happened. + */ +export function skillDeleteRetainedSourceLines(plan: SkillDeletePlan): string[] { + return plan.skills + .filter( + (skill) => + skill.placements.length > 0 && + !skill.placements.some((placement) => placement.kind === 'canonical') + ) + .map((skill) => + translate( + 'auto.components.skills.SkillDelete.retainedSource', + 'The skill itself stays at {{path}}.', + { path: parentDirectory(skill.canonicalPath) } + ) + ) +} + +export function skillDeleteNothingToDoLabel(count: number): string { + return count === 1 + ? translate( + 'auto.components.skills.SkillDelete.nothingOne', + 'That skill cannot be deleted from here.' + ) + : translate( + 'auto.components.skills.SkillDelete.nothingOther', + 'None of the {{count}} selected skills can be deleted from here.', + { count } + ) +} + +/** One line per skip reason rather than one merged "skipped" line: the plan + * already carries a typed reason per skill, so the dialog groups by it. */ +export function skillDeleteBlockedLines(plan: SkillDeletePlan): string[] { + const counts = new Map() + for (const skill of plan.skills) { + if (skill.blocked) { + counts.set(skill.blocked, (counts.get(skill.blocked) ?? 0) + 1) + } + } + return [...counts].map(([reason, count]) => + translate('auto.components.skills.SkillDelete.blockedLine', '{{count}} × {{reason}}', { + count, + reason: skillDeleteBlockReasonLabel(reason) + }) + ) +} + +export type SkillDeleteResultLine = { key: string; label: string } + +/** A toast disappears before a user can act on "3 failed", so anything that is + * not `deleted` gets a persistent, per-reason line instead. */ +export function skillDeleteResultLines( + skills: readonly SkillDeleteResultEntry[] +): SkillDeleteResultLine[] { + const grouped = new Map() + for (const skill of skills) { + if (skill.status === 'deleted') { + continue + } + const key = + skill.status === 'skipped' && skill.blocked ? `skipped:${skill.blocked}` : skill.status + const existing = grouped.get(key) + grouped.set(key, { + count: (existing?.count ?? 0) + 1, + label: existing?.label ?? statusLabel(skill) + }) + } + return [...grouped].map(([key, value]) => ({ + key, + label: translate('auto.components.skills.SkillDelete.resultLine', '{{count}} × {{label}}', { + count: value.count, + label: value.label + }) + })) +} + +function statusLabel(skill: SkillDeleteResultEntry): string { + switch (skill.status) { + case 'skipped': { + return skill.blocked + ? skillDeleteBlockReasonLabel(skill.blocked) + : translate('auto.components.skills.SkillDelete.statusSkipped', 'Skipped') + } + case 'busy': { + return translate( + 'auto.components.skills.SkillDelete.statusBusy', + 'Busy — another skill operation is in progress' + ) + } + case 'partial': { + return translate( + 'auto.components.skills.SkillDelete.statusPartial', + 'Partly removed — some files are still on disk under a hidden name' + ) + } + case 'failed': { + return translate( + 'auto.components.skills.SkillDelete.statusFailed', + 'Failed — nothing changed' + ) + } + case 'deleted': { + return translate('auto.components.skills.SkillDelete.statusDeleted', 'Deleted') + } + } +} diff --git a/src/renderer/src/components/skills/skill-delete-selection.test.ts b/src/renderer/src/components/skills/skill-delete-selection.test.ts new file mode 100644 index 00000000000..345419fda49 --- /dev/null +++ b/src/renderer/src/components/skills/skill-delete-selection.test.ts @@ -0,0 +1,80 @@ +import { describe, expect, it } from 'vitest' +import type { DiscoveredSkill } from '../../../../shared/skills' +import { + addDeletableSkillResults, + eligibleDeleteSkillCount, + isSkillDeleteEligible, + retainedDeletableSkillSelection, + skillDeleteEligibilityReason +} from './skill-delete-selection' + +function skill(overrides: Partial & { id: string }): DiscoveredSkill { + return { + name: 'demo', + description: null, + providers: ['agent-skills'], + sourceKind: 'home', + sourceLabel: 'Agent skills home', + rootPath: '/root', + directoryPath: '/root/demo', + skillFilePath: '/root/demo/SKILL.md', + installed: true, + updatedAt: 1, + ...overrides + } +} + +describe('delete selection', () => { + it('keeps two rows sharing a name independently selectable', () => { + // Share collapses duplicate names because two skills cannot publish under + // one name. Delete has no such constraint — these are two distinct files. + const skills = [ + skill({ id: 'a', rootPath: '/home/.agents/skills' }), + skill({ id: 'b', rootPath: '/repo/.claude/skills' }) + ] + expect(eligibleDeleteSkillCount(skills)).toBe(2) + expect([...addDeletableSkillResults(new Set(), skills, skills)]).toEqual(['a', 'b']) + }) + + it('excludes bundled and plugin rows from select-all', () => { + const skills = [ + skill({ id: 'a' }), + skill({ id: 'b', sourceKind: 'bundled' }), + skill({ id: 'c', sourceKind: 'plugin' }), + skill({ id: 'd', sourceKind: 'repo' }) + ] + expect(eligibleDeleteSkillCount(skills)).toBe(2) + expect([...addDeletableSkillResults(new Set(), skills, skills)]).toEqual(['a', 'd']) + }) + + it('drops a selection whose row no longer exists after a rescan', () => { + const current = new Set(['a', 'gone']) + const retained = retainedDeletableSkillSelection(current, [skill({ id: 'a' })]) + expect([...retained]).toEqual(['a']) + }) + + it('returns the same set reference when a rescan changes nothing', () => { + const skills = [skill({ id: 'a' }), skill({ id: 'b' })] + const current = new Set(['a', 'b']) + expect(retainedDeletableSkillSelection(current, skills)).toBe(current) + }) + + it('drops a row that became ineligible between scans', () => { + const current = new Set(['a']) + const retained = retainedDeletableSkillSelection(current, [ + skill({ id: 'a', sourceKind: 'plugin' }) + ]) + expect([...retained]).toEqual([]) + }) + + it('gives a reason only when the row is not deletable', () => { + expect(isSkillDeleteEligible(skill({ id: 'a' }))).toBe(true) + expect(skillDeleteEligibilityReason(skill({ id: 'a' }))).toBeNull() + expect(skillDeleteEligibilityReason(skill({ id: 'b', sourceKind: 'bundled' }))).toContain( + 'Bundled' + ) + expect(skillDeleteEligibilityReason(skill({ id: 'c', sourceKind: 'plugin' }))).toContain( + 'plugin' + ) + }) +}) diff --git a/src/renderer/src/components/skills/skill-delete-selection.ts b/src/renderer/src/components/skills/skill-delete-selection.ts new file mode 100644 index 00000000000..24d9a176ec7 --- /dev/null +++ b/src/renderer/src/components/skills/skill-delete-selection.ts @@ -0,0 +1,46 @@ +import type { DiscoveredSkill } from '../../../../shared/skills' +import { skillDeletionEligibility } from '../../../../shared/skill-deletion-eligibility' +import { skillDeleteBlockReasonLabel } from './skill-delete-copy' +import { + addSelectableSkillResults, + eligibleSkillSelectionCount, + retainedSkillSelection, + type SkillSelectionPolicy +} from './skill-selection' + +/** No collision key: two rows named `foo` from different roots are two distinct + * skills, and both must be independently selectable and reportable. */ +const DELETE_SELECTION_POLICY: SkillSelectionPolicy = { + isEligible: (skill) => skillDeletionEligibility(skill).deletable, + collisionKey: () => null +} + +export function isSkillDeleteEligible(skill: DiscoveredSkill): boolean { + return DELETE_SELECTION_POLICY.isEligible(skill) +} + +/** Advisory client-side reason. The host re-derives the verdict from the + * placement set and can refuse a row this reports as deletable. */ +export function skillDeleteEligibilityReason(skill: DiscoveredSkill): string | null { + const eligibility = skillDeletionEligibility(skill) + return eligibility.deletable ? null : skillDeleteBlockReasonLabel(eligibility.reason) +} + +export function eligibleDeleteSkillCount(results: readonly DiscoveredSkill[]): number { + return eligibleSkillSelectionCount(results, DELETE_SELECTION_POLICY) +} + +export function addDeletableSkillResults( + current: ReadonlySet, + skills: readonly DiscoveredSkill[], + results: readonly DiscoveredSkill[] +): Set { + return addSelectableSkillResults(current, skills, results, DELETE_SELECTION_POLICY) +} + +export function retainedDeletableSkillSelection( + current: Set, + skills: readonly DiscoveredSkill[] +): Set { + return retainedSkillSelection(current, skills, DELETE_SELECTION_POLICY) +} diff --git a/src/renderer/src/components/skills/skill-selection.ts b/src/renderer/src/components/skills/skill-selection.ts new file mode 100644 index 00000000000..6f5479bce69 --- /dev/null +++ b/src/renderer/src/components/skills/skill-selection.ts @@ -0,0 +1,97 @@ +import type { DiscoveredSkill } from '../../../../shared/skills' + +/** + * The range/select-all/retain-across-rescan logic both selection modes share. + * + * `collisionKey` is what they do *not* share. Share collapses selections by + * lowercase name, because two skills cannot publish under one name. Delete has + * no such constraint — two rows named `foo` from different roots must both be + * selectable — so it passes `null` and nothing collapses. + */ +export type SkillSelectionPolicy = { + isEligible: (skill: DiscoveredSkill) => boolean + collisionKey: (skill: DiscoveredSkill) => string | null +} + +function collisionKeys( + skills: readonly DiscoveredSkill[], + selectedIds: ReadonlySet, + policy: SkillSelectionPolicy +): Set { + const keys = new Set() + for (const skill of skills) { + const key = selectedIds.has(skill.id) ? policy.collisionKey(skill) : null + if (key !== null) { + keys.add(key) + } + } + return keys +} + +/** How many rows "Select all" would end up holding. */ +export function eligibleSkillSelectionCount( + results: readonly DiscoveredSkill[], + policy: SkillSelectionPolicy +): number { + const keys = new Set() + let uncollapsed = 0 + for (const skill of results) { + if (!policy.isEligible(skill)) { + continue + } + const key = policy.collisionKey(skill) + if (key === null) { + uncollapsed += 1 + continue + } + keys.add(key) + } + return keys.size + uncollapsed +} + +/** Used by both select-all and shift-range. */ +export function addSelectableSkillResults( + current: ReadonlySet, + skills: readonly DiscoveredSkill[], + results: readonly DiscoveredSkill[], + policy: SkillSelectionPolicy +): Set { + const next = new Set(current) + const taken = collisionKeys(skills, current, policy) + for (const skill of results) { + if (!policy.isEligible(skill)) { + continue + } + const key = policy.collisionKey(skill) + if (key !== null && taken.has(key)) { + continue + } + next.add(skill.id) + if (key !== null) { + taken.add(key) + } + } + return next +} + +/** Rebuilds the selection from a fresh scan, dropping rows that vanished or + * became ineligible. Returns the same reference when nothing changed. */ +export function retainedSkillSelection( + current: Set, + skills: readonly DiscoveredSkill[], + policy: SkillSelectionPolicy +): Set { + const next = new Set() + const taken = new Set() + for (const skill of skills) { + const key = policy.collisionKey(skill) + if (!current.has(skill.id) || !policy.isEligible(skill) || (key !== null && taken.has(key))) { + continue + } + next.add(skill.id) + if (key !== null) { + taken.add(key) + } + } + return next.size === current.size ? current : next +} diff --git a/src/renderer/src/components/skills/skill-share-selection.ts b/src/renderer/src/components/skills/skill-share-selection.ts index 861bd4f3f58..abb64e3e35c 100644 --- a/src/renderer/src/components/skills/skill-share-selection.ts +++ b/src/renderer/src/components/skills/skill-share-selection.ts @@ -1,5 +1,11 @@ import type { DiscoveredSkill } from '../../../../shared/skills' import { translate } from '@/i18n/i18n' +import { + addSelectableSkillResults, + eligibleSkillSelectionCount, + retainedSkillSelection, + type SkillSelectionPolicy +} from './skill-selection' export function isSkillShareEligible(skill: DiscoveredSkill, local: boolean): boolean { return local && skill.installed && (skill.sourceKind === 'home' || skill.sourceKind === 'repo') @@ -49,19 +55,25 @@ export function selectedShareSkillNameKeys( ) } -/** How many rows "Select all" would end up holding — duplicate names collapse to - * one, so the count has to dedup the same way the selection itself does. */ +const SHARE_SELECTION_POLICY_LOCAL: SkillSelectionPolicy = { + isEligible: (skill) => isSkillShareEligible(skill, true), + collisionKey: shareSkillNameKey +} + +const SHARE_SELECTION_POLICY_REMOTE: SkillSelectionPolicy = { + isEligible: () => false, + collisionKey: shareSkillNameKey +} + +function sharePolicy(local: boolean): SkillSelectionPolicy { + return local ? SHARE_SELECTION_POLICY_LOCAL : SHARE_SELECTION_POLICY_REMOTE +} + export function eligibleShareSkillCount( results: readonly DiscoveredSkill[], local: boolean ): number { - const names = new Set() - for (const skill of results) { - if (isSkillShareEligible(skill, local)) { - names.add(shareSkillNameKey(skill)) - } - } - return names.size + return eligibleSkillSelectionCount(results, sharePolicy(local)) } export function addShareableSkillResults( @@ -70,17 +82,7 @@ export function addShareableSkillResults( results: readonly DiscoveredSkill[], local: boolean ): Set { - const next = new Set(current) - const selectedNames = selectedShareSkillNameKeys(skills, current) - for (const skill of results) { - const name = shareSkillNameKey(skill) - if (!isSkillShareEligible(skill, local) || selectedNames.has(name)) { - continue - } - next.add(skill.id) - selectedNames.add(name) - } - return next + return addSelectableSkillResults(current, skills, results, sharePolicy(local)) } export function retainedShareableSkillSelection( @@ -88,17 +90,7 @@ export function retainedShareableSkillSelection( skills: readonly DiscoveredSkill[], local: boolean ): Set { - const next = new Set() - const names = new Set() - for (const skill of skills) { - const name = shareSkillNameKey(skill) - if (!current.has(skill.id) || !isSkillShareEligible(skill, local) || names.has(name)) { - continue - } - next.add(skill.id) - names.add(name) - } - return next.size === current.size ? current : next + return retainedSkillSelection(current, skills, sharePolicy(local)) } export function updatedSkillSelection( diff --git a/src/renderer/src/components/skills/use-skill-delete-flow.ts b/src/renderer/src/components/skills/use-skill-delete-flow.ts new file mode 100644 index 00000000000..eaa0e8f697f --- /dev/null +++ b/src/renderer/src/components/skills/use-skill-delete-flow.ts @@ -0,0 +1,188 @@ +import { useCallback, useEffect, useState } from 'react' +import { toast } from 'sonner' +import { translate } from '@/i18n/i18n' +import { useConfirmationDialog } from '@/components/confirmation-dialog-context' +import { useMountedRef } from '@/hooks/useMountedRef' +import type { + SkillDeletePlan, + 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, + previewSkillDeletionOnRuntimeTarget, + runtimeTargetSupportsSkillDelete +} from '../../runtime/runtime-skills-client' +import type { RuntimeClientTarget } from '../../runtime/runtime-rpc-client' +import { + skillDeleteActionLabel, + skillDeleteBlockedLines, + skillDeleteNothingToDoLabel, + skillDeletePlacementSummary, + skillDeleteRetainedSourceLines +} from './skill-delete-copy' + +export type SkillDeleteFlow = { + /** False while the target is unresolved or the host predates the capability. */ + supported: boolean + /** Why it is unsupported — the two causes need different copy. Null when + * delete is available. */ + unsupportedReason: string | null + running: boolean + result: SkillDeleteResult | null + dismissResult: () => void + requestDelete: (skills: readonly DiscoveredSkill[]) => Promise +} + +function toRequest(skills: readonly DiscoveredSkill[]): SkillDeleteRequest { + return { + operationId: crypto.randomUUID(), + skills: skills.map((skill) => ({ + id: skill.id, + directoryPath: skill.directoryPath, + skillFilePath: skill.skillFilePath, + name: skill.name, + updatedAt: skill.updatedAt + })) + } +} + +export function useSkillDeleteFlow( + runtimeTarget: RuntimeClientTarget | null, + hostLabel: string | null, + onDeleted: (result: SkillDeleteResult) => void +): SkillDeleteFlow { + const confirm = useConfirmationDialog() + const mountedRef = useMountedRef() + const [supported, setSupported] = useState(false) + const [running, setRunning] = useState(false) + const [result, setResult] = useState(null) + + useEffect(() => { + let current = true + setSupported(false) + void runtimeTargetSupportsSkillDelete(runtimeTarget).then((value) => { + if (current && mountedRef.current) { + setSupported(value) + } + }) + return () => { + current = false + } + }, [mountedRef, runtimeTarget]) + + const requestDelete = useCallback( + async (skills: readonly DiscoveredSkill[]): Promise => { + if (!runtimeTarget || skills.length === 0) { + return false + } + const request = toRequest(skills) + setRunning(true) + try { + const plan = await previewSkillDeletionOnRuntimeTarget(runtimeTarget, request) + const actionable = plan.skills.filter((skill) => !skill.blocked) + if (actionable.length === 0) { + // The preview already knows this would remove nothing. Offering a + // destructive confirm here is what let a blocked row be "deleted" + // over and over with no effect; say why instead, in the band, where + // it persists long enough to act on. + setResult(blockedOnlyResult(plan)) + toast.error(skillDeleteNothingToDoLabel(plan.skills.length)) + return false + } + const confirmed = await confirm({ + title: skillDeleteActionLabel(actionable.length), + description: [ + hostLabel + ? translate('auto.components.skills.SkillDelete.confirmHost', 'On {{host}}.', { + host: hostLabel + }) + : null, + skillDeletePlacementSummary(plan), + // Permanent, so a single-skill delete names the paths themselves. + actionable.length === 1 + ? actionable.flatMap((skill) => skill.placements.map((p) => p.path)).join('\n') + : null, + ...skillDeleteRetainedSourceLines(plan), + ...skillDeleteBlockedLines(plan), + translate( + 'auto.components.skills.SkillDelete.confirmPermanent', + 'This cannot be undone.' + ) + ] + .filter((line): line is string => Boolean(line)) + .join('\n'), + confirmLabel: skillDeleteActionLabel(actionable.length), + confirmVariant: 'destructive' + }) + if (!confirmed) { + return false + } + const outcome = await deleteSkillsOnRuntimeTarget(runtimeTarget, request) + if (!mountedRef.current) { + return true + } + const deleted = outcome.skills.filter((skill) => skill.status === 'deleted').length + if (deleted > 0) { + toast.success(skillDeletedToast(deleted)) + } + setResult(outcome) + onDeleted(outcome) + return true + } catch (error) { + toast.error( + error instanceof Error + ? error.message + : translate('auto.components.skills.SkillDelete.failed', 'Could not delete skills') + ) + return false + } finally { + if (mountedRef.current) { + setRunning(false) + } + } + }, + [confirm, hostLabel, mountedRef, onDeleted, runtimeTarget] + ) + + return { + supported, + unsupportedReason: supported ? null : unsupportedReason(runtimeTarget), + running, + result, + dismissResult: useCallback(() => setResult(null), []), + requestDelete + } +} + +/** Turns a plan in which nothing is actionable into the same result shape the + * band already groups, so one component renders every skip reason. */ +function blockedOnlyResult(plan: SkillDeletePlan): SkillDeleteResult { + return { + operationId: plan.operationId, + skills: plan.skills.map((skill) => ({ + id: skill.id, + name: skill.name, + status: 'skipped' as const, + ...(skill.blocked ? { blocked: skill.blocked } : {}), + removedPaths: [] + })) + } +} + +function unsupportedReason(runtimeTarget: RuntimeClientTarget | null): string { + return runtimeTarget + ? SKILL_DELETE_UPDATE_REQUIRED_MESSAGE + : translate( + 'auto.components.skills.SkillDelete.hostUnresolved', + 'Still finding the machine that owns these skills.' + ) +} + +function skillDeletedToast(count: number): string { + return count === 1 + ? translate('auto.components.skills.count.deletedOne', 'Deleted {{count}} skill', { count }) + : translate('auto.components.skills.count.deletedOther', 'Deleted {{count}} skills', { count }) +} diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 3600bbeb99b..bfa7afa23ab 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -4148,7 +4148,8 @@ "retry": "Retry", "remoteShareNotice": "These skills live on {{host}}. Open Skills on that machine to share them.", "viewSwitch": "Show", - "searchLinks": "Search links" + "searchLinks": "Search links", + "deleteSkills": "Delete skills…" }, "SkillFreshnessNudge": { "titleOne": "An installed Orca skill is out of date", @@ -4476,7 +4477,9 @@ "detailSource": "Source", "detailContents": "Contents", "skillActions": "Actions for {{value0}}", - "viewDetails": "View details" + "viewDetails": "View details", + "deleteSkill": "Delete…", + "notDeletable": "Not deletable" }, "SkillSelectionBar": { "selectAll": "Select all {{count}} eligible", @@ -4516,7 +4519,15 @@ "retryOne": "Retry {{count}} skill", "retryOther": "Retry {{count}} skills", "linkOne": "{{count}} link", - "linkOther": "{{count}} links" + "linkOther": "{{count}} links", + "deleteOne": "Delete {{count}} skill", + "deleteOther": "Delete {{count}} skills", + "deletedOne": "Deleted {{count}} skill", + "deletedOther": "Deleted {{count}} skills", + "deleteFolderOne": "{{count}} folder", + "deleteFolderOther": "{{count}} folders", + "deleteLinkOne": "{{count}} link", + "deleteLinkOther": "{{count}} links" }, "host": { "local": "This machine", @@ -4585,7 +4596,8 @@ "exitTooltip": "Exit selection · Esc", "title": "Select skills to share", "selectAll": "Select all {{count}} eligible", - "clear": "Clear" + "clear": "Clear", + "deleteTitle": "Select skills to delete" }, "SkillDetailDialog": { "agents": "Agents", @@ -4639,6 +4651,30 @@ "supportingFilesDescription": "Includes readable reference files beyond the main instructions.", "runnableFilesDescription": "Includes scripts that an agent may run with your access.", "binaryFilesDescription": "Includes opaque assets and an executable binary." + }, + "SkillDelete": { + "dismissResults": "Dismiss", + "reasonBundled": "Bundled with Orca — it would be restored", + "reasonPlugin": "Installed by a plugin — remove the plugin instead", + "reasonUnowned": "This skill lives outside Orca’s skill folders — delete it where it is stored", + "reasonMissing": "This skill is no longer on disk", + "reasonStale": "This skill changed since the list was loaded — refresh and try again", + "blockedLine": "{{count}} × {{reason}}", + "resultLine": "{{count}} × {{label}}", + "statusSkipped": "Skipped", + "statusBusy": "Busy — another skill operation is in progress", + "statusPartial": "Partly removed — some files are still on disk under a hidden name", + "statusFailed": "Failed — nothing changed", + "statusDeleted": "Deleted", + "confirmHost": "On {{host}}.", + "confirmPermanent": "This cannot be undone.", + "failed": "Could not delete skills", + "hostUnresolved": "Still finding the machine that owns these 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}}.", + "placementJoin": " and ", + "retainedSource": "The skill itself stays at {{path}}." } }, "sidebar": { diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 9f1ef9ca5cc..d015c9e6cb3 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -3960,6 +3960,10 @@ "SkillUpdateRow": { "oneLocation": "1 location", "manyLocations": "{{value0}} locations" + }, + "host": { + "local": "Esta máquina", + "remote": "Entorno conectado" } }, "sidebar": { diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 508c9d9bec3..7be319a6b13 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -3960,6 +3960,10 @@ "SkillUpdateRow": { "oneLocation": "1 件の場所", "manyLocations": "{{value0}} 件の場所" + }, + "host": { + "local": "このマシン", + "remote": "接続済みランタイム" } }, "sidebar": { diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 6164f1707bd..df6735ae6d5 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -3962,6 +3962,10 @@ "SkillUpdateRow": { "oneLocation": "1 location", "manyLocations": "{{value0}} locations" + }, + "host": { + "local": "이 컴퓨터", + "remote": "연결된 런타임" } }, "sidebar": { diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 84c05074bb5..19cfe205961 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -3972,6 +3972,36 @@ "SkillUpdateRow": { "oneLocation": "1 location", "manyLocations": "{{value0}} locations" + }, + "count": { + "deleteOne": "删除 {{count}} 个技能", + "deleteOther": "删除 {{count}} 个技能", + "deletedOne": "已删除 {{count}} 个技能", + "deletedOther": "已删除 {{count}} 个技能" + }, + "host": { + "local": "此机器", + "remote": "已连接的运行时" + }, + "SkillDelete": { + "dismissResults": "关闭", + "reasonBundled": "Orca 内置技能 — 删除后会恢复", + "reasonPlugin": "由插件安装 — 请改为移除插件", + "reasonUnowned": "此技能位于 Orca 技能文件夹之外 — 请在其存储位置删除", + "reasonMissing": "此技能已不在磁盘上", + "reasonStale": "此技能在列表加载后发生了变化 — 请刷新后重试", + "placementSummary": "将在 {{roots}} 中删除 {{folders}} 个文件夹和 {{links}} 个链接。", + "blockedLine": "{{count}} × {{reason}}", + "resultLine": "{{count}} × {{label}}", + "statusSkipped": "已跳过", + "statusBusy": "忙碌 — 另一项技能操作正在进行", + "statusPartial": "部分移除 — 某些文件仍以隐藏名称保留在磁盘上", + "statusFailed": "失败 — 未进行任何更改", + "statusDeleted": "已删除", + "confirmHost": "位于 {{host}}。", + "confirmPermanent": "此操作无法撤销。", + "failed": "无法删除技能", + "hostUnresolved": "仍在查找存储这些技能的计算机。" } }, "sidebar": { diff --git a/src/renderer/src/runtime/runtime-skills-client.ts b/src/renderer/src/runtime/runtime-skills-client.ts index 40101379a81..3a3cc224fdf 100644 --- a/src/renderer/src/runtime/runtime-skills-client.ts +++ b/src/renderer/src/runtime/runtime-skills-client.ts @@ -1,5 +1,19 @@ +import type { + SkillDeletePlan, + SkillDeleteRequest, + SkillDeleteResult +} from '../../../shared/skill-delete-contract' +import { + SKILL_DELETE_CAPABILITY, + SKILL_DELETE_UPDATE_REQUIRED_MESSAGE +} from '../../../shared/skill-install-capability' import type { SkillDiscoveryResult, SkillDiscoveryTarget } from '../../../shared/skills' -import { callRuntimeRpc, type RuntimeClientTarget } from './runtime-rpc-client' +import { + assertRuntimeEnvironmentCapability, + callRuntimeRpc, + runtimeEnvironmentSupportsCapability, + type RuntimeClientTarget +} from './runtime-rpc-client' const SKILL_DISCOVERY_TIMEOUT_MS = 15_000 @@ -35,3 +49,69 @@ export async function discoverSkillsForRuntimeTarget( { timeoutMs: SKILL_DISCOVERY_TIMEOUT_MS } ) } + +const SKILL_DELETE_PREVIEW_TIMEOUT_MS = 60_000 +const SKILL_DELETE_TIMEOUT_MS = 5 * 60_000 + +/** + * Whether the delete affordance may be offered at all. The gate lives here, + * beside where `callRuntimeRpc` is actually invoked — the main-process + * `callRuntimeEnvironment` path install and remove use is a different, + * non-overlapping transport, so a check there would never run for this. + */ +export async function runtimeTargetSupportsSkillDelete( + runtimeTarget: RuntimeClientTarget | null +): Promise { + if (!runtimeTarget) { + return false + } + if (runtimeTarget.kind === 'local') { + return true + } + return runtimeEnvironmentSupportsCapability(runtimeTarget.environmentId, SKILL_DELETE_CAPABILITY) +} + +async function assertSkillDeleteSupported(runtimeTarget: RuntimeClientTarget): Promise { + if (runtimeTarget.kind === 'local') { + return + } + try { + await assertRuntimeEnvironmentCapability( + runtimeTarget.environmentId, + SKILL_DELETE_CAPABILITY, + SKILL_DELETE_UPDATE_REQUIRED_MESSAGE + ) + } catch (error) { + // A capability change racing the gate has no main-process hook to reuse: + // `recordSkillCapabilityAbsence` imports the main tracer and its capability + // parameter is a closed union over main-side capabilities. + console.warn('[skills] delete capability absent at call time', error) + throw error + } +} + +export async function previewSkillDeletionOnRuntimeTarget( + runtimeTarget: RuntimeClientTarget, + request: SkillDeleteRequest +): Promise { + await assertSkillDeleteSupported(runtimeTarget) + if (runtimeTarget.kind === 'local') { + return window.api.skills.previewDelete(request) + } + return callRuntimeRpc(runtimeTarget, 'skills.previewDelete', request, { + timeoutMs: SKILL_DELETE_PREVIEW_TIMEOUT_MS + }) +} + +export async function deleteSkillsOnRuntimeTarget( + runtimeTarget: RuntimeClientTarget, + request: SkillDeleteRequest +): Promise { + await assertSkillDeleteSupported(runtimeTarget) + if (runtimeTarget.kind === 'local') { + return window.api.skills.delete(request) + } + return callRuntimeRpc(runtimeTarget, 'skills.delete', request, { + timeoutMs: SKILL_DELETE_TIMEOUT_MS + }) +} diff --git a/src/renderer/src/runtime/runtime-skills-delete-client.test.ts b/src/renderer/src/runtime/runtime-skills-delete-client.test.ts new file mode 100644 index 00000000000..88bfe40f5ed --- /dev/null +++ b/src/renderer/src/runtime/runtime-skills-delete-client.test.ts @@ -0,0 +1,98 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { SKILL_DELETE_UPDATE_REQUIRED_MESSAGE } from '../../../shared/skill-install-capability' +import type { SkillDeleteRequest } from '../../../shared/skill-delete-contract' + +const callRuntimeRpc = vi.fn() +const runtimeEnvironmentSupportsCapability = vi.fn() +const assertRuntimeEnvironmentCapability = vi.fn() + +vi.mock('./runtime-rpc-client', () => ({ + callRuntimeRpc: (...args: unknown[]) => callRuntimeRpc(...args), + runtimeEnvironmentSupportsCapability: (...args: unknown[]) => + runtimeEnvironmentSupportsCapability(...args), + assertRuntimeEnvironmentCapability: (...args: unknown[]) => + assertRuntimeEnvironmentCapability(...args) +})) + +const { + deleteSkillsOnRuntimeTarget, + previewSkillDeletionOnRuntimeTarget, + runtimeTargetSupportsSkillDelete +} = await import('./runtime-skills-client') + +const REQUEST: SkillDeleteRequest = { + operationId: 'op', + skills: [ + { id: 'a', directoryPath: '/d', skillFilePath: '/d/SKILL.md', name: 'demo', updatedAt: 1 } + ] +} + +const localDelete = vi.fn() +const localPreview = vi.fn() + +beforeEach(() => { + vi.clearAllMocks() + vi.stubGlobal('window', { + api: { skills: { delete: localDelete, previewDelete: localPreview } } + }) +}) + +describe('runtimeTargetSupportsSkillDelete', () => { + it('never probes the local target', async () => { + expect(await runtimeTargetSupportsSkillDelete({ kind: 'local' })).toBe(true) + expect(runtimeEnvironmentSupportsCapability).not.toHaveBeenCalled() + }) + + it('is unsupported while the runtime target is still unresolved', async () => { + expect(await runtimeTargetSupportsSkillDelete(null)).toBe(false) + expect(runtimeEnvironmentSupportsCapability).not.toHaveBeenCalled() + }) + + it('asks the environment for the capability', async () => { + runtimeEnvironmentSupportsCapability.mockResolvedValue(false) + expect( + await runtimeTargetSupportsSkillDelete({ kind: 'environment', environmentId: 'env-1' }) + ).toBe(false) + expect(runtimeEnvironmentSupportsCapability).toHaveBeenCalledWith('env-1', 'skills.delete.v1') + }) +}) + +describe('delete routing', () => { + it('issues no RPC against a host that lacks the capability', async () => { + assertRuntimeEnvironmentCapability.mockRejectedValue( + new Error(SKILL_DELETE_UPDATE_REQUIRED_MESSAGE) + ) + await expect( + deleteSkillsOnRuntimeTarget({ kind: 'environment', environmentId: 'env-1' }, REQUEST) + ).rejects.toThrow(SKILL_DELETE_UPDATE_REQUIRED_MESSAGE) + expect(callRuntimeRpc).not.toHaveBeenCalled() + }) + + it('routes a local delete through IPC rather than RPC', async () => { + localDelete.mockResolvedValue({ operationId: 'op', skills: [] }) + await deleteSkillsOnRuntimeTarget({ kind: 'local' }, REQUEST) + expect(localDelete).toHaveBeenCalledWith(REQUEST) + expect(callRuntimeRpc).not.toHaveBeenCalled() + expect(assertRuntimeEnvironmentCapability).not.toHaveBeenCalled() + }) + + it('sends the whole request to a capable remote host', async () => { + assertRuntimeEnvironmentCapability.mockResolvedValue(undefined) + callRuntimeRpc.mockResolvedValue({ operationId: 'op', skills: [] }) + await deleteSkillsOnRuntimeTarget({ kind: 'environment', environmentId: 'env-1' }, REQUEST) + expect(callRuntimeRpc).toHaveBeenCalledWith( + { kind: 'environment', environmentId: 'env-1' }, + 'skills.delete', + REQUEST, + expect.objectContaining({ timeoutMs: expect.any(Number) }) + ) + }) + + it('gates the preview on the same capability', async () => { + assertRuntimeEnvironmentCapability.mockRejectedValue(new Error('nope')) + await expect( + previewSkillDeletionOnRuntimeTarget({ kind: 'environment', environmentId: 'env-1' }, REQUEST) + ).rejects.toThrow('nope') + expect(callRuntimeRpc).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/web/web-preload-api.ts b/src/renderer/src/web/web-preload-api.ts index 5e492168b58..70a24dc3ee9 100644 --- a/src/renderer/src/web/web-preload-api.ts +++ b/src/renderer/src/web/web-preload-api.ts @@ -49,6 +49,7 @@ import type { } from '../../../shared/worktree/create-types' import type { WorkspaceLineage, WorktreeLineage } from '../../../shared/worktree/lineage-types' import type { DetectedWorktreeListResult, Worktree } from '../../../shared/worktree/types' +import type { SkillDeletePlan, SkillDeleteResult } from '../../../shared/skill-delete-contract' import type { SkillDiscoveryResult } from '../../../shared/skills' import type { SkillFreshnessInventory } from '../../../shared/skill-freshness' import type { SshConnectionState, SshTarget } from '../../../shared/ssh-types' @@ -3159,6 +3160,11 @@ function createSkillsApi(): NonNullable['skills']> { previewBundleInstall: () => Promise.reject(new Error('Skill installation requires the desktop app.')), removeInstall: () => Promise.reject(new Error('Skill installation requires the desktop app.')), + // Why routed rather than refused: on web the "local" host IS the runtime, so + // delete rides the same RPC the scan already does. + previewDelete: (request) => + callRuntimeResult('skills.previewDelete', request, 60_000), + delete: (request) => callRuntimeResult('skills.delete', request, 5 * 60_000), listManagedInstalls: () => Promise.reject(new Error('Skill installation requires the desktop app.')), getPackage: () => Promise.reject(new Error('Skill installation requires the desktop app.')), diff --git a/src/shared/protocol-version.ts b/src/shared/protocol-version.ts index d9d356c676a..69fe4da6f89 100644 --- a/src/shared/protocol-version.ts +++ b/src/shared/protocol-version.ts @@ -1,6 +1,7 @@ import { REMOTE_SERVER_UPDATE_CAPABILITY } from './remote-server-update' import { SKILL_BUNDLE_INSTALL_CAPABILITY, + SKILL_DELETE_CAPABILITY, SKILL_INSTALL_CAPABILITY, SKILL_INSTALL_CANCEL_CAPABILITY, SKILL_INSTALL_PROGRESS_CAPABILITY, @@ -161,7 +162,8 @@ export const RUNTIME_CAPABILITIES = [ SKILL_INSTALL_RESULT_V2_CAPABILITY, SKILL_UPLOAD_CAPABILITY, SKILL_MANAGEMENT_CAPABILITY, - SKILL_INSTALL_PROVIDERS_CAPABILITY + SKILL_INSTALL_PROVIDERS_CAPABILITY, + SKILL_DELETE_CAPABILITY ] as const export type RuntimeCapability = (typeof RUNTIME_CAPABILITIES)[number] | (string & {}) diff --git a/src/shared/skill-delete-contract.test.ts b/src/shared/skill-delete-contract.test.ts new file mode 100644 index 00000000000..91c0b276461 --- /dev/null +++ b/src/shared/skill-delete-contract.test.ts @@ -0,0 +1,111 @@ +import { describe, expect, it } from 'vitest' +import { + MAX_SKILL_DELETE_BATCH, + SkillDeletePlanSchema, + SkillDeleteRequestSchema, + SkillDeleteResultSchema +} from './skill-delete-contract' + +const SKILL = { + id: 'skill-id', + directoryPath: '/root/demo', + skillFilePath: '/root/demo/SKILL.md', + name: 'demo', + updatedAt: 1 +} + +describe('SkillDeleteRequestSchema', () => { + it('accepts a request with no target, which is what the page sends', () => { + expect(SkillDeleteRequestSchema.parse({ operationId: 'op', skills: [SKILL] })).toEqual({ + operationId: 'op', + skills: [SKILL] + }) + }) + + it('rejects an unknown top-level field rather than ignoring it', () => { + // Deliberately strict: an old host must refuse a field that would change + // what gets deleted, not silently drop it. + expect(() => + SkillDeleteRequestSchema.parse({ operationId: 'op', skills: [SKILL], force: true }) + ).toThrow() + }) + + it('strips unknown keys inside the nested discovery target', () => { + const parsed = SkillDeleteRequestSchema.parse({ + operationId: 'op', + target: { runtime: 'host', somethingNew: 1 }, + skills: [SKILL] + }) + expect(parsed.target).toEqual({ runtime: 'host' }) + }) + + it('keeps a null updatedAt, which the host fails closed on', () => { + const parsed = SkillDeleteRequestSchema.parse({ + operationId: 'op', + skills: [{ ...SKILL, updatedAt: null }] + }) + expect(parsed.skills[0].updatedAt).toBeNull() + }) + + it('refuses an empty or oversized batch', () => { + expect(() => SkillDeleteRequestSchema.parse({ operationId: 'op', skills: [] })).toThrow() + expect(() => + SkillDeleteRequestSchema.parse({ + operationId: 'op', + skills: Array.from({ length: MAX_SKILL_DELETE_BATCH + 1 }, () => SKILL) + }) + ).toThrow() + }) +}) + +describe('plan and result schemas', () => { + it('carries a typed block reason so the band can group without string matching', () => { + const plan = SkillDeletePlanSchema.parse({ + operationId: 'op', + skills: [ + { + id: 'a', + name: 'demo', + canonicalPath: '/root/demo/SKILL.md', + placements: [], + blocked: 'bundled' + } + ] + }) + expect(plan.skills[0].blocked).toBe('bundled') + }) + + it('rejects a block reason outside the shared vocabulary', () => { + expect(() => + SkillDeletePlanSchema.parse({ + operationId: 'op', + skills: [{ id: 'a', name: 'demo', canonicalPath: '/p', placements: [], blocked: 'because' }] + }) + ).toThrow() + }) + + it('has no `unsupported` status, because such a delete is never issued', () => { + expect(() => + SkillDeleteResultSchema.parse({ + operationId: 'op', + skills: [{ id: 'a', name: 'demo', status: 'unsupported', removedPaths: [] }] + }) + ).toThrow() + }) + + it('accepts a partial result carrying the staged paths', () => { + const result = SkillDeleteResultSchema.parse({ + operationId: 'op', + skills: [ + { + id: 'a', + name: 'demo', + status: 'partial', + removedPaths: [], + stagedPaths: ['/root/.demo.orca-skill-delete-1'] + } + ] + }) + expect(result.skills[0].stagedPaths).toHaveLength(1) + }) +}) diff --git a/src/shared/skill-delete-contract.ts b/src/shared/skill-delete-contract.ts new file mode 100644 index 00000000000..81aad0857c3 --- /dev/null +++ b/src/shared/skill-delete-contract.ts @@ -0,0 +1,120 @@ +import { z } from 'zod' +import { SkillDiscoveryTargetSchema } from './skills' + +/** Why a closed vocabulary: the result band groups skips by reason, and a remote + * host cannot localize free-text prose for the client that renders it. */ +export const SKILL_DELETE_BLOCK_REASONS = [ + 'bundled', + 'plugin', + 'unowned', + 'missing', + 'stale' +] as const +export type SkillDeleteBlockReason = (typeof SKILL_DELETE_BLOCK_REASONS)[number] + +export const SKILL_DELETE_PLACEMENT_KINDS = ['canonical', 'alias-dir', 'alias-file'] as const +export type SkillDeletePlacementKind = (typeof SKILL_DELETE_PLACEMENT_KINDS)[number] + +export const SKILL_DELETE_STATUSES = ['deleted', 'skipped', 'partial', 'failed', 'busy'] as const +export type SkillDeleteStatus = (typeof SKILL_DELETE_STATUSES)[number] + +/** One page of rows; a selection larger than this is not a thing the UI offers. */ +export const MAX_SKILL_DELETE_BATCH = 512 + +const SkillPathSchema = z.string().min(1).max(4096) + +const SkillDeleteTargetSkillSchema = z + .object({ + /** `DiscoveredSkill.id` — identity, so host and client agree without + * re-deriving it from a path that may have just been renamed. */ + id: z.string().min(1).max(128), + directoryPath: SkillPathSchema, + skillFilePath: SkillPathSchema, + name: z.string().min(1).max(256), + /** `stat(skillFilePath).mtimeMs` as displayed; null fails the guard closed. */ + updatedAt: z.number().nullable() + }) + .strict() + +/** Strict on purpose (matching `SkillInstallRequestSchema`): an old host must + * reject a field that would change what gets deleted, not ignore it. */ +export const SkillDeleteRequestSchema = z + .object({ + operationId: z.string().min(1).max(128), + // Send exactly what the scan sent — usually nothing. + target: SkillDiscoveryTargetSchema.optional(), + skills: z.array(SkillDeleteTargetSkillSchema).min(1).max(MAX_SKILL_DELETE_BATCH) + }) + .strict() + +export type SkillDeleteRequest = z.infer +export type SkillDeleteTargetSkill = z.infer + +export type SkillDeletePlacement = { + path: string + kind: SkillDeletePlacementKind + rootLabel: string +} + +export type SkillDeletePlanEntry = { + id: string + name: string + /** realpath of `skillFilePath`. NOT the lock key — see the delete service. */ + canonicalPath: string + placements: SkillDeletePlacement[] + blocked?: SkillDeleteBlockReason +} + +export type SkillDeletePlan = { + operationId: string + skills: SkillDeletePlanEntry[] +} + +export type SkillDeleteResultEntry = { + id: string + name: string + status: SkillDeleteStatus + /** Present when status is 'skipped'; same vocabulary as the plan. */ + blocked?: SkillDeleteBlockReason + removedPaths: string[] + /** Present when status is 'partial'. */ + stagedPaths?: string[] +} + +export type SkillDeleteResult = { + operationId: string + skills: SkillDeleteResultEntry[] +} + +const SkillDeletePlacementSchema = z.object({ + path: SkillPathSchema, + kind: z.enum(SKILL_DELETE_PLACEMENT_KINDS), + rootLabel: z.string().max(256) +}) + +export const SkillDeletePlanSchema: z.ZodType = z.object({ + operationId: z.string().min(1).max(128), + skills: z.array( + z.object({ + id: z.string().min(1).max(128), + name: z.string().max(256), + canonicalPath: SkillPathSchema, + placements: z.array(SkillDeletePlacementSchema).max(64), + blocked: z.enum(SKILL_DELETE_BLOCK_REASONS).optional() + }) + ) +}) + +export const SkillDeleteResultSchema: z.ZodType = z.object({ + operationId: z.string().min(1).max(128), + skills: z.array( + z.object({ + id: z.string().min(1).max(128), + name: z.string().max(256), + status: z.enum(SKILL_DELETE_STATUSES), + blocked: z.enum(SKILL_DELETE_BLOCK_REASONS).optional(), + removedPaths: z.array(SkillPathSchema).max(64), + stagedPaths: z.array(SkillPathSchema).max(64).optional() + }) + ) +}) diff --git a/src/shared/skill-deletion-eligibility.test.ts b/src/shared/skill-deletion-eligibility.test.ts new file mode 100644 index 00000000000..b54913b29bb --- /dev/null +++ b/src/shared/skill-deletion-eligibility.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, it } from 'vitest' +import { + isSkillDeletable, + skillDeletionEligibility, + SKILL_DELETE_BLOCK_MESSAGES +} from './skill-deletion-eligibility' +import type { SkillSourceKind } from './skills' + +describe('skillDeletionEligibility', () => { + it.each<[SkillSourceKind, boolean]>([ + ['home', true], + ['repo', true], + ['bundled', false], + ['plugin', false] + ])('decides %s', (sourceKind, deletable) => { + expect(skillDeletionEligibility({ sourceKind }).deletable).toBe(deletable) + expect(isSkillDeletable({ sourceKind })).toBe(deletable) + }) + + it('names the reason a bundled skill would come back', () => { + const eligibility = skillDeletionEligibility({ sourceKind: 'bundled' }) + expect(eligibility).toEqual({ + deletable: false, + reason: 'bundled', + message: SKILL_DELETE_BLOCK_MESSAGES.bundled + }) + }) + + it('points a plugin skill at the plugin rather than the file', () => { + const eligibility = skillDeletionEligibility({ sourceKind: 'plugin' }) + expect(eligibility).toMatchObject({ deletable: false, reason: 'plugin' }) + }) + + it('has a message for every host-only reason the client never predicts', () => { + // The client cannot know these; they arrive on the result, and the band + // still has to say something specific for each. + expect(SKILL_DELETE_BLOCK_MESSAGES.unowned).toContain('outside') + expect(SKILL_DELETE_BLOCK_MESSAGES.missing).toBeTruthy() + expect(SKILL_DELETE_BLOCK_MESSAGES.stale).toBeTruthy() + }) +}) diff --git a/src/shared/skill-deletion-eligibility.ts b/src/shared/skill-deletion-eligibility.ts new file mode 100644 index 00000000000..cb066fd42f6 --- /dev/null +++ b/src/shared/skill-deletion-eligibility.ts @@ -0,0 +1,54 @@ +import type { SkillDeleteBlockReason } from './skill-delete-contract' +import type { DiscoveredSkill, SkillSourceKind } from './skills' + +export type SkillDeletionEligibility = + | { deletable: true } + | { deletable: false; reason: SkillDeleteBlockReason; message: string } + +/** English defaults. The renderer translates by reason code; logs and the CLI + * use these directly. */ +export const SKILL_DELETE_BLOCK_MESSAGES: Record = { + bundled: 'Bundled with Orca — it would be restored', + plugin: 'Installed by a plugin — remove the plugin instead', + unowned: 'This skill lives outside Orca’s skill folders — delete it where it is stored', + missing: 'This skill is no longer on disk', + stale: 'This skill changed since the list was loaded — refresh and try again' +} + +function blocked(reason: SkillDeleteBlockReason): SkillDeletionEligibility { + return { deletable: false, reason, message: SKILL_DELETE_BLOCK_MESSAGES[reason] } +} + +/** + * Client-side predicate over the displayed row. + * + * Advisory for `bundled`/`plugin`: `sourceKind` is first-scanned-root-wins, so a + * plugin-cache skill symlinked into a plain home root reports `home` here. The + * host re-derives the verdict from the placement set and can be stricter. + */ +export function skillDeletionEligibility( + skill: Pick +): SkillDeletionEligibility { + return skillSourceKindDeletionEligibility(skill.sourceKind) +} + +export function skillSourceKindDeletionEligibility( + sourceKind: SkillSourceKind +): SkillDeletionEligibility { + switch (sourceKind) { + case 'bundled': { + return blocked('bundled') + } + case 'plugin': { + return blocked('plugin') + } + case 'home': + case 'repo': { + return { deletable: true } + } + } +} + +export function isSkillDeletable(skill: Pick): boolean { + return skillDeletionEligibility(skill).deletable +} diff --git a/src/shared/skill-discovery-depth.ts b/src/shared/skill-discovery-depth.ts new file mode 100644 index 00000000000..da3ed56f8a7 --- /dev/null +++ b/src/shared/skill-discovery-depth.ts @@ -0,0 +1,19 @@ +/** + * One source for how deep a skill file may sit below a discovery root. + * + * Why one constant for two numbers: the native walker bounds *directories* and + * WSL's `find -maxdepth` bounds the *file*, so the two spellings drifted apart + * historically. Expressing the limit on the `SKILL.md` path and deriving the + * directory bound from it keeps native, WSL, and the delete guard on one number. + */ +export const SKILL_FILE_MAX_DEPTH = 5 +export const PLUGIN_SKILL_FILE_MAX_DEPTH = 10 + +export function skillFileMaxDepth(sourceKind: 'home' | 'repo' | 'bundled' | 'plugin'): number { + return sourceKind === 'plugin' ? PLUGIN_SKILL_FILE_MAX_DEPTH : SKILL_FILE_MAX_DEPTH +} + +/** The directory the walker may still descend into — one level above the file. */ +export function skillDirectoryMaxDepth(sourceKind: 'home' | 'repo' | 'bundled' | 'plugin'): number { + return skillFileMaxDepth(sourceKind) - 1 +} diff --git a/src/shared/skill-install-capability.ts b/src/shared/skill-install-capability.ts index a8666fb1b5d..300bb07564b 100644 --- a/src/shared/skill-install-capability.ts +++ b/src/shared/skill-install-capability.ts @@ -12,3 +12,11 @@ export const SKILL_INSTALL_PROVIDERS_CAPABILITY = 'skills.install-providers.v1' export const SKILL_INSTALL_UPDATE_REQUIRED_MESSAGE = 'Update Orca on the selected machine to install shared skills.' + +/** Why capability-gated rather than Rule 1: `skills.delete` is a new RPC method, + * and an old host answers method-not-found, which surfaces as an unexplained + * failure instead of a disabled action with a reason. */ +export const SKILL_DELETE_CAPABILITY = 'skills.delete.v1' as const + +export const SKILL_DELETE_UPDATE_REQUIRED_MESSAGE = + 'Update Orca on the selected machine to delete skills.' diff --git a/src/shared/skill-path-containment.test.ts b/src/shared/skill-path-containment.test.ts new file mode 100644 index 00000000000..07bdc756aa5 --- /dev/null +++ b/src/shared/skill-path-containment.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from 'vitest' +import { + nativeSkillPathSemantics, + normalizedSkillPath, + POSIX_SKILL_PATH_SEMANTICS, + skillPathDepthBelow, + skillPathInside, + skillPathsEqual, + WINDOWS_SKILL_PATH_SEMANTICS +} from './skill-path-containment' + +describe('skillPathInside', () => { + it('refuses the root itself, so a delete can never target a discovery root', () => { + expect( + skillPathInside( + '/home/u/.claude/skills', + '/home/u/.claude/skills', + POSIX_SKILL_PATH_SEMANTICS + ) + ).toBe(false) + }) + + it('refuses a `..` escape', () => { + expect( + skillPathInside( + '/home/u/.claude/skills', + '/home/u/.claude/skills/../other', + POSIX_SKILL_PATH_SEMANTICS + ) + ).toBe(false) + expect( + skillPathInside('/home/u/.claude/skills', '/home/u/other', POSIX_SKILL_PATH_SEMANTICS) + ).toBe(false) + }) + + it('accepts a child', () => { + expect( + skillPathInside( + '/home/u/.claude/skills', + '/home/u/.claude/skills/foo', + POSIX_SKILL_PATH_SEMANTICS + ) + ).toBe(true) + }) + + it('keeps POSIX case-sensitive even when the process is win32', () => { + // A WSL runtime is POSIX whatever `process.platform` says. + expect( + skillPathInside('/home/u/Skills', '/home/u/skills/foo', POSIX_SKILL_PATH_SEMANTICS) + ).toBe(false) + expect(skillPathsEqual('/a/B', '/a/b', POSIX_SKILL_PATH_SEMANTICS)).toBe(false) + }) + + it('folds case for Windows semantics', () => { + expect( + skillPathInside( + 'C:\\Users\\u\\.claude\\skills', + 'c:\\users\\U\\.claude\\skills\\foo', + WINDOWS_SKILL_PATH_SEMANTICS + ) + ).toBe(true) + expect(skillPathsEqual('C:\\A', 'c:\\a', WINDOWS_SKILL_PATH_SEMANTICS)).toBe(true) + }) + + it('does not treat a `..`-prefixed name as a traversal', () => { + expect(skillPathInside('/root', '/root/..cache', POSIX_SKILL_PATH_SEMANTICS)).toBe(true) + }) +}) + +describe('skillPathDepthBelow', () => { + it('counts segments and returns null outside the root', () => { + expect(skillPathDepthBelow('/r', '/r/a/SKILL.md', POSIX_SKILL_PATH_SEMANTICS)).toBe(2) + expect(skillPathDepthBelow('/r', '/r/a/b/c/d/SKILL.md', POSIX_SKILL_PATH_SEMANTICS)).toBe(5) + expect(skillPathDepthBelow('/r', '/other/SKILL.md', POSIX_SKILL_PATH_SEMANTICS)).toBeNull() + }) +}) + +describe('nativeSkillPathSemantics', () => { + it('reads the platform it is given rather than the process it runs in', () => { + expect(nativeSkillPathSemantics('win32')).toEqual(WINDOWS_SKILL_PATH_SEMANTICS) + expect(nativeSkillPathSemantics('linux')).toEqual(POSIX_SKILL_PATH_SEMANTICS) + expect(nativeSkillPathSemantics('darwin')).toEqual(POSIX_SKILL_PATH_SEMANTICS) + }) +}) + +describe('normalizedSkillPath', () => { + it('resolves before comparing', () => { + expect(normalizedSkillPath('/a/b/../c', POSIX_SKILL_PATH_SEMANTICS)).toBe('/a/c') + }) +}) diff --git a/src/shared/skill-path-containment.ts b/src/shared/skill-path-containment.ts new file mode 100644 index 00000000000..f9d694aedfe --- /dev/null +++ b/src/shared/skill-path-containment.ts @@ -0,0 +1,72 @@ +import { posix, win32 } from 'node:path' + +/** + * Path semantics of the host that owns the files, which is not always the host + * running this process: a WSL runtime is POSIX and case-sensitive while the + * process platform is `win32`, and a remote Orca runtime has its own platform. + */ +export type SkillPathSemantics = { caseSensitive: boolean; sep: '/' | '\\' } + +export const POSIX_SKILL_PATH_SEMANTICS: SkillPathSemantics = { caseSensitive: true, sep: '/' } +export const WINDOWS_SKILL_PATH_SEMANTICS: SkillPathSemantics = { caseSensitive: false, sep: '\\' } + +/** Semantics of the process's own filesystem. */ +export function nativeSkillPathSemantics( + platform: NodeJS.Platform = process.platform +): SkillPathSemantics { + return platform === 'win32' ? WINDOWS_SKILL_PATH_SEMANTICS : POSIX_SKILL_PATH_SEMANTICS +} + +function pathApi(semantics: SkillPathSemantics): typeof posix { + return semantics.sep === '\\' ? win32 : posix +} + +function comparable(path: string, semantics: SkillPathSemantics): string { + const resolved = pathApi(semantics).resolve(path) + return semantics.caseSensitive ? resolved : resolved.toLocaleLowerCase('en-US') +} + +export function normalizedSkillPath(path: string, semantics: SkillPathSemantics): string { + return comparable(path, semantics) +} + +export function skillPathsEqual( + left: string, + right: string, + semantics: SkillPathSemantics +): boolean { + return comparable(left, semantics) === comparable(right, semantics) +} + +/** True when `path` sits strictly below `root`; the root itself is not inside. */ +export function skillPathInside( + root: string, + path: string, + semantics: SkillPathSemantics +): boolean { + return relativeInside(root, path, semantics) !== null +} + +/** Segment count of `path` below `root`; null when `path` is not inside `root`. */ +export function skillPathDepthBelow( + root: string, + path: string, + semantics: SkillPathSemantics +): number | null { + const child = relativeInside(root, path, semantics) + return child === null ? null : child.split(semantics.sep).length +} + +function relativeInside(root: string, path: string, semantics: SkillPathSemantics): string | null { + const api = pathApi(semantics) + const child = api.relative(comparable(root, semantics), comparable(path, semantics)) + if ( + child === '' || + child === '..' || + child.startsWith(`..${semantics.sep}`) || + api.isAbsolute(child) + ) { + return null + } + return child +}