diff --git a/src/skills/lifecycle/workspace-skill-write.test.ts b/src/skills/lifecycle/workspace-skill-write.test.ts new file mode 100644 index 000000000000..c2504fcf31e5 --- /dev/null +++ b/src/skills/lifecycle/workspace-skill-write.test.ts @@ -0,0 +1,87 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { createTrackedTempDirs } from "../../test-utils/tracked-temp-dirs.js"; +import { + applyWorkspaceSkillMutation, + prepareWorkspaceSkillMutation, + restoreWorkspaceSkillMutation, +} from "./workspace-skill-write.js"; + +const tempDirs = createTrackedTempDirs(); +const symlinkPolicy = { allowWrites: false, allowedTargetRealPaths: [] }; + +afterEach(async () => { + await tempDirs.cleanup(); +}); + +describe("workspace skill mutations", () => { + it("removes support files when the activating SKILL.md write fails", async () => { + const workspaceDir = await tempDirs.make("openclaw-workspace-skill-write-failure-"); + const skillDir = path.join(workspaceDir, "skills", "partial-create"); + const skillFile = path.join(skillDir, "SKILL.md"); + const supportFile = path.join(skillDir, "references", "proof.md"); + const mutation = await prepareWorkspaceSkillMutation({ + workspaceDir, + skillDir, + skillFile, + content: "# Partial Create\n", + supportFiles: [{ path: "references/proof.md", content: "new support\n" }], + mode: "create", + symlinkPolicy, + }); + await fs.mkdir(skillFile, { recursive: true }); + + await expect(applyWorkspaceSkillMutation(mutation)).rejects.toThrow(); + await expect(fs.access(supportFile)).rejects.toThrow(); + }); + + it("restores the complete previous update bundle", async () => { + const workspaceDir = await tempDirs.make("openclaw-workspace-skill-write-update-"); + const skillDir = path.join(workspaceDir, "skills", "reversible-update"); + const skillFile = path.join(skillDir, "SKILL.md"); + const supportFile = path.join(skillDir, "references", "proof.md"); + await fs.mkdir(path.dirname(supportFile), { recursive: true }); + await fs.writeFile(skillFile, "# Before\n", "utf8"); + await fs.writeFile(supportFile, "before support\n", "utf8"); + const mutation = await prepareWorkspaceSkillMutation({ + workspaceDir, + skillDir, + skillFile, + content: "# After\n", + supportFiles: [{ path: "references/proof.md", content: "after support\n" }], + mode: "update", + symlinkPolicy, + }); + + await applyWorkspaceSkillMutation(mutation); + await expect(fs.readFile(skillFile, "utf8")).resolves.toBe("# After\n"); + await expect(fs.readFile(supportFile, "utf8")).resolves.toBe("after support\n"); + + await restoreWorkspaceSkillMutation(mutation); + await expect(fs.readFile(skillFile, "utf8")).resolves.toBe("# Before\n"); + await expect(fs.readFile(supportFile, "utf8")).resolves.toBe("before support\n"); + }); + + it("removes every file from a restored create mutation", async () => { + const workspaceDir = await tempDirs.make("openclaw-workspace-skill-write-create-"); + const skillDir = path.join(workspaceDir, "skills", "reversible-create"); + const skillFile = path.join(skillDir, "SKILL.md"); + const supportFile = path.join(skillDir, "references", "proof.md"); + const mutation = await prepareWorkspaceSkillMutation({ + workspaceDir, + skillDir, + skillFile, + content: "# Created\n", + supportFiles: [{ path: "references/proof.md", content: "created support\n" }], + mode: "create", + symlinkPolicy, + }); + + await applyWorkspaceSkillMutation(mutation); + await restoreWorkspaceSkillMutation(mutation); + + await expect(fs.access(skillFile)).rejects.toThrow(); + await expect(fs.access(supportFile)).rejects.toThrow(); + }); +}); diff --git a/src/skills/lifecycle/workspace-skill-write.ts b/src/skills/lifecycle/workspace-skill-write.ts index 7d29dee2c21d..598efd0f5177 100644 --- a/src/skills/lifecycle/workspace-skill-write.ts +++ b/src/skills/lifecycle/workspace-skill-write.ts @@ -9,11 +9,11 @@ const ALLOWED_SUPPORT_FILE_ROOTS = new Set( ); export const MAX_WORKSPACE_SKILL_SUPPORT_FILE_BYTES = 256 * 1024; -type WorkspaceSkillSymlinkWritePolicy = { +export type WorkspaceSkillSymlinkWritePolicy = { allowWrites: boolean; allowedTargetRealPaths: readonly string[]; }; -type WorkspaceSkillSupportFileWrite = { path: string; content: string }; +export type WorkspaceSkillSupportFileWrite = { path: string; content: string }; type WorkspaceSkillWriteTargetParams = { workspaceDir: string; @@ -21,7 +21,21 @@ type WorkspaceSkillWriteTargetParams = { symlinkPolicy: WorkspaceSkillSymlinkWritePolicy; }; -type PreviousSupportFile = { path: string; existed: boolean; previousContent?: string }; +type PreparedWorkspaceSkillFileMutation = { + filePath: string; + rootDir: string; + relativePath: string; + previousContent: string | null; + content: string; +}; + +export type PreparedWorkspaceSkillMutation = { + mode: "create" | "update"; + workspaceDir: string; + skillDir: string; + skillFile: PreparedWorkspaceSkillFileMutation; + supportFiles: Array; +}; export function normalizeWorkspaceSkillSupportPath(input: string): string { const trimmed = input.trim(); @@ -113,48 +127,111 @@ export async function writeWorkspaceSkill(params: { mode: "create" | "update"; symlinkPolicy: WorkspaceSkillSymlinkWritePolicy; }): Promise { + const mutation = await prepareWorkspaceSkillMutation(params); + await applyWorkspaceSkillMutation(mutation); +} + +export async function prepareWorkspaceSkillMutation(params: { + workspaceDir: string; + skillDir: string; + skillFile: string; + content: string; + supportFiles?: readonly WorkspaceSkillSupportFileWrite[]; + mode: "create" | "update"; + symlinkPolicy: WorkspaceSkillSymlinkWritePolicy; +}): Promise { assertInsideWorkspace(params.workspaceDir, params.skillDir, "skill directory"); const supportFiles = normalizeSupportFiles(params.supportFiles ?? []); - const previousSupportFiles = await prepareWorkspaceSkillWrite({ + const skillTarget = await resolveWorkspaceSkillWriteTarget({ + workspaceDir: params.workspaceDir, + filePath: params.skillFile, + symlinkPolicy: params.symlinkPolicy, + }); + const previousContent = await readWorkspaceSkillFile(params.skillFile); + if (params.mode === "create" && previousContent !== null) { + throw new Error(`Target skill already exists: ${params.skillFile}`); + } + if (params.mode === "update" && previousContent === null) { + throw new Error(`Target skill is missing: ${params.skillFile}`); + } + + const preparedSupportFiles: PreparedWorkspaceSkillMutation["supportFiles"] = []; + for (const file of supportFiles) { + const filePath = path.join(params.skillDir, ...file.path.split("/")); + const target = await resolveWorkspaceSkillWriteTarget({ + workspaceDir: params.workspaceDir, + filePath, + symlinkPolicy: params.symlinkPolicy, + }); + const previousSupportContent = await readWorkspaceSupportFile({ + skillDir: params.skillDir, + relativePath: file.path, + }); + if (params.mode === "create" && previousSupportContent !== null) { + throw new Error(`Target support file already exists: ${filePath}`); + } + preparedSupportFiles.push({ + path: file.path, + filePath, + ...target, + previousContent: previousSupportContent, + content: file.content, + }); + } + + return { mode: params.mode, workspaceDir: params.workspaceDir, skillDir: params.skillDir, - skillFile: params.skillFile, - supportFiles, - symlinkPolicy: params.symlinkPolicy, - }); + skillFile: { + filePath: params.skillFile, + ...skillTarget, + previousContent, + content: params.content, + }, + supportFiles: preparedSupportFiles, + }; +} + +export async function applyWorkspaceSkillMutation( + mutation: PreparedWorkspaceSkillMutation, +): Promise { + const written: PreparedWorkspaceSkillFileMutation[] = []; const writtenSupportPaths: string[] = []; try { - for (const file of supportFiles) { - await writeWorkspaceFile({ - workspaceDir: params.workspaceDir, - filePath: path.join(params.skillDir, ...file.path.split("/")), - content: file.content, - overwrite: params.mode === "update", - symlinkPolicy: params.symlinkPolicy, - }); + for (const file of mutation.supportFiles) { + await writePreparedWorkspaceFile(file, mutation.mode === "update"); + written.push(file); writtenSupportPaths.push(file.path); } - await writeWorkspaceFile({ - workspaceDir: params.workspaceDir, - filePath: params.skillFile, - content: params.content, - overwrite: params.mode === "update", - symlinkPolicy: params.symlinkPolicy, - }); + await writePreparedWorkspaceFile(mutation.skillFile, mutation.mode === "update"); } catch (error) { - await restoreSupportFilesAfterFailedWrite({ - mode: params.mode, - workspaceDir: params.workspaceDir, - skillDir: params.skillDir, - writtenSupportPaths, - previousSupportFiles, - symlinkPolicy: params.symlinkPolicy, - }); + try { + await restorePreparedWorkspaceFiles(written.toReversed()); + } catch (restoreError) { + const failure = new Error( + `Skill write failed and ${writtenSupportPaths.length} support file restoration(s) failed.`, + { cause: error }, + ); + Object.assign(failure, { restoreError }); + throw failure; + } throw error; } } +export async function restoreWorkspaceSkillMutation( + mutation: PreparedWorkspaceSkillMutation, +): Promise { + // SKILL.md is the activation marker: restore support first for updates, but + // remove it first for failed creates so a partial new skill is not discoverable. + const files = + mutation.mode === "create" + ? [mutation.skillFile, ...mutation.supportFiles.toReversed()] + : [...mutation.supportFiles.toReversed(), mutation.skillFile]; + await restorePreparedWorkspaceFiles(files); +} + function normalizeSupportFiles( supportFiles: readonly WorkspaceSkillSupportFileWrite[], ): WorkspaceSkillSupportFileWrite[] { @@ -166,110 +243,45 @@ function normalizeSupportFiles( return normalized; } -async function prepareWorkspaceSkillWrite(params: { - mode: "create" | "update"; - workspaceDir: string; - skillDir: string; - skillFile: string; - supportFiles: readonly WorkspaceSkillSupportFileWrite[]; - symlinkPolicy: WorkspaceSkillSymlinkWritePolicy; -}): Promise { - await resolveWorkspaceSkillWriteTarget({ - workspaceDir: params.workspaceDir, - filePath: params.skillFile, - symlinkPolicy: params.symlinkPolicy, - }); - const previousContent = await readWorkspaceSkillFile(params.skillFile); - if (params.mode === "create" && previousContent !== null) { - throw new Error(`Target skill already exists: ${params.skillFile}`); - } - if (params.mode === "update" && previousContent === null) { - throw new Error(`Target skill is missing: ${params.skillFile}`); - } - - const previousSupportFiles: PreviousSupportFile[] = []; - for (const file of params.supportFiles) { - const filePath = path.join(params.skillDir, ...file.path.split("/")); - await resolveWorkspaceSkillWriteTarget({ - workspaceDir: params.workspaceDir, - filePath, - symlinkPolicy: params.symlinkPolicy, - }); - if (params.mode === "update") { - const previousSupportContent = await readWorkspaceSupportFile({ - skillDir: params.skillDir, - relativePath: file.path, - }); - previousSupportFiles.push( - previousSupportContent === null - ? { path: file.path, existed: false } - : { path: file.path, existed: true, previousContent: previousSupportContent }, - ); - } - } - return previousSupportFiles; -} - -async function writeWorkspaceFile(params: { - workspaceDir: string; - filePath: string; - content: string; - overwrite: boolean; - symlinkPolicy: WorkspaceSkillSymlinkWritePolicy; -}): Promise { - const target = await resolveWorkspaceSkillWriteTarget(params); - const targetRoot = await root(target.rootDir); - await targetRoot.write(target.relativePath, params.content, { +async function writePreparedWorkspaceFile( + file: PreparedWorkspaceSkillFileMutation, + overwrite: boolean, +): Promise { + const targetRoot = await root(file.rootDir); + await targetRoot.write(file.relativePath, file.content, { encoding: "utf8", mkdir: true, - overwrite: params.overwrite, + overwrite, }); } -async function removeWorkspaceFile(params: { - workspaceDir: string; - filePath: string; - symlinkPolicy: WorkspaceSkillSymlinkWritePolicy; -}): Promise { - const target = await resolveWorkspaceSkillWriteTarget(params); - const targetRoot = await root(target.rootDir); - await targetRoot.remove(target.relativePath).catch((error: unknown) => { - if ((error as { code?: string })?.code !== "ENOENT") { - throw error; - } - }); -} - -async function restoreSupportFilesAfterFailedWrite(params: { - mode: "create" | "update"; - workspaceDir: string; - skillDir: string; - writtenSupportPaths: readonly string[]; - previousSupportFiles: readonly PreviousSupportFile[]; - symlinkPolicy: WorkspaceSkillSymlinkWritePolicy; -}): Promise { - const previousByPath = new Map(params.previousSupportFiles.map((file) => [file.path, file])); - await Promise.allSettled( - params.writtenSupportPaths.toReversed().map(async (relativePath) => { - const filePath = path.join(params.skillDir, ...relativePath.split("/")); - const previous = previousByPath.get(relativePath); - if (params.mode === "update" && previous?.existed) { - await writeWorkspaceFile({ - workspaceDir: params.workspaceDir, - filePath, - content: previous.previousContent ?? "", - overwrite: true, - symlinkPolicy: params.symlinkPolicy, +async function restorePreparedWorkspaceFiles( + files: readonly PreparedWorkspaceSkillFileMutation[], +): Promise { + const errors: unknown[] = []; + for (const file of files) { + try { + const targetRoot = await root(file.rootDir); + if (file.previousContent === null) { + await targetRoot.remove(file.relativePath).catch((error: unknown) => { + if ((error as { code?: string })?.code !== "ENOENT") { + throw error; + } }); } else { - await removeWorkspaceFile({ - workspaceDir: params.workspaceDir, - filePath, - symlinkPolicy: params.symlinkPolicy, + await targetRoot.write(file.relativePath, file.previousContent, { + encoding: "utf8", + mkdir: true, + overwrite: true, }); } - }), - ); + } catch (error) { + errors.push(error); + } + } + if (errors.length > 0) { + throw new AggregateError(errors, "Failed to restore the previous workspace skill state."); + } } async function resolveWorkspaceSkillWriteTarget(