From 95774ba71fda2ec48e8cccaa58a4641936d6e726 Mon Sep 17 00:00:00 2001 From: "paseo-bot[bot]" <266920839+paseo-bot[bot]@users.noreply.github.com> Date: Thu, 1 Oct 2026 12:04:05 +0200 Subject: [PATCH] Show each Paseo skill once in Codex (#5827) * fix(skills): install one Codex-discoverable orchestration copy * fix(skills): keep installed skills when agent skill roots are symlinked together Retiring the old Codex copy deleted the skills just installed when ~/.codex/skills resolved to ~/.agents/skills or ~/.claude/skills (or the reverse), and status reported drift forever. The Codex skill directory is now treated as a leftover only when it is a separate directory from the shared and Claude copies. Co-authored-by: 3ae3ae * test(e2e): expect agent skills in the shared and Claude roots only The skills settings spec still expected every skill in ~/.codex/skills. It now checks the two install roots and that ~/.codex/skills holds no Paseo skill, which fails against the previous installer. --------- Co-authored-by: 3ae3ae Co-authored-by: Bae Seongmin <83870598+3ae3ae@users.noreply.github.com> Co-authored-by: paseo-bot[bot] <266920839+paseo-bot[bot]@users.noreply.github.com> --- docs/data-model.md | 6 +- .../app/e2e/support/helpers/agent-skills.ts | 7 +- .../internal/controller.test.ts | 17 +-- .../internal/operations.test.ts | 107 ++++++++++++++++-- .../internal/operations.ts | 28 ++++- .../internal/sync.test.ts | 17 +-- .../orchestration-skills/internal/sync.ts | 56 ++++++++- 7 files changed, 199 insertions(+), 39 deletions(-) diff --git a/docs/data-model.md b/docs/data-model.md index 8c63e67d0b..25a214b11c 100644 --- a/docs/data-model.md +++ b/docs/data-model.md @@ -182,8 +182,10 @@ Terminal activity contributes to the workspace status bucket **per `workspaceId` Single file, validated with `PersistedConfigSchema`. `agents.skills.selection` is the daemon host's orchestration-skill preference. Missing means -`{ mode: "all" }`. Installed state is not persisted; the daemon derives it from its three managed -skill directories and keeps config plus filesystem convergence behind one serialized owner. +`{ mode: "all" }`. Installed state is not persisted; the daemon derives it from the shared +`~/.agents/skills` and Claude skill directories and keeps config plus filesystem convergence behind +one serialized owner. Codex discovers the shared directory. Updates retire unchanged files from +Paseo-managed copies in `~/.codex/skills`, preserving user edits and untracked files. `paseo reload` reads and validates this file once inside the daemon. That snapshot drives resolution, classification, application, and reload bookkeeping. `DaemonConfigStore` owns applying runtime-safe diff --git a/packages/app/e2e/support/helpers/agent-skills.ts b/packages/app/e2e/support/helpers/agent-skills.ts index 76c7a56d4a..5d8c15633c 100644 --- a/packages/app/e2e/support/helpers/agent-skills.ts +++ b/packages/app/e2e/support/helpers/agent-skills.ts @@ -22,7 +22,7 @@ type AgentSkillsClient = Pick< "close" | "connect" | "getAgentSkillsStatus" | "saveAgentSkillsSelection" >; -export type SkillTargetName = "agents" | "claude" | "codex"; +export type SkillTargetName = "agents" | "claude"; export interface AgentSkillsSandbox { available: string[]; @@ -30,6 +30,7 @@ export interface AgentSkillsSandbox { daemon: IsolatedHostDaemon; home: string; targets: Record; + codexSkillsDir: string; blockAgentsDirectory(): Promise; unblockAgentsDirectory(): Promise; close(): Promise; @@ -63,8 +64,8 @@ export async function startAgentSkillsSandbox(): Promise { const targets = { agents: path.join(home, ".agents", "skills"), claude: path.join(home, ".claude", "skills"), - codex: path.join(home, ".codex", "skills"), }; + const codexSkillsDir = path.join(home, ".codex", "skills"); return { available, @@ -72,6 +73,7 @@ export async function startAgentSkillsSandbox(): Promise { daemon, home, targets, + codexSkillsDir, blockAgentsDirectory: async () => { await mkdir(path.dirname(targets.agents), { recursive: true }); await writeFile(targets.agents, "not a directory", "utf8"); @@ -196,6 +198,7 @@ export async function expectInstalledSkills( for (const directory of Object.values(sandbox.targets)) { await expect.poll(() => readInstalledSkills(directory)).toEqual([...expected].sort()); } + await expect.poll(() => readInstalledSkills(sandbox.codexSkillsDir)).toEqual([]); } export async function expectSavedSelection( diff --git a/packages/server/src/server/orchestration-skills/internal/controller.test.ts b/packages/server/src/server/orchestration-skills/internal/controller.test.ts index d3dab5aa78..23b541e67d 100644 --- a/packages/server/src/server/orchestration-skills/internal/controller.test.ts +++ b/packages/server/src/server/orchestration-skills/internal/controller.test.ts @@ -210,7 +210,7 @@ async function blockAgentsDir(targets: SkillTargets): Promise { } async function isInstalled(targets: SkillTargets, name: string): Promise { - const dirs = [targets.agentsDir, targets.claudeDir, targets.codexDir]; + const dirs = [targets.agentsDir, targets.claudeDir]; const present = await Promise.all( dirs.map((dir) => access(path.join(dir, name)) @@ -454,7 +454,7 @@ describe("skills controller", () => { expect(await installedEverywhere(readOnly.targets)).toEqual([ ["paseo", "paseo-loop"], ["paseo", "paseo-loop"], - ["paseo", "paseo-loop"], + ["paseo-loop"], ]); expect(await readUserFile(readOnly.targets, "paseo-loop", "notes/mine.md")).toEqual([ "hand written", @@ -575,6 +575,7 @@ describe("skills controller", () => { }; const next: SkillSelection = { mode: "custom", skills: ["paseo"] }; await harness.controller.save(previous); + await writeUserFile(harness.targets, "paseo-loop", "SKILL.md", "paseo-loop-v1"); const livePaths = [ harness.targets.agentsDir, harness.targets.claudeDir, @@ -601,6 +602,7 @@ describe("skills controller", () => { }; const next: SkillSelection = { mode: "custom", skills: ["paseo"] }; await harness.controller.save(previous); + await writeUserFile(harness.targets, "paseo-loop", "SKILL.md", "paseo-loop-v1"); const transaction = await beginSkillsTransaction(harness.targets, previous, next, [ { kind: "delete", name: "paseo-loop" }, @@ -666,6 +668,7 @@ describe("skills controller", () => { }; const next: SkillSelection = { mode: "custom", skills: ["paseo"] }; await harness.controller.save(previous); + await writeUserFile(harness.targets, "paseo-loop", "SKILL.md", "paseo-loop-v1"); await writeUserFile(harness.targets, "paseo-loop", "notes/mine.md", "staged notes"); const transaction = await beginSkillsTransaction(harness.targets, previous, next, [ @@ -1115,7 +1118,7 @@ describe("skills controller", () => { expect(await installedEverywhere(harness.targets)).toEqual([ ["paseo", "paseo-advisor", "paseo-loop"], ["paseo", "paseo-advisor", "paseo-loop"], - ["paseo", "paseo-advisor", "paseo-loop"], + ["paseo-advisor"], ]); expect(result.selection).toEqual({ mode: "custom", skills: ["paseo", "paseo-loop"] }); }); @@ -1131,7 +1134,7 @@ describe("skills controller", () => { expect(result.confirmationRequired).toBeNull(); expect(result.selection).toEqual({ mode: "custom", skills: ["paseo"] }); - expect(await installedEverywhere(harness.targets)).toEqual([["paseo"], ["paseo"], ["paseo"]]); + expect(await installedEverywhere(harness.targets)).toEqual([["paseo"], ["paseo"], []]); }); it("asks again when another directory appears before the retry", async () => { @@ -1150,7 +1153,7 @@ describe("skills controller", () => { 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"], + ["paseo-chat"], ]); }); @@ -1179,7 +1182,7 @@ describe("skills controller", () => { const result = await harness.controller.save({ mode: "custom", skills: ["paseo"] }); expect(result.confirmationRequired).toBeNull(); - expect(await installedEverywhere(harness.targets)).toEqual([["paseo"], ["paseo"], ["paseo"]]); + expect(await installedEverywhere(harness.targets)).toEqual([["paseo"], ["paseo"], []]); }); it("preserves a regular file at a skill path when save convergence fails", async () => { @@ -1211,7 +1214,7 @@ describe("skills controller", () => { ]); expect(saved.selection).toEqual({ mode: "custom", skills: ["paseo"] }); - expect(await installedEverywhere(harness.targets)).toEqual([["paseo"], ["paseo"], ["paseo"]]); + expect(await installedEverywhere(harness.targets)).toEqual([["paseo"], ["paseo"], []]); expect(await harness.controller.status()).toEqual({ state: "up-to-date", ops: [], diff --git a/packages/server/src/server/orchestration-skills/internal/operations.test.ts b/packages/server/src/server/orchestration-skills/internal/operations.test.ts index 6b188d24b0..14363c4b42 100644 --- a/packages/server/src/server/orchestration-skills/internal/operations.test.ts +++ b/packages/server/src/server/orchestration-skills/internal/operations.test.ts @@ -1,4 +1,5 @@ import { promises as fs } from "node:fs"; +import { createHash } from "node:crypto"; import os from "node:os"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; @@ -298,8 +299,8 @@ describe("custom skill selection", () => { 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")).toEqual([true, true, false]); + expect(await installedIn(sandbox.targets, "paseo-loop")).toEqual([true, true, false]); expect(await installedIn(sandbox.targets, "paseo-advisor")).toEqual([false, false, false]); }); @@ -322,7 +323,7 @@ describe("custom skill selection", () => { 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")).toEqual([true, true, false]); expect(await installedIn(sandbox.targets, "paseo-loop")).toEqual([false, false, false]); expect(await installedIn(sandbox.targets, "paseo-advisor")).toEqual([false, false, false]); }); @@ -395,7 +396,7 @@ describe("installSkills / updateSkills", () => { await fs.rm(sandbox.root, { recursive: true, force: true }); }); - it("installs from a clean machine, populates all three targets, and leaves user dirs alone", async () => { + it("installs into shared and Claude roots without duplicating Codex discovery", async () => { await writeCurrentBundle(sandbox.targets.sourceDir); await writeOnDiskSkill(sandbox.targets.agentsDir, "unslop", { "SKILL.md": "user-unslop" }); @@ -411,9 +412,7 @@ describe("installSkills / updateSkills", () => { expect( await fs.readFile(path.join(sandbox.targets.agentsDir, name, "SKILL.md"), "utf-8"), ).toBe(name === "paseo" ? "paseo-v1" : "loop-v1"); - expect( - await fs.readFile(path.join(sandbox.targets.codexDir, name, "SKILL.md"), "utf-8"), - ).toBe(name === "paseo" ? "paseo-v1" : "loop-v1"); + expect(await pathExists(path.join(sandbox.targets.codexDir, name, "SKILL.md"))).toBe(false); expect(await pathExists(path.join(sandbox.targets.claudeDir, name))).toBe(true); } expect( @@ -421,6 +420,100 @@ describe("installSkills / updateSkills", () => { ).toBe("user-unslop"); }); + it("retires unchanged managed Codex copies while preserving personal files", async () => { + await writeCurrentBundle(sandbox.targets.sourceDir); + await installSkills(sandbox.targets, ALL_SKILLS); + const legacy = path.join(sandbox.targets.codexDir, "paseo"); + const managed = { "SKILL.md": "paseo-v1", "references/old.md": "bundled reference" }; + const hashes = Object.fromEntries( + Object.entries(managed).map(([name, content]) => [ + name, + createHash("sha256").update(content).digest("hex"), + ]), + ); + await writeFiles(legacy, { + ...managed, + "notes/mine.md": "personal notes", + ".paseo-managed-files.json": JSON.stringify({ version: 1, files: hashes }), + }); + const status = await autoUpdateInstalledSkills(sandbox.targets, ALL_SKILLS); + expect(status.state).toBe("up-to-date"); + expect(status.ops).toEqual([]); + expect(await pathExists(path.join(legacy, "SKILL.md"))).toBe(false); + expect(await pathExists(path.join(legacy, "references/old.md"))).toBe(false); + expect(await fs.readFile(path.join(legacy, "notes/mine.md"), "utf8")).toBe("personal notes"); + expect(await autoUpdateInstalledSkills(sandbox.targets, ALL_SKILLS)).toEqual(status); + }); + + it("keeps edited or untracked Codex skill definitions", async () => { + await writeCurrentBundle(sandbox.targets.sourceDir); + await installSkills(sandbox.targets, ALL_SKILLS); + const originalHash = createHash("sha256").update("paseo-v1").digest("hex"); + await writeOnDiskSkill(sandbox.targets.codexDir, "paseo", { + "SKILL.md": "personalized skill", + ".paseo-managed-files.json": JSON.stringify({ + version: 1, + files: { "SKILL.md": originalHash }, + }), + }); + await writeOnDiskSkill(sandbox.targets.codexDir, "paseo-loop", { + "SKILL.md": "untracked skill", + }); + expect((await autoUpdateInstalledSkills(sandbox.targets, ALL_SKILLS)).state).toBe("up-to-date"); + expect( + await fs.readFile(path.join(sandbox.targets.codexDir, "paseo", "SKILL.md"), "utf8"), + ).toBe("personalized skill"); + expect( + await fs.readFile(path.join(sandbox.targets.codexDir, "paseo-loop", "SKILL.md"), "utf8"), + ).toBe("untracked skill"); + }); + + it.each([ + { alias: "codex", target: "agents" }, + { alias: "codex", target: "claude" }, + { alias: "claude", target: "codex" }, + ] as const)( + "keeps installed skills when the $alias root links to the $target root", + async ({ alias, target }) => { + await writeCurrentBundle(sandbox.targets.sourceDir); + const rootOf = { agents: "agentsDir", claude: "claudeDir", codex: "codexDir" } as const; + const aliasRoot = sandbox.targets[rootOf[alias]]; + const targetRoot = sandbox.targets[rootOf[target]]; + await fs.mkdir(targetRoot, { recursive: true }); + await fs.mkdir(path.dirname(aliasRoot), { recursive: true }); + await fs.symlink(targetRoot, aliasRoot, "dir"); + + const installed = await installSkills(sandbox.targets, ALL_SKILLS); + const updated = await autoUpdateInstalledSkills(sandbox.targets, ALL_SKILLS); + + expect(installed.state).toBe("up-to-date"); + expect(updated.state).toBe("up-to-date"); + for (const name of ["paseo", "paseo-loop"]) { + expect(await installedIn(sandbox.targets, name)).toEqual([true, true, true]); + } + }, + ); + + it("keeps an installed skill when its Codex directory links to the shared copy", async () => { + await writeCurrentBundle(sandbox.targets.sourceDir); + await fs.mkdir(path.join(sandbox.targets.agentsDir, "paseo"), { recursive: true }); + await fs.mkdir(sandbox.targets.codexDir, { recursive: true }); + await fs.symlink( + path.join(sandbox.targets.agentsDir, "paseo"), + path.join(sandbox.targets.codexDir, "paseo"), + "dir", + ); + + const installed = await installSkills(sandbox.targets, ALL_SKILLS); + const updated = await autoUpdateInstalledSkills(sandbox.targets, ALL_SKILLS); + + expect(installed.state).toBe("up-to-date"); + expect(updated.state).toBe("up-to-date"); + expect( + await fs.readFile(path.join(sandbox.targets.agentsDir, "paseo", "SKILL.md"), "utf8"), + ).toBe("paseo-v1"); + }); + 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" }); diff --git a/packages/server/src/server/orchestration-skills/internal/operations.ts b/packages/server/src/server/orchestration-skills/internal/operations.ts index a3b0831122..e45878a953 100644 --- a/packages/server/src/server/orchestration-skills/internal/operations.ts +++ b/packages/server/src/server/orchestration-skills/internal/operations.ts @@ -2,7 +2,13 @@ import { createHash } from "node:crypto"; import { promises as fs } from "node:fs"; import path from "node:path"; import type { AgentSkillSelection } from "@getpaseo/protocol/messages"; -import { listFilesRecursive, removeSkill, syncSkills } from "./sync.js"; +import { + isLegacyCodexCopy, + listFilesRecursive, + readManagedFilesManifest, + removeSkill, + syncSkills, +} from "./sync.js"; export type SkillsState = "not-installed" | "up-to-date" | "drift"; @@ -163,11 +169,23 @@ export async function getSkillsStatus( hashSkills(targets.claudeDir, names), hashSkills(targets.codexDir, names), ]); - const disks = [agentsDisk, claudeDisk, codexDisk]; - const ops = diff(bundle, disks, names, resolveDesiredSkills(selection, available)); - const installed = installedSkillNames(disks, names); + const disks = [agentsDisk, claudeDisk]; + const desired = resolveDesiredSkills(selection, available); + const ops = diff(bundle, disks, names, desired); + for (const name of codexDisk.keys()) { + if (ops.some((op) => op.name === name)) continue; + if (!(await isLegacyCodexCopy(name, targets))) continue; + if (!desired.has(name)) ops.push({ kind: "delete", name }); + else if (await readManagedFilesManifest(path.join(targets.codexDir, name))) { + ops.push({ kind: "update", name }); + } + } + ops.sort((a, b) => compareStrings(a.name, b.name)); + const allDisks = [...disks, codexDisk]; + const installed = installedSkillNames(allDisks, names); - if (!hasInstalledPaseoSkill(disks)) return { state: "not-installed", ops, available, installed }; + if (!hasInstalledPaseoSkill(allDisks)) + 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 }; } diff --git a/packages/server/src/server/orchestration-skills/internal/sync.test.ts b/packages/server/src/server/orchestration-skills/internal/sync.test.ts index da5b1c8765..1848330fdb 100644 --- a/packages/server/src/server/orchestration-skills/internal/sync.test.ts +++ b/packages/server/src/server/orchestration-skills/internal/sync.test.ts @@ -75,11 +75,9 @@ describe("syncSkills", () => { "utf-8", ); expect(claudeContent).toBe("new paseo content"); - const codexContent = await fs.readFile( - path.join(sandbox.codexDir, "paseo", "SKILL.md"), - "utf-8", - ); - expect(codexContent).toBe("new paseo content"); + await expect(fs.stat(path.join(sandbox.codexDir, "paseo", "SKILL.md"))).rejects.toMatchObject({ + code: "ENOENT", + }); }); it("installs new bundled skills, including references/, when not present on disk", async () => { @@ -105,12 +103,9 @@ describe("syncSkills", () => { "utf-8", ), ).toBe("roles content"); - expect( - await fs.readFile( - path.join(sandbox.codexDir, "paseo-committee", "references", "roles.md"), - "utf-8", - ), - ).toBe("roles content"); + await expect(fs.stat(path.join(sandbox.codexDir, "paseo-committee"))).rejects.toMatchObject({ + code: "ENOENT", + }); const claudeSkillDir = path.join(sandbox.claudeDir, "paseo-committee"); expect((await fs.lstat(claudeSkillDir)).isDirectory()).toBe(true); diff --git a/packages/server/src/server/orchestration-skills/internal/sync.ts b/packages/server/src/server/orchestration-skills/internal/sync.ts index 9233365549..255c61a274 100644 --- a/packages/server/src/server/orchestration-skills/internal/sync.ts +++ b/packages/server/src/server/orchestration-skills/internal/sync.ts @@ -51,7 +51,9 @@ export async function listFilesRecursive(rootDir: string): Promise { return out; } -async function readManagedFilesManifest(dstDir: string): Promise { +export async function readManagedFilesManifest( + dstDir: string, +): Promise { const raw = await fs .readFile(path.join(dstDir, MANAGED_FILES_MANIFEST), "utf-8") .catch(() => null); @@ -185,6 +187,50 @@ export async function removeSkill(skillName: string, targets: RemoveSkillTargets } } +async function isSameDirectory(a: string, b: string): Promise { + const [realA, realB] = await Promise.all([a, b].map((p) => fs.realpath(p).catch(() => null))); + return realA !== null && realA === realB; +} + +/** + * Codex also discovers .agents/skills, so its dedicated copy is a leftover. When + * the Codex skill directory resolves to the shared or Claude copy, it is that + * copy, and retiring it would delete the install. + */ +export async function isLegacyCodexCopy( + skillName: string, + targets: RemoveSkillTargets, +): Promise { + const codexSkillDir = path.join(targets.codexDir, skillName); + for (const root of [targets.agentsDir, targets.claudeDir]) { + if (await isSameDirectory(codexSkillDir, path.join(root, skillName))) return false; + } + return true; +} + +async function retireManagedSkill(dstDir: string): Promise { + const info = await fs.lstat(dstDir).catch(() => null); + if (!info?.isDirectory()) return 0; + const manifest = await readManagedFilesManifest(dstDir); + if (!manifest) return 0; + const files = Object.keys(manifest.files); + await assertManagedPathsStayInsideSkill(dstDir, [MANAGED_FILES_MANIFEST, ...files]); + const removed: string[] = []; + for (const [rel, previousHash] of Object.entries(manifest.files)) { + const filePath = path.join(dstDir, rel); + const currentHash = await hashFile(filePath).catch(() => null); + if (currentHash !== previousHash) continue; + await fs.rm(filePath); + removed.push(rel); + } + await pruneEmptyParentDirs(dstDir, removed); + await fs.rm(path.join(dstDir, MANAGED_FILES_MANIFEST)); + await fs.rmdir(dstDir).catch((error: NodeJS.ErrnoException) => { + if (error.code !== "ENOTEMPTY" && error.code !== "ENOENT") throw error; + }); + return removed.length + 1; +} + export async function syncSkills(options: SkillSyncOptions): Promise { let changedFiles = 0; let processedSkills = 0; @@ -206,10 +252,10 @@ export async function syncSkills(options: SkillSyncOptions): Promise