From cf6ea097116bdf3578b1e02e35c772d8aa92db00 Mon Sep 17 00:00:00 2001 From: Geekwolf Date: Wed, 14 Oct 2026 16:00:00 +0800 Subject: [PATCH] fix(setup): refuse linked skill installation directories (#3108) Validate final selected skill roots and references before preview, approval, and client/Hopper changes, with a repeated installation-boundary check. Preserve interactive unselected-client choices and registration-only setup. Share typed directory/read observations across setup, doctor, and update; report unmanaged paths and unknown reads with actionable paths/reasons. Retain uninstall child outcomes and ownership manifests when cleanup remains. Shared single-pass inspection retains all unreadable paths after stale or missing files and keeps observed metadata, without duplicate manifest reads. Absent installed files do not depend on generated bundled bytes. Validation: 217 related tests, 41 final CLI/MCP and filesystem checks, and all 7,446 source tests in 508 files pass (eight platform/condition skips). Static/docs checks pass. Normal four-call install/rerun/repair/uninstall tasks preserve results and output token counts. No native Windows follow-up or arbitrary concurrent swap containment claim. Fixes #3057. Co-authored-by: geekwolf <3125615+geekwolf@users.noreply.github.com> Co-authored-by: N0zoM1z0 <161784452+N0zoM1z0@users.noreply.github.com> Co-authored-by: Kaoru0822 <208281322+kaoru0822-kitauji@users.noreply.github.com> Co-authored-by: akushonkamen <74654766+akushonkamen@users.noreply.github.com> --- docs/installation.md | 10 + src/application/Doctor.ts | 70 +++-- src/application/DoctorScope.ts | 12 +- src/application/Setup.fixture.ts | 9 +- src/application/Setup.planning.test.ts | 91 +++++++ src/application/Setup.ts | 24 +- src/application/SetupHost.ts | 8 + src/application/SetupSkill.ts | 254 +++++++++++++----- src/application/SetupTypes.ts | 10 +- src/application/Uninstall.ts | 6 + .../setup/selectedSkillSetup.test.ts | 183 ++++++++++++- tests/boundary/filesystem/setupSkill.test.ts | 130 +++++++++ 12 files changed, 718 insertions(+), 89 deletions(-) diff --git a/docs/installation.md b/docs/installation.md index f41e419030..9672455df7 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -122,6 +122,16 @@ See [client configuration](client-configuration.md) for client-specific roots, profiles, and environment overrides. A mixed selection plans each distinct destination. Selecting the skill without a client uses `~/.agents/skills`. Setup leaves existing skill copies in other locations untouched. +Setup refuses symbolic links at the selected skill directory or its managed +`references` directory, matching uninstall and update's ownership rules. Use a +regular skill directory, or select `--skill=false` to configure the client +registration without installing the bundled skill. +The final selected directories are checked before approval and again before +setup changes any client or Hopper installation. Unselected client destinations +do not block other setup choices. Doctor reports linked or non-directory skill +paths as `unmanaged`, names the affected paths, and provides those alternatives; +an unreadable skill identity remains `unknown`. Uninstall retains and reports +files it cannot safely remove, including their ownership manifest. `rea setup --skill-client claude_code --yes --json` refreshes and verifies the selected skill destination independently of MCP registration selection. The diff --git a/src/application/Doctor.ts b/src/application/Doctor.ts index f014209d75..6659a75a68 100644 --- a/src/application/Doctor.ts +++ b/src/application/Doctor.ts @@ -23,6 +23,7 @@ import { import { readInstalledSkillIdentity, type InstalledSkillIdentity, + type SkillPathIssue, } from "./SetupSkill.js"; import { CATALOG_IDENTITY } from "../catalogIdentity.js"; import { PRODUCT_IDENTITY, SDK_IDENTITY } from "../identity.js"; @@ -132,6 +133,13 @@ export interface DoctorScope { } /** Installed-package, skill, client, and runtime identity observations. */ +export type DoctorSkillState = + | "aligned" + | "stale" + | "missing" + | "unmanaged" + | "unknown"; + interface DoctorIdentity { readonly cli_package_version: string; readonly expected_skill_version: string; @@ -148,7 +156,8 @@ interface DoctorIdentity { readonly skill: { readonly installed_version: string | null; readonly installed_tool_count: number | null; - readonly state: "aligned" | "stale" | "missing"; + readonly state: DoctorSkillState; + readonly issues?: readonly SkillPathIssue[]; readonly remediation: string | null; }; readonly registrations: readonly ClientRegistrationStatus[]; @@ -311,15 +320,13 @@ const collectDoctorIdentity = async ( skill: { installed_version: installedSkillIdentity?.version ?? null, installed_tool_count: installedSkillIdentity?.toolCount ?? null, - state: - installedSkillIdentity === undefined - ? "missing" - : skillAligned - ? "aligned" - : "stale", + state: skillIdentityState(installedSkillIdentity), + ...(installedSkillIdentity?.issues === undefined + ? {} + : { issues: installedSkillIdentity.issues }), remediation: skillAligned ? null - : "Run rea setup to update the installed REA skill.", + : skillIdentityRemediation(installedSkillIdentity), }, registrations, }, @@ -333,18 +340,45 @@ const skillIdentityAligned = ( identity.toolCount === CATALOG_IDENTITY.counts.mcp_tools && identity.canonical; +const skillIdentityState = ( + identity: InstalledSkillIdentity | undefined, +): DoctorSkillState => { + if (identity?.issues?.some(({ kind }) => kind === "unreadable")) + return "unknown"; + if ((identity?.issues?.length ?? 0) > 0) return "unmanaged"; + return identity === undefined + ? "missing" + : skillIdentityAligned(identity) + ? "aligned" + : "stale"; +}; + +const skillIdentityRemediation = ( + identity: InstalledSkillIdentity | undefined, +): string => { + const issues = identity?.issues; + if (issues !== undefined && issues.length > 0) + return `${issues.map(({ path, detail }) => `${detail} (${path})`).join(" ")} Verify the reported paths and use regular skill directories, or run rea setup with --skill=false to configure the client without installing the skill.`; + return "Run rea setup to update the installed REA skill."; +}; + const skillIdentityCheck = ( identity: InstalledSkillIdentity | undefined, -): DoctorCheck => ({ - name: "skill:identity", - ok: false, - classification: "config_drift", - detail: - identity === undefined - ? "Installed REA skill identity is missing." - : "Installed REA skill identity is stale.", - remediation: "Run rea setup to update the installed REA skill.", -}); +): DoctorCheck => { + const issues = identity?.issues; + return { + name: "skill:identity", + ok: false, + classification: "config_drift", + detail: + issues !== undefined && issues.length > 0 + ? `Selected skill identity is ${skillIdentityState(identity)}: ${issues.map(({ path }) => path).join(", ")}.` + : identity === undefined + ? "Installed REA skill identity is missing." + : "Installed REA skill identity is stale.", + remediation: skillIdentityRemediation(identity), + }; +}; const registrationCheck = ( registration: UnhealthyClientRegistrationStatus, diff --git a/src/application/DoctorScope.ts b/src/application/DoctorScope.ts index 0dd9dcd110..26838b63e4 100644 --- a/src/application/DoctorScope.ts +++ b/src/application/DoctorScope.ts @@ -8,6 +8,7 @@ import type { DoctorCheck, DoctorProviderInspection, DoctorScope, + DoctorSkillState, } from "./Doctor.js"; /** Normalized readiness boundary included in every doctor response. */ @@ -29,7 +30,7 @@ interface DoctorProviderState { interface DoctorScopeInput { readonly checks: readonly DoctorCheck[]; readonly registrations: readonly ClientRegistrationStatus[]; - readonly skillState: "aligned" | "stale" | "missing"; + readonly skillState: DoctorSkillState; readonly providers: DoctorProviderState; readonly scope: DoctorScopeReport; } @@ -188,7 +189,14 @@ const selectedSkillCheck = ( classification: state === "aligned" ? "healthy" : "config_drift", ...(state === "aligned" ? { detail: "Installed REA skill identity is aligned." } - : { remediation: "Run rea setup to update the installed REA skill." }), + : { + remediation: + state === "unmanaged" + ? "Use regular skill directories, or run rea setup with --skill=false to configure the client without installing the skill." + : state === "unknown" + ? "Verify access to the reported skill files before retrying." + : "Run rea setup to update the installed REA skill.", + }), }; const doctorProviderCheckName = (providerId: string, name: string): boolean => diff --git a/src/application/Setup.fixture.ts b/src/application/Setup.fixture.ts index ef7f37d3ad..e329b0eefc 100644 --- a/src/application/Setup.fixture.ts +++ b/src/application/Setup.fixture.ts @@ -8,7 +8,7 @@ import type { } from "./SetupTypes.js"; import type { SetupClient } from "./SupportedClients.js"; import type { SetupHopperInstallResult } from "./SetupInstallFailure.js"; -import { err, ok } from "../domain/result.js"; +import { err, ok, type Result } from "../domain/result.js"; import { SetupSkillInstallError } from "./SetupSkill.js"; import type { DoctorCheck, DoctorReport, DoctorScope } from "./Doctor.js"; import type { LinuxDistribution } from "./LinuxHopper.js"; @@ -43,6 +43,7 @@ export class FakeSetupHost implements SetupHost { hopperReplaceRequests: boolean[] = []; configurations = 0; skillInstalls = 0; + skillDirectoryValidations: Array = []; doctorCalls = 0; doctorScopes: Array = []; checkedHopperPaths: Array = []; @@ -129,6 +130,12 @@ export class FakeSetupHost implements SetupHost { ); skillNeedsInstall = (): Promise => Promise.resolve(this.skill !== "unchanged"); + validateSkillDirectories = ( + clientIds: readonly string[], + ): Promise> => { + this.skillDirectoryValidations.push(clientIds); + return Promise.resolve(ok(undefined)); + }; installSkill = () => { this.skillInstalls += 1; return Promise.resolve( diff --git a/src/application/Setup.planning.test.ts b/src/application/Setup.planning.test.ts index 2548772c91..ce52be4697 100644 --- a/src/application/Setup.planning.test.ts +++ b/src/application/Setup.planning.test.ts @@ -3,6 +3,97 @@ import { describe, expect, it } from "vitest"; import { runSetup } from "./Setup.js"; import type { SetupProgressEvent } from "./SetupTypes.js"; import { FakeSetupHost, options } from "./Setup.fixture.js"; +import { SetupSkillInstallError } from "./SetupSkill.js"; +import { err, ok } from "../domain/result.js"; + +const linkedSkillFailure = () => + new SetupSkillInstallError({ + path: "/fixture/home/.claude/skills/reverse-engineer-anything", + stage: "prepare", + reason: "Skill directory is a symbolic link", + rollbackFailures: [], + recoveryBackupPaths: [], + cause: undefined, + }); + +describe("selected skill admission", () => { + it.each(["codex", "shared"] as const)( + "keeps %s choices when an unselected client is linked", + async (choice) => { + const host = new FakeSetupHost(); + host.hopper = "/Applications/Hopper"; + host.clients = [ + { name: "claude_code", configPath: "/claude.json" }, + { name: "codex", configPath: "/codex.toml", format: "toml" }, + ]; + host.validateSkillDirectories = async (ids) => { + host.skillDirectoryValidations.push(ids); + return ids.includes("claude_code") + ? err(linkedSkillFailure()) + : ok(undefined); + }; + const result = await runSetup( + { ...options(false), structured: false }, + host, + async (actions, context) => { + if (context?.stage === "select") { + expect(actions.some(({ kind }) => kind === "install_skill")).toBe( + true, + ); + return { + approved: false, + selectedActionIds: + choice === "codex" + ? ["configure_client:codex"] + : ["install_skill"], + }; + } + return true; + }, + ); + expect(result.status).toBe("ready"); + expect(host.skillInstalls).toBe(1); + expect(host.configurations).toBe(choice === "codex" ? 1 : 0); + expect(host.skillDirectoryValidations).toEqual( + choice === "codex" ? [["codex"], ["codex"]] : [[], []], + ); + }, + ); + + it("rechecks directory admission after approval before changing clients or Hopper", async () => { + const host = new FakeSetupHost("linux"); + host.distribution = { + id: "ubuntu", + versionId: "24.04", + packageFamily: "deb", + hopperSetupSupported: true, + }; + host.clients = [{ name: "claude_code", configPath: "/claude.json" }]; + let linked = false; + host.validateSkillDirectories = async () => + linked ? err(linkedSkillFailure()) : ok(undefined); + const result = await runSetup( + { + ...options(false, true), + clientIds: ["claude_code"], + structured: false, + }, + host, + async () => { + linked = true; + return true; + }, + ); + expect(result).toMatchObject({ + status: "needs_human", + code: "skill_install_failed", + appliedActions: [], + }); + expect(host.configurations).toBe(0); + expect(host.hopperInstalls).toBe(0); + expect(host.skillInstalls).toBe(0); + }); +}); describe("skill plan destinations", () => { it("plans a Claude-only skill in its personal location", async () => { diff --git a/src/application/Setup.ts b/src/application/Setup.ts index 2b2fb5d456..a2a2459880 100644 --- a/src/application/Setup.ts +++ b/src/application/Setup.ts @@ -99,6 +99,12 @@ const runSetupWorkflow = async ( [host, plannedActions, appliedActions, clients, clientStates], readinessScope, ); + const failSkill = async ( + error: SetupSkillInstallError, + ): Promise => ({ + ...(await fail(error.remediation)), + code: "skill_install_failed", + }); const unsupported = await hostRemediation(host, false); if (unsupported !== undefined) return fail(unsupported); const initialState = await host.initialSetupState(options.readinessScope); @@ -235,6 +241,12 @@ const runSetupWorkflow = async ( ); const manualRemediation = manualClientsRemediation(planned.selectedClients); + if (skillSelected) { + const admission = await host.validateSkillDirectories( + selectedSkillClientIds, + ); + if (!admission.ok) return failSkill(admission.error); + } if (options.dryRun === true) return { status: "planned", @@ -285,6 +297,13 @@ const runSetupWorkflow = async ( : {}), }; + // Approval may pause while a skill manager changes a selected directory. + if (planSelection.skillSelected) { + const admission = await host.validateSkillDirectories( + selectedSkillClientIds, + ); + if (!admission.ok) return failSkill(admission.error); + } let selectedClients: readonly SetupClient[] = planSelection.clientsToConfigure; if ( @@ -325,10 +344,7 @@ const runSetupWorkflow = async ( selectedSkillClientIds, ); if (skillFailure !== undefined) { - return { - ...(await fail(skillFailure.remediation)), - code: "skill_install_failed", - }; + return failSkill(skillFailure); } } const doctor = summarizeDoctor(await host.doctor(readinessScope)); diff --git a/src/application/SetupHost.ts b/src/application/SetupHost.ts index 651616fd8b..3674cd01fe 100644 --- a/src/application/SetupHost.ts +++ b/src/application/SetupHost.ts @@ -31,6 +31,7 @@ import { skillDestinations, canonicalSkillNeedsInstall, installCanonicalSkill, + validateCanonicalSkillDirectories, } from "./SetupSkill.js"; import { clientConfigurationAligned, @@ -224,6 +225,13 @@ export const systemSetupHost = ( platform, resources, ), + validateSkillDirectories: (clientIds) => + validateCanonicalSkillDirectories( + homeDirectory, + clientIds, + environment, + platform, + ), installSkill: (clientIds) => installCanonicalSkill( homeDirectory, diff --git a/src/application/SetupSkill.ts b/src/application/SetupSkill.ts index 7eeec94560..7dd28dee0d 100644 --- a/src/application/SetupSkill.ts +++ b/src/application/SetupSkill.ts @@ -178,6 +178,14 @@ export interface InstalledSkillIdentity { readonly version: string | null; readonly toolCount: number | null; readonly canonical: boolean; + readonly issues?: readonly SkillPathIssue[]; +} + +/** Known management blockers and unresolved filesystem observations. */ +export interface SkillPathIssue { + readonly path: string; + readonly kind: "symbolic_link" | "not_directory" | "unreadable"; + readonly detail: string; } /** Resolve the skill roots required by a client selection, or all owned roots. */ @@ -209,8 +217,10 @@ export const existingSkillDestinations = async ( platform, )) { try { - const root = await lstat(destination.path); - if (root.isSymbolicLink() || !root.isDirectory()) continue; + const issues = await skillDirectoryIssues([destination]); + if (issues.some(({ kind }) => kind === "unreadable")) + throw skillDirectoryFailure(issues); + if (issues.length > 0) continue; const manifest = join(destination.path, "SKILL.md"); const content = await readSkillFile(manifest, resources); if (isManagedSkillManifest(content)) existing.push(destination); @@ -242,49 +252,187 @@ const readOptionalSkillState = async ( } }; +const inspectSkillDirectory = async ( + path: string, +): Promise> => { + try { + const directory = await lstat(path); + if (directory.isSymbolicLink()) + return err({ + path, + kind: "symbolic_link", + detail: "Skill directory is a symbolic link.", + }); + if (!directory.isDirectory()) + return err({ + path, + kind: "not_directory", + detail: "Skill path is not a directory.", + }); + return ok(undefined); + } catch (cause: unknown) { + if (cause instanceof Error && "code" in cause && cause.code === "ENOENT") + return ok(undefined); + return err({ + path, + kind: "unreadable", + detail: cause instanceof Error ? cause.message : String(cause), + }); + } +}; + +const skillDirectoryIssues = async ( + destinations: readonly SkillDestination[], +): Promise => { + const issues: SkillPathIssue[] = []; + for (const { path } of destinations) { + const root = await inspectSkillDirectory(path); + if (!root.ok) { + issues.push(root.error); + continue; + } + const references = await inspectSkillDirectory(join(path, "references")); + if (!references.ok) issues.push(references.error); + } + return issues; +}; + +const skillDirectoryFailure = ( + issues: readonly SkillPathIssue[], +): SetupSkillInstallError => + new SetupSkillInstallError({ + path: issues[0]?.path ?? "skill bundle", + stage: "prepare", + reason: `${issues.map(({ path, detail }) => `${detail} (${path})`).join(" ")} Use a regular skill directory, or run setup with --skill=false to configure the client without installing the skill`, + rollbackFailures: [], + recoveryBackupPaths: [], + cause: issues, + }); + +/** Validate only final selected roots; permission to install is separate from freshness. */ +export const validateCanonicalSkillDirectories = async ( + home: string, + clientIds: readonly string[], + environment: Readonly = {}, + platform: NodeJS.Platform = process.platform, +): Promise> => { + const issues = await skillDirectoryIssues( + skillDestinations(home, clientIds, environment, platform), + ); + return issues.length === 0 + ? ok(undefined) + : err(skillDirectoryFailure(issues)); +}; + const canonicalSkillFiles = async ( home: string, clientIds: readonly string[], environment: Readonly = {}, platform: NodeJS.Platform = process.platform, resources?: ArtifactResourceScope, -): Promise => - Promise.all( - skillDestinations(home, clientIds, environment, platform).flatMap( - ({ path: root }) => - MANAGED_SKILL_FILES.map(async (relativePath) => { - const destination = join(root, relativePath); - return { - destination, - content: await canonicalSkillContent(relativePath), - original: await readOptionalSkillState(destination, resources), - }; - }), +): Promise => { + const destinations = skillDestinations( + home, + clientIds, + environment, + platform, + ); + const admission = await validateCanonicalSkillDirectories( + home, + clientIds, + environment, + platform, + ); + if (!admission.ok) throw admission.error; + return Promise.all( + destinations.flatMap(({ path: root }) => + MANAGED_SKILL_FILES.map(async (relativePath) => { + const destination = join(root, relativePath); + return { + destination, + content: await canonicalSkillContent(relativePath), + original: await readOptionalSkillState(destination, resources), + }; + }), ), ); +}; -const canonicalSkillNeedsInstallByReading = async ( +interface CanonicalSkillInspection { + readonly canonical: boolean; + readonly manifest: string | undefined; +} + +interface SkillInspectionFailure { + readonly _tag: "SkillInspectionFailure"; + readonly issues: readonly SkillPathIssue[]; + readonly cause: unknown; + readonly manifest: string | undefined; +} + +const inspectCanonicalSkill = async ( home: string, clientIds: readonly string[], environment: Readonly, platform: NodeJS.Platform, resources?: ArtifactResourceScope, -): Promise => { - for (const { path: root } of skillDestinations( +): Promise> => { + const destinations = skillDestinations( home, clientIds, environment, platform, - )) { + ); + const issues = [...(await skillDirectoryIssues(destinations))]; + if (issues.length > 0) + return err({ + _tag: "SkillInspectionFailure", + issues, + cause: issues, + manifest: undefined, + }); + let canonical = true; + let manifest: string | undefined; + let failureCause: unknown; + for (const { path: root } of destinations) { for (const relativePath of MANAGED_SKILL_FILES) { const destination = join(root, relativePath); - const content = await canonicalSkillContent(relativePath); - const original = await readOptionalSkillState(destination, resources); - if (original?.bytes.equals(Buffer.from(content, "utf8")) !== true) - return true; + let original: SkillFileState | undefined; + try { + original = await readOptionalSkillState(destination, resources); + } catch (cause: unknown) { + if ( + cause instanceof RegularFileCleanupFailure || + cause instanceof ArtifactReaderFailure + ) + throw cause; + issues.push({ + path: destination, + kind: "unreadable", + detail: causeReason(cause), + }); + if (issues.length === 1) failureCause = cause; + canonical = false; + continue; + } + if (relativePath === "SKILL.md" && manifest === undefined) + manifest = original?.bytes.toString("utf8"); + // A mismatch settles freshness, but not coverage of later read failures. + if (original === undefined) canonical = false; + else if (canonical) { + const content = await canonicalSkillContent(relativePath); + canonical = original.bytes.equals(Buffer.from(content, "utf8")); + } } } - return false; + return issues.length === 0 + ? ok({ canonical, manifest }) + : err({ + _tag: "SkillInspectionFailure", + issues, + cause: failureCause, + manifest, + }); }; /** Report whether setup would change any file in the managed REA skill bundle. */ @@ -296,20 +444,21 @@ export const canonicalSkillNeedsInstall = async ( resources?: ArtifactResourceScope, ): Promise => { try { - return await canonicalSkillNeedsInstallByReading( + const inspection = await inspectCanonicalSkill( home, clientIds, environment, platform, resources, ); + return !inspection.ok || !inspection.value.canonical; } catch (cause: unknown) { if ( cause instanceof RegularFileCleanupFailure || cause instanceof ArtifactReaderFailure ) throw cause; - // Unreadable skill state fails open to install so setup can repair it. + // A copy needs attention; selected-root admission decides whether it may be installed. void cause; return true; } @@ -431,6 +580,8 @@ export const installCanonicalSkill = async ( } return ok("installed"); } catch (cause: unknown) { + if (stage === "prepare" && cause instanceof SetupSkillInstallError) + return err(cause); failingPath = causePath(cause) ?? failingPath; const rollbackFailures = await restoreSkillFiles(attempted, operations); return err( @@ -458,48 +609,29 @@ export const readInstalledSkillIdentity = async ( platform: NodeJS.Platform = process.platform, resources?: ArtifactResourceScope, ): Promise => { - const destinations = skillDestinations( + const inspection = await inspectCanonicalSkill( home, clientIds, environment, platform, + resources, ); - let content: string | undefined; - for (const destination of destinations) { - try { - content = await readSkillFile( - join(destination.path, "SKILL.md"), - resources, - ); - break; - } catch (cause: unknown) { - if ( - cause instanceof RegularFileCleanupFailure || - cause instanceof ArtifactReaderFailure - ) - throw cause; - // Another selected copy may provide metadata; canonical validation below - // still marks missing or unreadable copies as stale. - } - } - if (content === undefined) return undefined; - const toolCount = /^\s{2}tool_count:\s*(\d+)\s*$/mu.exec(content)?.[1]; - let canonical: boolean; - try { - canonical = !(await canonicalSkillNeedsInstall( - home, - clientIds, - environment, - platform, - resources, - )); - } catch (cause: unknown) { - if (!(cause instanceof ArtifactReaderFailure)) throw cause; - canonical = false; - } + const content = inspection.ok + ? inspection.value.manifest + : inspection.error.manifest; + const issues = inspection.ok ? [] : inspection.error.issues; + if (content === undefined && issues.length === 0) return undefined; + const toolCount = + content === undefined + ? undefined + : /^\s{2}tool_count:\s*(\d+)\s*$/mu.exec(content)?.[1]; return { - canonical, - version: /^\s{2}version:\s*"([^"]+)"\s*$/mu.exec(content)?.[1] ?? null, + canonical: inspection.ok && inspection.value.canonical, + version: + content === undefined + ? null + : (/^\s{2}version:\s*"([^"]+)"\s*$/mu.exec(content)?.[1] ?? null), toolCount: toolCount === undefined ? null : Number.parseInt(toolCount, 10), + ...(issues.length === 0 ? {} : { issues }), }; }; diff --git a/src/application/SetupTypes.ts b/src/application/SetupTypes.ts index 9d7bc44c69..dffc224ec7 100644 --- a/src/application/SetupTypes.ts +++ b/src/application/SetupTypes.ts @@ -1,6 +1,11 @@ import type { DoctorReport, DoctorScope, runDoctor } from "./Doctor.js"; import type { LinuxDistribution } from "./LinuxHopper.js"; -import type { SkillDestination, SkillInstallResult } from "./SetupSkill.js"; +import type { + SkillDestination, + SkillInstallResult, + SetupSkillInstallError, +} from "./SetupSkill.js"; +import type { Result } from "../domain/result.js"; import type { SetupClient } from "./SupportedClients.js"; import type { SetupFailureCode, @@ -89,6 +94,9 @@ export interface SetupHost { command: readonly string[], ): Promise; skillNeedsInstall(clientIds: readonly string[]): Promise; + validateSkillDirectories( + clientIds: readonly string[], + ): Promise>; installSkill(clientIds: readonly string[]): Promise; doctor(scope?: DoctorScope): Promise>>; } diff --git a/src/application/Uninstall.ts b/src/application/Uninstall.ts index d6447a8f69..b1871c2195 100644 --- a/src/application/Uninstall.ts +++ b/src/application/Uninstall.ts @@ -768,6 +768,12 @@ const removeManagedSkill = async ( const failed = results.find(({ status }) => status === "failed"); if (failed !== undefined) return item(name, "failed", results.map(({ detail }) => detail).join(" ")); + if (results.some(({ status }) => status === "retained")) + return item( + name, + "retained", + [...new Set(results.map(({ detail }) => detail))].join(" "), + ); const removed = results.some(({ status }) => status === "removed"); return item( name, diff --git a/tests/acceptance/setup/selectedSkillSetup.test.ts b/tests/acceptance/setup/selectedSkillSetup.test.ts index bc73c85b3d..8c76e1ac68 100644 --- a/tests/acceptance/setup/selectedSkillSetup.test.ts +++ b/tests/acceptance/setup/selectedSkillSetup.test.ts @@ -1,11 +1,190 @@ -import { access, readFile, readdir, writeFile } from "node:fs/promises"; -import { join, relative, resolve } from "node:path"; +import { + access, + cp, + lstat, + mkdir, + readFile, + readdir, + symlink, + writeFile, +} from "node:fs/promises"; +import { dirname, join, relative, resolve } from "node:path"; import { expect } from "vitest"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; import { cliTest } from "../../support/cli/cliFixture.js"; +cliTest.for(["root", "references"] as const)( + "rejects a linked skill %s during planning before configuring a client", + async (kind, { cli }) => { + const directory = await createTestTempDirectory("rea-skill-preflight-"); + const selected = join( + directory, + ".claude/skills/reverse-engineer-anything", + ); + const managed = join(directory, "user-managed"); + const source = resolve("skills/reverse-engineer-anything"); + await mkdir(kind === "root" ? dirname(selected) : selected, { + recursive: true, + }); + await cp(kind === "root" ? source : join(source, "references"), managed, { + recursive: true, + }); + if (kind === "references") + await cp(join(source, "SKILL.md"), join(selected, "SKILL.md")); + const link = kind === "root" ? selected : join(selected, "references"); + await symlink( + managed, + link, + process.platform === "win32" ? "junction" : "dir", + ); + const run = (arguments_: readonly string[]) => + cli.run({ + arguments: [...arguments_, "--json"], + cwd: directory, + environment: { USERPROFILE: directory }, + }); + const preview = await run([ + "setup", + "--client", + "claude_code", + "--dry-run", + ]); + const applied = await run(["setup", "--client", "claude_code", "--yes"]); + await expect(access(join(directory, ".claude.json"))).rejects.toMatchObject( + { + code: "ENOENT", + }, + ); + for (const result of [preview, applied]) { + expect(result.exitCode, result.stdout).toBe(1); + expect(result.json).toMatchObject({ + status: "needs_human", + code: "skill_install_failed", + appliedActions: [], + }); + expect(result.stdout).toContain(link); + expect(result.stdout).toContain("--skill=false"); + } + const doctor = await run(["doctor", "--client", "claude_code", "--skill"]); + expect(doctor.exitCode, doctor.stdout).toBe(1); + expect(doctor.json).toMatchObject({ + healthy: false, + identity: { skill: { state: "unmanaged" } }, + }); + expect(doctor.stdout).toContain(link); + expect((await lstat(link)).isSymbolicLink()).toBe(true); + }, +); + +cliTest( + "setup refuses a linked skill while allowing registration-only setup", + async ({ cli }) => { + const directory = await createTestTempDirectory("rea-linked-skill-cli-"); + const managed = join(directory, "managed"); + const selected = join( + directory, + ".claude/skills/reverse-engineer-anything", + ); + await mkdir(managed); + await mkdir(join(directory, ".claude/skills"), { recursive: true }); + await cp(resolve("skills/reverse-engineer-anything"), managed, { + recursive: true, + }); + const original = await readFile(join(managed, "SKILL.md"), "utf8"); + await symlink( + managed, + selected, + process.platform === "win32" ? "junction" : "dir", + ); + const run = (arguments_: readonly string[]) => + cli.run({ + arguments: [...arguments_, "--json"], + cwd: directory, + environment: { USERPROFILE: directory }, + }); + + const refused = await run([ + "setup", + "--skill-client", + "claude_code", + "--yes", + ]); + expect(refused.exitCode, refused.stdout).toBe(1); + expect(refused.json).toMatchObject({ + status: "needs_human", + code: "skill_install_failed", + }); + expect(refused.stdout).toContain("symbolic link"); + const doctor = await run(["doctor", "--client", "claude_code", "--skill"]); + expect(doctor.exitCode, doctor.stdout).toBe(1); + expect(doctor.json).toMatchObject({ + healthy: false, + identity: { skill: { state: "unmanaged" } }, + }); + const registration = await run([ + "setup", + "--client", + "claude_code", + "--skill=false", + "--yes", + ]); + expect(registration.exitCode, registration.stdout).toBe(0); + expect(registration.json).toMatchObject({ status: "ready" }); + await access(join(directory, ".claude.json")); + expect((await lstat(selected)).isSymbolicLink()).toBe(true); + expect((await readdir(managed)).sort()).toEqual(["SKILL.md", "references"]); + expect(await readFile(join(managed, "SKILL.md"), "utf8")).toBe(original); + }, +); + +cliTest( + "uninstall reports retained references and preserves their ownership manifest", + async ({ cli }) => { + const directory = await createTestTempDirectory("rea-skill-retained-cli-"); + const selected = join( + directory, + ".claude/skills/reverse-engineer-anything", + ); + const managed = join(directory, "user-managed-references"); + const source = resolve("skills/reverse-engineer-anything"); + await mkdir(selected, { recursive: true }); + await cp(join(source, "SKILL.md"), join(selected, "SKILL.md")); + await cp(join(source, "references"), managed, { recursive: true }); + await symlink( + managed, + join(selected, "references"), + process.platform === "win32" ? "junction" : "dir", + ); + const before = await readFile(join(managed, "native-and-artifacts.md")); + const result = await cli.run({ + arguments: ["uninstall", "--json"], + cwd: directory, + environment: { USERPROFILE: directory }, + }); + expect(result.exitCode, result.stdout).toBe(0); + expect(result.json).toMatchObject({ + items: expect.arrayContaining([ + expect.objectContaining({ + name: "skill", + status: "retained", + detail: expect.stringContaining("retained"), + }), + ]), + }); + expect(await readFile(join(selected, "SKILL.md"))).toEqual( + await readFile(join(source, "SKILL.md")), + ); + expect(await readFile(join(managed, "native-and-artifacts.md"))).toEqual( + before, + ); + expect((await lstat(join(selected, "references"))).isSymbolicLink()).toBe( + true, + ); + }, +); + cliTest( "skill-only setup verifies its selected client copy on install, rerun and repair", async ({ cli }) => { diff --git a/tests/boundary/filesystem/setupSkill.test.ts b/tests/boundary/filesystem/setupSkill.test.ts index 83b4d48cda..fae651cc79 100644 --- a/tests/boundary/filesystem/setupSkill.test.ts +++ b/tests/boundary/filesystem/setupSkill.test.ts @@ -3,10 +3,14 @@ import { access, chmod, copyFile, + lstat, mkdir, readFile, + readdir, + rename, rm, stat, + symlink, writeFile, } from "node:fs/promises"; import { constants as fsConstants } from "node:fs"; @@ -16,7 +20,9 @@ import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; import { canonicalSkillNeedsInstall, + existingSkillDestinations, installCanonicalSkill, + readInstalledSkillIdentity, skillDestinations, type SkillInstallFileOperations, } from "../../../src/application/SetupSkill.js"; @@ -123,6 +129,130 @@ describe("skill recovery byte preservation", () => { }); }); +describe("skill directory admission", () => { + it("excludes a linked references directory from update maintenance", async () => { + const directory = await createTestTempDirectory( + "rea-skill-linked-maintenance-", + ); + const installed = await installCanonicalSkill(directory, ["claude_code"]); + expect(installed.ok).toBe(true); + const [selected] = skillDestinations(directory, ["claude_code"]); + if (selected === undefined) throw new Error("Missing skill destination"); + const references = join(selected.path, "references"); + const retained = join(directory, "retained-references"); + await rename(references, retained); + await symlink( + retained, + references, + process.platform === "win32" ? "junction" : "dir", + ); + expect(await existingSkillDestinations(directory)).toEqual([]); + expect((await lstat(references)).isSymbolicLink()).toBe(true); + await access(join(retained, "native-and-artifacts.md")); + }); + + it + .skipIf(process.platform === "win32" || process.getuid?.() === 0) + .each(["aligned", "stale", "missing"] as const)( + "reports every unreadable reference with a %s manifest", + async (manifestState) => { + const directory = await createTestTempDirectory( + "rea-skill-unreadable-reference-", + ); + expect((await installCanonicalSkill(directory, ["claude_code"])).ok).toBe( + true, + ); + const [selected] = skillDestinations(directory, ["claude_code"]); + if (selected === undefined) throw new Error("Missing skill destination"); + const manifest = join(selected.path, "SKILL.md"); + if (manifestState === "stale") + await writeFile(manifest, "\nlocal changes\n", { flag: "a" }); + if (manifestState === "missing") await rm(manifest); + const references = [ + "references/connection-and-recovery.md", + "references/evidence-workflows.md", + ].map((relative) => join(selected.path, relative)); + for (const reference of references) await chmod(reference, 0o000); + try { + const identity = await readInstalledSkillIdentity(directory, [ + "claude_code", + ]); + const issues = references.map((path) => + expect.objectContaining({ + path, + kind: "unreadable", + detail: expect.stringContaining("EACCES"), + }), + ); + expect(identity).toMatchObject({ + canonical: false, + version: + manifestState === "missing" ? null : PRODUCT_IDENTITY.skillVersion, + issues, + }); + const report = await runDoctor( + undefined, + createDoctorHostFixture({ + installedSkillIdentity: () => Promise.resolve(identity), + }), + ); + expect(report.identity?.skill).toMatchObject({ + state: "unknown", + issues, + remediation: expect.stringContaining("evidence-workflows.md"), + }); + } finally { + for (const reference of references) await chmod(reference, 0o600); + } + }, + ); + + it.each(["root", "references"] as const)( + "refuses a linked skill %s before changing any selected destination", + async (linkedDirectory) => { + const home = await createTestTempDirectory("rea-skill-linked-root-"); + const destinations = skillDestinations(home, ["shared", "claude_code"]); + const selected = destinations.find( + ({ client }) => client === "claude_code", + ); + if (selected === undefined) + throw new Error("Missing Claude Code skill root"); + const managed = join(home, "user-managed"); + await mkdir(managed); + const original = "user-owned instructions\n"; + await writeFile(join(managed, "SKILL.md"), original); + const linked = + linkedDirectory === "root" + ? selected.path + : join(selected.path, "references"); + await mkdir(dirname(linked), { recursive: true }); + await symlink( + managed, + linked, + process.platform === "win32" ? "junction" : "dir", + ); + + const result = await installCanonicalSkill(home, [ + "shared", + "claude_code", + ]); + if (result.ok) throw new Error("Expected linked directory refusal"); + expect(result.error.stage).toBe("prepare"); + expect(result.error.path).toBe(linked); + expect(result.error.reason).toContain("symbolic link"); + expect(result.error.rollbackFailures).toEqual([]); + expect((await lstat(linked)).isSymbolicLink()).toBe(true); + expect(await readFile(join(managed, "SKILL.md"), "utf8")).toBe(original); + expect(await readdir(managed)).toEqual(["SKILL.md"]); + const shared = destinations.find(({ client }) => client === "shared"); + if (shared === undefined) throw new Error("Missing shared skill root"); + await expect(access(shared.path)).rejects.toMatchObject({ + code: "ENOENT", + }); + }, + ); +}); + describe("canonical skill transaction", () => { it("does not roll back files when backup preparation fails", async () => { const home = await createTestTempDirectory("rea-skill-backup-failure-");