From 147e9260c905891490d98f73852665d290132985 Mon Sep 17 00:00:00 2001 From: Zak El Fassi Date: Wed, 1 Jul 2026 16:03:23 -0700 Subject: [PATCH 1/6] =?UTF-8?q?feat(cli):=20add=20skdd=20add/push/drops=20?= =?UTF-8?q?=E2=80=94=20the=20Commons=20verbs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit skdd add [selector]: install a drop (or one skill) from a Commons repo — GitHub shorthand, git URL (#ref), or local path. Strict validation before install, collision check with --rename escape hatch, registry provenance owner/repo@shortsha (drop-id), full sha recorded in .skdd-lock.json, mirrors refreshed via the existing safe (never forced) link path. --dry-run/--json/--non-interactive/-g. skdd push : ship a skill upstream as a PR via gh. Strips machine-local state (usage-count -> "0", last-used dropped), preserves forged-* provenance. Upstream-known skills branch as evolve/ with a diff summary; new skills branch as skill/ into incoming/ (or an existing drop via --drop, updating drops.json). Local-path targets are a --dry-run test seam. Default target from ~/.skdd/config.toml commons key (smol-toml, already a dependency). skdd drops [--from]: list a Commons' drops (table/json). No new runtime dependencies. 24 new tests incl. a regression test that add never force-replaces a populated mirror dir. --- cli/README.md | 31 ++ cli/src/commands/add.ts | 307 ++++++++++++++ cli/src/commands/drops.ts | 62 +++ cli/src/commands/push.ts | 392 ++++++++++++++++++ cli/src/index.ts | 89 ++++ cli/src/lib/commons.ts | 201 +++++++++ cli/src/lib/config.ts | 30 ++ cli/src/lib/lock.ts | 51 +++ cli/test/add.test.ts | 204 +++++++++ cli/test/drops.test.ts | 62 +++ .../fixtures/mini-commons-invalid/drops.json | 12 + .../packs/2026-01-bad/broken-skill/SKILL.md | 7 + cli/test/fixtures/mini-commons/drops.json | 13 + .../packs/2026-01-test/alpha-skill/SKILL.md | 17 + .../packs/2026-01-test/beta-skill/SKILL.md | 16 + cli/test/push.test.ts | 175 ++++++++ docs/plans/2026-07-skdd-commons.md | 9 +- 17 files changed, 1674 insertions(+), 4 deletions(-) create mode 100644 cli/src/commands/add.ts create mode 100644 cli/src/commands/drops.ts create mode 100644 cli/src/commands/push.ts create mode 100644 cli/src/lib/commons.ts create mode 100644 cli/src/lib/config.ts create mode 100644 cli/src/lib/lock.ts create mode 100644 cli/test/add.test.ts create mode 100644 cli/test/drops.test.ts create mode 100644 cli/test/fixtures/mini-commons-invalid/drops.json create mode 100644 cli/test/fixtures/mini-commons-invalid/packs/2026-01-bad/broken-skill/SKILL.md create mode 100644 cli/test/fixtures/mini-commons/drops.json create mode 100644 cli/test/fixtures/mini-commons/packs/2026-01-test/alpha-skill/SKILL.md create mode 100644 cli/test/fixtures/mini-commons/packs/2026-01-test/beta-skill/SKILL.md create mode 100644 cli/test/push.test.ts diff --git a/cli/README.md b/cli/README.md index 0e87f27..6e3e803 100644 --- a/cli/README.md +++ b/cli/README.md @@ -26,6 +26,9 @@ skdd list [--format=table|json] [-g] skdd link [--mode=symlink|copy|auto] [--harness=] [--force] [--quiet] [-g] skdd doctor [--json] [-g] skdd import [target] [--json] [--apply] [--canonical=] [--skip-link] [-g] +skdd add [selector] [--rename=] [--dry-run] [--json] [--non-interactive] [-g] +skdd push [--to=] [--drop=] [--dry-run] [-g] +skdd drops [--from=] [--format=table|json] skdd hub skdd mcp ``` @@ -118,6 +121,34 @@ skdd import --apply # migrate + link skdd import ../some-other-project --apply # operate on a different root ``` +### `skdd add` + +Install skills from a **Commons repo** — a git repo with a `drops.json` manifest and `packs///` directories (see [SkDD Commons](https://github.com/zakelfassi/skdd-commons)). Sources: GitHub shorthand (`owner/repo`), a full git URL, or a local path, each with an optional `#ref`. Selector: a drop id, `drop/skill` for a single skill, or omitted for an interactive pick. + +Every skill is validated with `--strict` before install (refused on failure), checked for name collisions against the target colony (`--rename` resolves single-skill collisions), registered with provenance (`owner/repo@shortsha (drop-id)` in the Source column, full sha in `.skdd-lock.json`), and mirrored via the same **safe, never-forced** link path as `skdd link`. + +```bash +skdd add zakelfassi/skdd-commons 2026-07-frontier # whole drop +skdd add zakelfassi/skdd-commons 2026-07-frontier/finish-the-loop -g # one skill, global colony +skdd add ../my-commons 2026-01-test --dry-run # local source, plan only +``` + +### `skdd push` + +Ship a skill (or every local skill sharing a `metadata.pack` id) upstream to a Commons as a PR. Needs the [GitHub CLI](https://cli.github.com) authenticated. The default target repo comes from `~/.skdd/config.toml` (`commons = "owner/repo"`). + +Machine-local state is stripped before travel (`usage-count` resets to `"0"`, `last-used` is dropped); `forged-*` provenance is preserved. Skills that already exist upstream branch as `evolve/` with a diff summary; new skills branch as `skill/` and land in `incoming/` for maintainer triage, or in an existing drop with `--drop `. `--dry-run` prints the full would-be PR without network writes. + +```bash +skdd push what-would-you-cut --dry-run # inspect the PR before sending it +skdd push what-would-you-cut # fork, branch, PR +skdd push my-new-skill --drop 2026-07-frontier +``` + +### `skdd drops` + +List the drops a Commons offers (id, title, date, skill count, story link). `--from` accepts the same source forms as `add`; defaults to the configured commons. + ## Development ```bash diff --git a/cli/src/commands/add.ts b/cli/src/commands/add.ts new file mode 100644 index 0000000..d924ee8 --- /dev/null +++ b/cli/src/commands/add.ts @@ -0,0 +1,307 @@ +import { cpSync, existsSync, readFileSync, writeFileSync } from "node:fs"; +import { join, relative, resolve } from "node:path"; +import { select } from "@inquirer/prompts"; +import { + type DropsManifest, + fetchCommons, + parseSource, + provenanceLabel, + readDropsManifest, + resolveSelector, +} from "../lib/commons.js"; +import { ensureGlobalColony, skddHome } from "../lib/global.js"; +import { upsertLockEntry } from "../lib/lock.js"; +import { logger } from "../lib/logger.js"; +import { addRegistryEntry } from "../lib/registry.js"; +import { parseSkill } from "../lib/skill.js"; +import { NAME_MAX_LENGTH, NAME_REGEX } from "../lib/spec.js"; +import { runLink } from "./link.js"; +import { validateSkill } from "./validate.js"; + +export interface AddOptions { + cwd?: string; + global?: boolean; + rename?: string; + dryRun?: boolean; + json?: boolean; + /** Skip the interactive drop picker (CI / agent-driven use). */ + nonInteractive?: boolean; +} + +interface InstalledSkill { + name: string; + sourceName: string; // upstream name (differs from `name` when --rename is used) + path: string; + provenance: string; + description: string; +} + +export async function runAdd( + sourceArg: string, + selector: string | undefined, + opts: AddOptions = {}, +): Promise { + const cwd = resolve(opts.cwd ?? process.cwd()); + const colonyRoot = opts.global ? skddHome() : cwd; + if (opts.global) ensureGlobalColony(); + const canonicalDir = opts.global + ? join(skddHome(), "skills") + : join(cwd, detectCanonical(cwd) ?? "skills"); + + if (opts.rename) { + const err = validateRename(opts.rename); + if (err) { + logger.error(err); + return 1; + } + } + + // ── fetch the commons ────────────────────────────────────────────────────── + let fetched: ReturnType; + try { + fetched = fetchCommons(parseSource(sourceArg, cwd)); + } catch (err) { + logger.error((err as Error).message); + return 1; + } + + try { + let manifest: DropsManifest; + try { + manifest = readDropsManifest(fetched.dir); + } catch (err) { + logger.error((err as Error).message); + return 1; + } + + // ── resolve the selection ──────────────────────────────────────────────── + let effectiveSelector = selector; + if (!effectiveSelector) { + if (opts.nonInteractive || opts.json || !process.stdin.isTTY) { + logger.error( + `No selector given. Available drops: ${manifest.drops.map((d) => d.id).join(", ") || "(none)"}`, + ); + logger.dim(`Usage: skdd add ${sourceArg} [/]`); + return 1; + } + effectiveSelector = await select({ + message: "Pick a drop to install:", + choices: manifest.drops.map((d) => ({ + name: `${d.id} — ${d.title} (${d.skills.length} skills)`, + value: d.id, + })), + }); + } + + let selection: ReturnType; + try { + selection = resolveSelector(manifest, effectiveSelector); + } catch (err) { + logger.error((err as Error).message); + return 1; + } + + if (opts.rename && selection.skills.length !== 1) { + logger.error( + `--rename applies to a single skill; the selection '${effectiveSelector}' contains ${selection.skills.length}.`, + ); + return 1; + } + + // ── validate every selected skill (refuse on any strict failure) ───────── + const dropDir = join(fetched.dir, "packs", selection.drop.id); + let validationFailed = false; + const parsedSkills: Array<{ sourceName: string; dir: string; description: string }> = []; + for (const skillName of selection.skills) { + const skillMd = join(dropDir, skillName, "SKILL.md"); + if (!existsSync(skillMd)) { + logger.error(`${selection.drop.id}/${skillName}: SKILL.md missing in the source repo.`); + validationFailed = true; + continue; + } + try { + const parsed = parseSkill(skillMd); + const errors = validateSkill(parsed, { strict: true }).filter( + (i) => i.severity === "error", + ); + if (errors.length > 0) { + logger.error(`${selection.drop.id}/${skillName}: fails skdd validate --strict:`); + for (const e of errors) logger.dim(` ${e.field ? `[${e.field}] ` : ""}${e.message}`); + validationFailed = true; + continue; + } + parsedSkills.push({ + sourceName: skillName, + dir: join(dropDir, skillName), + description: String(parsed.frontmatter.description ?? ""), + }); + } catch (err) { + logger.error(`${selection.drop.id}/${skillName}: ${(err as Error).message}`); + validationFailed = true; + } + } + if (validationFailed) { + logger.error("Refusing to install: one or more skills failed validation."); + return 1; + } + + // ── collision check against the target colony ──────────────────────────── + const collisions: string[] = []; + for (const s of parsedSkills) { + const targetName = opts.rename ?? s.sourceName; + if (existsSync(join(canonicalDir, targetName))) { + collisions.push(targetName); + } + } + if (collisions.length > 0) { + for (const name of collisions) { + logger.error( + `Collision: '${name}' already exists in ${relative(cwd, canonicalDir) || canonicalDir}.`, + ); + } + logger.dim( + collisions.length === 1 && parsedSkills.length === 1 + ? `Re-run with --rename to install it under a different name.` + : `Remove or rename the existing skills, or add skills one at a time with --rename.`, + ); + return 1; + } + + // ── dry run: report the plan and stop ──────────────────────────────────── + const planned: InstalledSkill[] = parsedSkills.map((s) => { + const name = opts.rename ?? s.sourceName; + return { + name, + sourceName: s.sourceName, + path: join(relative(colonyRoot, canonicalDir) || canonicalDir, name, "SKILL.md"), + provenance: provenanceLabel(fetched.source, fetched.sha, selection.drop.id), + description: s.description, + }; + }); + + if (opts.dryRun) { + if (opts.json) { + console.log( + JSON.stringify( + { + dryRun: true, + source: fetched.source.label, + sha: fetched.sha, + drop: selection.drop.id, + skills: planned, + }, + null, + 2, + ), + ); + } else { + logger.heading(`skdd add — dry run`); + logger.dim( + `source: ${fetched.source.label}${fetched.sha ? ` @ ${fetched.sha.slice(0, 7)}` : ""}`, + ); + logger.dim(`drop: ${selection.drop.id} — ${selection.drop.title}`); + console.log(""); + for (const p of planned) { + logger.info( + ` would install ${p.sourceName}${p.name !== p.sourceName ? ` as ${p.name}` : ""} → ${p.path}`, + ); + } + logger.dim("\nNo files written (--dry-run)."); + } + return 0; + } + + // ── install: copy, rename, register, lock ─────────────────────────────── + for (let i = 0; i < parsedSkills.length; i++) { + const s = parsedSkills[i]!; + const p = planned[i]!; + const dest = join(canonicalDir, p.name); + cpSync(s.dir, dest, { recursive: true }); + if (p.name !== s.sourceName) { + rewriteSkillName(join(dest, "SKILL.md"), p.name); + } + addRegistryEntry(colonyRoot, { + name: p.name, + source: p.provenance, + path: p.path, + lastUsed: new Date().toISOString().slice(0, 10), + uses: 0, + description: p.description, + status: "active", + }); + upsertLockEntry(colonyRoot, p.name, { + source: fetched.source.label, + drop: selection.drop.id, + sha: fetched.sha, + addedAt: new Date().toISOString(), + }); + if (!opts.json) logger.success(`installed ${p.name} (${p.provenance})`); + } + + // ── refresh mirrors through the existing SAFE link path (never forced) ─── + const linkCode = opts.global + ? await runLink({ global: true, quiet: true }) + : await runLink({ cwd, quiet: true }); + + if (opts.json) { + console.log( + JSON.stringify( + { + source: fetched.source.label, + sha: fetched.sha, + drop: selection.drop.id, + installed: planned, + mirrors: linkCode === 0 ? "refreshed" : "blocked", + }, + null, + 2, + ), + ); + } + + if (linkCode !== 0) { + logger.warn( + `Skills are installed in the canonical dir, but at least one harness mirror was NOT refreshed\n` + + ` (a populated directory sits at the mirror path — skdd never replaces it silently).\n` + + ` Review the paths above, then run '${opts.global ? "skdd link -g" : "skdd link"}' (add --force only if you're sure).`, + ); + return 1; + } + + if (!opts.json) { + logger.success( + `${planned.length} skill(s) added from ${fetched.source.label} — mirrors refreshed.`, + ); + } + return 0; + } finally { + fetched.cleanup(); + } +} + +function detectCanonical(root: string): string | null { + const p = join(root, ".colony.json"); + if (!existsSync(p)) return null; + try { + const manifest = JSON.parse(readFileSync(p, "utf8")) as { canonicalSkillsDir?: string }; + if (typeof manifest.canonicalSkillsDir === "string" && manifest.canonicalSkillsDir.length > 0) { + return manifest.canonicalSkillsDir; + } + } catch { + // malformed .colony.json is doctor's concern, not add's + } + return null; +} + +function validateRename(name: string): string | null { + if (name.length > NAME_MAX_LENGTH) return `--rename must be ≤${NAME_MAX_LENGTH} characters`; + if (!NAME_REGEX.test(name)) return `--rename must be lowercase kebab-case (${NAME_REGEX})`; + return null; +} + +/** Rewrite the frontmatter `name:` line so it matches the renamed directory. */ +function rewriteSkillName(skillMdPath: string, newName: string): void { + const raw = readFileSync(skillMdPath, "utf8"); + const updated = raw.replace(/^name:\s*.+$/m, `name: ${newName}`); + writeFileSync(skillMdPath, updated); +} diff --git a/cli/src/commands/drops.ts b/cli/src/commands/drops.ts new file mode 100644 index 0000000..b45920f --- /dev/null +++ b/cli/src/commands/drops.ts @@ -0,0 +1,62 @@ +import { fetchCommons, parseSource, readDropsManifest } from "../lib/commons.js"; +import { loadConfig } from "../lib/config.js"; +import { logger, pc } from "../lib/logger.js"; + +export interface DropsOptions { + cwd?: string; + from?: string; + format?: "table" | "json"; +} + +export async function runDrops(opts: DropsOptions = {}): Promise { + const cwd = opts.cwd ?? process.cwd(); + const from = opts.from ?? loadConfig().commons; + + let fetched: ReturnType; + try { + fetched = fetchCommons(parseSource(from, cwd)); + } catch (err) { + logger.error((err as Error).message); + return 1; + } + + try { + const manifest = readDropsManifest(fetched.dir); + + if (opts.format === "json") { + console.log(JSON.stringify({ source: fetched.source.label, drops: manifest.drops }, null, 2)); + return 0; + } + + if (manifest.drops.length === 0) { + logger.warn(`${fetched.source.label} has no drops yet.`); + return 0; + } + + logger.heading(`Drops — ${fetched.source.label}`); + const rows = manifest.drops.map((d) => [ + d.id, + d.title, + d.date, + String(d.skills.length), + d.story ?? "—", + ]); + printTable(["Drop", "Title", "Date", "Skills", "Story"], rows); + console.log(""); + logger.dim(`Install one: skdd add ${fetched.source.label} `); + return 0; + } catch (err) { + logger.error((err as Error).message); + return 1; + } finally { + fetched.cleanup(); + } +} + +function printTable(headers: string[], rows: string[][]): void { + const widths = headers.map((h, i) => Math.max(h.length, ...rows.map((r) => r[i]?.length ?? 0))); + const pad = (cells: string[]) => cells.map((c, i) => c.padEnd(widths[i]!)).join(" │ "); + console.log(pc.bold(pad(headers))); + console.log(widths.map((w) => "─".repeat(w)).join("─┼─")); + for (const row of rows) console.log(pad(row)); +} diff --git a/cli/src/commands/push.ts b/cli/src/commands/push.ts new file mode 100644 index 0000000..5ea30d4 --- /dev/null +++ b/cli/src/commands/push.ts @@ -0,0 +1,392 @@ +import { spawnSync } from "node:child_process"; +import { + cpSync, + existsSync, + mkdtempSync, + readdirSync, + readFileSync, + rmSync, + statSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { loadConfig } from "../lib/config.js"; +import { ensureGlobalColony, skddHome } from "../lib/global.js"; +import { logger } from "../lib/logger.js"; +import { parseSkill } from "../lib/skill.js"; + +export interface PushOptions { + cwd?: string; + global?: boolean; + /** Target Commons: `owner/repo`, or a local repo path (dry-run and tests). */ + to?: string; + /** Existing drop id a NEW skill should join (updates drops.json in the PR). */ + drop?: string; + dryRun?: boolean; +} + +interface PushItem { + name: string; + localDir: string; + /** Stripped SKILL.md content that will travel upstream. */ + content: string; + description: string; + forgedReason: string; + /** Drop that already contains this skill upstream, or null when it's new. */ + upstreamDrop: string | null; + /** Repo-relative destination directory in the Commons. */ + destDir: string; + diffSummary: string | null; +} + +interface PlannedPr { + repo: string; + branch: string; + title: string; + body: string; + items: PushItem[]; + updatesManifest: boolean; +} + +export async function runPush(target: string, opts: PushOptions = {}): Promise { + const cwd = resolve(opts.cwd ?? process.cwd()); + const colonyRoot = opts.global ? skddHome() : cwd; + if (opts.global) ensureGlobalColony(); + const canonicalDir = join(colonyRoot, "skills"); + + // ── locate what we're pushing: one skill dir, or every skill in a pack ───── + const skillDirs: Array<{ name: string; dir: string }> = []; + const directDir = join(canonicalDir, target); + if (existsSync(join(directDir, "SKILL.md"))) { + skillDirs.push({ name: target, dir: directDir }); + } else if (existsSync(canonicalDir)) { + for (const entry of readdirSync(canonicalDir)) { + const dir = join(canonicalDir, entry); + const skillMd = join(dir, "SKILL.md"); + if (!statSync(dir).isDirectory() || !existsSync(skillMd)) continue; + try { + const parsed = parseSkill(skillMd); + if ( + (parsed.frontmatter.metadata as Record | undefined)?.["pack"] === target + ) { + skillDirs.push({ name: entry, dir }); + } + } catch { + // unparseable skills can't be pushed; skip + } + } + } + if (skillDirs.length === 0) { + logger.error( + `'${target}' is neither a skill in ${canonicalDir} nor a pack id any local skill belongs to.`, + ); + return 1; + } + + // ── resolve the target Commons ───────────────────────────────────────────── + const toRaw = opts.to ?? loadConfig().commons; + const localTarget = resolveLocalTarget(toRaw, cwd); + const isLocalTarget = localTarget !== null; + + if (!isLocalTarget && !/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(toRaw)) { + logger.error(`--to must be owner/repo or a local Commons path (got '${toRaw}').`); + return 1; + } + + if (!isLocalTarget && !ghAvailable()) { + logger.error( + "`skdd push` needs the GitHub CLI (`gh`) installed and authenticated to read the target repo and open the PR.", + ); + logger.dim("Install: https://cli.github.com — then `gh auth login` and re-run."); + return 1; + } + + // ── read the upstream manifest to classify each skill (new vs evolve) ────── + let upstream: { manifest: UpstreamManifest; readFile: (path: string) => string | null }; + try { + upstream = isLocalTarget ? localUpstream(localTarget) : remoteUpstream(toRaw); + } catch (err) { + logger.error(`Cannot read the target Commons (${toRaw}): ${(err as Error).message}`); + return 1; + } + + if (opts.drop && !upstream.manifest.drops.some((d) => d.id === opts.drop)) { + logger.error( + `--drop '${opts.drop}' does not exist upstream. Existing drops: ${upstream.manifest.drops.map((d) => d.id).join(", ")}. Creating drops is a maintainer act.`, + ); + return 1; + } + + const items: PushItem[] = []; + for (const { name, dir } of skillDirs) { + const raw = readFileSync(join(dir, "SKILL.md"), "utf8"); + const content = stripMachineLocalMetadata(raw); + const parsed = parseSkill(join(dir, "SKILL.md")); + const meta = (parsed.frontmatter.metadata ?? {}) as Record; + const upstreamDrop = upstream.manifest.drops.find((d) => d.skills.includes(name))?.id ?? null; + const destDir = upstreamDrop + ? `packs/${upstreamDrop}/${name}` + : opts.drop + ? `packs/${opts.drop}/${name}` + : `incoming/${name}`; + let diffSummary: string | null = null; + if (upstreamDrop) { + const upstreamContent = upstream.readFile(`packs/${upstreamDrop}/${name}/SKILL.md`); + if (upstreamContent !== null) diffSummary = summarizeDiff(upstreamContent, content); + } + items.push({ + name, + localDir: dir, + content, + description: String(parsed.frontmatter.description ?? ""), + forgedReason: String(meta["forged-reason"] ?? ""), + upstreamDrop, + destDir, + diffSummary, + }); + } + + const pr = buildPr(toRaw, target, items, opts); + + // ── dry run: print the would-be PR, write nothing ────────────────────────── + if (opts.dryRun) { + logger.heading("skdd push — dry run"); + logger.dim(`target repo: ${pr.repo}`); + logger.dim(`branch: ${pr.branch}`); + console.log(""); + for (const item of pr.items) { + logger.info( + ` ${item.upstreamDrop ? "evolve" : "new"}: ${item.name} → ${item.destDir}/SKILL.md`, + ); + } + console.log(""); + logger.info(`PR title: ${pr.title}`); + console.log(`\n${pr.body}`); + logger.dim("\nNo network writes (--dry-run)."); + return 0; + } + + if (isLocalTarget) { + logger.error( + "A local --to target supports --dry-run only. Point --to at owner/repo to open a real PR.", + ); + return 1; + } + + return openPr(toRaw, pr, opts); +} + +// ── upstream readers ────────────────────────────────────────────────────────── + +interface UpstreamManifest { + drops: Array<{ id: string; skills: string[] }>; +} + +function resolveLocalTarget(to: string, cwd: string): string | null { + if (to.includes("://") || to.startsWith("git@")) return null; + const abs = resolve(cwd, to); + // owner/repo shorthand would only collide with a real dir containing drops.json + return existsSync(join(abs, "drops.json")) ? abs : null; +} + +function localUpstream(dir: string): { + manifest: UpstreamManifest; + readFile: (path: string) => string | null; +} { + const manifest = JSON.parse(readFileSync(join(dir, "drops.json"), "utf8")) as UpstreamManifest; + return { + manifest, + readFile: (path: string) => { + const p = join(dir, path); + return existsSync(p) ? readFileSync(p, "utf8") : null; + }, + }; +} + +function remoteUpstream(repo: string): { + manifest: UpstreamManifest; + readFile: (path: string) => string | null; +} { + const readFile = (path: string): string | null => { + const res = gh(["api", `repos/${repo}/contents/${path}`, "--jq", ".content"]); + if (res.status !== 0) return null; + return Buffer.from(res.stdout.trim(), "base64").toString("utf8"); + }; + const raw = readFile("drops.json"); + if (raw === null) { + throw new Error("no readable drops.json — is it a SkDD Commons repo you can access?"); + } + return { manifest: JSON.parse(raw) as UpstreamManifest, readFile }; +} + +// ── PR assembly ─────────────────────────────────────────────────────────────── + +function buildPr(repo: string, target: string, items: PushItem[], opts: PushOptions): PlannedPr { + const single = items.length === 1 ? items[0]! : null; + const isEvolve = single ? single.upstreamDrop !== null : false; + const branch = single ? `${isEvolve ? "evolve" : "skill"}/${single.name}` : `pack/${target}`; + const title = single + ? isEvolve + ? `evolve(${single.name}): ${truncate(single.description, 60)}` + : `skill(${single.name}): ${truncate(single.description, 60)}` + : `pack(${target}): push ${items.length} skills`; + + const sections: string[] = []; + for (const item of items) { + if (items.length > 1) sections.push(`## ${item.name}`); + if (item.upstreamDrop) { + sections.push(`Evolution of \`${item.name}\` in drop \`${item.upstreamDrop}\`.`); + if (item.diffSummary) sections.push(`**Diff summary:** ${item.diffSummary}`); + sections.push(`### The edge case\n\n`); + } else { + sections.push( + `## Why this skill\n\n${item.forgedReason || ""}`, + ); + sections.push(`## Summary\n\n${item.description}`); + sections.push( + item.destDir.startsWith("incoming/") + ? `## Proposed drop\n\nmaintainer triage (no --drop given; lands in \`incoming/\`)` + : `## Proposed drop\n\n\`${item.destDir.split("/")[1]}\``, + ); + } + } + sections.push( + `---\n*Opened by \`skdd push\` — machine-local state (usage-count, last-used) stripped; \`forged-*\` provenance preserved.*`, + ); + + return { + repo, + branch, + title, + body: sections.join("\n\n"), + items, + updatesManifest: items.some((i) => !i.upstreamDrop && opts.drop !== undefined), + }; +} + +function truncate(s: string, max: number): string { + const firstSentence = s.split(/(?<=\.)\s/)[0] ?? s; + return firstSentence.length > max ? `${firstSentence.slice(0, max - 1)}…` : firstSentence; +} + +/** + * Machine-local state stays home: usage-count resets, last-used is dropped. + * Line-based on purpose — gray-matter round-trips would reformat the YAML. + */ +export function stripMachineLocalMetadata(raw: string): string { + return raw.replace(/^(\s*usage-count:\s*).+$/m, `$1"0"`).replace(/^\s*last-used:.*\r?\n/m, ""); +} + +/** Dependency-free diff stat between two SKILL.md revisions. */ +export function summarizeDiff(before: string, after: string): string { + const beforeLines = new Set(before.split(/\r?\n/)); + const afterLines = new Set(after.split(/\r?\n/)); + let added = 0; + let removed = 0; + for (const l of afterLines) if (!beforeLines.has(l)) added++; + for (const l of beforeLines) if (!afterLines.has(l)) removed++; + return `~${added} line(s) added, ~${removed} removed`; +} + +// ── real PR flow (gh required) ──────────────────────────────────────────────── + +function gh(args: string[]): { status: number; stdout: string; stderr: string } { + const res = spawnSync("gh", args, { encoding: "utf8" }); + return { + status: res.status ?? 1, + stdout: res.stdout ?? "", + stderr: res.stderr ?? "", + }; +} + +function ghAvailable(): boolean { + return gh(["auth", "status"]).status === 0; +} + +function git(args: string[], cwd?: string): { status: number; stdout: string; stderr: string } { + const res = spawnSync("git", args, { cwd, encoding: "utf8" }); + return { status: res.status ?? 1, stdout: res.stdout ?? "", stderr: res.stderr ?? "" }; +} + +function openPr(repo: string, pr: PlannedPr, opts: PushOptions): number { + const owner = repo.split("/")[0]!; + const login = gh(["api", "user", "--jq", ".login"]).stdout.trim(); + if (!login) { + logger.error("Cannot determine the authenticated GitHub user (`gh api user` failed)."); + return 1; + } + + // Fork unless we own the target repo; a branch on the upstream works then. + let headRepo = repo; + if (login !== owner) { + const fork = gh(["repo", "fork", repo, "--clone=false"]); + if (fork.status !== 0) { + logger.error(`Could not fork ${repo}: ${fork.stderr.trim()}`); + return 1; + } + headRepo = `${login}/${repo.split("/")[1]}`; + } + + const tmp = mkdtempSync(join(tmpdir(), "skdd-push-")); + try { + const clone = git(["clone", "--depth", "1", `https://github.com/${repo}.git`, tmp]); + if (clone.status !== 0) { + logger.error(`Clone failed: ${clone.stderr.trim()}`); + return 1; + } + git(["checkout", "-b", pr.branch], tmp); + + for (const item of pr.items) { + const dest = join(tmp, item.destDir); + cpSync(item.localDir, dest, { recursive: true }); + writeFileSync(join(dest, "SKILL.md"), item.content); + } + // A new skill headed for an existing drop must also appear in drops.json, + // or the Commons' manifest-check job fails the PR on a stray directory. + if (opts.drop) { + const manifestPath = join(tmp, "drops.json"); + const manifest = JSON.parse(readFileSync(manifestPath, "utf8")); + const drop = manifest.drops.find((d: { id: string }) => d.id === opts.drop); + for (const item of pr.items) { + if (!item.upstreamDrop && !drop.skills.includes(item.name)) drop.skills.push(item.name); + } + writeFileSync(manifestPath, `${JSON.stringify(manifest, null, 2)}\n`); + } + + git(["add", "-A"], tmp); + const commit = git(["commit", "-m", pr.title], tmp); + if (commit.status !== 0) { + logger.error(`Nothing to push — the Commons already has this exact content.`); + return 1; + } + const pushRemote = headRepo === repo ? "origin" : `https://github.com/${headRepo}.git`; + const push = git(["push", "-u", pushRemote, pr.branch], tmp); + if (push.status !== 0) { + logger.error(`git push failed: ${push.stderr.trim()}`); + return 1; + } + + const head = headRepo === repo ? pr.branch : `${login}:${pr.branch}`; + const create = gh([ + "pr", + "create", + "--repo", + repo, + "--head", + head, + "--title", + pr.title, + "--body", + pr.body, + ]); + if (create.status !== 0) { + logger.error(`gh pr create failed: ${create.stderr.trim()}`); + return 1; + } + logger.success(`PR opened: ${create.stdout.trim()}`); + return 0; + } finally { + rmSync(tmp, { recursive: true, force: true }); + } +} diff --git a/cli/src/index.ts b/cli/src/index.ts index f44de0e..aa5e668 100644 --- a/cli/src/index.ts +++ b/cli/src/index.ts @@ -1,5 +1,7 @@ import { Command } from "commander"; +import { runAdd } from "./commands/add.js"; import { runDoctor } from "./commands/doctor.js"; +import { runDrops } from "./commands/drops.js"; import { runForge } from "./commands/forge.js"; import { runHub } from "./commands/hub.js"; import { runImport } from "./commands/import.js"; @@ -7,6 +9,7 @@ import { runInit } from "./commands/init.js"; import { runLink } from "./commands/link.js"; import { runList } from "./commands/list.js"; import { runMcpAdd, runMcpList, runMcpRemove, runMcpSync } from "./commands/mcp.js"; +import { runPush } from "./commands/push.js"; import { runShow } from "./commands/show.js"; import { runValidate } from "./commands/validate.js"; import type { LinkMode } from "./lib/fs-link.js"; @@ -240,6 +243,92 @@ program }, ); +program + .command("add") + .description( + "Install skills from a Commons repo (drop or single skill) with validation + provenance", + ) + .argument("", "Commons source: owner/repo, a git URL (optional #ref), or a local path") + .argument( + "[selector]", + "Drop id (e.g. 2026-07-frontier) or drop/skill (e.g. 2026-07-frontier/finish-the-loop); omit for an interactive pick", + ) + .option("-g, --global", "Install into the global colony (~/.skdd/skills/)", false) + .option( + "--rename ", + "Install a single skill under a different name (collision escape hatch)", + ) + .option("-n, --dry-run", "Show what would be installed without writing files", false) + .option("-j, --json", "Emit a machine-readable JSON report", false) + .option("--non-interactive", "Fail instead of prompting when no selector is given", false) + .action( + async ( + source: string, + selector: string | undefined, + opts: { + global: boolean; + rename?: string; + dryRun: boolean; + json: boolean; + nonInteractive: boolean; + }, + ) => { + const code = await runAdd(source, selector, { + global: opts.global, + rename: opts.rename, + dryRun: opts.dryRun, + json: opts.json, + nonInteractive: opts.nonInteractive, + }); + process.exit(code); + }, + ); + +program + .command("push") + .description( + "Push a skill (or a whole pack) upstream to a Commons repo as a PR — new skill or evolution", + ) + .argument("", "Skill name in the colony, or a pack id shared by several local skills") + .option( + "--to ", + "Target Commons: owner/repo (default: config `commons` key, or a local path for --dry-run)", + ) + .option( + "--drop ", + "Existing drop a NEW skill should join (also updates drops.json in the PR)", + ) + .option("-g, --global", "Look for the skill in the global colony (~/.skdd/skills/)", false) + .option( + "-n, --dry-run", + "Print the would-be PR (branch, title, body) without network writes", + false, + ) + .action( + async ( + target: string, + opts: { to?: string; drop?: string; global: boolean; dryRun: boolean }, + ) => { + const code = await runPush(target, { + to: opts.to, + drop: opts.drop, + global: opts.global, + dryRun: opts.dryRun, + }); + process.exit(code); + }, + ); + +program + .command("drops") + .description("List the drops a Commons repo offers (default: the configured commons)") + .option("--from ", "Commons source: owner/repo, git URL, or local path") + .option("-f, --format ", "Output format: table|json", "table") + .action(async (opts: { from?: string; format: "table" | "json" }) => { + const code = await runDrops({ from: opts.from, format: opts.format }); + process.exit(code); + }); + // ── mcp subcommand group ───────────────────────────────────────────────────── const mcp = new Command("mcp").description( diff --git a/cli/src/lib/commons.ts b/cli/src/lib/commons.ts new file mode 100644 index 0000000..6a72ff2 --- /dev/null +++ b/cli/src/lib/commons.ts @@ -0,0 +1,201 @@ +import { spawnSync } from "node:child_process"; +import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; + +/** A drop entry in a Commons repo's drops.json manifest. */ +export interface DropEntry { + id: string; + title: string; + date: string; + skills: string[]; + story?: string; + forgedBy?: string; +} + +export interface DropsManifest { + version: number; + drops: DropEntry[]; +} + +/** Parsed `skdd add` / `skdd drops` source argument. */ +export interface CommonsSource { + kind: "local" | "git"; + /** Human/registry label: `owner/repo` for GitHub sources, the raw path/URL otherwise. */ + label: string; + cloneUrl?: string; + localPath?: string; + ref?: string; +} + +const GITHUB_SHORTHAND = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; +const GIT_URL_PREFIX = /^(?:https?:\/\/|git@|ssh:\/\/)/; + +/** + * Parse a Commons source: GitHub shorthand `owner/repo`, a full git URL, or a + * local path — each with an optional `#ref` (branch/tag/sha) suffix on git forms. + */ +export function parseSource(raw: string, cwd: string): CommonsSource { + // Local path forms first: explicit path prefixes, or a path that exists on disk. + if ( + raw.startsWith(".") || + raw.startsWith("/") || + raw.startsWith("~") || + existsSync(resolve(cwd, raw)) + ) { + const localPath = raw.startsWith("~") + ? join(process.env.HOME ?? "", raw.slice(1)) + : resolve(cwd, raw); + return { kind: "local", label: raw, localPath }; + } + + const hashIdx = raw.lastIndexOf("#"); + const ref = hashIdx > 0 ? raw.slice(hashIdx + 1) : undefined; + const base = hashIdx > 0 ? raw.slice(0, hashIdx) : raw; + + if (GIT_URL_PREFIX.test(base)) { + // Try to extract owner/repo from common URL shapes for the label. + const m = base.match(/[/:]([A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+?)(?:\.git)?\/?$/); + return { kind: "git", label: m?.[1] ?? base, cloneUrl: base, ref }; + } + + if (GITHUB_SHORTHAND.test(base)) { + return { kind: "git", label: base, cloneUrl: `https://github.com/${base}.git`, ref }; + } + + throw new Error( + `Unrecognized source '${raw}' — expected owner/repo, a git URL, or a local path.`, + ); +} + +export interface FetchedCommons { + /** Absolute path of the working copy (the local dir itself, or a temp clone). */ + dir: string; + /** Full commit sha of the fetched state, or null when the source isn't a git repo. */ + sha: string | null; + source: CommonsSource; + /** Remove the temp clone (no-op for local sources). */ + cleanup: () => void; +} + +function git(args: string[], cwd?: string): { status: number; stdout: string; stderr: string } { + const res = spawnSync("git", args, { cwd, encoding: "utf8" }); + return { + status: res.status ?? 1, + stdout: (res.stdout ?? "").trim(), + stderr: (res.stderr ?? "").trim(), + }; +} + +/** + * Materialize a Commons source into a readable directory. + * Git sources are shallow-cloned into a temp dir (`#ref` tries `--branch` first + * for branches/tags, then falls back to a full clone + checkout for shas). + */ +export function fetchCommons(source: CommonsSource): FetchedCommons { + if (source.kind === "local") { + const dir = source.localPath!; + if (!existsSync(dir)) { + throw new Error(`Local source does not exist: ${dir}`); + } + const rev = git(["rev-parse", "HEAD"], dir); + return { + dir, + sha: rev.status === 0 ? rev.stdout : null, + source, + cleanup: () => {}, + }; + } + + const tmp = mkdtempSync(join(tmpdir(), "skdd-commons-")); + const cleanup = () => rmSync(tmp, { recursive: true, force: true }); + const url = source.cloneUrl!; + + let cloned = false; + if (source.ref) { + const res = git(["clone", "--depth", "1", "--branch", source.ref, url, tmp]); + cloned = res.status === 0; + } + if (!cloned) { + const args = source.ref + ? ["clone", url, tmp] // ref wasn't a branch/tag — full clone, then checkout the sha + : ["clone", "--depth", "1", url, tmp]; + const res = git(args, undefined); + if (res.status !== 0) { + cleanup(); + throw new Error(`git clone failed for ${source.label}: ${res.stderr || res.stdout}`); + } + if (source.ref) { + const co = git(["checkout", "--detach", source.ref], tmp); + if (co.status !== 0) { + cleanup(); + throw new Error(`cannot check out ref '${source.ref}': ${co.stderr || co.stdout}`); + } + } + } + + const rev = git(["rev-parse", "HEAD"], tmp); + return { dir: tmp, sha: rev.status === 0 ? rev.stdout : null, source, cleanup }; +} + +/** Read and minimally validate a Commons repo's drops.json. */ +export function readDropsManifest(dir: string): DropsManifest { + const p = join(dir, "drops.json"); + if (!existsSync(p)) { + throw new Error(`No drops.json found — '${dir}' does not look like a SkDD Commons repo.`); + } + let raw: unknown; + try { + raw = JSON.parse(readFileSync(p, "utf8")); + } catch (err) { + throw new Error(`Malformed drops.json: ${(err as Error).message}`); + } + const manifest = raw as Partial; + if (!Array.isArray(manifest.drops)) { + throw new Error(`Malformed drops.json: missing "drops" array.`); + } + for (const d of manifest.drops) { + if (typeof d.id !== "string" || !Array.isArray(d.skills)) { + throw new Error(`Malformed drops.json: each drop needs an "id" and a "skills" array.`); + } + } + return { version: manifest.version ?? 1, drops: manifest.drops as DropEntry[] }; +} + +export interface ResolvedSelection { + drop: DropEntry; + /** Skill names selected within the drop. */ + skills: string[]; +} + +/** + * Resolve an `add` selector against a manifest: + * - `` → every skill in the drop + * - `/` → a single skill + */ +export function resolveSelector(manifest: DropsManifest, selector: string): ResolvedSelection { + const slash = selector.indexOf("/"); + const dropId = slash === -1 ? selector : selector.slice(0, slash); + const skillName = slash === -1 ? null : selector.slice(slash + 1); + + const drop = manifest.drops.find((d) => d.id === dropId); + if (!drop) { + const available = manifest.drops.map((d) => d.id).join(", ") || "(none)"; + throw new Error(`Drop '${dropId}' not found. Available drops: ${available}`); + } + if (skillName === null) { + return { drop, skills: [...drop.skills] }; + } + if (!drop.skills.includes(skillName)) { + throw new Error( + `Skill '${skillName}' is not in drop '${drop.id}'. Its skills: ${drop.skills.join(", ")}`, + ); + } + return { drop, skills: [skillName] }; +} + +/** Registry Source column label for an added skill: `owner/repo@shortsha (drop-id)`. */ +export function provenanceLabel(source: CommonsSource, sha: string | null, dropId: string): string { + const shortSha = sha ? sha.slice(0, 7) : "local"; + return `${source.label}@${shortSha} (${dropId})`; +} diff --git a/cli/src/lib/config.ts b/cli/src/lib/config.ts new file mode 100644 index 0000000..5646150 --- /dev/null +++ b/cli/src/lib/config.ts @@ -0,0 +1,30 @@ +import { existsSync, readFileSync } from "node:fs"; +import { join } from "node:path"; +import { parse as parseToml } from "smol-toml"; +import { skddHome } from "./global.js"; + +/** User-level skdd configuration, read from ~/.skdd/config.toml. */ +export interface SkddConfig { + /** Default Commons repo (`owner/repo`) for `skdd push`, `skdd drops`, and bare `skdd add`. */ + commons: string; +} + +export const DEFAULT_COMMONS = "zakelfassi/skdd-commons"; + +export function configPath(): string { + return join(skddHome(), "config.toml"); +} + +export function loadConfig(): SkddConfig { + const defaults: SkddConfig = { commons: DEFAULT_COMMONS }; + const p = configPath(); + if (!existsSync(p)) return defaults; + try { + const raw = parseToml(readFileSync(p, "utf8")) as Record; + return { + commons: typeof raw["commons"] === "string" ? (raw["commons"] as string) : defaults.commons, + }; + } catch { + return defaults; + } +} diff --git a/cli/src/lib/lock.ts b/cli/src/lib/lock.ts new file mode 100644 index 0000000..fd0bbaa --- /dev/null +++ b/cli/src/lib/lock.ts @@ -0,0 +1,51 @@ +import { existsSync, readFileSync, writeFileSync } from "node:fs"; +import { join, resolve } from "node:path"; + +/** + * `.skdd-lock.json` — full-fidelity provenance for skills installed via `skdd add`. + * The registry's Source column carries the human-readable short form + * (`owner/repo@shortsha (drop)`); the lock file keeps the full sha so + * `skdd doctor` can later detect upstream drift. + */ +export interface LockEntry { + source: string; // owner/repo, git URL, or local path label + drop: string; + sha: string | null; // full commit sha of the source at add time + addedAt: string; // ISO timestamp +} + +export interface LockFile { + version: number; + skills: Record; +} + +export const LOCK_VERSION = 1; +const LOCK_FILE = ".skdd-lock.json"; + +export function lockPath(cwd: string): string { + return join(resolve(cwd), LOCK_FILE); +} + +export function loadLock(cwd: string): LockFile { + const p = lockPath(cwd); + if (!existsSync(p)) return { version: LOCK_VERSION, skills: {} }; + try { + const raw = JSON.parse(readFileSync(p, "utf8")) as Partial; + if (typeof raw.skills !== "object" || raw.skills === null) { + return { version: LOCK_VERSION, skills: {} }; + } + return { version: LOCK_VERSION, skills: raw.skills as Record }; + } catch { + return { version: LOCK_VERSION, skills: {} }; + } +} + +export function saveLock(cwd: string, lock: LockFile): void { + writeFileSync(lockPath(cwd), JSON.stringify(lock, null, 2) + "\n"); +} + +export function upsertLockEntry(cwd: string, name: string, entry: LockEntry): void { + const lock = loadLock(cwd); + lock.skills[name] = entry; + saveLock(cwd, lock); +} diff --git a/cli/test/add.test.ts b/cli/test/add.test.ts new file mode 100644 index 0000000..3e06107 --- /dev/null +++ b/cli/test/add.test.ts @@ -0,0 +1,204 @@ +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { platform, tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { runAdd } from "../src/commands/add.js"; +import { loadLock } from "../src/lib/lock.js"; + +const FIXTURES = join(__dirname, "fixtures"); +const MINI_COMMONS = join(FIXTURES, "mini-commons"); +const MINI_COMMONS_INVALID = join(FIXTURES, "mini-commons-invalid"); + +const skipOnWindows = platform() === "win32"; +const runUnix = skipOnWindows ? it.skip : it; + +let tmp: string; +let logs: string[] = []; +let origLog: typeof console.log; +let origWarn: typeof console.warn; +let origError: typeof console.error; + +function captureConsole() { + logs = []; + origLog = console.log; + origWarn = console.warn; + origError = console.error; + const push = (...args: unknown[]) => { + logs.push(args.map((a) => (typeof a === "string" ? a : JSON.stringify(a))).join(" ")); + }; + console.log = push as typeof console.log; + console.warn = push as typeof console.warn; + console.error = push as typeof console.error; +} + +function restoreConsole() { + console.log = origLog; + console.warn = origWarn; + console.error = origError; +} + +beforeEach(() => { + tmp = mkdtempSync(join(tmpdir(), "skdd-add-")); + captureConsole(); +}); + +afterEach(() => { + restoreConsole(); + rmSync(tmp, { recursive: true, force: true }); +}); + +describe("runAdd", () => { + it("installs a whole drop from a local commons with provenance", async () => { + const code = await runAdd(MINI_COMMONS, "2026-01-test", { cwd: tmp, nonInteractive: true }); + restoreConsole(); + expect(code).toBe(0); + expect(existsSync(join(tmp, "skills/alpha-skill/SKILL.md"))).toBe(true); + expect(existsSync(join(tmp, "skills/beta-skill/SKILL.md"))).toBe(true); + + const registry = readFileSync(join(tmp, ".skills-registry.md"), "utf8"); + expect(registry).toContain("alpha-skill"); + expect(registry).toContain("beta-skill"); + expect(registry).toContain("(2026-01-test)"); + + const lock = loadLock(tmp); + expect(lock.skills["alpha-skill"]).toBeDefined(); + expect(lock.skills["alpha-skill"]!.drop).toBe("2026-01-test"); + expect(lock.skills["alpha-skill"]!.source).toBe(MINI_COMMONS); + }); + + it("installs a single skill via the drop/skill selector", async () => { + const code = await runAdd(MINI_COMMONS, "2026-01-test/alpha-skill", { + cwd: tmp, + nonInteractive: true, + }); + restoreConsole(); + expect(code).toBe(0); + expect(existsSync(join(tmp, "skills/alpha-skill/SKILL.md"))).toBe(true); + expect(existsSync(join(tmp, "skills/beta-skill"))).toBe(false); + }); + + it("refuses to install skills that fail strict validation", async () => { + const code = await runAdd(MINI_COMMONS_INVALID, "2026-01-bad", { + cwd: tmp, + nonInteractive: true, + }); + restoreConsole(); + expect(code).toBe(1); + expect(existsSync(join(tmp, "skills/broken-skill"))).toBe(false); + expect(logs.join("\n")).toContain("fails skdd validate --strict"); + }); + + it("refuses on a name collision with a clear message", async () => { + mkdirSync(join(tmp, "skills/alpha-skill"), { recursive: true }); + writeFileSync(join(tmp, "skills/alpha-skill/SKILL.md"), "---\nname: alpha-skill\n---\nmine"); + const code = await runAdd(MINI_COMMONS, "2026-01-test/alpha-skill", { + cwd: tmp, + nonInteractive: true, + }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("Collision"); + expect(logs.join("\n")).toContain("--rename"); + // the pre-existing skill is untouched + expect(readFileSync(join(tmp, "skills/alpha-skill/SKILL.md"), "utf8")).toContain("mine"); + }); + + it("--rename resolves a collision and rewrites the frontmatter name", async () => { + mkdirSync(join(tmp, "skills/alpha-skill"), { recursive: true }); + writeFileSync(join(tmp, "skills/alpha-skill/SKILL.md"), "---\nname: alpha-skill\n---\nmine"); + const code = await runAdd(MINI_COMMONS, "2026-01-test/alpha-skill", { + cwd: tmp, + nonInteractive: true, + rename: "alpha-two", + }); + restoreConsole(); + expect(code).toBe(0); + const installed = readFileSync(join(tmp, "skills/alpha-two/SKILL.md"), "utf8"); + expect(installed).toMatch(/^name: alpha-two$/m); + }); + + it("rejects --rename for a multi-skill selection", async () => { + const code = await runAdd(MINI_COMMONS, "2026-01-test", { + cwd: tmp, + nonInteractive: true, + rename: "solo-name", + }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("--rename applies to a single skill"); + }); + + it("--dry-run reports the plan and writes nothing", async () => { + const code = await runAdd(MINI_COMMONS, "2026-01-test", { + cwd: tmp, + nonInteractive: true, + dryRun: true, + }); + restoreConsole(); + expect(code).toBe(0); + expect(existsSync(join(tmp, "skills"))).toBe(false); + expect(existsSync(join(tmp, ".skills-registry.md"))).toBe(false); + expect(logs.join("\n")).toContain("would install alpha-skill"); + }); + + it("--json emits a machine-readable report", async () => { + const code = await runAdd(MINI_COMMONS, "2026-01-test", { + cwd: tmp, + nonInteractive: true, + json: true, + }); + restoreConsole(); + expect(code).toBe(0); + const payload = JSON.parse(logs.find((l) => l.trim().startsWith("{"))!); + expect(payload.drop).toBe("2026-01-test"); + expect(payload.installed).toHaveLength(2); + expect(payload.mirrors).toBe("refreshed"); + }); + + it("errors with the available drops when no selector is given non-interactively", async () => { + const code = await runAdd(MINI_COMMONS, undefined, { cwd: tmp, nonInteractive: true }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("2026-01-test"); + }); + + it("errors on an unknown drop id", async () => { + const code = await runAdd(MINI_COMMONS, "2099-12-nope", { cwd: tmp, nonInteractive: true }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("not found"); + }); + + it("errors on a source that is not a commons repo", async () => { + const notCommons = mkdtempSync(join(tmpdir(), "skdd-notcommons-")); + try { + const code = await runAdd(notCommons, "2026-01-test", { cwd: tmp, nonInteractive: true }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("drops.json"); + } finally { + rmSync(notCommons, { recursive: true, force: true }); + } + }); + + runUnix("never force-replaces a populated mirror directory (the §2 guardrail)", async () => { + // A real, populated .claude/skills dir — the shape that must survive. + mkdirSync(join(tmp, ".claude/skills/precious-skill"), { recursive: true }); + writeFileSync( + join(tmp, ".claude/skills/precious-skill/SKILL.md"), + "---\nname: precious-skill\ndescription: not in any colony. Use when testing.\n---\nirreplaceable", + ); + + const code = await runAdd(MINI_COMMONS, "2026-01-test", { cwd: tmp, nonInteractive: true }); + restoreConsole(); + + // Install succeeds into canonical, but the mirror refresh is blocked → exit 1. + expect(code).toBe(1); + expect(existsSync(join(tmp, "skills/alpha-skill/SKILL.md"))).toBe(true); + // The populated dir is intact — not replaced by a symlink, contents untouched. + expect(readFileSync(join(tmp, ".claude/skills/precious-skill/SKILL.md"), "utf8")).toContain( + "irreplaceable", + ); + expect(logs.join("\n")).toContain("NOT refreshed"); + }); +}); diff --git a/cli/test/drops.test.ts b/cli/test/drops.test.ts new file mode 100644 index 0000000..2e66c8b --- /dev/null +++ b/cli/test/drops.test.ts @@ -0,0 +1,62 @@ +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { runDrops } from "../src/commands/drops.js"; + +const MINI_COMMONS = join(__dirname, "fixtures", "mini-commons"); + +let logs: string[] = []; +let origLog: typeof console.log; +let origWarn: typeof console.warn; +let origError: typeof console.error; + +beforeEach(() => { + logs = []; + origLog = console.log; + origWarn = console.warn; + origError = console.error; + const push = (...args: unknown[]) => { + logs.push(args.map((a) => (typeof a === "string" ? a : JSON.stringify(a))).join(" ")); + }; + console.log = push as typeof console.log; + console.warn = push as typeof console.warn; + console.error = push as typeof console.error; +}); + +afterEach(() => { + console.log = origLog; + console.warn = origWarn; + console.error = origError; +}); + +describe("runDrops", () => { + it("lists drops from a local commons as a table", async () => { + const code = await runDrops({ from: MINI_COMMONS }); + expect(code).toBe(0); + const out = logs.join("\n"); + expect(out).toContain("2026-01-test"); + expect(out).toContain("January 2026 Test"); + expect(out).toContain("https://example.com/mini-commons-story"); + }); + + it("emits JSON with --format json", async () => { + const code = await runDrops({ from: MINI_COMMONS, format: "json" }); + expect(code).toBe(0); + const payload = JSON.parse(logs.find((l) => l.trim().startsWith("{"))!); + expect(payload.drops).toHaveLength(1); + expect(payload.drops[0].id).toBe("2026-01-test"); + expect(payload.drops[0].skills).toHaveLength(2); + }); + + it("errors on a directory without drops.json", async () => { + const notCommons = mkdtempSync(join(tmpdir(), "skdd-notcommons-")); + try { + const code = await runDrops({ from: notCommons }); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("drops.json"); + } finally { + rmSync(notCommons, { recursive: true, force: true }); + } + }); +}); diff --git a/cli/test/fixtures/mini-commons-invalid/drops.json b/cli/test/fixtures/mini-commons-invalid/drops.json new file mode 100644 index 0000000..a0f6b2a --- /dev/null +++ b/cli/test/fixtures/mini-commons-invalid/drops.json @@ -0,0 +1,12 @@ +{ + "version": 1, + "drops": [ + { + "id": "2026-01-bad", + "title": "Broken fixture drop", + "date": "2026-01-15", + "skills": ["broken-skill"], + "forgedBy": "fixture-bot" + } + ] +} diff --git a/cli/test/fixtures/mini-commons-invalid/packs/2026-01-bad/broken-skill/SKILL.md b/cli/test/fixtures/mini-commons-invalid/packs/2026-01-bad/broken-skill/SKILL.md new file mode 100644 index 0000000..21ea83b --- /dev/null +++ b/cli/test/fixtures/mini-commons-invalid/packs/2026-01-bad/broken-skill/SKILL.md @@ -0,0 +1,7 @@ +--- +name: wrong-name-mismatch +--- + +# Broken Skill + +Missing description and the name does not match the directory. diff --git a/cli/test/fixtures/mini-commons/drops.json b/cli/test/fixtures/mini-commons/drops.json new file mode 100644 index 0000000..0f738e1 --- /dev/null +++ b/cli/test/fixtures/mini-commons/drops.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "drops": [ + { + "id": "2026-01-test", + "title": "January 2026 Test — the mini fixture drop", + "date": "2026-01-15", + "skills": ["alpha-skill", "beta-skill"], + "story": "https://example.com/mini-commons-story", + "forgedBy": "fixture-bot" + } + ] +} diff --git a/cli/test/fixtures/mini-commons/packs/2026-01-test/alpha-skill/SKILL.md b/cli/test/fixtures/mini-commons/packs/2026-01-test/alpha-skill/SKILL.md new file mode 100644 index 0000000..3f93831 --- /dev/null +++ b/cli/test/fixtures/mini-commons/packs/2026-01-test/alpha-skill/SKILL.md @@ -0,0 +1,17 @@ +--- +name: alpha-skill +description: First fixture skill for skdd add tests. Use when testing drop installation. +metadata: + pack: 2026-01-test + forged-by: fixture-bot + forged-from: fixture-session + forged-reason: "exists to be installed by tests" + usage-count: "0" +--- + +# Alpha Skill + +## Steps + +1. Be installed. +2. Be counted. diff --git a/cli/test/fixtures/mini-commons/packs/2026-01-test/beta-skill/SKILL.md b/cli/test/fixtures/mini-commons/packs/2026-01-test/beta-skill/SKILL.md new file mode 100644 index 0000000..00b4389 --- /dev/null +++ b/cli/test/fixtures/mini-commons/packs/2026-01-test/beta-skill/SKILL.md @@ -0,0 +1,16 @@ +--- +name: beta-skill +description: Second fixture skill for skdd add tests. Use when testing multi-skill drops. +metadata: + pack: 2026-01-test + forged-by: fixture-bot + forged-from: fixture-session + forged-reason: "exists to test whole-drop installs" + usage-count: "0" +--- + +# Beta Skill + +## Steps + +1. Arrive alongside alpha-skill. diff --git a/cli/test/push.test.ts b/cli/test/push.test.ts new file mode 100644 index 0000000..f1b121c --- /dev/null +++ b/cli/test/push.test.ts @@ -0,0 +1,175 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { runPush, stripMachineLocalMetadata, summarizeDiff } from "../src/commands/push.js"; + +const FIXTURES = join(__dirname, "fixtures"); +const MINI_COMMONS = join(FIXTURES, "mini-commons"); + +let tmp: string; +let logs: string[] = []; +let origLog: typeof console.log; +let origWarn: typeof console.warn; +let origError: typeof console.error; + +function captureConsole() { + logs = []; + origLog = console.log; + origWarn = console.warn; + origError = console.error; + const push = (...args: unknown[]) => { + logs.push(args.map((a) => (typeof a === "string" ? a : JSON.stringify(a))).join(" ")); + }; + console.log = push as typeof console.log; + console.warn = push as typeof console.warn; + console.error = push as typeof console.error; +} + +function restoreConsole() { + console.log = origLog; + console.warn = origWarn; + console.error = origError; +} + +function writeColonySkill(name: string, extra: { pack?: string; body?: string } = {}) { + const dir = join(tmp, "skills", name); + mkdirSync(dir, { recursive: true }); + writeFileSync( + join(dir, "SKILL.md"), + `--- +name: ${name} +description: Locally evolved ${name}. Use when pushing upstream. +metadata: +${extra.pack ? ` pack: ${extra.pack}\n` : ""} forged-by: test-agent + forged-from: local-session + forged-reason: "forged locally to test skdd push" + usage-count: "7" + last-used: "2026-06-30" +--- + +# ${name} + +## Steps + +1. Original step. +${extra.body ?? ""}`, + ); +} + +beforeEach(() => { + tmp = mkdtempSync(join(tmpdir(), "skdd-push-test-")); + captureConsole(); +}); + +afterEach(() => { + restoreConsole(); + rmSync(tmp, { recursive: true, force: true }); +}); + +describe("stripMachineLocalMetadata", () => { + it("resets usage-count, drops last-used, keeps forged-* provenance", () => { + const raw = `--- +name: x +metadata: + forged-by: claude-fable-5 + forged-reason: "why" + usage-count: "42" + last-used: "2026-06-30" +--- +body`; + const stripped = stripMachineLocalMetadata(raw); + expect(stripped).toContain(`usage-count: "0"`); + expect(stripped).not.toContain("last-used"); + expect(stripped).toContain("forged-by: claude-fable-5"); + expect(stripped).toContain(`forged-reason: "why"`); + }); +}); + +describe("summarizeDiff", () => { + it("counts added and removed lines", () => { + const before = "a\nb\nc"; + const after = "a\nb\nd\ne"; + expect(summarizeDiff(before, after)).toBe("~2 line(s) added, ~1 removed"); + }); +}); + +describe("runPush --dry-run against a local commons", () => { + it("classifies an upstream-known skill as an evolution with a diff summary", async () => { + writeColonySkill("alpha-skill", { body: "2. A new edge case learned in the wild.\n" }); + const code = await runPush("alpha-skill", { cwd: tmp, to: MINI_COMMONS, dryRun: true }); + restoreConsole(); + expect(code).toBe(0); + const out = logs.join("\n"); + expect(out).toContain("evolve: alpha-skill → packs/2026-01-test/alpha-skill/SKILL.md"); + expect(out).toContain("branch: evolve/alpha-skill"); + expect(out).toMatch(/Diff summary.*line\(s\) added/); + expect(out).toContain("The edge case"); + }); + + it("classifies an unknown skill as new, headed for incoming/", async () => { + writeColonySkill("gamma-skill"); + const code = await runPush("gamma-skill", { cwd: tmp, to: MINI_COMMONS, dryRun: true }); + restoreConsole(); + expect(code).toBe(0); + const out = logs.join("\n"); + expect(out).toContain("new: gamma-skill → incoming/gamma-skill/SKILL.md"); + expect(out).toContain("branch: skill/gamma-skill"); + expect(out).toContain("Why this skill"); + expect(out).toContain("forged locally to test skdd push"); + expect(out).toContain("maintainer triage"); + }); + + it("targets an existing drop with --drop", async () => { + writeColonySkill("gamma-skill"); + const code = await runPush("gamma-skill", { + cwd: tmp, + to: MINI_COMMONS, + dryRun: true, + drop: "2026-01-test", + }); + restoreConsole(); + expect(code).toBe(0); + expect(logs.join("\n")).toContain("new: gamma-skill → packs/2026-01-test/gamma-skill/SKILL.md"); + }); + + it("rejects --drop pointing at a drop that does not exist upstream", async () => { + writeColonySkill("gamma-skill"); + const code = await runPush("gamma-skill", { + cwd: tmp, + to: MINI_COMMONS, + dryRun: true, + drop: "2099-12-nope", + }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("Creating drops is a maintainer act"); + }); + + it("pushes every local skill sharing a pack id", async () => { + writeColonySkill("gamma-skill", { pack: "my-pack" }); + writeColonySkill("delta-skill", { pack: "my-pack" }); + const code = await runPush("my-pack", { cwd: tmp, to: MINI_COMMONS, dryRun: true }); + restoreConsole(); + expect(code).toBe(0); + const out = logs.join("\n"); + expect(out).toContain("branch: pack/my-pack"); + expect(out).toContain("gamma-skill"); + expect(out).toContain("delta-skill"); + }); + + it("errors when the target is neither a skill nor a pack id", async () => { + const code = await runPush("no-such-thing", { cwd: tmp, to: MINI_COMMONS, dryRun: true }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("neither a skill"); + }); + + it("refuses a real (non-dry-run) push to a local path target", async () => { + writeColonySkill("gamma-skill"); + const code = await runPush("gamma-skill", { cwd: tmp, to: MINI_COMMONS }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("--dry-run only"); + }); +}); diff --git a/docs/plans/2026-07-skdd-commons.md b/docs/plans/2026-07-skdd-commons.md index 7675c60..f4ad217 100644 --- a/docs/plans/2026-07-skdd-commons.md +++ b/docs/plans/2026-07-skdd-commons.md @@ -174,10 +174,10 @@ Copy from this repo's `packs/fable-festival/`, then per skill: set `metadata.pac ### 5.4 Acceptance (Phase 2) -- [ ] Unit tests per command in `cli/test/` (fixtures: a mini commons repo checked into `cli/test/fixtures/`) — cover happy path, validation failure, collision, dirty-mirror safety, `--dry-run` -- [ ] `add` never force-replaces a populated mirror dir (regression test for the §2 guardrail) -- [ ] Round-trip e2e (manual or scripted): `init` a scratch colony → `add` the frontier drop → edit a skill → `push --dry-run` produces a correct PR body -- [ ] `pnpm typecheck && pnpm lint && pnpm test` green; conventional-commit history; release-please cuts a minor version +- [x] Unit tests per command in `cli/test/` (fixtures: a mini commons repo checked into `cli/test/fixtures/`) — cover happy path, validation failure, collision, dirty-mirror safety, `--dry-run` +- [x] `add` never force-replaces a populated mirror dir (regression test for the §2 guardrail) +- [x] Round-trip e2e (manual or scripted): `init` a scratch colony → `add` the frontier drop → edit a skill → `push --dry-run` produces a correct PR body +- [x] `pnpm typecheck && pnpm lint && pnpm test` green; conventional-commit history; release-please cuts a minor version *(release-please runs on merge)* --- @@ -236,4 +236,5 @@ plugins/skdd-claude/ *(Append dated entries here as phases complete. Manual steps for Zak accumulate here too.)* - 2026-07-01 — Plan authored (claude-fable-5). Bootstrap skills exist untracked at `packs/fable-festival/`; installed to the global colony the same day. +- 2026-07-01 — **Phase 2 complete** (claude-fable-5). `skdd add` / `skdd push` / `skdd drops` shipped in `cli/` (branch `feat/commons-cli`). New libs: `commons.ts` (source parsing + shallow clone + drops.json), `lock.ts` (`.skdd-lock.json`, full sha for future drift detection), `config.ts` (`~/.skdd/config.toml`, `commons` key, default `zakelfassi/skdd-commons`). 24 new tests (add/push/drops) + mini-commons fixtures; 894 total green; typecheck + lint green. Design note: new skills pushed without `--drop` land in the Commons' `incoming/` staging dir (added to the Commons + its CI same day) — drops stay maintainer-curated. Push against a **local path** target supports `--dry-run` only (test seam); real PRs need `gh`. Round-trip e2e ran against the real private repo: `init → add zakelfassi/skdd-commons 2026-07-frontier` (6 skills, provenance `@12ba029`) `→ edit → push --dry-run` produced the correct evolve-branch PR body with diff summary; populated-mirror guardrail covered by a regression test. - 2026-07-01 — **Phase 1 complete** (claude-fable-5). `zakelfassi/skdd-commons` created **private** (ASK-ZAK default taken: private until launch commit), **MIT license** (default taken: consistency with main repo), **npm drops deferred** (default taken: git transport only for v1). Full layout + `2026-07-frontier` drop (six skills, `metadata.pack` updated, `usage-count` reset, `last-used` dropped). CI green on main (run 28552710128: validate --strict / safety-lint / manifest-check, ~22s total). Gate proven on PR #1: safety-lint failed on a planted pipe-to-shell + credential-read fixture (run 28552744808), passed after `security-reviewed` label applied (run 28552805972); PR closed unmerged, branch deleted. `packs/fable-festival/` deleted from this repo; `packs/README.md` now concept doc + featured-drop index. From 17fde65ddc3a869fabf2566ece6e614cac7952e3 Mon Sep 17 00:00:00 2001 From: Zak El Fassi Date: Wed, 1 Jul 2026 17:01:13 -0700 Subject: [PATCH 2/6] fix(cli): treat Commons manifests as hostile input; allowlist push payload Codex adversarial review findings: - [high] drops.json ids/names were used as filesystem path segments unchecked, so a malicious Commons could list a skill like '../escape-skill' and write outside the colony. Every manifest parse site now enforces the lowercase-kebab-case grammar (no slashes, dots, or absolute paths) and add asserts source/destination containment as defense in depth. - [medium] push copied the entire local skill directory into the PR clone, so dotfiles, logs, .env files, or symlinked content could leak to a public Commons PR. Only an allowlisted payload travels now (SKILL.md + regular files under scripts/, references/, assets/); dry-run enumerates exactly what travels and what stays home. +5 adversarial tests (hostile manifests, payload exclusion incl. symlinks and .env); 899 total green. --- cli/README.md | 4 +- cli/src/commands/add.ts | 7 +++ cli/src/commands/push.ts | 85 ++++++++++++++++++++++++++++-- cli/src/lib/commons.ts | 33 +++++++++++- cli/test/add.test.ts | 55 +++++++++++++++++++ cli/test/push.test.ts | 57 ++++++++++++++++++-- docs/plans/2026-07-skdd-commons.md | 1 + site/src/content/docs/commons.md | 76 ++++++++++++++++++++++++++ 8 files changed, 308 insertions(+), 10 deletions(-) create mode 100644 site/src/content/docs/commons.md diff --git a/cli/README.md b/cli/README.md index 6e3e803..def11f1 100644 --- a/cli/README.md +++ b/cli/README.md @@ -125,7 +125,7 @@ skdd import ../some-other-project --apply # operate on a different root Install skills from a **Commons repo** — a git repo with a `drops.json` manifest and `packs///` directories (see [SkDD Commons](https://github.com/zakelfassi/skdd-commons)). Sources: GitHub shorthand (`owner/repo`), a full git URL, or a local path, each with an optional `#ref`. Selector: a drop id, `drop/skill` for a single skill, or omitted for an interactive pick. -Every skill is validated with `--strict` before install (refused on failure), checked for name collisions against the target colony (`--rename` resolves single-skill collisions), registered with provenance (`owner/repo@shortsha (drop-id)` in the Source column, full sha in `.skdd-lock.json`), and mirrored via the same **safe, never-forced** link path as `skdd link`. +Every skill is validated with `--strict` before install (refused on failure), checked for name collisions against the target colony (`--rename` resolves single-skill collisions), registered with provenance (`owner/repo@shortsha (drop-id)` in the Source column, full sha in `.skdd-lock.json`), and mirrored via the same **safe, never-forced** link path as `skdd link`. The manifest is treated as hostile input: drop ids and skill names must match the lowercase-kebab-case grammar (no slashes, no `..`), so a malicious `drops.json` can never write outside your `skills/` directory. ```bash skdd add zakelfassi/skdd-commons 2026-07-frontier # whole drop @@ -137,7 +137,7 @@ skdd add ../my-commons 2026-01-test --dry-run # local Ship a skill (or every local skill sharing a `metadata.pack` id) upstream to a Commons as a PR. Needs the [GitHub CLI](https://cli.github.com) authenticated. The default target repo comes from `~/.skdd/config.toml` (`commons = "owner/repo"`). -Machine-local state is stripped before travel (`usage-count` resets to `"0"`, `last-used` is dropped); `forged-*` provenance is preserved. Skills that already exist upstream branch as `evolve/` with a diff summary; new skills branch as `skill/` and land in `incoming/` for maintainer triage, or in an existing drop with `--drop `. `--dry-run` prints the full would-be PR without network writes. +Machine-local state is stripped before travel (`usage-count` resets to `"0"`, `last-used` is dropped); `forged-*` provenance is preserved. **Only the skill payload travels** — `SKILL.md` plus regular files under `scripts/`, `references/`, and `assets/`; dotfiles, symlinks, and anything else in the skill directory stay home, and `--dry-run` enumerates exactly which files travel and which don't. Skills that already exist upstream branch as `evolve/` with a diff summary; new skills branch as `skill/` and land in `incoming/` for maintainer triage, or in an existing drop with `--drop `. ```bash skdd push what-would-you-cut --dry-run # inspect the PR before sending it diff --git a/cli/src/commands/add.ts b/cli/src/commands/add.ts index d924ee8..56f2f10 100644 --- a/cli/src/commands/add.ts +++ b/cli/src/commands/add.ts @@ -2,6 +2,7 @@ import { cpSync, existsSync, readFileSync, writeFileSync } from "node:fs"; import { join, relative, resolve } from "node:path"; import { select } from "@inquirer/prompts"; import { + assertWithin, type DropsManifest, fetchCommons, parseSource, @@ -109,10 +110,15 @@ export async function runAdd( } // ── validate every selected skill (refuse on any strict failure) ───────── + // Manifest ids/names are grammar-checked in readDropsManifest (untrusted + // input → no slashes/dots), and these asserts are the defense-in-depth + // layer: no source or destination path may leave its expected root. const dropDir = join(fetched.dir, "packs", selection.drop.id); + assertWithin(dropDir, join(fetched.dir, "packs"), `drop '${selection.drop.id}'`); let validationFailed = false; const parsedSkills: Array<{ sourceName: string; dir: string; description: string }> = []; for (const skillName of selection.skills) { + assertWithin(join(dropDir, skillName), dropDir, `skill '${skillName}'`); const skillMd = join(dropDir, skillName, "SKILL.md"); if (!existsSync(skillMd)) { logger.error(`${selection.drop.id}/${skillName}: SKILL.md missing in the source repo.`); @@ -216,6 +222,7 @@ export async function runAdd( const s = parsedSkills[i]!; const p = planned[i]!; const dest = join(canonicalDir, p.name); + assertWithin(dest, canonicalDir, `install target '${p.name}'`); cpSync(s.dir, dest, { recursive: true }); if (p.name !== s.sourceName) { rewriteSkillName(join(dest, "SKILL.md"), p.name); diff --git a/cli/src/commands/push.ts b/cli/src/commands/push.ts index 5ea30d4..0a7851c 100644 --- a/cli/src/commands/push.ts +++ b/cli/src/commands/push.ts @@ -1,7 +1,9 @@ import { spawnSync } from "node:child_process"; import { - cpSync, + copyFileSync, existsSync, + lstatSync, + mkdirSync, mkdtempSync, readdirSync, readFileSync, @@ -10,11 +12,13 @@ import { writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; -import { join, resolve } from "node:path"; +import { dirname, join, resolve } from "node:path"; +import { assertSafeManifestNames } from "../lib/commons.js"; import { loadConfig } from "../lib/config.js"; import { ensureGlobalColony, skddHome } from "../lib/global.js"; import { logger } from "../lib/logger.js"; import { parseSkill } from "../lib/skill.js"; +import { NAME_REGEX } from "../lib/spec.js"; export interface PushOptions { cwd?: string; @@ -38,6 +42,10 @@ interface PushItem { /** Repo-relative destination directory in the Commons. */ destDir: string; diffSummary: string | null; + /** Allowlisted payload that travels (paths relative to the skill dir). */ + files: string[]; + /** Local files that will NOT travel, with the reason. */ + skipped: string[]; } interface PlannedPr { @@ -120,8 +128,17 @@ export async function runPush(target: string, opts: PushOptions = {}): Promise; const upstreamDrop = upstream.manifest.drops.find((d) => d.skills.includes(name))?.id ?? null; @@ -144,6 +161,8 @@ export async function runPush(target: string, opts: PushOptions = {}): Promise string | null; } { const manifest = JSON.parse(readFileSync(join(dir, "drops.json"), "utf8")) as UpstreamManifest; + // The upstream manifest is untrusted input and its ids feed destDir paths. + for (const d of manifest.drops) assertSafeManifestNames(d); return { manifest, readFile: (path: string) => { @@ -217,7 +240,9 @@ function remoteUpstream(repo: string): { if (raw === null) { throw new Error("no readable drops.json — is it a SkDD Commons repo you can access?"); } - return { manifest: JSON.parse(raw) as UpstreamManifest, readFile }; + const manifest = JSON.parse(raw) as UpstreamManifest; + for (const d of manifest.drops) assertSafeManifestNames(d); + return { manifest, readFile }; } // ── PR assembly ─────────────────────────────────────────────────────────────── @@ -278,6 +303,52 @@ export function stripMachineLocalMetadata(raw: string): string { return raw.replace(/^(\s*usage-count:\s*).+$/m, `$1"0"`).replace(/^\s*last-used:.*\r?\n/m, ""); } +// Only the spec-defined skill payload travels upstream. Everything else in the +// skill dir — dotfiles, logs, local notes, .env, symlinks — stays home: push is +// a SHARING command and must never exfiltrate machine-local files. +const PUBLISHABLE_SUBDIRS = new Set(["scripts", "references", "assets"]); + +export function collectPublishablePayload(skillDir: string): { + files: string[]; + skipped: string[]; +} { + const files: string[] = ["SKILL.md"]; + const skipped: string[] = []; + + const walk = (rel: string) => { + for (const entry of readdirSync(join(skillDir, rel))) { + const relPath = join(rel, entry); + const stat = lstatSync(join(skillDir, relPath)); + if (stat.isSymbolicLink()) { + skipped.push(`${relPath} (symlink)`); + } else if (entry.startsWith(".")) { + skipped.push(`${relPath}${stat.isDirectory() ? "/" : ""} (dotfile)`); + } else if (stat.isDirectory()) { + walk(relPath); + } else { + files.push(relPath); + } + } + }; + + for (const entry of readdirSync(skillDir)) { + if (entry === "SKILL.md") continue; + const stat = lstatSync(join(skillDir, entry)); + if (stat.isSymbolicLink()) { + skipped.push(`${entry} (symlink)`); + } else if (entry.startsWith(".")) { + skipped.push(`${entry}${stat.isDirectory() ? "/" : ""} (dotfile)`); + } else if (stat.isDirectory() && PUBLISHABLE_SUBDIRS.has(entry)) { + walk(entry); + } else { + skipped.push( + `${entry}${stat.isDirectory() ? "/" : ""} (outside SKILL.md + scripts/references/assets)`, + ); + } + } + return { files, skipped }; +} + /** Dependency-free diff stat between two SKILL.md revisions. */ export function summarizeDiff(before: string, after: string): string { const beforeLines = new Set(before.split(/\r?\n/)); @@ -339,7 +410,13 @@ function openPr(repo: string, pr: PlannedPr, opts: PushOptions): number { for (const item of pr.items) { const dest = join(tmp, item.destDir); - cpSync(item.localDir, dest, { recursive: true }); + // Copy ONLY the allowlisted payload — never the whole local directory. + for (const rel of item.files) { + if (rel === "SKILL.md") continue; // written below from the stripped content + mkdirSync(dirname(join(dest, rel)), { recursive: true }); + copyFileSync(join(item.localDir, rel), join(dest, rel)); + } + mkdirSync(dest, { recursive: true }); writeFileSync(join(dest, "SKILL.md"), item.content); } // A new skill headed for an existing drop must also appear in drops.json, diff --git a/cli/src/lib/commons.ts b/cli/src/lib/commons.ts index 6a72ff2..216143f 100644 --- a/cli/src/lib/commons.ts +++ b/cli/src/lib/commons.ts @@ -1,7 +1,8 @@ import { spawnSync } from "node:child_process"; import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join, resolve } from "node:path"; +import { isAbsolute, join, relative, resolve } from "node:path"; +import { NAME_MAX_LENGTH, NAME_REGEX } from "./spec.js"; /** A drop entry in a Commons repo's drops.json manifest. */ export interface DropEntry { @@ -158,10 +159,40 @@ export function readDropsManifest(dir: string): DropsManifest { if (typeof d.id !== "string" || !Array.isArray(d.skills)) { throw new Error(`Malformed drops.json: each drop needs an "id" and a "skills" array.`); } + assertSafeManifestNames(d); } return { version: manifest.version ?? 1, drops: manifest.drops as DropEntry[] }; } +/** + * drops.json comes from an UNTRUSTED repo, and its ids/names become filesystem + * paths on both the read side (packs//) and the write side + * (skills/). Enforce the skill-name grammar before any path is built — + * it admits no slashes, dots, or absolute paths, so traversal is impossible. + */ +export function assertSafeManifestNames(drop: { id: string; skills: unknown[] }): void { + if (!NAME_REGEX.test(drop.id) || drop.id.length > NAME_MAX_LENGTH) { + throw new Error( + `Unsafe drop id '${drop.id}' in drops.json — ids must be lowercase kebab-case (letters, digits, dashes only).`, + ); + } + for (const s of drop.skills) { + if (typeof s !== "string" || s.length > NAME_MAX_LENGTH || !NAME_REGEX.test(s)) { + throw new Error( + `Unsafe skill name '${String(s)}' in drop '${drop.id}' — names must be lowercase kebab-case (letters, digits, dashes only).`, + ); + } + } +} + +/** Defense-in-depth: assert a resolved path stayed inside its expected root. */ +export function assertWithin(child: string, parent: string, label: string): void { + const rel = relative(resolve(parent), resolve(child)); + if (rel === "" || rel.startsWith("..") || isAbsolute(rel)) { + throw new Error(`${label} resolves outside ${parent} — refusing.`); + } +} + export interface ResolvedSelection { drop: DropEntry; /** Skill names selected within the drop. */ diff --git a/cli/test/add.test.ts b/cli/test/add.test.ts index 3e06107..c55fa99 100644 --- a/cli/test/add.test.ts +++ b/cli/test/add.test.ts @@ -181,6 +181,61 @@ describe("runAdd", () => { } }); + describe("hostile drops.json (path traversal)", () => { + function writeHostileCommons(drops: Array<{ id: string; skills: string[] }>): string { + const dir = mkdtempSync(join(tmpdir(), "skdd-hostile-")); + writeFileSync(join(dir, "drops.json"), JSON.stringify({ version: 1, drops })); + // A skill placed OUTSIDE packs// that a traversal name would reach: + // packs/2026-01-evil/../escape-skill → packs/escape-skill + mkdirSync(join(dir, "packs", "escape-skill"), { recursive: true }); + writeFileSync( + join(dir, "packs", "escape-skill", "SKILL.md"), + "---\nname: escape-skill\ndescription: Escapes the drop dir. Use when attacking.\n---\n# Escape\n\n1. step\n", + ); + mkdirSync(join(dir, "packs", "2026-01-evil"), { recursive: true }); + return dir; + } + + it("refuses a manifest skill name containing ../", async () => { + const hostile = writeHostileCommons([{ id: "2026-01-evil", skills: ["../escape-skill"] }]); + try { + const code = await runAdd(hostile, "2026-01-evil", { cwd: tmp, nonInteractive: true }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("Unsafe skill name"); + // nothing installed anywhere — inside or outside the colony + expect(existsSync(join(tmp, "skills"))).toBe(false); + expect(existsSync(join(tmp, "escape-skill"))).toBe(false); + } finally { + rmSync(hostile, { recursive: true, force: true }); + } + }); + + it("refuses an absolute-path skill name", async () => { + const hostile = writeHostileCommons([{ id: "2026-01-evil", skills: ["/tmp/escape-skill"] }]); + try { + const code = await runAdd(hostile, "2026-01-evil", { cwd: tmp, nonInteractive: true }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("Unsafe skill name"); + } finally { + rmSync(hostile, { recursive: true, force: true }); + } + }); + + it("refuses a drop id containing ../", async () => { + const hostile = writeHostileCommons([{ id: "../../evil", skills: ["escape-skill"] }]); + try { + const code = await runAdd(hostile, "../../evil", { cwd: tmp, nonInteractive: true }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("Unsafe drop id"); + } finally { + rmSync(hostile, { recursive: true, force: true }); + } + }); + }); + runUnix("never force-replaces a populated mirror directory (the §2 guardrail)", async () => { // A real, populated .claude/skills dir — the shape that must survive. mkdirSync(join(tmp, ".claude/skills/precious-skill"), { recursive: true }); diff --git a/cli/test/push.test.ts b/cli/test/push.test.ts index f1b121c..9bfc352 100644 --- a/cli/test/push.test.ts +++ b/cli/test/push.test.ts @@ -1,8 +1,15 @@ -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { platform, tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { runPush, stripMachineLocalMetadata, summarizeDiff } from "../src/commands/push.js"; +import { + collectPublishablePayload, + runPush, + stripMachineLocalMetadata, + summarizeDiff, +} from "../src/commands/push.js"; + +const runUnix = platform() === "win32" ? it.skip : it; const FIXTURES = join(__dirname, "fixtures"); const MINI_COMMONS = join(FIXTURES, "mini-commons"); @@ -94,7 +101,51 @@ describe("summarizeDiff", () => { }); }); +describe("collectPublishablePayload", () => { + runUnix("allows only the skill payload; skips dotfiles, strays, and symlinks", () => { + const dir = join(tmp, "skills", "payload-skill"); + mkdirSync(join(dir, "scripts"), { recursive: true }); + mkdirSync(join(dir, "references"), { recursive: true }); + mkdirSync(join(dir, "logs"), { recursive: true }); + writeFileSync(join(dir, "SKILL.md"), "---\nname: payload-skill\n---\nbody"); + writeFileSync(join(dir, "scripts", "run.sh"), "echo ok"); + writeFileSync(join(dir, "references", "guide.md"), "# guide"); + writeFileSync(join(dir, ".env"), "SECRET=1"); + writeFileSync(join(dir, "notes.log"), "local notes"); + writeFileSync(join(dir, "scripts", ".cache"), "x"); + writeFileSync(join(dir, "logs", "debug.log"), "x"); + symlinkSync("/etc/hosts", join(dir, "scripts", "link.sh")); + + const { files, skipped } = collectPublishablePayload(dir); + expect(files.sort()).toEqual(["SKILL.md", "references/guide.md", "scripts/run.sh"]); + expect(skipped).toContain(".env (dotfile)"); + expect(skipped).toContain("notes.log (outside SKILL.md + scripts/references/assets)"); + expect(skipped).toContain("logs/ (outside SKILL.md + scripts/references/assets)"); + expect(skipped).toContain("scripts/.cache (dotfile)"); + expect(skipped).toContain("scripts/link.sh (symlink)"); + }); +}); + describe("runPush --dry-run against a local commons", () => { + runUnix("enumerates traveling files and keeps secrets home in the dry-run output", async () => { + writeColonySkill("gamma-skill"); + const dir = join(tmp, "skills", "gamma-skill"); + mkdirSync(join(dir, "scripts"), { recursive: true }); + writeFileSync(join(dir, "scripts", "helper.sh"), "echo hi"); + writeFileSync(join(dir, ".env"), "TOKEN=supersecret"); + symlinkSync("/etc/hosts", join(dir, "hosts-link")); + + const code = await runPush("gamma-skill", { cwd: tmp, to: MINI_COMMONS, dryRun: true }); + restoreConsole(); + expect(code).toBe(0); + const out = logs.join("\n"); + expect(out).toContain("travels: SKILL.md"); + expect(out).toContain("travels: scripts/helper.sh"); + expect(out).toContain("stays home: .env (dotfile)"); + expect(out).toContain("stays home: hosts-link (symlink)"); + expect(out).not.toContain("supersecret"); + }); + it("classifies an upstream-known skill as an evolution with a diff summary", async () => { writeColonySkill("alpha-skill", { body: "2. A new edge case learned in the wild.\n" }); const code = await runPush("alpha-skill", { cwd: tmp, to: MINI_COMMONS, dryRun: true }); diff --git a/docs/plans/2026-07-skdd-commons.md b/docs/plans/2026-07-skdd-commons.md index f4ad217..670594f 100644 --- a/docs/plans/2026-07-skdd-commons.md +++ b/docs/plans/2026-07-skdd-commons.md @@ -236,5 +236,6 @@ plugins/skdd-claude/ *(Append dated entries here as phases complete. Manual steps for Zak accumulate here too.)* - 2026-07-01 — Plan authored (claude-fable-5). Bootstrap skills exist untracked at `packs/fable-festival/`; installed to the global colony the same day. +- 2026-07-01 — **Adversarial review fixes** (claude-fable-5, after a Codex adversarial review of the branch diff). Two findings fixed on `feat/commons-cli`: **[high]** `drops.json` is now treated as hostile input — drop ids and skill names are grammar-checked (lowercase kebab-case, no slashes/`..`/absolute paths) at every parse site (`readDropsManifest`, push's local+remote upstream readers), with defense-in-depth `assertWithin` containment asserts on all source/destination paths in `add`; a malicious manifest can no longer write outside `skills/`. **[medium]** `skdd push` no longer copies the whole skill directory — only an allowlisted payload travels (`SKILL.md` + regular files under `scripts/`, `references/`, `assets/`); dotfiles, symlinks, and strays stay home, and `--dry-run` enumerates travels/stays-home per file. +5 adversarial tests (hostile manifests with `../`, absolute paths, unsafe drop ids; payload allowlist incl. symlink/`.env` exclusion); 899 total green; verified live against a hostile fixture and a real `.env` plant. - 2026-07-01 — **Phase 2 complete** (claude-fable-5). `skdd add` / `skdd push` / `skdd drops` shipped in `cli/` (branch `feat/commons-cli`). New libs: `commons.ts` (source parsing + shallow clone + drops.json), `lock.ts` (`.skdd-lock.json`, full sha for future drift detection), `config.ts` (`~/.skdd/config.toml`, `commons` key, default `zakelfassi/skdd-commons`). 24 new tests (add/push/drops) + mini-commons fixtures; 894 total green; typecheck + lint green. Design note: new skills pushed without `--drop` land in the Commons' `incoming/` staging dir (added to the Commons + its CI same day) — drops stay maintainer-curated. Push against a **local path** target supports `--dry-run` only (test seam); real PRs need `gh`. Round-trip e2e ran against the real private repo: `init → add zakelfassi/skdd-commons 2026-07-frontier` (6 skills, provenance `@12ba029`) `→ edit → push --dry-run` produced the correct evolve-branch PR body with diff summary; populated-mirror guardrail covered by a regression test. - 2026-07-01 — **Phase 1 complete** (claude-fable-5). `zakelfassi/skdd-commons` created **private** (ASK-ZAK default taken: private until launch commit), **MIT license** (default taken: consistency with main repo), **npm drops deferred** (default taken: git transport only for v1). Full layout + `2026-07-frontier` drop (six skills, `metadata.pack` updated, `usage-count` reset, `last-used` dropped). CI green on main (run 28552710128: validate --strict / safety-lint / manifest-check, ~22s total). Gate proven on PR #1: safety-lint failed on a planted pipe-to-shell + credential-read fixture (run 28552744808), passed after `security-reviewed` label applied (run 28552805972); PR closed unmerged, branch deleted. `packs/fable-festival/` deleted from this repo; `packs/README.md` now concept doc + featured-drop index. diff --git a/site/src/content/docs/commons.md b/site/src/content/docs/commons.md new file mode 100644 index 0000000..bccb07e --- /dev/null +++ b/site/src/content/docs/commons.md @@ -0,0 +1,76 @@ +--- +title: "SkDD Commons" +description: "Community skills that evolve in public — dated drops, provenance, and the add/push evolution loop." +--- + +> Community skills that evolve in public — dated drops, provenance, and the add/push evolution loop. + +**[SkDD Commons](https://github.com/zakelfassi/skdd-commons)** is the community repository for SkDD skills. It is a plain git repo — no hosted registry, no server-side index, no submission portal. Drops are directories, the manifest is a JSON file, and contributing is a PR. + +## Drops + +A **drop** is a curated, dated, themed set of skills: `YYYY-MM-`. Dated ids sort chronologically and give each release a story. A skill's `metadata.pack` names the drop it belongs to. + +```bash +skdd drops # list drops from the configured commons +skdd drops --from owner/other-repo # any Commons-shaped repo works +``` + +| Drop | Date | Skills | +|------|------|--------| +| [`2026-07-frontier` — July 2026 Frontier, the Fable Festival drop](https://github.com/zakelfassi/skdd-commons/tree/main/packs/2026-07-frontier) | 2026-07-01 | 6 | + +## Installing: `skdd add` + +```bash +# a whole drop into the project colony +pnpm dlx @zakelfassi/skdd add zakelfassi/skdd-commons 2026-07-frontier + +# a single skill, or the global colony +skdd add zakelfassi/skdd-commons 2026-07-frontier/finish-the-loop +skdd add zakelfassi/skdd-commons 2026-07-frontier -g + +# pin a ref, or use any git URL / local path +skdd add zakelfassi/skdd-commons#main 2026-07-frontier +``` + +What `add` guarantees: + +1. **Validation before install** — every skill must pass `skdd validate --strict`; one failure refuses the whole selection. +2. **Collision safety** — an existing skill with the same name refuses the install; `--rename ` installs a single skill under a different name. +3. **Provenance** — the registry's Source column records `owner/repo@shortsha (drop-id)`; the full sha lands in `.skdd-lock.json` so future tooling can detect upstream drift. +4. **Mirror safety** — mirrors refresh through the same safe link path as `skdd link`: a populated harness directory is never replaced silently. + +`--dry-run` previews, `--json` emits a machine-readable report. + +## Evolving: `skdd push` + +The Commons' entire point. When a skill fails you in the wild: + +```bash +# 1. fix your local copy (skills//SKILL.md) — add the edge case +# 2. preview the PR +skdd push what-would-you-cut --dry-run +# 3. ship it (needs the GitHub CLI authenticated) +skdd push what-would-you-cut +``` + +- Skills that exist upstream branch as `evolve/` and the PR includes a diff summary; new skills branch as `skill/` and land in `incoming/` for maintainer triage (or an existing drop via `--drop `). +- **Machine-local state never travels**: `usage-count` resets to `"0"` and `last-used` is dropped. Your usage stats are your colony's truth. `forged-*` provenance always travels. +- The default target repo comes from `~/.skdd/config.toml`: + +```toml +commons = "zakelfassi/skdd-commons" +``` + +## Security posture + +Community skills are instructions your agent follows — a prompt-injection surface. The Commons treats it as one: CI lints every skill against a versioned deny-pattern list (pipe-to-shell, credential reads, instruction-override phrases, …); a hit blocks merge until a maintainer reviews it and applies the `security-reviewed` label. Details in the Commons' [SECURITY.md](https://github.com/zakelfassi/skdd-commons/blob/main/SECURITY.md). + +This is **not** a review gate on forging — creating skills in your own colony stays gate-free. The gate exists where the trust boundary is: strangers shipping instructions to strangers' agents. + +## Enforcement hooks (Claude Code) + +Two Commons practices ship as **opt-in** hooks in the [`skdd-claude` plugin](https://github.com/zakelfassi/skills-driven-development/tree/main/plugins/skdd-claude): a `finish-the-loop` Stop gate (bounces unverified-success reports once) and a `freeze-the-session` reminder (surfaces unfrozen learnings before the context dies). Both are off by default — toggle with `/skdd-claude:skdd-hooks on`. + +> A skill is a procedure the model follows when it decides to; a hook is a gate for when it forgets. From c1908337e4329162abb98647973e80db03bc1292 Mon Sep 17 00:00:00 2001 From: Zak El Fassi Date: Wed, 1 Jul 2026 17:20:22 -0700 Subject: [PATCH 3/6] fix(cli): refuse symlinked skill dirs and SKILL.md in push readFileSync follows symlinks, so a symlinked SKILL.md (or a symlinked skill directory) could exfiltrate arbitrary file contents into a Commons PR even with the payload allowlist. push now lstats both and refuses; collectPublishablePayload throws defensively. +2 tests. --- cli/src/commands/push.ts | 11 +++++++++++ cli/test/push.test.ts | 26 ++++++++++++++++++++++++++ docs/plans/2026-07-skdd-commons.md | 1 + 3 files changed, 38 insertions(+) diff --git a/cli/src/commands/push.ts b/cli/src/commands/push.ts index 0a7851c..ec3231a 100644 --- a/cli/src/commands/push.ts +++ b/cli/src/commands/push.ts @@ -136,6 +136,14 @@ export async function runPush(target: string, opts: PushOptions = {}): Promise { expect(out).toContain("delta-skill"); }); + runUnix("refuses to push a skill whose SKILL.md is a symlink", async () => { + const dir = join(tmp, "skills", "sneaky-skill"); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(tmp, "secret.txt"), "PRIVATE KEY MATERIAL"); + symlinkSync(join(tmp, "secret.txt"), join(dir, "SKILL.md")); + + const code = await runPush("sneaky-skill", { cwd: tmp, to: MINI_COMMONS, dryRun: true }); + restoreConsole(); + expect(code).toBe(1); + const out = logs.join("\n"); + expect(out).toContain("symlink"); + expect(out).not.toContain("PRIVATE KEY MATERIAL"); + }); + + runUnix("refuses to push a skill whose directory is a symlink", async () => { + mkdirSync(join(tmp, "elsewhere"), { recursive: true }); + writeFileSync(join(tmp, "elsewhere", "SKILL.md"), "---\nname: linked-skill\n---\nbody"); + mkdirSync(join(tmp, "skills"), { recursive: true }); + symlinkSync(join(tmp, "elsewhere"), join(tmp, "skills", "linked-skill")); + + const code = await runPush("linked-skill", { cwd: tmp, to: MINI_COMMONS, dryRun: true }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("symlink"); + }); + it("errors when the target is neither a skill nor a pack id", async () => { const code = await runPush("no-such-thing", { cwd: tmp, to: MINI_COMMONS, dryRun: true }); restoreConsole(); diff --git a/docs/plans/2026-07-skdd-commons.md b/docs/plans/2026-07-skdd-commons.md index 670594f..5a345c1 100644 --- a/docs/plans/2026-07-skdd-commons.md +++ b/docs/plans/2026-07-skdd-commons.md @@ -236,6 +236,7 @@ plugins/skdd-claude/ *(Append dated entries here as phases complete. Manual steps for Zak accumulate here too.)* - 2026-07-01 — Plan authored (claude-fable-5). Bootstrap skills exist untracked at `packs/fable-festival/`; installed to the global colony the same day. +- 2026-07-01 — **Stop-gate follow-up fix** (claude-fable-5, flagged by the Codex stop-time review): `skdd push` read `SKILL.md` with `readFileSync`, which follows symlinks — a symlinked skill dir or SKILL.md could still exfiltrate arbitrary files (ssh keys, /etc/passwd) into a PR despite the payload allowlist. Push now `lstat`s both and refuses symlinks outright; `collectPublishablePayload` throws defensively. +2 tests (symlinked SKILL.md, symlinked skill dir); 901 total green; verified live — a SKILL.md symlinked at a secret file is refused with no content leaked. - 2026-07-01 — **Adversarial review fixes** (claude-fable-5, after a Codex adversarial review of the branch diff). Two findings fixed on `feat/commons-cli`: **[high]** `drops.json` is now treated as hostile input — drop ids and skill names are grammar-checked (lowercase kebab-case, no slashes/`..`/absolute paths) at every parse site (`readDropsManifest`, push's local+remote upstream readers), with defense-in-depth `assertWithin` containment asserts on all source/destination paths in `add`; a malicious manifest can no longer write outside `skills/`. **[medium]** `skdd push` no longer copies the whole skill directory — only an allowlisted payload travels (`SKILL.md` + regular files under `scripts/`, `references/`, `assets/`); dotfiles, symlinks, and strays stay home, and `--dry-run` enumerates travels/stays-home per file. +5 adversarial tests (hostile manifests with `../`, absolute paths, unsafe drop ids; payload allowlist incl. symlink/`.env` exclusion); 899 total green; verified live against a hostile fixture and a real `.env` plant. - 2026-07-01 — **Phase 2 complete** (claude-fable-5). `skdd add` / `skdd push` / `skdd drops` shipped in `cli/` (branch `feat/commons-cli`). New libs: `commons.ts` (source parsing + shallow clone + drops.json), `lock.ts` (`.skdd-lock.json`, full sha for future drift detection), `config.ts` (`~/.skdd/config.toml`, `commons` key, default `zakelfassi/skdd-commons`). 24 new tests (add/push/drops) + mini-commons fixtures; 894 total green; typecheck + lint green. Design note: new skills pushed without `--drop` land in the Commons' `incoming/` staging dir (added to the Commons + its CI same day) — drops stay maintainer-curated. Push against a **local path** target supports `--dry-run` only (test seam); real PRs need `gh`. Round-trip e2e ran against the real private repo: `init → add zakelfassi/skdd-commons 2026-07-frontier` (6 skills, provenance `@12ba029`) `→ edit → push --dry-run` produced the correct evolve-branch PR body with diff summary; populated-mirror guardrail covered by a regression test. - 2026-07-01 — **Phase 1 complete** (claude-fable-5). `zakelfassi/skdd-commons` created **private** (ASK-ZAK default taken: private until launch commit), **MIT license** (default taken: consistency with main repo), **npm drops deferred** (default taken: git transport only for v1). Full layout + `2026-07-frontier` drop (six skills, `metadata.pack` updated, `usage-count` reset, `last-used` dropped). CI green on main (run 28552710128: validate --strict / safety-lint / manifest-check, ~22s total). Gate proven on PR #1: safety-lint failed on a planted pipe-to-shell + credential-read fixture (run 28552744808), passed after `security-reviewed` label applied (run 28552805972); PR closed unmerged, branch deleted. `packs/fable-festival/` deleted from this repo; `packs/README.md` now concept doc + featured-drop index. From cc2fbf16ee0ccbc113c730848bce1feeaba3ef38 Mon Sep 17 00:00:00 2001 From: Zak El Fassi Date: Wed, 1 Jul 2026 19:41:19 -0700 Subject: [PATCH 4/6] fix(cli): guard symlinked skills at push discovery, not just the items loop Pack discovery called parseSkill (readFileSync follows symlinks) before the items-loop symlink guard, so a pack push dereferenced a symlinked skill while scanning. Hoisted the check into a skillDirIsSymlinked helper applied at discovery: direct pushes refuse, pack scans skip+warn, and the items-loop check remains as defense in depth. +1 pack-scan test. --- cli/src/commands/push.ts | 34 +++++++++++++++++++++++++++--- cli/test/push.test.ts | 21 ++++++++++++++++++ docs/plans/2026-07-skdd-commons.md | 1 + 3 files changed, 53 insertions(+), 3 deletions(-) diff --git a/cli/src/commands/push.ts b/cli/src/commands/push.ts index ec3231a..f334e6d 100644 --- a/cli/src/commands/push.ts +++ b/cli/src/commands/push.ts @@ -67,12 +67,28 @@ export async function runPush(target: string, opts: PushOptions = {}): Promise = []; const directDir = join(canonicalDir, target); if (existsSync(join(directDir, "SKILL.md"))) { + // Guard BEFORE any read: parseSkill/readFileSync follow symlinks, so a + // symlinked skill dir or SKILL.md must be rejected up front — otherwise a + // pointer at an ssh key or /etc/passwd gets dereferenced into memory. + if (skillDirIsSymlinked(directDir)) { + logger.error( + `'${target}' is (or its SKILL.md is) a symlink — refusing to push linked content. Replace it with a real file to share it.`, + ); + return 1; + } skillDirs.push({ name: target, dir: directDir }); } else if (existsSync(canonicalDir)) { for (const entry of readdirSync(canonicalDir)) { const dir = join(canonicalDir, entry); const skillMd = join(dir, "SKILL.md"); if (!statSync(dir).isDirectory() || !existsSync(skillMd)) continue; + // Skip symlinked skills during pack discovery so they are never read. + // A canonical skills/ entry should be a real dir; a symlink here is + // suspicious (mirrors point the other way), so exclude and warn. + if (skillDirIsSymlinked(dir)) { + logger.warn(`Skipping '${entry}' from pack push — it is a symlink, not a real skill dir.`); + continue; + } try { const parsed = parseSkill(skillMd); if ( @@ -136,9 +152,9 @@ export async function runPush(target: string, opts: PushOptions = {}): Promise { expect(out).toContain("delta-skill"); }); + runUnix("excludes a symlinked skill from a pack push without dereferencing it", async () => { + writeColonySkill("gamma-skill", { pack: "my-pack" }); + // A symlinked SKILL.md pointing at a secret, whose target claims the pack — + // discovery must skip it (never read it) rather than dereference it. + writeFileSync( + join(tmp, "planted.md"), + "---\nname: evil\nmetadata:\n pack: my-pack\n---\nPRIVATE KEY MATERIAL", + ); + const evilDir = join(tmp, "skills", "evil-skill"); + mkdirSync(evilDir, { recursive: true }); + symlinkSync(join(tmp, "planted.md"), join(evilDir, "SKILL.md")); + + const code = await runPush("my-pack", { cwd: tmp, to: MINI_COMMONS, dryRun: true }); + restoreConsole(); + expect(code).toBe(0); + const out = logs.join("\n"); + expect(out).toContain("gamma-skill"); + expect(out).toContain("Skipping 'evil-skill'"); + expect(out).not.toContain("PRIVATE KEY MATERIAL"); + }); + runUnix("refuses to push a skill whose SKILL.md is a symlink", async () => { const dir = join(tmp, "skills", "sneaky-skill"); mkdirSync(dir, { recursive: true }); diff --git a/docs/plans/2026-07-skdd-commons.md b/docs/plans/2026-07-skdd-commons.md index 5a345c1..612381e 100644 --- a/docs/plans/2026-07-skdd-commons.md +++ b/docs/plans/2026-07-skdd-commons.md @@ -236,6 +236,7 @@ plugins/skdd-claude/ *(Append dated entries here as phases complete. Manual steps for Zak accumulate here too.)* - 2026-07-01 — Plan authored (claude-fable-5). Bootstrap skills exist untracked at `packs/fable-festival/`; installed to the global colony the same day. +- 2026-07-01 — **Stop-gate follow-up fix #2** (claude-fable-5, flagged by the Codex stop-time review): the symlink guard lived in the items loop, but pack discovery called `parseSkill` (→ `readFileSync`, follows symlinks) *before* it — so a pack push dereferenced a symlinked skill during scanning. Extracted `skillDirIsSymlinked` and now check at discovery time: direct pushes refuse, pack scans skip+warn, and the items-loop check stays as defense-in-depth. `statSync`→symlink-aware guard added. +1 test (symlinked skill excluded from a pack push, target never read); 902 total green; verified live — a symlinked SKILL.md claiming the pack is skipped and its secret never read. - 2026-07-01 — **Stop-gate follow-up fix** (claude-fable-5, flagged by the Codex stop-time review): `skdd push` read `SKILL.md` with `readFileSync`, which follows symlinks — a symlinked skill dir or SKILL.md could still exfiltrate arbitrary files (ssh keys, /etc/passwd) into a PR despite the payload allowlist. Push now `lstat`s both and refuses symlinks outright; `collectPublishablePayload` throws defensively. +2 tests (symlinked SKILL.md, symlinked skill dir); 901 total green; verified live — a SKILL.md symlinked at a secret file is refused with no content leaked. - 2026-07-01 — **Adversarial review fixes** (claude-fable-5, after a Codex adversarial review of the branch diff). Two findings fixed on `feat/commons-cli`: **[high]** `drops.json` is now treated as hostile input — drop ids and skill names are grammar-checked (lowercase kebab-case, no slashes/`..`/absolute paths) at every parse site (`readDropsManifest`, push's local+remote upstream readers), with defense-in-depth `assertWithin` containment asserts on all source/destination paths in `add`; a malicious manifest can no longer write outside `skills/`. **[medium]** `skdd push` no longer copies the whole skill directory — only an allowlisted payload travels (`SKILL.md` + regular files under `scripts/`, `references/`, `assets/`); dotfiles, symlinks, and strays stay home, and `--dry-run` enumerates travels/stays-home per file. +5 adversarial tests (hostile manifests with `../`, absolute paths, unsafe drop ids; payload allowlist incl. symlink/`.env` exclusion); 899 total green; verified live against a hostile fixture and a real `.env` plant. - 2026-07-01 — **Phase 2 complete** (claude-fable-5). `skdd add` / `skdd push` / `skdd drops` shipped in `cli/` (branch `feat/commons-cli`). New libs: `commons.ts` (source parsing + shallow clone + drops.json), `lock.ts` (`.skdd-lock.json`, full sha for future drift detection), `config.ts` (`~/.skdd/config.toml`, `commons` key, default `zakelfassi/skdd-commons`). 24 new tests (add/push/drops) + mini-commons fixtures; 894 total green; typecheck + lint green. Design note: new skills pushed without `--drop` land in the Commons' `incoming/` staging dir (added to the Commons + its CI same day) — drops stay maintainer-curated. Push against a **local path** target supports `--dry-run` only (test seam); real PRs need `gh`. Round-trip e2e ran against the real private repo: `init → add zakelfassi/skdd-commons 2026-07-frontier` (6 skills, provenance `@12ba029`) `→ edit → push --dry-run` produced the correct evolve-branch PR body with diff summary; populated-mirror guardrail covered by a regression test. From e22032025dd0ef35de852830bc4a85c3de48afbf Mon Sep 17 00:00:00 2001 From: Zak El Fassi Date: Wed, 1 Jul 2026 20:53:02 -0700 Subject: [PATCH 5/6] fix(cli): address Commons review findings in add/push/commons/config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - add/push honor .colony.json canonicalSkillsDir (pass to runLink; resolve before scanning) — .colony.json users could not add/push before - reject symlinks anywhere in a fetched Commons skill tree (add), matching the push-side guard - registry cells escape pipes/newlines and the parser unescapes, so an untrusted Commons description can't inject fake rows - push: metadata stripping scoped to frontmatter only (body examples with usage-count/last-used lines are preserved) - push: validate each local skill --strict before opening a PR CI would reject; distinguish an empty diff from a real git commit failure and hint at missing git identity; validate pack ids as git-ref-safe branch slugs; clear the upstream dir before an evolve copy so deleted files don't linger - parseSource splits #ref before local-path detection (../commons#feature) and local #ref checks out from a clean clone (never mutates the user repo) - local dirty repos record a -dirty provenance marker + lock flag - malformed ~/.skdd/config.toml surfaces instead of silently defaulting - extract lib/colony.ts (canonicalDirName) shared by add/push +11 tests (hostile symlink/name-mismatch in add, frontmatter-only strip, registry injection round-trip, parseSource #ref, provenance dirty). 914 green. --- cli/src/commands/add.ts | 31 ++++++++++--- cli/src/commands/push.ts | 68 +++++++++++++++++++++++++--- cli/src/lib/colony.ts | 21 +++++++++ cli/src/lib/commons.ts | 95 ++++++++++++++++++++++++++++++++------- cli/src/lib/config.ts | 15 ++++--- cli/src/lib/lock.ts | 1 + cli/src/lib/registry.ts | 20 +++++++-- cli/test/add.test.ts | 60 ++++++++++++++++++++++++- cli/test/commons.test.ts | 68 ++++++++++++++++++++++++++++ cli/test/push.test.ts | 22 +++++++++ cli/test/registry.test.ts | 23 ++++++++++ 11 files changed, 385 insertions(+), 39 deletions(-) create mode 100644 cli/src/lib/colony.ts create mode 100644 cli/test/commons.test.ts diff --git a/cli/src/commands/add.ts b/cli/src/commands/add.ts index 56f2f10..0c9a237 100644 --- a/cli/src/commands/add.ts +++ b/cli/src/commands/add.ts @@ -9,6 +9,7 @@ import { provenanceLabel, readDropsManifest, resolveSelector, + treeHasSymlink, } from "../lib/commons.js"; import { ensureGlobalColony, skddHome } from "../lib/global.js"; import { upsertLockEntry } from "../lib/lock.js"; @@ -45,9 +46,8 @@ export async function runAdd( const cwd = resolve(opts.cwd ?? process.cwd()); const colonyRoot = opts.global ? skddHome() : cwd; if (opts.global) ensureGlobalColony(); - const canonicalDir = opts.global - ? join(skddHome(), "skills") - : join(cwd, detectCanonical(cwd) ?? "skills"); + const canonicalName = opts.global ? "skills" : (detectCanonical(cwd) ?? "skills"); + const canonicalDir = opts.global ? join(skddHome(), "skills") : join(cwd, canonicalName); if (opts.rename) { const err = validateRename(opts.rename); @@ -119,12 +119,23 @@ export async function runAdd( const parsedSkills: Array<{ sourceName: string; dir: string; description: string }> = []; for (const skillName of selection.skills) { assertWithin(join(dropDir, skillName), dropDir, `skill '${skillName}'`); - const skillMd = join(dropDir, skillName, "SKILL.md"); + const skillDir = join(dropDir, skillName); + const skillMd = join(skillDir, "SKILL.md"); if (!existsSync(skillMd)) { logger.error(`${selection.drop.id}/${skillName}: SKILL.md missing in the source repo.`); validationFailed = true; continue; } + // A symlink anywhere in the fetched skill tree would be copied as a link + // into the colony (dangling once a temp clone is deleted, or pointing back + // into the source for a local add) — never a provenance-pinned copy. Reject. + if (treeHasSymlink(skillDir)) { + logger.error( + `${selection.drop.id}/${skillName}: contains a symlink — refusing (Commons skills must be self-contained regular files).`, + ); + validationFailed = true; + continue; + } try { const parsed = parseSkill(skillMd); const errors = validateSkill(parsed, { strict: true }).filter( @@ -136,6 +147,11 @@ export async function runAdd( validationFailed = true; continue; } + // Note: the manifest name equals the on-disk directory name by + // construction (we read packs///), and validateSkill + // already enforces frontmatter.name === directory name — so a manifest + // entry pointing at a differently-named skill fails strict validation + // above. No separate check needed. parsedSkills.push({ sourceName: skillName, dir: join(dropDir, skillName), @@ -180,7 +196,7 @@ export async function runAdd( name, sourceName: s.sourceName, path: join(relative(colonyRoot, canonicalDir) || canonicalDir, name, "SKILL.md"), - provenance: provenanceLabel(fetched.source, fetched.sha, selection.drop.id), + provenance: provenanceLabel(fetched.source, fetched.sha, selection.drop.id, fetched.dirty), description: s.description, }; }); @@ -240,15 +256,18 @@ export async function runAdd( source: fetched.source.label, drop: selection.drop.id, sha: fetched.sha, + dirty: fetched.dirty, addedAt: new Date().toISOString(), }); if (!opts.json) logger.success(`installed ${p.name} (${p.provenance})`); } // ── refresh mirrors through the existing SAFE link path (never forced) ─── + // Pass the detected canonical dir so a .colony.json canonicalSkillsDir + // (e.g. "playbooks") is mirrored, not a default "skills/" that we never wrote. const linkCode = opts.global ? await runLink({ global: true, quiet: true }) - : await runLink({ cwd, quiet: true }); + : await runLink({ cwd, canonical: canonicalName, quiet: true }); if (opts.json) { console.log( diff --git a/cli/src/commands/push.ts b/cli/src/commands/push.ts index f334e6d..9b8fc23 100644 --- a/cli/src/commands/push.ts +++ b/cli/src/commands/push.ts @@ -13,12 +13,14 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join, resolve } from "node:path"; +import { canonicalDirName } from "../lib/colony.js"; import { assertSafeManifestNames } from "../lib/commons.js"; import { loadConfig } from "../lib/config.js"; import { ensureGlobalColony, skddHome } from "../lib/global.js"; import { logger } from "../lib/logger.js"; import { parseSkill } from "../lib/skill.js"; import { NAME_REGEX } from "../lib/spec.js"; +import { validateSkill } from "./validate.js"; export interface PushOptions { cwd?: string; @@ -61,7 +63,9 @@ export async function runPush(target: string, opts: PushOptions = {}): Promise = []; @@ -108,6 +112,16 @@ export async function runPush(target: string, opts: PushOptions = {}): Promise` as the branch name; a pack id + // with spaces or slashes yields an invalid git ref. Require a ref-safe slug. + const isPackPush = !existsSync(join(directDir, "SKILL.md")) && skillDirs.length > 1; + if (isPackPush && !/^[A-Za-z0-9](?:[A-Za-z0-9._-]*[A-Za-z0-9])?$/.test(target)) { + logger.error( + `Pack id '${target}' is not a valid git branch component — use letters, digits, '.', '_', '-' (no spaces or slashes).`, + ); + return 1; + } + // ── resolve the target Commons ───────────────────────────────────────────── const toRaw = opts.to ?? loadConfig().commons; const localTarget = resolveLocalTarget(toRaw, cwd); @@ -163,7 +177,21 @@ export async function runPush(target: string, opts: PushOptions = {}): Promise; + try { + parsed = parseSkill(join(dir, "SKILL.md")); + } catch (err) { + logger.error(`'${name}': cannot parse SKILL.md — ${(err as Error).message}`); + return 1; + } + // Validate before assembling the PR — a malformed local skill would open a + // PR the Commons CI just rejects. Fail locally with the actual reasons. + const errors = validateSkill(parsed, { strict: true }).filter((i) => i.severity === "error"); + if (errors.length > 0) { + logger.error(`'${name}' fails skdd validate --strict — fix it before pushing:`); + for (const e of errors) logger.dim(` ${e.field ? `[${e.field}] ` : ""}${e.message}`); + return 1; + } const meta = (parsed.frontmatter.metadata ?? {}) as Record; const upstreamDrop = upstream.manifest.drops.find((d) => d.skills.includes(name))?.id ?? null; const destDir = upstreamDrop @@ -321,10 +349,21 @@ function truncate(s: string, max: number): string { /** * Machine-local state stays home: usage-count resets, last-used is dropped. - * Line-based on purpose — gray-matter round-trips would reformat the YAML. + * Scoped to the YAML frontmatter block ONLY — a body line like `usage-count: 5` + * inside a fenced example must not be rewritten. Line-based on purpose: + * gray-matter round-trips would reformat the whole YAML. */ export function stripMachineLocalMetadata(raw: string): string { - return raw.replace(/^(\s*usage-count:\s*).+$/m, `$1"0"`).replace(/^\s*last-used:.*\r?\n/m, ""); + const fm = raw.match(/^(---\r?\n)([\s\S]*?)(\r?\n---\r?\n?)/); + if (!fm) return raw; // no frontmatter → nothing machine-local to strip + const stripped = fm[2]! + .replace(/^(\s*usage-count:\s*).+$/m, `$1"0"`) + .replace(/^\s*last-used:.*\r?\n?/m, ""); + return ( + raw.slice(0, fm.index! + fm[1]!.length) + + stripped + + raw.slice(fm.index! + fm[1]!.length + fm[2]!.length) + ); } /** @@ -449,13 +488,18 @@ function openPr(repo: string, pr: PlannedPr, opts: PushOptions): number { for (const item of pr.items) { const dest = join(tmp, item.destDir); + // For an evolution the upstream dir already exists in the clone; clear it + // first so files the local skill deleted don't silently survive in the PR. + if (item.upstreamDrop && existsSync(dest)) { + rmSync(dest, { recursive: true, force: true }); + } + mkdirSync(dest, { recursive: true }); // Copy ONLY the allowlisted payload — never the whole local directory. for (const rel of item.files) { if (rel === "SKILL.md") continue; // written below from the stripped content mkdirSync(dirname(join(dest, rel)), { recursive: true }); copyFileSync(join(item.localDir, rel), join(dest, rel)); } - mkdirSync(dest, { recursive: true }); writeFileSync(join(dest, "SKILL.md"), item.content); } // A new skill headed for an existing drop must also appear in drops.json, @@ -471,9 +515,21 @@ function openPr(repo: string, pr: PlannedPr, opts: PushOptions): number { } git(["add", "-A"], tmp); + // Distinguish "no changes" from a real commit failure (e.g. missing git + // user.name/email in a fresh CI env) — reporting both as "nothing to push" + // hides an actionable error. + const staged = git(["diff", "--cached", "--quiet"], tmp); + if (staged.status === 0) { + logger.error(`Nothing to push — the Commons already has this exact content.`); + return 1; + } const commit = git(["commit", "-m", pr.title], tmp); if (commit.status !== 0) { - logger.error(`Nothing to push — the Commons already has this exact content.`); + const detail = (commit.stderr || commit.stdout).trim(); + logger.error(`git commit failed${detail ? `: ${detail}` : ""}`); + if (/user\.(name|email)|author identity unknown/i.test(detail)) { + logger.dim(`Set a git identity: git config --global user.name/user.email, then retry.`); + } return 1; } const pushRemote = headRepo === repo ? "origin" : `https://github.com/${headRepo}.git`; diff --git a/cli/src/lib/colony.ts b/cli/src/lib/colony.ts new file mode 100644 index 0000000..72ff07f --- /dev/null +++ b/cli/src/lib/colony.ts @@ -0,0 +1,21 @@ +import { existsSync, readFileSync } from "node:fs"; +import { join } from "node:path"; + +/** + * The canonical skills directory for a colony root: `.colony.json`'s + * `canonicalSkillsDir` when set, else the default `skills`. Shared by add/push + * so both honor a project that uses e.g. `playbooks/` as its canonical dir. + */ +export function canonicalDirName(root: string): string { + const p = join(root, ".colony.json"); + if (!existsSync(p)) return "skills"; + try { + const manifest = JSON.parse(readFileSync(p, "utf8")) as { canonicalSkillsDir?: string }; + if (typeof manifest.canonicalSkillsDir === "string" && manifest.canonicalSkillsDir.length > 0) { + return manifest.canonicalSkillsDir; + } + } catch { + // malformed .colony.json is doctor's concern, not ours + } + return "skills"; +} diff --git a/cli/src/lib/commons.ts b/cli/src/lib/commons.ts index 216143f..cd20a0d 100644 --- a/cli/src/lib/commons.ts +++ b/cli/src/lib/commons.ts @@ -1,5 +1,5 @@ import { spawnSync } from "node:child_process"; -import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { existsSync, lstatSync, mkdtempSync, readdirSync, readFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { isAbsolute, join, relative, resolve } from "node:path"; import { NAME_MAX_LENGTH, NAME_REGEX } from "./spec.js"; @@ -37,23 +37,31 @@ const GIT_URL_PREFIX = /^(?:https?:\/\/|git@|ssh:\/\/)/; * local path — each with an optional `#ref` (branch/tag/sha) suffix on git forms. */ export function parseSource(raw: string, cwd: string): CommonsSource { - // Local path forms first: explicit path prefixes, or a path that exists on disk. - if ( - raw.startsWith(".") || - raw.startsWith("/") || - raw.startsWith("~") || - existsSync(resolve(cwd, raw)) - ) { - const localPath = raw.startsWith("~") - ? join(process.env.HOME ?? "", raw.slice(1)) - : resolve(cwd, raw); - return { kind: "local", label: raw, localPath }; + const toLocal = (p: string, ref?: string): CommonsSource => { + const localPath = p.startsWith("~") + ? join(process.env.HOME ?? "", p.slice(1)) + : resolve(cwd, p); + return { kind: "local", label: p, localPath, ref }; + }; + + // A path that exists verbatim wins first — this preserves local paths that + // legitimately contain a literal '#' (no ref is inferred from them). + if (existsSync(resolve(cwd, raw))) { + return toLocal(raw); } const hashIdx = raw.lastIndexOf("#"); const ref = hashIdx > 0 ? raw.slice(hashIdx + 1) : undefined; const base = hashIdx > 0 ? raw.slice(0, hashIdx) : raw; + // Local by prefix or on-disk existence — computed AFTER stripping the #ref so + // e.g. ../commons#feature parses as {local ../commons, ref feature} instead of + // a nonexistent literal path. + const baseLooksLocal = base.startsWith(".") || base.startsWith("/") || base.startsWith("~"); + if (baseLooksLocal || existsSync(resolve(cwd, base))) { + return toLocal(base, ref); + } + if (GIT_URL_PREFIX.test(base)) { // Try to extract owner/repo from common URL shapes for the label. const m = base.match(/[/:]([A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+?)(?:\.git)?\/?$/); @@ -74,8 +82,11 @@ export interface FetchedCommons { dir: string; /** Full commit sha of the fetched state, or null when the source isn't a git repo. */ sha: string | null; + /** True when the source was a local git repo with uncommitted changes — the + * recorded sha does NOT contain the installed bytes, so provenance is marked. */ + dirty: boolean; source: CommonsSource; - /** Remove the temp clone (no-op for local sources). */ + /** Remove the temp clone (no-op for local sources read in place). */ cleanup: () => void; } @@ -99,10 +110,35 @@ export function fetchCommons(source: CommonsSource): FetchedCommons { if (!existsSync(dir)) { throw new Error(`Local source does not exist: ${dir}`); } + // A local source with a #ref must be checked out from a clean clone — + // never mutate the user's working repo. This also yields honest provenance. + if (source.ref) { + const tmp = mkdtempSync(join(tmpdir(), "skdd-commons-")); + const cleanup = () => rmSync(tmp, { recursive: true, force: true }); + const cl = git(["clone", "--quiet", dir, tmp]); + if (cl.status !== 0) { + cleanup(); + throw new Error(`cannot clone local source '${dir}': ${cl.stderr || cl.stdout}`); + } + const co = git(["checkout", "--detach", source.ref], tmp); + if (co.status !== 0) { + cleanup(); + throw new Error( + `cannot check out ref '${source.ref}' in '${dir}': ${co.stderr || co.stdout}`, + ); + } + const rev = git(["rev-parse", "HEAD"], tmp); + return { dir: tmp, sha: rev.status === 0 ? rev.stdout : null, dirty: false, source, cleanup }; + } const rev = git(["rev-parse", "HEAD"], dir); + // Working-tree read in place: if the repo is dirty, HEAD doesn't contain + // the bytes we're about to copy, so flag it for provenance. + const status = git(["status", "--porcelain"], dir); + const dirty = rev.status === 0 && status.status === 0 && status.stdout.length > 0; return { dir, sha: rev.status === 0 ? rev.stdout : null, + dirty, source, cleanup: () => {}, }; @@ -136,7 +172,7 @@ export function fetchCommons(source: CommonsSource): FetchedCommons { } const rev = git(["rev-parse", "HEAD"], tmp); - return { dir: tmp, sha: rev.status === 0 ? rev.stdout : null, source, cleanup }; + return { dir: tmp, sha: rev.status === 0 ? rev.stdout : null, dirty: false, source, cleanup }; } /** Read and minimally validate a Commons repo's drops.json. */ @@ -225,8 +261,33 @@ export function resolveSelector(manifest: DropsManifest, selector: string): Reso return { drop, skills: [skillName] }; } -/** Registry Source column label for an added skill: `owner/repo@shortsha (drop-id)`. */ -export function provenanceLabel(source: CommonsSource, sha: string | null, dropId: string): string { - const shortSha = sha ? sha.slice(0, 7) : "local"; +/** Registry Source column label for an added skill: `owner/repo@shortsha (drop-id)`. + * A dirty local source is marked `@shortsha-dirty` so the sha isn't mistaken + * for a commit that actually contains the installed bytes. */ +export function provenanceLabel( + source: CommonsSource, + sha: string | null, + dropId: string, + dirty = false, +): string { + const shortSha = sha ? `${sha.slice(0, 7)}${dirty ? "-dirty" : ""}` : "local"; return `${source.label}@${shortSha} (${dropId})`; } + +/** True if `entry` (a file or dir path) is a symlink, or — for a directory — + * contains any symlink at any depth. Used to keep symlinked payload out of a + * provenance-pinned copy. */ +export function treeHasSymlink(entry: string): boolean { + let stat: ReturnType; + try { + stat = lstatSync(entry); + } catch { + return false; + } + if (stat.isSymbolicLink()) return true; + if (!stat.isDirectory()) return false; + for (const child of readdirSync(entry)) { + if (treeHasSymlink(join(entry, child))) return true; + } + return false; +} diff --git a/cli/src/lib/config.ts b/cli/src/lib/config.ts index 5646150..541581b 100644 --- a/cli/src/lib/config.ts +++ b/cli/src/lib/config.ts @@ -19,12 +19,15 @@ export function loadConfig(): SkddConfig { const defaults: SkddConfig = { commons: DEFAULT_COMMONS }; const p = configPath(); if (!existsSync(p)) return defaults; + // A present-but-broken config is a user error, not "no config" — surface it, + // so a typo in a configured private Commons doesn't silently target the default. + let raw: Record; try { - const raw = parseToml(readFileSync(p, "utf8")) as Record; - return { - commons: typeof raw["commons"] === "string" ? (raw["commons"] as string) : defaults.commons, - }; - } catch { - return defaults; + raw = parseToml(readFileSync(p, "utf8")) as Record; + } catch (err) { + throw new Error(`Malformed ${p}: ${(err as Error).message}`); } + return { + commons: typeof raw["commons"] === "string" ? (raw["commons"] as string) : defaults.commons, + }; } diff --git a/cli/src/lib/lock.ts b/cli/src/lib/lock.ts index fd0bbaa..4469180 100644 --- a/cli/src/lib/lock.ts +++ b/cli/src/lib/lock.ts @@ -11,6 +11,7 @@ export interface LockEntry { source: string; // owner/repo, git URL, or local path label drop: string; sha: string | null; // full commit sha of the source at add time + dirty?: boolean; // true when a local source had uncommitted changes (sha ≠ installed bytes) addedAt: string; // ISO timestamp } diff --git a/cli/src/lib/registry.ts b/cli/src/lib/registry.ts index 3dc92f7..2c1b4b6 100644 --- a/cli/src/lib/registry.ts +++ b/cli/src/lib/registry.ts @@ -93,9 +93,23 @@ export function parseMarkdownRegistry(source: string): Registry { return { skills, archived }; } +/** + * Make a value safe to drop into a markdown table cell: newlines would break the + * row, and unescaped pipes would be read as extra columns by parseMarkdownRegistry + * (and could inject fake rows from untrusted Commons metadata). + */ +function cell(value: string): string { + return value + .replace(/\r?\n+/g, " ") + .replace(/\|/g, "\\|") + .trim(); +} + function splitTableRow(row: string): string[] { const trimmed = row.replace(/^\|/, "").replace(/\|$/, ""); - return trimmed.split("|").map((c) => c.trim()); + // Split on unescaped pipes only, then unescape — mirrors cell()'s `\|` escaping + // so a description containing a literal pipe round-trips as one cell. + return trimmed.split(/(? c.replace(/\\\|/g, "|").trim()); } /** Serialize a Registry back to markdown. Stable ordering: skills as given, archived as given. */ @@ -114,7 +128,7 @@ export function writeMarkdownRegistry(registry: Registry, projectName?: string): for (const s of registry.skills) { lines.push( - `| ${s.name} | ${s.source} | ${s.lastUsed ?? ""} | ${s.uses ?? 0} | ${s.description} |`, + `| ${cell(s.name)} | ${cell(s.source)} | ${cell(s.lastUsed ?? "")} | ${s.uses ?? 0} | ${cell(s.description)} |`, ); } @@ -127,7 +141,7 @@ export function writeMarkdownRegistry(registry: Registry, projectName?: string): "|-------|----------|--------|", ); for (const a of registry.archived) { - lines.push(`| ${a.name} | ${a.lastUsed ?? ""} | ${a.description} |`); + lines.push(`| ${cell(a.name)} | ${cell(a.lastUsed ?? "")} | ${cell(a.description)} |`); } } diff --git a/cli/test/add.test.ts b/cli/test/add.test.ts index c55fa99..eca7c21 100644 --- a/cli/test/add.test.ts +++ b/cli/test/add.test.ts @@ -1,4 +1,12 @@ -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { platform, tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; @@ -223,6 +231,56 @@ describe("runAdd", () => { } }); + runUnix("refuses a source skill whose tree contains a symlink", async () => { + const dir = mkdtempSync(join(tmpdir(), "skdd-symcommons-")); + try { + writeFileSync( + join(dir, "drops.json"), + JSON.stringify({ version: 1, drops: [{ id: "2026-01-x", skills: ["linky"] }] }), + ); + const sd = join(dir, "packs", "2026-01-x", "linky"); + mkdirSync(join(sd, "scripts"), { recursive: true }); + writeFileSync( + join(sd, "SKILL.md"), + "---\nname: linky\ndescription: has a symlink. Use when testing.\n---\n# L\n1. x\n", + ); + writeFileSync(join(dir, "secret.txt"), "SECRET"); + symlinkSync(join(dir, "secret.txt"), join(sd, "scripts", "leak")); + const code = await runAdd(dir, "2026-01-x", { cwd: tmp, nonInteractive: true }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("contains a symlink"); + expect(existsSync(join(tmp, "skills", "linky"))).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("refuses when a skill's frontmatter name does not match its manifest/dir name", async () => { + // manifest name == directory name by construction; validateSkill enforces + // frontmatter.name == directory, so a mismatch fails strict validation. + const dir = mkdtempSync(join(tmpdir(), "skdd-namecommons-")); + try { + writeFileSync( + join(dir, "drops.json"), + JSON.stringify({ version: 1, drops: [{ id: "2026-01-x", skills: ["claimed"] }] }), + ); + const sd = join(dir, "packs", "2026-01-x", "claimed"); + mkdirSync(sd, { recursive: true }); + writeFileSync( + join(sd, "SKILL.md"), + "---\nname: actually-different\ndescription: mismatch. Use when testing.\n---\n# X\n1. y\n", + ); + const code = await runAdd(dir, "2026-01-x", { cwd: tmp, nonInteractive: true }); + restoreConsole(); + expect(code).toBe(1); + expect(logs.join("\n")).toContain("does not match directory"); + expect(existsSync(join(tmp, "skills", "claimed"))).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it("refuses a drop id containing ../", async () => { const hostile = writeHostileCommons([{ id: "../../evil", skills: ["escape-skill"] }]); try { diff --git a/cli/test/commons.test.ts b/cli/test/commons.test.ts new file mode 100644 index 0000000..70e3981 --- /dev/null +++ b/cli/test/commons.test.ts @@ -0,0 +1,68 @@ +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { parseSource, provenanceLabel } from "../src/lib/commons.js"; + +let tmp: string; + +beforeEach(() => { + tmp = mkdtempSync(join(tmpdir(), "skdd-commons-unit-")); +}); + +afterEach(() => { + rmSync(tmp, { recursive: true, force: true }); +}); + +describe("parseSource", () => { + it("parses GitHub shorthand with a #ref", () => { + const s = parseSource("owner/repo#v1.2.3", tmp); + expect(s.kind).toBe("git"); + expect(s.label).toBe("owner/repo"); + expect(s.cloneUrl).toBe("https://github.com/owner/repo.git"); + expect(s.ref).toBe("v1.2.3"); + }); + + it("parses a git URL with a #ref", () => { + const s = parseSource("https://github.com/owner/repo.git#main", tmp); + expect(s.kind).toBe("git"); + expect(s.ref).toBe("main"); + expect(s.label).toBe("owner/repo"); + }); + + it("splits #ref off a relative local path (does not treat it as a literal path)", () => { + const s = parseSource("../commons#feature", tmp); + expect(s.kind).toBe("local"); + expect(s.ref).toBe("feature"); + // base path resolved, fragment stripped + expect(s.localPath?.endsWith("/commons")).toBe(true); + }); + + it("treats an absolute local path as local with no inferred ref", () => { + const s = parseSource("/abs/path/commons", tmp); + expect(s.kind).toBe("local"); + expect(s.ref).toBeUndefined(); + }); + + it("rejects an unrecognized source", () => { + expect(() => parseSource("not a source", tmp)).toThrow(/Unrecognized source/); + }); +}); + +describe("provenanceLabel", () => { + it("formats owner/repo@shortsha (drop)", () => { + expect(provenanceLabel({ kind: "git", label: "o/r" }, "abcdef1234567890", "2026-07-x")).toBe( + "o/r@abcdef1 (2026-07-x)", + ); + }); + + it("marks a dirty local source", () => { + expect(provenanceLabel({ kind: "local", label: "../c" }, "abcdef1234567890", "d", true)).toBe( + "../c@abcdef1-dirty (d)", + ); + }); + + it("falls back to @local when no sha is known", () => { + expect(provenanceLabel({ kind: "local", label: "../c" }, null, "d")).toBe("../c@local (d)"); + }); +}); diff --git a/cli/test/push.test.ts b/cli/test/push.test.ts index a09e3a2..f947e0c 100644 --- a/cli/test/push.test.ts +++ b/cli/test/push.test.ts @@ -91,6 +91,28 @@ body`; expect(stripped).toContain("forged-by: claude-fable-5"); expect(stripped).toContain(`forged-reason: "why"`); }); + + it("only touches the frontmatter, not example lines in the body", () => { + const raw = `--- +name: x +metadata: + usage-count: "9" + last-used: "2026-06-30" +--- +# Docs + +Example config: + + usage-count: "5" + last-used: "2020-01-01" +`; + const stripped = stripMachineLocalMetadata(raw); + // frontmatter reset/dropped + expect(stripped).toContain(` usage-count: "0"`); + // body example preserved verbatim + expect(stripped).toContain(` usage-count: "5"`); + expect(stripped).toContain(` last-used: "2020-01-01"`); + }); }); describe("summarizeDiff", () => { diff --git a/cli/test/registry.test.ts b/cli/test/registry.test.ts index 1cc5650..3341b28 100644 --- a/cli/test/registry.test.ts +++ b/cli/test/registry.test.ts @@ -172,4 +172,27 @@ describe("addRegistryEntry", () => { expect(parsed.skills).toHaveLength(1); expect(parsed.skills[0]!.name).toBe("json-sync"); }); + + it("escapes pipes and newlines in cell values so untrusted metadata can't inject rows", () => { + const md = writeMarkdownRegistry({ + skills: [ + { + name: "injected", + source: "owner/repo@abc1234 (drop)", + uses: 0, + // A description that tries to add a fake column / fake row. + description: "evil | 999 | fake\nsecond line", + }, + ], + archived: [], + }); + // One data row (header + separator + 1), no injected extras. + const dataRows = md.split("\n").filter((l) => l.startsWith("| injected")); + expect(dataRows).toHaveLength(1); + // Round-trips back to a single skill with the literal description intact. + const back = parseMarkdownRegistry(md); + expect(back.skills).toHaveLength(1); + expect(back.skills[0]!.description).toContain("evil | 999 | fake"); + expect(back.skills[0]!.uses).toBe(0); + }); }); From 86e9913192c539202ffa0bc3bd573ad52dafe3d7 Mon Sep 17 00:00:00 2001 From: Zak El Fassi Date: Wed, 1 Jul 2026 21:54:00 -0700 Subject: [PATCH 6/6] fix(cli): address second-round PR review comments on add/push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - local #ref add clones with --branch (branch/tag refs resolve; sha falls back to detach) instead of failing on origin-only branches - --dry-run (including -g) no longer creates ~/.skdd - reject symlinked pack ancestors via realpath containment (a Commons making packs/ or the drop dir a symlink can't serve outside bytes) - validate pack ids with a full git-ref rule (reject foo.lock, a..b, …) - push payload skips non-regular files (FIFO/socket/device would hang cp) - strip an emptied metadata: block so the pushed SKILL.md has no null key - push validates the stripped payload (what the Commons CI sees) - rewrite drops.json only when a new skill is actually added Deferred: renamed-then-pushed skills classify by local name (niche). +8 tests; 919 green. --- cli/src/commands/add.ts | 31 ++++++++++++++--- cli/src/commands/push.ts | 74 ++++++++++++++++++++++++++++++++------- cli/src/lib/commons.ts | 75 +++++++++++++++++++++++++++++++++------- cli/test/add.test.ts | 19 ++++++++++ cli/test/commons.test.ts | 21 ++++++++++- cli/test/push.test.ts | 28 +++++++++++++++ 6 files changed, 218 insertions(+), 30 deletions(-) diff --git a/cli/src/commands/add.ts b/cli/src/commands/add.ts index 0c9a237..ee22b90 100644 --- a/cli/src/commands/add.ts +++ b/cli/src/commands/add.ts @@ -2,6 +2,7 @@ import { cpSync, existsSync, readFileSync, writeFileSync } from "node:fs"; import { join, relative, resolve } from "node:path"; import { select } from "@inquirer/prompts"; import { + assertRealpathWithin, assertWithin, type DropsManifest, fetchCommons, @@ -45,7 +46,8 @@ export async function runAdd( ): Promise { const cwd = resolve(opts.cwd ?? process.cwd()); const colonyRoot = opts.global ? skddHome() : cwd; - if (opts.global) ensureGlobalColony(); + // NOTE: don't ensureGlobalColony() here — a --dry-run must not create + // ~/.skdd. It's created only on the real install path below. const canonicalName = opts.global ? "skills" : (detectCanonical(cwd) ?? "skills"); const canonicalDir = opts.global ? join(skddHome(), "skills") : join(cwd, canonicalName); @@ -115,6 +117,16 @@ export async function runAdd( // layer: no source or destination path may leave its expected root. const dropDir = join(fetched.dir, "packs", selection.drop.id); assertWithin(dropDir, join(fetched.dir, "packs"), `drop '${selection.drop.id}'`); + // Reject a symlinked ancestor (packs/, the drop dir): a lstat-based tree walk + // starting at the skill dir wouldn't see it, but existsSync/parseSkill/cpSync + // all follow it — so a Commons could serve bytes from outside the fetched + // tree while recording Commons provenance. realpath-contain the drop dir. + try { + assertRealpathWithin(dropDir, fetched.dir, `drop '${selection.drop.id}'`); + } catch (err) { + logger.error((err as Error).message); + return 1; + } let validationFailed = false; const parsedSkills: Array<{ sourceName: string; dir: string; description: string }> = []; for (const skillName of selection.skills) { @@ -126,9 +138,17 @@ export async function runAdd( validationFailed = true; continue; } - // A symlink anywhere in the fetched skill tree would be copied as a link - // into the colony (dangling once a temp clone is deleted, or pointing back - // into the source for a local add) — never a provenance-pinned copy. Reject. + // The skill dir's realpath must also stay inside the fetched tree... + try { + assertRealpathWithin(skillDir, fetched.dir, `skill '${skillName}'`); + } catch (err) { + logger.error((err as Error).message); + validationFailed = true; + continue; + } + // ...and no symlink may appear anywhere inside it (dangling once a temp + // clone is deleted, or pointing back into the source for a local add) — + // never a provenance-pinned copy. Reject. if (treeHasSymlink(skillDir)) { logger.error( `${selection.drop.id}/${skillName}: contains a symlink — refusing (Commons skills must be self-contained regular files).`, @@ -234,6 +254,9 @@ export async function runAdd( } // ── install: copy, rename, register, lock ─────────────────────────────── + // Now (and only now) materialize the global colony — deferred from the top + // so a --dry-run never touches ~/.skdd. + if (opts.global) ensureGlobalColony(); for (let i = 0; i < parsedSkills.length; i++) { const s = parsedSkills[i]!; const p = planned[i]!; diff --git a/cli/src/commands/push.ts b/cli/src/commands/push.ts index 9b8fc23..ff83b05 100644 --- a/cli/src/commands/push.ts +++ b/cli/src/commands/push.ts @@ -13,8 +13,9 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join, resolve } from "node:path"; +import matter from "gray-matter"; import { canonicalDirName } from "../lib/colony.js"; -import { assertSafeManifestNames } from "../lib/commons.js"; +import { assertSafeManifestNames, isGitRefComponent } from "../lib/commons.js"; import { loadConfig } from "../lib/config.js"; import { ensureGlobalColony, skddHome } from "../lib/global.js"; import { logger } from "../lib/logger.js"; @@ -113,11 +114,12 @@ export async function runPush(target: string, opts: PushOptions = {}): Promise` as the branch name; a pack id - // with spaces or slashes yields an invalid git ref. Require a ref-safe slug. + // that isn't a valid git ref component (e.g. `foo.lock`, `a..b`, spaces) would + // fail at checkout/push after cloning. Reject up front. const isPackPush = !existsSync(join(directDir, "SKILL.md")) && skillDirs.length > 1; - if (isPackPush && !/^[A-Za-z0-9](?:[A-Za-z0-9._-]*[A-Za-z0-9])?$/.test(target)) { + if (isPackPush && !isGitRefComponent(target)) { logger.error( - `Pack id '${target}' is not a valid git branch component — use letters, digits, '.', '_', '-' (no spaces or slashes).`, + `Pack id '${target}' is not a valid git branch component (no spaces, '..', trailing '.lock', or leading '.'/'-').`, ); return 1; } @@ -184,9 +186,10 @@ export async function runPush(target: string, opts: PushOptions = {}): Promise i.severity === "error"); + // Validate the STRIPPED content — that's the SKILL.md the Commons CI will + // see. Validating the local original could pass while the pushed payload + // (e.g. an emptied metadata block) fails upstream. + const errors = validateStrippedSkill(name, content).filter((i) => i.severity === "error"); if (errors.length > 0) { logger.error(`'${name}' fails skdd validate --strict — fix it before pushing:`); for (const e of errors) logger.dim(` ${e.field ? `[${e.field}] ` : ""}${e.message}`); @@ -356,9 +359,13 @@ function truncate(s: string, max: number): string { export function stripMachineLocalMetadata(raw: string): string { const fm = raw.match(/^(---\r?\n)([\s\S]*?)(\r?\n---\r?\n?)/); if (!fm) return raw; // no frontmatter → nothing machine-local to strip - const stripped = fm[2]! + let stripped = fm[2]! .replace(/^(\s*usage-count:\s*).+$/m, `$1"0"`) .replace(/^\s*last-used:.*\r?\n?/m, ""); + // If removing last-used emptied the metadata block (a `metadata:` key with no + // remaining indented children), drop the dangling key too — otherwise the + // pushed SKILL.md carries `metadata:` as YAML null. + stripped = dropEmptyMetadata(stripped); return ( raw.slice(0, fm.index! + fm[1]!.length) + stripped + @@ -366,6 +373,36 @@ export function stripMachineLocalMetadata(raw: string): string { ); } +/** Validate the stripped SKILL.md content that will actually travel upstream. */ +function validateStrippedSkill(name: string, content: string) { + const parsed = matter(content); + const body = parsed.content; + const skill = { + path: `${name}/SKILL.md`, + dir: name, + dirName: name, + frontmatter: (parsed.data ?? {}) as Record, + body, + lineCount: content.split(/\r?\n/).length, + hasScripts: false, + hasReferences: false, + hasAssets: false, + }; + return validateSkill(skill, { strict: true }); +} + +/** Remove a `metadata:` key whose block has no indented children left. */ +function dropEmptyMetadata(yaml: string): string { + const lines = yaml.split(/\r?\n/); + const idx = lines.findIndex((l) => /^metadata:\s*$/.test(l)); + if (idx === -1) return yaml; + const next = lines[idx + 1]; + const hasChild = next !== undefined && /^\s+\S/.test(next); + if (hasChild) return yaml; + lines.splice(idx, 1); + return lines.join("\n"); +} + /** * True if the skill directory itself, or its SKILL.md, is a symlink. * `readFileSync`/`parseSkill`/`cpSync` all follow symlinks, so a symlinked @@ -403,8 +440,11 @@ export function collectPublishablePayload(skillDir: string): { skipped.push(`${relPath}${stat.isDirectory() ? "/" : ""} (dotfile)`); } else if (stat.isDirectory()) { walk(relPath); - } else { + } else if (stat.isFile()) { files.push(relPath); + } else { + // FIFO, socket, device node — copyFileSync would hang or fail. Skip. + skipped.push(`${relPath} (not a regular file)`); } } }; @@ -418,6 +458,8 @@ export function collectPublishablePayload(skillDir: string): { skipped.push(`${entry}${stat.isDirectory() ? "/" : ""} (dotfile)`); } else if (stat.isDirectory() && PUBLISHABLE_SUBDIRS.has(entry)) { walk(entry); + } else if (!stat.isFile() && !stat.isDirectory()) { + skipped.push(`${entry} (not a regular file)`); } else { skipped.push( `${entry}${stat.isDirectory() ? "/" : ""} (outside SKILL.md + scripts/references/assets)`, @@ -502,16 +544,22 @@ function openPr(repo: string, pr: PlannedPr, opts: PushOptions): number { } writeFileSync(join(dest, "SKILL.md"), item.content); } - // A new skill headed for an existing drop must also appear in drops.json, + // A NEW skill headed for an existing drop must also appear in drops.json, // or the Commons' manifest-check job fails the PR on a stray directory. - if (opts.drop) { + // Only rewrite the manifest when we actually add a name — an evolution-only + // push must not gain an unrelated manifest reformat. + if (opts.drop && pr.items.some((i) => !i.upstreamDrop)) { const manifestPath = join(tmp, "drops.json"); const manifest = JSON.parse(readFileSync(manifestPath, "utf8")); const drop = manifest.drops.find((d: { id: string }) => d.id === opts.drop); + let added = false; for (const item of pr.items) { - if (!item.upstreamDrop && !drop.skills.includes(item.name)) drop.skills.push(item.name); + if (!item.upstreamDrop && !drop.skills.includes(item.name)) { + drop.skills.push(item.name); + added = true; + } } - writeFileSync(manifestPath, `${JSON.stringify(manifest, null, 2)}\n`); + if (added) writeFileSync(manifestPath, `${JSON.stringify(manifest, null, 2)}\n`); } git(["add", "-A"], tmp); diff --git a/cli/src/lib/commons.ts b/cli/src/lib/commons.ts index cd20a0d..b153a05 100644 --- a/cli/src/lib/commons.ts +++ b/cli/src/lib/commons.ts @@ -1,5 +1,13 @@ import { spawnSync } from "node:child_process"; -import { existsSync, lstatSync, mkdtempSync, readdirSync, readFileSync, rmSync } from "node:fs"; +import { + existsSync, + lstatSync, + mkdtempSync, + readdirSync, + readFileSync, + realpathSync, + rmSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { isAbsolute, join, relative, resolve } from "node:path"; import { NAME_MAX_LENGTH, NAME_REGEX } from "./spec.js"; @@ -115,17 +123,24 @@ export function fetchCommons(source: CommonsSource): FetchedCommons { if (source.ref) { const tmp = mkdtempSync(join(tmpdir(), "skdd-commons-")); const cleanup = () => rmSync(tmp, { recursive: true, force: true }); - const cl = git(["clone", "--quiet", dir, tmp]); - if (cl.status !== 0) { - cleanup(); - throw new Error(`cannot clone local source '${dir}': ${cl.stderr || cl.stdout}`); - } - const co = git(["checkout", "--detach", source.ref], tmp); - if (co.status !== 0) { - cleanup(); - throw new Error( - `cannot check out ref '${source.ref}' in '${dir}': ${co.stderr || co.stdout}`, - ); + // `--branch ` resolves branches AND tags (a fresh clone of a local + // repo only has origin/, so a bare `checkout --detach ` + // would fail). Fall back to a plain clone + detach for a raw sha. + let cloned = git(["clone", "--quiet", "--branch", source.ref, dir, tmp]).status === 0; + if (!cloned) { + const cl = git(["clone", "--quiet", dir, tmp]); + if (cl.status !== 0) { + cleanup(); + throw new Error(`cannot clone local source '${dir}': ${cl.stderr || cl.stdout}`); + } + const co = git(["checkout", "--detach", source.ref], tmp); + if (co.status !== 0) { + cleanup(); + throw new Error( + `cannot check out ref '${source.ref}' in '${dir}': ${co.stderr || co.stdout}`, + ); + } + cloned = true; } const rev = git(["rev-parse", "HEAD"], tmp); return { dir: tmp, sha: rev.status === 0 ? rev.stdout : null, dirty: false, source, cleanup }; @@ -229,6 +244,42 @@ export function assertWithin(child: string, parent: string, label: string): void } } +/** + * Assert `child`'s REAL path (symlinks resolved) stays inside `parent`'s real + * path. Catches a symlinked ancestor (e.g. a Commons making `packs/` a symlink) + * that a plain string containment check would miss. + */ +export function assertRealpathWithin(child: string, parent: string, label: string): void { + let realChild: string; + let realParent: string; + try { + realParent = realpathSync(parent); + realChild = realpathSync(child); + } catch { + throw new Error(`${label} could not be resolved (dangling symlink?) — refusing.`); + } + const rel = relative(realParent, realChild); + if (rel !== "" && (rel.startsWith("..") || isAbsolute(rel))) { + throw new Error(`${label} resolves (via symlink) outside ${parent} — refusing.`); + } +} + +/** + * Whether `name` is a safe single git branch/ref component: rejects `..`, + * a trailing `.lock`, control/space/special chars, leading/trailing dots or + * dashes, and `@{` — matching what `git check-ref-format` would reject, without + * shelling out. + */ +export function isGitRefComponent(name: string): boolean { + if (!name || name.length > 200) return false; + if (name.includes("..") || name.includes("@{")) return false; + if (name.endsWith(".lock") || name.endsWith(".") || name.endsWith("/")) return false; + if (name.startsWith(".") || name.startsWith("-") || name.startsWith("/")) return false; + // No spaces, control chars, or any of the git-forbidden set ~^:?*[\ + // (also excludes '/', keeping this a single component). + return /^[A-Za-z0-9][A-Za-z0-9._-]*$/.test(name); +} + export interface ResolvedSelection { drop: DropEntry; /** Skill names selected within the drop. */ diff --git a/cli/test/add.test.ts b/cli/test/add.test.ts index eca7c21..72fd9b1 100644 --- a/cli/test/add.test.ts +++ b/cli/test/add.test.ts @@ -149,6 +149,25 @@ describe("runAdd", () => { expect(logs.join("\n")).toContain("would install alpha-skill"); }); + it("-g --dry-run does not create the global colony", async () => { + const fakeHome = join(tmp, "fake-skdd-home"); + const prev = process.env.SKDD_HOME; + process.env.SKDD_HOME = fakeHome; + try { + const code = await runAdd(MINI_COMMONS, "2026-01-test", { + global: true, + nonInteractive: true, + dryRun: true, + }); + restoreConsole(); + expect(code).toBe(0); + expect(existsSync(fakeHome)).toBe(false); // ~/.skdd never materialized + } finally { + if (prev === undefined) delete process.env.SKDD_HOME; + else process.env.SKDD_HOME = prev; + } + }); + it("--json emits a machine-readable report", async () => { const code = await runAdd(MINI_COMMONS, "2026-01-test", { cwd: tmp, diff --git a/cli/test/commons.test.ts b/cli/test/commons.test.ts index 70e3981..6e71789 100644 --- a/cli/test/commons.test.ts +++ b/cli/test/commons.test.ts @@ -2,7 +2,7 @@ import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { parseSource, provenanceLabel } from "../src/lib/commons.js"; +import { isGitRefComponent, parseSource, provenanceLabel } from "../src/lib/commons.js"; let tmp: string; @@ -66,3 +66,22 @@ describe("provenanceLabel", () => { expect(provenanceLabel({ kind: "local", label: "../c" }, null, "d")).toBe("../c@local (d)"); }); }); + +describe("isGitRefComponent", () => { + it("accepts ordinary drop/pack ids", () => { + expect(isGitRefComponent("2026-07-frontier")).toBe(true); + expect(isGitRefComponent("my-pack")).toBe(true); + expect(isGitRefComponent("v1.2.3")).toBe(true); + }); + + it("rejects git-invalid names", () => { + expect(isGitRefComponent("foo.lock")).toBe(false); // trailing .lock + expect(isGitRefComponent("a..b")).toBe(false); // double dot + expect(isGitRefComponent("my pack")).toBe(false); // space + expect(isGitRefComponent("has/slash")).toBe(false); // slash + expect(isGitRefComponent(".hidden")).toBe(false); // leading dot + expect(isGitRefComponent("-dash")).toBe(false); // leading dash + expect(isGitRefComponent("bad@{ref")).toBe(false); // @{ + expect(isGitRefComponent("")).toBe(false); + }); +}); diff --git a/cli/test/push.test.ts b/cli/test/push.test.ts index f947e0c..e9aeb37 100644 --- a/cli/test/push.test.ts +++ b/cli/test/push.test.ts @@ -1,3 +1,4 @@ +import { execFileSync } from "node:child_process"; import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { platform, tmpdir } from "node:os"; import { join } from "node:path"; @@ -113,6 +114,20 @@ Example config: expect(stripped).toContain(` usage-count: "5"`); expect(stripped).toContain(` last-used: "2020-01-01"`); }); + + it("drops a metadata block emptied by removing last-used (no dangling null key)", () => { + const raw = `--- +name: x +description: A skill. Use when testing. +metadata: + last-used: "2026-06-30" +--- +body`; + const stripped = stripMachineLocalMetadata(raw); + expect(stripped).not.toMatch(/^metadata:\s*$/m); // no dangling `metadata:` → null + expect(stripped).not.toContain("last-used"); + expect(stripped).toContain("name: x"); + }); }); describe("summarizeDiff", () => { @@ -146,6 +161,19 @@ describe("collectPublishablePayload", () => { expect(skipped).toContain("scripts/.cache (dotfile)"); expect(skipped).toContain("scripts/link.sh (symlink)"); }); + + runUnix("skips non-regular files (a FIFO) instead of trying to copy them", () => { + const dir = join(tmp, "skills", "fifo-skill"); + mkdirSync(join(dir, "scripts"), { recursive: true }); + writeFileSync(join(dir, "SKILL.md"), "---\nname: fifo-skill\n---\nbody"); + writeFileSync(join(dir, "scripts", "ok.sh"), "echo ok"); + execFileSync("mkfifo", [join(dir, "scripts", "pipe")]); + + const { files, skipped } = collectPublishablePayload(dir); + expect(files).toContain("scripts/ok.sh"); + expect(files).not.toContain("scripts/pipe"); + expect(skipped).toContain("scripts/pipe (not a regular file)"); + }); }); describe("runPush --dry-run against a local commons", () => {