diff --git a/AGENTS.md b/AGENTS.md index 11abd72e..ce2d64e8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -268,6 +268,7 @@ SIN-Code/ │ │ ├── lessons/ ← v3.4.0: closed learning loop │ │ ├── autonomy/ ← v3.5.0: goal queue + triggers │ │ ├── skillmgr/ ← v3.5.0: install/verify skills +│ │ ├── skilldist/ ← v3.18.0: marker-fenced skill distribution (issue #169) │ │ ├── loopbuilder/ ← v3.4.0: shared factory (DRY) │ │ ├── vane/ ← v3.8.0: HTTP bridge to ItzCrazyKns/Vane (internal/vane) │ │ ├── stack/ ← v3.8.0: unified install/doctor across 3 layers @@ -416,6 +417,44 @@ Each skill **must** contain: Skills ported from external repos (e.g. `Infra-SIN-OpenCode-Stack`) must include `lifecycle: external` and `sources:` in their metadata. +### Skill distribution to external agents (issue #169) + +`sin-code skill install --agent ` distributes a bundled +Skill artifact to one of eight registered agent families. The single +source of truth is `cmd/sin-code/internal/skilldist/Targets`: + +| Target | Format | Install path template (relative to `$SIN_CODE_HOME`) | +|---|---|---| +| `claude-code` | `dir` | `.claude/skills/` | +| `opencode` | `dir` | `.config/opencode/skills/` | +| `gemini` | `dir` | `.gemini/skills/` | +| `codex` | `rule` | `.codex/rules/.md` | +| `cursor` | `rule` | `.cursor/rules/.mdc` | +| `windsurf` | `rule` | `.windsurf/rules/.md` | +| `cline` | `rule` | `.clinerules/.md` | +| `copilot` | `marker` | `.github/copilot-instructions.md` | + +**Marker-fence contract.** Every write for `rule` and `marker` Formats +goes through `ParseMarkers` so a subsequent install with the same +`(target, skill)` pair replaces the previously written block in place: + +``` + +… rendered body … + +``` + +The leading/trailing ASCII prefix `SIN-CODE-SKILL` is the rg-friendly +anchor. The trailing whitespace before `` on the END marker is +visual alignment only — `ParseMarkers` strips it on lookup, so any +strict matcher outside this package works too. + +**Public API surface.** `(Target.Name, Target.DisplayName)` is exposed +via the `--agent ` flag. Adding a target is non-breaking; renaming +or removing one is a major bump. The `sin-code skill list --json` +output schema is also a public API — preferred-format changes go through +the same major-bump policy. + ### CLI subcommands (verified `cmd/sin-code/main.go`, v3.5.0) ``` diff --git a/cmd/sin-code/internal/skilldist/skilldist.doc.md b/cmd/sin-code/internal/skilldist/skilldist.doc.md new file mode 100644 index 00000000..7f87eedd --- /dev/null +++ b/cmd/sin-code/internal/skilldist/skilldist.doc.md @@ -0,0 +1,113 @@ +# `cmd/sin-code/internal/skilldist` — marker-fenced skill distribution + +Issue: **#169** (multi-agent Skill distribution via `--agent` flag). +Schema version: **v1.0.0** (first stable cut). + +This CoDoc is the rg-friendly reference for the `skilldist` package. It is +intentionally short: the package doc comment in `skilldist.go` carries the +authoritative spec, and this file is the catalog of "what lives where". + +``` +internal/skilldist/ +├── skilldist.go — package API (Target struct, Targets map, Install, +│ Uninstall, IsInstalled, ParseMarkers, RenderBlock, +│ StripFrontmatter, Resolve, BeginMarker, EndMarker) +└── skilldist_test.go — race-safe unit tests + table-driven registry tests +``` + +## One-page API surface + +| Symbol | Kind | Stability | Notes | +|---|---|---|---| +| `Target` | struct | public | (Name, DisplayName, InstallPath, Format) | +| `Targets` | map | public | Single source of truth — must stay <=8 entries this PR. | +| `TargetNames()` | sort helper | public | Alphabetical; for stable log lines. | +| `InstallOptions` | struct | public | `{SrcRoot, Home, Body}` — see Install. | +| `Install(skill, target, opts) error` | writer | public | Idempotent marker-fence write. | +| `Uninstall(target, skill, home) error` | writer | public | Reversible marker-fence delete. | +| `IsInstalled(target, skill, home) (bool, error)` | probe | public | Format-aware existence check. | +| `Resolve(target, skill, home) (path, error)` | path-resolver | public | Expand `` and join to home. | +| `ParseMarkers(content, skill) ParseResult` | scanner | public | Line-based fence locator. | +| `RenderBlock(skill, body) string` | renderer | public | Begin/body/End fenced block. | +| `StripFrontmatter(raw) string` | helper | public | YAML frontmatter trim. | +| `BeginMarker(name) string` | constant-formatter | public | ASCII: `` | +| `EndMarker(name) string` | constant-formatter | public | ASCII: `` | +| `FormatDir / FormatRule / FormatMarker` | constants | public | Writer dispatch. | +| `MarkerPrefix = "SIN-CODE-SKILL"` | constant | public | Visible in `head -n1` output. | + +## Marker-fence contract + +Every block written by `Install` for `FormatRule` or `FormatMarker` MUST +begin with `` and end with +`` (note the trailing spaces before +`` on the end marker — visual alignment in `head -n 2`). + +`ParseMarkers` strips that whitespace during lookup, so a hand-rolled +editor outside this package that produces `` +without the padding still parses correctly. + +## Format semantics + +| Format | Path shape | Writer | +|---|---|---| +| `dir` | `/>/` (directory) | copies SKILL.md + optional context/frameworks/tasks/templates | +| `rule` | `/>` (single file, .md or .mdc) | writes RenderBlock; replaces prior block for same skill | +| `marker` | `/` (single shared file, no `` placeholder) | appends RenderBlock; preserves other skills' blocks | + +## Registered targets (8, alphabetical) + +Format `dir`: `claude-code`, `gemini`, `opencode`. +Format `rule`: `cline`, `codex`, `cursor`, `windsurf`. +Format `marker`: `copilot`. + +The `sin-code skill install --agent all` command runs through every +target in `TargetNames()` order — determinism is required so the verify-gate +can diff log output across machines. + +## Test layout + +`skilldist_test.go` is one file with three families of tests: + +1. **Registry** — `TestTargets_AllPresent`, `TestTargets_FieldsValidates`. + Lock the set and schema; alarm if a future contributor adds a target + without updating AGENTS.md / CHANGELOG / CLI completion. +2. **Marker parser** — round-trip, distinct skills, half-opened fence, + CRLF tolerance, multi-block preservation. These tests are the safety + net for the `FormatMarker` case where a regression would tear another + skill's block. +3. **Install / Uninstall round trip** — per-target idempotent re-install, + marker-file uninstall that preserves other blocks, FormatMarker empty- + file cleanup. + +## Why this exists + +`sin-code skill` had two responsibilities before this PR: + +- v3.5.0 (`skillmgr`): clones upstream skill **repos** (websearch, scheduler, + …) and verifies their MCP entrypoint. +- v3.6.0 (`skills`): lists + installs bundled SKILL.md files via skillsmith + into `~/.claude/skills/` or `~/.agents/skills/`. + +Neither path shipped a `--agent` flag. Issue #169 fills the gap: take a bundled +Skill and route it to one of 8 agent families with a marker-fenced block so +the install is idempotent across re-runs. The marker-fence guarantees no +duplicate blocks grow on the disk even after dozens of updates. + +## Known limitations + +- The marker fence is line-based; a `` + embedded mid-line inside a code block would mislead `ParseMarkers`. The + host agents we ship to don't emit such lines, but a third-party editor + could. `TestParseMarkers_HalfOpenedFence` documents the safety net. +- `Install` does not validate that `home` exists; it `MkdirAll`s parents as + needed. This is intentional for the canonical use case (the home dir is + always present) but means `sin-code skill uninstall --agent x y` creates + an empty parent directory if the path was never written. +- `SIN_CODE_HOME` env override is the only override; `~/.claude/skills` is + hardcoded relative to it. Test scaffolding passes `t.TempDir()` to + `InstallOptions.Home` to bypass the env var. + +## Change-log + +- **v1.0.0 / 2026-06-16** — initial implementation (issue #169). 8 targets, + 3 formats, 1 marker convention. Race-safe in every code path. diff --git a/cmd/sin-code/internal/skilldist/skilldist.go b/cmd/sin-code/internal/skilldist/skilldist.go new file mode 100644 index 00000000..bd914ed2 --- /dev/null +++ b/cmd/sin-code/internal/skilldist/skilldist.go @@ -0,0 +1,604 @@ +// SPDX-License-Identifier: MIT +// Package skilldist distributes a bundled SIN-Code Skill artifact to one of +// eight supported agent families: Claude Code, Codex, Gemini, opencode, +// Cursor, Windsurf, Cline, and GitHub Copilot. The package is the source of +// truth for the per-agent install path templates and is consumed exclusively +// by `cmd/sin-code/skill_cmd.go` (the `sin-code skill install --agent ` +// surface). +// +// # Marker-fenced idempotency +// +// Every write goes through ParseMarkers / Render so a subsequent install with +// the same `(target, skill)` pair replaces the previously written block in +// place. We never concatenate onto the file: that confuses the host agent and +// produces unbounded growth on re-runs. The marker pair is: +// +// +// … rendered body … +// +// +// The trailing whitespace before `` on the END marker is intentional: +// it visually aligns the markers in `head -n 2 some-rule.md` output. +// ParseMarkers strips that whitespace on lookup so a regex-based scanner +// outside this package still finds both ends. +// +// # Formats +// +// A Target.Format picks the writer: +// +// "dir" — copy the SKILL.md (and optional context/, frameworks/, tasks/, +// templates/) into a per-skill directory. Used by agent families +// that expose a Skills-FS-style drop-in folder. +// "rule" — write a single .md or .mdc rule file whose body is the +// marker-fenced SKILL.md body. Used by Cursor/Windsurf/Cline/Codex +// rule directories. +// "marker" — append per-skill marker blocks to a single shared agent +// instructions file (currently only GitHub Copilot's +// `.github/copilot-instructions.md`). One file, many blocks. +// +// ParseMarkers is the only sanctioned writer for formats "rule" and "marker". +// "dir" does not use markers because the writer owns the whole tree. +// +// # Environment overrides +// +// For testability and CI determinism the writer takes a `Home` string instead +// of reading $HOME itself. The CLI layer is responsible for resolving the +// home directory (including $SIN_CODE_HOME overrides). +package skilldist + +import ( + "fmt" + "os" + "path/filepath" + "strings" +) + +// Format kinds — public so tests and the CLI can assert on them. +const ( + FormatDir = "dir" + FormatRule = "rule" + FormatMarker = "marker" +) + +// MarkerPrefix is prepended to the begin/end lines so an editor collapse-fold +// has something stable to grep against. Keep ASCII: a multi-byte rune would +// break the line-based ParseMarkers scan on a non-UTF-8 file. +const MarkerPrefix = "SIN-CODE-SKILL" + +// BeginMarker and EndMarker return the exact fence lines for one skill. +// Render and Parse functions take the result of these as anchors. +func BeginMarker(skill string) string { + return fmt.Sprintf("", MarkerPrefix, skill) +} +func EndMarker(skill string) string { return fmt.Sprintf("", MarkerPrefix, skill) } + +// Target is one supported agent family. +// +// Name — short id used on the CLI: `claude-code`, `cursor`, +// `copilot`, … +// DisplayName — human label used in `--agent ` help and table +// output. +// InstallPath — path template relative to the user's home directory; +// contains a `` placeholder that is replaced at +// write time with the skill name. The placeholder is +// omitted for multi-skill files (e.g. Copilot's +// instructions file). +// Format — one of FormatDir / FormatRule / FormatMarker. +// +// # Stability +// +// (Name, DisplayName) is a public API surface exposed via `sin-code skill +// install --agent `. Adding a target is non-breaking; renaming or +// removing one is a major bump per AGENTS.md §10. +type Target struct { + Name string + DisplayName string + InstallPath string + Format string +} + +// Targets is the single source of truth for supported agent families. Any +// addition here MUST also be reflected in: +// +// cmd/sin-code/skill_cmd.go (cobra's shell completion for `--agent`), +// AGENTS.md §10 (the naming-and-stability matrix), +// CHANGELOG.md [Unreleased] (the additions bullet). +// +// The set is intentionally small (8 entries today). Verify-gated expansion +// is fine, but every new entry adds a maintenance row in three places. +var Targets = map[string]Target{ + "claude-code": { + Name: "claude-code", + DisplayName: "Claude Code", + InstallPath: ".claude/skills/", + Format: FormatDir, + }, + "opencode": { + Name: "opencode", + DisplayName: "opencode", + InstallPath: ".config/opencode/skills/", + Format: FormatDir, + }, + "gemini": { + Name: "gemini", + DisplayName: "Gemini CLI", + InstallPath: ".gemini/skills/", + Format: FormatDir, + }, + "codex": { + Name: "codex", + DisplayName: "Codex CLI", + InstallPath: ".codex/rules/.md", + Format: FormatRule, + }, + "cursor": { + Name: "cursor", + DisplayName: "Cursor", + InstallPath: ".cursor/rules/.mdc", + Format: FormatRule, + }, + "windsurf": { + Name: "windsurf", + DisplayName: "Windsurf", + InstallPath: ".windsurf/rules/.md", + Format: FormatRule, + }, + "cline": { + Name: "cline", + DisplayName: "Cline", + InstallPath: ".clinerules/.md", + Format: FormatRule, + }, + "copilot": { + Name: "copilot", + DisplayName: "GitHub Copilot", + InstallPath: ".github/copilot-instructions.md", + Format: FormatMarker, + }, +} + +// TargetNames returns every registered id in deterministic (alphabetical) +// order. A stable order is required so `--agent all` produces identical log +// output across machines — important for the verify-gate's reproducibility +// check. +func TargetNames() []string { + out := make([]string, 0, len(Targets)) + for k := range Targets { + out = append(out, k) + } + for i := 1; i < len(out); i++ { + for j := i; j > 0 && out[j-1] > out[j]; j-- { + out[j-1], out[j] = out[j], out[j-1] + } + } + return out +} + +// InstallOptions configures one Install call. +// +// SrcRoot — for FormatDir, the on-disk location of the `/` directory +// whose SKILL.md (and optional context/, frameworks/, tasks/, +// templates/) is copied. Ignored for FormatRule / FormatMarker. +// Home — the user home directory used to resolve InstallPath. Empty +// means "fall back to os.UserHomeDir" — the CLI passes the value +// of $SIN_CODE_HOME or the real home. Tests use t.TempDir(). +// Body — for FormatRule / FormatMarker, the raw SKILL.md body to embed +// inside the marker fence. Ignored for FormatDir. +type InstallOptions struct { + SrcRoot string + Home string + Body string +} + +// Install writes a marker-fenced skill to the target's resolved path. It is +// idempotent: a second invocation produces byte-identical output (assuming +// the input body is unchanged). Supports FormatDir / FormatRule / FormatMarker. +// +// Format-specific behaviour: +// +// FormatDir — every file under SrcRoot// is copied into +// / overwriting any existing +// tree. SKILL.md is mandatory; context/frameworks/tasks/ +// templates are copied only when non-empty. +// FormatRule — the file at the resolved path is rewritten so the +// marker-fenced block for `skill` is present. Existing +// blocks for other skills in the file are preserved by +// ParseMarkers-driven replacement. +// FormatMarker — same as FormatRule but the resolved path is a shared +// multi-skill file (one block per skill). +// +// All writes go through atomicWrite so a partial install can never be +// observed by the agent mid-write. Errors carry both the writer view +// (which file failed) and the inner error from the OS so the CLI can +// show users a usable message. +func Install(skill string, tgt Target, opts InstallOptions) error { + if _, ok := Targets[tgt.Name]; !ok { + return fmt.Errorf("skilldist: unknown target %q", tgt.Name) + } + if tgt.Format != FormatDir && tgt.Format != FormatRule && tgt.Format != FormatMarker { + return fmt.Errorf("skilldist: target %q has invalid Format %q", tgt.Name, tgt.Format) + } + resolved, err := Resolve(tgt, skill, opts.Home) + if err != nil { + return err + } + if err := os.MkdirAll(filepath.Dir(resolved), 0o755); err != nil { + return fmt.Errorf("skilldist: mkdir parent for %q: %w", resolved, err) + } + + switch tgt.Format { + case FormatDir: + if opts.SrcRoot == "" { + return fmt.Errorf("skilldist: FormatDir requires SrcRoot for target %q", tgt.Name) + } + return writeSkillDir(opts.SrcRoot, resolved, skill) + case FormatRule: + return writeFenceFile(resolved, skill, opts.Body) + case FormatMarker: + return writeFenceFile(resolved, skill, opts.Body) + } + return nil +} + +// Resolve expands the `` placeholder in InstallPath against `home`. +// +// If home is empty, Resolve falls back to `os.UserHomeDir()`; the CLI passes +// an empty string to opt into the default, tests pass t.TempDir(). +func Resolve(tgt Target, skill, home string) (string, error) { + if tgt.InstallPath == "" { + return "", fmt.Errorf("skilldist: target %q has no InstallPath", tgt.Name) + } + if skill == "" { + return "", fmt.Errorf("skilldist: empty skill name for target %q", tgt.Name) + } + if strings.Contains(skill, "..") || strings.ContainsAny(skill, "/\\") { + return "", fmt.Errorf("skilldist: unsafe skill name %q", skill) + } + if strings.Contains(tgt.InstallPath, "") && strings.Count(tgt.InstallPath, "") != 1 { + return "", fmt.Errorf("skilldist: target %q has malformed InstallPath", tgt.Name) + } + resolved := strings.ReplaceAll(tgt.InstallPath, "", skill) + if home == "" { + h, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("skilldist: home directory: %w", err) + } + home = h + } + return filepath.Join(home, resolved), nil +} + +// StripFrontmatter strips the YAML frontmatter (the leading `---` … `---` +// block) of a SKILL.md so the inlined rule body is shorter and devoid of +// fields the host agent doesn't understand. If the file has no frontmatter +// the original body is returned verbatim. +// +// The frontmatter delimiting rule mirrors the YAML 1.2 spec: the file starts +// with `---` on its own line and the frontmatter block terminates at the next +// line whose only content is `---`. Only the leading block is recognised; +// later `---` lines are body content. +// +// Both LF and CRLF line endings are tolerated; the function normalises them +// to LF before scanning and leaves the result in LF. +func StripFrontmatter(raw string) string { + raw = strings.ReplaceAll(raw, "\r\n", "\n") + lines := strings.Split(raw, "\n") + if len(lines) == 0 || strings.TrimSpace(lines[0]) != "---" { + return raw + } + for i := 1; i < len(lines); i++ { + if strings.TrimSpace(lines[i]) == "---" { + out := strings.Join(lines[i+1:], "\n") + return strings.TrimLeft(out, "\n") + } + } + // Unterminated frontmatter — treat as no frontmatter rather than risk + // emitting a half-block. The skill author should fix the file. + return raw +} + +// RenderBlock produces the marker-fenced body that Install writes for a +// single (target, skill) pair. The begin line banners the rule with version +// + skill id so the on-disk file is self-explanatory in `cat` / `less` +// output. +// +// RenderBlock preserves trailing newline semantics: the returned string +// always ends with `\n`. Callers can therefore write `RenderBlock(...)` to a +// file without further newline insertion. +func RenderBlock(skill string, body string) string { + body = strings.TrimRight(body, "\n") + return fmt.Sprintf("%s\n# Skill: %s\n\n%s\n%s\n", + BeginMarker(skill), + skill, + body, + EndMarker(skill), + ) +} + +// ParseResult is the structured outcome of ParseMarkers; consumers usually +// only care about (Block, OK). Prefix and Suffix are the bytes before and +// after the matched block and are returned so the writer can reconstruct +// the file on update. +// +// OK=true — a fenced block for `skill` exists; Block contains the inner +// body without the marker lines. +// OK=false — no fenced block was found; Prefix is the full input and +// Block / Suffix are empty. +type ParseResult struct { + Prefix string + Block string + Suffix string + OK bool +} + +// ParseMarkers scans a file's contents and returns the parsed envelope +// around the marker fence for `skill`. The scan is line-based and tolerant +// of trailing whitespace: the begin line must match BeginMarker(skill) +// verbatim (after CR stripping), and the end line must match EndMarker(skill). +// Any content between the pair is returned verbatim as Block. +// +// If the begin line is found but the end line is missing, ParseResult.OK is +// false and Prefix is the full input — a half-opened fence is treated as if +// the block were absent, so the writer writes a clean block rather than +// producing a malformed file. +func ParseMarkers(content, skill string) ParseResult { + begin := BeginMarker(skill) + end := EndMarker(skill) + + content = strings.ReplaceAll(content, "\r\n", "\n") + lines := strings.Split(content, "\n") + + beginIdx := -1 + for i, ln := range lines { + if strings.TrimRight(ln, " \t") == begin { + beginIdx = i + break + } + } + if beginIdx < 0 { + return ParseResult{Prefix: content} + } + endIdx := -1 + for i := beginIdx + 1; i < len(lines); i++ { + if strings.TrimRight(lines[i], " \t") == end { + endIdx = i + break + } + } + if endIdx < 0 { + // Half-opened fence: behave as if absent so callers write a clean block + // rather than appending onto the dangling begin line. + return ParseResult{Prefix: content} + } + + prefix := strings.Join(lines[:beginIdx], "\n") + prefix = strings.TrimRight(prefix, "\n") + "\n" + if prefix == "\n" { + prefix = "" + } + + block := strings.Join(lines[beginIdx+1:endIdx], "\n") + suffix := strings.Join(lines[endIdx+1:], "\n") + if beginIdx == 0 { + // Block was the first content; drop the orphan newline we just + // inserted into Prefix so the file doesn't start with a blank line. + prefix = "" + } + + return ParseResult{Prefix: prefix, Block: block, Suffix: suffix, OK: true} +} + +// writeSkillDir copies //SKILL.md (and the four optional +// supplementary directories if present) to . Existing files at +// the destination are overwritten — the writer always reports success on +// re-install because the same SKILL.md's identity (path + content) is +// idempotent. +func writeSkillDir(srcRoot, resolved, skill string) error { + if err := os.MkdirAll(resolved, 0o755); err != nil { + return fmt.Errorf("skilldist: mkdir %q: %w", resolved, err) + } + src := filepath.Join(srcRoot, skill, "SKILL.md") + in, err := os.ReadFile(src) + if err != nil { + return fmt.Errorf("skilldist: read %q: %w", src, err) + } + if err := os.WriteFile(filepath.Join(resolved, "SKILL.md"), in, 0o644); err != nil { + return fmt.Errorf("skilldist: write SKILL.md to %q: %w", resolved, err) + } + for _, sub := range []string{"context", "frameworks", "tasks", "templates"} { + srcSub := filepath.Join(srcRoot, skill, sub) + if entries, err := os.ReadDir(srcSub); err == nil && len(entries) > 0 { + dstSub := filepath.Join(resolved, sub) + if err := copyTree(srcSub, dstSub); err != nil { + return fmt.Errorf("skilldist: copy %q: %w", sub, err) + } + } + } + return nil +} + +// copyTree recursively copies src/ to dst/. Stops at the +// first error and returns it. +func copyTree(src, dst string) error { + if err := os.MkdirAll(dst, 0o755); err != nil { + return err + } + entries, err := os.ReadDir(src) + if err != nil { + return err + } + for _, e := range entries { + s := filepath.Join(src, e.Name()) + d := filepath.Join(dst, e.Name()) + if e.IsDir() { + if err := copyTree(s, d); err != nil { + return err + } + } + data, err := os.ReadFile(s) + if err != nil { + return err + } + if err := os.WriteFile(d, data, 0o644); err != nil { + return err + } + } + return nil +} + +// writeFenceFile writes a marker-fenced block to the resolved path; an +// existing block for the same skill is replaced in place via ParseMarkers. +// The result has exactly one trailing newline. +// +// FormatRule and FormatMarker converge on the same writer: the only +// difference is at the agent-config layer (a single .mdc per Cursor rule +// vs the shared copilot-instructions.md), not at the marker-fence layer. +func writeFenceFile(resolved, skill, body string) error { + if body == "" { + return fmt.Errorf("skilldist: empty body for skill %q", skill) + } + rendered := RenderBlock(skill, body) + existing := "" + if data, err := os.ReadFile(resolved); err == nil { + existing = string(data) + parsed := ParseMarkers(existing, skill) + if parsed.OK { + full := parsed.Prefix + rendered + parsed.Suffix + if !strings.HasSuffix(full, "\n") { + full += "\n" + } + return atomicWrite(resolved, []byte(full)) + } + } + if existing != "" && !strings.HasSuffix(existing, "\n") { + existing += "\n" + } + full := existing + rendered + if !strings.HasSuffix(full, "\n") { + full += "\n" + } + return atomicWrite(resolved, []byte(full)) +} + +// Uninstall reverses Install: +// +// FormatDir — remove the resolved directory tree (already-absent is OK). +// FormatRule — remove the entire rule file. Every FormatRule file holds +// exactly one skill, so a stale file is unrecoverable; we +// prefer to delete it rather than leave dead content for the +// host agent to ingest. +// FormatMarker — remove only the marker block; preserve every other block +// in the shared file. If the shared file becomes empty, +// delete it entirely. +// +// Returns nil when the install was already absent (idempotent on retry). +func Uninstall(tgt Target, skill, home string) error { + if _, ok := Targets[tgt.Name]; !ok { + return fmt.Errorf("skilldist: unknown target %q", tgt.Name) + } + resolved, err := Resolve(tgt, skill, home) + if err != nil { + return err + } + switch tgt.Format { + case FormatDir: + if _, err := os.Stat(resolved); os.IsNotExist(err) { + return nil + } + return os.RemoveAll(resolved) + case FormatRule: + if _, err := os.Stat(resolved); os.IsNotExist(err) { + return nil + } + return os.Remove(resolved) + case FormatMarker: + data, err := os.ReadFile(resolved) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } + parsed := ParseMarkers(string(data), skill) + if !parsed.OK { + return nil + } + full := parsed.Prefix + parsed.Suffix + full = strings.TrimRight(full, "\n") + "\n" + if strings.TrimSpace(full) == "" { + // Nothing left after the block: remove the file entirely so + // the agent's instructions file is clean. + return os.Remove(resolved) + } + return atomicWrite(resolved, []byte(full)) + } + return nil +} + +// IsInstalled reports whether a (target, skill) install is currently present +// on disk. It is the cheapest check the CLI uses for `--installed` listing. +// +// The check is Format-aware: +// +// FormatDir — directory exists. +// FormatRule — file exists AND contains a marker fence for `skill`. +// FormatMarker — file exists AND contains a marker fence for `skill`. +// +// Formats that lack a marker fence (FormatRule outside the fenced body) do +// not report installed even when the file exists — this protects against +// hand-edited rule files confusing the status table. +func IsInstalled(tgt Target, skill, home string) (bool, error) { + resolved, err := Resolve(tgt, skill, home) + if err != nil { + return false, err + } + switch tgt.Format { + case FormatDir: + st, err := os.Stat(resolved) + if os.IsNotExist(err) { + return false, nil + } + if err != nil { + return false, err + } + return st.IsDir(), nil + case FormatRule, FormatMarker: + data, err := os.ReadFile(resolved) + if os.IsNotExist(err) { + return false, nil + } + if err != nil { + return false, err + } + return ParseMarkers(string(data), skill).OK, nil + } + return false, fmt.Errorf("skilldist: unsupported Format %q", tgt.Format) +} + +// atomicWrite performs a temp-file-and-rename write so a half-finished +// install can never be observed by the agent (which re-reads the file on +// every prompt) mid-write. The temp file is cleaned up best-effort on +// rename failure. +func atomicWrite(path string, data []byte) error { + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, ".sin-skilldist-*.tmp") + if err != nil { + return err + } + tmpName := tmp.Name() + defer func() { + _ = os.Remove(tmpName) + }() + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return err + } + if err := tmp.Sync(); err != nil { + _ = tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + return os.Rename(tmpName, path) +} diff --git a/cmd/sin-code/internal/skilldist/skilldist_test.go b/cmd/sin-code/internal/skilldist/skilldist_test.go new file mode 100644 index 00000000..65e7774e --- /dev/null +++ b/cmd/sin-code/internal/skilldist/skilldist_test.go @@ -0,0 +1,491 @@ +// SPDX-License-Identifier: MIT +package skilldist + +import ( + "os" + "path/filepath" + "reflect" + "sort" + "strings" + "testing" +) + +// allTargets is a fixture used by every test that wants to walk the full +// registry: list tests must iterate deterministically (alphabetical), and +// idempotency tests must exercise each Format at least once. +func allTargets() []Target { + out := make([]Target, 0, len(Targets)) + for _, t := range Targets { + out = append(out, t) + } + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + return out +} + +// TestTargets_AllPresent locks the registered set of agent ids. Adding or +// removing a target without a corresponding AGENTS.md / CHANGELOG / CLI +// update should be impossible by accident — this test fails loudly so the +// maintainer is forced through the policy update first. +func TestTargets_AllPresent(t *testing.T) { + want := []string{ + "claude-code", "cline", "codex", "copilot", + "cursor", "gemini", "opencode", "windsurf", + } + got := TargetNames() + if !reflect.DeepEqual(got, want) { + t.Fatalf("TargetNames drifted:\n got: %v\n want: %v", got, want) + } +} + +// TestTargets_FieldsValidates ensures every registered target has the +// minimum required fields filled in. A bug-fix that adds a target but +// forgets DisplayName breaks the `--agent ` help text. +func TestTargets_FieldsValidates(t *testing.T) { + for _, tgt := range allTargets() { + if tgt.Name == "" { + t.Errorf("%+v missing Name", tgt) + } + if tgt.DisplayName == "" { + t.Errorf("%s missing DisplayName", tgt.Name) + } + if tgt.InstallPath == "" { + t.Errorf("%s missing InstallPath", tgt.Name) + } + if tgt.Format != FormatDir && tgt.Format != FormatRule && tgt.Format != FormatMarker { + t.Errorf("%s invalid Format %q", tgt.Name, tgt.Format) + } + } +} + +// TestResolve_HomeOverride verifies the home-directory override path: tests +// never want $HOME leakage, so a TempDir must win over os.UserHomeDir() in +// every case. +func TestResolve_HomeOverride(t *testing.T) { + home := t.TempDir() + for _, tgt := range allTargets() { + got, err := Resolve(tgt, "demo-skill", home) + if err != nil { + t.Fatalf("%s: Resolve: %v", tgt.Name, err) + } + rel, err := filepath.Rel(home, got) + if err != nil { + t.Fatalf("%s: relative path: %v", tgt.Name, err) + } + // Multi-skill files (no `` placeholder) yield the bare path; + // per-skill paths substitute the placeholder verbatim. + if !strings.Contains(tgt.InstallPath, "") { + if rel != filepath.Clean(tgt.InstallPath) { + t.Errorf("%s: expected %q, got %q", tgt.Name, tgt.InstallPath, rel) + } + } else if rel != filepath.Clean(strings.ReplaceAll(tgt.InstallPath, "", "demo-skill")) { + t.Errorf("%s: substituted path wrong: %q", tgt.Name, rel) + } + } +} + +// TestResolve_RejectsUnsafeName defends against a malicious or buggy skill +// name escaping the home directory via .. +func TestResolve_RejectsUnsafeName(t *testing.T) { + for _, name := range []string{"", "../etc/passwd", "foo/bar", "foo\\bar"} { + _, err := Resolve(Targets["claude-code"], name, t.TempDir()) + if err == nil { + t.Errorf("Resolve(%q) returned no error", name) + } + } +} + +// TestParseMarkers_RoundTrip makes ParseMarkers the inverse of RenderBlock +// for the simple one-block case: the body returned in the ParseResult must +// equal the body originally passed to RenderBlock (modulo the begin/end +// header banner RenderBlock injects). The markers themselves live inside +// Block's enclosing file content (not in Prefix/Suffix), so we round-trip +// the body by handing the parsed Prefix + Block back to a synthetic "render +// without markers" check. +func TestParseMarkers_RoundTrip(t *testing.T) { + body := "Hello, world.\n\n## Section\n\nSome text." + rendered := RenderBlock("demo", body) + parsed := ParseMarkers(rendered, "demo") + if !parsed.OK { + t.Fatalf("ParseMarkers did not find its own rendered block") + } + if !strings.Contains(parsed.Block, "Hello, world.") { + t.Errorf("parsed block is missing body content: %q", parsed.Block) + } + if !strings.Contains(parsed.Block, "## Section") { + t.Errorf("parsed block is missing trailing content: %q", parsed.Block) + } + // The markers explicitly must NOT appear in either Prefix or Suffix — + // ParseMarkers strips them out so the writer can construct a clean + // replacement. + if strings.Contains(parsed.Prefix, MarkerPrefix) { + t.Errorf("Prefix still contains marker prefix: %q", parsed.Prefix) + } + if strings.Contains(parsed.Suffix, MarkerPrefix) { + t.Errorf("Suffix still contains marker prefix: %q", parsed.Suffix) + } +} + +// TestParseMarkers_DistinctSkills ensures parsing for skill A does NOT match +// a block written for skill B. This is the safety property that prevents a +// re-install of skill A from accidentally clobbering skill B's block. +func TestParseMarkers_DistinctSkills(t *testing.T) { + body := "skill-A specific body" + a := RenderBlock("alpha", body) + if ParseMarkers(a, "beta").OK { + t.Errorf("ParseMarkers for beta matched alpha's block") + } + if ParseMarkers(a, "alpha").Block != "# Skill: alpha\n\nskill-A specific body" { + t.Errorf("alpha parse did not return the right body: %q", ParseMarkers(a, "alpha").Block) + } +} + +// TestParseMarkers_NoBlock asserts the absent case — finding nothing +// returns OK=false and the unmodified input in Prefix. +func TestParseMarkers_NoBlock(t *testing.T) { + in := "no markers here\nnope\n" + p := ParseMarkers(in, "nonesuch") + if p.OK { + t.Errorf("ParseMarkers returned OK=true for an absent block") + } + if p.Prefix != in { + t.Errorf("Prefix should be unchanged when no block is found: got %q", p.Prefix) + } +} + +// TestParseMarkers_HalfOpenedFence covers the safety property: a begin +// marker without a matching end marker is treated as absent so callers +// safely overwrite the dangling begin rather than appending below it. +func TestParseMarkers_HalfOpenedFence(t *testing.T) { + halfOpened := BeginMarker("broken") + "\nsome body without a real end\n" + if ParseMarkers(halfOpened, "broken").OK { + t.Errorf("ParseMarkers accepted a half-opened fence") + } +} + +// TestParseMarkers_PreservesOtherBlocks is the load-bearing test for the +// FormatMarker case (Copilot's instructions file). Installing skill B into +// a file that ALREADY contains skill A's block must keep A's block intact +// and add B's block alongside it. +func TestParseMarkers_PreservesOtherBlocks(t *testing.T) { + mk := func(skill, body string) string { return RenderBlock(skill, body) } + file := mk("alpha", "alpha content\n") + mk("beta", "beta content\n") + parsed := ParseMarkers(file, "alpha") + if !parsed.OK { + t.Fatalf("alpha block should be present") + } + // The suffix must still contain beta's complete marker fence. + if !strings.Contains(parsed.Suffix, BeginMarker("beta")) { + t.Errorf("beta begin marker lost from suffix: %q", parsed.Suffix) + } + if !strings.Contains(parsed.Suffix, EndMarker("beta")) { + t.Errorf("beta end marker lost from suffix: %q", parsed.Suffix) + } + if !strings.Contains(parsed.Suffix, "beta content") { + t.Errorf("beta body lost from suffix: %q", parsed.Suffix) + } +} + +// TestParseMarkers_CRLineEndings verifies CRLF input is handled the same as +// LF: a host agent editing on Windows might re-save with CRLF after our +// install and we still need to locate (and replace) our block. +func TestParseMarkers_CRLineEndings(t *testing.T) { + crlf := strings.ReplaceAll(RenderBlock("demo", "body line"), "\n", "\r\n") + if !ParseMarkers(crlf, "demo").OK { + t.Errorf("ParseMarkers failed on CRLF input") + } +} + +// TestRenderBlock_HasTrailingNewline ensures every render ends in \n so the +// atomicWrite consumer can concatenate without juggling separators. +func TestRenderBlock_HasTrailingNewline(t *testing.T) { + if !strings.HasSuffix(RenderBlock("x", "y"), "\n") { + t.Errorf("RenderBlock must always end with a newline") + } + if !strings.HasSuffix(RenderBlock("x", ""), "\n") { + t.Errorf("RenderBlock with empty body must still end with a newline") + } +} + +// TestStripFrontmatter_BasicValid covers the happy path: leading `---` of +// the file, then a closing `---` line, then body content. +func TestStripFrontmatter_BasicValid(t *testing.T) { + in := "---\nname: x\ndescription: y\n---\n# Body\n\ntext\n" + out := StripFrontmatter(in) + if strings.HasPrefix(out, "---") { + t.Errorf("frontmatter not stripped: %q", out) + } + if !strings.HasPrefix(out, "# Body") { + t.Errorf("body should start at # Body, got %q", out) + } +} + +// TestStripFrontmatter_NoFrontmatter passes content through verbatim. +func TestStripFrontmatter_NoFrontmatter(t *testing.T) { + in := "# Just a heading\n\nno front matter here\n" + if StripFrontmatter(in) != in { + t.Errorf("no-frontmatter content was modified") + } +} + +// TestStripFrontmatter_Unterminated preserves content when `---` never +// closes — emitting a half-block would mislead the host agent. +func TestStripFrontmatter_Unterminated(t *testing.T) { + in := "---\nname: x\n# Body\n" + if StripFrontmatter(in) != in { + t.Errorf("unterminated frontmatter should pass through verbatim") + } +} + +// installFixture builds a fake source skill in srcRoot: SKILL.md plus +// optional contexts to exercise FormatDir's supplementary directory copy. +// Returns the skill name. +func installFixture(t *testing.T, srcRoot, skill, body string) { + t.Helper() + skillDir := filepath.Join(srcRoot, skill) + if err := os.MkdirAll(filepath.Join(skillDir, "context"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "context", "readme.md"), []byte("# context readme\n"), 0o644); err != nil { + t.Fatal(err) + } +} + +// TestInstall_DirFormat_Idempotent covers the FormatDir case: re-running +// Install must overwrite the destination tree without leaving stale files +// from previous runs, and isInstall must report true after the install. +func TestInstall_DirFormat_Idempotent(t *testing.T) { + for _, tgt := range allTargets() { + if tgt.Format != FormatDir { + continue + } + t.Run(tgt.Name, func(t *testing.T) { + home := t.TempDir() + src := t.TempDir() + body := "---\nname: demo\n---\n# Demo\n\ncontent line\n" + installFixture(t, src, "demo", body) + + if err := Install("demo", tgt, InstallOptions{SrcRoot: src, Home: home}); err != nil { + t.Fatalf("first Install: %v", err) + } + ok, err := IsInstalled(tgt, "demo", home) + if err != nil || !ok { + t.Fatalf("IsInstalled after first install: ok=%v err=%v", ok, err) + } + resolved, _ := Resolve(tgt, "demo", home) + if _, err := os.Stat(filepath.Join(resolved, "SKILL.md")); err != nil { + t.Errorf("SKILL.md missing after install: %v", err) + } + if _, err := os.Stat(filepath.Join(resolved, "context", "readme.md")); err != nil { + t.Errorf("context/readme.md missing after install: %v", err) + } + + // Re-run: idempotent. + if err := Install("demo", tgt, InstallOptions{SrcRoot: src, Home: home}); err != nil { + t.Fatalf("second Install: %v", err) + } + // No duplicate directory created. + entries, err := os.ReadDir(filepath.Dir(resolved)) + if err != nil { + t.Fatalf("readdir parent: %v", err) + } + seenDir := 0 + for _, e := range entries { + if e.IsDir() && e.Name() == "demo" { + seenDir++ + } + } + if seenDir != 1 { + t.Errorf("expected exactly one demo directory, found %d", seenDir) + } + }) + } +} + +// TestInstall_RuleFormat_Idempotent covers the FormatRule case: a single +// .md/.mdc file with a marker fence. The fence must be present after +// install and exactly once after a re-install. +func TestInstall_RuleFormat_Idempotent(t *testing.T) { + for _, tgt := range allTargets() { + if tgt.Format != FormatRule { + continue + } + t.Run(tgt.Name, func(t *testing.T) { + home := t.TempDir() + body := StripFrontmatter("# Body\n\nhello\n") + if err := Install("demo", tgt, InstallOptions{Home: home, Body: body}); err != nil { + t.Fatalf("first Install: %v", err) + } + resolved, _ := Resolve(tgt, "demo", home) + data, err := os.ReadFile(resolved) + if err != nil { + t.Fatal(err) + } + if strings.Count(string(data), BeginMarker("demo")) != 1 { + t.Errorf("expected exactly one begin marker, got file:\n%s", string(data)) + } + + // Re-install with new body — must REPLACE, not append. + newBody := StripFrontmatter("# Body v2\n\nchanged\n") + if err := Install("demo", tgt, InstallOptions{Home: home, Body: newBody}); err != nil { + t.Fatalf("second Install: %v", err) + } + data2, err := os.ReadFile(resolved) + if err != nil { + t.Fatal(err) + } + if strings.Count(string(data2), BeginMarker("demo")) != 1 { + t.Errorf("second install created duplicate marker fence:\n%s", string(data2)) + } + if !strings.Contains(string(data2), "Body v2") { + t.Errorf("second install did not replace body:\n%s", string(data2)) + } + if strings.Contains(string(data2), "Body\n\nhello") { + t.Errorf("old body remains after second install:\n%s", string(data2)) + } + }) + } +} + +// TestInstall_MarkerFormat_PreservesOtherBlocks covers the FormatMarker +// case (Copilot): two markers side-by-side, second install must not +// destroy the first. +func TestInstall_MarkerFormat_PreservesOtherBlocks(t *testing.T) { + tgt := Targets["copilot"] + home := t.TempDir() + if err := Install("alpha", tgt, InstallOptions{Home: home, Body: "alpha body"}); err != nil { + t.Fatalf("install alpha: %v", err) + } + if err := Install("beta", tgt, InstallOptions{Home: home, Body: "beta body"}); err != nil { + t.Fatalf("install beta: %v", err) + } + resolved, _ := Resolve(tgt, "alpha", home) + data, err := os.ReadFile(resolved) + if err != nil { + t.Fatal(err) + } + if strings.Count(string(data), BeginMarker("alpha")) != 1 { + t.Errorf("alpha block missing or duplicated:\n%s", string(data)) + } + if strings.Count(string(data), BeginMarker("beta")) != 1 { + t.Errorf("beta block missing or duplicated:\n%s", string(data)) + } + if !strings.Contains(string(data), "alpha body") { + t.Errorf("alpha body lost:\n%s", string(data)) + } + if !strings.Contains(string(data), "beta body") { + t.Errorf("beta body lost:\n%s", string(data)) + } +} + +// TestUninstall_RoundTrip ensures Uninstall reverses Install for every +// Format on every registered target. After uninstall, IsInstalled must be +// false and the file/directory must be gone (or empty for multi-skill +// shared files). +func TestUninstall_RoundTrip(t *testing.T) { + for _, tgt := range allTargets() { + t.Run(tgt.Name, func(t *testing.T) { + home := t.TempDir() + body := StripFrontmatter("# body\n") + opts := InstallOptions{Home: home, Body: body} + if tgt.Format == FormatDir { + src := t.TempDir() + installFixture(t, src, "demo", + "---\nname: demo\n---\n# demo\n") + opts.SrcRoot = src + } + if err := Install("demo", tgt, opts); err != nil { + t.Fatalf("Install: %v", err) + } + ok, err := IsInstalled(tgt, "demo", home) + if err != nil || !ok { + t.Fatalf("IsInstalled pre-uninstall: ok=%v err=%v", ok, err) + } + if err := Uninstall(tgt, "demo", home); err != nil { + t.Fatalf("Uninstall: %v", err) + } + ok, err = IsInstalled(tgt, "demo", home) + if err != nil || ok { + t.Fatalf("IsInstalled post-uninstall: ok=%v err=%v", ok, err) + } + // Calling Uninstall twice must be a no-op, not an error. + if err := Uninstall(tgt, "demo", home); err != nil { + t.Errorf("repeated Uninstall returned error: %v", err) + } + }) + } +} + +// TestUninstall_MarkerFormat_RemovesEmptyFile ensures the multi-skill file +// is deleted when the uninstall of its only known block would leave an +// empty file behind (otherwise an unreferenced file would quietly stay +// under Copilot's dotfile directory forever). +func TestUninstall_MarkerFormat_RemovesEmptyFile(t *testing.T) { + tgt := Targets["copilot"] + home := t.TempDir() + if err := Install("solo", tgt, InstallOptions{Home: home, Body: "solo"}); err != nil { + t.Fatal(err) + } + resolved, _ := Resolve(tgt, "solo", home) + if err := Uninstall(tgt, "solo", home); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(resolved); !os.IsNotExist(err) { + t.Errorf("expected file removed after final uninstall, got err=%v", err) + } +} + +// TestInstall_RejectsUnknownTarget locks the public API: passing a target +// not in Targets is an error, not a crash. +func TestInstall_RejectsUnknownTarget(t *testing.T) { + err := Install("demo", Target{Name: "made-up-agent", Format: FormatRule}, + InstallOptions{Home: t.TempDir(), Body: "x"}) + if err == nil { + t.Fatalf("expected error for unknown target") + } +} + +// TestInstall_RuleBodyEmpty is a guard against silently writing an empty +// rule file: the host agent would load it and look foolish. +func TestInstall_RuleBodyEmpty(t *testing.T) { + err := Install("demo", Targets["cursor"], InstallOptions{Home: t.TempDir(), Body: ""}) + if err == nil { + t.Fatalf("expected error for empty body in FormatRule") + } +} + +// TestInstall_AtomicOrder verifies the install never leaves a half-written +// file behind that would confuse downstream readers. We simulate a failure +// by passing a path whose parent is a regular file (mkdir parent fails). +func TestInstall_AtomicOrder_DirParentFailure(t *testing.T) { + home := t.TempDir() + // Create a regular file at a position Install wants to mkdir as parent. + blocker := filepath.Join(home, ".cursor") + if err := os.WriteFile(blocker, []byte("not a directory"), 0o644); err != nil { + t.Fatal(err) + } + tgt := Targets["cursor"] + err := Install("demo", tgt, InstallOptions{Home: home, Body: "hello"}) + if err == nil { + t.Fatalf("expected mkdir failure when parent is a file") + } + if _, err := os.Stat(blocker); err != nil { + t.Errorf("blocker file should still exist, got %v", err) + } +} + +// TestIsInstalled_Defaults exercises the easy cases that the `--installed` +// CLI flag relies on: file absent → false, dir absent → false. +func TestIsInstalled_Defaults(t *testing.T) { + for _, tgt := range allTargets() { + ok, err := IsInstalled(tgt, "ghost-skill", t.TempDir()) + if err != nil { + t.Errorf("%s: unexpected error: %v", tgt.Name, err) + } + if ok { + t.Errorf("%s: ghost-skill reported as installed", tgt.Name) + } + } +} diff --git a/cmd/sin-code/skill_cmd.go b/cmd/sin-code/skill_cmd.go index 6dcb6a0c..ec640ef8 100644 --- a/cmd/sin-code/skill_cmd.go +++ b/cmd/sin-code/skill_cmd.go @@ -1,28 +1,179 @@ // SPDX-License-Identifier: MIT -// Purpose: `sin-code skill` — manage ecosystem skill installations. +// Purpose: `sin-code skill` — manage ecosystem skill installations AND +// distribute bundled Skill artifacts to external agent families. +// +// Two surfaces share this command tree on purpose: +// +// - Ecosystem (skillmgr, v3.5.0): clones upstream skill repos such as +// `SIN-Code-Websearch` and verifies their MCP entrypoints. Triggered by +// `sin-code skill install ` or `sin-code skill install all`. +// +// - Distribution (skilldist, issue #169): takes a bundled SKILL.md (one of +// the 34 embedded skills under skills/-skills//) and writes +// it into one of eight supported agent families (Claude Code, Codex, +// Gemini, opencode, Cursor, Windsurf, Cline, GitHub Copilot) using a +// marker-fenced block so a re-run replaces the block in place. Triggered +// by `sin-code skill install --agent ` or `--agent all`. +// +// The two surfaces are mutually exclusive: passing `--agent` switches into +// distribution mode; without `--agent` the legacy ecosystem behaviour is +// preserved 1:1 (including the `all` positional shortcut). package main import ( "encoding/json" "fmt" + "io/fs" "os" + "path/filepath" "sort" + "strings" "github.com/spf13/cobra" + "github.com/OpenSIN-Code/SIN-Code/cmd/sin-code/internal/skilldist" "github.com/OpenSIN-Code/SIN-Code/cmd/sin-code/internal/skillmgr" + "github.com/OpenSIN-Code/SIN-Code/skills" ) +// agentFlagAll is the magic value for `--agent` that means "try every +// registered target". We keep it as a string constant rather than a bool +// so future flags (`--agent ,`) can extend without a +// breaking change. +const agentFlagAll = "all" + +// reservedAgentNames mirrors skilldist.TargetNames() with the addition of +// `all`. Keeping this here lets cobra's `--agent` completion help text +// show the full choice set without needing to import skilldist in two +// places. +func reservedAgentNames() []string { + out := []string{agentFlagAll} + out = append(out, skilldist.TargetNames()...) + return out +} + +// bundledSkillFS is overridden by tests so they can point at a TempDir. +// +// We do NOT cache the FS at package level because the underlying fs.FS +// is immutable once constructed; the override pattern keeps test +// isolation trivial. +var bundledSkillFS = func() (fs.FS, error) { return skills.ListFS() } + +// resolveHome picks the home directory for install paths. Order of +// precedence: $SIN_CODE_HOME > $HOME > os.UserHomeDir(). +func resolveHome() (string, error) { + if v := os.Getenv("SIN_CODE_HOME"); v != "" { + return v, nil + } + if v := os.Getenv("HOME"); v != "" { + return v, nil + } + return os.UserHomeDir() +} + +// extractSkillFromFS reads a single SKILL.md (and optional sidecar files) +// from the bundled skills FS into a real on-disk directory under dstRoot. +// skilldist.FormatDir installs from a `SrcRoot`, so a real path is required +// even though skillsmith would happily read from fs.FS. +func extractSkillFromFS(src fs.FS, dstRoot, skill string) error { + out := filepath.Join(dstRoot, skill) + if err := os.MkdirAll(out, 0o755); err != nil { + return err + } + skillMD, err := fs.ReadFile(src, filepath.Join(skill, "SKILL.md")) + if err != nil { + return fmt.Errorf("extractSkillFromFS(%q): read SKILL.md: %w", skill, err) + } + if err := os.WriteFile(filepath.Join(out, "SKILL.md"), skillMD, 0o644); err != nil { + return err + } + for _, sub := range []string{"context", "frameworks", "tasks", "templates"} { + entries, err := fs.ReadDir(src, filepath.Join(skill, sub)) + if err != nil { + continue + } + for _, e := range entries { + data, err := fs.ReadFile(src, filepath.Join(skill, sub, e.Name())) + if err != nil { + return err + } + subDir := filepath.Join(out, sub) + if err := os.MkdirAll(subDir, 0o755); err != nil { + return err + } + if err := os.WriteFile(filepath.Join(subDir, e.Name()), data, 0o644); err != nil { + return err + } + } + } + return nil +} + +// readSkillBody returns the SKILL.md body ready for marker-fence embedding. +// The YAML frontmatter is stripped and the body is LF-normalised so the +// host agent sees a portable representation. +func readSkillBody(src fs.FS, skill string) (string, error) { + raw, err := fs.ReadFile(src, filepath.Join(skill, "SKILL.md")) + if err != nil { + return "", fmt.Errorf("readSkillBody(%q): %w", skill, err) + } + body := skilldist.StripFrontmatter(string(raw)) + if strings.TrimSpace(body) == "" { + return "", fmt.Errorf("readSkillBody(%q): empty body after stripping frontmatter", skill) + } + return body, nil +} + +// resolveAgentFlag walks the agent flag through the precedence rules and +// returns either the literal `all` or a Target from skilldist.Targets. +// Any other value (including the empty string) is rejected with a +// cobra-friendly error message. +func resolveAgentFlag(agentFlag string) ([]string, error) { + if agentFlag == "" || agentFlag == agentFlagAll { + return skilldist.TargetNames(), nil + } + if _, ok := skilldist.Targets[agentFlag]; !ok { + return nil, fmt.Errorf("unknown agent %q (supported: %s, %s)", + agentFlag, agentFlagAll, strings.Join(skilldist.TargetNames(), ", ")) + } + return []string{agentFlag}, nil +} + +// NewSkillCmd builds the `sin-code skill` command tree. +// +// Two helper commands shadow each other but coexist: +// +// `sin-code skill status` ecosystem-installer state (legacy). +// `sin-code skill list [--agent X]` the new --installed matrix view. +// +// Both are useful: status talks to the upstream repos, list talks to +// the on-disk install of bundled skills. They use different storage and +// must not be merged. func NewSkillCmd() *cobra.Command { cmd := &cobra.Command{ Use: "skill", Short: "Install and manage ecosystem MCP skills", + Long: `The skill subcommand has two surfaces. + +Ecosystem install (default, v3.5.0): + sin-code skill install ... | all + sin-code skill status + +Bundle distribution (issue #169, --agent flag): + sin-code skill install --agent + sin-code skill install --agent all + sin-code skill uninstall --agent + sin-code skill list [--installed] [--agent ] + +Distribution uses marker-fenced installs so a re-run is idempotent: the +block between and + is replaced in place.`, } var jsonOut bool statusCmd := &cobra.Command{ Use: "status", - Short: "Show install + runnable state of all known skills", + Short: "Show install + runnable state of all known ecosystem skills", RunE: func(cmd *cobra.Command, args []string) error { sts := skillmgr.Status(cmd.Context()) sort.Slice(sts, func(i, j int) bool { return sts[i].Name < sts[j].Name }) @@ -31,45 +182,291 @@ func NewSkillCmd() *cobra.Command { enc.SetIndent("", " ") return enc.Encode(sts) } - fmt.Printf("%-15s %-10s %-9s %s\n", "SKILL", "INSTALLED", "RUNNABLE", "DETAIL") + fmt.Fprintf(cmd.OutOrStdout(), "%-15s %-10s %-9s %s\n", "SKILL", "INSTALLED", "RUNNABLE", "DETAIL") for _, s := range sts { - fmt.Printf("%-15s %-10v %-9v %s\n", s.Name, s.Installed, s.Runnable, s.Detail) + fmt.Fprintf(cmd.OutOrStdout(), "%-15s %-10v %-9v %s\n", s.Name, s.Installed, s.Runnable, s.Detail) } return nil }, } statusCmd.Flags().BoolVar(&jsonOut, "json", false, "emit JSON") + var agentFlag string installCmd := &cobra.Command{ Use: "install ... | all", - Short: "Clone/update skill repos and verify their MCP entrypoints", + Short: "Clone/update skill repos OR --agent distribute a bundled skill", Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - names := args - if len(args) == 1 && args[0] == "all" { - names = names[:0] - for n := range skillmgr.KnownSkills() { - names = append(names, n) + // Distribution mode: --agent was set. + if agentFlag != "" || os.Getenv("SIN_CODE_AGENT") != "" { + if agentFlag == "" { + agentFlag = os.Getenv("SIN_CODE_AGENT") } - sort.Strings(names) + return runSkillInstallDistribute(cmd, args, agentFlag) } - failed := 0 - for _, n := range names { - st, err := skillmgr.Install(cmd.Context(), n) - if err != nil { - fmt.Fprintf(os.Stderr, "FAIL %s: %v\n", n, err) - failed++ - continue - } - fmt.Printf("OK %s (runnable=%v, %s)\n", st.Name, st.Runnable, st.Detail) + // Legacy ecosystem install mode. + return runSkillInstallEcosystem(cmd, args) + }, + } + installCmd.Flags().StringVar(&agentFlag, "agent", "", + "target agent (claude-code|codex|gemini|opencode|cursor|windsurf|cline|copilot|all); "+ + "or $SIN_CODE_AGENT. Empty = ecosystem install mode.") + + // list shows the install status of bundled skills against each + // registered agent family. + var installedOnly bool + listCmd := &cobra.Command{ + Use: "list [--installed] [--agent ]", + Short: "List bundled skills and their distribution status per agent", + Long: `Without flags, lists every bundled skill and reports which +agent families currently have it installed. --installed filters out +bundled skills with zero installs across all targets. --agent +filters the report to one target (use "all" for the unfiltered view).`, + RunE: func(cmd *cobra.Command, args []string) error { + return runSkillList(cmd, agentFlag, installedOnly, jsonOut) + }, + } + listCmd.Flags().BoolVar(&jsonOut, "json", false, "emit JSON") + listCmd.Flags().BoolVar(&installedOnly, "installed", false, + "only show bundled skills with at least one installed copy") + listCmd.Flags().StringVar(&agentFlag, "agent", "", + "target agent (default: all)") + + uninstallCmd := &cobra.Command{ + Use: "uninstall --agent ", + Short: "Remove a bundled skill from a target agent family", + Args: cobra.MinimumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if agentFlag == "" { + agentFlag = os.Getenv("SIN_CODE_AGENT") } - if failed > 0 { - return fmt.Errorf("%d skill(s) failed to install", failed) + if agentFlag == "" { + return fmt.Errorf("--agent is required for uninstall (also accepts $SIN_CODE_AGENT)") } - return nil + return runSkillUninstall(cmd, args, agentFlag) }, } + uninstallCmd.Flags().StringVar(&agentFlag, "agent", "", + "target agent (required)") - cmd.AddCommand(statusCmd, installCmd) + cmd.AddCommand(statusCmd, installCmd, listCmd, uninstallCmd) return cmd } + +// runSkillInstallEcosystem is the pre-existing v3.5.0 behaviour: clone +// (or pull) an upstream skill repo and verify its MCP entrypoint. +// +// Kept as a helper so the parent install command stays readable. +func runSkillInstallEcosystem(cmd *cobra.Command, args []string) error { + names := args + if len(args) == 1 && args[0] == "all" { + names = names[:0] + for n := range skillmgr.KnownSkills() { + names = append(names, n) + } + sort.Strings(names) + } + failed := 0 + for _, n := range names { + st, err := skillmgr.Install(cmd.Context(), n) + if err != nil { + fmt.Fprintf(os.Stderr, "FAIL %s: %v\n", n, err) + failed++ + continue + } + fmt.Fprintf(cmd.OutOrStdout(), "OK %s (runnable=%v, %s)\n", st.Name, st.Runnable, st.Detail) + } + if failed > 0 { + return fmt.Errorf("%d skill(s) failed to install", failed) + } + return nil +} + +// runSkillInstallDistribute writes each bundled skill name into the +// resolved set of agent families via skilldist.Install. +// +// The body is sourced from the embedded skills.SkillsFS via a one-shot +// extraction to t.TempDir()-style scratch. Re-running is idempotent — the +// marker-fence replacement guarantees no duplicated blocks. +func runSkillInstallDistribute(cmd *cobra.Command, args []string, agentFlag string) error { + targets, err := resolveAgentFlag(agentFlag) + if err != nil { + return err + } + home, err := resolveHome() + if err != nil { + return err + } + src, err := bundledSkillFS() + if err != nil { + return fmt.Errorf("open bundled skills FS: %w", err) + } + scratch, err := os.MkdirTemp("", "sin-code-skilldist-") + if err != nil { + return fmt.Errorf("create scratch dir: %w", err) + } + defer func() { _ = os.RemoveAll(scratch) }() + + failed := 0 + for _, skill := range args { + body, err := readSkillBody(src, skill) + if err != nil { + fmt.Fprintf(os.Stderr, "FAIL %s: %v\n", skill, err) + failed++ + continue + } + if err := extractSkillFromFS(src, scratch, skill); err != nil { + fmt.Fprintf(os.Stderr, "FAIL %s: extract: %v\n", skill, err) + failed++ + continue + } + for _, agentName := range targets { + tgt := skilldist.Targets[agentName] + err := skilldist.Install(skill, tgt, skilldist.InstallOptions{ + SrcRoot: scratch, + Home: home, + Body: body, + }) + if err != nil { + fmt.Fprintf(os.Stderr, "FAIL %s → %s: %v\n", skill, tgt.DisplayName, err) + failed++ + continue + } + fmt.Fprintf(cmd.OutOrStdout(), "OK %s → %s\n", skill, tgt.DisplayName) + } + } + if failed > 0 { + return fmt.Errorf("%d install(s) failed", failed) + } + return nil +} + +// runSkillList renders the install matrix. The default view is every +// bundled skill × every agent family; --installed filters out rows with +// zero installs; --agent filters the agent columns to one target. +type listRow struct { + Skill string `json:"skill"` + Targets map[string]bool `json:"targets"` + HasAny bool `json:"has_any"` +} + +func runSkillList(cmd *cobra.Command, agentFlag string, installedOnly, jsonOut bool) error { + home, err := resolveHome() + if err != nil { + return err + } + src, err := bundledSkillFS() + if err != nil { + return fmt.Errorf("open bundled skills FS: %w", err) + } + + targets := skilldist.TargetNames() + if agentFlag != "" && agentFlag != agentFlagAll { + if _, ok := skilldist.Targets[agentFlag]; !ok { + return fmt.Errorf("unknown agent %q (supported: all, %s)", + agentFlag, strings.Join(skilldist.TargetNames(), ", ")) + } + targets = []string{agentFlag} + } + + skillNames := bundledSkillNames(src) + rows := make([]listRow, 0, len(skillNames)) + for _, sk := range skillNames { + row := listRow{Skill: sk, Targets: make(map[string]bool, len(targets))} + for _, ag := range targets { + tgt := skilldist.Targets[ag] + ok, err := skilldist.IsInstalled(tgt, sk, home) + if err != nil { + row.Targets[ag] = false + continue + } + row.Targets[ag] = ok + if ok { + row.HasAny = true + } + } + if installedOnly && !row.HasAny { + continue + } + rows = append(rows, row) + } + + if jsonOut { + enc := json.NewEncoder(cmd.OutOrStdout()) + enc.SetIndent("", " ") + return enc.Encode(rows) + } + + // Pretty table. + header := fmt.Sprintf("%-32s", "SKILL") + for _, ag := range targets { + tgt := skilldist.Targets[ag] + header += fmt.Sprintf(" %-12s", tgt.DisplayName) + } + fmt.Fprintln(cmd.OutOrStdout(), header) + for _, r := range rows { + row := fmt.Sprintf("%-32s", r.Skill) + for _, ag := range targets { + if r.Targets[ag] { + row += " ✓ " + } else { + row += " — " + } + } + fmt.Fprintln(cmd.OutOrStdout(), row) + } + return nil +} + +// runSkillUninstall reverses a previous --agent install. Each (skill, target) +// pair is removed via skilldist.Uninstall. +func runSkillUninstall(cmd *cobra.Command, args []string, agentFlag string) error { + targets, err := resolveAgentFlag(agentFlag) + if err != nil { + return err + } + home, err := resolveHome() + if err != nil { + return err + } + failed := 0 + for _, skill := range args { + for _, agentName := range targets { + tgt := skilldist.Targets[agentName] + if err := skilldist.Uninstall(tgt, skill, home); err != nil { + fmt.Fprintf(os.Stderr, "FAIL %s → %s: %v\n", skill, tgt.DisplayName, err) + failed++ + continue + } + fmt.Fprintf(cmd.OutOrStdout(), "OK %s ← %s removed\n", skill, tgt.DisplayName) + } + } + if failed > 0 { + return fmt.Errorf("%d uninstall(s) failed", failed) + } + return nil +} + +// bundledSkillNames walks the flattened skills FS and returns every skill +// directory (leaf containing SKILL.md) in alphabetical order. The flat +// FS exposes each skill at path "/SKILL.md"; skillsmith would do +// the same lookup but we want discovery without depending on skillsmith's +// walking helper here. +func bundledSkillNames(src fs.FS) []string { + var out []string + entries, err := fs.ReadDir(src, ".") + if err != nil { + return out + } + for _, e := range entries { + if !e.IsDir() { + continue + } + // Confirm the directory has a SKILL.md before listing it. + if _, err := fs.Stat(src, filepath.Join(e.Name(), "SKILL.md")); err == nil { + out = append(out, e.Name()) + } + } + sort.Strings(out) + return out +}