From 389486aecda8c66f67b0820a87a309b02fc98473 Mon Sep 17 00:00:00 2001 From: khustup2 Date: Mon, 19 Oct 2026 17:49:15 +0000 Subject: [PATCH] [hivemind-codex-skills] skillify: keep one version chain per own skill The gate could MERGE only into /.claude/skills, and pull wrote the user's own rows as -- beside the locally-mined . On a second machine or a nested repo the gate KEPT a new v1 under the same name, and pull installed the highest row as a second copy. - gate: the user's own global skill (also a legacy -- copy) is a merge target under its real name; a KEEP onto an existing merge target merges instead; a name collision keeps the higher version - pull: own rows install into the slot; skip when the slot or the cwd project copy has the same or a higher version; move a managed -- copy into a free slot; never record a locally-mined dir - skillify dedupe [--dry-run]: remove, move or replace existing -- copies of own skills (pull-managed entries only) --- .../skills/hivemind-memory/SKILL.md | 2 + .../codex/skills/deeplake-memory/SKILL.md | 2 + harnesses/pi/extension-source/hivemind.ts | 2 + src/cli/skillify-spec.ts | 9 + src/commands/skillify.ts | 27 ++ src/skillify/auto-pull.ts | 2 + src/skillify/dedupe.ts | 124 +++++++++ src/skillify/existing-skills.ts | 86 +++--- src/skillify/pull.ts | 104 ++++++-- src/skillify/skill-writer.ts | 4 +- src/skillify/skillify-worker.ts | 32 ++- tests/claude-code/skillify-cli.test.ts | 30 +++ .../skillify-existing-skills.test.ts | 14 +- .../skillify-own-skill-chain.test.ts | 245 ++++++++++++++++++ 14 files changed, 606 insertions(+), 77 deletions(-) create mode 100644 src/skillify/dedupe.ts create mode 100644 tests/claude-code/skillify-own-skill-chain.test.ts diff --git a/harnesses/claude-code/skills/hivemind-memory/SKILL.md b/harnesses/claude-code/skills/hivemind-memory/SKILL.md index 56670443f..6bf0b697e 100644 --- a/harnesses/claude-code/skills/hivemind-memory/SKILL.md +++ b/harnesses/claude-code/skills/hivemind-memory/SKILL.md @@ -68,6 +68,8 @@ Hivemind can mine reusable skills from agent session logs and share them across - `hivemind skillify unpull --user ` — remove only that author's pulls - `hivemind skillify unpull --not-mine` — remove all pulls except your own - `hivemind skillify unpull --dry-run` — preview without touching disk +- `hivemind skillify dedupe` — remove pulled -- copies of your own skills +- `hivemind skillify dedupe --dry-run` — preview without touching disk - `hivemind skillify scope ` — sharing scope for newly mined skills - `hivemind skillify install ` — default install location for new skills - `hivemind skillify promote ` — move a project skill to the global location diff --git a/harnesses/codex/skills/deeplake-memory/SKILL.md b/harnesses/codex/skills/deeplake-memory/SKILL.md index 88910a100..2e5cde84a 100644 --- a/harnesses/codex/skills/deeplake-memory/SKILL.md +++ b/harnesses/codex/skills/deeplake-memory/SKILL.md @@ -65,6 +65,8 @@ Hivemind can mine reusable skills from agent session logs and share them across - `hivemind skillify unpull --user ` — remove only that author's pulls - `hivemind skillify unpull --not-mine` — remove all pulls except your own - `hivemind skillify unpull --dry-run` — preview without touching disk +- `hivemind skillify dedupe` — remove pulled -- copies of your own skills +- `hivemind skillify dedupe --dry-run` — preview without touching disk - `hivemind skillify scope ` — sharing scope for newly mined skills - `hivemind skillify install ` — default install location for new skills - `hivemind skillify promote ` — move a project skill to the global location diff --git a/harnesses/pi/extension-source/hivemind.ts b/harnesses/pi/extension-source/hivemind.ts index 30be0ee2a..2a2f4cdf1 100644 --- a/harnesses/pi/extension-source/hivemind.ts +++ b/harnesses/pi/extension-source/hivemind.ts @@ -1352,6 +1352,8 @@ const PI_SKILLIFY_COMMANDS: { cmd: string; desc: string }[] = [ { cmd: "hivemind skillify unpull --user ", desc: "remove only that author's pulls" }, { cmd: "hivemind skillify unpull --not-mine", desc: "remove all pulls except your own" }, { cmd: "hivemind skillify unpull --dry-run", desc: "preview without touching disk" }, + { cmd: "hivemind skillify dedupe", desc: "remove pulled -- copies of your own skills" }, + { cmd: "hivemind skillify dedupe --dry-run", desc: "preview without touching disk" }, { cmd: "hivemind skillify scope ", desc: "sharing scope for newly mined skills" }, { cmd: "hivemind skillify install ", desc: "default install location for new skills" }, { cmd: "hivemind skillify promote ", desc: "move a project skill to the global location" }, diff --git a/src/cli/skillify-spec.ts b/src/cli/skillify-spec.ts index 564f67162..0ab3b348f 100644 --- a/src/cli/skillify-spec.ts +++ b/src/cli/skillify-spec.ts @@ -53,6 +53,8 @@ export const SKILLIFY_COMMANDS: SkillifyCommand[] = [ { cmd: "hivemind skillify unpull --user ", desc: "remove only that author's pulls" }, { cmd: "hivemind skillify unpull --not-mine", desc: "remove all pulls except your own" }, { cmd: "hivemind skillify unpull --dry-run", desc: "preview without touching disk" }, + { cmd: "hivemind skillify dedupe", desc: "remove pulled -- copies of your own skills" }, + { cmd: "hivemind skillify dedupe --dry-run", desc: "preview without touching disk" }, { cmd: "hivemind skillify scope ", desc: "sharing scope for newly mined skills" }, { cmd: "hivemind skillify install ", desc: "default install location for new skills" }, { cmd: "hivemind skillify promote ", desc: "move a project skill to the global location" }, @@ -126,6 +128,13 @@ export const SKILLIFY_SPEC: SkillifySubcommand[] = [ { flag: "--dry-run", desc: "preview without touching disk" }, ], }, + { + cmd: "hivemind skillify dedupe", + desc: "remove pulled -- copies of your own skills", + options: [ + { flag: "--dry-run", desc: "preview without touching disk" }, + ], + }, { cmd: "hivemind skillify scope", args: "", diff --git a/src/commands/skillify.ts b/src/commands/skillify.ts index f75ba777b..dbb1084d1 100644 --- a/src/commands/skillify.ts +++ b/src/commands/skillify.ts @@ -30,6 +30,7 @@ import { runPull, type PullSummary } from "../skillify/pull.js"; import { describeAgentSkillsRoots } from "../skillify/agent-roots.js"; import { runPush } from "../skillify/push.js"; import { runUnpull } from "../skillify/unpull.js"; +import { runDedupe } from "../skillify/dedupe.js"; import { loadConfig } from "../config.js"; import { loadRoutedConfig } from "../dir-config.js"; import { DeeplakeApi } from "../deeplake-api.js"; @@ -241,6 +242,8 @@ async function pullSkills(args: string[]): Promise { tableName: config.skillsTableName, install: toRaw, cwd: toRaw === "project" ? process.cwd() : undefined, + projectCwd: process.cwd(), + selfAuthor: config.userName, users, skillName, dryRun, @@ -409,6 +412,29 @@ async function unpullSkills(args: string[]): Promise { console.log(`Result: ${summary.removed} removed, ${summary.wouldRemove} dry-run, ${summary.kept} kept${prunedNote}.`); } +function dedupeSkills(args: string[]): void { + const dryRun = takeBooleanFlag([...args], "--dry-run"); + const config = loadConfig(); + if (!config) { + console.error("dedupe needs a logged-in user to know which skills are yours. Run: hivemind login"); + process.exit(1); + } + const entries = runDedupe({ myUsername: config.userName, cwd: process.cwd(), dryRun }); + console.log(`Own skills: ${config.userName}${dryRun ? " · dry-run" : ""}`); + const verb = { removed: "remove", moved: "move", replaced: "replace" } as const; + for (const e of entries) { + const tag = e.action === "kept" ? "· kept" + : e.action === "failed" ? "✗ failed" + : dryRun ? `→ would ${verb[e.action]}` : `✓ ${e.action}`; + console.log(` ${tag.padEnd(18)} ${e.dirName.padEnd(50)} (${e.reason})`); + } + const kept = entries.filter(e => e.action === "kept").length; + const failed = entries.filter(e => e.action === "failed").length; + const changed = entries.length - kept - failed; + console.log(`Result: ${changed} ${dryRun ? "to fix" : "fixed"}, ${kept} kept${failed ? `, ${failed} failed` : ""}.`); + if (failed) process.exit(1); +} + export function runSkillifyCommand(args: string[]): void { const sub = args[0]; if (!sub || sub === "status") { showStatus(); return; } @@ -433,6 +459,7 @@ export function runSkillifyCommand(args: string[]): void { .catch(() => { /* test-only safety net */ }); return; } + if (sub === "dedupe") { dedupeSkills(args.slice(1)); return; } if (sub === "unpull") { unpullSkills(args.slice(1)) .catch(e => { diff --git a/src/skillify/auto-pull.ts b/src/skillify/auto-pull.ts index 43220780b..7fe93f0df 100644 --- a/src/skillify/auto-pull.ts +++ b/src/skillify/auto-pull.ts @@ -144,6 +144,8 @@ export async function autoPullSkills(deps: AutoPullDeps = {}): Promise--` beside the locally-mined ``. + * + * Before pull wrote own rows into the `` slot, every own skill pulled + * back from the org table landed as `--`. With a locally-mined + * `` (same machine, another repo, or a second machine) the user saw + * two or more copies of one skill. Pull now keeps one dir per own skill; + * this command repairs installs made before that. + * + * Only pull-managed entries (in `pulled.json`) are candidates. Per entry: + * - a local `` at the same or a higher version exists → remove the copy + * - no local `` in the install root → move the copy into the `` slot + * - the install root's `` is older → replace its SKILL.md (old one kept + * as SKILL.md.bak), then remove the copy + * - only an older project `` exists → keep the copy (project files are + * the repo's; the user decides) + */ + +import { copyFileSync, existsSync, renameSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { loadManifest, recordPull, removePullEntry, unlinkSymlinks, type PulledEntry } from "./manifest.js"; +import { detectAgentSkillsRoots } from "./agent-roots.js"; +import { fanOutSymlinks, readLocalVersion, resolvePullDestination } from "./pull.js"; + +export interface DedupeOptions { + myUsername: string; + /** Project whose `.claude/skills/` also counts as a local copy. */ + cwd: string; + dryRun?: boolean; +} + +export type DedupeAction = "removed" | "moved" | "replaced" | "kept" | "failed"; + +export interface DedupeEntry { + dirName: string; + name: string; + path: string; + action: DedupeAction; + dryRun: boolean; + reason: string; +} + +function isOwnCopy(e: PulledEntry, me: string): boolean { + return e.author === me && e.dirName !== e.name && e.dirName.endsWith(`--${me}`); +} + +function removeCopy(e: PulledEntry): void { + rmSync(join(e.installRoot, e.dirName), { recursive: true, force: true }); + unlinkSymlinks(e.symlinks); + removePullEntry(e.install, e.installRoot, e.dirName); +} + +function moveCopy(e: PulledEntry): void { + const slot = join(e.installRoot, e.name); + renameSync(join(e.installRoot, e.dirName), slot); + unlinkSymlinks(e.symlinks); + removePullEntry(e.install, e.installRoot, e.dirName); + const symlinks = e.install === "global" ? fanOutSymlinks(slot, e.name, detectAgentSkillsRoots(e.installRoot)) : []; + recordPull({ ...e, dirName: e.name, symlinks }); +} + +function replaceSlot(e: PulledEntry): void { + const slotFile = join(e.installRoot, e.name, "SKILL.md"); + copyFileSync(slotFile, `${slotFile}.bak`); + copyFileSync(join(e.installRoot, e.dirName, "SKILL.md"), slotFile); + removeCopy(e); +} + +function decide(e: PulledEntry, cwd: string): { action: DedupeAction; reason: string } { + const copyVersion = readLocalVersion(join(e.installRoot, e.dirName, "SKILL.md")) ?? e.remoteVersion; + const slotVersion = readLocalVersion(join(e.installRoot, e.name, "SKILL.md")); + const projectVersion = e.install === "global" + ? readLocalVersion(join(resolvePullDestination("project", cwd), e.name, "SKILL.md")) + : null; + const localMax = Math.max(slotVersion ?? -1, projectVersion ?? -1); + if (localMax >= copyVersion) { + return { action: "removed", reason: `local ${e.name} v${localMax} >= copy v${copyVersion}` }; + } + const slotExists = existsSync(join(e.installRoot, e.name)); + if (slotExists && slotVersion === null) { + return { action: "kept", reason: `${e.name} exists without a version; resolve by hand` }; + } + if (!slotExists && projectVersion === null) { + return { action: "moved", reason: `no local ${e.name}; copy v${copyVersion} becomes ${e.name}` }; + } + if (slotVersion !== null) { + return { action: "replaced", reason: `local ${e.name} v${slotVersion} < copy v${copyVersion}` }; + } + return { action: "kept", reason: `project ${e.name} v${projectVersion} < copy v${copyVersion}; update the project copy by hand` }; +} + +const APPLY: Record, (e: PulledEntry) => void> = { + removed: removeCopy, + moved: moveCopy, + replaced: replaceSlot, +}; + +function apply( + e: PulledEntry, + decision: { action: DedupeAction; reason: string }, + dryRun: boolean, +): { action: DedupeAction; reason: string } { + if (dryRun || decision.action === "kept" || decision.action === "failed") return decision; + try { + APPLY[decision.action](e); + return decision; + } catch (err: any) { + return { action: "failed", reason: `${decision.action}: ${err?.message ?? err}` }; + } +} + +export function runDedupe(opts: DedupeOptions): DedupeEntry[] { + const dryRun = opts.dryRun ?? false; + const out: DedupeEntry[] = []; + for (const e of loadManifest().entries) { + if (!isOwnCopy(e, opts.myUsername)) continue; + const path = join(e.installRoot, e.dirName); + if (!existsSync(path)) continue; + const decision = decide(e, opts.cwd); + out.push({ dirName: e.dirName, name: e.name, path, dryRun, ...apply(e, decision, dryRun) }); + } + return out; +} diff --git a/src/skillify/existing-skills.ts b/src/skillify/existing-skills.ts index c9caca4ab..9665f7c15 100644 --- a/src/skillify/existing-skills.ts +++ b/src/skillify/existing-skills.ts @@ -23,7 +23,10 @@ import { listSkills, resolveSkillsRoot, parseFrontmatter } from "./skill-writer.js"; export interface TaggedSkill { + /** Name the gate sees and answers with. */ name: string; + /** Directory under the source root. Differs from `name` for a pulled `--` copy. */ + dirName: string; body: string; source: "project" | "global"; /** @@ -33,22 +36,37 @@ export interface TaggedSkill { * legacy local file isn't accidentally treated as cross-author. */ author?: string; + version: number; +} + +export interface MergeTarget { + install: "project" | "global"; + skillsRoot: string; + dirName: string; } export interface ExistingSkillsBlock { /** Names eligible as MERGE targets. Empty when no skills exist. */ mergeTargetNames: string[]; + /** Where each MERGE-eligible name lives on disk. */ + mergeTargets: Record; /** Rendered block of all skills (project + global) for the gate prompt. */ block: string; } /** * Collect every existing skill the gate should know about, with its - * source root + author tagged. If a name collides across roots, the - * project copy wins (the user is presumed to be actively editing it - * locally). + * source root + author tagged. + * + * `userName` lets the user's own pulled global copy (`--`) + * answer to its real ``, so the gate continues that version chain + * instead of starting a new v1 under the same name. + * + * Name collisions keep the higher version (a version only grows by a + * merge, so higher = newer); a tie keeps the project copy, which the + * user is presumed to be editing locally. */ -export function listAllExistingSkills(cwd: string): TaggedSkill[] { +export function listAllExistingSkills(cwd: string, userName?: string): TaggedSkill[] { const projectRoot = resolveSkillsRoot("project", cwd); const globalRoot = resolveSkillsRoot("global", cwd); const tag = (source: "project" | "global") => (s: { name: string; body: string }): TaggedSkill => { @@ -56,60 +74,53 @@ export function listAllExistingSkills(cwd: string): TaggedSkill[] { const author = typeof parsed?.fm.author === "string" && parsed.fm.author.length > 0 ? parsed.fm.author : undefined; - return { name: s.name, body: s.body, source, author }; + const version = typeof parsed?.fm.version === "number" ? parsed.fm.version : 1; + const fmName = typeof parsed?.fm.name === "string" ? parsed.fm.name : ""; + const ownPulledCopy = source === "global" && !!userName && author === userName + && s.name === `${fmName}--${userName}`; + return { name: ownPulledCopy ? fmName : s.name, dirName: s.name, body: s.body, source, author, version }; }; const tagged: TaggedSkill[] = [ ...listSkills(projectRoot).map(tag("project")), ...listSkills(globalRoot).map(tag("global")), ]; - const seen = new Set(); - const out: TaggedSkill[] = []; + const byName = new Map(); for (const s of tagged) { - if (seen.has(s.name)) continue; - seen.add(s.name); - out.push(s); + const kept = byName.get(s.name); + if (!kept || s.version > kept.version) byName.set(s.name, s); } - return out; + return [...byName.values()]; } /** * Render the gate-prompt block. `charCap` is the soft budget — once we * cross it, we emit a "[…N more omitted]" line and stop. * - * MERGE eligibility (post-#125 review): only `[project]` skills are - * returned in `mergeTargetNames`. A previous iteration of this PR opened - * MERGE to every rendered skill (including `[global]`), but the worker's - * mergeSkill path is rooted at `cfg.install` — for a `[global]` target - * the local file doesn't exist under that root, so mergeSkill throws and - * the fallback writes a brand-new skill (creating exactly the duplicate - * we set out to prevent). Until the worker carries the source root - * through the verdict and resolves a `/--` dirname - * for global targets, `[global]` skills stay reference-only here. - * Tracked as a follow-up (see PR #125 description). + * MERGE eligibility: `[project]` skills, and `[global]` skills authored by + * `userName`. A global skill from a teammate stays `[global, read-only]`: + * the worker writes MERGEs into the user's own skills only, and a + * teammate's chain continues on the teammate's machine. * - * Two narrower issues caught in the same review and fixed here: - * - `mergeTargetNames` must only include skills whose bodies were - * actually rendered into the prompt — otherwise the truncation tail - * leaks names the LLM never saw the context for. - * - The `[global, read-only]` tag is restored so the gate prompt's - * "MERGE only on [project]" rule has a stable surface to point at. + * `mergeTargetNames` only includes skills whose bodies were actually + * rendered into the prompt — otherwise the truncation tail leaks names + * the LLM never saw the context for. */ -export function renderExistingSkillsBlock(cwd: string, charCap: number): ExistingSkillsBlock { - const skills = listAllExistingSkills(cwd); +export function renderExistingSkillsBlock(cwd: string, charCap: number, userName?: string): ExistingSkillsBlock { + const skills = listAllExistingSkills(cwd, userName); if (skills.length === 0) { return { mergeTargetNames: [], + mergeTargets: {}, block: "(no existing skills — MERGE is NOT a valid choice; pick KEEP or SKIP only)", }; } let total = 0; const out: string[] = []; const mergeTargetNames: string[] = []; + const mergeTargets: Record = {}; for (const s of skills) { - // Tag carries both the install root and the author so the gate - // prompt can communicate "this one's yours, MERGE-eligible; this - // one's a teammate's pulled into your global root, reference only". - const sourceTag = s.source === "project" ? "project" : "global, read-only"; + const mergeable = s.source === "project" || (!!userName && s.author === userName); + const sourceTag = mergeable ? s.source : "global, read-only"; const authorTag = s.author ? `, author=${s.author}` : ""; const block = `--- existing skill [${sourceTag}${authorTag}]: ${s.name} ---\n${s.body}\n`; if (total + block.length > charCap) { @@ -118,10 +129,9 @@ export function renderExistingSkillsBlock(cwd: string, charCap: number): Existin } out.push(block); total += block.length; - // Only register the name once we know the block fit and the source - // is [project] — global skills are reference-only per the policy - // documented above. - if (s.source === "project") mergeTargetNames.push(s.name); + if (!mergeable) continue; + mergeTargetNames.push(s.name); + mergeTargets[s.name] = { install: s.source, skillsRoot: resolveSkillsRoot(s.source, cwd), dirName: s.dirName }; } - return { mergeTargetNames, block: out.join("\n") }; + return { mergeTargetNames, mergeTargets, block: out.join("\n") }; } diff --git a/src/skillify/pull.ts b/src/skillify/pull.ts index 44a968645..8fb29ab0a 100644 --- a/src/skillify/pull.ts +++ b/src/skillify/pull.ts @@ -68,6 +68,17 @@ export interface PullOptions { * couldn't be fetched) falls back to the SELECT-then-catch path below. */ tableExists?: (name: string) => boolean; + /** + * The current user. Their own rows install into the plain `` slot + * (where their locally-mined copy lives), not `--`, so one + * skill has one dir and the gate continues its version chain. + */ + selfAuthor?: string; + /** + * For global pulls: the project whose `.claude/skills/` also counts + * as the user's own copy. An own row at or below that version is skipped. + */ + projectCwd?: string; } export interface PullResultEntry { @@ -446,6 +457,29 @@ export function decideAction(args: { return args.dryRun ? "dryrun" : "wrote"; } +/** + * Move the user's own skill from a pull-managed legacy `--` dir + * into the `` slot, when that slot is free. Content and version are + * kept, so the normal version check runs on the moved copy. When the slot + * is already taken, the legacy dir stays for `skillify dedupe`. + */ +function adoptOwnLegacyDir(install: InstallLocation, root: string, name: string, legacyDirs: string[]): void { + const slot = join(root, name); + if (existsSync(slot)) return; + const entries = entriesForRoot(loadManifest(), install, root); + const legacy = entries.find(e => legacyDirs.includes(e.dirName) && existsSync(join(root, e.dirName))); + if (!legacy) return; + try { + renameSync(join(root, legacy.dirName), slot); + unlinkSymlinks(legacy.symlinks); + removePullEntry(install, root, legacy.dirName); + const symlinks = install === "global" ? fanOutSymlinks(slot, name, detectAgentSkillsRoots(root)) : []; + recordPull({ ...legacy, dirName: name, name, symlinks }); + } catch { + // Best effort: the legacy copy stays usable where it is. + } +} + /** * Main entry point. Queries the skills table, applies filters, decides * actions, and (unless dry-run) writes SKILL.md files to the destination. @@ -576,7 +610,25 @@ export async function runPull(opts: PullOptions): Promise { // install cleanly. The capped name drives BOTH the frontmatter `name` and the // `--` directory base so the two stay consistent. const name = capSkillName(rawName); - const dirName = `${name}--${author}`; + const own = !!opts.selfAuthor && author === opts.selfAuthor; + const dirName = own ? name : `${name}--${author}`; + + if (own) { + const projectFile = opts.install === "global" && opts.projectCwd + ? join(resolvePullDestination("project", opts.projectCwd), name, "SKILL.md") + : null; + const projectVersion = projectFile ? readLocalVersion(projectFile) : null; + if (projectFile && projectVersion !== null && projectVersion >= Number(row.version ?? 1)) { + summary.entries.push({ + name, remoteVersion: Number(row.version ?? 1), localVersion: projectVersion, + action: "skipped", destination: projectFile, + author, sourceAgent: String(row.source_agent ?? ""), + }); + summary.skipped++; + continue; + } + if (!opts.dryRun) adoptOwnLegacyDir(opts.install, root, name, [`${name}--${author}`, `${rawName}--${author}`]); + } // Two distinct over-long names can cap to the same destination (capping is // a lossy hash). Detect collisions both within this run (claimedDirs) AND @@ -613,7 +665,7 @@ export async function runPull(opts: PullOptions): Promise { // is to fix that existing broken install, which is normally at an equal // version. Below we either migrate the stale copy's content or install the // remote row, then retire the stale dir. - const staleDir = name !== rawName ? `${rawName}--${author}` : null; + const staleDir = !own && name !== rawName ? `${rawName}--${author}` : null; // Only treat a stale dir as ours to migrate/retire when the manifest says // WE pulled it. An unmanaged `--` dir the user happens to // own is left untouched, and its content is never copied into the capped @@ -631,6 +683,12 @@ export async function runPull(opts: PullOptions): Promise { dryRun: opts.dryRun ?? false, }); + // The user's own locally-mined skill in this slot is theirs, not pull's: + // update it to the newer version, but never record it, so `unpull` + // cannot delete it. + const unmanagedLocal = own && localVersion !== null + && !entriesForRoot(loadManifest(), opts.install, root).some(e => e.dirName === dirName); + let manifestError: string | undefined; if (action === "wrote") { mkdirSync(skillDir, { recursive: true }); @@ -660,32 +718,34 @@ export async function runPull(opts: PullOptions): Promise { // root, but only for global pulls. Project-local pulls live under // /.claude/skills and shouldn't leak into user-global agent // dirs — that would defeat the project-scoping intent. - const symlinks = opts.install === "global" + const symlinks = opts.install === "global" && !unmanagedLocal ? fanOutSymlinks(skillDir, dirName, detectAgentSkillsRoots(root)) : []; // Record in manifest so `unpull` can identify this entry as // pull-managed without relying on the `--` dirname heuristic // and so the symlinks created above can be reversed by a single // manifest-driven unlink pass. - try { - recordPull({ - dirName, - name, - rawName, - author, - projectKey: String(row.project_key ?? ""), - remoteVersion, - install: opts.install, - installRoot: root, - pulledAt: new Date().toISOString(), - symlinks, - }); - } catch (e: any) { - // Skill is on disk but the manifest didn't record it — surface - // this in the entry so the dispatcher can warn. `unpull` will - // not be able to clean this entry via the manifest path until - // a successful re-pull populates it. - manifestError = e?.message ?? String(e); + if (!unmanagedLocal) { + try { + recordPull({ + dirName, + name, + rawName, + author, + projectKey: String(row.project_key ?? ""), + remoteVersion, + install: opts.install, + installRoot: root, + pulledAt: new Date().toISOString(), + symlinks, + }); + } catch (e: any) { + // Skill is on disk but the manifest didn't record it — surface + // this in the entry so the dispatcher can warn. `unpull` will + // not be able to clean this entry via the manifest path until + // a successful re-pull populates it. + manifestError = e?.message ?? String(e); + } } } diff --git a/src/skillify/skill-writer.ts b/src/skillify/skill-writer.ts index c5d6ed338..3bc431957 100644 --- a/src/skillify/skill-writer.ts +++ b/src/skillify/skill-writer.ts @@ -63,6 +63,8 @@ export interface WriteSkillArgs { export interface MergeSkillArgs { skillsRoot: string; name: string; // existing skill to merge into + /** Directory of the target when it differs from `name` (a pulled `--` copy). */ + dirName?: string; description?: string; // optional override body: string; // merged body returned by gate newSourceSessions: string[]; @@ -331,7 +333,7 @@ export function writeNewSkill(args: WriteSkillArgs): SkillWriteResult { */ export function mergeSkill(args: MergeSkillArgs): SkillWriteResult { assertValidSkillName(args.name); - const path = skillPath(args.skillsRoot, args.name); + const path = skillPath(args.skillsRoot, args.dirName ?? args.name); if (!existsSync(path)) { throw new Error(`skill ${args.name} does not exist at ${path}; use writeNewSkill`); } diff --git a/src/skillify/skillify-worker.ts b/src/skillify/skillify-worker.ts index 28d98e6b9..ae3d810db 100644 --- a/src/skillify/skillify-worker.ts +++ b/src/skillify/skillify-worker.ts @@ -20,8 +20,9 @@ import { resolveSkillsRoot, writeNewSkill, mergeSkill, + capSkillName, } from "./skill-writer.js"; -import { renderExistingSkillsBlock } from "./existing-skills.js"; +import { renderExistingSkillsBlock, type ExistingSkillsBlock } from "./existing-skills.js"; import { insertSkillRow } from "./skills-table.js"; import { parseVerdict, type Verdict } from "./gate-parser.js"; import { runGate, type Agent } from "./gate-runner.js"; @@ -262,8 +263,7 @@ function renderPairsBlock(pairs: Pair[]): string { // unit-test the project+global merging and the [global, read-only] tagging // without instantiating the whole worker (which reads its config from argv). -function buildPrompt(pairs: Pair[]): string { - const existing = renderExistingSkillsBlock(cfg.cwd, EXISTING_SKILLS_CHAR_CAP); +function buildPrompt(pairs: Pair[], existing: ExistingSkillsBlock): string { const mergeTargetsClause = existing.mergeTargetNames.length > 0 ? `MERGE is allowed only if your "name" is EXACTLY one of: [${existing.mergeTargetNames.join(", ")}]. Any other name MUST use KEEP, not MERGE.` : `MERGE is FORBIDDEN — there are no existing skills to merge into. Use KEEP or SKIP only.`; @@ -390,7 +390,8 @@ async function main(): Promise { wlog(`extracted ${allPairs.length} pairs across ${usable.length} sessions`); - const prompt = buildPrompt(allPairs); + const existing = renderExistingSkillsBlock(cfg.cwd, EXISTING_SKILLS_CHAR_CAP, cfg.userName); + const prompt = buildPrompt(allPairs, existing); writeFileSync(promptPath, prompt); const gateAgent = (cfg.gateAgent ?? cfg.agent) as Agent; @@ -479,6 +480,7 @@ async function main(): Promise { contributors: string[]; }, verdict: Verdict, + install: "project" | "global" = cfg.install, ): Promise { // Author stamped on the DB row is the *original* author when one is // known (preserves lineage across merges); falls back to the current @@ -510,7 +512,7 @@ async function main(): Promise { project: cfg.project, projectKey: cfg.projectKey, localPath: result.path, - install: cfg.install, + install, sourceSessions, sourceAgent: cfg.agent, scope, @@ -533,6 +535,21 @@ async function main(): Promise { } } + // A KEEP under the name of an existing merge target would start a second + // v1 chain under that name (the fork users see as duplicates). Continue + // the existing chain instead. + const keepTarget = verdict.verdict === "KEEP" && verdict.name + ? existing.mergeTargets[capSkillName(verdict.name)] + : undefined; + if (keepTarget) { + wlog(`KEEP name matches existing skill ${verdict.name} — merging instead`); + verdict.verdict = "MERGE"; + verdict.name = capSkillName(verdict.name!); + } + const mergeTarget = verdict.verdict === "MERGE" && verdict.name + ? existing.mergeTargets[verdict.name] + : undefined; + if (verdict.verdict === "KEEP" && verdict.name && verdict.body) { try { const result = writeNewSkill({ @@ -558,7 +575,8 @@ async function main(): Promise { } else if (verdict.verdict === "MERGE" && verdict.name && verdict.body) { try { const result = mergeSkill({ - skillsRoot: resolveSkillsRoot(cfg.install, cfg.cwd), + skillsRoot: mergeTarget?.skillsRoot ?? resolveSkillsRoot(cfg.install, cfg.cwd), + dirName: mergeTarget?.dirName, name: verdict.name, description: verdict.description, body: verdict.body, @@ -568,7 +586,7 @@ async function main(): Promise { }); wlog(`merged into skill: ${result.path} (v${result.version})`); recordSkill(cfg.projectKey, verdict.name, watermarkUuid, watermarkDate); - await recordToDeeplake(result, verdict); + await recordToDeeplake(result, verdict, mergeTarget?.install); } catch (e: any) { // The gate sometimes hallucinates a MERGE target that exists in the // user's global skills (~/.claude/skills/) but not in this project. diff --git a/tests/claude-code/skillify-cli.test.ts b/tests/claude-code/skillify-cli.test.ts index ead613ffc..c3c2ea517 100644 --- a/tests/claude-code/skillify-cli.test.ts +++ b/tests/claude-code/skillify-cli.test.ts @@ -31,6 +31,7 @@ vi.mock("../../src/deeplake-api.js", () => ({ import { runSkillifyCommand } from "../../src/commands/skillify.js"; import { setFakeHome, clearFakeHome } from "../shared/fake-home.js"; import { loadConfig } from "../../src/config.js"; +import { recordPull } from "../../src/skillify/manifest.js"; const loadConfigMock = loadConfig as unknown as ReturnType; const VALID_CONFIG = { @@ -423,6 +424,35 @@ describe("push", () => { // ── unpull ──────────────────────────────────────────────────────────────── +describe("dedupe", () => { + let dedupeHome: string; + beforeEach(() => { + dedupeHome = mkdtempSync(join(tmpdir(), "skillify-cli-dedupe-home-")); + setFakeHome(dedupeHome); + }); + afterEach(() => { + clearFakeHome(); + try { rmSync(dedupeHome, { recursive: true, force: true }); } catch { /* nothing */ } + }); + + it("--dry-run lists the caller's own -- copy and keeps it on disk", () => { + const root = join(dedupeHome, ".claude", "skills"); + for (const dir of ["deploy", "deploy--tester"]) { + mkdirSync(join(root, dir), { recursive: true }); + writeFileSync(join(root, dir, "SKILL.md"), "---\nname: deploy\nauthor: tester\nversion: 2\n---\n\nbody\n"); + } + recordPull({ + dirName: "deploy--tester", name: "deploy", author: "tester", projectKey: "pk", remoteVersion: 2, + install: "global", installRoot: root, pulledAt: "2026", symlinks: [], + }); + runSkillifyCommand(["dedupe", "--dry-run"]); + const out = logged.join("\n"); + expect(out).toMatch(/→ would remove\s+deploy--tester/); + expect(out).toMatch(/Result: 1 to fix, 0 kept\./); + expect(existsSync(join(root, "deploy--tester"))).toBe(true); + }); +}); + describe("unpull", () => { // Each test runs under a fresh HOME so the manifest writes by // pull/unpull don't pollute the developer's real ~/.deeplake state. diff --git a/tests/claude-code/skillify-existing-skills.test.ts b/tests/claude-code/skillify-existing-skills.test.ts index baf4f6851..ca9f68ff8 100644 --- a/tests/claude-code/skillify-existing-skills.test.ts +++ b/tests/claude-code/skillify-existing-skills.test.ts @@ -57,8 +57,8 @@ describe("listAllExistingSkills", () => { const skills = listAllExistingSkills(projectCwd); // Project is enumerated first, then global. expect(skills).toEqual([ - { name: "deploy", body: expect.stringContaining("project body"), source: "project" }, - { name: "hivemind-plugin-testing--alice", body: expect.stringContaining("global body"), source: "global" }, + { name: "deploy", dirName: "deploy", body: expect.stringContaining("project body"), source: "project", version: 1 }, + { name: "hivemind-plugin-testing--alice", dirName: "hivemind-plugin-testing--alice", body: expect.stringContaining("global body"), source: "global", version: 1 }, ]); }); @@ -97,17 +97,13 @@ describe("renderExistingSkillsBlock", () => { expect(result.block).toMatch(/MERGE is NOT a valid choice/); }); - it("only [project] skills are MERGE-eligible (post-#125 review: worker's mergeSkill is rooted at cfg.install)", () => { + it("without a userName only [project] skills are MERGE-eligible", () => { writeSkill(projectRoot(), "deploy", "p body"); writeSkill(globalRoot(), "team-standup--d", "g body"); writeSkill(globalRoot(), "pg-deeplake-cred-callback--levon", "g body 2"); const result = renderExistingSkillsBlock(projectCwd, 10_000); - // [global] entries are reference-only until the worker can resolve a - // global mergeSkill root (`/--`) and translate - // the verdict name back to a DB name. Without that plumbing the - // worker would always fall through to writeNewSkill — i.e. silently - // create a new skill instead of merging — which is exactly the - // duplicate-producing bug #118 was meant to prevent. + // Global entries are MERGE targets only when authored by the given + // userName (see skillify-own-skill-chain.test.ts). expect(result.mergeTargetNames).toEqual(["deploy"]); }); diff --git a/tests/claude-code/skillify-own-skill-chain.test.ts b/tests/claude-code/skillify-own-skill-chain.test.ts new file mode 100644 index 000000000..4f443dd7c --- /dev/null +++ b/tests/claude-code/skillify-own-skill-chain.test.ts @@ -0,0 +1,245 @@ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { existsSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; + +import { runPull, type QueryFn } from "../../src/skillify/pull.js"; +import { loadManifest, recordPull } from "../../src/skillify/manifest.js"; +import { renderExistingSkillsBlock } from "../../src/skillify/existing-skills.js"; +import { mergeSkill } from "../../src/skillify/skill-writer.js"; +import { runDedupe } from "../../src/skillify/dedupe.js"; +import { setFakeHome, clearFakeHome } from "../shared/fake-home.js"; + +// One skill = one version chain. The user's own skill must never get a second +// dir (`--` beside ``), and the gate must continue the chain +// of the user's own global copy instead of starting a new v1 under the name. + +let projectCwd: string; +let fakeHome: string; + +beforeEach(() => { + projectCwd = mkdtempSync(join(tmpdir(), "skillify-chain-project-")); + fakeHome = mkdtempSync(join(tmpdir(), "skillify-chain-home-")); + setFakeHome(fakeHome); +}); + +afterEach(() => { + clearFakeHome(); + rmSync(projectCwd, { recursive: true, force: true }); + rmSync(fakeHome, { recursive: true, force: true }); +}); + +const globalRoot = () => join(fakeHome, ".claude", "skills"); +const projectRoot = () => join(projectCwd, ".claude", "skills"); + +function skillFile(name: string, author: string, version: number, body = "body"): string { + return `---\nname: ${name}\ndescription: "d"\nauthor: ${author}\nsource_sessions: []\nversion: ${version}\ncreated_by_agent: x\ncreated_at: 2026\nupdated_at: 2026\n---\n\n${body}\n`; +} + +function writeSkill(root: string, dirName: string, name: string, author: string, version: number, body = "body"): void { + mkdirSync(join(root, dirName), { recursive: true }); + writeFileSync(join(root, dirName, "SKILL.md"), skillFile(name, author, version, body)); +} + +function recordGlobal(dirName: string, name: string, author: string, version: number, symlinks: string[] = []): void { + recordPull({ + dirName, name, rawName: name, author, projectKey: "pk", remoteVersion: version, + install: "global", installRoot: globalRoot(), pulledAt: "2026", symlinks, + }); +} + +const row = (over: Record = {}) => ({ + name: "deploy", project: "p", project_key: "pk", body: "remote body", version: 3, + source_agent: "claude_code", scope: "me", author: "alice", description: "d", + trigger_text: "", source_sessions: "[]", install: "global", + created_at: "2026", updated_at: "2026", ...over, +}); + +const query = (rows: Record[]): QueryFn => async () => rows; + +const pull = (rows: Record[], over: Record = {}) => runPull({ + query: query(rows), tableName: "skills", install: "global", users: [], + selfAuthor: "alice", projectCwd, ...over, +}); + +describe("pull: own-author rows", () => { + it("installs an own row into the slot, not --", async () => { + const summary = await pull([row()]); + expect(summary.wrote).toBe(1); + expect(readdirSync(globalRoot())).toEqual(["deploy"]); + expect(loadManifest().entries.map(e => e.dirName)).toEqual(["deploy"]); + }); + + it("does not create a second dir beside a locally-mined at the same version", async () => { + writeSkill(globalRoot(), "deploy", "deploy", "alice", 3, "local body"); + const summary = await pull([row()]); + expect(summary.skipped).toBe(1); + expect(readdirSync(globalRoot())).toEqual(["deploy"]); + expect(readFileSync(join(globalRoot(), "deploy", "SKILL.md"), "utf-8")).toContain("local body"); + }); + + it("updates an older locally-mined but never records it, so unpull cannot delete it", async () => { + writeSkill(globalRoot(), "deploy", "deploy", "alice", 1, "local body"); + const summary = await pull([row()]); + expect(summary.wrote).toBe(1); + expect(readdirSync(globalRoot())).toEqual(["deploy"]); + expect(readFileSync(join(globalRoot(), "deploy", "SKILL.md"), "utf-8")).toContain("version: 3"); + expect(loadManifest().entries).toEqual([]); + }); + + it("skips an own row when the project copy has the same or a higher version", async () => { + writeSkill(projectRoot(), "deploy", "deploy", "alice", 4); + const summary = await pull([row()]); + expect(summary.skipped).toBe(1); + expect(existsSync(globalRoot())).toBe(false); + }); + + it("moves a managed legacy -- copy into the slot", async () => { + writeSkill(globalRoot(), "deploy--alice", "deploy", "alice", 3); + recordGlobal("deploy--alice", "deploy", "alice", 3); + await pull([row()]); + expect(readdirSync(globalRoot())).toEqual(["deploy"]); + expect(loadManifest().entries.map(e => e.dirName)).toEqual(["deploy"]); + }); + + it("keeps -- for teammates", async () => { + await pull([row({ author: "bob" })]); + expect(readdirSync(globalRoot())).toEqual(["deploy--bob"]); + }); + + it("keeps the old layout when the caller does not pass selfAuthor", async () => { + await pull([row()], { selfAuthor: undefined }); + expect(readdirSync(globalRoot())).toEqual(["deploy--alice"]); + }); +}); + +describe("gate: own global skill is a merge target", () => { + it("offers the user's own pulled -- copy under its real name", () => { + writeSkill(globalRoot(), "deploy--alice", "deploy", "alice", 6); + const block = renderExistingSkillsBlock(projectCwd, 100_000, "alice"); + expect(block.mergeTargetNames).toEqual(["deploy"]); + expect(block.mergeTargets.deploy).toEqual({ install: "global", skillsRoot: globalRoot(), dirName: "deploy--alice" }); + expect(block.block).toContain("[global, author=alice]: deploy"); + }); + + it("keeps a teammate's global skill read-only", () => { + writeSkill(globalRoot(), "deploy--bob", "deploy", "bob", 6); + const block = renderExistingSkillsBlock(projectCwd, 100_000, "alice"); + expect(block.mergeTargetNames).toEqual([]); + expect(block.block).toContain("[global, read-only, author=bob]: deploy--bob"); + }); + + it("picks the higher version when the project and global copies share a name", () => { + writeSkill(projectRoot(), "deploy", "deploy", "alice", 2); + writeSkill(globalRoot(), "deploy", "deploy", "alice", 7); + const block = renderExistingSkillsBlock(projectCwd, 100_000, "alice"); + expect(block.mergeTargets.deploy.install).toBe("global"); + }); + + it("a MERGE into the resolved target continues the chain at v+1", () => { + writeSkill(globalRoot(), "deploy--alice", "deploy", "alice", 6); + const target = renderExistingSkillsBlock(projectCwd, 100_000, "alice").mergeTargets.deploy; + const result = mergeSkill({ + skillsRoot: target.skillsRoot, dirName: target.dirName, name: "deploy", + body: "merged", newSourceSessions: ["s"], agent: "claude_code", editor: "alice", + }); + expect(result.version).toBe(7); + expect(readFileSync(join(globalRoot(), "deploy--alice", "SKILL.md"), "utf-8")).toMatch(/^name: deploy$/m); + }); +}); + +describe("dedupe", () => { + it("dry-run reports and changes nothing", () => { + writeSkill(globalRoot(), "deploy", "deploy", "alice", 5); + writeSkill(globalRoot(), "deploy--alice", "deploy", "alice", 5); + recordGlobal("deploy--alice", "deploy", "alice", 5); + const out = runDedupe({ myUsername: "alice", cwd: projectCwd, dryRun: true }); + expect(out.map(e => [e.dirName, e.action])).toEqual([["deploy--alice", "removed"]]); + expect(readdirSync(globalRoot()).sort()).toEqual(["deploy", "deploy--alice"]); + }); + + it("removes the copy and its fan-out link when a local is at the same or a higher version", () => { + writeSkill(projectRoot(), "deploy", "deploy", "alice", 6); + writeSkill(globalRoot(), "deploy--alice", "deploy", "alice", 5); + const link = join(fakeHome, ".agents", "skills", "deploy--alice"); + mkdirSync(join(fakeHome, ".agents", "skills"), { recursive: true }); + symlinkSync(join(globalRoot(), "deploy--alice"), link, "dir"); + recordGlobal("deploy--alice", "deploy", "alice", 5, [link]); + runDedupe({ myUsername: "alice", cwd: projectCwd }); + expect(readdirSync(globalRoot())).toEqual([]); + expect(() => lstatSync(link)).toThrow(); + expect(loadManifest().entries).toEqual([]); + }); + + it("moves the copy into the slot when no local exists", () => { + writeSkill(globalRoot(), "deploy--alice", "deploy", "alice", 5); + recordGlobal("deploy--alice", "deploy", "alice", 5); + runDedupe({ myUsername: "alice", cwd: projectCwd }); + expect(readdirSync(globalRoot())).toEqual(["deploy"]); + expect(loadManifest().entries.map(e => e.dirName)).toEqual(["deploy"]); + }); + + it("replaces an older global with the newer copy and keeps a .bak", () => { + writeSkill(globalRoot(), "deploy", "deploy", "alice", 2, "old"); + writeSkill(globalRoot(), "deploy--alice", "deploy", "alice", 5, "new"); + recordGlobal("deploy--alice", "deploy", "alice", 5); + runDedupe({ myUsername: "alice", cwd: projectCwd }); + expect(readdirSync(globalRoot())).toEqual(["deploy"]); + expect(readFileSync(join(globalRoot(), "deploy", "SKILL.md"), "utf-8")).toContain("new"); + expect(readFileSync(join(globalRoot(), "deploy", "SKILL.md.bak"), "utf-8")).toContain("old"); + }); + + it("keeps the copy when only an older project exists", () => { + writeSkill(projectRoot(), "deploy", "deploy", "alice", 2); + writeSkill(globalRoot(), "deploy--alice", "deploy", "alice", 5); + recordGlobal("deploy--alice", "deploy", "alice", 5); + const out = runDedupe({ myUsername: "alice", cwd: projectCwd }); + expect(out.map(e => e.action)).toEqual(["kept"]); + expect(readdirSync(globalRoot())).toEqual(["deploy--alice"]); + }); + + it("repairs a project-install copy within its own root", () => { + writeSkill(projectRoot(), "deploy--alice", "deploy", "alice", 5); + recordPull({ + dirName: "deploy--alice", name: "deploy", author: "alice", projectKey: "pk", remoteVersion: 5, + install: "project", installRoot: projectRoot(), pulledAt: "2026", symlinks: [], + }); + runDedupe({ myUsername: "alice", cwd: projectCwd }); + expect(readdirSync(projectRoot())).toEqual(["deploy"]); + }); + + it("keeps the copy when the slot exists without a readable version", () => { + mkdirSync(join(globalRoot(), "deploy"), { recursive: true }); + writeFileSync(join(globalRoot(), "deploy", "notes.txt"), "hand-made"); + writeSkill(globalRoot(), "deploy--alice", "deploy", "alice", 5); + recordGlobal("deploy--alice", "deploy", "alice", 5); + expect(runDedupe({ myUsername: "alice", cwd: projectCwd }).map(e => e.action)).toEqual(["kept"]); + expect(readdirSync(globalRoot()).sort()).toEqual(["deploy", "deploy--alice"]); + }); + + it("reports a failed entry and goes on with the next one", () => { + writeSkill(globalRoot(), "deploy", "deploy", "alice", 2); + writeSkill(globalRoot(), "deploy--alice", "deploy", "alice", 5); + recordGlobal("deploy--alice", "deploy", "alice", 5); + mkdirSync(join(globalRoot(), "deploy", "SKILL.md.bak")); + writeSkill(globalRoot(), "build--alice", "build", "alice", 1); + recordGlobal("build--alice", "build", "alice", 1); + const out = runDedupe({ myUsername: "alice", cwd: projectCwd }); + expect(out.map(e => [e.dirName, e.action])).toEqual([["deploy--alice", "failed"], ["build--alice", "moved"]]); + expect(existsSync(join(globalRoot(), "build", "SKILL.md"))).toBe(true); + }); + + it("skips a manifest entry whose copy is already gone", () => { + recordGlobal("deploy--alice", "deploy", "alice", 5); + expect(runDedupe({ myUsername: "alice", cwd: projectCwd })).toEqual([]); + }); + + it("never touches teammates' copies or unmanaged dirs", () => { + writeSkill(globalRoot(), "deploy", "deploy", "alice", 9); + writeSkill(globalRoot(), "deploy--bob", "deploy", "bob", 1); + recordGlobal("deploy--bob", "deploy", "bob", 1); + writeSkill(globalRoot(), "build--alice", "build", "alice", 1); + expect(runDedupe({ myUsername: "alice", cwd: projectCwd })).toEqual([]); + expect(readdirSync(globalRoot()).sort()).toEqual(["build--alice", "deploy", "deploy--bob"]); + }); +});