diff --git a/docs/design.md b/docs/design.md index 028989426..06316bea3 100644 --- a/docs/design.md +++ b/docs/design.md @@ -98,7 +98,7 @@ Five primitives. The pick is determined by option count, the need to search, and `` is for a focused task. Multi-field forms (`packages/app/src/components/add-host-modal.tsx`, `packages/app/src/components/pair-link-modal.tsx`, `packages/app/src/components/project-picker-modal.tsx`), confirmations with detail, anything that earns a backdrop. Bottom sheet on compact, centered card on desktop. Raw `Modal` is wrong for any of these. -`` owns compact bottom safe-area padding inside the sheet so the sheet background still reaches the screen bottom. If a sheet's first snap point is shorter than its header, content, and safe-area clearance, raise that snap point rather than moving the sheet container. +`` owns the presentation. Its content inset — the gutter that puts sheet content on the same rails as the sheet header — and compact bottom safe-area padding are the sheet's, not the caller's. A caller declares layout intent through `contentStyle` and never branches on form factor to add its own margins. If a sheet's first snap point is shorter than its header, content, and safe-area clearance, raise that snap point rather than moving the sheet container. `confirmDialog` is for destructive yes/no and imperative confirmation. Promise-based: `await confirmDialog({ destructive: true, ... })`. Anything where a wrong click loses work. diff --git a/docs/unistyles.md b/docs/unistyles.md index 619ceed3b..7d54a3b4e 100644 --- a/docs/unistyles.md +++ b/docs/unistyles.md @@ -129,7 +129,12 @@ This mistake once produced tens of thousands of warnings from retained sidebar r ## Main Gotcha: `contentContainerStyle` -`ScrollView.contentContainerStyle` is the canonical trap. It looks like a style prop, but it is not the same prop that Unistyles' remapped native component registers by default. The upstream tutorial calls this out directly in its [ScrollView Background Issue](https://www.unistyl.es/v3/tutorial/settings-screen#scrollview-background-issue) section. +`ScrollView.contentContainerStyle` is the canonical trap. It looks like a style prop, but it is not the `style` prop Unistyles registers. How badly that bites depends on who owns the component: + +- **A remapped React Native `ScrollView`** still picks the style up — the Babel plugin rewrites the import and the factory tracks `contentContainerStyle` alongside `style`. The failure mode here is staleness: on first mount it can paint with the theme that was active at mount time and never repaint. The upstream tutorial calls this out in its [ScrollView Background Issue](https://www.unistyl.es/v3/tutorial/settings-screen#scrollview-background-issue) section. +- **A third-party scroller** — `BottomSheetScrollView`, `FlashList`, anything Unistyles never rewrote — drops the style entirely on web. Nothing throws and the value looks right in JS, but the class Unistyles emitted is never applied, so the padding, gap, or background is simply absent. `AdaptiveModalSheet` shipped its compact sheet content flush to the screen edges for exactly this reason while the desktop card, which uses a real `ScrollView`, looked correct. + +The second case is why the wrapper-`View` fix below is not optional styling advice. A themed inset belongs on a `View` you own; a third-party scroller only gets theme-free layout. Avoid this pattern when the style depends on the theme: @@ -144,7 +149,7 @@ const styles = StyleSheet.create((theme) => ({ })); ``` -On first mount this can paint with the current adaptive or initial theme. If app settings later load a persisted theme and call [`UnistylesRuntime.setTheme`](https://www.unistyl.es/v3/guides/theming#change-theme), the JS-side style proxy may report the new theme while the native content container keeps the old background. That is how the welcome screen ended up with a light background and dark foreground/buttons. +If app settings later load a persisted theme and call [`UnistylesRuntime.setTheme`](https://www.unistyl.es/v3/guides/theming#change-theme), the JS-side style proxy reports the new theme while the native content container keeps the old background. That is how the welcome screen ended up with a light background and dark foreground/buttons. This applies broadly to non-`style` props that carry theme-dependent values, such as component props named `color`, `trackColor`, `tintColor`, `backgroundStyle`, `handleIndicatorStyle`, and other library-specific style props. The [3rd-party view decision algorithm](https://www.unistyl.es/v3/references/3rd-party-views) recommends explicit handling for these cases, and [issue #1030](https://github.com/jpudysz/react-native-unistyles/issues/1030) shows a related native-prop update edge case around `Image.tintColor`. Treat these values as React props unless wrapped with `withUnistyles`. diff --git a/packages/app/e2e/adaptive-sheet-content-inset.spec.ts b/packages/app/e2e/adaptive-sheet-content-inset.spec.ts new file mode 100644 index 000000000..1be86f2df --- /dev/null +++ b/packages/app/e2e/adaptive-sheet-content-inset.spec.ts @@ -0,0 +1,101 @@ +import { expect, test, type Page } from "./fixtures"; +import { gotoAppShell } from "./helpers/app"; +import { + readRails, + readSheetHeaderRails, + readVerticalGap, + waitForSettledPosition, +} from "./helpers/sheet-layout"; +import { + openCompactSkillsIntegrations, + openSkillSelection, + openSkillsIntegrations, + startSkillsSandbox, + type SkillsSandbox, +} from "./helpers/skills-selection"; + +const COMPACT_VIEWPORT = { width: 390, height: 844 }; +const DESKTOP_VIEWPORT = { width: 1280, height: 900 }; + +const sandboxes: SkillsSandbox[] = []; + +test.afterEach(async () => { + for (const sandbox of sandboxes.splice(0)) { + await sandbox.cleanup(); + } +}); + +async function openChooseSkillsOnPhone(page: Page): Promise { + sandboxes.push(await startSkillsSandbox(page)); + await gotoAppShell(page); + await openCompactSkillsIntegrations(page); + await openSkillSelection(page); + await waitForSettledPosition(page.getByTestId("skill-selection-sheet")); +} + +async function openChooseSkillsOnDesktop(page: Page): Promise { + sandboxes.push(await startSkillsSandbox(page)); + await gotoAppShell(page); + await openSkillsIntegrations(page); + await openSkillSelection(page); +} + +function sheetHeaderRails(page: Page) { + return readSheetHeaderRails(page, "skill-selection-sheet", "Choose skills"); +} + +function allSkillsCard(page: Page) { + return page.getByTestId("skill-selection-all-card"); +} + +function skillListCard(page: Page) { + return page.getByTestId("skill-selection-list-card"); +} + +function bundledSkillsLabel(page: Page) { + return page.getByText("Bundled skills", { exact: true }); +} + +test.describe("Choose skills sheet on a compact form factor", () => { + test.use({ viewport: COMPACT_VIEWPORT }); + + test("keeps its cards on the sheet header rails", async ({ page }) => { + await openChooseSkillsOnPhone(page); + + const header = await sheetHeaderRails(page); + + expect(await readRails(allSkillsCard(page))).toEqual(header); + expect(await readRails(skillListCard(page))).toEqual(header); + }); + + test("gives the Bundled skills label more room above it than below", async ({ page }) => { + await openChooseSkillsOnPhone(page); + + expect({ + aboveLabel: await readVerticalGap(allSkillsCard(page), bundledSkillsLabel(page)), + belowLabel: await readVerticalGap(bundledSkillsLabel(page), skillListCard(page)), + }).toEqual({ aboveLabel: 16, belowLabel: 12 }); + }); +}); + +test.describe("Choose skills sheet on a desktop form factor", () => { + test.use({ viewport: DESKTOP_VIEWPORT }); + + test("keeps its cards on the sheet header rails", async ({ page }) => { + await openChooseSkillsOnDesktop(page); + + const header = await sheetHeaderRails(page); + + expect(await readRails(allSkillsCard(page))).toEqual(header); + expect(await readRails(skillListCard(page))).toEqual(header); + }); + + test("gives the Bundled skills label more room above it than below", async ({ page }) => { + await openChooseSkillsOnDesktop(page); + + expect({ + aboveLabel: await readVerticalGap(allSkillsCard(page), bundledSkillsLabel(page)), + belowLabel: await readVerticalGap(bundledSkillsLabel(page), skillListCard(page)), + }).toEqual({ aboveLabel: 16, belowLabel: 12 }); + }); +}); diff --git a/packages/app/e2e/helpers/sheet-layout.ts b/packages/app/e2e/helpers/sheet-layout.ts new file mode 100644 index 000000000..08e5bb25c --- /dev/null +++ b/packages/app/e2e/helpers/sheet-layout.ts @@ -0,0 +1,68 @@ +import { expect, type Locator, type Page } from "@playwright/test"; + +export interface Rails { + left: number; + right: number; +} + +interface Box { + x: number; + y: number; + width: number; + height: number; +} + +async function readBox(locator: Locator): Promise { + const box = await locator.boundingBox(); + expect(box, "Expected the element to be laid out").not.toBeNull(); + return box as Box; +} + +/** Leading and trailing edges of an element, in whole pixels. */ +export async function readRails(locator: Locator): Promise { + const box = await readBox(locator); + return { left: Math.round(box.x), right: Math.round(box.x + box.width) }; +} + +/** + * The two rails an AdaptiveModalSheet header indents to: the title's leading + * glyph and the close button's trailing edge. Sheet content sits on the same + * two rails, in every presentation. + */ +export async function readSheetHeaderRails( + page: Page, + sheetTestID: string, + title: string, +): Promise { + const header = page.getByTestId(sheetTestID); + const [titleRails, closeRails] = await Promise.all([ + readRails(header.getByText(title, { exact: true })), + readRails(header.getByRole("button", { name: "Close" })), + ]); + return { left: titleRails.left, right: closeRails.right }; +} + +/** Vertical space between the bottom of one element and the top of the next. */ +export async function readVerticalGap(upper: Locator, lower: Locator): Promise { + const [above, below] = await Promise.all([readBox(upper), readBox(lower)]); + return Math.round(below.y - (above.y + above.height)); +} + +/** + * A compact sheet slides up into place, so anything measured across two reads + * has to wait for the surface to stop moving first. + */ +export async function waitForSettledPosition(locator: Locator): Promise { + let previousTop: number | null = null; + await expect + .poll( + async () => { + const { y } = await readBox(locator); + const settled = previousTop === y; + previousTop = y; + return settled; + }, + { message: "Expected the sheet to stop moving" }, + ) + .toBe(true); +} diff --git a/packages/app/e2e/helpers/skills-selection.ts b/packages/app/e2e/helpers/skills-selection.ts new file mode 100644 index 000000000..873fca479 --- /dev/null +++ b/packages/app/e2e/helpers/skills-selection.ts @@ -0,0 +1,410 @@ +import { mkdir, mkdtemp, readdir, readFile, rm, writeFile } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { expect, type Page } from "@playwright/test"; +import { buildOpenProjectRoute } from "@/utils/host-routes"; +import { openSettings } from "./app"; +import { injectDesktopBridge } from "./desktop-updates"; +import { getServerId } from "./server-id"; +import { openCompactSettings, openSettingsSection } from "./settings"; +// The desktop skills module is the real command surface: the same handlers the +// Electron main process registers, running against a temp bundle and temp user +// data. Nothing about persistence or convergence is simulated in the browser. +import type { + SkillSelection, + SkillTargets, +} from "../../../desktop/src/integrations/skills/operations.js"; +import { createSkillSelectionStore } from "../../../desktop/src/integrations/skills/selection-store.js"; +import { createSkillsCommandHandlers } from "../../../desktop/src/integrations/skills/skills-commands.js"; +import { createSkillsController } from "../../../desktop/src/integrations/skills/skills-controller.js"; + +const SKILL_COMMANDS = [ + "get_skills_status", + "install_skills", + "update_skills", + "uninstall_skills", + "save_skills_selection", +] as const; + +declare global { + interface Window { + __paseoSkillsInvoke?: ( + command: string, + args: Record | null, + ) => Promise; + } +} + +export interface SkillsSandbox { + targets: SkillTargets; + userDataPath: string; + bundledSkills: string[]; + /** Writes a managed skill directory the way an external tool would. */ + placeSkillOnDisk: (skill: string, target: SkillTargetName) => Promise; + /** Commits a selection through the real controller, as another window would. */ + commitSelectionExternally: ( + selection: SkillSelection, + confirmedRemovals?: readonly string[], + ) => Promise; + /** Waits until a held save reaches the desktop command boundary. */ + waitForHeldSave: () => Promise; + /** Releases a save held at the desktop command boundary. */ + releaseHeldSave: () => void; + /** Test-only command-boundary gate used to keep the renderer in its pending state. */ + holdSaveResponse?: () => Promise; + cleanup: () => Promise; +} + +export interface SkillsSandboxOptions { + bundledSkills?: string[]; + /** + * Puts a regular file where the `.agents` skills tree has to go, so the real + * convergence fails with ENOTDIR instead of a stubbed rejection. + */ + blockAgentsDir?: boolean; + /** Converge and commit this selection before the app loads, through the real controller. */ + installed?: SkillSelection; + /** + * Leaves a skill installed in one agent home only, the way a partial install + * or a hand-deleted directory leaves it. + */ + keepOnlyIn?: { skill: string; target: SkillTargetName }; + /** + * Writes a skill directory straight into one agent home after the selection is + * committed, the way a manual reinstall or an external sync leaves a directory + * the saved preference does not mention. + */ + placeOnDisk?: { skill: string; target: SkillTargetName; files: Record }; + /** Holds the save response so dismissal behavior can be asserted while pending. */ + holdSave?: boolean; +} + +export type SkillTargetName = "agents" | "claude" | "codex"; + +function targetDir(targets: SkillTargets, target: SkillTargetName): string { + if (target === "agents") return targets.agentsDir; + if (target === "claude") return targets.claudeDir; + return targets.codexDir; +} + +export async function createSkillsSandbox( + options: SkillsSandboxOptions = {}, +): Promise { + let releaseHeldSave: (() => void) | null = null; + let markSaveHeld: (() => void) | null = null; + const heldSave = new Promise((resolve) => { + markSaveHeld = resolve; + }); + const saveRelease = new Promise((resolve) => { + releaseHeldSave = resolve; + }); + const bundledSkills = options.bundledSkills ?? ["paseo", "paseo-advisor", "paseo-loop"]; + const root = await mkdtemp(path.join(os.tmpdir(), "paseo-e2e-skills-")); + const targets: SkillTargets = { + sourceDir: path.join(root, "bundle"), + agentsDir: path.join(root, "home", ".agents", "skills"), + claudeDir: path.join(root, "home", ".claude", "skills"), + codexDir: path.join(root, "home", ".codex", "skills"), + }; + + for (const name of bundledSkills) { + await mkdir(path.join(targets.sourceDir, name), { recursive: true }); + await writeFile(path.join(targets.sourceDir, name, "SKILL.md"), `# ${name}\n`, "utf8"); + } + + const userDataPath = path.join(root, "user-data"); + if (options.installed) { + await createSkillsController({ + resolveTargets: () => targets, + selectionStore: createSkillSelectionStore({ userDataPath }), + }).save(options.installed); + } + + if (options.keepOnlyIn) { + const kept = targetDir(targets, options.keepOnlyIn.target); + for (const dir of [targets.agentsDir, targets.claudeDir, targets.codexDir]) { + if (dir === kept) continue; + await rm(path.join(dir, options.keepOnlyIn.skill), { recursive: true, force: true }); + } + } + + if (options.placeOnDisk) { + const dir = path.join( + targetDir(targets, options.placeOnDisk.target), + options.placeOnDisk.skill, + ); + for (const [rel, contents] of Object.entries(options.placeOnDisk.files)) { + const file = path.join(dir, rel); + await mkdir(path.dirname(file), { recursive: true }); + await writeFile(file, contents, "utf8"); + } + } + + if (options.blockAgentsDir) { + await mkdir(path.dirname(targets.agentsDir), { recursive: true }); + await writeFile(targets.agentsDir, "not a directory", "utf8"); + } + + return { + targets, + userDataPath, + bundledSkills, + placeSkillOnDisk: async (skill, target) => { + const dir = path.join(targetDir(targets, target), skill); + await mkdir(dir, { recursive: true }); + await writeFile(path.join(dir, "SKILL.md"), `# ${skill}\n`, "utf8"); + }, + commitSelectionExternally: async (selection, confirmedRemovals = []) => { + await createSkillsController({ + resolveTargets: () => targets, + selectionStore: createSkillSelectionStore({ userDataPath }), + }).save({ ...selection, confirmedRemovals: [...confirmedRemovals] }); + }, + waitForHeldSave: () => heldSave, + releaseHeldSave: () => releaseHeldSave?.(), + holdSaveResponse: options.holdSave + ? async () => { + markSaveHeld?.(); + await saveRelease; + } + : undefined, + // The save path atomically renames skill directories. On Linux/overlayfs, + // recursive removal can briefly observe a repopulated directory and fail + // with ENOTEMPTY even after the command has completed. Node only retries + // that transient when maxRetries is set. + cleanup: () => rm(root, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }), + }; +} + +/** Reads the committed selection back through the same store the app writes. */ +export async function expectSavedSkillSelection( + sandbox: SkillsSandbox, + expected: SkillSelection, +): Promise { + const store = createSkillSelectionStore({ userDataPath: sandbox.userDataPath }); + await expect.poll(() => store.get(), { timeout: 15_000 }).toEqual(expected); +} + +/** + * Routes the skills desktop commands to the real handlers in Node, leaving every + * other command on the injected desktop bridge. Call after `injectDesktopBridge` + * and before navigation. + */ +export async function serveRealSkillsCommands(page: Page, sandbox: SkillsSandbox): Promise { + const handlers = createSkillsCommandHandlers({ + controller: createSkillsController({ + resolveTargets: () => sandbox.targets, + selectionStore: createSkillSelectionStore({ userDataPath: sandbox.userDataPath }), + }), + }); + + await page.exposeFunction( + "__paseoSkillsInvoke", + async (command: string, args: Record | null) => { + const handler = handlers[command]; + if (!handler) throw new Error(`Unknown skills command: ${command}`); + const result = await handler(args ?? undefined); + if (command === "save_skills_selection") { + await sandbox.holdSaveResponse?.(); + } + return result; + }, + ); + + await page.addInitScript((commands: readonly string[]) => { + const bridge = (window as unknown as { paseoDesktop?: { invoke: unknown } }).paseoDesktop; + if (!bridge) throw new Error("Desktop bridge must be injected before the skills bridge."); + const skillCommands = new Set(commands); + const inner = bridge.invoke as ( + command: string, + args?: Record, + ) => Promise; + bridge.invoke = (command: string, args?: Record) => + skillCommands.has(command) + ? window.__paseoSkillsInvoke!(command, args ?? null) + : inner(command, args); + }, SKILL_COMMANDS); +} + +/** Real skills commands over a temp bundle, wired to this page's desktop bridge. */ +export async function startSkillsSandbox( + page: Page, + options: SkillsSandboxOptions = {}, +): Promise { + const sandbox = await createSkillsSandbox(options); + await injectDesktopBridge(page, { serverId: getServerId(), confirmShouldAccept: false }); + await serveRealSkillsCommands(page, sandbox); + return sandbox; +} + +export async function openSkillsIntegrations(page: Page): Promise { + await openSettings(page); + await openSettingsSection(page, "integrations"); + await expect(page.getByText("Orchestration skills", { exact: true })).toBeVisible(); +} + +/** Settings on a compact form factor stacks list and detail, so it needs the menu first. */ +export async function openCompactSkillsIntegrations(page: Page): Promise { + await openCompactSettings(page, buildOpenProjectRoute()); + await openSettingsSection(page, "integrations"); + await expect(page.getByText("Orchestration skills", { exact: true })).toBeVisible(); +} + +export async function openSkillSelection(page: Page): Promise { + await page.getByRole("button", { name: "Choose skills", exact: true }).click(); + await expectSkillSelectionOpen(page); +} + +export async function expectSkillSelectionOpen(page: Page): Promise { + await expect(page.getByRole("switch", { name: "All skills", exact: true })).toBeVisible(); +} + +export async function expectSkillSelectionClosed(page: Page): Promise { + await expect(page.getByRole("switch", { name: "All skills", exact: true })).toHaveCount(0); +} + +export async function chooseAllSkills(page: Page): Promise { + const allSkills = page.getByRole("switch", { name: "All skills", exact: true }); + await expect(allSkills).not.toBeChecked(); + await allSkills.click(); + await expect(allSkills).toBeChecked(); +} + +export async function chooseCustomSkills(page: Page, skills: string[]): Promise { + const allSkills = page.getByRole("switch", { name: "All skills", exact: true }); + await expect(allSkills).toBeChecked(); + await allSkills.click(); + await expect(allSkills).not.toBeChecked(); + + const keep = new Set(skills); + for (const name of await listSkillChoices(page)) { + if (keep.has(name)) continue; + await page.getByRole("checkbox", { name, exact: true }).click(); + } + await expectSkillChoices(page, skills); +} + +export async function toggleSkill(page: Page, name: string): Promise { + await page.getByRole("checkbox", { name, exact: true }).click(); +} + +export async function saveSkillSelection(page: Page): Promise { + await page.getByRole("button", { name: "Save", exact: true }).click(); +} + +export async function cancelSkillSelection(page: Page): Promise { + await page.getByRole("button", { name: "Cancel", exact: true }).click(); + await expectSkillSelectionClosed(page); +} + +export async function listSkillChoices(page: Page): Promise { + const rows = page.getByRole("checkbox"); + const names = await rows.evaluateAll((elements) => + elements.map((element) => element.getAttribute("aria-label") ?? ""), + ); + return names.filter((name) => name.length > 0); +} + +/** Asserts exactly which skills are checked in the open sheet. */ +export async function expectSkillChoices(page: Page, skills: string[]): Promise { + const expected = new Set(skills); + for (const name of await listSkillChoices(page)) { + const choice = page.getByRole("checkbox", { name, exact: true }); + if (expected.has(name)) { + await expect(choice).toBeChecked(); + continue; + } + await expect(choice).not.toBeChecked(); + } +} + +/** Reopens the saved selection and asserts what the user sees, then closes it. */ +export async function expectSelectedSkills(page: Page, skills: string[]): Promise { + await openSkillSelection(page); + await expect(page.getByRole("switch", { name: "All skills", exact: true })).not.toBeChecked(); + await expectSkillChoices(page, skills); + await cancelSkillSelection(page); +} + +export async function expectAllSkillsSelected(page: Page): Promise { + await openSkillSelection(page); + const allSkills = page.getByRole("switch", { name: "All skills", exact: true }); + await expect(allSkills).toBeChecked(); + await expectSkillChoices(page, await listSkillChoices(page)); + await cancelSkillSelection(page); +} + +export async function expectSaveErrorKeepsSheetOpen(page: Page): Promise { + await expect(page.getByText("Could not save your skill selection.", { exact: true })).toBeVisible( + { timeout: 15_000 }, + ); + await expectSkillSelectionOpen(page); +} + +/** + * The removal warning is the desktop confirm dialog, which has no DOM to query. + * The injected bridge records the call the same way the daemon-management tests + * assert their confirmation copy. + */ +export async function expectRemovalWarning(page: Page, skills: string[]): Promise { + await page.waitForFunction(() => !!window.__capturedDialogCall, { timeout: 15_000 }); + const call = await page.evaluate(() => window.__capturedDialogCall); + expect(call?.title).toBe("Remove deselected skills?"); + for (const name of skills) { + expect(call?.message).toContain(name); + } + for (const dir of ["~/.agents", "~/.claude", "~/.codex"]) { + expect(call?.message).toContain(dir); + } + expect(call?.message).toContain("Anything you added inside those skill folders is deleted too."); +} + +export async function expectNoRemovalWarning(page: Page): Promise { + expect(await page.evaluate(() => window.__capturedDialogCall)).toBeUndefined(); +} + +export async function expectSkillPresentIn( + sandbox: SkillsSandbox, + target: SkillTargetName, + skill: string, +): Promise { + await expect + .poll(() => readInstalledSkills(targetDir(sandbox.targets, target)), { timeout: 15_000 }) + .toContain(skill); +} + +export async function expectSkillFilePresent( + sandbox: SkillsSandbox, + target: SkillTargetName, + skill: string, + relativePath: string, +): Promise { + const file = path.join(targetDir(sandbox.targets, target), skill, relativePath); + await expect + .poll(() => readFile(file, "utf8").catch(() => null), { timeout: 15_000 }) + .not.toBeNull(); +} + +export async function expectSkillsInstalled( + sandbox: SkillsSandbox, + skills: string[], +): Promise { + const dirs = [ + sandbox.targets.agentsDir, + sandbox.targets.claudeDir, + sandbox.targets.codexDir, + ] as const; + for (const dir of dirs) { + await expect + .poll(async () => readInstalledSkills(dir), { timeout: 15_000 }) + .toEqual([...skills].sort()); + } +} + +async function readInstalledSkills(dir: string): Promise { + const entries = await readdir(dir, { withFileTypes: true }).catch(() => []); + return entries + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + .sort(); +} diff --git a/packages/app/e2e/skills-selection.spec.ts b/packages/app/e2e/skills-selection.spec.ts new file mode 100644 index 000000000..4eb1185d3 --- /dev/null +++ b/packages/app/e2e/skills-selection.spec.ts @@ -0,0 +1,333 @@ +import { test as base } from "./fixtures"; +import { expect } from "@playwright/test"; +import { gotoAppShell } from "./helpers/app"; +import { injectDesktopBridge } from "./helpers/desktop-updates"; +import { getServerId } from "./helpers/server-id"; +import { + cancelSkillSelection, + chooseAllSkills, + chooseCustomSkills, + createSkillsSandbox, + expectAllSkillsSelected, + expectNoRemovalWarning, + expectRemovalWarning, + expectSavedSkillSelection, + expectSaveErrorKeepsSheetOpen, + expectSelectedSkills, + expectSkillChoices, + expectSkillFilePresent, + expectSkillPresentIn, + expectSkillSelectionOpen, + expectSkillsInstalled, + openSkillsIntegrations, + openSkillSelection, + saveSkillSelection, + toggleSkill, + serveRealSkillsCommands, + type SkillsSandbox, + type SkillsSandboxOptions, +} from "./helpers/skills-selection"; + +interface SkillsSandboxSetup extends SkillsSandboxOptions { + /** What the desktop confirm dialog answers when the removal warning fires. */ + confirmRemoval?: boolean; +} + +type StartSkills = (setup?: SkillsSandboxSetup) => Promise; + +// Every skills command runs against the real desktop handlers over a temp bundle +// and temp user data, so the assertions cover persistence and convergence, not a +// browser stand-in for them. +const test = base.extend<{ startSkills: StartSkills }>({ + startSkills: async ({ page }, provide) => { + const sandboxes: SkillsSandbox[] = []; + await provide(async (setup = {}) => { + const { confirmRemoval, ...options } = setup; + const sandbox = await createSkillsSandbox(options); + await injectDesktopBridge(page, { + serverId: getServerId(), + confirmShouldAccept: confirmRemoval ?? false, + }); + await serveRealSkillsCommands(page, sandbox); + sandboxes.push(sandbox); + return sandbox; + }); + for (const sandbox of sandboxes) { + await sandbox.cleanup(); + } + }, +}); + +test.describe("Choosing installed skills", () => { + test("installs only the skills selected in Settings", async ({ page, startSkills }) => { + const skills = await startSkills(); + await gotoAppShell(page); + await openSkillsIntegrations(page); + + await openSkillSelection(page); + await chooseCustomSkills(page, ["paseo", "paseo-loop"]); + await saveSkillSelection(page); + + await expectSkillsInstalled(skills, ["paseo", "paseo-loop"]); + await expectSelectedSkills(page, ["paseo", "paseo-loop"]); + }); + + test("all skills includes every available skill", async ({ page, startSkills }) => { + const skills = await startSkills(); + await gotoAppShell(page); + await openSkillsIntegrations(page); + await openSkillSelection(page); + await chooseCustomSkills(page, ["paseo"]); + await saveSkillSelection(page); + + await openSkillSelection(page); + await chooseAllSkills(page); + await saveSkillSelection(page); + + await expectSkillsInstalled(skills, skills.bundledSkills); + await expectAllSkillsSelected(page); + }); + + test("cancelling leaves the installed skills untouched", async ({ page, startSkills }) => { + const skills = await startSkills(); + await gotoAppShell(page); + await openSkillsIntegrations(page); + await openSkillSelection(page); + await chooseCustomSkills(page, ["paseo"]); + await saveSkillSelection(page); + + await openSkillSelection(page); + await toggleSkill(page, "paseo-loop"); + await cancelSkillSelection(page); + + await expectSkillsInstalled(skills, ["paseo"]); + await expectSelectedSkills(page, ["paseo"]); + }); + + test("a failed save keeps the sheet open with an error", async ({ page, startSkills }) => { + const skills = await startSkills({ blockAgentsDir: true }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + + await openSkillSelection(page); + await chooseCustomSkills(page, ["paseo"]); + await saveSkillSelection(page); + + await expectSaveErrorKeepsSheetOpen(page); + await expectSkillsInstalled(skills, []); + }); + + test("an empty selection does not offer an Install action that cannot install anything", async ({ + page, + startSkills, + }) => { + await startSkills({ installed: { mode: "all" }, confirmRemoval: true }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + + await openSkillSelection(page); + await chooseCustomSkills(page, []); + await saveSkillSelection(page); + + // The remaining Install action belongs to the command-line integration. + await expect(page.getByRole("button", { name: "Install", exact: true })).toHaveCount(1); + await expect(page.getByRole("button", { name: "Choose skills", exact: true })).toBeVisible(); + }); + + test("a selection containing only retired skills does not offer a no-op Install", async ({ + page, + startSkills, + }) => { + await startSkills({ installed: { mode: "custom", skills: ["retired-skill"] } }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + + // The remaining Install action belongs to the command-line integration. + await expect(page.getByRole("button", { name: "Install", exact: true })).toHaveCount(1); + await expect(page.getByRole("button", { name: "Choose skills", exact: true })).toBeVisible(); + }); + + test("an empty bundled catalog does not offer a no-op Install", async ({ page, startSkills }) => { + await startSkills({ bundledSkills: [] }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + + // The remaining Install action belongs to the command-line integration. + await expect(page.getByRole("button", { name: "Install", exact: true })).toHaveCount(1); + await expect(page.getByRole("button", { name: "Choose skills", exact: true })).toBeVisible(); + }); + + test("cannot dismiss the sheet while Save is pending", async ({ page, startSkills }) => { + const skills = await startSkills({ holdSave: true }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + await openSkillSelection(page); + await chooseCustomSkills(page, ["paseo"]); + + await saveSkillSelection(page); + await skills.waitForHeldSave(); + await page.keyboard.press("Escape"); + await expectSkillSelectionOpen(page); + + skills.releaseHeldSave(); + await expect(page.getByRole("switch", { name: "All skills", exact: true })).toHaveCount(0); + }); +}); + +test.describe("Removing an installed skill", () => { + test("delete-only drift stays in Choose skills instead of Update", async ({ + page, + startSkills, + }) => { + await startSkills({ + installed: { mode: "custom", skills: ["paseo"] }, + placeOnDisk: { + skill: "paseo-loop", + target: "claude", + files: { "SKILL.md": "# paseo-loop\n", "notes/mine.md": "my notes" }, + }, + }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + + await expect(page.getByText("Update available", { exact: true })).toHaveCount(0); + await expect(page.getByRole("button", { name: "Update", exact: true })).toHaveCount(0); + await expect(page.getByRole("button", { name: "Choose skills", exact: true })).toBeVisible(); + }); + + test("warns before deleting the skill folders", async ({ page, startSkills }) => { + const skills = await startSkills({ installed: { mode: "all" }, confirmRemoval: false }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + + await openSkillSelection(page); + await chooseCustomSkills(page, ["paseo", "paseo-advisor"]); + await saveSkillSelection(page); + + await expectRemovalWarning(page, ["paseo-loop"]); + await expectSkillSelectionOpen(page); + await expectSkillsInstalled(skills, skills.bundledSkills); + await expectSavedSkillSelection(skills, { mode: "all" }); + }); + + test("confirming the warning removes the skill", async ({ page, startSkills }) => { + const skills = await startSkills({ installed: { mode: "all" }, confirmRemoval: true }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + + await openSkillSelection(page); + await chooseCustomSkills(page, ["paseo", "paseo-advisor"]); + await saveSkillSelection(page); + + await expectRemovalWarning(page, ["paseo-loop"]); + await expectSkillsInstalled(skills, ["paseo", "paseo-advisor"]); + await expectSelectedSkills(page, ["paseo", "paseo-advisor"]); + }); + + test("warns for a skill installed in only one agent directory", async ({ page, startSkills }) => { + const skills = await startSkills({ + installed: { mode: "all" }, + keepOnlyIn: { skill: "paseo-loop", target: "claude" }, + confirmRemoval: false, + }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + + await openSkillSelection(page); + await chooseCustomSkills(page, ["paseo", "paseo-advisor"]); + await saveSkillSelection(page); + + await expectRemovalWarning(page, ["paseo-loop"]); + await expectSkillSelectionOpen(page); + await expectSkillPresentIn(skills, "claude", "paseo-loop"); + await expectSavedSkillSelection(skills, { mode: "all" }); + }); + + test("warns for a skill on disk that the saved selection never mentioned", async ({ + page, + startSkills, + }) => { + // The committed preference excludes paseo-loop, but the directory is back — + // a manual reinstall or an external sync. Saving any edit deletes it. + const skills = await startSkills({ + installed: { mode: "custom", skills: ["paseo"] }, + placeOnDisk: { + skill: "paseo-loop", + target: "claude", + files: { "SKILL.md": "# paseo-loop\n", "notes/mine.md": "my notes" }, + }, + confirmRemoval: false, + }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + + await openSkillSelection(page); + await toggleSkill(page, "paseo-advisor"); + await saveSkillSelection(page); + + await expectRemovalWarning(page, ["paseo-loop"]); + await expectSkillSelectionOpen(page); + await expectSkillFilePresent(skills, "claude", "paseo-loop", "notes/mine.md"); + await expectSavedSkillSelection(skills, { mode: "custom", skills: ["paseo"] }); + }); + + test("warns about a directory that appears after the sheet was opened", async ({ + page, + startSkills, + }) => { + const skills = await startSkills({ + installed: { mode: "custom", skills: ["paseo"] }, + confirmRemoval: false, + }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + await openSkillSelection(page); + + // Nothing to delete when the sheet opened; the host looks again at save time. + await skills.placeSkillOnDisk("paseo-loop", "claude"); + await toggleSkill(page, "paseo-advisor"); + await saveSkillSelection(page); + + await expectRemovalWarning(page, ["paseo-loop"]); + await expectSkillSelectionOpen(page); + await expectSkillPresentIn(skills, "claude", "paseo-loop"); + await expectSavedSkillSelection(skills, { mode: "custom", skills: ["paseo"] }); + }); + + test("keeps draft choices when fresher status arrives while the sheet is open", async ({ + page, + startSkills, + }) => { + const skills = await startSkills({ installed: { mode: "all" }, confirmRemoval: false }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + await openSkillSelection(page); + await chooseCustomSkills(page, ["paseo", "paseo-advisor"]); + + // Another window commits a different selection. Saving picks that up, so the + // sheet is handed a selection it did not start from while an edit is open. + await skills.commitSelectionExternally({ mode: "custom", skills: ["paseo", "paseo-loop"] }, [ + "paseo-advisor", + ]); + await saveSkillSelection(page); + await expectRemovalWarning(page, ["paseo-loop"]); + + await expectSkillChoices(page, ["paseo", "paseo-advisor"]); + }); + + test("adding a skill saves without a warning", async ({ page, startSkills }) => { + const skills = await startSkills({ + installed: { mode: "custom", skills: ["paseo"] }, + confirmRemoval: false, + }); + await gotoAppShell(page); + await openSkillsIntegrations(page); + + await openSkillSelection(page); + await toggleSkill(page, "paseo-loop"); + await saveSkillSelection(page); + + await expectSkillsInstalled(skills, ["paseo", "paseo-loop"]); + await expectNoRemovalWarning(page); + }); +}); diff --git a/packages/app/src/components/adaptive-modal-sheet.tsx b/packages/app/src/components/adaptive-modal-sheet.tsx index 8d06a2181..90f871833 100644 --- a/packages/app/src/components/adaptive-modal-sheet.tsx +++ b/packages/app/src/components/adaptive-modal-sheet.tsx @@ -165,19 +165,17 @@ const styles = StyleSheet.create((theme) => ({ flexShrink: 1, minHeight: 0, }, - desktopContent: { + // The sheet's content inset — one definition for every presentation, always + // applied through . + sheetContent: { padding: theme.spacing[SHEET_HORIZONTAL_PADDING_SCALE], gap: theme.spacing[4], + }, + contentGrow: { flexGrow: 1, }, - bottomSheetContent: { - padding: theme.spacing[SHEET_HORIZONTAL_PADDING_SCALE], - gap: theme.spacing[4], - }, - bottomSheetStaticContent: { + compactStaticContent: { flex: 1, - padding: theme.spacing[SHEET_HORIZONTAL_PADDING_SCALE], - gap: theme.spacing[4], minHeight: 0, }, bottomSheetVisibleContent: { @@ -191,8 +189,6 @@ const styles = StyleSheet.create((theme) => ({ desktopStaticContent: { flexShrink: 1, minHeight: 0, - padding: theme.spacing[SHEET_HORIZONTAL_PADDING_SCALE], - gap: theme.spacing[4], }, footer: { paddingHorizontal: theme.spacing[SHEET_HORIZONTAL_PADDING_SCALE], @@ -233,6 +229,20 @@ function SheetBackground({ style }: BottomSheetBackgroundProps) { return ; } +/** + * The sheet body, indented to the sheet's content inset. + * + * The inset lives on a real `View` and never on a scroller's + * `contentContainerStyle`: that is a library prop, not the `style` prop + * Unistyles registers, so a themed inset handed to a third-party scroller such + * as `BottomSheetScrollView` silently resolves to nothing on web — which is how + * the compact sheet ended up rendering its cards flush to the screen edges. See + * docs/unistyles.md "Main Gotcha: contentContainerStyle". + */ +function SheetContent({ style, children }: { style: StyleProp; children: ReactNode }) { + return {children}; +} + function BottomSheetVisibleContent({ children }: { children: ReactNode }) { const { animatedDetentsState, animatedKeyboardState, animatedLayoutState, animatedPosition } = useBottomSheetInternal(); @@ -372,6 +382,7 @@ export function SheetHeaderView({ {header.actions ? {header.actions} : null} {showCloseButton ? ( ; + /** Layout intent for the sheet body, composed over the sheet's own content inset. */ + contentStyle?: StyleProp; /** Size compact sheet content to the live snap height instead of its largest snap point. */ sizeContentToCurrentSnapPoint?: boolean; } @@ -495,7 +507,7 @@ export function AdaptiveModalSheet({ desktopMaxWidth, scrollable = true, presentation, - contentContainerStyle, + contentStyle, sizeContentToCurrentSnapPoint = false, }: AdaptiveModalSheetProps) { const { theme } = useUnistyles(); @@ -514,28 +526,26 @@ export function AdaptiveModalSheet({ }), [footer, insets.bottom, isMobile, theme.spacing], ); - const bottomSheetContentStyle = useMemo( - // Gorhom spreads this outer array into StyleSheet.compose, which accepts two arguments on web. + const compactContentStyle = useMemo( () => [ - styles.bottomSheetContent, - [ - contentContainerStyle, - compactSafeAreaPadding.contentPaddingBottom != null - ? { paddingBottom: compactSafeAreaPadding.contentPaddingBottom } - : null, - ], - ], - [compactSafeAreaPadding.contentPaddingBottom, contentContainerStyle], - ); - const bottomSheetStaticContentStyle = useMemo( - () => [ - styles.bottomSheetStaticContent, - contentContainerStyle, + contentStyle, compactSafeAreaPadding.contentPaddingBottom != null ? { paddingBottom: compactSafeAreaPadding.contentPaddingBottom } : null, ], - [compactSafeAreaPadding.contentPaddingBottom, contentContainerStyle], + [compactSafeAreaPadding.contentPaddingBottom, contentStyle], + ); + const compactStaticContentStyle = useMemo( + () => [styles.compactStaticContent, compactContentStyle], + [compactContentStyle], + ); + const desktopScrollContentStyle = useMemo( + () => [styles.contentGrow, contentStyle], + [contentStyle], + ); + const desktopStaticContentStyle = useMemo( + () => [styles.desktopStaticContent, contentStyle], + [contentStyle], ); const footerStyle = useMemo( () => [ @@ -649,14 +659,13 @@ export function AdaptiveModalSheet({ {scrollable ? ( - {children} + {children} ) : ( - {children} + {children} )} {footer ? {footer} : null} @@ -695,15 +704,15 @@ export function AdaptiveModalSheet({ - {children} + {children} ) : ( - {children} + {children} )} {footer ? {footer} : null} diff --git a/packages/app/src/composer/agent-controls/model-sheet.tsx b/packages/app/src/composer/agent-controls/model-sheet.tsx index b3ba7f1ee..a3ea5e022 100644 --- a/packages/app/src/composer/agent-controls/model-sheet.tsx +++ b/packages/app/src/composer/agent-controls/model-sheet.tsx @@ -155,7 +155,7 @@ export function CompactModelSheet({ sizeContentToCurrentSnapPoint={usesBottomSheet} footer={compactFooter} footerContainerStyle={usesBottomSheet ? styles.compactFooterContainer : undefined} - contentContainerStyle={styles.sheetBody} + contentStyle={styles.sheetBody} testID="agent-controls-model-sheet" > skillsStatus?.ops.filter((op) => op.kind !== "delete") ?? [], + [skillsStatus?.ops], + ); useFocusEffect( useCallback(() => { @@ -74,18 +81,17 @@ export function IntegrationsSection() { const handleUpdateSkills = useCallback(async () => { if (isSkillsWorking) return; - const ops = skillsStatus?.ops ?? []; const confirmed = await confirmDialog({ title: t("settings.integrations.skills.updateTitle"), message: - ops.length > 0 - ? formatUpdateMessage(ops, t) + skillMaintenanceOps.length > 0 + ? formatUpdateMessage(skillMaintenanceOps, t) : t("settings.integrations.skills.updateFallback"), confirmLabel: t("settings.integrations.actions.update"), }); if (!confirmed) return; await updateSkills(); - }, [isSkillsWorking, skillsStatus, t, updateSkills]); + }, [isSkillsWorking, skillMaintenanceOps, t, updateSkills]); const handleUninstallSkills = useCallback(async () => { if (isSkillsWorking) return; @@ -99,6 +105,14 @@ export function IntegrationsSection() { await uninstallSkills(); }, [isSkillsWorking, t, uninstallSkills]); + const handleOpenSkillSelection = useCallback(() => { + setIsChoosingSkills(true); + }, []); + + const handleCloseSkillSelection = useCallback(() => { + setIsChoosingSkills(false); + }, []); + const handleOpenCliDocs = useCallback(() => { void openExternalUrl(CLI_DOCS_URL); }, []); @@ -112,6 +126,11 @@ export function IntegrationsSection() { [theme.iconSize.sm, theme.colors.foregroundMuted], ); + const chooseSkillsIcon = useMemo( + () => , + [theme.iconSize.sm, theme.colors.foregroundMuted], + ); + const trailing = useMemo( () => ( @@ -146,7 +165,14 @@ export function IntegrationsSection() { return null; } - const skillsState = skillsStatus?.state ?? null; + const skillsState = + skillsStatus?.state === "drift" && skillMaintenanceOps.length === 0 + ? "up-to-date" + : (skillsStatus?.state ?? null); + const hasSelectedSkills = + (skillsStatus?.selection.mode === "all" && skillsStatus.available.length > 0) || + (skillsStatus?.selection.mode === "custom" && + skillsStatus.selection.skills.some((name) => skillsStatus.available.includes(name))); return ( @@ -193,21 +219,43 @@ export function IntegrationsSection() { : t("settings.integrations.skills.description")} - + + + + + ), + [handleDismiss, handleSave, isSaving, t], + ); + + const allSkills = draft.mode === "all"; + + return ( + + + + + + {t("settings.integrations.skills.chooseAll")} + + + {t("settings.integrations.skills.chooseAllHint")} + + + + + + + {/* The sheet body already spaces its blocks apart, so the section owns + only the gap between its label and the list the label names. */} + + + {available.length === 0 ? ( + + + {t("settings.integrations.skills.chooseEmpty")} + + + ) : ( + available.map((name, index) => ( + 0} + onToggle={handleToggleSkill} + /> + )) + )} + + + + {saveError ? {saveError} : null} + + ); +} + +interface SkillCheckboxRowProps { + name: string; + checked: boolean; + disabled: boolean; + withBorder: boolean; + onToggle: (name: string) => void; +} + +function SkillCheckboxRow({ + name, + checked, + disabled, + withBorder, + onToggle, +}: SkillCheckboxRowProps) { + const handlePress = useCallback(() => onToggle(name), [name, onToggle]); + const accessibilityState = useMemo(() => ({ checked, disabled }), [checked, disabled]); + const rowStyle = useMemo( + () => [ + settingsStyles.row, + withBorder ? settingsStyles.rowBorder : null, + styles.checkboxRow, + disabled ? styles.disabledRow : null, + ], + [disabled, withBorder], + ); + const boxStyle = useMemo( + () => [styles.checkbox, checked ? styles.checkboxChecked : null], + [checked], + ); + + return ( + + + {checked ? : null} + + {name} + + ); +} + +const styles = StyleSheet.create((theme) => ({ + footer: { + flex: 1, + flexDirection: "row", + gap: theme.spacing[3], + }, + footerButton: { + flex: 1, + }, + checkboxRow: { + justifyContent: "flex-start", + gap: theme.spacing[3], + }, + disabledRow: { + opacity: theme.opacity[50], + }, + checkbox: { + width: 22, + height: 22, + borderRadius: theme.borderRadius.sm, + borderWidth: 1, + borderColor: theme.colors.border, + alignItems: "center", + justifyContent: "center", + }, + checkboxChecked: { + backgroundColor: theme.colors.accent, + borderColor: theme.colors.accent, + }, +})); diff --git a/packages/app/src/desktop/daemon/desktop-daemon.ts b/packages/app/src/desktop/daemon/desktop-daemon.ts index e2729f2cb..a4292128d 100644 --- a/packages/app/src/desktop/daemon/desktop-daemon.ts +++ b/packages/app/src/desktop/daemon/desktop-daemon.ts @@ -1,5 +1,12 @@ import { getDesktopHost, isElectronRuntime } from "@/desktop/host"; import { invokeDesktopCommand } from "@/desktop/electron/invoke"; +import { + parseSkillsSaveResult, + parseSkillsSnapshot, + type SkillSelection, + type SkillsSaveResult, + type SkillsSnapshot, +} from "@/desktop/daemon/skills-snapshot"; export type DesktopDaemonState = "starting" | "running" | "stopped" | "errored"; export type DesktopDaemonStopReason = @@ -246,67 +253,38 @@ export async function installCli(): Promise { return parseInstallStatus(await invokeDesktopCommand("install_cli")); } -export type SkillsState = "not-installed" | "up-to-date" | "drift"; +export type { + SkillOp, + SkillSelection, + SkillsSaveResult, + SkillsSnapshot, + SkillsState, +} from "@/desktop/daemon/skills-snapshot"; -export type SkillOp = - | { kind: "add"; name: string } - | { kind: "update"; name: string } - | { kind: "delete"; name: string }; - -export interface SkillsStatus { - state: SkillsState; - ops: SkillOp[]; +export async function getSkillsSnapshot(): Promise { + return parseSkillsSnapshot(await invokeDesktopCommand("get_skills_status")); } -function parseSkillsState(value: unknown): SkillsState { - switch (value) { - case "not-installed": - case "up-to-date": - case "drift": - return value; - default: - throw new Error(`Unexpected skills status state: ${String(value)}`); - } +export async function installSkills(): Promise { + return parseSkillsSnapshot(await invokeDesktopCommand("install_skills")); } -function parseSkillOp(raw: unknown): SkillOp { - if (!isRecord(raw)) { - throw new Error("Unexpected skill op response."); - } - const name = toStringOrNull(raw.name); - if (!name) throw new Error("Skill op missing name."); - switch (raw.kind) { - case "add": - return { kind: "add", name }; - case "update": - return { kind: "update", name }; - case "delete": - return { kind: "delete", name }; - default: - throw new Error(`Unexpected skill op kind: ${String(raw.kind)}`); - } +export async function updateSkills(): Promise { + return parseSkillsSnapshot(await invokeDesktopCommand("update_skills")); } -function parseSkillsStatus(raw: unknown): SkillsStatus { - if (!isRecord(raw)) { - throw new Error("Unexpected skills status response."); - } - const ops = Array.isArray(raw.ops) ? raw.ops.map(parseSkillOp) : []; - return { state: parseSkillsState(raw.state), ops }; +export async function uninstallSkills(): Promise { + return parseSkillsSnapshot(await invokeDesktopCommand("uninstall_skills")); } -export async function getSkillsStatus(): Promise { - return parseSkillsStatus(await invokeDesktopCommand("get_skills_status")); -} - -export async function installSkills(): Promise { - return parseSkillsStatus(await invokeDesktopCommand("install_skills")); -} - -export async function updateSkills(): Promise { - return parseSkillsStatus(await invokeDesktopCommand("update_skills")); -} - -export async function uninstallSkills(): Promise { - return parseSkillsStatus(await invokeDesktopCommand("uninstall_skills")); +export async function saveSkillsSelection( + selection: SkillSelection, + confirmedRemovals: readonly string[] = [], +): Promise { + return parseSkillsSaveResult( + await invokeDesktopCommand("save_skills_selection", { + ...selection, + confirmedRemovals: [...confirmedRemovals], + }), + ); } diff --git a/packages/app/src/desktop/daemon/skills-snapshot.test.ts b/packages/app/src/desktop/daemon/skills-snapshot.test.ts new file mode 100644 index 000000000..3a860deb7 --- /dev/null +++ b/packages/app/src/desktop/daemon/skills-snapshot.test.ts @@ -0,0 +1,131 @@ +import { describe, expect, it } from "vitest"; +import { parseSkillsSaveResult, parseSkillsSnapshot } from "./skills-snapshot"; + +describe("parseSkillsSnapshot", () => { + it("parses a full snapshot from the desktop command surface", () => { + expect( + parseSkillsSnapshot({ + state: "drift", + ops: [ + { kind: "add", name: "paseo-loop" }, + { kind: "delete", name: "paseo-chat" }, + ], + available: ["paseo", "paseo-loop"], + installed: ["paseo"], + selection: { mode: "custom", skills: ["paseo", "paseo-loop"] }, + }), + ).toEqual({ + state: "drift", + ops: [ + { kind: "add", name: "paseo-loop" }, + { kind: "delete", name: "paseo-chat" }, + ], + available: ["paseo", "paseo-loop"], + installed: ["paseo"], + selection: { mode: "custom", skills: ["paseo", "paseo-loop"] }, + }); + }); + + it("parses the installed skills the host reports", () => { + expect( + parseSkillsSnapshot({ + state: "drift", + ops: [{ kind: "add", name: "paseo-loop" }], + available: ["paseo", "paseo-loop"], + installed: ["paseo-loop", 7], + selection: { mode: "all" }, + }).installed, + ).toEqual(["paseo-loop"]); + }); + + it("assumes the saved selection is installed when the host does not report it", () => { + // An older host has no `installed` field. Assuming the selection is on disk + // keeps the destructive confirmation firing rather than silently skipping it. + expect( + parseSkillsSnapshot({ + state: "up-to-date", + ops: [], + available: ["paseo", "paseo-advisor", "paseo-loop"], + selection: { mode: "custom", skills: ["paseo", "paseo-loop"] }, + }).installed, + ).toEqual(["paseo", "paseo-loop"]); + }); + + it("assumes every bundled skill is installed for an all selection from an older host", () => { + expect( + parseSkillsSnapshot({ + state: "up-to-date", + ops: [], + available: ["paseo", "paseo-loop"], + }).installed, + ).toEqual(["paseo", "paseo-loop"]); + }); + + it("reads a snapshot with no saved selection as all skills", () => { + expect(parseSkillsSnapshot({ state: "up-to-date", ops: [], available: ["paseo"] })).toEqual({ + state: "up-to-date", + ops: [], + available: ["paseo"], + installed: ["paseo"], + selection: { mode: "all" }, + }); + }); + + it("drops catalog and selection entries that are not skill names", () => { + expect( + parseSkillsSnapshot({ + state: "up-to-date", + ops: [], + available: ["paseo", 7, null, "paseo-loop"], + selection: { mode: "custom", skills: ["paseo", 7] }, + }), + ).toEqual({ + state: "up-to-date", + ops: [], + available: ["paseo", "paseo-loop"], + installed: ["paseo"], + selection: { mode: "custom", skills: ["paseo"] }, + }); + }); + + it("reads the removals the host wants confirmed", () => { + expect( + parseSkillsSaveResult({ + state: "up-to-date", + ops: [], + available: ["paseo", "paseo-loop"], + installed: ["paseo", "paseo-loop"], + selection: { mode: "all" }, + confirmationRequired: { removals: ["paseo-loop", 7] }, + }).confirmationRequired, + ).toEqual({ removals: ["paseo-loop"] }); + }); + + it("treats a save with no confirmation request as applied", () => { + expect( + parseSkillsSaveResult({ + state: "up-to-date", + ops: [], + available: ["paseo"], + installed: ["paseo"], + selection: { mode: "all" }, + }).confirmationRequired, + ).toBeNull(); + }); + + it("rejects a response that is not an object", () => { + expect(() => parseSkillsSnapshot("nope")).toThrow("Unexpected skills status response."); + }); + + it("rejects an unknown install state", () => { + expect(() => parseSkillsSnapshot({ state: "half-installed", ops: [] })).toThrow( + "Unexpected skills status state: half-installed", + ); + }); + + it("rejects an unknown pending operation kind", () => { + expect(() => + parseSkillsSnapshot({ state: "drift", ops: [{ kind: "relocate", name: "paseo" }] }), + ).toThrow("Unexpected skill op kind: relocate"); + }); +}); diff --git a/packages/app/src/desktop/daemon/skills-snapshot.ts b/packages/app/src/desktop/daemon/skills-snapshot.ts new file mode 100644 index 000000000..5129e1215 --- /dev/null +++ b/packages/app/src/desktop/daemon/skills-snapshot.ts @@ -0,0 +1,113 @@ +export type SkillsState = "not-installed" | "up-to-date" | "drift"; + +export type SkillOp = + | { kind: "add"; name: string } + | { kind: "update"; name: string } + | { kind: "delete"; name: string }; + +/** What the user asked to have installed. `all` follows the bundle as it grows. */ +export type SkillSelection = { mode: "all" } | { mode: "custom"; skills: string[] }; + +export interface SkillsSnapshot { + state: SkillsState; + ops: SkillOp[]; + /** Every skill the host currently bundles, sorted. The selectable catalog. */ + available: string[]; + /** + * Skills with a directory in at least one agent home. An `add` op only means + * "missing from at least one target", so it cannot stand in for this. + */ + installed: string[]; + selection: SkillSelection; +} + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function parseSkillsState(value: unknown): SkillsState { + switch (value) { + case "not-installed": + case "up-to-date": + case "drift": + return value; + default: + throw new Error(`Unexpected skills status state: ${String(value)}`); + } +} + +function parseSkillOp(raw: unknown): SkillOp { + if (!isRecord(raw)) { + throw new Error("Unexpected skill op response."); + } + const name = typeof raw.name === "string" ? raw.name.trim() : ""; + if (!name) throw new Error("Skill op missing name."); + switch (raw.kind) { + case "add": + return { kind: "add", name }; + case "update": + return { kind: "update", name }; + case "delete": + return { kind: "delete", name }; + default: + throw new Error(`Unexpected skill op kind: ${String(raw.kind)}`); + } +} + +function parseSkillNames(value: unknown): string[] { + if (!Array.isArray(value)) return []; + return value.filter((entry): entry is string => typeof entry === "string"); +} + +function parseSkillSelection(value: unknown): SkillSelection { + // A host that predates the picker sends no selection; it manages every + // bundled skill, which is exactly what `all` means. + if (!isRecord(value)) return { mode: "all" }; + if (value.mode === "custom") return { mode: "custom", skills: parseSkillNames(value.skills) }; + return { mode: "all" }; +} + +function selectedSkills(selection: SkillSelection, available: readonly string[]): string[] { + if (selection.mode === "all") return [...available]; + const chosen = new Set(selection.skills); + return available.filter((name) => chosen.has(name)); +} + +export interface SkillsSaveResult extends SkillsSnapshot { + /** + * Non-null when the host refused the save because these directories would be + * deleted. Confirm the names with the user and retry with them. + */ + confirmationRequired: { removals: string[] } | null; +} + +export function parseSkillsSaveResult(raw: unknown): SkillsSaveResult { + const snapshot = parseSkillsSnapshot(raw); + const required = + isRecord(raw) && isRecord(raw.confirmationRequired) + ? parseSkillNames(raw.confirmationRequired.removals) + : []; + return { + ...snapshot, + confirmationRequired: required.length > 0 ? { removals: required } : null, + }; +} + +export function parseSkillsSnapshot(raw: unknown): SkillsSnapshot { + if (!isRecord(raw)) { + throw new Error("Unexpected skills status response."); + } + const available = parseSkillNames(raw.available); + const selection = parseSkillSelection(raw.selection); + return { + state: parseSkillsState(raw.state), + ops: Array.isArray(raw.ops) ? raw.ops.map(parseSkillOp) : [], + available, + // A host that predates this field cannot say what is on disk. Assume the + // saved selection is, so removing a skill still asks before deleting it. + installed: Array.isArray(raw.installed) + ? parseSkillNames(raw.installed) + : selectedSkills(selection, available), + selection, + }; +} diff --git a/packages/app/src/desktop/hooks/use-install-status.test.tsx b/packages/app/src/desktop/hooks/use-install-status.test.tsx index 9610788a9..fda486986 100644 --- a/packages/app/src/desktop/hooks/use-install-status.test.tsx +++ b/packages/app/src/desktop/hooks/use-install-status.test.tsx @@ -17,10 +17,11 @@ const toast = vi.hoisted(() => ({ const desktopDaemon = vi.hoisted(() => ({ getCliInstallStatus: vi.fn(), installCli: vi.fn(), - getSkillsStatus: vi.fn(), + getSkillsSnapshot: vi.fn(), installSkills: vi.fn(), updateSkills: vi.fn(), uninstallSkills: vi.fn(), + saveSkillsSelection: vi.fn(), shouldUseDesktopDaemon: vi.fn(() => true), })); @@ -127,7 +128,7 @@ describe("useSkillsStatus", () => { }); it("loads the current skills status", async () => { - desktopDaemon.getSkillsStatus.mockResolvedValue({ + desktopDaemon.getSkillsSnapshot.mockResolvedValue({ state: "up-to-date", ops: [], }); @@ -142,7 +143,7 @@ describe("useSkillsStatus", () => { }); it("install transitions a not-installed status to up-to-date and reflects the response directly", async () => { - desktopDaemon.getSkillsStatus.mockResolvedValue({ + desktopDaemon.getSkillsSnapshot.mockResolvedValue({ state: "not-installed", ops: [{ kind: "add", name: "paseo" }], }); @@ -165,7 +166,7 @@ describe("useSkillsStatus", () => { }); it("update transitions drift to up-to-date", async () => { - desktopDaemon.getSkillsStatus.mockResolvedValue({ + desktopDaemon.getSkillsSnapshot.mockResolvedValue({ state: "drift", ops: [{ kind: "update", name: "paseo" }], }); @@ -188,7 +189,7 @@ describe("useSkillsStatus", () => { }); it("uninstall transitions up-to-date back to not-installed", async () => { - desktopDaemon.getSkillsStatus.mockResolvedValue({ state: "up-to-date", ops: [] }); + desktopDaemon.getSkillsSnapshot.mockResolvedValue({ state: "up-to-date", ops: [] }); desktopDaemon.uninstallSkills.mockResolvedValue({ state: "not-installed", ops: [{ kind: "add", name: "paseo" }], @@ -214,7 +215,7 @@ describe("useSkillsStatus", () => { }); it("isWorking flips while a mutation is in flight", async () => { - desktopDaemon.getSkillsStatus.mockResolvedValue({ + desktopDaemon.getSkillsSnapshot.mockResolvedValue({ state: "not-installed", ops: [{ kind: "add", name: "paseo" }], }); @@ -256,7 +257,7 @@ describe("useSkillsStatus", () => { it("toasts and exposes errors when install fails", async () => { const error = new Error("Missing IPC handler"); - desktopDaemon.getSkillsStatus.mockResolvedValue({ + desktopDaemon.getSkillsSnapshot.mockResolvedValue({ state: "not-installed", ops: [{ kind: "add", name: "paseo" }], }); diff --git a/packages/app/src/desktop/hooks/use-install-status.ts b/packages/app/src/desktop/hooks/use-install-status.ts index 5c2ddda6e..44595710c 100644 --- a/packages/app/src/desktop/hooks/use-install-status.ts +++ b/packages/app/src/desktop/hooks/use-install-status.ts @@ -3,12 +3,15 @@ import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { useTranslation } from "react-i18next"; import { getCliInstallStatus, - getSkillsStatus, + getSkillsSnapshot, installCli, installSkills, + saveSkillsSelection, shouldUseDesktopDaemon, type InstallStatus, - type SkillsStatus, + type SkillSelection, + type SkillsSaveResult, + type SkillsSnapshot, uninstallSkills, updateSkills, } from "@/desktop/daemon/desktop-daemon"; @@ -79,7 +82,7 @@ export function useCliInstall(): DesktopInstallHookResult { } export interface SkillsStatusHookResult { - status: SkillsStatus | null; + status: SkillsSnapshot | null; isLoading: boolean; isWorking: boolean; error: Error | null; @@ -87,6 +90,10 @@ export interface SkillsStatusHookResult { install: () => Promise; update: () => Promise; uninstall: () => Promise; + saveSelection: ( + selection: SkillSelection, + confirmedRemovals?: readonly string[], + ) => Promise; } export function useSkillsStatus(): SkillsStatusHookResult { @@ -95,9 +102,9 @@ export function useSkillsStatus(): SkillsStatusHookResult { const reportError = useDesktopIpcErrorReporter(); const enabled = shouldUseDesktopDaemon(); - const statusQuery = useQuery({ + const statusQuery = useQuery({ queryKey: SKILLS_STATUS_QUERY_KEY, - queryFn: getSkillsStatus, + queryFn: getSkillsSnapshot, enabled, retry: false, }); @@ -109,13 +116,13 @@ export function useSkillsStatus(): SkillsStatusHookResult { }); const setStatus = useCallback( - (next: SkillsStatus) => { - queryClient.setQueryData(SKILLS_STATUS_QUERY_KEY, next); + (next: SkillsSnapshot) => { + queryClient.setQueryData(SKILLS_STATUS_QUERY_KEY, next); }, [queryClient], ); - const installMutation = useMutation({ + const installMutation = useMutation({ mutationFn: installSkills, onError: (error) => { reportError({ @@ -127,7 +134,7 @@ export function useSkillsStatus(): SkillsStatusHookResult { onSuccess: setStatus, }); - const updateMutation = useMutation({ + const updateMutation = useMutation({ mutationFn: updateSkills, onError: (error) => { reportError({ @@ -139,7 +146,7 @@ export function useSkillsStatus(): SkillsStatusHookResult { onSuccess: setStatus, }); - const uninstallMutation = useMutation({ + const uninstallMutation = useMutation({ mutationFn: uninstallSkills, onError: (error) => { reportError({ @@ -151,8 +158,28 @@ export function useSkillsStatus(): SkillsStatusHookResult { onSuccess: setStatus, }); + const saveSelectionMutation = useMutation< + SkillsSaveResult, + Error, + { selection: SkillSelection; confirmedRemovals: readonly string[] } + >({ + mutationFn: ({ selection, confirmedRemovals }) => + saveSkillsSelection(selection, confirmedRemovals), + onError: (error) => { + reportError({ + error, + message: t("desktop.integrations.skills.saveSelectionFailed"), + logLabel: "[Integrations] Failed to save skills selection", + }); + }, + onSuccess: setStatus, + }); + const isWorking = - installMutation.isPending || updateMutation.isPending || uninstallMutation.isPending; + installMutation.isPending || + updateMutation.isPending || + uninstallMutation.isPending || + saveSelectionMutation.isPending; const refresh = useCallback(async () => { await refetch(); @@ -170,6 +197,12 @@ export function useSkillsStatus(): SkillsStatusHookResult { await uninstallMutation.mutateAsync().catch(() => undefined); }, [uninstallMutation]); + const saveSelection = useCallback( + async (selection: SkillSelection, confirmedRemovals: readonly string[] = []) => + saveSelectionMutation.mutateAsync({ selection, confirmedRemovals }), + [saveSelectionMutation], + ); + return { status: status ?? null, isLoading, @@ -179,10 +212,12 @@ export function useSkillsStatus(): SkillsStatusHookResult { installMutation.error ?? updateMutation.error ?? uninstallMutation.error ?? + saveSelectionMutation.error ?? null, refresh, install, update, uninstall, + saveSelection, }; } diff --git a/packages/app/src/i18n/resources/ar.ts b/packages/app/src/i18n/resources/ar.ts index c60d7ea16..c24cb3ddb 100644 --- a/packages/app/src/i18n/resources/ar.ts +++ b/packages/app/src/i18n/resources/ar.ts @@ -1160,6 +1160,7 @@ export const ar: TranslationResources = { installFailed: "غير قادر على تثبيت مهارات التنسيق.", updateFailed: "غير قادر على تحديث مهارات التنسيق.", uninstallFailed: "غير قادر على إلغاء تثبيت مهارات التنسيق.", + saveSelectionFailed: "غير قادر على حفظ اختيار مهارات التنسيق.", }, }, }, @@ -1863,6 +1864,15 @@ export const ar: TranslationResources = { updateFallback: "مزامنة المهارات المجمعة لجهازك.", uninstallTitle: "إلغاء تثبيت مهارات Paseo ؟", uninstallMessage: "يزيل جميع مهارات تنسيق Paseo من ~/.agents ، ~/.claude ، ~/.codex.", + choose: "اختيار المهارات", + chooseAll: "كل المهارات", + chooseAllHint: "أبقِ جميع المهارات المرفقة مثبتة، بما فيها ما يُضاف لاحقًا.", + chooseList: "المهارات المرفقة", + chooseEmpty: "لا تتضمن هذه النسخة أي مهارات.", + removeTitle: "إزالة المهارات غير المحددة؟", + removeMessage: + "سيتم حذف {{skills}} من ~/.agents و~/.claude و~/.codex. كما يُحذف كل ما أضفته داخل مجلدات المهارات تلك.", + saveFailed: "تعذر حفظ اختيار المهارات.", }, actions: { install: "ثَبَّتَ", @@ -1870,7 +1880,10 @@ export const ar: TranslationResources = { installed: "تم التثبيت", update: "تحديث", working: "عمل...", + remove: "إزالة", uninstall: "إلغاء التثبيت", + save: "حفظ", + saving: "جارٍ الحفظ...", }, operations: { add: "أضف مهارة", diff --git a/packages/app/src/i18n/resources/en.ts b/packages/app/src/i18n/resources/en.ts index b3407ebf3..dd0191c7a 100644 --- a/packages/app/src/i18n/resources/en.ts +++ b/packages/app/src/i18n/resources/en.ts @@ -1170,6 +1170,7 @@ export const en = { installFailed: "Unable to install orchestration skills.", updateFailed: "Unable to update orchestration skills.", uninstallFailed: "Unable to uninstall orchestration skills.", + saveSelectionFailed: "Unable to save the orchestration skills selection.", }, }, }, @@ -1874,6 +1875,15 @@ export const en = { uninstallTitle: "Uninstall Paseo skills?", uninstallMessage: "Removes all Paseo orchestration skills from ~/.agents, ~/.claude, ~/.codex.", + choose: "Choose skills", + chooseAll: "All skills", + chooseAllHint: "Keep every bundled skill installed, including ones added later.", + chooseList: "Bundled skills", + chooseEmpty: "This build bundles no skills.", + removeTitle: "Remove deselected skills?", + removeMessage: + "{{skills}} will be deleted from ~/.agents, ~/.claude, and ~/.codex. Anything you added inside those skill folders is deleted too.", + saveFailed: "Could not save your skill selection.", }, actions: { install: "Install", @@ -1881,7 +1891,10 @@ export const en = { installed: "Installed", update: "Update", working: "Working...", + remove: "Remove", uninstall: "Uninstall", + save: "Save", + saving: "Saving...", }, operations: { add: "Add skill", diff --git a/packages/app/src/i18n/resources/es.ts b/packages/app/src/i18n/resources/es.ts index 7c9f20eab..ebc95b0b7 100644 --- a/packages/app/src/i18n/resources/es.ts +++ b/packages/app/src/i18n/resources/es.ts @@ -1200,6 +1200,7 @@ export const es: TranslationResources = { installFailed: "No se pueden instalar habilidades de orquestación.", updateFailed: "No se pueden actualizar las habilidades de orquestación.", uninstallFailed: "No se pueden desinstalar las habilidades de orquestación.", + saveSelectionFailed: "No se puede guardar la selección de habilidades de orquestación.", }, }, }, @@ -1912,6 +1913,16 @@ export const es: TranslationResources = { uninstallTitle: "¿Desinstalar las habilidadesPaseo?", uninstallMessage: "Elimina todas las habilidades de orquestaciónPaseode ~/.agents, ~/.claude, ~/.codex.", + choose: "Elegir habilidades", + chooseAll: "Todas las habilidades", + chooseAllHint: + "Mantén instaladas todas las habilidades incluidas, también las que se agreguen después.", + chooseList: "Habilidades incluidas", + chooseEmpty: "Esta versión no incluye habilidades.", + removeTitle: "¿Eliminar las habilidades deseleccionadas?", + removeMessage: + "{{skills}} se eliminarán de ~/.agents, ~/.claude y ~/.codex. También se elimina todo lo que hayas añadido dentro de esas carpetas de habilidades.", + saveFailed: "No se pudo guardar tu selección de habilidades.", }, actions: { install: "Instalar", @@ -1919,7 +1930,10 @@ export const es: TranslationResources = { installed: "Instalado", update: "Actualizar", working: "Laboral...", + remove: "Eliminar", uninstall: "Desinstalar", + save: "Guardar", + saving: "Guardando...", }, operations: { add: "Agregar habilidad", diff --git a/packages/app/src/i18n/resources/fr.ts b/packages/app/src/i18n/resources/fr.ts index 6b6b13885..ab59cee82 100644 --- a/packages/app/src/i18n/resources/fr.ts +++ b/packages/app/src/i18n/resources/fr.ts @@ -1202,6 +1202,8 @@ export const fr: TranslationResources = { installFailed: "Impossible d'installer les compétences d'orchestration.", updateFailed: "Impossible de mettre à jour les compétences d'orchestration.", uninstallFailed: "Impossible de désinstaller les compétences d'orchestration.", + saveSelectionFailed: + "Impossible d'enregistrer la sélection des compétences d'orchestration.", }, }, }, @@ -1915,6 +1917,16 @@ export const fr: TranslationResources = { uninstallTitle: "Désinstaller les compétencesPaseo?", uninstallMessage: "Supprime toutes les compétences d'orchestrationPaseode ~/.agents, ~/.claude, ~/.codex.", + choose: "Choisir les compétences", + chooseAll: "Toutes les compétences", + chooseAllHint: + "Gardez installées toutes les compétences fournies, y compris celles ajoutées plus tard.", + chooseList: "Compétences fournies", + chooseEmpty: "Cette version ne fournit aucune compétence.", + removeTitle: "Supprimer les compétences décochées ?", + removeMessage: + "{{skills}} seront supprimées de ~/.agents, ~/.claude et ~/.codex. Tout ce que vous avez ajouté dans ces dossiers de compétences est également supprimé.", + saveFailed: "Impossible d'enregistrer votre sélection de compétences.", }, actions: { install: "Installer", @@ -1922,7 +1934,10 @@ export const fr: TranslationResources = { installed: "Installé", update: "Mise à jour", working: "Fonctionnement...", + remove: "Supprimer", uninstall: "Désinstaller", + save: "Enregistrer", + saving: "Enregistrement...", }, operations: { add: "Ajouter une compétence", diff --git a/packages/app/src/i18n/resources/ja.ts b/packages/app/src/i18n/resources/ja.ts index 21bc2106e..0cd866913 100644 --- a/packages/app/src/i18n/resources/ja.ts +++ b/packages/app/src/i18n/resources/ja.ts @@ -1173,6 +1173,7 @@ export const ja: TranslationResources = { installFailed: "オーケストレーションスキルをインストールできません。", updateFailed: "オーケストレーションスキルを更新できません。", uninstallFailed: "オーケストレーションスキルをアンインストールできません。", + saveSelectionFailed: "オーケストレーションスキルの選択を保存できません。", }, }, }, @@ -1881,6 +1882,16 @@ export const ja: TranslationResources = { uninstallTitle: "Paseoスキルをアンインストールしますか?", uninstallMessage: "~/.agents、~/.claude、~/.codexからすべてのPaseoオーケストレーションスキルを削除します。", + choose: "スキルを選択", + chooseAll: "すべてのスキル", + chooseAllHint: + "後から追加されるものも含め、バンドルされたスキルをすべてインストールしたままにします。", + chooseList: "バンドルされたスキル", + chooseEmpty: "このビルドにはスキルが含まれていません。", + removeTitle: "選択を外したスキルを削除しますか?", + removeMessage: + "{{skills}} を ~/.agents、~/.claude、~/.codex から削除します。それらのスキルフォルダー内に追加したものもすべて削除されます。", + saveFailed: "スキルの選択を保存できませんでした。", }, actions: { install: "インストール", @@ -1888,7 +1899,10 @@ export const ja: TranslationResources = { installed: "インストール済み", update: "更新", working: "処理中...", + remove: "削除", uninstall: "アンインストール", + save: "保存", + saving: "保存中...", }, operations: { add: "スキルを追加", diff --git a/packages/app/src/i18n/resources/pt-BR.ts b/packages/app/src/i18n/resources/pt-BR.ts index 2eeae3863..a826e149c 100644 --- a/packages/app/src/i18n/resources/pt-BR.ts +++ b/packages/app/src/i18n/resources/pt-BR.ts @@ -1186,6 +1186,7 @@ export const ptBR: TranslationResources = { installFailed: "Não foi possível instalar as skills de orquestração.", updateFailed: "Não foi possível atualizar as skills de orquestração.", uninstallFailed: "Não foi possível desinstalar as skills de orquestração.", + saveSelectionFailed: "Não foi possível salvar a seleção de skills de orquestração.", }, }, }, @@ -1896,6 +1897,16 @@ export const ptBR: TranslationResources = { uninstallTitle: "Desinstalar Paseo skills?", uninstallMessage: "Remove todas as skills de orquestração do Paseo de ~/.agents, ~/.claude e ~/.codex.", + choose: "Escolher skills", + chooseAll: "Todas as skills", + chooseAllHint: + "Mantenha instaladas todas as skills incluídas, inclusive as adicionadas depois.", + chooseList: "Skills incluídas", + chooseEmpty: "Esta versão não inclui skills.", + removeTitle: "Remover as skills desmarcadas?", + removeMessage: + "{{skills}} serão excluídas de ~/.agents, ~/.claude e ~/.codex. Tudo o que você adicionou dentro dessas pastas de skills também é excluído.", + saveFailed: "Não foi possível salvar sua seleção de skills.", }, actions: { install: "Instalar", @@ -1903,7 +1914,10 @@ export const ptBR: TranslationResources = { installed: "Instalado", update: "Atualizar", working: "Trabalhando...", + remove: "Remover", uninstall: "Desinstalar", + save: "Salvar", + saving: "Salvando...", }, operations: { add: "Adicionar skill", diff --git a/packages/app/src/i18n/resources/ru.ts b/packages/app/src/i18n/resources/ru.ts index 355c8657f..2e2fbc30a 100644 --- a/packages/app/src/i18n/resources/ru.ts +++ b/packages/app/src/i18n/resources/ru.ts @@ -1189,6 +1189,7 @@ export const ru: TranslationResources = { installFailed: "Невозможно установить навыки оркестровки.", updateFailed: "Невозможно обновить навыки оркестровки.", uninstallFailed: "Невозможно удалить навыки оркестровки.", + saveSelectionFailed: "Не удалось сохранить выбор навыков оркестровки.", }, }, }, @@ -1901,6 +1902,15 @@ export const ru: TranslationResources = { updateFallback: "Синхронизируйте связанные навыки с вашим компьютером.", uninstallTitle: "Удалить навыки Paseo?", uninstallMessage: "Удаляет все навыки оркестровки Paseo из ~/.agents, ~/.claude, ~/.codex.", + choose: "Выбрать навыки", + chooseAll: "Все навыки", + chooseAllHint: "Держите установленными все входящие навыки, включая добавленные позже.", + chooseList: "Входящие навыки", + chooseEmpty: "В этой сборке нет навыков.", + removeTitle: "Удалить снятые навыки?", + removeMessage: + "{{skills}} будут удалены из ~/.agents, ~/.claude и ~/.codex. Всё, что вы добавили внутри этих папок навыков, тоже удаляется.", + saveFailed: "Не удалось сохранить выбор навыков.", }, actions: { install: "Установить", @@ -1908,7 +1918,10 @@ export const ru: TranslationResources = { installed: "Установлено", update: "Обновлять", working: "Работающий...", + remove: "Удалить", uninstall: "Удалить", + save: "Сохранить", + saving: "Сохранение...", }, operations: { add: "Добавить навык", diff --git a/packages/app/src/i18n/resources/zh-CN.ts b/packages/app/src/i18n/resources/zh-CN.ts index 645d84076..0fb6af49c 100644 --- a/packages/app/src/i18n/resources/zh-CN.ts +++ b/packages/app/src/i18n/resources/zh-CN.ts @@ -1144,6 +1144,7 @@ export const zhCN: TranslationResources = { installFailed: "无法安装编排 skills。", updateFailed: "无法更新编排 skills。", uninstallFailed: "无法卸载编排 skills。", + saveSelectionFailed: "无法保存编排 skills 的选择。", }, }, }, @@ -1842,6 +1843,15 @@ export const zhCN: TranslationResources = { updateFallback: "将内置 skills 同步到你的机器。", uninstallTitle: "卸载 Paseo skills?", uninstallMessage: "会从 ~/.agents、~/.claude、~/.codex 移除所有 Paseo 编排 skills。", + choose: "选择 skills", + chooseAll: "全部 skills", + chooseAllHint: "保持安装所有内置 skills,包括以后新增的。", + chooseList: "内置 skills", + chooseEmpty: "此版本未内置 skills。", + removeTitle: "移除取消勾选的 skills?", + removeMessage: + "将从 ~/.agents、~/.claude、~/.codex 中删除 {{skills}}。你在这些 skill 文件夹里添加的内容也会一并删除。", + saveFailed: "无法保存你的 skills 选择。", }, actions: { install: "安装", @@ -1849,7 +1859,10 @@ export const zhCN: TranslationResources = { installed: "已安装", update: "更新", working: "处理中...", + remove: "移除", uninstall: "卸载", + save: "保存", + saving: "正在保存...", }, operations: { add: "添加 skill", diff --git a/packages/app/src/styles/settings.ts b/packages/app/src/styles/settings.ts index 08f685d80..2e5d514c0 100644 --- a/packages/app/src/styles/settings.ts +++ b/packages/app/src/styles/settings.ts @@ -11,13 +11,6 @@ export const settingsStyles = StyleSheet.create((theme) => ({ marginBottom: theme.spacing[3], marginLeft: theme.spacing[1], }, - sectionTitle: { - color: theme.colors.foregroundMuted, - fontSize: theme.fontSize.xs, - fontWeight: theme.fontWeight.normal, - marginBottom: theme.spacing[3], - marginLeft: theme.spacing[1], - }, sectionHeaderTitle: { color: theme.colors.foregroundMuted, fontSize: theme.fontSize.xs, diff --git a/packages/desktop/src/daemon/daemon-manager.ts b/packages/desktop/src/daemon/daemon-manager.ts index 6bac55585..982a62006 100644 --- a/packages/desktop/src/daemon/daemon-manager.ts +++ b/packages/desktop/src/daemon/daemon-manager.ts @@ -23,12 +23,7 @@ import { getCliInstallStatus, installCli, } from "../integrations/cli-install/index.js"; -import { - getSkillsStatus, - installSkills, - uninstallSkills, - updateSkills, -} from "../integrations/skills/index.js"; +import { createSkillsCommandHandlers, getSkillsController } from "../integrations/skills/index.js"; import { openLocalTransportSession, sendLocalTransportMessage, @@ -623,10 +618,7 @@ export function createDaemonCommandHandlers(): Record getLocalDaemonVersion(), install_cli: () => installCli(), get_cli_install_status: () => getCliInstallStatus(), - get_skills_status: () => getSkillsStatus(), - install_skills: () => installSkills(), - update_skills: () => updateSkills(), - uninstall_skills: () => uninstallSkills(), + ...createSkillsCommandHandlers({ controller: getSkillsController() }), }; } diff --git a/packages/desktop/src/integrations/skills/index.ts b/packages/desktop/src/integrations/skills/index.ts index d568e7436..49306a995 100644 --- a/packages/desktop/src/integrations/skills/index.ts +++ b/packages/desktop/src/integrations/skills/index.ts @@ -1,11 +1,49 @@ -export { - autoUpdateInstalledSkills, - getSkillsStatus, - installSkills, - uninstallSkills, - updateSkills, - type SkillOp, - type SkillsState, - type SkillsStatus, - type SkillTargets, +import { app } from "electron"; + +import type { SkillsStatus, SkillTargets } from "./operations.js"; +import { + getAgentsSkillsDir, + getBundledSkillsDir, + getClaudeSkillsDir, + getCodexSkillsDir, +} from "./paths.js"; +import { createSkillSelectionStore } from "./selection-store.js"; +import { createSkillsController, type SkillsController } from "./skills-controller.js"; + +export { createSkillsCommandHandlers } from "./skills-commands.js"; +export { type SkillsController, type SkillsSnapshot } from "./skills-controller.js"; +export type { + SkillOp, + SkillSelection, + SkillsState, + SkillsStatus, + SkillTargets, } from "./operations.js"; + +let controller: SkillsController | null = null; + +function getSkillTargets(): SkillTargets { + return { + sourceDir: getBundledSkillsDir(), + agentsDir: getAgentsSkillsDir(), + claudeDir: getClaudeSkillsDir(), + codexDir: getCodexSkillsDir(), + }; +} + +/** + * One controller per process. Startup convergence and the settings commands must + * share it, or they are two writers over the same directories again. + */ +export function getSkillsController(): SkillsController { + controller ??= createSkillsController({ + resolveTargets: getSkillTargets, + selectionStore: createSkillSelectionStore({ userDataPath: app.getPath("userData") }), + }); + return controller; +} + +/** Startup convergence: keep whatever the user selected in sync with the bundle. */ +export async function autoUpdateInstalledSkills(): Promise { + return getSkillsController().autoUpdate(); +} diff --git a/packages/desktop/src/integrations/skills/operations.test.ts b/packages/desktop/src/integrations/skills/operations.test.ts index 1980a29ec..6b188d24b 100644 --- a/packages/desktop/src/integrations/skills/operations.test.ts +++ b/packages/desktop/src/integrations/skills/operations.test.ts @@ -1,25 +1,25 @@ import { promises as fs } from "node:fs"; import os from "node:os"; import path from "node:path"; -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; - -vi.mock("electron", () => ({ - app: { - getPath: vi.fn(() => "/tmp/paseo-user-data"), - isPackaged: false, - }, -})); +import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { autoUpdateInstalledSkills, getSkillsStatus, installSkills, - PASEO_SKILL_NAMES, + LEGACY_SKILL_NAMES, + type SkillSelection, type SkillTargets, uninstallSkills, updateSkills, } from "./operations"; +const ALL_SKILLS: SkillSelection = { mode: "all" }; + +function only(...skills: string[]): SkillSelection { + return { mode: "custom", skills }; +} + interface Sandbox { root: string; targets: SkillTargets; @@ -85,6 +85,14 @@ async function pathExists(p: string): Promise { .catch(() => false); } +async function installedIn(targets: SkillTargets, name: string): Promise { + return Promise.all( + [targets.agentsDir, targets.claudeDir, targets.codexDir].map((dir) => + pathExists(path.join(dir, name)), + ), + ); +} + describe("getSkillsStatus", () => { let sandbox: Sandbox; @@ -99,7 +107,7 @@ describe("getSkillsStatus", () => { it("returns not-installed with add ops for every bundled skill when nothing is on disk", async () => { await writeCurrentBundle(sandbox.targets.sourceDir); - const status = await getSkillsStatus(sandbox.targets); + const status = await getSkillsStatus(sandbox.targets, ALL_SKILLS); expect(status.state).toBe("not-installed"); expect(status.ops).toEqual([ @@ -108,13 +116,46 @@ describe("getSkillsStatus", () => { ]); }); + it("reports every bundled skill as available", async () => { + await writeCurrentBundle(sandbox.targets.sourceDir); + await writeBundleSkill(sandbox.targets.sourceDir, "paseo-advisor", { + "SKILL.md": "advisor-v1", + }); + + const status = await getSkillsStatus(sandbox.targets, only("paseo")); + + expect(status.available).toEqual(["paseo", "paseo-advisor", "paseo-loop"]); + }); + + it("reports a skill present in only one target as installed", async () => { + await writeCurrentBundle(sandbox.targets.sourceDir); + await writeOnDiskSkill(sandbox.targets.claudeDir, "paseo", { "SKILL.md": "paseo-v1" }); + + const status = await getSkillsStatus(sandbox.targets, ALL_SKILLS); + + // `add` means "missing from at least one target", so it cannot answer + // "is there a directory here to delete". `installed` answers that. + expect(status.installed).toEqual(["paseo"]); + expect(status.ops).toEqual([ + { kind: "add", name: "paseo" }, + { kind: "add", name: "paseo-loop" }, + ]); + }); + + it("reports legacy skill directories left on disk as installed", async () => { + await writeCurrentBundle(sandbox.targets.sourceDir); + await writeOnDiskSkill(sandbox.targets.agentsDir, "paseo-chat", { "SKILL.md": "chat-old" }); + + expect((await getSkillsStatus(sandbox.targets, ALL_SKILLS)).installed).toEqual(["paseo-chat"]); + }); + it("returns not-installed when only user-personal skill dirs exist (the live bug)", async () => { await writeCurrentBundle(sandbox.targets.sourceDir); for (const name of ["unslop", "tdd", "devbox"]) { await writeOnDiskSkill(sandbox.targets.agentsDir, name, { "SKILL.md": `user-${name}` }); } - const status = await getSkillsStatus(sandbox.targets); + const status = await getSkillsStatus(sandbox.targets, ALL_SKILLS); expect(status.state).toBe("not-installed"); expect(status.ops).toEqual([ @@ -128,9 +169,14 @@ describe("getSkillsStatus", () => { await writeOnDiskSkillToAllTargets(sandbox.targets, "paseo", { "SKILL.md": "paseo-v1" }); await writeOnDiskSkillToAllTargets(sandbox.targets, "paseo-loop", { "SKILL.md": "loop-v1" }); - const status = await getSkillsStatus(sandbox.targets); + const status = await getSkillsStatus(sandbox.targets, ALL_SKILLS); - expect(status).toEqual({ state: "up-to-date", ops: [] }); + expect(status).toEqual({ + state: "up-to-date", + ops: [], + available: ["paseo", "paseo-loop"], + installed: ["paseo", "paseo-loop"], + }); }); it("ignores user-added files inside current managed skill dirs in every target", async () => { @@ -150,9 +196,14 @@ describe("getSkillsStatus", () => { "hooks/guard.sh": "user guard", }); - const status = await getSkillsStatus(sandbox.targets); + const status = await getSkillsStatus(sandbox.targets, ALL_SKILLS); - expect(status).toEqual({ state: "up-to-date", ops: [] }); + expect(status).toEqual({ + state: "up-to-date", + ops: [], + available: ["paseo", "paseo-loop"], + installed: ["paseo", "paseo-loop"], + }); }); it("returns drift with a single update op when one bundled file diverges", async () => { @@ -162,7 +213,7 @@ describe("getSkillsStatus", () => { await writeOnDiskSkill(sandbox.targets.codexDir, "paseo", { "SKILL.md": "paseo-v1" }); await writeOnDiskSkillToAllTargets(sandbox.targets, "paseo-loop", { "SKILL.md": "loop-v1" }); - const status = await getSkillsStatus(sandbox.targets); + const status = await getSkillsStatus(sandbox.targets, ALL_SKILLS); expect(status.state).toBe("drift"); expect(status.ops).toEqual([{ kind: "update", name: "paseo" }]); @@ -177,7 +228,7 @@ describe("getSkillsStatus", () => { await writeOnDiskSkill(sandbox.targets.codexDir, "paseo", { "SKILL.md": "paseo-v1" }); await writeOnDiskSkill(sandbox.targets.codexDir, "paseo-loop", { "SKILL.md": "loop-v1" }); - const status = await getSkillsStatus(sandbox.targets); + const status = await getSkillsStatus(sandbox.targets, ALL_SKILLS); expect(status.state).toBe("drift"); expect(status.ops).toEqual([{ kind: "update", name: "paseo" }]); @@ -187,7 +238,7 @@ describe("getSkillsStatus", () => { await writeCurrentBundle(sandbox.targets.sourceDir); await writeOnDiskSkillToAllTargets(sandbox.targets, "paseo", { "SKILL.md": "paseo-v1" }); - const status = await getSkillsStatus(sandbox.targets); + const status = await getSkillsStatus(sandbox.targets, ALL_SKILLS); expect(status.state).toBe("drift"); expect(status.ops).toEqual([{ kind: "add", name: "paseo-loop" }]); @@ -199,7 +250,7 @@ describe("getSkillsStatus", () => { await writeOnDiskSkillToAllTargets(sandbox.targets, "paseo-loop", { "SKILL.md": "loop-v1" }); await writeOnDiskSkill(sandbox.targets.agentsDir, "paseo-chat", { "SKILL.md": "chat-old" }); - const status = await getSkillsStatus(sandbox.targets); + const status = await getSkillsStatus(sandbox.targets, ALL_SKILLS); expect(status.state).toBe("drift"); expect(status.ops).toEqual([{ kind: "delete", name: "paseo-chat" }]); @@ -212,7 +263,7 @@ describe("getSkillsStatus", () => { await writeOnDiskSkill(sandbox.targets.codexDir, "paseo", { "SKILL.md": "paseo-v1" }); await writeOnDiskSkill(sandbox.targets.agentsDir, "paseo-chat", { "SKILL.md": "chat-old" }); - const status = await getSkillsStatus(sandbox.targets); + const status = await getSkillsStatus(sandbox.targets, ALL_SKILLS); expect(status.state).toBe("drift"); expect(status.ops).toEqual([ @@ -223,6 +274,116 @@ describe("getSkillsStatus", () => { }); }); +describe("custom skill selection", () => { + let sandbox: Sandbox; + + beforeEach(async () => { + sandbox = await makeSandbox(); + await writeCurrentBundle(sandbox.targets.sourceDir); + await writeBundleSkill(sandbox.targets.sourceDir, "paseo-advisor", { + "SKILL.md": "advisor-v1", + }); + }); + + afterEach(async () => { + await fs.rm(sandbox.root, { recursive: true, force: true }); + }); + + it("installs only the selected skills", async () => { + const status = await installSkills(sandbox.targets, only("paseo", "paseo-loop")); + + expect(status).toEqual({ + state: "up-to-date", + ops: [], + available: ["paseo", "paseo-advisor", "paseo-loop"], + installed: ["paseo", "paseo-loop"], + }); + expect(await installedIn(sandbox.targets, "paseo")).toEqual([true, true, true]); + expect(await installedIn(sandbox.targets, "paseo-loop")).toEqual([true, true, true]); + expect(await installedIn(sandbox.targets, "paseo-advisor")).toEqual([false, false, false]); + }); + + it("reports up-to-date while an unselected bundled skill is absent", async () => { + await installSkills(sandbox.targets, only("paseo")); + + const status = await getSkillsStatus(sandbox.targets, only("paseo")); + + expect(status).toEqual({ + state: "up-to-date", + ops: [], + available: ["paseo", "paseo-advisor", "paseo-loop"], + installed: ["paseo"], + }); + }); + + it("removes a previously installed skill once it leaves the selection", async () => { + await installSkills(sandbox.targets, ALL_SKILLS); + + const status = await installSkills(sandbox.targets, only("paseo")); + + expect(status.state).toBe("up-to-date"); + expect(await installedIn(sandbox.targets, "paseo")).toEqual([true, true, true]); + expect(await installedIn(sandbox.targets, "paseo-loop")).toEqual([false, false, false]); + expect(await installedIn(sandbox.targets, "paseo-advisor")).toEqual([false, false, false]); + }); + + it("reports a delete op for a deselected skill before it is applied", async () => { + await installSkills(sandbox.targets, ALL_SKILLS); + + const status = await getSkillsStatus(sandbox.targets, only("paseo", "paseo-advisor")); + + expect(status.state).toBe("drift"); + expect(status.ops).toEqual([{ kind: "delete", name: "paseo-loop" }]); + }); + + it("leaves unrelated user skills untouched", async () => { + await writeOnDiskSkill(sandbox.targets.agentsDir, "unslop", { "SKILL.md": "user-unslop" }); + + await installSkills(sandbox.targets, only("paseo")); + + expect( + await fs.readFile(path.join(sandbox.targets.agentsDir, "unslop", "SKILL.md"), "utf-8"), + ).toBe("user-unslop"); + }); + + it("treats an empty custom selection as no managed skills installed", async () => { + await installSkills(sandbox.targets, ALL_SKILLS); + + const status = await installSkills(sandbox.targets, only()); + + expect(status).toEqual({ + state: "not-installed", + ops: [], + available: ["paseo", "paseo-advisor", "paseo-loop"], + installed: [], + }); + expect(await installedIn(sandbox.targets, "paseo")).toEqual([false, false, false]); + expect(await installedIn(sandbox.targets, "paseo-loop")).toEqual([false, false, false]); + }); + + it("ignores selected names that the bundle does not ship", async () => { + const status = await installSkills(sandbox.targets, only("paseo", "not-a-skill")); + + expect(status).toEqual({ + state: "up-to-date", + ops: [], + available: ["paseo", "paseo-advisor", "paseo-loop"], + installed: ["paseo"], + }); + expect(await installedIn(sandbox.targets, "not-a-skill")).toEqual([false, false, false]); + }); + + it("still deletes legacy skill names that are not selectable", async () => { + await writeOnDiskSkillToAllTargets(sandbox.targets, "paseo-orchestrator", { + "SKILL.md": "orchestrator-old", + }); + + await installSkills(sandbox.targets, only("paseo")); + + expect(await installedIn(sandbox.targets, "paseo-orchestrator")).toEqual([false, false, false]); + }); +}); + describe("installSkills / updateSkills", () => { let sandbox: Sandbox; @@ -238,9 +399,14 @@ describe("installSkills / updateSkills", () => { await writeCurrentBundle(sandbox.targets.sourceDir); await writeOnDiskSkill(sandbox.targets.agentsDir, "unslop", { "SKILL.md": "user-unslop" }); - const status = await installSkills(sandbox.targets); + const status = await installSkills(sandbox.targets, ALL_SKILLS); - expect(status).toEqual({ state: "up-to-date", ops: [] }); + expect(status).toEqual({ + state: "up-to-date", + ops: [], + available: ["paseo", "paseo-loop"], + installed: ["paseo", "paseo-loop"], + }); for (const name of ["paseo", "paseo-loop"]) { expect( await fs.readFile(path.join(sandbox.targets.agentsDir, name, "SKILL.md"), "utf-8"), @@ -255,16 +421,21 @@ describe("installSkills / updateSkills", () => { ).toBe("user-unslop"); }); - it("converges to up-to-date when state has missing + edited + legacy skills", async () => { + it("repairs missing and edited skills without deleting a legacy directory", async () => { await writeCurrentBundle(sandbox.targets.sourceDir); await writeOnDiskSkill(sandbox.targets.agentsDir, "paseo", { "SKILL.md": "stale" }); await writeOnDiskSkill(sandbox.targets.agentsDir, "paseo-chat", { "SKILL.md": "chat-old" }); await writeOnDiskSkill(sandbox.targets.claudeDir, "paseo-chat", { "SKILL.md": "chat-old" }); await writeOnDiskSkill(sandbox.targets.codexDir, "paseo-chat", { "SKILL.md": "chat-old" }); - const status = await updateSkills(sandbox.targets); + const status = await updateSkills(sandbox.targets, ALL_SKILLS); - expect(status).toEqual({ state: "up-to-date", ops: [] }); + expect(status).toEqual({ + state: "drift", + ops: [{ kind: "delete", name: "paseo-chat" }], + available: ["paseo", "paseo-loop"], + installed: ["paseo", "paseo-chat", "paseo-loop"], + }); expect( await fs.readFile(path.join(sandbox.targets.agentsDir, "paseo", "SKILL.md"), "utf-8"), ).toBe("paseo-v1"); @@ -276,7 +447,7 @@ describe("installSkills / updateSkills", () => { sandbox.targets.claudeDir, sandbox.targets.codexDir, ]) { - expect(await pathExists(path.join(dir, "paseo-chat"))).toBe(false); + expect(await pathExists(path.join(dir, "paseo-chat"))).toBe(true); } }); @@ -297,10 +468,15 @@ describe("installSkills / updateSkills", () => { "prompts/local.md": "codex prompt", }); - const status = await updateSkills(sandbox.targets); + const status = await updateSkills(sandbox.targets, ALL_SKILLS); - expect(status).toEqual({ state: "up-to-date", ops: [] }); - expect(await getSkillsStatus(sandbox.targets)).toEqual({ state: "up-to-date", ops: [] }); + expect(status.state).toBe("up-to-date"); + expect(await getSkillsStatus(sandbox.targets, ALL_SKILLS)).toEqual({ + state: "up-to-date", + ops: [], + available: ["paseo", "paseo-loop"], + installed: ["paseo", "paseo-loop"], + }); expect( await fs.readFile( path.join(sandbox.targets.agentsDir, "paseo", "hooks", "guard.sh"), @@ -329,9 +505,9 @@ describe("installSkills / updateSkills", () => { await writeOnDiskSkill(sandbox.targets.codexDir, "paseo", { "SKILL.md": "paseo-v1" }); await writeOnDiskSkill(sandbox.targets.codexDir, "paseo-loop", { "SKILL.md": "loop-v1" }); - const status = await updateSkills(sandbox.targets); + const status = await updateSkills(sandbox.targets, ALL_SKILLS); - expect(status).toEqual({ state: "up-to-date", ops: [] }); + expect(status.state).toBe("up-to-date"); expect( await fs.readFile(path.join(sandbox.targets.claudeDir, "paseo-loop", "SKILL.md"), "utf-8"), ).toBe("loop-v1"); @@ -344,10 +520,10 @@ describe("installSkills / updateSkills", () => { "hooks/guard.sh": "user guard", }); - const status = await autoUpdateInstalledSkills(sandbox.targets); + const status = await autoUpdateInstalledSkills(sandbox.targets, ALL_SKILLS); - expect(status).toEqual({ state: "up-to-date", ops: [] }); - expect(await getSkillsStatus(sandbox.targets)).toEqual({ state: "up-to-date", ops: [] }); + expect(status.state).toBe("up-to-date"); + expect((await getSkillsStatus(sandbox.targets, ALL_SKILLS)).state).toBe("up-to-date"); expect( await fs.readFile(path.join(sandbox.targets.agentsDir, "paseo", "SKILL.md"), "utf-8"), ).toBe("paseo-v1"); @@ -362,7 +538,7 @@ describe("installSkills / updateSkills", () => { it("does not auto-install skills on a clean machine", async () => { await writeCurrentBundle(sandbox.targets.sourceDir); - const status = await autoUpdateInstalledSkills(sandbox.targets); + const status = await autoUpdateInstalledSkills(sandbox.targets, ALL_SKILLS); expect(status).toEqual({ state: "not-installed", @@ -370,20 +546,20 @@ describe("installSkills / updateSkills", () => { { kind: "add", name: "paseo" }, { kind: "add", name: "paseo-loop" }, ], + available: ["paseo", "paseo-loop"], + installed: [], }); - expect(await pathExists(path.join(sandbox.targets.agentsDir, "paseo"))).toBe(false); - expect(await pathExists(path.join(sandbox.targets.claudeDir, "paseo"))).toBe(false); - expect(await pathExists(path.join(sandbox.targets.codexDir, "paseo"))).toBe(false); + expect(await installedIn(sandbox.targets, "paseo")).toEqual([false, false, false]); }); it("is idempotent — running install twice keeps state at up-to-date", async () => { await writeCurrentBundle(sandbox.targets.sourceDir); - const first = await installSkills(sandbox.targets); - const second = await installSkills(sandbox.targets); + const first = await installSkills(sandbox.targets, ALL_SKILLS); + const second = await installSkills(sandbox.targets, ALL_SKILLS); - expect(first).toEqual({ state: "up-to-date", ops: [] }); - expect(second).toEqual({ state: "up-to-date", ops: [] }); + expect(first.state).toBe("up-to-date"); + expect(second.state).toBe("up-to-date"); }); }); @@ -400,18 +576,16 @@ describe("uninstallSkills", () => { it("removes every Paseo skill from all three targets and preserves user dirs", async () => { await writeCurrentBundle(sandbox.targets.sourceDir); - await installSkills(sandbox.targets); + await installSkills(sandbox.targets, ALL_SKILLS); for (const name of ["unslop", "tdd", "devbox"]) { await writeOnDiskSkill(sandbox.targets.agentsDir, name, { "SKILL.md": `user-${name}` }); } - const status = await uninstallSkills(sandbox.targets); + const status = await uninstallSkills(sandbox.targets, ALL_SKILLS); expect(status.state).toBe("not-installed"); - for (const name of PASEO_SKILL_NAMES) { - expect(await pathExists(path.join(sandbox.targets.agentsDir, name))).toBe(false); - expect(await pathExists(path.join(sandbox.targets.claudeDir, name))).toBe(false); - expect(await pathExists(path.join(sandbox.targets.codexDir, name))).toBe(false); + for (const name of ["paseo", "paseo-loop", ...LEGACY_SKILL_NAMES]) { + expect(await installedIn(sandbox.targets, name)).toEqual([false, false, false]); } for (const name of ["unslop", "tdd", "devbox"]) { expect( @@ -423,7 +597,7 @@ describe("uninstallSkills", () => { it("is a no-op when nothing is installed", async () => { await writeCurrentBundle(sandbox.targets.sourceDir); - const status = await uninstallSkills(sandbox.targets); + const status = await uninstallSkills(sandbox.targets, ALL_SKILLS); expect(status.state).toBe("not-installed"); }); @@ -438,15 +612,9 @@ describe("uninstallSkills", () => { await writeOnDiskSkill(dir, "paseo-chat", { "SKILL.md": "chat-old" }); } - const status = await uninstallSkills(sandbox.targets); + const status = await uninstallSkills(sandbox.targets, ALL_SKILLS); expect(status.state).toBe("not-installed"); - for (const dir of [ - sandbox.targets.agentsDir, - sandbox.targets.claudeDir, - sandbox.targets.codexDir, - ]) { - expect(await pathExists(path.join(dir, "paseo-chat"))).toBe(false); - } + expect(await installedIn(sandbox.targets, "paseo-chat")).toEqual([false, false, false]); }); }); diff --git a/packages/desktop/src/integrations/skills/operations.ts b/packages/desktop/src/integrations/skills/operations.ts index 0a226e9f8..da118a171 100644 --- a/packages/desktop/src/integrations/skills/operations.ts +++ b/packages/desktop/src/integrations/skills/operations.ts @@ -1,12 +1,6 @@ import { createHash } from "node:crypto"; import { promises as fs } from "node:fs"; import path from "node:path"; -import { - getAgentsSkillsDir, - getBundledSkillsDir, - getClaudeSkillsDir, - getCodexSkillsDir, -} from "./paths.js"; import { listFilesRecursive, removeSkill, syncSkills } from "./sync.js"; export type SkillsState = "not-installed" | "up-to-date" | "drift"; @@ -16,9 +10,20 @@ export type SkillOp = | { kind: "update"; name: string } | { kind: "delete"; name: string }; +/** What the user asked to have installed. `all` follows the bundle as it grows. */ +export type SkillSelection = { mode: "all" } | { mode: "custom"; skills: readonly string[] }; + export interface SkillsStatus { state: SkillsState; ops: SkillOp[]; + /** Every skill the bundle currently ships, sorted. The selectable catalog. */ + available: string[]; + /** + * Managed skills with a directory in at least one agent home, sorted. An `add` + * op means "missing from at least one target", so it cannot answer whether + * there is anything on disk to delete — this can. + */ + installed: string[]; } export interface SkillTargets { @@ -28,14 +33,10 @@ export interface SkillTargets { codexDir: string; } -export const PASEO_SKILL_NAMES = [ - "paseo", - "paseo-advisor", +// Names the bundle used to ship. They are never selectable, but every scan still +// covers them so an older install's copies get cleaned up. +export const LEGACY_SKILL_NAMES = [ "paseo-chat", - "paseo-committee", - "paseo-handoff", - "paseo-loop", - // Keep removed bundle names here so auto-update deletes stale installed copies. "paseo-epic", "paseo-orchestrate", "paseo-orchestrator", @@ -44,13 +45,35 @@ export const PASEO_SKILL_NAMES = [ type SkillFiles = Map; type TargetSkills = Map; -function resolveSkillTargets(): SkillTargets { - return { - sourceDir: getBundledSkillsDir(), - agentsDir: getAgentsSkillsDir(), - claudeDir: getClaudeSkillsDir(), - codexDir: getCodexSkillsDir(), - }; +/** + * The bundle directory is the catalog. Reading it instead of a hardcoded list is + * what makes `all` pick up skills added in a later release with no code change. + */ +async function listBundledSkills(sourceDir: string): Promise { + const entries = await fs.readdir(sourceDir, { withFileTypes: true }).catch(() => []); + return entries + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + .sort(compareStrings); +} + +/** Every name Paseo owns on disk: what it ships now plus what it used to ship. */ +function managedSkillNames(available: readonly string[]): string[] { + return [...new Set([...available, ...LEGACY_SKILL_NAMES])].sort(compareStrings); +} + +/** The names a convergence may create, replace, or delete. */ +export async function listManagedSkillNames(sourceDir: string): Promise { + return managedSkillNames(await listBundledSkills(sourceDir)); +} + +function resolveDesiredSkills( + selection: SkillSelection, + available: readonly string[], +): Set { + if (selection.mode === "all") return new Set(available); + const chosen = new Set(selection.skills); + return new Set(available.filter((name) => chosen.has(name))); } async function hashSkillDir(skillDir: string): Promise { @@ -67,19 +90,24 @@ async function hashSkillDir(skillDir: string): Promise { return files; } -async function hashSkills(rootDir: string): Promise> { - const out = new Map(); - for (const name of PASEO_SKILL_NAMES) { +async function hashSkills(rootDir: string, names: readonly string[]): Promise { + const out: TargetSkills = new Map(); + for (const name of names) { const files = await hashSkillDir(path.join(rootDir, name)); if (files !== null) out.set(name, files); } return out; } -function diff(bundle: TargetSkills, disks: readonly TargetSkills[]): SkillOp[] { +function diff( + bundle: TargetSkills, + disks: readonly TargetSkills[], + names: readonly string[], + desired: ReadonlySet, +): SkillOp[] { const ops: SkillOp[] = []; - for (const name of PASEO_SKILL_NAMES) { - const b = bundle.get(name); + for (const name of names) { + const b = desired.has(name) ? bundle.get(name) : undefined; const targetFiles = disks.map((disk) => disk.get(name)); const installedTargets = targetFiles.filter( (files): files is SkillFiles => files !== undefined, @@ -101,6 +129,10 @@ function hasInstalledPaseoSkill(disks: readonly TargetSkills[]): boolean { return disks.some((disk) => disk.size > 0); } +function installedSkillNames(disks: readonly TargetSkills[], names: readonly string[]): string[] { + return names.filter((name) => disks.some((disk) => disk.has(name))); +} + function bundleFilesMatch(bundle: SkillFiles, disk: SkillFiles): boolean { for (const [rel, sha] of bundle) { if (disk.get(rel) !== sha) return false; @@ -118,27 +150,33 @@ function compareStrings(a: string, b: string): number { return 0; } -export async function getSkillsStatus(targets?: SkillTargets): Promise { - const t = targets ?? resolveSkillTargets(); +export async function getSkillsStatus( + targets: SkillTargets, + selection: SkillSelection, +): Promise { + const available = await listBundledSkills(targets.sourceDir); + const names = managedSkillNames(available); const [bundle, agentsDisk, claudeDisk, codexDisk] = await Promise.all([ - hashSkills(t.sourceDir), - hashSkills(t.agentsDir), - hashSkills(t.claudeDir), - hashSkills(t.codexDir), + hashSkills(targets.sourceDir, available), + hashSkills(targets.agentsDir, names), + hashSkills(targets.claudeDir, names), + hashSkills(targets.codexDir, names), ]); const disks = [agentsDisk, claudeDisk, codexDisk]; - const ops = diff(bundle, disks); + const ops = diff(bundle, disks, names, resolveDesiredSkills(selection, available)); + const installed = installedSkillNames(disks, names); - if (!hasInstalledPaseoSkill(disks)) return { state: "not-installed", ops }; - if (ops.length === 0) return { state: "up-to-date", ops }; - return { state: "drift", ops }; + if (!hasInstalledPaseoSkill(disks)) return { state: "not-installed", ops, available, installed }; + if (ops.length === 0) return { state: "up-to-date", ops, available, installed }; + return { state: "drift", ops, available, installed }; } async function applySkills( targets: SkillTargets, + selection: SkillSelection, initialStatus?: SkillsStatus, ): Promise { - const status = initialStatus ?? (await getSkillsStatus(targets)); + const status = initialStatus ?? (await getSkillsStatus(targets, selection)); const writes = status.ops .filter((op) => op.kind === "add" || op.kind === "update") @@ -162,32 +200,52 @@ async function applySkills( }); } - return getSkillsStatus(targets); + return getSkillsStatus(targets, selection); } -export async function installSkills(targets?: SkillTargets): Promise { - return applySkills(targets ?? resolveSkillTargets()); +export async function installSkills( + targets: SkillTargets, + selection: SkillSelection, + /** Apply exactly this plan instead of rescanning, so a confirmed plan is the applied plan. */ + plan?: SkillsStatus, +): Promise { + return applySkills(targets, selection, plan); } -export async function updateSkills(targets?: SkillTargets): Promise { - return applySkills(targets ?? resolveSkillTargets()); +export async function updateSkills( + targets: SkillTargets, + selection: SkillSelection, +): Promise { + const status = await getSkillsStatus(targets, selection); + return applySkills(targets, selection, nonDestructivePlan(status)); } -export async function autoUpdateInstalledSkills(targets?: SkillTargets): Promise { - const t = targets ?? resolveSkillTargets(); - const status = await getSkillsStatus(t); +function nonDestructivePlan(status: SkillsStatus): SkillsStatus { + return { ...status, ops: status.ops.filter((op) => op.kind !== "delete") }; +} + +export async function autoUpdateInstalledSkills( + targets: SkillTargets, + selection: SkillSelection, +): Promise { + const status = await getSkillsStatus(targets, selection); if (status.state !== "drift") return status; - return applySkills(t, status); + // Automatic maintenance may repair selected skills, but removal is an + // interactive operation because managed directories can contain user files. + return applySkills(targets, selection, nonDestructivePlan(status)); } -export async function uninstallSkills(targets?: SkillTargets): Promise { - const t = targets ?? resolveSkillTargets(); - for (const name of PASEO_SKILL_NAMES) { +export async function uninstallSkills( + targets: SkillTargets, + selection: SkillSelection, +): Promise { + const available = await listBundledSkills(targets.sourceDir); + for (const name of managedSkillNames(available)) { await removeSkill(name, { - agentsDir: t.agentsDir, - claudeDir: t.claudeDir, - codexDir: t.codexDir, + agentsDir: targets.agentsDir, + claudeDir: targets.claudeDir, + codexDir: targets.codexDir, }); } - return getSkillsStatus(t); + return getSkillsStatus(targets, selection); } diff --git a/packages/desktop/src/integrations/skills/selection-store.test.ts b/packages/desktop/src/integrations/skills/selection-store.test.ts new file mode 100644 index 000000000..3f1ddc932 --- /dev/null +++ b/packages/desktop/src/integrations/skills/selection-store.test.ts @@ -0,0 +1,150 @@ +import { mkdtemp, readdir, readFile, rm, writeFile } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; + +import { createSkillSelectionStore } from "./selection-store"; + +const directories = new Set(); + +async function createTempUserDataDir(): Promise { + const dir = await mkdtemp(path.join(os.tmpdir(), "paseo-skill-selection-")); + directories.add(dir); + return dir; +} + +function selectionFilePath(userDataPath: string): string { + return path.join(userDataPath, "skill-selection.json"); +} + +async function seedSelectionFile(userDataPath: string, raw: string): Promise { + await writeFile(selectionFilePath(userDataPath), raw, "utf8"); +} + +afterEach(async () => { + for (const dir of directories) { + await rm(dir, { recursive: true, force: true }); + } + directories.clear(); +}); + +describe("createSkillSelectionStore", () => { + it("reports all skills when nothing has ever been saved", async () => { + const userDataPath = await createTempUserDataDir(); + const store = createSkillSelectionStore({ userDataPath }); + + expect(await store.get()).toEqual({ mode: "all" }); + expect(await readdir(userDataPath)).toEqual([]); + }); + + it("round-trips a custom selection through a fresh store", async () => { + const userDataPath = await createTempUserDataDir(); + + const saved = await createSkillSelectionStore({ userDataPath }).set({ + mode: "custom", + skills: ["paseo", "paseo-loop"], + }); + const reloaded = await createSkillSelectionStore({ userDataPath }).get(); + + expect(saved).toEqual({ mode: "custom", skills: ["paseo", "paseo-loop"] }); + expect(reloaded).toEqual({ mode: "custom", skills: ["paseo", "paseo-loop"] }); + expect(await readdir(userDataPath)).toEqual(["skill-selection.json"]); + }); + + it("sorts, de-duplicates, and drops non-string names before persisting", async () => { + const userDataPath = await createTempUserDataDir(); + const store = createSkillSelectionStore({ userDataPath }); + + const saved = await store.set({ + mode: "custom", + skills: ["paseo-loop", "paseo", "paseo-loop", " ", 7, null], + }); + + expect(saved).toEqual({ mode: "custom", skills: ["paseo", "paseo-loop"] }); + expect(JSON.parse(await readFile(selectionFilePath(userDataPath), "utf8"))).toEqual({ + version: 1, + selection: { mode: "custom", skills: ["paseo", "paseo-loop"] }, + }); + }); + + it("keeps an empty custom selection as a deliberate choice", async () => { + const userDataPath = await createTempUserDataDir(); + + await createSkillSelectionStore({ userDataPath }).set({ mode: "custom", skills: [] }); + + expect(await createSkillSelectionStore({ userDataPath }).get()).toEqual({ + mode: "custom", + skills: [], + }); + }); + + it("switches back to all and forgets the previous custom names", async () => { + const userDataPath = await createTempUserDataDir(); + const store = createSkillSelectionStore({ userDataPath }); + await store.set({ mode: "custom", skills: ["paseo"] }); + + const saved = await store.set({ mode: "all" }); + + expect(saved).toEqual({ mode: "all" }); + expect(await createSkillSelectionStore({ userDataPath }).get()).toEqual({ mode: "all" }); + }); + + it("falls back to all when the saved file is not valid JSON", async () => { + const userDataPath = await createTempUserDataDir(); + await seedSelectionFile(userDataPath, "{ not valid json"); + + expect(await createSkillSelectionStore({ userDataPath }).get()).toEqual({ mode: "all" }); + }); + + it("falls back to all when the saved mode is unknown", async () => { + const userDataPath = await createTempUserDataDir(); + await seedSelectionFile( + userDataPath, + JSON.stringify({ version: 1, selection: { mode: "some-future-mode", skills: ["paseo"] } }), + ); + + expect(await createSkillSelectionStore({ userDataPath }).get()).toEqual({ mode: "all" }); + }); + + it("normalizes a custom selection that was hand-edited on disk", async () => { + const userDataPath = await createTempUserDataDir(); + await seedSelectionFile( + userDataPath, + JSON.stringify({ + version: 1, + selection: { mode: "custom", skills: ["paseo-loop", 3, "paseo", "paseo"] }, + }), + ); + + expect(await createSkillSelectionStore({ userDataPath }).get()).toEqual({ + mode: "custom", + skills: ["paseo", "paseo-loop"], + }); + }); + + it("treats a custom selection with no usable skills array as an empty selection", async () => { + const userDataPath = await createTempUserDataDir(); + await seedSelectionFile( + userDataPath, + JSON.stringify({ version: 1, selection: { mode: "custom" } }), + ); + + expect(await createSkillSelectionStore({ userDataPath }).get()).toEqual({ + mode: "custom", + skills: [], + }); + }); + + it("leaves no temp files behind when saves overlap", async () => { + const userDataPath = await createTempUserDataDir(); + const store = createSkillSelectionStore({ userDataPath }); + + await Promise.all([ + store.set({ mode: "custom", skills: ["paseo"] }), + store.set({ mode: "all" }), + store.set({ mode: "custom", skills: ["paseo-loop"] }), + ]); + + expect(await readdir(userDataPath)).toEqual(["skill-selection.json"]); + }); +}); diff --git a/packages/desktop/src/integrations/skills/selection-store.ts b/packages/desktop/src/integrations/skills/selection-store.ts new file mode 100644 index 000000000..3de94c651 --- /dev/null +++ b/packages/desktop/src/integrations/skills/selection-store.ts @@ -0,0 +1,101 @@ +import { randomUUID } from "node:crypto"; +import { mkdir, readFile, rename, writeFile } from "node:fs/promises"; +import path from "node:path"; + +import type { SkillSelection } from "./operations.js"; + +export interface SkillSelectionStore { + get(): Promise; + set(selection: unknown): Promise; +} + +interface PersistedSkillSelectionDocument { + version: 1; + selection: SkillSelection; +} + +const SKILL_SELECTION_FILENAME = "skill-selection.json"; + +// Installs that predate the picker have no file. They keep every bundled skill, +// which is what they had before, and what `all` keeps meaning as the bundle grows. +const DEFAULT_SKILL_SELECTION: SkillSelection = { mode: "all" }; + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function isNodeError(error: unknown): error is NodeJS.ErrnoException { + return error instanceof Error; +} + +export function coerceSkillNames(value: unknown): string[] { + if (!Array.isArray(value)) return []; + const names = value + .filter((entry): entry is string => typeof entry === "string") + .map((entry) => entry.trim()) + .filter((entry) => entry.length > 0); + return [...new Set(names)].sort(); +} + +/** + * The one place an unvalidated selection becomes a `SkillSelection`. Whether a + * name is actually in the bundle is not decided here — the catalog belongs to + * the bundle directory, so `operations` resolves that against the live catalog. + */ +export function coerceSkillSelection(value: unknown): SkillSelection { + if (!isRecord(value)) return DEFAULT_SKILL_SELECTION; + if (value.mode === "all") return { mode: "all" }; + if (value.mode === "custom") return { mode: "custom", skills: coerceSkillNames(value.skills) }; + return DEFAULT_SKILL_SELECTION; +} + +function coerceDocument(value: unknown): PersistedSkillSelectionDocument { + const selection = isRecord(value) + ? coerceSkillSelection(value.selection) + : DEFAULT_SKILL_SELECTION; + return { version: 1, selection }; +} + +export function createSkillSelectionStore({ + userDataPath, +}: { + userDataPath: string; +}): SkillSelectionStore { + const filePath = path.join(userDataPath, SKILL_SELECTION_FILENAME); + let persistQueue: Promise = Promise.resolve(); + + async function persist(document: PersistedSkillSelectionDocument): Promise { + const write = async () => { + await mkdir(userDataPath, { recursive: true }); + const tempFilePath = `${filePath}.tmp.${process.pid}.${randomUUID()}`; + await writeFile(tempFilePath, `${JSON.stringify(document, null, 2)}\n`, "utf8"); + await rename(tempFilePath, filePath); + }; + const queued = persistQueue.then(write, write); + persistQueue = queued.catch(() => undefined); + await queued; + } + + return { + async get(): Promise { + let raw: string; + try { + raw = await readFile(filePath, "utf8"); + } catch (error) { + if (!isNodeError(error) || error.code !== "ENOENT") throw error; + return DEFAULT_SKILL_SELECTION; + } + try { + return coerceDocument(JSON.parse(raw)).selection; + } catch { + return DEFAULT_SKILL_SELECTION; + } + }, + + async set(selection: unknown): Promise { + const document = coerceDocument({ selection }); + await persist(document); + return document.selection; + }, + }; +} diff --git a/packages/desktop/src/integrations/skills/skills-commands.ts b/packages/desktop/src/integrations/skills/skills-commands.ts new file mode 100644 index 000000000..82cee1738 --- /dev/null +++ b/packages/desktop/src/integrations/skills/skills-commands.ts @@ -0,0 +1,16 @@ +import type { DesktopCommandHandler } from "../../settings/desktop-settings-commands.js"; +import type { SkillsController } from "./skills-controller.js"; + +export function createSkillsCommandHandlers({ + controller, +}: { + controller: SkillsController; +}): Record { + return { + get_skills_status: () => controller.status(), + install_skills: () => controller.install(), + update_skills: () => controller.update(), + uninstall_skills: () => controller.uninstall(), + save_skills_selection: (args) => controller.save(args), + }; +} diff --git a/packages/desktop/src/integrations/skills/skills-controller.test.ts b/packages/desktop/src/integrations/skills/skills-controller.test.ts new file mode 100644 index 000000000..cd94a18f5 --- /dev/null +++ b/packages/desktop/src/integrations/skills/skills-controller.test.ts @@ -0,0 +1,1204 @@ +import { + chmod, + lstat, + mkdir, + mkdtemp, + readdir, + readFile, + readlink, + rename, + rm, + symlink, + writeFile, +} from "node:fs/promises"; +import { access } from "node:fs/promises"; +import { watch } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import type { SkillSelection, SkillTargets } from "./operations"; +import { createSkillSelectionStore, type SkillSelectionStore } from "./selection-store"; +import { createSkillsController, type SkillsController } from "./skills-controller"; +import { beginSkillsTransaction } from "./skills-transaction"; + +interface Harness { + root: string; + targets: SkillTargets; + controller: SkillsController; + selectionStore: SkillSelectionStore; +} + +const BUNDLED_SKILLS = ["paseo", "paseo-advisor", "paseo-loop"]; + +async function makeHarness(selectionStore?: SkillSelectionStore): Promise { + const root = await mkdtemp(path.join(os.tmpdir(), "paseo-skills-controller-")); + const targets: SkillTargets = { + sourceDir: path.join(root, "bundle"), + agentsDir: path.join(root, "home", ".agents", "skills"), + claudeDir: path.join(root, "home", ".claude", "skills"), + codexDir: path.join(root, "home", ".codex", "skills"), + }; + for (const name of BUNDLED_SKILLS) { + await mkdir(path.join(targets.sourceDir, name), { recursive: true }); + await writeFile(path.join(targets.sourceDir, name, "SKILL.md"), `${name}-v1`); + } + const store = + selectionStore ?? createSkillSelectionStore({ userDataPath: path.join(root, "user-data") }); + return { + root, + targets, + controller: createSkillsController({ + resolveTargets: () => targets, + selectionStore: store, + }), + selectionStore: store, + }; +} + +/** + * The selection store is an injected port, so a store that refuses to persist is + * a fake adapter rather than a mock of the code under test. It is the only way + * to reach the "converged, then could not commit" path deterministically. + */ +function createUnwritableSelectionStore(initial: SkillSelection): SkillSelectionStore { + return { + get: async () => initial, + set: async () => { + throw new Error("selection store is read-only"); + }, + }; +} + +function createGatedUnwritableSelectionStore(initial: SkillSelection): { + store: SkillSelectionStore; + persistenceStarted: Promise; + failPersistence(): void; +} { + let markStarted!: () => void; + let fail!: () => void; + const persistenceStarted = new Promise((resolve) => { + markStarted = resolve; + }); + const failureGate = new Promise((resolve) => { + fail = resolve; + }); + return { + store: { + get: async () => initial, + set: async () => { + markStarted(); + await failureGate; + throw new Error("selection store is read-only"); + }, + }, + persistenceStarted, + failPersistence: fail, + }; +} + +function createGatedSelectionStore(initial: SkillSelection): { + store: SkillSelectionStore; + persistenceStarted: Promise; + finishPersistence(): void; +} { + let current = initial; + let markStarted!: () => void; + let finish!: () => void; + const persistenceStarted = new Promise((resolve) => { + markStarted = resolve; + }); + const gate = new Promise((resolve) => { + finish = resolve; + }); + return { + store: { + get: async () => current, + set: async (selection) => { + markStarted(); + await gate; + current = selection; + }, + }, + persistenceStarted, + finishPersistence: finish, + }; +} + +async function installedSkills(dir: string): Promise { + const entries = await readdir(dir, { withFileTypes: true }).catch(() => []); + return entries + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + .sort(); +} + +async function installedEverywhere(targets: SkillTargets): Promise { + return Promise.all([targets.agentsDir, targets.claudeDir, targets.codexDir].map(installedSkills)); +} + +async function writeUserFile( + targets: SkillTargets, + skill: string, + relativePath: string, + contents: string, +): Promise { + for (const dir of [targets.agentsDir, targets.claudeDir, targets.codexDir]) { + const file = path.join(dir, skill, relativePath); + await mkdir(path.dirname(file), { recursive: true }); + await writeFile(file, contents); + } +} + +async function readUserFile( + targets: SkillTargets, + skill: string, + relativePath: string, +): Promise> { + return Promise.all( + [targets.agentsDir, targets.claudeDir, targets.codexDir].map((dir) => + readFile(path.join(dir, skill, relativePath), "utf8").catch(() => null), + ), + ); +} + +/** Anything the transaction staged and failed to clean up sits beside the skills tree. */ +async function backupArtifacts(targets: SkillTargets): Promise { + return Promise.all( + [targets.agentsDir, targets.claudeDir, targets.codexDir].map(async (dir) => { + const parentEntries = await readdir(path.dirname(dir)).catch(() => []); + const rootEntries = await readdir(dir).catch(() => []); + return [ + ...parentEntries.filter( + (entry) => entry !== path.basename(dir) && !entry.startsWith(".paseo-skills-recovered-"), + ), + ...rootEntries.filter((entry) => entry.startsWith(".paseo-skills-transaction-")), + ].sort(); + }), + ); +} + +async function waitForTransactionDirectory(parent: string): Promise { + const events = watch(parent); + try { + for await (const event of events) { + if (event.filename?.startsWith(".paseo-skills-transaction-")) return; + } + } finally { + await events.return?.(); + } +} + +/** Puts a regular file where the agents skills tree goes, so convergence fails with ENOTDIR. */ +async function blockAgentsDir(targets: SkillTargets): Promise { + await rm(targets.agentsDir, { recursive: true, force: true }); + await mkdir(path.dirname(targets.agentsDir), { recursive: true }); + await writeFile(targets.agentsDir, "not a directory"); +} + +async function isInstalled(targets: SkillTargets, name: string): Promise { + const dirs = [targets.agentsDir, targets.claudeDir, targets.codexDir]; + const present = await Promise.all( + dirs.map((dir) => + access(path.join(dir, name)) + .then(() => true) + .catch(() => false), + ), + ); + return present.every(Boolean); +} + +describe("skills controller", () => { + let harness: Harness; + + beforeEach(async () => { + harness = await makeHarness(); + }); + + afterEach(async () => { + await rm(harness.root, { recursive: true, force: true }); + }); + + it("reports one snapshot with catalog, selection, status, and pending work", async () => { + expect(await harness.controller.status()).toEqual({ + state: "not-installed", + ops: [ + { kind: "add", name: "paseo" }, + { kind: "add", name: "paseo-advisor" }, + { kind: "add", name: "paseo-loop" }, + ], + available: BUNDLED_SKILLS, + installed: [], + selection: { mode: "all" }, + }); + }); + + it("installs every bundled skill while the selection is all", async () => { + expect(await harness.controller.install()).toEqual({ + state: "up-to-date", + ops: [], + available: BUNDLED_SKILLS, + installed: BUNDLED_SKILLS, + selection: { mode: "all" }, + }); + expect(await isInstalled(harness.targets, "paseo-advisor")).toBe(true); + }); + + it("does not remove deselected directories during install", async () => { + await harness.controller.save({ mode: "custom", skills: ["paseo"] }); + await writeUserFile(harness.targets, "paseo-loop", "notes/mine.md", "keep this"); + + await harness.controller.install(); + + expect(await readUserFile(harness.targets, "paseo-loop", "notes/mine.md")).toEqual([ + "keep this", + "keep this", + "keep this", + ]); + }); + + it("saves a custom selection, converges disk, and returns the refreshed snapshot", async () => { + const snapshot = await harness.controller.save({ + mode: "custom", + skills: ["paseo-loop", "paseo"], + }); + + expect(snapshot).toEqual({ + state: "up-to-date", + ops: [], + available: BUNDLED_SKILLS, + installed: ["paseo", "paseo-loop"], + selection: { mode: "custom", skills: ["paseo", "paseo-loop"] }, + confirmationRequired: null, + }); + expect(await isInstalled(harness.targets, "paseo")).toBe(true); + expect(await isInstalled(harness.targets, "paseo-loop")).toBe(true); + expect(await isInstalled(harness.targets, "paseo-advisor")).toBe(false); + }); + + it("removes a skill from disk when it is dropped from the selection", async () => { + await harness.controller.install(); + + await harness.controller.save({ + mode: "custom", + skills: ["paseo"], + confirmedRemovals: ["paseo-advisor", "paseo-loop"], + }); + + expect(await isInstalled(harness.targets, "paseo")).toBe(true); + expect(await isInstalled(harness.targets, "paseo-advisor")).toBe(false); + expect(await isInstalled(harness.targets, "paseo-loop")).toBe(false); + }); + + it("keeps the saved selection after uninstall so a later install restores it", async () => { + await harness.controller.save({ mode: "custom", skills: ["paseo"] }); + + const afterUninstall = await harness.controller.uninstall(); + const afterReinstall = await harness.controller.install(); + + expect(afterUninstall).toEqual({ + state: "not-installed", + ops: [{ kind: "add", name: "paseo" }], + available: BUNDLED_SKILLS, + installed: [], + selection: { mode: "custom", skills: ["paseo"] }, + }); + expect(afterReinstall).toEqual({ + state: "up-to-date", + ops: [], + available: BUNDLED_SKILLS, + installed: ["paseo"], + selection: { mode: "custom", skills: ["paseo"] }, + }); + expect(await isInstalled(harness.targets, "paseo")).toBe(true); + expect(await isInstalled(harness.targets, "paseo-loop")).toBe(false); + }); + + it("treats an empty custom selection as uninstall while keeping the preference", async () => { + await harness.controller.install(); + + const snapshot = await harness.controller.save({ + mode: "custom", + skills: [], + confirmedRemovals: BUNDLED_SKILLS, + }); + + expect(snapshot).toEqual({ + state: "not-installed", + ops: [], + available: BUNDLED_SKILLS, + installed: [], + selection: { mode: "custom", skills: [] }, + confirmationRequired: null, + }); + expect(await isInstalled(harness.targets, "paseo")).toBe(false); + }); + + it("returns to every bundled skill when the selection goes back to all", async () => { + await harness.controller.save({ mode: "custom", skills: ["paseo"] }); + + const snapshot = await harness.controller.save({ mode: "all" }); + + expect(snapshot).toEqual({ + state: "up-to-date", + ops: [], + available: BUNDLED_SKILLS, + installed: BUNDLED_SKILLS, + selection: { mode: "all" }, + confirmationRequired: null, + }); + expect(await isInstalled(harness.targets, "paseo-advisor")).toBe(true); + }); + + it("keeps the previous selection when the save fails to reach disk", async () => { + await harness.controller.save({ mode: "custom", skills: ["paseo"] }); + await blockAgentsDir(harness.targets); + + await expect(harness.controller.save({ mode: "all" })).rejects.toThrow(); + await rm(harness.targets.agentsDir, { force: true }); + + expect(await harness.controller.status()).toEqual({ + state: "drift", + ops: [{ kind: "add", name: "paseo" }], + available: BUNDLED_SKILLS, + installed: ["paseo"], + selection: { mode: "custom", skills: ["paseo"] }, + }); + }); + + it("saves no selection at all when the very first save fails", async () => { + await blockAgentsDir(harness.targets); + + await expect(harness.controller.save({ mode: "custom", skills: ["paseo"] })).rejects.toThrow(); + await rm(harness.targets.agentsDir, { force: true }); + + expect(await harness.controller.status()).toEqual({ + state: "not-installed", + ops: BUNDLED_SKILLS.map((name) => ({ kind: "add", name })), + available: BUNDLED_SKILLS, + installed: [], + selection: { mode: "all" }, + }); + }); + + it("restores deleted directories byte for byte when the selection cannot be committed", async () => { + const store = createUnwritableSelectionStore({ + mode: "custom", + skills: ["paseo", "paseo-loop"], + }); + const readOnly = await makeHarness(store); + await readOnly.controller.install(); + await writeUserFile(readOnly.targets, "paseo-loop", "notes/mine.md", "hand written"); + + // Deselects paseo-loop and adds paseo-advisor, then fails to commit. + await expect( + readOnly.controller.save({ + mode: "custom", + skills: ["paseo", "paseo-advisor"], + confirmedRemovals: ["paseo-loop"], + }), + ).rejects.toThrow("selection store is read-only"); + + expect(await readOnly.controller.status()).toEqual({ + state: "up-to-date", + ops: [], + available: BUNDLED_SKILLS, + installed: ["paseo", "paseo-loop"], + selection: { mode: "custom", skills: ["paseo", "paseo-loop"] }, + }); + expect(await installedEverywhere(readOnly.targets)).toEqual([ + ["paseo", "paseo-loop"], + ["paseo", "paseo-loop"], + ["paseo", "paseo-loop"], + ]); + expect(await readUserFile(readOnly.targets, "paseo-loop", "notes/mine.md")).toEqual([ + "hand written", + "hand written", + "hand written", + ]); + expect(await backupArtifacts(readOnly.targets)).toEqual([[], [], []]); + await rm(readOnly.root, { recursive: true, force: true }); + }); + + it("preserves files added by another writer before rollback", async () => { + const selection: SkillSelection = { mode: "custom", skills: ["paseo"] }; + const gated = createGatedUnwritableSelectionStore(selection); + const readOnly = await makeHarness(gated.store); + await readOnly.controller.install(); + await writeFile(path.join(readOnly.targets.sourceDir, "paseo", "SKILL.md"), "paseo-v2"); + + const save = readOnly.controller.save(selection); + await gated.persistenceStarted; + await writeUserFile(readOnly.targets, "paseo", "notes/concurrent.md", "keep this"); + gated.failPersistence(); + await expect(save).rejects.toThrow("selection store is read-only"); + + expect(await readUserFile(readOnly.targets, "paseo", "notes/concurrent.md")).toEqual([ + "keep this", + "keep this", + "keep this", + ]); + await rm(readOnly.root, { recursive: true, force: true }); + }); + + it("does not automatically delete files preserved from a rolled-back add", async () => { + const previous: SkillSelection = { mode: "custom", skills: ["paseo"] }; + const gated = createGatedUnwritableSelectionStore(previous); + const readOnly = await makeHarness(gated.store); + await readOnly.controller.install(); + + const save = readOnly.controller.save({ + mode: "custom", + skills: ["paseo", "paseo-loop"], + }); + await gated.persistenceStarted; + await writeUserFile(readOnly.targets, "paseo-loop", "notes/concurrent.md", "keep this"); + gated.failPersistence(); + await expect(save).rejects.toThrow("selection store is read-only"); + + await readOnly.controller.autoUpdate(); + expect(await readUserFile(readOnly.targets, "paseo-loop", "notes/concurrent.md")).toEqual([ + "keep this", + "keep this", + "keep this", + ]); + await rm(readOnly.root, { recursive: true, force: true }); + }); + + it("merges a deleted directory backup into files another writer recreated", async () => { + const previous: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-loop"], + }; + const gated = createGatedUnwritableSelectionStore(previous); + const readOnly = await makeHarness(gated.store); + await readOnly.controller.install(); + await writeUserFile(readOnly.targets, "paseo-loop", "notes/before.md", "restore this"); + + const save = readOnly.controller.save({ + mode: "custom", + skills: ["paseo"], + confirmedRemovals: ["paseo-loop"], + }); + await gated.persistenceStarted; + await writeUserFile(readOnly.targets, "paseo-loop", "notes/concurrent.md", "keep this"); + gated.failPersistence(); + await expect(save).rejects.toThrow("selection store is read-only"); + + expect(await readUserFile(readOnly.targets, "paseo-loop", "notes/before.md")).toEqual([ + "restore this", + "restore this", + "restore this", + ]); + expect(await readUserFile(readOnly.targets, "paseo-loop", "notes/concurrent.md")).toEqual([ + "keep this", + "keep this", + "keep this", + ]); + await rm(readOnly.root, { recursive: true, force: true }); + }); + + it("atomically stages a deletion before another writer can recreate its path", async () => { + const previous: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-loop"], + }; + const next: SkillSelection = { mode: "custom", skills: ["paseo"] }; + await harness.controller.save(previous); + + const transaction = await beginSkillsTransaction(harness.targets, previous, next, [ + { kind: "delete", name: "paseo-loop" }, + ]); + + expect(await isInstalled(harness.targets, "paseo-loop")).toBe(false); + await writeUserFile(harness.targets, "paseo-loop", "notes/concurrent.md", "keep this"); + await transaction.rollback(); + expect(await readUserFile(harness.targets, "paseo-loop", "notes/concurrent.md")).toEqual([ + "keep this", + "keep this", + "keep this", + ]); + expect(await isInstalled(harness.targets, "paseo-loop")).toBe(true); + }); + + it.skipIf(process.platform === "win32")( + "renames an untouched staged deletion back with its inode and mode", + async () => { + const previous: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-loop"], + }; + const next: SkillSelection = { mode: "custom", skills: ["paseo"] }; + await harness.controller.save(previous); + const livePaths = [ + harness.targets.agentsDir, + harness.targets.claudeDir, + harness.targets.codexDir, + ].map((root) => path.join(root, "paseo-loop")); + for (const live of livePaths) await chmod(live, 0o700); + const before = await Promise.all(livePaths.map(lstat)); + + const transaction = await beginSkillsTransaction(harness.targets, previous, next, [ + { kind: "delete", name: "paseo-loop" }, + ]); + await transaction.rollback(); + + const after = await Promise.all(livePaths.map(lstat)); + expect(after.map((entry) => entry.ino)).toEqual(before.map((entry) => entry.ino)); + expect(after.map((entry) => entry.mode & 0o777)).toEqual([0o700, 0o700, 0o700]); + }, + ); + + it("finishes rollback when an external deletion leaves no live or staged path", async () => { + const previous: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-loop"], + }; + const next: SkillSelection = { mode: "custom", skills: ["paseo"] }; + await harness.controller.save(previous); + + const transaction = await beginSkillsTransaction(harness.targets, previous, next, [ + { kind: "delete", name: "paseo-loop" }, + ]); + const codexStage = (await readdir(harness.targets.codexDir)).find((entry) => + entry.startsWith(".paseo-skills-transaction-"), + ); + expect(codexStage).toBeDefined(); + await rm(path.join(harness.targets.codexDir, codexStage!, "paseo-loop"), { + recursive: true, + force: true, + }); + + await transaction.rollback(); + + expect(await readUserFile(harness.targets, "paseo-loop", "SKILL.md")).toEqual([ + "paseo-loop-v1", + "paseo-loop-v1", + null, + ]); + expect(await backupArtifacts(harness.targets)).toEqual([[], [], []]); + await expect(harness.controller.status()).resolves.toMatchObject({ selection: previous }); + }); + + it("quarantines a staged directory when an external file takes its live path", async () => { + const previous: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-loop"], + }; + const next: SkillSelection = { mode: "custom", skills: ["paseo"] }; + await harness.controller.save(previous); + await writeUserFile(harness.targets, "paseo-loop", "notes/mine.md", "keep this"); + + const transaction = await beginSkillsTransaction(harness.targets, previous, next, [ + { kind: "delete", name: "paseo-loop" }, + ]); + const live = path.join(harness.targets.codexDir, "paseo-loop"); + await writeFile(live, "external replacement"); + + await transaction.rollback(); + + expect(await readFile(live, "utf8")).toBe("external replacement"); + const recovered = (await readdir(harness.targets.codexDir)).find((entry) => + entry.startsWith(".paseo-skills-recovered-"), + ); + expect(recovered).toBeDefined(); + expect( + await readFile(path.join(harness.targets.codexDir, recovered!, "notes", "mine.md"), "utf8"), + ).toBe("keep this"); + expect(await readUserFile(harness.targets, "paseo-loop", "notes/mine.md")).toEqual([ + "keep this", + "keep this", + null, + ]); + expect(await backupArtifacts(harness.targets)).toEqual([[], [], []]); + await expect(harness.controller.status()).resolves.toMatchObject({ selection: previous }); + }); + + it("quarantines staged files that collide with a recreated directory", async () => { + const previous: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-loop"], + }; + const next: SkillSelection = { mode: "custom", skills: ["paseo"] }; + await harness.controller.save(previous); + await writeUserFile(harness.targets, "paseo-loop", "notes/mine.md", "staged notes"); + + const transaction = await beginSkillsTransaction(harness.targets, previous, next, [ + { kind: "delete", name: "paseo-loop" }, + ]); + const live = path.join(harness.targets.codexDir, "paseo-loop"); + await mkdir(path.join(live, "notes"), { recursive: true }); + await writeFile(path.join(live, "SKILL.md"), "external skill"); + await writeFile(path.join(live, "notes", "mine.md"), "external notes"); + + await transaction.rollback(); + + expect(await readFile(path.join(live, "SKILL.md"), "utf8")).toBe("external skill"); + expect(await readFile(path.join(live, "notes", "mine.md"), "utf8")).toBe("external notes"); + const recovered = (await readdir(harness.targets.codexDir)).find((entry) => + entry.startsWith(".paseo-skills-recovered-paseo-loop-"), + ); + expect(recovered).toBeDefined(); + expect( + await readFile(path.join(harness.targets.codexDir, recovered!, "SKILL.md"), "utf8"), + ).toBe("paseo-loop-v1"); + expect( + await readFile(path.join(harness.targets.codexDir, recovered!, "notes", "mine.md"), "utf8"), + ).toBe("staged notes"); + expect(await backupArtifacts(harness.targets)).toEqual([[], [], []]); + await expect(harness.controller.status()).resolves.toMatchObject({ selection: previous }); + }); + + it("preserves incompatible live paths while rolling back adds and updates", async () => { + const previous: SkillSelection = { mode: "custom", skills: ["paseo"] }; + const next: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-advisor"], + }; + await harness.controller.save(previous); + await writeUserFile(harness.targets, "paseo", "notes/mine.md", "keep this"); + + const transaction = await beginSkillsTransaction(harness.targets, previous, next, [ + { kind: "update", name: "paseo" }, + { kind: "add", name: "paseo-advisor" }, + ]); + const replacedUpdate = path.join(harness.targets.agentsDir, "paseo"); + await rm(replacedUpdate, { recursive: true, force: true }); + await writeFile(replacedUpdate, "external update replacement"); + const replacedAdd = path.join(harness.targets.codexDir, "paseo-advisor"); + await writeFile(replacedAdd, "external add replacement"); + + await transaction.rollback(); + + expect(await readFile(replacedUpdate, "utf8")).toBe("external update replacement"); + expect(await readFile(replacedAdd, "utf8")).toBe("external add replacement"); + const recoveryParent = path.dirname(harness.targets.agentsDir); + const recovered = (await readdir(recoveryParent)).find((entry) => + entry.startsWith(".paseo-skills-recovered-paseo-"), + ); + expect(recovered).toBeDefined(); + expect(await readFile(path.join(recoveryParent, recovered!, "notes", "mine.md"), "utf8")).toBe( + "keep this", + ); + expect(await readUserFile(harness.targets, "paseo", "notes/mine.md")).toEqual([ + null, + "keep this", + "keep this", + ]); + expect(await backupArtifacts(harness.targets)).toEqual([[], [], []]); + await expect(harness.controller.status()).resolves.toMatchObject({ selection: previous }); + }); + + it("recovers after an update backup was quarantined before transaction cleanup", async () => { + const previous: SkillSelection = { mode: "custom", skills: ["paseo"] }; + const next: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-advisor"], + }; + await harness.controller.save(previous); + await writeUserFile(harness.targets, "paseo", "notes/mine.md", "captured notes"); + + await beginSkillsTransaction(harness.targets, previous, next, [ + { kind: "update", name: "paseo" }, + ]); + const transactionParent = path.dirname(harness.targets.agentsDir); + const transactionName = (await readdir(transactionParent)).find((entry) => + entry.startsWith(".paseo-skills-transaction-"), + ); + expect(transactionName).toBeDefined(); + const transactionDir = path.join(transactionParent, transactionName!); + const manifest = JSON.parse( + await readFile(path.join(transactionDir, "transaction.json"), "utf8"), + ) as { entries: Array<{ livePath: string; backupPath: string | null }> }; + const entry = manifest.entries.find( + (candidate) => candidate.livePath === path.join(harness.targets.agentsDir, "paseo"), + ); + expect(entry?.backupPath).toBeTruthy(); + const backup = path.join(transactionDir, entry!.backupPath!); + const recovered = path.join( + transactionParent, + `.paseo-skills-recovered-paseo-${transactionName!.replace(".paseo-skills-transaction-", "")}`, + ); + await rm(entry!.livePath, { recursive: true, force: true }); + await writeFile(entry!.livePath, "external replacement"); + await rename(backup, recovered); + + await expect(harness.controller.status()).resolves.toMatchObject({ selection: previous }); + + expect(await readFile(entry!.livePath, "utf8")).toBe("external replacement"); + expect(await readFile(path.join(recovered, "notes", "mine.md"), "utf8")).toBe("captured notes"); + expect(await backupArtifacts(harness.targets)).toEqual([[], [], []]); + }); + + it("preserves a directory that replaces a captured file before recovery", async () => { + const previous: SkillSelection = { mode: "custom", skills: ["paseo"] }; + const next: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-advisor"], + }; + await harness.controller.save(previous); + const live = path.join(harness.targets.agentsDir, "paseo"); + await rm(live, { recursive: true, force: true }); + await writeFile(live, "captured file"); + + await beginSkillsTransaction(harness.targets, previous, next, [ + { kind: "update", name: "paseo" }, + ]); + await rm(live, { force: true }); + await mkdir(live, { recursive: true }); + await writeFile(path.join(live, "external.md"), "external directory"); + + await expect(harness.controller.status()).resolves.toMatchObject({ selection: previous }); + + expect(await readFile(path.join(live, "external.md"), "utf8")).toBe("external directory"); + const recovered = (await readdir(path.dirname(harness.targets.agentsDir))).find((entry) => + entry.startsWith(".paseo-skills-recovered-paseo-"), + ); + expect(recovered).toBeDefined(); + expect( + await readFile(path.join(path.dirname(harness.targets.agentsDir), recovered!), "utf8"), + ).toBe("captured file"); + expect(await backupArtifacts(harness.targets)).toEqual([[], [], []]); + }); + + it.skipIf(process.platform !== "linux")( + "stages deletions when an agent skills root is on another filesystem", + async () => { + const crossFilesystemRoot = await mkdtemp("/dev/shm/paseo-skills-controller-"); + try { + harness.targets.claudeDir = path.join(crossFilesystemRoot, "skills"); + const previous: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-loop"], + }; + const next: SkillSelection = { mode: "custom", skills: ["paseo"] }; + await harness.controller.save(previous); + + expect((await lstat(harness.targets.agentsDir)).dev).not.toBe( + (await lstat(harness.targets.claudeDir)).dev, + ); + const transaction = await beginSkillsTransaction(harness.targets, previous, next, [ + { kind: "delete", name: "paseo-loop" }, + ]); + + expect(await isInstalled(harness.targets, "paseo-loop")).toBe(false); + await transaction.rollback(); + expect(await isInstalled(harness.targets, "paseo-loop")).toBe(true); + } finally { + await rm(crossFilesystemRoot, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(process.platform === "win32")( + "restores a relative symlink used as the skill directory", + async () => { + const previous: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-loop"], + }; + const next: SkillSelection = { mode: "custom", skills: ["paseo"] }; + await harness.controller.save(previous); + const shared = path.join(harness.root, "home", "shared", "paseo-loop"); + await mkdir(shared, { recursive: true }); + await writeFile(path.join(shared, "SKILL.md"), "shared target"); + const live = path.join(harness.targets.claudeDir, "paseo-loop"); + await rm(live, { recursive: true, force: true }); + await symlink(path.relative(harness.targets.claudeDir, shared), live, "dir"); + + const transaction = await beginSkillsTransaction(harness.targets, previous, next, [ + { kind: "delete", name: "paseo-loop" }, + ]); + await transaction.rollback(); + + expect((await lstat(live)).isSymbolicLink()).toBe(true); + expect(await readFile(path.join(live, "SKILL.md"), "utf8")).toBe("shared target"); + }, + ); + + it.skipIf(process.platform === "win32")( + "restores updates made through a relative skill-directory symlink", + async () => { + const selection: SkillSelection = { mode: "custom", skills: ["paseo"] }; + const gated = createGatedUnwritableSelectionStore(selection); + const readOnly = await makeHarness(gated.store); + await readOnly.controller.install(); + const shared = path.join(readOnly.root, "home", "shared", "paseo"); + await mkdir(shared, { recursive: true }); + await writeFile(path.join(shared, "SKILL.md"), "paseo-v1"); + const live = path.join(readOnly.targets.claudeDir, "paseo"); + await rm(live, { recursive: true, force: true }); + await symlink(path.relative(readOnly.targets.claudeDir, shared), live, "dir"); + await writeFile(path.join(readOnly.targets.sourceDir, "paseo", "SKILL.md"), "paseo-v2"); + + const save = readOnly.controller.save(selection); + await gated.persistenceStarted; + gated.failPersistence(); + await expect(save).rejects.toThrow("selection store is read-only"); + + expect((await lstat(live)).isSymbolicLink()).toBe(true); + expect(await readFile(path.join(shared, "SKILL.md"), "utf8")).toBe("paseo-v1"); + await rm(readOnly.root, { recursive: true, force: true }); + }, + ); + + it.skipIf(process.platform === "win32")( + "stages one deletion when two agent roots alias the same directory", + async () => { + await harness.controller.install(); + await rm(harness.targets.claudeDir, { recursive: true, force: true }); + await symlink( + path.relative(path.dirname(harness.targets.claudeDir), harness.targets.agentsDir), + harness.targets.claudeDir, + "dir", + ); + + const result = await harness.controller.save({ + mode: "custom", + skills: ["paseo", "paseo-advisor"], + confirmedRemovals: ["paseo-loop"], + }); + + expect(result.confirmationRequired).toBeNull(); + expect(await isInstalled(harness.targets, "paseo-loop")).toBe(false); + }, + ); + + it.skipIf(process.platform === "win32")( + "restores a user-added symlink when deletion rolls back", + async () => { + const previous: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-loop"], + }; + const gated = createGatedUnwritableSelectionStore(previous); + const readOnly = await makeHarness(gated.store); + await readOnly.controller.install(); + for (const root of [ + readOnly.targets.agentsDir, + readOnly.targets.claudeDir, + readOnly.targets.codexDir, + ]) { + const notes = path.join(root, "paseo-loop", "notes"); + await mkdir(notes, { recursive: true }); + await writeFile(path.join(notes, "before.md"), "target"); + await symlink("before.md", path.join(notes, "latest.md")); + } + + const save = readOnly.controller.save({ + mode: "custom", + skills: ["paseo"], + confirmedRemovals: ["paseo-loop"], + }); + await gated.persistenceStarted; + gated.failPersistence(); + await expect(save).rejects.toThrow("selection store is read-only"); + + for (const root of [ + readOnly.targets.agentsDir, + readOnly.targets.claudeDir, + readOnly.targets.codexDir, + ]) { + const restored = path.join(root, "paseo-loop", "notes", "latest.md"); + expect((await lstat(restored)).isSymbolicLink()).toBe(true); + expect(await readlink(restored)).toBe("before.md"); + } + await rm(readOnly.root, { recursive: true, force: true }); + }, + ); + + it.skipIf(process.platform === "win32")( + "restores executable permissions when deletion rolls back", + async () => { + const previous: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-loop"], + }; + const gated = createGatedUnwritableSelectionStore(previous); + const readOnly = await makeHarness(gated.store); + await readOnly.controller.install(); + await writeUserFile(readOnly.targets, "paseo-loop", "hooks/run.sh", "#!/bin/sh\n"); + for (const root of [ + readOnly.targets.agentsDir, + readOnly.targets.claudeDir, + readOnly.targets.codexDir, + ]) { + await chmod(path.join(root, "paseo-loop", "hooks", "run.sh"), 0o751); + } + + const save = readOnly.controller.save({ + mode: "custom", + skills: ["paseo"], + confirmedRemovals: ["paseo-loop"], + }); + await gated.persistenceStarted; + gated.failPersistence(); + await expect(save).rejects.toThrow("selection store is read-only"); + + for (const root of [ + readOnly.targets.agentsDir, + readOnly.targets.claudeDir, + readOnly.targets.codexDir, + ]) { + const restored = await lstat(path.join(root, "paseo-loop", "hooks", "run.sh")); + expect(restored.mode & 0o777).toBe(0o751); + } + await rm(readOnly.root, { recursive: true, force: true }); + }, + ); + + it("leaves no backup artifacts behind after a successful save", async () => { + await harness.controller.save({ mode: "custom", skills: ["paseo"] }); + + expect(await backupArtifacts(harness.targets)).toEqual([[], [], []]); + }); + + it.skipIf(process.platform === "win32")( + "reports a committed save as successful when cleanup is deferred", + async () => { + const gated = createGatedSelectionStore({ mode: "all" }); + const blocked = await makeHarness(gated.store); + const next: SkillSelection = { mode: "custom", skills: ["paseo"] }; + const parent = path.dirname(blocked.targets.agentsDir); + const movedParent = `${parent}-moved`; + + const save = blocked.controller.save(next); + await gated.persistenceStarted; + await rename(parent, movedParent); + await writeFile(parent, "block transaction cleanup with ENOTDIR"); + gated.finishPersistence(); + const outcome = await save.then( + () => ({ ok: true as const }), + (error: unknown) => ({ ok: false as const, error }), + ); + await rm(parent, { force: true }); + await rename(movedParent, parent); + + expect(outcome.ok).toBe(true); + expect(await gated.store.get()).toEqual(next); + await blocked.controller.status(); + expect(await backupArtifacts(blocked.targets)).toEqual([[], [], []]); + await rm(blocked.root, { recursive: true, force: true }); + }, + ); + + it("does not delete an unrelated file that resembles transaction staging", async () => { + const unrelated = path.join( + path.dirname(harness.targets.agentsDir), + ".paseo-skills-transaction-my-notes", + ); + await mkdir(unrelated, { recursive: true }); + await writeFile(path.join(unrelated, "mine.md"), "keep me"); + + await harness.controller.save({ mode: "custom", skills: ["paseo"] }); + + expect(await readFile(path.join(unrelated, "mine.md"), "utf8")).toBe("keep me"); + }); + + it("recovers an interrupted save before the next controller operation", async () => { + const previous: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-loop"], + }; + const next: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-advisor"], + }; + await harness.controller.save(previous); + await writeUserFile(harness.targets, "paseo-loop", "notes/mine.md", "hand written"); + await beginSkillsTransaction(harness.targets, previous, next, [ + { kind: "delete", name: "paseo-loop" }, + ]); + for (const root of [ + harness.targets.agentsDir, + harness.targets.claudeDir, + harness.targets.codexDir, + ]) { + await rm(path.join(root, "paseo-loop"), { recursive: true, force: true }); + } + + await harness.controller.status(); + + expect(await readUserFile(harness.targets, "paseo-loop", "notes/mine.md")).toEqual([ + "hand written", + "hand written", + "hand written", + ]); + expect(await backupArtifacts(harness.targets)).toEqual([[], [], []]); + }); + + it("does not roll back an interrupted transaction after the selection committed", async () => { + const previous: SkillSelection = { + mode: "custom", + skills: ["paseo", "paseo-loop"], + }; + const next: SkillSelection = { + mode: "custom", + skills: ["paseo"], + }; + await harness.controller.save(previous); + await beginSkillsTransaction(harness.targets, previous, next, [ + { kind: "delete", name: "paseo-loop" }, + ]); + for (const root of [ + harness.targets.agentsDir, + harness.targets.claudeDir, + harness.targets.codexDir, + ]) { + await rm(path.join(root, "paseo-loop"), { recursive: true, force: true }); + } + await harness.selectionStore.set(next); + + const snapshot = await harness.controller.status(); + + expect(snapshot.selection).toEqual(next); + expect(await isInstalled(harness.targets, "paseo-loop")).toBe(false); + expect(await backupArtifacts(harness.targets)).toEqual([[], [], []]); + }); + + it("asks for confirmation naming the directories a save would delete", async () => { + await harness.controller.save({ mode: "custom", skills: ["paseo", "paseo-loop"] }); + // Something puts a managed directory back after the UI took its snapshot. + await writeUserFile(harness.targets, "paseo-advisor", "SKILL.md", "external"); + + const result = await harness.controller.save({ + mode: "custom", + skills: ["paseo", "paseo-loop"], + }); + + expect(result.confirmationRequired).toEqual({ removals: ["paseo-advisor"] }); + expect(await installedEverywhere(harness.targets)).toEqual([ + ["paseo", "paseo-advisor", "paseo-loop"], + ["paseo", "paseo-advisor", "paseo-loop"], + ["paseo", "paseo-advisor", "paseo-loop"], + ]); + expect(result.selection).toEqual({ mode: "custom", skills: ["paseo", "paseo-loop"] }); + }); + + it("applies the save once the removals are confirmed", async () => { + await harness.controller.install(); + + const result = await harness.controller.save({ + mode: "custom", + skills: ["paseo"], + confirmedRemovals: ["paseo-advisor", "paseo-loop"], + }); + + expect(result.confirmationRequired).toBeNull(); + expect(result.selection).toEqual({ mode: "custom", skills: ["paseo"] }); + expect(await installedEverywhere(harness.targets)).toEqual([["paseo"], ["paseo"], ["paseo"]]); + }); + + it("asks again when another directory appears before the retry", async () => { + await harness.controller.install(); + await writeUserFile(harness.targets, "paseo-chat", "SKILL.md", "retired but present"); + + const result = await harness.controller.save({ + mode: "custom", + skills: ["paseo"], + confirmedRemovals: ["paseo-advisor", "paseo-loop"], + }); + + expect(result.confirmationRequired).toEqual({ + removals: ["paseo-advisor", "paseo-chat", "paseo-loop"], + }); + expect(await installedEverywhere(harness.targets)).toEqual([ + ["paseo", "paseo-advisor", "paseo-chat", "paseo-loop"], + ["paseo", "paseo-advisor", "paseo-chat", "paseo-loop"], + ["paseo", "paseo-advisor", "paseo-chat", "paseo-loop"], + ]); + }); + + it("does not commit when a new removal appears while the frozen plan is applying", async () => { + const selection: SkillSelection = { mode: "custom", skills: ["paseo"] }; + await harness.controller.save(selection); + await writeFile(path.join(harness.targets.sourceDir, "paseo", "SKILL.md"), "paseo-v2"); + + const transactionStarted = waitForTransactionDirectory(path.dirname(harness.targets.agentsDir)); + const save = harness.controller.save(selection); + await transactionStarted; + await writeUserFile(harness.targets, "paseo-chat", "notes/mine.md", "hand written"); + + const result = await save; + + expect(result.confirmationRequired).toEqual({ removals: ["paseo-chat"] }); + expect(result.selection).toEqual(selection); + expect(await readUserFile(harness.targets, "paseo-chat", "notes/mine.md")).toEqual([ + "hand written", + "hand written", + "hand written", + ]); + }); + + it("saves without asking when nothing would be deleted", async () => { + const result = await harness.controller.save({ mode: "custom", skills: ["paseo"] }); + + expect(result.confirmationRequired).toBeNull(); + expect(await installedEverywhere(harness.targets)).toEqual([["paseo"], ["paseo"], ["paseo"]]); + }); + + it("preserves a regular file at a skill path when save convergence fails", async () => { + await mkdir(harness.targets.agentsDir, { recursive: true }); + const collision = path.join(harness.targets.agentsDir, "paseo"); + await writeFile(collision, "keep this file"); + + await expect(harness.controller.save({ mode: "custom", skills: ["paseo"] })).rejects.toThrow(); + + expect(await readFile(collision, "utf8")).toBe("keep this file"); + }); + + it("serializes startup convergence with an interactive save", async () => { + await harness.controller.install(); + // Startup finds drift it wants to repair while the user narrows the + // selection. Whichever runs first, disk must end up matching what is saved. + await rm(path.join(harness.targets.claudeDir, "paseo-loop"), { + recursive: true, + force: true, + }); + + const [, saved] = await Promise.all([ + harness.controller.autoUpdate(), + harness.controller.save({ + mode: "custom", + skills: ["paseo"], + confirmedRemovals: ["paseo-advisor", "paseo-loop"], + }), + ]); + + expect(saved.selection).toEqual({ mode: "custom", skills: ["paseo"] }); + expect(await installedEverywhere(harness.targets)).toEqual([["paseo"], ["paseo"], ["paseo"]]); + expect(await harness.controller.status()).toEqual({ + state: "up-to-date", + ops: [], + available: BUNDLED_SKILLS, + installed: ["paseo"], + selection: { mode: "custom", skills: ["paseo"] }, + }); + }); + + it("updates a drifted install without touching the saved selection", async () => { + await harness.controller.save({ mode: "custom", skills: ["paseo"] }); + await writeFile(path.join(harness.targets.agentsDir, "paseo", "SKILL.md"), "stale"); + + expect(await harness.controller.update()).toEqual({ + state: "up-to-date", + ops: [], + available: BUNDLED_SKILLS, + installed: ["paseo"], + selection: { mode: "custom", skills: ["paseo"] }, + }); + }); + + it("does not remove deselected directories during a manual update", async () => { + await harness.controller.save({ mode: "custom", skills: ["paseo"] }); + await writeUserFile(harness.targets, "paseo-loop", "notes/mine.md", "keep this"); + + await harness.controller.update(); + + expect(await readUserFile(harness.targets, "paseo-loop", "notes/mine.md")).toEqual([ + "keep this", + "keep this", + "keep this", + ]); + }); +}); diff --git a/packages/desktop/src/integrations/skills/skills-controller.ts b/packages/desktop/src/integrations/skills/skills-controller.ts new file mode 100644 index 000000000..7c627bc8d --- /dev/null +++ b/packages/desktop/src/integrations/skills/skills-controller.ts @@ -0,0 +1,160 @@ +import { + autoUpdateInstalledSkills, + getSkillsStatus, + installSkills, + type SkillSelection, + type SkillsStatus, + type SkillTargets, + uninstallSkills, + updateSkills, +} from "./operations.js"; +import { + coerceSkillNames, + coerceSkillSelection, + type SkillSelectionStore, +} from "./selection-store.js"; +import { + beginSkillsTransaction, + recoverInterruptedSkillTransactions, +} from "./skills-transaction.js"; + +/** Everything the settings UI needs to render skills in one round trip. */ +export interface SkillsSnapshot extends SkillsStatus { + selection: SkillSelection; +} + +export interface SkillsSaveResult extends SkillsSnapshot { + /** + * Non-null when the save did nothing because these directories would be + * deleted and the request had not confirmed them. Retry with the names. + */ + confirmationRequired: { removals: string[] } | null; +} + +export interface SkillsController { + status(): Promise; + install(): Promise; + update(): Promise; + uninstall(): Promise; + autoUpdate(): Promise; + save(request: unknown): Promise; +} + +type Converge = (targets: SkillTargets, selection: SkillSelection) => Promise; +const MAX_SAVE_ATTEMPTS = 3; + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** + * The single writer for installed skills. Startup convergence and every settings + * command run through one queue, so no operation applies a plan computed from a + * selection another one has already replaced, and no read sees a half-applied + * directory. Ordering between concurrent callers is not promised — consistency + * after both finish is. + */ +export function createSkillsController({ + resolveTargets, + selectionStore, +}: { + // Resolved per operation, not at wiring time: the bundle path depends on how + // the app was packaged, which is not knowable when the controller is built. + resolveTargets: () => SkillTargets; + selectionStore: SkillSelectionStore; +}): SkillsController { + let queue: Promise = Promise.resolve(); + + function serialize(run: () => Promise): Promise { + const next = queue.then(run, run); + queue = next.then( + () => undefined, + () => undefined, + ); + return next; + } + + async function converge(apply: Converge): Promise { + const selection = await selectionStore.get(); + await recoverInterruptedSkillTransactions(resolveTargets(), selection); + return { ...(await apply(resolveTargets(), selection)), selection }; + } + + async function saveSelection(request: unknown): Promise { + const targets = resolveTargets(); + const next = coerceSkillSelection(request); + const previous = await selectionStore.get(); + await recoverInterruptedSkillTransactions(targets, previous); + const confirmed = new Set( + coerceSkillNames(isRecord(request) ? request.confirmedRemovals : null), + ); + + // The plan comes from a scan taken here, inside the queue, and is the exact + // plan applied below. Deciding what will be deleted from an older snapshot + // leaves a window for a directory to appear and be deleted unannounced. + let plan = await getSkillsStatus(targets, next); + for (let attempt = 0; attempt < MAX_SAVE_ATTEMPTS; attempt += 1) { + const removals = plan.ops.filter((op) => op.kind === "delete").map((op) => op.name); + if (removals.some((name) => !confirmed.has(name))) { + return { + ...(await getSkillsStatus(targets, previous)), + selection: previous, + confirmationRequired: { removals }, + }; + } + + // Capture only paths this frozen plan can mutate. Capturing every managed + // path would make rollback delete a new external directory that appeared + // after the scan merely because it was absent from the snapshot. + const transaction = await beginSkillsTransaction(targets, previous, next, plan.ops); + let status: SkillsStatus; + try { + status = await installSkills(targets, next, { + ...plan, + // beginSkillsTransaction atomically staged these directories. Running + // removeSkill afterward would delete a path another writer recreated. + ops: plan.ops.filter((op) => op.kind !== "delete"), + }); + } catch (error) { + await transaction.rollback(); + throw error; + } + + // An empty custom selection converges to the intentional not-installed + // state; zero remaining operations is the invariant, not the label. + if (status.ops.length === 0) { + try { + await selectionStore.set(next); + } catch (error) { + await transaction.rollback(); + throw error; + } + try { + await transaction.commit(); + } catch { + // Selection and disk convergence are already durable. The owned + // manifest stays behind so the next serialized operation retries + // cleanup; reporting this save as failed would misstate its outcome. + } + return { ...status, selection: next, confirmationRequired: null }; + } + + // The frozen plan became incomplete while it was being applied. Put only + // its mutations back, rescan, and either request confirmation for a new + // deletion or retry harmless additions/updates from the fresh plan. + await transaction.rollback(); + plan = await getSkillsStatus(targets, next); + } + + throw new Error("Skills changed repeatedly while the selection was being saved"); + } + + return { + status: () => serialize(() => converge(getSkillsStatus)), + install: () => serialize(() => converge(updateSkills)), + update: () => serialize(() => converge(updateSkills)), + uninstall: () => serialize(() => converge(uninstallSkills)), + autoUpdate: () => serialize(() => converge(autoUpdateInstalledSkills)), + save: (selection) => serialize(() => saveSelection(selection)), + }; +} diff --git a/packages/desktop/src/integrations/skills/skills-transaction.ts b/packages/desktop/src/integrations/skills/skills-transaction.ts new file mode 100644 index 000000000..cb4203917 --- /dev/null +++ b/packages/desktop/src/integrations/skills/skills-transaction.ts @@ -0,0 +1,607 @@ +import { createHash, randomUUID } from "node:crypto"; +import { + cp, + lstat, + mkdir, + readdir, + readFile, + readlink, + realpath, + rename, + rm, + stat, + writeFile, +} from "node:fs/promises"; +import path from "node:path"; + +import { + listManagedSkillNames, + type SkillOp, + type SkillSelection, + type SkillTargets, +} from "./operations.js"; +import { listFilesRecursive } from "./sync.js"; + +export interface SkillsTransaction { + /** Convergence stuck. Drop the staged copies. */ + commit(): Promise; + /** Put every captured directory back exactly as it was, then drop the staging. */ + rollback(): Promise; +} + +/** + * Written before anything is touched and updated once capture finishes. A + * directory without a readable one of these is not ours and is never deleted — + * the prefix alone is a guess, and guessing here deletes someone's files. + */ +interface TransactionManifest { + owner: typeof MANIFEST_OWNER; + version: 1; + /** `capturing` means convergence never started, so there is nothing to undo. */ + phase: "capturing" | "converging"; + previousSelection: SkillSelection; + nextSelection: SkillSelection; + entries: CapturedDirectory[]; +} + +interface CapturedDirectory { + livePath: string; + kind: SkillOp["kind"]; + /** Relative to the manifest directory, or an absolute same-filesystem delete stage. */ + backupPath: string | null; +} + +const MANIFEST_OWNER = "paseo-skills-transaction"; +const MANIFEST_FILENAME = "transaction.json"; +const TRANSACTION_PREFIX = ".paseo-skills-transaction-"; +const RECOVERED_PREFIX = ".paseo-skills-recovered-"; +const BACKUP_DIRNAME = "backup"; +const MANAGED_FILES_MANIFEST = ".paseo-managed-files.json"; + +async function isDirectory(target: string): Promise { + const info = await stat(target).catch(() => null); + return info?.isDirectory() === true; +} + +function isSkillSelection(value: unknown): value is SkillSelection { + if (typeof value !== "object" || value === null) return false; + const selection = value as Partial; + if (selection.mode === "all") return true; + return ( + selection.mode === "custom" && + Array.isArray(selection.skills) && + selection.skills.every((name) => typeof name === "string") + ); +} + +async function readManifest(transactionDir: string): Promise { + const raw = await readFile(path.join(transactionDir, MANIFEST_FILENAME), "utf8").catch( + () => null, + ); + if (raw === null) return null; + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return null; + } + if (typeof parsed !== "object" || parsed === null) return null; + const manifest = parsed as Partial; + if (manifest.owner !== MANIFEST_OWNER || manifest.version !== 1) return null; + if (manifest.phase !== "capturing" && manifest.phase !== "converging") return null; + if (!isSkillSelection(manifest.previousSelection) || !isSkillSelection(manifest.nextSelection)) { + return null; + } + if (!Array.isArray(manifest.entries)) return null; + const validEntries = manifest.entries.every( + (entry) => + typeof entry?.livePath === "string" && + (entry.kind === "add" || entry.kind === "update" || entry.kind === "delete") && + (entry.backupPath === null || typeof entry.backupPath === "string"), + ); + if (!validEntries) return null; + const entries = manifest.entries as CapturedDirectory[]; + return { + owner: MANIFEST_OWNER, + version: 1, + phase: manifest.phase, + previousSelection: manifest.previousSelection, + nextSelection: manifest.nextSelection, + entries, + }; +} + +async function writeManifest(transactionDir: string, manifest: TransactionManifest): Promise { + await writeFile( + path.join(transactionDir, MANIFEST_FILENAME), + `${JSON.stringify(manifest, null, 2)}\n`, + "utf8", + ); +} + +function selectionsEqual(left: SkillSelection, right: SkillSelection): boolean { + if (left.mode !== right.mode) return false; + if (left.mode === "all" || right.mode === "all") return true; + return ( + left.skills.length === right.skills.length && + left.skills.every((name, index) => name === right.skills[index]) + ); +} + +async function validateEntries( + targets: SkillTargets, + transactionDir: string, + entries: CapturedDirectory[], +): Promise { + const names = new Set(await listManagedSkillNames(targets.sourceDir)); + const roots = [targets.agentsDir, targets.claudeDir, targets.codexDir]; + return entries.every((entry) => { + const rootIndex = roots.findIndex((root) => path.dirname(entry.livePath) === root); + if (rootIndex === -1) return false; + const name = path.basename(entry.livePath); + if (!names.has(name)) return false; + if (entry.backupPath === null) return true; + const expected = path.join(BACKUP_DIRNAME, String(rootIndex), name); + const sameFilesystemStage = path.join(roots[rootIndex]!, path.basename(transactionDir), name); + if (entry.kind === "delete" && entry.backupPath === sameFilesystemStage) return true; + return ( + entry.backupPath === expected && + path + .resolve(transactionDir, entry.backupPath) + .startsWith(`${path.resolve(transactionDir)}${path.sep}`) + ); + }); +} + +function resolveBackupPath(transactionDir: string, backupPath: string | null): string | null { + if (backupPath === null) return null; + return path.isAbsolute(backupPath) ? backupPath : path.join(transactionDir, backupPath); +} + +async function discardTransaction( + transactionDir: string, + entries: readonly CapturedDirectory[], +): Promise { + const stagingDirs = new Set( + entries.flatMap((entry) => + entry.backupPath !== null && path.isAbsolute(entry.backupPath) + ? [path.dirname(entry.backupPath)] + : [], + ), + ); + for (const stagingDir of stagingDirs) { + await rm(stagingDir, { recursive: true, force: true }); + } + await rm(transactionDir, { recursive: true, force: true }); +} + +async function listAllFiles(rootDir: string): Promise { + const files: string[] = []; + async function walk(dir: string): Promise { + const entries = await readdir(dir, { withFileTypes: true }).catch(() => []); + for (const entry of entries) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) await walk(full); + else if (entry.isFile()) files.push(path.relative(rootDir, full)); + } + } + await walk(rootDir); + return files; +} + +async function expectedSyncedFiles(sourceDir: string, name: string): Promise> { + const skillDir = path.join(sourceDir, name); + const files = new Map(); + const hashes: Record = {}; + for (const rel of await listFilesRecursive(skillDir)) { + const contents = await readFile(path.join(skillDir, rel)); + files.set(rel, contents); + hashes[rel] = createHash("sha256").update(contents).digest("hex"); + } + files.set( + MANAGED_FILES_MANIFEST, + Buffer.from(`${JSON.stringify({ version: 1, files: hashes }, null, 2)}\n`), + ); + return files; +} + +async function copyStagedEntry(source: string, target: string): Promise { + const info = await lstat(source); + await mkdir(path.dirname(target), { recursive: true }); + await cp(source, target, { + recursive: info.isDirectory(), + preserveTimestamps: true, + verbatimSymlinks: true, + }); +} + +async function mergeBackupDirectory(livePath: string, backupPath: string | null): Promise { + if (backupPath === null) return false; + const liveInfo = await lstat(livePath).catch(() => null); + const destination = liveInfo?.isSymbolicLink() ? await realpath(livePath) : livePath; + await mkdir(destination, { recursive: true }); + let hasConflict = false; + + async function mergeMissing(sourceDir: string, destinationDir: string): Promise { + for (const entry of await readdir(sourceDir, { withFileTypes: true })) { + const source = path.join(sourceDir, entry.name); + const target = path.join(destinationDir, entry.name); + const sourceInfo = await lstat(source); + const targetInfo = await lstat(target).catch(() => null); + if (targetInfo === null) { + await copyStagedEntry(source, target); + continue; + } + if ( + sourceInfo.isDirectory() && + !sourceInfo.isSymbolicLink() && + targetInfo.isDirectory() && + !targetInfo.isSymbolicLink() + ) { + if ((sourceInfo.mode & 0o777) !== (targetInfo.mode & 0o777)) hasConflict = true; + await mergeMissing(source, target); + continue; + } + if (sourceInfo.isFile() && targetInfo.isFile()) { + const [sourceContents, targetContents] = await Promise.all([ + readFile(source), + readFile(target), + ]); + if ( + !sourceContents.equals(targetContents) || + (sourceInfo.mode & 0o777) !== (targetInfo.mode & 0o777) + ) { + hasConflict = true; + } + continue; + } + if (sourceInfo.isSymbolicLink() && targetInfo.isSymbolicLink()) { + const [sourceTarget, targetTarget] = await Promise.all([ + readlink(source), + readlink(target), + ]); + if (sourceTarget !== targetTarget) hasConflict = true; + continue; + } + hasConflict = true; + } + } + + await mergeMissing(backupPath, destination); + return hasConflict; +} + +function recoveryBase(transactionDir: string, livePath: string, backupPath: string): string { + const transactionId = path.basename(transactionDir).replace(TRANSACTION_PREFIX, ""); + // Update backups live inside the central transaction directory; delete stages + // live beside their target root. Keep quarantine on the backup's filesystem + // so preserving it remains an atomic rename across mounted agent homes. + const recoveryParent = backupPath.startsWith(`${transactionDir}${path.sep}`) + ? path.dirname(transactionDir) + : path.dirname(livePath); + return path.join( + recoveryParent, + `${RECOVERED_PREFIX}${path.basename(livePath)}-${transactionId}`, + ); +} + +async function findQuarantinedEntry( + transactionDir: string, + livePath: string, + backupPath: string, + claimed: ReadonlySet, +): Promise { + const base = recoveryBase(transactionDir, livePath, backupPath); + const name = path.basename(base); + const matches = (await readdir(path.dirname(base)).catch(() => [])) + .filter((entry) => entry === name || entry.startsWith(`${name}-`)) + .map((entry) => path.join(path.dirname(base), entry)) + .filter((entry) => !claimed.has(entry)) + .sort(); + return matches[0] ?? null; +} + +async function quarantineStagedEntry( + transactionDir: string, + livePath: string, + backupPath: string, +): Promise { + const base = recoveryBase(transactionDir, livePath, backupPath); + let destination = base; + for (let suffix = 1; await lstat(destination).catch(() => null); suffix += 1) { + destination = `${base}-${suffix}`; + } + await rename(backupPath, destination); +} + +async function restoreDeletedDirectory( + transactionDir: string, + livePath: string, + backupPath: string | null, +): Promise { + if (backupPath === null) return; + const liveInfo = await lstat(livePath).catch(() => null); + if (liveInfo === null) { + await mkdir(path.dirname(livePath), { recursive: true }); + await rename(backupPath, livePath); + return; + } + const backupInfo = await lstat(backupPath); + if (backupInfo.isDirectory() && !backupInfo.isSymbolicLink() && (await isDirectory(livePath))) { + const hasConflict = await mergeBackupDirectory(livePath, backupPath); + if (hasConflict) await quarantineStagedEntry(transactionDir, livePath, backupPath); + return; + } + // A concurrent writer recreated the path with an incompatible type. Keep its + // entry live and move the staged original outside transaction cleanup so + // neither side is overwritten and later controller operations can proceed. + await quarantineStagedEntry(transactionDir, livePath, backupPath); +} + +async function restoreStagedEntry( + liveRoot: string, + backupRoot: string | null, + rel: string, +): Promise { + const live = path.join(liveRoot, rel); + const backup = backupRoot && path.join(backupRoot, rel); + await rm(live, { recursive: true, force: true }); + if (backup === null || !(await lstat(backup).catch(() => null))) return; + await copyStagedEntry(backup, live); +} + +async function undoSyncedDirectory( + transactionDir: string, + livePath: string, + backupPath: string | null, + expectedFiles: ReadonlyMap, +): Promise { + for (const rel of await listAllFiles(livePath)) { + const target = path.join(livePath, rel); + const current = await readFile(target).catch(() => null); + const expected = expectedFiles.get(rel); + if (current === null || expected === undefined || !current.equals(expected)) continue; + await restoreStagedEntry(livePath, backupPath, rel); + } + + // Sync can prune files that an older managed-files manifest owned. Restore + // missing staged entries with their type and metadata, but never overwrite a + // path another writer recreated. + const hasConflict = await mergeBackupDirectory(livePath, backupPath); + if (hasConflict && backupPath !== null) { + await quarantineStagedEntry(transactionDir, livePath, backupPath); + } + if ((await readdir(livePath).catch(() => [])).length === 0) { + await rm(livePath, { recursive: true, force: true }); + } +} + +async function restore( + targets: SkillTargets, + transactionDir: string, + manifest: TransactionManifest, +): Promise { + // Nothing was converged yet, so the live tree is already the pre-save state. + if (manifest.phase === "capturing") return; + const expectedByName = new Map>(); + const claimedQuarantines = new Set(); + for (const entry of manifest.entries) { + const backup = resolveBackupPath(transactionDir, entry.backupPath); + if (backup !== null && !(await lstat(backup).catch(() => null))) { + // Atomic rename leaves either the live path or its stage. If neither is + // present, an external actor deleted it; there is nothing this + // transaction can restore. Other captured entries still need rollback. + if (entry.kind === "delete") continue; + const quarantined = await findQuarantinedEntry( + transactionDir, + entry.livePath, + backup, + claimedQuarantines, + ); + if (quarantined !== null) { + claimedQuarantines.add(quarantined); + continue; + } + throw new Error(`Skills transaction backup is missing: ${backup}`); + } + if (entry.kind === "delete") { + await restoreDeletedDirectory(transactionDir, entry.livePath, backup); + continue; + } + const liveInfo = await lstat(entry.livePath).catch(() => null); + if (liveInfo !== null && !(await isDirectory(entry.livePath))) { + // Add/update rollback has the same conflict state as delete rollback. A + // recreated file or file symlink belongs to the external writer; preserve + // it, and preserve any captured original outside transaction cleanup. + if (backup !== null) { + await quarantineStagedEntry(transactionDir, entry.livePath, backup); + } + continue; + } + const backupInfo = backup && (await lstat(backup).catch(() => null)); + if (backupInfo && !backupInfo.isDirectory()) { + if (liveInfo !== null && (await isDirectory(entry.livePath))) { + // The captured file was replaced by a directory after the transaction + // started. Preserve the external tree and quarantine the captured entry + // instead of recursively deleting the replacement during recovery. + await quarantineStagedEntry(transactionDir, entry.livePath, backup); + continue; + } + await restoreStagedEntry(entry.livePath, backup, "."); + continue; + } + const name = path.basename(entry.livePath); + let expected = expectedByName.get(name); + if (!expected) { + expected = await expectedSyncedFiles(targets.sourceDir, name); + expectedByName.set(name, expected); + } + await undoSyncedDirectory(transactionDir, entry.livePath, backup, expected); + } +} + +function transactionParents(targets: SkillTargets): string[] { + return [ + ...new Set( + [targets.agentsDir, targets.claudeDir, targets.codexDir].map((root) => path.dirname(root)), + ), + ]; +} + +/** + * Finishes what an interrupted save started. A transaction directory only exists + * while a save is mid-flight, so finding one means the process died between + * mutating the directories and committing the selection — the staged copies are + * the only place the user's files still exist. Restore them, then drop the + * transaction. Anything without our manifest is left where it is. + */ +export async function recoverInterruptedSkillTransactions( + targets: SkillTargets, + committedSelection: SkillSelection, +): Promise { + for (const parent of transactionParents(targets)) { + const entries = await readdir(parent).catch(() => []); + for (const entry of entries) { + if (!entry.startsWith(TRANSACTION_PREFIX)) continue; + const transactionDir = path.join(parent, entry); + const manifest = await readManifest(transactionDir); + if (!manifest) continue; + if (!(await validateEntries(targets, transactionDir, manifest.entries))) continue; + if (selectionsEqual(committedSelection, manifest.nextSelection)) { + await discardTransaction(transactionDir, manifest.entries); + continue; + } + if (!selectionsEqual(committedSelection, manifest.previousSelection)) { + throw new Error( + `Cannot safely recover interrupted skills transaction at ${transactionDir}`, + ); + } + await restore(targets, transactionDir, manifest); + await discardTransaction(transactionDir, manifest.entries); + } + } +} + +/** + * Captures the managed skill directories so a failed save can put the machine + * back exactly as it was — including files the user added inside them, which + * re-running an install cannot reconstruct. + */ +export async function beginSkillsTransaction( + targets: SkillTargets, + previousSelection: SkillSelection, + nextSelection: SkillSelection, + ops: readonly SkillOp[], +): Promise { + const roots = [targets.agentsDir, targets.claudeDir, targets.codexDir]; + const seenRoots = new Set(); + const uniqueRoots: Array<{ root: string; rootIndex: number }> = []; + for (const [rootIndex, root] of roots.entries()) { + const identity = await realpath(root).catch(() => path.resolve(root)); + if (seenRoots.has(identity)) continue; + seenRoots.add(identity); + uniqueRoots.push({ root, rootIndex }); + } + // Staged next to the skills tree rather than inside it: same filesystem, so a + // restore is a local copy, and never a directory a skill scan walks into. + const transactionDir = path.join(path.dirname(roots[0]!), `${TRANSACTION_PREFIX}${randomUUID()}`); + const entries: CapturedDirectory[] = []; + + async function discard(): Promise { + await discardTransaction(transactionDir, entries); + } + + await mkdir(transactionDir, { recursive: true }); + await writeManifest(transactionDir, { + owner: MANIFEST_OWNER, + version: 1, + phase: "capturing", + previousSelection, + nextSelection, + entries: [], + }); + + try { + for (const { root, rootIndex } of uniqueRoots) { + for (const op of ops) { + const name = op.name; + const livePath = path.join(root, name); + const liveInfo = await lstat(livePath).catch(() => null); + if (liveInfo === null) { + entries.push({ livePath, kind: op.kind, backupPath: null }); + continue; + } + const liveIsDirectory = await isDirectory(livePath); + const backupPath = + op.kind === "delete" + ? path.join(root, path.basename(transactionDir), name) + : path.join(BACKUP_DIRNAME, String(rootIndex), name); + if (op.kind !== "delete") { + const captureSource = + liveInfo.isSymbolicLink() && liveIsDirectory ? await realpath(livePath) : livePath; + const captureInfo = await lstat(captureSource); + await cp(captureSource, resolveBackupPath(transactionDir, backupPath)!, { + recursive: captureInfo.isDirectory(), + preserveTimestamps: true, + verbatimSymlinks: true, + }); + } + entries.push({ livePath, kind: op.kind, backupPath }); + } + } + await writeManifest(transactionDir, { + owner: MANIFEST_OWNER, + version: 1, + phase: "converging", + previousSelection, + nextSelection, + entries, + }); + } catch (error) { + // Nothing has been converged yet, so failing to stage means failing the save + // before it touches anything. + await discard(); + throw error; + } + + try { + // Each delete is staged inside its own skills root, so rename stays atomic + // even when the agent homes are symlinks or mounts on different filesystems. + // Any writer + // holding the old directory keeps writing into the staged inode; any writer + // recreating the old path remains live and is merged on rollback. The later + // convergence plan deliberately omits deletes. + for (const entry of entries) { + if (entry.kind !== "delete" || entry.backupPath === null) continue; + const backup = resolveBackupPath(transactionDir, entry.backupPath)!; + await mkdir(path.dirname(backup), { recursive: true }); + await rename(entry.livePath, backup); + } + } catch (error) { + await restore(targets, transactionDir, { + owner: MANIFEST_OWNER, + version: 1, + phase: "converging", + previousSelection, + nextSelection, + entries, + }); + await discard(); + throw error; + } + + return { + commit: discard, + async rollback(): Promise { + await restore(targets, transactionDir, { + owner: MANIFEST_OWNER, + version: 1, + phase: "converging", + previousSelection, + nextSelection, + entries, + }); + await discard(); + }, + }; +} diff --git a/packages/desktop/src/integrations/skills/sync.test.ts b/packages/desktop/src/integrations/skills/sync.test.ts index fda470fe4..da5b1c876 100644 --- a/packages/desktop/src/integrations/skills/sync.test.ts +++ b/packages/desktop/src/integrations/skills/sync.test.ts @@ -195,6 +195,39 @@ describe("syncSkills", () => { ); }); + it.skipIf(process.platform === "win32")( + "rejects managed paths that would write through a symlink", + async () => { + await writeBundleSkill(sandbox.sourceDir, "paseo", { + "SKILL.md": "new skill", + "references/guide.md": "new guide", + }); + const onDiskSkill = path.join(sandbox.agentsDir, "paseo"); + const externalReferences = path.join(sandbox.root, "external-references"); + await fs.mkdir(onDiskSkill, { recursive: true }); + await fs.mkdir(externalReferences, { recursive: true }); + await fs.writeFile(path.join(onDiskSkill, "SKILL.md"), "old skill"); + await fs.writeFile(path.join(externalReferences, "guide.md"), "external guide"); + await fs.symlink(externalReferences, path.join(onDiskSkill, "references"), "dir"); + + await expect( + syncSkills({ + sourceDir: sandbox.sourceDir, + agentsDir: sandbox.agentsDir, + claudeDir: sandbox.claudeDir, + codexDir: sandbox.codexDir, + skillNames: ["paseo"], + }), + ).rejects.toThrow("Cannot sync through symbolic link"); + + expect(await fs.readFile(path.join(onDiskSkill, "SKILL.md"), "utf-8")).toBe("old skill"); + expect(await fs.readFile(path.join(externalReferences, "guide.md"), "utf-8")).toBe( + "external guide", + ); + expect((await fs.lstat(path.join(onDiskSkill, "references"))).isSymbolicLink()).toBe(true); + }, + ); + it("reports zero changed files on a no-op resync", async () => { await writeBundleSkill(sandbox.sourceDir, "paseo", { "SKILL.md": "content", diff --git a/packages/desktop/src/integrations/skills/sync.ts b/packages/desktop/src/integrations/skills/sync.ts index 4741101ed..923336554 100644 --- a/packages/desktop/src/integrations/skills/sync.ts +++ b/packages/desktop/src/integrations/skills/sync.ts @@ -107,6 +107,31 @@ async function pruneEmptyParentDirs(rootDir: string, rels: readonly string[]): P } } +async function assertManagedPathsStayInsideSkill(rootDir: string, rels: readonly string[]) { + for (const rel of rels) { + const normalized = path.normalize(rel); + if ( + path.isAbsolute(normalized) || + normalized === ".." || + normalized.startsWith(`..${path.sep}`) + ) { + throw new Error(`Cannot sync managed path outside skill directory: ${rel}`); + } + let current = rootDir; + for (const segment of normalized.split(path.sep)) { + current = path.join(current, segment); + const info = await fs.lstat(current).catch((error: NodeJS.ErrnoException) => { + if (error.code === "ENOENT") return null; + throw error; + }); + if (info === null) break; + if (info.isSymbolicLink()) { + throw new Error(`Cannot sync through symbolic link: ${current}`); + } + } + } +} + async function syncDirectoryFiles(srcDir: string, dstDir: string): Promise { const files = await listFilesRecursive(srcDir); const srcFileSet = new Set(files); @@ -114,7 +139,13 @@ async function syncDirectoryFiles(srcDir: string, dstDir: string): Promise