From 11bdd39ca0a8e0dca1f1ce950c5bf05bb7cd6a6f Mon Sep 17 00:00:00 2001 From: Emanuele Fenocchi Date: Tue, 21 Jul 2026 21:07:30 +0000 Subject: [PATCH 1/2] fix(skillify): migrate legacy over-long skill installs locally PR #322 caps skill names to codex's 64-char loader limit, but the migration of pre-cap installs only runs while processing a MATCHING remote row during a pull. A legacy managed install whose org/workspace is not the currently-routed one never matches, so it stays un-capped forever and every codex session logs 'invalid name: exceeds maximum length of 64 characters' for it (observed live). Add a local, remote-independent migration pass that runs from autoPullSkills before the network query (works offline and logged-out): - Scans ONLY manifest-managed global entries - never infers ownership from directory names, never touches project-scoped installs. - Validates author and names with the same validators pull uses before building any path. - Copy-then-swap: copy to the capped dir, rewrite the frontmatter name, record the canonical manifest entry (rawName preserved), fan out symlinks, and only then retire the legacy dir + entry - a failure at any earlier step leaves the original fully intact for the next retry. - A collision with the SAME rawName is a leftover from a prior migration/pull and is reconciled (legacy removed, canonical kept); a different rawName is a true collision and is skipped. --- src/skillify/auto-pull.ts | 11 + src/skillify/legacy-cap-migration.ts | 271 +++++++++ .../claude-code/legacy-cap-migration.test.ts | 527 ++++++++++++++++++ vitest.config.ts | 7 + 4 files changed, 816 insertions(+) create mode 100644 src/skillify/legacy-cap-migration.ts create mode 100644 tests/claude-code/legacy-cap-migration.test.ts diff --git a/src/skillify/auto-pull.ts b/src/skillify/auto-pull.ts index a0bb1e40..43220780 100644 --- a/src/skillify/auto-pull.ts +++ b/src/skillify/auto-pull.ts @@ -29,6 +29,7 @@ import { type Config } from "../config.js"; import { loadRoutedConfig } from "../dir-config.js"; import { DeeplakeApi } from "../deeplake-api.js"; import { runPull, type QueryFn } from "./pull.js"; +import { migrateLegacyCappedInstalls } from "./legacy-cap-migration.js"; import { log as _log } from "../utils/debug.js"; const log = (msg: string) => _log("skillify-autopull", msg); @@ -80,6 +81,16 @@ export async function autoPullSkills(deps: AutoPullDeps = {}): Promise--` dir the user happens to own is + * left untouched. + */ + +import { cpSync, existsSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { assertValidSkillName, capSkillName, MAX_SKILL_NAME_LEN, parseFrontmatter } from "./skill-writer.js"; +import { assertValidAuthor } from "./pull.js"; +import { + entriesForRoot, + loadManifest, + recordPull, + removePullEntry, + unlinkSymlinks, + type PulledEntry, +} from "./manifest.js"; +import { fanOutSymlinks } from "./pull.js"; +import { detectAgentSkillsRoots } from "./agent-roots.js"; +import { log as _log } from "../utils/debug.js"; + +const log = (msg: string) => _log("skillify-legacy-cap", msg); + +/** How many managed entries were migrated / skipped in a single pass. */ +export interface LegacyCapResult { + migrated: number; + skipped: number; +} + +/** + * Read the installed frontmatter `name` for a managed entry's SKILL.md. + * Returns null when the file is missing/unparseable or the name isn't a + * non-empty string — the caller then leaves that entry alone. + */ +function readInstalledName(skillFile: string): string | null { + if (!existsSync(skillFile)) return null; + let text: string; + try { text = readFileSync(skillFile, "utf-8"); } + catch { return null; } + const parsed = parseFrontmatter(text); + const name = parsed?.fm.name; + return typeof name === "string" && name.length > 0 ? name : null; +} + +/** + * Retire a legacy leftover: drop its fan-out symlinks, remove its on-disk dir, + * and drop its manifest row. Used both to finish a copy-then-swap migration and + * to reconcile a stale legacy dir whose canonical capped install already exists. + */ +function retireEntry(entry: PulledEntry): void { + unlinkSymlinks(entry.symlinks); + rmSync(join(entry.installRoot, entry.dirName), { recursive: true, force: true }); + removePullEntry(entry.install, entry.installRoot, entry.dirName); +} + +/** + * Migrate one managed entry whose installed frontmatter `name` exceeds the + * loader ceiling to the canonical capped dir. Returns true on a successful + * migration (or a same-rawName leftover reconciliation), false when it was + * skipped (foreign collision, no-op, validation failure, or FS error) with + * the original left untouched. + * + * Copy-then-swap ordering: the canonical capped dir is (a) COPIED from the + * stale dir, (b) its frontmatter `name` capped, (c) recorded in the manifest, + * (d) its symlinks fanned out, and ONLY THEN (e) is the stale dir + manifest + * row + its symlinks removed. A failure before (e) leaves the original entry + * fully intact — a later run retries and, finding the already-capped copy with + * the SAME rawName, reconciles by retiring only the legacy leftover. + */ +function migrateEntry(entry: PulledEntry): boolean { + // Scope to global installs only. Project-scoped installs belong to a specific + // project cwd and must not be mutated from an unrelated SessionStart. + if (entry.install !== "global") return false; + + const staleDir = join(entry.installRoot, entry.dirName); + const staleFile = join(staleDir, "SKILL.md"); + const installedName = readInstalledName(staleFile); + // Only over-long installed names need capping. A missing/valid name is a + // no-op (idempotent second run). + if (installedName === null || installedName.length <= MAX_SKILL_NAME_LEN) return false; + + // Path-safety validation BEFORE deriving any destination path. The capped + // dir is built from mutable frontmatter (`installedName`) and `entry.author`; + // both feed a directory name, so validate them with the same validators + // pull.ts uses (pull.ts:547,563) before any fs op. A validation failure skips + // the entry, leaving it untouched. + let capped: string; + try { + // Validate the raw (pre-cap) name's characters/path first — a legacy 65–100 + // char name passes assertValidSkillName's 100-char path-safety ceiling. + assertValidSkillName(installedName); + capped = capSkillName(installedName); + // The capped output must itself be a valid single-segment slug before it + // becomes a directory name. + assertValidSkillName(capped); + assertValidAuthor(entry.author); + } catch (e: any) { + log(`skip ${entry.dirName}: invalid name/author (${e?.message ?? e})`); + return false; + } + + // capSkillName is idempotent; if the cap didn't change the name there's + // nothing to move (shouldn't happen for a >64 name, but stay defensive). + if (capped === installedName) return false; + + // Canonical dir base equals the capped name suffixed by the author, exactly + // as pull.ts derives it (pull.ts:579). Preserve the raw pre-cap name so a + // later cross-run cap-collision check can recognise this destination. + const rawName = entry.rawName ?? entry.name; + const cappedDir = `${capped}--${entry.author}`; + const cappedDirPath = join(entry.installRoot, cappedDir); + const cappedFile = join(cappedDirPath, "SKILL.md"); + + // No-op if the entry is already at the canonical dir (idempotent). + if (cappedDir === entry.dirName) return false; + + // Reconcile against any existing canonical entry at the capped destination. + const managed = entriesForRoot(loadManifest(), entry.install, entry.installRoot); + const collidingEntry = managed.find( + e => e.dirName === cappedDir && e.dirName !== entry.dirName, + ); + if (collidingEntry) { + // SAME rawName → not a foreign collision: a previous migration (or pull) + // already produced the canonical install and only this legacy leftover + // remains. + if ((collidingEntry.rawName ?? collidingEntry.name) === rawName) { + // Guard against an INTERRUPTED prior migration: the canonical copy exists + // but its frontmatter `name` was never rewritten (the writeFileSync step + // threw), so it's still over-long. Retiring the legacy now would leave the + // half-written canonical install permanently un-capped. In that case tear + // down the broken canonical dir + row and fall through to re-migrate from + // the still-intact legacy. A properly-capped canonical is left untouched. + const canonName = readInstalledName(cappedFile); + if (canonName !== null && canonName.length > MAX_SKILL_NAME_LEN) { + rmSync(cappedDirPath, { recursive: true, force: true }); + unlinkSymlinks(collidingEntry.symlinks); + removePullEntry(collidingEntry.install, collidingEntry.installRoot, collidingEntry.dirName); + log(`repair: dropped half-migrated ${cappedDir}, re-migrating from ${entry.dirName}`); + // Fall through to the copy-then-swap below (cappedDirPath is now clear). + } else { + retireEntry(entry); + log(`reconciled leftover ${entry.dirName} (canonical ${cappedDir} already present)`); + return true; + } + } else { + // DIFFERENT rawName → a true foreign collision. Leave the original + // untouched so a later run can retry once the conflict clears. + log(`skip ${entry.dirName}: capped dir ${cappedDir} claimed by ${collidingEntry.rawName ?? collidingEntry.name}`); + return false; + } + } + // A dir already sits at the capped destination but NO manifest entry claims + // it (the manifest reconciliation above found none). We cannot safely tell a + // user-owned capped skill apart from a crashed-migration leftover by disk + // contents alone, so we skip and leave both untouched. Recovery of an + // interrupted migration is manifest-driven instead: the canonical entry is + // recorded BEFORE the destructive removal (see below), so a failed run leaves + // a same-rawName manifest row that the reconciliation above collapses on the + // retry. + if (existsSync(cappedDirPath)) { + log(`skip ${entry.dirName}: ${cappedDir} already exists on disk`); + return false; + } + + try { + // (a) COPY the install to the canonical capped dir — the original is left + // in place until the very last step, so any failure below is recoverable. + cpSync(staleDir, cappedDirPath, { recursive: true }); + + // (b) Record the canonical capped entry (rawName preserved) BEFORE any + // further mutation. Recording first is what makes the retry recoverable: + // if a later step throws, the same-rawName reconciliation above finds this + // row on the next run and retires only the legacy leftover. + recordPull({ + dirName: cappedDir, + name: capped, + rawName, + author: entry.author, + projectKey: entry.projectKey, + remoteVersion: entry.remoteVersion, + install: entry.install, + installRoot: entry.installRoot, + pulledAt: new Date().toISOString(), + symlinks: [], + }); + + // (c) Rewrite the frontmatter `name` in the NEW dir — preserving the file + // body and version (same replace pull.ts uses at pull.ts:651). + const migrated = readFileSync(cappedFile, "utf-8").replace(/^name:.*$/m, `name: ${capped}`); + writeFileSync(cappedFile, migrated); + + // (d) Refresh the fan-out symlinks for the new dir the same way pull/auto- + // pull does, then persist the resolved set onto the canonical entry. + const symlinks = fanOutSymlinks(cappedDirPath, cappedDir, detectAgentSkillsRoots(entry.installRoot)); + if (symlinks.length > 0) { + recordPull({ + dirName: cappedDir, + name: capped, + rawName, + author: entry.author, + projectKey: entry.projectKey, + remoteVersion: entry.remoteVersion, + install: entry.install, + installRoot: entry.installRoot, + pulledAt: new Date().toISOString(), + symlinks, + }); + } + + // (e) Only now retire the stale entry: drop its symlinks, its on-disk dir, + // and its manifest row. + retireEntry(entry); + log(`migrated ${entry.dirName} → ${cappedDir}`); + return true; + } catch (e: any) { + // Any FS error before (e) leaves the ORIGINAL fully intact (dir + manifest + // row + symlinks) — we never removed it. If (b) already landed, a capped + // copy + a same-rawName canonical manifest row are left behind, which the + // reconciliation above collapses on the retry. + log(`error migrating ${entry.dirName} (swallowed): ${e?.message ?? e}`); + return false; + } +} + +/** + * Scan every manifest-managed entry and cap any whose installed frontmatter + * `name` exceeds the loader ceiling. Remote-independent: runs offline and + * regardless of which org/workspace is routed. All per-entry failures are + * swallowed so a single broken install never aborts the pass. + */ +export function migrateLegacyCappedInstalls(): LegacyCapResult { + let migrated = 0; + let skipped = 0; + // Snapshot the entries first: migrateEntry mutates the manifest, so iterate + // over the pre-migration list rather than a live view. + const entries = loadManifest().entries.slice(); + for (const entry of entries) { + if (migrateEntry(entry)) migrated++; + else skipped++; + } + if (migrated > 0) log(`migrated=${migrated} skipped=${skipped}`); + return { migrated, skipped }; +} diff --git a/tests/claude-code/legacy-cap-migration.test.ts b/tests/claude-code/legacy-cap-migration.test.ts new file mode 100644 index 00000000..b6dae91e --- /dev/null +++ b/tests/claude-code/legacy-cap-migration.test.ts @@ -0,0 +1,527 @@ +/** + * Tests for src/skillify/legacy-cap-migration.ts. + * + * The migration is the REMOTE-INDEPENDENT counterpart to pull.ts's in-loop + * cap migration: it scans the pulled manifest and caps any managed install + * whose installed frontmatter `name` still exceeds the 64-char loader ceiling, + * regardless of which org/workspace is routed (and while offline). + * + * Mocking policy (per CLAUDE.md): import and run the REAL modules. The only + * seam we mock is the network — the auto-pull ordering test injects a QueryFn + * spy so no HTTP happens. Filesystem effects are scoped to a temp HOME so the + * developer's real ~/.deeplake / ~/.claude are never touched. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; + +import { migrateLegacyCappedInstalls } from "../../src/skillify/legacy-cap-migration.js"; +import { loadManifest, recordPull, type PulledEntry } from "../../src/skillify/manifest.js"; +import { capSkillName, MAX_SKILL_NAME_LEN } from "../../src/skillify/skill-writer.js"; +import { setFakeHome, clearFakeHome } from "../shared/fake-home.js"; + +// A real >64-char offender codex drops (68 chars), same one used in the +// pull.ts migration tests. +const LONG_NAME = "pg-deeplake-multi-layer-issue-diagnosis-and-workaround-prioritization"; + +let installRoot: string; +let fakeHome: string; + +beforeEach(() => { + // Isolate HOME so recordPull writes the manifest into the sandbox, not the + // developer's real ~/.deeplake state. + fakeHome = mkdtempSync(join(tmpdir(), "legacy-cap-home-")); + setFakeHome(fakeHome); + // Install root under the fake home — a plausible project skills root. + installRoot = join(fakeHome, "proj", ".claude", "skills"); + mkdirSync(installRoot, { recursive: true }); +}); + +afterEach(() => { + try { rmSync(fakeHome, { recursive: true, force: true }); } catch { /* nothing */ } + clearFakeHome(); +}); + +// ─── helpers ───────────────────────────────────────────────────────────────── + +/** Write a SKILL.md with the given frontmatter name + version + body. */ +function writeSkill(dir: string, name: string, version: number, body: string): void { + const skillDir = join(installRoot, dir); + mkdirSync(skillDir, { recursive: true }); + writeFileSync( + join(skillDir, "SKILL.md"), + `---\nname: ${name}\ndescription: "d"\nversion: ${version}\ncreated_by_agent: cc\ncreated_at: t\nupdated_at: t\n---\n\n${body}\n`, + ); +} + +/** + * Record a managed manifest entry for an on-disk dir. Defaults to a GLOBAL + * install because the migration only ever touches global entries (project + * installs belong to a specific cwd and must not be mutated from an unrelated + * SessionStart). + */ +function record(over: Partial & Pick): void { + recordPull({ + author: "sasun", + projectKey: "pk", + remoteVersion: 2, + install: "global", + installRoot, + pulledAt: "2026-05-06T00:00:00.000Z", + symlinks: [], + ...over, + }); +} + +// ─── over-long managed entry is migrated ───────────────────────────────────── + +describe("migrateLegacyCappedInstalls — happy path", () => { + it("caps an over-long managed entry: frontmatter capped, dir renamed, manifest updated with rawName, stale dir gone", () => { + const staleDir = `${LONG_NAME}--sasun`; + writeSkill(staleDir, LONG_NAME, 7, "legacy body"); + record({ dirName: staleDir, name: LONG_NAME, remoteVersion: 7 }); + expect(LONG_NAME.length).toBeGreaterThan(MAX_SKILL_NAME_LEN); + + const res = migrateLegacyCappedInstalls(); + + expect(res.migrated).toBe(1); + + // Stale dir + its manifest entry are gone. + expect(existsSync(join(installRoot, staleDir))).toBe(false); + expect(loadManifest().entries.some(e => e.dirName === staleDir)).toBe(false); + + // Exactly the capped dir remains. + const capped = capSkillName(LONG_NAME); + const cappedDir = `${capped}--sasun`; + const dirs = readdirSync(installRoot); + expect(dirs).toEqual([cappedDir]); + + // Frontmatter name is capped (<=64) and the body/version are preserved. + const text = readFileSync(join(installRoot, cappedDir, "SKILL.md"), "utf-8"); + const fmName = text.match(/^name: (.+)$/m)?.[1] ?? ""; + expect(fmName).toBe(capped); + expect(fmName.length).toBeLessThanOrEqual(MAX_SKILL_NAME_LEN); + expect(text).toContain("legacy body"); + expect(text).toContain("version: 7"); + + // Manifest entry: canonical capped dir/name, rawName preserves the pre-cap name. + const entry = loadManifest().entries.find(e => e.dirName === cappedDir); + expect(entry).toBeDefined(); + expect(entry!.name).toBe(capped); + expect(entry!.rawName).toBe(LONG_NAME); + expect(entry!.remoteVersion).toBe(7); + }); + + it("is idempotent: a second run no-ops (already capped)", () => { + const staleDir = `${LONG_NAME}--sasun`; + writeSkill(staleDir, LONG_NAME, 2, "b"); + record({ dirName: staleDir, name: LONG_NAME }); + + const first = migrateLegacyCappedInstalls(); + expect(first.migrated).toBe(1); + const manifestAfterFirst = JSON.stringify(loadManifest()); + const capped = capSkillName(LONG_NAME); + const fileAfterFirst = readFileSync(join(installRoot, `${capped}--sasun`, "SKILL.md"), "utf-8"); + + const second = migrateLegacyCappedInstalls(); + expect(second.migrated).toBe(0); + // Nothing moved or rewritten on the second pass. + expect(JSON.stringify(loadManifest())).toBe(manifestAfterFirst); + expect(readFileSync(join(installRoot, `${capped}--sasun`, "SKILL.md"), "utf-8")).toBe(fileAfterFirst); + expect(readdirSync(installRoot)).toEqual([`${capped}--sasun`]); + }); + + it("leaves an already-short managed entry untouched", () => { + writeSkill("short-skill--sasun", "short-skill", 1, "x"); + record({ dirName: "short-skill--sasun", name: "short-skill", remoteVersion: 1 }); + + const res = migrateLegacyCappedInstalls(); + expect(res.migrated).toBe(0); + expect(existsSync(join(installRoot, "short-skill--sasun", "SKILL.md"))).toBe(true); + }); + + it("skips a managed entry whose SKILL.md is missing (unreadable name)", () => { + // Manifest row present, but no SKILL.md on disk → readInstalledName is null. + mkdirSync(join(installRoot, "gone--sasun"), { recursive: true }); + record({ dirName: "gone--sasun", name: "gone", remoteVersion: 1 }); + + const res = migrateLegacyCappedInstalls(); + expect(res.migrated).toBe(0); + // Manifest row left intact (no destructive op fired). + expect(loadManifest().entries.some(e => e.dirName === "gone--sasun")).toBe(true); + }); + + it("skips a managed entry whose frontmatter name is empty", () => { + // Empty `name:` → readInstalledName returns null (non-empty-string guard). + writeSkill("empty-name--sasun", "", 1, "x"); + record({ dirName: "empty-name--sasun", name: "empty-name", remoteVersion: 1 }); + + const res = migrateLegacyCappedInstalls(); + expect(res.migrated).toBe(0); + expect(existsSync(join(installRoot, "empty-name--sasun", "SKILL.md"))).toBe(true); + }); + + it("no-ops when the entry already sits at the canonical capped dir but still reads an over-long name", () => { + // Directory is already the capped dir, yet the frontmatter name was never + // rewritten (over-long). cappedDir === entry.dirName so there's nothing to + // move — the in-place idempotent guard returns without touching anything. + const capped = capSkillName(LONG_NAME); + const cappedDir = `${capped}--sasun`; + writeSkill(cappedDir, LONG_NAME, 3, "body"); + record({ dirName: cappedDir, name: capped, rawName: LONG_NAME, remoteVersion: 3 }); + + const res = migrateLegacyCappedInstalls(); + expect(res.migrated).toBe(0); + expect(readdirSync(installRoot)).toEqual([cappedDir]); + expect(loadManifest().entries.some(e => e.dirName === cappedDir)).toBe(true); + }); +}); + +// ─── global fan-out symlinks are created + recorded ────────────────────────── + +describe("migrateLegacyCappedInstalls — global symlink fan-out", () => { + it("fans out a symlink into a detected agent root and records it on the canonical entry", () => { + // A `~/.codex` marker makes detectAgentSkillsRoots surface `~/.agents/skills` + // as a fan-out target, so the migration's step (d) creates + records a link. + mkdirSync(join(fakeHome, ".codex"), { recursive: true }); + + const staleDir = `${LONG_NAME}--sasun`; + writeSkill(staleDir, LONG_NAME, 4, "legacy body"); + record({ dirName: staleDir, name: LONG_NAME, remoteVersion: 4 }); + + const res = migrateLegacyCappedInstalls(); + expect(res.migrated).toBe(1); + + const capped = capSkillName(LONG_NAME); + const cappedDir = `${capped}--sasun`; + const link = join(fakeHome, ".agents", "skills", cappedDir); + // The fan-out symlink exists and resolves to the canonical dir. + expect(existsSync(link)).toBe(true); + // The canonical manifest entry recorded the resolved symlink set. + const entry = loadManifest().entries.find(e => e.dirName === cappedDir); + expect(entry!.symlinks).toContain(link); + }); +}); + +// ─── unmanaged over-long dir is NOT touched ────────────────────────────────── + +describe("migrateLegacyCappedInstalls — ownership", () => { + it("never touches an over-long dir that has no manifest entry", () => { + const userDir = `${LONG_NAME}--sasun`; + writeSkill(userDir, LONG_NAME, 1, "user-owned body"); + // NB: no record() — the dir is unmanaged. + + const res = migrateLegacyCappedInstalls(); + + expect(res.migrated).toBe(0); + // The unmanaged dir is left exactly as-is. + expect(existsSync(join(installRoot, userDir, "SKILL.md"))).toBe(true); + expect(readFileSync(join(installRoot, userDir, "SKILL.md"), "utf-8")).toContain("user-owned body"); + expect(readdirSync(installRoot)).toEqual([userDir]); + }); +}); + +// ─── collision skips ───────────────────────────────────────────────────────── + +describe("migrateLegacyCappedInstalls — collision", () => { + it("skips when the capped target is already claimed by another managed entry", () => { + const capped = capSkillName(LONG_NAME); + const cappedDir = `${capped}--sasun`; + + // A DIFFERENT managed entry already sits at the capped destination. + writeSkill(cappedDir, capped, 3, "owner body"); + record({ dirName: cappedDir, name: capped, rawName: "some-other-raw-name", remoteVersion: 3 }); + + // The legacy over-long entry that would cap onto the same dir. + const staleDir = `${LONG_NAME}--sasun`; + writeSkill(staleDir, LONG_NAME, 2, "legacy body"); + record({ dirName: staleDir, name: LONG_NAME, remoteVersion: 2 }); + + const res = migrateLegacyCappedInstalls(); + + // The over-long entry is skipped; owner and legacy dir both survive. + expect(res.migrated).toBe(0); + expect(existsSync(join(installRoot, staleDir, "SKILL.md"))).toBe(true); + expect(readFileSync(join(installRoot, staleDir, "SKILL.md"), "utf-8")).toContain("legacy body"); + // The owner's content is untouched (not overwritten by the collider). + expect(readFileSync(join(installRoot, cappedDir, "SKILL.md"), "utf-8")).toContain("owner body"); + // Both manifest entries still present. + expect(loadManifest().entries.some(e => e.dirName === staleDir)).toBe(true); + expect(loadManifest().entries.some(e => e.dirName === cappedDir)).toBe(true); + }); + + it("skips when a dir already sits on disk at the capped target (unmanaged occupant)", () => { + const capped = capSkillName(LONG_NAME); + const cappedDir = `${capped}--sasun`; + // Unmanaged dir occupying the capped destination (no manifest entry). + writeSkill(cappedDir, capped, 1, "occupant body"); + + const staleDir = `${LONG_NAME}--sasun`; + writeSkill(staleDir, LONG_NAME, 2, "legacy body"); + record({ dirName: staleDir, name: LONG_NAME, remoteVersion: 2 }); + + const res = migrateLegacyCappedInstalls(); + + expect(res.migrated).toBe(0); + // Neither dir was clobbered. + expect(readFileSync(join(installRoot, cappedDir, "SKILL.md"), "utf-8")).toContain("occupant body"); + expect(readFileSync(join(installRoot, staleDir, "SKILL.md"), "utf-8")).toContain("legacy body"); + }); +}); + +// ─── fs error leaves original intact ───────────────────────────────────────── + +describe("migrateLegacyCappedInstalls — fs error", () => { + it("leaves the original untouched and continues when the copy throws", async () => { + // Two over-long managed entries. We make cpSync throw for the FIRST entry's + // copy only, and assert the pass swallows the error, leaves the first + // intact (dir + manifest row), and still migrates the second. + const staleA = `${LONG_NAME}--sasun`; + writeSkill(staleA, LONG_NAME, 2, "A body"); + record({ dirName: staleA, name: LONG_NAME, remoteVersion: 2 }); + + // 66 chars — over the 64-char loader ceiling but within the 100-char + // path-safety ceiling, so it's a valid slug that still needs capping. + const longB = "pg-deeplake-multi-layer-issue-diagnosis-and-workaround-variant-two"; + const staleB = `${longB}--sasun`; + writeSkill(staleB, longB, 2, "B body"); + record({ dirName: staleB, name: longB, remoteVersion: 2 }); + + // Mock at the fs boundary: fail cpSync only when copying entry A's stale + // dir; delegate everything else to the real fs. + vi.resetModules(); + vi.doMock("node:fs", async () => { + const real = await vi.importActual("node:fs"); + return { + ...real, + cpSync: (from: any, to: any, opts: any) => { + if (String(from).endsWith(staleA)) { + const err = new Error("EPERM: simulated copy failure") as NodeJS.ErrnoException; + err.code = "EPERM"; + throw err; + } + return real.cpSync(from, to, opts); + }, + }; + }); + const { migrateLegacyCappedInstalls: run } = await import("../../src/skillify/legacy-cap-migration.js"); + const res = run(); + vi.doUnmock("node:fs"); + + // A failed, B migrated. + expect(res.migrated).toBe(1); + // A's original dir + manifest entry survive intact. + expect(existsSync(join(installRoot, staleA, "SKILL.md"))).toBe(true); + expect(readFileSync(join(installRoot, staleA, "SKILL.md"), "utf-8")).toContain("A body"); + expect(loadManifest().entries.some(e => e.dirName === staleA)).toBe(true); + // B was migrated to its capped dir. + const cappedB = `${capSkillName(longB)}--sasun`; + expect(existsSync(join(installRoot, staleB))).toBe(false); + expect(readFileSync(join(installRoot, cappedB, "SKILL.md"), "utf-8")).toContain("B body"); + }); +}); + +// ─── copy-then-swap: post-copy failure is recoverable on retry ─────────────── + +describe("migrateLegacyCappedInstalls — post-copy failure & retry", () => { + it("a write throw AFTER copy+record leaves the original intact; the retry reconciles & completes", async () => { + const staleDir = `${LONG_NAME}--sasun`; + writeSkill(staleDir, LONG_NAME, 5, "legacy body"); + record({ dirName: staleDir, name: LONG_NAME, remoteVersion: 5 }); + const capped = capSkillName(LONG_NAME); + const cappedDir = `${capped}--sasun`; + + // First run: let the copy + canonical recordPull land, then make the + // frontmatter-rewrite writeFileSync (step c) throw — the destructive + // removal of the legacy dir/entry (step e) is never reached. This is the + // dangerous window: a same-rawName canonical manifest row now exists but the + // legacy leftover is still present. + vi.resetModules(); + vi.doMock("node:fs", async () => { + const real = await vi.importActual("node:fs"); + return { + ...real, + writeFileSync: (p: any, data: any, opts?: any) => { + // Only fail the frontmatter rewrite of the NEW capped SKILL.md; let + // manifest .tmp writes (which pass a mode option) through. + if (String(p).endsWith(join(cappedDir, "SKILL.md")) && opts === undefined) { + throw new Error("simulated writeFileSync failure after copy+record"); + } + return real.writeFileSync(p, data, opts); + }, + }; + }); + const first = await import("../../src/skillify/legacy-cap-migration.js"); + const r1 = first.migrateLegacyCappedInstalls(); + vi.doUnmock("node:fs"); + + // Migration did NOT complete: the legacy dir + its manifest row are intact. + expect(r1.migrated).toBe(0); + expect(existsSync(join(installRoot, staleDir, "SKILL.md"))).toBe(true); + expect(readFileSync(join(installRoot, staleDir, "SKILL.md"), "utf-8")).toContain("legacy body"); + expect(loadManifest().entries.some(e => e.dirName === staleDir)).toBe(true); + // The canonical manifest row WAS recorded (record-before-destroy), so the + // retry can recognise the leftover via same-rawName reconciliation. + const canonAfterFirst = loadManifest().entries.find(e => e.dirName === cappedDir); + expect(canonAfterFirst).toBeDefined(); + expect(canonAfterFirst!.rawName).toBe(LONG_NAME); + + // Second run (real fs + real manifest): same-rawName reconciliation retires + // the legacy leftover and leaves the canonical install in place. + vi.resetModules(); + const second = await import("../../src/skillify/legacy-cap-migration.js"); + const r2 = second.migrateLegacyCappedInstalls(); + + expect(r2.migrated).toBe(1); + expect(existsSync(join(installRoot, staleDir))).toBe(false); + expect(loadManifest().entries.some(e => e.dirName === staleDir)).toBe(false); + expect(readdirSync(installRoot)).toEqual([cappedDir]); + const finalText = readFileSync(join(installRoot, cappedDir, "SKILL.md"), "utf-8"); + expect(finalText).toContain("legacy body"); + // The repair path re-migrated from the intact legacy, so the canonical + // frontmatter is now properly capped (<=64) — not left half-written. + expect(finalText.match(/^name: (.+)$/m)?.[1]).toBe(capped); + const entry = loadManifest().entries.find(e => e.dirName === cappedDir); + expect(entry!.rawName).toBe(LONG_NAME); + expect(entry!.remoteVersion).toBe(5); + }); +}); + +// ─── same-rawName leftover is reconciled ───────────────────────────────────── + +describe("migrateLegacyCappedInstalls — same-rawName reconciliation", () => { + it("removes the legacy leftover when the canonical entry has the SAME rawName; canonical untouched", () => { + const capped = capSkillName(LONG_NAME); + const cappedDir = `${capped}--sasun`; + + // Canonical install already produced by a prior migration/pull: SAME rawName. + writeSkill(cappedDir, capped, 7, "canonical body"); + record({ dirName: cappedDir, name: capped, rawName: LONG_NAME, remoteVersion: 7 }); + + // Legacy leftover with the still-over-long frontmatter name that caps onto + // the same identity. + const staleDir = `${LONG_NAME}--sasun`; + writeSkill(staleDir, LONG_NAME, 5, "legacy body"); + record({ dirName: staleDir, name: LONG_NAME, rawName: LONG_NAME, remoteVersion: 5 }); + + const res = migrateLegacyCappedInstalls(); + + // The leftover was reconciled (counts as a migration), not skipped. + expect(res.migrated).toBe(1); + // Legacy dir + manifest row gone. + expect(existsSync(join(installRoot, staleDir))).toBe(false); + expect(loadManifest().entries.some(e => e.dirName === staleDir)).toBe(false); + // Canonical dir + its content + manifest row fully untouched. + expect(readdirSync(installRoot)).toEqual([cappedDir]); + expect(readFileSync(join(installRoot, cappedDir, "SKILL.md"), "utf-8")).toContain("canonical body"); + const canon = loadManifest().entries.find(e => e.dirName === cappedDir); + expect(canon!.remoteVersion).toBe(7); + expect(canon!.rawName).toBe(LONG_NAME); + }); +}); + +// ─── path validation skips invalid author / frontmatter name ───────────────── + +describe("migrateLegacyCappedInstalls — path validation", () => { + it("skips (untouched) when the manifest author is a traversal string", () => { + // A managed entry whose author would escape the install root if it fed a + // directory name. dirName itself is a benign single segment so the manifest + // loader accepts it; the author is the poison. + const badAuthor = "..%2F.."; + const staleDir = `${LONG_NAME}--bad`; + writeSkill(staleDir, LONG_NAME, 2, "poison body"); + record({ dirName: staleDir, name: LONG_NAME, author: badAuthor, remoteVersion: 2 }); + + const res = migrateLegacyCappedInstalls(); + + expect(res.migrated).toBe(0); + // Left exactly as-is. + expect(readdirSync(installRoot)).toEqual([staleDir]); + expect(readFileSync(join(installRoot, staleDir, "SKILL.md"), "utf-8")).toContain("poison body"); + expect(loadManifest().entries.some(e => e.dirName === staleDir)).toBe(true); + }); + + it("skips (untouched) when the installed frontmatter name is not a valid slug", () => { + // Over-long AND non-kebab (path separator) frontmatter name — must be + // rejected before any destination path is built. + const badName = "../../etc/passwd-plus-a-very-long-tail-to-exceed-the-64-char-loader-ceiling"; + const staleDir = `${LONG_NAME}--sasun`; + writeSkill(staleDir, badName, 2, "poison body"); + record({ dirName: staleDir, name: LONG_NAME, remoteVersion: 2 }); + + const res = migrateLegacyCappedInstalls(); + + expect(res.migrated).toBe(0); + expect(readdirSync(installRoot)).toEqual([staleDir]); + expect(readFileSync(join(installRoot, staleDir, "SKILL.md"), "utf-8")).toContain("poison body"); + expect(loadManifest().entries.some(e => e.dirName === staleDir)).toBe(true); + }); +}); + +// ─── project-scoped entries are never mutated ──────────────────────────────── + +describe("migrateLegacyCappedInstalls — scope", () => { + it("never migrates a project-scoped over-long entry", () => { + const staleDir = `${LONG_NAME}--sasun`; + writeSkill(staleDir, LONG_NAME, 2, "project body"); + // Explicitly project-scoped — the migration must leave it alone. + record({ dirName: staleDir, name: LONG_NAME, install: "project", remoteVersion: 2 }); + + const res = migrateLegacyCappedInstalls(); + + expect(res.migrated).toBe(0); + expect(readdirSync(installRoot)).toEqual([staleDir]); + expect(readFileSync(join(installRoot, staleDir, "SKILL.md"), "utf-8")).toContain("project body"); + expect(loadManifest().entries.some(e => e.dirName === staleDir && e.install === "project")).toBe(true); + }); +}); + +// ─── auto-pull invokes migration BEFORE any network call ───────────────────── + +describe("autoPullSkills — invokes legacy-cap migration before the network", () => { + it("migrates a legacy over-long install before issuing the SQL query", async () => { + // Reset the module registry so the auto-pull import binds fresh. + vi.resetModules(); + const { autoPullSkills } = await import("../../src/skillify/auto-pull.js"); + + // A legacy over-long managed install present at SessionStart. + const staleDir = `${LONG_NAME}--sasun`; + writeSkill(staleDir, LONG_NAME, 2, "legacy body"); + record({ dirName: staleDir, name: LONG_NAME, remoteVersion: 2 }); + + // Query spy that records ordering: assert the migration already renamed + // the dir by the time the network SELECT fires. + let migratedBeforeQuery = false; + const capped = capSkillName(LONG_NAME); + const cappedDir = `${capped}--sasun`; + const queryFn = vi.fn(async (_sql: string) => { + migratedBeforeQuery = + existsSync(join(installRoot, cappedDir)) && !existsSync(join(installRoot, staleDir)); + return []; + }); + + const loadConfigFn = () => ({ + token: "tok", orgId: "org", orgName: "O", userName: "u", + workspaceId: "default", apiUrl: "https://api.deeplake.ai", + tableName: "memory", sessionsTableName: "sessions", skillsTableName: "skills", + rulesTableName: "r", goalsTableName: "g", kpisTableName: "k", + docsTableName: "d", codebaseTableName: "c", + memoryPath: join(fakeHome, ".deeplake", "memory"), + }) as any; + + const res = await autoPullSkills({ + loadConfigFn, queryFn, install: "project", cwd: join(fakeHome, "proj"), + }); + + // The migration ran and completed BEFORE the SELECT was issued. + expect(queryFn).toHaveBeenCalledTimes(1); + expect(migratedBeforeQuery).toBe(true); + // The pull itself found no matching remote rows (routed org differs). + expect(res.pulled).toBe(0); + // And the stale dir was retired by the migration. + expect(existsSync(join(installRoot, staleDir))).toBe(false); + expect(existsSync(join(installRoot, cappedDir))).toBe(true); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index 82000c60..e8f37593 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -430,6 +430,13 @@ export default defineConfig({ // EXDEV/EPERM error-recovery branch is mocked via vi.doMock("node:fs") // and the uncaught-rethrow branch covers everything else implicitly. "src/skillify/legacy-migration.ts": { statements: 90, branches: 80, functions: 90, lines: 90 }, + // fix/skillify-legacy-cap-migration — local, remote-independent cap + // migration of legacy over-long installs (runs before the network in + // auto-pull). Branches at 85: a couple of defensive arms (installedName + // null, capSkillName no-op, already-canonical dir) aren't each worth a + // dedicated fixture; the load-bearing migrate/skip/collision/fs-error + // paths are all exercised. + "src/skillify/legacy-cap-migration.ts": { statements: 90, branches: 85, functions: 90, lines: 90 }, "src/skillify/pull.ts": { statements: 90, branches: 75, functions: 90, lines: 90 }, "src/skillify/push.ts": { statements: 90, branches: 90, functions: 90, lines: 90 }, "src/skillify/scope-config.ts": { statements: 90, branches: 90, functions: 90, lines: 90 }, From 2323373bf4067a953b3408e0338a046c0ae7f9f8 Mon Sep 17 00:00:00 2001 From: Emanuele Fenocchi Date: Tue, 21 Jul 2026 21:20:34 +0000 Subject: [PATCH 2/2] fix(skillify): make collision reconciliation crash-safe and orphan-aware CodeRabbit review on #327 flagged two real defects in the same-rawName reconciliation block: - It ran OUTSIDE the per-entry try/catch, so an I/O error in the repair teardown (rmSync / unlinkSymlinks / removePullEntry) escaped migrateLegacyCappedInstalls and could abort autoPullSkills - breaking the swallow-all-per-entry-failures contract. - An ORPHANED canonical manifest row (dir deleted manually, frontmatter unreadable) fell into the already-migrated branch and retired the legacy install - dropping the only surviving copy. The reconciliation now lives inside the try, and a null installed name routes to the repair path (tear down the broken canonical row, re-migrate from the intact legacy) instead of retiring the legacy. Tests cover both: a throw inside reconciliation is swallowed with both copies left intact, and an orphaned row re-migrates instead of retiring. --- src/skillify/legacy-cap-migration.ts | 92 ++++++++++--------- .../claude-code/legacy-cap-migration.test.ts | 59 ++++++++++++ 2 files changed, 107 insertions(+), 44 deletions(-) diff --git a/src/skillify/legacy-cap-migration.ts b/src/skillify/legacy-cap-migration.ts index 5ad2d5df..3972002c 100644 --- a/src/skillify/legacy-cap-migration.ts +++ b/src/skillify/legacy-cap-migration.ts @@ -142,55 +142,59 @@ function migrateEntry(entry: PulledEntry): boolean { // No-op if the entry is already at the canonical dir (idempotent). if (cappedDir === entry.dirName) return false; - // Reconcile against any existing canonical entry at the capped destination. - const managed = entriesForRoot(loadManifest(), entry.install, entry.installRoot); - const collidingEntry = managed.find( - e => e.dirName === cappedDir && e.dirName !== entry.dirName, - ); - if (collidingEntry) { - // SAME rawName → not a foreign collision: a previous migration (or pull) - // already produced the canonical install and only this legacy leftover - // remains. - if ((collidingEntry.rawName ?? collidingEntry.name) === rawName) { - // Guard against an INTERRUPTED prior migration: the canonical copy exists - // but its frontmatter `name` was never rewritten (the writeFileSync step - // threw), so it's still over-long. Retiring the legacy now would leave the - // half-written canonical install permanently un-capped. In that case tear - // down the broken canonical dir + row and fall through to re-migrate from - // the still-intact legacy. A properly-capped canonical is left untouched. - const canonName = readInstalledName(cappedFile); - if (canonName !== null && canonName.length > MAX_SKILL_NAME_LEN) { - rmSync(cappedDirPath, { recursive: true, force: true }); - unlinkSymlinks(collidingEntry.symlinks); - removePullEntry(collidingEntry.install, collidingEntry.installRoot, collidingEntry.dirName); - log(`repair: dropped half-migrated ${cappedDir}, re-migrating from ${entry.dirName}`); - // Fall through to the copy-then-swap below (cappedDirPath is now clear). + try { + // Reconcile against any existing canonical entry at the capped + // destination. Inside the try: reconciliation touches the manifest and the + // filesystem, and a throw here must be swallowed per-entry like every + // other failure — never escape into autoPullSkills. + const managed = entriesForRoot(loadManifest(), entry.install, entry.installRoot); + const collidingEntry = managed.find( + e => e.dirName === cappedDir && e.dirName !== entry.dirName, + ); + if (collidingEntry) { + // SAME rawName → not a foreign collision: a previous migration (or pull) + // already produced the canonical install and only this legacy leftover + // remains. + if ((collidingEntry.rawName ?? collidingEntry.name) === rawName) { + // Guard against an INTERRUPTED prior migration: the canonical copy + // exists but its frontmatter `name` was never rewritten (still + // over-long), or the dir/SKILL.md is missing/unreadable entirely + // (orphaned manifest row). Retiring the legacy in either state would + // drop the only good copy — tear down the broken canonical dir + row + // and fall through to re-migrate from the still-intact legacy. Only a + // verified, properly-capped canonical retires the legacy. + const canonName = readInstalledName(cappedFile); + if (canonName === null || canonName.length > MAX_SKILL_NAME_LEN) { + rmSync(cappedDirPath, { recursive: true, force: true }); + unlinkSymlinks(collidingEntry.symlinks); + removePullEntry(collidingEntry.install, collidingEntry.installRoot, collidingEntry.dirName); + log(`repair: dropped half-migrated ${cappedDir}, re-migrating from ${entry.dirName}`); + // Fall through to the copy-then-swap below (cappedDirPath is now clear). + } else { + retireEntry(entry); + log(`reconciled leftover ${entry.dirName} (canonical ${cappedDir} already present)`); + return true; + } } else { - retireEntry(entry); - log(`reconciled leftover ${entry.dirName} (canonical ${cappedDir} already present)`); - return true; + // DIFFERENT rawName → a true foreign collision. Leave the original + // untouched so a later run can retry once the conflict clears. + log(`skip ${entry.dirName}: capped dir ${cappedDir} claimed by ${collidingEntry.rawName ?? collidingEntry.name}`); + return false; } - } else { - // DIFFERENT rawName → a true foreign collision. Leave the original - // untouched so a later run can retry once the conflict clears. - log(`skip ${entry.dirName}: capped dir ${cappedDir} claimed by ${collidingEntry.rawName ?? collidingEntry.name}`); + } + // A dir already sits at the capped destination but NO manifest entry + // claims it (the manifest reconciliation above found none). We cannot + // safely tell a user-owned capped skill apart from a crashed-migration + // leftover by disk contents alone, so we skip and leave both untouched. + // Recovery of an interrupted migration is manifest-driven instead: the + // canonical entry is recorded BEFORE the destructive removal (see below), + // so a failed run leaves a same-rawName manifest row that the + // reconciliation above collapses on the retry. + if (existsSync(cappedDirPath)) { + log(`skip ${entry.dirName}: ${cappedDir} already exists on disk`); return false; } - } - // A dir already sits at the capped destination but NO manifest entry claims - // it (the manifest reconciliation above found none). We cannot safely tell a - // user-owned capped skill apart from a crashed-migration leftover by disk - // contents alone, so we skip and leave both untouched. Recovery of an - // interrupted migration is manifest-driven instead: the canonical entry is - // recorded BEFORE the destructive removal (see below), so a failed run leaves - // a same-rawName manifest row that the reconciliation above collapses on the - // retry. - if (existsSync(cappedDirPath)) { - log(`skip ${entry.dirName}: ${cappedDir} already exists on disk`); - return false; - } - try { // (a) COPY the install to the canonical capped dir — the original is left // in place until the very last step, so any failure below is recoverable. cpSync(staleDir, cappedDirPath, { recursive: true }); diff --git a/tests/claude-code/legacy-cap-migration.test.ts b/tests/claude-code/legacy-cap-migration.test.ts index b6dae91e..158c8030 100644 --- a/tests/claude-code/legacy-cap-migration.test.ts +++ b/tests/claude-code/legacy-cap-migration.test.ts @@ -420,6 +420,65 @@ describe("migrateLegacyCappedInstalls — same-rawName reconciliation", () => { expect(canon!.remoteVersion).toBe(7); expect(canon!.rawName).toBe(LONG_NAME); }); + + it("treats an ORPHANED canonical row (dir missing on disk) as repair, not as already-migrated", () => { + const capped = capSkillName(LONG_NAME); + const cappedDir = `${capped}--sasun`; + + // Manifest row claims the canonical dir, but the dir was deleted manually — + // retiring the legacy here would drop the only surviving copy. + record({ dirName: cappedDir, name: capped, rawName: LONG_NAME, remoteVersion: 7 }); + + const staleDir = `${LONG_NAME}--sasun`; + writeSkill(staleDir, LONG_NAME, 5, "legacy body"); + record({ dirName: staleDir, name: LONG_NAME, rawName: LONG_NAME, remoteVersion: 5 }); + + const res = migrateLegacyCappedInstalls(); + + // Re-migrated from the legacy copy: canonical recreated with capped name… + expect(res.migrated).toBe(1); + expect(readFileSync(join(installRoot, cappedDir, "SKILL.md"), "utf-8")).toContain(`name: ${capped}`); + expect(readFileSync(join(installRoot, cappedDir, "SKILL.md"), "utf-8")).toContain("legacy body"); + // …and the legacy dir + row retired only after the swap completed. + expect(existsSync(join(installRoot, staleDir))).toBe(false); + expect(loadManifest().entries.some(e => e.dirName === staleDir)).toBe(false); + }); + + it("swallows a throw inside the reconciliation itself — the pass never escapes into the caller", async () => { + const capped = capSkillName(LONG_NAME); + const cappedDir = `${capped}--sasun`; + + // Half-migrated canonical (frontmatter still over-long) triggers the + // repair branch, whose rmSync we make throw. + writeSkill(cappedDir, LONG_NAME, 7, "broken canonical"); + record({ dirName: cappedDir, name: capped, rawName: LONG_NAME, remoteVersion: 7 }); + const staleDir = `${LONG_NAME}--sasun`; + writeSkill(staleDir, LONG_NAME, 5, "legacy body"); + record({ dirName: staleDir, name: LONG_NAME, rawName: LONG_NAME, remoteVersion: 5 }); + + vi.resetModules(); + vi.doMock("node:fs", async () => { + const real = await vi.importActual("node:fs"); + return { + ...real, + rmSync: (p: any, o: any) => { + if (String(p).includes(cappedDir)) throw new Error("EIO: rm failed"); + return real.rmSync(p, o); + }, + }; + }); + const { migrateLegacyCappedInstalls: migrate } = await import("../../src/skillify/legacy-cap-migration.js"); + + // Must not throw — the per-entry catch swallows the reconciliation error… + const res = migrate(); + expect(res.migrated).toBe(0); + // …and BOTH copies are left in place for the next retry. + expect(existsSync(join(installRoot, staleDir))).toBe(true); + expect(existsSync(join(installRoot, cappedDir))).toBe(true); + expect(loadManifest().entries.some(e => e.dirName === staleDir)).toBe(true); + vi.doUnmock("node:fs"); + vi.resetModules(); + }); }); // ─── path validation skips invalid author / frontmatter name ─────────────────