feat(cli): skdd add/push/drops — Commons install + upstream evolution flow#8
Conversation
skdd add <source> [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 <skill|pack>: 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/<name> with a diff summary; new skills branch as skill/<name> 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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 147e9260c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const dest = join(canonicalDir, p.name); | ||
| cpSync(s.dir, dest, { recursive: true }); |
There was a problem hiding this comment.
When a Commons manifest lists a skill with path segments such as ../outside, strict validation can still pass if the source directory basename and frontmatter name are outside, but p.name remains the untrusted manifest string. This destination join then escapes the canonical skills/ directory and cpSync can write files outside the colony while the registry/lock record the traversal name. Validate manifest/selector skill ids with the skill-name regex and compare them to the parsed frontmatter before using them in paths.
Useful? React with 👍 / 👎.
| const linkCode = opts.global | ||
| ? await runLink({ global: true, quiet: true }) | ||
| : await runLink({ cwd, quiet: true }); |
There was a problem hiding this comment.
For projects whose .colony.json sets canonicalSkillsDir (for example playbooks), runAdd installs into that directory via detectCanonical, but this link call omits the same canonical value. runLink therefore defaults to skills/, so the add either fails after copying because skills/ is missing, or refreshes mirrors from a stale skills/ directory and leaves the newly added skill invisible to harnesses.
Useful? React with 👍 / 👎.
| const colonyRoot = opts.global ? skddHome() : cwd; | ||
| if (opts.global) ensureGlobalColony(); | ||
| const canonicalDir = join(colonyRoot, "skills"); |
There was a problem hiding this comment.
skdd push always searches <colonyRoot>/skills, so a project using .colony.json with canonicalSkillsDir (which doctor, import, and add support) cannot push skills installed in its actual canonical directory, and pack scans miss them as well. Resolve the canonical skills directory from the colony manifest before building directDir and scanning entries.
Useful? React with 👍 / 👎.
| * 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, ""); |
There was a problem hiding this comment.
When a skill's markdown body contains an example line starting with usage-count: or last-used: (for example in a YAML snippet), this regex rewrites or removes that body line because it scans the whole file instead of just the YAML frontmatter. skdd push then sends corrupted instructions upstream even though only machine-local metadata was supposed to be stripped; bound the replacement to the frontmatter block or parse only those metadata keys.
Useful? React with 👍 / 👎.
|
|
||
| for (const item of pr.items) { | ||
| const dest = join(tmp, item.destDir); | ||
| cpSync(item.localDir, dest, { recursive: true }); |
There was a problem hiding this comment.
For evolutions where the Commons already has packs/<drop>/<name>, cpSync overlays local files onto the cloned directory but never removes files the local skill deleted. A user who removes an obsolete script or reference and runs skdd push gets a PR that silently keeps the stale upstream file, so the Commons content no longer matches the local evolved skill; clear the destination before copying for existing skill dirs.
Useful? React with 👍 / 👎.
| } catch { | ||
| return defaults; |
There was a problem hiding this comment.
When ~/.skdd/config.toml exists but has invalid TOML, this catch silently falls back to zakelfassi/skdd-commons. In skdd push/drops, a typo in a user's configured private Commons can therefore target the default repo with no warning; surface the parse error instead of treating an existing-but-bad config the same as no config.
Useful? React with 👍 / 👎.
| const rev = git(["rev-parse", "HEAD"], dir); | ||
| return { | ||
| dir, | ||
| sha: rev.status === 0 ? rev.stdout : null, |
There was a problem hiding this comment.
For a local source that is a git repo with uncommitted changes, skdd add copies the working-tree bytes but records rev-parse HEAD as the lock sha. The registry/lock then claim the installed content came from a commit that does not contain it, breaking the provenance and future drift guarantees; detect dirty local repos and either refuse them or record a dirty/null sha.
Useful? React with 👍 / 👎.
| if ( | ||
| raw.startsWith(".") || | ||
| raw.startsWith("/") || | ||
| raw.startsWith("~") || | ||
| existsSync(resolve(cwd, raw)) |
There was a problem hiding this comment.
#ref before local-path detection
Local Commons sources with a ref, such as ../commons#feature, are treated as literal filesystem paths here because local-path detection runs before the fragment is split below. Existing local repos therefore fail with “Local source does not exist” instead of checking out the requested branch/tag; strip the fragment before resolving local paths.
Useful? React with 👍 / 👎.
…yload 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17fde65ddc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const p = planned[i]!; | ||
| const dest = join(canonicalDir, p.name); | ||
| assertWithin(dest, canonicalDir, `install target '${p.name}'`); | ||
| cpSync(s.dir, dest, { recursive: true }); |
There was a problem hiding this comment.
When a Commons repo contains a valid manifest entry whose skill directory, or any file inside it, is a symlink, validation follows the link but this copy preserves the symlink in the user's colony. For remote sources the temp clone is deleted immediately afterward, so a top-level symlinked skill can be installed as a broken link; for local sources it stays linked to the source repo instead of being a provenance-pinned copy. Reject symlinks in the source tree or copy with explicit dereferencing after checking the real paths stay within the fetched Commons payload.
Useful? React with 👍 / 👎.
| const raw = readFileSync(join(dir, "SKILL.md"), "utf8"); | ||
| const content = stripMachineLocalMetadata(raw); | ||
| const payload = collectPublishablePayload(dir); | ||
| const parsed = parseSkill(join(dir, "SKILL.md")); |
There was a problem hiding this comment.
For a local skill that was edited manually, this parse result is trusted without running the same validateSkill checks used on install, so a frontmatter name that no longer matches the directory (or other validation errors) can still produce and open a Commons PR that CI will reject; malformed frontmatter can also throw out of runPush instead of returning an actionable error. Validate the parsed skill and fail locally before building the payload.
Useful? React with 👍 / 👎.
| path: p.path, | ||
| lastUsed: new Date().toISOString().slice(0, 10), | ||
| uses: 0, | ||
| description: p.description, |
There was a problem hiding this comment.
When an installed Commons skill has a valid description containing | or a newline, this unescaped external value is written into the markdown registry table; the existing registry parser splits rows on |, so later skdd list/doctor runs can truncate or misread the entry after skdd add. Normalize or escape table-cell text before passing Commons metadata into addRegistryEntry.
Useful? React with 👍 / 👎.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1908337e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (commit.status !== 0) { | ||
| logger.error(`Nothing to push — the Commons already has this exact content.`); | ||
| return 1; |
There was a problem hiding this comment.
In fresh agent/CI environments where gh is authenticated but Git has no user.name/user.email, git commit -m ... exits non-zero with “Author identity unknown” (verified with HOME set to an empty temp dir). This branch treats every commit failure as “Nothing to push”, so a real push is blocked with a false no-op message and no hint to set Git identity; check for an empty diff separately and otherwise surface commit.stderr or configure a local author.
Useful? React with 👍 / 👎.
| 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}`; |
There was a problem hiding this comment.
When pushing a pack, target comes from the user-selected metadata.pack value and is not constrained to git-ref-safe text before becoming pack/${target}. I checked the git branch-name validator (git check-ref-format --branch <branchname-shorthand>); common pack labels such as my pack are rejected, so dry-run prints a branch that cannot be created and the real flow later fails after committing in the temp clone. Validate the pack id or derive a safe branch slug before building the PR plan.
Useful? React with 👍 / 👎.
| --- | ||
| title: "SkDD Commons" | ||
| description: "Community skills that evolve in public — dated drops, provenance, and the add/push evolution loop." |
There was a problem hiding this comment.
This page is added directly under site/src/content/docs, but the site runs pnpm sync before dev/build and site/scripts/sync-docs.mjs removes everything in that generated directory except index.mdx before copying its mapped canonical docs. Because there is no docs/commons.md mapping, pnpm build deletes this file and the Commons route will not ship; put the content in the canonical docs source and add it to the sync mappings/sidebar.
Useful? React with 👍 / 👎.
…s 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.
- 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.
|
Addressed the Codex review findings in commit e220320 (plus the earlier security passes): Fixed
+11 tests here; 914→932 green across the stack. Note: the manifest-name==frontmatter-name suggestion is already enforced transitively (manifest name = dir name by construction; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e22032025d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cleanup(); | ||
| throw new Error(`cannot clone local source '${dir}': ${cl.stderr || cl.stdout}`); | ||
| } | ||
| const co = git(["checkout", "--detach", source.ref], tmp); |
There was a problem hiding this comment.
Fresh evidence in this revision is that the local #ref path now reaches this checkout, but branch refs still fail when the source repo is not already on that branch: a fresh clone of a local repo has origin/feature, not a local feature, and git checkout -h describes --detach as detaching at a named commit, so git checkout --detach feature exits 128 instead of guessing the remote-tracking branch. This leaves the documented skdd add ../commons#feature flow failing with “cannot check out ref” for normal local branch refs; clone with --branch or resolve origin/<ref> before detaching.
Useful? React with 👍 / 👎.
| // A multi-skill pack push uses `pack/<target>` 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)) { |
There was a problem hiding this comment.
Fresh evidence in this revision: the new regex still accepts pack ids such as foo.lock and a..b, but git check-ref-format --branch pack/foo.lock and git check-ref-format --branch pack/a..b reject them as invalid branch names. For a multi-skill pack with one of those metadata.pack values, dry-run advertises pack/<target> and the real flow continues until checkout/push fails after cloning, so validate with Git's ref checker or a complete ref-name rule before building the PR plan.
Useful? React with 👍 / 👎.
| ): Promise<number> { | ||
| const cwd = resolve(opts.cwd ?? process.cwd()); | ||
| const colonyRoot = opts.global ? skddHome() : cwd; | ||
| if (opts.global) ensureGlobalColony(); |
There was a problem hiding this comment.
When skdd add -g ... --dry-run is run on a machine without a global colony, this call creates ~/.skdd/skills and .skills-registry.md before the later dry-run branch prints “No files written”. That makes the dry-run mutate the user's home directory even for a plan-only install; defer ensureGlobalColony() until the non-dry-run install path, or make it read-only for dry runs.
Useful? React with 👍 / 👎.
| // 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)) { |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new symlink guard starts at skillDir, so a Commons can make packs/<drop> (or packs) itself a symlink and this lstat-based walk never sees that ancestor; existsSync/parseSkill then follow the link and cpSync installs bytes from outside the fetched Commons tree while recording Commons provenance. Check the realpath/ancestors stay under the fetched repo, or run the symlink rejection on packs, the drop dir, and the skill dir before following them.
Useful? React with 👍 / 👎.
| } | ||
| // 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) { |
There was a problem hiding this comment.
When --drop is supplied for an evolution (or a pack where every item already has upstreamDrop), this branch still reads and rewrites drops.json even though no skill is being added to that drop. In Commons repos whose manifest formatting/order is not byte-for-byte JSON.stringify(..., 2), an evolution PR gains an unrelated manifest rewrite, or can become the only staged change; gate this on the planned manifest update instead of opts.drop alone.
Useful? React with 👍 / 👎.
| } else if (stat.isDirectory()) { | ||
| walk(relPath); | ||
| } else { | ||
| files.push(relPath); |
There was a problem hiding this comment.
For a skill that has a FIFO, socket, or device node under an allowlisted directory such as scripts/, this branch treats it as a publishable payload because it is neither a symlink, dotfile, nor directory. The real push later calls copyFileSync on that path, which can hang indefinitely for a FIFO waiting for a writer (or fail after cloning), despite the docs promising only regular files travel; require stat.isFile() here and skip everything else.
Useful? React with 👍 / 👎.
| return 1; | ||
| } | ||
| const meta = (parsed.frontmatter.metadata ?? {}) as Record<string, unknown>; | ||
| const upstreamDrop = upstream.manifest.drops.find((d) => d.skills.includes(name))?.id ?? null; |
There was a problem hiding this comment.
When a user installs alpha-skill with skdd add .../alpha-skill --rename alpha-two and later runs skdd push alpha-two, this lookup searches the upstream manifest for alpha-two, but the Commons still lists alpha-skill. The push is therefore misclassified as a new skill and writes incoming/alpha-two (or adds alpha-two to a drop) instead of evolving the original; persist the upstream source name during add and use it for this classification and destination path.
Useful? React with 👍 / 👎.
| } | ||
| // 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"); |
There was a problem hiding this comment.
For a local skill whose metadata only contains last-used, stripMachineLocalMetadata removes that child line and leaves metadata: with no value in the content that is written to the PR. This validation still checks the unstripped local parse, so skdd push can succeed locally while the Commons CI validates the submitted SKILL.md, sees metadata as null, and rejects it; validate the stripped content or remove empty metadata blocks before assembling the PR.
Useful? React with 👍 / 👎.
- 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.
|
Second-round review findings addressed in 86e9913:
Deferred (noted, non-destructive): a skill installed with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86e9913192
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const entry of readdirSync(canonicalDir)) { | ||
| const dir = join(canonicalDir, entry); | ||
| const skillMd = join(dir, "SKILL.md"); | ||
| if (!statSync(dir).isDirectory() || !existsSync(skillMd)) continue; |
There was a problem hiding this comment.
When skdd push <pack> scans a colony that contains a dangling symlink under the canonical skills directory, this statSync(dir) follows the broken link and throws before the symlink guard below can skip it. That aborts the whole push/lookup with an ENOENT-style error even though symlinked skills are meant to be ignored during pack discovery; use lstatSync first and skip symlinks before following the entry.
Useful? React with 👍 / 👎.
| } | ||
| let raw: unknown; | ||
| try { | ||
| raw = JSON.parse(readFileSync(p, "utf8")); |
There was a problem hiding this comment.
For a remote Commons repo that commits drops.json as a symlink, the temp clone preserves that link and this readFileSync follows it before any of the later realpath/symlink guards run. That lets an untrusted source used with skdd add or skdd drops make the CLI read, or block on, an arbitrary local path such as an absolute symlink to a JSON file or FIFO; lstat/realpath-check the manifest itself and refuse symlinks before parsing it.
Useful? React with 👍 / 👎.
| .replace(/^(\s*usage-count:\s*).+$/m, `$1"0"`) | ||
| .replace(/^\s*last-used:.*\r?\n?/m, ""); |
There was a problem hiding this comment.
When a valid skill uses inline YAML for metadata, e.g. metadata: { usage-count: "7", last-used: "2026-06-30" }, neither of these line-start regexes matches the machine-local keys. skdd push then writes the local usage count and last-used date into the Commons PR despite promising that this state stays home; strip these keys from the parsed frontmatter or reject inline metadata before publishing.
Useful? React with 👍 / 👎.
| } | ||
| return { version: LOCK_VERSION, skills: raw.skills as Record<string, LockEntry> }; | ||
| } catch { | ||
| return { version: LOCK_VERSION, skills: {} }; |
There was a problem hiding this comment.
If .skdd-lock.json exists but is temporarily malformed or truncated, this catch treats it as an empty lock; the next skdd add then calls saveLock with only the newly installed skill and overwrites all previous Commons provenance. Fail the add with the parse error or back up the bad file instead of silently discarding existing lock data.
Useful? React with 👍 / 👎.
1 participant
Phase 2 of docs/plans/2026-07-skdd-commons.md.
What
skdd add <source> [selector]— install a drop or single skill from a Commons repo (GitHub shorthand / git URL with#ref/ local path), with strict validation, collision check (--rename), provenance in the registry (owner/repo@shortsha (drop-id)) and full sha in.skdd-lock.json, mirror refresh through the existing safe link path (never forced).skdd push <skill|pack>— open a PR on the Commons viagh: strips machine-local state, preservesforged-*,evolve/<name>branch + diff summary for upstream-known skills,skill/<name>intoincoming/(or--drop <id>) for new ones.--dry-runprints the full PR without network writes.skdd drops [--from]— list a Commons' drops.commons.ts,lock.ts,config.ts(~/.skdd/config.toml, keycommons).Why no new runtime dependencies
Everything uses existing deps (commander, gray-matter, @inquirer/prompts, smol-toml) plus
git/ghas external tools — consistent with the no-hosted-registry stance: git is the transport.Tests
24 new tests across
add.test.ts,push.test.ts,drops.test.tswith amini-commonsfixture: happy paths, validation refusal, collision +--rename, dry-run, JSON output, and the §2 guardrail regression (add never force-replaces a populated mirror dir). Full suite: 894 passed. Round-trip e2e ran against the live Commons repo (see plan §9 log).Closes phase 2 of the Commons plan.